Fix JSON Merge on removing permissions (fixes #1322) #1353
Conversation
Allow null on permissions to support removing lists of principals on record permissions. As a side effect this allows passing null to regular patch operations with no effect.
|
Gabi \o/ Welcome back :) Thanks for your «patch» (huhu) To be honest, I could not take time to review it thoroughly yet. But the tests look good! I think we might have to be a bit more talkative about the changes in the changelog (and API changelog?) with regards to null values. (as well as in the docs on patch endpoints) |
|
Hey @leplatrem ! Good to see you! I can update the documentation as soon as I have some time or keep the current behavior by not allowing null values using custom validation. What do you think makes more sense? |
There was a problem hiding this comment.
Gabi! Sorry for taking so long to review this -- it looks great to me. I didn't understand your comment about not allowing null values using custom validation. However, when trying to understand the current behavior, I felt the documentation on PATCH was a little thin -- in particular, the section for Permissions is above the section for JSON Patch, even though permissions are modified using the same plain update/merge patch/JSON patch mechanism. So I'd vote for refreshing the docs -- not much is really needed IMHO, just a comment that permissions are modified according to the same mechanism as the rest of the request (attributes merge, merge patch, or JSON patch) and that nulls are ignored when using attributes merge (or whatever strange behavior you're trying to describe).
kinto/core/resource/__init__.py
Outdated
| @@ -1184,18 +1184,29 @@ def process_record(self, new, old=None): | |||
| """ | |||
| new = super().process_record(new, old) | |||
|
|
|||
| # patch is specified as a list of of operations (RFC 6902) | |||
| # patch is specified as a list of of operations (RFC i6902) | |||
| else: | ||
| permissions = self.request.validated['body'].get('permissions', {}) | ||
| permissions = {k: v for k, v in payload.get('permissions', {}).items() | ||
| if v is not None} | ||
|
|
||
| annotated = {**new} | ||
|
|
||
| if permissions: |
There was a problem hiding this comment.
This is outside the scope of this PR, but I guess this means we can never set the permissions to the empty object.
There was a problem hiding this comment.
Actually, this change is meaningful to the fix. Without it a null value on a regular patch would remove permissions as well.
There was a problem hiding this comment.
Yes, but IMHO passing an empty object should be different from not passing anything.
|
Also in the documentation is this example, which I think is incorrect: |
|
@glasserc Thanks for the review! I've tried to sketch some minor upgrades on the documentation for this section. What do you think? |
|
Flake8 seems to be failing because of this new error "E722 do not use bare except". May I just ignore this error and open an issue for it? |
| .. must be provided. | ||
|
|
||
| When permissions are modified, the same behaviour used for the body is expected. | ||
| But notice thought that there are some permissions specific behaviours, such as: |
There was a problem hiding this comment.
I think but and though (N.B. typo) are redundant -- either one is fine. I'd probably write note instead of notice. permissions-specific.
| else: | ||
| permissions = self.request.validated['body'].get('permissions', {}) | ||
| permissions = {k: v for k, v in payload.get('permissions', {}).items() | ||
| if v is not None} | ||
|
|
||
| annotated = {**new} | ||
|
|
||
| if permissions: |
There was a problem hiding this comment.
Yes, but IMHO passing an empty object should be different from not passing anything.
|
|
||
| 3. On JSON Merge operations, ``null`` values can be used to remove all principals | ||
| from a specific permission level. As a consequence, using ``null`` for permissions | ||
| on other types of patch operations is considered a no-op. |
There was a problem hiding this comment.
Wait, maybe I misunderstood, I didn't think that was a consequence. I would have written something like However, using null instead of a set of principals when using other patch types is not valid and will result in an error.
There was a problem hiding this comment.
From my understanding what I've described is the implemented behavior. This is what I've tried to explain in the PR by saying "As a side effect this allows passing null to regular patch operations with no effect".
There was a problem hiding this comment.
Ah, yes, then I misunderstood. I thought we had logic to only do the preprocess/postprocess null step if we were in a merge patch. In that case, why do we have this preprocess/postprocess step, versus just allowing permissions to be nullable? I'm guessing it's because we want to only allow nulls in certain kinds of operations, but it seems like we added it to the PermissionSchema, so it seems like it's global anyhow?
…-merge-permissions
|
Shall I merge and release this? |
Fixes #1322
Allow null on permissions to support removing lists of principals on record permissions. As a side effect this allows passing null to regular patch operations with no effect, but we can change this if needed.