diff --git a/integration/onboarding-progress.test.ts b/integration/onboarding-progress.test.ts new file mode 100644 index 0000000..34b24ff --- /dev/null +++ b/integration/onboarding-progress.test.ts @@ -0,0 +1,110 @@ +// Validates migration 055 (mark_onboarding_step_done) against a real Postgres: +// the atomic dedup-append into space_members.onboarding_progress that replaces +// the old read-modify-write in markOnboardingStepDone (deferred bug L3). The +// central case is CONCURRENCY: many simultaneous completions must all survive — +// the previous SELECT-then-UPDATE could silently drop steps under a lost update. +import { describe, it, expect, beforeAll, afterAll } from 'vitest' +import { promisify } from 'node:util' +import { execFile } from 'node:child_process' +import { + dbReady, + DB_URL, + rows, + execService, + execAsUser, + seedSpace, + seedMember, + dropSpace, + uuid, +} from './db' + +const execFileP = promisify(execFile) +const d = describe.skipIf(!dbReady) + +d('mark_onboarding_step_done (migration 055)', () => { + let spaceId: string + + beforeAll(() => { + spaceId = seedSpace().spaceId + }) + afterAll(() => dropSpace(spaceId)) + + // Completed step ids as the app stores/reads them: onboarding_progress + // -> completed_step_ids (a jsonb string array), or [] when unset. + function completedSteps(memberId: string): string[] { + const r = rows<{ ids: string[] | null }>( + `select (onboarding_progress -> 'completed_step_ids') as ids + from space_members where id = '${memberId}'`, + ) + return (r[0]?.ids as string[] | null) ?? [] + } + + const markService = (memberId: string, stepId: string) => + execService(`select mark_onboarding_step_done('${memberId}', '${stepId}')`) + + it('dedup-appends and is idempotent', () => { + const { memberId } = seedMember(spaceId) + const a = uuid() + const b = uuid() + expect(markService(memberId, a).ok).toBe(true) + expect(markService(memberId, b).ok).toBe(true) + expect(markService(memberId, a).ok).toBe(true) // repeat -> no-op + + const ids = completedSteps(memberId) + expect([...ids].sort()).toEqual([a, b].sort()) + expect(ids).toHaveLength(2) // 'a' not duplicated + }) + + it('preserves other keys already in onboarding_progress', () => { + const { memberId } = seedMember(spaceId) + execService( + `update space_members set onboarding_progress = '{"foo":"bar"}'::jsonb where id = '${memberId}'`, + ) + const a = uuid() + markService(memberId, a) + + const r = rows<{ p: { foo?: string; completed_step_ids?: string[] } }>( + `select onboarding_progress as p from space_members where id = '${memberId}'`, + ) + expect(r[0].p.foo).toBe('bar') + expect(r[0].p.completed_step_ids).toEqual([a]) + }) + + it('concurrent completions never lose an update (the L3 fix)', async () => { + const { memberId } = seedMember(spaceId) + const steps = Array.from({ length: 12 }, () => uuid()) + + // Fire every append at once through separate psql connections. The old + // read-modify-write would have interleaved SELECTs and dropped steps; the + // single-statement atomic UPDATE serializes on the row lock so all survive. + await Promise.all( + steps.map(s => + execFileP('psql', [ + DB_URL, + '-v', + 'ON_ERROR_STOP=1', + '-tAc', + `select mark_onboarding_step_done('${memberId}', '${s}')`, + ]), + ), + ) + + const ids = completedSteps(memberId) + expect([...ids].sort()).toEqual([...steps].sort()) // all 12 present, none lost + }) + + it('ownership: with a JWT a member can only complete steps on their OWN row', () => { + const owner = seedMember(spaceId) + const other = seedMember(spaceId) + const step = uuid() + + // `other`, acting as a logged-in user, aims the RPC at `owner`'s row. + // auth.uid() != owner.user_id -> the WHERE matches nothing -> no-op. + execAsUser(other.userId, `select mark_onboarding_step_done('${owner.memberId}', '${step}')`) + expect(completedSteps(owner.memberId)).toEqual([]) + + // The owner completing their own step works. + execAsUser(owner.userId, `select mark_onboarding_step_done('${owner.memberId}', '${step}')`) + expect(completedSteps(owner.memberId)).toEqual([step]) + }) +}) diff --git a/lib/actions/onboarding.ts b/lib/actions/onboarding.ts index 66fa861..8613dc7 100644 --- a/lib/actions/onboarding.ts +++ b/lib/actions/onboarding.ts @@ -130,20 +130,15 @@ export async function markOnboardingStepDone(stepId: string) { if (!auth.ok) return { error: auth.error } const { member } = auth - const { data: row } = await supabase - .from('space_members') - .select('onboarding_progress') - .eq('id', member.id) - .single() - - const progress = (row?.onboarding_progress ?? {}) as { completed_step_ids?: string[] } - const done = new Set(progress.completed_step_ids ?? []) - done.add(v.data) - - const { error } = await supabase - .from('space_members') - .update({ onboarding_progress: { ...progress, completed_step_ids: Array.from(done) } }) - .eq('id', member.id) + // Atomic dedup-append in the DB (migration 055). The old read-modify-write on + // onboarding_progress had a lost-update race: two near-simultaneous + // completions both read the same base array and the second write clobbered + // the first, silently dropping a step. The RPC pins the write to the caller's + // own row via auth.uid(). + const { error } = await supabase.rpc('mark_onboarding_step_done', { + p_member_id: member.id, + p_step_id: v.data, + }) if (error) return { error: error.message } return { success: true as const } diff --git a/scripts/055_onboarding_step_atomic_append.sql b/scripts/055_onboarding_step_atomic_append.sql new file mode 100644 index 0000000..425ae9e --- /dev/null +++ b/scripts/055_onboarding_step_atomic_append.sql @@ -0,0 +1,63 @@ +-- ============================================================================= +-- 055: atomic, concurrency-safe onboarding step completion +-- ============================================================================= +-- markOnboardingStepDone previously did a read-modify-write on the +-- space_members.onboarding_progress jsonb column: SELECT the object, add the +-- step id to completed_step_ids in JS, then UPDATE the whole object back. Two +-- near-simultaneous completions (a double-submit, or two steps ticked in quick +-- succession) both read the same base array and the second write clobbers the +-- first -- a lost update that silently drops a completed step (deferred bug L3). +-- +-- mark_onboarding_step_done replaces that with a single atomic UPDATE that +-- dedup-appends the step id into onboarding_progress.completed_step_ids. Because +-- it is one statement, the row lock serializes concurrent callers and Postgres +-- re-evaluates the jsonb expression against the latest committed tuple +-- (EvalPlanQual) under READ COMMITTED -- so no update is ever lost, and other +-- keys in the object are preserved. Appending an already-present id is a no-op +-- (idempotent), matching the old Set semantics. +-- +-- Ownership: the id-append is harmless, but a caller must still only be able to +-- touch their OWN membership row. When invoked with a JWT (auth.uid() set, the +-- normal RLS/user client path) the WHERE clause pins the row to +-- user_id = auth.uid(), so passing another member's id updates nothing. The +-- service/no-JWT path (auth.uid() IS NULL) is trusted and scoped by p_member_id, +-- mirroring the existing atomic RPCs (class_signup_tx et al.). +-- +-- Idempotent migration: CREATE OR REPLACE. Apply as-is (Supabase SQL editor / +-- psql); re-runs are no-ops. +-- ============================================================================= + +CREATE OR REPLACE FUNCTION public.mark_onboarding_step_done( + p_member_id uuid, p_step_id uuid +) RETURNS jsonb + LANGUAGE plpgsql SECURITY DEFINER SET search_path = public AS $$ +DECLARE + v_completed jsonb; +BEGIN + UPDATE public.space_members m + SET onboarding_progress = jsonb_set( + coalesce(m.onboarding_progress, '{}'::jsonb), + '{completed_step_ids}', + CASE + WHEN coalesce(m.onboarding_progress -> 'completed_step_ids', '[]'::jsonb) + @> to_jsonb(p_step_id::text) + THEN m.onboarding_progress -> 'completed_step_ids' + ELSE coalesce(m.onboarding_progress -> 'completed_step_ids', '[]'::jsonb) + || to_jsonb(p_step_id::text) + END + ) + WHERE m.id = p_member_id + AND (auth.uid() IS NULL OR m.user_id = auth.uid()) + RETURNING m.onboarding_progress -> 'completed_step_ids' INTO v_completed; + + -- NULL when no row matched (not the caller's membership); the resulting + -- completed_step_ids array otherwise. + RETURN v_completed; +END; +$$; + +-- Lock down default PUBLIC execute. The user (RLS) client calls this as the +-- authenticated role; the ownership guard above keeps it to the caller's own +-- row. service_role (the admin client) may also call it for the trusted path. +REVOKE EXECUTE ON FUNCTION public.mark_onboarding_step_done(uuid, uuid) FROM PUBLIC; +GRANT EXECUTE ON FUNCTION public.mark_onboarding_step_done(uuid, uuid) TO authenticated, service_role; diff --git a/scripts/schema.sql b/scripts/schema.sql index 68ea6c0..f9139ff 100644 --- a/scripts/schema.sql +++ b/scripts/schema.sql @@ -1655,6 +1655,41 @@ CREATE INDEX IF NOT EXISTS idx_onboarding_steps_space ON public.space_onboarding ALTER TABLE public.space_members ADD COLUMN IF NOT EXISTS onboarding_completed_at timestamptz; ALTER TABLE public.space_members ADD COLUMN IF NOT EXISTS onboarding_progress jsonb NOT NULL DEFAULT '{}'; +-- Atomic, concurrency-safe step completion (migration 055). Single-statement +-- dedup-append into onboarding_progress.completed_step_ids so two concurrent +-- completions can't lose an update (the old read-modify-write in +-- markOnboardingStepDone could). Ownership-guarded: with a JWT it only touches +-- the caller's own row; the no-JWT service path is trusted and scoped by +-- p_member_id. +CREATE OR REPLACE FUNCTION public.mark_onboarding_step_done( + p_member_id uuid, p_step_id uuid +) RETURNS jsonb + LANGUAGE plpgsql SECURITY DEFINER SET search_path = public AS $$ +DECLARE + v_completed jsonb; +BEGIN + UPDATE public.space_members m + SET onboarding_progress = jsonb_set( + coalesce(m.onboarding_progress, '{}'::jsonb), + '{completed_step_ids}', + CASE + WHEN coalesce(m.onboarding_progress -> 'completed_step_ids', '[]'::jsonb) + @> to_jsonb(p_step_id::text) + THEN m.onboarding_progress -> 'completed_step_ids' + ELSE coalesce(m.onboarding_progress -> 'completed_step_ids', '[]'::jsonb) + || to_jsonb(p_step_id::text) + END + ) + WHERE m.id = p_member_id + AND (auth.uid() IS NULL OR m.user_id = auth.uid()) + RETURNING m.onboarding_progress -> 'completed_step_ids' INTO v_completed; + + RETURN v_completed; +END; +$$; +REVOKE EXECUTE ON FUNCTION public.mark_onboarding_step_done(uuid, uuid) FROM PUBLIC; +GRANT EXECUTE ON FUNCTION public.mark_onboarding_step_done(uuid, uuid) TO authenticated, service_role; + CREATE OR REPLACE FUNCTION public.seed_default_onboarding_steps() RETURNS trigger LANGUAGE plpgsql SECURITY DEFINER SET search_path = public AS $$ diff --git a/types/database.ts b/types/database.ts index a2fe9a6..ce8febd 100644 --- a/types/database.ts +++ b/types/database.ts @@ -3462,6 +3462,10 @@ export type Database = { } expire_proposals: { Args: never; Returns: number } get_user_space_ids: { Args: { uid: string }; Returns: string[] } + mark_onboarding_step_done: { + Args: { p_member_id: string; p_step_id: string } + Returns: Json + } members_with_permission: { Args: { perm: string; sid: string } Returns: {