diff --git a/CHANGELOG.md b/CHANGELOG.md index 7703fa75..ffe95193 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/GoogleSignIn/Sources/GIDGoogleUser.m b/GoogleSignIn/Sources/GIDGoogleUser.m index f67bb4c8..065246d3 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser.m +++ b/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 *_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 *)keyPathsForValuesAffectingAccessToken { + return [NSSet setWithObject:NSStringFromSelector(@selector(tokens))]; +} + ++ (NSSet *)keyPathsForValuesAffectingRefreshToken { + return [NSSet setWithObject:NSStringFromSelector(@selector(tokens))]; +} + ++ (NSSet *)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 { diff --git a/GoogleSignIn/Sources/GIDGoogleUser_Private.h b/GoogleSignIn/Sources/GIDGoogleUser_Private.h index f07a1045..b6ec421c 100644 --- a/GoogleSignIn/Sources/GIDGoogleUser_Private.h +++ b/GoogleSignIn/Sources/GIDGoogleUser_Private.h @@ -22,6 +22,7 @@ #import #endif +@class GIDToken; @class OIDAuthState; NS_ASSUME_NONNULL_BEGIN @@ -29,18 +30,32 @@ 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 () +/// 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 () /// 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 fetcherAuthorizer; diff --git a/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m b/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m index a6a5788c..39edced7 100644 --- a/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m +++ b/GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m @@ -53,6 +53,31 @@ #import #endif +// Observer that runs `onChange` and `onChangeWithDictionary` whenever an observed key path changes. +@interface GIDGoogleUserTestKVOObserver : NSObject + +@property(nonatomic, copy, nullable) void (^onChange)(void); +@property(nonatomic, copy, nullable) + void (^onChangeWithDictionary)(NSDictionary *_Nullable change); + +@end + +@implementation GIDGoogleUserTestKVOObserver + +- (void)observeValueForKeyPath:(NSString *)keyPath + ofObject:(id)object + change:(NSDictionary *)change + context:(void *)context { + if (self.onChange) { + self.onChange(); + } + if (self.onChangeWithDictionary) { + self.onChangeWithDictionary(change); + } +} + +@end + static NSString *const kNewAccessToken = @"new_access_token"; static NSString *const kNewRefreshToken = @"new_refresh_token"; @@ -222,6 +247,211 @@ - (void)testUpdateAuthState_tokensAreNotChanged { XCTAssertIdentical(user.refreshToken, refreshTokenBeforeUpdate); } +- (void)testUpdateTokens_concurrentUpdates_leaveConsistentTokenSet { + GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:kAccessTokenExpiresIn + idTokenExpiresIn:kIDTokenExpiresIn]; + NSString *initialAccessToken = user.accessToken.tokenString; + NSString *initialIDToken = user.idToken.tokenString; + + NSString *idTokenA = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn]; + NSString *accessTokenA = @"access_token_A"; + OIDAuthState *authStateA = [OIDAuthState testInstanceWithIDToken:idTokenA + accessToken:accessTokenA + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:kNewRefreshToken]; + + NSString *idTokenB = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn + 1]; + NSString *accessTokenB = @"access_token_B"; + OIDAuthState *authStateB = [OIDAuthState testInstanceWithIDToken:idTokenB + accessToken:accessTokenB + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:kNewRefreshToken]; + + XCTestExpectation *updateExpectation = [self expectationWithDescription:@"Updates finished"]; + XCTestExpectation *readExpectation = [self expectationWithDescription:@"Reads finished"]; + + dispatch_queue_t updateQueue = dispatch_queue_create("com.google.gidgoogleuser.testUpdateTokens.update", + DISPATCH_QUEUE_CONCURRENT); + dispatch_queue_t readQueue = dispatch_queue_create("com.google.gidgoogleuser.testUpdateTokens.read", + DISPATCH_QUEUE_CONCURRENT); + + NSInteger iterations = 200; + + // Concurrent 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 accessToken and + // idToken must both come from the initial state, both from A, or both from B. + if ([tokens.accessToken.tokenString isEqualToString:accessTokenA]) { + XCTAssertEqualObjects(tokens.idToken.tokenString, idTokenA); + XCTAssertEqualObjects(tokens.refreshToken.tokenString, kNewRefreshToken); + } else if ([tokens.accessToken.tokenString isEqualToString:accessTokenB]) { + XCTAssertEqualObjects(tokens.idToken.tokenString, idTokenB); + XCTAssertEqualObjects(tokens.refreshToken.tokenString, kNewRefreshToken); + } else { + XCTAssertEqualObjects(tokens.accessToken.tokenString, initialAccessToken); + XCTAssertEqualObjects(tokens.idToken.tokenString, initialIDToken); + } + }); + [readExpectation fulfill]; + }); + + [self waitForExpectationsWithTimeout:5 handler:nil]; + + // Final state should be either A or B (whichever ran last) + BOOL matchesA = [user.accessToken.tokenString isEqualToString:accessTokenA] && + [user.idToken.tokenString isEqualToString:idTokenA]; + BOOL matchesB = [user.accessToken.tokenString isEqualToString:accessTokenB] && + [user.idToken.tokenString isEqualToString:idTokenB]; + XCTAssertTrue(matchesA || matchesB); + + // 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 +// without the lock that `-updateTokensWithAuthState:` writes them under. +- (void)testTokenProperties_concurrentUpdatesAndReads { + GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:kAccessTokenExpiresIn + idTokenExpiresIn:kIDTokenExpiresIn]; + + NSString *idTokenA = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn]; + OIDAuthState *authStateA = [OIDAuthState testInstanceWithIDToken:idTokenA + accessToken:@"access_token_A" + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:kNewRefreshToken]; + + NSString *idTokenB = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn + 1]; + OIDAuthState *authStateB = [OIDAuthState testInstanceWithIDToken:idTokenB + accessToken:@"access_token_B" + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:kNewRefreshToken]; + + XCTestExpectation *updateExpectation = [self expectationWithDescription:@"Updates finished"]; + XCTestExpectation *readExpectation = [self expectationWithDescription:@"Reads finished"]; + + dispatch_queue_t updateQueue = + dispatch_queue_create("com.google.gidgoogleuser.testTokenProperties.update", + DISPATCH_QUEUE_CONCURRENT); + dispatch_queue_t readQueue = + dispatch_queue_create("com.google.gidgoogleuser.testTokenProperties.read", + DISPATCH_QUEUE_CONCURRENT); + + size_t iterations = 2000; + + 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]; + }); + + dispatch_async(readQueue, ^{ + dispatch_apply(iterations, readQueue, ^(size_t i) { + (void)user.accessToken.tokenString; + (void)user.refreshToken.tokenString; + (void)user.idToken.tokenString; + }); + [readExpectation fulfill]; + }); + + [self waitForExpectationsWithTimeout:30 handler:nil]; + + XCTAssertNotNil(user.accessToken); +} + +// KVO observers of the token properties run while the user holds its 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 { + GIDGoogleUser *user = [self googleUserWithAccessTokenExpiresIn:kAccessTokenExpiresIn + idTokenExpiresIn:kIDTokenExpiresIn]; + + NSString *idTokenA = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn]; + OIDAuthState *authStateA = [OIDAuthState testInstanceWithIDToken:idTokenA + accessToken:@"access_token_A" + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:kNewRefreshToken]; + + NSString *idTokenB = [self idTokenWithExpiresIn:kNewIDTokenExpiresIn + 1]; + OIDAuthState *authStateB = [OIDAuthState testInstanceWithIDToken:idTokenB + accessToken:@"access_token_B" + accessTokenExpiresIn:kAccessTokenExpiresIn + refreshToken:kNewRefreshToken]; + + NSString *accessTokenKeyPath = NSStringFromSelector(@selector(accessToken)); + GIDGoogleUserTestKVOObserver *observer = [[GIDGoogleUserTestKVOObserver alloc] init]; + __block NSInteger notificationCount = 0; + __weak GIDGoogleUser *weakUser = user; + + observer.onChange = ^{ + notificationCount += 1; + // Only the first notification re-enters, so the re-entrant update cannot recurse forever. + if (notificationCount > 1) { + return; + } + GIDGoogleUser *strongUser = weakUser; + + // These reads take the user's auth state lock again on this thread. + (void)strongUser.grantedScopes; + (void)strongUser.profile; + XCTAssertNotNil(strongUser.configuration); + + // A re-entrant token update from inside the notification. + [strongUser updateWithTokenResponse:authStateB.lastTokenResponse + authorizationResponse:authStateB.lastAuthorizationResponse + profileData:nil]; + }; + + GIDGoogleUserTestKVOObserver *secondObserver = [[GIDGoogleUserTestKVOObserver alloc] init]; + NSMutableArray *observedTokenStrings = [NSMutableArray array]; + secondObserver.onChangeWithDictionary = ^(NSDictionary *change) { + GIDToken *newToken = change[NSKeyValueChangeNewKey]; + if (newToken.tokenString) { + [observedTokenStrings addObject:newToken.tokenString]; + } + }; + + [user addObserver:observer forKeyPath:accessTokenKeyPath options:0 context:NULL]; + [user addObserver:secondObserver + forKeyPath:accessTokenKeyPath + options:NSKeyValueObservingOptionNew + context:NULL]; + + [user updateWithTokenResponse:authStateA.lastTokenResponse + authorizationResponse:authStateA.lastAuthorizationResponse + profileData:nil]; + + [user removeObserver:observer forKeyPath:accessTokenKeyPath context:NULL]; + [user removeObserver:secondObserver forKeyPath:accessTokenKeyPath context:NULL]; + + // Both the outer update and the re-entrant one notify, and the re-entrant one is applied last. + XCTAssertGreaterThanOrEqual(notificationCount, 2); + XCTAssertEqualObjects(user.accessToken.tokenString, @"access_token_B"); + // KVO may deliver the nested B notification to `secondObserver` before A's, so only the last + // recorded value is asserted. + XCTAssertGreaterThanOrEqual(observedTokenStrings.count, (NSUInteger)1); + XCTAssertEqualObjects(observedTokenStrings.lastObject, @"access_token_B"); +} + - (void)testFetcherAuthorizer { // This is really hard to test without assuming how GTMAppAuthFetcherAuthorization works // internally, so let's just take the shortcut here by asserting we get a