diff --git a/docs/spec/profile-and-credentials.md b/docs/spec/profile-and-credentials.md index 836daf9..21c0589 100644 --- a/docs/spec/profile-and-credentials.md +++ b/docs/spec/profile-and-credentials.md @@ -1861,6 +1861,8 @@ Written out rather than described, because the three mistakes these are here to **`assign_profile_owner()` is gone.** Claiming is a plain `PATCH` setting `user_id`, and `practitioners_guard` stamps `owner_assigned_at/by` on the way through — which is strictly better than the RPC was, because an RPC can be bypassed by anyone holding the `update` grant and a trigger cannot. The ones that remain earn their place: two write across tables atomically, three write provenance from `auth.uid()` rather than from whatever the caller passes, and the fourth carries an intent no `PATCH` can express. +**Extended by #128: `apply_profile_children()`, and it is the first one on the practitioner's side of the line.** Every RPC above is an admin's; this one is granted to `authenticated` and applies a whole save to `practitioner_credentials` and `practitioner_services` in a single transaction, because separate PostgREST requests are separate transactions and `practitioner_services_cap` made a half-applied save reachable by ordinary use — the credential deletes commit, the fourth service is refused, and the practitioner is left holding fewer credentials than they started with on the table the badge attests to. It takes the rows to remove, update and insert rather than the state the practitioner wants, so that nothing is decided by absence: the editor drops a service whose catalogue label has left the closed vocabulary in `src/lib/practitioners.ts`, and a function reading that absence as a removal would delete it from every profile offering it, one save at a time. It is `security invoker`, which is load-bearing rather than a default left alone — `credentials_guard` reads `current_user` against an allow-list holding `postgres`, so a `security definer` version owned by the migration role would count as privileged and the pin holding `verified` to `OLD` would stop applying inside exactly the door a practitioner writes through. It takes no practitioner id either, reading the profile from `my_profile()` for the reason given there. See `20260830092253_apply_profile_children.sql` and `tests/db/profile-children.test.ts`. + ### The rollup, in the client The badge is derived where it is rendered: diff --git a/src/app/profile/_lib/actions.ts b/src/app/profile/_lib/actions.ts index 90cbd1e..f862cbf 100644 --- a/src/app/profile/_lib/actions.ts +++ b/src/app/profile/_lib/actions.ts @@ -64,10 +64,13 @@ import { * is deliberately the harmless one: if the second request fails, what is left * behind is a contact row nobody references rather than a published profile * nobody can reach. The spec accepts that and calls for an occasional sweep; - * this action does not try to be clever about it, because the alternatives — - * a transaction it cannot open over PostgREST, or a delete of the row it just - * wrote on an error path that may itself have failed — are both worse than a - * stray address. + * this action does not try to be clever about it, because the alternatives are + * both worse than a stray address: a delete of the row it just wrote, on an + * error path that may itself have failed, or folding the pair into an RPC the + * way `saveChildren` folds the child tables. The second is a real option now + * rather than an impossible one — it would be a transaction — but it also moves + * the profile's own update inside a function, which is where #129's concurrency + * token would then have to live. * * ## Failures come back as values * @@ -117,8 +120,8 @@ type Refusal = Extract; * `practitioner_services_cap` raises with the count in the message and * `credentials_guard` says nothing a practitioner has to act on, so this is * mostly a passthrough — but a constraint name on its own ("23505: - * practitioner_credentials_practitioner_id_catalogue_id_key") is not a - * sentence, so the two that are reachable through this form are translated. + * practitioner_credentials_one_claim_each") is not a sentence, so the two that + * are reachable through this form are translated. */ function refusal(error: { code?: string; message: string; hint?: string | null }): Refusal { if (error.code === "23505") { @@ -248,23 +251,16 @@ export async function saveProfileAction(draft: ProfileDraft): Promise>, practitionerId: string, payload: ProfileWrite, -): Promise { +): Promise { const [saved, savedServices, catalogue] = await Promise.all([ supabase.rpc("my_credentials"), supabase.from("practitioner_services").select("id,catalogue_id,label").eq("practitioner_id", practitionerId), supabase.from("service_catalogue").select("id,label"), ]); - /* Nothing has been written when a lookup fails, which is the one place in - this function `applied: false` is a fact rather than a claim. */ const lookupFailure = saved.error ?? savedServices.error ?? catalogue.error; - if (lookupFailure) return { ...refusal(lookupFailure), applied: false }; + if (lookupFailure) return refusal(lookupFailure); /* `my_credentials()` returns every credential the caller owns, which for one account is one profile's worth — `practitioners.user_id` is unique. Filtered @@ -318,55 +326,22 @@ async function saveChildren( (catalogue.data ?? []) as ServiceCatalogueEntry[], ); - /* Set by the first statement that commits, and read by the caller to decide - what to tell the practitioner. It is deliberately not a count of what - landed: this function knows that *something* did, and anything finer would - be a second description of the same rows for somebody to keep in step. */ - let applied = false; - const failed = (error: { code?: string; message: string; hint?: string | null }): ChildOutcome => ({ - ...refusal(error), - applied, + /* The plan, flattened onto the wire. `id` is separate from `row` in a + `CredentialPlan` because it is the row's identity rather than one of the + columns a practitioner may write — it is absent from the update grant — and + the two are joined only here, where the function's record definition names + it as a column of the same shape. + + No `practitioner_id` travels. `apply_profile_children()` reads the profile + from `my_profile()`, so the one thing that decides whose rows are written is + who is asking, exactly as it is for the reads above. */ + const { error } = await supabase.rpc("apply_profile_children", { + credential_removals: credentials.remove, + credential_updates: credentials.update.map(({ id, row }) => ({ id, ...row })), + credential_inserts: credentials.insert, + service_removals: services.remove, + service_inserts: services.insert, }); - if (credentials.remove.length > 0) { - const { error } = await supabase - .from("practitioner_credentials") - .delete() - .in("id", credentials.remove); - if (error) return failed(error); - applied = true; - } - - if (services.remove.length > 0) { - const { error } = await supabase - .from("practitioner_services") - .delete() - .in("id", services.remove); - if (error) return failed(error); - applied = true; - } - - for (const { id, row } of credentials.update) { - const { error } = await supabase.from("practitioner_credentials").update(row).eq("id", id); - if (error) return failed(error); - applied = true; - } - - if (credentials.insert.length > 0) { - const { error } = await supabase - .from("practitioner_credentials") - .insert(credentials.insert.map((row) => ({ ...row, practitioner_id: practitionerId }))); - if (error) return failed(error); - applied = true; - } - - if (services.insert.length > 0) { - const { error } = await supabase - .from("practitioner_services") - .insert(services.insert.map((id) => ({ practitioner_id: practitionerId, catalogue_id: id }))); - if (error) return failed(error); - applied = true; - } - - return { ok: true }; + return error ? refusal(error) : { ok: true }; } diff --git a/src/lib/database.types.ts b/src/lib/database.types.ts index 484c72e..8a7895f 100644 --- a/src/lib/database.types.ts +++ b/src/lib/database.types.ts @@ -374,6 +374,16 @@ export type Database = { id: string }[] } + apply_profile_children: { + Args: { + credential_inserts: Json + credential_removals: string[] + credential_updates: Json + service_inserts: string[] + service_removals: string[] + } + Returns: undefined + } approve_practitioner: { Args: { profile_id: string } Returns: { diff --git a/supabase/migrations/20260830092253_apply_profile_children.sql b/supabase/migrations/20260830092253_apply_profile_children.sql new file mode 100644 index 0000000..c80fe8f --- /dev/null +++ b/supabase/migrations/20260830092253_apply_profile_children.sql @@ -0,0 +1,177 @@ +-- Applying a save to the two child tables, in one transaction. +-- +-- PostgREST runs one request in one transaction and offers no way to span two, so a +-- save's whole effect on `practitioner_credentials` and `practitioner_services` has to +-- arrive as one request or it is not atomic at all. Split across several, the deletes +-- commit and a later refusal cannot take them back — and that refusal needs no crafted +-- payload, because `practitioner_services_cap` raises on the insert that would fill a +-- fourth slot and the credential removals have run by then. What is left is a +-- practitioner holding fewer credentials than they started with, on the table the +-- Verified badge attests to. `saveChildren` in `src/app/profile/_lib/actions.ts` is the +-- caller, and it builds the plan this takes. +-- +-- ## It takes the plan rather than the state the practitioner wants +-- +-- The obvious alternative is to take the state the practitioner wants — every +-- credential and every service — and let this function work out the difference against +-- what is stored. It cannot, and the reason is that "absent from the payload" has two +-- meanings the function cannot tell apart: the practitioner removed it, or the editor +-- was unable to draw it. `planServices` in `src/app/profile/_lib/profile-plan.ts` only +-- ever deletes a row the form could have shown, because a catalogue label an admin +-- renamed out from under the closed vocabulary in `src/lib/practitioners.ts` is dropped +-- from the draft on the way in — no chip can render it — and reading its absence as a +-- removal would delete it from every profile offering it, one save at a time, silently. +-- The ambiguity is created on the client, so re-reading the stored rows in here cannot +-- resolve it. +-- +-- Naming the rows to remove is what makes that irrelevant: nothing is decided by +-- absence, so a row the editor never saw is never reached. **Do not change these +-- arguments into the desired state without moving that protection somewhere it still +-- holds** — the failure is a silent deletion, which is the same class of failure the +-- transaction above exists to end. +-- +-- The column definition lists below expand `evidence_url` as `public.https_url` rather +-- than as text, so a link that is not `https://` is refused with `23514` by the domain +-- as the record is expanded, exactly as the column would have refused it. +-- +-- ## `security invoker`, and this one is load-bearing +-- +-- `credentials_guard` decides whether a caller may move `verified` by reading +-- `current_user` against an allow-list that includes `postgres` +-- (`20260820222040_practitioner_credentials.sql`). A function a migration creates is +-- owned by `postgres`, so inside a `security definer` one — and inside every trigger +-- the DML it issues fires — `current_user` is `postgres`, the caller counts as +-- privileged, and the pin holding `verified`, `verified_at` and `verified_by` to `OLD` +-- stops applying. The column grants are the other half of that invariant and they are +-- bypassed by a definer function too, so a definer version of this function is the one +-- door in the schema through which a practitioner's own save could carry an +-- attestation. Nothing here needs the escalation: every column this function writes is +-- in the `authenticated` grants already, and it reads none of the withheld ones. +-- +-- No authorization is written inside, for the same reason `approve_practitioner()` +-- writes none: the three layers that were already deciding these writes go on deciding +-- them, and a check in here would be a second model free to disagree with the first. +-- The column grants keep `verified` out of reach, `credentials_rw_own` and +-- `services_rw_own` narrow a practitioner's statements to rows they own, and the two +-- triggers still fire. +-- +-- **Every statement is bounded by `profile_id` as well, and that is scope rather than +-- authorization.** The inserts carry it because a new row has to belong to somebody; +-- the removals and the updates test it because this function applies a plan to *one* +-- profile, and without the condition they would apply it to whatever rows the caller +-- happens to be able to reach. For a practitioner those are the same set, since +-- `credentials_rw_own` and `services_rw_own` have already narrowed the statement. For +-- an admin they are not: `credentials_admin_all` and `services_admin_all` are +-- `using (true)`, so an admin's removals and updates would otherwise find another +-- profile's rows — and the update is the one worth naming, because it rewrites the +-- claim, `credentials_guard` clears the check on it, and what it leaves behind is a row +-- that still looks right to everybody but its owner. +-- +-- **It closes no privilege**, and it is not pretending to. `bluehex_admin` holds +-- `update` and `delete` on both tables outright and reaches the same rows with one +-- request that never comes near here. What the condition buys is that the +-- practitioner's own save cannot be the door a mistyped id travels through, and that +-- the bound still holds if a later migration widens a policy by accident — which is the +-- argument `credentials_guard` rests on one table over. +-- +-- ## It takes no practitioner id +-- +-- The profile is read from `my_profile()`, which answers for whoever is asking. +-- `20260822050002_profile_own_reads.sql` gives the reason for the two reads there and +-- it is the same one: there is nothing to pass, and an argument would be the obvious +-- way to turn a write over your own rows into a write over somebody else's. + +create function public.apply_profile_children( + credential_removals uuid[], + credential_updates jsonb, + credential_inserts jsonb, + service_removals uuid[], + service_inserts uuid[] +) +returns void language plpgsql +set search_path = '' +as $$ +declare profile_id uuid; +begin + select p.id into profile_id from public.my_profile() p; + + if profile_id is null then + raise exception 'no profile for this account' using errcode = 'P0002'; + end if; + + -- **Deletes go first, and both tables have a reason.** + -- `practitioner_services_cap` counts rows and refuses the fourth, so swapping one + -- service for another inserts into a full table unless the removal has already + -- happened; `unique (practitioner_id, catalogue_id)` does the same to a credential + -- moved from one row to another. Ordering the statements is cheaper than teaching + -- either constraint about intent. + delete from public.practitioner_credentials c + where c.id = any(credential_removals) and c.practitioner_id = profile_id; + delete from public.practitioner_services s + where s.id = any(service_removals) and s.practitioner_id = profile_id; + + -- An unchanged row is not in this pile, decided by `planCredentials` rather than + -- here. `credentials_guard` compares with `is distinct from`, so rewriting the same + -- values would not clear a check either way; what the skipping actually protects is + -- `updated_at`, which the owner reads and which would otherwise move every time + -- somebody pressed Save on a credential they had not touched. + update public.practitioner_credentials c + set catalogue_id = u.catalogue_id, + earned_at = u.earned_at, + evidence_url = u.evidence_url, + evidence_public = u.evidence_public + from jsonb_to_recordset(credential_updates) as u( + id uuid, + catalogue_id uuid, + earned_at date, + evidence_url public.https_url, + evidence_public boolean + ) + where c.id = u.id and c.practitioner_id = profile_id; + + insert into public.practitioner_credentials + (practitioner_id, catalogue_id, earned_at, evidence_url, evidence_public) + select profile_id, i.catalogue_id, i.earned_at, i.evidence_url, i.evidence_public + from jsonb_to_recordset(credential_inserts) as i( + catalogue_id uuid, + earned_at date, + evidence_url public.https_url, + evidence_public boolean + ); + + -- Services carry a catalogue reference and nothing else writable, so there is no + -- update pile: an edit is a different row. `label` is absent from this insert and + -- not merely left null — the rows carrying one were written by an admin during + -- curated intake and this form has no control that produces them. + -- + -- **It is also last, and that is worth keeping.** `practitioner_services_cap` takes + -- `pg_advisory_xact_lock` keyed on the profile, and an `xact` lock is held until + -- commit rather than released when the statement ends — so the only statement that + -- fires it being the final one is the shortest hold this function can have. Moving it + -- above the credential work would make every concurrent save of the same profile wait + -- on the whole of the one ahead of it instead of on one insert. Nothing would report + -- that: it is two tabs of one practitioner, and it comes back as slowness. + insert into public.practitioner_services (practitioner_id, catalogue_id) + select profile_id, s from unnest(service_inserts) as s; +end; +$$; + +-- A pile that is empty is a pile that does nothing: `unnest('{}')` and +-- `jsonb_to_recordset('[]')` both yield no rows, and a `delete` or `update` matching +-- nothing raises nothing. So a save that changed only the profile itself still calls +-- this, and it is a no-op rather than a special case anybody has to remember. +-- +-- Nothing raises when a removal matches no row either, and that is deliberate rather +-- than an omission. A stale tab asking to remove a credential another tab has already +-- removed is a concurrency problem — #129 — and it wants a token that detects the stale +-- read, not a refusal here that would also fire on the ordinary race. + +revoke execute on function public.apply_profile_children(uuid[], jsonb, jsonb, uuid[], uuid[]) + from public, anon; +grant execute on function public.apply_profile_children(uuid[], jsonb, jsonb, uuid[], uuid[]) + to authenticated, bluehex_admin; + +-- `bluehex_admin` is named although `grant authenticated to bluehex_admin` in the first +-- migration would carry `execute` in with the membership, matching `my_profile()` and +-- `my_credentials()`: an admin is a practitioner too, and their own profile should not +-- be editable by way of a role membership that exists for a different reason. diff --git a/tests/db/profile-children.test.ts b/tests/db/profile-children.test.ts new file mode 100644 index 0000000..5223510 --- /dev/null +++ b/tests/db/profile-children.test.ts @@ -0,0 +1,792 @@ +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; + +import type { Json } from "@/lib/database.types"; + +import { adminCaller, anonCaller, practitionerCaller, type Caller } from "./harness/callers"; +import { + expectAllowed, + expectPermissionDenied, + expectSqlstate, + sqlstate, +} from "./harness/result"; +import { sql } from "./harness/stack"; + +/** + * `apply_profile_children()` — a save's whole effect on the two child tables, in + * one transaction. + * + * **The assertion this file exists for is *rolls the credential removal back when + * the services cap refuses the insert***: a plan whose services insert is refused + * by `practitioner_services_cap` has to leave the credentials it asked to remove + * exactly where they were. Split across separate PostgREST requests that failure + * is reachable rather than contrived — the deletes commit, the cap raises, and the + * practitioner is left holding fewer credentials than they started with, on the + * table the Verified badge attests to. + * + * Everything else here exists because atomicity must not be bought with something + * else, and the something else is `verified`. The function is `security invoker` + * so that `credentials_guard` goes on seeing a caller it treats as unprivileged; + * made `security definer` it would run as `postgres`, which is on that trigger's + * allow-list, and the pin holding the attestation columns to `OLD` would quietly + * stop applying. Nothing would look different from the owner's side, which is why + * several of these are written around the badge rather than around the rows. + * + * The second signed-in practitioner is not padding. The function names rows by id + * and takes no profile id at all, so what keeps a payload off somebody else's rows + * is row level security, `my_profile()` and the `profile_id` bound on every + * statement — and an assertion written from the owner passes identically whether + * any of the three is doing anything. + * + * **The admin is the caller who separates them.** `credentials_admin_all` and + * `services_admin_all` are `using (true)`, so row level security narrows nothing + * for an admin and the bound is the only thing left. Everything a practitioner is + * asked here would pass with the bound deleted; the admin cases would not. A + * function granted to `authenticated` and to `bluehex_admin` behaves as two + * different things, and a file that only ever asked the practitioner would state + * the practitioner's answer as the function's. + */ + +let anon: Caller; +let owner: Caller; +let stranger: Caller; +/** A signed-in account owning no profile, which is the only way to reach `P0002`. */ +let unowned: Caller; +/** An admin who is also a practitioner, because every admin here is both. */ +let admin: Caller; + +/** The owner's profile: approved, and the subject of most of this file. */ +let mine: string; +/** The second practitioner's, so the ownership rules have something to refuse. */ +let theirs: string; +/** The admin's own, which is where `my_profile()` sends their inserts. */ +let adminsOwn: string; + +/** Credential catalogue entries. Three, because a repoint needs somewhere to go. */ +let entryA: string; +let entryB: string; +let entryC: string; +/** Service catalogue entries. Four, because a cap of three needs a fourth to refuse. */ +let serviceA: string; +let serviceB: string; +let serviceC: string; +let serviceD: string; + +const evidence = "https://certificates.example.invalid/harness-children"; + +beforeAll(async () => { + anon = anonCaller(); + /* Sequential rather than `Promise.all`: sign-ups are rate limited per IP and a + burst is the thing the limit counts. */ + owner = await practitionerCaller("owner"); + stranger = await practitionerCaller("stranger"); + unowned = await practitionerCaller("unowned"); + admin = await adminCaller("admin"); + + mine = await seedProfile(owner.userId, "Harness children owner"); + theirs = await seedProfile(stranger.userId, "Harness children stranger"); + adminsOwn = await seedProfile(admin.userId, "Harness children admin"); + + entryA = await seedCatalogueEntry(); + entryB = await seedCatalogueEntry(); + entryC = await seedCatalogueEntry(); + serviceA = await seedServiceEntry(); + serviceB = await seedServiceEntry(); + serviceC = await seedServiceEntry(); + serviceD = await seedServiceEntry(); +}); + +/* Both child tables, for both profiles. Every test builds the state it needs from + nothing, because what most of them assert is the state left behind. */ +afterEach(async () => { + await sql( + "delete from public.practitioner_credentials where practitioner_id = any($1::uuid[])", + [[mine, theirs, adminsOwn]], + ); + await sql( + "delete from public.practitioner_services where practitioner_id = any($1::uuid[])", + [[mine, theirs, adminsOwn]], + ); +}); + +afterAll(async () => { + await sql("delete from public.practitioners where name like 'Harness children%'"); + await sql( + "delete from public.practitioner_contacts where contact_email like 'harness-children-%'", + ); + await sql("delete from public.credential_catalogue where label like 'harness children%'"); + await sql("delete from public.service_catalogue where label like 'harness children%'"); + /* No `deleteCreatedUsers()`: `harness/setup.ts` calls it for every file in the + project, and its afterAll runs after this one. */ +}); + +describe("one call, one transaction", () => { + it("applies removals, updates and inserts together", async () => { + const dropped = await seedCredential(mine, entryA); + const edited = await seedCredential(mine, entryB); + const goneService = await seedService(mine, serviceA); + + const result = await apply(owner, { + credential_removals: [dropped], + credential_updates: [ + { + id: edited, + catalogue_id: entryB, + earned_at: "2026-03-04", + evidence_url: evidence, + evidence_public: true, + }, + ], + credential_inserts: [ + { + catalogue_id: entryC, + earned_at: "2026-05-06", + evidence_url: null, + evidence_public: false, + }, + ], + service_removals: [goneService], + service_inserts: [serviceB], + }); + + expectAllowed(result); + expect(await credentialsOf(owner)).toEqual([ + { catalogue_id: entryB, earned_at: "2026-03-04", verified: false, verified_at: null }, + { catalogue_id: entryC, earned_at: "2026-05-06", verified: false, verified_at: null }, + ]); + expect(await servicesOf(owner, mine)).toEqual([serviceB]); + + /* `evidence_url` is expanded as `public.https_url` in the function's record + definition rather than as text, so this is also the assertion that the + domain survives `jsonb_to_recordset` — and that a link the practitioner + typed reaches the column it was typed for. */ + const saved = await mineCredentials(owner); + expect(saved.find((row) => row.catalogue_id === entryB)).toMatchObject({ + evidence_url: evidence, + evidence_public: true, + }); + }); + + it("rolls the credential removal back when the services cap refuses the insert", async () => { + /* Deletes run first inside the function, so by the time the fourth service is + refused the credential is gone as far as this transaction is concerned, and + it has to come back. Verified on purpose: what a save that committed its + deletes and then failed would destroy is a check Bluehex performed. */ + const checked = await seedCredential(mine, entryA, { verified: true }); + await seedService(mine, serviceA); + await seedService(mine, serviceB); + + const result = await apply(owner, { + credential_removals: [checked], + service_inserts: [serviceC, serviceD], + }); + + expectSqlstate(result, sqlstate.checkViolation); + expect(await credentialsOf(owner)).toEqual([ + { + catalogue_id: entryA, + earned_at: "2026-01-15", + verified: true, + verified_at: expect.any(String), + }, + ]); + expect(await servicesOf(owner, mine)).toEqual([serviceA, serviceB].sort()); + }); + + it("does nothing when every pile is empty", async () => { + await seedCredential(mine, entryA, { verified: true }); + await seedService(mine, serviceA); + + /* A save that changed only the profile itself still calls this, so an empty + plan is the ordinary case rather than an edge one. */ + expectAllowed(await apply(owner, {})); + + expect(await credentialsOf(owner)).toEqual([ + { + catalogue_id: entryA, + earned_at: "2026-01-15", + verified: true, + verified_at: expect.any(String), + }, + ]); + expect(await servicesOf(owner, mine)).toEqual([serviceA]); + }); +}); + +describe("what the badge attests to", () => { + it("leaves a credential the plan does not name alone, timestamp included", async () => { + await seedCredential(mine, entryA, { verified: true }); + const before = await updatedAtOf(owner, entryA); + + /* The pin, which is what `security definer` would have switched off. Inserting + a *different* credential in the same call is what makes this a test of the + trigger rather than of the plan: the transaction writes the table, and the + untouched row has to come through it unmarked. */ + expectAllowed( + await apply(owner, { + credential_inserts: [ + { + catalogue_id: entryB, + earned_at: "2026-02-02", + evidence_url: null, + evidence_public: false, + }, + ], + }), + ); + + expect(await credentialsOf(owner)).toEqual([ + { + catalogue_id: entryA, + earned_at: "2026-01-15", + verified: true, + verified_at: expect.any(String), + }, + { catalogue_id: entryB, earned_at: "2026-02-02", verified: false, verified_at: null }, + ]); + /* `updated_at` is the only signal that a row was written at all, and it is + served to the owner — a timestamp that moves whenever somebody saves an + unrelated credential answers "when did this last change" wrongly. */ + expect(await updatedAtOf(owner, entryA)).toBe(before); + }); + + it("clears verification when the plan edits the claim", async () => { + const checked = await seedCredential(mine, entryA, { verified: true }); + + expectAllowed( + await apply(owner, { + credential_updates: [ + { + id: checked, + catalogue_id: entryA, + earned_at: "2026-07-08", + evidence_url: null, + evidence_public: false, + }, + ], + }), + ); + + /* `credentials_guard`'s clearing rule, reached through the RPC. The date is + part of what Bluehex checked, so the check stops being asserted and the + provenance goes with it rather than pointing at an admin who looked at + something else. */ + expect(await credentialsOf(owner)).toEqual([ + { catalogue_id: entryA, earned_at: "2026-07-08", verified: false, verified_at: null }, + ]); + }); + + it("keeps verification when an update rewrites the same values", async () => { + const checked = await seedCredential(mine, entryA, { verified: true }); + + /* `planCredentials` keeps an unchanged row out of the update pile, so this is + not a call the editor makes. It is asserted anyway because the guard + compares with `is distinct from`, and that is the layer that still holds if + the plan is ever rewritten. */ + expectAllowed( + await apply(owner, { + credential_updates: [ + { + id: checked, + catalogue_id: entryA, + earned_at: "2026-01-15", + evidence_url: null, + evidence_public: false, + }, + ], + }), + ); + + expect(await credentialsOf(owner)).toEqual([ + { + catalogue_id: entryA, + earned_at: "2026-01-15", + verified: true, + verified_at: expect.any(String), + }, + ]); + }); + + it("ignores an attestation somebody puts in the payload", async () => { + /* The record definition names the columns it expands and `verified` is not + among them, so an extra key in the JSON is dropped before Postgres ever + sees it — and the column grants and the guard are both still behind that. + Asserted from the outcome, because the outcome is what makes deleting and + re-adding a credential a way to lose a badge rather than a way to mint one. */ + expectAllowed( + await apply(owner, { + credential_inserts: [ + { + catalogue_id: entryA, + earned_at: "2026-01-15", + evidence_url: null, + evidence_public: false, + verified: true, + }, + ], + }), + ); + + expect(await credentialsOf(owner)).toEqual([ + { catalogue_id: entryA, earned_at: "2026-01-15", verified: false, verified_at: null }, + ]); + }); +}); + +describe("rows the caller does not own", () => { + it("removes nothing when the removal names another practitioner's credential", async () => { + const notMine = await seedCredential(theirs, entryA, { verified: true }); + + /* Silently, and that is the answer rather than a compromise. Two things narrow + the delete here and either would do it alone: `credentials_rw_own`, and the + `profile_id` bound on the statement. Refusing instead would mean the function + had looked the row up, which is a thing a stranger should not be able to make + it do. */ + expectAllowed(await apply(owner, { credential_removals: [notMine] })); + + expect(await credentialsOf(stranger)).toEqual([ + { + catalogue_id: entryA, + earned_at: "2026-01-15", + verified: true, + verified_at: expect.any(String), + }, + ]); + }); + + it("changes nothing when an update names another practitioner's credential", async () => { + const notMine = await seedCredential(theirs, entryA, { verified: true }); + + /* The update is the statement with the quietest failure of the four. A removal + that reaches too far leaves a missing row and an insert leaves an extra one, + but an update that reaches too far leaves a row that still looks right to + everyone except its owner — and this plan asks for the two changes that would + hurt most: `earned_at`, which `credentials_guard` clears the check on, and + `evidence_public`, which it does not, so that half would move silently. */ + expectAllowed( + await apply(owner, { + credential_updates: [ + { + id: notMine, + catalogue_id: entryA, + earned_at: "2027-12-31", + evidence_url: evidence, + evidence_public: true, + }, + ], + }), + ); + + expect(await credentialsOf(stranger)).toEqual([ + { + catalogue_id: entryA, + earned_at: "2026-01-15", + verified: true, + verified_at: expect.any(String), + }, + ]); + expect((await mineCredentials(stranger))[0]).toMatchObject({ + evidence_url: null, + evidence_public: false, + }); + }); + + it("leaves another practitioner's services alone", async () => { + const notMine = await seedService(theirs, serviceA); + + expectAllowed(await apply(owner, { service_removals: [notMine] })); + + expect(await servicesOf(stranger, theirs)).toEqual([serviceA]); + }); + + it("inserts onto the caller's own profile and no other", async () => { + /* There is no profile id in the payload to get wrong. `my_profile()` answers + for whoever is asking, so an identical call from the stranger lands on their + own row — which is the whole of why this function takes no argument naming a + practitioner. */ + expectAllowed( + await apply(stranger, { + credential_inserts: [ + { + catalogue_id: entryA, + earned_at: "2026-01-15", + evidence_url: null, + evidence_public: false, + }, + ], + service_inserts: [serviceA], + }), + ); + + expect(await credentialsOf(stranger)).toHaveLength(1); + expect(await credentialsOf(owner)).toHaveLength(0); + expect(await servicesOf(stranger, theirs)).toEqual([serviceA]); + expect(await servicesOf(owner, mine)).toEqual([]); + }); + + it("does not reach a service row the editor could not have drawn", async () => { + /* The free-text row an admin wrote during curated intake. This form has no + control that produces one, so a function reconciling against the desired + state would read its absence as a removal and delete it on the first save of + an unrelated field. Naming the rows to remove is what makes that + unreachable, and this is what holds the signature to it. */ + await sql( + `insert into public.practitioner_services (practitioner_id, label) + values ($1, 'Rescuing agent runs at 2am')`, + [mine], + ); + const chip = await seedService(mine, serviceA); + + expectAllowed(await apply(owner, { service_removals: [chip], service_inserts: [serviceB] })); + + const left = await owner.client + .from("practitioner_services") + .select("catalogue_id, label") + .eq("practitioner_id", mine) + .order("created_at"); + expectAllowed(left); + expect(left.data).toEqual([ + { catalogue_id: null, label: "Rescuing agent runs at 2am" }, + { catalogue_id: serviceB, label: null }, + ]); + }); +}); + +/** + * The caller the `profile_id` bound on each statement is written for. + * + * A practitioner's answers hold with or without it, because `credentials_rw_own` + * and `services_rw_own` have already narrowed the statement to rows they own. An + * admin is the caller who would reach further, so **the three cases below that name + * another profile's rows are the only assertions in this file that fail if the bound + * is dropped** — one per bounded statement, because dropping it from any one of the + * three would otherwise cost nothing. The insert case is the one that never needed + * it: a new row carries `profile_id` because it has to belong to somebody, so it has + * always landed on the caller's own profile. + */ +describe("an admin, whom the policies would otherwise let reach further", () => { + it("removes nothing from another practitioner's profile", async () => { + const notMine = await seedCredential(theirs, entryA, { verified: true }); + + expectAllowed(await apply(admin, { credential_removals: [notMine] })); + + expect(await credentialsOf(stranger)).toEqual([ + { + catalogue_id: entryA, + earned_at: "2026-01-15", + verified: true, + verified_at: expect.any(String), + }, + ]); + }); + + it("removes nothing from another practitioner's services either", async () => { + const notMine = await seedService(theirs, serviceA); + + /* The same bound on the other table, and the statement that would go unnoticed + longest without it. A service carries no attestation, so nothing clears and + nothing is stamped: the row is simply gone from a profile nobody was editing, + and the only trace is what the directory stops showing. */ + expectAllowed(await apply(admin, { service_removals: [notMine] })); + + expect(await servicesOf(stranger, theirs)).toEqual([serviceA]); + }); + + it("changes nothing on another practitioner's credential with an update", async () => { + const notMine = await seedCredential(theirs, entryA, { verified: true }); + + /* The statement that leaves the least behind to notice it by, which is why the + bound matters most here. A removal that reached too far would leave a missing + row and an insert an extra one; this would leave the row where it was, holding + a claim its owner never made. The plan asks for both kinds of damage at once: + `earned_at`, which `credentials_guard` clears the check on, and + `evidence_public`, which it does not — so that half would move with nothing on + the row to show that it had, and what it governs is whether a certificate + carrying somebody's full legal name is served to every signed-in caller. */ + expectAllowed( + await apply(admin, { + credential_updates: [ + { + id: notMine, + catalogue_id: entryA, + earned_at: "2029-09-09", + evidence_url: null, + evidence_public: true, + }, + ], + }), + ); + + expect(await credentialsOf(stranger)).toEqual([ + { + catalogue_id: entryA, + earned_at: "2026-01-15", + verified: true, + verified_at: expect.any(String), + }, + ]); + expect((await mineCredentials(stranger))[0]).toMatchObject({ evidence_public: false }); + }); + + it("still inserts onto its own profile and no other", async () => { + await seedCredential(theirs, entryA, { verified: true }); + + /* The half that holds for every caller, and the reason the signature takes no + practitioner id: `profile_id` comes from `my_profile()`, so there is nothing + in the payload that names whose profile gains a row. An admin can write onto + a stranger's profile over PostgREST and cannot do it through here, which is + the property a later change adding an id argument would remove. */ + expectAllowed( + await apply(admin, { + credential_inserts: [ + { + catalogue_id: entryB, + earned_at: "2026-04-04", + evidence_url: null, + evidence_public: false, + }, + ], + service_inserts: [serviceA], + }), + ); + + expect(await credentialsOf(admin)).toEqual([ + { catalogue_id: entryB, earned_at: "2026-04-04", verified: false, verified_at: null }, + ]); + expect(await servicesOf(admin, adminsOwn)).toEqual([serviceA]); + expect(await servicesOf(stranger, theirs)).toEqual([]); + expect(await credentialsOf(stranger)).toEqual([ + { + catalogue_id: entryA, + earned_at: "2026-01-15", + verified: true, + verified_at: expect.any(String), + }, + ]); + }); +}); + +describe("who may call it", () => { + it("refuses anon", async () => { + const result = await apply(anon, {}); + + /* The message as well as the code, because the code alone does not say which + grant refused. This function is `security invoker` and its first statement + calls `my_profile()`, which `anon` may not execute either — so a version of + this migration with its own `revoke` dropped is refused one level further + in, with the same `42501` and therefore the same 401. The function named in + the message is the only thing that tells the two apart. */ + expectPermissionDenied(anon, result); + expect(result.error!.message).toContain("apply_profile_children"); + }); + + it("refuses a signed-in account that has no profile", async () => { + /* `P0002` is `no_data_found`, which `approve_practitioner()` already raises + for the same shape of question. Only reachable if the profile went away + between the two requests a save makes, and asserted so that the answer stays + an error rather than becoming a silent write onto nothing. */ + expectSqlstate(await apply(unowned, {}), "P0002"); + }); +}); + +describe("the refusals a save has to explain", () => { + it("raises the services cap with its own message", async () => { + await seedService(mine, serviceA); + await seedService(mine, serviceB); + await seedService(mine, serviceC); + + const result = await apply(owner, { service_inserts: [serviceD] }); + + /* `refusal()` in `src/app/profile/_lib/actions.ts` passes this message through + to the practitioner, so the wording surviving the RPC is what stops the form + falling back to a constraint name nobody can act on. */ + expectSqlstate(result, sqlstate.checkViolation); + expect(result.error!.message).toContain("at most three services"); + }); + + it("raises a unique violation naming the credentials constraint", async () => { + /* Removing something in the same call, so the rollback is asserted on a second + sqlstate as well as on the cap. It follows from the transaction and proves + nothing new about Postgres — but what this function exists to prevent is a + delete that commits ahead of a refusal, and a test naming one constraint + holds only until the next constraint arrives. */ + const kept = await seedCredential(mine, entryB, { verified: true }); + + const result = await apply(owner, { + credential_removals: [kept], + credential_inserts: [ + { + catalogue_id: entryA, + earned_at: "2026-01-15", + evidence_url: null, + evidence_public: false, + }, + { + catalogue_id: entryA, + earned_at: "2026-02-02", + evidence_url: null, + evidence_public: false, + }, + ], + }); + + /* `refusal()` branches on the constraint name rather than on the code, because + three of them are reachable from one save and they mean different things to + the person reading. This is the branch that turns a duplicate into a + sentence. */ + expectSqlstate(result, sqlstate.uniqueViolation); + expect(result.error!.message).toContain("practitioner_credentials_"); + expect(await credentialsOf(owner)).toEqual([ + { + catalogue_id: entryB, + earned_at: "2026-01-15", + verified: true, + verified_at: expect.any(String), + }, + ]); + }); +}); + +/** The plan, as `apply_profile_children()` takes it. Every pile defaults to empty. */ +type Plan = { + credential_removals?: string[]; + credential_updates?: Json[]; + credential_inserts?: Json[]; + service_removals?: string[]; + service_inserts?: string[]; +}; + +function apply(caller: Caller, plan: Plan) { + return caller.client.rpc("apply_profile_children", { + credential_removals: plan.credential_removals ?? [], + credential_updates: plan.credential_updates ?? [], + credential_inserts: plan.credential_inserts ?? [], + service_removals: plan.service_removals ?? [], + service_inserts: plan.service_inserts ?? [], + }); +} + +/** + * The caller's own credentials, through `my_credentials()`. **Through a caller + * rather than through `sql()`**: `postgres` bypasses every policy, so re-reading a + * row as the table's owner would prove nothing about what was decided. + */ +async function mineCredentials(caller: Caller) { + const result = await caller.client.rpc("my_credentials"); + expectAllowed(result); + return result.data!; +} + +/** The same rows, reduced to what these tests are about and ordered for comparison. */ +async function credentialsOf(caller: Caller) { + return (await mineCredentials(caller)) + .map((row) => ({ + catalogue_id: row.catalogue_id, + earned_at: row.earned_at, + verified: row.verified, + /* Cast because the generator types every column of a `returns table` + function as non-nullable, which `verified_at` is not — the #14 read + functions carry the same caveat. */ + verified_at: row.verified_at as string | null, + })) + .sort((a, b) => a.earned_at.localeCompare(b.earned_at)); +} + +/** One credential's `updated_at`, keyed by catalogue entry because the row id is not + what a test holds when it is asserting that the row was left alone. */ +async function updatedAtOf(caller: Caller, catalogueId: string): Promise { + const rows = await mineCredentials(caller); + return rows.find((row) => row.catalogue_id === catalogueId)!.updated_at; +} + +/** A profile's catalogue services, read through a caller and sorted for comparison. */ +async function servicesOf(caller: Caller, practitionerId: string): Promise { + const result = await caller.client + .from("practitioner_services") + .select("catalogue_id") + .eq("practitioner_id", practitionerId); + expectAllowed(result); + return result.data!.map((row) => row.catalogue_id!).sort(); +} + +/** Writes a contact row as `postgres` and returns its id. */ +let contacts = 0; +async function seedContact(): Promise { + contacts += 1; + const [row] = await sql<{ id: string }>( + `insert into public.practitioner_contacts (contact_email) + values ($1) returning id`, + [`harness-children-${contacts}-${Date.now()}@bluehex.test`], + ); + return row!.id; +} + +/** Writes a profile as `postgres`. Set-up is SQL; every assertion is a caller. */ +async function seedProfile(userId: string | null, name: string): Promise { + const [row] = await sql<{ id: string }>( + `insert into public.practitioners (contact_id, user_id, name, status) + values ($1, $2, $3, 'approved'::public.practitioner_status) returning id`, + [await seedContact(), userId, name], + ); + return row!.id; +} + +/** + * Writes a credential catalogue entry. Counted rather than constant, because + * `unique (kind, platform, label)` refuses a second entry with the same three and a + * duplicate in `beforeAll` reports as every test in the file being skipped. + */ +let entries = 0; +async function seedCatalogueEntry(): Promise { + entries += 1; + const [row] = await sql<{ id: string }>( + `insert into public.credential_catalogue (kind, platform, label) + values ('course', 'Anthropic Academy', $1) returning id`, + [`harness children entry ${entries}`], + ); + return row!.id; +} + +/** Writes a service catalogue entry. `label` is unique there, so it is counted too. */ +let serviceEntries = 0; +async function seedServiceEntry(): Promise { + serviceEntries += 1; + const [row] = await sql<{ id: string }>( + `insert into public.service_catalogue (label, sort_order) + values ($1, $2) returning id`, + [`harness children service ${serviceEntries}`, 100 + serviceEntries], + ); + return row!.id; +} + +/** + * Writes a credential as `postgres`, optionally already verified. + * + * `verified` cannot be set through a caller at all — that is the point of the + * column — so the fixture for every badge assertion here has to be built as + * `postgres`, which `credentials_guard` treats as privileged. It stamps + * `verified_at` on the way in; `verified_by` stays null, because there is no + * `auth.uid()` on a direct connection. + */ +async function seedCredential( + practitionerId: string, + catalogueId: string, + options: { verified?: boolean } = {}, +): Promise { + const [row] = await sql<{ id: string }>( + `insert into public.practitioner_credentials + (practitioner_id, catalogue_id, earned_at, verified) + values ($1, $2, '2026-01-15', $3) returning id`, + [practitionerId, catalogueId, options.verified ?? false], + ); + return row!.id; +} + +/** Writes a catalogue service row as `postgres` and returns its id. */ +async function seedService(practitionerId: string, catalogueId: string): Promise { + const [row] = await sql<{ id: string }>( + `insert into public.practitioner_services (practitioner_id, catalogue_id) + values ($1, $2) returning id`, + [practitionerId, catalogueId], + ); + return row!.id; +}