From e1d1d033646ae6337d374a291e8626bec911fdc7 Mon Sep 17 00:00:00 2001 From: Yuri Lin Date: Mon, 21 Nov 2022 17:52:19 +0000 Subject: [PATCH] Revert "Allow conversation channels to inherit canBypassDnd from their parent." This reverts commit c6ca8afec1ee153c6e8bc733d98d23be7c080643. Reason for revert: actually we don't want to do this; see comments on original commit Change-Id: I4495bb3ae1cf0be0997efdbdaf150c2a4f63c450 --- .../NotificationManagerService.java | 17 +++--- .../notification/PreferencesHelper.java | 6 +- .../NotificationManagerServiceTest.java | 56 ++----------------- 3 files changed, 15 insertions(+), 64 deletions(-) diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 8a7fd82e6123d..c2df904d07cef 100755 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -3796,13 +3796,13 @@ public class NotificationManagerService extends SystemService { } private void createNotificationChannelsImpl(String pkg, int uid, - ParceledListSlice channelsList, boolean fromTargetApp) { - createNotificationChannelsImpl(pkg, uid, channelsList, fromTargetApp, + ParceledListSlice channelsList) { + createNotificationChannelsImpl(pkg, uid, channelsList, ActivityTaskManager.INVALID_TASK_ID); } private void createNotificationChannelsImpl(String pkg, int uid, - ParceledListSlice channelsList, boolean fromTargetApp, int startingTaskId) { + ParceledListSlice channelsList, int startingTaskId) { List channels = channelsList.getList(); final int channelsSize = channels.size(); ParceledListSlice oldChannels = @@ -3814,7 +3814,7 @@ public class NotificationManagerService extends SystemService { final NotificationChannel channel = channels.get(i); Objects.requireNonNull(channel, "channel in list is null"); needsPolicyFileChange = mPreferencesHelper.createNotificationChannel(pkg, uid, - channel, fromTargetApp, + channel, true /* fromTargetApp */, mConditionProviders.isPackageOrComponentAllowed( pkg, UserHandle.getUserId(uid))); if (needsPolicyFileChange) { @@ -3850,7 +3850,6 @@ public class NotificationManagerService extends SystemService { @Override public void createNotificationChannels(String pkg, ParceledListSlice channelsList) { checkCallerIsSystemOrSameApp(pkg); - boolean fromTargetApp = !isCallerSystemOrPhone(); // if not system, it's from the app int taskId = ActivityTaskManager.INVALID_TASK_ID; try { int uid = mPackageManager.getPackageUid(pkg, 0, @@ -3859,15 +3858,14 @@ public class NotificationManagerService extends SystemService { } catch (RemoteException e) { // Do nothing } - createNotificationChannelsImpl(pkg, Binder.getCallingUid(), channelsList, fromTargetApp, - taskId); + createNotificationChannelsImpl(pkg, Binder.getCallingUid(), channelsList, taskId); } @Override public void createNotificationChannelsForPackage(String pkg, int uid, ParceledListSlice channelsList) { enforceSystemOrSystemUI("only system can call this"); - createNotificationChannelsImpl(pkg, uid, channelsList, false /* fromTargetApp */); + createNotificationChannelsImpl(pkg, uid, channelsList); } @Override @@ -3882,8 +3880,7 @@ public class NotificationManagerService extends SystemService { CONVERSATION_CHANNEL_ID_FORMAT, parentId, conversationId)); conversationChannel.setConversationId(parentId, conversationId); createNotificationChannelsImpl( - pkg, uid, new ParceledListSlice(Arrays.asList(conversationChannel)), - false /* fromTargetApp */); + pkg, uid, new ParceledListSlice(Arrays.asList(conversationChannel))); mRankingHandler.requestSort(); handleSavePolicyFile(); } diff --git a/services/core/java/com/android/server/notification/PreferencesHelper.java b/services/core/java/com/android/server/notification/PreferencesHelper.java index 97911581c59cc..d8aa469bcd810 100644 --- a/services/core/java/com/android/server/notification/PreferencesHelper.java +++ b/services/core/java/com/android/server/notification/PreferencesHelper.java @@ -916,7 +916,7 @@ public class PreferencesHelper implements RankingConfig { throw new IllegalArgumentException("Reserved id"); } NotificationChannel existing = r.channels.get(channel.getId()); - if (existing != null) { + if (existing != null && fromTargetApp) { // Actually modifying an existing channel - keep most of the existing settings if (existing.isDeleted()) { // The existing channel was deleted - undelete it. @@ -1002,7 +1002,9 @@ public class PreferencesHelper implements RankingConfig { } if (fromTargetApp) { channel.setLockscreenVisibility(r.visibility); - channel.setAllowBubbles(NotificationChannel.DEFAULT_ALLOW_BUBBLE); + channel.setAllowBubbles(existing != null + ? existing.getAllowBubbles() + : NotificationChannel.DEFAULT_ALLOW_BUBBLE); } clearLockedFieldsLocked(channel); 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 6edd87c0c6cf8..7df4b57b6cf87 100755 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -1101,10 +1101,6 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { new NotificationChannel("id", "name", IMPORTANCE_HIGH); mBinderService.updateNotificationChannelForPackage(PKG, mUid, updatedChannel); - // pretend only this following part is called by the app (system permissions are required to - // update the notification channel on behalf of the user above) - mService.isSystemUid = false; - // Recreating with a lower importance leaves channel unchanged. final NotificationChannel dupeChannel = new NotificationChannel("id", "name", NotificationManager.IMPORTANCE_LOW); @@ -1129,46 +1125,6 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { assertEquals(IMPORTANCE_DEFAULT, createdChannel.getImportance()); } - @Test - public void testCreateNotificationChannels_fromAppCannotSetFields() throws Exception { - // Confirm that when createNotificationChannels is called from the relevant app and not - // system, then it cannot set fields that can't be set by apps - mService.isSystemUid = false; - - final NotificationChannel channel = - new NotificationChannel("id", "name", IMPORTANCE_DEFAULT); - channel.setBypassDnd(true); - channel.setAllowBubbles(true); - - mBinderService.createNotificationChannels(PKG, - new ParceledListSlice(Arrays.asList(channel))); - - final NotificationChannel createdChannel = - mBinderService.getNotificationChannel(PKG, mContext.getUserId(), PKG, "id"); - assertFalse(createdChannel.canBypassDnd()); - assertFalse(createdChannel.canBubble()); - } - - @Test - public void testCreateNotificationChannels_fromSystemCanSetFields() throws Exception { - // Confirm that when createNotificationChannels is called from system, - // then it can set fields that can't be set by apps - mService.isSystemUid = true; - - final NotificationChannel channel = - new NotificationChannel("id", "name", IMPORTANCE_DEFAULT); - channel.setBypassDnd(true); - channel.setAllowBubbles(true); - - mBinderService.createNotificationChannels(PKG, - new ParceledListSlice(Arrays.asList(channel))); - - final NotificationChannel createdChannel = - mBinderService.getNotificationChannel(PKG, mContext.getUserId(), PKG, "id"); - assertTrue(createdChannel.canBypassDnd()); - assertTrue(createdChannel.canBubble()); - } - @Test public void testBlockedNotifications_suspended() throws Exception { when(mPackageManager.isPackageSuspendedForUser(anyString(), anyInt())).thenReturn(true); @@ -3132,8 +3088,6 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { @Test public void testDeleteChannelGroupChecksForFgses() throws Exception { - // the setup for this test requires it to seem like it's coming from the app - mService.isSystemUid = false; when(mCompanionMgr.getAssociations(PKG, UserHandle.getUserId(mUid))) .thenReturn(singletonList(mock(AssociationInfo.class))); CountDownLatch latch = new CountDownLatch(2); @@ -3146,7 +3100,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { ParceledListSlice pls = new ParceledListSlice(ImmutableList.of(notificationChannel)); try { - mBinderService.createNotificationChannels(PKG, pls); + mBinderService.createNotificationChannelsForPackage(PKG, mUid, pls); } catch (RemoteException e) { throw new RuntimeException(e); } @@ -3165,10 +3119,8 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { ParceledListSlice pls = new ParceledListSlice(ImmutableList.of(notificationChannel)); try { - // Because existing channels won't have their groups overwritten when the call - // is from the app, this call won't take the channel out of the group - mBinderService.createNotificationChannels(PKG, pls); - mBinderService.deleteNotificationChannelGroup(PKG, "group"); + mBinderService.createNotificationChannelsForPackage(PKG, mUid, pls); + mBinderService.deleteNotificationChannelGroup(PKG, "group"); } catch (RemoteException e) { throw new RuntimeException(e); } @@ -8614,7 +8566,7 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { assertEquals("friend", friendChannel.getConversationId()); assertEquals(null, original.getConversationId()); assertEquals(original.canShowBadge(), friendChannel.canShowBadge()); - assertEquals(original.canBubble(), friendChannel.canBubble()); // called by system + assertFalse(friendChannel.canBubble()); // can't be modified by app assertFalse(original.getId().equals(friendChannel.getId())); assertNotNull(friendChannel.getId()); }