Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 85da667c authored by Yasin Kilicdere's avatar Yasin Kilicdere
Browse files

Prevent removing the current user or the target user of user switch. 

UserController.switchUser and UserController.startUserInternal both
share a validation check logic but the validation code is duplicated.

When switching a user, switchUser runs the validation checks, and if
they pass, it sets mTargetUserId and starts a call chain that
eventually calls startUserInternal.

Currently the validation checks are in sync, but if some other
validation would be added to startUserInternal but not in switchUser,
then even though switchUser returns true, it can fail asynchronously
while starting the user in startUserInternal. So it's not safe to
remove the current user after starting a switch.

Also currently it is possible to remove the target user during an
ongoing switch, which is also wrong.

This CL prevents removing the current user, or both current and target
users in case there is an ongoing user switch.
This CL also changes exiting tests to work with these new rules and
also adds new tests for coverage.

Bug: 264667155
Test: atest FrameworksServicesTests:com.android.server.pm.UserLifecycleStressTest#switchToExistingGuestAndStartOverStressTest
Test: atest FrameworksMockingServicesTests:com.android.server.pm.UserManagerServiceTest#testGetCurrentAndTargetUserIds
Test: atest FrameworksServicesTests:com.android.server.pm.UserManagerTest#testRemoveUserShouldNotRemoveCurrentUser
Test: atest FrameworksServicesTests:com.android.server.pm.UserManagerTest#testRemoveUserShouldNotRemoveCurrentUser_DuringUserSwitch
Test: atest FrameworksServicesTests:com.android.server.pm.UserManagerTest#testRemoveUserShouldNotRemoveTargetUser_DuringUserSwitch
Test: atest FrameworksServicesTests:com.android.server.pm.UserManagerTest#testRemoveUserWhenPossible_currentUserSetEphemeral_duringUserSwitch
Test: atest FrameworksServicesTests:com.android.server.pm.UserManagerTest#testRemoveUserWhenPossible_targetUserSetEphemeral_duringUserSwitch
Change-Id: I8db44b6bbc3c1c57b9bebe0d27be068ea2b654ed
parent c1de6541

core/java/android/app/ActivityManagerInternal.java

+6 −0
Original line number Diff line number Diff line
@@ -311,6 +311,12 @@ public abstract class ActivityManagerInternal { 
    @PermissionMethod

    public abstract void enforceCallingPermission(@PermissionName String permission, String func);



    /**

     * Returns the current and target user ids as a {@link Pair}. Target user id will be

     * {@link android.os.UserHandle#USER_NULL} if there is not an ongoing user switch.

     */

    public abstract Pair<Integer, Integer> getCurrentAndTargetUserIds();



    /** Returns the current user id. */

    public abstract int getCurrentUserId();

services/core/java/com/android/server/am/ActivityManagerService.java

+5 −0
Original line number Diff line number Diff line
@@ -17365,6 +17365,11 @@ public class ActivityManagerService extends IActivityManager.Stub 
            ActivityManagerService.this.enforceCallingPermission(permission, func);

        }













        @Override













        public Pair<Integer, Integer> getCurrentAndTargetUserIds() {













            return mUserController.getCurrentAndTargetUserIds();













        }



























        @Override















        public int getCurrentUserId() {















            return mUserController.getCurrentUserId();

























services/core/java/com/android/server/am/UserController.java



+6
−0




























Original line number
Diff line number
Diff line








@@ -2742,6 +2742,12 @@ class UserController implements Handler.Callback {













        return mTargetUserId != UserHandle.USER_NULL ? mTargetUserId : mCurrentUserId;















    }





























    Pair<Integer, Integer> getCurrentAndTargetUserIds() {













        synchronized (mLock) {













            return new Pair<>(mCurrentUserId, mTargetUserId);













        }













    }





























    @GuardedBy("mLock")















    private int getCurrentUserIdLU() {















        return mCurrentUserId;

































services/core/java/com/android/server/pm/UserManagerService.java



+40
−8




























Original line number
Diff line number
Diff line








@@ -101,6 +101,7 @@ import android.util.ArraySet;













import android.util.AtomicFile;















import android.util.IndentingPrintWriter;















import android.util.IntArray;













import android.util.Pair;















import android.util.Slog;















import android.util.SparseArray;















import android.util.SparseBooleanArray;










@@ -1836,6 +1837,27 @@ public class UserManagerService extends IUserManager.Stub {













        return mUserVisibilityMediator.isUserVisible(userId);















    }





























    /**













     * Gets the current and target user ids as a {@link Pair}, calling













     * {@link ActivityManagerInternal} directly (and without performing any permission check).













     *













     * @return ids of current foreground user and the target user. Target user will be













     * {@link UserHandle#USER_NULL} if there is not an ongoing user switch. And if













     * {@link ActivityManagerInternal} is not available yet, they will both be













     * {@link UserHandle#USER_NULL}.













     */













    @VisibleForTesting













    @NonNull













    Pair<Integer, Integer> getCurrentAndTargetUserIds() {













        ActivityManagerInternal activityManagerInternal = getActivityManagerInternal();













        if (activityManagerInternal == null) {













            Slog.w(LOG_TAG, "getCurrentAndTargetUserId() called too early, "













                    + "ActivityManagerInternal is not set yet");













            return new Pair<>(UserHandle.USER_NULL, UserHandle.USER_NULL);













        }













        return activityManagerInternal.getCurrentAndTargetUserIds();













    }





























    /**















     * Gets the current user id, calling {@link ActivityManagerInternal} directly (and without















     * performing any permission check).










@@ -5428,11 +5450,15 @@ public class UserManagerService extends IUserManager.Stub {













        final long ident = Binder.clearCallingIdentity();















        try {















            final UserData userData;













            int currentUser = getCurrentUserId();













            if (currentUser == userId) {













            Pair<Integer, Integer> currentAndTargetUserIds = getCurrentAndTargetUserIds();













            if (userId == currentAndTargetUserIds.first) {















                Slog.w(LOG_TAG, "Current user cannot be removed.");















                return false;















            }













            if (userId == currentAndTargetUserIds.second) {













                Slog.w(LOG_TAG, "Target user of an ongoing user switch cannot be removed.");













                return false;













            }















            synchronized (mPackagesLock) {















                synchronized (mUsersLock) {















                    userData = mUsers.get(userId);










@@ -5569,9 +5595,10 @@ public class UserManagerService extends IUserManager.Stub {













                    }















                }





























                // Attempt to immediately remove a non-current user













                final int currentUser = getCurrentUserId();













                if (currentUser != userId) {













                // Attempt to immediately remove a non-current and non-target user













                Pair<Integer, Integer> currentAndTargetUserIds = getCurrentAndTargetUserIds();













                if (userId != currentAndTargetUserIds.first













                        && userId != currentAndTargetUserIds.second) {















                    // Attempt to remove the user. This will fail if the user is the current user















                    if (removeUserWithProfilesUnchecked(userId)) {















                        return UserManager.REMOVE_RESULT_REMOVED;









@@ -5580,9 +5607,14 @@ public class UserManagerService extends IUserManager.Stub {













                // If the user was not immediately removed, make sure it is marked as ephemeral.















                // Don't mark as disabled since, per UserInfo.FLAG_DISABLED documentation, an















                // ephemeral user should only be marked as disabled when its removal is in progress.













                Slog.i(LOG_TAG, "Unable to immediately remove user " + userId + " (current user is "













                        + currentUser + "). User is set as ephemeral and will be removed on user "













                        + "switch or reboot.");













                Slog.i(LOG_TAG, TextUtils.formatSimple("Unable to immediately remove user %d "













                                + "(%s is %d). User is set as ephemeral and will be removed on "













                                + "user switch or reboot.",













                        userId,













                        userId == currentAndTargetUserIds.first













                                ? "current user"













                                : "target user of an ongoing user switch",













                        userId));















                userData.info.flags |= UserInfo.FLAG_EPHEMERAL;















                writeUserLP(userData);

















































services/tests/mockingservicestests/src/com/android/server/pm/UserManagerServiceTest.java



+18
−0




























Original line number
Diff line number
Diff line








@@ -34,6 +34,7 @@ import android.os.UserManager;













import android.os.storage.StorageManager;















import android.provider.Settings;















import android.util.Log;













import android.util.Pair;















import android.util.SparseArray;































import androidx.test.annotation.UiThreadTest;










@@ -153,6 +154,15 @@ public final class UserManagerServiceTest extends ExtendedMockitoTestCase {













                .isEqualTo(UserHandle.USER_NULL);















    }





























    @Test













    public void testGetCurrentAndTargetUserIds() {













        mockCurrentAndTargetUser(USER_ID, OTHER_USER_ID);



























        assertWithMessage("getCurrentAndTargetUserIds()")













                .that(mUms.getCurrentAndTargetUserIds())













                .isEqualTo(new Pair<>(USER_ID, OTHER_USER_ID));













    }





























    @Test















    public void testGetCurrentUserId() {















        mockCurrentUser(USER_ID);










@@ -329,6 +339,14 @@ public final class UserManagerServiceTest extends ExtendedMockitoTestCase {













        when(mActivityManagerInternal.getCurrentUserId()).thenReturn(userId);















    }





























    private void mockCurrentAndTargetUser(@UserIdInt int currentUserId,













            @UserIdInt int targetUserId) {













        mockGetLocalService(ActivityManagerInternal.class, mActivityManagerInternal);



























        when(mActivityManagerInternal.getCurrentAndTargetUserIds())













                .thenReturn(new Pair<>(currentUserId, targetUserId));













    }





























    private <T> void mockGetLocalService(Class<T> serviceClass, T service) {















        doReturn(service).when(() -> LocalServices.getService(serviceClass));















    }