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

added format to authorization details and respective format specific additional claims #219

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

tlodderstedt
Copy link
Collaborator

@tlodderstedt tlodderstedt commented Jan 19, 2024

resolves #217.

Copy link
Contributor

@paulbastian paulbastian left a comment

Choose a reason for hiding this comment

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

As I stated in the WG Call, I believe that credential_configuration_id is the better choice. In general, I have trouble understanding how OpenID4VCI works well without metadata, as the Wallet needs to known:

  • the credential endpoint
  • supported proof types
  • supported credentials and formats
  • display data for the issuer and the credential

Therefore I believe in productive environment it will be very common that the Issuer has the ability to host metadata.

Moreover there are two ways to start Auth Code flow:

  1. Credential Offer -> we need the credential_configuration_id anyway
  2. The Wallet has implicit knowledge/is pre-configured, in this case it needs to know the credential_issuer anyway and it is not a big difference between storing format/type and configuration_id

In the end, my argument is that its less options to support, which is generally better. However I see that this is becoming very similar to scope, which is an indicator that the concept is not ideal yet and we are missing more implementation experience.

Therefore, feel free to override my request for change, I just wanted to share my opinion.

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.

i think we need to figure out optional/must in the authorization request, but looks good otherwise.

@jogu
Copy link
Contributor

jogu commented Jan 22, 2024

As I stated in the WG Call, I believe that credential_configuration_id is the better choice. In general, I have trouble understanding how OpenID4VCI works well without metadata, as the Wallet needs to known:

  • the credential endpoint
  • supported proof types
  • supported credentials and formats
  • display data for the issuer and the credential

Therefore I believe in productive environment it will be very common that the Issuer has the ability to host metadata.

We had a fairly long conversation about this in one of last year's issues or PRs (likely the that one that first added configuration ids). As I understood it, the issue was not so much that people didn't want to have metadata at all - it was that they didn't want to be required to list every supported credential in the metadata. (There's parallels for this in OAuth, e.g. https://datatracker.ietf.org/doc/html/rfc8414#section-2 the AS isn't required to list every supported scope in scopes_supported.)

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

@awoie awoie left a comment

Choose a reason for hiding this comment

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

I'm not excited about this change. I agree with @paulbastian that metadata should be required, even if it was disseminated out-of-band. It would make things so much simpler. But I understand that people might have overlooked the implications in the previous PR that changed that assumption. I'm not blocking this PR, the content seems to be fine since it restores the previous state. I have just one suggestion below.

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
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.

I share some of the concerns of Paul (and the benefit of a reduction of complexity), but I agree that right now we probably have too little experience/feedback. So I guess the best approach at the current state of the draft is to keep it in and revisit later.
That being said, I do believe we should definitely revisit this sometime this year.

@Sakurann Sakurann dismissed paulbastian’s stale review January 25, 2024 19:13

per Paul's written and verbal comments

@Sakurann Sakurann merged commit f9a59ce into main Jan 25, 2024
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.

re-add an option to use format/type in authorization_details
7 participants