Skip to content

feat(key-auth): supporting key-auth plugin to get key from query string#4490

Merged
tokers merged 2 commits into
apache:masterfrom
wisdom-yzh:master
Jul 2, 2021
Merged

feat(key-auth): supporting key-auth plugin to get key from query string#4490
tokers merged 2 commits into
apache:masterfrom
wisdom-yzh:master

Conversation

@wisdom-yzh

@wisdom-yzh wisdom-yzh commented Jun 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

fixes issue #4421

Adding a new query property to key-auth plugin, which can get key value from http query string

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

tokers
tokers reviewed Jun 27, 2021
View reviewed changes
Comment thread apisix/plugins/key-auth.lua
Comment thread apisix/plugins/key-auth.lua
@tokers

tokers commented Jun 27, 2021

Copy link
Copy Markdown
Contributor

@wisdom-yzh From the changes I guess you tried to introduce an using order between the header and query, it will be somewhat obscure, we may treat them equally but set them as exclusive in jsonschema (e.g. by introducing the oneOf primitive).

@wisdom-yzh

wisdom-yzh commented Jun 27, 2021

Copy link
Copy Markdown
Contributor Author

@tokers Accord to description of #4421, I guess @forgaoqiang wants to get key from both query string and headers, not exclusive. Maybe I can define one field instead of two fields, and check key from both query and headers?

@spacewander

spacewander commented Jun 29, 2021

Copy link
Copy Markdown
Member

I think "header and query string" is OK, as jwt-auth plugin does it:

apisix/apisix/plugins/jwt-auth.lua

Line 154 in 07bfe5b

local function fetch_jwt_token(ctx)

spacewander
spacewander reviewed Jun 29, 2021
View reviewed changes
Comment thread t/plugin/key-auth.t Outdated


=== TEST 11: valid consumer
=== TEST 11: customize querystring

@spacewander spacewander Jun 29, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please don't add the test in the middle of the other test. And we need to verify it via GET /hello?xxx in its own version of "valid consumer"

@wisdom-yzh

wisdom-yzh commented Jun 29, 2021

Copy link
Copy Markdown
Contributor Author

@spacewander hi~I have changed strategy to "header and query string", and then rename field from "header" to "key_name“.

@spacewander

spacewander commented Jun 30, 2021

Copy link
Copy Markdown
Member

@wisdom-yzh
Err... We can't change the existing fields which will break the user's configuration. Your original solution is good enough, please bring it back.

@wisdom-yzh

wisdom-yzh commented Jun 30, 2021

Copy link
Copy Markdown
Contributor Author

@wisdom-yzh
Err... We can't change the existing fields which will break the user's configuration. Your original solution is good enough, please bring it back.

ok, i'll restore the origin commit and fix the test case issue soon

spacewander
spacewander approved these changes Jul 1, 2021
View reviewed changes
Comment thread t/plugin/key-auth.t Outdated
@tokers
tokers merged commit de20916 into apache:master Jul 2, 2021
@forgaoqiang

forgaoqiang commented Jul 30, 2021

Copy link
Copy Markdown

Great, this feature is what I needed, thanks

@liuxiran liuxiran mentioned this pull request Aug 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spacewander spacewander spacewander approved these changes
@tokers tokers tokers 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.

4 participants

@wisdom-yzh @tokers @spacewander @forgaoqiang