🐛(backend) fix wrong permission check on sub page duplicate action [DO NOT MERGE] #1356
Conversation
A user with only read access to a document and it sub documents should not be able to duplicate a sub document in the document tree. To avoid this we have to compute the direct parent abilities to determine if it can create a new children. Doing so computes the abilities in cascase from the document to the root tree. To asave some quesries and not compute again and again the same ability, we create a abilities cache. The hard part is to invalidate this cache. The cache is invalidated if a related DocumentAccess instance is updated or deleted and also if the document link_reach or link_role is updated. To introspect the modification made on the model it self when the user save it, we decided to use the library django-dirtyfields
lunika
added
bug
| """ | ||
| return not self.has_deleted_children and self.numchild == 0 | ||
|
|
||
| def get_ancestors(self): |
There was a problem hiding this comment.
Why do you need to override the base MP_Node method ?
There was a problem hiding this comment.
To add the select_releated("creator") to save one query. The creator is used in the abilities.
src/backend/core/models.py
Outdated
| """Actual link role on the document.""" | ||
| return self.computed_link_definition["link_role"] | ||
|
|
||
| def _compute_abilities_cache_key(self): |
There was a problem hiding this comment.
nit: "compute" what a big word for a simple function... "get"? or even simply a property?
There was a problem hiding this comment.
Ok I will change it for get.
| factories.UserDocumentAccessFactory(document=child1) | ||
|
|
||
| with django_assert_num_queries(10): | ||
| with django_assert_num_queries(17): |
There was a problem hiding this comment.
With out the cache it will be 36 🤷♂️
| "content": True, | ||
| "destroy": access.role in ["administrator", "owner"], | ||
| "duplicate": True, | ||
| "duplicate": access.role != "reader", |
There was a problem hiding this comment.
Reading the test, I don't even know if it's what we want or not... this has become too complexe for me ^^'
| } | ||
|
|
||
| DOCUMENT_ABILITIES_CACHE_TIMEOUT = values.IntegerValue( | ||
| default=60 * 60, # 1 hour |
There was a problem hiding this comment.
It could even be 12 hours, no? Leave it like that, we will fine tune in deployment
| with django_assert_num_queries(9): | ||
| APIClient().get(f"/api/v1.0/documents/{document.id!s}/children/") | ||
| with django_assert_num_queries(5): | ||
| with django_assert_num_queries(4): |
There was a problem hiding this comment.
The abilities are in cache, there is no need to make a query to retrieve the roles in the accesses
| factories.UserDocumentAccessFactory(document=child1) | ||
|
|
||
| with django_assert_num_queries(9): | ||
| with django_assert_num_queries(11): |
There was a problem hiding this comment.
This number of queries depends on the number of children. There is one more query for each child.
lunika
changed the title
|
The philosophy behind the duplicate feature is: "if you can see it you can duplicate it". More generally on the implementation, heritage is more complex than looking at the parent and caching get_abilities is more complicated than proposed here. I think it would trigger tricky bugs, and complicate group sharing. Also, the heavy lifting is done in the queryset and the get_abilities method is not where the time is spent so I think we should cache higher in the view if one day we do. But that's far from necessary with our current traffic so let's keep it live until we face a problem ? |
lunika
changed the title
Purpose
A user with only read access to a document and its sub documents should not be able to duplicate a sub document in the document tree. To avoid this we have to compute the direct parent abilities to determine if it can create a new children. Doing so computes the abilities in cascade from the document to the root tree. To save some queries and not compute again and again the same ability, we create a abilities cache. The hard part is to invalidate this cache. The cache is invalidated if a related DocumentAccess instance is updated or deleted and also if the document link_reach or link_role is updated. To introspect the modification made on the model itself when the user saves it, we decided to use the library django-dirtyfields
Proposal
Fixes #1329