Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[google_sign_in] Add doc for iOS auth with SERVER_CLIENT_ID #4747

Merged
merged 8 commits into from
Jan 28, 2023

Conversation

Milvintsiss
Copy link
Contributor

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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • 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.)
  • I signed the CLA.
  • 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.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

packages/google_sign_in/google_sign_in/README.md Outdated Show resolved Hide resolved
packages/google_sign_in/google_sign_in/README.md Outdated Show resolved Hide resolved
@Milvintsiss
Copy link
Contributor Author

Milvintsiss commented Feb 24, 2022

@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?

@cyanglaz
Copy link
Contributor

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.

@Milvintsiss
Copy link
Contributor Author

Milvintsiss commented Feb 26, 2022

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.

@stuartmorgan
Copy link
Contributor

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

@cyanglaz
Copy link
Contributor

@Milvintsiss There is a PR trying to add the serverClientId #5250. It is currently being worked on. Maybe we can cooperate the new changes into this doc change after the above PR lands?

@Milvintsiss
Copy link
Contributor Author

@cyanglaz Yes it would be great 👍

@Hixie
Copy link
Contributor

Hixie commented Jul 12, 2022

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

@Milvintsiss
Copy link
Contributor Author

@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?

@cyanglaz
Copy link
Contributor

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? :)

@Milvintsiss Milvintsiss requested review from stuartmorgan and cyanglaz and removed request for cyanglaz and stuartmorgan August 21, 2022 23:08
@Milvintsiss
Copy link
Contributor Author

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

@Milvintsiss Milvintsiss requested review from stuartmorgan and removed request for cyanglaz September 23, 2022 17:03
@stuartmorgan
Copy link
Contributor

@cyanglaz Ping on this review from triage.

@cliuff
Copy link

cliuff commented Oct 19, 2022

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.

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 27, 2023
@auto-submit auto-submit bot merged commit f5568e4 into flutter:main Jan 28, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 31, 2023
* 90f447313 [ci] Increase timeouts for platform_tests (flutter/plugins#7036)

* f5568e4b1 [google_sign_in] Add doc for iOS auth with SERVER_CLIENT_ID (flutter/plugins#4747)

* 0c05e8d91 Roll Flutter from a815ee6 to 75680ae (58 revisions) (flutter/plugins#7048)

* a4c320902 [camera]: Bump camerax_version from 1.3.0-alpha02 to 1.3.0-alpha03 in /packages/camera/camera_android_camerax/android (flutter/plugins#7061)

* 8f12b27b6 [ci] Add LUCI versions of macOS ARM tests (flutter/plugins#6984)

* 3843b38e2 [tool] Improve main-branch detection (flutter/plugins#7038)

* d39e7569c [in_app_purchase] Prep for more const widgets (flutter/plugins#7030)

* ddb9777ee [ci] Switch remaining macOS host tests to LUCI (flutter/plugins#7063)

* 2edf56324 [ci] Part 1 of swapping iOS platform test arch (flutter/plugins#7064)

* 35f0b1a49 [camerax] Add system services to plugin (flutter/plugins#6986)

* 5dd0f41a2 [webview]: Bump mockito-inline (flutter/plugins#7056)

* 1896f10ca [webview_flutter_wkwebview][webview_flutter_android] Fixes bug where the `WebView`s could not be released (flutter/plugins#6996)

* a494825fa [camerax] Allow instance manager to create identical objects (flutter/plugins#7034)

* 6ef73da26 [ci] Increase heavy workload memory (flutter/plugins#7065)

* 9da327ca3 [various] Update to use sharedDarwinSource (flutter/plugins#7027)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: google_sign_in
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants