From 34e85c9613bcb84f39584eb4a468106cf20dfea4 Mon Sep 17 00:00:00 2001 From: Fabian Kozynski Date: Wed, 18 Dec 2019 09:54:09 -0500 Subject: [PATCH] Prevent sending early termination of appop use If a package starts a particular appop both as active and noted, once one of them is finished (usually noted after 5s), a message will be sent to callbacks indicating the end of the use. However, the app op may still be active. This could result in the removal of indicators prematurely from notifications. This change prevents that from happening by checking if the app op is still in use by that combination uid/package (either active or noted) and not notifying listeners if that's the case. Also removed user sensitive checks. Test: atest AppOpsControllerTest Test: use app from bug report. Observe that notification does not lose the microphone indicator Bug: 144092031 Change-Id: I180e7c257e6171e7686ba7eda9bf02249358ed00 --- .../systemui/appops/AppOpsControllerImpl.java | 117 ++++++----------- .../systemui/appops/PermissionFlagsCache.kt | 70 ---------- .../systemui/appops/AppOpsControllerTest.java | 123 ++++++++++++++---- .../appops/PermissionFlagsCacheTest.kt | 88 ------------- 4 files changed, 142 insertions(+), 256 deletions(-) delete mode 100644 packages/SystemUI/src/com/android/systemui/appops/PermissionFlagsCache.kt delete mode 100644 packages/SystemUI/tests/src/com/android/systemui/appops/PermissionFlagsCacheTest.kt diff --git a/packages/SystemUI/src/com/android/systemui/appops/AppOpsControllerImpl.java b/packages/SystemUI/src/com/android/systemui/appops/AppOpsControllerImpl.java index b0831232bcd4b..23d6458a30a5f 100644 --- a/packages/SystemUI/src/com/android/systemui/appops/AppOpsControllerImpl.java +++ b/packages/SystemUI/src/com/android/systemui/appops/AppOpsControllerImpl.java @@ -18,7 +18,6 @@ package com.android.systemui.appops; import android.app.AppOpsManager; import android.content.Context; -import android.content.pm.PackageManager; import android.os.Handler; import android.os.Looper; import android.os.UserHandle; @@ -64,7 +63,6 @@ public class AppOpsControllerImpl implements AppOpsController, private H mBGHandler; private final List mCallbacks = new ArrayList<>(); private final ArrayMap> mCallbacksByCode = new ArrayMap<>(); - private final PermissionFlagsCache mFlagsCache; private boolean mListening; @GuardedBy("mActiveItems") @@ -81,17 +79,10 @@ public class AppOpsControllerImpl implements AppOpsController, }; @Inject - public AppOpsControllerImpl(Context context, @Background Looper bgLooper, - DumpController dumpController) { - this(context, bgLooper, new PermissionFlagsCache(context), dumpController); - } - - @VisibleForTesting - protected AppOpsControllerImpl(Context context, Looper bgLooper, PermissionFlagsCache cache, - DumpController dumpController) { + public AppOpsControllerImpl(Context context, + @Background Looper bgLooper, DumpController dumpController) { mContext = context; mAppOps = (AppOpsManager) context.getSystemService(Context.APP_OPS_SERVICE); - mFlagsCache = cache; mBGHandler = new H(bgLooper); final int numOps = OPS.length; for (int i = 0; i < numOps; i++) { @@ -209,7 +200,14 @@ public class AppOpsControllerImpl implements AppOpsController, mNotedItems.remove(item); if (DEBUG) Log.w(TAG, "Removed item: " + item.toString()); } - notifySuscribers(code, uid, packageName, false); + boolean active; + // Check if the item is also active + synchronized (mActiveItems) { + active = getAppOpItemLocked(mActiveItems, code, uid, packageName) != null; + } + if (!active) { + notifySuscribers(code, uid, packageName, false); + } } private boolean addNoted(int code, int uid, String packageName) { @@ -224,63 +222,12 @@ public class AppOpsControllerImpl implements AppOpsController, createdNew = true; } } + // We should keep this so we make sure it cannot time out. + mBGHandler.removeCallbacksAndMessages(item); mBGHandler.scheduleRemoval(item, NOTED_OP_TIME_DELAY_MS); return createdNew; } - /** - * Does the app-op code refer to a user sensitive permission for the specified user id - * and package. Only user sensitive permission should be shown to the user by default. - * - * @param appOpCode The code of the app-op. - * @param uid The uid of the user. - * @param packageName The name of the package. - * - * @return {@code true} iff the app-op item is user sensitive - */ - private boolean isUserSensitive(int appOpCode, int uid, String packageName) { - String permission = AppOpsManager.opToPermission(appOpCode); - if (permission == null) { - return false; - } - int permFlags = mFlagsCache.getPermissionFlags(permission, - packageName, UserHandle.getUserHandleForUid(uid)); - return (permFlags & PackageManager.FLAG_PERMISSION_USER_SENSITIVE_WHEN_GRANTED) != 0; - } - - /** - * Does the app-op item refer to an operation that should be shown to the user. - * Only specficic ops (like SYSTEM_ALERT_WINDOW) or ops that refer to user sensitive - * permission should be shown to the user by default. - * - * @param item The item - * - * @return {@code true} iff the app-op item should be shown to the user - */ - private boolean isUserVisible(AppOpItem item) { - return isUserVisible(item.getCode(), item.getUid(), item.getPackageName()); - } - - - /** - * Does the app-op, uid and package name, refer to an operation that should be shown to the - * user. Only specficic ops (like {@link AppOpsManager.OP_SYSTEM_ALERT_WINDOW}) or - * ops that refer to user sensitive permission should be shown to the user by default. - * - * @param item The item - * - * @return {@code true} iff the app-op for should be shown to the user - */ - private boolean isUserVisible(int appOpCode, int uid, String packageName) { - // currently OP_SYSTEM_ALERT_WINDOW does not correspond to a platform permission - // which may be user senstive, so for now always show it to the user. - if (appOpCode == AppOpsManager.OP_SYSTEM_ALERT_WINDOW) { - return true; - } - - return isUserSensitive(appOpCode, uid, packageName); - } - /** * Returns a copy of the list containing all the active AppOps that the controller tracks. * @@ -304,8 +251,8 @@ public class AppOpsControllerImpl implements AppOpsController, final int numActiveItems = mActiveItems.size(); for (int i = 0; i < numActiveItems; i++) { AppOpItem item = mActiveItems.get(i); - if ((userId == UserHandle.USER_ALL || UserHandle.getUserId(item.getUid()) == userId) - && isUserVisible(item)) { + if ((userId == UserHandle.USER_ALL + || UserHandle.getUserId(item.getUid()) == userId)) { list.add(item); } } @@ -314,8 +261,8 @@ public class AppOpsControllerImpl implements AppOpsController, final int numNotedItems = mNotedItems.size(); for (int i = 0; i < numNotedItems; i++) { AppOpItem item = mNotedItems.get(i); - if ((userId == UserHandle.USER_ALL || UserHandle.getUserId(item.getUid()) == userId) - && isUserVisible(item)) { + if ((userId == UserHandle.USER_ALL + || UserHandle.getUserId(item.getUid()) == userId)) { list.add(item); } } @@ -325,7 +272,21 @@ public class AppOpsControllerImpl implements AppOpsController, @Override public void onOpActiveChanged(int code, int uid, String packageName, boolean active) { - if (updateActives(code, uid, packageName, active)) { + if (DEBUG) { + Log.w(TAG, String.format("onActiveChanged(%d,%d,%s,%s", code, uid, packageName, + Boolean.toString(active))); + } + boolean activeChanged = updateActives(code, uid, packageName, active); + if (!activeChanged) return; // early return + // Check if the item is also noted, in that case, there's no update. + boolean alsoNoted; + synchronized (mNotedItems) { + alsoNoted = getAppOpItemLocked(mNotedItems, code, uid, packageName) != null; + } + // If active is true, we only send the update if the op is not actively noted (already true) + // If active is false, we only send the update if the op is not actively noted (prevent + // early removal) + if (!alsoNoted) { mBGHandler.post(() -> notifySuscribers(code, uid, packageName, active)); } } @@ -333,17 +294,23 @@ public class AppOpsControllerImpl implements AppOpsController, @Override public void onOpNoted(int code, int uid, String packageName, int result) { if (DEBUG) { - Log.w(TAG, "Op: " + code + " with result " + AppOpsManager.MODE_NAMES[result]); + Log.w(TAG, "Noted op: " + code + " with result " + + AppOpsManager.MODE_NAMES[result] + " for package " + packageName); } if (result != AppOpsManager.MODE_ALLOWED) return; - if (addNoted(code, uid, packageName)) { + boolean notedAdded = addNoted(code, uid, packageName); + if (!notedAdded) return; // early return + boolean alsoActive; + synchronized (mActiveItems) { + alsoActive = getAppOpItemLocked(mActiveItems, code, uid, packageName) != null; + } + if (!alsoActive) { mBGHandler.post(() -> notifySuscribers(code, uid, packageName, true)); } } private void notifySuscribers(int code, int uid, String packageName, boolean active) { - if (mCallbacksByCode.containsKey(code) - && isUserVisible(code, uid, packageName)) { + if (mCallbacksByCode.containsKey(code)) { if (DEBUG) Log.d(TAG, "Notifying of change in package " + packageName); for (Callback cb: mCallbacksByCode.get(code)) { cb.onActiveStateChanged(code, uid, packageName, active); @@ -368,7 +335,7 @@ public class AppOpsControllerImpl implements AppOpsController, } - protected final class H extends Handler { + protected class H extends Handler { H(Looper looper) { super(looper); } diff --git a/packages/SystemUI/src/com/android/systemui/appops/PermissionFlagsCache.kt b/packages/SystemUI/src/com/android/systemui/appops/PermissionFlagsCache.kt deleted file mode 100644 index f02c7afdb9ca6..0000000000000 --- a/packages/SystemUI/src/com/android/systemui/appops/PermissionFlagsCache.kt +++ /dev/null @@ -1,70 +0,0 @@ -/* - * Copyright (C) 2019 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License - */ - -package com.android.systemui.appops - -import android.content.Context -import android.content.pm.PackageManager -import android.os.UserHandle -import android.util.ArrayMap -import com.android.internal.annotations.VisibleForTesting - -private data class PermissionFlag(val flag: Int, val timestamp: Long) - -private data class PermissionFlagKey( - val permission: String, - val packageName: String, - val user: UserHandle -) - -internal const val CACHE_EXPIRATION = 10000L - -/** - * Cache for PackageManager's PermissionFlags. - * - * Flags older than [CACHE_EXPIRATION] will be retrieved again. - */ -internal open class PermissionFlagsCache(context: Context) { - private val packageManager = context.packageManager - private val permissionFlagsCache = ArrayMap() - - /** - * Retrieve permission flags from cache or PackageManager. There parameters will be passed - * directly to [PackageManager]. - * - * Calls to this method should be done from a background thread. - */ - fun getPermissionFlags(permission: String, packageName: String, user: UserHandle): Int { - val key = PermissionFlagKey(permission, packageName, user) - val now = getCurrentTime() - val value = permissionFlagsCache.getOrPut(key) { - PermissionFlag(getFlags(key), now) - } - if (now - value.timestamp > CACHE_EXPIRATION) { - val newValue = PermissionFlag(getFlags(key), now) - permissionFlagsCache.put(key, newValue) - return newValue.flag - } else { - return value.flag - } - } - - private fun getFlags(key: PermissionFlagKey) = - packageManager.getPermissionFlags(key.permission, key.packageName, key.user) - - @VisibleForTesting - protected open fun getCurrentTime() = System.currentTimeMillis() -} \ No newline at end of file diff --git a/packages/SystemUI/tests/src/com/android/systemui/appops/AppOpsControllerTest.java b/packages/SystemUI/tests/src/com/android/systemui/appops/AppOpsControllerTest.java index af0ef8298a3fa..e5ae1aacc7717 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/appops/AppOpsControllerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/appops/AppOpsControllerTest.java @@ -25,11 +25,11 @@ import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; + +import static java.lang.Thread.sleep; import android.app.AppOpsManager; import android.content.pm.PackageManager; @@ -57,7 +57,6 @@ public class AppOpsControllerTest extends SysuiTestCase { private static final String TEST_PACKAGE_NAME = "test"; private static final int TEST_UID = UserHandle.getUid(0, 0); private static final int TEST_UID_OTHER = UserHandle.getUid(1, 0); - private static final int TEST_UID_NON_USER_SENSITIVE = UserHandle.getUid(2, 0); @Mock private AppOpsManager mAppOpsManager; @@ -68,8 +67,6 @@ public class AppOpsControllerTest extends SysuiTestCase { @Mock private AppOpsControllerImpl.H mMockHandler; @Mock - private PermissionFlagsCache mFlagsCache; - @Mock private DumpController mDumpController; private AppOpsControllerImpl mController; @@ -85,17 +82,9 @@ public class AppOpsControllerTest extends SysuiTestCase { // All permissions of TEST_UID and TEST_UID_OTHER are user sensitive. None of // TEST_UID_NON_USER_SENSITIVE are user sensitive. getContext().setMockPackageManager(mPackageManager); - when(mFlagsCache.getPermissionFlags(anyString(), anyString(), - eq(UserHandle.getUserHandleForUid(TEST_UID)))).thenReturn( - PackageManager.FLAG_PERMISSION_USER_SENSITIVE_WHEN_GRANTED); - when(mFlagsCache.getPermissionFlags(anyString(), anyString(), - eq(UserHandle.getUserHandleForUid(TEST_UID_OTHER)))).thenReturn( - PackageManager.FLAG_PERMISSION_USER_SENSITIVE_WHEN_GRANTED); - when(mFlagsCache.getPermissionFlags(anyString(), anyString(), - eq(UserHandle.getUserHandleForUid(TEST_UID_NON_USER_SENSITIVE)))).thenReturn(0); - mController = new AppOpsControllerImpl(mContext, mTestableLooper.getLooper(), mFlagsCache, - mDumpController); + mController = + new AppOpsControllerImpl(mContext, mTestableLooper.getLooper(), mDumpController); } @Test @@ -190,14 +179,6 @@ public class AppOpsControllerTest extends SysuiTestCase { mController.getActiveAppOpsForUser(UserHandle.getUserId(TEST_UID_OTHER)).size()); } - @Test - public void nonUserSensitiveOpsAreIgnored() { - mController.onOpActiveChanged(AppOpsManager.OP_RECORD_AUDIO, - TEST_UID_NON_USER_SENSITIVE, TEST_PACKAGE_NAME, true); - assertEquals(0, mController.getActiveAppOpsForUser( - UserHandle.getUserId(TEST_UID_NON_USER_SENSITIVE)).size()); - } - @Test public void opNotedScheduledForRemoval() { mController.setBGHandler(mMockHandler); @@ -251,4 +232,100 @@ public class AppOpsControllerTest extends SysuiTestCase { // Only one post to notify subscribers verify(mMockHandler, times(2)).scheduleRemoval(any(), anyLong()); } + + @Test + public void testActiveOpNotRemovedAfterNoted() throws InterruptedException { + // Replaces the timeout delay with 5 ms + AppOpsControllerImpl.H testHandler = mController.new H(mTestableLooper.getLooper()) { + @Override + public void scheduleRemoval(AppOpItem item, long timeToRemoval) { + super.scheduleRemoval(item, 5L); + } + }; + + mController.addCallback(new int[]{AppOpsManager.OP_FINE_LOCATION}, mCallback); + mController.setBGHandler(testHandler); + + mController.onOpActiveChanged( + AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true); + + mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, + AppOpsManager.MODE_ALLOWED); + + mTestableLooper.processAllMessages(); + List list = mController.getActiveAppOps(); + verify(mCallback).onActiveStateChanged( + AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true); + + // Duplicates are not removed between active and noted + assertEquals(2, list.size()); + + sleep(10L); + + mTestableLooper.processAllMessages(); + + verify(mCallback, never()).onActiveStateChanged( + AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, false); + list = mController.getActiveAppOps(); + assertEquals(1, list.size()); + } + + @Test + public void testNotedNotRemovedAfterActive() { + mController.addCallback(new int[]{AppOpsManager.OP_FINE_LOCATION}, mCallback); + + mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, + AppOpsManager.MODE_ALLOWED); + + mController.onOpActiveChanged( + AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true); + + mTestableLooper.processAllMessages(); + List list = mController.getActiveAppOps(); + verify(mCallback).onActiveStateChanged( + AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true); + + // Duplicates are not removed between active and noted + assertEquals(2, list.size()); + + mController.onOpActiveChanged( + AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, false); + + mTestableLooper.processAllMessages(); + + verify(mCallback, never()).onActiveStateChanged( + AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, false); + list = mController.getActiveAppOps(); + assertEquals(1, list.size()); + } + + @Test + public void testNotedAndActiveOnlyOneCall() { + mController.addCallback(new int[]{AppOpsManager.OP_FINE_LOCATION}, mCallback); + + mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, + AppOpsManager.MODE_ALLOWED); + + mController.onOpActiveChanged( + AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true); + + mTestableLooper.processAllMessages(); + verify(mCallback).onActiveStateChanged( + AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true); + } + + @Test + public void testActiveAndNotedOnlyOneCall() { + mController.addCallback(new int[]{AppOpsManager.OP_FINE_LOCATION}, mCallback); + + mController.onOpActiveChanged( + AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true); + + mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, + AppOpsManager.MODE_ALLOWED); + + mTestableLooper.processAllMessages(); + verify(mCallback).onActiveStateChanged( + AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME, true); + } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/appops/PermissionFlagsCacheTest.kt b/packages/SystemUI/tests/src/com/android/systemui/appops/PermissionFlagsCacheTest.kt deleted file mode 100644 index dc070dea6d898..0000000000000 --- a/packages/SystemUI/tests/src/com/android/systemui/appops/PermissionFlagsCacheTest.kt +++ /dev/null @@ -1,88 +0,0 @@ -/* - * Copyright (C) 2019 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License - */ - -package com.android.systemui.appops - -import android.content.Context -import android.content.pm.PackageManager -import android.os.UserHandle -import android.testing.AndroidTestingRunner -import androidx.test.filters.SmallTest -import com.android.systemui.SysuiTestCase -import org.junit.Before -import org.junit.Test -import org.junit.runner.RunWith -import org.mockito.ArgumentMatchers.any -import org.mockito.ArgumentMatchers.anyString -import org.mockito.Mock -import org.mockito.Mockito.times -import org.mockito.Mockito.verify -import org.mockito.MockitoAnnotations - -@SmallTest -@RunWith(AndroidTestingRunner::class) -class PermissionFlagsCacheTest : SysuiTestCase() { - - companion object { - const val TEST_PERMISSION = "test_permission" - const val TEST_PACKAGE = "test_package" - } - - @Mock - private lateinit var mPackageManager: PackageManager - @Mock - private lateinit var mUserHandle: UserHandle - private lateinit var flagsCache: TestPermissionFlagsCache - - @Before - fun setUp() { - MockitoAnnotations.initMocks(this) - mContext.setMockPackageManager(mPackageManager) - flagsCache = TestPermissionFlagsCache(mContext) - } - - @Test - fun testCallsPackageManager_exactlyOnce() { - flagsCache.getPermissionFlags(TEST_PERMISSION, TEST_PACKAGE, mUserHandle) - flagsCache.time = CACHE_EXPIRATION - 1 - verify(mPackageManager).getPermissionFlags(TEST_PERMISSION, TEST_PACKAGE, mUserHandle) - } - - @Test - fun testCallsPackageManager_cacheExpired() { - flagsCache.getPermissionFlags(TEST_PERMISSION, TEST_PACKAGE, mUserHandle) - flagsCache.time = CACHE_EXPIRATION + 1 - flagsCache.getPermissionFlags(TEST_PERMISSION, TEST_PACKAGE, mUserHandle) - verify(mPackageManager, times(2)) - .getPermissionFlags(TEST_PERMISSION, TEST_PACKAGE, mUserHandle) - } - - @Test - fun testCallsPackageMaanger_multipleKeys() { - flagsCache.getPermissionFlags(TEST_PERMISSION, TEST_PACKAGE, mUserHandle) - flagsCache.getPermissionFlags(TEST_PERMISSION, "", mUserHandle) - verify(mPackageManager, times(2)) - .getPermissionFlags(anyString(), anyString(), any()) - } - - private class TestPermissionFlagsCache(context: Context) : PermissionFlagsCache(context) { - var time = 0L - - override fun getCurrentTime(): Long { - return time - } - } -} \ No newline at end of file