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

Jump to bottom

Ability to use "Authorization: WhaTeVer base64(username:password)" #2963

Merged
merged 2 commits into from
Oct 23, 2017
Merged

Ability to use "Authorization: WhaTeVer base64(username:password)" #2963

merged 2 commits into from
Oct 23, 2017

Conversation

francois-maillard
Copy link
Contributor

NOTE: Please read the CONTRIBUTING.md guidelines before submitting your patch,
and ensure you followed them all:

https://github.com/Mashape/kong/blob/master/CONTRIBUTING.md#contributing

Summary

Ability to use Authorization: WhaTeVer base64(username:password) has a authorization header for the ldap-auth plugin.

In particular, when not using the basic-auth plugin, using Authorization: Basic base64(username:password) instead of "Authorization: LDAP base64(username:password)` will allow you to use any client (web browser, curl) without having to fool around the headers manually.

Full changelog

  • The ldap-auth plugin can now use any Authorization type you want, such as Authorization: LDAP base64(username:password), Authorization: Basic base64(username:password), Authorization: WhaTeVer base64(username:password). This can be set through the new config.header_type configuration option for the ldap-auth plugin. Its default value is ldap

@hishamhm hishamhm changed the base branch from master to next October 20, 2017 01:09
@hishamhm hishamhm changed the base branch from next to master October 20, 2017 01:10
@hishamhm
Copy link
Contributor

Hi, thanks for contributing! Since this PR adds new functionality, according to semantic versioning it should be targeting the next minor version, that is, the next branch. (I tried changing this automatically there but the branches have changed so it looks like you'll need to rebase.)

I see that this patch also fixes a bug in the pattern-matching of the "LDAP" string, which is a nice side-effect. :)

I am a little concerned about constructing the case-insensitive pattern on every access, but this can be mitigated using memoization and a weak table.

@hishamhm hishamhm added the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Oct 20, 2017
@francois-maillard francois-maillard changed the base branch from master to next October 20, 2017 09:34
@francois-maillard
Copy link
Contributor Author

francois-maillard commented Oct 20, 2017 •
edited
Loading

Hi, thanks for contributing!

well, thanks for developping!

it should be targeting the next minor version, that is, the next branch.

that should be fine now

this patch also fixes a bug in the pattern-matching of the "LDAP" string

yes, I was surprised that it was not case-insensitive

I am a little concerned about constructing the case-insensitive pattern on every access

I thought it wouldn't be an issue on the admin API, but that's not a reason for knowingly degrading perfs. I pushed a second commit that converts to lower case before matching.

hishamhm
hishamhm approved these changes Oct 23, 2017
View reviewed changes
Copy link
Contributor

@hishamhm hishamhm left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for the updates in the PR!

@hishamhm hishamhm merged commit 9fdf995 into Kong:next Oct 23, 2017
hishamhm added a commit that referenced this pull request Oct 23, 2017
@hishamhm
tests(ldap) make sure it fails when given an invalid type
12af007 
Prior to #2963, this plugin did an incorrect pattern matching
leading it to accept invalid type strings, as long as they
ended with the letters in "LDAP". This adds a regression test
for that situation.
hishamhm added a commit that referenced this pull request Oct 23, 2017
@hishamhm
tests(ldap) adds tests for header_type functionality
7367d11 
Adds tests for the functionality introduced in PR #2963.
@hishamhm hishamhm mentioned this pull request Oct 23, 2017
Closed
thibaultcha pushed a commit that referenced this pull request Oct 26, 2017
@hishamhm @thibaultcha
hotfix(ldap) anchor search to avoid bad substring match
78532d4 
Make sure we don't match "invalidldap" when searching for "ldap".

Prior to #2963, this plugin did an incorrect pattern matching
leading it to accept invalid type strings, as long as they
ended with the letters in "LDAP". This adds a regression test
for that situation.

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
thibaultcha pushed a commit that referenced this pull request Oct 26, 2017
@hishamhm @thibaultcha
tests(ldap) adds tests for header_type functionality
fee5e71 
Adds tests for the functionality introduced in PR #2963.

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
thibaultcha pushed a commit that referenced this pull request Jan 16, 2018
@francois-maillard @thibaultcha
feat(ldap) configurable name for Authorization header
d9bcd47 
Makes the `Authorization` header type configurable. The default remains `LDAP`, but strings such as `Basic` can be used for ease of testing via the browser. Incidentally, this fixes the case-insensitive parsing of the `LDAP` string, which was broken.

PR: #2963.
thibaultcha pushed a commit that referenced this pull request Jan 16, 2018
@hishamhm @thibaultcha
hotfix(ldap) anchor search to avoid bad substring match
24331d9 
Make sure we don't match "invalidldap" when searching for "ldap".

Prior to #2963, this plugin did an incorrect pattern matching
leading it to accept invalid type strings, as long as they
ended with the letters in "LDAP". This adds a regression test
for that situation.

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
thibaultcha pushed a commit that referenced this pull request Jan 16, 2018
@hishamhm @thibaultcha
tests(ldap) adds tests for header_type functionality
333ee6d 
Adds tests for the functionality introduced in PR #2963.

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
thibaultcha pushed a commit that referenced this pull request Jan 19, 2018
@francois-maillard @thibaultcha
feat(ldap) configurable name for Authorization header
ea5aad6 
Makes the `Authorization` header type configurable. The default remains `LDAP`, but strings such as `Basic` can be used for ease of testing via the browser. Incidentally, this fixes the case-insensitive parsing of the `LDAP` string, which was broken.

PR: #2963.
thibaultcha pushed a commit that referenced this pull request Jan 19, 2018
@hishamhm @thibaultcha
hotfix(ldap) anchor search to avoid bad substring match
3867784 
Make sure we don't match "invalidldap" when searching for "ldap".

Prior to #2963, this plugin did an incorrect pattern matching
leading it to accept invalid type strings, as long as they
ended with the letters in "LDAP". This adds a regression test
for that situation.

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
thibaultcha pushed a commit that referenced this pull request Jan 19, 2018
@hishamhm @thibaultcha
tests(ldap) adds tests for header_type functionality
a84faee 
Adds tests for the functionality introduced in PR #2963.

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
@francois-maillard
Copy link
Contributor Author

I just saw the status pending author feedback... sorry for the delay. That's all good to me.

@hishamhm hishamhm removed the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Jul 30, 2018
@hishamhm
Copy link
Contributor

@francois-maillard don't worry, I should have removed that label when you responded last time and I merged it :)

@thibaultcha thibaultcha mentioned this pull request Aug 28, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rzozi Rzozi Rzozi approved these changes

@hishamhm hishamhm hishamhm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@francois-maillard @hishamhm @Rzozi