Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions GoogleSignIn/Sources/GIDSignIn.m
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,7 @@
// Error string for user cancelations.
static NSString *const kUserCanceledError = @"The user canceled the sign-in flow.";

// User preference key to detect fresh install of the app.
static NSString *const kAppHasRunBeforeKey = @"GID_AppHasRunBefore";
NSString *const kAppHasRunBeforeKey = @"GID_AppHasRunBefore";

// Maximum retry interval in seconds for the fetcher.
static const NSTimeInterval kFetcherMaxRetryInterval = 15.0;
Expand Down Expand Up @@ -672,6 +671,11 @@ - (instancetype)initWithKeychainStore:(GTMKeychainStore *)keychainStore

// Check to see if the 3P app is being run for the first time after a fresh install.
BOOL isFreshInstall = [self isFreshInstall];

// If this is a fresh install, ensure that any pre-existing keychain data is purged.
if (isFreshInstall) {
[self removeAllKeychainEntries];
}

NSString *authorizationEnpointURL = [NSString stringWithFormat:kAuthorizationURLTemplate,
[GIDSignInPreferences googleAuthorizationServer]];
Expand Down
3 changes: 3 additions & 0 deletions GoogleSignIn/Sources/GIDSignIn_Private.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ NS_ASSUME_NONNULL_BEGIN
@class GIDAppCheck;
@class GIDAuthStateMigration;

/// User preference key to detect fresh install of the app.
extern NSString *const kAppHasRunBeforeKey;

/// Represents a completion block that takes a `GIDSignInResult` on success or an error if the
/// operation was unsuccessful.
typedef void (^GIDSignInCompletion)(GIDSignInResult *_Nullable signInResult,
Expand Down
15 changes: 13 additions & 2 deletions GoogleSignIn/Tests/Unit/GIDSignInTest.m
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,6 @@
@"com.google.UnitTests:///emmcallback?action=unrecognized";
static NSString * const kDevicePolicyAppBundleID = @"com.google.DevicePolicy";

static NSString * const kAppHasRunBeforeKey = @"GPP_AppHasRunBefore";

static NSString * const kFingerprintKeychainName = @"fingerprint";
static NSString * const kVerifierKeychainName = @"verifier";
static NSString * const kVerifierKey = @"verifier";
Expand Down Expand Up @@ -1212,6 +1210,19 @@ - (void)testNotHandleWrongPath {
XCTAssertFalse(_completionCalled, @"should not call delegate");
}

#pragma mark - Test Fresh Install

- (void)testFreshInstall_removesKeychainEntries {
// Simulate that the app has been deleted and user defaults removed.
[NSUserDefaults.standardUserDefaults removeObjectForKey:kAppHasRunBeforeKey];
// Initialization should check `isFreshInstall`.
GIDSignIn *signIn = [[GIDSignIn alloc] initWithKeychainStore:_keychainStore
authStateMigrationService:_authStateMigrationService];
// If `isFreshInstall`, keychain entries should be removed.
XCTAssertNotNil(signIn);
XCTAssertTrue(self->_keychainRemoved);
}

#pragma mark - Tests - disconnectWithCallback:

// Verifies disconnect calls callback with no errors if access token is present.
Expand Down
27 changes: 27 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,3 +107,30 @@ let signInButton = GoogleSignInButton {
}
let hostedButton = NSHostingView(rootView: signInButton)
```

## A Note iOS Keychain Access Groups

On iOS, if you do not supply a custom Keychain access group, the system creates
a Keychain access group by prepending `$(AppIdentifierPrefix)` to your bundle
ID (e.g., `$(AppIdentifierPrefix).com.example.MyApp`), which becomes the
default access group for just your app ([Apple documentation](https://developer.apple.com/documentation/security/sharing-access-to-keychain-items-among-a-collection-of-apps#Establish-your-apps-private-access-group)).

If, however, you add a new Keychain access group (and add the entitlement to
your app), then Xcode will use whatever access group is listed first in the
list as the default. So, if the shared access group is first, then it becomes
the default Keychain for your app.

The implication of this scenario is that credentials saved by GSI (via
[GTMAppAuth](https://github.com/google/GTMAppAuth)) on behalf of your app will
be stored in the shared keychain access group.

You should make sure that you want this behavior because GSI [removes Keychain
items upon fresh install](https://github.com/google/GoogleSignIn-iOS/pull/567)
to ensure that stale credentials from previous installs of your app are not
mistakenly used. This behavior can lead new installs of apps sharing the same
Keychain access group to remove Keychain credentials for apps already installed.

You can mitigate this by explicitly listing the typical default access group
(or whatever you prefer) in your list first. GSI, via GTMAppAuth, will then use
that default access group. Make sure that you also update your code that writes
to the Keychain to explicitly use the shared access group as needed.
Loading