From 4eba17199c86d0e55facae19ad7e81a0fcc9c6c5 Mon Sep 17 00:00:00 2001 From: Samuel Paccoud - DINUM Date: Sun, 6 Apr 2025 21:12:34 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(backend)=20fix=20link=20definition?= =?UTF-8?q?=20select=20options=20linked=20to=20ancestors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We were returning too many select options for the restricted link reach: - when the "restricted" reach is an option (key present in the returned dictionary), the possible values for link roles are now always None to make it clearer that they don't matter and no select box should be shown for roles. - Never propose "restricted" as option for link reach when the ancestors already offer a public access. Indeed, restricted/editor was shown when the ancestors had public/read access. The logic was to propose editor role on more restricted reaches... but this does not make sense for restricted since the role does is not taken into account for this reach. Roles are set by each access line assign to users/teams. --- CHANGELOG.md | 1 + src/backend/core/models.py | 52 ++++++++++++------- .../documents/test_api_documents_retrieve.py | 4 +- .../documents/test_api_documents_trashbin.py | 2 +- .../core/tests/test_models_documents.py | 29 +++++------ 5 files changed, 48 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3738603c..c369884e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ and this project adheres to ## Fixed +- 🐛(backend) fix link definition select options linked to ancestors #846 - 🐛(back) validate document content in serializer #822 - 🐛(frontend) fix selection click past end of content #840 diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 2c5239ea..f4e64995 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -87,49 +87,61 @@ class LinkReachChoices(models.TextChoices): """ Determines the valid select options for link reach and link role depending on the list of ancestors' link reach/role. - Args: ancestors_links: List of dictionaries, each with 'link_reach' and 'link_role' keys representing the reach and role of ancestors links. - Returns: Dictionary mapping possible reach levels to their corresponding possible roles. """ # If no ancestors, return all options if not ancestors_links: - return dict.fromkeys(cls.values, LinkRoleChoices.values) + return { + reach: LinkRoleChoices.values if reach != cls.RESTRICTED else None + for reach in cls.values + } # Initialize result with all possible reaches and role options as sets - result = {reach: set(LinkRoleChoices.values) for reach in cls.values} + result = { + reach: set(LinkRoleChoices.values) if reach != cls.RESTRICTED else None + for reach in cls.values + } # Group roles by reach level reach_roles = defaultdict(set) for link in ancestors_links: reach_roles[link["link_reach"]].add(link["link_role"]) - # Apply constraints based on ancestor links - if LinkRoleChoices.EDITOR in reach_roles[cls.RESTRICTED]: - result[cls.RESTRICTED].discard(LinkRoleChoices.READER) + # Rule 1: public/editor → override everything + if LinkRoleChoices.EDITOR in reach_roles.get(cls.PUBLIC, set()): + return {cls.PUBLIC: [LinkRoleChoices.EDITOR]} - if LinkRoleChoices.EDITOR in reach_roles[cls.AUTHENTICATED]: + # Rule 2: public/reader + if LinkRoleChoices.READER in reach_roles.get(cls.PUBLIC, set()): + result.get(cls.AUTHENTICATED, set()).discard(LinkRoleChoices.READER) + result.pop(cls.RESTRICTED, None) + + # Rule 3: authenticated/editor + if LinkRoleChoices.EDITOR in reach_roles.get(cls.AUTHENTICATED, set()): result[cls.AUTHENTICATED].discard(LinkRoleChoices.READER) result.pop(cls.RESTRICTED, None) - elif LinkRoleChoices.READER in reach_roles[cls.AUTHENTICATED]: - result[cls.RESTRICTED].discard(LinkRoleChoices.READER) - if LinkRoleChoices.EDITOR in reach_roles[cls.PUBLIC]: - result[cls.PUBLIC].discard(LinkRoleChoices.READER) - result.pop(cls.AUTHENTICATED, None) + # Rule 4: authenticated/reader + if LinkRoleChoices.READER in reach_roles.get(cls.AUTHENTICATED, set()): result.pop(cls.RESTRICTED, None) - elif LinkRoleChoices.READER in reach_roles[cls.PUBLIC]: - result[cls.AUTHENTICATED].discard(LinkRoleChoices.READER) - result.get(cls.RESTRICTED, set()).discard(LinkRoleChoices.READER) - # Convert roles sets to lists while maintaining the order from LinkRoleChoices - for reach, roles in result.items(): - result[reach] = [role for role in LinkRoleChoices.values if role in roles] + # Clean up: remove empty entries and convert sets to ordered lists + cleaned = {} + for reach in cls.values: + if reach in result: + if result[reach]: + cleaned[reach] = [ + r for r in LinkRoleChoices.values if r in result[reach] + ] + else: + # Could be [] or None (for RESTRICTED reach) + cleaned[reach] = result[reach] - return result + return cleaned class DuplicateEmailError(Exception): diff --git a/src/backend/core/tests/documents/test_api_documents_retrieve.py b/src/backend/core/tests/documents/test_api_documents_retrieve.py index 38d66cd4..4dcc288d 100644 --- a/src/backend/core/tests/documents/test_api_documents_retrieve.py +++ b/src/backend/core/tests/documents/test_api_documents_retrieve.py @@ -45,7 +45,7 @@ def test_api_documents_retrieve_anonymous_public_standalone(): "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, "media_auth": True, "move": False, @@ -207,7 +207,7 @@ def test_api_documents_retrieve_authenticated_unrelated_public_or_authenticated( "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, "media_auth": True, "move": False, diff --git a/src/backend/core/tests/documents/test_api_documents_trashbin.py b/src/backend/core/tests/documents/test_api_documents_trashbin.py index 6db898ea..60b8ac62 100644 --- a/src/backend/core/tests/documents/test_api_documents_trashbin.py +++ b/src/backend/core/tests/documents/test_api_documents_trashbin.py @@ -88,7 +88,7 @@ def test_api_documents_trashbin_format(): "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, "media_auth": True, "move": False, # Can't move a deleted document diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index 3f2b8d6e..c66b0fbd 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -170,7 +170,7 @@ def test_models_documents_get_abilities_forbidden( "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, "partial_update": False, "restore": False, @@ -228,7 +228,7 @@ def test_models_documents_get_abilities_reader( "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, "media_auth": True, "move": False, @@ -290,7 +290,7 @@ def test_models_documents_get_abilities_editor( "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, "media_auth": True, "move": False, @@ -341,7 +341,7 @@ def test_models_documents_get_abilities_owner(django_assert_num_queries): "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, "media_auth": True, "move": True, @@ -389,7 +389,7 @@ def test_models_documents_get_abilities_administrator(django_assert_num_queries) "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, "media_auth": True, "move": True, @@ -440,7 +440,7 @@ def test_models_documents_get_abilities_editor_user(django_assert_num_queries): "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, "media_auth": True, "move": False, @@ -498,7 +498,7 @@ def test_models_documents_get_abilities_reader_user( "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, "media_auth": True, "move": False, @@ -554,7 +554,7 @@ def test_models_documents_get_abilities_preset_role(django_assert_num_queries): "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, "media_auth": True, "move": False, @@ -1174,7 +1174,6 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): ( [{"link_reach": "public", "link_role": "reader"}], { - "restricted": ["editor"], "authenticated": ["editor"], "public": ["reader", "editor"], }, @@ -1183,7 +1182,6 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): ( [{"link_reach": "authenticated", "link_role": "reader"}], { - "restricted": ["editor"], "authenticated": ["reader", "editor"], "public": ["reader", "editor"], }, @@ -1195,7 +1193,7 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): ( [{"link_reach": "restricted", "link_role": "reader"}], { - "restricted": ["reader", "editor"], + "restricted": None, "authenticated": ["reader", "editor"], "public": ["reader", "editor"], }, @@ -1203,7 +1201,7 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): ( [{"link_reach": "restricted", "link_role": "editor"}], { - "restricted": ["editor"], + "restricted": None, "authenticated": ["reader", "editor"], "public": ["reader", "editor"], }, @@ -1229,7 +1227,7 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): {"link_reach": "restricted", "link_role": "editor"}, ], { - "restricted": ["editor"], + "restricted": None, "authenticated": ["reader", "editor"], "public": ["reader", "editor"], }, @@ -1241,7 +1239,6 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): {"link_reach": "public", "link_role": "reader"}, ], { - "restricted": ["editor"], "authenticated": ["editor"], "public": ["reader", "editor"], }, @@ -1253,7 +1250,6 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): {"link_reach": "public", "link_role": "reader"}, ], { - "restricted": ["editor"], "authenticated": ["editor"], "public": ["reader", "editor"], }, @@ -1279,7 +1275,6 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): {"link_reach": "authenticated", "link_role": "reader"}, ], { - "restricted": ["editor"], "authenticated": ["reader", "editor"], "public": ["reader", "editor"], }, @@ -1297,7 +1292,7 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): { "public": ["reader", "editor"], "authenticated": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, ), ],