Improve `dumpsys backup`

This is a re-submit of the previously reverted ag/22327091 by
piee@google.com.

Original description:
This change refactors `dumpsys backup` code and adds some improvements -
	- Removes `isUserReadyForBackup()` check on calling user. This check is not needed, as it prevents a User with INTERACT_ACROSS_USERS_FULL permission, but not Backup activated, to get info about Backup service on other users. This is particularly relevant in case of headless mode, as Backup service isn't running for SYSTEM_USER.
	- Adds `--user <userId>` optional parameter for `dumpsys backup`, which will return info about Backup service for the user passed in argument. This is also helpul in case of multiple users on the device.

Changes by sarpm@google.com over the original:
	- Some test improvements: remove @VisibleForTesting, increase
	  coverage slightly.
	- Remove redundant robo tests that are now covered by the unit
	  tests.

Bug: 267755698
Test: atest BackupManagerServiceTest
	Manual test by running dumpsys backup with optional arguments.
Change-Id: Id7790a501a8befb12a212f66adfc0cd8cc9f9e8b

3 files changed, 146 insertions, 123 deletions

diff --git a/services/backup/java/com/android/server/backup/BackupManagerService.java b/services/backup/java/com/android/server/backup/BackupManagerService.java
index ce9cdc2aeab5..0353d5a962c7 100644
--- a/services/backup/java/com/android/server/backup/BackupManagerService.java
+++ b/services/backup/java/com/android/server/backup/BackupManagerService.java
@@ -1510,46 +1510,74 @@ public class BackupManagerService extends IBackupManager.Stub {
         if (!DumpUtils.checkDumpAndUsageStatsPermission(mContext, TAG, pw)) {
             return;
         }
-        dumpWithoutCheckingPermission(fd, pw, args);
-    }
 
-    @VisibleForTesting
-    void dumpWithoutCheckingPermission(FileDescriptor fd, PrintWriter pw, String[] args) {
-        int userId = binderGetCallingUserId();
-        if (!isUserReadyForBackup(userId)) {
-            pw.println("Inactive");
+        int argIndex = 0;
+
+        String op = nextArg(args, argIndex);
+        argIndex++;
+
+        if ("--help".equals(op)) {
+            showDumpUsage(pw);
             return;
         }
-
-        if (args != null) {
-            for (String arg : args) {
-                if ("-h".equals(arg)) {
-                    pw.println("'dumpsys backup' optional arguments:");
-                    pw.println("  -h       : this help text");
-                    pw.println("  a[gents] : dump information about defined backup agents");
-                    pw.println("  transportclients : dump information about transport clients");
-                    pw.println("  transportstats : dump transport statts");
-                    pw.println("  users    : dump the list of users for which backup service "
-                            + "is running");
-                    return;
-                } else if ("users".equals(arg.toLowerCase())) {
-                    pw.print(DUMP_RUNNING_USERS_MESSAGE);
-                    for (int i = 0; i < mUserServices.size(); i++) {
-                        pw.print(" " + mUserServices.keyAt(i));
-                    }
-                    pw.println();
-                    return;
+        if ("users".equals(op)) {
+            pw.print(DUMP_RUNNING_USERS_MESSAGE);
+            for (int i = 0; i < mUserServices.size(); i++) {
+                UserBackupManagerService userBackupManagerService =
+                        getServiceForUserIfCallerHasPermission(mUserServices.keyAt(i),
+                                "dump()");
+                if (userBackupManagerService != null) {
+                    pw.print(" " + userBackupManagerService.getUserId());
                 }
             }
+            pw.println();
+            return;
         }
-
-        for (int i = 0; i < mUserServices.size(); i++) {
+        if ("--user".equals(op)) {
+            String userArg = nextArg(args, argIndex);
+            argIndex++;
+            if (userArg == null) {
+                showDumpUsage(pw);
+                return;
+            }
+            int userId = UserHandle.parseUserArg(userArg);
             UserBackupManagerService userBackupManagerService =
-                    getServiceForUserIfCallerHasPermission(mUserServices.keyAt(i), "dump()");
+                    getServiceForUserIfCallerHasPermission(userId, "dump()");
             if (userBackupManagerService != null) {
                 userBackupManagerService.dump(fd, pw, args);
             }
+            return;
         }
+        if (op == null || "agents".startsWith(op) || "transportclients".equals(op)
+                || "transportstats".equals(op)) {
+            for (int i = 0; i < mUserServices.size(); i++) {
+                UserBackupManagerService userBackupManagerService =
+                        getServiceForUserIfCallerHasPermission(mUserServices.keyAt(i), "dump()");
+                if (userBackupManagerService != null) {
+                    userBackupManagerService.dump(fd, pw, args);
+                }
+            }
+            return;
+        }
+
+        showDumpUsage(pw);
+    }
+
+    private String nextArg(String[] args, int argIndex) {
+        if (argIndex >= args.length) {
+            return null;
+        }
+        return args[argIndex];
+    }
+
+    private static void showDumpUsage(PrintWriter pw) {
+        pw.println("'dumpsys backup' optional arguments:");
+        pw.println("  --help    : this help text");
+        pw.println("  a[gents] : dump information about defined backup agents");
+        pw.println("  transportclients : dump information about transport clients");
+        pw.println("  transportstats : dump transport stats");
+        pw.println("  users    : dump the list of users for which backup service is running");
+        pw.println("  --user <userId> : dump information for user userId");
     }
 
     /**
@@ -1661,7 +1689,7 @@ public class BackupManagerService extends IBackupManager.Stub {
      * @param message A message to include in the exception if it is thrown.
      */
     void enforceCallingPermissionOnUserId(@UserIdInt int userId, String message) {
-        if (Binder.getCallingUserHandle().getIdentifier() != userId) {
+        if (binderGetCallingUserId() != userId) {
             mContext.enforceCallingOrSelfPermission(
                     Manifest.permission.INTERACT_ACROSS_USERS_FULL, message);
         }
diff --git a/services/robotests/backup/src/com/android/server/backup/BackupManagerServiceRoboTest.java b/services/robotests/backup/src/com/android/server/backup/BackupManagerServiceRoboTest.java
index 94ee0a871448..a547d0f94ea3 100644
--- a/services/robotests/backup/src/com/android/server/backup/BackupManagerServiceRoboTest.java
+++ b/services/robotests/backup/src/com/android/server/backup/BackupManagerServiceRoboTest.java
@@ -17,9 +17,7 @@
 package com.android.server.backup;
 
 import static android.Manifest.permission.BACKUP;
-import static android.Manifest.permission.DUMP;
 import static android.Manifest.permission.INTERACT_ACROSS_USERS_FULL;
-import static android.Manifest.permission.PACKAGE_USAGE_STATS;
 
 import static com.android.server.backup.testing.TransportData.backupTransport;
 
@@ -73,10 +71,7 @@ import org.robolectric.shadow.api.Shadow;
 import org.robolectric.shadows.ShadowContextWrapper;
 
 import java.io.File;
-import java.io.FileDescriptor;
 import java.io.IOException;
-import java.io.PrintWriter;
-import java.io.StringWriter;
 
 /** Tests for {@link BackupManagerService}. */
 @RunWith(RobolectricTestRunner.class)
@@ -1461,67 +1456,12 @@ public class BackupManagerServiceRoboTest {
     //  Service tests
     // ---------------------------------------------
 
-    /** Test that the backup service routes methods correctly to the user that requests it. */
-    @Test
-    public void testDump_onRegisteredUser_callsMethodForUser() throws Exception {
-        grantDumpPermissions();
-        BackupManagerService backupManagerService = createSystemRegisteredService();
-        File testFile = createTestFile();
-        FileDescriptor fileDescriptor = new FileDescriptor();
-        PrintWriter printWriter = new PrintWriter(testFile);
-        String[] args = {"1", "2"};
-        ShadowBinder.setCallingUserHandle(UserHandle.of(UserHandle.USER_SYSTEM));
-
-        backupManagerService.dump(fileDescriptor, printWriter, args);
-
-        verify(mUserSystemService).dump(fileDescriptor, printWriter, args);
-    }
-
-    /** Test that the backup service does not route methods for non-registered users. */
-    @Test
-    public void testDump_onUnknownUser_doesNotPropagateCall() throws Exception {
-        grantDumpPermissions();
-        BackupManagerService backupManagerService = createService();
-        File testFile = createTestFile();
-        FileDescriptor fileDescriptor = new FileDescriptor();
-        PrintWriter printWriter = new PrintWriter(testFile);
-        String[] args = {"1", "2"};
-
-        backupManagerService.dump(fileDescriptor, printWriter, args);
-
-        verify(mUserOneService, never()).dump(fileDescriptor, printWriter, args);
-    }
-
-    /** Test that 'dumpsys backup users' dumps the list of users registered in backup service*/
-    @Test
-    public void testDump_users_dumpsListOfRegisteredUsers() {
-        grantDumpPermissions();
-        BackupManagerService backupManagerService = createSystemRegisteredService();
-        registerUser(backupManagerService, mUserOneId, mUserOneService);
-        StringWriter out = new StringWriter();
-        PrintWriter writer = new PrintWriter(out);
-        String[] args = {"users"};
-
-        backupManagerService.dump(null, writer, args);
-
-        writer.flush();
-        assertEquals(
-                String.format("%s %d %d\n", BackupManagerService.DUMP_RUNNING_USERS_MESSAGE,
-                        UserHandle.USER_SYSTEM, mUserOneId),
-                out.toString());
-    }
-
     private File createTestFile() throws IOException {
         File testFile = new File(mContext.getFilesDir(), "test");
         testFile.createNewFile();
         return testFile;
     }
 
-    private void grantDumpPermissions() {
-        mShadowContext.grantPermissions(DUMP);
-        mShadowContext.grantPermissions(PACKAGE_USAGE_STATS);
-    }
-
     /**
      * Test that the backup services throws a {@link SecurityException} if the caller does not have
      * INTERACT_ACROSS_USERS_FULL permission and passes a different user id.
diff --git a/services/tests/mockingservicestests/src/com/android/server/backup/BackupManagerServiceTest.java b/services/tests/mockingservicestests/src/com/android/server/backup/BackupManagerServiceTest.java
index b203cf640097..c4a042370de5 100644
--- a/services/tests/mockingservicestests/src/com/android/server/backup/BackupManagerServiceTest.java
+++ b/services/tests/mockingservicestests/src/com/android/server/backup/BackupManagerServiceTest.java
@@ -38,7 +38,6 @@ import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import android.Manifest;
-import android.annotation.UserIdInt;
 import android.app.backup.BackupManager;
 import android.app.backup.ISelectBackupTransportCallback;
 import android.app.job.JobScheduler;
@@ -59,10 +58,12 @@ import androidx.test.ext.junit.runners.AndroidJUnit4;
 import androidx.test.filters.SmallTest;
 
 import com.android.dx.mockito.inline.extended.ExtendedMockito;
+import com.android.internal.util.DumpUtils;
 import com.android.server.SystemService;
 import com.android.server.backup.utils.RandomAccessFileUtils;
 
 import org.junit.After;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -74,6 +75,7 @@ import org.mockito.quality.Strictness;
 import java.io.File;
 import java.io.FileDescriptor;
 import java.io.PrintWriter;
+import java.io.Writer;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.TimeUnit;
 
@@ -85,8 +87,6 @@ public class BackupManagerServiceTest {
             "class");
     private static final int NON_SYSTEM_USER = UserHandle.USER_SYSTEM + 1;
 
-    @UserIdInt
-    private int mUserId;
     @Mock
     private UserBackupManagerService mSystemUserBackupManagerService;
     @Mock
@@ -94,8 +94,6 @@ public class BackupManagerServiceTest {
     @Mock
     private Context mContextMock;
     @Mock
-    private PrintWriter mPrintWriterMock;
-    @Mock
     private UserManager mUserManagerMock;
     @Mock
     private UserInfo mUserInfoMock;
@@ -104,6 +102,7 @@ public class BackupManagerServiceTest {
 
     private BackupManagerServiceTestable mService;
     private BackupManagerService.Lifecycle mServiceLifecycle;
+    private FakePrintWriter mFakePrintWriter;
     private static File sTestDir;
     private MockitoSession mSession;
 
@@ -114,6 +113,7 @@ public class BackupManagerServiceTest {
                                 this)
                         .strictness(Strictness.LENIENT)
                         .spyStatic(UserBackupManagerService.class)
+                        .spyStatic(DumpUtils.class)
                         .startMocking();
         doReturn(mSystemUserBackupManagerService).when(
                 () -> UserBackupManagerService.createAndInitializeService(
@@ -122,8 +122,7 @@ public class BackupManagerServiceTest {
                 () -> UserBackupManagerService.createAndInitializeService(eq(NON_SYSTEM_USER),
                         any(), any(), any()));
 
-        mUserId = UserHandle.USER_SYSTEM;
-
+        when(mNonSystemUserBackupManagerService.getUserId()).thenReturn(NON_SYSTEM_USER);
         when(mUserManagerMock.getUserInfo(UserHandle.USER_SYSTEM)).thenReturn(mUserInfoMock);
         when(mUserManagerMock.getUserInfo(NON_SYSTEM_USER)).thenReturn(mUserInfoMock);
         // Null main user means there is no main user on the device.
@@ -139,6 +138,8 @@ public class BackupManagerServiceTest {
 
         when(mContextMock.getSystemService(Context.JOB_SCHEDULER_SERVICE))
                 .thenReturn(mock(JobScheduler.class));
+
+        mFakePrintWriter = new FakePrintWriter();
     }
 
     @After
@@ -552,7 +553,8 @@ public class BackupManagerServiceTest {
                     }
                 };
 
-        mService.selectBackupTransportAsyncForUser(mUserId, TRANSPORT_COMPONENT_NAME, listener);
+        mService.selectBackupTransportAsyncForUser(
+                UserHandle.USER_SYSTEM, TRANSPORT_COMPONENT_NAME, listener);
 
         assertEquals(BackupManager.ERROR_BACKUP_NOT_ALLOWED, (int) future.get(5, TimeUnit.SECONDS));
     }
@@ -560,9 +562,10 @@ public class BackupManagerServiceTest {
     @Test
     public void selectBackupTransportAsyncForUser_beforeUserUnlockedWithNullListener_doesNotThrow()
             throws Exception {
-        createBackupManagerServiceAndUnlockSystemUser();
+        mService = new BackupManagerServiceTestable(mContextMock);
 
-        mService.selectBackupTransportAsyncForUser(mUserId, TRANSPORT_COMPONENT_NAME, null);
+        mService.selectBackupTransportAsyncForUser(
+                UserHandle.USER_SYSTEM, TRANSPORT_COMPONENT_NAME, null);
 
         // No crash.
     }
@@ -570,13 +573,11 @@ public class BackupManagerServiceTest {
     @Test
     public void selectBackupTransportAsyncForUser_beforeUserUnlockedListenerThrowing_doesNotThrow()
             throws Exception {
-        createBackupManagerServiceAndUnlockSystemUser();
-
+        mService = new BackupManagerServiceTestable(mContextMock);
         ISelectBackupTransportCallback.Stub listener =
                 new ISelectBackupTransportCallback.Stub() {
                     @Override
-                    public void onSuccess(String transportName) {
-                    }
+                    public void onSuccess(String transportName) {}
 
                     @Override
                     public void onFailure(int reason) throws RemoteException {
@@ -584,55 +585,91 @@ public class BackupManagerServiceTest {
                     }
                 };
 
-        mService.selectBackupTransportAsyncForUser(mUserId, TRANSPORT_COMPONENT_NAME, listener);
+        mService.selectBackupTransportAsyncForUser(
+                UserHandle.USER_SYSTEM, TRANSPORT_COMPONENT_NAME, listener);
 
         // No crash.
     }
 
     @Test
-    public void dump_callerDoesNotHaveDumpPermission_ignored() {
+    public void dump_callerDoesNotHaveDumpOrUsageStatsPermission_ignored() {
+        mockDumpPermissionsGranted(false);
         createBackupManagerServiceAndUnlockSystemUser();
-        when(mContextMock.checkCallingOrSelfPermission(
-                Manifest.permission.DUMP)).thenReturn(
-                PackageManager.PERMISSION_DENIED);
+        when(mContextMock.checkCallingOrSelfPermission(Manifest.permission.DUMP))
+                .thenReturn(PackageManager.PERMISSION_DENIED);
 
-        mService.dump(mFileDescriptorStub, mPrintWriterMock, new String[0]);
+        mService.dump(mFileDescriptorStub, mFakePrintWriter, new String[0]);
 
         verify(mSystemUserBackupManagerService, never()).dump(any(), any(), any());
         verify(mNonSystemUserBackupManagerService, never()).dump(any(), any(), any());
     }
 
     @Test
-    public void dump_callerDoesNotHavePackageUsageStatsPermission_ignored() {
+    public void dump_forOneUser_callerDoesNotHaveInteractAcrossUsersFullPermission_ignored() {
+        mockDumpPermissionsGranted(true);
         createBackupManagerServiceAndUnlockSystemUser();
-        when(mContextMock.checkCallingOrSelfPermission(
-                Manifest.permission.PACKAGE_USAGE_STATS)).thenReturn(
-                PackageManager.PERMISSION_DENIED);
+        mService.setBackupServiceActive(NON_SYSTEM_USER, true);
+        simulateUserUnlocked(NON_SYSTEM_USER);
+        doThrow(new SecurityException())
+                .when(mContextMock)
+                .enforceCallingOrSelfPermission(
+                        eq(Manifest.permission.INTERACT_ACROSS_USERS_FULL), anyString());
 
-        mService.dump(mFileDescriptorStub, mPrintWriterMock, new String[0]);
+        String[] args = new String[] {"--user", Integer.toString(NON_SYSTEM_USER)};
+        Assert.assertThrows(
+                SecurityException.class,
+                () -> mService.dump(mFileDescriptorStub, mFakePrintWriter, args));
 
+        verify(mNonSystemUserBackupManagerService, never()).dump(any(), any(), any());
+    }
+
+    @Test
+    public void dump_forOneUser_callerHasInteractAcrossUsersFullPermission_dumpsSpecifiedUser() {
+        mockDumpPermissionsGranted(true);
+        createBackupManagerServiceAndUnlockSystemUser();
+        mService.setBackupServiceActive(NON_SYSTEM_USER, true);
+        simulateUserUnlocked(NON_SYSTEM_USER);
+
+        String[] args = new String[] {"--user", Integer.toString(UserHandle.USER_SYSTEM)};
+        mService.dump(mFileDescriptorStub, mFakePrintWriter, args);
+
+        verify(mSystemUserBackupManagerService).dump(any(), any(), any());
+    }
+
+    @Test
+    public void dump_users_callerHasInteractAcrossUsersFullPermission_dumpsUsers() {
+        mockDumpPermissionsGranted(true);
+        createBackupManagerServiceAndUnlockSystemUser();
+        mService.setBackupServiceActive(NON_SYSTEM_USER, true);
+        simulateUserUnlocked(NON_SYSTEM_USER);
+
+        String[] args = new String[] {"users"};
+        mService.dump(mFileDescriptorStub, mFakePrintWriter, args);
+
+        // Check that dump() invocations are not called on user's Backup service,
+        // as 'dumpsys backup users' only list users for whom Backup service is running.
         verify(mSystemUserBackupManagerService, never()).dump(any(), any(), any());
         verify(mNonSystemUserBackupManagerService, never()).dump(any(), any(), any());
+        assertThat(mFakePrintWriter.mPrintedSoFar)
+                .isEqualTo("Backup Manager is running for users: 0 1");
     }
 
-    /**
-     * Test that {@link BackupManagerService#dump(FileDescriptor, PrintWriter, String[])} dumps
-     * system user information before non-system user information.
-     */
     @Test
-    public void testDump_systemUserFirst() {
+    public void dump_allUsers_dumpsSystemUserFirst() {
+        mockDumpPermissionsGranted(true);
         createBackupManagerServiceAndUnlockSystemUser();
         mService.setBackupServiceActive(NON_SYSTEM_USER, true);
         simulateUserUnlocked(NON_SYSTEM_USER);
+
         String[] args = new String[0];
-        mService.dumpWithoutCheckingPermission(mFileDescriptorStub, mPrintWriterMock, args);
+        mService.dump(mFileDescriptorStub, mFakePrintWriter, args);
 
         InOrder inOrder =
                 inOrder(mSystemUserBackupManagerService, mNonSystemUserBackupManagerService);
         inOrder.verify(mSystemUserBackupManagerService)
-                .dump(mFileDescriptorStub, mPrintWriterMock, args);
+                .dump(mFileDescriptorStub, mFakePrintWriter, args);
         inOrder.verify(mNonSystemUserBackupManagerService)
-                .dump(mFileDescriptorStub, mPrintWriterMock, args);
+                .dump(mFileDescriptorStub, mFakePrintWriter, args);
         inOrder.verifyNoMoreInteractions();
     }
 
@@ -753,6 +790,11 @@ public class BackupManagerServiceTest {
         return new File(sTestDir, "rememberActivated-" + userId);
     }
 
+    private static void mockDumpPermissionsGranted(boolean granted) {
+        doReturn(granted)
+                .when(() -> DumpUtils.checkDumpAndUsageStatsPermission(any(), any(), any()));
+    }
+
     private static class BackupManagerServiceTestable extends BackupManagerService {
         static boolean sBackupDisabled = false;
         static int sCallingUserId = -1;
@@ -803,4 +845,17 @@ public class BackupManagerServiceTest {
             runnable.run();
         }
     }
+
+    private static class FakePrintWriter extends PrintWriter {
+        String mPrintedSoFar = "";
+
+        FakePrintWriter() {
+            super(Writer.nullWriter());
+        }
+
+        @Override
+        public void print(String s) {
+            mPrintedSoFar = mPrintedSoFar + s;
+        }
+    }
 }