Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce new query language (Approach 3) #266

Merged
merged 57 commits into from
Oct 23, 2024
Merged

Conversation

danielfett
Copy link
Contributor

@danielfett danielfett commented Sep 18, 2024

This approach uses ids and a separate definition of valid sets of claims/credentials to return.

The draft text has not been adapted yet, let's discuss the JSON examples first!

Text is now updated as well, please review.

List of issues to solve in separate PRs:

  • Introduce transformed claims?
  • Determine if we need any other format-specific parameters
  • Define interaction with existing query lang (see co-existence of multiple query languages #211)
  • Define how requesting presentations with scope works with the new QL.
  • Define new format for the VP Token
  • Maybe re-introduce purpose?

Fixes #178
Fixes #249
Fixes #255 (or rather, makes it obsolete)
Fixes #193
Fixes #161 (that was the discussion on the requirements for this new language)
Fixes #157 (mandatory to implement features of the query language)
Fixes #164
Fixes #160
Fixes #148

Copy link
Contributor

@paulbastian paulbastian left a comment

Choose a reason for hiding this comment

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

I would prefer having the format specific parameters defined in Appendix B instead of defining mdoc + SD-JWT VC there, but that's not a blocker for me to merge this important piece forward

Copy link

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

I don't see a way to filter on conditional data, has this been excluded intentionally?

For example, requiring that an age field is over 21 is incredibly useful and prevents unnecessary disclosure.

If I have a credential and I'm 20 then I should not need to send the credential (or a subset of its fields) when I could simply determine that I should send nothing.

Without these types of conditionals the query language promotes over-identification.

@danielfett
Copy link
Contributor Author

I don't see a way to filter on conditional data, has this been excluded intentionally?

For example, requiring that an age field is over 21 is incredibly useful and prevents unnecessary disclosure.

If I have a credential and I'm 20 then I should not need to send the credential (or a subset of its fields) when I could simply determine that I should send nothing.

Without these types of conditionals the query language promotes over-identification.

This is not part of this PR, but there's a plan how to do it — we can use the "transformed claims" syntax from "Advanced Syntax for Claims" from the OpenID eKYC working group.

@danielfett
Copy link
Contributor Author

I would prefer having the format specific parameters defined in Appendix B instead of defining mdoc + SD-JWT VC there, but that's not a blocker for me to merge this important piece forward

We can easily move these later with an editorial PR.

Co-authored-by: Paul Bastian <paul.bastian@posteo.de>
@danielfett
Copy link
Contributor Author

I think we should define a better name for the query language; somewhat following @decentralgabe's proposal above, I propose Digital Credential Query Language (DCQL, pronounced Dackel).

@bc-pi bc-pi mentioned this pull request Oct 22, 2024
Copy link
Contributor

@David-Chadwick David-Chadwick left a comment

Choose a reason for hiding this comment

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

minor editorial change requested

- optionally, any of the other Credential Set Queries.

To satisfy a Credential Set Query, the Wallet MUST return a presentation of a
Credential or of Credentials that match to one of the `options` inside the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Credential or of Credentials that match to one of the `options` inside the
set of Credentials that match to one of the `options` inside the

Comment on lines +728 to +736
`namespace`:
: REQUIRED if the Credential Format is based on the mdoc format defined in ISO 18013-5; MUST NOT be present otherwise.
The value MUST be a string that specifies the namespace of the data element
within the mdoc, e.g., `org.iso.18013.5.1`.

`claim_name`:
: REQUIRED if the Credential Format is based on mdoc format defined in ISO 18013-5; MUST NOT be present otherwise.
The value MUST be a string that specifies the data element identifier of the data element within the provided namespace
in the mdoc, e.g., `first_name`.
Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand why these can't or shouldn't or aren't conveyed with path too i.e. ["org.iso.18013.5.1","first_name"] but I'll not stand in the way of progress for this PR and make a note to myself to ask/complain about it later. Or maybe just forget.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I still don't really like this special handling for mdocs. Its pain when implementing it too. So I guess same question from me, can everything just be paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please continue the discussion in #293

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should have noted #293 here when I created it.

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.

"I have some questions, a couple of suggestions ... " but overall approve.

Co-authored-by: Brian Campbell <71398439+bc-pi@users.noreply.github.com>
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.

I share some of the questions already raised, but we should start with something and iterate from there. I think this PR provides a good point for that and we can work out some of the details in the coming weeks/months.
Thanks for the patience @danielfett!


`values`:
: OPTIONAL. An array of strings, integers or boolean values that specifies the expected values of the claim.
If the `values` property is present, the Wallet SHOULD return the claim only if the
Copy link
Member

Choose a reason for hiding this comment

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

Things like "contains" or more advanced functions would (if added) live on this level as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes! Please see the proposal linked in #288

@jogu
Copy link
Collaborator

jogu commented Oct 23, 2024

Discussed on working group call yesterday - there was unanimous agreement to merge this once Daniel's suggested changes for naming are added (which they have been), and to open issues for all items that still need further discussion. To follow the WG's consensus we will dismiss the 'request for changes' and merge.

This is very much a first version, but well over 200 comments it is very difficult to have productive discussions on this PR, and having focussed issues and PRs will make progress easier and make sure we can track items, get consensus and close them off. For clarity we can (and very likely will) make breaking changes to the query language between now and the spec going final. Nothing is being set in stone by merging this.

It would be a great help to the chairs if people can make sure there are issues open for any unaddressed comments they have made - many thanks to those of you that have already done that! We will try to cover at least some of these issues at the hybrid meeting next week.

Many thanks to Daniel for all his hard work on this, and thanks to everyone that has provided feedback and suggestions that have helped improve the proposal - all the time that has been spent on this is greatly appreciated.

@jogu jogu dismissed stale reviews from martijnharing and decentralgabe October 23, 2024 10:33
@jogu jogu merged commit 2eaa550 into main Oct 23, 2024
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