From 8297373a79a7e4ac9dc2b0daefbb7f03ee7f7436 Mon Sep 17 00:00:00 2001 From: Winson Chiu Date: Thu, 9 Mar 2023 21:30:17 +0000 Subject: [PATCH] Migrate DevicePolicyManagerService to use PackageManagerLocal Because this code runs in system server and clears calling identity anyways, the PackageManagerLocal API is lighter weight and allows logging failures with more specificity. Also fixes the tests to use proper mocking of the snapshot APIs. Bug: 241384985 Test: atest com.android.server.devicepolicy Change-Id: Iebfdb3e9da33629ad768436c9f9d16721d7595af --- .../DevicePolicyManagerService.java | 86 ++++++++++++------- .../DevicePolicyManagerServiceTestable.java | 6 ++ .../devicepolicy/DevicePolicyManagerTest.java | 36 ++++---- .../server/devicepolicy/DpmMockContext.java | 7 +- .../server/devicepolicy/DpmTestBase.java | 7 +- .../devicepolicy/MockSystemServices.java | 30 ++++++- 6 files changed, 115 insertions(+), 57 deletions(-) diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index e9c23a052422a..14bf8cfb41130 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -471,6 +471,7 @@ import com.android.modules.utils.TypedXmlPullParser; import com.android.modules.utils.TypedXmlSerializer; import com.android.net.module.util.ProxyUtils; import com.android.server.AlarmManagerInternal; +import com.android.server.LocalManagerRegistry; import com.android.server.LocalServices; import com.android.server.LockGuard; import com.android.server.PersistentDataBlockManagerInternal; @@ -481,6 +482,7 @@ import com.android.server.inputmethod.InputMethodManagerInternal; import com.android.server.net.NetworkPolicyManagerInternal; import com.android.server.pm.DefaultCrossProfileIntentFilter; import com.android.server.pm.DefaultCrossProfileIntentFiltersUtils; +import com.android.server.pm.PackageManagerLocal; import com.android.server.pm.RestrictionsSet; import com.android.server.pm.UserManagerInternal; import com.android.server.pm.UserManagerInternal.UserRestrictionsListener; @@ -1614,6 +1616,10 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { return LocalServices.getService(PackageManagerInternal.class); } + PackageManagerLocal getPackageManagerLocal() { + return LocalManagerRegistry.getManager(PackageManagerLocal.class); + } + ActivityTaskManagerInternal getActivityTaskManagerInternal() { return LocalServices.getService(ActivityTaskManagerInternal.class); } @@ -16344,19 +16350,26 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { // The per-Q behavior was to not check the app-ops state. granted = mIPackageManager.checkPermission(permission, packageName, userId); } else { - try { - int uid = mInjector.getPackageManager().getPackageUidAsUser( - packageName, userId); - if (PermissionChecker.checkPermissionForPreflight(mContext, permission, - PermissionChecker.PID_UNKNOWN, uid, packageName) - != PermissionChecker.PERMISSION_GRANTED) { - granted = PackageManager.PERMISSION_DENIED; + try (var snapshot = mInjector.getPackageManagerLocal().withUnfilteredSnapshot()) { + var packageState = snapshot.getPackageStates().get(packageName); + if (packageState == null) { + Slog.w(LOG_TAG, "Can't get permission state for missing package " + + packageName); + return DevicePolicyManager.PERMISSION_GRANT_STATE_DEFAULT; + } else if (!packageState.getUserStateOrDefault(userId).isInstalled()) { + Slog.w(LOG_TAG, "Can't get permission state for uninstalled package " + + packageName); + return DevicePolicyManager.PERMISSION_GRANT_STATE_DEFAULT; } else { - granted = PackageManager.PERMISSION_GRANTED; + if (PermissionChecker.checkPermissionForPreflight(mContext, permission, + PermissionChecker.PID_UNKNOWN, + UserHandle.getUid(userId, packageState.getAppId()), packageName) + != PermissionChecker.PERMISSION_GRANTED) { + granted = PackageManager.PERMISSION_DENIED; + } else { + granted = PackageManager.PERMISSION_GRANTED; + } } - } catch (NameNotFoundException e) { - // Package does not exit - return DevicePolicyManager.PERMISSION_GRANT_STATE_DEFAULT; } } int permFlags = mInjector.getPackageManager().getPermissionFlags( @@ -19334,23 +19347,29 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { @Override public boolean isPackageAllowedToAccessCalendarForUser(String packageName, - int userHandle) { + @UserIdInt int userId) { if (!mHasFeature) { return false; } Preconditions.checkStringNotEmpty(packageName, "Package name is null or empty"); - Preconditions.checkArgumentNonnegative(userHandle, "Invalid userId"); + Preconditions.checkArgumentNonnegative(userId, "Invalid userId"); - final CallerIdentity caller = getCallerIdentity(); - final int packageUid = mInjector.binderWithCleanCallingIdentity(() -> { - try { - return mInjector.getPackageManager().getPackageUidAsUser(packageName, userHandle); - } catch (NameNotFoundException e) { - Slogf.w(LOG_TAG, e, - "Couldn't find package %s in user %d", packageName, userHandle); - return -1; + final int packageUid; + try (var snapshot = mInjector.getPackageManagerLocal().withUnfilteredSnapshot()) { + var packageState = snapshot.getPackageStates().get(packageName); + if (packageState == null) { + Slogf.w(LOG_TAG, "Couldn't find package %s in user %d", packageName, + userId); + return false; + } else if (!packageState.getUserStateOrDefault(userId).isInstalled()) { + Slogf.w(LOG_TAG, "Couldn't find installed package %s in user %d", packageName, + userId); + return false; + } else { + packageUid = UserHandle.getUid(userId, packageState.getAppId()); } - }); + } + final CallerIdentity caller = getCallerIdentity(); if (caller.getUid() != packageUid) { Preconditions.checkCallAuthorization( hasCallingOrSelfPermission(permission.INTERACT_ACROSS_USERS) @@ -19359,10 +19378,10 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { synchronized (getLockObject()) { if (mInjector.settingsSecureGetIntForUser( - Settings.Secure.CROSS_PROFILE_CALENDAR_ENABLED, 0, userHandle) == 0) { + Settings.Secure.CROSS_PROFILE_CALENDAR_ENABLED, 0, userId) == 0) { return false; } - final ActiveAdmin admin = getProfileOwnerAdminLocked(userHandle); + final ActiveAdmin admin = getProfileOwnerAdminLocked(userId); if (admin != null) { if (admin.mCrossProfileCalendarPackages == null) { return true; @@ -19716,16 +19735,19 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { } private boolean isCallingFromPackage(String packageName, int callingUid) { - return mInjector.binderWithCleanCallingIdentity(() -> { - try { - final int packageUid = mInjector.getPackageManager().getPackageUidAsUser( - packageName, UserHandle.getUserId(callingUid)); - return packageUid == callingUid; - } catch (NameNotFoundException e) { - Slogf.d(LOG_TAG, "Calling package not found", e); + try (var snapshot = mInjector.getPackageManagerLocal().withUnfilteredSnapshot()) { + var packageState = snapshot.getPackageStates().get(packageName); + var userId = UserHandle.getUserId(callingUid); + if (packageState == null) { + Slogf.d(LOG_TAG, "Calling UID " + callingUid + " not found"); return false; + } else if (!packageState.getUserStateOrDefault(userId).isInstalled()) { + Slogf.d(LOG_TAG, "Calling UID " + callingUid + " not installed"); + return false; + } else { + return callingUid == UserHandle.getUid(userId, packageState.getAppId()); } - }); + } } private DevicePolicyConstants loadConstants() { diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceTestable.java b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceTestable.java index f0013a6f98b98..9b32a809d2b50 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceTestable.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceTestable.java @@ -56,6 +56,7 @@ import com.android.server.AlarmManagerInternal; import com.android.server.LocalServices; import com.android.server.PersistentDataBlockManagerInternal; import com.android.server.net.NetworkPolicyManagerInternal; +import com.android.server.pm.PackageManagerLocal; import com.android.server.pm.UserManagerInternal; import com.android.server.wm.ActivityTaskManagerInternal; @@ -149,6 +150,11 @@ public class DevicePolicyManagerServiceTestable extends DevicePolicyManagerServi return services.packageManagerInternal; } + @Override + PackageManagerLocal getPackageManagerLocal() { + return services.packageManagerLocal; + } + @Override PowerManagerInternal getPowerManagerInternal() { return services.powerManagerInternal; 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 1f25da7a3cefe..d0fcd43cff368 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerTest.java @@ -162,9 +162,9 @@ import com.android.server.pm.UserRestrictionsUtils; import org.hamcrest.BaseMatcher; import org.hamcrest.Description; import org.hamcrest.Matcher; -import org.junit.Ignore; import org.junit.After; import org.junit.Before; +import org.junit.Ignore; import org.junit.Test; import org.mockito.Mockito; import org.mockito.internal.util.collections.Sets; @@ -1388,8 +1388,8 @@ public class DevicePolicyManagerTest extends DpmTestBase { when(getServices().userManager.getUserRestrictions()).thenReturn(new Bundle()); // Now call clear. - doReturn(DpmMockContext.CALLER_SYSTEM_USER_UID).when(getServices().packageManager). - getPackageUidAsUser(eq(admin1.getPackageName()), anyInt()); + getServices().addTestPackageUid(admin1.getPackageName(), + DpmMockContext.CALLER_SYSTEM_USER_UID); // But first pretend the user is locked. Then it should fail. when(getServices().userManager.isUserUnlocked(anyInt())).thenReturn(false); @@ -1485,9 +1485,7 @@ public class DevicePolicyManagerTest extends DpmTestBase { mContext.binder.callingUid = DpmMockContext.CALLER_UID; // Now call clear. - doReturn(DpmMockContext.CALLER_UID).when(getServices().packageManager).getPackageUidAsUser( - eq(admin1.getPackageName()), - anyInt()); + getServices().addTestPackageUid(admin1.getPackageName(), DpmMockContext.CALLER_UID); assertExpectException(SecurityException.class, /* messageRegex =*/ "clearDeviceOwner can only be called by the device owner", () -> dpm.clearDeviceOwnerApp(admin1.getPackageName())); @@ -1724,9 +1722,7 @@ public class DevicePolicyManagerTest extends DpmTestBase { eq(userId)); doReturn(true).when(getServices().ipackageManager).isPackageAvailable(packageName, userId); // Setup application UID with the PackageManager - doReturn(uid).when(getServices().packageManager).getPackageUidAsUser( - eq(packageName), - eq(userId)); + getServices().addTestPackageUid(packageName, uid); // Associate packageName to uid doReturn(packageName).when(getServices().ipackageManager).getNameForUid(eq(uid)); doReturn(new String[]{packageName}) @@ -3943,9 +3939,9 @@ public class DevicePolicyManagerTest extends DpmTestBase { assertThat(dpms.hasUserSetupCompleted()).isFalse(); } - private void clearDeviceOwner() throws Exception { - doReturn(DpmMockContext.CALLER_SYSTEM_USER_UID).when(getServices().packageManager) - .getPackageUidAsUser(eq(admin1.getPackageName()), anyInt()); + private void clearDeviceOwner() { + getServices().addTestPackageUid(admin1.getPackageName(), + DpmMockContext.CALLER_SYSTEM_USER_UID); mAdmin1Context.binder.callingUid = DpmMockContext.CALLER_SYSTEM_USER_UID; runAsCaller(mAdmin1Context, dpms, dpm -> { @@ -6303,7 +6299,7 @@ public class DevicePolicyManagerTest extends DpmTestBase { mAdmin1Context.binder.callingUid = DpmMockContext.CALLER_UID; setAsProfileOwner(admin1); - final DpmMockContext caller = new DpmMockContext(getServices(), mRealTestContext); + var caller = new DpmMockContext(getServices(), mRealTestContext); caller.packageName = "com.example.delegate"; caller.binder.callingUid = setupPackageInPackageManager(caller.packageName, CALLER_USER_HANDLE, 20988, ApplicationInfo.FLAG_HAS_CODE); @@ -6947,6 +6943,7 @@ public class DevicePolicyManagerTest extends DpmTestBase { @Test public void testIsPackageAllowedToAccessCalendar_adminNotAllowed() { + final String testPackage = "TEST_PACKAGE"; setAsProfileOwner(admin1); dpm.setCrossProfileCalendarPackages(admin1, Collections.emptySet()); when(getServices().settings.settingsSecureGetIntForUser( @@ -6954,7 +6951,7 @@ public class DevicePolicyManagerTest extends DpmTestBase { 0, CALLER_USER_HANDLE)).thenReturn(1); mContext.permissions.add(permission.INTERACT_ACROSS_USERS); - assertThat(dpm.isPackageAllowedToAccessCalendar("TEST_PACKAGE")).isFalse(); + assertThat(dpm.isPackageAllowedToAccessCalendar(testPackage)).isFalse(); } @Test @@ -6973,6 +6970,7 @@ public class DevicePolicyManagerTest extends DpmTestBase { @Test public void testIsPackageAllowedToAccessCalendar_bothAllowed() { final String testPackage = "TEST_PACKAGE"; + getServices().addTestPackageUid(testPackage, DpmMockContext.ANOTHER_UID); setAsProfileOwner(admin1); dpm.setCrossProfileCalendarPackages(admin1, null); when(getServices().settings.settingsSecureGetIntForUser( @@ -6986,24 +6984,22 @@ public class DevicePolicyManagerTest extends DpmTestBase { @Test public void testIsPackageAllowedToAccessCalendar_requiresPermission() { final String testPackage = "TEST_PACKAGE"; + getServices().addTestPackageUid(testPackage, DpmMockContext.ANOTHER_UID); assertExpectException(SecurityException.class, /* messageRegex= */ null, () -> dpm.isPackageAllowedToAccessCalendar(testPackage)); } @Test - public void testIsPackageAllowedToAccessCalendar_samePackageAndSameUser_noPermissionRequired() - throws Exception { + public void testIsPackageAllowedToAccessCalendar_samePackageAndSameUser_noPermissionRequired() { final String testPackage = "TEST_PACKAGE"; setAsProfileOwner(admin1); dpm.setCrossProfileCalendarPackages(admin1, null); when(getServices().settings.settingsSecureGetIntForUser( Settings.Secure.CROSS_PROFILE_CALENDAR_ENABLED, 0, CALLER_USER_HANDLE)).thenReturn(1); - doReturn(mContext.binder.callingUid) - .when(getServices().packageManager).getPackageUidAsUser( - eq(testPackage), - anyInt()); + + getServices().addTestPackageUid(testPackage, mContext.binder.callingUid); assertThat(dpm.isPackageAllowedToAccessCalendar(testPackage)).isTrue(); } diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/DpmMockContext.java b/services/tests/servicestests/src/com/android/server/devicepolicy/DpmMockContext.java index ded8ad5956456..9ff600a6d0f86 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/DpmMockContext.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DpmMockContext.java @@ -171,10 +171,15 @@ public class DpmMockContext extends MockContext { public ApplicationInfo applicationInfo = null; public DpmMockContext(MockSystemServices mockSystemServices, Context context) { + this(mockSystemServices, context, new MockBinder()); + } + + public DpmMockContext(MockSystemServices mockSystemServices, Context context, + @NonNull MockBinder mockBinder) { mMockSystemServices = mockSystemServices; realTestContext = context; + binder = mockBinder; - binder = new MockBinder(); resources = mock(Resources.class); spiedContext = mock(Context.class); diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/DpmTestBase.java b/services/tests/servicestests/src/com/android/server/devicepolicy/DpmTestBase.java index b8824c3d8268a..03aaeb7e0db8f 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/DpmTestBase.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DpmTestBase.java @@ -67,8 +67,9 @@ public abstract class DpmTestBase { @Before public void setFixtures() throws Exception { - mServices = new MockSystemServices(mRealTestContext, "test-data"); - mMockContext = new DpmMockContext(mServices, mRealTestContext); + var mockBinder = new DpmMockContext.MockBinder(); + mServices = new MockSystemServices(mRealTestContext, "test-data", mockBinder); + mMockContext = new DpmMockContext(mServices, mRealTestContext, mockBinder); admin1 = new ComponentName(mRealTestContext, DummyDeviceAdmins.Admin1.class); admin2 = new ComponentName(mRealTestContext, DummyDeviceAdmins.Admin2.class); @@ -150,7 +151,7 @@ public abstract class DpmTestBase { doReturn(pi).when(mServices.ipackageManager).getPackageInfo(packageName, 0, userId); - doReturn(ai.uid).when(mServices.packageManager).getPackageUidAsUser(packageName, userId); + mServices.addTestPackageUid(packageName, ai.uid); } protected void markDelegatedCertInstallerAsInstalled() throws Exception { diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/MockSystemServices.java b/services/tests/servicestests/src/com/android/server/devicepolicy/MockSystemServices.java index 4163f33e94e9f..16fdfb16444a0 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/MockSystemServices.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/MockSystemServices.java @@ -25,6 +25,7 @@ import static org.mockito.Mockito.when; import android.accounts.Account; import android.accounts.AccountManager; +import android.annotation.NonNull; import android.app.ActivityManagerInternal; import android.app.AlarmManager; import android.app.AppOpsManager; @@ -55,6 +56,7 @@ import android.net.IIpConnectivityMetrics; import android.net.Uri; import android.net.VpnManager; import android.net.wifi.WifiManager; +import android.os.Binder; import android.os.Handler; import android.os.PowerManager; import android.os.PowerManagerInternal; @@ -77,7 +79,10 @@ import com.android.internal.widget.LockSettingsInternal; import com.android.server.AlarmManagerInternal; import com.android.server.PersistentDataBlockManagerInternal; import com.android.server.net.NetworkPolicyManagerInternal; +import com.android.server.pm.PackageManagerLocal; import com.android.server.pm.UserManagerInternal; +import com.android.server.pm.pkg.PackageState; +import com.android.server.pm.pkg.PackageUserState; import com.android.server.wm.ActivityTaskManagerInternal; import java.io.File; @@ -101,6 +106,7 @@ public class MockSystemServices { public final UsageStatsManagerInternal usageStatsManagerInternal; public final NetworkPolicyManagerInternal networkPolicyManagerInternal; public final PackageManagerInternal packageManagerInternal; + public final PackageManagerLocal packageManagerLocal; public final UserManagerForMock userManagerForMock; public final PowerManagerForMock powerManager; public final PowerManagerInternal powerManagerInternal; @@ -143,7 +149,10 @@ public class MockSystemServices { public final File dataDir; public final PolicyPathProvider pathProvider; - public MockSystemServices(Context realContext, String name) { + private final Map mTestPackageStates = new ArrayMap<>(); + + public MockSystemServices(Context realContext, String name, + @NonNull DpmMockContext.MockBinder mockBinder) { dataDir = new File(realContext.getCacheDir(), name); DpmTestUtils.clearDir(dataDir); @@ -157,6 +166,7 @@ public class MockSystemServices { userManagerForMock = mock(UserManagerForMock.class); packageManagerInternal = mock(PackageManagerInternal.class); + packageManagerLocal = mock(PackageManagerLocal.class); powerManager = mock(PowerManagerForMock.class); powerManagerInternal = mock(PowerManagerInternal.class); recoverySystem = mock(RecoverySystemForMock.class); @@ -197,6 +207,14 @@ public class MockSystemServices { when(packageManagerInternal.getSystemUiServiceComponent()).thenReturn( new ComponentName("com.android.systemui", ".Service")); + addTestPackageUid("android", DpmMockContext.SYSTEM_UID); + addTestPackageUid(realContext.getPackageName(), Binder.getCallingUid()); + when(packageManagerLocal.withUnfilteredSnapshot()).thenAnswer(unused -> { + var snapshot = mock(PackageManagerLocal.UnfilteredSnapshot.class); + when(snapshot.getPackageStates()).thenAnswer(unused1 -> mTestPackageStates); + return snapshot; + }); + contentResolver = new MockContentResolver(); contentResolver.addProvider("telephony", new MockContentProvider(realContext) { @Override @@ -409,6 +427,16 @@ public class MockSystemServices { throw new UnsupportedOperationException("No package " + packageName + " for user " + user); } + public void addTestPackageUid(@NonNull String packageName, int uid) { + var packageState = mock(PackageState.class); + when(packageState.getAppId()).thenReturn(UserHandle.getAppId(uid)); + when(packageState.getUserStateOrDefault(anyInt())).thenAnswer(invocation -> { + var userState = mock(PackageUserState.class); + when(userState.isInstalled()).thenReturn(true); + return userState; + }); + mTestPackageStates.put(packageName, packageState); + } public static class EnvironmentForMock { public File getUserSystemDirectory(int userId) {