From f382a7635e299327fc52b322dc1a0bb66526004f Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 17 Aug 2026 05:51:44 +0200 Subject: [PATCH] refactor(collab): note-file deletion through storage --- server/src/nest/collab/collab.controller.ts | 8 +++---- server/src/nest/collab/collab.mcp.ts | 2 +- server/src/nest/collab/collab.service.ts | 22 +++++++++---------- server/tests/helpers/mcp-test-controllers.ts | 4 ++-- server/tests/helpers/plugin-host.ts | 4 ++-- .../tests/unit/nest/collab.controller.test.ts | 16 +++++++------- server/tests/unit/nest/collab.service.test.ts | 12 +++++----- .../unit/nest/trip-read-model.service.test.ts | 2 +- server/tests/unit/nest/trips.service.test.ts | 2 +- 9 files changed, 36 insertions(+), 36 deletions(-) diff --git a/server/src/nest/collab/collab.controller.ts b/server/src/nest/collab/collab.controller.ts index 04c7ffe4f..a8a54bb7e 100644 --- a/server/src/nest/collab/collab.controller.ts +++ b/server/src/nest/collab/collab.controller.ts @@ -135,8 +135,8 @@ export class CollabController { @UseGuards(TripAccessGuard) @RequirePermission('collab_edit') @Delete('notes/:id') - deleteNote(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, @Headers('x-socket-id') socketId?: string) { - if (!this.collab.deleteNote(tripId, id)) { + async deleteNote(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, @Headers('x-socket-id') socketId?: string) { + if (!(await this.collab.deleteNote(tripId, id))) { throw new HttpException({ error: 'Note not found' }, 404); } this.collab.broadcast(tripId, 'collab:note:deleted', { noteId: Number(id) }, socketId); @@ -167,8 +167,8 @@ export class CollabController { @UseGuards(TripAccessGuard) @RequirePermission('collab_edit') @Delete('notes/:id/files/:fileId') - deleteNoteFile(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, @Param('fileId') fileId: string, @Headers('x-socket-id') socketId?: string) { - if (!this.collab.deleteNoteFile(tripId, id, fileId)) { + async deleteNoteFile(@CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, @Param('fileId') fileId: string, @Headers('x-socket-id') socketId?: string) { + if (!(await this.collab.deleteNoteFile(tripId, id, fileId))) { throw new HttpException({ error: 'File not found' }, 404); } this.collab.broadcast(tripId, 'collab:note:updated', { note: this.collab.getFormattedNoteById(tripId, id) }, socketId); diff --git a/server/src/nest/collab/collab.mcp.ts b/server/src/nest/collab/collab.mcp.ts index 7a4bec645..b6c4893c8 100644 --- a/server/src/nest/collab/collab.mcp.ts +++ b/server/src/nest/collab/collab.mcp.ts @@ -145,7 +145,7 @@ export class CollabMcp { if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); if (!this.collab.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!this.guards.hasTripPermission('collab_edit', tripId, ctx.userId)) return permissionDenied(); - const deleted = this.collab.deleteNote(tripId, noteId); + const deleted = await this.collab.deleteNote(tripId, noteId); if (!deleted) return errorResult('Note not found.'); this.guards.safeBroadcast(tripId, 'collab:note:deleted', { noteId }); return ok({ success: true }); diff --git a/server/src/nest/collab/collab.service.ts b/server/src/nest/collab/collab.service.ts index e6e94e6ac..efd1b2d1b 100644 --- a/server/src/nest/collab/collab.service.ts +++ b/server/src/nest/collab/collab.service.ts @@ -1,15 +1,14 @@ import path from 'path'; -import fs from 'fs'; import { Injectable } from '@nestjs/common'; import { DatabaseService, type TripAccess } from '../database/database.service'; import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; import { avatarUrl } from '../common/avatarUrl'; -import { filesDir } from '../files/files.constants'; import { checkSsrf, createPinnedDispatcher } from '../../utils/ssrfGuard'; import type { CollabNote, CollabPoll, CollabMessage, TripFile, User } from '../../types'; import { NotificationsService } from '../notifications/notifications.service'; +import { StorageService } from '../storage/storage.service'; type Trip = TripAccess; @@ -70,6 +69,7 @@ export class CollabService { private readonly permissions: PermissionsService, private readonly realtime: RealtimeService, private readonly notifications: NotificationsService, + private readonly storage: StorageService, ) {} verifyTripAccess(tripId: string | number, userId: number) { @@ -194,18 +194,17 @@ export class CollabService { return this.formatNote(note); } - deleteNote(tripId: string | number, noteId: string | number): boolean { + async deleteNote(tripId: string | number, noteId: string | number): Promise { const existing = this.db.get('SELECT id FROM collab_notes WHERE id = ? AND trip_id = ?', noteId, tripId); if (!existing) return false; - // Clean up attached files from disk (unlink-first is intentional — a - // failed row delete leaves dangling rows, never orphaned files). - // basename() tolerates any legacy 'files/'-prefixed row the boot migration - // has not seen; the raw unlink moves to storage.delete in the serving slice. + // Clean up attached objects first (delete-first is intentional — a failed + // row delete leaves dangling rows, never orphaned files). basename() + // tolerates any legacy 'files/'-prefixed row the boot migration has not + // seen. const noteFiles = this.db.all('SELECT id, filename FROM trip_files WHERE note_id = ?', noteId); for (const f of noteFiles) { - const filePath = path.join(filesDir, path.basename(f.filename)); - try { fs.unlinkSync(filePath); } catch { /* ignore */ } + await this.storage.delete('files', path.basename(f.filename)).catch(() => { /* ignore */ }); } this.db.transaction(() => { this.db.run('DELETE FROM trip_files WHERE note_id = ?', noteId); @@ -237,7 +236,7 @@ export class CollabService { return this.formatNote(note); } - deleteNoteFile(tripId: string | number, noteId: string | number, fileId: string | number): boolean { + async deleteNoteFile(tripId: string | number, noteId: string | number, fileId: string | number): Promise { // Scope to the trip — like every sibling collab op — so a caller authorized for THEIR // trip can't delete a note-file that belongs to someone else's trip (IDOR). trip_files // carries trip_id, so this ties the deleted object to the URL's :tripId the controller @@ -245,8 +244,7 @@ export class CollabService { const file = this.db.get('SELECT * FROM trip_files WHERE id = ? AND note_id = ? AND trip_id = ?', fileId, noteId, tripId); if (!file) return false; - const filePath = path.join(filesDir, path.basename(file.filename)); - try { fs.unlinkSync(filePath); } catch { /* ignore */ } + await this.storage.delete('files', path.basename(file.filename)).catch(() => { /* ignore */ }); this.db.run('DELETE FROM trip_files WHERE id = ?', fileId); return true; diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 20c3293f3..198837143 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -86,6 +86,7 @@ import { makeStorageFixture } from './storage-fixture'; */ export function createMcpTestRegistry(): McpRegistry { const dbService = new DatabaseService(db); + const generalStorage = makeStorageFixture('').storage; const permissionsService = new PermissionsService(dbService); // Same argument list as auth.bridge.ts. AtlasService used to sit in third // place; when getTravelStats moved onto AtlasService itself the edge was @@ -110,11 +111,10 @@ export function createMcpTestRegistry(): McpRegistry { const daysService = new DaysService(dbService, permissionsService, realtimeService, queryHelpersService); const todoService = new TodoService(dbService, permissionsService, realtimeService); const packingService = new PackingService(dbService, permissionsService, realtimeService, notificationsStub()); - const collabService = new CollabService(dbService, permissionsService, realtimeService, notificationsStub()); + const collabService = new CollabService(dbService, permissionsService, realtimeService, notificationsStub(), generalStorage); // Exactly one instance, shared by maps, places and share: its stampede guard // and its on-disk set only work if all three readers see the same maps. const placePhotoCache = new PlacePhotoCacheService(dbService, makeStorageFixture('photos/google/').storage); - const generalStorage = makeStorageFixture('').storage; const mapsService = new MapsService(dbService, placePhotoCache); const journeyDomain = new JourneyDomainService(dbService, realtimeService, new TrekPhotosRepository(dbService)); // The last three were previously omitted, which left them `undefined` at diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index 60fa852cc..e0bf6340d 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -83,6 +83,7 @@ import { makeStorageFixture } from './storage-fixture'; * container would discover, handed to the host factory as a registry. */ export function createPluginRpcHostFactory(dbs: DatabaseService): PluginRpcHostFactory { + const generalStorage = makeStorageFixture('').storage; const permissions = new PermissionsService(dbs); const exchangeRates = new ExchangeRatesService(); const realtime = new RealtimeService(); @@ -93,11 +94,10 @@ export function createPluginRpcHostFactory(dbs: DatabaseService): PluginRpcHostF const packing = new PackingService(dbs, permissions, realtime, notificationsStub()); const files = new FilesService(dbs, permissions, realtime, new EphemeralTokenService()); const reservations = new ReservationsService(dbs, permissions, budget, realtime, notificationsStub(), new ReservationsReadRepository(dbs)); - const collab = new CollabService(dbs, permissions, realtime, notificationsStub()); + const collab = new CollabService(dbs, permissions, realtime, notificationsStub(), generalStorage); const vacay = new VacayService(dbs, realtime, notificationsStub()); const days = new DaysService(dbs, permissions, realtime, queryHelpers); const photoCache = new PlacePhotoCacheService(dbs, makeStorageFixture('photos/google/').storage); - const generalStorage = makeStorageFixture('').storage; const unsplash = new UnsplashService(dbs, new RuntimeEnvService(), generalStorage); const journey = new JourneyDomainService(dbs, realtime, new TrekPhotosRepository(dbs)); const places = new PlacesService(dbs, permissions, realtime, new MapsService(dbs, photoCache), queryHelpers, unsplash, photoCache, journey, generalStorage); diff --git a/server/tests/unit/nest/collab.controller.test.ts b/server/tests/unit/nest/collab.controller.test.ts index 8fe14df45..29292f8ac 100644 --- a/server/tests/unit/nest/collab.controller.test.ts +++ b/server/tests/unit/nest/collab.controller.test.ts @@ -67,11 +67,11 @@ describe('CollabController (parity with the legacy /api/trips/:tripId/collab rou expect(broadcast).toHaveBeenCalledWith('5', 'collab:note:updated', { note: { id: 9 } }, 'sock'); }); - it('DELETE 404 when missing, else success + broadcasts', () => { - expect(thrown(() => new CollabController(svc({ deleteNote: vi.fn().mockReturnValue(false) } as Partial), storageStub).deleteNote(user, '5', '9'))).toEqual({ status: 404, body: { error: 'Note not found' } }); + it('DELETE 404 when missing, else success + broadcasts', async () => { + expect(await thrownAsync(() => new CollabController(svc({ deleteNote: vi.fn().mockResolvedValue(false) } as Partial), storageStub).deleteNote(user, '5', '9'))).toEqual({ status: 404, body: { error: 'Note not found' } }); const broadcast = vi.fn(); - const s = svc({ deleteNote: vi.fn().mockReturnValue(true), broadcast } as Partial); - expect(new CollabController(s, storageStub).deleteNote(user, '5', '9', 'sock')).toEqual({ success: true }); + const s = svc({ deleteNote: vi.fn().mockResolvedValue(true), broadcast } as Partial); + expect(await new CollabController(s, storageStub).deleteNote(user, '5', '9', 'sock')).toEqual({ success: true }); expect(broadcast).toHaveBeenCalledWith('5', 'collab:note:deleted', { noteId: 9 }, 'sock'); }); }); @@ -89,10 +89,10 @@ describe('CollabController (parity with the legacy /api/trips/:tripId/collab rou expect(broadcast).toHaveBeenCalledWith('5', 'collab:note:updated', { note: { id: 9 } }, 'sock'); }); - it('DELETE file 404 when missing, else success', () => { - expect(thrown(() => new CollabController(svc({ deleteNoteFile: vi.fn().mockReturnValue(false) } as Partial), storageStub).deleteNoteFile(user, '5', '9', '3'))).toEqual({ status: 404, body: { error: 'File not found' } }); - const s = svc({ deleteNoteFile: vi.fn().mockReturnValue(true), getFormattedNoteById: vi.fn().mockReturnValue({ id: 9 }), broadcast: vi.fn() } as Partial); - expect(new CollabController(s, storageStub).deleteNoteFile(user, '5', '9', '3')).toEqual({ success: true }); + it('DELETE file 404 when missing, else success', async () => { + expect(await thrownAsync(() => new CollabController(svc({ deleteNoteFile: vi.fn().mockResolvedValue(false) } as Partial), storageStub).deleteNoteFile(user, '5', '9', '3'))).toEqual({ status: 404, body: { error: 'File not found' } }); + const s = svc({ deleteNoteFile: vi.fn().mockResolvedValue(true), getFormattedNoteById: vi.fn().mockReturnValue({ id: 9 }), broadcast: vi.fn() } as Partial); + expect(await new CollabController(s, storageStub).deleteNoteFile(user, '5', '9', '3')).toEqual({ success: true }); }); }); diff --git a/server/tests/unit/nest/collab.service.test.ts b/server/tests/unit/nest/collab.service.test.ts index a64990f66..ecb0cee68 100644 --- a/server/tests/unit/nest/collab.service.test.ts +++ b/server/tests/unit/nest/collab.service.test.ts @@ -67,8 +67,10 @@ import { PermissionsService } from '../../../src/nest/permissions/permissions.se import { CollabService } from '../../../src/nest/collab/collab.service'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { notificationsStub } from '../../helpers/notifications'; +import { makeStorageFixture } from '../../helpers/storage-fixture'; -const svc = new CollabService(new DatabaseService(testDb), new PermissionsService(new DatabaseService(testDb)), new RealtimeService(), notificationsStub()); +const collabFx = makeStorageFixture('files/'); +const svc = new CollabService(new DatabaseService(testDb), new PermissionsService(new DatabaseService(testDb)), new RealtimeService(), notificationsStub(), collabFx.storage); beforeAll(() => { createTables(testDb); @@ -421,7 +423,7 @@ describe('hardening', () => { it('COLLAB-SVC-034: votePoll switch is atomic — prior vote survives a failed INSERT', () => { const { user1, trip } = setup(); const dbs = new DatabaseService(testDb); - const failing = new CollabService(dbs, new PermissionsService(dbs), new RealtimeService(), notificationsStub()); + const failing = new CollabService(dbs, new PermissionsService(dbs), new RealtimeService(), notificationsStub(), collabFx.storage); const poll = failing.createPoll(trip.id, user1.id, { question: 'Q?', options: ['A', 'B'] }); failing.votePoll(trip.id, poll!.id, user1.id, 0); @@ -439,10 +441,10 @@ describe('hardening', () => { expect(votes).toEqual([{ option_index: 0 }]); }); - it('COLLAB-SVC-035: deleteNote is atomic — trip_files rows survive a failed note DELETE', () => { + it('COLLAB-SVC-035: deleteNote is atomic — trip_files rows survive a failed note DELETE', async () => { const { user1, trip } = setup(); const dbs = new DatabaseService(testDb); - const failing = new CollabService(dbs, new PermissionsService(dbs), new RealtimeService(), notificationsStub()); + const failing = new CollabService(dbs, new PermissionsService(dbs), new RealtimeService(), notificationsStub(), collabFx.storage); const note = failing.createNote(trip.id, user1.id, { title: 'With file' }); testDb.prepare('INSERT INTO trip_files (trip_id, note_id, filename, original_name) VALUES (?, ?, ?, ?)') .run(trip.id, note.id, 'files/a.pdf', 'a.pdf'); @@ -452,7 +454,7 @@ describe('hardening', () => { if (sql.includes('DELETE FROM collab_notes')) throw new Error('boom'); return realRun(sql, ...params); }); - expect(() => failing.deleteNote(trip.id, note.id)).toThrow('boom'); + await expect(failing.deleteNote(trip.id, note.id)).rejects.toThrow('boom'); spy.mockRestore(); expect(testDb.prepare('SELECT COUNT(*) as c FROM trip_files WHERE note_id = ?').get(note.id)).toEqual({ c: 1 }); diff --git a/server/tests/unit/nest/trip-read-model.service.test.ts b/server/tests/unit/nest/trip-read-model.service.test.ts index eb3f31747..e0616faea 100644 --- a/server/tests/unit/nest/trip-read-model.service.test.ts +++ b/server/tests/unit/nest/trip-read-model.service.test.ts @@ -100,7 +100,7 @@ const buildReadModel = (database: DatabaseService, roster: TripMembersService = database, roster, daysSvc, accommodationsSvc, budgetSvc, new PackingService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub()), new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs())), - new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub()), + new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub(), makeStorageFixture('').storage), placesSvc, new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), new FilesService(dbs(), new PermissionsService(dbs()), new RealtimeService(), new EphemeralTokenService()), diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index ac2510f4b..d13222754 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -130,7 +130,7 @@ const readModelSvc = new TripReadModelService( dbs(), membersSvc, daysSvc, accommodationsSvc, budgetSvc, new PackingService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub()), new ReservationsService(dbs(), new PermissionsService(dbs()), budgetSvc, new RealtimeService(), notificationsStub(), new ReservationsReadRepository(dbs())), - new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub()), + new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub(), coversFx.storage), placesSvc, new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), new FilesService(dbs(), new PermissionsService(dbs()), new RealtimeService(), new EphemeralTokenService()),