core: delegated group member management (#9254)

* fix API permissions Signed-off-by: Jens Langhammer <jens@goauthentik.io> * fix group member remove notification label Signed-off-by: Jens Langhammer <jens@goauthentik.io> * consistent naming assign vs grant Signed-off-by: Jens Langhammer <jens@goauthentik.io> * only set table search query when searching is enabled Signed-off-by: Jens Langhammer <jens@goauthentik.io> * fix hidden object permissions Signed-off-by: Jens Langhammer <jens@goauthentik.io> * replace checkmark/cross with fa icons Signed-off-by: Jens Langhammer <jens@goauthentik.io> * update website Signed-off-by: Jens Langhammer <jens@goauthentik.io> * add tests Signed-off-by: Jens Langhammer <jens@goauthentik.io> * fix tests and fix permission bug Signed-off-by: Jens Langhammer <jens@goauthentik.io> * fix migrations Signed-off-by: Jens Langhammer <jens@goauthentik.io> * reword Signed-off-by: Jens Langhammer <jens@goauthentik.io> --------- Signed-off-by: Jens Langhammer <jens@goauthentik.io>
2024-04-15 14:14:26 +02:00
parent bcc8d5e76c
commit 4a9c95b44e
18 changed files with 160 additions and 53 deletions
--- a/authentik/core/api/groups.py
+++ b/authentik/core/api/groups.py
@ -146,15 +146,14 @@ class GroupFilter(FilterSet):
        fields = ["name", "is_superuser", "members_by_pk", "attributes", "members_by_username"]


-class UserAccountSerializer(PassiveSerializer):
-    """Account adding/removing operations"""
-
-    pk = IntegerField(required=True)
-
-
 class GroupViewSet(UsedByMixin, ModelViewSet):
    """Group Viewset"""

+    class UserAccountSerializer(PassiveSerializer):
+        """Account adding/removing operations"""
+
+        pk = IntegerField(required=True)
+
    queryset = Group.objects.all().select_related("parent").prefetch_related("users")
    serializer_class = GroupSerializer
    search_fields = ["name", "is_superuser"]
@ -169,7 +168,7 @@ class GroupViewSet(UsedByMixin, ModelViewSet):
    def list(self, request, *args, **kwargs):
        return super().list(request, *args, **kwargs)

-    @permission_required(None, ["authentik_core.add_user"])
+    @permission_required("authentik_core.add_user_to_group")
    @extend_schema(
        request=UserAccountSerializer,
        responses={
@ -177,7 +176,13 @@ class GroupViewSet(UsedByMixin, ModelViewSet):
            404: OpenApiResponse(description="User not found"),
        },
    )
-    @action(detail=True, methods=["POST"], pagination_class=None, filter_backends=[])
+    @action(
+        detail=True,
+        methods=["POST"],
+        pagination_class=None,
+        filter_backends=[],
+        permission_classes=[],
+    )
    def add_user(self, request: Request, pk: str) -> Response:
        """Add user to group"""
        group: Group = self.get_object()
@ -193,7 +198,7 @@ class GroupViewSet(UsedByMixin, ModelViewSet):
        group.users.add(user)
        return Response(status=204)

-    @permission_required(None, ["authentik_core.add_user"])
+    @permission_required("authentik_core.remove_user_from_group")
    @extend_schema(
        request=UserAccountSerializer,
        responses={
@ -201,7 +206,13 @@ class GroupViewSet(UsedByMixin, ModelViewSet):
            404: OpenApiResponse(description="User not found"),
        },
    )
-    @action(detail=True, methods=["POST"], pagination_class=None, filter_backends=[])
+    @action(
+        detail=True,
+        methods=["POST"],
+        pagination_class=None,
+        filter_backends=[],
+        permission_classes=[],
+    )
    def remove_user(self, request: Request, pk: str) -> Response:
        """Add user to group"""
        group: Group = self.get_object()
--- a/authentik/core/migrations/0034_group_authentik_c_name_9ba8e4_idx_and_more.py
+++ b/authentik/core/migrations/0034_group_authentik_c_name_9ba8e4_idx_and_more.py
@ -1,4 +1,4 @@
-# Generated by Django 5.0.4 on 2024-04-10 19:05
+# Generated by Django 5.0.4 on 2024-04-15 11:28

 from django.db import migrations, models

@ -7,11 +7,22 @@ class Migration(migrations.Migration):

    dependencies = [
        ("auth", "0012_alter_user_first_name_max_length"),
-        ("authentik_core", "0033_alter_user_options"),
+        ("authentik_core", "0034_alter_authenticatedsession_expires_and_more"),
        ("authentik_rbac", "0003_alter_systempermission_options"),
    ]

    operations = [
+        migrations.AlterModelOptions(
+            name="group",
+            options={
+                "permissions": [
+                    ("add_user_to_group", "Add user to group"),
+                    ("remove_user_from_group", "Remove user from group"),
+                ],
+                "verbose_name": "Group",
+                "verbose_name_plural": "Groups",
+            },
+        ),
        migrations.AddIndex(
            model_name="group",
            index=models.Index(fields=["name"], name="authentik_c_name_9ba8e4_idx"),
--- a/authentik/core/models.py
+++ b/authentik/core/models.py
@ -188,6 +188,10 @@ class Group(SerializerModel):
        indexes = [models.Index(fields=["name"])]
        verbose_name = _("Group")
        verbose_name_plural = _("Groups")
+        permissions = [
+            ("add_user_to_group", _("Add user to group")),
+            ("remove_user_from_group", _("Remove user from group")),
+        ]


 class UserQuerySet(models.QuerySet):
--- a/authentik/core/tests/test_groups_api.py
+++ b/authentik/core/tests/test_groups_api.py
@ -1,10 +1,11 @@
 """Test Groups API"""

 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.tests.utils import create_test_admin_user
+from authentik.core.tests.utils import create_test_user
 from authentik.lib.generators import generate_id


@ -12,13 +13,15 @@ class TestGroupsAPI(APITestCase):
    """Test Groups API"""

    def setUp(self) -> None:
-        self.admin = create_test_admin_user()
+        self.login_user = create_test_user()
        self.user = User.objects.create(username="test-user")

    def test_add_user(self):
        """Test add_user"""
        group = Group.objects.create(name=generate_id())
-        self.client.force_login(self.admin)
+        assign_perm("authentik_core.add_user_to_group", self.login_user, group)
+        assign_perm("authentik_core.view_user", self.login_user)
+        self.client.force_login(self.login_user)
        res = self.client.post(
            reverse("authentik_api:group-add-user", kwargs={"pk": group.pk}),
            data={
@ -32,7 +35,9 @@ class TestGroupsAPI(APITestCase):
    def test_add_user_404(self):
        """Test add_user"""
        group = Group.objects.create(name=generate_id())
-        self.client.force_login(self.admin)
+        assign_perm("authentik_core.add_user_to_group", self.login_user, group)
+        assign_perm("authentik_core.view_user", self.login_user)
+        self.client.force_login(self.login_user)
        res = self.client.post(
            reverse("authentik_api:group-add-user", kwargs={"pk": group.pk}),
            data={
@ -44,8 +49,10 @@ class TestGroupsAPI(APITestCase):
    def test_remove_user(self):
        """Test remove_user"""
        group = Group.objects.create(name=generate_id())
+        assign_perm("authentik_core.remove_user_from_group", self.login_user, group)
+        assign_perm("authentik_core.view_user", self.login_user)
        group.users.add(self.user)
-        self.client.force_login(self.admin)
+        self.client.force_login(self.login_user)
        res = self.client.post(
            reverse("authentik_api:group-remove-user", kwargs={"pk": group.pk}),
            data={
@ -59,8 +66,10 @@ class TestGroupsAPI(APITestCase):
    def test_remove_user_404(self):
        """Test remove_user"""
        group = Group.objects.create(name=generate_id())
+        assign_perm("authentik_core.remove_user_from_group", self.login_user, group)
+        assign_perm("authentik_core.view_user", self.login_user)
        group.users.add(self.user)
-        self.client.force_login(self.admin)
+        self.client.force_login(self.login_user)
        res = self.client.post(
            reverse("authentik_api:group-remove-user", kwargs={"pk": group.pk}),
            data={
@ -72,11 +81,12 @@ class TestGroupsAPI(APITestCase):
    def test_parent_self(self):
        """Test parent"""
        group = Group.objects.create(name=generate_id())
-        self.client.force_login(self.admin)
+        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={
-                "pk": self.user.pk + 3,
                "parent": group.pk,
            },
        )
--- a/authentik/rbac/permissions.py
+++ b/authentik/rbac/permissions.py
@ -8,7 +8,16 @@ from rest_framework.request import Request
 class ObjectPermissions(DjangoObjectPermissions):
    """RBAC Permissions"""

-    def has_object_permission(self, request: Request, view, obj: Model):
+    def has_permission(self, request: Request, view) -> bool:
+        """Always grant permission for object-specific requests
+        as view permission checking is done by `ObjectFilter`,
+        and write permission checking is done by `has_object_permission`"""
+        lookup = getattr(view, "lookup_url_kwarg", None) or getattr(view, "lookup_field", None)
+        if lookup and lookup in view.kwargs:
+            return True
+        return super().has_permission(request, view)
+
+    def has_object_permission(self, request: Request, view, obj: Model) -> bool:
        queryset = self._queryset(view)
        model_cls = queryset.model
        perms = self.get_required_object_permissions(request.method, model_cls)
--- a/authentik/rbac/tests/test_api_filters.py
+++ b/authentik/rbac/tests/test_api_filters.py
@ -121,3 +121,29 @@ class TestAPIPerms(APITestCase):
            },
        )
        self.assertEqual(res.status_code, 403)
+
+    def test_update_simple(self):
+        """Test update with permission"""
+        self.client.force_login(self.user)
+        inv = Invitation.objects.create(name=generate_id(), created_by=self.superuser)
+        self.role.assign_permission("authentik_stages_invitation.view_invitation", obj=inv)
+        self.role.assign_permission("authentik_stages_invitation.change_invitation", obj=inv)
+        res = self.client.patch(
+            reverse("authentik_api:invitation-detail", kwargs={"pk": inv.pk}),
+            data={
+                "name": generate_id(),
+            },
+        )
+        self.assertEqual(res.status_code, 200)
+
+    def test_update_simple_denied(self):
+        """Test update without assigning permission"""
+        self.client.force_login(self.user)
+        inv = Invitation.objects.create(name=generate_id(), created_by=self.superuser)
+        res = self.client.patch(
+            reverse("authentik_api:invitation-detail", kwargs={"pk": inv.pk}),
+            data={
+                "name": generate_id(),
+            },
+        )
+        self.assertEqual(res.status_code, 403)
--- a/web/src/admin/groups/RelatedUserList.ts
+++ b/web/src/admin/groups/RelatedUserList.ts
@ -165,6 +165,7 @@ export class RelatedUserList extends WithBrandConfig(WithCapabilitiesConfig(Tabl
        return html`<ak-forms-delete-bulk
            objectLabel=${msg("User(s)")}
            actionLabel=${msg("Remove Users(s)")}
+            action=${msg("removed")}
            actionSubtext=${msg(
                str`Are you sure you want to remove the selected users from the group ${this.targetGroup?.name}?`,
            )}
--- a/web/src/admin/roles/RoleAssignedGlobalPermissionsTable.ts
+++ b/web/src/admin/roles/RoleAssignedGlobalPermissionsTable.ts
@ -85,6 +85,10 @@ export class RoleAssignedGlobalPermissionsTable extends Table<Permission> {
    }

    row(item: Permission): TemplateResult[] {
-        return [html`${item.modelVerbose}`, html`${item.name}`, html`✓`];
+        return [
+            html`${item.modelVerbose}`,
+            html`${item.name}`,
+            html`<i class="fas fa-check pf-m-success"></i>`,
+        ];
    }
 }
--- a/web/src/admin/roles/RoleAssignedObjectPermissionTable.ts
+++ b/web/src/admin/roles/RoleAssignedObjectPermissionTable.ts
@ -90,7 +90,7 @@ export class RoleAssignedObjectPermissionTable extends Table<ExtraRoleObjectPerm
                  >
                      <pre>${item.objectPk}</pre>
                  </pf-tooltip>`}`,
-            html`✓`,
+            html`<i class="fas fa-check pf-m-success"></i>`,
        ];
    }
 }
--- a/web/src/admin/users/UserAssignedGlobalPermissionsTable.ts
+++ b/web/src/admin/users/UserAssignedGlobalPermissionsTable.ts
@ -84,6 +84,10 @@ export class UserAssignedGlobalPermissionsTable extends Table<Permission> {
    }

    row(item: Permission): TemplateResult[] {
-        return [html`${item.modelVerbose}`, html`${item.name}`, html`✓`];
+        return [
+            html`${item.modelVerbose}`,
+            html`${item.name}`,
+            html`<i class="fas fa-check pf-m-success"></i>`,
+        ];
    }
 }
--- a/web/src/admin/users/UserAssignedObjectPermissionsTable.ts
+++ b/web/src/admin/users/UserAssignedObjectPermissionsTable.ts
@ -86,7 +86,7 @@ export class UserAssignedObjectPermissionsTable extends Table<ExtraUserObjectPer
                  >
                      <pre>${item.objectPk}</pre>
                  </pf-tooltip>`}`,
-            html`✓`,
+            html`<i class="fas fa-check pf-m-success"></i>`,
        ];
    }
 }
--- a/web/src/elements/forms/DeleteBulkForm.ts
+++ b/web/src/elements/forms/DeleteBulkForm.ts
@ -134,6 +134,12 @@ export class DeleteBulkForm<T> extends ModalButton {
    @property()
    buttonLabel = msg("Delete");

+    /**
+     * Action shown in messages, for example `deleted` or `removed`
+     */
+    @property()
+    action = msg("deleted");
+
    @property({ attribute: false })
    metadata: (item: T) => BulkDeleteMetadata = (item: T) => {
        const rec = item as Record<string, unknown>;
--- a/web/src/elements/rbac/PermissionSelectModal.ts
+++ b/web/src/elements/rbac/PermissionSelectModal.ts
@ -67,7 +67,7 @@ export class PermissionSelectModal extends TableModal<Permission> {
    renderModalInner(): TemplateResult {
        return html`<section class="pf-c-modal-box__header pf-c-page__main-section pf-m-light">
                <div class="pf-c-content">
-                    <h1 class="pf-c-title pf-m-2xl">${msg("Select permissions to grant")}</h1>
+                    <h1 class="pf-c-title pf-m-2xl">${msg("Select permissions to assign")}</h1>
                </div>
            </section>
            <section class="pf-c-modal-box__body pf-m-light">${this.renderTable()}</section>
--- a/web/src/elements/rbac/RoleObjectPermissionTable.ts
+++ b/web/src/elements/rbac/RoleObjectPermissionTable.ts
@ -113,9 +113,9 @@ export class RoleAssignedObjectPermissionTable extends Table<RoleAssignedObjectP
            baseRow.push(
                html`${granted
                    ? html`<pf-tooltip position="top" content=${msg("Directly assigned")}
-                          >✓</pf-tooltip
-                      >`
-                    : html`X`} `,
+                          ><i class="fas fa-check pf-m-success"></i
+                      ></pf-tooltip>`
+                    : html`<i class="fas fa-times pf-m-danger"></i>`} `,
            );
        });
        return baseRow;
--- a/web/src/elements/rbac/UserObjectPermissionForm.ts
+++ b/web/src/elements/rbac/UserObjectPermissionForm.ts
@ -93,19 +93,24 @@ export class UserObjectPermissionForm extends ModelForm<UserAssignData, number>
                >
                </ak-search-select>
            </ak-form-element-horizontal>
-            ${this.modelPermissions?.results.map((perm) => {
-                return html` <ak-form-element-horizontal name="permissions.${perm.codename}">
-                    <label class="pf-c-switch">
-                        <input class="pf-c-switch__input" type="checkbox" />
-                        <span class="pf-c-switch__toggle">
-                            <span class="pf-c-switch__toggle-icon">
-                                <i class="fas fa-check" aria-hidden="true"></i>
+            ${this.modelPermissions?.results
+                .filter((perm) => {
+                    const [_app, model] = this.model?.split(".") || "";
+                    return perm.codename !== `add_${model}`;
+                })
+                .map((perm) => {
+                    return html` <ak-form-element-horizontal name="permissions.${perm.codename}">
+                        <label class="pf-c-switch">
+                            <input class="pf-c-switch__input" type="checkbox" />
+                            <span class="pf-c-switch__toggle">
+                                <span class="pf-c-switch__toggle-icon">
+                                    <i class="fas fa-check" aria-hidden="true"></i>
+                                </span>
                            </span>
-                        </span>
-                        <span class="pf-c-switch__label">${perm.name}</span>
-                    </label>
-                </ak-form-element-horizontal>`;
-            })}
+                            <span class="pf-c-switch__label">${perm.name}</span>
+                        </label>
+                    </ak-form-element-horizontal>`;
+                })}
        </form>`;
    }
 }
--- a/web/src/elements/rbac/UserObjectPermissionTable.ts
+++ b/web/src/elements/rbac/UserObjectPermissionTable.ts
@ -45,7 +45,7 @@ export class UserAssignedObjectPermissionTable extends Table<UserAssignedObjectP
            ordering: "codename",
        });
        modelPermissions.results = modelPermissions.results.filter((value) => {
-            return !value.codename.startsWith("add_");
+            return value.codename !== `add_${this.model?.split(".")[1]}`;
        });
        this.modelPermissions = modelPermissions;
        return perms;
@ -113,13 +113,15 @@ export class UserAssignedObjectPermissionTable extends Table<UserAssignedObjectP
    row(item: UserAssignedObjectPermission): TemplateResult[] {
        const baseRow = [html` <a href="#/identity/users/${item.pk}"> ${item.username} </a> `];
        this.modelPermissions?.results.forEach((perm) => {
-            let cell = html`X`;
+            let cell = html`<i class="fas fa-times pf-m-danger"></i>`;
            if (item.permissions.filter((uperm) => uperm.codename === perm.codename).length > 0) {
                cell = html`<pf-tooltip position="top" content=${msg("Directly assigned")}
-                    >✓</pf-tooltip
-                >`;
+                    ><i class="fas fa-check pf-m-success"></i
+                ></pf-tooltip>`;
            } else if (item.isSuperuser) {
-                cell = html`<pf-tooltip position="top" content=${msg("Superuser")}>✓</pf-tooltip>`;
+                cell = html`<pf-tooltip position="top" content=${msg("Superuser")}
+                    ><i class="fas fa-check pf-m-success"></i
+                ></pf-tooltip>`;
            }
            baseRow.push(cell);
        });
--- a/web/src/elements/table/Table.ts
+++ b/web/src/elements/table/Table.ts
@ -131,7 +131,7 @@ export abstract class Table<T> extends AKElement implements TableLike {
    order?: string;

    @property({ type: String })
-    search: string = getURLParam("search", "");
+    search: string = "";

    @property({ type: Boolean })
    checkbox = false;
@ -198,6 +198,9 @@ export abstract class Table<T> extends AKElement implements TableLike {
                this.selectedElements = [];
            }
        });
+        if (this.searchEnabled()) {
+            this.search = getURLParam("search", "");
+        }
    }

    public groupBy(items: T[]): [string, T[]][] {
--- a/website/docs/user-group-role/groups/manage_groups.md
+++ b/website/docs/user-group-role/groups/manage_groups.md
@ -34,12 +34,23 @@ To delete a group, follow these steps:
 2. Select the checkbox beside the name of the group that you want to delete.
 3. Click **Delete**.

-## Assign, modify, or remove permissions for a group
-
-You can grant a group specific global or object-level permissions. Any user who is a member of a group inherits all of the group's permissions.
-
-For more information, review ["Permissions"](../access-control/permissions.md).
-
 ## Assign a role to a group

 You can assign a role to a group, and then all users in the group inherit the permissions assigned to that role. For instructions and more information, see ["Assign a role to a group"](../roles/manage_roles.md#assign-a-role-to-a-group).
+
+## Delegating group member management
+
+:::info
+Requires authentik 2024.4
+:::
+
+To give a specific Role or User the ability to manage group members, the following permissions need to be granted on the matching Group object:
+
+-   Can view group
+-   Can add user to group
+-   Can remove user from group
+-   Can access admin interface (for managing a group's user within the authentik Admin interface)
+
+In addition, the permission "Can view User" needs to be assigned, either globally or on specific users that should be manageable.
+
+These permissions can be assigned to a [Role](../roles/index.mdx) or directly to a [User](../user/index.mdx).