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

Docs for CLP Pointer permissions. #700

Open
wants to merge 37 commits into
base: gh-pages
Choose a base branch
from

Conversation

BufferUnderflower
Copy link
Contributor

@TomWFox
Copy link
Contributor

TomWFox commented Jan 28, 2020

Wow, this is quite substantial, reviewing this is on my to-do list now that the parse server pr has been merged but this won’t be merged until the next release.

Copy link
Contributor

@TomWFox TomWFox left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, here's what I've got so far. Also is this still a work in progress?

For my reference I got down to line 162.

_includes/common/security.md Outdated Show resolved Hide resolved
_includes/common/security.md Outdated Show resolved Hide resolved
_includes/common/security.md Outdated Show resolved Hide resolved
_includes/common/security.md Outdated Show resolved Hide resolved
_includes/common/security.md Outdated Show resolved Hide resolved
_includes/common/security.md Outdated Show resolved Hide resolved
_includes/common/security.md Outdated Show resolved Hide resolved
_includes/common/security.md Outdated Show resolved Hide resolved
@TomWFox
Copy link
Contributor

TomWFox commented Feb 16, 2020

@BufferUnderflower If you've got maintainer commits turned on I could commit these suggestions and others myself. Otherwise could you look over my review? - there is going to be a Parse Server release soon so I'd like to get this ready to go.

_includes/common/security.md Outdated Show resolved Hide resolved
_includes/common/security.md Outdated Show resolved Hide resolved
Copy link
Contributor

@TomWFox TomWFox left a comment

Choose a reason for hiding this comment

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

We're getting there! I've looked over all of it now, there may be a few more changes after this round but it's looking pretty good I'd say 🙂

Multi-line suggestions are a thing now which is nice 😅

_includes/parse-server/class-level-permissions.md Outdated Show resolved Hide resolved
_includes/parse-server/class-level-permissions.md Outdated Show resolved Hide resolved
_includes/parse-server/class-level-permissions.md Outdated Show resolved Hide resolved
_includes/parse-server/class-level-permissions.md Outdated Show resolved Hide resolved
Comment on lines +132 to +151
While permissions discussed before let you explicitly target a user by id or a role,
Pointer Permissions let you dynamically enforce permissions without knowing the id or assigning roles in advance. Instead you define one or more field names of this class, and any users pointed by such fields of a particular object are granted the permission.

To use this feature, a field must:

* already exist in this collection's schema
* be of either `Pointer<_User>` or `Array` type

In case of `Array`, only items that are of `Pointer<_User>` type are evaluated, other items silently ignored.

You can think of it as a virtual ACL or a dynamic role defined per-object in its own field.

There are two ways you can set Pointer Permission in schema:

* [Using granular permissions](#granular-pointer-permissions) - `pointerFields` *requires Parse-Server v3.11 and above*
* [Using grouped permissions](#grouped-pointer-permissions) - `readUserFields`/`writeUserFields`

⚠️ `create` operation can't be allowed by pointer permissions, because there is literally no object to check it's field untill it is created;

⚠️ `addField` grants permission to only update an object with a new field, but it is advised to set addField permission using other means (e.g. restrict to a role or particular admin user by id).
Copy link
Contributor

Choose a reason for hiding this comment

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

There is quite a discrepancy between here and the corresponding common section...
https://github.com/parse-community/docs/pull/700/files#diff-22c65bfd6614d949ec20e9d4461ce3cfR163-R169

This part needs some of the grammatical improvements that I suggested in the common section although I wouldn't add all the explanation - in fact I would suggest removing some of the long example paragraphs from the common doc on this section as they seem a bit repetitive / overly wordy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This brought me to a question - why do we have CLP section in Parse-Server guide in the first place? It seems to have info about server as a binary, its setup and launch, while CLP is more about schema level feature. I suggest to remove CLP from server guide as a whole.

Copy link
Contributor

Choose a reason for hiding this comment

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

good question, I think the Parse Server guide has been under loved so only contains limited info about basic / required setup - and there are discrepancies in what is included in the Parse Server README and the guide - I did aim to consolidate all the info into the guide but didn't get around to it.

I'm wondering whether we could remove the CLP section in the server guide in favour of adding the common security doc but I'm not sure how this would work in practice with the code sample etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact the code samples are mostly schema configs - plain JSON objects. And a few examples come with several demo setup lines (that i've already translated to most sdk languages in this pr).
Anyway clp and protected fields are used by database admin rather than client code or any of SDKs. I also think it could be very helpful and time saving if we maintain such examples as a shared postman collection.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that.

I think adding the common security doc to the server guide would be good, that way there is only one 'copy' to be maintained and its available on all the guides which would encourage devs to think about security when they are implementing their client side.

Copy link
Contributor

Choose a reason for hiding this comment

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

So are you happy with that or do you have other ideas?

I think it needs to be in there server guide in some way as it is related to setup - especially as you have gone to all the effort of writing it and I don't think it would make sense to have it in the client guides but not the server one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree we should merge it and only have a single copy as you proposed. As for Parse-Server Guide probably REST-flavored examples should fit best.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, REST example would make sense. I'll leave that with you then, or if you've got maintainer commits turned on I could take a look later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe i created this pr with edits enabled. Not sure if there is anything else i should allow?
image
I'm planning to work on it later today anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that’s the one, the annoying thing is that on GitHub there is no indication to me whether you have that ticked or not.

_includes/parse-server/class-level-permissions.md Outdated Show resolved Hide resolved
_includes/parse-server/class-level-permissions.md Outdated Show resolved Hide resolved
_includes/common/security.md Outdated Show resolved Hide resolved
Comment on lines 271 to 281
Class-Level Permissions (CLPs) and Access Control Lists (ACLs) are both powerful tools for securing your app, but they don't always interact exactly how you might expect. They actually represent two separate layers of security that each request has to pass through to return the correct information or make the intended change. These layers, one at the class level, and one at the object level, are shown below. A request must pass through BOTH layers of checks in order to be authorized. Note that despite acting similarly to ACLs, [Pointer Permissions](#pointer-permissions) are a type of class level permission, so a request must pass the pointer permission check in order to pass the CLP check.

<img alt="CLP vs ACL Diagram" data-echo="{{ '/assets/images/clp_vs_acl_diagram.png' | prepend: site.baseurl }}"/>

As you can see, whether a user is authorized to make a request can become complicated when you use both CLPs and ACLs. Let's look at an example to get a better sense of how CLPs and ACLs can interact. Say we have a `Photo` class, with an object, `photoObject`. There are 2 users in our app, `user1` and `user2`. Now lets say we set a Get CLP on the `Photo` class, disabling public Get, but allowing `user1` to perform Get. Now let's also set an ACL on `photoObject` to allow Read - which includes GET - for only `user2`.

You may expect this will allow both `user1` and `user2` to Get `photoObject`, but because the CLP layer of authentication and the ACL layer are both in effect at all times, it actually makes it so *neither* `user1` nor `user2` can Get `photoObject`. If `user1` tries to Get `photoObject`, it will get through the CLP layer of authentication, but then will be rejected because it does not pass the ACL layer. In the same way, if `user2` tries to Get `photoObject`, it will also be rejected at the CLP layer of authentication.

Now lets look at example that uses Pointer Permissions. Say we have a `Post` class, with an object, `myPost`. There are 2 users in our app, `poster`, and `viewer`. Lets say we add a pointer permission that gives anyone in the `Creator` field of the `Post` class read and write access to the object, and for the `myPost` object, `poster` is the user in that field. There is also an ACL on the object that gives read access to `viewer`. You may expect that this will allow `poster` to read and edit `myPost`, and `viewer` to read it, but `viewer` will be rejected by the Pointer Permission, and `poster` will be rejected by the ACL, so again, neither user will be able to access the object.

Because of the complex interaction between CLPs, Pointer Permissions, and ACLs, we recommend being careful when using them together. Often it can be useful to use CLPs only to disable all permissions for a certain request type, and then using Pointer Permissions or ACLs for other request types. For example, you may want to disable Delete for a `Photo` class, but then put a Pointer Permission on `Photo` so the user who created it can edit it, just not delete it. Because of the especially complex way that Pointer Permissions and ACLs interact, we usually recommend only using one of those two types of security mechanisms.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll look at this properly later but at first glance this seems very dense. Would it be possible to shorten this section or replace some of it with a diagram or schema.

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, I believe there is some room to make it more concise. May be code will read easier than words

@TomWFox TomWFox changed the title Docs for CLP Pointer permissions [WIP] Docs for CLP Pointer permissions Feb 27, 2020
leaving a draft for protected fields, needs some more work for sure but its at least something
@BufferUnderflower BufferUnderflower changed the title Docs for CLP Pointer permissions Docs for CLP Pointer permissions and Protected Fields Feb 28, 2020
@BufferUnderflower
Copy link
Contributor Author

BufferUnderflower commented Mar 1, 2020

A note on protected fields. It's probably too early to disclose the documentation, since it's pretty far for being production ready. I'm now working on some tests to show what's left to be done there. Main concerns:

  • We don't cover create/update operations, and they're pretty complicated to handle.
  • As of now protection is not always enforced.

@TomWFox
Copy link
Contributor

TomWFox commented Mar 1, 2020

Ok, thanks for that info.

Perhaps if it's not too difficult you could remove the content about protected fields from this PR and make a separate draft PR for when its more production ready or do you want to hold back this whole PR until those issues have been addressed on parse server?

@BufferUnderflower BufferUnderflower changed the title Docs for CLP Pointer permissions and Protected Fields Docs for CLP Pointer permissions. Mar 1, 2020
@BufferUnderflower
Copy link
Contributor Author

There is no need to hold CLP, in fact it would be nice to finish it asap and merge, because protected fields would require lots of love before being ready i'm afraid.

@BufferUnderflower
Copy link
Contributor Author

Probably it would be wise to publish some warning in docs against use for sensitive data temporarily, since it appears in 4.0 changelog and there were several issues by folks interested in it.

@TomWFox
Copy link
Contributor

TomWFox commented Mar 1, 2020

If your right about the issues with protected fields I would absolutely agree that warnings should be given. Feel free to open an issue/pr on parse server for that.

@TomWFox
Copy link
Contributor

TomWFox commented Apr 8, 2020

@BufferUnderflower Sorry this has been neglected, a final review of this is on my to-do list!

"s0meUs3r1d": true, // key must be an id of `_User`
"role:admin": true, // key must be `role:${role_name}`
"requiresAuthentication": true, // any authenticated users
"pointerFields": ["onwer", "followers"] // pointer permissions

Choose a reason for hiding this comment

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

owner.

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

Successfully merging this pull request may close these issues.

📙 Add a code example to CLP Ponter permissions in Rest Guide
4 participants