diff --git a/docs/employee-deletion-api-ru.md b/docs/employee-deletion-api-ru.md new file mode 100644 index 0000000..d4480e5 --- /dev/null +++ b/docs/employee-deletion-api-ru.md @@ -0,0 +1,98 @@ +# Полное удаление сотрудника + +Администратор может полностью удалить аккаунт сотрудника через существующий +маршрут управления пользователями: + +```http +DELETE /api/v1/users/admin/users/{user_id}/ +Authorization: Bearer +``` + +Тело запроса не требуется. Успех — `204 No Content` без JSON-тела. Сотрудник +исчезает из списка, поиска и карточки; повторный DELETE возвращает 404. +Деактивация и обратная активация остаются отдельными операциями. + +## Права и ошибки + +Право удаления имеет существующий активный пользователь с `is_staff=True`, +как и для остальных административных endpoint'ов. Проверка повторяется внутри +транзакции после получения блокировок: ранее аутентифицированный, но уже удалённый +или лишённый прав администратор не может завершить изменение. + +| HTTP | Код ошибки | Значение | +|---|---|---| +| 400 | `self_delete_forbidden` | Нельзя удалить самого себя | +| 401 | Стандартная ошибка аутентификации | Нет действующего access token | +| 403 | Стандартная ошибка прав / `permission_denied` | Требуется активный администратор | +| 404 | `not_found` | Сотрудник отсутствует, в том числе после удаления | +| 409 | `last_active_admin` | Нельзя оставить систему без активного администратора | +| 409 | `user_delete_conflict` | Защищённая связь препятствует удалению | + +Ошибки используют стандартный envelope проекта: `success: false`, `data: null`, +`errors: [{code, message}]`, `meta`. Запрет удаления себя имеет приоритет над +проверкой последнего администратора. Существующие связи не блокируют удаление; +409 для защищённой связи сохраняет безопасное поведение при расширении модели. + +DELETE, создание аккаунта администратором, смена роли/активности и соответствующие +операции Django admin используют одни блокировки пользователей. Пакетное действие +Django admin атомарно: если +выбран сам исполнитель, вся операция отклоняется. +Сохранение собственных учётных данных и смена пароля также блокируют актуальную +строку User, чтобы начавшийся до DELETE запрос не создал аккаунт заново. + +## Данные после удаления + +- Физически удаляются User, Profile, связи с группами/permissions, записи + OutstandingToken и связанные BlacklistedToken. Сами справочники прав остаются. +- Отчёты ReportUpload, загрузки реестров RegisterUpload, ExchangePackageImport + сохраняются; их ссылки на автора обнуляются штатным SET_NULL. Сохраняются + загруженные файлы, предметные данные и цепочки импортов. +- BackgroundJob сохраняются, `user_id` очищается явно, поскольку это integer, + а не внешний ключ. Выполняющиеся задачи не отменяются. +- Аватар удаляется из storage после commit, если его не использует другой + профиль. При ошибке storage удаление аккаунта остаётся успешным, ошибка + регистрируется в серверном журнале для последующей очистки файла. +- Технический Django admin LogEntry сохраняет штатный CASCADE для записей, + исполнителем которых был удалённый пользователь; предметные журналы выше + не удаляются. Содержимое исторических документов не переписывается. +- Старый access перестаёт проходить JWTAuthentication. Refresh endpoint + дополнительно проверяет существование и активность пользователя и возвращает + 401 для удалённого/неактивного аккаунта. Повторный refresh активного пользователя + поддерживается как раньше. Записей восстановления пароля в текущей модели нет. + +Изменений схемы БД и миграций не требуется. Откат версии приложения не +восстанавливает уже удалённые аккаунты. + +## Передача frontend-разработчику (Глебу) + +1. Добавить отдельное действие «Удалить» в меню сотрудника; для текущего + пользователя скрыть или отключить его. +2. Перед запросом показать имя выбранного сотрудника и подтверждение полного + необратимого удаления аккаунта. Сообщить, что отчёты и история импортов останутся. +3. Вызвать DELETE; на время запроса блокировать повторное действие. После 204 + закрыть диалог и обновить список/счётчик; если последняя строка страницы удалена, + перейти на предыдущую существующую страницу. +4. Обработать стандартный error envelope, включая 400/403/404/409. При 404 обновить + список. Не пытаться читать JSON из успешного ответа 204. +5. Обновить клиент из OpenAPI и проверить удаление обычного, неактивного и другого + административного аккаунта, запрет удаления себя и потерю прав во время запроса. + +## Проверка backend + +```bash +PYTHONPATH=src uv run --no-sync pytest tests/apps/user +``` + +Восемь тестов `test_management_concurrency.py` пропускаются SQLite и требуют +PostgreSQL. Для них использовать отдельную пустую тестовую БД и явно задавать все +`TEST_POSTGRES_HOST`, `TEST_POSTGRES_PORT`, `TEST_POSTGRES_USER`, +`TEST_POSTGRES_PASSWORD`, `TEST_POSTGRES_DB`, чтобы исключить fallback к рабочей БД: + +```bash +scripts/run-tests-prod.sh ../tests/apps/user +``` + +Регрессии обмена и подготовленных выгрузок находятся в +`tests/apps/exchange/test_api.py`, `tests/apps/external_data/test_source_record_export.py` +и `tests/apps/external_data/test_export_tasks.py`. Изменений upload/export API эта +доработка не вносит. diff --git a/src/apps/user/admin.py b/src/apps/user/admin.py index 6c389b3..d980625 100644 --- a/src/apps/user/admin.py +++ b/src/apps/user/admin.py @@ -1,13 +1,21 @@ """Admin configuration for user app.""" from contextlib import suppress +from functools import wraps +from apps.core.exceptions import BaseAPIException from apps.core.models import BackgroundJob +from apps.user.management import lock_managed_users, validate_access_change from apps.user.models import Profile, User -from django.contrib import admin +from apps.user.services import UserService +from django.contrib import admin, messages from django.contrib.admin.sites import NotRegistered +from django.contrib.admin.utils import unquote from django.contrib.auth.admin import UserAdmin as BaseUserAdmin from django.contrib.auth.models import Group +from django.db import transaction +from django.http import HttpResponseRedirect +from django.urls import reverse from django.utils.html import format_html from django.utils.translation import gettext_lazy as _ @@ -47,6 +55,20 @@ def _unregister(model) -> None: _unregister(User) +def _management_errors(view): + """Present guarded account-change failures through Django admin messages.""" + + @wraps(view) + def wrapped(self, request, *args, **kwargs): + try: + return view(self, request, *args, **kwargs) + except BaseAPIException as exc: + self.message_user(request, exc.message, level=messages.ERROR) + return HttpResponseRedirect(reverse("admin:user_user_changelist")) + + return wrapped + + class ProfileInline(admin.StackedInline): """Inline для профиля пользователя.""" @@ -121,6 +143,50 @@ class UserAdmin(BaseUserAdmin): readonly_fields = ["created_at", "updated_at", "last_login", "date_joined"] actions = ["verify_users", "unverify_users", "activate_users", "deactivate_users"] + @_management_errors + def changeform_view(self, request, object_id=None, form_url="", extra_context=None): + return super().changeform_view(request, object_id, form_url, extra_context) + + @_management_errors + def delete_view(self, request, object_id, extra_context=None): + return super().delete_view(request, object_id, extra_context) + + @_management_errors + def user_change_password(self, request, id, form_url=""): + user = self.get_object(request, unquote(id)) + if request.method == "POST" and user is not None: + with lock_managed_users(actor_id=request.user.pk, user_ids=[user.pk]): + return super().user_change_password(request, id, form_url) + return super().user_change_password(request, id, form_url) + + @_management_errors + def changelist_view(self, request, extra_context=None): + # delete_selected writes LogEntry rows before delete_queryset. Roll those + # back too if the complete selection fails an account-management guard. + with transaction.atomic(): + return super().changelist_view(request, extra_context) + + def save_model(self, request, obj, form, change): + with lock_managed_users( + actor_id=request.user.pk, user_ids=[obj.pk] if change else [] + ) as users: + if change: + validate_access_change( + users[obj.pk], + actor_id=request.user.pk, + is_active=obj.is_active, + is_staff=obj.is_staff, + ) + super().save_model(request, obj, form, change) + + def delete_model(self, request, obj): + UserService.delete_user(obj.pk, actor_id=request.user.pk) + + def delete_queryset(self, request, queryset): + UserService.delete_users( + queryset.values_list("pk", flat=True), actor_id=request.user.pk + ) + def is_verified_badge(self, obj): if obj.is_verified: return format_html( @@ -157,12 +223,20 @@ class UserAdmin(BaseUserAdmin): @admin.action(description="Активировать пользователей") def activate_users(self, request, queryset): - updated = queryset.update(is_active=True) + updated = UserService.set_users_active( + queryset.values_list("pk", flat=True), + actor_id=request.user.pk, + is_active=True, + ) self.message_user(request, f"Активировано {updated} пользователей") @admin.action(description="Деактивировать пользователей") def deactivate_users(self, request, queryset): - updated = queryset.update(is_active=False) + updated = UserService.set_users_active( + queryset.values_list("pk", flat=True), + actor_id=request.user.pk, + is_active=False, + ) self.message_user(request, f"Деактивировано {updated} пользователей") diff --git a/src/apps/user/management.py b/src/apps/user/management.py new file mode 100644 index 0000000..53850ce --- /dev/null +++ b/src/apps/user/management.py @@ -0,0 +1,133 @@ +"""Transactional guards shared by account management API and Django admin.""" + +import logging +from collections.abc import Iterable, Iterator +from contextlib import contextmanager + +from apps.core.exceptions import ( + AuthenticationError, + BadRequestError, + ConflictError, + NotFoundError, + PermissionDeniedError, +) +from apps.core.models import BackgroundJob +from django.core.files.storage import Storage +from django.db import transaction +from django.db.models import Q +from django.db.models.deletion import ProtectedError +from rest_framework_simplejwt.token_blacklist.models import OutstandingToken + +from .models import Profile, User + +logger = logging.getLogger(__name__) + + +@contextmanager +def lock_active_user(user_id: int) -> Iterator[User]: + """Keep an in-flight self-service save from recreating a deleted account.""" + with transaction.atomic(): + try: + user = User.objects.select_for_update().get(pk=user_id) + except User.DoesNotExist as exc: + raise NotFoundError("Пользователь не найден.") from exc + if not user.is_active: + raise AuthenticationError("Учётная запись недоступна.") + yield user + + +@contextmanager +def lock_managed_users( + *, actor_id: int, user_ids: Iterable[int] +) -> Iterator[dict[int, User]]: + """Serialize changes that can remove an administrator and recheck the actor.""" + target_ids = set(user_ids) + with transaction.atomic(): + users = { + user.pk: user + for user in User.objects.select_for_update() + .filter( + Q(is_active=True, is_staff=True) | Q(pk__in=target_ids | {actor_id}) + ) + .order_by("pk") + } + # A request may have authenticated before another administrator deleted or + # demoted its actor. Never authorize from the stale request.user object. + actor = users.get(actor_id) + if actor is None or not actor.is_active or not actor.is_staff: + raise PermissionDeniedError( + "Доступ разрешён только активному администратору." + ) + if target_ids - users.keys(): + raise NotFoundError("Пользователь не найден.") + yield {user_id: users[user_id] for user_id in target_ids} + + +def ensure_admins_remain(*, removed_ids: Iterable[int]) -> None: + """Require a remaining active staff account while holding management locks.""" + if ( + not User.objects.filter(is_active=True, is_staff=True) + .exclude(pk__in=removed_ids) + .exists() + ): + raise ConflictError( + "Нельзя удалить или отключить последнего активного администратора.", + code="last_active_admin", + ) + + +def validate_access_change( + user: User, *, actor_id: int, is_active: bool, is_staff: bool +) -> None: + """Validate a proposed status/role change under the management locks.""" + if user.pk == actor_id and (not is_active or not is_staff): + raise BadRequestError( + "Нельзя деактивировать себя или снять у себя роль администратора.", + code="self_deactivation_forbidden", + ) + if user.is_active and user.is_staff and not (is_active and is_staff): + ensure_admins_remain(removed_ids=[user.pk]) + + +def _delete_unused_avatar(*, storage: Storage, name: str, user_id: int) -> None: + """Clean up a committed deletion without removing another profile's file.""" + try: + if not Profile.objects.filter(avatar=name).exists(): + storage.delete(name) + except Exception: # noqa: BLE001 + # The account is already deleted. Storage failure must not pretend the + # transaction failed or disclose a storage path in an API response. + logger.error("Avatar cleanup failed after deleting user id=%s", user_id) + + +def delete_accounts(*, actor_id: int, user_ids: Iterable[int]) -> None: + """Delete accounts atomically, preserving business records and their files.""" + with lock_managed_users(actor_id=actor_id, user_ids=user_ids) as users: + if actor_id in users: + raise BadRequestError( + "Нельзя удалить самого себя.", code="self_delete_forbidden" + ) + ensure_admins_remain(removed_ids=users) + for user in users.values(): + profile = Profile.objects.filter(user_id=user.pk).first() + avatar = profile.avatar if profile is not None else None + if avatar: + storage, name, user_id = avatar.storage, avatar.name, user.pk + transaction.on_commit( + lambda storage=storage, + name=name, + user_id=user_id: _delete_unused_avatar( + storage=storage, name=name, user_id=user_id + ) + ) + # OutstandingToken uses SET_NULL, while BackgroundJob stores a plain + # integer. Neither is cleaned up automatically by User.delete(). + OutstandingToken.objects.filter(user_id=user.pk).delete() + BackgroundJob.objects.filter(user_id=user.pk).update(user_id=None) + try: + user.delete() + except ProtectedError as exc: + raise ConflictError( + "Связанные записи препятствуют удалению пользователя.", + code="user_delete_conflict", + ) from exc diff --git a/src/apps/user/services.py b/src/apps/user/services.py index 7474116..1c1dde3 100644 --- a/src/apps/user/services.py +++ b/src/apps/user/services.py @@ -1,3 +1,4 @@ +from collections.abc import Iterable from typing import Any from apps.core.exceptions import NotFoundError @@ -7,6 +8,13 @@ from django.db import transaction from django.db.models import F, Q from rest_framework_simplejwt.tokens import RefreshToken +from .management import ( + delete_accounts, + ensure_admins_remain, + lock_active_user, + lock_managed_users, + validate_access_change, +) from .models import Profile User = get_user_model() @@ -177,19 +185,17 @@ class UserService: Raises: NotFoundError: Если пользователь не найден """ - user = cls.get_user_by_id(user_id) - - for field, value in fields.items(): - setattr(user, field, value) - - user.save() - return user + with lock_active_user(user_id) as user: + for field, value in fields.items(): + setattr(user, field, value) + user.save() + return user @classmethod - @transaction.atomic def create_managed_user( cls, *, + actor_id: int, email: str, username: str, password: str, @@ -200,26 +206,40 @@ class UserService: **extra_fields, ) -> User: """Создаёт пользователя администратором и назначает ему роль.""" - user = User.objects.create_user( - email=email, - username=username, - password=password, - **extra_fields, - ) - cls.assign_role(user, role) - cls._update_or_create_profile( - user=user, - first_name=first_name, - middle_name=middle_name or "", - last_name=last_name, - ) - return cls.get_users_queryset().get(id=user.id) + with lock_managed_users(actor_id=actor_id, user_ids=[]): + user = User.objects.create_user( + email=email, + username=username, + password=password, + **extra_fields, + ) + cls.assign_role(user, role) + cls._update_or_create_profile( + user=user, + first_name=first_name, + middle_name=middle_name or "", + last_name=last_name, + ) + return cls.get_users_queryset().get(id=user.id) @classmethod - @transaction.atomic - def update_managed_user(cls, user_id: int, **fields) -> User: + def update_managed_user(cls, user_id: int, *, actor_id: int, **fields) -> User: """Обновляет учётные данные, профиль, пароль и роль пользователя.""" - user = cls.get_user_by_id(user_id) + with lock_managed_users(actor_id=actor_id, user_ids=[user_id]) as users: + user = users[user_id] + validate_access_change( + user, + actor_id=actor_id, + is_active=fields.get("is_active", user.is_active), + is_staff=(fields["role"] == cls.ROLE_ADMIN) + if "role" in fields + else user.is_staff, + ) + return cls._update_managed_user(user, **fields) + + @classmethod + def _update_managed_user(cls, user: User, **fields) -> User: + """Apply validated fields while the caller holds management locks.""" role = fields.pop("role", None) password = fields.pop("password", None) profile_fields = { @@ -243,34 +263,40 @@ class UserService: return cls.get_users_queryset().get(id=user.id) @classmethod - def deactivate_user(cls, user_id: int) -> User: + def deactivate_user(cls, user_id: int, *, actor_id: int) -> User: """Деактивирует пользователя без удаления записи.""" - user = cls.get_user_by_id(user_id) - user.is_active = False - user.save(update_fields=["is_active"]) - return user + cls.set_users_active([user_id], actor_id=actor_id, is_active=False) + return cls.get_user_by_id(user_id) @classmethod - def activate_user(cls, user_id: int) -> User: + def activate_user(cls, user_id: int, *, actor_id: int) -> User: """Возвращает пользователя в активное состояние.""" - user = cls.get_user_by_id(user_id) - user.is_active = True - user.save(update_fields=["is_active"]) - return user + cls.set_users_active([user_id], actor_id=actor_id, is_active=True) + return cls.get_user_by_id(user_id) @classmethod - def delete_user(cls, user_id: int) -> None: - """ - Удаляет пользователя + def set_users_active( + cls, user_ids: Iterable[int], *, actor_id: int, is_active: bool + ) -> int: + """Change activity atomically, including Django admin bulk actions.""" + with lock_managed_users(actor_id=actor_id, user_ids=user_ids) as users: + for user in users.values(): + validate_access_change( + user, actor_id=actor_id, is_active=is_active, is_staff=user.is_staff + ) + if not is_active: + ensure_admins_remain(removed_ids=users) + return User.objects.filter(pk__in=users).update(is_active=is_active) - Args: - user_id: ID пользователя + @classmethod + def delete_user(cls, user_id: int, *, actor_id: int) -> None: + """Delete an employee without deleting reports, imports or task history.""" + cls.delete_users([user_id], actor_id=actor_id) - Raises: - NotFoundError: Если пользователь не найден - """ - user = cls.get_user_by_id(user_id) - user.delete() + @classmethod + def delete_users(cls, user_ids: Iterable[int], *, actor_id: int) -> None: + """Share the same guarded deletion with Django admin bulk operations.""" + delete_accounts(actor_id=actor_id, user_ids=user_ids) @classmethod def get_tokens_for_user(cls, user: User) -> dict[str, str]: diff --git a/src/apps/user/views.py b/src/apps/user/views.py index 18dbd06..a9c0650 100644 --- a/src/apps/user/views.py +++ b/src/apps/user/views.py @@ -17,8 +17,10 @@ from rest_framework.permissions import AllowAny, IsAdminUser, IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView from rest_framework_simplejwt.exceptions import TokenError +from rest_framework_simplejwt.settings import api_settings as jwt_settings from rest_framework_simplejwt.tokens import RefreshToken +from .management import lock_active_user from .serializers import ( AdminUserCreateSerializer, AdminUserListResponseSerializer, @@ -278,21 +280,20 @@ class PasswordChangeView(APIView): def post(self, request): serializer = PasswordChangeSerializer(data=request.data) if serializer.is_valid(): - user = request.user old_password = serializer.validated_data["old_password"] - - if check_password(old_password, user.password): - new_password = serializer.validated_data["new_password"] - user.set_password(new_password) - user.save() - return Response( - {"message": "Пароль успешно изменен"}, status=status.HTTP_200_OK - ) - else: - return Response( - {"error": "Неверный старый пароль"}, - status=status.HTTP_400_BAD_REQUEST, - ) + with lock_active_user(request.user.id) as user: + if check_password(old_password, user.password): + new_password = serializer.validated_data["new_password"] + user.set_password(new_password) + user.save() + return Response( + {"message": "Пароль успешно изменен"}, status=status.HTTP_200_OK + ) + else: + return Response( + {"error": "Неверный старый пароль"}, + status=status.HTTP_400_BAD_REQUEST, + ) return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) @@ -392,7 +393,9 @@ class AdminUsersManagementView(APIView): def post(self, request): serializer = AdminUserCreateSerializer(data=request.data) serializer.is_valid(raise_exception=True) - user = UserService.create_managed_user(**serializer.validated_data) + user = UserService.create_managed_user( + actor_id=request.user.id, **serializer.validated_data + ) return Response( ManagedUserSerializer(user).data, status=status.HTTP_201_CREATED, @@ -400,7 +403,7 @@ class AdminUsersManagementView(APIView): class AdminUserDetailView(APIView): - """Просмотр и частичное обновление пользователя администратором.""" + """Просмотр, изменение и удаление пользователя администратором.""" permission_classes = [IsAdminUser] @@ -443,10 +446,32 @@ class AdminUserDetailView(APIView): updated_user = UserService.update_managed_user( user_id=user.id, + actor_id=request.user.id, **serializer.validated_data, ) return Response(ManagedUserSerializer(updated_user).data) + @swagger_auto_schema( + tags=[USER_TAG], + operation_summary="Удалить сотрудника (admin)", + operation_description=( + "Полностью удаляет аккаунт и профиль. Отчёты, импорты и задачи " + "сохраняются без ссылки на сотрудника. Нельзя удалить себя или " + "последнего активного администратора." + ), + responses={ + 204: "Сотрудник удалён", + 400: "Нельзя удалить самого себя", + 401: "Требуется аутентификация", + 403: "Требуется активный администратор", + 404: "Пользователь не найден", + 409: "Удаление нарушает ограничения", + }, + ) + def delete(self, request, user_id: int): + UserService.delete_user(user_id, actor_id=request.user.id) + return Response(status=status.HTTP_204_NO_CONTENT) + class AdminUserDeactivateView(APIView): """Деактивация пользователя администратором.""" @@ -464,7 +489,7 @@ class AdminUserDeactivateView(APIView): {"detail": "Нельзя деактивировать самого себя."}, status=status.HTTP_400_BAD_REQUEST, ) - user = UserService.deactivate_user(user_id) + user = UserService.deactivate_user(user_id, actor_id=request.user.id) return Response(ManagedUserSerializer(user).data) @@ -479,7 +504,7 @@ class AdminUserActivateView(APIView): responses={200: ManagedUserSerializer}, ) def post(self, request, user_id: int): - user = UserService.activate_user(user_id) + user = UserService.activate_user(user_id, actor_id=request.user.id) return Response(ManagedUserSerializer(user).data) @@ -527,10 +552,15 @@ class TokenRefreshView(APIView): try: refresh = RefreshToken(refresh_token) + user_id = refresh.get(jwt_settings.USER_ID_CLAIM) + if not User.objects.filter( + **{jwt_settings.USER_ID_FIELD: user_id}, is_active=True + ).exists(): + raise TokenError("User is unavailable") return Response( {"access": str(refresh.access_token), "refresh": str(refresh)} ) - except TokenError: + except (TokenError, TypeError, ValueError, OverflowError): return Response( {"error": "Неверный refresh token"}, status=status.HTTP_401_UNAUTHORIZED ) diff --git a/tests/apps/user/test_deletion.py b/tests/apps/user/test_deletion.py new file mode 100644 index 0000000..116f5e1 --- /dev/null +++ b/tests/apps/user/test_deletion.py @@ -0,0 +1,258 @@ +"""Account deletion keeps business history and revokes authentication.""" + +from datetime import date +from tempfile import TemporaryDirectory +from unittest.mock import patch + +from apps.core.models import BackgroundJob, ReportUpload +from apps.exchange.models import ExchangePackageImport +from apps.registers.models import Register, RegisterUpload +from apps.user.models import Profile, User +from apps.user.services import UserService +from apps.user.views import AdminUserDetailView +from django.contrib.auth.models import Group +from django.core.files.base import ContentFile +from django.db import transaction +from django.db.models.deletion import ProtectedError +from django.test import override_settings +from django.urls import path, reverse +from drf_yasg import openapi +from drf_yasg.generators import OpenAPISchemaGenerator +from rest_framework.test import APITestCase +from rest_framework_simplejwt.token_blacklist.models import ( + BlacklistedToken, + OutstandingToken, +) +from rest_framework_simplejwt.tokens import RefreshToken + +from tests.apps.user.factories import UserFactory + + +class UserDeletionTest(APITestCase): + """Verify deletion through the public administrative API.""" + + def setUp(self): + self.admin = UserFactory.create_superuser() + self.user = UserFactory.create_user() + self.user_id = self.user.pk + self.url = reverse("api_v1:user:admin-user-detail", args=[self.user_id]) + self.client.force_authenticate(self.admin) + + def test_delete_removes_account_and_credentials_but_preserves_history(self): + group = Group.objects.create(name="deletion-test") + self.user.groups.add(group) + refresh = RefreshToken.for_user(self.user) + refresh.blacklist() + report = ReportUpload.objects.create( + form="f1", + original_file="reports/keep.xlsx", + file_name="keep.xlsx", + content_type="application/octet-stream", + file_size=1, + file_hash="hash", + uploaded_by=self.user, + ) + upload = RegisterUpload.objects.create( + registry=Register.objects.create(name="History"), + actual_date=date.today(), + file_name="keep.xlsx", + file_hash="hash", + uploaded_by=self.user, + ) + imported = ExchangePackageImport.objects.create( + package_id="history-package", + package_name="keep.zip", + package_hash="hash", + imported_by=self.user, + ) + job = BackgroundJob.objects.create( + task_id="history-task", + task_name="test", + user_id=self.user_id, + ) + + response = self.client.delete(self.url) + + self.assertEqual(response.status_code, 204) + self.assertEqual(response.content, b"") + self.assertFalse(User.objects.filter(pk=self.user_id).exists()) + self.assertFalse(Profile.objects.filter(user_id=self.user_id).exists()) + self.assertFalse(OutstandingToken.objects.filter(jti=refresh["jti"]).exists()) + self.assertEqual(BlacklistedToken.objects.count(), 0) + self.assertTrue(Group.objects.filter(pk=group.pk).exists()) + self.assertEqual( + User.groups.through.objects.filter(user_id=self.user_id).count(), 0 + ) + for record, field in [ + (report, "uploaded_by_id"), + (upload, "uploaded_by_id"), + (imported, "imported_by_id"), + (job, "user_id"), + ]: + record.refresh_from_db() + self.assertIsNone(getattr(record, field)) + self.assertEqual(report.original_file.name, "reports/keep.xlsx") + self.assertEqual(job.status, "pending") + self.assertEqual(self.client.get(self.url).status_code, 404) + self.assertEqual(self.client.delete(self.url).status_code, 404) + response = self.client.get( + reverse("api_v1:user:admin_users"), {"search": self.user.username} + ) + self.assertEqual(response.data["count"], 0) + replacement = UserFactory.create_user( + username=self.user.username, email=self.user.email + ) + self.assertNotEqual(replacement.pk, self.user_id) + + def test_deletion_revokes_access_and_refresh_even_without_outstanding_row(self): + tokens = UserService.get_tokens_for_user(self.user) + self.assertEqual(self.client.delete(self.url).status_code, 204) + self.client.force_authenticate(user=None) + self.client.credentials(HTTP_AUTHORIZATION=f"Bearer {tokens['access']}") + self.assertEqual( + self.client.get(reverse("api_v1:user:current_user")).status_code, 401 + ) + self.client.credentials() + response = self.client.post( + reverse("api_v1:user:token_refresh"), {"refresh": tokens["refresh"]} + ) + self.assertEqual(response.status_code, 401) + + def test_inactive_user_cannot_refresh(self): + tokens = UserService.get_tokens_for_user(self.user) + User.objects.filter(pk=self.user_id).update(is_active=False) + self.client.force_authenticate(user=None) + response = self.client.post( + reverse("api_v1:user:token_refresh"), {"refresh": tokens["refresh"]} + ) + self.assertEqual(response.status_code, 401) + + def test_invalid_user_claim_cannot_refresh(self): + refresh = RefreshToken.for_user(self.user) + refresh["user_id"] = "invalid-user-id" + self.client.force_authenticate(user=None) + response = self.client.post( + reverse("api_v1:user:token_refresh"), {"refresh": str(refresh)} + ) + self.assertEqual(response.status_code, 401) + + def test_self_delete_is_rejected_and_account_remains(self): + url = reverse("api_v1:user:admin-user-detail", args=[self.admin.pk]) + response = self.client.delete(url) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.data["errors"][0]["code"], "self_delete_forbidden") + self.assertTrue( + User.objects.filter( + pk=self.admin.pk, is_active=True, is_staff=True + ).exists() + ) + + def test_another_admin_and_inactive_user_can_be_deleted(self): + for attributes in ({"is_staff": True}, {"is_active": False}): + user = UserFactory.create_user(**attributes) + url = reverse("api_v1:user:admin-user-detail", args=[user.pk]) + self.assertEqual(self.client.delete(url).status_code, 204) + + def test_openapi_describes_delete_without_a_success_body(self): + generator = OpenAPISchemaGenerator( + info=openapi.Info(title="Account management", default_version="v1"), + patterns=[ + path( + "api/v1/users/admin/users//", + AdminUserDetailView.as_view(), + ) + ], + ) + schema = generator.get_schema(request=None, public=True) + operation = next( + item["delete"] for item in schema.paths.values() if "delete" in item + ) + self.assertEqual( + set(operation.responses), {"204", "400", "401", "403", "404", "409"} + ) + self.assertNotIn("schema", operation.responses["204"]) + + def test_permission_is_rechecked_for_stale_authenticated_actor(self): + User.objects.filter(pk=self.admin.pk).update(is_staff=False) + self.assertEqual(self.client.delete(self.url).status_code, 403) + self.assertTrue(User.objects.filter(pk=self.user_id).exists()) + + def test_non_admin_and_anonymous_cannot_delete(self): + self.client.force_authenticate(self.user) + self.assertEqual(self.client.delete(self.url).status_code, 403) + self.client.force_authenticate(user=None) + self.assertEqual(self.client.delete(self.url).status_code, 401) + + def test_transaction_rollback_restores_credentials_and_author_links(self): + refresh = RefreshToken.for_user(self.user) + job = BackgroundJob.objects.create( + task_id="rollback-job", task_name="test", user_id=self.user_id + ) + with patch.object( + User, "delete", side_effect=RuntimeError("test rollback") + ), self.assertRaises(RuntimeError): + UserService.delete_user(self.user_id, actor_id=self.admin.pk) + self.assertTrue(User.objects.filter(pk=self.user_id).exists()) + self.assertTrue(Profile.objects.filter(user_id=self.user_id).exists()) + self.assertTrue(OutstandingToken.objects.filter(jti=refresh["jti"]).exists()) + job.refresh_from_db() + self.assertEqual(job.user_id, self.user_id) + + def test_avatar_cleanup_waits_for_commit_and_preserves_shared_files(self): + with TemporaryDirectory(prefix="statecorp-avatar-") as media, override_settings( + MEDIA_ROOT=media + ): + avatar = self.user.profile.avatar + avatar.save("test.png", ContentFile(b"avatar")) + storage, name = avatar.storage, avatar.name + with self.captureOnCommitCallbacks(execute=True): + UserService.delete_user(self.user_id, actor_id=self.admin.pk) + self.assertTrue(storage.exists(name)) + self.assertFalse(storage.exists(name)) + + owner = UserFactory.create_user() + owner.profile.avatar.save("shared.png", ContentFile(b"shared")) + name = owner.profile.avatar.name + self.admin.profile.avatar = name + self.admin.profile.save() + with self.captureOnCommitCallbacks(execute=True): + UserService.delete_user(owner.pk, actor_id=self.admin.pk) + self.assertTrue(storage.exists(name)) + + def test_avatar_survives_rollback(self): + with TemporaryDirectory( + prefix="statecorp-avatar-rollback-" + ) as media, override_settings(MEDIA_ROOT=media): + self.user.profile.avatar.save("test.png", ContentFile(b"avatar")) + storage, name = ( + self.user.profile.avatar.storage, + self.user.profile.avatar.name, + ) + with self.captureOnCommitCallbacks(execute=True), self.assertRaises( + RuntimeError + ), transaction.atomic(): + UserService.delete_user(self.user_id, actor_id=self.admin.pk) + raise RuntimeError("rollback outer transaction") + self.assertTrue(storage.exists(name)) + self.assertTrue(User.objects.filter(pk=self.user_id).exists()) + + def test_protected_relation_returns_conflict_and_rolls_back(self): + refresh = RefreshToken.for_user(self.user) + with patch.object(User, "delete", side_effect=ProtectedError("protected", [])): + response = self.client.delete(self.url) + self.assertEqual(response.status_code, 409) + self.assertEqual(response.data["errors"][0]["code"], "user_delete_conflict") + self.assertTrue(User.objects.filter(pk=self.user_id).exists()) + self.assertTrue(OutstandingToken.objects.filter(jti=refresh["jti"]).exists()) + + def test_avatar_storage_failure_does_not_undo_account_deletion(self): + self.user.profile.avatar = "avatars/unavailable.png" + self.user.profile.save() + with patch("apps.user.management.logger.error") as log_error, patch.object( + self.user.profile.avatar.storage, + "delete", + side_effect=OSError("storage unavailable"), + ), self.captureOnCommitCallbacks(execute=True): + self.assertEqual(self.client.delete(self.url).status_code, 204) + self.assertFalse(User.objects.filter(pk=self.user_id).exists()) + log_error.assert_called_once() diff --git a/tests/apps/user/test_management_admin.py b/tests/apps/user/test_management_admin.py new file mode 100644 index 0000000..cdd3db8 --- /dev/null +++ b/tests/apps/user/test_management_admin.py @@ -0,0 +1,122 @@ +"""Django admin must share the account deletion and access guards.""" + +from unittest.mock import patch + +from apps.core.exceptions import BadRequestError, ConflictError, PermissionDeniedError +from apps.user.admin import UserAdmin +from apps.user.management import ensure_admins_remain, lock_managed_users +from apps.user.models import User +from django.contrib.admin import AdminSite +from django.contrib.admin.models import LogEntry +from django.test import RequestFactory, TestCase +from django.urls import reverse +from rest_framework_simplejwt.token_blacklist.models import OutstandingToken +from rest_framework_simplejwt.tokens import RefreshToken + +from tests.apps.user.factories import UserFactory + + +class ManagedUserAdminTest(TestCase): + def setUp(self): + self.actor = UserFactory.create_superuser() + self.user = UserFactory.create_user() + self.model_admin = UserAdmin(User, AdminSite()) + self.request = RequestFactory().post("/admin/user/user/") + self.request.user = self.actor + self.initial_user_count = User.objects.count() + + def test_admin_single_and_bulk_delete_clean_up_tokens(self): + second = UserFactory.create_user() + first_id, second_id = self.user.pk, second.pk + RefreshToken.for_user(self.user) + RefreshToken.for_user(second) + self.model_admin.delete_model(self.request, self.user) + self.model_admin.delete_queryset( + self.request, User.objects.filter(pk=second_id) + ) + self.assertFalse(User.objects.filter(pk__in=[first_id, second_id]).exists()) + self.assertFalse( + OutstandingToken.objects.filter(user_id__in=[first_id, second_id]).exists() + ) + + def test_bulk_delete_is_atomic_when_selection_includes_actor(self): + with self.assertRaises(BadRequestError): + self.model_admin.delete_queryset(self.request, User.objects.all()) + self.assertEqual(User.objects.count(), self.initial_user_count) + + def test_admin_bulk_deactivate_and_change_form_cannot_disable_self(self): + with self.assertRaises(BadRequestError): + self.model_admin.deactivate_users(self.request, User.objects.all()) + self.assertTrue(User.objects.get(pk=self.user.pk).is_active) + edited_actor = User.objects.get(pk=self.actor.pk) + edited_actor.is_staff = False + with self.assertRaises(BadRequestError): + self.model_admin.save_model(self.request, edited_actor, None, True) + self.assertTrue(User.objects.get(pk=self.actor.pk).is_staff) + + def test_admin_rechecks_actor_before_saving_or_bulk_deactivating(self): + User.objects.filter(pk=self.actor.pk).update(is_active=False) + with self.assertRaises(PermissionDeniedError): + self.model_admin.save_model(self.request, self.user, None, True) + with self.assertRaises(PermissionDeniedError): + self.model_admin.deactivate_users( + self.request, User.objects.filter(pk=self.user.pk) + ) + + def test_bulk_deactivation_preserves_other_administrator(self): + with patch.object(self.model_admin, "message_user"): + self.model_admin.deactivate_users( + self.request, User.objects.filter(pk=self.user.pk) + ) + self.assertFalse(User.objects.get(pk=self.user.pk).is_active) + self.assertTrue(User.objects.get(pk=self.actor.pk).is_active) + + def test_last_active_administrator_guard_excludes_inactive_staff(self): + User.objects.exclude(pk=self.actor.pk).update(is_active=False) + inactive = UserFactory.create_user(is_staff=True, is_active=False) + with lock_managed_users( + actor_id=self.actor.pk, user_ids=[inactive.pk] + ), self.assertRaises(ConflictError) as error: + ensure_admins_remain(removed_ids=[self.actor.pk]) + self.assertEqual(error.exception.code, "last_active_admin") + + def test_admin_selected_delete_failure_does_not_publish_false_audit_entries(self): + self.client.force_login(self.actor) + response = self.client.post( + reverse("admin:user_user_changelist"), + { + "action": "delete_selected", + "post": "yes", + "_selected_action": [str(self.actor.pk), str(self.user.pk)], + }, + ) + self.assertEqual(response.status_code, 302) + self.assertEqual(User.objects.count(), self.initial_user_count) + self.assertEqual(LogEntry.objects.count(), 0) + + def test_admin_single_delete_error_is_presented_without_server_error(self): + self.client.force_login(self.actor) + response = self.client.post( + reverse("admin:user_user_delete", args=[self.actor.pk]), {"post": "yes"} + ) + self.assertEqual(response.status_code, 302) + self.assertTrue(User.objects.filter(pk=self.actor.pk).exists()) + self.assertEqual(LogEntry.objects.count(), 0) + + def test_separate_admin_password_route_rechecks_actor(self): + old_hash = self.user.password + User.objects.filter(pk=self.actor.pk).update(is_staff=False) + request = RequestFactory().post( + "/admin/user/user/password/", + { + "password1": "changed-test-password", + "password2": "changed-test-password", + }, + ) + request.user = self.actor + with patch.object(self.model_admin, "message_user") as message: + response = self.model_admin.user_change_password(request, str(self.user.pk)) + self.assertEqual(response.status_code, 302) + message.assert_called_once() + self.user.refresh_from_db() + self.assertEqual(self.user.password, old_hash) diff --git a/tests/apps/user/test_management_concurrency.py b/tests/apps/user/test_management_concurrency.py new file mode 100644 index 0000000..336e8c8 --- /dev/null +++ b/tests/apps/user/test_management_concurrency.py @@ -0,0 +1,159 @@ +"""Use real row locks to exercise concurrent administrative mutations.""" + +from concurrent.futures import ThreadPoolExecutor +from threading import Event + +from apps.core.exceptions import PermissionDeniedError +from apps.user.admin import UserAdmin +from apps.user.models import User +from apps.user.services import UserService +from django.contrib.admin import AdminSite +from django.db import close_old_connections, connection, transaction +from django.test import RequestFactory, TransactionTestCase, skipUnlessDBFeature +from django.urls import reverse +from rest_framework.test import APIClient + +from tests.apps.user.factories import UserFactory + + +@skipUnlessDBFeature("has_select_for_update") +class UserManagementConcurrencyTest(TransactionTestCase): + """A request authenticated before deletion must lose mutation privileges.""" + + def _assert_deleted_actor_cannot_mutate(self, operation: str) -> None: + actor = UserFactory.create_superuser() + removed_actor = UserFactory.create_superuser() + actor_id, removed_id = actor.pk, removed_actor.pk + first_deleted = Event() + second_lock_query = Event() + release_first = Event() + + def delete_other_actor(): + close_old_connections() + try: + with transaction.atomic(): + UserService.delete_user(removed_id, actor_id=actor_id) + first_deleted.set() + if not release_first.wait(10): + raise AssertionError("Concurrent test did not release deletion") + finally: + connection.close() + + def observe_lock(execute, sql, params, many, context): + if "FOR UPDATE" in sql: + second_lock_query.set() + return execute(sql, params, many, context) + + def stale_request(): + close_old_connections() + try: + with connection.execute_wrapper(observe_lock): + if operation == "admin-deactivate": + request = RequestFactory().post("/admin/user/user/") + request.user = removed_actor + model_admin = UserAdmin(User, AdminSite()) + try: + model_admin.deactivate_users( + request, User.objects.filter(pk=actor_id) + ) + except PermissionDeniedError: + return 403 + return 200 + client = APIClient() + client.force_authenticate(removed_actor) + if operation == "self-password": + return client.post( + reverse("api_v1:user:password_change"), + { + "old_password": "testpass123", + "new_password": "changed-test-password", + "new_password_confirm": "changed-test-password", + }, + format="json", + ).status_code + if operation == "self-update": + return client.patch( + reverse("api_v1:user:user_update"), + { + "username": "replacement-admin", + }, + format="json", + ).status_code + if operation == "create-admin": + return client.post( + reverse("api_v1:user:admin_users"), + { + "username": "replacement-admin", + "email": "replacement@example.test", + "password": "integration-test-password", + "role": "admin", + "first_name": "Test", + "last_name": "Admin", + }, + format="json", + ).status_code + url = reverse("api_v1:user:admin-user-detail", args=[actor_id]) + if operation == "delete": + return client.delete(url).status_code + if operation == "demote": + return client.patch( + url, {"role": "user"}, format="json" + ).status_code + if operation == "patch-inactive": + return client.patch( + url, {"is_active": False}, format="json" + ).status_code + url = reverse("api_v1:user:admin-user-deactivate", args=[actor_id]) + return client.post(url).status_code + finally: + connection.close() + + with ThreadPoolExecutor(max_workers=2) as pool: + first = pool.submit(delete_other_actor) + try: + self.assertTrue( + first_deleted.wait(10), + "First deletion did not reach its transaction barrier", + ) + second = pool.submit(stale_request) + self.assertTrue( + second_lock_query.wait(10), + "Second request did not try to acquire row locks", + ) + self.assertFalse(second.done()) + finally: + release_first.set() + first.result(timeout=10) + expected_status = ( + 404 if operation in {"self-password", "self-update"} else 403 + ) + self.assertEqual(second.result(timeout=10), expected_status) + self.assertFalse(User.objects.filter(pk=removed_id).exists()) + self.assertTrue( + User.objects.filter(pk=actor_id, is_active=True, is_staff=True).exists() + ) + self.assertFalse(User.objects.filter(username="replacement-admin").exists()) + + def test_two_admins_cannot_delete_each_other(self): + self._assert_deleted_actor_cannot_mutate("delete") + + def test_deleted_admin_cannot_deactivate_survivor(self): + self._assert_deleted_actor_cannot_mutate("deactivate") + + def test_deleted_admin_cannot_demote_survivor(self): + self._assert_deleted_actor_cannot_mutate("demote") + + def test_deleted_admin_cannot_patch_survivor_inactive(self): + self._assert_deleted_actor_cannot_mutate("patch-inactive") + + def test_deleted_admin_cannot_use_django_admin_bulk_deactivation(self): + self._assert_deleted_actor_cannot_mutate("admin-deactivate") + + def test_deleted_admin_cannot_create_a_replacement_admin(self): + self._assert_deleted_actor_cannot_mutate("create-admin") + + def test_in_flight_self_password_change_cannot_recreate_deleted_user(self): + self._assert_deleted_actor_cannot_mutate("self-password") + + def test_in_flight_self_update_cannot_recreate_deleted_user(self): + self._assert_deleted_actor_cannot_mutate("self-update") diff --git a/tests/apps/user/test_services.py b/tests/apps/user/test_services.py index a836541..f740a3a 100644 --- a/tests/apps/user/test_services.py +++ b/tests/apps/user/test_services.py @@ -109,7 +109,8 @@ class UserServiceTest(TestCase): def test_delete_user_success(self): """Test successful user deletion""" user_id = self.user.id - UserService.delete_user(user_id) + admin = UserFactory.create_superuser() + UserService.delete_user(user_id, actor_id=admin.pk) # Verify user is deleted with self.assertRaises(NotFoundError): @@ -118,8 +119,9 @@ class UserServiceTest(TestCase): def test_delete_user_not_found(self): """Test deleting non-existing user raises NotFoundError""" nonexistent_id = fake.pyint(min_value=900000, max_value=999999) + admin = UserFactory.create_superuser() with self.assertRaises(NotFoundError): - UserService.delete_user(nonexistent_id) + UserService.delete_user(nonexistent_id, actor_id=admin.pk) def test_get_tokens_for_user(self): """Test JWT token generation"""