Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c24e8ac5 authored by BK Choi's avatar BK Choi Committed by bkchoi
Browse files

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

This reverts commit ffe0e966f884b1e132c1b09af3b1c7dbda67d640.

Reason for revert: Fixed the issue from b/317683513. For a calling
profile user, we need to check if its parent user is an admin user.

Bug: 315382159

Test: atest BugreportManagerServiceImplTest \
            android.bugreport.cts.BugreportManagerTest
Test: Manual test

Change-Id: I5e570165aed373d38183a66b1f2124bab0b5878d
parent eac6f482

services/core/java/com/android/server/os/BugreportManagerServiceImpl.java

+45 −36
Original line number Diff line number Diff line
@@ -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);

        }



        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.

            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();

            }



        if (currentUser == null) {

            logAndThrow("There is no current user, so no bugreport can be requested.");

        } 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
Original line number Diff line number Diff line
@@ -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;

        }

    }

}