Convert ParentAccessServiceTest to LoggedInUserMixin for child login

Currently, ParentAccessServiceTest uses LoginPolicyTestBase to log in
as child user for tests. We should deprecate LoginPolicyTestBase in
favor of LoggedInUserMixin instead. This CL converts
ParentAccessServiceTest to replace LoginPolicyTestBase with
LoggedInUserMixin for logging in as child user for tests.

Bug: 1014663
Change-Id: Ied5b53ee8ec2f44d5dc2193f011b741217080eed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1909273Reviewed-by: Aga Wronska <agawronska@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Toby Huang <tobyhuang@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#714592}
Signed-off-by: Toby Huang <tobyhuang@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1914348
Cr-Commit-Position: refs/heads/master@{#715496}

chrome/browser/chromeos/child_accounts/parent_access_code/parent_access_service_browsertest.cc

@@ -10,17 +10,16 @@
 #include "base/bind.h"
 #include "base/json/json_writer.h"
 #include "base/macros.h"
-#include "chrome/browser/chromeos/child_accounts/child_account_test_utils.h"
 #include "chrome/browser/chromeos/child_accounts/parent_access_code/config_source.h"
 #include "chrome/browser/chromeos/child_accounts/parent_access_code/parent_access_service.h"
 #include "chrome/browser/chromeos/child_accounts/parent_access_code/parent_access_test_utils.h"
-#include "chrome/browser/chromeos/policy/login_policy_test_base.h"
 #include "chrome/browser/chromeos/policy/user_policy_test_helper.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/supervised_user/logged_in_user_mixin.h"
+#include "chrome/test/base/mixin_based_in_process_browser_test.h"
+#include "chromeos/constants/chromeos_switches.h"
 #include "components/account_id/account_id.h"
-#include "components/policy/core/browser/browser_policy_connector.h"
-#include "components/policy/policy_constants.h"
 #include "components/prefs/pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -82,66 +81,57 @@ class TestParentAccessServiceObserver : public ParentAccessService::Observer {
   DISALLOW_COPY_AND_ASSIGN(TestParentAccessServiceObserver);
 };
 
-class ParentAccessServiceTest : public policy::LoginPolicyTestBase {
+class ParentAccessServiceTest : public MixinBasedInProcessBrowserTest {
  public:
   ParentAccessServiceTest()
-      : test_observer_(
-            std::make_unique<TestParentAccessServiceObserver>(child_)) {}
+      : test_observer_(std::make_unique<TestParentAccessServiceObserver>(
+            logged_in_user_mixin_.GetAccountId())) {}
   ~ParentAccessServiceTest() override = default;
 
-  void SetUp() override {
-    // Recognize example.com (used by LoginPolicyTestBase) as non-enterprise
-    // account.
-    policy::BrowserPolicyConnector::SetNonEnterpriseDomainForTesting(
-        "example.com");
-
-    policy::LoginPolicyTestBase::SetUp();
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    MixinBasedInProcessBrowserTest::SetUpCommandLine(command_line);
+    command_line->AppendSwitch(switches::kOobeSkipPostLogin);
   }
 
   void SetUpOnMainThread() override {
     ASSERT_NO_FATAL_FAILURE(GetTestAccessCodeValues(&test_values_));
     ParentAccessService::Get().AddObserver(test_observer_.get());
-    policy::LoginPolicyTestBase::SetUpOnMainThread();
+    MixinBasedInProcessBrowserTest::SetUpOnMainThread();
+    logged_in_user_mixin_.SetUpOnMainThreadHelper(host_resolver(), this);
   }
 
   void TearDownOnMainThread() override {
     ParentAccessService::Get().RemoveObserver(test_observer_.get());
-    policy::LoginPolicyTestBase::TearDownOnMainThread();
-  }
-
-  std::string GetIdToken() const override {
-    return test::GetChildAccountOAuthIdToken();
+    MixinBasedInProcessBrowserTest::TearDownOnMainThread();
   }
 
  protected:
-  void LogInChild() {
-    SkipToLoginScreen();
-    LogIn(kAccountId, kAccountPassword, test::kChildAccountServiceFlags);
-  }
-
   // Updates the policy containing the Parent Access Code config.
   void UpdatePolicy(const base::DictionaryValue& dict) {
+    std::string config_string;
+    base::JSONWriter::Write(dict, &config_string);
+
+    logged_in_user_mixin_.GetUserPolicyMixin()
+        ->RequestPolicyUpdate()
+        ->policy_payload()
+        ->mutable_parentaccesscodeconfig()
+        ->set_value(config_string);
+
     const user_manager::UserManager* const user_manager =
         user_manager::UserManager::Get();
     EXPECT_TRUE(user_manager->GetActiveUser()->IsChild());
     Profile* child_profile =
         ProfileHelper::Get()->GetProfileByUser(user_manager->GetActiveUser());
 
-    std::string config_string;
-    base::JSONWriter::Write(dict, &config_string);
-
-    base::DictionaryValue policy;
-    policy.SetKey(policy::key::kParentAccessCodeConfig,
-                  base::Value(config_string));
-    user_policy_helper()->SetPolicyAndWait(
-        std::move(policy), base::DictionaryValue(), child_profile);
+    logged_in_user_mixin_.GetUserPolicyTestHelper()->RefreshPolicyAndWait(
+        child_profile);
   }
 
   // Performs |code| validation on ParentAccessService singleton using the
   // |validation time| and returns the result.
   bool ValidateAccessCode(const std::string& code, base::Time validation_time) {
-    return ParentAccessService::Get().ValidateParentAccessCode(child_, code,
-                                                               validation_time);
+    return ParentAccessService::Get().ValidateParentAccessCode(
+        logged_in_user_mixin_.GetAccountId(), code, validation_time);
   }
 
   // Checks if ParentAccessServiceObserver and ValidateParentAccessCodeCallback
@@ -152,8 +142,11 @@ class ParentAccessServiceTest : public policy::LoginPolicyTestBase {
     EXPECT_EQ(failure_count, test_observer_->validation_results_.failure_count);
   }
 
-  const AccountId child_ = AccountId::FromUserEmail(kAccountId);
   AccessCodeValues test_values_;
+  chromeos::LoggedInUserMixin logged_in_user_mixin_{
+      &mixin_host_,           LoggedInUserMixin::LogInType::kChild,
+      embedded_test_server(), true /*should_launch_browser*/,
+      base::nullopt,          false /*include_initial_user*/};
   std::unique_ptr<TestParentAccessServiceObserver> test_observer_;
 
  private:
@@ -161,8 +154,6 @@ class ParentAccessServiceTest : public policy::LoginPolicyTestBase {
 };
 
 IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoConfigAvailable) {
-  LogInChild();
-
   auto test_value = test_values_.begin();
   EXPECT_FALSE(ValidateAccessCode(test_value->second, test_value->first));
 
@@ -170,8 +161,6 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoConfigAvailable) {
 }
 
 IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoValidConfigAvailable) {
-  LogInChild();
-
   std::vector<AccessCodeConfig> old_configs;
   old_configs.emplace_back(GetInvalidTestConfig());
   UpdatePolicy(PolicyFromConfigs(GetInvalidTestConfig(), GetInvalidTestConfig(),
@@ -184,8 +173,6 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoValidConfigAvailable) {
 }
 
 IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithFutureConfig) {
-  LogInChild();
-
   std::vector<AccessCodeConfig> old_configs;
   old_configs.emplace_back(GetInvalidTestConfig());
   UpdatePolicy(PolicyFromConfigs(GetDefaultTestConfig(), GetInvalidTestConfig(),
@@ -198,8 +185,6 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithFutureConfig) {
 }
 
 IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithCurrentConfig) {
-  LogInChild();
-
   std::vector<AccessCodeConfig> old_configs;
   old_configs.emplace_back(GetInvalidTestConfig());
   UpdatePolicy(PolicyFromConfigs(GetInvalidTestConfig(), GetDefaultTestConfig(),
@@ -212,8 +197,6 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithCurrentConfig) {
 }
 
 IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithOldConfig) {
-  LogInChild();
-
   std::vector<AccessCodeConfig> old_configs;
   old_configs.emplace_back(GetInvalidTestConfig());
   old_configs.emplace_back(GetDefaultTestConfig());
@@ -227,8 +210,6 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithOldConfig) {
 }
 
 IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, MultipleValidationAttempts) {
-  LogInChild();
-
   AccessCodeValues::iterator test_value = test_values_.begin();
 
   // No config - validation should fail.
@@ -251,8 +232,6 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, MultipleValidationAttempts) {
 }
 
 IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoObserver) {
-  LogInChild();
-
   ParentAccessService::Get().RemoveObserver(test_observer_.get());
 
   UpdatePolicy(
@@ -265,8 +244,6 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoObserver) {
 }
 
 IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoAccountId) {
-  LogInChild();
-
   ParentAccessService::Get().RemoveObserver(test_observer_.get());
 
   UpdatePolicy(
@@ -279,8 +256,6 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoAccountId) {
 }
 
 IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, InvalidAccountId) {
-  LogInChild();
-
   ParentAccessService::Get().RemoveObserver(test_observer_.get());
 
   UpdatePolicy(

chrome/browser/chromeos/policy/user_policy_test_helper.cc

@@ -75,7 +75,10 @@ void UserPolicyTestHelper::SetPolicyAndWait(
     const base::Value& recommended_policy,
     Profile* profile) {
   SetPolicy(mandatory_policy, recommended_policy);
+  RefreshPolicyAndWait(profile);
+}
 
+void UserPolicyTestHelper::RefreshPolicyAndWait(Profile* profile) {
   policy::ProfilePolicyConnector* const profile_connector =
       profile->GetProfilePolicyConnector();
   policy::PolicyService* const policy_service =

chrome/browser/chromeos/policy/user_policy_test_helper.h

@@ -39,12 +39,15 @@ class UserPolicyTestHelper {
   // unnecessary to call this function.
   void WaitForInitialPolicy(Profile* profile);
 
-  // Update the policy test server with the given policy. Then refresh and wait
-  // for the new policy being applied to |profile|.
+  // Updates the policy test server with the given policy. Then calls
+  // RefreshPolicyAndWait().
   void SetPolicyAndWait(const base::Value& mandatory_policy,
                         const base::Value& recommended_policy,
                         Profile* profile);
 
+  // Refreshes and waits for the new policy being applied to |profile|.
+  void RefreshPolicyAndWait(Profile* profile);
+
  private:
   const std::string account_id_;
   chromeos::LocalPolicyTestServerMixin* local_policy_server_;

chrome/browser/supervised_user/logged_in_user_mixin.cc

@@ -46,8 +46,10 @@ LoggedInUserMixin::LoggedInUserMixin(
                                                FakeGaiaMixin::kFakeUserGaiaId)),
             ConvertUserType(type)),
       login_manager_(mixin_host, GetInitialUsers(user_, include_initial_user)),
-      policy_server_(mixin_host),
-      user_policy_(mixin_host, user_.account_id, &policy_server_),
+      local_policy_server_(mixin_host),
+      user_policy_(mixin_host, user_.account_id, &local_policy_server_),
+      user_policy_helper_(user_.account_id.GetUserEmail(),
+                          &local_policy_server_),
       embedded_test_server_setup_(mixin_host, embedded_test_server),
       fake_gaia_(mixin_host, embedded_test_server) {
   // By default, LoginManagerMixin will set up user session manager not to
@@ -67,6 +69,10 @@ void LoggedInUserMixin::SetUpOnMainThreadHelper(
   // account.google.com requests would never reach fake GAIA server without
   // this.
   host_resolver->AddRule("*", "127.0.0.1");
+  // Call RequestPolicyUpdate() to set up policy, which prevents the call to
+  // LogInUser() below from hanging indefinitely when there's no initial user
+  // and wait_for_active_session is true.
+  GetUserPolicyMixin()->RequestPolicyUpdate();
   LogInUser(issue_any_scope_token, wait_for_active_session);
   // Set the private |browser_| member in InProcessBrowserTest.
   // Otherwise calls to InProcessBrowserTest::browser() returns null and leads

chrome/browser/supervised_user/logged_in_user_mixin.h

@@ -12,6 +12,7 @@
 #include "chrome/browser/chromeos/login/test/local_policy_test_server_mixin.h"
 #include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
 #include "chrome/browser/chromeos/login/test/user_policy_mixin.h"
+#include "chrome/browser/chromeos/policy/user_policy_test_helper.h"
 
 class AccountId;
 
@@ -76,10 +77,11 @@ class LoggedInUserMixin {
   // Call this function in your test class's SetUpOnMainThread() after calling
   // MixinBasedInProcessBrowserTest::SetUpOnMainThread().
   // This functions does the following:
-  // * Reroute all requests to localhost.
-  // * Log in as regular or child account depending on the |type| argument
+  // * Reroutes all requests to localhost.
+  // * Sets up user policy.
+  // * Logs in as regular or child account depending on the |type| argument
   // passed to the constructor.
-  // * Call InProcessBrowserTest::SelectFirstBrowser() so that browser()
+  // * Calls InProcessBrowserTest::SelectFirstBrowser() so that browser()
   // returns a non-null browser instance. Note: This call will only be effective
   // if should_launch_browser was set to true in the constructor.
   void SetUpOnMainThreadHelper(net::RuleBasedHostResolverProc* host_resolver,
@@ -99,19 +101,24 @@ class LoggedInUserMixin {
   LoginManagerMixin* GetLoginManagerMixin() { return &login_manager_; }
 
   LocalPolicyTestServerMixin* GetLocalPolicyTestServerMixin() {
-    return &policy_server_;
+    return &local_policy_server_;
   }
 
   UserPolicyMixin* GetUserPolicyMixin() { return &user_policy_; }
 
+  policy::UserPolicyTestHelper* GetUserPolicyTestHelper() {
+    return &user_policy_helper_;
+  }
+
   const AccountId& GetAccountId() { return user_.account_id; }
 
  private:
   LoginManagerMixin::TestUserInfo user_;
   LoginManagerMixin login_manager_;
 
-  LocalPolicyTestServerMixin policy_server_;
+  LocalPolicyTestServerMixin local_policy_server_;
   UserPolicyMixin user_policy_;
+  policy::UserPolicyTestHelper user_policy_helper_;
 
   EmbeddedTestServerSetupMixin embedded_test_server_setup_;
   FakeGaiaMixin fake_gaia_;