Adds support for direct_post for verification. #798
Conversation
Previously we only supported direct_post.jwt. This adds support for direct_post. The main difference is in OpenID4VPPresentationActivity.kt, the new getDirectPostAuthorizationResponseBody function (in contrast to the getDirectPostJwtAuthorizationResponseBody function). Most of the other changes are just plumbing, or allowing the user to decide between direct_post and direct_post.jwt. Tested by: - Manual testing of both direct_post and direct_post.jwt, using openid4vp:// and both mdoc and VC. - ./gradlew check - ./gradlew connectedCheck Signed-off-by: Kevin Deus <[email protected]>
There was a problem hiding this comment.
No blockers, just a few exploratory thoughts which either might help to reveal an issue or help me to understand the code facilities gearing together here. Any reply would be highly appreciated!
(Not approving as I don't understand the change fully yet, sorry).
| session.deviceResponse = vpToken.fromBase64Url() | ||
| } else { | ||
| session.deviceResponse = vpToken.toByteArray() | ||
| if (session.responseMode == "direct_post.jwt") { |
There was a problem hiding this comment.
Perhaps, it is possible to extract the constant for the future-proofness of the code?
| session.deviceResponse = vpToken.toByteArray() | ||
| if (session.responseMode == "direct_post.jwt") { | ||
| updateSessionFromDirectPostJwtResponse(kvPairs, session) | ||
| } else if (session.responseMode == "direct_post") { |
There was a problem hiding this comment.
Same suggestion here. In addition, there seem to be also the else case not covered now (neither of two). Is there such a possibility?
| @@ -1436,7 +1469,7 @@ private fun calcClientMetadata(session: Session, format: String): JSONObject { | |||
| val client_metadata = JSONObject() | |||
| client_metadata.put("authorization_encrypted_response_alg", "ECDH-ES") | |||
| client_metadata.put("authorization_encrypted_response_enc", "A128CBC-HS256") | |||
| client_metadata.put("response_mode", "direct_post.jwt") | |||
| client_metadata.put("response_mode", session.responseMode ?: "direct_post.jwt") | |||
There was a problem hiding this comment.
Can't see the whole picture, but ?: operator is a frequent source of errors when confused with the java ?:
operator. I saw that now we have 2 distinct values for that mode, so just the null check might be inappropriate (just something I would personally doublecheck understanding the code and intent here better).
| @@ -829,8 +880,7 @@ internal fun getAuthRequestFromJwt(signedJWT: SignedJWT, clientId: String?): Aut | |||
| Logger.i(TAG, "presentation_definition: $jsonStr") | |||
| Logger.i(TAG, "Claims Set: ${signedJWT.jwtClaimsSet}") | |||
|
|
|||
| // Check the response mode. This is required as part of mdoc profile; NOTE that sd-jwt profile | |||
| // requires direct_post. | |||
| // Check the response mode. | |||
There was a problem hiding this comment.
Is the original comment became irrelevant in that part after the change?
| @@ -761,6 +811,7 @@ private suspend fun getAuthorizationRequest( | |||
| clientId = requestUri.getQueryParameter("client_id")!!, | |||
| nonce = requestUri.getQueryParameter("nonce")!!, | |||
| responseUri = requestUri.getQueryParameter("response_uri")!!, | |||
| responseMode = requestUri.getQueryParameter("response_mode")!!, | |||
There was a problem hiding this comment.
So the response mode can't be null here? I have no full picture, but in 2 places above I saw that the null case for it treated (with when()) and no null case treated (if else). Could be unrelated to this check but might be worth investigating.
Previously we only supported direct_post.jwt. This adds support for direct_post. The main difference is in OpenID4VPPresentationActivity.kt, the new getDirectPostAuthorizationResponseBody function (in contrast to the getDirectPostJwtAuthorizationResponseBody function). Most of the other changes are just plumbing, or allowing the user to decide between direct_post and direct_post.jwt.
Tested by:
Manual testing of both direct_post and direct_post.jwt, using openid4vp:// and both mdoc and VC.
./gradlew check
./gradlew connectedCheck
Tests pass