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.

Sign up for GitHub

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

Jump to bottom

Fix JSON Merge on removing permissions (fixes #1322) #1353

Merged
merged 4 commits into from
Nov 16, 2017
Merged

Fix JSON Merge on removing permissions (fixes #1322) #1353

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9fd24fd
Fix JSON Merge on permissions
gabisurita Oct 21, 2017
b9dd0de
typos
gabisurita Nov 4, 2017
3a66798
sketch some improvements on the documentation
gabisurita Nov 4, 2017
2c3a5c5
Merge branch 'master' of https://github.com/Kinto/kinto into fix-json…
gabisurita Nov 9, 2017
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
47 changes: 28 additions & 19 deletions docs/api/1.x/_details-patch-object.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,23 +39,6 @@ only the fields whose value was changed are returned. If set to
``diff``, only the fields whose value became different than
the one provided are returned.

Permissions
-----------

In the JSON request payloads, at least one of ``data`` and ``permissions`` must be provided. Permissions can thus be modified independently from data.

The :ref:`current user id <api-current-userid>` **is always added** among the ``write`` principals.

See :ref:`api-permissions-payload`.

..
.. Kinto.core feature, not used in Kinto:
..
.. Read-only fields
.. ----------------

.. If a read-only field is modified, a |status-400| error is returned.

JSON Patch Operations
---------------------

Expand All @@ -82,8 +65,20 @@ For more information about each operation, please refer to
`JSON-Patch Specification <https://tools.ietf.org/html/rfc6902>`_.

This is very useful when altering permissions since there is no need to get
the current value before adding some principal to the list. Value is not used
on permission operations and can be omitted.
the current value before adding some principal to the list.

Permissions
-----------

.. In the JSON request payloads, at least one of ``data`` and ``permissions``
.. 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:
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 but and though (N.B. typo) are redundant -- either one is fine. I'd probably write note instead of notice. permissions-specific.


1. The :ref:`current user id <api-current-userid>` **is always added** among the
``write`` principals.
2. Value is not used on permission JSON Patch operations and can be omitted.

.. code-block:: javascript

Expand All @@ -92,3 +87,17 @@ on permission operations and can be omitted.
{ "op": "add", "path": "/permissions/read/system.Everyone" },
{ "op": "remove", "path": "/permissions/read/fxa:bob" }
]

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?


For more information on permissions, see :ref:`api-permissions-payload`.

..
.. Kinto.core feature, not used in Kinto:
..
.. Read-only fields
.. ----------------

.. If a read-only field is modified, a |status-400| error is returned.