Merge Deferred Credential Endpoint text into Credential Endpoint #583
Conversation
| In subsequent (i.e Deferred) Credential Requests the Client instead includes the following parameter: | ||
|
|
||
| * `transaction_id`: REQUIRED. String identifying a Deferred Issuance transaction. |
There was a problem hiding this comment.
I don't think we had working group consensus to allow transaction_id to be sent to the normal credential endpoint, which seems to be what is now allowed?
I don't really mind whether we do this or not, but if we do allow transaction_id to be used at the credential endpoint then I think we have to remove the deferred credential endpoint entirely, as it wouldn't be adding anything useful that I can see and just introduces extra potential protocol flows that have to be implemented/tested.
There was a problem hiding this comment.
As it stands, the text does not allow returning it to the Credential Endpoint (due to the text when transaction_id is returned in the Credential Response requiring subsequent Credential Requests be made to the Deferred Credential Endpoint. So as it stands it is only editorial, as it just makes clear that Credential Requests/Responses are being made to both endpoints.
Obviously this also makes it trivial to remove Deferred Credential Endpoint if we so desire if there is consensus by updating that text to allow doing subsequent requests to the Credential Endpoint.
There was a problem hiding this comment.
Ah okay... I've suggested a couple of wording improvements based on that.
Overall I'm not convinced the current state is an improvement, currently it feels to me like the existing text is clearer, and it seems like we've changed normative behaviour in a few places (allowing interval to be omitted and allowing transaction_id to be changed during a deferred issuance), and if we're going this way we should probably retain an example of a deferred credential response.
There was a problem hiding this comment.
moved away from this, now there is never a difference in deferred Credential response
Co-authored-by: Andres Olave <sloops77@users.noreply.github.com>
| * `credentials`: OPTIONAL. Contains an array of one or more issued Credentials. It MUST NOT be used if the `transaction_id` parameter is present. The elements of the array MUST be objects. The number of elements in the `credentials` array matches the number of keys that the Wallet has provided via the `proofs` parameter of the Credential Request, unless the Issuer decides to issue fewer Credentials. Each key provided by the Wallet is used to bind to, at most, one Credential. This specification defines the following parameters to be used inside this object: | ||
| * `credential`: REQUIRED. Contains one issued Credential. The encoding of the Credential depends on the Credential Format and MAY be a string or an object. Credential Formats expressed as binary data MUST be base64url-encoded and returned as a string. More details are defined in the Credential Format Profiles in (#format-profiles). | ||
| * `transaction_id`: OPTIONAL. String identifying a Deferred Issuance transaction. This parameter is contained in the response if the Credential Issuer cannot immediately issue the Credential. The value is subsequently used to obtain the respective Credential with the Deferred Credential Endpoint (see (#deferred-credential-issuance)). It MUST not be used if the `credentials` parameter is present. It MUST be invalidated after the Credential for which it was meant has been obtained by the Wallet. | ||
| * `transaction_id`: OPTIONAL. String identifying a Deferred Issuance transaction. This parameter is contained in the response if the Credential Issuer cannot immediately issue the Credential. The value is subsequently used to obtain the respective Credential with the Deferred Credential Endpoint (see (#deferred-credential-issuance)). It MUST not be used if the `credentials` parameter is present. It MUST be invalidated after the Credential for which it was meant has been obtained by the Wallet. This MAY be omitted if the request contained `transaction_id`, in which case the Client should continue to use the previously obtained `transaction_id`. |
There was a problem hiding this comment.
I'm not sure I'm keen on introducing behaviour where the Credential Issuer can return a new transaction_id midway through a deferred issuance.
There was a problem hiding this comment.
fair enough, resolved by mandating it be unchanged.
| ``` | ||
|
|
||
| In the event the Credential Issuer can no longer issue the credential(s), the `credential_request_denied` error code as defined in (#credential-request-errors) should be used in response to a request. A wallet upon receiving this error SHOULD stop making requests to the deferred credential endpoint for the given `transaction_id`. | ||
| This endpoint is a specialized Credential Endpoint for handling Deferred Credential Requests. The Deferred Credential Endpoint follows all the requirements in {#credential-endpoint}. All Credential Requests MUST contain `transaction_id`. Support for this endpoint is OPTIONAL. |
There was a problem hiding this comment.
I think the "specialised Credential Endpoint" is part of what confused me - "specialized" I think can infer that normal credential endpoint can also be used for this case. Not sure what a better word is though.
There was a problem hiding this comment.
tried something better.
|
|
||
| The `c_nonce` value is retrieved from the Nonce Endpoint as defined in (#nonce-endpoint). | ||
|
|
||
| In subsequent (i.e Deferred) Credential Requests the Client instead includes the following parameter: |
There was a problem hiding this comment.
| In subsequent (i.e Deferred) Credential Requests the Client instead includes the following parameter: | |
| If Credential Issuance was deferred, the Wallet continues by making a request to the Deferred Credential Endpoint containing the following parameter: |
There was a problem hiding this comment.
updated to something equivalent (and the lead in to the previous section to specify Credential Endpoint rather than being more open.
| * `credential`: REQUIRED. Contains one issued Credential. The encoding of the Credential depends on the Credential Format and MAY be a string or an object. Credential Formats expressed as binary data MUST be base64url-encoded and returned as a string. More details are defined in the Credential Format Profiles in (#format-profiles). | ||
| * `transaction_id`: OPTIONAL. String identifying a Deferred Issuance transaction. This parameter is contained in the response if the Credential Issuer cannot immediately issue the Credential. The value is subsequently used to obtain the respective Credential with the Deferred Credential Endpoint (see (#deferred-credential-issuance)). It MUST not be used if the `credentials` parameter is present. It MUST be invalidated after the Credential for which it was meant has been obtained by the Wallet. | ||
| * `transaction_id`: OPTIONAL. String identifying a Deferred Issuance transaction. This parameter is contained in the response if the Credential Issuer cannot immediately issue the Credential. The value is subsequently used to obtain the respective Credential with the Deferred Credential Endpoint (see (#deferred-credential-issuance)). It MUST not be used if the `credentials` parameter is present. It MUST be invalidated after the Credential for which it was meant has been obtained by the Wallet. This MAY be omitted if the request contained `transaction_id`, in which case the Client should continue to use the previously obtained `transaction_id`. | ||
| * `interval`: REQUIRED if `transaction_id` is present. Contains a positive number that represents the minimum amount of time in seconds that the Wallet SHOULD wait after receiving the response before sending a new request to the Deferred Credential Endpoint. It MUST NOT be used if the `credentials` parameter is present. |
There was a problem hiding this comment.
We've (I think accidentally) changed interval in the deferred credential response to being optional.
There was a problem hiding this comment.
resolved by requiring transaction_id
There was a problem hiding this comment.
interval is required is transaction_id is present, what's wrong here?
| In subsequent (i.e Deferred) Credential Requests the Client instead includes the following parameter: | ||
|
|
||
| * `transaction_id`: REQUIRED. String identifying a Deferred Issuance transaction. |
There was a problem hiding this comment.
Ah okay... I've suggested a couple of wording improvements based on that.
Overall I'm not convinced the current state is an improvement, currently it feels to me like the existing text is clearer, and it seems like we've changed normative behaviour in a few places (allowing interval to be omitted and allowing transaction_id to be changed during a deferred issuance), and if we're going this way we should probably retain an example of a deferred credential response.
|
So, taking a step back here (and we can chat about this in the WG to make sure we are aligned): The goal of merging text here is to provide implementations certainty that the Responses from the Deferred Credential Endpoint and Credential Endpoints are the same, and requests are the same except in how they refer to what needs to be retrieved.This can lead to a simpler implementation as well as prevent mistakes where we update text for Credential Endpoint with a new feature, but not deferred. Right now, the only difference in the Credential Response and Deferred Credential Response is that transaction_id is absent for the continuing deferred case. My proposal is as follows:
|
|
If we don't agree that any response from Credential Endpoint should be valid for Deferred Credential Endpoint (and, similarly, any value from Request outside of the proofs should be present) then we should not be merging them (as they are not intended to be semantically the same). |
…al Endpoint unique roles
|
updated description and text to more accurately reflect the goals of this merge and why I believe it to be an improvement semantically over the current text. |
GarethCOliver
requested review from
awoie,
danielfett,
jogu,
paulbastian,
tlodderstedt and
tplooker
|
Discussed on today's WG call, @tplooker to review. |
Co-authored-by: Torsten Lodderstedt <torsten@lodderstedt.net>
| * `credential_response_encryption`: OPTIONAL. Object containing information for encrypting the Credential Response. If this request element is not present, the corresponding credential response returned is not encrypted. | ||
| * `jwk`: REQUIRED. Object containing a single public key as a JWK used for encrypting the Credential Response. | ||
| * `enc`: REQUIRED. JWE [@!RFC7516] `enc` algorithm [@!RFC7518] for encrypting Credential Responses. | ||
| * `zip`: OPTIONAL. JWE [@!RFC7516] `zip` algorithm [@!RFC7518] for compressing Credential Responses prior to encryption. If absent then compression MUST not be used. |
There was a problem hiding this comment.
why is this removed from here? this applies to non-deferred cases, too, right?
There was a problem hiding this comment.
It's in the section of common parameters. See line 935-938.
The structure of this section is:
parameters only for Credential Requests to the Credential Endpoint.
parameters only for Credential Requests to Deferred Credential Endpoint
parameters common to All Credential Requests.
| ``` | ||
|
|
||
| In the event the Credential Issuer can no longer issue the credential(s), the `credential_request_denied` error code as defined in (#credential-request-errors) should be used in response to a request. A wallet upon receiving this error SHOULD stop making requests to the deferred credential endpoint for the given `transaction_id`. | ||
| The Deferred Credential Endpoint is used to handle deferred Credential Issuance. The Deferred Credential Endpoint follows the requirements in {#credential-endpoint}. Support for this endpoint is OPTIONAL. |
There was a problem hiding this comment.
Is support for the deferred credential endpoint optional for both the wallet and the issuer or only the issuer?
There was a problem hiding this comment.
I guess if Wallets don't support it, they can not successfully retrieve credentials that are deferred. I don't know what requirements we make around wallets in VCI, but I'd mandate it for wallets in HAIP.
|
With the changes in this PR I question why there is a need to have a separate endpoint for deferred issuance. There is so much duplication that it feels like the credential endpoint can do both jobs just by checking whether the payload has a Getting rid of deferred endpoint altogether, or at least making it an optional extra (e.g. use the credential endpoint for deferred issuance unless the metadata specifies a deferred credential endpoint). Instead of having everything described in a Also I noticed that |
|
I am not a fan of structure that results from these changes. I think it makes it harder to implement The definition of the request parameter used in I also agree with @charsleysa, that it would be a lot cleaner to combine the endpoints completely |
|
@charsleysa I personally feel the same way, but making such a change at this time seemed like more than people wanted to take on. If there is support from the WG to make that change it is trivial to do so. What you noted here is basically the motivation for starting this PR. I generally find that in practice having two things that are meant to have the same functionality but are subtly different results in practical inter-op issues. I could update the structure even as-is by adding some sections for clarity, if it helps readability: (Common? Base? Shared) Credential Request (Parameters?)
Credential Request (Initial Credential Request?)Deferred Credential Request
Credential Response
Oh, Encrypted Messages should have one less #, it's not meant to be in the Deferred Credential Endpoint but a section of its own (as it applies to both, in the same manner). |
There was a problem hiding this comment.
I appreciate the effort behind this PR and the motivation to improve the spec. If it would result in less code and a more maintainable structure, that would be a great outcome. That said, I'm not entirely sure this is necessarily the case here.
First of all, it's not entirely clear whether, with the current proposed changes, there's an unintended side effect where the deferred endpoint could be used to send a regular credential request without a transaction_id, since the text simply states that the endpoint follows the requirements of the regular credential endpoint.
From an editorial perspective, I find some of the structural changes make the document harder to follow:
- I would expect all parameters for the credential request sent to the credential endpoint to be listed together at the start of the credential request section.
- The "common parameters" section feels quite hidden, as it appears only after all the examples.
- It's harder to follow that transaction_id can be provided in the credential request without first introducing the fact that the credential response (in deferred issuance) can contain a transaction_id.
- There is a broken reference in the deferred endpoint section to the {credential-endpoint} section.
- ...
Personally, I'd be more inclined to merge the credential and deferred credential endpoints, as others have suggested, to keep the flow cohesive and reduce duplication.
|
I agree that merging the 2 endpoints would probably be a lot cleaner spec-wise, but it seems too late in the process for that. Not sure how to best proceed with this tbh. |
paulbastian
left a comment
paulbastian
left a comment
There was a problem hiding this comment.
I have to admit that this reshuffeling of sections seems confusing to me. If we want to simplify this, then we should merge both endpoints together. Also, it is even possible with the current draft to run both endpoints over the same URL.
|
Discussed on today's WG call - given the recent comments it seems we have a consensus not to move forward with this one and to close. Gareth to open a new PR that unifies the deferred/non-deferred responses. |
Resolves #523
The goal of this is to change the semantics of how requests to and from Credential Endpoint and Deferred Credential Endpoint work.
Today: Credential Endpoint takes Credential Requests and returns Credential Responses, while Deferred Credential Endpoint takes Deferred Credential Requests (which references some parameters from Credential Request) and return Deferred Credential Responses (which references some parameters from Credential Responses).
After this change: Credential Endpoint and Deferred Credential Endpoint take Credential Requests and return Credential Responses.
transaction_id.This results in one normative change:
transaction_id, unchanged from the request. This does not result in a behavioral change, and aligns what is returned from Credential Endpoint and Deferred Credential Endpoint.The intended benefits of this semantic change are as follows: