Skip to content
Snippets Groups Projects
Commit 0beed9fa authored by BK Choi's avatar BK Choi Committed by Android (Google) Code Review
Browse files

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

parents 1b9602d1 c24e8ac5
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
+ 45
− 36
View file @ 0beed9fa
......@@ -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;
}
......
services/tests/servicestests/src/com/android/server/os/BugreportManagerServiceImplTest.java
+ 67
− 5
View file @ 0beed9fa
......@@ -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
......@@ -181,6 +189,36 @@ public class BugreportManagerServiceImplTest {
/* forceUpdateMapping= */ true));
}
@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);
......@@ -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;
}
}
}
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment