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/29 08:08:21 UTC
[GitHub] [apisix] liangliang4ward opened a new pull request #6745: fix: hide 5xx error message from client
liangliang4ward opened a new pull request #6745:
URL: https://github.com/apache/apisix/pull/6745
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
hidding 5xx error message from client
<!-- Please also include relevant motivation and context. -->
Fixes https://github.com/apache/apisix/issues/6699
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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 #6745: fix: hide 5xx error message from client
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #6745:
URL: https://github.com/apache/apisix/pull/6745#discussion_r838026879
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -581,17 +582,17 @@ local function evaluate_permissions(conf, ctx, token)
-- Ensure service account access token.
local sa_access_token, err = authz_keycloak_ensure_sa_access_token(conf)
if err then
- return 500, err
+ return 503
Review comment:
Missing a log here?
##########
File path: apisix/plugins/authz-casbin.lua
##########
@@ -112,7 +113,8 @@ function _M.rewrite(conf, ctx)
-- creates an enforcer when request sent for the first time
local ok, err = new_enforcer_if_need(conf)
if not ok then
- return 503, {message = err}
+ log.error(err)
Review comment:
We can use core.log.error directly?
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -581,17 +582,17 @@ local function evaluate_permissions(conf, ctx, token)
-- Ensure service account access token.
local sa_access_token, err = authz_keycloak_ensure_sa_access_token(conf)
if err then
- return 500, err
+ return 503
end
-- Resolve URI to resource(s).
- permission, err = authz_keycloak_resolve_resource(conf, ctx.var.request_uri,
Review comment:
We should not hide the err
--
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] liangliang4ward commented on a change in pull request #6745: fix: hide 5xx error message from client
Posted by GitBox <gi...@apache.org>.
liangliang4ward commented on a change in pull request #6745:
URL: https://github.com/apache/apisix/pull/6745#discussion_r837248900
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -572,7 +572,7 @@ local function evaluate_permissions(conf, ctx, token)
-- Ensure discovered data.
local err = authz_keycloak_ensure_discovered_data(conf)
if err then
- return 500, err
Review comment:
done
##########
File path: apisix/plugins/authz-casbin.lua
##########
@@ -110,9 +110,9 @@ end
function _M.rewrite(conf, ctx)
-- creates an enforcer when request sent for the first time
- local ok, err = new_enforcer_if_need(conf)
+ local ok = new_enforcer_if_need(conf)
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] shuaijinchao commented on a change in pull request #6745: fix: hide 5xx error message from client
Posted by GitBox <gi...@apache.org>.
shuaijinchao commented on a change in pull request #6745:
URL: https://github.com/apache/apisix/pull/6745#discussion_r838057174
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -581,17 +582,17 @@ local function evaluate_permissions(conf, ctx, token)
-- Ensure service account access token.
local sa_access_token, err = authz_keycloak_ensure_sa_access_token(conf)
if err then
- return 500, err
+ return 503
end
-- Resolve URI to resource(s).
permission, err = authz_keycloak_resolve_resource(conf, ctx.var.request_uri,
sa_access_token)
-- Check result.
- if permission == nil then
+ if permission == nil or err then
-- No result back from resource registration endpoint.
- return 500, err
+ return 503
Review comment:
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] moonming commented on a change in pull request #6745: fix: hide 5xx error message from client
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #6745:
URL: https://github.com/apache/apisix/pull/6745#discussion_r837252027
##########
File path: t/plugin/authz-keycloak.t
##########
@@ -337,8 +339,7 @@ passed
GET /t
--- response_body
false
---- error_log
Review comment:
I think we also need to check the error 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] tokers commented on a change in pull request #6745: fix: hide 5xx error message from client
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #6745:
URL: https://github.com/apache/apisix/pull/6745#discussion_r837242850
##########
File path: apisix/plugins/authz-casbin.lua
##########
@@ -110,9 +110,9 @@ end
function _M.rewrite(conf, ctx)
-- creates an enforcer when request sent for the first time
- local ok, err = new_enforcer_if_need(conf)
+ local ok = new_enforcer_if_need(conf)
Review comment:
We need to log the error message or we as the admin cannot know the details about the `503` requests.
##########
File path: apisix/plugins/authz-keycloak.lua
##########
@@ -572,7 +572,7 @@ local function evaluate_permissions(conf, ctx, token)
-- Ensure discovered data.
local err = authz_keycloak_ensure_discovered_data(conf)
if err then
- return 500, err
Review comment:
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