Use Result for certificate uploads

Replace StatusCallback with ResultCallback for all certificate uploads.
This will allow to eliminate StatusCallback eventually.

      unit_tests
        *ActiveDirectoryDeviceStateUploaderTest.EnrollmentIdUploadFails*
        *EnrollmentCertificateUploaderTest*

Test: components_unittests *CloudPolicyClient*
Bug: b:256553070
Change-Id: I08224369a0f9363dd549852805830169a99a7a34
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4206769
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Artem Sumaneev <asumaneev@google.com>
Commit-Queue: Miriam Polzer <mpolzer@google.com>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1103287}

chrome/browser/ash/attestation/enrollment_certificate_uploader_impl.cc

@@ -194,8 +194,9 @@ void EnrollmentCertificateUploaderImpl::UploadCertificateIfNeeded(
                      weak_factory_.GetWeakPtr()));
 }
 
-void EnrollmentCertificateUploaderImpl::OnUploadComplete(bool status) {
-  if (status) {
+void EnrollmentCertificateUploaderImpl::OnUploadComplete(
+    policy::CloudPolicyClient::Result result) {
+  if (result.IsSuccess()) {
     has_already_uploaded_ = true;
     if (num_retries_ != 0) {
       LOG(WARNING) << "Enterprise Enrollment Certificate uploaded to DMServer "

chrome/browser/ash/attestation/enrollment_certificate_uploader_impl.h

@@ -14,10 +14,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/ash/attestation/enrollment_certificate_uploader.h"
 #include "chromeos/ash/components/dbus/constants/attestation_constants.h"
-
-namespace policy {
-class CloudPolicyClient;
-}  // namespace policy
+#include "components/policy/core/common/cloud/cloud_policy_client.h"
 
 namespace ash {
 namespace attestation {
@@ -79,9 +76,8 @@ class EnrollmentCertificateUploaderImpl : public EnrollmentCertificateUploader {
   // upload.
   void UploadCertificateIfNeeded(const std::string& pem_certificate_chain);
 
-  // Called when a certificate upload operation completes. On success, |status|
-  // will be true.
-  void OnUploadComplete(bool status);
+  // Called when a certificate upload operation completes.
+  void OnUploadComplete(policy::CloudPolicyClient::Result result);
 
   // Reschedules certificate upload from |Start()| checkpoint and returns true.
   // If |retry_limit_| is exceeded, does not reschedule and returns false.

chrome/browser/ash/attestation/enrollment_certificate_uploader_impl_unittest.cc

@@ -57,14 +57,18 @@ void CertCallbackBadRequestFailure(CertCallback callback) {
                                 ATTESTATION_SERVER_BAD_REQUEST_FAILURE, ""));
 }
 
-void StatusCallbackFailure(policy::CloudPolicyClient::StatusCallback callback) {
+void ResultCallbackFailure(policy::CloudPolicyClient::ResultCallback callback) {
   base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), false));
+      FROM_HERE, base::BindOnce(std::move(callback),
+                                policy::CloudPolicyClient::Result(
+                                    policy::DM_STATUS_TEMPORARY_UNAVAILABLE)));
 }
 
-void StatusCallbackSuccess(policy::CloudPolicyClient::StatusCallback callback) {
+void ResultCallbackSuccess(policy::CloudPolicyClient::ResultCallback callback) {
   base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), true));
+      FROM_HERE,
+      base::BindOnce(std::move(callback), policy::CloudPolicyClient::Result(
+                                              policy::DM_STATUS_SUCCESS)));
 }
 
 }  // namespace
@@ -176,7 +180,7 @@ TEST_F(EnrollmentCertificateUploaderTest, UploadCertificateFailure) {
     EXPECT_CALL(policy_client_,
                 UploadEnterpriseEnrollmentCertificate(valid_certificate, _))
         .Times(1)
-        .WillOnce(WithArgs<1>(Invoke(StatusCallbackFailure)));
+        .WillOnce(WithArgs<1>(Invoke(ResultCallbackFailure)));
   }
 
   Run(/*expected_status=*/CertStatus::kFailedToUpload);
@@ -200,9 +204,9 @@ TEST_F(EnrollmentCertificateUploaderTest,
               UploadEnterpriseEnrollmentCertificate(valid_certificate, _))
       .Times(1)
       .WillOnce(WithArgs<1>(
-          Invoke([this](policy::CloudPolicyClient::StatusCallback callback) {
+          Invoke([this](policy::CloudPolicyClient::ResultCallback callback) {
             policy_client_.SetDMToken("");
-            StatusCallbackFailure(std::move(callback));
+            ResultCallbackFailure(std::move(callback));
           })));
 
   EXPECT_CALL(attestation_flow_,
@@ -272,7 +276,7 @@ TEST_F(EnrollmentCertificateUploaderTest, UploadValidCertificate) {
   EXPECT_CALL(policy_client_,
               UploadEnterpriseEnrollmentCertificate(valid_certificate, _))
       .Times(1)
-      .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
+      .WillOnce(WithArgs<1>(Invoke(ResultCallbackSuccess)));
 
   Run(/*expected_status=*/CertStatus::kSuccess);
 }
@@ -305,7 +309,7 @@ TEST_F(EnrollmentCertificateUploaderTest,
   EXPECT_CALL(policy_client_,
               UploadEnterpriseEnrollmentCertificate(valid_certificate, _))
       .Times(1)
-      .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
+      .WillOnce(WithArgs<1>(Invoke(ResultCallbackSuccess)));
 
   Run(/*expected_status=*/CertStatus::kSuccess);
 }
@@ -338,7 +342,7 @@ TEST_F(EnrollmentCertificateUploaderTest,
   EXPECT_CALL(policy_client_,
               UploadEnterpriseEnrollmentCertificate(valid_certificate, _))
       .Times(1)
-      .WillOnce(WithArgs<1>(Invoke(StatusCallbackFailure)));
+      .WillOnce(WithArgs<1>(Invoke(ResultCallbackFailure)));
   // After upload failure, shall fetch existing certificate.
   for (int i = 0; i < kRetryLimit; ++i) {
     // Cannot use Times(kRetryLimit) because of expected sequence.
@@ -353,7 +357,7 @@ TEST_F(EnrollmentCertificateUploaderTest,
     EXPECT_CALL(policy_client_,
                 UploadEnterpriseEnrollmentCertificate(valid_certificate, _))
         .Times(1)
-        .WillOnce(WithArgs<1>(Invoke(StatusCallbackFailure)));
+        .WillOnce(WithArgs<1>(Invoke(ResultCallbackFailure)));
   }
 
   Run(/*expected_status=*/CertStatus::kFailedToUpload);
@@ -378,7 +382,7 @@ TEST_F(EnrollmentCertificateUploaderTest,
     EXPECT_CALL(policy_client_,
                 UploadEnterpriseEnrollmentCertificate(empty_certificate, _))
         .Times(1)
-        .WillOnce(WithArgs<1>(Invoke(StatusCallbackFailure)));
+        .WillOnce(WithArgs<1>(Invoke(ResultCallbackFailure)));
   }
 
   Run(/*expected_status=*/CertStatus::kFailedToUpload);
@@ -399,7 +403,7 @@ TEST_F(EnrollmentCertificateUploaderTest, UploadValidCertificateOnlyOnce) {
   EXPECT_CALL(policy_client_,
               UploadEnterpriseEnrollmentCertificate(valid_certificate, _))
       .Times(1)
-      .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
+      .WillOnce(WithArgs<1>(Invoke(ResultCallbackSuccess)));
 
   Run(/*expected_status=*/CertStatus::kSuccess);
 

chrome/browser/ash/attestation/enrollment_id_upload_manager.cc

@@ -188,11 +188,11 @@ void EnrollmentIdUploadManager::RescheduleGetEnrollmentId() {
 
 void EnrollmentIdUploadManager::OnUploadComplete(
     const std::string& enrollment_id,
-    bool status) {
+    policy::CloudPolicyClient::Result result) {
   const std::string& printable_enrollment_id = base::ToLowerASCII(
       base::HexEncode(enrollment_id.data(), enrollment_id.size()));
 
-  if (!status) {
+  if (!result.IsSuccess()) {
     LOG(ERROR) << "Failed to upload Enrollment Identifier \""
                << printable_enrollment_id << "\" to DMServer.";
     RunCallbacks(/*status=*/false);

chrome/browser/ash/attestation/enrollment_id_upload_manager.h

@@ -15,10 +15,7 @@
 #include "chrome/browser/ash/settings/device_settings_service.h"
 #include "chromeos/ash/components/dbus/attestation/interface.pb.h"
 #include "chromeos/ash/components/dbus/constants/attestation_constants.h"
-
-namespace policy {
-class CloudPolicyClient;
-}  // namespace policy
+#include "components/policy/core/common/cloud/cloud_policy_client.h"
 
 namespace ash {
 namespace attestation {
@@ -90,9 +87,10 @@ class EnrollmentIdUploadManager : public DeviceSettingsService::Observer {
   void RescheduleGetEnrollmentId();
 
   // Called when an enrollment identifier upload operation completes.
-  // On success, |status| will be true. The string |enrollment_id| contains
+  // On success, |result| will be true. The string |enrollment_id| contains
   // the enrollment identifier that was uploaded.
-  void OnUploadComplete(const std::string& enrollment_id, bool status);
+  void OnUploadComplete(const std::string& enrollment_id,
+                        policy::CloudPolicyClient::Result result);
 
   // Run all callbacks with |status|.
   void RunCallbacks(bool status);

chrome/browser/ash/attestation/enrollment_id_upload_manager_unittest.cc

@@ -37,9 +37,11 @@ using ::testing::WithArgs;
 
 constexpr int kRetryLimit = 3;
 
-void StatusCallbackSuccess(policy::CloudPolicyClient::StatusCallback callback) {
+void ResultCallbackSuccess(policy::CloudPolicyClient::ResultCallback callback) {
   base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), true));
+      FROM_HERE,
+      base::BindOnce(std::move(callback), policy::CloudPolicyClient::Result(
+                                              policy::DM_STATUS_SUCCESS)));
 }
 
 }  // namespace
@@ -107,7 +109,7 @@ class EnrollmentIdUploadManagerTest : public DeviceSettingsTestBase {
     }
     EXPECT_CALL(policy_client_, UploadEnterpriseEnrollmentId(enrollment_id_, _))
         .Times(times)
-        .WillRepeatedly(WithArgs<1>(Invoke(StatusCallbackSuccess)));
+        .WillRepeatedly(WithArgs<1>(Invoke(ResultCallbackSuccess)));
   }
 
   void SetUpDevicePolicy(bool enrollment_id_needed) {

chrome/browser/ash/attestation/machine_certificate_uploader_impl.cc

@@ -233,8 +233,9 @@ void MachineCertificateUploaderImpl::CheckIfUploaded(
   UploadCertificate(reply.certificate());
 }
 
-void MachineCertificateUploaderImpl::OnUploadComplete(bool status) {
-  if (status) {
+void MachineCertificateUploaderImpl::OnUploadComplete(
+    policy::CloudPolicyClient::Result result) {
+  if (result.IsSuccess()) {
     VLOG(1) << "Enterprise Machine Certificate uploaded to DMServer.";
     ::attestation::GetKeyInfoRequest request;
     request.set_username("");
@@ -243,7 +244,7 @@ void MachineCertificateUploaderImpl::OnUploadComplete(bool status) {
         request, base::BindOnce(&MachineCertificateUploaderImpl::MarkAsUploaded,
                                 weak_factory_.GetWeakPtr()));
   }
-  certificate_uploaded_ = status;
+  certificate_uploaded_ = result.IsSuccess();
   RunCallbacks(certificate_uploaded_.value());
 }
 

chrome/browser/ash/attestation/machine_certificate_uploader_impl.h

@@ -14,12 +14,9 @@
 #include "chrome/browser/ash/attestation/machine_certificate_uploader.h"
 #include "chromeos/ash/components/dbus/attestation/interface.pb.h"
 #include "chromeos/ash/components/dbus/constants/attestation_constants.h"
+#include "components/policy/core/common/cloud/cloud_policy_client.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 
-namespace policy {
-class CloudPolicyClient;
-}  // namespace policy
-
 namespace ash {
 namespace attestation {
 
@@ -90,9 +87,8 @@ class MachineCertificateUploaderImpl : public MachineCertificateUploader {
   // upload.
   void CheckIfUploaded(const ::attestation::GetKeyInfoReply& reply);
 
-  // Called when a certificate upload operation completes.  On success, |status|
-  // will be true.
-  void OnUploadComplete(bool status);
+  // Called when a certificate upload operation completes.
+  void OnUploadComplete(policy::CloudPolicyClient::Result result);
 
   // Marks a key as uploaded in the payload proto.
   void MarkAsUploaded(const ::attestation::GetKeyInfoReply& reply);

chrome/browser/ash/attestation/machine_certificate_uploader_impl_unittest.cc

@@ -62,9 +62,11 @@ void CertCallbackNotAvailableFailure(
       base::BindOnce(std::move(callback), ATTESTATION_NOT_AVAILABLE, ""));
 }
 
-void StatusCallbackSuccess(policy::CloudPolicyClient::StatusCallback callback) {
+void ResultCallbackSuccess(policy::CloudPolicyClient::ResultCallback callback) {
   base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), true));
+      FROM_HERE,
+      base::BindOnce(std::move(callback), policy::CloudPolicyClient::Result(
+                                              policy::DM_STATUS_SUCCESS)));
 }
 
 class CallbackObserver {
@@ -167,7 +169,7 @@ class MachineCertificateUploaderTestBase : public ::testing::Test {
       EXPECT_CALL(policy_client_,
                   UploadEnterpriseMachineCertificate(
                       new_key ? kFakeCertificate : certificate, _))
-          .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
+          .WillOnce(WithArgs<1>(Invoke(ResultCallbackSuccess)));
     }
 
     // Setup expected key generations.  Again use WillOnce().  Key generation is

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

@@ -551,7 +551,7 @@ void CloudPolicyClient::FetchRobotAuthCodes(
 
 void CloudPolicyClient::UploadEnterpriseMachineCertificate(
     const std::string& certificate_data,
-    CloudPolicyClient::StatusCallback callback) {
+    CloudPolicyClient::ResultCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   UploadCertificate(certificate_data,
@@ -561,7 +561,7 @@ void CloudPolicyClient::UploadEnterpriseMachineCertificate(
 
 void CloudPolicyClient::UploadEnterpriseEnrollmentCertificate(
     const std::string& certificate_data,
-    CloudPolicyClient::StatusCallback callback) {
+    CloudPolicyClient::ResultCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   UploadCertificate(
@@ -572,7 +572,7 @@ void CloudPolicyClient::UploadEnterpriseEnrollmentCertificate(
 
 void CloudPolicyClient::UploadEnterpriseEnrollmentId(
     const std::string& enrollment_id,
-    CloudPolicyClient::StatusCallback callback) {
+    CloudPolicyClient::ResultCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   std::unique_ptr<DMServerJobConfiguration> config =
@@ -1023,7 +1023,7 @@ void CloudPolicyClient::SetURLLoaderFactoryForTesting(
 void CloudPolicyClient::UploadCertificate(
     const std::string& certificate_data,
     em::DeviceCertUploadRequest::CertificateType certificate_type,
-    CloudPolicyClient::StatusCallback callback) {
+    CloudPolicyClient::ResultCallback callback) {
   std::unique_ptr<DMServerJobConfiguration> config =
       CreateCertUploadJobConfiguration(std::move(callback));
   PrepareCertUploadRequest(config.get(), certificate_data, certificate_type);
@@ -1044,7 +1044,7 @@ void CloudPolicyClient::PrepareCertUploadRequest(
 
 std::unique_ptr<DMServerJobConfiguration>
 CloudPolicyClient::CreateCertUploadJobConfiguration(
-    CloudPolicyClient::StatusCallback callback) {
+    CloudPolicyClient::ResultCallback callback) {
   CHECK(is_registered());
   return std::make_unique<DMServerJobConfiguration>(
       service_,
@@ -1220,19 +1220,18 @@ void CloudPolicyClient::OnPolicyFetchCompleted(DMServerJobResult result) {
 }
 
 void CloudPolicyClient::OnCertificateUploadCompleted(
-    CloudPolicyClient::StatusCallback callback,
+    CloudPolicyClient::ResultCallback callback,
     DMServerJobResult result) {
-  bool success = true;
   last_dm_status_ = result.dm_status;
   if (result.dm_status != DM_STATUS_SUCCESS) {
-    success = false;
     NotifyClientError();
-  } else if (!result.response.has_cert_upload_response()) {
+  } else if (result.dm_status == DM_STATUS_SUCCESS &&
+             !result.response.has_cert_upload_response()) {
     LOG_POLICY(WARNING, CBCM_ENROLLMENT)
         << "Empty upload certificate response.";
-    success = false;
+    result.dm_status = DM_STATUS_RESPONSE_DECODING_ERROR;
   }
-  std::move(callback).Run(success);
+  std::move(callback).Run(CloudPolicyClient::Result(result.dm_status));
   RemoveJob(result.job);
 }
 

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

@@ -305,22 +305,22 @@ class POLICY_EXPORT CloudPolicyClient {
   // will be called when the operation completes.
   virtual void UploadEnterpriseMachineCertificate(
       const std::string& certificate_data,
-      StatusCallback callback);
+      ResultCallback callback);
 
   // Upload an enrollment certificate to the server.  Like FetchPolicy, this
   // method requires that the client is in a registered state.
   // |certificate_data| must hold the X.509 certificate data to be sent to the
   // server.  The |callback| will be called when the operation completes.
   virtual void UploadEnterpriseEnrollmentCertificate(
       const std::string& certificate_data,
-      StatusCallback callback);
+      ResultCallback callback);
 
   // Upload an enrollment identifier to the server. Like FetchPolicy, this
   // method requires that the client is in a registered state.
   // |enrollment_id| must hold an enrollment identifier. The |callback| will be
   // called when the operation completes.
   virtual void UploadEnterpriseEnrollmentId(const std::string& enrollment_id,
-                                            StatusCallback callback);
+                                            ResultCallback callback);
 
   // Uploads status to the server. The client must be in a registered state.
   // Only non-null statuses will be included in the upload status request. The
@@ -604,7 +604,7 @@ class POLICY_EXPORT CloudPolicyClient {
       const std::string& certificate_data,
       enterprise_management::DeviceCertUploadRequest::CertificateType
           certificate_type,
-      StatusCallback callback);
+      ResultCallback callback);
 
   // This is called when a RegisterWithCertiifcate request has been signed.
   void OnRegisterWithCertificateRequestSigned(
@@ -623,7 +623,7 @@ class POLICY_EXPORT CloudPolicyClient {
                                       DMServerJobResult result);
 
   // Callback for certificate upload requests.
-  void OnCertificateUploadCompleted(StatusCallback callback,
+  void OnCertificateUploadCompleted(ResultCallback callback,
                                     DMServerJobResult result);
 
   // Callback for several types of status/report upload requests.
@@ -778,7 +778,7 @@ class POLICY_EXPORT CloudPolicyClient {
 
   // Creates a job config to upload a certificate.
   std::unique_ptr<DMServerJobConfiguration> CreateCertUploadJobConfiguration(
-      CloudPolicyClient::StatusCallback callback);
+      CloudPolicyClient::ResultCallback callback);
 
   // Creates a job config to upload a report.
   std::unique_ptr<DMServerJobConfiguration> CreateReportUploadJobConfiguration(