Replace @synchronized(self) in GIDGoogleUser with a private lock

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<NSString *, NSString *> *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,15 +332,20 @@ - (void)addScopes:(NSArray<NSString *> *)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
 
 - (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

GoogleSignIn/Sources/GIDGoogleUser_Private.h

@@ -58,11 +58,14 @@ typedef void (^GIDGoogleUserCompletion)(GIDGoogleUser *_Nullable user, NSError *
 @interface GIDGoogleUser () <OIDAuthStateChangeDelegate>
 
 /// 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

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