All DCP manager tests that enroll/register are tested with all auths.

Use a FakeSigningService in the DeviceCloudPolicyInitializer in order
to be able to unit test re-enrollment for locked devices when
registering with a certificate.

BUG=624187,641447
TEST=unit_tests
TBR=pastarmovj

Review-Url: https://codereview.chromium.org/2303483002
Cr-Commit-Position: refs/heads/master@{#415799}

chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc

@@ -65,7 +65,7 @@ namespace policy {
 namespace {
 
 // Install attributes for tests.
-EnterpriseInstallAttributes* g_testing_install_attributes = NULL;
+EnterpriseInstallAttributes* g_testing_install_attributes = nullptr;
 
 // Helper that returns a new SequencedTaskRunner backed by the blocking pool.
 // Each SequencedTaskRunner returned is independent from the others.
@@ -79,12 +79,12 @@ scoped_refptr<base::SequencedTaskRunner> GetBackgroundTaskRunner() {
 }  // namespace
 
 BrowserPolicyConnectorChromeOS::BrowserPolicyConnectorChromeOS()
-    : device_cloud_policy_manager_(NULL),
-      global_user_cloud_policy_provider_(NULL),
+    : device_cloud_policy_manager_(nullptr),
+      global_user_cloud_policy_provider_(nullptr),
       weak_ptr_factory_(this) {
   if (g_testing_install_attributes) {
     install_attributes_.reset(g_testing_install_attributes);
-    g_testing_install_attributes = NULL;
+    g_testing_install_attributes = nullptr;
   }
 
   // SystemSaltGetter or DBusThreadManager may be uninitialized on unit tests.
@@ -271,7 +271,7 @@ void BrowserPolicyConnectorChromeOS::SetInstallAttributesForTesting(
 void BrowserPolicyConnectorChromeOS::RemoveInstallAttributesForTesting() {
   if (g_testing_install_attributes) {
     delete g_testing_install_attributes;
-    g_testing_install_attributes = NULL;
+    g_testing_install_attributes = nullptr;
   }
 }
 

chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc

@@ -73,7 +73,13 @@ DeviceCloudPolicyInitializer::DeviceCloudPolicyInitializer(
       device_store_(device_store),
       manager_(manager),
       attestation_flow_(std::move(attestation_flow)),
-      signing_service_(async_method_caller) {}
+      signing_service_(base::MakeUnique<TpmEnrollmentKeySigningService>(
+          async_method_caller)) {}
+
+void DeviceCloudPolicyInitializer::SetSigningServiceForTesting(
+    std::unique_ptr<policy::SigningService> signing_service) {
+  signing_service_ = std::move(signing_service);
+}
 
 DeviceCloudPolicyInitializer::~DeviceCloudPolicyInitializer() {
   DCHECK(!is_initialized_);
@@ -257,7 +263,7 @@ std::unique_ptr<CloudPolicyClient> DeviceCloudPolicyInitializer::CreateClient(
       DeviceCloudPolicyManagerChromeOS::GetMachineID(),
       DeviceCloudPolicyManagerChromeOS::GetMachineModel(),
       kPolicyVerificationKeyHash, device_management_service,
-      g_browser_process->system_request_context(), &signing_service_);
+      g_browser_process->system_request_context(), signing_service_.get());
 }
 
 void DeviceCloudPolicyInitializer::TryToCreateClient() {

chrome/browser/chromeos/policy/device_cloud_policy_initializer.h

@@ -11,6 +11,7 @@
 
 #include "base/callback_forward.h"
 #include "base/compiler_specific.h"
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/chromeos/policy/server_backed_state_keys_broker.h"
@@ -97,8 +98,12 @@ class DeviceCloudPolicyInitializer : public CloudPolicyStore::Observer {
   void OnStoreLoaded(CloudPolicyStore* store) override;
   void OnStoreError(CloudPolicyStore* store) override;
 
+  // Allows testing code to set a signing service tailored to its needs.
+  void SetSigningServiceForTesting(
+      std::unique_ptr<policy::SigningService> signing_service);
+
  private:
-  // Signing class implemting the policy::SigningService interface to
+  // Signing class implementing the policy::SigningService interface to
   // sign data using the enrollment certificate's TPM-bound key.
   class TpmEnrollmentKeySigningService : public policy::SigningService {
    public:
@@ -148,7 +153,7 @@ class DeviceCloudPolicyInitializer : public CloudPolicyStore::Observer {
   ServerBackedStateKeysBroker::Subscription state_keys_update_subscription_;
 
   // Our signing service.
-  TpmEnrollmentKeySigningService signing_service_;
+  std::unique_ptr<SigningService> signing_service_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceCloudPolicyInitializer);
 };

chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc

@@ -46,6 +46,7 @@
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/policy/core/common/cloud/cloud_policy_core.h"
 #include "components/policy/core/common/cloud/mock_device_management_service.h"
+#include "components/policy/core/common/cloud/mock_signing_service.h"
 #include "components/policy/core/common/external_data_fetcher.h"
 #include "components/policy/core/common/policy_types.h"
 #include "components/policy/core/common/schema_registry.h"
@@ -124,7 +125,7 @@ class DeviceCloudPolicyManagerChromeOSTest
     chromeos::system::StatisticsProvider::SetTestProvider(NULL);
   }
 
-  virtual bool ShouldRegisterWitCert() const { return false; }
+  virtual bool ShouldRegisterWithCert() const { return false; }
 
   void SetUp() override {
     DeviceSettingsTestBase::SetUp();
@@ -168,7 +169,7 @@ class DeviceCloudPolicyManagerChromeOSTest
   CreateAttestationFlow() {
     StrictMock<chromeos::attestation::MockAttestationFlow>* mock =
         new StrictMock<chromeos::attestation::MockAttestationFlow>();
-    if (ShouldRegisterWitCert()) {
+    if (ShouldRegisterWithCert()) {
       EXPECT_CALL(*mock, GetCertificate(_, _, _, _, _))
           .WillOnce(WithArgs<4>(Invoke(CertCallbackSuccess)));
     }
@@ -211,6 +212,8 @@ class DeviceCloudPolicyManagerChromeOSTest
         base::ThreadTaskRunnerHandle::Get(), install_attributes_.get(),
         &state_keys_broker_, store_, manager_.get(),
         cryptohome::AsyncMethodCaller::GetInstance(), std::move(unique_flow)));
+    initializer_->SetSigningServiceForTesting(
+        base::MakeUnique<FakeSigningService>());
     initializer_->Init();
   }
 
@@ -394,7 +397,8 @@ TEST_F(DeviceCloudPolicyManagerChromeOSTest, ConnectAndDisconnect) {
 }
 
 class DeviceCloudPolicyManagerChromeOSEnrollmentTest
-    : public DeviceCloudPolicyManagerChromeOSTest {
+    : public DeviceCloudPolicyManagerChromeOSTest,
+      public testing::WithParamInterface<bool> {
  public:
   void Done(EnrollmentStatus status) {
     status_ = status;
@@ -459,7 +463,7 @@ class DeviceCloudPolicyManagerChromeOSEnrollmentTest
   }
 
   void RunTest() {
-    const bool with_cert = ShouldRegisterWitCert();
+    const bool with_cert = ShouldRegisterWithCert();
     // Trigger enrollment.
     MockDeviceManagementJob* register_job = NULL;
     EXPECT_CALL(
@@ -598,6 +602,28 @@ class DeviceCloudPolicyManagerChromeOSEnrollmentTest
     ReloadDeviceSettings();
   }
 
+  bool ShouldRegisterWithCert() const override { return GetParam(); }
+
+  const std::unique_ptr<em::DeviceRegisterRequest> GetDeviceRegisterRequest() {
+    auto req = base::MakeUnique<em::DeviceRegisterRequest>();
+    if (ShouldRegisterWithCert()) {
+      em::CertificateBasedDeviceRegistrationData data;
+      const em::SignedData& signed_request =
+          register_request_.cert_based_register_request().signed_request();
+      EXPECT_TRUE(data.ParseFromString(signed_request.data().substr(
+          0,
+          signed_request.data().size() - signed_request.extra_data_bytes())));
+      EXPECT_EQ(em::CertificateBasedDeviceRegistrationData::
+                    ENTERPRISE_ENROLLMENT_CERTIFICATE,
+                data.certificate_type());
+      req->CopyFrom(data.device_register_request());
+    } else {
+      req->CopyFrom(
+          register_request_.register_request());
+    }
+    return req;
+  }
+
   DeviceManagementStatus register_status_;
   em::DeviceManagementResponse register_response_;
 
@@ -620,43 +646,34 @@ class DeviceCloudPolicyManagerChromeOSEnrollmentTest
   DISALLOW_COPY_AND_ASSIGN(DeviceCloudPolicyManagerChromeOSEnrollmentTest);
 };
 
-// TODO(drcrash): Handle cert-based tests (http://crbug.com/641447).
-TEST_F(DeviceCloudPolicyManagerChromeOSEnrollmentTest, Reenrollment) {
-  LockDevice();
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, Success) {
   RunTest();
   ExpectSuccessfulEnrollment();
-  EXPECT_TRUE(register_request_.register_request().reregister());
-  EXPECT_EQ(PolicyBuilder::kFakeDeviceId, client_id_);
 }
 
-class ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest
-    : public DeviceCloudPolicyManagerChromeOSEnrollmentTest,
-      public testing::WithParamInterface<bool> {
- protected:
-  bool ShouldRegisterWitCert() const override { return GetParam(); }
-};
-
-TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest, Success) {
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, Reenrollment) {
+  LockDevice();
   RunTest();
   ExpectSuccessfulEnrollment();
+  EXPECT_TRUE(GetDeviceRegisterRequest()->reregister());
+  EXPECT_EQ(PolicyBuilder::kFakeDeviceId, client_id_);
 }
 
-TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
-       RegistrationFailed) {
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, RegistrationFailed) {
   register_status_ = DM_STATUS_REQUEST_FAILED;
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_REGISTRATION_FAILED);
   EXPECT_EQ(DM_STATUS_REQUEST_FAILED, status_.client_status());
 }
 
-TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest,
        RobotAuthCodeFetchFailed) {
   robot_auth_fetch_status_ = DM_STATUS_REQUEST_FAILED;
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_ROBOT_AUTH_FETCH_FAILED);
 }
 
-TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest,
        RobotRefreshTokenFetchResponseCodeFailed) {
   url_fetcher_response_code_ = 400;
   RunTest();
@@ -664,14 +681,14 @@ TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
   EXPECT_EQ(400, status_.http_status());
 }
 
-TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest,
        RobotRefreshTokenFetchResponseStringFailed) {
   url_fetcher_response_string_ = "invalid response json";
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_ROBOT_REFRESH_FETCH_FAILED);
 }
 
-TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest,
        RobotRefreshEncryptionFailed) {
   // The encryption lib is a noop for tests, but empty results from encryption
   // is an error, so we simulate an encryption error by returning an empty
@@ -683,16 +700,14 @@ TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_ROBOT_REFRESH_STORE_FAILED);
 }
 
-TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
-       PolicyFetchFailed) {
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, PolicyFetchFailed) {
   policy_fetch_status_ = DM_STATUS_REQUEST_FAILED;
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_POLICY_FETCH_FAILED);
   EXPECT_EQ(DM_STATUS_REQUEST_FAILED, status_.client_status());
 }
 
-TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
-       ValidationFailed) {
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, ValidationFailed) {
   device_policy_.policy().set_policy_data_signature("bad");
   policy_fetch_response_.clear_policy_response();
   policy_fetch_response_.mutable_policy_response()->add_response()->CopyFrom(
@@ -703,8 +718,7 @@ TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
             status_.validation_status());
 }
 
-TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
-       StoreError) {
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, StoreError) {
   store_result_ = false;
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_STORE_ERROR);
@@ -712,7 +726,7 @@ TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
             status_.store_status());
 }
 
-TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest, LoadError) {
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, LoadError) {
   loaded_blob_.clear();
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_STORE_ERROR);
@@ -720,8 +734,7 @@ TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest, LoadError) {
             status_.store_status());
 }
 
-TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
-       UnregisterSucceeds) {
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, UnregisterSucceeds) {
   // Enroll first.
   RunTest();
   ExpectSuccessfulEnrollment();
@@ -741,8 +754,7 @@ TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
       base::Unretained(this)));
 }
 
-TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
-       UnregisterFails) {
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, UnregisterFails) {
   // Enroll first.
   RunTest();
   ExpectSuccessfulEnrollment();
@@ -770,16 +782,20 @@ class DeviceCloudPolicyManagerChromeOSEnrollmentBlankSystemSaltTest
   }
 };
 
-TEST_F(DeviceCloudPolicyManagerChromeOSEnrollmentBlankSystemSaltTest,
+TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentBlankSystemSaltTest,
        RobotRefreshSaveFailed) {
   // Without the system salt, the robot token can't be stored.
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_ROBOT_REFRESH_STORE_FAILED);
 }
 
+INSTANTIATE_TEST_CASE_P(Cert,
+                        DeviceCloudPolicyManagerChromeOSEnrollmentTest,
+                        ::testing::Values(false, true));
+
 INSTANTIATE_TEST_CASE_P(
     Cert,
-    ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
+    DeviceCloudPolicyManagerChromeOSEnrollmentBlankSystemSaltTest,
     ::testing::Values(false, true));
 
 }  // namespace

components/policy/core/common/cloud/cloud_policy_client_unittest.cc

@@ -111,7 +111,7 @@ class CloudPolicyClientTest : public testing::Test {
 
     em::CertificateBasedDeviceRegisterRequest* cert_based_register_request =
         cert_based_registration_request_.mutable_cert_based_register_request();
-    fake_signing_service_.SignRegistrationData(&data,
+    fake_signing_service_.SignDataSynchronously(data.SerializeAsString(),
         cert_based_register_request->mutable_signed_request());
 
     em::PolicyFetchRequest* policy_fetch_request =

components/policy/core/common/cloud/mock_signing_service.cc

@@ -19,22 +19,17 @@ FakeSigningService::FakeSigningService() {}
 
 FakeSigningService::~FakeSigningService() {}
 
-void FakeSigningService::SignRegistrationData(
-      const em::CertificateBasedDeviceRegistrationData* registration_data,
-      em::SignedData* signed_data) {
-    DoSignData(registration_data->SerializeAsString(), signed_data);
-  }
-
 void FakeSigningService::SignData(const std::string& data,
                                   const SigningCallback& callback) {
   em::SignedData signed_data;
-  if (success_)
-    DoSignData(data, &signed_data);
+  if (success_) {
+    SignDataSynchronously(data, &signed_data);
+  }
   callback.Run(success_, signed_data);
 }
 
-void FakeSigningService::DoSignData(const std::string& data,
-                                    em::SignedData* signed_data) {
+void FakeSigningService::SignDataSynchronously(const std::string& data,
+    em::SignedData* signed_data) {
   signed_data->set_data(data + kSignedDataNonce);
   signed_data->set_signature(kSignature);
   signed_data->set_extra_data_bytes(sizeof(kSignedDataNonce) - 1);
@@ -49,4 +44,3 @@ MockSigningService::MockSigningService() {}
 MockSigningService::~MockSigningService() {}
 
 } // namespace policy
-

components/policy/core/common/cloud/mock_signing_service.h

@@ -17,19 +17,17 @@ class FakeSigningService : public SigningService {
   FakeSigningService();
   virtual ~FakeSigningService();
 
-  void SignRegistrationData(
-      const enterprise_management::CertificateBasedDeviceRegistrationData*
-          registration_data,
-      enterprise_management::SignedData* signed_data);
-
   void SignData(const std::string& data, const SigningCallback& callback)
       override;
 
+  // Useful for test setups without having to deal with callbacks.
+  void SignDataSynchronously(const std::string& data,
+      enterprise_management::SignedData* signed_data);
+
+  // Determine whether SignData will appear successful or not.
   void set_success(bool success);
- private:
-  void DoSignData(const std::string& data,
-                         enterprise_management::SignedData* signed_data);
 
+ private:
   bool success_ = true;
 };