Restricted access#756
Conversation
e8d21cc to
001fd58
Compare
The soft delete assertion sat inside the pytest.raises block and never ran, hiding a wrong expected message. Indexer error tests now keep a single raising invocation inside the block so the failure source is unambiguous. Flagged by Sonar on PR #756.
The soft delete assertion sat inside the pytest.raises block and never ran, hiding a wrong expected message. Indexer error tests now keep a single raising invocation inside the block so the failure source is unambiguous. Flagged by Sonar on PR #756.
c48a165 to
1cc98ba
Compare
The soft delete assertion sat inside the pytest.raises block and never ran, hiding a wrong expected message. Indexer error tests now keep a single raising invocation inside the block so the failure source is unambiguous. Flagged by Sonar on PR #756.
1cc98ba to
94113e5
Compare
The soft delete assertion sat inside the pytest.raises block and never ran, hiding a wrong expected message. Indexer error tests now keep a single raising invocation inside the block so the failure source is unambiguous. Flagged by Sonar on PR #756.
94113e5 to
1d95fb3
Compare
The soft delete assertion sat inside the pytest.raises block and never ran, hiding a wrong expected message. Indexer error tests now keep a single raising invocation inside the block so the failure source is unambiguous. Flagged by Sonar on PR #756.
ee6b94a to
8d0b54d
Compare
The soft delete assertion sat inside the pytest.raises block and never ran, hiding a wrong expected message. Indexer error tests now keep a single raising invocation inside the block so the failure source is unambiguous. Flagged by Sonar on PR #756.
66fe632 to
fee9799
Compare
The soft delete assertion sat inside the pytest.raises block and never ran, hiding a wrong expected message. Indexer error tests now keep a single raising invocation inside the block so the failure source is unambiguous. Flagged by Sonar on PR #756.
fee9799 to
7d0b98d
Compare
The soft delete assertion sat inside the pytest.raises block and never ran, hiding a wrong expected message. Indexer error tests now keep a single raising invocation inside the block so the failure source is unambiguous. Flagged by Sonar on PR #756.
7d0b98d to
0d0bcbc
Compare
0d0bcbc to
1dc030a
Compare
Allow owners to activate/deactivate restriction on folders via PATCH with permission check through get_abilities.
The soft delete assertion sat inside the pytest.raises block and never ran, hiding a wrong expected message. Indexer error tests now keep a single raising invocation inside the block so the failure source is unambiguous. Flagged by Sonar on PR #756.
e7c1786 to
6311151
Compare
The soft delete assertion sat inside the pytest.raises block and never ran, hiding a wrong expected message. Indexer error tests now keep a single raising invocation inside the block so the failure source is unambiguous. Flagged by Sonar on PR #756.
6311151 to
afea861
Compare
Parent owner excluded from a restricted child folder cannot trash it. The folder is moved to root intact with its accesses and descendants preserved.
Roles granted above a restricted folder were leaking through the annotation subquery used by children lists and search results, and through the non-annotated get_role fallbacks of ItemAccess and Invitation. The annotation is now built by the permissions backend, dropping the duplicated ItemQuerySet override.
Restricted folders can set any link reach regardless of parent configuration. Propagating link reach to descendants now stops at restriction boundaries.
Restricted folders accept any explicit role regardless of ancestor roles. Synchronizing accesses to descendants now stops at restriction boundaries.
Items behind a restriction boundary with no effective role are now filtered out after role annotation.
Items behind a restriction boundary are now filtered out of ZIP exports using role annotation.
get_ancestor_to_descendants_map now queries for restricted folders among ancestor paths and stops the mapping at restriction boundaries, preventing inherited accesses from crossing into restricted subtrees.
TreeModel.parent() uses .last() which follows Meta.ordering (created_at). After a move(), the grandparent created later is returned instead of the direct parent. The parent path is known statically, so look it up by equality instead of scanning ancestors.
Sonar S3516 flags validate() because every return yields the same attrs value. Merge the restricted early return into an if/else with a single exit and move the ancestors validation to a helper to keep cognitive complexity low; behavior is unchanged.
The soft delete assertion sat inside the pytest.raises block and never ran, hiding a wrong expected message. Indexer error tests now keep a single raising invocation inside the block so the failure source is unambiguous. Flagged by Sonar on PR #756.
Each ability now reads as a named rule on ItemAbilities, fixing the Sonar S3776 complexity of the former monolithic function. Per-action properties also sketch the vocabulary a future ABAC engine will implement, one check per action.
Setting is_restricted required a create then update round trip, since CreateItemSerializer didn't expose the field. The creator is always granted owner access right after creation, so the restrict ability is already satisfied at that point. Restricting a file is rejected, mirroring the existing DB constraint that only folders can be restricted.
afea861 to
47a544b
Compare
|
| ) | ||
|
|
||
| if attrs.get("is_restricted") and attrs["type"] != models.ItemTypeChoices.FOLDER: | ||
| raise serializers.ValidationError( |
There was a problem hiding this comment.
I think this constraint should be defined on the model directly
| GistIndex( | ||
| fields=["path"], | ||
| name="drive_item_restricted_path_ix", | ||
| condition=models.Q(is_restricted=True), | ||
| ), |
There was a problem hiding this comment.
The "why" of this is not announced in the commit and no tests confirm the sought behavior.
| # Pre-compute number of accesses | ||
| # Pre-compute number of accesses; the parent's count does not apply | ||
| # to restricted children which cut inheritance | ||
| item_nb_accesses = item.nb_accesses |
There was a problem hiding this comment.
I think this should be renamed to nb_effective_accesses.
The current name is a bit confusing. I had to reread the nb_accesses method on the model to remember what past me wrote. 😅
| if item.is_restricted: | ||
| # Inheritance is cut on the item itself: only direct accesses apply | ||
| return models.ItemAccess.objects.filter( | ||
| Q(user=user) | Q(team__in=user.teams), | ||
| item=item, | ||
| ).values_list("role", flat=True) |
There was a problem hiding this comment.
smells a bit like AI slope.
Here you are shortcircuiting your own roles_at method instead of passing it an argument "should_inherit=True" and let it do the job.
And above _cut_by_restriction is worse because you went down to the path from the item and go back to check is_restricted on the item via a query 😲
I think you should trash the _cut_by_restriction method and modify each existing filter clause directly looking at the should_inherit argument.
| @abstractmethod | ||
| def ancestors_links_paths_mapping(self, item: models.Item) -> dict[str, list[dict]]: | ||
| """Return the link definitions applying to each ancestor path of the item.""" | ||
|
|
||
| @abstractmethod | ||
| def link_definition_for(self, item: models.Item) -> dict: | ||
| """Return the effective link definition of the item, own and inherited combined.""" | ||
|
|
There was a problem hiding this comment.
It would have been nice to move it in the first commit creating the backend since they were already existing.
|
|
||
| redundant_ids = [ | ||
| access.id | ||
| for access in ItemAccess.objects.filter(item=self) |
There was a problem hiding this comment.
You have the same problem on all its descendants... Maybe you should test that these redundant accesses are not causing any functional problem and if this is confirmed, you can clean them in an asynchronous job?
But actually I'm having second thoughts : IF after removing restriction, an access is more open than what is defined on the parent, I think the user should be warned! Because it is an open access that was not effective because of restriction and will now be effective for all users of the tree above!
So now I think everything should be reset on the whole subtree when a restriction is removed.
| assert folder.link_role == "reader" | ||
|
|
||
|
|
||
| def test_models_items_restricted_uproot_moves_to_root(): |
There was a problem hiding this comment.
your commit message reads: "When an ancestor owner removes a restricted folder they don't
have access to"
How could I remove it if I don't see it?
| - ✨(frontend) add file type, contact and modification date topbar filters | ||
| - ✨(frontend) add location, file type, contact and date search filters | ||
| - ♻️(backend) route permission decisions through a swappable backend | ||
| - ✨(backend) preserve restricted folders when an ancestor is soft-deleted |
There was a problem hiding this comment.
This is going to create confusion and break things. Instead you should forbid deletion by a user if there is a restricted folder on which s.he does not have deletion rights...
Uprooting the restricted folder can be suggested but should be done explicitly... Maybe it means that owners should see restricted folders (greyed) and be able to move them out of their folders even if they have no rights on them...
| def visible( | ||
| self, | ||
| queryset: QuerySet[models.Item], | ||
| user: models.User | AnonymousUser, | ||
| path_field: str = "path", | ||
| ) -> QuerySet[models.Item]: | ||
| """Filter the queryset to the rows on which the user holds a role.""" | ||
| return self.annotate_roles(queryset, user, path_field=path_field).exclude(user_roles=[]) |
There was a problem hiding this comment.
This is not a correct :
- the semantic "visible" because an item may be visible through a link_reach
- the filtering because you will exclude the items that the user can see thanks to link_reach
| descendants = folder.descendants().filter(ancestors_deleted_at__isnull=True).order_by("path") | ||
| descendants = folder.descendants().filter(ancestors_deleted_at__isnull=True) | ||
| if user and user.is_authenticated: | ||
| descendants = get_permissions_backend().visible(descendants, user) |
There was a problem hiding this comment.
Same. This is not correct because it forgets about link_reach.
This time it seems that you painted yourself in a corner as fixing this would lead to awful code and performance issues.
In the end, I think the correct way to do restriction is probably to move items that are getting restricted to the root of the tree and materialize their appearance in the origin folder by an item of type "restriction_link". By doing this, the whole access right chain will work without any modification and the item can be moved back to its location when the restriction is lifted.
before restriction
└── containing_folder (0001)
├── folder1 (00010001)
│ ├── file1 (000100010001)
│ └── file2 (000100010002)
├── folder2 (00010002)
└── file3 (00010003)
After restriction
├── folder1 (0002)
│ ├── file1 (00020001)
│ └── file2 (00020002)
└── containing_folder (0001)
├── folder1 [restriction shortcut → 0002] (00010001)
├── folder2 (00010002)
└── file3 (00010003)
Instead of modifying all the access rules and making the code buggy and complicated, you just need to handle :
- hide it from the real root : easy since they are flagged as restricted items...
- show them in their folder of origin : easy since you can add a shortcut of type "restriction" in the said folder
|
This review was made commit by commit. The first commits were fully commented as everything looked fine but at some point, the commits starting fixing caveats and everything goes wrong. The conclusion is that the method is a deadend and leads to code that is hard to understand and I'm afraid will never work (link reach was forgotten in restricted items accesses). I ended-up suggesting another approach that should be simpler and work well : move an item to the root when it is being restricted. The review was aborted after this point. |
|
Thanks @sampaccoud for the review. So the alternative is to move a restricted folder to root, with a "restriction link" placeholder marking its original spot. A few UX/UI questions this raises:
@robin-lecomte, what do you think? |
I could be a separate object or a shortcut flagged as "restriction" 🤔
You don't need to tell them about moving the item to the root (plus it will be hidden from the root..) The reason for informing the user is that access rights on the item are changing, not that we move the item to/from the root (not his business and only a technical subject)
This was discussed elsewhere in my review with several cases:
|
Purpose
Allow marking a folder as "restricted": it stops inheriting roles and link
configuration from its ancestors, and only explicit accesses granted from
the folder down apply.
Proposal
is_restrictedtoItem(folders only), exposed in the API with arestrictability for ownersuser_rolesannotation,nb_accesses, access management, link propagation, search, ZIP export, search index ACLscore/permissions,PERMISSIONS_BACKENDsetting), one property per ability, paving the way for ABACparent()to resolve the direct parent by exact path