Skip to content
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.

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

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

Merged
merged 7 commits into from
Mar 8, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
59 changes: 55 additions & 4 deletions apisix/plugins/consumer-restriction.lua
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
--
local ipairs = ipairs
local core = require("apisix.core")

local ngx = ngx
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 it is better to align with the above format, for example:

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

local schema = {
type = "object",
oneOf = {
Expand Down Expand Up @@ -53,6 +53,38 @@ local schema = {
rejected_code = {type = "integer", minimum = 200, default = 403}
},
required = {"whitelist"},
},
{
title = "allowed_methods",
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to use allowed_by_methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree

properties = {
type = {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

type = "string",
enum = {"consumer_name", "service_id"},
default = "consumer_name"
},
whitelist = {
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}
},
required = {"allowed_methods"},
}
}
}
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if value.user == user then
for k,allowed_method in ipairs(value.methods) do
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

if allowed_method == method then
return true
end
end
return false
end
end
return true
end

function _M.check_schema(conf)
local ok, err = core.schema.check(schema, conf)

if not ok then
return false, err
end

if ((conf.allowed_methods and #conf.allowed_methods > 0) and not conf.whitelist ) then
return false, "allowed_methods set but no whitelist provided"
end
return true
end


function _M.access(conf, ctx)
local value = fetch_val_funcs[conf.type](ctx)
local method = ngx.req.get_method()
if not value then
return 401, { message = "Missing authentication or identity verification."}
end
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

"is_method_allowed"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree

block = true
end
end
end

if block then
return conf.rejected_code, { message = "The " .. conf.type .. " is forbidden." }
end
end


return _M
75 changes: 75 additions & 0 deletions docs/en/latest/plugins/consumer-restriction.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ The `consumer-restriction` makes corresponding access restrictions based on diff
| whitelist | array[string] | required | | | Choose one of the two with `blacklist`, only whitelist or blacklist can be enabled separately, and the two cannot be used together. |
| blacklist | array[string] | required | | | Choose one of the two with `whitelist`, only whitelist or blacklist can be enabled separately, and the two cannot be used together. |
| rejected_code | integer | optional | 403 | [200,...] | The HTTP status code returned when the request is rejected. |
| allowed_methods | array[object] | optional | | | Set a list of allowed_methods for users declared in the whitelist section |

For the `type` field is an enumerated type, it can be `consumer_name` or `service_id`. They stand for the following meanings:

Expand Down Expand Up @@ -116,6 +117,80 @@ HTTP/1.1 403 Forbidden
{"message":"The consumer_name is forbidden."}
```

### How to restrict `allowed_methods`

This example restrict the user `jack1` to only `POST` on the resource

```shell
curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
{
"uri": "/index.html",
"upstream": {
"type": "roundrobin",
"nodes": {
"127.0.0.1:1980": 1
}
},
"plugins": {
"basic-auth": {},
"consumer-restriction": {
"whitelist": [
Copy link
Member

Choose a reason for hiding this comment

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

        "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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jp-gouin jp-gouin Mar 2, 2021

Choose a reason for hiding this comment

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

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 ?
            }]
        }

Copy link
Member

Choose a reason for hiding this comment

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

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"}}
    },
}

Copy link
Member

Choose a reason for hiding this comment

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

We can change the title of

=== TEST 2: whitelist and blacklist mutual exclusive
and say that the priority is blacklist > whitelist > allowed_by_methods.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"jack1"
],
"allowed_methods":[{
"user": "jack1",
"methods": ["POST"]
}]
}
}
}'
```

**Test Plugin**

Requests from jack1:

```shell
curl -u jack2019:123456 http://127.0.0.1:9080/index.html
HTTP/1.1 403 Forbidden
...
{"message":"The consumer_name is forbidden."}
```

Add the capability for `jack1` to get the resource

```shell
curl http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -X PUT -d '
{
"uri": "/index.html",
"upstream": {
"type": "roundrobin",
"nodes": {
"127.0.0.1:1980": 1
}
},
"plugins": {
"basic-auth": {},
"consumer-restriction": {
"whitelist": [
"jack1"
],
"allowed_methods":[{
"user": "jack1",
"methods": ["POST","GET"]
}]
}
}
}'
```

Requests from jack1:

```shell
curl -u jack2019:123456 http://127.0.0.1:9080/index.html
HTTP/1.1 200 OK
```

## How to restrict `service_id`

The `service_id` method needs to be used together with the authorization plug-in. Here, the key-auth authorization plug-in is taken as an example.
Expand Down
Loading