Use policy data to determine whether to upload an enrollment ID

Act directly on changes to policy data. Do not store a CrOS setting
for when an enrollment identifier is needed. Do not convert the
current policy in ChromeDeviceSettingsProto to anything (it will be
removed).

BUG=chromium:778535
TEST=unit_tests

Change-Id: I49fb2537556f9e3781267f9eb2ede1b2d5dee964
Reviewed-on: https://chromium-review.googlesource.com/827575Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524523}

chrome/browser/chromeos/attestation/enrollment_policy_observer.cc

@@ -63,26 +63,24 @@ namespace attestation {
 
 EnrollmentPolicyObserver::EnrollmentPolicyObserver(
     policy::CloudPolicyClient* policy_client)
-    : cros_settings_(CrosSettings::Get()),
+    : device_settings_service_(DeviceSettingsService::Get()),
       policy_client_(policy_client),
-      cryptohome_client_(NULL),
-      attestation_flow_(NULL),
+      cryptohome_client_(nullptr),
+      attestation_flow_(nullptr),
       num_retries_(0),
       retry_delay_(kRetryDelay),
       weak_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  attestation_subscription_ = cros_settings_->AddSettingsObserver(
-      kDeviceEnrollmentIdNeeded,
-      base::Bind(&EnrollmentPolicyObserver::EnrollmentSettingChanged,
-                 base::Unretained(this)));
+  device_settings_service_->AddObserver(this);
   Start();
 }
 
 EnrollmentPolicyObserver::EnrollmentPolicyObserver(
     policy::CloudPolicyClient* policy_client,
+    DeviceSettingsService* device_settings_service,
     CryptohomeClient* cryptohome_client,
     AttestationFlow* attestation_flow)
-    : cros_settings_(CrosSettings::Get()),
+    : device_settings_service_(device_settings_service),
       policy_client_(policy_client),
       cryptohome_client_(cryptohome_client),
       attestation_flow_(attestation_flow),
@@ -90,28 +88,28 @@ EnrollmentPolicyObserver::EnrollmentPolicyObserver(
       retry_delay_(kRetryDelay),
       weak_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  attestation_subscription_ = cros_settings_->AddSettingsObserver(
-      kDeviceEnrollmentIdNeeded,
-      base::Bind(&EnrollmentPolicyObserver::EnrollmentSettingChanged,
-                 base::Unretained(this)));
+
+  device_settings_service_->AddObserver(this);
   Start();
 }
 
 EnrollmentPolicyObserver::~EnrollmentPolicyObserver() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(DeviceSettingsService::IsInitialized());
+  device_settings_service_->RemoveObserver(this);
 }
 
-void EnrollmentPolicyObserver::EnrollmentSettingChanged() {
+void EnrollmentPolicyObserver::DeviceSettingsUpdated() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   num_retries_ = 0;
   Start();
 }
 
 void EnrollmentPolicyObserver::Start() {
-  // If we identification for enrollment isn't needed, there is nothing to do.
-  bool needed = false;
-  if (!cros_settings_->GetBoolean(kDeviceEnrollmentIdNeeded, &needed) ||
-      !needed)
+  // If identification for enrollment isn't needed, there is nothing to do.
+  const enterprise_management::PolicyData* policy_data =
+      device_settings_service_->policy_data();
+  if (!policy_data || !policy_data->enrollment_id_needed())
     return;
 
   // We expect a registered CloudPolicyClient.
@@ -169,7 +167,6 @@ void EnrollmentPolicyObserver::OnUploadComplete(bool status) {
   if (!status)
     return;
   VLOG(1) << "Enterprise Enrollment Certificate uploaded to DMServer.";
-  cros_settings_->SetBoolean(kDeviceEnrollmentIdNeeded, false);
 }
 
 void EnrollmentPolicyObserver::Reschedule() {

chrome/browser/chromeos/attestation/enrollment_policy_observer.h

@@ -11,7 +11,7 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/settings/device_settings_service.h"
 
 namespace policy {
 class CloudPolicyClient;
@@ -19,7 +19,6 @@ class CloudPolicyClient;
 
 namespace chromeos {
 
-class CrosSettings;
 class CryptohomeClient;
 
 namespace attestation {
@@ -28,28 +27,28 @@ class AttestationFlow;
 
 // A class which observes policy changes and triggers uploading identification
 // for enrollment if necessary.
-class EnrollmentPolicyObserver {
+class EnrollmentPolicyObserver : public DeviceSettingsService::Observer {
  public:
-  // The observer immediately connects with CrosSettings to listen for policy
-  // changes.  The CloudPolicyClient is used to upload data to the server; it
-  // must be in the registered state.  This class does not take ownership of
-  // |policy_client|.
+  // The observer immediately connects with DeviceSettingsService to listen for
+  // policy changes.  The CloudPolicyClient is used to upload data to the
+  // server; it must be in the registered state.  This class does not take
+  // ownership of |policy_client|.
   explicit EnrollmentPolicyObserver(policy::CloudPolicyClient* policy_client);
 
-  // A constructor which allows custom CryptohomeClient and AttestationFlow
-  // implementations.  Useful for testing.
+  // A constructor which accepts custom instances useful for testing.
   EnrollmentPolicyObserver(policy::CloudPolicyClient* policy_client,
+                           DeviceSettingsService* device_settings_service,
                            CryptohomeClient* cryptohome_client,
                            AttestationFlow* attestation_flow);
 
-  ~EnrollmentPolicyObserver();
+  ~EnrollmentPolicyObserver() override;
 
   // Sets the retry delay in seconds; useful in testing.
   void set_retry_delay(int retry_delay) { retry_delay_ = retry_delay; }
 
  private:
-  // Called when the enrollment setting changes.
-  void EnrollmentSettingChanged();
+  // Called when the device settings change.
+  void DeviceSettingsUpdated() override;
 
   // Checks enrollment setting and starts any necessary work.
   void Start();
@@ -69,7 +68,7 @@ class EnrollmentPolicyObserver {
   // signal which indicates the system is ready to process this task.
   void Reschedule();
 
-  CrosSettings* cros_settings_;
+  DeviceSettingsService* device_settings_service_;
   policy::CloudPolicyClient* policy_client_;
   CryptohomeClient* cryptohome_client_;
   AttestationFlow* attestation_flow_;
@@ -77,12 +76,12 @@ class EnrollmentPolicyObserver {
   int num_retries_;
   int retry_delay_;
 
-  std::unique_ptr<CrosSettings::ObserverSubscription> attestation_subscription_;
-
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate the weak pointers before any other members are destroyed.
   base::WeakPtrFactory<EnrollmentPolicyObserver> weak_factory_;
 
+  friend class EnrollmentPolicyObserverTest;
+
   DISALLOW_COPY_AND_ASSIGN(EnrollmentPolicyObserver);
 };
 

chrome/browser/chromeos/attestation/enrollment_policy_observer_unittest.cc

@@ -13,16 +13,16 @@
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chromeos/attestation/enrollment_policy_observer.h"
-#include "chrome/browser/chromeos/settings/scoped_cros_settings_test_helper.h"
+#include "chrome/browser/chromeos/settings/device_settings_test_helper.h"
 #include "chromeos/attestation/mock_attestation_flow.h"
 #include "chromeos/dbus/fake_cryptohome_client.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
-#include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
 using testing::Invoke;
+using testing::Return;
 using testing::StrictMock;
 using testing::WithArgs;
 
@@ -44,40 +44,47 @@ void StatusCallbackSuccess(
 
 }  // namespace
 
-class EnrollmentPolicyObserverTest : public ::testing::Test {
+class EnrollmentPolicyObserverTest : public DeviceSettingsTestBase {
  public:
   EnrollmentPolicyObserverTest() {
-    settings_helper_.ReplaceProvider(kDeviceEnrollmentIdNeeded);
-    settings_helper_.SetBoolean(kDeviceEnrollmentIdNeeded, true);
     policy_client_.SetDMToken("fake_dm_token");
   }
 
  protected:
-  // Configures mock expectations when identification for enrollment is needed.
-  void SetupMocks() {
-    EXPECT_CALL(attestation_flow_, GetCertificate(_, _, _, _, _))
-        .WillOnce(WithArgs<4>(Invoke(CertCallbackSuccess)));
-    EXPECT_CALL(policy_client_,
-                UploadEnterpriseEnrollmentCertificate("fake_cert", _))
-        .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
+  void SetUpEnrollmentIdNeeded(bool enrollment_id_needed) {
+    if (enrollment_id_needed) {
+      EXPECT_CALL(attestation_flow_, GetCertificate(_, _, _, _, _))
+          .WillOnce(WithArgs<4>(Invoke(CertCallbackSuccess)));
+      EXPECT_CALL(policy_client_,
+                  UploadEnterpriseEnrollmentCertificate("fake_cert", _))
+          .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
+    }
+    device_policy_.policy_data().set_enrollment_id_needed(enrollment_id_needed);
+    device_policy_.Build();
+    session_manager_client_.set_device_policy(device_policy_.GetBlob());
+    ReloadDeviceSettings();
   }
 
   void Run() {
-    EnrollmentPolicyObserver observer(&policy_client_, &cryptohome_client_,
-                                      &attestation_flow_);
+    EnrollmentPolicyObserver observer(&policy_client_,
+                                      &device_settings_service_,
+                                      &cryptohome_client_, &attestation_flow_);
     observer.set_retry_delay(0);
     base::RunLoop().RunUntilIdle();
   }
 
-  content::TestBrowserThreadBundle test_browser_thread_bundle_;
-  ScopedCrosSettingsTestHelper settings_helper_;
   FakeCryptohomeClient cryptohome_client_;
   StrictMock<MockAttestationFlow> attestation_flow_;
   StrictMock<policy::MockCloudPolicyClient> policy_client_;
 };
 
+TEST_F(EnrollmentPolicyObserverTest, UploadEnterpriseEnrollmentCertificate) {
+  SetUpEnrollmentIdNeeded(true);
+  Run();
+}
+
 TEST_F(EnrollmentPolicyObserverTest, FeatureDisabled) {
-  settings_helper_.SetBoolean(kDeviceEnrollmentIdNeeded, false);
+  SetUpEnrollmentIdNeeded(false);
   Run();
 }
 
@@ -87,7 +94,7 @@ TEST_F(EnrollmentPolicyObserverTest, UnregisteredPolicyClient) {
 }
 
 TEST_F(EnrollmentPolicyObserverTest, DBusFailureRetry) {
-  SetupMocks();
+  SetUpEnrollmentIdNeeded(true);
   // Simulate a DBus failure.
   cryptohome_client_.SetServiceIsAvailable(false);
 

chrome/browser/chromeos/settings/device_settings_provider.cc

@@ -66,7 +66,6 @@ const char* const kKnownSettings[] = {
     kDeviceAttestationEnabled,
     kDeviceDisabled,
     kDeviceDisabledMessage,
-    kDeviceEnrollmentIdNeeded,
     kDeviceHostnameTemplate,
     kDeviceLoginScreenAppInstallList,
     kDeviceLoginScreenInputMethods,
@@ -608,15 +607,6 @@ void DecodeGenericPolicies(
     }
   }
 
-  if (policy.has_forced_reenrollment()) {
-    const em::ForcedReenrollmentProto& container(policy.forced_reenrollment());
-    if (container.has_enrollment_id_needed()) {
-      new_values_cache->SetValue(
-          kDeviceEnrollmentIdNeeded,
-          base::MakeUnique<base::Value>(container.enrollment_id_needed()));
-    }
-  }
-
   if (policy.has_unaffiliated_arc_allowed()) {
     const em::UnaffiliatedArcAllowedProto& container(
         policy.unaffiliated_arc_allowed());

chromeos/settings/cros_settings_names.cc

@@ -294,10 +294,6 @@ const char kMinimumRequiredChromeVersion[] = "cros.min_version.chrome";
 // If the string is empty or blank the system name will be used.
 const char kCastReceiverName[] = "cros.device.cast_receiver.name";
 
-// Boolean indicating whether the client needs to upload an enrollment ID
-// which can be used for automatic forced re-enrollment.
-const char kDeviceEnrollmentIdNeeded[] = "cros.device.enrollment_id_needed";
-
 // A boolean pref that indicates whether unaffiliated users are allowed to
 // use ARC.
 const char kUnaffiliatedArcAllowed[] = "cros.device.unaffiliated_arc_allowed";

chromeos/settings/cros_settings_names.h

@@ -135,8 +135,6 @@ CHROMEOS_EXPORT extern const char kMinimumRequiredChromeVersion[];
 
 CHROMEOS_EXPORT extern const char kCastReceiverName[];
 
-CHROMEOS_EXPORT extern const char kDeviceEnrollmentIdNeeded[];
-
 CHROMEOS_EXPORT extern const char kUnaffiliatedArcAllowed[];
 
 CHROMEOS_EXPORT extern const char kDeviceHostnameTemplate[];