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 credential queries #206

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

acrusage-iaik
Copy link
Contributor

No description provided.

@acrusage-iaik acrusage-iaik force-pushed the feature/2025/01/14-DCQL-credential-queries branch from 13484be to b5e3812 Compare January 28, 2025 17:51
Copy link
Contributor

@nodh nodh left a comment

Choose a reason for hiding this comment

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

Great work!

if(credentialSetQuery.required) {
throw IllegalArgumentException("Required credential set query is not satisfiable.", it)
if (credentialSetQuery.required) {
throw IllegalArgumentException(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please refactor into one line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestions please, autoformat breaks this otherwise?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not when your margin is at 120, as it is throughout the code base

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

* see [createAuthnRequest]
*/
@Suppress("DEPRECATION")
suspend fun prepareAuthnRequest(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this function in addition to createAuthnRequest()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not this function itself that's necessary, but the corresponding submitAuthnRequest.

Presentation definitions/dcql queries allow rich configurations, which the current api does not support

We therefore need a way to feed the adjusted auth request to the state map which is later used when receiving a presentation to link it to the original request.
Especially with DCQL, when verifying a presentation we need to know the corresponding credential format, which can only be deduced from the original request.

This function just prevents unnecessarily adding values to the state map

Copy link
Contributor

@nodh nodh Jan 28, 2025

Choose a reason for hiding this comment

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

Okay, but then the existing createAuthnRequest() method should call this new method, because the code is the partly the same, right?

But we could build a DCQL Query from the RequestOptions in this class, right? At least something like now for presentation definition: Type, format, attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@acrusage-iaik acrusage-iaik force-pushed the feature/2025/01/14-DCQL-credential-queries branch from b5e3812 to 237d7a5 Compare January 28, 2025 18:03
@@ -86,13 +116,13 @@ interface Holder {
* @param pathAuthorizationValidator Provides the user of this library with a way to enforce
* authorization rules.
*/
@Deprecated("Replace with more general implementation due to increasing number of presentation mechanisms")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we migrate the existing calls from test code, or should this better be done in a second PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a separate refactor would be good, a lot of tests are affected

@acrusage-iaik acrusage-iaik force-pushed the feature/2025/01/14-DCQL-credential-queries branch from c00a611 to 2efe7ab Compare January 29, 2025 14:01
Copy link
Collaborator

@JesusMcCloud JesusMcCloud left a comment

Choose a reason for hiding this comment

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

Smashing!

I'm trusting @nodh on the functional and architectural correctness on this one, but I do have a change request.

@@ -14,6 +14,8 @@ enum class CredentialFormatEnum(val text: String) {
JSON_LD("ldp_vc"),
MSO_MDOC("mso_mdoc");

fun coerce() = if(this == VC_SD_JWT) DC_SD_JWT else this
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer to have proper API docs on this function and have CredentialFormat, VC_SD_JWT, and DC_SD_JWT mention it too, or maybe even have a format constant that encompasses both. one hierarchy level above and mark the existing ones with a proper deprecation annotation. If we go for the latter approach, we won't need any coerce-function at all

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