Invalidations logic for Lacros

This CL allows Ash to notify Lacros with the new policy data for device
account after it is validated and installed in Ash.

BUG=crbug/1114069
TEST=Manual test on device

Change-Id: I99cc96785d6487f4f90034818b3f7f2928537d8b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2640497
Reviewed-by: Igor <igorcov@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Roland Bock <rbock@google.com>
Commit-Queue: Igor <igorcov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#859127}

chrome/browser/chromeos/crosapi/browser_manager.cc

@@ -591,6 +591,10 @@ void BrowserManager::NotifyMojoDisconnected() {
 
 void BrowserManager::SetDeviceAccountPolicy(const std::string& policy_blob) {
   environment_provider_->SetDeviceAccountPolicy(policy_blob);
+  if (browser_service_.has_value()) {
+    browser_service_->service->UpdateDeviceAccountPolicy(
+        std::vector<uint8_t>(policy_blob.begin(), policy_blob.end()));
+  }
 }
 
 }  // namespace crosapi

chrome/browser/chromeos/crosapi/test_mojo_connection_manager_unittest.cc

@@ -68,6 +68,7 @@ class TestBrowserService : public crosapi::mojom::BrowserService {
   void GetFeedbackData(GetFeedbackDataCallback callback) override {}
   void GetHistograms(GetHistogramsCallback callback) override {}
   void GetActiveTabUrl(GetActiveTabUrlCallback callback) override {}
+  void UpdateDeviceAccountPolicy(const std::vector<uint8_t>& policy) override {}
 
   bool init_is_called() { return init_is_called_; }
 

chromeos/crosapi/mojom/crosapi.mojom

@@ -367,6 +367,12 @@ interface BrowserService {
   // Returns Url of the active tab from the browser if there is any.
   [MinVersion=8]
   GetActiveTabUrl@5() => (url.mojom.Url? url);
+
+  // Notifies Lacros to update the policy data from the device account with the
+  // new data from input. The data is serialized blob of PolicyFetchResponse
+  // object.
+  [MinVersion=9]
+  UpdateDeviceAccountPolicy@6(array<uint8> policy);
 };
 
 // TODO(crbug.com/1180712): move to its own file. Currently due to circular

chromeos/lacros/lacros_chrome_service_impl.cc

@@ -130,6 +130,14 @@ class LacrosChromeServiceNeverBlockingState
                                std::move(callback))));
   }
 
+  void UpdateDeviceAccountPolicy(const std::vector<uint8_t>& policy) override {
+    owner_sequence_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &LacrosChromeServiceImpl::UpdateDeviceAccountPolicyAffineSequence,
+            owner_, policy));
+  }
+
   // Unlike most of other methods of this class, this is called on the
   // affined thread. Specifically, it is intended to be called before starting
   // the message pumping of the affined thread to pass the initialization
@@ -339,7 +347,9 @@ LacrosChromeServiceImpl* LacrosChromeServiceImpl::Get() {
 LacrosChromeServiceImpl::LacrosChromeServiceImpl(
     std::unique_ptr<LacrosChromeServiceDelegate> delegate)
     : delegate_(std::move(delegate)),
-      sequenced_state_(nullptr, base::OnTaskRunnerDeleter(nullptr)) {
+      sequenced_state_(nullptr, base::OnTaskRunnerDeleter(nullptr)),
+      observer_list_(
+          base::MakeRefCounted<base::ObserverListThreadSafe<Observer>>()) {
   if (g_disable_all_crosapi_for_tests) {
     // Tests don't call BrowserService::InitDeprecated(), so provide
     // BrowserInitParams with default values.
@@ -842,4 +852,19 @@ base::Optional<uint32_t> LacrosChromeServiceImpl::CrosapiVersion() const {
   return init_params_->crosapi_version;
 }
 
+void LacrosChromeServiceImpl::AddObserver(Observer* obs) {
+  observer_list_->AddObserver(obs);
+}
+
+void LacrosChromeServiceImpl::RemoveObserver(Observer* obs) {
+  observer_list_->RemoveObserver(obs);
+}
+
+void LacrosChromeServiceImpl::UpdateDeviceAccountPolicyAffineSequence(
+    const std::vector<uint8_t>& policy_fetch_response) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(affine_sequence_checker_);
+  observer_list_->Notify(FROM_HERE, &Observer::NotifyPolicyUpdate,
+                         policy_fetch_response);
+}
+
 }  // namespace chromeos

chromeos/lacros/lacros_chrome_service_impl.h

@@ -5,11 +5,15 @@
 #ifndef CHROMEOS_LACROS_LACROS_CHROME_SERVICE_IMPL_H_
 #define CHROMEOS_LACROS_LACROS_CHROME_SERVICE_IMPL_H_
 
+#include <stdint.h>
+
 #include <memory>
+#include <vector>
 
 #include "base/component_export.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list_threadsafe.h"
 #include "base/optional.h"
 #include "base/sequence_checker.h"
 #include "base/sequenced_task_runner.h"
@@ -55,6 +59,16 @@ class LacrosChromeServiceNeverBlockingState;
 // documented with threading requirements.
 class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
  public:
+  class Observer {
+   public:
+    // Called when the new policy data is received from Ash.
+    virtual void NotifyPolicyUpdate(
+        const std::vector<uint8_t>& policy_fetch_response) {}
+
+   protected:
+    virtual ~Observer() = default;
+  };
+
   // The getter is safe to call from all threads.
   //
   // This method returns nullptr very early or late in the application
@@ -106,6 +120,10 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
   bool IsTestControllerAvailable() const;
   bool IsUrlHandlerAvailable() const;
 
+  // Methods to add/remove observer. Safe to call from any thread.
+  void AddObserver(Observer* obs);
+  void RemoveObserver(Observer* obs);
+
   // --------------------------------------------------------------------------
   // mojo::Remote is sequence affine. The following methods are convenient
   // helpers that expose pre-established Remotes that can only be used from the
@@ -290,6 +308,11 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
   // Gets Url of the active tab on the affine sequence.
   void GetActiveTabUrlAffineSequence(GetActiveTabUrlCallback callback);
 
+  // Update device account policy with the input data. The data comes as
+  // serialized blob of PolicyFetchResponse object.
+  void UpdateDeviceAccountPolicyAffineSequence(
+      const std::vector<uint8_t>& policy);
+
   // Returns ash's version of the Crosapi mojo interface version. This
   // determines which interface methods are available. This is safe to call from
   // any sequence. This can only be called after BindReceiver().
@@ -334,6 +357,9 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
   // Set to true after BindReceiver() is called.
   bool did_bind_receiver_ = false;
 
+  // The list of observers.
+  scoped_refptr<base::ObserverListThreadSafe<Observer>> observer_list_;
+
   // Checks that the method is called on the affine sequence.
   SEQUENCE_CHECKER(affine_sequence_checker_);
 

components/policy/core/common/BUILD.gn

@@ -517,6 +517,9 @@ source_set("unit_tests") {
   if (is_win) {
     sources += [ "policy_loader_win_unittest.cc" ]
   }
+  if (is_chromeos_lacros) {
+    sources += [ "policy_loader_lacros_unittest.cc" ]
+  }
 
   deps = [
     ":common_constants",
@@ -543,6 +546,14 @@ source_set("unit_tests") {
   if (is_android) {
     deps += [ "//components/policy/android:test_jni_headers" ]
   }
+  if (is_chromeos_lacros) {
+    deps += [
+      "//chrome/browser",
+      "//chromeos/crosapi/mojom",
+      "//chromeos/lacros",
+      "//chromeos/startup:startup",
+    ]
+  }
 }
 
 if (is_win || is_linux || is_chromeos) {

components/policy/core/common/policy_loader_lacros.cc

@@ -11,6 +11,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/check.h"
 #include "base/logging.h"
 #include "base/memory/weak_ptr.h"
@@ -25,48 +26,66 @@ namespace policy {
 
 PolicyLoaderLacros::PolicyLoaderLacros(
     scoped_refptr<base::SequencedTaskRunner> task_runner)
-    : AsyncPolicyLoader(task_runner), task_runner_(task_runner) {}
-
-PolicyLoaderLacros::~PolicyLoaderLacros() = default;
-
-void PolicyLoaderLacros::InitOnBackgroundThread() {
-  DCHECK(task_runner_->RunsTasksInCurrentSequence());
-}
-
-std::unique_ptr<PolicyBundle> PolicyLoaderLacros::Load() {
-  std::unique_ptr<PolicyBundle> bundle = std::make_unique<PolicyBundle>();
-
+    : AsyncPolicyLoader(task_runner), task_runner_(task_runner) {
   auto* lacros_chrome_service = chromeos::LacrosChromeServiceImpl::Get();
   if (!lacros_chrome_service) {
     // LacrosChromeService should be available at this timing in production.
     // However, in some existing tests, it is not.
     // TODO(crbug.com/1114069): Set up LacrosChromeServiceImpl in tests.
     LOG(ERROR) << "No LacrosChromeService is found.";
-    return bundle;
+    return;
   }
   const crosapi::mojom::BrowserInitParams* init_params =
       lacros_chrome_service->init_params();
   if (!init_params) {
     LOG(ERROR) << "No init params";
-    return bundle;
+    return;
   }
-
   if (!init_params->device_account_policy) {
     LOG(ERROR) << "No policy data";
-    return bundle;
+    return;
+  }
+  policy_fetch_response_ = init_params->device_account_policy.value();
+  last_modification_ = base::Time::Now();
+}
+
+PolicyLoaderLacros::~PolicyLoaderLacros() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  auto* lacros_chrome_service = chromeos::LacrosChromeServiceImpl::Get();
+  if (lacros_chrome_service) {
+    lacros_chrome_service->RemoveObserver(this);
   }
+}
 
-  std::vector<uint8_t> data = init_params->device_account_policy.value();
-  if (data.empty()) {
+void PolicyLoaderLacros::InitOnBackgroundThread() {
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+  // We add this as observer on background thread to avoid a situation when
+  // notification comes after the object is destroyed, but not removed from the
+  // list yet.
+  // TODO(crbug.com/1114069): Set up LacrosChromeServiceImpl in tests.
+  auto* lacros_chrome_service = chromeos::LacrosChromeServiceImpl::Get();
+  if (lacros_chrome_service) {
+    lacros_chrome_service->AddObserver(this);
+  }
+}
+
+std::unique_ptr<PolicyBundle> PolicyLoaderLacros::Load() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  std::unique_ptr<PolicyBundle> bundle = std::make_unique<PolicyBundle>();
+
+  if (!policy_fetch_response_ || policy_fetch_response_->empty()) {
     return bundle;
   }
 
   auto policy = std::make_unique<enterprise_management::PolicyFetchResponse>();
-  if (!policy->ParseFromString(std::string(data.begin(), data.end()))) {
+  if (!policy->ParseFromArray(policy_fetch_response_.value().data(),
+                              policy_fetch_response_->size())) {
     LOG(ERROR) << "Failed to parse policy data";
     return bundle;
   }
-  UserCloudPolicyValidator validator(std::move(policy), task_runner_);
+  UserCloudPolicyValidator validator(std::move(policy),
+                                     /*background_task_runner=*/nullptr);
   validator.ValidatePayload();
   validator.RunValidation();
 
@@ -77,12 +96,29 @@ std::unique_ptr<PolicyBundle> PolicyLoaderLacros::Load() {
                     PolicyScope::POLICY_SCOPE_USER, &policy_map);
   bundle->Get(PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()))
       .MergeFrom(policy_map);
-  last_modification_ = base::Time::Now();
   return bundle;
 }
 
 base::Time PolicyLoaderLacros::LastModificationTime() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return last_modification_;
 }
 
+void PolicyLoaderLacros::NotifyPolicyUpdate(
+    const std::vector<uint8_t>& policy_fetch_response) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&PolicyLoaderLacros::UpdatePolicyData,
+                     weak_factory_.GetWeakPtr(), policy_fetch_response));
+}
+
+void PolicyLoaderLacros::UpdatePolicyData(
+    const std::vector<uint8_t>& policy_fetch_response) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  policy_fetch_response_ = policy_fetch_response;
+  last_modification_ = base::Time::Now();
+  Reload(true);
+}
+
 }  // namespace policy

components/policy/core/common/policy_loader_lacros.h

@@ -5,16 +5,26 @@
 #ifndef COMPONENTS_POLICY_CORE_COMMON_POLICY_LOADER_LACROS_H_
 #define COMPONENTS_POLICY_CORE_COMMON_POLICY_LOADER_LACROS_H_
 
+#include <stdint.h>
+
+#include <vector>
+
 #include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/optional.h"
+#include "base/sequence_checker.h"
 #include "base/sequenced_task_runner.h"
 #include "base/time/time.h"
+#include "chromeos/lacros/lacros_chrome_service_impl.h"
 #include "components/policy/core/common/async_policy_loader.h"
 
 namespace policy {
 
 // A policy loader for Lacros. The data is taken from Ash and the validatity of
 // data is trusted, since they have been validated by Ash.
-class POLICY_EXPORT PolicyLoaderLacros : public AsyncPolicyLoader {
+class POLICY_EXPORT PolicyLoaderLacros
+    : public AsyncPolicyLoader,
+      public chromeos::LacrosChromeServiceImpl::Observer {
  public:
   // Creates the policy loader, saving the task_runner internally. Later
   // task_runner is used to have in sequence the process of policy parsing and
@@ -27,20 +37,38 @@ class POLICY_EXPORT PolicyLoaderLacros : public AsyncPolicyLoader {
   ~PolicyLoaderLacros() override;
 
   // AsyncPolicyLoader implementation.
-  // Verifies that it runs on correct thread.
+  // Verifies that it runs on correct thread. Detaches from the sequence checker
+  // which allows all other methods to check that they are executed on the same
+  // sequence. |sequence_checker_| is used for that.
   void InitOnBackgroundThread() override;
   // Loads the policy data from LacrosInitParams and populates it in the bundle
   // that is returned.
   std::unique_ptr<PolicyBundle> Load() override;
   // Returns the last time the policy successfully loaded.
   base::Time LastModificationTime() override;
 
+  // LacrosChromeServiceDelegateImpl::Observer implementation.
+  // Update and reload the policy with new data in the background thread.
+  void NotifyPolicyUpdate(
+      const std::vector<uint8_t>& policy_fetch_response) override;
+
  private:
+  // Update and reload the policy with new data.
+  void UpdatePolicyData(const std::vector<uint8_t>& policy_fetch_response);
+
   // Task runner for running background jobs.
   const scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
+  // Serialized blob of PolicyFetchResponse object received from the server.
+  base::Optional<std::vector<uint8_t>> policy_fetch_response_;
+
   // The time of last modification.
   base::Time last_modification_;
+
+  // Checks that the method is called on the right sequence.
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  base::WeakPtrFactory<PolicyLoaderLacros> weak_factory_{this};
 };
 
 }  // namespace policy