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

[google_sign_in] add serverAuthCode to GoogleSignInAccount #4180

Merged
merged 14 commits into from Oct 21, 2021
Merged

[google_sign_in] add serverAuthCode to GoogleSignInAccount #4180

merged 14 commits into from Oct 21, 2021

Conversation

p-shapovalov
Copy link
Contributor

@p-shapovalov p-shapovalov commented Jul 22, 2021 •
edited

Description

Reuse serverAuthCode obtained on login in /pull/4179

Related issues

flutter/flutter#57712
flutter/flutter#57741

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]. (Note that 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].
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@google-cla google-cla bot added the cla: yes label Jul 22, 2021
@p-shapovalov p-shapovalov mentioned this pull request Jul 22, 2021
Merged
13 tasks
@stuartmorgan
Copy link
Contributor

Thanks for the contribution!

  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.

Same question here as in the other PR; I'm not seeing these tests in the PR.

@stuartmorgan stuartmorgan changed the title reuse serverAuthCode obtained on login [google_sign_in] reuse serverAuthCode obtained on login Jul 22, 2021
@stuartmorgan
Copy link
Contributor

I've fixed the title, but this was not the case:

  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]

Please keep in mind for future submissions that the checklist is to help make sure all the important steps are followed; just checking all the boxes without doing the steps causes PRs to take longer to triage and review.

@p-shapovalov
Copy link
Contributor Author

fixed

@p-shapovalov
Copy link
Contributor Author

Just in case clarification here too. I've removed serverAuthCode from getToken response because that was wrong - android/ios native response doesn't contain serverAuthCode field

And I've added serverAuthCode to user data response according to my changes because serverAuthCode obtained on that step

@p-shapovalov p-shapovalov changed the title [google_sign_in] reuse serverAuthCode obtained on login [google_sign_in] move serverAuthCode to GoogleSignInAccount Oct 8, 2021
@p-shapovalov
Copy link
Contributor Author

Ok, serverAuthCode property now moved from tokens response to signIn response

@p-shapovalov p-shapovalov changed the title [google_sign_in] move serverAuthCode to GoogleSignInAccount [google_sign_in] add serverAuthCode to GoogleSignInAccount Oct 8, 2021
@p-shapovalov
Copy link
Contributor Author

Hi @cyanglaz
Interface package on pub.dev already https://pub.dev/packages/google_sign_in_platform_interface/changelog
Anything else I can add here?

@p-shapovalov
Copy link
Contributor Author

@cyanglaz please let me know if you need anything else.

cyanglaz
cyanglaz approved these changes Oct 21, 2021
View reviewed changes
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

@cyanglaz cyanglaz added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Oct 21, 2021
@fluttergithubbot fluttergithubbot merged commit 7df7a8f into flutter:master Oct 21, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 21, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 21, 2021
@engine-flutter-autoroll
7df7a8f [google_sign_in] add serverAuthCode to GoogleSignInAccount (f…
77dfc59 
…lutter/plugins#4180)
@p-shapovalov
Copy link
Contributor Author

Thank you!

stuartmorgan
stuartmorgan reviewed Oct 21, 2021
View reviewed changes
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.

Post-submit LGTM (with one question), since this should have had two approvals before landing.

packages/google_sign_in/google_sign_in/test/google_sign_in_test.dart
@@ -350,7 +351,8 @@ void main() {

expect(auth.accessToken, '456');
expect(auth.idToken, '123');
expect(auth.serverAuthCode, '789');
// fix deprecated_member_use_from_same_package
// expect(auth.serverAuthCode, '789');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was commented-out code checked in?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, sorry I should have caught it. I'll create a PR to remove it

Copy link
Contributor

Choose a reason for hiding this comment

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

#4442

@cyanglaz
Copy link
Contributor

cyanglaz commented Oct 21, 2021 •
edited

Post-submit LGTM (with one question), since this should have had two approvals before landing.

Is it a new policy that we are enforcing for the whole repo? (All PRs need at least 2 approvals before landing)

@cyanglaz cyanglaz mentioned this pull request Oct 21, 2021
Merged
11 tasks
NickalasB added a commit to NickalasB/plugins that referenced this pull request Oct 21, 2021
@NickalasB
Merge branch 'master' into implement_zoomEnabled_for_ios_and_android
6672b4f 
* master:
  [webview] Fix typos in the README (flutter#4249)
  [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180)
  [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429)
  [shared_preferences] Switch to new analysis options (flutter#4384)
  [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413)
  [camera] Run iOS methods on UI thread by default (flutter#4140)
  [ci] Always run all `format` steps (flutter#4427)
  [flutter_plugin_tools] Fix license-check on Windows (flutter#4425)
  [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400)
  [image_picker][android] suppress unchecked warning (flutter#4408)
  [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436)
  [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality  (flutter#4389)
  Bump compileSdkVersion to 31 (flutter#4432)
  [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426)
  Update integration_test README  (flutter#3824)
  [webview_flutter] Adjust test URLs again (flutter#4407)
  [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179)
  [camera] Add filter for unsupported cameras on Android (flutter#4418)
@stuartmorgan
Copy link
Contributor

Post-submit LGTM (with one question), since this should have had two approvals before landing.

Is it a new policy that we are enforcing for the whole repo? (All PRs need at least 2 approvals before landing)

There's a new-ish policy for all Flutter repos that all PRs need the involvement of two commiters: https://github.com/flutter/flutter/wiki/Tree-hygiene#getting-a-code-review second paragraph.

NickalasB added a commit to NickalasB/plugins that referenced this pull request Oct 21, 2021
@NickalasB
Merge branch 'master' into add_zoom_to_android_webview
eab9765 
* master:
  [webview] Fix typos in the README (flutter#4249)
  [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180)
  [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429)
  [shared_preferences] Switch to new analysis options (flutter#4384)
  [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413)
  [camera] Run iOS methods on UI thread by default (flutter#4140)
  [ci] Always run all `format` steps (flutter#4427)
  [flutter_plugin_tools] Fix license-check on Windows (flutter#4425)
  [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400)
  [image_picker][android] suppress unchecked warning (flutter#4408)
  [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436)
  [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality  (flutter#4389)
  Bump compileSdkVersion to 31 (flutter#4432)
  [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426)
  Update integration_test README  (flutter#3824)
  [webview_flutter] Adjust test URLs again (flutter#4407)
  [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179)
  [camera] Add filter for unsupported cameras on Android (flutter#4418)

# Conflicts:
#	packages/webview_flutter/webview_flutter/CHANGELOG.md
#	packages/webview_flutter/webview_flutter/pubspec.yaml
@p-shapovalov p-shapovalov deleted the fix/reuse-server-auth-code branch October 23, 2021 06:28
yasargil added a commit to yasargil/plugins that referenced this pull request Oct 25, 2021
@yasargil
Merge branch 'master' into gmain
d37dd69 
* master: (364 commits)
  Use OpenJDK 11 in CI jobs  (flutter#4419)
  [google_sign_in] remove the commented out code in tests (flutter#4442)
  [webview] Fix typos in the README (flutter#4249)
  [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180)
  [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429)
  [shared_preferences] Switch to new analysis options (flutter#4384)
  [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413)
  [camera] Run iOS methods on UI thread by default (flutter#4140)
  [ci] Always run all `format` steps (flutter#4427)
  [flutter_plugin_tools] Fix license-check on Windows (flutter#4425)
  [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400)
  [image_picker][android] suppress unchecked warning (flutter#4408)
  [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436)
  [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality  (flutter#4389)
  Bump compileSdkVersion to 31 (flutter#4432)
  [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426)
  Update integration_test README  (flutter#3824)
  [webview_flutter] Adjust test URLs again (flutter#4407)
  [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179)
  [camera] Add filter for unsupported cameras on Android (flutter#4418)
  ...

# Conflicts:
#	packages/google_maps_flutter/google_maps_flutter/android/src/main/java/io/flutter/plugins/googlemaps/GoogleMapController.java
This was referenced Oct 29, 2021
Closed
Closed
hofmannfelix pushed a commit to hofmannfelix/plugins that referenced this pull request Oct 30, 2021
Merge commit '76e84c0679dbab7bfaaaa553b17bb0dbdb9a3c33' into update_v…
1c83c0a 
…ideo_src_on_same_controller

* commit '76e84c0679dbab7bfaaaa553b17bb0dbdb9a3c33': (537 commits)
  [video_player] Initialize player when size and duration become available (flutter#4438)
  [webview_flutter] Implement zoom enabled for ios and android (flutter#4417)
  Partial revert of "upgraded usage of BinaryMessenger (flutter#4451)" (flutter#4453)
  Implement Android WebView api with pigeon (Java portion) (flutter#4441)
  [in_app_purchase] Update to the latest pkg:json_serializable (flutter#4434)
  Implement Android WebView api with pigeon (Dart portion)  (flutter#4435)
  upgraded usage of BinaryMessenger (flutter#4451)
  [flutter_plugin_tools] Fix pubspec-check on Windows (flutter#4428)
  Use OpenJDK 11 in CI jobs  (flutter#4419)
  [google_sign_in] remove the commented out code in tests (flutter#4442)
  [webview] Fix typos in the README (flutter#4249)
  [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180)
  [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429)
  [shared_preferences] Switch to new analysis options (flutter#4384)
  [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413)
  [camera] Run iOS methods on UI thread by default (flutter#4140)
  [ci] Always run all `format` steps (flutter#4427)
  [flutter_plugin_tools] Fix license-check on Windows (flutter#4425)
  [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400)
  [image_picker][android] suppress unchecked warning (flutter#4408)
  ...

# Conflicts:
#	packages/video_player/video_player/pubspec.yaml
#	packages/video_player/video_player_web/lib/video_player_web.dart
#	packages/video_player/video_player_web/pubspec.yaml
@jainkrati
Copy link

Thanks for the fix. Somehow I am still getting null serverAuthCode. Is this change already pushed in latest published package for google_sign_in?

Also, are you aware of any ongoing or past fix for ios for this scenario? I am integrating google sign in for my app and need both these fixes. Will appreciate any help :)

cc @p-shapovalov @stuartmorgan

@p-shapovalov
Copy link
Contributor Author

You need to specify correct server client id to get serverAuthCode. Please ensure you have it in SERVER_CLIENT_ID(iOS) and default_web_client_id(Android). For android you also can pass value directly into plugin constructor

@jainkrati
Copy link

Thanks for the quick tip @p-shapovalov . I made the changes but it keeps crashing everytime google sign in opens in ios after account selection. Strangely, the exception is not throw in the console but the device keeps disconnecting.

I believe its a config issue but I am not able to put a finger on it. So sharing the details here, please let me know if you notice anything that needs to change in the way I am using/configuring the plugin:

  1. In GoogleService-Info.plist, following values are present:
    CLIENT_ID
    IOSClientID.apps.googleusercontent.com
    REVERSED_CLIENT_ID
    com.googleusercontent.apps.IOSClientID
    SERVER_CLIENT_ID
    webClientID.apps.googleusercontent.com
    PLIST_VERSION
    1
    BUNDLE_ID
    myAppsBundleID
  2. I am instantiating googleSignIn this way:
    account = await GoogleSignIn(clientId: IOSClientID, hostedDomain: "", scopes: [
    'email',
    'https://www.googleapis.com/auth/contacts.readonly'
    ]).signIn();
  3. If account is not null then accessing serverAuthCode:
    if (account != null) {
    final GoogleSignInAuthentication googleSignInAuthentication =
    await account.authentication;
    print("serverAuthCode is ${googleSignInAuthentication.serverAuthCode}");
    }

@p-shapovalov
Copy link
Contributor Author

I'm not sure what exactly wrong with your configuration, because I didn't use google sign in for ios, I guess you can try to catch exception somewhere in https://github.com/flutter/plugins/blob/master/packages/google_sign_in/google_sign_in/lib/google_sign_in.dart#L376

However according to your sample

If account is not null then accessing serverAuthCode:

Please check deprecation warning and use account.serverAuthCode instead

@stuartmorgan
Copy link
Contributor

Please do not use PRs for support requests.

@devbytyler
Copy link

devbytyler commented Nov 30, 2021 •
edited

@jainkrati I initially had the same problem as you. (My implementation is very similar to yours) Then I realized that my GoogleService-Info.plist in VSCode did not match in Xcode. Fixed that and everything started working.

@jainkrati
Copy link

Thanks @tylerstephens814 :)

Yesterday even I got google sign in working on ios by fixing two issues:

  1. Adding SERVER_CLIENT_ID
  2. Removing and re-adding all entries of plist file from XCode instead of VS Code. Apparently there were some extra characters when we added from VS Code last time.

amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021
@p-shapovalov @amantoux
[google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180
4a0fc21 
)
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
@p-shapovalov @KyleFin
[google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180
3d72641 
)
@stuartmorgan stuartmorgan mentioned this pull request Jan 6, 2022
Merged
13 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stuartmorgan stuartmorgan stuartmorgan left review comments

@cyanglaz cyanglaz cyanglaz approved these changes

Assignees
No one assigned
Labels
cla: yes p: google_sign_in waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Milestone
No milestone
6 participants
@p-shapovalov @stuartmorgan @cyanglaz @jainkrati @devbytyler @fluttergithubbot