Make PlatformVerificationFlow::ChallengeCallback a base::OnceCallback

PlatformVerificationFlow::ChallengeCallback was previously unspecified
as to whether it was a base::OnceCallback or base::RepeatingCallback
(base::Callback aliases base::RepeatingCallback and should not be used).

Since it's really a base::OnceCallback, this CL changes it to be that
and adjusts the surrounding code appropriately.

Bug: 1007635
Change-Id: Idf531ef4e2048cc31ed4d6ce15559dd3543725f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2321793Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Will Cassella <cassew@google.com>
Cr-Commit-Position: refs/heads/master@{#792735}

chrome/browser/chromeos/attestation/attestation_policy_browsertest.cc

@@ -67,8 +67,9 @@ class AttestationDevicePolicyTest
             nullptr, nullptr, chromeos::FakeCryptohomeClient::Get(), nullptr));
     verifier->ChallengePlatformKey(
         browser()->tab_strip_model()->GetActiveWebContents(), "fake_service_id",
-        "fake_challenge", base::Bind(&AttestationDevicePolicyTest::Callback,
-                                     base::Unretained(this)));
+        "fake_challenge",
+        base::BindOnce(&AttestationDevicePolicyTest::Callback,
+                       base::Unretained(this)));
     WaitForAsyncOperation();
     return result_;
   }

chrome/browser/chromeos/attestation/platform_verification_flow.h

@@ -115,11 +115,11 @@ class PlatformVerificationFlow
   // signature.  This key may be generated on demand and is not guaranteed to
   // persist across multiple calls to this method.  The browser does not check
   // the validity of |signature| or |platform_key_certificate|.
-  typedef base::Callback<void(Result result,
+  using ChallengeCallback =
+      base::OnceCallback<void(Result result,
                               const std::string& signed_data,
                               const std::string& signature,
-                              const std::string& platform_key_certificate)>
-      ChallengeCallback;
+                              const std::string& platform_key_certificate)>;
 
   // A constructor that uses the default implementation of all dependencies
   // including Delegate.
@@ -145,7 +145,7 @@ class PlatformVerificationFlow
   void ChallengePlatformKey(content::WebContents* web_contents,
                             const std::string& service_id,
                             const std::string& challenge,
-                            const ChallengeCallback& callback);
+                            ChallengeCallback callback);
 
   void set_timeout_delay(const base::TimeDelta& timeout_delay) {
     timeout_delay_ = timeout_delay;
@@ -161,8 +161,8 @@ class PlatformVerificationFlow
     ChallengeContext(content::WebContents* web_contents,
                      const std::string& service_id,
                      const std::string& challenge,
-                     const ChallengeCallback& callback);
-    ChallengeContext(const ChallengeContext& other);
+                     ChallengeCallback callback);
+    ChallengeContext(ChallengeContext&& other);
     ~ChallengeContext();
 
     content::WebContents* web_contents;
@@ -176,7 +176,7 @@ class PlatformVerificationFlow
   // Callback for attestation preparation. The arguments to ChallengePlatformKey
   // are in |context|, and |attestation_prepared| specifies whether attestation
   // has been prepared on this device.
-  void OnAttestationPrepared(const ChallengeContext& context,
+  void OnAttestationPrepared(ChallengeContext context,
                              bool attestation_prepared);
 
   // Initiates the flow to get a platform key certificate.  The arguments to
@@ -184,9 +184,10 @@ class PlatformVerificationFlow
   // for which to get a certificate.  If |force_new_key| is true then any
   // existing key for the same user and service will be ignored and a new key
   // will be generated and certified.
-  void GetCertificate(const ChallengeContext& context,
-                      const AccountId& account_id,
-                      bool force_new_key);
+  void GetCertificate(
+      scoped_refptr<base::RefCountedData<ChallengeContext>> context,
+      const AccountId& account_id,
+      bool force_new_key);
 
   // A callback called when an attestation certificate request operation
   // completes.  The arguments to ChallengePlatformKey are in |context|.
@@ -197,16 +198,18 @@ class PlatformVerificationFlow
   // a request to sign the challenge.  If the operation timed out prior to this
   // method being called, this method does nothing - notably, the callback is
   // not invoked.
-  void OnCertificateReady(const ChallengeContext& context,
-                          const AccountId& account_id,
-                          std::unique_ptr<base::OneShotTimer> timer,
-                          AttestationStatus operation_status,
-                          const std::string& certificate_chain);
+  void OnCertificateReady(
+      scoped_refptr<base::RefCountedData<ChallengeContext>> context,
+      const AccountId& account_id,
+      std::unique_ptr<base::OneShotTimer> timer,
+      AttestationStatus operation_status,
+      const std::string& certificate_chain);
 
   // A callback run after a constant delay to handle timeouts for lengthy
   // certificate requests.  |context.callback| will be invoked with a TIMEOUT
   // result.
-  void OnCertificateTimeout(const ChallengeContext& context);
+  void OnCertificateTimeout(
+      scoped_refptr<base::RefCountedData<ChallengeContext>> context);
 
   // A callback called when a challenge signing request has completed.  The
   // |certificate_chain| is the platform certificate chain for the key which
@@ -217,7 +220,7 @@ class PlatformVerificationFlow
   // challenge signing operation was successful.  If it was successful,
   // |response_data| holds the challenge response and the method will invoke
   // |context.callback|.
-  void OnChallengeReady(const ChallengeContext& context,
+  void OnChallengeReady(ChallengeContext context,
                         const AccountId& account_id,
                         const std::string& certificate_chain,
                         bool is_expiring_soon,

chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc

@@ -112,13 +112,15 @@ class PlatformVerificationFlowTest : public ::testing::Test {
                                              &fake_delegate_);
 
     // Create callbacks for tests to use with verifier_.
-    callback_ = base::Bind(&PlatformVerificationFlowTest::FakeChallengeCallback,
-                           base::Unretained(this));
-
     settings_helper_.ReplaceDeviceSettingsProviderWithStub();
     settings_helper_.SetBoolean(kAttestationForContentProtectionEnabled, true);
   }
 
+  PlatformVerificationFlow::ChallengeCallback CreateChallengeCallback() {
+    return base::BindOnce(&PlatformVerificationFlowTest::FakeChallengeCallback,
+                          base::Unretained(this));
+  }
+
   void ExpectAttestationFlow() {
     // When consent is not given or the feature is disabled, it is important
     // that there are no calls to the attestation service.  Thus, a test must
@@ -196,7 +198,6 @@ class PlatformVerificationFlowTest : public ::testing::Test {
   bool sign_challenge_success_;
 
   // Callback functions and data.
-  PlatformVerificationFlow::ChallengeCallback callback_;
   PlatformVerificationFlow::Result result_;
   std::string challenge_salt_;
   std::string challenge_signature_;
@@ -205,7 +206,8 @@ class PlatformVerificationFlowTest : public ::testing::Test {
 
 TEST_F(PlatformVerificationFlowTest, Success) {
   ExpectAttestationFlow();
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::SUCCESS, result_);
   EXPECT_EQ(kTestSignedData, challenge_salt_);
@@ -215,14 +217,16 @@ TEST_F(PlatformVerificationFlowTest, Success) {
 
 TEST_F(PlatformVerificationFlowTest, NotPermittedByUser) {
   fake_delegate_.set_is_permitted_by_user(false);
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::USER_REJECTED, result_);
 }
 
 TEST_F(PlatformVerificationFlowTest, FeatureDisabledByPolicy) {
   settings_helper_.SetBoolean(kAttestationForContentProtectionEnabled, false);
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::POLICY_REJECTED, result_);
 }
@@ -230,7 +234,8 @@ TEST_F(PlatformVerificationFlowTest, FeatureDisabledByPolicy) {
 TEST_F(PlatformVerificationFlowTest, NotVerifiedDueToUnspeciedFailure) {
   certificate_status_ = ATTESTATION_UNSPECIFIED_FAILURE;
   ExpectAttestationFlow();
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::PLATFORM_NOT_VERIFIED, result_);
 }
@@ -238,7 +243,8 @@ TEST_F(PlatformVerificationFlowTest, NotVerifiedDueToUnspeciedFailure) {
 TEST_F(PlatformVerificationFlowTest, NotVerifiedDueToBadRequestFailure) {
   certificate_status_ = ATTESTATION_SERVER_BAD_REQUEST_FAILURE;
   ExpectAttestationFlow();
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::PLATFORM_NOT_VERIFIED, result_);
 }
@@ -246,14 +252,16 @@ TEST_F(PlatformVerificationFlowTest, NotVerifiedDueToBadRequestFailure) {
 TEST_F(PlatformVerificationFlowTest, ChallengeSigningError) {
   sign_challenge_success_ = false;
   ExpectAttestationFlow();
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::INTERNAL_ERROR, result_);
 }
 
 TEST_F(PlatformVerificationFlowTest, DBusFailure) {
   fake_cryptohome_client_.SetServiceIsAvailable(false);
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::INTERNAL_ERROR, result_);
 }
@@ -261,7 +269,8 @@ TEST_F(PlatformVerificationFlowTest, DBusFailure) {
 TEST_F(PlatformVerificationFlowTest, Timeout) {
   verifier_->set_timeout_delay(base::TimeDelta::FromSeconds(0));
   ExpectAttestationFlow();
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::TIMEOUT, result_);
 }
@@ -277,7 +286,8 @@ TEST_F(PlatformVerificationFlowTest, ExpiredCert) {
   // that it does not pass through the certificate expiry check again.
   ASSERT_TRUE(GetFakeCertificatePEM(base::TimeDelta::FromDays(-1),
                                     &fake_certificate_list_[2]));
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::SUCCESS, result_);
   EXPECT_EQ(fake_certificate_list_[1], certificate_);
@@ -297,7 +307,8 @@ TEST_F(PlatformVerificationFlowTest, ExpiredIntermediateCert) {
   fake_certificate_list_[0] = leaf_cert + intermediate_cert;
   ASSERT_TRUE(GetFakeCertificatePEM(base::TimeDelta::FromDays(90),
                                     &fake_certificate_list_[1]));
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::SUCCESS, result_);
   EXPECT_EQ(fake_certificate_list_[1], certificate_);
@@ -312,9 +323,12 @@ TEST_F(PlatformVerificationFlowTest, AsyncRenewalMultipleHits) {
                                     &fake_certificate_list_[0]));
   std::fill(fake_certificate_list_.begin() + 1, fake_certificate_list_.end(),
             fake_certificate_list_[0]);
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::SUCCESS, result_);
   EXPECT_EQ(fake_certificate_list_[0], certificate_);
@@ -325,7 +339,8 @@ TEST_F(PlatformVerificationFlowTest, AsyncRenewalMultipleHits) {
 TEST_F(PlatformVerificationFlowTest, CertificateNotPEM) {
   ExpectAttestationFlow();
   fake_certificate_list_.push_back("invalid_pem");
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::SUCCESS, result_);
   EXPECT_EQ(fake_certificate_list_[0], certificate_);
@@ -349,7 +364,8 @@ TEST_F(PlatformVerificationFlowTest, CertificateNotX509) {
       "M1pXeFdXR1ZHWkZWaVJYQmFWa2QwCk5GSkdjRFlLVFVSc1JGcDZNRGxEWnowOUNnPT0K\n"
       "-----END CERTIFICATE-----\n";
   fake_certificate_list_.push_back(not_x509);
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::SUCCESS, result_);
   EXPECT_EQ(fake_certificate_list_[0], certificate_);
@@ -357,7 +373,8 @@ TEST_F(PlatformVerificationFlowTest, CertificateNotX509) {
 
 TEST_F(PlatformVerificationFlowTest, UnsupportedMode) {
   fake_delegate_.set_is_in_supported_mode(false);
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::PLATFORM_NOT_VERIFIED, result_);
 }
@@ -365,7 +382,8 @@ TEST_F(PlatformVerificationFlowTest, UnsupportedMode) {
 TEST_F(PlatformVerificationFlowTest, AttestationNotPrepared) {
   fake_cryptohome_client_.set_tpm_attestation_is_enrolled(false);
   fake_cryptohome_client_.set_tpm_attestation_is_prepared(false);
-  verifier_->ChallengePlatformKey(NULL, kTestID, kTestChallenge, callback_);
+  verifier_->ChallengePlatformKey(nullptr, kTestID, kTestChallenge,
+                                  CreateChallengeCallback());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(PlatformVerificationFlow::PLATFORM_NOT_VERIFIED, result_);
 }

chrome/browser/media/platform_verification_impl.cc

@@ -89,8 +89,8 @@ void PlatformVerificationImpl::ChallengePlatform(
   platform_verification_flow_->ChallengePlatformKey(
       content::WebContents::FromRenderFrameHost(render_frame_host()),
       service_id, challenge,
-      base::Bind(&PlatformVerificationImpl::OnPlatformChallenged,
-                 weak_factory_.GetWeakPtr(), base::Passed(&callback)));
+      base::BindOnce(&PlatformVerificationImpl::OnPlatformChallenged,
+                     weak_factory_.GetWeakPtr(), base::Passed(&callback)));
 #else
   // Not supported, so return failure.
   std::move(callback).Run(false, std::string(), std::string(), std::string());