From 239bcc21f89e3947411e57ebc6bc60467bf21d86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C3=ADas=20Hern=C3=A1ndez?= Date: Fri, 21 Jul 2023 14:41:39 +0200 Subject: [PATCH] Clear notifications for packages whose permission is revoked This is a rollforward of ag/24038746, with two differences: * Ignore onOpChanged callbacks for non-initialized users. We realized this is being called for all packages on first start. * Only send the ACTION_APP_BLOCK_STATE_CHANGED broadcast on manual changes coming form Settings. This is less "theoretically correct" than the previous CL, but added in the event there are more cases where onOpChanged is called unexpectedly. Test: atest NotificationManagerService + manual Fixes: 288651621 Change-Id: I7222cdbd09d51523ab16fe44a3c12dfdce0e7365 --- .../NotificationManagerService.java | 35 +++++- .../NotificationManagerServiceTest.java | 116 ++++++++++++++++++ 2 files changed, 145 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 37879effc41c2..eec52db8256a2 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -2489,6 +2489,16 @@ public class NotificationManagerService extends SystemService { getContext().registerReceiver(mReviewNotificationPermissionsReceiver, ReviewNotificationPermissionsReceiver.getFilter(), Context.RECEIVER_NOT_EXPORTED); + + mAppOps.startWatchingMode(AppOpsManager.OP_POST_NOTIFICATION, null, + new AppOpsManager.OnOpChangedInternalListener() { + @Override + public void onOpChanged(@NonNull String op, @NonNull String packageName, + int userId) { + mHandler.post( + () -> handleNotificationPermissionChange(packageName, userId)); + } + }); } /** @@ -3555,13 +3565,9 @@ public class NotificationManagerService extends SystemService { .setPackageName(pkg) .setSubtype(enabled ? 1 : 0)); mNotificationChannelLogger.logAppNotificationsAllowed(uid, pkg, enabled); - // Now, cancel any outstanding notifications that are part of a just-disabled app - if (!enabled) { - cancelAllNotificationsInt(MY_UID, MY_PID, pkg, null, 0, 0, - UserHandle.getUserId(uid), REASON_PACKAGE_BANNED); - } - handleSavePolicyFile(); + // Outstanding notifications from this package will be cancelled as soon as we get the + // callback from AppOpsManager. } /** @@ -5884,6 +5890,23 @@ public class NotificationManagerService extends SystemService { } }; + private void handleNotificationPermissionChange(String pkg, @UserIdInt int userId) { + if (!mUmInternal.isUserInitialized(userId)) { + return; // App-op "updates" are sent when starting a new user the first time. + } + int uid = mPackageManagerInternal.getPackageUid(pkg, 0, userId); + if (uid == INVALID_UID) { + Log.e(TAG, String.format("No uid found for %s, %s!", pkg, userId)); + return; + } + boolean hasPermission = mPermissionHelper.hasPermission(uid); + if (!hasPermission) { + cancelAllNotificationsInt(MY_UID, MY_PID, pkg, /* channelId= */ null, + /* mustHaveFlags= */ 0, /* mustNotHaveFlags= */ 0, userId, + REASON_PACKAGE_BANNED); + } + } + protected void checkNotificationListenerAccess() { if (!isCallerSystemOrPhone()) { getContext().enforceCallingPermission( 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 0292bca068133..44aa5825d6f0a 100755 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -405,6 +405,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { UriGrantsManagerInternal mUgmInternal; @Mock AppOpsManager mAppOpsManager; + private AppOpsManager.OnOpChangedListener mOnPermissionChangeListener; @Mock private TestableNotificationManagerService.NotificationAssistantAccessGrantedCallback mNotificationAssistantAccessGrantedCallback; @@ -604,6 +605,13 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { tr.addOverride(com.android.internal.R.string.config_defaultSearchSelectorPackageName, SEARCH_SELECTOR_PKG); + doAnswer(invocation -> { + mOnPermissionChangeListener = invocation.getArgument(2); + return null; + }).when(mAppOpsManager).startWatchingMode(eq(AppOpsManager.OP_POST_NOTIFICATION), any(), + any()); + when(mUmInternal.isUserInitialized(anyInt())).thenReturn(true); + mWorkerHandler = spy(mService.new WorkerHandler(mTestableLooper.getLooper())); mService.init(mWorkerHandler, mRankingHandler, mPackageManager, mPackageManagerClient, mockLightsManager, mListeners, mAssistants, mConditionProviders, mCompanionMgr, @@ -3215,6 +3223,108 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { anyBoolean(), anyBoolean()); } + @Test + public void onOpChanged_permissionRevoked_cancelsAllNotificationsFromPackage() + throws RemoteException { + // Have preexisting posted notifications from revoked package and other packages. + mService.addNotification(new NotificationRecord(mContext, + generateSbn("revoked", 1001, 1, 0), mTestNotificationChannel)); + mService.addNotification(new NotificationRecord(mContext, + generateSbn("other", 1002, 2, 0), mTestNotificationChannel)); + // Have preexisting enqueued notifications from revoked package and other packages. + mService.addEnqueuedNotification(new NotificationRecord(mContext, + generateSbn("revoked", 1001, 3, 0), mTestNotificationChannel)); + mService.addEnqueuedNotification(new NotificationRecord(mContext, + generateSbn("other", 1002, 4, 0), mTestNotificationChannel)); + assertThat(mService.mNotificationList).hasSize(2); + assertThat(mService.mEnqueuedNotifications).hasSize(2); + + when(mPackageManagerInternal.getPackageUid("revoked", 0, 0)).thenReturn(1001); + when(mPermissionHelper.hasPermission(eq(1001))).thenReturn(false); + + mOnPermissionChangeListener.onOpChanged( + AppOpsManager.OPSTR_POST_NOTIFICATION, "revoked", 0); + waitForIdle(); + + assertThat(mService.mNotificationList).hasSize(1); + assertThat(mService.mNotificationList.get(0).getSbn().getPackageName()).isEqualTo("other"); + assertThat(mService.mEnqueuedNotifications).hasSize(1); + assertThat(mService.mEnqueuedNotifications.get(0).getSbn().getPackageName()).isEqualTo( + "other"); + } + + @Test + public void onOpChanged_permissionStillGranted_notificationsAreNotAffected() + throws RemoteException { + // NOTE: This combination (receiving the onOpChanged broadcast for a package, the permission + // being now granted, AND having previously posted notifications from said package) should + // never happen (if we trust the broadcasts are correct). So this test is for a what-if + // scenario, to verify we still handle it reasonably. + + // Have preexisting posted notifications from specific package and other packages. + mService.addNotification(new NotificationRecord(mContext, + generateSbn("granted", 1001, 1, 0), mTestNotificationChannel)); + mService.addNotification(new NotificationRecord(mContext, + generateSbn("other", 1002, 2, 0), mTestNotificationChannel)); + // Have preexisting enqueued notifications from specific package and other packages. + mService.addEnqueuedNotification(new NotificationRecord(mContext, + generateSbn("granted", 1001, 3, 0), mTestNotificationChannel)); + mService.addEnqueuedNotification(new NotificationRecord(mContext, + generateSbn("other", 1002, 4, 0), mTestNotificationChannel)); + assertThat(mService.mNotificationList).hasSize(2); + assertThat(mService.mEnqueuedNotifications).hasSize(2); + + when(mPackageManagerInternal.getPackageUid("granted", 0, 0)).thenReturn(1001); + when(mPermissionHelper.hasPermission(eq(1001))).thenReturn(true); + + mOnPermissionChangeListener.onOpChanged( + AppOpsManager.OPSTR_POST_NOTIFICATION, "granted", 0); + waitForIdle(); + + assertThat(mService.mNotificationList).hasSize(2); + assertThat(mService.mEnqueuedNotifications).hasSize(2); + } + + @Test + public void onOpChanged_notInitializedUser_ignored() throws RemoteException { + when(mUmInternal.isUserInitialized(eq(0))).thenReturn(false); + + mOnPermissionChangeListener.onOpChanged( + AppOpsManager.OPSTR_POST_NOTIFICATION, "package", 0); + waitForIdle(); + + // We early-exited and didn't even query PM for package details. + verify(mPackageManagerInternal, never()).getPackageUid(any(), anyLong(), anyInt()); + } + + @Test + public void setNotificationsEnabledForPackage_disabling_clearsNotifications() throws Exception { + mService.addNotification(new NotificationRecord(mContext, + generateSbn("package", 1001, 1, 0), mTestNotificationChannel)); + assertThat(mService.mNotificationList).hasSize(1); + when(mPackageManagerInternal.getPackageUid("package", 0, 0)).thenReturn(1001); + when(mPermissionHelper.hasRequestedPermission(any(), eq("package"), anyInt())).thenReturn( + true); + + // Start with granted permission and simulate effect of revoking it. + when(mPermissionHelper.hasPermission(1001)).thenReturn(true); + doAnswer(invocation -> { + when(mPermissionHelper.hasPermission(1001)).thenReturn(false); + mOnPermissionChangeListener.onOpChanged( + AppOpsManager.OPSTR_POST_NOTIFICATION, "package", 0); + return null; + }).when(mPermissionHelper).setNotificationPermission("package", 0, false, true); + + mBinderService.setNotificationsEnabledForPackage("package", 1001, false); + waitForIdle(); + + assertThat(mService.mNotificationList).hasSize(0); + + mTestableLooper.moveTimeForward(500); + waitForIdle(); + verify(mContext).sendBroadcastAsUser(any(), eq(UserHandle.of(0)), eq(null)); + } + @Test public void testUpdateAppNotifyCreatorBlock() throws Exception { when(mPermissionHelper.hasPermission(mUid)).thenReturn(true); @@ -4235,6 +4345,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Test public void testSetNASMigrationDoneAndResetDefault_enableNAS() throws Exception { int userId = 10; + setNASMigrationDone(false, userId); when(mUm.getProfileIds(userId, false)).thenReturn(new int[]{userId}); mBinderService.setNASMigrationDoneAndResetDefault(userId, true); @@ -4246,6 +4357,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Test public void testSetNASMigrationDoneAndResetDefault_disableNAS() throws Exception { int userId = 10; + setNASMigrationDone(false, userId); when(mUm.getProfileIds(userId, false)).thenReturn(new int[]{userId}); mBinderService.setNASMigrationDoneAndResetDefault(userId, false); @@ -4258,6 +4370,8 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { public void testSetNASMigrationDoneAndResetDefault_multiProfile() throws Exception { int userId1 = 11; int userId2 = 12; //work profile + setNASMigrationDone(false, userId1); + setNASMigrationDone(false, userId2); setUsers(new int[]{userId1, userId2}); when(mUm.isManagedProfile(userId2)).thenReturn(true); when(mUm.getProfileIds(userId1, false)).thenReturn(new int[]{userId1, userId2}); @@ -4271,6 +4385,8 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { public void testSetNASMigrationDoneAndResetDefault_multiUser() throws Exception { int userId1 = 11; int userId2 = 12; + setNASMigrationDone(false, userId1); + setNASMigrationDone(false, userId2); setUsers(new int[]{userId1, userId2}); when(mUm.getProfileIds(userId1, false)).thenReturn(new int[]{userId1}); when(mUm.getProfileIds(userId2, false)).thenReturn(new int[]{userId2});