-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[google_sign_in] Add serverClientId option to return serverAuthCode #879
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 (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
56fbd63
to
46c4bfc
Compare
Also should fix flutter/flutter#14245 |
CLAs look good, thanks! |
@kroike (Not sure if you are the right person to ping) Any chance of getting a review on this? |
@kroikie (mis-spelled the first time) Any chance of getting a review on this? |
@bparrishMines any chance of getting a review on this? |
@bparrishMines When will this pullRequest to be confirmed and merged ? |
This is a necessary feature. Would love to see this merged! @bparrishMines |
Hi @fbcouch, Thank you for the contribution! Unfortunately, we currently don't have a harness for testing the platform side of plugins, current and new features are not properly covered by tests. In order to maintain a quality bar for flutter/plugins we are going to avoid adding new features that aren't critical until they can be properly tested. The test harness work is tracked in flutter/flutter#10824 and flutter/flutter#26080, your feedback on the testing work will be really appreciated (will it properly support the testing needs of this PR?) some design details should be available on flutter/flutter#10824 soon(contributions to that effort are welcomed!). Would you mind re-opening this PR with e2e tests once a test harness is ready? Meanwhile, you can use this branch in your project by including the lines below in your dependencies:
google_sign_in:
git:
url: git://github.com/fbcouch/plugins.git
path: packages/google_sign_in
ref: server-auth-code |
any idea when this will get pulled? |
…. Relates to issues #16613 and #17813
…rverAuthCode on android so that the server always gets the refresh token
67e3992
to
82ffca6
Compare
@bparrishMines Do you know if all of this will be usable with those test harnesses? In the specific case of this PR, we need to get a code from one google client ID for another google client ID and then exchange that code for an access token (as you would on a server) and check if it has a refresh token or not. To be honest, since this meets my needs, it's not likely I'm going to set all of that up for this small change, so someone else may want to take over from here. |
apologies for probably just being a n00b here 🙈 , but can we get an example with using this new option? {"token_type":"Bearer","access_token":"ya...","scope":"email profile https://www.googleapis.com/auth/userinfo.profile openid https://www.googleapis.com/auth/userinfo.email","login_hint":"...","expires_in":3599,"id_token":"ey...","session_state":{"extraQueryParams":{"authuser":"0"}}} from my understanding from peeking at the code, the intended way to login with the flutter library is like: GoogleSignIn _googleSignIn = GoogleSignIn(
serverClientId: <insert server client id here>, // NEW: add cliend id here
scopes: <String>[
<the scopes you want>,
],
);
Future<void> login() async {
try {
await _googleSignIn.signIn();
} catch (error) {
print(error);
}
} am I missing anything? (probably something simple I just overlooked such as additional configuration needed... :/) tl;dr attempting to authenticate without firebase, is this even the right way to attempt this? |
Try calling |
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 PR! I just did the first round of review.
It mostly looks good. I left some comments for certain places.
We also need dart side unit test for this new parameter to be added.
We would also need E2E test to merge this PR as mentioned by @bparrishMines.
@@ -246,6 +255,10 @@ public void init( | |||
if (!Strings.isNullOrEmpty(hostedDomain)) { | |||
optionsBuilder.setHostedDomain(hostedDomain); | |||
} | |||
if (!Strings.isNullOrEmpty(serverClientId)) { | |||
optionsBuilder.requestServerAuthCode(serverClientId, true); |
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.
If we need to expose this API, let's also expose the boolean parameter forceCodeForRefreshToken
to the public API and make it false
by default.
@@ -75,6 +75,9 @@ - (void)handleMethodCall:(FlutterMethodCall *)call result:(FlutterResult)result | |||
[GIDSignIn sharedInstance].clientID = plist[kClientIdKey]; | |||
[GIDSignIn sharedInstance].scopes = call.arguments[@"scopes"]; | |||
[GIDSignIn sharedInstance].hostedDomain = call.arguments[@"hostedDomain"]; | |||
if (![call.arguments[@"serverClientId"] isEqual:[NSNull 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.
nit:
if (![call.arguments[@"serverClientId"] isEqual:[NSNull null]]) { | |
if ([call.arguments[@"serverClientId"] isKindOfClass:[NSString class]]) { |
@@ -246,6 +255,10 @@ public void init( | |||
if (!Strings.isNullOrEmpty(hostedDomain)) { | |||
optionsBuilder.setHostedDomain(hostedDomain); | |||
} | |||
if (!Strings.isNullOrEmpty(serverClientId)) { | |||
optionsBuilder.requestServerAuthCode(serverClientId, true); | |||
optionsBuilder.requestIdToken(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.
Is this required? Why do we not have the same on iOS? What happens if this block also calls requestIdToken
?
if (clientIdIdentifier != 0) {
optionsBuilder.requestIdToken(registrar.context().getString(clientIdIdentifier));
}
@@ -163,14 +165,17 @@ class GoogleSignIn { | |||
/// The [hostedDomain] argument specifies a hosted domain restriction. By | |||
/// setting this, sign in will be restricted to accounts of the user in the | |||
/// specified domain. By default, the list of accounts will not be restricted. | |||
GoogleSignIn({this.signInOption, this.scopes, this.hostedDomain}); | |||
GoogleSignIn( |
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.
Needs dart doc for serverClientId
as well.
@@ -207,6 +212,8 @@ class GoogleSignIn { | |||
/// Domain to restrict sign-in to. | |||
final String hostedDomain; | |||
|
|||
final String 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.
Needs dart doc
I just tried this code in my project, but im getting the below error
But im pretty sure that calling the below code only once
Infact i have only one google clinet id, i dont know how other one came |
It is not the package problem, i used firebase and own client id, i just copied the clinet id from googleservies.json to this package. and it is working fine. |
I have checked the code working fine, but |
any updates? |
@fbcouch Seems like you have not been actively working on this for a while. Are you still able to work on this? |
@cyanglaz I might circle back eventually, but it will probably be awhile. If someone else wants to take it over, that's fine with me. |
I started modifying the google_sign_in plugin in the same manner as this PR, but I got the same error as @pavankumarkatakam.
I'm using Firebase and I want to get the auth code so I can pass it to my server. I'm wondering if Firebase is setting the |
Were you able to solve it? |
No. I haven't tried to move forward with it. I'm using Firebase with Google Sign In, and I assume that there's something in the Firebase Android/iOS library that is setting (or adding) a server client id, because it's required to use Firebase, but that's not clearly documented. So, if you want to use Firebase you can't set your own server client id to enable authorization code flow for your server. |
Does anyone know how to implement the serverClientId option to return serverAuthCode? |
This should fix issues flutter/flutter#16613 and flutter/flutter#17813
There are a number of formatting changes introduced by the flutter_plugin_tool...I'm happy to back those out if desired.