From 0499aec624fbcd9c60c3f57f11a9b2b17d7fd073 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 | 5 ++ 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, 52 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ebf80bfc..51ace2f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ and this project adheres to ### Fixed +- 🐛(backend) fix link definition select options linked to ancestors #846 - 🐛(frontend) table of content disappearing #982 - 🐛(frontend) fix multiple EmojiPicker #1012 - 🐛(frontend) fix meta title #1017 @@ -111,6 +112,10 @@ and this project adheres to - 🐛(backend) race condition create doc #633 - 🐛(frontend) fix breaklines in custom blocks #908 +## Fixed + +- 🐛(backend) fix link definition select options linked to ancestors #846 + ## [3.1.0] - 2025-04-07 ## Added diff --git a/src/backend/core/models.py b/src/backend/core/models.py index ead7161e..160839be 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 fbecf2f5..81ae8756 100644 --- a/src/backend/core/tests/documents/test_api_documents_retrieve.py +++ b/src/backend/core/tests/documents/test_api_documents_retrieve.py @@ -46,7 +46,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, "media_check": True, @@ -212,7 +212,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, "media_check": True, 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 61ccc021..77e67b81 100644 --- a/src/backend/core/tests/documents/test_api_documents_trashbin.py +++ b/src/backend/core/tests/documents/test_api_documents_trashbin.py @@ -89,7 +89,7 @@ def test_api_documents_trashbin_format(): "link_select_options": { "authenticated": ["reader", "editor"], "public": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, "media_auth": True, "media_check": True, diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index a791f3d8..af001a1a 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -172,7 +172,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, @@ -231,7 +231,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, "media_check": True, @@ -295,7 +295,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, "media_check": True, @@ -348,7 +348,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, "media_check": True, @@ -398,7 +398,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, "media_check": True, @@ -451,7 +451,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, "media_check": True, @@ -511,7 +511,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, "media_check": True, @@ -569,7 +569,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, "media_check": True, @@ -1190,7 +1190,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"], }, @@ -1199,7 +1198,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"], }, @@ -1211,7 +1209,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"], }, @@ -1219,7 +1217,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"], }, @@ -1245,7 +1243,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"], }, @@ -1257,7 +1255,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"], }, @@ -1269,7 +1266,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"], }, @@ -1295,7 +1291,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"], }, @@ -1313,7 +1308,7 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): { "public": ["reader", "editor"], "authenticated": ["reader", "editor"], - "restricted": ["reader", "editor"], + "restricted": None, }, ), ],