diff --git a/apps/web/prisma/migrations/20260821090000_ose_initiated_onboarding_proposals/migration.sql b/apps/web/prisma/migrations/20260821090000_ose_initiated_onboarding_proposals/migration.sql new file mode 100644 index 00000000..fecf3366 --- /dev/null +++ b/apps/web/prisma/migrations/20260821090000_ose_initiated_onboarding_proposals/migration.sql @@ -0,0 +1,93 @@ +-- CreateEnum +CREATE TYPE "OnboardingProposalStatus" AS ENUM ('DRAFT', 'PENDING_DIRECTOR', 'APPROVED', 'REJECTED', 'WITHDRAWN', 'EXPIRED'); + +-- CreateEnum +CREATE TYPE "OnboardingSubjectKind" AS ENUM ('MEMBER', 'ADVISOR', 'OTHER'); + +-- CreateEnum +CREATE TYPE "OnboardingEventKind" AS ENUM ('CREATED', 'SUBMITTED', 'APPROVED', 'REJECTED', 'WITHDRAWN', 'EXPIRED'); + +-- CreateTable +CREATE TABLE "OnboardingProposal" ( + "id" TEXT NOT NULL, + "institutionId" TEXT NOT NULL, + "subjectName" TEXT NOT NULL, + "subjectEmail" TEXT NOT NULL, + "subjectEmailNormalized" TEXT NOT NULL, + "subjectKind" "OnboardingSubjectKind" NOT NULL, + "subjectKindOther" TEXT, + "cohort" TEXT NOT NULL, + "organizationId" TEXT, + "submittedById" TEXT NOT NULL, + "status" "OnboardingProposalStatus" NOT NULL DEFAULT 'DRAFT', + "expiresAt" TIMESTAMP(3) NOT NULL, + "submittedAt" TIMESTAMP(3), + "decidedAt" TIMESTAMP(3), + "decidedById" TEXT, + "decisionReason" TEXT, + "openSubjectKey" TEXT, + "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + "updatedAt" TIMESTAMP(3) NOT NULL, + + CONSTRAINT "OnboardingProposal_pkey" PRIMARY KEY ("id") +); + +-- CreateTable +CREATE TABLE "OnboardingProposalEvent" ( + "id" TEXT NOT NULL, + "proposalId" TEXT NOT NULL, + "institutionId" TEXT NOT NULL, + "kind" "OnboardingEventKind" NOT NULL, + "fromStatus" "OnboardingProposalStatus" NOT NULL, + "toStatus" "OnboardingProposalStatus" NOT NULL, + "actorId" TEXT, + "actorRole" TEXT, + "onBehalfOfId" TEXT, + "reason" TEXT, + "occurredAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + + CONSTRAINT "OnboardingProposalEvent_pkey" PRIMARY KEY ("id") +); + +-- CreateIndex +CREATE INDEX "OnboardingProposal_institutionId_status_idx" ON "OnboardingProposal"("institutionId", "status"); + +-- CreateIndex +CREATE INDEX "OnboardingProposal_institutionId_expiresAt_idx" ON "OnboardingProposal"("institutionId", "expiresAt"); + +-- CreateIndex +CREATE INDEX "OnboardingProposal_submittedById_idx" ON "OnboardingProposal"("submittedById"); + +-- CreateIndex +CREATE UNIQUE INDEX "OnboardingProposal_id_institutionId_key" ON "OnboardingProposal"("id", "institutionId"); + +-- CreateIndex +CREATE UNIQUE INDEX "OnboardingProposal_institutionId_openSubjectKey_key" ON "OnboardingProposal"("institutionId", "openSubjectKey"); + +-- CreateIndex +CREATE INDEX "OnboardingProposalEvent_proposalId_occurredAt_idx" ON "OnboardingProposalEvent"("proposalId", "occurredAt"); + +-- CreateIndex +CREATE INDEX "OnboardingProposalEvent_institutionId_occurredAt_idx" ON "OnboardingProposalEvent"("institutionId", "occurredAt"); + +-- AddForeignKey +ALTER TABLE "OnboardingProposal" ADD CONSTRAINT "OnboardingProposal_institutionId_fkey" FOREIGN KEY ("institutionId") REFERENCES "Institution"("id") ON DELETE CASCADE ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "OnboardingProposal" ADD CONSTRAINT "OnboardingProposal_organizationId_institutionId_fkey" FOREIGN KEY ("organizationId", "institutionId") REFERENCES "Organization"("id", "institutionId") ON DELETE RESTRICT ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "OnboardingProposal" ADD CONSTRAINT "OnboardingProposal_submittedById_fkey" FOREIGN KEY ("submittedById") REFERENCES "User"("id") ON DELETE RESTRICT ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "OnboardingProposal" ADD CONSTRAINT "OnboardingProposal_decidedById_fkey" FOREIGN KEY ("decidedById") REFERENCES "User"("id") ON DELETE RESTRICT ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "OnboardingProposalEvent" ADD CONSTRAINT "OnboardingProposalEvent_proposalId_institutionId_fkey" FOREIGN KEY ("proposalId", "institutionId") REFERENCES "OnboardingProposal"("id", "institutionId") ON DELETE CASCADE ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "OnboardingProposalEvent" ADD CONSTRAINT "OnboardingProposalEvent_actorId_fkey" FOREIGN KEY ("actorId") REFERENCES "User"("id") ON DELETE RESTRICT ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "OnboardingProposalEvent" ADD CONSTRAINT "OnboardingProposalEvent_onBehalfOfId_fkey" FOREIGN KEY ("onBehalfOfId") REFERENCES "User"("id") ON DELETE RESTRICT ON UPDATE CASCADE; + diff --git a/apps/web/prisma/schema.prisma b/apps/web/prisma/schema.prisma index d25e6f86..910208d0 100644 --- a/apps/web/prisma/schema.prisma +++ b/apps/web/prisma/schema.prisma @@ -83,6 +83,7 @@ model Institution { exceptions Exception[] seatMeterEvents SeatMeterEvent[] registrySeal RestrictedRegistrySeal? + onboardingProposals OnboardingProposal[] } enum InstitutionRole { @@ -123,17 +124,21 @@ model User { sessions Session[] institutionMembership InstitutionMembership[] roleAssignments RoleAssignment[] - roleTransfersFrom RoleTransfer[] @relation("RoleTransferFrom") - roleTransfersTo RoleTransfer[] @relation("RoleTransferTo") - roleTransfersInitiated RoleTransfer[] @relation("RoleTransferInitiator") - delegationsGiven ApprovalDelegation[] @relation("DelegationFrom") - delegationsReceived ApprovalDelegation[] @relation("DelegationTo") + roleTransfersFrom RoleTransfer[] @relation("RoleTransferFrom") + roleTransfersTo RoleTransfer[] @relation("RoleTransferTo") + roleTransfersInitiated RoleTransfer[] @relation("RoleTransferInitiator") + delegationsGiven ApprovalDelegation[] @relation("DelegationFrom") + delegationsReceived ApprovalDelegation[] @relation("DelegationTo") messagesSent Message[] participants Participant[] notifications Notification[] notificationPrefs NotificationPreference[] deliverableReminders DeliverableReminder[] - resourcesAuthored Resource[] @relation("ResourceAuthor") + resourcesAuthored Resource[] @relation("ResourceAuthor") + onboardingProposed OnboardingProposal[] @relation("OnboardingProposedBy") + onboardingDecided OnboardingProposal[] @relation("OnboardingDecidedBy") + onboardingActed OnboardingProposalEvent[] @relation("OnboardingEventActor") + onboardingActedFor OnboardingProposalEvent[] @relation("OnboardingEventOnBehalfOf") } // ─── Organizations & Role Seats ─────────────────────────────────────────────── @@ -162,20 +167,21 @@ model Organization { createdAt DateTime @default(now()) updatedAt DateTime @updatedAt - advisors OrganizationAdvisor[] - roles Role[] - approvals ApprovalRequest[] - events Event[] - conversations Conversation[] - documents Document[] - budgets Budget[] - vendors Vendor[] - memoryRecords MemoryRecord[] - feedPosts FeedPost[] - collabInterests CollabInterest[] - budgetLines BudgetLine[] - ledgerEntries LedgerEntry[] - exceptions Exception[] + advisors OrganizationAdvisor[] + roles Role[] + approvals ApprovalRequest[] + events Event[] + conversations Conversation[] + documents Document[] + budgets Budget[] + vendors Vendor[] + memoryRecords MemoryRecord[] + feedPosts FeedPost[] + collabInterests CollabInterest[] + budgetLines BudgetLine[] + ledgerEntries LedgerEntry[] + onboardingProposals OnboardingProposal[] + exceptions Exception[] // The target of the composite foreign keys on Role, OrganizationAdvisor and // the club-scoped half of Exception. @@ -1727,6 +1733,209 @@ model RestrictedRegistrySeal { institution Institution @relation(fields: [institutionId], references: [id], onDelete: Cascade) } +// ─── OSE-initiated onboarding ───────────────────────────────────────────────── + +/// Where a proposal is. Mirrors `OnboardingStatus` in +/// `src/lib/identity/onboarding-chain.ts` exactly; `onboarding-chain.test.ts` +/// reads this file and fails if the two ever disagree, so the rules module +/// cannot be reasoned about against a set of states the database does not have. +enum OnboardingProposalStatus { + DRAFT + PENDING_DIRECTOR + APPROVED + REJECTED + WITHDRAWN + /// Nobody decided in time. Terminal, and reached by the clock rather than by + /// an actor — see `expiresAt` below. + EXPIRED +} + +/// What the person is proposed AS. Not a role grant: it is the kind of access +/// being asked for, which is what decides the registry cohort a decision writes. +enum OnboardingSubjectKind { + MEMBER + ADVISOR + /// Anything else OSE admits — a graduate assistant, a staff observer. Requires + /// `subjectKindOther` to say what, so "other" never becomes the default that + /// means nothing. + OTHER +} + +/// Every transition a proposal has been through, append-only. +enum OnboardingEventKind { + CREATED + SUBMITTED + APPROVED + REJECTED + WITHDRAWN + EXPIRED +} + +/// An OSE-initiated proposal to admit ONE PERSON to the institution. +/// +/// ── Why this is its own model (ADR-0013) ──────────────────────────────────── +/// +/// `ApprovalRequest.organizationId` is a NON-NULL foreign key to `Organization` +/// with `onDelete: Cascade`, and an onboarding proposal has no club. Relaxing +/// that column to nullable would weaken the invariant for every club approval +/// and every reimbursement on the same table — a nullable column cannot say +/// "null only for onboarding" — and the cascade, which is right for a club +/// approval, is meaningless for a proposal that has no club. ADR-0013 records +/// the full argument and the two options it refuses. +/// +/// ── Why the grain is a PERSON, not a seat ─────────────────────────────────── +/// +/// Measured from the tracked roster workbook (column D of the four club sheets): +/// 64 students hold 106 occupied club/position pairs, and 40 of the 64 hold more +/// than one — 38 hold two, 2 hold three. With 18 Simon-domain advisors that is +/// the 82 people, on 82 distinct addresses, that `eligibility.ts` documents. A +/// proposal is therefore one row per PERSON — the unit that admits somebody to +/// the institution exactly once, no matter how many seats they go on to hold. +/// Anything counted per seat over-counts people by 1.66x, and the access +/// boundary this feeds (`RestrictedIdentity`) is itself keyed +/// `(institutionId, emailNormalized)`, one row per address. The grains agree. +/// +/// This comment said 145 pairs and 2.3x until the workbook was re-read. 145 is +/// roughly every email cell in those sheets — 106 student cells plus 40 advisor +/// cells — which counts an advisor's attachment to a seat as though it were a +/// seat somebody holds. ADR-0013 records the correction. +/// +/// What a decision then COSTS is a separate question with a separate answer; +/// this model deliberately does not encode a billing unit, because a proposal +/// that admits a person is not evidence about how seats are charged. +model OnboardingProposal { + id String @id @default(cuid()) + institutionId String + institution Institution @relation(fields: [institutionId], references: [id], onDelete: Cascade) + + /// Who is proposed. `subjectEmail` is kept as typed for display; every + /// comparison goes through `subjectEmailNormalized`, which is the same + /// normalisation `RestrictedIdentity.emailNormalized` uses so the two can be + /// matched without a second opinion about what an address is. + subjectName String + subjectEmail String + subjectEmailNormalized String + subjectKind OnboardingSubjectKind + /// Required when `subjectKind` is OTHER. The database cannot express that + /// conditional, so `createProposal` refuses it and a test proves the refusal. + subjectKindOther String? + /// The registry cohort a decision would write. Carried on the proposal so the + /// Director decides the same value that later reaches the boundary. + cohort String + + /// The club this person is proposed into, if any. NULLABLE, and that is the + /// whole reason this model exists: an advisor may belong to no club, and an + /// institution-level member belongs to none by definition. + /// + /// The relation is composite — `(organizationId, institutionId)` — so a + /// proposal cannot name a club belonging to a different institution. Postgres + /// applies MATCH SIMPLE, so the constraint is enforced when organizationId is + /// present and simply absent when it is null, which is exactly the semantics + /// wanted here. + organizationId String? + /// `Restrict`, not `SetNull`: the composite key includes the non-null + /// `institutionId`, and SET NULL would have to null that column too. Clubs are + /// archived rather than deleted here, so this refuses a delete that would + /// silently rewrite what a Director was asked to decide. + organization Organization? @relation(fields: [organizationId, institutionId], references: [id, institutionId], onDelete: Restrict) + + submittedById String + submittedBy User @relation("OnboardingProposedBy", fields: [submittedById], references: [id]) + + status OnboardingProposalStatus @default(DRAFT) + + /// The instant this proposal stops being decidable. + /// + /// STORED rather than computed from a constant, because it is a promise made + /// to a named person waiting for an answer: redeploying with a different + /// window must not move a deadline that has already been set. The clock is + /// authoritative — `effectiveProposalStatus` reads EXPIRED past this instant + /// whatever the column says — so an unrun sweep cannot leave a stale proposal + /// decidable. The sweep only makes the record catch up with the clock. + expiresAt DateTime + + /// Set when the proposal enters PENDING_DIRECTOR, so "how long has this person + /// been waiting" is a column rather than a scan of the event log. + submittedAt DateTime? + + /// The outcome, denormalised from the event log for lists and for the + /// downstream write. The log is the record; these are the current answer. + decidedAt DateTime? + decidedById String? + /// `Restrict`, following `AuditEvent`'s precedent: deleting a person must not + /// silently erase who decided an admission. The absence of a cascade is the + /// protection, and a tenant teardown removes these rows as a separate, + /// visible act. + decidedBy User? @relation("OnboardingDecidedBy", fields: [decidedById], references: [id], onDelete: Restrict) + decisionReason String? + + /// `subjectEmailNormalized` while this proposal is OPEN, and NULL once it is + /// terminal. + /// + /// Postgres treats NULLs as distinct in a unique index, so this is a partial + /// unique constraint expressed in a column: one open proposal per person per + /// institution, and any number of settled ones. Prisma cannot declare a + /// partial index, and adding one as raw SQL would make the migration stop + /// reproducing this file — which CI checks. Two open proposals for the same + /// address is not a cosmetic duplicate: it is two Directors approving the + /// same admission twice. + openSubjectKey String? + + createdAt DateTime @default(now()) + updatedAt DateTime @updatedAt + + events OnboardingProposalEvent[] + + /// The target of the composite foreign key on OnboardingProposalEvent. + @@unique([id, institutionId]) + @@unique([institutionId, openSubjectKey]) + @@index([institutionId, status]) + @@index([institutionId, expiresAt]) + @@index([submittedById]) +} + +/// One transition of one proposal. Append-only: rows are written, never edited. +/// +/// Separate from `AuditEvent` on purpose, and both are written. `AuditEvent` is +/// the security log — who tried what, and whether it was allowed — and +/// `audit.view` is minRole OSE_ADVISOR, so anything put in its metadata is +/// readable by every advisor at the institution. A proposal's payload is a named +/// person's address and cohort. That confidentiality inversion is why the +/// subject never appears on an audit row; it lives here, behind the same +/// permission as the proposal itself. +model OnboardingProposalEvent { + id String @id @default(cuid()) + proposalId String + /// Denormalised from the proposal and held to it by the composite foreign key + /// below, so the tenant chokepoint can filter this table directly instead of + /// reaching its tenant through a join it cannot add. `ApprovalStep` is the + /// older shape and is still in UNENFORCEABLE for exactly this reason. + institutionId String + proposal OnboardingProposal @relation(fields: [proposalId, institutionId], references: [id, institutionId], onDelete: Cascade) + + kind OnboardingEventKind + fromStatus OnboardingProposalStatus + toStatus OnboardingProposalStatus + + /// Null for EXPIRED, and only for EXPIRED: nobody acted, the clock did. Every + /// other transition names the person who caused it. + actorId String? + actor User? @relation("OnboardingEventActor", fields: [actorId], references: [id], onDelete: Restrict) + /// The actor's institution role at the instant of the transition, and — when + /// authority was borrowed — whose it was. Recorded rather than looked up + /// later, because a role held today is not evidence about a decision taken in + /// March. + actorRole String? + onBehalfOfId String? + onBehalfOf User? @relation("OnboardingEventOnBehalfOf", fields: [onBehalfOfId], references: [id], onDelete: Restrict) + + reason String? + occurredAt DateTime @default(now()) + + @@index([proposalId, occurredAt]) + @@index([institutionId, occurredAt]) +} + // ─── Inbound webhooks ──────────────────────────────────────────────────────── /// What this deployment expects a provider to deliver, and where. @@ -1899,4 +2108,4 @@ enum WebhookReceiptOutcome { NO_ACTION /// Accepted and recorded; no code path acts on it. `note` says which. NOT_PROCESSED -} +} \ No newline at end of file diff --git a/apps/web/src/lib/admin/capabilities.ts b/apps/web/src/lib/admin/capabilities.ts index 83bc8676..a00e2ca4 100644 --- a/apps/web/src/lib/admin/capabilities.ts +++ b/apps/web/src/lib/admin/capabilities.ts @@ -29,6 +29,8 @@ export type CapabilityId = | "directory.manage" | "institution.grantRole" | "institution.transferRole" + | "onboarding.propose" + | "onboarding.decide" | "approval.override" | "event.override" | "content.override" @@ -117,6 +119,27 @@ export const CAPABILITIES: Record = { description: "Add and edit directory people used for assignments.", minRole: "OSE_STAFF", }, + "onboarding.propose": { + id: "onboarding.propose", + label: "Propose an onboarding", + description: "Raise a proposal to admit a person who is not on the access registry.", + // Staff, not Director. Making the Director the only proposer would mean + // every proposal is self-approved by construction, which is the single + // outcome the requirement forbids — and this is where the sibling power + // already sits: `directory.manage` is minRole OSE_STAFF. + minRole: "OSE_STAFF", + }, + "onboarding.decide": { + id: "onboarding.decide", + label: "Decide an onboarding proposal", + description: "Approve or decline a proposal to admit a person to the institution.", + // Director, and Director is the top rank, so this reads as EXACTLY one role. + // `onboarding-chain.ts` derives R2 from this entry rather than keeping its + // own list, and `onboarding-chain.test.ts` pins the derived set to + // ["OSE_DIRECTOR"] — so lowering this minRole fails CI here rather than + // quietly widening who may admit a person to the institution. + minRole: "OSE_DIRECTOR", + }, "institution.grantRole": { id: "institution.grantRole", label: "Grant OSE access", @@ -319,6 +342,21 @@ export function hasCapability( return RANK[role] >= RANK[CAPABILITIES[capId].minRole] } +/** + * The roles that hold a capability, widest authority first. + * + * The inverse of `capabilitiesForRole`, derived from the same `RANK` table, so + * the two cannot disagree. It exists so an authority rule elsewhere can be + * DERIVED from this catalog instead of restating it: two lists of role names + * for one power is how they come to differ, and the one that is wrong is + * whichever nobody edited. + */ +export function rolesHolding(capId: CapabilityId): InstitutionRole[] { + return (Object.keys(RANK) as InstitutionRole[]) + .filter((r) => RANK[r] >= RANK[CAPABILITIES[capId].minRole]) + .sort((a, b) => RANK[b] - RANK[a]) +} + /** Capability ids this admin role holds — drives what the console renders. */ export function capabilitiesForRole(role: InstitutionRole): CapabilityId[] { return (Object.keys(CAPABILITIES) as CapabilityId[]).filter( diff --git a/apps/web/src/lib/auth/eligibility.test.ts b/apps/web/src/lib/auth/eligibility.test.ts index 58486e4f..e6c48ab1 100644 --- a/apps/web/src/lib/auth/eligibility.test.ts +++ b/apps/web/src/lib/auth/eligibility.test.ts @@ -2,6 +2,7 @@ import { decideEligibility, emailDomain, enforcementState, + isAddressShaped, lookupIn, normalizeEmail, type RegistryLookup, @@ -330,3 +331,64 @@ describe("the flip is gated on evidence, not on optimism", () => { expect(checked).toBe(32) }) }) + +describe("is this a shape an address can have", () => { + /** + * The SHAPES below are the ones the tracked leadership workbook uses, written + * as synthetic addresses rather than copied out of it — the roster is kept out + * of this repository on purpose, and a test fixture is as public as the + * repository is. The rule was run over every address-bearing cell in that + * workbook before it shipped and refused none of them, so it refuses nobody + * who is really on the roster. + */ + it("admits every shape the roster uses", () => { + for (const shaped of [ + "abcdefg@example.test", + "Given.Family@Example.Test", // capitalised, and dotted + "afamily_Unit@example.test", // underscore, mixed case + "abc123@Example.test", // digits + " agiven@example.test ", // the workbook's stray whitespace, outside + "advisor@other.example.test", // a second valid domain at the same university + ]) { + expect(isAddressShaped(shaped)).toBe(true) + } + }) + + it("refuses the strings `includes(\"@\")` used to admit", () => { + // Each of these was accepted by the onboarding path, and each would have + // become a row on the access boundary that no sign-in could ever match. + for (const junk of [ + "@", + "@example.test", + "two words@example.test", + "someone@example.test extra", + "a@@example.test", + "a@b@c.test", + "someone@localhost", + "someone@.example.test", + "someone@example.test.", + "someone@example..test", + "not-an-address", + "", + " ", + ]) { + expect(isAddressShaped(junk)).toBe(false) + } + }) + + it("is NOT a domain gate — the roster is the boundary, not the domain", () => { + // §3.2, and the reason the identity specification excludes one advisor: a + // valid University address that is not the tenant's is still an address. + // Being shaped like one is not being on the roster, and only the roster + // decides. + expect(isAddressShaped("stranger@elsewhere.test")).toBe(true) + expect(decideEligibility("stranger@elsewhere.test", sealed("stranger@elsewhere.test")).allow).toBe( + false, + ) + }) + + it("agrees with normalizeEmail about what it is looking at", () => { + expect(isAddressShaped(" Given.Family@Example.Test ")).toBe(true) + expect(normalizeEmail(" Given.Family@Example.Test ")).toBe("given.family@example.test") + }) +}) diff --git a/apps/web/src/lib/auth/eligibility.ts b/apps/web/src/lib/auth/eligibility.ts index aa4e1048..0d6831dc 100644 --- a/apps/web/src/lib/auth/eligibility.ts +++ b/apps/web/src/lib/auth/eligibility.ts @@ -104,6 +104,53 @@ export function normalizeEmail(email: string): string { return email.trim().toLowerCase() } +/** + * Is this string shaped like an address at all? + * + * ── Why this exists, and why it is HERE ───────────────────────────────────── + * + * `includes("@")` was the whole test, in three places, and it admits `"@"`, + * `"@simon.rochester.edu"` and `"two words@simon.rochester.edu"`. That was + * survivable while the only writer was a seeder reading a reconciled workbook. + * It stops being survivable on the OSE onboarding path, where a human types the + * address into a form and an approval writes it onto the access boundary: a + * mistyped address becomes a registry row that no sign-in can ever match, and — + * once seats are charged — a person-shaped row that is not a person. + * + * It lives beside `normalizeEmail` for the reason that function gives: the + * codebase must not hold two opinions about what an address is. Any caller that + * needs the check imports this one. + * + * ── What it deliberately does NOT do ──────────────────────────────────────── + * + * It is not a domain gate and it is not RFC 5322. `eligibility.ts` is explicit + * that the domain is never sufficient and never the boundary — the roster is — + * and the Identity Continuity specification names an advisor holding only a + * `@ur.rochester.edu` address, a valid University identity that is not a Simon + * one. This admits that address, and every one of the address-bearing cells in + * the tracked leadership workbook — all 86 were run through it before it + * shipped. It refuses only strings that cannot be an address: no local part, no + * domain, more than one `@`, a domain with no dot, or whitespace anywhere + * inside. + * + * `decideEligibility` deliberately still uses its own weaker check. Tightening + * the SIGN-IN path is a separate change with a different blast radius — it can + * only ever refuse somebody who is already on the roster — and it is not made + * here as a side effect of an admission path. + */ +export function isAddressShaped(email: string): boolean { + const s = normalizeEmail(email) + if (/\s/.test(s)) return false + const at = s.indexOf("@") + // `at <= 0` covers both "no @ at all" and "nothing before it". + if (at <= 0) return false + if (s.indexOf("@", at + 1) !== -1) return false + const domain = s.slice(at + 1) + if (!domain.includes(".")) return false + if (domain.startsWith(".") || domain.endsWith(".") || domain.includes("..")) return false + return true +} + /** * The domain policy for this tenant, read from configuration. * diff --git a/apps/web/src/lib/identity/onboarding-actor.test.ts b/apps/web/src/lib/identity/onboarding-actor.test.ts new file mode 100644 index 00000000..198afd6d --- /dev/null +++ b/apps/web/src/lib/identity/onboarding-actor.test.ts @@ -0,0 +1,238 @@ +import type { InstitutionRole } from "@prisma/client" +import type { UserContext } from "@/lib/rbac" + +const delegationFindMany = jest.fn() +jest.mock("@/lib/db", () => ({ + db: { approvalDelegation: { findMany: (...a: unknown[]) => delegationFindMany(...a) } }, +})) + +const getUserContext = jest.fn() +jest.mock("@/lib/rbac", () => ({ getUserContext: (...a: unknown[]) => getUserContext(...a) })) + +import { effectiveOnboardingActor, toOnboardingActor } from "./onboarding-actor" +import { + availableActions, + canDecide, + refusalFor, + type OnboardingActor, + type ProposalView, +} from "./onboarding-chain" + +/** + * THE DELEGATION ATTACK. + * + * `onboarding-chain.test.ts` proves R3 refuses a hand-built actor that happens + * to carry OSE_DIRECTOR. That is the rule in isolation, and it is not the + * question. The question is whether the MECHANISM that grants a proposer the + * Director's roles — `ApprovalDelegation`, resolved by + * `effectiveApprovalContext` — actually produces an actor R3 still refuses. + * + * So nothing here is hand-built. A real delegation row goes into a mocked + * database, the real resolver reads it, the real merge happens, the real actor + * comes out, and the real rule judges it. Everything between the row and the + * refusal is production code. + * + * The threat is ordinary, which is what makes it dangerous: a Director going on + * leave names a backup. If the backup is also the person who raised a proposal, + * a role-shaped rule hands them their own approval, and nobody involved did + * anything unusual. + */ + +const INST = "inst_simon" +const OTHER_INST = "inst_other" +const NOW = new Date("2026-08-21T12:00:00.000Z") +const FAR_FUTURE = new Date("2027-01-01T00:00:00.000Z") + +const ctxFor = (userId: string, roles: { institutionId: string; role: InstitutionRole }[]): UserContext => ({ + userId, + evaluatedAt: NOW, + institutionRoles: roles, + orgRoles: [], +}) + +const PROPOSER = ctxFor("staff_1", [{ institutionId: INST, role: "OSE_STAFF" }]) +const DIRECTOR = ctxFor("dir_1", [{ institutionId: INST, role: "OSE_DIRECTOR" }]) + +const ownProposal: ProposalView = { + institutionId: INST, + submittedById: "staff_1", + status: "PENDING_DIRECTOR", + expiresAt: FAR_FUTURE, +} + +/** A live delegation row, exactly as `effectiveApprovalContext` selects it. */ +const delegationFrom = (fromUserId: string, name: string) => ({ + fromUserId, + fromUser: { id: fromUserId, name, email: null }, +}) + +beforeEach(() => { + delegationFindMany.mockReset() + getUserContext.mockReset() + delegationFindMany.mockResolvedValue([]) +}) + +describe("the Director's delegation reaches the proposer — the attack sets up", () => { + it("really does hand the proposer OSE_DIRECTOR", async () => { + // Asserted FIRST and on its own. If the merge silently failed — a changed + // where-clause, a mock that returns nothing — every refusal below would pass + // for the wrong reason, and this file would report that an attack it never + // performed had been defeated. + delegationFindMany.mockResolvedValue([delegationFrom("dir_1", "The Director")]) + getUserContext.mockResolvedValue(DIRECTOR) + + const { actor, delegators } = await effectiveOnboardingActor("staff_1", PROPOSER, INST) + + expect(actor.institutionRoles).toContainEqual({ institutionId: INST, role: "OSE_DIRECTOR" }) + expect(delegators).toEqual([{ id: "dir_1", name: "The Director" }]) + // And the resolver was asked for ACTIVE delegations at THIS institution. + expect(delegationFindMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: { toUserId: "staff_1", revokedAt: null, institutionId: INST }, + }), + ) + }) +}) + +describe("and R3 refuses it anyway", () => { + beforeEach(() => { + delegationFindMany.mockResolvedValue([delegationFrom("dir_1", "The Director")]) + getUserContext.mockResolvedValue(DIRECTOR) + }) + + it("a proposer holding a Director delegation cannot approve their own proposal", async () => { + const { actor } = await effectiveOnboardingActor("staff_1", PROPOSER, INST) + + expect(actor.userId).toBe("staff_1") + expect(canDecide(actor, ownProposal)).toBe(false) + expect(refusalFor(actor, ownProposal, "approve")).toMatch( + /cannot be decided by the person who raised it/, + ) + expect(refusalFor(actor, ownProposal, "reject")).toMatch( + /cannot be decided by the person who raised it/, + ) + // The only thing left to them is retracting it, which is the author's right + // and grants nothing. + expect(availableActions(actor, ownProposal)).toEqual(["withdraw"]) + }) + + it("nor if they ALSO hold the Director role in their own right", async () => { + // Belt and braces: R3 is about identity, so a Director raising a proposal is + // refused whether or not delegation is in the picture. + const dirWhoProposed = ctxFor("dir_1", [{ institutionId: INST, role: "OSE_DIRECTOR" }]) + getUserContext.mockResolvedValue(ctxFor("dir_2", [{ institutionId: INST, role: "OSE_DIRECTOR" }])) + delegationFindMany.mockResolvedValue([delegationFrom("dir_2", "The Other Director")]) + + const { actor } = await effectiveOnboardingActor("dir_1", dirWhoProposed, INST) + expect(canDecide(actor, { ...ownProposal, submittedById: "dir_1" })).toBe(false) + }) + + it("a GENUINE delegate still decides other people's proposals", async () => { + // Otherwise delegation would be useless and the Director's absence would + // stall onboarding — which is how a control becomes the thing people route + // around. Same delegation row, different actor. + const backup = ctxFor("backup_1", [{ institutionId: INST, role: "OSE_STAFF" }]) + const { actor } = await effectiveOnboardingActor("backup_1", backup, INST) + + expect(actor.userId).toBe("backup_1") + expect(canDecide(actor, ownProposal)).toBe(true) + expect(availableActions(actor, ownProposal).sort()).toEqual(["approve", "reject"]) + }) + + it("a delegation from ANOTHER institution's Director confers nothing here", async () => { + // `effectiveApprovalContext` filters by institution, and `holds` filters + // again. Both matter: the first is a query and queries get edited. + getUserContext.mockResolvedValue( + ctxFor("dir_x", [{ institutionId: OTHER_INST, role: "OSE_DIRECTOR" }]), + ) + const outsider = ctxFor("staff_2", [{ institutionId: INST, role: "OSE_STAFF" }]) + const { actor } = await effectiveOnboardingActor("staff_2", outsider, INST) + + expect(canDecide(actor, ownProposal)).toBe(false) + }) +}) + +describe("the test that fails if R3 is 'simplified' to a role check", () => { + /** + * What R3 becomes if somebody removes the identity comparison and leaves the + * role check that sits right beneath it — the single most likely edit, because + * from inside `canDecide` the identity line looks redundant next to a check + * that already demands OSE_DIRECTOR. + */ + const roleShapedR3 = (actor: OnboardingActor, proposal: ProposalView): boolean => + actor.institutionRoles.some( + (m) => m.institutionId === proposal.institutionId && m.role === "OSE_DIRECTOR", + ) + + it("the role-shaped rule IS defeated by the delegation, and the real one is not", async () => { + delegationFindMany.mockResolvedValue([delegationFrom("dir_1", "The Director")]) + getUserContext.mockResolvedValue(DIRECTOR) + const { actor } = await effectiveOnboardingActor("staff_1", PROPOSER, INST) + + // The attack succeeds against the simplification... + expect(roleShapedR3(actor, ownProposal)).toBe(true) + // ...and fails against the rule as written. + expect(canDecide(actor, ownProposal)).toBe(false) + // Stated as the disagreement itself, so simplifying `canDecide` to the role + // check makes these two agree and fails HERE, naming the reason. + expect(canDecide(actor, ownProposal)).not.toBe(roleShapedR3(actor, ownProposal)) + }) + + it("no set of roles whatsoever lets the submitter decide their own proposal", () => { + // The property, proven by enumeration rather than asserted. Any rule + // expressed in terms of roles must say yes for at least one of these eight + // — the ones containing OSE_DIRECTOR — so this fails the moment R3 stops + // being about identity, no matter which role-shaped rule replaces it. + const ROLES: InstitutionRole[] = ["OSE_DIRECTOR", "OSE_STAFF", "OSE_ADVISOR"] + const subsets: InstitutionRole[][] = [] + for (let mask = 0; mask < 1 << ROLES.length; mask++) { + subsets.push(ROLES.filter((_, i) => mask & (1 << i))) + } + expect(subsets).toHaveLength(8) + + let sawDirector = 0 + for (const roles of subsets) { + const actor: OnboardingActor = { + userId: "staff_1", // the submitter, always + institutionRoles: roles.map((role) => ({ institutionId: INST, role })), + evaluatedAt: NOW, + } + if (roles.includes("OSE_DIRECTOR")) sawDirector++ + expect(canDecide(actor, ownProposal)).toBe(false) + expect(availableActions(actor, ownProposal)).not.toContain("approve") + } + // The enumeration really did include the dangerous cases. + expect(sawDirector).toBe(4) + }) +}) + +describe("the invariant R3 rests on, guarded where it is used", () => { + it("toOnboardingActor carries the real identity and the context's clock", () => { + const a = toOnboardingActor(PROPOSER) + expect(a.userId).toBe("staff_1") + expect(a.evaluatedAt).toBe(NOW) + }) + + it("throws if the delegation resolver ever stops preserving identity", async () => { + // R3 is checked on `actor.userId`, and that field is filled in by another + // module. A refactor there that returned the delegator's context wholesale + // would re-open the attack silently — every rule would still look right. + // So it is asserted at the point of use, and this is the negative control + // for that assertion. + jest.resetModules() + jest.doMock("@/lib/delegation", () => ({ + effectiveApprovalContext: async (_u: string, ctx: UserContext) => ({ + ctx: { ...ctx, userId: "dir_1" }, // the regression, made concrete + delegators: [{ id: "dir_1", name: "The Director" }], + }), + })) + const mod = await import("./onboarding-actor") + + await expect(mod.effectiveOnboardingActor("staff_1", PROPOSER, INST)).rejects.toThrow( + /must preserve the acting user's identity/, + ) + + jest.dontMock("@/lib/delegation") + jest.resetModules() + }) +}) diff --git a/apps/web/src/lib/identity/onboarding-actor.ts b/apps/web/src/lib/identity/onboarding-actor.ts new file mode 100644 index 00000000..0667a5c8 --- /dev/null +++ b/apps/web/src/lib/identity/onboarding-actor.ts @@ -0,0 +1,95 @@ +import { effectiveApprovalContext } from "@/lib/delegation" +import type { UserContext } from "@/lib/rbac" +import type { OnboardingActor } from "./onboarding-chain" + +/** + * Turning a signed-in person into an actor the onboarding chain can judge — + * including the delegation step, which is where this feature could be defeated. + * + * ── The attack ────────────────────────────────────────────────────────────── + * + * `ApprovalDelegation` lets a Director name a backup. `effectiveApprovalContext` + * implements it by reading every active delegation TO the actor and pushing the + * DELEGATOR'S ENTIRE ROLE SET onto the actor's context. So: + * + * 1. staff_1 raises a proposal. `submittedById = staff_1`. + * 2. The Director creates `ApprovalDelegation { from: director, to: staff_1 }` + * — for any reason at all: a holiday, a conference, a routine backup. + * 3. staff_1 now carries OSE_DIRECTOR at that institution. + * + * At step 3 every ROLE-shaped expression of "the proposer may not decide their + * own proposal" is already defeated. "The actor is not merely staff" is false — + * they hold Director. "The actor holds authority the submitter lacks" is false — + * they are the submitter and they hold it. A chain guarded that way would let + * staff_1 approve staff_1, and the Director would have handed it over by doing + * something entirely ordinary. + * + * ── Why it does not work ──────────────────────────────────────────────────── + * + * R3 is expressed on IDENTITY: `actor.userId === proposal.submittedById`. + * Delegation lends roles and never changes `userId` — `effectiveApprovalContext` + * returns `{ userId, ...merged roles }`, preserving the real actor precisely so + * that "is this the requester?" stays answerable. So the merged context is still + * staff_1, and R3 still refuses. + * + * That property is load-bearing and it lives in ANOTHER module, so this one + * asserts it rather than trusting it: if `effectiveApprovalContext` is ever + * changed to return the delegator's id, `toOnboardingActor` throws instead of + * quietly handing the chain an actor wearing somebody else's identity. + * `onboarding-actor.test.ts` drives the whole path — real delegation resolver, + * real merge, real rule — and proves the refusal. + * + * ── Why delegation is honoured at all ─────────────────────────────────────── + * + * Because refusing it would be worse. A Director on leave with no backup means + * proposals sit until they expire, and a control that blocks the day job is a + * control that gets switched off. A genuine delegate deciding OTHER people's + * proposals is exactly what delegation is for; R3 removes the one case that + * matters and leaves the rest working. + */ + +/** + * A `UserContext` as the chain sees it. + * + * Direct authority only — no delegation. Use it for reads and for a listing; use + * `effectiveOnboardingActor` where an action is about to be taken. + */ +export function toOnboardingActor(ctx: UserContext): OnboardingActor { + return { + userId: ctx.userId, + institutionRoles: ctx.institutionRoles, + // The context's clock, not a fresh reading: two checks rendering one page + // must not disagree about whether a deadline had passed. + evaluatedAt: ctx.evaluatedAt, + } +} + +/** + * The actor with any borrowed authority merged in, plus whose it was. + * + * `delegators` is returned so the caller can record "on behalf of" on the + * proposal's event row — a borrowed decision that does not say it was borrowed + * is a worse record than no delegation at all. + */ +export async function effectiveOnboardingActor( + userId: string, + ctx: UserContext, + institutionId: string, +): Promise<{ actor: OnboardingActor; delegators: { id: string; name: string }[] }> { + const { ctx: merged, delegators } = await effectiveApprovalContext(userId, ctx, institutionId) + + // The guard, not a formality. R3 is the only thing standing between a + // delegated proposer and their own approval, and it is checked on this exact + // field. A future refactor that "simplifies" the merge by returning the + // delegator's context wholesale would silently re-open the attack; this turns + // that into a loud failure at the point of use. + if (merged.userId !== userId) { + throw new Error( + "effectiveApprovalContext must preserve the acting user's identity: " + + `expected ${userId}, got ${merged.userId}. Delegation lends roles, never identity — ` + + "R3 (a proposer may not decide their own proposal) is checked on this field.", + ) + } + + return { actor: toOnboardingActor(merged), delegators } +} diff --git a/apps/web/src/lib/identity/onboarding-attack.itest.ts b/apps/web/src/lib/identity/onboarding-attack.itest.ts new file mode 100644 index 00000000..f7595327 --- /dev/null +++ b/apps/web/src/lib/identity/onboarding-attack.itest.ts @@ -0,0 +1,286 @@ +import { PrismaClient, type InstitutionRole } from "@prisma/client" +import { Refusal } from "@/lib/admin/action-state" +import type { UserContext } from "@/lib/rbac" +import { actOnProposal, createProposal, decidersFor } from "./onboarding-proposals" + +/** + * Adversarial, against a real PostgreSQL. No mocked database. + * + * The unit suite mocks `@/lib/db`, so every write it asserts on is a write that + * never met a foreign key. These run the same commands against the schema. + */ +const raw = new PrismaClient({ log: ["error"] }) + +const S = "atk116" +const A = `inst-a-${S}` +const B = `inst-b-${S}` +const GHOST = `inst-ghost-${S}` // deliberately never created +const NOW = new Date("2026-08-21T12:00:00.000Z") + +const ctx = (userId: string, roles: { institutionId: string; role: InstitutionRole }[]): UserContext => ({ + userId, + evaluatedAt: NOW, + institutionRoles: roles, + orgRoles: [], +}) + +const STAFF_A = ctx(`staff-a-${S}`, [{ institutionId: A, role: "OSE_STAFF" }]) +const DIR_A = ctx(`dir-a-${S}`, [{ institutionId: A, role: "OSE_DIRECTOR" }]) +const DIR2_A = ctx(`dir2-a-${S}`, [{ institutionId: A, role: "OSE_DIRECTOR" }]) +const STAFF_B = ctx(`staff-b-${S}`, [{ institutionId: B, role: "OSE_STAFF" }]) +const DIR_B = ctx(`dir-b-${S}`, [{ institutionId: B, role: "OSE_DIRECTOR" }]) + +const ALL = [STAFF_A, DIR_A, DIR2_A, STAFF_B, DIR_B] + +async function wipe() { + await raw.onboardingProposalEvent.deleteMany({ where: { institutionId: { in: [A, B] } } }) + await raw.onboardingProposal.deleteMany({ where: { institutionId: { in: [A, B] } } }) + await raw.auditEvent.deleteMany({ where: { institutionId: { in: [A, B] } } }) + await raw.approvalDelegation.deleteMany({ where: { institutionId: { in: [A, B] } } }) + await raw.restrictedIdentity.deleteMany({ where: { institutionId: { in: [A, B] } } }) + await raw.institutionMembership.deleteMany({ where: { institutionId: { in: [A, B] } } }) + await raw.user.deleteMany({ where: { id: { in: ALL.map((c) => c.userId) } } }) + await raw.institution.deleteMany({ where: { id: { in: [A, B] } } }) +} + +beforeAll(async () => { + await wipe() + for (const [id, name] of [[A, "A"], [B, "B"]] as const) { + await raw.institution.create({ data: { id, name: `Inst ${name} ${S}`, slug: `${id}` } }) + } + for (const c of ALL) { + await raw.user.create({ data: { id: c.userId, email: `${c.userId}@example.test`, name: c.userId } }) + for (const m of c.institutionRoles) { + await raw.institutionMembership.create({ + data: { userId: c.userId, institutionId: m.institutionId, role: m.role }, + }) + } + } +}) + +afterAll(async () => { + await wipe() + await raw.$disconnect() +}) + +beforeEach(async () => { + await raw.onboardingProposalEvent.deleteMany({ where: { institutionId: { in: [A, B] } } }) + await raw.onboardingProposal.deleteMany({ where: { institutionId: { in: [A, B] } } }) + await raw.auditEvent.deleteMany({ where: { institutionId: { in: [A, B] } } }) + await raw.approvalDelegation.deleteMany({ where: { institutionId: { in: [A, B] } } }) +}) + +// `example.test` rather than the tenant's domain, twice over: RFC 2606 reserves +// it so nothing can be delivered to one, and `fork-prevention.test.ts` refuses a +// new file that hard-codes a tenant. The domain is not the boundary anyway — the +// roster is — so nothing here depends on which domain these addresses carry. +const subject = (n: string) => ({ + institutionId: A, + subjectName: `Subject ${n}`, + subjectEmail: `subject-${n}-${S}@example.test`, + subjectKind: "MEMBER" as const, + cohort: "STUDENT_LEADER", +}) + +async function pending(by: UserContext, n: string) { + const p = await createProposal(by, subject(n)) + return actOnProposal(by, A, p.id, "submit") +} + +// ─── ATTACK 1: approve your own proposal ───────────────────────────────────── + +describe("ATTACK — self-approval", () => { + it("staff cannot approve the proposal they raised", async () => { + const p = await pending(STAFF_A, "a1") + await expect(actOnProposal(STAFF_A, A, p.id, "approve")).rejects.toThrow(Refusal) + const after = await raw.onboardingProposal.findUniqueOrThrow({ where: { id: p.id } }) + expect(after.status).toBe("PENDING_DIRECTOR") + }) + + it("the DIRECTOR cannot approve the proposal the Director raised", async () => { + const p = await pending(DIR_A, "a2") + await expect(actOnProposal(DIR_A, A, p.id, "approve")).rejects.toThrow( + /cannot be decided by the person who raised it/i, + ) + const after = await raw.onboardingProposal.findUniqueOrThrow({ where: { id: p.id } }) + expect(after.status).toBe("PENDING_DIRECTOR") + expect(after.decidedById).toBeNull() + }) + + it("nor reject it — R3 covers both halves of a decision", async () => { + const p = await pending(DIR_A, "a3") + await expect(actOnProposal(DIR_A, A, p.id, "reject")).rejects.toThrow(Refusal) + }) + + it("a REAL Director delegation to the proposer does not defeat it", async () => { + // The setup must actually work, or the refusal proves nothing. + await raw.approvalDelegation.create({ + data: { institutionId: A, fromUserId: DIR_A.userId, toUserId: STAFF_A.userId }, + }) + const deciders = await decidersFor(DIR_A, { + institutionId: A, + submittedById: "someone-else", + status: "PENDING_DIRECTOR", + expiresAt: new Date("2027-01-01"), + }) + expect(deciders!.userIds).toContain(STAFF_A.userId) // the borrowed authority is real + + const p = await pending(STAFF_A, "a4") + await expect(actOnProposal(STAFF_A, A, p.id, "approve")).rejects.toThrow( + /cannot be decided by the person who raised it/i, + ) + const after = await raw.onboardingProposal.findUniqueOrThrow({ where: { id: p.id } }) + expect(after.status).toBe("PENDING_DIRECTOR") + }) + + it("and the same delegate DOES decide somebody else's proposal", async () => { + await raw.approvalDelegation.create({ + data: { institutionId: A, fromUserId: DIR_A.userId, toUserId: STAFF_B.userId }, + }) + const p = await pending(STAFF_A, "a5") + const out = await actOnProposal(STAFF_B, A, p.id, "approve") + expect(out.status).toBe("APPROVED") + expect(out.decidedById).toBe(STAFF_B.userId) + }) +}) + +// ─── ATTACK 2: escalation ──────────────────────────────────────────────────── + +describe("ATTACK — escalation", () => { + it("an approved proposal creates NO membership, role or seat of any kind", async () => { + const p = await pending(STAFF_A, "e1") + await actOnProposal(DIR_A, A, p.id, "approve") + expect(await raw.institutionMembership.count({ where: { institutionId: A } })).toBe( + ALL.filter((c) => c.institutionRoles.some((m) => m.institutionId === A)).length, + ) + expect(await raw.restrictedIdentity.count({ where: { institutionId: A } })).toBe(0) + expect(await raw.roleAssignment.count({ where: { institutionId: A } })).toBe(0) + }) +}) + +// ─── ATTACK 3: cross-tenant ────────────────────────────────────────────────── + +describe("ATTACK — cross-tenant", () => { + it("B's Director cannot approve A's proposal", async () => { + const p = await pending(STAFF_A, "x1") + await expect(actOnProposal(DIR_B, A, p.id, "approve")).rejects.toThrow(Refusal) + const after = await raw.onboardingProposal.findUniqueOrThrow({ where: { id: p.id } }) + expect(after.status).toBe("PENDING_DIRECTOR") + }) + + it("B's staff cannot raise a proposal into A", async () => { + await expect(createProposal(STAFF_B, subject("x2"))).rejects.toThrow(Refusal) + expect(await raw.onboardingProposal.count({ where: { institutionId: A } })).toBe(0) + // and the refusal is logged where the ACTOR is accountable, not in A's log + expect(await raw.auditEvent.count({ where: { institutionId: A } })).toBe(0) + expect(await raw.auditEvent.count({ where: { institutionId: B } })).toBe(1) + }) + + it("an outsider naming A does not get a row written into A's audit log", async () => { + // The same standard `auditRefusedProposal` already holds `createProposal` + // to. `actOnProposal` takes the institution id straight from its caller. + await expect(actOnProposal(DIR_B, A, `no-such-proposal-${S}`, "approve")).rejects.toThrow( + Refusal, + ) + expect(await raw.auditEvent.count({ where: { institutionId: A } })).toBe(0) + }) + + it("cannot tell a real proposal at A from an imaginary one", async () => { + const p = await pending(STAFF_A, "x3") + const real = await actOnProposal(DIR_B, A, p.id, "approve").catch((e: Error) => e.message) + const imaginary = await actOnProposal(DIR_B, A, `nope-${S}`, "approve").catch( + (e: Error) => e.message, + ) + expect(real).toBe(imaginary) + }) + + it("naming an institution that does not exist is a refusal, not a P2003", async () => { + await expect( + actOnProposal(DIR_B, GHOST, `no-such-proposal-${S}`, "approve"), + ).rejects.toThrow(Refusal) + }) +}) + +// ─── ATTACK 4: the billing grain ───────────────────────────────────────────── + +describe("ATTACK — the billing grain", () => { + it("the person on FOUR seats is ONE proposal and ZERO ledger entries", async () => { + // Four seats, one person. The chain admits the PERSON; nothing here counts + // seats, so four is one — and, in this PR, one is still zero charges. + const org = await raw.organization.create({ + data: { id: `org-${S}`, institutionId: A, name: `Club ${S}`, slug: `club-${S}` }, + }) + const four = { + institutionId: A, + subjectName: "Four Seat Person", + subjectEmail: `four-seats-${S}@example.test`, + subjectKind: "MEMBER" as const, + cohort: "STUDENT_LEADER", + organizationId: org.id, + } + const ledgerBefore = await raw.ledgerEntry.count() + const p = await createProposal(STAFF_A, four) + await actOnProposal(STAFF_A, A, p.id, "submit") + const approved = await actOnProposal(DIR_A, A, p.id, "approve") + expect(approved.status).toBe("APPROVED") + + expect( + await raw.onboardingProposal.count({ + where: { institutionId: A, subjectEmailNormalized: four.subjectEmail.toLowerCase() }, + }), + ).toBe(1) + expect(await raw.ledgerEntry.count()).toBe(ledgerBefore) + await raw.organization.delete({ where: { id: org.id } }).catch(() => {}) + }) + + it("the same approval delivered TWICE settles once", async () => { + const p = await pending(STAFF_A, "b2") + const first = await actOnProposal(DIR_A, A, p.id, "approve") + await expect(actOnProposal(DIR_A, A, p.id, "approve")).rejects.toThrow(/already approved/i) + const events = await raw.onboardingProposalEvent.findMany({ + where: { proposalId: p.id, kind: "APPROVED" }, + }) + expect(events).toHaveLength(1) + const after = await raw.onboardingProposal.findUniqueOrThrow({ where: { id: p.id } }) + expect(after.decidedAt?.toISOString()).toBe(first.decidedAt?.toISOString()) + }) + + it("a settled proposal cannot be reversed into another outcome", async () => { + const p = await pending(STAFF_A, "b3") + await actOnProposal(DIR_A, A, p.id, "reject") + for (const a of ["approve", "withdraw", "submit"] as const) { + await expect(actOnProposal(DIR_A, A, p.id, a)).rejects.toThrow(Refusal) + } + const after = await raw.onboardingProposal.findUniqueOrThrow({ where: { id: p.id } }) + expect(after.status).toBe("REJECTED") + }) + + it("one OPEN proposal per person, and the second is a sentence not a stack trace", async () => { + await createProposal(STAFF_A, subject("b4")) + await expect(createProposal(STAFF_A, subject("b4"))).rejects.toThrow( + /already an open proposal/i, + ) + }) +}) + +// ─── ATTACK 5: what reaches the registry ───────────────────────────────────── + +describe("ATTACK — input that reaches the access boundary", () => { + it("refuses an address that has no local part and no domain", async () => { + await expect( + createProposal(STAFF_A, { ...subject("v1"), subjectEmail: "@" }), + ).rejects.toThrow(/valid institutional email/) + }) + + it("refuses an address with a domain but no local part", async () => { + await expect( + createProposal(STAFF_A, { ...subject("v2"), subjectEmail: "@example.test" }), + ).rejects.toThrow(/valid institutional email/) + }) + + it("refuses an address with whitespace inside it", async () => { + await expect( + createProposal(STAFF_A, { ...subject("v3"), subjectEmail: "two words@example.test" }), + ).rejects.toThrow(/valid institutional email/) + }) +}) diff --git a/apps/web/src/lib/identity/onboarding-chain.test.ts b/apps/web/src/lib/identity/onboarding-chain.test.ts index 7c52b2a6..964c27fb 100644 --- a/apps/web/src/lib/identity/onboarding-chain.test.ts +++ b/apps/web/src/lib/identity/onboarding-chain.test.ts @@ -1,14 +1,25 @@ -import { existsSync, readFileSync, readdirSync, statSync } from "node:fs" +import { readFileSync } from "node:fs" import path from "node:path" +import { CAPABILITIES, rolesHolding } from "@/lib/admin/capabilities" import { ALL_ACTIONS, ALL_STATUSES, + DRAFT_TTL_DAYS, + MAY_DECIDE, + MAY_PROPOSE, + PENDING_TTL_DAYS, availableActions, canDecide, canPropose, + chainStall, decideRefusal, + effectiveProposalStatus, + expiryFor, + expirySweepTarget, grantsRegistryEntry, + isTerminal, nextStatus, + refusalFor, type OnboardingActor, type OnboardingStatus, type ProposalView, @@ -16,15 +27,25 @@ import { const INST = "inst_simon" -const actor = (userId: string, role?: "OSE_DIRECTOR" | "OSE_STAFF" | "OSE_ADVISOR"): OnboardingActor => ({ +/** A fixed clock. Every rule here is judged at an instant, so the tests name one. */ +const NOW = new Date("2026-08-21T12:00:00.000Z") +const FAR_FUTURE = new Date("2027-01-01T00:00:00.000Z") + +const actor = ( + userId: string, + role?: "OSE_DIRECTOR" | "OSE_STAFF" | "OSE_ADVISOR", + at: Date = NOW, +): OnboardingActor => ({ userId, institutionRoles: role ? [{ institutionId: INST, role }] : [], + evaluatedAt: at, }) const proposal = (over: Partial = {}): ProposalView => ({ institutionId: INST, submittedById: "staff_1", status: "PENDING_DIRECTOR", + expiresAt: FAR_FUTURE, ...over, }) @@ -53,6 +74,7 @@ describe("R1 — who may propose", () => { const elsewhere: OnboardingActor = { userId: "s", institutionRoles: [{ institutionId: "inst_other", role: "OSE_STAFF" }], + evaluatedAt: NOW, } expect(canPropose(elsewhere, INST)).toBe(false) }) @@ -78,6 +100,7 @@ describe("R2 — only the Director decides", () => { const elsewhere: OnboardingActor = { userId: "dir", institutionRoles: [{ institutionId: "inst_other", role: "OSE_DIRECTOR" }], + evaluatedAt: NOW, } expect(canDecide(elsewhere, proposal())).toBe(false) }) @@ -106,6 +129,7 @@ describe("R3 — the proposer never decides their own proposal", () => { { institutionId: INST, role: "OSE_STAFF" }, { institutionId: INST, role: "OSE_DIRECTOR" }, // inherited from the Director ], + evaluatedAt: NOW, } const own = proposal({ submittedById: "staff_1" }) expect(canDecide(proposerWithDelegatedDirector, own)).toBe(false) @@ -122,6 +146,7 @@ describe("R3 — the proposer never decides their own proposal", () => { { institutionId: INST, role: "OSE_STAFF" }, { institutionId: INST, role: "OSE_DIRECTOR" }, ], + evaluatedAt: NOW, } expect(canDecide(delegate, proposal({ submittedById: "staff_1" }))).toBe(true) }) @@ -154,7 +179,7 @@ describe("available actions by state", () => { }) it("every terminal state offers nothing to anyone", () => { - for (const status of ["APPROVED", "REJECTED", "WITHDRAWN"] as OnboardingStatus[]) { + for (const status of ["APPROVED", "REJECTED", "WITHDRAWN", "EXPIRED"] as OnboardingStatus[]) { for (const who of [dir, author, actor("adv", "OSE_ADVISOR")]) { expect(availableActions(who, proposal({ status }))).toEqual([]) } @@ -165,7 +190,7 @@ describe("available actions by state", () => { for (const status of ALL_STATUSES) { expect(() => availableActions(dir, proposal({ status }))).not.toThrow() } - expect(ALL_STATUSES).toHaveLength(5) + expect(ALL_STATUSES).toHaveLength(6) }) }) @@ -189,7 +214,7 @@ describe("the state machine", () => { it("withdraw acts before a decision and not after", () => { expect(nextStatus("withdraw", "DRAFT")).toBe("WITHDRAWN") expect(nextStatus("withdraw", "PENDING_DIRECTOR")).toBe("WITHDRAWN") - for (const s of ["APPROVED", "REJECTED", "WITHDRAWN"] as OnboardingStatus[]) { + for (const s of ["APPROVED", "REJECTED", "WITHDRAWN", "EXPIRED"] as OnboardingStatus[]) { expect(nextStatus("withdraw", s)).toBeNull() } }) @@ -197,7 +222,7 @@ describe("the state machine", () => { it("no action escapes a terminal state", () => { // Reversing a decision is a new proposal, so the record of what was decided // stays true. - for (const s of ["APPROVED", "REJECTED", "WITHDRAWN"] as OnboardingStatus[]) { + for (const s of ["APPROVED", "REJECTED", "WITHDRAWN", "EXPIRED"] as OnboardingStatus[]) { for (const a of ALL_ACTIONS) expect(nextStatus(a, s)).toBeNull() } }) @@ -247,43 +272,230 @@ describe("the acceptance test the requirement names", () => { }) }) -describe("the module is inert until the store is decided", () => { - it("is imported by nothing but its own test, and no barrel can hide it", () => { - // ADR-0013 says the rules ship and the store does not, and that the module - // is "inert until then, and deliberately so". That is a claim about the - // repository, so it is checked rather than asserted in prose. - // - // The failure this prevents is a half-ship: wiring the chain into a surface - // before deciding where proposals live means the persistence choice gets - // made by whoever writes the first `create`, silently, and ADR-0013 becomes - // a description of a decision nobody took. - // - // A review bot pointed out that a text search for the module path misses a - // re-export — `lib/identity/index.ts` exporting the chain would make it - // reachable as `@/lib/identity` while this guard still reported "inert". So - // the barrel is forbidden outright rather than parsed for: a directory with - // one module does not need one, and its absence is far cheaper to check - // than a module graph is to walk. - // - // When the ADR is accepted, this test is DELETED in the same change that - // adds the store — not weakened, and not skipped. - const SRC = path.resolve(__dirname, "../..") - const walk = (dir: string): string[] => - readdirSync(dir).flatMap((e) => { - if (e === "node_modules" || e === ".next") return [] - const full = path.join(dir, e) - return statSync(full).isDirectory() ? walk(full) : /\.tsx?$/.test(e) ? [full] : [] - }) - - const importers = walk(SRC) - .filter((f) => !f.endsWith("onboarding-chain.test.ts")) - .filter((f) => !f.endsWith("onboarding-chain.ts")) - .filter((f) => /identity\/onboarding-chain|from "@\/lib\/identity"/.test(readFileSync(f, "utf8"))) - .map((f) => path.relative(SRC, f)) - - expect(importers).toEqual([]) - // No barrel: `@/lib/identity` must not resolve to anything. - expect(existsSync(path.join(SRC, "lib/identity/index.ts"))).toBe(false) - expect(existsSync(path.join(SRC, "lib/identity/index.tsx"))).toBe(false) + +describe("the store landed, so the module is no longer inert", () => { + // This file used to assert that nothing imported the chain — ADR-0013 said the + // rules ship and the store does not, and that guard is what stopped the + // persistence choice being made silently by whoever wrote the first `create`. + // + // ADR-0013 is now Accepted, the store is `onboarding-proposals.ts`, and the + // ADR itself said this test is DELETED in the same change that adds the store + // — not weakened and not skipped. What replaces it is the claim that actually + // matters now: the chain is REACHED. A rules module nobody calls and a rules + // module everybody bypasses look identical from the outside. + it("is imported by the store that executes it", () => { + const store = readFileSync(path.join(__dirname, "onboarding-proposals.ts"), "utf8") + expect(store).toMatch(/from "\.\/onboarding-chain"/) + expect(store).toMatch(/\brefusalFor\b/) + }) +}) + +describe("R1 and R2 are DERIVED from the capability catalog", () => { + // Two lists of role names for one power is a second authorization path + // wearing a helpful comment. These pin the derivation in both directions, so + // editing either end without the other fails here. + it("R1 is exactly who holds onboarding.propose", () => { + expect([...MAY_PROPOSE]).toEqual(rolesHolding("onboarding.propose")) + expect([...MAY_PROPOSE]).toEqual(["OSE_DIRECTOR", "OSE_STAFF"]) + expect(CAPABILITIES["onboarding.propose"].minRole).toBe("OSE_STAFF") + }) + + it("R2 is exactly one role, and it is the Director", () => { + // The load-bearing one. The club engine's final gate is `isOse`, which is + // true for ANY institution membership including OSE_ADVISOR. Lowering + // onboarding.decide's minRole would silently make this chain that wide. + expect([...MAY_DECIDE]).toEqual(rolesHolding("onboarding.decide")) + expect([...MAY_DECIDE]).toEqual(["OSE_DIRECTOR"]) + expect(CAPABILITIES["onboarding.decide"].minRole).toBe("OSE_DIRECTOR") + }) + + it("an advisor holds neither", () => { + expect(rolesHolding("onboarding.propose")).not.toContain("OSE_ADVISOR") + expect(rolesHolding("onboarding.decide")).not.toContain("OSE_ADVISOR") + }) +}) + +describe("the statuses match the database's", () => { + it("ALL_STATUSES is exactly the OnboardingProposalStatus enum in schema.prisma", () => { + // Reasoning about a rules module against a set of states the database does + // not have is how a status becomes unreachable — or worse, unhandled. Read + // rather than restated, so adding one anywhere fails until it is handled + // everywhere. Anchored on __dirname for the same reason registry.test.ts is. + const schema = readFileSync( + path.join(__dirname, "..", "..", "..", "prisma", "schema.prisma"), + "utf8", + ) + const block = /enum OnboardingProposalStatus \{([\s\S]*?)\}/.exec(schema) + expect(block).not.toBeNull() + const members = block![1] + .split("\n") + .map((l) => l.replace(/\/\/.*/, "").replace(/\/\/\/.*/, "").trim()) + .filter((l) => /^[A-Z_]+$/.test(l)) + expect(members.sort()).toEqual([...ALL_STATUSES].sort()) + }) +}) + +describe("expiry — a proposal that sits forever is a person waiting forever", () => { + const DAY = 24 * 60 * 60 * 1000 + + it("dates a pending proposal 30 days out and a draft 90", () => { + expect(expiryFor("PENDING_DIRECTOR", NOW).getTime()).toBe(NOW.getTime() + PENDING_TTL_DAYS * DAY) + expect(expiryFor("DRAFT", NOW).getTime()).toBe(NOW.getTime() + DRAFT_TTL_DAYS * DAY) + expect(PENDING_TTL_DAYS).toBe(30) + expect(DRAFT_TTL_DAYS).toBe(90) + }) + + it("expires AT the deadline, not a tick later", () => { + // Half-open, exactly as rbac.ts's effective-dating window is. + const deadline = new Date(NOW.getTime()) + expect(effectiveProposalStatus("PENDING_DIRECTOR", deadline, NOW)).toBe("EXPIRED") + expect( + effectiveProposalStatus("PENDING_DIRECTOR", new Date(NOW.getTime() + 1), NOW), + ).toBe("PENDING_DIRECTOR") + }) + + it("NEVER un-decides a terminal proposal", () => { + // The clock only ever subtracts. A deadline cannot un-approve an approval, + // and an expired proposal cannot be resurrected by moving the clock back. + const past = new Date(NOW.getTime() - 1) + for (const s of ["APPROVED", "REJECTED", "WITHDRAWN", "EXPIRED"] as OnboardingStatus[]) { + expect(effectiveProposalStatus(s, past, NOW)).toBe(s) + expect(isTerminal(s)).toBe(true) + } + expect(isTerminal("DRAFT")).toBe(false) + expect(isTerminal("PENDING_DIRECTOR")).toBe(false) + }) + + it("refuses every action on a lapsed proposal, INCLUDING the Director's", () => { + // The point of putting the clock in the rule rather than in a cron job: an + // unrun sweep cannot leave a stale proposal decidable, so a Director cannot + // approve a lapsed proposal by being quick. + const lapsed = proposal({ status: "PENDING_DIRECTOR", expiresAt: new Date(NOW.getTime() - 1) }) + const dir = actor("dir", "OSE_DIRECTOR") + expect(availableActions(dir, lapsed)).toEqual([]) + expect(refusalFor(dir, lapsed, "approve")).toMatch(/expired without a decision/) + // And its author cannot withdraw it either — there is nothing left to withdraw. + expect(availableActions(actor("staff_1", "OSE_STAFF"), lapsed)).toEqual([]) + }) + + it("the sweep only records what the clock already decided", () => { + const lapsed = proposal({ status: "PENDING_DIRECTOR", expiresAt: new Date(NOW.getTime() - 1) }) + expect(expirySweepTarget(lapsed, NOW)).toBe("EXPIRED") + // Nothing to do when the record already agrees, and nothing to do to a + // proposal a person decided. + expect(expirySweepTarget(proposal({ status: "PENDING_DIRECTOR" }), NOW)).toBeNull() + expect(expirySweepTarget({ ...lapsed, status: "APPROVED" }, NOW)).toBeNull() + expect(expirySweepTarget({ ...lapsed, status: "EXPIRED" }, NOW)).toBeNull() + }) + + it("an EXPIRED proposal authorises nothing", () => { + expect(grantsRegistryEntry("EXPIRED")).toBe(false) + }) +}) + +describe("the Director seat — a role, never a person", () => { + const p = proposal({ submittedById: "staff_1" }) + + it("names nobody: the chain is headed by a ROLE", () => { + // A hard-coded person is a fork-prevention violation and breaks the day she + // is on leave. The rules module must contain no person, no address, no + // institution. + const src = readFileSync(path.join(__dirname, "onboarding-chain.ts"), "utf8") + expect(src).not.toMatch(/@[\w.-]+\.(edu|com|org)/) + expect(src.toLowerCase()).not.toContain("brittany") + expect(src).not.toContain("simon") + }) + + it("a vacant Director seat stalls the chain, visibly", () => { + // Staff do NOT inherit the power under load — that is R2, and relaxing it + // is how a chain 'headed by the Director' becomes a chain anyone can close. + // So the honest outcome is: nothing decides, and it is reported. + expect(chainStall(p, [])).toBe("NO_DIRECTOR") + expect(canDecide(actor("staff_2", "OSE_STAFF"), p)).toBe(false) + expect(availableActions(actor("staff_2", "OSE_STAFF"), p)).toEqual([]) + }) + + it("the only Director being the proposer stalls it too, and says which", () => { + const own = proposal({ submittedById: "dir_1" }) + expect(chainStall(own, ["dir_1"])).toBe("ONLY_DIRECTOR_IS_THE_PROPOSER") + expect(canDecide(actor("dir_1", "OSE_DIRECTOR"), own)).toBe(false) + }) + + it("a second Director, or a delegate, clears the stall", () => { + // The designed remedy. A delegate is not the submitter, so R3 does not + // touch them. + expect(chainStall(proposal({ submittedById: "dir_1" }), ["dir_1", "dir_2"])).toBeNull() + expect(chainStall(proposal({ submittedById: "dir_1" }), ["dir_1", "backup_1"])).toBeNull() + expect(chainStall(p, ["dir_1"])).toBeNull() + }) +}) + +describe("refusalFor is the single composed check", () => { + it("availableActions offers exactly the actions refusalFor permits", () => { + // Two expressions of one rule is how a listing comes to offer a button the + // command then refuses. Proven over the whole cross product rather than + // asserted. + const actors = [ + actor("dir", "OSE_DIRECTOR"), + actor("staff_1", "OSE_STAFF"), + actor("staff_2", "OSE_STAFF"), + actor("adv", "OSE_ADVISOR"), + actor("nobody"), + ] + const proposals = ALL_STATUSES.flatMap((status) => [ + proposal({ status }), + proposal({ status, expiresAt: new Date(NOW.getTime() - 1) }), + proposal({ status, submittedById: "dir" }), + ]) + for (const a of actors) { + for (const pr of proposals) { + const offered = availableActions(a, pr) + for (const action of ALL_ACTIONS) { + expect(offered.includes(action)).toBe(refusalFor(a, pr, action) === null) + } + } + } + }) + + it("never names the person under consideration", () => { + // A refusal that leaks whose address is being discussed defeats the point + // of keeping the payload off the audit row. + const reasons = new Set() + for (const status of ALL_STATUSES) { + for (const action of ALL_ACTIONS) { + for (const a of [actor("dir", "OSE_DIRECTOR"), actor("staff_1", "OSE_STAFF"), actor("x")]) { + const r = refusalFor(a, proposal({ status }), action) + if (r) reasons.add(r) + } + } + } + expect(reasons.size).toBeGreaterThan(3) + for (const r of reasons) { + expect(r).not.toMatch(/@/) + expect(r).not.toMatch(/staff_1|dir\b/) + } + }) +}) + +describe("R3 has exactly one implementation", () => { + it("canDecide and decideRefusal cannot disagree", () => { + // They used to be two copies of R2+R3, and a negative control caught it: + // breaking `canDecide` left `refusalFor` — the path every command takes — + // still refusing, which means the reverse edit could have removed R3 from + // the product while these tests stayed green. `canDecide` is derived now, + // and this fails if anybody re-splits them. + const actors = [ + actor("dir", "OSE_DIRECTOR"), + actor("staff_1", "OSE_STAFF"), + actor("staff_1", "OSE_DIRECTOR"), // the submitter, holding the deciding role + actor("adv", "OSE_ADVISOR"), + actor("nobody"), + ] + for (const a of actors) { + for (const submittedById of ["staff_1", "dir", "someone_else"]) { + const p = proposal({ submittedById }) + expect(canDecide(a, p)).toBe(decideRefusal(a, p) === null) + } + } }) }) diff --git a/apps/web/src/lib/identity/onboarding-chain.ts b/apps/web/src/lib/identity/onboarding-chain.ts index 6657d1b8..f9bdf55f 100644 --- a/apps/web/src/lib/identity/onboarding-chain.ts +++ b/apps/web/src/lib/identity/onboarding-chain.ts @@ -1,4 +1,5 @@ import type { InstitutionRole } from "@prisma/client" +import { rolesHolding } from "@/lib/admin/capabilities" /** * The approval chain for adding a person to the Tenant #1 access registry. @@ -54,6 +55,23 @@ import type { InstitutionRole } from "@prisma/client" * **R4 — Nothing reaches the registry before APPROVED.** The chain's terminal * state is the only thing that authorises a write, and that is asserted here * rather than left to the caller to remember. + * + * ── Two things this module gained when the store landed ───────────────────── + * + * **The role sets are DERIVED, not restated.** `MAY_PROPOSE` and `MAY_DECIDE` + * come from `onboarding.propose` and `onboarding.decide` in the capability + * catalog. Two lists of role names for one power is a second authorization + * path wearing a helpful comment, and the one that is wrong is whichever + * nobody edited. The catalog is pure — no database, no session, no framework — + * so nothing about this module's testability changed. + * + * **The clock decides expiry, not a job.** A proposal that sits forever is a + * person waiting forever, so a proposal carries `expiresAt` and every rule here + * is evaluated against `actor.evaluatedAt`. Past the deadline a non-terminal + * proposal READS as EXPIRED whatever the database says — which means an unrun + * sweep cannot leave a stale proposal decidable, and the sweep's only job is to + * make the record catch up. This is `rbac.ts` rule 3 in another table: the + * clock only ever SUBTRACTS authority. */ /** Where a proposal is. */ @@ -63,6 +81,8 @@ export type OnboardingStatus = | "APPROVED" | "REJECTED" | "WITHDRAWN" + /** Nobody decided in time. Reached by the clock, never by an actor. */ + | "EXPIRED" export type OnboardingAction = "submit" | "approve" | "reject" | "withdraw" @@ -76,20 +96,47 @@ export type OnboardingAction = "submit" | "approve" | "reject" | "withdraw" export interface OnboardingActor { readonly userId: string readonly institutionRoles: readonly { institutionId: string; role: InstitutionRole }[] + /** + * The instant every decision made from this actor is evaluated at. + * + * Explicit and stamped once, exactly as `UserContext.evaluatedAt` is, so two + * checks rendering the same page cannot disagree about whether a deadline had + * passed — and so a test can put the clock where it needs it without mocking + * global time. + */ + readonly evaluatedAt: Date } /** The proposal, reduced to what the rules need. */ export interface ProposalView { readonly institutionId: string readonly submittedById: string + /** What the DATABASE says. Every rule below reads the effective status instead. */ readonly status: OnboardingStatus + /** The instant this proposal stops being decidable. */ + readonly expiresAt: Date } -/** Roles permitted to raise a proposal. Ordered widest-authority-first. */ -const MAY_PROPOSE: readonly InstitutionRole[] = ["OSE_DIRECTOR", "OSE_STAFF"] +/** + * Roles permitted to raise a proposal, widest authority first. + * + * Derived from the capability catalog, which today answers + * `["OSE_DIRECTOR", "OSE_STAFF"]` — R1. A test pins that, so widening + * `onboarding.propose` fails CI here rather than silently changing who may put + * a name in front of the Director. + */ +export const MAY_PROPOSE: readonly InstitutionRole[] = rolesHolding("onboarding.propose") -/** The single role permitted to close the chain. */ -const MAY_DECIDE: InstitutionRole = "OSE_DIRECTOR" +/** + * Roles permitted to close the chain — R2. + * + * Today `["OSE_DIRECTOR"]`, because Director is the top rank and + * `onboarding.decide` is minRole Director. A test pins the derived set to + * exactly that one element: this is the rule that must stay narrower than the + * club engine's `isOse`, which is true for ANY institution membership including + * OSE_ADVISOR. + */ +export const MAY_DECIDE: readonly InstitutionRole[] = rolesHolding("onboarding.decide") function holds(actor: OnboardingActor, institutionId: string, roles: readonly InstitutionRole[]) { return actor.institutionRoles.some( @@ -102,15 +149,98 @@ export function canPropose(actor: OnboardingActor, institutionId: string): boole return holds(actor, institutionId, MAY_PROPOSE) } +// ─── Expiry ────────────────────────────────────────────────────────────────── + +/** + * How long a proposal stays open once it reaches the Director. + * + * 30 days is a deliberate choice between two failures. Shorter, and a Director + * on leave for a term loses proposals that were perfectly good — the office + * re-raises them and the record shows a refusal-shaped event for a person who + * did nothing wrong. Longer, and "we are still looking at it" stops being true + * long before it stops being said. Thirty days is also the window a delegation + * comfortably covers, which is the designed remedy for an absent Director. + */ +export const PENDING_TTL_DAYS = 30 + +/** + * How long a DRAFT survives. + * + * Longer, because nobody is waiting on a draft — but not unbounded, because a + * draft holds a named person's address and cohort just as a submitted proposal + * does, and an access-boundary system that accumulates unreviewed personal data + * forever is one nobody can defend. + */ +export const DRAFT_TTL_DAYS = 90 + +const DAY_MS = 24 * 60 * 60 * 1000 + +/** The deadline a proposal entering `status` at `at` should carry. */ +export function expiryFor(status: OnboardingStatus, at: Date): Date { + const days = status === "DRAFT" ? DRAFT_TTL_DAYS : PENDING_TTL_DAYS + return new Date(at.getTime() + days * DAY_MS) +} + +/** Terminal states — no action and no clock reading moves a proposal out of one. */ +export function isTerminal(status: OnboardingStatus): boolean { + return ( + status === "APPROVED" || + status === "REJECTED" || + status === "WITHDRAWN" || + status === "EXPIRED" + ) +} + +/** + * The status that actually binds at `at`: what the row says, capped by the + * clock. + * + * Deliberately one-directional. A terminal status is returned unchanged — a + * deadline cannot un-approve an approval — so like `rbac.ts` rule 3, the clock + * only ever subtracts. The window is half-open: `at >= expiresAt` has expired, + * so a proposal expires at its deadline instant rather than one tick later. + */ +export function effectiveProposalStatus( + stored: OnboardingStatus, + expiresAt: Date, + at: Date, +): OnboardingStatus { + if (isTerminal(stored)) return stored + return at.getTime() >= expiresAt.getTime() ? "EXPIRED" : stored +} + +/** + * Should the sweep write EXPIRED onto this row? + * + * Null when the record already agrees with the clock. The sweep exists only to + * make the stored value catch up; it grants nothing and revokes nothing that + * `effectiveProposalStatus` has not already decided. + */ +export function expirySweepTarget( + proposal: ProposalView, + at: Date, +): OnboardingStatus | null { + const effective = effectiveProposalStatus(proposal.status, proposal.expiresAt, at) + return effective === proposal.status ? null : effective +} + /** * R2 + R3 together: may this actor decide THIS proposal? * * Both halves are required and neither is sufficient. A Director who raised the * proposal is refused; a non-Director who did not raise it is refused. + * + * DERIVED from `decideRefusal` rather than implemented alongside it. It was + * implemented alongside it, and a negative control caught what that costs: both + * functions carried their own copy of R3, `refusalFor` — the path every command + * actually takes — routed through `decideRefusal`, and removing the identity + * check from `canDecide` left the store still refusing while the rule tests went + * red. The reverse edit is the dangerous one: R3 could have been deleted from + * the function the product uses while the function the tests exercise kept + * passing. One implementation, one place to break. */ export function canDecide(actor: OnboardingActor, proposal: ProposalView): boolean { - if (actor.userId === proposal.submittedById) return false - return holds(actor, proposal.institutionId, [MAY_DECIDE]) + return decideRefusal(actor, proposal) === null } /** @@ -124,52 +254,84 @@ export function decideRefusal(actor: OnboardingActor, proposal: ProposalView): s if (actor.userId === proposal.submittedById) { return "a proposal cannot be decided by the person who raised it" } - if (!holds(actor, proposal.institutionId, [MAY_DECIDE])) { + if (!holds(actor, proposal.institutionId, MAY_DECIDE)) { return "only the OSE Director closes an onboarding proposal" } return null } /** - * What this actor may do to this proposal, right now. + * Why this actor may NOT take this action on this proposal, or null if they may. * - * An empty array is a complete answer: it means "nothing", not "not yet - * implemented". + * The single composed check, and the only one a caller should ever gate on. + * `availableActions` is derived from it below rather than repeating the logic, + * because two expressions of one rule is how a listing comes to offer a button + * the command then refuses — or worse, the other way round. + * + * The strings are deliberately about the ACTOR's standing and the proposal's + * state, never about the proposal's SUBJECT, so a refusal cannot leak whose + * address is under consideration. */ -export function availableActions( +export function refusalFor( actor: OnboardingActor, proposal: ProposalView, -): OnboardingAction[] { - const actions: OnboardingAction[] = [] + action: OnboardingAction, +): string | null { + const status = effectiveProposalStatus(proposal.status, proposal.expiresAt, actor.evaluatedAt) + + // Terminal first, and expiry is terminal. Checking this before authority is + // what makes "the sweep has not run yet" indistinguishable from "the sweep + // ran": the clock has already decided, so a Director cannot approve a lapsed + // proposal by being quick. + if (isTerminal(status)) { + return status === "EXPIRED" + ? "this proposal expired without a decision and cannot be acted on" + : `this proposal is already ${status.toLowerCase()}` + } + + if (nextStatus(action, status) === null) { + const past = { submit: "submitted", approve: "approved", reject: "declined", withdraw: "withdrawn" } + const waiting = status === "DRAFT" ? "not yet been submitted" : "already been submitted" + return `a proposal that has ${waiting} cannot be ${past[action]}` + } + const isSubmitter = actor.userId === proposal.submittedById - switch (proposal.status) { - case "DRAFT": - // Only its author moves a draft, and only if they still hold the - // authority to propose — a staff member who has left OSE cannot submit a - // draft they wrote while they were there. - if (isSubmitter && canPropose(actor, proposal.institutionId)) actions.push("submit") - if (isSubmitter) actions.push("withdraw") - break - - case "PENDING_DIRECTOR": - if (canDecide(actor, proposal)) actions.push("approve", "reject") + switch (action) { + case "submit": + // Only its author moves a draft, and only if they still hold the authority + // to propose — a staff member who has left OSE cannot submit a draft they + // wrote while they were there. + if (!isSubmitter) return "only the person who raised a proposal may submit it" + if (!canPropose(actor, proposal.institutionId)) { + return "you no longer hold the authority to raise onboarding proposals" + } + return null + + case "withdraw": // Withdrawing is the author's, and stays available while it is pending — // an office that cannot retract a mistaken proposal will instead ask the // Director to reject it, which puts a refusal in the record of a person // who did nothing wrong. - if (isSubmitter) actions.push("withdraw") - break - - case "APPROVED": - case "REJECTED": - case "WITHDRAWN": - // Terminal. Reversing a decision is a new proposal, so the record of what - // was decided stays true. - break + return isSubmitter ? null : "only the person who raised a proposal may withdraw it" + + case "approve": + case "reject": + return decideRefusal(actor, proposal) } +} - return actions +/** + * What this actor may do to this proposal, right now. + * + * An empty array is a complete answer: it means "nothing", not "not yet + * implemented". + */ +export function availableActions( + actor: OnboardingActor, + proposal: ProposalView, +): OnboardingAction[] { + return ALL_ACTIONS.filter((a) => refusalFor(actor, proposal, a) === null) } /** @@ -198,6 +360,43 @@ export function nextStatus( } } +// ─── Who is left to decide ─────────────────────────────────────────────────── + +/** + * Why a pending proposal has nobody who can decide it, or null if somebody can. + * + * The chain is headed by the DIRECTOR ROLE, never by a named person — encoding + * a person would break the day she is on leave and would make a second tenant an + * edit to the first tenant's branch. That leaves two states a role-headed chain + * can genuinely reach, and both are answered here rather than surfacing as a + * proposal that mysteriously never moves: + * + * **The Director seat is vacant.** Nothing decides. Staff do NOT inherit the + * power — that is R2, and relaxing it under load is how a chain "headed by the + * Director" becomes a chain anyone at OSE can close. Proposals stay pending and + * expire, which is visible and reversible. Filling the seat needs + * `institution.grantRole`, which is itself Director-only, so a fully vacant seat + * is refilled by the platform operator out of band, not from inside the product. + * That is a deliberate limit and it is stated so nobody has to discover it. + * + * **The only Director is the proposer.** R3 refuses them their own proposal, so + * the remedy is delegation: a Director names a backup, and the backup — who is + * not the submitter — decides. A Director cannot delegate their way out of R3, + * because R3 reads the actor's own user id and delegation never changes it. + * + * `directorUserIds` is every user holding OSE_DIRECTOR at this institution, + * INCLUDING anyone holding it by an active delegation, because that is who can + * really act. + */ +export function chainStall( + proposal: ProposalView, + directorUserIds: readonly string[], +): "NO_DIRECTOR" | "ONLY_DIRECTOR_IS_THE_PROPOSER" | null { + if (directorUserIds.length === 0) return "NO_DIRECTOR" + const eligible = directorUserIds.filter((id) => id !== proposal.submittedById) + return eligible.length === 0 ? "ONLY_DIRECTOR_IS_THE_PROPOSER" : null +} + /** * R4 — the only state that authorises a registry write. * @@ -215,6 +414,7 @@ export const ALL_STATUSES: readonly OnboardingStatus[] = [ "APPROVED", "REJECTED", "WITHDRAWN", + "EXPIRED", ] export const ALL_ACTIONS: readonly OnboardingAction[] = ["submit", "approve", "reject", "withdraw"] diff --git a/apps/web/src/lib/identity/onboarding-proposals.test.ts b/apps/web/src/lib/identity/onboarding-proposals.test.ts new file mode 100644 index 00000000..03ad6100 --- /dev/null +++ b/apps/web/src/lib/identity/onboarding-proposals.test.ts @@ -0,0 +1,647 @@ +import type { InstitutionRole } from "@prisma/client" +import type { UserContext } from "@/lib/rbac" + +// ─── The database, as thin as it can be while still being the real query ───── + +const proposalCreate = jest.fn() +const proposalFindFirst = jest.fn() +const proposalFindFirstOrThrow = jest.fn() +const proposalFindMany = jest.fn() +const proposalUpdateMany = jest.fn() +const eventCreate = jest.fn() +const auditCreate = jest.fn() +const orgFindFirst = jest.fn() +const registryFindFirst = jest.fn() +const membershipFindMany = jest.fn() +const delegationFindMany = jest.fn() + +jest.mock("@/lib/db", () => ({ + db: { + onboardingProposal: { + create: (...a: unknown[]) => proposalCreate(...a), + findFirst: (...a: unknown[]) => proposalFindFirst(...a), + findFirstOrThrow: (...a: unknown[]) => proposalFindFirstOrThrow(...a), + findMany: (...a: unknown[]) => proposalFindMany(...a), + updateMany: (...a: unknown[]) => proposalUpdateMany(...a), + }, + onboardingProposalEvent: { create: (...a: unknown[]) => eventCreate(...a), findMany: jest.fn() }, + auditEvent: { create: (...a: unknown[]) => auditCreate(...a) }, + organization: { findFirst: (...a: unknown[]) => orgFindFirst(...a) }, + restrictedIdentity: { findFirst: (...a: unknown[]) => registryFindFirst(...a) }, + institutionMembership: { findMany: (...a: unknown[]) => membershipFindMany(...a) }, + approvalDelegation: { findMany: (...a: unknown[]) => delegationFindMany(...a) }, + }, +})) + +const getUserContext = jest.fn() +jest.mock("@/lib/rbac", () => ({ getUserContext: (...a: unknown[]) => getUserContext(...a) })) + +import { Refusal } from "@/lib/admin/action-state" +import { + actOnProposal, + createProposal, + decidersFor, + registryGrantFor, + sweepExpiredProposals, + type Proposal, + type RegistryGrant, +} from "./onboarding-proposals" + +/** + * The composition, not the rules. + * + * `onboarding-chain.test.ts` proves the rules and `onboarding-actor.test.ts` + * proves delegation cannot defeat R3. Neither can see whether this module + * ACTUALLY ASKS. That distinction is not academic here: the last access control + * this codebase shipped was correct in its pure function and broken in the + * query that fed it, and every unit test passed while the gate refused nobody. + * + * So these drive the real command path — real chain, real actor resolution, + * real compare-and-swap — with only the database mocked. + */ + +const INST = "inst_simon" +const OTHER = "inst_other" +const NOW = new Date("2026-08-21T12:00:00.000Z") +const FAR = new Date("2027-01-01T00:00:00.000Z") + +const ctxFor = (userId: string, roles: { institutionId: string; role: InstitutionRole }[]): UserContext => ({ + userId, + evaluatedAt: NOW, + institutionRoles: roles, + orgRoles: [], +}) + +const STAFF = ctxFor("staff_1", [{ institutionId: INST, role: "OSE_STAFF" }]) +const STAFF_2 = ctxFor("staff_2", [{ institutionId: INST, role: "OSE_STAFF" }]) +const DIRECTOR = ctxFor("dir_1", [{ institutionId: INST, role: "OSE_DIRECTOR" }]) +const ADVISOR = ctxFor("adv_1", [{ institutionId: INST, role: "OSE_ADVISOR" }]) + +const row = (over: Record = {}) => ({ + id: "prop_1", + institutionId: INST, + subjectName: "A New Officer", + subjectEmail: "New.Officer@simon.rochester.edu", + subjectEmailNormalized: "new.officer@simon.rochester.edu", + subjectKind: "MEMBER", + subjectKindOther: null, + cohort: "STUDENT_LEADER", + organizationId: null, + submittedById: "staff_1", + status: "PENDING_DIRECTOR", + expiresAt: FAR, + submittedAt: NOW, + decidedAt: null, + decidedById: null, + decisionReason: null, + ...over, +}) + +const validInput = { + institutionId: INST, + subjectName: "A New Officer", + subjectEmail: "New.Officer@simon.rochester.edu", + subjectKind: "MEMBER" as const, + cohort: "STUDENT_LEADER", +} + +beforeEach(() => { + for (const m of [ + proposalCreate, proposalFindFirst, proposalFindFirstOrThrow, proposalFindMany, + proposalUpdateMany, eventCreate, auditCreate, orgFindFirst, registryFindFirst, + membershipFindMany, delegationFindMany, getUserContext, + ]) m.mockReset() + + orgFindFirst.mockResolvedValue(null) + registryFindFirst.mockResolvedValue(null) + delegationFindMany.mockResolvedValue([]) + membershipFindMany.mockResolvedValue([]) + eventCreate.mockResolvedValue({}) + auditCreate.mockResolvedValue({}) + proposalUpdateMany.mockResolvedValue({ count: 1 }) +}) + +/** Every audit row this call wrote. */ +const auditRows = () => auditCreate.mock.calls.map((c) => (c[0] as { data: Record }).data) + +// ─── Negative control 1: an OSE admin cannot propose into another institution ─ + +describe("the institution boundary", () => { + it("REFUSES an OSE admin proposing someone into another institution", async () => { + // Simon's staff, aimed at another tenant. This is the control that stops one + // institution's officer admitting a person to another institution's + // access boundary. + await expect(createProposal(STAFF, { ...validInput, institutionId: OTHER })).rejects.toThrow( + Refusal, + ) + expect(proposalCreate).not.toHaveBeenCalled() + // The refusal is logged where the ACTOR is accountable, not in the targeted + // institution's log. Auditing the target would have the cross-tenant check + // perform a cross-tenant write — and `AuditEvent.institutionId` is a real + // foreign key, so a target that does not exist would be a raw P2003 in place + // of a refusal that had already been decided. + expect(auditRows()).toEqual([ + expect.objectContaining({ + institutionId: INST, + outcome: "DENY", + action: "Onboarding.create", + metadata: { targetInstitutionId: OTHER }, + }), + ]) + }) + + it("refuses, and writes NO audit row, for an actor who belongs to no institution", async () => { + // There is no tenant that owns that event. Inventing one is worse than the + // gap; being refused is the part that has to be true. + const stranger = ctxFor("nobody", []) + await expect(createProposal(stranger, validInput)).rejects.toThrow(Refusal) + expect(auditCreate).not.toHaveBeenCalled() + expect(proposalCreate).not.toHaveBeenCalled() + }) + + it("logs an ordinary in-tenant refusal against that tenant, with no target noted", async () => { + await expect(createProposal(ADVISOR, validInput)).rejects.toThrow(Refusal) + const rows = auditRows() + expect(rows).toHaveLength(1) + expect(rows[0]).toMatchObject({ institutionId: INST, outcome: "DENY" }) + expect(rows[0].metadata).toBeUndefined() + }) + + it("REFUSES a Director of another institution just as firmly", async () => { + const foreignDirector = ctxFor("dir_x", [{ institutionId: OTHER, role: "OSE_DIRECTOR" }]) + await expect(createProposal(foreignDirector, validInput)).rejects.toThrow(Refusal) + expect(proposalCreate).not.toHaveBeenCalled() + }) + + it("refuses a club belonging to a different institution", async () => { + orgFindFirst.mockResolvedValue(null) // the composite key would refuse it too + await expect( + createProposal(STAFF, { ...validInput, organizationId: "org_elsewhere" }), + ).rejects.toThrow(/does not belong to this institution/) + expect(proposalCreate).not.toHaveBeenCalled() + }) + + it("never reads or writes without naming the institution", async () => { + proposalCreate.mockResolvedValue(row({ status: "DRAFT" })) + await createProposal(STAFF, validInput) + expect(registryFindFirst).toHaveBeenCalledWith( + expect.objectContaining({ where: expect.objectContaining({ institutionId: INST }) }), + ) + expect(proposalCreate.mock.calls[0][0].data.institutionId).toBe(INST) + }) +}) + +// ─── Negative control 2: R1 ────────────────────────────────────────────────── + +describe("R1 — who may raise a proposal", () => { + it("REFUSES an advisor", async () => { + await expect(createProposal(ADVISOR, validInput)).rejects.toThrow(Refusal) + expect(proposalCreate).not.toHaveBeenCalled() + }) + + it("admits staff, and records the creation twice", async () => { + proposalCreate.mockResolvedValue(row({ status: "DRAFT" })) + const p = await createProposal(STAFF, validInput) + + expect(p.status).toBe("DRAFT") + expect(eventCreate).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ kind: "CREATED", actorId: "staff_1", institutionId: INST }), + }), + ) + expect(auditRows()).toEqual([expect.objectContaining({ outcome: "ALLOW" })]) + }) + + it("normalises the address the same way the access boundary does", async () => { + proposalCreate.mockResolvedValue(row({ status: "DRAFT" })) + await createProposal(STAFF, validInput) + const data = proposalCreate.mock.calls[0][0].data + expect(data.subjectEmailNormalized).toBe("new.officer@simon.rochester.edu") + expect(data.subjectEmail).toBe("New.Officer@simon.rochester.edu") + // Held open against a second proposal for the same person. + expect(data.openSubjectKey).toBe("new.officer@simon.rochester.edu") + }) + + it("refuses a person who is already on the access registry", async () => { + // Admitting somebody twice is a second admission and, once seats are + // charged, a second charge. + registryFindFirst.mockResolvedValue({ id: "ri_1" }) + await expect(createProposal(STAFF, validInput)).rejects.toThrow(/already on the access registry/) + }) + + it("refuses OTHER without saying what, and a description without OTHER", async () => { + await expect( + createProposal(STAFF, { ...validInput, subjectKind: "OTHER" }), + ).rejects.toThrow(/saying what/) + await expect( + createProposal(STAFF, { ...validInput, subjectKindOther: "a visitor" }), + ).rejects.toThrow(/only belongs on an 'other' proposal/) + }) + + it("turns a duplicate open proposal into a sentence rather than a stack trace", async () => { + proposalCreate.mockRejectedValue(Object.assign(new Error("unique"), { code: "P2002" })) + await expect(createProposal(STAFF, validInput)).rejects.toThrow(/already an open proposal/) + }) +}) + +// ─── Negative controls 3 and 4: R2, R3, and R3 under delegation ────────────── + +describe("R2 and R3 on the real command path", () => { + beforeEach(() => { + proposalFindFirst.mockResolvedValue(row()) + proposalFindFirstOrThrow.mockResolvedValue(row({ status: "APPROVED", decidedAt: NOW, decidedById: "dir_1" })) + }) + + it("REFUSES a proposer approving their own proposal", async () => { + await expect(actOnProposal(STAFF, INST, "prop_1", "approve")).rejects.toThrow( + /cannot be decided by the person who raised it/i, + ) + expect(proposalUpdateMany).not.toHaveBeenCalled() + expect(eventCreate).not.toHaveBeenCalled() + expect(auditRows()).toEqual([ + expect.objectContaining({ outcome: "DENY", action: "Onboarding.approve" }), + ]) + }) + + it("REFUSES a proposer who holds a DIRECTOR DELEGATION approving their own proposal", async () => { + // The attack, driven through the command rather than the rule: a real + // delegation row, the real resolver, the real merge, the real write path. + // Nothing about this is a fixture except the rows. + delegationFindMany.mockResolvedValue([ + { fromUserId: "dir_1", fromUser: { id: "dir_1", name: "The Director", email: null } }, + ]) + getUserContext.mockResolvedValue(DIRECTOR) + + await expect(actOnProposal(STAFF, INST, "prop_1", "approve")).rejects.toThrow( + /cannot be decided by the person who raised it/i, + ) + // The delegation really was resolved — this refusal is not the mock failing. + expect(delegationFindMany).toHaveBeenCalled() + expect(getUserContext).toHaveBeenCalledWith("dir_1") + expect(proposalUpdateMany).not.toHaveBeenCalled() + }) + + it("REFUSES a non-Director — staff and advisor alike", async () => { + for (const who of [STAFF_2, ADVISOR]) { + proposalUpdateMany.mockClear() + await expect(actOnProposal(who, INST, "prop_1", "approve")).rejects.toThrow( + /only the OSE Director/i, + ) + expect(proposalUpdateMany).not.toHaveBeenCalled() + } + }) + + it("admits the Director, and swaps on the status the decision was made against", async () => { + const p = await actOnProposal(DIRECTOR, INST, "prop_1", "approve", "Checked with the department.") + + expect(proposalUpdateMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: { id: "prop_1", institutionId: INST, status: "PENDING_DIRECTOR" }, + }), + ) + const data = proposalUpdateMany.mock.calls[0][0].data + expect(data.status).toBe("APPROVED") + expect(data.decidedById).toBe("dir_1") + // Terminal releases the open-proposal slot. + expect(data.openSubjectKey).toBeNull() + expect(p.effectiveStatus).toBe("APPROVED") + }) + + it("loses the race rather than deciding twice", async () => { + // Two Directors, same proposal, same second. The second swap matches no row. + proposalUpdateMany.mockResolvedValue({ count: 0 }) + await expect(actOnProposal(DIRECTOR, INST, "prop_1", "approve")).rejects.toThrow( + /Somebody else acted on this proposal first/, + ) + expect(eventCreate).not.toHaveBeenCalled() + }) + + it("records a borrowed decision as borrowed", async () => { + delegationFindMany.mockResolvedValue([ + { fromUserId: "dir_1", fromUser: { id: "dir_1", name: "The Director", email: null } }, + ]) + getUserContext.mockResolvedValue(DIRECTOR) + await actOnProposal(STAFF_2, INST, "prop_1", "approve") + + expect(eventCreate).toHaveBeenCalledWith( + expect.objectContaining({ data: expect.objectContaining({ onBehalfOfId: "dir_1" }) }), + ) + }) + + it("refuses a proposal at another institution without saying it exists", async () => { + proposalFindFirst.mockResolvedValue(null) + await expect(actOnProposal(DIRECTOR, OTHER, "prop_1", "approve")).rejects.toThrow( + /could not be found/, + ) + expect(proposalFindFirst).toHaveBeenCalledWith( + expect.objectContaining({ where: { id: "prop_1", institutionId: OTHER } }), + ) + }) +}) + +// ─── Negative control 5: a declined proposal grants nothing ────────────────── + +describe("a declined proposal grants nothing", () => { + const decided = (status: string): Proposal => ({ + ...(row({ status, decidedAt: NOW, decidedById: "dir_1", decisionReason: "Not this term." }) as unknown as Proposal), + effectiveStatus: status as Proposal["effectiveStatus"], + }) + + it("produces no registry grant when REJECTED", () => { + expect(registryGrantFor(decided("REJECTED"))).toBeNull() + }) + + it("produces no registry grant in ANY status but APPROVED", () => { + for (const s of ["DRAFT", "PENDING_DIRECTOR", "REJECTED", "WITHDRAWN", "EXPIRED"]) { + expect(registryGrantFor(decided(s))).toBeNull() + } + const grant = registryGrantFor(decided("APPROVED")) + expect(grant).not.toBeNull() + expect(grant).toMatchObject({ + institutionId: INST, + emailNormalized: "new.officer@simon.rochester.edu", + cohort: "STUDENT_LEADER", + addedBy: "dir_1", + sourceVersion: "prop_1", + addedVia: "onboarding-proposal", + }) + }) + + it("cannot be assembled by hand — the seam is a mint, not a shape", () => { + // A COMPILE-TIME control, and `npx tsc --noEmit` is the thing that runs it: + // @ts-expect-error fails the build if this line stops being an error, which + // is exactly what removing the brand would do. Without it the claim above — + // "nothing else can produce one" — was false, because TypeScript is + // structural: the downstream write could have written this literal and + // admitted a person with no proposal, no Director and no record. + const forged: RegistryGrant = { + institutionId: INST, + emailNormalized: "new.officer@simon.rochester.edu", + displayName: "A New Officer", + cohort: "STUDENT_LEADER", + organizationId: null, + addedBy: "staff_1", + sourceVersion: "prop_1", + addedVia: "onboarding-proposal", + decidedAt: NOW, + } + expect(forged.addedBy).toBe("staff_1") + }) + + it("refuses to grant on an APPROVED row with no decider — an impossible row, refused anyway", () => { + const forged = { ...decided("APPROVED"), decidedById: null } as Proposal + expect(registryGrantFor(forged)).toBeNull() + }) + + it("a rejected proposal cannot then be approved", async () => { + proposalFindFirst.mockResolvedValue(row({ status: "REJECTED" })) + await expect(actOnProposal(DIRECTOR, INST, "prop_1", "approve")).rejects.toThrow( + /already rejected/, + ) + expect(proposalUpdateMany).not.toHaveBeenCalled() + }) +}) + +// ─── Expiry, and the audit payload ─────────────────────────────────────────── + +describe("expiry", () => { + it("refuses a decision on a lapsed proposal even though the sweep never ran", async () => { + // The row still says PENDING_DIRECTOR in the database. The clock disagrees, + // and the clock wins. + proposalFindFirst.mockResolvedValue(row({ expiresAt: new Date(NOW.getTime() - 1) })) + await expect(actOnProposal(DIRECTOR, INST, "prop_1", "approve")).rejects.toThrow( + /expired without a decision/, + ) + expect(proposalUpdateMany).not.toHaveBeenCalled() + }) + + it("the sweep records what the clock decided, with no actor", async () => { + proposalFindMany.mockResolvedValue([row({ expiresAt: new Date(NOW.getTime() - 1) })]) + const { expired } = await sweepExpiredProposals(INST, NOW) + + expect(expired).toEqual(["prop_1"]) + expect(proposalUpdateMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: { id: "prop_1", institutionId: INST, status: "PENDING_DIRECTOR" }, + data: { status: "EXPIRED", openSubjectKey: null }, + }), + ) + // Nobody expired it. The event says so rather than blaming whoever ran the job. + expect(eventCreate.mock.calls[0][0].data).toMatchObject({ kind: "EXPIRED", actorId: null }) + }) + + it("never overwrites a decision a person reached in the same instant", async () => { + proposalFindMany.mockResolvedValue([row({ expiresAt: new Date(NOW.getTime() - 1) })]) + proposalUpdateMany.mockResolvedValue({ count: 0 }) + const { expired } = await sweepExpiredProposals(INST, NOW) + expect(expired).toEqual([]) + expect(eventCreate).not.toHaveBeenCalled() + }) +}) + +describe("the audit row never carries the person under consideration", () => { + it("holds the proposal id and the outcome, and no payload", async () => { + // `audit.view` is minRole OSE_ADVISOR and the audit page prints event + // metadata. A proposal's payload is a named person's address and cohort. + proposalFindFirst.mockResolvedValue(row()) + proposalFindFirstOrThrow.mockResolvedValue(row({ status: "APPROVED", decidedAt: NOW, decidedById: "dir_1" })) + await actOnProposal(DIRECTOR, INST, "prop_1", "approve", "Spoke to the department about Jane.") + + proposalCreate.mockResolvedValue(row({ status: "DRAFT" })) + await createProposal(STAFF, validInput) + + // CASE-INSENSITIVE, and that is not a detail. This assertion listed the + // NORMALISED address only, while the row carries `subjectEmail` as typed — + // "New.Officer@…". Writing the as-typed address (the display field, so the + // likelier mistake of the two) into an audit reason passed all 85 tests. A + // control that misses the realistic regression is not a control. + const serialised = JSON.stringify(auditRows()).toLowerCase() + for (const secret of [ + validInput.subjectEmail, + row().subjectEmail, + row().subjectEmailNormalized, + validInput.subjectName, + validInput.cohort, + "Jane", // the operator's own words go on the proposal's record, not here + ]) { + expect(serialised).not.toContain(secret.toLowerCase()) + } + + // And a SHAPE assertion, because a denylist only ever catches the values it + // was told about. A new field carrying the subject under any other value + // fails here instead of shipping. + const permitted = new Set([ + "institutionId", + "actorId", + "actorRole", + "action", + "resourceType", + "resourceId", + "organizationId", + "outcome", + "reason", + "metadata", + ]) + for (const r of auditRows()) { + expect(Object.keys(r).filter((k) => !permitted.has(k))).toEqual([]) + } + expect(auditRows()[0]).toMatchObject({ resourceType: "OnboardingProposal", resourceId: "prop_1" }) + }) +}) + +describe("who is left to decide", () => { + it("reports a vacant Director seat rather than letting a proposal sit", async () => { + membershipFindMany.mockResolvedValue([]) + const answer = await decidersFor(STAFF, row() as never) + expect(answer).not.toBeNull() + expect(answer!.userIds).toEqual([]) + expect(answer!.stall).toBe("NO_DIRECTOR") + }) + + it("counts a delegate as somebody who can act", async () => { + membershipFindMany.mockResolvedValue([{ userId: "dir_1" }]) + delegationFindMany.mockResolvedValue([{ toUserId: "backup_1" }]) + const answer = await decidersFor(STAFF, row({ submittedById: "dir_1" }) as never) + expect(answer!.userIds.sort()).toEqual(["backup_1", "dir_1"]) + expect(answer!.stall).toBeNull() + }) + + it("says so when the only Director is the proposer", async () => { + membershipFindMany.mockResolvedValue([{ userId: "dir_1" }]) + const answer = await decidersFor(STAFF, row({ submittedById: "dir_1" }) as never) + expect(answer!.stall).toBe("ONLY_DIRECTOR_IS_THE_PROPOSER") + }) + + it("REFUSES to enumerate another institution's Directors", async () => { + // The fabricated view is the attack: nothing about a `ProposalView` proves + // it came from a row this caller may read, so the capability is asked at the + // institution the view names. + membershipFindMany.mockResolvedValue([{ userId: "dir_x" }]) + const foreign = row({ institutionId: OTHER }) as never + expect(await decidersFor(STAFF, foreign)).toBeNull() + expect(await decidersFor(DIRECTOR, foreign)).toBeNull() + expect(membershipFindMany).not.toHaveBeenCalled() + }) + + it("REFUSES an advisor, who may not see a proposal either", async () => { + expect(await decidersFor(ADVISOR, row() as never)).toBeNull() + expect(membershipFindMany).not.toHaveBeenCalled() + }) +}) + +describe("the domain is deliberately not a gate", () => { + it("accepts a University address that is not the tenant's eligible domain", async () => { + // The Identity Continuity specification excludes one advisor from the July + // roster for holding only `@ur.rochester.edu`. This path is how that person + // would be admitted if OSE decided to, so refusing the address here would + // refuse the exact case the specification raises. R2 is the control: the + // Director reads the address and decides. + proposalCreate.mockResolvedValue(row({ status: "DRAFT" })) + await createProposal(STAFF, { + ...validInput, + subjectEmail: "An.Advisor@ur.rochester.edu", + subjectKind: "ADVISOR", + cohort: "ADVISOR", + }) + expect(proposalCreate.mock.calls[0][0].data.subjectEmailNormalized).toBe( + "an.advisor@ur.rochester.edu", + ) + }) + + it("still refuses something that is not an address at all", async () => { + await expect( + createProposal(STAFF, { ...validInput, subjectEmail: "not-an-address" }), + ).rejects.toThrow(/valid institutional email/) + expect(proposalCreate).not.toHaveBeenCalled() + }) + + it("refuses the three strings the old `includes(\"@\")` check admitted", async () => { + // Each was accepted, and each would have reached `RestrictedIdentity` as a + // row no sign-in could ever match — and, once seats are charged, as a + // person-shaped row that is not a person. + for (const junk of ["@", "@simon.rochester.edu", "two words@simon.rochester.edu"]) { + await expect( + createProposal(STAFF, { ...validInput, subjectEmail: junk }), + ).rejects.toThrow(/valid institutional email/) + } + expect(proposalCreate).not.toHaveBeenCalled() + }) +}) + +// ─── Negative control 6: a refusal is logged where the ACTOR is accountable ── + +describe("a refusal never writes into the institution it was refused from", () => { + it("an unknown proposal at another institution is logged against the actor's tenant", async () => { + // Reproduced against a real database first: this wrote a DENY row into the + // NAMED institution's audit log, and raised a raw P2003 on + // `AuditEvent_institutionId_fkey` when the named institution did not exist. + // `institutionId` arrives from the caller and nothing above this had + // established it was a tenant at all. + proposalFindFirst.mockResolvedValue(null) + await expect(actOnProposal(DIRECTOR, OTHER, "prop_1", "approve")).rejects.toThrow( + /could not be found/, + ) + expect(auditRows()).toEqual([ + expect.objectContaining({ + institutionId: INST, + outcome: "DENY", + action: "Onboarding.approve", + metadata: { targetInstitutionId: OTHER }, + }), + ]) + }) + + it("an outsider refused on a proposal that DOES exist is logged the same way", async () => { + const outsider = ctxFor("dir_x", [{ institutionId: OTHER, role: "OSE_DIRECTOR" }]) + proposalFindFirst.mockResolvedValue(row()) + await expect(actOnProposal(outsider, INST, "prop_1", "approve")).rejects.toThrow(Refusal) + const rows = auditRows() + expect(rows).toHaveLength(1) + expect(rows[0]).toMatchObject({ institutionId: OTHER, metadata: { targetInstitutionId: INST } }) + // and this tenant's club id does not travel into the other tenant's log + expect(rows[0].organizationId).toBeUndefined() + }) + + it("tells an outsider the SAME thing whether or not the proposal exists", async () => { + // `getProposal` states the rule and this path broke it: a real id answered + // "only the OSE Director closes an onboarding proposal" while a made-up one + // answered "could not be found" — an existence oracle across the boundary. + const outsider = ctxFor("dir_x", [{ institutionId: OTHER, role: "OSE_DIRECTOR" }]) + + proposalFindFirst.mockResolvedValue(row()) + const real = await actOnProposal(outsider, INST, "prop_1", "approve").catch((e) => e.message) + + proposalFindFirst.mockResolvedValue(null) + const imaginary = await actOnProposal(outsider, INST, "nope", "approve").catch((e) => e.message) + + expect(real).toBe(imaginary) + expect(real).toMatch(/could not be found/) + }) + + it("but an AUTHOR who has left OSE entirely may still withdraw their own", async () => { + // Membership OR authorship. Masking on membership alone would take + // withdrawal away from the one person `refusalFor` grants it to. + const departed = ctxFor("staff_1", []) + proposalFindFirst.mockResolvedValue(row()) + proposalFindFirstOrThrow.mockResolvedValue(row({ status: "WITHDRAWN" })) + const out = await actOnProposal(departed, INST, "prop_1", "withdraw") + expect(out.status).toBe("WITHDRAWN") + }) + + it("an IN-TENANT refusal still lands in that tenant's log, unchanged", async () => { + proposalFindFirst.mockResolvedValue(row({ submittedById: "dir_1" })) + await expect(actOnProposal(DIRECTOR, INST, "prop_1", "approve")).rejects.toThrow(Refusal) + const rows = auditRows() + expect(rows).toHaveLength(1) + expect(rows[0]).toMatchObject({ institutionId: INST, outcome: "DENY" }) + expect(rows[0].metadata).toBeUndefined() + }) + + it("an actor with no institution at all gets no row, and is still refused", async () => { + proposalFindFirst.mockResolvedValue(null) + await expect(actOnProposal(ctxFor("nobody", []), INST, "prop_1", "approve")).rejects.toThrow( + Refusal, + ) + expect(auditCreate).not.toHaveBeenCalled() + }) +}) diff --git a/apps/web/src/lib/identity/onboarding-proposals.ts b/apps/web/src/lib/identity/onboarding-proposals.ts new file mode 100644 index 00000000..40e05f6f --- /dev/null +++ b/apps/web/src/lib/identity/onboarding-proposals.ts @@ -0,0 +1,728 @@ +import type { OnboardingEventKind, OnboardingSubjectKind, Prisma } from "@prisma/client" +import { Refusal } from "@/lib/admin/action-state" +import { adminRoleAt, hasCapability } from "@/lib/admin/capabilities" +import { isAddressShaped, normalizeEmail } from "@/lib/auth/eligibility" +import { db } from "@/lib/db" +import type { UserContext } from "@/lib/rbac" +import { effectiveOnboardingActor, toOnboardingActor } from "./onboarding-actor" +import { + ALL_STATUSES, + chainStall, + effectiveProposalStatus, + expiryFor, + expirySweepTarget, + grantsRegistryEntry, + isTerminal, + refusalFor, + availableActions, + type OnboardingAction, + type OnboardingStatus, + type ProposalView, +} from "./onboarding-chain" + +/** + * The store the onboarding chain runs on, and the only way a proposal changes. + * + * ── What this module is, and what it is not ───────────────────────────────── + * + * `onboarding-chain.ts` decides. This executes: it loads a proposal, asks the + * chain, writes the transition, and records it twice — once in the proposal's + * own append-only log and once in `AuditEvent`. It builds no UI and performs no + * downstream effect. `registryGrantFor` is the seam both of those live behind. + * + * ── Why every command re-checks authority ─────────────────────────────────── + * + * Server actions gate on `requireCapability`, which is the right outer door and + * cannot express R3 — "not the person who raised it" is a fact about a row, and + * a capability is a fact about a role. So the chain is asked again here, against + * the same capability catalog, at the moment of the write. That is not a second + * authorization path: `MAY_PROPOSE` and `MAY_DECIDE` are DERIVED from + * `onboarding.propose` and `onboarding.decide`. It is the same catalog, asked + * where the row is in hand. + * + * ── Why the writes are compare-and-swap ───────────────────────────────────── + * + * Two Directors on the same proposal in the same second must produce one + * decision, not two. Every transition updates `WHERE id AND institutionId AND + * status = `, and a count of 0 means + * somebody else moved it first — reported as a refusal, not silently retried. + * + * ── What never reaches an audit row ───────────────────────────────────────── + * + * `audit.view` is minRole OSE_ADVISOR and the audit page prints event metadata, + * so a proposal's payload — a named person's address and cohort — would be + * readable by every advisor at the institution if it were recorded there the + * usual way. Audit rows here carry the proposal id, the action and the outcome, + * and nothing about the subject. The subject lives on the proposal, behind the + * permission the proposal has. + */ + +// ─── The shapes callers use ────────────────────────────────────────────────── + +/** Everything a proposal is, as this module hands it out. */ +export interface Proposal extends ProposalView { + readonly id: string + readonly subjectName: string + readonly subjectEmail: string + readonly subjectEmailNormalized: string + readonly subjectKind: OnboardingSubjectKind + readonly subjectKindOther: string | null + readonly cohort: string + readonly organizationId: string | null + readonly submittedAt: Date | null + readonly decidedAt: Date | null + readonly decidedById: string | null + readonly decisionReason: string | null + /** What the row says, capped by the clock. Never below `status`. */ + readonly effectiveStatus: OnboardingStatus +} + +export interface CreateProposalInput { + institutionId: string + subjectName: string + subjectEmail: string + subjectKind: OnboardingSubjectKind + /** Required when `subjectKind` is OTHER, refused otherwise. */ + subjectKindOther?: string | null + cohort: string + /** Null for an institution-level person — an advisor with no club. */ + organizationId?: string | null +} + +/** + * What an APPROVED proposal authorises — R4's payload, and the whole of this + * module's contract with whatever performs the downstream effect. + * + * Deliberately a plain descriptor rather than a write. This module decides + * WHETHER a person may be admitted; what admission then creates — + * `RestrictedIdentity` alone, or a `DirectoryPerson`, or a `User` — is the + * question ADR-0009 already owns and leaves open. Returning a value keeps that + * answerable later without reopening this one. + * + * The field names line up with the provenance columns the access registry + * carries: an admitted row can say which decision put it there, who decided, + * and by what mechanism. + */ +declare const registryGrantBrand: unique symbol + +export interface RegistryGrant { + /** + * Minted by `registryGrantFor` and by nothing else. + * + * The brand is the point, and it follows `AccountingEventKey` in + * `lib/accounting-events.ts` exactly. Without it the sentence below — "a + * caller that holds a `RegistryGrant` is holding proof that a Director + * approved this admission" — was FALSE: TypeScript is structural, so the + * downstream write could have assembled the object literal itself and + * admitted a person with no proposal, no Director and no record, while still + * type-checking against the seam that was supposed to prevent exactly that. + * It costs one cast, in one function, here. + */ + readonly [registryGrantBrand]: true + readonly institutionId: string + readonly emailNormalized: string + readonly displayName: string + readonly cohort: string + readonly organizationId: string | null + /** The deciding Director's user id. */ + readonly addedBy: string + /** The proposal id — the decision this admission traces to. */ + readonly sourceVersion: string + readonly addedVia: "onboarding-proposal" + readonly decidedAt: Date +} + +// ─── Reading ───────────────────────────────────────────────────────────────── + +const SELECT = { + id: true, + institutionId: true, + subjectName: true, + subjectEmail: true, + subjectEmailNormalized: true, + subjectKind: true, + subjectKindOther: true, + cohort: true, + organizationId: true, + submittedById: true, + status: true, + expiresAt: true, + submittedAt: true, + decidedAt: true, + decidedById: true, + decisionReason: true, +} satisfies Prisma.OnboardingProposalSelect + +type Row = Prisma.OnboardingProposalGetPayload<{ select: typeof SELECT }> + +function toProposal(row: Row, at: Date): Proposal { + return { + ...row, + status: row.status as OnboardingStatus, + effectiveStatus: effectiveProposalStatus( + row.status as OnboardingStatus, + row.expiresAt, + at, + ), + } +} + +/** + * One proposal, scoped to an institution the caller actually administers. + * + * Returns null rather than throwing for "not found" and for "not yours", which + * are deliberately the same answer: distinguishing them tells an admin at one + * institution that a proposal exists at another. + */ +export async function getProposal( + ctx: UserContext, + institutionId: string, + proposalId: string, +): Promise { + if (!hasCapability(ctx, "onboarding.propose", institutionId)) return null + const row = await db.onboardingProposal.findFirst({ + where: { id: proposalId, institutionId }, + select: SELECT, + }) + return row ? toProposal(row, ctx.evaluatedAt) : null +} + +/** + * Proposals at one institution, newest first. + * + * `status` filters on the STORED value; every row also carries + * `effectiveStatus`, so a caller listing "pending" gets rows the clock has + * already expired and can say so rather than offering a decision that will be + * refused. + */ +export async function listProposals( + ctx: UserContext, + institutionId: string, + filter: { status?: readonly OnboardingStatus[] } = {}, +): Promise { + if (!hasCapability(ctx, "onboarding.propose", institutionId)) return [] + const rows = await db.onboardingProposal.findMany({ + where: { + institutionId, + ...(filter.status ? { status: { in: filter.status as OnboardingStatus[] } } : {}), + }, + select: SELECT, + orderBy: [{ createdAt: "desc" }], + }) + return rows.map((r) => toProposal(r, ctx.evaluatedAt)) +} + +/** The proposal's own append-only record, oldest first. */ +export async function proposalHistory( + ctx: UserContext, + institutionId: string, + proposalId: string, +) { + if (!hasCapability(ctx, "onboarding.propose", institutionId)) return [] + return db.onboardingProposalEvent.findMany({ + where: { proposalId, institutionId }, + orderBy: [{ occurredAt: "asc" }], + }) +} + +/** + * Who could decide this proposal, and — if nobody can — why not. + * + * Counts Directors by direct membership AND by active delegation, because a + * delegate is who can really act. Surfaced so a stalled chain is visible in the + * console rather than discovered when the proposal expires. + * + * Gated like every other reader here, and it was not: it took a `ProposalView` + * with no context at all, so any caller could hand it a fabricated view naming + * another institution and be told that institution's Director user ids — an + * enumeration primitive, in the module whose subject is who may reach across a + * tenant boundary. `getProposal`, `listProposals` and `proposalHistory` all ask + * `onboarding.propose` at the named institution; this now asks the same thing. + * + * Null rather than an empty list, for `getProposal`'s reason: "there is nobody + * to decide" and "you may not ask" are different facts, and answering the second + * with the first tells an outsider that a Director seat is vacant. + */ +export async function decidersFor( + ctx: UserContext, + proposal: ProposalView, +): Promise<{ userIds: string[]; stall: ReturnType } | null> { + if (!hasCapability(ctx, "onboarding.propose", proposal.institutionId)) return null + const direct = await db.institutionMembership.findMany({ + where: { institutionId: proposal.institutionId, role: "OSE_DIRECTOR" }, + select: { userId: true }, + }) + const delegated = await db.approvalDelegation.findMany({ + where: { + institutionId: proposal.institutionId, + revokedAt: null, + fromUserId: { in: direct.map((d) => d.userId) }, + }, + select: { toUserId: true }, + }) + const userIds = [...new Set([...direct.map((d) => d.userId), ...delegated.map((d) => d.toUserId)])] + return { userIds, stall: chainStall(proposal, userIds) } +} + +// ─── Writing ───────────────────────────────────────────────────────────────── + +async function audit( + institutionId: string, + actorId: string | null, + ctx: UserContext | null, + action: string, + proposalId: string | null, + outcome: "ALLOW" | "DENY", + reason: string | null, + organizationId?: string | null, +) { + await db.auditEvent.create({ + data: { + institutionId, + actorId: actorId ?? undefined, + actorRole: (ctx ? adminRoleAt(ctx, institutionId) : null) ?? undefined, + action: `Onboarding.${action}`, + resourceType: "OnboardingProposal", + resourceId: proposalId ?? undefined, + organizationId: organizationId ?? undefined, + outcome, + // The control's own words only. Never the subject: see the note at the + // top of this file about `audit.view` being minRole OSE_ADVISOR. + reason: reason ?? undefined, + }, + }) +} + +/** + * Raise a proposal. R1, plus the institution boundary and the shape rules the + * database cannot express. + */ +export async function createProposal( + ctx: UserContext, + input: CreateProposalInput, +): Promise { + const { institutionId } = input + const actor = toOnboardingActor(ctx) + + // R1, and the tenant boundary in the same breath: `canPropose` asks for the + // role AT THIS INSTITUTION, so being staff somewhere else is being nobody + // here. This is the control that stops an OSE admin at one institution + // admitting a person into another. + if (!hasCapability(ctx, "onboarding.propose", institutionId)) { + await auditRefusal(ctx, institutionId, "create", "not permitted to raise onboarding proposals") + throw new Refusal("You do not have permission to raise onboarding proposals here.") + } + + const subjectName = input.subjectName.trim() + const subjectEmail = input.subjectEmail.trim() + const normalized = normalizeEmail(subjectEmail) + if (!subjectName) throw new Refusal("A proposal needs the person's name.") + // `includes("@")` was the check, and it admitted "@", "@simon.rochester.edu" + // and "two words@simon.rochester.edu" — three strings that reach the access + // boundary as a row no sign-in can ever match. `isAddressShaped` is the same + // one opinion about what an address is that `normalizeEmail` is, and it is + // NOT a domain gate: see the note below about why the domain is not checked. + if (!isAddressShaped(subjectEmail)) { + throw new Refusal("A proposal needs a valid institutional email address.") + } + + // NOT checked: that the address is at the tenant's eligible domain. + // + // Deliberate, and worth stating because it looks like an omission. The + // Identity Continuity specification excludes one advisor from the July roster + // for holding only a `@ur.rochester.edu` address — a valid University identity + // that is not a Simon one. This path is precisely how that person would be + // admitted if OSE decided to admit them, so a domain gate here would refuse + // the exact case the specification raises. `eligibility.ts` is also explicit + // that the domain is never sufficient and never the boundary: the roster is. + // + // What stops a stranger's address reaching the registry is not a regex, it is + // R2 — the Director reads the name and the address and decides. + const cohort = input.cohort.trim() + if (!cohort) throw new Refusal("A proposal needs the registry cohort the person would join.") + + // The conditional the schema cannot state: OTHER must say what. + const kindOther = input.subjectKindOther?.trim() || null + if (input.subjectKind === "OTHER" && !kindOther) { + throw new Refusal("Choosing 'other' means saying what — otherwise it is a category that means nothing.") + } + if (input.subjectKind !== "OTHER" && kindOther) { + throw new Refusal("A description only belongs on an 'other' proposal.") + } + + // A club named on a proposal must be this institution's. The composite foreign + // key refuses the write anyway; this turns a constraint violation into a + // sentence somebody can read. + if (input.organizationId) { + const org = await db.organization.findFirst({ + where: { id: input.organizationId, institutionId }, + select: { id: true }, + }) + if (!org) throw new Refusal("That club does not belong to this institution.") + } + + // Someone already admitted does not need admitting. Left out, this would + // create a second admission — and, once seats are charged, a second charge — + // for a person who already has access. + const already = await db.restrictedIdentity.findFirst({ + where: { institutionId, emailNormalized: normalized, status: "ACTIVE" }, + select: { id: true }, + }) + if (already) throw new Refusal("That person is already on the access registry for this institution.") + + const now = ctx.evaluatedAt + try { + const row = await db.onboardingProposal.create({ + data: { + institutionId, + subjectName, + subjectEmail, + subjectEmailNormalized: normalized, + subjectKind: input.subjectKind, + subjectKindOther: kindOther, + cohort, + organizationId: input.organizationId ?? null, + submittedById: actor.userId, + status: "DRAFT", + expiresAt: expiryFor("DRAFT", now), + openSubjectKey: normalized, + }, + select: SELECT, + }) + + await db.onboardingProposalEvent.create({ + data: { + proposalId: row.id, + institutionId, + kind: "CREATED", + fromStatus: "DRAFT", + toStatus: "DRAFT", + actorId: actor.userId, + actorRole: adminRoleAt(ctx, institutionId), + }, + }) + await audit(institutionId, actor.userId, ctx, "create", row.id, "ALLOW", null, row.organizationId) + return toProposal(row, now) + } catch (e) { + // The partial-unique on (institutionId, openSubjectKey). Two open proposals + // for one address is two Directors approving one admission twice. + if (isUniqueViolation(e)) { + throw new Refusal("There is already an open proposal for that person at this institution.") + } + throw e + } +} + +/** + * Record a REFUSAL against a tenant that owns the event — every refusal in this + * module, not just the one on `createProposal`. + * + * This began as `auditRefusedProposal`, reasoning carefully about `create` and + * leaving `actOnProposal` writing straight to the institution id its caller + * handed it. Both faults below were then reproduced against a real database on + * that path (`onboarding-attack.itest.ts`), which is what one rule written + * twice always costs — the same lesson as R3, in the audit layer. + * + * The obvious version — always audit the TARGET institution — has two faults, + * and both matter on a control whose whole purpose is stopping one institution + * reaching into another. + * + * It writes a row into another tenant's audit log. An outsider who names a + * neighbouring institution would put a row in that institution's security log, + * which is a small cross-tenant write performed by the very check that exists to + * prevent cross-tenant writes. + * + * And `AuditEvent.institutionId` is a real foreign key, so a target that does not + * exist at all fails with P2003 — a raw 500 from a hand-edited request, in place + * of the refusal that was already decided. + * + * So the row goes to a tenant the ACTOR belongs to, with the target recorded + * beside it (an institution id, not a person — nothing here is confidential). + * An actor who belongs to no institution gets no row at all: there is no tenant + * that owns that event, and inventing one is worse than the gap. They are + * refused either way, which is the part that has to be true. + */ +async function auditRefusal( + ctx: UserContext, + targetInstitutionId: string, + action: string, + reason: string, + resourceId?: string | null, + organizationId?: string | null, +) { + const ownsTarget = ctx.institutionRoles.some((m) => m.institutionId === targetInstitutionId) + const home = ownsTarget ? targetInstitutionId : ctx.institutionRoles[0]?.institutionId + if (!home) return + + await db.auditEvent.create({ + data: { + institutionId: home, + actorId: ctx.userId, + actorRole: adminRoleAt(ctx, home) ?? undefined, + action: `Onboarding.${action}`, + resourceType: "OnboardingProposal", + resourceId: resourceId ?? undefined, + // Another tenant's club id has no meaning in this tenant's log, and + // printing it there is a small disclosure by the very row that records a + // refusal to reach across. + organizationId: (ownsTarget ? organizationId : null) ?? undefined, + outcome: "DENY", + reason, + ...(ownsTarget ? {} : { metadata: { targetInstitutionId } }), + }, + }) +} + +function isUniqueViolation(e: unknown): boolean { + return typeof e === "object" && e !== null && (e as { code?: string }).code === "P2002" +} + +const KIND_FOR: Record = { + submit: "SUBMITTED", + approve: "APPROVED", + reject: "REJECTED", + withdraw: "WITHDRAWN", +} + +/** + * Take an action on a proposal — the one path for submit, approve, reject and + * withdraw. + * + * One function rather than four because the sequence is identical and the only + * thing that differs is which rule refuses: load, resolve the actor INCLUDING + * borrowed authority, ask the chain, compare-and-swap, record twice. Four copies + * of that is four places for the audit write or the swap to be forgotten. + */ +export async function actOnProposal( + ctx: UserContext, + institutionId: string, + proposalId: string, + action: OnboardingAction, + reason?: string | null, +): Promise { + const row = await db.onboardingProposal.findFirst({ + where: { id: proposalId, institutionId }, + select: SELECT, + }) + if (!row) { + // NOT `audit(institutionId, ...)`. Nothing has established that + // `institutionId` is a tenant this actor belongs to, or that it is a tenant + // at all — it arrives from the caller. Writing there has both faults + // `auditRefusal` exists to avoid: a row in a neighbouring institution's + // security log, put there by an outsider who guessed at a proposal id, and + // a raw P2003 from `AuditEvent_institutionId_fkey` for an institution that + // does not exist — a 500 in place of the refusal that was already decided. + // Both were reproduced against a real database before this line changed. + await auditRefusal(ctx, institutionId, action, "no such proposal at this institution", proposalId) + throw new Refusal("That proposal could not be found.") + } + + const proposal = toProposal(row, ctx.evaluatedAt) + + // Borrowed authority is resolved HERE, and only here. R3 is checked on + // `actor.userId`, which delegation never changes — see `onboarding-actor.ts`. + const { actor, delegators } = await effectiveOnboardingActor(ctx.userId, ctx, institutionId) + const onBehalfOf = delegators.length > 0 ? delegators[0] : null + + // An outsider gets the SAME answer for a proposal that exists as for one that + // does not. + // + // `getProposal` states this rule — "not found" and "not yours" are + // deliberately the same answer, because distinguishing them tells an admin at + // one institution that a proposal exists at another — and this path broke it: + // a real id came back "only the OSE Director closes an onboarding proposal" + // while a made-up one came back "could not be found". That difference is an + // existence oracle across the tenant boundary, in the module whose subject is + // that boundary. Ids are cuids, so it is a narrow one; it is also free to + // close. + // + // Read from `actor`, AFTER the merge, and not from `ctx`: the first version of + // this read `ctx` and refused a Director's genuine delegate who is not + // themselves a member of the institution — a case `ApprovalDelegation` + // expresses, and one only the real-database suite caught. Standing includes + // borrowed standing. + // + // Membership OR authorship, never a capability: an author may withdraw a + // proposal they raised after losing the authority to raise one, and that rule + // is `refusalFor`'s to apply, not this one's. + const hasStanding = + actor.institutionRoles.some((m) => m.institutionId === institutionId) || + proposal.submittedById === actor.userId + if (!hasStanding) { + await auditRefusal(ctx, institutionId, action, "no standing at this institution", proposalId) + throw new Refusal("That proposal could not be found.") + } + + const refusal = refusalFor(actor, proposal, action) + if (refusal) { + // Same routing for the same reason: an in-tenant refusal lands in this + // tenant's log exactly as before, and an outsider's refusal lands in a log + // they are accountable in. One answer to the question, not two. + await auditRefusal(ctx, institutionId, action, refusal, proposal.id, proposal.organizationId) + throw new Refusal(capitalise(refusal) + ".") + } + + const from = proposal.effectiveStatus + const to = nextStatusOrThrow(action, from) + const now = ctx.evaluatedAt + const decided = action === "approve" || action === "reject" + + // Compare-and-swap on the status the decision was made against. A second + // Director acting on the same row in the same second changes 0 rows here. + const swap = await db.onboardingProposal.updateMany({ + where: { id: proposal.id, institutionId, status: from }, + data: { + status: to, + // Terminal: release the open-proposal slot so the person can be proposed + // again later. Non-terminal: hold it, and re-date the deadline to the + // window the new state carries. + openSubjectKey: isTerminal(to) ? null : proposal.subjectEmailNormalized, + expiresAt: isTerminal(to) ? proposal.expiresAt : expiryFor(to, now), + ...(action === "submit" ? { submittedAt: now } : {}), + ...(decided + ? { decidedAt: now, decidedById: actor.userId, decisionReason: reason?.trim() || null } + : {}), + }, + }) + if (swap.count !== 1) { + await auditRefusal(ctx, institutionId, action, "the proposal changed while this decision was being made", proposal.id, proposal.organizationId) + throw new Refusal("Somebody else acted on this proposal first. Reload to see where it stands.") + } + + await db.onboardingProposalEvent.create({ + data: { + proposalId: proposal.id, + institutionId, + kind: KIND_FOR[action], + fromStatus: from, + toStatus: to, + actorId: actor.userId, + actorRole: adminRoleAt(ctx, institutionId), + onBehalfOfId: onBehalfOf?.id ?? null, + // The operator's own words stay on the proposal's record, which is behind + // the proposal's permission — never on the audit row, which is not. + reason: reason?.trim() || null, + }, + }) + + await audit( + institutionId, + actor.userId, + ctx, + action, + proposal.id, + "ALLOW", + onBehalfOf ? `on behalf of ${onBehalfOf.name}` : null, + proposal.organizationId, + ) + + const updated = await db.onboardingProposal.findFirstOrThrow({ + where: { id: proposal.id, institutionId }, + select: SELECT, + }) + return toProposal(updated, now) +} + +function capitalise(s: string): string { + return s.charAt(0).toUpperCase() + s.slice(1) +} + +function nextStatusOrThrow(action: OnboardingAction, from: OnboardingStatus): OnboardingStatus { + // `refusalFor` has already established this transition is legal; this is the + // narrowing, not a second check. If it ever fires, the two disagree and that + // is a fault rather than a refusal. + const map: Record = { + submit: "PENDING_DIRECTOR", + approve: "APPROVED", + reject: "REJECTED", + withdraw: "WITHDRAWN", + } + const to = map[action] + if (!ALL_STATUSES.includes(to)) throw new Error(`unknown target status ${to}`) + if (from === to) throw new Error(`refusalFor permitted a no-op ${action} from ${from}`) + return to +} + +/** + * Write EXPIRED onto proposals the clock has already expired. + * + * Records rather than decides: `effectiveProposalStatus` has expired these rows + * for every reader since the instant their deadline passed, so this changes no + * answer. It exists so the stored record matches what the product has been + * saying, and so the event log carries the moment it happened. + * + * Takes no actor. Nobody expires a proposal — the clock does — which is why the + * event row's `actorId` is null and why the audit row records `system`. + */ +export async function sweepExpiredProposals( + institutionId: string, + at: Date, +): Promise<{ expired: string[] }> { + const open = await db.onboardingProposal.findMany({ + where: { institutionId, status: { in: ["DRAFT", "PENDING_DIRECTOR"] }, expiresAt: { lte: at } }, + select: SELECT, + }) + + const expired: string[] = [] + for (const row of open) { + const view = toProposal(row, at) + const target = expirySweepTarget(view, at) + if (target !== "EXPIRED") continue + + const swap = await db.onboardingProposal.updateMany({ + where: { id: row.id, institutionId, status: row.status }, + data: { status: "EXPIRED", openSubjectKey: null }, + }) + // 0 means somebody decided it in the same instant. Their decision stands: + // expiry never overwrites an outcome a person reached. + if (swap.count !== 1) continue + + await db.onboardingProposalEvent.create({ + data: { + proposalId: row.id, + institutionId, + kind: "EXPIRED", + fromStatus: row.status, + toStatus: "EXPIRED", + actorId: null, + actorRole: null, + reason: "no decision within the window", + }, + }) + await audit(institutionId, null, null, "expire", row.id, "ALLOW", "expired without a decision", row.organizationId) + expired.push(row.id) + } + return { expired } +} + +// ─── R4: the only thing that authorises a registry write ───────────────────── + +/** + * What this proposal authorises, or null. + * + * R4 in one function, and the seam the downstream effect is built on: a caller + * that holds a `RegistryGrant` is holding proof that a Director approved this + * admission, because nothing else can produce one. Every other status — DRAFT, + * PENDING_DIRECTOR, REJECTED, WITHDRAWN, EXPIRED — returns null, so "declined + * grants nothing" is a value, not a convention. + */ +export function registryGrantFor(proposal: Proposal): RegistryGrant | null { + if (!grantsRegistryEntry(proposal.effectiveStatus)) return null + if (!proposal.decidedById || !proposal.decidedAt) return null + // The one cast the brand costs. Everything above it is R4. + return { + institutionId: proposal.institutionId, + emailNormalized: proposal.subjectEmailNormalized, + displayName: proposal.subjectName, + cohort: proposal.cohort, + organizationId: proposal.organizationId, + addedBy: proposal.decidedById, + sourceVersion: proposal.id, + addedVia: "onboarding-proposal", + decidedAt: proposal.decidedAt, + } as RegistryGrant +} + +/** Re-exported so a surface needs one import to render a proposal and its buttons. */ +export { availableActions, refusalFor } +export type { OnboardingAction, OnboardingStatus } diff --git a/apps/web/src/lib/tenancy/registry.test.ts b/apps/web/src/lib/tenancy/registry.test.ts index 88500016..703cdeee 100644 --- a/apps/web/src/lib/tenancy/registry.test.ts +++ b/apps/web/src/lib/tenancy/registry.test.ts @@ -158,8 +158,7 @@ describe("the registry matches prisma/schema.prisma", () => { // `organizationId` is nullable — an institution-level exception has no // club — which is why it is a new table rather than an `ApprovalRequest`; // see ADR-0015. - // - // 2026-08-21 (this merge): 25 → 27 tenant-scoped, 44 → 46 models. + // 2026-08-21 (landed on main): 25 → 27 tenant-scoped, 44 → 46 models. // `WebhookSubscription` and `WebhookReceipt` are the inbound-webhook pair — // what a connection is subscribed to receive, and what verifiably arrived. // Both are scoped: a subscription belongs to the institution whose @@ -175,19 +174,38 @@ describe("the registry matches prisma/schema.prisma", () => { // that do carry a tenant must be filtered by it, and the query layer's // `institutionId: ` predicate correctly hides a null-tenant row from // every tenant. + + // 2026-08-21 (on this branch, before the train): 25 → 27 tenant-scoped, 44 → 46 models. + // `OnboardingProposal` is the OSE-initiated path ONTO that access boundary + // (ADR-0013), and `OnboardingProposalEvent` is its append-only record of who + // decided what. Both carry institutionId with a real foreign key, and both + // are scoped for the same reason the rows above are: one institution's admin + // admitting a person to another institution's boundary is §3.2's failure + // with a new table in it. The event table carries institutionId with a + // COMPOSITE key back to its proposal rather than reaching its tenant through + // a join — `ApprovalStep`, the older shape of the same idea, is still in + // UNENFORCEABLE for exactly the lack of it. + + // + // 2026-08-21 (this merge): 27 → 29 tenant-scoped, 46 → 48 models. Two + // independent pairs landed on the same day and the two paragraphs above were + // each written as "44 → 46" about their own pair, so BOTH were stale the + // moment they met. The four assertions below auto-merged silently from one + // side — they came through reading 27/5/14/46, which is main's pair counted + // and this branch's pair not — and a suite that only ever compares the pins + // to the schema would have failed without saying why. // - // These four numbers were MEASURED against `schema.prisma` at merge time, - // not incremented from either side. `grep -c '^model ' prisma/schema.prisma` - // gives 46; the models whose body declares an `institutionId` field number - // 27. This branch was written against 43/24 and main had moved to 44/25; - // taking either side's four assertions would have been wrong by two or by - // three, and they auto-merge silently from one side, so nothing but - // measuring would have caught it. The buckets are asserted to sum to the - // model count immediately below for the same reason. - expect(TENANT_SCOPED).toHaveLength(27) + // Every number below was MEASURED on the merge result, not incremented: + // `grep -c '^model ' apps/web/prisma/schema.prisma` gives 48, and the blocks + // whose body matches `/^\s*institutionId\s/m` — the same regex this file + // parses with, a few lines up — number 29. All four new models carry it, so + // all four are TENANT_SCOPED, PLATFORM_GLOBAL and UNENFORCEABLE are + // untouched at 5 and 14, and 29 + 5 + 14 = 48 closes against the model + // count. The sum is asserted immediately below for exactly this reason. + expect(TENANT_SCOPED).toHaveLength(29) expect(PLATFORM_GLOBAL).toHaveLength(5) expect(Object.keys(UNENFORCEABLE)).toHaveLength(14) - expect(schemaModels).toHaveLength(46) + expect(schemaModels).toHaveLength(48) }) it("registry.ts's own prose agrees with these pins", () => { diff --git a/apps/web/src/lib/tenancy/registry.ts b/apps/web/src/lib/tenancy/registry.ts index 301021c9..11a9fd32 100644 --- a/apps/web/src/lib/tenancy/registry.ts +++ b/apps/web/src/lib/tenancy/registry.ts @@ -21,7 +21,7 @@ * decision about its tenancy at the moment it is added, rather than leaving one * to be discovered later by a customer. * - * The three buckets are honest about a real limitation: only 27 of 46 models + * The three buckets are honest about a real limitation: only 29 of 48 models * carry `institutionId`, so only those can be filtered by the query layer * today. The rest are named in UNENFORCEABLE rather than quietly omitted. * @@ -42,6 +42,27 @@ * have been wrong by two, which is exactly how this sentence went wrong before. * `WebhookSubscription` and `WebhookReceipt` are the models added here; both * carry `institutionId`, nullably in `WebhookReceipt`'s case (see its entry). + * + * 2026-08-21: 25 of 44 → 27 of 46 on merging `main` into the onboarding + * proposal chain. MEASURED against `schema.prisma` — `grep -c '^model '` gives + * 46, and 27 of those blocks declare an `institutionId` field — not incremented + * from either side. Neither side's number was right on its own: this branch was + * written against 25 of 44 while `SeatMeterEvent` landed on main, and main was + * written without `OnboardingProposal` or `OnboardingProposalEvent`. Both of + * those carry `institutionId`, so both are TENANT_SCOPED, and 27 + 5 + 14 = 46 + * closes against the model count. + * + * 2026-08-21: 27 of 46 → 29 of 48 on merging `main` into the onboarding + * proposal chain for the merge train. MEASURED, not incremented, and neither of + * the two entries above survived it: both were written as "27 of 46" on the same + * day, about different merges, and each was correct only about its own two + * models. `grep -c '^model ' apps/web/prisma/schema.prisma` now gives 48, and + * counting the blocks whose body matches `/^\s*institutionId\s/m` — the exact + * test `registry.test.ts` applies — gives 29. The four models are + * `OnboardingProposal` and `OnboardingProposalEvent` from this branch and + * `WebhookSubscription` and `WebhookReceipt` from main; all four carry + * `institutionId`, so all four are TENANT_SCOPED, and 29 + 5 + 14 = 48 closes + * against the model count. */ /** @@ -75,6 +96,7 @@ export const TENANT_SCOPED = [ // fit (a failure whose tenant cannot be determined) and why it stays a log. "Exception", "RestrictedRegistrySeal", + "OnboardingProposal", // Tenure's meter, on the tenant's wall. It records the institution's own seat // consumption and carries no rate, amount or currency — those are platform // records that are not in this schema (ADR-0017). Scoped rather than global @@ -93,6 +115,7 @@ export const TENANT_SCOPED = [ "RoleAssignment", "SeatHolding", "OrganizationAdvisor", + "OnboardingProposalEvent", // institutionId as a bare String, with no foreign key backing it "ApprovalRequest", "Event", diff --git a/docs/PROGRAM-BACKLOG.md b/docs/PROGRAM-BACKLOG.md index 813fe882..f1a93002 100644 --- a/docs/PROGRAM-BACKLOG.md +++ b/docs/PROGRAM-BACKLOG.md @@ -350,15 +350,41 @@ Two structural facts that change how items are scheduled: test for exactly that; **R4** only `APPROVED` authorises a registry write. The acceptance test the item names is implemented literally: approved by staff and an advisor, the proposal never leaves `PENDING_DIRECTOR` and confers nothing. -- **Blocked on a decision, recorded as ADR-0013**: `ApprovalRequest.organizationId` is a **non-null FK to - `Organization`** and an onboarding proposal has no club. Extending the one engine means making that column - nullable on a live table that also carries reimbursements; a dedicated model avoids that but is the second - approvals surface the item warns against. The rules ship first because authority rules are where this - codebase has been wrong in ways its tests did not notice, and a test asserts the module is imported by - nothing so it cannot half-ship while the store is undecided. -- **Also surfaced**: `audit.view` is minRole OSE_ADVISOR and the audit page prints event metadata, so a - proposal's payload — a named person's address and cohort — would be readable by every advisor if metadata - is populated the usual way. Recorded in ADR-0013 rather than left to be rediscovered. +- **Slice 2 done**: the store, and the chain is no longer inert. **ADR-0013 is Accepted** — an onboarding + proposal gets its OWN model rather than `ApprovalRequest.organizationId` becoming nullable on a live table + that also carries reimbursements. A nullable column cannot say "null only for onboarding": it adds a case + to every row including the ones that move money, and the `onDelete: Cascade` that is right for a club + approval is wrong for a record of who was admitted. PR #101 answered the same fork on a new table, so this + keeps one precedent rather than two. `OnboardingProposal` + `OnboardingProposalEvent` (append-only), both + tenant-scoped; `onboarding-proposals.ts` executes; `onboarding-actor.ts` resolves delegated authority and + asserts the identity invariant R3 depends on. Terminal states are APPROVED / REJECTED / WITHDRAWN / + **EXPIRED**, the last decided by the clock against a stored `expiresAt` rather than by a job, so an unrun + sweep cannot leave a stale proposal decidable. R1 and R2 are now DERIVED from `onboarding.propose` and + `onboarding.decide` in the capability catalog rather than restated. +- **The delegation attack, proven closed**: `ApprovalDelegation` + `effectiveApprovalContext` really does + hand a proposer the Director's entire role set — the test asserts that FIRST, so the refusal below cannot + pass for the wrong reason — and R3 refuses anyway because it reads `userId`. The test states the + disagreement with the role-shaped rule directly, and proves by enumeration over all eight role subsets + that no set of roles lets a submitter decide their own proposal, so simplifying R3 to a role check fails + there rather than in production. +- **The confidentiality inversion is closed**, not carried forward: `audit.view` is minRole OSE_ADVISOR and + the audit page prints event metadata, so audit rows here carry the proposal id, the action and the outcome + and nothing about the subject — not the address, not the name, not the cohort, not the operator's free + text. Those live on the proposal and its event log, behind the proposal's own permission. A test asserts + it by searching the serialised audit payload for each of them. +- **A number corrected on the way**: the grain argument was carried on "64 students hold 145 + club/position pairs, 51 holding more than one, four holding four". Re-measured from the tracked + workbook (`2026.2027 Club Org Student Leadership 7.17.xlsx`, column D of the four club sheets): + **106** occupied pairs across **64** students, **40** holding more than one — 38 hold two, 2 hold + three, nobody holds four. 145 is approximately every email cell in those sheets (106 student + 40 + advisor), which counts an advisor's attachment to a seat as a seat. The over-count factor is + **1.66x**, not 2.3x; the argument is unchanged because 63% of the roll still holds more than one + seat. The club sheets' student set is exactly the No-DUP members sheet's set, so two sheets agree + independently. Corrected in ADR-0013 and in the schema comment. +- **Still open, under ADR-0009**: what an approved proposal CREATES — `RestrictedIdentity` alone, a + `DirectoryPerson`, or a `User`. That is ADR-0009's question, and answering it here would settle the + Person/role/seat model as a side effect of an onboarding form. The seam is `registryGrantFor(proposal)`, + which returns a descriptor for APPROVED and null for every other status. - **Why**: the restricted registry (Tenure@21e8e33) is the Tenant #1 access boundary, and today its only source is the reconciled workbook. OSE runs a live institution: people join mid-year, advisors change, a club elects an officer who was never on the July roster. Without an admitted path to add them, the office diff --git a/docs/SESSION-STATE.md b/docs/SESSION-STATE.md index c8ebb9bd..0e68f56a 100644 --- a/docs/SESSION-STATE.md +++ b/docs/SESSION-STATE.md @@ -31,6 +31,9 @@ nothing is only in a worktree. ## Blocked on a decision, not on effort + + - **Transactional outbox** — Identity §21.2 and Integration §9 require **disjoint** envelope fields. One envelope cannot satisfy both as written; needs an ADR. -- **OSE onboarding beyond slice 1** — ADR-0013: does `ApprovalRequest.organizationId` become nullable on a live financial table, or does onboarding get its own model? - **Seat-as-billable-event** — depends on the outbox, and on an open commercial question: is the unit a *seat* or an *occupied seat*? Those invoice differently for a club with vacant officer positions. diff --git a/docs/decisions/ADR-0013-where-onboarding-proposals-live.md b/docs/decisions/ADR-0013-where-onboarding-proposals-live.md index 81347a33..5c144400 100644 --- a/docs/decisions/ADR-0013-where-onboarding-proposals-live.md +++ b/docs/decisions/ADR-0013-where-onboarding-proposals-live.md @@ -1,8 +1,10 @@ # ADR-0013 — Where an onboarding proposal lives -- **Status:** Proposed (2026-08-19). Records a fork; the rules ship, the store does not. +- **Status:** Accepted - **Date:** 2026-08-19 +- **Decided:** 2026-08-21 - **Related:** ADR-0009 (Person/role/seat target model — this touches it and must not decide it) +- **Deferred to:** ADR-0009 (Person/role/seat target model — it already owns what an approved proposal creates) - **Implements (partly):** the backlog item *[identity] OSE-initiated onboarding, with an approval chain headed by the OSE Director* ## The problem in one line @@ -46,42 +48,188 @@ migration and the worst idea: it makes `Organization` mean two things, and every club listing, count and permission check then has to know about a row that is not a club. Recorded so it is refused explicitly rather than rediscovered. -## What ships now, and why the store can wait - -The rules — who may propose, who may decide, that the proposer may never decide -their own proposal, that only `APPROVED` authorises a registry write — are a pure -module with no database, no session and no framework -(`apps/web/src/lib/identity/onboarding-chain.ts`). They are the same rules under -all three options. - -That ordering is deliberate. Authority rules are where this codebase has been -wrong before in ways its tests did not notice, most recently an access gate that -refused nobody because the query, not the rule, was broken. Getting the rules -exhaustively tested first — 27 assertions, 7 negative controls, including one -proving the self-approval check survives delegation — means the persistence -choice cannot quietly change what the chain permits. - -## What this ADR does not decide - -Which option. It also does not decide what a proposal *creates* on approval — -`RestrictedIdentity` alone, or a `DirectoryPerson`, or a `User` — because that is -the question ADR-0009 leaves open, and answering it here would settle the -Person/role/seat model as a side effect of an onboarding form. - -## The related thing that must not be forgotten - -`audit.view` is minRole **OSE_ADVISOR**, and the audit page prints the first -entries of each event's metadata. A proposal's payload is a named person's email -address and cohort. Whichever store is chosen, the audit events it writes must not -put that payload where every advisor can read it — the confidentiality inversion -is not hypothetical, it is the default if metadata is populated the usual way. - -## What would unblock it - -A decision on A versus B from the platform architecture owner. No external -access is needed. The rules module is inert until then, and deliberately so: it -is imported by nothing, which a test asserts, so it cannot half-ship. +## The decision: B + +An onboarding proposal gets its own model. Three things decided it. + +**A nullable column cannot say "null only for onboarding".** That is the whole +argument against A, and it is a property of the column rather than a matter of +discipline. `organizationId` is `String NOT NULL` today, so every one of the +hundreds of reads in the club and reimbursement paths is *proved* by the type +system to have a club. Relaxing it to `String?` does not add a case to the +onboarding rows; it adds a case to **every row in the table**, including the ones +that move money. TypeScript would flag the reads it can see, and would say +nothing about a Prisma `where: { organizationId }` that starts matching +differently, a `groupBy` that gains a null bucket, or a join that silently drops +rows. The invariant is doing work for the club flow right now, and A pays for the +onboarding flow by taking it away from everybody. + +**The cascade is right for a club approval and meaningless without a club.** +`onDelete: Cascade` on that relation encodes a real rule: a club's approvals are +the club's, and if the club goes, they go. An onboarding proposal is a fact about +a *person* and about the institution's access boundary — it must outlive any club +that happens to be named on it, because the record of who was admitted and who +decided is the audit trail. Under A those two lifetimes share one foreign key and +one delete rule, and only one of them can be right. + +**PR #101 already answered this same fork on a new table.** The precedent exists, +it is recent, and it was taken for the same reason. Choosing A here would leave +the repository with two contradictory answers to "a new kind of approval arrives — +widen the shared table or add one?", which is worse than either answer +consistently applied. One precedent, not two. + +### What option B's costs actually turned out to be + +The warning against a second approvals surface was about audit and concurrency +being got right twice. Both were, and neither is a copy: + +- **Concurrency.** Every transition is a compare-and-swap on the status the + decision was made against (`WHERE id AND institutionId AND status = `), + and a count of 0 is reported as a refusal rather than retried. Two Directors on + one proposal in one second produce one decision. +- **Audit.** Two records, deliberately. `OnboardingProposalEvent` is the + proposal's own append-only log, behind the proposal's permission; + `AuditEvent` is the security log and carries the proposal id, the action and + the outcome and **nothing about the subject**. That is the resolution of the + confidentiality inversion this ADR flagged and is described below. + +What was *not* duplicated is the authority model. `MAY_PROPOSE` and `MAY_DECIDE` +in `onboarding-chain.ts` are derived from `onboarding.propose` and +`onboarding.decide` in the capability catalog rather than restated, so this is a +second *store*, not a second authorization path. + +## The grain: one proposal admits one PERSON + +Measured from the roster workbook this repository tracks — +`2026.2027 Club Org Student Leadership 7.17.xlsx`, column D of the four club +sheets *Professional Clubs*, *Community Enrichment Clubs*, *Social Clubs* and +*Organizations*: + +| | | +|---|---| +| clubs and organizations | 28 | +| board seats | 209 | +| **occupied** club/position pairs | **106** | +| distinct students holding them | **64** | +| hold more than one seat | **40 of 64 (63%)** | +| distribution | 24 hold one · 38 hold two · 2 hold three | +| **pairs per person** | **1.66x** | + +Two sheets agree on the roll independently: the student set derived from the four +club sheets is *exactly* the set on `26-27_B. Members_No DUP_4.13.26` — 64 +addresses, same membership, no row in either that is missing from the other. With +the 18 Simon-domain advisors on `Board Advisors No Duplicates` that is the 82 +`lib/auth/eligibility.ts` documents. + +> **A correction, recorded because the number was nearly written into a decision.** +> This section first said **145** pairs, 51 of 64 holding more than one, and four +> holding four. It is wrong, and wrong in a specific way worth naming: 145 is +> approximately the count of *every email cell* in those four sheets — 106 +> student-column cells plus 40 advisor-column cells is 146. That conflates a seat +> a student holds with an advisor's attachment to a seat, which is two different +> things being counted as one. The real figures are above; nobody holds four +> seats, and the most anybody holds is three. + +So a proposal is one row per person. The alternative grain — a proposal per seat — +would raise two or three proposals for most of the roll, put the same name in +front of the Director more than once, and **over-count people by 1.66x**. It also +disagrees with the thing it feeds: `RestrictedIdentity` is keyed +`(institutionId, emailNormalized)`, one row per address, because access is +something a person has or does not have. A proposal per seat would have to be +de-duplicated back down to a person before it could be applied, and a +de-duplication step is a place to be wrong. + +The correction does not weaken the argument and is not offered as if it might: +1.66x is a smaller multiplier than 2.3x, and **63% of the roll still holds more +than one seat**, so multi-seat holders remain the ordinary case rather than a +handful of exceptions. That is the fact the grain rests on, and it survives the +number being fixed. + +The model therefore carries a nullable `organizationId` — the club a person is +proposed *into*, when there is one — rather than being keyed by it. An advisor may +have no club, and an institution-level member has none by definition. + +**This decides the grain of a proposal and deliberately does not decide a billing +unit.** A proposal that admits a person is evidence about admission, not about how +seats are charged; those are different questions, and the gap between 106 seats +and 64 people is the reason to keep them apart rather than a reason to answer both +here. + +## The confidentiality inversion, resolved + +This ADR flagged it and it is now closed rather than carried forward. +`audit.view` is minRole **OSE_ADVISOR** and the audit page prints the first +entries of each event's metadata, so populating metadata the usual way would put a +named person's address and cohort in front of every advisor at the institution. + +The resolution is that the two logs carry different things. `AuditEvent` rows +record the institution, the actor, the action, the proposal id and ALLOW/DENY with +the control's own words for a refusal — never the subject, and never the +operator's free-text reason, which is about a person. The subject and the reason +live on `OnboardingProposal` and `OnboardingProposalEvent`, behind the proposal's +own permission. A test asserts the audit payload contains neither the address, nor +the name, nor the cohort, nor the words the operator typed. + +## What this does not decide + +**What a proposal creates on approval** — `RestrictedIdentity` alone, or a +`DirectoryPerson`, or a `User`. Answering it here would settle the Person/role/seat +model as a side effect of an onboarding form. + +That open part is a separate record, as it must be — a status reading *Accepted +(mostly)* is a `PARTIAL` wearing an ADR status. But it is **ADR-0009**, which +already exists, is already `Proposed`, and is already the decision that owns which +of those three objects is canonical. A new ADR here would have restated ADR-0009's +conflict under a second number and left two records to keep in step; the register +row `IDENT-002-person-role-seat-model` already tracks it. + +One constraint is specific to this path and does not belong to ADR-0009, so it is +recorded here rather than lost between the two. **Whatever is chosen, the write +must carry provenance.** `RestrictedIdentity` rows without `addedBy`, `addedVia` +and `sourceVersion` cannot be sealed, and an unsealed registry does not enforce — +so an onboarding path that writes an unattributable row would silently turn the +access boundary off for everybody. That is a failure with no error message. +`RegistryGrant` carries all three fields for exactly this reason. + +The seam is explicit rather than implied: `registryGrantFor(proposal)` returns a +`RegistryGrant` descriptor for an APPROVED proposal and `null` for every other +status. A caller holding one is holding proof that a Director approved the +admission, because nothing else can produce one. What is then written from it is +decided by whoever implements the write, under ADR-0009 — and that decision gets +its own record at that point, when there is something to record beyond the fork +ADR-0009 already states. + +## What shipped + +- `OnboardingProposal` and `OnboardingProposalEvent`, both tenant-scoped, both + registered in `lib/tenancy/registry.ts` with the pinned counts updated. +- `onboarding-chain.ts` is no longer inert. It gained `EXPIRED`, a stored + `expiresAt`, and role sets derived from the capability catalog. R1–R4 are + unchanged in substance, and **R3 still reads the actor's user id rather than + their roles** — see below. +- `onboarding-proposals.ts`, the store, and `onboarding-actor.ts`, which resolves + delegated authority and asserts the identity invariant R3 depends on. + +### The rule that could not be relaxed + +R3 — the proposer never decides their own proposal — is checked on +`actor.userId === proposal.submittedById`, never on roles. `ApprovalDelegation` +lets a Director name a backup, and `effectiveApprovalContext` implements that by +merging the delegator's **entire role set** into the actor's context while leaving +`userId` alone. So a Director who names the proposer as their backup — for a +holiday, for a conference, for no reason at all — hands that person OSE_DIRECTOR. +Every role-shaped expression of R3 is defeated at that moment, by two people doing +something entirely ordinary. + +It is expressed on identity, so it holds. `onboarding-actor.test.ts` drives the +whole path — a real delegation row, the real resolver, the real merge, the real +rule — proves the proposer really does receive OSE_DIRECTOR, and proves the +refusal anyway. It also states the disagreement with the role-shaped rule +directly, and proves by enumeration over all eight role subsets that no set of +roles whatsoever lets a submitter decide their own proposal, so simplifying R3 to +a role check fails there rather than in production. ## Review -**2026-11-15**, owned by the platform architecture owner. +Closed. This record is superseded only by a later ADR, per the vocabulary in +`docs/decisions/README.md`. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index fe73b551..ae5e452e 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -88,7 +88,7 @@ would satisfy 28 times over — every one of them a *database* migration. | [0010](ADR-0010-money-storage-type.md) | Money storage type: the conflict does not exist, the requirement is still unmet | Proposed | 2026-08-17 | | [0011](ADR-0011-oklch-authoring-and-the-gamut-gate.md) | OKLCH authoring versus the gamut gate that rejects it | Proposed | 2026-08-17 | | [0012](ADR-0012-execution-ledger-in-the-deploying-repo.md) | The execution ledger in the deploying repository, and the status vocabulary it may use | Proposed | 2026-08-17 | -| [0013](ADR-0013-where-onboarding-proposals-live.md) | Where an onboarding proposal lives | Proposed | 2026-08-19 | +| [0013](ADR-0013-where-onboarding-proposals-live.md) | Where an onboarding proposal lives | Accepted | 2026-08-19 | | [0014](ADR-0014-tenant-hostnames-and-the-platform-router.md) | Tenant hostnames, and the router that sends people to them | Accepted | 2026-08-20 | | [0015](ADR-0015-the-platform-exception-object.md) | One exception object, and why it is not an ApprovalRequest | Accepted | 2026-08-20 | | 0016 | *Reserved — claimed by another change in the same merge sequence.* | — | — | @@ -121,12 +121,12 @@ duplicate number and one decision would become unreachable by the number every reference to it uses. **0015 was one of those reservations and is now filled** — by the platform -exception object, added in this change. Its row above is a link to a real file, -and the reservation row that stood in for it is GONE rather than left beside it. -That deletion is the half of the mechanism people forget: `a reserved number has -no ADR file` fails if a reservation outlives its ADR, so a stale reservation is -as loud as an undeclared hole. This is the mechanism working exactly as -designed — 0015 arrived after 0017 and 0018 and nothing had to be renumbered. +exception object. Its row above is a link to a real file, and the reservation +row that stood in for it is GONE rather than left beside it. That deletion is +the half of the mechanism people forget: `a reserved number has no ADR file` +fails if a reservation outlives its ADR, so a stale reservation is as loud as an +undeclared hole. This is the mechanism working exactly as designed — 0015 +arrived after 0017 and 0018 and nothing had to be renumbered. Reserving them, rather than leaving 0017 to follow 0014 across a hole, is what lets those changes merge in any order: each declares the numbers it does not @@ -134,14 +134,20 @@ hold, and clears its own row when the ADR arrives. Without the mechanism, contiguity would force merge order and number order to be the same thing, and one late review would renumber everything behind it. -### 9 of 17 are Proposed, and that is the point +### 8 of 17 are Proposed, and that is the point -ADR-0007 to ADR-0013, and ADR-0018, record conflicts rather than decisions — -each names two governing clauses that cannot both be satisfied, lists the -options, and stops. Constitution §4 permits exactly that response and forbids -the alternatives: "Do not resolve conflicts by choosing the easier +ADR-0004, ADR-0007 to ADR-0012, and ADR-0018 record conflicts rather than +decisions — each names two governing clauses that cannot both be satisfied, +lists the options, and stops. Constitution §4 permits exactly that response and +forbids the alternatives: "Do not resolve conflicts by choosing the easier implementation, silently weakening a rule, or creating a tenant fork." +ADR-0013 left that set on 2026-08-21. It is the shape the rule intends: the +conflict was recorded first and the rules shipped inert behind it, and when it +was decided the part that stayed open was handed to the ADR that already owned +it (ADR-0009) rather than becoming a qualifier on ADR-0013's status — or a +second number restating a conflict already on record. + ADR-0017 is the counter-example worth reading beside them: the programme item it answers also recorded an open decision, but that one was a question of commercial intent with no conflicting clauses behind it, so leaving it open @@ -149,6 +155,14 @@ would have meant a pricing policy arriving as a schema column instead. A `Proposed` ADR is the right answer to a conflict and the wrong answer to a choice nobody had made. +This heading is COUNTED, not carried forward. Both sides of this merge were +wrong about it: the onboarding branch said "7 of 13" because it was written +before four ADRs landed on main, and main said "9 of 17" because it still had +ADR-0013 open. `ls docs/decisions/ADR-*.md` is 17 and eight of those files carry +`Status: Proposed`, which is what `decision-records.test.ts` reads off disk. A +clean merge can invalidate this number with no textual conflict at all, so the +guard is the only thing standing between the heading and quiet drift. + They are enumerated, with owners and review dates, in the `BLOCKED_ARCHITECTURE` register at `apps/web/src/lib/governance/register.ts`. diff --git a/docs/implementation/global-engine-execution-ledger.md b/docs/implementation/global-engine-execution-ledger.md index 169e1dc0..104cbbf1 100644 --- a/docs/implementation/global-engine-execution-ledger.md +++ b/docs/implementation/global-engine-execution-ledger.md @@ -1,6 +1,6 @@ # Tenure pilot (Simon OSE) — execution ledger - + The authoritative record of what this repository has actually implemented, with evidence. This is the repository that builds and deploys the live tenant, so @@ -121,14 +121,16 @@ citing parent-only tests. ADR-0012 records this. - Status: FAIL - Evidence: cross-tenant denial is enforced at the query layer and tested (ADR-0002; `apps/web/src/lib/tenancy/`), which covers the database path for the - 27 TENANT_SCOPED models — the roster itself (seats, assignments, seat + 29 TENANT_SCOPED models — the roster itself (seats, assignments, seat holdings and advisor links) joined them, held to their parent's tenant by composite foreign keys rather than by convention, and `RestrictedRegistrySeal`, - `SeatMeterEvent`, `Exception`, `WebhookSubscription` and `WebhookReceipt` + `SeatMeterEvent`, `Exception`, `OnboardingProposal` with its append-only + `OnboardingProposalEvent`, and `WebhookSubscription` with `WebhookReceipt` joined them as the access seal, the seat meter, the platform exception - register and the inbound-webhook pair. It is not proven per-path for API - routes, background jobs, exports, search or AI answers, and - 14 of 46 models are UNENFORCEABLE at the chokepoint. + register, the path onto the access boundary and the inbound-webhook pair. It + is not proven per-path for API routes, background jobs, exports, search or AI + answers, and + 14 of 48 models are UNENFORCEABLE at the chokepoint. - [ ] **SIMON-070-003** — Render all target resources through Parent IaC with deterministic tags, ownership, stack boundaries, deletion/retention policy, and drift detection. - Status: FAIL