From 5697a4c37eb7e49d3c2d7292f3486b6407f21a44 Mon Sep 17 00:00:00 2001 From: Winson Chiu Date: Fri, 3 Jun 2022 22:49:17 +0000 Subject: [PATCH] Fix app links verification resetting user state In cases where domain verification comes in and does not take over verification from another app (the owning app has link handling disabled or the domain was already previous verified), the user selection for that domain should not be reset. This preserves user approved apps for links which the user has disabled the official verified app when that verified app update, as this event kicks off a verification through StatementService, which may have caused the reset with the buggy code. Bug: 221059351 Test: atest DomainVerificationPackageTest Test: atest DomainVerificationIntentStandaloneTests Test: manual, reset verification using shell set-app-links Test: manual, reset verification using verified app update Change-Id: Ia8aa70be485b2f94920fd97275fd16178895ef33 --- .../domain/DomainVerificationService.java | 72 +++++--- .../domain/DomainVerificationPackageTest.kt | 169 ++++++++++++++++++ 2 files changed, 220 insertions(+), 21 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 67aed4506b274..3cd7795005a77 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 @@ -317,7 +317,7 @@ public class DomainVerificationService extends SystemService mEnforcer.assertApprovedVerifier(callingUid, mProxy); final Computer snapshot = mConnection.snapshot(); synchronized (mLock) { - List verifiedDomains = new ArrayList<>(); + List newlyVerifiedDomains = new ArrayList<>(); GetAttachedResult result = getAndValidateAttachedLocked(domainSetId, domains, true /* forAutoVerify */, callingUid, null /* userId */, snapshot); @@ -329,21 +329,28 @@ public class DomainVerificationService extends SystemService ArrayMap stateMap = pkgState.getStateMap(); for (String domain : domains) { Integer previousState = stateMap.get(domain); - if (previousState != null - && !DomainVerificationState.isModifiable(previousState)) { - continue; + // Skip if the state hasn't changed or can't be changed + if (previousState != null) { + if (previousState == state + || !DomainVerificationState.isModifiable(previousState)) { + continue; + } } if (DomainVerificationState.isVerified(state)) { - verifiedDomains.add(domain); + if (previousState == null + || !DomainVerificationState.isVerified(previousState)) { + newlyVerifiedDomains.add(domain); + } } stateMap.put(domain, state); } - int size = verifiedDomains.size(); + // For newly verified domains, revoke their user states across other packages + int size = newlyVerifiedDomains.size(); for (int index = 0; index < size; index++) { - removeUserStatesForDomain(verifiedDomains.get(index)); + removeUserStatesForDomain(pkgState, newlyVerifiedDomains.get(index)); } } @@ -367,7 +374,6 @@ public class DomainVerificationService extends SystemService "State must be one of NO_RESPONSE, SUCCESS, APPROVED, or DENIED"); } - ArraySet verifiedDomains = new ArraySet<>(); if (packageName == null) { final Computer snapshot = mConnection.snapshot(); synchronized (mLock) { @@ -395,10 +401,6 @@ public class DomainVerificationService extends SystemService validDomains.retainAll(autoVerifyDomains); } - if (DomainVerificationState.isVerified(state)) { - verifiedDomains.addAll(validDomains); - } - setDomainVerificationStatusInternal(pkgState, state, validDomains); } } @@ -424,19 +426,33 @@ public class DomainVerificationService extends SystemService validDomains.retainAll(mCollector.collectValidAutoVerifyDomains(pkg)); } + ArraySet newlyVerifiedDomains = null; if (DomainVerificationState.isVerified(state)) { - verifiedDomains.addAll(validDomains); + newlyVerifiedDomains = new ArraySet<>(); + ArrayMap stateMap = pkgState.getStateMap(); + int domainsSize = validDomains.size(); + for (int domainIndex = 0; domainIndex < domainsSize; domainIndex++) { + String domain = validDomains.valueAt(domainIndex); + Integer oldState = stateMap.get(domain); + // Only remove if not previously verified + if (oldState == null || !DomainVerificationState.isVerified(oldState)) { + newlyVerifiedDomains.add(domain); + } + } } setDomainVerificationStatusInternal(pkgState, state, validDomains); - } - } - // 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)); + // Mirror SystemApi behavior of revoking user selection for approved domains. + // This is done in a second pass so that the previous state can be compared before + // the previous method overwrites it with the new state. + if (newlyVerifiedDomains != null) { + int domainsSize = newlyVerifiedDomains.size(); + for (int domainIndex = 0; domainIndex < domainsSize; domainIndex++) { + String domain = newlyVerifiedDomains.valueAt(domainIndex); + removeUserStatesForDomain(pkgState, domain); + } + } } } @@ -452,7 +468,10 @@ public class DomainVerificationService extends SystemService } } - private void removeUserStatesForDomain(@NonNull String domain) { + private void removeUserStatesForDomain(@NonNull DomainVerificationPkgState owningPkgState, + @NonNull String domain) { + SparseArray owningUserStates = + owningPkgState.getUserStates(); synchronized (mLock) { final int size = mAttachedPkgStates.size(); for (int index = 0; index < size; index++) { @@ -460,6 +479,15 @@ public class DomainVerificationService extends SystemService SparseArray array = pkgState.getUserStates(); int arraySize = array.size(); for (int arrayIndex = 0; arrayIndex < arraySize; arrayIndex++) { + int userId = array.keyAt(arrayIndex); + DomainVerificationInternalUserState owningUserState = + owningUserStates.get(userId); + if (owningUserState != null && !owningUserState.isLinkHandlingAllowed()) { + // Skip users where the owning package has their link handling disabled, + // since revoking those users would lead to no apps being able to handle + // the domain. + continue; + } array.valueAt(arrayIndex).removeHost(domain); } } @@ -527,6 +555,8 @@ public class DomainVerificationService extends SystemService mConnection.scheduleWriteSettings(); } + @CheckResult + @DomainVerificationManager.Error public int setDomainVerificationUserSelection(@NonNull UUID domainSetId, @NonNull Set domains, boolean enabled, @UserIdInt int userId) throws NameNotFoundException { 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 40f37a7ee1f7c..ed60c5064eb1b 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 @@ -78,6 +78,8 @@ class DomainVerificationPackageTest { private val DOMAIN_4 = "four.$DOMAIN_BASE" private const val USER_ID = 0 + private const val USER_ID_SECONDARY = 10 + private val USER_IDS = listOf(USER_ID, USER_ID_SECONDARY) } private val pkg1 = mockPkgState(PKG_ONE, UUID_ONE, SIGNATURE_ONE) @@ -813,6 +815,173 @@ class DomainVerificationPackageTest { assertExpectedState(serviceAfter) } + @Test + fun verifiedUnapproved_unverifiedSelected_approvalCausesUnselect_systemApi() { + verifiedUnapproved_unverifiedSelected_approvalCausesUnselect { + setDomainVerificationStatus(it.domainSetId, setOf(DOMAIN_1, DOMAIN_2), STATE_SUCCESS) + } + } + + @Test + fun verifiedUnapproved_unverifiedSelected_approvalCausesUnselect_internalApi() { + verifiedUnapproved_unverifiedSelected_approvalCausesUnselect { + setDomainVerificationStatusInternal(it.packageName, STATE_SUCCESS, + ArraySet(setOf(DOMAIN_1, DOMAIN_2))) + } + } + + private fun verifiedUnapproved_unverifiedSelected_approvalCausesUnselect( + setStatusBlock: DomainVerificationService.(PackageStateInternal) -> Unit + ) { + /* + Domains tested: + 1: Becomes verified in package 1, but package 1 disabled in secondary user, only + disables selection for package 2 in main user + 2: Becomes verified in package 1, unselected by package 2, remains unselected + 3: Is autoVerify, but unverified, selected by package 2, remains selected + 4: Non-autoVerify, selected by package 2, remains selected + */ + + val pkg1 = mockPkgState( + PKG_ONE, + UUID_ONE, + SIGNATURE_ONE, + autoVerifyDomains = listOf(DOMAIN_1, DOMAIN_2, DOMAIN_3), + otherDomains = listOf(DOMAIN_4) + ) + val pkg2 = mockPkgState( + PKG_TWO, + UUID_TWO, + SIGNATURE_TWO, + autoVerifyDomains = emptyList(), + otherDomains = listOf(DOMAIN_1, DOMAIN_2, DOMAIN_3, DOMAIN_4) + ) + + val service = makeService(pkg1, pkg2) + service.addPackage(pkg1) + service.addPackage(pkg2) + + // Approve domain 1, 3, and 4 for package 2 for both users + USER_IDS.forEach { + assertThat( + service.setDomainVerificationUserSelection( + UUID_TWO, + setOf(DOMAIN_1, DOMAIN_3, DOMAIN_4), + true, + it + ) + ).isEqualTo(DomainVerificationManager.STATUS_OK) + } + + // But disable the owner package link handling in the secondary user + service.setDomainVerificationLinkHandlingAllowed(pkg1.packageName, false, + USER_ID_SECONDARY + ) + + service.assertState( + pkg1, + verifyState = listOf( + DOMAIN_1 to STATE_NO_RESPONSE, + DOMAIN_2 to STATE_NO_RESPONSE, + DOMAIN_3 to STATE_NO_RESPONSE, + ), + userState2LinkHandlingAllowed = false + ) + + service.assertState( + pkg2, + verifyState = null, + userState1DomainState1 = DOMAIN_STATE_SELECTED, + userState1DomainState3 = DOMAIN_STATE_SELECTED, + userState1DomainState4 = DOMAIN_STATE_SELECTED, + userState2DomainState1 = DOMAIN_STATE_SELECTED, + userState2DomainState3 = DOMAIN_STATE_SELECTED, + userState2DomainState4 = DOMAIN_STATE_SELECTED, + ) + + // Verify the owner package + service.setStatusBlock(pkg1) + + // Assert that package 1 is now verified, but link handling disabled in secondary user + service.assertState( + pkg1, + verifyState = listOf( + DOMAIN_1 to STATE_SUCCESS, + DOMAIN_2 to STATE_SUCCESS, + DOMAIN_3 to STATE_NO_RESPONSE, + ), + userState1DomainState1 = DOMAIN_STATE_VERIFIED, + userState1DomainState2 = DOMAIN_STATE_VERIFIED, + userState1DomainState3 = DOMAIN_STATE_NONE, + userState1DomainState4 = DOMAIN_STATE_NONE, + userState2LinkHandlingAllowed = false, + userState2DomainState1 = DOMAIN_STATE_VERIFIED, + userState2DomainState2 = DOMAIN_STATE_VERIFIED, + userState2DomainState3 = DOMAIN_STATE_NONE, + userState2DomainState4 = DOMAIN_STATE_NONE, + ) + + // Assert package 2 maintains selected in user where package 1 had link handling disabled + service.assertState( + pkg2, + verifyState = null, + userState1DomainState1 = DOMAIN_STATE_NONE, + userState1DomainState3 = DOMAIN_STATE_SELECTED, + userState1DomainState4 = DOMAIN_STATE_SELECTED, + userState2DomainState1 = DOMAIN_STATE_SELECTED, + userState2DomainState3 = DOMAIN_STATE_SELECTED, + userState2DomainState4 = DOMAIN_STATE_SELECTED, + ) + } + + fun DomainVerificationService.assertState( + pkg: PackageStateInternal, + verifyState: List>?, + userState1LinkHandlingAllowed: Boolean = true, + userState1DomainState1: Int = DOMAIN_STATE_NONE, + userState1DomainState2: Int = DOMAIN_STATE_NONE, + userState1DomainState3: Int = DOMAIN_STATE_NONE, + userState1DomainState4: Int = DOMAIN_STATE_NONE, + userState2LinkHandlingAllowed: Boolean = true, + userState2DomainState1: Int = DOMAIN_STATE_NONE, + userState2DomainState2: Int = DOMAIN_STATE_NONE, + userState2DomainState3: Int = DOMAIN_STATE_NONE, + userState2DomainState4: Int = DOMAIN_STATE_NONE, + ) { + if (verifyState == null) { + // If no auto verify domains, the info itself will be null + assertThat(getDomainVerificationInfo(pkg.packageName)).isNull() + } else { + getInfo(pkg.packageName).run { + assertThat(hostToStateMap).containsExactlyEntriesIn(verifyState.associate { it }) + } + } + + getUserState(pkg.packageName, USER_ID).run { + assertThat(isLinkHandlingAllowed).isEqualTo(userState1LinkHandlingAllowed) + assertThat(hostToStateMap).containsExactlyEntriesIn( + mapOf( + DOMAIN_1 to userState1DomainState1, + DOMAIN_2 to userState1DomainState2, + DOMAIN_3 to userState1DomainState3, + DOMAIN_4 to userState1DomainState4, + ) + ) + } + + getUserState(pkg.packageName, USER_ID_SECONDARY).run { + assertThat(isLinkHandlingAllowed).isEqualTo(userState2LinkHandlingAllowed) + assertThat(hostToStateMap).containsExactlyEntriesIn( + mapOf( + DOMAIN_1 to userState2DomainState1, + DOMAIN_2 to userState2DomainState2, + DOMAIN_3 to userState2DomainState3, + DOMAIN_4 to userState2DomainState4, + ) + ) + } + } + private fun DomainVerificationService.getInfo(pkgName: String) = getDomainVerificationInfo(pkgName) .also { assertThat(it).isNotNull() }!!