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/03/02 06:14:29 UTC
[GitHub] [apisix] oil-oil opened a new pull request #6485: feat: authz-keycloak plugin support redirect
oil-oil opened a new pull request #6485:
URL: https://github.com/apache/apisix/pull/6485
### What this PR does / why we need it:
Added access_denied_redirect_uri property to the authz-keycloak plugin, if this property is added to the configuration, it will redirect to the configured uri when permission is empty and enforcement mode is "ENFORCING"
resolve #6460
### 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?
* [ ] 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] oil-oil commented on a change in pull request #6485: feat: authz-keycloak plugin support redirect not authorized user
Posted by GitBox <gi...@apache.org>.
oil-oil commented on a change in pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#discussion_r820232993
##########
File path: docs/zh/latest/plugins/authz-keycloak.md
##########
@@ -49,6 +49,8 @@ title: authz-keycloak
| refresh_token_expires_leeway| integer | 可选 | 0 | [0, ...] | refresh token 提前更新时间（秒，如果设置了此值，允许在该时间段内使用相同的 refresh token 令牌来解决潜在的网络并发问题） |
| ssl_verify | boolean | 可选 | true | [0, ...] | 验证 SSL 证书与主机名是否匹配 |
| policy_enforcement_mode | string | 可选 | "ENFORCING" | ["ENFORCING", "PERMISSIVE"] | |
+| access_denied_redirect_uri | string | 可选 | | [1, 2048] |当访问失败时重定向的 uri
Review comment:
> Should update the zh one
Sorry, I forgot to sync Chinese. Now I have updated it. Thank you for your reminder. 😀
--
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 #6485: feat: authz-keycloak plugin support redirect not authorized user
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#discussion_r820381123
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -592,9 +593,17 @@ local function evaluate_permissions(conf, ctx, token)
permission = conf.permissions
end
- -- Return 403 if permission is empty and enforcement mode is "ENFORCING".
+ -- Return 403 or 302 if permission is empty and enforcement mode is "ENFORCING".
if #permission == 0 and conf.policy_enforcement_mode == "ENFORCING" then
-- Return Keycloak-style message for consistency.
+ if conf.access_denied_redirect_uri then
+ core.response.set_header("Location", conf.access_denied_redirect_uri)
+ if ctx.var.request_method == "POST" then
+ return 307
+ else
+ return 302
Review comment:
Would be better to use 307 in all cases?
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -592,9 +593,17 @@ local function evaluate_permissions(conf, ctx, token)
permission = conf.permissions
end
- -- Return 403 if permission is empty and enforcement mode is "ENFORCING".
+ -- Return 403 or 302 if permission is empty and enforcement mode is "ENFORCING".
Review comment:
Need to update
--
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] juzhiyuan commented on a change in pull request #6485: feat: authz-keycloak plugin support redirect not authorized user
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#discussion_r821377757
##########
File path: docs/en/latest/plugins/authz-keycloak.md
##########
@@ -63,6 +63,7 @@ For more information on Keycloak, refer to [Keycloak Authorization Docs](https:/
| keepalive | boolean | optional | true | | Enable HTTP keep-alive to keep connections open after use. Set to `true` if you expect a lot of requests to Keycloak. |
| keepalive_timeout | integer | optional | 60000 | positive integer >= 1000 | Idle timeout after which established HTTP connections will be closed. |
| keepalive_pool | integer | optional | 5 | positive integer >= 1 | Maximum number of connections in the connection pool. |
+| access_denied_redirect_uri | string | optional | | [1, 2048] | Redirect not authorized user with the given uri like "http://127.0.0.1/test", instead of returning `"error_description":"not_authorized"`. |
Review comment:
```suggestion
| access_denied_redirect_uri | string | optional | | [1, 2048] | Redirect unauthorized user with the given uri like "http://127.0.0.1/test", instead of returning `"error_description":"not_authorized"`. |
```
##########
File path: docs/zh/latest/plugins/authz-keycloak.md
##########
@@ -49,6 +49,8 @@ title: authz-keycloak
| refresh_token_expires_leeway| integer | 可选 | 0 | [0, ...] | refresh token 提前更新时间（秒，如果设置了此值，允许在该时间段内使用相同的 refresh token 令牌来解决潜在的网络并发问题） |
| ssl_verify | boolean | 可选 | true | [0, ...] | 验证 SSL 证书与主机名是否匹配 |
| policy_enforcement_mode | string | 可选 | "ENFORCING" | ["ENFORCING", "PERMISSIVE"] | |
+| access_denied_redirect_uri | string | 可选 | | [1, 2048] |未授权的用户不会返回 `"error_description":"not_authorized"`，而是会定重定向至给定的uri，如 "http://127.0.0.1/test"
Review comment:
```suggestion
| access_denied_redirect_uri | string | 可选 | | [1, 2048] |未授权的用户不会返回 `"error_description":"not_authorized"`，而是会重定向至给定的 uri，如 "http://127.0.0.1/test"
```
--
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 #6485: feat: authz-keycloak plugin support redirect
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#discussion_r819240005
##########
File path: docs/en/latest/plugins/authz-keycloak.md
##########
@@ -63,6 +63,7 @@ For more information on Keycloak, refer to [Keycloak Authorization Docs](https:/
| keepalive | boolean | optional | true | | Enable HTTP keep-alive to keep connections open after use. Set to `true` if you expect a lot of requests to Keycloak. |
| keepalive_timeout | integer | optional | 60000 | positive integer >= 1000 | Idle timeout after which established HTTP connections will be closed. |
| keepalive_pool | integer | optional | 5 | positive integer >= 1 | Maximum number of connections in the connection pool. |
+| access_denied_redirect_uri | string | optional | | [1, 2048] | Uri redirected when access fails.|
Review comment:
```suggestion
| access_denied_redirect_uri | string | optional | | [1, 2048] | Redirect not authorized user with the given uri like "http://127.0.0.1/test", instead of returning `"error_description":"not_authorized"`. |
```
--
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 #6485: feat: authz-keycloak plugin support redirect
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#discussion_r818234419
##########
File path: docs/en/latest/plugins/authz-keycloak.md
##########
@@ -63,6 +63,8 @@ For more information on Keycloak, refer to [Keycloak Authorization Docs](https:/
| keepalive | boolean | optional | true | | Enable HTTP keep-alive to keep connections open after use. Set to `true` if you expect a lot of requests to Keycloak. |
| keepalive_timeout | integer | optional | 60000 | positive integer >= 1000 | Idle timeout after which established HTTP connections will be closed. |
| keepalive_pool | integer | optional | 5 | positive integer >= 1 | Maximum number of connections in the connection pool. |
+| keepalive_pool | integer | optional | 5 | positive integer >= 1 | Maximum number of connections in the connection pool. |
Review comment:
Duplicate?
##########
File path: t/plugin/authz-keycloak.t
##########
@@ -551,3 +551,99 @@ GET /t
--- response_body
{"error":"access_denied","error_description":"not_authorized"}
--- no_error_log
+
+
+
+=== TEST 16: set enforcement mode is "ENFORCING", lazy_load_paths and permissions use default values , access_denied_redirect_uri is "http://127.0.0.1/test"
+--- 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": {
+ "authz-keycloak": {
+ "token_endpoint": "http://127.0.0.1:8443/auth/realms/University/protocol/openid-connect/token",
+ "client_id": "course_management",
+ "grant_type": "urn:ietf:params:oauth:grant-type:uma-ticket",
+ "policy_enforcement_mode": "ENFORCING",
+ "timeout": 3000,
+ "access_denied_redirect_uri": "http://127.0.0.1/test"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello1"
+ }]],
+ [[{
+ "node": {
+ "value": {
+ "plugins": {
+ "authz-keycloak": {
+ "token_endpoint": "http://127.0.0.1:8443/auth/realms/University/protocol/openid-connect/token",
+ "client_id": "course_management",
+ "grant_type": "urn:ietf:params:oauth:grant-type:uma-ticket",
+ "policy_enforcement_mode": "ENFORCING",
+ "timeout": 3000,
+ "access_denied_redirect_uri": "http://127.0.0.1/test"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello1"
+ },
+ "key": "/apisix/routes/1"
+ },
+ "action": "set"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 17: test for permission is empty and enforcement mode is "ENFORCING" , access_denied_redirect_uri is "http://127.0.0.1/test".
+--- config
+ location /t {
+ content_by_lua_block {
+ local http = require "resty.http"
+ local httpc = http.new()
+ local uri = "http://127.0.0.1:" .. ngx.var.server_port .. "/hello1"
+ local res, err = httpc:request_uri(uri, {
+ method = "GET",
+ headers = {
+ ["Authorization"] = "Bearer " .. "fake access token",
+ }
+ })
+ if res.status >= 300 then
+ ngx.status = res.status
+ ngx.header["Location"] = res.headers["Location"]
+ end
+ }
+ }
+--- request
+GET /t
+--- response_headers
+Location: http://127.0.0.1/test
+--- error_code: 302
+--- no_error_log
Review comment:
Empty `--- no_error_log`
##########
File path: t/plugin/authz-keycloak.t
##########
@@ -551,3 +551,99 @@ GET /t
--- response_body
{"error":"access_denied","error_description":"not_authorized"}
--- no_error_log
+
+
+
+=== TEST 16: set enforcement mode is "ENFORCING", lazy_load_paths and permissions use default values , access_denied_redirect_uri is "http://127.0.0.1/test"
+--- 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": {
+ "authz-keycloak": {
+ "token_endpoint": "http://127.0.0.1:8443/auth/realms/University/protocol/openid-connect/token",
+ "client_id": "course_management",
+ "grant_type": "urn:ietf:params:oauth:grant-type:uma-ticket",
+ "policy_enforcement_mode": "ENFORCING",
+ "timeout": 3000,
+ "access_denied_redirect_uri": "http://127.0.0.1/test"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1982": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello1"
+ }]],
+ [[{
+ "node": {
Review comment:
There is no need to check the response data, please don't do 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] starsz commented on a change in pull request #6485: feat: authz-keycloak plugin support redirect
Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#discussion_r819199362
##########
File path: docs/en/latest/plugins/authz-keycloak.md
##########
@@ -63,6 +63,7 @@ For more information on Keycloak, refer to [Keycloak Authorization Docs](https:/
| keepalive | boolean | optional | true | | Enable HTTP keep-alive to keep connections open after use. Set to `true` if you expect a lot of requests to Keycloak. |
| keepalive_timeout | integer | optional | 60000 | positive integer >= 1000 | Idle timeout after which established HTTP connections will be closed. |
| keepalive_pool | integer | optional | 5 | positive integer >= 1 | Maximum number of connections in the connection pool. |
+| access_denied_redirect_uri | string | optional | | [1, 2048] | Uri redirected when access fails.|
Review comment:
Better to align the `|`
--
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] guoqqqi commented on a change in pull request #6485: feat: authz-keycloak plugin support redirect
Posted by GitBox <gi...@apache.org>.
guoqqqi commented on a change in pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#discussion_r817501466
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -592,9 +593,13 @@ local function evaluate_permissions(conf, ctx, token)
permission = conf.permissions
end
- -- Return 403 if permission is empty and enforcement mode is "ENFORCING".
+ -- Return 403 or 302 if permission is empty and enforcement mode is "ENFORCING".
if #permission == 0 and conf.policy_enforcement_mode == "ENFORCING" then
-- Return Keycloak-style message for consistency.
+ if conf.access_denied_redirect_uri then
+ core.response.set_header("Location", conf.access_denied_redirect_uri)
Review comment:
```suggestion
if conf.access_denied_redirect_uri then
core.response.set_header("Location", conf.access_denied_redirect_uri)
```
--
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 #6485: feat: authz-keycloak plugin support redirect not authorized user
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#discussion_r820217967
##########
File path: docs/zh/latest/plugins/authz-keycloak.md
##########
@@ -49,6 +49,8 @@ title: authz-keycloak
| refresh_token_expires_leeway| integer | 可选 | 0 | [0, ...] | refresh token 提前更新时间（秒，如果设置了此值，允许在该时间段内使用相同的 refresh token 令牌来解决潜在的网络并发问题） |
| ssl_verify | boolean | 可选 | true | [0, ...] | 验证 SSL 证书与主机名是否匹配 |
| policy_enforcement_mode | string | 可选 | "ENFORCING" | ["ENFORCING", "PERMISSIVE"] | |
+| access_denied_redirect_uri | string | 可选 | | [1, 2048] |当访问失败时重定向的 uri
Review comment:
Should update the zh one
--
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] oil-oil commented on a change in pull request #6485: feat: authz-keycloak plugin support redirect not authorized user
Posted by GitBox <gi...@apache.org>.
oil-oil commented on a change in pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#discussion_r820252213
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -592,9 +593,13 @@ local function evaluate_permissions(conf, ctx, token)
permission = conf.permissions
end
- -- Return 403 if permission is empty and enforcement mode is "ENFORCING".
+ -- Return 403 or 302 if permission is empty and enforcement mode is "ENFORCING".
if #permission == 0 and conf.policy_enforcement_mode == "ENFORCING" then
-- Return Keycloak-style message for consistency.
+ if conf.access_denied_redirect_uri then
+ core.response.set_header("Location", conf.access_denied_redirect_uri)
+ return 302
Review comment:
> I noticed that there is no judgment request method here. And if the request method is `POST`, 302 status code can not achieve the expected effect, 307 might be more appropriate.
Thank you for your correction. I'll update 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] soulbird commented on a change in pull request #6485: feat: authz-keycloak plugin support redirect not authorized user
Posted by GitBox <gi...@apache.org>.
soulbird commented on a change in pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#discussion_r820174477
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -592,9 +593,13 @@ local function evaluate_permissions(conf, ctx, token)
permission = conf.permissions
end
- -- Return 403 if permission is empty and enforcement mode is "ENFORCING".
+ -- Return 403 or 302 if permission is empty and enforcement mode is "ENFORCING".
if #permission == 0 and conf.policy_enforcement_mode == "ENFORCING" then
-- Return Keycloak-style message for consistency.
+ if conf.access_denied_redirect_uri then
+ core.response.set_header("Location", conf.access_denied_redirect_uri)
+ return 302
Review comment:
I noticed that there is no judgment request method here. And if the request method is `POST`, 302 status code can not achieve the expected effect, 307 might be more appropriate.
--
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 #6485: feat: authz-keycloak plugin support redirect not authorized user
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #6485:
URL: https://github.com/apache/apisix/pull/6485
--
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] oil-oil commented on pull request #6485: feat: authz-keycloak plugin support redirect not authorized user
Posted by GitBox <gi...@apache.org>.
oil-oil commented on pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#issuecomment-1060612848
@spacewander How can I modify the code to pass the test ? 😭
--
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] oil-oil commented on pull request #6485: feat: authz-keycloak plugin support redirect not authorized user
Posted by GitBox <gi...@apache.org>.
oil-oil commented on pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#issuecomment-1060131866
> Wrong submodule added.
I deleted the submodule, please help me re-execute 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] spacewander commented on a change in pull request #6485: feat: authz-keycloak plugin support redirect not authorized user
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6485:
URL: https://github.com/apache/apisix/pull/6485#discussion_r821261376
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -592,9 +593,13 @@ local function evaluate_permissions(conf, ctx, token)
permission = conf.permissions
end
- -- Return 403 if permission is empty and enforcement mode is "ENFORCING".
+ -- Return 403 or 307 if permission is empty and enforcement mode is "ENFORCING".
if #permission == 0 and conf.policy_enforcement_mode == "ENFORCING" then
-- Return Keycloak-style message for consistency.
+ if conf.access_denied_redirect_uri then
+ core.response.set_header("Location", conf.access_denied_redirect_uri)
+ return 307
Review comment:
```suggestion
return 307
```
--
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