Add support for handling UserRecoverableAuthException #771

bparrishMines · 2018-09-06T22:19:42Z

alanrussian · 2018-09-06T23:20:27Z

packages/google_sign_in/lib/google_sign_in.dart

+/// Is thrown only on android.
+///
+/// Dart equivalent of android `UserRecoverableAuthException`.
+class AndroidUserRecoverableAuthException implements Exception {


This doesn't help mulligan, but is there an equivalent to this for iOS? Just wondering if it's necessary to put "Android" in the name here I guess if iOS support is ever added.

Oh sorry. I think I misunderstood your request then. Did you want a functionality similar to this, but for iOS?

My team doesn't need it for iOS; however, I assume the same problem exists on iOS, and I'm asking if it might be a good idea to not scope this API specifically to Android to avoid a breaking change in the future if support for iOS were added. To be clear, this pull request probably does what we need (still have to look over it more thoroughly).

Agreed. I removed Android. Also, I was unable to find a similar API for iOS so far.

alanrussian

I'll review this more thoroughly tomorrow. Thanks for the quick turnaround!

alanrussian · 2018-09-06T23:30:44Z

...google_sign_in/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java

+      if (userRecoverableAuthIntent == null) {
+        throw new IllegalStateException("No recoverable auth intent available.");
+      } else if (currentAccount != null && !accountId.equals(currentAccount.getId())) {
+        throw new IllegalStateException("No recoverable auth intent for this account.");


Instead of throwing, can we make the result a failure? I would prefer to not cause the app to crash.

I changed it to throw a PlatformException instead. This check is to just make sure the developer doesn't sign in -> get UserRecoverAuthException -> log out -> try to start intent and recover Auth.

Also, recoverAuth() now returns a boolean to indicate if the User took successful actions to recover auth.

alanrussian · 2018-09-06T23:31:40Z

packages/google_sign_in/lib/google_sign_in.dart

@@ -55,16 +81,35 @@ class GoogleSignInAccount implements GoogleIdentity {
  final String _idToken;
  final GoogleSignIn _googleSignIn;

+  /// Retrieve [GoogleSignInAuthentication] for this account.
+  ///
+  /// Throws a [AndroidUserRecoverableAuthException] on Android to signal that a


Nit: Throws an

alanrussian · 2018-09-10T18:09:38Z

...google_sign_in/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java

+    public void recoverAuth(Result result, String accountId) {
+      if (currentAccount == null
+          || userRecoverableAuthIntent == null
+          || !accountId.equals(currentAccount.getId())) {


I don't think this will work for us. We have use cases where we try to get the token when no account is selected. We try to get the token for every account before any account is selected.

Some ideas:

Store every Intent. While this leaks memory, I think intents are pretty tiny and don't hold any resources.

Map of account to Intent. Slightly better than above.

Have an option in the get token method to automatically launch the recovery flow if necessary.

cc @jacobklintg who thought about this for iOS

Alan covered the cases pretty well. We also want to be able to support silent sign-in attempts, which explicitly do not show any UI.

I think an optional boolean parameter at the dart level (defaulting to true) to control whether or not to attempt reauth when needed will work for us on both platforms.

It might be simplest on the app developer if the plugin just passes this boolean along to the native code and lets it perform reauth if desired, without making the developer explicitly perform reauth. Either the boolean is true so the developer wants reauth (which I imagine is the common case), or else it's false and the call will fail if reauth is needed.

@alanrussian @jklint-g I added an option to attempt to recover auth automatically once. If it doesn't work, authentication will return null.

packages/google_sign_in/CHANGELOG.md

alanrussian · 2018-09-18T18:18:45Z

...google_sign_in/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java

@@ -135,7 +137,7 @@ public void init(
     * Gets an OAuth access token with the scopes that were specified during initialization for the
     * user with the specified email address.
     */
-    public void getTokens(final Result result, final String email);
+    public void getTokens(final Result result, final String email, final boolean shouldRecoverAuth);


Can you document shouldRecoverAuth?

alanrussian · 2018-09-18T18:22:18Z

...google_sign_in/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java

@@ -342,7 +346,26 @@ public void run(Future<String> tokenFuture) {
                // instead of the value we cached during sign in. At least, that's
                // how it works on iOS.
                result.success(tokenResult);
-              } catch (ExecutionException e) {
+              } catch (final ExecutionException e) {
+                if (shouldRecoverAuth && e.getCause() instanceof UserRecoverableAuthException) {


If there's a UserRecoverableAuthException but shouldRecoverAuth is false, it might still be useful to report the exception to the Dart side (without the recover method that you had before).

alanrussian · 2018-09-18T18:26:16Z

...google_sign_in/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java

+            getTokens(pendingOperation.result, (String) pendingOperation.data, false);
+            pendingOperation = null;
+          } else {
+            finishWithSuccess(null);


My instinct here is that it would be better to make the result a failure. Is there a reason why you prefer a success result with null?

packages/google_sign_in/test/google_sign_in_test.dart

alanrussian · 2018-09-18T18:28:11Z

packages/google_sign_in/lib/google_sign_in.dart

-  Future<GoogleSignInAuthentication> get authentication async {
+  /// Retrieve [GoogleSignInAuthentication] for this account.
+  ///
+  /// `shouldRecoverAuth` sets whether to attempt to recover authentication if


Nit: [shouldRecoverAuth] ?

mehmetf

I would like to review this more carefully later today. Will post my updates.

mehmetf · 2018-09-20T15:10:56Z

...google_sign_in/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java

@@ -198,12 +206,12 @@ public GoogleSignInAccount getCurrentAccount() {
      return currentAccount;
    }

-    private void checkAndSetPendingOperation(String method, Result result) {
+    private void checkAndSetPendingOperation(String method, Result result, Object data) {


Keep the existing signature and just call checkAndSetPendingOperation with null data. That way you don't need to update all callers.

mehmetf · 2018-09-20T15:12:58Z

...google_sign_in/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java

+          return true;
+        } else if (requestCode == REQUEST_CODE_RECOVER_AUTH) {
+          if (resultCode == Activity.RESULT_OK) {
+            getTokens(pendingOperation.result, (String) pendingOperation.data, false);


Do we know the pendingOperation type?

I would assert that pendingOperation was getTokens().

mehmetf

Given @alanrussian 's use case, I think this PR is good. Thanks Maurice.

mehmetf · 2018-09-20T19:21:12Z

...google_sign_in/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java

+                              public void run() {
+                                UserRecoverableAuthException exception =
+                                    (UserRecoverableAuthException) e.getCause();
+                                checkAndSetPendingOperation(METHOD_GET_TOKENS, result, email);


Why set this here instead of at the very beginning?

mehmetf · 2018-09-20T19:24:13Z

...google_sign_in/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java

@@ -464,6 +506,19 @@ public boolean onActivityResult(int requestCode, int resultCode, Intent data) {
          } else if (pendingOperation != null && pendingOperation.method.equals(METHOD_INIT)) {
            finishWithError(ERROR_REASON_CONNECTION_FAILED, String.valueOf(resultCode));
          }
+          return true;
+        } else if (requestCode == REQUEST_CODE_RECOVER_AUTH) {


Perhaps this is big enough now that we should make it a switch statement:

switch (requestCode) { case REQUEST_CODE_RESOLVE_ERROR: case REQUEST_CODE_RECOVER_AUTH: case REQUEST_CODE: break; default: return false; }

mehmetf · 2018-09-20T19:25:29Z

...google_sign_in/android/src/main/java/io/flutter/plugins/googlesignin/GoogleSignInPlugin.java

@@ -161,6 +166,7 @@ public void init(
   */
  public static final class Delegate implements IDelegate {
    private static final int REQUEST_CODE = 53293;
+    private static final int REQUEST_CODE_RECOVER_AUTH = 12345;


Just increment REQUEST_CODE by 1.

mehmetf · 2018-09-20T19:27:30Z

packages/google_sign_in/lib/google_sign_in.dart

+  /// recovered by user action a [PlatformException] is thrown with error code
+  /// [kUserRecoverableAuthError].
+  Future<GoogleSignInAuthentication> getAuthentication({
+    bool shouldRecoverAuth = true,


Can we set this as a property of the Account (or even a property of the GoogleSignIn API itself?)

That way it is not a breaking change. Otherwise, I assume you also need to make authHeaders a function that takes this parameter. Most apps do not deal with tokens directly. They just fetch the headers.

I confirmed that Mulligan is OK with this. They don't care about this Dart interface since they don't use it internally.

I now propose that we don't make a change to the Dart interface at all. We just pass in true as shouldRecoverAuth in here.

Add support for handling UserRecoverableAuthException (flutter#771)

…r#771)" This reverts commit c9e40d6.

bparrishMines added 4 commits September 5, 2018 13:10

change start

995a9c3

Setup android side of UserRecoverableAuthException

f3cef76

Dart side of UserRecoverableAuthException

79d456c

Comments and tests

aec5637

googlebot added the cla: yes label Sep 6, 2018

bparrishMines added 2 commits September 6, 2018 15:30

Version bump + small code cleanup

7c8482a

formatting

3c7d2e4

bparrishMines requested review from amirh and alanrussian September 6, 2018 23:17

Another few small changes

6c07624

alanrussian reviewed Sep 6, 2018

View reviewed changes

bparrishMines added 2 commits September 7, 2018 15:19

recoverAuth now indicates success or failure

9af762e

formatting

267a845

alanrussian reviewed Sep 10, 2018

View reviewed changes

kroikie added flutterfire and removed flutterfire labels Sep 10, 2018

bparrishMines added 6 commits September 13, 2018 15:39

Change to auto start activity to recover auth

15242ff

Spaces

aff9766

Return null on failure to recover auth

6112120

Formatting

a0f3086

Merge branch 'master' of github.com:flutter/plugins into google_sign

73e2476

Removed unused variable

dcdd03c

alanrussian reviewed Sep 18, 2018

View reviewed changes

packages/google_sign_in/CHANGELOG.md Show resolved Hide resolved

alanrussian suggested changes Sep 18, 2018

View reviewed changes

bparrishMines added 4 commits September 19, 2018 11:19

Semantic Versioning

541efd2

Comments update

1182d29

Error code for auth failures

c60c745

Formatting

9ad805d

mehmetf reviewed Sep 20, 2018

View reviewed changes

bparrishMines added 2 commits September 20, 2018 10:20

Additional method

646d6e4

Verify method

f508ed6

mehmetf reviewed Sep 20, 2018

View reviewed changes

Revert method name and always pass true

d479e18

bparrishMines merged commit b889b40 into flutter:master Sep 21, 2018

bparrishMines deleted the google_sign branch September 21, 2018 18:26

mridul-sahu added a commit to mridul-sahu/plugins that referenced this pull request Sep 22, 2018

Merge pull request #1 from flutter/master

5c8dd30

Add support for handling UserRecoverableAuthException (flutter#771)

andreidiaconu pushed a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019

Add support for handling UserRecoverableAuthException (flutter#771)

c9e40d6

andreidiaconu added a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019

Revert "Add support for handling UserRecoverableAuthException (flutte…

4ce82e3

…r#771)" This reverts commit c9e40d6.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for handling UserRecoverableAuthException #771

Add support for handling UserRecoverableAuthException #771

bparrishMines commented Sep 6, 2018

alanrussian Sep 6, 2018

bparrishMines Sep 7, 2018

alanrussian Sep 7, 2018

bparrishMines Sep 7, 2018

alanrussian left a comment

alanrussian Sep 6, 2018

bparrishMines Sep 7, 2018

alanrussian Sep 6, 2018

alanrussian Sep 10, 2018 •

edited

Loading

alanrussian Sep 10, 2018

jklint-g Sep 10, 2018

bparrishMines Sep 18, 2018

alanrussian Sep 18, 2018

alanrussian Sep 18, 2018

alanrussian Sep 18, 2018

alanrussian Sep 18, 2018

mehmetf left a comment

mehmetf Sep 20, 2018

mehmetf Sep 20, 2018

mehmetf left a comment

mehmetf Sep 20, 2018

mehmetf Sep 20, 2018

mehmetf Sep 20, 2018

mehmetf Sep 20, 2018

mehmetf Sep 20, 2018

Add support for handling UserRecoverableAuthException #771

Add support for handling UserRecoverableAuthException #771

Conversation

bparrishMines commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanrussian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanrussian Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehmetf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehmetf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanrussian Sep 10, 2018 •

edited

Loading