Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
remove
c_nonce
from the token endpoint response #381remove
c_nonce
from the token endpoint response #381Changes from 7 commits
ce28a10
074065b
48a3678
58bbf81
0e0d188
b7b4c08
aeb3839
aadb2bb
55c109e
f7b4dc8
55c8388
9e0d059
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Wouldn't a POST request be more appropriate? I mean the endpoint is supposed to provide a new/different value with every response and the result should not be cached (in my opinion).
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.
+1, I tried to push in this direction as well: #381 (comment)
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 didn't realize this was GET. my suggestion has been POST. In german wallet project we have been prototyping with POST and it works well https://bmi.usercontent.opencode.de/eudi-wallet/eidas-2.0-architekturkonzept/flows/PID-IssuerSigned-cloud/#issuer-session-endpoint-at-the-pid-provider
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.
Other than describing the media type of content that doesn't exist as JSON, which is semantically invalid and maybe syntactically wrong too, what value does the German wallet project get from using POST?
=== Screenshot of the previously cited german wallet project ===
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.
No.
That's why it was made not cacheable in the example https://github.com/openid/OpenID4VCI/pull/381/files#diff-1f424614b35a9899813079f1b1f6218631a2aedd993368ccb89bb81a9eda0289R741 and text https://github.com/openid/OpenID4VCI/pull/381/files#diff-1f424614b35a9899813079f1b1f6218631a2aedd993368ccb89bb81a9eda0289R734 using the
Cache-Control
construct that HTTP gives for control of caching.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.
My 2 cents for what they are worth,
If we asked the question "is this call safe (i.e. is it read-only)" I think the answer would be "maybe". Safe calls shouldn't alter the state of the server in any way but without knowing how the server implements the nonce endpoint we would not know the answer to the question.
If we asked the question "is this call idempotent" I think we would also get a "maybe". Client could do this call just to "fetch a value" which might not assume any intention of a side-effect, and from the perspective of the server it would depend on the implementation.
I do see the point in making it a GET because it is technically just fetching a random value, so semantically if fits, but there might be an embedded assumption here that this random value is a prerequisite for some other steps, and that getting a nonce can be viewed as setting up a state that needs to be validated by the server.
One could argue that it might be safer to make this a
POST
because this would communicate that the call might not be safe or idempotent and that subsequent calls could cause load, burden, and side-effects for the server.But this is just philosophizing, I personally do not have a clear preference, I see arguments for both
GET
andPOST
.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 can see arguments for both as well but was really trying to avoid them entirely with a simple (maybe simplistic) approach.
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.
but avoidance didn't work and so f7b4dc8 Removed the HTTP method specificity from the Nonce Request text and change the example from GET to 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.
After reading the comments and listening to the discussions, I'm ok with GET or POST, but we need to pick one.
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.
as discussed on the call - it's POST now 55c8388