-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[google_sign_in] Add ability to return serverAuthCode #2116
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
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.
Thank you for providing this. I left some comments. Could you also update the pubspec?
packages/google_sign_in/CHANGELOG.md
Outdated
@@ -1,3 +1,7 @@ | |||
## 4.0.10 |
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.
This patch actually updated the response of the public API so it should be 4.1.0
@@ -239,6 +239,7 @@ public void init( | |||
"default_web_client_id", "string", registrar.context().getPackageName()); | |||
if (clientIdIdentifier != 0) { | |||
optionsBuilder.requestIdToken(registrar.context().getString(clientIdIdentifier)); | |||
optionsBuilder.requestServerAuthCode(registrar.context().getString(clientIdIdentifier)); |
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 the serverClientId always static? We might want to have it be able to set through code as well. Same in iOS
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.
@cyanglaz
Thank you for review and comment. I'm going to modify them.
About here, In my opinion I think serverClientId is static define, because it is not dynamic value.
but if you have idea or opinion, i give priority to them. please tell me :)
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.
returning serverAuthCode is using my flutter app. so I'm positive about support this modify.
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.
and return null if SERVER_CLIENT_ID value is null.
@@ -91,6 +97,10 @@ class GoogleSignInAccount implements GoogleIdentity { | |||
if (response['idToken'] == null) { | |||
response['idToken'] = _idToken; | |||
} | |||
|
|||
if (response['serverAuthCode'] == 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.
nits: response['serverAuthCode'] ??=_serverAuthCode;
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.
It's cool
once this is available, we can pass in custom serverClientId ? |
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.
LGTM to the approach. Could you add tests for this change? Also, please update the pubspec with a new version.
@@ -1,3 +1,7 @@ | |||
## 4.5.0 | |||
|
|||
* Add support for getting `serverAuthCode` |
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.
nits:
* Add support for getting `serverAuthCode` | |
* Add support for getting `serverAuthCode`. |
<?xml version="1.0" encoding="utf-8"?> | ||
<resources> | ||
<string name="default_web_client_id">YOUR_WEB_CLIENT_ID</string> | ||
</resources> |
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.
new line EOF
@@ -240,6 +240,7 @@ public void init( | |||
"default_web_client_id", "string", registrar.context().getPackageName()); | |||
if (clientIdIdentifier != 0) { | |||
optionsBuilder.requestIdToken(registrar.context().getString(clientIdIdentifier)); | |||
optionsBuilder.requestServerAuthCode(registrar.context().getString(clientIdIdentifier)); |
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.
It seems only gets context from registrar. With v2 embedding, we would need to get the context from the plugin binding.
I've modified review point. @cyanglaz |
hello, any update when this will get merged? |
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.
LGTM
Description
Supports SERVER_CLIENT_ID(iOS) and default_web_client_id(Android) parameter so that serverAuthCode can be returned.
Related Issues
flutter/flutter#16613
#879
#1704
#1475
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?