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 {
 
 // Path within |user_policy_key_dir_| that contains the policy key.
 // "%s" must be substituted with the sanitized username.
 const base::FilePath::CharType kPolicyKeyFile[] =
     FILE_PATH_LITERAL("%s/policy.pub");
 
 // Maximum key size that will be loaded, in bytes.
 const size_t kKeySizeLimit = 16 * 1024;
 
+const char kSessionDoesNotExist[] =
+    "org.chromium.SessionManagerInterface.SessionDoesNotExist";

[Daniel Erat, 2017/04/10 18:59:26]

this is an implementation detail of the d-bus interface between chrome and
session_manager and probably doesn't belong here. it'd likely be better to check
for this in SessionManagerClient, and the string constant should be defined in
system_api if both chrome and session_manager are using it.

[igorcov, 2017/04/18 10:23:18]

Done - https://chromium-review.googlesource.com/c/474806/

+
 enum ValidationFailure {
   VALIDATION_FAILURE_DBUS,
   VALIDATION_FAILURE_LOAD_KEY,
   VALIDATION_FAILURE_SIZE,
 };
 
 void SampleValidationFailure(ValidationFailure sample) {
   UMA_HISTOGRAM_ENUMERATION("Enterprise.UserPolicyValidationFailure",
                             sample,
                             VALIDATION_FAILURE_SIZE);
@@ skipping 33 matching lines @@
       new em::PolicyFetchResponse(policy));
   EnsurePolicyKeyLoaded(
       base::Bind(&UserCloudPolicyStoreChromeOS::ValidatePolicyForStore,
                  weak_factory_.GetWeakPtr(),
                  base::Passed(&response)));
 }
 
 void UserCloudPolicyStoreChromeOS::Load() {
   // Cancel all pending requests.
   weak_factory_.InvalidateWeakPtrs();
-  session_manager_client_->RetrievePolicyForUser(
+  session_manager_client_->RetrievePolicyForUserWithErrorCallback(
       cryptohome::Identification(account_id_),
       base::Bind(&UserCloudPolicyStoreChromeOS::OnPolicyRetrieved,
+                 weak_factory_.GetWeakPtr()),
+      base::Bind(&UserCloudPolicyStoreChromeOS::OnPolicyRetrievedWithError,
                  weak_factory_.GetWeakPtr()));
 }
 
 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
@@ skipping 126 matching lines @@
   // Load |cached_policy_key_| to verify the loaded policy.
   if (is_active_directory_) {
     ValidateRetrievedPolicy(std::move(policy));
   } else {
     EnsurePolicyKeyLoaded(
         base::Bind(&UserCloudPolicyStoreChromeOS::ValidateRetrievedPolicy,
                    weak_factory_.GetWeakPtr(), base::Passed(&policy)));
   }
 }
 
+void UserCloudPolicyStoreChromeOS::OnPolicyRetrievedWithError(
+    const std::string& error_name,
+    const std::string& error_message) {
+  LOG(ERROR) << "Error on policy retrieved " << error_name << ":"

[Daniel Erat, 2017/04/10 18:59:26]

add space after ':'

[igorcov, 2017/04/18 10:23:18]

Done.

+             << error_message;
+  // Disallow the sign in when the error is dbus_error::kSessionDoesNotExist
+  // from Chrome OS.

[Daniel Erat, 2017/04/10 18:59:26]

this is chrome-os-only code, so you should probably write "from session_manager"
here instead

[igorcov, 2017/04/18 10:23:18]

Done.

+  // TODO(igorcov): crbug/689206. Find the root cause for the behavior that
+  // makes Chrome request the user policy before the session is started.
+  if (error_name == kSessionDoesNotExist) {
+    chrome::AttemptUserExit();

[Daniel Erat, 2017/04/10 18:59:26]

should chrome crash instead, or will that put us into a restart loop?

signing out silently is going to be confusing for users, i think.

[Andrew T Wilson (Slow), 2017/04/11 13:35:04]

Signing out silently can indeed be confusing for users, but this is what we've
done in other places where assumptions around loading policy have been violated.
The idea is that the situation is rare enough that just signing out the user is
sufficient (we don't need to build and maintain more elaborate UI).

I would suggest maybe adding a UMA stat though for how often this routine is
called and how often the value is kSessionDoesNotExist.

+    return;
+  }
+
+  status_ = STATUS_PARSE_ERROR;
+  NotifyStoreError();
+}
+
 void UserCloudPolicyStoreChromeOS::ValidateRetrievedPolicy(
     std::unique_ptr<em::PolicyFetchResponse> policy) {
   // Create and configure a validator for the loaded policy.
   std::unique_ptr<UserCloudPolicyValidator> validator =
       CreateValidatorForLoad(std::move(policy));
   // Start validation. The Validator will delete itself once validation is
   // complete.
   validator.release()->StartValidation(
       base::Bind(&UserCloudPolicyStoreChromeOS::OnRetrievedPolicyValidated,
                  weak_factory_.GetWeakPtr()));
@@ skipping 119 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