From 6cd9d5eea1734942c378b840dc276e4c5bd228e7 Mon Sep 17 00:00:00 2001 From: Julia Reynolds Date: Wed, 19 Aug 2020 14:22:34 -0400 Subject: [PATCH] Stop providing duplicate info to NAS Since the NAS is a notification listener, it has access to notifications in its current user and enabled managed profiles. However, unlike other listeners, it also has an instance running in the managed profile. This adds extra filtering on 'current user' before sending information to the NAS so the NAS doesn't have to filter itself. Test: CTS tests for listeners and assistants Test: atest Fixes: 154938387 Change-Id: If8dee2ac6d6ce71fdaf6f43bd240b361e8828686 --- .../server/notification/ManagedServices.java | 2 +- .../NotificationManagerService.java | 99 ++++++++++--------- .../notification/ManagedServicesTest.java | 1 + .../NotificationManagerServiceTest.java | 63 +++++++++++- 4 files changed, 114 insertions(+), 51 deletions(-) diff --git a/services/core/java/com/android/server/notification/ManagedServices.java b/services/core/java/com/android/server/notification/ManagedServices.java index a604625460a77..74b7bd76b0471 100644 --- a/services/core/java/com/android/server/notification/ManagedServices.java +++ b/services/core/java/com/android/server/notification/ManagedServices.java @@ -1554,7 +1554,7 @@ abstract public class ManagedServices { if (!isEnabledForCurrentProfiles()) { return false; } - return this.userid == userId; + return userId == USER_ALL || userId == this.userid; } public boolean enabledAndUserMatches(int nid) { diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 04658555f22b6..f247920789ca1 100755 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -262,7 +262,6 @@ import com.android.server.EventLogTags; import com.android.server.IoThread; import com.android.server.LocalServices; import com.android.server.SystemService; -import com.android.server.SystemService.TargetUser; import com.android.server.UiThread; import com.android.server.lights.LightsManager; import com.android.server.lights.LogicalLight; @@ -966,8 +965,7 @@ public class NotificationManagerService extends SystemService { nv.recycle(); } reportUserInteraction(r); - mAssistants.notifyAssistantActionClicked( - r.getSbn(), actionIndex, action, generatedByAssistant); + mAssistants.notifyAssistantActionClicked(r.getSbn(), action, generatedByAssistant); } } @@ -8629,12 +8627,25 @@ public class NotificationManagerService extends SystemService { ServiceManager.getService(Context.COMPANION_DEVICE_SERVICE)); } - private boolean isVisibleToListener(StatusBarNotification sbn, ManagedServiceInfo listener) { + @VisibleForTesting + boolean isVisibleToListener(StatusBarNotification sbn, ManagedServiceInfo listener) { if (!listener.enabledAndUserMatches(sbn.getUserId())) { return false; } - // TODO: remove this for older listeners. - return true; + return isInteractionVisibleToListener(listener, sbn.getUserId()); + } + + /** + * Returns whether the given assistant should be informed about interactions on the given user. + * + * Normally an assistant would be able to see all interactions on the current user and any + * associated profiles because they are notification listeners, but since NASes have one + * instance per user, we want to filter out interactions that are not for the user that the + * given NAS is bound in. + */ + private boolean isInteractionVisibleToListener(ManagedServiceInfo info, int userId) { + boolean isAssistantService = mAssistants.isServiceTokenValidLocked(info.service); + return !isAssistantService || info.isSameUser(userId); } private boolean isPackageSuspendedForUser(String pkg, int uid) { @@ -8856,8 +8867,6 @@ public class NotificationManagerService extends SystemService { } protected void onNotificationsSeenLocked(ArrayList records) { - // There should be only one, but it's a list, so while we enforce - // singularity elsewhere, we keep it general here, to avoid surprises. for (final ManagedServiceInfo info : NotificationAssistants.this.getServices()) { ArrayList keys = new ArrayList<>(records.size()); for (NotificationRecord r : records) { @@ -8875,6 +8884,8 @@ public class NotificationManagerService extends SystemService { } protected void onPanelRevealed(int items) { + // send to all currently bounds NASes since notifications from both users will appear in + // the panel for (final ManagedServiceInfo info : NotificationAssistants.this.getServices()) { mHandler.post(() -> { final INotificationListener assistant = (INotificationListener) info.service; @@ -8888,6 +8899,8 @@ public class NotificationManagerService extends SystemService { } protected void onPanelHidden() { + // send to all currently bounds NASes since notifications from both users will appear in + // the panel for (final ManagedServiceInfo info : NotificationAssistants.this.getServices()) { mHandler.post(() -> { final INotificationListener assistant = (INotificationListener) info.service; @@ -8976,7 +8989,7 @@ public class NotificationManagerService extends SystemService { } notifyAssistantLocked( sbn, - false /* sameUserOnly */, + true /* sameUserOnly */, (assistant, sbnHolder) -> { try { assistant.onNotificationVisibilityChanged(key, isVisible); @@ -8994,7 +9007,7 @@ public class NotificationManagerService extends SystemService { final String key = sbn.getKey(); notifyAssistantLocked( sbn, - false /* sameUserOnly */, + true /* sameUserOnly */, (assistant, sbnHolder) -> { try { assistant.onNotificationExpansionChanged(key, isUserAction, isExpanded); @@ -9010,7 +9023,7 @@ public class NotificationManagerService extends SystemService { final String key = sbn.getKey(); notifyAssistantLocked( sbn, - false /* sameUserOnly */, + true /* sameUserOnly */, (assistant, sbnHolder) -> { try { assistant.onNotificationDirectReply(key); @@ -9026,7 +9039,7 @@ public class NotificationManagerService extends SystemService { final String key = sbn.getKey(); notifyAssistantLocked( sbn, - false /* sameUserOnly */, + true /* sameUserOnly */, (assistant, sbnHolder) -> { try { assistant.onSuggestedReplySent( @@ -9043,12 +9056,12 @@ public class NotificationManagerService extends SystemService { @GuardedBy("mNotificationLock") void notifyAssistantActionClicked( - final StatusBarNotification sbn, int actionIndex, Notification.Action action, + final StatusBarNotification sbn, Notification.Action action, boolean generatedByAssistant) { final String key = sbn.getKey(); notifyAssistantLocked( sbn, - false /* sameUserOnly */, + true /* sameUserOnly */, (assistant, sbnHolder) -> { try { assistant.onActionClicked( @@ -9072,7 +9085,7 @@ public class NotificationManagerService extends SystemService { final StatusBarNotification sbn, final String snoozeCriterionId) { notifyAssistantLocked( sbn, - false /* sameUserOnly */, + true /* sameUserOnly */, (assistant, sbnHolder) -> { try { assistant.onNotificationSnoozedUntilContext( @@ -9293,10 +9306,12 @@ public class NotificationManagerService extends SystemService { } public void onStatusBarIconsBehaviorChanged(boolean hideSilentStatusIcons) { + // send to all currently bounds NASes since notifications from both users will appear in + // the status bar for (final ManagedServiceInfo info : getServices()) { mHandler.post(() -> { final INotificationListener listener = (INotificationListener) info.service; - try { + try { listener.onStatusBarIconsBehaviorChanged(hideSilentStatusIcons); } catch (RemoteException ex) { Slog.e(TAG, "unable to notify listener " @@ -9470,7 +9485,8 @@ public class NotificationManagerService extends SystemService { && changedHiddenNotifications.size() > 0; for (final ManagedServiceInfo serviceInfo : getServices()) { - if (!serviceInfo.isEnabledForCurrentProfiles()) { + if (!serviceInfo.isEnabledForCurrentProfiles() || !isInteractionVisibleToListener( + serviceInfo, ActivityManager.getCurrentUser())) { continue; } @@ -9489,12 +9505,7 @@ public class NotificationManagerService extends SystemService { final NotificationRankingUpdate update = makeRankingUpdateLocked( serviceInfo); - mHandler.post(new Runnable() { - @Override - public void run() { - notifyRankingUpdate(serviceInfo, update); - } - }); + mHandler.post(() -> notifyRankingUpdate(serviceInfo, update)); } } } @@ -9502,15 +9513,11 @@ public class NotificationManagerService extends SystemService { @GuardedBy("mNotificationLock") public void notifyListenerHintsChangedLocked(final int hints) { for (final ManagedServiceInfo serviceInfo : getServices()) { - if (!serviceInfo.isEnabledForCurrentProfiles()) { + if (!serviceInfo.isEnabledForCurrentProfiles() || !isInteractionVisibleToListener( + serviceInfo, ActivityManager.getCurrentUser())) { continue; } - mHandler.post(new Runnable() { - @Override - public void run() { - notifyListenerHintsChanged(serviceInfo, hints); - } - }); + mHandler.post(() -> notifyListenerHintsChanged(serviceInfo, hints)); } } @@ -9562,15 +9569,12 @@ public class NotificationManagerService extends SystemService { public void notifyInterruptionFilterChanged(final int interruptionFilter) { for (final ManagedServiceInfo serviceInfo : getServices()) { - if (!serviceInfo.isEnabledForCurrentProfiles()) { + if (!serviceInfo.isEnabledForCurrentProfiles() || !isInteractionVisibleToListener( + serviceInfo, ActivityManager.getCurrentUser())) { continue; } - mHandler.post(new Runnable() { - @Override - public void run() { - notifyInterruptionFilterChanged(serviceInfo, interruptionFilter); - } - }); + mHandler.post( + () -> notifyInterruptionFilterChanged(serviceInfo, interruptionFilter)); } } @@ -9579,15 +9583,16 @@ public class NotificationManagerService extends SystemService { if (channel == null) { return; } - for (final ManagedServiceInfo serviceInfo : getServices()) { - if (!serviceInfo.enabledAndUserMatches(UserHandle.getCallingUserId())) { + for (final ManagedServiceInfo info : getServices()) { + if (!info.enabledAndUserMatches(UserHandle.getCallingUserId()) + || !isInteractionVisibleToListener(info, UserHandle.getCallingUserId())) { continue; } BackgroundThread.getHandler().post(() -> { - if (serviceInfo.isSystem || hasCompanionDevice(serviceInfo)) { + if (info.isSystem || hasCompanionDevice(info)) { notifyNotificationChannelChanged( - serviceInfo, pkg, user, channel, modificationType); + info, pkg, user, channel, modificationType); } }); } @@ -9599,15 +9604,16 @@ public class NotificationManagerService extends SystemService { if (group == null) { return; } - for (final ManagedServiceInfo serviceInfo : getServices()) { - if (!serviceInfo.enabledAndUserMatches(UserHandle.getCallingUserId())) { + for (final ManagedServiceInfo info : getServices()) { + if (!info.enabledAndUserMatches(UserHandle.getCallingUserId()) + || !isInteractionVisibleToListener(info, UserHandle.getCallingUserId())) { continue; } BackgroundThread.getHandler().post(() -> { - if (serviceInfo.isSystem || hasCompanionDevice(serviceInfo)) { + if (info.isSystem || hasCompanionDevice(info)) { notifyNotificationChannelGroupChanged( - serviceInfo, pkg, user, group, modificationType); + info, pkg, user, group, modificationType); } }); } @@ -9626,9 +9632,6 @@ public class NotificationManagerService extends SystemService { private void notifyRemoved(ManagedServiceInfo info, StatusBarNotification sbn, NotificationRankingUpdate rankingUpdate, NotificationStats stats, int reason) { - if (!info.enabledAndUserMatches(sbn.getUserId())) { - return; - } final INotificationListener listener = (INotificationListener) info.service; StatusBarNotificationHolder sbnHolder = new StatusBarNotificationHolder(sbn); try { diff --git a/services/tests/uiservicestests/src/com/android/server/notification/ManagedServicesTest.java b/services/tests/uiservicestests/src/com/android/server/notification/ManagedServicesTest.java index 99433a6603c91..d7e431f3bb51c 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/ManagedServicesTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/ManagedServicesTest.java @@ -978,6 +978,7 @@ public class ManagedServicesTest extends UiServiceTestCase { assertFalse(services.isSameUser(service, 0)); assertTrue(services.isSameUser(service, 10)); + assertTrue(services.isSameUser(service, UserHandle.USER_ALL)); } @Test 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 9319bea497fb7..86447192a441c 100755 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -5058,7 +5058,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { 10, 10, r.getKey(), actionIndex, action, notificationVisibility, generatedByAssistant); verify(mAssistants).notifyAssistantActionClicked( - eq(r.getSbn()), eq(actionIndex), eq(action), eq(generatedByAssistant)); + eq(r.getSbn()), eq(action), eq(generatedByAssistant)); assertEquals(1, mNotificationRecordLogger.numCalls()); assertEquals( @@ -5082,7 +5082,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { 10, 10, r.getKey(), actionIndex, action, notificationVisibility, generatedByAssistant); verify(mAssistants).notifyAssistantActionClicked( - eq(r.getSbn()), eq(actionIndex), eq(action), eq(generatedByAssistant)); + eq(r.getSbn()), eq(action), eq(generatedByAssistant)); assertEquals(1, mNotificationRecordLogger.numCalls()); assertEquals( @@ -6948,4 +6948,63 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { assertEquals(NotificationManagerService.MAX_PACKAGE_NOTIFICATIONS + 1, mService.getNotificationRecordCount()); } + + @Test + public void testIsVisibleToListener_notEnabled() { + StatusBarNotification sbn = mock(StatusBarNotification.class); + when(sbn.getUserId()).thenReturn(10); + ManagedServices.ManagedServiceInfo info = mock(ManagedServices.ManagedServiceInfo.class); + ManagedServices.ManagedServiceInfo assistant = mock(ManagedServices.ManagedServiceInfo.class); + info.userid = 10; + when(info.isSameUser(anyInt())).thenReturn(true); + when(assistant.isSameUser(anyInt())).thenReturn(true); + when(info.enabledAndUserMatches(info.userid)).thenReturn(false); + when(mAssistants.checkServiceTokenLocked(any())).thenReturn(assistant); + + assertFalse(mService.isVisibleToListener(sbn, info)); + } + + @Test + public void testIsVisibleToListener_noAssistant() { + StatusBarNotification sbn = mock(StatusBarNotification.class); + when(sbn.getUserId()).thenReturn(10); + ManagedServices.ManagedServiceInfo info = mock(ManagedServices.ManagedServiceInfo.class); + info.userid = 10; + when(info.isSameUser(anyInt())).thenReturn(true); + when(info.enabledAndUserMatches(info.userid)).thenReturn(true); + when(mAssistants.checkServiceTokenLocked(any())).thenReturn(null); + + assertTrue(mService.isVisibleToListener(sbn, info)); + } + + @Test + public void testIsVisibleToListener_assistant_differentUser() { + StatusBarNotification sbn = mock(StatusBarNotification.class); + when(sbn.getUserId()).thenReturn(10); + ManagedServices.ManagedServiceInfo info = mock(ManagedServices.ManagedServiceInfo.class); + ManagedServices.ManagedServiceInfo assistant = mock(ManagedServices.ManagedServiceInfo.class); + info.userid = 0; + when(info.isSameUser(anyInt())).thenReturn(true); + when(assistant.isSameUser(anyInt())).thenReturn(true); + when(info.enabledAndUserMatches(info.userid)).thenReturn(true); + when(mAssistants.checkServiceTokenLocked(any())).thenReturn(assistant); + + assertFalse(mService.isVisibleToListener(sbn, info)); + } + + @Test + public void testIsVisibleToListener_assistant_sameUser() { + StatusBarNotification sbn = mock(StatusBarNotification.class); + when(sbn.getUserId()).thenReturn(10); + ManagedServices.ManagedServiceInfo info = mock(ManagedServices.ManagedServiceInfo.class); + ManagedServices.ManagedServiceInfo assistant = mock(ManagedServices.ManagedServiceInfo.class); + info.userid = 10; + when(info.isSameUser(anyInt())).thenReturn(true); + when(assistant.isSameUser(anyInt())).thenReturn(true); + when(info.enabledAndUserMatches(info.userid)).thenReturn(true); + when(mAssistants.checkServiceTokenLocked(any())).thenReturn(assistant); + + assertTrue(mService.isVisibleToListener(sbn, info)); + } + }