fix: Rechteausweitung ueber Rollenzuweisung und weitere Auth-Haertung
Ein Sicherheitsaudit deckte auf, dass jede Rolle mit users:update sich selbst per PATCH /users/:id auf eine Admin-Rolle heraufstufen konnte, da roleId ohne eigene Berechtigung und ohne Selbstbezug-Pruefung akzeptiert wurde. Ausserdem lag ein fest im Code hinterlegtes Standard-Admin-Passwort vor, das bei fehlender SEED_ADMIN_PASSWORD-Env greift. - Neue Berechtigung users:assign-role, getrennt von users:create/update, fuer jede Rollenzuweisung erforderlich - Selbstaenderung der eigenen Rolle serverseitig blockiert (auch fuer Admins), ebenso Selbst-Reset der eigenen MFA ueber die Admin-Route - Seed erzeugt bei fehlender Env ein zufaelliges statt eines fixen Passworts - Wiederverwendung eines bereits rotierten Refresh-Tokens (Diebstahl- Indiz) widerruft nun alle Sessions des betroffenen Nutzers - Rate-Limit auf MFA-Enrollment-Bestaetigung ergaenzt Co-Authored-By: Claude Sonnet 5 <[email protected]>
This commit is contained in:
1 parent
7bd2a3dc3a
commit
1b16b49913
7 files changed
+146
-7
No files matched your search
@@ -1,11 +1,19 @@
|
|||||||
import 'dotenv/config';
|
import 'dotenv/config';
|
||||||
import * as bcrypt from 'bcryptjs';
|
import * as bcrypt from 'bcryptjs';
|
||||||
|
import * as crypto from 'node:crypto';
|
||||||
import { PrismaPg } from '@prisma/adapter-pg';
|
import { PrismaPg } from '@prisma/adapter-pg';
|
||||||
import { Prisma, PrismaClient } from '../generated/prisma';
|
import { Prisma, PrismaClient } from '../generated/prisma';
|
||||||
|
|
||||||
const BCRYPT_SALT_ROUNDS = 12;
|
const BCRYPT_SALT_ROUNDS = 12;
|
||||||
const DEFAULT_ADMIN_EMAIL = '[email protected]';
|
const DEFAULT_ADMIN_EMAIL = '[email protected]';
|
||||||
const DEFAULT_ADMIN_PASSWORD = 'ChangeMe123!';
|
|
||||||
|
// No fixed fallback password: a fixed literal shipped in source would be a
|
||||||
|
// well-known, guessable credential for any deployment that forgets to set
|
||||||
|
// SEED_ADMIN_PASSWORD. A random one is generated instead and printed once
|
||||||
|
// below, same as an operator-supplied one would be.
|
||||||
|
function generateRandomPassword(): string {
|
||||||
|
return crypto.randomBytes(18).toString('base64url');
|
||||||
|
}
|
||||||
|
|
||||||
// Only printed the moment the account is actually created (first deploy on
|
// Only printed the moment the account is actually created (first deploy on
|
||||||
// an empty database), never on later restarts: by then the real admin may
|
// an empty database), never on later restarts: by then the real admin may
|
||||||
@@ -28,7 +36,7 @@ async function main(): Promise<void> {
|
|||||||
});
|
});
|
||||||
|
|
||||||
const email = process.env.SEED_ADMIN_EMAIL ?? DEFAULT_ADMIN_EMAIL;
|
const email = process.env.SEED_ADMIN_EMAIL ?? DEFAULT_ADMIN_EMAIL;
|
||||||
const password = process.env.SEED_ADMIN_PASSWORD ?? DEFAULT_ADMIN_PASSWORD;
|
const password = process.env.SEED_ADMIN_PASSWORD ?? generateRandomPassword();
|
||||||
const existing = await prisma.user.findUnique({ where: { email } });
|
const existing = await prisma.user.findUnique({ where: { email } });
|
||||||
if (existing) {
|
if (existing) {
|
||||||
console.log(`Seed: admin user ${email} already exists, skipping.`);
|
console.log(`Seed: admin user ${email} already exists, skipping.`);
|
||||||
|
|||||||
@@ -19,7 +19,10 @@ describe('AuthService', () => {
|
|||||||
};
|
};
|
||||||
let jwtService: { sign: jest.Mock };
|
let jwtService: { sign: jest.Mock };
|
||||||
let auditLog: { record: jest.Mock };
|
let auditLog: { record: jest.Mock };
|
||||||
let permissionResolver: { getPermissions: jest.Mock };
|
let permissionResolver: {
|
||||||
|
getPermissions: jest.Mock;
|
||||||
|
invalidateUser: jest.Mock;
|
||||||
|
};
|
||||||
let mfaService: {
|
let mfaService: {
|
||||||
signChallengeToken: jest.Mock;
|
signChallengeToken: jest.Mock;
|
||||||
verifyChallengeToken: jest.Mock;
|
verifyChallengeToken: jest.Mock;
|
||||||
@@ -52,6 +55,7 @@ describe('AuthService', () => {
|
|||||||
auditLog = { record: jest.fn().mockResolvedValue(undefined) };
|
auditLog = { record: jest.fn().mockResolvedValue(undefined) };
|
||||||
permissionResolver = {
|
permissionResolver = {
|
||||||
getPermissions: jest.fn().mockResolvedValue(new Set()),
|
getPermissions: jest.fn().mockResolvedValue(new Set()),
|
||||||
|
invalidateUser: jest.fn(),
|
||||||
};
|
};
|
||||||
mfaService = {
|
mfaService = {
|
||||||
signChallengeToken: jest.fn().mockReturnValue('mfa-challenge-token'),
|
signChallengeToken: jest.fn().mockReturnValue('mfa-challenge-token'),
|
||||||
@@ -264,16 +268,26 @@ describe('AuthService', () => {
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('throws when the refresh token is revoked', async () => {
|
it('throws when the refresh token is revoked and revokes all other sessions for that user (reuse detection)', async () => {
|
||||||
prisma.refreshToken.findUnique.mockResolvedValue({
|
prisma.refreshToken.findUnique.mockResolvedValue({
|
||||||
id: 'rt-1',
|
id: 'rt-1',
|
||||||
userId: activeUser.id,
|
userId: activeUser.id,
|
||||||
revokedAt: new Date(),
|
revokedAt: new Date(),
|
||||||
expiresAt: new Date(Date.now() + 1000),
|
expiresAt: new Date(Date.now() + 1000),
|
||||||
});
|
});
|
||||||
|
prisma.refreshToken.updateMany.mockResolvedValue({ count: 2 });
|
||||||
|
|
||||||
await expect(authService.refresh('revoked-token')).rejects.toBeInstanceOf(
|
await expect(authService.refresh('revoked-token')).rejects.toBeInstanceOf(
|
||||||
UnauthorizedException,
|
UnauthorizedException,
|
||||||
);
|
);
|
||||||
|
expect(prisma.refreshToken.updateMany).toHaveBeenCalledWith(
|
||||||
|
expect.objectContaining({
|
||||||
|
where: { userId: activeUser.id, revokedAt: null },
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
expect(permissionResolver.invalidateUser).toHaveBeenCalledWith(
|
||||||
|
activeUser.id,
|
||||||
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('throws when the refresh token is expired', async () => {
|
it('throws when the refresh token is expired', async () => {
|
||||||
|
|||||||
@@ -207,7 +207,25 @@ export class AuthService {
|
|||||||
const stored = await this.prisma.refreshToken.findUnique({
|
const stored = await this.prisma.refreshToken.findUnique({
|
||||||
where: { tokenHash },
|
where: { tokenHash },
|
||||||
});
|
});
|
||||||
if (!stored || stored.revokedAt || stored.expiresAt < new Date()) {
|
if (!stored) {
|
||||||
|
throw new UnauthorizedException('Invalid refresh token');
|
||||||
|
}
|
||||||
|
if (stored.revokedAt) {
|
||||||
|
// Reuse of an already-rotated-out refresh token is a strong signal
|
||||||
|
// it was stolen (a legitimate client only ever presents the newest
|
||||||
|
// token) - revoke every other active session for this user rather
|
||||||
|
// than trusting rotation alone to have contained the leak.
|
||||||
|
await this.prisma.refreshToken.updateMany({
|
||||||
|
where: { userId: stored.userId, revokedAt: null },
|
||||||
|
data: { revokedAt: new Date() },
|
||||||
|
});
|
||||||
|
this.permissionResolver.invalidateUser(stored.userId);
|
||||||
|
this.logger.warn(
|
||||||
|
`Refresh token reuse detected for user ${stored.userId}; all sessions revoked`,
|
||||||
|
);
|
||||||
|
throw new UnauthorizedException('Invalid refresh token');
|
||||||
|
}
|
||||||
|
if (stored.expiresAt < new Date()) {
|
||||||
throw new UnauthorizedException('Invalid refresh token');
|
throw new UnauthorizedException('Invalid refresh token');
|
||||||
}
|
}
|
||||||
const user = await this.usersService.findById(stored.userId);
|
const user = await this.usersService.findById(stored.userId);
|
||||||
|
|||||||
@@ -255,6 +255,7 @@ export const PERMISSION_CATALOG = [
|
|||||||
['create', 'Anlegen'],
|
['create', 'Anlegen'],
|
||||||
['update', 'Bearbeiten'],
|
['update', 'Bearbeiten'],
|
||||||
['deactivate', 'Deaktivieren'],
|
['deactivate', 'Deaktivieren'],
|
||||||
|
['assign-role', 'Rolle zuweisen'],
|
||||||
],
|
],
|
||||||
2300,
|
2300,
|
||||||
),
|
),
|
||||||
|
|||||||
@@ -100,6 +100,7 @@ export class ProfileController {
|
|||||||
return this.profileService.enrollMfa(actor.id, actor.email);
|
return this.profileService.enrollMfa(actor.id, actor.email);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Throttle(SENSITIVE_ACTION_THROTTLE)
|
||||||
@Post('mfa/enroll/confirm')
|
@Post('mfa/enroll/confirm')
|
||||||
@HttpCode(204)
|
@HttpCode(204)
|
||||||
confirmMfaEnrollment(
|
confirmMfaEnrollment(
|
||||||
|
|||||||
@@ -1,4 +1,8 @@
|
|||||||
import { ConflictException, NotFoundException } from '@nestjs/common';
|
import {
|
||||||
|
ConflictException,
|
||||||
|
ForbiddenException,
|
||||||
|
NotFoundException,
|
||||||
|
} from '@nestjs/common';
|
||||||
import { UsersService } from './users.service';
|
import { UsersService } from './users.service';
|
||||||
import { PrismaService } from '../prisma/prisma.service';
|
import { PrismaService } from '../prisma/prisma.service';
|
||||||
import { AuditLogService } from '../audit/audit-log.service';
|
import { AuditLogService } from '../audit/audit-log.service';
|
||||||
@@ -22,6 +26,7 @@ describe('UsersService', () => {
|
|||||||
let permissionResolver: {
|
let permissionResolver: {
|
||||||
invalidateUser: jest.Mock;
|
invalidateUser: jest.Mock;
|
||||||
invalidateAll: jest.Mock;
|
invalidateAll: jest.Mock;
|
||||||
|
hasAny: jest.Mock;
|
||||||
};
|
};
|
||||||
|
|
||||||
const staffRole = {
|
const staffRole = {
|
||||||
@@ -77,6 +82,7 @@ describe('UsersService', () => {
|
|||||||
permissionResolver = {
|
permissionResolver = {
|
||||||
invalidateUser: jest.fn(),
|
invalidateUser: jest.fn(),
|
||||||
invalidateAll: jest.fn(),
|
invalidateAll: jest.fn(),
|
||||||
|
hasAny: jest.fn().mockResolvedValue(true),
|
||||||
};
|
};
|
||||||
usersService = new UsersService(
|
usersService = new UsersService(
|
||||||
prisma as unknown as PrismaService,
|
prisma as unknown as PrismaService,
|
||||||
@@ -324,4 +330,53 @@ describe('UsersService', () => {
|
|||||||
).rejects.toBeInstanceOf(ConflictException);
|
).rejects.toBeInstanceOf(ConflictException);
|
||||||
expect(prisma.user.update).not.toHaveBeenCalled();
|
expect(prisma.user.update).not.toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('update throws ForbiddenException when the caller tries to change their own role', async () => {
|
||||||
|
prisma.user.findUnique.mockResolvedValue(adminUser);
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
usersService.update(adminUser.id, { roleId: staffRole.id }, adminUser.id),
|
||||||
|
).rejects.toBeInstanceOf(ForbiddenException);
|
||||||
|
expect(prisma.user.update).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('update throws ForbiddenException when the caller lacks users:assign-role', async () => {
|
||||||
|
prisma.user.findUnique.mockResolvedValue(existingUser);
|
||||||
|
prisma.role.findUnique.mockResolvedValue(adminRole);
|
||||||
|
permissionResolver.hasAny.mockResolvedValue(false);
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
usersService.update(
|
||||||
|
existingUser.id,
|
||||||
|
{ roleId: adminRole.id },
|
||||||
|
adminUser.id,
|
||||||
|
),
|
||||||
|
).rejects.toBeInstanceOf(ForbiddenException);
|
||||||
|
expect(prisma.user.update).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('create throws ForbiddenException when the caller lacks users:assign-role', async () => {
|
||||||
|
prisma.user.findUnique.mockResolvedValue(null);
|
||||||
|
permissionResolver.hasAny.mockResolvedValue(false);
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
usersService.create(
|
||||||
|
{
|
||||||
|
email: '[email protected]',
|
||||||
|
password: 'password123',
|
||||||
|
name: 'New User',
|
||||||
|
roleId: staffRole.id,
|
||||||
|
},
|
||||||
|
adminUser.id,
|
||||||
|
),
|
||||||
|
).rejects.toBeInstanceOf(ForbiddenException);
|
||||||
|
expect(prisma.user.create).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('resetMfa throws ForbiddenException when targeting your own account', async () => {
|
||||||
|
await expect(
|
||||||
|
usersService.resetMfa(adminUser.id, adminUser.id),
|
||||||
|
).rejects.toBeInstanceOf(ForbiddenException);
|
||||||
|
expect(prisma.user.update).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
@@ -1,5 +1,6 @@
|
|||||||
import {
|
import {
|
||||||
ConflictException,
|
ConflictException,
|
||||||
|
ForbiddenException,
|
||||||
Injectable,
|
Injectable,
|
||||||
NotFoundException,
|
NotFoundException,
|
||||||
} from '@nestjs/common';
|
} from '@nestjs/common';
|
||||||
@@ -55,6 +56,27 @@ export class UsersService {
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Assigning a role is gated by its own permission, separate from
|
||||||
|
* users:create/users:update - otherwise any role holding those (e.g. a
|
||||||
|
* minimal "user manager" role) could hand out any role, including a
|
||||||
|
* system/admin one, to itself or a colluding account. actingUserId is
|
||||||
|
* only absent for non-HTTP callers (there are none today, and tests that
|
||||||
|
* exercise the service directly without an actor), where there is no
|
||||||
|
* caller to check permissions against.
|
||||||
|
*/
|
||||||
|
private async assertCanAssignRole(actingUserId?: string): Promise<void> {
|
||||||
|
if (!actingUserId) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
const allowed = await this.permissionResolver.hasAny(actingUserId, [
|
||||||
|
'users:assign-role',
|
||||||
|
]);
|
||||||
|
if (!allowed) {
|
||||||
|
throw new ForbiddenException('Missing permission to assign roles');
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
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) {
|
||||||
@@ -127,6 +149,7 @@ export class UsersService {
|
|||||||
throw new ConflictException('Email already in use');
|
throw new ConflictException('Email already in use');
|
||||||
}
|
}
|
||||||
await this.assertActiveRole(dto.roleId);
|
await this.assertActiveRole(dto.roleId);
|
||||||
|
await this.assertCanAssignRole(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: {
|
||||||
@@ -158,6 +181,13 @@ export class UsersService {
|
|||||||
await this.assertActiveRole(dto.roleId);
|
await this.assertActiveRole(dto.roleId);
|
||||||
}
|
}
|
||||||
if (roleChanged) {
|
if (roleChanged) {
|
||||||
|
// Role changes for yourself always require a second party, regardless
|
||||||
|
// of what permission the caller holds - otherwise anyone who can
|
||||||
|
// reach this endpoint at all could grant themselves admin.
|
||||||
|
if (actingUserId && id === actingUserId) {
|
||||||
|
throw new ForbiddenException('Cannot change your own role');
|
||||||
|
}
|
||||||
|
await this.assertCanAssignRole(actingUserId);
|
||||||
await this.assertNotLastSystemRoleUser(id);
|
await this.assertNotLastSystemRoleUser(id);
|
||||||
}
|
}
|
||||||
const user = await this.prisma.user.update({
|
const user = await this.prisma.user.update({
|
||||||
@@ -202,8 +232,20 @@ export class UsersService {
|
|||||||
return this.toSummary(user);
|
return this.toSummary(user);
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Recovery path for a user who lost their MFA device (no email-reset flow exists in this project). */
|
/**
|
||||||
|
* Recovery path for a user who lost their MFA device (no email-reset flow
|
||||||
|
* exists in this project). Deliberately blocked for the caller's own
|
||||||
|
* account: unlike resetting someone else's MFA, self-service MFA removal
|
||||||
|
* belongs behind a password check (POST /profile/mfa/disable) - otherwise
|
||||||
|
* a stolen access token alone (no password) would be enough to strip the
|
||||||
|
* victim's own MFA protection via this admin route.
|
||||||
|
*/
|
||||||
async resetMfa(id: string, actingUserId?: string): Promise<UserSummary> {
|
async resetMfa(id: string, actingUserId?: string): Promise<UserSummary> {
|
||||||
|
if (actingUserId && id === actingUserId) {
|
||||||
|
throw new ForbiddenException(
|
||||||
|
'Use /profile/mfa/disable to disable your own MFA',
|
||||||
|
);
|
||||||
|
}
|
||||||
await this.findById(id);
|
await this.findById(id);
|
||||||
const user = await this.prisma.user.update({
|
const user = await this.prisma.user.update({
|
||||||
where: { id },
|
where: { id },
|
||||||
|
|||||||
Reference in new issue
Block a user