Refactor policy registration and isUserManaged

The main purpose here is the refactoring of the almost identical
RegisterAndFetchPolicyBeforeSignIn and isUserManager. This brought some
simplification on the java code, with the removal of the native call to
ShouldLoadPolicyForUser.

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

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

@@ -443,13 +443,6 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
             return;
         }
 
-        if (!SigninManagerJni.get().shouldLoadPolicyForUser(mSignInState.mAccount.name)) {
-            // Proceed with the sign-in flow without checking for policy if it can be determined
-            // that this account can't have management enabled based on the username.
-            finishSignIn();
-            return;
-        }
-
         Log.d(TAG, "Checking if account has policy management enabled");
         // This will call back to onPolicyFetchedBeforeSignIn.
         SigninManagerJni.get().registerAndFetchPolicyBeforeSignIn(
@@ -458,9 +451,11 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
 
     @CalledByNative
     @VisibleForTesting
+    /**
+     * If the user is managed, its policy has been fetched and is being enforced; features like sync
+     * may now be disabled by policy, and the rest of the sign-in flow can be resumed.
+     */
     void onPolicyFetchedBeforeSignIn() {
-        // Policy has been fetched for the user and is being enforced; features like sync may now
-        // be disabled by policy, and the rest of the sign-in flow can be resumed.
         finishSignIn();
     }
 
@@ -727,7 +722,7 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
      *         otherwise. May be called synchronously from this function.
      */
     public void isUserManaged(String email, final Callback<Boolean> callback) {
-        SigninManagerJni.get().isUserManaged(email, callback);
+        SigninManagerJni.get().isUserManaged(this, mNativeSigninManagerAndroid, email, callback);
     }
 
     public static String extractDomainName(String email) {
@@ -773,9 +768,8 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
 
         boolean isSignedInOnNative(@JCaller SigninManager self, long nativeSigninManagerAndroid);
 
-        boolean shouldLoadPolicyForUser(String username);
-
-        void isUserManaged(String username, Callback<Boolean> callback);
+        void isUserManaged(@JCaller SigninManager self, long nativeSigninManagerAndroid,
+                String username, Callback<Boolean> callback);
 
         String extractDomainName(String email);
     }

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

@@ -212,7 +212,6 @@ public class SigninManagerTest {
         doReturn(true).when(mAccountTrackerService).checkAndSeedSystemAccounts();
         // Request that policy is loaded. It will pause sign-in until onPolicyCheckedBeforeSignIn is
         // invoked.
-        doReturn(true).when(mNativeMock).shouldLoadPolicyForUser(any());
         doNothing().when(mNativeMock).registerAndFetchPolicyBeforeSignIn(any(), anyLong(), any());
 
         doReturn(true).when(mSigninManager).isSigninSupported();

chrome/browser/android/signin/signin_manager_android.cc

@@ -209,6 +209,31 @@ void SigninManagerAndroid::WipeGoogleServiceWorkerCaches(
                       weak_factory_.GetWeakPtr()));
 }
 
+void SigninManagerAndroid::RegisterPolicyWithAccount(
+    const CoreAccountInfo& account,
+    RegisterPolicyWithAccountCallback callback) {
+  if (!ShouldLoadPolicyForUser(account.email)) {
+    std::move(callback).Run(base::nullopt);
+    return;
+  }
+
+  policy::UserPolicySigninService* service =
+      policy::UserPolicySigninServiceFactory::GetForProfile(profile_);
+
+  service->RegisterForPolicyWithAccountId(
+      account.email, account.account_id,
+      base::AdaptCallbackForRepeating(base::BindOnce(
+          [](RegisterPolicyWithAccountCallback callback,
+             const std::string& dm_token, const std::string& client_id) {
+            base::Optional<ManagementCredentials> credentials;
+            if (!dm_token.empty()) {
+              credentials.emplace(dm_token, client_id);
+            }
+            std::move(callback).Run(credentials);
+          },
+          std::move(callback))));
+}
+
 void SigninManagerAndroid::RegisterAndFetchPolicyBeforeSignIn(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
@@ -220,43 +245,39 @@ void SigninManagerAndroid::RegisterAndFetchPolicyBeforeSignIn(
   // ExtractDomainName Dchecks that username is a valid email, in practice
   // this checks that @ is present and is not the last character.
   gaia::ExtractDomainName(username);
-
-  policy::UserPolicySigninService* service =
-      policy::UserPolicySigninServiceFactory::GetForProfile(profile_);
   CoreAccountInfo account =
       IdentityManagerFactory::GetForProfile(profile_)
           ->FindAccountInfoForAccountWithRefreshTokenByEmailAddress(username)
           .value();
-  service->RegisterForPolicyWithAccountId(
-      username, account.account_id,
-      base::Bind(&SigninManagerAndroid::OnPolicyRegisterDone,
-                 weak_factory_.GetWeakPtr(), account));
+
+  RegisterPolicyWithAccount(
+      account, base::BindOnce(&SigninManagerAndroid::OnPolicyRegisterDone,
+                              weak_factory_.GetWeakPtr(), account));
 }
 
-void SigninManagerAndroid::OnPolicyRegisterDone(const CoreAccountInfo& account,
-                                                const std::string& dm_token,
-                                                const std::string& client_id) {
-  if (dm_token.empty()) {
+void SigninManagerAndroid::OnPolicyRegisterDone(
+    const CoreAccountInfo& account,
+    const base::Optional<ManagementCredentials>& credentials) {
+  if (credentials) {
+    FetchPolicyBeforeSignIn(account, credentials.value());
+  } else {
     // User's account does not have a policy to fetch.
     JNIEnv* env = base::android::AttachCurrentThread();
     Java_SigninManager_onPolicyFetchedBeforeSignIn(env, java_signin_manager_);
-  } else {
-    FetchPolicyBeforeSignIn(account, dm_token, client_id);
   }
 }
 
 void SigninManagerAndroid::FetchPolicyBeforeSignIn(
     const CoreAccountInfo& account,
-    const std::string& dm_token,
-    const std::string& client_id) {
+    const ManagementCredentials& credentials) {
   policy::UserPolicySigninService* service =
       policy::UserPolicySigninServiceFactory::GetForProfile(profile_);
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory =
       content::BrowserContext::GetDefaultStoragePartition(profile_)
           ->GetURLLoaderFactoryForBrowserProcess();
   service->FetchPolicyForSignedInUser(
-      AccountIdFromAccountInfo(account), dm_token, client_id,
-      url_loader_factory,
+      AccountIdFromAccountInfo(account), credentials.dm_token,
+      credentials.client_id, url_loader_factory,
       base::Bind(&SigninManagerAndroid::OnPolicyFetchDone,
                  weak_factory_.GetWeakPtr()));
 }
@@ -335,43 +356,26 @@ static jlong JNI_SigninManager_Init(JNIEnv* env,
   return reinterpret_cast<intptr_t>(signin_manager_android);
 }
 
-static jboolean JNI_SigninManager_ShouldLoadPolicyForUser(
-    JNIEnv* env,
-    const JavaParamRef<jstring>& j_username) {
-  std::string username =
-      base::android::ConvertJavaStringToUTF8(env, j_username);
-  return ShouldLoadPolicyForUser(username);
-}
-
-static void JNI_SigninManager_IsUserManaged(
+void SigninManagerAndroid::IsUserManaged(
     JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
     const JavaParamRef<jstring>& j_username,
     const JavaParamRef<jobject>& j_callback) {
   base::android::ScopedJavaGlobalRef<jobject> callback(env, j_callback);
   std::string username =
       base::android::ConvertJavaStringToUTF8(env, j_username);
-  if (!ShouldLoadPolicyForUser(username)) {
-    base::android::RunBooleanCallbackAndroid(callback, false);
-    return;
-  }
 
-  Profile* profile = ProfileManager::GetActiveUserProfile();
-  policy::UserPolicySigninService* service =
-      policy::UserPolicySigninServiceFactory::GetForProfile(profile);
-  service->RegisterForPolicyWithAccountId(
-      username,
-      IdentityManagerFactory::GetForProfile(profile)
-          ->FindAccountInfoForAccountWithRefreshTokenByEmailAddress(username)
-          .value_or(AccountInfo{})
-          .account_id,
-      // TODO: if UserPolicySigninService::PolicyRegistrationCallback is changed
-      // to base::OnceCallback, change this to base::BindOnce; otherwise remove
-      // this comment. See https://crbug.com/948098 for more details.
-      base::BindRepeating(
+  base::Optional<CoreAccountInfo> account =
+      IdentityManagerFactory::GetForProfile(profile_)
+          ->FindAccountInfoForAccountWithRefreshTokenByEmailAddress(username);
+
+  RegisterPolicyWithAccount(
+      account.value_or(CoreAccountInfo{}),
+      base::BindOnce(
           [](base::android::ScopedJavaGlobalRef<jobject> callback,
-             const std::string& dm_token, const std::string& client_id) {
+             const base::Optional<ManagementCredentials>& credentials) {
             base::android::RunBooleanCallbackAndroid(callback,
-                                                     !dm_token.empty());
+                                                     credentials.has_value());
           },
           callback));
 }

chrome/browser/android/signin/signin_manager_android.h

@@ -79,24 +79,44 @@ class SigninManagerAndroid : public identity::IdentityManager::Observer {
   jboolean IsSignedInOnNative(JNIEnv* env,
                               const base::android::JavaParamRef<jobject>& obj);
 
+  void IsUserManaged(JNIEnv* env,
+                     const base::android::JavaParamRef<jobject>& obj,
+                     const base::android::JavaParamRef<jstring>& j_username,
+                     const base::android::JavaParamRef<jobject>& j_callback);
+
   // identity::IdentityManager::Observer implementation.
   void OnPrimaryAccountCleared(
       const CoreAccountInfo& previous_primary_account_info) override;
 
  private:
+  struct ManagementCredentials {
+    ManagementCredentials(const std::string& dm_token,
+                          const std::string& client_id)
+        : dm_token(dm_token), client_id(client_id) {}
+    const std::string dm_token;
+    const std::string client_id;
+  };
+
+  using RegisterPolicyWithAccountCallback = base::OnceCallback<void(
+      const base::Optional<ManagementCredentials>& credentials)>;
+
   friend class SigninManagerAndroidTest;
   FRIEND_TEST_ALL_PREFIXES(SigninManagerAndroidTest,
                            DeleteGoogleServiceWorkerCaches);
 
   ~SigninManagerAndroid() override;
 
-  void OnPolicyRegisterDone(const CoreAccountInfo& account_id,
-                            const std::string& dm_token,
-                            const std::string& client_id);
+  // If required registers for policy with given account. callback will be
+  // called with credentials if the account is managed.
+  void RegisterPolicyWithAccount(const CoreAccountInfo& account,
+                                 RegisterPolicyWithAccountCallback callback);
+
+  void OnPolicyRegisterDone(
+      const CoreAccountInfo& account_id,
+      const base::Optional<ManagementCredentials>& credentials);
 
   void FetchPolicyBeforeSignIn(const CoreAccountInfo& account_id,
-                               const std::string& dm_token,
-                               const std::string& client_id);
+                               const ManagementCredentials& credentials);
 
   void OnPolicyFetchDone(bool success) const;