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

Rework W3C DC API examples #254

Merged
merged 15 commits into from
Oct 23, 2024
Merged

Rework W3C DC API examples #254

merged 15 commits into from
Oct 23, 2024

Conversation

marcoscaceres
Copy link
Contributor

@marcoscaceres marcoscaceres commented Sep 10, 2024

As the W3C spec is changing a lot, it's best to not include any examples yet. That way, this spec can progress without locking in changes on the W3C side.

@jogu
Copy link
Collaborator

jogu commented Sep 10, 2024

Are we expecting breaking changes to the Browser API, I thought it was fairly settled for now?

I'm not sure that removing the examples helps. The examples are non-normative and don't really show anything that's not already described in the normative text.

(We can actually update the examples relatively easily, both during any public review period for the 1.0 spec revision, and even after 1.0 is published by making an errata update, but if the normative text is likely to require changes as well that's a much bigger problem.)

@jogu
Copy link
Collaborator

jogu commented Sep 10, 2024

Ah, I guess WICG/digital-credentials#165 may be the driver?

Although the 'providers' -> 'requests' change itself looks like it shouldn't break normative text in VP, It is slightly concerning that there are suggestions on that PR that would affect the normative text, like changing 'request' to 'query'.

@samuelgoto
Copy link
Contributor

I'm concerned about this here: WICG/digital-credentials#164

@RByers
Copy link

RByers commented Sep 10, 2024

Maybe leaving a link for where to see current examples would be better than just removing them? Eg. maybe they belong in this GitHub repo but in their own non-normative file for easier updating?

@marcoscaceres
Copy link
Contributor Author

I don't think we should lock in this spec with the other at all. We might want to move things around and we haven't yet added the examples to the Digital Credentials Spec. Rest assured that we will add examples, but it might not happen in the accelerated timeframe OpenID4VP is currently on. We can add examples to a future version of OpenID4VP once the W3C Spec is a) in a working group, and b) we are confident on the design (e.g. the spec reaches CR on the W3C side). We don't have enough implementation experience yet to be confident we won't change more things - specially as the W3C spec hasn't been road tested with more request formats, which may change the underlying request model/structure.

openid-4-verifiable-presentations-1_0.md Outdated Show resolved Hide resolved
// Handle errors and/or fallback to other invocation mechanisms
}
```
The W3C Digital Credentials API [@!w3c.digital_credentials_api] contains an OpenID4VP Authorization Request, where every OpenID4VP Authorization Request parameter is represented as a top-level JavaScript object member.
Copy link
Collaborator

Choose a reason for hiding this comment

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

where every OpenID4VP Authorization Request parameter is represented as a top-level JavaScript object member

Is this a new development in Digital Credentials API? How would this work for signed requests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The change here is removing "request member", I think that's okay, it's removing the coupling between the two specs but it doesn't change how signed requests are handled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

is there any risk of this changing in the future? wouldn't this prevent passing multiple requests in an array? Would really prefer a text like The Verifier MAY send all the OpenID4VP request parameters to the W3C Digital Credentials API as defined in [@!w3c.digital_credentials_api]

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 best to say less. We don't know what the final API will look like.

@@ -1591,7 +1568,7 @@ Any OpenID4VP request compliant to this section of this specification can be use

### Unsigned Request {#unsigned_request}

The Verifier MAY send all the OpenID4VP request parameters as members in the `request` member passed to the API. In this case, the Wallet will use the Verifier's origin as asserted by the user agent as the Verifer's Client Identifier.
The Verifier MAY send all the OpenID4VP request parameters as members to the W3C Digital Credentials API [@!w3c.digital_credentials_api]. In this case, the Wallet will use the Verifier's origin as asserted by the user agent as the Verifer's Client Identifier.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, how does this work for signed request where all the parameters are within a signed JWT which contains all of the parameters?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is in the "Unsigned request" section, this text won't affect signed requests.

Copy link
Collaborator

@Sakurann Sakurann Sep 26, 2024

Choose a reason for hiding this comment

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

i would prefer something like this:

Suggested change
The Verifier MAY send all the OpenID4VP request parameters as members to the W3C Digital Credentials API [@!w3c.digital_credentials_api]. In this case, the Wallet will use the Verifier's origin as asserted by the user agent as the Verifer's Client Identifier.
The Verifier MAY send all the OpenID4VP request parameters to the W3C Digital Credentials API as defined in [@!w3c.digital_credentials_api]. In this case, the Wallet will use the Verifier's origin as asserted by the user agent as the Verifer's Client Identifier.

@Sakurann
Copy link
Collaborator

Text in this PR does not sufficiently decouple openid4vp text and still makes it susceptible to any future breaking changes in the credentials API.

"Put openid4vp request parameters in an appropriate place as defined in digital credentials API; the openid4vp parameter is X for signed requests and Y for unsigned requests" is what I would expect.

Also very unsure about removing all of the examples. Will digital credentials API provide examples how to include openid4vp request there once it is stable? If not, we should keep some examples as of today in the spec.

If the text is generic enough, changing the examples later as digital credentials API evolves is possible.

@jogu jogu added this to the Final 1.0 milestone Sep 24, 2024
openid-4-verifiable-presentations-1_0.md Outdated Show resolved Hide resolved
@marcoscaceres marcoscaceres force-pushed the marcoscaceres-patch-1 branch from a3ed327 to 3b0d32c Compare October 4, 2024 05:13
Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
Copy link
Collaborator

@jogu jogu left a comment

Choose a reason for hiding this comment

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

Thanks Marcos! Some small comments, could you update the PR title/first comment to reflect what is changing now too please?

openid-4-verifiable-presentations-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-presentations-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-presentations-1_0.md Outdated Show resolved Hide resolved
marcoscaceres and others added 3 commits October 15, 2024 09:09
Co-authored-by: Joseph Heenan <joseph@authlete.com>
Co-authored-by: Joseph Heenan <joseph@authlete.com>
Copy link
Contributor

@samuelgoto samuelgoto left a comment

Choose a reason for hiding this comment

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

LGTM++

This would make it easier to decouple the OpenID4VP spec and the Digital Credentials API spec.

openid-4-verifiable-presentations-1_0.md Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Contributor Author

Hopefully better now... did this in a hurry at the ARF event last week.

marcoscaceres and others added 4 commits October 22, 2024 13:56
Co-authored-by: Giuseppe De Marco <giuseppe.demarco@teamdigitale.governo.it>
Co-authored-by: Giuseppe De Marco <giuseppe.demarco@teamdigitale.governo.it>
Co-authored-by: Giuseppe De Marco <giuseppe.demarco@teamdigitale.governo.it>
Co-authored-by: Giuseppe De Marco <giuseppe.demarco@teamdigitale.governo.it>
@Sakurann Sakurann changed the title Remove W3C DC API examples Rework W3C DC API examples Oct 22, 2024
@@ -14,11 +14,7 @@ participant "App Platform" as ap
participant "Wallet" as w

u --> r : use
activate r

r -> wp: navigator.identity.get(\nprotocol="openid4vp",\nrequest="client_id, \nrequest_uri, request_uri_method=post, \npresentation_definition")
Copy link
Collaborator

Choose a reason for hiding this comment

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

so isn't this better be something like

r -> wp: send OpenID4VP request over the W3C Digital Credentials API

@@ -14,11 +14,7 @@ participant "App Platform" as ap
participant "Wallet" as w

u --> r : use
activate r

r -> wp: navigator.identity.get(\nprotocol="openid4vp",\nrequest="client_id, \nrequest_uri, request_uri_method=post, \npresentation_definition")
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above. shouldn't this better be something like

r -> wp: send OpenID4VP request over the W3C Digital Credentials API

@@ -19,7 +19,6 @@ activate r
note over r,wp
Note that the signed request object contains the Verifier's origin.
end note
r -> wp: navigator.identity.get(\nprotocol="openid4vp",\nrequest="client_id, request")
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above. shouldn't this better be something like

r -> wp: send OpenID4VP request over the W3C Digital Credentials API

As agreed on WG call

Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
response_mode: "w3c_dc_api",
nonce: "n-0S6_WzA2Mj",
client_metadata: {...},
presentation_definition: {...}
Copy link
Member

@bc-pi bc-pi Oct 22, 2024

Choose a reason for hiding this comment

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

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.

approved per WG discussion yesterday

@Sakurann Sakurann merged commit d979ac6 into main Oct 23, 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.

8 participants