Commit d9afc66b authored by Aga Wronska's avatar Aga Wronska Committed by Commit Bot

Revert "Convert ParentAccessServiceTest to LoggedInUserMixin for child login"

This reverts commit 5e94ee4e.

Reason for revert: ParentAccessServiceTest tests are failing on linux-chromeos-google-rel https://crbug.com/1023998

Original change's description:
> 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/+/1909273
> Reviewed-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}

TBR=xiyuan@chromium.org,agawronska@chromium.org,tobyhuang@chromium.org

Change-Id: I0bb109845474112a545431adb5f575f135e69dfd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1014663, 1023998
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1913577Reviewed-by: default avatarAga Wronska <agawronska@chromium.org>
Commit-Queue: Aga Wronska <agawronska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714723}
parent e88f0124
...@@ -10,15 +10,17 @@ ...@@ -10,15 +10,17 @@
#include "base/bind.h" #include "base/bind.h"
#include "base/json/json_writer.h" #include "base/json/json_writer.h"
#include "base/macros.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/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_service.h"
#include "chrome/browser/chromeos/child_accounts/parent_access_code/parent_access_test_utils.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/policy/user_policy_test_helper.h"
#include "chrome/browser/chromeos/profiles/profile_helper.h" #include "chrome/browser/chromeos/profiles/profile_helper.h"
#include "chrome/browser/profiles/profile.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/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 "components/prefs/pref_service.h"
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
...@@ -80,52 +82,66 @@ class TestParentAccessServiceObserver : public ParentAccessService::Observer { ...@@ -80,52 +82,66 @@ class TestParentAccessServiceObserver : public ParentAccessService::Observer {
DISALLOW_COPY_AND_ASSIGN(TestParentAccessServiceObserver); DISALLOW_COPY_AND_ASSIGN(TestParentAccessServiceObserver);
}; };
class ParentAccessServiceTest : public MixinBasedInProcessBrowserTest { class ParentAccessServiceTest : public policy::LoginPolicyTestBase {
public: public:
ParentAccessServiceTest() ParentAccessServiceTest()
: test_observer_(std::make_unique<TestParentAccessServiceObserver>( : test_observer_(
logged_in_user_mixin_.GetAccountId())) {} std::make_unique<TestParentAccessServiceObserver>(child_)) {}
~ParentAccessServiceTest() override = default; ~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 { void SetUpOnMainThread() override {
ASSERT_NO_FATAL_FAILURE(GetTestAccessCodeValues(&test_values_)); ASSERT_NO_FATAL_FAILURE(GetTestAccessCodeValues(&test_values_));
ParentAccessService::Get().AddObserver(test_observer_.get()); ParentAccessService::Get().AddObserver(test_observer_.get());
MixinBasedInProcessBrowserTest::SetUpOnMainThread(); policy::LoginPolicyTestBase::SetUpOnMainThread();
logged_in_user_mixin_.SetUpOnMainThreadHelper(host_resolver(), this);
} }
void TearDownOnMainThread() override { void TearDownOnMainThread() override {
ParentAccessService::Get().RemoveObserver(test_observer_.get()); ParentAccessService::Get().RemoveObserver(test_observer_.get());
MixinBasedInProcessBrowserTest::TearDownOnMainThread(); policy::LoginPolicyTestBase::TearDownOnMainThread();
}
std::string GetIdToken() const override {
return test::GetChildAccountOAuthIdToken();
} }
protected: protected:
void LogInChild() {
SkipToLoginScreen();
LogIn(kAccountId, kAccountPassword, test::kChildAccountServiceFlags);
}
// Updates the policy containing the Parent Access Code config. // Updates the policy containing the Parent Access Code config.
void UpdatePolicy(const base::DictionaryValue& dict) { 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 = const user_manager::UserManager* const user_manager =
user_manager::UserManager::Get(); user_manager::UserManager::Get();
EXPECT_TRUE(user_manager->GetActiveUser()->IsChild()); EXPECT_TRUE(user_manager->GetActiveUser()->IsChild());
Profile* child_profile = Profile* child_profile =
ProfileHelper::Get()->GetProfileByUser(user_manager->GetActiveUser()); ProfileHelper::Get()->GetProfileByUser(user_manager->GetActiveUser());
logged_in_user_mixin_.GetUserPolicyTestHelper()->RefreshPolicyAndWait( std::string config_string;
child_profile); 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);
} }
// Performs |code| validation on ParentAccessService singleton using the // Performs |code| validation on ParentAccessService singleton using the
// |validation time| and returns the result. // |validation time| and returns the result.
bool ValidateAccessCode(const std::string& code, base::Time validation_time) { bool ValidateAccessCode(const std::string& code, base::Time validation_time) {
return ParentAccessService::Get().ValidateParentAccessCode( return ParentAccessService::Get().ValidateParentAccessCode(child_, code,
logged_in_user_mixin_.GetAccountId(), code, validation_time); validation_time);
} }
// Checks if ParentAccessServiceObserver and ValidateParentAccessCodeCallback // Checks if ParentAccessServiceObserver and ValidateParentAccessCodeCallback
...@@ -136,11 +152,8 @@ class ParentAccessServiceTest : public MixinBasedInProcessBrowserTest { ...@@ -136,11 +152,8 @@ class ParentAccessServiceTest : public MixinBasedInProcessBrowserTest {
EXPECT_EQ(failure_count, test_observer_->validation_results_.failure_count); EXPECT_EQ(failure_count, test_observer_->validation_results_.failure_count);
} }
const AccountId child_ = AccountId::FromUserEmail(kAccountId);
AccessCodeValues test_values_; 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_; std::unique_ptr<TestParentAccessServiceObserver> test_observer_;
private: private:
...@@ -148,6 +161,8 @@ class ParentAccessServiceTest : public MixinBasedInProcessBrowserTest { ...@@ -148,6 +161,8 @@ class ParentAccessServiceTest : public MixinBasedInProcessBrowserTest {
}; };
IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoConfigAvailable) { IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoConfigAvailable) {
LogInChild();
auto test_value = test_values_.begin(); auto test_value = test_values_.begin();
EXPECT_FALSE(ValidateAccessCode(test_value->second, test_value->first)); EXPECT_FALSE(ValidateAccessCode(test_value->second, test_value->first));
...@@ -155,6 +170,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoConfigAvailable) { ...@@ -155,6 +170,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoConfigAvailable) {
} }
IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoValidConfigAvailable) { IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoValidConfigAvailable) {
LogInChild();
std::vector<AccessCodeConfig> old_configs; std::vector<AccessCodeConfig> old_configs;
old_configs.emplace_back(GetInvalidTestConfig()); old_configs.emplace_back(GetInvalidTestConfig());
UpdatePolicy(PolicyFromConfigs(GetInvalidTestConfig(), GetInvalidTestConfig(), UpdatePolicy(PolicyFromConfigs(GetInvalidTestConfig(), GetInvalidTestConfig(),
...@@ -167,6 +184,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoValidConfigAvailable) { ...@@ -167,6 +184,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoValidConfigAvailable) {
} }
IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithFutureConfig) { IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithFutureConfig) {
LogInChild();
std::vector<AccessCodeConfig> old_configs; std::vector<AccessCodeConfig> old_configs;
old_configs.emplace_back(GetInvalidTestConfig()); old_configs.emplace_back(GetInvalidTestConfig());
UpdatePolicy(PolicyFromConfigs(GetDefaultTestConfig(), GetInvalidTestConfig(), UpdatePolicy(PolicyFromConfigs(GetDefaultTestConfig(), GetInvalidTestConfig(),
...@@ -179,6 +198,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithFutureConfig) { ...@@ -179,6 +198,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithFutureConfig) {
} }
IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithCurrentConfig) { IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithCurrentConfig) {
LogInChild();
std::vector<AccessCodeConfig> old_configs; std::vector<AccessCodeConfig> old_configs;
old_configs.emplace_back(GetInvalidTestConfig()); old_configs.emplace_back(GetInvalidTestConfig());
UpdatePolicy(PolicyFromConfigs(GetInvalidTestConfig(), GetDefaultTestConfig(), UpdatePolicy(PolicyFromConfigs(GetInvalidTestConfig(), GetDefaultTestConfig(),
...@@ -191,6 +212,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithCurrentConfig) { ...@@ -191,6 +212,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithCurrentConfig) {
} }
IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithOldConfig) { IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithOldConfig) {
LogInChild();
std::vector<AccessCodeConfig> old_configs; std::vector<AccessCodeConfig> old_configs;
old_configs.emplace_back(GetInvalidTestConfig()); old_configs.emplace_back(GetInvalidTestConfig());
old_configs.emplace_back(GetDefaultTestConfig()); old_configs.emplace_back(GetDefaultTestConfig());
...@@ -204,6 +227,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithOldConfig) { ...@@ -204,6 +227,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, ValidationWithOldConfig) {
} }
IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, MultipleValidationAttempts) { IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, MultipleValidationAttempts) {
LogInChild();
AccessCodeValues::iterator test_value = test_values_.begin(); AccessCodeValues::iterator test_value = test_values_.begin();
// No config - validation should fail. // No config - validation should fail.
...@@ -226,6 +251,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, MultipleValidationAttempts) { ...@@ -226,6 +251,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, MultipleValidationAttempts) {
} }
IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoObserver) { IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoObserver) {
LogInChild();
ParentAccessService::Get().RemoveObserver(test_observer_.get()); ParentAccessService::Get().RemoveObserver(test_observer_.get());
UpdatePolicy( UpdatePolicy(
...@@ -238,6 +265,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoObserver) { ...@@ -238,6 +265,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoObserver) {
} }
IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoAccountId) { IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoAccountId) {
LogInChild();
ParentAccessService::Get().RemoveObserver(test_observer_.get()); ParentAccessService::Get().RemoveObserver(test_observer_.get());
UpdatePolicy( UpdatePolicy(
...@@ -250,6 +279,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoAccountId) { ...@@ -250,6 +279,8 @@ IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, NoAccountId) {
} }
IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, InvalidAccountId) { IN_PROC_BROWSER_TEST_F(ParentAccessServiceTest, InvalidAccountId) {
LogInChild();
ParentAccessService::Get().RemoveObserver(test_observer_.get()); ParentAccessService::Get().RemoveObserver(test_observer_.get());
UpdatePolicy( UpdatePolicy(
......
...@@ -75,10 +75,7 @@ void UserPolicyTestHelper::SetPolicyAndWait( ...@@ -75,10 +75,7 @@ void UserPolicyTestHelper::SetPolicyAndWait(
const base::Value& recommended_policy, const base::Value& recommended_policy,
Profile* profile) { Profile* profile) {
SetPolicy(mandatory_policy, recommended_policy); SetPolicy(mandatory_policy, recommended_policy);
RefreshPolicyAndWait(profile);
}
void UserPolicyTestHelper::RefreshPolicyAndWait(Profile* profile) {
policy::ProfilePolicyConnector* const profile_connector = policy::ProfilePolicyConnector* const profile_connector =
profile->GetProfilePolicyConnector(); profile->GetProfilePolicyConnector();
policy::PolicyService* const policy_service = policy::PolicyService* const policy_service =
......
...@@ -39,15 +39,12 @@ class UserPolicyTestHelper { ...@@ -39,15 +39,12 @@ class UserPolicyTestHelper {
// unnecessary to call this function. // unnecessary to call this function.
void WaitForInitialPolicy(Profile* profile); void WaitForInitialPolicy(Profile* profile);
// Updates the policy test server with the given policy. Then calls // Update the policy test server with the given policy. Then refresh and wait
// RefreshPolicyAndWait(). // for the new policy being applied to |profile|.
void SetPolicyAndWait(const base::Value& mandatory_policy, void SetPolicyAndWait(const base::Value& mandatory_policy,
const base::Value& recommended_policy, const base::Value& recommended_policy,
Profile* profile); Profile* profile);
// Refreshes and waits for the new policy being applied to |profile|.
void RefreshPolicyAndWait(Profile* profile);
private: private:
const std::string account_id_; const std::string account_id_;
chromeos::LocalPolicyTestServerMixin* local_policy_server_; chromeos::LocalPolicyTestServerMixin* local_policy_server_;
......
...@@ -46,10 +46,8 @@ LoggedInUserMixin::LoggedInUserMixin( ...@@ -46,10 +46,8 @@ LoggedInUserMixin::LoggedInUserMixin(
FakeGaiaMixin::kFakeUserGaiaId)), FakeGaiaMixin::kFakeUserGaiaId)),
ConvertUserType(type)), ConvertUserType(type)),
login_manager_(mixin_host, GetInitialUsers(user_, include_initial_user)), login_manager_(mixin_host, GetInitialUsers(user_, include_initial_user)),
local_policy_server_(mixin_host), policy_server_(mixin_host),
user_policy_(mixin_host, user_.account_id, &local_policy_server_), user_policy_(mixin_host, user_.account_id, &policy_server_),
user_policy_helper_(user_.account_id.GetUserEmail(),
&local_policy_server_),
embedded_test_server_setup_(mixin_host, embedded_test_server), embedded_test_server_setup_(mixin_host, embedded_test_server),
fake_gaia_(mixin_host, embedded_test_server) { fake_gaia_(mixin_host, embedded_test_server) {
// By default, LoginManagerMixin will set up user session manager not to // By default, LoginManagerMixin will set up user session manager not to
...@@ -69,10 +67,6 @@ void LoggedInUserMixin::SetUpOnMainThreadHelper( ...@@ -69,10 +67,6 @@ void LoggedInUserMixin::SetUpOnMainThreadHelper(
// account.google.com requests would never reach fake GAIA server without // account.google.com requests would never reach fake GAIA server without
// this. // this.
host_resolver->AddRule("*", "127.0.0.1"); 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); LogInUser(issue_any_scope_token, wait_for_active_session);
// Set the private |browser_| member in InProcessBrowserTest. // Set the private |browser_| member in InProcessBrowserTest.
// Otherwise calls to InProcessBrowserTest::browser() returns null and leads // Otherwise calls to InProcessBrowserTest::browser() returns null and leads
......
...@@ -12,7 +12,6 @@ ...@@ -12,7 +12,6 @@
#include "chrome/browser/chromeos/login/test/local_policy_test_server_mixin.h" #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/login_manager_mixin.h"
#include "chrome/browser/chromeos/login/test/user_policy_mixin.h" #include "chrome/browser/chromeos/login/test/user_policy_mixin.h"
#include "chrome/browser/chromeos/policy/user_policy_test_helper.h"
class AccountId; class AccountId;
...@@ -100,24 +99,19 @@ class LoggedInUserMixin { ...@@ -100,24 +99,19 @@ class LoggedInUserMixin {
LoginManagerMixin* GetLoginManagerMixin() { return &login_manager_; } LoginManagerMixin* GetLoginManagerMixin() { return &login_manager_; }
LocalPolicyTestServerMixin* GetLocalPolicyTestServerMixin() { LocalPolicyTestServerMixin* GetLocalPolicyTestServerMixin() {
return &local_policy_server_; return &policy_server_;
} }
UserPolicyMixin* GetUserPolicyMixin() { return &user_policy_; } UserPolicyMixin* GetUserPolicyMixin() { return &user_policy_; }
policy::UserPolicyTestHelper* GetUserPolicyTestHelper() {
return &user_policy_helper_;
}
const AccountId& GetAccountId() { return user_.account_id; } const AccountId& GetAccountId() { return user_.account_id; }
private: private:
LoginManagerMixin::TestUserInfo user_; LoginManagerMixin::TestUserInfo user_;
LoginManagerMixin login_manager_; LoginManagerMixin login_manager_;
LocalPolicyTestServerMixin local_policy_server_; LocalPolicyTestServerMixin policy_server_;
UserPolicyMixin user_policy_; UserPolicyMixin user_policy_;
policy::UserPolicyTestHelper user_policy_helper_;
EmbeddedTestServerSetupMixin embedded_test_server_setup_; EmbeddedTestServerSetupMixin embedded_test_server_setup_;
FakeGaiaMixin fake_gaia_; FakeGaiaMixin fake_gaia_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment