Merge "Prevent sending early termination of appop use"
diff --git a/packages/SystemUI/src/com/android/systemui/appops/AppOpsControllerImpl.java b/packages/SystemUI/src/com/android/systemui/appops/AppOpsControllerImpl.java
index b083123..23d6458 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 @@
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 @@
private H mBGHandler;
private final List<AppOpsController.Callback> mCallbacks = new ArrayList<>();
private final ArrayMap<Integer, Set<Callback>> mCallbacksByCode = new ArrayMap<>();
- private final PermissionFlagsCache mFlagsCache;
private boolean mListening;
@GuardedBy("mActiveItems")
@@ -81,17 +79,10 @@
};
@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 @@
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,64 +222,13 @@
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.
*
* @return List of active AppOps information
@@ -304,8 +251,8 @@
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 @@
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 @@
@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 @@
@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 @@
}
- 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 f02c7af..0000000
--- 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<PermissionFlagKey, PermissionFlag>()
-
- /**
- * 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 af0ef82..e5ae1aa 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.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 @@
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 @@
@Mock
private AppOpsControllerImpl.H mMockHandler;
@Mock
- private PermissionFlagsCache mFlagsCache;
- @Mock
private DumpController mDumpController;
private AppOpsControllerImpl mController;
@@ -85,17 +82,9 @@
// 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
@@ -191,14 +180,6 @@
}
@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);
mController.onOpNoted(AppOpsManager.OP_FINE_LOCATION, TEST_UID, TEST_PACKAGE_NAME,
@@ -251,4 +232,100 @@
// 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<AppOpItem> 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<AppOpItem> 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 dc070de..0000000
--- 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