-
Notifications
You must be signed in to change notification settings - Fork 20
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
clarify client id scheme description #51
Conversation
@@ -417,7 +417,7 @@ This specification defines the following values for the `client_id_scheme` param | |||
|
|||
* `pre-registered`: This value represents the [@!RFC6749] default behavior, i.e., the Client Identifier needs to be known to the Wallet in advance of the Authorization Request. The Verifier metadata is obtained using [@!RFC7591] or through out-of-band mechanisms. | |||
|
|||
* `redirect_uri`: This value indicates that the Verifier's redirect URI is also the value of the Client Identifier. In this case, the Authorization Request MUST NOT be signed, the Verifier MAY omit the `redirect_uri` Authorization Request parameter, and all Verifier metadata parameters MUST be passed using the `client_metadata` or `client_metadata_uri` parameter defined in (#vp_token_request). | |||
* `redirect_uri`: This value indicates that the Verifier's redirect URI is also the value of the Client Identifier. In this case, the Authorization Request MUST NOT be signed. The Verifier MAY omit the `redirect_uri` Authorization Request parameter. All Verifier metadata parameters MUST be passed using the `client_metadata` or `client_metadata_uri` parameter defined in (#vp_token_request). When Response Mode `direct_post` is used, these restrictions apply to the `response_uri` value instead of `redirect_uri` value. |
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 the 'MAY' clause is technically a restriction so a different word might be better. I'm struggling to come up with a really good word but 'clauses' may be better?
* `redirect_uri`: This value indicates that the Verifier's redirect URI is also the value of the Client Identifier. In this case, the Authorization Request MUST NOT be signed. The Verifier MAY omit the `redirect_uri` Authorization Request parameter. All Verifier metadata parameters MUST be passed using the `client_metadata` or `client_metadata_uri` parameter defined in (#vp_token_request). When Response Mode `direct_post` is used, these restrictions apply to the `response_uri` value instead of `redirect_uri` value. | |
* `redirect_uri`: This value indicates that the Verifier's redirect URI is also the value of the Client Identifier. In this case, the Authorization Request MUST NOT be signed. The Verifier MAY omit the `redirect_uri` Authorization Request parameter. All Verifier metadata parameters MUST be passed using the `client_metadata` or `client_metadata_uri` parameter defined in (#vp_token_request). When Response Mode `direct_post` is used, these clauses apply to the `response_uri` value instead of `redirect_uri` value. |
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.
omitting something is a technical restriction, no?
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. I think allowing someone to optionally omit something is technically an addition rather than a restriction. [addition probably isn't quite the right word, hopefully the point still makes sense :) ]
I didn't want to give anyone any wiggle room to argue they they don't have to allow verifiers to omit response_uri.
Co-authored-by: Joseph Heenan <[email protected]>
how the issue #33 might take benefits from this PR? |
@@ -417,7 +417,7 @@ This specification defines the following values for the `client_id_scheme` param | |||
|
|||
* `pre-registered`: This value represents the [@!RFC6749] default behavior, i.e., the Client Identifier needs to be known to the Wallet in advance of the Authorization Request. The Verifier metadata is obtained using [@!RFC7591] or through out-of-band mechanisms. | |||
|
|||
* `redirect_uri`: This value indicates that the Verifier's redirect URI is also the value of the Client Identifier. In this case, the Authorization Request MUST NOT be signed, the Verifier MAY omit the `redirect_uri` Authorization Request parameter, and all Verifier metadata parameters MUST be passed using the `client_metadata` or `client_metadata_uri` parameter defined in (#vp_token_request). | |||
* `redirect_uri`: This value indicates that the Verifier's redirect URI is also the value of the Client Identifier. In this case, the Authorization Request MUST NOT be signed. The Verifier MAY omit the `redirect_uri` Authorization Request parameter. All Verifier metadata parameters MUST be passed using the `client_metadata` or `client_metadata_uri` parameter defined in (#vp_token_request). When Response Mode `direct_post` is used, these clauses apply to the `response_uri` value instead of `redirect_uri` value. |
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 statement is not clear
it is not clear why the client uid must (or is) the same of the redirect uri.
it is not clear why the authorization must not be signed.
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.
"it is not clear why the client uid must (or is) the same of the redirect uri."
there is no way to link the client_id to a redirect_uri in this scheme then making both the same
"it is not clear why the authorization must not be signed."
I think we can drop that requirement. It was used in the past (before client id schemes were introduced) to differentiate redirect_uri as client id and other options. With the client id scheme redirect_uri
, it is no longer needed.
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.
suggest to drop the requirement to sign requests for redirect_uri scheme
@@ -417,7 +417,7 @@ This specification defines the following values for the `client_id_scheme` param | |||
|
|||
* `pre-registered`: This value represents the [@!RFC6749] default behavior, i.e., the Client Identifier needs to be known to the Wallet in advance of the Authorization Request. The Verifier metadata is obtained using [@!RFC7591] or through out-of-band mechanisms. | |||
|
|||
* `redirect_uri`: This value indicates that the Verifier's redirect URI is also the value of the Client Identifier. In this case, the Authorization Request MUST NOT be signed, the Verifier MAY omit the `redirect_uri` Authorization Request parameter, and all Verifier metadata parameters MUST be passed using the `client_metadata` or `client_metadata_uri` parameter defined in (#vp_token_request). | |||
* `redirect_uri`: This value indicates that the Verifier's redirect URI is also the value of the Client Identifier. In this case, the Authorization Request MUST NOT be signed. The Verifier MAY omit the `redirect_uri` Authorization Request parameter. All Verifier metadata parameters MUST be passed using the `client_metadata` or `client_metadata_uri` parameter defined in (#vp_token_request). When Response Mode `direct_post` is used, these clauses apply to the `response_uri` value instead of `redirect_uri` value. |
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.
"it is not clear why the client uid must (or is) the same of the redirect uri."
there is no way to link the client_id to a redirect_uri in this scheme then making both the same
"it is not clear why the authorization must not be signed."
I think we can drop that requirement. It was used in the past (before client id schemes were introduced) to differentiate redirect_uri as client id and other options. With the client id scheme redirect_uri
, it is no longer needed.
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 that we should follow Torsten's points here: https://github.com/openid/OpenID4VP/pull/51/files#r1456848468
I think https://github.com/openid/OpenID4VP/pull/84/files has solved the main issue this PR was trying to solve. I suggest closing this PR. |
clarify that when certain client id schemes mention redirect_uri, it is only when response mode is not direct_post and when response mode is direct_post, those restrictions apply to response_uri instead of redirect_uri.