Refactor ArcAndroidManagementChecker.

chrome/browser/chromeos/arc/arc_auth_service.cc

Index: chrome/browser/chromeos/arc/arc_auth_service.cc
diff --git a/chrome/browser/chromeos/arc/arc_auth_service.cc b/chrome/browser/chromeos/arc/arc_auth_service.cc
index 00f012152ec36e34680d35f90d32f3609b5b4f34..b914d3ea788065fae51540dde87e2f02e875d9a4 100644
--- a/chrome/browser/chromeos/arc/arc_auth_service.cc
+++ b/chrome/browser/chromeos/arc/arc_auth_service.cc
@@ -20,6 +20,7 @@
 #include "chrome/browser/chromeos/arc/arc_auth_notification.h"
 #include "chrome/browser/chromeos/arc/arc_optin_uma.h"
 #include "chrome/browser/chromeos/arc/arc_support_host.h"
+#include "chrome/browser/chromeos/arc/policy/arc_policy_util.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/policy/profile_policy_connector.h"
@@ -38,7 +39,6 @@
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/session_manager_client.h"
 #include "components/arc/arc_bridge_service.h"
-#include "components/policy/core/browser/browser_policy_connector.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/syncable_prefs/pref_service_syncable.h"
@@ -76,11 +76,6 @@ const char kStateStopped[] = "STOPPED";
 const char kStateFetchingCode[] = "FETCHING_CODE";
 const char kStateActive[] = "ACTIVE";
 
-bool IsAccountManaged(Profile* profile) {
-  return policy::ProfilePolicyConnectorFactory::GetForBrowserContext(profile)
-      ->IsManaged();
-}
-
 bool IsArcDisabledForEnterprise() {

[Luis Héctor Chávez, 2016/10/24 22:39:57]

Can we also move this to arc_policy_util.h?

[hidehiko, 2016/10/25 07:22:42]

Addressed in https://codereview.chromium.org/2448013002/

   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       chromeos::switches::kEnterpriseDisableArc);
@@ -368,9 +363,9 @@ void ArcAuthService::OnSignInComplete() {
   profile_->GetPrefs()->SetBoolean(prefs::kArcSignedIn, true);
   CloseUI();
   UpdateProvisioningTiming(base::Time::Now() - sign_in_time_, true,
-                           IsAccountManaged(profile_));
+                           policy_util::IsAccountManaged(profile_));
   UpdateProvisioningResultUMA(ProvisioningResult::SUCCESS,
-                              IsAccountManaged(profile_));
+                              policy_util::IsAccountManaged(profile_));
 
   for (auto& observer : observer_list_)
     observer.OnInitialStart();
@@ -389,9 +384,9 @@ void ArcAuthService::OnSignInFailedInternal(ProvisioningResult result) {
   arc_sign_in_timer_.Stop();
 
   UpdateProvisioningTiming(base::Time::Now() - sign_in_time_, false,
-                           IsAccountManaged(profile_));
+                           policy_util::IsAccountManaged(profile_));
   UpdateOptInCancelUMA(OptInCancelReason::CLOUD_PROVISION_FLOW_FAIL);
-  UpdateProvisioningResultUMA(result, IsAccountManaged(profile_));
+  UpdateProvisioningResultUMA(result, policy_util::IsAccountManaged(profile_));
 
   int error_message_id;
   switch (result) {
@@ -451,7 +446,7 @@ void ArcAuthService::GetIsAccountManaged(
     const GetIsAccountManagedCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  callback.Run(IsAccountManaged(profile_));
+  callback.Run(policy_util::IsAccountManaged(profile_));
 }
 
 void ArcAuthService::SetState(State state) {
@@ -478,7 +473,7 @@ void ArcAuthService::OnPrimaryUserProfilePrepared(Profile* profile) {
     return;
 
   // TODO(khmel): Move this to IsAllowedForProfile.
-  if (IsArcDisabledForEnterprise() && IsAccountManaged(profile)) {
+  if (IsArcDisabledForEnterprise() && policy_util::IsAccountManaged(profile)) {
     VLOG(2) << "Enterprise users are not supported in ARC.";
     return;
   }
@@ -575,7 +570,15 @@ void ArcAuthService::OnContextReady() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   DCHECK(!initial_opt_in_);
-  CheckAndroidManagement(false);
+
+  // TODO(hidehiko): The check is not necessary if this is a part of re-auth
+  // flow. Remove this.
+  android_management_checker_.reset(new ArcAndroidManagementChecker(
+      profile_, context_->token_service(), context_->account_id(),
+      false /* retry_on_error */));
+  android_management_checker_->StartCheck(
+      base::Bind(&ArcAuthService::OnAndroidManagementChecked,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ArcAuthService::OnSyncedPrefChanged(const std::string& path,
@@ -632,12 +635,20 @@ void ArcAuthService::OnOptInPreferenceChanged() {
     initial_opt_in_ = true;
     StartUI();
   } else {
-    // Ready to start Arc, but check Android management first.
+    // Ready to start Arc, but check Android management in parallel.
+    StartArc();

[Luis Héctor Chávez, 2016/10/24 22:36:02]

IIUC you are not calling FetchAuthCode(); in this flow, whereas it was called
before (unless IsOptInVerificationDisabled()).

[hidehiko, 2016/10/25 07:22:42]

FetchAuthCode was not called even in the original code.

Here, kArcSignedIn pref is *true*, so if-stmt in OnAndroidManagementPassed()
runs its then-body always in this case, which is StartArc().

[Luis Héctor Chávez, 2016/10/25 17:11:39]

Oh, right (good thing we're cleaning up this code).

+    // Note: Because the callback may be called in synchronous way (i.e. called
+    // on the same stack), StartCheck() needs to be called *after* StartArc().
+    // Otherwise, DisableArc() which may be called in
+    // OnBackgroundAndroidManagementChecked() could be ignored.
     if (!g_disable_ui_for_testing ||
         g_enable_check_android_management_for_testing) {
-      CheckAndroidManagement(true);
-    } else {
-      StartArc();
+      android_management_checker_.reset(new ArcAndroidManagementChecker(
+          profile_, context_->token_service(), context_->account_id(),
+          true /* retry_on_error */));
+      android_management_checker_->StartCheck(

[Luis Héctor Chávez, 2016/10/24 22:36:02]

Seems like you need some state to track whether there is one pending
StartCheck() call in between disabling/reenabling of ARC (or document clearly
how that scenario is impossible).

[hidehiko, 2016/10/25 07:22:42]

Inflight check is canceled by reset above and/or (probably) ShutdownBridge().
Added the explicit comment about the cancel in arc_android_management_checker.h.

+          base::Bind(&ArcAuthService::OnBackgroundAndroidManagementChecked,
+                     weak_ptr_factory_.GetWeakPtr()));
     }
   }
 
@@ -875,38 +886,14 @@ void ArcAuthService::OnAuthCodeFailed() {
   UpdateOptInCancelUMA(OptInCancelReason::NETWORK_ERROR);
 }
 
-void ArcAuthService::CheckAndroidManagement(bool background_mode) {
-  // Do not send requests for Chrome OS managed users.
-  if (IsAccountManaged(profile_)) {
-    OnAndroidManagementPassed();
-    return;
-  }
-
-  // Do not send requests for well-known consumer domains.
-  if (policy::BrowserPolicyConnector::IsNonEnterpriseUser(
-          profile_->GetProfileUserName())) {
-    OnAndroidManagementPassed();
-    return;
-  }
-
-  android_management_checker_.reset(
-      new ArcAndroidManagementChecker(this, context_->token_service(),
-                                      context_->account_id(), background_mode));
-  if (background_mode)
-    OnAndroidManagementPassed();
-}
-
 void ArcAuthService::OnAndroidManagementChecked(
     policy::AndroidManagementClient::Result result) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   switch (result) {
     case policy::AndroidManagementClient::Result::RESULT_UNMANAGED:
       OnAndroidManagementPassed();
       break;
     case policy::AndroidManagementClient::Result::RESULT_MANAGED:
-      if (android_management_checker_->background_mode()) {
-        DisableArc();
-        return;
-      }
       ShutdownBridgeAndShowUI(
           UIPage::ERROR,
           l10n_util::GetStringUTF16(IDS_ARC_ANDROID_MANAGEMENT_REQUIRED_ERROR));
@@ -923,6 +910,23 @@ void ArcAuthService::OnAndroidManagementChecked(
   }
 }
 
+void ArcAuthService::OnBackgroundAndroidManagementChecked(
+    policy::AndroidManagementClient::Result result) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  switch (result) {
+    case policy::AndroidManagementClient::Result::RESULT_UNMANAGED:
+      // Do nothing. The ARC should be started already.

[Luis Héctor Chávez, 2016/10/24 22:36:02]

nit: s/The ARC/ARC/.

[hidehiko, 2016/10/25 07:22:42]

Done.

+      break;
+    case policy::AndroidManagementClient::Result::RESULT_MANAGED:
+      DisableArc();

[Luis Héctor Chávez, 2016/10/24 22:36:03]

Is there a way to intentionally delay the receipt of this message (e.g. by
selectively blocking certain network connections) such that after your change it
is possible to interact with ARC in a managed environment whereas it was not
possible before? I think there might be. If there aren't, please clearly
document the reason and/or enforce it explicitly in some way. Otherwise, we
might have to return to the previous structure.

[hidehiko, 2016/10/25 07:22:42]

Maybe, I'm not understanding the goal to delay the message.
Could you tell me more details about your concern here?

As for network error, it is handled in ArcAndroidManagementChecker as is (=
retry on network error). Is it what you're looking for?

[Luis Héctor Chávez, 2016/10/25 17:11:39]

The concern is about a malicious actor that wants to work around management
restrictions. It seems that constantly "causing" network errors will result in a
semi-permanent state where ARC is running and might* be interacted with.

* Users _shouldn't_ be able to interact with ARC at that point, but there might
be other bugs we are not aware of and should be a bit more defensive here.

[Luis Héctor Chávez, 2016/10/25 17:47:58]

As discussed offline, since this is *not* a regression and to keep making
progress on this much-needed cleanup, this will be addressed separately.

[hidehiko, 2016/10/26 12:46:05]

Acknowledged. Let's discuss separately.

+      break;
+    case policy::AndroidManagementClient::Result::RESULT_ERROR:
+      // This code should not be reached. For background check,
+      // retry_on_error sould be set.

[Luis Héctor Chávez, 2016/10/24 22:36:03]

nit: s/sould/should/

[hidehiko, 2016/10/25 07:22:42]

Done.

+      NOTREACHED();
+  }
+}
+
 void ArcAuthService::FetchAuthCode() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);