fixup! core: fix group creation for limited permission users

Broken by https://github.com/goauthentik/authentik/pull/12900

authentik/core/api/groups.py

@@ -16,10 +16,12 @@ from drf_spectacular.utils import (
 from guardian.shortcuts import get_objects_for_user
 from rest_framework.decorators import action
 from rest_framework.fields import CharField, IntegerField, SerializerMethodField
+from rest_framework.permissions import SAFE_METHODS, BasePermission
 from rest_framework.request import Request
 from rest_framework.response import Response
 from rest_framework.serializers import ListSerializer, ValidationError
 from rest_framework.validators import UniqueValidator
+from rest_framework.views import View
 from rest_framework.viewsets import ModelViewSet
 
 from authentik.core.api.used_by import UsedByMixin
@@ -85,34 +87,6 @@ class GroupSerializer(ModelSerializer):
             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 = [
@@ -180,6 +154,36 @@ class GroupFilter(FilterSet):
         fields = ["name", "is_superuser", "members_by_pk", "attributes", "members_by_username"]
 
 
+class SuperuserSetter(BasePermission):
+    """Check for enable_group_superuser or disable_group_superuser permissions"""
+
+    message = _("User does not have permission to set the given superuser status.")
+    enable_perm = "authentik_core.enable_group_superuser"
+    disable_perm = "authentik_core.disable_group_superuser"
+
+    def has_permission(self, request: Request, view: View):
+        if request.method != "POST":
+            return True
+
+        is_superuser = request.data.get("is_superuser", False)
+        if not is_superuser:
+            return True
+
+        return request.user.has_perm(self.enable_perm)
+
+    def has_object_permission(self, request: Request, view: View, object: Group):
+        if request.method in SAFE_METHODS:
+            return True
+
+        new_value = request.data.get("is_superuser")
+        old_value = object.is_superuser
+        if new_value is None or new_value == old_value:
+            return True
+
+        perm = self.enable_perm if new_value else self.disable_perm
+        return request.user.has_perm(perm) or request.user.has_perm(perm, object)
+
+
 class GroupViewSet(UsedByMixin, ModelViewSet):
     """Group Viewset"""
 
@@ -192,6 +196,7 @@ class GroupViewSet(UsedByMixin, ModelViewSet):
     serializer_class = GroupSerializer
     search_fields = ["name", "is_superuser"]
     filterset_class = GroupFilter
+    permission_classes = [SuperuserSetter]
     ordering = ["name"]
 
     def get_queryset(self):

authentik/core/tests/test_groups_api.py

@@ -118,12 +118,25 @@ class TestGroupsAPI(APITestCase):
             reverse("authentik_api:group-list"),
             data={"name": generate_id(), "is_superuser": True},
         )
-        self.assertEqual(res.status_code, 400)
+        self.assertEqual(res.status_code, 403)
         self.assertJSONEqual(
             res.content,
-            {"is_superuser": ["User does not have permission to set superuser status to True."]},
+            {"detail": "User does not have permission to set the given superuser status."},
         )
 
+    def test_superuser_update_object_perm(self):
+        """Test updating a superuser group with object permission"""
+        group = Group.objects.create(name=generate_id(), is_superuser=False)
+        assign_perm("view_group", self.login_user, group)
+        assign_perm("change_group", self.login_user, group)
+        assign_perm("enable_group_superuser", 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": True},
+        )
+        self.assertEqual(res.status_code, 200)
+
     def test_superuser_update_no_perm(self):
         """Test updating a superuser group without permission"""
         group = Group.objects.create(name=generate_id(), is_superuser=True)
@@ -134,10 +147,10 @@ class TestGroupsAPI(APITestCase):
             reverse("authentik_api:group-detail", kwargs={"pk": group.pk}),
             data={"is_superuser": False},
         )
-        self.assertEqual(res.status_code, 400)
+        self.assertEqual(res.status_code, 403)
         self.assertJSONEqual(
             res.content,
-            {"is_superuser": ["User does not have permission to set superuser status to False."]},
+            {"detail": "User does not have permission to set the given superuser status."},
         )
 
     def test_superuser_update_no_change(self):
@@ -163,3 +176,27 @@ class TestGroupsAPI(APITestCase):
             data={"name": generate_id(), "is_superuser": True},
         )
         self.assertEqual(res.status_code, 201)
+
+    def test_superuser_create_no_perm(self):
+        """Test creating a superuser group with no 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, 403)
+        self.assertJSONEqual(
+            res.content,
+            {"detail": "User does not have permission to set the given superuser status."},
+        )
+
+    def test_no_superuser_create_no_perm(self):
+        """Test creating a non-superuser group with no 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()},
+        )
+        self.assertEqual(res.status_code, 201)