-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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) skip authenticating preflight OPTIONS requests #2743
Conversation
Preflight OPTIONS requests explicitly exclude user credentials and custom headers. The key-auth plugin should not attempt to authenticate OPTIONS requests, since browsers will strip Kong's apiKey header. Fix #1292
new config variable authenticate_preflight to control whether or not to authenticate OPTIONS requests.
2b3a041
to
a454a98
Compare
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.
Suggestion to use a "default false" option. The same would apply to the other plugin pr too.
kong/plugins/key-auth/handler.lua
Outdated
@@ -149,6 +150,15 @@ end | |||
function KeyAuthHandler:access(conf) | |||
KeyAuthHandler.super.access(self) | |||
|
|||
-- check if preflight request and wether it should be authenticated | |||
if conf.authenticate_preflight == false and get_method() == "OPTIONS" then |
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.
Instead of making a "default true" option (authenticate_preflight
) we could make a "default false" one (skip_preflight
). That way the default nil would just work: if conf.skip_preflight and ...
. No migration 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.
there are others already: https://github.com/Mashape/kong/blob/master/kong/plugins/cors/schema.lua#L24
if this gets merged into master
I plan to create a PR against next
that removes the comments, reverts this to if not conf.authenticate_preflight and ...
, and implements the migration.
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.
Sounds good then!
This belongs to PR #2743 which went into a minor version.
This belongs to PR #2743 which went into a minor version.
a454a98
to
3c10393
Compare
…m/ecmendenhall/kong into fix/key-auth-cors-options-requests
3c10393
to
59b607a
Compare
This belongs to PR #2743 which went into a minor version.
rebased version of #1535
fixes #1292, #1535