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

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 103 matching lines @@
   return ProvisioningResult::UNKNOWN_ERROR;
 }
 
 }  // namespace
 
 // TODO(lhchavez): Get rid of this class once we can safely remove all the
 // deprecated interfaces and only need to care about one type of callback.
 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) {}
 
   explicit AccountInfoNotifier(
       const GetAuthCodeAndAccountTypeDeprecatedCallback& auth_account_callback)
       : callback_type_(CallbackType::AUTH_CODE_AND_ACCOUNT),
         auth_account_callback_(auth_account_callback) {}
 
   explicit AccountInfoNotifier(const AccountInfoCallback& account_info_callback)
       : callback_type_(CallbackType::ACCOUNT_INFO),
         account_info_callback_(account_info_callback) {}
 
   void Notify(bool is_enforced,
               const std::string& auth_code,
               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);
         break;
       case CallbackType::AUTH_CODE_AND_ACCOUNT:
         DCHECK(!auth_account_callback_.is_null());
         auth_account_callback_.Run(auth_code, is_enforced, account_type);
         break;
       case CallbackType::ACCOUNT_INFO:
         DCHECK(!account_info_callback_.is_null());
         mojom::AccountInfoPtr account_info = mojom::AccountInfo::New();
         if (!is_enforced) {
           account_info->auth_code = nullptr;
         } else {
           account_info->auth_code = auth_code;
         }
         account_info->account_type = account_type;
         account_info->is_managed = is_managed;
         account_info_callback_.Run(std::move(account_info));
         break;
     }
   }
 
  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_;
 };
 
 ArcAuthService::ArcAuthService(ArcBridgeService* bridge_service)
     : ArcService(bridge_service), binding_(this), weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!g_arc_auth_service);
 
@@ skipping 119 matching lines @@
 }
 
 void ArcAuthService::RemoveArcData() {
   if (!arc_bridge_service()->stopped()) {
     // Just set a flag. On bridge stopped, this will be re-called,
     // then session manager should remove the data.
     clear_required_ = true;
     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_)),
       base::Bind(&ArcAuthService::OnArcDataRemoved,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 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
   // "on managed lost" case. In that case, OnBridgeStopped() should trigger
   // the RemoveArcData(), then this.
   // TODO(hidehiko): Restructure the code.
   if (!reenable_arc_)
     return;
 
   // Restart ARC anyway. Let the enterprise reporting instance decide whether
   // the ARC user data wipe is still required or not.
   reenable_arc_ = false;
   VLOG(1) << "Reenable ARC";
   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(
     const GetAuthCodeDeprecatedCallback& callback) {
   RequestAccountInfoInternal(
       base::MakeUnique<ArcAuthService::AccountInfoNotifier>(callback));
 }
 
 void ArcAuthService::GetAuthCodeAndAccountTypeDeprecated(
     const GetAuthCodeAndAccountTypeDeprecatedCallback& callback) {
@@ skipping 16 matching lines @@
   instance->OnAccountInfoReady(std::move(account_info));
 }
 
 void ArcAuthService::RequestAccountInfoInternal(
     std::unique_ptr<ArcAuthService::AccountInfoNotifier>
         account_info_notifier) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   // 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;
   }
 
   account_info_notifier_ = std::move(account_info_notifier);
   PrepareContextForAuthCodeRequest();
 }
 
 bool ArcAuthService::IsAuthCodeRequest() const {
@@ skipping 159 matching lines @@
 
   SetState(State::STOPPED);
 
   PrefServiceSyncableFromProfile(profile_)->AddSyncedPrefObserver(
       prefs::kArcEnabled, this);
 
   context_.reset(new ArcAuthContext(this, profile_));
 
   // In case UI is disabled we assume that ARC is opted-in.
   if (IsOptInVerificationDisabled()) {
-    auth_code_.clear();
     StartArc();
     return;
   }
 
   if (!g_disable_ui_for_testing ||
       g_enable_check_android_management_for_testing) {
     ArcAndroidManagementChecker::StartClient();
   }
   pref_change_registrar_.Init(profile_->GetPrefs());
   pref_change_registrar_.Add(
@@ skipping 103 matching lines @@
 
   if (!arc_enabled) {
     StopArc();
     RemoveArcData();
     return;
   }
 
   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();
     }
   } 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().
     // Otherwise, DisableArc() which may be called in
@@ skipping 68 matching lines @@
   reenable_arc_ = true;
   StopArc();
 }
 
 void ArcAuthService::StartArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   arc_bridge_service()->RequestStart();
   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() {
   LOG(ERROR) << "Timed out waiting for first sign in.";
   OnSignInFailedInternal(ProvisioningResult::OVERALL_SIGN_IN_TIMEOUT);
 }
 
 void ArcAuthService::StartLso() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   DCHECK(arc_bridge_service()->stopped());
   SetState(State::ANDROID_MANAGEMENT_CHECK);
   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 && 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;
   }
 
   // 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 55 matching lines @@
         ArcSupportHost::UIPage::ERROR,
         l10n_util::GetStringUTF16(IDS_ARC_SIGN_IN_SERVICE_UNAVAILABLE_ERROR));
     return;
   }
 
   SetState(State::TERMS);
   ShowUI(ArcSupportHost::UIPage::TERMS, base::string16());
 }
 
 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));
   UpdateOptInCancelUMA(OptInCancelReason::NETWORK_ERROR);
 }
 
 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));
   UpdateOptInCancelUMA(OptInCancelReason::NETWORK_ERROR);
 }
 
 void ArcAuthService::StartArcAndroidManagementCheck() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(arc_bridge_service()->stopped());
   SetState(State::ANDROID_MANAGEMENT_CHECK);
 
   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:
-      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(
           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 52 matching lines @@
   // 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);
+  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::TERMS:
       ShowUI(ArcSupportHost::UIPage::TERMS, base::string16());
       break;
     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;
   }
 }
 
 void ArcAuthService::OnSendFeedbackClicked() {
   chrome::OpenFeedbackDialog(nullptr);
 }
 
 void ArcAuthService::OnMetricsModeChanged(bool enabled, bool managed) {
   if (!support_host_)
@@ skipping 16 matching lines @@
 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:
       NOTREACHED();
       return os;
   }
 }
 
 }  // namespace arc