From 9de0b77cbc76bb475c97ff5a100b2026ac171397 Mon Sep 17 00:00:00 2001 From: Ruslan Tkhakokhov Date: Mon, 10 Feb 2020 10:26:21 +0000 Subject: [PATCH] Get blocked restore keys directly from UserBMS Bug: 145126096 Test: atest KeyValueRestoreExclusionHostSideTest atest PerformUnifiedRestoreHostSideTest atest UserBackupPreferencesTest Currently PerformUnifiedRestoreTask gets the list of blocked restore keys at construction. However, at that point the list might not be fully constructed yet. We should get the keys through the getter avaialble in UserBMS when we need them. Change-Id: I62ad34138ba7a893e66d6af05d2e242c9c964a44 --- .../backup/UserBackupManagerService.java | 11 ++--- .../server/backup/UserBackupPreferences.java | 13 +----- .../server/backup/internal/BackupHandler.java | 3 +- .../server/backup/params/RestoreParams.java | 29 ++++-------- .../backup/restore/ActiveRestoreSession.java | 9 ++-- .../restore/PerformUnifiedRestoreTask.java | 16 +++---- .../ShadowPerformUnifiedRestoreTask.java | 3 +- .../backup/UserBackupPreferencesTest.java | 33 +++----------- .../PerformUnifiedRestoreTaskTest.java | 44 +++++++++++++++---- 9 files changed, 67 insertions(+), 94 deletions(-) diff --git a/services/backup/java/com/android/server/backup/UserBackupManagerService.java b/services/backup/java/com/android/server/backup/UserBackupManagerService.java index e3d2dcc8141fa..6247a635233ab 100644 --- a/services/backup/java/com/android/server/backup/UserBackupManagerService.java +++ b/services/backup/java/com/android/server/backup/UserBackupManagerService.java @@ -1080,12 +1080,8 @@ public class UserBackupManagerService { } } - public Map> getExcludedRestoreKeys(String... packages) { - return mBackupPreferences.getExcludedRestoreKeysForPackages(packages); - } - - public Map> getAllExcludedRestoreKeys() { - return mBackupPreferences.getAllExcludedRestoreKeys(); + public Set getExcludedRestoreKeys(String packageName) { + return mBackupPreferences.getExcludedRestoreKeysForPackage(packageName); } /** Used for generating random salts or passwords. */ @@ -3356,8 +3352,7 @@ public class UserBackupManagerService { restoreSet, packageName, token, - listener, - getExcludedRestoreKeys(packageName)); + listener); mBackupHandler.sendMessage(msg); } catch (Exception e) { // Calling into the transport broke; back off and proceed with the installation. diff --git a/services/backup/java/com/android/server/backup/UserBackupPreferences.java b/services/backup/java/com/android/server/backup/UserBackupPreferences.java index 41b9719d273ba..bb8bf52187c55 100644 --- a/services/backup/java/com/android/server/backup/UserBackupPreferences.java +++ b/services/backup/java/com/android/server/backup/UserBackupPreferences.java @@ -48,16 +48,7 @@ public class UserBackupPreferences { mEditor.commit(); } - Map> getExcludedRestoreKeysForPackages(String... packages) { - Map> excludedKeys = new HashMap<>(); - for (String packageName : packages) { - excludedKeys.put(packageName, - mPreferences.getStringSet(packageName, Collections.emptySet())); - } - return excludedKeys; - } - - Map> getAllExcludedRestoreKeys() { - return (Map>) mPreferences.getAll(); + Set getExcludedRestoreKeysForPackage(String packageName) { + return mPreferences.getStringSet(packageName, Collections.emptySet()); } } diff --git a/services/backup/java/com/android/server/backup/internal/BackupHandler.java b/services/backup/java/com/android/server/backup/internal/BackupHandler.java index 05396f36b3640..87a8e49825293 100644 --- a/services/backup/java/com/android/server/backup/internal/BackupHandler.java +++ b/services/backup/java/com/android/server/backup/internal/BackupHandler.java @@ -299,8 +299,7 @@ public class BackupHandler extends Handler { params.pmToken, params.isSystemRestore, params.filterSet, - params.listener, - params.excludedKeys); + params.listener); synchronized (backupManagerService.getPendingRestores()) { if (backupManagerService.isRestoreInProgress()) { diff --git a/services/backup/java/com/android/server/backup/params/RestoreParams.java b/services/backup/java/com/android/server/backup/params/RestoreParams.java index 09b7e3535e2e0..a6fea6cc75a0f 100644 --- a/services/backup/java/com/android/server/backup/params/RestoreParams.java +++ b/services/backup/java/com/android/server/backup/params/RestoreParams.java @@ -37,7 +37,6 @@ public class RestoreParams { public final boolean isSystemRestore; @Nullable public final String[] filterSet; public final OnTaskFinishedListener listener; - public final Map> excludedKeys; /** * No kill after restore. @@ -48,8 +47,7 @@ public class RestoreParams { IBackupManagerMonitor monitor, long token, PackageInfo packageInfo, - OnTaskFinishedListener listener, - Map> excludedKeys) { + OnTaskFinishedListener listener) { return new RestoreParams( transportClient, observer, @@ -59,8 +57,7 @@ public class RestoreParams { /* pmToken */ 0, /* isSystemRestore */ false, /* filterSet */ null, - listener, - excludedKeys); + listener); } /** @@ -73,8 +70,7 @@ public class RestoreParams { long token, String packageName, int pmToken, - OnTaskFinishedListener listener, - Map> excludedKeys) { + OnTaskFinishedListener listener) { String[] filterSet = {packageName}; return new RestoreParams( transportClient, @@ -85,8 +81,7 @@ public class RestoreParams { pmToken, /* isSystemRestore */ false, filterSet, - listener, - excludedKeys); + listener); } /** @@ -97,8 +92,7 @@ public class RestoreParams { IRestoreObserver observer, IBackupManagerMonitor monitor, long token, - OnTaskFinishedListener listener, - Map> excludedKeys) { + OnTaskFinishedListener listener) { return new RestoreParams( transportClient, observer, @@ -108,8 +102,7 @@ public class RestoreParams { /* pmToken */ 0, /* isSystemRestore */ true, /* filterSet */ null, - listener, - excludedKeys); + listener); } /** @@ -122,8 +115,7 @@ public class RestoreParams { long token, String[] filterSet, boolean isSystemRestore, - OnTaskFinishedListener listener, - Map> excludedKeys) { + OnTaskFinishedListener listener) { return new RestoreParams( transportClient, observer, @@ -133,8 +125,7 @@ public class RestoreParams { /* pmToken */ 0, isSystemRestore, filterSet, - listener, - excludedKeys); + listener); } private RestoreParams( @@ -146,8 +137,7 @@ public class RestoreParams { int pmToken, boolean isSystemRestore, @Nullable String[] filterSet, - OnTaskFinishedListener listener, - Map> excludedKeys) { + OnTaskFinishedListener listener) { this.transportClient = transportClient; this.observer = observer; this.monitor = monitor; @@ -157,6 +147,5 @@ public class RestoreParams { this.isSystemRestore = isSystemRestore; this.filterSet = filterSet; this.listener = listener; - this.excludedKeys = excludedKeys; } } diff --git a/services/backup/java/com/android/server/backup/restore/ActiveRestoreSession.java b/services/backup/java/com/android/server/backup/restore/ActiveRestoreSession.java index c0f76c39e04f6..5a57cdc39402a 100644 --- a/services/backup/java/com/android/server/backup/restore/ActiveRestoreSession.java +++ b/services/backup/java/com/android/server/backup/restore/ActiveRestoreSession.java @@ -178,8 +178,7 @@ public class ActiveRestoreSession extends IRestoreSession.Stub { observer, monitor, token, - listener, - mBackupManagerService.getAllExcludedRestoreKeys()), + listener), "RestoreSession.restoreAll()"); } finally { Binder.restoreCallingIdentity(oldId); @@ -272,8 +271,7 @@ public class ActiveRestoreSession extends IRestoreSession.Stub { token, packages, /* isSystemRestore */ packages.length > 1, - listener, - mBackupManagerService.getExcludedRestoreKeys(packages)), + listener), "RestoreSession.restorePackages(" + packages.length + " packages)"); } finally { Binder.restoreCallingIdentity(oldId); @@ -365,8 +363,7 @@ public class ActiveRestoreSession extends IRestoreSession.Stub { monitor, token, app, - listener, - mBackupManagerService.getExcludedRestoreKeys(app.packageName)), + listener), "RestoreSession.restorePackage(" + packageName + ")"); } finally { Binder.restoreCallingIdentity(oldId); diff --git a/services/backup/java/com/android/server/backup/restore/PerformUnifiedRestoreTask.java b/services/backup/java/com/android/server/backup/restore/PerformUnifiedRestoreTask.java index f90d936a5f4dd..3c37f737f8beb 100644 --- a/services/backup/java/com/android/server/backup/restore/PerformUnifiedRestoreTask.java +++ b/services/backup/java/com/android/server/backup/restore/PerformUnifiedRestoreTask.java @@ -155,8 +155,6 @@ public class PerformUnifiedRestoreTask implements BackupRestoreTask { // When finished call listener private final OnTaskFinishedListener mListener; - private final Map> mExcludedKeys; - // Key/value: bookkeeping about staged data and files for agent access private File mBackupDataName; private File mStageName; @@ -168,14 +166,14 @@ public class PerformUnifiedRestoreTask implements BackupRestoreTask { private final BackupAgentTimeoutParameters mAgentTimeoutParameters; @VisibleForTesting - PerformUnifiedRestoreTask(Map> excludedKeys) { - mExcludedKeys = excludedKeys; + PerformUnifiedRestoreTask(UserBackupManagerService backupManagerService) { mListener = null; mAgentTimeoutParameters = null; mTransportClient = null; mTransportManager = null; mEphemeralOpToken = 0; mUserId = 0; + this.backupManagerService = backupManagerService; } // This task can assume that the wakelock is properly held for it and doesn't have to worry @@ -190,8 +188,7 @@ public class PerformUnifiedRestoreTask implements BackupRestoreTask { int pmToken, boolean isFullSystemRestore, @Nullable String[] filterSet, - OnTaskFinishedListener listener, - Map> excludedKeys) { + OnTaskFinishedListener listener) { this.backupManagerService = backupManagerService; mUserId = backupManagerService.getUserId(); mTransportManager = backupManagerService.getTransportManager(); @@ -213,8 +210,6 @@ public class PerformUnifiedRestoreTask implements BackupRestoreTask { backupManagerService.getAgentTimeoutParameters(), "Timeout parameters cannot be null"); - mExcludedKeys = excludedKeys; - if (targetPackage != null) { // Single package restore mAcceptSet = new ArrayList<>(); @@ -791,8 +786,9 @@ public class PerformUnifiedRestoreTask implements BackupRestoreTask { !getExcludedKeysForPackage(PLATFORM_PACKAGE_NAME).isEmpty(); } - private Set getExcludedKeysForPackage(String packageName) { - return mExcludedKeys.getOrDefault(packageName, Collections.emptySet()); + @VisibleForTesting + Set getExcludedKeysForPackage(String packageName) { + return backupManagerService.getExcludedRestoreKeys(packageName); } @VisibleForTesting diff --git a/services/robotests/src/com/android/server/testing/shadows/ShadowPerformUnifiedRestoreTask.java b/services/robotests/src/com/android/server/testing/shadows/ShadowPerformUnifiedRestoreTask.java index 223a98b6c8f6a..8daef5fad0320 100644 --- a/services/robotests/src/com/android/server/testing/shadows/ShadowPerformUnifiedRestoreTask.java +++ b/services/robotests/src/com/android/server/testing/shadows/ShadowPerformUnifiedRestoreTask.java @@ -67,8 +67,7 @@ public class ShadowPerformUnifiedRestoreTask { int pmToken, boolean isFullSystemRestore, @Nullable String[] filterSet, - OnTaskFinishedListener listener, - Map> excludedKeys) { + OnTaskFinishedListener listener) { mBackupManagerService = backupManagerService; mPackage = targetPackage; mIsFullSystemRestore = isFullSystemRestore; diff --git a/services/tests/servicestests/src/com/android/server/backup/UserBackupPreferencesTest.java b/services/tests/servicestests/src/com/android/server/backup/UserBackupPreferencesTest.java index d6efe35723db5..5800acabe8a50 100644 --- a/services/tests/servicestests/src/com/android/server/backup/UserBackupPreferencesTest.java +++ b/services/tests/servicestests/src/com/android/server/backup/UserBackupPreferencesTest.java @@ -16,8 +16,6 @@ package com.android.server.backup; -import static junit.framework.Assert.assertEquals; -import static junit.framework.Assert.assertFalse; import static junit.framework.Assert.assertTrue; import androidx.test.InstrumentationRegistry; @@ -33,18 +31,15 @@ import org.junit.runner.RunWith; import org.mockito.MockitoAnnotations; import java.util.Arrays; -import java.util.HashSet; import java.util.List; -import java.util.Map; import java.util.Set; @Presubmit @RunWith(AndroidJUnit4.class) public class UserBackupPreferencesTest { private static final String EXCLUDED_PACKAGE_1 = "package1"; - private static final String EXCLUDED_PACKAGE_2 = "package2"; private static final List EXCLUDED_KEYS_1 = Arrays.asList("key1", "key2"); - private static final List EXCLUDED_KEYS_2 = Arrays.asList("key1"); + private static final List EXCLUDED_KEYS_2 = Arrays.asList("key3"); @Rule public TemporaryFolder mTemporaryFolder = new TemporaryFolder(); @@ -60,27 +55,13 @@ public class UserBackupPreferencesTest { } @Test - public void testGetExcludedKeysForPackages_returnsExcludedKeys() { + public void testGetExcludedKeysForPackage_returnsExcludedKeys() { mExcludedRestoreKeysStorage.addExcludedKeys(EXCLUDED_PACKAGE_1, EXCLUDED_KEYS_1); - mExcludedRestoreKeysStorage.addExcludedKeys(EXCLUDED_PACKAGE_2, EXCLUDED_KEYS_2); + mExcludedRestoreKeysStorage.addExcludedKeys(EXCLUDED_PACKAGE_1, EXCLUDED_KEYS_2); - Map> excludedKeys = - mExcludedRestoreKeysStorage.getExcludedRestoreKeysForPackages(EXCLUDED_PACKAGE_1); - assertTrue(excludedKeys.containsKey(EXCLUDED_PACKAGE_1)); - assertFalse(excludedKeys.containsKey(EXCLUDED_PACKAGE_2)); - assertEquals(new HashSet<>(EXCLUDED_KEYS_1), excludedKeys.get(EXCLUDED_PACKAGE_1)); - } - - @Test - public void testGetExcludedKeysForPackages_withEmpty_list_returnsAllExcludedKeys() { - mExcludedRestoreKeysStorage.addExcludedKeys(EXCLUDED_PACKAGE_1, EXCLUDED_KEYS_1); - mExcludedRestoreKeysStorage.addExcludedKeys(EXCLUDED_PACKAGE_2, EXCLUDED_KEYS_2); - - Map> excludedKeys = - mExcludedRestoreKeysStorage.getAllExcludedRestoreKeys(); - assertTrue(excludedKeys.containsKey(EXCLUDED_PACKAGE_1)); - assertTrue(excludedKeys.containsKey(EXCLUDED_PACKAGE_2)); - assertEquals(new HashSet<>(EXCLUDED_KEYS_1), excludedKeys.get(EXCLUDED_PACKAGE_1)); - assertEquals(new HashSet<>(EXCLUDED_KEYS_2), excludedKeys.get(EXCLUDED_PACKAGE_2)); + Set excludedKeys = + mExcludedRestoreKeysStorage.getExcludedRestoreKeysForPackage(EXCLUDED_PACKAGE_1); + assertTrue(excludedKeys.containsAll(EXCLUDED_KEYS_1)); + assertTrue(excludedKeys.containsAll(EXCLUDED_KEYS_2)); } } diff --git a/services/tests/servicestests/src/com/android/server/backup/restore/PerformUnifiedRestoreTaskTest.java b/services/tests/servicestests/src/com/android/server/backup/restore/PerformUnifiedRestoreTaskTest.java index 3d220432cc8eb..017c93975286c 100644 --- a/services/tests/servicestests/src/com/android/server/backup/restore/PerformUnifiedRestoreTaskTest.java +++ b/services/tests/servicestests/src/com/android/server/backup/restore/PerformUnifiedRestoreTaskTest.java @@ -20,7 +20,9 @@ import static junit.framework.Assert.assertEquals; import static junit.framework.Assert.assertFalse; import static junit.framework.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.when; import androidx.test.InstrumentationRegistry; @@ -30,6 +32,8 @@ import android.app.backup.BackupDataInput; import android.app.backup.BackupDataOutput; import android.platform.test.annotations.Presubmit; +import com.android.server.backup.UserBackupManagerService; + import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -40,6 +44,7 @@ import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; import java.util.ArrayDeque; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -59,6 +64,7 @@ public class PerformUnifiedRestoreTaskTest { @Mock private BackupDataInput mBackupDataInput; @Mock private BackupDataOutput mBackupDataOutput; + @Mock private UserBackupManagerService mBackupManagerService; private Set mExcludedkeys = new HashSet<>(); private Map mBackupData = new HashMap<>(); @@ -99,6 +105,8 @@ public class PerformUnifiedRestoreTaskTest { return null; } }); + + mRestoreTask = new PerformUnifiedRestoreTask(mBackupManagerService); } private void populateTestData() { @@ -114,8 +122,9 @@ public class PerformUnifiedRestoreTaskTest { @Test public void testFilterExcludedKeys() throws Exception { - mRestoreTask = new PerformUnifiedRestoreTask(Collections.singletonMap( - PACKAGE_NAME, mExcludedkeys)); + when(mBackupManagerService.getExcludedRestoreKeys(eq(PACKAGE_NAME))).thenReturn( + mExcludedkeys); + mRestoreTask.filterExcludedKeys(PACKAGE_NAME, mBackupDataInput, mBackupDataOutput); // Verify only the correct were written into BackupDataOutput object. @@ -124,33 +133,50 @@ public class PerformUnifiedRestoreTaskTest { assertEquals(allowedBackupKeys, mBackupDataDump); } + @Test + public void testGetExcludedKeysForPackage_alwaysReturnsLatestKeys() { + Set firstExcludedKeys = new HashSet<>(Collections.singletonList(EXCLUDED_KEY_1)); + when(mBackupManagerService.getExcludedRestoreKeys(eq(PACKAGE_NAME))).thenReturn( + firstExcludedKeys); + assertEquals(firstExcludedKeys, mRestoreTask.getExcludedKeysForPackage(PACKAGE_NAME)); + + + Set secondExcludedKeys = new HashSet<>(Arrays.asList(EXCLUDED_KEY_1, + EXCLUDED_KEY_2)); + when(mBackupManagerService.getExcludedRestoreKeys(eq(PACKAGE_NAME))).thenReturn( + secondExcludedKeys); + assertEquals(secondExcludedKeys, mRestoreTask.getExcludedKeysForPackage(PACKAGE_NAME)); + } + @Test public void testStageBackupData_stageForNonSystemPackageWithKeysToExclude() { - mRestoreTask = new PerformUnifiedRestoreTask(Collections.singletonMap( - PACKAGE_NAME, mExcludedkeys)); + when(mBackupManagerService.getExcludedRestoreKeys(eq(NON_SYSTEM_PACKAGE_NAME))).thenReturn( + mExcludedkeys); assertTrue(mRestoreTask.shouldStageBackupData(NON_SYSTEM_PACKAGE_NAME)); } @Test public void testStageBackupData_stageForNonSystemPackageWithNoKeysToExclude() { - mRestoreTask = new PerformUnifiedRestoreTask(Collections.emptyMap()); + when(mBackupManagerService.getExcludedRestoreKeys(any())).thenReturn( + Collections.emptySet()); assertTrue(mRestoreTask.shouldStageBackupData(NON_SYSTEM_PACKAGE_NAME)); } @Test public void testStageBackupData_doNotStageForSystemPackageWithNoKeysToExclude() { - mRestoreTask = new PerformUnifiedRestoreTask(Collections.emptyMap()); + when(mBackupManagerService.getExcludedRestoreKeys(any())).thenReturn( + Collections.emptySet()); assertFalse(mRestoreTask.shouldStageBackupData(SYSTEM_PACKAGE_NAME)); } @Test public void testStageBackupData_stageForSystemPackageWithKeysToExclude() { - mRestoreTask = new PerformUnifiedRestoreTask(Collections.singletonMap( - PACKAGE_NAME, mExcludedkeys)); + when(mBackupManagerService.getExcludedRestoreKeys(eq(SYSTEM_PACKAGE_NAME))).thenReturn( + mExcludedkeys); - assertTrue(mRestoreTask.shouldStageBackupData(NON_SYSTEM_PACKAGE_NAME)); + assertTrue(mRestoreTask.shouldStageBackupData(SYSTEM_PACKAGE_NAME)); } }