From ad993720aa31e786d5b9684c056420226a89d2ec Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Fri, 27 Jan 2023 11:39:29 -0500 Subject: [PATCH] Remove binder call And do the check in system_server instead. And make similar permission checks in NMS use the same permission class while I'm there. Test: atest com.android.server.notification Test: DeviceProvisionedCoordinatorTest Fixes: 165877658 Change-Id: Id6619ec9095368d4c52e3f1b824defa5b70a60e7 --- .../DeviceProvisionedCoordinator.java | 8 +-- .../DeviceProvisionedCoordinatorTest.java | 11 +--- .../NotificationManagerService.java | 56 +++++++++++-------- .../NotificationManagerServiceTest.java | 50 ++++++++++++++--- .../server/notification/RoleObserverTest.java | 6 +- 5 files changed, 81 insertions(+), 50 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/DeviceProvisionedCoordinator.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/DeviceProvisionedCoordinator.java index 058042c4bccd9..0c95eab979a53 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/DeviceProvisionedCoordinator.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/coordinator/DeviceProvisionedCoordinator.java @@ -71,11 +71,9 @@ public class DeviceProvisionedCoordinator implements Coordinator { * marking them as relevant for setup are allowed to show when device is unprovisioned */ private boolean showNotificationEvenIfUnprovisioned(StatusBarNotification sbn) { - final boolean hasPermission = checkUidPermission( - Manifest.permission.NOTIFICATION_DURING_SETUP, - sbn.getUid()) == PackageManager.PERMISSION_GRANTED; - return hasPermission - && sbn.getNotification().extras.getBoolean(Notification.EXTRA_ALLOW_DURING_SETUP); + // system_server checks the permission so systemui can just check whether the + // extra exists + return sbn.getNotification().extras.getBoolean(Notification.EXTRA_ALLOW_DURING_SETUP); } private int checkUidPermission(String permission, int uid) { diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/DeviceProvisionedCoordinatorTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/DeviceProvisionedCoordinatorTest.java index 701cf95736ead..af5245934c129 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/DeviceProvisionedCoordinatorTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/coordinator/DeviceProvisionedCoordinatorTest.java @@ -117,10 +117,6 @@ public class DeviceProvisionedCoordinatorTest extends SysuiTestCase { extras.putBoolean(SHOW_WHEN_UNPROVISIONED_FLAG, true); mNotification.extras = extras; - // GIVEN notification has the permission to display during setup - when(mIPackageManager.checkUidPermission(SETUP_NOTIF_PERMISSION, NOTIF_UID)) - .thenReturn(PackageManager.PERMISSION_GRANTED); - // THEN don't filter out the notification assertFalse(mDeviceProvisionedFilter.shouldFilterOut(mEntry, 0)); } @@ -130,15 +126,10 @@ public class DeviceProvisionedCoordinatorTest extends SysuiTestCase { // GIVEN device is unprovisioned when(mDeviceProvisionedController.isDeviceProvisioned()).thenReturn(false); - // GIVEN notification has a flag to allow the notification during setup + // GIVEN notification does not have the flag to allow the notification during setup Bundle extras = new Bundle(); - extras.putBoolean(SHOW_WHEN_UNPROVISIONED_FLAG, true); mNotification.extras = extras; - // GIVEN notification does NOT have permission to display during setup - when(mIPackageManager.checkUidPermission(SETUP_NOTIF_PERMISSION, NOTIF_UID)) - .thenReturn(PackageManager.PERMISSION_DENIED); - // THEN filter out the notification assertTrue(mDeviceProvisionedFilter.shouldFilterOut(mEntry, 0)); } diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index b06c4118f9b84..77c5608964fe3 100755 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -521,6 +521,8 @@ public class NotificationManagerService extends SystemService { private PackageManager mPackageManagerClient; PackageManagerInternal mPackageManagerInternal; private PermissionPolicyInternal mPermissionPolicyInternal; + + private PermissionManagerServiceInternal mPermissionInternal; AudioManager mAudioManager; AudioManagerInternal mAudioManagerInternal; // Can be null for wear @@ -2210,7 +2212,8 @@ public class NotificationManagerService extends SystemService { TelephonyManager telephonyManager, ActivityManagerInternal ami, MultiRateLimiter toastRateLimiter, PermissionHelper permissionHelper, UsageStatsManagerInternal usageStatsManagerInternal, - TelecomManager telecomManager, NotificationChannelLogger channelLogger) { + TelecomManager telecomManager, NotificationChannelLogger channelLogger, + PermissionManagerServiceInternal permInternal) { mHandler = handler; Resources resources = getContext().getResources(); mMaxPackageEnqueueRate = Settings.Global.getFloat(getContext().getContentResolver(), @@ -2228,6 +2231,7 @@ public class NotificationManagerService extends SystemService { mPackageManagerClient = packageManagerClient; mPackageManagerInternal = LocalServices.getService(PackageManagerInternal.class); mPermissionPolicyInternal = LocalServices.getService(PermissionPolicyInternal.class); + mPermissionInternal = permInternal; mUmInternal = LocalServices.getService(UserManagerInternal.class); mUsageStatsManagerInternal = usageStatsManagerInternal; mAppOps = appOps; @@ -2539,7 +2543,8 @@ public class NotificationManagerService extends SystemService { AppGlobals.getPermissionManager()), LocalServices.getService(UsageStatsManagerInternal.class), getContext().getSystemService(TelecomManager.class), - new NotificationChannelLoggerImpl()); + new NotificationChannelLoggerImpl(), + LocalServices.getService(PermissionManagerServiceInternal.class)); publishBinderService(Context.NOTIFICATION_SERVICE, mService, /* allowIsolated= */ false, DUMP_FLAG_PRIORITY_CRITICAL | DUMP_FLAG_PRIORITY_NORMAL); @@ -6689,17 +6694,30 @@ public class NotificationManagerService extends SystemService { (userId == UserHandle.USER_ALL) ? USER_SYSTEM : userId); Notification.addFieldsFromContext(ai, notification); - int canColorize = mPackageManagerClient.checkPermission( - android.Manifest.permission.USE_COLORIZED_NOTIFICATIONS, pkg); + int canColorize = mPermissionInternal.checkPermission( + android.Manifest.permission.USE_COLORIZED_NOTIFICATIONS, pkg, userId); if (canColorize == PERMISSION_GRANTED) { notification.flags |= Notification.FLAG_CAN_COLORIZE; } else { notification.flags &= ~Notification.FLAG_CAN_COLORIZE; } + if (notification.extras.getBoolean(Notification.EXTRA_ALLOW_DURING_SETUP, false)) { + int hasShowDuringSetupPerm = mPermissionInternal.checkPermission( + android.Manifest.permission.NOTIFICATION_DURING_SETUP, pkg, userId); + if (hasShowDuringSetupPerm != PERMISSION_GRANTED) { + notification.extras.remove(Notification.EXTRA_ALLOW_DURING_SETUP); + if (DBG) { + Slog.w(TAG, "warning: pkg " + pkg + " attempting to show during setup" + + " without holding perm " + + Manifest.permission.NOTIFICATION_DURING_SETUP); + } + } + } + if (notification.fullScreenIntent != null && ai.targetSdkVersion >= Build.VERSION_CODES.Q) { - int fullscreenIntentPermission = mPackageManagerClient.checkPermission( - android.Manifest.permission.USE_FULL_SCREEN_INTENT, pkg); + int fullscreenIntentPermission = mPermissionInternal.checkPermission( + android.Manifest.permission.USE_FULL_SCREEN_INTENT, pkg, userId); if (fullscreenIntentPermission != PERMISSION_GRANTED) { notification.fullScreenIntent = null; Slog.w(TAG, "Package " + pkg + @@ -6719,7 +6737,7 @@ public class NotificationManagerService extends SystemService { // Ensure MediaStyle has correct permissions for remote device extras if (notification.isStyle(Notification.MediaStyle.class)) { - int hasMediaContentControlPermission = mPackageManager.checkPermission( + int hasMediaContentControlPermission = mPermissionInternal.checkPermission( android.Manifest.permission.MEDIA_CONTENT_CONTROL, pkg, userId); if (hasMediaContentControlPermission != PERMISSION_GRANTED) { notification.extras.remove(Notification.EXTRA_MEDIA_REMOTE_DEVICE); @@ -6734,7 +6752,7 @@ public class NotificationManagerService extends SystemService { // Ensure only allowed packages have a substitute app name if (notification.extras.containsKey(Notification.EXTRA_SUBSTITUTE_APP_NAME)) { - int hasSubstituteAppNamePermission = mPackageManager.checkPermission( + int hasSubstituteAppNamePermission = mPermissionInternal.checkPermission( permission.SUBSTITUTE_NOTIFICATION_APP_NAME, pkg, userId); if (hasSubstituteAppNamePermission != PERMISSION_GRANTED) { notification.extras.remove(Notification.EXTRA_SUBSTITUTE_APP_NAME); @@ -8338,15 +8356,9 @@ public class NotificationManagerService extends SystemService { } private boolean isExemptFromRateLimiting(String pkg, int userId) { - boolean isExemptFromRateLimiting = false; - try { - isExemptFromRateLimiting = mPackageManager.checkPermission( - android.Manifest.permission.UNLIMITED_TOASTS, pkg, userId) - == PackageManager.PERMISSION_GRANTED; - } catch (RemoteException e) { - Slog.e(TAG, "Failed to connect with package manager"); - } - return isExemptFromRateLimiting; + return mPermissionInternal.checkPermission( + android.Manifest.permission.UNLIMITED_TOASTS, pkg, userId) + == PackageManager.PERMISSION_GRANTED; } /** Reports rate limiting toasts compat change (used when the toast was blocked). */ @@ -9992,13 +10004,9 @@ public class NotificationManagerService extends SystemService { boolean canUseManagedServices(String pkg, Integer userId, String requiredPermission) { boolean canUseManagedServices = true; if (requiredPermission != null) { - try { - if (mPackageManager.checkPermission(requiredPermission, pkg, userId) - != PackageManager.PERMISSION_GRANTED) { - canUseManagedServices = false; - } - } catch (RemoteException e) { - Slog.e(TAG, "can't talk to pm", e); + if (mPermissionInternal.checkPermission(requiredPermission, pkg, userId) + != PackageManager.PERMISSION_GRANTED) { + canUseManagedServices = false; } } diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java index 5f8a2b5027efc..20e8cd4671ba1 100755 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -19,6 +19,7 @@ package com.android.server.notification; import static android.app.ActivityManager.RunningAppProcessInfo.IMPORTANCE_FOREGROUND; import static android.app.ActivityManager.RunningAppProcessInfo.IMPORTANCE_VISIBLE; import static android.app.ActivityTaskManager.INVALID_TASK_ID; +import static android.app.Notification.EXTRA_ALLOW_DURING_SETUP; import static android.app.Notification.FLAG_AUTO_CANCEL; import static android.app.Notification.FLAG_BUBBLE; import static android.app.Notification.FLAG_CAN_COLORIZE; @@ -217,6 +218,7 @@ import com.android.server.notification.NotificationManagerService.NotificationAs import com.android.server.notification.NotificationManagerService.NotificationListeners; import com.android.server.pm.PackageManagerService; import com.android.server.pm.UserManagerInternal; +import com.android.server.pm.permission.PermissionManagerServiceInternal; import com.android.server.policy.PermissionPolicyInternal; import com.android.server.statusbar.StatusBarManagerInternal; import com.android.server.uri.UriGrantsManagerInternal; @@ -305,6 +307,9 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { ActivityManager mActivityManager; @Mock TelecomManager mTelecomManager; + + @Mock + PermissionManagerServiceInternal mPermissionInternal; @Mock Resources mResources; @Mock @@ -518,7 +523,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { mAppOpsManager, mAppOpsService, mUm, mHistoryManager, mStatsManager, mock(TelephonyManager.class), mAmi, mToastRateLimiter, mPermissionHelper, mock(UsageStatsManagerInternal.class), - mTelecomManager, mLogger); + mTelecomManager, mLogger, mPermissionInternal); // Return first true for RoleObserver main-thread check when(mMainLooper.isCurrentThread()).thenReturn(true).thenReturn(false); mService.onBootPhase(SystemService.PHASE_SYSTEM_SERVICES_READY, mMainLooper); @@ -4167,9 +4172,36 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { verify(mGroupHelper, never()).onNotificationPosted(any(), anyBoolean()); } + @Test + public void testNoNotificationDuringSetupPermission() throws Exception { + when(mPermissionInternal.checkPermission(any(), any(), anyInt())) + .thenReturn(PERMISSION_DENIED); + Bundle extras = new Bundle(); + extras.putBoolean(EXTRA_ALLOW_DURING_SETUP, true); + Notification.Builder nb = new Notification.Builder(mContext, + mTestNotificationChannel.getId()) + .setContentTitle("foo") + .addExtras(extras) + .setSmallIcon(android.R.drawable.sym_def_app_icon); + StatusBarNotification sbn = new StatusBarNotification(PKG, PKG, 1, + "testNoNotificationDuringSetupPermission", mUid, 0, + nb.build(), UserHandle.getUserHandleForUid(mUid), null, 0); + NotificationRecord nr = new NotificationRecord(mContext, sbn, mTestNotificationChannel); + + mBinderService.enqueueNotificationWithTag(PKG, PKG, sbn.getTag(), + nr.getSbn().getId(), nr.getSbn().getNotification(), nr.getSbn().getUserId()); + waitForIdle(); + + NotificationRecord posted = mService.findNotificationLocked( + PKG, nr.getSbn().getTag(), nr.getSbn().getId(), nr.getSbn().getUserId()); + + assertFalse(posted.getNotification().extras.containsKey(EXTRA_ALLOW_DURING_SETUP)); + } + @Test public void testNoFakeColorizedPermission() throws Exception { - when(mPackageManagerClient.checkPermission(any(), any())).thenReturn(PERMISSION_DENIED); + when(mPermissionInternal.checkPermission(any(), any(), anyInt())) + .thenReturn(PERMISSION_DENIED); Notification.Builder nb = new Notification.Builder(mContext, mTestNotificationChannel.getId()) .setContentTitle("foo") @@ -4194,7 +4226,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Test public void testMediaStyleRemote_hasPermission() throws RemoteException { String deviceName = "device"; - when(mPackageManager.checkPermission( + when(mPermissionInternal.checkPermission( eq(android.Manifest.permission.MEDIA_CONTENT_CONTROL), any(), anyInt())) .thenReturn(PERMISSION_GRANTED); Notification.MediaStyle style = new Notification.MediaStyle(); @@ -4223,7 +4255,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Test public void testMediaStyleRemote_noPermission() throws RemoteException { String deviceName = "device"; - when(mPackageManager.checkPermission( + when(mPermissionInternal.checkPermission( eq(android.Manifest.permission.MEDIA_CONTENT_CONTROL), any(), anyInt())) .thenReturn(PERMISSION_DENIED); Notification.MediaStyle style = new Notification.MediaStyle(); @@ -4251,7 +4283,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Test public void testSubstituteAppName_hasPermission() throws RemoteException { String subName = "Substitute Name"; - when(mPackageManager.checkPermission( + when(mPermissionInternal.checkPermission( eq(android.Manifest.permission.SUBSTITUTE_NOTIFICATION_APP_NAME), any(), anyInt())) .thenReturn(PERMISSION_GRANTED); Bundle extras = new Bundle(); @@ -4278,7 +4310,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Test public void testSubstituteAppName_noPermission() throws RemoteException { - when(mPackageManager.checkPermission( + when(mPermissionInternal.checkPermission( eq(android.Manifest.permission.SUBSTITUTE_NOTIFICATION_APP_NAME), any(), anyInt())) .thenReturn(PERMISSION_DENIED); Bundle extras = new Bundle(); @@ -5788,7 +5820,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Test public void testCanUseManagedServices_hasPermission() throws Exception { - when(mPackageManager.checkPermission("perm", "pkg", 0)) + when(mPermissionInternal.checkPermission("perm", "pkg", 0)) .thenReturn(PackageManager.PERMISSION_GRANTED); assertEquals(true, mService.canUseManagedServices("pkg", 0, "perm")); @@ -5796,7 +5828,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Test public void testCanUseManagedServices_noPermission() throws Exception { - when(mPackageManager.checkPermission("perm", "pkg", 0)) + when(mPermissionInternal.checkPermission("perm", "pkg", 0)) .thenReturn(PackageManager.PERMISSION_DENIED); assertEquals(false, mService.canUseManagedServices("pkg", 0, "perm")); @@ -6751,7 +6783,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { private void setIfPackageHasPermissionToAvoidToastRateLimiting( String pkg, boolean hasPermission) throws Exception { - when(mPackageManager.checkPermission(android.Manifest.permission.UNLIMITED_TOASTS, + when(mPermissionInternal.checkPermission(android.Manifest.permission.UNLIMITED_TOASTS, pkg, UserHandle.getUserId(mUid))) .thenReturn(hasPermission ? PERMISSION_GRANTED : PERMISSION_DENIED); } diff --git a/services/tests/uiservicestests/src/com/android/server/notification/RoleObserverTest.java b/services/tests/uiservicestests/src/com/android/server/notification/RoleObserverTest.java index 98c156e6f3b50..3a01c2d26e50f 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/RoleObserverTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/RoleObserverTest.java @@ -72,6 +72,7 @@ import com.android.server.UiServiceTestCase; import com.android.server.lights.LightsManager; import com.android.server.notification.NotificationManagerService.NotificationAssistants; import com.android.server.notification.NotificationManagerService.NotificationListeners; +import com.android.server.pm.permission.PermissionManagerServiceInternal; import com.android.server.uri.UriGrantsManagerInternal; import com.android.server.utils.quota.MultiRateLimiter; import com.android.server.wm.ActivityTaskManagerInternal; @@ -168,8 +169,9 @@ public class RoleObserverTest extends UiServiceTestCase { mock(StatsManager.class), mock(TelephonyManager.class), mock(ActivityManagerInternal.class), mock(MultiRateLimiter.class), mock(PermissionHelper.class), - mock(UsageStatsManagerInternal.class), mock (TelecomManager.class), - mock(NotificationChannelLogger.class)); + mock(UsageStatsManagerInternal.class), mock(TelecomManager.class), + mock(NotificationChannelLogger.class), + mock(PermissionManagerServiceInternal.class)); } catch (SecurityException e) { if (!e.getMessage().contains("Permission Denial: not allowed to send broadcast")) { throw e;