Skip to content

Commit 4474fc8

Browse files
sampaccoudPanchoutNathan
authored andcommitted
♻️(backend) simplify roles by returning only the max role
We were returning the list of roles a user has on a document (direct and inherited). Now that we introduced priority on roles, we are able to determine what is the max role and return only this one. This commit also changes the role that is returned for the restricted reach: we now return None because the role is not relevant in this case.
1 parent 2f980e8 commit 4474fc8

14 files changed

+406
-350
lines changed

CHANGELOG.md

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ and this project adheres to
8585

8686
## Added
8787

88-
- ✨(backend) include ancestors accesses on document accesses list view # 846
88+
- ✨(backend) include ancestors accesses on document accesses list view #846
8989
- ✨(backend) add ancestors links definitions to document abilities #846
9090
- 🚸(backend) make document search on title accent-insensitive #874
9191
- 🚩 add homepage feature flag #861
@@ -98,22 +98,20 @@ and this project adheres to
9898

9999
## Changed
100100

101+
- ♻️(backend) simplify roles by ranking them and return only the max role #846
101102
- ⚡️(frontend) reduce unblocking time for config #867
102103
- ♻️(frontend) bind UI with ability access #900
103104
- ♻️(frontend) use built-in Quote block #908
104105

105106
## Fixed
106107

108+
- 🐛(backend) fix link definition select options linked to ancestors #846
107109
- 🐛(nginx) fix 404 when accessing a doc #866
108110
- 🔒️(drf) disable browsable HTML API renderer #919
109111
- 🔒(frontend) enhance file download security #889
110112
- 🐛(backend) race condition create doc #633
111113
- 🐛(frontend) fix breaklines in custom blocks #908
112114

113-
## Fixed
114-
115-
- 🐛(backend) fix link definition select options linked to ancestors #846
116-
117115
## [3.1.0] - 2025-04-07
118116

119117
## Added

src/backend/core/api/serializers.py

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ class ListDocumentSerializer(serializers.ModelSerializer):
171171
is_favorite = serializers.BooleanField(read_only=True)
172172
nb_accesses_ancestors = serializers.IntegerField(read_only=True)
173173
nb_accesses_direct = serializers.IntegerField(read_only=True)
174-
user_roles = serializers.SerializerMethodField(read_only=True)
174+
user_role = serializers.SerializerMethodField(read_only=True)
175175
abilities = serializers.SerializerMethodField(read_only=True)
176176

177177
class Meta:
@@ -192,7 +192,7 @@ class Meta:
192192
"path",
193193
"title",
194194
"updated_at",
195-
"user_roles",
195+
"user_role",
196196
]
197197
read_only_fields = [
198198
"id",
@@ -209,34 +209,36 @@ class Meta:
209209
"numchild",
210210
"path",
211211
"updated_at",
212-
"user_roles",
212+
"user_role",
213213
]
214214

215-
def get_abilities(self, document) -> dict:
216-
"""Return abilities of the logged-in user on the instance."""
217-
request = self.context.get("request")
215+
def to_representation(self, instance):
216+
"""Precompute once per instance"""
217+
paths_links_mapping = self.context.get("paths_links_mapping")
218218

219-
if request:
220-
paths_links_mapping = self.context.get("paths_links_mapping", None)
221-
# Retrieve ancestor links from paths_links_mapping (if provided)
222-
ancestors_links = (
223-
paths_links_mapping.get(document.path[: -document.steplen])
224-
if paths_links_mapping
225-
else None
219+
if paths_links_mapping is not None:
220+
links = paths_links_mapping.get(instance.path[: -instance.steplen], [])
221+
instance.ancestors_link_definition = choices.get_equivalent_link_definition(
222+
links
226223
)
227-
return document.get_abilities(request.user, ancestors_links=ancestors_links)
228224

229-
return {}
225+
return super().to_representation(instance)
230226

231-
def get_user_roles(self, document):
227+
def get_abilities(self, instance) -> dict:
228+
"""Return abilities of the logged-in user on the instance."""
229+
request = self.context.get("request")
230+
if not request:
231+
return {}
232+
233+
return instance.get_abilities(request.user)
234+
235+
def get_user_role(self, instance):
232236
"""
233237
Return roles of the logged-in user for the current document,
234238
taking into account ancestors.
235239
"""
236240
request = self.context.get("request")
237-
if request:
238-
return document.get_roles(request.user)
239-
return []
241+
return instance.get_role(request.user) if request else None
240242

241243

242244
class DocumentSerializer(ListDocumentSerializer):
@@ -263,7 +265,7 @@ class Meta:
263265
"path",
264266
"title",
265267
"updated_at",
266-
"user_roles",
268+
"user_role",
267269
]
268270
read_only_fields = [
269271
"id",
@@ -279,7 +281,7 @@ class Meta:
279281
"numchild",
280282
"path",
281283
"updated_at",
282-
"user_roles",
284+
"user_role",
283285
]
284286

285287
def get_fields(self):

src/backend/core/api/viewsets.py

Lines changed: 26 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -443,14 +443,15 @@ def filter_queryset(self, queryset):
443443
queryset = queryset.annotate_user_roles(user)
444444
return queryset
445445

446-
def get_response_for_queryset(self, queryset):
446+
def get_response_for_queryset(self, queryset, context=None):
447447
"""Return paginated response for the queryset if requested."""
448+
context = context or self.get_serializer_context()
448449
page = self.paginate_queryset(queryset)
449450
if page is not None:
450-
serializer = self.get_serializer(page, many=True)
451+
serializer = self.get_serializer(page, many=True, context=context)
451452
return self.get_paginated_response(serializer.data)
452453

453-
serializer = self.get_serializer(queryset, many=True)
454+
serializer = self.get_serializer(queryset, many=True, context=context)
454455
return drf.response.Response(serializer.data)
455456

456457
def list(self, request, *args, **kwargs):
@@ -460,9 +461,6 @@ def list(self, request, *args, **kwargs):
460461
This method applies filtering based on request parameters using `ListDocumentFilter`.
461462
It performs early filtering on model fields, annotates user roles, and removes
462463
descendant documents to keep only the highest ancestors readable by the current user.
463-
464-
Additional annotations (e.g., `is_highest_ancestor_for_user`, favorite status) are
465-
applied before ordering and returning the response.
466464
"""
467465
user = self.request.user
468466

@@ -490,12 +488,6 @@ def list(self, request, *args, **kwargs):
490488
)
491489
queryset = queryset.filter(path__in=root_paths)
492490

493-
# Annotate the queryset with an attribute marking instances as highest ancestor
494-
# in order to save some time while computing abilities on the instance
495-
queryset = queryset.annotate(
496-
is_highest_ancestor_for_user=db.Value(True, output_field=db.BooleanField())
497-
)
498-
499491
# Annotate favorite status and filter if applicable as late as possible
500492
queryset = queryset.annotate_is_favorite(user)
501493
queryset = filterset.filters["is_favorite"].filter(
@@ -750,7 +742,17 @@ def children(self, request, *args, **kwargs):
750742

751743
queryset = filterset.qs
752744

753-
return self.get_response_for_queryset(queryset)
745+
# Pass ancestors' links paths mapping to the serializer as a context variable
746+
# in order to allow saving time while computing abilities on the instance
747+
paths_links_mapping = document.compute_ancestors_links_paths_mapping()
748+
749+
return self.get_response_for_queryset(
750+
queryset,
751+
context={
752+
"request": request,
753+
"paths_links_mapping": paths_links_mapping,
754+
},
755+
)
754756

755757
@drf.decorators.action(
756758
detail=True,
@@ -809,39 +811,28 @@ def tree(self, request, pk, *args, **kwargs):
809811
ancestors_links = []
810812
children_clause = db.Q()
811813
for ancestor in ancestors:
812-
if ancestor.depth < highest_readable.depth:
813-
continue
814-
815-
children_clause |= db.Q(
816-
path__startswith=ancestor.path, depth=ancestor.depth + 1
817-
)
818-
819814
# Compute cache for ancestors links to avoid many queries while computing
820815
# abilities for his documents in the tree!
821816
ancestors_links.append(
822817
{"link_reach": ancestor.link_reach, "link_role": ancestor.link_role}
823818
)
824819
paths_links_mapping[ancestor.path] = ancestors_links.copy()
825820

821+
if ancestor.depth < highest_readable.depth:
822+
continue
823+
824+
children_clause |= db.Q(
825+
path__startswith=ancestor.path, depth=ancestor.depth + 1
826+
)
827+
826828
children = self.queryset.filter(children_clause, deleted_at__isnull=True)
827829

828830
queryset = ancestors.filter(depth__gte=highest_readable.depth) | children
829831
queryset = queryset.order_by("path")
830-
# Annotate if the current document is the highest ancestor for the user
831-
queryset = queryset.annotate(
832-
is_highest_ancestor_for_user=db.Case(
833-
db.When(
834-
path=db.Value(highest_readable.path),
835-
then=db.Value(True),
836-
),
837-
default=db.Value(False),
838-
output_field=db.BooleanField(),
839-
)
840-
)
841832
queryset = queryset.annotate_user_roles(user)
842833
queryset = queryset.annotate_is_favorite(user)
843834

844-
# Pass ancestors' links definitions to the serializer as a context variable
835+
# Pass ancestors' links paths mapping to the serializer as a context variable
845836
# in order to allow saving time while computing abilities on the instance
846837
serializer = self.get_serializer(
847838
queryset,
@@ -1441,8 +1432,8 @@ def list(self, request, *args, **kwargs):
14411432
except models.Document.DoesNotExist:
14421433
return drf.response.Response([])
14431434

1444-
roles = set(document.get_roles(user))
1445-
if not roles:
1435+
role = document.get_role(user)
1436+
if role is None:
14461437
return drf.response.Response([])
14471438

14481439
ancestors = (
@@ -1460,7 +1451,7 @@ def list(self, request, *args, **kwargs):
14601451
document__in=ancestors.filter(depth__gte=highest_readable.depth)
14611452
)
14621453

1463-
is_privileged = bool(roles.intersection(set(choices.PRIVILEGED_ROLES)))
1454+
is_privileged = role in choices.PRIVILEGED_ROLES
14641455
if is_privileged:
14651456
serializer_class = serializers.DocumentAccessSerializer
14661457
else:

src/backend/core/choices.py

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,18 +11,18 @@ class PriorityTextChoices(TextChoices):
1111
"""
1212

1313
@classmethod
14-
def get_priority(cls, value):
15-
"""Returns the priority of the given value based on its order in the class."""
14+
def get_priority(cls, role):
15+
"""Returns the priority of the given role based on its order in the class."""
16+
1617
members = list(cls.__members__.values())
17-
return members.index(value) + 1 if value in members else 0
18+
return members.index(role) + 1 if role in members else 0
1819

1920
@classmethod
2021
def max(cls, *roles):
2122
"""
2223
Return the highest-priority role among the given roles, using get_priority().
2324
If no valid roles are provided, returns None.
2425
"""
25-
2626
valid_roles = [role for role in roles if cls.get_priority(role) is not None]
2727
if not valid_roles:
2828
return None
@@ -61,7 +61,6 @@ class LinkReachChoices(PriorityTextChoices):
6161
) # Any authenticated user can access the document
6262
PUBLIC = "public", _("Public") # Even anonymous users can access the document
6363

64-
6564
@classmethod
6665
def get_select_options(cls, link_reach, link_role):
6766
"""
@@ -110,3 +109,34 @@ def get_select_options(cls, link_reach, link_role):
110109
reach: sorted(roles, key=LinkRoleChoices.get_priority) if roles else roles
111110
for reach, roles in result.items()
112111
}
112+
113+
114+
def get_equivalent_link_definition(ancestors_links):
115+
"""
116+
Return the (reach, role) pair with:
117+
1. Highest reach
118+
2. Highest role among links having that reach
119+
"""
120+
if not ancestors_links:
121+
return {"link_reach": None, "link_role": None}
122+
123+
# 1) Find the highest reach
124+
max_reach = max(
125+
ancestors_links,
126+
key=lambda link: LinkReachChoices.get_priority(link["link_reach"]),
127+
)["link_reach"]
128+
129+
# 2) Among those, find the highest role (ignore role if RESTRICTED)
130+
if max_reach == LinkReachChoices.RESTRICTED:
131+
max_role = None
132+
else:
133+
max_role = max(
134+
(
135+
link["link_role"]
136+
for link in ancestors_links
137+
if link["link_reach"] == max_reach
138+
),
139+
key=LinkRoleChoices.get_priority,
140+
)
141+
142+
return {"link_reach": max_reach, "link_role": max_role}

0 commit comments

Comments
 (0)