Revert "Refactor BrowserDMTokenStorage for easier platform implementations."

This reverts commit 1c479eb8.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> Refactor BrowserDMTokenStorage for easier platform implementations.
> 
> This CL refactors BrowserDMTokenStorage & BrowserDMTokenStorageWin to move code that is common to all platforms.
> 
> After this CL, adding a new platform requires implementing the following methods:
> 
> -  virtual std::string InitClientId();
> -  virtual std::string InitEnrollmentToken();
> -  virtual std::string InitDMToken();
> -  virtual void SaveDMToken(const std::string& token);
> 
> Note that this CL keeps the existing Windows implementation, it only refactors the code (no functional change).
> 
> BUG=812641
> 
> Change-Id: If362c4368160e7bd038a8a4b1d93e151c0a60e1b
> Reviewed-on: https://chromium-review.googlesource.com/1133315
> Commit-Queue: Georges Khalil <georgesak@chromium.org>
> Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
> Reviewed-by: Roger Tawa <rogerta@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#575276}

TBR=rogerta@chromium.org,pastarmovj@chromium.org,georgesak@chromium.org,zmin@chromium.org

Change-Id: Idbf78da964051aaac710b780fabccf248c8d583e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 812641
Reviewed-on: https://chromium-review.googlesource.com/1138513Reviewed-by: Owen Min <zmin@chromium.org>
Commit-Queue: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575281}

chrome/browser/BUILD.gn

@@ -1056,7 +1056,6 @@ jumbo_split_static_library("browser") {
     "plugins/pdf_iframe_navigation_throttle.h",
     "plugins/pdf_plugin_placeholder_observer.cc",
     "plugins/pdf_plugin_placeholder_observer.h",
-    "policy/browser_dm_token_storage.cc",
     "policy/browser_dm_token_storage.h",
     "policy/browser_dm_token_storage_win.cc",
     "policy/browser_dm_token_storage_win.h",
@@ -3164,6 +3163,8 @@ jumbo_split_static_library("browser") {
   } else {
     # Non-Windows.
     sources += [
+      "policy/browser_dm_token_storage_stub.cc",
+      "policy/browser_dm_token_storage_stub.h",
       "profile_resetter/triggered_profile_resetter_stub.cc",
       "profiles/profile_shortcut_manager_stub.cc",
     ]

chrome/browser/policy/browser_dm_token_storage.cc

-// Copyright 2018 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/policy/browser_dm_token_storage.h"
-
-#include <memory>
-#include <utility>
-#include <vector>
-
-#include "base/base64.h"
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/callback.h"
-#include "base/logging.h"
-#include "base/no_destructor.h"
-#include "base/strings/string16.h"
-#include "base/strings/utf_string_conversions.h"
-#include "base/task_runner_util.h"
-#include "base/task_scheduler/post_task.h"
-#include "base/threading/sequenced_task_runner_handle.h"
-#include "base/threading/thread_task_runner_handle.h"
-#include "build/build_config.h"
-#include "content/public/browser/browser_thread.h"
-
-namespace policy {
-
-// static
-BrowserDMTokenStorage* BrowserDMTokenStorage::storage_for_testing_ = nullptr;
-
-// Static function that can't be overridden. Implementation is only compiled for
-// non-supported platforms.
-#if !defined(OS_WIN)
-// static
-BrowserDMTokenStorage* BrowserDMTokenStorage::Get() {
-  if (storage_for_testing_)
-    return storage_for_testing_;
-
-  static base::NoDestructor<BrowserDMTokenStorage> storage;
-  return storage.get();
-}
-#endif
-
-BrowserDMTokenStorage::BrowserDMTokenStorage() : is_initialized_(false) {
-  DETACH_FROM_SEQUENCE(sequence_checker_);
-
-  // We don't call InitIfNeeded() here so that the global instance can be
-  // created early during startup if needed. The tokens and client ID are read
-  // from the system as part of the first retrieve or store operation.
-}
-
-BrowserDMTokenStorage::~BrowserDMTokenStorage() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-}
-
-std::string BrowserDMTokenStorage::RetrieveClientId() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  InitIfNeeded();
-  return client_id_;
-}
-
-std::string BrowserDMTokenStorage::RetrieveEnrollmentToken() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  InitIfNeeded();
-  return enrollment_token_;
-}
-
-void BrowserDMTokenStorage::StoreDMToken(const std::string& dm_token,
-                                         StoreCallback callback) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(!store_callback_);
-  InitIfNeeded();
-
-  dm_token_ = dm_token;
-
-  store_callback_ = std::move(callback);
-
-  SaveDMToken(dm_token);
-}
-
-std::string BrowserDMTokenStorage::RetrieveDMToken() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(!store_callback_);
-
-  InitIfNeeded();
-  return dm_token_;
-}
-
-void BrowserDMTokenStorage::InitIfNeeded() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  if (is_initialized_)
-    return;
-
-  is_initialized_ = true;
-  client_id_ = InitClientId();
-  DVLOG(1) << "Client ID = " << client_id_;
-  if (client_id_.empty())
-    return;
-
-  enrollment_token_ = InitEnrollmentToken();
-  DVLOG(1) << "Enrollment token = " << enrollment_token_;
-
-  dm_token_ = InitDMToken();
-  DVLOG(1) << "DM Token = " << dm_token_;
-}
-
-void BrowserDMTokenStorage::OnDMTokenStored(bool success) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(store_callback_);
-
-  if (!store_callback_.is_null())
-    std::move(store_callback_).Run(success);
-}
-
-// Stub implementation. This function will become virtual pure once Mac & Linux
-// implementations are done.
-std::string BrowserDMTokenStorage::InitClientId() {
-  return std::string();
-}
-
-// Stub implementation. This function will become virtual pure once Mac & Linux
-// implementations are done.
-std::string BrowserDMTokenStorage::InitEnrollmentToken() {
-  return std::string();
-}
-
-// Stub implementation. This function will become virtual pure once Mac & Linux
-// implementations are done.
-std::string BrowserDMTokenStorage::InitDMToken() {
-  return std::string();
-}
-
-// Stub implementation. This function will become virtual pure once Mac & Linux
-// implementations are done.
-void BrowserDMTokenStorage::SaveDMToken(const std::string& token) {
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(store_callback_), false));
-}
-
-}  // namespace policy

chrome/browser/policy/browser_dm_token_storage.h

@@ -10,11 +10,6 @@
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
-#include "base/memory/scoped_refptr.h"
-#include "base/memory/weak_ptr.h"
-#include "base/no_destructor.h"
-#include "base/sequence_checker.h"
-#include "base/single_thread_task_runner.h"
 
 namespace policy {
 
@@ -30,22 +25,19 @@ class BrowserDMTokenStorage {
 
   // Returns the global singleton object. Must be called from the UI thread.
   static BrowserDMTokenStorage* Get();
-  // Returns a client ID unique to the machine. Virtual for tests.
-  virtual std::string RetrieveClientId();
-  // Returns the enrollment token, or an empty string if there is none. Virtual
-  // for tests.
-  virtual std::string RetrieveEnrollmentToken();
-  // Asynchronously stores |dm_token| and calls |callback| with a boolean to
-  // indicate success or failure. It is an error to attempt concurrent store
-  // operations. Virtual for tests.
+  // Returns a client ID unique to the machine.
+  virtual std::string RetrieveClientId() = 0;
+  // Returns the enrollment token, or an empty string if there is none.
+  virtual std::string RetrieveEnrollmentToken() = 0;
+  // Asynchronously stores |dm_token| in the registry and calls |callback| with
+  // a boolean to indicate success or failure. It is an error to attempt
+  // concurrent store operations.
   virtual void StoreDMToken(const std::string& dm_token,
-                            StoreCallback callback);
-  // Returns an already stored DM token. An empty token is returned if no DM
-  // token exists on the system or an error is encountered. Virtual for tests.
-  virtual std::string RetrieveDMToken();
-  // Must be called after the DM token is saved, to ensure that the callback is
-  // invoked.
-  void OnDMTokenStored(bool success);
+                            StoreCallback callback) = 0;
+  // Returns an already stored DM token from the registry or from the cache in
+  // memory. An empty token is returned if no DM token exists on the system or
+  // an error is encountered.
+  virtual std::string RetrieveDMToken() = 0;
 
   // Set the mock BrowserDMTokenStorage for testing. The caller owns the
   // instance of the storage.
@@ -54,42 +46,12 @@ class BrowserDMTokenStorage {
   }
 
  protected:
-  friend class base::NoDestructor<BrowserDMTokenStorage>;
-
-  // Get the global singleton instance by calling BrowserDMTokenStorage::Get().
-  BrowserDMTokenStorage();
-  virtual ~BrowserDMTokenStorage();
+  BrowserDMTokenStorage() = default;
+  virtual ~BrowserDMTokenStorage() = default;
 
  private:
   static BrowserDMTokenStorage* storage_for_testing_;
 
-  // Initializes the DMTokenStorage object and caches the ids and tokens. This
-  // is called the first time the BrowserDMTokenStorage is interacted with.
-  void InitIfNeeded();
-
-  // Gets the client ID and stores it in |client_id_|. This implementation is
-  // platform dependant.
-  virtual std::string InitClientId();
-  // Gets the enrollment token and stores it in |enrollment_token_|. This
-  // implementation is platform dependant.
-  virtual std::string InitEnrollmentToken();
-  // Gets the DM token and stores it in |dm_token_|. This implementation is
-  // platform dependant.
-  virtual std::string InitDMToken();
-  // Saves the DM token. This implementation is platform dependant.
-  virtual void SaveDMToken(const std::string& token);
-
-  // Will be called after the DM token is stored.
-  StoreCallback store_callback_;
-
-  bool is_initialized_;
-
-  std::string client_id_;
-  std::string enrollment_token_;
-  std::string dm_token_;
-
-  SEQUENCE_CHECKER(sequence_checker_);
-
   DISALLOW_COPY_AND_ASSIGN(BrowserDMTokenStorage);
 };
 

chrome/browser/policy/browser_dm_token_storage_stub.cc

+// Copyright 2018 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/policy/browser_dm_token_storage_stub.h"
+
+#include <memory>
+#include <utility>
+
+#include "base/callback.h"
+#include "base/threading/thread_task_runner_handle.h"
+
+namespace policy {
+
+namespace {
+BrowserDMTokenStorage* g_browser_dm_token_storage = nullptr;
+}  // namespace
+
+// static
+BrowserDMTokenStorage* BrowserDMTokenStorage::storage_for_testing_ = nullptr;
+
+// static
+BrowserDMTokenStorage* BrowserDMTokenStorage::Get() {
+  if (storage_for_testing_)
+    return storage_for_testing_;
+  if (g_browser_dm_token_storage == nullptr)
+    g_browser_dm_token_storage = new BrowserDMTokenStorageStub();
+  return g_browser_dm_token_storage;
+}
+
+std::string BrowserDMTokenStorageStub::RetrieveClientId() {
+  return std::string();
+}
+
+std::string BrowserDMTokenStorageStub::RetrieveEnrollmentToken() {
+  return std::string();
+}
+
+void BrowserDMTokenStorageStub::StoreDMToken(const std::string& dm_token,
+                                             StoreCallback callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback), false));
+}
+
+std::string BrowserDMTokenStorageStub::RetrieveDMToken() {
+  return std::string();
+}
+
+}  // namespace policy

chrome/browser/policy/browser_dm_token_storage_stub.h

+// Copyright 2018 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.
+
+#ifndef CHROME_BROWSER_POLICY_BROWSER_DM_TOKEN_STORAGE_STUB_H_
+#define CHROME_BROWSER_POLICY_BROWSER_DM_TOKEN_STORAGE_STUB_H_
+
+#include "chrome/browser/policy/browser_dm_token_storage.h"
+
+#include <string>
+
+#include "base/macros.h"
+
+namespace policy {
+
+// No-op implementation of BrowserDMTokenStorage. The global singleton instance
+// can be retrieved by calling BrowserDMTokenStorage::Get().
+class BrowserDMTokenStorageStub : public BrowserDMTokenStorage {
+ public:
+  // Get the global singleton instance by calling BrowserDMTokenStorage::Get().
+  BrowserDMTokenStorageStub() = default;
+  ~BrowserDMTokenStorageStub() override = default;
+
+  // override BrowserDMTokenStorage
+  std::string RetrieveClientId() override;
+  std::string RetrieveEnrollmentToken() override;
+  void StoreDMToken(const std::string& dm_token,
+                    StoreCallback callback) override;
+  std::string RetrieveDMToken() override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BrowserDMTokenStorageStub);
+};
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_POLICY_BROWSER_DM_TOKEN_STORAGE_STUB_H_

chrome/browser/policy/browser_dm_token_storage_unittest.cc

-// Copyright 2018 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/policy/browser_dm_token_storage.h"
-
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/macros.h"
-#include "base/run_loop.h"
-#include "base/threading/thread_task_runner_handle.h"
-#include "content/public/test/test_browser_thread_bundle.h"
-#include "testing/gmock/include/gmock/gmock.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-using ::testing::IsEmpty;
-
-namespace policy {
-
-namespace {
-
-constexpr char kClientId1[] = "fake-client-id-1";
-constexpr char kClientId2[] = "fake-client-id-2";
-constexpr char kEnrollmentToken1[] = "fake-enrollment-token-1";
-constexpr char kEnrollmentToken2[] = "fake-enrollment-token-2";
-constexpr char kDMToken1[] = "fake-dm-token-1";
-constexpr char kDMToken2[] = "fake-dm-token-2";
-
-}  // namespace
-
-class MockBrowserDMTokenStorage : public BrowserDMTokenStorage {
- public:
-  MockBrowserDMTokenStorage() {
-    set_test_client_id(kClientId1);
-    set_test_enrollment_token(kEnrollmentToken1);
-    set_test_dm_token(kDMToken1);
-  }
-
-  // BrowserDMTokenStorage override
-  std::string InitClientId() override { return test_client_id_; }
-  std::string InitEnrollmentToken() override { return test_enrollment_token_; }
-  std::string InitDMToken() override { return test_dm_token_; }
-
-  void set_test_client_id(std::string test_client_id) {
-    test_client_id_ = test_client_id;
-  }
-  void set_test_enrollment_token(std::string test_enrollment_token) {
-    test_enrollment_token_ = test_enrollment_token;
-  }
-  void set_test_dm_token(std::string test_dm_token) {
-    test_dm_token_ = test_dm_token;
-  }
-
- private:
-  std::string test_client_id_;
-  std::string test_enrollment_token_;
-  std::string test_dm_token_;
-};
-
-class BrowserDMTokenStorageTest : public testing::Test {
- private:
-  content::TestBrowserThreadBundle thread_bundle_;
-};
-
-TEST_F(BrowserDMTokenStorageTest, RetrieveClientId) {
-  MockBrowserDMTokenStorage storage;
-  EXPECT_EQ(kClientId1, storage.RetrieveClientId());
-
-  // The client ID value should be cached in memory and not read from the system
-  // again.
-  storage.set_test_client_id(kClientId2);
-  EXPECT_EQ(kClientId1, storage.RetrieveClientId());
-}
-
-TEST_F(BrowserDMTokenStorageTest, RetrieveEnrollmentToken) {
-  MockBrowserDMTokenStorage storage;
-  EXPECT_EQ(kEnrollmentToken1, storage.RetrieveEnrollmentToken());
-
-  // The enrollment token should be cached in memory and not read from the
-  // system again.
-  storage.set_test_enrollment_token(kEnrollmentToken2);
-  EXPECT_EQ(kEnrollmentToken1, storage.RetrieveEnrollmentToken());
-}
-
-TEST_F(BrowserDMTokenStorageTest, RetrieveDMToken) {
-  MockBrowserDMTokenStorage storage;
-  EXPECT_EQ(kDMToken1, storage.RetrieveDMToken());
-
-  // The DM token should be cached in memory and not read from the system again.
-  storage.set_test_dm_token(kDMToken2);
-  EXPECT_EQ(kDMToken1, storage.RetrieveDMToken());
-}
-
-class TestStoreDMTokenDelegate {
- public:
-  TestStoreDMTokenDelegate() : called_(false), success_(true) {}
-  ~TestStoreDMTokenDelegate() {}
-
-  void OnDMTokenStored(bool success) {
-    run_loop_.Quit();
-    called_ = true;
-    success_ = success;
-  }
-
-  bool WasCalled() {
-    bool was_called = called_;
-    called_ = false;
-    return was_called;
-  }
-
-  bool success() { return success_; }
-
-  void Wait() { run_loop_.Run(); }
-
- private:
-  bool called_;
-  bool success_;
-  base::RunLoop run_loop_;
-};
-
-TEST_F(BrowserDMTokenStorageTest, StoreDMToken) {
-  MockBrowserDMTokenStorage storage;
-  TestStoreDMTokenDelegate delegate;
-
-  storage.StoreDMToken(
-      kDMToken1, base::BindOnce(&TestStoreDMTokenDelegate::OnDMTokenStored,
-                                base::Unretained(&delegate)));
-
-  delegate.Wait();
-
-  EXPECT_TRUE(delegate.WasCalled());
-  EXPECT_FALSE(delegate.success());
-}
-
-}  // namespace policy

chrome/browser/policy/browser_dm_token_storage_win.cc

@@ -40,7 +40,81 @@
 #endif  // defined(GOOGLE_CHROME_BUILD)
 
 namespace policy {
+
 namespace {
+
+std::string GetClientId() {
+  // For the client id, use the Windows machine GUID.
+  // TODO(crbug.com/821977): Need a backup plan if machine GUID doesn't exist.
+  base::win::RegKey key;
+  LSTATUS status = key.Open(HKEY_LOCAL_MACHINE,
+                            L"SOFTWARE\\Microsoft\\Cryptography", KEY_READ);
+  if (status != ERROR_SUCCESS)
+    return std::string();
+
+  base::string16 value;
+  status = key.ReadValue(L"MachineGuid", &value);
+  if (status != ERROR_SUCCESS)
+    return std::string();
+
+  std::string client_id;
+  if (!base::WideToUTF8(value.c_str(), value.length(), &client_id))
+    return std::string();
+  return client_id;
+}
+
+std::string GetDMToken() {
+  base::win::RegKey key;
+  base::string16 dm_token_key_path;
+  base::string16 dm_token_value_name;
+  InstallUtil::GetMachineLevelUserCloudPolicyDMTokenRegistryPath(
+      &dm_token_key_path, &dm_token_value_name);
+  LONG result = key.Open(HKEY_LOCAL_MACHINE, dm_token_key_path.c_str(),
+                         KEY_QUERY_VALUE | KEY_WOW64_64KEY);
+  if (result != ERROR_SUCCESS)
+    return std::string();
+
+  // At the time of writing (January 2018), the DM token is about 200 bytes
+  // long. The initial size of the buffer should be enough to cover most
+  // realistic future size-increase scenarios, although we still make an effort
+  // to support somewhat larger token sizes just to be safe.
+  constexpr size_t kInitialDMTokenSize = 512;
+
+  DWORD size = kInitialDMTokenSize;
+  std::vector<char> raw_value(size);
+  DWORD dtype = REG_NONE;
+  result = key.ReadValue(dm_token_value_name.c_str(), raw_value.data(), &size,
+                         &dtype);
+  if (result == ERROR_MORE_DATA && size <= installer::kMaxDMTokenLength) {
+    raw_value.resize(size);
+    result = key.ReadValue(dm_token_value_name.c_str(), raw_value.data(), &size,
+                           &dtype);
+  }
+
+  if (result != ERROR_SUCCESS || dtype != REG_BINARY || size == 0) {
+    DVLOG(1) << "Failed to get DMToken from Registry.";
+    return std::string();
+  }
+  DCHECK_LE(size, installer::kMaxDMTokenLength);
+  std::string dm_token;
+  dm_token.assign(raw_value.data(), size);
+  return dm_token;
+}
+
+#if defined(GOOGLE_CHROME_BUILD)
+// Explicitly allow DMTokenStorage impersonate the client since some COM code
+// elsewhere in the browser process may have previously used
+// CoInitializeSecurity to set the impersonation level to something other than
+// the default. Ignore errors since an attempt to use Google Update may succeed
+// regardless.
+void ConfigureProxyBlanket(IUnknown* interface_pointer) {
+  ::CoSetProxyBlanket(
+      interface_pointer, RPC_C_AUTHN_DEFAULT, RPC_C_AUTHZ_DEFAULT,
+      COLE_DEFAULT_PRINCIPAL, RPC_C_AUTHN_LEVEL_PKT_PRIVACY,
+      RPC_C_IMP_LEVEL_IMPERSONATE, nullptr, EOAC_DYNAMIC_CLOAKING);
+}
+#endif  // defined(GOOGLE_CHROME_BUILD)
+
 bool StoreDMTokenInRegistry(const std::string& token) {
 #if defined(GOOGLE_CHROME_BUILD)
   if (token.empty())
@@ -110,107 +184,103 @@ bool StoreDMTokenInRegistry(const std::string& token) {
   return false;
 #endif  // defined(GOOGLE_CHROME_BUILD)
 }
+
 }  // namespace
 
-std::string BrowserDMTokenStorageWin::InitClientId() {
-  // For the client id, use the Windows machine GUID.
-  // TODO(crbug.com/821977): Need a backup plan if machine GUID doesn't exist.
-  base::win::RegKey key;
-  LSTATUS status = key.Open(HKEY_LOCAL_MACHINE,
-                            L"SOFTWARE\\Microsoft\\Cryptography", KEY_READ);
-  if (status != ERROR_SUCCESS)
-    return std::string();
+// static
+BrowserDMTokenStorage* BrowserDMTokenStorage::storage_for_testing_ = nullptr;
 
-  base::string16 value;
-  status = key.ReadValue(L"MachineGuid", &value);
-  if (status != ERROR_SUCCESS)
-    return std::string();
+// static
+BrowserDMTokenStorage* BrowserDMTokenStorage::Get() {
+  if (storage_for_testing_)
+    return storage_for_testing_;
 
-  std::string client_id;
-  if (!base::WideToUTF8(value.c_str(), value.length(), &client_id))
-    return std::string();
-  return client_id;
+  static base::NoDestructor<BrowserDMTokenStorageWin> storage;
+  return storage.get();
 }
 
-std::string BrowserDMTokenStorageWin::InitEnrollmentToken() {
-  return base::WideToUTF8(
-      InstallUtil::GetMachineLevelUserCloudPolicyEnrollmentToken());
-}
+BrowserDMTokenStorageWin::BrowserDMTokenStorageWin()
+    : com_sta_task_runner_(
+          base::CreateCOMSTATaskRunnerWithTraits({base::MayBlock()})),
+      is_initialized_(false),
+      weak_factory_(this) {
+  DETACH_FROM_SEQUENCE(sequence_checker_);
 
-std::string BrowserDMTokenStorageWin::InitDMToken() {
-  base::win::RegKey key;
-  base::string16 dm_token_key_path;
-  base::string16 dm_token_value_name;
-  InstallUtil::GetMachineLevelUserCloudPolicyDMTokenRegistryPath(
-      &dm_token_key_path, &dm_token_value_name);
-  LONG result = key.Open(HKEY_LOCAL_MACHINE, dm_token_key_path.c_str(),
-                         KEY_QUERY_VALUE | KEY_WOW64_64KEY);
-  if (result != ERROR_SUCCESS)
-    return std::string();
+  // We don't call InitIfNeeded() here so that the global instance can be
+  // created early during startup if needed. The tokens and client ID are read
+  // from the system as part of the first retrieve or store operation.
+}
 
-  // At the time of writing (January 2018), the DM token is about 200 bytes
-  // long. The initial size of the buffer should be enough to cover most
-  // realistic future size-increase scenarios, although we still make an effort
-  // to support somewhat larger token sizes just to be safe.
-  constexpr size_t kInitialDMTokenSize = 512;
+BrowserDMTokenStorageWin::~BrowserDMTokenStorageWin() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
 
-  DWORD size = kInitialDMTokenSize;
-  std::vector<char> raw_value(size);
-  DWORD dtype = REG_NONE;
-  result = key.ReadValue(dm_token_value_name.c_str(), raw_value.data(), &size,
-                         &dtype);
-  if (result == ERROR_MORE_DATA && size <= installer::kMaxDMTokenLength) {
-    raw_value.resize(size);
-    result = key.ReadValue(dm_token_value_name.c_str(), raw_value.data(), &size,
-                           &dtype);
-  }
+std::string BrowserDMTokenStorageWin::RetrieveClientId() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (result != ERROR_SUCCESS || dtype != REG_BINARY || size == 0) {
-    DVLOG(1) << "Failed to get DMToken from Registry.";
-    return std::string();
-  }
-  DCHECK_LE(size, installer::kMaxDMTokenLength);
-  std::string dm_token;
-  dm_token.assign(raw_value.data(), size);
-  return dm_token;
+  InitIfNeeded();
+  return client_id_;
 }
 
-#if defined(GOOGLE_CHROME_BUILD)
-// Explicitly allow DMTokenStorage impersonate the client since some COM code
-// elsewhere in the browser process may have previously used
-// CoInitializeSecurity to set the impersonation level to something other than
-// the default. Ignore errors since an attempt to use Google Update may succeed
-// regardless.
-void ConfigureProxyBlanket(IUnknown* interface_pointer) {
-  ::CoSetProxyBlanket(
-      interface_pointer, RPC_C_AUTHN_DEFAULT, RPC_C_AUTHZ_DEFAULT,
-      COLE_DEFAULT_PRINCIPAL, RPC_C_AUTHN_LEVEL_PKT_PRIVACY,
-      RPC_C_IMP_LEVEL_IMPERSONATE, nullptr, EOAC_DYNAMIC_CLOAKING);
+std::string BrowserDMTokenStorageWin::RetrieveEnrollmentToken() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  InitIfNeeded();
+  return enrollment_token_;
 }
-#endif  // defined(GOOGLE_CHROME_BUILD)
 
-void BrowserDMTokenStorageWin::SaveDMToken(const std::string& token) {
+void BrowserDMTokenStorageWin::StoreDMToken(const std::string& dm_token,
+                                            StoreCallback callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!store_callback_);
+
+  InitIfNeeded();
+
+  dm_token_ = dm_token;
+
+  store_callback_ = std::move(callback);
+
   base::PostTaskAndReplyWithResult(
       com_sta_task_runner_.get(), FROM_HERE,
-      base::BindOnce(&StoreDMTokenInRegistry, token),
-      base::BindOnce(&BrowserDMTokenStorage::OnDMTokenStored,
+      base::BindOnce(&StoreDMTokenInRegistry, dm_token),
+      base::BindOnce(&BrowserDMTokenStorageWin::OnDMTokenStored,
                      weak_factory_.GetWeakPtr()));
 }
 
-// static
-BrowserDMTokenStorage* BrowserDMTokenStorage::Get() {
-  if (storage_for_testing_)
-    return storage_for_testing_;
+std::string BrowserDMTokenStorageWin::RetrieveDMToken() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!store_callback_);
 
-  static base::NoDestructor<BrowserDMTokenStorageWin> storage;
-  return storage.get();
+  InitIfNeeded();
+  return dm_token_;
 }
 
-BrowserDMTokenStorageWin::BrowserDMTokenStorageWin()
-    : com_sta_task_runner_(
-          base::CreateCOMSTATaskRunnerWithTraits({base::MayBlock()})),
-      weak_factory_(this) {}
+void BrowserDMTokenStorageWin::InitIfNeeded() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (is_initialized_)
+    return;
+
+  is_initialized_ = true;
+  client_id_ = GetClientId();
+  DVLOG(1) << "Client ID = " << client_id_;
+  if (client_id_.empty())
+    return;
+
+  enrollment_token_ = base::WideToUTF8(
+      InstallUtil::GetMachineLevelUserCloudPolicyEnrollmentToken());
+  DVLOG(1) << "Enrollment token = " << enrollment_token_;
+
+  dm_token_ = GetDMToken();
+  DVLOG(1) << "DM Token = " << dm_token_;
+}
 
-BrowserDMTokenStorageWin::~BrowserDMTokenStorageWin() {}
+void BrowserDMTokenStorageWin::OnDMTokenStored(bool success) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(store_callback_);
+
+  if (!store_callback_.is_null())
+    std::move(store_callback_).Run(success);
+}
 
 }  // namespace policy

chrome/browser/policy/browser_dm_token_storage_win.h

@@ -9,7 +9,6 @@
 
 #include <string>
 
-#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
@@ -26,23 +25,34 @@ class BrowserDMTokenStorageWin : public BrowserDMTokenStorage {
   BrowserDMTokenStorageWin();
   ~BrowserDMTokenStorageWin() override;
 
- private:
   // override BrowserDMTokenStorage
-  std::string InitClientId() override;
-  std::string InitEnrollmentToken() override;
-  std::string InitDMToken() override;
-  void SaveDMToken(const std::string& token) override;
+  std::string RetrieveClientId() override;
+  std::string RetrieveEnrollmentToken() override;
+  void StoreDMToken(const std::string& dm_token,
+                    StoreCallback callback) override;
+  std::string RetrieveDMToken() override;
+
+ private:
+  // Initialize the DMTokenStorage, reads the |enrollment_token_| and
+  // |dm_token_| from Registry synchronously.
+  void InitIfNeeded();
+
+  void OnDMTokenStored(bool success);
 
   scoped_refptr<base::SingleThreadTaskRunner> com_sta_task_runner_;
+  StoreCallback store_callback_;
+
+  bool is_initialized_;
+
+  std::string client_id_;
+  std::string enrollment_token_;
+  std::string dm_token_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
 
   // This should always be the last member of the class.
   base::WeakPtrFactory<BrowserDMTokenStorageWin> weak_factory_;
 
-  FRIEND_TEST_ALL_PREFIXES(BrowserDMTokenStorageWinTest, InitClientId);
-  FRIEND_TEST_ALL_PREFIXES(BrowserDMTokenStorageWinTest, InitEnrollmentToken);
-  FRIEND_TEST_ALL_PREFIXES(BrowserDMTokenStorageWinTest, InitDMToken);
-  FRIEND_TEST_ALL_PREFIXES(BrowserDMTokenStorageWinTest, SaveDMToken);
-
   DISALLOW_COPY_AND_ASSIGN(BrowserDMTokenStorageWin);
 };
 

chrome/browser/policy/browser_dm_token_storage_win_unittest.cc

@@ -20,8 +20,11 @@ namespace policy {
 namespace {
 
 constexpr wchar_t kClientId1[] = L"fake-client-id-1";
+constexpr wchar_t kClientId2[] = L"fake-client-id-2";
 constexpr wchar_t kEnrollmentToken1[] = L"fake-enrollment-token-1";
+constexpr wchar_t kEnrollmentToken2[] = L"fake-enrollment-token-2";
 constexpr char kDMToken1[] = "fake-dm-token-1";
+constexpr char kDMToken2[] = "fake-dm-token-2";
 
 }  // namespace
 
@@ -70,26 +73,45 @@ class BrowserDMTokenStorageWinTest : public testing::Test {
   registry_util::RegistryOverrideManager registry_override_manager_;
 };
 
-TEST_F(BrowserDMTokenStorageWinTest, InitClientId) {
+TEST_F(BrowserDMTokenStorageWinTest, RetrieveClientId) {
   ASSERT_TRUE(SetMachineGuid(kClientId1));
   BrowserDMTokenStorageWin storage;
-  EXPECT_EQ(base::WideToUTF8(kClientId1), storage.InitClientId());
+  EXPECT_EQ(base::WideToUTF8(kClientId1), storage.RetrieveClientId());
+
+  // The client ID value should be cached in memory and not read from the system
+  // again.
+  ASSERT_TRUE(SetMachineGuid(kClientId2));
+  EXPECT_EQ(base::WideToUTF8(kClientId1), storage.RetrieveClientId());
 }
 
-TEST_F(BrowserDMTokenStorageWinTest, InitEnrollmentToken) {
+TEST_F(BrowserDMTokenStorageWinTest, RetrieveEnrollmentToken) {
   ASSERT_TRUE(SetMachineGuid(kClientId1));
   ASSERT_TRUE(SetEnrollmentToken(kEnrollmentToken1));
 
   BrowserDMTokenStorageWin storage;
-  EXPECT_EQ(base::WideToUTF8(kEnrollmentToken1), storage.InitEnrollmentToken());
+  EXPECT_EQ(base::WideToUTF8(kEnrollmentToken1),
+            storage.RetrieveEnrollmentToken());
+  // The enrollment token should be cached in memory and not read from the
+  // system again.
+  ASSERT_TRUE(SetEnrollmentToken(kEnrollmentToken2));
+  EXPECT_EQ(base::WideToUTF8(kEnrollmentToken1),
+            storage.RetrieveEnrollmentToken());
 }
 
-TEST_F(BrowserDMTokenStorageWinTest, InitDMToken) {
+TEST_F(BrowserDMTokenStorageWinTest, RetrieveDMToken) {
   ASSERT_TRUE(SetMachineGuid(kClientId1));
 
+  // The DM token will be read from the system regardless there is an enrollment
+  // token or not.
   ASSERT_TRUE(SetDMToken(kDMToken1));
   BrowserDMTokenStorageWin storage;
-  EXPECT_EQ(std::string(kDMToken1), storage.InitDMToken());
-}
+  EXPECT_EQ(std::string(kDMToken1), storage.RetrieveDMToken());
 
+  ASSERT_TRUE(SetEnrollmentToken(kEnrollmentToken1));
+  EXPECT_EQ(std::string(kDMToken1), storage.RetrieveDMToken());
+
+  // The DM token should be cached in memory and not read from the system again.
+  ASSERT_TRUE(SetDMToken(kDMToken2));
+  EXPECT_EQ(std::string(kDMToken1), storage.RetrieveDMToken());
+}
 }  // namespace policy

chrome/browser/policy/machine_level_user_cloud_policy_controller.cc

@@ -103,7 +103,7 @@ void MachineLevelUserCloudPolicyController::Init(
   if (!policy_manager)
     return;
   // If there exists an enrollment token, then there are two states:
-  //   1/ There also exists a DM token.  This machine is already registered, so
+  //   1/ There also exists a DM token.  This machine is already registeted, so
   //      the next step is to fetch policies.
   //   2/ There is no DM token.  In this case the machine is not already
   //      registered and needs to request a DM token.
@@ -144,9 +144,9 @@ void MachineLevelUserCloudPolicyController::Init(
                    base::Unretained(this)));
 #if defined(OS_WIN)
     // This metric is only published on Windows to indicate how many user level
-    // installs try to enroll, as these can't store the DM token
-    // in the registry at the end of enrollment. Mac and Linux do not need
-    // this metric for now as they might use a different token storage mechanism
+    // install Chrome try to enroll the policy which can't store the DM token
+    // in the Registry in the end of enrollment. Mac and Linux does not need
+    // this metric for now as they might use different token storage mechanism
     // in the future.
     UMA_HISTOGRAM_BOOLEAN(
         "Enterprise.MachineLevelUserCloudPolicyEnrollment.InstallLevel_Win",

chrome/test/BUILD.gn

@@ -2537,7 +2537,6 @@ test("unit_tests") {
     "../browser/permissions/permission_request_manager_unittest.cc",
     "../browser/permissions/permission_util_unittest.cc",
     "../browser/plugins/pdf_iframe_navigation_throttle_unittest.cc",
-    "../browser/policy/browser_dm_token_storage_unittest.cc",
     "../browser/policy/browser_dm_token_storage_win_unittest.cc",
     "../browser/policy/cloud/cloud_policy_invalidator_unittest.cc",
     "../browser/policy/cloud/cloud_policy_test_utils.cc",