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 80eea15d16b62eed82e1b59b75916a042a238829..5f5d9ec1ebac2f96d11188820dd68fbe064f10e2 100644
--- a/chrome/browser/chromeos/arc/arc_auth_service.cc
+++ b/chrome/browser/chromeos/arc/arc_auth_service.cc
@@ -121,6 +121,11 @@ ProvisioningResult ConvertArcSignInFailureReasonToProvisioningResult(
 class ArcAuthService::AccountInfoNotifier {
  public:
   explicit AccountInfoNotifier(
+      const GetAuthCodeDeprecated0Callback& auth0_callback)

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

This hasn't been used since M52. Let's get rid of it.

[hidehiko, 2016/11/14 11:29:31]

Extracted to crrev.com/2499933002.
For this topic, let's discuss on the CL, although I imported the code in this CL
to resolve the compile error. I'll just rebase if necessary.

+      : callback_type_(CallbackType::AUTH_CODE0),
+        auth0_callback_(auth0_callback) {}
+
+  explicit AccountInfoNotifier(
       const GetAuthCodeDeprecatedCallback& auth_callback)
       : callback_type_(CallbackType::AUTH_CODE),
         auth_callback_(auth_callback) {}
@@ -139,6 +144,10 @@ class ArcAuthService::AccountInfoNotifier {
               mojom::ChromeAccountType account_type,
               bool is_managed) {
     switch (callback_type_) {
+      case CallbackType::AUTH_CODE0:
+        DCHECK(!auth0_callback_.is_null());
+        auth0_callback_.Run(auth_code);
+        break;
       case CallbackType::AUTH_CODE:
         DCHECK(!auth_callback_.is_null());
         auth_callback_.Run(auth_code, is_enforced);
@@ -163,9 +172,15 @@ class ArcAuthService::AccountInfoNotifier {
   }
 
  private:
-  enum class CallbackType { AUTH_CODE, AUTH_CODE_AND_ACCOUNT, ACCOUNT_INFO };
+  enum class CallbackType {
+    AUTH_CODE0,
+    AUTH_CODE,
+    AUTH_CODE_AND_ACCOUNT,
+    ACCOUNT_INFO
+  };
 
   const CallbackType callback_type_;
+  const GetAuthCodeDeprecated0Callback auth0_callback_;
   const GetAuthCodeDeprecatedCallback auth_callback_;
   const GetAuthCodeAndAccountTypeDeprecatedCallback auth_account_callback_;
   const AccountInfoCallback account_info_callback_;
@@ -305,6 +320,8 @@ void ArcAuthService::RemoveArcData() {
     return;
   }
   clear_required_ = false;
+  DCHECK(!arc_data_is_being_removed_);
+  arc_data_is_being_removed_ = true;
   chromeos::DBusThreadManager::Get()->GetSessionManagerClient()->RemoveArcData(
       cryptohome::Identification(
           multi_user_util::GetAccountIdFromProfile(profile_)),
@@ -314,6 +331,7 @@ void ArcAuthService::RemoveArcData() {
 
 void ArcAuthService::OnArcDataRemoved(bool success) {
   LOG_IF(ERROR, !success) << "Required ARC user data wipe failed.";
+  arc_data_is_being_removed_ = false;
 
   // Here check if |reenable_arc_| is marked or not.
   // The only case this happens should be in the special case for enterprise
@@ -330,18 +348,10 @@ 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());
-  callback.Run(GetAndResetAuthCode());

[hidehiko, 2016/11/10 06:49:28]

Note: If you prefer, I can extract this part into another CL.

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

Actually can you log that this method is deprecated and
callback.Run(std::string());? Let's stop maintaining this.

[hidehiko, 2016/11/14 11:29:31]

Acknowledged.

+  RequestAccountInfoInternal(
+      base::MakeUnique<ArcAuthService::AccountInfoNotifier>(callback));
 }
 
 void ArcAuthService::GetAuthCodeDeprecated(
@@ -378,10 +388,8 @@ 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,
+  if (IsOptInVerificationDisabled()) {
+    account_info_notifier->Notify(false /* = is_enforced */, "",

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

nit: s/""/std::string()/

[hidehiko, 2016/11/14 11:29:31]

Done.

                                   mojom::ChromeAccountType::USER_ACCOUNT,
                                   policy_util::IsAccountManaged(profile_));
     return;
@@ -561,7 +569,6 @@ void ArcAuthService::OnPrimaryUserProfilePrepared(Profile* profile) {
 
   // In case UI is disabled we assume that ARC is opted-in.
   if (IsOptInVerificationDisabled()) {
-    auth_code_.clear();
     StartArc();
     return;
   }
@@ -685,13 +692,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();
@@ -780,34 +784,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,
-                                   mojom::ChromeAccountType::USER_ACCOUNT,
-                                   policy_util::IsAccountManaged(profile_));
-    account_info_notifier_.reset();
-    return;
-  }
-
-  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.";

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

Is there no way of knowing if this is the first sign-in? Are there other logs
that already replace this?

[hidehiko, 2016/11/14 11:29:31]

Moved to OnAndroidManagementChecked().

-
-  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,
+                                 mojom::ChromeAccountType::USER_ACCOUNT,
+                                 policy_util::IsAccountManaged(profile_));
+  account_info_notifier_.reset();
 }
 
 void ArcAuthService::OnArcSignInTimeout() {
@@ -834,8 +818,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::TERMS &&
-       state_ != State::ANDROID_MANAGEMENT_CHECK) &&
+  if ((state_ != State::TERMS && state_ != State::ANDROID_MANAGEMENT_CHECK) &&
       ui_page_ != ArcSupportHost::UIPage::ERROR &&
       ui_page_ != ArcSupportHost::UIPage::ERROR_WITH_FEEDBACK) {
     return;
@@ -911,8 +894,6 @@ void ArcAuthService::StartUI() {
 }
 
 void ArcAuthService::OnPrepareContextFailed() {
-  DCHECK_EQ(state_, State::FETCHING_CODE);

[hidehiko, 2016/11/10 06:49:28]

NOTE: This was actually wrong. This could be ACTIVE...

-
   ShutdownBridgeAndShowUI(
       ArcSupportHost::UIPage::ERROR,
       l10n_util::GetStringUTF16(IDS_ARC_SERVER_COMMUNICATION_ERROR));
@@ -920,12 +901,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);

[hidehiko, 2016/11/10 06:49:28]

Ditto.

   ShutdownBridgeAndShowUI(
       ArcSupportHost::UIPage::ERROR,
       l10n_util::GetStringUTF16(IDS_ARC_SERVER_COMMUNICATION_ERROR));
@@ -952,13 +932,10 @@ 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();
-      }
+      arc_sign_in_timer_.Start(FROM_HERE, kArcSignInTimeout,

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

|sign_in_time_| should always be updated in tandem with starting this timer.

[hidehiko, 2016/11/14 11:29:31]

Good catch. Done.

+                               base::Bind(&ArcAuthService::OnArcSignInTimeout,
+                                          weak_ptr_factory_.GetWeakPtr()));
+      StartArc();
       break;
     case policy::AndroidManagementClient::Result::MANAGED:
       ShutdownBridgeAndShowUI(
@@ -1031,11 +1008,12 @@ 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() {
@@ -1057,9 +1035,16 @@ void ArcAuthService::OnRetryClicked() {
     case State::ANDROID_MANAGEMENT_CHECK:
       StartArcAndroidManagementCheck();
       break;
-    case State::FETCHING_CODE:
     case State::ACTIVE:
-      PrepareContextForAuthCodeRequest();
+      SetUIPage(ArcSupportHost::UIPage::START_PROGRESS, base::string16());
+      if (state_ != State::STOPPED) {

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

Shouldn't this be always true? (|state_| should be State::ACTIVE here, and I
don't see SetUIPage synchronously changing state).

If it's not, please add a comment explaining since it's very surprising.

[hidehiko, 2016/11/14 11:29:31]

Unfortunately, there is a case this triggers. Commented.

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

Oops, no. I was misunderstanding, and my test coverage was poor...

Now the condition was fixed.

+        PrepareContextForAuthCodeRequest();
+      } else if (!arc_bridge_service()->stopped() ||
+                 arc_data_is_being_removed_) {
+        reenable_arc_ = true;
+      } else {
+        StartArc();
+      }
       break;
   }
 }
@@ -1096,8 +1081,6 @@ std::ostream& operator<<(std::ostream& os, const ArcAuthService::State& state) {
       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: