mDL Presentment Flow: Unification of Presentment Prompts #647

dritan-x · 2024-06-17T10:46:33Z

Created PresentmentFlow to start a Presentment Flow for all document requests and obtain the response bytes with all the Documents from successful Presentment Flows (once all the prompts succeed). An exception is thrown otherwise.

Refactored PresentationActivity in Wallet to be a transparent Activity and removed all Compose components. Once the device listener has the device request bytes it immediately uses PresentmentFlow to start showing the Presentment Prompts and receive the response bytes to send back to the device.

Test: all unit tests ran and passed.
Test: manual mDL Presentment via NFC and QR engagement between Wallet and AppVerifier.
Test: Requesting multiple docRequests via Request mDL + micov with linkage — works for only the Mdoc type since I couldn't find a way to add a micov docType to Wallet.

davidz25

Hi, sorry for the late reply, comments inline, looks like we're getting closer!

identity/src/main/java/com/android/identity/document/DocumentStore.kt

wallet/src/main/java/com/android/identity_credential/wallet/DocumentModel.kt

wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt

davidz25 · 2024-06-19T19:07:45Z

wallet/src/main/java/com/android/identity_credential/wallet/presentation/PresentmentFlow.kt

+class PresentmentFlow(
+    private val walletApp: WalletApplication,
+    private val fragmentActivity: FragmentActivity
+) {


Why is this a class and now just a single function? I mean, I expected it to look like this:

suspend fun showPresentmentFlow( activity: FragmentActivity, walletApp: WalletApplication, documentRequest: DocumentRequest, encodedSessionTranscript: ByteArray ): ByteArray

Also - as I alluded to in the above - this needs to take a DocumentRequest instead of DeviceRequest CBOR because we want it to be usable also for Credman and OpenID4VP and the request format in both of those cases are not CBOR.

Also, it should not handle multiple document requests in a single request, that's the wrong layer at the stack to do this. That's only relevant for proximity presentations (ie. when you have DeviceRequest CBOR) and in this case the caller (e.g. PresentationActivity) can simply just parse the CBOR themselves, get all the N DocumentRequest instances, and then call showPresentmentFlow() for each of them.

Also, btw, the longer-term goal is to factor this function - as well as things like DocumentModel and showBiometricPrompt() into a identity-appsupport library which wallet and also things like wallet-wearos can call into. So it can't really take a WalletApplication and thus we need at some point to feed the dependencies directly... but no need to change that right now, should be an easy change once we get there...

davidz25 · 2024-06-19T19:25:21Z

Also, please rebase to on top of main so there are no conflicts.

Created PresentmentFlow to start a Presentment Flow for all document requests and obtain the response bytes with all the Documents from successful Presentment Flows (once all the prompts succeed). An exception is thrown otherwise. Refactored PresentationActivity in Wallet to be a transparent Activity and removed all Compose components. Once the device listener has the device request bytes it immediately uses PresentmentFlow to start showing the Presentment Prompts and receive the response bytes to send back to the device. Test: all unit tests ran and passed. Test: manual mDL Presentment via NFC and QR engagement between Wallet and AppVerifier. Test: Requesting multiple docRequests via Request mDL + micov with linkage — works for only the Mdoc type since I couldnt find a way to add a micov docType to Wallet. Signed-off-by: dritan-x <dritan.xhabija@gmail.com>

- Showing the Presentment Flow is now done via showPresentmentFlow() - Wallet PresentationActivity now parses the device request bytes and calls showPresentmentFlow() with each parsed DocumentRequest - Removed one of the PassphraseEntryFields and updated its references - Changed the access modifiers to public for the ConsentPrompt and PassphrasePrompt Dialog Fragments, that addresses IllegalStateException: Fragment must be a public static class to be properly recreated from instance state. test: manual presentment between Wallet and AppVerifier test: all unit tests pass (419) Signed-off-by: dritan-x <dritan.xhabija@gmail.com>

davidz25

Almost there!

davidz25 · 2024-06-21T22:59:46Z

wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt

-    }
+    // TODO: add SD_JWT_VC support
+    // the supported formats for the Credentials of a Document
+    val supportedCredentialFormats = listOf(CredentialFormat.MDOC_MSO)


This TODO should be removed, the only credentials that can be presented over ISO 18013-5 DeviceRetrieval are mdoc credentials. It's possible this could change in the future but it would require the ISO group to make that decision. For the same reason, I we can remove supportedCredentialFormats too.

I only brought it here because when I cleaned up TransferHelper for removal, the existing TODO's (from you) asked for these changes - to process multiple docRequests and support more credential formats.

Initially I defined supportedCredentialFormats inside the function's signature with a default value (and not a val)

fun isDocumentSuitableForDocRequest( document: Document, docType: String, supportedCredentialFormats: List<CredentialFormat> = listOf(CredentialFormat.MDOC_MSO) ): Boolean { ... }

This provides a default value for the 3rd parameter and it does not need to be provided when calling the function (unless you want to check for additional or non-mdoc-mso credential formats).

I think I'll nest supportedCredentialFormats back in the function's signature and give the option to the caller to provide their own list of supported credential formats (esp. useful for testing) if they wanted to but by default it is mdoc mso.

davidz25 · 2024-06-21T23:02:21Z

wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt

+
+                            deviceRequest.docRequests.forEach { docRequest ->
+                                // find a suitable Document for the docRequest
+                                val document = findSuitableDocumentForRequest(docRequest)


I think it would be clearer if findSuitableDocumentForRequest() was called findMdocCredentialForRequest() and returned a MdocCredential? ... then I think you can just do

val mdocCredential = findMdocCredentialForRequest() ?: return@forEach

which I think is quite readable.

In the current behavior findSuitableDocument() throws if it can't find a document and this fails the entire transaction... which is not what we want, for example, suppose an RP asks for two docs (for example, mDL and COVID cert) ... you still want that transaction to succeed even if we can return one of the documents.

Great idea for findMdocCredentialForRequest 👍 and you're absolutely right about the exception of findSuitableDocument, it should iterate through every docRequest even if the previous one had no matching MdocCredentials :) thanks!

davidz25 · 2024-06-21T23:08:45Z

wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt

+                                    Clock.System.now()
+                                ) as MdocCredential
+
+                                // extract the TrustPoint if possible


This makes it sound like the TrustPoint is embedded in the request, which it isn't. I think it would be clearer if the comment said "See if we recognize the reader..."

davidz25 · 2024-06-21T23:10:28Z

wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt

-                    }
-                }
+                                // generate the DocumentRequest from the current docRequest
+                                val documentRequest = MdocUtil.generateDocumentRequest(docRequest)


Nit: Moving documentRequest = MdocUtil.generateDocumentRequest(docRequest) into the showPresentmentCall() would save a couple of lines!

davidz25 · 2024-06-21T23:17:10Z

wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt

+                                    trustPoint = trustPoint,
+                                    encodedSessionTranscript = deviceRetrievalHelper!!.sessionTranscript
+                                )
+                                sendResponseToDevice(responseBytes)


This won't work with multi-document presentations b/c here we're sending a DeviceResponse per document request. This is easily fixed by changing showPresentmentFlow() so it returns the bytes of Document CBOR according to 18013-5. That is, the output of DocumentGenerator.generate().

With this, you simply init a DeviceResponseGenerator at the top (before deviceRequest.docRequests.forEach loop), then call addDocument(documentResponse) and after the loop, you can do deviceRetrievalHelper.sendDeviceResponse(deviceResponseGenerator.generate())

ah I see, I literally had this exact code that you mention in the first commit but it was in the wrong layer, I'll bring it back on PresentationActivity 🙏

davidz25 · 2024-06-21T23:20:57Z

wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt

+            state.value = State.RESPONSE_SENT
+        }
+            ?: throw IllegalStateException("Unable to send response bytes, deviceRetrievalHelper is [null].")
+}


This seems unnecessary to have in a helper function, just use deviceRetrievalHelper from the call site.

Totally! that makes sense however this is a reusable function that is called in 2 places, it hides the low level details of what it takes to send bytes via deviceRetrievalHelper. Copy pasting that code around doesn't sound too maintainable and it costs 7 lines of code vs 1 line to call "sendResponseToDevice".

I think the current implementation is clear to read, it's 1 line of code

sendResponseToDevice(bytes)

Without the function sendResponseToDevice(bytes), the deviceRetrievalHelper code can be inlined to

deviceRetrievalHelper?.run { sendDeviceResponse( deviceResponseGenerator.generate(), Constants.SESSION_DATA_STATUS_SESSION_TERMINATION ) state.value = State.RESPONSE_SENT }

Ends up costing the reviewer 14 lines to read and no maintainability
vs
2 + 7 lines + easy to change 1 function so its changes propagate to all callers (you don't need to go in 2..n places in the code to make changes to how bytes are sent via deviceRetrievalHelper)

davidz25 · 2024-06-21T23:21:48Z

wallet/src/main/java/com/android/identity_credential/wallet/presentation/PresentmentFlow.kt

+    mdocCredential: MdocCredential,
+    trustPoint: TrustPoint?,
+    encodedSessionTranscript: ByteArray
+): ByteArray {


As discussed this needs to return the bytes of the Document CBOR according to 18013-5. That's actually already what the kdoc says but the code does something else... should be a very simple change!

I believe this code is already doing that!

Once user succeeds the Consent Prompt on line 79, it uses thedeviceResponseGenerator on line 85

val deviceResponseGenerator = DeviceResponseGenerator(Constants.DEVICE_RESPONSE_STATUS_OK)

to add the Document on line 108 after successfully passing all Prompts when trying to sign the Document data with Credentials and KeyUnlockData

If successful because the auth key is unlocked for the secure area the credential is in (and it's not throwing a KeyLockedException). then finally returns the bytes of DeviceResponse CBOR on line 109

return deviceResponseGenerator.generate()

I'm talking about the Document CBOR and you are talking about the DeviceResponse CBOR. They are different things. Each DeviceResponse has a number of Document instances, one for each of the mdocs that the user sends.

davidz25 · 2024-06-21T23:24:42Z

wallet/src/main/java/com/android/identity_credential/wallet/PresentationActivity.kt

-                    deviceRetrievalHelper?.disconnect()
-                    deviceRetrievalHelper = null
-                    state.value = State.NOT_CONNECTED
+    fun findSuitableDocumentForRequest(docRequest: DeviceRequestParser.DocRequest): Document {


As discussed should be called findMdocCredentialForRequest() and return a MdocCredential?

Test: all unit tests pass Test: successful mDL Presentment Flow between Wallet and AppVerifier Signed-off-by: dritan-x <dritan.xhabija@gmail.com>

davidz25

LGTM, thanks for all the work and enduring all the ping-pongs!

dritan-x requested a review from davidz25 June 17, 2024 10:46

dritan-x mentioned this pull request Jun 17, 2024

MDL Presentation Flow: Unification of Presentation Prompts #641

Closed

dritan-x changed the title ~~MDL Presentation Flow: Unification of Presentation Prompts~~ MDL Presentation Flow: Unification of Presentment Prompts Jun 17, 2024

dritan-x force-pushed the presentment-unification branch from f34fad5 to ae71f97 Compare June 17, 2024 10:54

dritan-x changed the title ~~MDL Presentation Flow: Unification of Presentment Prompts~~ mDL Presentment Flow: Unification of Presentment Prompts Jun 17, 2024

davidz25 reviewed Jun 19, 2024

View reviewed changes

dritan-x force-pushed the presentment-unification branch from ae71f97 to b20ec5a Compare June 21, 2024 08:59

davidz25 reviewed Jun 21, 2024

View reviewed changes

Updates to Presentment Flow

06930a1

Test: all unit tests pass Test: successful mDL Presentment Flow between Wallet and AppVerifier Signed-off-by: dritan-x <dritan.xhabija@gmail.com>

davidz25 approved these changes Jun 25, 2024

View reviewed changes

davidz25 merged commit 8fd5692 into main Jun 25, 2024
4 checks passed

davidz25 deleted the presentment-unification branch June 25, 2024 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mDL Presentment Flow: Unification of Presentment Prompts #647

mDL Presentment Flow: Unification of Presentment Prompts #647

dritan-x commented Jun 17, 2024 •

edited

Loading

davidz25 left a comment

davidz25 Jun 19, 2024

davidz25 commented Jun 19, 2024

davidz25 left a comment

davidz25 Jun 21, 2024

dritan-x Jun 22, 2024

davidz25 Jun 21, 2024

dritan-x Jun 22, 2024 •

edited

Loading

davidz25 Jun 21, 2024

davidz25 Jun 21, 2024

davidz25 Jun 21, 2024

dritan-x Jun 22, 2024

davidz25 Jun 21, 2024

dritan-x Jun 22, 2024 •

edited

Loading

davidz25 Jun 21, 2024

dritan-x Jun 22, 2024

davidz25 Jun 22, 2024

davidz25 Jun 21, 2024

davidz25 left a comment

mDL Presentment Flow: Unification of Presentment Prompts #647

mDL Presentment Flow: Unification of Presentment Prompts #647

Conversation

dritan-x commented Jun 17, 2024 • edited Loading

davidz25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz25 commented Jun 19, 2024

davidz25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dritan-x Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dritan-x Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz25 left a comment

Choose a reason for hiding this comment

dritan-x commented Jun 17, 2024 •

edited

Loading

dritan-x Jun 22, 2024 •

edited

Loading

dritan-x Jun 22, 2024 •

edited

Loading