From f2e0460ca4a4ccff1c1fbd38940e6c330844fe7c Mon Sep 17 00:00:00 2001 From: Felipe Leme Date: Tue, 9 Aug 2022 12:41:25 -0700 Subject: [PATCH] Added more checks on startUserInBackgroundOnSecondaryDisplay(): - Profile can only start on same display as user. - Only one (full) user can be started in a display. - Cannot be called on DEFAULT_DISPLAY Test: atest CtsMultiUserTestCases:android.multiuser.cts.UserManagerTest Bug: 240736142 Bug: 239824814 Change-Id: I8c5540ec34aab6a84e5ab29f89ad8baf658df1d0 --- .../com/android/server/am/UserController.java | 18 +++++++-- .../android/server/pm/UserManagerService.java | 37 +++++++++++++++++-- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/services/core/java/com/android/server/am/UserController.java b/services/core/java/com/android/server/am/UserController.java index 1edfabed7ad1d..369aa25849290 100644 --- a/services/core/java/com/android/server/am/UserController.java +++ b/services/core/java/com/android/server/am/UserController.java @@ -1468,7 +1468,16 @@ class UserController implements Handler.Callback { checkCallingHasOneOfThosePermissions("startUserOnSecondaryDisplay", MANAGE_USERS, CREATE_USERS); - return startUserNoChecks(userId, displayId, /* foreground= */ false, unlockListener); + // DEFAULT_DISPLAY is used for "regular" start user operations + Preconditions.checkArgument(displayId != Display.DEFAULT_DISPLAY, + "Cannot use DEFAULT_DISPLAY"); + + try { + return startUserNoChecks(userId, displayId, /* foreground= */ false, unlockListener); + } catch (RuntimeException e) { + Slogf.w(TAG, "startUserOnSecondaryDisplay(%d, %d) failed: %s", userId, displayId, e); + return false; + } } private boolean startUserNoChecks(@UserIdInt int userId, int displayId, boolean foreground, @@ -1492,13 +1501,15 @@ class UserController implements Handler.Callback { foreground ? " in foreground" : ""); } + // TODO(b/239982558): move logic below to a different class (like DisplayAssignmentManager) if (displayId != Display.DEFAULT_DISPLAY) { + // This is called by startUserOnSecondaryDisplay() if (!UserManager.isUsersOnSecondaryDisplaysEnabled()) { - // TODO(b/239824814): add CTS test and/or unit test + // TODO(b/239824814): add CTS test and/or unit test for all exceptional cases throw new UnsupportedOperationException("Not supported by device"); } - // TODO(b/239982558): add unit test for the exceptional cases below + // TODO(b/239982558): call DisplayManagerInternal to check if display is valid instead Preconditions.checkArgument(displayId > 0, "Invalid display id (%d)", displayId); Preconditions.checkArgument(userId != UserHandle.USER_SYSTEM, "Cannot start system user" + " on secondary display (%d)", displayId); @@ -1508,7 +1519,6 @@ class UserController implements Handler.Callback { // TODO(b/239982558): for now we're just updating the user's visibility, but most likely // we'll need to remove this call and handle that as part of the user state workflow // instead. - // TODO(b/239982558): check if display is valid mInjector.getUserManagerInternal().assignUserToDisplay(userId, displayId); } diff --git a/services/core/java/com/android/server/pm/UserManagerService.java b/services/core/java/com/android/server/pm/UserManagerService.java index f08bea31c20a0..c0176501b0260 100644 --- a/services/core/java/com/android/server/pm/UserManagerService.java +++ b/services/core/java/com/android/server/pm/UserManagerService.java @@ -1587,6 +1587,10 @@ public class UserManagerService extends IUserManager.Stub { public boolean isProfile(@UserIdInt int userId) { checkQueryOrInteractPermissionIfCallerInOtherProfileGroup(userId, "isProfile"); + return isProfileUnchecked(userId); + } + + private boolean isProfileUnchecked(@UserIdInt int userId) { synchronized (mUsersLock) { UserInfo userInfo = getUserInfoLU(userId); return userInfo != null && userInfo.isProfile(); @@ -6827,9 +6831,6 @@ public class UserManagerService extends IUserManager.Stub { return; } - // TODO(b/239982558) check for invalid cases like: - // - userId already assigned to another display - // - displayId already assigned to another user synchronized (mUsersLock) { if (mUsersOnSecondaryDisplays == null) { if (DBG) { @@ -6841,6 +6842,36 @@ public class UserManagerService extends IUserManager.Stub { Slogf.d(LOG_TAG, "Adding %d->%d to mUsersOnSecondaryDisplays", userId, displayId); } + + if (isProfileUnchecked(userId)) { + // Profile can only start in the same display as parent + int parentUserId = getProfileParentId(userId); + int parentDisplayId = mUsersOnSecondaryDisplays.get(parentUserId); + if (displayId != parentDisplayId) { + throw new IllegalStateException("Cannot assign profile " + userId + " to " + + "display " + displayId + " as its parent (user " + parentUserId + + ") is assigned to display " + parentDisplayId); + } + } else { + // Check if display is available + for (int i = 0; i < mUsersOnSecondaryDisplays.size(); i++) { + // Make sure display is not used by other users... + // TODO(b/240736142); currently, if a user was started in a display, it + // would need to be stopped first, so "switching" a user on secondary + // diplay requires 2 non-atomic operations (stop and start). Once this logic + // is refactored, it should be atomic. + if (mUsersOnSecondaryDisplays.valueAt(i) == displayId) { + throw new IllegalStateException("Cannot assign " + userId + " to " + + "display " + displayId + " as it's already assigned to " + + "user " + mUsersOnSecondaryDisplays.keyAt(i)); + } + // TODO(b/239982558) also check that user is not already assigned to other + // display (including 0). That would be harder to tested under CTS though + // (for example, would need to add a new AM method to start user in bg on + // main display), so it's better to test on unit tests + } + } + mUsersOnSecondaryDisplays.put(userId, displayId); } }