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.

Sign up for GitHub

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

Jump to bottom

Clarify language around opening Credential Offer Endpoint #380

Merged
merged 6 commits into from
Oct 9, 2024
Merged

Clarify language around opening Credential Offer Endpoint #380

merged 6 commits into from
Oct 9, 2024

Conversation

jogu
Copy link
Contributor

@jogu jogu commented Aug 25, 2024 •
edited
Loading

As per consensus on Oct-10-2022 working group call, the credential offer endpoint must be redirected to in order to allow the wallet to have a user interaction.

The languaged used is the same as used in RFC6749, but with resource owner changed to End-User for consistency with rest of VCI spec.

closes #13

closes #201

@jogu
Clarify language around opening Credential Offer Endpoint
899c512 
As per consensus on Oct-10-2022 working group call, the credential offer
endpoint must be redirected to in order to allow the wallet to have a user
interaction.

The languaged used is the same as used in RFC6749.

closes #13
c2bo
c2bo reviewed Sep 4, 2024
View reviewed changes
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.

While "resource owner's user agent" might be technically correct, it sounds a bit off to me - especially given, that we have not used the term resource owner once in the whole spec so far.

bc-pi
bc-pi requested changes Sep 4, 2024
View reviewed changes
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.

minor but legit nit from @c2bo that should be fixed

@paulbastian
Copy link
Contributor

Could we move up the last sentence from this section ("The Credential Issuer MAY render a QR code containing the Credential Offer that can be scanned by the End-User using a Wallet, or a link that the End-User can click.") Reading this very common option in the last sentence, seems weird.

jogu and others added 2 commits September 10, 2024 14:55
@jogu @bc-pi
resource owner -> End-User
02adeb4 
As per Christian's comment on PR and Brian's suggestion on how to fix it.

Co-authored-by: Brian Campbell <71398439+bc-pi@users.noreply.github.com>
@jogu
Reorder credential offer section slightly
03fadd4 
As per Paul's suggestion:

#380 (comment)
@jogu
Copy link
Contributor Author

jogu commented Sep 10, 2024

Could we move up the last sentence from this section ("The Credential Issuer MAY render a QR code containing the Credential Offer that can be scanned by the End-User using a Wallet, or a link that the End-User can click.") Reading this very common option in the last sentence, seems weird.

Yeah, I think that helps, I've done that.

@jogu
Copy link
Contributor Author

jogu commented Sep 10, 2024

This is ready for re-reviews please :)

Sakurann
Sakurann requested changes Sep 10, 2024
View reviewed changes
Copy link
Collaborator

@Sakurann Sakurann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

the current text feels pretty wrong to me. we should also not preclude a future option to send a credential offer using browser api

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
@Sakurann Sakurann requested review from bc-pi and c2bo September 10, 2024 14:26
@paulbastian
Copy link
Contributor

This change just moved the existing line up to the top of the section. The underlying problem is that OpenID4VCI is missing a Wallet Invocation section like OpenID4VP, that could be linked here, I believe.

@jogu jogu force-pushed the clarify-credential-offer branch from ac575eb to 30c0bca Compare September 10, 2024 16:19
@jogu
Copy link
Contributor Author

jogu commented Sep 10, 2024

I've had another go based on Kristina's feedback and Paul's suggestion of having similar text to the wallet invocation section in VP. Please re-review!

Sakurann
Sakurann approved these changes Sep 10, 2024
View reviewed changes
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
paulbastian
paulbastian reviewed Sep 10, 2024
View reviewed changes
openid-4-verifiable-credential-issuance-1_0.md Outdated
@@ -333,7 +333,7 @@ This endpoint is used by a Credential Issuer that is already interacting with an

## Credential Offer {#credential-offer}

The Credential Issuer sends Credential Offer using an HTTP GET request or an HTTP redirect to the Wallet's Credential Offer Endpoint defined in (#client-metadata).
The Credential Issuer makes a Credential Offer by allowing the End-User to invoke the Wallet using the Wallet's Credential Offer Endpoint defined in (#client-metadata) (for example by clicking a link) and/or rendering a QR code containing the Credential Offer that the End-User can scan in a wallet or an arbitrary camera application.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about custom url schemes as clickable links?

Copy link
Contributor Author

@jogu jogu Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In that case the "Wallet's Credential Offer Endpoint" is a custom scheme url, I think that case is covered?

@jogu jogu mentioned this pull request Sep 16, 2024
Closed
@jogu jogu mentioned this pull request Sep 27, 2024
Closed
@jogu @Sakurann
Apply Kristina's suggestion
2b1fdf4 
Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
@jogu
Copy link
Contributor Author

jogu commented Oct 9, 2024

4 approvals, open for over a month and mentioned on yesterday's WG call (and probably other WG calls too) without any objections being raised - merging - thanks everyone!

@jogu jogu merged commit 58ea726 into main Oct 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulbastian paulbastian paulbastian left review comments

@bc-pi bc-pi bc-pi approved these changes

@nemqe nemqe nemqe approved these changes

@c2bo c2bo c2bo approved these changes

@Sakurann Sakurann Sakurann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a section of Wallet invocation credential offer - successful & error responses need to be defined
6 participants
@jogu @paulbastian @nemqe @c2bo @Sakurann @bc-pi