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: I66f8c308e87efd49945ac322d969d162104ff130
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-Commit-Position: refs/heads/master@{#714592}

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

@@ -10,17 +10,15 @@
 #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 "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 +80,52 @@ 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 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 +136,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 +148,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 +155,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 +167,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 +179,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 +191,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 +204,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 +226,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 +238,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 +250,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;
 
@@ -99,19 +100,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_;