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

fix(oauth2) body params creds: require both client_id and client_secret #2577

Closed
wants to merge 1 commit into from

Conversation

cedum
Copy link
Contributor

@cedum cedum commented May 30, 2017

Summary

Fixes POST /oauth2/token request client credentials checking when having:

  • client_id:client_secret passed via the authorization header;
  • and client_id passed as a request body parameter.
    To retrieve client credentials from the body parameters, both client_id and client_secret must be present.

Full changelog

  • oauth2: body params creds: require both client_id and client_secret
  • added a test case into spec/03-plugins/26-oauth2/03-access_spec.lua

Issues resolved

Fix #2446

Fixes POST /oauth2/token request client credentials checking when having:
* `client_id:client_secret` passed via the authorization header;
* and `client_id` passed as a request body parameter.
To retrieve client credentials from the body parameters, both `client_id` and `client_secret` must be present.

Fix Kong#2446
Copy link
Contributor

@p0pr0ck5 p0pr0ck5 left a comment

Choose a reason for hiding this comment

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

LGTM, would like a second pair of eyes here. @thefosk or @thibaultcha ?

}
})
local body = assert.res_status(200, res)
assert.is_table(ngx.re.match(body, [[^\{"token_type":"bearer","access_token":"[\w]{32,32}","expires_in":5\}$]]))
Copy link
Contributor

Choose a reason for hiding this comment

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

oof, these tests are in a really poor form. we should be decoding the returned JSON body and examining each element we care about, not presuming some details about the form of the encoded payload. this isnt a blocker for this review, since it looks like a copypasta from previous commits, more of a note that we should clean up this test in the future :)

@thibaultcha
Copy link
Member

Nice, thanks @cedum for the patch! I believe this should be opened against master though.

@thibaultcha
Copy link
Member

Manually merged to master. Thanks!

@thibaultcha thibaultcha closed this Jun 6, 2017
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