Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[google_sign_in_ios] Upgrade GoogleSignIn iOS SDK to 7.0 #5240

Merged
merged 9 commits into from
Nov 7, 2023
Merged

[google_sign_in_ios] Upgrade GoogleSignIn iOS SDK to 7.0 #5240

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
067f3fe
upgrade Google Sign In to 7.0 for iOS
vashworth Oct 26, 2023
bb30b60
update changelog and version
vashworth Oct 26, 2023
815fd8f
remove comment and make match
vashworth Oct 27, 2023
177f2f6
Update README, update example app to use recommended settings, add va…
vashworth Nov 3, 2023
57a2b31
update README again
vashworth Nov 3, 2023
798fc19
fix format
vashworth Nov 3, 2023
04fe8a7
add changelog, update false secrets, use code-excerpt
vashworth Nov 3, 2023
f66c4cb
fix more repo checks
vashworth Nov 6, 2023
f366a3e
fix nits
vashworth Nov 7, 2023
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
4 changes: 4 additions & 0 deletions packages/google_sign_in/google_sign_in_ios/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 5.6.5

* Upgrades GoogleSignIn iOS SDK to 7.0.

## 5.6.4

* Converts platform communication to Pigeon.
Expand Down
274 changes: 153 additions & 121 deletions packages/google_sign_in/google_sign_in_ios/example/ios/RunnerTests/GoogleSignInTests.m
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 8 additions & 0 deletions packages/google_sign_in/google_sign_in_ios/ios/Classes/FLTGoogleSignInPlugin.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,13 @@

#import "messages.g.h"

#import <GoogleSignIn/GoogleSignIn.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?


@interface FLTGoogleSignInPlugin : NSObject <FlutterPlugin, FSIGoogleSignInApi>

// Configuration wrapping Google Cloud Console, Google Apps, OpenID,
// and other initialization metadata.
// @property(strong) GIDConfiguration *configuration;
@property(strong, nonatomic) GIDConfiguration *configuration;

@end
125 changes: 71 additions & 54 deletions packages/google_sign_in/google_sign_in_ios/ios/Classes/FLTGoogleSignInPlugin.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@

@interface FLTGoogleSignInPlugin ()

// Configuration wrapping Google Cloud Console, Google Apps, OpenID,
// and other initialization metadata.
@property(strong) GIDConfiguration *configuration;

// Permissions requested during at sign in "init" method call
// unioned with scopes requested later with incremental authorization
// "requestScopes" method call.
Expand Down Expand Up @@ -116,18 +112,14 @@ - (void)initializeSignInWithParameters:(nonnull FSIInitParams *)params
if (configuration != nil) {
self.requestedScopes = [NSSet setWithArray:params.scopes];
self.configuration = configuration;
} else {
*error = [FlutterError errorWithCode:@"missing-config"
message:@"GoogleService-Info.plist file not found and clientId "
@"was not provided programmatically."
details:nil];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Google Sign In SDK now gets the configuration settings within the SDK itself from the Info.plist and that's now the preferred way to do it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove the GoogleService-Info.plist from the example then? We generally want the example to show the preferred approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we should put an updated version of https://pub.dev/packages/google_sign_in#ios-integration into this package's README, and then update the main package README to point to that for iOS integration details. That's the pattern we are increasingly using for non-trivial setup docs in federated plugins (due to issues like this, where the setup steps actually depend on the version of the sub-package you are using).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I moved GoogleService-Info.plist from the Runner target to the RunnerTests target, so we can still use it for the example tests.

}
}

- (void)signInSilentlyWithCompletion:(nonnull void (^)(FSIUserData *_Nullable,
FlutterError *_Nullable))completion {
[self.signIn restorePreviousSignInWithCallback:^(GIDGoogleUser *user, NSError *error) {
[self didSignInForUser:user withCompletion:completion error:error];
[self.signIn restorePreviousSignInWithCompletion:^(GIDGoogleUser *_Nullable user,
NSError *_Nullable error) {
[self didSignInForUser:user serverAuthCode:nil withCompletion:completion error:error];
Copy link
Contributor Author

@vashworth vashworth Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may notice here I set serverAuthCode to nil. This is something I was unsure about. In previous version, the serverAuthCode was a property of the user. However, in the new version, it's now a property of GIDSignInResult, which you get when you sign in but not restore. I did confirm that in the old version on restore, the serverAuthCode was included.

However, I think the new way where it doesn't include the serverAuthCode is more inline with how OAuth works. Usually that code is a temporary one-time use code and then after that tokens are use to validate (see diagram in https://developers.google.com/identity/sign-in/web/server-side-flow).

}];
}

Expand All @@ -139,19 +131,36 @@ - (nullable NSNumber *)isSignedInWithError:
- (void)signInWithCompletion:(nonnull void (^)(FSIUserData *_Nullable,
FlutterError *_Nullable))completion {
@try {
GIDConfiguration *configuration = self.configuration
?: [self configurationWithClientIdArgument:nil
serverClientIdArgument:nil
hostedDomainArgument:nil];
[self.signIn signInWithConfiguration:configuration
presentingViewController:[self topViewController]
hint:nil
additionalScopes:self.requestedScopes.allObjects
callback:^(GIDGoogleUser *user, NSError *error) {
[self didSignInForUser:user
withCompletion:completion
error:error];
}];
// If the configuration settings are passed from the Dart API, use those.
// Otherwise, use settings from the GoogleService-Info.plist if available.
// If neither are available, do not set the configuration - GIDSignIn will automatically use
// settings from the Info.plist (which is the recommended method).
if (!self.configuration && self.googleServiceProperties) {
self.configuration = [self configurationWithClientIdArgument:nil
serverClientIdArgument:nil
hostedDomainArgument:nil];
}
if (self.configuration) {
self.signIn.configuration = self.configuration;
}

[self.signIn signInWithPresentingViewController:[self topViewController]
hint:nil
additionalScopes:self.requestedScopes.allObjects
completion:^(GIDSignInResult *_Nullable signInResult,
NSError *_Nullable error) {
GIDGoogleUser *user;
NSString *serverAuthCode;
if (signInResult) {
user = signInResult.user;
serverAuthCode = signInResult.serverAuthCode;
}

[self didSignInForUser:user
serverAuthCode:serverAuthCode
withCompletion:completion
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the with should go on the first additional argument, so didSignInForUser:withServerAuthCode:completion:error:.

error:error];
}];
} @catch (NSException *e) {
completion(nil, [FlutterError errorWithCode:@"google_sign_in" message:e.reason details:e.name]);
[e raise];
Expand All @@ -161,13 +170,13 @@ - (void)signInWithCompletion:(nonnull void (^)(FSIUserData *_Nullable,
- (void)getAccessTokenWithCompletion:(nonnull void (^)(FSITokenData *_Nullable,
FlutterError *_Nullable))completion {
GIDGoogleUser *currentUser = self.signIn.currentUser;
GIDAuthentication *auth = currentUser.authentication;
[auth doWithFreshTokens:^void(GIDAuthentication *authentication, NSError *error) {
[currentUser refreshTokensIfNeededWithCompletion:^(GIDGoogleUser *_Nullable user,
NSError *_Nullable error) {
if (error) {
completion(nil, getFlutterError(error));
} else {
completion([FSITokenData makeWithIdToken:authentication.idToken
accessToken:authentication.accessToken],
completion([FSITokenData makeWithIdToken:user.idToken.tokenString
accessToken:user.accessToken.tokenString],
nil);
}
}];
Expand All @@ -177,7 +186,7 @@ - (void)signOutWithError:(FlutterError *_Nullable *_Nonnull)error;
{ [self.signIn signOut]; }

- (void)disconnectWithCompletion:(nonnull void (^)(FlutterError *_Nullable))completion {
[self.signIn disconnectWithCallback:^(NSError *error) {
[self.signIn disconnectWithCompletion:^(NSError *_Nullable error) {
// TODO(stuartmorgan): This preserves the pre-Pigeon-migration behavior, but it's unclear why
// 'error' is being ignored here.
completion(nil);
Expand All @@ -190,31 +199,38 @@ - (void)requestScopes:(nonnull NSArray<NSString *> *)scopes
NSSet<NSString *> *requestedScopes = self.requestedScopes;

@try {
[self.signIn addScopes:requestedScopes.allObjects
GIDGoogleUser *currentUser = self.signIn.currentUser;
if (currentUser == nil) {
completion(nil, [FlutterError errorWithCode:@"sign_in_required"
message:@"No account to grant scopes."
details:nil]);
}
[currentUser addScopes:requestedScopes.allObjects
presentingViewController:[self topViewController]
callback:^(GIDGoogleUser *addedScopeUser, NSError *addedScopeError) {
BOOL granted = NO;
FlutterError *error = nil;
if ([addedScopeError.domain isEqualToString:kGIDSignInErrorDomain] &&
addedScopeError.code == kGIDSignInErrorCodeNoCurrentUser) {
error = [FlutterError errorWithCode:@"sign_in_required"
message:@"No account to grant scopes."
details:nil];
} else if ([addedScopeError.domain
isEqualToString:kGIDSignInErrorDomain] &&
addedScopeError.code ==
kGIDSignInErrorCodeScopesAlreadyGranted) {
// Scopes already granted, report success.
granted = YES;
} else if (addedScopeUser == nil) {
granted = NO;
} else {
NSSet<NSString *> *grantedScopes =
[NSSet setWithArray:addedScopeUser.grantedScopes];
granted = [requestedScopes isSubsetOfSet:grantedScopes];
}
completion(error == nil ? @(granted) : nil, error);
}];
completion:^(GIDSignInResult *_Nullable signInResult,
NSError *_Nullable addedScopeError) {
BOOL granted = NO;
FlutterError *error = nil;

if ([addedScopeError.domain isEqualToString:kGIDSignInErrorDomain] &&
addedScopeError.code == kGIDSignInErrorCodeMismatchWithCurrentUser) {
error =
[FlutterError errorWithCode:@"request_scopes"
vashworth marked this conversation as resolved.
Show resolved Hide resolved
message:@"There is an operation on a previous "
@"user. Try signing in again."
details:nil];
Copy link
Contributor Author

@vashworth vashworth Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, they removed kGIDSignInErrorCodeNoCurrentUser as an error, I'm assuming because the method is now part of the GIDGoogleUser class, so there has to be a user. They also added a new error kGIDSignInErrorCodeMismatchWithCurrentUser, which I believe happens when GIDSignIn.currentUser doesn't match the user that makes this request. This shouldn't ever happen since we get the user from self.signIn.currentUser, but I added a new error just in case.

} else if ([addedScopeError.domain isEqualToString:kGIDSignInErrorDomain] &&
addedScopeError.code ==
kGIDSignInErrorCodeScopesAlreadyGranted) {
// Scopes already granted, report success.
granted = YES;
} else if (signInResult.user) {
NSSet<NSString *> *grantedScopes =
[NSSet setWithArray:signInResult.user.grantedScopes];
granted = [requestedScopes isSubsetOfSet:grantedScopes];
}
completion(error == nil ? @(granted) : nil, error);
}];
} @catch (NSException *e) {
completion(nil, [FlutterError errorWithCode:@"request_scopes" message:e.reason details:e.name]);
}
Expand Down Expand Up @@ -265,6 +281,7 @@ - (GIDConfiguration *)configurationWithClientIdArgument:(id)clientIDArg
}

- (void)didSignInForUser:(GIDGoogleUser *)user
serverAuthCode:(NSString *_Nullable)serverAuthCode
withCompletion:(nonnull void (^)(FSIUserData *_Nullable,
FlutterError *_Nullable))completion
error:(NSError *)error {
Expand All @@ -281,7 +298,7 @@ - (void)didSignInForUser:(GIDGoogleUser *)user
email:user.profile.email
userId:user.userID
photoUrl:[photoUrl absoluteString]
serverAuthCode:user.serverAuthCode],
serverAuthCode:serverAuthCode],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you audited the new API against the Dart types to see if we want any more changes here? The current situation is that we have optional types on various return objects (GoogleSignInUserData, GoogleSignInTokenData) and we populate what we can, which has varied from platform to platform and version to version. Notably, the release notes mention a new idToken property on GIDGoogleUser, and we have that on the Dart side, so it's very likely we should update the Pigeon FSIUserData definition to include it, and then pass it back here and use it to populate that class on the Dart side here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idToken was the only one from what I could tell.

nil);
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/google_sign_in/google_sign_in_ios/ios/google_sign_in_ios.podspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Enables Google Sign-In in Flutter apps.
s.public_header_files = 'Classes/**/*.h'
s.module_map = 'Classes/FLTGoogleSignInPlugin.modulemap'
s.dependency 'Flutter'
s.dependency 'GoogleSignIn', '~> 6.2'
s.dependency 'GoogleSignIn', '~> 7.0'
s.static_framework = true
s.platform = :ios, '11.0'
s.pod_target_xcconfig = { 'DEFINES_MODULE' => 'YES' }
Expand Down
2 changes: 1 addition & 1 deletion packages/google_sign_in/google_sign_in_ios/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: google_sign_in_ios
description: iOS implementation of the google_sign_in plugin.
repository: https://github.com/flutter/packages/tree/main/packages/google_sign_in/google_sign_in_ios
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+google_sign_in%22
version: 5.6.4
version: 5.6.5

environment:
sdk: ">=2.19.0 <4.0.0"
Expand Down