From 19d5e5d65c6e508dbd8ccd46133cb45e89f9a233 Mon Sep 17 00:00:00 2001 From: Nick Szaunig Date: Wed, 2 Sep 2026 14:44:28 +0200 Subject: [PATCH] fix: PermissionsGuard schliesst jetzt statt bei fehlendem Decorator zu oeffnen PermissionsGuard erlaubte bisher jede Route ohne @RequirePermission automatisch fuer jeden eingeloggten Nutzer (fail-open) und verliess sich allein auf test/authorization-coverage.e2e-spec.ts, um eine vergessene Decorator-Angabe vor dem Deploy zu erwischen - der Guard selbst hatte kein Default-Deny. Neuer Decorator @AnyAuthenticatedUser() macht das bisher stillschweigend erlaubte Verhalten explizit fuer die 12 Routen, die es tatsaechlich brauchen (ProfileController komplett, NotificationsController komplett, AuthController.me - alle bereits vorher auf der Allowlist des e2e-Tests). PermissionsGuard verweigert jetzt jede Route, die weder @RequirePermission noch @AnyAuthenticatedUser traegt - eine vergessene Decorator-Angabe schlaegt jetzt zur Laufzeit fehl (403), nicht mehr nur im Test. Der e2e-Test prueft jetzt denselben Decorator-Vertrag statt einer separat gepflegten Allowlist, kann also nicht mehr vom Guard-Verhalten abweichen. Neue Unit-Tests fuer PermissionsGuard decken beide Codepfade und das Fail-Closed-Verhalten ab. Co-Authored-By: Claude Sonnet 5 --- ApiCore-Backend/src/auth/auth.controller.ts | 2 + .../any-authenticated-user.decorator.ts | 16 ++++ .../src/auth/guards/permissions.guard.spec.ts | 88 +++++++++++++++++++ .../src/auth/guards/permissions.guard.ts | 36 +++++--- .../notifications/notifications.controller.ts | 2 + .../src/profile/profile.controller.ts | 2 + .../test/authorization-coverage.e2e-spec.ts | 41 ++++----- 7 files changed, 151 insertions(+), 36 deletions(-) create mode 100644 ApiCore-Backend/src/auth/decorators/any-authenticated-user.decorator.ts create mode 100644 ApiCore-Backend/src/auth/guards/permissions.guard.spec.ts diff --git a/ApiCore-Backend/src/auth/auth.controller.ts b/ApiCore-Backend/src/auth/auth.controller.ts index efbab53..e6f3258 100644 --- a/ApiCore-Backend/src/auth/auth.controller.ts +++ b/ApiCore-Backend/src/auth/auth.controller.ts @@ -12,6 +12,7 @@ import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; import { Throttle } from '@nestjs/throttler'; import type { Request, Response } from 'express'; import { Public } from './decorators/public.decorator'; +import { AnyAuthenticatedUser } from './decorators/any-authenticated-user.decorator'; import { CurrentUser } from './decorators/current-user.decorator'; import type { AuthenticatedUser } from './types/authenticated-user'; import { LoginDto } from './dto/login.dto'; @@ -92,6 +93,7 @@ export class AuthController { } @ApiBearerAuth() + @AnyAuthenticatedUser() @Get('me') me(@CurrentUser() user: AuthenticatedUser): Promise { return this.authService.getCurrentUserProfile(user.id); diff --git a/ApiCore-Backend/src/auth/decorators/any-authenticated-user.decorator.ts b/ApiCore-Backend/src/auth/decorators/any-authenticated-user.decorator.ts new file mode 100644 index 0000000..f6f421d --- /dev/null +++ b/ApiCore-Backend/src/auth/decorators/any-authenticated-user.decorator.ts @@ -0,0 +1,16 @@ +import { SetMetadata } from '@nestjs/common'; + +export const ANY_AUTHENTICATED_USER_KEY = 'any-authenticated-user'; + +/** + * Explicit opt-out of permission checking for a route that is meant to be + * reachable by any logged-in user regardless of role - it acts only on the + * caller's own data (profile, own notifications) or is the auth bootstrap + * endpoint every authenticated user must reach. Required by + * PermissionsGuard, which fails closed on any route carrying neither this + * nor @RequirePermission: a route is either explicitly public, explicitly + * permission-gated, or explicitly opted into "any authenticated user" - + * there is no fourth, silent option. + */ +export const AnyAuthenticatedUser = () => + SetMetadata(ANY_AUTHENTICATED_USER_KEY, true); diff --git a/ApiCore-Backend/src/auth/guards/permissions.guard.spec.ts b/ApiCore-Backend/src/auth/guards/permissions.guard.spec.ts new file mode 100644 index 0000000..d75dd59 --- /dev/null +++ b/ApiCore-Backend/src/auth/guards/permissions.guard.spec.ts @@ -0,0 +1,88 @@ +import { ExecutionContext } from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import { PermissionsGuard } from './permissions.guard'; +import { REQUIRE_PERMISSION_KEY } from '../decorators/require-permission.decorator'; +import { ANY_AUTHENTICATED_USER_KEY } from '../decorators/any-authenticated-user.decorator'; +import { PermissionResolverService } from '../permissions/permission-resolver.service'; + +describe('PermissionsGuard', () => { + let guard: PermissionsGuard; + let reflectorValues: Map; + let reflector: { getAllAndOverride: jest.Mock }; + let permissionResolver: { hasAny: jest.Mock }; + + function contextWithUser(user: { id: string } | undefined): ExecutionContext { + const request = { user }; + return { + getHandler: () => ({}), + getClass: () => ({}), + switchToHttp: () => ({ getRequest: () => request }), + } as unknown as ExecutionContext; + } + + beforeEach(() => { + reflectorValues = new Map(); + reflector = { + getAllAndOverride: jest.fn((key: string) => reflectorValues.get(key)), + }; + permissionResolver = { hasAny: jest.fn() }; + guard = new PermissionsGuard( + reflector as unknown as Reflector, + permissionResolver as unknown as PermissionResolverService, + ); + }); + + it('denies a route that carries neither @RequirePermission nor @AnyAuthenticatedUser (fails closed)', async () => { + await expect( + guard.canActivate(contextWithUser({ id: 'user-1' })), + ).resolves.toBe(false); + expect(permissionResolver.hasAny).not.toHaveBeenCalled(); + }); + + it('checks the caller permission set when @RequirePermission is present', async () => { + reflectorValues.set(REQUIRE_PERMISSION_KEY, ['apiaries:read']); + permissionResolver.hasAny.mockResolvedValue(true); + + await expect( + guard.canActivate(contextWithUser({ id: 'user-1' })), + ).resolves.toBe(true); + expect(permissionResolver.hasAny).toHaveBeenCalledWith('user-1', [ + 'apiaries:read', + ]); + }); + + it('denies when the caller lacks all required permissions', async () => { + reflectorValues.set(REQUIRE_PERMISSION_KEY, ['apiaries:read']); + permissionResolver.hasAny.mockResolvedValue(false); + + await expect( + guard.canActivate(contextWithUser({ id: 'user-1' })), + ).resolves.toBe(false); + }); + + it('denies a @RequirePermission route when there is no authenticated user on the request', async () => { + reflectorValues.set(REQUIRE_PERMISSION_KEY, ['apiaries:read']); + + await expect(guard.canActivate(contextWithUser(undefined))).resolves.toBe( + false, + ); + expect(permissionResolver.hasAny).not.toHaveBeenCalled(); + }); + + it('allows an authenticated user through a route marked @AnyAuthenticatedUser', async () => { + reflectorValues.set(ANY_AUTHENTICATED_USER_KEY, true); + + await expect( + guard.canActivate(contextWithUser({ id: 'user-1' })), + ).resolves.toBe(true); + expect(permissionResolver.hasAny).not.toHaveBeenCalled(); + }); + + it('denies @AnyAuthenticatedUser when there is no user on the request', async () => { + reflectorValues.set(ANY_AUTHENTICATED_USER_KEY, true); + + await expect(guard.canActivate(contextWithUser(undefined))).resolves.toBe( + false, + ); + }); +}); diff --git a/ApiCore-Backend/src/auth/guards/permissions.guard.ts b/ApiCore-Backend/src/auth/guards/permissions.guard.ts index ba35c12..31c78fa 100644 --- a/ApiCore-Backend/src/auth/guards/permissions.guard.ts +++ b/ApiCore-Backend/src/auth/guards/permissions.guard.ts @@ -1,5 +1,6 @@ import { CanActivate, ExecutionContext, Injectable } from '@nestjs/common'; import { Reflector } from '@nestjs/core'; +import { ANY_AUTHENTICATED_USER_KEY } from '../decorators/any-authenticated-user.decorator'; import { REQUIRE_PERMISSION_KEY } from '../decorators/require-permission.decorator'; import { PermissionKey } from '../permissions/permission-catalog'; import { PermissionResolverService } from '../permissions/permission-resolver.service'; @@ -10,14 +11,15 @@ interface RequestWithUser { } /** - * Fail-open by decorator absence: a route with no @RequirePermission is - * allowed through untouched (any authenticated user can reach it), only - * routes that carry the decorator are enforced against the caller's live - * permission set. There is no default-deny here, so - * test/authorization-coverage.e2e-spec.ts — which fails the build if any - * non-public route lacks a catalog-valid @RequirePermission — is the sole - * safety net for "someone added a route and forgot the decorator", not a - * defense-in-depth nicety. + * Fails closed by decorator absence: a non-public route reaches this guard + * only after JwtAuthGuard confirms a valid, active session, and from here it + * is denied unless it carries either @RequirePermission (checked against the + * caller's live permission set) or the explicit @AnyAuthenticatedUser opt-out + * for routes that are meant to be reachable by any logged-in user regardless + * of role. A route with neither is a bug - forgetting to decorate a new + * route now fails shut at runtime instead of silently granting access to + * every authenticated user and relying solely on + * test/authorization-coverage.e2e-spec.ts to catch it before deploy. */ @Injectable() export class PermissionsGuard implements CanActivate { @@ -30,14 +32,20 @@ export class PermissionsGuard implements CanActivate { const requiredPermissions = this.reflector.getAllAndOverride< PermissionKey[] >(REQUIRE_PERMISSION_KEY, [context.getHandler(), context.getClass()]); - if (!requiredPermissions || requiredPermissions.length === 0) { - return true; - } const request = context.switchToHttp().getRequest(); const user = request.user; - if (!user) { - return false; + + if (requiredPermissions && requiredPermissions.length > 0) { + if (!user) { + return false; + } + return this.permissionResolver.hasAny(user.id, requiredPermissions); } - return this.permissionResolver.hasAny(user.id, requiredPermissions); + + const isAnyAuthenticatedUser = this.reflector.getAllAndOverride( + ANY_AUTHENTICATED_USER_KEY, + [context.getHandler(), context.getClass()], + ); + return Boolean(isAnyAuthenticatedUser && user); } } diff --git a/ApiCore-Backend/src/notifications/notifications.controller.ts b/ApiCore-Backend/src/notifications/notifications.controller.ts index b8b6b29..9fcd8ee 100644 --- a/ApiCore-Backend/src/notifications/notifications.controller.ts +++ b/ApiCore-Backend/src/notifications/notifications.controller.ts @@ -1,11 +1,13 @@ import { Controller, Get, Param, Post, Query } from '@nestjs/common'; import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; +import { AnyAuthenticatedUser } from '../auth/decorators/any-authenticated-user.decorator'; import { CurrentUser } from '../auth/decorators/current-user.decorator'; import type { AuthenticatedUser } from '../auth/types/authenticated-user'; import { NotificationsService } from './notifications.service'; @ApiTags('notifications') @ApiBearerAuth() +@AnyAuthenticatedUser() @Controller('notifications') export class NotificationsController { constructor(private readonly notificationsService: NotificationsService) {} diff --git a/ApiCore-Backend/src/profile/profile.controller.ts b/ApiCore-Backend/src/profile/profile.controller.ts index 5511173..9eb7589 100644 --- a/ApiCore-Backend/src/profile/profile.controller.ts +++ b/ApiCore-Backend/src/profile/profile.controller.ts @@ -11,6 +11,7 @@ import { import { ApiBearerAuth, ApiTags } from '@nestjs/swagger'; import { Throttle } from '@nestjs/throttler'; import type { Response } from 'express'; +import { AnyAuthenticatedUser } from '../auth/decorators/any-authenticated-user.decorator'; import { CurrentUser } from '../auth/decorators/current-user.decorator'; import type { AuthenticatedUser } from '../auth/types/authenticated-user'; import { SENSITIVE_ACTION_THROTTLE } from '../auth/util/throttle'; @@ -33,6 +34,7 @@ import { ProfileService } from './profile.service'; */ @ApiTags('profile') @ApiBearerAuth() +@AnyAuthenticatedUser() @Controller('profile') export class ProfileController { constructor(private readonly profileService: ProfileService) {} diff --git a/ApiCore-Backend/test/authorization-coverage.e2e-spec.ts b/ApiCore-Backend/test/authorization-coverage.e2e-spec.ts index e093709..2bfc2cb 100644 --- a/ApiCore-Backend/test/authorization-coverage.e2e-spec.ts +++ b/ApiCore-Backend/test/authorization-coverage.e2e-spec.ts @@ -9,28 +9,20 @@ import { PATH_METADATA } from '@nestjs/common/constants'; import { AppModule } from '../src/app.module'; import { IS_PUBLIC_KEY } from '../src/auth/decorators/public.decorator'; import { REQUIRE_PERMISSION_KEY } from '../src/auth/decorators/require-permission.decorator'; +import { ANY_AUTHENTICATED_USER_KEY } from '../src/auth/decorators/any-authenticated-user.decorator'; import { PERMISSION_CATALOG } from '../src/auth/permissions/permission-catalog'; -// Routes intentionally left without a role/permission check: they act on the -// caller's own data (notifications) or are the auth bootstrap endpoint that -// every authenticated user must be able to reach regardless of permissions. -const ALLOWLIST = new Set([ - 'NotificationsController.findAll', - 'NotificationsController.markRead', - 'AuthController.me', - 'ProfileController.updateProfile', - 'ProfileController.changePassword', - 'ProfileController.deactivateAccount', - 'ProfileController.uploadAvatar', - 'ProfileController.deleteAvatar', - 'ProfileController.getAvatar', - 'ProfileController.enrollMfa', - 'ProfileController.confirmMfaEnrollment', - 'ProfileController.disableMfa', -]); - +/** + * Mirrors PermissionsGuard's own runtime contract exactly (see + * src/auth/guards/permissions.guard.ts): every non-public route must carry + * either @RequirePermission or @AnyAuthenticatedUser, or the guard denies it + * at runtime regardless of what this test says. This test exists to catch + * the mistake at build time - with a clear message pointing at the missing + * decorator - rather than as a live production 403 discovered by a user, but + * a route that fails it would also fail closed live, not open. + */ describe('Authorization coverage (e2e)', () => { - it('every non-public route requires a catalog-valid permission', async () => { + it('every non-public route requires a permission or an explicit any-authenticated-user opt-in', async () => { const moduleRef = await Test.createTestingModule({ imports: [AppModule, DiscoveryModule], }).compile(); @@ -68,7 +60,7 @@ describe('Authorization coverage (e2e)', () => { const routeKey = `${controllerName}.${methodName}`; const isPublicMethod = reflector.get(IS_PUBLIC_KEY, handler) === true; - if (isPublicClass || isPublicMethod || ALLOWLIST.has(routeKey)) { + if (isPublicClass || isPublicMethod) { continue; } @@ -76,10 +68,15 @@ describe('Authorization coverage (e2e)', () => { REQUIRE_PERMISSION_KEY, [handler, constructor], ); - const hasPermissions = Array.isArray(permissions) && permissions.length > 0; - if (!hasPermissions) { + const isAnyAuthenticatedUser = + reflector.getAllAndOverride(ANY_AUTHENTICATED_USER_KEY, [ + handler, + constructor, + ]) === true; + + if (!hasPermissions && !isAnyAuthenticatedUser) { uncoveredRoutes.push(routeKey); } if (hasPermissions) {