Dedupe credential options There is inconsistency in how credential providers process credential options and returns credential responses. Some providers return entries for each option while some providers return entries for deduped options. This results in inconsitent output to users. To solve this, dedupe credential options in CredentialAutofillService, and track which autofill ids requested which credential option. Then make a single credential request with deduped options and when it receives the entries, duplicate them for each autofill id that requested the credential option. Test: atest CtsAutoFillServiceTestCases:android.autofillservice.cts.inline.InlineLoginMixedCredentialActivityTest Bug: 324126795 Change-Id: Ie236676a756a0bbabca37965a72cd65e25b2065a
diff --git a/core/java/android/app/assist/AssistStructure.java b/core/java/android/app/assist/AssistStructure.java index d139134c..fb1b17b 100644 --- a/core/java/android/app/assist/AssistStructure.java +++ b/core/java/android/app/assist/AssistStructure.java
@@ -10,6 +10,7 @@ import android.app.Activity; import android.content.ComponentName; import android.content.Context; +import android.credentials.CredentialOption; import android.credentials.GetCredentialException; import android.credentials.GetCredentialRequest; import android.credentials.GetCredentialResponse; @@ -29,6 +30,7 @@ import android.os.RemoteException; import android.os.SystemClock; import android.service.autofill.FillRequest; +import android.service.credentials.CredentialProviderService; import android.text.InputType; import android.text.Spanned; import android.text.TextUtils; @@ -2258,6 +2260,17 @@ @NonNull OutcomeReceiver<GetCredentialResponse, GetCredentialException> callback) { mNode.mGetCredentialRequest = request; mNode.mGetCredentialCallback = callback; + for (CredentialOption option : request.getCredentialOptions()) { + ArrayList<AutofillId> ids = option.getCandidateQueryData() + .getParcelableArrayList( + CredentialProviderService.EXTRA_AUTOFILL_ID, AutofillId.class); + ids = ids != null ? ids : new ArrayList<>(); + if (!ids.contains(getAutofillId())) { + ids.add(getAutofillId()); + } + option.getCandidateQueryData() + .putParcelableArrayList(CredentialProviderService.EXTRA_AUTOFILL_ID, ids); + } } @Override
diff --git a/core/java/android/view/View.java b/core/java/android/view/View.java index 028448c..06dc275 100644 --- a/core/java/android/view/View.java +++ b/core/java/android/view/View.java
@@ -85,6 +85,7 @@ import android.content.res.Resources; import android.content.res.TypedArray; import android.credentials.CredentialManager; +import android.credentials.CredentialOption; import android.credentials.GetCredentialException; import android.credentials.GetCredentialRequest; import android.credentials.GetCredentialResponse; @@ -129,6 +130,7 @@ import android.os.Trace; import android.os.Vibrator; import android.os.vibrator.Flags; +import android.service.credentials.CredentialProviderService; import android.sysprop.DisplayProperties; import android.text.InputType; import android.text.TextUtils; @@ -7033,6 +7035,18 @@ @NonNull OutcomeReceiver<GetCredentialResponse, GetCredentialException> callback) { Preconditions.checkNotNull(request, "request must not be null"); Preconditions.checkNotNull(callback, "request must not be null"); + + for (CredentialOption option : request.getCredentialOptions()) { + ArrayList<AutofillId> ids = option.getCandidateQueryData() + .getParcelableArrayList( + CredentialProviderService.EXTRA_AUTOFILL_ID, AutofillId.class); + ids = ids != null ? ids : new ArrayList<>(); + if (!ids.contains(getAutofillId())) { + ids.add(getAutofillId()); + } + option.getCandidateQueryData() + .putParcelableArrayList(CredentialProviderService.EXTRA_AUTOFILL_ID, ids); + } mViewCredentialHandler = new ViewCredentialHandler(request, callback); }
diff --git a/packages/CredentialManager/src/com/android/credentialmanager/autofill/CredentialAutofillService.kt b/packages/CredentialManager/src/com/android/credentialmanager/autofill/CredentialAutofillService.kt index 5599ccd..33cd2dd 100644 --- a/packages/CredentialManager/src/com/android/credentialmanager/autofill/CredentialAutofillService.kt +++ b/packages/CredentialManager/src/com/android/credentialmanager/autofill/CredentialAutofillService.kt
@@ -40,6 +40,7 @@ import android.service.autofill.FillCallback import android.service.autofill.FillRequest import android.service.autofill.FillResponse +import android.service.autofill.Flags import android.service.autofill.InlinePresentation import android.service.autofill.Presentations import android.service.autofill.SaveCallback @@ -479,18 +480,28 @@ val autofillIdToCredentialEntries: MutableMap<AutofillId, ArrayList<Entry>> = mutableMapOf() credentialEntryList.forEach entryLoop@{ credentialEntry -> - val autofillId: AutofillId? = credentialEntry - .frameworkExtrasIntent - ?.getParcelableExtra( - CredentialProviderService.EXTRA_AUTOFILL_ID, - AutofillId::class.java) - if (autofillId == null) { - Log.e(TAG, "AutofillId is missing from credential entry. Credential" + - " Integration might be disabled.") - return@entryLoop - } - autofillIdToCredentialEntries.getOrPut(autofillId) { ArrayList() } - .add(credentialEntry) + val intent = credentialEntry.frameworkExtrasIntent + intent?.getParcelableExtra( + CredentialProviderService.EXTRA_GET_CREDENTIAL_REQUEST, + android.service.credentials.GetCredentialRequest::class.java) + ?.credentialOptions + ?.forEach { credentialOption -> + credentialOption.candidateQueryData.getParcelableArrayList( + CredentialProviderService.EXTRA_AUTOFILL_ID, AutofillId::class.java) + ?.forEach { autofillId -> + intent.putExtra( + CredentialProviderService.EXTRA_AUTOFILL_ID, + autofillId) + val entry = Entry( + credentialEntry.key, + credentialEntry.subkey, + credentialEntry.slice, + intent) + autofillIdToCredentialEntries + .getOrPut(autofillId) { ArrayList() } + .add(entry) + } + } } return autofillIdToCredentialEntries } @@ -573,23 +584,31 @@ cmRequests: MutableList<CredentialOption>, responseClientState: Bundle ) { + val traversedViewNodes: MutableSet<AutofillId> = mutableSetOf() + val credentialOptionsFromHints: MutableMap<String, CredentialOption> = mutableMapOf() val windowNodes: List<AssistStructure.WindowNode> = structure.run { (0 until windowNodeCount).map { getWindowNodeAt(it) } } windowNodes.forEach { windowNode: AssistStructure.WindowNode -> - traverseNodeForRequest(windowNode.rootViewNode, cmRequests, responseClientState) + traverseNodeForRequest( + windowNode.rootViewNode, cmRequests, responseClientState, traversedViewNodes, + credentialOptionsFromHints) } } private fun traverseNodeForRequest( viewNode: AssistStructure.ViewNode, cmRequests: MutableList<CredentialOption>, - responseClientState: Bundle + responseClientState: Bundle, + traversedViewNodes: MutableSet<AutofillId>, + credentialOptionsFromHints: MutableMap<String, CredentialOption> ) { viewNode.autofillId?.let { - cmRequests.addAll(getCredentialOptionsFromViewNode(viewNode, it, responseClientState)) + cmRequests.addAll(getCredentialOptionsFromViewNode(viewNode, it, responseClientState, + traversedViewNodes, credentialOptionsFromHints)) + traversedViewNodes.add(it) } val children: List<AssistStructure.ViewNode> = @@ -598,26 +617,37 @@ } children.forEach { childNode: AssistStructure.ViewNode -> - traverseNodeForRequest(childNode, cmRequests, responseClientState) + traverseNodeForRequest(childNode, cmRequests, responseClientState, traversedViewNodes, + credentialOptionsFromHints) } } private fun getCredentialOptionsFromViewNode( viewNode: AssistStructure.ViewNode, autofillId: AutofillId, - responseClientState: Bundle + responseClientState: Bundle, + traversedViewNodes: MutableSet<AutofillId>, + credentialOptionsFromHints: MutableMap<String, CredentialOption> ): MutableList<CredentialOption> { - if (viewNode.credentialManagerRequest != null) { - val options = viewNode.credentialManagerRequest?.getCredentialOptions() - if (options != null) { - for (option in options) { - option.candidateQueryData.putParcelable( - CredentialProviderService.EXTRA_AUTOFILL_ID, autofillId - ) - } - return options + val credentialOptions: MutableList<CredentialOption> = mutableListOf() + if (Flags.autofillCredmanDevIntegration() && viewNode.credentialManagerRequest != null) { + viewNode.credentialManagerRequest + ?.getCredentialOptions() + ?.forEach { credentialOption -> + credentialOption.candidateQueryData + .getParcelableArrayList( + CredentialProviderService.EXTRA_AUTOFILL_ID, AutofillId::class.java) + ?.let { associatedAutofillIds -> + // Check whether any of the associated autofill ids have already been + // traversed. If so, skip, to dedupe on duplicate credential options. + if ((traversedViewNodes intersect associatedAutofillIds.toSet()) + .isEmpty()) { + credentialOptions.add(credentialOption) + } + } } } + // TODO(b/325502552): clean up cred option logic in autofill hint val credentialHints: MutableList<String> = mutableListOf() if (viewNode.autofillHints != null) { @@ -631,10 +661,10 @@ } } - val credentialOptions: MutableList<CredentialOption> = mutableListOf() for (credentialHint in credentialHints) { try { - convertJsonToCredentialOption(credentialHint, autofillId) + convertJsonToCredentialOption( + credentialHint, autofillId, credentialOptionsFromHints) .let { credentialOptions.addAll(it) } } catch (e: JSONException) { Log.i(TAG, "Exception while parsing response: " + e.message) @@ -643,10 +673,11 @@ return credentialOptions } - private fun convertJsonToCredentialOption(jsonString: String, autofillId: AutofillId): - List<CredentialOption> { - // TODO(b/302000646) Move this logic to jetpack so that is consistent - // with building the json + private fun convertJsonToCredentialOption( + jsonString: String, + autofillId: AutofillId, + credentialOptionsFromHints: MutableMap<String, CredentialOption> + ): List<CredentialOption> { val credentialOptions: MutableList<CredentialOption> = mutableListOf() val json = JSONObject(jsonString) @@ -654,16 +685,34 @@ val options = jsonGet.getJSONArray(CRED_OPTIONS_KEY) for (i in 0 until options.length()) { val option = options.getJSONObject(i) - val candidateBundle = convertJsonToBundle(option.getJSONObject(CANDIDATE_DATA_KEY)) - candidateBundle.putParcelable( + val optionString = option.toString() + credentialOptionsFromHints[optionString] + ?.let { credentialOption -> + // if the current credential option was seen before, add the current + // viewNode to the credential option, but do not add it to the option list + // again. This will result in the same result as deduping based on + // traversed viewNode. + credentialOption.candidateQueryData.getParcelableArrayList( + CredentialProviderService.EXTRA_AUTOFILL_ID, AutofillId::class.java) + ?.let { + it.add(autofillId) + credentialOption.candidateQueryData.putParcelableArrayList( + CredentialProviderService.EXTRA_AUTOFILL_ID, it) + } + } ?: run { + val candidateBundle = convertJsonToBundle(option.getJSONObject(CANDIDATE_DATA_KEY)) + candidateBundle.putParcelableArrayList( CredentialProviderService.EXTRA_AUTOFILL_ID, - autofillId) - credentialOptions.add(CredentialOption( + arrayListOf(autofillId)) + val credentialOption = CredentialOption( option.getString(TYPE_KEY), convertJsonToBundle(option.getJSONObject(REQUEST_DATA_KEY)), candidateBundle, option.getBoolean(SYS_PROVIDER_REQ_KEY), - )) + ) + credentialOptions.add(credentialOption) + credentialOptionsFromHints[optionString] = credentialOption + } } return credentialOptions }
diff --git a/services/credentials/java/com/android/server/credentials/ProviderGetSession.java b/services/credentials/java/com/android/server/credentials/ProviderGetSession.java index ca23d62..fa63bc8 100644 --- a/services/credentials/java/com/android/server/credentials/ProviderGetSession.java +++ b/services/credentials/java/com/android/server/credentials/ProviderGetSession.java
@@ -30,9 +30,7 @@ import android.credentials.selection.Entry; import android.credentials.selection.GetCredentialProviderData; import android.credentials.selection.ProviderPendingIntentResponse; -import android.os.Bundle; import android.os.ICancellationSignal; -import android.service.autofill.Flags; import android.service.credentials.Action; import android.service.credentials.BeginGetCredentialOption; import android.service.credentials.BeginGetCredentialRequest; @@ -44,7 +42,6 @@ import android.service.credentials.RemoteEntry; import android.util.Pair; import android.util.Slog; -import android.view.autofill.AutofillId; import java.util.ArrayList; import java.util.HashMap; @@ -77,10 +74,6 @@ @NonNull private final Map<String, CredentialOption> mBeginGetOptionToCredentialOptionMap; - @NonNull - private final Map<String, AutofillId> mCredentialEntryKeyToAutofilLIdMap; - - /** The complete request to be used in the second round. */ private final android.credentials.GetCredentialRequest mCompleteRequest; private final CallingAppInfo mCallingAppInfo; @@ -249,7 +242,6 @@ mBeginGetOptionToCredentialOptionMap = new HashMap<>(beginGetOptionToCredentialOptionMap); mProviderResponseDataHandler = new ProviderResponseDataHandler( ComponentName.unflattenFromString(hybridService)); - mCredentialEntryKeyToAutofilLIdMap = new HashMap<>(); } /** Called when the provider response has been updated by an external source. */ @@ -303,7 +295,7 @@ invokeCallbackOnInternalInvalidState(); return; } - onCredentialEntrySelected(providerPendingIntentResponse, entryKey); + onCredentialEntrySelected(providerPendingIntentResponse); break; case ACTION_ENTRY_KEY: Action actionEntry = mProviderResponseDataHandler.getActionEntry(entryKey); @@ -312,7 +304,7 @@ invokeCallbackOnInternalInvalidState(); return; } - onActionEntrySelected(providerPendingIntentResponse, entryKey); + onActionEntrySelected(providerPendingIntentResponse); break; case AUTHENTICATION_ACTION_ENTRY_KEY: Action authenticationEntry = mProviderResponseDataHandler @@ -342,7 +334,7 @@ break; case REMOTE_ENTRY_KEY: if (mProviderResponseDataHandler.getRemoteEntry(entryKey) != null) { - onRemoteEntrySelected(providerPendingIntentResponse, entryKey); + onRemoteEntrySelected(providerPendingIntentResponse); } else { Slog.i(TAG, "Unexpected remote entry key"); invokeCallbackOnInternalInvalidState(); @@ -381,7 +373,7 @@ return null; } - private Intent setUpFillInIntentWithFinalRequest(@NonNull String id, String entryKey) { + private Intent setUpFillInIntentWithFinalRequest(@NonNull String id) { // TODO: Determine if we should skip this entry if entry id is not set, or is set // but does not resolve to a valid option. For now, not skipping it because // it may be possible that the provider adds their own extras and expects to receive @@ -392,13 +384,6 @@ Slog.w(TAG, "Id from Credential Entry does not resolve to a valid option"); return intent; } - AutofillId autofillId = credentialOption - .getCandidateQueryData() - .getParcelable(CredentialProviderService.EXTRA_AUTOFILL_ID, AutofillId.class); - if (autofillId != null && Flags.autofillCredmanIntegration()) { - intent.putExtra(CredentialProviderService.EXTRA_AUTOFILL_ID, autofillId); - mCredentialEntryKeyToAutofilLIdMap.put(entryKey, autofillId); - } return intent.putExtra( CredentialProviderService.EXTRA_GET_CREDENTIAL_REQUEST, new GetCredentialRequest( @@ -414,13 +399,12 @@ } private void onRemoteEntrySelected( - ProviderPendingIntentResponse providerPendingIntentResponse, String entryKey) { - onCredentialEntrySelected(providerPendingIntentResponse, entryKey); + ProviderPendingIntentResponse providerPendingIntentResponse) { + onCredentialEntrySelected(providerPendingIntentResponse); } private void onCredentialEntrySelected( - ProviderPendingIntentResponse providerPendingIntentResponse, - String entryKey) { + ProviderPendingIntentResponse providerPendingIntentResponse) { if (providerPendingIntentResponse == null) { invokeCallbackOnInternalInvalidState(); return; @@ -437,18 +421,7 @@ GetCredentialResponse getCredentialResponse = PendingIntentResultHandler .extractGetCredentialResponse( providerPendingIntentResponse.getResultData()); - if (getCredentialResponse != null && getCredentialResponse.getCredential() != null) { - Bundle credentialData = getCredentialResponse.getCredential().getData(); - AutofillId autofillId = mCredentialEntryKeyToAutofilLIdMap.get(entryKey); - if (Flags.autofillCredmanIntegration() - && entryKey != null && autofillId != null && credentialData != null - ) { - Slog.d(TAG, "Adding autofillId to credential response: " + autofillId); - credentialData.putParcelable( - CredentialProviderService.EXTRA_AUTOFILL_ID, - mCredentialEntryKeyToAutofilLIdMap.get(entryKey) - ); - } + if (getCredentialResponse != null) { mCallbacks.onFinalResponseReceived(mComponentName, getCredentialResponse); return; @@ -532,9 +505,9 @@ /** Returns true if either an exception or a response is found. */ private void onActionEntrySelected(ProviderPendingIntentResponse - providerPendingIntentResponse, String entryKey) { + providerPendingIntentResponse) { Slog.i(TAG, "onActionEntrySelected"); - onCredentialEntrySelected(providerPendingIntentResponse, entryKey); + onCredentialEntrySelected(providerPendingIntentResponse); } @@ -632,7 +605,7 @@ Entry entry = new Entry(CREDENTIAL_ENTRY_KEY, id, credentialEntry.getSlice(), setUpFillInIntentWithFinalRequest(credentialEntry - .getBeginGetCredentialOptionId(), id)); + .getBeginGetCredentialOptionId())); mUiCredentialEntries.put(id, new Pair<>(credentialEntry, entry)); mCredentialEntryTypes.add(credentialEntry.getType()); }