-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[google_sign_in] Add implementations of requestScopes. #2599
Conversation
The web part of this, looks good to me! What's the use case here? Requesting a minimal set of scopes at first, and then if the user interacts further in the app request some more? |
That's the primary one (basically approach scopes the same as other permissions). This way, we ask for permission to sensitive data in the context where it makes sense. The other is to handle adding scopes as part of an app update. We see issues adding new scopes to the constructor when a user has signed in before an update that adds a scope (issues vary by platform). This way, we won't have to sign users out when we add scopes. |
This sounds very interesting. I'm not sure if this is documented anywhere in the Google Sign In API or similar, but it'd probably be worth a medium blog post or similar! We can help you publish an article about this progressive approach to scopes, if you're interested in writing it! |
It does sound like something worth writing, I'll ping you about it offline. |
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.
iOS and Android part looks good. Could we possibly add JUnit tests and XCTests?
@cyanglaz, Happy to, I just couldn't find any existing ones for this plugin (and couldn't figure out how to add some easily). Do they live somewhere else and I just missed them? |
@ditman, No problem, thanks for the heads up! |
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.
GLTM on mobile
Description
Adds implementation for
requestScopes
method introduced to the plugin interface in #2547.Recreated since I messed up rebasing after first PR.
Related Issues
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?