diff --git a/docs/team-security-api.md b/docs/team-security-api.md new file mode 100644 index 00000000..954f6857 --- /dev/null +++ b/docs/team-security-api.md @@ -0,0 +1,56 @@ +# Legacy-команда: авторизация и контролируемые ошибки (PR A) + +База: `dev 0976fdf9ff08e85922a38508509fc31347ef1fd3`. +Область: Project / Collaborator; без новых конкурсных правил и изменений схемы. + +## До изменения и целевое поведение + +`SwitchLeaderRole.patch` и `ProjectCollaborators.delete` обходили object permission. +Публичность проекта позволяла постороннему удалить участника, а смена лидера была +доступна постороннему даже для приватного проекта. Проверки должны выполняться +до mutation в HTTP-слое и повторно на заблокированном Project внутри service. + +Управляет командой только текущий лидер. Read involvement, staff и участие в +команде сами по себе не дают права удалить другого участника или сменить лидера. +Участник по-прежнему может выйти сам; лидер сначала должен передать лидерство. + +## API до / после + +Все существующие пути, GET и успешные ответы `204` сохранены. +Ошибки service имеют форму `{"code": "stable_code", "detail": "сообщение"}`. + +| Операция / ситуация | До | После | +| --- | --- | --- | +| PATCH `/projects/{id}/collaborators/{user}/switch-leader/`, outsider | 204 | 403 `not_project_team_manager` | +| DELETE `/projects/{id}/collaborators/?id={user}`, outsider | 204 на публичном проекте | 403 `not_project_team_manager` | +| Удаление лидером самого себя | 500 | 422 `leader_cannot_leave` | +| Удаление отсутствующего участника | 500 | 422 `collaborator_not_found` | +| Некорректный / отсутствующий query `id` | 500 | 422 `invalid_collaborator_id` | +| POST direct add | 500 AttributeError | 405 `direct_member_add_unsupported` после авторизации | +| Смена на текущего лидера | 422 | 422 `already_project_leader` | +| Несуществующий проект | 404 | 404 | + +Direct add не поддерживался рабочей реализацией; добавление через Invite сохраняется. +Для операций применяется порядок locks `Project → Collaborator` внутри atomic. +Лидер повторно проверяется после получения Project lock, поэтому разрешение, +выданное до concurrent switch, не позволяет бывшему лидеру продолжить mutation. + +## Acceptance criteria и evidence + +| AC | Предусловие / действие / результат | Проверка | +| --- | --- | --- | +| A1 | Outsider switch public/private → 403, лидер сохранён | `test_outsider_cannot_remove_or_switch_public_or_private_project` | +| A2 | Outsider remove public/private → 403, участник сохранён | тот же тест | +| A3 | Read collaborator не получает manage permission | `test_read_involvement_does_not_grant_team_management` | +| A4 | Лидер может удалить участника и передать лидерство | существующий `ProjectCollaboratorRegressionTests` | +| A5 | Self-remove, missing, malformed ID, direct add → controlled 4xx | `TeamSecurityTests` | +| A6 | Вызов service без HTTP gate также проверяет actor | `test_service_rechecks_actor_without_http_permission_gate` | +| A7 | Бывший лидер после switch теряет manage permission | `test_former_leader_cannot_manage_after_switch` | + +## Совместимость и границы + +Angular остаётся consumer существующих маршрутов. Ошибочный Angular DELETE query +теперь даёт 422 вместо 500; исправление самого URL относится к отдельному PR D. +React не исследовался и не менялся согласно ТЗ. UI / Design System не затронуты. +Freeze, min/max, program context и exclusivity относятся к PR C; lifecycle Invite — +к PR B. Migrations и DB constraints отсутствуют. Production data не изменяются. diff --git a/projects/permissions.py b/projects/permissions.py index b555f7d1..7bb0890d 100644 --- a/projects/permissions.py +++ b/projects/permissions.py @@ -125,6 +125,14 @@ def has_object_permission(self, request, view, obj): return False +class IsProjectTeamManager(IsProjectLeader): + def has_object_permission(self, request, view, obj): + from projects.team_service import require_team_manager + + require_team_manager(obj, request.user) + return True + + class HasInvolvementInProjectOrReadOnly(BasePermission): """ READ: published projects pass this gate; drafts use shared restricted visibility. diff --git a/projects/team_service.py b/projects/team_service.py new file mode 100644 index 00000000..67e82e20 --- /dev/null +++ b/projects/team_service.py @@ -0,0 +1,93 @@ +"""Операции над legacy-командой: авторизация и изменения в одной транзакции.""" + +from django.db import transaction +from django.shortcuts import get_object_or_404 +from rest_framework.exceptions import APIException + +from projects.models import Collaborator, Project + + +class TeamError(APIException): + status_code = 409 + + def __init__(self, code, detail, *, status_code=409): + self.status_code = status_code + super().__init__({"code": code, "detail": detail}, code=code) + + +def require_team_manager(project, actor): + if not actor or not actor.is_authenticated or project.leader_id != actor.pk: + raise TeamError( + "not_project_team_manager", + "Управлять командой может только лидер проекта.", + status_code=403, + ) + + +def _locked_project(project_id): + return get_object_or_404(Project.objects.select_for_update(), pk=project_id) + + +def _locked_member(project, user_id): + member = ( + Collaborator.objects.select_for_update() + .filter(project=project, user_id=user_id) + .first() + ) + if member is None: + raise TeamError( + "collaborator_not_found", + "Пользователь не является участником проекта.", + status_code=422, + ) + return member + + +def _require_not_leader(project, user_id): + if project.leader_id == user_id: + raise TeamError( + "leader_cannot_leave", + "Перед выходом из проекта передайте права лидера другому участнику.", + status_code=422, + ) + + +@transaction.atomic +def remove_member(*, project_id, user_id, actor): + project = _locked_project(project_id) + require_team_manager(project, actor) + _require_not_leader(project, user_id) + _locked_member(project, user_id).delete() + + +@transaction.atomic +def leave_team(*, project_id, actor): + project = _locked_project(project_id) + _require_not_leader(project, actor.pk) + _locked_member(project, actor.pk).delete() + + +@transaction.atomic +def switch_leader(*, project_id, user_id, actor): + project = _locked_project(project_id) + require_team_manager(project, actor) + if project.leader_id == user_id: + raise TeamError( + "already_project_leader", + "Пользователь уже является лидером проекта.", + status_code=422, + ) + member = _locked_member(project, user_id) + project.leader_id = member.user_id + project.save(update_fields=["leader", "datetime_updated"]) + + +@transaction.atomic +def reject_direct_add(*, project_id, actor): + project = _locked_project(project_id) + require_team_manager(project, actor) + raise TeamError( + "direct_member_add_unsupported", + "Для добавления участника отправьте приглашение в проект.", + status_code=405, + ) diff --git a/projects/tests/test_team_security.py b/projects/tests/test_team_security.py new file mode 100644 index 00000000..7b18319d --- /dev/null +++ b/projects/tests/test_team_security.py @@ -0,0 +1,126 @@ +from django.test import TestCase +from rest_framework.test import APIClient + +from projects import team_service +from projects.models import Collaborator +from projects.tests.helpers import ( + create_collaborator, + create_project_context, + create_user, +) + + +class TeamSecurityTests(TestCase): + def setUp(self): + context = create_project_context(draft=True) + self.project, self.leader = context.project, context.user + self.member = create_user() + create_collaborator(self.project, user=self.member) + self.outsider = create_user() + self.client = APIClient() + self.remove_url = f"/projects/{self.project.pk}/collaborators/" + self.switch_url = f"{self.remove_url}{self.member.pk}/switch-leader/" + + def assert_error(self, response, status_code, code): + self.assertEqual(response.status_code, status_code, response.data) + self.assertEqual(response.data["code"], code) + + def test_outsider_cannot_remove_or_switch_public_or_private_project(self): + self.client.force_authenticate(self.outsider) + for is_public in (True, False): + with self.subTest(is_public=is_public): + self.project.is_public = is_public + self.project.save(update_fields=["is_public"]) + for response in ( + self.client.delete(self.remove_url, {"id": self.member.pk}), + self.client.patch(self.switch_url), + ): + self.assert_error(response, 403, "not_project_team_manager") + self.project.refresh_from_db() + self.assertEqual(self.project.leader_id, self.leader.pk) + self.assertTrue( + Collaborator.objects.filter( + project=self.project, user=self.member + ).exists() + ) + + def test_read_involvement_does_not_grant_team_management(self): + self.client.force_authenticate(self.member) + self.assertEqual(self.client.get(self.remove_url).status_code, 200) + self.assert_error( + self.client.patch(self.switch_url), 403, "not_project_team_manager" + ) + self.assert_error( + self.client.delete(f"{self.remove_url}?id={self.outsider.pk}"), + 403, + "not_project_team_manager", + ) + + def test_leader_self_removal_is_controlled(self): + self.client.force_authenticate(self.leader) + self.assert_error( + self.client.delete(f"{self.remove_url}?id={self.leader.pk}"), + 422, + "leader_cannot_leave", + ) + + def test_missing_or_repeated_removal_is_controlled(self): + self.client.force_authenticate(self.leader) + url = f"{self.remove_url}?id={self.member.pk}" + self.assertEqual(self.client.delete(url).status_code, 204) + self.assert_error(self.client.delete(url), 422, "collaborator_not_found") + + def test_malformed_collaborator_ids_are_controlled(self): + self.client.force_authenticate(self.leader) + for query in ("", "?id=", "?id=7/", "?id=text", "?id=-1"): + with self.subTest(query=query): + self.assert_error( + self.client.delete(self.remove_url + query), + 422, + "invalid_collaborator_id", + ) + + def test_direct_add_is_explicitly_unsupported_for_leader(self): + self.client.force_authenticate(self.leader) + self.assert_error( + self.client.post(self.remove_url, {}, format="json"), + 405, + "direct_member_add_unsupported", + ) + + def test_direct_add_rejects_outsider_before_input_validation(self): + self.client.force_authenticate(self.outsider) + self.assert_error( + self.client.post(self.remove_url, {}, format="json"), + 403, + "not_project_team_manager", + ) + + def test_service_rechecks_actor_without_http_permission_gate(self): + for operation in (team_service.remove_member, team_service.switch_leader): + with self.subTest(operation=operation.__name__): + with self.assertRaises(team_service.TeamError) as raised: + operation( + project_id=self.project.pk, + user_id=self.member.pk, + actor=self.outsider, + ) + self.assertEqual(raised.exception.status_code, 403) + + def test_former_leader_cannot_manage_after_switch(self): + self.client.force_authenticate(self.leader) + self.assertEqual(self.client.patch(self.switch_url).status_code, 204) + self.assert_error( + self.client.delete(f"{self.remove_url}?id={self.member.pk}"), + 403, + "not_project_team_manager", + ) + + def test_missing_project_returns_404(self): + self.client.force_authenticate(self.leader) + self.assertEqual( + self.client.patch( + f"/projects/2147483647/collaborators/{self.member.pk}/switch-leader/" + ).status_code, + 404, + ) diff --git a/projects/views.py b/projects/views.py index c4e92aa7..3c6c6f13 100644 --- a/projects/views.py +++ b/projects/views.py @@ -1,8 +1,6 @@ import logging -from typing import Annotated from django.contrib.auth import get_user_model -from django.core.exceptions import ObjectDoesNotExist from django.db import transaction from django.db.models import QuerySet from django.shortcuts import get_object_or_404 @@ -23,7 +21,6 @@ PartnerProgramProject, PartnerProgramUserProfile, ) -from projects.exceptions import CollaboratorDoesNotExist from projects.cover_reset import reset_project_cover from projects.filters import ProjectFilter from projects.helpers import ( @@ -41,10 +38,12 @@ Resource, ) from projects.pagination import ProjectsPagination +from projects import team_service from projects.permissions import ( CanBindProjectToProgram, HasInvolvementInProjectOrReadOnly, IsProjectLeader, + IsProjectTeamManager, IsProjectLeaderOrReadOnly, IsProjectLeaderOrReadOnlyForNonDrafts, ProjectVisibilityPermission, @@ -284,59 +283,31 @@ def get(self, request, pk: int): serializer = self.get_serializer(instance) return Response(serializer.data) + def get_permissions(self): + if self.request.method in permissions.SAFE_METHODS: + return super().get_permissions() + return [IsAuthenticated(), IsProjectTeamManager()] + def post(self, request, pk: int): - """add collaborators to the project""" - m2m_manager = self.get_object().collaborators - serializer = self.get_serializer(data=request.data) - serializer.is_valid(raise_exception=True) - collaborators = serializer.validated_data["collaborators"] - for user in collaborators: - m2m_manager.add(user) - return Response(status=200) + project = self.get_object() + team_service.reject_direct_add(project_id=project.pk, actor=request.user) def delete(self, request, pk: int): - """delete collaborator from project""" - requested_collab_id: int = int(self.request.query_params.get("id")) - - project_id, leader_id = self._project_data(pk) - existing_collab_id = self._collabs_queryset( - project_id, requested_collab_id, leader_id - ) - - if leader_id == requested_collab_id: - return Response( - { - "error": f"User with id: {leader_id} is a leader of a project. " - f"Be careful not to delete yourself from a project!" - }, - status=status.HTTP_422_UNPROCESSABLE_ENTITY, - ) - if not existing_collab_id: - return Response( - { - "error": f"User with id: {requested_collab_id} are not part of this project." - }, - status=status.HTTP_422_UNPROCESSABLE_ENTITY, + project = self.get_object() + try: + user_id = int(request.query_params.get("id")) + if user_id <= 0: + raise ValueError + except (TypeError, ValueError): + raise team_service.TeamError( + "invalid_collaborator_id", + "Укажите корректный ID участника.", + status_code=422, ) - - existing_collab_id.delete() - return Response(status=204) - - def _project_data( - self, project_pk: int - ) -> tuple[Annotated[int, "ID проекта"], Annotated[int, "ID лидера проекта"]]: - project = get_object_or_404( - Project.objects.select_related("leader"), id=project_pk - ) - return project.id, project.leader.id - - @staticmethod - def _collabs_queryset(project_id: int, requested_id: int, leader_id: int) -> QuerySet: - return Collaborator.objects.exclude( - user__id=leader_id - ).get( # чтоб случайно лидер сам себя не удалил - user__id=requested_id, project__id=project_id + team_service.remove_member( + project_id=project.pk, user_id=user_id, actor=request.user ) + return Response(status=204) class AchievementList(generics.ListCreateAPIView): @@ -483,108 +454,29 @@ class LeaveProject(generics.GenericAPIView): permission_classes = [IsAuthenticated] def delete(self, request, project_pk: int) -> Response: - current_user_id = self.request.user.id - collaborator = get_object_or_404( - Collaborator.objects.all(), - project_id=project_pk, - user_id=current_user_id, - ) - project = Project.objects.select_related("leader").get(id=project_pk) - if project.leader.id == current_user_id: - return Response( - { - "error": "You can't leave if you are a leader of a project. " - "Please, switch leadership!" - }, - status=status.HTTP_422_UNPROCESSABLE_ENTITY, - ) - collaborator.delete() + team_service.leave_team(project_id=project_pk, actor=request.user) return Response(status=204) -class DeleteProjectCollaborators(generics.GenericAPIView): - permission_classes = [IsProjectLeader] - - def _project_data( - self, project_pk: int - ) -> tuple[Annotated[int, "ID проекта"], Annotated[int, "ID лидера проекта"]]: - project = get_object_or_404( - Project.objects.select_related("leader"), id=project_pk - ) - return project.id, project.leader.id - - @staticmethod - def _collabs_queryset(project_id: int, requested_id: int, leader_id: int) -> QuerySet: - return Collaborator.objects.exclude( - user__id=leader_id - ).get( # чтоб случайно лидер сам себя не удалил - user__id=requested_id, project__id=project_id - ) +class DeleteProjectCollaborators(ProjectCollaborators): + # Сохраняем legacy-класс без отдельного обходного пути авторизации. + lookup_url_kwarg = "project_pk" def delete(self, request, project_pk: int) -> Response: - requested_collab_id: int = int(self.request.query_params.get("id")) - - project_id, leader_id = self._project_data(project_pk) - existing_collab_id = self._collabs_queryset( - project_id, requested_collab_id, leader_id - ) - - if leader_id == requested_collab_id: - return Response( - { - "error": f"User with id: {leader_id} is a leader of a project. " - f"Be careful not to delete yourself from a project!" - }, - status=status.HTTP_422_UNPROCESSABLE_ENTITY, - ) - if not existing_collab_id: - return Response( - { - "error": f"User with id: {requested_collab_id} are not part of this project." - }, - status=status.HTTP_422_UNPROCESSABLE_ENTITY, - ) - - existing_collab_id.delete() - return Response(status=204) + return super().delete(request, pk=project_pk) class SwitchLeaderRole(generics.GenericAPIView): - permission_classes = [IsProjectLeader] - queryset = Project.objects.all().select_related("leader") + permission_classes = [IsAuthenticated, IsProjectTeamManager] + queryset = Project.objects.all() serializer_class = EmptySerializer - - @staticmethod - def _get_new_leader(user_id: int, project: Project) -> Collaborator: - try: - return Collaborator.objects.select_related("user").get( - user_id=user_id, project=project - ) - except ObjectDoesNotExist: - raise CollaboratorDoesNotExist( - f"""Collaborator with user_id: {user_id} does not exist. Either user_id is not correct, or project_id - is not correct, or try adding this user to a project (as collaborator) before making them a leader. """ - ) - - @staticmethod - def _get_project(project_pk: int) -> Project: - return get_object_or_404(Project.objects.all(), id=project_pk) + lookup_url_kwarg = "project_pk" def patch(self, request, project_pk: int, user_to_leader_pk: int) -> Response: - project = self._get_project(project_pk) - - new_leader_id = user_to_leader_pk - - if project.leader.id == new_leader_id: - return Response( - {"error": "User is already a leader of a project"}, - status=status.HTTP_422_UNPROCESSABLE_ENTITY, - ) - - new_leader = self._get_new_leader(new_leader_id, project) - - project.leader = new_leader.user - project.save() + project = self.get_object() + team_service.switch_leader( + project_id=project.pk, user_id=user_to_leader_pk, actor=request.user + ) return Response(status=204)