From efdf07b668f06920eef47ce0a1dba04db2335f85 Mon Sep 17 00:00:00 2001 From: Winson Date: Wed, 17 Mar 2021 13:26:19 -0700 Subject: [PATCH 1/6] Revoke domain user selection when approved through shell This branch existed for the API, but not for the internal method used for shell. Bug: 183226822 Test: manual, debug while writing CTS Change-Id: If19bf9395f445249c3ca3b93b7f6d1a1bad3c5d8 --- .../domain/DomainVerificationService.java | 200 +++++++++--------- .../domain/DomainVerificationShell.java | 12 +- 2 files changed, 104 insertions(+), 108 deletions(-) diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java index 63515b99b4287..d73f40f0deb3b 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java @@ -357,6 +357,8 @@ public class DomainVerificationService extends SystemService "State must be one of NO_RESPONSE, SUCCESS, APPROVED, or DENIED"); } + ArraySet verifiedDomains = new ArraySet<>(); + if (packageName == null) { synchronized (mLock) { ArraySet validDomains = new ArraySet<>(); @@ -383,6 +385,10 @@ public class DomainVerificationService extends SystemService validDomains.retainAll(autoVerifyDomains); } + if (DomainVerificationState.isVerified(state)) { + verifiedDomains.addAll(validDomains); + } + setDomainVerificationStatusInternal(pkgState, state, validDomains); } } @@ -405,10 +411,22 @@ public class DomainVerificationService extends SystemService domains.retainAll(mCollector.collectValidAutoVerifyDomains(pkg)); } + if (DomainVerificationState.isVerified(state)) { + verifiedDomains.addAll(domains); + } + setDomainVerificationStatusInternal(pkgState, state, domains); } } + // Mirror SystemApi behavior of revoking user selection for approved domains. + if (DomainVerificationState.isVerified(state)) { + final int size = verifiedDomains.size(); + for (int index = 0; index < size; index++) { + removeUserStatesForDomain(verifiedDomains.valueAt(index)); + } + } + mConnection.scheduleWriteSettings(); } @@ -522,47 +540,9 @@ public class DomainVerificationService extends SystemService // enabling. This allows an escape hatch in case multiple packages somehow get selected. // They can be disabled without blocking in a circular dependency. if (enabled) { - // Cache the approved packages from the 1st pass because the search is expensive - ArrayMap> domainToApprovedPackages = new ArrayMap<>(); - - for (String domain : domains) { - if (userState.getEnabledHosts().contains(domain)) { - continue; - } - - Pair, Integer> packagesToLevel = getApprovedPackages(domain, - userId, APPROVAL_LEVEL_NONE + 1, mConnection::getPackageSettingLocked); - int highestApproval = packagesToLevel.second; - if (highestApproval > APPROVAL_LEVEL_SELECTION) { - return DomainVerificationManager.ERROR_UNABLE_TO_APPROVE; - } - - domainToApprovedPackages.put(domain, packagesToLevel.first); - } - - // The removal for other packages must be done in a 2nd pass after it's determined - // that no higher priority owners exist for all of the domains in the set. - int mapSize = domainToApprovedPackages.size(); - for (int mapIndex = 0; mapIndex < mapSize; mapIndex++) { - String domain = domainToApprovedPackages.keyAt(mapIndex); - List approvedPackages = domainToApprovedPackages.valueAt(mapIndex); - int approvedSize = approvedPackages.size(); - for (int approvedIndex = 0; approvedIndex < approvedSize; approvedIndex++) { - String approvedPackage = approvedPackages.get(approvedIndex); - DomainVerificationPkgState approvedPkgState = - mAttachedPkgStates.get(approvedPackage); - if (approvedPkgState == null) { - continue; - } - - DomainVerificationInternalUserState approvedUserState = - approvedPkgState.getUserState(userId); - if (approvedUserState == null) { - continue; - } - - approvedUserState.removeHost(domain); - } + int statusCode = revokeOtherUserSelections(userState, userId, domains); + if (statusCode != DomainVerificationManager.STATUS_OK) { + return statusCode; } } @@ -579,83 +559,99 @@ public class DomainVerificationService extends SystemService @Override public void setDomainVerificationUserSelectionInternal(@UserIdInt int userId, - @Nullable String packageName, boolean enabled, @Nullable ArraySet domains) + @NonNull String packageName, boolean enabled, @Nullable ArraySet domains) throws NameNotFoundException { mEnforcer.assertInternal(mConnection.getCallingUid()); - - if (packageName == null) { - synchronized (mLock) { - Set validDomains = new ArraySet<>(); - int size = mAttachedPkgStates.size(); - for (int index = 0; index < size; index++) { - DomainVerificationPkgState pkgState = mAttachedPkgStates.valueAt(index); - String pkgName = pkgState.getPackageName(); - PackageSetting pkgSetting = mConnection.getPackageSettingLocked(pkgName); - AndroidPackage pkg = pkgSetting == null ? null : pkgSetting.getPkg(); - if (pkg == null) { - continue; - } - - if (domains == null) { - validDomains = mCollector.collectAllWebDomains(pkg); - } else { - validDomains.clear(); - validDomains.addAll(domains); - } - - setDomainVerificationUserSelectionInternal(userId, pkgState, - pkg, enabled, validDomains); - } + synchronized (mLock) { + DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); + if (pkgState == null) { + throw DomainVerificationUtils.throwPackageUnavailable(packageName); } - } else { - synchronized (mLock) { - DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); - if (pkgState == null) { - throw DomainVerificationUtils.throwPackageUnavailable(packageName); + + PackageSetting pkgSetting = mConnection.getPackageSettingLocked(packageName); + AndroidPackage pkg = pkgSetting == null ? null : pkgSetting.getPkg(); + if (pkg == null) { + throw DomainVerificationUtils.throwPackageUnavailable(packageName); + } + + Set validDomains = + domains == null ? mCollector.collectAllWebDomains(pkg) : domains; + + validDomains.retainAll(mCollector.collectAllWebDomains(pkg)); + + if (userId == UserHandle.USER_ALL) { + for (int aUserId : mConnection.getAllUserIds()) { + DomainVerificationInternalUserState userState = + pkgState.getOrCreateUserState(aUserId); + revokeOtherUserSelections(userState, aUserId, validDomains); + if (enabled) { + userState.addHosts(validDomains); + } else { + userState.removeHosts(validDomains); + } } - - PackageSetting pkgSetting = mConnection.getPackageSettingLocked(packageName); - AndroidPackage pkg = pkgSetting == null ? null : pkgSetting.getPkg(); - if (pkg == null) { - throw DomainVerificationUtils.throwPackageUnavailable(packageName); + } else { + DomainVerificationInternalUserState userState = + pkgState.getOrCreateUserState(userId); + revokeOtherUserSelections(userState, userId, validDomains); + if (enabled) { + userState.addHosts(validDomains); + } else { + userState.removeHosts(validDomains); } - - Set validDomains = - domains == null ? mCollector.collectAllWebDomains(pkg) : domains; - - setDomainVerificationUserSelectionInternal(userId, pkgState, pkgSetting.getPkg(), - enabled, validDomains); } } mConnection.scheduleWriteSettings(); } - private void setDomainVerificationUserSelectionInternal(int userId, - @NonNull DomainVerificationPkgState pkgState, @NonNull AndroidPackage pkg, - boolean enabled, Set domains) { - domains.retainAll(mCollector.collectAllWebDomains(pkg)); + private int revokeOtherUserSelections(@NonNull DomainVerificationInternalUserState userState, + @UserIdInt int userId, @NonNull Set domains) { + // Cache the approved packages from the 1st pass because the search is expensive + ArrayMap> domainToApprovedPackages = new ArrayMap<>(); - if (userId == UserHandle.USER_ALL) { - for (int aUserId : mConnection.getAllUserIds()) { - DomainVerificationInternalUserState userState = - pkgState.getOrCreateUserState(aUserId); - if (enabled) { - userState.addHosts(domains); - } else { - userState.removeHosts(domains); - } + for (String domain : domains) { + if (userState.getEnabledHosts().contains(domain)) { + continue; } - } else { - DomainVerificationInternalUserState userState = - pkgState.getOrCreateUserState(userId); - if (enabled) { - userState.addHosts(domains); - } else { - userState.removeHosts(domains); + + Pair, Integer> packagesToLevel = getApprovedPackages(domain, + userId, APPROVAL_LEVEL_NONE + 1, mConnection::getPackageSettingLocked); + int highestApproval = packagesToLevel.second; + if (highestApproval > APPROVAL_LEVEL_SELECTION) { + return DomainVerificationManager.ERROR_UNABLE_TO_APPROVE; + } + + domainToApprovedPackages.put(domain, packagesToLevel.first); + } + + // The removal for other packages must be done in a 2nd pass after it's determined + // that no higher priority owners exist for all of the domains in the set. + int mapSize = domainToApprovedPackages.size(); + for (int mapIndex = 0; mapIndex < mapSize; mapIndex++) { + String domain = domainToApprovedPackages.keyAt(mapIndex); + List approvedPackages = domainToApprovedPackages.valueAt(mapIndex); + int approvedSize = approvedPackages.size(); + for (int approvedIndex = 0; approvedIndex < approvedSize; approvedIndex++) { + String approvedPackage = approvedPackages.get(approvedIndex); + DomainVerificationPkgState approvedPkgState = + mAttachedPkgStates.get(approvedPackage); + if (approvedPkgState == null) { + continue; + } + + DomainVerificationInternalUserState approvedUserState = + approvedPkgState.getUserState(userId); + if (approvedUserState == null) { + continue; + } + + approvedUserState.removeHost(domain); } } + + return DomainVerificationManager.STATUS_OK; } @Nullable diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationShell.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationShell.java index 7e755fa384dbb..d180c4b860dcc 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationShell.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationShell.java @@ -89,7 +89,7 @@ public class DomainVerificationShell { pw.println(" must be declared by the package for this to work. This command will not"); pw.println(" report a failure for domains that could not be applied."); pw.println(" --user : the user to change selections for"); - pw.println(" --package : the package to set, or \"all\" to set all packages"); + pw.println(" --package : the package to set"); pw.println(" : whether or not to approve the domain"); pw.println(" : space separated list of domains to change, or \"all\" to"); pw.println(" change every domain."); @@ -217,8 +217,6 @@ public class DomainVerificationShell { if (TextUtils.isEmpty(packageName)) { commandHandler.getErrPrintWriter().println("Error: no package specified"); return false; - } else if (packageName.equalsIgnoreCase("all")) { - packageName = null; } if (userId == null) { @@ -469,13 +467,15 @@ public class DomainVerificationShell { * Variant for use by PackageManagerShellCommand to allow the system/developer to override * the state for a domain. * - * @param packageName the package whose state to change, or all packages if non is - * specified + * If an approval fails because of a higher level owner, this method will silently skip the + * domain. + * + * @param packageName the package whose state to change * @param enabled whether the domain is now approved by the user * @param domains the set of domains to change, or null to affect all domains */ void setDomainVerificationUserSelectionInternal(@UserIdInt int userId, - @Nullable String packageName, boolean enabled, @Nullable ArraySet domains) + @NonNull String packageName, boolean enabled, @Nullable ArraySet domains) throws PackageManager.NameNotFoundException; /** From 5f5d4f6071e9f13b2b4deac77face295ce94aaf4 Mon Sep 17 00:00:00 2001 From: Winson Date: Fri, 19 Mar 2021 13:28:31 -0700 Subject: [PATCH 2/6] Check installed and enabled state for package domain approval If the package isn't installed or isn't enabled for a user, it cannot be approved for that user. Also hooks into package uninstall for a single user to remove the domain state for that package for that user. Bug: 183226822 Test: atest DomainVerificationManagerApiTest#getOwnersForDomain Test: atest com.android.server.pm.test.verify.domain Change-Id: I04942e1491d470fdd41e99f207bbf85baae87d4c --- .../android/content/pm/PackageUserState.java | 16 ++- .../server/pm/PackageManagerService.java | 1 + .../DomainVerificationManagerInternal.java | 5 + .../domain/DomainVerificationService.java | 32 ++++++ .../domain/DomainVerificationSettings.java | 42 ++++++-- .../domain/DomainVerificationEnforcerTest.kt | 8 +- .../DomainVerificationManagerApiTest.kt | 97 +++++++++++++++++-- .../domain/DomainVerificationPackageTest.kt | 3 + .../DomainVerificationSettingsMutationTest.kt | 8 +- ...inVerificationUserSelectionOverrideTest.kt | 4 + 10 files changed, 187 insertions(+), 29 deletions(-) diff --git a/core/java/android/content/pm/PackageUserState.java b/core/java/android/content/pm/PackageUserState.java index e115597865b3e..55a6ab708da5c 100644 --- a/core/java/android/content/pm/PackageUserState.java +++ b/core/java/android/content/pm/PackageUserState.java @@ -32,6 +32,7 @@ import android.annotation.Nullable; import android.compat.annotation.UnsupportedAppUsage; import android.content.ComponentName; import android.content.pm.overlay.OverlayPaths; +import android.content.pm.parsing.ParsingPackageRead; import android.content.pm.parsing.component.ParsedMainComponent; import android.os.BaseBundle; import android.os.Debug; @@ -53,7 +54,6 @@ import org.xmlpull.v1.XmlPullParserException; import org.xmlpull.v1.XmlSerializer; import java.io.IOException; -import java.util.Arrays; import java.util.Map; import java.util.Objects; @@ -310,6 +310,20 @@ public class PackageUserState { return result; } + public boolean isPackageEnabled(@NonNull ParsingPackageRead pkg) { + switch (this.enabled) { + case COMPONENT_ENABLED_STATE_ENABLED: + return true; + case COMPONENT_ENABLED_STATE_DISABLED: + case COMPONENT_ENABLED_STATE_DISABLED_USER: + case COMPONENT_ENABLED_STATE_DISABLED_UNTIL_USED: + return false; + default: + case COMPONENT_ENABLED_STATE_DEFAULT: + return pkg.isEnabled(); + } + } + public boolean isEnabled(ComponentInfo componentInfo, int flags) { return isEnabled(componentInfo.applicationInfo.enabled, componentInfo.enabled, componentInfo.name, flags); diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index cb68cc9d5e453..28d04b8689862 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -21759,6 +21759,7 @@ public class PackageManagerService extends IPackageManager.Stub clearPackagePreferredActivities(ps.name, nextUserId); mPermissionManager.onPackageUninstalled(ps.name, ps.appId, pkg, sharedUserPkgs, nextUserId); + mDomainVerificationManager.clearPackageForUser(ps.name, nextUserId); } if (outInfo != null) { diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java index 73182732cdaf6..5e57ccb9cee29 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java @@ -227,6 +227,11 @@ public interface DomainVerificationManagerInternal { */ void clearPackage(@NonNull String packageName); + /** + * Remove all state for the given package for the given user. + */ + void clearPackageForUser(@NonNull String packageName, @UserIdInt int userId); + /** * Delete all the state for a user. This can be because the user has been removed from the * device, or simply that the state for a user should be deleted. diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java index d73f40f0deb3b..7e3fb53a4da4c 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java @@ -29,6 +29,7 @@ import android.content.Intent; import android.content.pm.IntentFilterVerificationInfo; import android.content.pm.PackageManager; import android.content.pm.PackageManager.NameNotFoundException; +import android.content.pm.PackageUserState; import android.content.pm.ResolveInfo; import android.content.pm.parsing.component.ParsedActivity; import android.content.pm.verify.domain.DomainOwner; @@ -1042,6 +1043,21 @@ public class DomainVerificationService extends SystemService public void clearPackage(@NonNull String packageName) { synchronized (mLock) { mAttachedPkgStates.remove(packageName); + mSettings.removePackage(packageName); + } + + mConnection.scheduleWriteSettings(); + } + + @Override + public void clearPackageForUser(@NonNull String packageName, @UserIdInt int userId) { + synchronized (mLock) { + final DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); + if (pkgState != null) { + pkgState.removeUser(userId); + } + + mSettings.removePackageForUser(packageName, userId); } mConnection.scheduleWriteSettings(); @@ -1544,6 +1560,22 @@ public class DomainVerificationService extends SystemService String packageName = pkgSetting.getName(); final AndroidPackage pkg = pkgSetting.getPkg(); + final PackageUserState pkgUserState = pkgSetting.readUserState(userId); + if (pkgUserState == null) { + if (DEBUG_APPROVAL) { + debugApproval(packageName, debugObject, userId, false, + "PackageUserState unavailable"); + } + return APPROVAL_LEVEL_NONE; + } + + if (!pkgUserState.installed || !pkgUserState.isPackageEnabled(pkg)) { + if (DEBUG_APPROVAL) { + debugApproval(packageName, debugObject, userId, false, "package not enabled"); + } + return APPROVAL_LEVEL_NONE; + } + // Should never be null, but if it is, skip this and assume that v2 is enabled if (pkg != null && !DomainVerificationUtils.isChangeEnabled(mPlatformCompat, pkg, SETTINGS_API_V2)) { diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationSettings.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationSettings.java index 8b59da7bb9445..d3cb0b33f45bf 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationSettings.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationSettings.java @@ -241,17 +241,41 @@ class DomainVerificationSettings { } } - public void removeUser(@UserIdInt int userId) { - int pendingSize = mPendingPkgStates.size(); - for (int index = 0; index < pendingSize; index++) { - mPendingPkgStates.valueAt(index).removeUser(userId); + public void removePackage(@NonNull String packageName) { + synchronized (mLock) { + mPendingPkgStates.remove(packageName); + mRestoredPkgStates.remove(packageName); } + } - // TODO(b/170746586): Restored assumes user IDs match, which is probably not the case - // on a new device - int restoredSize = mRestoredPkgStates.size(); - for (int index = 0; index < restoredSize; index++) { - mRestoredPkgStates.valueAt(index).removeUser(userId); + public void removePackageForUser(@NonNull String packageName, @UserIdInt int userId) { + synchronized (mLock) { + final DomainVerificationPkgState pendingPkgState = mPendingPkgStates.get(packageName); + if (pendingPkgState != null) { + pendingPkgState.removeUser(userId); + } + // TODO(b/170746586): Restored assumes user IDs match, which is probably not the case + // on a new device + final DomainVerificationPkgState restoredPkgState = mRestoredPkgStates.get(packageName); + if (restoredPkgState != null) { + restoredPkgState.removeUser(userId); + } + } + } + + public void removeUser(@UserIdInt int userId) { + synchronized (mLock) { + int pendingSize = mPendingPkgStates.size(); + for (int index = 0; index < pendingSize; index++) { + mPendingPkgStates.valueAt(index).removeUser(userId); + } + + // TODO(b/170746586): Restored assumes user IDs match, which is probably not the case + // on a new device + int restoredSize = mRestoredPkgStates.size(); + for (int index = 0; index < restoredSize; index++) { + mRestoredPkgStates.valueAt(index).removeUser(userId); + } } } diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt index 537a49e4adcda..01a76eea2b0d8 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt @@ -290,6 +290,7 @@ class DomainVerificationEnforcerTest { fun mockPkg(packageName: String) = mockThrowOnUnmocked { whenever(this.packageName) { packageName } whenever(targetSdkVersion) { Build.VERSION_CODES.S } + whenever(isEnabled) { true } whenever(activities) { listOf( ParsedActivity().apply { @@ -330,11 +331,8 @@ class DomainVerificationEnforcerTest { whenever(getName()) { packageName } whenever(getPkg()) { mockPkg(packageName) } whenever(this.domainSetId) { domainSetId } - whenever(userState) { - SparseArray().apply { - this[0] = PackageUserState() - } - } + whenever(readUserState(0)) { PackageUserState() } + whenever(readUserState(1)) { PackageUserState() } whenever(getInstantApp(anyInt())) { false } } } diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt index 881604ffe6f60..f8e0c60c25df6 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt @@ -19,6 +19,7 @@ package com.android.server.pm.test.verify.domain import android.content.Context import android.content.Intent import android.content.pm.PackageManager +import android.content.pm.PackageUserState import android.content.pm.parsing.component.ParsedActivity import android.content.pm.parsing.component.ParsedIntentInfo import android.content.pm.verify.domain.DomainVerificationInfo @@ -41,6 +42,7 @@ import org.mockito.ArgumentMatchers.anyInt import org.mockito.ArgumentMatchers.anyLong import org.mockito.ArgumentMatchers.anyString import java.util.UUID +import java.util.concurrent.atomic.AtomicBoolean import kotlin.test.assertFailsWith class DomainVerificationManagerApiTest { @@ -267,7 +269,16 @@ class DomainVerificationManagerApiTest { @Test fun getOwnersForDomain() { - val pkg1 = mockPkgSetting(PKG_ONE, UUID_ONE, listOf(DOMAIN_1, DOMAIN_2)) + val pkg1User0Enabled = AtomicBoolean(true) + + val pkg1 = mockPkgSetting(PKG_ONE, UUID_ONE, listOf(DOMAIN_1, DOMAIN_2), pkgUserState0 = { + mockThrowOnUnmocked { + whenever(isPackageEnabled(any())) { + pkg1User0Enabled.get() + } + installed = true + } + }) val pkg2 = mockPkgSetting(PKG_TWO, UUID_TWO, listOf(DOMAIN_1, DOMAIN_2)) val service = makeService(pkg1, pkg2).apply { @@ -276,9 +287,20 @@ class DomainVerificationManagerApiTest { assertThat(service.getOwnersForDomain(DOMAIN_1, 0)).isEmpty() - service.setStatus(pkg1.domainSetId, setOf(DOMAIN_1), DomainVerificationInfo.STATE_SUCCESS) - - service.setStatus(pkg2.domainSetId, setOf(DOMAIN_1), DomainVerificationInfo.STATE_SUCCESS) + assertThat( + service.setStatus( + pkg1.domainSetId, + setOf(DOMAIN_1), + DomainVerificationInfo.STATE_SUCCESS + ) + ).isEqualTo(DomainVerificationManager.STATUS_OK) + assertThat( + service.setStatus( + pkg2.domainSetId, + setOf(DOMAIN_1), + DomainVerificationInfo.STATE_SUCCESS + ) + ).isEqualTo(DomainVerificationManager.STATUS_OK) service.setUserSelection(pkg1.domainSetId, setOf(DOMAIN_2), true, 0) @@ -295,15 +317,65 @@ class DomainVerificationManagerApiTest { assertThat(it.single().packageName).isEqualTo(pkg1.getName()) assertThat(it.single().isOverrideable).isEqualTo(true) } - assertThat(service.getOwnersForDomain(DOMAIN_2, 1)).isEmpty() + assertThat(service.getOwnersForDomain(DOMAIN_2, 1)).isEmpty() + service.setUserSelection(pkg1.domainSetId, setOf(DOMAIN_2), true, 1) + service.getOwnersForDomain(DOMAIN_2, 1).let { + assertThat(it).hasSize(1) + assertThat(it.single().packageName).isEqualTo(pkg1.getName()) + assertThat(it.single().isOverrideable).isEqualTo(true) + } + + // "Uninstall" the package from user 0 and ensure it's stripped from the results + pkg1User0Enabled.set(false) + service.clearPackageForUser(pkg1.getName(), 0) + + service.getOwnersForDomain(DOMAIN_1, 0).let { + assertThat(it).hasSize(1) + assertThat(it.single().packageName).isEqualTo(pkg2.getName()) + assertThat(it.single().isOverrideable).isEqualTo(false) + } + + // Domain 2 user selection gone for user 0 + assertThat(service.getOwnersForDomain(DOMAIN_2, 0)).isEmpty() + + // Domain 2 user selection still around for user 1 + service.getOwnersForDomain(DOMAIN_2, 1).let { + assertThat(it).hasSize(1) + assertThat(it.single().packageName).isEqualTo(pkg1.getName()) + assertThat(it.single().isOverrideable).isEqualTo(true) + } + + // Now assert for user 1 that it was unaffected by the change to user 0 + service.getOwnersForDomain(DOMAIN_1, 1).let { + assertThat(it).hasSize(2) + assertThat(it[0].packageName).isEqualTo(pkg1.getName()) + assertThat(it[0].isOverrideable).isEqualTo(false) + assertThat(it[1].packageName).isEqualTo(pkg2.getName()) + assertThat(it[1].isOverrideable).isEqualTo(false) + } + + service.setUserSelection(pkg1.domainSetId, setOf(DOMAIN_2), true, 0) + + service.getOwnersForDomain(DOMAIN_2, 1).let { + assertThat(it).hasSize(1) + assertThat(it.single().packageName).isEqualTo(pkg1.getName()) + assertThat(it.single().isOverrideable).isEqualTo(true) + } + + // "Reinstall" the package to user 0 + pkg1User0Enabled.set(false) + + // This state should have been cleared when the package was uninstalled + assertThat(service.getOwnersForDomain(DOMAIN_2, 0)).isEmpty() + + // Other package unaffected service.setUserSelection(pkg2.domainSetId, setOf(DOMAIN_2), true, 0) service.getOwnersForDomain(DOMAIN_2, 0).let { assertThat(it).hasSize(1) assertThat(it.single().packageName).isEqualTo(pkg2.getName()) assertThat(it.single().isOverrideable).isEqualTo(true) } - assertThat(service.getOwnersForDomain(DOMAIN_2, 1)).isEmpty() } @Test @@ -382,12 +454,17 @@ class DomainVerificationManagerApiTest { }) } - private fun mockPkgSetting(pkgName: String, domainSetId: UUID, domains: List = listOf( - DOMAIN_1, DOMAIN_2 - )) = mockThrowOnUnmocked { + private fun mockPkgSetting( + pkgName: String, + domainSetId: UUID, + domains: List = listOf(DOMAIN_1, DOMAIN_2), + pkgUserState0: PackageSetting.() -> PackageUserState = { PackageUserState() }, + pkgUserState1: PackageSetting.() -> PackageUserState = { PackageUserState() } + ) = mockThrowOnUnmocked { val pkg = mockThrowOnUnmocked { whenever(packageName) { pkgName } whenever(targetSdkVersion) { Build.VERSION_CODES.S } + whenever(isEnabled) { true } val activityList = listOf( ParsedActivity().apply { @@ -416,6 +493,8 @@ class DomainVerificationManagerApiTest { whenever(this.domainSetId) { domainSetId } whenever(getInstantApp(anyInt())) { false } whenever(firstInstallTime) { 0L } + whenever(readUserState(0)) { pkgUserState0() } + whenever(readUserState(1)) { pkgUserState1() } } fun DomainVerificationService.addPackages(vararg pkgSettings: PackageSetting) = diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt index 0ce16e6b6af28..5f601e2648cb5 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt @@ -18,6 +18,7 @@ package com.android.server.pm.test.verify.domain import android.content.Intent import android.content.pm.PackageManager +import android.content.pm.PackageUserState import android.content.pm.parsing.component.ParsedActivity import android.content.pm.parsing.component.ParsedIntentInfo import android.content.pm.verify.domain.DomainVerificationInfo.STATE_NO_RESPONSE @@ -394,6 +395,7 @@ class DomainVerificationPackageTest { val pkg = mockThrowOnUnmocked { whenever(packageName) { pkgName } whenever(targetSdkVersion) { Build.VERSION_CODES.S } + whenever(isEnabled) { true } val activityList = listOf( ParsedActivity().apply { @@ -422,5 +424,6 @@ class DomainVerificationPackageTest { whenever(this.domainSetId) { domainSetId } whenever(getInstantApp(anyInt())) { false } whenever(firstInstallTime) { 0L } + whenever(readUserState(USER_ID)) { PackageUserState() } } } diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt index b7c69226ded20..1cf57c41145a0 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt @@ -192,6 +192,7 @@ class DomainVerificationSettingsMutationTest { fun mockPkg() = mockThrowOnUnmocked { whenever(packageName) { TEST_PKG } whenever(targetSdkVersion) { Build.VERSION_CODES.S } + whenever(isEnabled) { true } whenever(activities) { listOf( ParsedActivity().apply { @@ -233,11 +234,8 @@ class DomainVerificationSettingsMutationTest { whenever(getName()) { TEST_PKG } whenever(getPkg()) { mockPkg() } whenever(domainSetId) { TEST_UUID } - whenever(userState) { - SparseArray().apply { - this[0] = PackageUserState() - } - } + whenever(readUserState(0)) { PackageUserState() } + whenever(readUserState(10)) { PackageUserState() } whenever(getInstantApp(anyInt())) { false } } } diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt index 54648ab674229..474e8012d43bd 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt @@ -18,6 +18,7 @@ package com.android.server.pm.test.verify.domain import android.content.Intent import android.content.pm.PackageManager +import android.content.pm.PackageUserState import android.content.pm.parsing.component.ParsedActivity import android.content.pm.parsing.component.ParsedIntentInfo import android.content.pm.verify.domain.DomainVerificationManager @@ -100,6 +101,7 @@ class DomainVerificationUserStateOverrideTest { val pkg = mockThrowOnUnmocked { whenever(packageName) { pkgName } whenever(targetSdkVersion) { Build.VERSION_CODES.S } + whenever(isEnabled) { true } val activityList = listOf( ParsedActivity().apply { @@ -137,6 +139,8 @@ class DomainVerificationUserStateOverrideTest { whenever(this.domainSetId) { domainSetId } whenever(getInstantApp(anyInt())) { false } whenever(firstInstallTime) { 0L } + whenever(readUserState(0)) { PackageUserState() } + whenever(readUserState(1)) { PackageUserState() } } @Test From 897a2256c817d6c9b9d677debc5b7a1e7735c1e1 Mon Sep 17 00:00:00 2001 From: Winson Date: Wed, 24 Mar 2021 15:24:52 -0700 Subject: [PATCH 3/6] Fix DomainVerificationService deadlock Enforces the PackageManagerService lock to be taken before the DVS lock, when neccessary, so that the locking is always one-way and cannot deadlock when Settings attempts to serialize state. Bug: 183643808 Test: manual run CTS which previously reproduced this issue Change-Id: Ibccde458df16238755f3a67080321cb3ebdf7233 --- .../server/pm/PackageManagerService.java | 102 ++- .../DomainVerificationManagerInternal.java | 71 +- .../domain/DomainVerificationService.java | 710 ++++++++++-------- .../domain/DomainVerificationEnforcerTest.kt | 12 +- .../DomainVerificationManagerApiTest.kt | 8 +- .../domain/DomainVerificationPackageTest.kt | 8 +- .../DomainVerificationSettingsMutationTest.kt | 9 +- .../domain/DomainVerificationTestUtils.kt | 51 ++ ...inVerificationUserSelectionOverrideTest.kt | 12 +- 9 files changed, 632 insertions(+), 351 deletions(-) create mode 100644 services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationTestUtils.kt diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 28d04b8689862..7e58c8b2a8225 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -356,6 +356,7 @@ import com.android.internal.util.CollectionUtils; import com.android.internal.util.ConcurrentUtils; import com.android.internal.util.DumpUtils; import com.android.internal.util.FrameworkStatsLog; +import com.android.internal.util.FunctionalUtils; import com.android.internal.util.IndentingPrintWriter; import com.android.internal.util.Preconditions; import com.android.permission.persistence.RuntimePermissionsPersistence; @@ -1686,9 +1687,8 @@ public class PackageManagerService extends IPackageManager.Stub private final DomainVerificationConnection mDomainVerificationConnection = new DomainVerificationConnection(); - private class DomainVerificationConnection implements - DomainVerificationService.Connection, DomainVerificationProxyV1.Connection, - DomainVerificationProxyV2.Connection { + private class DomainVerificationConnection implements DomainVerificationService.Connection, + DomainVerificationProxyV1.Connection, DomainVerificationProxyV2.Connection { @Override public void scheduleWriteSettings() { @@ -1732,22 +1732,77 @@ public class PackageManagerService extends IPackageManager.Stub return callingUid == getPackageUid(packageName, 0, callingUserId); } - @Nullable - @Override - public PackageSetting getPackageSettingLocked(@NonNull String pkgName) { - return PackageManagerService.this.getPackageSetting(pkgName); - } - - @Nullable - @Override - public AndroidPackage getPackageLocked(@NonNull String pkgName) { - return PackageManagerService.this.getPackage(pkgName); - } - @Nullable @Override public AndroidPackage getPackage(@NonNull String packageName) { - return getPackageLocked(packageName); + return PackageManagerService.this.getPackage(packageName); + } + + @NonNull + @Override + public void withPackageSettings(@NonNull Consumer> block) { + final Computer snapshot = snapshotComputer(); + + // This method needs to either lock or not lock consistently throughout the method, + // so if the live computer is returned, force a wrapping sync block. + if (snapshot == mLiveComputer) { + synchronized (mLock) { + block.accept(snapshot::getPackageSetting); + } + } else { + block.accept(snapshot::getPackageSetting); + } + } + + @Override + public Output withPackageSettingsReturning( + @NonNull FunctionalUtils.ThrowingFunction, Output> + block) { + final Computer snapshot = snapshotComputer(); + + // This method needs to either lock or not lock consistently throughout the method, + // so if the live computer is returned, force a wrapping sync block. + if (snapshot == mLiveComputer) { + synchronized (mLock) { + return block.apply(snapshot::getPackageSetting); + } + } else { + return block.apply(snapshot::getPackageSetting); + } + } + + @Override + public void withPackageSettingsThrowing( + @NonNull ThrowingConsumer, ExceptionType> block) + throws ExceptionType { + final Computer snapshot = snapshotComputer(); + + // This method needs to either lock or not lock consistently throughout the method, + // so if the live computer is returned, force a wrapping sync block. + if (snapshot == mLiveComputer) { + synchronized (mLock) { + block.accept(snapshot::getPackageSetting); + } + } else { + block.accept(snapshot::getPackageSetting); + } + } + + @Override + public Output + withPackageSettingsReturningThrowing(@NonNull ThrowingFunction, Output, ExceptionType> block) throws ExceptionType { + final Computer snapshot = snapshotComputer(); + + // This method needs to either lock or not lock consistently throughout the method, + // so if the live computer is returned, force a wrapping sync block. + if (snapshot == mLiveComputer) { + synchronized (mLock) { + return block.apply(snapshot::getPackageSetting); + } + } else { + return block.apply(snapshot::getPackageSetting); + } } @Override @@ -2012,7 +2067,7 @@ public class PackageManagerService extends IPackageManager.Stub // Cached attributes. The names in this class are the same as the // names in PackageManagerService; see that class for documentation. - private final Settings mSettings; + protected final Settings mSettings; private final WatchedSparseIntArray mIsolatedOwners; private final WatchedArrayMap mPackages; private final WatchedArrayMap @@ -4669,6 +4724,17 @@ public class PackageManagerService extends IPackageManager.Stub mLock = mService.mLock; } + /** + * Explicilty snapshot {@link Settings#mPackages} for cases where the caller must not lock + * in order to get package data. It is expected that the caller locks itself to be able + * to block on changes to the package data and bring itself up to date once the change + * propagates to it. Use with heavy caution. + * @return + */ + private Map snapshotPackageSettings() { + return mSettings.snapshot().mPackages; + } + public @NonNull List queryIntentServicesInternalBody(Intent intent, String resolvedType, int flags, int userId, int callingUid, String instantAppPkgName) { @@ -4800,7 +4866,7 @@ public class PackageManagerService extends IPackageManager.Stub // Compute read-only functions, based on live data. This attribute may be modified multiple // times during the PackageManagerService constructor but it should not be modified thereafter. - private Computer mLiveComputer; + private ComputerLocked mLiveComputer; // A lock-free cache for frequently called functions. private volatile Computer mSnapshotComputer; // If true, the snapshot is invalid (stale). The attribute is static since it may be diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java index 5e57ccb9cee29..0f99e1963f28d 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java @@ -28,6 +28,7 @@ import android.content.pm.PackageManager.NameNotFoundException; import android.content.pm.ResolveInfo; import android.content.pm.verify.domain.DomainVerificationInfo; import android.content.pm.verify.domain.DomainVerificationManager; +import android.content.pm.verify.domain.DomainVerificationState; import android.os.Binder; import android.os.UserHandle; import android.util.IndentingPrintWriter; @@ -35,8 +36,10 @@ import android.util.Pair; import android.util.TypedXmlPullParser; import android.util.TypedXmlSerializer; +import com.android.internal.util.FunctionalUtils; +import com.android.server.pm.PackageManagerService; import com.android.server.pm.PackageSetting; -import com.android.server.pm.parsing.pkg.AndroidPackage; +import com.android.server.pm.Settings; import com.android.server.pm.verify.domain.models.DomainVerificationPkgState; import com.android.server.pm.verify.domain.proxy.DomainVerificationProxy; @@ -46,6 +49,7 @@ import java.io.IOException; import java.util.List; import java.util.Set; import java.util.UUID; +import java.util.function.Consumer; import java.util.function.Function; public interface DomainVerificationManagerInternal { @@ -108,9 +112,10 @@ public interface DomainVerificationManagerInternal { int APPROVAL_LEVEL_INSTANT_APP = 5; /** - * Defines the possible values for {@link #approvalLevelForDomain(PackageSetting, Intent, int)} - * which sorts packages by approval priority. A higher numerical value means the package should - * override all lower values. This means that comparison using less/greater than IS valid. + * Defines the possible values for + * {@link #approvalLevelForDomain(PackageSetting, Intent, List, int, int)} which sorts packages + * by approval priority. A higher numerical value means the package should override all lower + * values. This means that comparison using less/greater than IS valid. * * Negative values are possible, although not implemented, reserved if explicit disable of a * package for a domain needs to be tracked. @@ -184,7 +189,7 @@ public interface DomainVerificationManagerInternal { /** * Migrates verification state from a previous install to a new one. It is expected that the * {@link PackageSetting#getDomainSetId()} already be set to the correct value, usually from - * {@link #generateNewId()}. This will preserve {@link DomainVerificationManager#STATE_SUCCESS} + * {@link #generateNewId()}. This will preserve {@link DomainVerificationState#STATE_SUCCESS} * domains under the assumption that the new package will pass the same server side config as * the previous package, as they have matching signatures. *

@@ -276,7 +281,7 @@ public interface DomainVerificationManagerInternal { /** * Until the legacy APIs are entirely removed, returns the legacy state from the previously - * written info stored in {@link com.android.server.pm.Settings}. + * written info stored in {@link Settings}. */ int getLegacyState(@NonNull String packageName, @UserIdInt int userId); @@ -288,15 +293,12 @@ public interface DomainVerificationManagerInternal { * @param userId the specific user to print, or null to skip printing user selection * states, supports {@link android.os.UserHandle#USER_ALL} * @param pkgSettingFunction the method by which to retrieve package data; if this is called - * from {@link com.android.server.pm.PackageManagerService}, it is - * expected to pass in the snapshot of {@link PackageSetting} objects, - * or if null is passed, the manager may decide to lock {@link - * com.android.server.pm.PackageManagerService} through {@link - * Connection#getPackageSettingLocked(String)} + * from {@link PackageManagerService}, it is + * expected to pass in the snapshot of {@link PackageSetting} objects */ void printState(@NonNull IndentingPrintWriter writer, @Nullable String packageName, @Nullable @UserIdInt Integer userId, - @Nullable Function pkgSettingFunction) + @NonNull Function pkgSettingFunction) throws NameNotFoundException; @NonNull @@ -368,17 +370,46 @@ public interface DomainVerificationManagerInternal { */ void schedule(int code, @Nullable Object object); - // TODO(b/178733426): Make DomainVerificationService PMS snapshot aware so it can avoid - // locking package state at all. This can be as simple as removing this method in favor of - // accepting a PackageSetting function in at every method call, although should probably - // be abstracted to a wrapper class. - @Nullable - PackageSetting getPackageSettingLocked(@NonNull String pkgName); + /** + * Run a function block that requires access to {@link PackageSetting} data. This will + * ensure the {@link PackageManagerService} is taken before + * {@link DomainVerificationManagerInternal}'s lock is taken to avoid deadlock. + */ + void withPackageSettings(@NonNull Consumer> block); - @Nullable - AndroidPackage getPackageLocked(@NonNull String pkgName); + /** + * Variant which returns a value to the caller. + * @see #withPackageSettings(Consumer) + */ + Output withPackageSettingsReturning( + @NonNull FunctionalUtils.ThrowingFunction, Output> + block); + + /** + * Variant which throws. + * @see #withPackageSettings(Consumer) + */ + void withPackageSettingsThrowing( + @NonNull ThrowingConsumer, ExceptionType> block) + throws ExceptionType; + + /** + * Variant which returns a value to the caller and throws. + * @see #withPackageSettings(Consumer) + */ + Output withPackageSettingsReturningThrowing( + @NonNull ThrowingFunction, Output, ExceptionType> + block) throws ExceptionType; @UserIdInt int[] getAllUserIds(); + + interface ThrowingConsumer { + void accept(Input input) throws ExceptionType; + } + + interface ThrowingFunction { + Output apply(Input input) throws ExceptionType; + } } } diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java index 7e3fb53a4da4c..81adbe197ee27 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java @@ -38,6 +38,7 @@ import android.content.pm.verify.domain.DomainVerificationManager; import android.content.pm.verify.domain.DomainVerificationState; import android.content.pm.verify.domain.DomainVerificationUserState; import android.content.pm.verify.domain.IDomainVerificationManager; +import android.os.Build; import android.os.UserHandle; import android.util.ArrayMap; import android.util.ArraySet; @@ -51,10 +52,12 @@ import android.util.TypedXmlSerializer; import com.android.internal.annotations.GuardedBy; import com.android.internal.util.CollectionUtils; +import com.android.internal.util.FunctionalUtils; import com.android.server.SystemConfig; import com.android.server.SystemService; import com.android.server.compat.PlatformCompat; import com.android.server.pm.PackageSetting; +import com.android.server.pm.Settings; import com.android.server.pm.parsing.pkg.AndroidPackage; import com.android.server.pm.verify.domain.models.DomainVerificationInternalUserState; import com.android.server.pm.verify.domain.models.DomainVerificationPkgState; @@ -73,6 +76,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.UUID; +import java.util.function.Consumer; import java.util.function.Function; public class DomainVerificationService extends SystemService @@ -100,9 +104,9 @@ public class DomainVerificationService extends SystemService * immediately attached once its available. *

* Generally this should be not accessed directly. Prefer calling {@link - * #getAndValidateAttachedLocked(UUID, Set, boolean, int, Integer)}. + * #getAndValidateAttachedLocked(UUID, Set, boolean, int, Integer, Function)}. * - * @see #getAndValidateAttachedLocked(UUID, Set, boolean, int, Integer) + * @see #getAndValidateAttachedLocked(UUID, Set, boolean, int, Integer, Function) **/ @GuardedBy("mLock") @NonNull @@ -169,7 +173,12 @@ public class DomainVerificationService extends SystemService @Override public void setConnection(@NonNull Connection connection) { - mConnection = connection; + if (Build.IS_USERDEBUG || Build.IS_ENG) { + mConnection = new LockSafeConnection(connection); + } else { + mConnection = connection; + } + mEnforcer.setCallback(mConnection); } @@ -250,41 +259,44 @@ public class DomainVerificationService extends SystemService public DomainVerificationInfo getDomainVerificationInfo(@NonNull String packageName) throws NameNotFoundException { mEnforcer.assertApprovedQuerent(mConnection.getCallingUid(), mProxy); - synchronized (mLock) { - AndroidPackage pkg = mConnection.getPackageLocked(packageName); - if (pkg == null) { - throw DomainVerificationUtils.throwPackageUnavailable(packageName); + return mConnection.withPackageSettingsReturningThrowing(pkgSettings -> { + synchronized (mLock) { + PackageSetting pkgSetting = pkgSettings.apply(packageName); + AndroidPackage pkg = pkgSetting == null ? null : pkgSetting.getPkg(); + if (pkg == null) { + throw DomainVerificationUtils.throwPackageUnavailable(packageName); + } + + DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); + if (pkgState == null) { + return null; + } + + ArrayMap hostToStateMap = new ArrayMap<>(pkgState.getStateMap()); + + // TODO(b/159952358): Should the domain list be cached? + ArraySet domains = mCollector.collectValidAutoVerifyDomains(pkg); + if (domains.isEmpty()) { + return null; + } + + int size = domains.size(); + for (int index = 0; index < size; index++) { + hostToStateMap.putIfAbsent(domains.valueAt(index), + DomainVerificationState.STATE_NO_RESPONSE); + } + + final int mapSize = hostToStateMap.size(); + for (int index = 0; index < mapSize; index++) { + int internalValue = hostToStateMap.valueAt(index); + int publicValue = DomainVerificationState.convertToInfoState(internalValue); + hostToStateMap.setValueAt(index, publicValue); + } + + // TODO(b/159952358): Do not return if no values are editable (all ignored states)? + return new DomainVerificationInfo(pkgState.getId(), packageName, hostToStateMap); } - - DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); - if (pkgState == null) { - return null; - } - - ArrayMap hostToStateMap = new ArrayMap<>(pkgState.getStateMap()); - - // TODO(b/159952358): Should the domain list be cached? - ArraySet domains = mCollector.collectValidAutoVerifyDomains(pkg); - if (domains.isEmpty()) { - return null; - } - - int size = domains.size(); - for (int index = 0; index < size; index++) { - hostToStateMap.putIfAbsent(domains.valueAt(index), - DomainVerificationState.STATE_NO_RESPONSE); - } - - final int mapSize = hostToStateMap.size(); - for (int index = 0; index < mapSize; index++) { - int internalValue = hostToStateMap.valueAt(index); - int publicValue = DomainVerificationState.convertToInfoState(internalValue); - hostToStateMap.setValueAt(index, publicValue); - } - - // TODO(b/159952358): Do not return if no values are editable (all ignored states)? - return new DomainVerificationInfo(pkgState.getId(), packageName, hostToStateMap); - } + }); } @DomainVerificationManager.Error @@ -307,44 +319,47 @@ public class DomainVerificationService extends SystemService @NonNull Set domains, int state) throws NameNotFoundException { mEnforcer.assertApprovedVerifier(callingUid, mProxy); - synchronized (mLock) { - List verifiedDomains = new ArrayList<>(); + return mConnection.withPackageSettingsReturningThrowing(pkgSettings -> { + synchronized (mLock) { + List verifiedDomains = new ArrayList<>(); - GetAttachedResult result = getAndValidateAttachedLocked(domainSetId, domains, - true /* forAutoVerify */, callingUid, null /* userId */); - if (result.isError()) { - return result.getErrorCode(); - } - - DomainVerificationPkgState pkgState = result.getPkgState(); - ArrayMap stateMap = pkgState.getStateMap(); - for (String domain : domains) { - Integer previousState = stateMap.get(domain); - if (previousState != null - && !DomainVerificationState.isModifiable(previousState)) { - continue; + GetAttachedResult result = getAndValidateAttachedLocked(domainSetId, domains, + true /* forAutoVerify */, callingUid, null /* userId */, + pkgSettings); + if (result.isError()) { + return result.getErrorCode(); } - if (DomainVerificationState.isVerified(state)) { - verifiedDomains.add(domain); + DomainVerificationPkgState pkgState = result.getPkgState(); + ArrayMap stateMap = pkgState.getStateMap(); + for (String domain : domains) { + Integer previousState = stateMap.get(domain); + if (previousState != null + && !DomainVerificationState.isModifiable(previousState)) { + continue; + } + + if (DomainVerificationState.isVerified(state)) { + verifiedDomains.add(domain); + } + + stateMap.put(domain, state); } - stateMap.put(domain, state); + int size = verifiedDomains.size(); + for (int index = 0; index < size; index++) { + removeUserStatesForDomain(verifiedDomains.get(index)); + } } - int size = verifiedDomains.size(); - for (int index = 0; index < size; index++) { - removeUserStatesForDomain(verifiedDomains.get(index)); - } - } - - mConnection.scheduleWriteSettings(); - return DomainVerificationManager.STATUS_OK; + mConnection.scheduleWriteSettings(); + return DomainVerificationManager.STATUS_OK; + }); } @Override public void setDomainVerificationStatusInternal(@Nullable String packageName, int state, - @Nullable ArraySet domains) throws NameNotFoundException { + @Nullable final ArraySet domains) throws NameNotFoundException { mEnforcer.assertInternal(mConnection.getCallingUid()); switch (state) { @@ -359,31 +374,61 @@ public class DomainVerificationService extends SystemService } ArraySet verifiedDomains = new ArraySet<>(); - if (packageName == null) { - synchronized (mLock) { - ArraySet validDomains = new ArraySet<>(); + mConnection.withPackageSettings(pkgSettings -> { + synchronized (mLock) { + ArraySet validDomains = new ArraySet<>(); - int size = mAttachedPkgStates.size(); - for (int index = 0; index < size; index++) { - DomainVerificationPkgState pkgState = mAttachedPkgStates.valueAt(index); - String pkgName = pkgState.getPackageName(); - PackageSetting pkgSetting = mConnection.getPackageSettingLocked(pkgName); + int size = mAttachedPkgStates.size(); + for (int index = 0; index < size; index++) { + DomainVerificationPkgState pkgState = mAttachedPkgStates.valueAt(index); + String pkgName = pkgState.getPackageName(); + PackageSetting pkgSetting = pkgSettings.apply(pkgName); + if (pkgSetting == null || pkgSetting.getPkg() == null) { + continue; + } + + AndroidPackage pkg = pkgSetting.getPkg(); + + validDomains.clear(); + + ArraySet autoVerifyDomains = + mCollector.collectValidAutoVerifyDomains(pkg); + if (domains == null) { + validDomains.addAll(autoVerifyDomains); + } else { + validDomains.addAll(domains); + validDomains.retainAll(autoVerifyDomains); + } + + if (DomainVerificationState.isVerified(state)) { + verifiedDomains.addAll(validDomains); + } + + setDomainVerificationStatusInternal(pkgState, state, validDomains); + } + } + }); + } else { + mConnection.withPackageSettingsThrowing(pkgSettings -> { + synchronized (mLock) { + DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); + if (pkgState == null) { + throw DomainVerificationUtils.throwPackageUnavailable(packageName); + } + + PackageSetting pkgSetting = pkgSettings.apply(packageName); if (pkgSetting == null || pkgSetting.getPkg() == null) { - continue; + throw DomainVerificationUtils.throwPackageUnavailable(packageName); } AndroidPackage pkg = pkgSetting.getPkg(); - - validDomains.clear(); - - ArraySet autoVerifyDomains = - mCollector.collectValidAutoVerifyDomains(pkg); + final ArraySet validDomains; if (domains == null) { - validDomains.addAll(autoVerifyDomains); + validDomains = mCollector.collectValidAutoVerifyDomains(pkg); } else { - validDomains.addAll(domains); - validDomains.retainAll(autoVerifyDomains); + validDomains = domains; + validDomains.retainAll(mCollector.collectValidAutoVerifyDomains(pkg)); } if (DomainVerificationState.isVerified(state)) { @@ -392,32 +437,7 @@ public class DomainVerificationService extends SystemService setDomainVerificationStatusInternal(pkgState, state, validDomains); } - } - } else { - synchronized (mLock) { - DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); - if (pkgState == null) { - throw DomainVerificationUtils.throwPackageUnavailable(packageName); - } - - PackageSetting pkgSetting = mConnection.getPackageSettingLocked(packageName); - if (pkgSetting == null || pkgSetting.getPkg() == null) { - throw DomainVerificationUtils.throwPackageUnavailable(packageName); - } - - AndroidPackage pkg = pkgSetting.getPkg(); - if (domains == null) { - domains = mCollector.collectValidAutoVerifyDomains(pkg); - } else { - domains.retainAll(mCollector.collectValidAutoVerifyDomains(pkg)); - } - - if (DomainVerificationState.isVerified(state)) { - verifiedDomains.addAll(domains); - } - - setDomainVerificationStatusInternal(pkgState, state, domains); - } + }); } // Mirror SystemApi behavior of revoking user selection for approved domains. @@ -518,44 +538,48 @@ public class DomainVerificationService extends SystemService public int setDomainVerificationUserSelection(@NonNull UUID domainSetId, @NonNull Set domains, boolean enabled, @UserIdInt int userId) throws NameNotFoundException { - synchronized (mLock) { - final int callingUid = mConnection.getCallingUid(); - // Pass null for package name here and do the app visibility enforcement inside - // getAndValidateAttachedLocked instead, since this has to fail with the same invalid - // ID reason if the target app is invisible - if (!mEnforcer.assertApprovedUserSelector(callingUid, mConnection.getCallingUserId(), - null /* packageName */, userId)) { - return DomainVerificationManager.ERROR_DOMAIN_SET_ID_INVALID; - } + final int callingUid = mConnection.getCallingUid(); + // Pass null for package name here and do the app visibility enforcement inside + // getAndValidateAttachedLocked instead, since this has to fail with the same invalid + // ID reason if the target app is invisible + if (!mEnforcer.assertApprovedUserSelector(callingUid, mConnection.getCallingUserId(), + null /* packageName */, userId)) { + return DomainVerificationManager.ERROR_DOMAIN_SET_ID_INVALID; + } - GetAttachedResult result = getAndValidateAttachedLocked(domainSetId, domains, - false /* forAutoVerify */, callingUid, userId); - if (result.isError()) { - return result.getErrorCode(); - } + return mConnection.withPackageSettingsReturningThrowing(pkgSettings -> { + synchronized (mLock) { + GetAttachedResult result = getAndValidateAttachedLocked(domainSetId, domains, + false /* forAutoVerify */, callingUid, userId, pkgSettings); + if (result.isError()) { + return result.getErrorCode(); + } - DomainVerificationPkgState pkgState = result.getPkgState(); - DomainVerificationInternalUserState userState = pkgState.getOrCreateUserState(userId); + DomainVerificationPkgState pkgState = result.getPkgState(); + DomainVerificationInternalUserState userState = pkgState.getOrCreateUserState( + userId); - // Disable other packages if approving this one. Note that this check is only done for - // enabling. This allows an escape hatch in case multiple packages somehow get selected. - // They can be disabled without blocking in a circular dependency. - if (enabled) { - int statusCode = revokeOtherUserSelections(userState, userId, domains); - if (statusCode != DomainVerificationManager.STATUS_OK) { - return statusCode; + // Disable other packages if approving this one. Note that this check is only done + // for enabling. This allows an escape hatch in case multiple packages somehow get + // selected. They can be disabled without blocking in a circular dependency. + if (enabled) { + int statusCode = revokeOtherUserSelectionsLocked(userState, userId, domains, + pkgSettings); + if (statusCode != DomainVerificationManager.STATUS_OK) { + return statusCode; + } + } + + if (enabled) { + userState.addHosts(domains); + } else { + userState.removeHosts(domains); } } - if (enabled) { - userState.addHosts(domains); - } else { - userState.removeHosts(domains); - } - } - - mConnection.scheduleWriteSettings(); - return DomainVerificationManager.STATUS_OK; + mConnection.scheduleWriteSettings(); + return DomainVerificationManager.STATUS_OK; + }); } @Override @@ -563,52 +587,57 @@ public class DomainVerificationService extends SystemService @NonNull String packageName, boolean enabled, @Nullable ArraySet domains) throws NameNotFoundException { mEnforcer.assertInternal(mConnection.getCallingUid()); + mConnection.withPackageSettingsThrowing(pkgSettings -> { + synchronized (mLock) { + DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); + if (pkgState == null) { + throw DomainVerificationUtils.throwPackageUnavailable(packageName); + } - synchronized (mLock) { - DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); - if (pkgState == null) { - throw DomainVerificationUtils.throwPackageUnavailable(packageName); - } + PackageSetting pkgSetting = pkgSettings.apply(packageName); + AndroidPackage pkg = pkgSetting == null ? null : pkgSetting.getPkg(); + if (pkg == null) { + throw DomainVerificationUtils.throwPackageUnavailable(packageName); + } - PackageSetting pkgSetting = mConnection.getPackageSettingLocked(packageName); - AndroidPackage pkg = pkgSetting == null ? null : pkgSetting.getPkg(); - if (pkg == null) { - throw DomainVerificationUtils.throwPackageUnavailable(packageName); - } + Set validDomains = + domains == null ? mCollector.collectAllWebDomains(pkg) : domains; - Set validDomains = - domains == null ? mCollector.collectAllWebDomains(pkg) : domains; + validDomains.retainAll(mCollector.collectAllWebDomains(pkg)); - validDomains.retainAll(mCollector.collectAllWebDomains(pkg)); - - if (userId == UserHandle.USER_ALL) { - for (int aUserId : mConnection.getAllUserIds()) { + if (userId == UserHandle.USER_ALL) { + for (int aUserId : mConnection.getAllUserIds()) { + DomainVerificationInternalUserState userState = + pkgState.getOrCreateUserState(aUserId); + revokeOtherUserSelectionsLocked(userState, aUserId, validDomains, + pkgSettings); + if (enabled) { + userState.addHosts(validDomains); + } else { + userState.removeHosts(validDomains); + } + } + } else { DomainVerificationInternalUserState userState = - pkgState.getOrCreateUserState(aUserId); - revokeOtherUserSelections(userState, aUserId, validDomains); + pkgState.getOrCreateUserState(userId); + revokeOtherUserSelectionsLocked(userState, userId, validDomains, pkgSettings); if (enabled) { userState.addHosts(validDomains); } else { userState.removeHosts(validDomains); } } - } else { - DomainVerificationInternalUserState userState = - pkgState.getOrCreateUserState(userId); - revokeOtherUserSelections(userState, userId, validDomains); - if (enabled) { - userState.addHosts(validDomains); - } else { - userState.removeHosts(validDomains); - } } - } + }); mConnection.scheduleWriteSettings(); } - private int revokeOtherUserSelections(@NonNull DomainVerificationInternalUserState userState, - @UserIdInt int userId, @NonNull Set domains) { + @GuardedBy("mLock") + private int revokeOtherUserSelectionsLocked( + @NonNull DomainVerificationInternalUserState userState, @UserIdInt int userId, + @NonNull Set domains, + @NonNull Function pkgSettingFunction) { // Cache the approved packages from the 1st pass because the search is expensive ArrayMap> domainToApprovedPackages = new ArrayMap<>(); @@ -617,8 +646,8 @@ public class DomainVerificationService extends SystemService continue; } - Pair, Integer> packagesToLevel = getApprovedPackages(domain, - userId, APPROVAL_LEVEL_NONE + 1, mConnection::getPackageSettingLocked); + Pair, Integer> packagesToLevel = getApprovedPackagesLocked(domain, + userId, APPROVAL_LEVEL_NONE + 1, pkgSettingFunction); int highestApproval = packagesToLevel.second; if (highestApproval > APPROVAL_LEVEL_SELECTION) { return DomainVerificationManager.ERROR_UNABLE_TO_APPROVE; @@ -663,46 +692,52 @@ public class DomainVerificationService extends SystemService mConnection.getCallingUserId(), packageName, userId)) { throw DomainVerificationUtils.throwPackageUnavailable(packageName); } - synchronized (mLock) { - AndroidPackage pkg = mConnection.getPackageLocked(packageName); - if (pkg == null) { - throw DomainVerificationUtils.throwPackageUnavailable(packageName); - } - DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); - if (pkgState == null) { - return null; - } - - ArraySet webDomains = mCollector.collectAllWebDomains(pkg); - int webDomainsSize = webDomains.size(); - - Map domains = new ArrayMap<>(webDomainsSize); - ArrayMap stateMap = pkgState.getStateMap(); - DomainVerificationInternalUserState userState = pkgState.getUserState(userId); - Set enabledHosts = userState == null ? emptySet() : userState.getEnabledHosts(); - - for (int index = 0; index < webDomainsSize; index++) { - String host = webDomains.valueAt(index); - Integer state = stateMap.get(host); - - int domainState; - if (state != null && DomainVerificationState.isVerified(state)) { - domainState = DomainVerificationUserState.DOMAIN_STATE_VERIFIED; - } else if (enabledHosts.contains(host)) { - domainState = DomainVerificationUserState.DOMAIN_STATE_SELECTED; - } else { - domainState = DomainVerificationUserState.DOMAIN_STATE_NONE; + return mConnection.withPackageSettingsReturningThrowing(pkgSettings -> { + synchronized (mLock) { + PackageSetting pkgSetting = pkgSettings.apply(packageName); + AndroidPackage pkg = pkgSetting == null ? null : pkgSetting.getPkg(); + if (pkg == null) { + throw DomainVerificationUtils.throwPackageUnavailable(packageName); } - domains.put(host, domainState); + DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); + if (pkgState == null) { + return null; + } + + ArraySet webDomains = mCollector.collectAllWebDomains(pkg); + int webDomainsSize = webDomains.size(); + + Map domains = new ArrayMap<>(webDomainsSize); + ArrayMap stateMap = pkgState.getStateMap(); + DomainVerificationInternalUserState userState = pkgState.getUserState(userId); + Set enabledHosts = + userState == null ? emptySet() : userState.getEnabledHosts(); + + for (int index = 0; index < webDomainsSize; index++) { + String host = webDomains.valueAt(index); + Integer state = stateMap.get(host); + + int domainState; + if (state != null && DomainVerificationState.isVerified(state)) { + domainState = DomainVerificationUserState.DOMAIN_STATE_VERIFIED; + } else if (enabledHosts.contains(host)) { + domainState = DomainVerificationUserState.DOMAIN_STATE_SELECTED; + } else { + domainState = DomainVerificationUserState.DOMAIN_STATE_NONE; + } + + domains.put(host, domainState); + } + + boolean linkHandlingAllowed = + userState == null || userState.isLinkHandlingAllowed(); + + return new DomainVerificationUserState(pkgState.getId(), packageName, + UserHandle.of(userId), linkHandlingAllowed, domains); } - - boolean linkHandlingAllowed = userState == null || userState.isLinkHandlingAllowed(); - - return new DomainVerificationUserState(pkgState.getId(), packageName, - UserHandle.of(userId), linkHandlingAllowed, domains); - } + }); } public List getOwnersForDomain(@NonNull String domain, @UserIdInt int userId) { @@ -710,67 +745,68 @@ public class DomainVerificationService extends SystemService userId); SparseArray> levelToPackages = new SparseArray<>(); + return mConnection.withPackageSettingsReturningThrowing(pkgSettings -> { + // First, collect the raw approval level values + synchronized (mLock) { + final int size = mAttachedPkgStates.size(); + for (int index = 0; index < size; index++) { + DomainVerificationPkgState pkgState = mAttachedPkgStates.valueAt(index); + String packageName = pkgState.getPackageName(); + PackageSetting pkgSetting = pkgSettings.apply(packageName); + if (pkgSetting == null) { + continue; + } - // First, collect the raw approval level values - synchronized (mLock) { - final int size = mAttachedPkgStates.size(); + int level = approvalLevelForDomain(pkgSetting, domain, userId, domain); + if (level <= APPROVAL_LEVEL_NONE) { + continue; + } + List list = levelToPackages.get(level); + if (list == null) { + list = new ArrayList<>(); + levelToPackages.put(level, list); + } + list.add(packageName); + } + } + + final int size = levelToPackages.size(); + if (size == 0) { + return emptyList(); + } + + // Then sort them ascending by first installed time, with package name as tie breaker for (int index = 0; index < size; index++) { - DomainVerificationPkgState pkgState = mAttachedPkgStates.valueAt(index); - String packageName = pkgState.getPackageName(); - PackageSetting pkgSetting = mConnection.getPackageSettingLocked(packageName); - if (pkgSetting == null) { - continue; - } + levelToPackages.valueAt(index).sort((first, second) -> { + PackageSetting firstPkgSetting = pkgSettings.apply(first); + PackageSetting secondPkgSetting = pkgSettings.apply(second); - int level = approvalLevelForDomain(pkgSetting, domain, userId, domain); - if (level <= APPROVAL_LEVEL_NONE) { - continue; - } - List list = levelToPackages.get(level); - if (list == null) { - list = new ArrayList<>(); - levelToPackages.put(level, list); - } - list.add(packageName); + long firstInstallTime = + firstPkgSetting == null ? -1L : firstPkgSetting.getFirstInstallTime(); + long secondInstallTime = + secondPkgSetting == null ? -1L : secondPkgSetting.getFirstInstallTime(); + + if (firstInstallTime != secondInstallTime) { + return (int) (firstInstallTime - secondInstallTime); + } + + return first.compareToIgnoreCase(second); + }); } - } - final int size = levelToPackages.size(); - if (size == 0) { - return emptyList(); - } - - // Then sort them ascending by first installed time, with package name as the tie breaker - for (int index = 0; index < size; index++) { - levelToPackages.valueAt(index).sort((first, second) -> { - PackageSetting firstPkgSetting = mConnection.getPackageSettingLocked(first); - PackageSetting secondPkgSetting = mConnection.getPackageSettingLocked(second); - - long firstInstallTime = - firstPkgSetting == null ? -1L : firstPkgSetting.getFirstInstallTime(); - long secondInstallTime = - secondPkgSetting == null ? -1L : secondPkgSetting.getFirstInstallTime(); - - if (firstInstallTime != secondInstallTime) { - return (int) (firstInstallTime - secondInstallTime); + List owners = new ArrayList<>(); + for (int index = 0; index < size; index++) { + int level = levelToPackages.keyAt(index); + boolean overrideable = level <= APPROVAL_LEVEL_SELECTION; + List packages = levelToPackages.valueAt(index); + int packagesSize = packages.size(); + for (int packageIndex = 0; packageIndex < packagesSize; packageIndex++) { + owners.add(new DomainOwner(packages.get(packageIndex), overrideable)); } - - return first.compareToIgnoreCase(second); - }); - } - - List owners = new ArrayList<>(); - for (int index = 0; index < size; index++) { - int level = levelToPackages.keyAt(index); - boolean overrideable = level <= APPROVAL_LEVEL_SELECTION; - List packages = levelToPackages.valueAt(index); - int packagesSize = packages.size(); - for (int packageIndex = 0; packageIndex < packagesSize; packageIndex++) { - owners.add(new DomainOwner(packages.get(packageIndex), overrideable)); } - } - return owners; + return owners; + }); } @NonNull @@ -1085,21 +1121,15 @@ public class DomainVerificationService extends SystemService @Override public void printState(@NonNull IndentingPrintWriter writer, @Nullable String packageName, @Nullable Integer userId) throws NameNotFoundException { - // This method is only used by DomainVerificationShell, which doesn't lock PMS, so it's - // safe to pass mConnection directly here and lock PMS. This method is not exposed - // to the general system server/PMS. - printState(writer, packageName, userId, mConnection::getPackageSettingLocked); + mConnection.withPackageSettingsThrowing( + pkgSettings -> printState(writer, packageName, userId, pkgSettings)); } @Override public void printState(@NonNull IndentingPrintWriter writer, @Nullable String packageName, @Nullable @UserIdInt Integer userId, - @Nullable Function pkgSettingFunction) + @NonNull Function pkgSettingFunction) throws NameNotFoundException { - if (pkgSettingFunction == null) { - pkgSettingFunction = mConnection::getPackageSettingLocked; - } - synchronized (mLock) { mDebug.printState(writer, packageName, userId, pkgSettingFunction, mAttachedPkgStates); } @@ -1148,7 +1178,9 @@ public class DomainVerificationService extends SystemService @GuardedBy("mLock") private GetAttachedResult getAndValidateAttachedLocked(@NonNull UUID domainSetId, @NonNull Set domains, boolean forAutoVerify, int callingUid, - @Nullable Integer userIdForFilter) throws NameNotFoundException { + @Nullable Integer userIdForFilter, + @NonNull Function pkgSettingFunction) + throws NameNotFoundException { if (domainSetId == null) { throw new IllegalArgumentException("domainSetId cannot be null"); } @@ -1165,7 +1197,7 @@ public class DomainVerificationService extends SystemService return GetAttachedResult.error(DomainVerificationManager.ERROR_DOMAIN_SET_ID_INVALID); } - PackageSetting pkgSetting = mConnection.getPackageSettingLocked(pkgName); + PackageSetting pkgSetting = pkgSettingFunction.apply(pkgName); if (pkgSetting == null || pkgSetting.getPkg() == null) { throw DomainVerificationUtils.throwPackageUnavailable(pkgName); } @@ -1253,31 +1285,33 @@ public class DomainVerificationService extends SystemService @Override public void clearDomainVerificationState(@Nullable List packageNames) { mEnforcer.assertInternal(mConnection.getCallingUid()); - synchronized (mLock) { - if (packageNames == null) { - int size = mAttachedPkgStates.size(); - for (int index = 0; index < size; index++) { - DomainVerificationPkgState pkgState = mAttachedPkgStates.valueAt(index); - String pkgName = pkgState.getPackageName(); - PackageSetting pkgSetting = mConnection.getPackageSettingLocked(pkgName); - if (pkgSetting == null || pkgSetting.getPkg() == null) { - continue; + mConnection.withPackageSettings(pkgSettings -> { + synchronized (mLock) { + if (packageNames == null) { + int size = mAttachedPkgStates.size(); + for (int index = 0; index < size; index++) { + DomainVerificationPkgState pkgState = mAttachedPkgStates.valueAt(index); + String pkgName = pkgState.getPackageName(); + PackageSetting pkgSetting = pkgSettings.apply(pkgName); + if (pkgSetting == null || pkgSetting.getPkg() == null) { + continue; + } + resetDomainState(pkgState, pkgSetting.getPkg()); } - resetDomainState(pkgState, pkgSetting.getPkg()); - } - } else { - int size = packageNames.size(); - for (int index = 0; index < size; index++) { - String pkgName = packageNames.get(index); - DomainVerificationPkgState pkgState = mAttachedPkgStates.get(pkgName); - PackageSetting pkgSetting = mConnection.getPackageSettingLocked(pkgName); - if (pkgSetting == null || pkgSetting.getPkg() == null) { - continue; + } else { + int size = packageNames.size(); + for (int index = 0; index < size; index++) { + String pkgName = packageNames.get(index); + DomainVerificationPkgState pkgState = mAttachedPkgStates.get(pkgName); + PackageSetting pkgSetting = pkgSettings.apply(pkgName); + if (pkgSetting == null || pkgSetting.getPkg() == null) { + continue; + } + resetDomainState(pkgState, pkgSetting.getPkg()); } - resetDomainState(pkgState, pkgSetting.getPkg()); } } - } + }); mConnection.scheduleWriteSettings(); } @@ -1692,8 +1726,9 @@ public class DomainVerificationService extends SystemService /** * @return the filtered list paired with the corresponding approval level */ + @GuardedBy("mLock") @NonNull - private Pair, Integer> getApprovedPackages(@NonNull String domain, + private Pair, Integer> getApprovedPackagesLocked(@NonNull String domain, @UserIdInt int userId, int minimumApproval, @NonNull Function pkgSettingFunction) { int highestApproval = minimumApproval; @@ -1792,4 +1827,93 @@ public class DomainVerificationService extends SystemService return mErrorCode; } } + + /** + * Wraps a {@link Connection} to verify that the {@link PackageSetting} calls do not hold + * {@link #mLock}, as that can cause deadlock when {@link Settings} tries to serialize state to + * disk. Only enabled if {@link Build#IS_USERDEBUG} or {@link Build#IS_ENG} is true. + */ + private class LockSafeConnection implements Connection { + + @NonNull + private final Connection mConnection; + + private LockSafeConnection(@NonNull Connection connection) { + mConnection = connection; + } + + private void enforceLocking() { + if (Thread.holdsLock(mLock)) { + Slog.wtf(TAG, "Method should not hold DVS lock when accessing package data"); + } + } + + @Override + public void withPackageSettings( + @NonNull Consumer> block) { + enforceLocking(); + mConnection.withPackageSettings(block); + } + + @Override + public Output withPackageSettingsReturning( + @NonNull FunctionalUtils.ThrowingFunction, Output> + block) { + enforceLocking(); + return mConnection.withPackageSettingsReturning(block); + } + + @Override + public void withPackageSettingsThrowing( + @NonNull ThrowingConsumer, ExceptionType> block) + throws ExceptionType { + enforceLocking(); + mConnection.withPackageSettingsThrowing(block); + } + + @Override + public Output + withPackageSettingsReturningThrowing(@NonNull ThrowingFunction, Output, ExceptionType> block) throws ExceptionType { + enforceLocking(); + return mConnection.withPackageSettingsReturningThrowing(block); + } + + @Override + public void scheduleWriteSettings() { + mConnection.scheduleWriteSettings(); + } + + @Override + public int getCallingUid() { + return mConnection.getCallingUid(); + } + + @Override + @UserIdInt + public int getCallingUserId() { + return mConnection.getCallingUserId(); + } + + @Override + public void schedule(int code, @Nullable Object object) { + mConnection.schedule(code, object); + } + + @Override + @UserIdInt + public int[] getAllUserIds() { + return mConnection.getAllUserIds(); + } + + @Override + public boolean filterAppAccess(@NonNull String packageName, int callingUid, int userId) { + return mConnection.filterAppAccess(packageName, callingUid, userId); + } + + @Override + public boolean doesUserExist(int userId) { + return mConnection.doesUserExist(userId); + } + } } diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt index 01a76eea2b0d8..2d852e5017b45 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt @@ -31,6 +31,7 @@ import android.util.SparseArray import androidx.test.platform.app.InstrumentationRegistry import com.android.server.pm.PackageSetting import com.android.server.pm.parsing.pkg.AndroidPackage +import com.android.server.pm.test.verify.domain.DomainVerificationTestUtils.mockPackageSettings import com.android.server.pm.verify.domain.DomainVerificationEnforcer import com.android.server.pm.verify.domain.DomainVerificationManagerInternal import com.android.server.pm.verify.domain.DomainVerificationService @@ -98,10 +99,13 @@ class DomainVerificationEnforcerTest { mockThrowOnUnmocked { whenever(callingUid) { callingUidInt.get() } whenever(callingUserId) { callingUserIdInt.get() } - whenever(getPackageSettingLocked(VISIBLE_PKG)) { visiblePkgSetting } - whenever(getPackageLocked(VISIBLE_PKG)) { visiblePkg } - whenever(getPackageSettingLocked(INVISIBLE_PKG)) { invisiblePkgSetting } - whenever(getPackageLocked(INVISIBLE_PKG)) { invisiblePkg } + mockPackageSettings { + when (it) { + VISIBLE_PKG -> visiblePkgSetting + INVISIBLE_PKG -> invisiblePkgSetting + else -> null + } + } whenever(schedule(anyInt(), any())) whenever(scheduleWriteSettings()) whenever(filterAppAccess(eq(VISIBLE_PKG), anyInt(), anyInt())) { false } diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt index f8e0c60c25df6..adc1dab1572e2 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt @@ -32,6 +32,7 @@ import android.os.Process import android.util.ArraySet import com.android.server.pm.PackageSetting import com.android.server.pm.parsing.pkg.AndroidPackage +import com.android.server.pm.test.verify.domain.DomainVerificationTestUtils.mockPackageSettings import com.android.server.pm.verify.domain.DomainVerificationService import com.android.server.testutils.mockThrowOnUnmocked import com.android.server.testutils.whenever @@ -445,11 +446,8 @@ class DomainVerificationManagerApiTest { whenever(callingUid) { Process.ROOT_UID } whenever(callingUserId) { 0 } - whenever(getPackageSettingLocked(anyString())) { - pkgSettingFunction(arguments[0] as String) - } - whenever(getPackageLocked(anyString())) { - pkgSettingFunction(arguments[0] as String)?.getPkg() + mockPackageSettings { + pkgSettingFunction(it) } }) } diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt index 5f601e2648cb5..8a549731af3a2 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt @@ -36,6 +36,7 @@ import android.util.ArraySet import android.util.Xml import com.android.server.pm.PackageSetting import com.android.server.pm.parsing.pkg.AndroidPackage +import com.android.server.pm.test.verify.domain.DomainVerificationTestUtils.mockPackageSettings import com.android.server.pm.verify.domain.DomainVerificationService import com.android.server.testutils.mockThrowOnUnmocked import com.android.server.testutils.whenever @@ -380,11 +381,8 @@ class DomainVerificationPackageTest { whenever(callingUid) { Process.ROOT_UID } whenever(callingUserId) { 0 } - whenever(getPackageSettingLocked(anyString())) { - pkgSettingFunction(arguments[0] as String)!! - } - whenever(getPackageLocked(anyString())) { - pkgSettingFunction(arguments[0] as String)!!.getPkg() + mockPackageSettings { + pkgSettingFunction(it) } }) } diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt index 1cf57c41145a0..26e8d286cabe9 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationSettingsMutationTest.kt @@ -29,6 +29,7 @@ import android.util.ArraySet import android.util.SparseArray import com.android.server.pm.PackageSetting import com.android.server.pm.parsing.pkg.AndroidPackage +import com.android.server.pm.test.verify.domain.DomainVerificationTestUtils.mockPackageSettings import com.android.server.pm.verify.domain.DomainVerificationManagerInternal import com.android.server.pm.verify.domain.DomainVerificationService import com.android.server.pm.verify.domain.proxy.DomainVerificationProxy @@ -256,8 +257,12 @@ class DomainVerificationSettingsMutationTest { mockThrowOnUnmocked { whenever(callingUid) { TEST_UID } whenever(callingUserId) { TEST_USER_ID } - whenever(getPackageSettingLocked(TEST_PKG)) { mockPkgSetting() } - whenever(getPackageLocked(TEST_PKG)) { mockPkg() } + mockPackageSettings { + when (it) { + TEST_PKG -> mockPkgSetting() + else -> null + } + } whenever(schedule(anyInt(), any())) whenever(scheduleWriteSettings()) diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationTestUtils.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationTestUtils.kt new file mode 100644 index 0000000000000..e1da727fb64ff --- /dev/null +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationTestUtils.kt @@ -0,0 +1,51 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.pm.test.verify.domain + +import com.android.internal.util.FunctionalUtils +import com.android.server.pm.PackageSetting +import com.android.server.pm.verify.domain.DomainVerificationManagerInternal +import com.android.server.testutils.whenever +import org.mockito.ArgumentMatchers.any +import java.util.function.Consumer +import java.util.function.Function + +internal object DomainVerificationTestUtils { + + @Suppress("UNCHECKED_CAST") + fun DomainVerificationManagerInternal.Connection.mockPackageSettings( + block: (String) -> PackageSetting? + ) { + whenever(withPackageSettings(any())) { + (arguments[0] as Consumer>).accept { block(it) } + } + whenever(withPackageSettingsReturning(any())) { + (arguments[0] as FunctionalUtils.ThrowingFunction, *>) + .apply { block(it) } + } + whenever(withPackageSettingsThrowing(any())) { + (arguments[0] as DomainVerificationManagerInternal.Connection.ThrowingConsumer< + Function, *>) + .accept { block(it) } + } + whenever(withPackageSettingsReturningThrowing(any())) { + (arguments[0] as DomainVerificationManagerInternal.Connection.ThrowingFunction< + Function, *, *>) + .apply { block(it) } + } + } +} diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt index 474e8012d43bd..52ae7a5356571 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationUserSelectionOverrideTest.kt @@ -30,6 +30,7 @@ import android.os.Process import android.util.ArraySet import com.android.server.pm.PackageSetting import com.android.server.pm.parsing.pkg.AndroidPackage +import com.android.server.pm.test.verify.domain.DomainVerificationTestUtils.mockPackageSettings import com.android.server.pm.verify.domain.DomainVerificationService import com.android.server.testutils.mockThrowOnUnmocked import com.android.server.testutils.whenever @@ -83,10 +84,13 @@ class DomainVerificationUserStateOverrideTest { // Need to provide an internal UID so some permission checks are ignored whenever(callingUid) { Process.ROOT_UID } whenever(callingUserId) { 0 } - whenever(getPackageSettingLocked(PKG_ONE)) { pkg1 } - whenever(getPackageSettingLocked(PKG_TWO)) { pkg2 } - whenever(getPackageLocked(PKG_ONE)) { pkg1.getPkg() } - whenever(getPackageLocked(PKG_TWO)) { pkg2.getPkg() } + mockPackageSettings { + when (it) { + PKG_ONE -> pkg1 + PKG_TWO -> pkg2 + else -> null + } + } }) addPackage(pkg1) addPackage(pkg2) From 091a4e9d05b507c954b1ba7718d1615feb599389 Mon Sep 17 00:00:00 2001 From: Winson Date: Tue, 23 Mar 2021 15:33:01 -0700 Subject: [PATCH 4/6] Add domain verification CTS to TEST_MAPPING Bug: 178424377 Test: atest CtsDomainVerificationDeviceTestCases Test: atest CtsDomainVerificationHostTestCases Change-Id: Ic8b93a8c5e9d0eb69262ec6bc7a196639d06b272 --- core/java/android/content/pm/verify/domain/TEST_MAPPING | 6 ++++++ .../java/com/android/server/pm/verify/domain/TEST_MAPPING | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/core/java/android/content/pm/verify/domain/TEST_MAPPING b/core/java/android/content/pm/verify/domain/TEST_MAPPING index c6c979107e75c..5fcf4118ddd1b 100644 --- a/core/java/android/content/pm/verify/domain/TEST_MAPPING +++ b/core/java/android/content/pm/verify/domain/TEST_MAPPING @@ -7,6 +7,12 @@ "include-filter": "com.android.server.pm.test.verify.domain" } ] + }, + { + "name": "CtsDomainVerificationDeviceTestCases" + }, + { + "name": "CtsDomainVerificationHostTestCases" } ] } diff --git a/services/core/java/com/android/server/pm/verify/domain/TEST_MAPPING b/services/core/java/com/android/server/pm/verify/domain/TEST_MAPPING index c6c979107e75c..5fcf4118ddd1b 100644 --- a/services/core/java/com/android/server/pm/verify/domain/TEST_MAPPING +++ b/services/core/java/com/android/server/pm/verify/domain/TEST_MAPPING @@ -7,6 +7,12 @@ "include-filter": "com.android.server.pm.test.verify.domain" } ] + }, + { + "name": "CtsDomainVerificationDeviceTestCases" + }, + { + "name": "CtsDomainVerificationHostTestCases" } ] } From d65f0926dc58707ab50c3c70a3171a8a5831c1ac Mon Sep 17 00:00:00 2001 From: Winson Date: Wed, 31 Mar 2021 12:35:51 -0700 Subject: [PATCH 5/6] Support serializing package signatures for domain verification state To prepare for backup/restore changes which need to verify that the package signatures match. Bug: 182207215 Test: atest DomainVerificationPersistenceTest Change-Id: I9e4bd24256604b649acf2c925cce2ac65331fa2c --- .../domain/DomainVerificationPersistence.java | 52 ++++++--- .../domain/DomainVerificationService.java | 26 ++++- .../domain/DomainVerificationSettings.java | 10 +- .../models/DomainVerificationPkgState.java | 59 ++++++++-- .../DomainVerificationPersistenceTest.kt | 110 ++++++++++++++---- 5 files changed, 206 insertions(+), 51 deletions(-) diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationPersistence.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationPersistence.java index abb8d2fb6e1e4..f0ad98c26ede3 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationPersistence.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationPersistence.java @@ -18,10 +18,12 @@ package com.android.server.pm.verify.domain; import android.annotation.NonNull; import android.annotation.Nullable; +import android.content.pm.Signature; import android.content.pm.verify.domain.DomainVerificationState; import android.text.TextUtils; import android.util.ArrayMap; import android.util.ArraySet; +import android.util.PackageUtils; import android.util.SparseArray; import android.util.TypedXmlPullParser; import android.util.TypedXmlSerializer; @@ -36,6 +38,7 @@ import org.xmlpull.v1.XmlPullParserException; import java.io.IOException; import java.util.Collection; import java.util.UUID; +import java.util.function.Function; public class DomainVerificationPersistence { @@ -49,6 +52,7 @@ public class DomainVerificationPersistence { private static final String ATTR_PACKAGE_NAME = "packageName"; private static final String ATTR_ID = "id"; private static final String ATTR_HAS_AUTO_VERIFY_DOMAINS = "hasAutoVerifyDomains"; + private static final String ATTR_SIGNATURE = "signature"; private static final String TAG_USER_STATES = "user-states"; public static final String TAG_USER_STATE = "user-state"; @@ -62,10 +66,18 @@ public class DomainVerificationPersistence { public static final String ATTR_NAME = "name"; public static final String ATTR_STATE = "state"; + /** + * @param pkgNameToSignature Converts package name to a string representation of its signature. + * Usually this is the SHA-256 hash from + * {@link PackageUtils#computeSignaturesSha256Digest(Signature[])}, + * but can be an arbitrary string for testing purposes. Pass non-null + * to write out signatures, or null to ignore. + */ public static void writeToXml(@NonNull TypedXmlSerializer xmlSerializer, @NonNull DomainVerificationStateMap attached, @NonNull ArrayMap pending, - @NonNull ArrayMap restored) throws IOException { + @NonNull ArrayMap restored, + @Nullable Function pkgNameToSignature) throws IOException { try (SettingsXml.Serializer serializer = SettingsXml.serializer(xmlSerializer)) { try (SettingsXml.WriteSection ignored = serializer.startSection( TAG_DOMAIN_VERIFICATIONS)) { @@ -86,25 +98,26 @@ public class DomainVerificationPersistence { } try (SettingsXml.WriteSection activeSection = serializer.startSection(TAG_ACTIVE)) { - writePackageStates(activeSection, active); + writePackageStates(activeSection, active, pkgNameToSignature); } try (SettingsXml.WriteSection restoredSection = serializer.startSection( TAG_RESTORED)) { - writePackageStates(restoredSection, restored.values()); + writePackageStates(restoredSection, restored.values(), pkgNameToSignature); } } } } private static void writePackageStates(@NonNull SettingsXml.WriteSection section, - @NonNull Collection states) throws IOException { + @NonNull Collection states, + @Nullable Function pkgNameToSignature) throws IOException { if (states.isEmpty()) { return; } for (DomainVerificationPkgState state : states) { - writePkgStateToXml(section, state); + writePkgStateToXml(section, state, pkgNameToSignature); } } @@ -146,11 +159,12 @@ public class DomainVerificationPersistence { * been entered. */ @Nullable - public static DomainVerificationPkgState createPkgStateFromXml( + private static DomainVerificationPkgState createPkgStateFromXml( @NonNull SettingsXml.ReadSection section) { String packageName = section.getString(ATTR_PACKAGE_NAME); String idString = section.getString(ATTR_ID); boolean hasAutoVerifyDomains = section.getBoolean(ATTR_HAS_AUTO_VERIFY_DOMAINS); + String signature = section.getString(ATTR_SIGNATURE); if (TextUtils.isEmpty(packageName) || TextUtils.isEmpty(idString)) { return null; } @@ -172,7 +186,7 @@ public class DomainVerificationPersistence { } return new DomainVerificationPkgState(packageName, id, hasAutoVerifyDomains, stateMap, - userStates); + userStates, signature); } private static void readUserStates(@NonNull SettingsXml.ReadSection section, @@ -196,14 +210,26 @@ public class DomainVerificationPersistence { } } - public static void writePkgStateToXml(@NonNull SettingsXml.WriteSection parentSection, - @NonNull DomainVerificationPkgState pkgState) throws IOException { + private static void writePkgStateToXml(@NonNull SettingsXml.WriteSection parentSection, + @NonNull DomainVerificationPkgState pkgState, + @Nullable Function pkgNameToSignature) throws IOException { + String packageName = pkgState.getPackageName(); + String signature = pkgNameToSignature == null + ? null : pkgNameToSignature.apply(packageName); + if (signature == null) { + // If a package isn't available to get its signature, fallback to the previously stored + // result, which can occur if the package has been marked for restore but hasn't + // been installed on the new device yet. + signature = pkgState.getBackupSignatureHash(); + } + try (SettingsXml.WriteSection ignored = parentSection.startSection(TAG_PACKAGE_STATE) - .attribute(ATTR_PACKAGE_NAME, pkgState.getPackageName()) + .attribute(ATTR_PACKAGE_NAME, packageName) .attribute(ATTR_ID, pkgState.getId().toString()) .attribute(ATTR_HAS_AUTO_VERIFY_DOMAINS, - pkgState.isHasAutoVerifyDomains())) { + pkgState.isHasAutoVerifyDomains()) + .attribute(ATTR_SIGNATURE, signature)) { writeStateMap(parentSection, pkgState.getStateMap()); writeUserStates(parentSection, pkgState.getUserStates()); } @@ -245,7 +271,7 @@ public class DomainVerificationPersistence { * entered. */ @Nullable - public static DomainVerificationInternalUserState createUserStateFromXml( + private static DomainVerificationInternalUserState createUserStateFromXml( @NonNull SettingsXml.ReadSection section) { int userId = section.getInt(ATTR_USER_ID); if (userId == -1) { @@ -274,7 +300,7 @@ public class DomainVerificationPersistence { } } - public static void writeUserStateToXml(@NonNull SettingsXml.WriteSection parentSection, + private static void writeUserStateToXml(@NonNull SettingsXml.WriteSection parentSection, @NonNull DomainVerificationInternalUserState userState) throws IOException { try (SettingsXml.WriteSection section = parentSection.startSection(TAG_USER_STATE) diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java index 81adbe197ee27..b0ee46d54f269 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java @@ -43,6 +43,7 @@ import android.os.UserHandle; import android.util.ArrayMap; import android.util.ArraySet; import android.util.IndentingPrintWriter; +import android.util.PackageUtils; import android.util.Pair; import android.util.Slog; import android.util.SparseArray; @@ -842,7 +843,8 @@ public class DomainVerificationService extends SystemService + ", new pkg = " + newPkg, new Exception()); DomainVerificationPkgState newPkgState = new DomainVerificationPkgState( - pkgName, newDomainSetId, true, newStateMap, newUserStates); + pkgName, newDomainSetId, true, newStateMap, newUserStates, + null /* signature */); mAttachedPkgStates.put(pkgName, newDomainSetId, newPkgState); return; } @@ -894,7 +896,8 @@ public class DomainVerificationService extends SystemService sendBroadcast = hasAutoVerifyDomains && needsBroadcast; mAttachedPkgStates.put(pkgName, newDomainSetId, new DomainVerificationPkgState( - pkgName, newDomainSetId, hasAutoVerifyDomains, newStateMap, newUserStates)); + pkgName, newDomainSetId, hasAutoVerifyDomains, newStateMap, newUserStates, + null /* signature */)); } if (sendBroadcast) { @@ -1019,9 +1022,22 @@ public class DomainVerificationService extends SystemService @Override public void writeSettings(@NonNull TypedXmlSerializer serializer) throws IOException { - synchronized (mLock) { - mSettings.writeSettings(serializer, mAttachedPkgStates); - } + mConnection.withPackageSettingsThrowing(pkgSettings -> { + synchronized (mLock) { + mSettings.writeSettings(serializer, mAttachedPkgStates, pkgName -> { + PackageSetting pkgSetting = pkgSettings.apply(pkgName); + if (pkgSetting == null) { + // If querying for a user restored package that isn't installed on the + // device yet, there will be no signature to write out. In that case, + // it's expected that this returns null and it falls back to the restored + // state's stored signature if it exists. + return null; + } + + return PackageUtils.computeSignaturesSha256Digest(pkgSetting.getSignatures()); + }); + } + }); mLegacySettings.writeSettings(serializer); } diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationSettings.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationSettings.java index d3cb0b33f45bf..44ffc5c146c57 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationSettings.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationSettings.java @@ -36,6 +36,7 @@ import com.android.server.pm.verify.domain.models.DomainVerificationStateMap; import org.xmlpull.v1.XmlPullParserException; import java.io.IOException; +import java.util.function.Function; class DomainVerificationSettings { @@ -68,18 +69,19 @@ class DomainVerificationSettings { public void writeSettings(@NonNull TypedXmlSerializer xmlSerializer, - @NonNull DomainVerificationStateMap liveState) - throws IOException { + @NonNull DomainVerificationStateMap liveState, + @NonNull Function pkgSignatureFunction) throws IOException { synchronized (mLock) { DomainVerificationPersistence.writeToXml(xmlSerializer, liveState, - mPendingPkgStates, mRestoredPkgStates); + mPendingPkgStates, mRestoredPkgStates, pkgSignatureFunction); } } /** * Parses a previously stored set of states and merges them with {@param liveState}, directly * mutating the values. This is intended for reading settings written by {@link - * #writeSettings(TypedXmlSerializer, DomainVerificationStateMap)} on the same device setup. + * #writeSettings(TypedXmlSerializer, DomainVerificationStateMap, Function)} on the same device + * setup. */ public void readSettings(@NonNull TypedXmlPullParser parser, @NonNull DomainVerificationStateMap liveState) diff --git a/services/core/java/com/android/server/pm/verify/domain/models/DomainVerificationPkgState.java b/services/core/java/com/android/server/pm/verify/domain/models/DomainVerificationPkgState.java index 40c70915f3cea..6c56360177d84 100644 --- a/services/core/java/com/android/server/pm/verify/domain/models/DomainVerificationPkgState.java +++ b/services/core/java/com/android/server/pm/verify/domain/models/DomainVerificationPkgState.java @@ -19,6 +19,7 @@ package com.android.server.pm.verify.domain.models; import android.annotation.NonNull; import android.annotation.Nullable; import android.annotation.UserIdInt; +import android.content.pm.Signature; import android.content.pm.verify.domain.DomainVerificationState; import android.util.ArrayMap; import android.util.SparseArray; @@ -63,15 +64,28 @@ public class DomainVerificationPkgState { @NonNull private final SparseArray mUserStates; + /** + * If previously recorded, the SHA-256 signing cert digest of the package to attach to. + * When doing restoration of a previously backed up state, if the signature does not + * match the package being scanned/installed on device, it will be rejected. + * + * It's assumed the domain verification agent will eventually re-verify this domain + * and revoke if necessary. + * + * @see android.util.PackageUtils#computeSignaturesSha256Digest(Signature[]) + */ + @Nullable + private final String mBackupSignatureHash; + public DomainVerificationPkgState(@NonNull String packageName, @NonNull UUID id, boolean hasAutoVerifyDomains) { - this(packageName, id, hasAutoVerifyDomains, new ArrayMap<>(0), new SparseArray<>(0)); + this(packageName, id, hasAutoVerifyDomains, new ArrayMap<>(0), new SparseArray<>(0), null); } public DomainVerificationPkgState(@NonNull DomainVerificationPkgState pkgState, @NonNull UUID id, boolean hasAutoVerifyDomains) { this(pkgState.getPackageName(), id, hasAutoVerifyDomains, pkgState.getStateMap(), - pkgState.getUserStates()); + pkgState.getUserStates(), null); } @Nullable @@ -109,7 +123,7 @@ public class DomainVerificationPkgState { - // Code below generated by codegen v1.0.22. + // Code below generated by codegen v1.0.23. // // DO NOT MODIFY! // CHECKSTYLE:OFF Generated code @@ -137,6 +151,13 @@ public class DomainVerificationPkgState { * * TODO(b/159952358): Hide the state map entirely from the caller, to allow optimizations, * such as storing no state when the package is marked as a linked app in SystemConfig. + * @param backupSignatureHash + * If previously recorded, the SHA-256 signing cert digest of the package to attach to. + * When doing restoration of a previously backed up state, if the signature does not + * match the package being scanned/installed on device, it will be rejected. + * + * It's assumed the domain verification agent will eventually re-verify this domain + * and revoke if necessary. */ @DataClass.Generated.Member public DomainVerificationPkgState( @@ -144,7 +165,8 @@ public class DomainVerificationPkgState { @NonNull UUID id, boolean hasAutoVerifyDomains, @NonNull ArrayMap stateMap, - @NonNull SparseArray userStates) { + @NonNull SparseArray userStates, + @Nullable String backupSignatureHash) { this.mPackageName = packageName; com.android.internal.util.AnnotationValidations.validate( NonNull.class, null, mPackageName); @@ -158,6 +180,7 @@ public class DomainVerificationPkgState { this.mUserStates = userStates; com.android.internal.util.AnnotationValidations.validate( NonNull.class, null, mUserStates); + this.mBackupSignatureHash = backupSignatureHash; // onConstructed(); // You can define this method to get a callback } @@ -201,6 +224,21 @@ public class DomainVerificationPkgState { return mUserStates; } + /** + * If previously recorded, the SHA-256 signing cert digest of the package to attach to. + * When doing restoration of a previously backed up state, if the signature does not + * match the package being scanned/installed on device, it will be rejected. + * + * It's assumed the domain verification agent will eventually re-verify this domain + * and revoke if necessary. + * + * @see android.util.PackageUtils#computeSignaturesSha256Digest(Signature[]) + */ + @DataClass.Generated.Member + public @Nullable String getBackupSignatureHash() { + return mBackupSignatureHash; + } + @Override @DataClass.Generated.Member public String toString() { @@ -212,7 +250,8 @@ public class DomainVerificationPkgState { "id = " + mId + ", " + "hasAutoVerifyDomains = " + mHasAutoVerifyDomains + ", " + "stateMap = " + mStateMap + ", " + - "userStates = " + mUserStates + + "userStates = " + mUserStates + ", " + + "backupSignatureHash = " + mBackupSignatureHash + " }"; } @@ -233,7 +272,8 @@ public class DomainVerificationPkgState { && Objects.equals(mId, that.mId) && mHasAutoVerifyDomains == that.mHasAutoVerifyDomains && Objects.equals(mStateMap, that.mStateMap) - && userStatesEquals(that.mUserStates); + && userStatesEquals(that.mUserStates) + && Objects.equals(mBackupSignatureHash, that.mBackupSignatureHash); } @Override @@ -248,14 +288,15 @@ public class DomainVerificationPkgState { _hash = 31 * _hash + Boolean.hashCode(mHasAutoVerifyDomains); _hash = 31 * _hash + Objects.hashCode(mStateMap); _hash = 31 * _hash + userStatesHashCode(); + _hash = 31 * _hash + Objects.hashCode(mBackupSignatureHash); return _hash; } @DataClass.Generated( - time = 1614818362549L, - codegenVersion = "1.0.22", + time = 1617315369614L, + codegenVersion = "1.0.23", sourceFile = "frameworks/base/services/core/java/com/android/server/pm/verify/domain/models/DomainVerificationPkgState.java", - inputSignatures = "private final @android.annotation.NonNull java.lang.String mPackageName\nprivate @android.annotation.NonNull java.util.UUID mId\nprivate final boolean mHasAutoVerifyDomains\nprivate final @android.annotation.NonNull android.util.ArrayMap mStateMap\nprivate final @android.annotation.NonNull android.util.SparseArray mUserStates\npublic @android.annotation.Nullable com.android.server.pm.verify.domain.models.DomainVerificationInternalUserState getUserState(int)\npublic @android.annotation.Nullable com.android.server.pm.verify.domain.models.DomainVerificationInternalUserState getOrCreateUserState(int)\npublic void setId(java.util.UUID)\npublic void removeUser(int)\npublic void removeAllUsers()\nprivate int userStatesHashCode()\nprivate boolean userStatesEquals(android.util.SparseArray)\nclass DomainVerificationPkgState extends java.lang.Object implements []\n@com.android.internal.util.DataClass(genToString=true, genEqualsHashCode=true)") + inputSignatures = "private final @android.annotation.NonNull java.lang.String mPackageName\nprivate @android.annotation.NonNull java.util.UUID mId\nprivate final boolean mHasAutoVerifyDomains\nprivate final @android.annotation.NonNull android.util.ArrayMap mStateMap\nprivate final @android.annotation.NonNull android.util.SparseArray mUserStates\nprivate final @android.annotation.Nullable java.lang.String mBackupSignatureHash\npublic @android.annotation.Nullable com.android.server.pm.verify.domain.models.DomainVerificationInternalUserState getUserState(int)\npublic @android.annotation.Nullable com.android.server.pm.verify.domain.models.DomainVerificationInternalUserState getOrCreateUserState(int)\npublic void removeUser(int)\npublic void removeAllUsers()\nprivate int userStatesHashCode()\nprivate boolean userStatesEquals(android.util.SparseArray)\nclass DomainVerificationPkgState extends java.lang.Object implements []\n@com.android.internal.util.DataClass(genToString=true, genEqualsHashCode=true)") @Deprecated private void __metadata() {} diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPersistenceTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPersistenceTest.kt index f8fda12f806d8..7ffbbf6af159e 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPersistenceTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPersistenceTest.kt @@ -18,6 +18,7 @@ package com.android.server.pm.test.verify.domain import android.content.pm.verify.domain.DomainVerificationState import android.util.ArrayMap +import android.util.SparseArray import android.util.TypedXmlPullParser import android.util.TypedXmlSerializer import android.util.Xml @@ -84,23 +85,32 @@ class DomainVerificationPersistenceTest { @JvmField val tempFolder = TemporaryFolder() - @Test - fun writeAndReadBackNormal() { + private fun mockWriteValues( + pkgNameToSignature: (String) -> String? = { null } + ): Triple, + ArrayMap, + ArrayMap> { val attached = DomainVerificationStateMap().apply { - mockPkgState(0).let { put(it.packageName, it.id, it) } - mockPkgState(1).let { put(it.packageName, it.id, it) } + mockPkgState(0, pkgNameToSignature).let { put(it.packageName, it.id, it) } + mockPkgState(1, pkgNameToSignature).let { put(it.packageName, it.id, it) } } val pending = ArrayMap().apply { - mockPkgState(2).let { put(it.packageName, it) } - mockPkgState(3).let { put(it.packageName, it) } + mockPkgState(2, pkgNameToSignature).let { put(it.packageName, it) } + mockPkgState(3, pkgNameToSignature).let { put(it.packageName, it) } } val restored = ArrayMap().apply { - mockPkgState(4).let { put(it.packageName, it) } - mockPkgState(5).let { put(it.packageName, it) } + mockPkgState(4, pkgNameToSignature).let { put(it.packageName, it) } + mockPkgState(5, pkgNameToSignature).let { put(it.packageName, it) } } + return Triple(attached, pending, restored) + } + + @Test + fun writeAndReadBackNormal() { + val (attached, pending, restored) = mockWriteValues() val file = tempFolder.newFile().writeXml { - DomainVerificationPersistence.writeToXml(it, attached, pending, restored) + DomainVerificationPersistence.writeToXml(it, attached, pending, restored, null) } val xml = file.readText() @@ -114,9 +124,57 @@ class DomainVerificationPersistenceTest { assertWithMessage(xml).that(readRestored.values).containsExactlyElementsIn(restored.values) } + @Test + fun writeAndReadBackWithSignature() { + val (attached, pending, restored) = mockWriteValues() + val file = tempFolder.newFile().writeXml { + DomainVerificationPersistence.writeToXml(it, attached, pending, restored) { + "SIGNATURE_$it" + } + } + + val (readActive, readRestored) = file.readXml { + DomainVerificationPersistence.readFromXml(it) + } + + // Assign the signatures to a fresh set of data structures, to ensure the previous write + // call did not use the signatures from the data structure. This is because the method is + // intended to optionally append signatures, regardless of if the existing data structures + // contain them or not. + val (attached2, pending2, restored2) = mockWriteValues { "SIGNATURE_$it" } + + assertThat(readActive.values) + .containsExactlyElementsIn(attached2.values() + pending2.values) + assertThat(readRestored.values).containsExactlyElementsIn(restored2.values) + + (readActive + readRestored).forEach { (_, value) -> + assertThat(value.backupSignatureHash).isEqualTo("SIGNATURE_${value.packageName}") + } + } + + @Test + fun writeStateSignatureIfFunctionReturnsNull() { + val (attached, pending, restored) = mockWriteValues { "SIGNATURE_$it" } + val file = tempFolder.newFile().writeXml { + DomainVerificationPersistence.writeToXml(it, attached, pending, restored) { null } + } + + val (readActive, readRestored) = file.readXml { + DomainVerificationPersistence.readFromXml(it) + } + + assertThat(readActive.values) + .containsExactlyElementsIn(attached.values() + pending.values) + assertThat(readRestored.values).containsExactlyElementsIn(restored.values) + + (readActive + readRestored).forEach { (_, value) -> + assertThat(value.backupSignatureHash).isEqualTo("SIGNATURE_${value.packageName}") + } + } + @Test fun readMalformed() { - val stateZero = mockEmptyPkgState(0).apply { + val stateZero = mockEmptyPkgState(0, pkgNameToSignature = { "ACTIVE" }).apply { stateMap["example.com"] = DomainVerificationState.STATE_SUCCESS stateMap["example.org"] = DomainVerificationState.STATE_FIRST_VERIFIER_DEFINED @@ -128,7 +186,7 @@ class DomainVerificationPersistenceTest { isLinkHandlingAllowed = true } } - val stateOne = mockEmptyPkgState(1).apply { + val stateOne = mockEmptyPkgState(1, pkgNameToSignature = { "RESTORED" }).apply { // It's valid to have a user selection without any autoVerify domains userStates[1] = DomainVerificationInternalUserState(1).apply { addHosts(setOf("example-user1.com", "example-user1.org")) @@ -156,6 +214,7 @@ class DomainVerificationPersistenceTest { packageName="${stateZero.packageName}" id="${stateZero.id}" hasAutoVerifyDomains="true" + signature="ACTIVE" > @@ -226,20 +286,30 @@ class DomainVerificationPersistenceTest { private fun mockEmptyPkgState( id: Int, - hasAutoVerifyDomains: Boolean = true + hasAutoVerifyDomains: Boolean = true, + pkgNameToSignature: (String) -> String? = { null } ): DomainVerificationPkgState { val pkgName = pkgName(id) val domainSetId = UUID(0L, id.toLong()) - return DomainVerificationPkgState(pkgName, domainSetId, hasAutoVerifyDomains) + return DomainVerificationPkgState( + pkgName, + domainSetId, + hasAutoVerifyDomains, + ArrayMap(), + SparseArray(), + pkgNameToSignature(pkgName) + ) } - private fun mockPkgState(id: Int) = mockEmptyPkgState(id).apply { - stateMap["$packageName.com"] = id - userStates[id] = DomainVerificationInternalUserState(id).apply { - addHosts(setOf("$packageName-user.com")) - isLinkHandlingAllowed = true - } - } + private fun mockPkgState(id: Int, pkgNameToSignature: (String) -> String? = { null }) = + mockEmptyPkgState(id, pkgNameToSignature = pkgNameToSignature) + .apply { + stateMap["$packageName.com"] = id + userStates[id] = DomainVerificationInternalUserState(id).apply { + addHosts(setOf("$packageName-user.com")) + isLinkHandlingAllowed = true + } + } private fun pkgName(id: Int) = "$PKG_PREFIX.pkg$id" } From 3482338bc1870a6f612dcc1d25c3a82791168daa Mon Sep 17 00:00:00 2001 From: Winson Date: Wed, 31 Mar 2021 14:04:26 -0700 Subject: [PATCH 6/6] Fix domain verify restore and add signature check Verifies that all signatures of the package being restored matches by SHA256 hash. And fixes a bug where the restored packages weren't being added to the internal structures. Bug: 170746586 Test: atest DomainVerificationPackageTest Change-Id: I2cf2e739a88396475599bc4c1ff27c36e09a8d45 --- .../domain/DomainVerificationService.java | 5 + .../domain/DomainVerificationSettings.java | 8 +- .../domain/DomainVerificationPackageTest.kt | 141 +++++++++++++++--- 3 files changed, 134 insertions(+), 20 deletions(-) diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java index b0ee46d54f269..4ae79a209524f 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java @@ -925,6 +925,11 @@ public class DomainVerificationService extends SystemService sendBroadcast = false; } else { pkgState = mSettings.removeRestoredState(pkgName); + if (pkgState != null && !Objects.equals(pkgState.getBackupSignatureHash(), + PackageUtils.computeSignaturesSha256Digest(newPkgSetting.getSignatures()))) { + // If restoring and the signatures don't match, drop the state + pkgState = null; + } } AndroidPackage pkg = newPkgSetting.getPkg(); diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationSettings.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationSettings.java index 44ffc5c146c57..c8e95b585b7a4 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationSettings.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationSettings.java @@ -157,16 +157,20 @@ class DomainVerificationSettings { // mergePkgState runs, without the merge part. ArrayMap stateMap = newState.getStateMap(); int size = stateMap.size(); - for (int index = 0; index < size; index++) { + for (int index = size - 1; index >= 0; index--) { Integer stateInteger = stateMap.valueAt(index); if (stateInteger != null) { int state = stateInteger; if (state == DomainVerificationState.STATE_SUCCESS || state == DomainVerificationState.STATE_RESTORED) { - stateMap.setValueAt(index, state); + stateMap.setValueAt(index, DomainVerificationState.STATE_RESTORED); + } else { + stateMap.removeAt(index); } } } + + mRestoredPkgStates.put(pkgName, newState); } } } diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt index 8a549731af3a2..1097c45f8a9ff 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt @@ -19,8 +19,10 @@ package com.android.server.pm.test.verify.domain import android.content.Intent import android.content.pm.PackageManager import android.content.pm.PackageUserState +import android.content.pm.Signature import android.content.pm.parsing.component.ParsedActivity import android.content.pm.parsing.component.ParsedIntentInfo +import android.content.pm.verify.domain.DomainVerificationInfo.STATE_MODIFIABLE_VERIFIED import android.content.pm.verify.domain.DomainVerificationInfo.STATE_NO_RESPONSE import android.content.pm.verify.domain.DomainVerificationInfo.STATE_SUCCESS import android.content.pm.verify.domain.DomainVerificationInfo.STATE_UNMODIFIABLE @@ -42,7 +44,6 @@ import com.android.server.testutils.mockThrowOnUnmocked import com.android.server.testutils.whenever import com.google.common.truth.Truth.assertThat import org.junit.Test -import org.mockito.ArgumentMatchers import org.mockito.ArgumentMatchers.any import org.mockito.ArgumentMatchers.anyInt import org.mockito.ArgumentMatchers.anyLong @@ -56,6 +57,10 @@ class DomainVerificationPackageTest { private const val PKG_TWO = "com.test.two" private val UUID_ONE = UUID.fromString("1b041c96-8d37-4932-a858-561bfac5947c") private val UUID_TWO = UUID.fromString("a3389c16-7f9f-4e86-85e3-500d1249c74c") + private const val SIGNATURE_ONE = "AA" + private const val DIGEST_ONE = + "BCEEF655B5A034911F1C3718CE056531B45EF03B4C7B1F15629E867294011A7D" + private const val SIGNATURE_TWO = "BB" private val DOMAIN_BASE = DomainVerificationPackageTest::class.java.packageName private val DOMAIN_1 = "one.$DOMAIN_BASE" @@ -66,8 +71,8 @@ class DomainVerificationPackageTest { private const val USER_ID = 0 } - private val pkg1 = mockPkgSetting(PKG_ONE, UUID_ONE) - private val pkg2 = mockPkgSetting(PKG_TWO, UUID_TWO) + private val pkg1 = mockPkgSetting(PKG_ONE, UUID_ONE, SIGNATURE_ONE) + private val pkg2 = mockPkgSetting(PKG_TWO, UUID_TWO, SIGNATURE_TWO) @Test fun addPackageFirstTime() { @@ -95,6 +100,104 @@ class DomainVerificationPackageTest { .containsExactly(pkg1.getName()) } + @Test + fun addPackageRestoredMatchingSignature() { + // language=XML + val xml = """ + + + + + + + + + + + """ + + val service = makeService(pkg1, pkg2) + service.restoreSettings(Xml.resolvePullParser(xml.byteInputStream())) + service.addPackage(pkg1) + val info = service.getInfo(pkg1.getName()) + assertThat(info.packageName).isEqualTo(pkg1.getName()) + assertThat(info.identifier).isEqualTo(pkg1.domainSetId) + assertThat(info.hostToStateMap).containsExactlyEntriesIn( + mapOf( + DOMAIN_1 to STATE_MODIFIABLE_VERIFIED, + DOMAIN_2 to STATE_NO_RESPONSE, + ) + ) + + val userState = service.getUserState(pkg1.getName()) + assertThat(userState.packageName).isEqualTo(pkg1.getName()) + assertThat(userState.identifier).isEqualTo(pkg1.domainSetId) + assertThat(userState.isLinkHandlingAllowed).isEqualTo(true) + assertThat(userState.user.identifier).isEqualTo(USER_ID) + assertThat(userState.hostToStateMap).containsExactlyEntriesIn( + mapOf( + DOMAIN_1 to DOMAIN_STATE_VERIFIED, + DOMAIN_2 to DOMAIN_STATE_NONE, + ) + ) + + assertThat(service.queryValidVerificationPackageNames()) + .containsExactly(pkg1.getName()) + } + + @Test + fun addPackageRestoredMismatchSignature() { + // language=XML + val xml = """ + + + + + + + + + + + """ + + val service = makeService(pkg1, pkg2) + service.restoreSettings(Xml.resolvePullParser(xml.byteInputStream())) + service.addPackage(pkg1) + val info = service.getInfo(pkg1.getName()) + assertThat(info.packageName).isEqualTo(pkg1.getName()) + assertThat(info.identifier).isEqualTo(pkg1.domainSetId) + assertThat(info.hostToStateMap).containsExactlyEntriesIn( + mapOf( + DOMAIN_1 to STATE_NO_RESPONSE, + DOMAIN_2 to STATE_NO_RESPONSE, + ) + ) + + val userState = service.getUserState(pkg1.getName()) + assertThat(userState.packageName).isEqualTo(pkg1.getName()) + assertThat(userState.identifier).isEqualTo(pkg1.domainSetId) + assertThat(userState.isLinkHandlingAllowed).isEqualTo(true) + assertThat(userState.user.identifier).isEqualTo(USER_ID) + assertThat(userState.hostToStateMap).containsExactlyEntriesIn( + mapOf( + DOMAIN_1 to DOMAIN_STATE_NONE, + DOMAIN_2 to DOMAIN_STATE_NONE, + ) + ) + + assertThat(service.queryValidVerificationPackageNames()) + .containsExactly(pkg1.getName()) + } + @Test fun addPackageActive() { // language=XML @@ -153,10 +256,9 @@ class DomainVerificationPackageTest { @Test fun migratePackageDropDomain() { val pkgName = PKG_ONE - val pkgBefore = mockPkgSetting(pkgName, UUID_ONE, - listOf(DOMAIN_1, DOMAIN_2, DOMAIN_3, DOMAIN_4)) - val pkgAfter = mockPkgSetting(pkgName, UUID_TWO, - listOf(DOMAIN_1, DOMAIN_2)) + val pkgBefore = mockPkgSetting(pkgName, UUID_ONE, SIGNATURE_ONE, + listOf(DOMAIN_1, DOMAIN_2, DOMAIN_3, DOMAIN_4)) + val pkgAfter = mockPkgSetting(pkgName, UUID_TWO, SIGNATURE_TWO, listOf(DOMAIN_1, DOMAIN_2)) // Test 4 domains: // 1 will be approved and preserved, 2 will be selected and preserved, @@ -215,8 +317,8 @@ class DomainVerificationPackageTest { @Test fun migratePackageDropAll() { val pkgName = PKG_ONE - val pkgBefore = mockPkgSetting(pkgName, UUID_ONE, listOf(DOMAIN_1, DOMAIN_2)) - val pkgAfter = mockPkgSetting(pkgName, UUID_TWO, emptyList()) + val pkgBefore = mockPkgSetting(pkgName, UUID_ONE, SIGNATURE_ONE, listOf(DOMAIN_1, DOMAIN_2)) + val pkgAfter = mockPkgSetting(pkgName, UUID_TWO, SIGNATURE_TWO, emptyList()) val map = mutableMapOf() val service = makeService { map[it] } @@ -257,10 +359,9 @@ class DomainVerificationPackageTest { @Test fun migratePackageAddDomain() { val pkgName = PKG_ONE - val pkgBefore = mockPkgSetting(pkgName, UUID_ONE, - listOf(DOMAIN_1, DOMAIN_2)) - val pkgAfter = mockPkgSetting(pkgName, UUID_TWO, - listOf(DOMAIN_1, DOMAIN_2, DOMAIN_3)) + val pkgBefore = mockPkgSetting(pkgName, UUID_ONE, SIGNATURE_ONE, listOf(DOMAIN_1, DOMAIN_2)) + val pkgAfter = mockPkgSetting(pkgName, UUID_TWO, SIGNATURE_TWO, + listOf(DOMAIN_1, DOMAIN_2, DOMAIN_3)) // Test 3 domains: // 1 will be verified and preserved, 2 will be selected and preserved, @@ -309,8 +410,8 @@ class DomainVerificationPackageTest { @Test fun migratePackageAddAll() { val pkgName = PKG_ONE - val pkgBefore = mockPkgSetting(pkgName, UUID_ONE, emptyList()) - val pkgAfter = mockPkgSetting(pkgName, UUID_TWO, listOf(DOMAIN_1, DOMAIN_2)) + val pkgBefore = mockPkgSetting(pkgName, UUID_ONE, SIGNATURE_ONE, emptyList()) + val pkgAfter = mockPkgSetting(pkgName, UUID_TWO, SIGNATURE_TWO, listOf(DOMAIN_1, DOMAIN_2)) val map = mutableMapOf() val service = makeService { map[it] } @@ -387,9 +488,12 @@ class DomainVerificationPackageTest { }) } - private fun mockPkgSetting(pkgName: String, domainSetId: UUID, domains: List = listOf( - DOMAIN_1, DOMAIN_2 - )) = mockThrowOnUnmocked { + private fun mockPkgSetting( + pkgName: String, + domainSetId: UUID, + signature: String, + domains: List = listOf(DOMAIN_1, DOMAIN_2) + ) = mockThrowOnUnmocked { val pkg = mockThrowOnUnmocked { whenever(packageName) { pkgName } whenever(targetSdkVersion) { Build.VERSION_CODES.S } @@ -423,5 +527,6 @@ class DomainVerificationPackageTest { whenever(getInstantApp(anyInt())) { false } whenever(firstInstallTime) { 0L } whenever(readUserState(USER_ID)) { PackageUserState() } + whenever(signatures) { arrayOf(Signature(signature)) } } }