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 377 matching lines @@
   }
 
   account_info_notifier_ = std::move(account_info_notifier);
   PrepareContextForAuthCodeRequest();
 }
 
 bool ArcAuthService::IsAuthCodeRequest() const {
   return account_info_notifier_ != nullptr;
 }
 
 void ArcAuthService::PrepareContextForAuthCodeRequest() {

[Luis Héctor Chávez, 2016/11/10 23:30:58]

Is it possible to split this method so we have tighter guarantees about what
state is it when this is called? That way we can eliminate the pattern of

SetState(...);
PrepareContextforAuthCodeRequest();

and instead try to enforce the pattern

SetState(...);
PerformAsyncOperation(); // or reach a steady state.

[hidehiko, 2016/11/14 10:55:09]

Hmm... Maybe I do not understand what you mean.

Do you mean "please define different functions for auth-token prefetching and
re-auth flow", so that SetState(...) can be forced to be called always?

Then, can we skip it? I know this is NOT a good code. But;
- The prefetching code will be migrated into re-auth in a following CL.
- This is existing code, so it is not the regression.
- Splitting the code path into two looks almost just duplicating the code.

WDYT?

If I'm wrong, please fix me.

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

If this will be addressed in a following CL, then I'm fine with it. The goal
should be to have more clear and understandable transitions between states.

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

I will. However, TBH, I do not think it is reasonable to address the problem in
this CL due to its own current complexity, unfortunately.
I'll keep in my mind about your comment, and will make it happen later.

   // 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());
   context_->PrepareContext();
 }
 
@@ skipping 219 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 129 matching lines @@
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   arc_bridge_service()->RequestStart();
   SetState(State::ACTIVE);
 }
 
 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);

[hidehiko, 2016/11/10 01:46:23]

here, state_ should be State::ACTIVE in most cases, but may be not (race
issue... It needs to be addressed later.).

-    SetState(State::ACTIVE);
     account_info_notifier_->Notify(!IsOptInVerificationDisabled(), auth_code,
                                    mojom::ChromeAccountType::USER_ACCOUNT,
                                    policy_util::IsAccountManaged(profile_));
     account_info_notifier_.reset();
     return;
   }
 
-  if (state_ != State::FETCHING_CODE) {
+  if (state_ != State::TERMS && state_ != State::ANDROID_MANAGEMENT_CHECK &&
+      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);

[hidehiko, 2016/11/10 01:46:23]

Note: Moved to OnTermsAccepted(). Actually, this is not a part of Lso starting.

-
-  // 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);
+  SetState(State::ANDROID_MANAGEMENT_CHECK);

[xiyuan, 2016/11/10 17:55:21]

Why ANDROID_MANAGEMENT_CHECK instead of FETCHING_CODE?

[hidehiko, 2016/11/14 10:55:09]

Sorry this was my mistake. We no longer need StartLso(). Removed.

   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::TERMS &&
+       state_ != State::ANDROID_MANAGEMENT_CHECK) &&
       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::ERROR_WITH_FEEDBACK)
     UpdateOptInCancelUMA(OptInCancelReason::USER_CANCEL);
 
   StopArc();
@@ skipping 50 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::TERMS);
   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::ANDROID_MANAGEMENT_CHECK);

[Luis Héctor Chávez, 2016/11/10 23:30:58]

Is it possible to have a DCHECK_EQ(state_, ...) every time SetState is called?
That way the transitions are more explicit.

[hidehiko, 2016/11/14 10:55:09]

WDY mean?
SetState(...) is just "state_ = ...". So;

SetState(CHECKING_ANDROID_MANAGEMENT);
DCHECK_EQ(CHECKING_ANDROID_MANAGEMENT, state_);

looks redundant to me?

Note: let me keep SetState() for now. It will be gone when we introduce
ArcSessionManager.

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

Sorry, I meant before assigning. In other words, making sure the transition is
happening from an expected state (if possible avoid being in an expected set of
states to simplify logic and make it more understandable).

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

I got your point.
However, currently, the state transition is un-manageable, IMHO...
Many methods are called without taking care of its state already.
Even just adding DCHECK is sometimes difficult or more complex than simple
thought, and it looks not practical to me at the moment.

In following CLs, I'll give it a try to have clearer state machine...

+
+  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::ANDROID_MANAGEMENT_CHECK);
+
   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) {

[hidehiko, 2016/11/10 01:46:23]

Because AndroidManagement check is no longer running for re-auth flow, so this
is gone.

-    if (IsAuthCodeRequest())
-      FetchAuthCode();
-    return;
-  }
-
-  if (profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn) ||

[hidehiko, 2016/11/10 01:46:23]

This is migrated into OnAndroidManagementChecked().

-      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::TERMS:
+      ShowUI(ArcSupportHost::UIPage::TERMS, base::string16());
+      break;
+    case State::ANDROID_MANAGEMENT_CHECK:
+      StartArcAndroidManagementCheck();
+      break;
+    case State::FETCHING_CODE:
+    case State::ACTIVE:
+      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::TERMS:
+      return os << "TERMS";
+    case ArcAuthService::State::ANDROID_MANAGEMENT_CHECK:
+      return os << "ANDROID_MANAGEMENT_CHECK";
     case ArcAuthService::State::FETCHING_CODE:
       return os << "FETCHING_CODE";
     case ArcAuthService::State::ACTIVE:
       return os << "ACTIVE";
     default:

[Luis Héctor Chávez, 2016/11/10 23:30:58]

nit: remove the default: case and move the NOTREACHED(); return os;  outside the
switch so the compiler yells at us if this is not exhaustive.

[hidehiko, 2016/11/14 10:55:09]

Addressed in a separate CL.
https://codereview.chromium.org/2495273002/

       NOTREACHED();
       return os;
   }
 }
 
 }  // namespace arc