openid / OpenID4VCI

64 stars 18 forks source link

Asymmetry between credential and deferred credential endpoints #167

Open nklomp opened 8 months ago

nklomp commented 8 months ago

Currently the credentials and deferred credentials endpoint are having a bit of an asymmetrical implementation.

The distinction between whether it is deferred or not is based on the HTTP response code 202 vs 200 and the response body of the credential endpoint. When deferred the transaction_id from the response needs to be used to post that value with the access token to the deferred endpoint, which in turn needs to be looked up from the metadata.

Then the deferred endpoint is returning the same response as the credential endpoint when the credential is available. If not available a 400 bad request with error issuance_pending. Given it is pretty normal to query the deferred endpoint whilst the credential is not available yet, it is odd to return an error for that. It also is not symmetrical with how the normal endpoint indicates that it will be a deferred flow by returning a 202. Returning a 202 when the issuance is still pending in the deferred endpoint makes sense. If you would go down that route the handling from a wallets perspective is the same for both endpoints.

However then it would also raise the question mentioned in #6 and #18 about the need for a separate endpoint for deferred credentials to begin with.

I see benefits from just having one endpoint for both direct and deferred issuance.

The drawback of course is that you actually have 2 different requests for the same endpoint. One being the initial request, the other being the deferred request with the transaction_id. Given the endpoints are not following REST conventions anyway this drawback is a non-issue if you ask me.

nklomp commented 8 months ago

BTW if regular and deferred issuance would be unified into one endpoint a similar argument could be made to remove the batch endpoint altogether. Simply always have an array in the request and responses. Only need one credential, simply have one element in the array. One of the distinctions of course is that you would not know up front whether the issuer supports batched credentials (more than one VC), but that could easily be remedied with a metadata option replacing current batch_endpoint url, or simply requiring that issuers should support issuing multiple credentials.

I do see some arguments against this as it is a bit more muddy, so not feeling strongly about merging batched credentials as well, but just putting it out here that it could all be unified into one endpoint

decentralgabe commented 7 months ago

Then the deferred endpoint is returning the same response as the credential endpoint when the credential is available. If not available a 400 bad request with error issuance_pending. Given it is pretty normal to query the deferred endpoint whilst the credential is not available yet, it is odd to return an error for that.

Agree 100%. Related to #226

I am also in favor of the proposal to condense the two endpoints into one, I believe it will simplify implementations and lighten the burden on both servers and clients alike.

Sakurann commented 7 months ago

I do see the benefit of merging deferred capability into credential endpoint.

Regarding the batch endpoint, deferred issuance capability should be merged into batch credential endpoint, as opposed to merging all three capabilities into one single endpoint. Currently batch endpoint uses the same syntax as the credential endpoint, but I would not expect it to stay this way because as proposed in issuance #93, there are some breaking changes needed for the batch issuance to add missing capabilities. so I would expect Credential Endpoint to stay simple and as-is for the interoperability purposes. (for the context, when taking VCI to the implementer's draft, WG agreed to there is a high chance breaking changes will be made to the batch endpoint but it is compensated by credential endpoint remaining unchanged.)

We should also introduce an issuer metadata parameter deferred_credential_issuance_supported (and deferred_batch_issuance_supported) or something for the issuers to indicate whether they support deferred capability or not, because I do not think all the issuers should be required to support deferred capability.

jogu commented 1 month ago

https://github.com/openid/OpenID4VCI/pull/364 has removed the batch credential endpoint (now that the normal credential endpoint can issue batches of a single credential there was no clear need for an endpoint that issued different datasets in a single request, and there were lots of unsolved problems with doing so so the working group agreed removing was the best way forward).

Hence closing this issue. Feel free to comment/reopen if I missed some aspect that's still applicable.

nklomp commented 1 month ago

I guess the title of this ticket was wrong. It mainly talks about the asymmetry between deferred endpoint and regular credential endpoint and then also discusses the batch endpoint to some extent. I will have a look whether the asymmetry is gone in the latest commits

jogu commented 1 month ago

Darn, I missed that, thanks - let's fix the title & reopen.

I think there are definitely some issues with deferred endpoint still.