diff --git a/GoogleSignIn/Sources/GIDGoogleUser.m b/GoogleSignIn/Sources/GIDGoogleUser.m index 11f36149..6cd7e47a 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser.m +++ b/GoogleSignIn/Sources/GIDGoogleUser.m @@ -127,6 +127,14 @@ @implementation GIDGoogleUser { // from no profile having been supplied, which keeps the current one. GIDProfileData *_pendingProfile; BOOL _hasPendingProfile; + + // A private object to `@synchronized` on, guarding `authState` updates and reads. Private so + // that callers cannot contend on the user object's own monitor. `@synchronized` is recursive, + // which a) synchronous AppAuth -didChangeState: calls and b) KVO observers of the token / + // profile properties rely on, and it releases the lock if an observer throws. + // Observers must not synchronously wait on another thread that is updating the same user! It + // would deadlock. + NSObject *_authStateLock; } - (GIDToken *)accessToken { @@ -215,28 +223,47 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion } } // This is the first handler in the queue, a fetch is needed. + // Read the auth state under `_authStateLock` so the reads cannot interleave with token + // response updates. Only the reads are locked: building the parameters can block (the EMM + // helper may wait on a keychain probe), so it runs outside the lock. + NSDictionary *lastTokenAdditionalParameters; + @synchronized(_authStateLock) { + lastTokenAdditionalParameters = self.authState.lastTokenResponse.request.additionalParameters; + } + NSMutableDictionary *additionalParameters = [@{} mutableCopy]; #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST [additionalParameters addEntriesFromDictionary: - [GIDEMMSupport updatedEMMParametersWithParameters: - self.authState.lastTokenResponse.request.additionalParameters]]; + [GIDEMMSupport updatedEMMParametersWithParameters:lastTokenAdditionalParameters]]; #elif TARGET_OS_OSX || TARGET_OS_MACCATALYST - [additionalParameters addEntriesFromDictionary: - self.authState.lastTokenResponse.request.additionalParameters]; + [additionalParameters addEntriesFromDictionary:lastTokenAdditionalParameters]; #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST [additionalParameters addEntriesFromDictionary:[GIDSignInPreferences loggingParameters]]; - OIDTokenRequest *tokenRefreshRequest = - [self.authState tokenRefreshRequestWithAdditionalParameters:additionalParameters]; + // Read the authorization response and build the request in one lock window so they come + // from the same auth state. `tokenRefreshRequestWithAdditionalParameters:` does not call back + // into this object. + OIDAuthorizationResponse *authorizationResponse; + OIDTokenRequest *tokenRefreshRequest; + @synchronized(_authStateLock) { + authorizationResponse = self.authState.lastAuthorizationResponse; + tokenRefreshRequest = + [self.authState tokenRefreshRequestWithAdditionalParameters:additionalParameters]; + } + [OIDAuthorizationService performTokenRequest:tokenRefreshRequest - originalAuthorizationResponse:self.authState.lastAuthorizationResponse + originalAuthorizationResponse:authorizationResponse callback:^(OIDTokenResponse *_Nullable tokenResponse, NSError *_Nullable error) { - if (tokenResponse) { - [self.authState updateWithTokenResponse:tokenResponse error:nil]; - } else { - if (error.domain == OIDOAuthTokenErrorDomain) { - [self.authState updateWithAuthorizationError:error]; + // Update the auth state under `_authStateLock` so this refresh cannot interleave with + // -updateWithTokenResponse:authorizationResponse:profileData:. + @synchronized(self->_authStateLock) { + if (tokenResponse) { + [self.authState updateWithTokenResponse:tokenResponse error:nil]; + } else { + if (error.domain == OIDOAuthTokenErrorDomain) { + [self.authState updateWithAuthorizationError:error]; + } } } #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST @@ -305,8 +332,10 @@ - (void)addScopes:(NSArray *)scopes #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST - (nullable NSString *)emmSupport { - return self.authState.lastAuthorizationResponse - .request.additionalParameters[kEMMSupportParameterName]; + @synchronized(_authStateLock) { + return self.authState.lastAuthorizationResponse + .request.additionalParameters[kEMMSupportParameterName]; + } } #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST @@ -314,6 +343,9 @@ - (instancetype)initWithAuthState:(OIDAuthState *)authState profileData:(nullable GIDProfileData *)profileData { self = [super init]; if (self) { + // Create the lock object before -updateTokensWithAuthState: below synchronizes on it. + _authStateLock = [[NSObject alloc] init]; + _tokenRefreshHandlerQueue = [[NSMutableArray alloc] init]; // Park the profile for the first snapshot, built by -updateTokensWithAuthState: below. @@ -336,7 +368,7 @@ - (instancetype)initWithAuthState:(OIDAuthState *)authState - (void)updateWithTokenResponse:(OIDTokenResponse *)tokenResponse authorizationResponse:(OIDAuthorizationResponse *)authorizationResponse profileData:(nullable GIDProfileData *)profileData { - @synchronized(self) { + @synchronized(_authStateLock) { // Park the profile for the snapshot built when the token response update below calls back // through -didChangeState:. _pendingProfile = profileData; @@ -359,7 +391,7 @@ - (void)updateWithTokenResponse:(OIDTokenResponse *)tokenResponse } - (void)updateTokensWithAuthState:(OIDAuthState *)authState { - @synchronized(self) { + @synchronized(_authStateLock) { GIDGoogleUserTokens *current = self.tokens; GIDProfileData *profile = _hasPendingProfile ? _pendingProfile : current.profile; _pendingProfile = nil; @@ -480,8 +512,11 @@ - (nullable instancetype)initWithCoder:(NSCoder *)decoder { } - (void)encodeWithCoder:(NSCoder *)encoder { - [encoder encodeObject:self.profile forKey:kProfileDataKey]; - [encoder encodeObject:self.authState forKey:kAuthStateKey]; + // Holds `_authStateLock` so the encoded profile and auth state come from the same update. + @synchronized(_authStateLock) { + [encoder encodeObject:self.profile forKey:kProfileDataKey]; + [encoder encodeObject:self.authState forKey:kAuthStateKey]; + } } @end diff --git a/GoogleSignIn/Sources/GIDGoogleUser_Private.h b/GoogleSignIn/Sources/GIDGoogleUser_Private.h index 343f4baf..61cfe9aa 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser_Private.h +++ b/GoogleSignIn/Sources/GIDGoogleUser_Private.h @@ -58,11 +58,14 @@ typedef void (^GIDGoogleUserCompletion)(GIDGoogleUser *_Nullable user, NSError * @interface GIDGoogleUser () /// 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. Tracked in https://github.com/google/GoogleSignIn-iOS/issues/648. @property(nonatomic, readonly) OIDAuthState *authState; /// The user's current tokens, granted scopes, configuration and profile. Read once - accessing /// individual properties in sequence is not recommended. Reading once ensures that each property is -/// from the same update. Writes are serialized by `@synchronized(self)`. +/// from the same update. Writes are serialized by `_authStateLock`. @property(atomic, strong, nullable) GIDGoogleUserTokens *tokens; #pragma clang diagnostic push diff --git a/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m b/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m index 5a2b79a9..9566a271 100644 --- a/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m +++ b/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m @@ -556,7 +556,7 @@ - (void)testTokenProperties_concurrentUpdatesAndReads { XCTAssertNotNil(user.accessToken); } -// KVO observers of the token properties run while the user holds its lock. This checks +// 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 { @@ -896,6 +896,132 @@ - (void)testRefreshTokensIfNeededWithCompletion_handleConcurrentRefresh { [self waitForExpectationsWithTimeout:1 handler:nil]; } +// Races the token refresh callback (the response AppAuth delivers for a request made by +// -refreshTokensIfNeededWithCompletion:) against -updateWithTokenResponse:authorizationResponse: +// profileData: and snapshot reads. The callback applies state C; the sign-in updates apply states +// A and B. All three updates take `_authStateLock`, so every snapshot must come from one state. +- (void)testRefreshTokensIfNeeded_concurrentSignInUpdates_leaveConsistentTokenSet { + // The access token expired 10 seconds ago, so the refresh performs a fetch. + GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:-10 + idTokenExpiresIn:kIDTokenExpiresIn]; + NSString *initialAccessToken = user.accessToken.tokenString; + NSString *initialRefreshToken = user.refreshToken.tokenString; + NSString *initialIDToken = user.idToken.tokenString; + + NSString *idTokenA = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn]; + NSString *accessTokenA = @"access_token_A"; + NSString *refreshTokenA = @"refresh_token_A"; + OIDAuthState *authStateA = [OIDAuthState testInstanceWithIDToken:idTokenA + accessToken:accessTokenA + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:refreshTokenA]; + + NSString *idTokenB = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn + 1]; + NSString *accessTokenB = @"access_token_B"; + NSString *refreshTokenB = @"refresh_token_B"; + OIDAuthState *authStateB = [OIDAuthState testInstanceWithIDToken:idTokenB + accessToken:accessTokenB + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:refreshTokenB]; + + NSString *idTokenC = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn + 2]; + NSString *accessTokenC = @"access_token_C"; + NSString *refreshTokenC = @"refresh_token_C"; + OIDTokenResponse *refreshResponse = + [OIDTokenResponse testInstanceWithIDToken:idTokenC + accessToken:accessTokenC + expiresIn:@(kAccessTokenExpiresIn) + refreshToken:refreshTokenC + tokenRequest:nil]; + + XCTestExpectation *completionExpectation = + [self expectationWithDescription:@"Refresh completion is called"]; + [user refreshTokensIfNeededWithCompletion:^(GIDGoogleUser *_Nullable refreshedUser, + NSError *_Nullable error) { + [completionExpectation fulfill]; + }]; + XCTAssertNotNil(_tokenFetchHandler); + OIDTokenCallback tokenFetchHandler = _tokenFetchHandler; + + XCTestExpectation *refreshExpectation = [self expectationWithDescription:@"Refreshes finished"]; + XCTestExpectation *updateExpectation = [self expectationWithDescription:@"Updates finished"]; + XCTestExpectation *readExpectation = [self expectationWithDescription:@"Reads finished"]; + + dispatch_queue_t refreshQueue = + dispatch_queue_create("com.google.gidgoogleuser.testRefreshTokensIfNeeded.refresh", + DISPATCH_QUEUE_CONCURRENT); + dispatch_queue_t updateQueue = + dispatch_queue_create("com.google.gidgoogleuser.testRefreshTokensIfNeeded.update", + DISPATCH_QUEUE_CONCURRENT); + dispatch_queue_t readQueue = + dispatch_queue_create("com.google.gidgoogleuser.testRefreshTokensIfNeeded.read", + DISPATCH_QUEUE_CONCURRENT); + + NSInteger iterations = 200; + + // Concurrent refresh responses. Delivering the same response repeatedly is harmless: the first + // delivery drains the completion queue and the rest only update the auth state again. + dispatch_async(refreshQueue, ^{ + dispatch_apply(iterations, refreshQueue, ^(size_t i) { + tokenFetchHandler(refreshResponse, nil); + }); + [refreshExpectation fulfill]; + }); + + // Concurrent sign-in 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) { + GIDGoogleUserTokens *tokens = user.tokens; + + // Consistency check: the three tokens come from one snapshot read, so all three tokens + // (access, refresh and ID) must come from the same state (initial, A, B, or C). + if ([tokens.accessToken.tokenString isEqualToString:accessTokenA]) { + XCTAssertEqualObjects(tokens.idToken.tokenString, idTokenA); + XCTAssertEqualObjects(tokens.refreshToken.tokenString, refreshTokenA); + } else if ([tokens.accessToken.tokenString isEqualToString:accessTokenB]) { + XCTAssertEqualObjects(tokens.idToken.tokenString, idTokenB); + XCTAssertEqualObjects(tokens.refreshToken.tokenString, refreshTokenB); + } else if ([tokens.accessToken.tokenString isEqualToString:accessTokenC]) { + XCTAssertEqualObjects(tokens.idToken.tokenString, idTokenC); + XCTAssertEqualObjects(tokens.refreshToken.tokenString, refreshTokenC); + } else { + XCTAssertEqualObjects(tokens.accessToken.tokenString, initialAccessToken); + XCTAssertEqualObjects(tokens.idToken.tokenString, initialIDToken); + XCTAssertEqualObjects(tokens.refreshToken.tokenString, initialRefreshToken); + } + }); + [readExpectation fulfill]; + }); + + [self waitForExpectationsWithTimeout:5 handler:nil]; + + // Final state should be A, B or C (whichever update ran last) + BOOL matchesA = [user.accessToken.tokenString isEqualToString:accessTokenA] && + [user.refreshToken.tokenString isEqualToString:refreshTokenA] && + [user.idToken.tokenString isEqualToString:idTokenA]; + BOOL matchesB = [user.accessToken.tokenString isEqualToString:accessTokenB] && + [user.refreshToken.tokenString isEqualToString:refreshTokenB] && + [user.idToken.tokenString isEqualToString:idTokenB]; + BOOL matchesC = [user.accessToken.tokenString isEqualToString:accessTokenC] && + [user.refreshToken.tokenString isEqualToString:refreshTokenC] && + [user.idToken.tokenString isEqualToString:idTokenC]; + XCTAssertTrue(matchesA || matchesB || matchesC); + + // This test is a reliable failure detector only under Thread Sanitizer. + // The consistency assertion above is what gives it meaning without TSan. +} + - (void)testRefreshTokensIfNeededWithCompletion_noRefresh_givenRefreshTokenExpired { NSTimeInterval expiresIn = -10; GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:expiresIn