-
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
fix(oauth2) ensuring that the request indeed has a payload #3063
Conversation
Great! Thank you for the second patch, we’ll have a look at it as soon as we can manage! |
kong/plugins/oauth2/access.lua
Outdated
return utils.table_merge(ngx.req.get_uri_args(), public_utils.get_body_args()) | ||
local uri_args = ngx.req.get_uri_args() | ||
local body_args = {} | ||
if ngx.req.get_method() ~= "GET" 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.
Why create the body args table outside this condition? It's a needless table creation and gc on GET requests. We should rather create this temp table and merge it with uri args only when the req method is appropriate
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.
thanks, I'll fix it
LGTM |
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.
Thank you for submitting this patch! I believe it is a very valuable fix that we should include as soon as possible.
Will you also provide a regression test that proves the faulty behavior and ensures the patch takes care of it (for each HTTP method)? We cannot accept the patch without it. Thank you!
kong/plugins/oauth2/access.lua
Outdated
ngx.req.read_body() | ||
local body_args = public_utils.get_body_args() | ||
return utils.table_merge(uri_args, body_args) | ||
else |
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 is no need for this else
branch if the previous one is guaranteed to return
. This would be better written as:
if get_method() ~= "GET" then
read_body()
-- ...
return ...
end
return uri_args
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.
"else" does not imply a separate "branch" is created? there is no semantically meaningful difference here
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.
See https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#conditional-expressions (5th code block)
There is no need for the indentation/boilerplate.
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.
(if there are readability concerns, thats certainly valid. just noting that functionally there is no difference)
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.
This is indeed about readability.
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.
yes, I'll read the CONTRIBUTING.md again.
kong/plugins/oauth2/access.lua
Outdated
-- OAuth2 parameters could be in both the querystring or body | ||
return utils.table_merge(ngx.req.get_uri_args(), public_utils.get_body_args()) | ||
local uri_args = ngx.req.get_uri_args() | ||
if ngx.req.get_method() ~= "GET" 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.
There are other HTTP verbs that should not carry a body: we should think about HEAD
, OPTIONS
, and probably DELETE
as well.
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.
fixed
there should be call ngx.req.read_body() or public_utils.get_body_args() for requests method is POST, PUT or PATCH Fix #3055
@thibaultcha add regression test. |
} | ||
}) | ||
assert.res_status(200, res) | ||
end) |
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.
Those tests aren't failing at all when I try them against the current master? Are we sure they are representative of the error encountered in #3055? For them to qualify as regression tests, we need to demonstrate the observed issue in those tests, and ensure that this patch fixes it.
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 think what we should test is the presence (or not) of the error log observed by #3055? We could read the logs in servroot/logs/error.log
and parse them to see if this error exists. Unfortunately, we do not have many tests following this pattern, and we tend to ignore those tests rather than trying to write them. I'd be willing to make another exception until we come up with a more "native" solution to do so.
Merged in 552360a with the proper regression test. Thank you for your contribution! |
Summary
there should be no reason to call ngx.req.read_body() or
public_utils.get_body_args() for requests that do not have a payload.
Issues resolved
Fix #3055