Merge "Check the calling user instead of the current user." into main
diff --git a/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java b/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
index 1660c3e..e546f42 100644
--- a/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
+++ b/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
@@ -21,13 +21,11 @@
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;
import android.content.Context;
import android.content.pm.PackageManager;
-import android.content.pm.UserInfo;
import android.os.Binder;
import android.os.BugreportManager.BugreportCallback;
import android.os.BugreportParams;
@@ -39,6 +37,7 @@
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 +95,7 @@
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;
@@ -346,6 +346,14 @@
AtomicFile getMappingFile() {
return mMappingFile;
}
+
+ UserManager getUserManager() {
+ return mContext.getSystemService(UserManager.class);
+ }
+
+ DevicePolicyManager getDevicePolicyManager() {
+ return mContext.getSystemService(DevicePolicyManager.class);
+ }
}
BugreportManagerServiceImpl(Context context) {
@@ -357,6 +365,7 @@
@VisibleForTesting(visibility = VisibleForTesting.Visibility.PRIVATE)
BugreportManagerServiceImpl(Injector injector) {
+ mInjector = injector;
mContext = injector.getContext();
mAppOps = mContext.getSystemService(AppOpsManager.class);
mTelephonyManager = mContext.getSystemService(TelephonyManager.class);
@@ -389,12 +398,7 @@
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) {
@@ -433,7 +437,6 @@
@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);
@@ -565,54 +568,48 @@
}
/**
- * 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 is not an admin user
*/
private void ensureUserCanTakeBugReport(int bugreportMode) {
- UserInfo currentUser = null;
+ // Get the calling userId before clearing the caller identity.
+ int callingUserId = 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.
+ isAdminUser = mInjector.getUserManager().isUserAdmin(callingUserId);
+ } finally {
+ Binder.restoreCallingIdentity(identity);
}
-
- if (currentUser == null) {
- logAndThrow("There is no current user, so no bugreport can be requested.");
- }
-
- if (!currentUser.isAdmin()) {
+ if (!isAdminUser) {
if (bugreportMode == BugreportParams.BUGREPORT_MODE_REMOTE
- && isCurrentUserAffiliated(currentUser.id)) {
+ && isUserAffiliated(callingUserId)) {
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 are allowed to take bugreport.", callingUserId));
}
}
/**
- * 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 dc1d2c5..21b8a94 100644
--- a/services/tests/servicestests/src/com/android/server/os/BugreportManagerServiceImplTest.java
+++ b/services/tests/servicestests/src/com/android/server/os/BugreportManagerServiceImplTest.java
@@ -16,23 +16,26 @@
package com.android.server.os;
-import android.app.admin.flags.Flags;
-import static android.app.admin.flags.Flags.onboardingBugreportV2Enabled;
-
import static com.android.compatibility.common.util.SystemUtil.runWithShellPermissionIdentity;
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;
+import static org.mockito.ArgumentMatchers.anyInt;
+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.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.RequiresFlagsEnabled;
import android.platform.test.flag.junit.CheckFlagsRule;
import android.platform.test.flag.junit.DeviceFlagsValueProvider;
@@ -48,6 +51,8 @@
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
import java.io.FileDescriptor;
import java.util.concurrent.CompletableFuture;
@@ -66,6 +71,11 @@
private BugreportManagerServiceImpl mService;
private BugreportManagerServiceImpl.BugreportFileManager mBugreportFileManager;
+ @Mock
+ private UserManager mMockUserManager;
+ @Mock
+ private DevicePolicyManager mMockDevicePolicyManager;
+
private int mCallingUid = 1234;
private String mCallingPackage = "test.package";
private AtomicFile mMappingFile;
@@ -75,14 +85,17 @@
@Before
public void setUp() {
+ MockitoAnnotations.initMocks(this);
mContext = InstrumentationRegistry.getInstrumentation().getContext();
mMappingFile = new AtomicFile(mContext.getFilesDir(), "bugreport-mapping.xml");
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);
+ // The calling user is an admin user by default.
+ when(mMockUserManager.isUserAdmin(anyInt())).thenReturn(true);
}
@After
@@ -165,6 +178,36 @@
}
@Test
+ public void testStartBugreport_throwsForNonAdminUser() throws Exception {
+ when(mMockUserManager.isUserAdmin(anyInt())).thenReturn(false);
+
+ Exception thrown = assertThrows(Exception.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(Exception.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);
@@ -207,7 +250,8 @@
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 {
@@ -258,4 +302,27 @@
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;
+ }
+ }
}