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.

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

Support processing parameters sent as a URL-encoded form #8325

Merged
merged 5 commits into from
Feb 12, 2020

Conversation

kalafut
Copy link
Contributor

@kalafut kalafut commented Feb 10, 2020

The approach taken is intended to be conservative since we've always
assumed JSON and not required Content-Type. Tools and libraries that
default to "application/x-www-form-urlencoded" even when JSON is being
sent (e.g. curl) will break without these extra backwards compatibility
measures.

This PR is a reboot of: #5935

The approach taken is intended to be conservative since we've always
assumed JSON and not required Content-Type. Tools and libraries that
default to "application/x-www-form-urlencoded" even when JSON is being
sent (e.g. curl) will break without these extra backwards compatibility
measures.

This PR is a reboot of: #5935
@kalafut kalafut added this to the 1.4 milestone Feb 10, 2020
jefferai
jefferai previously approved these changes Feb 11, 2020
Copy link
Member

@jefferai jefferai left a comment

Choose a reason for hiding this comment

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

Looks great!

briankassouf
briankassouf previously approved these changes Feb 12, 2020
Copy link
Contributor

@briankassouf briankassouf left a comment

Choose a reason for hiding this comment

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

👍 nice!

Copy link
Collaborator

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

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

Looks good modulo one potential bug.

@kalafut kalafut dismissed stale reviews from briankassouf and jefferai via 8540377 February 12, 2020 21:42
@kalafut kalafut merged commit 9d31716 into master Feb 12, 2020
@kalafut kalafut deleted the form-processing branch February 12, 2020 22:20
kalafut pushed a commit to hashicorp/vault-plugin-auth-jwt that referenced this pull request Feb 13, 2020
form_post is not a commonly used mode but is needed for certain
applications. Adding support for it impacted both the CLI and UI handler,
as well as Vault (hashicorp/vault#8325).

In addition to allowing configuration of `response_mode=form_post`, the
operator can now also choose to configure `response_types`, which today
is always `code`. This PR allows adding the `id_token` type as well. This
is only supported if form_post is enabled.

A curious aspect of this PR is to respond with some Javascript that
writes to localStorage. This is a effectively doing what the UI does
after a redirect, and allows the form_post support without modifying the
UI code at all. There are cleaner options, but they would require a
significant update to the UI.
kalafut pushed a commit to hashicorp/vault-plugin-auth-jwt that referenced this pull request Feb 15, 2020
* Support form_post mode

form_post is not a commonly used mode but is needed for certain
applications. Adding support for it impacted both the CLI and UI handler,
as well as Vault (hashicorp/vault#8325).

In addition to allowing configuration of `response_mode=form_post`, the
operator can now also choose to configure `response_types`, which today
is always `code`. This PR allows adding the `id_token` type as well. This
is only supported if form_post is enabled.

A curious aspect of this PR is to respond with some Javascript that
writes to localStorage. This is a effectively doing what the UI does
after a redirect, and allows the form_post support without modifying the
UI code at all. There are cleaner options, but they would require a
significant update to the UI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants