diff --git a/CHANGELOG.md b/CHANGELOG.md index fa5d4193..3b73c848 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - `GIDGoogleUser`'s `accessToken`, `refreshToken`, `idToken` and `profile` are now all derived from that snapshot, so key-value observers of any one of them are notified whenever any of them changes, not only when that property's own value changes. `profile` now sends key-value notifications; previously it sent none. - Key-value observers of those properties are notified while `GIDGoogleUser` holds its internal lock. Calling back into the same user from the observer is fine, but an observer must not synchronously wait (for example with `dispatch_sync`) on another thread that is updating the same user. - `GIDGoogleUser`'s `configuration` is now computed once, when the user is created, and stays fixed for the user's lifetime. Previously it was computed on first access, so a first read after a token update could reflect that update's client ID, server client ID or hosted domain. +- Fix a token refresh that was in flight while scopes were added overwriting the newer tokens. `GIDGoogleUser` now drops a refresh response when the authorization it was requested for has been replaced, so `grantedScopes` and `accessToken` no longer revert to their pre-add-scopes values. - Fix EMM remediation dialogs being suppressed for the rest of the app's lifetime after a single error arrived while no key window was available. The pending-dialog flag is now cleared on every exit path, and its clearing write is synchronized to match the read. - Fix a token request that failed with an EMM error sometimes being reported with the underlying OAuth error instead of `kGIDSignInErrorCodeEMM` when no remediation dialog could be presented. - Fix an EMM error that arrived while a remediation dialog was already showing being reported as `kGIDSignInErrorCodeUnknown` (or as the underlying OAuth error, for a token request) instead of `kGIDSignInErrorCodeEMM`. No second dialog is shown. diff --git a/GoogleSignIn/Sources/GIDGoogleUser.m b/GoogleSignIn/Sources/GIDGoogleUser.m index 6cd7e47a..ad4ba2bc 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser.m +++ b/GoogleSignIn/Sources/GIDGoogleUser.m @@ -257,8 +257,14 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion NSError *_Nullable error) { // Update the auth state under `_authStateLock` so this refresh cannot interleave with // -updateWithTokenResponse:authorizationResponse:profileData:. + NSError *effectiveError = error; @synchronized(self->_authStateLock) { - if (tokenResponse) { + if (self.authState.lastAuthorizationResponse != authorizationResponse) { + // Drop the response if a new authorization (such as from add-scopes) replaced the one this + // refresh was requested for while it was in flight, so the old grant cannot overwrite the + // newer tokens. + effectiveError = nil; + } else if (tokenResponse) { [self.authState updateWithTokenResponse:tokenResponse error:nil]; } else { if (error.domain == OIDOAuthTokenErrorDomain) { @@ -267,7 +273,7 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion } } #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST - [GIDEMMSupport handleTokenFetchEMMError:error completion:^(NSError *_Nullable error) { + [GIDEMMSupport handleTokenFetchEMMError:effectiveError completion:^(NSError *_Nullable error) { // Process the handler queue to call back. NSArray *refreshTokensHandlerQueue; @synchronized(self->_tokenRefreshHandlerQueue) { @@ -288,7 +294,7 @@ - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion } for (GIDGoogleUserCompletion completion in refreshTokensHandlerQueue) { dispatch_async(dispatch_get_main_queue(), ^{ - completion(error ? nil : self, error); + completion(effectiveError ? nil : self, effectiveError); }); } #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST diff --git a/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m b/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m index 9566a271..cf142bea 100644 --- a/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m +++ b/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m @@ -1041,6 +1041,74 @@ - (void)testRefreshTokensIfNeededWithCompletion_noRefresh_givenRefreshTokenExpir [self waitForExpectationsWithTimeout:1 handler:nil]; } +- (void)testRefreshTokensIfNeeded_dropsResponseForReplacedAuthorization { + GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:-10 + idTokenExpiresIn:kIDTokenExpiresIn]; + XCTAssertEqualObjects(user.grantedScopes, @[ OIDAuthorizationRequestTestingScope2 ]); + + XCTestExpectation *expectation = [self expectationWithDescription:@"Callback is called"]; + [user refreshTokensIfNeededWithCompletion:^(GIDGoogleUser *_Nullable refreshedUser, + NSError *_Nullable error) { + [expectation fulfill]; + XCTAssertNotNil(refreshedUser); + XCTAssertNil(error); + }]; + + // The add-scopes flow replaces the authorization while the refresh is in flight. + NSArray *scopesAfterAddScopes = @[ OIDAuthorizationRequestTestingScope2, kNewScope ]; + OIDAuthState *updatedAuthState = + [self authStateWithGrantedScopes:scopesAfterAddScopes + accessToken:@"access_token_after_add_scopes"]; + [user updateWithTokenResponse:updatedAuthState.lastTokenResponse + authorizationResponse:updatedAuthState.lastAuthorizationResponse + profileData:nil]; + + // The stale response carries the old grant's scope and a different access token. + OIDTokenResponse *staleResponse = + [OIDTokenResponse testInstanceWithIDToken:[self idTokenWithExpiresIn:kNewIDTokenExpiresIn] + accessToken:kNewAccessToken + expiresIn:@(kAccessTokenExpiresIn) + refreshToken:kRefreshToken + tokenRequest:nil]; + _tokenFetchHandler(staleResponse, nil); + [self waitForExpectationsWithTimeout:1 handler:nil]; + + XCTAssertEqualObjects(user.accessToken.tokenString, @"access_token_after_add_scopes"); + XCTAssertEqualObjects(user.grantedScopes, scopesAfterAddScopes); +} + +- (void)testRefreshTokensIfNeeded_dropsErrorForReplacedAuthorization { + GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:-10 + idTokenExpiresIn:kIDTokenExpiresIn]; + + XCTestExpectation *expectation = [self expectationWithDescription:@"Callback is called"]; + [user refreshTokensIfNeededWithCompletion:^(GIDGoogleUser *_Nullable refreshedUser, + NSError *_Nullable error) { + [expectation fulfill]; + XCTAssertNotNil(refreshedUser); + XCTAssertNil(error); + }]; + + // The add-scopes flow replaces the authorization while the refresh is in flight. + OIDAuthState *updatedAuthState = + [self authStateWithGrantedScopes:@[ OIDAuthorizationRequestTestingScope2, kNewScope ] + accessToken:@"access_token_after_add_scopes"]; + [user updateWithTokenResponse:updatedAuthState.lastTokenResponse + authorizationResponse:updatedAuthState.lastAuthorizationResponse + profileData:nil]; + + // A stale OAuth token error for the old grant must not invalidate the new authorization. + NSError *staleError = [NSError errorWithDomain:OIDOAuthTokenErrorDomain + code:OIDErrorCodeOAuthTokenInvalidGrant + userInfo:nil]; + _tokenFetchHandler(nil, staleError); + [self waitForExpectationsWithTimeout:1 handler:nil]; + + XCTAssertTrue(user.authState.isAuthorized); + XCTAssertNil(user.authState.authorizationError); + XCTAssertEqualObjects(user.accessToken.tokenString, @"access_token_after_add_scopes"); +} + - (void)testWrapperIdentifier_PresentOnRefreshRequestWhenSet { GIDSignIn.wrapperIdentifier = @"firebase";