Skip to content

Commit

Permalink
[M118] Ownership: Child users should become device owners
Browse files Browse the repository at this point in the history 
Make child users able to take device ownership.
Also refactor the code and add a switch-case, so all future user
types have to be added to the new method (otherwise it won't
compile).

(cherry picked from commit 1f7d660)

Bug: b:244407123
Test: Existing automated tests
Change-Id: I1e0d532cd777a69c5212af20d5bea84ee4f8eac8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4892006
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Michael Ershov <miersh@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1202036}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4905129
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/5993@{#965}
Cr-Branched-From: 5113507-refs/heads/main@{#1192594}
  • Loading branch information
Michael Ershov authored and Chromium LUCI CQ committed Sep 29, 2023
1 parent 404b45e commit 0562668
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 15 deletions.
20 changes: 19 additions & 1 deletion chrome/browser/ash/ownership/owner_key_loader.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,24 @@ inline bool AreKeysPresent(
return IsKeyPresent(public_key) && IsKeyPresent(private_key);
}

bool UserCanBecomeOwner(const user_manager::User* user) {
if (!user) {
return false;
}
switch (user->GetType()) {
case user_manager::USER_TYPE_REGULAR:
case user_manager::USER_TYPE_CHILD:
return true;
case user_manager::USER_TYPE_GUEST:
case user_manager::USER_TYPE_PUBLIC_ACCOUNT:
case user_manager::USER_TYPE_KIOSK_APP:
case user_manager::USER_TYPE_ARC_KIOSK_APP:
case user_manager::USER_TYPE_WEB_KIOSK_APP:
case user_manager::NUM_USER_TYPES:
return false;
}
}

} // namespace

OwnerKeyLoader::OwnerKeyLoader(
Expand Down Expand Up @@ -263,7 +281,7 @@ void OwnerKeyLoader::OnPrivateKeyLoaded(
void OwnerKeyLoader::MaybeGenerateNewKey() {
const user_manager::User* user =
ProfileHelper::Get()->GetUserByProfile(profile_);
if (!user || (user->GetType() != user_manager::USER_TYPE_REGULAR)) {
if (!UserCanBecomeOwner(user)) {
RecordOwnerKeyEvent(OwnerKeyEvent::kUserNotAnOwnerBasedOnUserType,
/*success=*/IsKeyPresent(public_key_));
return std::move(callback_).Run(public_key_, nullptr);
Expand Down
68 changes: 54 additions & 14 deletions chrome/browser/ash/ownership/owner_key_loader_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,11 @@ std::vector<uint8_t> ExtractBytes(
return bytes;
}

class OwnerKeyLoaderTest : public testing::Test {
class OwnerKeyLoaderTestBase : public testing::Test {
public:
explicit OwnerKeyLoaderTestBase(user_manager::UserType user_type)
: user_type_(user_type) {}

// testing::Test:
void SetUp() override {
auto fake_user_manager = std::make_unique<ash::FakeChromeUserManager>();
Expand All @@ -59,7 +62,7 @@ class OwnerKeyLoaderTest : public testing::Test {

user_manager_->AddUserWithAffiliationAndTypeAndProfile(
AccountId::FromUserEmail(kUserEmail), /*is_affiliated=*/false,
user_manager::USER_TYPE_REGULAR, profile_.get());
user_type_, profile_.get());

FakeNssService::InitializeForBrowserContext(profile_.get(),
/*enable_system_slot=*/false);
Expand Down Expand Up @@ -89,6 +92,7 @@ class OwnerKeyLoaderTest : public testing::Test {
std::unique_ptr<user_manager::ScopedUserManager> scoped_user_manager_;
raw_ptr<ash::FakeChromeUserManager, ExperimentalAsh> user_manager_ = nullptr;

const user_manager::UserType user_type_;
scoped_refptr<ownership::MockOwnerKeyUtil> owner_key_util_;
FakeSessionManagerClient session_manager_client_;
std::unique_ptr<TestingProfile> profile_;
Expand All @@ -98,8 +102,15 @@ class OwnerKeyLoaderTest : public testing::Test {
base::HistogramTester histogram_tester_;
};

// Test that the first user generates a new owner key.
TEST_F(OwnerKeyLoaderTest, FirstUserGeneratesOwnerKey) {
class RegularOwnerKeyLoaderTest : public OwnerKeyLoaderTestBase {
public:
RegularOwnerKeyLoaderTest()
: OwnerKeyLoaderTestBase(user_manager::USER_TYPE_REGULAR) {}
};

// Test that the first user generates a new owner key (when the user is a
// regular user).
TEST_F(RegularOwnerKeyLoaderTest, FirstUserGeneratesOwnerKey) {
// In real code DeviceSettingsService must call this for the first user.
device_settings_service_.MarkWillEstablishConsumerOwnership();
// Do not prepare any keys, so key_loader_ has to generate a new one.
Expand All @@ -123,7 +134,7 @@ TEST_F(OwnerKeyLoaderTest, FirstUserGeneratesOwnerKey) {
// indicate again that the consumer owners needs to be established.
// In such a case OwnerKeyLoader should read the identity of the owner from
// local state.
TEST_F(OwnerKeyLoaderTest, FirstUserGeneratesOwnerKeyAfterCrash) {
TEST_F(RegularOwnerKeyLoaderTest, FirstUserGeneratesOwnerKeyAfterCrash) {
// Populate local state data that OwnerKeyLoader should read.
user_manager_->RecordOwner(
AccountId::FromUserEmail(profile_->GetProfileUserName()));
Expand All @@ -145,7 +156,7 @@ TEST_F(OwnerKeyLoaderTest, FirstUserGeneratesOwnerKeyAfterCrash) {
}

// Test that the second user doesn't try to generate a new owner key.
TEST_F(OwnerKeyLoaderTest, SecondUserDoesNotTakeOwnership) {
TEST_F(RegularOwnerKeyLoaderTest, SecondUserDoesNotTakeOwnership) {
// In real code the first user would have created some device policies and
// saved the public owner key on disk. Emulate that.
auto signing_key = ConfigureExistingPolicies("owner@example.com");
Expand All @@ -167,7 +178,7 @@ TEST_F(OwnerKeyLoaderTest, SecondUserDoesNotTakeOwnership) {

// Test that an owner user gets recognized as the owner when it's mentioned in
// the existing device policies and owns the key.
TEST_F(OwnerKeyLoaderTest, OwnerUserLoadsExistingKey) {
TEST_F(RegularOwnerKeyLoaderTest, OwnerUserLoadsExistingKey) {
// Configure existing device policies and the owner key.
auto signing_key = ConfigureExistingPolicies(profile_->GetProfileUserName());
owner_key_util_->ImportPrivateKeyAndSetPublicKey(signing_key->Copy());
Expand All @@ -188,7 +199,7 @@ TEST_F(OwnerKeyLoaderTest, OwnerUserLoadsExistingKey) {

// Test that even without existing device policies the owner key gets loaded
// (that will help Chrome to recognize the current user as the owner).
TEST_F(OwnerKeyLoaderTest, OwnerUserLoadsExistingKeyWithoutPolicies) {
TEST_F(RegularOwnerKeyLoaderTest, OwnerUserLoadsExistingKeyWithoutPolicies) {
policy::DevicePolicyBuilder policy_builder;
auto signing_key = policy_builder.GetSigningKey();

Expand All @@ -209,7 +220,7 @@ TEST_F(OwnerKeyLoaderTest, OwnerUserLoadsExistingKeyWithoutPolicies) {

// Test that the second user is not falsely recognized as the owner even if
// policies fail to load and does not have the owner key.
TEST_F(OwnerKeyLoaderTest, SecondaryUserWithoutPolicies) {
TEST_F(RegularOwnerKeyLoaderTest, SecondaryUserWithoutPolicies) {
policy::DevicePolicyBuilder policy_builder;
auto signing_key = policy_builder.GetSigningKey();

Expand All @@ -231,7 +242,8 @@ TEST_F(OwnerKeyLoaderTest, SecondaryUserWithoutPolicies) {
// Test that an owner user still gets recognized as the owner when it's
// mentioned in the existing device policies, but the owner key was lost.
// The key must be re-generated in such a case.
TEST_F(OwnerKeyLoaderTest, OwnerUserRegeneratesMissingKeyBasedOnPolicies) {
TEST_F(RegularOwnerKeyLoaderTest,
OwnerUserRegeneratesMissingKeyBasedOnPolicies) {
auto signing_key = ConfigureExistingPolicies(profile_->GetProfileUserName());
// Configure that the public key is on disk, but the private key doesn't
// exist.
Expand All @@ -258,7 +270,8 @@ TEST_F(OwnerKeyLoaderTest, OwnerUserRegeneratesMissingKeyBasedOnPolicies) {
// Test that an owner user still gets recognized as the owner when it's
// mentioned in the local state, but the device policies and the owner key were
// lost. The key must be re-generated in such a case.
TEST_F(OwnerKeyLoaderTest, OwnerUserRegeneratesMissingKeyBasedOnLocalState) {
TEST_F(RegularOwnerKeyLoaderTest,
OwnerUserRegeneratesMissingKeyBasedOnLocalState) {
// Populate local state.
user_manager_->RecordOwner(
AccountId::FromUserEmail(profile_->GetProfileUserName()));
Expand Down Expand Up @@ -288,7 +301,7 @@ TEST_F(OwnerKeyLoaderTest, OwnerUserRegeneratesMissingKeyBasedOnLocalState) {

// Test that OwnerKeyLoader makes several attempts to generate the owner key
// pair.
TEST_F(OwnerKeyLoaderTest, KeyGenerationRetriedSuccessfully) {
TEST_F(RegularOwnerKeyLoaderTest, KeyGenerationRetriedSuccessfully) {
// Make key_loader_ generate the key for the first user.
device_settings_service_.MarkWillEstablishConsumerOwnership();
owner_key_util_->SimulateGenerateKeyFailure(/*fail_times=*/5);
Expand All @@ -310,7 +323,7 @@ TEST_F(OwnerKeyLoaderTest, KeyGenerationRetriedSuccessfully) {

// Test that OwnerKeyLoader gives up to generate the owner key pair after a
// certain amount of attempts.
TEST_F(OwnerKeyLoaderTest, KeyGenerationRetriedUnsuccessfully) {
TEST_F(RegularOwnerKeyLoaderTest, KeyGenerationRetriedUnsuccessfully) {
// Make key_loader_ generate the key for the first user.
device_settings_service_.MarkWillEstablishConsumerOwnership();
owner_key_util_->SimulateGenerateKeyFailure(/*fail_times=*/10);
Expand All @@ -330,7 +343,7 @@ TEST_F(OwnerKeyLoaderTest, KeyGenerationRetriedUnsuccessfully) {
// Test that enterprise devices don't attempt to load private key. The signing
// key of the device policy is owned by the backend server in the
// enterprise-enrolled case.
TEST_F(OwnerKeyLoaderTest, EnterpriseDevicesDontNeedPrivateKey) {
TEST_F(RegularOwnerKeyLoaderTest, EnterpriseDevicesDontNeedPrivateKey) {
// Create favorable conditions for the code to load private key. Claim in the
// device policies that the user is the owner (shouldn't happen on a real
// device) and prepare a private key in case OwnerKeyLoader tries to load it.
Expand All @@ -353,4 +366,31 @@ TEST_F(OwnerKeyLoaderTest, EnterpriseDevicesDontNeedPrivateKey) {
ElementsAre(Bucket(OwnerKeyUmaEvent::kManagedDeviceSuccess, 1)));
}

class ChildOwnerKeyLoaderTest : public OwnerKeyLoaderTestBase {
public:
ChildOwnerKeyLoaderTest()
: OwnerKeyLoaderTestBase(user_manager::USER_TYPE_CHILD) {}
};

// Test that the first user generates a new owner key (when the user is a
// child user).
TEST_F(ChildOwnerKeyLoaderTest, FirstUserGeneratesOwnerKey) {
// In real code DeviceSettingsService must call this for the first user.
device_settings_service_.MarkWillEstablishConsumerOwnership();
// Do not prepare any keys, so key_loader_ has to generate a new one.

key_loader_->Run();

ASSERT_TRUE(result_observer_.Get<PublicKeyRefPtr>());
EXPECT_TRUE(!result_observer_.Get<PublicKeyRefPtr>()->is_empty());
ASSERT_TRUE(result_observer_.Get<PrivateKeyRefPtr>());
EXPECT_TRUE(result_observer_.Get<PrivateKeyRefPtr>()->key());

EXPECT_THAT(
histogram_tester_.GetAllSamples(kOwnerKeyHistogramName),
ElementsAre(
Bucket(OwnerKeyUmaEvent::kEstablishingConsumerOwnershipSuccess, 1),
Bucket(OwnerKeyUmaEvent::kOwnerKeyGeneratedSuccess, 1)));
}

} // namespace ash

0 comments on commit 0562668

Please sign in to comment.