feat: Add filter on HTTP methods for consumer-restriction plugin #3691

jp-gouin · 2021-02-26T16:12:33Z

What this PR does / why we need it:

Fix for #3635
Add the possibility to add allowed_methods for a user when whitelist is set.
This will restrict the user to only performed the HTTP action matching the list specified in allowed_method.methods

This is the format :

"plugins": {
        "key-auth": {},
        "consumer-restriction": {
            "whitelist": [
                "jack1"
            ],
            "allowed_methods":[{
              "user": "jack1",
              "methods": ["POST"]
            }]
        }
    }

I choose to add a dedicated section instead of modifying the existing whitelist one because i think it's more readable.
When nothing is set , then only the whitelist is applied .
And in order to set allowed_methods , whitelist is required.

Pre-submission checklist:

Test case updated with 2 basics tests

only allow post on resources and try to get the resource -> unauthorized
Add getcapability to the user and try to get the resource -> authorized

Add 2 test cases for testing the plugin scheme

Only allowed_methods is set -> failed
allowed_methods is set with blacklist -> failed

English documentation updated , any help for the Chineese one would be appreciated.

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

… associated and update documentation (en)

spacewander · 2021-03-01T01:26:51Z

apisix/plugins/consumer-restriction.lua

@@ -114,12 +161,16 @@ function _M.access(conf, ctx)
        if not is_include(value, conf.whitelist) then
            block = true
        end
+        if conf.allowed_methods and #conf.allowed_methods > 0 then
+            if not is_method_allow(conf.allowed_methods, method, value) then


"is_method_allowed"

spacewander · 2021-03-01T01:27:53Z

apisix/plugins/consumer-restriction.lua

+        {
+            title = "allowed_methods",
+            properties = {
+                type = {


Can we extra this part and share across the components with different titles?

Sorry, I'm not sure to understand what you mean by extra

Sorry, it is my typo. It should be extract.

spacewander · 2021-03-01T01:28:28Z

apisix/plugins/consumer-restriction.lua

@@ -53,6 +53,38 @@ local schema = {
                rejected_code = {type = "integer", minimum = 200, default = 403}
            },
            required = {"whitelist"},
+        },
+        {
+            title = "allowed_methods",


Would it be better to use allowed_by_methods?

spacewander · 2021-03-01T01:34:34Z

docs/en/latest/plugins/consumer-restriction.md

+    "plugins": {
+        "basic-auth": {},
+        "consumer-restriction": {
+            "whitelist": [


"consumer-restriction": { "allowed_by_methods":[{ "user": "jack1", "methods": ["POST"] }] }

is enough. There is no need to configure jack1 in two places. And the allow already has the meaning of whitelist.

Ok so i completely drop the whitelist part in favor of allowed_by_methods ?

If so that would mean some rework of the whole plugin.
Right now the allowed_by_methods required to set the list of methods , so if we drop whitelistwe need a quick setup . Like if there is no method , then full access ? or the user still has to set the list of method but we take into account a * wildcard ?

"consumer-restriction": { "allowed_by_methods":[{ "user": "jack1", "methods": [] ---> Full access or full restriction ? }] }

"consumer-restriction": { "allowed_by_methods":[{ "user": "jack1", "methods": ["*"] ---> Full access instead of putting the full list ? }] }

I think we can use the schema below to allow both whitelist and allowed_by_methods:

local schema = { type = "object", properties = { type = { type = "string", enum = {"consumer_name", "service_id"}, default = "consumer_name" }, blacklist = { type = "array", minItems = 1, items = {type = "string"} }, whitelist = { type = "array", minItems = 1, items = {type = "string"} }, allowed_by_methods = { type = "array", items = { type = "object", properties = { user = { type = "string" }, methods = { type = "array", minItems = 1, items = { type = "string", enum = {"GET", "POST", "PUT", "DELETE", "PATCH", "HEAD", "OPTIONS", "CONNECT", "TRACE"}, } } } } }, rejected_code = {type = "integer", minimum = 200, default = 403} }, anyOf = { {required = {"blacklist"}}, {required = {"whitelist"}}, {required = {"allowed_by_methods"}} }, }

We can change the title of

apisix/t/plugin/consumer-restriction.t

Line 57 in 29b1e2d

=== TEST 2: whitelist and blacklist mutual exclusive

and say that the priority is blacklist > whitelist > allowed_by_methods.

It is a large change but I think it is flexible for the users.

Ok , no problem.
So allowed_by_methods could exist without whitelist.
If whitelist is set it will override what could be specified in the allowed_by_methods section

Firstsawyou · 2021-03-01T10:41:48Z

apisix/plugins/consumer-restriction.lua

@@ -16,7 +16,7 @@
 --
 local ipairs    = ipairs
 local core      = require("apisix.core")
-
+local ngx          = ngx


I think it is better to align with the above format, for example:

local ipairs = ipairs local core = require("apisix.core") local ngx = ngx

Firstsawyou · 2021-03-01T10:46:32Z

apisix/plugins/consumer-restriction.lua

@@ -84,20 +116,35 @@ local function is_include(value, tab)
    return false
 end

+local function is_method_allow(allowed_methods, method, user)
+    for key,value in ipairs(allowed_methods) do


There should be a space after ,, and key is not used, it should be replaced with a placeholder. like this:

for _, value in ipairs(allowed_methods) do

Firstsawyou · 2021-03-01T10:47:03Z

apisix/plugins/consumer-restriction.lua

+local function is_method_allow(allowed_methods, method, user)
+    for key,value in ipairs(allowed_methods) do
+        if value.user == user then
+            for k,allowed_method in ipairs(value.methods) do


…hod_allow to is_method_allowed, add indentation for import. replace unused key in pairs and ipairs call by _

…e all set , the priority is blacklist > whitelist > allowed_by_methods. update test case accordingly and update the documentation with this new behavior

jp-gouin · 2021-03-04T08:52:22Z

I made the following update :
blacklist , whitelist and allowed_by_methods can be set at the same time.
blacklist has the priority over whitelist and whitelist has the priority over allowed_by_methods.

There is now a direct reject of the connection if the user is blacklisted

if conf.blacklist and #conf.blacklist > 0 then
        if is_include(value, conf.blacklist) then
            return reject(conf)
        end
    end

allowed_by_methods is only performed if the user is not whitelisted

 if conf.allowed_by_methods and #conf.allowed_by_methods > 0 and not whitelisted then
        if not is_method_allowed(conf.allowed_by_methods, method, value) then
            block = true
        end
    end

The overall behavior stay the same is you set only blacklist and or whitelist

spacewander · 2021-03-04T11:19:07Z

t/plugin/consumer-restriction.t

+
+
+
+=== TEST 28: test blacklist priority


test whitelist priority

Ah yes my bad

spacewander · 2021-03-04T11:20:15Z

t/plugin/consumer-restriction.t

@@ -475,7 +474,7 @@ Authorization: Basic amFjazIwMjA6MTIzNDU2



-=== TEST 21: remove consumer-restriction


Why change this test?

I bump the remove restriction to test 31 due to the tests i added and to remains consistent .

jp-gouin added 4 commits February 26, 2021 16:45

Add http methods filter on consumer-restriction plugin, add test case…

d998793

… associated and update documentation (en)

update indentation and format test file

e30f03f

change pairs to ipairs and correct typo

a251b28

add EOL in consumer-restriction

b96ac7e

jp-gouin changed the title ~~Add filter on HTTP methods for consumer-restriction plugin~~ feat: Add filter on HTTP methods for consumer-restriction plugin Feb 26, 2021

spacewander requested changes Mar 1, 2021

View reviewed changes

Firstsawyou reviewed Mar 1, 2021

View reviewed changes

jp-gouin added 2 commits March 2, 2021 16:36

replace allowed_methods by allowed_by_methods, correct typo on is_met…

770e201

…hod_allow to is_method_allowed, add indentation for import. replace unused key in pairs and ipairs call by _

Add new scheme, now blacklist, whitelist and allowed_by_methods can b…

720926f

…e all set , the priority is blacklist > whitelist > allowed_by_methods. update test case accordingly and update the documentation with this new behavior

spacewander reviewed Mar 4, 2021

View reviewed changes

change title of test 28 blacklist -> whitelist

c62b512

spacewander approved these changes Mar 5, 2021

View reviewed changes

moonming approved these changes Mar 8, 2021

View reviewed changes

spacewander merged commit 6cbaf55 into apache:master Mar 8, 2021

monkeyDluffy6017 mentioned this pull request Feb 6, 2023

help request: about the field allowed_by_methods of the plugin consumer-restriction cause unexpected behavior #8792

Closed

ronething mentioned this pull request Feb 6, 2023

fix: the field allowed_by_methods of consumer-restriction #8795

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add filter on HTTP methods for consumer-restriction plugin #3691

feat: Add filter on HTTP methods for consumer-restriction plugin #3691

jp-gouin commented Feb 26, 2021

spacewander Mar 1, 2021

jp-gouin Mar 2, 2021

spacewander Mar 1, 2021

jp-gouin Mar 2, 2021

spacewander Mar 3, 2021

spacewander Mar 1, 2021

jp-gouin Mar 2, 2021

spacewander Mar 1, 2021

jp-gouin Mar 2, 2021

jp-gouin Mar 2, 2021 •

edited

Loading

spacewander Mar 3, 2021

spacewander Mar 3, 2021

spacewander Mar 3, 2021

jp-gouin Mar 3, 2021

Firstsawyou Mar 1, 2021

Firstsawyou Mar 1, 2021

Firstsawyou Mar 1, 2021

jp-gouin commented Mar 4, 2021

spacewander Mar 4, 2021

jp-gouin Mar 4, 2021

spacewander Mar 4, 2021

jp-gouin Mar 4, 2021

		@@ -475,7 +474,7 @@ Authorization: Basic amFjazIwMjA6MTIzNDU2



		=== TEST 21: remove consumer-restriction

feat: Add filter on HTTP methods for consumer-restriction plugin #3691

feat: Add filter on HTTP methods for consumer-restriction plugin #3691

Conversation

jp-gouin commented Feb 26, 2021

What this PR does / why we need it:

Pre-submission checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jp-gouin Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jp-gouin commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jp-gouin Mar 2, 2021 •

edited

Loading