From 60949288064460894bdd288f4865cdb180c0e501 Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Wed, 17 Feb 2016 16:52:59 -0800 Subject: [PATCH] Fix bugs in user restriction migration Originally I didn't know user-0 could have PO, so I excluded this case from migration. Now we handle it properly. Also make sure only restrictions that can actually be set by each owner moves to the owner restriction. (Because of this, we no longer have to have DISALLOW_WALLPAPER in the exception list, because owners can't set DISALLOW_WALLPAPER.) Bug 27225996 Change-Id: I6ad79d90e6c4400abbb1e4feba6ba59e3b650815 --- .../DevicePolicyManagerService.java | 59 +++++----- .../legacy_device_owner.xml | 2 + .../legacy_device_policies.xml | 5 + ...vicePolicyManagerServiceMigrationTest.java | 111 ++++++++++++++++-- 4 files changed, 138 insertions(+), 39 deletions(-) create mode 100644 services/tests/servicestests/assets/DevicePolicyManagerServiceMigrationTest2/legacy_device_owner.xml create mode 100644 services/tests/servicestests/assets/DevicePolicyManagerServiceMigrationTest2/legacy_device_policies.xml diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index 6ec0ba11517d0..07539cc513d8c 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -1689,7 +1689,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { final ActiveAdmin deviceOwnerAdmin = getDeviceOwnerAdminLocked(); migrateUserRestrictionsForUser(UserHandle.SYSTEM, deviceOwnerAdmin, - /* exceptionList =*/ null); + /* exceptionList =*/ null, /* isDeviceOwner =*/ true); // Push DO user restrictions to user manager. pushUserRestrictions(UserHandle.USER_SYSTEM); @@ -1697,39 +1697,36 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { mOwners.setDeviceOwnerUserRestrictionsMigrated(); } - // Migrate for POs. We have a few more exceptions. - final Set normalExceptionList = Sets.newArraySet( + // Migrate for POs. + + // The following restrictions can be set on secondary users by the device owner, so we + // assume they're not from the PO. + final Set secondaryUserExceptionList = Sets.newArraySet( UserManager.DISALLOW_OUTGOING_CALLS, UserManager.DISALLOW_SMS); - final Set managedExceptionList = new ArraySet<>(normalExceptionList.size() + 1); - managedExceptionList.addAll(normalExceptionList); - managedExceptionList.add(UserManager.DISALLOW_WALLPAPER); - for (UserInfo ui : mUserManager.getUsers()) { final int userId = ui.id; if (mOwners.getProfileOwnerUserRestrictionsNeedsMigration(userId)) { - if (userId != UserHandle.USER_SYSTEM) { - if (VERBOSE_LOG) { - Log.v(LOG_TAG, "Migrating PO user restrictions for user " + userId); - } - migrated = true; - - final ActiveAdmin profileOwnerAdmin = getProfileOwnerAdminLocked(userId); - - final Set exceptionList = - ui.isManagedProfile() ? managedExceptionList : normalExceptionList; - - migrateUserRestrictionsForUser(ui.getUserHandle(), profileOwnerAdmin, - exceptionList); - - // Note if a secondary user has no PO but has a DA that disables camera, we - // don't get here and won't push the camera user restriction to UserManager - // here. That's okay because we'll push user restrictions anyway when a user - // starts. But we still do it because we want to let user manager persist - // upon migration. - pushUserRestrictions(userId); + if (VERBOSE_LOG) { + Log.v(LOG_TAG, "Migrating PO user restrictions for user " + userId); } + migrated = true; + + final ActiveAdmin profileOwnerAdmin = getProfileOwnerAdminLocked(userId); + + final Set exceptionList = + (userId == UserHandle.USER_SYSTEM) ? null : secondaryUserExceptionList; + + migrateUserRestrictionsForUser(ui.getUserHandle(), profileOwnerAdmin, + exceptionList, /* isDeviceOwner =*/ false); + + // Note if a secondary user has no PO but has a DA that disables camera, we + // don't get here and won't push the camera user restriction to UserManager + // here. That's okay because we'll push user restrictions anyway when a user + // starts. But we still do it because we want to let user manager persist + // upon migration. + pushUserRestrictions(userId); mOwners.setProfileOwnerUserRestrictionsMigrated(userId); } @@ -1740,7 +1737,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { } private void migrateUserRestrictionsForUser(UserHandle user, ActiveAdmin admin, - Set exceptionList) { + Set exceptionList, boolean isDeviceOwner) { final Bundle origRestrictions = mUserManagerInternal.getBaseUserRestrictions( user.getIdentifier()); @@ -1751,7 +1748,11 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { if (!origRestrictions.getBoolean(key)) { continue; } - if (exceptionList!= null && exceptionList.contains(key)) { + final boolean canOwnerChange = isDeviceOwner + ? UserRestrictionsUtils.canDeviceOwnerChange(key) + : UserRestrictionsUtils.canProfileOwnerChange(key, user.getIdentifier()); + + if (!canOwnerChange || (exceptionList!= null && exceptionList.contains(key))) { newBaseRestrictions.putBoolean(key, true); } else { newOwnerRestrictions.putBoolean(key, true); diff --git a/services/tests/servicestests/assets/DevicePolicyManagerServiceMigrationTest2/legacy_device_owner.xml b/services/tests/servicestests/assets/DevicePolicyManagerServiceMigrationTest2/legacy_device_owner.xml new file mode 100644 index 0000000000000..c0977f79127a4 --- /dev/null +++ b/services/tests/servicestests/assets/DevicePolicyManagerServiceMigrationTest2/legacy_device_owner.xml @@ -0,0 +1,2 @@ + + diff --git a/services/tests/servicestests/assets/DevicePolicyManagerServiceMigrationTest2/legacy_device_policies.xml b/services/tests/servicestests/assets/DevicePolicyManagerServiceMigrationTest2/legacy_device_policies.xml new file mode 100644 index 0000000000000..6b53840434c1e --- /dev/null +++ b/services/tests/servicestests/assets/DevicePolicyManagerServiceMigrationTest2/legacy_device_policies.xml @@ -0,0 +1,5 @@ + + + + + diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceMigrationTest.java b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceMigrationTest.java index f32f2096b5920..3a6b983854af8 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceMigrationTest.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DevicePolicyManagerServiceMigrationTest.java @@ -92,6 +92,7 @@ public class DevicePolicyManagerServiceMigrationTest extends DpmTestBase { when(mMockContext.userManagerInternal.getBaseUserRestrictions( eq(10))).thenReturn(DpmTestUtils.newRestrictions( UserManager.DISALLOW_REMOVE_USER, + UserManager.DISALLOW_ADD_USER, UserManager.DISALLOW_SMS, UserManager.DISALLOW_OUTGOING_CALLS, UserManager.DISALLOW_WALLPAPER, @@ -100,6 +101,7 @@ public class DevicePolicyManagerServiceMigrationTest extends DpmTestBase { when(mMockContext.userManagerInternal.getBaseUserRestrictions( eq(11))).thenReturn(DpmTestUtils.newRestrictions( UserManager.DISALLOW_REMOVE_USER, + UserManager.DISALLOW_ADD_USER, UserManager.DISALLOW_SMS, UserManager.DISALLOW_OUTGOING_CALLS, UserManager.DISALLOW_WALLPAPER, @@ -137,53 +139,142 @@ public class DevicePolicyManagerServiceMigrationTest extends DpmTestBase { mContext.binder.restoreCallingIdentity(ident); } + assertTrue(dpms.mOwners.hasDeviceOwner()); + assertFalse(dpms.mOwners.hasProfileOwner(UserHandle.USER_SYSTEM)); + assertTrue(dpms.mOwners.hasProfileOwner(10)); + assertTrue(dpms.mOwners.hasProfileOwner(11)); + assertFalse(dpms.mOwners.hasProfileOwner(12)); + // Now all information should be migrated. assertFalse(dpms.mOwners.getDeviceOwnerUserRestrictionsNeedsMigration()); + assertFalse(dpms.mOwners.getProfileOwnerUserRestrictionsNeedsMigration( + UserHandle.USER_SYSTEM)); assertFalse(dpms.mOwners.getProfileOwnerUserRestrictionsNeedsMigration(10)); assertFalse(dpms.mOwners.getProfileOwnerUserRestrictionsNeedsMigration(11)); assertFalse(dpms.mOwners.getProfileOwnerUserRestrictionsNeedsMigration(12)); // Check the new base restrictions. DpmTestUtils.assertRestrictions( - DpmTestUtils.newRestrictions(), + DpmTestUtils.newRestrictions( + UserManager.DISALLOW_RECORD_AUDIO + ), newBaseRestrictions.get(UserHandle.USER_SYSTEM)); DpmTestUtils.assertRestrictions( DpmTestUtils.newRestrictions( + UserManager.DISALLOW_ADD_USER, UserManager.DISALLOW_SMS, - UserManager.DISALLOW_OUTGOING_CALLS + UserManager.DISALLOW_OUTGOING_CALLS, + UserManager.DISALLOW_RECORD_AUDIO, + UserManager.DISALLOW_WALLPAPER ), newBaseRestrictions.get(10)); DpmTestUtils.assertRestrictions( DpmTestUtils.newRestrictions( + UserManager.DISALLOW_ADD_USER, UserManager.DISALLOW_SMS, UserManager.DISALLOW_OUTGOING_CALLS, - UserManager.DISALLOW_WALLPAPER + UserManager.DISALLOW_WALLPAPER, + UserManager.DISALLOW_RECORD_AUDIO ), newBaseRestrictions.get(11)); // Check the new owner restrictions. DpmTestUtils.assertRestrictions( DpmTestUtils.newRestrictions( - UserManager.DISALLOW_ADD_USER, - UserManager.DISALLOW_RECORD_AUDIO + UserManager.DISALLOW_ADD_USER ), dpms.getDeviceOwnerAdminLocked().ensureUserRestrictions()); DpmTestUtils.assertRestrictions( DpmTestUtils.newRestrictions( - UserManager.DISALLOW_REMOVE_USER, - UserManager.DISALLOW_WALLPAPER, - UserManager.DISALLOW_RECORD_AUDIO + UserManager.DISALLOW_REMOVE_USER ), dpms.getProfileOwnerAdminLocked(10).ensureUserRestrictions()); DpmTestUtils.assertRestrictions( DpmTestUtils.newRestrictions( - UserManager.DISALLOW_REMOVE_USER, - UserManager.DISALLOW_RECORD_AUDIO + UserManager.DISALLOW_REMOVE_USER ), dpms.getProfileOwnerAdminLocked(11).ensureUserRestrictions()); } + + public void testMigration2_profileOwnerOnUser0() throws Exception { + setUpPackageManagerForAdmin(admin2, DpmMockContext.CALLER_SYSTEM_USER_UID); + + // Create the legacy owners & policies file. + DpmTestUtils.writeToFile( + (new File(mContext.dataDir, OwnersTestable.LEGACY_FILE)).getAbsoluteFile(), + DpmTestUtils.readAsset(mRealTestContext, + "DevicePolicyManagerServiceMigrationTest2/legacy_device_owner.xml")); + + DpmTestUtils.writeToFile( + (new File(mContext.systemUserDataDir, "device_policies.xml")).getAbsoluteFile(), + DpmTestUtils.readAsset(mRealTestContext, + "DevicePolicyManagerServiceMigrationTest2/legacy_device_policies.xml")); + + // Set up UserManager + when(mMockContext.userManagerInternal.getBaseUserRestrictions( + eq(UserHandle.USER_SYSTEM))).thenReturn(DpmTestUtils.newRestrictions( + UserManager.DISALLOW_ADD_USER, + UserManager.DISALLOW_RECORD_AUDIO, + UserManager.DISALLOW_SMS, + UserManager.DISALLOW_OUTGOING_CALLS)); + + final Map newBaseRestrictions = new HashMap<>(); + + doAnswer(new Answer() { + @Override + public Void answer(InvocationOnMock invocation) throws Throwable { + Integer userId = (Integer) invocation.getArguments()[0]; + Bundle bundle = (Bundle) invocation.getArguments()[1]; + + newBaseRestrictions.put(userId, bundle); + + return null; + } + }).when(mContext.userManagerInternal).setBaseUserRestrictionsByDpmsForMigration( + anyInt(), any(Bundle.class)); + + // Initialize DPM/DPMS and let it migrate the persisted information. + // (Need clearCallingIdentity() to pass permission checks.) + + final DevicePolicyManagerServiceTestable dpms; + + final long ident = mContext.binder.clearCallingIdentity(); + try { + LocalServices.removeServiceForTest(DevicePolicyManagerInternal.class); + + dpms = new DevicePolicyManagerServiceTestable(mContext, dataDir); + + dpms.systemReady(SystemService.PHASE_LOCK_SETTINGS_READY); + dpms.systemReady(SystemService.PHASE_BOOT_COMPLETED); + } finally { + mContext.binder.restoreCallingIdentity(ident); + } + assertFalse(dpms.mOwners.hasDeviceOwner()); + assertTrue(dpms.mOwners.hasProfileOwner(UserHandle.USER_SYSTEM)); + + // Now all information should be migrated. + assertFalse(dpms.mOwners.getDeviceOwnerUserRestrictionsNeedsMigration()); + assertFalse(dpms.mOwners.getProfileOwnerUserRestrictionsNeedsMigration( + UserHandle.USER_SYSTEM)); + + // Check the new base restrictions. + DpmTestUtils.assertRestrictions( + DpmTestUtils.newRestrictions( + UserManager.DISALLOW_RECORD_AUDIO + ), + newBaseRestrictions.get(UserHandle.USER_SYSTEM)); + + // Check the new owner restrictions. + DpmTestUtils.assertRestrictions( + DpmTestUtils.newRestrictions( + UserManager.DISALLOW_ADD_USER, + UserManager.DISALLOW_SMS, + UserManager.DISALLOW_OUTGOING_CALLS + ), + dpms.getProfileOwnerAdminLocked(UserHandle.USER_SYSTEM).ensureUserRestrictions()); + } }