diff --git a/.github/workflows/unit_tests.yml b/.github/workflows/unit_tests.yml index 67c9fd09..5567c9c2 100644 --- a/.github/workflows/unit_tests.yml +++ b/.github/workflows/unit_tests.yml @@ -68,6 +68,31 @@ jobs: -destination ${{ matrix.destination }} \ test-without-building + thread-sanitizer-test: + # Run the iOS unit tests with Thread Sanitizer so data races (for example on + # GIDGoogleUser's token properties) fail CI instead of shipping silently. + runs-on: macos-15 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: Select Xcode + run: sudo xcode-select -s /Applications/Xcode_16.4.app/Contents/Developer + - name: Build unit test target with Thread Sanitizer + run: | + xcodebuild \ + -scheme GoogleSignIn-Package \ + -sdk iphonesimulator \ + -destination "platform=iOS Simulator,name=iPhone 16,OS=18.6" \ + -enableThreadSanitizer YES \ + build-for-testing + - name: Run unit test target with Thread Sanitizer + run: | + xcodebuild \ + -scheme GoogleSignIn-Package \ + -sdk iphonesimulator \ + -destination "platform=iOS Simulator,name=iPhone 16,OS=18.6" \ + -enableThreadSanitizer YES \ + test-without-building + signin-sample-spm-build: runs-on: macos-15 steps: diff --git a/CHANGELOG.md b/CHANGELOG.md index 7703fa75..e490dd66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Unreleased - Fix a crash when a server error response carries a non-string value under its `error` key. The EMM error handler sent `-hasPrefix:` to whatever value was present, raising an unrecognized selector exception on a number, array or object. - Fix a custom `nonce` and requested token `claims` being dropped when a sign-in is continued after a Device Policy app restart. +- Fix a data race on `GIDGoogleUser`'s access, refresh and ID tokens. Concurrent token refreshes could previously write the three properties from different queues at once, and readers could observe a partially-updated set. # 10.0.0 - **BREAKING**: Update to AppAuth 3.0.0 and GTMAppAuth 6.0.0, which raises the minimum deployment targets to iOS 15.0 and macOS 12.0, widens the `GTMSessionFetcher` dependency to allow 4.x and 5.x, and renames the version-specific Swift Package Manager manifest to `Package@swift-5.7.swift`. Projects that must keep supporting earlier OS versions should stay on GoogleSignIn 9.2.0. ([#628](https://github.com/google/GoogleSignIn-iOS/pull/628)) diff --git a/GoogleSignIn/Sources/GIDGoogleUser.m b/GoogleSignIn/Sources/GIDGoogleUser.m index f67bb4c8..3c2b7847 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser.m +++ b/GoogleSignIn/Sources/GIDGoogleUser.m @@ -34,6 +34,8 @@ #import #endif +#import + NS_ASSUME_NONNULL_BEGIN // The ID Token claim key for the hosted domain value. @@ -61,12 +63,180 @@ @interface GIDGoogleUser () @end #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST +// An immutable snapshot of a user's tokens and the values derived from the same auth state. It is +// replaced as a whole, so readers never see a mix of old and new values. +@interface GIDGoogleUserTokens : NSObject + +@property(nonatomic, readonly) GIDToken *accessToken; +@property(nonatomic, readonly) GIDToken *refreshToken; +@property(nonatomic, readonly, nullable) GIDToken *idToken; +@property(nonatomic, readonly, nullable) NSArray *grantedScopes; +@property(nonatomic, readonly) GIDConfiguration *configuration; + +- (instancetype)initWithAccessToken:(GIDToken *)accessToken + refreshToken:(GIDToken *)refreshToken + idToken:(nullable GIDToken *)idToken + grantedScopes:(nullable NSArray *)grantedScopes + configuration:(GIDConfiguration *)configuration NS_DESIGNATED_INITIALIZER; +- (instancetype)init NS_UNAVAILABLE; + +@end + +@implementation GIDGoogleUserTokens + +- (instancetype)initWithAccessToken:(GIDToken *)accessToken + refreshToken:(GIDToken *)refreshToken + idToken:(nullable GIDToken *)idToken + grantedScopes:(nullable NSArray *)grantedScopes + configuration:(GIDConfiguration *)configuration { + self = [super init]; + if (self) { + _accessToken = accessToken; + _refreshToken = refreshToken; + _idToken = idToken; + _grantedScopes = [grantedScopes copy]; + _configuration = configuration; + } + return self; +} + +@end + +@interface GIDGoogleUser () + +// The user's current tokens. The getter and setter take `_tokenLock`, so the property is atomic. +@property(atomic, strong, nullable) GIDGoogleUserTokens *tokens; + +@end + +@interface GIDGoogleUser (Internal) + +- (void)getAccessToken:(GIDToken *_Nullable *_Nullable)accessToken + refreshToken:(GIDToken *_Nullable *_Nullable)refreshToken + idToken:(GIDToken *_Nullable *_Nullable)idToken; + +@end + +// Parses the `scope` parameter returned by the backend, which is authoritative when present. +static NSArray *_Nullable GIDGrantedScopesFromScopeString( + NSString *_Nullable scopeString) { + if (!scopeString) { + return nil; + } + // Remove leading and trailing whitespace. + scopeString = + [scopeString stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceCharacterSet]]; + // Tokenize with space as a delimiter. + NSMutableArray *parsedScopes = + [[scopeString componentsSeparatedByString:@" "] mutableCopy]; + // Remove empty strings. + [parsedScopes removeObject:@""]; + return [parsedScopes copy]; +} + +// Returns the hosted domain claim of the given ID token, if it has one. +static NSString *_Nullable GIDHostedDomainFromIDTokenString(NSString *_Nullable idTokenString) { + if (idTokenString) { + OIDIDToken *idTokenDecoded = [[OIDIDToken alloc] initWithIDTokenString:idTokenString]; + if (idTokenDecoded && idTokenDecoded.claims[kHostedDomainIDTokenClaimKey]) { + return idTokenDecoded.claims[kHostedDomainIDTokenClaimKey]; + } + } + return nil; +} + @implementation GIDGoogleUser { - GIDConfiguration *_cachedConfiguration; - // A queue for pending token refresh handlers so we don't fire multiple requests in parallel. // Access to this ivar should be synchronized. NSMutableArray *_tokenRefreshHandlerQueue; + + GIDGoogleUserTokens *_tokens; + + // Guards `_tokens` and `_profile`. It is only ever held for a single read or write of those + // ivars, never while calling out to other code. + os_unfair_lock _tokenLock; + + // Guards `authState` updates and reads that span several of its fields. Recursive because + // AppAuth calls -didChangeState: synchronously while it is held, and token KVO observers may + // re-enter (see -testTokenObserver_reentersUserDuringUpdate). Take it via -lockAuthState. + NSRecursiveLock *_authStateLock; +} + +// `profile` is readonly and its getter below is hand-written, which turns off autosynthesis, so +// the backing ivar is synthesized explicitly. +@synthesize profile = _profile; + +// Lock order: `_authStateLock`, then `_tokenLock`. +- (void)lockAuthState { + os_unfair_lock_assert_not_owner(&_tokenLock); + [_authStateLock lock]; +} + +- (void)unlockAuthState { + [_authStateLock unlock]; +} + +- (nullable GIDGoogleUserTokens *)tokens { + os_unfair_lock_lock(&_tokenLock); + GIDGoogleUserTokens *tokens = _tokens; + os_unfair_lock_unlock(&_tokenLock); + return tokens; +} + +- (void)setTokens:(nullable GIDGoogleUserTokens *)tokens { + os_unfair_lock_lock(&_tokenLock); + _tokens = tokens; + os_unfair_lock_unlock(&_tokenLock); +} + +- (GIDToken *)accessToken { + return self.tokens.accessToken; +} + +- (GIDToken *)refreshToken { + return self.tokens.refreshToken; +} + +- (nullable GIDToken *)idToken { + return self.tokens.idToken; +} + +- (nullable GIDProfileData *)profile { + os_unfair_lock_lock(&_tokenLock); + GIDProfileData *profile = _profile; + os_unfair_lock_unlock(&_tokenLock); + return profile; +} + +// The token properties are derived from `tokens`, so KVO observers of each one are notified +// whenever `tokens` is replaced. ++ (NSSet *)keyPathsForValuesAffectingAccessToken { + return [NSSet setWithObject:NSStringFromSelector(@selector(tokens))]; +} + ++ (NSSet *)keyPathsForValuesAffectingRefreshToken { + return [NSSet setWithObject:NSStringFromSelector(@selector(tokens))]; +} + ++ (NSSet *)keyPathsForValuesAffectingIdToken { + return [NSSet setWithObject:NSStringFromSelector(@selector(tokens))]; +} + +- (void)getAccessToken:(GIDToken *_Nullable *_Nullable)accessToken + refreshToken:(GIDToken *_Nullable *_Nullable)refreshToken + idToken:(GIDToken *_Nullable *_Nullable)idToken { + // A single read of `tokens` gives a consistent snapshot of all three. + GIDGoogleUserTokens *tokens = self.tokens; + + if (accessToken) { + *accessToken = tokens.accessToken; + } + if (refreshToken) { + *refreshToken = tokens.refreshToken; + } + if (idToken) { + *idToken = tokens.idToken; + } } - (nullable NSString *)userID { @@ -81,51 +251,27 @@ - (nullable NSString *)userID { } - (nullable NSArray *)grantedScopes { - NSArray *grantedScopes; - NSString *grantedScopeString = self.authState.lastTokenResponse.scope; - if (grantedScopeString) { - // If we have a 'scope' parameter from the backend, this is authoritative. - // Remove leading and trailing whitespace. - grantedScopeString = [grantedScopeString stringByTrimmingCharactersInSet: - [NSCharacterSet whitespaceCharacterSet]]; - // Tokenize with space as a delimiter. - NSMutableArray *parsedScopes = - [[grantedScopeString componentsSeparatedByString:@" "] mutableCopy]; - // Remove empty strings. - [parsedScopes removeObject:@""]; - grantedScopes = [parsedScopes copy]; - } - return grantedScopes; + return self.tokens.grantedScopes; } - (GIDConfiguration *)configuration { - @synchronized(self) { - // Caches the configuration since it would not change for one GIDGoogleUser instance. - if (!_cachedConfiguration) { - NSString *clientID = self.authState.lastAuthorizationResponse.request.clientID; - NSString *serverClientID = - self.authState.lastTokenResponse.request.additionalParameters[kAudienceParameter]; - NSString *openIDRealm = - self.authState.lastTokenResponse.request.additionalParameters[kOpenIDRealmParameter]; - - _cachedConfiguration = [[GIDConfiguration alloc] initWithClientID:clientID - serverClientID:serverClientID - hostedDomain:[self hostedDomain] - openIDRealm:openIDRealm]; - }; - } - return _cachedConfiguration; + return self.tokens.configuration; } - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion { - if (!([self.accessToken.expirationDate timeIntervalSinceNow] < kMinimalTimeToExpire || - (self.idToken && [self.idToken.expirationDate timeIntervalSinceNow] < kMinimalTimeToExpire))) { + GIDToken *accessToken; + GIDToken *refreshToken; + GIDToken *idToken; + [self getAccessToken:&accessToken refreshToken:&refreshToken idToken:&idToken]; + + if (!([accessToken.expirationDate timeIntervalSinceNow] < kMinimalTimeToExpire || + (idToken && [idToken.expirationDate timeIntervalSinceNow] < kMinimalTimeToExpire))) { dispatch_async(dispatch_get_main_queue(), ^{ completion(self, nil); }); return; } - if (self.refreshToken.expirationDate && [self.refreshToken.expirationDate timeIntervalSinceNow] <= 0) { + if (refreshToken.expirationDate && [refreshToken.expirationDate timeIntervalSinceNow] <= 0) { NSError *error = [NSError errorWithDomain:kGIDSignInErrorDomain code:kGIDSignInErrorCodeRefreshTokenExpired userInfo:nil]; @@ -145,6 +291,9 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion } // This is the first handler in the queue, a fetch is needed. NSMutableDictionary *additionalParameters = [@{} mutableCopy]; + // Read the auth state under `_authStateLock` so building the request cannot interleave with + // -updateWithTokenResponse:authorizationResponse:profileData:. + [self lockAuthState]; #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST [additionalParameters addEntriesFromDictionary: [GIDEMMSupport updatedEMMParametersWithParameters: @@ -155,12 +304,18 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST [additionalParameters addEntriesFromDictionary:[GIDSignInPreferences loggingParameters]]; + OIDAuthorizationResponse *authorizationResponse = self.authState.lastAuthorizationResponse; OIDTokenRequest *tokenRefreshRequest = [self.authState tokenRefreshRequestWithAdditionalParameters:additionalParameters]; + [self unlockAuthState]; + [OIDAuthorizationService performTokenRequest:tokenRefreshRequest - originalAuthorizationResponse:self.authState.lastAuthorizationResponse + originalAuthorizationResponse:authorizationResponse callback:^(OIDTokenResponse *_Nullable tokenResponse, NSError *_Nullable error) { + // Update the auth state under `_authStateLock` so this refresh cannot interleave with + // -updateWithTokenResponse:authorizationResponse:profileData:. + [self lockAuthState]; if (tokenResponse) { [self.authState updateWithTokenResponse:tokenResponse error:nil]; } else { @@ -168,6 +323,7 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion [self.authState updateWithAuthorizationError:error]; } } + [self unlockAuthState]; #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST [GIDEMMSupport handleTokenFetchEMMError:error completion:^(NSError *_Nullable error) { // Process the handler queue to call back. @@ -234,8 +390,11 @@ - (void)addScopes:(NSArray *)scopes #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST - (nullable NSString *)emmSupport { - return self.authState.lastAuthorizationResponse + [self lockAuthState]; + NSString *emmSupport = self.authState.lastAuthorizationResponse .request.additionalParameters[kEMMSupportParameterName]; + [self unlockAuthState]; + return emmSupport; } #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST @@ -243,6 +402,10 @@ - (instancetype)initWithAuthState:(OIDAuthState *)authState profileData:(nullable GIDProfileData *)profileData { self = [super init]; if (self) { + // Initialize the locks before -updateTokensWithAuthState: below uses them. + _tokenLock = OS_UNFAIR_LOCK_INIT; + _authStateLock = [[NSRecursiveLock alloc] init]; + _tokenRefreshHandlerQueue = [[NSMutableArray alloc] init]; _profile = profileData; @@ -262,28 +425,30 @@ - (instancetype)initWithAuthState:(OIDAuthState *)authState - (void)updateWithTokenResponse:(OIDTokenResponse *)tokenResponse authorizationResponse:(OIDAuthorizationResponse *)authorizationResponse profileData:(nullable GIDProfileData *)profileData { - @synchronized(self) { - _profile = profileData; - - // We don't want to trigger the delegate before we update authState completely. So we unset the - // delegate before the first update. Also the order of updates is important because - // `updateWithAuthorizationResponse` would clear the last token reponse and refresh token. - // TODO: Rewrite authState update logic when the issue is addressed.(openid/AppAuth-iOS#728) - self.authState.stateChangeDelegate = nil; - [self.authState updateWithAuthorizationResponse:authorizationResponse error:nil]; - self.authState.stateChangeDelegate = self; - [self.authState updateWithTokenResponse:tokenResponse error:nil]; - } + [self lockAuthState]; + os_unfair_lock_lock(&_tokenLock); + _profile = profileData; + os_unfair_lock_unlock(&_tokenLock); + + // We don't want to trigger the delegate before we update authState completely. So we unset the + // delegate before the first update. Also the order of updates is important because + // `updateWithAuthorizationResponse` would clear the last token reponse and refresh token. + // TODO: Rewrite authState update logic when the issue is addressed.(openid/AppAuth-iOS#728) + self.authState.stateChangeDelegate = nil; + [self.authState updateWithAuthorizationResponse:authorizationResponse error:nil]; + self.authState.stateChangeDelegate = self; + [self.authState updateWithTokenResponse:tokenResponse error:nil]; + [self unlockAuthState]; } - (void)updateTokensWithAuthState:(OIDAuthState *)authState { + [self lockAuthState]; + GIDGoogleUserTokens *current = self.tokens; + GIDToken *accessToken = [[GIDToken alloc] initWithTokenString:authState.lastTokenResponse.accessToken expirationDate:authState.lastTokenResponse.accessTokenExpirationDate]; - if (![self.accessToken isEqualToToken:accessToken]) { - self.accessToken = accessToken; - } - + NSDictionary *additionalParameters = authState.lastTokenResponse.additionalParameters; NSNumber *refreshTokenExpiresIn = nil; NSDate *refreshTokenExpirationDate = nil; @@ -295,10 +460,7 @@ - (void)updateTokensWithAuthState:(OIDAuthState *)authState { } GIDToken *refreshToken = [[GIDToken alloc] initWithTokenString:authState.refreshToken expirationDate:refreshTokenExpirationDate]; - if (![self.refreshToken isEqualToToken:refreshToken]) { - self.refreshToken = refreshToken; - } - + GIDToken *idToken; NSString *idTokenString = authState.lastTokenResponse.idToken; if (idTokenString) { @@ -309,22 +471,53 @@ - (void)updateTokensWithAuthState:(OIDAuthState *)authState { } else { idToken = nil; } - if ((self.idToken || idToken) && ![self.idToken isEqualToToken:idToken]) { - self.idToken = idToken; + + NSArray *grantedScopes = + GIDGrantedScopesFromScopeString(authState.lastTokenResponse.scope); + + GIDConfiguration *configuration; + if (current.configuration) { + // The configuration is fixed for the lifetime of a user, so it is computed once. + configuration = current.configuration; + } else { + NSString *clientID = authState.lastAuthorizationResponse.request.clientID; + NSString *serverClientID = + authState.lastTokenResponse.request.additionalParameters[kAudienceParameter]; + NSString *openIDRealm = + authState.lastTokenResponse.request.additionalParameters[kOpenIDRealmParameter]; + + configuration = [[GIDConfiguration alloc] + initWithClientID:clientID + serverClientID:serverClientID + hostedDomain:GIDHostedDomainFromIDTokenString(idToken.tokenString) + openIDRealm:openIDRealm]; } -} -#pragma mark - Helpers + // Keep the existing token objects when they are unchanged, so an update that changes nothing + // leaves `tokens` untouched and sends no KVO notifications. + if ([current.accessToken isEqualToToken:accessToken]) { + accessToken = current.accessToken; + } + if ([current.refreshToken isEqualToToken:refreshToken]) { + refreshToken = current.refreshToken; + } + if ([current.idToken isEqualToToken:idToken]) { + idToken = current.idToken; + } -- (nullable NSString *)hostedDomain { - NSString *idTokenString = self.idToken.tokenString; - if (idTokenString) { - OIDIDToken *idTokenDecoded = [[OIDIDToken alloc] initWithIDTokenString:idTokenString]; - if (idTokenDecoded && idTokenDecoded.claims[kHostedDomainIDTokenClaimKey]) { - return idTokenDecoded.claims[kHostedDomainIDTokenClaimKey]; - } + BOOL grantedScopesChanged = !((!grantedScopes && !current.grantedScopes) || + [grantedScopes isEqualToArray:current.grantedScopes]); + + if (!current || accessToken != current.accessToken || + refreshToken != current.refreshToken || idToken != current.idToken || + grantedScopesChanged) { + self.tokens = [[GIDGoogleUserTokens alloc] initWithAccessToken:accessToken + refreshToken:refreshToken + idToken:idToken + grantedScopes:grantedScopes + configuration:configuration]; } - return nil; + [self unlockAuthState]; } #pragma mark - OIDAuthStateChangeDelegate @@ -360,8 +553,11 @@ - (nullable instancetype)initWithCoder:(NSCoder *)decoder { } - (void)encodeWithCoder:(NSCoder *)encoder { - [encoder encodeObject:_profile forKey:kProfileDataKey]; + // Holds `_authStateLock` so the encoded profile and auth state come from the same update. + [self lockAuthState]; + [encoder encodeObject:self.profile forKey:kProfileDataKey]; [encoder encodeObject:self.authState forKey:kAuthStateKey]; + [self unlockAuthState]; } @end diff --git a/GoogleSignIn/Sources/GIDGoogleUser_Private.h b/GoogleSignIn/Sources/GIDGoogleUser_Private.h index f07a1045..b08a2069 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser_Private.h +++ b/GoogleSignIn/Sources/GIDGoogleUser_Private.h @@ -32,13 +32,10 @@ typedef void (^GIDGoogleUserCompletion)(GIDGoogleUser *_Nullable user, NSError * /// Internal methods for the class that are not part of the public API. @interface GIDGoogleUser () -@property(nonatomic, readwrite) GIDToken *accessToken; - -@property(nonatomic, readwrite) GIDToken *refreshToken; - -@property(nonatomic, readwrite, nullable) GIDToken *idToken; - /// A representation of the state of the OAuth session for this instance. +// TODO: Reads through this property bypass the lock GIDGoogleUser takes around its own auth +// state updates, and apps can reach the same object through `fetcherAuthorizer`. Fixing this +// may need a public API change. @property(nonatomic, readonly) OIDAuthState *authState; #pragma clang diagnostic push diff --git a/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m b/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m index a6a5788c..5ab7b6f7 100644 --- a/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m +++ b/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m @@ -53,6 +53,34 @@ #import #endif +@interface GIDGoogleUser () + +- (void)getAccessToken:(GIDToken *_Nullable *_Nullable)accessToken + refreshToken:(GIDToken *_Nullable *_Nullable)refreshToken + idToken:(GIDToken *_Nullable *_Nullable)idToken; + +@end + +// Observer that runs `onChange` whenever an observed key path changes. +@interface GIDGoogleUserTestKVOObserver : NSObject + +@property(nonatomic, copy) void (^onChange)(void); + +@end + +@implementation GIDGoogleUserTestKVOObserver + +- (void)observeValueForKeyPath:(NSString *)keyPath + ofObject:(id)object + change:(NSDictionary *)change + context:(void *)context { + if (self.onChange) { + self.onChange(); + } +} + +@end + static NSString *const kNewAccessToken = @"new_access_token"; static NSString *const kNewRefreshToken = @"new_refresh_token"; @@ -222,6 +250,222 @@ - (void)testUpdateAuthState_tokensAreNotChanged { XCTAssertIdentical(user.refreshToken, refreshTokenBeforeUpdate); } +- (void)testUpdateTokens_concurrentUpdates_leaveConsistentTokenSet { + GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:kAccessTokenExpiresIn + idTokenExpiresIn:kIDTokenExpiresIn]; + + NSString *idTokenA = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn]; + NSString *accessTokenA = @"access_token_A"; + OIDAuthState *authStateA = [OIDAuthState testInstanceWithIDToken:idTokenA + accessToken:accessTokenA + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:kNewRefreshToken]; + + NSString *idTokenB = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn + 1]; + NSString *accessTokenB = @"access_token_B"; + OIDAuthState *authStateB = [OIDAuthState testInstanceWithIDToken:idTokenB + accessToken:accessTokenB + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:kNewRefreshToken]; + + XCTestExpectation *updateExpectation = [self expectationWithDescription:@"Updates finished"]; + XCTestExpectation *readExpectation = [self expectationWithDescription:@"Reads finished"]; + + dispatch_queue_t updateQueue = dispatch_queue_create("com.google.gidgoogleuser.testUpdateTokens.update", + DISPATCH_QUEUE_CONCURRENT); + dispatch_queue_t readQueue = dispatch_queue_create("com.google.gidgoogleuser.testUpdateTokens.read", + DISPATCH_QUEUE_CONCURRENT); + + NSInteger iterations = 200; + + // Concurrent updates + dispatch_async(updateQueue, ^{ + dispatch_apply(iterations, updateQueue, ^(size_t i) { + OIDAuthState *state = (i % 2 == 0) ? authStateA : authStateB; + [user updateWithTokenResponse:state.lastTokenResponse + authorizationResponse:state.lastAuthorizationResponse + profileData:nil]; + }); + [updateExpectation fulfill]; + }); + + // Concurrent reads + dispatch_async(readQueue, ^{ + dispatch_apply(iterations, readQueue, ^(size_t i) { + GIDToken *accessToken = nil; + GIDToken *refreshToken = nil; + GIDToken *idToken = nil; + [user getAccessToken:&accessToken refreshToken:&refreshToken idToken:&idToken]; + + // Consistency check: accessToken and idToken must both come from A or both from B. + // The consistency guarantee comes from the snapshot accessor's single lock acquisition. + // Reading the three properties individually would NOT be atomic even with the per-accessor + // locking, by design. + if ([accessToken.tokenString isEqualToString:accessTokenA]) { + XCTAssertEqualObjects(idToken.tokenString, idTokenA); + XCTAssertEqualObjects(refreshToken.tokenString, kNewRefreshToken); + } else if ([accessToken.tokenString isEqualToString:accessTokenB]) { + XCTAssertEqualObjects(idToken.tokenString, idTokenB); + XCTAssertEqualObjects(refreshToken.tokenString, kNewRefreshToken); + } + }); + [readExpectation fulfill]; + }); + + [self waitForExpectationsWithTimeout:5 handler:nil]; + + // Final state should be either A or B (whichever ran last) + BOOL matchesA = [user.accessToken.tokenString isEqualToString:accessTokenA] && + [user.idToken.tokenString isEqualToString:idTokenA]; + BOOL matchesB = [user.accessToken.tokenString isEqualToString:accessTokenB] && + [user.idToken.tokenString isEqualToString:idTokenB]; + XCTAssertTrue(matchesA || matchesB); + + // This test is a reliable failure detector only under Thread Sanitizer. + // The consistency assertion above is what gives it meaning without TSan. +} + +- (void)testRefreshTokensIfNeeded_readsConsistentSnapshot { + // Access token expired 10 seconds ago. ID token will expire in 10 minutes. + // This matches the shape in testRefreshTokensIfNeededWithCompletion_refresh_givenAccessTokenExpired. + GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:-10 idTokenExpiresIn:10 * 60]; + + XCTestExpectation *expectation = [self expectationWithDescription:@"Callback is called"]; + + // Call -refreshTokensIfNeededWithCompletion: and assert the completion fires with the user and no + // error, i.e. the decision path is unchanged by the snapshot refactor. + [user refreshTokensIfNeededWithCompletion:^(GIDGoogleUser *_Nullable user, NSError *_Nullable error) { + [expectation fulfill]; + XCTAssertNotNil(user); + XCTAssertNil(error); + }]; + + // We need to provide a response because the access token is expired, so it WILL attempt a refresh. + NSString *newIdToken = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn]; + OIDTokenResponse *fakeResponse = [OIDTokenResponse testInstanceWithIDToken:newIdToken + accessToken:kNewAccessToken + expiresIn:@(kAccessTokenExpiresIn) + refreshToken:kRefreshToken + tokenRequest:nil]; + _tokenFetchHandler(fakeResponse, nil); + + [self waitForExpectationsWithTimeout:1 handler:nil]; + + // This is a guard against the snapshot refactor changing the refresh decision, not a race test. +} + +// Races concurrent token updates against plain reads of the three public token properties, which +// is how apps read them. Without Thread Sanitizer this only checks that nothing crashes; under +// Thread Sanitizer (`-enableThreadSanitizer YES`) it fails if any of the three properties is read +// without the lock that `-updateTokensWithAuthState:` writes them under. +- (void)testTokenProperties_concurrentUpdatesAndReads { + GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:kAccessTokenExpiresIn + idTokenExpiresIn:kIDTokenExpiresIn]; + + NSString *idTokenA = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn]; + OIDAuthState *authStateA = [OIDAuthState testInstanceWithIDToken:idTokenA + accessToken:@"access_token_A" + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:kNewRefreshToken]; + + NSString *idTokenB = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn + 1]; + OIDAuthState *authStateB = [OIDAuthState testInstanceWithIDToken:idTokenB + accessToken:@"access_token_B" + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:kNewRefreshToken]; + + XCTestExpectation *updateExpectation = [self expectationWithDescription:@"Updates finished"]; + XCTestExpectation *readExpectation = [self expectationWithDescription:@"Reads finished"]; + + dispatch_queue_t updateQueue = + dispatch_queue_create("com.google.gidgoogleuser.testTokenProperties.update", + DISPATCH_QUEUE_CONCURRENT); + dispatch_queue_t readQueue = + dispatch_queue_create("com.google.gidgoogleuser.testTokenProperties.read", + DISPATCH_QUEUE_CONCURRENT); + + size_t iterations = 2000; + + dispatch_async(updateQueue, ^{ + dispatch_apply(iterations, updateQueue, ^(size_t i) { + OIDAuthState *state = (i % 2 == 0) ? authStateA : authStateB; + [user updateWithTokenResponse:state.lastTokenResponse + authorizationResponse:state.lastAuthorizationResponse + profileData:nil]; + }); + [updateExpectation fulfill]; + }); + + dispatch_async(readQueue, ^{ + dispatch_apply(iterations, readQueue, ^(size_t i) { + (void)user.accessToken.tokenString; + (void)user.refreshToken.tokenString; + (void)user.idToken.tokenString; + }); + [readExpectation fulfill]; + }); + + [self waitForExpectationsWithTimeout:30 handler:nil]; + + XCTAssertNotNil(user.accessToken); +} + +// KVO observers of the token properties run while the user holds its auth state lock. This checks +// that an observer can call back into the user on the same thread, including starting another +// token update, without deadlocking or aborting. +- (void)testTokenObserver_reentersUserDuringUpdate { + GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:kAccessTokenExpiresIn + idTokenExpiresIn:kIDTokenExpiresIn]; + + NSString *idTokenA = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn]; + OIDAuthState *authStateA = [OIDAuthState testInstanceWithIDToken:idTokenA + accessToken:@"access_token_A" + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:kNewRefreshToken]; + + NSString *idTokenB = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn + 1]; + OIDAuthState *authStateB = [OIDAuthState testInstanceWithIDToken:idTokenB + accessToken:@"access_token_B" + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:kNewRefreshToken]; + + NSString *accessTokenKeyPath = NSStringFromSelector(@selector(accessToken)); + GIDGoogleUserTestKVOObserver *observer = [[GIDGoogleUserTestKVOObserver alloc] init]; + __block NSInteger notificationCount = 0; + __weak GIDGoogleUser *weakUser = user; + + observer.onChange = ^{ + notificationCount += 1; + // Only the first notification re-enters, so the re-entrant update cannot recurse forever. + if (notificationCount > 1) { + return; + } + GIDGoogleUser *strongUser = weakUser; + + // These reads take the user's auth state lock again on this thread. + (void)strongUser.grantedScopes; + (void)strongUser.profile; + XCTAssertNotNil(strongUser.configuration); + + // A re-entrant token update from inside the notification. + [strongUser updateWithTokenResponse:authStateB.lastTokenResponse + authorizationResponse:authStateB.lastAuthorizationResponse + profileData:nil]; + }; + + [user addObserver:observer forKeyPath:accessTokenKeyPath options:0 context:NULL]; + + [user updateWithTokenResponse:authStateA.lastTokenResponse + authorizationResponse:authStateA.lastAuthorizationResponse + profileData:nil]; + + [user removeObserver:observer forKeyPath:accessTokenKeyPath context:NULL]; + + // Both the outer update and the re-entrant one notify, and the re-entrant one is applied last. + XCTAssertGreaterThanOrEqual(notificationCount, 2); + XCTAssertEqualObjects(user.accessToken.tokenString, @"access_token_B"); +} + - (void)testFetcherAuthorizer { // This is really hard to test without assuming how GTMAppAuthFetcherAuthorization works // internally, so let's just take the shortcut here by asserting we get a diff --git a/GoogleSignIn/Tests/Unit/GIDSignInTest.m b/GoogleSignIn/Tests/Unit/GIDSignInTest.m index ed959f12..574ea36c 100644 --- a/GoogleSignIn/Tests/Unit/GIDSignInTest.m +++ b/GoogleSignIn/Tests/Unit/GIDSignInTest.m @@ -532,7 +532,8 @@ - (void)testRestorePreviousSignInNoRefresh_hasPreviousUser { OCMStub([idTokenDecoded initWithIDTokenString:OCMOCK_ANY]).andReturn(idTokenDecoded); OCMStub([idTokenDecoded subject]).andReturn(kFakeGaiaID); - // Mock generating a GIDConfiguration when initializing GIDGoogleUser. + // Mock generating the token snapshot (tokens, granted scopes and GIDConfiguration) when + // initializing GIDGoogleUser. OIDAuthorizationResponse *authResponse = [OIDAuthorizationResponse testInstance]; @@ -542,6 +543,7 @@ - (void)testRestorePreviousSignInNoRefresh_hasPreviousUser { OCMStub([_tokenRequest additionalParameters]).andReturn(nil); OCMStub([_tokenResponse accessToken]).andReturn(kAccessToken); OCMStub([_tokenResponse accessTokenExpirationDate]).andReturn(nil); + OCMStub([_tokenResponse scope]).andReturn(nil); [_signIn restorePreviousSignInNoRefresh];