From 3e818243882df55f9d80b980cb1ad46c351c83e1 Mon Sep 17 00:00:00 2001 From: Jens L Date: Tue, 26 Sep 2023 12:04:40 +0200 Subject: [PATCH] core: prevent self-impersonation (#6885) Signed-off-by: Jens Langhammer --- authentik/core/api/users.py | 4 ++- authentik/core/tests/test_impersonation.py | 33 +++++++++++++++++++++- web/src/admin/users/RelatedUserList.ts | 14 +++++++-- web/src/admin/users/UserListPage.ts | 19 +++++++++++-- web/src/admin/users/UserViewPage.ts | 32 +++++++++++++-------- 5 files changed, 84 insertions(+), 18 deletions(-) diff --git a/authentik/core/api/users.py b/authentik/core/api/users.py index 2029ff7a6..09a5dc553 100644 --- a/authentik/core/api/users.py +++ b/authentik/core/api/users.py @@ -616,8 +616,10 @@ class UserViewSet(UsedByMixin, ModelViewSet): if not request.user.has_perm("impersonate"): LOGGER.debug("User attempted to impersonate without permissions", user=request.user) return Response(status=401) - user_to_be = self.get_object() + if user_to_be.pk == self.request.user.pk: + LOGGER.debug("User attempted to impersonate themselves", user=request.user) + return Response(status=401) request.session[SESSION_KEY_IMPERSONATE_ORIGINAL_USER] = request.user request.session[SESSION_KEY_IMPERSONATE_USER] = user_to_be diff --git a/authentik/core/tests/test_impersonation.py b/authentik/core/tests/test_impersonation.py index 6d17393a8..6d07f4bbc 100644 --- a/authentik/core/tests/test_impersonation.py +++ b/authentik/core/tests/test_impersonation.py @@ -6,6 +6,7 @@ from rest_framework.test import APITestCase from authentik.core.models import User from authentik.core.tests.utils import create_test_admin_user +from authentik.lib.config import CONFIG class TestImpersonation(APITestCase): @@ -46,12 +47,42 @@ class TestImpersonation(APITestCase): """test impersonation without permissions""" self.client.force_login(self.other_user) - self.client.get(reverse("authentik_api:user-impersonate", kwargs={"pk": self.user.pk})) + response = self.client.post( + reverse("authentik_api:user-impersonate", kwargs={"pk": self.user.pk}) + ) + self.assertEqual(response.status_code, 403) response = self.client.get(reverse("authentik_api:user-me")) response_body = loads(response.content.decode()) self.assertEqual(response_body["user"]["username"], self.other_user.username) + @CONFIG.patch("impersonation", False) + def test_impersonate_disabled(self): + """test impersonation that is disabled""" + self.client.force_login(self.user) + + response = self.client.post( + reverse("authentik_api:user-impersonate", kwargs={"pk": self.other_user.pk}) + ) + self.assertEqual(response.status_code, 401) + + response = self.client.get(reverse("authentik_api:user-me")) + response_body = loads(response.content.decode()) + self.assertEqual(response_body["user"]["username"], self.user.username) + + def test_impersonate_self(self): + """test impersonation that user can't impersonate themselves""" + self.client.force_login(self.user) + + response = self.client.post( + reverse("authentik_api:user-impersonate", kwargs={"pk": self.user.pk}) + ) + self.assertEqual(response.status_code, 401) + + response = self.client.get(reverse("authentik_api:user-me")) + response_body = loads(response.content.decode()) + self.assertEqual(response_body["user"]["username"], self.user.username) + def test_un_impersonate_empty(self): """test un-impersonation without impersonating first""" self.client.force_login(self.other_user) diff --git a/web/src/admin/users/RelatedUserList.ts b/web/src/admin/users/RelatedUserList.ts index 7bd907bde..3ebc5dbb5 100644 --- a/web/src/admin/users/RelatedUserList.ts +++ b/web/src/admin/users/RelatedUserList.ts @@ -3,6 +3,7 @@ import "@goauthentik/admin/users/UserActiveForm"; import "@goauthentik/admin/users/UserForm"; import "@goauthentik/admin/users/UserPasswordForm"; import "@goauthentik/admin/users/UserResetEmailForm"; +import { me } from "@goauthentik/app/common/users"; import { DEFAULT_CONFIG } from "@goauthentik/common/api/config"; import { MessageLevel } from "@goauthentik/common/messages"; import { uiConfig } from "@goauthentik/common/ui/config"; @@ -37,6 +38,7 @@ import { CoreUsersListTypeEnum, Group, ResponseError, + SessionUser, User, } from "@goauthentik/api"; @@ -123,12 +125,15 @@ export class RelatedUserList extends Table { @property({ type: Boolean }) hideServiceAccounts = getURLParam("hideServiceAccounts", true); + @state() + me?: SessionUser; + static get styles(): CSSResult[] { return super.styles.concat(PFDescriptionList, PFAlert, PFBanner); } async apiEndpoint(page: number): Promise> { - return new CoreApi(DEFAULT_CONFIG).coreUsersList({ + const users = await new CoreApi(DEFAULT_CONFIG).coreUsersList({ ordering: this.order, page: page, pageSize: (await uiConfig()).pagination.perPage, @@ -138,6 +143,8 @@ export class RelatedUserList extends Table { ? [CoreUsersListTypeEnum.External, CoreUsersListTypeEnum.Internal] : undefined, }); + this.me = await me(); + return users; } columns(): TableColumn[] { @@ -181,6 +188,9 @@ export class RelatedUserList extends Table { } row(item: User): TemplateResult[] { + const canImpersonate = + rootInterface()?.config?.capabilities.includes(CapabilitiesEnum.CanImpersonate) && + item.pk !== this.me?.user.pk; return [ html`
${item.username}
@@ -200,7 +210,7 @@ export class RelatedUserList extends Table { - ${rootInterface()?.config?.capabilities.includes(CapabilitiesEnum.CanImpersonate) + ${canImpersonate ? html` { @@ -62,6 +70,9 @@ export class UserListPage extends TablePage { @state() userPaths?: UserPath; + @state() + me?: SessionUser; + static get styles(): CSSResult[] { return super.styles.concat(PFDescriptionList, PFCard, PFAlert); } @@ -88,6 +99,7 @@ export class UserListPage extends TablePage { this.userPaths = await new CoreApi(DEFAULT_CONFIG).coreUsersPathsRetrieve({ search: this.search, }); + this.me = await me(); return users; } @@ -179,6 +191,9 @@ export class UserListPage extends TablePage { } row(item: User): TemplateResult[] { + const canImpersonate = + rootInterface()?.config?.capabilities.includes(CapabilitiesEnum.CanImpersonate) && + item.pk !== this.me?.user.pk; return [ html`
${item.username}
@@ -198,7 +213,7 @@ export class UserListPage extends TablePage { - ${rootInterface()?.config?.capabilities.includes(CapabilitiesEnum.CanImpersonate) + ${canImpersonate ? html` { - this.user = user; - }); + me().then((me) => { + this.me = me; + new CoreApi(DEFAULT_CONFIG) + .coreUsersRetrieve({ + id: id, + }) + .then((user) => { + this.user = user; + }); + }); } @property({ attribute: false }) user?: User; + @state() + me?: SessionUser; + static get styles(): CSSResult[] { return [ PFBase, @@ -103,6 +110,9 @@ export class UserViewPage extends AKElement { if (!this.user) { return html``; } + const canImpersonate = + rootInterface()?.config?.capabilities.includes(CapabilitiesEnum.CanImpersonate) && + this.user.pk !== this.me?.user.pk; return html`
${msg("User Info")}
@@ -213,9 +223,7 @@ export class UserViewPage extends AKElement { - ${rootInterface()?.config?.capabilities.includes( - CapabilitiesEnum.CanImpersonate, - ) + ${canImpersonate ? html`