From 36a0d1eabab65776139ea9261bd06e6876a638b0 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 15:56:09 +0200 Subject: [PATCH] fix(auth): erase plugin user data in the transaction that deletes the user erasePluginUserData deleted the host-side plugin rows and then queued the plugin erasures as separate commits, and deleteGuest ran it before its own transaction. When the guest delete failed, the guest kept its account but had lost its plugin rows, with erasures queued for a user that still exists. The erasure is now one transaction, and deleteGuest runs it inside the same transaction as the budget re-split and the user delete, so it becomes a savepoint of that write. Only DB writes and a synchronous readdir run inside. --- server/scripts/tx-writes-baseline.json | 2 - server/src/nest/auth/user-cleanup.service.ts | 37 +++++++++++-------- .../nest/trip-members/trip-members.service.ts | 16 ++++---- .../unit/nest/trip-members.service.test.ts | 28 ++++++++++++++ 4 files changed, 58 insertions(+), 25 deletions(-) diff --git a/server/scripts/tx-writes-baseline.json b/server/scripts/tx-writes-baseline.json index 05dcbee14..8b6101c56 100644 --- a/server/scripts/tx-writes-baseline.json +++ b/server/scripts/tx-writes-baseline.json @@ -8,7 +8,6 @@ "src/nest/auth/passkey.service.ts#PasskeyService.passkeyLoginVerify": 2, "src/nest/auth/passkey.service.ts#PasskeyService.passkeyRegisterOptions": 2, "src/nest/auth/passkey.service.ts#PasskeyService.passkeyRegisterVerify": 2, - "src/nest/auth/user-cleanup.service.ts#UserCleanupService.erasePluginUserData": 2, "src/nest/booking-import/booking-import.service.ts#BookingImportService.confirm": 4, "src/nest/doc-sync/doc-sync-config.service.ts#DocSyncConfigService.upsertConnection": 3, "src/nest/doc-sync/doc-sync.controller.ts#DocSyncController.createLink": 4, @@ -97,7 +96,6 @@ "src/nest/plugins/plugins.controller.ts#PluginsController.update": 2, "src/nest/plugins/plugins.controller.ts#PluginsController.updateConfig": 3, "src/nest/reservations/reservations.controller.ts#ReservationsController.update": 2, - "src/nest/trip-members/trip-members.service.ts#TripMembersService.deleteGuest": 2, "src/nest/trips/trips.service.ts#TripsService.update": 2, "src/nest/vacay/vacay.service.ts#VacayService.updatePlan": 2 } diff --git a/server/src/nest/auth/user-cleanup.service.ts b/server/src/nest/auth/user-cleanup.service.ts index 48aa522ae..651b981d6 100644 --- a/server/src/nest/auth/user-cleanup.service.ts +++ b/server/src/nest/auth/user-cleanup.service.ts @@ -116,22 +116,29 @@ export class UserCleanupService { * here, now via the same repository. */ async erasePluginUserData(userId: number): Promise { - await this.maintenance.deletePluginUserData(userId); // UC1, Plan 4 Task 4 - try { - const rows = await this.pluginsRepo.listIdsAndPermissions(); // UC2 — Plan 4 Task 8a - const installed = new Set(rows.map((r) => r.id)); - await enqueueHookUserDataErasures(this.pluginUserErasureQueueRepo, rows, userId); // UC3 — Plan 4 Task 8a - // Also enqueue for plugins UNINSTALLED with their data retained (deleteData=false): - // their data dir still holds the user's rows and a same-id reinstall would re-adopt - // them. No permissions record survives uninstall, so we can't check hook:user-data — - // enqueue for every orphan data dir; the row sits inert (no FK) and drains only if - // that id is reinstalled + active (erasure delivery is a duty, not grant-gated). + // The delete and the erasures it queues are one write: the host rows never go + // without the queue rows that purge the plugins' own copies. Only DB writes and + // a synchronous readdir run in here, no network I/O. A caller that deletes the + // user (deleteUserCompletely, TripMembersService.deleteGuest) calls this inside + // its own transaction, so this becomes a savepoint of it. + await this.uow.transactional(async () => { + await this.maintenance.deletePluginUserData(userId); // UC1, Plan 4 Task 4 try { - for (const entry of fs.readdirSync(pluginsDataRoot(), { withFileTypes: true })) { - if (entry.isDirectory() && !installed.has(entry.name)) await this.pluginUserErasureQueueRepo.insertIgnore(entry.name, userId); - } - } catch { /* no plugin data root yet */ } - } catch { /* plugins / queue table absent (slim schema) */ } + const rows = await this.pluginsRepo.listIdsAndPermissions(); // UC2, Plan 4 Task 8a + const installed = new Set(rows.map((r) => r.id)); + await enqueueHookUserDataErasures(this.pluginUserErasureQueueRepo, rows, userId); // UC3, Plan 4 Task 8a + // Also enqueue for plugins UNINSTALLED with their data retained (deleteData=false): + // their data dir still holds the user's rows and a same-id reinstall would re-adopt + // them. No permissions record survives uninstall, so we can't check hook:user-data; + // enqueue for every orphan data dir; the row sits inert (no FK) and drains only if + // that id is reinstalled + active (erasure delivery is a duty, not grant-gated). + try { + for (const entry of fs.readdirSync(pluginsDataRoot(), { withFileTypes: true })) { + if (entry.isDirectory() && !installed.has(entry.name)) await this.pluginUserErasureQueueRepo.insertIgnore(entry.name, userId); + } + } catch { /* no plugin data root yet */ } + } catch { /* plugins / queue table absent (slim schema) */ } + }); } /** diff --git a/server/src/nest/trip-members/trip-members.service.ts b/server/src/nest/trip-members/trip-members.service.ts index 055d4597f..8aeadc5d6 100644 --- a/server/src/nest/trip-members/trip-members.service.ts +++ b/server/src/nest/trip-members/trip-members.service.ts @@ -279,15 +279,15 @@ export class TripMembersService { async deleteGuest(tripId: string | number, guestUserId: number): Promise { if (!(await this.guestOfTrip(tripId, guestUserId))) return false; - // A guest is still a user id a plugin may hold data for, so erase that too — the - // host-side per-user tables + a durable own-db erasure per granted plugin — exactly - // like a full account deletion (otherwise a deleted guest's plugin data lingers). - await this.userCleanup.erasePluginUserData(guestUserId); - // Quirk fix on top of the 1:1 move: the budget re-split and the user delete - // run in one transaction, so a failure mid-flow can't leave the expense - // divisors re-derived for a guest that still exists (or vice versa). The - // plugin-side erasure/notification keep their order around it. + // The plugin erasure, the budget re-split and the user delete run in one + // transaction, so a failure mid-flow can't leave the expense divisors re-derived, + // or the guest's plugin rows erased, for a guest that still exists (or vice + // versa). The plugin notification goes out after the commit. await this.uow.transactional(async () => { + // A guest is still a user id a plugin may hold data for, so erase that too (the + // host-side per-user tables plus a durable own-db erasure per granted plugin), exactly + // like a full account deletion; otherwise a deleted guest's plugin data lingers. + await this.userCleanup.erasePluginUserData(guestUserId); // Re-split the expenses they were part of before the cascade takes their member // rows away — the divisor is denormalized and cannot follow a foreign key (#1553). await this.budget.removeUserFromBudgetItems(guestUserId); diff --git a/server/tests/unit/nest/trip-members.service.test.ts b/server/tests/unit/nest/trip-members.service.test.ts index 5ae44fa41..38f3faf6d 100644 --- a/server/tests/unit/nest/trip-members.service.test.ts +++ b/server/tests/unit/nest/trip-members.service.test.ts @@ -430,6 +430,34 @@ describe('Task 6 review items — rollback and concurrency', () => { expect(after).toBe(before); }); + it('MEMBERS-SVC-019: deleteGuest rolls the plugin erasure back with the user delete, so a guest that survives keeps its plugin rows and no erasure is queued', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const { member: guest } = await roster.createGuest(trip.id, 'Rollback Ray', owner.id); + testDb.prepare('INSERT INTO plugins (id, name, version, permissions) VALUES (?, ?, ?, ?)') + .run('members-svc-019', 'members-svc-019', '1.0.0', JSON.stringify(['hook:user-data'])); + testDb.prepare('INSERT INTO plugin_user_config (plugin_id, user_id, config) VALUES (?, ?, ?)') + .run('members-svc-019', guest.id, '{"token":"keep-me"}'); + + const spy = vi.spyOn(usersRepo, 'deleteGuest').mockRejectedValueOnce(new Error('boom')); + try { + await expect(roster.deleteGuest(trip.id, guest.id)).rejects.toThrow('boom'); + + // The erasure ran first, then the users delete rejected. Erasing outside the + // transaction would have left the host rows deleted and an erasure queued for + // a guest that still exists. + expect(testDb.prepare('SELECT id FROM users WHERE id = ?').get(guest.id)).toBeDefined(); + expect(testDb.prepare('SELECT user_id FROM plugin_user_config WHERE user_id = ?').get(guest.id)).toBeDefined(); + expect(testDb.prepare('SELECT plugin_id FROM plugin_user_erasure_queue WHERE user_id = ?').all(guest.id)).toEqual([]); + } finally { + spy.mockRestore(); + // resetTestDb leaves the plugin tables alone, so this test cleans up its own rows. + for (const t of ['plugin_user_erasure_queue', 'plugin_user_config', 'plugins']) { + testDb.prepare(`DELETE FROM ${t} WHERE ${t === 'plugins' ? 'id' : 'plugin_id'} = ?`).run('members-svc-019'); + } + } + }); + it('MEMBERS-SVC-018 (concurrency, §18.4, copies TRIP-JOIN-005): two concurrent addMember calls for the same (trip, user) race the TM5→TM6 check-then-act window', async () => { const { user: owner } = createUser(testDb); const { user: invitee } = createUser(testDb);