Refactor ArcAndroidManagementChecker.

chrome/browser/chromeos/arc/arc_auth_service.cc

 // Copyright 2015 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.
 
 #include "chrome/browser/chromeos/arc/arc_auth_service.h"
 
 #include <utility>
 
 #include "ash/common/shelf/shelf_delegate.h"
 #include "ash/common/wm_shell.h"
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/arc/arc_android_management_checker.h"
 #include "chrome/browser/chromeos/arc/arc_auth_code_fetcher.h"
 #include "chrome/browser/chromeos/arc/arc_auth_context.h"
 #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"
 #include "chrome/browser/policy/profile_policy_connector_factory.h"
 #include "chrome/browser/prefs/pref_service_syncable_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_launcher.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_util.h"
 #include "chrome/browser/ui/extensions/app_launch_params.h"
 #include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
 #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"
 #include "components/user_manager/user.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/app_window/app_window_registry.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "ui/base/l10n/l10n_util.h"
 
@@ skipping 12 matching lines @@
 
 // The Android management check is disabled by default, it's used only for
 // testing.
 bool g_enable_check_android_management_for_testing = false;
 
 // Maximum amount of time we'll wait for ARC to finish booting up. Once this
 // timeout expires, keep ARC running in case the user wants to file feedback,
 // but present the UI to try again.
 constexpr base::TimeDelta kArcSignInTimeout = base::TimeDelta::FromMinutes(5);
 
 const char kStateNotInitialized[] = "NOT_INITIALIZED";

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

can you make these constexpr while you're at it?

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

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

 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);
 }
 
 ash::ShelfDelegate* GetShelfDelegate() {
   if (g_shelf_delegate_for_testing)
     return g_shelf_delegate_for_testing;
   if (ash::WmShell::HasInstance()) {
     DCHECK(ash::WmShell::Get()->shelf_delegate());
     return ash::WmShell::Get()->shelf_delegate();
@@ skipping 266 matching lines @@
 
   if (!IsOptInVerificationDisabled() &&
       !profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn)) {
     playstore_launcher_.reset(
         new ArcAppLauncher(profile_, kPlayStoreAppId, true));
   }
 
   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();
 }
 
 void ArcAuthService::OnSignInFailed(mojom::ArcSignInFailureReason reason) {
   OnSignInFailedInternal(
       ConvertArcSignInFailureReasonToProvisioningResult(reason));
 }
 
 void ArcAuthService::OnSignInFailedInternal(ProvisioningResult result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK_EQ(state_, State::ACTIVE);
   DCHECK(!sign_in_time_.is_null());
 
   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) {
     case ProvisioningResult::GMS_NETWORK_ERROR:
       error_message_id = IDS_ARC_SIGN_IN_NETWORK_ERROR;
       break;
     case ProvisioningResult::GMS_SERVICE_UNAVAILABLE:
     case ProvisioningResult::GMS_SIGN_IN_FAILED:
     case ProvisioningResult::GMS_SIGN_IN_TIMEOUT:
     case ProvisioningResult::GMS_SIGN_IN_INTERNAL_ERROR:
@@ skipping 39 matching lines @@
   // We'll delay shutting down the bridge in this case to allow people to send
   // feedback.
   ShowUI(UIPage::ERROR_WITH_FEEDBACK,
          l10n_util::GetStringUTF16(error_message_id));
 }
 
 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) {
   if (state_ == state)
     return;
 
   state_ = state;
   for (auto& observer : observer_list_)
     observer.OnOptInChanged(state_);
 }
 
 bool ArcAuthService::IsAllowed() const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   return profile_ != nullptr;
 }
 
 void ArcAuthService::OnPrimaryUserProfilePrepared(Profile* profile) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(profile && profile != profile_);
 
   Shutdown();
 
   if (!IsAllowedForProfile(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;
   }
 
   profile_ = profile;
   SetState(State::STOPPED);
 
   PrefServiceSyncableFromProfile(profile_)->AddSyncedPrefObserver(
       prefs::kArcEnabled, this);
 
@@ skipping 76 matching lines @@
 
   OpenApplication(CreateAppLaunchParamsUserContainer(
       profile_, extension, WindowOpenDisposition::NEW_WINDOW,
       extensions::SOURCE_CHROME_INTERNAL));
 }
 
 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,
                                          bool from_sync) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // Update UMA only for local changes
   if (!from_sync) {
     const bool arc_enabled =
         profile_->GetPrefs()->GetBoolean(prefs::kArcEnabled);
@@ skipping 36 matching lines @@
   if (state_ == State::ACTIVE)
     return;
   CloseUI();
   auth_code_.clear();
 
   if (!profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn)) {
     // Need pre-fetch auth code and show OptIn UI if needed.
     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()));
     }
   }
 
   UpdateEnabledStateUMA(true);
 }
 
 void ArcAuthService::ShutdownBridge() {
   arc_sign_in_timer_.Stop();
   playstore_launcher_.reset();
   auth_callback_.Reset();
@@ skipping 217 matching lines @@
 
 void ArcAuthService::OnAuthCodeFailed() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK_EQ(state_, State::FETCHING_CODE);
   ShutdownBridgeAndShowUI(
       UIPage::ERROR,
       l10n_util::GetStringUTF16(IDS_ARC_SERVER_COMMUNICATION_ERROR));
   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));
       UpdateOptInCancelUMA(OptInCancelReason::ANDROID_MANAGEMENT_REQUIRED);
       break;
     case policy::AndroidManagementClient::Result::RESULT_ERROR:
       ShutdownBridgeAndShowUI(
           UIPage::ERROR,
           l10n_util::GetStringUTF16(IDS_ARC_SERVER_COMMUNICATION_ERROR));
       UpdateOptInCancelUMA(OptInCancelReason::NETWORK_ERROR);
       break;
     default:
       NOTREACHED();
   }
 }
 
+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);
 
   const base::CommandLine* command_line =
       base::CommandLine::ForCurrentProcess();
   std::string auth_endpoint;
   if (command_line->HasSwitch(chromeos::switches::kArcUseAuthEndpoint)) {
     auth_endpoint = command_line->GetSwitchValueASCII(
         chromeos::switches::kArcUseAuthEndpoint);
   }
@@ skipping 33 matching lines @@
       return os << kStateFetchingCode;
     case ArcAuthService::State::ACTIVE:
       return os << kStateActive;
     default:
       NOTREACHED();
       return os;
   }
 }
 
 }  // namespace arc