-
Notifications
You must be signed in to change notification settings - Fork 205
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
Add extra_params argument to signin call #369
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
completion:(nullable GIDSignInCompletion)completion { | ||
GIDSignInInternalOptions *options = | ||
GIDSignInInternalOptions *options = |
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.
Could you please remove the extra space in the beginning of this line?
@@ -160,6 +160,7 @@ typedef NS_ERROR_ENUM(kGIDSignInErrorDomain, GIDSignInErrorCode) { | |||
- (void)signInWithPresentingViewController:(UIViewController *)presentingViewController | |||
hint:(nullable NSString *)hint | |||
additionalScopes:(nullable NSArray<NSString *> *)additionalScopes | |||
additionalParams:(nullable NSDictionary *)extraParams |
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.
Could we keep the original method and create another method with the additionalParams parameter, to prevent breaking existing client integrations with this API? We can have the original method call this method with nil as additionalParams.
@@ -160,6 +160,7 @@ typedef NS_ERROR_ENUM(kGIDSignInErrorDomain, GIDSignInErrorCode) { | |||
- (void)signInWithPresentingViewController:(UIViewController *)presentingViewController |
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.
Please add documentation comment on the new parameter.
[GIDSignInInternalOptions defaultOptionsWithConfiguration:_configuration | ||
presentingViewController:presentingViewController | ||
loginHint:hint | ||
addScopesFlow:NO | ||
scopes:additionalScopes | ||
completion:completion]; | ||
[self signInWithOptions:options]; | ||
|
||
if (extraParams != nil) { |
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 we add tests for this method?
28a2db8
to
a4c63f8
Compare
I wanted to set a custom prompt https://developers.google.com/identity/openid-connect/openid-connect#prompt in the request so that I can always show the Google account list instead of automatically use the logged-in user.
But I couldn't find a way to do it inside this library.
What I saw is that AppAuth supports it by setting it in extraParams: openid/AppAuth-iOS#6 (comment)
The only problem is that I couldn't find a way to set these extraParams from outside of Google Sign in library.
So I added an extra params argument to the signin function and internally set them inside AppAuth call.