Skip to content
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.

Sign up for GitHub

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 default response_mode being fragment #389

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

remove default response_mode being fragment #389

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion openid-4-verifiable-presentations-1_0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ The following additional considerations are given for pre-existing Authorization
: OPTIONAL. Defined in [@!RFC6749]. The Wallet MAY allow Verifiers to request presentation of Verifiable Credentials by utilizing a pre-defined scope value. See (#request_scope) for more details.

`response_mode`:
: OPTIONAL. Defined in [@!OAuth.Responses]. This parameter is used (through the new Response Mode `direct_post`) to ask the Wallet to send the response to the Verifier via an HTTPS connection (see (#response_mode_post) for more details). It is also used to request signing and encrypting (see (#jarm) for more details). If the parameter is not present, the default value is `fragment`.
: REQUIRED. Defined in [@!OAuth.Responses]. This parameter is used (through the new Response Mode `direct_post`) to ask the Wallet to send the response to the Verifier via an HTTPS connection (see (#response_mode_post) for more details). It is also used to request signing and encrypting (see (#jarm) for more details).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know what you're aiming for here (I think anyway) but does it warrant what is effect a breaking change for some?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you trying to say that we need to ask a wider set of implementers before making this change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what I'm saying TBH - just noting that it's a maybe more impactful change than might be apparent at first glance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, the conformance tests treat response_mode as mandatory already, but technically that is because they only support direct_post and direct_post.jwt (and we've had no requests to implement other modes).

response_mode is also already mandatory in the Browser DC API.

Is there a GitHub issue for this change? I'm not currently clear why we're making this change and we should at least state that so any verifier that does get broken knows why. But I'm currently feeling like it would be okay to make this change, my gut feeling is not too many people are using the fragment response mode, and we already have some significant breaking changes since the last ID anyway (e.g. requiring typ in request objects), so verifiers are going to have to make updates regardless and this one would be a very simple update.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I obviously do not know all the verifiers but i know quite a few and none of them use fragment. this PR has been open for a while and we raised it in the WG few times and no one objected so i think we cleared. can also send out in the ML.

Is there a GitHub issue for this change? I'm not currently clear why we're making this change and we should at least state that so any verifier that does get broken knows why.

because this requirement does not make much sense... I think there was an issue that mentioned this that we were reviewing on the editors call.

Do we need to replace default with direct_post? personally don't think so, but wanted to clarify.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to replace default with direct_post? personally don't think so, but wanted to clarify.

I don't have a strong opinion either way.


`client_id`:
: REQUIRED. Defined in [@!RFC6749]. This specification defines additional requirements to enable the use of Client Identifier Schemes as described in (#client_metadata_management).
Expand Down
Loading