Abandon user sign in when policy is retrieved before session started

chromeos/dbus/session_manager_client.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 "chromeos/dbus/session_manager_client.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/files/file_path.h"
 #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"
 #include "base/task_scheduler/post_task.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chromeos/chromeos_paths.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
 #include "chromeos/dbus/blocking_method_caller.h"
 #include "chromeos/dbus/cryptohome_client.h"
 #include "components/policy/proto/device_management_backend.pb.h"
@@ skipping 39 matching lines @@
 // Helper to write a file in a background thread.
 void StoreFile(const base::FilePath& path, const std::string& data) {
   const int size = static_cast<int>(data.size());
   if (path.empty() ||
       !base::CreateDirectory(path.DirName()) ||
       base::WriteFile(path, data.data(), size) != size) {
     LOG(WARNING) << "Failed to write to " << path.value();
   }
 }
 
+// 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);
+}
+
 }  // namespace
 
 // The SessionManagerClient implementation used in production.
 class SessionManagerClientImpl : public SessionManagerClient {
  public:
   SessionManagerClientImpl()
       : session_manager_proxy_(NULL),
         screen_is_locked_(false),
         weak_ptr_factory_(this) {}
 
@@ skipping 108 matching lines @@
         dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
         base::Bind(&SessionManagerClientImpl::OnRetrieveActiveSessions,
                    weak_ptr_factory_.GetWeakPtr(),
                    login_manager::kSessionManagerRetrieveActiveSessions,
                    callback));
   }
 
   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(), callback));
   }
 
   std::string BlockingRetrieveDevicePolicy() override {
     dbus::MethodCall method_call(login_manager::kSessionManagerInterface,
                                  login_manager::kSessionManagerRetrievePolicy);
     std::unique_ptr<dbus::Response> response =
         blocking_method_caller_->CallMethodAndBlock(&method_call);
     std::string policy;
     ExtractString(login_manager::kSessionManagerRetrievePolicy, response.get(),
                   &policy);
@@ skipping 21 matching lines @@
                   response.get(),
                   &policy);
     return policy;
   }
 
   void RetrieveDeviceLocalAccountPolicy(
       const std::string& account_name,
       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 =
@@ skipping 223 matching lines @@
   }
 
   // Helper for RetrieveDeviceLocalAccountPolicy and RetrievePolicyForUser.
   void CallRetrievePolicyByUsername(const std::string& method_name,
                                     const std::string& account_id,
                                     const RetrievePolicyCallback& callback) {
     dbus::MethodCall method_call(login_manager::kSessionManagerInterface,
                                  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(), callback));
   }
 
   void CallStorePolicyByUsername(const std::string& method_name,
                                  const std::string& account_id,
                                  const std::string& policy_blob,
                                  const StorePolicyCallback& callback) {
     dbus::MethodCall method_call(login_manager::kSessionManagerInterface,
                                  method_name);
     dbus::MessageWriter writer(&method_call);
     writer.AppendString(account_id);
@@ skipping 91 matching lines @@
     size_t length = 0;
     if (!reader.PopArrayOfBytes(&values, &length)) {
       LOG(ERROR) << "Invalid response: " << response->ToString();
       return;
     }
     // static_cast does not work due to signedness.
     extracted->assign(reinterpret_cast<const char*>(values), length);
   }
 
   // Called when kSessionManagerRetrievePolicy or
   // kSessionManagerRetrievePolicyForUser method is complete.

[emaxx, 2017/04/18 15:03:17]

nit: Maybe change "complete" to "successfully complete"?

[igorcov, 2017/04/20 14:52:29]

Done.

-  void OnRetrievePolicy(const std::string& method_name,
-                        const RetrievePolicyCallback& callback,
-                        dbus::Response* response) {
+  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);
+
+    UMA_HISTOGRAM_ENUMERATION("Enterprise.RetrievePolicyResponse", SUCCESS,
+                              RETRIEVE_POLICY_RESPONSE_TYPE_COUNT);
+  }
+
+  void OnRetrievePolicyError(const RetrievePolicyCallback& callback,

[emaxx, 2017/04/18 15:03:17]

nit: Please add a comment for this method.

[igorcov, 2017/04/20 14:52:29]

Done.

+                             dbus::ErrorResponse* response) {
+    RetrievePolicyResponseType response_type = OTHER_ERROR;
+    if (response->GetErrorName() ==
+        login_manager::dbus_error::kSessionDoesNotExist) {
+      response_type = SESSION_DOES_NOT_EXIST;
+    }
+    callback.Run(std::string(), response_type);
+
+    UMA_HISTOGRAM_ENUMERATION("Enterprise.RetrievePolicyResponse",
+                              response_type,
+                              RETRIEVE_POLICY_RESPONSE_TYPE_COUNT);
   }
 
   // Called when kSessionManagerStorePolicy or kSessionManagerStorePolicyForUser
   // method is complete.
   void OnStorePolicy(const std::string& method_name,
                      const StorePolicyCallback& callback,
                      dbus::Response* response) {
     bool success = false;
     if (!response) {
       LOG(ERROR) << "Failed to call " << method_name;
@@ skipping 220 matching lines @@
   void NotifyLockScreenDismissed() override {
     screen_is_locked_ = false;
     for (auto& observer : observers_)
       observer.ScreenIsUnlocked();
   }
   void RetrieveActiveSessions(const ActiveSessionsCallback& callback) override {
   }
   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 {
     base::FilePath owner_key_path;
     if (!PathService::Get(chromeos::FILE_OWNER_KEY, &owner_key_path)) {
       return "";
     }
     base::FilePath device_policy_path =
         owner_key_path.DirName().AppendASCII(kStubDevicePolicyFile);
     return GetFileContent(device_policy_path);
   }
   void RetrievePolicyForUser(const cryptohome::Identification& cryptohome_id,
                              const RetrievePolicyCallback& callback) override {
     base::PostTaskWithTraitsAndReplyWithResult(
         FROM_HERE,
         base::TaskTraits()
             .WithShutdownBehavior(
                 base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)
             .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));
   }
   void RetrieveDeviceLocalAccountPolicy(
       const std::string& account_id,
       const RetrievePolicyCallback& callback) override {
     RetrievePolicyForUser(cryptohome::Identification::FromString(account_id),
                           callback);
@@ skipping 147 matching lines @@
 
 SessionManagerClient* SessionManagerClient::Create(
     DBusClientImplementationType type) {
   if (type == REAL_DBUS_CLIENT_IMPLEMENTATION)
     return new SessionManagerClientImpl();
   DCHECK_EQ(FAKE_DBUS_CLIENT_IMPLEMENTATION, type);
   return new SessionManagerClientStubImpl();
 }
 
 }  // namespace chromeos