[google_sign_in] Fix issue obtaining serverAuthCode on Android and add forceCodeForRefreshToken parameter

[fbcouch]

trafficstars

Description

This is a follow up to https://github.com/flutter/plugins/pull/2116 to fix the following issues:

• ~~serverAuthCode is always null on Android because it is not passed to the GoogleSignInAuthentication object as that is created by the getTokens call rather than from the data obtained during the initial signIn call (which does contain a serverAuthCode, it's just not accessible anywhere that I can tell)~~
• A refresh token is only given to the server on the first sign in without the ability to set the forceCodeForRefreshToken parameter

Related Issues

• flutter/flutter#17813
• flutter/flutter#14245
• flutter/flutter#15796
• flutter/flutter#45847

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
• [x] All existing and new tests are passing.
• [x] I updated/added relevant documentation (doc comments with ///).
• [x] The analyzer (flutter analyze) does not report any problems on my PR.
• [x] I read and followed the Flutter Style Guide.
• [x] The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• [x] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
• [x] I updated CHANGELOG.md to add a description of the change.
• [x] I signed the CLA.
• [x] I am willing to follow-up on review comments in a timely manner.

A note on flutter analyze: this plugin updates google_sign_in and google_sign_in_platform_interface, so flutter analyze won't pass until the google_sign_in_platform_interface update is published or an additional commit is applied that sets up local path dependencies (see https://github.com/fbcouch/plugins/commit/3bbdfec45244a4ef3e47977a4d0bfd84cda9f5aa ). I can split that into two pull requests if needed.

Breaking Change

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

• [x] 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.

~~Festivus Note~~

~~As the season of Festivus is upon us, I do have a grievance to air from the last time I submitted a PR related to this plugin. I was told on https://github.com/flutter/plugins/pull/879 that my pull request to add the serverAuthCode (which worked on android, as a side note) would not be accepted without E2E tests using a test harness that wasn't even ready yet. You can imagine my surprise when https://github.com/flutter/plugins/pull/2116 was merged a few months later without them (no disrespect to the author of that PR, they did what was asked of them).~~

[stuartmorgan-g]

Thanks for the submission! We’re currently working through a large backlog of PRs, and this will require non-trivial review, so it will take some time before we’re able to review it. As explained in CONTRIBUTING.md, votes for the corresponding issue are the primary way we’re prioritizing non-trivial reviews, so we encourage anyone interested in this PR to vote for the corresponding issue.

We apologize for the long delay in triaging this PR. We’re in the process of overhauling our PR triage system to respond much more quickly, as well as working through the backlog.

(Note that https://github.com/flutter/plugins/pull/2792 is making a similar change, so when they are reviewed we should ensure we converge on a single PR that addresses everything.)

[stuartmorgan-g]

As the season of Festivus is upon us, I do have a grievance to air from the last time I submitted a PR related to this plugin. I was told on #879 that my pull request to add the serverAuthCode (which worked on android, as a side note) would not be accepted without E2E tests using a test harness that wasn't even ready yet. You can imagine my surprise when #2116 was merged a few months later without them (no disrespect to the author of that PR, they did what was asked of them).

Unfortunately our push toward better testing coverage occasionally misses things, leading to inconsistencies like that. As evidenced by the need for this PR, that one really shouldn't have landed either without e2e testing!

[dreenot]

This problem is very relevant, and it is around for a long time. A huge part of Google's Api depends on server side authentication.

I am currently trying some workaround to implement his changes on my installation, but I'm getting an error "google_sign_in_platform_interface ^1.2.0 which doesn't match any versions, google_sign_in from git is forbidden." If I can, I'll use it before it's merged into master

[dreenot]

I updated the code to work with the current master. It's working fine for me. But it would be better if this PR was updated instead. I'm not very experienced making PR to large projects. But I made it available since it might be useful to someone who needs this specific task or in case this is not being updated. If necessary, I'll fix the issues.

[fbcouch]

@dreenot It looks like basically what was needed was updating the version numbers and adding some nullable types, right? I updated this PR accordingly...I think your PR was missing the updated implementations of hashCode and == on the GoogleSignInUserData interface. And I think the CHANGELOG entry for the platform interface didn't make it over. Let me know if the updated PR works for you, though!

@stuartmorgan No problem! I was just a bit frustrated at the time and about to embark on a saga of a PR to add scribble support (which is still in progress, actually)

[fbcouch]

BTW here is a patch that makes the analyze work properly:

diff --git a/packages/google_sign_in/google_sign_in/pubspec.yaml b/packages/google_sign_in/google_sign_in/pubspec.yaml
index d1443664..69daa456 100644
--- a/packages/google_sign_in/google_sign_in/pubspec.yaml
+++ b/packages/google_sign_in/google_sign_in/pubspec.yaml
@@ -16,8 +16,10 @@ flutter:
         default_package: google_sign_in_web
 
 dependencies:
-  google_sign_in_platform_interface: ^2.1.0
-  google_sign_in_web: ^0.10.0
+  google_sign_in_platform_interface:
+    path: ../google_sign_in_platform_interface
+  google_sign_in_web:
+    path: ../google_sign_in_web
   flutter:
     sdk: flutter
   meta: ^1.3.0
diff --git a/packages/google_sign_in/google_sign_in_web/pubspec.yaml b/packages/google_sign_in/google_sign_in_web/pubspec.yaml
index 52eedf8f..74cac167 100644
--- a/packages/google_sign_in/google_sign_in_web/pubspec.yaml
+++ b/packages/google_sign_in/google_sign_in_web/pubspec.yaml
@@ -12,7 +12,8 @@ flutter:
         fileName: google_sign_in_web.dart
 
 dependencies:
-  google_sign_in_platform_interface: ^2.0.0
+  google_sign_in_platform_interface:
+    path: ../google_sign_in_platform_interface
   flutter:
     sdk: flutter
   flutter_web_plugins:

[dreenot]

@fbcouch I noticed another problem that I'm afraid we can't fix here. Yes, we are able to get the serverAuthCode using your code. But the code is invalid. You cannot use the code because your Access Type is not Offline. The problem is not in your code.

I consulted the documentation all the way down to Google Play Services, it SHOULD be setting it to offline when you use requestServerAuthCode either you setting forceCodeForRefreshToken or not. It is well stated in the documentation. Therefore, it does not happen. The code is sent, but your authorization remains Online. And the attempt to use the code returns "error": "invalid_grant"

requestServerAuthCode is being successfully sent, otherwise I'd not be able to get the serverAuthCode . Since the documentation says it supposedly set it to offline, it's a bug. I didn't find another way to set it. I reported it to Android Issue Tracker https://issuetracker.google.com/issues/186806274

https://developers.google.com/android/reference/com/google/android/gms/auth/api/signin/GoogleSignInOptions.Builder

[fbcouch]

@dreenot Oh that is very interesting...what version of google play services are you using?

I am able to get valid serverAuthCodes on com.google.gms:google-services:4.3.0 (I think that would be the relevant library, not 100% sure of that though)

[dreenot]

@fbcouch It happens I had an invalid_redirect_uri before, and I didn't notice it, what "used" the code even though it was not successful. The token is still access_type Online for some reason, but it is working for Offline access. So, I guess you can disregard my last comment since it's going to work. But part of the info in https://www.googleapis.com/oauth2/v1/tokeninfo is not correct.

[stuartmorgan-g]

I believe the first of the two bullet points in the PR description was obsoleted by https://github.com/flutter/plugins/pull/4180

For the second part, can you explain why this is something configured for the entire app via an XML property, rather than being part of the API surface?

[flutter-dashboard[bot]]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[fbcouch]

Yes, it looks like at least the serverAuthCode part is handled already now, so we just need the forceCodeForRefreshToken stuff. I think I had done it through the XML because that's the way most of the android options are defined. Adding it to the API surface sounds better to me. I'll see if I can make some time in the next couple of weeks to update this to target the latest main branch and reduce it down to just the forceCodeForRefreshToken stuff.

[fbcouch]

@stuartmorgan I updated the PR to add the forceCodeForRefreshTokean to the GoogleSignIn() api – let me know how that looks. Since both the platform interface and plugin are changing, this patch is needed to make the tests pass:

diff --git a/packages/google_sign_in/google_sign_in/pubspec.yaml b/packages/google_sign_in/google_sign_in/pubspec.yaml
index 4ed33ce1c..c25a40d06 100644
--- a/packages/google_sign_in/google_sign_in/pubspec.yaml
+++ b/packages/google_sign_in/google_sign_in/pubspec.yaml
@@ -36,6 +36,10 @@ dev_dependencies:
     sdk: flutter
   pedantic: ^1.10.0
 
+dependency_overrides:
+  google_sign_in_platform_interface:
+    path: ../google_sign_in_platform_interface
+
 # The example deliberately includes limited-use secrets.
 false_secrets:
   - /example/android/app/google-services.json
diff --git a/packages/google_sign_in/google_sign_in_web/pubspec.yaml b/packages/google_sign_in/google_sign_in_web/pubspec.yaml
index 7e6d20b8a..38d89c0e0 100644
--- a/packages/google_sign_in/google_sign_in_web/pubspec.yaml
+++ b/packages/google_sign_in/google_sign_in_web/pubspec.yaml
@@ -29,3 +29,7 @@ dependencies:
 dev_dependencies:
   flutter_test:
     sdk: flutter
+
+dependency_overrides:
+  google_sign_in_platform_interface:
+    path: ../google_sign_in_platform_interface
\ No newline at end of file

Let me know how that looks. I can open a separate PR to update the platform interface first if that would be preferable.

[stuartmorgan-g]

Since both the platform interface and plugin are changing, this patch is needed to make the tests pass

See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins for the process to follow for multi-package changes.

[fbcouch]

@stuartmorgan Thanks – taking a look at the process – is adding the forceCodeForRefreshToken param considered a breaking change? I assume "yes" since all implementations have to update init() overrides – would it make sense to add another constructor instead to avoid the breaking change?

[stuartmorgan-g]

Yes, see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-platform-interface-method-parameters

[fbcouch]

@stuartmorgan I think I have this in a good spot now, let me know how this looks and I can get the platform interface pull request going.

[fbcouch]

@stuartmorgan As far as I know, android is the only platform that needs this extra parameter. On iOS and web, I believe server auth codes always return a refresh token.

[ditman]

AFAIK web doesn't retrieve serverAuthCode, because it requires a completely different authentication flow that is not supported: https://github.com/flutter/flutter/issues/62474 (by the plugin, not by the SDK)

[ditman]

(I can't remove myself from here, because I'm a code-owner, it seems! I'll ship the web override when the change gets to that package!)

[stuartmorgan-g]

@blasten Ping on this review

[stuartmorgan-g]

Adding @camsim99 for Android review, per the new owners mapping.

[stuartmorgan-g]

@fbcouch Now that this has approval, you can go ahead and split out the platform interface part as a separate PR, and we can get that reviewed and landed to start unwinding this main PR.

[jmagman]

Formatting issue:

To fix run "pub global activate flutter_plugin_tools && pub global run flutter_plugin_tools format" or copy-paste this command into your terminal:
patch -p1 <<DONE
diff --git a/packages/google_sign_in/google_sign_in_android/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java b/packages/google_sign_in/google_sign_in_android/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java
index e49a55b08..d9df30ac9 100644
--- a/packages/google_sign_in/google_sign_in_android/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java
+++ b/packages/google_sign_in/google_sign_in_android/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java
@@ -42,8 +42,6 @@ import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
-import android.content.res.Resources;
-import android.util.Log;
 /** Google sign-in plugin for Flutter. */
 public class GoogleSignInPlugin implements MethodCallHandler, FlutterPlugin, ActivityAware {
@@ -260,26 +258,25 @@ public class GoogleSignInPlugin implements MethodCallHandler, FlutterPlugin, Act
   }
   /**
-   * Factory class that generates the GoogleSignInOptions.Builder. This is exposed so that tests
-   * can inject a mock instance of the GoogleSignInOptions.Builder.
+   * Factory class that generates the GoogleSignInOptions.Builder. This is exposed so that tests can
+   * inject a mock instance of the GoogleSignInOptions.Builder.
    */
   public static class OptionsBuilderFactory {
     public static final String DEFAULT_SIGN_IN = "SignInOption.standard";
     public static final String DEFAULT_GAMES_SIGN_IN = "SignInOption.games";
-    /**
-     * Returns an instance of GoogleSignInOptions.Builder with the passed signInOption.
-     */
+    /** Returns an instance of GoogleSignInOptions.Builder with the passed signInOption. */
     public GoogleSignInOptions.Builder get(String signInOption) {
       switch (signInOption) {
-          case DEFAULT_GAMES_SIGN_IN:
-            return new GoogleSignInOptions.Builder(GoogleSignInOptions.DEFAULT_GAMES_SIGN_IN);
+        case DEFAULT_GAMES_SIGN_IN:
+          return new GoogleSignInOptions.Builder(GoogleSignInOptions.DEFAULT_GAMES_SIGN_IN);
-          case DEFAULT_SIGN_IN:
-            return new GoogleSignInOptions.Builder(GoogleSignInOptions.DEFAULT_SIGN_IN).requestEmail();
-          default:
-            throw new IllegalStateException("Unknown signInOption");
-        }
+        case DEFAULT_SIGN_IN:
+          return new GoogleSignInOptions.Builder(GoogleSignInOptions.DEFAULT_SIGN_IN)
+              .requestEmail();
+        default:
+          throw new IllegalStateException("Unknown signInOption");
+      }
     }
   }
@@ -365,8 +362,8 @@ public class GoogleSignInPlugin implements MethodCallHandler, FlutterPlugin, Act
         return clientIdIdentifierOverride;
       }
       return context
-                .getResources()
-                .getIdentifier("default_web_client_id", "string", context.getPackageName());
+          .getResources()
+          .getIdentifier("default_web_client_id", "string", context.getPackageName());
     }
     /**
diff --git a/packages/google_sign_in/google_sign_in_android/android/src/test/java/io/flutter/plugins/googlesignin/GoogleSignInTest.java b/packages/google_sign_in/google_sign_in_android/android/src/test/java/io/flutter/plugins/googlesignin/GoogleSignInTest.java
index 563849949..3f15cb9af 100644
--- a/packages/google_sign_in/google_sign_in_android/android/src/test/java/io/flutter/plugins/googlesignin/GoogleSignInTest.java
+++ b/packages/google_sign_in/google_sign_in_android/android/src/test/java/io/flutter/plugins/googlesignin/GoogleSignInTest.java
@@ -47,8 +47,10 @@ public class GoogleSignInTest {
     when(mockRegistrar.messenger()).thenReturn(mockMessenger);
     when(mockRegistrar.context()).thenReturn(mockContext);
     when(mockRegistrar.activity()).thenReturn(mockActivity);
-    when(optionsBuilderFactory.get(GoogleSignInPlugin.OptionsBuilderFactory.DEFAULT_SIGN_IN)).thenReturn(optionsBuilder);
-    when(optionsBuilderFactory.get(GoogleSignInPlugin.OptionsBuilderFactory.DEFAULT_GAMES_SIGN_IN)).thenReturn(optionsBuilder);
+    when(optionsBuilderFactory.get(GoogleSignInPlugin.OptionsBuilderFactory.DEFAULT_SIGN_IN))
+        .thenReturn(optionsBuilder);
+    when(optionsBuilderFactory.get(GoogleSignInPlugin.OptionsBuilderFactory.DEFAULT_GAMES_SIGN_IN))
+        .thenReturn(optionsBuilder);
     plugin = new GoogleSignInPlugin();
     plugin.initInstance(mockRegistrar.messenger(), mockRegistrar.context(), mockGoogleSignIn);
     plugin.setUpRegistrar(mockRegistrar);
DONE

[stuartmorgan-g]

Sorry, I lost track of the state of this PR. The next step here would be to split the implementation package parts into a new PR. While from a compilation standpoint this would be landable as-is, we don't want to add a new app-facing parameter that doesn't actually work (if someone updates only the app-facing package, which happens more than you might think). So generally we land the implementations, and then the final PR for the app-facing package updates the minimum version of the implementation packages (in this case we only need to rev Android) so that when someone gets the new API they will definitely also get a working implementation of it.

[fbcouch]

Sorry, I lost track of the state of this PR. The next step here would be to split the implementation package parts into a new PR. While from a compilation standpoint this would be landable as-is, we don't want to add a new app-facing parameter that doesn't actually work (if someone updates only the app-facing package, which happens more than you might think). So generally we land the implementations, and then the final PR for the app-facing package updates the minimum version of the implementation packages (in this case we only need to rev Android) so that when someone gets the new API they will definitely also get a working implementation of it.

Sorry, I haven't had a chance to come back to this in awhile – so you are saying I should separate the google_sign_in_x parts into another separate PR, get that landed, then finally land the changes to google_sign_in itself?

[fbcouch]

All right assuming that is correct, the platform implementations are here: https://github.com/flutter/plugins/pull/6130

[stuartmorgan-g]

so you are saying I should separate the google_sign_in_x parts into another separate PR, get that landed, then finally land the changes to google_sign_in itself?

Yes, that's correct; once the implementation lands, then there will be published versions you can use as the new minimum constraints in the app-facing package.

[stuartmorgan-g]

The platform interface changes have landed and been published, so the implementation parts can now be split out into a new PR.