diff --git a/CHANGELOG.md b/CHANGELOG.md index ed4f587b..44206ea4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to ## Fixed - 🐛(backend) compute ancestor_links in get_abilities if needed #725 +- 🔒️(back) restrict access to document accesses #801 ## [2.6.0] - 2025-03-21 diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index 387cf0d8..268b5874 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -27,6 +27,26 @@ class UserSerializer(serializers.ModelSerializer): read_only_fields = ["id", "email", "full_name", "short_name"] +class UserLightSerializer(UserSerializer): + """Serialize users with limited fields.""" + + id = serializers.SerializerMethodField(read_only=True) + email = serializers.SerializerMethodField(read_only=True) + + def get_id(self, _user): + """Return always None. Here to have the same fields than in UserSerializer.""" + return None + + def get_email(self, _user): + """Return always None. Here to have the same fields than in UserSerializer.""" + return None + + class Meta: + model = models.User + fields = ["id", "email", "full_name", "short_name"] + read_only_fields = ["id", "email", "full_name", "short_name"] + + class BaseAccessSerializer(serializers.ModelSerializer): """Serialize template accesses.""" @@ -118,6 +138,17 @@ class DocumentAccessSerializer(BaseAccessSerializer): read_only_fields = ["id", "abilities"] +class DocumentAccessLightSerializer(DocumentAccessSerializer): + """Serialize document accesses with limited fields.""" + + user = UserLightSerializer(read_only=True) + + class Meta: + model = models.DocumentAccess + fields = ["id", "user", "team", "role", "abilities"] + read_only_fields = ["id", "team", "role", "abilities"] + + class TemplateAccessSerializer(BaseAccessSerializer): """Serialize template accesses.""" diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 71b030d3..432ca02c 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -1420,12 +1420,7 @@ class DocumentViewSet( class DocumentAccessViewSet( ResourceAccessViewsetMixin, - drf.mixins.CreateModelMixin, - drf.mixins.DestroyModelMixin, - drf.mixins.ListModelMixin, - drf.mixins.RetrieveModelMixin, - drf.mixins.UpdateModelMixin, - viewsets.GenericViewSet, + viewsets.ModelViewSet, ): """ API ViewSet for all interactions with document accesses. @@ -1457,6 +1452,32 @@ class DocumentAccessViewSet( queryset = models.DocumentAccess.objects.select_related("user").all() resource_field_name = "document" serializer_class = serializers.DocumentAccessSerializer + is_current_user_owner_or_admin = False + + def get_queryset(self): + """Return the queryset according to the action.""" + queryset = super().get_queryset() + + if self.action == "list": + try: + document = models.Document.objects.get(pk=self.kwargs["resource_id"]) + except models.Document.DoesNotExist: + return queryset.none() + + roles = set(document.get_roles(self.request.user)) + is_owner_or_admin = bool(roles.intersection(set(models.PRIVILEGED_ROLES))) + self.is_current_user_owner_or_admin = is_owner_or_admin + if not is_owner_or_admin: + # Return only the document owner access + queryset = queryset.filter(role__in=models.PRIVILEGED_ROLES) + + return queryset + + def get_serializer_class(self): + if self.action == "list" and not self.is_current_user_owner_or_admin: + return serializers.DocumentAccessLightSerializer + + return super().get_serializer_class() def perform_create(self, serializer): """Add a new access to the document and send an email to the new added user.""" diff --git a/src/backend/core/models.py b/src/backend/core/models.py index cff7933b..2c5239ea 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -364,10 +364,9 @@ class BaseAccess(BaseModel): class Meta: abstract = True - def _get_abilities(self, resource, user): + def _get_roles(self, resource, user): """ - Compute and return abilities for a given user taking into account - the current state of the object. + Get the roles a user has on a resource. """ roles = [] if user.is_authenticated: @@ -382,6 +381,15 @@ class BaseAccess(BaseModel): except (self._meta.model.DoesNotExist, IndexError): roles = [] + return roles + + def _get_abilities(self, resource, user): + """ + Compute and return abilities for a given user taking into account + the current state of the object. + """ + roles = self._get_roles(resource, user) + is_owner_or_admin = bool( set(roles).intersection({RoleChoices.OWNER, RoleChoices.ADMIN}) ) @@ -1103,7 +1111,41 @@ class DocumentAccess(BaseAccess): """ Compute and return abilities for a given user on the document access. """ - return self._get_abilities(self.document, user) + roles = self._get_roles(self.document, user) + is_owner_or_admin = bool(set(roles).intersection(set(PRIVILEGED_ROLES))) + if self.role == RoleChoices.OWNER: + can_delete = ( + RoleChoices.OWNER in roles + and self.document.accesses.filter(role=RoleChoices.OWNER).count() > 1 + ) + set_role_to = ( + [RoleChoices.ADMIN, RoleChoices.EDITOR, RoleChoices.READER] + if can_delete + else [] + ) + else: + can_delete = is_owner_or_admin + set_role_to = [] + if RoleChoices.OWNER in roles: + set_role_to.append(RoleChoices.OWNER) + if is_owner_or_admin: + set_role_to.extend( + [RoleChoices.ADMIN, RoleChoices.EDITOR, RoleChoices.READER] + ) + + # Remove the current role as we don't want to propose it as an option + try: + set_role_to.remove(self.role) + except ValueError: + pass + + return { + "destroy": can_delete, + "update": bool(set_role_to) and is_owner_or_admin, + "partial_update": bool(set_role_to) and is_owner_or_admin, + "retrieve": self.user and self.user.id == user.id or is_owner_or_admin, + "set_role_to": set_role_to, + } class Template(BaseModel): diff --git a/src/backend/core/tests/documents/test_api_document_accesses.py b/src/backend/core/tests/documents/test_api_document_accesses.py index dca6fa79..bf5ef182 100644 --- a/src/backend/core/tests/documents/test_api_document_accesses.py +++ b/src/backend/core/tests/documents/test_api_document_accesses.py @@ -59,8 +59,32 @@ def test_api_document_accesses_list_authenticated_unrelated(): } +def test_api_document_accesses_list_unexisting_document(): + """ + Listing document accesses for an unexisting document should return an empty list. + """ + user = factories.UserFactory() + + client = APIClient() + client.force_login(user) + + response = client.get(f"/api/v1.0/documents/{uuid4()!s}/accesses/") + assert response.status_code == 200 + assert response.json() == { + "count": 0, + "next": None, + "previous": None, + "results": [], + } + + @pytest.mark.parametrize("via", VIA) -def test_api_document_accesses_list_authenticated_related(via, mock_user_teams): +@pytest.mark.parametrize( + "role", [role for role in models.RoleChoices if role not in models.PRIVILEGED_ROLES] +) +def test_api_document_accesses_list_authenticated_related_non_privileged( + via, role, mock_user_teams +): """ Authenticated users should be able to list document accesses for a document to which they are directly related, whatever their role in the document. @@ -70,24 +94,114 @@ def test_api_document_accesses_list_authenticated_related(via, mock_user_teams): client = APIClient() client.force_login(user) - document = factories.DocumentFactory() + owner = factories.UserFactory() + accesses = [] + + document_access = factories.UserDocumentAccessFactory( + user=owner, role=models.RoleChoices.OWNER + ) + accesses.append(document_access) + document = document_access.document + if via == USER: + models.DocumentAccess.objects.create( + document=document, + user=user, + role=role, + ) + elif via == TEAM: + mock_user_teams.return_value = ["lasuite", "unknown"] + models.DocumentAccess.objects.create( + document=document, + team="lasuite", + role=role, + ) + + access1 = factories.TeamDocumentAccessFactory(document=document) + access2 = factories.UserDocumentAccessFactory(document=document) + accesses.append(access1) + accesses.append(access2) + + # Accesses for other documents to which the user is related should not be listed either + other_access = factories.UserDocumentAccessFactory(user=user) + factories.UserDocumentAccessFactory(document=other_access.document) + + response = client.get( + f"/api/v1.0/documents/{document.id!s}/accesses/", + ) + + # Return only owners + owners_accesses = [ + access for access in accesses if access.role in models.PRIVILEGED_ROLES + ] + assert response.status_code == 200 + content = response.json() + assert content["count"] == len(owners_accesses) + assert sorted(content["results"], key=lambda x: x["id"]) == sorted( + [ + { + "id": str(access.id), + "user": { + "id": None, + "email": None, + "full_name": access.user.full_name, + "short_name": access.user.short_name, + } + if access.user + else None, + "team": access.team, + "role": access.role, + "abilities": access.get_abilities(user), + } + for access in owners_accesses + ], + key=lambda x: x["id"], + ) + + for access in content["results"]: + assert access["role"] in models.PRIVILEGED_ROLES + + +@pytest.mark.parametrize("via", VIA) +@pytest.mark.parametrize("role", models.PRIVILEGED_ROLES) +def test_api_document_accesses_list_authenticated_related_privileged_roles( + via, role, mock_user_teams +): + """ + Authenticated users should be able to list document accesses for a document + to which they are directly related, whatever their role in the document. + """ + user = factories.UserFactory() + + client = APIClient() + client.force_login(user) + + owner = factories.UserFactory() + accesses = [] + + document_access = factories.UserDocumentAccessFactory( + user=owner, role=models.RoleChoices.OWNER + ) + accesses.append(document_access) + document = document_access.document user_access = None if via == USER: user_access = models.DocumentAccess.objects.create( document=document, user=user, - role=random.choice(models.RoleChoices.values), + role=role, ) elif via == TEAM: mock_user_teams.return_value = ["lasuite", "unknown"] user_access = models.DocumentAccess.objects.create( document=document, team="lasuite", - role=random.choice(models.RoleChoices.values), + role=role, ) access1 = factories.TeamDocumentAccessFactory(document=document) access2 = factories.UserDocumentAccessFactory(document=document) + accesses.append(access1) + accesses.append(access2) # Accesses for other documents to which the user is related should not be listed either other_access = factories.UserDocumentAccessFactory(user=user) @@ -102,7 +216,7 @@ def test_api_document_accesses_list_authenticated_related(via, mock_user_teams): assert response.status_code == 200 content = response.json() - assert len(content["results"]) == 3 + assert len(content["results"]) == 4 assert sorted(content["results"], key=lambda x: x["id"]) == sorted( [ { @@ -126,6 +240,13 @@ def test_api_document_accesses_list_authenticated_related(via, mock_user_teams): "role": access2.role, "abilities": access2.get_abilities(user), }, + { + "id": str(document_access.id), + "user": serializers.UserSerializer(instance=owner).data, + "team": "", + "role": models.RoleChoices.OWNER, + "abilities": document_access.get_abilities(user), + }, ], key=lambda x: x["id"], ) @@ -184,7 +305,10 @@ def test_api_document_accesses_retrieve_authenticated_unrelated(): @pytest.mark.parametrize("via", VIA) -def test_api_document_accesses_retrieve_authenticated_related(via, mock_user_teams): +@pytest.mark.parametrize("role", models.RoleChoices) +def test_api_document_accesses_retrieve_authenticated_related( + via, role, mock_user_teams +): """ A user who is related to a document should be allowed to retrieve the associated document user accesses. @@ -196,10 +320,12 @@ def test_api_document_accesses_retrieve_authenticated_related(via, mock_user_tea document = factories.DocumentFactory() if via == USER: - factories.UserDocumentAccessFactory(document=document, user=user) + factories.UserDocumentAccessFactory(document=document, user=user, role=role) elif via == TEAM: mock_user_teams.return_value = ["lasuite", "unknown"] - factories.TeamDocumentAccessFactory(document=document, team="lasuite") + factories.TeamDocumentAccessFactory( + document=document, team="lasuite", role=role + ) access = factories.UserDocumentAccessFactory(document=document) @@ -207,16 +333,19 @@ def test_api_document_accesses_retrieve_authenticated_related(via, mock_user_tea f"/api/v1.0/documents/{document.id!s}/accesses/{access.id!s}/", ) - access_user = serializers.UserSerializer(instance=access.user).data + if not role in models.PRIVILEGED_ROLES: + assert response.status_code == 403 + else: + access_user = serializers.UserSerializer(instance=access.user).data - assert response.status_code == 200 - assert response.json() == { - "id": str(access.id), - "user": access_user, - "team": "", - "role": access.role, - "abilities": access.get_abilities(user), - } + assert response.status_code == 200 + assert response.json() == { + "id": str(access.id), + "user": access_user, + "team": "", + "role": access.role, + "abilities": access.get_abilities(user), + } def test_api_document_accesses_update_anonymous(): diff --git a/src/backend/core/tests/test_models_document_accesses.py b/src/backend/core/tests/test_models_document_accesses.py index 62c03665..fe0e7c1c 100644 --- a/src/backend/core/tests/test_models_document_accesses.py +++ b/src/backend/core/tests/test_models_document_accesses.py @@ -7,7 +7,7 @@ from django.core.exceptions import ValidationError import pytest -from core import factories +from core import factories, models pytestmark = pytest.mark.django_db @@ -294,7 +294,7 @@ def test_models_document_access_get_abilities_for_editor_of_owner(): abilities = access.get_abilities(user) assert abilities == { "destroy": False, - "retrieve": True, + "retrieve": False, "update": False, "partial_update": False, "set_role_to": [], @@ -311,7 +311,7 @@ def test_models_document_access_get_abilities_for_editor_of_administrator(): abilities = access.get_abilities(user) assert abilities == { "destroy": False, - "retrieve": True, + "retrieve": False, "update": False, "partial_update": False, "set_role_to": [], @@ -333,7 +333,7 @@ def test_models_document_access_get_abilities_for_editor_of_editor_user( assert abilities == { "destroy": False, - "retrieve": True, + "retrieve": False, "update": False, "partial_update": False, "set_role_to": [], @@ -353,7 +353,7 @@ def test_models_document_access_get_abilities_for_reader_of_owner(): abilities = access.get_abilities(user) assert abilities == { "destroy": False, - "retrieve": True, + "retrieve": False, "update": False, "partial_update": False, "set_role_to": [], @@ -370,7 +370,7 @@ def test_models_document_access_get_abilities_for_reader_of_administrator(): abilities = access.get_abilities(user) assert abilities == { "destroy": False, - "retrieve": True, + "retrieve": False, "update": False, "partial_update": False, "set_role_to": [], @@ -392,7 +392,7 @@ def test_models_document_access_get_abilities_for_reader_of_reader_user( assert abilities == { "destroy": False, - "retrieve": True, + "retrieve": False, "update": False, "partial_update": False, "set_role_to": [], @@ -412,8 +412,16 @@ def test_models_document_access_get_abilities_preset_role(django_assert_num_quer assert abilities == { "destroy": False, - "retrieve": True, + "retrieve": False, "update": False, "partial_update": False, "set_role_to": [], } + + +@pytest.mark.parametrize("role", models.RoleChoices) +def test_models_document_access_get_abilities_retrieve_own_access(role): + """Check abilities of self access for the owner of a document.""" + access = factories.UserDocumentAccessFactory(role=role) + abilities = access.get_abilities(access.user) + assert abilities["retrieve"] is True