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 38 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'm not sure why we're changing this language about PE in this PR; I'd suggest dropping this change (particularly as it's causing a git conflict).
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 don't know when this was changed. Will revert it to the text in "main".
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 presume the parameters are mutually exclusive?
Also, in case of the
...uri
, the P.D. JSON is not contained in the 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.
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.
Media type being the content-type or accept header here?
If the former, is it intentional that this is the same as in the response?
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.
JWT's are fine as nonces. I think we can help by stating how it's passed back too. And we don't need the 'JSON' qualifier on String.
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.
...and neither on object (above), so we can drop that as well.
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.
What is the presentation request object?
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 above, media type meaning the content type?
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.
If this is meant as "The Verifier MUST create the request according to (#vp_token_request)" then this should be moved up. If this is meant to be a MUST for the Wallet to verify, it should be reworded and probably folded into the previous paragraph.
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.
...otherwise?
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 wonder if the first sentence is not already implied by "The Wallet MUST process the request as defined in [@RFC9101]."
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.
This must be expanded IMO. We discussed aspects of this on the calls or on PRs here in this repository: