Skip to content

fix: consumer key duplication check #12040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 25 commits into from
Closed

fix: consumer key duplication check #12040

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ce80722
feat: consumer key duplication check
Baoyuantop Mar 11, 2025
489a985
fix: improve code logic
Baoyuantop Mar 12, 2025
a3a4ac2
fix: lint
Baoyuantop Mar 12, 2025
fa3caa8
test: add test case
Baoyuantop Mar 12, 2025
ef0a8ea
fix: format code
Baoyuantop Mar 12, 2025
80ce431
fix: format code
Baoyuantop Mar 12, 2025
93a4b63
fix: test
Baoyuantop Mar 12, 2025
5e0fe0f
fix: lint code
Baoyuantop Mar 13, 2025
5660ad2
fix: test
Baoyuantop Mar 14, 2025
6cb6b78
fix: add log
Baoyuantop Mar 14, 2025
02daa2c
fix: format code
Baoyuantop Mar 17, 2025
3b2aa93
update
Baoyuantop Mar 21, 2025
7d2d930
fix: test
Baoyuantop Mar 25, 2025
7fe1827
fix: lint code
Baoyuantop Mar 25, 2025
38430a0
fix: remove error log
Baoyuantop Mar 27, 2025
95c2a92
fix: update code
Baoyuantop Mar 28, 2025
de68197
fix: lint
Baoyuantop Apr 1, 2025
b5efe91
fix: update code
Baoyuantop Apr 2, 2025
f5f883d
fix: update
Baoyuantop Apr 2, 2025
e3af265
fix: update
Baoyuantop Apr 2, 2025
3717039
update
Baoyuantop Apr 3, 2025
07b2eca
fix: merge function
Baoyuantop Apr 3, 2025
9caab8f
fix: lint code
Baoyuantop Apr 3, 2025
74d4219
fix: adjust function position
Baoyuantop Apr 27, 2025
5a528fa
fix
Baoyuantop Apr 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 47 additions & 2 deletions apisix/admin/consumers.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
local core = require("apisix.core")
local plugins = require("apisix.admin.plugins")
local resource = require("apisix.admin.resource")

local plugin = require("apisix.plugin")
local pairs = pairs
local consumer = require("apisix.consumer")

local function check_conf(username, conf, need_username, schema)
local ok, err = core.schema.check(schema, conf)
Expand All @@ -32,7 +34,50 @@ local function check_conf(username, conf, need_username, schema)
if conf.plugins then
ok, err = plugins.check_schema(conf.plugins, core.schema.TYPE_CONSUMER)
if not ok then
return nil, {error_msg = "invalid plugins configuration: " .. err}
return nil, {
error_msg = "invalid plugins configuration: " .. err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can keep this line unchanged.

}
end

-- check duplicate key
for plugin_name, plugin_conf in pairs(conf.plugins or {}) do
local plugin_obj = plugin.get(plugin_name)
if not plugin_obj then
return nil, {error_msg = "unknown plugin " .. plugin_name}
end

if plugin_obj.type == "auth" then
local decrypted_conf = core.table.deepcopy(plugin_conf)
plugin.decrypt_conf(plugin_name, decrypted_conf, core.schema.TYPE_CONSUMER)

local plugin_key_map = {
["key-auth"] = "key",
["basic-auth"] = "username",
["jwt-auth"] = "key",
["hmac-auth"] = "key_id"
}

local key_field = plugin_key_map[plugin_name]

if key_field then
local key_value = decrypted_conf[key_field]

if key_value then
local consumer, _, err = consumer
.find_consumer(plugin_name, key_field, key_value)
if err then
core.log.warn("failed to find consumer: ", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the logic here. In the find_consumer() method call above. We are checking if the given consumer key already exists, no? If it doesn't exist then the duplication check should be successful (no duplicate found), right?

If yes, then there is no point in printing a warn log.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When fixing the failure test, I found that when calling the find_consumer() method, its internal call will do a secret find and replace, and when given a non-existing secret, there will be an internal error log, where the err data is captured and printed.
It is also possible to remove this log.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can add this info as a comment in the code.

end

if consumer and consumer.username ~= conf.username then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Is it guaranteed that the returned consumer will have the field username in it? Different consumers have different key fields right? Take your own code snippet for example:
    image

  2. This block will execute only if a consumer with given key_field already exists. So, if it does, we should directly raise an error, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this is to allow the user to edit a pre-existing consumer. If the consuner being edited matches the username of the consumer being looked up, then the same consumer is being edited, and this behavior should be allowed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it. What about my first question?

Is it guaranteed that the returned consumer will have the field username in it? Different consumers have different key fields right? Take your own code snippet for example:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

username is a unique field for the consumer and must exist. https://apisix.apache.org/docs/apisix/terminology/consumer/#configuration-options

return nil, {
error_msg = "duplicate key found with consumer: "
.. consumer.username
}
end
end
end
end
end
end

Expand Down
39 changes: 37 additions & 2 deletions apisix/admin/credentials.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ local core = require("apisix.core")
local plugins = require("apisix.admin.plugins")
local plugin = require("apisix.plugin")
local resource = require("apisix.admin.resource")
local consumer = require("apisix.consumer")
local pairs = pairs

local function check_conf(_id, conf, _need_id, schema)
local function check_conf(id, conf, _need_id, schema)
local ok, err = core.schema.check(schema, conf)
if not ok then
return nil, {error_msg = "invalid configuration: " .. err}
Expand All @@ -32,14 +33,48 @@ local function check_conf(_id, conf, _need_id, schema)
return nil, {error_msg = "invalid plugins configuration: " .. err}
end

for name, _ in pairs(conf.plugins) do
for name, plugin_conf in pairs(conf.plugins) do
local plugin_obj = plugin.get(name)
if not plugin_obj then
return nil, {error_msg = "unknown plugin " .. name}
end

if plugin_obj.type ~= "auth" then
return nil, {error_msg = "only supports auth type plugins in consumer credential"}
end

-- check duplicate key
local decrypted_conf = core.table.deepcopy(plugin_conf)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are too many nested judgments inside, but I think we can extract this logic into a universal function that can be used by both consumers and credentials.

plugin.decrypt_conf(name, decrypted_conf, core.schema.TYPE_CONSUMER)

local plugin_key_map = {
["key-auth"] = "key",
["basic-auth"] = "username",
["jwt-auth"] = "key",
["hmac-auth"] = "key_id"
}

local key_field = plugin_key_map[name]
if key_field then
local key_value = decrypted_conf[key_field]

if key_value then
local consumer, _, err = consumer
.find_consumer(name, key_field, key_value)

if err then
core.log.warn("failed to find consumer: ", err)
end

if consumer and consumer.credential_id ~= id then
return nil, {
error_msg = "duplicate key found with consumer: "
.. consumer.username
}
end
end
end

end
end

Expand Down
83 changes: 83 additions & 0 deletions t/admin/consumers2.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,3 +174,86 @@ __DATA__
}
--- response_body
{"error_msg":"wrong username"}



=== TEST 6: create consumer
--- 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": "jack",
"desc": "key-auth for jack",
"plugins": {
"key-auth": {
"key": "the-key"
}
}
}]]
)
}
}
--- request
GET /t



=== TEST 7: duplicate consumer key, PUT
--- 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": "jack2",
"desc": "key-auth for jack2",
"plugins": {
"key-auth": {
"key": "the-key"
}
}
}]]
)

ngx.status = code
ngx.print(body)
}
}
--- request
GET /t
--- error_code: 400
--- response_body
{"error_msg":"duplicate key found with consumer: jack"}



=== TEST 8: update consumer jack
--- 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": "jack",
"desc": "key-auth for jack",
"plugins": {
"key-auth": {
"key": "the-key"
}
}
}]]
)

ngx.status = code
ngx.say(body)
}
}
--- request
GET /t
--- response_body
passed
139 changes: 137 additions & 2 deletions t/admin/credentials.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ passed
"desc": "basic-auth for jack",
"plugins": {
"basic-auth": {
"username": "the-user",
"username": "the-new-user",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After adding the duplicate judgment, it cannot be added here again, so I made a modification.

"password": "the-password"
}
}
Expand All @@ -119,7 +119,7 @@ passed
"value":{
"desc":"basic-auth for jack",
"id":"credential_a",
"plugins":{"basic-auth":{"username":"the-user","password":"WvF5kpaLvIzjuk4GNIMTJg=="}}
"plugins":{"basic-auth":{"username":"the-new-user","password":"WvF5kpaLvIzjuk4GNIMTJg=="}}
},
"key":"/apisix/consumers/jack/credentials/credential_a"
}]]
Expand Down Expand Up @@ -492,3 +492,138 @@ GET /t
--- error_code: 400
--- response_body
{"error_msg":"missing credential id"}



=== TEST 17: create a consumer bar
--- 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": "bar" }]])
}
}
--- request
GET /t



=== TEST 18: create a credential with key-auth for the consumer bar
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/consumers/bar/credentials/credential_c',
ngx.HTTP_PUT,
[[{
"desc": "key-auth for bar",
"plugins": {
"key-auth": {
"key": "the-key-bar"
}
}
}]]
)
}
}
--- request
GET /t



=== TEST 19: can not create a credential with duplicate key
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/consumers/bar/credentials/credential_d',
ngx.HTTP_PUT,
[[{
"desc": "key-auth for bar",
"plugins": {
"key-auth": {
"key": "the-key-bar"
}
}
}]]
)

ngx.status = code
ngx.print(body)
}
}
--- request
GET /t
--- error_code: 400
--- response_body
{"error_msg":"duplicate key found with consumer: bar"}



=== TEST 20: can update credential credential_c with same key
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test

-- update desc, keep same key
local code, body = t('/apisix/admin/consumers/bar/credentials/credential_c',
ngx.HTTP_PUT,
[[{
"desc": "new description",
"plugins": {
"key-auth": {
"key": "the-key-bar"
}
}
}]]
)

ngx.status = code
ngx.say(body)
}
}
--- request
GET /t
--- response_body
passed
--- error_code: 200



=== TEST 21: delete credential credential_c
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/consumers/bar/credentials/credential_c', ngx.HTTP_DELETE)
}
}
--- request
GET /t



=== TEST 22: delete consumer bar
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/consumers/bar', ngx.HTTP_DELETE)
}
}
--- request
GET /t



=== TEST 23: delete consumer jack
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/consumers/jack', ngx.HTTP_DELETE)
}
}
--- request
GET /t
2 changes: 2 additions & 0 deletions t/secret/aws.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,3 +314,5 @@ GET /t
}
--- response_body
all done
--- error_log
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The testing of these changes is an expected change. These tests have an admin api call to the consumer and use a non-existent secret reference, which in the new logic triggers a secret lookup failure and an error log. It didn't break the original test path.

failed to fetch secret value: no secret conf, secret_uri: $secret://aws/mysecret/jack/key
2 changes: 2 additions & 0 deletions t/secret/gcp.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,8 @@ kEJQcmfVew5mFXyxuEn3zA==
}
--- response_body
all done
--- error_log
failed to fetch secret value: no secret conf, secret_uri: $secret://gcp/mysecret/jack/key



Expand Down
2 changes: 2 additions & 0 deletions t/secret/secret_lru.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,5 @@ GET /t
}
--- response_body
nil
--- error_log
failed to fetch secret value: no secret conf, secret_uri: $secret://vault/mysecret/jack/auth-key
Loading