Publish GIDGoogleUser's tokens as immutable snapshots

CHANGELOG.md

@@ -1,6 +1,9 @@
 # Unreleased
 - Fix a crash when a server error response carries a non-string value under its `error` key. The EMM error handler sent `-hasPrefix:` to whatever value was present, raising an unrecognized selector exception on a number, array or object.
 - Fix a custom `nonce` and requested token `claims` being dropped when a sign-in is continued after a Device Policy app restart.
+- Fix a data race on `GIDGoogleUser`'s tokens. Concurrent token updates could write the access, refresh and ID tokens from different threads at once, and readers could observe a partially updated set. The three tokens are now published together as one snapshot.
+- `GIDGoogleUser`'s `accessToken`, `refreshToken` and `idToken` 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.
+- 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.
 
 # 10.0.0
 - **BREAKING**: Update to AppAuth 3.0.0 and GTMAppAuth 6.0.0, which raises the minimum deployment targets to iOS 15.0 and macOS 12.0, widens the `GTMSessionFetcher` dependency to allow 4.x and 5.x, and renames the version-specific Swift Package Manager manifest to `Package@swift-5.7.swift`. Projects that must keep supporting earlier OS versions should stay on GoogleSignIn 9.2.0. ([#628](https://github.com/google/GoogleSignIn-iOS/pull/628))

GoogleSignIn/Sources/GIDGoogleUser.m

@@ -61,6 +61,22 @@ @interface GIDGoogleUser ()
 @end
 #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST
 
+@implementation GIDGoogleUserTokens
+
+- (instancetype)initWithAccessToken:(GIDToken *)accessToken
+                       refreshToken:(GIDToken *)refreshToken
+                            idToken:(nullable GIDToken *)idToken {
+  self = [super init];
+  if (self) {
+    _accessToken = accessToken;
+    _refreshToken = refreshToken;
+    _idToken = idToken;
+  }
+  return self;
+}
+
+@end
+
 @implementation GIDGoogleUser {
   GIDConfiguration *_cachedConfiguration;
 
@@ -69,6 +85,32 @@ @implementation GIDGoogleUser {
   NSMutableArray<GIDGoogleUserCompletion> *_tokenRefreshHandlerQueue;
 }
 
+- (GIDToken *)accessToken {
+  return self.tokens.accessToken;
+}
+
+- (GIDToken *)refreshToken {
+  return self.tokens.refreshToken;
+}
+
+- (nullable GIDToken *)idToken {
+  return self.tokens.idToken;
+}
+
+// The three token properties are derived from `tokens`, so KVO observers of each one are notified
+// whenever `tokens` is replaced (a change to any of them notifies observers of all three).
++ (NSSet<NSString *> *)keyPathsForValuesAffectingAccessToken {
+  return [NSSet setWithObject:NSStringFromSelector(@selector(tokens))];
+}
+
++ (NSSet<NSString *> *)keyPathsForValuesAffectingRefreshToken {
+  return [NSSet setWithObject:NSStringFromSelector(@selector(tokens))];
+}
+
++ (NSSet<NSString *> *)keyPathsForValuesAffectingIdToken {
+  return [NSSet setWithObject:NSStringFromSelector(@selector(tokens))];
+}
+
 - (nullable NSString *)userID {
   NSString *idTokenString = self.idToken.tokenString;
   if (idTokenString) {
@@ -118,14 +160,19 @@ - (GIDConfiguration *)configuration {
 }
 
 - (void)refreshTokensIfNeededWithCompletion:(GIDGoogleUserCompletion)completion {
-  if (!([self.accessToken.expirationDate timeIntervalSinceNow] < kMinimalTimeToExpire ||
-      (self.idToken && [self.idToken.expirationDate timeIntervalSinceNow] < kMinimalTimeToExpire))) {
+  // A single read here avoids accidnetally reading tokens from multiple snapshots.
+  GIDGoogleUserTokens *tokens = self.tokens;
+
+  if (!([tokens.accessToken.expirationDate timeIntervalSinceNow] < kMinimalTimeToExpire ||
+      (tokens.idToken &&
+       [tokens.idToken.expirationDate timeIntervalSinceNow] < kMinimalTimeToExpire))) {
     dispatch_async(dispatch_get_main_queue(), ^{
       completion(self, nil);
     });
     return;
   }
-  if (self.refreshToken.expirationDate && [self.refreshToken.expirationDate timeIntervalSinceNow] <= 0) {
+  if (tokens.refreshToken.expirationDate &&
+      [tokens.refreshToken.expirationDate timeIntervalSinceNow] <= 0) {
     NSError *error = [NSError errorWithDomain:kGIDSignInErrorDomain
                                          code:kGIDSignInErrorCodeRefreshTokenExpired
                                      userInfo:nil];
@@ -276,41 +323,63 @@ - (void)updateWithTokenResponse:(OIDTokenResponse *)tokenResponse
   }
 }
 
+// KVO observers of the token properties run while `@synchronized(self)` is held; same-thread
+// re-entry is fine because it is recursive, but an observer must not synchronously wait on another
+// thread that is updating the same user.
 - (void)updateTokensWithAuthState:(OIDAuthState *)authState {
-  GIDToken *accessToken =
-      [[GIDToken alloc] initWithTokenString:authState.lastTokenResponse.accessToken
-                             expirationDate:authState.lastTokenResponse.accessTokenExpirationDate];
-  if (![self.accessToken isEqualToToken:accessToken]) {
-    self.accessToken = accessToken;
-  }
-
-  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];
-  if (![self.refreshToken isEqualToToken:refreshToken]) {
-    self.refreshToken = refreshToken;
-  }
-
-  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;
-  }
-  if ((self.idToken || idToken) && ![self.idToken isEqualToToken:idToken]) {
-    self.idToken = idToken;
+  @synchronized(self) {
+    GIDGoogleUserTokens *current = self.tokens;
+
+    // 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;
+    }
+
+    // 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;
+    }
+
+    if (!current ||
+        accessToken != current.accessToken ||
+        refreshToken != current.refreshToken ||
+        idToken != current.idToken) {
+      self.tokens = [[GIDGoogleUserTokens alloc] initWithAccessToken:accessToken
+                                                        refreshToken:refreshToken
+                                                             idToken:idToken];
+    }
   }
 }
 
@@ -340,23 +409,19 @@ + (BOOL)supportsSecureCoding {
 }
 
 - (nullable instancetype)initWithCoder:(NSCoder *)decoder {
-  self = [super init];
-  if (self) {
-    GIDProfileData *profile =
-        [decoder decodeObjectOfClass:[GIDProfileData class] forKey:kProfileDataKey];
-
-    OIDAuthState *authState;
-    if ([decoder containsValueForKey:kAuthStateKey]) { // Current encoding
-      authState = [decoder decodeObjectOfClass:[OIDAuthState class] forKey:kAuthStateKey];
-    } else { // Old encoding
-      GIDAuthentication *authentication = [decoder decodeObjectOfClass:[GIDAuthentication class]
-                                                                forKey:@"authentication"];
-      authState = authentication.authState;
-    }
-
-    self = [self initWithAuthState:authState profileData:profile];
+  GIDProfileData *profile =
+      [decoder decodeObjectOfClass:[GIDProfileData class] forKey:kProfileDataKey];
+
+  OIDAuthState *authState;
+  if ([decoder containsValueForKey:kAuthStateKey]) { // Current encoding
+    authState = [decoder decodeObjectOfClass:[OIDAuthState class] forKey:kAuthStateKey];
+  } else { // Old encoding
+    GIDAuthentication *authentication = [decoder decodeObjectOfClass:[GIDAuthentication class]
+                                                              forKey:@"authentication"];
+    authState = authentication.authState;
   }
-  return self;
+
+  return [self initWithAuthState:authState profileData:profile];
 }
 
 - (void)encodeWithCoder:(NSCoder *)encoder {

GoogleSignIn/Sources/GIDGoogleUser_Private.h

@@ -22,25 +22,40 @@
 #import <AppAuth/AppAuth.h>
 #endif
 
+@class GIDToken;
 @class OIDAuthState;
 
 NS_ASSUME_NONNULL_BEGIN
 
 /// A completion block that takes a `GIDGoogleUser` or an error if the attempt to refresh tokens was unsuccessful.
 typedef void (^GIDGoogleUserCompletion)(GIDGoogleUser *_Nullable user, NSError *_Nullable error);
 
-/// Internal methods for the class that are not part of the public API.
-@interface GIDGoogleUser () <OIDAuthStateChangeDelegate>
+/// An immutable snapshot of a user's access, refresh and ID tokens.
+/// This value is replaced as a whole so that readers never see a mix of old and new tokens.
+@interface GIDGoogleUserTokens : NSObject
 
-@property(nonatomic, readwrite) GIDToken *accessToken;
+@property(nonatomic, readonly) GIDToken *accessToken;
+@property(nonatomic, readonly) GIDToken *refreshToken;
+@property(nonatomic, readonly, nullable) GIDToken *idToken;
 
-@property(nonatomic, readwrite) GIDToken *refreshToken;
+- (instancetype)initWithAccessToken:(GIDToken *)accessToken
+                       refreshToken:(GIDToken *)refreshToken
+                            idToken:(nullable GIDToken *)idToken NS_DESIGNATED_INITIALIZER;
+- (instancetype)init NS_UNAVAILABLE;
 
-@property(nonatomic, readwrite, nullable) GIDToken *idToken;
+@end
+
+/// Internal methods for the class that are not part of the public API.
+@interface GIDGoogleUser () <OIDAuthStateChangeDelegate>
 
 /// A representation of the state of the OAuth session for this instance.
 @property(nonatomic, readonly) OIDAuthState *authState;
 
+/// The user's current tokens. 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)`.
+@property(atomic, strong, nullable) GIDGoogleUserTokens *tokens;
+
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
 @property(nonatomic, readwrite) id<GTMFetcherAuthorizationProtocol> fetcherAuthorizer;