Cert Provisioning: Update UI on state change notifications

Update the Cert Provisioning UI when the state of a certificate
provisioning process changes.
Rate limit updates to the UI to max 1 update per 2 seconds to avoid
flickering / performance issues.

Bug: 1081396
Test: unit_tests
Change-Id: If539d19885e9e2804dfc9d3f662a58dff8f0be53
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2377729Reviewed-by: Denis Kuznetsov [CET] <antrim@chromium.org>
Reviewed-by: Michael Ershov <miersh@google.com>
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805449}

chrome/browser/ui/webui/certificate_provisioning_ui_handler.cc

@@ -14,7 +14,6 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_process_platform_part.h"
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_common.h"
-#include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_scheduler.h"
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_scheduler_user_service.h"
 #include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_worker.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
@@ -165,7 +164,12 @@ CertificateProvisioningUiHandler::CertificateProvisioningUiHandler(
     : scheduler_for_user_(scheduler_for_user),
       scheduler_for_device_(ShouldUseDeviceWideProcesses(user_profile)
                                 ? scheduler_for_device
-                                : nullptr) {}
+                                : nullptr) {
+  if (scheduler_for_user_)
+    observed_schedulers_.Add(scheduler_for_user_);
+  if (scheduler_for_device_)
+    observed_schedulers_.Add(scheduler_for_device_);
+}
 
 CertificateProvisioningUiHandler::~CertificateProvisioningUiHandler() = default;
 
@@ -185,6 +189,34 @@ void CertificateProvisioningUiHandler::RegisterMessages() {
                           base::Unretained(this)));
 }
 
+void CertificateProvisioningUiHandler::OnVisibleStateChanged() {
+  // If Javascript is not allowed yet, we don't need to cache the update,
+  // because the UI will request a refresh during its first message to the
+  // handler.
+  if (!IsJavascriptAllowed())
+    return;
+  if (hold_back_updates_timer_.IsRunning()) {
+    update_after_hold_back_ = true;
+    return;
+  }
+  constexpr base::TimeDelta kTimeToHoldBackUpdates =
+      base::TimeDelta::FromMilliseconds(300);
+  hold_back_updates_timer_.Start(
+      FROM_HERE, kTimeToHoldBackUpdates,
+      base::BindOnce(
+          &CertificateProvisioningUiHandler::OnHoldBackUpdatesTimerExpired,
+          weak_ptr_factory_.GetWeakPtr()));
+
+  RefreshCertificateProvisioningProcesses();
+}
+
+unsigned int
+CertificateProvisioningUiHandler::ReadAndResetUiRefreshCountForTesting() {
+  unsigned int value = ui_refresh_count_for_testing_;
+  ui_refresh_count_for_testing_ = 0;
+  return value;
+}
+
 void CertificateProvisioningUiHandler::
     HandleRefreshCertificateProvisioningProcesses(const base::ListValue* args) {
   CHECK_EQ(0U, args->GetSize());
@@ -214,21 +246,6 @@ void CertificateProvisioningUiHandler::
     return;
 
   scheduler->UpdateOneCert(cert_profile_id.GetString());
-
-  // Send an update to the UI immediately to reflect a possible status change.
-  RefreshCertificateProvisioningProcesses();
-
-  // Trigger a refresh in a few seconds, in case the state has triggered a
-  // refresh with the server.
-  // TODO(https://crbug.com/1045895): Use a real observer instead.
-  constexpr base::TimeDelta kTimeToWaitBeforeRefresh =
-      base::TimeDelta::FromSeconds(10);
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(&CertificateProvisioningUiHandler::
-                         RefreshCertificateProvisioningProcesses,
-                     weak_ptr_factory_.GetWeakPtr()),
-      kTimeToWaitBeforeRefresh);
 }
 
 void CertificateProvisioningUiHandler::
@@ -244,10 +261,18 @@ void CertificateProvisioningUiHandler::
                                  /*is_device_wide=*/true);
   }
 
+  ++ui_refresh_count_for_testing_;
   FireWebUIListener("certificate-provisioning-processes-changed",
                     std::move(all_processes));
 }
 
+void CertificateProvisioningUiHandler::OnHoldBackUpdatesTimerExpired() {
+  if (update_after_hold_back_) {
+    update_after_hold_back_ = false;
+    RefreshCertificateProvisioningProcesses();
+  }
+}
+
 // static
 bool CertificateProvisioningUiHandler::ShouldUseDeviceWideProcesses(
     Profile* user_profile) {

chrome/browser/ui/webui/certificate_provisioning_ui_handler.h

@@ -8,7 +8,10 @@
 #include <utility>
 
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_observer.h"
+#include "base/timer/timer.h"
 #include "base/values.h"
+#include "chrome/browser/chromeos/cert_provisioning/cert_provisioning_scheduler.h"
 #include "content/public/browser/web_ui_message_handler.h"
 
 class Profile;
@@ -16,9 +19,9 @@ class Profile;
 namespace chromeos {
 namespace cert_provisioning {
 
-class CertProvisioningScheduler;
-
-class CertificateProvisioningUiHandler : public content::WebUIMessageHandler {
+class CertificateProvisioningUiHandler
+    : public content::WebUIMessageHandler,
+      public CertProvisioningSchedulerObserver {
  public:
   // Creates a CertificateProvisioningUiHandler for |user_profile|, which uses:
   // (*) The CertProvisioningScheduler associated with |user_profile|, if any.
@@ -51,6 +54,13 @@ class CertificateProvisioningUiHandler : public content::WebUIMessageHandler {
   // content::WebUIMessageHandler.
   void RegisterMessages() override;
 
+  // CertProvisioningSchedulerObserver:
+  void OnVisibleStateChanged() override;
+
+  // For testing: Reads the count of UI refreshes sent to the WebUI (since
+  // instantiation or the last call to this function) and resets it to 0.
+  unsigned int ReadAndResetUiRefreshCountForTesting();
+
  private:
   // Send the list of certificate provisioning processes to the UI, triggered by
   // the UI when it loads.
@@ -70,6 +80,9 @@ class CertificateProvisioningUiHandler : public content::WebUIMessageHandler {
   // Send the list of certificate provisioning processes to the UI.
   void RefreshCertificateProvisioningProcesses();
 
+  // Called when the |hold_back_updates_timer_| expires.
+  void OnHoldBackUpdatesTimerExpired();
+
   // Returns true if device-wide certificate provisioning processes should be
   // displayed, i.e. if the |user_profile| is affiliated.
   static bool ShouldUseDeviceWideProcesses(Profile* user_profile);
@@ -82,6 +95,23 @@ class CertificateProvisioningUiHandler : public content::WebUIMessageHandler {
   // Unowned.
   CertProvisioningScheduler* const scheduler_for_device_;
 
+  // When this timer is running, updates provided by the schedulers should not
+  // be forwarded to the UI until it fires. Used to prevent spamming the UI if
+  // many events come in in rapid succession.
+  base::OneShotTimer hold_back_updates_timer_;
+
+  // When this is true, an update should be sent to the UI when
+  // |hold_back_updates_timer_| fires.
+  bool update_after_hold_back_ = false;
+
+  // Keeps track of the count of UI refreshes sent to the WebUI.
+  unsigned int ui_refresh_count_for_testing_ = 0;
+
+  // Keeps track of the CertProvisioningSchedulers that this UI handler
+  // observes.
+  ScopedObserver<CertProvisioningScheduler, CertProvisioningSchedulerObserver>
+      observed_schedulers_{this};
+
   base::WeakPtrFactory<CertificateProvisioningUiHandler> weak_ptr_factory_{
       this};
 };

chrome/browser/ui/webui/certificate_provisioning_ui_handler_unittest.cc

@@ -36,6 +36,8 @@ namespace {
 using ::testing::Return;
 using ::testing::ReturnPointee;
 using ::testing::ReturnRef;
+using ::testing::SaveArg;
+using ::testing::StrictMock;
 using ::testing::UnorderedElementsAre;
 
 // Extracted from a X.509 certificate using the command:
@@ -137,20 +139,28 @@ class CertificateProvisioningUiHandlerTestBase : public ::testing::Test {
             profile_helper_for_testing_.GetProfile()));
     web_ui_.set_web_contents(web_contents_.get());
 
-    auto handler = std::make_unique<CertificateProvisioningUiHandler>(
-        GetProfile(), &scheduler_for_user_, &scheduler_for_device_);
-    handler_ = handler.get();
-    web_ui_.AddMessageHandler(std::move(handler));
-
     EXPECT_CALL(scheduler_for_user_, GetWorkers)
         .WillRepeatedly(ReturnRef(user_workers_));
     EXPECT_CALL(scheduler_for_user_, GetFailedCertProfileIds)
         .WillRepeatedly(ReturnRef(user_failed_workers_));
+    EXPECT_CALL(scheduler_for_user_, AddObserver(_))
+        .WillOnce(SaveArg<0>(&scheduler_observer_for_user_));
+    EXPECT_CALL(scheduler_for_user_, RemoveObserver(_)).Times(1);
+
+    if (user_is_affiliated) {
+      EXPECT_CALL(scheduler_for_device_, GetWorkers)
+          .WillRepeatedly(ReturnRef(device_workers_));
+      EXPECT_CALL(scheduler_for_device_, GetFailedCertProfileIds)
+          .WillRepeatedly(ReturnRef(device_failed_workers_));
+      EXPECT_CALL(scheduler_for_device_, AddObserver(_))
+          .WillOnce(SaveArg<0>(&scheduler_observer_for_device_));
+      EXPECT_CALL(scheduler_for_device_, RemoveObserver(_)).Times(1);
+    }
 
-    EXPECT_CALL(scheduler_for_device_, GetWorkers)
-        .WillRepeatedly(ReturnRef(device_workers_));
-    EXPECT_CALL(scheduler_for_device_, GetFailedCertProfileIds)
-        .WillRepeatedly(ReturnRef(device_failed_workers_));
+    auto handler = std::make_unique<CertificateProvisioningUiHandler>(
+        GetProfile(), &scheduler_for_user_, &scheduler_for_device_);
+    handler_ = handler.get();
+    web_ui_.AddMessageHandler(std::move(handler));
   }
 
   ~CertificateProvisioningUiHandlerTestBase() override {}
@@ -166,20 +176,14 @@ class CertificateProvisioningUiHandlerTestBase : public ::testing::Test {
     crypto::EnsureNSSInit();
   }
 
-  void RefreshCertProvisioningProcesses(
+  // Use in ASSERT_NO_FATAL_FAILURE.
+  void ExtractCertProvisioningProcesses(
+      std::vector<base::Value>& args,
       base::Value* out_all_processes,
       std::vector<std::string>* out_profile_ids) {
-    content::TestWebUIListenerObserver result_waiter(
-        &web_ui_, "certificate-provisioning-processes-changed");
-
-    base::ListValue args;
-    web_ui_.HandleReceivedMessage("refreshCertificateProvisioningProcessses",
-                                  &args);
-
-    result_waiter.Wait();
-    ASSERT_EQ(1U, result_waiter.args().size());
-    ASSERT_TRUE(result_waiter.args()[0].is_list());
-    *out_all_processes = std::move(result_waiter.args()[0]);
+    ASSERT_EQ(1U, args.size());
+    ASSERT_TRUE(args[0].is_list());
+    *out_all_processes = std::move(args[0]);
 
     // Extract all profile ids for easier verification.
     if (!out_profile_ids)
@@ -192,21 +196,40 @@ class CertificateProvisioningUiHandlerTestBase : public ::testing::Test {
     }
   }
 
+  // Use in ASSERT_NO_FATAL_FAILURE.
+  void RefreshCertProvisioningProcesses(
+      base::Value* out_all_processes,
+      std::vector<std::string>* out_profile_ids) {
+    content::TestWebUIListenerObserver result_waiter(
+        &web_ui_, "certificate-provisioning-processes-changed");
+
+    base::ListValue args;
+    web_ui_.HandleReceivedMessage("refreshCertificateProvisioningProcessses",
+                                  &args);
+
+    result_waiter.Wait();
+    ASSERT_NO_FATAL_FAILURE(ExtractCertProvisioningProcesses(
+        result_waiter.args(), out_all_processes, out_profile_ids));
+  }
+
  protected:
   Profile* GetProfile() { return profile_helper_for_testing_.GetProfile(); }
 
   std::string der_encoded_spki_;
 
-  content::BrowserTaskEnvironment task_environment_;
+  content::BrowserTaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
   ProfileHelperForTesting profile_helper_for_testing_;
 
   WorkerMap user_workers_;
   base::flat_map<CertProfileId, FailedWorkerInfo> user_failed_workers_;
-  MockCertProvisioningScheduler scheduler_for_user_;
+  StrictMock<MockCertProvisioningScheduler> scheduler_for_user_;
+  CertProvisioningSchedulerObserver* scheduler_observer_for_user_ = nullptr;
 
   WorkerMap device_workers_;
   base::flat_map<CertProfileId, FailedWorkerInfo> device_failed_workers_;
-  MockCertProvisioningScheduler scheduler_for_device_;
+  StrictMock<MockCertProvisioningScheduler> scheduler_for_device_;
+  CertProvisioningSchedulerObserver* scheduler_observer_for_device_ = nullptr;
 
   content::TestWebUI web_ui_;
   std::unique_ptr<content::WebContents> web_contents_;
@@ -266,14 +289,14 @@ TEST_F(CertificateProvisioningUiHandlerTest, HasProcesses) {
           R"({
                "certProfileId": "user_cert_profile_1",
                "isDeviceWide": false,
-               "publicKey": "$1",
+               "publicKey": "$0",
                "stateId": 1,
-               "status": "$0",
+               "status": "$1",
                "timeSinceLastUpdate": ""
              })",
-          {l10n_util::GetStringUTF8(
-               IDS_SETTINGS_CERTIFICATE_MANAGER_PROVISIONING_STATUS_PREPARING_CSR_WAITING),
-           kFormattedPublicKey}));
+          {kFormattedPublicKey,
+           l10n_util::GetStringUTF8(
+               IDS_SETTINGS_CERTIFICATE_MANAGER_PROVISIONING_STATUS_PREPARING_CSR_WAITING)}));
 }
 
 TEST_F(CertificateProvisioningUiHandlerAffiliatedTest, HasProcessesAffiliated) {
@@ -304,28 +327,93 @@ TEST_F(CertificateProvisioningUiHandlerAffiliatedTest, HasProcessesAffiliated) {
           R"({
                "certProfileId": "user_cert_profile_1",
                "isDeviceWide": false,
-               "publicKey": "$1",
+               "publicKey": "$0",
                "stateId": 1,
-               "status": "$0",
+               "status": "$1",
                "timeSinceLastUpdate": ""
              })",
-          {l10n_util::GetStringUTF8(
-               IDS_SETTINGS_CERTIFICATE_MANAGER_PROVISIONING_STATUS_PREPARING_CSR_WAITING),
-           kFormattedPublicKey}));
+          {kFormattedPublicKey,
+           l10n_util::GetStringUTF8(
+               IDS_SETTINGS_CERTIFICATE_MANAGER_PROVISIONING_STATUS_PREPARING_CSR_WAITING)}));
   EXPECT_EQ(
       GetByProfileId(all_processes, "device_cert_profile_1"),
       FormatJsonDict(
           R"({
                "certProfileId": "device_cert_profile_1",
                "isDeviceWide": true,
-               "publicKey": "$1",
+               "publicKey": "$0",
                "stateId": 10,
-               "status": "$0",
+               "status": "$1",
+               "timeSinceLastUpdate": ""
+             })",
+          {kFormattedPublicKey,
+           l10n_util::GetStringUTF8(
+               IDS_SETTINGS_CERTIFICATE_MANAGER_PROVISIONING_STATUS_FAILURE)}));
+}
+
+TEST_F(CertificateProvisioningUiHandlerTest, Updates) {
+  base::Value all_processes;
+  std::vector<std::string> profile_ids;
+
+  // Perform an initial JS-side initiated refresh so that javascript is
+  // considered allowed by the UI handler.
+  ASSERT_NO_FATAL_FAILURE(
+      RefreshCertProvisioningProcesses(&all_processes, &profile_ids));
+  ASSERT_THAT(profile_ids, UnorderedElementsAre());
+  EXPECT_EQ(1U, handler_->ReadAndResetUiRefreshCountForTesting());
+
+  auto user_cert_worker = std::make_unique<MockCertProvisioningWorker>();
+  SetupMockCertProvisioningWorker(
+      user_cert_worker.get(), CertProvisioningWorkerState::kKeypairGenerated,
+      &der_encoded_spki_);
+  user_workers_["user_cert_profile_1"] = std::move(user_cert_worker);
+
+  // The user worker triggers an update
+  content::TestWebUIListenerObserver result_waiter_1(
+      &web_ui_, "certificate-provisioning-processes-changed");
+
+  scheduler_observer_for_user_->OnVisibleStateChanged();
+
+  EXPECT_EQ(1U, handler_->ReadAndResetUiRefreshCountForTesting());
+  result_waiter_1.Wait();
+  ASSERT_NO_FATAL_FAILURE(ExtractCertProvisioningProcesses(
+      result_waiter_1.args(), &all_processes, &profile_ids));
+
+  // Only the user worker is expected to be displayed in the UI, because the
+  // user is not affiliated.
+  ASSERT_THAT(profile_ids, UnorderedElementsAre("user_cert_profile_1"));
+
+  EXPECT_EQ(
+      GetByProfileId(all_processes, "user_cert_profile_1"),
+      FormatJsonDict(
+          R"({
+               "certProfileId": "user_cert_profile_1",
+               "isDeviceWide": false,
+               "publicKey": "$0",
+               "stateId": 1,
+               "status": "$1",
                "timeSinceLastUpdate": ""
              })",
-          {l10n_util::GetStringUTF8(
-               IDS_SETTINGS_CERTIFICATE_MANAGER_PROVISIONING_STATUS_FAILURE),
-           kFormattedPublicKey}));
+          {kFormattedPublicKey,
+           l10n_util::GetStringUTF8(
+               IDS_SETTINGS_CERTIFICATE_MANAGER_PROVISIONING_STATUS_PREPARING_CSR_WAITING)}));
+
+  content::TestWebUIListenerObserver result_waiter_2(
+      &web_ui_, "certificate-provisioning-processes-changed");
+  scheduler_observer_for_user_->OnVisibleStateChanged();
+  // Another update does not trigger a UI update for the holdoff time.
+  task_environment_.FastForwardBy(base::TimeDelta::FromMilliseconds(299));
+  EXPECT_EQ(0U, handler_->ReadAndResetUiRefreshCountForTesting());
+
+  // When the holdoff time has elapsed, an UI update is triggered.
+  task_environment_.FastForwardBy(base::TimeDelta::FromMilliseconds(2));
+  EXPECT_EQ(1U, handler_->ReadAndResetUiRefreshCountForTesting());
+  result_waiter_2.Wait();
+
+  base::Value all_processes_2;
+  ASSERT_NO_FATAL_FAILURE(ExtractCertProvisioningProcesses(
+      result_waiter_2.args(), &all_processes_2, /*profile_ids=*/nullptr));
+  EXPECT_EQ(all_processes, all_processes_2);
 }
 
 }  // namespace