Abandon user sign in when policy is retrieved before session started

chromeos/dbus/session_manager_client.cc

Index: chromeos/dbus/session_manager_client.cc
diff --git a/chromeos/dbus/session_manager_client.cc b/chromeos/dbus/session_manager_client.cc
index 90322ff80789326d2542afd306515200c97449c8..fbf34447da3704605f2862bbe41102183578532b 100644
--- a/chromeos/dbus/session_manager_client.cc
+++ b/chromeos/dbus/session_manager_client.cc
@@ -15,6 +15,7 @@
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/macros.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ -30,16 +31,13 @@
 #include "dbus/message.h"
 #include "dbus/object_path.h"
 #include "dbus/object_proxy.h"
+#include "dbus/scoped_dbus_error.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
 namespace {
 
-// TODO(hidehiko): Share the constant between Chrome and ChromeOS.
-constexpr char kArcLowDiskError[] =
-    "org.chromium.SessionManagerInterface.LowFreeDisk";
-
 constexpr char kStubPolicyFile[] = "stub_policy";
 constexpr char kStubDevicePolicyFile[] = "stub_device_policy";
 constexpr char kStubStateKeysFile[] = "stub_state_keys";
@@ -103,6 +101,51 @@ void RunStateKeysCallbackStub(SessionManagerClient::StateKeysCallback callback,
   callback.Run(state_keys);
 }
 
+// Helper to notify the callback with SUCCESS, to be used by the stub.
+void NotifyOnRetrievePolicySuccess(
+    const SessionManagerClient::RetrievePolicyCallback& callback,
+    const std::string& protobuf) {
+  callback.Run(protobuf, SessionManagerClient::SUCCESS);
+}
+
+// Helper to get the enum type of RetrievePolicyResponseType based on error
+// name.
+SessionManagerClient::RetrievePolicyResponseType GetResponseTypeBasedOnError(
+    const std::string& error_name) {
+  if (error_name == login_manager::dbus_error::kNone) {
+    return SessionManagerClient::SUCCESS;
+  } else if (error_name == login_manager::dbus_error::kSessionDoesNotExist) {

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

nit: Sorry for not pointing this out before, but it's recommended to omit "else"
after "return":
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

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

Done.

+    return SessionManagerClient::SESSION_DOES_NOT_EXIST;
+  } else if (error_name == login_manager::dbus_error::kSigEncodeFail) {
+    return SessionManagerClient::SIG_ENCODE_FAIL;
+  } else {
+    return SessionManagerClient::OTHER_ERROR;
+  }
+}
+
+// Logs UMA stat for retrieve policy request, corresponding to D-Bus method name
+// used.
+void LogUma(const std::string& method_name,

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

nit: Give this function a more specific name, e.g. LogPolicyResponseUma.

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

Done.

+            SessionManagerClient::RetrievePolicyResponseType response) {
+  std::string stat_name;
+  if (method_name ==
+      login_manager::kSessionManagerRetrieveDeviceLocalAccountPolicy) {
+    stat_name = "RetrieveDeviceLocalAccountPolicyResponse";
+  } else if (method_name == login_manager::kSessionManagerRetrievePolicy) {
+    stat_name = "RetrieveDevicePolicyResponse";
+  } else if (method_name ==
+             login_manager::kSessionManagerRetrievePolicyForUser) {
+    stat_name = "RetrieveUserPolicyResponse";
+  } else {
+    LOG(ERROR) << "Invalid method_name: " << method_name;
+    return;
+  }
+
+  UMA_HISTOGRAM_ENUMERATION(
+      "Enterprise." + stat_name, response,

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

I doubt that it will work as expected. The |name| parameter of the macro is
expected to be a constant, see here:
https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?l=23
// All of these macros must be called with |name| as a runtime constant - it
// doesn't have to literally be a constant, but it must be the same string on
// all calls from a particular call site.

Therefore the typical pattern I believe is to have a separate invocation of
UMA_HISTOGRAM_* macro for each name. (It's possible to bypass these issues by
using factory methods of base::Histogram, but this sacrifices the performance.)


It'll be also useful to test in the end that everything works as expected by
looking at chrome://histograms.

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

Changed to have separate calls. Thanks.

+      SessionManagerClient::RETRIEVE_POLICY_RESPONSE_TYPE_COUNT);
+}
+
 }  // namespace
 
 // The SessionManagerClient implementation used in production.
@@ -231,24 +274,34 @@ class SessionManagerClientImpl : public SessionManagerClient {
   void RetrieveDevicePolicy(const RetrievePolicyCallback& callback) override {
     dbus::MethodCall method_call(login_manager::kSessionManagerInterface,
                                  login_manager::kSessionManagerRetrievePolicy);
-    session_manager_proxy_->CallMethod(
-        &method_call,
-        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::Bind(&SessionManagerClientImpl::OnRetrievePolicy,
+    session_manager_proxy_->CallMethodWithErrorCallback(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&SessionManagerClientImpl::OnRetrievePolicySuccess,
                    weak_ptr_factory_.GetWeakPtr(),
-                   login_manager::kSessionManagerRetrievePolicy,
-                   callback));
+                   login_manager::kSessionManagerRetrievePolicy, callback),
+        base::Bind(&SessionManagerClientImpl::OnRetrievePolicyError,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   login_manager::kSessionManagerRetrievePolicy, callback));
   }
 
-  std::string BlockingRetrieveDevicePolicy() override {
+  RetrievePolicyResponseType BlockingRetrieveDevicePolicy(
+      std::string* policy) override {
     dbus::MethodCall method_call(login_manager::kSessionManagerInterface,
                                  login_manager::kSessionManagerRetrievePolicy);
+    dbus::ScopedDBusError error;
     std::unique_ptr<dbus::Response> response =
-        blocking_method_caller_->CallMethodAndBlock(&method_call);
-    std::string policy;
-    ExtractString(login_manager::kSessionManagerRetrievePolicy, response.get(),
-                  &policy);
-    return policy;
+        blocking_method_caller_->CallMethodAndBlockWithError(&method_call,
+                                                             &error);
+    RetrievePolicyResponseType result = RetrievePolicyResponseType::SUCCESS;
+    if (error.is_set() && error.name()) {
+      result = GetResponseTypeBasedOnError(error.name());
+    }
+    if (result == RetrievePolicyResponseType::SUCCESS) {
+      ExtractString(login_manager::kSessionManagerRetrievePolicy,
+                    response.get(), policy);
+    }
+    LogUma(login_manager::kSessionManagerRetrievePolicy, result);
+    return result;
   }
 
   void RetrievePolicyForUser(const cryptohome::Identification& cryptohome_id,
@@ -258,20 +311,12 @@ class SessionManagerClientImpl : public SessionManagerClient {
         callback);
   }
 
-  std::string BlockingRetrievePolicyForUser(
-      const cryptohome::Identification& cryptohome_id) override {
-    dbus::MethodCall method_call(
-        login_manager::kSessionManagerInterface,
-        login_manager::kSessionManagerRetrievePolicyForUser);
-    dbus::MessageWriter writer(&method_call);
-    writer.AppendString(cryptohome_id.id());
-    std::unique_ptr<dbus::Response> response =
-        blocking_method_caller_->CallMethodAndBlock(&method_call);
-    std::string policy;
-    ExtractString(login_manager::kSessionManagerRetrievePolicyForUser,
-                  response.get(),
-                  &policy);
-    return policy;
+  RetrievePolicyResponseType BlockingRetrievePolicyForUser(
+      const cryptohome::Identification& cryptohome_id,
+      std::string* policy) override {
+    return BlockingRetrievePolicyByUsername(
+        login_manager::kSessionManagerRetrievePolicyForUser, cryptohome_id.id(),
+        policy);
   }
 
   void RetrieveDeviceLocalAccountPolicy(
@@ -279,24 +324,15 @@ class SessionManagerClientImpl : public SessionManagerClient {
       const RetrievePolicyCallback& callback) override {
     CallRetrievePolicyByUsername(
         login_manager::kSessionManagerRetrieveDeviceLocalAccountPolicy,
-        account_name,
-        callback);
+        account_name, callback);
   }
 
-  std::string BlockingRetrieveDeviceLocalAccountPolicy(
-      const std::string& account_name) override {
-    dbus::MethodCall method_call(
-        login_manager::kSessionManagerInterface,
-        login_manager::kSessionManagerRetrieveDeviceLocalAccountPolicy);
-    dbus::MessageWriter writer(&method_call);
-    writer.AppendString(account_name);
-    std::unique_ptr<dbus::Response> response =
-        blocking_method_caller_->CallMethodAndBlock(&method_call);
-    std::string policy;
-    ExtractString(
+  RetrievePolicyResponseType BlockingRetrieveDeviceLocalAccountPolicy(
+      const std::string& account_name,
+      std::string* policy) override {
+    return BlockingRetrievePolicyByUsername(
         login_manager::kSessionManagerRetrieveDeviceLocalAccountPolicy,
-        response.get(), &policy);
-    return policy;
+        account_name, policy);
   }
 
   void StoreDevicePolicy(const std::string& policy_blob,
@@ -524,14 +560,37 @@ class SessionManagerClientImpl : public SessionManagerClient {
                                  method_name);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(account_id);
-    session_manager_proxy_->CallMethod(
-        &method_call,
-        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::Bind(
-            &SessionManagerClientImpl::OnRetrievePolicy,
-            weak_ptr_factory_.GetWeakPtr(),
-            method_name,
-            callback));
+    session_manager_proxy_->CallMethodWithErrorCallback(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&SessionManagerClientImpl::OnRetrievePolicySuccess,
+                   weak_ptr_factory_.GetWeakPtr(), method_name, callback),
+        base::Bind(&SessionManagerClientImpl::OnRetrievePolicyError,
+                   weak_ptr_factory_.GetWeakPtr(), method_name, callback));
+  }
+
+  // Helper for blocking RetrievePolicyForUser and
+  // RetrieveDeviceLocalAccountPolicy.
+  RetrievePolicyResponseType BlockingRetrievePolicyByUsername(
+      const std::string& method_name,
+      const std::string& account_name,
+      std::string* policy) {
+    dbus::MethodCall method_call(login_manager::kSessionManagerInterface,
+                                 method_name);
+    dbus::MessageWriter writer(&method_call);
+    writer.AppendString(account_name);
+    dbus::ScopedDBusError error;
+    std::unique_ptr<dbus::Response> response =
+        blocking_method_caller_->CallMethodAndBlockWithError(&method_call,
+                                                             &error);
+    RetrievePolicyResponseType result = RetrievePolicyResponseType::SUCCESS;
+    if (error.is_set() && error.name()) {
+      result = GetResponseTypeBasedOnError(error.name());
+    }
+    if (result == RetrievePolicyResponseType::SUCCESS) {
+      ExtractString(method_name, response.get(), policy);
+    }
+    LogUma(method_name, result);
+    return result;
   }
 
   void CallStorePolicyByUsername(const std::string& method_name,
@@ -643,13 +702,27 @@ class SessionManagerClientImpl : public SessionManagerClient {
   }
 
   // Called when kSessionManagerRetrievePolicy or
-  // kSessionManagerRetrievePolicyForUser method is complete.
-  void OnRetrievePolicy(const std::string& method_name,
-                        const RetrievePolicyCallback& callback,
-                        dbus::Response* response) {
+  // kSessionManagerRetrievePolicyForUser method is successfully complete.
+  void OnRetrievePolicySuccess(const std::string& method_name,
+                               const RetrievePolicyCallback& callback,
+                               dbus::Response* response) {
     std::string serialized_proto;
     ExtractString(method_name, response, &serialized_proto);
-    callback.Run(serialized_proto);
+    callback.Run(serialized_proto, SUCCESS);
+
+    LogUma(method_name, SUCCESS);
+  }
+
+  // Called when kSessionManagerRetrievePolicy or
+  // kSessionManagerRetrievePolicyForUser method fails.
+  void OnRetrievePolicyError(const std::string& method_name,
+                             const RetrievePolicyCallback& callback,
+                             dbus::ErrorResponse* response) {
+    RetrievePolicyResponseType response_type =
+        GetResponseTypeBasedOnError(response->GetErrorName());
+    callback.Run(std::string(), response_type);
+
+    LogUma(method_name, response_type);
   }
 
   // Called when kSessionManagerStorePolicy or kSessionManagerStorePolicyForUser
@@ -821,7 +894,8 @@ class SessionManagerClientImpl : public SessionManagerClient {
     LOG(ERROR) << "Failed to call StartArcInstance: "
                << (response ? response->ToString() : "(null)");
     if (!callback.is_null())

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

not your fault, but please add curly brackets here

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

Done.

-      callback.Run(response && response->GetErrorName() == kArcLowDiskError
+      callback.Run(response && response->GetErrorName() ==
+                                   login_manager::dbus_error::kLowFreeDisk
                        ? StartArcInstanceResult::LOW_FREE_DISK_SPACE
                        : StartArcInstanceResult::UNKNOWN_ERROR);
   }
@@ -890,26 +964,31 @@ class SessionManagerClientStubImpl : public SessionManagerClient {
   void RetrieveDevicePolicy(const RetrievePolicyCallback& callback) override {
     base::FilePath owner_key_path;
     if (!PathService::Get(chromeos::FILE_OWNER_KEY, &owner_key_path)) {
-      callback.Run("");
+      callback.Run("", SUCCESS);
       return;
     }
     base::FilePath device_policy_path =
         owner_key_path.DirName().AppendASCII(kStubDevicePolicyFile);
     base::PostTaskWithTraitsAndReplyWithResult(
-        FROM_HERE, base::TaskTraits()
-                       .WithShutdownBehavior(
-                           base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)
-                       .MayBlock(),
-        base::Bind(&GetFileContent, device_policy_path), callback);
+        FROM_HERE,
+        base::TaskTraits()
+            .WithShutdownBehavior(
+                base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)
+            .MayBlock(),
+        base::Bind(&GetFileContent, device_policy_path),
+        base::Bind(&NotifyOnRetrievePolicySuccess, callback));
   }
-  std::string BlockingRetrieveDevicePolicy() override {
+  RetrievePolicyResponseType BlockingRetrieveDevicePolicy(
+      std::string* policy) override {
     base::FilePath owner_key_path;
     if (!PathService::Get(chromeos::FILE_OWNER_KEY, &owner_key_path)) {
-      return "";
+      *policy = "";
+      return SUCCESS;
     }
     base::FilePath device_policy_path =
         owner_key_path.DirName().AppendASCII(kStubDevicePolicyFile);
-    return GetFileContent(device_policy_path);
+    *policy = GetFileContent(device_policy_path);
+    return SUCCESS;
   }
   void RetrievePolicyForUser(const cryptohome::Identification& cryptohome_id,
                              const RetrievePolicyCallback& callback) override {
@@ -921,11 +1000,13 @@ class SessionManagerClientStubImpl : public SessionManagerClient {
             .MayBlock(),
         base::Bind(&GetFileContent,
                    GetUserFilePath(cryptohome_id, kStubPolicyFile)),
-        callback);
+        base::Bind(&NotifyOnRetrievePolicySuccess, callback));
   }
-  std::string BlockingRetrievePolicyForUser(
-      const cryptohome::Identification& cryptohome_id) override {
-    return GetFileContent(GetUserFilePath(cryptohome_id, kStubPolicyFile));
+  RetrievePolicyResponseType BlockingRetrievePolicyForUser(
+      const cryptohome::Identification& cryptohome_id,
+      std::string* policy) override {
+    *policy = GetFileContent(GetUserFilePath(cryptohome_id, kStubPolicyFile));
+    return SUCCESS;
   }
   void RetrieveDeviceLocalAccountPolicy(
       const std::string& account_id,
@@ -933,10 +1014,11 @@ class SessionManagerClientStubImpl : public SessionManagerClient {
     RetrievePolicyForUser(cryptohome::Identification::FromString(account_id),
                           callback);
   }
-  std::string BlockingRetrieveDeviceLocalAccountPolicy(
-      const std::string& account_id) override {
+  RetrievePolicyResponseType BlockingRetrieveDeviceLocalAccountPolicy(
+      const std::string& account_id,
+      std::string* policy) override {
     return BlockingRetrievePolicyForUser(
-        cryptohome::Identification::FromString(account_id));
+        cryptohome::Identification::FromString(account_id), policy);
   }
   void StoreDevicePolicy(const std::string& policy_blob,
                          const StorePolicyCallback& callback) override {