From c4a11b4704265994f5d5896a409e6de0c6491cd4 Mon Sep 17 00:00:00 2001 From: Nick Szaunig Date: Thu, 1 Oct 2026 01:16:37 +0200 Subject: [PATCH] fix(users): never manage or hand out more than the caller holds A role with users:update could reset an administrator's password or MFA, change the admin's email to one it controls, or give a colluding account the ADMIN role (with users:assign-role) - and so become administrator. Every management action (update, deactivate, activate, reset password, reset MFA) and every role assignment now requires the target role's grants to be a subset of the caller's own, like role editing already did. The target role is read from its grants, so a deactivated admin still counts as an admin. Changing another user's email revokes their sessions. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/users/users.service.spec.ts | 23 +++ ApiCore-Backend/src/users/users.service.ts | 54 ++++++- .../test/user-escalation.e2e-spec.ts | 148 ++++++++++++++++++ 3 files changed, 222 insertions(+), 3 deletions(-) create mode 100644 ApiCore-Backend/test/user-escalation.e2e-spec.ts diff --git a/ApiCore-Backend/src/users/users.service.spec.ts b/ApiCore-Backend/src/users/users.service.spec.ts index e73e938..38e2b3b 100644 --- a/ApiCore-Backend/src/users/users.service.spec.ts +++ b/ApiCore-Backend/src/users/users.service.spec.ts @@ -22,12 +22,14 @@ describe('UsersService', () => { findUnique: jest.Mock; }; refreshToken: { updateMany: jest.Mock }; + rolePermission: { findMany: jest.Mock }; }; let auditLog: { record: jest.Mock }; let permissionResolver: { invalidateUser: jest.Mock; invalidateAll: jest.Mock; hasAny: jest.Mock; + getPermissions: jest.Mock; }; const staffRole = { @@ -79,12 +81,14 @@ describe('UsersService', () => { findUnique: jest.fn().mockResolvedValue(staffRole), }, refreshToken: { updateMany: jest.fn() }, + rolePermission: { findMany: jest.fn().mockResolvedValue([]) }, }; auditLog = { record: jest.fn().mockResolvedValue(undefined) }; permissionResolver = { invalidateUser: jest.fn(), invalidateAll: jest.fn(), hasAny: jest.fn().mockResolvedValue(true), + getPermissions: jest.fn().mockResolvedValue(new Set()), }; usersService = new UsersService( prisma as unknown as PrismaService, @@ -430,4 +434,23 @@ describe('UsersService', () => { expect(prisma.user.update).not.toHaveBeenCalled(); }); }); + + it('refuses to reset the password of an account with more permissions than the caller', async () => { + prisma.user.findUnique.mockResolvedValue(adminUser); + prisma.rolePermission.findMany.mockResolvedValue([ + { permission: { key: 'backup:import' } }, + ]); + permissionResolver.getPermissions.mockResolvedValue( + new Set(['users:update']), + ); + + await expect( + usersService.resetPassword( + adminUser.id, + { password: 'Hijacked-Password-1!' }, + existingUser.id, + ), + ).rejects.toBeInstanceOf(ForbiddenException); + expect(prisma.user.update).not.toHaveBeenCalled(); + }); }); diff --git a/ApiCore-Backend/src/users/users.service.ts b/ApiCore-Backend/src/users/users.service.ts index 9409ef5..86d6166 100644 --- a/ApiCore-Backend/src/users/users.service.ts +++ b/ApiCore-Backend/src/users/users.service.ts @@ -78,6 +78,38 @@ export class UsersService { } } + /** + * Nobody may act on - or hand out - more than they can do themselves: a + * limited "user manager" holding users:update could otherwise reset an + * administrator's password or MFA, change their email to one they control, + * or give a colluding account the ADMIN role, and so become admin. Same + * rule as RolesService.assertCanGrantPermissions. Reads the role's grants + * directly (not the cached effective set), so a deactivated admin counts + * as an admin too. + */ + private async assertWithinCallerPermissions( + roleId: string, + actingUserId?: string, + ): Promise { + if (!actingUserId) { + return; + } + const grants = await this.prisma.rolePermission.findMany({ + where: { roleId }, + select: { permission: { select: { key: true } } }, + }); + const callerPermissions = + await this.permissionResolver.getPermissions(actingUserId); + const exceeds = grants.some( + (grant) => !callerPermissions.has(grant.permission.key), + ); + if (exceeds) { + throw new ForbiddenException( + 'Dieses Konto hat mehr Rechte als Sie selbst und kann von Ihnen nicht verwaltet werden', + ); + } + } + private async assertActiveRole(roleId: string): Promise { const role = await this.prisma.role.findUnique({ where: { id: roleId } }); if (!role) { @@ -151,6 +183,7 @@ export class UsersService { } await this.assertActiveRole(dto.roleId); await this.assertCanAssignRole(actingUserId); + await this.assertWithinCallerPermissions(dto.roleId, actingUserId); const passwordHash = await bcrypt.hash(dto.password, BCRYPT_SALT_ROUNDS); const user = await this.prisma.user.create({ data: { @@ -177,7 +210,9 @@ export class UsersService { actingUserId?: string, ): Promise { const before = await this.findById(id); + await this.assertWithinCallerPermissions(before.roleId, actingUserId); const roleChanged = Boolean(dto.roleId) && dto.roleId !== before.roleId; + const emailChanged = dto.email !== undefined && dto.email !== before.email; if (dto.roleId) { await this.assertActiveRole(dto.roleId); } @@ -189,6 +224,7 @@ export class UsersService { throw new ForbiddenException('Cannot change your own role'); } await this.assertCanAssignRole(actingUserId); + await this.assertWithinCallerPermissions(dto.roleId!, actingUserId); await this.assertNotLastSystemRoleUser(id); } const user = await this.prisma.user.update({ @@ -200,7 +236,15 @@ export class UsersService { }, ...USER_WITH_ROLE, }); - if (roleChanged) { + if (emailChanged) { + // The email is the login name and the SSO link: sessions opened under + // the old one end here. + await this.prisma.refreshToken.updateMany({ + where: { userId: id, revokedAt: null }, + data: { revokedAt: new Date() }, + }); + } + if (roleChanged || emailChanged) { this.permissionResolver.invalidateUser(id); } await this.auditLog.record({ @@ -216,6 +260,7 @@ export class UsersService { async deactivate(id: string, actingUserId?: string): Promise { const before = await this.findById(id); + await this.assertWithinCallerPermissions(before.roleId, actingUserId); await this.assertNotLastSystemRoleUser(id); const user = await this.prisma.user.update({ where: { id }, @@ -262,7 +307,8 @@ export class UsersService { 'Use /profile/password to change your own password', ); } - await this.findById(id); + const target = await this.findById(id); + await this.assertWithinCallerPermissions(target.roleId, actingUserId); const passwordHash = await bcrypt.hash(dto.password, BCRYPT_SALT_ROUNDS); const user = await this.prisma.user.update({ where: { id }, @@ -291,7 +337,8 @@ export class UsersService { 'Use /profile/mfa/disable to disable your own MFA', ); } - await this.findById(id); + const target = await this.findById(id); + await this.assertWithinCallerPermissions(target.roleId, actingUserId); const user = await this.prisma.user.update({ where: { id }, data: { mfaEnabled: false, mfaSecret: null }, @@ -308,6 +355,7 @@ export class UsersService { async activate(id: string, actingUserId?: string): Promise { const before = await this.findById(id); + await this.assertWithinCallerPermissions(before.roleId, actingUserId); if (before.isActive) { throw new ConflictException('User is already active'); } diff --git a/ApiCore-Backend/test/user-escalation.e2e-spec.ts b/ApiCore-Backend/test/user-escalation.e2e-spec.ts new file mode 100644 index 0000000..948b34c --- /dev/null +++ b/ApiCore-Backend/test/user-escalation.e2e-spec.ts @@ -0,0 +1,148 @@ +import { INestApplication } from '@nestjs/common'; +import * as bcrypt from 'bcryptjs'; +import request from 'supertest'; +import { App } from 'supertest/types'; +import { PrismaService } from '../src/prisma/prisma.service'; +import { createTestApp } from './utils/create-test-app'; +import { login } from './utils/api'; + +/** + * A limited "user manager" may look after ordinary accounts but must never + * take over - or hand out - an account that can do more than they can. + */ +describe('User management cannot escalate (e2e)', () => { + let app: INestApplication; + let prisma: PrismaService; + const suffix = `${Date.now()}-${Math.round(Math.random() * 1e6)}`; + const password = 'SuperSecret123!'; + let managerToken: string; + let adminId: string; + let beekeeperId: string; + let adminRoleId: string; + + const server = () => app.getHttpServer(); + const asManager = (req: request.Test) => + req.set('Authorization', `Bearer ${managerToken}`); + + beforeAll(async () => { + app = await createTestApp(); + prisma = app.get(PrismaService); + const passwordHash = await bcrypt.hash(password, 12); + + const managerRole = await prisma.role.create({ + data: { + key: `USER_MANAGER_${suffix}`, + name: 'Benutzerpflege', + permissions: { + create: [ + 'users:read', + 'users:create', + 'users:update', + 'users:deactivate', + 'users:assign-role', + ].map((key) => ({ permission: { connect: { key } } })), + }, + }, + }); + await prisma.user.create({ + data: { + email: `manager-${suffix}@example.com`, + passwordHash, + name: 'Manager', + roleId: managerRole.id, + }, + }); + const admin = await prisma.user.create({ + data: { + email: `victim-admin-${suffix}@example.com`, + passwordHash, + name: 'Admin', + role: { connect: { key: 'ADMIN' } }, + }, + }); + adminId = admin.id; + adminRoleId = admin.roleId; + // A second admin, so "last admin" protection is not what blocks us. + await prisma.user.create({ + data: { + email: `other-admin-${suffix}@example.com`, + passwordHash, + name: 'Admin 2', + role: { connect: { key: 'ADMIN' } }, + }, + }); + const narrowRole = await prisma.role.create({ + data: { + key: `USER_READER_${suffix}`, + name: 'Benutzer ansehen', + permissions: { + create: [{ permission: { connect: { key: 'users:read' } } }], + }, + }, + }); + const narrow = await prisma.user.create({ + data: { + email: `narrow-${suffix}@example.com`, + passwordHash, + name: 'Leser', + roleId: narrowRole.id, + }, + }); + beekeeperId = narrow.id; + + managerToken = (await login(app, `manager-${suffix}@example.com`, password)) + .accessToken; + }); + + afterAll(async () => { + await app.close(); + }); + + it('cannot reset the password of a more privileged user', async () => { + await asManager( + request(server()).post(`/api/v1/users/${adminId}/reset-password`), + ) + .send({ password: 'Hijacked-Password-1!' }) + .expect(403); + }); + + it('cannot reset the MFA of a more privileged user', async () => { + await asManager( + request(server()).post(`/api/v1/users/${adminId}/mfa/reset`), + ).expect(403); + }); + + it('cannot change the email of a more privileged user', async () => { + await asManager(request(server()).patch(`/api/v1/users/${adminId}`)) + .send({ email: `attacker-${suffix}@example.com` }) + .expect(403); + }); + + it('cannot deactivate a more privileged user', async () => { + await asManager( + request(server()).post(`/api/v1/users/${adminId}/deactivate`), + ).expect(403); + }); + + it('cannot hand out a role with more permissions than they hold', async () => { + await asManager(request(server()).patch(`/api/v1/users/${beekeeperId}`)) + .send({ roleId: adminRoleId }) + .expect(403); + await asManager(request(server()).post('/api/v1/users')) + .send({ + email: `new-admin-${suffix}@example.com`, + password: 'AnotherSecret123!', + name: 'Neuer Admin', + roleId: adminRoleId, + }) + .expect(403); + }); + + it('can still look after a less privileged user', async () => { + await asManager( + request(server()).post(`/api/v1/users/${beekeeperId}/reset-password`), + ) + .send({ password: 'Fresh-Password-12!' }) + .expect(201); + }); +});