From da26f028aeb54125923284002b24fcd7058d15d8 Mon Sep 17 00:00:00 2001 From: Piyush Mehrotra Date: Fri, 3 Feb 2023 17:25:14 +0000 Subject: Improve `dumpsys backup` 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 ` 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. Bug: 267755698 Test: atest BackupManagerServiceTest Manual test by running dumpsys backup with optional arguments. Change-Id: Id7790a501a8befb12a212f66adfc0cd8cc9f9e8b --- .../server/backup/BackupManagerService.java | 84 +++++++++++++++------- .../server/backup/BackupManagerServiceTest.java | 45 ++++++++++++ 2 files changed, 103 insertions(+), 26 deletions(-) diff --git a/services/backup/java/com/android/server/backup/BackupManagerService.java b/services/backup/java/com/android/server/backup/BackupManagerService.java index 3a7aa855ea90..978dc56201da 100644 --- a/services/backup/java/com/android/server/backup/BackupManagerService.java +++ b/services/backup/java/com/android/server/backup/BackupManagerService.java @@ -1515,41 +1515,73 @@ public class BackupManagerService extends IBackupManager.Stub { @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 : dump information for user userId"); } /** @@ -1654,7 +1686,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/tests/mockingservicestests/src/com/android/server/backup/BackupManagerServiceTest.java b/services/tests/mockingservicestests/src/com/android/server/backup/BackupManagerServiceTest.java index b203cf640097..36f52515f1ea 100644 --- a/services/tests/mockingservicestests/src/com/android/server/backup/BackupManagerServiceTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/backup/BackupManagerServiceTest.java @@ -63,6 +63,7 @@ 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; @@ -615,6 +616,50 @@ public class BackupManagerServiceTest { verify(mNonSystemUserBackupManagerService, never()).dump(any(), any(), any()); } + @Test + public void testDumpForOneUser_callerDoesNotHaveInteractAcrossUsersFullPermission_ignored() { + mService.setBackupServiceActive(NON_SYSTEM_USER, true); + simulateUserUnlocked(NON_SYSTEM_USER); + + doThrow(new SecurityException()) + .when(mContextMock) + .enforceCallingOrSelfPermission( + eq(Manifest.permission.INTERACT_ACROSS_USERS_FULL), anyString()); + + String[] args = new String[]{"--user", Integer.toString(NON_SYSTEM_USER)}; + Assert.assertThrows(SecurityException.class, + () -> mService.dumpWithoutCheckingPermission(mFileDescriptorStub, mPrintWriterMock, + args)); + + verify(mNonSystemUserBackupManagerService, never()).dump(any(), any(), any()); + } + + @Test + public void + testDumpForOneUser_callerHasInteractAcrossUsersFullPermission_dumpsOnlySpecifiedUser() { + mService.setBackupServiceActive(NON_SYSTEM_USER, true); + simulateUserUnlocked(NON_SYSTEM_USER); + + String[] args = new String[]{"--user", Integer.toString(UserHandle.USER_SYSTEM)}; + mService.dumpWithoutCheckingPermission(mFileDescriptorStub, mPrintWriterMock, args); + + verify(mSystemUserBackupManagerService).dump(any(), any(), any()); + } + + @Test + public void testDumpForAllUsers_callerHasInteractAcrossUsersFullPermission_dumpsAllUsers() { + mService.setBackupServiceActive(NON_SYSTEM_USER, true); + simulateUserUnlocked(NON_SYSTEM_USER); + + String[] args = new String[]{"users"}; + mService.dumpWithoutCheckingPermission(mFileDescriptorStub, mPrintWriterMock, 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()); + } + /** * Test that {@link BackupManagerService#dump(FileDescriptor, PrintWriter, String[])} dumps * system user information before non-system user information. -- cgit v1.2.3-59-g8ed1b From 6cf0c3038b92832ba06431db28a17744652b5cc3 Mon Sep 17 00:00:00 2001 From: Piyush Mehrotra Date: Fri, 17 Feb 2023 12:28:10 +0000 Subject: Modifying Backup code to support HSUM mode. Bug: 266703231 Test: Run CTS/GTS tests for backup Change-Id: I7a8d09f9f1be83ff4d9f545439f93b2c5319f007 --- cmds/bmgr/src/com/android/commands/bmgr/Bmgr.java | 2 +- cmds/bu/src/com/android/commands/bu/Backup.java | 3 +++ .../java/com/android/server/backup/BackupManagerService.java | 2 +- .../com/android/server/backup/UserBackupManagerService.java | 12 +----------- .../android/server/backup/BackupManagerServiceRoboTest.java | 9 +++++++-- .../com/android/server/backup/BackupManagerServiceTest.java | 3 +++ 6 files changed, 16 insertions(+), 15 deletions(-) diff --git a/cmds/bmgr/src/com/android/commands/bmgr/Bmgr.java b/cmds/bmgr/src/com/android/commands/bmgr/Bmgr.java index b6dc32a29f04..7d09b992d080 100644 --- a/cmds/bmgr/src/com/android/commands/bmgr/Bmgr.java +++ b/cmds/bmgr/src/com/android/commands/bmgr/Bmgr.java @@ -253,7 +253,7 @@ public class Bmgr { try { boolean enable = Boolean.parseBoolean(arg); - mBmgr.setAutoRestore(enable); + mBmgr.setAutoRestoreForUser(userId, enable); System.out.println( "Auto restore is now " + (enable ? "enabled" : "disabled") diff --git a/cmds/bu/src/com/android/commands/bu/Backup.java b/cmds/bu/src/com/android/commands/bu/Backup.java index 373677eccf62..11c9773a7a6c 100644 --- a/cmds/bu/src/com/android/commands/bu/Backup.java +++ b/cmds/bu/src/com/android/commands/bu/Backup.java @@ -56,6 +56,7 @@ public final class Backup { } public void run(String[] args) { + Log.d(TAG, "Called run() with args: " + String.join(" ", args)); if (mBackupManager == null) { Log.e(TAG, "Can't obtain Backup Manager binder"); return; @@ -70,6 +71,8 @@ public final class Backup { return; } + Log.d(TAG, "UserId : " + userId); + String arg = nextArg(); if (arg.equals("backup")) { doBackup(OsConstants.STDOUT_FILENO, userId); diff --git a/services/backup/java/com/android/server/backup/BackupManagerService.java b/services/backup/java/com/android/server/backup/BackupManagerService.java index 978dc56201da..7a3b1190cffa 100644 --- a/services/backup/java/com/android/server/backup/BackupManagerService.java +++ b/services/backup/java/com/android/server/backup/BackupManagerService.java @@ -813,7 +813,7 @@ public class BackupManagerService extends IBackupManager.Stub { } UserBackupManagerService userBackupManagerService = getServiceForUserIfCallerHasPermission( - UserHandle.USER_SYSTEM, "hasBackupPassword()"); + userId, "hasBackupPassword()"); return userBackupManagerService != null && userBackupManagerService.hasBackupPassword(); } diff --git a/services/backup/java/com/android/server/backup/UserBackupManagerService.java b/services/backup/java/com/android/server/backup/UserBackupManagerService.java index 7261709d7b8d..b65681104527 100644 --- a/services/backup/java/com/android/server/backup/UserBackupManagerService.java +++ b/services/backup/java/com/android/server/backup/UserBackupManagerService.java @@ -2797,11 +2797,6 @@ public class UserBackupManagerService { boolean includeSystem, boolean compress, boolean doKeyValue, String[] pkgList) { mContext.enforceCallingPermission(android.Manifest.permission.BACKUP, "adbBackup"); - final int callingUserHandle = UserHandle.getCallingUserId(); - if (callingUserHandle != UserHandle.USER_SYSTEM) { - throw new IllegalStateException("Backup supported only for the device owner"); - } - // Validate if (!doAllApps) { if (!includeShared) { @@ -2972,11 +2967,6 @@ public class UserBackupManagerService { public void adbRestore(ParcelFileDescriptor fd) { mContext.enforceCallingPermission(android.Manifest.permission.BACKUP, "adbRestore"); - final int callingUserHandle = UserHandle.getCallingUserId(); - if (callingUserHandle != UserHandle.USER_SYSTEM) { - throw new IllegalStateException("Restore supported only for the device owner"); - } - final long oldId = Binder.clearCallingIdentity(); try { @@ -3085,7 +3075,7 @@ public class UserBackupManagerService { "com.android.backupconfirm.BackupRestoreConfirmation"); confIntent.putExtra(FullBackup.CONF_TOKEN_INTENT_EXTRA, token); confIntent.addFlags(Intent.FLAG_ACTIVITY_SINGLE_TOP); - mContext.startActivityAsUser(confIntent, UserHandle.SYSTEM); + mContext.startActivityAsUser(confIntent, UserHandle.of(mUserId)); } catch (ActivityNotFoundException e) { return false; } 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..91dcd50f176a 100644 --- a/services/robotests/backup/src/com/android/server/backup/BackupManagerServiceRoboTest.java +++ b/services/robotests/backup/src/com/android/server/backup/BackupManagerServiceRoboTest.java @@ -33,6 +33,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import static org.robolectric.Shadows.shadowOf; import static org.testng.Assert.expectThrows; @@ -118,6 +119,10 @@ public class BackupManagerServiceRoboTest { mShadowUserManager.addUser(mUserOneId, "mUserOneId", 0); mShadowUserManager.addUser(mUserTwoId, "mUserTwoId", 0); + when(mUserSystemService.getUserId()).thenReturn(UserHandle.USER_SYSTEM); + when(mUserOneService.getUserId()).thenReturn(mUserOneId); + when(mUserTwoService.getUserId()).thenReturn(mUserTwoId); + mShadowContext.grantPermissions(BACKUP); mShadowContext.grantPermissions(INTERACT_ACROSS_USERS_FULL); @@ -1469,9 +1474,9 @@ public class BackupManagerServiceRoboTest { File testFile = createTestFile(); FileDescriptor fileDescriptor = new FileDescriptor(); PrintWriter printWriter = new PrintWriter(testFile); - String[] args = {"1", "2"}; ShadowBinder.setCallingUserHandle(UserHandle.of(UserHandle.USER_SYSTEM)); + String[] args = {"--user", "0"}; backupManagerService.dump(fileDescriptor, printWriter, args); verify(mUserSystemService).dump(fileDescriptor, printWriter, args); @@ -1485,8 +1490,8 @@ public class BackupManagerServiceRoboTest { File testFile = createTestFile(); FileDescriptor fileDescriptor = new FileDescriptor(); PrintWriter printWriter = new PrintWriter(testFile); - String[] args = {"1", "2"}; + String[] args = {"--user", "10"}; backupManagerService.dump(fileDescriptor, printWriter, args); verify(mUserOneService, never()).dump(fileDescriptor, printWriter, args); 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 36f52515f1ea..f99e156ed139 100644 --- a/services/tests/mockingservicestests/src/com/android/server/backup/BackupManagerServiceTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/backup/BackupManagerServiceTest.java @@ -618,6 +618,7 @@ public class BackupManagerServiceTest { @Test public void testDumpForOneUser_callerDoesNotHaveInteractAcrossUsersFullPermission_ignored() { + createBackupManagerServiceAndUnlockSystemUser(); mService.setBackupServiceActive(NON_SYSTEM_USER, true); simulateUserUnlocked(NON_SYSTEM_USER); @@ -637,6 +638,7 @@ public class BackupManagerServiceTest { @Test public void testDumpForOneUser_callerHasInteractAcrossUsersFullPermission_dumpsOnlySpecifiedUser() { + createBackupManagerServiceAndUnlockSystemUser(); mService.setBackupServiceActive(NON_SYSTEM_USER, true); simulateUserUnlocked(NON_SYSTEM_USER); @@ -648,6 +650,7 @@ public class BackupManagerServiceTest { @Test public void testDumpForAllUsers_callerHasInteractAcrossUsersFullPermission_dumpsAllUsers() { + createBackupManagerServiceAndUnlockSystemUser(); mService.setBackupServiceActive(NON_SYSTEM_USER, true); simulateUserUnlocked(NON_SYSTEM_USER); -- cgit v1.2.3-59-g8ed1b