Skip to content

Commit

Permalink
Make PersistentSessionManagerClient more widely usable
Browse files Browse the repository at this point in the history 
PersistentSessionManagerClient is fake session manager client
implementation used in auto_launched_kiosk_browsertest to test
kiosk session restart (e.g. due a crash), and handling of non empty set
of policy login switches. The client keeps track of the session state
and per-user flags, persisting them between PRE_ and main test runs.

This CL extracts this logic into a helper class - SessionFlagsManager.
The helper class does not implement FakeSessionManagerClient any more.
It relies on the existing session manager client to keep track of
session state. The session state (active user, and user flags) is
read from the session manager client, and persisted as
SessionFlagsManager starts going out of scope.

This makes LoginManagerMixin use SessionFlagsManager to set up login
screen command line (which is SessionFlagsManager's default behavior).
LoginManagerMixin exposes few methods that allow tests to configure
SessionFlagsManager - for one, to enable logic for persisting session
state between test runs (this is expected to be needed for a minority
of test, so SessionFlagsManager will not run this logic by default)

BUG=957725

Change-Id: Ia02b3cb660d206449dbad488ec9c57d2c9d67481
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1586516
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#655382}
  • Loading branch information
Toni Barzic authored and Commit Bot committed Apr 30, 2019
1 parent 3ac0e91 commit ea500dc
Show file tree
Hide file tree
Showing 10 changed files with 370 additions and 237 deletions.
1 change: 0 additions & 1 deletion chrome/browser/chromeos/chrome_browser_main_chromeos.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,6 @@ void ChromeOSVersionCallback(const std::string& version) {
bool ShouldAutoLaunchKioskApp(const base::CommandLine& command_line) {
KioskAppManager* app_manager = KioskAppManager::Get();
return command_line.HasSwitch(switches::kLoginManager) &&
!command_line.HasSwitch(switches::kForceLoginManagerInTests) &&
app_manager->IsAutoLaunchEnabled() &&
KioskAppLaunchError::Get() == KioskAppLaunchError::NONE;
}
Expand Down
245 changes: 40 additions & 205 deletions chrome/browser/chromeos/login/auto_launched_kiosk_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
#include "chrome/browser/chromeos/app_mode/kiosk_app_launch_error.h"
#include "chrome/browser/chromeos/app_mode/kiosk_app_manager.h"
#include "chrome/browser/chromeos/login/app_launch_controller.h"
#include "chrome/browser/chromeos/login/mixin_based_in_process_browser_test.h"
#include "chrome/browser/chromeos/login/test/embedded_test_server_mixin.h"
#include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
#include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.h"
#include "chrome/browser/chromeos/policy/device_local_account.h"
#include "chrome/browser/chromeos/policy/device_policy_builder.h"
Expand All @@ -31,11 +34,11 @@
#include "chrome/common/chrome_constants.h"
#include "chrome/common/chrome_paths.h"
#include "chrome/common/pref_names.h"
#include "chromeos/dbus/cryptohome/cryptohome_client.h"
#include "chromeos/dbus/dbus_thread_manager.h"
#include "chromeos/dbus/session_manager/fake_session_manager_client.h"
#include "chromeos/dbus/shill/shill_manager_client.h"
#include "chromeos/tpm/stub_install_attributes.h"
#include "components/crx_file/crx_verifier.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "content/public/browser/notification_service.h"
Expand All @@ -47,7 +50,6 @@
#include "extensions/test/extension_test_message_listener.h"
#include "extensions/test/result_catcher.h"
#include "net/dns/mock_host_resolver.h"
#include "third_party/cros_system_api/switches/chrome_switches.h"

namespace em = enterprise_management;

Expand Down Expand Up @@ -93,168 +95,6 @@ constexpr char kTestManagementApiSecondaryApp[] =

constexpr char kTestAccountId[] = "enterprise-kiosk-app@localhost";

constexpr char kSessionManagerStateCache[] = "test_session_manager_state.json";

// Keys for values in dictionary used to preserve session manager state.
constexpr char kLoginArgsKey[] = "login_args";
constexpr char kExtraArgsKey[] = "extra_args";
constexpr char kArgNameKey[] = "name";
constexpr char kArgValueKey[] = "value";

// Default set policy switches.
constexpr struct {
const char* name;
const char* value;
} kDefaultPolicySwitches[] = {{"test_switch_1", ""},
{"test_switch_2", "test_switch_2_value"}};

// Fake session manager implementation that persists its state in local file.
// It can be used to preserve session state in PRE_ browser tests.
// Primarily used for testing user/login switches.
class PersistentSessionManagerClient : public FakeSessionManagerClient {
public:
PersistentSessionManagerClient() {}

~PersistentSessionManagerClient() override {
PersistFlagsToFile(backing_file_);
}

// Initializes session state (primarily session flags)- if |backing_file|
// exists, the session state is restored from the file value. Otherwise it's
// set to the default session state.
void Initialize(const base::FilePath& backing_file) {
backing_file_ = backing_file;

if (ExtractFlagsFromFile(backing_file_))
return;

// Failed to extract ached flags - set the default values.
login_args_ = {{"login-manager", ""}};

extra_args_ = {{switches::kPolicySwitchesBegin, ""}};
for (size_t i = 0; i < base::size(kDefaultPolicySwitches); ++i) {
extra_args_.push_back(
{kDefaultPolicySwitches[i].name, kDefaultPolicySwitches[i].value});
}
extra_args_.push_back({switches::kPolicySwitchesEnd, ""});
}

void AppendSwitchesToCommandLine(base::CommandLine* command_line) {
for (const auto& flag : login_args_)
command_line->AppendSwitchASCII(flag.name, flag.value);
for (const auto& flag : extra_args_)
command_line->AppendSwitchASCII(flag.name, flag.value);
}

void StartSession(
const cryptohome::AccountIdentifier& cryptohome_id) override {
FakeSessionManagerClient::StartSession(cryptohome_id);

std::string user_id_hash =
CryptohomeClient::GetStubSanitizedUsername(cryptohome_id);
login_args_ = {{"login-user", cryptohome_id.account_id()},
{"login-profile", user_id_hash}};
}

void StopSession() override {
FakeSessionManagerClient::StopSession();

login_args_ = {{"login-manager", ""}};
}

bool SupportsRestartToApplyUserFlags() const override { return true; }

void SetFlagsForUser(const cryptohome::AccountIdentifier& identification,
const std::vector<std::string>& flags) override {
extra_args_.clear();
FakeSessionManagerClient::SetFlagsForUser(identification, flags);

std::vector<std::string> argv = {"" /* Empty program */};
argv.insert(argv.end(), flags.begin(), flags.end());

// Parse flag name-value pairs using command line initialization.
base::CommandLine cmd_line(base::CommandLine::NO_PROGRAM);
cmd_line.InitFromArgv(argv);

for (const auto& flag : cmd_line.GetSwitches())
extra_args_.push_back({flag.first, flag.second});
}

private:
// Keeps information about a switch - its name and value.
struct Switch {
std::string name;
std::string value;
};

bool ExtractFlagsFromFile(const base::FilePath& backing_file) {
JSONFileValueDeserializer deserializer(backing_file);

int error_code = 0;
std::unique_ptr<base::Value> value =
deserializer.Deserialize(&error_code, nullptr);
if (error_code != JSONFileValueDeserializer::JSON_NO_ERROR)
return false;

std::unique_ptr<base::DictionaryValue> value_dict =
base::DictionaryValue::From(std::move(value));
CHECK(value_dict);

CHECK(InitArgListFromCachedValue(*value_dict, kLoginArgsKey, &login_args_));
CHECK(InitArgListFromCachedValue(*value_dict, kExtraArgsKey, &extra_args_));
return true;
}

bool PersistFlagsToFile(const base::FilePath& backing_file) {
base::DictionaryValue cached_state;
cached_state.Set(kLoginArgsKey, GetArgListValue(login_args_));
cached_state.Set(kExtraArgsKey, GetArgListValue(extra_args_));

JSONFileValueSerializer serializer(backing_file);
return serializer.Serialize(cached_state);
}

std::unique_ptr<base::ListValue> GetArgListValue(
const std::vector<Switch>& args) {
std::unique_ptr<base::ListValue> result(new base::ListValue());
for (const auto& arg : args) {
result->Append(extensions::DictionaryBuilder()
.Set(kArgNameKey, arg.name)
.Set(kArgValueKey, arg.value)
.Build());
}
return result;
}

bool InitArgListFromCachedValue(const base::DictionaryValue& cache_value,
const std::string& list_key,
std::vector<Switch>* arg_list_out) {
arg_list_out->clear();
const base::ListValue* arg_list_value;
if (!cache_value.GetList(list_key, &arg_list_value))
return false;
for (size_t i = 0; i < arg_list_value->GetSize(); ++i) {
const base::DictionaryValue* arg_value;
if (!arg_list_value->GetDictionary(i, &arg_value))
return false;
Switch arg;
if (!arg_value->GetStringASCII(kArgNameKey, &arg.name) ||
!arg_value->GetStringASCII(kArgValueKey, &arg.value)) {
return false;
}
arg_list_out->push_back(arg);
}
return true;
}

std::vector<Switch> login_args_;
std::vector<Switch> extra_args_;

base::FilePath backing_file_;

DISALLOW_COPY_AND_ASSIGN(PersistentSessionManagerClient);
};

// Used to listen for app termination notification.
class TerminationObserver : public content::NotificationObserver {
public:
Expand Down Expand Up @@ -284,15 +124,13 @@ class TerminationObserver : public content::NotificationObserver {

} // namespace

class AutoLaunchedKioskTest : public extensions::ExtensionApiTest {
class AutoLaunchedKioskTest : public MixinBasedInProcessBrowserTest {
public:
AutoLaunchedKioskTest()
: install_attributes_(
chromeos::StubInstallAttributes::CreateCloudManaged("domain.com",
"device_id")),
fake_cws_(new FakeCWS) {
set_chromeos_user_ = false;
}
verifier_format_override_(crx_file::VerifierFormat::CRX3) {}

~AutoLaunchedKioskTest() override = default;

Expand All @@ -302,22 +140,23 @@ class AutoLaunchedKioskTest : public extensions::ExtensionApiTest {
}

void SetUp() override {
// Will be destroyed in ChromeBrowserMain.
fake_session_manager_ = new PersistentSessionManagerClient();

ASSERT_TRUE(embedded_test_server()->InitializeAndListen());
AppLaunchController::SkipSplashWaitForTesting();

extensions::ExtensionApiTest::SetUp();
login_manager_.set_session_restore_enabled();
login_manager_.SetDefaultLoginSwitches(
{std::make_pair("test_switch_1", ""),
std::make_pair("test_switch_2", "test_switch_2_value")});
MixinBasedInProcessBrowserTest::SetUp();
}

void SetUpCommandLine(base::CommandLine* command_line) override {
fake_cws_->Init(embedded_test_server());
fake_cws_->SetUpdateCrx(GetTestAppId(), GetTestAppId() + ".crx", "1.0.0");
fake_cws_.Init(embedded_test_server());
fake_cws_.SetUpdateCrx(GetTestAppId(), GetTestAppId() + ".crx", "1.0.0");

std::vector<std::string> secondary_apps = GetTestSecondaryAppIds();
for (const auto& secondary_app : secondary_apps)
fake_cws_->SetUpdateCrx(secondary_app, secondary_app + ".crx", "1.0.0");
extensions::ExtensionApiTest::SetUpCommandLine(command_line);
fake_cws_.SetUpdateCrx(secondary_app, secondary_app + ".crx", "1.0.0");

MixinBasedInProcessBrowserTest::SetUpCommandLine(command_line);
}

bool SetUpUserDataDirectory() override {
Expand All @@ -332,28 +171,25 @@ class AutoLaunchedKioskTest : public extensions::ExtensionApiTest {
if (!CacheDevicePolicyToLocalState(user_data_path))
return false;

// Restore session_manager state and ensure session manager flags are
// applied.
fake_session_manager_->Initialize(
user_data_path.Append(kSessionManagerStateCache));
fake_session_manager_->AppendSwitchesToCommandLine(
base::CommandLine::ForCurrentProcess());

return true;
return MixinBasedInProcessBrowserTest::SetUpUserDataDirectory();
}

void SetUpInProcessBrowserTestFixture() override {
host_resolver()->AddRule("*", "127.0.0.1");

fake_session_manager_->set_device_policy(
SessionManagerClient::InitializeFakeInMemory();

FakeSessionManagerClient::Get()->set_supports_restart_to_apply_user_flags(
true);
FakeSessionManagerClient::Get()->set_device_policy(
device_policy_helper_.device_policy()->GetBlob());
fake_session_manager_->set_device_local_account_policy(
FakeSessionManagerClient::Get()->set_device_local_account_policy(
kTestAccountId, device_local_account_policy_.GetBlob());

// Arbitrary non-empty state keys.
fake_session_manager_->set_server_backed_state_keys({"1"});
FakeSessionManagerClient::Get()->set_server_backed_state_keys({"1"});

extensions::ExtensionApiTest::SetUpInProcessBrowserTestFixture();
MixinBasedInProcessBrowserTest::SetUpInProcessBrowserTestFixture();
}

void PreRunTestOnMainThread() override {
Expand All @@ -370,17 +206,14 @@ class AutoLaunchedKioskTest : public extensions::ExtensionApiTest {

void SetUpOnMainThread() override {
extensions::browsertest_util::CreateAndInitializeLocalCache();

embedded_test_server()->StartAcceptingConnections();

extensions::ExtensionApiTest::SetUpOnMainThread();
MixinBasedInProcessBrowserTest::SetUpOnMainThread();
}

void TearDownOnMainThread() override {
app_window_loaded_listener_.reset();
termination_observer_.reset();

extensions::ExtensionApiTest::TearDownOnMainThread();
MixinBasedInProcessBrowserTest::TearDownOnMainThread();
}

void InitDevicePolicy() {
Expand Down Expand Up @@ -478,13 +311,11 @@ class AutoLaunchedKioskTest : public extensions::ExtensionApiTest {

void ExpectCommandLineHasDefaultPolicySwitches(
const base::CommandLine& cmd_line) {
for (size_t i = 0u; i < base::size(kDefaultPolicySwitches); ++i) {
EXPECT_TRUE(cmd_line.HasSwitch(kDefaultPolicySwitches[i].name))
<< "Missing flag " << kDefaultPolicySwitches[i].name;
EXPECT_EQ(kDefaultPolicySwitches[i].value,
cmd_line.GetSwitchValueASCII(kDefaultPolicySwitches[i].name))
<< "Invalid value for switch " << kDefaultPolicySwitches[i].name;
}
EXPECT_TRUE(cmd_line.HasSwitch("test_switch_1"));
EXPECT_EQ("", cmd_line.GetSwitchValueASCII("test_switch_1"));
EXPECT_TRUE(cmd_line.HasSwitch("test_switch_2"));
EXPECT_EQ("test_switch_2_value",
cmd_line.GetSwitchValueASCII("test_switch_2"));
}

protected:
Expand All @@ -495,9 +326,13 @@ class AutoLaunchedKioskTest : public extensions::ExtensionApiTest {
chromeos::ScopedStubInstallAttributes install_attributes_;
policy::UserPolicyBuilder device_local_account_policy_;
policy::DevicePolicyCrosTestHelper device_policy_helper_;
// Owned by SessionManagerClient global instance.
PersistentSessionManagerClient* fake_session_manager_;
std::unique_ptr<FakeCWS> fake_cws_;
FakeCWS fake_cws_;
extensions::SandboxedUnpacker::ScopedVerifierFormatOverrideForTest
verifier_format_override_;

EmbeddedTestServerSetupMixin embedded_test_server_setup_{
&mixin_host_, embedded_test_server()};
LoginManagerMixin login_manager_{&mixin_host_, {}};

DISALLOW_COPY_AND_ASSIGN(AutoLaunchedKioskTest);
};
Expand Down
19 changes: 1 addition & 18 deletions chrome/browser/chromeos/login/crash_restore_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,27 +266,10 @@ IN_PROC_BROWSER_TEST_F(CrashRestoreComplexTest, RestoreSessionForThreeUsers) {
// Tests crash restore flow for child user.
class CrashRestoreChildUserTest : public MixinBasedInProcessBrowserTest {
protected:
CrashRestoreChildUserTest() {
if (!content::IsPreTest())
login_manager_.set_skip_flags_setup(true);
}
CrashRestoreChildUserTest() { login_manager_.set_session_restore_enabled(); }
~CrashRestoreChildUserTest() override = default;

// MixinBasedInProcessBrowserTest:
void SetUpCommandLine(base::CommandLine* command_line) override {
if (!content::IsPreTest()) {
const cryptohome::AccountIdentifier cryptohome_id =
cryptohome::CreateAccountIdentifierFromAccountId(
test_user_.account_id);
command_line->AppendSwitchASCII(switches::kLoginUser,
cryptohome_id.account_id());
command_line->AppendSwitchASCII(
switches::kLoginProfile,
CryptohomeClient::GetStubSanitizedUsername(cryptohome_id));
}
MixinBasedInProcessBrowserTest::SetUpCommandLine(command_line);
}

void SetUpInProcessBrowserTestFixture() override {
// SessionManagerClient has to be in-memory to support setting sessionless
// user policy blob.
Expand Down
1 change: 0 additions & 1 deletion chrome/browser/chromeos/login/session/chrome_session_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ namespace {
bool ShouldAutoLaunchKioskApp(const base::CommandLine& command_line) {
KioskAppManager* app_manager = KioskAppManager::Get();
return command_line.HasSwitch(switches::kLoginManager) &&
!command_line.HasSwitch(switches::kForceLoginManagerInTests) &&
app_manager->IsAutoLaunchEnabled() &&
KioskAppLaunchError::Get() == KioskAppLaunchError::NONE &&
// IsOobeCompleted() is needed to prevent kiosk session start in case
Expand Down
Loading

0 comments on commit ea500dc

Please sign in to comment.