Load DeviceLocalAccount policy and DeviceSettings immediately on restore after Chrome crash.

chrome/browser/chromeos/policy/device_local_account_policy_store.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/policy/device_local_account_policy_store.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 25 matching lines @@
 DeviceLocalAccountPolicyStore::~DeviceLocalAccountPolicyStore() {}
 
 void DeviceLocalAccountPolicyStore::Load() {
   weak_factory_.InvalidateWeakPtrs();
   session_manager_client_->RetrieveDeviceLocalAccountPolicy(
       account_id_,
       base::Bind(&DeviceLocalAccountPolicyStore::ValidateLoadedPolicyBlob,
                  weak_factory_.GetWeakPtr()));
 }
 
+void DeviceLocalAccountPolicyStore::LoadImmediately() {

[Andrew T Wilson (Slow), 2017/02/23 12:05:48]

In general, there will also be an asynchronous load in progress - do we need to
cancel that (perhaps via a call to weak_factory_.InvalidateWeakPtrs())?

[Sergey Poromov, 2017/02/28 14:01:17]

Done.

+  // This blocking D-Bus call is in the startup path and will block the UI
+  // thread. This only happens when the Profile is created synchronously, which
+  // on Chrome OS happens whenever the browser is restarted into the same
+  // session. That happens when the browser crashes, or right after signin if

[emaxx, 2017/02/22 17:24:33]

nit: It's a little bit unclear what "that happens" refers here to. Maybe combine
this sentence with the previous one?

[Sergey Poromov, 2017/02/28 14:01:17]

Done.

+  // the user has flags configured in about:flags.
+  // However, on those paths we must load policy synchronously so that the
+  // Profile initialization never sees unmanaged prefs, which would lead to
+  // data loss. http://crbug.com/263061
+
+  std::string policy_blob =

[emaxx, 2017/02/22 17:24:33]

nit: const

[Sergey Poromov, 2017/02/28 14:01:17]

Done.

+      session_manager_client_->BlockingRetrieveDeviceLocalAccountPolicy(
+          account_id_);
+  if (policy_blob.empty()) {
+    status_ = CloudPolicyStore::STATUS_LOAD_ERROR;

[emaxx, 2017/02/22 17:24:33]

Isn't it possible to get rid of this code duplication by sharing the common
validation code with the asynchronous load path?
They must be pretty similar to each other, except that async calls (to D-Bus, to
validator) have to be replaced with sync analogs.
I think this can be achieved by passing a boolean flag along the chain of calls,
starting, for instance, from ValidateLoadedPolicyBlob.
(BTW, this is what is done in the analogous case for
UserCloudPolicyStore::PolicyLoaded()).

[Sergey Poromov, 2017/02/28 14:01:17]

Done.

+    NotifyStoreError();
+    return;
+  }
+  std::unique_ptr<em::PolicyFetchResponse> policy(
+      new em::PolicyFetchResponse());
+  if (!policy->ParseFromString(policy_blob)) {
+    status_ = CloudPolicyStore::STATUS_PARSE_ERROR;
+    NotifyStoreError();
+    return;
+  }
+
+  chromeos::DeviceSettingsService::OwnershipStatus ownership_status =
+      device_settings_service_->GetOwnershipStatus();
+
+  DCHECK_NE(chromeos::DeviceSettingsService::OWNERSHIP_UNKNOWN,
+            ownership_status);
+  const em::PolicyData* device_policy_data =
+      device_settings_service_->policy_data();
+  // Note that the key is obtained through the device settings service instead
+  // of using |policy_signature_public_key_| member, as the latter one is
+  // updated only after the successful installation of the policy.
+  scoped_refptr<ownership::PublicKey> key =
+      device_settings_service_->GetPublicKey();
+  if (!key.get() || !key->is_loaded() || !device_policy_data) {
+    status_ = CloudPolicyStore::STATUS_BAD_STATE;
+    NotifyStoreLoaded();
+    return;
+  }
+
+  std::unique_ptr<UserCloudPolicyValidator> validator =
+      CreateValidatorForLoad(false /*valid_timestamp_required*/,
+                             device_policy_data, key, std::move(policy));
+  // Start validation. The Validator will delete itself once validation is
+  // complete.

[Andrew T Wilson (Slow), 2017/02/23 12:05:48]

Wait, what? But we have a unique_ptr holding |validator| so if the validator
deletes itself won't we get a double free?

[Sergey Poromov, 2017/02/28 14:01:17]

You are right, that was true only for StartValidation()

+  validator->RunValidation();
+
+  UpdatePolicy(key->as_string(), validator.get());
+}
+
 void DeviceLocalAccountPolicyStore::Store(
     const em::PolicyFetchResponse& policy) {
   weak_factory_.InvalidateWeakPtrs();
   CheckKeyAndValidate(
       true, base::MakeUnique<em::PolicyFetchResponse>(policy),
       base::Bind(&DeviceLocalAccountPolicyStore::StoreValidatedPolicy,
                  weak_factory_.GetWeakPtr()));
 }
 
 void DeviceLocalAccountPolicyStore::ValidateLoadedPolicyBlob(
@@ skipping 93 matching lines @@
   // of using |policy_signature_public_key_| member, as the latter one is
   // updated only after the successful installation of the policy.
   scoped_refptr<ownership::PublicKey> key =
       device_settings_service_->GetPublicKey();
   if (!key.get() || !key->is_loaded() || !device_policy_data) {
     status_ = CloudPolicyStore::STATUS_BAD_STATE;
     NotifyStoreLoaded();
     return;
   }
 
+  std::unique_ptr<UserCloudPolicyValidator> validator =
+      CreateValidatorForLoad(valid_timestamp_required, device_policy_data, key,
+                             std::move(policy_response));
+  // Start validation. The Validator will delete itself once validation is
+  // complete.
+  validator.release()->StartValidation(base::Bind(callback, key->as_string()));
+}
+
+std::unique_ptr<UserCloudPolicyValidator>
+DeviceLocalAccountPolicyStore::CreateValidatorForLoad(
+    bool valid_timestamp_required,
+    const em::PolicyData* device_policy_data,
+    scoped_refptr<ownership::PublicKey> key,
+    std::unique_ptr<em::PolicyFetchResponse> policy_response) {
   std::unique_ptr<UserCloudPolicyValidator> validator(
       UserCloudPolicyValidator::Create(std::move(policy_response),
                                        background_task_runner()));
   validator->ValidateUsername(account_id_, false);
   validator->ValidatePolicyType(dm_protocol::kChromePublicAccountPolicyType);
   // The timestamp is verified when storing a new policy downloaded from the
   // server but not when loading a cached policy from disk.
   // See SessionManagerOperation::ValidateDeviceSettings for the rationale.
   validator->ValidateAgainstCurrentPolicy(
       policy(),
       valid_timestamp_required
           ? CloudPolicyValidatorBase::TIMESTAMP_FULLY_VALIDATED
           : CloudPolicyValidatorBase::TIMESTAMP_NOT_VALIDATED,
       CloudPolicyValidatorBase::DM_TOKEN_NOT_REQUIRED,
       CloudPolicyValidatorBase::DEVICE_ID_NOT_REQUIRED);
 
   // Validate the DMToken to match what device policy has.
   validator->ValidateDMToken(device_policy_data->request_token(),
                              CloudPolicyValidatorBase::DM_TOKEN_REQUIRED);
 
   // Validate the device id to match what device policy has.
   validator->ValidateDeviceId(device_policy_data->device_id(),
                               CloudPolicyValidatorBase::DEVICE_ID_REQUIRED);
 
   validator->ValidatePayload();
   validator->ValidateSignature(key->as_string());
-  validator.release()->StartValidation(base::Bind(callback, key->as_string()));
+  return validator;
 }
 
 }  // namespace policy