Do not run Android management check for re-auth case.

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"
@@ skipping 358 matching lines @@
       base::MakeUnique<ArcAuthService::AccountInfoNotifier>(callback));
 }
 
 void ArcAuthService::GetAuthCodeAndAccountTypeDeprecated(
     const GetAuthCodeAndAccountTypeDeprecatedCallback& callback) {
   RequestAccountInfoInternal(
       base::MakeUnique<ArcAuthService::AccountInfoNotifier>(callback));
 }
 
 void ArcAuthService::RequestAccountInfo() {
   RequestAccountInfoInternal(

[Luis Héctor Chávez, 2016/11/14 17:06:50]

What states can this call be made in? ACTIVE and CHECKING_ANDROID_MANAGEMENT?

[hidehiko, 2016/11/14 19:07:57]

Practically, ACTIVE.
However, some browser tests calls AuthHost implementation under non-ACTIVE
state. Considering that extracting AuthHost implementation will naturally
resolve the issue, can we leave it, for now?

[Luis Héctor Chávez, 2016/11/14 19:19:25]

sgtm

       base::MakeUnique<ArcAuthService::AccountInfoNotifier>(
           base::Bind(&ArcAuthService::OnAccountInfoReady,
                      weak_ptr_factory_.GetWeakPtr())));
 }
 
 void ArcAuthService::OnAccountInfoReady(mojom::AccountInfoPtr account_info) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   auto* instance = arc_bridge_service()->auth()->GetInstanceForMethod(
       "OnAccountInfoReady", kMinVersionForOnAccountInfoReady);
   DCHECK(instance);
@@ skipping 53 matching lines @@
   return account_info_notifier_ != nullptr;
 }
 
 void ArcAuthService::PrepareContextForAuthCodeRequest() {
   // Requesting auth code on demand happens in following cases:
   // 1. To handle account password revoke.
   // 2. In case Arc is activated in OOBE flow.
   // 3. For any other state on Android side that leads device appears in
   // non-signed state.
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(state_ != State::ACTIVE || IsAuthCodeRequest());

[Luis Héctor Chávez, 2016/11/14 17:06:50]

IIUC this can be DCHECK_(state_ == State::FETCHING_CODE || IsAuthCodeRequest());

[hidehiko, 2016/11/14 19:07:57]

Added slightly more straight forward checking. How about this?

[Luis Héctor Chávez, 2016/11/14 19:19:25]

sgtm

   DCHECK(!IsArcKioskMode());
   context_->PrepareContext();
 }
 
 void ArcAuthService::OnSignInComplete() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK_EQ(state_, State::ACTIVE);
 
   if (!sign_in_time_.is_null()) {
     arc_sign_in_timer_.Stop();
@@ skipping 130 matching lines @@
   // TODO(hidehiko): Revisit to think about lazy initialization.
   support_host_.reset(new ArcSupportHost());
   support_host_->AddObserver(this);
   if (!g_disable_ui_for_testing && !IsOptInVerificationDisabled()) {
     preference_handler_ = base::MakeUnique<arc::ArcOptInPreferenceHandler>(
         this, profile_->GetPrefs());
     // This automatically updates all preferences.
     preference_handler_->Start();
   }
 
   SetState(State::STOPPED);

[Luis Héctor Chávez, 2016/11/14 17:06:50]

DCHECK_EQ(State::NOT_INITIALIZED, state_);

[hidehiko, 2016/11/14 19:07:57]

Done.

 
   PrefServiceSyncableFromProfile(profile_)->AddSyncedPrefObserver(
       prefs::kArcEnabled, this);
 
   context_.reset(new ArcAuthContext(this, profile_));
 
   // In case UI is disabled we assume that ARC is opted-in. For ARC Kiosk we
   // skip ToS because it is very likely that near the device there will be
   // no one who is eligible to accept them. We skip if Android management check
   // because there are no managed human users for Kiosk exist.
@@ skipping 73 matching lines @@
           ArcSupportHost::kHostAppId);
   CHECK(extension && extensions::util::IsAppLaunchable(
                          ArcSupportHost::kHostAppId, profile_));
   OpenApplication(CreateAppLaunchParamsUserContainer(
       profile_, extension, WindowOpenDisposition::NEW_WINDOW,
       extensions::SOURCE_CHROME_INTERNAL));
 }
 
 void ArcAuthService::OnContextReady() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  // TODO(hidehiko): The check is not necessary if this is a part of re-auth
-  // flow and OOBE OptIn where Android Management check must be a part of
-  // checking if Arc OptIn should be skip. 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()));
+  FetchAuthCode();
 }
 
 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 34 matching lines @@
   }
 
   if (state_ == State::ACTIVE)
     return;
   CloseUI();
   auth_code_.clear();
 
   if (!profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn)) {
     if (profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
       // Need pre-fetch auth code and start Arc.
       SetState(State::FETCHING_CODE);

[Luis Héctor Chávez, 2016/11/14 17:06:50]

DCHECK_EQ(State::CHECKING_ANDROID_MANAGEMENT, state_);

[hidehiko, 2016/11/14 19:07:57]

Practically, we expect STOPPED here.
However, state_ can be any value except ACTIVE or SHOWING_TERMS_OF_SERVICE,
because OnOptInPreferenceChanged() can be called at anytime...

Let's address this in ArcSessionManager CL in more precise.

       PrepareContextForAuthCodeRequest();
     } else {
       // Need pre-fetch auth code and show OptIn UI if needed.
       StartUI();
     }
   } else {
     // Ready to start Arc, but check Android management in parallel.
     StartArc();
     // Note: Because the callback may be called in synchronous way (i.e. called
     // on the same stack), StartCheck() needs to be called *after* StartArc().
@@ skipping 66 matching lines @@
 void ArcAuthService::StopAndEnableArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!arc_bridge_service()->stopped());
   reenable_arc_ = true;
   StopArc();
 }
 
 void ArcAuthService::StartArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   arc_bridge_service()->RequestStart();
   SetState(State::ACTIVE);

[Luis Héctor Chávez, 2016/11/14 17:06:50]

Can we check that state is either FETCHING_CODE or STOPPED?

[hidehiko, 2016/11/14 19:07:57]

Unfortunately, StartArc() can be called in other state_, e.g.,
CHECKING_ANDROID_MANAGEMENT (in IsOptInVerificationDisabled() == true case). Can
we leave it at the moment?

[Luis Héctor Chávez, 2016/11/14 19:19:25]

sure

 }
 
 void ArcAuthService::SetAuthCodeAndStartArc(const std::string& auth_code) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!auth_code.empty());
 
   if (IsAuthCodeRequest()) {
-    DCHECK_EQ(state_, State::FETCHING_CODE);
-    SetState(State::ACTIVE);
     account_info_notifier_->Notify(!IsOptInVerificationDisabled(), auth_code,
                                    GetAccountType(),
                                    policy_util::IsAccountManaged(profile_));
     account_info_notifier_.reset();
     return;
   }
 
-  if (state_ != State::FETCHING_CODE) {
+  if (state_ != State::SHOWING_TERMS_OF_SERVICE &&
+      state_ != State::CHECKING_ANDROID_MANAGEMENT &&
+      state_ != State::FETCHING_CODE) {
     ShutdownBridgeAndCloseUI();
     return;
   }
 
   sign_in_time_ = base::Time::Now();
   VLOG(1) << "Starting ARC for first sign in.";
 
   SetUIPage(ArcSupportHost::UIPage::START_PROGRESS, base::string16());
   ShutdownBridge();
   auth_code_ = auth_code;
   arc_sign_in_timer_.Start(FROM_HERE, kArcSignInTimeout,
                            base::Bind(&ArcAuthService::OnArcSignInTimeout,
                                       weak_ptr_factory_.GetWeakPtr()));
   StartArc();
 }
 
 void ArcAuthService::OnArcSignInTimeout() {
   LOG(ERROR) << "Timed out waiting for first sign in.";
   OnSignInFailedInternal(ProvisioningResult::OVERALL_SIGN_IN_TIMEOUT);
 }
 
-void ArcAuthService::StartLso() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  // Terms were accepted
-  profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
-
-  // Update UMA only if error (with or without feedback) is currently shown.
-  if (ui_page_ == ArcSupportHost::UIPage::ERROR) {
-    UpdateOptInActionUMA(OptInActionType::RETRY);
-  } else if (ui_page_ == ArcSupportHost::UIPage::ERROR_WITH_FEEDBACK) {
-    UpdateOptInActionUMA(OptInActionType::RETRY);
-    ShutdownBridge();
-  }
-
-  DCHECK(arc_bridge_service()->stopped());
-  SetState(State::FETCHING_CODE);
-  PrepareContextForAuthCodeRequest();
-}
-
 void ArcAuthService::CancelAuthCode() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   if (state_ == State::NOT_INITIALIZED) {
     NOTREACHED();
     return;
   }
 
   // In case |state_| is ACTIVE, |ui_page_| can be START_PROGRESS (which means
   // normal Arc booting) or  ERROR or ERROR_WITH_FEEDBACK (in case Arc can not
   // be started). If Arc is booting normally dont't stop it on progress close.
-  if (state_ != State::FETCHING_CODE &&
+  if ((state_ != State::FETCHING_CODE &&
+       state_ != State::SHOWING_TERMS_OF_SERVICE &&
+       state_ != State::CHECKING_ANDROID_MANAGEMENT) &&
       ui_page_ != ArcSupportHost::UIPage::ERROR &&
       ui_page_ != ArcSupportHost::UIPage::ERROR_WITH_FEEDBACK) {
     return;
   }
 
   // Update UMA with user cancel only if error is not currently shown.
   if (ui_page_ != ArcSupportHost::UIPage::NO_PAGE &&
       ui_page_ != ArcSupportHost::UIPage::ERROR &&
       ui_page_ != ArcSupportHost::UIPage::ERROR_WITH_FEEDBACK) {
     UpdateOptInCancelUMA(OptInCancelReason::USER_CANCEL);
@@ skipping 53 matching lines @@
 
   if (!arc_bridge_service()->stopped()) {
     // If the user attempts to re-enable ARC while the bridge is still running
     // the user should not be able to continue until the bridge has stopped.
     ShowUI(
         ArcSupportHost::UIPage::ERROR,
         l10n_util::GetStringUTF16(IDS_ARC_SIGN_IN_SERVICE_UNAVAILABLE_ERROR));
     return;
   }
 
-  SetState(State::FETCHING_CODE);
+  SetState(State::SHOWING_TERMS_OF_SERVICE);

[Luis Héctor Chávez, 2016/11/14 17:06:50]

DCHECK_EQ(State::STOPPED, state_);

[hidehiko, 2016/11/14 19:07:57]

Practically, we expect STOPPED, but technically this can be in other values,
e.g. CHECKING_ANDROID_MANAGEMENT or FETCHING_CODE. (kind of race issue...) Can
we address this later?

[Luis Héctor Chávez, 2016/11/14 19:19:25]

sure

   ShowUI(ArcSupportHost::UIPage::TERMS, base::string16());
 }
 
 void ArcAuthService::OnPrepareContextFailed() {
   DCHECK_EQ(state_, State::FETCHING_CODE);
 
   ShutdownBridgeAndShowUI(
       ArcSupportHost::UIPage::ERROR,
       l10n_util::GetStringUTF16(IDS_ARC_SERVER_COMMUNICATION_ERROR));
   UpdateOptInCancelUMA(OptInCancelReason::NETWORK_ERROR);
 }
 
 void ArcAuthService::OnAuthCodeSuccess(const std::string& auth_code) {
   SetAuthCodeAndStartArc(auth_code);
 }
 
 void ArcAuthService::OnAuthCodeFailed() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK_EQ(state_, State::FETCHING_CODE);
   ShutdownBridgeAndShowUI(
       ArcSupportHost::UIPage::ERROR,
       l10n_util::GetStringUTF16(IDS_ARC_SERVER_COMMUNICATION_ERROR));
   UpdateOptInCancelUMA(OptInCancelReason::NETWORK_ERROR);
 }
 
+void ArcAuthService::StartArcAndroidManagementCheck() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(arc_bridge_service()->stopped());
+  SetState(State::CHECKING_ANDROID_MANAGEMENT);

[Luis Héctor Chávez, 2016/11/14 17:06:50]

DCHECK_EQ(State::SHOWING_TERMS_OF_SERVICE, state_);

Since you can also call this function while it is already in the
CHECKING_ANDROID_MANAGEMENT state, maybe you can move this DCHECK_EQ/SetState
combo to L1091?

[hidehiko, 2016/11/14 19:07:57]

Added DCHECK.

I know the state machine handling in this class is complicated. However, it is
too complicated to handle it within only this CL, and I'd like to focus on
fixing check-android-management phase in this CL. So, can we revisit here in a
following CL?

+
+  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::OnAndroidManagementChecked(
     policy::AndroidManagementClient::Result result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK_EQ(state_, State::CHECKING_ANDROID_MANAGEMENT);
+
   switch (result) {
     case policy::AndroidManagementClient::Result::UNMANAGED:
-      OnAndroidManagementPassed();
+      if (IsOptInVerificationDisabled()) {
+        StartArc();
+      } else {
+        // TODO(hidehiko): Merge this prefetching into re-auth case.
+        SetState(State::FETCHING_CODE);
+        PrepareContextForAuthCodeRequest();
+      }
       break;
     case policy::AndroidManagementClient::Result::MANAGED:
       ShutdownBridgeAndShowUI(
           ArcSupportHost::UIPage::ERROR,
           l10n_util::GetStringUTF16(IDS_ARC_ANDROID_MANAGEMENT_REQUIRED_ERROR));
       UpdateOptInCancelUMA(OptInCancelReason::ANDROID_MANAGEMENT_REQUIRED);
       break;
     case policy::AndroidManagementClient::Result::ERROR:
       ShutdownBridgeAndShowUI(
           ArcSupportHost::UIPage::ERROR,
@@ skipping 32 matching lines @@
   }
 
   if (!auth_endpoint.empty()) {
     auth_code_fetcher_.reset(new ArcAuthCodeFetcher(
         this, context_->GetURLRequestContext(), profile_, auth_endpoint));
   } else {
     ShowUI(ArcSupportHost::UIPage::LSO_PROGRESS, base::string16());
   }
 }
 
-void ArcAuthService::OnAndroidManagementPassed() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  if (state_ == State::ACTIVE) {
-    if (IsAuthCodeRequest())
-      FetchAuthCode();
-    return;
-  }
-
-  if (profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn) ||
-      IsOptInVerificationDisabled()) {
-    StartArc();
-  } else {
-    FetchAuthCode();
-  }
-}
-
 void ArcAuthService::OnWindowClosed() {
   CancelAuthCode();
 }
 
 void ArcAuthService::OnTermsAgreed(bool is_metrics_enabled,
                                    bool is_backup_and_restore_enabled,
                                    bool is_location_service_enabled) {
+  // Terms were accepted
+  profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
+
   // This is ARC support's UI event callback, so this is called only when
   // the UI is visible. The condition to open the UI is
   // !g_disable_ui_for_testing && !IsOptInVerificationDisabled() (see ShowUI())
   // and in the case, preference_handler_ should be always created (see
   // OnPrimaryUserProfilePrepared()),
   // TODO(hidehiko): Simplify the logic with the code restructuring.
   DCHECK(preference_handler_);
   preference_handler_->EnableMetrics(is_metrics_enabled);
   preference_handler_->EnableBackupRestore(is_backup_and_restore_enabled);
   preference_handler_->EnableLocationService(is_location_service_enabled);
-  StartLso();
+  StartArcAndroidManagementCheck();
 }
 
 void ArcAuthService::OnAuthSucceeded(const std::string& auth_code) {
   SetAuthCodeAndStartArc(auth_code);
 }
 
+void ArcAuthService::OnRetryClicked() {
+  UpdateOptInActionUMA(OptInActionType::RETRY);
+  // ERROR_WITH_FEEDBACK is set in OnSignInFailed(). In the case, we postpone
+  // to stop the ARC to obtain the internal state.
+  // Here, on retry, stop it.
+  if (ui_page_ == ArcSupportHost::UIPage::ERROR_WITH_FEEDBACK)
+    ShutdownBridge();
+
+  switch (state_) {
+    case State::NOT_INITIALIZED:
+      NOTREACHED();
+      break;
+    case State::STOPPED:
+    case State::SHOWING_TERMS_OF_SERVICE:
+      ShowUI(ArcSupportHost::UIPage::TERMS, base::string16());
+      break;
+    case State::CHECKING_ANDROID_MANAGEMENT:
+      StartArcAndroidManagementCheck();
+      break;
+    case State::FETCHING_CODE:
+    case State::ACTIVE:

[Luis Héctor Chávez, 2016/11/14 17:06:50]

If State::ACTIVE, you also need to check if there is an ongoing request for an
auth code.

[hidehiko, 2016/11/14 19:07:57]

Done in PrepareContextForAuthCodeRequest().

+      PrepareContextForAuthCodeRequest();
+      break;
+  }
+}
+
 void ArcAuthService::OnSendFeedbackClicked() {
   chrome::OpenFeedbackDialog(nullptr);
 }
 
 void ArcAuthService::OnMetricsModeChanged(bool enabled, bool managed) {
   if (!support_host_)
     return;
   support_host_->SetMetricsPreferenceCheckbox(enabled, managed);
 }
 
 void ArcAuthService::OnBackupAndRestoreModeChanged(bool enabled, bool managed) {
   if (!support_host_)
     return;
   support_host_->SetBackupAndRestorePreferenceCheckbox(enabled, managed);
 }
 
 void ArcAuthService::OnLocationServicesModeChanged(bool enabled, bool managed) {
   if (!support_host_)
     return;
   support_host_->SetLocationServicesPreferenceCheckbox(enabled, managed);
 }
 
 std::ostream& operator<<(std::ostream& os, const ArcAuthService::State& state) {
   switch (state) {
     case ArcAuthService::State::NOT_INITIALIZED:
       return os << "NOT_INITIALIZED";
     case ArcAuthService::State::STOPPED:
       return os << "STOPPED";
+    case ArcAuthService::State::SHOWING_TERMS_OF_SERVICE:
+      return os << "SHOWING_TERMS_OF_SERVICE";
+    case ArcAuthService::State::CHECKING_ANDROID_MANAGEMENT:
+      return os << "CHECKING_ANDROID_MANAGEMENT";
     case ArcAuthService::State::FETCHING_CODE:
       return os << "FETCHING_CODE";
     case ArcAuthService::State::ACTIVE:
       return os << "ACTIVE";
     default:
       NOTREACHED();
       return os;
   }
 }
 
 }  // namespace arc