From 3b10c9ee89034e38077498017ef0cda0b80d965d Mon Sep 17 00:00:00 2001 From: Zhefeng Chen Date: Mon, 17 Jun 2024 17:33:34 +0800 Subject: [PATCH] feat(acl): added a config `always_use_authenticated_groups` to support using authenticated groups even when an authenticated consumer exists. Currently, authenticated groups will only be used when there is no consumer or the consumer is anonymous. When there is an authenticated consumer, there is no way to use authenticated groups, only the groups associated with the consumer will be used. This PR adds a config `always_use_authenticated_groups` to support using authenticated groups even when an authenticated consumer exists. If enabled, it will first try to use authenticated groups and will fallback to use the groups associated with the consumer if authenticated groups don't exist, which is consistent with the logic in the anonymous consumer case. https://konghq.atlassian.net/browse/FTI-5945 --- .../acl-always-use-authenticated-groups.yml | 3 + kong/clustering/compat/removed_fields.lua | 3 + kong/plugins/acl/handler.lua | 2 +- kong/plugins/acl/schema.lua | 1 + .../03-db/08-declarative_spec.lua | 2 + .../09-hybrid_mode/09-config-compat_spec.lua | 40 ++++++ spec/03-plugins/18-acl/02-access_spec.lua | 134 ++++++++++++++++++ 7 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 changelog/unreleased/kong/acl-always-use-authenticated-groups.yml diff --git a/changelog/unreleased/kong/acl-always-use-authenticated-groups.yml b/changelog/unreleased/kong/acl-always-use-authenticated-groups.yml new file mode 100644 index 000000000000..a2da56e2fc68 --- /dev/null +++ b/changelog/unreleased/kong/acl-always-use-authenticated-groups.yml @@ -0,0 +1,3 @@ +message: "**acl:** Added a new config `always_use_authenticated_groups` to support using authenticated groups even when an authenticated consumer already exists." +type: feature +scope: Plugin diff --git a/kong/clustering/compat/removed_fields.lua b/kong/clustering/compat/removed_fields.lua index 4b0ac6e48631..78465f47268b 100644 --- a/kong/clustering/compat/removed_fields.lua +++ b/kong/clustering/compat/removed_fields.lua @@ -167,5 +167,8 @@ return { "traces_endpoint", "logs_endpoint", }, + acl = { + "always_use_authenticated_groups", + }, }, } diff --git a/kong/plugins/acl/handler.lua b/kong/plugins/acl/handler.lua index 803fa472ad1e..af4f04b36f07 100644 --- a/kong/plugins/acl/handler.lua +++ b/kong/plugins/acl/handler.lua @@ -80,7 +80,7 @@ function ACLHandler:access(conf) else local credential = kong.client.get_credential() local authenticated_groups - if not credential then + if not credential or conf.always_use_authenticated_groups then -- authenticated groups overrides anonymous groups authenticated_groups = groups.get_authenticated_groups() end diff --git a/kong/plugins/acl/schema.lua b/kong/plugins/acl/schema.lua index df0afc638edf..14a70e67ba5f 100644 --- a/kong/plugins/acl/schema.lua +++ b/kong/plugins/acl/schema.lua @@ -16,6 +16,7 @@ return { type = "array", elements = { type = "string" }, }, }, { hide_groups_header = { type = "boolean", required = true, default = false, description = "If enabled (`true`), prevents the `X-Consumer-Groups` header from being sent in the request to the upstream service." }, }, + { always_use_authenticated_groups = { type = "boolean", required = true, default = false, description = "If enabled (`true`), the authenticated groups will always be used even when an authenticated consumer already exists. If the authenticated groups don't exist, it will fallback to use the groups associated with the consumer. By default the authenticated groups will only be used when there is no consumer or the consumer is anonymous." } }, }, } } diff --git a/spec/02-integration/03-db/08-declarative_spec.lua b/spec/02-integration/03-db/08-declarative_spec.lua index 4bfc44f16507..9c6b80af2e1c 100644 --- a/spec/02-integration/03-db/08-declarative_spec.lua +++ b/spec/02-integration/03-db/08-declarative_spec.lua @@ -132,6 +132,7 @@ for _, strategy in helpers.each_strategy() do deny = ngx.null, allow = { "*" }, hide_groups_header = false, + always_use_authenticated_groups = false, } } @@ -146,6 +147,7 @@ for _, strategy in helpers.each_strategy() do deny = ngx.null, allow = { "*" }, hide_groups_header = false, + always_use_authenticated_groups = false, } } diff --git a/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua b/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua index 8473bdf3b528..19b972a2ca79 100644 --- a/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua +++ b/spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua @@ -769,6 +769,46 @@ describe("CP/DP config compat transformations #" .. strategy, function() admin.plugins:remove({ id = rt.id }) end) end) + + describe("compatibility test for acl plugin", function() + it("removes `config.always_use_authenticated_groups` before sending them to older(less than 3.8.0.0) DP nodes", function() + local acl = admin.plugins:insert { + name = "acl", + enabled = true, + config = { + allow = { "admin" }, + -- [[ new fields 3.8.0 + always_use_authenticated_groups = true, + -- ]] + } + } + + assert.not_nil(acl.config.always_use_authenticated_groups) + local expected_acl = cycle_aware_deep_copy(acl) + expected_acl.config.always_use_authenticated_groups = nil + do_assert(uuid(), "3.7.0", expected_acl) + + -- cleanup + admin.plugins:remove({ id = acl.id }) + end) + + it("does not remove `config.always_use_authenticated_groups` from DP nodes that are already compatible", function() + local acl = admin.plugins:insert { + name = "acl", + enabled = true, + config = { + allow = { "admin" }, + -- [[ new fields 3.8.0 + always_use_authenticated_groups = true, + -- ]] + } + } + do_assert(uuid(), "3.8.0", acl) + + -- cleanup + admin.plugins:remove({ id = acl.id }) + end) + end) end) end) diff --git a/spec/03-plugins/18-acl/02-access_spec.lua b/spec/03-plugins/18-acl/02-access_spec.lua index 157fc2afcf7b..8b69f0f24346 100644 --- a/spec/03-plugins/18-acl/02-access_spec.lua +++ b/spec/03-plugins/18-acl/02-access_spec.lua @@ -80,6 +80,20 @@ for _, strategy in helpers.each_strategy() do consumer = { id = consumer4.id }, } + local consumer5 = bp.consumers:insert { + username = "consumer5" + } + + bp.keyauth_credentials:insert { + key = "apikey127", + consumer = { id = consumer5.id }, + } + + bp.acls:insert { + group = "acl_group1", + consumer = { id = consumer5.id }, + } + local anonymous = bp.consumers:insert { username = "anonymous" } @@ -739,6 +753,86 @@ for _, strategy in helpers.each_strategy() do } } + local route15 = bp.routes:insert({ + hosts = { "acl15.test" } + }) + + bp.plugins:insert { + name = "acl", + route = { id = route15.id }, + config = { + allow = { "auth_group1" }, + hide_groups_header = false, + always_use_authenticated_groups = true, + } + } + + bp.plugins:insert { + name = "key-auth", + route = { id = route15.id }, + config = {} + } + + bp.plugins:insert { + name = "ctx-checker", + route = { id = route15.id }, + config = { + ctx_kind = "kong.ctx.shared", + ctx_set_field = "authenticated_groups", + ctx_set_array = { "auth_group1" }, + } + } + + local route16 = bp.routes:insert({ + hosts = { "acl16.test" } + }) + + bp.plugins:insert { + name = "acl", + route = { id = route16.id }, + config = { + allow = { "auth_group1" }, + hide_groups_header = false, + always_use_authenticated_groups = true, + } + } + + bp.plugins:insert { + name = "key-auth", + route = { id = route16.id }, + config = {} + } + + bp.plugins:insert { + name = "ctx-checker", + route = { id = route16.id }, + config = { + ctx_kind = "kong.ctx.shared", + ctx_set_field = "authenticated_groups", + ctx_set_array = { "auth_group2" }, + } + } + + local route17 = bp.routes:insert({ + hosts = { "acl17.test" } + }) + + bp.plugins:insert { + name = "acl", + route = { id = route17.id }, + config = { + allow = { "acl_group1" }, + hide_groups_header = false, + always_use_authenticated_groups = true, + } + } + + bp.plugins:insert { + name = "key-auth", + route = { id = route17.id }, + config = {} + } + assert(helpers.start_kong({ plugins = "bundled, ctx-checker", database = strategy, @@ -1380,6 +1474,46 @@ for _, strategy in helpers.each_strategy() do assert.res_status(200, res) end) end) + + describe("always_use_authenticated_groups", function() + it("if authenticated_groups is set, it'll be used", function() + local res = assert(proxy_client:get("/request?apikey=apikey127", { + headers = { + ["Host"] = "acl15.test" + } + })) + local body = assert(cjson.decode(assert.res_status(200, res))) + assert.equal("auth_group1", body.headers["x-authenticated-groups"]) + assert.equal(nil, body.headers["x-consumer-groups"]) + + res = assert(proxy_client:get("/request?apikey=apikey127", { + headers = { + ["Host"] = "acl16.test" + } + })) + body = assert(cjson.decode(assert.res_status(403, res))) + assert.matches("You cannot consume this service", body.message) + end) + + it("if authenticated_groups is not set, fallback to use acl groups", function() + local res = assert(proxy_client:get("/request?apikey=apikey127", { + headers = { + ["Host"] = "acl17.test" + } + })) + local body = assert(cjson.decode(assert.res_status(200, res))) + assert.equal(nil, body.headers["x-authenticated-groups"]) + assert.equal("acl_group1", body.headers["x-consumer-groups"]) + + local res = assert(proxy_client:get("/request?apikey=apikey126", { + headers = { + ["Host"] = "acl17.test" + } + })) + body = assert(cjson.decode(assert.res_status(403, res))) + assert.matches("You cannot consume this service", body.message) + end) + end) end)