-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add one more option to invalid_request if none of the valid options is provided #101
Conversation
…on has to be provided
@@ -659,7 +659,7 @@ The error response follows the rules as defined in [@!RFC6749], with the followi | |||
|
|||
`invalid_request`: | |||
|
|||
- The request contains more than one out of the following three options to communicate a requested Credential: a `presentation_definition` parameter, a `presentation_definition_uri` parameter, or a scope value representing a Presentation Definition. | |||
- The request contains none or more than one out of the following three options to communicate a requested Credential: a `presentation_definition` parameter, a `presentation_definition_uri` parameter, or a scope value representing a Presentation Definition. |
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.
We may want to make it clearer that a wallet isn't required to return an error in the 'none of' case (but is required to return an error in the "more than one" case). [As the wallet may support other protocols, e.g. SIOPv2]
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.
In the context of OpenID4VP it is required to provide one of the options, otherwise it is not an OpenID4VP request. Especially, I would expect an error if response_type is vp_token. I agree that for response_type code, the situation is different. If the request is also a SIOPv2 request using vp_token id_token, then I would still expect an error.
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.
Perhaps we should think about a dedicated scope value that clearly defines that the request is an OpenID4VP request? This would be only required for response_type code though.
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 updated the language in the PR. Would you agree with the new language @jogu ?
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.
vp_token
response type is not mandatory when response type is code
. Also, why none or more
? one of these three options has to be provided for the wallet to know what credential is being requested
The paragraph is about raising an error in case none or more than one option is provided. I agree that there is an exception for response_type code. That is why I added the "The request uses the |
Does this now make sense? @Sakurann |
Co-authored-by: Kristina <[email protected]>
Co-authored-by: Joseph Heenan <[email protected]>
This PR does:
This PR is non-breaking since we already have the requirement that one of the options must be present.