nic-6443 commented on code in PR #13929:
URL: https://github.com/apache/apisix/pull/13929#discussion_r4407903414
##########
apisix/plugins/wolf-rbac.lua:
##########
@@ -158,6 +192,29 @@ function _M.check_schema(conf)
end
+local function get_route_plugin_conf(ctx)
+ for i = 1, #ctx.plugins, 2 do
+ if ctx.plugins[i].name == plugin_name then
+ return ctx.plugins[i + 1]
+ end
+ end
+
+ return core.response.exit(500, fail_response("Missing wolf-rbac
configuration"))
+end
Review Comment:
I'd drop `get_route_plugin_conf`. No other plugin with public APIs finds its
config by scanning `ctx.plugins`, and it only sees the top-level list, so
login/change_pwd/user_info return 500 whenever wolf-rbac is nested in
multi-auth or configured as a global rule.
`rewrite` already runs on the public API route (that's what
`is_wolf_public_api` is for), so it's simpler to stash the conf there and let
the handlers read it:
```lua
-- in rewrite, right after clearing the headers
ctx.wolf_rbac_conf = conf
if is_wolf_public_api(ctx) then
return
end
-- in the handlers
local conf = ctx.wolf_rbac_conf
if not conf then
return core.response.exit(500, fail_response("Missing wolf-rbac
configuration"))
end
```
That works for the nested and global rule cases too, since the conf comes
from whichever wolf-rbac instance actually ran.
##########
apisix/plugins/wolf-rbac.lua:
##########
@@ -31,39 +32,68 @@ local req_read_body = ngx.req.read_body
local req_get_body_data = ngx.req.get_body_data
local plugin_name = "wolf-rbac"
+local default_server = "http://127.0.0.1:12180";
+local default_header_prefix = "X-"
local schema = {
type = "object",
properties = {
- appid = {
- type = "string",
- default = "unset"
- },
server = {
type = "string",
- default = "http://127.0.0.1:12180";
+ default = default_server
},
Review Comment:
Since we're redesigning the schema anyway, I'd make `server` required
instead of defaulting to `http://127.0.0.1:12180`. That default is only right
when Wolf runs next to the gateway; otherwise a route that forgets `server`
silently sends every permission check to localhost and fails at request time,
instead of being rejected by the Admin API. Same reasoning as making `appid`
required. With that, `default_server` and the `or default_server` fallbacks at
L344/L472/L514/L529 go away.
For `header_prefix` I don't see why the `or default_header_prefix` fallbacks
(L85, L366) are needed either. multi-auth's `check_schema` calls
`auth.check_schema(auth_plugin_conf, auth.schema)` on every nested conf, which
goes through the route schema here and fills in the defaults in place, and the
DP runs the same check when it loads the route. What does TEST 28 fail with
without them? If it really is needed, resolving it once at the top of `rewrite`
is better than fallbacks scattered across the file.
##########
t/plugin/wolf-rbac.t:
##########
@@ -77,7 +86,24 @@ done
-=== TEST 3: setup public API route
+=== TEST 3b: appid is required in Consumer configuration
Review Comment:
Same here, leftover `3b`.
##########
apisix/plugins/wolf-rbac.lua:
##########
@@ -158,6 +192,29 @@ function _M.check_schema(conf)
end
+local function get_route_plugin_conf(ctx)
+ for i = 1, #ctx.plugins, 2 do
+ if ctx.plugins[i].name == plugin_name then
+ return ctx.plugins[i + 1]
+ end
+ end
+
+ return core.response.exit(500, fail_response("Missing wolf-rbac
configuration"))
+end
+
+
+local function is_wolf_public_api(ctx)
+ for i = 1, #ctx.plugins, 2 do
Review Comment:
This crashes when wolf-rbac is configured as a global rule. The global
rules' rewrite runs before `api_ctx.plugins` is assigned (in `init.lua`,
`run_global_rules(..., "rewrite")` comes before `api_ctx.plugins = plugins`,
and global rule plugins live in `api_ctx.global_plugins`), so `#ctx.plugins`
errors on nil and every request gets a 500. That setup works on master.
Needs a nil guard here, plus a test with wolf-rbac on a global rule.
##########
t/plugin/multi-auth.t:
##########
@@ -655,3 +655,231 @@ GET /t
code: 401
--- no_error_log
[error]
+
+
+
+=== TEST 23: add a Wolf consumer and configure Wolf before key-auth
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/consumers',
+ ngx.HTTP_PUT,
+ [[{
+ "username": "wolf-client",
+ "plugins": {
+ "wolf-rbac": {
+ "appid": "multi-auth-wolf-app"
+ }
+ }
+ }]]
+ )
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(body)
+ return
+ end
+
+ code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "plugins": {
+ "multi-auth": {
+ "auth_plugins": [
+ {
+ "wolf-rbac": {
+ "server": "http://127.0.0.1:1982";,
+ "header_prefix": "X-Wolf-"
+ }
+ },
+ {
+ "key-auth": {}
+ }
+ ]
+ },
+ "serverless-post-function": {
+ "phase": "access",
+ "functions": [
+ "return function(conf, ctx) local core =
require(\"apisix.core\"); local names = {\"X-Wolf-UserId\",
\"X-Wolf-Username\", \"X-Wolf-Nickname\", \"X-Consumer-Username\"}; local
values = {}; for i, name in ipairs(names) do values[i] =
core.request.header(ctx, name) or \"nil\"; end; core.response.exit(200,
table.concat(values, \",\")); end"
+ ]
+ }
+ },
+ "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
+
+
+
+=== TEST 24: Wolf clears its Route header namespace before fallback
+--- request
+GET /hello
+--- more_headers
+apikey: auth-one
+X-Wolf-UserId: admin-001
+X-Wolf-Username: admin
+X-Wolf-Nickname: administrator
+--- response_body eval
+"nil,nil,nil,foo"
+
+
+
+=== TEST 25b: malformed Wolf token does not preserve Route identity headers
Review Comment:
Leftover `25b` from the reindex, please renumber.
##########
apisix/plugins/wolf-rbac.lua:
##########
@@ -239,6 +296,14 @@ end
function _M.rewrite(conf, ctx)
+ if not auth_utils.is_running_under_multi_auth(ctx) then
+ clear_identity_headers(conf, ctx)
+ end
Review Comment:
Now that `header_prefix` lives on the route, this can be unconditional:
clear the route-prefix headers as the very first thing in `rewrite`, before any
early return. Clearing is idempotent, so running it again after multi-auth's
pre-clear is harmless, and that lets us drop the `is_running_under_multi_auth`
branch, the `clear_identity_headers` wrapper (L84-94) and the `auth_utils`
require. Something like:
```lua
local identity_headers = {"UserId", "Username", "Nickname"}
function _M.clear_auth_headers(conf, ctx)
for _, name in ipairs(identity_headers) do
core.request.set_header(ctx, conf.header_prefix .. name, nil)
end
end
function _M.rewrite(conf, ctx)
_M.clear_auth_headers(conf, ctx)
...
```
The `clear_auth_headers` hook in multi-auth should stay though, since
wolf-rbac never runs at all when an earlier auth plugin succeeds.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
