Skip to content

Conversation

@danielfett
Copy link
Contributor

@danielfett danielfett commented Mar 28, 2025

This PR does the following:

  • Remove Presentation Exchange as a query language incl. the auth request parameters presentation_definition and presentation_definition_uri
  • Introduce the DCQL feature contains to enable matching W3C VC types, which so far was not possible with DCQL - reverted, see discussion
  • Update the DCQL rules and extra parameters for W3C VCs and AnonCreds to ensure that useful queries can be made
  • Update all relevant examples

Fixes #211

merging this unblocks PR on #6

@danielfett danielfett marked this pull request as ready for review March 28, 2025 12:59
@danielfett danielfett requested review from c2bo, Copilot and jogu March 28, 2025 12:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 25 changed files in this pull request and generated no comments.

Files not reviewed (20)
  • examples/request/dcql_ac_vc_sd.json: Language not supported
  • examples/request/dcql_jwt_vc.json: Language not supported
  • examples/request/dcql_ldp_vc.json: Language not supported
  • examples/request/pd_ac_vc.json: Language not supported
  • examples/request/pd_ac_vc_sd.json: Language not supported
  • examples/request/pd_jwt_vc.json: Language not supported
  • examples/request/pd_ldp_vc.json: Language not supported
  • examples/request/pd_sd_jwt_vc.json: Language not supported
  • examples/request/request_object_client_id_did.json: Language not supported
  • examples/request/vp_token_alternative_credentials.json: Language not supported
  • examples/request/vp_token_type_and_claims.json: Language not supported
  • examples/request/vp_token_type_only.json: Language not supported
  • examples/response/jarm_jwt_enc_only_vc_json_body.json: Language not supported
  • examples/response/jarm_jwt_vc_json_body.json: Language not supported
  • examples/response/presentation_submission.json: Language not supported
  • examples/response/presentation_submission_multiple_vps.json: Language not supported
  • examples/response/ps_ac_vc_sd.json: Language not supported
  • examples/response/ps_jwt_vc.json: Language not supported
  • examples/response/ps_ldp_vc.json: Language not supported
  • examples/response/ps_sd_jwt_vc.json: Language not supported

@danielfett danielfett requested a review from Sakurann March 28, 2025 13:00
Copy link
Contributor

@leecam leecam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to split out the support for contains in a separate PR. But looks good to me either way

Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
Comment on lines -1424 to -1447
`invalid_presentation_definition_uri`:

- The Presentation Definition URL cannot be reached.

`invalid_presentation_definition_reference`:

- The Presentation Definition URL can be reached, but the specified `presentation_definition` cannot be found at the URL.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support it, and we never defined dcql_query_uri parameter but I think it's worth noting that with this PR, we are losing the ability to pass query language by reference

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That ability doesn't work with DC API anyway so I'm not sure it's a big loss.

<{{examples/response/token_response_vp_token_sd_jwt_vc.txt}}

In this example the `vp_token` contains only the disclosures for the claims specified in the `presentation_submission`, along with a Key Binding JWT.
The `transaction_data_hashes` response parameter defined in (#transaction_data), if Transaction Data is used, MUST be included in the Key Binding JWT as a top level claim. This means that transaction data mechanism cannot be used with SD-JWT VCs without cryptographic key binding (i.e., which do not use the KB-JWT).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will probably conflict with #421. tho #421 probably getting merged first. @sander

Copy link
Member

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good to me, especially the removal of the Presentation Exchange part.

I see some missing gaps related to DCQL querying and the Verifier Metadata which were highlighted because of the changes in this PR, for which I've opened #495 and #494 (I'm happy to a PR for both of those issues).

Copy link
Member

@c2bo c2bo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about that one sentence to validate vp_token - everything else looks good to me

Comment on lines -1424 to -1447
`invalid_presentation_definition_uri`:

- The Presentation Definition URL cannot be reached.

`invalid_presentation_definition_reference`:

- The Presentation Definition URL can be reached, but the specified `presentation_definition` cannot be found at the URL.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That ability doesn't work with DC API anyway so I'm not sure it's a big loss.


* `proof_type_values`: OPTIONAL. A JSON array containing types of proofs that
the Verifier accepts to be used in the Verifiable Presentation, for example
`["RsaSignature2018"]`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'18 was a good vintage for RSA

@TimoGlastra TimoGlastra mentioned this pull request Apr 4, 2025
Copy link
Member

@bc-pi bc-pi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swooping in at the 11th hour with some trivial nitpicking suggestions but overall approve

(done with permission of Daniel, who checked them but isn't currently somewhere where he could easily apply them)

Co-authored-by: Brian Campbell <71398439+bc-pi@users.noreply.github.com>
@jogu
Copy link
Collaborator

jogu commented Apr 4, 2025

Merging as per agreement on yesterday's WG call and noting that there have been no objections to Kristina explicitly asking on the mailing list if anyone had any concerns ( https://lists.openid.net/pipermail/openid-specs-digital-credentials-protocols/Week-of-Mon-20250331/000707.html )

@jogu jogu merged commit d2edbe6 into main Apr 4, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

co-existence of multiple query languages (to PE, or not to PE)