Migrate opt-in auth flow to re-auth flow.

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 e18b9dab2881c3c140956ddfba74e83fc4cb2122..1a8aabf50985c78b489c7420c1200c65363d8d20 100644
--- a/chrome/browser/chromeos/arc/arc_auth_service.cc
+++ b/chrome/browser/chromeos/arc/arc_auth_service.cc
@@ -343,21 +343,11 @@ void ArcAuthService::OnArcDataRemoved(bool success) {
   EnableArc();
 }
 
-std::string ArcAuthService::GetAndResetAuthCode() {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  std::string auth_code;
-  auth_code_.swap(auth_code);
-  return auth_code;
-}
-
 void ArcAuthService::GetAuthCodeDeprecated0(
     const GetAuthCodeDeprecated0Callback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(!IsOptInVerificationDisabled());
-  // For robot account we must use RequestAccountInfo because it allows to
-  // specify account type.
-  DCHECK(!IsArcKioskMode());
-  callback.Run(GetAndResetAuthCode());
+  NOTREACHED() << "GetAuthCodeDeprecated0() should no longer be callable";
+  callback.Run(std::string());
 }
 
 void ArcAuthService::GetAuthCodeDeprecated(
@@ -397,10 +387,9 @@ void ArcAuthService::RequestAccountInfoInternal(
   // No other auth code-related operation may be in progress.
   DCHECK(!account_info_notifier_);
 
-  const std::string auth_code = GetAndResetAuthCode();
-  const bool is_enforced = !IsOptInVerificationDisabled();
-  if (!auth_code.empty() || !is_enforced) {
-    account_info_notifier->Notify(is_enforced, auth_code, GetAccountType(),
+  if (IsOptInVerificationDisabled()) {
+    account_info_notifier->Notify(false /* = is_enforced */, std::string(),
+                                  GetAccountType(),
                                   policy_util::IsAccountManaged(profile_));
     return;
   }
@@ -613,7 +602,6 @@ void ArcAuthService::OnPrimaryUserProfilePrepared(Profile* profile) {
   // no one who is eligible to accept them. We skip if Android management check
   // because there are no managed human users for Kiosk exist.
   if (IsOptInVerificationDisabled() || IsArcKioskMode()) {
-    auth_code_.clear();
     StartArc();
     return;
   }
@@ -745,13 +733,10 @@ void ArcAuthService::OnOptInPreferenceChanged() {
   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);
-      PrepareContextForAuthCodeRequest();
+      StartArc();
     } else {
       // Need pre-fetch auth code and show OptIn UI if needed.
       StartUI();
@@ -840,35 +825,14 @@ void ArcAuthService::StartArc() {
   SetState(State::ACTIVE);
 }
 
-void ArcAuthService::SetAuthCodeAndStartArc(const std::string& auth_code) {
+void ArcAuthService::OnAuthCodeObtained(const std::string& auth_code) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!auth_code.empty());
 
-  if (IsAuthCodeRequest()) {
-    account_info_notifier_->Notify(!IsOptInVerificationDisabled(), auth_code,
-                                   GetAccountType(),
-                                   policy_util::IsAccountManaged(profile_));
-    account_info_notifier_.reset();
-    return;
-  }
-
-  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();
+  account_info_notifier_->Notify(!IsOptInVerificationDisabled(), auth_code,
+                                 GetAccountType(),
+                                 policy_util::IsAccountManaged(profile_));
+  account_info_notifier_.reset();
 }
 
 void ArcAuthService::OnArcSignInTimeout() {
@@ -887,8 +851,7 @@ void ArcAuthService::CancelAuthCode() {
   // 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 &&
-       state_ != State::SHOWING_TERMS_OF_SERVICE &&
+  if ((state_ != State::SHOWING_TERMS_OF_SERVICE &&
        state_ != State::CHECKING_ANDROID_MANAGEMENT) &&
       ui_page_ != ArcSupportHost::UIPage::ERROR &&
       ui_page_ != ArcSupportHost::UIPage::ERROR_WITH_FEEDBACK) {
@@ -968,8 +931,6 @@ void ArcAuthService::StartUI() {
 }
 
 void ArcAuthService::OnPrepareContextFailed() {
-  DCHECK_EQ(state_, State::FETCHING_CODE);
-
   ShutdownBridgeAndShowUI(
       ArcSupportHost::UIPage::ERROR,
       l10n_util::GetStringUTF16(IDS_ARC_SERVER_COMMUNICATION_ERROR));
@@ -977,12 +938,11 @@ void ArcAuthService::OnPrepareContextFailed() {
 }
 
 void ArcAuthService::OnAuthCodeSuccess(const std::string& auth_code) {
-  SetAuthCodeAndStartArc(auth_code);
+  OnAuthCodeObtained(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));
@@ -1009,13 +969,12 @@ void ArcAuthService::OnAndroidManagementChecked(
 
   switch (result) {
     case policy::AndroidManagementClient::Result::UNMANAGED:
-      if (IsOptInVerificationDisabled()) {
-        StartArc();
-      } else {
-        // TODO(hidehiko): Merge this prefetching into re-auth case.
-        SetState(State::FETCHING_CODE);
-        PrepareContextForAuthCodeRequest();
-      }
+      VLOG(1) << "Starting ARC for first sign in.";
+      sign_in_time_ = base::Time::Now();
+      arc_sign_in_timer_.Start(FROM_HERE, kArcSignInTimeout,
+                               base::Bind(&ArcAuthService::OnArcSignInTimeout,
+                                          weak_ptr_factory_.GetWeakPtr()));
+      StartArc();
       break;
     case policy::AndroidManagementClient::Result::MANAGED:
       ShutdownBridgeAndShowUI(
@@ -1088,36 +1047,43 @@ void ArcAuthService::OnTermsAgreed(bool is_metrics_enabled,
   preference_handler_->EnableMetrics(is_metrics_enabled);
   preference_handler_->EnableBackupRestore(is_backup_and_restore_enabled);
   preference_handler_->EnableLocationService(is_location_service_enabled);
+  SetUIPage(ArcSupportHost::UIPage::START_PROGRESS, base::string16());
   StartArcAndroidManagementCheck();
 }
 
 void ArcAuthService::OnAuthSucceeded(const std::string& auth_code) {
-  SetAuthCodeAndStartArc(auth_code);
+  OnAuthCodeObtained(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:
-      PrepareContextForAuthCodeRequest();
-      break;
+  // TODO(hidehiko): Simplify the retry logic.
+  if (!profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
+    // If the user has not yet agreed on Terms of Service, then show it.
+    ShowUI(ArcSupportHost::UIPage::TERMS, base::string16());
+  } else if (ui_page_ == ArcSupportHost::UIPage::ERROR_WITH_FEEDBACK) {
+    // ERROR_WITH_FEEDBACK is set in OnSignInFailed(). In the case, stopping
+    // ARC was postponed to contain its internal state into the report.
+    // Here, on retry, stop it, then restart.
+    SetUIPage(ArcSupportHost::UIPage::START_PROGRESS, base::string16());

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

DCHECK_EQ(State::ACTIVE, state_); ?

[hidehiko, 2016/11/15 11:26:47]

Done.

+    ShutdownBridge();

[hidehiko, 2016/11/14 16:57:00]

FYI: this implementation seems to hit some hidden race around session manager.
ArcStopInstance DBus signal looks sent to Chrome twice asynchronously so that
restarting is not yet working. I'll investigate it more tomorrow on my local TZ.

+    reenable_arc_ = true;
+  } else if (state_ == State::ACTIVE) {
+    // This happens when ARC support Chrome app reports an error on "Sign in"
+    // page.
+    // TODO(hidehiko): Currently, due to the existing code structure, we need
+    // to call PrepareContextForAuthCodeRequest() always. However, to fetch
+    // an authtoken via LSO page, it is not necessary to call PrepareContext().
+    // Instead, it is possible to show LSO page, immediately.
+    SetUIPage(ArcSupportHost::UIPage::START_PROGRESS, base::string16());
+    PrepareContextForAuthCodeRequest();
+  } else {
+    // Otherwise, we restart the ARC. Note: this is the first boot case.

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

nit: s/the ARC/ARC/g

[hidehiko, 2016/11/15 11:26:47]

Done.

+    // For second or later boot, either ERROR_WITH_FEEDBACK case or ACTIVE
+    // case must hit.
+    SetUIPage(ArcSupportHost::UIPage::START_PROGRESS, base::string16());
+    StartArcAndroidManagementCheck();
   }
 }
 
@@ -1153,8 +1119,6 @@ std::ostream& operator<<(std::ostream& os, const ArcAuthService::State& state) {
       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: