Prefer the product-neutral DM token storage location over the browser-specific one.

This aligns Chrome's DM token storage with Google Update's.

This CL also makes base::win::RegKey a movable type.

BUG=970162

Change-Id: Id6270411d2417501746ad85f8061cd7d93841618
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1997184
Commit-Queue: Greg Thompson <grt@chromium.org>
Reviewed-by: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#731691}

base/win/registry.cc

@@ -106,6 +106,22 @@ RegKey::RegKey(HKEY rootkey, const wchar_t* subkey, REGSAM access) {
   }
 }
 
+RegKey::RegKey(RegKey&& other)
+    : key_(other.key_),
+      wow64access_(other.wow64access_),
+      key_watcher_(std::move(other.key_watcher_)) {
+  other.key_ = nullptr;
+  other.wow64access_ = 0;
+}
+
+RegKey& RegKey::operator=(RegKey&& other) {
+  Close();
+  std::swap(key_, other.key_);
+  std::swap(wow64access_, other.wow64access_);
+  key_watcher_ = std::move(other.key_watcher_);
+  return *this;
+}
+
 RegKey::~RegKey() {
   Close();
 }

base/win/registry.h

@@ -38,6 +38,8 @@ class BASE_EXPORT RegKey {
   RegKey();
   explicit RegKey(HKEY key);
   RegKey(HKEY rootkey, const wchar_t* subkey, REGSAM access);
+  RegKey(RegKey&& other) noexcept;
+  RegKey& operator=(RegKey&& other);
   ~RegKey();
 
   LONG Create(HKEY rootkey, const wchar_t* subkey, REGSAM access);

base/win/registry_unittest.cc

@@ -402,6 +402,47 @@ TEST_F(RegistryTest, ChangeCallback) {
   EXPECT_FALSE(delegate.WasCalled());
 }
 
+TEST_F(RegistryTest, TestMoveConstruct) {
+  RegKey key;
+  std::wstring foo_key(kRootKey);
+
+  ASSERT_EQ(key.Create(HKEY_CURRENT_USER, foo_key.c_str(), KEY_SET_VALUE),
+            ERROR_SUCCESS);
+  RegKey key2(std::move(key));
+
+  // The old key should be meaningless now.
+  EXPECT_EQ(key.Handle(), nullptr);
+
+  // And the new one should work just fine.
+  EXPECT_NE(key2.Handle(), nullptr);
+  EXPECT_EQ(key2.WriteValue(L"foo", 1U), ERROR_SUCCESS);
+}
+
+TEST_F(RegistryTest, TestMoveAssign) {
+  RegKey key;
+  RegKey key2;
+  std::wstring foo_key(kRootKey);
+  const wchar_t kFooValueName[] = L"foo";
+
+  ASSERT_EQ(key.Create(HKEY_CURRENT_USER, foo_key.c_str(),
+                       KEY_SET_VALUE | KEY_QUERY_VALUE),
+            ERROR_SUCCESS);
+  ASSERT_EQ(key.WriteValue(kFooValueName, 1U), ERROR_SUCCESS);
+  ASSERT_EQ(key2.Create(HKEY_CURRENT_USER, (foo_key + L"\\child").c_str(),
+                        KEY_SET_VALUE),
+            ERROR_SUCCESS);
+  key2 = std::move(key);
+
+  // The old key should be meaningless now.
+  EXPECT_EQ(key.Handle(), nullptr);
+
+  // And the new one should hold what was the old one.
+  EXPECT_NE(key2.Handle(), nullptr);
+  DWORD foo = 0;
+  ASSERT_EQ(key2.ReadValueDW(kFooValueName, &foo), ERROR_SUCCESS);
+  EXPECT_EQ(foo, 1U);
+}
+
 }  // namespace
 
 }  // namespace win

chrome/browser/policy/browser_dm_token_storage_win.cc

@@ -15,6 +15,7 @@
 #include <wrl/client.h>
 
 #include <memory>
+#include <tuple>
 #include <utility>
 #include <vector>
 
@@ -25,6 +26,7 @@
 #include "base/logging.h"
 #include "base/no_destructor.h"
 #include "base/strings/string16.h"
+#include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/post_task.h"
@@ -155,41 +157,46 @@ std::string BrowserDMTokenStorageWin::InitEnrollmentToken() {
 }
 
 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();
-
   // 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);
+  base::win::RegKey key;
+  base::string16 dm_token_value_name;
+  std::vector<char> raw_value(kInitialDMTokenSize);
+
+  // Prefer the app-neutral location over the browser's to match Google Update's
+  // behavior.
+  for (const auto& location : {InstallUtil::BrowserLocation(false),
+                               InstallUtil::BrowserLocation(true)}) {
+    std::tie(key, dm_token_value_name) =
+        InstallUtil::GetCloudManagementDmTokenLocation(
+            InstallUtil::ReadOnly(true), location);
+    if (!key.Valid())
+      continue;
+
+    DWORD dtype = REG_NONE;
+    DWORD size = DWORD{raw_value.size()};
+    auto 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)
+      continue;
+
+    DCHECK_LE(size, installer::kMaxDMTokenLength);
+    return base::TrimWhitespaceASCII(base::StringPiece(raw_value.data(), size),
+                                     base::TRIM_ALL)
+        .as_string();
   }
 
-  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 base::TrimWhitespaceASCII(dm_token, base::TRIM_ALL).as_string();
+  DVLOG(1) << "Failed to get DMToken from Registry.";
+  return std::string();
 }
 
 bool BrowserDMTokenStorageWin::InitEnrollmentErrorOption() {

chrome/browser/policy/browser_dm_token_storage_win.h

@@ -44,6 +44,8 @@ class BrowserDMTokenStorageWin : public BrowserDMTokenStorage {
   FRIEND_TEST_ALL_PREFIXES(BrowserDMTokenStorageWinTest,
                            InitEnrollmentTokenFromSecondary);
   FRIEND_TEST_ALL_PREFIXES(BrowserDMTokenStorageWinTest, InitDMToken);
+  FRIEND_TEST_ALL_PREFIXES(BrowserDMTokenStorageWinTest,
+                           InitDMTokenFromBrowserLocation);
 
   DISALLOW_COPY_AND_ASSIGN(BrowserDMTokenStorageWin);
 };

chrome/browser/policy/browser_dm_token_storage_win_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/policy/browser_dm_token_storage_win.h"
 
+#include <tuple>
+
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_reg_util_win.h"
@@ -23,6 +25,7 @@ constexpr wchar_t kClientId1[] = L"fake-client-id-1";
 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
 
@@ -66,16 +69,18 @@ class BrowserDMTokenStorageWinTest : public testing::Test {
                ERROR_SUCCESS;
   }
 
-  bool SetDMToken(const std::string& dm_token) {
+  // Sets a DM token in either the app-neutral or browser-specific location in
+  // the registry.
+  bool SetDMToken(const std::string& dm_token,
+                  InstallUtil::BrowserLocation browser_location) {
     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);
-    return (key.Create(HKEY_LOCAL_MACHINE, dm_token_key_path.c_str(),
-                       KEY_SET_VALUE | KEY_WOW64_64KEY) == ERROR_SUCCESS) &&
-           (key.WriteValue(dm_token_value_name.c_str(), dm_token.c_str(),
-                           dm_token.size(), REG_BINARY) == ERROR_SUCCESS);
+    std::tie(key, dm_token_value_name) =
+        InstallUtil::GetCloudManagementDmTokenLocation(
+            InstallUtil::ReadOnly(false), browser_location);
+    return key.Valid() &&
+           key.WriteValue(dm_token_value_name.c_str(), dm_token.c_str(),
+                          dm_token.size(), REG_BINARY) == ERROR_SUCCESS;
   }
 
   content::BrowserTaskEnvironment task_environment_;
@@ -108,9 +113,20 @@ TEST_F(BrowserDMTokenStorageWinTest, InitEnrollmentTokenFromSecondary) {
 TEST_F(BrowserDMTokenStorageWinTest, InitDMToken) {
   ASSERT_TRUE(SetMachineGuid(kClientId1));
 
-  ASSERT_TRUE(SetDMToken(kDMToken1));
+  // The app-neutral location should be preferred.
+  ASSERT_TRUE(SetDMToken(kDMToken1, InstallUtil::BrowserLocation(false)));
+  ASSERT_TRUE(SetDMToken(kDMToken2, InstallUtil::BrowserLocation(true)));
   BrowserDMTokenStorageWin storage;
   EXPECT_EQ(std::string(kDMToken1), storage.InitDMToken());
 }
 
+TEST_F(BrowserDMTokenStorageWinTest, InitDMTokenFromBrowserLocation) {
+  ASSERT_TRUE(SetMachineGuid(kClientId1));
+
+  // If there's no app-neutral token, the browser-specific one should be used.
+  ASSERT_TRUE(SetDMToken(kDMToken2, InstallUtil::BrowserLocation(true)));
+  BrowserDMTokenStorageWin storage;
+  EXPECT_EQ(std::string(kDMToken2), storage.InitDMToken());
+}
+
 }  // namespace policy

chrome/installer/setup/setup_util.cc

@@ -879,31 +879,37 @@ bool StoreDMToken(const std::string& token) {
     return false;
   }
 
-  std::wstring path;
-  std::wstring name;
-  InstallUtil::GetMachineLevelUserCloudPolicyDMTokenRegistryPath(&path,
-                                                                 &name);
-
+  // Write the token both to the app-neutral and browser-specific locations.
+  // Only the former is mandatory -- the latter is best-effort.
   base::win::RegKey key;
-  LONG result = key.Create(HKEY_LOCAL_MACHINE, path.c_str(),
-                           KEY_WRITE | KEY_WOW64_64KEY);
-  if (result != ERROR_SUCCESS) {
-    LOG(ERROR) << "Unable to create/open registry key HKLM\\" << path
-               << " for writing result=" << result;
-    return false;
-  }
+  std::wstring value_name;
+  bool succeeded = false;
+  for (const auto& is_browser_location : {InstallUtil::BrowserLocation(false),
+                                          InstallUtil::BrowserLocation(true)}) {
+    std::tie(key, value_name) = InstallUtil::GetCloudManagementDmTokenLocation(
+        InstallUtil::ReadOnly(false), is_browser_location);
+    // If the key couldn't be opened on the first iteration (the mandatory
+    // location), return failure straight away. Otherwise, continue iterating.
+    if (!key.Valid()) {
+      if (succeeded)
+        continue;
+      // Logging already performed in GetCloudManagementDmTokenLocation.
+      return false;
+    }
 
-  result =
-      key.WriteValue(name.c_str(), token.data(),
-                     base::saturated_cast<DWORD>(token.size()), REG_BINARY);
-  if (result != ERROR_SUCCESS) {
-    LOG(ERROR) << "Unable to write specified DMToken to the registry at HKLM\\"
-               << path << "\\" << name << " result=" << result;
-    return false;
+    auto result =
+        key.WriteValue(value_name.c_str(), token.data(),
+                       base::saturated_cast<DWORD>(token.size()), REG_BINARY);
+    if (result == ERROR_SUCCESS) {
+      succeeded = true;
+    } else if (!succeeded) {
+      ::SetLastError(result);
+      PLOG(ERROR) << "Unable to write specified DMToken to the registry";
+      return false;
+    }  // Else ignore the failure to write to the best-effort location.
   }
 
   VLOG(1) << "Successfully stored specified DMToken in the registry.";
-
   return true;
 }
 

chrome/installer/setup/setup_util_unittest.cc

@@ -10,6 +10,7 @@
 #include <ios>
 #include <memory>
 #include <string>
+#include <tuple>
 
 #include "base/base64.h"
 #include "base/command_line.h"
@@ -488,12 +489,11 @@ TEST(SetupUtilTest, StoreDMTokenToRegistrySuccess) {
   ASSERT_EQ(kExpectedSize, token.length());
   EXPECT_TRUE(installer::StoreDMToken(token));
 
-  std::wstring path;
-  std::wstring name;
-  InstallUtil::GetMachineLevelUserCloudPolicyDMTokenRegistryPath(&path, &name);
   base::win::RegKey key;
-  ASSERT_EQ(ERROR_SUCCESS, key.Open(HKEY_LOCAL_MACHINE, path.c_str(),
-                                    KEY_QUERY_VALUE | KEY_WOW64_64KEY));
+  std::wstring name;
+  std::tie(key, name) = InstallUtil::GetCloudManagementDmTokenLocation(
+      InstallUtil::ReadOnly(true), InstallUtil::BrowserLocation(false));
+  ASSERT_TRUE(key.Valid());
 
   DWORD size = kExpectedSize;
   std::vector<char> raw_value(size);
@@ -503,6 +503,17 @@ TEST(SetupUtilTest, StoreDMTokenToRegistrySuccess) {
   EXPECT_EQ(REG_BINARY, dtype);
   ASSERT_EQ(kExpectedSize, size);
   EXPECT_EQ(0, memcmp(token.data(), raw_value.data(), kExpectedSize));
+
+  std::tie(key, name) = InstallUtil::GetCloudManagementDmTokenLocation(
+      InstallUtil::ReadOnly(true), InstallUtil::BrowserLocation(true));
+  ASSERT_TRUE(key.Valid());
+
+  size = kExpectedSize;
+  ASSERT_EQ(ERROR_SUCCESS,
+            key.ReadValue(name.c_str(), raw_value.data(), &size, &dtype));
+  EXPECT_EQ(REG_BINARY, dtype);
+  ASSERT_EQ(kExpectedSize, size);
+  EXPECT_EQ(0, memcmp(token.data(), raw_value.data(), kExpectedSize));
 }
 
 TEST(SetupUtilTest, StoreDMTokenToRegistryShouldFailWhenDMTokenTooLarge) {

chrome/installer/util/install_util.cc

@@ -24,7 +24,6 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/system/sys_info.h"
 #include "base/values.h"
-#include "base/win/registry.h"
 #include "base/win/shlwapi.h"
 #include "base/win/shortcut.h"
 #include "base/win/win_util.h"
@@ -599,18 +598,39 @@ InstallUtil::GetCloudManagementEnrollmentTokenRegistryPaths() {
 }
 
 // static
-void InstallUtil::GetMachineLevelUserCloudPolicyDMTokenRegistryPath(
-    base::string16* key_path,
-    base::string16* value_name) {
-  // This token applies to all installs on the machine, even though only a
-  // system install can set it.  This is to prevent users from doing a user
-  // install of chrome to get around policies.
-  *key_path = L"SOFTWARE\\";
-  install_static::AppendChromeInstallSubDirectory(
-      install_static::InstallDetails::Get().mode(), false /* !include_suffix */,
-      key_path);
-  key_path->append(L"\\Enrollment");
-  *value_name = L"dmtoken";
+std::pair<base::win::RegKey, std::wstring>
+InstallUtil::GetCloudManagementDmTokenLocation(
+    ReadOnly read_only,
+    BrowserLocation browser_location) {
+  // The location dictates the path and WoW bit.
+  REGSAM wow_access = 0;
+  std::wstring key_path(L"SOFTWARE\\");
+  if (browser_location) {
+    wow_access |= KEY_WOW64_64KEY;
+    install_static::AppendChromeInstallSubDirectory(
+        install_static::InstallDetails::Get().mode(), /*include_suffix=*/false,
+        &key_path);
+  } else {
+    wow_access |= KEY_WOW64_32KEY;
+    key_path.append(install_static::kCompanyPathName);
+  }
+  key_path.append(L"\\Enrollment");
+
+  base::win::RegKey key;
+  if (read_only) {
+    key.Open(HKEY_LOCAL_MACHINE, key_path.c_str(),
+             KEY_QUERY_VALUE | wow_access);
+  } else {
+    auto result = key.Create(HKEY_LOCAL_MACHINE, key_path.c_str(),
+                             KEY_SET_VALUE | wow_access);
+    if (result != ERROR_SUCCESS) {
+      ::SetLastError(result);
+      PLOG(ERROR) << "Failed to create/open registry key HKLM\\" << key_path
+                  << " for writing";
+    }
+  }
+
+  return {std::move(key), L"dmtoken"};
 }
 
 // static

chrome/installer/util/install_util.h

@@ -21,7 +21,9 @@
 #include "base/optional.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_piece.h"
+#include "base/util/type_safety/strong_alias.h"
 #include "base/version.h"
+#include "base/win/registry.h"
 #include "base/win/scoped_handle.h"
 #include "chrome/installer/util/util_constants.h"
 
@@ -183,11 +185,18 @@ class InstallUtil {
   static std::vector<std::pair<std::wstring, std::wstring>>
   GetCloudManagementEnrollmentTokenRegistryPaths();
 
-  // Returns the registry key path and value name where the DM token is stored
-  // for machine level user cloud policies.
-  static void GetMachineLevelUserCloudPolicyDMTokenRegistryPath(
-      base::string16* key_path,
-      base::string16* value_name);
+  using ReadOnly = util::StrongAlias<class ReadOnlyTag, bool>;
+  using BrowserLocation = util::StrongAlias<class BrowserLocationTag, bool>;
+
+  // Returns the registry key and value name from/to which a cloud management DM
+  // token may be read/written. |read_only| indicates whether they key is opened
+  // for reading the value or writing it. |browser_location| indicates whether
+  // the legacy browser-specific location is returned rather than the
+  // app-neutral location. The returned key will be invalid if it could not be
+  // opened/created.
+  static std::pair<base::win::RegKey, std::wstring>
+  GetCloudManagementDmTokenLocation(ReadOnly read_only,
+                                    BrowserLocation browser_location);
 
   // Returns the token used to enroll this chrome instance for machine level
   // user cloud policies.  Returns an empty string if this machine should not