fix: clean up dangling FK references before deleting a user

Resolves FOREIGN KEY constraint failed (500) on DELETE /api/admin/users/:id
and DELETE /api/auth/me when the target user had rows in trip_members.invited_by,
share_tokens.created_by, budget_items.paid_by_user_id, journeys.user_id,
journey_entries.author_id, journey_contributors.user_id, or
journey_share_tokens.created_by — none of which had ON DELETE clauses.

Introduces deleteUserCompletely() in userCleanupService.ts which wraps all
cleanup and the final DELETE FROM users in a single transaction. Both
adminService.deleteUser and authService.deleteAccount now call it instead of
the bare DELETE. Tests ADMIN-005b and AUTH-040 cover all reference types
including notification sender/recipient and notice dismissals.
This commit is contained in:
jubnl
2026-04-27 12:41:10 +02:00
parent ca832e8d88
commit 185a41831a
5 changed files with 164 additions and 4 deletions
+2 -1
View File
@@ -8,6 +8,7 @@ import { updateJwtSecret } from '../config';
import { maybe_encrypt_api_key, decrypt_api_key } from './apiKeyCrypto';
import { getAllPermissions, savePermissions as savePerms, PERMISSION_ACTIONS } from './permissions';
import { revokeUserSessions, revokeUserSessionsForClient } from '../mcp';
import { deleteUserCompletely } from './userCleanupService';
import { validatePassword } from './passwordPolicy';
import { getPhotoProviderConfig } from './memories/helpersService';
import { send as sendNotification } from './notificationService';
@@ -170,7 +171,7 @@ export function deleteUser(id: string, currentUserId: number) {
const userToDel = db.prepare('SELECT id, email FROM users WHERE id = ?').get(id) as { id: number; email: string } | undefined;
if (!userToDel) return { error: 'User not found', status: 404 };
db.prepare('DELETE FROM users WHERE id = ?').run(id);
deleteUserCompletely(userToDel.id);
return { email: userToDel.email };
}
+2 -1
View File
@@ -15,6 +15,7 @@ import { decrypt_api_key, maybe_encrypt_api_key, encrypt_api_key } from './apiKe
import { createEphemeralToken } from './ephemeralTokens';
import { revokeUserSessions } from '../mcp';
import { startTripReminders } from '../scheduler';
import { deleteUserCompletely } from './userCleanupService';
import { verifyJwtAndLoadUser } from '../middleware/auth';
import { User } from '../types';
import { DEMO_EMAIL_PRIMARY, isDemoEmail } from './demo';
@@ -527,7 +528,7 @@ export function deleteAccount(userId: number, userEmail: string, userRole: strin
return { error: 'Cannot delete the last admin account', status: 400 };
}
}
db.prepare('DELETE FROM users WHERE id = ?').run(userId);
deleteUserCompletely(userId);
return { success: true };
}
+21
View File
@@ -0,0 +1,21 @@
import { db } from '../db/database';
function cleanupUserReferences(userId: number): void {
db.prepare('UPDATE trip_members SET invited_by = NULL WHERE invited_by = ?').run(userId);
db.prepare('UPDATE budget_items SET paid_by_user_id = NULL WHERE paid_by_user_id = ?').run(userId);
db.prepare('DELETE FROM share_tokens WHERE created_by = ?').run(userId);
db.prepare('DELETE FROM journey_share_tokens WHERE created_by = ?').run(userId);
// Owned journeys cascade-delete their entries/contributors/share_tokens/photos via journey_id FKs
db.prepare('DELETE FROM journeys WHERE user_id = ?').run(userId);
// Entries authored on other users' journeys (not covered by the cascade above)
db.prepare('DELETE FROM journey_entries WHERE author_id = ?').run(userId);
db.prepare('DELETE FROM journey_contributors WHERE user_id = ?').run(userId);
}
export function deleteUserCompletely(userId: number): void {
const tx = db.transaction((id: number) => {
cleanupUserReferences(id);
db.prepare('DELETE FROM users WHERE id = ?').run(id);
});
tx(userId);
}
+65 -1
View File
@@ -41,7 +41,7 @@ import { createApp } from '../../src/app';
import { createTables } from '../../src/db/schema';
import { runMigrations } from '../../src/db/migrations';
import { resetTestDb } from '../helpers/test-db';
import { createUser, createAdmin, createInviteToken } from '../helpers/factories';
import { createUser, createAdmin, createInviteToken, createTrip, createBudgetItem, createJourney, createJourneyEntry, addJourneyContributor } from '../helpers/factories';
import { authCookie } from '../helpers/auth';
import { loginAttempts, mfaAttempts } from '../../src/routes/auth';
@@ -148,6 +148,70 @@ describe('Admin user management', () => {
expect(deleted).toBeUndefined();
});
it('ADMIN-005b — DELETE /admin/users/:id succeeds when user has FK references', async () => {
const { user: admin } = createAdmin(testDb);
const { user: target } = createUser(testDb);
const { user: otherUser } = createUser(testDb);
const { user: thirdUser } = createUser(testDb);
// trip_members.invited_by: target invited thirdUser to otherUser's trip
// (trip survives deletion; only invited_by should become NULL)
const otherTrip = createTrip(testDb, otherUser.id);
testDb.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(otherTrip.id, thirdUser.id, target.id);
// share_tokens.created_by: target created a share token for otherUser's trip
testDb.prepare("INSERT INTO share_tokens (trip_id, token, created_by) VALUES (?, 'tok-admin-test', ?)").run(otherTrip.id, target.id);
// budget_items.paid_by_user_id: target paid for an expense on otherUser's trip
const budgetItem = createBudgetItem(testDb, otherTrip.id);
testDb.prepare('UPDATE budget_items SET paid_by_user_id = ? WHERE id = ?').run(target.id, budgetItem.id);
// journey_contributors: target is a contributor on otherUser's journey
const otherJourney = createJourney(testDb, otherUser.id);
addJourneyContributor(testDb, otherJourney.id, target.id);
// journey_entries: target authored an entry on otherUser's journey
createJourneyEntry(testDb, otherJourney.id, target.id);
// journey_share_tokens: target created a share token for otherUser's journey
testDb.prepare("INSERT INTO journey_share_tokens (journey_id, token, created_by) VALUES (?, 'jst-admin-test', ?)").run(otherJourney.id, target.id);
// notifications.sender_id (SET NULL): target sent a notification to otherUser
const sentNotif = testDb.prepare(
"INSERT INTO notifications (type, scope, target, sender_id, recipient_id, title_key, text_key) VALUES ('simple', 'trip', ?, ?, ?, 'k', 'k')"
).run(otherTrip.id, target.id, otherUser.id);
// notifications.recipient_id (CASCADE): otherUser sent a notification to target
testDb.prepare(
"INSERT INTO notifications (type, scope, target, sender_id, recipient_id, title_key, text_key) VALUES ('simple', 'trip', ?, ?, ?, 'k', 'k')"
).run(otherTrip.id, otherUser.id, target.id);
// user_notice_dismissals (CASCADE): target dismissed a notice
testDb.prepare(
"INSERT INTO user_notice_dismissals (user_id, notice_id, dismissed_at) VALUES (?, 'test-notice', ?)"
).run(target.id, Date.now());
const res = await request(app)
.delete(`/api/admin/users/${target.id}`)
.set('Cookie', authCookie(admin.id));
expect(res.status).toBe(200);
expect(res.body.success).toBe(true);
expect(testDb.prepare('SELECT id FROM users WHERE id = ?').get(target.id)).toBeUndefined();
// trip_members row survives but invited_by is now NULL
expect((testDb.prepare('SELECT invited_by FROM trip_members WHERE trip_id = ? AND user_id = ?').get(otherTrip.id, thirdUser.id) as any).invited_by).toBeNull();
expect(testDb.prepare('SELECT id FROM share_tokens WHERE created_by = ?').get(target.id)).toBeUndefined();
expect((testDb.prepare('SELECT paid_by_user_id FROM budget_items WHERE id = ?').get(budgetItem.id) as any).paid_by_user_id).toBeNull();
expect(testDb.prepare('SELECT user_id FROM journey_contributors WHERE journey_id = ? AND user_id = ?').get(otherJourney.id, target.id)).toBeUndefined();
expect(testDb.prepare('SELECT id FROM journey_entries WHERE author_id = ?').get(target.id)).toBeUndefined();
expect(testDb.prepare('SELECT id FROM journey_share_tokens WHERE created_by = ?').get(target.id)).toBeUndefined();
// sent notification survives but sender_id becomes NULL
expect((testDb.prepare('SELECT sender_id FROM notifications WHERE id = ?').get(sentNotif.lastInsertRowid) as any).sender_id).toBeNull();
// received notification is cascade-deleted
expect(testDb.prepare('SELECT id FROM notifications WHERE recipient_id = ?').get(target.id)).toBeUndefined();
// notice dismissals are cascade-deleted
expect(testDb.prepare("SELECT user_id FROM user_notice_dismissals WHERE user_id = ? AND notice_id = 'test-notice'").get(target.id)).toBeUndefined();
});
it('ADMIN-006 — admin cannot delete their own account', async () => {
const { user: admin } = createAdmin(testDb);
+74 -1
View File
@@ -52,7 +52,7 @@ import { createApp } from '../../src/app';
import { createTables } from '../../src/db/schema';
import { runMigrations } from '../../src/db/migrations';
import { resetTestDb } from '../helpers/test-db';
import { createUser, createAdmin, createUserWithMfa, createInviteToken } from '../helpers/factories';
import { createUser, createAdmin, createUserWithMfa, createInviteToken, createTrip, createBudgetItem, createJourney, createJourneyEntry, addJourneyContributor } from '../helpers/factories';
import { authCookie, authHeader } from '../helpers/auth';
import { loginAttempts, mfaAttempts } from '../../src/routes/auth';
@@ -509,6 +509,79 @@ describe('Extended auth scenarios', () => {
});
});
// ─────────────────────────────────────────────────────────────────────────────
// Account deletion
// ─────────────────────────────────────────────────────────────────────────────
describe('Account deletion', () => {
it('AUTH-040 — DELETE /auth/me succeeds when user has FK references', async () => {
const { user: admin } = createAdmin(testDb);
const { user: target } = createUser(testDb);
const { user: otherUser } = createUser(testDb);
const { user: thirdUser } = createUser(testDb);
// trip_members.invited_by: target invited thirdUser to otherUser's trip
// (trip survives deletion; only invited_by should become NULL)
const otherTrip = createTrip(testDb, otherUser.id);
testDb.prepare('INSERT INTO trip_members (trip_id, user_id, invited_by) VALUES (?, ?, ?)').run(otherTrip.id, thirdUser.id, target.id);
// share_tokens.created_by: target created a share token for otherUser's trip
testDb.prepare("INSERT INTO share_tokens (trip_id, token, created_by) VALUES (?, 'tok-auth-test', ?)").run(otherTrip.id, target.id);
// budget_items.paid_by_user_id: target paid for an expense on otherUser's trip
const budgetItem = createBudgetItem(testDb, otherTrip.id);
testDb.prepare('UPDATE budget_items SET paid_by_user_id = ? WHERE id = ?').run(target.id, budgetItem.id);
// journey_contributors: target is a contributor on otherUser's journey
const otherJourney = createJourney(testDb, otherUser.id);
addJourneyContributor(testDb, otherJourney.id, target.id);
// journey_entries: target authored an entry on otherUser's journey
createJourneyEntry(testDb, otherJourney.id, target.id);
// journey_share_tokens: target created a share token for otherUser's journey
testDb.prepare("INSERT INTO journey_share_tokens (journey_id, token, created_by) VALUES (?, 'jst-auth-test', ?)").run(otherJourney.id, target.id);
// notifications.sender_id (SET NULL): target sent a notification to otherUser
const sentNotif = testDb.prepare(
"INSERT INTO notifications (type, scope, target, sender_id, recipient_id, title_key, text_key) VALUES ('simple', 'trip', ?, ?, ?, 'k', 'k')"
).run(otherTrip.id, target.id, otherUser.id);
// notifications.recipient_id (CASCADE): otherUser sent a notification to target
testDb.prepare(
"INSERT INTO notifications (type, scope, target, sender_id, recipient_id, title_key, text_key) VALUES ('simple', 'trip', ?, ?, ?, 'k', 'k')"
).run(otherTrip.id, otherUser.id, target.id);
// user_notice_dismissals (CASCADE): target dismissed a notice
testDb.prepare(
"INSERT INTO user_notice_dismissals (user_id, notice_id, dismissed_at) VALUES (?, 'test-notice', ?)"
).run(target.id, Date.now());
// admin exists to ensure target (non-admin user) passes the last-admin guard
void admin;
const res = await request(app)
.delete('/api/auth/me')
.set('Cookie', authCookie(target.id));
expect(res.status).toBe(200);
expect(res.body.success).toBe(true);
expect(testDb.prepare('SELECT id FROM users WHERE id = ?').get(target.id)).toBeUndefined();
// trip_members row survives but invited_by is now NULL
expect((testDb.prepare('SELECT invited_by FROM trip_members WHERE trip_id = ? AND user_id = ?').get(otherTrip.id, thirdUser.id) as any).invited_by).toBeNull();
expect(testDb.prepare('SELECT id FROM share_tokens WHERE created_by = ?').get(target.id)).toBeUndefined();
expect((testDb.prepare('SELECT paid_by_user_id FROM budget_items WHERE id = ?').get(budgetItem.id) as any).paid_by_user_id).toBeNull();
expect(testDb.prepare('SELECT user_id FROM journey_contributors WHERE journey_id = ? AND user_id = ?').get(otherJourney.id, target.id)).toBeUndefined();
expect(testDb.prepare('SELECT id FROM journey_entries WHERE author_id = ?').get(target.id)).toBeUndefined();
expect(testDb.prepare('SELECT id FROM journey_share_tokens WHERE created_by = ?').get(target.id)).toBeUndefined();
// sent notification survives but sender_id becomes NULL
expect((testDb.prepare('SELECT sender_id FROM notifications WHERE id = ?').get(sentNotif.lastInsertRowid) as any).sender_id).toBeNull();
// received notification is cascade-deleted
expect(testDb.prepare('SELECT id FROM notifications WHERE recipient_id = ?').get(target.id)).toBeUndefined();
// notice dismissals are cascade-deleted
expect(testDb.prepare("SELECT user_id FROM user_notice_dismissals WHERE user_id = ? AND notice_id = 'test-notice'").get(target.id)).toBeUndefined();
});
});
// ─────────────────────────────────────────────────────────────────────────────
// Rate limiting (AUTH-004, AUTH-018) — placed last
// ─────────────────────────────────────────────────────────────────────────────