Drop token refresh responses for a replaced authorization

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.

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<GIDGoogleUserCompletion> *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

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<NSString *> *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";