diff --git a/CHANGELOG.md b/CHANGELOG.md index e42e2206a..c05cae1fe 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 81b26d5e8..c175745f3 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/api/viewsets.py b/src/backend/core/api/viewsets.py index 84402ceaa..21cd64739 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -2162,7 +2162,18 @@ 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")) + target_role = serializer.validated_data.get( + "role", document_ask_for_access.role + ) + abilities = document_ask_for_access.get_abilities(request.user) + + 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, + ) + + 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/models.py b/src/backend/core/models.py index 6e0ad69e4..7a93b82c9 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 4e09dc5d1..2d93b7531 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() @@ -266,6 +287,7 @@ def test_api_documents_ask_for_access_list_authenticated_own_request(): "update": False, "partial_update": False, "retrieve": False, + "set_role_to": [], }, } ], @@ -335,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, @@ -354,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 @@ -446,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), @@ -460,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, }, } @@ -749,6 +789,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): """ 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 9e0ee759c..22d56e58b 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 d1305f2f3..23dfc0aaf 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 d786366e2..f1c5d7215 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 2deb388c5..1796e5192 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 d04cdb9c0..5bdd1cdbc 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} />