Skip to content

Conversation

@awoie
Copy link
Contributor

@awoie awoie commented Mar 20, 2025

This PR makes editorial improvements on DC API. Also made the text more consistent with DCQL.

Fixes #453
Fixes #437
Fixes #528
Fixes #541

Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
@jogu
Copy link
Collaborator

jogu commented Mar 25, 2025

Note that I added state to the list of possible DC API parameters since it was not clear whether or not is was supported. By reading the current text, it looked like it was, so I added it explicitly.

state serves no useful purpose with the DC API and there's no need for wallets to support it.

Otherwise the following seemed to be a bit contradicting:

Additional request parameters MAY be defined and used with OpenID4VP over the DC API.
The Wallet MUST ignore any unrecognized parameters.

This text is intended to say "Other OAuth parameters must not be used unless they have explicitly been described somewhere as being suitable to use over the DC API." (along with the standard "If you get something you don't understand, ignore it.")

@jogu @Sakurann @danielfett Is state unrecognized in this case and therefore the verifier shouldn't expect the state to come back?

Yes, correct.

@awoie awoie requested a review from TimoGlastra March 25, 2025 13:38
@awoie awoie requested a review from Sakurann April 15, 2025 12:42
Copy link
Collaborator

@tlodderstedt tlodderstedt left a comment

Choose a reason for hiding this comment

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

suggested small changes

Co-authored-by: Torsten Lodderstedt <torsten@lodderstedt.net>
@Sakurann
Copy link
Collaborator

i think this also fixes #541 by separating security considerations section?

Copy link
Contributor

@martijnharing martijnharing left a comment

Choose a reason for hiding this comment

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

If the proposed change is merged (see comment), I can approve.

@awoie awoie requested a review from martijnharing April 22, 2025 11:54
@awoie
Copy link
Contributor Author

awoie commented Apr 22, 2025

If the proposed change is merged (see comment), I can approve.

done

awoie and others added 4 commits April 22, 2025 20:07
Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
Co-authored-by: Tim Cappalli <tim@cappalli.me>
@Sakurann Sakurann merged commit c151814 into main Apr 22, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

8 participants