From f0812970a668d6dfd31d7723cb7f54d45fd117ec Mon Sep 17 00:00:00 2001 From: Camden King <74014435+camden-king@users.noreply.github.com> Date: Mon, 9 Jun 2025 10:31:09 -0700 Subject: [PATCH 1/4] Attempt migrations once (#528) --- GoogleSignIn/Sources/GIDAuthStateMigration.m | 8 -------- GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m | 2 +- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/GoogleSignIn/Sources/GIDAuthStateMigration.m b/GoogleSignIn/Sources/GIDAuthStateMigration.m index 4df01d11..0ccd979a 100644 --- a/GoogleSignIn/Sources/GIDAuthStateMigration.m +++ b/GoogleSignIn/Sources/GIDAuthStateMigration.m @@ -107,10 +107,6 @@ - (void)performDataProtectedMigrationIfNeeded { if (authSession) { NSError *err; [self.keychainStore saveAuthSession:authSession error:&err]; - // If we're unable to save to the keychain, return without marking migration performed. - if (err) { - return; - }; [keychainStoreLegacy removeAuthSessionWithError:nil]; } @@ -138,10 +134,6 @@ - (void)performGIDMigrationIfNeededWithTokenURL:(NSURL *)tokenURL if (authSession) { NSError *err; [self.keychainStore saveAuthSession:authSession error:&err]; - // If we're unable to save to the keychain, return without marking migration performed. - if (err) { - return; - }; } // Mark the migration check as having been performed. diff --git a/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m b/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m index 6379fb4f..1a482657 100644 --- a/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m +++ b/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m @@ -181,7 +181,7 @@ - (void)testMigrateIfNeeded_KeychainFailure_DataProtectedMigration { callbackPath:kCallbackPath keychainName:kKeychainName isFreshInstall:NO]; - XCTAssertNotNil([_realLegacyGTMKeychainStore retrieveAuthSessionWithError:nil]); + XCTAssertNil([_realLegacyGTMKeychainStore retrieveAuthSessionWithError:nil]); } #else From 0b30eb6bbedeaa6852b3d0b16dfda25be68e1326 Mon Sep 17 00:00:00 2001 From: Camden King <74014435+camden-king@users.noreply.github.com> Date: Wed, 11 Jun 2025 09:14:56 -0700 Subject: [PATCH 2/4] Update GIDAuthStateMigration to not perform migration when built for Mac Catalyst (#526) --- GoogleSignIn/Sources/GIDAuthStateMigration.m | 18 +++++----- .../Tests/Unit/GIDAuthStateMigrationTest.m | 34 +++++++++---------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/GoogleSignIn/Sources/GIDAuthStateMigration.m b/GoogleSignIn/Sources/GIDAuthStateMigration.m index 0ccd979a..cb4cfbe6 100644 --- a/GoogleSignIn/Sources/GIDAuthStateMigration.m +++ b/GoogleSignIn/Sources/GIDAuthStateMigration.m @@ -71,24 +71,24 @@ - (void)migrateIfNeededWithTokenURL:(NSURL *)tokenURL // performed. if (isFreshInstall) { NSUserDefaults* defaults = [NSUserDefaults standardUserDefaults]; -#if TARGET_OS_OSX || TARGET_OS_MACCATALYST +#if TARGET_OS_OSX [defaults setBool:YES forKey:kDataProtectedMigrationCheckPerformedKey]; -#elif TARGET_OS_IOS +#elif TARGET_OS_IOS && !TARGET_OS_MACCATALYST [defaults setBool:YES forKey:kGTMAppAuthMigrationCheckPerformedKey]; -#endif // TARGET_OS_OSX || TARGET_OS_MACCATALYST +#endif // TARGET_OS_OSX return; } -#if TARGET_OS_OSX || TARGET_OS_MACCATALYST +#if TARGET_OS_OSX [self performDataProtectedMigrationIfNeeded]; -#elif TARGET_OS_IOS +#elif TARGET_OS_IOS && !TARGET_OS_MACCATALYST [self performGIDMigrationIfNeededWithTokenURL:tokenURL callbackPath:callbackPath keychainName:keychainName]; -#endif // TARGET_OS_OSX || TARGET_OS_MACCATALYST +#endif // TARGET_OS_OSX } -#if TARGET_OS_OSX || TARGET_OS_MACCATALYST +#if TARGET_OS_OSX // Migrate from the fileBasedKeychain to dataProtectedKeychain with GTMAppAuth 5.0. - (void)performDataProtectedMigrationIfNeeded { // See if we've performed the migration check previously. @@ -114,7 +114,7 @@ - (void)performDataProtectedMigrationIfNeeded { [defaults setBool:YES forKey:kDataProtectedMigrationCheckPerformedKey]; } -#elif TARGET_OS_IOS +#elif TARGET_OS_IOS && !TARGET_OS_MACCATALYST // Migrate from GPPSignIn 1.x or GIDSignIn 1.0 - 4.x to the GTMAppAuth storage introduced in // GIDSignIn 5.0. - (void)performGIDMigrationIfNeededWithTokenURL:(NSURL *)tokenURL @@ -238,7 +238,7 @@ + (nullable NSString *)passwordForService:(NSString *)service { NSString *password = [[NSString alloc] initWithData:passwordData encoding:NSUTF8StringEncoding]; return password; } -#endif // TARGET_OS_OSX || TARGET_OS_MACCATALYST +#endif // TARGET_OS_OSX @end diff --git a/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m b/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m index 1a482657..457b84de 100644 --- a/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m +++ b/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m @@ -16,9 +16,9 @@ #import "GoogleSignIn/Sources/GIDAuthStateMigration.h" #import "GoogleSignIn/Sources/GIDSignInCallbackSchemes.h" -#if TARGET_OS_OSX || TARGET_OS_MACCATALYST +#if TARGET_OS_OSX #import "GoogleSignIn/Tests/Unit/OIDAuthState+Testing.h" -#endif // TARGET_OS_OSX || TARGET_OS_MACCATALYST +#endif // TARGET_OS_OSX @import GTMAppAuth; @@ -84,9 +84,9 @@ @implementation GIDAuthStateMigrationTest { id _mockNSBundle; id _mockGIDSignInCallbackSchemes; id _mockGTMOAuth2Compatibility; -#if TARGET_OS_OSX || TARGET_OS_MACCATALYST +#if TARGET_OS_OSX id _realLegacyGTMKeychainStore; -#endif // TARGET_OS_OSX || TARGET_OS_MACCATALYST +#endif // TARGET_OS_OSX } - (void)setUp { @@ -100,12 +100,12 @@ - (void)setUp { _mockNSBundle = OCMStrictClassMock([NSBundle class]); _mockGIDSignInCallbackSchemes = OCMStrictClassMock([GIDSignInCallbackSchemes class]); _mockGTMOAuth2Compatibility = OCMStrictClassMock([GTMOAuth2Compatibility class]); -#if TARGET_OS_OSX || TARGET_OS_MACCATALYST +#if TARGET_OS_OSX GTMKeychainAttribute *fileBasedKeychain = [GTMKeychainAttribute useFileBasedKeychain]; NSSet *attributes = [NSSet setWithArray:@[fileBasedKeychain]]; _realLegacyGTMKeychainStore = [[GTMKeychainStore alloc] initWithItemName:kKeychainName keychainAttributes:attributes]; -#endif // TARGET_OS_OSX || TARGET_OS_MACCATALYST +#endif // TARGET_OS_OSX } - (void)tearDown { @@ -125,16 +125,16 @@ - (void)tearDown { [_mockGIDSignInCallbackSchemes stopMocking]; [_mockGTMOAuth2Compatibility verify]; [_mockGTMOAuth2Compatibility stopMocking]; -#if TARGET_OS_OSX || TARGET_OS_MACCATALYST +#if TARGET_OS_OSX [_realLegacyGTMKeychainStore removeAuthSessionWithError:nil]; -#endif // TARGET_OS_OSX || TARGET_OS_MACCATALYST +#endif // TARGET_OS_OSX [super tearDown]; } #pragma mark - Tests -#if TARGET_OS_OSX || TARGET_OS_MACCATALYST +#if TARGET_OS_OSX - (void)testMigrateIfNeeded_NoPreviousMigration_DataProtectedMigration { [[[_mockUserDefaults stub] andReturn:_mockUserDefaults] standardUserDefaults]; [[[_mockUserDefaults expect] andReturnValue:@NO] boolForKey:kDataProtectedMigrationCheckPerformedKey]; @@ -184,7 +184,7 @@ - (void)testMigrateIfNeeded_KeychainFailure_DataProtectedMigration { XCTAssertNil([_realLegacyGTMKeychainStore retrieveAuthSessionWithError:nil]); } -#else +#elif TARGET_OS_IOS && !TARGET_OS_MACCATALYST - (void)testMigrateIfNeeded_NoPreviousMigration_GTMAppAuthMigration { [[[_mockUserDefaults stub] andReturn:_mockUserDefaults] standardUserDefaults]; [[[_mockUserDefaults expect] andReturnValue:@NO] boolForKey:kGTMAppAuthMigrationCheckPerformedKey]; @@ -242,17 +242,17 @@ - (void)testExtractAuthorization_HostedDomain { XCTAssertNotNil(authorization); } -#endif // TARGET_OS_OSX || TARGET_OS_MACCATALYST +#endif // TARGET_OS_OSX - (void)testMigrateIfNeeded_HasPreviousMigration { [[[_mockUserDefaults stub] andReturn:_mockUserDefaults] standardUserDefaults]; -#if TARGET_OS_OSX || TARGET_OS_MACCATALYST +#if TARGET_OS_OSX [[[_mockUserDefaults expect] andReturnValue:@YES] boolForKey:kDataProtectedMigrationCheckPerformedKey]; [[_mockUserDefaults reject] setBool:YES forKey:kDataProtectedMigrationCheckPerformedKey]; -#else +#elif TARGET_OS_IOS && !TARGET_OS_MACCATALYST [[[_mockUserDefaults expect] andReturnValue:@YES] boolForKey:kGTMAppAuthMigrationCheckPerformedKey]; [[_mockUserDefaults reject] setBool:YES forKey:kGTMAppAuthMigrationCheckPerformedKey]; -#endif // TARGET_OS_OSX || TARGET_OS_MACCATALYST +#endif // TARGET_OS_OSX GIDAuthStateMigration *migration = [[GIDAuthStateMigration alloc] initWithKeychainStore:_mockGTMKeychainStore]; @@ -264,11 +264,11 @@ - (void)testMigrateIfNeeded_HasPreviousMigration { - (void)testMigrateIfNeeded_isFreshInstall { [[[_mockUserDefaults stub] andReturn:_mockUserDefaults] standardUserDefaults]; -#if TARGET_OS_OSX || TARGET_OS_MACCATALYST +#if TARGET_OS_OSX [[_mockUserDefaults expect] setBool:YES forKey:kDataProtectedMigrationCheckPerformedKey]; -#else +#elif TARGET_OS_IOS && !TARGET_OS_MACCATALYST [[_mockUserDefaults expect] setBool:YES forKey:kGTMAppAuthMigrationCheckPerformedKey]; -#endif // TARGET_OS_OSX || TARGET_OS_MACCATALYST +#endif // TARGET_OS_OSX GIDAuthStateMigration *migration = [[GIDAuthStateMigration alloc] initWithKeychainStore:_mockGTMKeychainStore]; From e81db5b25e37b9e80a02a48fc69cb52cb3900676 Mon Sep 17 00:00:00 2001 From: Camden King <74014435+camden-king@users.noreply.github.com> Date: Wed, 18 Jun 2025 12:47:30 -0700 Subject: [PATCH 3/4] Create GIDFakeAuthStateMigration (#529) --- .../Fake/GIDFakeAuthStateMigration.h | 27 ++++++++++++ .../Fake/GIDFakeAuthStateMigration.m | 42 +++++++++++++++++++ .../GIDAuthStateMigration.h | 0 .../Implementation}/GIDAuthStateMigration.m | 3 +- GoogleSignIn/Sources/GIDSignIn.m | 25 ++++++----- GoogleSignIn/Sources/GIDSignIn_Private.h | 5 ++- .../Tests/Unit/GIDAuthStateMigrationTest.m | 2 +- GoogleSignIn/Tests/Unit/GIDSignInTest.m | 40 +++++++++++++++--- 8 files changed, 125 insertions(+), 19 deletions(-) create mode 100644 GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.h create mode 100644 GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.m rename GoogleSignIn/Sources/{ => GIDAuthStateMigration}/GIDAuthStateMigration.h (100%) rename GoogleSignIn/Sources/{ => GIDAuthStateMigration/Implementation}/GIDAuthStateMigration.m (99%) diff --git a/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.h b/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.h new file mode 100644 index 00000000..fc611e53 --- /dev/null +++ b/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.h @@ -0,0 +1,27 @@ +/* + * Copyright 2025 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import "GoogleSignIn/Sources/GIDAuthStateMigration/GIDAuthStateMigration.h" + +/// A fake |GIDAuthStateMigration| for testing. +@interface GIDFakeAuthStateMigration : GIDAuthStateMigration + +/// Callback that is called when `migrateIfNeededWithTokenURL` is invoked. +@property (nonatomic, nullable) void (^migrationInvokedCallback) + (NSURL * _Nullable tokenURL, NSString * _Nullable callbackPath, NSString * _Nullable keychainName, + BOOL isFreshInstall); + +@end diff --git a/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.m b/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.m new file mode 100644 index 00000000..0346dba1 --- /dev/null +++ b/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.m @@ -0,0 +1,42 @@ +/* + * Copyright 2025 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import "GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.h" + +NS_ASSUME_NONNULL_BEGIN + +@implementation GIDFakeAuthStateMigration + +@synthesize migrationInvokedCallback = _migrationInvokedCallback; + +- (instancetype)initWithKeychainStore:(GTMKeychainStore *)keychainStore { + self = [super initWithKeychainStore:keychainStore]; + return self; +} + +- (void)migrateIfNeededWithTokenURL:(NSURL *)tokenURL + callbackPath:(NSString *)callbackPath + keychainName:(NSString *)keychainName + isFreshInstall:(BOOL)isFreshInstall { + if (_migrationInvokedCallback) { + _migrationInvokedCallback(tokenURL, callbackPath, keychainName, isFreshInstall); + } + return; +} + +@end + +NS_ASSUME_NONNULL_END diff --git a/GoogleSignIn/Sources/GIDAuthStateMigration.h b/GoogleSignIn/Sources/GIDAuthStateMigration/GIDAuthStateMigration.h similarity index 100% rename from GoogleSignIn/Sources/GIDAuthStateMigration.h rename to GoogleSignIn/Sources/GIDAuthStateMigration/GIDAuthStateMigration.h diff --git a/GoogleSignIn/Sources/GIDAuthStateMigration.m b/GoogleSignIn/Sources/GIDAuthStateMigration/Implementation/GIDAuthStateMigration.m similarity index 99% rename from GoogleSignIn/Sources/GIDAuthStateMigration.m rename to GoogleSignIn/Sources/GIDAuthStateMigration/Implementation/GIDAuthStateMigration.m index cb4cfbe6..b5c21773 100644 --- a/GoogleSignIn/Sources/GIDAuthStateMigration.m +++ b/GoogleSignIn/Sources/GIDAuthStateMigration/Implementation/GIDAuthStateMigration.m @@ -12,8 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -#import "GoogleSignIn/Sources/GIDAuthStateMigration.h" - +#import "GoogleSignIn/Sources/GIDAuthStateMigration/GIDAuthStateMigration.h" #import "GoogleSignIn/Sources/GIDSignInCallbackSchemes.h" @import GTMAppAuth; diff --git a/GoogleSignIn/Sources/GIDSignIn.m b/GoogleSignIn/Sources/GIDSignIn.m index 640761e1..ef7ac3eb 100644 --- a/GoogleSignIn/Sources/GIDSignIn.m +++ b/GoogleSignIn/Sources/GIDSignIn.m @@ -21,7 +21,7 @@ #import "GoogleSignIn/Sources/Public/GoogleSignIn/GIDProfileData.h" #import "GoogleSignIn/Sources/Public/GoogleSignIn/GIDSignInResult.h" -#import "GoogleSignIn/Sources/GIDAuthStateMigration.h" +#import "GoogleSignIn/Sources/GIDAuthStateMigration/GIDAuthStateMigration.h" #import "GoogleSignIn/Sources/GIDEMMSupport.h" #import "GoogleSignIn/Sources/GIDSignInInternalOptions.h" #import "GoogleSignIn/Sources/GIDSignInPreferences.h" @@ -490,15 +490,19 @@ + (GIDSignIn *)sharedInstance { dispatch_once(&once, ^{ GTMKeychainStore *keychainStore = [[GTMKeychainStore alloc] initWithItemName:kGTMAppAuthKeychainName]; + GIDAuthStateMigration *authStateMigrationService = + [[GIDAuthStateMigration alloc] initWithKeychainStore:keychainStore]; #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST if (@available(iOS 14.0, *)) { GIDAppCheck *appCheck = [GIDAppCheck appCheckUsingAppAttestProvider]; sharedInstance = [[self alloc] initWithKeychainStore:keychainStore + authStateMigrationService:authStateMigrationService appCheck:appCheck]; } #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST if (!sharedInstance) { - sharedInstance = [[self alloc] initWithKeychainStore:keychainStore]; + sharedInstance = [[self alloc] initWithKeychainStore:keychainStore + authStateMigrationService:authStateMigrationService]; } }); return sharedInstance; @@ -533,7 +537,8 @@ - (void)configureDebugProviderWithAPIKey:(NSString *)APIKey #pragma mark - Private methods -- (instancetype)initWithKeychainStore:(GTMKeychainStore *)keychainStore { +- (instancetype)initWithKeychainStore:(GTMKeychainStore *)keychainStore + authStateMigrationService:(GIDAuthStateMigration *)authStateMigrationService { self = [super init]; if (self) { // Get the bundle of the current executable. @@ -561,20 +566,20 @@ - (instancetype)initWithKeychainStore:(GTMKeychainStore *)keychainStore { tokenEndpoint:[NSURL URLWithString:tokenEndpointURL]]; _keychainStore = keychainStore; // Perform migration of auth state from old versions of the SDK if needed. - GIDAuthStateMigration *migration = - [[GIDAuthStateMigration alloc] initWithKeychainStore:_keychainStore]; - [migration migrateIfNeededWithTokenURL:_appAuthConfiguration.tokenEndpoint - callbackPath:kBrowserCallbackPath - keychainName:kGTMAppAuthKeychainName - isFreshInstall:isFreshInstall]; + [authStateMigrationService migrateIfNeededWithTokenURL:_appAuthConfiguration.tokenEndpoint + callbackPath:kBrowserCallbackPath + keychainName:kGTMAppAuthKeychainName + isFreshInstall:isFreshInstall]; } return self; } #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST - (instancetype)initWithKeychainStore:(GTMKeychainStore *)keychainStore + authStateMigrationService:(GIDAuthStateMigration *)authStateMigrationService appCheck:(GIDAppCheck *)appCheck { - self = [self initWithKeychainStore:keychainStore]; + self = [self initWithKeychainStore:keychainStore + authStateMigrationService:authStateMigrationService]; if (self) { _appCheck = appCheck; _configureAppCheckCalled = NO; diff --git a/GoogleSignIn/Sources/GIDSignIn_Private.h b/GoogleSignIn/Sources/GIDSignIn_Private.h index 4072a4a0..bb642cb7 100644 --- a/GoogleSignIn/Sources/GIDSignIn_Private.h +++ b/GoogleSignIn/Sources/GIDSignIn_Private.h @@ -30,6 +30,7 @@ NS_ASSUME_NONNULL_BEGIN @class GIDSignInInternalOptions; @class GTMKeychainStore; @class GIDAppCheck; +@class GIDAuthStateMigration; /// Represents a completion block that takes a `GIDSignInResult` on success or an error if the /// operation was unsuccessful. @@ -46,11 +47,13 @@ typedef void (^GIDDisconnectCompletion)(NSError *_Nullable error); @property(nonatomic, readwrite, nullable) GIDGoogleUser *currentUser; /// Private initializer taking a `GTMKeychainStore`. -- (instancetype)initWithKeychainStore:(GTMKeychainStore *)keychainStore; +- (instancetype)initWithKeychainStore:(GTMKeychainStore *)keychainStore + authStateMigrationService:(GIDAuthStateMigration *)authStateMigrationService; #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST /// Private initializer taking a `GTMKeychainStore` and `GIDAppCheckProvider`. - (instancetype)initWithKeychainStore:(GTMKeychainStore *)keychainStore + authStateMigrationService:(GIDAuthStateMigration *)authStateMigrationService appCheck:(GIDAppCheck *)appCheck API_AVAILABLE(ios(14)); #endif // TARGET_OS_IOS || !TARGET_OS_MACCATALYST diff --git a/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m b/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m index 457b84de..20b16451 100644 --- a/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m +++ b/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m @@ -14,7 +14,7 @@ #import -#import "GoogleSignIn/Sources/GIDAuthStateMigration.h" +#import "GoogleSignIn/Sources/GIDAuthStateMigration/GIDAuthStateMigration.h" #import "GoogleSignIn/Sources/GIDSignInCallbackSchemes.h" #if TARGET_OS_OSX #import "GoogleSignIn/Tests/Unit/OIDAuthState+Testing.h" diff --git a/GoogleSignIn/Tests/Unit/GIDSignInTest.m b/GoogleSignIn/Tests/Unit/GIDSignInTest.m index 7d5195d8..eb367322 100644 --- a/GoogleSignIn/Tests/Unit/GIDSignInTest.m +++ b/GoogleSignIn/Tests/Unit/GIDSignInTest.m @@ -40,6 +40,7 @@ #import "GoogleSignIn/Sources/GIDEMMErrorHandler.h" #endif // TARGET_OS_IOS && !TARGET_OS_MACCATALYST +#import "GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.h" #import "GoogleSignIn/Tests/Unit/GIDFakeFetcher.h" #import "GoogleSignIn/Tests/Unit/GIDFakeFetcherService.h" #import "GoogleSignIn/Tests/Unit/GIDFakeMainBundle.h" @@ -221,6 +222,9 @@ @interface GIDSignInTest : XCTestCase { // Whether callback block has been called. BOOL _completionCalled; + // Fake for |GIDAuthStateMigration|. + GIDFakeAuthStateMigration *_authStateMigrationService; + // Fake fetcher service to emulate network requests. GIDFakeFetcherService *_fetcherService; @@ -331,6 +335,7 @@ - (void)setUp { callback:COPY_TO_ARG_BLOCK(self->_savedTokenCallback)]); // Fakes + _authStateMigrationService = [[GIDFakeAuthStateMigration alloc] init]; _fetcherService = [[GIDFakeFetcherService alloc] init]; _fakeMainBundle = [[GIDFakeMainBundle alloc] init]; [_fakeMainBundle startFakingWithClientID:kClientId]; @@ -339,7 +344,8 @@ - (void)setUp { // Object under test [[NSUserDefaults standardUserDefaults] setBool:YES forKey:kAppHasRunBeforeKey]; - _signIn = [[GIDSignIn alloc] initWithKeychainStore:_keychainStore]; + _signIn = [[GIDSignIn alloc] initWithKeychainStore:_keychainStore + authStateMigrationService:_authStateMigrationService]; _hint = nil; #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST @@ -398,6 +404,7 @@ - (void)testConfigureSucceeds { userDefaults:_testUserDefaults]; GIDSignIn *signIn = [[GIDSignIn alloc] initWithKeychainStore:_keychainStore + authStateMigrationService:_authStateMigrationService appCheck:appCheck]; [signIn configureWithCompletion:^(NSError * _Nullable error) { XCTAssertNil(error); @@ -421,6 +428,7 @@ - (void)testConfigureFailsNoTokenOrError { userDefaults:_testUserDefaults]; GIDSignIn *signIn = [[GIDSignIn alloc] initWithKeychainStore:_keychainStore + authStateMigrationService:_authStateMigrationService appCheck:appCheck]; // Should fail if missing both token and error @@ -439,7 +447,8 @@ - (void)testConfigureFailsNoTokenOrError { - (void)testInitWithKeychainStore { GTMKeychainStore *store = [[GTMKeychainStore alloc] initWithItemName:@"foo"]; GIDSignIn *signIn; - signIn = [[GIDSignIn alloc] initWithKeychainStore:store]; + signIn = [[GIDSignIn alloc] initWithKeychainStore:store + authStateMigrationService:_authStateMigrationService]; XCTAssertNotNil(signIn.configuration); XCTAssertEqual(signIn.configuration.clientID, kClientId); XCTAssertNil(signIn.configuration.serverClientID); @@ -454,7 +463,8 @@ - (void)testInitWithKeychainStore_noConfig { openIDRealm:nil]; GTMKeychainStore *store = [[GTMKeychainStore alloc] initWithItemName:@"foo"]; GIDSignIn *signIn; - signIn = [[GIDSignIn alloc] initWithKeychainStore:store]; + signIn = [[GIDSignIn alloc] initWithKeychainStore:store + authStateMigrationService:_authStateMigrationService]; XCTAssertNil(signIn.configuration); } @@ -466,7 +476,8 @@ - (void)testInitWithKeychainStore_fullConfig { GTMKeychainStore *store = [[GTMKeychainStore alloc] initWithItemName:@"foo"]; GIDSignIn *signIn; - signIn = [[GIDSignIn alloc] initWithKeychainStore:store]; + signIn = [[GIDSignIn alloc] initWithKeychainStore:store + authStateMigrationService:_authStateMigrationService]; XCTAssertNotNil(signIn.configuration); XCTAssertEqual(signIn.configuration.clientID, kClientId); XCTAssertEqual(signIn.configuration.serverClientID, kServerClientId); @@ -481,10 +492,29 @@ - (void)testInitWithKeychainStore_invalidConfig { openIDRealm:nil]; GTMKeychainStore *store = [[GTMKeychainStore alloc] initWithItemName:@"foo"]; GIDSignIn *signIn; - signIn = [[GIDSignIn alloc] initWithKeychainStore:store]; + signIn = [[GIDSignIn alloc] initWithKeychainStore:store + authStateMigrationService:_authStateMigrationService]; XCTAssertNil(signIn.configuration); } +- (void)testInitWithKeychainStore_attemptsMigration { + NSString *expectedKeychainName = @"foo"; + + XCTestExpectation *expectation = [self expectationWithDescription:@"Callback should be called."]; + _authStateMigrationService.migrationInvokedCallback = + ^(NSURL *tokenURL, NSString *callbackPath, NSString *keychainName, BOOL isFreshInstall) { + XCTAssertFalse(isFreshInstall); + [expectation fulfill]; + }; + + GTMKeychainStore *store = [[GTMKeychainStore alloc] initWithItemName:expectedKeychainName]; + GIDSignIn *signIn = [[GIDSignIn alloc] initWithKeychainStore:store + authStateMigrationService:_authStateMigrationService]; + + XCTAssertNotNil(signIn.configuration); + [self waitForExpectationsWithTimeout:1 handler:nil]; +} + - (void)testRestorePreviousSignInNoRefresh_hasPreviousUser { [[[_authorization stub] andReturn:_authState] authState]; #if TARGET_OS_IOS && !TARGET_OS_MACCATALYST From 5b30eb9ee7d807161048ffd75610abb406c249e9 Mon Sep 17 00:00:00 2001 From: Camden King <74014435+camden-king@users.noreply.github.com> Date: Wed, 18 Jun 2025 14:12:53 -0700 Subject: [PATCH 4/4] Remove keychain name param (#530) --- .../GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.h | 3 +-- .../GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.m | 3 +-- .../Sources/GIDAuthStateMigration/GIDAuthStateMigration.h | 1 - .../Implementation/GIDAuthStateMigration.m | 7 ++----- GoogleSignIn/Sources/GIDSignIn.m | 1 - GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m | 6 ------ GoogleSignIn/Tests/Unit/GIDSignInTest.m | 6 ++---- 7 files changed, 6 insertions(+), 21 deletions(-) diff --git a/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.h b/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.h index fc611e53..24555f30 100644 --- a/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.h +++ b/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.h @@ -21,7 +21,6 @@ /// Callback that is called when `migrateIfNeededWithTokenURL` is invoked. @property (nonatomic, nullable) void (^migrationInvokedCallback) - (NSURL * _Nullable tokenURL, NSString * _Nullable callbackPath, NSString * _Nullable keychainName, - BOOL isFreshInstall); + (NSURL * _Nullable tokenURL, NSString * _Nullable callbackPath, BOOL isFreshInstall); @end diff --git a/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.m b/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.m index 0346dba1..b753b814 100644 --- a/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.m +++ b/GoogleSignIn/Sources/GIDAuthStateMigration/Fake/GIDFakeAuthStateMigration.m @@ -29,10 +29,9 @@ - (instancetype)initWithKeychainStore:(GTMKeychainStore *)keychainStore { - (void)migrateIfNeededWithTokenURL:(NSURL *)tokenURL callbackPath:(NSString *)callbackPath - keychainName:(NSString *)keychainName isFreshInstall:(BOOL)isFreshInstall { if (_migrationInvokedCallback) { - _migrationInvokedCallback(tokenURL, callbackPath, keychainName, isFreshInstall); + _migrationInvokedCallback(tokenURL, callbackPath, isFreshInstall); } return; } diff --git a/GoogleSignIn/Sources/GIDAuthStateMigration/GIDAuthStateMigration.h b/GoogleSignIn/Sources/GIDAuthStateMigration/GIDAuthStateMigration.h index 738368fc..3f246b77 100644 --- a/GoogleSignIn/Sources/GIDAuthStateMigration/GIDAuthStateMigration.h +++ b/GoogleSignIn/Sources/GIDAuthStateMigration/GIDAuthStateMigration.h @@ -30,7 +30,6 @@ NS_ASSUME_NONNULL_BEGIN /// Perform necessary migrations from legacy auth state storage to most recent GTMAppAuth version. - (void)migrateIfNeededWithTokenURL:(NSURL *)tokenURL callbackPath:(NSString *)callbackPath - keychainName:(NSString *)keychainName isFreshInstall:(BOOL)isFreshInstall; @end diff --git a/GoogleSignIn/Sources/GIDAuthStateMigration/Implementation/GIDAuthStateMigration.m b/GoogleSignIn/Sources/GIDAuthStateMigration/Implementation/GIDAuthStateMigration.m index b5c21773..e9c1c107 100644 --- a/GoogleSignIn/Sources/GIDAuthStateMigration/Implementation/GIDAuthStateMigration.m +++ b/GoogleSignIn/Sources/GIDAuthStateMigration/Implementation/GIDAuthStateMigration.m @@ -64,7 +64,6 @@ - (instancetype)init { - (void)migrateIfNeededWithTokenURL:(NSURL *)tokenURL callbackPath:(NSString *)callbackPath - keychainName:(NSString *)keychainName isFreshInstall:(BOOL)isFreshInstall { // If this is a fresh install, take no action and mark the migration checks as having been // performed. @@ -82,8 +81,7 @@ - (void)migrateIfNeededWithTokenURL:(NSURL *)tokenURL [self performDataProtectedMigrationIfNeeded]; #elif TARGET_OS_IOS && !TARGET_OS_MACCATALYST [self performGIDMigrationIfNeededWithTokenURL:tokenURL - callbackPath:callbackPath - keychainName:keychainName]; + callbackPath:callbackPath]; #endif // TARGET_OS_OSX } @@ -117,8 +115,7 @@ - (void)performDataProtectedMigrationIfNeeded { // Migrate from GPPSignIn 1.x or GIDSignIn 1.0 - 4.x to the GTMAppAuth storage introduced in // GIDSignIn 5.0. - (void)performGIDMigrationIfNeededWithTokenURL:(NSURL *)tokenURL - callbackPath:(NSString *)callbackPath - keychainName:(NSString *)keychainName { + callbackPath:(NSString *)callbackPath { // See if we've performed the migration check previously. NSUserDefaults* defaults = [NSUserDefaults standardUserDefaults]; if ([defaults boolForKey:kGTMAppAuthMigrationCheckPerformedKey]) { diff --git a/GoogleSignIn/Sources/GIDSignIn.m b/GoogleSignIn/Sources/GIDSignIn.m index ef7ac3eb..48b6d749 100644 --- a/GoogleSignIn/Sources/GIDSignIn.m +++ b/GoogleSignIn/Sources/GIDSignIn.m @@ -568,7 +568,6 @@ - (instancetype)initWithKeychainStore:(GTMKeychainStore *)keychainStore // Perform migration of auth state from old versions of the SDK if needed. [authStateMigrationService migrateIfNeededWithTokenURL:_appAuthConfiguration.tokenEndpoint callbackPath:kBrowserCallbackPath - keychainName:kGTMAppAuthKeychainName isFreshInstall:isFreshInstall]; } return self; diff --git a/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m b/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m index 20b16451..1504224a 100644 --- a/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m +++ b/GoogleSignIn/Tests/Unit/GIDAuthStateMigrationTest.m @@ -154,7 +154,6 @@ - (void)testMigrateIfNeeded_NoPreviousMigration_DataProtectedMigration { [[GIDAuthStateMigration alloc] initWithKeychainStore:_mockGTMKeychainStore]; [migration migrateIfNeededWithTokenURL:[NSURL URLWithString:kTokenURL] callbackPath:kCallbackPath - keychainName:kKeychainName isFreshInstall:NO]; // verify that the auth session was removed during migration @@ -179,7 +178,6 @@ - (void)testMigrateIfNeeded_KeychainFailure_DataProtectedMigration { [[GIDAuthStateMigration alloc] initWithKeychainStore:_mockGTMKeychainStore]; [migration migrateIfNeededWithTokenURL:[NSURL URLWithString:kTokenURL] callbackPath:kCallbackPath - keychainName:kKeychainName isFreshInstall:NO]; XCTAssertNil([_realLegacyGTMKeychainStore retrieveAuthSessionWithError:nil]); } @@ -198,7 +196,6 @@ - (void)testMigrateIfNeeded_NoPreviousMigration_GTMAppAuthMigration { [[GIDAuthStateMigration alloc] initWithKeychainStore:_mockGTMKeychainStore]; [migration migrateIfNeededWithTokenURL:[NSURL URLWithString:kTokenURL] callbackPath:kCallbackPath - keychainName:kKeychainName isFreshInstall:NO]; } @@ -215,7 +212,6 @@ - (void)testMigrateIfNeeded_KeychainFailure_GTMAppAuthMigration { [[GIDAuthStateMigration alloc] initWithKeychainStore:_mockGTMKeychainStore]; [migration migrateIfNeededWithTokenURL:[NSURL URLWithString:kTokenURL] callbackPath:kCallbackPath - keychainName:kKeychainName isFreshInstall:NO]; } @@ -258,7 +254,6 @@ - (void)testMigrateIfNeeded_HasPreviousMigration { [[GIDAuthStateMigration alloc] initWithKeychainStore:_mockGTMKeychainStore]; [migration migrateIfNeededWithTokenURL:[NSURL URLWithString:kTokenURL] callbackPath:kCallbackPath - keychainName:kKeychainName isFreshInstall:NO]; } @@ -274,7 +269,6 @@ - (void)testMigrateIfNeeded_isFreshInstall { [[GIDAuthStateMigration alloc] initWithKeychainStore:_mockGTMKeychainStore]; [migration migrateIfNeededWithTokenURL:[NSURL URLWithString:kTokenURL] callbackPath:kCallbackPath - keychainName:kKeychainName isFreshInstall:YES]; } diff --git a/GoogleSignIn/Tests/Unit/GIDSignInTest.m b/GoogleSignIn/Tests/Unit/GIDSignInTest.m index eb367322..71974e0f 100644 --- a/GoogleSignIn/Tests/Unit/GIDSignInTest.m +++ b/GoogleSignIn/Tests/Unit/GIDSignInTest.m @@ -498,16 +498,14 @@ - (void)testInitWithKeychainStore_invalidConfig { } - (void)testInitWithKeychainStore_attemptsMigration { - NSString *expectedKeychainName = @"foo"; - XCTestExpectation *expectation = [self expectationWithDescription:@"Callback should be called."]; _authStateMigrationService.migrationInvokedCallback = - ^(NSURL *tokenURL, NSString *callbackPath, NSString *keychainName, BOOL isFreshInstall) { + ^(NSURL *tokenURL, NSString *callbackPath, BOOL isFreshInstall) { XCTAssertFalse(isFreshInstall); [expectation fulfill]; }; - GTMKeychainStore *store = [[GTMKeychainStore alloc] initWithItemName:expectedKeychainName]; + GTMKeychainStore *store = [[GTMKeychainStore alloc] initWithItemName:kKeychainName]; GIDSignIn *signIn = [[GIDSignIn alloc] initWithKeychainStore:store authStateMigrationService:_authStateMigrationService];