From 8799b4aa2f983a4a6ad331a6ade144f4377293d8 Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Wed, 12 Nov 2025 11:54:55 +0100 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F(backend)=20role=20in?= =?UTF-8?q?=20ask=5Ffor=5Faccess=20must=20be=20lower=20than=20user=20role?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We check that the role set in a ask_for_access is not higher than the user's role accepting the request. We prevent case where ad min will grant a user owner in order to take control of the document. Only owner can accept an owner role. --- src/backend/core/api/viewsets.py | 14 +++++- .../test_api_documents_ask_for_access.py | 47 +++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 84402ceaae..a9f99d0444 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -2162,7 +2162,19 @@ def accept(self, request, *args, **kwargs): serializer = serializers.RoleSerializer(data=request.data) serializer.is_valid(raise_exception=True) - document_ask_for_access.accept(role=serializer.validated_data.get("role")) + document = self.get_document_or_404() + user_role = document.get_role(request.user) + target_role = serializer.validated_data.get("role") + + if models.RoleChoices.get_priority(user_role) < models.RoleChoices.get_priority( + target_role + ): + return drf.response.Response( + {"detail": "You cannot accept a role higher than your own."}, + status=drf.status.HTTP_400_BAD_REQUEST, + ) + + document_ask_for_access.accept(role=target_role) return drf.response.Response(status=drf.status.HTTP_204_NO_CONTENT) diff --git a/src/backend/core/tests/documents/test_api_documents_ask_for_access.py b/src/backend/core/tests/documents/test_api_documents_ask_for_access.py index 4e09dc5d10..321315e891 100644 --- a/src/backend/core/tests/documents/test_api_documents_ask_for_access.py +++ b/src/backend/core/tests/documents/test_api_documents_ask_for_access.py @@ -749,6 +749,53 @@ def test_api_documents_ask_for_access_accept_authenticated_owner_or_admin_update assert document_access.role == RoleChoices.ADMIN +def test_api_documents_ask_for_access_accept_admin_cannot_accept_owner_role(): + """ + Admin users should not be able to accept document ask for access with the owner role. + """ + user = UserFactory() + document = DocumentFactory(users=[(user, RoleChoices.ADMIN)]) + document_ask_for_access = DocumentAskForAccessFactory( + document=document, role=RoleChoices.READER + ) + + client = APIClient() + client.force_login(user) + + response = client.post( + f"/api/v1.0/documents/{document.id}/ask-for-access/{document_ask_for_access.id}/accept/", + data={"role": RoleChoices.OWNER}, + ) + assert response.status_code == 400 + assert response.json() == { + "detail": "You cannot accept a role higher than your own." + } + + +def test_api_documents_ask_for_access_accept_owner_can_accept_owner_role(): + """ + Owner users should be able to accept document ask for access with the owner role. + """ + user = UserFactory() + document = DocumentFactory(users=[(user, RoleChoices.OWNER)]) + document_ask_for_access = DocumentAskForAccessFactory( + document=document, role=RoleChoices.READER + ) + + client = APIClient() + client.force_login(user) + + response = client.post( + f"/api/v1.0/documents/{document.id}/ask-for-access/{document_ask_for_access.id}/accept/", + data={"role": RoleChoices.OWNER}, + ) + assert response.status_code == 204 + + assert not DocumentAskForAccess.objects.filter( + id=document_ask_for_access.id + ).exists() + + @pytest.mark.parametrize("role", [RoleChoices.OWNER, RoleChoices.ADMIN]) def test_api_documents_ask_for_access_accept_authenticated_non_root_document(role): """ From bf68a5ae40bc9bebf52739d7037fbda75c34dfb7 Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Wed, 12 Nov 2025 11:58:50 +0100 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F(backend)=20remove=20o?= =?UTF-8?q?wner=20as=20valid=20role=20for=20ask=5Ffor=5Faccess=20serialize?= =?UTF-8?q?r?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a ask_for_access creation is made, we explicitly remove the owner role to prevent role escalation. --- CHANGELOG.md | 4 +++ src/backend/core/api/serializers.py | 8 +++--- .../test_api_documents_ask_for_access.py | 27 ++++++++++++++++--- 3 files changed, 33 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e42e2206a3..c05cae1fe5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,10 @@ and this project adheres to - 🐛(frontend) fix pdf embed to use full width #1526 - 🐛(pdf) fix table cell alignment issue in exported documents #1582 +### Security + +- mitigate role escalation in the ask_for_access viewset #1580 + ## [3.9.0] - 2025-11-10 ### Added diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index 81b26d5e80..c175745f3f 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -786,7 +786,9 @@ class DocumentAskForAccessCreateSerializer(serializers.Serializer): """Serializer for creating a document ask for access.""" role = serializers.ChoiceField( - choices=models.RoleChoices.choices, + choices=[ + role for role in choices.RoleChoices if role != models.RoleChoices.OWNER + ], required=False, default=models.RoleChoices.READER, ) @@ -810,11 +812,11 @@ class Meta: ] read_only_fields = ["id", "document", "user", "role", "created_at", "abilities"] - def get_abilities(self, invitation) -> dict: + def get_abilities(self, instance) -> dict: """Return abilities of the logged-in user on the instance.""" request = self.context.get("request") if request: - return invitation.get_abilities(request.user) + return instance.get_abilities(request.user) return {} diff --git a/src/backend/core/tests/documents/test_api_documents_ask_for_access.py b/src/backend/core/tests/documents/test_api_documents_ask_for_access.py index 321315e891..e566ff1448 100644 --- a/src/backend/core/tests/documents/test_api_documents_ask_for_access.py +++ b/src/backend/core/tests/documents/test_api_documents_ask_for_access.py @@ -115,7 +115,10 @@ def test_api_documents_ask_for_access_create_authenticated_non_root_document(): assert response.status_code == 404 -def test_api_documents_ask_for_access_create_authenticated_specific_role(): +@pytest.mark.parametrize( + "role", [role for role in RoleChoices if role != RoleChoices.OWNER] +) +def test_api_documents_ask_for_access_create_authenticated_specific_role(role): """ Authenticated users should be able to create a document ask for access with a specific role. """ @@ -127,17 +130,35 @@ def test_api_documents_ask_for_access_create_authenticated_specific_role(): response = client.post( f"/api/v1.0/documents/{document.id}/ask-for-access/", - data={"role": RoleChoices.EDITOR}, + data={"role": role}, ) assert response.status_code == 201 assert DocumentAskForAccess.objects.filter( document=document, user=user, - role=RoleChoices.EDITOR, + role=role, ).exists() +def test_api_documents_ask_for_access_create_authenticated_owner_role(): + """ + Authenticated users should not be able to create a document ask for access with the owner role. + """ + document = DocumentFactory() + user = UserFactory() + + client = APIClient() + client.force_login(user) + + response = client.post( + f"/api/v1.0/documents/{document.id}/ask-for-access/", + data={"role": RoleChoices.OWNER}, + ) + assert response.status_code == 400 + assert response.json() == {"role": ['"owner" is not a valid choice.']} + + def test_api_documents_ask_for_access_create_authenticated_already_has_access(): """Authenticated users with existing access can ask for access with a different role.""" user = UserFactory() From 1292c33a58e4a826921b3fbf792aa385b55215a2 Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Thu, 13 Nov 2025 14:44:28 +0100 Subject: [PATCH 3/4] =?UTF-8?q?=E2=99=BB=EF=B8=8F(backend)=20rely=20on=20s?= =?UTF-8?q?et=5Frole=5Fto=20from=20DocumentAskForAccess=20abilities?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Like in other abilities, we compute a set_role_to property on the abilities. This set_role_to contains all the roles lower or equal than the current user role. We rely on this propoerty to validate the accept endpoint and it will be used by the front allpication to built the role select list. --- src/backend/core/api/viewsets.py | 11 ++++----- src/backend/core/models.py | 24 +++++++------------ .../test_api_documents_ask_for_access.py | 19 +++++++++++++++ 3 files changed, 32 insertions(+), 22 deletions(-) diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index a9f99d0444..21cd647392 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -2162,13 +2162,12 @@ def accept(self, request, *args, **kwargs): serializer = serializers.RoleSerializer(data=request.data) serializer.is_valid(raise_exception=True) - document = self.get_document_or_404() - user_role = document.get_role(request.user) - target_role = serializer.validated_data.get("role") + target_role = serializer.validated_data.get( + "role", document_ask_for_access.role + ) + abilities = document_ask_for_access.get_abilities(request.user) - if models.RoleChoices.get_priority(user_role) < models.RoleChoices.get_priority( - target_role - ): + if target_role not in abilities["set_role_to"]: return drf.response.Response( {"detail": "You cannot accept a role higher than your own."}, status=drf.status.HTTP_400_BAD_REQUEST, diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 6e0ad69e4f..7a93b82c96 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -1205,23 +1205,14 @@ def __str__(self): def get_abilities(self, user): """Compute and return abilities for a given user.""" - roles = [] + user_role = self.document.get_role(user) + is_admin_or_owner = user_role in PRIVILEGED_ROLES - if user.is_authenticated: - teams = user.teams - try: - roles = self.user_roles or [] - except AttributeError: - try: - roles = self.document.accesses.filter( - models.Q(user=user) | models.Q(team__in=teams), - ).values_list("role", flat=True) - except (self._meta.model.DoesNotExist, IndexError): - roles = [] - - is_admin_or_owner = bool( - set(roles).intersection({RoleChoices.OWNER, RoleChoices.ADMIN}) - ) + set_role_to = [ + role + for role in RoleChoices.values + if RoleChoices.get_priority(role) <= RoleChoices.get_priority(user_role) + ] return { "destroy": is_admin_or_owner, @@ -1229,6 +1220,7 @@ def get_abilities(self, user): "partial_update": is_admin_or_owner, "retrieve": is_admin_or_owner, "accept": is_admin_or_owner, + "set_role_to": set_role_to, } def accept(self, role=None): diff --git a/src/backend/core/tests/documents/test_api_documents_ask_for_access.py b/src/backend/core/tests/documents/test_api_documents_ask_for_access.py index e566ff1448..2d93b75313 100644 --- a/src/backend/core/tests/documents/test_api_documents_ask_for_access.py +++ b/src/backend/core/tests/documents/test_api_documents_ask_for_access.py @@ -287,6 +287,7 @@ def test_api_documents_ask_for_access_list_authenticated_own_request(): "update": False, "partial_update": False, "retrieve": False, + "set_role_to": [], }, } ], @@ -356,6 +357,15 @@ def test_api_documents_ask_for_access_list_owner_or_admin(role): response = client.get(f"/api/v1.0/documents/{document.id}/ask-for-access/") assert response.status_code == 200 + + expected_set_role_to = [ + RoleChoices.READER, + RoleChoices.EDITOR, + RoleChoices.ADMIN, + ] + if role == RoleChoices.OWNER: + expected_set_role_to.append(RoleChoices.OWNER) + assert response.json() == { "count": 3, "next": None, @@ -375,6 +385,7 @@ def test_api_documents_ask_for_access_list_owner_or_admin(role): "update": True, "partial_update": True, "retrieve": True, + "set_role_to": expected_set_role_to, }, } for document_ask_for_access in document_ask_for_accesses @@ -467,6 +478,13 @@ def test_api_documents_ask_for_access_retrieve_owner_or_admin(role): f"/api/v1.0/documents/{document.id}/ask-for-access/{document_ask_for_access.id}/" ) assert response.status_code == 200 + expected_set_role_to = [ + RoleChoices.READER, + RoleChoices.EDITOR, + RoleChoices.ADMIN, + ] + if role == RoleChoices.OWNER: + expected_set_role_to.append(RoleChoices.OWNER) assert response.json() == { "id": str(document_ask_for_access.id), "document": str(document.id), @@ -481,6 +499,7 @@ def test_api_documents_ask_for_access_retrieve_owner_or_admin(role): "update": True, "partial_update": True, "retrieve": True, + "set_role_to": expected_set_role_to, }, } From b069310bf05d874bbaccd5a33a566a778bb427bd Mon Sep 17 00:00:00 2001 From: Anthony LC Date: Fri, 14 Nov 2025 11:29:19 +0100 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=9B=82(frontend)=20disabled=20role=20?= =?UTF-8?q?not=20allowed=20to=20be=20assigned?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We disable roles that the current user is not allowed to assign when sharing a document. This prevents users from selecting roles they cannot actually assign, improving the user experience and reducing confusion. --- .../__tests__/app-impress/doc-member-list.spec.ts | 13 ++++++++++++- .../apps/e2e/__tests__/app-impress/utils-share.ts | 4 ++-- .../src/features/docs/doc-management/types.tsx | 1 + .../docs/doc-share/components/DocRoleDropdown.tsx | 4 +++- .../doc-share/components/DocShareAccessRequest.tsx | 1 + 5 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/frontend/apps/e2e/__tests__/app-impress/doc-member-list.spec.ts b/src/frontend/apps/e2e/__tests__/app-impress/doc-member-list.spec.ts index 9e0ee759ce..22d56e58bb 100644 --- a/src/frontend/apps/e2e/__tests__/app-impress/doc-member-list.spec.ts +++ b/src/frontend/apps/e2e/__tests__/app-impress/doc-member-list.spec.ts @@ -160,6 +160,9 @@ test.describe('Document list members', () => { `You are the sole owner of this group, make another member the group owner before you can change your own role or be removed from your document.`, ); await expect(soloOwner).toBeVisible(); + await expect( + page.getByRole('menuitem', { name: 'Administrator' }), + ).toBeDisabled(); await list.click({ force: true, // Force click to close the dropdown @@ -186,9 +189,17 @@ test.describe('Document list members', () => { await list.click(); await expect(currentUserRole).toBeVisible(); + await newUserRoles.click(); + await expect(page.getByRole('menuitem', { name: 'Owner' })).toBeDisabled(); + await list.click({ + force: true, // Force click to close the dropdown + }); + await currentUserRole.click(); await page.getByRole('menuitem', { name: 'Reader' }).click(); - await list.click(); + await list.click({ + force: true, // Force click to close the dropdown + }); await expect(currentUserRole).toBeHidden(); }); diff --git a/src/frontend/apps/e2e/__tests__/app-impress/utils-share.ts b/src/frontend/apps/e2e/__tests__/app-impress/utils-share.ts index d1305f2f38..23dfc0aaf6 100644 --- a/src/frontend/apps/e2e/__tests__/app-impress/utils-share.ts +++ b/src/frontend/apps/e2e/__tests__/app-impress/utils-share.ts @@ -7,14 +7,14 @@ import { verifyDocName, } from './utils-common'; -export type Role = 'Administrator' | 'Owner' | 'Member' | 'Editor' | 'Reader'; +export type Role = 'Administrator' | 'Owner' | 'Editor' | 'Reader'; export type LinkReach = 'Private' | 'Connected' | 'Public'; export type LinkRole = 'Reading' | 'Editing'; export const addNewMember = async ( page: Page, index: number, - role: 'Administrator' | 'Owner' | 'Editor' | 'Reader', + role: Role, fillText = 'user.test', ) => { const responsePromiseSearchUser = page.waitForResponse( diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/types.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/types.tsx index d786366e2d..f1c5d7215b 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-management/types.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-management/types.tsx @@ -132,5 +132,6 @@ export interface AccessRequest { partial_update: boolean; retrieve: boolean; accept: boolean; + set_role_to: Role[]; }; } diff --git a/src/frontend/apps/impress/src/features/docs/doc-share/components/DocRoleDropdown.tsx b/src/frontend/apps/impress/src/features/docs/doc-share/components/DocRoleDropdown.tsx index 2deb388c57..1796e5192d 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-share/components/DocRoleDropdown.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-share/components/DocRoleDropdown.tsx @@ -90,12 +90,14 @@ export const DocRoleDropdown = ({ const roles: DropdownMenuOption[] = Object.keys(translatedRoles).map( (key, index) => { const isLast = index === Object.keys(translatedRoles).length - 1; + const isRoleAllowed = rolesAllowed?.includes(key as Role) ?? true; + return { label: transRole(key as Role), callback: () => onSelectRole?.(key as Role), isSelected: currentRole === (key as Role), showSeparator: isLast, - disabled: isLastOwner && key !== 'owner', + disabled: (isLastOwner && key !== 'owner') || !isRoleAllowed, }; }, ); diff --git a/src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareAccessRequest.tsx b/src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareAccessRequest.tsx index d04cdb9c02..5bdd1cdbc2 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareAccessRequest.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareAccessRequest.tsx @@ -77,6 +77,7 @@ const DocShareAccessRequestItem = ({ doc, accessRequest }: Props) => { currentRole={role} onSelectRole={setRole} canUpdate={doc.abilities.accesses_manage} + rolesAllowed={accessRequest.abilities.set_role_to} />