From 01b7062b34fbe8be7895be02835f29664dd6170c Mon Sep 17 00:00:00 2001 From: Nikhil Kumar Date: Fri, 2 Aug 2024 14:41:10 +0100 Subject: [PATCH] Restrict admin status change when DISALLOW_GRANT_ADMIN is present This change addresses a security gap where users with the "DISALLOW_GRANT_ADMIN" restriction could still grant admin privileges to others, potentially undermining supervised user models. We've extended the existing logic to ensure that restricted users cannot grant or receive admin privileges when the DISALLOW_GRANT_ADMIN restriction is in place. This prevents scenarios where supervised users could create new admins to bypass restrictions and factory reset the device. In addition to the core functionality improvement, significant code refactoring has been done to untangle complex multiple if conditions. The logic for determining when admin status changes are allowed is now clearer and more readable, taking into account both the "current user" (initiating the change) and the "target user" (whose privileges are being modified). Bug: 357056776 Test: atest UserDetailsSettingsTest and manually verified the user having DISALLOW_GRANT_ADMIN restriction is not allowed to change admin status of any other user. Flag: android.multiuser.unicorn_mode_refactoring_for_hsum_read_only Change-Id: If02c9355ee7ce285b5b7bcddae630d716d5bf333 --- .../settings/users/UserDetailsSettings.java | 44 +++++++++++++++++-- .../users/UserDetailsSettingsTest.java | 13 ++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/com/android/settings/users/UserDetailsSettings.java b/src/com/android/settings/users/UserDetailsSettings.java index 588f01aaa79..66c278ed733 100644 --- a/src/com/android/settings/users/UserDetailsSettings.java +++ b/src/com/android/settings/users/UserDetailsSettings.java @@ -370,11 +370,18 @@ public class UserDetailsSettings extends SettingsPreferenceFragment } mSwitchUserPref.setOnPreferenceClickListener(this); } - if (mUserInfo.isMain() || mUserInfo.isGuest() || !UserManager.isMultipleAdminEnabled() - || mUserManager.hasUserRestrictionForUser(UserManager.DISALLOW_GRANT_ADMIN, - mUserInfo.getUserHandle()) || !mUserManager.isAdminUser()) { - removePreference(KEY_GRANT_ADMIN); + if (android.multiuser.Flags.unicornModeRefactoringForHsumReadOnly()) { + if (isChangingAdminStatusRestricted()) { + removePreference(KEY_GRANT_ADMIN); + } + } else { + if (mUserInfo.isMain() || mUserInfo.isGuest() || !UserManager.isMultipleAdminEnabled() + || mUserManager.hasUserRestrictionForUser(UserManager.DISALLOW_GRANT_ADMIN, + mUserInfo.getUserHandle()) || !mUserManager.isAdminUser()) { + removePreference(KEY_GRANT_ADMIN); + } } + if (!mUserManager.isAdminUser()) { // non admin users can't remove users and allow calls removePreference(KEY_ENABLE_TELEPHONY); removePreference(KEY_REMOVE_USER); @@ -552,4 +559,33 @@ public class UserDetailsSettings extends SettingsPreferenceFragment // return true so there will be no setup prompt dialog shown to the user anymore. return isSecondaryUser(mUserInfo) && !mUserInfo.isInitialized(); } + + /** + * Determines if changing admin status is restricted. + * + *

Admin status change is restricted under the following conditions of current & target user. + * + *

+ * + * @return true if changing admin status is restricted, false otherwise + */ + private boolean isChangingAdminStatusRestricted() { + boolean currentUserRestricted = !mUserManager.isAdminUser() + || !UserManager.isMultipleAdminEnabled() + || mUserManager.hasUserRestriction(UserManager.DISALLOW_GRANT_ADMIN); + + boolean targetUserRestricted = mUserInfo.isMain() + || mUserInfo.isGuest() + || mUserManager.hasUserRestrictionForUser(UserManager.DISALLOW_GRANT_ADMIN, + mUserInfo.getUserHandle()); + + return currentUserRestricted || targetUserRestricted; + } } diff --git a/tests/robotests/src/com/android/settings/users/UserDetailsSettingsTest.java b/tests/robotests/src/com/android/settings/users/UserDetailsSettingsTest.java index e035274a5de..482aa5d5b93 100644 --- a/tests/robotests/src/com/android/settings/users/UserDetailsSettingsTest.java +++ b/tests/robotests/src/com/android/settings/users/UserDetailsSettingsTest.java @@ -729,12 +729,25 @@ public class UserDetailsSettingsTest { public void initialize_restrictUserSelected_shouldNotShowGrantAdminPref_MultipleAdminEnabled() { setupSelectedUser(); ShadowUserManager.setIsMultipleAdminEnabled(true); + // target user has DISALLOW_GRANT_ADMIN restriction mUserManager.setUserRestriction(mUserInfo.getUserHandle(), UserManager.DISALLOW_GRANT_ADMIN, true); mFragment.initialize(mActivity, mArguments); verify(mFragment).removePreference(KEY_GRANT_ADMIN); } + @Test + @RequiresFlagsEnabled(Flags.FLAG_UNICORN_MODE_REFACTORING_FOR_HSUM_READ_ONLY) + public void initialize_currentUserRestrict_shouldNotShowGrantAdminPref_MultipleAdminEnabled() { + setupSelectedUser(); + ShadowUserManager.setIsMultipleAdminEnabled(true); + // current user has DISALLOW_GRANT_ADMIN restriction + mUserManager.setUserRestriction(mContext.getUser(), + UserManager.DISALLOW_GRANT_ADMIN, true); + mFragment.initialize(mActivity, mArguments); + verify(mFragment).removePreference(KEY_GRANT_ADMIN); + } + @Test public void initialize_mainUserSelected_shouldShowGrantAdminPref_MultipleAdminEnabled() { setupSelectedMainUser();