You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/01/07 03:08:14 UTC
[GitHub] [apisix] mangoGoForward opened a new pull request #6039: feat: support hide the authentication header in basic-auth with a config
mangoGoForward opened a new pull request #6039:
URL: https://github.com/apache/apisix/pull/6039
### What this PR does / why we need it:
Resolves #5900
### Pre-submission checklist:
<!--
Please follow the PR manners:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. If you need to resolve merge conflicts after the PR is reviewed, please merge master but do not rebase
6. Use "request review" to notify the reviewer once you have resolved the review
7. Only reviewer can click "Resolve conversation" to mark the reviewer's review resolved
-->
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [x] Have you modified the corresponding document?
* [x] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
leslie-tsang commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780093960
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -30,7 +30,12 @@ local consumers_lrucache = core.lrucache.new({
local schema = {
type = "object",
title = "work with route or service object",
- properties = {},
+ properties = {
+ hide_auth_header = {
+ type = "boolean",
+ default = true,
Review comment:
```suggestion
default = true,
```
Ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] imjoey commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
imjoey commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r782815357
##########
File path: docs/en/latest/plugins/basic-auth.md
##########
@@ -39,10 +39,18 @@ For more information on Basic authentication, refer to [Wiki](https://en.wikiped
## Attributes
-| Name | Type | Requirement | Default | Valid | Description |
-| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
-| password | string | required | | | the user's password |
+For consumer side:
+
+| Name | Type | Requirement | Default | Valid | Description |
+| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
+| password | string | required | | | the user's password |
Review comment:
@mangoGoForward IMHO there's no need to change this. How about revert it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] imjoey edited a comment on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
imjoey edited a comment on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1012695403
@mangoGoForward The changes look good to me. Could you explain why you committed several other commits after Zexuan's approval? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1012277585
Strange, I had no problem checking the format of this test case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780092367
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -30,7 +30,12 @@ local consumers_lrucache = core.lrucache.new({
local schema = {
type = "object",
title = "work with route or service object",
- properties = {},
+ properties = {
+ hide_auth_header = {
+ type = "boolean",
+ default = true,
Review comment:
I think the default value should be false.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780092367
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -30,7 +30,12 @@ local consumers_lrucache = core.lrucache.new({
local schema = {
type = "object",
title = "work with route or service object",
- properties = {},
+ properties = {
+ hide_auth_header = {
+ type = "boolean",
+ default = true,
Review comment:
I think the default value should be false.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r781137587
##########
File path: t/plugin/basic-auth.t
##########
@@ -356,3 +356,55 @@ GET /t
GET /t
--- no_error_log
[error]
+
+
+
+=== TEST 15: enable basic auth plugin using admin api, set hide_credentials = true
+--- 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": {
+ "basic-auth": {
+ "hide_credentials": true
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 16: verify
Review comment:
we can mock an upstream, and check headers in upstream, take a look at: https://github.com/apache/apisix/blob/70174d58cd4248f55dada5c2bc7396cdf73972c7/t/plugin/proxy-mirror.t#L35-L55
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780054968
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -161,6 +170,11 @@ function _M.rewrite(conf, ctx)
return 401, { message = "Password is error" }
end
+ -- 5. hide `Authentication` header if `hide_auth_header` is `true`
+ if conf.hide_auth_header == true then
+ core.response.set_header("Authentication", "")
Review comment:
It seems you misunderstand the original issue. We want to hide the request header.
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -30,7 +30,12 @@ local consumers_lrucache = core.lrucache.new({
local schema = {
type = "object",
title = "work with route or service object",
- properties = {},
+ properties = {
+ hide_auth_header = {
Review comment:
`hide_credentials` would be better? Kong uses this field in their basic-auth.
##########
File path: docs/en/latest/plugins/basic-auth.md
##########
@@ -39,10 +39,11 @@ For more information on Basic authentication, refer to [Wiki](https://en.wikiped
## Attributes
-| Name | Type | Requirement | Default | Valid | Description |
-| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
-| password | string | required | | | the user's password |
+| Name | Type | Requirement | Default | Valid | Description |
+| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
+| password | string | required | | | the user's password |
+| hide_auth_header | boolean | optional | false | | Whether to return the Authentication request headers to the upstream. |
Review comment:
Let's distinguish route conf from the consumer's like this one: https://github.com/apache/apisix/blob/master/docs/en/latest/plugins/ldap-auth.md#attributes
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -39,6 +44,10 @@ local consumer_schema = {
properties = {
username = { type = "string" },
password = { type = "string" },
+ hide_auth_header = {
Review comment:
We don't need to configure it in the consumer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
leslie-tsang commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780092059
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -39,6 +44,10 @@ local consumer_schema = {
properties = {
username = { type = "string" },
password = { type = "string" },
+ hide_auth_header = {
+ type = "boolean",
+ default = true,
Review comment:
```suggestion
default = false,
```
Need to discuss it in maillist if use `true` as default value
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -161,6 +170,11 @@ function _M.rewrite(conf, ctx)
return 401, { message = "Password is error" }
end
+ -- 5. hide `Authentication` request header if `hide_auth_header` is `true`
+ if conf.hide_auth_header == true then
+ core.request.set_header(ctx, "Authentication", "")
+ end
+
Review comment:
The original issue seems to avoid to send the header to upstream.
##########
File path: docs/zh/latest/plugins/basic-auth.md
##########
@@ -43,6 +43,7 @@ title: basic-auth
| -------- | ------ | ------ | ------ | ------ | ------------------------------------------------------------------------------------------------------------------ |
| username | string | 必须 | | | 不同的 `consumer` 对象应有不同的值，它应当是唯一的。不同 consumer 使用了相同的 `username` ，将会出现请求匹配异常。 |
| password | string | 必须 | | | 用户的密码 |
+| hide_auth_header | boolean | 可选 | true | | 是否将 Authentication 请求头返回给客户端. |
Review comment:
```suggestion
| hide_auth_header | boolean | 可选 | false | | 是否将 Authentication 请求头传递给 upstream。 |
```
##########
File path: docs/en/latest/plugins/basic-auth.md
##########
@@ -39,10 +39,11 @@ For more information on Basic authentication, refer to [Wiki](https://en.wikiped
## Attributes
-| Name | Type | Requirement | Default | Valid | Description |
-| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
-| password | string | required | | | the user's password |
+| Name | Type | Requirement | Default | Valid | Description |
+| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
+| password | string | required | | | the user's password |
+| hide_auth_header | boolean | optional | true | | Whether to return the Authentication request headers to the client. |
Review comment:
```suggestion
| Name | Type | Requirement | Default | Valid | Description |
| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
| password | string | required | | | the user's password |
| hide_auth_header | boolean | optional | true | | Whether to return the Authentication request headers to the upstream. |
```
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -30,7 +30,12 @@ local consumers_lrucache = core.lrucache.new({
local schema = {
type = "object",
title = "work with route or service object",
- properties = {},
+ properties = {
+ hide_auth_header = {
+ type = "boolean",
+ default = true,
Review comment:
```suggestion
default = true,
```
Ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780160420
##########
File path: docs/zh/latest/plugins/basic-auth.md
##########
@@ -43,6 +43,7 @@ title: basic-auth
| -------- | ------ | ------ | ------ | ------ | ------------------------------------------------------------------------------------------------------------------ |
| username | string | 必须 | | | 不同的 `consumer` 对象应有不同的值，它应当是唯一的。不同 consumer 使用了相同的 `username` ，将会出现请求匹配异常。 |
| password | string | 必须 | | | 用户的密码 |
+| hide_auth_header | boolean | 可选 | true | | 是否将 Authentication 请求头返回给客户端. |
Review comment:
Done.
##########
File path: docs/en/latest/plugins/basic-auth.md
##########
@@ -39,10 +39,11 @@ For more information on Basic authentication, refer to [Wiki](https://en.wikiped
## Attributes
-| Name | Type | Requirement | Default | Valid | Description |
-| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
-| password | string | required | | | the user's password |
+| Name | Type | Requirement | Default | Valid | Description |
+| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
+| password | string | required | | | the user's password |
+| hide_auth_header | boolean | optional | true | | Whether to return the Authentication request headers to the client. |
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1011864512
> @mangoGoForward Please make lint pass.
That's strange, it's good when I run `reindex` on local(MacOS) and Centos environment, please see the screenshot below:
I will test again on ubantu later, If you know where the error location, please help me to improve, Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r781038423
##########
File path: t/plugin/basic-auth.t
##########
@@ -356,3 +356,55 @@ GET /t
GET /t
--- no_error_log
[error]
+
+
+
+=== TEST 15: enable basic auth plugin using admin api, set hide_credentials = true
+--- 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": {
+ "basic-auth": {
+ "hide_credentials": true
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 16: verify
Review comment:
I am ashamed not found such parse in project of test-nginx, how could we detect request header?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] imjoey commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
imjoey commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1012695403
@mangoGoForward The changes look good to me. Could you explain why you committed several other commits? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1010752480
> @mangoGoForward Hi, as @spacewander suggested, you could use `/echo` instead of `httpbin.org` to check the headers. Please refer to
>
> https://github.com/apache/apisix/blob/d4a7ea208fa748ac52a9a77d82d018e1fb971307/t/plugin/zipkin.t#L419
>
> .
Thanks, have changed, please see 25602a7.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] imjoey commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
imjoey commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r782848396
##########
File path: t/plugin/basic-auth.t
##########
@@ -20,6 +20,7 @@ repeat_each(2);
no_long_string();
no_root_location();
no_shuffle();
+log_level('info');
Review comment:
@mangoGoForward please take a check whether this change is needed. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1015092301
>
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780067867
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -161,6 +170,11 @@ function _M.rewrite(conf, ctx)
return 401, { message = "Password is error" }
end
+ -- 5. hide `Authentication` header if `hide_auth_header` is `true`
+ if conf.hide_auth_header == true then
+ core.response.set_header("Authentication", "")
Review comment:
My fault. Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r782794683
##########
File path: t/plugin/basic-auth.t
##########
@@ -20,6 +20,46 @@ repeat_each(2);
no_long_string();
no_root_location();
no_shuffle();
+log_level('info');
+worker_connections(1024);
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ my $http_config = $block->http_config // <<_EOC_;
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r782828919
##########
File path: docs/en/latest/plugins/basic-auth.md
##########
@@ -39,10 +39,18 @@ For more information on Basic authentication, refer to [Wiki](https://en.wikiped
## Attributes
-| Name | Type | Requirement | Default | Valid | Description |
-| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
-| password | string | required | | | the user's password |
+For consumer side:
+
+| Name | Type | Requirement | Default | Valid | Description |
+| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
+| password | string | required | | | the user's password |
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1008587189
> and need to add test cases to cover this. I see that you have test cases in your previous submission records……
Because I misunderstand the original issue, and those test cases seems incorrect, so I removed. Now test cases updated in 210c9d5, please give me some suggestion if you have a free times, Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1012700078
> @mangoGoForward The changes look good to me. Could you explain why you committed several other commits? Thanks.
I want to trigger the actions of my forked repo, so I commit a PR to my repo's master branch, and got file conflict when merged from master, so i commit 8440045 to resolved. but the master branch's code is polluted, so I revert the commit to 49b7850, and I also checked those file changes, it is same as 49b7850, I'm sorry about troubled your review work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780054968
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -161,6 +170,11 @@ function _M.rewrite(conf, ctx)
return 401, { message = "Password is error" }
end
+ -- 5. hide `Authentication` header if `hide_auth_header` is `true`
+ if conf.hide_auth_header == true then
+ core.response.set_header("Authentication", "")
Review comment:
It seems you misunderstand the original issue. We want to hide the request header.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780161145
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -161,6 +170,11 @@ function _M.rewrite(conf, ctx)
return 401, { message = "Password is error" }
end
+ -- 5. hide `Authentication` request header if `hide_auth_header` is `true`
+ if conf.hide_auth_header == true then
+ core.request.set_header(ctx, "Authentication", "")
+ end
+
Review comment:
Yes, use `core.request.set_header(ctx, "Authentication", nil)` can avoid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r781057214
##########
File path: t/plugin/basic-auth.t
##########
@@ -356,3 +356,55 @@ GET /t
GET /t
--- no_error_log
[error]
+
+
+
+=== TEST 15: enable basic auth plugin using admin api, set hide_credentials = true
+--- 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": {
+ "basic-auth": {
+ "hide_credentials": true
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 16: verify
Review comment:
The `response_headers` data section can be used to validate response header entries, but `Authorization` is request header, so I don't know whether `response_headers` can used in this test case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] imjoey commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
imjoey commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1010701965
@mangoGoForward Hi, as @spacewander suggested, you could use `/echo` instead of `httpbin.org` to check the headers. Please refer to https://github.com/apache/apisix/blob/d4a7ea208fa748ac52a9a77d82d018e1fb971307/t/plugin/zipkin.t#L419.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1011910634
I run `reindex` to check test code style on `Ubuntu Server 20.04 LTS`, also is right, so strangely. please see:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r782854693
##########
File path: t/plugin/basic-auth.t
##########
@@ -20,6 +20,7 @@ repeat_each(2);
no_long_string();
no_root_location();
no_shuffle();
+log_level('info');
Review comment:
Seems unnecessary, removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1012681593
> Strange, I had no problem checking the format of this test case.
I have run workflow in my fork repo, it also failed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] imjoey commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
imjoey commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1012717572
> > @mangoGoForward The changes look good to me. Could you explain why you committed several other commits? Thanks.
>
> I want to trigger the actions of my forked repo, so I commit a PR to my repo's master branch, and got file conflict when merged from master, so i commit [8440045](https://github.com/apache/apisix/commit/8440045f020e33466ae296401e12b185c707d6e7) to resolved. but the master branch's code is polluted, so I revert the commit to [49b7850](https://github.com/apache/apisix/commit/49b7850f3c91d03398ad753fa9074499e04800eb), and I also checked those file changes, it is same as [49b7850](https://github.com/apache/apisix/commit/49b7850f3c91d03398ad753fa9074499e04800eb), I'm sorry about troubled your review work.
@mangoGoForward Got it and thanks. Let's wait for the CI. :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] imjoey commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
imjoey commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1012725793
@spacewander @mangoGoForward The CI job failure of `linux_openresty on ubuntu-18.04` seemed not related to the code and I restarted that job. I would prefer to leave an approval in prior and merge this after that job passed. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #6039:
URL: https://github.com/apache/apisix/pull/6039
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780860306
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -39,6 +44,10 @@ local consumer_schema = {
properties = {
username = { type = "string" },
password = { type = "string" },
+ hide_auth_header = {
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r781306825
##########
File path: t/plugin/basic-auth.t
##########
@@ -356,3 +356,55 @@ GET /t
GET /t
--- no_error_log
[error]
+
+
+
+=== TEST 15: enable basic auth plugin using admin api, set hide_credentials = true
+--- 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": {
+ "basic-auth": {
+ "hide_credentials": true
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 16: verify
Review comment:
Done. please see cb53193
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r781690551
##########
File path: t/plugin/basic-auth.t
##########
@@ -20,6 +20,46 @@ repeat_each(2);
no_long_string();
no_root_location();
no_shuffle();
+log_level('info');
+worker_connections(1024);
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ my $http_config = $block->http_config // <<_EOC_;
Review comment:
We already have an upstream for this, see: https://github.com/apache/apisix/blob/70174d58cd4248f55dada5c2bc7396cdf73972c7/t/lib/server.lua#L392
Use /echo is enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780067867
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -161,6 +170,11 @@ function _M.rewrite(conf, ctx)
return 401, { message = "Password is error" }
end
+ -- 5. hide `Authentication` header if `hide_auth_header` is `true`
+ if conf.hide_auth_header == true then
+ core.response.set_header("Authentication", "")
Review comment:
My fault. Done.
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -30,7 +30,12 @@ local consumers_lrucache = core.lrucache.new({
local schema = {
type = "object",
title = "work with route or service object",
- properties = {},
+ properties = {
+ hide_auth_header = {
+ type = "boolean",
+ default = true,
Review comment:
Done.
##########
File path: docs/zh/latest/plugins/basic-auth.md
##########
@@ -43,6 +43,7 @@ title: basic-auth
| -------- | ------ | ------ | ------ | ------ | ------------------------------------------------------------------------------------------------------------------ |
| username | string | 必须 | | | 不同的 `consumer` 对象应有不同的值，它应当是唯一的。不同 consumer 使用了相同的 `username` ，将会出现请求匹配异常。 |
| password | string | 必须 | | | 用户的密码 |
+| hide_auth_header | boolean | 可选 | true | | 是否将 Authentication 请求头返回给客户端. |
Review comment:
Done.
##########
File path: docs/en/latest/plugins/basic-auth.md
##########
@@ -39,10 +39,11 @@ For more information on Basic authentication, refer to [Wiki](https://en.wikiped
## Attributes
-| Name | Type | Requirement | Default | Valid | Description |
-| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
-| password | string | required | | | the user's password |
+| Name | Type | Requirement | Default | Valid | Description |
+| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
+| password | string | required | | | the user's password |
+| hide_auth_header | boolean | optional | true | | Whether to return the Authentication request headers to the client. |
Review comment:
Done.
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -161,6 +170,11 @@ function _M.rewrite(conf, ctx)
return 401, { message = "Password is error" }
end
+ -- 5. hide `Authentication` request header if `hide_auth_header` is `true`
+ if conf.hide_auth_header == true then
+ core.request.set_header(ctx, "Authentication", "")
+ end
+
Review comment:
Yes, use `core.request.set_header(ctx, "Authentication", nil)` can avoid.
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -30,7 +30,12 @@ local consumers_lrucache = core.lrucache.new({
local schema = {
type = "object",
title = "work with route or service object",
- properties = {},
+ properties = {
+ hide_auth_header = {
+ type = "boolean",
+ default = true,
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780160318
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -30,7 +30,12 @@ local consumers_lrucache = core.lrucache.new({
local schema = {
type = "object",
title = "work with route or service object",
- properties = {},
+ properties = {
+ hide_auth_header = {
+ type = "boolean",
+ default = true,
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780161369
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -30,7 +30,12 @@ local consumers_lrucache = core.lrucache.new({
local schema = {
type = "object",
title = "work with route or service object",
- properties = {},
+ properties = {
+ hide_auth_header = {
+ type = "boolean",
+ default = true,
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1011845215
@mangoGoForward
Please make lint pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward edited a comment on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward edited a comment on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1011864512
> @mangoGoForward Please make lint pass.
That's strange, it's good when I run `reindex` on local(MacOS) and Centos environment, please see the screenshot below:
I will test again on ubuntu later, If you know the error's location, please help me to improve, Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r781024044
##########
File path: t/plugin/basic-auth.t
##########
@@ -356,3 +356,55 @@ GET /t
GET /t
--- no_error_log
[error]
+
+
+
+=== TEST 15: enable basic auth plugin using admin api, set hide_credentials = true
+--- 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": {
+ "basic-auth": {
+ "hide_credentials": true
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 16: verify
Review comment:
Can we strengthen this case that we can make sure the header is really removed, rather than just checking out the log.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780273110
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -30,7 +30,12 @@ local consumers_lrucache = core.lrucache.new({
local schema = {
type = "object",
title = "work with route or service object",
- properties = {},
+ properties = {
+ hide_auth_header = {
Review comment:
`hide_credentials` would be better? Kong uses this field in their basic-auth.
##########
File path: docs/en/latest/plugins/basic-auth.md
##########
@@ -39,10 +39,11 @@ For more information on Basic authentication, refer to [Wiki](https://en.wikiped
## Attributes
-| Name | Type | Requirement | Default | Valid | Description |
-| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
-| password | string | required | | | the user's password |
+| Name | Type | Requirement | Default | Valid | Description |
+| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
+| password | string | required | | | the user's password |
+| hide_auth_header | boolean | optional | false | | Whether to return the Authentication request headers to the upstream. |
Review comment:
Let's distinguish route conf from the consumer's like this one: https://github.com/apache/apisix/blob/master/docs/en/latest/plugins/ldap-auth.md#attributes
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -39,6 +44,10 @@ local consumer_schema = {
properties = {
username = { type = "string" },
password = { type = "string" },
+ hide_auth_header = {
Review comment:
We don't need to configure it in the consumer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
leslie-tsang commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r781044936
##########
File path: t/plugin/basic-auth.t
##########
@@ -356,3 +356,55 @@ GET /t
GET /t
--- no_error_log
[error]
+
+
+
+=== TEST 15: enable basic auth plugin using admin api, set hide_credentials = true
+--- 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": {
+ "basic-auth": {
+ "hide_credentials": true
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 16: verify
Review comment:
Try `response_headers` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780926146
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -161,6 +166,11 @@ function _M.rewrite(conf, ctx)
return 401, { message = "Password is error" }
end
+ -- 5. hide `Authentication` request header if `hide_credentials` is `true`
+ if conf.hide_credentials == true then
Review comment:
Done.
##########
File path: docs/en/latest/plugins/basic-auth.md
##########
@@ -39,11 +39,18 @@ For more information on Basic authentication, refer to [Wiki](https://en.wikiped
## Attributes
+For consumer side:
+
| Name | Type | Requirement | Default | Valid | Description |
| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
| password | string | required | | | the user's password |
-| hide_auth_header | boolean | optional | false | | Whether to return the Authentication request headers to the upstream. |
+
+For route side:
+
+| Name | Type | Requirement | Default | Valid | Description |
+| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| hide_credentials | boolean | optional | false | | Whether to return the Authentication request headers to the upstream. |
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] lijing-21 commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
lijing-21 commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1015014485
Hi @mangoGoForward , thank you for your contribution!
Here is the Contributor T-shirt form[1], if you're interested, kindly take a look :)
[1] https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#contributor-t-shirt
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward edited a comment on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward edited a comment on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1011864512
> @mangoGoForward Please make lint pass.
That's strange, it's good when I run `reindex` on local(MacOS) and Centos environment, please see the screenshot below:
I will test again on ubantu later, If you know the error's location, please help me to improve, Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1015093509
> Hi @mangoGoForward , thank you for your contribution!
>
> Here is the Contributor T-shirt form[1], if you're interested, kindly take a look :)
>
> [1] https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#contributor-t-shirt
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward removed a comment on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward removed a comment on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1015092301
>
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780860328
##########
File path: docs/en/latest/plugins/basic-auth.md
##########
@@ -39,10 +39,11 @@ For more information on Basic authentication, refer to [Wiki](https://en.wikiped
## Attributes
-| Name | Type | Requirement | Default | Valid | Description |
-| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
-| password | string | required | | | the user's password |
+| Name | Type | Requirement | Default | Valid | Description |
+| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
+| password | string | required | | | the user's password |
+| hide_auth_header | boolean | optional | false | | Whether to return the Authentication request headers to the upstream. |
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] mangoGoForward commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
mangoGoForward commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780860240
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -30,7 +30,12 @@ local consumers_lrucache = core.lrucache.new({
local schema = {
type = "object",
title = "work with route or service object",
- properties = {},
+ properties = {
+ hide_auth_header = {
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780888438
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -161,6 +166,11 @@ function _M.rewrite(conf, ctx)
return 401, { message = "Password is error" }
end
+ -- 5. hide `Authentication` request header if `hide_credentials` is `true`
+ if conf.hide_credentials == true then
Review comment:
```suggestion
if conf.hide_credentials then
```
is ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#issuecomment-1008539717
and need to add test cases to cover this. I see that you have test cases in your previous submission records……
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780908837
##########
File path: docs/en/latest/plugins/basic-auth.md
##########
@@ -39,11 +39,18 @@ For more information on Basic authentication, refer to [Wiki](https://en.wikiped
## Attributes
+For consumer side:
+
| Name | Type | Requirement | Default | Valid | Description |
| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
| password | string | required | | | the user's password |
-| hide_auth_header | boolean | optional | false | | Whether to return the Authentication request headers to the upstream. |
+
+For route side:
+
+| Name | Type | Requirement | Default | Valid | Description |
+| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| hide_credentials | boolean | optional | false | | Whether to return the Authentication request headers to the upstream. |
Review comment:
```suggestion
| hide_credentials | boolean | optional | false | | Whether to pass the Authentication request headers to the upstream. |
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang commented on a change in pull request #6039: feat: support hide the authentication header in basic-auth with a config
Posted by GitBox <gi...@apache.org>.
leslie-tsang commented on a change in pull request #6039:
URL: https://github.com/apache/apisix/pull/6039#discussion_r780092059
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -39,6 +44,10 @@ local consumer_schema = {
properties = {
username = { type = "string" },
password = { type = "string" },
+ hide_auth_header = {
+ type = "boolean",
+ default = true,
Review comment:
```suggestion
default = false,
```
Need to discuss it in maillist if use `true` as default value
##########
File path: apisix/plugins/basic-auth.lua
##########
@@ -161,6 +170,11 @@ function _M.rewrite(conf, ctx)
return 401, { message = "Password is error" }
end
+ -- 5. hide `Authentication` request header if `hide_auth_header` is `true`
+ if conf.hide_auth_header == true then
+ core.request.set_header(ctx, "Authentication", "")
+ end
+
Review comment:
The original issue seems to avoid to send the header to upstream.
##########
File path: docs/zh/latest/plugins/basic-auth.md
##########
@@ -43,6 +43,7 @@ title: basic-auth
| -------- | ------ | ------ | ------ | ------ | ------------------------------------------------------------------------------------------------------------------ |
| username | string | 必须 | | | 不同的 `consumer` 对象应有不同的值，它应当是唯一的。不同 consumer 使用了相同的 `username` ，将会出现请求匹配异常。 |
| password | string | 必须 | | | 用户的密码 |
+| hide_auth_header | boolean | 可选 | true | | 是否将 Authentication 请求头返回给客户端. |
Review comment:
```suggestion
| hide_auth_header | boolean | 可选 | false | | 是否将 Authentication 请求头传递给 upstream。 |
```
##########
File path: docs/en/latest/plugins/basic-auth.md
##########
@@ -39,10 +39,11 @@ For more information on Basic authentication, refer to [Wiki](https://en.wikiped
## Attributes
-| Name | Type | Requirement | Default | Valid | Description |
-| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
-| password | string | required | | | the user's password |
+| Name | Type | Requirement | Default | Valid | Description |
+| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
+| password | string | required | | | the user's password |
+| hide_auth_header | boolean | optional | true | | Whether to return the Authentication request headers to the client. |
Review comment:
```suggestion
| Name | Type | Requirement | Default | Valid | Description |
| -------- | ------ | ----------- | ------- | ----- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| username | string | required | | | Different `consumer` should have different value which is unique. When different `consumer` use a same `username`, a request matching exception would be raised. |
| password | string | required | | | the user's password |
| hide_auth_header | boolean | optional | true | | Whether to return the Authentication request headers to the upstream. |
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org