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

Revert of All DCP manager tests that enroll/register are tested with all auths. (patchset #7 id:120001 of https://codereview.chromium.org/2287343004/ )

Reason for revert:
Reverting due to use after free. See http://crbug.com/624187#c28

Original issue's description:
> 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
>
> Committed: https://crrev.com/f71058accb8d57221145dfb2c34769244d6cc54d
> Cr-Commit-Position: refs/heads/master@{#415574}

TBR=pastarmovj@chromium.org,drcrash@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=624187,641447

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

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 = nullptr;
+EnterpriseInstallAttributes* g_testing_install_attributes = NULL;
 
 // 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_(nullptr),
-      global_user_cloud_policy_provider_(nullptr),
+    : device_cloud_policy_manager_(NULL),
+      global_user_cloud_policy_provider_(NULL),
       weak_ptr_factory_(this) {
   if (g_testing_install_attributes) {
     install_attributes_.reset(g_testing_install_attributes);
-    g_testing_install_attributes = nullptr;
+    g_testing_install_attributes = NULL;
   }
 
   // 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 = nullptr;
+    g_testing_install_attributes = NULL;
   }
 }
 

chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc

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

chrome/browser/chromeos/policy/device_cloud_policy_initializer.h

@@ -11,7 +11,6 @@
 
 #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"
@@ -98,12 +97,8 @@ 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 implementing the policy::SigningService interface to
+  // Signing class implemting the policy::SigningService interface to
   // sign data using the enrollment certificate's TPM-bound key.
   class TpmEnrollmentKeySigningService : public policy::SigningService {
    public:
@@ -153,7 +148,7 @@ class DeviceCloudPolicyInitializer : public CloudPolicyStore::Observer {
   ServerBackedStateKeysBroker::Subscription state_keys_update_subscription_;
 
   // Our signing service.
-  std::unique_ptr<SigningService> signing_service_;
+  TpmEnrollmentKeySigningService signing_service_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceCloudPolicyInitializer);
 };

chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc

@@ -46,7 +46,6 @@
 #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"
@@ -125,7 +124,7 @@ class DeviceCloudPolicyManagerChromeOSTest
     chromeos::system::StatisticsProvider::SetTestProvider(NULL);
   }
 
-  virtual bool ShouldRegisterWithCert() const { return false; }
+  virtual bool ShouldRegisterWitCert() const { return false; }
 
   void SetUp() override {
     DeviceSettingsTestBase::SetUp();
@@ -169,7 +168,7 @@ class DeviceCloudPolicyManagerChromeOSTest
   CreateAttestationFlow() {
     StrictMock<chromeos::attestation::MockAttestationFlow>* mock =
         new StrictMock<chromeos::attestation::MockAttestationFlow>();
-    if (ShouldRegisterWithCert()) {
+    if (ShouldRegisterWitCert()) {
       EXPECT_CALL(*mock, GetCertificate(_, _, _, _, _))
           .WillOnce(WithArgs<4>(Invoke(CertCallbackSuccess)));
     }
@@ -212,8 +211,6 @@ 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();
   }
 
@@ -397,8 +394,7 @@ TEST_F(DeviceCloudPolicyManagerChromeOSTest, ConnectAndDisconnect) {
 }
 
 class DeviceCloudPolicyManagerChromeOSEnrollmentTest
-    : public DeviceCloudPolicyManagerChromeOSTest,
-      public testing::WithParamInterface<bool> {
+    : public DeviceCloudPolicyManagerChromeOSTest {
  public:
   void Done(EnrollmentStatus status) {
     status_ = status;
@@ -463,7 +459,7 @@ class DeviceCloudPolicyManagerChromeOSEnrollmentTest
   }
 
   void RunTest() {
-    const bool with_cert = ShouldRegisterWithCert();
+    const bool with_cert = ShouldRegisterWitCert();
     // Trigger enrollment.
     MockDeviceManagementJob* register_job = NULL;
     EXPECT_CALL(
@@ -602,25 +598,6 @@ class DeviceCloudPolicyManagerChromeOSEnrollmentTest
     ReloadDeviceSettings();
   }
 
-  bool ShouldRegisterWithCert() const override { return GetParam(); }
-
-  const em::DeviceRegisterRequest& GetDeviceRegisterRequest() {
-    if (ShouldRegisterWithCert()) {
-      const em::SignedData& signed_request =
-          register_request_.cert_based_register_request().signed_request();
-      em::CertificateBasedDeviceRegistrationData data;
-      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());
-      return data.device_register_request();
-    } else {
-      return register_request_.register_request();
-    }
-  }
-
   DeviceManagementStatus register_status_;
   em::DeviceManagementResponse register_response_;
 
@@ -643,34 +620,43 @@ class DeviceCloudPolicyManagerChromeOSEnrollmentTest
   DISALLOW_COPY_AND_ASSIGN(DeviceCloudPolicyManagerChromeOSEnrollmentTest);
 };
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, Success) {
+// TODO(drcrash): Handle cert-based tests (http://crbug.com/641447).
+TEST_F(DeviceCloudPolicyManagerChromeOSEnrollmentTest, Reenrollment) {
+  LockDevice();
   RunTest();
   ExpectSuccessfulEnrollment();
+  EXPECT_TRUE(register_request_.register_request().reregister());
+  EXPECT_EQ(PolicyBuilder::kFakeDeviceId, client_id_);
 }
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, Reenrollment) {
-  LockDevice();
+class ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest
+    : public DeviceCloudPolicyManagerChromeOSEnrollmentTest,
+      public testing::WithParamInterface<bool> {
+ protected:
+  bool ShouldRegisterWitCert() const override { return GetParam(); }
+};
+
+TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest, Success) {
   RunTest();
   ExpectSuccessfulEnrollment();
-  EXPECT_TRUE(GetDeviceRegisterRequest().reregister());
-  EXPECT_EQ(PolicyBuilder::kFakeDeviceId, client_id_);
 }
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, RegistrationFailed) {
+TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
+       RegistrationFailed) {
   register_status_ = DM_STATUS_REQUEST_FAILED;
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_REGISTRATION_FAILED);
   EXPECT_EQ(DM_STATUS_REQUEST_FAILED, status_.client_status());
 }
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest,
+TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
        RobotAuthCodeFetchFailed) {
   robot_auth_fetch_status_ = DM_STATUS_REQUEST_FAILED;
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_ROBOT_AUTH_FETCH_FAILED);
 }
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest,
+TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
        RobotRefreshTokenFetchResponseCodeFailed) {
   url_fetcher_response_code_ = 400;
   RunTest();
@@ -678,14 +664,14 @@ TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest,
   EXPECT_EQ(400, status_.http_status());
 }
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest,
+TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
        RobotRefreshTokenFetchResponseStringFailed) {
   url_fetcher_response_string_ = "invalid response json";
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_ROBOT_REFRESH_FETCH_FAILED);
 }
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest,
+TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
        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
@@ -697,14 +683,16 @@ TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest,
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_ROBOT_REFRESH_STORE_FAILED);
 }
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, PolicyFetchFailed) {
+TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
+       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(DeviceCloudPolicyManagerChromeOSEnrollmentTest, ValidationFailed) {
+TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
+       ValidationFailed) {
   device_policy_.policy().set_policy_data_signature("bad");
   policy_fetch_response_.clear_policy_response();
   policy_fetch_response_.mutable_policy_response()->add_response()->CopyFrom(
@@ -715,7 +703,8 @@ TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, ValidationFailed) {
             status_.validation_status());
 }
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, StoreError) {
+TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
+       StoreError) {
   store_result_ = false;
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_STORE_ERROR);
@@ -723,7 +712,7 @@ TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, StoreError) {
             status_.store_status());
 }
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, LoadError) {
+TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest, LoadError) {
   loaded_blob_.clear();
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STATUS_STORE_ERROR);
@@ -731,7 +720,8 @@ TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, LoadError) {
             status_.store_status());
 }
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, UnregisterSucceeds) {
+TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
+       UnregisterSucceeds) {
   // Enroll first.
   RunTest();
   ExpectSuccessfulEnrollment();
@@ -751,7 +741,8 @@ TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, UnregisterSucceeds) {
       base::Unretained(this)));
 }
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, UnregisterFails) {
+TEST_P(ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
+       UnregisterFails) {
   // Enroll first.
   RunTest();
   ExpectSuccessfulEnrollment();
@@ -779,20 +770,16 @@ class DeviceCloudPolicyManagerChromeOSEnrollmentBlankSystemSaltTest
   }
 };
 
-TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentBlankSystemSaltTest,
+TEST_F(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,
-    DeviceCloudPolicyManagerChromeOSEnrollmentBlankSystemSaltTest,
+    ParameterizedDeviceCloudPolicyManagerChromeOSEnrollmentTest,
     ::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_.SignDataSynchronously(data.SerializeAsString(),
+    fake_signing_service_.SignRegistrationData(&data,
         cert_based_register_request->mutable_signed_request());
 
     em::PolicyFetchRequest* policy_fetch_request =

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

@@ -19,17 +19,22 @@ 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_) {
-    SignDataSynchronously(data, &signed_data);
-  }
+  if (success_)
+    DoSignData(data, &signed_data);
   callback.Run(success_, signed_data);
 }
 
-void FakeSigningService::SignDataSynchronously(const std::string& data,
-    em::SignedData* signed_data) {
+void FakeSigningService::DoSignData(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);
@@ -44,3 +49,4 @@ MockSigningService::MockSigningService() {}
 MockSigningService::~MockSigningService() {}
 
 } // namespace policy
+

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

@@ -17,17 +17,19 @@ 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);
+
   bool success_ = true;
 };