Skip to content

[google_sign_in] Enable FedCM for web. Use token expiration.#5225

Merged
auto-submit[bot] merged 4 commits intoflutter:mainfrom
ditman:gsi-web-use-fedcm
Nov 2, 2023
Merged

[google_sign_in] Enable FedCM for web. Use token expiration.#5225
auto-submit[bot] merged 4 commits intoflutter:mainfrom
ditman:gsi-web-use-fedcm

Conversation

@ditman
Copy link
Member

@ditman ditman commented Oct 25, 2023

  • Enables FedCM API on compatible browsers.
    • The GIS JS SDK falls-back to the JS implementation on browsers that don't support the new standard. See migration instructions).
  • Uses the supplied token expiration information to more accurately compute isSignedIn() and canAccessScopes(scopes).
    • This does not handle the case where users sign in/out in another tab or from outside the web app, that's still something that needs to be checked server-side.
  • Deprecates the signIn() method on the web.
    • Users should migrate to a combination of renderButton() and silentSignIn(), as described here.

Issues

Testing

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/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package 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.

@ditman ditman requested a review from stuartmorgan-g October 25, 2023 02:10
@ditman ditman force-pushed the gsi-web-use-fedcm branch from 668441f to 28d04f2 Compare October 25, 2023 02:13
@ditman

This comment was marked as resolved.

Sign in to view
@ditman
Copy link
Member Author

ditman commented Oct 26, 2023 •
edited
Loading

I'm awaiting confirmation from the GIS team to see if they want Flutter users to opt-in to this new API (would require a platform interface + core plugin change), or is it OK that we opt them in like this.

Internally confirmed that it's OK that we flip this switch on for our users (b/301259123#comment5). The more, the merrier.

stuartmorgan-g
stuartmorgan-g approved these changes Nov 1, 2023
View reviewed changes
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

Small comments, but otherwise LGTM

packages/google_sign_in/google_sign_in_web/lib/src/gis_client.dart Outdated
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Nov 1, 2023

Choose a reason for hiding this comment

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

Why is a private method deprecated?

Copy link
Member Author

@ditman ditman Nov 1, 2023

Choose a reason for hiding this comment

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

I added these because I don't want to forget about removing these when actually removing the signIn method, but this should probably be an issue + proper TODO.

I'll create the issue right away and link it properly.

Copy link
Member Author

@ditman ditman Nov 1, 2023

Choose a reason for hiding this comment

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

Linked these breadcrumbs to this issue:

packages/google_sign_in/google_sign_in_web/lib/src/gis_client.dart Outdated
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Nov 1, 2023

Choose a reason for hiding this comment

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

So a missing expiration field would indicate that it's already expired? Naively (not knowing the API surface) I would expect that if expiration is optional, then not having it should yield a true result here, so maybe a comment would be helpful?

Copy link
Member Author

@ditman ditman Nov 1, 2023

Choose a reason for hiding this comment

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

I'm going to document this a little bit further, but in this case, if the CredentialResponse payload cannot be decoded, is null, or doesn't contain an "exp" claim, we assume that the token is bad, and thus the user is not authenticated.

In a valid Google ID Token, the "exp" claim should always be provided:

https://developers.google.com/identity/openid-connect/openid-connect#an-id-tokens-payload

packages/google_sign_in/google_sign_in_web/lib/src/utils.dart Outdated
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Nov 1, 2023

Choose a reason for hiding this comment

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

This is conceptually removed enough from the code that ensures that the ! is safe (it's in another method) that I worry about someone changing that code later and this becoming a crash. Couldn't it just be credentialResponse?.credential so it's safe, since it's a nullable field?

Copy link
Member Author

@ditman ditman Nov 1, 2023

Choose a reason for hiding this comment

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

Yes, in this case payload != null implies credentialResponse != null but as you said, the connection is pretty vague.

I think what I really want here is an assertion that (credentialResponse?.credential != null) before returning the GoogleSignInUserData, that way I'm expressing in code that the credential must NOT be null before returning a GoogleSignInUserData.

@ditman
Copy link
Member Author

ditman commented Nov 1, 2023

Thanks for the review! I'll address the comments shortly!

@ditman ditman force-pushed the gsi-web-use-fedcm branch from 28d04f2 to 9560921 Compare November 2, 2023 00:03
@ditman
Copy link
Member Author

ditman commented Nov 2, 2023

I'll land this now, and talk to the Cocoon ppl.

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2023
@auto-submit auto-submit bot merged commit 33c2b4e into flutter:main Nov 2, 2023
@ditman ditman deleted the gsi-web-use-fedcm branch November 2, 2023 01:07
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 2, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 2, 2023
@engine-flutter-autoroll
33c2b4e [google_sign_in] Enable FedCM for web. Use token expiration. (f…
eeb0fcb 
…lutter/packages#5225)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 2, 2023
@engine-flutter-autoroll
Roll Packages from 6ad40b9 to 33c2b4e (3 revisions) (#137763)
8d2fe7d 
flutter/packages@6ad40b9...33c2b4e

2023-11-02 ditman@gmail.com [google_sign_in] Enable FedCM for web. Use token expiration. (flutter/packages#5225)
2023-11-01 ditman@gmail.com [video_player_web] Listen to loadedmetadata event from video element. (flutter/packages#5289)
2023-11-01 stuartmorgan@google.com [local_auth] Update iOS to Pigeon 13 (flutter/packages#5269)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
sybrands-place pushed a commit to sybrands-place/packages that referenced this pull request Nov 8, 2023
Merge branch 'main' into webview_patchedv5
49a3170 
* main: (59 commits)
  [pointer_interceptor] Move pointer_interceptor to sub folder (flutter#5291)
  [webview_flutter] Update iOS to Pigeon 13 (flutter#5271)
  [repo] Adjust error message layout for repo checks (flutter#5241)
  [in_app_pur]: Bump org.json:json from 20230618 to 20231013 in /packages/in_app_purchase/in_app_purchase/example/android/app (flutter#5153)
  Roll Flutter from f5a9835 to 5a6a322 (15 revisions) (flutter#5345)
  [google_sign_in_ios] Upgrade GoogleSignIn iOS SDK to 7.0 (flutter#5240)
  Roll Flutter from 29b2516 to f5a9835 (101 revisions) (flutter#5341)
  [pigeon]: Bump org.jetbrains.kotlin:kotlin-gradle-plugin from 1.9.10 to 1.9.20 in /packages/pigeon/platform_tests/test_plugin/android (flutter#5334)
  [flutter_markdown] Strip leading whitespace from list items (flutter#5294)
  [two_dimensional_scrollables] Add borderRadius support to TableSpanDecoration (flutter#5184)
  [go_router] Adds an ability to add a custom codec for serializing/des… (flutter#5288)
  [go_router] Fixes crashes when dynamically updates routing tables wit… (flutter#5242)
  [camerax] Fix `_getResolutionSelectorFromPreset` NPE (flutter#5287)
  [go_router]Fixes the problem what pathParameters is null in redirect when the Router is recreated. (flutter#5258)
  [google_sign_in] Fix Obj-C formatting bug (flutter#5310)
  Fix mounted checks (flutter#5305)
  [flutter_image] [flutter_markdown] Do not use dynamic in an == operator override (flutter#5298)
  [google_sign_in] Enable FedCM for web. Use token expiration. (flutter#5225)
  [video_player_web] Listen to loadedmetadata event from video element. (flutter#5289)
  [local_auth] Update iOS to Pigeon 13 (flutter#5269)
  ...

# Conflicts:
#	packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FWFWebViewHostApi.m
@reidbaker reidbaker mentioned this pull request Jan 12, 2024
Closed
@reidbaker reidbaker mentioned this pull request Jan 19, 2024
Merged
12 tasks
@reidbaker reidbaker mentioned this pull request Feb 19, 2024
Merged
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: google_sign_in platform-web

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[cocoon] Opt-in to FedCM Beta for GIS in Build Dashboard.

2 participants

@ditman @stuartmorgan-g