From 74e090239a4bc2e019274d043d5d11d8acbcec67 Mon Sep 17 00:00:00 2001 From: "Jens L." Date: Mon, 17 Feb 2025 16:57:21 +0100 Subject: [PATCH] core: add additional RBAC permission to restrict setting the superuser status on groups (#12900) Signed-off-by: Jens Langhammer --- authentik/core/api/groups.py | 31 +++++++++- .../migrations/0043_alter_group_options.py | 26 +++++++++ authentik/core/models.py | 2 + authentik/core/tests/test_groups_api.py | 58 ++++++++++++++++++- blueprints/schema.json | 6 ++ 5 files changed, 120 insertions(+), 3 deletions(-) create mode 100644 authentik/core/migrations/0043_alter_group_options.py diff --git a/authentik/core/api/groups.py b/authentik/core/api/groups.py index fd9358152b..6a341947d9 100644 --- a/authentik/core/api/groups.py +++ b/authentik/core/api/groups.py @@ -4,6 +4,7 @@ from json import loads from django.db.models import Prefetch from django.http import Http404 +from django.utils.translation import gettext as _ from django_filters.filters import CharFilter, ModelMultipleChoiceFilter from django_filters.filterset import FilterSet from drf_spectacular.utils import ( @@ -81,9 +82,37 @@ class GroupSerializer(ModelSerializer): if not self.instance or not parent: return parent if str(parent.group_uuid) == str(self.instance.group_uuid): - raise ValidationError("Cannot set group as parent of itself.") + raise ValidationError(_("Cannot set group as parent of itself.")) return parent + def validate_is_superuser(self, superuser: bool): + """Ensure that the user creating this group has permissions to set the superuser flag""" + request: Request = self.context.get("request", None) + if not request: + return superuser + # If we're updating an instance, and the state hasn't changed, we don't need to check perms + if self.instance and superuser == self.instance.is_superuser: + return superuser + user: User = request.user + perm = ( + "authentik_core.enable_group_superuser" + if superuser + else "authentik_core.disable_group_superuser" + ) + has_perm = user.has_perm(perm) + if self.instance and not has_perm: + has_perm = user.has_perm(perm, self.instance) + if not has_perm: + raise ValidationError( + _( + ( + "User does not have permission to set " + "superuser status to {superuser_status}." + ).format_map({"superuser_status": superuser}) + ) + ) + return superuser + class Meta: model = Group fields = [ diff --git a/authentik/core/migrations/0043_alter_group_options.py b/authentik/core/migrations/0043_alter_group_options.py new file mode 100644 index 0000000000..6f1fda05df --- /dev/null +++ b/authentik/core/migrations/0043_alter_group_options.py @@ -0,0 +1,26 @@ +# Generated by Django 5.0.11 on 2025-01-30 23:55 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("authentik_core", "0042_authenticatedsession_authentik_c_expires_08251d_idx_and_more"), + ] + + operations = [ + migrations.AlterModelOptions( + name="group", + options={ + "permissions": [ + ("add_user_to_group", "Add user to group"), + ("remove_user_from_group", "Remove user from group"), + ("enable_group_superuser", "Enable superuser status"), + ("disable_group_superuser", "Disable superuser status"), + ], + "verbose_name": "Group", + "verbose_name_plural": "Groups", + }, + ), + ] diff --git a/authentik/core/models.py b/authentik/core/models.py index 5caee66c72..2aa09fd5cd 100644 --- a/authentik/core/models.py +++ b/authentik/core/models.py @@ -204,6 +204,8 @@ class Group(SerializerModel, AttributesMixin): permissions = [ ("add_user_to_group", _("Add user to group")), ("remove_user_from_group", _("Remove user from group")), + ("enable_group_superuser", _("Enable superuser status")), + ("disable_group_superuser", _("Disable superuser status")), ] def __str__(self): diff --git a/authentik/core/tests/test_groups_api.py b/authentik/core/tests/test_groups_api.py index eb31bf4095..3b8216bfd4 100644 --- a/authentik/core/tests/test_groups_api.py +++ b/authentik/core/tests/test_groups_api.py @@ -4,7 +4,7 @@ from django.urls.base import reverse from guardian.shortcuts import assign_perm from rest_framework.test import APITestCase -from authentik.core.models import Group, User +from authentik.core.models import Group from authentik.core.tests.utils import create_test_admin_user, create_test_user from authentik.lib.generators import generate_id @@ -14,7 +14,7 @@ class TestGroupsAPI(APITestCase): def setUp(self) -> None: self.login_user = create_test_user() - self.user = User.objects.create(username="test-user") + self.user = create_test_user() def test_list_with_users(self): """Test listing with users""" @@ -109,3 +109,57 @@ class TestGroupsAPI(APITestCase): }, ) self.assertEqual(res.status_code, 400) + + def test_superuser_no_perm(self): + """Test creating a superuser group without permission""" + assign_perm("authentik_core.add_group", self.login_user) + self.client.force_login(self.login_user) + res = self.client.post( + reverse("authentik_api:group-list"), + data={"name": generate_id(), "is_superuser": True}, + ) + self.assertEqual(res.status_code, 400) + self.assertJSONEqual( + res.content, + {"is_superuser": ["User does not have permission to set superuser status to True."]}, + ) + + def test_superuser_update_no_perm(self): + """Test updating a superuser group without permission""" + group = Group.objects.create(name=generate_id(), is_superuser=True) + assign_perm("view_group", self.login_user, group) + assign_perm("change_group", self.login_user, group) + self.client.force_login(self.login_user) + res = self.client.patch( + reverse("authentik_api:group-detail", kwargs={"pk": group.pk}), + data={"is_superuser": False}, + ) + self.assertEqual(res.status_code, 400) + self.assertJSONEqual( + res.content, + {"is_superuser": ["User does not have permission to set superuser status to False."]}, + ) + + def test_superuser_update_no_change(self): + """Test updating a superuser group without permission + and without changing the superuser status""" + group = Group.objects.create(name=generate_id(), is_superuser=True) + assign_perm("view_group", self.login_user, group) + assign_perm("change_group", self.login_user, group) + self.client.force_login(self.login_user) + res = self.client.patch( + reverse("authentik_api:group-detail", kwargs={"pk": group.pk}), + data={"name": generate_id(), "is_superuser": True}, + ) + self.assertEqual(res.status_code, 200) + + def test_superuser_create(self): + """Test creating a superuser group with permission""" + assign_perm("authentik_core.add_group", self.login_user) + assign_perm("authentik_core.enable_group_superuser", self.login_user) + self.client.force_login(self.login_user) + res = self.client.post( + reverse("authentik_api:group-list"), + data={"name": generate_id(), "is_superuser": True}, + ) + self.assertEqual(res.status_code, 201) diff --git a/blueprints/schema.json b/blueprints/schema.json index 9bdc1aef92..d43da273e2 100644 --- a/blueprints/schema.json +++ b/blueprints/schema.json @@ -6565,6 +6565,8 @@ "authentik_core.delete_token", "authentik_core.delete_user", "authentik_core.delete_usersourceconnection", + "authentik_core.disable_group_superuser", + "authentik_core.enable_group_superuser", "authentik_core.impersonate", "authentik_core.preview_user", "authentik_core.remove_user_from_group", @@ -12936,6 +12938,8 @@ "enum": [ "add_user_to_group", "remove_user_from_group", + "enable_group_superuser", + "disable_group_superuser", "add_group", "change_group", "delete_group", @@ -13068,6 +13072,8 @@ "authentik_core.delete_token", "authentik_core.delete_user", "authentik_core.delete_usersourceconnection", + "authentik_core.disable_group_superuser", + "authentik_core.enable_group_superuser", "authentik_core.impersonate", "authentik_core.preview_user", "authentik_core.remove_user_from_group",