fix: Rechteausweitung ueber Rollen-Rechtezuweisung geschlossen
RolesService.create/update uebernahm eine explizit uebergebene permissionKeys-Liste ungeprueft. Ein Nutzer mit nur roles:update konnte damit JEDER Rolle - auch der eigenen, ohne users:assign-role - beliebige weitere Rechte (bis hin zu allen 92 Katalog-Eintraegen) hinzufuegen, da das Bearbeiten der eigenen Rollen-Rechte nicht ueber User.roleId laeuft und daher vom bereits gefixten users:assign-role-Check nicht erfasst wurde. Neue Regel (Standard-RBAC-Prinzip "keine Rechte ueber das eigene Rechte-Set hinaus vergeben"): create/update pruefen jetzt, dass der aufrufende Nutzer jedes zu vergebende Recht selbst bereits besitzt, bevor die Rolle angelegt/geaendert wird. roles.service.ts hatte bislang keine Unit-Tests; neue Spec-Datei deckt den Fix sowie die uebrigen Methoden (create/update/deactivate/ reactivate) ab. Co-Authored-By: Claude Sonnet 5 <[email protected]>
This commit is contained in:
Nick SzaunigandClaude Sonnet 5 committed
1 parent
8b8616ce03
commit
f57cffe6a8
2 files changed
+317
No files matched your search
@@ -0,0 +1,288 @@
|
||||
import {
|
||||
ConflictException,
|
||||
ForbiddenException,
|
||||
NotFoundException,
|
||||
} from '@nestjs/common';
|
||||
import { RolesService } from './roles.service';
|
||||
import { PrismaService } from '../prisma/prisma.service';
|
||||
import { AuditLogService } from '../audit/audit-log.service';
|
||||
import { PermissionResolverService } from '../auth/permissions/permission-resolver.service';
|
||||
|
||||
describe('RolesService', () => {
|
||||
let service: RolesService;
|
||||
let tx: {
|
||||
role: {
|
||||
update: jest.Mock;
|
||||
findUniqueOrThrow: jest.Mock;
|
||||
create: jest.Mock;
|
||||
};
|
||||
rolePermission: { deleteMany: jest.Mock; createMany: jest.Mock };
|
||||
permission: { findMany: jest.Mock };
|
||||
};
|
||||
let prisma: {
|
||||
role: { findUnique: jest.Mock; findMany: jest.Mock; update: jest.Mock };
|
||||
$transaction: jest.Mock;
|
||||
};
|
||||
let auditLog: { record: jest.Mock };
|
||||
let permissionResolver: {
|
||||
invalidateAll: jest.Mock;
|
||||
getPermissions: jest.Mock;
|
||||
};
|
||||
|
||||
const staffRole = {
|
||||
id: 'role-staff',
|
||||
key: 'STAFF',
|
||||
name: 'Mitarbeiter',
|
||||
description: null,
|
||||
isSystem: false,
|
||||
status: 'ACTIVE',
|
||||
createdAt: new Date('2026-01-01'),
|
||||
updatedAt: new Date('2026-01-01'),
|
||||
deactivatedAt: null as Date | null,
|
||||
permissions: [{ permission: { key: 'apiaries:read' } }],
|
||||
_count: { users: 0 },
|
||||
};
|
||||
|
||||
const adminRole = {
|
||||
...staffRole,
|
||||
id: 'role-admin',
|
||||
key: 'ADMIN',
|
||||
isSystem: true,
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
tx = {
|
||||
role: {
|
||||
update: jest.fn().mockResolvedValue({}),
|
||||
findUniqueOrThrow: jest.fn(),
|
||||
create: jest.fn(),
|
||||
},
|
||||
rolePermission: {
|
||||
deleteMany: jest.fn().mockResolvedValue({}),
|
||||
createMany: jest.fn().mockResolvedValue({}),
|
||||
},
|
||||
permission: {
|
||||
findMany: jest.fn().mockResolvedValue([{ id: 'perm-1' }]),
|
||||
},
|
||||
};
|
||||
prisma = {
|
||||
role: {
|
||||
findUnique: jest.fn(),
|
||||
findMany: jest.fn(),
|
||||
update: jest.fn(),
|
||||
},
|
||||
$transaction: jest.fn((callback: (txArg: typeof tx) => unknown) =>
|
||||
callback(tx),
|
||||
),
|
||||
};
|
||||
auditLog = { record: jest.fn().mockResolvedValue(undefined) };
|
||||
permissionResolver = {
|
||||
invalidateAll: jest.fn(),
|
||||
getPermissions: jest.fn().mockResolvedValue(new Set(['apiaries:read'])),
|
||||
};
|
||||
service = new RolesService(
|
||||
prisma as unknown as PrismaService,
|
||||
auditLog as unknown as AuditLogService,
|
||||
permissionResolver as unknown as PermissionResolverService,
|
||||
);
|
||||
});
|
||||
|
||||
describe('findById', () => {
|
||||
it('throws NotFoundException when the role does not exist', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue(null);
|
||||
await expect(service.findById('missing')).rejects.toBeInstanceOf(
|
||||
NotFoundException,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('create', () => {
|
||||
it('throws ConflictException when the key is already in use', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue(staffRole);
|
||||
await expect(
|
||||
service.create(
|
||||
{ key: 'STAFF', name: 'x', permissionKeys: [] },
|
||||
'user-1',
|
||||
),
|
||||
).rejects.toBeInstanceOf(ConflictException);
|
||||
});
|
||||
|
||||
it('throws ForbiddenException when granting a permission the caller does not hold', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue(null);
|
||||
permissionResolver.getPermissions.mockResolvedValue(
|
||||
new Set(['apiaries:read']),
|
||||
);
|
||||
|
||||
await expect(
|
||||
service.create(
|
||||
{
|
||||
key: 'MANAGER',
|
||||
name: 'Manager',
|
||||
permissionKeys: ['apiaries:read', 'users:assign-role'],
|
||||
},
|
||||
'user-1',
|
||||
),
|
||||
).rejects.toBeInstanceOf(ForbiddenException);
|
||||
expect(tx.rolePermission.createMany).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('creates the role and grants only permissions the caller already holds', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue(null);
|
||||
permissionResolver.getPermissions.mockResolvedValue(
|
||||
new Set(['apiaries:read']),
|
||||
);
|
||||
tx.role.create.mockResolvedValue(staffRole);
|
||||
tx.role.findUniqueOrThrow.mockResolvedValue(staffRole);
|
||||
|
||||
const result = await service.create(
|
||||
{
|
||||
key: 'STAFF',
|
||||
name: 'Mitarbeiter',
|
||||
permissionKeys: ['apiaries:read'],
|
||||
},
|
||||
'user-1',
|
||||
);
|
||||
|
||||
expect(result.key).toBe('STAFF');
|
||||
expect(permissionResolver.invalidateAll).toHaveBeenCalled();
|
||||
expect(auditLog.record).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ entityType: 'Role', action: 'CREATE' }),
|
||||
);
|
||||
});
|
||||
|
||||
it('skips the permission-holding check when there is no acting user', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue(null);
|
||||
tx.role.create.mockResolvedValue(staffRole);
|
||||
tx.role.findUniqueOrThrow.mockResolvedValue(staffRole);
|
||||
|
||||
await expect(
|
||||
service.create({
|
||||
key: 'STAFF',
|
||||
name: 'Mitarbeiter',
|
||||
permissionKeys: ['users:assign-role'],
|
||||
}),
|
||||
).resolves.toBeDefined();
|
||||
expect(permissionResolver.getPermissions).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('update', () => {
|
||||
it('throws ForbiddenException when the role is a system role', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue(adminRole);
|
||||
await expect(
|
||||
service.update('role-admin', { name: 'x' }, 'user-1'),
|
||||
).rejects.toBeInstanceOf(ForbiddenException);
|
||||
});
|
||||
|
||||
it('throws ForbiddenException when granting a permission the caller does not hold, even for their own role', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue(staffRole);
|
||||
permissionResolver.getPermissions.mockResolvedValue(
|
||||
new Set(['apiaries:read']),
|
||||
);
|
||||
|
||||
await expect(
|
||||
service.update(
|
||||
'role-staff',
|
||||
{
|
||||
permissionKeys: [
|
||||
'apiaries:read',
|
||||
'roles:update',
|
||||
'users:assign-role',
|
||||
],
|
||||
},
|
||||
'user-1',
|
||||
),
|
||||
).rejects.toBeInstanceOf(ForbiddenException);
|
||||
expect(tx.role.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('updates name/description without triggering the permission check when permissionKeys is absent', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue(staffRole);
|
||||
tx.role.findUniqueOrThrow.mockResolvedValue(staffRole);
|
||||
|
||||
await service.update('role-staff', { name: 'Neuer Name' }, 'user-1');
|
||||
|
||||
expect(permissionResolver.getPermissions).not.toHaveBeenCalled();
|
||||
expect(tx.rolePermission.deleteMany).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('replaces permissions and invalidates the cache when the caller holds everything granted', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue(staffRole);
|
||||
permissionResolver.getPermissions.mockResolvedValue(
|
||||
new Set(['apiaries:read']),
|
||||
);
|
||||
tx.role.findUniqueOrThrow.mockResolvedValue(staffRole);
|
||||
|
||||
const result = await service.update(
|
||||
'role-staff',
|
||||
{ permissionKeys: ['apiaries:read'] },
|
||||
'user-1',
|
||||
);
|
||||
|
||||
expect(result.key).toBe('STAFF');
|
||||
expect(tx.rolePermission.deleteMany).toHaveBeenCalledWith({
|
||||
where: { roleId: 'role-staff' },
|
||||
});
|
||||
expect(permissionResolver.invalidateAll).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('deactivate', () => {
|
||||
it('throws ForbiddenException for a system role', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue(adminRole);
|
||||
await expect(
|
||||
service.deactivate('role-admin', 'user-1'),
|
||||
).rejects.toBeInstanceOf(ForbiddenException);
|
||||
});
|
||||
|
||||
it('throws ConflictException when users are still assigned', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue({
|
||||
...staffRole,
|
||||
_count: { users: 2 },
|
||||
});
|
||||
await expect(
|
||||
service.deactivate('role-staff', 'user-1'),
|
||||
).rejects.toBeInstanceOf(ConflictException);
|
||||
});
|
||||
|
||||
it('deactivates an unused role', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue(staffRole);
|
||||
prisma.role.update.mockResolvedValue({
|
||||
...staffRole,
|
||||
status: 'INACTIVE',
|
||||
deactivatedAt: new Date(),
|
||||
});
|
||||
|
||||
const result = await service.deactivate('role-staff', 'user-1');
|
||||
|
||||
expect(result.status).toBe('INACTIVE');
|
||||
expect(permissionResolver.invalidateAll).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('reactivate', () => {
|
||||
it('throws ConflictException when already active', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue(staffRole);
|
||||
await expect(
|
||||
service.reactivate('role-staff', 'user-1'),
|
||||
).rejects.toBeInstanceOf(ConflictException);
|
||||
});
|
||||
|
||||
it('reactivates an inactive role', async () => {
|
||||
prisma.role.findUnique.mockResolvedValue({
|
||||
...staffRole,
|
||||
status: 'INACTIVE',
|
||||
deactivatedAt: new Date(),
|
||||
});
|
||||
prisma.role.update.mockResolvedValue({
|
||||
...staffRole,
|
||||
status: 'ACTIVE',
|
||||
deactivatedAt: null,
|
||||
});
|
||||
|
||||
const result = await service.reactivate('role-staff', 'user-1');
|
||||
|
||||
expect(result.status).toBe('ACTIVE');
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -81,6 +81,31 @@ export class RolesService {
|
||||
return this.toSummary(await this.findById(id));
|
||||
}
|
||||
|
||||
/**
|
||||
* Nobody can grant a role a permission they do not hold themselves - a
|
||||
* caller with only roles:update could otherwise turn any role (including
|
||||
* one they belong to) into a shadow-admin by adding every catalog key, no
|
||||
* users:assign-role needed at all since editing a role's own grants
|
||||
* doesn't touch User.roleId. This is the standard "can't escalate beyond
|
||||
* what you have" RBAC rule.
|
||||
*/
|
||||
private async assertCanGrantPermissions(
|
||||
permissionKeys: string[],
|
||||
actingUserId?: string,
|
||||
): Promise<void> {
|
||||
if (!actingUserId) {
|
||||
return;
|
||||
}
|
||||
const callerPermissions =
|
||||
await this.permissionResolver.getPermissions(actingUserId);
|
||||
const missing = permissionKeys.filter((key) => !callerPermissions.has(key));
|
||||
if (missing.length > 0) {
|
||||
throw new ForbiddenException(
|
||||
`Cannot grant permissions you do not hold yourself: ${missing.join(', ')}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
async create(
|
||||
dto: CreateRoleDto,
|
||||
actingUserId?: string,
|
||||
@@ -91,6 +116,7 @@ export class RolesService {
|
||||
if (existing) {
|
||||
throw new ConflictException('Role key already in use');
|
||||
}
|
||||
await this.assertCanGrantPermissions(dto.permissionKeys, actingUserId);
|
||||
const role = await this.prisma.$transaction(async (tx) => {
|
||||
const created = await tx.role.create({
|
||||
data: {
|
||||
@@ -126,6 +152,9 @@ export class RolesService {
|
||||
if (before.isSystem) {
|
||||
throw new ForbiddenException('System roles cannot be edited');
|
||||
}
|
||||
if (dto.permissionKeys) {
|
||||
await this.assertCanGrantPermissions(dto.permissionKeys, actingUserId);
|
||||
}
|
||||
const role = await this.prisma.$transaction(async (tx) => {
|
||||
await tx.role.update({
|
||||
where: { id },
|
||||
|
||||
Reference in new issue
Block a user