Extract ArcTermsOfServiceNegotiator implementation.

chrome/browser/chromeos/arc/arc_session_manager.cc

 // Copyright 2016 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_session_manager.h"
 
 #include <utility>
 
 #include "ash/common/shelf/shelf_delegate.h"
 #include "ash/common/wm_shell.h"
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/arc/arc_auth_code_fetcher.h"
 #include "chrome/browser/chromeos/arc/arc_auth_context.h"
 #include "chrome/browser/chromeos/arc/arc_optin_uma.h"
 #include "chrome/browser/chromeos/arc/arc_support_host.h"
 #include "chrome/browser/chromeos/arc/auth/arc_robot_auth.h"
-#include "chrome/browser/chromeos/arc/optin/arc_optin_preference_handler.h"
+#include "chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h"
 #include "chrome/browser/chromeos/arc/policy/arc_android_management_checker.h"
 #include "chrome/browser/chromeos/arc/policy/arc_policy_util.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/policy/profile_policy_connector.h"
 #include "chrome/browser/policy/profile_policy_connector_factory.h"
 #include "chrome/browser/prefs/pref_service_syncable_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_launcher.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
@@ skipping 337 matching lines @@
   //
   // Don't show UI for ARC Kiosk because the only one UI in kiosk mode must
   // be the kiosk app. In case of error the UI will be useless as well, because
   // in typical use case there will be no one nearby the kiosk device, who can
   // do some action to solve the problem be means of UI.
   if (!g_disable_ui_for_testing && !IsOptInVerificationDisabled() &&
       !IsArcKioskMode()) {
     DCHECK(!support_host_);
     support_host_ = base::MakeUnique<ArcSupportHost>(profile_);
     support_host_->AddObserver(this);
-
-    preference_handler_ = base::MakeUnique<arc::ArcOptInPreferenceHandler>(
-        this, profile_->GetPrefs());
-    // This automatically updates all preferences.
-    preference_handler_->Start();
   }
 
   DCHECK_EQ(State::NOT_INITIALIZED, state_);
   SetState(State::STOPPED);
 
   PrefServiceSyncableFromProfile(profile_)->AddSyncedPrefObserver(
       prefs::kArcEnabled, this);
 
   context_.reset(new ArcAuthContext(profile_));
 
@@ skipping 91 matching lines @@
     RemoveArcData();
     return;
   }
 
   if (state_ == State::ACTIVE)
     return;
 
   if (support_host_)
     support_host_->SetArcManaged(IsArcManaged());
 
-  // 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.
-  if (IsOptInVerificationDisabled() || IsArcKioskMode()) {
-    // Automatically accept terms in kiosk mode. This is not required for
-    // IsOptInVerificationDisabled mode because in last case it may cause
-    // a privacy issue on next run without this flag set.
-    if (IsArcKioskMode())
-      profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
-    StartArc();
+  // 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.
+  // TODO(poromov): Move to more Kiosk dedicated set-up phase.
+  if (IsArcKioskMode())
+    profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
+
+  // In case UI is disabled we assume that ARC is opted-in.
+  if (!profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted) &&
+      !IsOptInVerificationDisabled()) {
+    // Need user's explicit Terms Of Service agreement.
+    StartTermsOfServiceNegotiation();
     return;
   }
 
-  if (!profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn)) {
-    if (profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
-      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
-    // OnBackgroundAndroidManagementChecked() could be ignored.
-    if (!g_disable_ui_for_testing ||
-        g_enable_check_android_management_for_testing) {
-      android_management_checker_.reset(new ArcAndroidManagementChecker(
-          profile_, context_->token_service(), context_->account_id(),
-          true /* retry_on_error */));
-      android_management_checker_->StartCheck(
-          base::Bind(&ArcSessionManager::OnBackgroundAndroidManagementChecked,
-                     weak_ptr_factory_.GetWeakPtr()));
-    }
+  StartArc();
+
+  // Skip Android management check for testing.
+  if (IsOptInVerificationDisabled() ||
+      (g_disable_ui_for_testing &&
+       !g_enable_check_android_management_for_testing)) {
+    return;
   }
+
+  // We skip following if Android management check because there are no
+  // managed human users for Kiosk exist.
+  if (IsArcKioskMode())
+    return;
+
+  // If Sign-in is not yet completed, but terms-of-service is already agreed,
+  // then it should have been done in OOBE phase.

[Luis Héctor Chávez, 2016/11/29 18:18:12]

ah! now I understand this part.

"If Sign-in is not yet completed, then management checking will be performed in
the OOBE phase."

IIUC, we never perform management checking (from the Chrome side) on first
sign-in, which means that management checking will be done later, either in
Android or next boot on Chrome.

[hidehiko, 2016/12/01 14:20:09]

Chatted with khmel@ offline.
It was not, but it was planned.

However, this check hit an edge case (2) of the comment in new code), and there
were no good way to split them.

So, instead, we agreed to check Android management status even OOBE case here.
Also, I found that it is necessary to check kArcSignIn flag first, for backward
compatibility.

Revised the code.

+  // The Android management check should be also done in the OOBE phase.
+  if (!profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn))
+    return;
+
+  // Check Android management in parallel.
+  // 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
+  // OnBackgroundAndroidManagementChecked() could be ignored.
+  android_management_checker_ = base::MakeUnique<ArcAndroidManagementChecker>(
+      profile_, context_->token_service(), context_->account_id(),
+      true /* retry_on_error */);
+  android_management_checker_->StartCheck(
+      base::Bind(&ArcSessionManager::OnBackgroundAndroidManagementChecked,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ArcSessionManager::ShutdownBridge() {
   arc_sign_in_timer_.Stop();
   playstore_launcher_.reset();
+  terms_of_service_negotiator_.reset();
   android_management_checker_.reset();
   arc_bridge_service()->RequestStop();
   if (state_ != State::NOT_INITIALIZED)
     SetState(State::STOPPED);
   for (auto& observer : observer_list_)
     observer.OnShutdownBridge();
 }
 
 void ArcSessionManager::AddObserver(Observer* observer) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ skipping 92 matching lines @@
   profile_->GetPrefs()->SetBoolean(prefs::kArcEnabled, false);
 }
 
 void ArcSessionManager::RecordArcState() {
   // Only record Enabled state if ARC is allowed in the first place, so we do
   // not split the ARC population by devices that cannot run ARC.
   if (IsAllowed())
     UpdateEnabledStateUMA(IsArcEnabled());
 }
 
-void ArcSessionManager::StartUI() {
+void ArcSessionManager::StartTermsOfServiceNegotiation() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(!terms_of_service_negotiator_);
 
   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.
     if (support_host_) {
       support_host_->ShowError(
           ArcSupportHost::Error::SIGN_IN_SERVICE_UNAVAILABLE_ERROR, false);
     }
     return;
   }
 
   SetState(State::SHOWING_TERMS_OF_SERVICE);
-  if (support_host_)
-    support_host_->ShowTermsOfService();
+  if (support_host_) {
+    terms_of_service_negotiator_ =
+        base::MakeUnique<ArcTermsOfServiceNegotiator>(profile_->GetPrefs(),
+                                                      support_host_.get());
+    terms_of_service_negotiator_->StartNegotiation(
+        base::Bind(&ArcSessionManager::OnTermsOfServiceNegotiated,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
+void ArcSessionManager::OnTermsOfServiceNegotiated(bool agreed) {

[Luis Héctor Chávez, 2016/11/29 18:18:12]

nit: let's standardize on "accepted" since you're then storing this in a pref
called prefs::kArcTermsAccepted.

[hidehiko, 2016/12/01 14:20:09]

Done for ArcTermsOfServiceNegotiator and above layers.

I left OnTermsAgreed as is, because the button is "AGREE" for now.
I'm happy to change them, too (either changing ArcSupportHost /
arc_support/background.js and/or UI itself) if necessary, but then let me work
on in a separate CL.

+  DCHECK(terms_of_service_negotiator_);
+  terms_of_service_negotiator_.reset();
+
+  if (!agreed) {
+    // To cancel, user needs to close the window. Note that clicking "Cancel"
+    // button effectively just closes the window.
+    CancelAuthCode();
+    return;
+  }
+
+  // Terms were accepted.
+  profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
+
+  support_host_->ShowArcLoading();
+  StartArcAndroidManagementCheck();
 }
 
 void ArcSessionManager::StartArcAndroidManagementCheck() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(arc_bridge_service()->stopped());
   DCHECK(state_ == State::SHOWING_TERMS_OF_SERVICE ||
          state_ == State::CHECKING_ANDROID_MANAGEMENT);
   SetState(State::CHECKING_ANDROID_MANAGEMENT);
 
   android_management_checker_.reset(new ArcAndroidManagementChecker(
@@ skipping 50 matching lines @@
       break;
     case policy::AndroidManagementClient::Result::ERROR:
       // This code should not be reached. For background check,
       // retry_on_error should be set.
       NOTREACHED();
   }
 }
 
 void ArcSessionManager::OnWindowClosed() {
   DCHECK(support_host_);
+  if (terms_of_service_negotiator_) {
+    // In this case, ArcTermsOfServiceNegotiator should handle the case.
+    // Do nothing.
+    return;
+  }
   CancelAuthCode();
 }
 
 void ArcSessionManager::OnTermsAgreed(bool is_metrics_enabled,
                                       bool is_backup_and_restore_enabled,
                                       bool is_location_service_enabled) {
   DCHECK(support_host_);
-
-  // Terms were accepted
-  profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
-
-  // Since this is ARC support's UI event callback, 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);
-  support_host_->ShowArcLoading();
-  StartArcAndroidManagementCheck();
+  DCHECK(terms_of_service_negotiator_);
+  // This should be handled in ArcTermsOfServiceNegotiator. Do nothing here.
 }
 
 void ArcSessionManager::OnRetryClicked() {
   DCHECK(support_host_);
 
   UpdateOptInActionUMA(OptInActionType::RETRY);
 
   // 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.
-    support_host_->ShowTermsOfService();
+  if (terms_of_service_negotiator_) {
+    // Currently Terms of service is shown. ArcTermsOfServiceNegotiator should
+    // handle this.
+  } else if (!profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
+    StartTermsOfServiceNegotiation();
   } else if (support_host_->ui_page() == ArcSupportHost::UIPage::ERROR &&
              !arc_bridge_service()->stopped()) {
     // 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.
     DCHECK_EQ(State::ACTIVE, state_);
     support_host_->ShowArcLoading();
     ShutdownBridge();
     reenable_arc_ = true;
   } else if (state_ == State::ACTIVE) {
     // This case is handled in ArcAuthService.
     // Do nothing.
   } else {
     // Otherwise, we restart ARC. Note: this is the first boot case.
     // For second or later boot, either ERROR_WITH_FEEDBACK case or ACTIVE
     // case must hit.
     support_host_->ShowArcLoading();
     StartArcAndroidManagementCheck();
   }
 }
 
 void ArcSessionManager::OnSendFeedbackClicked() {
   DCHECK(support_host_);
   chrome::OpenFeedbackDialog(nullptr);
 }
 
-void ArcSessionManager::OnMetricsModeChanged(bool enabled, bool managed) {
-  if (!support_host_)
-    return;
-  support_host_->SetMetricsPreferenceCheckbox(enabled, managed);
-}
-
-void ArcSessionManager::OnBackupAndRestoreModeChanged(bool enabled,
-                                                      bool managed) {
-  if (!support_host_)
-    return;
-  support_host_->SetBackupAndRestorePreferenceCheckbox(enabled, managed);
-}
-
-void ArcSessionManager::OnLocationServicesModeChanged(bool enabled,
-                                                      bool managed) {
-  if (!support_host_)
-    return;
-  support_host_->SetLocationServicesPreferenceCheckbox(enabled, managed);
-}
-
 std::ostream& operator<<(std::ostream& os,
                          const ArcSessionManager::State& state) {
   switch (state) {
     case ArcSessionManager::State::NOT_INITIALIZED:
       return os << "NOT_INITIALIZED";
     case ArcSessionManager::State::STOPPED:
       return os << "STOPPED";
     case ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE:
       return os << "SHOWING_TERMS_OF_SERVICE";
     case ArcSessionManager::State::CHECKING_ANDROID_MANAGEMENT:
       return os << "CHECKING_ANDROID_MANAGEMENT";
     case ArcSessionManager::State::ACTIVE:
       return os << "ACTIVE";
   }
 
   // Some compiler reports an error even if all values of an enum-class are
   // covered indivisually in a switch statement.
   NOTREACHED();
   return os;
 }
 
 }  // namespace arc