-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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: add hide_credentials for multi-auth plugin #11256
base: master
Are you sure you want to change the base?
Conversation
7a041fc
to
6b7bbed
Compare
Can you please review this pull request? |
@@ -0,0 +1,56 @@ | |||
-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new file has only been used twice for the application, does it have more use cases? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, It can be used for future autherntication plugins to clear cookies and query params
docs/en/latest/plugins/multi-auth.md
Outdated
@@ -41,6 +41,7 @@ For Route: | |||
| Name | Type | Required | Default | Description | | |||
|--------------|-------|----------|---------|-----------------------------------------------------------------------| | |||
| auth_plugins | array | True | - | Add supporting auth plugins configuration. expects at least 2 plugins | | |||
|hide_credentials|boolean|False|False|Set to true will not pass the authorization request of header\query\cookie to the Upstream.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I think this should ideally be formatted, such as spaces or indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
t/plugin/multi-auth2.t
Outdated
--- request | ||
GET /t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A similar section can be found by referring to this test, they can simplify
https://github.com/apache/apisix/blob/master/t/plugin/forward-auth.t#L23-L29
https://github.com/apache/apisix/blob/master/t/plugin/forward-auth.t#L54-L56
t/plugin/multi-auth.t
Outdated
=== TEST 23: enable multi auth plugin with same header without hide credential | ||
--- config | ||
location /t { | ||
content_by_lua_block { | ||
local t = require("lib.test_admin").test | ||
local code, body = t('/apisix/admin/routes/1', | ||
ngx.HTTP_PUT, | ||
[[{ | ||
"plugins": { | ||
"multi-auth": { | ||
"auth_plugins": [ | ||
{ | ||
"basic-auth": {} | ||
}, | ||
{ | ||
"key-auth": { | ||
"query": "apikey", | ||
"header": "authorization" | ||
} | ||
}, | ||
{ | ||
"jwt-auth": { | ||
"cookie": "jwt", | ||
"query": "jwt", | ||
"header": "authorization" | ||
} | ||
} | ||
] | ||
} | ||
}, | ||
"upstream": { | ||
"nodes": { | ||
"127.0.0.1:1980": 1 | ||
}, | ||
"type": "roundrobin" | ||
}, | ||
"uri": "/echo" | ||
}]] | ||
) | ||
|
||
if code >= 300 then | ||
ngx.status = code | ||
end | ||
ngx.say(body) | ||
} | ||
} | ||
--- request | ||
GET /t | ||
--- response_body | ||
passed | ||
|
||
|
||
|
||
=== TEST 24: verify key-auth using the same authorization header for jwt-auth | ||
--- request | ||
GET /echo | ||
--- more_headers | ||
Authorization: auth-one | ||
--- response_headers | ||
Authorization: auth-one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if these tests could be split to a new document, this is already long.
Some other questions, what are these tests to ensure? It doesn't look like your modifications matter much?
I don't see any settings or notes that are not the same as ensuring the plugin request header is configured, what should their behavior be? Pass the token into all plugins until any of them return?
t/plugin/multi-auth2.t
Outdated
--- request | ||
GET /t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
BTW, regarding the error in the Chinese documentation lint, you can try merging the master branches into your work branch, they are already fixed. |
6b7bbed
to
e047b6e
Compare
8e8976f
to
f0b84b7
Compare
f0b84b7
to
44300ce
Compare
@bzp2010 @shreemaan-abhishek Can you review again |
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions. |
@bzp2010 is this PR ready for merge? |
I just wanted to kindly check in on the status of PR #11256 . I understand there may be other priorities, but I was wondering if there’s anything that needs further clarification or adjustments on my part to help move it forward. |
Fixes #11069 11069
Checklist