Browser tests for the new way of handling device ID in Chrome OS.

chrome/browser/chromeos/login/signin/device_id_browsertest.cc

Index: chrome/browser/chromeos/login/signin/device_id_browsertest.cc
diff --git a/chrome/browser/chromeos/login/signin/device_id_browsertest.cc b/chrome/browser/chromeos/login/signin/device_id_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2af9cd437b9f0b59d200b084a03a2e03cb728174
--- /dev/null
+++ b/chrome/browser/chromeos/login/signin/device_id_browsertest.cc
@@ -0,0 +1,274 @@
+// Copyright 2015 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 "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
+#include "base/message_loop/message_loop.h"
+#include "base/prefs/pref_service.h"
+#include "base/run_loop.h"
+#include "base/strings/stringprintf.h"
+#include "chrome/browser/chromeos/login/test/oobe_base_test.h"
+#include "chrome/browser/chromeos/login/test/oobe_screen_waiter.h"
+#include "chrome/browser/chromeos/login/ui/oobe_display.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/signin/chrome_signin_client_factory.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chromeos/chromeos_switches.h"
+#include "components/signin/core/common/signin_pref_names.h"
+#include "components/user_manager/remove_user_delegate.h"
+#include "components/user_manager/user_manager.h"
+
+namespace {
+
+char kRefreshToken1[] = "refresh_token_1";
+char kRefreshToken2[] = "refresh_token_2";
+const base::FilePath::CharType kRefreshTokenToDeviceIdMapPath[] =

[achuithb, 2015/05/26 06:28:20]

kRefreshTokenToDeviceIdMapFile or kRefreshTokenToDeviceIdMapFilename?

[dzhioev (left Google), 2015/05/26 21:18:15]

Done.

+    FILE_PATH_LITERAL("refrest_token_to_device_id.json");
+
+char kSecondUserEmail[] = "second_user@gmail.com";
+char kSecondUserPassword[] = "password";
+char kSecondUserRefreshToken1[] = "refresh_token_second_user_1";
+char kSecondUserRefreshToken2[] = "refresh_token_second_user_2";
+
+}  // namespace
+
+namespace chromeos {
+
+class DeviceIDTest : public OobeBaseTest,
+                     public user_manager::RemoveUserDelegate {
+ public:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    OobeBaseTest::SetUpCommandLine(command_line);
+    command_line->AppendSwitch(switches::kOobeSkipPostLogin);
+  }
+
+  void SetUpOnMainThread() override {
+    OobeBaseTest::SetUpOnMainThread();
+    LoadRefreshTokenToDeviceIdMap();
+  }
+
+  void TearDownOnMainThread() override {
+    SaveRefreshTokenToDeviceIdMap();
+    OobeBaseTest::TearDownOnMainThread();
+  }
+
+  std::string GetDeviceId(const std::string& user_id) {
+    return user_manager::UserManager::Get()->GetKnownUserDeviceId(user_id);
+  }
+
+  std::string GetDeviceIdFromSigninClient(const std::string& user_id) {
+    return ChromeSigninClientFactory::GetForProfile(
+               ProfileHelper::Get()->GetProfileByUser(
+                   user_manager::UserManager::Get()->FindUser(user_id)))
+        ->GetSigninScopedDeviceId();
+  }
+
+  std::string GetDeviceIdFromGAIA(const std::string& refresh_token) {
+    return fake_gaia_->GetDeviceIdByRefreshToken(refresh_token);
+  }
+
+  // Checks that user's device ID retrieved from UserManager and SigninClient
+  // are the same.
+  // If |refresh_token| is not empty, checks that device ID associated with the
+  // |refresh_token| in GAIA is the same as ID saved on device.
+  void CheckDeviceIDIsConsistent(const std::string& user_id,
+                                 const std::string& refresh_token) {
+    std::string device_id_in_signin_client =

[achuithb, 2015/05/26 06:28:20]

nit: const

[dzhioev (left Google), 2015/05/26 21:18:14]

Done.

+        GetDeviceIdFromSigninClient(user_id);
+    std::string device_id_in_local_state = GetDeviceId(user_id);

[achuithb, 2015/05/26 06:28:20]

nit: const

[dzhioev (left Google), 2015/05/26 21:18:14]

Done.

+
+    EXPECT_FALSE(device_id_in_signin_client.empty());
+    EXPECT_EQ(device_id_in_signin_client, device_id_in_local_state);
+
+    if (!refresh_token.empty()) {
+      std::string device_id_in_gaia = GetDeviceIdFromGAIA(refresh_token);

[achuithb, 2015/05/26 06:28:20]

nit: const

[dzhioev (left Google), 2015/05/26 21:18:14]

Done.

+      EXPECT_EQ(device_id_in_signin_client, device_id_in_gaia);
+    }
+  }
+
+  void WaitForSessionStart() {
+    content::WindowedNotificationObserver(
+        chrome::NOTIFICATION_SESSION_STARTED,
+        content::NotificationService::AllSources()).Wait();
+  }
+
+  void SignInOnline(const std::string& user_id,
+                    const std::string& password,
+                    const std::string& refresh_token) {
+    WaitForGaiaPageLoad();
+
+    FakeGaia::MergeSessionParams params;
+    params.email = user_id;
+    params.refresh_token = refresh_token;
+    fake_gaia_->UpdateMergeSessionParams(params);
+
+    GetLoginDisplay()->ShowSigninScreenForCreds(user_id, password);
+    WaitForSessionStart();
+
+    CheckDeviceIDIsConsistent(user_id, refresh_token);

[achuithb, 2015/05/26 06:28:20]

I feel you should pull this function out and explicitly call it from the tests
instead of having it be a side effect of signing in. I understand that this will
add some verbosity to the tests.

[dzhioev (left Google), 2015/05/26 21:18:14]

Done.

+  }
+
+  void SignInOffline(const std::string& user_id,
+                     const std::string& password,
+                     const std::string& refresh_token) {
+    WaitForSigninScreen();
+
+    JS().Execute(
+        base::StringPrintf("chrome.send('authenticateUser', ['%s', '%s'])",
+                           user_id.c_str(), password.c_str()));
+    WaitForSessionStart();
+
+    CheckDeviceIDIsConsistent(user_id, refresh_token);

[achuithb, 2015/05/26 06:28:20]

Same

[dzhioev (left Google), 2015/05/26 21:18:15]

Done.

+  }
+
+  void RemoveUser(const std::string& user_id) {
+    user_manager::UserManager::Get()->RemoveUser(user_id, this);
+    user_removal_loop_.Run();
+  }
+
+ private:
+  void OnBeforeUserRemoved(const std::string& username) override {}

[achuithb, 2015/05/26 06:28:19]

// RemoveUserDelegate:

[dzhioev (left Google), 2015/05/26 21:18:14]

Done.

+
+  void OnUserRemoved(const std::string& username) override {
+    user_removal_loop_.Quit();
+  }
+
+  void LoadRefreshTokenToDeviceIdMap() {
+    base::FilePath path = base::CommandLine::ForCurrentProcess()

[achuithb, 2015/05/26 06:28:20]

nit: const, also a slightly more descriptive name would be nice here

[dzhioev (left Google), 2015/05/26 21:18:14]

Done.

+                              ->GetSwitchValuePath(::switches::kUserDataDir)
+                              .Append(kRefreshTokenToDeviceIdMapPath);
+    std::string file_contents;
+    if (!base::ReadFileToString(path, &file_contents))
+      return;

[achuithb, 2015/05/26 06:28:20]

When is this a valid outcome?

[dzhioev (left Google), 2015/05/26 21:18:15]

When the file doesn't exist. This happens in the very first test in PRE_* tests
chain, when data dir is empty.

+    scoped_ptr<base::Value> value(base::JSONReader::Read(file_contents));
+    base::DictionaryValue* dictionary;
+    EXPECT_TRUE(value->GetAsDictionary(&dictionary));
+    FakeGaia::RefreshTokenToDeviceIdMap map;
+    for (base::DictionaryValue::Iterator it(*dictionary); !it.IsAtEnd();

[achuithb, 2015/05/26 06:28:20]

Why not auto here?

[dzhioev (left Google), 2015/05/26 21:18:14]

base::Dictionary doesn't have begin() method returning an iterator, as standard
containers do.

+         it.Advance()) {
+      std::string device_id;
+      EXPECT_TRUE(it.value().GetAsString(&device_id));
+      map[it.key()] = device_id;
+    }
+    fake_gaia_->SetRefreshTokenToDeviceIdMap(map);
+  }
+
+  void SaveRefreshTokenToDeviceIdMap() const {
+    base::DictionaryValue dictionary;
+    for (const auto& kv : fake_gaia_->refresh_token_to_device_id_map())
+      dictionary.SetStringWithoutPathExpansion(kv.first, kv.second);
+    std::string json;
+    EXPECT_TRUE(base::JSONWriter::Write(dictionary, &json));
+    base::FilePath path = base::CommandLine::ForCurrentProcess()

[achuithb, 2015/05/26 06:28:20]

nit: const, also more descriptive name.

[dzhioev (left Google), 2015/05/26 21:18:14]

Done.

+                              ->GetSwitchValuePath(::switches::kUserDataDir)
+                              .Append(kRefreshTokenToDeviceIdMapPath);
+    EXPECT_TRUE(base::WriteFile(path, json.c_str(), json.length()));
+  }
+
+  base::RunLoop user_removal_loop_;
+};
+
+// Add the first user and check that device ID is consistent.
+IN_PROC_BROWSER_TEST_F(DeviceIDTest, PRE_PRE_PRE_PRE_PRE_NewUsers) {
+  SignInOnline(kFakeUserEmail, kFakeUserPassword, kRefreshToken1);
+}
+
+// Authenticate the first user through GAIA and verify that device ID remains
+// the same.
+IN_PROC_BROWSER_TEST_F(DeviceIDTest, PRE_PRE_PRE_PRE_NewUsers) {
+  std::string device_id = GetDeviceId(kFakeUserEmail);

[achuithb, 2015/05/26 06:28:20]

nit: const

[dzhioev (left Google), 2015/05/26 21:18:15]

Done.

+  EXPECT_FALSE(device_id.empty());
+  EXPECT_EQ(device_id, GetDeviceIdFromGAIA(kRefreshToken1));
+
+  SignInOnline(kFakeUserEmail, kFakeUserPassword, kRefreshToken2);
+
+  CHECK_EQ(device_id, GetDeviceId(kFakeUserEmail));
+}
+
+// Authenticate the first user online and verify that device ID remains
+// the same.
+IN_PROC_BROWSER_TEST_F(DeviceIDTest, PRE_PRE_PRE_NewUsers) {
+  std::string device_id = GetDeviceId(kFakeUserEmail);

[achuithb, 2015/05/26 06:28:20]

nit:const

[dzhioev (left Google), 2015/05/26 21:18:14]

Done.

+  EXPECT_FALSE(device_id.empty());
+
+  SignInOffline(kFakeUserEmail, kFakeUserPassword, kRefreshToken2);
+
+  // Verify that device ID remained the same after offline auth.
+  CHECK_EQ(device_id, GetDeviceId(kFakeUserEmail));
+}
+
+// Add the second user.
+IN_PROC_BROWSER_TEST_F(DeviceIDTest, PRE_PRE_NewUsers) {
+  WaitForSigninScreen();
+  JS().Execute("chrome.send('showAddUser')");
+  SignInOnline(kSecondUserEmail, kSecondUserPassword, kSecondUserRefreshToken1);
+}
+
+// Remove the second user.
+IN_PROC_BROWSER_TEST_F(DeviceIDTest, PRE_NewUsers) {
+  WaitForSigninScreen();
+  RemoveUser(kSecondUserEmail);
+}
+
+// Add the second user back. Verify that device ID has been changed.
+IN_PROC_BROWSER_TEST_F(DeviceIDTest, NewUsers) {
+  EXPECT_TRUE(GetDeviceId(kSecondUserEmail).empty());
+  SignInOnline(kSecondUserEmail, kSecondUserPassword, kSecondUserRefreshToken2);
+  EXPECT_NE(GetDeviceIdFromGAIA(kSecondUserRefreshToken1),
+            GetDeviceId(kSecondUserEmail));
+}
+
+// Tests migration process from the old way of storing device ID (in
+// preference), to the new one (in Local State).
+IN_PROC_BROWSER_TEST_F(DeviceIDTest, PRE_Migration) {

[achuithb, 2015/05/26 06:28:20]

This "test" isn't testing anything, is it?

If it's not, shouldn't it just be a method instead of a test?

[dzhioev (left Google), 2015/05/26 21:18:15]

Yes, that is PRE test case where we set up the state for the real test case
(DeviceIDTest.Migration). User can't "sign out" on Chrome OS without restarting
a browser process, and the only way to keep the user data dir between browser
restarts in browser tests is a PRE_* test.

I reworded comments a little to make it clear what happens in each test case.

+  SignInOnline(kFakeUserEmail, kFakeUserPassword, kRefreshToken1);
+
+  // Simulate user that has device ID saved only in preferences (pre-M44).
+  PrefService* prefs =
+      ProfileHelper::Get()
+          ->GetProfileByUser(user_manager::UserManager::Get()->GetActiveUser())
+          ->GetPrefs();
+  prefs->SetString(prefs::kGoogleServicesSigninScopedDeviceId,
+                   GetDeviceId(kFakeUserEmail));
+
+  // Can't use SetKnownUserDeviceId here, because it forbids changing a device
+  // ID.
+  user_manager::UserManager::Get()->SetKnownUserStringPref(
+      kFakeUserEmail, "device_id", std::string());
+}
+
+IN_PROC_BROWSER_TEST_F(DeviceIDTest, Migration) {
+  EXPECT_TRUE(GetDeviceId(kFakeUserEmail).empty());
+  SignInOffline(kFakeUserEmail, kFakeUserPassword, kRefreshToken1);
+}
+
+// Tests that device ID is generated for users that has device ID stored neither
+// in preference, nor in Local State.
+IN_PROC_BROWSER_TEST_F(DeviceIDTest, PRE_LegacyUsers) {
+  SignInOnline(kFakeUserEmail, kFakeUserPassword, kRefreshToken1);
+
+  PrefService* prefs =
+      ProfileHelper::Get()
+          ->GetProfileByUser(user_manager::UserManager::Get()->GetActiveUser())
+          ->GetPrefs();
+  EXPECT_TRUE(
+      prefs->GetString(prefs::kGoogleServicesSigninScopedDeviceId).empty());
+
+  // Can't use SetKnownUserDeviceId here, because it forbids changing a device
+  // ID.
+  user_manager::UserManager::Get()->SetKnownUserStringPref(
+      kFakeUserEmail, "device_id", std::string());
+}
+
+IN_PROC_BROWSER_TEST_F(DeviceIDTest, LegacyUsers) {
+  EXPECT_TRUE(GetDeviceId(kFakeUserEmail).empty());
+  // Last param |auth_code| is empty, because we don't pass a device ID to GAIA
+  // in this case.
+  SignInOffline(kFakeUserEmail, kFakeUserPassword, std::string());
+}
+
+}  // namespace chromeos