Only kill when needed when changing permissions
Only kill app when an app-op actually changed.
Do not call revokePermission if permission is already revoked. Even
when the permission is alrady revoked, calling this method will kill the
app the permission belongs to.
Also, apply all device policy state and then perist the state for the
whole app at one. Before we persisted after every individual permission
which caused permissions to be temporarily revoked and then re-granted.
Test: Set up device with device-owner, set up work-profile
Fixes: 126185337, 126326421
Change-Id: Id28caebd36d0c66e83ee6ab4299f0ccb3bc6b488
(cherry picked from commit 7380bf8d3f680f32d027a7106e454e9134555edb)
diff --git a/src/com/android/packageinstaller/permission/model/AppPermissionGroup.java b/src/com/android/packageinstaller/permission/model/AppPermissionGroup.java
index f26d657..b6a447c 100644
--- a/src/com/android/packageinstaller/permission/model/AppPermissionGroup.java
+++ b/src/com/android/packageinstaller/permission/model/AppPermissionGroup.java
@@ -26,7 +26,6 @@
import static android.app.AppOpsManager.MODE_ALLOWED;
import static android.app.AppOpsManager.MODE_FOREGROUND;
import static android.app.AppOpsManager.MODE_IGNORED;
-import static android.content.pm.PackageManager.FLAG_PERMISSION_SYSTEM_FIXED;
import static android.content.pm.PackageManager.PERMISSION_GRANTED;
import android.app.ActivityManager;
@@ -629,6 +628,25 @@
}
/**
+ * Set mode of an app-op if needed.
+ *
+ * @param op The op to set
+ * @param uid The uid the app-op belongs top
+ * @param mode The new mode
+ *
+ * @return {@code true} iff app-op was changed
+ */
+ private boolean setAppOpMode(@NonNull String op, int uid, int mode) {
+ int currentMode = mAppOps.unsafeCheckOpRaw(op, uid, mPackageInfo.packageName);
+ if (currentMode == mode) {
+ return false;
+ }
+
+ mAppOps.setUidMode(op, uid, mode);
+ return true;
+ }
+
+ /**
* Allow the app op for a permission/uid.
*
* <p>There are three cases:
@@ -649,8 +667,12 @@
*
* @param permission The permission which has an appOps that should be allowed
* @param uid The uid of the process the app op if for
+ *
+ * @return {@code true} iff app-op was changed
*/
- private void allowAppOp(Permission permission, int uid) {
+ private boolean allowAppOp(Permission permission, int uid) {
+ boolean wasChanged = false;
+
if (permission.isBackgroundPermission()) {
ArrayList<Permission> foregroundPermissions = permission.getForegroundPermissions();
@@ -658,7 +680,7 @@
for (int i = 0; i < numForegroundPermissions; i++) {
Permission foregroundPermission = foregroundPermissions.get(i);
if (foregroundPermission.isAppOpAllowed()) {
- mAppOps.setUidMode(foregroundPermission.getAppOp(), uid, MODE_ALLOWED);
+ wasChanged |= setAppOpMode(foregroundPermission.getAppOp(), uid, MODE_ALLOWED);
}
}
} else {
@@ -669,18 +691,20 @@
// The app requested a permission that has a background permission but it did
// not request the background permission, hence it can never get background
// access
- mAppOps.setUidMode(permission.getAppOp(), uid, MODE_FOREGROUND);
+ wasChanged = setAppOpMode(permission.getAppOp(), uid, MODE_FOREGROUND);
} else {
if (backgroundPermission.isAppOpAllowed()) {
- mAppOps.setUidMode(permission.getAppOp(), uid, MODE_ALLOWED);
+ wasChanged = setAppOpMode(permission.getAppOp(), uid, MODE_ALLOWED);
} else {
- mAppOps.setUidMode(permission.getAppOp(), uid, MODE_FOREGROUND);
+ wasChanged = setAppOpMode(permission.getAppOp(), uid, MODE_FOREGROUND);
}
}
} else {
- mAppOps.setUidMode(permission.getAppOp(), uid, MODE_ALLOWED);
+ wasChanged = setAppOpMode(permission.getAppOp(), uid, MODE_ALLOWED);
}
}
+
+ return wasChanged;
}
/**
@@ -846,8 +870,12 @@
*
* @param permission The permission which has an appOps that should be disallowed
* @param uid The uid of the process the app op if for
+ *
+ * @return {@code true} iff app-op was changed
*/
- private void disallowAppOp(Permission permission, int uid) {
+ private boolean disallowAppOp(Permission permission, int uid) {
+ boolean wasChanged = false;
+
if (permission.isBackgroundPermission()) {
ArrayList<Permission> foregroundPermissions = permission.getForegroundPermissions();
@@ -855,12 +883,15 @@
for (int i = 0; i < numForegroundPermissions; i++) {
Permission foregroundPermission = foregroundPermissions.get(i);
if (foregroundPermission.isAppOpAllowed()) {
- mAppOps.setUidMode(foregroundPermission.getAppOp(), uid, MODE_FOREGROUND);
+ wasChanged |= setAppOpMode(foregroundPermission.getAppOp(), uid,
+ MODE_FOREGROUND);
}
}
} else {
- mAppOps.setUidMode(permission.getAppOp(), uid, MODE_IGNORED);
+ wasChanged = setAppOpMode(permission.getAppOp(), uid, MODE_IGNORED);
}
+
+ return wasChanged;
}
/**
@@ -1134,7 +1165,9 @@
* app ops change. If this is set to {@code false} the
* caller has to make sure to kill the app if needed.
*/
- public void persistChanges(boolean mayKillBecauseOfAppOpsChange) {
+ void persistChanges(boolean mayKillBecauseOfAppOpsChange) {
+ int uid = mPackageInfo.applicationInfo.uid;
+
int numPermissions = mPermissions.size();
boolean shouldKillApp = false;
boolean shouldUpdateStorage = false;
@@ -1147,8 +1180,13 @@
mPackageManager.grantRuntimePermission(mPackageInfo.packageName,
permission.getName(), mUserHandle);
} else {
- mPackageManager.revokeRuntimePermission(mPackageInfo.packageName,
- permission.getName(), mUserHandle);
+ boolean isCurrentlyGranted = mContext.checkPermission(permission.getName(), -1,
+ uid) == PERMISSION_GRANTED;
+
+ if (isCurrentlyGranted) {
+ mPackageManager.revokeRuntimePermission(mPackageInfo.packageName,
+ permission.getName(), mUserHandle);
+ }
}
}
@@ -1171,16 +1209,14 @@
if (permission.affectsAppOp()) {
if (!permission.isSystemFixed()) {
- if (permission.isAppOpAllowed()) {
- allowAppOp(permission, mPackageInfo.applicationInfo.uid);
- } else {
- disallowAppOp(permission, mPackageInfo.applicationInfo.uid);
- }
-
- // Enabling/Disabling an app op may put the app in a situation in which it has a
- // handle to state it shouldn't have, so we have to kill the app. This matches
+ // Enabling/Disabling an app op may put the app in a situation in which it has
+ // a handle to state it shouldn't have, so we have to kill the app. This matches
// the revoke runtime permission behavior.
- shouldKillApp = true;
+ if (permission.isAppOpAllowed()) {
+ shouldKillApp |= allowAppOp(permission, uid);
+ } else {
+ shouldKillApp |= disallowAppOp(permission, uid);
+ }
}
}
@@ -1201,12 +1237,12 @@
// case, whenever any of the new split permissions are granted to an
// app, we also grant them the legacy "Storage" permission.
if (StorageManager.hasIsolatedStorage() && shouldUpdateStorage) {
- boolean audioGranted = mPackageManager.checkPermission(READ_MEDIA_AUDIO,
- mPackageInfo.packageName) == PERMISSION_GRANTED;
- boolean videoGranted = mPackageManager.checkPermission(READ_MEDIA_VIDEO,
- mPackageInfo.packageName) == PERMISSION_GRANTED;
- boolean imagesGranted = mPackageManager.checkPermission(READ_MEDIA_IMAGES,
- mPackageInfo.packageName) == PERMISSION_GRANTED;
+ boolean audioGranted = mContext.checkPermission(READ_MEDIA_AUDIO,
+ -1, uid) == PERMISSION_GRANTED;
+ boolean videoGranted = mContext.checkPermission(READ_MEDIA_VIDEO,
+ -1, uid) == PERMISSION_GRANTED;
+ boolean imagesGranted = mContext.checkPermission(READ_MEDIA_IMAGES,
+ -1, uid) == PERMISSION_GRANTED;
if (!ArrayUtils.isEmpty(mPackageInfo.requestedPermissions)) {
for (String permission : mPackageInfo.requestedPermissions) {
@@ -1216,8 +1252,13 @@
mPackageManager.grantRuntimePermission(mPackageInfo.packageName,
permission, mUserHandle);
} else {
- mPackageManager.revokeRuntimePermission(mPackageInfo.packageName,
- permission, mUserHandle);
+ boolean isCurrentlyGranted = mContext.checkPermission(permission, -1,
+ uid) == PERMISSION_GRANTED;
+
+ if (isCurrentlyGranted) {
+ mPackageManager.revokeRuntimePermission(mPackageInfo.packageName,
+ permission, mUserHandle);
+ }
}
}
}
diff --git a/src/com/android/packageinstaller/permission/model/AppPermissions.java b/src/com/android/packageinstaller/permission/model/AppPermissions.java
index 125074f..93e517a 100644
--- a/src/com/android/packageinstaller/permission/model/AppPermissions.java
+++ b/src/com/android/packageinstaller/permission/model/AppPermissions.java
@@ -179,18 +179,22 @@
/**
* If the changes to the permission groups were delayed, persist them now.
+ *
+ * @param mayKillBecauseOfAppOpsChange If the app may be killed if app ops change. If this is
+ * set to {@code false} the caller has to make sure to kill
+ * the app if needed.
*/
- public void persistChanges() {
+ public void persistChanges(boolean mayKillBecauseOfAppOpsChange) {
if (mDelayChanges) {
int numGroups = mGroups.size();
for (int i = 0; i < numGroups; i++) {
AppPermissionGroup group = mGroups.get(i);
- group.persistChanges(true);
+ group.persistChanges(mayKillBecauseOfAppOpsChange);
AppPermissionGroup backgroundGroup = group.getBackgroundPermissions();
if (backgroundGroup != null) {
- backgroundGroup.persistChanges(true);
+ backgroundGroup.persistChanges(mayKillBecauseOfAppOpsChange);
}
}
}
diff --git a/src/com/android/packageinstaller/permission/service/BackupHelper.java b/src/com/android/packageinstaller/permission/service/BackupHelper.java
index ab04716..d55d3f9 100644
--- a/src/com/android/packageinstaller/permission/service/BackupHelper.java
+++ b/src/com/android/packageinstaller/permission/service/BackupHelper.java
@@ -17,7 +17,6 @@
package com.android.packageinstaller.permission.service;
import static android.content.Context.MODE_PRIVATE;
-import static android.content.pm.PackageManager.FLAG_PERMISSION_GRANTED_BY_DEFAULT;
import static android.content.pm.PackageManager.FLAG_PERMISSION_POLICY_FIXED;
import static android.content.pm.PackageManager.FLAG_PERMISSION_SYSTEM_FIXED;
import static android.content.pm.PackageManager.GET_PERMISSIONS;
@@ -53,7 +52,6 @@
import org.xmlpull.v1.XmlPullParserException;
import org.xmlpull.v1.XmlSerializer;
-import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.OutputStream;
@@ -750,7 +748,7 @@
}
}
- appPerms.persistChanges();
+ appPerms.persistChanges(true);
}
}
}
diff --git a/src/com/android/packageinstaller/permission/service/PermissionControllerServiceImpl.java b/src/com/android/packageinstaller/permission/service/PermissionControllerServiceImpl.java
index f04634c..09539d6 100644
--- a/src/com/android/packageinstaller/permission/service/PermissionControllerServiceImpl.java
+++ b/src/com/android/packageinstaller/permission/service/PermissionControllerServiceImpl.java
@@ -286,7 +286,7 @@
if (!doDryRun) {
int numChangedApps = appsWithRevokedPerms.size();
for (int i = 0; i < numChangedApps; i++) {
- appsWithRevokedPerms.get(i).persistChanges();
+ appsWithRevokedPerms.get(i).persistChanges(true);
}
}
@@ -528,33 +528,41 @@
Collections.singletonList(unexpandedPermission),
callerPkgInfo.applicationInfo.targetSdkVersion);
+ AppPermissions app = new AppPermissions(this, pkgInfo, false, null);
+
int numPerms = expandedPermissions.size();
for (int i = 0; i < numPerms; i++) {
- String perm = expandedPermissions.get(i);
- AppPermissionGroup group = AppPermissionGroup.create(this, pkgInfo, perm, true);
+ String permName = expandedPermissions.get(i);
+ AppPermissionGroup group = app.getGroupForPermission(permName);
if (group == null || group.isSystemFixed()) {
continue;
}
+ Permission perm = group.getPermission(permName);
+ if (perm == null) {
+ continue;
+ }
+
switch (grantState) {
case PERMISSION_GRANT_STATE_GRANTED:
- group.getPermission(perm).setPolicyFixed(true);
- group.grantRuntimePermissions(false, new String[]{perm});
+ perm.setPolicyFixed(true);
+ group.grantRuntimePermissions(false, new String[]{permName});
break;
case PERMISSION_GRANT_STATE_DENIED:
- group.getPermission(perm).setPolicyFixed(true);
- group.revokeRuntimePermissions(false, new String[]{perm});
+ perm.setPolicyFixed(true);
+ group.revokeRuntimePermissions(false, new String[]{permName});
break;
case PERMISSION_GRANT_STATE_DEFAULT:
- group.getPermission(perm).setPolicyFixed(false);
+ perm.setPolicyFixed(false);
break;
default:
return false;
}
-
- group.persistChanges(!callerPackageName.equals(packageName));
}
+ app.persistChanges(grantState == PERMISSION_GRANT_STATE_DENIED
+ || !callerPackageName.equals(packageName));
+
return true;
}
}
diff --git a/src/com/android/packageinstaller/permission/ui/handheld/ReviewPermissionsFragment.java b/src/com/android/packageinstaller/permission/ui/handheld/ReviewPermissionsFragment.java
index dd854b7..3504488 100644
--- a/src/com/android/packageinstaller/permission/ui/handheld/ReviewPermissionsFragment.java
+++ b/src/com/android/packageinstaller/permission/ui/handheld/ReviewPermissionsFragment.java
@@ -211,7 +211,7 @@
}
}
- mAppPermissions.persistChanges();
+ mAppPermissions.persistChanges(true);
}
private void bindUi() {
