diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index 10c321b5ef892..ea19f412afe40 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -5958,8 +5958,9 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { throw new IllegalArgumentException("Invalid component " + admin + " for device owner"); } + final boolean hasIncompatibleAccounts = hasIncompatibleAccountsNoLock(userId, admin); synchronized (this) { - enforceCanSetDeviceOwnerLocked(admin, userId); + enforceCanSetDeviceOwnerLocked(admin, userId, hasIncompatibleAccounts); final ActiveAdmin activeAdmin = getActiveAdminUncheckedLocked(admin, userId); if (activeAdmin == null || getUserData(userId).mRemovingAdmins.contains(admin)) { @@ -6184,8 +6185,9 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { throw new IllegalArgumentException("Component " + who + " not installed for userId:" + userHandle); } + final boolean hasIncompatibleAccounts = hasIncompatibleAccountsNoLock(userHandle, who); synchronized (this) { - enforceCanSetProfileOwnerLocked(who, userHandle); + enforceCanSetProfileOwnerLocked(who, userHandle, hasIncompatibleAccounts); if (getActiveAdminUncheckedLocked(who, userHandle) == null || getUserData(userHandle).mRemovingAdmins.contains(who)) { @@ -6517,9 +6519,10 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { * The profile owner can only be set before the user setup phase has completed, * except for: * - SYSTEM_UID - * - adb if there are no accounts. (But see {@link #hasIncompatibleAccountsLocked}) + * - adb unless hasIncompatibleAccounts is true. */ - private void enforceCanSetProfileOwnerLocked(@Nullable ComponentName owner, int userHandle) { + private void enforceCanSetProfileOwnerLocked(@Nullable ComponentName owner, int userHandle, + boolean hasIncompatibleAccounts) { UserInfo info = getUserInfo(userHandle); if (info == null) { // User doesn't exist. @@ -6539,7 +6542,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { } if (isAdb()) { if ((mIsWatch || hasUserSetupCompleted(userHandle)) - && hasIncompatibleAccountsLocked(userHandle, owner)) { + && hasIncompatibleAccounts) { throw new IllegalStateException("Not allowed to set the profile owner because " + "there are already some accounts on the profile"); } @@ -6556,12 +6559,14 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { * The Device owner can only be set by adb or an app with the MANAGE_PROFILE_AND_DEVICE_OWNERS * permission. */ - private void enforceCanSetDeviceOwnerLocked(@Nullable ComponentName owner, int userId) { + private void enforceCanSetDeviceOwnerLocked(@Nullable ComponentName owner, int userId, + boolean hasIncompatibleAccounts) { if (!isAdb()) { enforceCanManageProfileAndDeviceOwners(); } - final int code = checkDeviceOwnerProvisioningPreConditionLocked(owner, userId, isAdb()); + final int code = checkDeviceOwnerProvisioningPreConditionLocked( + owner, userId, isAdb(), hasIncompatibleAccounts); switch (code) { case CODE_OK: return; @@ -8846,7 +8851,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { * except for adb command if no accounts or additional users are present on the device. */ private int checkDeviceOwnerProvisioningPreConditionLocked(@Nullable ComponentName owner, - int deviceOwnerUserId, boolean isAdb) { + int deviceOwnerUserId, boolean isAdb, boolean hasIncompatibleAccounts) { if (mOwners.hasDeviceOwner()) { return CODE_HAS_DEVICE_OWNER; } @@ -8866,7 +8871,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { if (mUserManager.getUserCount() > 1) { return CODE_NONSYSTEM_USER_EXISTS; } - if (hasIncompatibleAccountsLocked(UserHandle.USER_SYSTEM, owner)) { + if (hasIncompatibleAccounts) { return CODE_ACCOUNTS_NOT_EMPTY; } } else { @@ -8893,8 +8898,9 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { private int checkDeviceOwnerProvisioningPreCondition(int deviceOwnerUserId) { synchronized (this) { + // hasIncompatibleAccounts doesn't matter since the caller is not adb. return checkDeviceOwnerProvisioningPreConditionLocked(/* owner unknown */ null, - deviceOwnerUserId, /* isAdb= */ false); + deviceOwnerUserId, /* isAdb= */ false, /* hasIncompatibleAccounts=*/ true); } } @@ -9791,8 +9797,15 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { * - Otherwise, if there's any account that does not have ..._ALLOWED, or does have * ..._DISALLOWED, return true. * - Otherwise return false. + * + * DO NOT CALL IT WITH THE DPMS LOCK HELD. */ - private boolean hasIncompatibleAccountsLocked(int userId, @Nullable ComponentName owner) { + private boolean hasIncompatibleAccountsNoLock(int userId, @Nullable ComponentName owner) { + if (Thread.holdsLock(this)) { + Slog.wtf(LOG_TAG, "hasIncompatibleAccountsNoLock() called with the DPMS lock held."); + return true; + } + final long token = mInjector.binderClearCallingIdentity(); try { final AccountManager am = AccountManager.get(mContext); @@ -9800,22 +9813,30 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { if (accounts.length == 0) { return false; } + synchronized (this) { + if (owner == null || !isAdminTestOnlyLocked(owner, userId)) { + Log.w(LOG_TAG, + "Non test-only owner can't be installed with existing accounts."); + return true; + } + } + final String[] feature_allow = { DevicePolicyManager.ACCOUNT_FEATURE_DEVICE_OR_PROFILE_OWNER_ALLOWED }; final String[] feature_disallow = { DevicePolicyManager.ACCOUNT_FEATURE_DEVICE_OR_PROFILE_OWNER_DISALLOWED }; - // Even if we find incompatible accounts along the way, we still check all accounts - // for logging. boolean compatible = true; for (Account account : accounts) { if (hasAccountFeatures(am, account, feature_disallow)) { Log.e(LOG_TAG, account + " has " + feature_disallow[0]); compatible = false; + break; } if (!hasAccountFeatures(am, account, feature_allow)) { Log.e(LOG_TAG, account + " doesn't have " + feature_allow[0]); compatible = false; + break; } } if (compatible) { @@ -9823,28 +9844,6 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub { } else { Log.e(LOG_TAG, "Found incompatible accounts"); } - - // Then check if the owner is test-only. - String log; - if (owner == null) { - // Owner is unknown. Suppose it's not test-only - compatible = false; - log = "Only test-only device/profile owner can be installed with accounts"; - } else if (isAdminTestOnlyLocked(owner, userId)) { - if (compatible) { - log = "Installing test-only owner " + owner; - } else { - log = "Can't install test-only owner " + owner + " with incompatible accounts"; - } - } else { - compatible = false; - log = "Can't install non test-only owner " + owner + " with accounts"; - } - if (compatible) { - Log.w(LOG_TAG, log); - } else { - Log.e(LOG_TAG, log); - } return !compatible; } finally { mInjector.binderRestoreCallingIdentity(token); diff --git a/services/tests/servicestests/src/com/android/server/devicepolicy/DpmMockContext.java b/services/tests/servicestests/src/com/android/server/devicepolicy/DpmMockContext.java index 1247b2d7005be..65255d9ef7ad1 100644 --- a/services/tests/servicestests/src/com/android/server/devicepolicy/DpmMockContext.java +++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DpmMockContext.java @@ -16,6 +16,8 @@ package com.android.server.devicepolicy; +import android.accounts.Account; +import android.accounts.AccountManager; import android.app.IActivityManager; import android.app.NotificationManager; import android.app.backup.IBackupManager; @@ -273,6 +275,7 @@ public class DpmMockContext extends MockContext { public final SettingsForMock settings; public final MockContentResolver contentResolver; public final TelephonyManager telephonyManager; + public final AccountManager accountManager; /** Note this is a partial mock, not a real mock. */ public final PackageManager packageManager; @@ -315,6 +318,7 @@ public class DpmMockContext extends MockContext { wifiManager = mock(WifiManager.class); settings = mock(SettingsForMock.class); telephonyManager = mock(TelephonyManager.class); + accountManager = mock(AccountManager.class); // Package manager is huge, so we use a partial mock instead. packageManager = spy(context.getPackageManager()); @@ -375,6 +379,7 @@ public class DpmMockContext extends MockContext { } } ); + when(accountManager.getAccountsAsUser(anyInt())).thenReturn(new Account[0]); // Create a data directory. final File dir = new File(dataDir, "user" + userId); @@ -464,6 +469,8 @@ public class DpmMockContext extends MockContext { return powerManager; case Context.WIFI_SERVICE: return wifiManager; + case Context.ACCOUNT_SERVICE: + return accountManager; } throw new UnsupportedOperationException(); }