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(key-auth): supporting key-auth plugin to get key from query string #4490

Merged
merged 2 commits into from
Jul 2, 2021

Conversation

wisdom-yzh
Copy link
Contributor

@wisdom-yzh wisdom-yzh commented Jun 26, 2021

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

apisix/plugins/key-auth.lua Show resolved Hide resolved
apisix/plugins/key-auth.lua Show resolved Hide resolved
@tokers
Copy link
Contributor

tokers commented Jun 27, 2021

@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
Copy link
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
Copy link
Member

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

local function fetch_jwt_token(ctx)

@@ -300,7 +300,45 @@ passed



=== TEST 11: valid consumer
=== TEST 11: customize querystring
Copy link
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
Copy link
Contributor Author

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

@spacewander
Copy link
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
Copy link
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

t/plugin/key-auth.t Outdated Show resolved Hide resolved
@tokers tokers merged commit de20916 into apache:master Jul 2, 2021
@forgaoqiang
Copy link

Great, this feature is what I needed, thanks

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