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

Request validation plugin #1709

Merged
merged 17 commits into from
Jul 21, 2020
Merged

Request validation plugin #1709

merged 17 commits into from
Jul 21, 2020

Conversation

sshniro
Copy link
Member

@sshniro sshniro commented Jun 13, 2020

Resolves #1643
The plugin uses the json-schema validator to validate requests before sending them to upstream.
This plugin can be used to validate the header and body data.

@sshniro
Copy link
Member Author

sshniro commented Jun 13, 2020

@membphis , @moonming should I write more test cases for different types of schema validations or rely on the JSON schema validator itself?

Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

Add test case: Use illegal JSON request.

apisix/plugins/request-validation.lua Outdated Show resolved Hide resolved
apisix/plugins/request-validation.lua Outdated Show resolved Hide resolved
apisix/plugins/request-validation.lua Outdated Show resolved Hide resolved
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

we need more test cases about the header schema

apisix/plugins/request-validation.lua Outdated Show resolved Hide resolved


function _M.check_schema(conf)
return core.schema.check(schema, conf)
Copy link
Member

@membphis membphis Jun 16, 2020

Choose a reason for hiding this comment

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

export the API create_validator : https://github.com/apache/incubator-apisix/blob/master/apisix/core/schema.lua#L26

then we can use it to confirm if the input conf is a valid JSON Schema.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not understand this comment @membphis , ain't coreschema.check internally calls create_validator for validation?

Copy link
Member

Choose a reason for hiding this comment

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

here is the an valid example you provide, the user maybe provide a invalid schema:

"body_schema": {
    "type": "object",
    "required": ["required_payload"],
    "properties": {
            "emum_payload": {
            "type": "string",  # the user maybe specified a wrong type, eg: `str`
            "enum": ["enum_string_1", "enum_string_2"],
            "default": "enum_string_1"
        }
    }
}

here is detail:

image

doc/plugins/request-validation.md Outdated Show resolved Hide resolved
apisix/plugins/request-validation.lua Outdated Show resolved Hide resolved
apisix/plugins/request-validation.lua Outdated Show resolved Hide resolved
apisix/plugins/request-validation.lua Show resolved Hide resolved
Nirojan Selvanathan and others added 3 commits July 16, 2020 19:48
# Conflicts:
#	conf/config.yaml
#	t/admin/plugins.t
#	t/debug/debug-mode.t
t/plugin/request-validation.t Show resolved Hide resolved
end
end

if conf.body_schema then
Copy link
Member

Choose a reason for hiding this comment

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

another style, I think this one is better:

if not conf.body_schema then
    return
end

ngx.req.read_body()
local body = ngx.req.get_body_data()
local req_body, err

if headers["content-type"] == "application/x-www-form-urlencoded" then
    req_body, err = ngx.decode_args(body)
else -- JSON as default
    req_body, err = core.json.decode(body)
end

if not req_body then
    ... ...
    return 
end

local ok, err = core.schema.check(conf.body_schema, req_body)
...

@membphis
Copy link
Member

@sshniro you can rebase your branch with master, the CI will faster.



function _M.check_schema(conf)
return core.schema.check(schema, conf)
Copy link
Member

Choose a reason for hiding this comment

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

here is the an valid example you provide, the user maybe provide a invalid schema:

"body_schema": {
    "type": "object",
    "required": ["required_payload"],
    "properties": {
            "emum_payload": {
            "type": "string",  # the user maybe specified a wrong type, eg: `str`
            "enum": ["enum_string_1", "enum_string_2"],
            "default": "enum_string_1"
        }
    }
}

here is detail:

image


**Using ENUMS:**

```shell
Copy link
Member

Choose a reason for hiding this comment

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

we should use json here

"properties": {
"emum_payload": {
"type": "string",
enum: ["enum_string_1", "enum_string_2"]
Copy link
Member

Choose a reason for hiding this comment

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

this line should be wrong, it should be "enum":


**JSON with multiple levels:**

```shell
Copy link
Member

Choose a reason for hiding this comment

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

json ditto

@moonming
Copy link
Member

we can merge first, then fix minor issues.

@membphis membphis merged commit a617999 into apache:master Jul 21, 2020
@membphis
Copy link
Member

@sshniro merged, many thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

request help: How to validate request in APISIX
4 participants