From dc3727ebd753074ab4aabf9bbe0720999a03dbe6 Mon Sep 17 00:00:00 2001 From: Bernardo Rufino Date: Tue, 20 Apr 2021 09:55:50 +0100 Subject: [PATCH] Exempt the default browser from trampoline restrictions After conversations with Chrome team, it was decided that we would exempt the default browser from trampoline restrictions until T on the grounds that they have to migrate the web ecosystem for a proper long-term solution and couldn't avoid targeting the latest SDK. Their transition plan is on: https://docs.google.com/document/d/1ptX0057LGTGCtTT_a3Gi2_MMyh8Pw-FM8aiRH4IWdVs/edit Test: CTS coming Test: atest com.android.server.notification.RoleObserverTest Bug: 185460438 Change-Id: Ie86060c9f202776cb24bb5a58eb15bb7722d090e --- .../NotificationManagerService.java | 100 +++++++++++-- .../NotificationManagerServiceTest.java | 7 +- .../server/notification/RoleObserverTest.java | 136 +++++++++++++----- 3 files changed, 195 insertions(+), 48 deletions(-) diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index a71722c93d48a..282761ace8dca 100755 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -123,7 +123,7 @@ import static com.android.server.utils.PriorityDump.PRIORITY_ARG_NORMAL; import android.Manifest; import android.Manifest.permission; -import android.annotation.CallbackExecutor; +import android.annotation.MainThread; import android.annotation.NonNull; import android.annotation.Nullable; import android.annotation.RequiresPermission; @@ -492,7 +492,7 @@ public class NotificationManagerService extends SystemService { private DeviceIdleManager mDeviceIdleManager; private IUriGrantsManager mUgm; private UriGrantsManagerInternal mUgmInternal; - private RoleObserver mRoleObserver; + private volatile RoleObserver mRoleObserver; private UserManager mUm; private IPlatformCompat mPlatformCompat; private ShortcutHelper mShortcutHelper; @@ -2639,6 +2639,11 @@ public class NotificationManagerService extends SystemService { @Override public void onBootPhase(int phase) { + onBootPhase(phase, Looper.getMainLooper()); + } + + @VisibleForTesting + void onBootPhase(int phase, Looper mainLooper) { if (phase == SystemService.PHASE_SYSTEM_SERVICES_READY) { // no beeping until we're basically done booting mSystemReady = true; @@ -2648,9 +2653,11 @@ public class NotificationManagerService extends SystemService { mAudioManagerInternal = getLocalService(AudioManagerInternal.class); mWindowManagerInternal = LocalServices.getService(WindowManagerInternal.class); mZenModeHelper.onSystemReady(); - mRoleObserver = new RoleObserver(getContext().getSystemService(RoleManager.class), - mPackageManager, getContext().getMainExecutor()); - mRoleObserver.init(); + RoleObserver roleObserver = new RoleObserver(getContext(), + getContext().getSystemService(RoleManager.class), + mPackageManager, mainLooper); + roleObserver.init(); + mRoleObserver = roleObserver; LauncherApps launcherApps = (LauncherApps) getContext().getSystemService(Context.LAUNCHER_APPS_SERVICE); mShortcutHelper = new ShortcutHelper(launcherApps, mShortcutListener, getLocalService( @@ -10673,26 +10680,40 @@ public class NotificationManagerService extends SystemService { // Role name : user id : list of approved packages private ArrayMap>> mNonBlockableDefaultApps; + /** + * Writes should be pretty rare (only when default browser changes) and reads are done + * during activity start code-path, so we're optimizing for reads. This means this set is + * immutable once written and we'll recreate the set every time there is a role change and + * then assign that new set to the volatile below, so reads can be done without needing to + * hold a lock. Every write is done on the main-thread, so write atomicity is guaranteed. + * + * Didn't use unmodifiable set to enforce immutability to avoid iterating via iterators. + */ + private volatile ArraySet mTrampolineExemptUids = new ArraySet<>(); + private final RoleManager mRm; private final IPackageManager mPm; private final Executor mExecutor; + private final Looper mMainLooper; - RoleObserver(@NonNull RoleManager roleManager, - @NonNull IPackageManager pkgMgr, - @NonNull @CallbackExecutor Executor executor) { + RoleObserver(Context context, @NonNull RoleManager roleManager, + @NonNull IPackageManager pkgMgr, @NonNull Looper mainLooper) { mRm = roleManager; mPm = pkgMgr; - mExecutor = executor; + mExecutor = context.getMainExecutor(); + mMainLooper = mainLooper; } + /** Should be called from the main-thread. */ + @MainThread public void init() { - List users = mUm.getUsers(); + List users = mUm.getUserHandles(/* excludeDying */ true); mNonBlockableDefaultApps = new ArrayMap<>(); for (int i = 0; i < NON_BLOCKABLE_DEFAULT_ROLES.length; i++) { final ArrayMap> userToApprovedList = new ArrayMap<>(); mNonBlockableDefaultApps.put(NON_BLOCKABLE_DEFAULT_ROLES[i], userToApprovedList); for (int j = 0; j < users.size(); j++) { - Integer userId = users.get(j).getUserHandle().getIdentifier(); + Integer userId = users.get(j).getIdentifier(); ArraySet approvedForUserId = new ArraySet<>(mRm.getRoleHoldersAsUser( NON_BLOCKABLE_DEFAULT_ROLES[i], UserHandle.of(userId))); ArraySet> approvedAppUids = new ArraySet<>(); @@ -10703,7 +10724,7 @@ public class NotificationManagerService extends SystemService { mPreferencesHelper.updateDefaultApps(userId, null, approvedAppUids); } } - + updateTrampolineExemptUidsForUsers(users.toArray(new UserHandle[0])); mRm.addOnRoleHoldersChangedListenerAsUser(mExecutor, this, UserHandle.ALL); } @@ -10712,6 +10733,11 @@ public class NotificationManagerService extends SystemService { return mNonBlockableDefaultApps.get(role).get(userId).contains(pkg); } + @VisibleForTesting + public boolean isUidExemptFromTrampolineRestrictions(int uid) { + return mTrampolineExemptUids.contains(uid); + } + /** * Convert the assistant-role holder into settings. The rest of the system uses the * settings. @@ -10721,6 +10747,12 @@ public class NotificationManagerService extends SystemService { */ @Override public void onRoleHoldersChanged(@NonNull String roleName, @NonNull UserHandle user) { + onRoleHoldersChangedForNonBlockableDefaultApps(roleName, user); + onRoleHoldersChangedForTrampolines(roleName, user); + } + + private void onRoleHoldersChangedForNonBlockableDefaultApps(@NonNull String roleName, + @NonNull UserHandle user) { // we only care about a couple of the roles they'll tell us about boolean relevantChange = false; for (int i = 0; i < NON_BLOCKABLE_DEFAULT_ROLES.length; i++) { @@ -10768,6 +10800,41 @@ public class NotificationManagerService extends SystemService { // write of the notification policy xml for this change } + private void onRoleHoldersChangedForTrampolines(@NonNull String roleName, + @NonNull UserHandle user) { + if (!RoleManager.ROLE_BROWSER.equals(roleName)) { + return; + } + updateTrampolineExemptUidsForUsers(user); + } + + private void updateTrampolineExemptUidsForUsers(UserHandle... users) { + Preconditions.checkState(mMainLooper.isCurrentThread()); + ArraySet oldUids = mTrampolineExemptUids; + ArraySet newUids = new ArraySet<>(); + // Add the uids from previous set for the users that we won't update. + for (int i = 0, n = oldUids.size(); i < n; i++) { + int uid = oldUids.valueAt(i); + UserHandle user = UserHandle.of(UserHandle.getUserId(uid)); + if (!ArrayUtils.contains(users, user)) { + newUids.add(uid); + } + } + // Now lookup the new uids for the users that we want to update. + for (int i = 0, n = users.length; i < n; i++) { + UserHandle user = users[i]; + for (String pkg : mRm.getRoleHoldersAsUser(RoleManager.ROLE_BROWSER, user)) { + int uid = getUidForPackage(pkg, user.getIdentifier()); + if (uid != -1) { + newUids.add(uid); + } else { + Slog.e(TAG, "Bad uid (-1) for browser package " + pkg); + } + } + } + mTrampolineExemptUids = newUids; + } + private int getUidForPackage(String pkg, int userId) { try { return mPm.getPackageUid(pkg, MATCH_ALL, userId); @@ -10932,7 +10999,7 @@ public class NotificationManagerService extends SystemService { } String logcatMessage = "Indirect notification activity start (trampoline) from " + packageName; - if (CompatChanges.isChangeEnabled(NOTIFICATION_TRAMPOLINE_BLOCK, uid)) { + if (blockTrampoline(uid)) { // Post toast() call to mHandler to offload PM lookup from the activity start path mHandler.post(() -> toast(packageName, uid)); Slog.e(TAG, logcatMessage + " blocked"); @@ -10943,6 +11010,13 @@ public class NotificationManagerService extends SystemService { } } + private boolean blockTrampoline(int uid) { + if (mRoleObserver != null && mRoleObserver.isUidExemptFromTrampolineRestrictions(uid)) { + return false; + } + return CompatChanges.isChangeEnabled(NOTIFICATION_TRAMPOLINE_BLOCK, uid); + } + @Override public boolean canCloseSystemDialogs(Collection tokens, int uid) { // If the start is allowed via notification, we allow the app to close system dialogs 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 5743decbfe24c..4d28a560bffec 100755 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -141,6 +141,7 @@ import android.os.Binder; import android.os.Build; import android.os.Bundle; import android.os.IBinder; +import android.os.Looper; import android.os.Parcel; import android.os.Process; import android.os.RemoteException; @@ -267,6 +268,8 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { RankingHandler mRankingHandler; @Mock ActivityManagerInternal mAmi; + @Mock + private Looper mMainLooper; @Mock IIntentSender pi1; @@ -514,7 +517,9 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { mAppUsageStats, mock(DevicePolicyManagerInternal.class), mUgm, mUgmInternal, mAppOpsManager, mUm, mHistoryManager, mStatsManager, mock(TelephonyManager.class), mAmi, mToastRateLimiter); - mService.onBootPhase(SystemService.PHASE_SYSTEM_SERVICES_READY); + // Return first true for RoleObserver main-thread check + when(mMainLooper.isCurrentThread()).thenReturn(true).thenReturn(false); + mService.onBootPhase(SystemService.PHASE_SYSTEM_SERVICES_READY, mMainLooper); mService.setAudioManager(mAudioManager); 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 4ce237e3aadcd..27ae46c87b28d 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/RoleObserverTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/RoleObserverTest.java @@ -16,6 +16,7 @@ package com.android.server.notification; +import static android.app.role.RoleManager.ROLE_BROWSER; import static android.app.role.RoleManager.ROLE_DIALER; import static android.app.role.RoleManager.ROLE_EMERGENCY; import static android.content.pm.PackageManager.MATCH_ALL; @@ -23,6 +24,7 @@ import static android.content.pm.PackageManager.MATCH_ALL; import static junit.framework.Assert.assertFalse; import static junit.framework.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.any; import static org.mockito.Mockito.mock; @@ -31,6 +33,8 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static java.util.Arrays.asList; + import android.app.ActivityManager; import android.app.ActivityManagerInternal; import android.app.AppOpsManager; @@ -44,7 +48,6 @@ import android.companion.ICompanionDeviceManager; import android.content.Context; import android.content.pm.IPackageManager; import android.content.pm.PackageManager; -import android.content.pm.UserInfo; import android.os.Looper; import android.os.UserHandle; import android.os.UserManager; @@ -80,7 +83,6 @@ import org.mockito.MockitoAnnotations; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.Executor; @SmallTest @RunWith(AndroidTestingRunner.class) @@ -98,13 +100,13 @@ public class RoleObserverTest extends UiServiceTestCase { @Mock private UserManager mUm; @Mock - private Executor mExecutor; - @Mock private RoleManager mRoleManager; + @Mock + private Looper mMainLooper; NotificationRecordLoggerFake mNotificationRecordLogger = new NotificationRecordLoggerFake(); private InstanceIdSequence mNotificationInstanceIdSequence = new InstanceIdSequenceFake( 1 << 30); - private List mUsers; + private List mUsers; private static class TestableNotificationManagerService extends NotificationManagerService { TestableNotificationManagerService(Context context, @@ -133,13 +135,15 @@ public class RoleObserverTest extends UiServiceTestCase { mContext.addMockSystemService(AppOpsManager.class, mock(AppOpsManager.class)); mUsers = new ArrayList<>(); - mUsers.add(new UserInfo(0, "system", 0)); - mUsers.add(new UserInfo(10, "second", 0)); - when(mUm.getUsers()).thenReturn(mUsers); + mUsers.add(new UserHandle(0)); + mUsers.add(new UserHandle(10)); + when(mUm.getUserHandles(anyBoolean())).thenReturn(mUsers); + + when(mMainLooper.isCurrentThread()).thenReturn(true); mService = new TestableNotificationManagerService(mContext, mNotificationRecordLogger, mNotificationInstanceIdSequence); - mRoleObserver = mService.new RoleObserver(mRoleManager, mPm, mExecutor); + mRoleObserver = mService.new RoleObserver(mContext, mRoleManager, mPm, mMainLooper); try { mService.init(mService.new WorkerHandler(mock(Looper.class)), @@ -174,7 +178,7 @@ public class RoleObserverTest extends UiServiceTestCase { } @Test - public void testInit() throws Exception { + public void testInit_forNonBlockableDefaultApps() throws Exception { List dialer0 = new ArrayList<>(); dialer0.add("dialer"); List emer0 = new ArrayList<>(); @@ -191,29 +195,29 @@ public class RoleObserverTest extends UiServiceTestCase { when(mRoleManager.getRoleHoldersAsUser( ROLE_DIALER, - mUsers.get(0).getUserHandle())). - thenReturn(dialer0); + mUsers.get(0))) + .thenReturn(dialer0); when(mRoleManager.getRoleHoldersAsUser( ROLE_EMERGENCY, - mUsers.get(0).getUserHandle())). - thenReturn(emer0); + mUsers.get(0))) + .thenReturn(emer0); mRoleObserver.init(); // verify internal records of current state of the world assertTrue(mRoleObserver.isApprovedPackageForRoleForUser( - ROLE_DIALER, dialer0.get(0), mUsers.get(0).id)); + ROLE_DIALER, dialer0.get(0), mUsers.get(0).getIdentifier())); assertFalse(mRoleObserver.isApprovedPackageForRoleForUser( - ROLE_DIALER, dialer0.get(0), mUsers.get(1).id)); + ROLE_DIALER, dialer0.get(0), mUsers.get(1).getIdentifier())); assertTrue(mRoleObserver.isApprovedPackageForRoleForUser( - ROLE_EMERGENCY, emer0.get(0), mUsers.get(0).id)); + ROLE_EMERGENCY, emer0.get(0), mUsers.get(0).getIdentifier())); assertFalse(mRoleObserver.isApprovedPackageForRoleForUser( - ROLE_EMERGENCY, emer0.get(0), mUsers.get(1).id)); + ROLE_EMERGENCY, emer0.get(0), mUsers.get(1).getIdentifier())); // make sure we're listening to updates verify(mRoleManager, times(1)).addOnRoleHoldersChangedListenerAsUser( - eq(mExecutor), any(), eq(UserHandle.ALL)); + any(), any(), eq(UserHandle.ALL)); // make sure we told pref helper about the state of the world verify(mPreferencesHelper, times(1)).updateDefaultApps(0, null, dialer0Pair); @@ -221,14 +225,31 @@ public class RoleObserverTest extends UiServiceTestCase { } @Test - public void testSwapDefault() throws Exception { + public void testInit_forTrampolines() throws Exception { + when(mPm.getPackageUid("com.browser", MATCH_ALL, 0)).thenReturn(30); + when(mRoleManager.getRoleHoldersAsUser( + ROLE_BROWSER, + mUsers.get(0))) + .thenReturn(asList("com.browser")); + + mRoleObserver.init(); + + assertTrue(mRoleObserver.isUidExemptFromTrampolineRestrictions(30)); + + // make sure we're listening to updates + verify(mRoleManager, times(1)).addOnRoleHoldersChangedListenerAsUser(any(), any(), + eq(UserHandle.ALL)); + } + + @Test + public void testSwapDefault_forNonBlockableDefaultApps() throws Exception { List dialer0 = new ArrayList<>(); dialer0.add("dialer"); when(mRoleManager.getRoleHoldersAsUser( ROLE_DIALER, - mUsers.get(0).getUserHandle())). - thenReturn(dialer0); + mUsers.get(0))) + .thenReturn(dialer0); mRoleObserver.init(); @@ -241,8 +262,8 @@ public class RoleObserverTest extends UiServiceTestCase { when(mRoleManager.getRoleHoldersAsUser( ROLE_DIALER, - mUsers.get(0).getUserHandle())). - thenReturn(newDefault); + mUsers.get(0))) + .thenReturn(newDefault); mRoleObserver.onRoleHoldersChanged(ROLE_DIALER, UserHandle.of(0)); @@ -251,15 +272,39 @@ public class RoleObserverTest extends UiServiceTestCase { } @Test - public void testSwapDefault_multipleOverlappingApps() throws Exception { + public void testSwapDefault_forTrampolines() throws Exception { + List dialer0 = new ArrayList<>(); + when(mPm.getPackageUid("com.browser", MATCH_ALL, 0)).thenReturn(30); + when(mPm.getPackageUid("com.browser2", MATCH_ALL, 0)).thenReturn(31); + when(mRoleManager.getRoleHoldersAsUser( + ROLE_BROWSER, + mUsers.get(0))) + .thenReturn(asList("com.browser")); + mRoleObserver.init(); + assertTrue(mRoleObserver.isUidExemptFromTrampolineRestrictions(30)); + assertFalse(mRoleObserver.isUidExemptFromTrampolineRestrictions(31)); + // Default changed + when(mRoleManager.getRoleHoldersAsUser( + ROLE_BROWSER, + mUsers.get(0))) + .thenReturn(asList("com.browser2")); + mRoleObserver.onRoleHoldersChanged(ROLE_BROWSER, UserHandle.of(0)); + + assertFalse(mRoleObserver.isUidExemptFromTrampolineRestrictions(30)); + assertTrue(mRoleObserver.isUidExemptFromTrampolineRestrictions(31)); + } + + @Test + public void testSwapDefault_multipleOverlappingApps_forNonBlockableDefaultApps() + throws Exception { List dialer0 = new ArrayList<>(); dialer0.add("dialer"); dialer0.add("phone"); when(mRoleManager.getRoleHoldersAsUser( ROLE_DIALER, - mUsers.get(0).getUserHandle())). - thenReturn(dialer0); + mUsers.get(0))) + .thenReturn(dialer0); mRoleObserver.init(); @@ -273,8 +318,8 @@ public class RoleObserverTest extends UiServiceTestCase { when(mRoleManager.getRoleHoldersAsUser( ROLE_DIALER, - mUsers.get(0).getUserHandle())). - thenReturn(newDefault); + mUsers.get(0))) + .thenReturn(newDefault); ArraySet expectedRemove = new ArraySet<>(); expectedRemove.add("dialer"); @@ -294,14 +339,14 @@ public class RoleObserverTest extends UiServiceTestCase { } @Test - public void testSwapDefault_newUser() throws Exception { + public void testSwapDefault_newUser_forNonBlockableDefaultApps() throws Exception { List dialer0 = new ArrayList<>(); dialer0.add("dialer"); when(mRoleManager.getRoleHoldersAsUser( ROLE_DIALER, - mUsers.get(0).getUserHandle())). - thenReturn(dialer0); + mUsers.get(0))) + .thenReturn(dialer0); mRoleObserver.init(); @@ -310,8 +355,8 @@ public class RoleObserverTest extends UiServiceTestCase { when(mRoleManager.getRoleHoldersAsUser( ROLE_DIALER, - mUsers.get(1).getUserHandle())). - thenReturn(dialer10); + mUsers.get(1))) + .thenReturn(dialer10); ArraySet> expectedAddPair = new ArraySet<>(); expectedAddPair.add(new Pair("phone", 30)); @@ -329,4 +374,27 @@ public class RoleObserverTest extends UiServiceTestCase { assertTrue(mRoleObserver.isApprovedPackageForRoleForUser(ROLE_DIALER, "phone", 10)); assertTrue(mRoleObserver.isApprovedPackageForRoleForUser(ROLE_DIALER, "dialer", 0)); } + + @Test + public void testSwapDefault_newUser_forTrampolines() throws Exception { + List dialer0 = new ArrayList<>(); + when(mPm.getPackageUid("com.browser", MATCH_ALL, 0)).thenReturn(30); + when(mPm.getPackageUid("com.browser2", MATCH_ALL, 10)).thenReturn(1031); + when(mRoleManager.getRoleHoldersAsUser( + ROLE_BROWSER, + mUsers.get(0))) + .thenReturn(asList("com.browser")); + mRoleObserver.init(); + assertTrue(mRoleObserver.isUidExemptFromTrampolineRestrictions(30)); + assertFalse(mRoleObserver.isUidExemptFromTrampolineRestrictions(1031)); + // New user + when(mRoleManager.getRoleHoldersAsUser( + ROLE_BROWSER, + mUsers.get(1))) + .thenReturn(asList("com.browser2")); + mRoleObserver.onRoleHoldersChanged(ROLE_BROWSER, UserHandle.of(10)); + + assertTrue(mRoleObserver.isUidExemptFromTrampolineRestrictions(30)); + assertTrue(mRoleObserver.isUidExemptFromTrampolineRestrictions(1031)); + } }