Validate header and querystring with cornice schemas (fixes #873) #1021

gabisurita · 2017-01-17T02:33:30Z

Fixes #873
Related to #1006

Fix unicode in headers
Fix JSON Patch (related to Validate json patch body with Colander #880)
Investigate if there's anything else that can be validated (specially some filters)
Add a changelog entry

r? @

leplatrem

This is excellent, very well executed (as usual)! Thank you for being so thorough!

I made a few comments, but none requires structural change I believe...

leplatrem · 2017-01-17T09:22:13Z

kinto/core/resource/__init__.py

@@ -542,7 +542,8 @@ def patch(self):
                new_record[extra_field] = existing[extra_field]

        # Adjust response according to ``Response-Behavior`` header
-        body_behavior = self.request.headers.get('Response-Behavior', 'full')
+        body_behavior = self.request.validated.get('header',
+                                                   {}).get('Response-Behavior', 'full')


When does it happen that validated doesn't have a header entry? See https://github.com/Cornices/cornice/blob/9b73c5ae8dfbebede6413a007ffac7fe28e76401/cornice/validators/__init__.py#L60-L62

leplatrem · 2017-01-17T09:22:50Z

kinto/core/resource/__init__.py

-                return
-            raise_invalid(self.request, **error_details)
+        if if_none_match == '*':
+            return


leplatrem · 2017-01-17T09:24:00Z

kinto/core/resource/__init__.py

@@ -918,9 +887,8 @@ def _raise_400_if_id_mismatch(self, new_id, record_id):
    def _extract_partial_fields(self):
        """Extract the fields to do the projection from QueryString parameters.
        """
-        fields = self.request.GET.get('_fields', None)
+        fields = self.request.validated.get('querystring', {}).get('_fields')


Ditto: could this happen that querystring is missing? If it's for the test, we could fix the test setUp instead I believe.

leplatrem · 2017-01-17T09:27:08Z

kinto/core/resource/schema.py

@@ -150,3 +151,71 @@ class BookmarkSchema(ResourceSchema):

    def preparer(self, appstruct):
        return strip_whitespace(appstruct)
+
+
+class CSVQuerystring(colander.SchemaNode):


What do you think of FieldList or StringList instead ?

leplatrem · 2017-01-17T09:27:33Z

kinto/core/resource/schema.py

+        params = super(CSVQuerystring, self).deserialize(cstruct)
+        if params is colander.drop:
+            return params
+        else:


nit: superfluous else

leplatrem · 2017-01-17T09:28:40Z

kinto/core/resource/schema.py

+        if params is colander.drop:
+            return params
+        else:
+            return params.split(',')


I think there is the notion of preparer in Colander that could split, and then the notion of Sequence. In a second iteration, we can try to leverage that if possible

This can definitely be improved but I didn't understand how to use preparer to do it. Thinking through it, maybe defining it as Sequence we can do something as simple as:

class FieldList(colander.SchemaNode): fields = colander.SchemaNode(colander.String(), missing=colander.drop) def deserialize(self, cstruct=colander.null): if isinstance(cstruct, six.string_types): cstruct = cstruct.split(',') return super(FieldList, self).deserialize(cstruct)

leplatrem · 2017-01-17T09:29:55Z

kinto/core/resource/schema.py

+        if isinstance(cstruct, six.string_types):
+            try:
+                cstruct = decode_header(cstruct)
+            except:


except UnicodeDecodeError ?

leplatrem · 2017-01-17T09:32:14Z

kinto/core/resource/schema.py

+                                             name='Response-Behavior',
+                                             validator=colander.OneOf(
+                                                 ['full', 'light', 'diff']),
+                                             missing=colander.drop)


Is decode_header not necessary for this one? Maybe we could run decode_header here before deserailize each sub-node so that we have it one place only? Or do it in Cornice instead...

I guess handling it on cornice is a good idea. I'm also not sure if it's still needed.

I can't remember why we had that, but if I remember well it's because of Webob (/cc @Natim)

leplatrem · 2017-01-17T09:32:44Z

kinto/core/resource/schema.py

+
+class HeaderSchema(colander.MappingSchema):
+    if_match = HeaderQuotedInteger(name='If-Match', missing=colander.drop)
+    if_none_match = HeaderQuotedInteger(name='If-None-Match', missing=colander.drop)


I see that you repeat missing=drop, maybe we can remove it from each schema then!

leplatrem · 2017-01-17T09:35:07Z

kinto/core/resource/viewset.py

            record_schema = self.get_record_schema(resource_cls, method)
            record_schema.name = 'body'
            schema.add(record_schema)
            args['schema'] = schema
        else:
-            args['schema'] = SimpleSchema()
+            args['schema'] = RequestSchema()


One was Partial and the other one Simple. Is that ok to now give the same behaviour to both?

Well, I've got why StrictSchema and SimpleSchemawere needed by get_record_schemabut I actually didn't understand why we needed PartialSchema and SimpleSchema on this one, aren't we just setting the request schema here (in contrary to the body schema)?

Also, this didn't break any tests and the API behavior looks ok, so I think it's ok to keep the same behavior to both.

gabisurita · 2017-01-17T15:42:12Z

Note: I'm probably going to address #880 on this PR as well.

glasserc · 2017-01-17T19:17:05Z

Looks OK to me. Maybe I misunderstood what you were saying in our meeting earlier, but it sounded like you needed to fix the JSON thing to fix the build failures, but the build failures don't seem related to the JSON library validation per se. So what's the rationale for fixing the JSON thing in this PR? (Just curious...)

gabisurita · 2017-01-17T20:03:15Z

So what's the rationale for fixing the JSON thing in this PR?

Getting JSON Patch requests to be validated by cornice. We need this because now we are using colander for validating and deserializing other aspects of the request that are also needed on JSON Patch (sync headers, etc). Right now we only trust the external library for validation and ignore cornice validation for this content-type. https://github.com/Kinto/kinto/blob/master/kinto/core/resource/__init__.py#L101

The problem is that I just discovered colander doesn't accept JSON Arrays at the top level, so this may be a bit more tricky than it seems. Cornices/cornice#433

…erialize all the filters (including the unknown ones) during the deserialize call

…date-header-with-cornice

gabisurita · 2017-01-19T21:52:41Z

kinto/core/resource/schema.py

+        if schema_values is colander.drop:
+            return schema_values
+
+        # Deserialize filters


It does not look very good, but think it's better to handle the filter deserialization here than leave it to the Resource._extract_filters method.

Opinions?

It doesn't look bad by any means IMHO, and I really like having an explicit, formal schema, so I think this is an improvement.

Me neither, well done!

I have one remark though: the above comment could be more explicit about filters (field filters?) to help understand what the code does. A couple of minimalist examples with boolean or list value could help for example (?deleted=true -> {"deleted": True})

leplatrem · 2017-01-19T22:55:51Z

kinto/core/resource/schema.py

+    response_behaviour = HeaderField(colander.String(),
+                                     name='Response-Behavior',
+                                     validator=colander.OneOf(['full', 'light', 'diff']),
+                                     missing=colander.drop)


Why couldn't we set missing in HeaderField class instead? (like FieldList)

leplatrem · 2017-01-19T23:00:23Z

kinto/core/resource/schema.py

+        if schema_values is colander.drop:
+            return schema_values
+
+        # Deserialize filters


Me neither, well done!

I have one remark though: the above comment could be more explicit about filters (field filters?) to help understand what the code does. A couple of minimalist examples with boolean or list value could help for example (?deleted=true -> {"deleted": True})

leplatrem · 2017-01-19T23:02:49Z

kinto/core/resource/schema.py

+
+    op = colander.SchemaNode(colander.String(),
+                             validator=colander.OneOf(
+                                 ['test', 'add', 'remove', 'replace', 'move', 'copy']))


please move this list elsewhere (or whole validator) to improve indentation ;) #IndentationFreak

leplatrem · 2017-01-19T23:04:32Z

kinto/core/resource/schema.py

+
+    @staticmethod
+    def schema_type():
+        return colander.Mapping(unknown='preserve')


shouldn't we allow value only instead ?

The problem is that value don't have an specific type, so we can't set it on the schema (or at least I don't know how to do it). Maybe we could check this at deserialize?

Or maybe define a new colander type that deserialize returns ctruct?

Just check its presence in deserialize maybe then

leplatrem · 2017-01-19T23:05:45Z

kinto/core/resource/schema.py

+                             validator=colander.OneOf(
+                                 ['test', 'add', 'remove', 'replace', 'move', 'copy']))
+    path = colander.SchemaNode(colander.String())
+    from_ = colander.SchemaNode(colander.String(), name='from', missing=colander.drop)


cherry on cake: you could have a regex to make sure those look valid :)

That would be cool, but I'm not sure what we can validate here. I know they all have to start with /, but I'm not sure if there's anything else to check here. Maybe if there is anything between two /? IDK

Yeah something like ^(/\w)+$ ? .

leplatrem · 2017-01-19T23:09:20Z

kinto/core/resource/schema.py

+
+
+class JsonPatchRequestSchema(RequestSchema):
+    body = JsonPatchBodySchema()


Technically we don't support as many querystring values for a patch. But maybe we can keep it simple as it is, they don't harm.

Your call!

Yeah, that was something I was thinking about earlier... this doesn't apply only for JSON Patch.

Should we use the same schema for all requests and validate all parameters, even the ones we won't use it, or set individual request schemas for each method with only the expected params?

I think it's more explicit to have multiple request schemas, and it's better for documentation purposes, but that also means more code to maintain.

As a step 1, what we have here is fine. If you struggle with that when working on #1006 then you can do a second round.

leplatrem · 2017-01-20T11:40:11Z

The remaining changes to do are nice to have. You can merge as soon as you feel good about the code :)

GG !

leplatrem · 2017-01-20T11:41:03Z

(are you sure you added a changelog entry?)

Fixes: Kinto#124 Ref: Kinto/kinto#1021

gabisurita added 2 commits January 17, 2017 00:25

add basic header/query validation

b2a1143

use validated instead of raw headers and queries

defc70b

gabisurita added the in progress label Jan 17, 2017

gabisurita force-pushed the 873-validate-header-with-cornice branch from 55d3d24 to d2d96de Compare January 17, 2017 02:42

patch existing tests to match changes

d2d96de

gabisurita force-pushed the 873-validate-header-with-cornice branch from d2d96de to 57398be Compare January 17, 2017 02:58

leplatrem requested changes Jan 17, 2017

View reviewed changes

@leplatrem review

4dba538

gabisurita force-pushed the 873-validate-header-with-cornice branch from 57398be to cf050ab Compare January 17, 2017 15:29

gabisurita added 2 commits January 17, 2017 13:30

ensure request.validated is defined on resource tests

cf050ab

more minor comments

0f3104e

gabisurita force-pushed the 873-validate-header-with-cornice branch from 7debfd1 to b45d0f7 Compare January 19, 2017 16:15

add basic JSON Patch validation

6d2f100

gabisurita force-pushed the 873-validate-header-with-cornice branch from b45d0f7 to 3f07d5a Compare January 19, 2017 21:39

gabisurita added 2 commits January 19, 2017 19:40

include some static filters on the Request querystring schema and des…

3f07d5a

…erialize all the filters (including the unknown ones) during the deserialize call

Merge branch 'master' of https://github.com/Kinto/kinto into 873-vali…

faed41a

…date-header-with-cornice

gabisurita changed the title ~~[WIP] validate header and querystring with cornice schemas (fixes #873)~~ Validate header and querystring with cornice schemas (fixes #873) Jan 19, 2017

update changelog

abed8db

gabisurita commented Jan 19, 2017

View reviewed changes

gabisurita removed the in progress label Jan 19, 2017

leplatrem reviewed Jan 19, 2017

View reviewed changes

leplatrem approved these changes Jan 20, 2017

View reviewed changes

gabisurita added 3 commits January 20, 2017 12:20

@leplatrem comments

faf35e2

use validated body on patch requests

ea6038d

Merge branch 'master' into 873-validate-header-with-cornice

b9b6a9e

pep8

7ad266f

leplatrem merged commit c7fd161 into Kinto:master Jan 20, 2017

gabisurita deleted the 873-validate-header-with-cornice branch January 20, 2017 17:56

gabisurita mentioned this pull request Feb 13, 2017

Plugin doesn't work with request validation Kinto/kinto-attachment#124

Closed

gabisurita added a commit to gabisurita/kinto-attachment that referenced this pull request Feb 13, 2017

Fix crash with kinto validated headers

87900c6

Fixes: Kinto#124 Ref: Kinto/kinto#1021



		class JsonPatchRequestSchema(RequestSchema):
		body = JsonPatchBodySchema()

Validate header and querystring with cornice schemas (fixes #873) #1021

Validate header and querystring with cornice schemas (fixes #873) #1021

Conversation

gabisurita commented Jan 17, 2017 • edited

leplatrem left a 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

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

gabisurita commented Jan 17, 2017

glasserc commented Jan 17, 2017

gabisurita commented Jan 17, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leplatrem Jan 20, 2017 • edited

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

leplatrem commented Jan 20, 2017

leplatrem commented Jan 20, 2017

gabisurita commented Jan 17, 2017 •

edited

gabisurita commented Jan 17, 2017 •

edited

leplatrem Jan 20, 2017 •

edited