diff --git a/SORT/settings.py b/SORT/settings.py index fcc7f34b..87492b79 100644 --- a/SORT/settings.py +++ b/SORT/settings.py @@ -214,7 +214,12 @@ def cast_to_boolean(obj: Any) -> bool: DEFAULT_FROM_EMAIL = os.getenv("DJANGO_DEFAULT_FROM_EMAIL", "noreply@noreply.com") AUTHENTICATION_BACKENDS = ( - "django.contrib.auth.backends.ModelBackend", + # Allows authenticate() to succeed for inactive (suspended) users so that + # AuthenticationForm.confirm_login_allowed() can reject them with a specific + # error, reusing the single password hash check already performed here rather + # than requiring a second check_password() call that leaks suspension status + # via a timing side-channel. + "django.contrib.auth.backends.AllowAllUsersModelBackend", "allauth.account.auth_backends.AuthenticationBackend", ) diff --git a/home/constants.py b/home/constants.py index 48297acf..861831ca 100644 --- a/home/constants.py +++ b/home/constants.py @@ -34,5 +34,7 @@ class OrganisationMembershipRole: (PERMISSION_EDIT, "View and Edit"), ] -# Email domain used to mark accounts anonymised for GDPR erasure (see UserService.anonymise) -DELETED_EMAIL_DOMAIN = "deleted.invalid" +# Email domain used to anonymise a user's address on GDPR erasure (see +# UserService.anonymise). Used to distinguish erased accounts from merely +# suspended ones, since both share is_active=False. +DELETED_ACCOUNT_EMAIL_DOMAIN = "deleted.invalid" diff --git a/home/models.py b/home/models.py index 7e09f233..81959d1b 100644 --- a/home/models.py +++ b/home/models.py @@ -8,7 +8,7 @@ from django.dispatch import receiver from django.urls import reverse -from .constants import DELETED_EMAIL_DOMAIN, ROLE_ADMIN, ROLE_PROJECT_MANAGER, ROLES +from .constants import DELETED_ACCOUNT_EMAIL_DOMAIN, ROLE_ADMIN, ROLE_PROJECT_MANAGER, ROLES class UserManager(BaseUserManager): @@ -59,11 +59,10 @@ def __str__(self): @property def is_deleted(self) -> bool: - """ - Whether this account has been anonymised for GDPR erasure (see UserService.anonymise), - as opposed to just having a blank name. - """ - return self.email.endswith(f"@{DELETED_EMAIL_DOMAIN}") + """True once this account has been anonymised via GDPR erasure (see + UserService.anonymise), as opposed to merely suspended — both set + is_active=False.""" + return self.email.endswith(f"@{DELETED_ACCOUNT_EMAIL_DOMAIN}") @property def active_projects(self) -> int: diff --git a/home/services/user.py b/home/services/user.py index 55a5e6ff..7ee8aa1b 100644 --- a/home/services/user.py +++ b/home/services/user.py @@ -1,6 +1,6 @@ import uuid -from ..constants import DELETED_EMAIL_DOMAIN +from ..constants import DELETED_ACCOUNT_EMAIL_DOMAIN from ..models import OrganisationMembership, User @@ -8,7 +8,7 @@ class UserService: def anonymise(self, user: User) -> None: user.first_name = "Deleted" user.last_name = "User" - user.email = f"deleted-{uuid.uuid4().hex}@{DELETED_EMAIL_DOMAIN}" + user.email = f"deleted-{uuid.uuid4().hex}@{DELETED_ACCOUNT_EMAIL_DOMAIN}" user.is_active = False user.set_unusable_password() user.save() diff --git a/home/templates/console/suspend_user_confirm.html b/home/templates/console/suspend_user_confirm.html new file mode 100644 index 00000000..17408275 --- /dev/null +++ b/home/templates/console/suspend_user_confirm.html @@ -0,0 +1,53 @@ +{% extends "base_console.html" %} +{% block title %}| Admin — Suspend account{% endblock %} + +{% block content %} +
+ + + +
+
+
+
+

Suspend account

+
+
+

Are you sure you want to suspend {{ viewed_user }}?

+

+ While suspended, this user cannot log in or submit data. Their personal data + and survey responses are retained, not deleted, and you can + lift the suspension at any time. +

+
+ + +
+
+
+ Cancel +
+ {% csrf_token %} + +
+
+ +
+
+
+ +
+{% endblock %} diff --git a/home/templates/console/user_detail.html b/home/templates/console/user_detail.html index f2d8237a..c2f98b64 100644 --- a/home/templates/console/user_detail.html +++ b/home/templates/console/user_detail.html @@ -23,8 +23,19 @@

{{ viewed_user }}

{% if viewed_user.is_superuser %} Superuser {% endif %} - {% if not viewed_user.is_active %} - Inactive + {% if viewed_user.is_deleted %} + Deleted + {% elif not viewed_user.is_active %} + Suspended + {% endif %} + {% if viewed_user.is_deleted %} + {% elif not viewed_user.is_active %} +
+ {% csrf_token %} + +
+ {% elif viewed_user != request.user and not viewed_user.is_superuser %} + Suspend account {% endif %} {% if viewed_user.is_active and not viewed_user.is_staff and not viewed_user.is_superuser and viewed_user != request.user %} Delete user diff --git a/home/templates/console/users.html b/home/templates/console/users.html index 96162002..519d3c46 100644 --- a/home/templates/console/users.html +++ b/home/templates/console/users.html @@ -9,6 +9,7 @@

Users

Status: Active + Suspended Deleted All
@@ -21,6 +22,7 @@

Users

Name Email + Status Organisations Joined @@ -38,17 +40,23 @@

Users

(no name set) {% endif %} - {% if not user.is_active %} - Deleted - {% endif %} {{ user.email }} + + {% if user.is_active %} + Active + {% elif user.is_deleted %} + Deleted + {% else %} + Suspended + {% endif %} + {{ user.organisationmembership_set.count }} {{ user.date_joined|date:"d M Y" }} {% empty %} - No users + No users {% endfor %} diff --git a/home/tests/test_console_views.py b/home/tests/test_console_views.py index b779e331..12a29873 100644 --- a/home/tests/test_console_views.py +++ b/home/tests/test_console_views.py @@ -2,7 +2,7 @@ import SORT.test.test_case from SORT.test.model_factory import OrganisationFactory, OrganisationMembershipFactory, ProjectFactory, SurveyFactory, \ - UserFactory + SuperUserFactory, UserFactory from SORT.test.model_factory.user.constants import PASSWORD @@ -87,6 +87,17 @@ def test_console_users_deleted_filter_shows_only_inactive(self): self.assertNotIn(active_user, response.context["users"]) self.assertIn(deleted_user, response.context["users"]) + def test_console_users_suspended_filter_shows_only_suspended(self): + """?status=suspended shows only suspended (inactive, non-deleted) users.""" + active_user = UserFactory() + suspended_user = UserFactory(is_active=False) + deleted_user = UserFactory(is_active=False, first_name="", last_name="", email="deleted-sus@deleted.invalid") + self.login_staff() + response = self.client.get("/console/users/?status=suspended") + self.assertNotIn(active_user, response.context["users"]) + self.assertIn(suspended_user, response.context["users"]) + self.assertNotIn(deleted_user, response.context["users"]) + def test_console_users_all_filter_shows_both(self): """?status=all shows both active and deleted users.""" active_user = UserFactory() @@ -261,6 +272,87 @@ def test_console_remove_member_post_removes_membership(self): from home.models import OrganisationMembership self.assertFalse(OrganisationMembership.objects.filter(pk=membership_pk).exists()) + # -- Suspend / unsuspend user -------------------------------------------- + + def test_console_suspend_user_get_accessible_to_staff(self): + """Staff users can access the suspend confirmation page.""" + user = UserFactory() + self.login_staff() + response = self.client.get(f"/console/users/{user.pk}/suspend/") + self.assertEqual(response.status_code, HTTPStatus.OK) + + def test_console_suspend_user_get_redirects_anonymous(self): + """Anonymous users are redirected away from the suspend page.""" + user = UserFactory() + response = self.client.get(f"/console/users/{user.pk}/suspend/") + self.assertEqual(response.status_code, HTTPStatus.FOUND) + + def test_console_suspend_user_get_forbidden_for_regular_users(self): + """Regular users cannot access the suspend page.""" + user = UserFactory() + self.login() + response = self.client.get(f"/console/users/{user.pk}/suspend/") + self.assertEqual(response.status_code, HTTPStatus.FORBIDDEN) + + def test_console_suspend_user_post_suspends_without_deleting_data(self): + """POSTing to suspend sets is_active=False, leaving data intact.""" + user = UserFactory() + membership = OrganisationMembershipFactory(user=user, organisation=OrganisationFactory()) + self.login_staff() + response = self.client.post(f"/console/users/{user.pk}/suspend/") + self.assertRedirects(response, f"/console/users/{user.pk}/") + user.refresh_from_db() + self.assertFalse(user.is_active) + # No cascade: the user and their memberships still exist. + self.assertTrue(UserFactory._meta.model.objects.filter(pk=user.pk).exists()) + from home.models import OrganisationMembership + self.assertTrue(OrganisationMembership.objects.filter(pk=membership.pk).exists()) + + def test_console_suspend_user_post_unsuspends(self): + """POSTing to unsuspend restores is_active=True.""" + user = UserFactory(is_active=False) + self.login_staff() + response = self.client.post(f"/console/users/{user.pk}/unsuspend/") + self.assertRedirects(response, f"/console/users/{user.pk}/") + user.refresh_from_db() + self.assertTrue(user.is_active) + + def test_console_suspend_self_forbidden(self): + """Staff cannot suspend their own account.""" + self.login_staff() + response = self.client.post(f"/console/users/{self.staff_user.pk}/suspend/") + self.assertEqual(response.status_code, HTTPStatus.FORBIDDEN) + self.staff_user.refresh_from_db() + self.assertTrue(self.staff_user.is_active) + + def test_console_suspend_superuser_forbidden(self): + """Superuser accounts cannot be suspended.""" + target = SuperUserFactory() + self.login_staff() + response = self.client.post(f"/console/users/{target.pk}/suspend/") + self.assertEqual(response.status_code, HTTPStatus.FORBIDDEN) + target.refresh_from_db() + self.assertTrue(target.is_active) + + def test_console_suspend_staff_forbidden(self): + """Staff cannot suspend another staff member's account.""" + target = UserFactory(is_staff=True) + self.login_staff() + response = self.client.post(f"/console/users/{target.pk}/suspend/") + self.assertEqual(response.status_code, HTTPStatus.FORBIDDEN) + target.refresh_from_db() + self.assertTrue(target.is_active) + + def test_console_user_list_shows_suspended_users(self): + """Suspended users remain visible in the console user list.""" + suspended = UserFactory(is_active=False) + self.login_staff() + response = self.client.get("/console/users/") + self.assertContains(response, suspended.email) + self.assertContains(response, "Suspended") + + # -- Delete (anonymise) user --------------------------------------------- + def test_delete_user_get_shows_confirmation(self): """Staff users see the delete confirmation page for a regular user.""" target = UserFactory() diff --git a/home/tests/test_user_views.py b/home/tests/test_user_views.py index fe90fe04..244075d0 100644 --- a/home/tests/test_user_views.py +++ b/home/tests/test_user_views.py @@ -5,6 +5,8 @@ from http import HTTPStatus import SORT.test.test_case +from SORT.test.model_factory import UserFactory +from SORT.test.model_factory.user.constants import PASSWORD class UserViewTestCase(SORT.test.test_case.ViewTestCase): @@ -31,3 +33,24 @@ def test_login_post(self): def test_logout(self): # Expect to be redirected self.post("logout", expected_status_code=HTTPStatus.FOUND, login=True) + + def test_login_suspended_user_shows_suspension_message(self): + """A suspended user with correct credentials sees a clear message.""" + suspended = UserFactory(is_active=False) + response = self.post( + "login", + data=dict(username=suspended.email, password=PASSWORD), + login=False, + ) + self.assertContains(response, "suspended") + # They are not authenticated. + self.assertFalse(response.wsgi_request.user.is_authenticated) + + def test_login_wrong_password_shows_generic_message(self): + """An incorrect password shows the generic error, not the suspension one.""" + response = self.post( + "login", + data=dict(username=self.user.email, password="not-the-password"), + login=False, + ) + self.assertContains(response, "Invalid email or password") diff --git a/home/urls.py b/home/urls.py index 7086b35d..7c05e1f1 100644 --- a/home/urls.py +++ b/home/urls.py @@ -160,6 +160,16 @@ path("console/users/", views.ConsoleUserListView.as_view(), name="admin_users"), path("console/users//", views.ConsoleUserDetailView.as_view(), name="admin_user_detail"), path("console/users//delete/", views.ConsoleDeleteUserView.as_view(), name="admin_delete_user"), + path( + "console/users//suspend/", + views.ConsoleSuspendUserView.as_view(), + name="admin_suspend_user", + ), + path( + "console/users//unsuspend/", + views.ConsoleUnsuspendUserView.as_view(), + name="admin_unsuspend_user", + ), path( "console/surveys/", views.ConsoleSurveyListView.as_view(), name="admin_surveys" ), diff --git a/home/views/__init__.py b/home/views/__init__.py index 687f30cd..77589e56 100644 --- a/home/views/__init__.py +++ b/home/views/__init__.py @@ -41,6 +41,8 @@ ConsoleProjectDetailView, ConsoleProjectListView, ConsoleRemoveMemberView, + ConsoleSuspendUserView, + ConsoleUnsuspendUserView, ConsoleUserDetailView, ) from .project import ( @@ -91,6 +93,8 @@ "ConsoleProjectDetailView", "ConsoleProjectListView", "ConsoleRemoveMemberView", + "ConsoleSuspendUserView", + "ConsoleUnsuspendUserView", "ConsoleUserDetailView", "ConsoleDataProtectionLogView", ] diff --git a/home/views/auth.py b/home/views/auth.py index 56fe8950..f7a34c05 100644 --- a/home/views/auth.py +++ b/home/views/auth.py @@ -13,7 +13,7 @@ PasswordResetDoneView, PasswordResetView, ) -from django.core.exceptions import PermissionDenied +from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied from django.db import IntegrityError from django.shortcuts import redirect from django.urls import reverse_lazy @@ -82,7 +82,11 @@ def form_valid(self, form): "administrator of your organisation to re-send your invitation.", ) return self.form_invalid(form) - login(self.request, user, backend="django.contrib.auth.backends.ModelBackend") + login( + self.request, + user, + backend="django.contrib.auth.backends.AllowAllUsersModelBackend", + ) return redirect(reverse_lazy("dashboard")) @@ -96,7 +100,18 @@ class LoginInterfaceView(LoginView): success_url = reverse_lazy("dashboard") def form_invalid(self, form): - messages.error(self.request, "Invalid email or password.") + # AllowAllUsersModelBackend lets authenticate() succeed for suspended + # (is_active=False) users, so confirm_login_allowed() raises this + # "inactive" error only once the password has already been verified — + # avoiding a second check_password() call that would otherwise leak + # suspension status via a timing side-channel. + if form.has_error(NON_FIELD_ERRORS, code="inactive"): + messages.error( + self.request, + "Your account has been suspended. Please contact your administrator.", + ) + else: + messages.error(self.request, "Invalid email or password.") return super().form_invalid(form) def dispatch(self, request, *args, **kwargs): diff --git a/home/views/console.py b/home/views/console.py index 6aa7ad87..0eb0a710 100644 --- a/home/views/console.py +++ b/home/views/console.py @@ -12,6 +12,7 @@ from django.views.generic import TemplateView, View from django.views.generic.base import TemplateResponseMixin +from home.constants import DELETED_ACCOUNT_EMAIL_DOMAIN from home.mixins import StaffRequiredMixin from home.models import ( DataProtectionEvent, @@ -85,13 +86,16 @@ def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) status = self.request.GET.get("status", "active") qs = User.objects.order_by("last_name", "first_name") + deleted_filter = {"email__endswith": f"@{DELETED_ACCOUNT_EMAIL_DOMAIN}"} if status == "deleted": - qs = qs.filter(is_active=False) + qs = qs.filter(**deleted_filter) + elif status == "suspended": + qs = qs.filter(is_active=False).exclude(**deleted_filter) elif status == "all": pass else: status = "active" - qs = qs.filter(is_active=True) + qs = qs.exclude(**deleted_filter) context["users"] = qs context["status_filter"] = status return context @@ -259,6 +263,50 @@ def post(self, request, org_pk, membership_pk): return redirect("admin_organisation_detail", pk=org_pk) +class ConsoleSuspendUserView(StaffRequiredMixin, TemplateResponseMixin, View): + """ + Suspend a user account (UK GDPR Article 18, Right to Restriction). + + Sets ``is_active = False`` so the user can no longer log in. No data is + deleted and the action is reversible via :class:`ConsoleUnsuspendUserView`. + """ + + template_name = "console/suspend_user_confirm.html" + + def _get_suspendable_user(self, request, pk): + user = get_object_or_404(User, pk=pk) + # Guard against locking out yourself, a fellow staff member, or a + # superuser, and against suspending an already-anonymised (deleted) + # account. + if user == request.user or user.is_staff or user.is_superuser or user.is_deleted: + raise PermissionDenied("This account cannot be suspended.") + return user + + def get(self, request, pk): + user = self._get_suspendable_user(request, pk) + return self.render_to_response({"viewed_user": user}) + + def post(self, request, pk): + user = self._get_suspendable_user(request, pk) + user.is_active = False + user.save(update_fields=["is_active"]) + messages.success(request, f"{user} has been suspended.") + return redirect("admin_user_detail", pk=pk) + + +class ConsoleUnsuspendUserView(StaffRequiredMixin, View): + """Lift the suspension on a user account, restoring login access.""" + + def post(self, request, pk): + user = get_object_or_404(User, pk=pk) + if user.is_deleted: + raise PermissionDenied("This account has been deleted and cannot be reactivated.") + user.is_active = True + user.save(update_fields=["is_active"]) + messages.success(request, f"The suspension on {user} has been lifted.") + return redirect("admin_user_detail", pk=pk) + + class ConsoleDataProtectionLogView(StaffRequiredMixin, TemplateView): template_name = "console/data_protection_log.html"