Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 693c165b authored by Hai Zhang's avatar Hai Zhang
Browse files

Fix role permission granting.

Don't override user set/fixed permissions when we are re-granting
permissions during reconciliation. Otherwise, users won't be able to
revoke permissions granted by role (it will be constantly overridden
by the re-grant). This change fixed it by separating
overrideDisabledSystemPackage and overrideUserSetAndFixed, so that for
roles we can say we don't care about disabled system package, but we
do care about whether to override user choice.

Bug: 124452117
Test: presubmit
Change-Id: I9faa2f8bde5c9e6e0558822f1109d3fc2cadd5d7
parent c12ca3c4
Loading
Loading
Loading
Loading
+23 −26
Original line number Diff line number Diff line
@@ -62,13 +62,11 @@ public class Permissions {
     *
     * @param packageName the package name of the application to be granted permissions to
     * @param permissions the list of permissions to be granted
     * @param overrideDisabledSystemPackageAndUserSetAndFixed whether to ignore the permissions of a
     *                                                        disabled system package (if this
     *                                                        package is an updated system package),
     *                                                        and whether to override user set and
     *                                                        fixed flags on the permission
     * @param overrideDisabledSystemPackage whether to ignore the permissions of a disabled system
     *                                      package (if this package is an updated system package)
     * @param overrideUserSetAndFixed whether to override user set and fixed flags on the permission
     * @param setGrantedByDefault whether the permissions will be granted as granted-by-default
     * @param setSystemFixed whether the permissions will be granted as system-fixed
     * @param defaultGrant whether this is a default grant
     * @param context the {@code Context} to retrieve system services
     *
     * @return whether any permission or app op changed
@@ -77,8 +75,8 @@ public class Permissions {
     *      PackageInfo, java.util.Set, boolean, boolean, int)
     */
    public static boolean grant(@NonNull String packageName, @NonNull List<String> permissions,
            boolean overrideDisabledSystemPackageAndUserSetAndFixed, boolean setSystemFixed,
            boolean defaultGrant, @NonNull Context context) {
            boolean overrideDisabledSystemPackage, boolean overrideUserSetAndFixed,
            boolean setGrantedByDefault, boolean setSystemFixed, @NonNull Context context) {
        PackageInfo packageInfo = getPackageInfo(packageName, context);
        if (packageInfo == null) {
            return false;
@@ -115,7 +113,7 @@ public class Permissions {
        // choice to grant this app the permissions needed to function. For all other
        // apps, (default grants on first boot and user creation) we don't grant default
        // permissions if the version on the system image does not declare them.
        if (!overrideDisabledSystemPackageAndUserSetAndFixed && isUpdatedSystemApp(packageInfo)) {
        if (!overrideDisabledSystemPackage && isUpdatedSystemApp(packageInfo)) {
            PackageInfo disabledSystemPackageInfo = getFactoryPackageInfo(packageName, context);
            if (disabledSystemPackageInfo != null) {
                if (ArrayUtils.isEmpty(disabledSystemPackageInfo.requestedPermissions)) {
@@ -148,29 +146,28 @@ public class Permissions {
            }
        }

        Set<String> whitelistedRestrictedPermissions = new ArraySet<>(context.getPackageManager()
                .getWhitelistedRestrictedPermissions(packageName,
                        PackageManager.FLAG_PERMISSION_WHITELIST_SYSTEM));
        boolean permissionOrAppOpChanged = false;

        PackageManager packageManager = context.getPackageManager();
        Set<String> whitelistedRestrictedPermissions = new ArraySet<>(
                packageManager.getWhitelistedRestrictedPermissions(packageName,
                        PackageManager.FLAG_PERMISSION_WHITELIST_SYSTEM));
        List<String> smsPermissions = Utils.getPlatformPermissionNamesOfGroup(
                Manifest.permission_group.SMS);
        List<String> callLogPermissions = Utils.getPlatformPermissionNamesOfGroup(
                Manifest.permission_group.CALL_LOG);

        boolean permissionOrAppOpChanged = false;

        int sortedPermissionsToGrantLength = sortedPermissionsToGrant.length;
        for (int i = 0; i < sortedPermissionsToGrantLength; i++) {
            String permission = sortedPermissionsToGrant[i];

            permissionOrAppOpChanged |= grantSingle(packageName, permission,
                    overrideDisabledSystemPackageAndUserSetAndFixed, setSystemFixed,
                    defaultGrant, context);
                    overrideUserSetAndFixed, setGrantedByDefault, setSystemFixed, context);

            if ((smsPermissions.contains(permission) || callLogPermissions.contains(permission))
                    && whitelistedRestrictedPermissions.add(permission)) {
                context.getPackageManager().addWhitelistedRestrictedPermission(packageName,
                        permission, PackageManager.FLAG_PERMISSION_WHITELIST_SYSTEM);
                packageManager.addWhitelistedRestrictedPermission(packageName, permission,
                        PackageManager.FLAG_PERMISSION_WHITELIST_SYSTEM);
            }
        }

@@ -178,7 +175,7 @@ public class Permissions {
    }

    private static boolean grantSingle(@NonNull String packageName, @NonNull String permission,
            boolean overrideUserSetAndFixed, boolean setSystemFixed, boolean defaultGrant,
            boolean overrideUserSetAndFixed, boolean setGrantedByDefault, boolean setSystemFixed,
            @NonNull Context context) {
        boolean wasPermissionOrAppOpGranted = isPermissionAndAppOpGranted(packageName, permission,
                context);
@@ -213,7 +210,7 @@ public class Permissions {

        // Update permission flags.
        int newFlags = 0;
        if (defaultGrant) {
        if (setGrantedByDefault) {
            newFlags |= PackageManager.FLAG_PERMISSION_GRANTED_BY_DEFAULT;
        }
        if (setSystemFixed) {
@@ -228,7 +225,7 @@ public class Permissions {
        }
        // If a component gets a permission for being the default handler A and also default handler
        // B, we grant the weaker grant form. This only applies to default permission grant.
        if (defaultGrant && !setSystemFixed) {
        if (setGrantedByDefault && !setSystemFixed) {
            int oldFlags = getPermissionFlags(packageName, permission, context);
            if ((oldFlags & PackageManager.FLAG_PERMISSION_GRANTED_BY_DEFAULT) != 0
                    && (oldFlags & PackageManager.FLAG_PERMISSION_SYSTEM_FIXED) != 0) {
@@ -398,8 +395,9 @@ public class Permissions {
            }
        }

        Set<String> whitelistedRestrictedPermissions = context.getPackageManager()
                .getWhitelistedRestrictedPermissions(packageName,
        PackageManager packageManager = context.getPackageManager();
        Set<String> whitelistedRestrictedPermissions =
                packageManager.getWhitelistedRestrictedPermissions(packageName,
                        Utils.FLAGS_PERMISSION_WHITELIST_ALL);

        boolean permissionOrAppOpChanged = false;
@@ -413,10 +411,9 @@ public class Permissions {

            // Remove from the system whitelist only if not granted by default.
            if (!isPermissionGrantedByDefault(packageName, permission, context)
                    && whitelistedRestrictedPermissions != null
                    && whitelistedRestrictedPermissions.remove(permission)) {
                context.getPackageManager().removeWhitelistedRestrictedPermission(packageName,
                        permission, PackageManager.FLAG_PERMISSION_WHITELIST_SYSTEM);
                packageManager.removeWhitelistedRestrictedPermission(packageName, permission,
                        PackageManager.FLAG_PERMISSION_WHITELIST_SYSTEM);
            }
        }

+5 −3
Original line number Diff line number Diff line
@@ -533,12 +533,14 @@ public class Role {
     *
     * @param packageName the package name of the application to be granted this role to
     * @param dontKillApp whether this application should not be killed despite changes
     * @param overrideUserSetAndFixedPermissions whether to override user set and fixed flags on
     *                                           permissions
     * @param context the {@code Context} to retrieve system services
     */
    public void grant(@NonNull String packageName, boolean dontKillApp, @NonNull Context context) {
    public void grant(@NonNull String packageName, boolean dontKillApp,
            boolean overrideUserSetAndFixedPermissions, @NonNull Context context) {
        boolean permissionOrAppOpChanged = Permissions.grant(packageName, mPermissions,
                true /*overrideDisabledSystemPackageAndUserSetAndFixedPermissions*/,
                false /*setPermissionsSystemFixed*/, false /*defaultGrant*/, context);
                true, overrideUserSetAndFixedPermissions, false, false, context);

        int appOpsSize = mAppOps.size();
        for (int i = 0; i < appOpsSize; i++) {
+18 −17
Original line number Diff line number Diff line
@@ -105,10 +105,10 @@ public class RoleControllerServiceImpl extends RoleControllerService {
                String packageName = currentPackageNames.get(currentPackageNamesIndex);

                if (role.isPackageQualified(packageName, this)) {
                    // NOTE: We should not override only if we are granting default permissions
                    // that are not based on user controlled roles, so for now override.
                    addRoleHolderInternal(role, packageName, false /* dontKillApp */,
                            true /* added */);
                    // We should not override user set or fixed permissions because we are only
                    // redoing the grant here. Otherwise, user won't be able to revoke permissions
                    // granted by role.
                    addRoleHolderInternal(role, packageName, false, false, true);
                } else {
                    Log.i(LOG_TAG, "Removing package that no longer qualifies for the role,"
                            + " package: " + packageName + ", role: " + roleName);
@@ -142,9 +142,11 @@ public class RoleControllerServiceImpl extends RoleControllerService {
                    }
                    Log.i(LOG_TAG, "Adding package as default/fallback role holder, package: "
                            + packageName + ", role: " + roleName);
                    // NOTE: We should not override only if we are granting default permissions
                    // that are not based on user controlled roles, so for now override.
                    addRoleHolderInternal(role, packageName);
                    // TODO: If we don't override user here, user might end up missing incoming
                    // phone calls or SMS, so we just keep the old behavior. But overriding user
                    // choice about permission without explicit user action is bad, so maybe we
                    // should at least show a notification?
                    addRoleHolderInternal(role, packageName, true);
                }
            }

@@ -221,7 +223,7 @@ public class RoleControllerServiceImpl extends RoleControllerService {
        }

        boolean dontKillApp = hasFlag(flags, RoleManager.MANAGE_HOLDERS_FLAG_DONT_KILL_APP);
        added = addRoleHolderInternal(role, packageName, dontKillApp, added);
        added = addRoleHolderInternal(role, packageName, dontKillApp, true, added);
        if (!added) {
            return false;
        }
@@ -302,16 +304,16 @@ public class RoleControllerServiceImpl extends RoleControllerService {
    }

    @WorkerThread
    private boolean addRoleHolderInternal(@NonNull Role role, @NonNull String packageName) {
        return addRoleHolderInternal(role, packageName, false /* dontKillApp */,
                false /* added */);
    private boolean addRoleHolderInternal(@NonNull Role role, @NonNull String packageName,
            boolean overrideUserSetAndFixedPermissions) {
        return addRoleHolderInternal(role, packageName, false, overrideUserSetAndFixedPermissions,
                false);
    }

    @WorkerThread
    private boolean addRoleHolderInternal(@NonNull Role role, @NonNull String packageName,
            boolean dontKillApp, boolean added) {
        // TODO: STOPSHIP: Pass in appropriate arguments.
        role.grant(packageName, dontKillApp, this);
            boolean dontKillApp, boolean overrideUserSetAndFixedPermissions, boolean added) {
        role.grant(packageName, dontKillApp, overrideUserSetAndFixedPermissions, this);

        String roleName = role.getName();
        if (!added) {
@@ -388,12 +390,11 @@ public class RoleControllerServiceImpl extends RoleControllerService {

        Log.i(LOG_TAG, "Adding package as fallback role holder, package: " + fallbackPackageName
                + ", role: " + roleName);
        // TODO: If we don't override user here, user might end up missing incoming
        // phone calls or SMS, so we just keep the old behavior. But overriding user
        // choice about permission without explicit user action is bad, so maybe we
        // should at least show a notification?
        // NOTE: We should not override only if we are granting default permissions
        // that are not based on user controlled roles, so for now override.
        return addRoleHolderInternal(role, fallbackPackageName);
        return addRoleHolderInternal(role, fallbackPackageName, true);
    }

    @Override