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 <noreply@anthropic.com> * docs: log profiles tombstone-column drift-capture decision Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
@@ -13,3 +13,4 @@ One line per decision: `[YYYY-MM-DD] <decision>: <why>`. 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.
|
||||
|
||||
@@ -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';
|
||||
@@ -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.
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user