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/custom oauth2 header #2928

Closed
wants to merge 1 commit into from

Conversation

supraja93
Copy link

Summary

Extended the oauth2 plugin to have a new parameter "auth_header_name" which by default is "authorization" but can be overwritten to any custom header name when enabling oauth2 for an api

Full changelog

  • kong/plugins/oauth2/schema.lua
  • kong/plugins/oauth2/access.lua

Test cases

  • spec/03-plugins/26-oauth2/01-schema_spec.lua
  • spec/03-plugins/26-oauth2/03-access_spec.lua

@supraja93
Copy link
Author

Currently Kong uses the "Authorization" header to check for oauth2 tokens, the changes in this request enables kong to verify tokens in the custom headers configured when enabling the Oauth2 plugin on an api.

eg:
curl -X POST http://localhost:8001/apis/test-api/plugins
--data "name=oauth2"
--data "config.enable_authorization_code=true"
--data "config.scopes=email"
--data "config.auth_header_name=Custom-Auth-Header"

If the config.auth_header_name is not set then the default header checked is "Authorization"

Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

Seems like a nice feature, can you please update based on the suggestions?

Thx for your contribution.

@@ -31,6 +31,7 @@ return {
accept_http_if_already_terminated = { required = false, type = "boolean", default = false },
anonymous = {type = "string", default = "", func = check_user},
global_credentials = {type = "boolean", default = false},
auth_header_name = {required = false, type = "string", default = "authorization"},
Copy link
Member

Choose a reason for hiding this comment

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

this will require a migration. Existing records in the datastore will not have this field, nor the default. (The default is not applied when loading from the datastore, but only when adding the plugin config)

This also means that this PR cannot go into master but has to go into next (migrations go in the next major release, which is the next branch)

As an example, look at the migrations in this PR: #2883
(you need to rebase on next to get those files)

@@ -218,9 +218,9 @@ local function authorize(conf)
})
end

local function retrieve_client_credentials(parameters)
local function retrieve_client_credentials(parameters,conf)
Copy link
Member

Choose a reason for hiding this comment

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

nit pick: add a space after the comma

@@ -271,7 +271,7 @@ local function issue_token(conf)
response_params = {[ERROR] = "unsupported_grant_type", error_description = "Invalid " .. GRANT_TYPE}
end

local client_id, client_secret, from_authorization_header = retrieve_client_credentials(parameters)
local client_id, client_secret, from_authorization_header = retrieve_client_credentials(parameters,conf)
Copy link
Member

Choose a reason for hiding this comment

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

nit pick: add a space after the comma

auth_header_name = "custom_header_name",
},
})
local api11 = assert(helpers.dao.apis:insert {
Copy link
Member

Choose a reason for hiding this comment

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

api11 is added twice. Please rename to api12.

@supraja93
Copy link
Author

@Tieske incorporated the suggested changes, please let me know your thoughts on them

@Tieske Tieske changed the base branch from master to next October 21, 2017 01:17
Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

One more comment. I changed the PR to go against next, which now removes the extra commits from the github ui.

Looking good!

}
},
{
name = "2017-10-19-set_auth_header_name_default",
Copy link
Member

Choose a reason for hiding this comment

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

This needs to lexically sort, so please add the time in hhmmss format to the key (see examples above)

Copy link
Member

Choose a reason for hiding this comment

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

nvm, just saw the older ones did this neither.

@Tieske Tieske added pr/status/please review pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/status/please review labels Oct 23, 2017
Tieske added a commit to Kong/docs.konghq.com that referenced this pull request Oct 25, 2017
hishamhm pushed a commit that referenced this pull request Oct 25, 2017
Adds to the `oauth2` plugin a new parameter `auth_header_name` to define the header name
to use. By default its value is `"authorization"`.

From #2928

Signed-off-by: Hisham Muhammad <hisham@gobolinux.org>
@hishamhm
Copy link
Contributor

Merged manually, adjusting the commit message. Thank you @supraja93!

@hishamhm hishamhm closed this Oct 25, 2017
@Tieske
Copy link
Member

Tieske commented Oct 25, 2017

👍

@supraja93
Copy link
Author

@Tieske @hishamhm Thank you 👍

thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request Dec 18, 2017
thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request Jan 12, 2018
thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request Jan 16, 2018
thibaultcha added a commit to Kong/docs.konghq.com that referenced this pull request Jan 16, 2018
thibaultcha pushed a commit that referenced this pull request Jan 16, 2018
Adds to the `oauth2` plugin a new parameter `auth_header_name` to define the header name
to use. By default its value is `"authorization"`.

From #2928

Signed-off-by: Hisham Muhammad <hisham@gobolinux.org>
thibaultcha pushed a commit that referenced this pull request Jan 19, 2018
Adds to the `oauth2` plugin a new parameter `auth_header_name` to define the header name
to use. By default its value is `"authorization"`.

From #2928

Signed-off-by: Hisham Muhammad <hisham@gobolinux.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants