-
Notifications
You must be signed in to change notification settings - Fork 80
Restricted access #756
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
base: main
Are you sure you want to change the base?
Restricted access #756
Changes from 17 commits
77c6124
54a07fc
e5dc7db
99ccd58
214cabb
e1d8846
7d55533
82f3b2c
ef01070
29cefa3
4697db3
c42b3d3
f76987d
f54f395
d7c2f16
144ff71
6091744
faf0580
6c267c2
c9f7b2c
955de79
2c57264
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,13 +2,16 @@ | |
|
|
||
| # pylint: disable=no-name-in-module | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import logging | ||
| from datetime import timedelta | ||
| from os.path import splitext | ||
| from urllib.parse import quote | ||
|
|
||
| from django.conf import settings | ||
| from django.db.models import Q | ||
| from django.urls import reverse | ||
| from django.utils.translation import gettext_lazy as _ | ||
|
|
||
|
|
@@ -219,6 +222,43 @@ class Meta: | |
| ] | ||
|
|
||
|
|
||
| class ShortcutTargetSerializer(serializers.ModelSerializer): | ||
| """Serialize the restricted folder a shortcut points to.""" | ||
|
|
||
| deleted = serializers.SerializerMethodField() | ||
| can_access = serializers.SerializerMethodField() | ||
|
|
||
| class Meta: | ||
| model = models.Item | ||
| fields = ["id", "title", "is_restricted", "deleted", "can_access"] | ||
| read_only_fields = ["id", "title", "is_restricted", "deleted", "can_access"] | ||
|
|
||
| def get_deleted(self, target) -> bool: | ||
| """Return whether the target is in the trash.""" | ||
| return target.deleted_at is not None | ||
|
|
||
| def get_can_access(self, target) -> bool: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can_access should be in abilities to respect the pattern used elsewhere? |
||
| """Return whether the request user can open the target.""" | ||
| request = self.context.get("request") | ||
| user = request.user if request else None | ||
| if user is not None and user.is_authenticated: | ||
| accesses = getattr(target, "viewer_accesses", None) | ||
| if accesses is None: | ||
| has_access = models.ItemAccess.objects.filter( | ||
| Q(user=user) | Q(team__in=user.teams), | ||
| item=target, | ||
| ).exists() | ||
| else: | ||
| has_access = bool(accesses) | ||
| if has_access: | ||
| return True | ||
| return target.link_reach == LinkReachChoices.PUBLIC or ( | ||
| target.link_reach == LinkReachChoices.AUTHENTICATED | ||
| and user is not None | ||
| and user.is_authenticated | ||
| ) | ||
|
|
||
|
|
||
| class ListItemSerializer(serializers.ModelSerializer): | ||
| """Serialize items with limited fields for display in lists.""" | ||
|
|
||
|
|
@@ -232,6 +272,7 @@ class ListItemSerializer(serializers.ModelSerializer): | |
| creator = UserLightSerializer(read_only=True) | ||
| hard_delete_at = serializers.SerializerMethodField(read_only=True) | ||
| is_wopi_supported = serializers.SerializerMethodField() | ||
| target = ShortcutTargetSerializer(read_only=True, allow_null=True) | ||
|
|
||
| class Meta: | ||
| model = models.Item | ||
|
|
@@ -248,10 +289,12 @@ class Meta: | |
| "is_favorite", | ||
| "link_role", | ||
| "link_reach", | ||
| "is_restricted", | ||
| "nb_accesses", | ||
| "numchild", | ||
| "numchild_folder", | ||
| "path", | ||
| "target", | ||
| "title", | ||
| "updated_at", | ||
| "user_role", | ||
|
|
@@ -280,10 +323,12 @@ class Meta: | |
| "creator", | ||
| "depth", | ||
| "is_favorite", | ||
| "is_restricted", | ||
| "link_role", | ||
| "link_reach", | ||
| "nb_accesses", | ||
| "path", | ||
| "target", | ||
| "updated_at", | ||
| "user_role", | ||
| "type", | ||
|
|
@@ -478,10 +523,12 @@ class Meta: | |
| "is_favorite", | ||
| "link_role", | ||
| "link_reach", | ||
| "is_restricted", | ||
| "nb_accesses", | ||
| "numchild", | ||
| "numchild_folder", | ||
| "path", | ||
| "target", | ||
| "title", | ||
| "updated_at", | ||
| "user_role", | ||
|
|
@@ -510,6 +557,7 @@ class Meta: | |
| "creator", | ||
| "depth", | ||
| "is_favorite", | ||
| "is_restricted", | ||
| "nb_accesses", | ||
| "link_role", | ||
| "link_reach", | ||
|
|
@@ -534,7 +582,7 @@ def create(self, validated_data): | |
| raise NotImplementedError("Create method can not be used.") | ||
|
|
||
| def update(self, instance, validated_data): | ||
| """Validate that the title is unique in the current path.""" | ||
| """Update an item, handling title uniqueness.""" | ||
| if validated_data.get("title") and instance.title != validated_data.get("title"): | ||
| if instance.depth > 1: | ||
| validated_data["title"] = instance.manage_unique_title(validated_data.get("title")) | ||
|
|
@@ -744,15 +792,8 @@ class Meta: | |
| "link_reach", | ||
| ] | ||
|
|
||
| def validate(self, attrs): | ||
| """Validate that link_role and link_reach are compatible using get_select_options.""" | ||
| link_reach = attrs.get("link_reach") | ||
| link_role = attrs.get("link_role") | ||
|
|
||
| if not link_reach: | ||
| raise serializers.ValidationError({"link_reach": _("This field is required.")}) | ||
|
|
||
| # Get available options based on ancestors' link definition | ||
| def _validate_against_ancestors(self, link_reach: str, link_role: str) -> None: | ||
| """Validate the link definition against the options allowed by ancestors.""" | ||
| available_options = LinkReachChoices.get_select_options( | ||
| **self.instance.ancestors_link_definition | ||
| ) | ||
|
|
@@ -784,12 +825,22 @@ def validate(self, attrs): | |
| raise serializers.ValidationError( | ||
| { | ||
| "link_role": ( | ||
| f"Link role '{link_role}' is not allowed for link reach '{link_reach}'. " | ||
| f"Allowed roles: {allowed_roles_str}" | ||
| f"Link role '{link_role}' is not allowed for link reach " | ||
| f"'{link_reach}'. Allowed roles: {allowed_roles_str}" | ||
| ) | ||
| } | ||
| ) | ||
|
|
||
| def validate(self, attrs: dict) -> dict: | ||
| """Validate that link_role and link_reach are compatible using get_select_options.""" | ||
| link_reach = attrs.get("link_reach") | ||
| link_role = attrs.get("link_role") | ||
|
|
||
| if not link_reach: | ||
| raise serializers.ValidationError({"link_reach": _("This field is required.")}) | ||
|
|
||
| self._validate_against_ancestors(link_reach, link_role) | ||
|
|
||
| return attrs | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -768,6 +768,32 @@ def list(self, request, *args, **kwargs): | |
| ) | ||
| queryset = queryset.filter(path__in=root_paths) | ||
|
|
||
| # Hide restricted roots the user already reaches through a live | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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... |
||
| # shortcut, so the folder shows up in a single location | ||
| if user.is_authenticated: | ||
| reachable_shortcuts = models.Item.objects.filter( | ||
| type=models.ItemTypeChoices.SHORTCUT, | ||
| target_id=db.OuterRef("pk"), | ||
| ancestors_deleted_at__isnull=True, | ||
| ).filter( | ||
| db.Exists( | ||
| models.ItemAccess.objects.filter( | ||
| db.Q(user=user) | db.Q(team__in=user.teams), | ||
| item__path__ancestors=db.OuterRef("path"), | ||
| ) | ||
| ) | ||
| | db.Exists( | ||
| models.Item.objects.filter( | ||
| path__ancestors=db.OuterRef("path"), | ||
| link_reach__in=[ | ||
| LinkReachChoices.PUBLIC, | ||
| LinkReachChoices.AUTHENTICATED, | ||
| ], | ||
| ) | ||
| ) | ||
| ) | ||
|
Comment on lines
+782
to
+803
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope this scales well in a huge db
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The exclusion is two correlated EXISTS backed by the GiST index on path, and it only runs against the restricted roots of the page being listed. The query count is constant regardless of the number of restricted roots (test_api_items_list_restricted_constant_queries), and I can share local bench numbers if useful. |
||
| queryset = queryset.exclude(db.Q(is_restricted=True) & db.Exists(reachable_shortcuts)) | ||
|
|
||
| # Annotate the queryset with an attribute marking instances as highest ancestor | ||
| # in order to save some time while computing abilities in the instance | ||
| queryset = queryset.annotate( | ||
|
|
@@ -1141,7 +1167,19 @@ def children(self, request, *args, **kwargs): | |
| ) | ||
|
|
||
| # GET: List children | ||
| queryset = item.children().select_related("creator").filter(deleted_at__isnull=True) | ||
| queryset = ( | ||
| item.children().select_related("creator", "target").filter(deleted_at__isnull=True) | ||
| ) | ||
| if request.user.is_authenticated: | ||
| queryset = queryset.prefetch_related( | ||
| db.Prefetch( | ||
| "target__accesses", | ||
| queryset=models.ItemAccess.objects.filter( | ||
| db.Q(user=request.user) | db.Q(team__in=request.user.teams) | ||
| ), | ||
| to_attr="viewer_accesses", | ||
| ) | ||
| ) | ||
| queryset = self._filter_suspicious_items(queryset, request.user) | ||
| queryset = self._exclude_pending_items(queryset) | ||
| queryset = self.filter_queryset(queryset) | ||
|
|
@@ -1238,8 +1276,12 @@ def tree(self, request, pk=None): | |
| paths_links_mapping[str(ancestor.path)] = ancestors_links.copy() | ||
|
|
||
| tree = ( | ||
| self.queryset.select_related("creator") | ||
| .filter(clause, type=models.ItemTypeChoices.FOLDER, deleted_at__isnull=True) | ||
| self.queryset.select_related("creator", "target") | ||
| .filter( | ||
| clause, | ||
| type__in=[models.ItemTypeChoices.FOLDER, models.ItemTypeChoices.SHORTCUT], | ||
| deleted_at__isnull=True, | ||
| ) | ||
| .order_by("created_at") | ||
| ) | ||
|
|
||
|
|
@@ -1624,6 +1666,19 @@ def favorite(self, request, *args, **kwargs): | |
| status=drf.status.HTTP_200_OK, | ||
| ) | ||
|
|
||
| @drf.decorators.action(detail=True, methods=["post", "delete"], url_path="restrict") | ||
| def restrict(self, request, *args, **kwargs): | ||
| """Activate or deactivate restriction on the folder based on the HTTP method.""" | ||
| item = self.get_object() | ||
|
|
||
| if request.method == "POST": | ||
| item = item.restrict(request.user) | ||
| else: | ||
| item = item.unrestrict() | ||
|
|
||
| serializer = self.get_serializer(item) | ||
| return drf.response.Response(serializer.data, status=drf.status.HTTP_200_OK) | ||
|
|
||
| def _authorize_subrequest(self, request, pattern): | ||
| """ | ||
| Shared method to authorize access based on the original URL of an Nginx subrequest | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| # Generated by Django 5.2.14 on 2026-07-24 15:11 | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('core', '0028_item_creator_size_quota_idx'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name='item', | ||
| name='is_restricted', | ||
| field=models.BooleanField(default=False), | ||
| ), | ||
| migrations.AddConstraint( | ||
| model_name='item', | ||
| constraint=models.CheckConstraint(condition=models.Q(('is_restricted', False), ('type', 'folder'), _connector='OR'), name='check_is_restricted_only_on_folders'), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
paige?