Get account features before taking lock (cherry-pick from master)

Test: cts-tradefed run cts --skip-device-info --skip-preconditions --skip-system-status-check com.android.compatibility.common.tradefed.targetprep.NetworkConnectivityChecker -a armeabi-v7a -m CtsDevicePolicyManagerTestCases -t com.android.cts.devicepolicy.AccountCheckHostSideTest
* without having Id49f2bd5dfa80ecf35b3a23c789100ade38c2656 *

Test: adb shell am instrument -e class com.android.server.devicepolicy.DevicePolicyManagerTest -w com.android.frameworks.servicestests

Bug: 33481725
Change-Id: Ie2fe9aea87d1a7167581f4cd74ae063ef24a4567
Merged-in: I1e4dd9701a76ca366f86fdaf2fc6c282e9dbe5c1

diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java
index 46cc3a9..41c2e44 100644 (file)
--- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java
+++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java
@@ -5906,8 +5906,10 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub {
             throw new IllegalArgumentException("Invalid component " + admin
                     + " for device owner");
         }
+        final boolean hasIncompatibleAccountsOrNonAdb =
+                hasIncompatibleAccountsOrNonAdbNoLock(userId, admin);
         synchronized (this) {
-            enforceCanSetDeviceOwnerLocked(admin, userId);
+            enforceCanSetDeviceOwnerLocked(admin, userId, hasIncompatibleAccountsOrNonAdb);
             if (getActiveAdminUncheckedLocked(admin, userId) == null
                     || getUserData(userId).mRemovingAdmins.contains(admin)) {
                 throw new IllegalArgumentException("Not active admin: " + admin);
@@ -6098,8 +6100,10 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub {
             throw new IllegalArgumentException("Component " + who
                     + " not installed for userId:" + userHandle);
         }
+        final boolean hasIncompatibleAccountsOrNonAdb =
+                hasIncompatibleAccountsOrNonAdbNoLock(userHandle, who);
         synchronized (this) {
-            enforceCanSetProfileOwnerLocked(who, userHandle);
+            enforceCanSetProfileOwnerLocked(who, userHandle, hasIncompatibleAccountsOrNonAdb);
 
             if (getActiveAdminUncheckedLocked(who, userHandle) == null
                     || getUserData(userHandle).mRemovingAdmins.contains(who)) {
@@ -6420,9 +6424,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 hasIncompatibleAccountsOrNonAdb is true.
      */
-    private void enforceCanSetProfileOwnerLocked(@Nullable ComponentName owner, int userHandle) {
+    private void enforceCanSetProfileOwnerLocked(@Nullable ComponentName owner, int userHandle,
+            boolean hasIncompatibleAccountsOrNonAdb) {
         UserInfo info = getUserInfo(userHandle);
         if (info == null) {
             // User doesn't exist.
@@ -6443,7 +6448,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub {
         int callingUid = mInjector.binderGetCallingUid();
         if (callingUid == Process.SHELL_UID || callingUid == Process.ROOT_UID) {
             if ((mIsWatch || hasUserSetupCompleted(userHandle))
-                    && hasIncompatibleAccountsLocked(userHandle, owner)) {
+                    && hasIncompatibleAccountsOrNonAdb) {
                 throw new IllegalStateException("Not allowed to set the profile owner because "
                         + "there are already some accounts on the profile");
             }
@@ -6460,14 +6465,16 @@ 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 hasIncompatibleAccountsOrNonAdb) {
         int callingUid = mInjector.binderGetCallingUid();
         boolean isAdb = callingUid == Process.SHELL_UID || callingUid == Process.ROOT_UID;
         if (!isAdb) {
             enforceCanManageProfileAndDeviceOwners();
         }
 
-        final int code = checkSetDeviceOwnerPreConditionLocked(owner, userId, isAdb);
+        final int code = checkSetDeviceOwnerPreConditionLocked(owner, userId, isAdb,
+                hasIncompatibleAccountsOrNonAdb);
         switch (code) {
             case CODE_OK:
                 return;
@@ -8716,7 +8723,8 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub {
      * except for adb command if no accounts or additional users are present on the device.
      */
     private synchronized @DeviceOwnerPreConditionCode int checkSetDeviceOwnerPreConditionLocked(
-            @Nullable ComponentName owner, int deviceOwnerUserId, boolean isAdb) {
+            @Nullable ComponentName owner, int deviceOwnerUserId, boolean isAdb,
+            boolean hasIncompatibleAccountsOrNonAdb) {
         if (mOwners.hasDeviceOwner()) {
             return CODE_HAS_DEVICE_OWNER;
         }
@@ -8736,7 +8744,7 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub {
                     if (mUserManager.getUserCount() > 1) {
                         return CODE_NONSYSTEM_USER_EXISTS;
                     }
-                    if (hasIncompatibleAccountsLocked(UserHandle.USER_SYSTEM, owner)) {
+                    if (hasIncompatibleAccountsOrNonAdb) {
                         return CODE_ACCOUNTS_NOT_EMPTY;
                     }
                 } else {
@@ -8764,7 +8772,8 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub {
     private boolean isDeviceOwnerProvisioningAllowed(int deviceOwnerUserId) {
         synchronized (this) {
             return CODE_OK == checkSetDeviceOwnerPreConditionLocked(
-                    /* owner unknown */ null, deviceOwnerUserId, /* isAdb */ false);
+                    /* owner unknown */ null, deviceOwnerUserId, /* isAdb */ false,
+                    /* hasIncompatibleAccountsOrNonAdb=*/ true);
         }
     }
 
@@ -9358,8 +9367,25 @@ 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.
+     *
+     * If the caller is *not* ADB, it also returns true.  The returned value shouldn't be used
+     * when the caller is not ADB.
+     *
+     * DO NOT CALL IT WITH THE DPMS LOCK HELD.
      */
-    private boolean hasIncompatibleAccountsLocked(int userId, @Nullable ComponentName owner) {
+    private boolean hasIncompatibleAccountsOrNonAdbNoLock(
+            int userId, @Nullable ComponentName owner) {
+        final boolean isAdb = (mInjector.binderGetCallingUid() == Process.SHELL_UID)
+                || (mInjector.binderGetCallingUid() == Process.ROOT_UID);
+        if (!isAdb) {
+            return true;
+        }
+
+        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);
@@ -9367,22 +9393,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) {
@@ -9390,28 +9424,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 0783afc..956d83a 100644 (file)
--- a/services/tests/servicestests/src/com/android/server/devicepolicy/DpmMockContext.java
+++ b/services/tests/servicestests/src/com/android/server/devicepolicy/DpmMockContext.java
@@ -16,8 +16,15 @@
 
 package com.android.server.devicepolicy;
 
-import com.android.internal.widget.LockPatternUtils;
+import static org.mockito.Matchers.anyBoolean;
+import static org.mockito.Matchers.anyInt;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
 
+import android.accounts.Account;
+import android.accounts.AccountManager;
 import android.app.IActivityManager;
 import android.app.NotificationManager;
 import android.app.backup.IBackupManager;
@@ -44,6 +51,8 @@ import android.test.mock.MockContentResolver;
 import android.test.mock.MockContext;
 import android.view.IWindowManager;
 
+import com.android.internal.widget.LockPatternUtils;
+
 import org.junit.Assert;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
@@ -52,13 +61,6 @@ import java.io.File;
 import java.util.ArrayList;
 import java.util.List;
 
-import static org.mockito.Matchers.anyBoolean;
-import static org.mockito.Matchers.anyInt;
-import static org.mockito.Matchers.eq;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.when;
-
 /**
  * Context used throughout DPMS tests.
  */
@@ -264,6 +266,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;
@@ -298,6 +301,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());
@@ -360,6 +364,7 @@ public class DpmMockContext extends MockContext {
                     }
                 }
         );
+        when(accountManager.getAccountsAsUser(anyInt())).thenReturn(new Account[0]);
 
 
         // Create a data directory.
@@ -418,6 +423,8 @@ public class DpmMockContext extends MockContext {
                 return powerManager;
             case Context.WIFI_SERVICE:
                 return wifiManager;
+            case Context.ACCOUNT_SERVICE:
+                return accountManager;
         }
         throw new UnsupportedOperationException();
     }