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
db2114c to
6c85764
Compare
Allow login with demo users in local development.
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.
6c85764 to
6c5eebf
Compare
Move role and access resolution behind a backend resolved from the new PERMISSIONS_BACKEND setting, following the entitlements backend convention. Behavior is unchanged; this gives a single decision point to swap for an ABAC engine later.
Pure move of Item.get_abilities into the role backend so every permission decision sits behind the same facade.
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.
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.
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.
Allow folders to be marked as restricted. A DB constraint prevents setting is_restricted on non-folder items.
A shortcut materializes the original location of a restricted folder moved to the tree root. The OneToOne target enforces a single shortcut per folder and a DB constraint ties the target to the shortcut type.
Only an explicit owner can toggle restriction on a folder. A folder needs a parent to host its shortcut on activation, while an already restricted folder lives at the tree root and must stay deactivatable.
Restriction is structural: the folder physically leaves its parent so inheritance stops applying without any query-level cut. A shortcut materializes its origin location. Explicit link reach is kept and defaults to restricted only when it was inherited.
The folder returns under its shortcut's current parent and the shortcut disappears. Without a live shortcut the folder stays a detached root. Inheritance applies again through the tree structure.
Explicit roles granted during restriction that are now covered by inheritance are dropped, so the sharing screen does not keep dead entries. Superior roles and roles without inherited counterpart stay.
The reach kept from the restriction period is reset to inherit when the reattached parent already grants as much or more. A more open explicit reach survives, matching the role normalization rule.
Owners activate and deactivate restriction on folders via PATCH, gated by the restrict ability. The serializer keeps working on the instance returned by the toggle since the item physically moves.
Shortcuts expose their target's id, title and a can_access flag so the frontend can grey out entries pointing to folders the user cannot open. The children listing prefetches the viewer accesses to keep the query count flat, and the tree includes shortcut entries.
A restricted folder lives at the tree root but its members reach it through the shortcut when they can open the containing folder. The listing hides the root in that case so the folder shows up in a single location, and keeps it for members without container access.
Deleting a shortcut removes the entry from the containing folder without trashing anything. An owner excluded from the target acts on the container only and can neither destroy, declassify nor read it: the folder keeps its accesses, stays restricted, and surfaces in its members' top-level listing.
Trashing a folder must not drag restricted folders with it: their shortcuts are removed before the subtree is marked deleted, so each target survives untouched for its members while the rest of the branch goes to the trash as usual. Restoring the ancestor does not bring the shortcuts back.
An explicit owner trashing a restricted folder leaves no entry pointing into the trash. The folder restores as a detached root, still restricted, reachable by its members from their listing.
Shortcuts are tree entries, not content: they never match a search, never reach the search index, and leave no entry in an exported archive. The target itself is indexed and exported through its own root, so users excluded from a restricted folder cannot find its content through search or an ancestor export.
Creating a child folder with is_restricted chains creation and activation atomically, so the folder starts as a restricted root with its shortcut in place. Only owners of the parent may use it: a lower role would promote itself to owner through restriction. Shortcuts themselves can never be created directly.
|
| }, | ||
| { | ||
| "username": "paige", | ||
| "email": "page.turner@library.book", |
| @@ -1,7 +1,12 @@ | |||
| """Role-based permissions backend.""" | |||
|
|
|||
| from __future__ import annotations | |||
There was a problem hiding this comment.
I agree that this get_abilities method has become very big but what this commit proposes looks like object-maxing to me.
I’m concerned that cached_property hides the cost of permission checks. Attribute access can trigger DB work, making performance and N+1s much harder to reason about. This will lead to convoluted and inefficient code in the future.
I’d keep ItemAbilities as the API, but make the expensive computes explicit and compute them once when the object is created or when a called property requires it if you really want to optimize... but in the case of API calls you need to build the whole dict anyway so KISS.
The individual abilities can then remain small, pure methods and you avoid using cached_property systematically without explicit reason (the next developer will forget it):
class ItemAbilities:
def __init__(self, user, item):
self.user = user
self.item = item
# Explicitly compute/load anything that may hit the DB.
self.role = item.get_role(user)
self.link_definition = item.computed_link_definition
self.is_deleted = bool(item.ancestors_deleted_at)
def can_update(self):
return (
self.role in {RoleChoices.OWNER, RoleChoices.ADMIN}
and not self.is_deleted
)
def can_delete(self):
return (
self.role == RoleChoices.OWNER
and not self.is_deleted
)
def can_get(self):
return self.role is not None and not self.is_deleted
def as_dict(self):
return {
"can_update": self.can_update(),
"can_delete": self.can_delete(),
"can_get": self.can_get(),
}This preserves the readability of one rule per ability, but avoids making each attribute access a potentially expensive, hidden computation. It also gives us one obvious place to optimize or batch the DB work later.r
Add tests to secure number of db calls if missing and make use of these new properties in python code now that we have them?
|
|
||
| with pytest.raises(RuntimeError) as exc_info: | ||
| items[-1].soft_delete() | ||
| assert str(exc_info) == "This item is already deleted or has deleted ancestors." |
There was a problem hiding this comment.
I didn't get why it's never caught. It never ran because the error was not a RuntimeError? Then it's the error the being caught that we should change?
| parent.move(new_grandparent) | ||
| child.refresh_from_db() | ||
|
|
||
| assert child.parent() == parent |
There was a problem hiding this comment.
So this is a bug in django-ltree right? Should we make a pull request on the lib as well?l
|
|
||
|
|
||
| def test_models_items_shortcuts_unique_per_target(): | ||
| """A restricted folder cannot be targeted by two shortcuts.""" |
There was a problem hiding this comment.
I understand that you decided not to use the same object for the shortcut feature that will come next and that will probably need to allow several shortcuts per item?
It's probably a good decision that the restricted access target uses a specific item type but you should use another name that is more explicit and keep the name "shortcut" for the future shortcuts?
| path=RawSQL("%s || subpath(path, nlevel(%s))", (str(self.path), str(old_path))) | ||
| ) | ||
|
|
||
| @transaction.atomic |
There was a problem hiding this comment.
add a test on atomicity by simulating a break in the middle of the method and checking that the folder is not broken?
|
|
||
| # The flag must fall before the move: move() refuses restricted folders | ||
| item.is_restricted = False | ||
| item.save(update_fields=["is_restricted", "updated_at"]) |
There was a problem hiding this comment.
I'm thinking: this is_restricted field is a weak and breaks information unicity.
An item is restricted when it is targeted by a restriction type item... so we shouldn't make it a db column.
| """Return whether the target is in the trash.""" | ||
| return target.deleted_at is not None | ||
|
|
||
| def get_can_access(self, target) -> bool: |
There was a problem hiding this comment.
can_access should be in abilities to respect the pattern used elsewhere?
| ) | ||
| queryset = queryset.filter(path__in=root_paths) | ||
|
|
||
| # Hide restricted roots the user already reaches through a live |
There was a problem hiding this comment.
the fact that you have to do this reinforces my earlier take on removing the is_restricted field... the only remaining case for it was that it would ease filtering on the root... But in fact filtering on the root is not automatic for all restricted folders...
| **serializer.validated_data, | ||
| **self.get_create_extra_attributes(), | ||
| ) | ||
| if is_restricted: |
There was a problem hiding this comment.
I don't see the point of allowing this. It's seems simpler and more robust to consider restriction as a definite action taken on an already existing folder?



Purpose
Closes #731.
Let a folder owner restrict a folder so that only its explicit members
keep access, cutting the roles and link reach inherited from the tree.
This replaces the previous approach of this PR (cutting inheritance with
query-level exclusions) following review: the boundary had to be
reimplemented in every access-related query, and any forgotten spot
leaked content — link reach was already missing from the visibility
filters. The branch was rebuilt from main.
Proposal
A restricted folder physically becomes a root of the tree, so
inheritance stops by construction and every existing and future query
is correct without knowing about restriction. A new
shortcutitemtype materializes its original location.
is_restrictedfield andshortcutitem type targeting a restricted rootrestrictability: explicit owners only, activation needs a parent, deactivation always possiblePATCH {"is_restricted": bool},targetpayload on shortcuts (can_accessdrives the greyed entry), shortcuts in the tree endpointparent()resolved by exact path after a moveFrontend follow-up needed: render the
shortcuttype (opentarget.idwhen
can_access, greyed otherwise) and the activation/deactivationwarnings.