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

fix(oauth2) ensuring that the request indeed has a payload #3063

Closed
wants to merge 1 commit into from
Closed

fix(oauth2) ensuring that the request indeed has a payload #3063

Show file tree
Hide file tree
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
12 changes: 9 additions & 3 deletions kong/plugins/oauth2/access.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,16 @@ local function get_redirect_uri(client_id)
end

local function retrieve_parameters()
ngx.req.read_body()

-- 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()
local method = ngx.req.get_method()
if method == "POST" or method == "PUT" or method == "PATCH" then
ngx.req.read_body()
local body_args = public_utils.get_body_args()
return utils.table_merge(uri_args, body_args)
end

return uri_args
end

local function retrieve_scopes(parameters, conf)
Expand Down
34 changes: 33 additions & 1 deletion spec/03-plugins/26-oauth2/03-access_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1709,7 +1709,7 @@ describe("Plugin: oauth2 (access)", function()
local json = cjson.decode(body)
assert.same({ error_description = "The access token is invalid or has expired", error = "invalid_token" }, json)
end)
it("works when a correct access_token is being sent in a form body", function()
it("works when a correct access_token is being sent in a form body (POST)", function()
local token = provision_token()

local res = assert(proxy_ssl_client:send {
Expand All @@ -1725,6 +1725,38 @@ describe("Plugin: oauth2 (access)", function()
})
assert.res_status(200, res)
end)
it("works when a correct access_token is being sent in a form body (PUT)", function()
local token = provision_token()

local res = assert(proxy_ssl_client:send {
method = "PUT",
path = "/request",
body = {
access_token = token.access_token
},
headers = {
["Host"] = "oauth2.com",
["Content-Type"] = "application/json"
}
})
assert.res_status(200, res)
end)
it("works when a correct access_token is being sent in a form body (PATCH)", function()
local token = provision_token()

local res = assert(proxy_ssl_client:send {
method = "PATCH",
path = "/request",
body = {
access_token = token.access_token
},
headers = {
["Host"] = "oauth2.com",
["Content-Type"] = "application/json"
}
})
assert.res_status(200, res)
end)
Copy link
Member

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.

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 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.

it("works when a correct access_token is being sent in an authorization header (bearer)", function()
local token = provision_token()

Expand Down