Merge "Revert^2 "Check the calling user instead of the current user."" into main

2 files changed, 112 insertions, 41 deletions

diff --git a/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java b/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
index 8452c0e61a81..4f86adfe2d8d 100644
--- a/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
+++ b/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
@@ -21,7 +21,6 @@ import static android.app.admin.flags.Flags.onboardingBugreportV2Enabled;
 import android.Manifest;
 import android.annotation.Nullable;
 import android.annotation.RequiresPermission;
-import android.app.ActivityManager;
 import android.app.AppOpsManager;
 import android.app.admin.DevicePolicyManager;
 import android.app.role.RoleManager;
@@ -39,6 +38,7 @@ import android.os.ServiceManager;
 import android.os.SystemClock;
 import android.os.SystemProperties;
 import android.os.UserHandle;
+import android.os.UserManager;
 import android.telephony.TelephonyManager;
 import android.text.TextUtils;
 import android.util.ArrayMap;
@@ -96,6 +96,7 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub {
     private static final long DEFAULT_BUGREPORT_SERVICE_TIMEOUT_MILLIS = 30 * 1000;
 
     private final Object mLock = new Object();
+    private final Injector mInjector;
     private final Context mContext;
     private final AppOpsManager mAppOps;
     private final TelephonyManager mTelephonyManager;
@@ -345,6 +346,14 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub {
         AtomicFile getMappingFile() {
             return mMappingFile;
         }
+
+        UserManager getUserManager() {
+            return mContext.getSystemService(UserManager.class);
+        }
+
+        DevicePolicyManager getDevicePolicyManager() {
+            return mContext.getSystemService(DevicePolicyManager.class);
+        }
     }
 
     BugreportManagerServiceImpl(Context context) {
@@ -356,6 +365,7 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub {
 
     @VisibleForTesting(visibility = VisibleForTesting.Visibility.PRIVATE)
     BugreportManagerServiceImpl(Injector injector) {
+        mInjector = injector;
         mContext = injector.getContext();
         mAppOps = mContext.getSystemService(AppOpsManager.class);
         mTelephonyManager = mContext.getSystemService(TelephonyManager.class);
@@ -388,12 +398,7 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub {
         int callingUid = Binder.getCallingUid();
         enforcePermission(callingPackage, callingUid, bugreportMode
                 == BugreportParams.BUGREPORT_MODE_TELEPHONY /* checkCarrierPrivileges */);
-        final long identity = Binder.clearCallingIdentity();
-        try {
-            ensureUserCanTakeBugReport(bugreportMode);
-        } finally {
-            Binder.restoreCallingIdentity(identity);
-        }
+        ensureUserCanTakeBugReport(bugreportMode);
 
         Slogf.i(TAG, "Starting bugreport for %s / %d", callingPackage, callingUid);
         synchronized (mLock) {
@@ -432,7 +437,6 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub {
     @RequiresPermission(value = Manifest.permission.DUMP, conditional = true)
     public void retrieveBugreport(int callingUidUnused, String callingPackage, int userId,
             FileDescriptor bugreportFd, String bugreportFile,
-
             boolean keepBugreportOnRetrievalUnused, IDumpstateListener listener) {
         int callingUid = Binder.getCallingUid();
         enforcePermission(callingPackage, callingUid, false);
@@ -564,54 +568,59 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub {
     }
 
     /**
-     * Validates that the current user is an admin user or, when bugreport is requested remotely
-     * that the current user is an affiliated user.
+     * Validates that the calling user is an admin user or, when bugreport is requested remotely
+     * that the user is an affiliated user.
      *
-     * @throws IllegalArgumentException if the current user is not an admin user
+     * @throws IllegalArgumentException if the calling user or the parent of the calling profile
+     *                                  user is not an admin user.
      */
     private void ensureUserCanTakeBugReport(int bugreportMode) {
-        UserInfo currentUser = null;
+        // Get the calling userId before clearing the caller identity.
+        int effectiveCallingUserId = UserHandle.getUserId(Binder.getCallingUid());
+        boolean isAdminUser = false;
+        final long identity = Binder.clearCallingIdentity();
         try {
-            currentUser = ActivityManager.getService().getCurrentUser();
-        } catch (RemoteException e) {
-            // Impossible to get RemoteException for an in-process call.
-        }
-
-        if (currentUser == null) {
-            logAndThrow("There is no current user, so no bugreport can be requested.");
+            UserInfo profileParent =
+                    mInjector.getUserManager().getProfileParent(effectiveCallingUserId);
+            if (profileParent == null) {
+                isAdminUser = mInjector.getUserManager().isUserAdmin(effectiveCallingUserId);
+            } else {
+                // If the caller is a profile, we need to check its parent user instead.
+                // Therefore setting the profile parent user as the effective calling user.
+                effectiveCallingUserId = profileParent.id;
+                isAdminUser = profileParent.isAdmin();
+            }
+        } finally {
+            Binder.restoreCallingIdentity(identity);
         }
-
-        if (!currentUser.isAdmin()) {
+        if (!isAdminUser) {
             if (bugreportMode == BugreportParams.BUGREPORT_MODE_REMOTE
-                    && isCurrentUserAffiliated(currentUser.id)) {
+                    && isUserAffiliated(effectiveCallingUserId)) {
                 return;
             }
-            logAndThrow(TextUtils.formatSimple("Current user %s is not an admin user."
-                    + " Only admin users are allowed to take bugreport.", currentUser.id));
+            logAndThrow(TextUtils.formatSimple("Calling user %s is not an admin user."
+                    + " Only admin users and their profiles are allowed to take bugreport.",
+                    effectiveCallingUserId));
         }
     }
 
     /**
-     * Returns {@code true} if the device has device owner and the current user is affiliated
+     * Returns {@code true} if the device has device owner and the specified user is affiliated
      * with the device owner.
      */
-    private boolean isCurrentUserAffiliated(int currentUserId) {
-        DevicePolicyManager dpm = mContext.getSystemService(DevicePolicyManager.class);
+    private boolean isUserAffiliated(int userId) {
+        DevicePolicyManager dpm = mInjector.getDevicePolicyManager();
         int deviceOwnerUid = dpm.getDeviceOwnerUserId();
         if (deviceOwnerUid == UserHandle.USER_NULL) {
             return false;
         }
 
-        int callingUserId = UserHandle.getUserId(Binder.getCallingUid());
-
-        Slog.i(TAG, "callingUid: " + callingUserId + " deviceOwnerUid: " + deviceOwnerUid
-                + " currentUserId: " + currentUserId);
-
-        if (callingUserId != deviceOwnerUid) {
-            logAndThrow("Caller is not device owner on provisioned device.");
+        if (DEBUG) {
+            Slog.d(TAG, "callingUid: " + userId + " deviceOwnerUid: " + deviceOwnerUid);
         }
-        if (!dpm.isAffiliatedUser(currentUserId)) {
-            logAndThrow("Current user is not affiliated to the device owner.");
+
+        if (userId != deviceOwnerUid && !dpm.isAffiliatedUser(userId)) {
+            logAndThrow("User " + userId + " is not affiliated to the device owner.");
         }
         return true;
     }
diff --git a/services/tests/servicestests/src/com/android/server/os/BugreportManagerServiceImplTest.java b/services/tests/servicestests/src/com/android/server/os/BugreportManagerServiceImplTest.java
index ea84eb2fbf73..e0ef035de735 100644
--- a/services/tests/servicestests/src/com/android/server/os/BugreportManagerServiceImplTest.java
+++ b/services/tests/servicestests/src/com/android/server/os/BugreportManagerServiceImplTest.java
@@ -16,8 +16,6 @@
 
 package com.android.server.os;
 
-import android.app.admin.flags.Flags;
-
 import static com.android.compatibility.common.util.SystemUtil.runWithShellPermissionIdentity;
 
 import static com.google.common.truth.Truth.assertThat;
@@ -27,15 +25,19 @@ import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.when;
 
+import android.app.admin.DevicePolicyManager;
+import android.app.admin.flags.Flags;
 import android.app.role.RoleManager;
 import android.content.Context;
 import android.content.pm.PackageManager;
 import android.os.Binder;
 import android.os.BugreportManager.BugreportCallback;
+import android.os.BugreportParams;
 import android.os.IBinder;
 import android.os.IDumpstateListener;
 import android.os.Process;
 import android.os.RemoteException;
+import android.os.UserManager;
 import android.platform.test.annotations.RequiresFlagsDisabled;
 import android.platform.test.annotations.RequiresFlagsEnabled;
 import android.platform.test.flag.junit.CheckFlagsRule;
@@ -75,6 +77,10 @@ public class BugreportManagerServiceImplTest {
 
     @Mock
     private PackageManager mPackageManager;
+    @Mock
+    private UserManager mMockUserManager;
+    @Mock
+    private DevicePolicyManager mMockDevicePolicyManager;
 
     private int mCallingUid = 1234;
     private String mCallingPackage  = "test.package";
@@ -91,10 +97,12 @@ public class BugreportManagerServiceImplTest {
         ArraySet<String> mAllowlistedPackages = new ArraySet<>();
         mAllowlistedPackages.add(mContext.getPackageName());
         mService = new BugreportManagerServiceImpl(
-                new BugreportManagerServiceImpl.Injector(mContext, mAllowlistedPackages,
-                        mMappingFile));
+                new TestInjector(mContext, mAllowlistedPackages, mMappingFile,
+                        mMockUserManager, mMockDevicePolicyManager));
         mBugreportFileManager = new BugreportManagerServiceImpl.BugreportFileManager(mMappingFile);
         when(mPackageManager.getPackageUidAsUser(anyString(), anyInt())).thenReturn(mCallingUid);
+        // The calling user is an admin user by default.
+        when(mMockUserManager.isUserAdmin(anyInt())).thenReturn(true);
     }
 
     @After
@@ -182,6 +190,36 @@ public class BugreportManagerServiceImplTest {
     }
 
     @Test
+    public void testStartBugreport_throwsForNonAdminUser() throws Exception {
+        when(mMockUserManager.isUserAdmin(anyInt())).thenReturn(false);
+
+        Exception thrown = assertThrows(IllegalArgumentException.class,
+                () -> mService.startBugreport(mCallingUid, mContext.getPackageName(),
+                        new FileDescriptor(), /* screenshotFd= */ null,
+                        BugreportParams.BUGREPORT_MODE_FULL,
+                        /* flags= */ 0, new Listener(new CountDownLatch(1)),
+                        /* isScreenshotRequested= */ false));
+
+        assertThat(thrown.getMessage()).contains("not an admin user");
+    }
+
+    @Test
+    public void testStartBugreport_throwsForNotAffiliatedUser() throws Exception {
+        when(mMockUserManager.isUserAdmin(anyInt())).thenReturn(false);
+        when(mMockDevicePolicyManager.getDeviceOwnerUserId()).thenReturn(-1);
+        when(mMockDevicePolicyManager.isAffiliatedUser(anyInt())).thenReturn(false);
+
+        Exception thrown = assertThrows(IllegalArgumentException.class,
+                () -> mService.startBugreport(mCallingUid, mContext.getPackageName(),
+                        new FileDescriptor(), /* screenshotFd= */ null,
+                        BugreportParams.BUGREPORT_MODE_REMOTE,
+                        /* flags= */ 0, new Listener(new CountDownLatch(1)),
+                        /* isScreenshotRequested= */ false));
+
+        assertThat(thrown.getMessage()).contains("not affiliated to the device owner");
+    }
+
+    @Test
     public void testRetrieveBugreportWithoutFilesForCaller() throws Exception {
         CountDownLatch latch = new CountDownLatch(1);
         Listener listener = new Listener(latch);
@@ -224,7 +262,8 @@ public class BugreportManagerServiceImplTest {
 
     private void clearAllowlist() {
         mService = new BugreportManagerServiceImpl(
-                new BugreportManagerServiceImpl.Injector(mContext, new ArraySet<>(), mMappingFile));
+                new TestInjector(mContext, new ArraySet<>(), mMappingFile,
+                        mMockUserManager, mMockDevicePolicyManager));
     }
 
     private static class Listener implements IDumpstateListener {
@@ -275,4 +314,27 @@ public class BugreportManagerServiceImplTest {
             complete(successful);
         }
     }
+
+    private static class TestInjector extends BugreportManagerServiceImpl.Injector {
+
+        private final UserManager mUserManager;
+        private final DevicePolicyManager mDevicePolicyManager;
+
+        TestInjector(Context context, ArraySet<String> allowlistedPackages, AtomicFile mappingFile,
+                UserManager um, DevicePolicyManager dpm) {
+            super(context, allowlistedPackages, mappingFile);
+            mUserManager = um;
+            mDevicePolicyManager = dpm;
+        }
+
+        @Override
+        public UserManager getUserManager() {
+            return mUserManager;
+        }
+
+        @Override
+        public DevicePolicyManager getDevicePolicyManager() {
+            return mDevicePolicyManager;
+        }
+    }
 }