fix: Access-Token wird bei deaktivierten Usern sofort ungueltig
Bisher griff der isActive-Check nur auf Routen mit @RequirePermission. Ein deaktivierter User mit gueltigem Token konnte Routen ohne Permission-Decorator bis zum Token-Ablauf (bis zu 15 Min) weiter erreichen. JwtAuthGuard prueft jetzt PermissionResolverService.isUserActive auf jedem authentifizierten Request, ueber denselben 30s-Cache wie die Permission-Aufloesung, also ohne zusaetzliche DB-Last.
This commit is contained in:
1 parent
5a1015c4a0
commit
adfbc2ab83
4 files changed
+190
-18
No files matched your search
@@ -0,0 +1,70 @@
|
|||||||
|
import { ExecutionContext, UnauthorizedException } from '@nestjs/common';
|
||||||
|
import { Reflector } from '@nestjs/core';
|
||||||
|
import { JwtAuthGuard } from './jwt-auth.guard';
|
||||||
|
import { PermissionResolverService } from '../permissions/permission-resolver.service';
|
||||||
|
|
||||||
|
describe('JwtAuthGuard', () => {
|
||||||
|
let guard: JwtAuthGuard;
|
||||||
|
let reflector: { getAllAndOverride: jest.Mock };
|
||||||
|
let permissionResolver: { isUserActive: jest.Mock };
|
||||||
|
|
||||||
|
function contextWithUser(user: { id: string } | undefined): ExecutionContext {
|
||||||
|
const request = { user };
|
||||||
|
return {
|
||||||
|
getHandler: () => ({}),
|
||||||
|
getClass: () => ({}),
|
||||||
|
switchToHttp: () => ({ getRequest: () => request }),
|
||||||
|
} as unknown as ExecutionContext;
|
||||||
|
}
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
reflector = { getAllAndOverride: jest.fn().mockReturnValue(false) };
|
||||||
|
permissionResolver = { isUserActive: jest.fn() };
|
||||||
|
guard = new JwtAuthGuard(
|
||||||
|
reflector as unknown as Reflector,
|
||||||
|
permissionResolver as unknown as PermissionResolverService,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('allows public routes through without checking authentication', async () => {
|
||||||
|
reflector.getAllAndOverride.mockReturnValue(true);
|
||||||
|
|
||||||
|
await expect(guard.canActivate(contextWithUser(undefined))).resolves.toBe(
|
||||||
|
true,
|
||||||
|
);
|
||||||
|
expect(permissionResolver.isUserActive).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('allows the request when the JWT is valid and the user is active', async () => {
|
||||||
|
const context = contextWithUser({ id: 'user-1' });
|
||||||
|
jest
|
||||||
|
.spyOn(
|
||||||
|
Object.getPrototypeOf(JwtAuthGuard.prototype) as {
|
||||||
|
canActivate: (context: ExecutionContext) => boolean;
|
||||||
|
},
|
||||||
|
'canActivate',
|
||||||
|
)
|
||||||
|
.mockReturnValue(true);
|
||||||
|
permissionResolver.isUserActive.mockResolvedValue(true);
|
||||||
|
|
||||||
|
await expect(guard.canActivate(context)).resolves.toBe(true);
|
||||||
|
expect(permissionResolver.isUserActive).toHaveBeenCalledWith('user-1');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('throws UnauthorizedException when the authenticated user has been deactivated', async () => {
|
||||||
|
const context = contextWithUser({ id: 'user-1' });
|
||||||
|
jest
|
||||||
|
.spyOn(
|
||||||
|
Object.getPrototypeOf(JwtAuthGuard.prototype) as {
|
||||||
|
canActivate: (context: ExecutionContext) => boolean;
|
||||||
|
},
|
||||||
|
'canActivate',
|
||||||
|
)
|
||||||
|
.mockReturnValue(true);
|
||||||
|
permissionResolver.isUserActive.mockResolvedValue(false);
|
||||||
|
|
||||||
|
await expect(guard.canActivate(context)).rejects.toBeInstanceOf(
|
||||||
|
UnauthorizedException,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -1,15 +1,37 @@
|
|||||||
import { ExecutionContext, Injectable } from '@nestjs/common';
|
import {
|
||||||
|
ExecutionContext,
|
||||||
|
Injectable,
|
||||||
|
UnauthorizedException,
|
||||||
|
} from '@nestjs/common';
|
||||||
import { Reflector } from '@nestjs/core';
|
import { Reflector } from '@nestjs/core';
|
||||||
import { AuthGuard } from '@nestjs/passport';
|
import { AuthGuard } from '@nestjs/passport';
|
||||||
import { IS_PUBLIC_KEY } from '../decorators/public.decorator';
|
import { IS_PUBLIC_KEY } from '../decorators/public.decorator';
|
||||||
|
import { PermissionResolverService } from '../permissions/permission-resolver.service';
|
||||||
|
import { AuthenticatedUser } from '../types/authenticated-user';
|
||||||
|
|
||||||
|
interface RequestWithUser {
|
||||||
|
user?: AuthenticatedUser;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Enforces User.isActive on every authenticated request, not just ones
|
||||||
|
* gated by @RequirePermission. Without this, a deactivated user's
|
||||||
|
* still-valid access token would keep working on any route that carries no
|
||||||
|
* permission decorator until the token's own expiry (up to
|
||||||
|
* JWT_ACCESS_EXPIRES_IN). isUserActive shares PermissionsGuard's cache, so
|
||||||
|
* this check costs no extra query beyond what permission resolution already
|
||||||
|
* does.
|
||||||
|
*/
|
||||||
@Injectable()
|
@Injectable()
|
||||||
export class JwtAuthGuard extends AuthGuard('jwt') {
|
export class JwtAuthGuard extends AuthGuard('jwt') {
|
||||||
constructor(private readonly reflector: Reflector) {
|
constructor(
|
||||||
|
private readonly reflector: Reflector,
|
||||||
|
private readonly permissionResolver: PermissionResolverService,
|
||||||
|
) {
|
||||||
super();
|
super();
|
||||||
}
|
}
|
||||||
|
|
||||||
canActivate(context: ExecutionContext) {
|
async canActivate(context: ExecutionContext): Promise<boolean> {
|
||||||
const isPublic = this.reflector.getAllAndOverride<boolean>(IS_PUBLIC_KEY, [
|
const isPublic = this.reflector.getAllAndOverride<boolean>(IS_PUBLIC_KEY, [
|
||||||
context.getHandler(),
|
context.getHandler(),
|
||||||
context.getClass(),
|
context.getClass(),
|
||||||
@@ -17,6 +39,15 @@ export class JwtAuthGuard extends AuthGuard('jwt') {
|
|||||||
if (isPublic) {
|
if (isPublic) {
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
return super.canActivate(context);
|
const authenticated = (await super.canActivate(context)) as boolean;
|
||||||
|
if (!authenticated) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
const request = context.switchToHttp().getRequest<RequestWithUser>();
|
||||||
|
const user = request.user;
|
||||||
|
if (!user || !(await this.permissionResolver.isUserActive(user.id))) {
|
||||||
|
throw new UnauthorizedException();
|
||||||
|
}
|
||||||
|
return true;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -93,6 +93,57 @@ describe('PermissionResolverService', () => {
|
|||||||
expect(prisma.user.findUnique).toHaveBeenCalledTimes(2);
|
expect(prisma.user.findUnique).toHaveBeenCalledTimes(2);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('isUserActive', () => {
|
||||||
|
it('returns true for an active user with an active role', async () => {
|
||||||
|
prisma.user.findUnique.mockResolvedValue(
|
||||||
|
activeUserWithPermissions('colonies:read'),
|
||||||
|
);
|
||||||
|
|
||||||
|
await expect(resolver.isUserActive('user-1')).resolves.toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns false for a deactivated user', async () => {
|
||||||
|
prisma.user.findUnique.mockResolvedValue({
|
||||||
|
isActive: false,
|
||||||
|
role: {
|
||||||
|
status: 'ACTIVE',
|
||||||
|
permissions: [{ permission: { key: 'colonies:read' } }],
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
|
await expect(resolver.isUserActive('user-1')).resolves.toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns false when the role is inactive', async () => {
|
||||||
|
prisma.user.findUnique.mockResolvedValue({
|
||||||
|
isActive: true,
|
||||||
|
role: {
|
||||||
|
status: 'INACTIVE',
|
||||||
|
permissions: [{ permission: { key: 'colonies:read' } }],
|
||||||
|
},
|
||||||
|
});
|
||||||
|
|
||||||
|
await expect(resolver.isUserActive('user-1')).resolves.toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns false when the user does not exist', async () => {
|
||||||
|
prisma.user.findUnique.mockResolvedValue(null);
|
||||||
|
|
||||||
|
await expect(resolver.isUserActive('missing')).resolves.toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('shares the cached entry with getPermissions instead of re-querying', async () => {
|
||||||
|
prisma.user.findUnique.mockResolvedValue(
|
||||||
|
activeUserWithPermissions('colonies:read'),
|
||||||
|
);
|
||||||
|
|
||||||
|
await resolver.getPermissions('user-1');
|
||||||
|
await resolver.isUserActive('user-1');
|
||||||
|
|
||||||
|
expect(prisma.user.findUnique).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it('re-queries after invalidateAll', async () => {
|
it('re-queries after invalidateAll', async () => {
|
||||||
prisma.user.findUnique.mockResolvedValue(
|
prisma.user.findUnique.mockResolvedValue(
|
||||||
activeUserWithPermissions('colonies:read'),
|
activeUserWithPermissions('colonies:read'),
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ import { PrismaService } from '../../prisma/prisma.service';
|
|||||||
import { PermissionKey } from './permission-catalog';
|
import { PermissionKey } from './permission-catalog';
|
||||||
|
|
||||||
interface CacheEntry {
|
interface CacheEntry {
|
||||||
|
isActive: boolean;
|
||||||
permissions: ReadonlySet<string>;
|
permissions: ReadonlySet<string>;
|
||||||
expiresAt: number;
|
expiresAt: number;
|
||||||
}
|
}
|
||||||
@@ -17,6 +18,11 @@ const CACHE_TTL_MS = 30_000;
|
|||||||
* invalidateUser/invalidateAll after any write to Role, Permission,
|
* invalidateUser/invalidateAll after any write to Role, Permission,
|
||||||
* RolePermission, or User.roleId so the change is visible immediately
|
* RolePermission, or User.roleId so the change is visible immediately
|
||||||
* instead of waiting out the cache window.
|
* instead of waiting out the cache window.
|
||||||
|
*
|
||||||
|
* isUserActive shares the same cached entry: JwtAuthGuard calls it on every
|
||||||
|
* authenticated request (including routes with no @RequirePermission) so a
|
||||||
|
* deactivated user's still-valid access token stops working within the
|
||||||
|
* cache TTL instead of staying usable until the token expires.
|
||||||
*/
|
*/
|
||||||
@Injectable()
|
@Injectable()
|
||||||
export class PermissionResolverService {
|
export class PermissionResolverService {
|
||||||
@@ -25,16 +31,13 @@ export class PermissionResolverService {
|
|||||||
constructor(private readonly prisma: PrismaService) {}
|
constructor(private readonly prisma: PrismaService) {}
|
||||||
|
|
||||||
async getPermissions(userId: string): Promise<ReadonlySet<string>> {
|
async getPermissions(userId: string): Promise<ReadonlySet<string>> {
|
||||||
const cached = this.cache.get(userId);
|
const entry = await this.getEntry(userId);
|
||||||
if (cached && cached.expiresAt > Date.now()) {
|
return entry.permissions;
|
||||||
return cached.permissions;
|
|
||||||
}
|
}
|
||||||
const permissions = await this.loadPermissions(userId);
|
|
||||||
this.cache.set(userId, {
|
async isUserActive(userId: string): Promise<boolean> {
|
||||||
permissions,
|
const entry = await this.getEntry(userId);
|
||||||
expiresAt: Date.now() + CACHE_TTL_MS,
|
return entry.isActive;
|
||||||
});
|
|
||||||
return permissions;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
async hasAny(
|
async hasAny(
|
||||||
@@ -56,7 +59,17 @@ export class PermissionResolverService {
|
|||||||
this.cache.clear();
|
this.cache.clear();
|
||||||
}
|
}
|
||||||
|
|
||||||
private async loadPermissions(userId: string): Promise<ReadonlySet<string>> {
|
private async getEntry(userId: string): Promise<CacheEntry> {
|
||||||
|
const cached = this.cache.get(userId);
|
||||||
|
if (cached && cached.expiresAt > Date.now()) {
|
||||||
|
return cached;
|
||||||
|
}
|
||||||
|
const entry = await this.loadEntry(userId);
|
||||||
|
this.cache.set(userId, entry);
|
||||||
|
return entry;
|
||||||
|
}
|
||||||
|
|
||||||
|
private async loadEntry(userId: string): Promise<CacheEntry> {
|
||||||
const user = await this.prisma.user.findUnique({
|
const user = await this.prisma.user.findUnique({
|
||||||
where: { id: userId },
|
where: { id: userId },
|
||||||
select: {
|
select: {
|
||||||
@@ -71,9 +84,16 @@ export class PermissionResolverService {
|
|||||||
},
|
},
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
if (!user || !user.isActive || user.role.status !== 'ACTIVE') {
|
const isEligible = Boolean(
|
||||||
return new Set();
|
user && user.isActive && user.role.status === 'ACTIVE',
|
||||||
}
|
);
|
||||||
return new Set(user.role.permissions.map((grant) => grant.permission.key));
|
const permissions = isEligible
|
||||||
|
? new Set(user!.role.permissions.map((grant) => grant.permission.key))
|
||||||
|
: new Set<string>();
|
||||||
|
return {
|
||||||
|
isActive: isEligible,
|
||||||
|
permissions,
|
||||||
|
expiresAt: Date.now() + CACHE_TTL_MS,
|
||||||
|
};
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Reference in new issue
Block a user