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

feat(key-auth) optionally search the request body for credentials #2493

Merged
merged 1 commit into from
May 4, 2017

Conversation

p0pr0ck5
Copy link
Contributor

@p0pr0ck5 p0pr0ck5 commented May 3, 2017

Summary

This commit provides an optional plugin configuration field to
search the request body for the authentication credentials. This
behavior respects the hide_credentials field.

This commit also expands the access test suite to cover a few more
use cases.

Full changelog

  • add the key_in_body configuration option
  • add related tests
  • expand config.hide_credentials tests to cover all use cases (headers, query string, req body)

@p0pr0ck5 p0pr0ck5 force-pushed the feat/key-auth-body branch 2 times, most recently from a943285 to 906419c Compare May 3, 2017 20:34
@thibaultcha thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/status/please review labels May 4, 2017
if type == "postData" then
local t = json[type].text:sub(8)
field = { apikey = t ~= "" and t or nil }
else
Copy link
Member

Choose a reason for hiding this comment

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

I think we should apply our codestyle to tests as well

if type == "postData" then
local t = json[type].text:sub(8)
field = { apikey = t ~= "" and t or nil }
else
Copy link
Member

Choose a reason for hiding this comment

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

ditto

This commit provides an optional plugin configuration field to
search the request body for the authentication credentials. This
behavior respects the hide_credentials field.

This commit also expands the access test suite to cover a few more
use cases.
@p0pr0ck5 p0pr0ck5 force-pushed the feat/key-auth-body branch from 906419c to 4cd50e9 Compare May 4, 2017 20:38
@p0pr0ck5 p0pr0ck5 merged commit d3e9071 into master May 4, 2017
@p0pr0ck5 p0pr0ck5 deleted the feat/key-auth-body branch May 4, 2017 20:39
@p0pr0ck5
Copy link
Contributor Author

p0pr0ck5 commented May 4, 2017

Thanks, minor style fixes applied and merged.

@thibaultcha
Copy link
Member

I made a slight oversight: we should have updated the "No API key found in ..." error message returned on HTTP 401, since we now take the body in consideration as well. Maybe even better, we could change it to simply: "No API key provided" or something similar. After all, where else than the headers, the querystring, and the body could once insert an arbitrary value? Not that many places...

@p0pr0ck5
Copy link
Contributor Author

p0pr0ck5 commented May 5, 2017

@thibaultcha would the resulting change in the return values constitute a breaking change, and thus belong in the next major release? or this is something we're comfortable patching now?

@thibaultcha
Copy link
Member

I think as long as the HTTP status code doesn't change, this is hardly considered a breaking change. It should be fine.

@p0pr0ck5
Copy link
Contributor Author

p0pr0ck5 commented May 5, 2017

👍 wasn't sure about our stance on the message content itself (i imagine a UI application working based on the contents of the message field). done in #2502.

thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request May 25, 2017
thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request May 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants