From 6ad393d8960d03ba560e7cca09ceba8a0dd6860a Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Tue, 5 Jan 2021 17:53:32 -0800 Subject: [PATCH] Added evenWhenDisallowed param to removeUserOrSetEphemeral(). UserManager.removeUserOrSetEphemeral() was added primarily to be used by CarDevicePolicyManager, in which case it should ignore the no_remove_user restriction. But as it's also used in other places, it needs a new parameter to define this behavior. Test: atest FrameworksServicesTests:com.android.server.pm.UserManagerTest#testRemoveUserOrSetEphemeral_evenWhenRestricted Test: atest FrameworksServicesTests:com.android.server.pm.UserManagerTest # to make sure it didn't break anything Test: atest android.car.apitest.CarDevicePolicyManagerTest#testRemoveUser_whenDisallowed # on automotive Bug: 170887769 Change-Id: If797ace64c0fa0262116f649212bbcb1d61e2046 --- core/java/android/os/IUserManager.aidl | 2 +- core/java/android/os/UserManager.java | 8 +++- .../server/pm/PackageManagerShellCommand.java | 2 +- .../android/server/pm/UserManagerService.java | 18 ++++---- .../android/server/pm/UserManagerTest.java | 43 ++++++++++++++----- 5 files changed, 51 insertions(+), 22 deletions(-) diff --git a/core/java/android/os/IUserManager.aidl b/core/java/android/os/IUserManager.aidl index 6fe57774f6f33..b39c182fcab6a 100644 --- a/core/java/android/os/IUserManager.aidl +++ b/core/java/android/os/IUserManager.aidl @@ -86,7 +86,7 @@ interface IUserManager { Bundle getApplicationRestrictionsForUser(in String packageName, int userId); void setDefaultGuestRestrictions(in Bundle restrictions); Bundle getDefaultGuestRestrictions(); - int removeUserOrSetEphemeral(int userId); + int removeUserOrSetEphemeral(int userId, boolean evenWhenDisallowed); boolean markGuestForDeletion(int userId); UserInfo findCurrentGuestUser(); boolean isQuietModeEnabled(int userId); diff --git a/core/java/android/os/UserManager.java b/core/java/android/os/UserManager.java index e7d19c5eb18a9..0dff2bf2bd339 100644 --- a/core/java/android/os/UserManager.java +++ b/core/java/android/os/UserManager.java @@ -4036,14 +4036,18 @@ public class UserManager { * the current user, then set the user as ephemeral so that it will be removed when it is * stopped. * + * @param evenWhenDisallowed when {@code true}, user is removed even if the caller user has the + * {@link #DISALLOW_REMOVE_USER} or {@link #DISALLOW_REMOVE_MANAGED_PROFILE} restriction + * * @return the {@link RemoveResult} code * @hide */ @RequiresPermission(anyOf = {Manifest.permission.MANAGE_USERS, Manifest.permission.CREATE_USERS}) - public @RemoveResult int removeUserOrSetEphemeral(@UserIdInt int userId) { + public @RemoveResult int removeUserOrSetEphemeral(@UserIdInt int userId, + boolean evenWhenDisallowed) { try { - return mService.removeUserOrSetEphemeral(userId); + return mService.removeUserOrSetEphemeral(userId, evenWhenDisallowed); } catch (RemoteException re) { throw re.rethrowFromSystemServer(); } diff --git a/services/core/java/com/android/server/pm/PackageManagerShellCommand.java b/services/core/java/com/android/server/pm/PackageManagerShellCommand.java index 9eae1174fb739..446342a8f5125 100644 --- a/services/core/java/com/android/server/pm/PackageManagerShellCommand.java +++ b/services/core/java/com/android/server/pm/PackageManagerShellCommand.java @@ -2732,7 +2732,7 @@ class PackageManagerShellCommand extends ShellCommand { private int removeUserOrSetEphemeral(IUserManager um, @UserIdInt int userId) throws RemoteException { Slog.i(TAG, "Removing " + userId + " or set as ephemeral if in use."); - int result = um.removeUserOrSetEphemeral(userId); + int result = um.removeUserOrSetEphemeral(userId, /* evenWhenDisallowed= */ false); switch (result) { case UserManager.REMOVE_RESULT_REMOVED: getOutPrintWriter().printf("Success: user %d removed\n", userId); diff --git a/services/core/java/com/android/server/pm/UserManagerService.java b/services/core/java/com/android/server/pm/UserManagerService.java index 9347ce1c6c0fc..3f8db16b3db4b 100644 --- a/services/core/java/com/android/server/pm/UserManagerService.java +++ b/services/core/java/com/android/server/pm/UserManagerService.java @@ -3842,7 +3842,6 @@ public class UserManagerService extends IUserManager.Stub { */ @Override public boolean removeUser(@UserIdInt int userId) { - Slog.i(LOG_TAG, "removeUser u" + userId, new Exception()); checkManageOrCreateUsersPermission("Only the system can remove users"); final String restriction = getUserRemovalRestriction(userId); @@ -3967,13 +3966,16 @@ public class UserManagerService extends IUserManager.Stub { } @Override - public @UserManager.RemoveResult int removeUserOrSetEphemeral(@UserIdInt int userId) { - Slog.i(LOG_TAG, "removeUserOrSetEphemeral u" + userId); + public @UserManager.RemoveResult int removeUserOrSetEphemeral(@UserIdInt int userId, + boolean evenWhenDisallowed) { checkManageOrCreateUsersPermission("Only the system can remove users"); - final String restriction = getUserRemovalRestriction(userId); - if (getUserRestrictions(UserHandle.getCallingUserId()).getBoolean(restriction, false)) { - Slog.w(LOG_TAG, "Cannot remove user. " + restriction + " is enabled."); - return UserManager.REMOVE_RESULT_ERROR; + + if (!evenWhenDisallowed) { + final String restriction = getUserRemovalRestriction(userId); + if (getUserRestrictions(UserHandle.getCallingUserId()).getBoolean(restriction, false)) { + Slog.w(LOG_TAG, "Cannot remove user. " + restriction + " is enabled."); + return UserManager.REMOVE_RESULT_ERROR; + } } if (userId == UserHandle.USER_SYSTEM) { Slog.e(LOG_TAG, "System user cannot be removed."); @@ -4002,7 +4004,7 @@ public class UserManagerService extends IUserManager.Stub { final int currentUser = ActivityManager.getCurrentUser(); if (currentUser != userId) { // Attempt to remove the user. This will fail if the user is the current user - if (removeUser(userId)) { + if (removeUserUnchecked(userId)) { return UserManager.REMOVE_RESULT_REMOVED; } } diff --git a/services/tests/servicestests/src/com/android/server/pm/UserManagerTest.java b/services/tests/servicestests/src/com/android/server/pm/UserManagerTest.java index b190339b129b9..395b643e37770 100644 --- a/services/tests/servicestests/src/com/android/server/pm/UserManagerTest.java +++ b/services/tests/servicestests/src/com/android/server/pm/UserManagerTest.java @@ -219,8 +219,8 @@ public final class UserManagerTest { mUserManager.setUserRestriction(UserManager.DISALLOW_REMOVE_USER, /* value= */ true, asHandle(currentUser)); try { - assertThat(mUserManager.removeUserOrSetEphemeral(user1.id)).isEqualTo( - UserManager.REMOVE_RESULT_ERROR); + assertThat(mUserManager.removeUserOrSetEphemeral(user1.id, + /* evenWhenDisallowed= */ false)).isEqualTo(UserManager.REMOVE_RESULT_ERROR); } finally { mUserManager.setUserRestriction(UserManager.DISALLOW_REMOVE_USER, /* value= */ false, asHandle(currentUser)); @@ -230,11 +230,34 @@ public final class UserManagerTest { assertThat(getUser(user1.id).isEphemeral()).isFalse(); } + @MediumTest + @Test + public void testRemoveUserOrSetEphemeral_evenWhenRestricted() throws Exception { + final int currentUser = ActivityManager.getCurrentUser(); + final UserInfo user1 = createUser("User 1", /* flags= */ 0); + mUserManager.setUserRestriction(UserManager.DISALLOW_REMOVE_USER, /* value= */ true, + asHandle(currentUser)); + try { + synchronized (mUserRemoveLock) { + assertThat(mUserManager.removeUserOrSetEphemeral(user1.id, + /* evenWhenDisallowed= */ true)) + .isEqualTo(UserManager.REMOVE_RESULT_REMOVED); + waitForUserRemovalLocked(user1.id); + } + + } finally { + mUserManager.setUserRestriction(UserManager.DISALLOW_REMOVE_USER, /* value= */ false, + asHandle(currentUser)); + } + + assertThat(hasUser(user1.id)).isFalse(); + } + @MediumTest @Test public void testRemoveUserOrSetEphemeral_systemUserReturnsError() throws Exception { - assertThat(mUserManager.removeUserOrSetEphemeral(UserHandle.USER_SYSTEM)).isEqualTo( - UserManager.REMOVE_RESULT_ERROR); + assertThat(mUserManager.removeUserOrSetEphemeral(UserHandle.USER_SYSTEM, + /* evenWhenDisallowed= */ false)).isEqualTo(UserManager.REMOVE_RESULT_ERROR); assertThat(hasUser(UserHandle.USER_SYSTEM)).isTrue(); } @@ -243,8 +266,8 @@ public final class UserManagerTest { @Test public void testRemoveUserOrSetEphemeral_invalidUserReturnsError() throws Exception { assertThat(hasUser(Integer.MAX_VALUE)).isFalse(); - assertThat(mUserManager.removeUserOrSetEphemeral(Integer.MAX_VALUE)).isEqualTo( - UserManager.REMOVE_RESULT_ERROR); + assertThat(mUserManager.removeUserOrSetEphemeral(Integer.MAX_VALUE, + /* evenWhenDisallowed= */ false)).isEqualTo(UserManager.REMOVE_RESULT_ERROR); } @MediumTest @@ -255,8 +278,8 @@ public final class UserManagerTest { // Switch to the user just created. switchUser(user1.id, null, /* ignoreHandle= */ true); - assertThat(mUserManager.removeUserOrSetEphemeral(user1.id)).isEqualTo( - UserManager.REMOVE_RESULT_SET_EPHEMERAL); + assertThat(mUserManager.removeUserOrSetEphemeral(user1.id, /* evenWhenDisallowed= */ false)) + .isEqualTo(UserManager.REMOVE_RESULT_SET_EPHEMERAL); assertThat(hasUser(user1.id)).isTrue(); assertThat(getUser(user1.id).isEphemeral()).isTrue(); @@ -276,8 +299,8 @@ public final class UserManagerTest { public void testRemoveUserOrSetEphemeral_nonCurrentUserRemoved() throws Exception { final UserInfo user1 = createUser("User 1", /* flags= */ 0); synchronized (mUserRemoveLock) { - assertThat(mUserManager.removeUserOrSetEphemeral(user1.id)).isEqualTo( - UserManager.REMOVE_RESULT_REMOVED); + assertThat(mUserManager.removeUserOrSetEphemeral(user1.id, + /* evenWhenDisallowed= */ false)).isEqualTo(UserManager.REMOVE_RESULT_REMOVED); waitForUserRemovalLocked(user1.id); }