[Android] Wait for cookie regeneration to redirect URL for web sign-in

This CL redirects the continue URL after the signed-in account is in
the cookie jar.

Bug: 1093741
Change-Id: Ia19a856fa50b1a27e2373d42499908461ba8573b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2339355
Commit-Queue: Alice Wang <aliceywang@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Tanmoy Mollik <triploblastic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800029}

chrome/android/chrome_junit_test_java_sources.gni

@@ -215,6 +215,7 @@ chrome_junit_test_java_sources = [
   "junit/src/org/chromium/chrome/browser/signin/SigninPreferencesManagerTest.java",
   "junit/src/org/chromium/chrome/browser/signin/SigninPromoUtilTest.java",
   "junit/src/org/chromium/chrome/browser/signin/SigninUtilsStartActivityTest.java",
+  "junit/src/org/chromium/chrome/browser/signin/account_picker/AccountPickerDelegateTest.java",
   "junit/src/org/chromium/chrome/browser/signin/account_picker/AccountPickerMediatorTest.java",
   "junit/src/org/chromium/chrome/browser/site_settings/SingleWebsiteSettingsTest.java",
   "junit/src/org/chromium/chrome/browser/status_indicator/StatusIndicatorMediatorTest.java",

chrome/android/java/src/org/chromium/chrome/browser/signin/SigninUtils.java

@@ -83,7 +83,8 @@ public class SigninUtils {
             AccountPickerBottomSheetCoordinator coordinator =
                     new AccountPickerBottomSheetCoordinator(activity,
                             BottomSheetControllerProvider.from(activity.getWindowAndroid()),
-                            new AccountPickerDelegate(windowAndroid, continueUrl));
+                            new AccountPickerDelegate(
+                                    windowAndroid, new WebSigninBridge.Factory(), continueUrl));
         }
     }
 

chrome/android/java/src/org/chromium/chrome/browser/signin/WebSigninBridge.java

@@ -35,6 +35,22 @@ public class WebSigninBridge {
         void onSigninFailed(GoogleServiceAuthError error);
     }
 
+    /**
+     * Factory to create WebSigninBridge object.
+     */
+    public static class Factory {
+        /**
+         * Creates a WebSigninBridge object.
+         *
+         * @param profile The profile to use for the sign-in.
+         * @param account The primary account account used for the sign-in process.
+         * @param listener The listener to be notified about sign-in completion.
+         */
+        public WebSigninBridge create(Profile profile, CoreAccountInfo account, Listener listener) {
+            return new WebSigninBridge(profile, account, listener);
+        }
+    }
+
     private long mNativeWebSigninBridge;
 
     /**
@@ -44,7 +60,7 @@ public class WebSigninBridge {
      * @param account The primary account account used for the sign-in process.
      * @param listener The listener to be notified about sign-in completion.
      */
-    public WebSigninBridge(Profile profile, CoreAccountInfo account, Listener listener) {
+    private WebSigninBridge(Profile profile, CoreAccountInfo account, Listener listener) {
         assert account != null && listener != null;
         mNativeWebSigninBridge = WebSigninBridgeJni.get().create(profile, account, listener);
         assert mNativeWebSigninBridge != 0 : "Couldn't create native WebSigninBridge object!";

chrome/android/java/src/org/chromium/chrome/browser/signin/account_picker/AccountPickerDelegate.java

@@ -8,10 +8,11 @@ import android.accounts.AccountManager;
 import android.app.Activity;
 import android.content.Intent;
 
+import androidx.annotation.MainThread;
 import androidx.annotation.Nullable;
 
 import org.chromium.base.Callback;
-import org.chromium.base.task.PostTask;
+import org.chromium.base.ThreadUtils;
 import org.chromium.chrome.browser.app.ChromeActivity;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.signin.IdentityServicesProvider;
@@ -24,7 +25,6 @@ import org.chromium.components.signin.base.CoreAccountInfo;
 import org.chromium.components.signin.base.GoogleServiceAuthError;
 import org.chromium.components.signin.metrics.SigninAccessPoint;
 import org.chromium.content_public.browser.LoadUrlParams;
-import org.chromium.content_public.browser.UiThreadTaskTraits;
 import org.chromium.ui.base.WindowAndroid;
 
 /**
@@ -37,15 +37,18 @@ public class AccountPickerDelegate implements WebSigninBridge.Listener {
     private final WindowAndroid mWindowAndroid;
     private final ChromeActivity mChromeActivity;
     private final Tab mTab;
+    private final WebSigninBridge.Factory mWebSigninBridgeFactory;
     private final String mContinueUrl;
     private final SigninManager mSigninManager;
-    private WebSigninBridge mWebSigninBridge;
+    private @Nullable WebSigninBridge mWebSigninBridge;
     private Callback<GoogleServiceAuthError> mOnSignInErrorCallback;
 
-    public AccountPickerDelegate(WindowAndroid windowAndroid, String continueUrl) {
+    public AccountPickerDelegate(WindowAndroid windowAndroid,
+            WebSigninBridge.Factory webSigninBridgeFactory, String continueUrl) {
         mWindowAndroid = windowAndroid;
         mChromeActivity = (ChromeActivity) mWindowAndroid.getActivity().get();
         mTab = mChromeActivity.getActivityTab();
+        mWebSigninBridgeFactory = webSigninBridgeFactory;
         mContinueUrl = continueUrl;
         mSigninManager = IdentityServicesProvider.get().getSigninManager(
                 Profile.getLastUsedRegularProfile());
@@ -55,10 +58,7 @@ public class AccountPickerDelegate implements WebSigninBridge.Listener {
      * Releases resources used by this class.
      */
     public void onDismiss() {
-        if (mWebSigninBridge != null) {
-            mWebSigninBridge.destroy();
-            mWebSigninBridge = null;
-        }
+        destroyWebSigninBridge();
     }
 
     /**
@@ -67,27 +67,27 @@ public class AccountPickerDelegate implements WebSigninBridge.Listener {
     public void signIn(CoreAccountInfo coreAccountInfo,
             Callback<GoogleServiceAuthError> onSignInErrorCallback) {
         mOnSignInErrorCallback = onSignInErrorCallback;
+        mWebSigninBridge = mWebSigninBridgeFactory.create(
+                Profile.getLastUsedRegularProfile(), coreAccountInfo, this);
         mSigninManager.signIn(
                 SigninAccessPoint.WEB_SIGNIN, coreAccountInfo, new SigninManager.SignInCallback() {
                     @Override
                     public void onSignInComplete() {
-                        // TODO(https://crbug.com/1092399): Implement sign-in properly in delegate
-                        // We should wait for the cookie signin and cookie regeneration here,
-                        // PostTask.postDelayedTask is just a temporary measure for testing the
-                        // flow, it will be removed soon.
-                        PostTask.postDelayedTask(UiThreadTaskTraits.DEFAULT,
-                                () -> { mTab.loadUrl(new LoadUrlParams(mContinueUrl)); }, 2000);
+                        // After the sign-in is finished in Chrome, we still need to wait for
+                        // WebSigninBridge to be called to redirect to the continue url.
                     }
 
                     @Override
                     public void onSignInAborted() {
-                        // TODO(https//crbug.com/1092399): Add UI to show in this case
+                        AccountPickerDelegate.this.destroyWebSigninBridge();
                     }
                 });
     }
 
     /**
      * Notifies when the user clicked the "add account" button.
+     *
+     * TODO(https//crbug.com/1117000): Change the callback argument to CoreAccountInfo
      */
     public void addAccount(Callback<String> callback) {
         AccountManagerFacadeProvider.getInstance().createAddAccountIntent(
@@ -119,11 +119,14 @@ public class AccountPickerDelegate implements WebSigninBridge.Listener {
     public void goIncognitoMode() {}
 
     /**
-     * TODO(https://crbug.com/1092399): Redirect URL when web sign-in succeeded.
      * Sign-in completed successfully and the primary account is available in the cookie jar.
      */
+    @MainThread
     @Override
-    public void onSigninSucceded() {}
+    public void onSigninSucceded() {
+        ThreadUtils.assertOnUiThread();
+        mTab.loadUrl(new LoadUrlParams(mContinueUrl));
+    }
 
     /**
      * Sign-in process failed.
@@ -134,4 +137,11 @@ public class AccountPickerDelegate implements WebSigninBridge.Listener {
     public void onSigninFailed(GoogleServiceAuthError error) {
         mOnSignInErrorCallback.onResult(error);
     }
+
+    private void destroyWebSigninBridge() {
+        if (mWebSigninBridge != null) {
+            mWebSigninBridge.destroy();
+            mWebSigninBridge = null;
+        }
+    }
 }

chrome/android/junit/src/org/chromium/chrome/browser/signin/account_picker/AccountPickerDelegateTest.java

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.signin.account_picker;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.inOrder;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static org.mockito.MockitoAnnotations.initMocks;
+
+import android.accounts.Account;
+import android.app.Activity;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Answers;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
+import org.mockito.InOrder;
+import org.mockito.Mock;
+
+import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.chrome.browser.app.ChromeActivity;
+import org.chromium.chrome.browser.profiles.Profile;
+import org.chromium.chrome.browser.signin.IdentityServicesProvider;
+import org.chromium.chrome.browser.signin.SigninManager;
+import org.chromium.chrome.browser.signin.WebSigninBridge;
+import org.chromium.chrome.test.util.browser.signin.AccountManagerTestRule;
+import org.chromium.components.signin.base.CoreAccountInfo;
+import org.chromium.components.signin.identitymanager.IdentityManager;
+import org.chromium.components.signin.metrics.SigninAccessPoint;
+import org.chromium.content_public.browser.LoadUrlParams;
+import org.chromium.ui.base.WindowAndroid;
+
+import java.lang.ref.WeakReference;
+
+/**
+ * This class tests the {@link AccountPickerDelegate}.
+ */
+@RunWith(BaseRobolectricTestRunner.class)
+public class AccountPickerDelegateTest {
+    private static final String CONTINUE_URL = "https://test-continue-url.com";
+
+    @Rule
+    public final AccountManagerTestRule mAccountManagerTestRule = new AccountManagerTestRule();
+
+    @Mock
+    public WebSigninBridge.Factory mWebSigninBridgeFactoryMock;
+
+    @Mock
+    private SigninManager mSigninManagerMock;
+
+    @Mock
+    private Profile mProfileMock;
+
+    @Mock
+    private WindowAndroid mWindowAndroidMock;
+
+    @Mock(answer = Answers.RETURNS_DEEP_STUBS)
+    private ChromeActivity mChromeActivityMock;
+
+    @Captor
+    private ArgumentCaptor<LoadUrlParams> mLoadUrlParamsCaptor;
+
+    private AccountPickerDelegate mDelegate;
+
+    private final IdentityManager mIdentityManager =
+            new IdentityManager(/* nativeIdentityManager= */ 0, /* OAuth2TokenService= */ null);
+
+    @Before
+    public void setUp() {
+        initMocks(this);
+        Profile.setLastUsedProfileForTesting(mProfileMock);
+        IdentityServicesProvider.setInstanceForTests(mock(IdentityServicesProvider.class));
+        when(IdentityServicesProvider.get().getIdentityManager(any())).thenReturn(mIdentityManager);
+        when(IdentityServicesProvider.get().getSigninManager(any())).thenReturn(mSigninManagerMock);
+
+        when(mWindowAndroidMock.getActivity())
+                .thenReturn(new WeakReference<Activity>(mChromeActivityMock));
+        mDelegate = new AccountPickerDelegate(
+                mWindowAndroidMock, mWebSigninBridgeFactoryMock, CONTINUE_URL);
+    }
+
+    @After
+    public void tearDown() {
+        mDelegate.onDismiss();
+    }
+
+    @Test
+    public void testSignInSucceeded() {
+        Account account =
+                mAccountManagerTestRule.addAccount(AccountManagerTestRule.TEST_ACCOUNT_EMAIL);
+        CoreAccountInfo coreAccountInfo = mAccountManagerTestRule.toCoreAccountInfo(account.name);
+        mDelegate.signIn(coreAccountInfo, error -> {});
+        InOrder calledInOrder = inOrder(mWebSigninBridgeFactoryMock, mSigninManagerMock);
+        calledInOrder.verify(mWebSigninBridgeFactoryMock)
+                .create(mProfileMock, coreAccountInfo, mDelegate);
+        calledInOrder.verify(mSigninManagerMock)
+                .signIn(eq(SigninAccessPoint.WEB_SIGNIN), eq(coreAccountInfo), any());
+        mDelegate.onSigninSucceded();
+        verify(mChromeActivityMock.getActivityTab()).loadUrl(mLoadUrlParamsCaptor.capture());
+        LoadUrlParams loadUrlParams = mLoadUrlParamsCaptor.getValue();
+        Assert.assertEquals("Continue url does not match!", CONTINUE_URL, loadUrlParams.getUrl());
+    }
+}

chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/signin/AccountManagerTestRule.java

@@ -81,6 +81,8 @@ public class AccountManagerTestRule implements TestRule {
     }
 
     /**
+     * TODO(https://crbug.com/1117006): Change the return type of addAccount() to CoreAccountInfo
+     *
      * Add an account to the fake AccountManagerFacade.
      * @return The account added.
      */