Skip to content

Commit

Permalink
Revert "Stop supporting Plugin VM device license key"
Browse files Browse the repository at this point in the history
This reverts commit 1cb9f0f.

Reason for revert: Breaking uprev 

Original change's description:
> Stop supporting Plugin VM device license key
>
> This has been superseded by ByteBot licensing, and has caused some
> confusion by users who had previously added such a key, as the policy
> is no longer available in the admin console (excl managedchrome).
>
> Bug: b/178895204
> Change-Id: I8c1715a2d7db9a5618c14e8f9805b831ab9e3f21
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2766562
> Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> Reviewed-by: Jason Lin <lxj@google.com>
> Commit-Queue: Timothy Loh <timloh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#865901}

Bug: b/178895204, 1192299
Change-Id: Ie09228479700abf1ed668ca1ec58651f8571168e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2784676
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Timothy Loh <timloh@chromium.org>
Commit-Queue: Tim Sergeant <tsergeant@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Owners-Override: Tim Sergeant <tsergeant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#866480}
  • Loading branch information
tim-loh authored and Chromium LUCI CQ committed Mar 25, 2021
1 parent c9fc5a5 commit 99a7837
Show file tree
Hide file tree
Showing 19 changed files with 152 additions and 50 deletions.
9 changes: 6 additions & 3 deletions chrome/browser/ash/plugin_vm/plugin_vm_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ PluginVmFeatures::~PluginVmFeatures() = default;
// * User should be affiliated.
// * PluginVmAllowed device policy should be set to true.
// * UserPluginVmAllowed user policy should be set to true.
// * PluginVmUserId user policy should be set.
// * At least one of the following should be set:
// * PluginVmLicenseKey policy.
// * PluginVmUserId policy.
bool PluginVmFeatures::IsAllowed(const Profile* profile, std::string* reason) {
// Check that PluginVm feature is enabled.
if (!base::FeatureList::IsEnabled(features::kPluginVm)) {
Expand All @@ -69,7 +71,7 @@ bool PluginVmFeatures::IsAllowed(const Profile* profile, std::string* reason) {
}

// Bypass other checks when a fake policy is set, or running linux-chromeos.
if (FakePolicyIsSet() || !base::SysInfo::IsRunningOnChromeOS())
if (FakeLicenseKeyIsSet() || !base::SysInfo::IsRunningOnChromeOS())
return true;

// Check that the device is enterprise enrolled.
Expand Down Expand Up @@ -111,7 +113,8 @@ bool PluginVmFeatures::IsAllowed(const Profile* profile, std::string* reason) {
return false;
}

if (GetPluginVmUserIdForProfile(profile).empty()) {
if (GetPluginVmLicenseKey().empty() &&
GetPluginVmUserIdForProfile(profile).empty()) {
VLOG(1) << "Parallels require a license be set up in policy.";
*reason = "License for the product is not set up in policy";
return false;
Expand Down
4 changes: 3 additions & 1 deletion chrome/browser/ash/plugin_vm/plugin_vm_installer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,9 @@ PluginVmInstaller::~PluginVmInstaller() = default;
void PluginVmInstaller::CheckLicense() {
UpdateInstallingState(InstallingState::kCheckingLicense);

if (skip_license_check_for_testing_) {
// If the server has provided a license key, responsibility of validating is
// passed to the Plugin VM application.
if (!GetPluginVmLicenseKey().empty()) {
OnLicenseChecked(true);
return;
}
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/ash/plugin_vm/plugin_vm_installer.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,6 @@ class PluginVmInstaller : public KeyedService,
// Returns free disk space required to install Plugin VM in bytes.
int64_t RequiredFreeDiskSpace();

void SkipLicenseCheckForTesting() { skip_license_check_for_testing_ = true; }
void SetFreeDiskSpaceForTesting(int64_t bytes) {
free_disk_space_for_testing_ = bytes;
}
Expand Down Expand Up @@ -278,7 +277,6 @@ class PluginVmInstaller : public KeyedService,
std::unique_ptr<PluginVmLicenseChecker> license_checker_;
bool using_drive_download_service_ = false;

bool skip_license_check_for_testing_ = false;
// -1 indicates not set
int64_t free_disk_space_for_testing_ = -1;
base::Optional<base::FilePath> downloaded_image_for_testing_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,6 @@ class PluginVmInstallerTestBase : public testing::Test {
installer_ = PluginVmInstallerFactory::GetForProfile(profile_.get());
observer_ = std::make_unique<StrictMock<MockObserver>>();
installer_->SetObserver(observer_.get());
installer_->SkipLicenseCheckForTesting();
installer_->SetFreeDiskSpaceForTesting(std::numeric_limits<int64_t>::max());
installer_->SetDownloadedImageForTesting(CreateZipFile());

Expand Down
5 changes: 3 additions & 2 deletions chrome/browser/ash/plugin_vm/plugin_vm_test_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ namespace plugin_vm {
namespace {

const char kDiskImageImportCommandUuid[] = "3922722bd7394acf85bf4d5a330d4a47";
const char kPluginVmLicenseKey[] = "LICENSE_KEY";
const char kDomain[] = "example.com";
const char kDeviceId[] = "device_id";

Expand Down Expand Up @@ -137,10 +138,10 @@ PluginVmTestHelper::~PluginVmTestHelper() = default;
void PluginVmTestHelper::SetPolicyRequirementsToAllowPluginVm() {
testing_profile_->GetPrefs()->SetBoolean(plugin_vm::prefs::kPluginVmAllowed,
true);
testing_profile_->GetPrefs()->SetString(plugin_vm::prefs::kPluginVmUserId,
"fake-id");
testing_profile_->ScopedCrosSettingsTestHelper()->SetBoolean(
chromeos::kPluginVmAllowed, true);
testing_profile_->ScopedCrosSettingsTestHelper()->SetString(
chromeos::kPluginVmLicenseKey, kPluginVmLicenseKey);
}

void PluginVmTestHelper::SetUserRequirementsToAllowPluginVm() {
Expand Down
50 changes: 45 additions & 5 deletions chrome/browser/ash/plugin_vm/plugin_vm_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,20 @@ const char kChromeOSBaseDirectoryDisplayText[] = "Network \u203a ChromeOS";

namespace {

static bool g_fake_policy_is_set = false;
static std::string& GetFakeLicenseKey() {
static base::NoDestructor<std::string> license_key;
return *license_key;
}

static base::RepeatingClosureList& GetFakeLicenceKeyListeners() {
static base::NoDestructor<base::RepeatingClosureList> instance;
return *instance;
}

static std::string& GetFakeUserId() {
static base::NoDestructor<std::string> user_id;
return *user_id;
}

} // namespace

Expand All @@ -57,24 +70,44 @@ bool IsPluginVmAppWindow(const aura::Window* window) {
return *app_id == "org.chromium.plugin_vm_ui";
}

std::string GetPluginVmLicenseKey() {
if (FakeLicenseKeyIsSet())
return GetFakeLicenseKey();
std::string plugin_vm_license_key;
if (!ash::CrosSettings::Get()->GetString(chromeos::kPluginVmLicenseKey,
&plugin_vm_license_key)) {
return std::string();
}
return plugin_vm_license_key;
}

std::string GetPluginVmUserIdForProfile(const Profile* profile) {
DCHECK(profile);
return profile->GetPrefs()->GetString(plugin_vm::prefs::kPluginVmUserId);
}

void SetFakePluginVmPolicy(Profile* profile,
const std::string& image_url,
const std::string& image_hash) {
const std::string& image_hash,
const std::string& license_key) {
DictionaryPrefUpdate update(profile->GetPrefs(),
plugin_vm::prefs::kPluginVmImage);
base::DictionaryValue* dict = update.Get();
dict->SetPath("url", base::Value(image_url));
dict->SetPath("hash", base::Value(image_hash));
g_fake_policy_is_set = true;

GetFakeLicenseKey() = license_key;

GetFakeLicenceKeyListeners().Notify();
GetFakeUserId() = "FAKE_USER_ID";
}

bool FakeLicenseKeyIsSet() {
return !GetFakeLicenseKey().empty();
}

bool FakePolicyIsSet() {
return g_fake_policy_is_set;
bool FakeUserIdIsSet() {
return !GetFakeUserId().empty();
}

void RemoveDriveDownloadDirectoryIfExists() {
Expand Down Expand Up @@ -142,6 +175,13 @@ PluginVmPolicySubscription::PluginVmPolicySubscription(
chromeos::kPluginVmAllowed,
base::BindRepeating(&PluginVmPolicySubscription::OnPolicyChanged,
base::Unretained(this)));
license_subscription_ = cros_settings->AddSettingsObserver(
chromeos::kPluginVmLicenseKey,
base::BindRepeating(&PluginVmPolicySubscription::OnPolicyChanged,
base::Unretained(this)));
fake_license_subscription_ = GetFakeLicenceKeyListeners().Add(
base::BindRepeating(&PluginVmPolicySubscription::OnPolicyChanged,
base::Unretained(this)));

is_allowed_ = PluginVmFeatures::Get()->IsAllowed(profile);
}
Expand Down
19 changes: 13 additions & 6 deletions chrome/browser/ash/plugin_vm/plugin_vm_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,18 +68,24 @@ void ShowPluginVmInstallerView(Profile* profile);
// the Plugin VM installer.
bool IsPluginVmAppWindow(const aura::Window* window);

// Retrieves the license key to be used for Plugin VM. If
// none is set this will return an empty string.
std::string GetPluginVmLicenseKey();

// Retrieves the User Id to be used for Plugin VM. If none is set this will
// return an empty string.
std::string GetPluginVmUserIdForProfile(const Profile* profile);

// Sets fake policy values and enables Plugin VM for tast tests. This sets
// global state.
// TODO(crbug.com/1025136): Set policy directly from tast instead of using a
// test helper function.
// Sets fake policy values and enables Plugin VM for testing. These set global
// state so this should be called with empty strings on tear down.
// TODO(crbug.com/1025136): Remove this once Tast supports setting test
// policies.
void SetFakePluginVmPolicy(Profile* profile,
const std::string& image_path,
const std::string& image_hash);
bool FakePolicyIsSet();
const std::string& image_hash,
const std::string& license_key);
bool FakeLicenseKeyIsSet();
bool FakeUserIdIsSet();

// Used to clean up the Plugin VM Drive download directory if it did not get
// removed when it should have, perhaps due to a crash.
Expand Down Expand Up @@ -114,6 +120,7 @@ class PluginVmPolicySubscription {

std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
base::CallbackListSubscription device_allowed_subscription_;
base::CallbackListSubscription license_subscription_;
base::CallbackListSubscription fake_license_subscription_;
};

Expand Down
38 changes: 21 additions & 17 deletions chrome/browser/ash/plugin_vm/plugin_vm_util_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,16 @@ TEST_F(PluginVmUtilTest, PluginVmShouldBeConfiguredOnceAllConditionsAreMet) {
EXPECT_TRUE(PluginVmFeatures::Get()->IsConfigured(testing_profile_.get()));
}

TEST_F(PluginVmUtilTest, GetPluginVmLicenseKey) {
// If no license key is set, the method should return the empty string.
EXPECT_EQ(std::string(), GetPluginVmLicenseKey());

const std::string kLicenseKey = "LICENSE_KEY";
testing_profile_->ScopedCrosSettingsTestHelper()->SetString(
chromeos::kPluginVmLicenseKey, kLicenseKey);
EXPECT_EQ(kLicenseKey, GetPluginVmLicenseKey());
}

TEST_F(PluginVmUtilTest, AddPluginVmPolicyObserver) {
const std::unique_ptr<PluginVmPolicySubscription> subscription =
std::make_unique<plugin_vm::PluginVmPolicySubscription>(
Expand All @@ -90,35 +100,29 @@ TEST_F(PluginVmUtilTest, AddPluginVmPolicyObserver) {
testing::Mock::VerifyAndClearExpectations(this);

EXPECT_CALL(*this, OnPolicyChanged(false));
testing_profile_->ScopedCrosSettingsTestHelper()->SetBoolean(
chromeos::kPluginVmAllowed, false);
testing_profile_->ScopedCrosSettingsTestHelper()->SetString(
chromeos::kPluginVmLicenseKey, "");
testing::Mock::VerifyAndClearExpectations(this);

EXPECT_CALL(*this, OnPolicyChanged(true));
testing_profile_->ScopedCrosSettingsTestHelper()->SetBoolean(
chromeos::kPluginVmAllowed, true);
const std::string kLicenseKey = "LICENSE_KEY";
testing_profile_->ScopedCrosSettingsTestHelper()->SetString(
chromeos::kPluginVmLicenseKey, kLicenseKey);
testing::Mock::VerifyAndClearExpectations(this);

EXPECT_CALL(*this, OnPolicyChanged(false));
testing_profile_->GetPrefs()->SetBoolean(plugin_vm::prefs::kPluginVmAllowed,
false);
testing_profile_->ScopedCrosSettingsTestHelper()->SetBoolean(
chromeos::kPluginVmAllowed, false);
testing::Mock::VerifyAndClearExpectations(this);

EXPECT_CALL(*this, OnPolicyChanged(true));
testing_profile_->GetPrefs()->SetBoolean(plugin_vm::prefs::kPluginVmAllowed,
true);
testing_profile_->ScopedCrosSettingsTestHelper()->SetBoolean(
chromeos::kPluginVmAllowed, true);
testing::Mock::VerifyAndClearExpectations(this);

EXPECT_CALL(*this, OnPolicyChanged(false));
testing_profile_->GetPrefs()->SetString(plugin_vm::prefs::kPluginVmUserId,
"");
testing::Mock::VerifyAndClearExpectations(this);

EXPECT_CALL(*this, OnPolicyChanged(true));
const std::string kPluginVmUserId = "fancy-user-id";
testing_profile_->GetPrefs()->SetString(plugin_vm::prefs::kPluginVmUserId,
kPluginVmUserId);
testing::Mock::VerifyAndClearExpectations(this);
testing_profile_->GetPrefs()->SetBoolean(plugin_vm::prefs::kPluginVmAllowed,
false);
}

TEST_F(PluginVmUtilTest, DriveUrlNonMatches) {
Expand Down
10 changes: 10 additions & 0 deletions chrome/browser/ash/settings/device_settings_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ const char* const kKnownSettings[] = {
kLoginAuthenticationBehavior,
kLoginVideoCaptureAllowedUrls,
kPluginVmAllowed,
kPluginVmLicenseKey,
kPolicyMissingMitigationMode,
kRebootOnShutdown,
kReleaseChannel,
Expand Down Expand Up @@ -901,6 +902,15 @@ void DecodeGenericPolicies(const em::ChromeDeviceSettingsProto& policy,
}
}

if (policy.has_plugin_vm_license_key()) {
const em::PluginVmLicenseKeyProto& container(
policy.plugin_vm_license_key());
if (container.has_plugin_vm_license_key()) {
new_values_cache->SetValue(
kPluginVmLicenseKey, base::Value(container.plugin_vm_license_key()));
}
}

// Default value of the policy in case it's missing.
int access_mode = em::DevicePrintersAccessModeProto::ACCESS_MODE_ALL;
// Use DevicePrintersAccessMode policy if present, otherwise Native version.
Expand Down
12 changes: 12 additions & 0 deletions chrome/browser/ash/settings/device_settings_provider_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,13 @@ class DeviceSettingsProviderTest : public DeviceSettingsTestBase {
BuildAndInstallDevicePolicy();
}

void SetPluginVmLicenseKeySetting(const std::string& plugin_vm_license_key) {
em::PluginVmLicenseKeyProto* proto =
device_policy_->payload().mutable_plugin_vm_license_key();
proto->set_plugin_vm_license_key(plugin_vm_license_key);
BuildAndInstallDevicePolicy();
}

void SetDeviceRebootOnUserSignout(
em::DeviceRebootOnUserSignoutProto::RebootOnSignoutMode value) {
EXPECT_CALL(*this, SettingChanged(_)).Times(AtLeast(1));
Expand Down Expand Up @@ -924,6 +931,11 @@ TEST_F(DeviceSettingsProviderTest, DecodePluginVmAllowedSetting) {
EXPECT_EQ(base::Value(false), *provider_->Get(kPluginVmAllowed));
}

TEST_F(DeviceSettingsProviderTest, DecodePluginVmLicenseKeySetting) {
SetPluginVmLicenseKeySetting("LICENSE_KEY");
EXPECT_EQ(base::Value("LICENSE_KEY"), *provider_->Get(kPluginVmLicenseKey));
}

TEST_F(DeviceSettingsProviderTest, DeviceRebootAfterUserSignout) {
using PolicyProto = em::DeviceRebootOnUserSignoutProto;

Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/chromeos/dbus/plugin_vm_service_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,14 @@ void PluginVmServiceProvider::GetLicenseData(
std::unique_ptr<dbus::Response> response =
dbus::Response::FromMethodCall(method_call);
plugin_vm_service::GetLicenseDataResponse payload;
if (plugin_vm::FakePolicyIsSet()) {
if (plugin_vm::FakeLicenseKeyIsSet()) {
payload.set_device_id(kFakeUUID);
} else {
payload.set_device_id(g_browser_process->platform_part()
->browser_policy_connector_chromeos()
->GetDirectoryApiID());
}
payload.set_license_key(plugin_vm::GetPluginVmLicenseKey());
dbus::MessageWriter writer(response.get());
writer.AppendProtoAsArrayOfBytes(payload);
std::move(response_sender).Run(std::move(response));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2202,11 +2202,11 @@ AutotestPrivateSetPluginVMPolicyFunction::Run() {
api::autotest_private::SetPluginVMPolicy::Params::Create(*args_));
EXTENSION_FUNCTION_VALIDATE(params);
DVLOG(1) << "AutotestPrivateSetPluginVMPolicyFunction " << params->image_url
<< ", " << params->image_hash;
<< ", " << params->image_hash << ", " << params->license_key;

Profile* profile = Profile::FromBrowserContext(browser_context());
plugin_vm::SetFakePluginVmPolicy(profile, params->image_url,
params->image_hash);
params->image_hash, params->license_key);

return RespondNow(NoArguments());
}
Expand Down
10 changes: 10 additions & 0 deletions chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1720,6 +1720,16 @@ void DecodeGenericPolicies(const em::ChromeDeviceSettingsProto& policy,
}
}

if (policy.has_plugin_vm_license_key()) {
const em::PluginVmLicenseKeyProto& container(
policy.plugin_vm_license_key());
if (container.has_plugin_vm_license_key()) {
policies->Set(key::kPluginVmLicenseKey, POLICY_LEVEL_MANDATORY,
POLICY_SCOPE_MACHINE, POLICY_SOURCE_CLOUD,
base::Value(container.plugin_vm_license_key()), nullptr);
}
}

if (policy.has_device_wilco_dtc_allowed() &&
policy.device_wilco_dtc_allowed().has_device_wilco_dtc_allowed()) {
VLOG(2) << "Set Wilco DTC allowed to "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ class PluginVmInstallerViewBrowserTest : public DialogBrowserTest {
auto* installer = plugin_vm::PluginVmInstallerFactory::GetForProfile(
browser()->profile());
installer->SetFreeDiskSpaceForTesting(installer->RequiredFreeDiskSpace());
installer->SkipLicenseCheckForTesting();
}

void SetPluginVmImagePref(std::string url, std::string hash) {
Expand Down Expand Up @@ -166,6 +165,8 @@ class PluginVmInstallerViewBrowserTest : public DialogBrowserTest {
// Device policies.
scoped_testing_cros_settings_.device_settings()->Set(
chromeos::kPluginVmAllowed, base::Value(true));
scoped_testing_cros_settings_.device_settings()->Set(
chromeos::kPluginVmLicenseKey, base::Value("LICENSE_KEY"));
}

void SetUserWithAffiliation() {
Expand Down
Loading

0 comments on commit 99a7837

Please sign in to comment.