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

Commit acdeac68 authored by Robin Lee's avatar Robin Lee
Browse files

Fix broken RestrictedLockUtils KeyGuard APIs 

They were all broken in that they returned profile admins for parent
queries even when they clearly shouldn't.

Examples:
 - disable unredacted notifications
 - disable fingerprint

This doesn't seem to have been tested beyond the bare basics of one
user with one device admin. Added some reasonable coverage. It could
still do with more.

Test: make RunSettingsLibRoboTests
Bug: 34929375
Change-Id: I1b0e986056ffa62d47091c0010977ac810ebd690
parent c98c16de

core/java/android/app/admin/DevicePolicyManager.java

+11 −0
Original line number Diff line number Diff line
@@ -3345,6 +3345,17 @@ public class DevicePolicyManager { 
     */

    public static final int KEYGUARD_DISABLE_FEATURES_ALL = 0x7fffffff;



    /**

     * Keyguard features that when set on a managed profile that doesn't have its own challenge will

     * affect the profile's parent user. These can also be set on the managed profile's parent

     * {@link DevicePolicyManager} instance.

     *

     * @hide

     */

    public static final int PROFILE_KEYGUARD_FEATURES_AFFECT_OWNER =

            DevicePolicyManager.KEYGUARD_DISABLE_TRUST_AGENTS

            | DevicePolicyManager.KEYGUARD_DISABLE_FINGERPRINT;



    /**

     * Called by an application that is administering the device to request that the storage system

     * be encrypted.

packages/SettingsLib/src/com/android/settingslib/RestrictedLockUtils.java

+80 −33
Original line number Diff line number Diff line
@@ -16,6 +16,11 @@ 


package com.android.settingslib;



import static android.app.admin.DevicePolicyManager.KEYGUARD_DISABLE_FEATURES_NONE;

import static android.app.admin.DevicePolicyManager.PROFILE_KEYGUARD_FEATURES_AFFECT_OWNER;



import android.annotation.NonNull;

import android.annotation.Nullable;

import android.annotation.UserIdInt;

import android.app.AppGlobals;

import android.app.admin.DevicePolicyManager;
@@ -29,6 +34,7 @@ import android.os.RemoteException; 
import android.os.UserHandle;

import android.os.UserManager;

import android.provider.Settings;

import android.support.annotation.VisibleForTesting;

import android.text.Spanned;

import android.text.SpannableStringBuilder;

import android.text.style.ForegroundColorSpan;
@@ -108,30 +114,54 @@ public class RestrictedLockUtils { 
    }



    /**

     * Checks if keyguard features are disabled by policy.

     * Checks whether keyguard features are disabled by policy.

     *

     * @param context {@link Context} for the calling user.

     *

     * @param keyguardFeatures Could be any of keyguard features that can be

     * @param keyguardFeatures Any one of keyguard features that can be

     * disabled by {@link android.app.admin.DevicePolicyManager#setKeyguardDisabledFeatures}.

     *

     * @param userId User to check enforced admin status for.

     *

     * @return EnforcedAdmin Object containing the enforced admin component and admin user details,

     * or {@code null} If the notification features are not disabled. If the restriction is set by

     * multiple admins, then the admin component will be set to {@code null} and userId to

     * {@link UserHandle#USER_NULL}.

     */

    public static EnforcedAdmin checkIfKeyguardFeaturesDisabled(Context context,

            int keyguardFeatures, int userId) {

        final LockSettingCheck check =

                (DevicePolicyManager dpm, ComponentName admin, @UserIdInt int checkUser) ->

                        (dpm.getKeyguardDisabledFeatures(admin, checkUser) & keyguardFeatures) != 0;



        final DevicePolicyManager dpm = (DevicePolicyManager) context.getSystemService(

                Context.DEVICE_POLICY_SERVICE);

        if (dpm == null) {

            return null;

            int keyguardFeatures, final @UserIdInt int userId) {

        final LockSettingCheck check = (dpm, admin, checkUser) -> {

            int effectiveFeatures = dpm.getKeyguardDisabledFeatures(admin, checkUser);

            if (checkUser != userId) {

                effectiveFeatures &= PROFILE_KEYGUARD_FEATURES_AFFECT_OWNER;

            }

            return (effectiveFeatures & keyguardFeatures) != KEYGUARD_DISABLE_FEATURES_NONE;

        };

        if (UserManager.get(context).getUserInfo(userId).isManagedProfile()) {

            DevicePolicyManager dpm =

                    (DevicePolicyManager) context.getSystemService(Context.DEVICE_POLICY_SERVICE);

            return findEnforcedAdmin(dpm.getActiveAdminsAsUser(userId), dpm, userId, check);

        }

        return checkForLockSetting(context, userId, check);

    }



        final UserManager um = (UserManager) context.getSystemService(Context.USER_SERVICE);

        if (um.getUserInfo(userId).isManagedProfile()) {

            final List<ComponentName> admins = dpm.getActiveAdminsAsUser(userId);

    /**

     * Filter a set of device admins based on a predicate {@code check}. This is equivalent to

     * {@code admins.stream().filter(check).map(x → new EnforcedAdmin(admin, userId)} except it's

     * returning a zero/one/many-type thing.

     *

     * @param admins set of candidate device admins identified by {@link ComponentName}.

     * @param userId user to create the resultant {@link EnforcedAdmin} as.

     * @param check filter predicate.

     *

     * @return {@code null} if none of the {@param admins} match.

     *         An {@link EnforcedAdmin} if exactly one of the admins matches.

     *         Otherwise, {@link EnforcedAdmin#MULTIPLE_ENFORCED_ADMIN} for multiple matches.

     */

    @Nullable

    private static EnforcedAdmin findEnforcedAdmin(@Nullable List<ComponentName> admins,

            @NonNull DevicePolicyManager dpm, @UserIdInt int userId,

            @NonNull LockSettingCheck check) {

        if (admins == null) {

            return null;

        }
@@ -146,9 +176,6 @@ public class RestrictedLockUtils { 
            }

        }

        return enforcedAdmin;

        } else {

            return checkForLockSetting(context, userId, check);

        }

    }



    public static EnforcedAdmin checkIfUninstallBlocked(Context context,
@@ -361,7 +388,7 @@ public class RestrictedLockUtils { 
        }



        LockPatternUtils lockPatternUtils = new LockPatternUtils(context);

        if (lockPatternUtils.isSeparateProfileChallengeEnabled(userId)) {

        if (sProxy.isSeparateProfileChallengeEnabled(lockPatternUtils, userId)) {

            // userId is managed profile and has a separate challenge, only consider

            // the admins in that user.

            final List<ComponentName> admins = dpm.getActiveAdminsAsUser(userId);
@@ -428,7 +455,7 @@ public class RestrictedLockUtils { 
                if (userInfo.isManagedProfile()) {

                    // If userInfo.id is a managed profile, we also need to look at

                    // the policies set on the parent.

                    final DevicePolicyManager parentDpm = dpm.getParentProfileInstance(userInfo);

                    DevicePolicyManager parentDpm = sProxy.getParentProfileInstance(dpm, userInfo);

                    if (parentDpm.getMaximumTimeToLock(admin, userInfo.id) > 0) {

                        if (enforcedAdmin == null) {

                            enforcedAdmin = new EnforcedAdmin(admin, userInfo.id);
@@ -448,8 +475,9 @@ public class RestrictedLockUtils { 


    /**

     * Checks whether any of the user's profiles enforce the lock setting. A managed profile is only

     * included if it does not have a separate challenege but the settings for it's parent (i.e. the

     * user being checked) are always included.

     * included if it does not have a separate challenge.

     *

     * The user identified by {@param userId} is always included.

     */

    private static EnforcedAdmin checkForLockSetting(

            Context context, @UserIdInt int userId, LockSettingCheck check) {
@@ -468,7 +496,7 @@ public class RestrictedLockUtils { 
                continue;

            }

            final boolean isSeparateProfileChallengeEnabled =

                    lockPatternUtils.isSeparateProfileChallengeEnabled(userInfo.id);

                    sProxy.isSeparateProfileChallengeEnabled(lockPatternUtils, userInfo.id);

            for (ComponentName admin : admins) {

                if (!isSeparateProfileChallengeEnabled) {

                    if (check.isEnforcing(dpm, admin, userInfo.id)) {
@@ -487,7 +515,7 @@ public class RestrictedLockUtils { 
                if (userInfo.isManagedProfile()) {

                    // If userInfo.id is a managed profile, we also need to look at

                    // the policies set on the parent.

                    final DevicePolicyManager parentDpm = dpm.getParentProfileInstance(userInfo);

                    DevicePolicyManager parentDpm = sProxy.getParentProfileInstance(dpm, userInfo);

                    if (check.isEnforcing(parentDpm, admin, userInfo.id)) {

                        if (enforcedAdmin == null) {

                            enforcedAdmin = new EnforcedAdmin(admin, userInfo.id);
@@ -733,4 +761,23 @@ public class RestrictedLockUtils { 
            other.userId = userId;

        }

    }



    /**

     * Static {@link LockPatternUtils} and {@link DevicePolicyManager} wrapper for testing purposes.

     * {@link LockPatternUtils} is an internal API not supported by robolectric.

     * {@link DevicePolicyManager} has a {@code getProfileParent} not yet suppored by robolectric.

     */

    @VisibleForTesting

    static Proxy sProxy = new Proxy();



    @VisibleForTesting

    static class Proxy {

        public boolean isSeparateProfileChallengeEnabled(LockPatternUtils utils, int userHandle) {

            return utils.isSeparateProfileChallengeEnabled(userHandle);

        }



        public DevicePolicyManager getParentProfileInstance(DevicePolicyManager dpm, UserInfo ui) {

            return dpm.getParentProfileInstance(ui);

        }

    }

}

packages/SettingsLib/tests/robotests/src/com/android/settingslib/RestrictedLockUtilsTest.java

+142 −14
Original line number Diff line number Diff line
@@ -16,6 +16,17 @@ 


package com.android.settingslib;



import static android.app.admin.DevicePolicyManager.KEYGUARD_DISABLE_FEATURES_NONE;

import static android.app.admin.DevicePolicyManager.KEYGUARD_DISABLE_FINGERPRINT;

import static android.app.admin.DevicePolicyManager.KEYGUARD_DISABLE_REMOTE_INPUT;

import static android.app.admin.DevicePolicyManager.KEYGUARD_DISABLE_UNREDACTED_NOTIFICATIONS;

import static com.android.settingslib.RestrictedLockUtils.EnforcedAdmin;

import static com.google.common.truth.Truth.assertThat;

import static org.mockito.Matchers.any;

import static org.mockito.Matchers.eq;

import static org.mockito.Mockito.doReturn;

import static org.mockito.Mockito.when;



import android.app.admin.DevicePolicyManager;

import android.content.ComponentName;

import android.content.Context;
@@ -25,18 +36,13 @@ import android.os.UserManager; 
import org.junit.Before;

import org.junit.Test;

import org.junit.runner.RunWith;

import org.mockito.Answers;

import org.mockito.Mock;

import org.mockito.MockitoAnnotations;

import org.robolectric.annotation.Config;



import java.util.Arrays;



import static android.app.admin.DevicePolicyManager.KEYGUARD_DISABLE_FINGERPRINT;

import static android.app.admin.DevicePolicyManager.KEYGUARD_DISABLE_REMOTE_INPUT;

import static com.android.settingslib.RestrictedLockUtils.EnforcedAdmin;

import static com.google.common.truth.Truth.assertThat;

import static org.mockito.Mockito.when;



@RunWith(SettingLibRobolectricTestRunner.class)

@Config(manifest = TestConfig.MANIFEST_PATH, sdk = TestConfig.SDK_VERSION)

public class RestrictedLockUtilsTest {
@@ -47,8 +53,11 @@ public class RestrictedLockUtilsTest { 
    private DevicePolicyManager mDevicePolicyManager;

    @Mock

    private UserManager mUserManager;

    @Mock(answer = Answers.RETURNS_DEEP_STUBS)

    private RestrictedLockUtils.Proxy mProxy;



    private static final int mUserId = 194;

    private static final int mProfileId = 160;

    private static final ComponentName mAdmin1 = new ComponentName("admin1", "admin1class");

    private static final ComponentName mAdmin2 = new ComponentName("admin2", "admin2class");
@@ -60,12 +69,13 @@ public class RestrictedLockUtilsTest { 
                .thenReturn(mDevicePolicyManager);

        when(mContext.getSystemService(Context.USER_SERVICE))

                .thenReturn(mUserManager);



        RestrictedLockUtils.sProxy = mProxy;

    }



    @Test

    public void checkIfKeyguardFeaturesDisabled_noEnforcedAdminForManagedProfile() {

        setUpManagedProfile(mUserId);

        setUpActiveAdmins(mUserId, new ComponentName[] {mAdmin1, mAdmin2});

        setUpManagedProfile(mUserId, new ComponentName[] {mAdmin1, mAdmin2});



        final EnforcedAdmin enforcedAdmin = RestrictedLockUtils.checkIfKeyguardFeaturesDisabled(

                mContext, KEYGUARD_DISABLE_FINGERPRINT, mUserId);
@@ -75,8 +85,7 @@ public class RestrictedLockUtilsTest { 


    @Test

    public void checkIfKeyguardFeaturesDisabled_oneEnforcedAdminForManagedProfile() {

        setUpManagedProfile(mUserId);

        setUpActiveAdmins(mUserId, new ComponentName[] {mAdmin1, mAdmin2});

        setUpManagedProfile(mUserId, new ComponentName[] {mAdmin1, mAdmin2});



        when(mDevicePolicyManager.getKeyguardDisabledFeatures(mAdmin1, mUserId))

                .thenReturn(KEYGUARD_DISABLE_FINGERPRINT);
@@ -89,8 +98,7 @@ public class RestrictedLockUtilsTest { 


    @Test

    public void checkIfKeyguardFeaturesDisabled_multipleEnforcedAdminForManagedProfile() {

        setUpManagedProfile(mUserId);

        setUpActiveAdmins(mUserId, new ComponentName[] {mAdmin1, mAdmin2});

        setUpManagedProfile(mUserId, new ComponentName[] {mAdmin1, mAdmin2});



        when(mDevicePolicyManager.getKeyguardDisabledFeatures(mAdmin1, mUserId))

                .thenReturn(KEYGUARD_DISABLE_REMOTE_INPUT);
@@ -103,9 +111,129 @@ public class RestrictedLockUtilsTest { 
        assertThat(enforcedAdmin).isEqualTo(EnforcedAdmin.MULTIPLE_ENFORCED_ADMIN);

    }



    private UserInfo setUpManagedProfile(int userId) {

        final UserInfo userInfo = new UserInfo(userId, "myuser", UserInfo.FLAG_MANAGED_PROFILE);

    @Test

    public void checkIfKeyguardFeaturesAreDisabled_doesMatchAllowedFeature_unifiedManagedProfile() {

        UserInfo userInfo = setUpUser(mUserId, new ComponentName[] {mAdmin1});

        UserInfo profileInfo = setUpManagedProfile(mProfileId, new ComponentName[] {mAdmin2});

        when(mUserManager.getProfiles(mUserId)).thenReturn(Arrays.asList(new UserInfo[] {

                userInfo, profileInfo}));



        when(mDevicePolicyManager.getKeyguardDisabledFeatures(mAdmin1, mUserId))

                .thenReturn(KEYGUARD_DISABLE_FEATURES_NONE);

        when(mDevicePolicyManager.getKeyguardDisabledFeatures(mAdmin2, mProfileId))

                .thenReturn(KEYGUARD_DISABLE_FINGERPRINT);



        // Querying the parent should return the policy, since it affects the parent.

        EnforcedAdmin parent = RestrictedLockUtils.checkIfKeyguardFeaturesDisabled(

                mContext, KEYGUARD_DISABLE_FINGERPRINT, mUserId);

        assertThat(parent).isEqualTo(new EnforcedAdmin(mAdmin2, mProfileId));



        // Querying the child should return that too.

        EnforcedAdmin profile = RestrictedLockUtils.checkIfKeyguardFeaturesDisabled(

                mContext, KEYGUARD_DISABLE_FINGERPRINT, mProfileId);

        assertThat(profile).isEqualTo(new EnforcedAdmin(mAdmin2, mProfileId));



        // Querying for some unrelated feature should return nothing. Nothing!

        assertThat(RestrictedLockUtils.checkIfKeyguardFeaturesDisabled(

                mContext, KEYGUARD_DISABLE_REMOTE_INPUT, mUserId)).isNull();

        assertThat(RestrictedLockUtils.checkIfKeyguardFeaturesDisabled(

                mContext, KEYGUARD_DISABLE_REMOTE_INPUT, mProfileId)).isNull();

    }



    @Test

    public void checkIfKeyguardFeaturesAreDisabled_notMatchOtherFeatures_unifiedManagedProfile() {

        UserInfo userInfo = setUpUser(mUserId, new ComponentName[] {mAdmin1});

        UserInfo profileInfo = setUpManagedProfile(mProfileId, new ComponentName[] {mAdmin2});

        when(mUserManager.getProfiles(mUserId)).thenReturn(Arrays.asList(new UserInfo[] {

                userInfo, profileInfo}));



        when(mDevicePolicyManager.getKeyguardDisabledFeatures(mAdmin1, mUserId))

                .thenReturn(KEYGUARD_DISABLE_FEATURES_NONE);

        when(mDevicePolicyManager.getKeyguardDisabledFeatures(mAdmin2, mProfileId))

                .thenReturn(KEYGUARD_DISABLE_UNREDACTED_NOTIFICATIONS);



        // Querying the parent should not return the policy, because it's not a policy that should

        // affect parents even when the lock screen is unified.

        EnforcedAdmin primary = RestrictedLockUtils.checkIfKeyguardFeaturesDisabled(

                mContext, KEYGUARD_DISABLE_UNREDACTED_NOTIFICATIONS, mUserId);

        assertThat(primary).isNull();



        // Querying the child should still return the policy.

        EnforcedAdmin profile = RestrictedLockUtils.checkIfKeyguardFeaturesDisabled(

                mContext, KEYGUARD_DISABLE_UNREDACTED_NOTIFICATIONS, mProfileId);

        assertThat(profile).isEqualTo(new EnforcedAdmin(mAdmin2, mProfileId));

    }



    @Test

    public void checkIfKeyguardFeaturesAreDisabled_onlyMatchesProfile_separateManagedProfile() {

        UserInfo userInfo = setUpUser(mUserId, new ComponentName[] {mAdmin1});

        UserInfo profileInfo = setUpManagedProfile(mProfileId, new ComponentName[] {mAdmin2});

        when(mUserManager.getProfiles(mUserId)).thenReturn(Arrays.asList(new UserInfo[] {

                userInfo, profileInfo}));



        when(mDevicePolicyManager.getKeyguardDisabledFeatures(mAdmin1, mUserId))

                .thenReturn(KEYGUARD_DISABLE_FEATURES_NONE);

        when(mDevicePolicyManager.getKeyguardDisabledFeatures(mAdmin2, mProfileId))

                .thenReturn(KEYGUARD_DISABLE_FINGERPRINT);



        // Crucially for this test, isSeparateWorkChallengeEnabled => true.

        doReturn(true).when(mProxy).isSeparateProfileChallengeEnabled(any(), eq(mProfileId));



        // Querying the parent should not return the policy, even though it's shared by default,

        // because the parent doesn't share a lock screen with the profile any more.

        EnforcedAdmin parent = RestrictedLockUtils.checkIfKeyguardFeaturesDisabled(

                mContext, KEYGUARD_DISABLE_FINGERPRINT, mUserId);

        assertThat(parent).isNull();



        // Querying the child should still return the policy.

        EnforcedAdmin profile = RestrictedLockUtils.checkIfKeyguardFeaturesDisabled(

                mContext, KEYGUARD_DISABLE_FINGERPRINT, mProfileId);

        assertThat(profile).isEqualTo(new EnforcedAdmin(mAdmin2, mProfileId));

    }



    /**

     * This test works great. The real world implementation is sketchy though.

     * <p>

     * DevicePolicyManager.getParentProfileInstance(UserInfo) does not do what it looks like it does

     * (which would be to get an instance for the parent of the user that's passed in to it.)

     * <p>

     * Instead it just always returns a parent instance for the current user.

     * <p>

     * Still, the test works.

     */

    @Test

    public void checkIfKeyguardFeaturesAreDisabled_onlyMatchesParent_profileParentPolicy() {

        UserInfo userInfo = setUpUser(mUserId, new ComponentName[] {mAdmin1});

        UserInfo profileInfo = setUpManagedProfile(mProfileId, new ComponentName[] {mAdmin2});

        when(mUserManager.getProfiles(mUserId)).thenReturn(Arrays.asList(new UserInfo[] {

                userInfo, profileInfo}));



        when(mProxy.getParentProfileInstance(any(DevicePolicyManager.class), any())

                .getKeyguardDisabledFeatures(mAdmin2, mProfileId))

                .thenReturn(KEYGUARD_DISABLE_FINGERPRINT);



        // Parent should get the policy.

        EnforcedAdmin parent = RestrictedLockUtils.checkIfKeyguardFeaturesDisabled(

                mContext, KEYGUARD_DISABLE_FINGERPRINT, mUserId);

        assertThat(parent).isEqualTo(new EnforcedAdmin(mAdmin2, mProfileId));



        // Profile should not get the policy.

        EnforcedAdmin profile = RestrictedLockUtils.checkIfKeyguardFeaturesDisabled(

                mContext, KEYGUARD_DISABLE_FINGERPRINT, mProfileId);

        assertThat(profile).isNull();

    }



    private UserInfo setUpUser(int userId, ComponentName[] admins) {

        UserInfo userInfo = new UserInfo(userId, "primary", 0);

        when(mUserManager.getUserInfo(userId)).thenReturn(userInfo);

        setUpActiveAdmins(userId, admins);

        return userInfo;

    }



    private UserInfo setUpManagedProfile(int userId, ComponentName[] admins) {

        UserInfo userInfo = new UserInfo(userId, "profile", UserInfo.FLAG_MANAGED_PROFILE);

        when(mUserManager.getUserInfo(userId)).thenReturn(userInfo);

        setUpActiveAdmins(userId, admins);

        return userInfo;

    }

services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java

+1 −9
Original line number Diff line number Diff line
@@ -41,6 +41,7 @@ import static android.app.admin.DevicePolicyManager.DELEGATION_KEEP_UNINSTALLED_ 
import static android.app.admin.DevicePolicyManager.DELEGATION_PACKAGE_ACCESS;

import static android.app.admin.DevicePolicyManager.DELEGATION_PERMISSION_GRANT;

import static android.app.admin.DevicePolicyManager.PASSWORD_QUALITY_COMPLEX;

import static android.app.admin.DevicePolicyManager.PROFILE_KEYGUARD_FEATURES_AFFECT_OWNER;

import static android.app.admin.DevicePolicyManager.WIPE_EXTERNAL_STORAGE;

import static android.app.admin.DevicePolicyManager.WIPE_RESET_PROTECTION_DATA;

import static android.content.pm.PackageManager.MATCH_UNINSTALLED_PACKAGES;
@@ -330,15 +331,6 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { 
        GLOBAL_SETTINGS_DEPRECATED.add(Settings.Global.WIFI_ON);

    }



    /**

     * Keyguard features that when set on a managed profile that doesn't have its own challenge will

     * affect the profile's parent user. These can also be set on the managed profile's parent DPM

     * instance.

     */

    private static final int PROFILE_KEYGUARD_FEATURES_AFFECT_OWNER =

            DevicePolicyManager.KEYGUARD_DISABLE_TRUST_AGENTS

            | DevicePolicyManager.KEYGUARD_DISABLE_FINGERPRINT;



    /**

     * Keyguard features that when set on a profile affect the profile content or challenge only.

     * These cannot be set on the managed profile's parent DPM instance