-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[google_sign_in] Redesign API for current identity SDKs #9267
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.
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
base: main
Are you sure you want to change the base?
[google_sign_in] Redesign API for current identity SDKs #9267
Conversation
Also reworks app-facing package example app for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Android implementation LGTM!
Future<PlatformGoogleIdTokenCredential?> _authenticate({ | ||
required bool filterToAuthorized, | ||
required bool autoSelectEnabled, | ||
required bool useButtonFlow, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok cool this is much clearer, thank you!
@@ -13,3 +13,28 @@ should add it to your `pubspec.yaml` as usual. | |||
|
|||
[1]: https://pub.dev/packages/google_sign_in | |||
[2]: https://flutter.dev/to/endorsed-federated-plugin | |||
|
|||
## Integration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hahaha totally understandable! LGTM!
@LongCatIsLooong / @cbracken Ping on the iOS portion of this review. |
@bparrishMines Could you review the platform interface and app-facing package changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As someone that's new to this plugin, thanks for the design doc! I still have a few questions after reading the doc.
|
||
### Requesting more scopes when needed | ||
|
||
If an app determines that the user hasn't granted the scopes it requires, it | ||
should initiate an Authorization request. (Remember that in the web platform, | ||
this request **must be initiated from an user interaction**, like a button press). | ||
should initiate an Authorization request. On some platforms, such as web, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this sounds like there are platforms other than web where user interactions are required for Authorization request. Is that the case? How do I know which of the platforms my app targets have such restriction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supportsAuthenticate
? But that's for authentication I assume?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, it's just web (and that may well be true forever). I changed this because we are generally moving toward trying to be as platform agnostic as possible in the app-facing package, because the idea of a federated plugin is that we don't know what other platforms might be supported. You make a great point though; making the README more general without providing a corresponding API just means it's vague and less actionable.
I like the idea of using support queries for this. I plumbed through an authorizationRequiresUserInteraction()
for this purpose, and updated the README and API docs accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good (I don't see a new commit tho).
} | ||
// #enddocregion CanAccessScopes | ||
// #docregion Setup | ||
final GoogleSignIn signIn = GoogleSignIn.instance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this reminds me of flutter/flutter#168100, if the user connects / disconnects a mouse / keyboard on Android the activity will restart and that would result in the element tree being re-inflated anew.
I guess that does not break the new flow (from reading the docs initialize
has to be called exactly once in a program) since the new activity will spin up a new dart vm? Still it would be an annoyance that every time I unplug my keyboard I have to go over the sign-in process again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or when the activity restarts, the lightweight authentication path will be taken and the flow will typically be invisible to user / require no additional user interactions?
EDIT: Just read the design doc and it mentioned "Fully silent sign in is no longer available"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still it would be an annoyance that every time I unplug my keyboard I have to go over the sign-in process again.
That's up to whether the app developer chooses to include auth state in their state restoration.
Or when the activity restarts, the lightweight authentication path will be taken
That's up to the app developer too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include auth state in their state restoration
Oh I thought the developer would have to restore GoogleSignIn, and state restoration only supports value types. If the dart part of GoogleSignIn doesn't keep any secret state itself then that makes sense.
Future<void> _handleAuthenticationEvent( | ||
GoogleSignInAuthenticationEvent event) async { | ||
// #docregion CheckAuthorization | ||
GoogleSignInAccount? user; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uber nit, consider making user
and authorization
final if possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made user
final. Making authorization
final would require adding an else { authorization = null; }
or restructuring to use a ternary in the assignment or using patterns, all of which would make the example more complicated for the README snippet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work?
final authorization = await user?.authorizationClient.authorizationForScopes(scopes);
/// | ||
/// Returned Future resolves to an instance of [GoogleSignInAccount] for a | ||
/// successful sign in or `null` in case sign in process was aborted. | ||
/// If this returns false, [authenticate] will throw an UnsupportedError if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing backticks / square brackets around UnsupportedError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
/// successful sign in or `null` in case sign in process was aborted. | ||
/// If this returns false, [authenticate] will throw an UnsupportedError if | ||
/// called. See the platform-specific documentation for the package to | ||
/// determine how authentication his handled. For instance, the platform may |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
his -> is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
||
/// A base class for authentication event streams. | ||
@immutable | ||
sealed class GoogleSignInAuthenticationEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these classes needed? It seems the sealed type is a glorified GoogleSignInAccount?
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally there were three subclasses, but exceptions moved into onError
; now we could make them GoogleSignInAccount?
.
It would make things less self-documenting though. It's non-obvious that getting null
as an authenticationEvent
means "the user signed out". Is that worth the slightly easier usage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking maybe we can avoid introducing new classes by using typedef or extension types. I played with that idea but found the resulting code harder to read (the user code would still be the same though). So never mind.
|
||
// The plugin registrar, for querying views. | ||
@property(strong, nonnull) id<FlutterPluginRegistrar> registrar; | ||
@property(nonatomic) id<FlutterPluginRegistrar> registrar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still nonnull
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I must have deleted that by mistake when removing strong
. Re-added.
completion:(nullable void (^)(GIDSignInResult *_Nullable signInResult, | ||
NSError *_Nullable error))completion { | ||
GIDGoogleUser *currentUser = self.signIn.currentUser; | ||
forGoogleSignInUser:(GIDGoogleUser *)user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the user
argument is not nullable, otherwise it's failing silently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I've added some more nullability annotations for the private methods; in past projects we didn't annotate implementation files in general, but there's no reason not to here, and since Obj-C nullability is basically just documentation anyway we may as well express it (as we generally do with our newer Java plugin code).
}]; | ||
} | ||
|
||
- (void)addScopes:(nonnull NSArray<NSString *> *)scopes | ||
forUser:(NSString *)userId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can userId be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, added annotation.
- (void)signOutWithError:(FlutterError *_Nullable *_Nonnull)error { | ||
[self.signIn signOut]; | ||
[self.usersByIdentifier removeAllObjects]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signOut
method seems to only sign out the current user: https://developers.google.com/identity/sign-in/ios/reference/Classes/GIDSignIn
Why do we have to remove everything in the dictionary? If refreshedAuthorizationTokensForUser
gets called for a different user (is that possible`?) the app would get an error it seems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
signOut
method seems to only sign out the current user
That's true, but unlike the other platforms, Google Sign In for iOS bakes in the idea of a single current user in various places.
Why do we have to remove everything in the dictionary? If
refreshedAuthorizationTokensForUser
gets called for a different user (is that possible`?) the app would get an error it seems?
I was going to say it would anyway, but it looks like valid tokens would still be returned. Adding scopes, on the other hand will just fail. The underlying SDK API is a little strange in that it will potentially vend multiple user objects, but some operations will fail on all but one (and as a result, our API has the same behavior on iOS).
But it looks like we'll actually get better behavior by never clearing usersByIdentifier
, so calls will continue to use the underlying SDK user objects if they have ever been available, and we can leave it to the SDK to work or not depending on its implementation details. User objects should be pretty lightweight, and in practice I expect it will be rare to make more than one anyway.
I've replaced this with a comment explaining why we're not removing anything from the dictionary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes SGTM but I don't see a new commit?
|
||
### Requesting more scopes when needed | ||
|
||
If an app determines that the user hasn't granted the scopes it requires, it | ||
should initiate an Authorization request. (Remember that in the web platform, | ||
this request **must be initiated from an user interaction**, like a button press). | ||
should initiate an Authorization request. On some platforms, such as web, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good (I don't see a new commit tho).
} | ||
// #enddocregion CanAccessScopes | ||
// #docregion Setup | ||
final GoogleSignIn signIn = GoogleSignIn.instance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include auth state in their state restoration
Oh I thought the developer would have to restore GoogleSignIn, and state restoration only supports value types. If the dart part of GoogleSignIn doesn't keep any secret state itself then that makes sense.
Future<void> _handleAuthenticationEvent( | ||
GoogleSignInAuthenticationEvent event) async { | ||
// #docregion CheckAuthorization | ||
GoogleSignInAccount? user; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work?
final authorization = await user?.authorizationClient.authorizationForScopes(scopes);
|
||
/// A base class for authentication event streams. | ||
@immutable | ||
sealed class GoogleSignInAuthenticationEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking maybe we can avoid introducing new classes by using typedef or extension types. I played with that idea but found the resulting code harder to read (the user code would still be the same though). So never mind.
9fe4f32
to
2823a54
Compare
Oops, I didn't notice that the push failed (I forgot I'd done a merge from main via the GitHub UI). |
🤦🏻 Yes, that's much simpler. (I can't reply inline to that comment, I think because I force-pushed so the commit it was on no longer exists.) |
This is a full overhaul of the
google_sign_in
API, with breaking changes for all component packages—including the platform interface. The usual model of adding the new approach while keeping the old one is not viable here, as the underlying SDKs have changed significantly since the original API was designed. Web already had some only-partially-compatible shims for this reason, and Android would have had to do something similar; see flutter/flutter#119300 and flutter/flutter#154205, and the design doc for more background.google_sign_in
to reflect changes in underlying SDKs flutter#119300play-services-auth
flutter#150365signIn
method. flutter#137727canAccessScopes
on mobile flutter#124206Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3