Skip to content

Conversation

@aaronpk
Copy link
Contributor

@aaronpk aaronpk commented Mar 21, 2025

This is to help avoid the confusion encountered in #376 by avoiding the use of the term "scheme" which implies that the client ID value is a URL. Using the term "prefix" avoids this association while still being descriptive.

Copy link
Member

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

"Prefix" is better.

@Sakurann Sakurann added this to the Final 1.0 milestone Mar 21, 2025
@c2bo
Copy link
Member

c2bo commented Mar 21, 2025

Agreed, prefix is probably a better term.

I just did a quick glance through the changes and noticed one part that needs to be adjusted: line 2239

The client_id parameter MUST be omitted in unsigned requests defined in (#unsigned_request). The Wallet determines the effective Client Identifier from the Origin. The effective Client Identifier is composed of a synthetic Client Identifier Scheme of web-origin and the Origin itself. For example, an Origin of https://verifier.example.com would result in an effective Client Identifier of web-origin:https://verifier.example.com. The transport of the request and Origin to the Wallet is platform-specific and is out of scope of OpenID4VP over the W3C Digital Credentials API. The Wallet MUST ignore any client_id parameter that is present in an unsigned request.

@jogu
Copy link
Collaborator

jogu commented Mar 24, 2025

Thanks for doing this Aaron, it's appreciated! Unfortunately I'm pretty sure this will conflict with #448 :( It would be good to merge that first if possible - I think it just needs one more approval before we'd be ready to merge it. (The line Christian mentions in his comment above is modified by 448 too.)

@aaronpk
Copy link
Contributor Author

aaronpk commented Mar 24, 2025

I will gladly update this PR after #448 is merged

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

chair hat off

@Sakurann
Copy link
Collaborator

chair hat on. this is a pretty big breaking change. @aaronpk can we please discuss these kind of changes/proposals in the WG before doing a PR? thank you.

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

thinking more about this. this might be a better name, but I want implementer feedback first before we introduce these kind of breaking changes (in the metadata and error codes, but still) for the aesthetics reasons, at this point in time.

@jogu
Copy link
Collaborator

jogu commented Mar 24, 2025

@aaronpk #448 is merged now if you'd like to update this one to resolve the conflicts.

@Sakurann

thinking more about this. this might be a better name, but I want implementer feedback first before we introduce these kind of breaking changes (in the metadata and error codes, but still) for the aesthetics reasons, at this point in time.

I agree with getting implementer feedback, but I think the impact is fairly limited - wallet metadata isn't used much, and the change in the error is in the error_description field which isn't intended to be machine readable. (The error code remains invalid_request unless I missed a change somewhere.)

aaronpk added 2 commits March 26, 2025 09:54
# Conflicts:
#	openid-4-verifiable-presentations-1_0.md
@aaronpk
Copy link
Contributor Author

aaronpk commented Mar 26, 2025

chair hat on. this is a pretty big breaking change. @aaronpk can we please discuss these kind of changes/proposals in the WG before doing a PR? thank you.

It seemed easier to submit the PR to start the discussion so folks have a more concrete proposal to work from. It's almost entirely an editorial change. The only actual protocol change is changing the metadata parameter from client_id_schemes_supported to client_id_prefixes_supported. The other place the term appears on the wire is in the error_description which should not be being read by machines anyway.

@aaronpk
Copy link
Contributor Author

aaronpk commented Mar 26, 2025

I updated this PR and resolved the conflicts from #448

Copy link

@babisRoutis babisRoutis left a comment

Choose a reason for hiding this comment

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

Prefix is a better term and changes (to implementations) are limited to the wallet metatada, used mostly - I guess - conditionally, in case of a request_uri_method equal to post.

LGTM

aaronpk and others added 2 commits April 1, 2025 16:38
Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
Co-authored-by: Brian Campbell <71398439+bc-pi@users.noreply.github.com>
Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

approving since it looks like the benefits of this naming change outweights my worrys about a small breaking change.

@Sakurann Sakurann requested a review from lj-raidiam April 2, 2025 21:04
@Sakurann
Copy link
Collaborator

Sakurann commented Apr 3, 2025

@c2bo to open issues in VCI and HAIP to fix terminology there

@jogu
Copy link
Collaborator

jogu commented Apr 3, 2025

Merging this as agree on today's WG call - and as agreed I'll update #493 to move the changelog entry to -26.

@jogu jogu merged commit 0895a6e into openid:main Apr 3, 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.

10 participants