plugins icon indicating copy to clipboard operation
plugins copied to clipboard

[google_sign_in] Add doc for iOS auth with SERVER_CLIENT_ID

Open Milvintsiss opened this issue 2 years ago • 10 comments

Add documentation for iOS auth with SERVER_CLIENT_ID.

After a long time struggling with a non-working iOS configuration but Android working well, I figured out that firebase was not adding the SERVER_CLIENT_ID field in the Google-info-service.plist file genereated. This can be really long to troubleshoot for someone like me who don't know really much about GoogleAuth with flutter plugin.

Pre-launch Checklist

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • [x] I signed the CLA.
  • [x] The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • [ ] I listed at least one issue that this PR fixes in the description above.
  • [x] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • [x] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • [ ] I updated/added relevant documentation (doc comments with ///).
  • [x] I added new tests to check the change I am making, or this PR is test-exempt.
  • [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Milvintsiss avatar Feb 07 '22 19:02 Milvintsiss

@Milvintsiss Thanks for sending this PR. ~Although this documentation is good to have, we probably also~ ~want to update the code to make the clientId programmatically configurable from a dart API. I think this~ ~proposal here is valid.~

We actually have already implemented a way to dynamically set up the clientId, which should be petty easily discoverable. :https://github.com/flutter/plugins/blob/main/packages/google_sign_in/google_sign_in/lib/google_sign_in.dart#L186

If we are adding the plist approach for clientId in README, then we should add the programmatic approach too so users can choose the way they want to do it

As per documentation this is only for web. Currently there is no way to programmatically configure this.

EDIT: I looked a bit to the native code using the programmatically configured clientId, in fact iOS and Android use it, so the documentation is outdated, but it has nothing to do with serverClientId. If a clientId is set, it will replace the CLIENT_ID of the google_services file, not the SERVER_CLIENT_ID.

serverClientId has currently no way to be programmatically configured, but I can implement it if needed. Maybe this deserves another PR?

Milvintsiss avatar Feb 24 '22 14:02 Milvintsiss

serverClientId has currently no way to be programmatically configured, but I can implement it if needed. Maybe this deserves another PR?

I see, I got confused with these 2 IDs. Yes I don't see why it should not be programmatically configurable. And we should update the documentation about saying the clientId only supports web as well.

cyanglaz avatar Feb 24 '22 20:02 cyanglaz

I see, I got confused with these 2 IDs. Yes I don't see why it should not be programmatically configurable. And we should update the documentation about saying the clientId only supports web as well.

While trying to implement a programmatically configurable serverClientId for the plugin I found some strange behaviors: It seems that on Android no clientId is needed, but only a serverClientId (so the current clientId is used as serverClientId). This is the prototype of android signIn method:

public GoogleSignInOptions.Builder requestIdToken (String serverClientId)

and how we are using it here. I tried using the clientId need by iOS to logIn and it doesn't work but work great with the serverClientId.

Therefor on iOS both clientId and serverClientId are needed.

This really need clarification, the current clientId should be named serverClientId and be used as it for iOS (currently, you get a crash on iOS if you specify your clientId in the current clientId parameter). This would be a breaking change but the current behavior is wrong and can easily mislead.

Hope this is readable, and please notify me if I'm misunderstanding something.

Milvintsiss avatar Feb 26 '22 09:02 Milvintsiss

@cyanglaz Can you do the primary review here since you've been looking at this the most?

stuartmorgan avatar Apr 12 '22 18:04 stuartmorgan

@Milvintsiss There is a PR trying to add the serverClientId https://github.com/flutter/plugins/pull/5250. It is currently being worked on. Maybe we can cooperate the new changes into this doc change after the above PR lands?

cyanglaz avatar Apr 14 '22 18:04 cyanglaz

@cyanglaz Yes it would be great :+1:

Milvintsiss avatar Apr 15 '22 23:04 Milvintsiss

@cyanglaz @Milvintsiss looks like #5250 has landed; what are the next steps on this PR?

Hixie avatar Jul 12 '22 23:07 Hixie

@Hixie I think we can close this one, the author has already added documentation in the README.

Maybe we can add a note about configuring the SERVER_CLIENT_ID in the GoogleService-Info.plist, but as the recommended way to configure the server client ID is to configure it programmatically we probably don't need to document this?

Milvintsiss avatar Jul 13 '22 04:07 Milvintsiss

Maybe we can add a note about configuring the SERVER_CLIENT_ID in the GoogleService-Info.plist

Yes, we should also add a document for adding SERVER_CLIENT_ID in GoogleService-Info.plist. Would you like to rebase main to your PR and work on it? :)

cyanglaz avatar Jul 21 '22 19:07 cyanglaz

Hey @cyanglaz can I have a review on this? :)

Milvintsiss avatar Sep 15 '22 13:09 Milvintsiss

@cyanglaz Ping on this review from triage.

stuartmorgan avatar Oct 13 '22 20:10 stuartmorgan

Why is this pr still open? Changes regarding the doc configuring SERVER_CLIENT_ID in GoogleService-Info.plist is not yet merged. Stumbled upon this ISSUE today and found that setting serverClientId in dart code alone does not work at all. Finally got it working by adding SERVER_CLIENT_ID in GoogleService-Info.plist.

cliuff avatar Oct 19 '22 09:10 cliuff