Skip to content

Commit d2ac2a8

Browse files
committed
♻️(backend) set_role_to min role to max_ancestors_role for is_explicit
The set_role_to was computed to have only roles strictly higher than the max_ancestors_role. We want this behavior only for inherited accesses. When the role is explicit, we want to allow the lower role in set_role_to equal to max_ancestors_role
1 parent 9a24a12 commit d2ac2a8

File tree

5 files changed

+34
-21
lines changed

5 files changed

+34
-21
lines changed

src/backend/core/api/permissions.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,9 @@ def has_permission(self, request, view):
120120

121121
def has_object_permission(self, request, view, obj):
122122
"""Check permission for a given object."""
123-
abilities = obj.get_abilities(request.user)
123+
abilities = obj.get_abilities(
124+
request.user, is_explicit=str(view.item.id) == str(obj.item_id)
125+
)
124126

125127
requested_role = request.data.get("role")
126128
if requested_role and requested_role not in abilities.get("set_role_to", []):

src/backend/core/api/serializers.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,9 @@ def get_abilities(self, instance):
8989
"""Return abilities of the logged-in user on the instance."""
9090
request = self.context.get("request")
9191
if request:
92-
return instance.get_abilities(request.user)
92+
return instance.get_abilities(
93+
request.user, is_explicit=self.get_is_explicit(instance)
94+
)
9395
return {}
9496

9597
def get_max_ancestors_role(self, instance):

src/backend/core/models.py

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1248,7 +1248,7 @@ def max_ancestors_role(self, max_ancestors_role):
12481248
"""Cache the max_ancestors_role."""
12491249
self._max_ancestors_role = max_ancestors_role
12501250

1251-
def get_abilities(self, user):
1251+
def get_abilities(self, user, is_explicit=True):
12521252
"""
12531253
Compute and return abilities for a given user on the item access.
12541254
"""
@@ -1278,11 +1278,19 @@ def get_abilities(self, user):
12781278

12791279
# Filter out roles that would be lower than the one the user already has
12801280
ancestors_role_priority = RoleChoices.get_priority(self.max_ancestors_role)
1281-
set_role_to = [
1282-
candidate_role
1283-
for candidate_role in set_role_to
1284-
if RoleChoices.get_priority(candidate_role) > ancestors_role_priority
1285-
]
1281+
if is_explicit:
1282+
set_role_to = [
1283+
candidate_role
1284+
for candidate_role in set_role_to
1285+
if RoleChoices.get_priority(candidate_role) >= ancestors_role_priority
1286+
]
1287+
else:
1288+
set_role_to = [
1289+
candidate_role
1290+
for candidate_role in set_role_to
1291+
if RoleChoices.get_priority(candidate_role) > ancestors_role_priority
1292+
]
1293+
12861294
if len(set_role_to) == 1:
12871295
set_role_to = []
12881296

src/backend/core/tests/items/test_api_item_accesses.py

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,7 @@ def test_api_item_accesses_retrieve_set_role_to_child():
337337
result["id"]: result["abilities"]["set_role_to"] for result in content
338338
}
339339
assert result_dict[str(item_access_other_user.id)] == [
340+
"editor",
340341
"administrator",
341342
"owner",
342343
]
@@ -357,23 +358,23 @@ def test_api_item_accesses_retrieve_set_role_to_child():
357358
[
358359
["reader", "editor", "administrator"],
359360
[],
360-
["editor", "administrator"],
361+
["reader", "editor", "administrator"],
361362
],
362363
],
363364
[
364365
["owner", "reader", "reader", "reader"],
365366
[
366367
["reader", "editor", "administrator", "owner"],
367368
[],
368-
["editor", "administrator", "owner"],
369+
["reader", "editor", "administrator", "owner"],
369370
],
370371
],
371372
[
372373
["owner", "reader", "reader", "owner"],
373374
[
374375
["reader", "editor", "administrator", "owner"],
375376
[],
376-
["editor", "administrator", "owner"],
377+
["reader", "editor", "administrator", "owner"],
377378
],
378379
],
379380
],
@@ -432,31 +433,31 @@ def test_api_item_accesses_list_authenticated_related_same_user(roles, results):
432433
[
433434
["reader", "editor", "administrator"],
434435
[],
435-
["editor", "administrator"],
436+
["reader", "editor", "administrator"],
436437
],
437438
],
438439
[
439440
["owner", "reader", "reader", "reader"],
440441
[
441442
["reader", "editor", "administrator", "owner"],
442443
[],
443-
["editor", "administrator", "owner"],
444+
["reader", "editor", "administrator", "owner"],
444445
],
445446
],
446447
[
447448
["owner", "reader", "reader", "owner"],
448449
[
449450
["reader", "editor", "administrator", "owner"],
450451
[],
451-
["editor", "administrator", "owner"],
452+
["reader", "editor", "administrator", "owner"],
452453
],
453454
],
454455
[
455456
["reader", "reader", "reader", "owner"],
456457
[
457458
["reader", "editor", "administrator", "owner"],
458459
[],
459-
["editor", "administrator", "owner"],
460+
["reader", "editor", "administrator", "owner"],
460461
],
461462
],
462463
[
@@ -1595,7 +1596,7 @@ def test_api_item_accesses_explicit():
15951596
"update": True,
15961597
"partial_update": True,
15971598
"retrieve": True,
1598-
"set_role_to": ["administrator", "owner"],
1599+
"set_role_to": ["editor", "administrator", "owner"],
15991600
},
16001601
"max_ancestors_role": "editor",
16011602
"max_role": "owner",
@@ -1743,10 +1744,10 @@ def test_api_item_accesses_explicit():
17431744
"role": "owner",
17441745
"abilities": {
17451746
"destroy": True,
1746-
"update": False,
1747-
"partial_update": False,
1747+
"update": True,
1748+
"partial_update": True,
17481749
"retrieve": True,
1749-
"set_role_to": [],
1750+
"set_role_to": ["administrator", "owner"],
17501751
},
17511752
"max_ancestors_role": "administrator",
17521753
"max_role": "owner",

src/backend/core/tests/test_models_item_accesses.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -449,7 +449,7 @@ def test_models_item_access_get_abilities_explicit():
449449
"update": True,
450450
"partial_update": True,
451451
"retrieve": True,
452-
"set_role_to": ["administrator", "owner"],
452+
"set_role_to": ["editor", "administrator", "owner"],
453453
}
454454

455455
# Owner user on the root item, acting on the previous user's accesses.
@@ -468,5 +468,5 @@ def test_models_item_access_get_abilities_explicit():
468468
"update": True,
469469
"partial_update": True,
470470
"retrieve": True,
471-
"set_role_to": ["administrator", "owner"],
471+
"set_role_to": ["editor", "administrator", "owner"],
472472
}

0 commit comments

Comments
 (0)