Clarify an empty object in a VP Token cannot be used to signify an error response#745
Clarify an empty object in a VP Token cannot be used to signify an error response#745awoie wants to merge 3 commits into
Conversation
|
We probably will need to apply this logic to DC API as well. I will need to double check if this is included with the current proposed text. |
fkj
left a comment
fkj
left a comment
There was a problem hiding this comment.
If people don't object to the rewriting of the definition, I agree that this makes it easier to understand what we actually mean.
| * The object MUST NOT contain an entry for an optional Credential Query when | ||
| there are no matching Credentials for that Credential Query. |
There was a problem hiding this comment.
This is hard to understand for me, is this repeating things from DCQL rules? Why "optional"?
There was a problem hiding this comment.
I think this line is rather confusing and repeating DCQL rules
There was a problem hiding this comment.
I know that but it was there previously. Happy to remove it but on the WG call on July 2, we decided to keep it.
|
wg discussion, should be in the errata as well. |
| : REQUIRED. A JSON-encoded object subject to the following requirements: | ||
|
|
||
| * Each key MUST be the `id` of a Credential Query in the DCQL query. | ||
|
|
There was a problem hiding this comment.
we usually don't use empty lines in enumerations
There was a problem hiding this comment.
But we do also in this spec doc and if I remember correctly, this won't render nicely if it was removed.
Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
| * The object MUST NOT be empty | ||
| returning any Presentation according to | ||
| (#dcql_query_lang_processing_rules). |
There was a problem hiding this comment.
There's missing words or something here. This isn't a sentence that makes sense.
| The Wallet MUST return a VP Token only if the set of Presentations represented | ||
| by the VP Token satisfies the requirements of the DCQL query according to (#dcql_query_lang_processing_rules). |
There was a problem hiding this comment.
There's a gap/ambiguity here I think. If all the requested credentials are set as optional, returning no credentials satisfies the DCQL query, so you can/must return a VP Token. But then the rules below leave no VP Token that is valid to return.
New test module oid4vp-1final-wallet-negative-test-required-non-matching-credential sends a DCQL query whose credential_sets require both the real credential and a non-existent one. As the query as a whole cannot be satisfied, the wallet must not return a vp_token - neither an empty object nor a partial one containing only the real credential - as clarified in openid/OpenID4VP#745. The module accepts either an error response (access_denied expected; other codes give a warning; encrypted error responses are supported in the .jwt response modes) or the wallet rejecting the request and displaying an error, verified via a screenshot placeholder. AddOptionalNonMatchingCredentialToDcqlQuery is refactored into an abstract base shared with the new required variant, and the browser API response parsing/rejection handling in AbstractVP1FinalWalletTest is extracted into reusable helpers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per WG consensus on #743, an empty VP Token (i.e., {}) MUST NOT be used, including to signify an error. When the Wallet has no Presentation to return - e.g., no consent, no matching Credentials, or a DCQL query satisfiable without any Presentation because all unfulfilled Credential Queries are optional - any response it returns MUST be an error response rather than a VP Token. Also fixes the truncated sentence in the vp_token requirements, applies the same clarification to 1.0 as an errata set 1 change with changelog entries in both documents, and removes the accidentally committed .DS_Store.
|
Please review again @jogu @paulbastian @Sakurann . @jogu Note that if my memory is correct that the WG agreed to prohibit empty vp_token's even for the case where the DCQL query has only optional queries. |
6 participants
Fixes #743
I also improved the vp_token definition because it is a quite important part of the spec and the readability was not great.
I removed this editorially because I don't think it had any meaning anymore: