From 1a5ee776ee51ae6fba30c8f3b33e26eb7f9dedc6 Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Fri, 12 Feb 2016 15:34:57 -0800 Subject: [PATCH] Don't allow deactivating DAs when the user is not unlocked Bug 27149570 Change-Id: I772d9cbd6edc822c8f7b1988905b702e05e674cd --- .../android/internal/util/Preconditions.java | 18 ++++- .../DevicePolicyManagerService.java | 10 +++ .../devicepolicy/DevicePolicyManagerTest.java | 69 ++++++++++++++++++- 3 files changed, 92 insertions(+), 5 deletions(-) diff --git a/core/java/com/android/internal/util/Preconditions.java b/core/java/com/android/internal/util/Preconditions.java index 2f26e921d03b3..53cb56ebecd9d 100644 --- a/core/java/com/android/internal/util/Preconditions.java +++ b/core/java/com/android/internal/util/Preconditions.java @@ -99,6 +99,20 @@ public class Preconditions { return reference; } + /** + * Ensures the truth of an expression involving the state of the calling + * instance, but not involving any parameters to the calling method. + * + * @param expression a boolean expression + * @param message exception message + * @throws IllegalStateException if {@code expression} is false + */ + public static void checkState(final boolean expression, String message) { + if (!expression) { + throw new IllegalStateException(message); + } + } + /** * Ensures the truth of an expression involving the state of the calling * instance, but not involving any parameters to the calling method. @@ -107,9 +121,7 @@ public class Preconditions { * @throws IllegalStateException if {@code expression} is false */ public static void checkState(final boolean expression) { - if (!expression) { - throw new IllegalStateException(); - } + checkState(expression, null); } /** diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index d7222a5d548d7..55d41bfb414b2 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -2813,6 +2813,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { return; } enforceFullCrossUsersPermission(userHandle); + enforceUserUnlocked(userHandle); synchronized (this) { ActiveAdmin admin = getActiveAdminUncheckedLocked(adminReceiver, userHandle); if (admin == null) { @@ -5524,6 +5525,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { throw new SecurityException( "clearDeviceOwner can only be called by the device owner"); } + enforceUserUnlocked(deviceOwnerUserId); final ActiveAdmin admin = getDeviceOwnerAdminLocked(); if (admin != null) { @@ -5578,6 +5580,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { final UserHandle callingUser = mInjector.binderGetCallingUserHandle(); final int userId = callingUser.getIdentifier(); enforceNotManagedProfile(userId, "clear profile owner"); + enforceUserUnlocked(userId); // Check if this is the profile owner who is calling final ActiveAdmin admin = getActiveAdminForCallerLocked(who, DeviceAdminInfo.USES_POLICY_PROFILE_OWNER); @@ -5953,6 +5956,11 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { } } + private void enforceUserUnlocked(int userId) { + Preconditions.checkState(mUserManager.isUserUnlocked(userId), + "User must be running and unlocked"); + } + private void enforceManageUsers() { final int callingUid = mInjector.binderGetCallingUid(); if (!(isCallerWithSystemUid() || callingUid == Process.ROOT_UID)) { @@ -8517,6 +8525,8 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { final int userId = mInjector.userHandleGetCallingUserId(); + enforceUserUnlocked(userId); + final ComponentName profileOwner = getProfileOwner(userId); if (profileOwner != null && packageName.equals(profileOwner.getPackageName())) { throw new IllegalArgumentException("Cannot uninstall a package with a profile owner"); diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java index 64f60d93628b8..4a87e5b36124d 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java @@ -43,7 +43,6 @@ import org.mockito.stubbing.Answer; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -91,6 +90,9 @@ public class DevicePolicyManagerTest extends DpmTestBase { when(mContext.packageManager.hasSystemFeature(eq(PackageManager.FEATURE_DEVICE_ADMIN))) .thenReturn(true); + // By default, pretend all users are running and unlocked. + when(mContext.userManager.isUserUnlocked(anyInt())).thenReturn(true); + initializeDpms(); setUpPackageManagerForAdmin(admin1, DpmMockContext.CALLER_UID); @@ -411,6 +413,45 @@ public class DevicePolicyManagerTest extends DpmTestBase { } } + /** + * {@link DevicePolicyManager#removeActiveAdmin} should fail with the user is not unlocked + * (because we can't send the remove broadcast). + */ + public void testRemoveActiveAdmin_userNotRunningOrLocked() { + mContext.callerPermissions.add(android.Manifest.permission.MANAGE_DEVICE_ADMINS); + + mContext.binder.callingUid = DpmMockContext.CALLER_UID; + + // Add admin. + + dpm.setActiveAdmin(admin1, /* replace =*/ false); + + assertTrue(dpm.isAdminActive(admin1)); + + assertFalse(dpm.isRemovingAdmin(admin1, DpmMockContext.CALLER_USER_HANDLE)); + + // 1. User not unlocked. + when(mContext.userManager.isUserUnlocked(eq(DpmMockContext.CALLER_USER_HANDLE))) + .thenReturn(false); + try { + dpm.removeActiveAdmin(admin1); + fail("Didn't throw IllegalStateException"); + } catch (IllegalStateException expected) { + MoreAsserts.assertContainsRegex( + "User must be running and unlocked", expected.getMessage()); + } + + assertFalse(dpm.isRemovingAdmin(admin1, DpmMockContext.CALLER_USER_HANDLE)); + + // 2. User unlocked. + when(mContext.userManager.isUserUnlocked(eq(DpmMockContext.CALLER_USER_HANDLE))) + .thenReturn(true); + + dpm.removeActiveAdmin(admin1); + + assertTrue(dpm.isRemovingAdmin(admin1, DpmMockContext.CALLER_USER_HANDLE)); + } + /** * Test for: * {@link DevicePolicyManager#removeActiveAdmin} @@ -779,6 +820,18 @@ public class DevicePolicyManagerTest extends DpmTestBase { doReturn(DpmMockContext.CALLER_SYSTEM_USER_UID).when(mContext.packageManager).getPackageUidAsUser( eq(admin1.getPackageName()), anyInt()); + + // But first pretend the user is locked. Then it should fail. + when(mContext.userManager.isUserUnlocked(anyInt())).thenReturn(false); + try { + dpm.clearDeviceOwnerApp(admin1.getPackageName()); + fail("Didn't throw IllegalStateException"); + } catch (IllegalStateException expected) { + MoreAsserts.assertContainsRegex( + "User must be running and unlocked", expected.getMessage()); + } + + when(mContext.userManager.isUserUnlocked(anyInt())).thenReturn(true); reset(mContext.userManagerInternal); dpm.clearDeviceOwnerApp(admin1.getPackageName()); @@ -866,7 +919,19 @@ public class DevicePolicyManagerTest extends DpmTestBase { assertTrue(dpm.isProfileOwnerApp(admin1.getPackageName())); assertFalse(dpm.isRemovingAdmin(admin1, DpmMockContext.CALLER_USER_HANDLE)); - // Clear + // First try when the user is locked, which should fail. + when(mContext.userManager.isUserUnlocked(anyInt())) + .thenReturn(false); + try { + dpm.clearProfileOwner(admin1); + fail("Didn't throw IllegalStateException"); + } catch (IllegalStateException expected) { + MoreAsserts.assertContainsRegex( + "User must be running and unlocked", expected.getMessage()); + } + // Clear, really. + when(mContext.userManager.isUserUnlocked(anyInt())) + .thenReturn(true); dpm.clearProfileOwner(admin1); // Check