Abandon user sign in when policy is retrieved before session started

chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.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/user_cloud_policy_store_chromeos.h"
 
 #include <stddef.h>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/chromeos/policy/user_policy_token_loader.h"
+#include "chrome/browser/lifetime/application_lifetime.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
 #include "chromeos/dbus/cryptohome_client.h"
-#include "chromeos/dbus/session_manager_client.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/policy/proto/cloud_policy.pb.h"
 #include "components/policy/proto/device_management_local.pb.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 
 namespace em = enterprise_management;
 
 namespace policy {
 
 namespace {
@@ skipping 67 matching lines @@
 
 void UserCloudPolicyStoreChromeOS::LoadImmediately() {
   // 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
   // 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 =
+  std::string policy_blob;
+  chromeos::SessionManagerClient::RetrievePolicyResponseType response_type =
       session_manager_client_->BlockingRetrievePolicyForUser(
-          cryptohome::Identification(account_id_));
-  if (policy_blob.empty()) {
+          cryptohome::Identification(account_id_), &policy_blob);
+
+  if (response_type == chromeos::SessionManagerClient::
+                           RetrievePolicyResponseType::SESSION_DOES_NOT_EXIST) {

[Daniel Erat, 2017/04/20 21:06:38]

if this is unexpected, can you at least do something like:

LOG(ERROR) << "Session manager claims that session doesn't exist; signing out";

here?

[igorcov, 2017/04/21 11:36:21]

Done.

+    chrome::AttemptUserExit();
+    return;
+  }
+
+  if (response_type ==
+          chromeos::SessionManagerClient::RetrievePolicyResponseType::SUCCESS &&
+      policy_blob.empty()) {
     // The session manager doesn't have policy, or the call failed.

[emaxx, 2017/04/21 00:01:52]

nit: Looks like the "or the call failed" part is not applicable anymore?

[igorcov, 2017/04/21 11:36:21]

Done.

     NotifyStoreLoaded();
     return;
   }
 
   std::unique_ptr<em::PolicyFetchResponse> policy(
       new em::PolicyFetchResponse());
-  if (!policy->ParseFromString(policy_blob)) {
+  if (response_type !=
+          chromeos::SessionManagerClient::RetrievePolicyResponseType::SUCCESS ||
+      !policy->ParseFromString(policy_blob)) {
     status_ = STATUS_PARSE_ERROR;

[emaxx, 2017/04/21 00:01:52]

The STATUS_PARSE_ERROR error is probably not the best match to the errors from
the session manager. I think STATUS_LOAD_ERROR would be more suitable (while
keeping STATUS_PARSE_ERROR only for ParseFromString failures).
(Same applies to OnPolicyRetrieved().)

[igorcov, 2017/04/21 11:36:21]

Done.

     NotifyStoreError();
     return;
   }
 
   std::string sanitized_username =
       cryptohome_client_->BlockingGetSanitizedUsername(
           cryptohome::Identification(account_id_));
   if (sanitized_username.empty()) {
     status_ = STATUS_LOAD_ERROR;
     NotifyStoreError();
@@ skipping 72 matching lines @@
   } else {
     // Load the policy right after storing it, to make sure it was accepted by
     // the session manager. An additional validation is performed after the
     // load; reload the key for that validation too, in case it was rotated.
     ReloadPolicyKey(base::Bind(&UserCloudPolicyStoreChromeOS::Load,
                                weak_factory_.GetWeakPtr()));
   }
 }
 
 void UserCloudPolicyStoreChromeOS::OnPolicyRetrieved(
-    const std::string& policy_blob) {
-  if (policy_blob.empty()) {
+    const std::string& policy_blob,
+    chromeos::SessionManagerClient::RetrievePolicyResponseType response_type) {
+  // Disallow the sign in when the Chrome OS user session has not started, which
+  // should always happen before the profile construction. An attempt to read
+  // the policy outside the session will always fail and return an empty policy
+  // blob.
+  if (response_type == chromeos::SessionManagerClient::
+                           RetrievePolicyResponseType::SESSION_DOES_NOT_EXIST) {

[Daniel Erat, 2017/04/20 21:06:38]

same comment here about logging an error

[igorcov, 2017/04/21 11:36:21]

Done.

+    chrome::AttemptUserExit();
+    return;
+  }
+
+  if (policy_blob.empty() &&
+      response_type ==
+          chromeos::SessionManagerClient::RetrievePolicyResponseType::SUCCESS) {
     // session_manager doesn't have policy. Adjust internal state and notify
     // the world about the policy update.
     policy_map_.Clear();
     policy_.reset();
     policy_signature_public_key_.clear();
     NotifyStoreLoaded();
     return;
   }
 
   std::unique_ptr<em::PolicyFetchResponse> policy(
       new em::PolicyFetchResponse());
-  if (!policy->ParseFromString(policy_blob)) {
+  if (response_type !=
+          chromeos::SessionManagerClient::RetrievePolicyResponseType::SUCCESS ||
+      !policy->ParseFromString(policy_blob)) {
     status_ = STATUS_PARSE_ERROR;
     NotifyStoreError();
     return;
   }
 
   // Load |cached_policy_key_| to verify the loaded policy.
   if (is_active_directory_) {
     ValidateRetrievedPolicy(std::move(policy));
   } else {
     EnsurePolicyKeyLoaded(
@@ skipping 134 matching lines @@
     validator->ValidateUsername(account_id_.GetUserEmail(), true);
     // The policy loaded from session manager need not be validated using the
     // verification key since it is secure, and since there may be legacy policy
     // data that was stored without a verification key.
     validator->ValidateSignature(cached_policy_key_);
   }
   return validator;
 }
 
 }  // namespace policy