improvements from review comments

sleidig · sleidig · commit ad124f8d9bab · 2026-03-25T17:34:08.000+01:00
diff --git a/src/couchdb/document-changes.service.spec.ts b/src/couchdb/document-changes.service.spec.ts
@@ -62,10 +62,9 @@ describe('DocumentChangesService', () => {
   });
 
   it('should reuse the same feed for the same database', () => {
-    const subject1 = service.getChanges('app');
-    const subject2 = service.getChanges('app');
+    service.getChanges('app');
+    service.getChanges('app');
 
-    expect(subject1).toBe(subject2);
     // Only one _changes request should be started
     expect(mockCouchdbService.get).toHaveBeenCalledTimes(1);
   });
@@ -76,10 +75,9 @@ describe('DocumentChangesService', () => {
       .spyOn(mockCouchdbService, 'get')
       .mockReturnValue(changesSubject as any);
 
-    const subject1 = service.getChanges('app');
-    const subject2 = service.getChanges('other-db');
+    service.getChanges('app');
+    service.getChanges('other-db');
 
-    expect(subject1).not.toBe(subject2);
     expect(mockCouchdbService.get).toHaveBeenCalledTimes(2);
   });
 
diff --git a/src/couchdb/document-changes.service.ts b/src/couchdb/document-changes.service.ts
@@ -1,5 +1,14 @@
 import { Injectable, Logger } from '@nestjs/common';
-import { catchError, concatMap, defer, of, repeat, retry, Subject } from 'rxjs';
+import {
+  catchError,
+  concatMap,
+  defer,
+  Observable,
+  of,
+  repeat,
+  retry,
+  Subject,
+} from 'rxjs';
 import {
   ChangeResult,
   ChangesResponse,
@@ -24,13 +33,13 @@ export class DocumentChangesService {
    * The underlying longpoll connection is started lazily on first
    * subscription and shared across all subscribers for that database.
    */
-  getChanges(db: string): Subject<ChangeResult> {
+  getChanges(db: string): Observable<ChangeResult> {
     if (!this.feeds.has(db)) {
       const subject = new Subject<ChangeResult>();
       this.feeds.set(db, subject);
       this.startFeed(db, subject);
     }
-    return this.feeds.get(db);
+    return this.feeds.get(db).asObservable();
   }
 
   private startFeed(db: string, subject: Subject<ChangeResult>): void {
diff --git a/src/permissions/permission-check/permission-check.controller.spec.ts b/src/permissions/permission-check/permission-check.controller.spec.ts
@@ -1,8 +1,14 @@
-import { BadGatewayException } from '@nestjs/common';
+import {
+  BadGatewayException,
+  BadRequestException,
+  HttpException,
+} from '@nestjs/common';
 import { Test, TestingModule } from '@nestjs/testing';
 import { AxiosError, AxiosHeaders, AxiosResponse } from 'axios';
+import { of } from 'rxjs';
 import { authGuardMockProviders } from '../../auth/auth-guard-mock.providers';
-import { CombinedAuthGuard } from '../../auth/guards/combined-auth/combined-auth.guard';
+import { BasicAuthGuard } from '../../auth/guards/basic-auth/basic-auth.guard';
+import { CouchdbService } from '../../couchdb/couchdb.service';
 import { UserInfo } from '../../restricted-endpoints/session/user-auth.dto';
 import { PermissionService } from '../permission/permission.service';
 import { UserIdentityService } from '../user-identity/user-identity.service';
@@ -12,6 +18,7 @@ describe('PermissionCheckController', () => {
   let controller: PermissionCheckController;
   let mockUserIdentityService: UserIdentityService;
   let mockPermissionService: PermissionService;
+  let mockCouchdbService: CouchdbService;
 
   beforeEach(async () => {
     mockUserIdentityService = {
@@ -20,14 +27,18 @@ describe('PermissionCheckController', () => {
     mockPermissionService = {
       isAllowedTo: jest.fn(),
     } as any;
+    mockCouchdbService = {
+      get: jest.fn().mockReturnValue(of({ _id: 'Child:1' })),
+    } as any;
 
     const module: TestingModule = await Test.createTestingModule({
       controllers: [PermissionCheckController],
       providers: [
         ...authGuardMockProviders,
-        { provide: CombinedAuthGuard, useValue: {} },
+        { provide: BasicAuthGuard, useValue: {} },
         { provide: UserIdentityService, useValue: mockUserIdentityService },
         { provide: PermissionService, useValue: mockPermissionService },
+        { provide: CouchdbService, useValue: mockCouchdbService },
       ],
     }).compile();
 
@@ -47,10 +58,11 @@ describe('PermissionCheckController', () => {
 
     const result = await controller.checkPermissions({
       userIds: ['u1', 'u2'],
-      entityDoc: { _id: 'Child:1' },
+      entityId: 'Child:1',
       action: 'read',
     });
 
+    expect(mockCouchdbService.get).toHaveBeenCalledWith('app', 'Child:1');
     expect(result).toEqual({
       u1: { permitted: true },
       u2: { permitted: false },
@@ -64,7 +76,7 @@ describe('PermissionCheckController', () => {
 
     const result = await controller.checkPermissions({
       userIds: ['u1'],
-      entityDoc: { _id: 'Child:1' },
+      entityId: 'Child:1',
       action: 'read',
     });
 
@@ -93,7 +105,21 @@ describe('PermissionCheckController', () => {
 
     const result = await controller.checkPermissions({
       userIds: ['u1'],
-      entityDoc: { _id: 'Child:1' },
+      entityId: 'Child:1',
+      action: 'read',
+    });
+
+    expect(result).toEqual({ u1: { permitted: false, error: 'NOT_FOUND' } });
+  });
+
+  it('should return NOT_FOUND when user lookup throws HttpException 404', async () => {
+    jest
+      .spyOn(mockUserIdentityService, 'resolveUser')
+      .mockRejectedValue(new HttpException('User not found', 404));
+
+    const result = await controller.checkPermissions({
+      userIds: ['u1'],
+      entityId: 'Child:1',
       action: 'read',
     });
 
@@ -110,7 +136,7 @@ describe('PermissionCheckController', () => {
     await expect(
       controller.checkPermissions({
         userIds: ['u1'],
-        entityDoc: { _id: 'Child:1' },
+        entityId: 'Child:1',
         action: 'read',
       }),
     ).rejects.toThrow(BadGatewayException);
@@ -139,9 +165,45 @@ describe('PermissionCheckController', () => {
     await expect(
       controller.checkPermissions({
         userIds: ['u1'],
-        entityDoc: { _id: 'Child:1' },
+        entityId: 'Child:1',
         action: 'read',
       }),
     ).rejects.toThrow(BadGatewayException);
   });
+
+  it('should throw BadRequestException for malformed userIds', async () => {
+    await expect(
+      controller.checkPermissions({
+        userIds: 'u1' as any,
+        entityId: 'Child:1',
+        action: 'read',
+      }),
+    ).rejects.toThrow('userIds is required');
+  });
+
+  it('should throw BadRequestException for invalid action', async () => {
+    await expect(
+      controller.checkPermissions({
+        userIds: ['u1'],
+        entityId: 'Child:1',
+        action: 'approve' as any,
+      }),
+    ).rejects.toThrow('action is invalid');
+  });
+
+  it('should throw BadRequestException when entity document is not found', async () => {
+    jest.spyOn(mockCouchdbService, 'get').mockReturnValue({
+      subscribe: (observer) => {
+        observer.error(new HttpException('not found', 404));
+      },
+    } as any);
+
+    await expect(
+      controller.checkPermissions({
+        userIds: ['u1'],
+        entityId: 'Child:doesnotexist',
+        action: 'read',
+      }),
+    ).rejects.toThrow(BadRequestException);
+  });
 });
diff --git a/src/permissions/permission-check/permission-check.controller.ts b/src/permissions/permission-check/permission-check.controller.ts
@@ -3,6 +3,7 @@ import {
   BadRequestException,
   Body,
   Controller,
+  HttpException,
   Logger,
   Post,
   UseGuards,
@@ -17,9 +18,10 @@ import {
   ApiUnauthorizedResponse,
 } from '@nestjs/swagger';
 import { AxiosError } from 'axios';
-import { CombinedAuthGuard } from '../../auth/guards/combined-auth/combined-auth.guard';
-import { OnlyAuthenticated } from '../../auth/only-authenticated.decorator';
-import { PermissionService } from '../permission/permission.service';
+import { firstValueFrom } from 'rxjs';
+import { BasicAuthGuard } from '../../auth/guards/basic-auth/basic-auth.guard';
+import { CouchdbService } from '../../couchdb/couchdb.service';
+import { Action, PermissionService } from '../permission/permission.service';
 import { UserIdentityService } from '../user-identity/user-identity.service';
 import { PermissionCheckRequestDto } from './permission-check.request.dto';
 
@@ -28,15 +30,22 @@ import { PermissionCheckRequestDto } from './permission-check.request.dto';
  */
 @ApiTags('permissions')
 @ApiBasicAuth('BasicAuth')
-@OnlyAuthenticated()
-@UseGuards(CombinedAuthGuard)
+@UseGuards(BasicAuthGuard)
 @Controller('permissions')
 export class PermissionCheckController {
   private readonly logger = new Logger(PermissionCheckController.name);
+  private static readonly VALID_ACTIONS = new Set<Action>([
+    'read',
+    'create',
+    'update',
+    'delete',
+    'manage',
+  ]);
 
   constructor(
     private readonly userIdentityService: UserIdentityService,
     private readonly permissionService: PermissionService,
+    private readonly couchdbService: CouchdbService,
   ) {}
 
   /**
@@ -71,30 +80,49 @@ export class PermissionCheckController {
   })
   @ApiBadRequestResponse({
     description:
-      'Request payload is invalid (e.g. missing userIds or entityDoc._id).',
+      'Request payload is invalid (e.g. missing userIds or entityId).',
   })
   @ApiUnauthorizedResponse({ description: 'Authentication required.' })
   async checkPermissions(@Body() body: PermissionCheckRequestDto) {
     this.logger.debug(
-      `Incoming permission check: userIds=${JSON.stringify(body?.userIds)}, ` +
-        `entityDoc._id=${body?.entityDoc?._id}, action=${body?.action}, ` +
+      `Incoming permission check: userCount=${Array.isArray(body?.userIds) ? body.userIds.length : 0}, ` +
+        `entityId=${body?.entityId}, action=${body?.action ?? 'read'}, ` +
         `body keys=${body ? Object.keys(body) : 'null'}`,
     );
-    if (!body?.userIds?.length) {
+
+    if (
+      !Array.isArray(body?.userIds) ||
+      body.userIds.length === 0 ||
+      body.userIds.some((id) => typeof id !== 'string' || id.trim() === '')
+    ) {
       throw new BadRequestException('userIds is required');
     }
-    if (!body?.entityDoc?._id) {
-      throw new BadRequestException('entityDoc._id is required');
+
+    if (
+      !body?.entityId ||
+      typeof body.entityId !== 'string' ||
+      body.entityId.trim() === ''
+    ) {
+      throw new BadRequestException('entityId is required');
+    }
+
+    if (
+      body.action &&
+      !PermissionCheckController.VALID_ACTIONS.has(body.action)
+    ) {
+      throw new BadRequestException('action is invalid');
     }
 
-    const action = body.action || 'read';
+    const action: Action = body.action ?? 'read';
+    const entityDoc = await this.loadCanonicalEntityDoc(body.entityId);
+
     const results = await Promise.all(
       body.userIds.map(async (userId) => {
         try {
           const user = await this.userIdentityService.resolveUser(userId);
           const permitted = await this.permissionService.isAllowedTo(
             action,
-            body.entityDoc,
+            entityDoc,
             user,
             'app',
           );
@@ -111,8 +139,15 @@ export class PermissionCheckController {
             );
           }
 
-          // User not found in Keycloak
-          if (error instanceof AxiosError && error.response?.status === 404) {
+          // User not found in Keycloak (404) or bad user ID format (400)
+          if (
+            (error instanceof AxiosError &&
+              error.response?.status >= 400 &&
+              error.response?.status < 500) ||
+            (error instanceof HttpException &&
+              error.getStatus() >= 400 &&
+              error.getStatus() < 500)
+          ) {
             return [userId, { permitted: false, error: 'NOT_FOUND' }] as const;
           }
 
@@ -127,4 +162,20 @@ export class PermissionCheckController {
 
     return Object.fromEntries(results);
   }
+
+  private async loadCanonicalEntityDoc(entityId: string) {
+    try {
+      return await firstValueFrom(this.couchdbService.get('app', entityId));
+    } catch (error) {
+      if (error instanceof HttpException && error.getStatus() === 404) {
+        throw new BadRequestException(`entityDoc not found: ${entityId}`);
+      }
+
+      this.logger.error(
+        `Failed to load canonical entity document ${entityId}`,
+        error?.stack || error,
+      );
+      throw new BadGatewayException('Failed to load target entity document');
+    }
+  }
 }
diff --git a/src/permissions/permission-check/permission-check.request.dto.ts b/src/permissions/permission-check/permission-check.request.dto.ts
@@ -1,5 +1,4 @@
 import { ApiProperty } from '@nestjs/swagger';
-import { DatabaseDocument } from '../../restricted-endpoints/replication/bulk-document/couchdb-dtos/bulk-docs.dto';
 import { Action } from '../permission/permission.service';
 
 /**
@@ -13,15 +12,12 @@ export class PermissionCheckRequestDto {
   userIds: string[];
 
   @ApiProperty({
-    description: 'Target entity document used for permission evaluation.',
-    type: 'object',
-    additionalProperties: true,
-    required: ['_id'],
-    properties: {
-      _id: { type: 'string' },
-    },
+    description:
+      'The _id of the target entity document to check permissions against.',
+    type: 'string',
+    example: 'Child:1',
   })
-  entityDoc: DatabaseDocument;
+  entityId: string;
 
   @ApiProperty({
     description: 'Action to evaluate.',
diff --git a/src/permissions/permission.module.ts b/src/permissions/permission.module.ts
@@ -26,7 +26,7 @@ export const KEYCLOAK_HTTP_SERVICE = 'KEYCLOAK_HTTP_SERVICE';
       // CouchDB basic-auth headers and an error interceptor, which would
       // cause Keycloak admin API requests to fail with 401.
       provide: KEYCLOAK_HTTP_SERVICE,
-      useFactory: () => new HttpService(axios.create()),
+      useFactory: () => new HttpService(axios.create({ timeout: 5000 })),
     },
     {
       provide: UserAdminService,
diff --git a/src/permissions/rules/rules.service.ts b/src/permissions/rules/rules.service.ts
diff --git a/src/permissions/user-identity/user-identity.service.spec.ts b/src/permissions/user-identity/user-identity.service.spec.ts

Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@ export const KEYCLOAK_HTTP_SERVICE = 'KEYCLOAK_HTTP_SERVICE';`
`26`	`26`	`// CouchDB basic-auth headers and an error interceptor, which would`
`27`	`27`	`// cause Keycloak admin API requests to fail with 401.`
`28`	`28`	`provide: KEYCLOAK_HTTP_SERVICE,`
`29`		`- useFactory: () => new HttpService(axios.create()),`
	`29`	`+ useFactory: () => new HttpService(axios.create({ timeout: 5000 })),`
`30`	`30`	`},`
`31`	`31`	`{`
`32`	`32`	`provide: UserAdminService,`