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
request_uri extension #59
request_uri extension #59
Changes from 66 commits
0ef69f7
b402aa7
673fcf7
9dff282
f945d54
0a42d0f
37fbccf
b7c2570
350391d
7822756
8056fc1
9061d50
ba585be
b1ee70e
d29107b
d198019
30d59a8
63a86f7
70ac39b
e35ab32
8cb645f
0ced90a
42f6866
4f335fc
18fd478
bfc2bdc
acd0c13
1101fda
c1ee35a
357222c
0844acb
f0fa298
cfb2d36
2c49b9b
21696fd
a8f4917
2f08615
9a021bb
8719239
e7b8871
9d78904
0e58e39
af6c572
02a58d0
76098d7
8666529
396f007
b1e94aa
d9a49d9
63d1f7f
6edd8d1
e8a6cde
e1276ec
6d5601b
c4d175b
3b0a9fb
df5d0f1
f4e4236
ac1ffc0
803cbe6
95ac037
6890440
148e1bd
ae65453
e3df8ac
0c9413d
91f811f
56ef646
a21eba3
efd0722
4ea299d
3208b16
55a4faf
5a0b1af
bee5ed9
286bf67
3c298b7
4540cca
e5add86
06aef10
e437b50
4140e35
1e033f7
d167829
fb39140
674d0aa
fe8ae63
1088682
c6057d9
e937d3a
f36cfe6
cdfea43
1737a30
d2ba099
a478244
c6db7d5
d3e6894
17a60fc
ada0a45
6f9c661
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.
I think somebody mentioned it somewhere already, but the "first" in this sentence is not ideal. This is an authorization request, so it will be the only 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.
removed "first"
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 request object obtained from the request_uri is aslo an Authorization Request or part of the same multi step Authorization Request flow. The wording is a bit tricky.
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 it be better to name this parameter something like
wallet_metadata_support
instead of naming this to the technical solution?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 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.
Nit: The HTTP spec states "The method token is case-sensitive (...). By convention, standardized methods are defined in all-uppercase US-ASCII letters" - https://www.rfc-editor.org/rfc/rfc9110.html#section-9.
The IANA registry at https://www.iana.org/assignments/http-methods/http-methods.xhtml has
GET
andPOST
so I would suggest that we also use uppercase 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.
I can accept either upper or lower case, but since parameters in this context are usually lower case, I have a preference for that.
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.
client_metadata
". This is only required if the Verifier assumes the Wallet has no other method to retrieve theclient_metadata
. If theclient_id_scheme
supports client metadata resolution, thenclient_metadata
parameter is not necessary. I would suggest the following:Does this 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.
why not just MAY?
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.
MAY is redundant since this is already the case, 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.
I would adjust the language. I will make a proposal tomorrow. It should be highlighted that client_metadata in the authz request is not required and can be obtained from different sources and still use this feature.
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.
Different sources, e.g., by retrieving it via entity statements using a combination of client_id and client_id_scheme.
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.
@awoie I modified the text so the Verifier should add the parameter only if there is no other way. I did not want to tie it t the client id scheme Please have a look whether the text works for you.
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 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.
also
wallet_nonce
authorization request parameter needs to be defined somewhere else (authorization request/section 5 probably). this section is wallet's request parameter.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'm fine with this direction but I would feel better if we could include the following
wallet_metadata
parameters as well to indicate support for encryption.And for signatures:
This is also what we do in ISO 18013-7 where JARM encrption is mandatory. In ISO 18013-7, the wallet metadata is assumed to be statically pre-populated to the verifier.
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.
please propose text
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 proposal below.