Use IdentityManager.java observer in SigninManager

SigninManager uses IdentityManager's observer (onPrimaryAccountSet/Cleared)
through IdentityManager.java instead of signin_manager_android.

Bug: 934688
Change-Id: I037906b6c59254a9c205d615284c503af268ecd3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1751245
Commit-Queue: Pâris Meuleman <pmeuleman@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Auto-Submit: Pâris Meuleman <pmeuleman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#694791}

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

@@ -31,6 +31,8 @@ import org.chromium.components.signin.AccountIdProvider;
 import org.chromium.components.signin.AccountManagerFacade;
 import org.chromium.components.signin.AccountTrackerService;
 import org.chromium.components.signin.ChromeSigninController;
+import org.chromium.components.signin.identitymanager.CoreAccountInfo;
+import org.chromium.components.signin.identitymanager.IdentityManager;
 import org.chromium.components.sync.AndroidSyncSettings;
 import org.chromium.content_public.browser.UiThreadTaskTraits;
 
@@ -47,7 +49,8 @@ import java.util.List;
  * <p/>
  * See chrome/browser/signin/signin_manager_android.h for more details.
  */
-public class SigninManager implements AccountTrackerService.OnSystemAccountsSeededListener {
+public class SigninManager
+        implements AccountTrackerService.OnSystemAccountsSeededListener, IdentityManager.Observer {
     private static final String TAG = "SigninManager";
 
     /**
@@ -184,6 +187,7 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
     private long mNativeSigninManagerAndroid;
     private final Context mContext;
     private final AccountTrackerService mAccountTrackerService;
+    private final IdentityManager mIdentityManager;
     private final AndroidSyncSettings mAndroidSyncSettings;
     private final ObserverList<SignInStateObserver> mSignInStateObservers = new ObserverList<>();
     private final ObserverList<SignInAllowedObserver> mSignInAllowedObservers =
@@ -218,29 +222,33 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
      * @return
      */
     @CalledByNative
-    private static SigninManager create(
-            long nativeSigninManagerAndroid, AccountTrackerService accountTrackerService) {
+    private static SigninManager create(long nativeSigninManagerAndroid,
+            AccountTrackerService accountTrackerService, IdentityManager identityManager) {
         assert nativeSigninManagerAndroid != 0;
         assert accountTrackerService != null;
+        assert identityManager != null;
         return new SigninManager(ContextUtils.getApplicationContext(), nativeSigninManagerAndroid,
-                accountTrackerService, AndroidSyncSettings.get());
+                accountTrackerService, identityManager, AndroidSyncSettings.get());
     }
 
     @VisibleForTesting
     SigninManager(Context context, long nativeSigninManagerAndroid,
-            AccountTrackerService accountTrackerService, AndroidSyncSettings androidSyncSettings) {
+            AccountTrackerService accountTrackerService, IdentityManager identityManager,
+            AndroidSyncSettings androidSyncSettings) {
         ThreadUtils.assertOnUiThread();
         assert context != null;
         assert androidSyncSettings != null;
         mContext = context;
         mNativeSigninManagerAndroid = nativeSigninManagerAndroid;
         mAccountTrackerService = accountTrackerService;
+        mIdentityManager = identityManager;
         mAndroidSyncSettings = androidSyncSettings;
 
         mSigninAllowedByPolicy =
                 SigninManagerJni.get().isSigninAllowedByPolicy(mNativeSigninManagerAndroid);
 
         mAccountTrackerService.addSystemAccountsSeededListener(this);
+        mIdentityManager.addObserver(this);
     }
 
     /**
@@ -249,6 +257,7 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
      */
     @CalledByNative
     public void destroy() {
+        mIdentityManager.removeObserver(this);
         mAccountTrackerService.removeSystemAccountsSeededListener(this);
         mNativeSigninManagerAndroid = 0;
     }
@@ -501,6 +510,18 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
         }
     }
 
+    /**
+     * Implements {@link IdentityManager.Observer}: take action when primary account is set.
+     * Simply verify that the request is ongoing (mSignInState != null), as only SigninManager
+     * should update IdentityManager. This is triggered by the call to
+     * SigninManagerJni.get().setPrimaryAccount.
+     */
+    @VisibleForTesting
+    @Override
+    public void onPrimaryAccountSet(CoreAccountInfo account) {
+        assert mSignInState != null;
+    }
+
     /**
      * Returns true if a sign-in or sign-out operation is in progress. See also
      * {@link #runAfterOperationInProgress}.
@@ -583,7 +604,8 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
                 callback, wipeDataHooks, forceWipeUserData || managementDomain != null);
         Log.d(TAG, "Signing out, management domain: " + managementDomain);
 
-        // User data will be wiped in disableSyncAndWipeData(), called from onNativeSignOut().
+        // User data will be wiped in disableSyncAndWipeData(), called from
+        // onPrimaryAccountcleared().
         SigninManagerJni.get().signOut(mNativeSigninManagerAndroid, signoutSource);
     }
 
@@ -625,9 +647,8 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
         notifySignInAllowedChanged();
     }
 
-    @VisibleForTesting
-    @CalledByNative
-    void onNativeSignOut() {
+    @Override
+    public void onPrimaryAccountCleared(CoreAccountInfo account) {
         if (mSignOutState == null) {
             // mSignOutState can only be null when the sign out is triggered by
             // native (since otherwise SigninManager.signOut would have created

chrome/android/junit/src/org/chromium/chrome/browser/signin/SigninManagerTest.java

@@ -15,7 +15,6 @@ import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.MockitoAnnotations.initMocks;
@@ -34,6 +33,9 @@ import org.chromium.base.test.BaseRobolectricTestRunner;
 import org.chromium.base.test.util.JniMocker;
 import org.chromium.components.signin.AccountManagerFacade;
 import org.chromium.components.signin.AccountTrackerService;
+import org.chromium.components.signin.identitymanager.CoreAccountId;
+import org.chromium.components.signin.identitymanager.CoreAccountInfo;
+import org.chromium.components.signin.identitymanager.IdentityManager;
 import org.chromium.components.sync.AndroidSyncSettings;
 
 import java.util.concurrent.atomic.AtomicInteger;
@@ -49,7 +51,9 @@ public class SigninManagerTest {
     SigninManager.Natives mNativeMock;
 
     private AccountTrackerService mAccountTrackerService;
+    private IdentityManager mIdentityManager;
     private SigninManager mSigninManager;
+    private CoreAccountInfo mAccount;
 
     @Before
     public void setUp() {
@@ -61,10 +65,16 @@ public class SigninManagerTest {
 
         mAccountTrackerService = mock(AccountTrackerService.class);
 
+        mIdentityManager = new IdentityManager(0 /* nativeIdentityManager */);
+
         AndroidSyncSettings androidSyncSettings = mock(AndroidSyncSettings.class);
 
-        mSigninManager = spy(new SigninManager(ContextUtils.getApplicationContext(),
-                0 /* nativeSigninManagerAndroid */, mAccountTrackerService, androidSyncSettings));
+        mSigninManager = new SigninManager(ContextUtils.getApplicationContext(),
+                0 /* nativeSigninManagerAndroid */, mAccountTrackerService, mIdentityManager,
+                androidSyncSettings);
+
+        mAccount = new CoreAccountInfo(new CoreAccountId("gaia-id-user"),
+                AccountManagerFacade.createAccountFromName("user@domain.com"));
     }
 
     @Test
@@ -80,13 +90,13 @@ public class SigninManagerTest {
         // Trigger the sign out flow!
         mSigninManager.signOut(SignoutReason.SIGNOUT_TEST);
 
-        // nativeSignOut should be called *before* clearing any account data.
+        // PrimaryAccountCleared should be called *before* clearing any account data.
         // http://crbug.com/589028
         verify(mNativeMock, never()).wipeProfileData(anyLong(), any());
         verify(mNativeMock, never()).wipeGoogleServiceWorkerCaches(anyLong(), any());
 
         // Simulate native callback to trigger clearing of account data.
-        mSigninManager.onNativeSignOut();
+        mIdentityManager.onPrimaryAccountCleared(mAccount);
 
         // Sign-out should only clear the profile when the user is managed.
         verify(mNativeMock, times(1)).wipeProfileData(anyLong(), any());
@@ -106,13 +116,13 @@ public class SigninManagerTest {
         // Trigger the sign out flow!
         mSigninManager.signOut(SignoutReason.SIGNOUT_TEST);
 
-        // nativeSignOut should be called *before* clearing any account data.
+        // PrimaryAccountCleared should be called *before* clearing any account data.
         // http://crbug.com/589028
         verify(mNativeMock, never()).wipeProfileData(anyLong(), any());
         verify(mNativeMock, never()).wipeGoogleServiceWorkerCaches(anyLong(), any());
 
         // Simulate native callback to trigger clearing of account data.
-        mSigninManager.onNativeSignOut();
+        mIdentityManager.onPrimaryAccountCleared(mAccount);
 
         // Sign-out should only clear the service worker cache when the user is not managed.
         verify(mNativeMock, never()).wipeProfileData(anyLong(), any());
@@ -132,13 +142,13 @@ public class SigninManagerTest {
         // Trigger the sign out flow
         mSigninManager.signOut(SignoutReason.SIGNOUT_TEST, null, null, true);
 
-        // nativeSignOut should be called *before* clearing any account data.
+        // PrimaryAccountCleared should be called *before* clearing any account data.
         // http://crbug.com/589028
         verify(mNativeMock, never()).wipeProfileData(anyLong(), any());
         verify(mNativeMock, never()).wipeGoogleServiceWorkerCaches(anyLong(), any());
 
         // Simulate native callback to trigger clearing of account data.
-        mSigninManager.onNativeSignOut();
+        mIdentityManager.onPrimaryAccountCleared(mAccount);
 
         // Sign-out should only clear the service worker cache when the user is not managed.
         verify(mNativeMock, times(1)).wipeProfileData(anyLong(), any());
@@ -153,7 +163,7 @@ public class SigninManagerTest {
         doNothing().when(mNativeMock).wipeGoogleServiceWorkerCaches(anyLong(), any());
 
         // Trigger the sign out flow!
-        mSigninManager.onNativeSignOut();
+        mIdentityManager.onPrimaryAccountCleared(mAccount);
 
         // Sign-out should only clear the profile when the user is managed.
         verify(mNativeMock, times(1)).wipeProfileData(anyLong(), any());
@@ -172,7 +182,7 @@ public class SigninManagerTest {
     @Test
     public void callbackNotifiedOnSignout() {
         doAnswer(invocation -> {
-            mSigninManager.onNativeSignOut();
+            mIdentityManager.onPrimaryAccountCleared(mAccount);
             return null;
         })
                 .when(mNativeMock)
@@ -197,9 +207,8 @@ public class SigninManagerTest {
         // invoked.
         doNothing().when(mNativeMock).fetchAndApplyCloudPolicy(anyLong(), any(), any());
 
-        doReturn(true).when(mSigninManager).isSigninSupported();
         doReturn(true).when(mNativeMock).setPrimaryAccount(anyLong(), any());
-        doNothing().when(mSigninManager).logInSignedInUser();
+        doNothing().when(mNativeMock).logInSignedInUser(anyLong());
 
         mSigninManager.onFirstRunCheckDone(); // Allow sign-in.
 

chrome/browser/android/signin/signin_manager_android.cc

@@ -134,7 +134,6 @@ SigninManagerAndroid::SigninManagerAndroid(
   DCHECK(identity_manager_);
   DCHECK(user_cloud_policy_manager_);
   DCHECK(user_policy_signin_service_);
-  identity_manager_->AddObserver(this);
 
   signin_allowed_.Init(
       prefs::kSigninAllowed, profile_->GetPrefs(),
@@ -146,7 +145,8 @@ SigninManagerAndroid::SigninManagerAndroid(
 
   java_signin_manager_ = Java_SigninManager_create(
       base::android::AttachCurrentThread(), reinterpret_cast<intptr_t>(this),
-      identity_manager_->LegacyGetAccountTrackerServiceJavaObject());
+      identity_manager_->LegacyGetAccountTrackerServiceJavaObject(),
+      identity_manager_->GetJavaObject());
 }
 
 base::android::ScopedJavaLocalRef<jobject>
@@ -157,7 +157,6 @@ SigninManagerAndroid::GetJavaObject() {
 SigninManagerAndroid::~SigninManagerAndroid() {}
 
 void SigninManagerAndroid::Shutdown() {
-  identity_manager_->RemoveObserver(this);
   Java_SigninManager_destroy(base::android::AttachCurrentThread(),
                              java_signin_manager_);
 }
@@ -218,13 +217,6 @@ jboolean SigninManagerAndroid::IsSignedInOnNative(JNIEnv* env) {
   return identity_manager_->HasPrimaryAccount();
 }
 
-void SigninManagerAndroid::OnPrimaryAccountCleared(
-    const CoreAccountInfo& previous_primary_account_info) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  Java_SigninManager_onNativeSignOut(base::android::AttachCurrentThread(),
-                                     java_signin_manager_);
-}
-
 void SigninManagerAndroid::OnSigninAllowedPrefChanged() const {
   Java_SigninManager_onSigninAllowedByPolicyChanged(
       base::android::AttachCurrentThread(), java_signin_manager_,

chrome/browser/android/signin/signin_manager_android.h

@@ -13,13 +13,17 @@
 #include "base/threading/thread_checker.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/prefs/pref_member.h"
-#include "components/signin/public/identity_manager/identity_manager.h"
 
 namespace policy {
 class UserCloudPolicyManager;
 class UserPolicySigninService;
 }  // namespace policy
 
+namespace signin {
+class IdentityManager;
+}
+
+struct CoreAccountInfo;
 class Profile;
 
 // Android wrapper of Chrome's C++ identity management code which provides
@@ -30,8 +34,7 @@ class Profile;
 //
 // This class implements parts of the sign-in flow, to make sure that policy
 // is available before sign-in completes.
-class SigninManagerAndroid : public KeyedService,
-                             public signin::IdentityManager::Observer {
+class SigninManagerAndroid : public KeyedService {
  public:
   SigninManagerAndroid(Profile* profile,
                        signin::IdentityManager* identity_manager);
@@ -61,10 +64,6 @@ class SigninManagerAndroid : public KeyedService,
 
   jboolean IsSignedInOnNative(JNIEnv* env);
 
-  // signin::IdentityManager::Observer implementation.
-  void OnPrimaryAccountCleared(
-      const CoreAccountInfo& previous_primary_account_info) override;
-
   // Registers a CloudPolicyClient for fetching policy for a user and fetches
   // the policy if necessary.
   void FetchAndApplyCloudPolicy(

components/signin/public/identity_manager/android/java/src/org/chromium/components/signin/identitymanager/IdentityManager.java

@@ -5,6 +5,7 @@
 package org.chromium.components.signin.identitymanager;
 
 import org.chromium.base.ObserverList;
+import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.NativeMethods;
 
@@ -42,11 +43,12 @@ public class IdentityManager {
      */
     @CalledByNative
     static private IdentityManager create(long nativeIdentityManager) {
+        assert nativeIdentityManager != 0;
         return new IdentityManager(nativeIdentityManager);
     }
-    private IdentityManager(long nativeIdentityManager) {
-        assert nativeIdentityManager != 0;
 
+    @VisibleForTesting
+    public IdentityManager(long nativeIdentityManager) {
         mNativeIdentityManager = nativeIdentityManager;
     }
 
@@ -86,7 +88,8 @@ public class IdentityManager {
      * Notifies observers that the primary account was cleared in C++.
      */
     @CalledByNative
-    private void onPrimaryAccountCleared(CoreAccountInfo account) {
+    @VisibleForTesting
+    public void onPrimaryAccountCleared(CoreAccountInfo account) {
         for (Observer observer : mObservers) {
             observer.onPrimaryAccountCleared(account);
         }