use AttestationClient to get enrollment id.

We are deprecating attestation methods by CryptohomeClient.

BUG=b:158955123
TEST=unit_tests.

Change-Id: I4a046aa0c11b720ecb1d6fee99afdba025234537
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2497390
Commit-Queue: Leo Lai <cylai@google.com>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#821226}

chrome/browser/chromeos/attestation/enrollment_policy_observer.cc

@@ -18,7 +18,8 @@
 #include "chrome/browser/chromeos/attestation/attestation_key_payload.pb.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
-#include "chromeos/dbus/cryptohome/cryptohome_client.h"
+#include "chromeos/dbus/attestation/attestation_client.h"
+#include "chromeos/dbus/attestation/interface.pb.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "components/account_id/account_id.h"
@@ -36,26 +37,6 @@ namespace {
 const int kRetryDelay = 5;  // Seconds.
 const int kRetryLimit = 100;
 
-// A dbus callback which handles a string result.
-//
-// Parameters
-//   on_success - Called when result is successful and has a value.
-//   on_failure - Called otherwise.
-void DBusStringCallback(
-    base::OnceCallback<void(const std::string&)> on_success,
-    base::OnceClosure on_failure,
-    const base::Location& from_here,
-    base::Optional<chromeos::CryptohomeClient::TpmAttestationDataResult>
-        result) {
-  if (!result.has_value() || !result->success) {
-    LOG(ERROR) << "Cryptohome DBus method failed: " << from_here.ToString();
-    if (!on_failure.is_null())
-      std::move(on_failure).Run();
-    return;
-  }
-  std::move(on_success).Run(result->data);
-}
-
 }  // namespace
 
 namespace chromeos {
@@ -65,7 +46,6 @@ EnrollmentPolicyObserver::EnrollmentPolicyObserver(
     policy::CloudPolicyClient* policy_client)
     : device_settings_service_(DeviceSettingsService::Get()),
       policy_client_(policy_client),
-      cryptohome_client_(nullptr),
       num_retries_(0),
       retry_limit_(kRetryLimit),
       retry_delay_(kRetryDelay) {
@@ -76,11 +56,9 @@ EnrollmentPolicyObserver::EnrollmentPolicyObserver(
 
 EnrollmentPolicyObserver::EnrollmentPolicyObserver(
     policy::CloudPolicyClient* policy_client,
-    DeviceSettingsService* device_settings_service,
-    CryptohomeClient* cryptohome_client)
+    DeviceSettingsService* device_settings_service)
     : device_settings_service_(device_settings_service),
       policy_client_(policy_client),
-      cryptohome_client_(cryptohome_client),
       num_retries_(0),
       retry_delay_(kRetryDelay) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -126,33 +104,32 @@ void EnrollmentPolicyObserver::Start() {
     return;
   request_in_flight_ = true;
 
-  if (!cryptohome_client_)
-    cryptohome_client_ = CryptohomeClient::Get();
-
   GetEnrollmentId();
 }
 
 void EnrollmentPolicyObserver::GetEnrollmentId() {
-  cryptohome_client_->TpmAttestationGetEnrollmentId(
-      true /* ignore_cache */,
-      base::BindOnce(
-          DBusStringCallback,
-          base::BindOnce(&EnrollmentPolicyObserver::HandleEnrollmentId,
-                         weak_factory_.GetWeakPtr()),
-          base::BindOnce(&EnrollmentPolicyObserver::RescheduleGetEnrollmentId,
-                         weak_factory_.GetWeakPtr()),
-          FROM_HERE));
+  ::attestation::GetEnrollmentIdRequest request;
+  request.set_ignore_cache(true);
+  AttestationClient::Get()->GetEnrollmentId(
+      request, base::BindOnce(&EnrollmentPolicyObserver::OnGetEnrollmentId,
+                              weak_factory_.GetWeakPtr()));
 }
 
-void EnrollmentPolicyObserver::HandleEnrollmentId(
-    const std::string& enrollment_id) {
-  if (enrollment_id.empty()) {
+void EnrollmentPolicyObserver::OnGetEnrollmentId(
+    const ::attestation::GetEnrollmentIdReply& reply) {
+  if (reply.status() != ::attestation::STATUS_SUCCESS) {
+    LOG(WARNING) << "Failed to get enrollment id: " << reply.status();
+    RescheduleGetEnrollmentId();
+    return;
+  }
+  if (reply.enrollment_id().empty()) {
     LOG(WARNING) << "EnrollmentPolicyObserver: The enrollment identifier"
                     " obtained is empty.";
   }
   policy_client_->UploadEnterpriseEnrollmentId(
-      enrollment_id, base::BindOnce(&EnrollmentPolicyObserver::OnUploadComplete,
-                                    weak_factory_.GetWeakPtr(), enrollment_id));
+      reply.enrollment_id(),
+      base::BindOnce(&EnrollmentPolicyObserver::OnUploadComplete,
+                     weak_factory_.GetWeakPtr(), reply.enrollment_id()));
 }
 
 void EnrollmentPolicyObserver::RescheduleGetEnrollmentId() {

chrome/browser/chromeos/attestation/enrollment_policy_observer.h

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
+#include "chromeos/dbus/attestation/interface.pb.h"
 #include "chromeos/dbus/constants/attestation_constants.h"
 
 namespace policy {
@@ -19,9 +20,6 @@ class CloudPolicyClient;
 }
 
 namespace chromeos {
-
-class CryptohomeClient;
-
 namespace attestation {
 
 // A class which observes policy changes and triggers uploading identification
@@ -36,8 +34,7 @@ class EnrollmentPolicyObserver : public DeviceSettingsService::Observer {
 
   // A constructor which accepts custom instances useful for testing.
   EnrollmentPolicyObserver(policy::CloudPolicyClient* policy_client,
-                           DeviceSettingsService* device_settings_service,
-                           CryptohomeClient* cryptohome_client);
+                           DeviceSettingsService* device_settings_service);
 
   ~EnrollmentPolicyObserver() override;
 
@@ -56,8 +53,8 @@ class EnrollmentPolicyObserver : public DeviceSettingsService::Observer {
   // Gets an enrollment identifier directly.
   void GetEnrollmentId();
 
-  // Handles an enrollment identifer obtained directly.
-  void HandleEnrollmentId(const std::string& enrollment_id);
+  // Handles an enrollment identifier obtained directly.
+  void OnGetEnrollmentId(const ::attestation::GetEnrollmentIdReply& reply);
 
   // Reschedule an attempt to get an enrollment identifier directly.
   void RescheduleGetEnrollmentId();
@@ -69,7 +66,6 @@ class EnrollmentPolicyObserver : public DeviceSettingsService::Observer {
 
   DeviceSettingsService* device_settings_service_;
   policy::CloudPolicyClient* policy_client_;
-  CryptohomeClient* cryptohome_client_;
   int num_retries_;
   int retry_limit_;
   int retry_delay_;

chrome/browser/chromeos/attestation/enrollment_policy_observer_unittest.cc

@@ -18,7 +18,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chromeos/attestation/enrollment_policy_observer.h"
 #include "chrome/browser/chromeos/settings/device_settings_test_helper.h"
-#include "chromeos/dbus/cryptohome/fake_cryptohome_client.h"
+#include "chromeos/dbus/attestation/fake_attestation_client.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -34,47 +34,13 @@ namespace attestation {
 
 namespace {
 
+constexpr int kRetryLimit = 3;
+
 void StatusCallbackSuccess(policy::CloudPolicyClient::StatusCallback callback) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), true));
 }
 
-// A FakeCryptohomeClient that can hold call until told to flush them all.
-class CallsHoldingFakeCryptohomeClient : public FakeCryptohomeClient {
- public:
-  void set_hold_calls(bool hold_calls) { hold_calls_ = hold_calls; }
-
-  void TpmAttestationGetEnrollmentId(
-      bool ignore_cache,
-      DBusMethodCallback<TpmAttestationDataResult> callback) override {
-    if (hold_calls_) {
-      held_calls_.push(base::BindOnce(
-          &CallsHoldingFakeCryptohomeClient::DoTpmAttestationGetEnrollmentId,
-          base::Unretained(this), ignore_cache, std::move(callback)));
-    } else {
-      DoTpmAttestationGetEnrollmentId(ignore_cache, std::move(callback));
-    }
-  }
-
-  void FlushCalls() {
-    while (!held_calls_.empty()) {
-      std::move(held_calls_.front()).Run();
-      held_calls_.pop();
-    }
-  }
-
- private:
-  void DoTpmAttestationGetEnrollmentId(
-      bool ignore_cache,
-      DBusMethodCallback<TpmAttestationDataResult> callback) {
-    FakeCryptohomeClient::TpmAttestationGetEnrollmentId(ignore_cache,
-                                                        std::move(callback));
-  }
-
-  bool hold_calls_ = false;
-  std::queue<base::OnceClosure> held_calls_;
-};
-
 }  // namespace
 
 class EnrollmentPolicyObserverTest : public DeviceSettingsTestBase {
@@ -83,23 +49,24 @@ class EnrollmentPolicyObserverTest : public DeviceSettingsTestBase {
   ~EnrollmentPolicyObserverTest() override = default;
 
   void SetUp() override {
+    AttestationClient::InitializeFake();
     DeviceSettingsTestBase::SetUp();
 
     policy_client_.SetDMToken("fake_dm_token");
 
     EXPECT_TRUE(base::HexStringToString(kEnrollmentId, &enrollment_id_));
 
-    // Destroy the DeviceSettingsTestBase fake client and replace it.
-    CryptohomeClient::Shutdown();
-    // This will be destroyed in DeviceSettingsTestBase::TearDown().
-    cryptohome_client_ = new CallsHoldingFakeCryptohomeClient();
-    cryptohome_client_->set_tpm_attestation_enrollment_id(
-        true /* ignore_cache */, enrollment_id_);
+    AttestationClient::Get()
+        ->GetTestInterface()
+        ->set_enrollment_id_ignore_cache(enrollment_id_);
+    AttestationClient::Get()->GetTestInterface()->set_cached_enrollment_id(
+        "unexpeted query to cached enrollment id");
   }
 
   void TearDown() override {
     observer_.reset();
     DeviceSettingsTestBase::TearDown();
+    AttestationClient::Shutdown();
   }
 
  protected:
@@ -108,12 +75,16 @@ class EnrollmentPolicyObserverTest : public DeviceSettingsTestBase {
 
   void SetUpObserver() {
     observer_ = std::make_unique<EnrollmentPolicyObserver>(
-        &policy_client_, device_settings_service_.get(), cryptohome_client_);
-    observer_->set_retry_limit(3);
+        &policy_client_, device_settings_service_.get());
+    observer_->set_retry_limit(kRetryLimit);
     observer_->set_retry_delay(0);
   }
 
   void ExpectUploadEnterpriseEnrollmentId(int times) {
+    // Setting a mock behavior with 0 times causes warnings.
+    if (times == 0) {
+      return;
+    }
     EXPECT_CALL(policy_client_, UploadEnterpriseEnrollmentId(enrollment_id_, _))
         .Times(times)
         .WillRepeatedly(WithArgs<1>(Invoke(StatusCallbackSuccess)));
@@ -133,9 +104,6 @@ class EnrollmentPolicyObserverTest : public DeviceSettingsTestBase {
     base::RunLoop().RunUntilIdle();
   }
 
-  // Owned by the global instance, shut down in DeviceSettingsTestBase.
-  CallsHoldingFakeCryptohomeClient* cryptohome_client_ = nullptr;
-
   StrictMock<policy::MockCloudPolicyClient> policy_client_;
   std::unique_ptr<EnrollmentPolicyObserver> observer_;
   std::string enrollment_id_;
@@ -166,20 +134,6 @@ TEST_F(EnrollmentPolicyObserverTest,
   Run();
 }
 
-TEST_F(EnrollmentPolicyObserverTest,
-       UploadEnterpriseEnrollmentIdWithDelayedCallbacks) {
-  // We hold calls to cryptohome so that one is still pending by the time the
-  // observer gets notified. We expect only one upload despite the concurrent
-  // calls.
-  cryptohome_client_->set_hold_calls(true);
-  SetUpDevicePolicy(true);
-  ExpectUploadEnterpriseEnrollmentId(1);
-  PropagateDevicePolicy();
-  SetUpObserver();
-  cryptohome_client_->FlushCalls();
-  Run();
-}
-
 TEST_F(EnrollmentPolicyObserverTest, FeatureDisabled) {
   SetUpDevicePolicy(false);
   SetUpObserver();
@@ -196,8 +150,9 @@ TEST_F(EnrollmentPolicyObserverTest, UnregisteredPolicyClient) {
 }
 
 TEST_F(EnrollmentPolicyObserverTest, DBusFailureRetry) {
-  // Simulate a DBus failure.
-  cryptohome_client_->SetServiceIsAvailable(false);
+  AttestationClient::Get()
+      ->GetTestInterface()
+      ->set_enrollment_id_dbus_error_count(kRetryLimit - 1);
 
   ExpectUploadEnterpriseEnrollmentId(1);
 
@@ -205,16 +160,19 @@ TEST_F(EnrollmentPolicyObserverTest, DBusFailureRetry) {
   PropagateDevicePolicy();
   SetUpObserver();
 
-  // Emulate delayed service initialization.
-  // The observer we create synchronously calls TpmAttestationGetEnrollmentId()
-  // and fails. During this call, we make the service available in the next
-  // run, so on retry, it will successfully return the result.
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(
-                     [](FakeCryptohomeClient* cryptohome_client) {
-                       cryptohome_client->SetServiceIsAvailable(true);
-                     },
-                     base::Unretained(cryptohome_client_)));
+  Run();
+}
+
+TEST_F(EnrollmentPolicyObserverTest, DBusFailureRetryUntilLimit) {
+  AttestationClient::Get()
+      ->GetTestInterface()
+      ->set_enrollment_id_dbus_error_count(kRetryLimit);
+
+  ExpectUploadEnterpriseEnrollmentId(0);
+
+  SetUpDevicePolicy(true);
+  PropagateDevicePolicy();
+  SetUpObserver();
 
   Run();
 }

chromeos/dbus/attestation/attestation_client.h

@@ -118,6 +118,14 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_ATTESTATION) AttestationClient {
 
     // Clears the request history of `DeleteKeys()`.
     virtual void ClearDeleteKeysHistory() = 0;
+
+    // Sets returned enrollment ids, when ignoring/not ignoring cache,
+    // respectively.
+    virtual void set_enrollment_id_ignore_cache(const std::string& id) = 0;
+    virtual void set_cached_enrollment_id(const std::string& id) = 0;
+    // Sets the returned status of `GetEnrollmentId()` to be D-Bus error for
+    // `count` times to emulate D-Bus late availability.
+    virtual void set_enrollment_id_dbus_error_count(int count) = 0;
   };
 
   // Not copyable or movable.

chromeos/dbus/attestation/fake_attestation_client.cc

@@ -249,7 +249,16 @@ void FakeAttestationClient::ResetIdentity(
 void FakeAttestationClient::GetEnrollmentId(
     const ::attestation::GetEnrollmentIdRequest& request,
     GetEnrollmentIdCallback callback) {
-  NOTIMPLEMENTED();
+  ::attestation::GetEnrollmentIdReply reply;
+  if (enrollment_id_dbus_error_count_ != 0) {
+    reply.set_status(::attestation::STATUS_DBUS_ERROR);
+    enrollment_id_dbus_error_count_--;
+  } else {
+    reply.set_status(::attestation::STATUS_SUCCESS);
+    reply.set_enrollment_id(request.ignore_cache() ? enrollment_id_ignore_cache_
+                                                   : enrollment_id_);
+  }
+  PostProtoResponse(std::move(callback), reply);
 }
 
 void FakeAttestationClient::GetCertifiedNvIndex(
@@ -308,6 +317,19 @@ void FakeAttestationClient::ClearDeleteKeysHistory() {
   delete_keys_history_.clear();
 }
 
+void FakeAttestationClient::set_enrollment_id_ignore_cache(
+    const std::string& id) {
+  enrollment_id_ignore_cache_ = id;
+}
+
+void FakeAttestationClient::set_cached_enrollment_id(const std::string& id) {
+  enrollment_id_ = id;
+}
+
+void FakeAttestationClient::set_enrollment_id_dbus_error_count(int count) {
+  enrollment_id_dbus_error_count_ = count;
+}
+
 AttestationClient::TestInterface* FakeAttestationClient::GetTestInterface() {
   return this;
 }

chromeos/dbus/attestation/fake_attestation_client.h

@@ -8,6 +8,7 @@
 #include "chromeos/dbus/attestation/attestation_client.h"
 
 #include <deque>
+#include <string>
 #include <vector>
 
 #include "base/component_export.h"
@@ -109,6 +110,9 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_ATTESTATION) FakeAttestationClient
   const std::vector<::attestation::DeleteKeysRequest>& delete_keys_history()
       const override;
   void ClearDeleteKeysHistory() override;
+  void set_enrollment_id_ignore_cache(const std::string& id) override;
+  void set_cached_enrollment_id(const std::string& id) override;
+  void set_enrollment_id_dbus_error_count(int count) override;
 
   AttestationClient::TestInterface* GetTestInterface() override;
 
@@ -136,6 +140,11 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_ATTESTATION) FakeAttestationClient
 
   // Maintains the input request history of `DeleteKeys()`.
   std::vector<::attestation::DeleteKeysRequest> delete_keys_history_;
+
+  // Maintains building components reply to `GetEnrollmentId()`.
+  std::string enrollment_id_;
+  std::string enrollment_id_ignore_cache_;
+  int enrollment_id_dbus_error_count_ = 0;
 };
 
 }  // namespace chromeos