cert_provisioning: Fix invalidation handling

The invalidation topics in certificate provisioning are public, not
private.
Also, fix CertProvisioningWorker accidentally invalidating the weak
pointer it used to bind the OnShouldContinue callback.
Add a unit test for that case (fails without the fix).

Bug: 1146410
Change-Id: Ib3024d977de3de2a2b75de0c0b279dd0c8764883
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2523208
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Michael Ershov <miersh@google.com>
Cr-Commit-Position: refs/heads/master@{#824935}

chrome/browser/chromeos/cert_provisioning/cert_provisioning_invalidator.cc

@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_common.h"
 #include "chrome/browser/invalidation/profile_invalidation_provider_factory.h"
@@ -19,6 +20,11 @@ namespace cert_provisioning {
 
 namespace {
 
+// Topics that start with this prefix are considered to be "public" FCM topics.
+// This allows us to migrate to "private" FCM topics (which would get a
+// different prefix) server-side without client-side changes.
+constexpr char kFcmCertProvisioningPublicTopicPrefix[] = "cert-";
+
 // Shall be expanded to cert.[scope].[topic]
 const char* kOwnerNameFormat = "cert.%s.%s";
 
@@ -148,8 +154,9 @@ std::string CertProvisioningInvalidationHandler::GetOwnerName() const {
 }
 
 bool CertProvisioningInvalidationHandler::IsPublicTopic(
-    const syncer::Topic& /*topic*/) const {
-  return false;
+    const syncer::Topic& topic) const {
+  return base::StartsWith(topic, kFcmCertProvisioningPublicTopicPrefix,
+                          base::CompareCase::SENSITIVE);
 }
 
 }  // namespace internal

chrome/browser/chromeos/cert_provisioning/cert_provisioning_worker.cc

@@ -895,10 +895,13 @@ void CertProvisioningWorkerImpl::RegisterForInvalidationTopic() {
     return;
   }
 
+  // Registering the callback with base::Unretained is OK because this class
+  // owns |invalidator_|, and the callback will never be called after
+  // |invalidator_| is destroyed.
   invalidator_->Register(
       invalidation_topic_,
       base::BindRepeating(&CertProvisioningWorkerImpl::OnShouldContinue,
-                          weak_factory_.GetWeakPtr(),
+                          base::Unretained(this),
                           ContinueReason::kInvalidation));
 
   RecordEvent(cert_scope_,

chrome/browser/chromeos/cert_provisioning/cert_provisioning_worker_unittest.cc

@@ -49,6 +49,7 @@ using chromeos::attestation::MockTpmChallengeKeySubtle;
 using testing::_;
 using testing::AtLeast;
 using testing::Mock;
+using testing::SaveArg;
 using testing::StrictMock;
 
 namespace chromeos {
@@ -189,7 +190,7 @@ void VerifyDeleteKeyCalledOnce(CertScope cert_scope) {
         .WillOnce(RunOnceCallback<4>(                              \
             policy::DeviceManagementStatus::DM_STATUS_SUCCESS,     \
             /*response_error=*/base::nullopt,                      \
-            /*try_again_later_ms=*/(DELAY_MS), /*invalidation_topic=*/"", \
+            /*try_again_later_ms=*/(DELAY_MS), kInvalidationTopic, \
             /*va_challenge=*/"",                                   \
             enterprise_management::HashingAlgorithm::              \
                 HASHING_ALGORITHM_UNSPECIFIED,                     \
@@ -845,6 +846,128 @@ TEST_F(CertProvisioningWorkerTest, TryLaterWait) {
   }
 }
 
+// Checks that when the server returns try_again_later field, the worker will
+// retry when the invalidation is triggered.
+TEST_F(CertProvisioningWorkerTest, InvalidationRespected) {
+  CertProfile cert_profile(kCertProfileId, kCertProfileVersion,
+                           /*is_va_enabled=*/true, kCertProfileRenewalPeriod);
+
+  MockTpmChallengeKeySubtle* mock_tpm_challenge_key = PrepareTpmChallengeKey();
+  MockCertProvisioningInvalidator* mock_invalidator = nullptr;
+  CertProvisioningWorkerImpl worker(
+      CertScope::kUser, GetProfile(), &testing_pref_service_, cert_profile,
+      &cloud_policy_client_, MakeInvalidator(&mock_invalidator),
+      GetStateChangeCallback(), GetResultCallback());
+
+  const TimeDelta start_csr_delay = TimeDelta::FromSeconds(30);
+  const TimeDelta finish_csr_delay = TimeDelta::FromSeconds(30);
+  const TimeDelta download_cert_server_delay = TimeDelta::FromMilliseconds(100);
+  const TimeDelta small_delay = TimeDelta::FromMilliseconds(500);
+
+  EXPECT_CALL(state_change_callback_observer_, StateChangeCallback)
+      .Times(AtLeast(1));
+  {
+    testing::InSequence seq;
+
+    EXPECT_PREPARE_KEY_OK(
+        *mock_tpm_challenge_key,
+        StartPrepareKeyStep(attestation::AttestationKeyType::KEY_USER,
+                            /*will_register_key=*/true,
+                            GetKeyName(kCertProfileId),
+                            /*profile=*/_,
+                            /*callback=*/_));
+
+    EXPECT_START_CSR_TRY_LATER(
+        ClientCertProvisioningStartCsr(kCertScopeStrUser, kCertProfileId,
+                                       kCertProfileVersion, GetPublicKey(),
+                                       /*callback=*/_),
+        start_csr_delay.InMilliseconds());
+
+    worker.DoStep();
+    EXPECT_EQ(worker.GetState(),
+              CertProvisioningWorkerState::kKeypairGenerated);
+  }
+
+  base::RepeatingClosure on_invalidation_callback;
+  {
+    testing::InSequence seq;
+
+    EXPECT_START_CSR_OK(ClientCertProvisioningStartCsr(
+        kCertScopeStrUser, kCertProfileId, kCertProfileVersion, GetPublicKey(),
+        /*callback=*/_));
+    EXPECT_CALL(*mock_invalidator, Register(kInvalidationTopic, _))
+        .WillOnce(SaveArg<1>(&on_invalidation_callback));
+
+    EXPECT_SIGN_CHALLENGE_OK(*mock_tpm_challenge_key,
+                             StartSignChallengeStep(kChallenge,
+                                                    /*callback=*/_));
+
+    EXPECT_REGISTER_KEY_OK(*mock_tpm_challenge_key, StartRegisterKeyStep);
+
+    EXPECT_CALL(
+        *key_permissions_manager_,
+        AllowKeyForUsage(/*callback=*/_, platform_keys::KeyUsage::kCorporate,
+                         GetPublicKey()));
+
+    EXPECT_SET_ATTRIBUTE_FOR_KEY_OK(SetAttributeForKey(
+        platform_keys::TokenId::kUser, GetPublicKey(),
+        platform_keys::KeyAttributeType::kCertificateProvisioningId,
+        kCertProfileId, _));
+
+    EXPECT_SIGN_RSAPKC1_DIGEST_OK(SignRSAPKCS1Digest(
+        ::testing::Optional(platform_keys::TokenId::kUser), kDataToSign,
+        GetPublicKey(), kPkHashAlgo, /*callback=*/_));
+
+    EXPECT_FINISH_CSR_TRY_LATER(
+        ClientCertProvisioningFinishCsr(
+            kCertScopeStrUser, kCertProfileId, kCertProfileVersion,
+            GetPublicKey(), kChallengeResponse, kSignature, /*callback=*/_),
+        finish_csr_delay.InMilliseconds());
+
+    FastForwardBy(start_csr_delay + small_delay);
+    EXPECT_EQ(worker.GetState(), CertProvisioningWorkerState::kSignCsrFinished);
+  }
+
+  {
+    testing::InSequence seq;
+
+    EXPECT_FINISH_CSR_OK(ClientCertProvisioningFinishCsr(
+        kCertScopeStrUser, kCertProfileId, kCertProfileVersion, GetPublicKey(),
+        kChallengeResponse, kSignature, /*callback=*/_));
+
+    EXPECT_DOWNLOAD_CERT_TRY_LATER(
+        ClientCertProvisioningDownloadCert(kCertScopeStrUser, kCertProfileId,
+                                           kCertProfileVersion, GetPublicKey(),
+                                           /*callback=*/_),
+        download_cert_server_delay.InMilliseconds());
+
+    FastForwardBy(finish_csr_delay + small_delay);
+    EXPECT_EQ(worker.GetState(),
+              CertProvisioningWorkerState::kFinishCsrResponseReceived);
+  }
+
+  {
+    EXPECT_EQ(worker.GetState(),
+              CertProvisioningWorkerState::kFinishCsrResponseReceived);
+
+    testing::InSequence seq;
+
+    EXPECT_DOWNLOAD_CERT_OK(ClientCertProvisioningDownloadCert);
+
+    EXPECT_IMPORT_CERTIFICATE_OK(ImportCertificate(
+        platform_keys::TokenId::kUser, /*certificate=*/_, /*callback=*/_));
+
+    EXPECT_CALL(*mock_invalidator, Unregister()).Times(1);
+
+    EXPECT_CALL(callback_observer_,
+                Callback(cert_profile, CertProvisioningWorkerState::kSucceeded))
+        .Times(1);
+
+    on_invalidation_callback.Run();
+    EXPECT_EQ(worker.GetState(), CertProvisioningWorkerState::kSucceeded);
+  }
+}
+
 // Checks that when the server returns error status, the worker will enter an
 // error state and stop the provisioning.
 TEST_F(CertProvisioningWorkerTest, StatusErrorHandling) {