Abandon user sign in when policy is retrieved before session started

chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc

Index: chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc
diff --git a/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc b/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc
index f73ec0ceb7924b4cee34ba619a98db1525294b92..e299227f1895e3edc29e13b3293f1dcbf3772d0b 100644
--- a/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc
+++ b/chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc
@@ -19,6 +19,7 @@
 #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"
@@ -41,6 +42,9 @@ const base::FilePath::CharType kPolicyKeyFile[] =
 // 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,
@@ -94,9 +98,11 @@ void UserCloudPolicyStoreChromeOS::Store(
 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()));
 }
 
@@ -243,6 +249,24 @@ void UserCloudPolicyStoreChromeOS::OnPolicyRetrieved(
   }
 }
 
+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.