Parent attributes with child creation (fixes #803, #101) #828

leplatrem · 2016-09-21T14:54:29Z

Fixes #803, Fixes #101

)

leplatrem · 2016-09-26T22:55:53Z

I applied the same logics to bucket metadata (collection:create and group:create gives access to (parent) bucket metadata).

Maybe we shouldn't and keep it for record+collection metadata only?

What do you think @enguerran / @magopian ?

magopian · 2016-09-27T07:22:37Z

I don't see any drawbacks in having the same behavior. Do you think it could be a security issue or something?

magopian

Commented ;)

magopian · 2016-09-27T07:44:16Z

docs/api/index.rst

+
+- Parent metadata is now readable if children creation is allowed
+- Now returns a ``412`` instead of a ``403`` if the ``If-None-Match: *`` header is
+  provided and the ``create`` permission is allowed


Do you want to add a note about the fact that GETing the list of records will return an empty list (and not a 403) if the user doesn't have the read permission on the collection, and has not created any records yet? (as discussed in the issue #803)

I believe test_list_can_be_obtained_if_allowed_to_create and test_list_is_denied_if_not_allowed_to_create cover this (?)

Sure, the tests and the code are there, but how was wondering if you wanted to add a note to the changelog about that?

magopian · 2016-09-27T07:58:56Z

tests/test_views_objects_permissions.py

+    def test_list_can_be_obtained_if_allowed_to_create(self):
+        resp = self.app.get('/buckets/beer/collections', headers=self.bob_headers)
+        self.assertEqual(len(resp.json['data']), 1)
+        self.assertEqual(resp.json['data'][0]['id'], 'barley')


I thought one could only see the list of his own created entries, or is that only for records? Is it expected that Bob can see this collection, while he only has the "create" permission on collections, and the 'barley' collection has been created by Alice?

the barley collection was created by Bob (and Alice is allowed to create records). But you're right, the current version of the patch is not correct. I added a failing test that shows that users should only see their own records in this particular case.

leplatrem · 2016-09-27T08:28:52Z

Do you think it could be a security issue or something?

I wouldn't say a security issue, but it makes the impact of the change bigger.

enguerran · 2016-09-27T14:27:20Z

I cannot take a decision on this as I am not yet aware enough of kinto permissions system. I do discussed with @Natim. But I cannot understand how my needs are different from formbuilder's.

Extract from https://github.com/Kinto/formbuilder/blob/master/formbuilder/actions/server.js:

bucket: { "collection:create": ["System.Authenticated"], "write": [] }
collection (a form): { "record:create": ["System.Authenticated"], "read": [userId] }

The extra need is

We also want to be able to list the collection the user can administrate. It makes it impossible if we give the read permission on the collection.

In formbuilder, access are controlled by "encoded" URL. If you have the admin token, you'll have access to the collection's administration. If you have the user token, you'll have access to the form and can only create a record. As far as I could understand formbuilder's code.

If I create a collection of "url with admin token" in a specific user's default bucket, I guess an admin could have access to several forms' administration.

Again, I am just a beginner user of kinto and maybe I am wrong.

@magopian

suggested by @magopian

…-with-child-creation

leplatrem · 2016-09-28T11:24:47Z

This cannot be reviewed without going through each commit. I'm very confident in the code and the test suite though.

I changed and simplified a lot of code. Basically:

I renamed many variables and method names in the authorization code. It is now more consistent with what it does.
I added a level of keys in the inheritance tree of permissions. Before it used to have some concat and join strings for the keys.
There is now a read:attributes permission in the inheritance tree of permissions. It is not exposed yet to the HTTP API but we could eventually. It gives the finer grain we need (ETag is only returned on GET #110) to allow reading attributes and not the whole list of records.
I changed the way the authorization code relies on settings. This paves the way for No inheritance for permissions forced in settings #350
I removed the specific case we had for allowing listing buckets. It now behaves as other: if you can create buckets, you can receive an empty list on /buckets if none are accessible to you.

Don't hesitate to ping me for questions!

@magopian r?

leplatrem · 2016-10-03T14:07:38Z

kinto/core/authorization.py

-            allowed = shared is not None
+            # If allowed to create this kind of object on parent, then allow to obtain the list.
+            if len(bound_perms) > 0:
+                parent_uri = bound_perms[0][0]  # Bounds perms are ordered.


This takes the first object URI of the bound perms.
But the first is not always the parent! (Ex. record → collection → bucket)
So I guess this is wrong for records, because it will check that the user has the permission record:create on /buckets/test which will never be the case.

…-with-child-creation

leplatrem · 2016-10-03T15:18:52Z

Final r+ my friends?

leplatrem · 2016-10-04T07:58:34Z

\o/

leplatrem added the in progress label Sep 21, 2016

leplatrem added 2 commits September 27, 2016 00:12

Add failing test for parent metadata with child creation (ref #803)

f4f4f6c

Allow to read parent metadata when child creation is allowed (fixes #803

9eb3249

)

leplatrem force-pushed the 803-parent-metadata-with-child-creation branch from 5939c63 to 9eb3249 Compare September 26, 2016 22:46

Update docs

dccdf70

leplatrem added 2 commits September 27, 2016 00:58

Bump API version

cc70a16

Modify load tests assumptions

3dc1746

magopian reviewed Sep 27, 2016

View reviewed changes

Add failing test for isolation

3200c4e

leplatrem changed the title ~~Add failing test for parent metadata with child creation (ref #803)~~ [WIP] parent metadata with child creation (ref #803) Sep 27, 2016

leplatrem added 8 commits September 28, 2016 11:35

Update changelog

6614659

suggested by @magopian

Simplify some authorization code

1023ed0

Cleaner permissions tree

b61eee8

Additional level of permissions for object attributes

6679cf8

Improve code and docs of authorization helpers

380226f

Allow to list if allowed to create

3cab17c

Remove custom root factory for buckets and generalize permissions

cc5a143

Simplify and clean-up authorization

efe3867

leplatrem changed the title ~~[WIP] parent metadata with child creation (ref #803)~~ Parent metadata with child creation (ref #803) Sep 28, 2016

leplatrem added 2 commits September 28, 2016 13:12

Merge remote-tracking branch 'origin/master' into 803-parent-metadata…

46c40ef

…-with-child-creation

Fix Python3 syntax

b970c5d

leplatrem changed the title ~~Parent metadata with child creation (ref #803)~~ Parent metadata with child creation (fixes #803, #101) Sep 28, 2016

leplatrem commented Oct 3, 2016

View reviewed changes

leplatrem added 2 commits October 3, 2016 16:19

Merge remote-tracking branch 'origin/master' into 803-parent-metadata…

fd0d788

…-with-child-creation

Parent at one level above in URI path

8e238d4

leplatrem force-pushed the 803-parent-metadata-with-child-creation branch from f87e8d6 to 8e238d4 Compare October 3, 2016 14:53

leplatrem mentioned this pull request Oct 3, 2016

Can't create a record when only the record:create permission is set on a collection Kinto/kinto-admin#197

Closed

Natim approved these changes Oct 4, 2016

View reviewed changes

leplatrem merged commit bb8e66d into master Oct 4, 2016

leplatrem deleted the 803-parent-metadata-with-child-creation branch October 4, 2016 07:58

leplatrem removed the in progress label Oct 4, 2016

Natim mentioned this pull request Oct 5, 2016

Do not give everyone permission to read answers Kinto/formbuilder#156

Merged

leplatrem changed the title ~~Parent metadata with child creation (fixes #803, #101)~~ Parent attributes with child creation (fixes #803, #101) Oct 25, 2016

leplatrem mentioned this pull request Oct 25, 2016

Add safety check for list of collections Kinto/kinto-admin#297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parent attributes with child creation (fixes #803, #101) #828

Parent attributes with child creation (fixes #803, #101) #828

leplatrem commented Sep 21, 2016 •

edited

Loading

leplatrem commented Sep 26, 2016

magopian commented Sep 27, 2016

magopian left a comment

magopian Sep 27, 2016

leplatrem Sep 27, 2016

magopian Sep 27, 2016

magopian Sep 27, 2016

leplatrem Sep 27, 2016

leplatrem commented Sep 27, 2016

enguerran commented Sep 27, 2016

leplatrem commented Sep 28, 2016

leplatrem Oct 3, 2016

leplatrem commented Oct 3, 2016

leplatrem commented Oct 4, 2016

Parent attributes with child creation (fixes #803, #101) #828

Parent attributes with child creation (fixes #803, #101) #828

Conversation

leplatrem commented Sep 21, 2016 • edited Loading

leplatrem commented Sep 26, 2016

magopian commented Sep 27, 2016

magopian left a comment

Choose a reason for hiding this comment

magopian Sep 27, 2016

Choose a reason for hiding this comment

leplatrem Sep 27, 2016

Choose a reason for hiding this comment

magopian Sep 27, 2016

Choose a reason for hiding this comment

magopian Sep 27, 2016

Choose a reason for hiding this comment

leplatrem Sep 27, 2016

Choose a reason for hiding this comment

leplatrem commented Sep 27, 2016

enguerran commented Sep 27, 2016

leplatrem commented Sep 28, 2016

leplatrem Oct 3, 2016

Choose a reason for hiding this comment

leplatrem commented Oct 3, 2016

leplatrem commented Oct 4, 2016

leplatrem commented Sep 21, 2016 •

edited

Loading