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>
goauthentik · Apr 15, 2024 · 4a9c95b · 4a9c95b
1 parent bcc8d5e
commit 4a9c95b
Show file tree

Hide file tree

Showing 18 changed files with 159 additions and 52 deletions.
diff --git a/authentik/core/api/groups.py b/authentik/core/api/groups.py
@@ -146,15 +146,14 @@ class Meta:
         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,15 +168,21 @@ 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={
             204: OpenApiResponse(description="User added"),
             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,15 +198,21 @@ def add_user(self, request: Request, pk: str) -> Response:
         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={
             204: OpenApiResponse(description="User added"),
             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()

diff --git a/...p_authentik_c_name_9ba8e4_idx_and_more.py → ...ions/0035_alter_group_options_and_more.py b/...p_authentik_c_name_9ba8e4_idx_and_more.py → ...ions/0035_alter_group_options_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"),

diff --git a/authentik/core/models.py b/authentik/core/models.py
@@ -188,6 +188,10 @@ class Meta:
         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):

diff --git a/authentik/core/tests/test_groups_api.py b/authentik/core/tests/test_groups_api.py
@@ -1,24 +1,27 @@
 """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
 
 
 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 @@ def test_add_user(self):
     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 @@ def test_add_user_404(self):
     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 @@ def test_remove_user(self):
     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 @@ def test_remove_user_404(self):
     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,
             },
         )

diff --git a/authentik/rbac/permissions.py b/authentik/rbac/permissions.py
@@ -8,7 +8,16 @@
 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)

diff --git a/authentik/rbac/tests/test_api_filters.py b/authentik/rbac/tests/test_api_filters.py
@@ -121,3 +121,29 @@ def test_create_simple_denied(self):
             },
         )
         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)
diff --git 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}?`,
             )}

diff --git 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>`,
+        ];
     }
 }
diff --git 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>`,
         ];
     }
 }
diff --git 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>`,
+        ];
     }
 }
diff --git 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>`,
         ];
     }
 }
diff --git 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>;

diff --git 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>

diff --git 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;

diff --git 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>`;
     }
 }
diff --git 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);
         });