From b522b960adda81b85039c1462c9ca24026190a53 Mon Sep 17 00:00:00 2001 From: Worthing ~ <115107835+w-goog@users.noreply.github.com> Date: Wed, 30 Sep 2026 13:56:10 -0700 Subject: [PATCH 1/8] g-orchestrated: Replace @synchronized in GIDGoogleUser with an auth state lock --- GoogleSignIn/Sources/GIDGoogleUser.m | 259 +++++++++++-------- GoogleSignIn/Sources/GIDGoogleUser_Private.h | 5 +- GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m | 2 +- 3 files changed, 154 insertions(+), 112 deletions(-) diff --git a/GoogleSignIn/Sources/GIDGoogleUser.m b/GoogleSignIn/Sources/GIDGoogleUser.m index 11f36149..d10cc483 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser.m +++ b/GoogleSignIn/Sources/GIDGoogleUser.m @@ -127,6 +127,22 @@ @implementation GIDGoogleUser { // from no profile having been supplied, which keeps the current one. GIDProfileData *_pendingProfile; BOOL _hasPendingProfile; + + // Guards `authState` updates and reads that span several of its fields. Recursive because + // AppAuth calls -didChangeState: synchronously while it is held, and KVO observers of the + // token and profile properties also run while it is held: same-thread re-entry is fine + // (see -testTokenObserver_reentersUserDuringUpdate), but an observer must not synchronously + // wait on another thread that is updating the same user, as that would deadlock. Take it via + // -lockAuthState. + NSRecursiveLock *_authStateLock; +} + +- (void)lockAuthState { + [_authStateLock lock]; +} + +- (void)unlockAuthState { + [_authStateLock unlock]; } - (GIDToken *)accessToken { @@ -215,23 +231,36 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion } } // This is the first handler in the queue, a fetch is needed. + // Read the auth state under `_authStateLock` so building the request cannot interleave with + // -updateWithTokenResponse:authorizationResponse:profileData:, and build the parameter + // dictionary outside the lock so helper calls do not run while it is held. + [self lockAuthState]; + NSDictionary *lastTokenAdditionalParameters = + self.authState.lastTokenResponse.request.additionalParameters; + [self unlockAuthState]; + 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]]; + [self lockAuthState]; + 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 { @@ -239,6 +268,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. @@ -305,8 +335,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 @@ -314,6 +347,9 @@ - (instancetype)initWithAuthState:(OIDAuthState *)authState profileData:(nullable GIDProfileData *)profileData { self = [super init]; if (self) { + // Initialize the lock before -updateTokensWithAuthState: below uses it. + _authStateLock = [[NSRecursiveLock alloc] init]; + _tokenRefreshHandlerQueue = [[NSMutableArray alloc] init]; // Park the profile for the first snapshot, built by -updateTokensWithAuthState: below. @@ -336,119 +372,119 @@ - (instancetype)initWithAuthState:(OIDAuthState *)authState - (void)updateWithTokenResponse:(OIDTokenResponse *)tokenResponse authorizationResponse:(OIDAuthorizationResponse *)authorizationResponse profileData:(nullable GIDProfileData *)profileData { - @synchronized(self) { - // Park the profile for the snapshot built when the token response update below calls back - // through -didChangeState:. - _pendingProfile = profileData; - _hasPendingProfile = YES; - - // 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]; - // AppAuth does not call -didChangeState: for a nil token response, so if the profile was not - // folded into a snapshot above, build one here. - if (_hasPendingProfile) { - [self updateTokensWithAuthState:self.authState]; - } + [self lockAuthState]; + // Park the profile for the snapshot built when the token response update below calls back + // through -didChangeState:. + _pendingProfile = profileData; + _hasPendingProfile = YES; + + // 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]; + // AppAuth does not call -didChangeState: for a nil token response, so if the profile was not + // folded into a snapshot above, build one here. + if (_hasPendingProfile) { + [self updateTokensWithAuthState:self.authState]; } + [self unlockAuthState]; } - (void)updateTokensWithAuthState:(OIDAuthState *)authState { - @synchronized(self) { - GIDGoogleUserTokens *current = self.tokens; - GIDProfileData *profile = _hasPendingProfile ? _pendingProfile : current.profile; - _pendingProfile = nil; - _hasPendingProfile = NO; - - // Build the access token - GIDToken *accessToken = - [[GIDToken alloc] initWithTokenString:authState.lastTokenResponse.accessToken - expirationDate:authState.lastTokenResponse.accessTokenExpirationDate]; - - // Build the refresh token - NSDictionary *additionalParameters = authState.lastTokenResponse.additionalParameters; - NSNumber *refreshTokenExpiresIn = nil; - NSDate *refreshTokenExpirationDate = nil; - id expiresInValue = additionalParameters[@"refresh_token_expires_in"]; - if ([expiresInValue isKindOfClass:[NSNumber class]]) { - refreshTokenExpiresIn = (NSNumber *)expiresInValue; - NSTimeInterval interval = [refreshTokenExpiresIn doubleValue]; - refreshTokenExpirationDate = [NSDate dateWithTimeIntervalSinceNow:interval]; - } - GIDToken *refreshToken = [[GIDToken alloc] initWithTokenString:authState.refreshToken - expirationDate:refreshTokenExpirationDate]; - - // Build the ID token - GIDToken *idToken; - NSString *idTokenString = authState.lastTokenResponse.idToken; - if (idTokenString) { - NSDate *idTokenExpirationDate = - [[[OIDIDToken alloc] initWithIDTokenString:idTokenString] expiresAt]; - idToken = [[GIDToken alloc] initWithTokenString:idTokenString - expirationDate:idTokenExpirationDate]; - } else { - idToken = nil; - } + [self lockAuthState]; + GIDGoogleUserTokens *current = self.tokens; + GIDProfileData *profile = _hasPendingProfile ? _pendingProfile : current.profile; + _pendingProfile = nil; + _hasPendingProfile = NO; + + // Build the access token + GIDToken *accessToken = + [[GIDToken alloc] initWithTokenString:authState.lastTokenResponse.accessToken + expirationDate:authState.lastTokenResponse.accessTokenExpirationDate]; + + // Build the refresh token + NSDictionary *additionalParameters = authState.lastTokenResponse.additionalParameters; + NSNumber *refreshTokenExpiresIn = nil; + NSDate *refreshTokenExpirationDate = nil; + id expiresInValue = additionalParameters[@"refresh_token_expires_in"]; + if ([expiresInValue isKindOfClass:[NSNumber class]]) { + refreshTokenExpiresIn = (NSNumber *)expiresInValue; + NSTimeInterval interval = [refreshTokenExpiresIn doubleValue]; + refreshTokenExpirationDate = [NSDate dateWithTimeIntervalSinceNow:interval]; + } + GIDToken *refreshToken = [[GIDToken alloc] initWithTokenString:authState.refreshToken + expirationDate:refreshTokenExpirationDate]; - NSArray *grantedScopes = - GIDGrantedScopesFromScopeString(authState.lastTokenResponse.scope); + // Build the ID token + GIDToken *idToken; + NSString *idTokenString = authState.lastTokenResponse.idToken; + if (idTokenString) { + NSDate *idTokenExpirationDate = + [[[OIDIDToken alloc] initWithIDTokenString:idTokenString] expiresAt]; + idToken = [[GIDToken alloc] initWithTokenString:idTokenString + expirationDate:idTokenExpirationDate]; + } else { + idToken = nil; + } - 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]; - } + 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]; + } - // If the computed values are equal to the existing ones, keep the existing ones. In that case, - // an update 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; - } - // Both nil must be checked explicitly: isEqualToArray: returns NO when either side is nil. - if ((!grantedScopes && !current.grantedScopes) || - [grantedScopes isEqualToArray:current.grantedScopes]) { - grantedScopes = current.grantedScopes; - } + // If the computed values are equal to the existing ones, keep the existing ones. In that case, + // an update 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; + } + // Both nil must be checked explicitly: isEqualToArray: returns NO when either side is nil. + if ((!grantedScopes && !current.grantedScopes) || + [grantedScopes isEqualToArray:current.grantedScopes]) { + grantedScopes = current.grantedScopes; + } - if (!current || - accessToken != current.accessToken || - refreshToken != current.refreshToken || - idToken != current.idToken || - grantedScopes != current.grantedScopes || - profile != current.profile) { - // Replacing `tokens` notifies KVO observers of the derived properties while the lock is - // held; see the key-value observing note on GIDGoogleUser in GIDGoogleUser.h. - self.tokens = [[GIDGoogleUserTokens alloc] initWithAccessToken:accessToken - refreshToken:refreshToken - idToken:idToken - grantedScopes:grantedScopes - configuration:configuration - profile:profile]; - } + if (!current || + accessToken != current.accessToken || + refreshToken != current.refreshToken || + idToken != current.idToken || + grantedScopes != current.grantedScopes || + profile != current.profile) { + // Replacing `tokens` notifies KVO observers of the derived properties while the lock is + // held; see the key-value observing note on GIDGoogleUser in GIDGoogleUser.h. + self.tokens = [[GIDGoogleUserTokens alloc] initWithAccessToken:accessToken + refreshToken:refreshToken + idToken:idToken + grantedScopes:grantedScopes + configuration:configuration + profile:profile]; } + [self unlockAuthState]; } #pragma mark - OIDAuthStateChangeDelegate @@ -480,8 +516,11 @@ - (nullable instancetype)initWithCoder:(NSCoder *)decoder { } - (void)encodeWithCoder:(NSCoder *)encoder { + // 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 343f4baf..b20e8f02 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 . @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..6f6a91c1 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 { From e8423a2641da2b0a19e6dd90c87d449c8c21c009 Mon Sep 17 00:00:00 2001 From: Worthing ~ <115107835+w-goog@users.noreply.github.com> Date: Thu, 15 Oct 2026 13:03:16 -0700 Subject: [PATCH 2/8] g-orchestrated: Link the authState TODO to issue #648 --- GoogleSignIn/Sources/GIDGoogleUser_Private.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GoogleSignIn/Sources/GIDGoogleUser_Private.h b/GoogleSignIn/Sources/GIDGoogleUser_Private.h index b20e8f02..61cfe9aa 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser_Private.h +++ b/GoogleSignIn/Sources/GIDGoogleUser_Private.h @@ -60,7 +60,7 @@ typedef void (^GIDGoogleUserCompletion)(GIDGoogleUser *_Nullable user, NSError * /// 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 . +// 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 From 480fd4dbaadec6d49d3d7dabbec3d6ee520bc32c Mon Sep 17 00:00:00 2001 From: Worthing ~ <115107835+w-goog@users.noreply.github.com> Date: Thu, 15 Oct 2026 15:12:24 -0700 Subject: [PATCH 3/8] g-orchestrated: Build the refresh request in one auth state lock window --- GoogleSignIn/Sources/GIDGoogleUser.m | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/GoogleSignIn/Sources/GIDGoogleUser.m b/GoogleSignIn/Sources/GIDGoogleUser.m index d10cc483..c282e821 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser.m +++ b/GoogleSignIn/Sources/GIDGoogleUser.m @@ -231,13 +231,13 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion } } // This is the first handler in the queue, a fetch is needed. - // Read the auth state under `_authStateLock` so building the request cannot interleave with - // -updateWithTokenResponse:authorizationResponse:profileData:, and build the parameter - // dictionary outside the lock so helper calls do not run while it is held. + // Build the request under `_authStateLock` so the parameters, the authorization response and + // the refresh token all come from the same auth state, and building the request cannot + // interleave with -updateWithTokenResponse:authorizationResponse:profileData:. The helpers + // called here only build dictionaries and never call back into this object. [self lockAuthState]; NSDictionary *lastTokenAdditionalParameters = self.authState.lastTokenResponse.request.additionalParameters; - [self unlockAuthState]; NSMutableDictionary *additionalParameters = [@{} mutableCopy]; #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST @@ -248,7 +248,6 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST [additionalParameters addEntriesFromDictionary:[GIDSignInPreferences loggingParameters]]; - [self lockAuthState]; OIDAuthorizationResponse *authorizationResponse = self.authState.lastAuthorizationResponse; OIDTokenRequest *tokenRefreshRequest = [self.authState tokenRefreshRequestWithAdditionalParameters:additionalParameters]; From 4c36f8482a8fc96b8ca8ead43e0a8a32548f0efa Mon Sep 17 00:00:00 2001 From: Worthing ~ <115107835+w-goog@users.noreply.github.com> Date: Thu, 15 Oct 2026 16:02:22 -0700 Subject: [PATCH 4/8] g-orchestrated: Reword the auth state lock comments --- GoogleSignIn/Sources/GIDGoogleUser.m | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/GoogleSignIn/Sources/GIDGoogleUser.m b/GoogleSignIn/Sources/GIDGoogleUser.m index c282e821..abc038ac 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser.m +++ b/GoogleSignIn/Sources/GIDGoogleUser.m @@ -128,12 +128,11 @@ @implementation GIDGoogleUser { GIDProfileData *_pendingProfile; BOOL _hasPendingProfile; - // Guards `authState` updates and reads that span several of its fields. Recursive because - // AppAuth calls -didChangeState: synchronously while it is held, and KVO observers of the - // token and profile properties also run while it is held: same-thread re-entry is fine - // (see -testTokenObserver_reentersUserDuringUpdate), but an observer must not synchronously - // wait on another thread that is updating the same user, as that would deadlock. Take it via - // -lockAuthState. + // This lock guards `authState` updates and reads. + // The lock is recursive due to a) synchronous AppAuth -didChangeState: calls and b) KVO + // observers of the token / profile properties. + // Observers must not synchronously wait on another thread that is updating the same user! It + // would deadlock. Take the lock via -lockAuthState. NSRecursiveLock *_authStateLock; } @@ -231,10 +230,9 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion } } // This is the first handler in the queue, a fetch is needed. - // Build the request under `_authStateLock` so the parameters, the authorization response and - // the refresh token all come from the same auth state, and building the request cannot - // interleave with -updateWithTokenResponse:authorizationResponse:profileData:. The helpers - // called here only build dictionaries and never call back into this object. + // Build the request under `_authStateLock`. This prevents the request-building from + // interleaving with token response updates. + // These helpers do not (and must not) call back into this object. [self lockAuthState]; NSDictionary *lastTokenAdditionalParameters = self.authState.lastTokenResponse.request.additionalParameters; From 11c88cdada64fb5e0b3f51db148fb1ca1c22fea1 Mon Sep 17 00:00:00 2001 From: Worthing ~ <115107835+w-goog@users.noreply.github.com> Date: Mon, 19 Oct 2026 12:52:16 -0700 Subject: [PATCH 5/8] g-orchestrated: Synchronize on a private lock object instead of NSRecursiveLock --- GoogleSignIn/Sources/GIDGoogleUser.m | 288 +++++++++++++-------------- 1 file changed, 141 insertions(+), 147 deletions(-) diff --git a/GoogleSignIn/Sources/GIDGoogleUser.m b/GoogleSignIn/Sources/GIDGoogleUser.m index abc038ac..e2e999e0 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser.m +++ b/GoogleSignIn/Sources/GIDGoogleUser.m @@ -128,20 +128,13 @@ @implementation GIDGoogleUser { GIDProfileData *_pendingProfile; BOOL _hasPendingProfile; - // This lock guards `authState` updates and reads. - // The lock is recursive due to a) synchronous AppAuth -didChangeState: calls and b) KVO - // observers of the token / profile properties. + // 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. Take the lock via -lockAuthState. - NSRecursiveLock *_authStateLock; -} - -- (void)lockAuthState { - [_authStateLock lock]; -} - -- (void)unlockAuthState { - [_authStateLock unlock]; + // would deadlock. + NSObject *_authStateLock; } - (GIDToken *)accessToken { @@ -233,23 +226,25 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion // Build the request under `_authStateLock`. This prevents the request-building from // interleaving with token response updates. // These helpers do not (and must not) call back into this object. - [self lockAuthState]; - NSDictionary *lastTokenAdditionalParameters = - self.authState.lastTokenResponse.request.additionalParameters; + OIDAuthorizationResponse *authorizationResponse; + OIDTokenRequest *tokenRefreshRequest; + @synchronized(_authStateLock) { + NSDictionary *lastTokenAdditionalParameters = + self.authState.lastTokenResponse.request.additionalParameters; - NSMutableDictionary *additionalParameters = [@{} mutableCopy]; + NSMutableDictionary *additionalParameters = [@{} mutableCopy]; #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST - [additionalParameters addEntriesFromDictionary: - [GIDEMMSupport updatedEMMParametersWithParameters:lastTokenAdditionalParameters]]; + [additionalParameters addEntriesFromDictionary: + [GIDEMMSupport updatedEMMParametersWithParameters:lastTokenAdditionalParameters]]; #elif TARGET_OS_OSX || TARGET_OS_MACCATALYST - [additionalParameters addEntriesFromDictionary:lastTokenAdditionalParameters]; + [additionalParameters addEntriesFromDictionary:lastTokenAdditionalParameters]; #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST - [additionalParameters addEntriesFromDictionary:[GIDSignInPreferences loggingParameters]]; + [additionalParameters addEntriesFromDictionary:[GIDSignInPreferences loggingParameters]]; - OIDAuthorizationResponse *authorizationResponse = self.authState.lastAuthorizationResponse; - OIDTokenRequest *tokenRefreshRequest = - [self.authState tokenRefreshRequestWithAdditionalParameters:additionalParameters]; - [self unlockAuthState]; + authorizationResponse = self.authState.lastAuthorizationResponse; + tokenRefreshRequest = + [self.authState tokenRefreshRequestWithAdditionalParameters:additionalParameters]; + } [OIDAuthorizationService performTokenRequest:tokenRefreshRequest originalAuthorizationResponse:authorizationResponse @@ -257,15 +252,15 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion 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 { - if (error.domain == OIDOAuthTokenErrorDomain) { - [self.authState updateWithAuthorizationError:error]; + @synchronized(self->_authStateLock) { + if (tokenResponse) { + [self.authState updateWithTokenResponse:tokenResponse error:nil]; + } else { + if (error.domain == OIDOAuthTokenErrorDomain) { + [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. @@ -332,11 +327,10 @@ - (void)addScopes:(NSArray *)scopes #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST - (nullable NSString *)emmSupport { - [self lockAuthState]; - NSString *emmSupport = self.authState.lastAuthorizationResponse - .request.additionalParameters[kEMMSupportParameterName]; - [self unlockAuthState]; - return emmSupport; + @synchronized(_authStateLock) { + return self.authState.lastAuthorizationResponse + .request.additionalParameters[kEMMSupportParameterName]; + } } #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST @@ -344,8 +338,8 @@ - (instancetype)initWithAuthState:(OIDAuthState *)authState profileData:(nullable GIDProfileData *)profileData { self = [super init]; if (self) { - // Initialize the lock before -updateTokensWithAuthState: below uses it. - _authStateLock = [[NSRecursiveLock alloc] init]; + // Create the lock object before -updateTokensWithAuthState: below synchronizes on it. + _authStateLock = [[NSObject alloc] init]; _tokenRefreshHandlerQueue = [[NSMutableArray alloc] init]; @@ -369,119 +363,119 @@ - (instancetype)initWithAuthState:(OIDAuthState *)authState - (void)updateWithTokenResponse:(OIDTokenResponse *)tokenResponse authorizationResponse:(OIDAuthorizationResponse *)authorizationResponse profileData:(nullable GIDProfileData *)profileData { - [self lockAuthState]; - // Park the profile for the snapshot built when the token response update below calls back - // through -didChangeState:. - _pendingProfile = profileData; - _hasPendingProfile = YES; - - // 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]; - // AppAuth does not call -didChangeState: for a nil token response, so if the profile was not - // folded into a snapshot above, build one here. - if (_hasPendingProfile) { - [self updateTokensWithAuthState:self.authState]; + @synchronized(_authStateLock) { + // Park the profile for the snapshot built when the token response update below calls back + // through -didChangeState:. + _pendingProfile = profileData; + _hasPendingProfile = YES; + + // 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]; + // AppAuth does not call -didChangeState: for a nil token response, so if the profile was not + // folded into a snapshot above, build one here. + if (_hasPendingProfile) { + [self updateTokensWithAuthState:self.authState]; + } } - [self unlockAuthState]; } - (void)updateTokensWithAuthState:(OIDAuthState *)authState { - [self lockAuthState]; - GIDGoogleUserTokens *current = self.tokens; - GIDProfileData *profile = _hasPendingProfile ? _pendingProfile : current.profile; - _pendingProfile = nil; - _hasPendingProfile = NO; - - // Build the access token - GIDToken *accessToken = - [[GIDToken alloc] initWithTokenString:authState.lastTokenResponse.accessToken - expirationDate:authState.lastTokenResponse.accessTokenExpirationDate]; - - // Build the refresh token - NSDictionary *additionalParameters = authState.lastTokenResponse.additionalParameters; - NSNumber *refreshTokenExpiresIn = nil; - NSDate *refreshTokenExpirationDate = nil; - id expiresInValue = additionalParameters[@"refresh_token_expires_in"]; - if ([expiresInValue isKindOfClass:[NSNumber class]]) { - refreshTokenExpiresIn = (NSNumber *)expiresInValue; - NSTimeInterval interval = [refreshTokenExpiresIn doubleValue]; - refreshTokenExpirationDate = [NSDate dateWithTimeIntervalSinceNow:interval]; - } - GIDToken *refreshToken = [[GIDToken alloc] initWithTokenString:authState.refreshToken - expirationDate:refreshTokenExpirationDate]; + @synchronized(_authStateLock) { + GIDGoogleUserTokens *current = self.tokens; + GIDProfileData *profile = _hasPendingProfile ? _pendingProfile : current.profile; + _pendingProfile = nil; + _hasPendingProfile = NO; + + // Build the access token + GIDToken *accessToken = + [[GIDToken alloc] initWithTokenString:authState.lastTokenResponse.accessToken + expirationDate:authState.lastTokenResponse.accessTokenExpirationDate]; + + // Build the refresh token + NSDictionary *additionalParameters = authState.lastTokenResponse.additionalParameters; + NSNumber *refreshTokenExpiresIn = nil; + NSDate *refreshTokenExpirationDate = nil; + id expiresInValue = additionalParameters[@"refresh_token_expires_in"]; + if ([expiresInValue isKindOfClass:[NSNumber class]]) { + refreshTokenExpiresIn = (NSNumber *)expiresInValue; + NSTimeInterval interval = [refreshTokenExpiresIn doubleValue]; + refreshTokenExpirationDate = [NSDate dateWithTimeIntervalSinceNow:interval]; + } + GIDToken *refreshToken = [[GIDToken alloc] initWithTokenString:authState.refreshToken + expirationDate:refreshTokenExpirationDate]; + + // Build the ID token + GIDToken *idToken; + NSString *idTokenString = authState.lastTokenResponse.idToken; + if (idTokenString) { + NSDate *idTokenExpirationDate = + [[[OIDIDToken alloc] initWithIDTokenString:idTokenString] expiresAt]; + idToken = [[GIDToken alloc] initWithTokenString:idTokenString + expirationDate:idTokenExpirationDate]; + } else { + idToken = nil; + } - // Build the ID token - GIDToken *idToken; - NSString *idTokenString = authState.lastTokenResponse.idToken; - if (idTokenString) { - NSDate *idTokenExpirationDate = - [[[OIDIDToken alloc] initWithIDTokenString:idTokenString] expiresAt]; - idToken = [[GIDToken alloc] initWithTokenString:idTokenString - expirationDate:idTokenExpirationDate]; - } else { - idToken = nil; - } + NSArray *grantedScopes = + GIDGrantedScopesFromScopeString(authState.lastTokenResponse.scope); - 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]; - } + 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]; + } - // If the computed values are equal to the existing ones, keep the existing ones. In that case, - // an update 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; - } - // Both nil must be checked explicitly: isEqualToArray: returns NO when either side is nil. - if ((!grantedScopes && !current.grantedScopes) || - [grantedScopes isEqualToArray:current.grantedScopes]) { - grantedScopes = current.grantedScopes; - } + // If the computed values are equal to the existing ones, keep the existing ones. In that case, + // an update 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; + } + // Both nil must be checked explicitly: isEqualToArray: returns NO when either side is nil. + if ((!grantedScopes && !current.grantedScopes) || + [grantedScopes isEqualToArray:current.grantedScopes]) { + grantedScopes = current.grantedScopes; + } - if (!current || - accessToken != current.accessToken || - refreshToken != current.refreshToken || - idToken != current.idToken || - grantedScopes != current.grantedScopes || - profile != current.profile) { - // Replacing `tokens` notifies KVO observers of the derived properties while the lock is - // held; see the key-value observing note on GIDGoogleUser in GIDGoogleUser.h. - self.tokens = [[GIDGoogleUserTokens alloc] initWithAccessToken:accessToken - refreshToken:refreshToken - idToken:idToken - grantedScopes:grantedScopes - configuration:configuration - profile:profile]; + if (!current || + accessToken != current.accessToken || + refreshToken != current.refreshToken || + idToken != current.idToken || + grantedScopes != current.grantedScopes || + profile != current.profile) { + // Replacing `tokens` notifies KVO observers of the derived properties while the lock is + // held; see the key-value observing note on GIDGoogleUser in GIDGoogleUser.h. + self.tokens = [[GIDGoogleUserTokens alloc] initWithAccessToken:accessToken + refreshToken:refreshToken + idToken:idToken + grantedScopes:grantedScopes + configuration:configuration + profile:profile]; + } } - [self unlockAuthState]; } #pragma mark - OIDAuthStateChangeDelegate @@ -514,10 +508,10 @@ - (nullable instancetype)initWithCoder:(NSCoder *)decoder { - (void)encodeWithCoder:(NSCoder *)encoder { // 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]; + @synchronized(_authStateLock) { + [encoder encodeObject:self.profile forKey:kProfileDataKey]; + [encoder encodeObject:self.authState forKey:kAuthStateKey]; + } } @end From 75884b53db48b4134d5b12cd8987d19a96166e37 Mon Sep 17 00:00:00 2001 From: Worthing ~ <115107835+w-goog@users.noreply.github.com> Date: Mon, 19 Oct 2026 13:12:20 -0700 Subject: [PATCH 6/8] g-orchestrated: Test refresh responses racing sign-in updates --- GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m | 126 ++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m b/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m index 6f6a91c1..a30599d5 100644 --- a/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m +++ b/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m @@ -500,6 +500,132 @@ - (void)testDidChangeState_concurrentUpdates_leaveConsistentTokenSet { // The consistency assertion above is what gives it meaning without TSan. } +// 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. +} + // 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 From 374f97d214134b2abee22ff47da41d07db352542 Mon Sep 17 00:00:00 2001 From: Worthing ~ <115107835+w-goog@users.noreply.github.com> Date: Tue, 20 Oct 2026 09:34:04 -0700 Subject: [PATCH 7/8] g-orchestrated: Build the refresh parameters outside the auth state lock --- GoogleSignIn/Sources/GIDGoogleUser.m | 29 ++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/GoogleSignIn/Sources/GIDGoogleUser.m b/GoogleSignIn/Sources/GIDGoogleUser.m index e2e999e0..6cd7e47a 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser.m +++ b/GoogleSignIn/Sources/GIDGoogleUser.m @@ -223,24 +223,29 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion } } // This is the first handler in the queue, a fetch is needed. - // Build the request under `_authStateLock`. This prevents the request-building from - // interleaving with token response updates. - // These helpers do not (and must not) call back into this object. - OIDAuthorizationResponse *authorizationResponse; - OIDTokenRequest *tokenRefreshRequest; + // 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) { - NSDictionary *lastTokenAdditionalParameters = - self.authState.lastTokenResponse.request.additionalParameters; + lastTokenAdditionalParameters = self.authState.lastTokenResponse.request.additionalParameters; + } - NSMutableDictionary *additionalParameters = [@{} mutableCopy]; + NSMutableDictionary *additionalParameters = [@{} mutableCopy]; #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST - [additionalParameters addEntriesFromDictionary: - [GIDEMMSupport updatedEMMParametersWithParameters:lastTokenAdditionalParameters]]; + [additionalParameters addEntriesFromDictionary: + [GIDEMMSupport updatedEMMParametersWithParameters:lastTokenAdditionalParameters]]; #elif TARGET_OS_OSX || TARGET_OS_MACCATALYST - [additionalParameters addEntriesFromDictionary:lastTokenAdditionalParameters]; + [additionalParameters addEntriesFromDictionary:lastTokenAdditionalParameters]; #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST - [additionalParameters addEntriesFromDictionary:[GIDSignInPreferences loggingParameters]]; + [additionalParameters addEntriesFromDictionary:[GIDSignInPreferences loggingParameters]]; + // 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]; From 64fc548f6e259b7a13de1bff384f72491561bbb7 Mon Sep 17 00:00:00 2001 From: Worthing ~ <115107835+w-goog@users.noreply.github.com> Date: Tue, 20 Oct 2026 09:34:04 -0700 Subject: [PATCH 8/8] g-orchestrated: Move the refresh race test into the refreshTokensIfNeeded section --- GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m | 252 ++++++++++---------- 1 file changed, 126 insertions(+), 126 deletions(-) diff --git a/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m b/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m index a30599d5..9566a271 100644 --- a/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m +++ b/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m @@ -500,132 +500,6 @@ - (void)testDidChangeState_concurrentUpdates_leaveConsistentTokenSet { // The consistency assertion above is what gives it meaning without TSan. } -// 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. -} - // 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 @@ -1022,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