SessionManagerClient: Add generic storage/retrieval methods

Adds StorePolicy, RetrievePolicy and BlockingRetrievePolicy methods to
SessionManagerClient, which map to the new storage interface in Session
Manager, and marks the old methods as deprecated. The next step will be
to replace all existing Store/Retrieve calls by these three.

This also adds support for storing/retrieving extension policy, which
is needed for Chromad (see crbug.com/735100).

Points all storage/retrieval methods in FakeSessionManagerClient to the
generic implementations, reducing the difference between the two code
path controlled by the PolicyStorageType enum. In a nutshell, policy
is stored
- in a file at <stub file path> if PolicyStorageType::kOnDisk is set and
- in a memory-based map at key <stub file path> if
  PolicyStorageType::kInMemory is set.
The old way to store different policy in a separate location
(device_policy_, user_policies_ etc.) starts to get messy once extension
policy is introduced (you'd need an extra dimension of mappings for the
extension id aka component id).

BUG=chromium:765644
TEST=Ran some affected tests (DeviceSettingsServiceTest,
     UserCloudPolicyStoreChromeOSTest
     SiteIsolationFlagHandlingTest
     DeviceCloudPolicyManagerChromeOSEnrollmentTest
     OwnerSettingsServiceChromeOSTest), but mostly rely on trybots.

Change-Id: I1794d9264dc2ac8e7702cec4a079d6c9eb4c4091
Reviewed-on: https://chromium-review.googlesource.com/1024034
Commit-Queue: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554808}

chrome/browser/chromeos/ownership/owner_settings_service_chromeos_unittest.cc

@@ -165,7 +165,7 @@ TEST_F(OwnerSettingsServiceChromeOSTest, MultipleSetTest) {
 }
 
 TEST_F(OwnerSettingsServiceChromeOSTest, FailedSetRequest) {
-  session_manager_client_.set_store_device_policy_success(false);
+  session_manager_client_.set_store_policy_success(false);
   std::string current_channel;
   ASSERT_TRUE(provider_->Get(kReleaseChannel)->GetAsString(&current_channel));
   ASSERT_NE(current_channel, "stable-channel");

chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc

@@ -745,7 +745,7 @@ TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, ValidationFailed) {
 }
 
 TEST_P(DeviceCloudPolicyManagerChromeOSEnrollmentTest, StoreError) {
-  session_manager_client_.set_store_device_policy_success(false);
+  session_manager_client_.set_store_policy_success(false);
   RunTest();
   ExpectFailedEnrollment(EnrollmentStatus::STORE_ERROR);
   EXPECT_EQ(CloudPolicyStore::STATUS_STORE_ERROR,

chrome/browser/chromeos/policy/site_isolation_flag_handling_browsertest.cc

@@ -277,7 +277,7 @@ class SiteIsolationFlagHandlingTest
     // user session.
     auto fake_session_manager_client =
         std::make_unique<FakeSessionManagerClient>(
-            FakeSessionManagerClient::USE_HOST_POLICY);
+            FakeSessionManagerClient::PolicyStorageType::kOnDisk);
     fake_session_manager_client_ = fake_session_manager_client.get();
     DBusThreadManager::GetSetterForTesting()->SetSessionManagerClient(
         std::move(fake_session_manager_client));

chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos_unittest.cc

@@ -36,6 +36,7 @@ namespace em = enterprise_management;
 using RetrievePolicyResponseType =
     chromeos::SessionManagerClient::RetrievePolicyResponseType;
 
+using testing::_;
 using testing::AllOf;
 using testing::Eq;
 using testing::Mock;
@@ -43,7 +44,6 @@ using testing::Property;
 using testing::Return;
 using testing::SaveArg;
 using testing::SetArgPointee;
-using testing::_;
 
 namespace policy {
 
@@ -149,10 +149,10 @@ class UserCloudPolicyStoreChromeOSTest : public testing::Test {
 
   // Install an expectation on |observer_| for an error code.
   void ExpectError(CloudPolicyStore::Status error) {
-    EXPECT_CALL(observer_,
+    EXPECT_CALL(
+        observer_,
         OnStoreError(AllOf(Eq(store_.get()),
-                                   Property(&CloudPolicyStore::status,
-                                            Eq(error)))));
+                           Property(&CloudPolicyStore::status, Eq(error)))));
   }
 
   // Triggers a store_->Load() operation, handles the expected call to
@@ -324,7 +324,7 @@ TEST_F(UserCloudPolicyStoreChromeOSTest, StoreWithRotationValidationError) {
 
 TEST_F(UserCloudPolicyStoreChromeOSTest, StoreFail) {
   // Let store policy fail.
-  session_manager_client_->set_store_user_policy_success(false);
+  session_manager_client_->set_store_policy_success(false);
 
   ExpectError(CloudPolicyStore::STATUS_STORE_ERROR);
   store_->Store(policy_.policy());

chrome/browser/chromeos/settings/device_settings_service_unittest.cc

@@ -152,7 +152,7 @@ TEST_F(DeviceSettingsServiceTest, StoreFailure) {
   EXPECT_EQ(DeviceSettingsService::STORE_KEY_UNAVAILABLE,
             device_settings_service_.status());
 
-  session_manager_client_.set_store_device_policy_success(false);
+  session_manager_client_.set_store_policy_success(false);
   device_settings_service_.Store(
       device_policy_.GetCopy(),
       base::Bind(&DeviceSettingsServiceTest::SetOperationCompleted,

chromeos/dbus/fake_session_manager_client.h

@@ -23,14 +23,15 @@ namespace chromeos {
 // returns them unmodified.
 class FakeSessionManagerClient : public SessionManagerClient {
  public:
-  enum FakeSessionManagerOptions : uint32_t {
-    NONE = 0,
-    USE_HOST_POLICY = 1 << 0,
+  enum class PolicyStorageType {
+    kOnDisk,    // Store policy in regular files on disk. Usually used for
+                // fake D-Bus client implementation, see
+                // SessionManagerClient::Create().
+    kInMemory,  // Store policy in memory only. Usually used for tests.
   };
 
   FakeSessionManagerClient();
-  explicit FakeSessionManagerClient(
-      uint32_t options /* bitwise or of multiple FakeSessionManagerOptions */);
+  explicit FakeSessionManagerClient(PolicyStorageType policy_storage);
   ~FakeSessionManagerClient() override;
 
   // SessionManagerClient overrides
@@ -73,6 +74,11 @@ class FakeSessionManagerClient : public SessionManagerClient {
   RetrievePolicyResponseType BlockingRetrieveDeviceLocalAccountPolicy(
       const std::string& account_id,
       std::string* policy_out) override;
+  void RetrievePolicy(const login_manager::PolicyDescriptor& descriptor,
+                      RetrievePolicyCallback callback) override;
+  RetrievePolicyResponseType BlockingRetrievePolicy(
+      const login_manager::PolicyDescriptor& descriptor,
+      std::string* policy_out) override;
   void StoreDevicePolicy(const std::string& policy_blob,
                          VoidDBusMethodCallback callback) override;
   void StorePolicyForUser(const cryptohome::Identification& cryptohome_id,
@@ -81,6 +87,9 @@ class FakeSessionManagerClient : public SessionManagerClient {
   void StoreDeviceLocalAccountPolicy(const std::string& account_id,
                                      const std::string& policy_blob,
                                      VoidDBusMethodCallback callback) override;
+  void StorePolicy(const login_manager::PolicyDescriptor& descriptor,
+                   const std::string& policy_blob,
+                   VoidDBusMethodCallback callback) override;
   bool SupportsRestartToApplyUserFlags() const override;
   void SetFlagsForUser(const cryptohome::Identification& cryptohome_id,
                        const std::vector<std::string>& flags) override;
@@ -122,12 +131,15 @@ class FakeSessionManagerClient : public SessionManagerClient {
         supports_restart_to_apply_user_flags;
   }
 
-  void set_store_device_policy_success(bool success) {
-    store_device_policy_success_ = success;
+  void set_store_policy_success(bool success) {
+    store_policy_success_ = success;
   }
+  // Accessors for device policy. Only available for
+  // PolicyStorageType::kInMemory.
   const std::string& device_policy() const;
   void set_device_policy(const std::string& policy_blob);
 
+  // Accessors for user policy. Only available for PolicyStorageType::kInMemory.
   const std::string& user_policy(
       const cryptohome::Identification& cryptohome_id) const;
   void set_user_policy(const cryptohome::Identification& cryptohome_id,
@@ -136,10 +148,8 @@ class FakeSessionManagerClient : public SessionManagerClient {
       const cryptohome::Identification& cryptohome_id,
       const std::string& policy_blob);
 
-  void set_store_user_policy_success(bool success) {
-    store_user_policy_success_ = success;
-  }
-
+  // Accessors for device local account policy. Only available for
+  // PolicyStorageType::kInMemory.
   const std::string& device_local_account_policy(
       const std::string& account_id) const;
   void set_device_local_account_policy(const std::string& account_id,
@@ -154,9 +164,11 @@ class FakeSessionManagerClient : public SessionManagerClient {
   void OnPropertyChangeComplete(bool success);
 
   // Configures the list of state keys used to satisfy
-  // GetServerBackedStateKeys() requests.
+  // GetServerBackedStateKeys() requests. Only available for
+  // PolicyStorageType::kInMemory.
   void set_server_backed_state_keys(
       const std::vector<std::string>& state_keys) {
+    DCHECK(policy_storage_ == PolicyStorageType::kInMemory);
     server_backed_state_keys_ = state_keys;
   }
 
@@ -191,20 +203,18 @@ class FakeSessionManagerClient : public SessionManagerClient {
  private:
   bool supports_restart_to_apply_user_flags_ = false;
 
-  bool store_device_policy_success_ = true;
-  std::string device_policy_;
-  std::map<cryptohome::Identification, std::string> user_policies_;
-  std::map<cryptohome::Identification, std::string>
-      user_policies_without_session_;
-
-  // Controls whether StorePolicyForUser() should report success or not.
-  bool store_user_policy_success_ = true;
-
-  std::map<std::string, std::string> device_local_account_policy_;
   base::ObserverList<Observer> observers_;
   SessionManagerClient::ActiveSessionsMap user_sessions_;
   std::vector<std::string> server_backed_state_keys_;
 
+  // Policy is stored in |policy_| if |policy_storage_| type is
+  // PolicyStorageType::kInMemory. Uses the relative stub file path as key.
+  const PolicyStorageType policy_storage_;
+  std::map<std::string, std::string> policy_;
+
+  // If set to false, StorePolicy() always fails.
+  bool store_policy_success_ = true;
+
   int start_device_wipe_call_count_;
   int request_lock_screen_call_count_;
   int notify_lock_screen_shown_call_count_;
@@ -223,10 +233,6 @@ class FakeSessionManagerClient : public SessionManagerClient {
 
   StubDelegate* delegate_;
 
-  // Options for FakeSessionManagerClient with value of bitwise or of
-  // multiple FakeSessionManagerOptions.
-  uint32_t options_;
-
   // The last-set flags for user set through |SetFlagsForUser|.
   std::map<cryptohome::Identification, std::vector<std::string>>
       flags_for_user_;

chromeos/dbus/session_manager_client.cc

@@ -319,6 +319,17 @@ class SessionManagerClientImpl : public SessionManagerClient {
     return BlockingRetrievePolicy(descriptor, policy_out);
   }
 
+  void RetrievePolicy(const login_manager::PolicyDescriptor& descriptor,
+                      RetrievePolicyCallback callback) override {
+    CallRetrievePolicy(descriptor, std::move(callback));
+  }
+
+  RetrievePolicyResponseType BlockingRetrievePolicy(
+      const login_manager::PolicyDescriptor& descriptor,
+      std::string* policy_out) override {
+    return CallBlockingRetrievePolicy(descriptor, policy_out);
+  }
+
   void StoreDevicePolicy(const std::string& policy_blob,
                          VoidDBusMethodCallback callback) override {
     login_manager::PolicyDescriptor descriptor = MakeChromePolicyDescriptor(
@@ -342,6 +353,12 @@ class SessionManagerClientImpl : public SessionManagerClient {
     CallStorePolicy(descriptor, policy_blob, std::move(callback));
   }
 
+  void StorePolicy(const login_manager::PolicyDescriptor& descriptor,
+                   const std::string& policy_blob,
+                   VoidDBusMethodCallback callback) override {
+    CallStorePolicy(descriptor, policy_blob, std::move(callback));
+  }
+
   bool SupportsRestartToApplyUserFlags() const override { return true; }
 
   void SetFlagsForUser(const cryptohome::Identification& cryptohome_id,
@@ -550,7 +567,7 @@ class SessionManagerClientImpl : public SessionManagerClient {
   }
 
   // Blocking call to Session Manager to retrieve policy.
-  RetrievePolicyResponseType BlockingRetrievePolicy(
+  RetrievePolicyResponseType CallBlockingRetrievePolicy(
       const login_manager::PolicyDescriptor& descriptor,
       std::string* policy_out) {
     dbus::MethodCall method_call(
@@ -862,7 +879,7 @@ SessionManagerClient* SessionManagerClient::Create(
     return new SessionManagerClientImpl();
   DCHECK_EQ(FAKE_DBUS_CLIENT_IMPLEMENTATION, type);
   return new FakeSessionManagerClient(
-      FakeSessionManagerClient::USE_HOST_POLICY);
+      FakeSessionManagerClient::PolicyStorageType::kOnDisk);
 }
 
 }  // namespace chromeos

chromeos/dbus/session_manager_client.h

@@ -25,9 +25,10 @@ class Identification;
 }
 
 namespace login_manager {
+class PolicyDescriptor;
 class StartArcMiniContainerRequest;
 class UpgradeArcContainerRequest;
-}
+}  // namespace login_manager
 
 namespace chromeos {
 
@@ -187,6 +188,7 @@ class CHROMEOS_EXPORT SessionManagerClient : public DBusClient {
 
   // Fetches the device policy blob stored by the session manager.  Upon
   // completion of the retrieve attempt, we will call the provided callback.
+  // DEPRECATED, use RetrievePolicy() instead.
   virtual void RetrieveDevicePolicy(RetrievePolicyCallback callback) = 0;
 
   // Same as RetrieveDevicePolicy() but blocks until a reply is received, and
@@ -196,12 +198,14 @@ class CHROMEOS_EXPORT SessionManagerClient : public DBusClient {
   // considered acceptable (e.g. restarting the browser after a crash or after
   // a flag change).
   // TODO(crbug.com/160522): Get rid of blocking calls.
+  // DEPRECATED, use BlockingRetrievePolicy() instead.
   virtual RetrievePolicyResponseType BlockingRetrieveDevicePolicy(
       std::string* policy_out) = 0;
 
   // Fetches the user policy blob stored by the session manager for the given
   // |cryptohome_id|. Upon completion of the retrieve attempt, we will call the
   // provided callback.
+  // DEPRECATED, use RetrievePolicy() instead.
   virtual void RetrievePolicyForUser(
       const cryptohome::Identification& cryptohome_id,
       RetrievePolicyCallback callback) = 0;
@@ -213,18 +217,21 @@ class CHROMEOS_EXPORT SessionManagerClient : public DBusClient {
   // considered acceptable (e.g. restarting the browser after a crash or after
   // a flag change).
   // TODO(crbug.com/160522): Get rid of blocking calls.
+  // DEPRECATED, use BlockingRetrievePolicy() instead.
   virtual RetrievePolicyResponseType BlockingRetrievePolicyForUser(
       const cryptohome::Identification& cryptohome_id,
       std::string* policy_out) = 0;
 
   // Fetches the user policy blob for a hidden user home mount. |callback| is
   // invoked upon completition.
+  // DEPRECATED, use RetrievePolicy() instead.
   virtual void RetrievePolicyForUserWithoutSession(
       const cryptohome::Identification& cryptohome_id,
       RetrievePolicyCallback callback) = 0;
 
   // Fetches the policy blob associated with the specified device-local account
   // from session manager.  |callback| is invoked up on completion.
+  // DEPRECATED, use RetrievePolicy() instead.
   virtual void RetrieveDeviceLocalAccountPolicy(
       const std::string& account_id,
       RetrievePolicyCallback callback) = 0;
@@ -236,10 +243,27 @@ class CHROMEOS_EXPORT SessionManagerClient : public DBusClient {
   // considered acceptable (e.g. restarting the browser after a crash or after
   // a flag change).
   // TODO(crbug.com/165022): Get rid of blocking calls.
+  // DEPRECATED, use BlockingRetrievePolicy() instead.
   virtual RetrievePolicyResponseType BlockingRetrieveDeviceLocalAccountPolicy(
       const std::string& account_id,
       std::string* policy_out) = 0;
 
+  // Fetches a policy blob stored by the session manager. Invokes |callback|
+  // upon completion.
+  virtual void RetrievePolicy(const login_manager::PolicyDescriptor& descriptor,
+                              RetrievePolicyCallback callback) = 0;
+
+  // Same as RetrievePolicy() but blocks until a reply is
+  // received, and populates the policy synchronously. Returns SUCCESS when
+  // successful, or the corresponding error from enum in case of a failure.
+  // This may only be called in situations where blocking the UI thread is
+  // considered acceptable (e.g. restarting the browser after a crash or after
+  // a flag change).
+  // TODO(crbug.com/165022): Get rid of blocking calls.
+  virtual RetrievePolicyResponseType BlockingRetrievePolicy(
+      const login_manager::PolicyDescriptor& descriptor,
+      std::string* policy_out) = 0;
+
   // Attempts to asynchronously store |policy_blob| as device policy.  Upon
   // completion of the store attempt, we will call callback.
   virtual void StoreDevicePolicy(const std::string& policy_blob,
@@ -260,6 +284,13 @@ class CHROMEOS_EXPORT SessionManagerClient : public DBusClient {
       const std::string& policy_blob,
       VoidDBusMethodCallback callback) = 0;
 
+  // Sends a request to store a |policy_blob| to Session Manager. The storage
+  // location is determined by |descriptor|. The result of the operation is
+  // reported through |callback|.
+  virtual void StorePolicy(const login_manager::PolicyDescriptor& descriptor,
+                           const std::string& policy_blob,
+                           VoidDBusMethodCallback callback) = 0;
+
   // Returns whether session manager can be used to restart Chrome in order to
   // apply per-user session flags.
   virtual bool SupportsRestartToApplyUserFlags() const = 0;