-
Notifications
You must be signed in to change notification settings - Fork 47
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
Refactor presentation endpoints into exchange endpoints #258
Conversation
"description": "Data necessary to initiate the exchange." | ||
} | ||
- | ||
$ref: "#/components/schemas/NotifyPresentationAvailableRequest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept this here for the time being, we could probably keep it here -- but I do want to have a chat w/ the Traceability folks to understand why they're approaching the problem in this way. There might be a simpler way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is always a simpler way, but it must be defined in a schema to be accepted :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss on next VC API call -- it's not clear to me why the Traceability folks chose to use a VPR here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We chose to support VPR in the hopes that eventually browser and HTTP APIs would support the same request / respond query data model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VPR is supported in the response format for this call (so there would be no drop in support for VPR). What I'm wondering is why /POST/ a VPR. What you get in return is a VPR. So, with the traceability stuff (IIUC) you POST a VPR and then get a VPR in response.
description: internal error | ||
/presentations/submissions: | ||
description: Internal server error. | ||
/exchanges/{exchange-id}/{transaction-uuid}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msporny @mkhraisha -- I'm not sure if this is what you guys were talking about wrt. two IDs to track a particular exchange or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a fan of this ... I am pretty sure {transaction-uuid}
is a post body argument in our cases.
these are values that are inside VPs or VCs for the purposes of relating them (intentional correlation fields).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a fan of this ... I am pretty sure {transaction-uuid} is a post body argument in our cases.
This is not what I heard @mkhraisha and @mprorock request on the call. It was not entirely clear to me, but these were the ACTIONS that were taken on the last VC API call and what I implemented. This was the "We need to follow REST principles" argument that @mprorock was making throughout the call, which I agree with, so we really need to sort this out as it was this approach that enabled us to merge the presentation/workflow endpoints in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current endpoints support POST /presentations/available
and POST /presentations/submissions
...
It's not clear to me that adding another URL parameter does anything helpful wrt presentation exchange, given we don't need it currently.
Controller style endpoints are defined in the style guide we agreed to use.
https://restfulapi.net/resource-naming/
for example:
http://api.example.com/cart-management/users/{id}/cart/checkout
Since this PR is attempting to merge the existing endpoints with a new proposed one, I suggest only adding 1 id
at a time... and given the id
here is exchange-id
.. I don't understand the relation to the other IDs or the existing use cases for grouping presentations by unique identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me that adding another URL parameter does anything helpful wrt presentation exchange, given we don't need it currently.
See this comment for detail: https://github.com/w3c-ccg/vc-api/pull/258/files#r801205266
holder.yml
Outdated
schema: | ||
type: string | ||
minimum: 3 | ||
pattern: "[a-z0-9\\-]{3,}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very western-centric regex. I expect people to object to the fact we're not allowing non-western characters in here (in time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we encourage so much diversity in characters for an id that all parties need to agree too?
See json-schema-org/json-schema-spec#542
I think we should consider if this id
also shows up inside VPs and VCs and if so, is in a proper IRI or not.
I prefer for it to be urn:uuid:
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we encourage so much diversity in characters for an id that all parties need to agree too?
Not all parties need to agree to this identifier. It is (or can be) issuer-specific.
I prefer for it to be urn:uuid:
I don't understand this desire, so we should discuss on the next VC API call. In any case, my suggestion is that we deal w/ the range of exchange-id later.
@@ -555,21 +555,21 @@ <h4>Prove Presentation</h4> | |||
</section> | |||
|
|||
<section> | |||
<h4>Presentation Availability</h4> | |||
<h4>Initiate Exchange</h4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jandrieu I waffled back and forth on "Initiate Presentation Exchange" and what I ended up using above. Any ideas? Is this what you were going for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initiate exchange sounds pretty good to me
</section> | ||
|
||
<section> | ||
<h4>Submit Presentation</h4> | ||
<h4>Continue Exchange</h4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jandrieu same here -- "continue exchange" seems strange... maybe? I don't know. Any suggestions?
paths: | ||
components: | ||
schemas: | ||
ErrorResponse: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
finally! real and potentially useful errors! <3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If people like this, I'll apply it to all 4xx error codes. This standard object also enables us to define standard error code responses more specifically in the spec.
holder.yml
Outdated
the request, it returns a Verifiable Presentation Request. A request | ||
that the server cannot understand results in an error. | ||
parameters: | ||
- in: path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can define these one at the top and reuse them see:
https://github.com/dgarcia360/openapi-boilerplate/blob/master/src/parameters/path/petId.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 748d5be.
Gave up trying to correctly reference them using JSON Schemas completely broken referencing system after 30 minutes and referred to them directly. We have a broken pattern at the bottom of many of these files w/ relative references that we're going to want to clean up in a whole suite refactor. I'll do that once we have the basic shape of the APIs agreed to.
anyOf: | ||
- | ||
{ | ||
"type": "object", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not add this... since you just destroyed the ability to know the shape of the data... of course if you have a new shape you want to define I would be happy to see it defined and added but this harmful for interop as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not add this... since you just destroyed the ability to know the shape of the data... of course if you have a new shape you want to define I would be happy to see it defined and added but this harmful for interop as is.
If the server doesn't understand the POST data, they return a 400 error code (as described in the PR).
Accepting an arbitrary object with arbitrary fields is exactly what DB needs here (we have customers that require exchange types to be set up, where they take arbitrary JSON input for a defined exchange type, and then use that to drive VPR logic).
We discussed this on the last VC API call and there wasn't push back on it. You need to show up on the next VC API call and make your point (or clearly articulate it here). Please refer to the (very long and fruitful) discussion that you missed here: https://w3c-ccg.github.io/meetings/2022-02-01-vcapi/#topic-2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accepting an arbitrary object with arbitrary fields is exactly what DB needs here (we have customers that require exchange types to be set up, where they take arbitrary JSON input for a defined exchange type, and then use that to drive VPR logic).
I doubt very much that your customers like reading API docs that say "just give us any JSON you can imagine"...
However, I would bet they would be happy to see a schema defined that accepted some additional properties.
You can set additionalProperties: true
and still provide a useful API definition for the request format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt very much that your customers like reading API docs that say "just give us any JSON you can imagine"...
Clearly, that is not the case.
The case is this: The customers are the ones that define what is valid to send to a particular exchange endpoint... the /customer/ supplies the JSON Schema and then what to do with the input JSON.
So, to clarify for the endpoint: /exchanges/age-verification-training
, the client system posts {"retailerId": "abc-store-145", "clerkId": "423"}
to the endpoint, and the issuer server logic goes 1) does that match the JSON Schema, configured by the administrator, for that endpoint and 2) runs code that takes retailerId
and clerkId
as input, to produce a VPR that is specific to that retailer and clerk (request a specific employment credential).
Do you understand the use case? We can't limit the JSON Schema for our use case because the input set is unbounded for our servers (but the Traceability folks are free to limit the input set to what makes sense to them).
However, I would bet they would be happy to see a schema defined that accepted some additional properties.
No, they really don't want us to define any JSON Schema. They want to do it.
You can set
additionalProperties: true
and still provide a useful API definition for the request format.
Not really, see above.
content: | ||
application/json: | ||
schema: | ||
$ref: "#/components/schemas/NotifyPresentationAvailableResponse" | ||
$ref: "#/components/schemas/VerifiablePresentationRequest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
holder.yml
Outdated
required: true | ||
schema: | ||
type: string | ||
pattern: "([0-9a-f]{8}\b-[0-9a-f]{4}\b-[0-9a-f]{4}\b-[0-9a-f]{4}\b-[0-9a-f]{12}){0,1}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm might be better to make this an IRI same comment as before, it may show up in other JSON-LD id
fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a use case to understand what you're saying. I'm sure it's relevant, and yes, we need to be careful about things like this showing up in JSON-LD id
fields... that said, it's a UUID in a URL, which can fit in a JSON-LD id
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A. d2dfc100-9384-49e4-b547-5e7d84ec5bf5
-> not an IRI
B. urn:uuid:d2dfc100-9384-49e4-b547-5e7d84ec5bf5
-> IRI
C. https://app.example/d2dfc100-9384-49e4-b547-5e7d84ec5bf5
-> IRI
D. https://app.example/urn:uuid:d2dfc100-9384-49e4-b547-5e7d84ec5bf5
-> IRI
My understanding of JSON-LD is that only B-D would be acceptable for a credentialSubject.id
or issuer.id
or VerifiableCredential.id
or VerifiablePresentation.id
... does that match you understanding?
Given:
https://github.com/w3c/vc-data-model/blob/v1.1/contexts/credentials/v1#L6
https://github.com/w3c/vc-data-model/blob/v1.1/contexts/credentials/v1#L38
https://github.com/w3c/vc-data-model/blob/v1.1/contexts/credentials/v1#L43
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does that match you understanding?
Yes, that matches my understanding and I now understand what you are saying.
Based on the discussion during the telecon yesterday, I believe the group decided that we need to be very careful about this point. That is, if a server does not have control over what an id
or name
can be, we should not allow it to be a part of a path parameter (to avoid ugly URL encoding). I believe we agreed to that as a general rule and will need to determine how this fact affects how future path parameters are defined and used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking:
- remove the "any object" is a valid request part.
- remove the
transaction-uuid
part.
Nonblocking:
We should handle all id
values consistently... this is not a place to support diversity.
I suggest we make them all valid IRIs so that they can be safely embedded in JSON-LD.
I propose urn:uuid:
.
This was not an objection by the other two traceability cohort members, so I suggest you speak with them to see why they didn't object to this on the call. As explained in #258 (comment), DB requires this feature for its customers and product line. The reason it's an empty object is because our products enable arbitrary objects to be posted to start a flow (and we have server-side logic that does stuff with that arbitrary input -- for a particular exchange). Let me know where the Traceability folks want to go on this. DB will object to being blocked on both
This was specifically requested by two of the members in the Traceability cohort. I suggest you discuss this with them first and then come back with a group suggestion. DB requires
If we want to take a unified approach, DB will object to using only UUIDs everywhere. We will suggest being able to select between multibase character sets limited to 128-bit values OR UUIDs.
I could agree to "the entire URL should be a valid IRI", but I'll note that includes UTF-8 characters and some fairly hairy encoding/decoding rules. The rules at present ensure that the entire URL can be safely embedded in JSON-LD. Are you suggesting that we expand the scope of valid value range for all VC API endpoint URLs to include UTF-8 characters?
Confused by this, so we should discuss. |
I am good with this |
I am totally fine if the server says, "I accept JSON" and an empty JSON object is sent. It is up to the server to know what to do with it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not yet perfect, but it moves significantly in the right direction. I say we get it in and move things forward, opening issues if we need to address specific items. If we we in a WG I might be more inclined to get it "just right"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see note around diff between PUT and POST
description: internal error | ||
/presentations/submissions: | ||
description: Internal server error. | ||
/exchanges/{exchange-id}/{transaction-uuid}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So some clarification:
/exchanges/{exchange-id}/{transaction-uuid}
is good for PUT
because you already know the UUID of the object you are referrencing - on POST
you are creating an object and therefore don't have the ID yet, in this case of a certain type, e.g. /exchanges/{exchange-id}/
makes sense for POST, if exchange-id is referring to some type of exchange.
In the case of REST we SHOULD have the IDs for objects in both URI and in body
@msporny does that make sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This article on the topic may help clarify what I mean https://restfulapi.net/rest-put-vs-post/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following normal collection style naming convention, I would assume:
POST /exchanges -> create {id: 123}
PUT or PATCH /exchanges/{id} -> update an existing resource with the given id.
I don't know what transaction-uuid
is... and I suggest we not create it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mprorock wrote:
@msporny does that make sense
Yes, I think what you want is for POST /exchanges/{exchange-id}/{transaction-uuid}
to be changed to PUT /exchanges/{exchange-id}/{transaction-uuid}
... if so, I'm good with that.
However, it seems like @mprorock and @OR13 disagree? Still hard for me to tell.
To be clear, Digital Bazaar would like to be able to identify different types of exchanges at different endpoints (load balancing via URL instead of having to look into each message -- useful at scale). We would also like to be able to refer to individual instances of each exchange (again, load balancing via URL instead of having to look into each message -- also useful at scale). Remember that the age verification system does >40 million individual interactions per day, and the number of HTTP calls are multiples of that... so, not having to look into the payload body is of significant importance at scale.
Moving these things into the payload body doesn't allow us to use some of the faster load balancing frameworks out there that only have to look at the first few lines of the HTTP Headers instead of having to consume the entire message.
holder.yml
Outdated
A client can use this endpoint to continue the exchange of information | ||
associated with an initiated exchange. A Verifiable Presentation is | ||
sent to this endpoint. If the server has received all of the information | ||
it needs, a 200 OK is returned with either an empty response or a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bit of a nit, but your description is redundant to the description for the supported statuses.
You can keep the description focused on defining the operationId
and use the status to convey the different responses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OR13 wrote:
a bit of a nit, but your description is redundant to the description for the supported statuses.
Does this work for you? https://github.com/w3c-ccg/vc-api/pull/258/files#r801206879
description: Server is unwilling to risk processing a request that might be replayed | ||
"501": | ||
description: Not implemented | ||
description: Received data is malformed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this could ever be the case given https://github.com/w3c-ccg/vc-api/pull/258/files#r799493530
Implementers should be throwing 400 as soon as they have validated the request data according to a schema, per OWASP best practices...
https://cheatsheetseries.owasp.org/cheatsheets/Input_Validation_Cheat_Sheet.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OR13 wrote:
I don't see how this could ever be the case
Please see this for a more detailed explanation with a use case: https://github.com/w3c-ccg/vc-api/pull/258/files#r801201273
@msporny want to trim out the contentious bits so we can merge the good, agreed on stuff that is in here, and then I can PR separately around stateful stuff like getters/delete, etc ? |
What are the contentious bits? I need a list that both @OR13, @mprorock, @mkhraisha, and I can agree to. :) -- in the very worst case, we'll spend VC API time today discussing what, of this PR, is merge-able and has consensus. |
index.html
Outdated
<p> | ||
</p> | ||
|
||
<div class="api-detail" | ||
data-api-endpoint="post /presentations/submissions"></div> | ||
data-api-endpoint="post /exchanges/{exchange-id}/{transaction-uuid}"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this needs to be updated to the put route
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a172d81.
Probably better to merge and have @mprorock or @mkhraisha do a follow up PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approval contingent on follow up PRs to this
Merging based on conversations yesterday in the VC API group meeting: https://w3c-ccg.github.io/meetings/2022-02-08-vcapi/ |
This PR is based on ACTIONS requested to be taken on the last VC API call (see summary at top of transcription): https://w3c-ccg.github.io/meetings/2022-02-01-vcapi/
The goals here are to:
I've done the first of three items and will halt here for feedback from the Traceability work item group to ensure that this meets their expectations from the call two days ago.
The target merge window is in 7 days, after the VC API call next week.
Preview | Diff