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

Refactor presentation endpoints into exchange endpoints #258

Merged
merged 7 commits into from
Feb 9, 2022
Merged

Refactor presentation endpoints into exchange endpoints #258

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1680068
Add ErrorResponse component.
msporny Feb 4, 2022
86fcb9e
Rename presentation endpoints.
msporny Feb 4, 2022
fdf675e
Refactor endpoints from presentation -> exchange.
msporny Feb 4, 2022
748d5be
Define exchange-id and transaction-uuid path parameters in separate f…
msporny Feb 4, 2022
83e0bf2
Refactor continue exchange endpoint description.
msporny Feb 9, 2022
a172d81
Update POST to PUT in main spec for continue exchange endpoint.
msporny Feb 9, 2022
fc195cd
Change POST to PUT for continue exchange endpoint yaml.
msporny Feb 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions components/ErrorResponse.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
openapi: 3.0.0
info:
version: "0.0.3-unstable"
title: VC HTTP API
description: This is an Experimental Open API Specification for the [VC Data Model](https://www.w3.org/TR/vc-data-model/).
license:
name: W3C Software and Document License
url: http://www.w3.org/Consortium/Legal/copyright-software.
contact:
name: GitHub Source Code
url: https://github.com/w3c-ccg/vc-api
paths:
components:
schemas:
ErrorResponse:
Copy link
Contributor

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

Copy link
Contributor Author

@msporny msporny Feb 4, 2022 •
edited
Loading

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.

type: object
description: A response that denotes that an error has occurred.
properties:
id:
type: "string"
pattern: "[a-z0-9\\-]{8,}"
message:
type: "string"
minLength: 10
maxLength: 100
details:
type: "object"
required: ["id", "message"]
example:
{
"id": "invalid-json",
"message": "The provided JSON data was malformed."
}
8 changes: 8 additions & 0 deletions components/parameters/path/ExchangeId.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
name: exchange-id
description: A potentially human-readable identifier for an exchange.
in: path
required: true
schema:
type: string
minimum: 3
pattern: "[a-z0-9][a-z0-9\\-]{2,}"
6 changes: 6 additions & 0 deletions components/parameters/path/TransactionUuid.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
in: path
name: transaction-uuid
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}"
100 changes: 68 additions & 32 deletions holder.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,60 +56,90 @@ paths:
description: invalid input!
"500":
description: error!
/presentations/available:
/exchanges/{exchange-id}:
post:
summary: Notifies a holder of an available presentation.
operationId: notifyPresentationAvailable
summary: Initiates an exchange of information.
operationId: initiateExchange
description:
This API is the first part of a holder to holder credential exchange over http.
Notifies this server-holder that the client-holder (caller) is prepared to deliver a presentation compliant with the provided Verifiable Presentation Request query. This is the first part of a presentation exchange over this HTTP API. This server-holder MUST return the required 'domain' and 'challenge' parameters for the Presentation. This server-holder MAY reply with an alternate Verifiable Presentation Request query if it requires something different than the one proposed by the client-holder. The client-holder MUST return a Presentation in accordance with the response parameters.
A client can use this endpoint to initiate an exchange of a particular
type. The client can include HTTP POST information related to the
details of exchange it would like to initiate. If the server understands
the request, it returns a Verifiable Presentation Request. A request
that the server cannot understand results in an error.
parameters:
- $ref: "./components/parameters/path/ExchangeId.yml"
requestBody:
description: Details the client provides to the server to help it decide if the presentation should be made.
description:
Information related to the type of exchange the client would like
to start.
content:
application/json:
schema:
$ref: "#/components/schemas/NotifyPresentationAvailableRequest"
anyOf:
-
{
"type": "object",
Copy link
Contributor

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.

Copy link
Contributor Author

@msporny msporny Feb 4, 2022 •
edited
Loading

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

Copy link
Contributor

@OR13 OR13 Feb 7, 2022 •
edited
Loading

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.

Copy link
Contributor Author

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.

"description": "Data necessary to initiate the exchange."
}
-
$ref: "#/components/schemas/NotifyPresentationAvailableRequest"
Copy link
Contributor Author

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.

Copy link
Contributor

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 :)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

responses:
"200":
description: Proceed with presentation
description: Proceed with exchange.
content:
application/json:
schema:
$ref: "#/components/schemas/NotifyPresentationAvailableResponse"
$ref: "#/components/schemas/VerifiablePresentationRequest"
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

"400":
description: Request for presentation is malformed
description: Request is malformed.
content:
application/json:
schema:
$ref: "#/components/schemas/ErrorResponse"
"501":
description: Not implemented
description: Service not implemented.
"500":
description: internal error
/presentations/submissions:
description: Internal server error.
/exchanges/{exchange-id}/{transaction-uuid}:
Copy link
Contributor Author

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?

Copy link
Contributor

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).

Copy link
Contributor Author

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.

Copy link
Contributor

@OR13 OR13 Feb 7, 2022 •
edited
Loading

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.

Copy link
Contributor Author

@msporny msporny Feb 8, 2022 •
edited
Loading

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

Copy link
Contributor

@mprorock mprorock Feb 7, 2022 •
edited
Loading

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

Copy link
Contributor

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/

Copy link
Contributor

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.

Copy link
Contributor Author

@msporny msporny Feb 8, 2022 •
edited
Loading

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.

post:
msporny marked this conversation as resolved.
Show resolved Hide resolved
summary: Provide a presentation for a holder to store.
operationId: storePresentation
description: "Delivery endpoint for a client-holder to provide a presentation in compliance with the Verifiable Presentation Request provided in the Notification call. This server-holder MUST match the 'domain' and 'challenge' to a 'domain' and 'challenge' previously sent in a Notification Response, and not already received. The server-holder MUST verify the Presentation. The server MUST store the Presentation if verification passes."
summary: Receives information related to an existing exchange.
operationId: receiveExchangeData
description:
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
Copy link
Contributor

@OR13 OR13 Feb 7, 2022 •
edited
Loading

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.

Copy link
Contributor Author

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

Verifiable Presentation containing requested credentials by the client.
If the server requires more information, it returns a Verifiable
Presentation Request. A request that the server cannot understand
results in an error.
msporny marked this conversation as resolved.
Show resolved Hide resolved
parameters:
- $ref: "./components/parameters/path/ExchangeId.yml"
- $ref: "./components/parameters/path/TransactionUuid.yml"
requestBody:
description: Details the client provides to the server to help it decide if the presentation should be made.
description:
A Verifiable Presentation.
content:
application/json:
schema:
$ref: "#/components/schemas/StorePresentationRequest"
$ref: "#/components/schemas/VerifiablePresentation"
responses:
"202":
description: Presentation accepted
"200":
description: Received data was accepted.
content:
application/json:
schema:
$ref: "#/components/schemas/VerifiablePresentationRequest"
"400":
description: Presentation is malformed
"401":
description: Presentation did not contain a proof
"402":
description: Payment required
"403":
description: Presentation verification failed
"425":
description: Server is unwilling to risk processing a request that might be replayed
"501":
description: Not implemented
description: Received data is malformed.
Copy link
Contributor

@OR13 OR13 Feb 7, 2022 •
edited
Loading

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

Copy link
Contributor Author

@msporny msporny Feb 8, 2022 •
edited
Loading

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

content:
application/json:
schema:
$ref: "#/components/schemas/ErrorResponse"
"500":
description: internal error
description: Internal server error.
"501":
description: Service not implemented.
components:
schemas:
DeriveCredentialRequest:
Expand Down Expand Up @@ -213,5 +243,11 @@ components:
"domain": "jobs.example.com",
"challenge": "3182bdea-63d9-11ea-b6de-3b7c1404d57f",
}
ErrorResponse:
$ref: "./components/ErrorResponse.yml#/components/schemas/ErrorResponse"
StorePresentationRequest:
$ref: "./components/VerifiablePresentation.yml#/components/schemas/VerifiablePresentation"
VerifiablePresentation:
$ref: "./components/VerifiablePresentation.yml#/components/schemas/VerifiablePresentation"
VerifiablePresentationRequest:
$ref: "./components/VerifiablePresentationRequest.yml#/components/schemas/VerifiablePresentationRequest"
8 changes: 4 additions & 4 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,21 +555,21 @@ <h4>Prove Presentation</h4>
</section>

<section>
<h4>Presentation Availability</h4>
<h4>Initiate Exchange</h4>
Copy link
Contributor Author

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?

Copy link
Collaborator

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

<p>
</p>

<div class="api-detail"
data-api-endpoint="post /presentations/available"></div>
data-api-endpoint="post /exchanges/{exchange-id}"></div>
</section>

<section>
<h4>Submit Presentation</h4>
<h4>Continue Exchange</h4>
Copy link
Contributor Author

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?

<p>
</p>

<div class="api-detail"
data-api-endpoint="post /presentations/submissions"></div>
data-api-endpoint="post /exchanges/{exchange-id}/{transaction-uuid}"></div>
Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a172d81.

</section>

</section>
Expand Down