From 05f9c20ecfd8810a7760f96c9dbe162d59131b59 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Mon, 8 Jun 2026 10:51:32 -0500 Subject: [PATCH] fix(admin): user deletion failed with FOREIGN KEY constraint (#18) DELETE /api/auth/users/:id ran a bare `DELETE FROM users`, but 23 columns reference users(id) and only 4 cascade, so with foreign_keys=ON the delete fails the moment the user is referenced anywhere - and a real user always is (owns an org, created a workspace, has login activity). Reproduces on a fresh DB, exactly as reported. The schema also lacks cascades from workspaces -> tenant resources, so the DB can't clean up on its own. New lib/user-deletion.js resolves every reference in one transaction (defer_foreign_keys=ON for forgiving order; table-existence guard for resilience): - Refuse (409) if the user OWNS an organization that has other members - don't nuke a shared tenant; transfer ownership first. - Hard-delete the organizations they SOLELY own (workspaces + all contents). - In orgs they don't own, PRESERVE resources: SET NULL the nullable creator/inviter columns, and reassign the NOT NULL legacy creator user_id to the resource's org owner (fallback: the acting admin). - Memberships (organization_members/workspace_members/team_members/ content_folders) cascade on the user delete; pending invites they sent and legacy teams they own are removed. The handler now 404s an unknown id and 409s the shared-org case. Tests (node:test): reproduces the FK failure, then verifies provisioned-member delete (resources preserved + unlinked/reassigned), solo-org-owner cascade, shared-org refusal (409), self-delete 400, non-superadmin 403, unknown 404. Full suite 22/22. Verified end-to-end on a copy of a real DB: deleted a user owning 2 solo orgs, foreign_key_check clean. Closes #18. --- server/lib/user-deletion.js | 149 +++++++++++++++++++++++ server/routes/auth.js | 15 ++- server/test/user-deletion.test.js | 194 ++++++++++++++++++++++++++++++ 3 files changed, 357 insertions(+), 1 deletion(-) create mode 100644 server/lib/user-deletion.js create mode 100644 server/test/user-deletion.test.js diff --git a/server/lib/user-deletion.js b/server/lib/user-deletion.js new file mode 100644 index 0000000..2bedc6a --- /dev/null +++ b/server/lib/user-deletion.js @@ -0,0 +1,149 @@ +'use strict'; + +// Issue #18: deleting a user 500'd with "FOREIGN KEY constraint failed". +// +// 23 columns reference users(id) and only 4 (the *_members join tables + +// content_folders) carry ON DELETE CASCADE, so a bare `DELETE FROM users` +// fails the moment the user is referenced anywhere - and a real user always is +// (owns an org, created a workspace, has login activity). The schema also lacks +// cascades from workspaces -> tenant resources, so we cannot rely on the DB to +// clean up; we do it explicitly here in one transaction. +// +// Policy (chosen for #18): +// - Refuse (OrgHasOtherMembersError -> 409) if the user OWNS an organization +// that has any other member: don't nuke a shared tenant; transfer first. +// - Otherwise hard-delete the organizations they solely own (and everything +// inside), and for orgs they DON'T own, preserve the resources - just unlink +// the user (SET NULL where the column is nullable, or reassign the legacy +// creator user_id to the resource's org owner where it is NOT NULL). +// +// defer_foreign_keys=ON makes intra-transaction delete ORDER forgiving (FKs are +// validated once at COMMIT); we still clear every reference so COMMIT is clean. +// A table-existence guard keeps this resilient to partial/older schemas (and +// makes it unit-testable without standing up all ~25 tables). + +class OrgHasOtherMembersError extends Error { + constructor(message, sharedOrgCount) { + super(message); + this.name = 'OrgHasOtherMembersError'; + this.sharedOrgCount = sharedOrgCount; + } +} + +// Workspace-scoped tables whose rows must be deleted before their workspace +// (workspace_id is NO ACTION). CASCADE child tables (playlist_items, telemetry, +// assignments, layout_zones, *_devices, *_members) clean themselves up. +const WORKSPACE_SCOPED = [ + 'playlists', 'schedules', 'video_walls', 'device_groups', 'devices', + 'content', 'layouts', 'widgets', 'content_folders', 'kiosk_pages', + 'white_labels', 'alert_configs', +]; +// Logs that carry a device_id but NO foreign key (so they don't block, but we +// clean them to avoid dangling rows). +const DEVICE_LOG_TABLES = ['device_status_log', 'player_debug_logs']; +// Nullable creator/inviter columns -> SET NULL (preserve the resource). +const NULLABLE_USER_REFS = [ + ['content', 'user_id'], ['devices', 'user_id'], ['layouts', 'user_id'], ['widgets', 'user_id'], + ['workspaces', 'created_by'], ['organization_members', 'invited_by'], ['workspace_members', 'invited_by'], + ['team_members', 'invited_by'], ['device_fingerprints', 'user_id'], + ['activity_log', 'user_id'], ['activity_log', 'acting_user_id'], +]; +// NOT NULL legacy creator columns on workspace-scoped resources -> reassign to +// the resource's org owner (fallback: the acting admin) so the row survives. +const REASSIGN_USER_TABLES = [ + 'playlists', 'schedules', 'video_walls', 'device_groups', 'kiosk_pages', 'white_labels', 'alert_configs', +]; + +function listOwnedOrgsWithSharing(db, userId) { + let orgs = []; + try { orgs = db.prepare('SELECT id FROM organizations WHERE owner_user_id = ?').all(userId); } + catch { return []; } // no organizations table (legacy) -> nothing owned + return orgs.map(o => { + const otherOrgMembers = db.prepare( + 'SELECT COUNT(*) AS c FROM organization_members WHERE organization_id = ? AND user_id != ?' + ).get(o.id, userId).c; + const otherWsMembers = db.prepare(` + SELECT COUNT(*) AS c FROM workspace_members wm + JOIN workspaces w ON w.id = wm.workspace_id + WHERE w.organization_id = ? AND wm.user_id != ? + `).get(o.id, userId).c; + return { id: o.id, shared: (otherOrgMembers + otherWsMembers) > 0 }; + }); +} + +// Throws OrgHasOtherMembersError if the user owns a shared org. Otherwise +// deletes the user and resolves every reference in one transaction. +function deleteUserCascade(db, { targetId, actingAdminId }) { + const owned = listOwnedOrgsWithSharing(db, targetId); + const shared = owned.filter(o => o.shared); + if (shared.length > 0) { + throw new OrgHasOtherMembersError( + 'User owns an organization with other members - reassign ownership before deleting', + shared.length + ); + } + const soloOrgIds = owned.map(o => o.id); + + const have = new Set(db.prepare("SELECT name FROM sqlite_master WHERE type='table'").all().map(r => r.name)); + const inClause = n => Array.from({ length: n }, () => '?').join(','); + + const run = db.transaction(() => { + // FK checks deferred to COMMIT: order of our deletes no longer matters, only + // that no dangling reference remains at the end. + db.pragma('defer_foreign_keys = ON'); + + // 1) Hard-delete the orgs the user solely owns (and everything inside). + if (soloOrgIds.length) { + const wsIds = db.prepare( + `SELECT id FROM workspaces WHERE organization_id IN (${inClause(soloOrgIds.length)})` + ).all(...soloOrgIds).map(r => r.id); + + if (wsIds.length) { + const wph = inClause(wsIds.length); + if (have.has('devices')) { + const devIds = db.prepare(`SELECT id FROM devices WHERE workspace_id IN (${wph})`).all(...wsIds).map(r => r.id); + if (devIds.length) { + const dph = inClause(devIds.length); + for (const lt of DEVICE_LOG_TABLES) if (have.has(lt)) db.prepare(`DELETE FROM ${lt} WHERE device_id IN (${dph})`).run(...devIds); + } + } + for (const t of WORKSPACE_SCOPED) if (have.has(t)) db.prepare(`DELETE FROM ${t} WHERE workspace_id IN (${wph})`).run(...wsIds); + if (have.has('activity_log')) db.prepare(`UPDATE activity_log SET workspace_id = NULL WHERE workspace_id IN (${wph})`).run(...wsIds); + db.prepare(`DELETE FROM workspaces WHERE id IN (${wph})`).run(...wsIds); // cascades workspace_members/invites + } + + const oph = inClause(soloOrgIds.length); + if (have.has('activity_log')) db.prepare(`UPDATE activity_log SET organization_id = NULL WHERE organization_id IN (${oph})`).run(...soloOrgIds); + db.prepare(`DELETE FROM organizations WHERE id IN (${oph})`).run(...soloOrgIds); // cascades organization_members + } + + // 2) Unlink the user's footprint in orgs they DON'T own (rows still present). + // 2a) nullable creator/inviter columns -> SET NULL. + for (const [t, c] of NULLABLE_USER_REFS) if (have.has(t)) db.prepare(`UPDATE ${t} SET ${c} = NULL WHERE ${c} = ?`).run(targetId); + + // 2b) NOT NULL legacy creator columns -> reassign to the resource's org owner + // (fallback acting admin), preserving the resource under a valid owner. + for (const t of REASSIGN_USER_TABLES) { + if (!have.has(t)) continue; + db.prepare(` + UPDATE ${t} SET user_id = COALESCE( + (SELECT o.owner_user_id FROM workspaces w JOIN organizations o ON o.id = w.organization_id WHERE w.id = ${t}.workspace_id), + ? + ) WHERE user_id = ? + `).run(actingAdminId, targetId); + } + + // 2c) Legacy teams + NOT NULL invite rows the user owns / sent. + if (have.has('teams')) db.prepare('DELETE FROM teams WHERE owner_id = ?').run(targetId); // cascades team_members/invites + if (have.has('team_invites')) db.prepare('DELETE FROM team_invites WHERE invited_by = ?').run(targetId); + if (have.has('workspace_invites')) db.prepare('DELETE FROM workspace_invites WHERE invited_by = ?').run(targetId); + + // 3) Finally the user. Their own memberships (organization_members, + // workspace_members, team_members, content_folders) CASCADE on this delete. + db.prepare('DELETE FROM users WHERE id = ?').run(targetId); + }); + + run(); +} + +module.exports = { deleteUserCascade, OrgHasOtherMembersError, listOwnedOrgsWithSharing }; diff --git a/server/routes/auth.js b/server/routes/auth.js index 03b7299..c319f6a 100644 --- a/server/routes/auth.js +++ b/server/routes/auth.js @@ -9,6 +9,7 @@ const { generateToken, requireAuth, requireAdmin, requireSuperAdmin, isPlatformR const { resolveTenancy } = require('../lib/tenancy'); const { logActivity, getClientIp } = require('../services/activity'); const { sendSignupEmails } = require('../services/signupEmails'); +const { deleteUserCascade, OrgHasOtherMembersError } = require('../lib/user-deletion'); const config = require('../config'); // Phase 2.1: find or create the user's default org+workspace. Returns the @@ -488,7 +489,19 @@ router.get('/users', requireAuth, requireAdmin, (req, res) => { // Delete user (superadmin only) router.delete('/users/:id', requireAuth, requireSuperAdmin, (req, res) => { if (req.params.id === req.user.id) return res.status(400).json({ error: 'Cannot delete yourself' }); - db.prepare('DELETE FROM users WHERE id = ?').run(req.params.id); + const target = db.prepare('SELECT id, email FROM users WHERE id = ?').get(req.params.id); + if (!target) return res.status(404).json({ error: 'User not found' }); + // #18: a bare DELETE FROM users fails the FK constraints (23 uncascaded refs). + // deleteUserCascade resolves every reference in one transaction: hard-deletes + // orgs the user solely owns, preserves (unlinks/reassigns) resources in orgs + // they don't own, and refuses if they own a shared org. + try { + deleteUserCascade(db, { targetId: target.id, actingAdminId: req.user.id }); + } catch (e) { + if (e instanceof OrgHasOtherMembersError) return res.status(409).json({ error: e.message }); + throw e; + } + logActivity(req.user.id, 'delete_user', `target: ${target.email}`, null, getClientIp(req)); res.json({ success: true }); }); diff --git a/server/test/user-deletion.test.js b/server/test/user-deletion.test.js new file mode 100644 index 0000000..fba6fe8 --- /dev/null +++ b/server/test/user-deletion.test.js @@ -0,0 +1,194 @@ +'use strict'; + +// Issue #18: DELETE /api/auth/users/:id failed with "FOREIGN KEY constraint +// failed". This suite reproduces the failure faithfully (foreign_keys ON, +// prod-like uncascaded FKs) and verifies the cascade/unlink/refuse behaviour. +// +// Isolated in-memory better-sqlite3 injected into the require cache (same +// harness as the other suites); Node v20 built-ins only. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const Database = require('better-sqlite3'); + +process.env.JWT_SECRET = 'test-secret-user-deletion'; + +const db = new Database(':memory:'); +db.pragma('foreign_keys = ON'); +// Representative subset of the real schema with the SAME uncascaded FKs to +// users that caused #18. (The cascade helper's table-existence guard skips the +// tables we don't model here.) +db.exec(` + CREATE TABLE users ( + id TEXT PRIMARY KEY, email TEXT UNIQUE NOT NULL, name TEXT DEFAULT '', + password_hash TEXT, auth_provider TEXT NOT NULL DEFAULT 'local', avatar_url TEXT, + role TEXT NOT NULL DEFAULT 'user', plan_id TEXT DEFAULT 'free', email_alerts INTEGER DEFAULT 1, + must_change_password INTEGER NOT NULL DEFAULT 0, + created_at INTEGER NOT NULL DEFAULT (strftime('%s','now')) + ); + CREATE TABLE organizations ( + id TEXT PRIMARY KEY, name TEXT NOT NULL, + owner_user_id TEXT NOT NULL REFERENCES users(id) + ); + CREATE TABLE organization_members ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + organization_id TEXT NOT NULL REFERENCES organizations(id) ON DELETE CASCADE, + user_id TEXT NOT NULL REFERENCES users(id) ON DELETE CASCADE, + invited_by TEXT REFERENCES users(id), + role TEXT NOT NULL, UNIQUE(organization_id, user_id) + ); + CREATE TABLE workspaces ( + id TEXT PRIMARY KEY, + organization_id TEXT NOT NULL REFERENCES organizations(id) ON DELETE CASCADE, + name TEXT NOT NULL, created_by TEXT REFERENCES users(id) + ); + CREATE TABLE workspace_members ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + workspace_id TEXT NOT NULL REFERENCES workspaces(id) ON DELETE CASCADE, + user_id TEXT NOT NULL REFERENCES users(id) ON DELETE CASCADE, + invited_by TEXT REFERENCES users(id), + role TEXT NOT NULL, UNIQUE(workspace_id, user_id) + ); + CREATE TABLE workspace_invites ( + id TEXT PRIMARY KEY, + workspace_id TEXT NOT NULL REFERENCES workspaces(id) ON DELETE CASCADE, + email TEXT NOT NULL, role TEXT NOT NULL, + invited_by TEXT NOT NULL REFERENCES users(id), expires_at INTEGER NOT NULL + ); + CREATE TABLE playlists ( + id TEXT PRIMARY KEY, name TEXT, + user_id TEXT NOT NULL REFERENCES users(id), + workspace_id TEXT REFERENCES workspaces(id) + ); + CREATE TABLE devices ( + id TEXT PRIMARY KEY, name TEXT, + user_id TEXT REFERENCES users(id), + workspace_id TEXT REFERENCES workspaces(id) + ); + CREATE TABLE content ( + id TEXT PRIMARY KEY, filename TEXT, + user_id TEXT REFERENCES users(id), + workspace_id TEXT REFERENCES workspaces(id) + ); + CREATE TABLE activity_log ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + user_id TEXT REFERENCES users(id), + acting_user_id TEXT REFERENCES users(id), + organization_id TEXT REFERENCES organizations(id), + workspace_id TEXT REFERENCES workspaces(id), + action TEXT NOT NULL, details TEXT, created_at INTEGER NOT NULL DEFAULT (strftime('%s','now')) + ); +`); + +const dbModulePath = require.resolve('../db/database'); +require.cache[dbModulePath] = { + id: dbModulePath, filename: dbModulePath, loaded: true, + exports: { db, pruneTelemetry() {}, pruneScreenshots() {} }, +}; + +const express = require('express'); +const { generateToken, requireAuth } = require('../middleware/auth'); +const authRouter = require('../routes/auth'); + +// ---- seed: an admin caller + a separate org owner ---- +function user(id, role = 'user') { + db.prepare("INSERT INTO users (id, email, role, password_hash) VALUES (?, ?, ?, 'x')").run(id, id + '@test.local', role); + return { id, email: id + '@test.local', role }; +} +const admin = user('u-admin', 'platform_admin'); +const regular = user('u-regular', 'user'); // non-superadmin caller +const orgOwner = user('u-owner', 'user'); // owns org X (the "other" tenant) +db.prepare("INSERT INTO organizations (id, name, owner_user_id) VALUES ('orgX','Org X','u-owner')").run(); +db.prepare("INSERT INTO organization_members (organization_id, user_id, role) VALUES ('orgX','u-owner','org_owner')").run(); +db.prepare("INSERT INTO workspaces (id, organization_id, name, created_by) VALUES ('wsX','orgX','WS X','u-owner')").run(); +db.prepare("INSERT INTO workspace_members (workspace_id, user_id, role) VALUES ('wsX','u-owner','workspace_admin')").run(); + +const tokens = { admin: generateToken(admin, null), regular: generateToken(regular, null) }; + +const app = express(); +app.use(express.json()); +app.use('/api/auth', authRouter); +const server = app.listen(0); +let base; +test.before(async () => { await new Promise(r => server.listening ? r() : server.once('listening', r)); base = `http://127.0.0.1:${server.address().port}`; }); +test.after(() => { server.close(); db.close(); }); + +const del = (id, token) => fetch(`${base}/api/auth/users/${id}`, { method: 'DELETE', headers: { Authorization: `Bearer ${token}` } }); +const exists = (table, id) => !!db.prepare(`SELECT 1 FROM ${table} WHERE id = ?`).get(id); + +test('reproduces #18: a bare DELETE FROM users fails the FK constraint', () => { + user('u-fkprobe'); + db.prepare("INSERT INTO playlists (id, user_id) VALUES ('p-fkprobe','u-fkprobe')").run(); + assert.throws(() => db.prepare('DELETE FROM users WHERE id = ?').run('u-fkprobe'), /FOREIGN KEY constraint failed/); + db.prepare("DELETE FROM playlists WHERE id='p-fkprobe'").run(); + db.prepare("DELETE FROM users WHERE id='u-fkprobe'").run(); +}); + +test('provisioned member (owns no org): deleted; resources preserved + unlinked', async () => { + const t = user('u-member'); + db.prepare("INSERT INTO workspace_members (workspace_id, user_id, role) VALUES ('wsX','u-member','workspace_editor')").run(); + db.prepare("INSERT INTO playlists (id, name, user_id, workspace_id) VALUES ('p1','P1','u-member','wsX')").run(); + db.prepare("INSERT INTO devices (id, name, user_id, workspace_id) VALUES ('d1','D1','u-member','wsX')").run(); + db.prepare("INSERT INTO activity_log (user_id, action) VALUES ('u-member','login')").run(); + + const res = await del('u-member', tokens.admin); + assert.equal(res.status, 200); + assert.equal(exists('users', 'u-member'), false, 'user deleted'); + assert.equal(db.prepare("SELECT COUNT(*) c FROM workspace_members WHERE user_id='u-member'").get().c, 0, 'membership cascaded'); + // org X untouched (not owned by the deleted user) + assert.equal(exists('organizations', 'orgX'), true); + assert.equal(exists('workspaces', 'wsX'), true); + // playlist preserved, NOT NULL user_id reassigned to the workspace's org owner + assert.equal(db.prepare("SELECT user_id FROM playlists WHERE id='p1'").get().user_id, 'u-owner', 'playlist reassigned to org owner'); + // device preserved, nullable user_id set null + assert.equal(db.prepare("SELECT user_id FROM devices WHERE id='d1'").get().user_id, null, 'device unlinked'); + // activity preserved, user_id set null + assert.equal(db.prepare("SELECT COUNT(*) c FROM activity_log WHERE user_id IS NULL").get().c >= 1, true, 'activity preserved, unlinked'); +}); + +test('solo-org owner: org + workspace + contents hard-deleted with the user', async () => { + user('u-solo'); + db.prepare("INSERT INTO organizations (id, name, owner_user_id) VALUES ('orgS','Solo Org','u-solo')").run(); + db.prepare("INSERT INTO organization_members (organization_id, user_id, role) VALUES ('orgS','u-solo','org_owner')").run(); + db.prepare("INSERT INTO workspaces (id, organization_id, name, created_by) VALUES ('wsS','orgS','WS S','u-solo')").run(); + db.prepare("INSERT INTO workspace_members (workspace_id, user_id, role) VALUES ('wsS','u-solo','workspace_admin')").run(); + db.prepare("INSERT INTO playlists (id, user_id, workspace_id) VALUES ('pS','u-solo','wsS')").run(); + db.prepare("INSERT INTO devices (id, user_id, workspace_id) VALUES ('dS','u-solo','wsS')").run(); + db.prepare("INSERT INTO content (id, user_id, workspace_id) VALUES ('cS','u-solo','wsS')").run(); + + const res = await del('u-solo', tokens.admin); + assert.equal(res.status, 200); + assert.equal(exists('users', 'u-solo'), false); + assert.equal(exists('organizations', 'orgS'), false, 'owned org deleted'); + assert.equal(exists('workspaces', 'wsS'), false, 'workspace deleted'); + assert.equal(exists('playlists', 'pS'), false); + assert.equal(exists('devices', 'dS'), false); + assert.equal(exists('content', 'cS'), false); +}); + +test('shared-org owner: refused (409), nothing deleted', async () => { + user('u-shared'); + db.prepare("INSERT INTO organizations (id, name, owner_user_id) VALUES ('orgH','Shared Org','u-shared')").run(); + db.prepare("INSERT INTO organization_members (organization_id, user_id, role) VALUES ('orgH','u-shared','org_owner')").run(); + db.prepare("INSERT INTO workspaces (id, organization_id, name) VALUES ('wsH','orgH','WS H')").run(); + db.prepare("INSERT INTO workspace_members (workspace_id, user_id, role) VALUES ('wsH','u-shared','workspace_admin')").run(); + db.prepare("INSERT INTO workspace_members (workspace_id, user_id, role) VALUES ('wsH','u-owner','workspace_editor')").run(); // OTHER member + + const res = await del('u-shared', tokens.admin); + assert.equal(res.status, 409); + assert.equal(exists('users', 'u-shared'), true, 'not deleted'); + assert.equal(exists('organizations', 'orgH'), true, 'org intact'); +}); + +test('cannot delete yourself (400) and non-superadmin denied (403)', async () => { + const self = await del('u-admin', tokens.admin); + assert.equal(self.status, 400); + const denied = await del('u-owner', tokens.regular); + assert.equal(denied.status, 403); + assert.equal(exists('users', 'u-owner'), true); +}); + +test('missing user -> 404', async () => { + const res = await del('does-not-exist', tokens.admin); + assert.equal(res.status, 404); +});