From 8e7e7201d3ee9e6c09d8064e5d315e841a632726 Mon Sep 17 00:00:00 2001 From: Jakob Wennberg <149234542+jakobwennberg@users.noreply.github.com> Date: Mon, 6 Jul 2026 11:20:34 +0200 Subject: [PATCH] fix(db): drop delete_user_account RPC that bypassed BFL retention (#901) * fix(db): drop delete_user_account RPC that bypassed BFL retention delete_user_account disabled the retention/immutability/audit triggers, deleted audit_log rows, and cascaded auth.users, destroying 7 years of legally retained rakenskapsinformation (BFL 7 kap 2 paragraf). It was SECURITY DEFINER with only a self-only guard and no REVOKE, so any authenticated user could call it via PostgREST. The product path already uses anonymize_user_account, which so far existed only on production (drift). This migration drops the dangerous RPC, commits the prod definition of anonymize_user_account verbatim, adds the profiles tombstone columns it writes (also drift), and locks grants down to authenticated only. Closes #342 Co-Authored-By: Claude Fable 5 * docs: log profiles tombstone-column drift-capture decision Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- DECISIONS.md | 1 + ...0260706100000_drop_delete_user_account.sql | 82 +++++++++++ tests/pg/account-deletion-rpcs.pg.test.ts | 130 ++++++++++++++++++ 3 files changed, 213 insertions(+) create mode 100644 supabase/migrations/20260706100000_drop_delete_user_account.sql create mode 100644 tests/pg/account-deletion-rpcs.pg.test.ts diff --git a/DECISIONS.md b/DECISIONS.md index 0b7821cc..3f01c42b 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -13,3 +13,4 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-07-03] New user-facing strings on skattekonto follow that file's existing hardcoded-Swedish convention; the deadlines callout uses next-intl (page already translated). Year-end stays Swedish per .claude/rules/i18n.md. [2026-07-05] Salary run "Ångra godkännande" transitions approved → review (not straight to draft) and hard-deletes generated-but-unfiled AGI declarations — symmetric with the approve step for a clean audit trail, and stale AGI XML must not stay exportable. Blocked with 409 once the AGI is pending_signature/submitted/accepted: the lawful path is then a correction AGI with the same specifikationsnummer. Payment-file tracking is cleared; whether the file reached the bank is outside app knowledge, so the UI confirm makes the user own that check. [2026-07-05] PR #894 bot triage: accepted the delete-after-update reorder (destructive op last) and the manual-filing warning in confirm_unapprove_agi; declined soft-cancel status for unfiled AGI drafts and preserving approved_by on recall — a never-filed generated AGI is regenerable working data derived entirely from retained run data (not räkenskapsinformation; unapprove 409s once anything is filed), and the approval with legal weight is the one in force at booking, which unapprove can never touch (paid/booked runs are locked out). +[2026-07-06] Migration 20260706100000 adds profiles.deleted_at/anonymized_at (ADD COLUMN IF NOT EXISTS) alongside committing anonymize_user_account verbatim: the prod function writes those columns but no repo migration ever created them, so without the columns the drift capture would ship a function that fails on every from-scratch database (CI replay, self-hosted). No-op on prod. diff --git a/supabase/migrations/20260706100000_drop_delete_user_account.sql b/supabase/migrations/20260706100000_drop_delete_user_account.sql new file mode 100644 index 00000000..fb20501f --- /dev/null +++ b/supabase/migrations/20260706100000_drop_delete_user_account.sql @@ -0,0 +1,82 @@ +-- Drop delete_user_account and commit anonymize_user_account to the repo. +-- +-- WHY THIS MIGRATION EXISTS (issue #342, critical) +-- ------------------------------------------------ +-- public.delete_user_account (last defined by 20260415000000_schema_sync.sql) +-- DISABLEs the BFL retention triggers (block_document_deletion, +-- enforce_retention_journal_entries, enforce_journal_entry_immutability, +-- audit_log_no_delete, and friends), DELETEs audit_log rows for every company +-- the user created, and then DELETEs the auth.users row, cascading away +-- journal entries and documents. That destroys rakenskapsinformation that +-- BFL 7 kap 2 paragraf requires us to retain for 7 years. It is SECURITY +-- DEFINER with only a self-only guard (auth.uid() = target_user_id) and no +-- REVOKE, so any authenticated user could call it via PostgREST and legally +-- wipe their own company's books. The function must not exist at all: the +-- retention triggers are the legal backstop and no callable path may disable +-- them. +-- +-- The product path already uses public.anonymize_user_account +-- (app/api/account/delete/route.ts): it scrubs PII from profiles and removes +-- memberships/keys, but leaves all bookkeeping data untouched. That function +-- so far existed only on production with no migration in the repo; this +-- migration commits the production definition verbatim (drift capture, no +-- behavior change) and locks down its grants in house style. + +-- 1. Drop the retention-bypassing RPC. +DROP FUNCTION IF EXISTS public.delete_user_account(uuid); + +-- 2. Drift capture: production's profiles table carries the tombstone columns +-- the function writes, but no repo migration ever added them. Add them +-- idempotently so from-scratch databases (CI replay, self-hosted) match. +ALTER TABLE public.profiles ADD COLUMN IF NOT EXISTS deleted_at timestamptz; +ALTER TABLE public.profiles ADD COLUMN IF NOT EXISTS anonymized_at timestamptz; + +-- 3. Commit the production definition of anonymize_user_account verbatim. +CREATE OR REPLACE FUNCTION public.anonymize_user_account(target_user_id uuid) + RETURNS void + LANGUAGE plpgsql + SECURITY DEFINER + SET search_path TO 'public' +AS $function$ +DECLARE + blocker_count int; +BEGIN + IF auth.uid() IS DISTINCT FROM target_user_id THEN + RAISE EXCEPTION 'Can only delete your own account'; + END IF; + + SELECT count(*) INTO blocker_count + FROM public.company_members cm + JOIN public.companies c ON c.id = cm.company_id + WHERE cm.user_id = target_user_id + AND cm.role = 'owner' + AND c.archived_at IS NULL; + + IF blocker_count > 0 THEN + RAISE EXCEPTION 'Cannot delete account: user still owns % active compan(y/ies)', blocker_count + USING ERRCODE = 'P0001'; + END IF; + + DELETE FROM public.company_members WHERE user_id = target_user_id; + DELETE FROM public.team_members WHERE user_id = target_user_id; + DELETE FROM public.bankid_identities WHERE user_id = target_user_id; + + DELETE FROM public.user_preferences WHERE user_id = target_user_id; + DELETE FROM public.api_keys WHERE user_id = target_user_id; + + UPDATE public.profiles + SET email = NULL, + full_name = NULL, + avatar_url = NULL, + deleted_at = now(), + anonymized_at = now(), + updated_at = now() + WHERE id = target_user_id; +END; +$function$; + +-- 4. Grants: self-only guard inside, but never callable by anon/PUBLIC. +REVOKE ALL ON FUNCTION public.anonymize_user_account(uuid) FROM PUBLIC, anon; +GRANT EXECUTE ON FUNCTION public.anonymize_user_account(uuid) TO authenticated; + +NOTIFY pgrst, 'reload schema'; diff --git a/tests/pg/account-deletion-rpcs.pg.test.ts b/tests/pg/account-deletion-rpcs.pg.test.ts new file mode 100644 index 00000000..9d972b26 --- /dev/null +++ b/tests/pg/account-deletion-rpcs.pg.test.ts @@ -0,0 +1,130 @@ +import { randomUUID } from 'node:crypto' +import { describe, expect, it } from 'vitest' +import { getClient, getPool, withUserContext } from './setup' +import { insertAuthUser, seedCompany } from './fixtures' + +/** + * Account deletion RPCs (issue #342): + * + * Migration 20260706100000 drops public.delete_user_account, the SECURITY + * DEFINER RPC that disabled the BFL retention triggers, deleted audit_log + * rows, and cascaded auth.users, destroying rakenskapsinformation that + * BFL 7 kap 2 paragraf requires us to retain for 7 years. These tests pin + * that the function stays gone and that the surviving anonymize-only flow + * (public.anonymize_user_account) is present, SECURITY DEFINER, and not + * callable by anon/PUBLIC. + */ +describe('account deletion RPCs (pg)', () => { + it('delete_user_account no longer exists in pg_proc', async () => { + const { rows } = await getPool().query<{ n: number }>( + `SELECT count(*)::int AS n + FROM pg_proc p + JOIN pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' AND p.proname = 'delete_user_account'`, + ) + expect(rows[0]!.n).toBe(0) + }) + + it('anonymize_user_account exists and is SECURITY DEFINER', async () => { + const { rows } = await getPool().query<{ prosecdef: boolean }>( + `SELECT p.prosecdef + FROM pg_proc p + JOIN pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' AND p.proname = 'anonymize_user_account'`, + ) + expect(rows).toHaveLength(1) + expect(rows[0]!.prosecdef).toBe(true) + }) + + it('anonymize_user_account has no EXECUTE grant for anon or PUBLIC, but authenticated has it', async () => { + // anon inherits any PUBLIC grant, so anon=false also proves PUBLIC=false; + // the aclexplode check makes the PUBLIC assertion explicit (grantee oid 0). + const { rows } = await getPool().query<{ + anon_can_execute: boolean + authenticated_can_execute: boolean + public_grant_count: number + }>( + `SELECT + has_function_privilege('anon', 'public.anonymize_user_account(uuid)', 'EXECUTE') + AS anon_can_execute, + has_function_privilege('authenticated', 'public.anonymize_user_account(uuid)', 'EXECUTE') + AS authenticated_can_execute, + (SELECT count(*)::int + FROM pg_proc p + JOIN pg_namespace n ON n.oid = p.pronamespace, + LATERAL aclexplode(coalesce(p.proacl, acldefault('f', p.proowner))) AS acl + WHERE n.nspname = 'public' + AND p.proname = 'anonymize_user_account' + AND acl.grantee = 0 + AND acl.privilege_type = 'EXECUTE') AS public_grant_count`, + ) + expect(rows[0]!.anon_can_execute).toBe(false) + expect(rows[0]!.public_grant_count).toBe(0) + expect(rows[0]!.authenticated_can_execute).toBe(true) + }) + + it('denies EXECUTE to the anon role at call time', async () => { + const client = await getClient() + try { + await client.query('BEGIN') + await client.query('SET LOCAL ROLE anon') + await expect( + client.query('SELECT public.anonymize_user_account($1)', [randomUUID()]), + ).rejects.toThrow(/permission denied/i) + } finally { + await client.query('ROLLBACK').catch(() => {}) + client.release() + } + }) + + it('refuses to anonymize while the user still owns an active company', async () => { + const { userId } = await seedCompany() + await withUserContext(userId, async (client) => { + await expect( + client.query('SELECT public.anonymize_user_account($1)', [userId]), + ).rejects.toThrow(/still owns/i) + }) + }) + + it('refuses to anonymize another user', async () => { + const userId = await insertAuthUser() + const victimId = await insertAuthUser() + await withUserContext(userId, async (client) => { + await expect( + client.query('SELECT public.anonymize_user_account($1)', [victimId]), + ).rejects.toThrow(/only delete your own account/i) + }) + }) + + it('anonymizes a company-less user: scrubs profile PII, sets tombstones', async () => { + const userId = await insertAuthUser() + // insertAuthUser may or may not fire a profiles trigger depending on the + // harness image; make the profile row deterministic. + await getPool().query( + `INSERT INTO public.profiles (id, email, full_name) + VALUES ($1, $2, 'PG Real') + ON CONFLICT (id) DO UPDATE SET email = EXCLUDED.email, full_name = EXCLUDED.full_name`, + [userId, `pg-real-${userId}@test.invalid`], + ) + + await withUserContext(userId, async (client) => { + await client.query('SELECT public.anonymize_user_account($1)', [userId]) + const { rows } = await client.query<{ + email: string | null + full_name: string | null + deleted_at: string | null + anonymized_at: string | null + }>( + `SELECT email, full_name, deleted_at, anonymized_at + FROM public.profiles WHERE id = $1`, + [userId], + ) + expect(rows).toHaveLength(1) + expect(rows[0]!.email).toBeNull() + expect(rows[0]!.full_name).toBeNull() + expect(rows[0]!.deleted_at).not.toBeNull() + expect(rows[0]!.anonymized_at).not.toBeNull() + }) + // withUserContext rolls back, so the seeded rows do not leak. + }) +})