Merge pull request #21 from screentinker/fix/delete-user-fk-cascade-18

fix: user deletion fails with FOREIGN KEY constraint (#18)
2026-06-15 02:33:15 -06:00 · 2026-06-08 10:52:50 -05:00 · 2026-06-08 10:52:50 -05:00 · ec44cb785a
parent 7615eabdd5 05f9c20ecf
commit ec44cb785a
3 changed files with 357 additions and 1 deletions
--- a/server/lib/user-deletion.js
+++ 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 };
--- 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 });
 });

--- a/server/test/user-deletion.test.js
+++ 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);
+});