Skip to content

Should practitioner_services_cap be an AFTER constraint trigger? #107

Description

@davidtaing

practitioner_services_cap is a before insert or update trigger, per docs/spec/profile-and-credentials.md:1710. A before row trigger runs ahead of the policy's WITH CHECK, so a non-owner writing to an approved profile already at three is refused 23514 by the cap rather than 42501 by the policy — a 400 where 403 is the honest answer. It discloses nothing: the count it reflects is already public through an ordinary GET, and on a profile whose services are not public the invoker-scoped count is zero and the policy answers correctly. Found during review of #106; the reasoning is recorded in the migration comment and in this thread.

A constraint trigger fires AFTER, and RLS's WITH CHECK runs before AFTER row triggers, so the ordering corrects itself with no authorization logic — nothing gated on who the caller is, so nothing that can fail open. Probed against the local stack: the stranger's refusal becomes 42501, and the full database suite passes unchanged at 169/169. It also retires id is distinct from new.id rather than working around it, since the row is already in the table by then and the check collapses to count(*) > 3.

That is the reason this is a spec question rather than a fix. id is distinct from new.id is not an implementation detail — the spec reasons about it, #90's body calls it out, and its Done-when list names it as its own assertion ("editing an existing third row in place still succeeds — the assertion that catches the missing id is distinct from new.id"). Retiring it is a design change, so it starts with a spec amendment and the migration follows.

The alternative that was considered and rejected in review: an early return new in the cap, gated on owns_profile(new.practitioner_id) plus a privileged-role allow-list. The allow-list is not optional — without it the cap stops applying to bluehex_admin and #90's "holds against an admin too" assertion goes red. That trades a wrong status code on a public fact for an authorization allow-list inside a function whose whole argument is that it is a rule about what a profile may say rather than an authority over who may say it, and one that fails open on the cap the day it is wrong. Recorded so it is not re-proposed.

Unvetted parts of the AFTER variant

Stated so nobody adopts it off this issue alone. It needs the same mutation testing the current trigger got before it lands.

  • The multi-row bulk-insert probe was not re-run against it — the current BEFORE trigger is proven to refuse four rows inserted in one statement.
  • The advisory-lock concurrency case was not re-argued for an AFTER trigger.
  • It would have to be NOT DEFERRABLE. A DEFERRABLE constraint trigger is a weaker cap than the current one.

Not a bug

Nothing leaks and nothing bypasses. "Won't fix, the migration comment is enough" is a legitimate outcome and this should be closeable that way without it reading as a loose end. What is not acceptable is the question going unrecorded, which is why it is here rather than only in a review thread on a merged PR.

Whichever way it goes, the answer belongs in docs/spec/profile-and-credentials.md before it belongs in a migration.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: dbPostgres schema, migrations, RLS, querieshitlNeeds a human (decision/design/review)size: SAn evening for someone new to this codebase

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions