diff --git a/ephios/core/forms/events.py b/ephios/core/forms/events.py index 14974f4ed..ce199e170 100644 --- a/ephios/core/forms/events.py +++ b/ephios/core/forms/events.py @@ -119,7 +119,7 @@ def save(self, commit=True): # delete existing permissions # (better implement https://github.com/django-guardian/django-guardian/issues/654) for group in get_groups_with_perms( - event, only_with_perms_in=["view_event", "change_event"] + event, only_with_perms_in=["view_event", "change_event"], must_have_all_perms=False ): remove_perm("view_event", group, event) remove_perm("change_event", group, event) diff --git a/ephios/core/forms/users.py b/ephios/core/forms/users.py index 54c14727e..668c5fde3 100644 --- a/ephios/core/forms/users.py +++ b/ephios/core/forms/users.py @@ -25,7 +25,7 @@ from ephios.core.signals import register_group_permission_fields from ephios.core.widgets import MultiUserProfileWidget from ephios.extra.crispy import AbortLink -from ephios.extra.permissions import PermissionField, PermissionFormMixin +from ephios.extra.permissions import PermissionField, PermissionFormMixin, get_groups_with_perms from ephios.extra.widgets import CustomDateInput from ephios.modellogging.log import add_log_recorder from ephios.modellogging.recorders import DerivedFieldsLogRecorder @@ -176,6 +176,24 @@ def __init__(self, **kwargs): ), ) + def clean_is_management_group(self): + is_management_group = self.cleaned_data["is_management_group"] + if self.fields["is_management_group"].initial and not is_management_group: + other_management_groups = get_groups_with_perms( + only_with_perms_in=CORE_MANAGEMENT_PERMISSIONS, + must_have_all_perms=True, + ).exclude(pk=self.instance.pk) + if not other_management_groups.exists(): + raise ValidationError( + _( + "At least one group with management permissions must exist. " + "Please promote another group before demoting this one." + ) + ) + if is_management_group: + self.cleaned_data["is_hr_group"] = True + return is_management_group + def save(self, commit=True): add_log_recorder(self.instance, DerivedFieldsLogRecorder(get_group_permission_log_fields)) group = super().save(commit) @@ -200,7 +218,7 @@ def save(self, commit=True): return group -class UserProfileForm(ModelForm): +class UserProfileForm(PermissionFormMixin, ModelForm): groups = ModelMultipleChoiceField( label=_("Groups"), queryset=Group.objects.all(), @@ -208,6 +226,14 @@ class UserProfileForm(ModelForm): required=False, disabled=True, # explicitly enable for users with `change_group` permission ) + is_staff = PermissionField( + label=_("Administrator"), + help_text=_( + "If checked, this user can change technical ephios settings as well as edit all user profiles, " + "groups, qualifications, events and event types." + ), + permissions=CORE_MANAGEMENT_PERMISSIONS, + ) def __init__(self, *args, **kwargs): request = kwargs.pop("request") @@ -230,6 +256,10 @@ def __init__(self, *args, **kwargs): "Only other technical administrators can change this." ) + @property + def permission_target(self): + return self.instance + field_order = [ "email", "first_name", @@ -280,6 +310,44 @@ def save(self, commit=True): return userprofile +class DeleteUserProfileForm(Form): + def __init__(self, *args, **kwargs): + self.instance = kwargs.pop("instance") + super().__init__(*args, **kwargs) + + def clean(self): + other_staff = UserProfile.objects.filter(is_staff=True).exclude(pk=self.instance.pk) + if self.instance.is_staff and not other_staff.exists(): + raise ValidationError( + _( + "At least one user must be technical administrator. " + "Please promote another user before deleting this one." + ) + ) + + +class DeleteGroupForm(Form): + def __init__(self, *args, **kwargs): + self.instance = kwargs.pop("instance") + super().__init__(*args, **kwargs) + + def clean(self): + management_groups = get_groups_with_perms( + only_with_perms_in=CORE_MANAGEMENT_PERMISSIONS, must_have_all_perms=True + ) + + if ( + self.instance in management_groups + and not management_groups.exclude(pk=self.instance.pk).exists() + ): + raise ValidationError( + _( + "At least one group with management permissions must exist. " + "Please promote another group before deleting this one." + ) + ) + + class QualificationGrantForm(ModelForm): model = QualificationGrant diff --git a/ephios/core/models/users.py b/ephios/core/models/users.py index 92136f83b..e7481a38b 100644 --- a/ephios/core/models/users.py +++ b/ephios/core/models/users.py @@ -104,8 +104,7 @@ class UserProfile(guardian.mixins.GuardianUserMixin, PermissionsMixin, AbstractB is_visible = BooleanField(default=True, verbose_name=_("Visible")) is_staff = BooleanField( default=False, - verbose_name=_("Technical Administrator"), - help_text=_("If checked, this user can change technical ephios settings."), + verbose_name=_("Administrator"), ) first_name = CharField(_("first name"), max_length=254) last_name = CharField(_("last name"), max_length=254) diff --git a/ephios/core/templates/core/group_confirm_delete.html b/ephios/core/templates/core/group_confirm_delete.html index 0d84343b8..369de67f6 100644 --- a/ephios/core/templates/core/group_confirm_delete.html +++ b/ephios/core/templates/core/group_confirm_delete.html @@ -1,4 +1,5 @@ {% extends "base.html" %} +{% load crispy_forms_filters %} {% load i18n %} {% load static %} @@ -12,6 +13,7 @@

{% trans "Delete group" %}

{% csrf_token %} + {{ form|as_crispy_errors }}

{% blocktrans %}Are you sure you want to delete the group "{{ group }}"?{% endblocktrans %}

{% trans "Back" %} diff --git a/ephios/core/templates/core/userprofile_confirm_delete.html b/ephios/core/templates/core/userprofile_confirm_delete.html index bb29c65f2..e8a26987b 100644 --- a/ephios/core/templates/core/userprofile_confirm_delete.html +++ b/ephios/core/templates/core/userprofile_confirm_delete.html @@ -1,4 +1,5 @@ {% extends "base.html" %} +{% load crispy_forms_filters %} {% load i18n %} {% load static %} @@ -12,6 +13,7 @@

{% translate "Delete user" %}

{% csrf_token %} + {{ form|as_crispy_errors }}

{% blocktranslate trimmed with name=userprofile.get_full_name %} Are you sure you want to delete the user {{ name }} ({{ userprofile }})? {% endblocktranslate %}

diff --git a/ephios/core/views/accounts.py b/ephios/core/views/accounts.py index 154f01123..29ed6e3bd 100644 --- a/ephios/core/views/accounts.py +++ b/ephios/core/views/accounts.py @@ -10,7 +10,13 @@ from dynamic_preferences.registries import global_preferences_registry from ephios.api.access.auth import revoke_all_access_tokens -from ephios.core.forms.users import GroupForm, QualificationGrantFormset, UserProfileForm +from ephios.core.forms.users import ( + DeleteGroupForm, + DeleteUserProfileForm, + GroupForm, + QualificationGrantFormset, + UserProfileForm, +) from ephios.core.models import QualificationGrant, UserProfile from ephios.core.services.notifications.types import ( NewProfileNotification, @@ -111,8 +117,8 @@ def post(self, request, *args, **kwargs): qualification_formset.save() messages.success( self.request, - _("User {name} ({user}) updated successfully.").format( - name=self.object.get_full_name(), user=self.object + _("User {name} ({email}) updated successfully.").format( + name=self.object.get_full_name(), email=self.object.email ), ) if request.user != userprofile: @@ -130,12 +136,18 @@ class UserProfileDeleteView(CustomPermissionRequiredMixin, DeleteView): model = UserProfile permission_required = "core.delete_userprofile" template_name = "core/userprofile_confirm_delete.html" + form_class = DeleteUserProfileForm + + def get_form_kwargs(self): + kwargs = super().get_form_kwargs() + kwargs["instance"] = self.object + return kwargs def get_success_url(self): messages.info( self.request, - _("The user {name} ({user}) was deleted.").format( - name=self.object.get_full_name(), user=self.object + _("The user {name} ({email}) was deleted.").format( + name=self.object.get_full_name(), email=self.object.email ), ) return reverse("core:userprofile_list") @@ -233,6 +245,12 @@ class GroupDeleteView(CustomPermissionRequiredMixin, DeleteView): model = Group permission_required = "auth.delete_group" template_name = "core/group_confirm_delete.html" + form_class = DeleteGroupForm + + def get_form_kwargs(self): + kwargs = super().get_form_kwargs() + kwargs["instance"] = self.object + return kwargs def get_success_url(self): messages.info(self.request, _('The group "{group}" was deleted.').format(group=self.object)) diff --git a/ephios/core/views/event.py b/ephios/core/views/event.py index 78914c3bc..4f603d346 100644 --- a/ephios/core/views/event.py +++ b/ephios/core/views/event.py @@ -412,15 +412,21 @@ def form_valid(self, form): event.pk = None event.save() assign_perm( - "view_event", get_groups_with_perms(self.get_object(), ["view_event"]), event + "view_event", + get_groups_with_perms(self.get_object(), only_with_perms_in=["view_event"]), + event, ) assign_perm( - "change_event", get_groups_with_perms(self.get_object(), ["change_event"]), event + "change_event", + get_groups_with_perms(self.get_object(), only_with_perms_in=["change_event"]), + event, ) assign_perm( "change_event", get_users_with_perms( - self.get_object(), only_with_perms_in=["change_event"], with_group_users=False + self.get_object(), + only_with_perms_in=["change_event"], + with_group_users=False, ), event, ) diff --git a/ephios/extra/permissions.py b/ephios/extra/permissions.py index 82c44e536..3341467e5 100644 --- a/ephios/extra/permissions.py +++ b/ephios/extra/permissions.py @@ -1,12 +1,17 @@ from functools import wraps +from typing import Optional +from django.contrib.auth import get_user_model from django.contrib.auth.models import Group, Permission from django.core.exceptions import PermissionDenied +from django.db.models import Q from django.forms import BooleanField from guardian.ctypes import get_content_type from guardian.shortcuts import assign_perm, remove_perm from guardian.utils import get_group_obj_perms_model +Q_FALSE = Q(pk__in=[]) + def staff_required(view_func): @wraps(view_func) @@ -22,35 +27,85 @@ def _wrapped_view(request, *args, **kwargs): return _wrapped_view -def get_groups_with_perms(obj, only_with_perms_in): - ctype = get_content_type(obj) - group_model = get_group_obj_perms_model(obj) - - group_rel_name = group_model.group.field.related_query_name() - - if group_model.objects.is_generic(): - group_filters = { - f"{group_rel_name}__content_type": ctype, - f"{group_rel_name}__object_pk": obj.pk, - } +def _make_permission_names_qualified(permission_names, obj: Optional): + """ + Return a list of permission names in the form "app_label.codename" for the given permissions codenames. + If obj is given, the app_label is inferred from the object's meta. + Otherwise, the permission name must already be qualified. + """ + qualified_permission_names = [] + for name in permission_names or []: + if "." in name: + qualified_permission_names.append(name) + elif obj is not None: + app_label = obj._meta.app_label # pylint: disable=protected-access + qualified_permission_names.append(f"{app_label}.{name}") + else: + raise ValueError("Cannot infer permission app_label from name without obj") + return qualified_permission_names + + +def get_groups_with_perms(obj=None, *, only_with_perms_in=None, must_have_all_perms=False): + qs = Group.objects.all() + qualified_permission_names = _make_permission_names_qualified(only_with_perms_in or [], obj) + required_perms = get_permissions_from_qualified_names(qualified_permission_names) + + group_perms_model = get_group_obj_perms_model(obj) + group_perms_rel_name = group_perms_model.group.field.related_query_name() + obj_filter = {} + if obj is not None: + ctype = get_content_type(obj) + if group_perms_model.objects.is_generic(): + obj_filter = { + f"{group_perms_rel_name}__content_type": ctype, + f"{group_perms_rel_name}__object_pk": obj.pk, + } + else: + obj_filter = {f"{group_perms_rel_name}__content_object": obj} + + if must_have_all_perms: + for perm in required_perms: + if obj is not None: + qs = qs.filter( + Q(permissions=perm) + | Q( + **{ + f"{group_perms_rel_name}__permission": perm, + **obj_filter, + } + ) + ) + else: + qs = qs.filter(permissions=perm) else: - group_filters = {f"{group_rel_name}__content_object": obj} - - permission_ids = Permission.objects.filter( - content_type=ctype, codename__in=only_with_perms_in - ).values_list("id", flat=True) - group_filters.update( - { - f"{group_rel_name}__permission_id__in": permission_ids, - } - ) - return Group.objects.filter(**group_filters).distinct() + if obj is not None: + qs = qs.filter( + Q(permissions__in=required_perms) + | Q( + **{ + f"{group_perms_rel_name}__permission__in": required_perms, + **obj_filter, + } + ) + ) + else: + qs = qs.filter(Q(permissions__in=required_perms)) + return qs.distinct() + + +def get_permissions_from_qualified_names(qualified_names): + perms_filter = Q_FALSE + for qualified_name in qualified_names: + app_label, codename = qualified_name.split(".") + perms_filter = perms_filter | Q(content_type__app_label=app_label, codename=codename) + return Permission.objects.filter(perms_filter) class PermissionField(BooleanField): """ - This field takes a list of permissions and a permission_target and renders a checkbox that is checked if the target - has all given permissions. It requires a permission_target attribute on the form as well as calling the appropriate + This boolean field takes an additional list of permissions and a permission_target and ist true by default if the + permission_target (user or group) has all given permissions. + It requires a permission_target attribute on the form as well as calling the appropriate methods which is taken care of by PermissionFormMixin """ @@ -60,11 +115,16 @@ def __init__(self, *args, **kwargs): def set_initial_value(self, user_or_group): self.target = user_or_group - codename_set = set(map(lambda perm: perm.split(".")[-1], self.permission_set)) - self.initial = codename_set <= set( - self.target.permissions.values_list("codename", flat=True) + wanted_permission_objects = get_permissions_from_qualified_names(self.permission_set) + self.initial = set(wanted_permission_objects) <= set( + self._get_permissions_from_user_or_group(self.target) ) + def _get_permissions_from_user_or_group(self, user_or_group): + if isinstance(user_or_group, get_user_model()): + return user_or_group.user_permissions.all() + return user_or_group.permissions.all() + def assign_permissions(self, target): for permission in self.permission_set: assign_perm(permission, target) diff --git a/tests/core/test_event_copy.py b/tests/core/test_event_copy.py index f12f1433d..3e5c73471 100644 --- a/tests/core/test_event_copy.py +++ b/tests/core/test_event_copy.py @@ -16,8 +16,12 @@ def assert_dates(self, event, occurrences, volunteers, planners): assert shift.event.get_start_time() == shift.start_time assert shift.meeting_time.date() == shift.start_time.date() assert shift.end_time.date() == shift.start_time.date() - assert (volunteers and planners) in get_groups_with_perms(shift.event, ["view_event"]) - assert planners in get_groups_with_perms(shift.event, ["change_event"]) + assert (volunteers and planners) in get_groups_with_perms( + shift.event, only_with_perms_in=["view_event"] + ) + assert planners in get_groups_with_perms( + shift.event, only_with_perms_in=["change_event"] + ) def test_event_copy_by_rule(self, django_app, planner, event, groups): managers, planners, volunteers = groups @@ -63,9 +67,9 @@ def test_event_copy_by_date(self, django_app, planner, event, groups, volunteer) assert Event.objects.all().count() == event_count + 2 assert Shift.objects.filter(start_time__date__in=occurrences).count() == 2 self.assert_dates(event, occurrences, volunteers, planners) - assert volunteer.has_perm("change_event", new_event) - assert set(get_groups_with_perms(new_event, ["change_event"])) == set( - get_groups_with_perms(event, ["change_event"]) + assert planner.has_perm("change_event", new_event) + assert set(get_groups_with_perms(new_event, only_with_perms_in=["change_event"])) == set( + get_groups_with_perms(event, only_with_perms_in=["change_event"]) ) def test_event_multi_shift_copy(self, django_app, planner, groups, multi_shift_event): @@ -99,8 +103,12 @@ def test_event_multi_shift_copy(self, django_app, planner, groups, multi_shift_e shift.start_time.date(), shift.start_time.date() + timedelta(days=1), ] - assert (volunteers and planners) in get_groups_with_perms(shift.event, ["view_event"]) - assert planners in get_groups_with_perms(shift.event, ["change_event"]) + assert (volunteers and planners) in get_groups_with_perms( + shift.event, only_with_perms_in=["view_event"] + ) + assert planners in get_groups_with_perms( + shift.event, only_with_perms_in=["change_event"] + ) second_shift = shift.event.shifts.get(start_time__date=shift_date + timedelta(days=1)) assert second_shift.start_time.date() == shift.start_time.date() + timedelta(days=1) @@ -131,5 +139,9 @@ def test_event_to_next_day_copy(self, django_app, planner, event_to_next_day, gr assert shift.event.get_start_time() == shift.start_time assert shift.meeting_time.date() == shift.start_time.date() assert shift.end_time.date() == shift.start_time.date() + timedelta(days=1) - assert (volunteers and planners) in get_groups_with_perms(shift.event, ["view_event"]) - assert planners in get_groups_with_perms(shift.event, ["change_event"]) + assert (volunteers and planners) in get_groups_with_perms( + shift.event, only_with_perms_in=["view_event"] + ) + assert planners in get_groups_with_perms( + shift.event, only_with_perms_in=["change_event"] + ) diff --git a/tests/core/test_event_form.py b/tests/core/test_event_form.py index 378431529..1ca15437a 100644 --- a/tests/core/test_event_form.py +++ b/tests/core/test_event_form.py @@ -32,8 +32,12 @@ def test_create_event(django_app, planner, superuser, service_event_type, groups assert set(get_groups_with_perms(event, only_with_perms_in=["view_event"])) == { volunteers, planners, + managers, + } + assert set(get_groups_with_perms(event, only_with_perms_in=["change_event"])) == { + planners, + managers, } - assert set(get_groups_with_perms(event, only_with_perms_in=["change_event"])) == {planners} assert set( get_users_with_perms(event, only_with_perms_in=["change_event"], with_group_users=False) ) == {planner} diff --git a/tests/core/test_views_group.py b/tests/core/test_views_group.py index ea371cb7c..ff6557f4d 100644 --- a/tests/core/test_views_group.py +++ b/tests/core/test_views_group.py @@ -3,6 +3,9 @@ from django.urls import reverse from guardian.shortcuts import get_group_perms +from ephios.core.forms.users import CORE_MANAGEMENT_PERMISSIONS +from ephios.extra.permissions import get_groups_with_perms + class TestGroupView: def test_group_list_permission_required(self, django_app, volunteer): @@ -79,9 +82,16 @@ def test_group_create_with_permissions(self, django_app, groups, manager): assert group.permissions.filter(codename="view_group").exists() def test_group_edit(self, django_app, groups, manager): - group = manager.groups.first() + managers, planners, volunteers = groups + # promote planning group to management group, so we can delete the management group + form = django_app.get( + reverse("core:group_edit", kwargs={"pk": planners.id}), user=manager + ).form + form["is_management_group"] = True + form.submit() + form = django_app.get( - reverse("core:group_edit", kwargs={"pk": group.id}), user=manager + reverse("core:group_edit", kwargs={"pk": managers.id}), user=manager ).form group_name = "New name" form["name"] = group_name @@ -91,12 +101,12 @@ def test_group_edit(self, django_app, groups, manager): form["publish_event_for_group"].select_multiple(texts=["Volunteers"]) response = form.submit() assert response.status_code == 302 - group.refresh_from_db() - assert group.name == group_name - assert set(group.user_set.all()) == {manager} - assert not group.permissions.filter(codename="add_event").exists() + managers.refresh_from_db() + assert managers.name == group_name + assert set(managers.user_set.all()) == {manager} + assert not managers.permissions.filter(codename="add_event").exists() assert "publish_event_for_group" not in get_group_perms( - group, Group.objects.get(name="Volunteers") + managers, Group.objects.get(name="Volunteers") ) def test_group_delete(self, django_app, groups, manager): @@ -110,3 +120,32 @@ def test_group_delete(self, django_app, groups, manager): assert response.status_code == 302 with pytest.raises(Group.DoesNotExist): Group.objects.get(name=group.name).exists() + + def test_cannot_delete_last_management_group(self, django_app, groups, manager): + group = Group.objects.get(name="Managers") + response = django_app.get( + reverse("core:group_delete", kwargs={"pk": group.id}), + user=manager, + status=200, + ) + response = response.form.submit() + assert response.status_code == 200 + assert "least one group with management permissions" in response.text + assert Group.objects.filter(name="Managers").exists() + + def test_management_perms_cannot_be_removed_from_last_management_group( + self, django_app, superuser, groups + ): + group = Group.objects.get(name="Managers") + form = django_app.get( + reverse("core:group_edit", kwargs={"pk": group.id}), + user=superuser, + status=200, + ).form + form["is_management_group"] = False + response = form.submit() + assert response.status_code == 200 + assert "least one group with management permissions must exist" in response.text + assert Group.objects.get(name="Managers") in get_groups_with_perms( + only_with_perms_in=CORE_MANAGEMENT_PERMISSIONS, must_have_all_perms=True + ) diff --git a/tests/core/test_views_userprofile.py b/tests/core/test_views_userprofile.py index 8c0ef978a..7524bc4c6 100644 --- a/tests/core/test_views_userprofile.py +++ b/tests/core/test_views_userprofile.py @@ -184,3 +184,23 @@ def test_staffuser_can_change_is_staff_flag(self, django_app, volunteer, superus form["is_staff"] = True form.submit() assert UserProfile.objects.get(id=volunteer.id).is_staff + + def test_staff_flag_cannot_be_removed_from_last_staff_user(self, django_app, superuser): + form = django_app.get( + reverse("core:userprofile_edit", kwargs={"pk": superuser.id}), user=superuser + ).form + form["is_staff"] = False + response = form.submit() + assert response.status_code == 200 + assert "least one user must be technical administrator" in response.text + assert UserProfile.objects.get(id=superuser.id).is_staff + + def test_last_staff_user_cannot_be_deleted(self, django_app, groups, manager, superuser): + response = django_app.get( + reverse("core:userprofile_delete", kwargs={"pk": superuser.id}), + user=manager, + ) + response = response.form.submit() + assert response.status_code == 200 + assert "least one user must be technical administrator" in response.text + assert UserProfile.objects.get(id=superuser.id).is_staff diff --git a/tests/extra/test_permissions.py b/tests/extra/test_permissions.py new file mode 100644 index 000000000..9fd365a12 --- /dev/null +++ b/tests/extra/test_permissions.py @@ -0,0 +1,38 @@ +import pytest + +from ephios.core.forms.users import CORE_MANAGEMENT_PERMISSIONS +from ephios.extra.permissions import get_groups_with_perms + + +def test_querying_get_groups_with_perms(groups, event): + managers, planners, volunteers = groups + + groups_with_any_perm = get_groups_with_perms( + event, + only_with_perms_in=["view_event", "change_event"], + must_have_all_perms=False, + ) + assert set(groups) == set(groups_with_any_perm) + + groups_with_all_perms = get_groups_with_perms( + event, only_with_perms_in=["view_event", "change_event"], must_have_all_perms=True + ) + assert set(groups_with_all_perms) == {managers, planners} + + management_groups_by_list = get_groups_with_perms( + only_with_perms_in=CORE_MANAGEMENT_PERMISSIONS, + must_have_all_perms=True, + ) + assert set(management_groups_by_list) == {managers} + + management_groups_by_list = get_groups_with_perms( + only_with_perms_in=["core.view_event", "core.change_event"], + must_have_all_perms=False, + ) + assert set(management_groups_by_list) == {managers} + + with pytest.raises(ValueError): + # view_event is not a specific enough to identify a permission + get_groups_with_perms( + only_with_perms_in=["view_event"], + )