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

Dcql #171

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Dcql #171

wants to merge 12 commits into from

Conversation

auer-martin
Copy link
Contributor

@auer-martin auer-martin commented Nov 21, 2024

Add experimental DCQL query language support

Initial implementation of DCQL (Digital Credential Query Language.

Status

  • ✅ Basic DCQL functionality implemented and tested
  • ⚠️ Known rough edges in underlying DCQL library
  • 🚫 W3C Credential support pending
  • 📝 Includes no other updates regarding the latest OpenID4VP draft

initial tests have verified the basic functionality. More testing is required to ensure that everything works correctly.

Copy link
Collaborator

@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.

Very nice, left some nits. I think the main thing is the comment about linking the submission to the query entries. But maybe it's just hard to see and that happens in the dcql library (as long as it's easy to do this matching for the end user).

Also because vp_token is a mapping from query id to VP, I assume every vp will only contain one credential?

Copy link
Collaborator

@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.

LGTM

@TimoGlastra
Copy link
Collaborator

Hey @nklomp just checking whether you think you can merge / release this already before the funke deadline? Otherwise we'll create a fork now.

Signed-off-by: Timo Glastra <timo@animo.id>
Copy link
Contributor

@nklomp nklomp left a comment

Choose a reason for hiding this comment

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

Some small logic change and a new bug introduced

}

return {
...(opts.id_token ? { id_token: opts.id_token } : {}),
...((opts.vp_token.presentation_definition || opts.vp_token.presentation_definition_uri) && {
...((opts.vp_token.presentation_definition || opts.vp_token.presentation_definition_uri || opts.vp_token.dcql_query) && {
vp_token: {
...(!opts.vp_token.presentation_definition_uri && { presentation_definition: opts.vp_token.presentation_definition }),
Copy link
Contributor

Choose a reason for hiding this comment

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

This line needs to be adjusted, as the definition_uri would not be present with a dcql_query. Suggest to move the opts.vc_token.dclq_query expression to a signle line, so it makes a clear distinction between either a PD or PD URI (current logic) and a dcql_query

@@ -139,6 +144,14 @@ export class AuthorizationResponse {
hasher: verifyOpts.hasher,
},
})
} else {
Copy link
Contributor

@nklomp nklomp Nov 24, 2024

Choose a reason for hiding this comment

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

Rather would have seen an else if, with the below throw in an else. Right now the if expression is asymmetric. In the end it really checks for 3 states, but the if else is only 2 states with then an additional check in the else

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.

3 participants