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) <[email protected]>
This commit is contained in:
1 parent
e0bd7b2095
commit
c4a11b4704
3 files changed
+222
-3
No files matched your search
@@ -22,12 +22,14 @@ describe('UsersService', () => {
|
|||||||
findUnique: jest.Mock;
|
findUnique: jest.Mock;
|
||||||
};
|
};
|
||||||
refreshToken: { updateMany: jest.Mock };
|
refreshToken: { updateMany: jest.Mock };
|
||||||
|
rolePermission: { findMany: jest.Mock };
|
||||||
};
|
};
|
||||||
let auditLog: { record: jest.Mock };
|
let auditLog: { record: jest.Mock };
|
||||||
let permissionResolver: {
|
let permissionResolver: {
|
||||||
invalidateUser: jest.Mock;
|
invalidateUser: jest.Mock;
|
||||||
invalidateAll: jest.Mock;
|
invalidateAll: jest.Mock;
|
||||||
hasAny: jest.Mock;
|
hasAny: jest.Mock;
|
||||||
|
getPermissions: jest.Mock;
|
||||||
};
|
};
|
||||||
|
|
||||||
const staffRole = {
|
const staffRole = {
|
||||||
@@ -79,12 +81,14 @@ describe('UsersService', () => {
|
|||||||
findUnique: jest.fn().mockResolvedValue(staffRole),
|
findUnique: jest.fn().mockResolvedValue(staffRole),
|
||||||
},
|
},
|
||||||
refreshToken: { updateMany: jest.fn() },
|
refreshToken: { updateMany: jest.fn() },
|
||||||
|
rolePermission: { findMany: jest.fn().mockResolvedValue([]) },
|
||||||
};
|
};
|
||||||
auditLog = { record: jest.fn().mockResolvedValue(undefined) };
|
auditLog = { record: jest.fn().mockResolvedValue(undefined) };
|
||||||
permissionResolver = {
|
permissionResolver = {
|
||||||
invalidateUser: jest.fn(),
|
invalidateUser: jest.fn(),
|
||||||
invalidateAll: jest.fn(),
|
invalidateAll: jest.fn(),
|
||||||
hasAny: jest.fn().mockResolvedValue(true),
|
hasAny: jest.fn().mockResolvedValue(true),
|
||||||
|
getPermissions: jest.fn().mockResolvedValue(new Set<string>()),
|
||||||
};
|
};
|
||||||
usersService = new UsersService(
|
usersService = new UsersService(
|
||||||
prisma as unknown as PrismaService,
|
prisma as unknown as PrismaService,
|
||||||
@@ -430,4 +434,23 @@ describe('UsersService', () => {
|
|||||||
expect(prisma.user.update).not.toHaveBeenCalled();
|
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();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
@@ -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<void> {
|
||||||
|
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<void> {
|
private async assertActiveRole(roleId: string): Promise<void> {
|
||||||
const role = await this.prisma.role.findUnique({ where: { id: roleId } });
|
const role = await this.prisma.role.findUnique({ where: { id: roleId } });
|
||||||
if (!role) {
|
if (!role) {
|
||||||
@@ -151,6 +183,7 @@ export class UsersService {
|
|||||||
}
|
}
|
||||||
await this.assertActiveRole(dto.roleId);
|
await this.assertActiveRole(dto.roleId);
|
||||||
await this.assertCanAssignRole(actingUserId);
|
await this.assertCanAssignRole(actingUserId);
|
||||||
|
await this.assertWithinCallerPermissions(dto.roleId, actingUserId);
|
||||||
const passwordHash = await bcrypt.hash(dto.password, BCRYPT_SALT_ROUNDS);
|
const passwordHash = await bcrypt.hash(dto.password, BCRYPT_SALT_ROUNDS);
|
||||||
const user = await this.prisma.user.create({
|
const user = await this.prisma.user.create({
|
||||||
data: {
|
data: {
|
||||||
@@ -177,7 +210,9 @@ export class UsersService {
|
|||||||
actingUserId?: string,
|
actingUserId?: string,
|
||||||
): Promise<UserSummary> {
|
): Promise<UserSummary> {
|
||||||
const before = await this.findById(id);
|
const before = await this.findById(id);
|
||||||
|
await this.assertWithinCallerPermissions(before.roleId, actingUserId);
|
||||||
const roleChanged = Boolean(dto.roleId) && dto.roleId !== before.roleId;
|
const roleChanged = Boolean(dto.roleId) && dto.roleId !== before.roleId;
|
||||||
|
const emailChanged = dto.email !== undefined && dto.email !== before.email;
|
||||||
if (dto.roleId) {
|
if (dto.roleId) {
|
||||||
await this.assertActiveRole(dto.roleId);
|
await this.assertActiveRole(dto.roleId);
|
||||||
}
|
}
|
||||||
@@ -189,6 +224,7 @@ export class UsersService {
|
|||||||
throw new ForbiddenException('Cannot change your own role');
|
throw new ForbiddenException('Cannot change your own role');
|
||||||
}
|
}
|
||||||
await this.assertCanAssignRole(actingUserId);
|
await this.assertCanAssignRole(actingUserId);
|
||||||
|
await this.assertWithinCallerPermissions(dto.roleId!, actingUserId);
|
||||||
await this.assertNotLastSystemRoleUser(id);
|
await this.assertNotLastSystemRoleUser(id);
|
||||||
}
|
}
|
||||||
const user = await this.prisma.user.update({
|
const user = await this.prisma.user.update({
|
||||||
@@ -200,7 +236,15 @@ export class UsersService {
|
|||||||
},
|
},
|
||||||
...USER_WITH_ROLE,
|
...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);
|
this.permissionResolver.invalidateUser(id);
|
||||||
}
|
}
|
||||||
await this.auditLog.record({
|
await this.auditLog.record({
|
||||||
@@ -216,6 +260,7 @@ export class UsersService {
|
|||||||
|
|
||||||
async deactivate(id: string, actingUserId?: string): Promise<UserSummary> {
|
async deactivate(id: string, actingUserId?: string): Promise<UserSummary> {
|
||||||
const before = await this.findById(id);
|
const before = await this.findById(id);
|
||||||
|
await this.assertWithinCallerPermissions(before.roleId, actingUserId);
|
||||||
await this.assertNotLastSystemRoleUser(id);
|
await this.assertNotLastSystemRoleUser(id);
|
||||||
const user = await this.prisma.user.update({
|
const user = await this.prisma.user.update({
|
||||||
where: { id },
|
where: { id },
|
||||||
@@ -262,7 +307,8 @@ export class UsersService {
|
|||||||
'Use /profile/password to change your own password',
|
'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 passwordHash = await bcrypt.hash(dto.password, BCRYPT_SALT_ROUNDS);
|
||||||
const user = await this.prisma.user.update({
|
const user = await this.prisma.user.update({
|
||||||
where: { id },
|
where: { id },
|
||||||
@@ -291,7 +337,8 @@ export class UsersService {
|
|||||||
'Use /profile/mfa/disable to disable your own MFA',
|
'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({
|
const user = await this.prisma.user.update({
|
||||||
where: { id },
|
where: { id },
|
||||||
data: { mfaEnabled: false, mfaSecret: null },
|
data: { mfaEnabled: false, mfaSecret: null },
|
||||||
@@ -308,6 +355,7 @@ export class UsersService {
|
|||||||
|
|
||||||
async activate(id: string, actingUserId?: string): Promise<UserSummary> {
|
async activate(id: string, actingUserId?: string): Promise<UserSummary> {
|
||||||
const before = await this.findById(id);
|
const before = await this.findById(id);
|
||||||
|
await this.assertWithinCallerPermissions(before.roleId, actingUserId);
|
||||||
if (before.isActive) {
|
if (before.isActive) {
|
||||||
throw new ConflictException('User is already active');
|
throw new ConflictException('User is already active');
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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<App>;
|
||||||
|
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);
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in new issue
Block a user