diff --git a/apps/backstage/src/components/member-home.tsx b/apps/backstage/src/components/member-home.tsx index 2bbf95c6..a6bee5d5 100644 --- a/apps/backstage/src/components/member-home.tsx +++ b/apps/backstage/src/components/member-home.tsx @@ -15,6 +15,7 @@ import { useActivitiesByTerm } from "../features/activities/hooks/use-activities import { useInitiativesByTerm } from "../features/initiatives/hooks/use-initiatives-by-term"; import { usePositions } from "../features/positions/hooks/use-positions"; import { joinYear } from "../features/members/lib/member-display"; +import { isSelfMember } from "../features/members/lib/member-permissions"; import { summarizeParticipations } from "../features/members/lib/participation-summary"; import { MemberPointsSummary } from "../features/members/components/member-points-summary"; import { MemberCredentialCard } from "../features/members/components/member-credential-card"; @@ -92,7 +93,7 @@ export function MemberHome() { // at a role, so the honest mirror is doc ownership — NOT the CASL own-doc grant, which // only members carrying the built-in Member role hold (a roles:["Treasury"] principal // would lose a self-edit the rules would have accepted). - const canEditSelf = member.uid !== undefined && member.uid === uid; + const canEditSelf = isSelfMember(member, uid); const cargoId = member.positions?.[termId]?.cargoId ?? null; const cargo = cargoId ? positionsById.get(cargoId) : null; diff --git a/apps/backstage/src/features/members/components/member-drawer.test.tsx b/apps/backstage/src/features/members/components/member-drawer.test.tsx index 46656f78..b6fa3685 100644 --- a/apps/backstage/src/features/members/components/member-drawer.test.tsx +++ b/apps/backstage/src/features/members/components/member-drawer.test.tsx @@ -10,6 +10,15 @@ vi.mock("@tanstack/react-router", () => ({ Link: ({ children }: { children: React.ReactNode }) => {children}, })); +// The drawer now reads the caller's uid to decide whether the row it opened is the caller's +// OWN (the members table lists it too), which makes the edit a SELF-assignment. Mocked as a +// factory with no `importOriginal`: lib/auth/auth builds its store from getFirebase().auth at +// module scope, so merely evaluating the real module initializes Firebase and the whole file +// fails to collect. uid "someone-else" keeps every case below a non-self edit. +vi.mock("../../../lib/auth/auth", () => ({ + useAuth: () => ({ user: { uid: "someone-else" }, claims: { roles: ["Admin"] } }), +})); + const m: Member = { id: "1", name: "Ana Gómez", diff --git a/apps/backstage/src/features/members/components/member-drawer.tsx b/apps/backstage/src/features/members/components/member-drawer.tsx index 4dbbf5bd..a1e9fd29 100644 --- a/apps/backstage/src/features/members/components/member-drawer.tsx +++ b/apps/backstage/src/features/members/components/member-drawer.tsx @@ -14,7 +14,9 @@ import { MemberForm } from "./member-form"; import { joinYear, memberPositionLabel } from "../lib/member-display"; import { memberFormDefaults } from "../lib/member-form-defaults"; import { useMemberPhoto } from "../hooks/use-member-photo"; +import { isSelfMember } from "../lib/member-permissions"; import { Can } from "../../../lib/authz/ability-context"; +import { useAuth } from "../../../lib/auth/auth"; import { useCan } from "../../../lib/authz/use-can"; interface MemberDrawerProps { @@ -136,7 +138,9 @@ function EditBody({ onSubmit: (data: MemberInput) => Promise; }) { const { onUpload, onRemove } = useMemberPhoto(member.id); - const { canAssignBoardSeat } = useCan(); + const { canAssignBoardSeat, isAdmin } = useCan(); + // The table lists the caller's own row too, so this drawer can be a self-assignment. + const uid = useAuth().user?.uid; return (
diff --git a/apps/backstage/src/features/members/components/member-form.test.tsx b/apps/backstage/src/features/members/components/member-form.test.tsx index c2252593..9163e9b2 100644 --- a/apps/backstage/src/features/members/components/member-form.test.tsx +++ b/apps/backstage/src/features/members/components/member-form.test.tsx @@ -3,8 +3,36 @@ import { render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import type { MemberInput, Position } from "@luminova/types"; import { MemberForm } from "./member-form"; +import { cargoNoteIds } from "./no-assignable-cargos-note"; import { toMemberUpdateDoc } from "../repositories/member-mapper"; import { pickDate } from "../../../test/pick-date"; +import { permissionLabel } from "../../permissions/lib/permission-matrix"; + +// Through the same helper the form calls, not a hand-typed literal: the ids are no longer +// exported individually, and a test that re-typed one would keep passing after a rename. +const MINT_PENDING_NOTE_ID = cargoNoteIds("member").mintPending; + +// The note names the permission through `permissionLabel`, and its own comment says the two +// features must not drift. Assert against the same source, not a hardcoded copy — a literal +// here would keep passing after either half of the label is renamed, which is exactly the +// coupling the note is worried about. +const BOARD_SEAT_LABEL = permissionLabel("update:BoardSeat"); + +// The mint-pending note used to say "permisos de administrador", which was true only of its +// one original trigger. It now fires for a SELF-assignment of any granting cargo, so copy +// naming administrator permissions would be a lie in that case. Matched on the outcome half of +// the sentence, which is the part both triggers share. +const MINT_PENDING_COPY = /no se aplicarán hasta que un administrador confirme la asignación/i; + +// The four authority props are REQUIRED on the component (a call site that forgets one used to +// compile clean and fail OPEN on `isSelfAssignment`). Spread FIRST in every render below so a +// case that cares about one still just names it — the explicit prop wins. +const FORM_AUTHORITY = { + allowPowerGrants: false, + allowReplacePowerCargo: false, + assignerIsAdmin: false, + isSelfAssignment: false, +} as const; const positions: Position[] = [ { @@ -73,7 +101,9 @@ const inactiveCargoPosition: Position = { describe("MemberForm", () => { it("blocks submit and shows an error when required fields are empty", async () => { const onSubmit = vi.fn(); - render(); + render( + , + ); await userEvent.click(screen.getByRole("button", { name: /crear/i })); expect(await screen.findAllByText("Mínimo 3 caracteres.")).not.toHaveLength(0); expect(onSubmit).not.toHaveBeenCalled(); @@ -81,7 +111,9 @@ describe("MemberForm", () => { it("renders the gender toggle and requires it on submit", async () => { const onSubmit = vi.fn(); - render(); + render( + , + ); expect(screen.getByRole("group", { name: "Género" })).toBeInTheDocument(); await userEvent.type(screen.getByLabelText(/nombre/i), "Ana Pérez"); await userEvent.type(screen.getByLabelText(/correo/i), "ana@jci.bo"); @@ -97,7 +129,13 @@ describe("MemberForm", () => { // authority that renders them all. it("shows gendered cargo labels and excludes comisiones from the cargo options", async () => { render( - , + , ); await userEvent.click(screen.getByRole("button", { name: "Femenino" })); await userEvent.click(screen.getByLabelText("Cargo")); @@ -131,20 +169,27 @@ describe("MemberForm", () => { ]; const { unmount } = render( , ); - expect(screen.getByRole("note")).toHaveTextContent(/Asientos de directiva/); + expect(screen.getByRole("note")).toHaveTextContent(BOARD_SEAT_LABEL); await userEvent.click(screen.getByLabelText("Cargo")); expect(await screen.findByText("Sin resultados")).toBeInTheDocument(); unmount(); // The delegate sees the very same catalog as assignable, and no note. render( - , + , ); expect(screen.queryByRole("note")).not.toBeInTheDocument(); await userEvent.click(screen.getByLabelText("Cargo")); @@ -158,6 +203,7 @@ describe("MemberForm", () => { const onSubmit = vi.fn().mockResolvedValue(undefined); render( { const onSubmit = vi.fn(); render( { it("submits valid data with the chosen cargo and comisiones", async () => { const onSubmit = vi.fn().mockResolvedValue(undefined); - render(); + render( + , + ); await userEvent.type(screen.getByLabelText(/nombre/i), "Ana Pérez"); await userEvent.type(screen.getByLabelText(/correo/i), "ana@jci.bo"); await userEvent.click(screen.getByRole("button", { name: "Femenino" })); @@ -239,7 +293,13 @@ describe("MemberForm", () => { it("locks comisiones as Comité Ejecutivo Local and clears them for a CEL cargo", async () => { const onSubmit = vi.fn().mockResolvedValue(undefined); render( - , + , ); await userEvent.type(screen.getByLabelText(/nombre/i), "Ana Pérez"); await userEvent.type(screen.getByLabelText(/correo/i), "ana@jci.bo"); @@ -258,14 +318,21 @@ describe("MemberForm", () => { }); it("groups fields under section headers", () => { - render( {}} />); + render( + {}} + />, + ); expect(screen.getByText("Datos personales")).toBeInTheDocument(); expect(screen.getByText("Membresía")).toBeInTheDocument(); }); it("renders a children slot before the submit button", () => { render( - {}}> + {}}> extra-slot , ); @@ -275,6 +342,7 @@ describe("MemberForm", () => { it("shows inactive assigned cargo with (inactivo) suffix in combobox trigger", async () => { render( { // (createPositionsSafe applies the same predicate). Without it a non-Admin sees a // grant-free CEL cargo, picks 'Presidente', and the create 403s into a generic error. it("hides a grant-free CEL cargo from a non-Admin and keeps the JDL dirección", async () => { - render(); + render( + , + ); await userEvent.click(screen.getByLabelText("Cargo")); expect(await screen.findByText("Director de Área")).toBeInTheDocument(); expect(screen.queryByText("Presidente")).not.toBeInTheDocument(); @@ -296,7 +371,13 @@ describe("MemberForm", () => { it("shows a grant-free CEL cargo to an Admin", async () => { render( - , + , ); await userEvent.click(screen.getByLabelText("Cargo")); expect(await screen.findByText("Presidente")).toBeInTheDocument(); @@ -324,13 +405,16 @@ describe("MemberForm", () => { it("BLOCKING: does NOT lock a grant-free CEL seat — clearing it is the allowed takedown", () => { render( , ); - expect(screen.queryByText(/Solo un Admin puede cambiar el cargo/i)).not.toBeInTheDocument(); + expect( + screen.queryByText(/Solo un administrador puede cambiar el cargo/i), + ).not.toBeInTheDocument(); }); // Dropping the seat from the options handed it to the `(inactivo)` fallback, which re-added @@ -339,6 +423,7 @@ describe("MemberForm", () => { it("BLOCKING: never labels the active grant-free CEL seat '(inactivo)' to a non-Admin", () => { render( { const onSubmit = vi.fn().mockResolvedValue(undefined); render( { const onSubmit = vi.fn().mockResolvedValue(undefined); render( { const onSubmit = vi.fn().mockResolvedValue(undefined); render( { it("does NOT lock a non-Admin editing a member on a grant-free JDL dirección", () => { render( , ); - expect(screen.queryByText(/Solo un Admin puede cambiar el cargo/i)).not.toBeInTheDocument(); + expect( + screen.queryByText(/Solo un administrador puede cambiar el cargo/i), + ).not.toBeInTheDocument(); + }); + + // BLOCKING: the two rules conjuncts of positionsAssignmentSafe() are gated on DIFFERENT + // principals. `update:BoardSeat` lifts the NEW side (cargoAssignableByNonAdmin, the cargo + // written in) — which is what `allowPowerGrants` carries — but the OLD side + // (currentCargoGrantsEmpty, the cargo being REPLACED) is Admin-ROLE only and is deliberately + // NOT delegated. So the delegate is the one principal for whom both flags disagree, and the + // form must still lock. While the lock was `!allowPowerGrants && locked(...)` this render + // handed a delegate an open picker on a write the rules ALWAYS deny: render-then-403. + const powerCargo: Position = { + id: "pos-secre", + title: "Secretario", + titleFemale: "Secretaria", + category: "CEL", + grants: ["Secretary"], + term: null, + sigla: null, + description: "Lleva las actas.", + active: true, + deletedAt: null, + }; + + it("BLOCKING: locks for a board-seat DELEGATE on a member seated on a power-granting cargo", () => { + render( + , + ); + const trigger = screen.getByLabelText("Cargo"); + expect(trigger).toBeDisabled(); + const note = screen.getByText(/Solo un administrador puede cambiar el cargo/i); + expect(note).toBeInTheDocument(); + // The note sits after the field in the DOM, so the association is the only way a + // screen-reader user reaching a disabled trigger meets the reason. + expect(trigger).toHaveAttribute("aria-describedby", note.id); + }); + + it("does NOT lock an Admin on that same power-granting seat", () => { + render( + , + ); + expect(screen.getByLabelText("Cargo")).not.toBeDisabled(); + expect( + screen.queryByText(/Solo un administrador puede cambiar el cargo/i), + ).not.toBeInTheDocument(); + }); + + // ---- self-assignment: the second, disjoint refusal in resolveTrustedGrants ---- + // + // BLOCKING: the finding. A delegate holding update:Member + update:BoardSeat opens THEIR OWN + // profile and seats themselves on a vacant NON-Admin-granting power cargo. Every gate says + // yes — boardSeatDelegate() permits the write, the seat publishes to the Directiva, the save + // returns 200 — but `resolveTrustedGrants` computes `selfAssigned = assignedBy === memberUid` + // and honors it only for an Admin, so no claim is minted. syncMemberClaims is a background + // trigger, so no response carries the refusal; and while the warning keyed on + // `grants.includes("Admin")` alone, a Secretario seat rendered NO note whatsoever. + const pickSecretario = async () => { + await userEvent.click(screen.getByLabelText("Cargo")); + await userEvent.click(await screen.findByText("Secretario")); + }; + + it("BLOCKING: warns a delegate seating THEMSELVES on a non-Admin power cargo", async () => { + render( + , + ); + expect(screen.queryByText(MINT_PENDING_COPY)).not.toBeInTheDocument(); + await pickSecretario(); + const note = screen.getByText(MINT_PENDING_COPY); + expect(note.id).toBe(MINT_PENDING_NOTE_ID); + // The note sits after the field in the DOM, so the association is the only way a + // screen-reader user on the trigger meets it before committing the save. + expect(screen.getByLabelText("Cargo")).toHaveAttribute( + "aria-describedby", + MINT_PENDING_NOTE_ID, + ); + // The copy must not name administrator permissions: this cargo grants Secretary. + expect(note).not.toHaveTextContent(/permisos de administrador/i); + }); + + it("BLOCKING: the SAME delegate on the SAME cargo for someone else stays silent", async () => { + // The control that makes the case above about self-assignment and nothing else. Identical + // props but `isSelfAssignment={false}`: update:BoardSeat DOES mint a Secretary seat for + // another member, so a note here would be false and would train users past the real one. + render( + , + ); + await pickSecretario(); + expect(screen.queryByText(MINT_PENDING_COPY)).not.toBeInTheDocument(); + expect(screen.getByLabelText("Cargo")).not.toHaveAttribute("aria-describedby"); + }); + + it("stays silent for an ADMIN seating themselves — they mint it", async () => { + // `assignerIsAdmin` satisfies both arms of the trust gate, so self-assignment is not a + // refusal for them. Without this cell the fix could be "warn on any self-assignment". + render( + , + ); + await pickSecretario(); + expect(screen.queryByText(MINT_PENDING_COPY)).not.toBeInTheDocument(); + }); + + // Both new props default to false, which is what keeps the ~20 renders above (and the + // invite drawer, which passes only `assignerIsAdmin`) compiling. Pin the defaults: a + // required `isSelfAssignment` would be a breaking prop, but a default of TRUE would put the + // note on every create. + it("defaults both new props to false rather than warning by accident", async () => { + render( + , + ); + await pickSecretario(); + expect(screen.queryByText(MINT_PENDING_COPY)).not.toBeInTheDocument(); + }); + + // BLOCKING: the takedown note now has an id and is the third arm of cargoNoteId(). While the + // association was a two-branch ternary over noCargos/locked, a takedown-only editor got + // `aria-describedby={undefined}`: the note rendered, sat AFTER the field in the DOM, and a + // screen-reader user reaching a trigger whose seat option is disabled met no reason at all. + it("BLOCKING: associates the takedown note with the trigger", () => { + render( + , + ); + const note = screen.getByText(/solo un administrador puede asignarlo/i); + expect(note.id).toBeTruthy(); + expect(screen.getByLabelText("Cargo")).toHaveAttribute("aria-describedby", note.id); + // Not the mint-pending id: a grant-free seat mints nothing to warn about, and the two + // notes' ids must not be interchangeable. + expect(note.id).not.toBe(MINT_PENDING_NOTE_ID); }); it("renders comisión option as 'sigla — title' when sigla is present", async () => { render( ; @@ -43,7 +58,20 @@ interface MemberFormProps { * ones and CEL seats alike (rules' `cargoAssignableByNonAdmin`, applied by both * `createPositionsSafe` and `positionsAssignmentSafe`). Non-Admin sees only assignable * cargos plus the current selection. */ - allowPowerGrants?: boolean; + allowPowerGrants: boolean; + /** Whether the editor may REPLACE a cargo that already confers power (rules' + * `currentCargoGrantsEmpty`, the other conjunct). Admin role only — `update:BoardSeat` + * deliberately does NOT lift this one, so it must not be folded into `allowPowerGrants`. + * See positionsLockedForEditor(). */ + allowReplacePowerCargo: boolean; + /** Whether the CALLER holds the Admin role, which is what beacon's `resolveTrustedGrants` + * keys the mint on. Named after the minting authority, not after `allowReplacePowerCargo`, + * which mirrors a different rules predicate and only happens to equal it today. */ + assignerIsAdmin: boolean; + /** Whether the member being edited IS the caller. The trust gate refuses to mint a + * self-assignment of any granting cargo from a non-Admin — confer power on others, never on + * yourself — so the picker must say so before the click. */ + isSelfAssignment: boolean; children?: ReactNode; } @@ -73,7 +101,10 @@ export function MemberForm({ onSubmit, showPreview, avatarSeed, - allowPowerGrants = false, + allowPowerGrants, + allowReplacePowerCargo, + assignerIsAdmin, + isSelfAssignment, children, }: MemberFormProps) { const [formError, setFormError] = useState(null); @@ -113,14 +144,14 @@ export function MemberForm({ // per-form copy is what let this one re-add the held seat labelled "(inactivo)" while the // other dropped it). const assignedCargoId = defaultValues?.cargoId ?? null; - // A power-granting assigned cargo locks cargo/comisiones for a non-Admin — the write - // re-stamps the same cargoId and `currentCargoGrantsEmpty()` blocks clearing it, so no + // A power-granting assigned cargo locks cargo/comisiones for anyone but an Admin — the + // write re-stamps the same cargoId and `currentCargoGrantsEmpty()` blocks clearing it, so no // positions change succeeds. Bio edits still save, because the mapper omits an unchanged // slot. A grant-free CEL seat is NOT locked: clearing it is deliberately allowed, so the // form stays open, the seat renders disabled (visible, not assignable) and "Quitar cargo" - // makes the takedown reachable. See positionsLockedForNonAdmin() / cargoTakedownOnly(). - const assignedCargo = positions.find((p) => p.id === assignedCargoId); - const positionsLocked = !allowPowerGrants && positionsLockedForNonAdmin(assignedCargo); + // makes the takedown reachable. See positionsLockedForEditor() / cargoTakedownOnly(). + const held = heldCargo(positions, assignedCargoId); + const positionsLocked = positionsLockedForEditor(held, allowReplacePowerCargo); const cargoTakedown = cargoTakedownOnly(selectedCargo, allowPowerGrants); const cargoOptions = cargoOptionsForEditor({ positions, @@ -128,6 +159,19 @@ export function MemberForm({ allowPowerGrants, assignedCargoId, }); + const noCargos = noAssignableCargos({ cargoOptions, allowPowerGrants, locked: positionsLocked }); + const mintPending = cargoGrantNeedsAdminAssigner( + selectedCargo, + assignerIsAdmin, + isSelfAssignment, + ); + // Every note explaining the picker sits after the field in the DOM, so without this a + // screen-reader user reaching the trigger hears "Sin resultados" or a disabled control and + // never meets the reason. Priority order and the co-firing rules live in cargoNoteId(). + const describedBy = cargoNoteId( + { noCargos, locked: positionsLocked, takedown: cargoTakedown, mintPending }, + NOTE_IDS, + ); const comisionLabel = (p: Position) => (p.sigla ? `${p.sigla} — ${p.title}` : p.title); const activeComisionOptions = positions @@ -249,6 +293,7 @@ export function MemberForm({ }} placeholder="Sin cargo" disabled={positionsLocked} + aria-describedby={describedBy} /> {cargoTakedown && ( + {/* The BUTTON goes away when the callable would refuse; the feedback below does not. + `blocked` becomes true the moment this invite succeeds (the member now has a uid and + the hook invalidates the query), so gating the whole component on it would erase the + result of the click that set it. */} + {!blocked && ( + + )} {error && (

{error}

)} - {sent && ( + {result?.emailSent && (

Invitación enviada por correo.

)} - + { + if (!o) setDismissed(true); + }} + title="Acceso de miembro" + >
+ {/* This dialog exists ONLY on the mail-failure branch, so it never claims the member + was emailed. The mail failure is repeated here rather than left to the header + alert: the modal's aria-hidden takes that alert out of the accessibility tree. */}

- Comparte este enlace con el miembro para que cree su contraseña e inicie sesión. + No se pudo enviar el correo. Comparte este enlace con el miembro para que cree su + contraseña e inicie sesión.

{link} - + {copyState === "failed" && ( +

+ No se pudo copiar. Selecciona el enlace de arriba y cópialo manualmente. +

+ )}
diff --git a/apps/backstage/src/features/members/components/member-row-menu.test.tsx b/apps/backstage/src/features/members/components/member-row-menu.test.tsx index 792236f7..9590ae7a 100644 --- a/apps/backstage/src/features/members/components/member-row-menu.test.tsx +++ b/apps/backstage/src/features/members/components/member-row-menu.test.tsx @@ -2,7 +2,7 @@ import { render, screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { describe, it, expect, vi } from "vitest"; import { Timestamp } from "firebase/firestore"; -import type { Member } from "@luminova/types"; +import type { Member, Position } from "@luminova/types"; import type { AuthClaims } from "@luminova/auth/roles"; import { roleClaims } from "@luminova/auth/test-helpers"; import { AbilityProvider } from "../../../lib/authz/ability-context"; @@ -33,19 +33,54 @@ const handlers = { onUnpublish: noop, }; +const cargo = (id: string, grants: Position["grants"]): Position => ({ + id, + title: id, + titleFemale: id, + category: "CEL", + grants, + term: null, + sigla: null, + description: "", + active: true, + deletedAt: null, +}); + +// The catalog the row menu resolves seated cargos against. `pos-power` confers a role, so +// beacon's power-seat guard refuses a non-Admin provisioning its holder; `pos-plain` is the +// grant-free control that proves the gate keys on the grants, not on being seated at all. +const POSITIONS_BY_ID: ReadonlyMap = new Map([ + ["pos-power", cargo("pos-power", ["Secretary"])], + ["pos-plain", cargo("pos-plain", [])], +]); + // Render the menu behind the REAL Can/ActionGate wiring so the per-item authz gates are // actually exercised (the previous suite mocked both to always-render children, making // every "is visible" assertion unconditionally true). uid="admin" so no uid-scoped // conditional grant muddies the coarse gates under test. -function renderMenu(m: Member, claims: AuthClaims, overrides?: Partial) { +function renderMenu( + m: Member, + claims: AuthClaims, + overrides?: Partial, + positionsById: ReadonlyMap = POSITIONS_BY_ID, +) { return render( - + , ); } const ADMIN: AuthClaims = roleClaims("Admin"); +// The delegation principal: create:MemberLogin without the Admin role. update:Member is what +// keeps the menu itself reachable. +const DELEGATE: AuthClaims = { + roles: ["Member"], + perms: ["update:Member", "create:MemberLogin"], +}; +const seated = (cargoId: string | null, term = "2026") => ({ + positions: { [term]: { cargoId, comisionIds: [] } }, +}); describe("MemberRowMenu", () => { it("shows Desactivar for an active member and Invitar when no uid", async () => { @@ -104,7 +139,11 @@ describe("MemberRowMenu", () => { const claims: AuthClaims = { roles: ["Member"], perms: ["read:Member"] }; render( - + , ); await userEvent.click(screen.getByLabelText(/Acciones para Ana/)); @@ -145,4 +184,97 @@ describe("MemberRowMenu", () => { await userEvent.click(screen.getByLabelText(/Acciones para Ana/)); expect(screen.queryByText("Invitar a la app")).not.toBeInTheDocument(); }); + + // --- memberProvisionBlocked, the non-Admin half of the invite gate --- + // + // Every case above renders either a member with NO uid or an Admin caller, so the second + // disjunct of the old `isAdmin || !member.uid` gate was never the deciding term and the + // whole conjunct survived deletion. These pin each refusal `provisionMember` applies to a + // NON-Admin caller: adoption, direct grants, and a power-granting cargo in any term. + + const openMenu = () => userEvent.click(screen.getByLabelText(/Acciones para Ana/)); + + it("shows the invite item to a delegate for a clean, unseated member", async () => { + renderMenu(member({ status: "Activo" }), DELEGATE); + await openMenu(); + expect(screen.getByText("Invitar a la app")).toBeInTheDocument(); + }); + + it("shows it to a delegate for a member seated on a GRANT-FREE cargo", async () => { + // The control for the two cases below: being seated is not the blocker, conferring power + // is. Without this the cargo clause could be "seated at all" and nothing would notice. + renderMenu(member({ status: "Activo", ...seated("pos-plain") }), DELEGATE); + await openMenu(); + expect(screen.getByText("Invitar a la app")).toBeInTheDocument(); + }); + + it("BLOCKING: hides it from a delegate for a member who already has a login (adoption)", async () => { + // beacon tags this reprovision-requires-admin: a delegate may only mint a NEW login, so + // "Reenviar invitación" would 403 on every click. + renderMenu(member({ status: "Activo", uid: "u1" }), DELEGATE); + await openMenu(); + expect(screen.queryByText("Reenviar invitación")).not.toBeInTheDocument(); + }); + + it("BLOCKING: hides it from a delegate for a member seated on a power-granting cargo", async () => { + renderMenu(member({ status: "Activo", ...seated("pos-power") }), DELEGATE); + await openMenu(); + expect(screen.queryByText("Invitar a la app")).not.toBeInTheDocument(); + }); + + it("BLOCKING: hides it from a delegate for a member carrying direct roleIds", async () => { + renderMenu(member({ status: "Activo", roleIds: ["custom-role"] }), DELEGATE); + await openMenu(); + expect(screen.queryByText("Invitar a la app")).not.toBeInTheDocument(); + }); + + it("BLOCKING: hides it from a delegate while the cargo catalog is still empty (fails closed)", async () => { + // The positions query has not landed, so the seated id resolves to undefined. Fail closed: + // offer the invite once the catalog arrives rather than offer it and then 403. + renderMenu( + member({ status: "Activo", ...seated("pos-power") }), + DELEGATE, + undefined, + new Map(), + ); + await openMenu(); + expect(screen.queryByText("Invitar a la app")).not.toBeInTheDocument(); + }); + + // BLOCKING: an EMPTY-STRING cargoId is a MALFORMED seat, not an empty one, and the gate used + // to drop it with a truthiness test — so this member looked unseated, the invite was offered, + // and the click 403'd with power-seat-requires-admin naming a cargo that does not exist. + // beacon's readCargoIds pushes "" on purpose ("a malformed shape must never read as 'no + // cargo' — that is the guard's own bypass") and refuses it at isSafeDocId; the client now + // lets it fall through to the unresolvable-cargo clause and fails closed the same way. + it("BLOCKING: hides it from a delegate for a member whose cargoId is an empty string", async () => { + renderMenu(member({ status: "Activo", ...seated("") }), DELEGATE); + await openMenu(); + expect(screen.queryByText("Invitar a la app")).not.toBeInTheDocument(); + }); + + // The paired negative, so the fix cannot be over-applied into "any falsy cargoId blocks": a + // null cargoId is a genuinely unseated term (readCargoIds `continue`s past it) and is the + // ordinary shape of most member docs. Blocking here would hide the invite chapter-wide. + it("shows it to a delegate for a member whose term has a NULL cargoId", async () => { + renderMenu(member({ status: "Activo", ...seated(null) }), DELEGATE); + await openMenu(); + expect(screen.getByText("Invitar a la app")).toBeInTheDocument(); + }); + + it("still shows it to an Admin in every one of those cases", async () => { + for (const m of [ + member({ status: "Activo", uid: "u1" }), + member({ status: "Activo", ...seated("pos-power") }), + member({ status: "Activo", ...seated("") }), + member({ status: "Activo", roleIds: ["custom-role"] }), + ]) { + const { unmount } = renderMenu(m, ADMIN); + await openMenu(); + expect( + screen.getByText(m.uid ? "Reenviar invitación" : "Invitar a la app"), + ).toBeInTheDocument(); + unmount(); + } + }); }); diff --git a/apps/backstage/src/features/members/components/member-row-menu.tsx b/apps/backstage/src/features/members/components/member-row-menu.tsx index 03d9a6f9..e8f7b76b 100644 --- a/apps/backstage/src/features/members/components/member-row-menu.tsx +++ b/apps/backstage/src/features/members/components/member-row-menu.tsx @@ -1,11 +1,13 @@ import { Menu, MenuItem, MenuSeparator } from "@luminova/ui"; -import type { Member, MemberStatus } from "@luminova/types"; +import type { Member, MemberStatus, Position } from "@luminova/types"; import { Can } from "../../../lib/authz/ability-context"; import { ActionGate } from "../../../lib/authz/action-gate"; import { useCan } from "../../../lib/authz/use-can"; +import { memberProvisionBlocked } from "../lib/provision-gate"; interface MemberRowMenuProps { member: Member; + positionsById: ReadonlyMap; onView: (member: Member) => void; onEdit: (member: Member) => void; onProvision: (member: Member) => void; @@ -15,6 +17,7 @@ interface MemberRowMenuProps { export function MemberRowMenu({ member, + positionsById, onView, onEdit, onProvision, @@ -22,6 +25,7 @@ export function MemberRowMenu({ onUnpublish, }: MemberRowMenuProps) { const { canProvisionLogin, isAdmin } = useCan(); + const provisionBlocked = memberProvisionBlocked(member, (id) => positionsById.get(id), isAdmin); return ( {/* provisionMemberLogin is requireAdminOrPerm(create:MemberLogin) — the Admin role or - that exact code, never the manage:all perm. The `!member.uid` half is the adoption - guard mirrored: a delegate may only mint a NEW login, so offering "Reenviar" to one - would be an item that 403s on every click. */} - + that exact code, never the manage:all perm. memberProvisionBlocked mirrors every + refusal the callable applies to a non-Admin (adoption, direct grants, a power-granting + cargo in any term), so a delegate is not offered an item that 403s on every click. + The residual case the client cannot see — an Auth account already existing for the + address — still 403s, and provisionErrorMessage names it. */} + onProvision(member)}> {member.uid ? "Reenviar invitación" : "Invitar a la app"} diff --git a/apps/backstage/src/features/members/components/member-table.tsx b/apps/backstage/src/features/members/components/member-table.tsx index f9602355..ebcc21b3 100644 --- a/apps/backstage/src/features/members/components/member-table.tsx +++ b/apps/backstage/src/features/members/components/member-table.tsx @@ -169,6 +169,7 @@ export function MemberTable({ ({ vi.mock("../hooks/use-provision-member-login", () => ({ useProvisionMemberLogin: () => ({ mutateAsync: vi.fn(), mutate: vi.fn(), isPending: false }), })); +// MemberDrawer (rendered from this page) now reads the caller's uid to decide whether the row +// it opened is the caller's OWN — the table lists it too, so that edit is a SELF-assignment. +// Mocked as a factory with no `importOriginal`: lib/auth/auth builds its store from +// getFirebase().auth at module scope, so evaluating the real module initializes Firebase and +// the whole file fails to collect. +vi.mock("../../../lib/auth/auth", () => ({ + useAuth: () => ({ user: { uid: "u" }, claims: { roles: ["Admin"] } }), +})); import { MembersPage } from "./members-page"; import { AbilityProvider } from "../../../lib/authz/ability-context"; diff --git a/apps/backstage/src/features/members/components/members-page.tsx b/apps/backstage/src/features/members/components/members-page.tsx index 8de299bc..ad55d8a5 100644 --- a/apps/backstage/src/features/members/components/members-page.tsx +++ b/apps/backstage/src/features/members/components/members-page.tsx @@ -9,7 +9,6 @@ import { useSetMemberStatus } from "../hooks/use-set-member-status"; import { useUnpublishMember } from "../hooks/use-unpublish-member"; import { useProvisionMemberLogin } from "../hooks/use-provision-member-login"; import { provisionErrorMessage } from "../lib/provision-error"; -import { requestPasswordReset } from "../../../lib/auth/request-password-reset"; import { MemberTable } from "./member-table"; import { MemberStatusFilter } from "./member-status-filter"; import { MemberFilterMeta } from "./member-filter-meta"; @@ -34,7 +33,12 @@ const NO_MEMBERS: Member[] = []; export function MembersPage() { const { data: members, isLoading, isError } = useMembers(); - const { data: positions } = usePositions(); + // isError, not just data: the row menu's "Invitar a la app" is gated on + // `memberProvisionBlocked`, which fails CLOSED on an unresolvable cargo — so a failed catalog + // query removes the affordance from every seated member's menu with nothing said, and the + // invite drawer's picker renders "ningún cargo es asignable con tus permisos", a permissions + // explanation for a failed query. Guardrail #3. + const { data: positions, isError: positionsFailed } = usePositions(); const addMember = useAddMember(); const updateMember = useUpdateMember(); const setMemberStatus = useSetMemberStatus(); @@ -92,13 +96,12 @@ export function MembersPage() { const handleProvision = async (member: Member) => { if (provision.isPending) return; try { - const { email } = await provision.mutateAsync(member.id); - try { - await requestPasswordReset(email); - setToast(actionMessage(member.name, "invited")); - } catch { - setToast("Acceso creado, pero el correo no se envió."); - } + const { emailSent } = await provision.mutateAsync(member.id); + setToast( + emailSent + ? actionMessage(member.name, "invited") + : "Acceso creado, pero el correo no se envió.", + ); } catch (err) { setToast(provisionErrorMessage(err, "No se pudo enviar la invitación.")); } @@ -182,6 +185,13 @@ export function MembersPage() { onClearAll={clearAll} /> + {positionsFailed && ( +

+ No se pudo cargar el catálogo de cargos. Los cargos no se muestran y las invitaciones no + están disponibles hasta que recargues la página. +

+ )} + {isError ? (

No se pudieron cargar los miembros. diff --git a/apps/backstage/src/features/members/components/no-assignable-cargos-note.test.tsx b/apps/backstage/src/features/members/components/no-assignable-cargos-note.test.tsx new file mode 100644 index 00000000..d5d89a39 --- /dev/null +++ b/apps/backstage/src/features/members/components/no-assignable-cargos-note.test.tsx @@ -0,0 +1,161 @@ +import { describe, expect, it, vi } from "vitest"; +import { render } from "@testing-library/react"; +import type { Position } from "@luminova/types"; +import { cargoNoteIds } from "./no-assignable-cargos-note"; +import { MemberForm } from "./member-form"; +import { MemberPositionsForm } from "./member-positions-form"; + +// A catalog whose only cargo confers a role. Two states fall out of it and they are the two +// this file needs: +// cargoId null → every option filtered away for a non-delegate → the noCargos note, +// which is a SHARED component (both forms render the same element). +// cargoId set → positionsLockedForEditor → the locked note, which each form renders +// itself with its own wording. +const POWER_CARGO: Position = { + id: "pos-power", + title: "Secretario", + titleFemale: "Secretaria", + category: "CEL", + grants: ["Secretary"], + term: null, + sigla: null, + description: "", + active: true, + deletedAt: null, +}; + +/** + * The cargo Combobox's trigger, scoped to ONE form's container. + * + * Selected structurally rather than by id or label, because BOTH forms give their trigger the + * same `id="cargoId"` (each pairs with its own `

+

Ningún cargo del catálogo es asignable con tus permisos. Los cargos del Comité Ejecutivo Local - y los que otorgan permisos requieren un Admin o el permiso «Editar Asientos de directiva». + y los que otorgan permisos requieren un administrador o el permiso « + {permissionLabel("update:BoardSeat")}». +

+ ); +} + +/** Why a cargo this editor CAN assign will not actually confer its permissions — see + * `cargoGrantNeedsAdminAssigner`. Shared by both forms for the same reason as the note above: + * the wording does not differ between them, so a second copy would only drift. */ +export function MintPendingNote({ id }: { id: string }) { + return ( +

+ Este cargo otorga permisos, pero no se aplicarán hasta que un administrador confirme la + asignación. El cargo sí queda registrado y visible.

); } diff --git a/apps/backstage/src/features/members/hooks/use-provision-member-login.test.tsx b/apps/backstage/src/features/members/hooks/use-provision-member-login.test.tsx new file mode 100644 index 00000000..4a38268b --- /dev/null +++ b/apps/backstage/src/features/members/hooks/use-provision-member-login.test.tsx @@ -0,0 +1,107 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import type { ReactNode } from "react"; +import { renderHook, waitFor } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +const callable = vi.fn(); +vi.mock("firebase/functions", () => ({ httpsCallable: () => callable })); +vi.mock("@luminova/firebase/functions", () => ({ getFunctionsService: () => ({}) })); +vi.mock("../../../lib/auth/request-password-reset", () => ({ + requestPasswordReset: vi.fn().mockResolvedValue(undefined), +})); + +import { requestPasswordReset } from "../../../lib/auth/request-password-reset"; +import { useProvisionMemberLogin } from "./use-provision-member-login"; + +const mockedReset = vi.mocked(requestPasswordReset); + +function wrapper(client: QueryClient) { + return ({ children }: { children: ReactNode }) => ( + {children} + ); +} + +function setup() { + const client = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }); + const invalidate = vi.spyOn(client, "invalidateQueries"); + const hook = renderHook(() => useProvisionMemberLogin(), { wrapper: wrapper(client) }); + return { ...hook, client, invalidate }; +} + +describe("useProvisionMemberLogin", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockedReset.mockResolvedValue(undefined); + callable.mockResolvedValue({ + data: { email: "ana@jci.bo", actionLink: "https://example.com/link" }, + }); + }); + + it("provisions, then mails the member — and withholds the link that mail invalidated", async () => { + const { result } = setup(); + const invite = await result.current.mutateAsync("m1"); + expect(callable).toHaveBeenCalledWith({ memberId: "m1" }); + expect(mockedReset).toHaveBeenCalledWith("ana@jci.bo"); + // Firebase keeps only the most recent password-reset oobCode valid, so the mail above + // killed `actionLink`. Offering it as "por si no le llega" would hand the operator a link + // that fails with auth/invalid-action-code. + expect(invite).toEqual({ + email: "ana@jci.bo", + emailSent: true, + fallbackLink: null, + mailError: null, + }); + }); + + it("surfaces the link only when the mail did NOT go out", async () => { + mockedReset.mockRejectedValue(new Error("network error")); + const { result } = setup(); + const invite = await result.current.mutateAsync("m1"); + expect(invite.emailSent).toBe(false); + expect(invite.fallbackLink).toBe("https://example.com/link"); + expect(invite.mailError).toBe("network error"); + }); + + it("does not reject when only the mail fails: the account exists and the uid is linked", async () => { + mockedReset.mockRejectedValue(new Error("network error")); + const { result } = setup(); + await expect(result.current.mutateAsync("m1")).resolves.toBeDefined(); + }); + + // BLOCKING — the regression this hook was restructured for. The mail used to be sent from a + // component-scoped `provision.mutate(id, { onSuccess })`, and TanStack Query v5 runs those + // callbacks only while the observer still `hasListeners()`. An operator who navigated away + // (or, on the profile page, merely switched members — InviteAccess is keyed by member id) + // got the Auth account created and the uid linked with NO mail ever sent and no error + // anywhere. `mutationFn` has no such condition. + it("BLOCKING: sends the mail even when the caller unmounts before the callable resolves", async () => { + let resolveCallable: (v: unknown) => void = () => {}; + callable.mockReturnValue( + new Promise((resolve) => { + resolveCallable = resolve; + }), + ); + const { result, unmount } = setup(); + result.current.mutate("m1"); + unmount(); + resolveCallable({ data: { email: "ana@jci.bo", actionLink: "https://example.com/link" } }); + await waitFor(() => expect(mockedReset).toHaveBeenCalledWith("ana@jci.bo")); + }); + + // beacon writes members/{id}.uid. Without this the cached member keeps `uid: undefined` for + // the 5-minute default staleTime, so the invite button neither disappears nor relabels and a + // second click 403s on the adoption guard. + it("BLOCKING: invalidates the members cache, including after a failure", async () => { + const { result, invalidate } = setup(); + await result.current.mutateAsync("m1"); + expect(invalidate).toHaveBeenCalledWith({ queryKey: ["members"] }); + + invalidate.mockClear(); + callable.mockRejectedValue(new Error("boom")); + const second = setup(); + await expect(second.result.current.mutateAsync("m2")).rejects.toThrow("boom"); + expect(second.invalidate).toHaveBeenCalledWith({ queryKey: ["members"] }); + }); +}); diff --git a/apps/backstage/src/features/members/hooks/use-provision-member-login.ts b/apps/backstage/src/features/members/hooks/use-provision-member-login.ts index 0b21bcbc..350934a8 100644 --- a/apps/backstage/src/features/members/hooks/use-provision-member-login.ts +++ b/apps/backstage/src/features/members/hooks/use-provision-member-login.ts @@ -1,20 +1,76 @@ -import { useMutation } from "@tanstack/react-query"; +import { useMutation, useQueryClient } from "@tanstack/react-query"; import { httpsCallable } from "firebase/functions"; import { getFunctionsService } from "@luminova/firebase/functions"; +import { requestPasswordReset } from "../../../lib/auth/request-password-reset"; +import { memberKeys } from "./member-keys"; interface ProvisionResult { email: string; actionLink: string; } +/** What an invite actually produced. The MAIL is part of it, not a follow-up the caller + * arranges: every new login is delivered by `sendPasswordResetEmail`, board seat or not, Admin + * caller or delegate. */ +export interface InviteResult { + email: string; + emailSent: boolean; + /** The action link, and ONLY when the mail did not go out. + * + * It is `generatePasswordResetLink`'s oobCode URL, and Firebase keeps just the most recent + * password-reset code valid per user — so the mail this hook sends right after INVALIDATES + * it. Offering it as "por si no le llega el correo" alongside a mail that did go out hands + * the operator a link that fails with `auth/invalid-action-code`. Nulled here rather than at + * each call site: the three surfaces that render it cannot each be trusted to re-derive + * which of two secrets is the live one. */ + fallbackLink: string | null; + /** The mail failure's raw message — the only diagnostic for App Check / quota / config. */ + mailError: string | null; +} + +/** + * Provision a member's login AND deliver it. Both steps live in `mutationFn` on purpose. + * + * The mail used to be sent from a component-scoped `provision.mutate(id, { onSuccess })` + * callback. TanStack Query v5 runs those only `if (this.#mutateOptions && this.hasListeners())` + * (query-core `mutationObserver`), so an operator who navigated away — or, on the profile page, + * merely switched to another member, since `InviteAccess` is keyed by member id — got the Auth + * account created, the uid linked, and NO mail ever sent, with no error anywhere. The member + * then has a login they were never told about, and `memberProvisionBlocked` (hasLogin) hides + * the retry from the delegate who caused it. + * + * `mutationFn` has no such condition: it runs to completion regardless of who is still + * mounted. + */ export function useProvisionMemberLogin() { + const queryClient = useQueryClient(); return useMutation({ - mutationFn: async (memberId: string) => { + mutationFn: async (memberId: string): Promise => { const fn = httpsCallable<{ memberId: string }, ProvisionResult>( getFunctionsService(), "provisionMemberLogin", ); - return (await fn({ memberId })).data; + const { email, actionLink } = (await fn({ memberId })).data; + // A mail failure is NOT a provisioning failure: the account exists and the uid is + // linked, and rejecting here would read as "nothing happened" and invite a retry the + // adoption guard refuses. + try { + await requestPasswordReset(email); + return { email, emailSent: true, fallbackLink: null, mailError: null }; + } catch (err) { + console.error("No se pudo enviar el correo de acceso", err); + return { + email, + emailSent: false, + fallbackLink: actionLink || null, + mailError: err instanceof Error ? err.message : String(err), + }; + } }, + // beacon writes `members/{id}.uid`; without this the cached member keeps `uid: undefined` + // for the 5-minute default staleTime, so the button neither disappears nor relabels and a + // second click 403s on the adoption guard. `settled`, not `success`: the callable can fail + // after linkUid. memberKeys.all is a prefix of memberKeys.detail, so one call covers both. + onSettled: () => queryClient.invalidateQueries({ queryKey: memberKeys.all }), }); } diff --git a/apps/backstage/src/features/members/lib/assignable-cargo-core.ts b/apps/backstage/src/features/members/lib/assignable-cargo-core.ts new file mode 100644 index 00000000..b5103183 --- /dev/null +++ b/apps/backstage/src/features/members/lib/assignable-cargo-core.ts @@ -0,0 +1,258 @@ +import type { PositionCategory } from "@luminova/types"; + +/** + * The rules-mirroring half of `assignable-cargo.ts`: every predicate that answers a + * `firestore.rules` question about a cargo, and nothing that renders one. + * + * Split out for ONE reason, and it is load-bearing: `tests/firestore-rules/` drives these + * predicates and the real emulator from the same fixture + * (`cargo-assignment-parity.test.ts`), and that package cannot resolve `@luminova/types` at + * RUNTIME — it has no such dependency. `assignable-cargo.ts` imports `currentTermKey` and + * `positionTitle` from it for VALUE, so importing that file there throws. This module has no + * runtime imports; the one `import type` above is erased before it can be resolved, which is + * the same trick `nav-equivalence.test.ts` documents for `nav-config.ts`'s `IconKey`. + * + * Import `positionsLockedForEditor` / `cargoTakedownOnly` from HERE, not through + * `assignable-cargo.ts` — it used to re-export them, which read as a convenience and was + * really a barrel that erased the one distinction this split exists to make legible: a + * predicate's file says whether the emulator parity test holds it to `firestore.rules`. + * `cargoSlotsForEditor` is the exception, wrapped by `cargoOptionsForEditor` because the + * labelling needs `positionTitle`. + * + * What deliberately did NOT move: the render states (`noAssignableCargos`, `cargoNoteId`), the + * labelling itself, and `cargoGrantNeedsAdminAssigner` — which mirrors BEACON's trust gate, + * not a rules predicate, so no rules parity test can hold it and it has no business here. + */ + +/** + * The cargo fields these predicates read — structural rather than `Pick`, so a + * caller may pass anything cargo-shaped, and `cargoSlotsForEditor` (generic in `P`) hands back + * the very objects it was given rather than a lossy copy. + * + * `category` carries `Position`'s real union, NOT a widened `string`. That matters more than it + * looks: `cargoAssignableByNonAdmin` compares it against the literal `"CEL"`, and against a + * `string` a rename in POSITION_CATEGORIES would quietly make that comparison always-true — + * offering every CEL cargo to a non-delegate while the rules kept denying, on the publication + * boundary this module exists to mirror. Neither the compiler nor the parity test would catch + * that: the same fixture feeds the predicate and the emulator, so both sides would agree, + * wrongly and green. The `import type` costs nothing — it is erased before the rules-test + * package could fail to resolve it. + */ +export interface CargoLike { + id: string; + grants: readonly string[]; + category: PositionCategory; + term: number | null; + active: boolean; +} + +/** + * Client mirror of firestore.rules `cargoAssignableByNonAdmin()` — the two questions a + * non-Admin assignment must answer about the cargo being written in: + * grants.length === 0 the claims-mint boundary (assigning it would mint custom claims). + * category !== "CEL" the publication boundary. boardGroupFromCategory publishes CEL and + * JDL alike and boardRank puts 'Presidente' at rank 0, so a + * grant-free CEL cargo seats its holder at the head of the + * world-readable Directiva. JDL direcciones stay assignable — that + * is the accepted exposure this lane exists to deliver. + * + * The rules apply it on BOTH member lanes (`createPositionsSafe` and + * `positionsAssignmentSafe`), so both member forms apply it here. One function, not a + * `grants.length === 0` re-typed per form: rendering an option the save will 403 on is the + * render-then-die shape this repo guards against, and two copies drift. + */ +// Module-local: every consumer goes through cargoOptionsForEditor() / cargoTakedownOnly() / +// positionsLockedForEditor(), which is the point — a caller that re-derived the option list +// from this raw predicate is how the two forms drifted apart in the first place. Exporting it +// again would give that back. +function cargoAssignableByNonAdmin(cargo: Pick): boolean { + return cargo.grants.length === 0 && cargo.category !== "CEL"; +} + +/** The raw shape of the rules' OLD-side question, with no permission flag folded in. Every + * caller goes through `positionsLockedForEditor` / `cargoTakedownOnly` / + * `cargoGrantNeedsAdminAssigner`, which each fold in the flag the rules actually gate that + * side on — and those flags are NOT all the same one. + * + * Exported ONLY so `cargoGrantNeedsAdminAssigner` can keep living next to the render states + * in `assignable-cargo.ts` (it mirrors beacon's trust gate, not a rules predicate, so it has + * no business here). Do not derive an option list from it: a call site that re-derives + * "assignable" from the raw predicates is how the two member forms drifted apart. */ +export function cargoConfersPower(cargo: Pick | undefined): boolean { + return cargo !== undefined && cargo.grants.length > 0; +} + +/** + * The cargo a member currently holds, resolved against the catalog — and, distinctly, whether + * they hold one AT ALL. Those are two different questions and collapsing them to a bare + * `Position | undefined` is what made the lock fail OPEN: an id that does not resolve read as + * "no cargo", so `cargoConfersPower(undefined)` was false and the slot unlocked. + * + * That is reachable without a console edit. The catalog is `parseDocs(positionDocSchema, …)`, + * which DROPS any doc failing the schema — a `grants` entry outside `ROLES`, a bad `category` — + * so the very corruption that makes a cargo's power unknowable is what removes it from this + * array. The rules have no such gap: `currentCargoGrantsEmpty()` `get()`s the real doc, and a + * missing one errors the rule, which denies. + * + * Built here rather than at each call site because both forms need it and `positions.find(…)` + * repeated per form is the shape these predicates already drifted through once. + */ +export interface HeldCargo

{ + /** What the member doc says, verbatim. `""` is NOT "no cargo" — it is an unresolvable id. */ + cargoId: string | null | undefined; + /** The catalog entry, or `undefined` when the id resolves to nothing. */ + cargo: P | undefined; +} + +export function heldCargo

( + positions: readonly P[], + cargoId: string | null | undefined, +): HeldCargo

{ + return { + cargoId, + cargo: + cargoId === null || cargoId === undefined + ? undefined + : positions.find((p) => p.id === cargoId), + }; +} + +/** Whether the member is seated on SOMETHING whose power this editor cannot establish. + * + * KNOWN and accepted: "unresolvable" here means ABSENT FROM THE PARSED CATALOG, which is a + * slightly wider net than the rules cast. `parseDocs(positionDocSchema, …)` drops a doc for any + * schema violation, so a cargo that exists with `grants: []` but a missing `description` or an + * off-enum `category` is dropped here while `currentCargoGrantsEmpty()` reads `grants.size() == 0` + * and would ALLOW a non-Admin to clear it. The editor is then locked out of a takedown the rules + * keep open. Availability only, it needs a malformed catalog doc, and an Admin is not stuck — + * though the remedy is REASSIGNING the seat, not clearing it: an unresolvable cargo is absent + * from the option list, and Combobox clears by re-selecting the selected option, which has to + * exist. Resolving the held cargo from the raw snapshot instead would close the gap and cost + * more than it buys: it means reading positions around the schema that exists to keep + * unvalidated data out of the client. + * + * The corruption that actually matters — a NON-EMPTY `grants` carrying an unknown role — locks + * on both sides, since it fails `size() == 0` too. Not every malformed `grants` does: CEL + * defines `.size()` on strings and maps, so a stored `grants: ""` or `grants: {}` reads as + * empty to the rules and lands back in the availability-only bucket above. */ +function heldCargoUnresolvable(held: HeldCargo): boolean { + return held.cargoId !== null && held.cargoId !== undefined && held.cargo === undefined; +} + +/** + * Whether this editor is barred from touching the positions slot AT ALL, given the cargo the + * member currently holds. NOT the negation of `cargoAssignableByNonAdmin` — the two rules + * conjuncts are asymmetric, and mirroring the wrong one strands a takedown: + * + * grants.length > 0 → locked. `currentCargoGrantsEmpty()` gates the cargo being REPLACED, + * so the editor can neither keep it (the save re-stamps it) nor clear + * it. Nothing they can do here succeeds. + * unresolvable id → locked, for the same reason and by the same authority: the rules + * `get()` the doc and deny on a missing one, so an editor who cannot + * read the cargo cannot submit anything either. See HeldCargo. + * grant-free CEL → NOT locked. `currentCargoGrantsEmpty()` is deliberately not + * category-gated — firestore.rules says denying this "would strand a + * takedown behind an Admin" — so clearing the seat is allowed even + * though keeping it is not. The cargo is dropped from the options + * instead, which makes the only submittable states "clear" or "some + * other assignable cargo" — exactly the rules' answer. + * + * `allowReplacePowerCargo` is a PARAMETER, and it is deliberately not the same flag as + * `allowPowerGrants`. positionsAssignmentSafe() gates its two cargo conjuncts on different + * principals, and this function mirrors the OLD one: + * + * NEW side (cargoAssignableByNonAdmin, the cargo written IN) → `allowPowerGrants`, + * which update:BoardSeat lifts. + * OLD side (currentCargoGrantsEmpty, the cargo REPLACED) → Admin ROLE only, never + * delegated. + * + * The flag is taken here rather than `&&`-ed at each call site because that is exactly how the + * two drifted: both forms typed `!allowPowerGrants && positionsLockedForNonAdmin(...)`, which + * was correct only while `allowPowerGrants` MEANT `isAdmin`. Widening it to include the + * delegate silently unlocked the editor for a write the rules always deny. + */ +export function positionsLockedForEditor( + held: HeldCargo>, + allowReplacePowerCargo: boolean, +): boolean { + if (allowReplacePowerCargo) return false; + return cargoConfersPower(held.cargo) || heldCargoUnresolvable(held); +} + +/** + * The takedown-only state: the member is seated on a cargo this editor may NOT keep but MAY + * clear — a grant-free CEL seat for a non-Admin. It is the one state where the honest render + * is neither "pick anything" nor "locked": + * - the seat must still be VISIBLE (the holder holds it), so it is offered as a disabled + * option and the Combobox trigger shows its title instead of the "Sin cargo" placeholder; + * - the seat must not be re-submittable (the rules 403 any positions write that keeps it); + * - clearing it must be reachable, which a disabled option cannot do on its own — Combobox + * clears by re-selecting the SELECTED option, and a disabled item swallows the select. So + * the forms render an explicit "Quitar cargo" action while this is true. + * Takes the CURRENT selection, not the stored one: once cleared or switched away the state is + * over, and the takedown affordance goes with it. + */ +export function cargoTakedownOnly( + cargo: Pick | undefined, + allowPowerGrants: boolean, +): boolean { + return ( + !allowPowerGrants && + cargo !== undefined && + !cargoAssignableByNonAdmin(cargo) && + !cargoConfersPower(cargo) + ); +} + +/** One row of the cargo Combobox, before labelling. `retired` and `disabled` are the two + * things the label/render layer needs and the predicates above decide. */ +export interface CargoSlot

{ + position: P; + /** Not assignable any more: deactivated, or belonging to a past term. Labelled "(inactivo)". */ + retired: boolean; + /** The rules would 403 a write keeping this cargo — shown for truth, not submittable. */ + disabled: boolean; +} + +/** + * WHICH cargos this editor may pick, and which of them are shown-but-denied. The whole of + * `cargoOptionsForEditor` except the labels, so the parity test can drive the real rules + * predicate without `positionTitle` (see the header). + * + * Assignable cargos of `term`, plus the HELD cargo when it is not among them — appended + * `disabled` when this editor may not assign it, so the trigger tells the truth about who holds + * what without making a denied write one click away. Keyed on the stored assignment, never the + * live selection: an option list that reacts to the selection deletes the entry the user just + * switched away from, so switching back is impossible. + */ +export function cargoSlotsForEditor

({ + positions, + allowPowerGrants, + assignedCargoId, + term, +}: { + positions: readonly P[]; + allowPowerGrants: boolean; + assignedCargoId: string | null | undefined; + term: string; +}): CargoSlot

[] { + const currentTerm = (p: P) => p.term === null || String(p.term) === term; + const slots = positions + .filter((p) => p.active && p.category !== "Comision" && currentTerm(p)) + .filter((p) => allowPowerGrants || cargoAssignableByNonAdmin(p)) + .map((p) => ({ position: p, retired: false, disabled: false })); + + // heldCargo(), not `assignedCargoId ? find(…)`: a truthiness test reads `""` as "no cargo", + // and `""` is exactly the shape termPositionsDocSchema admits and beacon's readCargoIds + // manufactures on purpose so the guard refuses it. + const held = heldCargo(positions, assignedCargoId).cargo; + if (held === undefined || slots.some((s) => s.position.id === held.id)) return slots; + return [ + ...slots, + { + position: held, + retired: !held.active || !currentTerm(held), + disabled: !allowPowerGrants && !cargoAssignableByNonAdmin(held), + }, + ]; +} diff --git a/apps/backstage/src/features/members/lib/assignable-cargo.test.ts b/apps/backstage/src/features/members/lib/assignable-cargo.test.ts new file mode 100644 index 00000000..66d34df6 --- /dev/null +++ b/apps/backstage/src/features/members/lib/assignable-cargo.test.ts @@ -0,0 +1,424 @@ +import { describe, expect, it } from "vitest"; +import type { Position } from "@luminova/types"; +import { + cargoGrantNeedsAdminAssigner, + cargoNoteId, + cargoOptionsForEditor, + noAssignableCargos, + type CargoOption, +} from "./assignable-cargo"; +// From the rules-mirroring module directly. This test file covers BOTH halves, and which +// import line a predicate is on is the only thing that says whether the emulator parity test +// (tests/firestore-rules/cargo-assignment-parity.test.ts) also holds it to firestore.rules. +import { cargoTakedownOnly, positionsLockedForEditor } from "./assignable-cargo-core"; + +const cargo = ( + category: Position["category"], + grants: Position["grants"] = [], +): Pick => ({ category, grants }); + +const POWER = cargo("CEL", ["Secretary"]); +const CEL_FREE = cargo("CEL"); +const JDL_FREE = cargo("JDL"); + +/** A full catalog entry for the one assertion that has to go through the option list. */ +const POWER_POSITION: Position = { + id: "pos-power", + title: "Secretario", + titleFemale: "Secretaria", + category: "CEL", + grants: ["Secretary"], + term: null, + sigla: null, + description: "", + active: true, + deletedAt: null, +}; + +// positionsLockedForEditor mirrors the OLD side of firestore.rules positionsAssignmentSafe() +// (`currentCargoGrantsEmpty`, the cargo being REPLACED), which is Admin-ROLE only and is +// deliberately NOT lifted by update:BoardSeat. Its flag is therefore NOT `allowPowerGrants`. +// The full truth table lives here because that asymmetry is now the load-bearing one: the +// forms exercise the two diagonal cells, and the delegate cell is the one the old +// `!allowPowerGrants && …` call sites got wrong. +describe("positionsLockedForEditor", () => { + /** A seated member, id resolved against a one-cargo catalog. */ + const seated = (c: Pick) => ({ cargoId: "held", cargo: c }); + + it("locks a power-granting cargo for anyone who may not replace one", () => { + expect(positionsLockedForEditor(seated(POWER), false)).toBe(true); + }); + + it("does not lock a power-granting cargo for an Admin", () => { + expect(positionsLockedForEditor(seated(POWER), true)).toBe(false); + }); + + it("never locks a grant-free cargo, CEL or JDL, either way", () => { + // The asymmetric case: keeping a grant-free CEL seat is denied but CLEARING it is allowed + // on purpose, so locking here would strand the takedown behind an Admin. + for (const flag of [true, false]) { + expect(positionsLockedForEditor(seated(CEL_FREE), flag)).toBe(false); + expect(positionsLockedForEditor(seated(JDL_FREE), flag)).toBe(false); + } + }); + + it("never locks when there is no assigned cargo", () => { + expect(positionsLockedForEditor({ cargoId: null, cargo: undefined }, false)).toBe(false); + expect(positionsLockedForEditor({ cargoId: undefined, cargo: undefined }, true)).toBe(false); + }); + + // Fails CLOSED, and the direction is the whole point: `cargoConfersPower(undefined)` is + // false, so a seat whose id resolves to nothing used to read as "no cargo" and unlock the + // picker for a write firestore.rules always denies (`currentCargoGrantsEmpty()` get()s the + // real doc and a missing one errors the rule). Reachable without a console edit — the + // catalog is parseDocs(), which DROPS a position whose `grants`/`category` fails the schema, + // so the very corruption that hides the cargo's power is what removes it from the array. + it("BLOCKING: locks a seat whose cargo id does not resolve", () => { + expect(positionsLockedForEditor({ cargoId: "ghost", cargo: undefined }, false)).toBe(true); + // "" is an id, not "no cargo" — the shape termPositionsDocSchema admits and beacon's + // readCargoIds manufactures on purpose so its own guard refuses it. + expect(positionsLockedForEditor({ cargoId: "", cargo: undefined }, false)).toBe(true); + // An Admin may replace it, so the lock lifts for them exactly as it does for a real seat. + expect(positionsLockedForEditor({ cargoId: "ghost", cargo: undefined }, true)).toBe(false); + }); + + // The #224 regression, and NOT just "false locks / true does not" restated — that pair is + // already the two tests above and would stay green with both flags collapsed into one. What + // makes this load-bearing is the third assertion: for the SAME principal the two flags must + // disagree, so a delegate may ASSIGN a power cargo (the NEW side, which update:BoardSeat + // lifts) while still being locked out of REPLACING the one a member already holds (the OLD + // side, Admin-role only). Collapse them either way and one of these three goes red. + it("BLOCKING: a delegate may assign a power cargo but not replace one", () => { + const delegate = { allowPowerGrants: true, allowReplacePowerCargo: false }; + expect(positionsLockedForEditor(seated(POWER), delegate.allowReplacePowerCargo)).toBe(true); + // The counterfactual: wiring the NEW-side flag in here is what unlocked the editor. + expect(positionsLockedForEditor(seated(POWER), delegate.allowPowerGrants)).toBe(false); + const offered = cargoOptionsForEditor({ + positions: [POWER_POSITION], + gender: "Masculino", + allowPowerGrants: delegate.allowPowerGrants, + assignedCargoId: null, + }); + expect(offered.map((o) => o.value)).toContain(POWER_POSITION.id); + }); +}); + +// The OTHER rules-mirroring predicate, and the one state where the client is stricter on one +// side and not the other: a non-delegate may not KEEP a grant-free CEL seat but must be able to +// CLEAR it. Its truth table was covered only by the parity test's single row, and the one +// assertion this file had for it was decided by the cargo alone. +describe("cargoTakedownOnly", () => { + it("is true only for a grant-free CEL seat held by a non-delegate", () => { + expect(cargoTakedownOnly(CEL_FREE, false)).toBe(true); + }); + + it("is false for a delegate: they may keep the seat, so there is nothing to take down", () => { + expect(cargoTakedownOnly(CEL_FREE, true)).toBe(false); + }); + + it("is false for a cargo the non-delegate may simply assign", () => { + expect(cargoTakedownOnly(JDL_FREE, false)).toBe(false); + }); + + it("is false for a power-granting cargo — that is `locked`, not takedown-only", () => { + // The two states are mutually exclusive on purpose: a power seat cannot be cleared either + // (currentCargoGrantsEmpty gates the OLD side), so offering "Quitar cargo" would promise a + // write the rules deny. + expect(cargoTakedownOnly(POWER, false)).toBe(false); + expect(cargoTakedownOnly(cargo("JDL", ["Secretary"]), false)).toBe(false); + }); + + it("is false when nothing is selected", () => { + expect(cargoTakedownOnly(undefined, false)).toBe(false); + }); +}); + +// The third derived render-state. Two of its three clauses are unreachable through the form +// tests, so each is pinned as an explicit row rather than inferred from a rendered note. +describe("noAssignableCargos", () => { + const OPTION: CargoOption = { value: "dir", label: "Director" }; + + it("is false for a delegate: they may assign, the ceiling filtered nothing", () => { + expect(noAssignableCargos({ cargoOptions: [], allowPowerGrants: true, locked: false })).toBe( + false, + ); + }); + + it("is false when locked: the locked note is the honest explanation, not this one", () => { + expect(noAssignableCargos({ cargoOptions: [], allowPowerGrants: false, locked: true })).toBe( + false, + ); + }); + + it("is true for a non-delegate whose option list came back empty", () => { + expect(noAssignableCargos({ cargoOptions: [], allowPowerGrants: false, locked: false })).toBe( + true, + ); + }); + + it("is false whenever there is anything to pick", () => { + expect( + noAssignableCargos({ cargoOptions: [OPTION], allowPowerGrants: false, locked: false }), + ).toBe(false); + expect( + noAssignableCargos({ cargoOptions: [OPTION], allowPowerGrants: true, locked: false }), + ).toBe(false); + }); + + // The `!locked` clause carries the two locked states differently, and BOTH are asserted + // below because only one of them is redundant. + // + // held cargo RESOLVES and confers power — `cargoOptionsForEditor` appends it as a disabled + // option, so the length clause alone already returns false. `!locked` is defensive here, + // and this pins the coincidence that makes it so rather than deleting a clause whose + // redundancy depends on two other functions agreeing. + // held cargo does NOT resolve — nothing is appended, so on a catalog with no assignable + // cargo the list really is empty while `locked` is true. `!locked` is the ONLY thing + // keeping the empty-catalog note ("ningún cargo es asignable con tus permisos", a + // permissions explanation) from rendering under a slot that is locked for a different + // reason. Not defensive at all since positionsLockedForEditor started failing closed. + it("BLOCKING: !locked suppresses the empty-catalog note for both locked states", () => { + const unresolved = { cargoId: "ghost", cargo: undefined }; + expect(positionsLockedForEditor(unresolved, false)).toBe(true); + expect(noAssignableCargos({ cargoOptions: [], allowPowerGrants: false, locked: true })).toBe( + false, + ); + + const held = POWER_POSITION; + const locked = positionsLockedForEditor({ cargoId: held.id, cargo: held }, false); + expect(locked).toBe(true); + const cargoOptions = cargoOptionsForEditor({ + positions: [held], + gender: "Masculino", + allowPowerGrants: false, + assignedCargoId: held.id, + }); + expect(cargoOptions).toHaveLength(1); + expect(cargoOptions[0]).toMatchObject({ value: "pos-power", disabled: true }); + expect(noAssignableCargos({ cargoOptions, allowPowerGrants: false, locked })).toBe(false); + }); +}); + +// The one outcome in this lane that fails SILENTLY. boardSeatDelegate() lets a delegate write +// a vacant power cargo and the write succeeds — but resolveTrustedGrants refuses to mint on +// EITHER of two disjoint conditions, so the seat publishes and nothing says the claim is +// missing. Keyed on the Admin ROLE (beacon's `assignerIsAdmin`), NOT on allowPowerGrants: the +// perm lifts the WRITE, never the MINT. +describe("cargoGrantNeedsAdminAssigner", () => { + const ADMIN_CARGO = cargo("CEL", ["Admin"]); + + // BLOCKING: the full truth table of all three arguments, because the predicate previously + // mirrored only ONE of resolveTrustedGrants' two refusals: + // + // trusted = (grants.includes("Admin") || selfAssigned) ? assignerIsAdmin + // : assignerIsAdmin || hasBoardSeatPerm + // + // Enumerated rather than sampled: the missing term (`selfAssigned`) sat in exactly two cells + // of this table, both of them non-Admin + non-Admin-granting, which is the shape every + // pre-existing case here already asserted as SILENT. A sampled suite therefore agreed with + // the bug. `expected` is derived from nothing — it is transcribed from beacon. + const TABLE: { + name: string; + cargo: Pick | undefined; + assignerIsAdmin: boolean; + isSelfAssignment: boolean; + expected: boolean; + }[] = [ + // No cargo at all: nothing to mint, nothing to warn about, either principal, either way. + { + name: "none", + cargo: undefined, + assignerIsAdmin: false, + isSelfAssignment: false, + expected: false, + }, + { + name: "none", + cargo: undefined, + assignerIsAdmin: false, + isSelfAssignment: true, + expected: false, + }, + { + name: "none", + cargo: undefined, + assignerIsAdmin: true, + isSelfAssignment: false, + expected: false, + }, + { + name: "none", + cargo: undefined, + assignerIsAdmin: true, + isSelfAssignment: true, + expected: false, + }, + // Grant-free: resolveTrustedGrants returns [] on `grants.length === 0` BEFORE it reads + // either flag, so there is no pending mint to warn about even when self-assigning. + { + name: "grant-free CEL", + cargo: CEL_FREE, + assignerIsAdmin: false, + isSelfAssignment: false, + expected: false, + }, + { + name: "grant-free CEL", + cargo: CEL_FREE, + assignerIsAdmin: false, + isSelfAssignment: true, + expected: false, + }, + { + name: "grant-free CEL", + cargo: CEL_FREE, + assignerIsAdmin: true, + isSelfAssignment: false, + expected: false, + }, + { + name: "grant-free CEL", + cargo: CEL_FREE, + assignerIsAdmin: true, + isSelfAssignment: true, + expected: false, + }, + { + name: "grant-free JDL", + cargo: JDL_FREE, + assignerIsAdmin: false, + isSelfAssignment: false, + expected: false, + }, + { + name: "grant-free JDL", + cargo: JDL_FREE, + assignerIsAdmin: false, + isSelfAssignment: true, + expected: false, + }, + // Non-Admin-granting POWER cargo. The delegation's whole point is the first row: a + // Secretary-granting seat IS honored from an update:BoardSeat assigner, so warning there + // would be false. The second row is the finding — same cargo, same principal, and the mint + // silently stops the moment the target is the caller. + { + name: "Secretary-granting", + cargo: POWER, + assignerIsAdmin: false, + isSelfAssignment: false, + expected: false, + }, + { + name: "Secretary-granting", + cargo: POWER, + assignerIsAdmin: false, + isSelfAssignment: true, + expected: true, + }, + { + name: "Secretary-granting", + cargo: POWER, + assignerIsAdmin: true, + isSelfAssignment: false, + expected: false, + }, + // An Admin self-assigning mints normally — `assignerIsAdmin` satisfies BOTH arms — so the + // note must not fire. This is the cell that stops the fix from being "warn on any self". + { + name: "Secretary-granting", + cargo: POWER, + assignerIsAdmin: true, + isSelfAssignment: true, + expected: false, + }, + // Admin-granting: refused from a non-Admin whoever the target is. + { + name: "Admin-granting", + cargo: ADMIN_CARGO, + assignerIsAdmin: false, + isSelfAssignment: false, + expected: true, + }, + { + name: "Admin-granting", + cargo: ADMIN_CARGO, + assignerIsAdmin: false, + isSelfAssignment: true, + expected: true, + }, + { + name: "Admin-granting", + cargo: ADMIN_CARGO, + assignerIsAdmin: true, + isSelfAssignment: false, + expected: false, + }, + { + name: "Admin-granting", + cargo: ADMIN_CARGO, + assignerIsAdmin: true, + isSelfAssignment: true, + expected: false, + }, + ]; + + it.each(TABLE)( + "BLOCKING: $name, admin=$assignerIsAdmin, self=$isSelfAssignment → $expected", + ({ cargo: c, assignerIsAdmin, isSelfAssignment, expected }) => { + expect(cargoGrantNeedsAdminAssigner(c, assignerIsAdmin, isSelfAssignment)).toBe(expected); + }, + ); + + // The finding in one line, spelled out away from the table so a future reader meets the + // scenario and not just a row. A delegate holding update:Position + update:BoardSeat opens + // their OWN profile and seats themselves on a vacant Secretario: firestore.rules permits the + // write, the seat publishes to the Directiva, and resolveTrustedGrants mints nothing because + // `selfAssigned && !assignerIsAdmin`. No response carries that — the note is the only channel. + it("BLOCKING: a delegate seating THEMSELVES on a non-Admin power cargo mints nothing", () => { + expect(cargoGrantNeedsAdminAssigner(POWER, false, true)).toBe(true); + // Same delegate, same cargo, someone else's profile: honored, so silence is correct. + expect(cargoGrantNeedsAdminAssigner(POWER, false, false)).toBe(false); + }); +}); + +// The fourth derived render-state. Both forms hand-rolled this as a two-branch ternary that +// only knew `noCargos` and `locked`, so the takedown and mint-pending notes were rendered but +// never ASSOCIATED — a screen-reader user on the trigger met neither. +describe("cargoNoteId", () => { + const IDS = { + noCargos: "no-cargos", + locked: "locked", + takedown: "takedown", + mintPending: "mint", + }; + const NONE = { noCargos: false, locked: false, takedown: false, mintPending: false }; + + it("returns undefined when no note is rendered", () => { + expect(cargoNoteId(NONE, IDS)).toBeUndefined(); + }); + + it("returns each state's own id when it is the only one firing", () => { + expect(cargoNoteId({ ...NONE, noCargos: true }, IDS)).toBe(IDS.noCargos); + expect(cargoNoteId({ ...NONE, locked: true }, IDS)).toBe(IDS.locked); + expect(cargoNoteId({ ...NONE, takedown: true }, IDS)).toBe(IDS.takedown); + expect(cargoNoteId({ ...NONE, mintPending: true }, IDS)).toBe(IDS.mintPending); + }); + + // BLOCKING: priority is most-blocking-first, and it is only observable when states co-fire. + // Asserting one state at a time would pass under ANY ordering of the four ifs. + it("BLOCKING: resolves co-firing states most-blocking-first", () => { + const all = { noCargos: true, locked: true, takedown: true, mintPending: true }; + expect(cargoNoteId(all, IDS)).toBe(IDS.noCargos); + expect(cargoNoteId({ ...all, noCargos: false }, IDS)).toBe(IDS.locked); + expect(cargoNoteId({ ...all, noCargos: false, locked: false }, IDS)).toBe(IDS.takedown); + expect(cargoNoteId({ ...NONE, takedown: true, mintPending: true }, IDS)).toBe(IDS.takedown); + }); + + // The ids are passed in, not owned here, because the locked and takedown wordings differ + // between the two forms. Pin that they are echoed verbatim — a hardcoded id here would send + // aria-describedby at an element that exists in only one of the two forms. + it("echoes the caller's ids rather than owning any", () => { + const other = { noCargos: "a", locked: "b", takedown: "c", mintPending: "d" }; + expect(cargoNoteId({ ...NONE, mintPending: true }, other)).toBe("d"); + }); +}); diff --git a/apps/backstage/src/features/members/lib/assignable-cargo.ts b/apps/backstage/src/features/members/lib/assignable-cargo.ts index 982c985c..7d8a2896 100644 --- a/apps/backstage/src/features/members/lib/assignable-cargo.ts +++ b/apps/backstage/src/features/members/lib/assignable-cargo.ts @@ -1,78 +1,59 @@ import { currentTermKey, positionTitle, type MemberGender, type Position } from "@luminova/types"; +import { cargoConfersPower, cargoSlotsForEditor } from "./assignable-cargo-core"; /** - * Client mirror of firestore.rules `cargoAssignableByNonAdmin()` — the two questions a - * non-Admin assignment must answer about the cargo being written in: - * grants.length === 0 the claims-mint boundary (assigning it would mint custom claims). - * category !== "CEL" the publication boundary. boardGroupFromCategory publishes CEL and - * JDL alike and boardRank puts 'Presidente' at rank 0, so a - * grant-free CEL cargo seats its holder at the head of the - * world-readable Directiva. JDL direcciones stay assignable — that - * is the accepted exposure this lane exists to deliver. + * The predicates that MIRROR firestore.rules — `positionsLockedForEditor`, + * `cargoTakedownOnly` and the option ceiling — live in `./assignable-cargo-core`, an + * import-free module, so `tests/firestore-rules/cargo-assignment-parity.test.ts` can drive + * them and the real emulator from one fixture. That package cannot resolve `@luminova/types`, + * which this file needs for VALUE (`currentTermKey`, `positionTitle`), so a test importing + * THIS module would throw at load. Same trick `nav-equivalence.test.ts` documents for + * `nav-config.ts`. * - * The rules apply it on BOTH member lanes (`createPositionsSafe` and - * `positionsAssignmentSafe`), so both member forms apply it here. One function, not a - * `grants.length === 0` re-typed per form: rendering an option the save will 403 on is the - * render-then-die shape this repo guards against, and two copies drift. - */ -// Module-local: every consumer now goes through cargoOptionsForEditor() / -// cargoTakedownOnly() / positionsLockedForNonAdmin(), which is the point — a caller that -// re-derived the option list from this raw predicate is how the two forms drifted apart in -// the first place. Exporting it again would give that back. -function cargoAssignableByNonAdmin(cargo: Pick): boolean { - return cargo.grants.length === 0 && cargo.category !== "CEL"; -} - -/** - * Whether a non-Admin is barred from touching the positions slot AT ALL, given the cargo the - * member currently holds. NOT the negation of `cargoAssignableByNonAdmin` — the two rules - * conjuncts are asymmetric, and mirroring the wrong one strands a takedown: + * Every caller imports those two DIRECTLY from `./assignable-cargo-core`, and that is the + * point rather than an inconvenience: which file a predicate comes from is what says whether + * the emulator parity test holds it to `firestore.rules` or whether it is local render state. + * A pass-through re-export here erased exactly that distinction (and was a barrel besides — + * CLAUDE.md: import directly from the file). * - * grants.length > 0 → locked. `currentCargoGrantsEmpty()` gates the cargo being REPLACED, - * so a non-Admin can neither keep it (the save re-stamps it) nor clear - * it. Nothing they can do here succeeds. - * grant-free CEL → NOT locked. `currentCargoGrantsEmpty()` is deliberately not - * category-gated — firestore.rules says denying this "would strand a - * takedown behind an Admin" — so clearing the seat is allowed even - * though keeping it is not. The cargo is dropped from the options - * instead, which makes the only submittable states "clear" or "some - * other assignable cargo" — exactly the rules' answer. + * What lives HERE: the render states (`noAssignableCargos`, `cargoNoteId`), the labelling half + * of `cargoOptionsForEditor`, and `cargoGrantNeedsAdminAssigner` — which mirrors BEACON's trust + * gate, not a rules predicate, so the rules-parity module is the wrong home for it. Nothing in + * this file is covered by the parity test. */ -export function positionsLockedForNonAdmin( - cargo: Pick | undefined, -): boolean { - return cargo !== undefined && cargo.grants.length > 0; -} /** - * The takedown-only state: the member is seated on a cargo this editor may NOT keep but MAY - * clear — a grant-free CEL seat for a non-Admin. It is the one state where the honest render - * is neither "pick anything" nor "locked": - * - the seat must still be VISIBLE (the holder holds it), so it is offered as a disabled - * option and the Combobox trigger shows its title instead of the "Sin cargo" placeholder; - * - the seat must not be re-submittable (the rules 403 any positions write that keeps it); - * - clearing it must be reachable, which a disabled option cannot do on its own — Combobox - * clears by re-selecting the SELECTED option, and a disabled item swallows the select. So - * the forms render an explicit "Quitar cargo" action while this is true. - * Takes the CURRENT selection, not the stored one: once cleared or switched away the state is - * over, and the takedown affordance goes with it. + * A seat the editor may WRITE but whose grants will not be MINTED — the one outcome in this + * lane that fails silently. The rules allow the write, the seat publishes to the Directiva, and + * `syncMemberClaims` is a trigger, so there is no response the client could learn this from. + * Warning before the click is the only channel. + * + * Mirrors BOTH refusals in `resolveTrustedGrants`, which are disjoint and were separately + * argued for — covering only the first is how this note would go quietly half-right: + * grants include Admin → honored only for an assigner holding the Admin ROLE. + * SELF-assignment → honored only for an Admin, WHATEVER the cargo grants. A delegate + * may confer power on others, never on themselves. + * + * `assignerIsAdmin` is the MINTING authority (beacon's `assignerIsAdmin`), deliberately named + * after that rather than after `allowReplacePowerCargo`, which mirrors a different predicate + * (`currentCargoGrantsEmpty`) and merely happens to equal it today. Feeding one to the other is + * the conflation this file exists to prevent. + * + * An Admin re-saving the same slot re-stamps `assignedBy` and completes the mint. */ -export function cargoTakedownOnly( - cargo: Pick | undefined, - allowPowerGrants: boolean, +export function cargoGrantNeedsAdminAssigner( + cargo: Pick | undefined, + assignerIsAdmin: boolean, + isSelfAssignment: boolean, ): boolean { - return ( - !allowPowerGrants && - cargo !== undefined && - !cargoAssignableByNonAdmin(cargo) && - !positionsLockedForNonAdmin(cargo) - ); + if (assignerIsAdmin || !cargoConfersPower(cargo)) return false; + return isSelfAssignment || (cargo?.grants.includes("Admin") ?? false); } export type CargoOption = { value: string; label: string; disabled?: boolean }; /** - * The third derived render-state, alongside `positionsLockedForNonAdmin` and + * The third derived render-state, alongside `positionsLockedForEditor` and * `cargoTakedownOnly`: this editor may assign, but the ceiling filtered every option away. * * Lives here for the same reason as its two siblings — both member forms ask it, and typing @@ -89,17 +70,60 @@ export function noAssignableCargos(input: { return !input.locked && !input.allowPowerGrants && input.cargoOptions.length === 0; } +/** + * Which note the cargo Combobox's `aria-describedby` should point at — the fourth derived + * render-state, and here for the same reason as the other three: both forms ask it, and a + * re-typed ternary at each call site is how they drift. + * + * Order is priority and it is load-bearing, because of the one pair that CAN co-fire: + * locked ∧ mintPending REACHABLE — a delegate opening a member seated on an Admin-granting + * cargo has both. `locked` must win: the mint note's render is guarded + * by `!locked`, so pointing aria-describedby at it would reference an + * element that is not in the DOM, which is worse than no association. + * takedown ∧ mintPending impossible — takedown needs grants.length === 0, mintPending needs + * grants.length > 0, both off the same selected cargo. + * noCargos ∧ locked impossible — a locked slot's held cargo is always in the list. + * The remaining three pairs are unreachable only because of facts OUTSIDE this file — the + * Combobox is `disabled={locked}`, so the selection cannot leave the held cargo, and + * `cargoSlotsForEditor` never offers a grant-free CEL while `!allowPowerGrants`. Change either + * and `locked ∧ takedown` becomes reachable, at which point this order matters again. + * First match wins, most-blocking first: not being able to pick anything outranks what a pick + * would mint. + * + * The ids differ per form (the locked and takedown wordings legitimately differ between them), + * so they are passed in rather than owned here. Build the argument with `cargoNoteIds(prefix)` + * (./../components/no-assignable-cargos-note) — never by hand: this signature is the only + * thing that would notice a form that forgot one, and it cannot notice a copy that has all + * four but points one at the OTHER form's element. + */ +export interface CargoNoteIds { + noCargos: string; + locked: string; + takedown: string; + mintPending: string; +} + +export function cargoNoteId( + state: { noCargos: boolean; locked: boolean; takedown: boolean; mintPending: boolean }, + ids: CargoNoteIds, +): string | undefined { + if (state.noCargos) return ids.noCargos; + if (state.locked) return ids.locked; + if (state.takedown) return ids.takedown; + if (state.mintPending) return ids.mintPending; + return undefined; +} + /** * The cargo Combobox options for one editor, shared by both member forms so they cannot * disagree about the same rules predicate (they did: one dropped the held CEL seat, the other * re-added it labelled "(inactivo)" — an ACTIVE cargo, mislabelled and re-offered to the very * editor whose write the rules reject). * - * Assignable cargos of the current term, plus the HELD cargo when it is not among them — - * appended `disabled` when this editor may not assign it, so the trigger tells the truth about - * who holds what without making a denied write one click away. Keyed on the stored assignment, - * never the live selection: an option list that reacts to the selection deletes the entry the - * user just switched away from, so switching back is impossible. + * WHICH cargos, and which of them are shown-but-denied, is `cargoSlotsForEditor` + * (./assignable-cargo-core) — the rules mirror, held to the emulator by the parity test. This + * function is the labelling layer over it: gendered titles, plus the "(inactivo)" suffix on a + * held cargo that is deactivated or belongs to a past term. */ export function cargoOptionsForEditor({ positions, @@ -114,21 +138,13 @@ export function cargoOptionsForEditor({ assignedCargoId: string | null | undefined; term?: string; }): CargoOption[] { - const currentTerm = (p: Position) => p.term === null || String(p.term) === term; - const options = positions - .filter((p) => p.active && p.category !== "Comision" && currentTerm(p)) - .filter((p) => allowPowerGrants || cargoAssignableByNonAdmin(p)) - .map((p) => ({ value: p.id, label: positionTitle(p, gender) })); - - const held = assignedCargoId ? positions.find((p) => p.id === assignedCargoId) : undefined; - if (held === undefined || options.some((o) => o.value === held.id)) return options; - const retired = !held.active || !currentTerm(held); - return [ - ...options, - { - value: held.id, - label: retired ? `${positionTitle(held, gender)} (inactivo)` : positionTitle(held, gender), - disabled: !allowPowerGrants && !cargoAssignableByNonAdmin(held), - }, - ]; + return cargoSlotsForEditor({ positions, allowPowerGrants, assignedCargoId, term }).map( + ({ position, retired, disabled }) => ({ + value: position.id, + label: retired + ? `${positionTitle(position, gender)} (inactivo)` + : positionTitle(position, gender), + ...(disabled ? { disabled } : {}), + }), + ); } diff --git a/apps/backstage/src/features/members/lib/member-permissions.test.ts b/apps/backstage/src/features/members/lib/member-permissions.test.ts index 7f3a0032..85b9f6ab 100644 --- a/apps/backstage/src/features/members/lib/member-permissions.test.ts +++ b/apps/backstage/src/features/members/lib/member-permissions.test.ts @@ -1,6 +1,43 @@ import { describe, expect, it } from "vitest"; import type { Position } from "@luminova/types"; -import { effectiveRoles } from "./member-permissions"; +import { effectiveRoles, isSelfMember } from "./member-permissions"; + +// The full truth table over (member.uid, caller uid), because the interesting cell is the one +// a bare `member.uid === uid` gets WRONG and no other cell notices. Three call sites had this +// typed by hand before it was extracted (both cargo editors and /me's self-edit gate). +describe("isSelfMember", () => { + it("is true only when a present uid matches the caller", () => { + expect(isSelfMember({ uid: "u1" }, "u1")).toBe(true); + }); + + it("is false for a different caller", () => { + expect(isSelfMember({ uid: "u1" }, "u2")).toBe(false); + }); + + // BLOCKING: the trap the function exists for. An UNPROVISIONED member has no uid — the + // commonest doc shape in the collection — and a caller whose auth has not resolved yet has + // no uid either, so `undefined === undefined` calls a stranger "yourself". That answer feeds + // `isSelfAssignment`, which decides whether a delegate is warned that seating this member + // mints nothing: get it wrong and the warning fires on the wrong person. + it("BLOCKING: is false when BOTH are undefined", () => { + expect(isSelfMember({ uid: undefined }, undefined)).toBe(false); + }); + + it("is false for a member with no uid and a resolved caller", () => { + expect(isSelfMember({ uid: undefined }, "u1")).toBe(false); + }); + + it("is false for a provisioned member and an unresolved caller", () => { + expect(isSelfMember({ uid: "u1" }, undefined)).toBe(false); + }); + + // A member doc that never carried the key at all, not just one holding `undefined`. Same + // answer, but it is the shape Firestore actually returns for an unlinked member. + it("is false when the member doc has no uid key at all", () => { + expect(isSelfMember({}, undefined)).toBe(false); + expect(isSelfMember({}, "u1")).toBe(false); + }); +}); const pos = (id: string, grants: Position["grants"]): Position => ({ id, diff --git a/apps/backstage/src/features/members/lib/member-permissions.ts b/apps/backstage/src/features/members/lib/member-permissions.ts index bacecb60..fd3a4967 100644 --- a/apps/backstage/src/features/members/lib/member-permissions.ts +++ b/apps/backstage/src/features/members/lib/member-permissions.ts @@ -1,5 +1,20 @@ import { ROLES, type Member, type Position, type Role } from "@luminova/types"; +/** + * Whether this member doc IS the signed-in user. + * + * The `uid !== undefined` half is the whole reason this is a function: an unprovisioned member + * has no `uid`, and so does a caller whose auth has not resolved yet, so a bare `===` reads + * `undefined === undefined` and calls a stranger "yourself". That answer feeds + * `isSelfAssignment`, which decides whether the delegate is warned that seating this member + * mints nothing — get it wrong and the warning fires on the wrong person, or not at all. + * + * Three call sites had this typed out by hand (both cargo editors and /me's self-edit gate). + */ +export function isSelfMember(member: Pick, uid: string | undefined): boolean { + return member.uid !== undefined && member.uid === uid; +} + // Cargo grants only — comisiones are chips-only, matching the claims-sync trigger, which // ignores comisión grants. This is the UPPER BOUND of what the trigger will mint, not an // exact mirror: resolveTrustedGrants (apps/beacon/src/claims-sync/sync.ts) additionally diff --git a/apps/backstage/src/features/members/lib/provision-error.test.ts b/apps/backstage/src/features/members/lib/provision-error.test.ts index 0aa902a2..aa0fe5d5 100644 --- a/apps/backstage/src/features/members/lib/provision-error.test.ts +++ b/apps/backstage/src/features/members/lib/provision-error.test.ts @@ -1,23 +1,76 @@ import { describe, expect, it } from "vitest"; +import { PROVISION_BLOCK_REASONS, type ProvisionBlockReason } from "@luminova/types"; import { provisionErrorMessage } from "./provision-error"; const FALLBACK = "No se pudo enviar la invitación."; +const withReason = (reason: unknown) => + Object.assign(new Error("failed-precondition"), { details: { reason } }); + +/** Typed narrowing of `withReason` for the real tags: a reason renamed in beacon fails to + * compile here too, instead of quietly asserting a message nothing throws any more. */ +const blocked = (reason: ProvisionBlockReason) => withReason(reason); + describe("provisionErrorMessage", () => { it("explains the console unlink when the callable reports a uid conflict", () => { - const err = Object.assign(new Error("failed-precondition"), { - details: { reason: "linked-to-different-login" }, - }); - expect(provisionErrorMessage(err, FALLBACK)).toBe( + expect(provisionErrorMessage(blocked("linked-to-different-login"), FALLBACK)).toBe( "El miembro ya está vinculado a otro acceso (correo cambiado). Desvincúlalo desde la consola antes de reintentar.", ); }); + // The three non-Admin refusals. provisionBlockedForNonAdmin sees the member DOC, not the + // Auth directory, so a delegate still reaches these — and without a message each reads as a + // transient failure the operator retries forever. + it("names beacon's adoption refusal (reprovision-requires-admin)", () => { + expect(provisionErrorMessage(blocked("reprovision-requires-admin"), FALLBACK)).toBe( + "Ya existe un acceso para este correo. Pídele a un administrador que lo reenvíe o lo vincule.", + ); + }); + + it("names beacon's direct-grants refusal (granted-member-requires-admin)", () => { + expect(provisionErrorMessage(blocked("granted-member-requires-admin"), FALLBACK)).toBe( + "Este miembro tiene roles o permisos asignados: solo un administrador puede crear su acceso.", + ); + }); + + it("names beacon's power-seat refusal (power-seat-requires-admin)", () => { + expect(provisionErrorMessage(blocked("power-seat-requires-admin"), FALLBACK)).toBe( + "El cargo de este miembro otorga permisos: solo un administrador puede crear su acceso.", + ); + }); + + it("names the malformed stored email, which no retry can fix", () => { + expect(provisionErrorMessage(blocked("member-email-malformed"), FALLBACK)).toBe( + "El correo guardado de este miembro no es válido. Corrígelo en su ficha antes de crear su acceso.", + ); + }); + + it("gives EVERY reason beacon can throw a distinct message", () => { + // Iterates the shared union rather than re-listing the literals: a reason added in beacon + // and not given a message here fails this test (it falls back), and a copy-paste that + // leaves two reasons sharing a message — telling the operator to do the wrong thing about + // half the time — fails it too. + const messages = PROVISION_BLOCK_REASONS.map((reason) => + provisionErrorMessage(blocked(reason), FALLBACK), + ); + expect(messages).toHaveLength(PROVISION_BLOCK_REASONS.length); + expect(messages).not.toContain(FALLBACK); + expect(new Set(messages).size).toBe(messages.length); + }); + it("falls back to the generic message for any other failure", () => { expect(provisionErrorMessage(new Error("boom"), FALLBACK)).toBe(FALLBACK); expect(provisionErrorMessage(undefined, FALLBACK)).toBe(FALLBACK); + expect(provisionErrorMessage(null, FALLBACK)).toBe(FALLBACK); expect( provisionErrorMessage(Object.assign(new Error("x"), { details: "otra cosa" }), FALLBACK), ).toBe(FALLBACK); + // An unknown code, and a non-string reason: neither may index the table. + expect(provisionErrorMessage(withReason("no-such-reason"), FALLBACK)).toBe(FALLBACK); + expect(provisionErrorMessage(withReason(42), FALLBACK)).toBe(FALLBACK); + expect(provisionErrorMessage(withReason(undefined), FALLBACK)).toBe(FALLBACK); + // Inherited Object.prototype keys must not resolve to a function-as-message. + expect(provisionErrorMessage(withReason("toString"), FALLBACK)).toBe(FALLBACK); + expect(provisionErrorMessage(withReason("constructor"), FALLBACK)).toBe(FALLBACK); }); }); diff --git a/apps/backstage/src/features/members/lib/provision-error.ts b/apps/backstage/src/features/members/lib/provision-error.ts index bde16daf..16791d69 100644 --- a/apps/backstage/src/features/members/lib/provision-error.ts +++ b/apps/backstage/src/features/members/lib/provision-error.ts @@ -1,15 +1,52 @@ -// provisionMemberLogin tags its uid-conflict rejection with details.reason so -// the UI can point the operator at the console unlink instead of a dead-end -// generic failure (the callable refuses to relink a member whose stored uid -// still resolves to a live, different Auth account). -export function provisionErrorMessage(err: unknown, fallback: string): string { +import type { ProvisionBlockReason } from "@luminova/types"; + +// provisionMemberLogin tags every refusal it can be argued with using details.reason, so the +// UI can name the actual blocker instead of a dead-end generic failure. Three of the five are +// non-Admin refusals a delegate can hit on a member the client cannot fully evaluate +// (provisionBlockedForNonAdmin sees the member doc, not the Auth directory), and without a +// message each reads as a transient error the operator retries forever. +// +// Keyed by ProvisionBlockReason, the union beacon throws from (@luminova/types) — a renamed +// or added reason is a compile error here rather than a silent fall-through to `fallback`. +// Type-only import: the runtime union array is used by the test, never by the bundle. +const MESSAGES: Readonly> = { + "linked-to-different-login": + "El miembro ya está vinculado a otro acceso (correo cambiado). Desvincúlalo desde la consola antes de reintentar.", + "reprovision-requires-admin": + "Ya existe un acceso para este correo. Pídele a un administrador que lo reenvíe o lo vincule.", + "granted-member-requires-admin": + "Este miembro tiene roles o permisos asignados: solo un administrador puede crear su acceso.", + "power-seat-requires-admin": + "El cargo de este miembro otorga permisos: solo un administrador puede crear su acceso.", + "member-email-malformed": + "El correo guardado de este miembro no es válido. Corrígelo en su ficha antes de crear su acceso.", +}; + +// A Map, not the object literal above: `reason` is attacker-adjacent input (it arrives in the +// callable's error payload), and `{...}[reason]` resolves "toString" / "constructor" / +// "valueOf" to the inherited Object.prototype FUNCTION, which `?? fallback` then happily +// returns as the message. TypeScript types that `string` and React would render a function. +// The literal buys exhaustiveness against the union; the Map buys a safe lookup. +const REASON_MESSAGES = new Map(Object.entries(MESSAGES)); + +/** The callable's own explanation for a refusal, or null when it did not give one (a + * transient failure — App Check, quota, config — or a reason this build does not know). + * + * Separate from `provisionErrorMessage` because the two answer different questions. A caller + * that only needs text takes the message; a caller that must also decide WHAT TO SAY NEXT + * needs to know whether the server refused on purpose. The invite drawer needs the second: + * its headline otherwise tells the operator to retry from the row menu on a refusal only an + * Admin can clear, with the real explanation demoted to small print underneath. */ +export function provisionRefusalMessage(err: unknown): string | null { const details = (err as { details?: unknown } | null | undefined)?.details; const reason = typeof details === "object" && details !== null ? (details as { reason?: unknown }).reason : undefined; - if (reason === "linked-to-different-login") { - return "El miembro ya está vinculado a otro acceso (correo cambiado). Desvincúlalo desde la consola antes de reintentar."; - } - return fallback; + if (typeof reason !== "string") return null; + return REASON_MESSAGES.get(reason) ?? null; +} + +export function provisionErrorMessage(err: unknown, fallback: string): string { + return provisionRefusalMessage(err) ?? fallback; } diff --git a/apps/backstage/src/features/members/lib/provision-gate.test.ts b/apps/backstage/src/features/members/lib/provision-gate.test.ts new file mode 100644 index 00000000..34b84f45 --- /dev/null +++ b/apps/backstage/src/features/members/lib/provision-gate.test.ts @@ -0,0 +1,213 @@ +import { describe, expect, it } from "vitest"; +import { Timestamp } from "firebase/firestore"; +import { currentTermKey, type Member, type Position } from "@luminova/types"; +import { draftProvisionBlocked, memberProvisionBlocked, type CargoLookup } from "./provision-gate"; + +const POWER = "pos-power"; +const PLAIN = "pos-plain"; + +const catalog: CargoLookup = (id) => + ({ + [POWER]: { grants: ["Secretary"] as Position["grants"] }, + [PLAIN]: { grants: [] as Position["grants"] }, + })[id]; + +function member(p: Partial = {}): Member { + return { + id: "m1", + name: "Ana", + email: "a@jci.bo", + joinDate: Timestamp.now(), + birthdate: Timestamp.now(), + status: "Activo", + profilePicture: null, + totalPoints: 0, + active: true, + deletedAt: null, + ...p, + }; +} + +const term = currentTermKey(); +const nextTerm = String(Number(term) + 1); +const seat = (cargoId: string | null, key = term) => ({ + positions: { [key]: { cargoId, comisionIds: [] } }, +}); + +describe("memberProvisionBlocked", () => { + it("does not block a clean, unseated member", () => { + expect(memberProvisionBlocked(member(), catalog, false)).toBe(false); + }); + + it("does not block a member seated on a grant-free cargo", () => { + // The control for the cargo clause: being seated is not the refusal, conferring power is. + expect(memberProvisionBlocked(member(seat(PLAIN)), catalog, false)).toBe(false); + }); + + it("does not block a member whose only term entry has a null cargo", () => { + expect(memberProvisionBlocked(member(seat(null)), catalog, false)).toBe(false); + }); + + it("blocks a member who already has a login (beacon's adoption guard)", () => { + expect(memberProvisionBlocked(member({ uid: "u1" }), catalog, false)).toBe(true); + }); + + it("treats an empty-string uid as no login", () => { + // A stored empty string is not a linked account; blocking on it would hide the invite for + // exactly the member who still needs one. + expect(memberProvisionBlocked(member({ uid: "" }), catalog, false)).toBe(false); + }); + + it("blocks a member carrying direct roleIds", () => { + expect(memberProvisionBlocked(member({ roleIds: ["custom"] }), catalog, false)).toBe(true); + }); + + it("does not block on an empty roleIds array", () => { + expect(memberProvisionBlocked(member({ roleIds: [] }), catalog, false)).toBe(false); + }); + + it("blocks a member carrying a permissionOverrides GRANT", () => { + expect( + memberProvisionBlocked( + member({ permissionOverrides: { grant: ["update:Member"], revoke: [] } }), + catalog, + false, + ), + ).toBe(true); + }); + + // Deliberate mirror of beacon's hasDirectGrants, which checks `grant` only: a revoke-only + // override mints nothing, so it is not a reason to withhold the invite. Widening this to + // "has any override" would silently take the affordance away from a delegate for a member + // who has strictly FEWER permissions than the default. + it("BLOCKING: does NOT block on a revoke-only override — it mints nothing", () => { + expect( + memberProvisionBlocked( + member({ permissionOverrides: { grant: [], revoke: ["update:Member"] } }), + catalog, + false, + ), + ).toBe(false); + }); + + it("blocks a member seated on a power-granting cargo in the CURRENT term", () => { + expect(memberProvisionBlocked(member(seat(POWER)), catalog, false)).toBe(true); + }); + + // syncMemberClaims reads the current term at trigger time, so a future-term seat mints on + // the year rollover. Reading only the current term here would offer the invite today and + // 403 on it — beacon reads every term. + it("BLOCKING: blocks a power-granting cargo seated in a FUTURE term", () => { + expect(memberProvisionBlocked(member(seat(POWER, nextTerm)), catalog, false)).toBe(true); + }); + + it("blocks a power-granting cargo seated in a PAST term", () => { + expect(memberProvisionBlocked(member(seat(POWER, "2020")), catalog, false)).toBe(true); + }); + + // Fails CLOSED in the same direction as beacon, which reads grants === null from an + // unreadable cargo the same way. A stale or still-loading `positions` prop must hide the + // affordance rather than promise a 403. + it("BLOCKING: blocks when the cargo id does not resolve against the catalog", () => { + expect(memberProvisionBlocked(member(seat("gone")), catalog, false)).toBe(true); + expect(memberProvisionBlocked(member(seat(PLAIN)), () => undefined, false)).toBe(true); + }); + + it("blocks when any ONE term is power-granting among several clean ones", () => { + const m = member({ + positions: { + "2024": { cargoId: PLAIN, comisionIds: [] }, + [term]: { cargoId: POWER, comisionIds: [] }, + [nextTerm]: { cargoId: null, comisionIds: [] }, + }, + }); + expect(memberProvisionBlocked(m, catalog, false)).toBe(true); + }); + + // BLOCKING: an EMPTY-STRING cargoId is a MALFORMED seat, not an empty one. This mirrors + // beacon's readCargoIds, which pushes "" rather than skipping it — "a malformed shape must + // never read as 'no cargo', that is the guard's own bypass" — and then refuses it at + // isSafeDocId. The gate used to test `term.cargoId ? [...] : []`, so "" read as "no cargo" + // here: the invite was offered, the click 403'd with power-seat-requires-admin, and the + // message named a cargo that does not exist. Now only undefined/null are skipped, so "" + // falls through to the unresolvable-cargo clause and blocks — same direction as beacon. + it("BLOCKING: blocks an EMPTY-STRING cargoId instead of reading it as no cargo", () => { + expect(memberProvisionBlocked(member(seat("")), catalog, false)).toBe(true); + // It blocks THROUGH the unresolvable-cargo clause, not through a clause of its own: no + // Firestore document id can be "", so every CargoLookup answers undefined for it. Pinned + // as the mechanism because that is the whole fix — "" was being dropped before it ever + // reached the lookup, and there is no other clause standing behind it. + expect(memberProvisionBlocked(member(seat("gone")), catalog, false)).toBe(true); + }); + + // The `undefined`/`null` half of that same change, so the fix cannot be over-applied into + // "any falsy cargoId blocks". A term row with no cargo is the ordinary shape of an unseated + // member and readCargoIds `continue`s past both — blocking here would hide the invite from + // nearly every member in the chapter. + // The `undefined` arm of the same guard is not exercised here on purpose: TermPositions + // declares `cargoId` as required (`string | null`), so an absent key is unrepresentable in a + // well-typed doc and asserting it would need a cast. It is kept in the gate as a read-side + // defence — Firestore hands back whatever is stored — not as a reachable branch. + it("BLOCKING: still skips a NULL cargoId, which is a genuinely unseated term", () => { + expect(memberProvisionBlocked(member(seat(null)), catalog, false)).toBe(false); + }); + + // The callerIsAdmin short-circuit, which replaced a `!isAdmin &&` conjunct re-typed at three + // call sites. Every refusal beacon applies is guarded by `!callerHoldsAdminRole`, so an Admin + // is subject to none of them. Swept over every blocking fixture above rather than sampled: + // the parameter is a single early return, so a regression that dropped it would show up on + // whichever refusal a sampled test happened not to cover. + it("BLOCKING: never blocks an Admin caller, on any refusal", () => { + const blocked: Member[] = [ + member({ uid: "u1" }), + member({ roleIds: ["custom"] }), + member({ permissionOverrides: { grant: ["update:Member"], revoke: [] } }), + member(seat(POWER)), + member(seat(POWER, nextTerm)), + member(seat("gone")), + member(seat("")), + ]; + for (const m of blocked) { + expect(memberProvisionBlocked(m, catalog, false)).toBe(true); + expect(memberProvisionBlocked(m, catalog, true)).toBe(false); + } + }); +}); + +describe("draftProvisionBlocked", () => { + it("does not block a draft with no cargo", () => { + expect(draftProvisionBlocked(null, catalog, false)).toBe(false); + expect(draftProvisionBlocked(undefined, catalog, false)).toBe(false); + }); + + // The SAME answer as the member variant, which is the point. This used to read "" as "no + // cargo" and the test enshrined the divergence as deliberate — but a mirror whose two halves + // disagree about what "no cargo" means is precisely how these predicates drift, and the + // reasoning ("`z.string().min(1).nullable()` cannot produce one") makes the case UNREACHABLE, + // not the fail-open answer correct. "" is an id that resolves to nothing, both sides. + it("BLOCKING: treats an empty-string draft cargoId as unresolvable, like the member variant", () => { + expect(draftProvisionBlocked("", catalog, false)).toBe(true); + // …and an Admin is subject to none of it, on either side. + expect(draftProvisionBlocked("", catalog, true)).toBe(false); + }); + + it("does not block a draft seated on a grant-free cargo", () => { + expect(draftProvisionBlocked(PLAIN, catalog, false)).toBe(false); + }); + + it("blocks a draft seated on a power-granting cargo", () => { + expect(draftProvisionBlocked(POWER, catalog, false)).toBe(true); + }); + + it("BLOCKING: fails closed on a cargo id the catalog cannot resolve", () => { + expect(draftProvisionBlocked("gone", catalog, false)).toBe(true); + }); + + // Same short-circuit as the member variant, and the reason the invite drawer no longer types + // `!isAdmin &&` at its call site. Without it an Admin creating a board member would be told + // "solo un administrador puede enviarle el acceso" — copy that contradicts itself. + it("BLOCKING: never blocks an Admin caller, on either refusal", () => { + expect(draftProvisionBlocked(POWER, catalog, true)).toBe(false); + expect(draftProvisionBlocked("gone", catalog, true)).toBe(false); + }); +}); diff --git a/apps/backstage/src/features/members/lib/provision-gate.ts b/apps/backstage/src/features/members/lib/provision-gate.ts new file mode 100644 index 00000000..ccacd6d4 --- /dev/null +++ b/apps/backstage/src/features/members/lib/provision-gate.ts @@ -0,0 +1,93 @@ +import type { Member, Position } from "@luminova/types"; + +/** + * Client mirror of the refusals `provisionMember` applies to a non-Admin caller — the + * adoption guard and both halves of the power-seat guard. The reasons they throw are the + * `ProvisionBlockReason` union in `@luminova/types` — named there, not spelled out here, so + * this comment cannot quietly outlive a rename the way an unchecked prose copy would. + * + * NOT a security boundary: beacon is, and it re-derives all of this server-side from the + * stored doc. This exists so the three entry points that offer "invitar / enviar acceso" do + * not offer it for a member the callable refuses on every click — the render-then-die shape + * this feature already guards against on the cargo picker. One function, not a predicate + * re-typed per entry point: the invite drawer had it and the row menu and profile header did + * not, which is exactly how the two cargo forms drifted before. + * + * Fails CLOSED in the same direction as beacon: an unresolvable cargo id counts as + * power-conferring (beacon reads `grants === null` from an unreadable cargo the same way), so + * a stale `positions` prop hides the affordance rather than promising a 403. + */ +// Module-local, like assignable-cargo's raw predicates: the two adapters below are the whole +// public surface, and a caller that re-assembled the input itself is how a mirror drifts from +// the guard it mirrors. +function provisionBlockedForNonAdmin(input: { + /** The member already has a login, or an Auth account exists for their address. Only the + * first half is visible to the client; beacon checks both, so this is a subset — the + * residual case still 403s and is what `provisionErrorMessage` explains. */ + hasLogin: boolean; + hasDirectGrants: boolean; + /** Every seated cargo across EVERY term, resolved against the catalog. `undefined` = the id + * did not resolve. Beacon reads every term too: syncMemberClaims reads the current term at + * trigger time, so a future-term seat mints on the year rollover. */ + seatedCargos: readonly (Pick | undefined)[]; +}): boolean { + return ( + input.hasLogin || + input.hasDirectGrants || + input.seatedCargos.some((cargo) => cargo === undefined || cargo.grants.length > 0) + ); +} + +/** Resolves a cargo id against the catalog. A callback rather than a `Position[]` or a Map so + * each caller passes whatever it already holds — the members table has a `positionsById` Map, + * the profile page and the invite drawer have arrays — without one of them allocating. */ +export type CargoLookup = (id: string) => Pick | undefined; + +/** `provisionBlockedForNonAdmin` for a stored member doc. + * + * `callerIsAdmin` is a PARAMETER rather than a `!isAdmin &&` at each call site, for the same + * reason `positionsLockedForEditor` takes its flag: three call sites typing the same conjunct + * is how the cargo predicates drifted, and this function is even NAMED for the conjunct. An + * Admin is subject to none of these refusals — beacon's guards are all `!callerHoldsAdminRole`. */ +export function memberProvisionBlocked( + member: Member, + cargo: CargoLookup, + callerIsAdmin: boolean, +): boolean { + if (callerIsAdmin) return false; + // `?? []` on absent/null only — an EMPTY-STRING cargoId must NOT be skipped. beacon's + // readCargoIds pushes "" deliberately ("a malformed shape must never read as 'no cargo' — + // that is the guard's own bypass"), and "" then fails isSafeDocId at the port and refuses. + // A truthiness test here would skip it, promise an invite, and 403 with a message about a + // cargo that does not exist. It falls through to the `cargo === undefined` clause instead. + const cargoIds = Object.values(member.positions ?? {}).flatMap((term) => + term.cargoId === undefined || term.cargoId === null ? [] : [term.cargoId], + ); + return provisionBlockedForNonAdmin({ + hasLogin: typeof member.uid === "string" && member.uid.length > 0, + // `grant` only, mirroring beacon's hasDirectGrants: a revoke-only override mints nothing, + // so it is not a reason to withhold the invite. + hasDirectGrants: + (member.roleIds?.length ?? 0) > 0 || (member.permissionOverrides?.grant?.length ?? 0) > 0, + seatedCargos: cargoIds.map(cargo), + }); +} + +/** `provisionBlockedForNonAdmin` for a member about to be CREATED: the create arm forbids + * `uid`, `roleIds` and `permissionOverrides` to a non-Admin, so the cargo is the only half + * that can be true. */ +export function draftProvisionBlocked( + cargoId: string | null | undefined, + cargo: CargoLookup, + callerIsAdmin: boolean, +): boolean { + if (callerIsAdmin) return false; + return provisionBlockedForNonAdmin({ + hasLogin: false, + hasDirectGrants: false, + // Explicitly null/undefined, NOT truthiness — same rule its sibling states 25 lines up. + // `memberSchema` keeps "" out of this form today, so the divergence is latent; a mirror + // whose two halves disagree about what "no cargo" means is how they drift apart anyway. + seatedCargos: cargoId === null || cargoId === undefined ? [] : [cargo(cargoId)], + }); +} diff --git a/apps/backstage/src/lib/authz/capability-flags.ts b/apps/backstage/src/lib/authz/capability-flags.ts new file mode 100644 index 00000000..a80bf929 --- /dev/null +++ b/apps/backstage/src/lib/authz/capability-flags.ts @@ -0,0 +1,66 @@ +import { hasAnyRole, hasPerm, type AuthClaims } from "@luminova/auth/roles"; +import type { PermissionCode } from "@luminova/types"; + +/** + * The claims → capability-flag derivation, split out of `buildCan` (./use-can) for ONE reason: + * `tests/firestore-rules/cargo-assignment-parity.test.ts` needs the flags the member forms are + * actually wired from, and it cannot load `use-can.ts` (React) — so it re-implemented this + * mapping by hand, which is the mirror class that whole test exists to delete, applied to the + * very flag whose widening caused the #224 regression. + * + * No runtime `@luminova/types` import: `PermissionCode` is type-only and erased, the same trick + * `assignable-cargo-core.ts` and `nav-config.ts` document. Keep it that way — the rules-test + * package cannot resolve that package at runtime. + */ + +/** The shape every delegable capability gate takes: the Admin ROLE, or one exact permission + * code. Extracted at the third occurrence — `hasPerm` is deliberately NOT `abilityAllows`, + * and re-deriving that decision per flag is how one of them ends up looser than its rule. + * See `canFeatureInitiatives` below for the full reasoning it encodes. */ +function adminOrPerm(claims: AuthClaims, code: PermissionCode): boolean { + return hasAnyRole(claims, ["Admin"]) || hasPerm(claims, code); +} + +export interface CapabilityFlags { + /** Shorthand for the Admin role (not the `manage:all` perm). */ + readonly isAdmin: boolean; + /** May curate the public /programas page (rules' `canCurateFeatured`). */ + readonly canFeatureInitiatives: boolean; + /** May SEAT a member on a cargo the plain non-Admin lane refuses — a power-granting one or + * a CEL one. Mirrors firestore.rules' `boardSeatDelegate()` disjunct for disjunct. + * Governs the member CREATE and UPDATE lanes only. */ + readonly canAssignBoardSeat: boolean; + /** May AUTHOR the positions catalog. Split from `canAssignBoardSeat` and deliberately NOT + * widened by the delegation: re-unifying them would hand a seat delegate the catalog, and + * the catalog is the door round the back — mint a grant-free CEL 'Presidente', then seat + * yourself on it at public board rank 0. */ + readonly canEditCargoCatalog: boolean; + /** May run `provisionMemberLogin`. Mirrors beacon's + * `requireAdminOrPerm(request, "create:MemberLogin")`. Cargo-agnostic. NOT the invite email + * itself — `requestPasswordReset` is a client-side `sendPasswordResetEmail` any signed-in + * user can already call. */ + readonly canProvisionLogin: boolean; +} + +export function capabilityFlags(claims: AuthClaims): CapabilityFlags { + return { + isAdmin: hasAnyRole(claims, ["Admin"]), + // Mirrors canCurateFeatured() in firestore.rules disjunct for disjunct: Admin by ROLE + // (locked + undeactivatable, so its name carries none of the staleness this gate fixes), + // everyone else by the update:Showcase PERM — so deactivating a role revokes curation, + // which the surviving role NAME in the claim would not. + // + // `hasPerm` is the client mirror of the rules' own `hasPerm()` — an exact code test on + // the claim, deliberately NOT `abilityAllows(..., "update", "Showcase")`: CASL's + // `manage:all` wildcard would answer yes to the ability question. That would show the + // Destacar checkbox to a manage:all perm holder whose write firestore.rules then + // rejects — taking the whole save down with it. `probe.ts` does not help here: it + // narrows CONDITIONAL grants, and the divergence is the unconditional wildcard. + canFeatureInitiatives: adminOrPerm(claims, "update:Showcase"), + // Same exact-code discipline as canFeatureInitiatives above, for the same reason: a + // `manage:all` holder must not see an affordance firestore.rules then rejects. + canAssignBoardSeat: adminOrPerm(claims, "update:BoardSeat"), + canEditCargoCatalog: hasAnyRole(claims, ["Admin"]), + canProvisionLogin: adminOrPerm(claims, "create:MemberLogin"), + }; +} diff --git a/apps/backstage/src/lib/authz/use-can.ts b/apps/backstage/src/lib/authz/use-can.ts index b6505fa0..98f400d8 100644 --- a/apps/backstage/src/lib/authz/use-can.ts +++ b/apps/backstage/src/lib/authz/use-can.ts @@ -1,8 +1,8 @@ import { useMemo } from "react"; -import { hasAnyRole, hasPerm, type AuthClaims, type Role } from "@luminova/auth/roles"; +import { hasAnyRole, type AuthClaims, type Role } from "@luminova/auth/roles"; import type { Action, AppAbility, Subject } from "@luminova/auth/ability"; -import type { PermissionCode } from "@luminova/types"; import type { ParticipationRole } from "@luminova/types/engine"; +import { capabilityFlags, type CapabilityFlags } from "./capability-flags"; import { isNavItemVisible, type NavItem } from "../../components/nav-config"; import { canRemoveEntry } from "../../features/check-in/lib/can-remove-entry"; import { isMemberOnly } from "./is-member-only"; @@ -13,7 +13,7 @@ import { abilityAllows, type SubjectFields } from "./probe"; * Firestore rules use: coarse `action:subject` perms (via the CASL ability) and * the built-in `roles` claim (Admin / ExecutiveCommittee / ProjectManager gates * that no perm expresses). Keeps the UI's affordances in lock-step with the rules. */ -export interface Can { +export interface Can extends CapabilityFlags { /** Perm gate. Without `on` this asks the COLLECTION-level question (unconditional * grants only); pass the document's fields to ask about one document. See * `abilityAllows` — a bare subject type would let a conditional own-doc grant @@ -26,46 +26,14 @@ export interface Can { navItemVisible(item: NavItem): boolean; /** May the caller undo THIS roster row? (features/check-in/lib/can-remove-entry) */ canRemoveCheckIn(entry: { role: ParticipationRole }): boolean; - /** Shorthand for the Admin role (not the `manage:all` perm). */ - readonly isAdmin: boolean; - /** May curate the public /programas page (rules' `canCurateFeatured`). Named here so the - * policy lives in one place, not scattered role-array literals at each call site. */ - readonly canFeatureInitiatives: boolean; - /** May SEAT a member on a cargo the plain non-Admin lane refuses — a power-granting one or - * a CEL one. Mirrors firestore.rules' `boardSeatDelegate()` disjunct for disjunct. - * Governs the member CREATE and UPDATE lanes only. */ - readonly canAssignBoardSeat: boolean; - /** May AUTHOR the positions catalog: create a board-surfacing cargo - * (`boardSurfacingCategory()`), and edit a stored cargo's `grants`, `category` or — on a - * board cargo — `title`/`titleFemale`. - * - * Split from `canAssignBoardSeat` and deliberately NOT widened by the delegation. These - * were one flag while both were `hasAnyRole(['Admin'])`; they are different authorities - * and firestore.rules now keys them on different predicates. Re-unifying them would hand - * a seat delegate the catalog, and the catalog is the door round the back: mint a - * grant-free CEL 'Presidente', then seat yourself on it at public board rank 0. */ - readonly canEditCargoCatalog: boolean; - /** May run `provisionMemberLogin` — create the member's Auth account, link their uid, get - * the password-reset link. Mirrors beacon's - * `requireAdminOrPerm(request, "create:MemberLogin")`. Cargo-agnostic: it applies to every - * new member, board seat or not. - * - * NOT the invite email itself — `requestPasswordReset` is a client-side - * `sendPasswordResetEmail` any signed-in user can already call. */ - readonly canProvisionLogin: boolean; } -/** The shape every delegable capability gate takes: the Admin ROLE, or one exact permission - * code. Extracted at the third occurrence — `hasPerm` is deliberately NOT `abilityAllows`, - * and re-deriving that decision per flag is how one of them ends up looser than its rule. - * See the canFeatureInitiatives comment below for the full reasoning it encodes. */ -function adminOrPerm(claims: AuthClaims, code: PermissionCode): boolean { - return hasAnyRole(claims, ["Admin"]) || hasPerm(claims, code); -} - -/** Pure builder — no React — so the gate logic is unit-testable. */ +/** Pure builder — no React — so the gate logic is unit-testable. The capability FLAGS come + * from `./capability-flags`, which the emulator parity test loads directly (this module pulls + * React and `@luminova/types` for value, so it cannot). */ export function buildCan(ability: AppAbility, claims: AuthClaims): Can { return { + ...capabilityFlags(claims), can: (action, subject, on) => abilityAllows(ability, action, subject, on), hasRole: (roles) => hasAnyRole(claims, roles), // A member-only user is bounced from `/` to `/me` by _app.index, so the Inicio @@ -77,24 +45,6 @@ export function buildCan(ability: AppAbility, claims: AuthClaims): Can { navItemVisible: (item) => isNavItemVisible(item, ability, claims) && !(item.to === "/" && isMemberOnly(claims)), canRemoveCheckIn: (entry) => canRemoveEntry(ability, claims, entry), - isAdmin: hasAnyRole(claims, ["Admin"]), - // Mirrors canCurateFeatured() in firestore.rules disjunct for disjunct: Admin by ROLE - // (locked + undeactivatable, so its name carries none of the staleness this gate fixes), - // everyone else by the update:Showcase PERM — so deactivating a role revokes curation, - // which the surviving role NAME in the claim would not. - // - // `hasPerm` is the client mirror of the rules' own `hasPerm()` — an exact code test on - // the claim, deliberately NOT `abilityAllows(..., "update", "Showcase")`: CASL's - // `manage:all` wildcard would answer yes to the ability question. That would show the - // Destacar checkbox to a manage:all perm holder whose write firestore.rules then - // rejects — taking the whole save down with it. `probe.ts` does not help here: it - // narrows CONDITIONAL grants, and the divergence is the unconditional wildcard. - canFeatureInitiatives: adminOrPerm(claims, "update:Showcase"), - // Same exact-code discipline as canFeatureInitiatives above, for the same reason: a - // `manage:all` holder must not see an affordance firestore.rules then rejects. - canAssignBoardSeat: adminOrPerm(claims, "update:BoardSeat"), - canEditCargoCatalog: hasAnyRole(claims, ["Admin"]), - canProvisionLogin: adminOrPerm(claims, "create:MemberLogin"), }; } diff --git a/apps/backstage/src/lib/use-copy-to-clipboard.test.ts b/apps/backstage/src/lib/use-copy-to-clipboard.test.ts new file mode 100644 index 00000000..712944a2 --- /dev/null +++ b/apps/backstage/src/lib/use-copy-to-clipboard.test.ts @@ -0,0 +1,110 @@ +import { describe, expect, it, afterEach, vi } from "vitest"; +import { act, renderHook } from "@testing-library/react"; +import { useCopyToClipboard } from "./use-copy-to-clipboard"; + +/** Install a `navigator.clipboard` for one test. `undefined` models an INSECURE CONTEXT, where + * the property does not exist at all — jsdom's navigator has no clipboard either, so this is + * the honest default rather than a contrivance. Configurable so afterEach can take it back. */ +function stubClipboard(clipboard: { writeText: (t: string) => Promise } | undefined) { + Object.defineProperty(navigator, "clipboard", { + value: clipboard, + configurable: true, + writable: true, + }); +} + +afterEach(() => { + Reflect.deleteProperty(navigator, "clipboard"); +}); + +describe("useCopyToClipboard", () => { + it("starts idle and writes the text through to the clipboard", async () => { + const writeText = vi.fn().mockResolvedValue(undefined); + stubClipboard({ writeText }); + const { result } = renderHook(() => useCopyToClipboard()); + expect(result.current.copyState).toBe("idle"); + await act(async () => result.current.copy("https://example.com/link")); + expect(writeText).toHaveBeenCalledWith("https://example.com/link"); + expect(result.current.copyState).toBe("copied"); + }); + + it("reports a rejected write as failed", async () => { + stubClipboard({ writeText: vi.fn().mockRejectedValue(new Error("denied")) }); + const { result } = renderHook(() => useCopyToClipboard()); + await act(async () => result.current.copy("x")); + expect(result.current.copyState).toBe("failed"); + }); + + // BLOCKING: the reason this hook exists at all. Outside a secure context (plain http, an + // embedded webview) `navigator.clipboard` is `undefined`, so `navigator.clipboard.writeText` + // throws a TypeError SYNCHRONOUSLY — there is no promise, so the `.catch()` both call sites + // used to hang off the call never ran. The failure affordance ("selecciona el enlace y + // cópialo manualmente") is the ONE case that surface exists for, and it was exactly the case + // that instead threw out of the onClick handler and rendered nothing. + it("BLOCKING: reports a synchronous throw (no clipboard API) as failed, not an exception", async () => { + stubClipboard(undefined); + const { result } = renderHook(() => useCopyToClipboard()); + expect(() => act(() => result.current.copy("x"))).not.toThrow(); + expect(result.current.copyState).toBe("failed"); + }); + + // The same shape one layer in: a clipboard object whose writeText throws rather than + // rejecting. Pinned separately because a fix that only null-checked `navigator.clipboard` + // would pass the case above and still throw here. + it("BLOCKING: reports a writeText that THROWS rather than rejecting as failed", async () => { + stubClipboard({ + writeText: () => { + throw new Error("not allowed"); + }, + }); + const { result } = renderHook(() => useCopyToClipboard()); + expect(() => act(() => result.current.copy("x"))).not.toThrow(); + expect(result.current.copyState).toBe("failed"); + }); + + it("resetCopyState returns to idle so a re-opened surface does not show a stale result", async () => { + stubClipboard({ writeText: vi.fn().mockResolvedValue(undefined) }); + const { result } = renderHook(() => useCopyToClipboard()); + await act(async () => result.current.copy("x")); + expect(result.current.copyState).toBe("copied"); + act(() => result.current.resetCopyState()); + expect(result.current.copyState).toBe("idle"); + }); + + // BLOCKING: both callbacks are useCallback-wrapped, and the identity is the contract, not an + // optimization. The invite drawer captures `resetCopyState` in `reset()`, `reset()` in + // `close()`, and hands `close()` to the Sheet's `onOpenChange` — a fresh closure per render + // changes that prop on every render of the drawer. Asserted across a rerender AND across a + // state change, because a `useCallback(fn, [copyState])` would pass the first and fail the + // second while looking stable in casual use. + it("keeps copy and resetCopyState referentially stable across renders", async () => { + stubClipboard({ writeText: vi.fn().mockResolvedValue(undefined) }); + const { result, rerender } = renderHook(() => useCopyToClipboard()); + const firstCopy = result.current.copy; + const firstReset = result.current.resetCopyState; + + rerender(); + expect(result.current.copy).toBe(firstCopy); + expect(result.current.resetCopyState).toBe(firstReset); + + // …and still stable after the hook's own state moves, which is when a dependency-carrying + // callback would quietly get a new identity. + await act(async () => result.current.copy("x")); + expect(result.current.copyState).toBe("copied"); + expect(result.current.copy).toBe(firstCopy); + expect(result.current.resetCopyState).toBe(firstReset); + }); + + // A retry after a failure has to be able to succeed: `copy` sets state on BOTH outcomes, so + // nothing has to be reset in between. If it only ever set "failed" the button would stay + // wrong for the rest of the session. + it("a later successful copy overwrites an earlier failure", async () => { + stubClipboard(undefined); + const { result } = renderHook(() => useCopyToClipboard()); + act(() => result.current.copy("x")); + expect(result.current.copyState).toBe("failed"); + stubClipboard({ writeText: vi.fn().mockResolvedValue(undefined) }); + await act(async () => result.current.copy("x")); + expect(result.current.copyState).toBe("copied"); + }); +}); diff --git a/apps/backstage/src/lib/use-copy-to-clipboard.ts b/apps/backstage/src/lib/use-copy-to-clipboard.ts new file mode 100644 index 00000000..9c647888 --- /dev/null +++ b/apps/backstage/src/lib/use-copy-to-clipboard.ts @@ -0,0 +1,39 @@ +import { useCallback, useState } from "react"; + +export type CopyState = "idle" | "copied" | "failed"; + +/** + * Copy-to-clipboard with a result the caller can render. Both places that offer a + * password-reset link as a manual fallback need exactly this, and a second copy would drift the + * way the cargo predicates already did once. + * + * `navigator.clipboard` is `undefined` outside a secure context, so the property access throws + * SYNCHRONOUSLY — a bare `.catch()` on the returned promise never runs and the failure + * affordance never renders, which is the one case it exists for. Hence the try/catch around the + * call itself, not only the rejection. + */ +export function useCopyToClipboard(): { + copyState: CopyState; + copy: (text: string) => void; + resetCopyState: () => void; +} { + const [copyState, setCopyState] = useState("idle"); + // Stable identities as a cheap property of a shared hook, NOT because a consumer depends on + // it today: both call sites wrap these in unmemoized handlers and pass inline arrows, so + // nothing downstream currently observes the difference. Kept so a future memoized consumer + // is not defeated by the hook itself. + const copy = useCallback((text: string) => { + try { + void navigator.clipboard + .writeText(text) + .then(() => setCopyState("copied")) + .catch(() => setCopyState("failed")); + } catch { + // No clipboard API at all (insecure context, embedded webview). Same outcome as a + // rejected write: tell the user to select the text themselves. + setCopyState("failed"); + } + }, []); + const resetCopyState = useCallback(() => setCopyState("idle"), []); + return { copyState, copy, resetCopyState }; +} diff --git a/apps/beacon/src/callable-auth.test.ts b/apps/beacon/src/callable-auth.test.ts index 9a9357e0..f6769481 100644 --- a/apps/beacon/src/callable-auth.test.ts +++ b/apps/beacon/src/callable-auth.test.ts @@ -84,8 +84,9 @@ describe("requireAdminOrPerm", () => { }); it("keeps the two delegations independent", () => { - // A board-seat delegate is not a login provisioner and vice versa. Pinned because both - // codes ship together and the obvious future mistake is to conflate them. + // A board-seat delegate is not a login provisioner and vice versa. Pins the gate against a + // widening that keys on the subject FAMILY (e.g. "holds any MemberLogin/BoardSeat + // delegation") — every other case here is same-subject and would still pass. expect( codeOf(() => requireAdminOrPerm( @@ -115,7 +116,7 @@ describe("callerIsAdmin", () => { expect(callerIsAdmin(req())).toBe(false); expect(callerIsAdmin(req({ roles: ["Member"], perms: ["manage:all"] }))).toBe(false); }); - it("is true only for the Admin role", () => { + it("is true for the Admin role", () => { expect(callerIsAdmin(req({ roles: ["Admin"] }))).toBe(true); }); }); diff --git a/apps/beacon/src/claims-sync/firestore-deps.test.ts b/apps/beacon/src/claims-sync/firestore-deps.test.ts index 3e7d33fd..fcde16ec 100644 --- a/apps/beacon/src/claims-sync/firestore-deps.test.ts +++ b/apps/beacon/src/claims-sync/firestore-deps.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import type { Auth } from "firebase-admin/auth"; import type { Firestore } from "firebase-admin/firestore"; import { firestoreClaimsDeps } from "./firestore-deps.js"; +import { truncateForLog } from "../firestore-util.js"; type RoleFixture = { id: string; data: Record }; @@ -70,6 +71,21 @@ function fakeDb(fixtures: RoleFixture[]) { /** Cast, not a fabricated Auth: every dep exercised here is a pure Firestore read. */ const auth = {} as Auth; +/** An Auth stub narrow enough to drive `auth.getUser(uid)` — the only Auth surface the claims + * accessors reach. The cast is test-only and justified: UserRecord carries a dozen fields + * (metadata, providerData, toJSON) that nothing under test reads, and fabricating them would + * assert nothing. A missing uid throws the real `auth/user-not-found` code, because that is + * the ONE error getUserOrNull swallows. */ +function fakeAuth(users: Record): Auth { + return { + getUser: async (uid: string) => { + const user = users[uid]; + if (!user) throw Object.assign(new Error("user not found"), { code: "auth/user-not-found" }); + return user; + }, + } as unknown as Auth; +} + const builtIn = (id: string, key: string, extra: Record = {}): RoleFixture => ({ id, data: { builtIn: true, builtInKey: key, permissions: ["read:Member"], active: true, ...extra }, @@ -87,6 +103,32 @@ afterEach(() => { vi.restoreAllMocks(); }); +// Both sinks are OPTIONAL on ClaimsSyncDeps, so their presence in production rests entirely on +// this factory — an omission would compile, pass every unit test (the fakes supply their own), +// and silently drop the very lines the fail-closed screens were instrumented to emit. Pin that +// the real factory wires both, and that they are distinct: routing the designed refusal back +// into the error stream is the mistake the severity split exists to prevent. +describe("firestoreClaimsDeps log sinks", () => { + it("BLOCKING: supplies BOTH logError and logWarn, and they are not the same sink", () => { + const deps = firestoreClaimsDeps({} as unknown as Firestore, {} as unknown as Auth); + expect(typeof deps.logError).toBe("function"); + expect(typeof deps.logWarn).toBe("function"); + expect(deps.logWarn).not.toBe(deps.logError); + }); + + it("routes them to console.warn and console.error respectively", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const deps = firestoreClaimsDeps({} as unknown as Firestore, {} as unknown as Auth); + deps.logWarn?.("w", { a: 1 }); + deps.logError?.("e", { b: 2 }); + expect(warn).toHaveBeenCalledWith("w", { a: 1 }); + expect(error).toHaveBeenCalledWith("e", { b: 2 }); + warn.mockRestore(); + error.mockRestore(); + }); +}); + describe("getRoleDocsByBuiltInKeys coverage anomalies", () => { it("logs nothing for a well-formed built-in doc", async () => { const errors = captureErrors(); @@ -153,6 +195,48 @@ describe("getRoleDocsByBuiltInKeys coverage anomalies", () => { }); }); +/** The wire that carries the assigner's claims into resolveTrustedGrants' power-grant trust + * gate. sync.test.ts drives that gate through an in-memory fake, so this is the only place the + * real accessor's filtering is exercised — and a `perms` array that arrived unfiltered would + * hand the gate a junk code to match on. */ +describe("getAssignerClaims", () => { + it("BLOCKING: round-trips roles and perms, dropping entries outside the vocabulary", async () => { + const { db } = fakeDb([]); + const deps = firestoreClaimsDeps( + db, + fakeAuth({ + "delegate-uid": { + customClaims: { + roles: ["Member", "NotARole", 42], + perms: ["update:BoardSeat", "nope:Thing", null], + }, + }, + }), + ); + await expect(deps.getAssignerClaims("delegate-uid")).resolves.toEqual({ + roles: ["Member"], + perms: ["update:BoardSeat"], + }); + }); + + it("fails closed to empty claims for an absent, claimless or malformed-claim assigner", async () => { + // Unlike getExistingClaims, `perms` is never undefined here: the gate does an `.includes()` + // on it, so absence must arrive as an empty array and DENY rather than throw. + const { db } = fakeDb([]); + const deps = firestoreClaimsDeps( + db, + fakeAuth({ + claimless: {}, + malformed: { customClaims: { roles: "Admin", perms: "update:BoardSeat" } }, + }), + ); + const empty = { roles: [], perms: [] }; + await expect(deps.getAssignerClaims("ghost")).resolves.toEqual(empty); + await expect(deps.getAssignerClaims("claimless")).resolves.toEqual(empty); + await expect(deps.getAssignerClaims("malformed")).resolves.toEqual(empty); + }); +}); + describe("getRolesByIds id screening", () => { const custom = (id: string, extra: Record = {}): RoleFixture => ({ id, @@ -189,15 +273,22 @@ describe("getRolesByIds id screening", () => { const { db } = fakeDb([]); const junk = Array.from({ length: 10_000 }, (_, i) => `bad/${i}`); const longId = `x/${"y".repeat(5_000)}`; - expect(await firestoreClaimsDeps(db, auth).getRolesByIds([...junk, longId])).toEqual([]); + // FIRST, not last. `sampleRejectedIds` takes `.slice(0, 10)`, so appending the one + // oversized id after 10,000 short ones put it outside the sampled window entirely: the + // per-entry length assertion below then only ever saw 9-character ids and could not fail. + // Deleting `.map(truncateForLog)` left this test green, which is the failure it exists to + // catch — a member doc whose FIRST junk roleId is 1,500 bytes serializes raw. + expect(await firestoreClaimsDeps(db, auth).getRolesByIds([longId, ...junk])).toEqual([]); const meta = errors[0]?.[1] as { rejectedCount: number; rejectedSample: string[]; }; expect(meta.rejectedCount).toBe(10_001); expect(meta.rejectedSample).toHaveLength(10); - // Every sampled entry is length-capped too, so one enormous id cannot blow the budget - // through the sample either. + // The oversized id is in the window, and truncated — so one enormous id cannot blow the + // budget through the sample either. + expect(meta.rejectedSample[0]).toBe(truncateForLog(longId)); + expect(meta.rejectedSample[0]).toHaveLength(65); for (const entry of meta.rejectedSample) expect(entry.length).toBeLessThanOrEqual(65); expect(JSON.stringify(meta).length).toBeLessThan(2_000); }); diff --git a/apps/beacon/src/claims-sync/firestore-deps.ts b/apps/beacon/src/claims-sync/firestore-deps.ts index f81c4958..840d20fd 100644 --- a/apps/beacon/src/claims-sync/firestore-deps.ts +++ b/apps/beacon/src/claims-sync/firestore-deps.ts @@ -3,7 +3,7 @@ import type { Firestore } from "firebase-admin/firestore"; import { isValidRole, type Role } from "@luminova/auth/roles"; import { isValidPermissionCode, type PermissionCode } from "@luminova/types/permission"; import { chunk } from "../chunk.js"; -import { isSafeDocId } from "../firestore-util.js"; +import { isSafeDocId, logError, logWarn, truncateForLog } from "../firestore-util.js"; import { readPositionGrants } from "../read-position-grants.js"; import { isActiveRoleDoc, permsFromRoleDoc } from "./role-doc.js"; import type { LiveBuiltInRoleDoc } from "./resolve-member-perms.js"; @@ -27,16 +27,16 @@ function permsFromClaims( * `roleIds` is Admin-writable with no size or length cap in rules, so serializing every * rejected entry lets one junk-filled member doc exceed Cloud Logging's 256 KB per-entry * limit — the entry is then DROPPED, making the anomaly invisible at exactly the scale - * that matters. The count is the alertable signal; the sample is for diagnosis. */ + * that matters. The count is the alertable signal; the sample is for diagnosis. + * + * REACH: this bounds an ALL-STRINGS array carrying an id `isSafeDocId` rejects, and nothing + * else. A junk-FILLED `roleIds` (any non-string entry) never arrives — `parseMember` replaces + * a mixed-type array with `[]` upstream, and reports that drop itself. Do not read the screen + * below as covering both. */ const REJECTED_ID_SAMPLE = 10; -const REJECTED_ID_MAX_CHARS = 64; function sampleRejectedIds(rejected: readonly string[]): string[] { - return rejected - .slice(0, REJECTED_ID_SAMPLE) - .map((id) => - id.length > REJECTED_ID_MAX_CHARS ? `${id.slice(0, REJECTED_ID_MAX_CHARS)}…` : id, - ); + return rejected.slice(0, REJECTED_ID_SAMPLE).map(truncateForLog); } /** The built-in role docs covering `keys`, plus a log line for every coverage anomaly the @@ -186,6 +186,12 @@ export interface FirestoreClaimsDeps extends ClaimsSyncDeps { staleBuiltInRoleKeys(): Promise; } +/** Re-exported so the trigger/callable call sites can hand the SAME sink to `parseMember`, + * which runs before any deps instance exists. The sink itself lives in `firestore-util.ts` + * alongside `LogSink` — a second `console.error` wrapper per adapter is the copy guardrail #1 + * forbids, and there WAS one in `provision-deps.ts` while this comment claimed otherwise. */ +export { logError }; + export function firestoreClaimsDeps(db: Firestore, auth: Auth): FirestoreClaimsDeps { const userCache = new Map>(); function loadUser(uid: string): Promise { @@ -280,7 +286,7 @@ export function firestoreClaimsDeps(db: Firestore, auth: Auth): FirestoreClaimsD // the port-independent gate the test fakes inherit), but readPositionGrants screens // again at the site where an unscreened id becomes a permanent db.doc() throw, so this // does not rely on its caller. - const grants = await readPositionGrants(db, id); + const grants = await readPositionGrants(db, id, logError); return grants === null ? null : { grants }; }, getAssignerClaims: async (uid) => { @@ -346,6 +352,9 @@ export function firestoreClaimsDeps(db: Firestore, auth: Auth): FirestoreClaimsD setClaims: async (uid, next) => { await auth.setCustomUserClaims(uid, next); }, - logError: (message, meta) => console.error(message, meta), + logError, + // Cloud Logging maps console.warn to WARNING, which is the point: the designed refusals + // must not share a severity with the malformed-doc screens an operator has to act on. + logWarn, }; } diff --git a/apps/beacon/src/claims-sync/parse-member.test.ts b/apps/beacon/src/claims-sync/parse-member.test.ts index f598815b..d0df43e0 100644 --- a/apps/beacon/src/claims-sync/parse-member.test.ts +++ b/apps/beacon/src/claims-sync/parse-member.test.ts @@ -1,16 +1,42 @@ import { describe, expect, it } from "vitest"; -import { parseMember } from "./parse-member.js"; +import { parseMember, type MemberParseContext } from "./parse-member.js"; + +/** For tests exercising pure parse behavior with nothing to log to — `context` is required + * on `parseMember` precisely so a real call site can't get away with omitting one. */ +const silent = (): MemberParseContext => ({ memberId: "m-test", logError: () => {} }); + +/** A capturing LogSink plus the context that feeds it. Not `vi.fn()`: these assertions are + * about the CONTENT of the meta object (ids, types, counts — and nothing else), which reads + * better against a plain array than through mock-call indexing. */ +function recorder(): { + lines: { message: string; meta: Record }[]; + context: MemberParseContext; +} { + const lines: { message: string; meta: Record }[] = []; + return { + lines, + context: { + memberId: "member-1", + logError: (message, meta) => { + lines.push({ message, meta }); + }, + }, + }; +} describe("parseMember", () => { const NO_GRANTS = { grant: [], revoke: [] }; it("passes through a well-formed member (incl. assignedBy)", () => { - const result = parseMember({ - uid: "u1", - positions: { - "2026": { cargoId: "pos-pres", comisionIds: ["com-a", "com-b"], assignedBy: "admin-uid" }, + const result = parseMember( + { + uid: "u1", + positions: { + "2026": { cargoId: "pos-pres", comisionIds: ["com-a", "com-b"], assignedBy: "admin-uid" }, + }, }, - }); + silent(), + ); expect(result).toEqual({ uid: "u1", positions: { @@ -22,12 +48,15 @@ describe("parseMember", () => { }); it("extracts roleIds and filters override codes to the known vocabulary", () => { - const result = parseMember({ - uid: "u1", - positions: {}, - roleIds: ["custom-1", "custom-2"], - permissionOverrides: { grant: ["manage:Position", "bogus:Code"], revoke: ["read:Member"] }, - }); + const result = parseMember( + { + uid: "u1", + positions: {}, + roleIds: ["custom-1", "custom-2"], + permissionOverrides: { grant: ["manage:Position", "bogus:Code"], revoke: ["read:Member"] }, + }, + silent(), + ); expect(result.roleIds).toEqual(["custom-1", "custom-2"]); expect(result.permissionOverrides).toEqual({ grant: ["manage:Position"], @@ -36,34 +65,44 @@ describe("parseMember", () => { }); it("defaults roleIds to [] and overrides to empty when absent or malformed", () => { - expect(parseMember({ uid: "u1", positions: {}, roleIds: "nope" }).roleIds).toEqual([]); - expect(parseMember({ uid: "u1", positions: {} }).permissionOverrides).toEqual(NO_GRANTS); + expect(parseMember({ uid: "u1", positions: {}, roleIds: "nope" }, silent()).roleIds).toEqual( + [], + ); + expect(parseMember({ uid: "u1", positions: {} }, silent()).permissionOverrides).toEqual( + NO_GRANTS, + ); expect( - parseMember({ uid: "u1", positions: {}, permissionOverrides: { grant: "x" } }) + parseMember({ uid: "u1", positions: {}, permissionOverrides: { grant: "x" } }, silent()) .permissionOverrides, ).toEqual(NO_GRANTS); }); it("drops a term whose comisionIds is present but not a string array", () => { - const result = parseMember({ - uid: "u1", - positions: { - good: { cargoId: "p", comisionIds: ["c"] }, - bad: { cargoId: "p", comisionIds: "not-an-array" }, - alsoBad: { cargoId: "p", comisionIds: [1, 2] }, + const result = parseMember( + { + uid: "u1", + positions: { + good: { cargoId: "p", comisionIds: ["c"] }, + bad: { cargoId: "p", comisionIds: "not-an-array" }, + alsoBad: { cargoId: "p", comisionIds: [1, 2] }, + }, }, - }); + silent(), + ); expect(result.positions).toEqual({ good: { cargoId: "p", comisionIds: ["c"] } }); }); it("defaults an absent comisionIds to [] when cargoId is valid", () => { - const result = parseMember({ - uid: "u1", - positions: { - "2026": { cargoId: "pos-pres", assignedBy: "admin-uid" }, - nullCargo: { cargoId: null }, + const result = parseMember( + { + uid: "u1", + positions: { + "2026": { cargoId: "pos-pres", assignedBy: "admin-uid" }, + nullCargo: { cargoId: null }, + }, }, - }); + silent(), + ); expect(result.positions["2026"]).toEqual({ cargoId: "pos-pres", comisionIds: [], @@ -73,41 +112,215 @@ describe("parseMember", () => { }); it("yields empty positions when positions is an array", () => { - expect(parseMember({ uid: "u1", positions: [] }).positions).toEqual({}); - expect(parseMember({ uid: "u1", positions: ["x"] }).positions).toEqual({}); + expect(parseMember({ uid: "u1", positions: [] }, silent()).positions).toEqual({}); + expect(parseMember({ uid: "u1", positions: ["x"] }, silent()).positions).toEqual({}); }); it("yields empty positions when positions is a string", () => { - expect(parseMember({ uid: "u1", positions: "nope" }).positions).toEqual({}); + expect(parseMember({ uid: "u1", positions: "nope" }, silent()).positions).toEqual({}); }); it("returns undefined uid when missing or non-string", () => { - expect(parseMember({ positions: {} }).uid).toBeUndefined(); - expect(parseMember({ uid: 42, positions: {} }).uid).toBeUndefined(); + expect(parseMember({ positions: {} }, silent()).uid).toBeUndefined(); + expect(parseMember({ uid: 42, positions: {} }, silent()).uid).toBeUndefined(); }); it("preserves a null cargoId", () => { - const result = parseMember({ - uid: "u1", - positions: { "2026": { cargoId: null, comisionIds: [] } }, - }); + const result = parseMember( + { + uid: "u1", + positions: { "2026": { cargoId: null, comisionIds: [] } }, + }, + silent(), + ); expect(result.positions["2026"]).toEqual({ cargoId: null, comisionIds: [] }); }); it("drops a term whose cargoId is missing or non-string/non-null", () => { - const result = parseMember({ - uid: "u1", - positions: { - missing: { comisionIds: [] }, - numeric: { cargoId: 7, comisionIds: [] }, + const result = parseMember( + { + uid: "u1", + positions: { + missing: { comisionIds: [] }, + numeric: { cargoId: 7, comisionIds: [] }, + }, }, - }); + silent(), + ); expect(result.positions).toEqual({}); }); it("handles null/undefined raw input", () => { const empty = { uid: undefined, positions: {}, roleIds: [], permissionOverrides: NO_GRANTS }; - expect(parseMember(null)).toEqual(empty); - expect(parseMember(undefined)).toEqual(empty); + expect(parseMember(null, silent())).toEqual(empty); + expect(parseMember(undefined, silent())).toEqual(empty); + }); + + it("logs a present-but-non-string uid — the member is skipped by both fan-outs", () => { + const { lines, context } = recorder(); + expect(parseMember({ uid: 42, positions: {} }, context).uid).toBeUndefined(); + expect(lines).toHaveLength(1); + expect(lines[0]?.message).toContain("uid is present but not a string"); + expect(lines[0]?.meta).toEqual({ memberId: "member-1", uidType: "number" }); + }); + + it("logs a positions that is not a map, naming the shape", () => { + const cases: { positions: unknown; positionsType: string }[] = [ + { positions: [], positionsType: "array" }, + { positions: ["x"], positionsType: "array" }, + { positions: "nope", positionsType: "string" }, + // Falsy non-objects used to take the same silent branch as an ABSENT positions. + { positions: false, positionsType: "boolean" }, + ]; + for (const { positions, positionsType } of cases) { + const { lines, context } = recorder(); + expect(parseMember({ uid: "u1", positions }, context).positions).toEqual({}); + expect(lines).toHaveLength(1); + expect(lines[0]?.message).toContain("positions is not a map"); + expect(lines[0]?.meta).toEqual({ memberId: "member-1", positionsType }); + } + }); + + it("logs every dropped term entry in ONE bounded line, naming the term and the shape", () => { + const { lines, context } = recorder(); + // The reachable case this log exists for: `comisionIds` is a string, so the WHOLE 2026 + // entry goes — cargoId and assignedBy with it — and the sitting president reads as + // holding no seat everywhere downstream. firestore.rules type-checks none of this. + const result = parseMember( + { + uid: "pres", + positions: { + "2022": ["x"], + "2023": "nope", + "2024": { comisionIds: [] }, + "2025": { cargoId: 7, comisionIds: [] }, + "2026": { cargoId: "pos-presidente", assignedBy: "admin-uid", comisionIds: "COM-1" }, + kept: { cargoId: null, comisionIds: [] }, + }, + }, + context, + ); + expect(result.positions).toEqual({ kept: { cargoId: null, comisionIds: [] } }); + expect(lines).toHaveLength(1); + expect(lines[0]?.message).toContain("position entries are malformed"); + // Integer-like keys iterate numerically first, then insertion order — hence 2022…2026, kept. + expect(lines[0]?.meta).toEqual({ + memberId: "member-1", + droppedCount: 5, + dropped: [ + { term: "2022", reason: "term-entry-not-a-map:array" }, + { term: "2023", reason: "term-entry-not-a-map:string" }, + { term: "2024", reason: "cargo-id-not-a-string:undefined" }, + { term: "2025", reason: "cargo-id-not-a-string:number" }, + { term: "2026", reason: "comision-ids-not-a-string-array:string" }, + ], + }); + + // BOUNDED, in the same line: the count stays exact while the sample caps, because a + // console or migration write can author arbitrarily many term keys and an over-large + // Cloud Logging entry is dropped whole. + const many = recorder(); + const term = (i: number) => `t${i}-${"x".repeat(80)}`; + const positions = Object.fromEntries( + Array.from({ length: 25 }, (_, i) => [term(i), { cargoId: 7 }]), + ); + expect(parseMember({ uid: "u1", positions }, many.context).positions).toEqual({}); + expect(many.lines).toHaveLength(1); + expect(many.lines[0]?.meta).toEqual({ + memberId: "member-1", + droppedCount: 25, + // Ten rows, and every named term truncated to the shared 64-char log cap. + dropped: Array.from({ length: 10 }, (_, i) => ({ + term: `${term(i).slice(0, 64)}…`, + reason: "cargo-id-not-a-string:number", + })), + }); + }); + + it("logs a roleIds that is not an array of strings — valid entries go with it", () => { + const { lines, context } = recorder(); + expect( + parseMember({ uid: "u1", positions: {}, roleIds: ["ok", 7, null] }, context).roleIds, + ).toEqual([]); + expect(lines).toHaveLength(1); + expect(lines[0]?.message).toContain("roleIds is not an array of strings"); + expect(lines[0]?.meta).toEqual({ + memberId: "member-1", + roleIdsType: "array", + entryCount: 3, + nonStringCount: 2, + }); + + const scalar = recorder(); + parseMember({ uid: "u1", positions: {}, roleIds: "nope" }, scalar.context); + expect(scalar.lines[0]?.meta).toEqual({ + memberId: "member-1", + roleIdsType: "string", + entryCount: null, + nonStringCount: null, + }); + }); + + it("logs dropped permissionOverrides — the map shape, a non-array arm, unknown codes", () => { + const notAMap = recorder(); + parseMember({ uid: "u1", positions: {}, permissionOverrides: ["manage:all"] }, notAMap.context); + expect(notAMap.lines).toHaveLength(1); + expect(notAMap.lines[0]?.message).toContain("permissionOverrides entries are malformed"); + expect(notAMap.lines[0]?.meta).toEqual({ + memberId: "member-1", + problems: [{ field: "permissionOverrides", reason: "not-a-map", valueType: "array" }], + }); + + const arms = recorder(); + const result = parseMember( + { + uid: "u1", + positions: {}, + permissionOverrides: { grant: "update:BoardSeat", revoke: ["read:Member", "bogus:Code"] }, + }, + arms.context, + ); + expect(result.permissionOverrides).toEqual({ grant: [], revoke: ["read:Member"] }); + expect(arms.lines).toHaveLength(1); + // Counts, never the rejected code itself: an unknown code is by definition unbounded free + // text off a console edit. + expect(arms.lines[0]?.meta).toEqual({ + memberId: "member-1", + problems: [ + { field: "grant", reason: "not-an-array", valueType: "string" }, + { field: "revoke", reason: "unknown-code", count: 1 }, + ], + }); + }); + + it("stays SILENT on every ordinary member shape", () => { + // The paired negative for all five screens above. These are the shapes of nearly every + // member write — an unprovisioned member, a rank-and-file member with no seat, the + // explicit nulls the rules' unchanged()/touched() gap admits. Logging any of them would + // fire constantly and bury the anomalies, which is the whole reason the screens in + // sync.ts guard on `rejectedCargoId !== null`. + const ordinary: unknown[] = [ + null, + undefined, + {}, + { uid: "u1" }, + { uid: null, positions: {} }, + { uid: "u1", positions: null }, + { positions: { "2026": { cargoId: null, comisionIds: [] } } }, + { uid: "u1", positions: { "2026": { cargoId: "pos-pres" } } }, + { uid: "u1", positions: { "2026": { cargoId: "p", comisionIds: ["c"], assignedBy: "a" } } }, + { uid: "u1", roleIds: null }, + { uid: "u1", roleIds: [] }, + { uid: "u1", roleIds: ["custom-1"] }, + { uid: "u1", permissionOverrides: null }, + { uid: "u1", permissionOverrides: {} }, + { uid: "u1", permissionOverrides: { grant: null, revoke: undefined } }, + { uid: "u1", permissionOverrides: { grant: ["manage:Position"], revoke: ["read:Member"] } }, + ]; + for (const shape of ordinary) { + const { lines, context } = recorder(); + parseMember(shape, context); + expect(lines).toEqual([]); + } }); }); diff --git a/apps/beacon/src/claims-sync/parse-member.ts b/apps/beacon/src/claims-sync/parse-member.ts index 319793be..f824364d 100644 --- a/apps/beacon/src/claims-sync/parse-member.ts +++ b/apps/beacon/src/claims-sync/parse-member.ts @@ -1,5 +1,6 @@ import type { TermPositions } from "@luminova/types"; import { isValidPermissionCode, type PermissionCode } from "@luminova/types/permission"; +import { truncateForLog, type LogSink } from "../firestore-util.js"; export interface SafeMember { uid?: string; @@ -11,6 +12,31 @@ export interface SafeMember { /** Member fields the claims-sync needs — used to project member-collection scans. */ export const MEMBER_SYNC_FIELDS = ["uid", "positions", "roleIds", "permissionOverrides"] as const; +/** Who to blame and where to say it, for the drops below. + * + * REQUIRED, not optional: an optional sink compiles fine at a future call site that forgets + * it, silently reinstating the exact silent-drop bug this parameter was added to close. Tests + * that only exercise pure parse behavior pass a no-op `silent()` context (see + * parse-member.test.ts) instead of getting a free pass to omit one. */ +export interface MemberParseContext { + /** The Firestore doc id (`members/{id}`), NOT `member.uid`. Both are handles on the same + * doc, but only the doc id survives what is being reported here: `uid` is itself one of the + * fields this parser can find absent or non-string, so a line keyed on it would be blank + * for exactly the docs an operator has to open. Every call site has the doc id + * unconditionally (`event.params.id` / `doc.id`); none has a trustworthy uid yet. Matches + * the `memberId` field the sibling fan-out lines in index.ts already use. */ + memberId: string; + logError: LogSink; +} + +/** Malformed entries named in one line, per collection. Same rationale as REJECTED_ID_SAMPLE + * in firestore-deps.ts, one axis over: `positions` is a map with no key-count cap in + * firestore.rules, so a console or migration write can produce arbitrarily many bad term + * entries. One line per entry would be arbitrarily many Cloud Logging entries per member + * write; serializing all of them into one would push that entry past the 256 KB limit and get + * it DROPPED. The count is the alertable signal, the sample is for diagnosis. */ +const DROPPED_ENTRY_SAMPLE = 10; + function isStringArray(v: unknown): v is string[] { return Array.isArray(v) && v.every((x) => typeof x === "string"); } @@ -19,40 +45,171 @@ function permissionCodes(v: unknown): PermissionCode[] { return Array.isArray(v) ? v.filter((x): x is PermissionCode => isValidPermissionCode(x)) : []; } +/** `typeof`, but distinguishing the two shapes that matter most here and that `typeof` calls + * `"object"`: an array (a `positions` written as a list) and an explicit null. */ +function shapeOf(v: unknown): string { + return Array.isArray(v) ? "array" : v === null ? "null" : typeof v; +} + +/** ABSENT, for every field below: `undefined` or an explicit `null`. Both are the ordinary + * shape of an ordinary member — and null specifically is admitted by the rules' + * unchanged()/touched() gap (see hasDirectGrants in provision-member-login.ts), so it reaches + * here on real docs. Neither is an anomaly, and logging them would fire on nearly every + * member write, burying the anomalies in the noise. Same reason the `rejectedCargoId !== null` + * guard in sync.ts exists. */ +function isAbsent(v: unknown): boolean { + return v === undefined || v === null; +} + /** Extract a structurally-safe member from raw Firestore data. Malformed term * entries are dropped (not thrown) so a bad doc can't cause a retry storm. * An absent comisionIds defaults to [] (the cargo grant is still honored); * a present-but-malformed comisionIds drops the entry. roleIds defaults to [] - * when absent/malformed; override codes are filtered to the known vocabulary. */ -export function parseMember(raw: unknown): SafeMember { + * when absent/malformed; override codes are filtered to the known vocabulary. + * + * DROPPED IS NOT SILENT (guardrail #4). Every drop below is a fail-closed decision taken on + * data no client could have written — firestore.rules type-checks none of these fields — and + * the consequences are load-bearing: dropping one malformed `comisionIds` discards the whole + * term entry, `cargoId` and `assignedBy` with it, which reads downstream as "holds no seat" + * and strips a sitting president's Admin role from their claims on their next member write. + * Nothing further down can report that, because by then the shape is gone. So each drop emits + * one bounded line here, keyed on the doc id, carrying ids/types/counts only — never the doc, + * never PII. `context` is required — see MemberParseContext for why. */ +export function parseMember(raw: unknown, context: MemberParseContext): SafeMember { const data = (raw ?? {}) as { uid?: unknown; positions?: unknown; roleIds?: unknown; permissionOverrides?: unknown; }; + const report = (message: string, meta: Record): void => { + context.logError(message, { memberId: truncateForLog(context.memberId), ...meta }); + }; + const uid = typeof data.uid === "string" ? data.uid : undefined; + if (uid === undefined && !isAbsent(data.uid)) { + // Reported, not just dropped: both fan-outs skip a member with no uid via `if + // (!member.uid) continue`, so a non-string uid removes that member from the sync + // ENTIRELY — the one drop here that costs a member every grant at once. + report( + "claims-sync: member uid is present but not a string — the member reads as unprovisioned, so their claims are never synced", + { + uidType: shapeOf(data.uid), + }, + ); + } + const positions: Record = {}; - if (data.positions && typeof data.positions === "object" && !Array.isArray(data.positions)) { - for (const [term, value] of Object.entries(data.positions as Record)) { - const v = value as { cargoId?: unknown; comisionIds?: unknown; assignedBy?: unknown }; - if (!v || typeof v !== "object") continue; - const cargoId = - typeof v.cargoId === "string" ? v.cargoId : v.cargoId === null ? null : undefined; - if (cargoId === undefined) continue; - if (v.comisionIds !== undefined && !isStringArray(v.comisionIds)) continue; - positions[term] = { - cargoId, - comisionIds: isStringArray(v.comisionIds) ? v.comisionIds : [], - ...(typeof v.assignedBy === "string" ? { assignedBy: v.assignedBy } : {}), - }; + const droppedTerms: { term: string; reason: string }[] = []; + if (!isAbsent(data.positions)) { + if (typeof data.positions !== "object" || Array.isArray(data.positions)) { + report( + "claims-sync: member positions is not a map — every term entry is dropped, so no cargo grants are minted", + { + positionsType: shapeOf(data.positions), + }, + ); + } else { + for (const [term, value] of Object.entries(data.positions as Record)) { + const v = value as { cargoId?: unknown; comisionIds?: unknown; assignedBy?: unknown }; + // `Array.isArray` folded in: `typeof [] === "object"`, so a term written as a list + // used to fall through to the cargoId screen and be dropped there under a reason that + // named the wrong field. Same drop, honest reason. + if (!v || typeof v !== "object" || Array.isArray(v)) { + droppedTerms.push({ term, reason: `term-entry-not-a-map:${shapeOf(value)}` }); + continue; + } + const cargoId = + typeof v.cargoId === "string" ? v.cargoId : v.cargoId === null ? null : undefined; + if (cargoId === undefined) { + // `cargoId: null` is the ordinary no-seat shape and is KEPT, not dropped — it never + // reaches here. An ABSENT cargoId does: the client mapper always writes the key + // (member-mapper.ts), so a term entry without it is a console/migration artifact. + droppedTerms.push({ term, reason: `cargo-id-not-a-string:${shapeOf(v.cargoId)}` }); + continue; + } + if (v.comisionIds !== undefined && !isStringArray(v.comisionIds)) { + droppedTerms.push({ + term, + reason: `comision-ids-not-a-string-array:${shapeOf(v.comisionIds)}`, + }); + continue; + } + positions[term] = { + cargoId, + comisionIds: isStringArray(v.comisionIds) ? v.comisionIds : [], + ...(typeof v.assignedBy === "string" ? { assignedBy: v.assignedBy } : {}), + }; + } } } + if (droppedTerms.length > 0) { + report( + "claims-sync: member position entries are malformed — each is dropped WHOLE, so its cargoId and assignedBy mint no grants", + { + droppedCount: droppedTerms.length, + dropped: droppedTerms + .slice(0, DROPPED_ENTRY_SAMPLE) + .map((d) => ({ term: truncateForLog(d.term), reason: d.reason })), + }, + ); + } + const roleIds = isStringArray(data.roleIds) ? data.roleIds : []; + if (!isAbsent(data.roleIds) && !isStringArray(data.roleIds)) { + // The WHOLE array goes, valid entries included — which is also why the + // "roleIds entries cannot be a doc id" screen in firestore-deps.ts can never see a + // mixed-type array: it is replaced by [] here, upstream of it. + report( + "claims-sync: member roleIds is not an array of strings — the WHOLE array is dropped, so no custom role grants any perms", + { + roleIdsType: shapeOf(data.roleIds), + entryCount: Array.isArray(data.roleIds) ? data.roleIds.length : null, + nonStringCount: Array.isArray(data.roleIds) + ? data.roleIds.filter((x) => typeof x !== "string").length + : null, + }, + ); + } + const rawOverrides = (data.permissionOverrides ?? {}) as { grant?: unknown; revoke?: unknown }; const permissionOverrides = { grant: permissionCodes(rawOverrides.grant), revoke: permissionCodes(rawOverrides.revoke), }; + const overrideProblems: { field: string; reason: string; valueType?: string; count?: number }[] = + []; + if (!isAbsent(data.permissionOverrides)) { + if (typeof data.permissionOverrides !== "object" || Array.isArray(data.permissionOverrides)) { + overrideProblems.push({ + field: "permissionOverrides", + reason: "not-a-map", + valueType: shapeOf(data.permissionOverrides), + }); + } else { + for (const field of ["grant", "revoke"] as const) { + const value = rawOverrides[field]; + if (isAbsent(value)) continue; + if (!Array.isArray(value)) { + overrideProblems.push({ field, reason: "not-an-array", valueType: shapeOf(value) }); + continue; + } + const rejected = value.filter((x) => !isValidPermissionCode(x)).length; + if (rejected > 0) overrideProblems.push({ field, reason: "unknown-code", count: rejected }); + } + } + } + if (overrideProblems.length > 0) { + // At most three rows (the map itself, then grant and revoke), so no sampling needed. Codes + // are counted, never serialized: a rejected code is by definition outside the vocabulary, + // so its VALUE is unbounded free text off a console edit. + report( + "claims-sync: member permissionOverrides entries are malformed — dropped, so those grants and revocations are not applied", + { + problems: overrideProblems, + }, + ); + } + return { uid, positions, roleIds, permissionOverrides }; } diff --git a/apps/beacon/src/claims-sync/sync.test.ts b/apps/beacon/src/claims-sync/sync.test.ts index b5bef8f0..92e37051 100644 --- a/apps/beacon/src/claims-sync/sync.test.ts +++ b/apps/beacon/src/claims-sync/sync.test.ts @@ -3,6 +3,7 @@ import type { Role } from "@luminova/auth/roles"; import type { PermissionCode, RoleDefinition } from "@luminova/types"; import { BUILT_IN_ROLE_PERMS } from "@luminova/types/role-definition"; import { ACTIONS, SUBJECTS } from "@luminova/types/permission"; +import { isSafeDocId } from "../firestore-util.js"; import { syncMemberClaims, type ClaimsSyncDeps, type MemberClaims } from "./sync.js"; import { parseMember } from "./parse-member.js"; import { isActiveRoleDoc } from "./role-doc.js"; @@ -41,6 +42,7 @@ function fakeDeps(opts: { builtInDocs?: RoleDefinition[]; customRoles?: Record; logError?: (message: string, meta: Record) => void; + logWarn?: (message: string, meta: Record) => void; }) { const writes: Record = {}; const deps: ClaimsSyncDeps = { @@ -87,6 +89,7 @@ function fakeDeps(opts: { writes[uid] = claims; }, logError: opts.logError, + logWarn: opts.logWarn, }; return { deps, writes }; } @@ -200,6 +203,71 @@ describe("syncMemberClaims", () => { expect(writes["delegate-uid"]).toBeUndefined(); }); + it("BLOCKING: WARNS (never errors) on the DESIGNED refusal, naming which half denied it", async () => { + // The likeliest real cause of "they're on the Directiva with no permissions", so it is + // logged — but on the warn sink. This branch is the feature working as specified and it + // re-fires on every write to that member (including the totalPoints mirror awardPoints + // does per check-in), so an ERROR here would out-volume the malformed-doc screens that + // sink exists for. The meta must distinguish the two reasons — self-assignment vs an + // Admin-granting cargo — because the remedy differs: an Admin re-saves the slot in the + // first case, and only an Admin may seat it in the second. + const logged: { message: string; meta: Record }[] = []; + const errors: string[] = []; + const { deps } = fakeDeps({ + positions: { "pos-presi": { grants: ["Admin"] } }, + userRoles: { "delegate-uid": ["Member"] }, + userPerms: { "delegate-uid": ["update:BoardSeat"] }, + existing: { "target-uid": { roles: ["Member"] } }, + logError: (message) => errors.push(message), + logWarn: (message, meta) => logged.push({ message, meta }), + }); + await syncMemberClaims( + deps, + { + uid: "target-uid", + positions: { + "2026": { cargoId: "pos-presi", comisionIds: [], assignedBy: "delegate-uid" }, + }, + }, + "2026", + ); + expect(logged).toHaveLength(1); + expect(logged[0]?.message).toMatch(/NOT minted/); + // The severity split, asserted rather than assumed: moving this line back onto logError + // must fail here, not just change which stream it lands on. + expect(errors).toEqual([]); + expect(logged[0]?.meta).toMatchObject({ + uid: "target-uid", + cargoId: "pos-presi", + selfAssigned: false, + grantsAdmin: true, + assignerIsAdmin: false, + }); + }); + + it("stays quiet on BOTH sinks when the grants ARE minted", async () => { + // The paired negative: a log on the success path would bury the refusals it exists to + // surface, since every ordinary seating writes claims. + const logged: string[] = []; + const { deps } = fakeDeps({ + positions: { "pos-sec": { grants: ["Secretary"] } }, + userRoles: { "delegate-uid": ["Member"] }, + userPerms: { "delegate-uid": ["update:BoardSeat"] }, + existing: { "target-uid": { roles: ["Member"] } }, + logError: (message) => logged.push(message), + logWarn: (message) => logged.push(message), + }); + await syncMemberClaims( + deps, + { + uid: "target-uid", + positions: { "2026": { cargoId: "pos-sec", comisionIds: [], assignedBy: "delegate-uid" } }, + }, + "2026", + ); + expect(logged).toEqual([]); + }); + it("still lets a delegate confer a non-Admin cargo on SOMEONE ELSE", async () => { // The paired ALLOW — the restriction is on self-dealing, not on the delegation itself. const { deps, writes } = fakeDeps({ @@ -275,9 +343,12 @@ describe("syncMemberClaims", () => { positions: { "pos-pres": { grants: ["Admin"] } }, userRoles: { "president-uid": ["Admin", "Member"] }, userPerms: { "president-uid": ["manage:all"] }, - existing: { - "president-uid": { roles: ["Admin", "Member"], perms: permsFor(["Admin", "Member"]) }, - }, + // Seeded DELIBERATELY below the denied outcome, not byte-equal to it: seeding the + // already-correct claims would make the idempotent no-op path indistinguishable from a + // strip, and the assertion would only prove "nothing was written". From ["Member"] the + // guard's two outcomes diverge — honored writes the Admin claim, denied writes the plain + // Member one — so the write itself is the evidence. + existing: { "president-uid": { roles: ["Member"] } }, }); await syncMemberClaims( deps, @@ -289,8 +360,12 @@ describe("syncMemberClaims", () => { }, "2026", ); - // Idempotent no-op: the claims are already correct, so no write — and crucially not a strip. - expect(writes["president-uid"]).toBeUndefined(); + // The Admin grant is HONORED through a self-stamped assignedBy: the full claim is written, + // not withheld and not stripped down to Member. + expect(writes["president-uid"]).toEqual({ + roles: ["Admin", "Member"], + perms: permsFor(["Admin", "Member"]), + }); }); it("de-elevates a delegate-conferred NON-Admin grant once the perm is revoked", async () => { @@ -452,10 +527,12 @@ describe("syncMemberClaims", () => { // than in each getPosition impl. Fails closed: no cargo means no grants. const reached: string[] = []; for (const cargoId of ["a/b", "", ".", "..", "__name__", `${"x".repeat(1501)}`]) { + const logged: { message: string; meta: Record }[] = []; const { deps, writes } = fakeDeps({ positions: { "a/b": { grants: ["Admin"] }, "": { grants: ["Admin"] } }, userRoles: { "admin-uid": ["Admin"] }, existing: { "target-uid": { roles: ["Member"] } }, + logError: (message, meta) => logged.push({ message, meta }), }); const spy: typeof deps.getPosition = async (id) => { reached.push(id); @@ -475,6 +552,166 @@ describe("syncMemberClaims", () => { expect(reached).toEqual([]); // Fails closed: the Admin grant behind that cargo is NOT minted. expect(writes["target-uid"]).toEqual({ roles: ["Member"], perms: permsFor(["Member"]) }); + // ...and NOT silently (guardrail #4). Failing closed here strips a seated member's + // roles; an operator seeing a Directiva row with no permissions has this line and + // nothing else to explain it. Pinned so the screen cannot go quiet again. + expect(logged).toHaveLength(1); + expect(logged[0].message).toMatch(/cargoId/); + // Names the member, and bounds the offending id rather than serializing 1501 chars — + // Cloud Logging drops an over-large entry whole, losing the anomaly when it is biggest. + expect(logged[0].meta).toMatchObject({ uid: "target-uid", cargoIdLength: cargoId.length }); + expect(String(logged[0].meta.cargoId).length).toBeLessThanOrEqual(65); + } + }); + + it("does NOT log the screen for a member holding no cargo at all", async () => { + // The other half of the log above, and the half that decides whether it is a signal or + // noise: `cargoId: null` fails the same `isSafeDocId` check, but it is the ordinary state + // of every member without a seat. Logging it would emit an error per member per write and + // bury the real anomaly. Absent map, absent entry and explicit null all stay quiet. + for (const positions of [ + undefined, + {}, + { "2026": { cargoId: null, comisionIds: [], assignedBy: "admin-uid" } }, + ]) { + const logged: string[] = []; + const { deps } = fakeDeps({ + positions: {}, + userRoles: {}, + existing: { "target-uid": { roles: ["Member"], perms: permsFor(["Member"]) } }, + logError: (message) => logged.push(message), + }); + await syncMemberClaims(deps, { uid: "target-uid", positions }, "2026"); + expect(logged).toEqual([]); + } + }); + + it("BLOCKING: screens an unusable assignedBy instead of failing that member's sync forever", async () => { + // The OTHER id on the same line, and the same failure mode as the cargoId screen above. + // `assignedBy` reaches auth.getUser(), whose uid contract is a non-empty string of at most + // 128 chars; anything else is a PERMANENT auth/invalid-uid, and getAssignerClaims rethrows + // everything but auth/user-not-found. onMemberWritten is retry:false and the bad value + // PERSISTS in the member doc, so every later write re-throws — and only on power-granting + // cargos, i.e. exactly the members whose claims matter most. Nothing in firestore.rules + // caps assignedBy's length; the console and the admin SDK reach this shape. + const reached: string[] = []; + const logged: { message: string; meta: Record }[] = []; + for (const assignedBy of ["", "x".repeat(129)]) { + const { deps, writes } = fakeDeps({ + positions: { "pos-pres": { grants: ["Admin"] } }, + // The fixture would hand back Admin if the screen fell through, so the assertion + // below is about the screen, not about the assigner failing the trust gate. + userRoles: { [assignedBy]: ["Admin"] }, + existing: { "target-uid": { roles: ["Member"] } }, + logError: (message, meta) => logged.push({ message, meta }), + }); + const spied: ClaimsSyncDeps = { + ...deps, + getAssignerClaims: async (uid) => { + reached.push(uid); + return deps.getAssignerClaims(uid); + }, + }; + await syncMemberClaims( + spied, + { + uid: "target-uid", + positions: { "2026": { cargoId: "pos-pres", comisionIds: [], assignedBy } }, + }, + "2026", + ); + // The screened uid never reaches the port, so it never reaches auth.getUser(). + expect(reached).toEqual([]); + // Fails closed: the Admin grant behind that cargo is NOT minted. + expect(writes["target-uid"]).toEqual({ roles: ["Member"], perms: permsFor(["Member"]) }); + } + // ...and not silently. This branch is reached ONLY on a power-granting cargo, so its + // silence hid exactly the claims an operator would come looking for. The uid VALUE is + // deliberately absent from the meta: type and length identify the shape, and the member + // uid plus cargo id already locate the doc to fix. + expect(logged).toHaveLength(2); + expect(logged.map((l) => l.message)).toEqual([ + expect.stringMatching(/assignedBy/), + expect.stringMatching(/assignedBy/), + ]); + expect(logged[0].meta).toEqual({ + uid: "target-uid", + cargoId: "pos-pres", + assignedByType: "string", + assignedByLength: 0, + }); + expect(logged[1].meta).toMatchObject({ assignedByLength: 129 }); + }); + + it("logs the legacy no-assignedBy power seat too, and stays quiet off the power path", async () => { + // A power cargo with no attribution still says so, but at WARN, not ERROR: an absent + // assignedBy is a legacy doc shape that predates the field and is re-emitted on every + // write to that member forever, so putting it in the same bucket as corruption is how the + // error stream stops being read. The screen sits BEHIND the grants check, so a grant-free + // cargo with no assignedBy — the ordinary shape of every rank-and-file seat — never + // reaches it and must stay quiet on BOTH sinks. + for (const [cargoId, expectedWarns] of [ + ["pos-pres", 1], + ["pos-plain", 0], + ] as const) { + const warns: string[] = []; + const errors: string[] = []; + const { deps } = fakeDeps({ + positions: { "pos-pres": { grants: ["Admin"] }, "pos-plain": { grants: [] } }, + userRoles: {}, + existing: { "target-uid": { roles: ["Member"] } }, + logError: (message) => errors.push(message), + logWarn: (message) => warns.push(message), + }); + await syncMemberClaims( + deps, + { uid: "target-uid", positions: { "2026": { cargoId, comisionIds: [] } } }, + "2026", + ); + expect(warns).toHaveLength(expectedWarns); + // BLOCKING: never the error sink — that is the whole point of the split. + expect(errors).toEqual([]); + } + }); + + it("ACCEPTS a uid isSafeDocId would reject — the screen is a uid contract, not a path one", async () => { + // The other half of the screen above, and the half a "let's reuse isSafeDocId for + // consistency" edit would silently break: a uid is NOT a path segment. auth.getUser() + // accepts any 1..128-char string, so "/" and the reserved "." / ".." / "__x__" forms are + // legitimate uids (third-party/SAML-derived ids carry them), while isSafeDocId rejects + // every one. Tightening the screen to match it fails CLOSED — no throw, no log, just a + // legitimate delegate's grants quietly never minted. So pin the acceptance too. + for (const assignedBy of ["saml/acme|ana", ".", "..", "__soporte__"]) { + // Asserted, not assumed: this is the divergence the test exists to hold open. + expect(isSafeDocId(assignedBy)).toBe(false); + const { deps, writes } = fakeDeps({ + positions: { "pos-pres": { grants: ["Admin"] } }, + userRoles: { [assignedBy]: ["Admin"] }, + existing: { "target-uid": { roles: ["Member"] } }, + }); + const reached: string[] = []; + const spied: ClaimsSyncDeps = { + ...deps, + getAssignerClaims: async (uid) => { + reached.push(uid); + return deps.getAssignerClaims(uid); + }, + }; + await syncMemberClaims( + spied, + { + uid: "target-uid", + positions: { "2026": { cargoId: "pos-pres", comisionIds: [], assignedBy } }, + }, + "2026", + ); + // The assigner lookup IS reached... + expect(reached).toEqual([assignedBy]); + // ...and their grants ARE minted. + expect(writes["target-uid"]).toEqual({ + roles: ["Admin", "Member"], + perms: permsFor(["Admin", "Member"]), + }); } }); @@ -694,8 +931,9 @@ describe("syncMemberClaims", () => { }); }); - it("fail-closed: writes empty perms (not stale claims) when effective perms exceed the cap", async () => { + it("fail-closed: writes empty perms (not stale claims) and WARNS when effective perms exceed the cap", async () => { const errors: string[] = []; + const warnings: string[] = []; const { deps, writes } = fakeDeps({ positions: {}, userRoles: {}, @@ -703,6 +941,7 @@ describe("syncMemberClaims", () => { existing: { "target-uid": { roles: ["Admin", "Member"], perms: ["manage:all"] } }, customRoles: { big: customRole("big", distinctCodes(31)) }, logError: (message) => errors.push(message), + logWarn: (message) => warnings.push(message), }); await syncMemberClaims( deps, @@ -715,7 +954,9 @@ describe("syncMemberClaims", () => { "2026", ); expect(writes["target-uid"]).toEqual({ roles: ["Member"], perms: [] }); - expect(errors[0]).toMatch(/cap/i); + // An expected ceiling, not a malformed doc: same severity split as the trust gate. + expect(warnings[0]).toMatch(/cap/i); + expect(errors).toEqual([]); }); it("propagates and writes nothing when a dependency rejects", async () => { @@ -751,10 +992,13 @@ describe("syncMemberClaims", () => { userRoles: { "membership-uid": ["Membership", "Member"] }, existing: { "target-uid": { roles: ["Member"] } }, }); - const member = parseMember({ - uid: "target-uid", - positions: { "2026": { cargoId: "pos-pres", assignedBy: "membership-uid" } }, - }); + const member = parseMember( + { + uid: "target-uid", + positions: { "2026": { cargoId: "pos-pres", assignedBy: "membership-uid" } }, + }, + { memberId: "m-test", logError: () => {} }, + ); await syncMemberClaims(deps, member, "2026"); // No Admin escalation: recomputed to a plain Member claim. expect(writes["target-uid"]).toEqual({ roles: ["Member"], perms: permsFor(["Member"]) }); diff --git a/apps/beacon/src/claims-sync/sync.ts b/apps/beacon/src/claims-sync/sync.ts index a0b861a7..36ab079d 100644 --- a/apps/beacon/src/claims-sync/sync.ts +++ b/apps/beacon/src/claims-sync/sync.ts @@ -1,7 +1,7 @@ import type { Role } from "@luminova/auth/roles"; import type { TermPositions, PermissionCode } from "@luminova/types"; import { PERMISSION_CAP } from "@luminova/types/permission"; -import { isSafeDocId } from "../firestore-util.js"; +import { isSafeDocId, truncateForLog, type LogSink } from "../firestore-util.js"; import { computeMemberRoles } from "./compute-roles.js"; import { resolveMemberPerms, type RolePermsDeps } from "./resolve-member-perms.js"; @@ -23,8 +23,16 @@ export interface ClaimsSyncDeps extends RolePermsDeps { /** The target member's existing custom claims. */ getExistingClaims(uid: string): Promise<{ roles: Role[]; perms?: PermissionCode[] }>; setClaims(uid: string, claims: MemberClaims): Promise; - /** Structured error sink (defaults to console.error in the Firestore impl). */ - logError?(message: string, meta: Record): void; + /** Malformed input the rules allow but this code cannot use — someone must edit the doc. + * Defaults to console.error in the Firestore impl. */ + logError?: LogSink; + /** Outcomes that are working as designed but leave a member without claims they look like + * they should have — a refused trust gate, a perms-cap breach. Separated from `logError` + * because both are steady-state: `onMemberWritten` fires on ANY member write (awardPoints + * mirrors `totalPoints` on every check-in) and this runs before the idempotency + * short-circuit, so one seat in that state would emit an ERROR per write forever and train + * the operator to filter the sink out. Defaults to console.warn (Cloud Logging: WARNING). */ + logWarn?: LogSink; } type MemberLike = { @@ -110,10 +118,55 @@ async function resolveTrustedGrants( // that member re-throws. Their claims never sync again until someone edits the id out. // Screened HERE rather than in each port impl so the in-memory test fakes inherit it. // Fails closed in the right direction: no cargo means no grants. - if (!isSafeDocId(cargoId)) return []; + // Captured BEFORE the screen: `isSafeDocId` is an `id is string` predicate, so TS narrows + // the REJECTED branch to `null` and the log below could not name the offending string. + const rejectedCargoId = typeof cargoId === "string" ? cargoId : null; + if (!isSafeDocId(cargoId)) { + // Failing closed is deliberate; failing SILENTLY is not (guardrail #4). Without this the + // member is published on the world-readable Directiva seated on a cargo whose grants were + // never minted, and nothing anywhere says why — no throw, no log, no metric. A null + // cargoId is the ordinary "no cargo" case and is NOT an anomaly: logging it would fire on + // every write to every member who holds no seat. Ids only, bounded — never the doc. + if (rejectedCargoId !== null) { + deps.logError?.("claims-sync: cargoId is not a usable doc id — minting no cargo grants", { + uid: memberUid, + cargoId: truncateForLog(rejectedCargoId), + cargoIdLength: rejectedCargoId.length, + }); + } + return []; + } + // A null `position` is logged by readPositionGrants, at the site that can tell an unusable + // id from a missing doc from a malformed `grants`; re-logging here would double every line. const position = await deps.getPosition(cargoId); if (!position || position.grants.length === 0) return []; - if (!assignedBy) return []; + // Screened for the same reason as cargoId above, one line up: `assignedBy` reaches + // auth.getUser(), which rejects anything outside the admin SDK's uid shape with a PERMANENT + // auth/invalid-uid that getAssignerClaims rethrows — under retry:false that ends this + // member's claims sync for good, and only on power-granting cargos. NOT isSafeDocId: a uid + // is not a path segment ("/" and reserved forms are legal in one), and the 128-char cap + // isSafeDocId lacks is the half that actually bites here. Untrusted shape → no grants. + if (typeof assignedBy !== "string" || assignedBy.length === 0 || assignedBy.length > 128) { + // Reached ONLY on a power-granting cargo, i.e. exactly the members whose missing claims + // matter. Split by severity for the same reason the designed refusal is a warning: an + // ABSENT assignedBy is a legacy doc shape that predates the field, steady-state, and + // re-emitted on every write to that member forever; a present-but-malformed one is + // corruption a person must go edit. + const legacyMissing = assignedBy === undefined || assignedBy === null; + const sink = legacyMissing ? deps.logWarn : deps.logError; + sink?.( + legacyMissing + ? "claims-sync: power seat has no assignedBy — minting no cargo grants" + : "claims-sync: assignedBy is not a usable uid — minting no cargo grants", + { + uid: memberUid, + cargoId: truncateForLog(cargoId), + assignedByType: typeof assignedBy, + assignedByLength: typeof assignedBy === "string" ? assignedBy.length : null, + }, + ); + return []; + } const assigner = await deps.getAssignerClaims(assignedBy); const assignerIsAdmin = assigner.roles.includes("Admin"); // A delegate may confer power on OTHERS, never on themselves, and never Admin at all. @@ -123,6 +176,20 @@ async function resolveTrustedGrants( position.grants.includes("Admin") || selfAssigned ? assignerIsAdmin : assignerIsAdmin || assigner.perms.includes("update:BoardSeat"); + if (!trusted) { + // WARN, not ERROR: the DESIGNED refusal, reached on an ordinary delegate path (an + // Admin-granting cargo, or a self-assignment) and on a stale one (the assigner has since + // lost update:BoardSeat). Still logged, because it is the likeliest real cause of the + // support question this feature will generate — "they're on the Directiva with no + // permissions" — and nothing server-side said which of the two happened. + deps.logWarn?.("claims-sync: cargo grants NOT minted — assigner is not trusted for them", { + uid: memberUid, + cargoId: truncateForLog(cargoId), + selfAssigned, + grantsAdmin: position.grants.includes("Admin"), + assignerIsAdmin, + }); + } return trusted ? [...new Set(position.grants)] : []; } @@ -173,10 +240,12 @@ export async function syncMemberClaims( // keep a stale grant while dropping a revoke). We still write the recomputed // `roles` + empty `perms` so a concurrent role revocation always lands — // never leave the member on stale, possibly-elevated claims. - // Note for update:BoardSeat holders: this takes their delegation with it, silently. A - // delegate over the cap keeps seating (their cached token still passes the rules) while - // this function stops honoring the grants — the seat publishes, no claim is minted. - deps.logError?.("effective perms exceed cap; writing empty perms (fail-closed)", { + // WARN for the same reason as the trust gate above: an expected ceiling, not malformed + // input, and it persists across every write to that member. Note for update:BoardSeat + // holders: this takes their delegation with it. A delegate over the cap keeps seating + // (their cached token still passes the rules) while this function stops honoring the + // grants — the seat publishes, no claim is minted. + deps.logWarn?.("effective perms exceed cap; writing empty perms (fail-closed)", { uid: member.uid, count: perms.length, cap: PERMISSION_CAP, diff --git a/apps/beacon/src/firestore-util.test.ts b/apps/beacon/src/firestore-util.test.ts new file mode 100644 index 00000000..474e3a06 --- /dev/null +++ b/apps/beacon/src/firestore-util.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from "vitest"; +import { truncateForLog } from "./firestore-util.js"; + +// A shared primitive across three claims-sync log sites, previously exercised only through +// them — so its boundary was never asserted anywhere. It exists because `isSafeDocId` tolerates +// 1500 bytes and Cloud Logging drops an over-large entry ENTIRELY, which loses the anomaly +// exactly when it is biggest. +describe("truncateForLog", () => { + it("returns a short value unchanged, with no marker", () => { + expect(truncateForLog("pos-presidente")).toBe("pos-presidente"); + expect(truncateForLog("")).toBe(""); + }); + + it("BLOCKING: bounds anything longer, keeping the head and marking the cut", () => { + const long = "x".repeat(1500); + const out = truncateForLog(long); + expect(out).toHaveLength(65); // 64 + the ellipsis + expect(out.startsWith("x".repeat(64))).toBe(true); + expect(out.endsWith("…")).toBe(true); + }); + + it("does not truncate at exactly the cap, and does one char past it", () => { + // The off-by-one that a `>=` would introduce: a 64-char id is representable in full, so + // marking it as cut would misreport the anomaly it exists to describe. + expect(truncateForLog("y".repeat(64))).toBe("y".repeat(64)); + expect(truncateForLog("y".repeat(65))).toBe(`${"y".repeat(64)}…`); + }); + + // This case used to be asserted with an ALL-astral fixture, which cannot produce it: pairs + // are 2 units each and the cap is 64, an even index, so the cut always landed ON a boundary. + // Both of its assertions were unconditionally true besides — JSON.stringify has not thrown on + // a lone surrogate since ES2019, and `expect(JSON.parse(…)).toBeTruthy()` holds for every + // object. A leading BMP character is what shifts the pairs onto odd indices. + it("BLOCKING: never emits a lone surrogate, even when the cut lands mid-pair", () => { + const mixed = `a${"𝒳".repeat(40)}`; + // The premise, asserted rather than claimed: this fixture really does split a pair. + expect(mixed.slice(0, 64).isWellFormed()).toBe(false); + + const out = truncateForLog(mixed); + expect(out.isWellFormed()).toBe(true); + expect(out.endsWith("…")).toBe(true); + // Round-trips as the SAME string — an escaped orphan would come back as \uD835. + expect(JSON.parse(JSON.stringify({ id: out })).id).toBe(out); + }); + + it("keeps the full cap when the boundary is clean", () => { + // The orphan trim must cost one char only when there IS an orphan. + const astral = "𝒳".repeat(40); // pairs on even indices: the 64-char cut is a boundary + expect(truncateForLog(astral)).toBe(`${astral.slice(0, 64)}…`); + }); +}); diff --git a/apps/beacon/src/firestore-util.ts b/apps/beacon/src/firestore-util.ts index 1777d7d9..37deda88 100644 --- a/apps/beacon/src/firestore-util.ts +++ b/apps/beacon/src/firestore-util.ts @@ -10,19 +10,46 @@ export function hasToMillis(v: unknown): v is Timestamp { /** Whether `id` is safe to interpolate into a `collection/${id}` doc-path template. * - * The rejected shapes do NOT all fail at the same point, and an earlier version of this - * comment flattened them into "`db.doc()` throws": only the empty and "/"-bearing cases - * throw synchronously in `db.doc()` (wrong path-segment count). `.`, `..` and `__x__` build - * a reference fine and fail LATER, at `get()`, with a permanent INVALID_ARGUMENT from the - * server. Either way the failure is permanent rather than transient, which is the property - * that matters: on a retry:true trigger it is a redelivery loop, and on a claims read it - * fails that member's sync forever (every later write re-throws, because the offending id - * persists in the doc) until someone edits it out. - * Screen instead — the id contributing nothing fails closed. - * Extracted from currentCargoId, which was the first path to need it. */ + * The rejected shapes do not all fail at the same point: the empty and "/"-bearing cases + * throw synchronously in `db.doc()` (wrong path-segment count), while `.`, `..` and `__x__` + * build a reference fine and fail LATER, at `get()`, with a permanent INVALID_ARGUMENT from + * the server. Either way the failure is permanent rather than transient, which is the + * property that matters: on a retry:true trigger it is a redelivery loop, and on a claims + * read it fails that member's sync forever (every later write re-throws, because the + * offending id persists in the doc) until someone edits it out. + * Screen instead — the id contributing nothing fails closed. */ export function isSafeDocId(id: unknown): id is string { if (typeof id !== "string" || id.length === 0 || id.includes("/")) return false; if (id === "." || id === "..") return false; if (id.startsWith("__") && id.endsWith("__")) return false; return UTF8.encode(id).length <= 1500; } + +/** A structured log sink, injected so a shared fail-closed read is not welded to `console`. */ +export type LogSink = (message: string, meta: Record) => void; + +/** The default sinks, HERE rather than one per adapter file. Routing beacon's structured logs + * anywhere else — a Cloud Logging client, a redaction wrapper, another severity split — is + * then one edit that reaches every port, which is the property `claims-sync/firestore-deps.ts` + * claimed while a byte-identical second copy lived in `provision-deps.ts` and silently kept + * `readPositionGrants`'s anomaly lines on the old path. */ +export const logError: LogSink = (message, meta) => console.error(message, meta); +export const logWarn: LogSink = (message, meta) => console.warn(message, meta); + +const LOG_ID_MAX_CHARS = 64; + +/** An id bounded for a structured-log field. The values screened by `isSafeDocId` run to + * 1500 bytes and Cloud Logging drops an over-large entry ENTIRELY — so serializing them raw + * loses the anomaly precisely when it is biggest. Shared so every screen's log line is + * bounded the same way. */ +export function truncateForLog(value: string): string { + if (value.length <= LOG_ID_MAX_CHARS) return value; + const cut = value.slice(0, LOG_ID_MAX_CHARS); + // A cut at a fixed UTF-16 index can land BETWEEN the halves of a surrogate pair, leaving a + // lone high surrogate — an ill-formed string. JSON.stringify escapes it rather than throwing + // (well-formed stringify, ES2019), so nothing here fails; the damage is downstream, in + // whatever reads the log field. Drop the orphan instead of shipping it. + const last = cut.charCodeAt(cut.length - 1); + const orphaned = last >= 0xd800 && last <= 0xdbff; + return `${orphaned ? cut.slice(0, -1) : cut}…`; +} diff --git a/apps/beacon/src/index.ts b/apps/beacon/src/index.ts index 459ee4da..ca143fe6 100644 --- a/apps/beacon/src/index.ts +++ b/apps/beacon/src/index.ts @@ -28,7 +28,7 @@ import { PUBLIC_PROFILE_DEFAULT, } from "./showcase/default-public-profile.js"; import type { ShowcasePerson } from "@luminova/types/engine"; -import { firestoreClaimsDeps } from "./claims-sync/firestore-deps.js"; +import { firestoreClaimsDeps, logError } from "./claims-sync/firestore-deps.js"; import { syncMemberClaims } from "./claims-sync/sync.js"; import { roleClaimsChanged } from "./claims-sync/role-change.js"; import { builtInKeyFromRoleDoc } from "./claims-sync/role-doc.js"; @@ -229,7 +229,7 @@ export const onActivityWritten = onDocumentWritten("activities/{id}", async (eve export const onMemberWritten = onDocumentWritten("members/{id}", async (event) => { const after = event.data?.after; if (!after?.exists) return; // deletes leave the Auth user untouched - const member = parseMember(after.data()); + const member = parseMember(after.data(), { memberId: event.params.id, logError }); if (!member.uid) return; // not provisioned → no Auth user to claim await syncMemberClaims(firestoreClaimsDeps(db(), getAuth()), member, currentTermKey()); }); @@ -313,7 +313,7 @@ export const onRoleWritten = onDocumentWritten( let scanned = 0; let failed = 0; for (const doc of docs) { - const member = parseMember(doc.data()); + const member = parseMember(doc.data(), { memberId: doc.id, logError }); if (!member.uid) continue; scanned += 1; try { diff --git a/apps/beacon/src/provision-deps.test.ts b/apps/beacon/src/provision-deps.test.ts new file mode 100644 index 00000000..0763a52b --- /dev/null +++ b/apps/beacon/src/provision-deps.test.ts @@ -0,0 +1,163 @@ +import { describe, expect, it } from "vitest"; +import type { Auth } from "firebase-admin/auth"; +import type { Firestore } from "firebase-admin/firestore"; +import { firestoreProvisionDeps } from "./provision-deps.js"; + +/** Nothing under test reads Firestore — `createUser` is a pure Auth path. */ +const db = {} as Firestore; + +/** An Auth stub narrow enough to drive `createUser`'s one call chain. The cast is test-only + * and justified: `UserRecord` carries a dozen fields (metadata, providerData, toJSON) that + * nothing here reads, and fabricating them would assert nothing. A missing email throws the + * real `auth/user-not-found`, which is what the live SDK does. */ +function fakeAuth(opts: { + createError?: unknown; + byEmailError?: unknown; + byUidError?: unknown; + byEmail?: Record; + byUid?: Record; +}) { + const calls = { + createUser: [] as string[], + getUserByEmail: [] as string[], + getUser: [] as string[], + }; + const auth = { + createUser: async ({ email }: { email: string }) => { + calls.createUser.push(email); + if (opts.createError !== undefined) throw opts.createError; + return { uid: `new-${email}`, email }; + }, + getUserByEmail: async (email: string) => { + calls.getUserByEmail.push(email); + if (opts.byEmailError !== undefined) throw opts.byEmailError; + const user = opts.byEmail?.[email]; + if (!user) throw Object.assign(new Error("no user"), { code: "auth/user-not-found" }); + return user; + }, + getUser: async (uid: string) => { + calls.getUser.push(uid); + if (opts.byUidError !== undefined) throw opts.byUidError; + const user = opts.byUid?.[uid]; + if (!user) throw Object.assign(new Error("no user"), { code: "auth/user-not-found" }); + return user; + }, + } as unknown as Auth; + return { auth, calls }; +} + +// The contract `nullIfUserNotFound` exists to hold, stated in the ProvisionDeps docblock and +// until now pinned by NOTHING: "null ONLY when the account does not exist — transient Auth +// errors must throw, or a blip would misread a live linked account as safely deleted". +// +// getUserByUid is the one the relink guard calls, and it had no coverage at all here; +// provision-member-login.test.ts drives hand-written fakes that bypass this port entirely. The +// mutation that was invisible: widen the null branch to `code === "auth/user-not-found" || +// code === "auth/internal-error"` and the whole beacon suite stays green, while an Identity +// Toolkit blip during the relink guard reads a LIVE linked account as safely deleted and lets +// the caller re-provision over it. +describe("firestoreProvisionDeps — the null-vs-throw contract", () => { + const lookups = ["getUserByEmail", "getUserByUid"] as const; + + it("returns null for user-not-found, on both lookups", async () => { + for (const method of lookups) { + const { auth } = fakeAuth({}); + await expect(firestoreProvisionDeps(db, auth)[method]("a@b.co")).resolves.toBeNull(); + } + }); + + it("BLOCKING: a transient Auth error throws — it must never read as a deleted account", async () => { + for (const method of lookups) { + for (const err of [ + authError("auth/internal-error"), + authError("auth/network-request-failed"), + new Error("socket hang up"), + ]) { + const { auth } = fakeAuth( + method === "getUserByEmail" ? { byEmailError: err } : { byUidError: err }, + ); + await expect(firestoreProvisionDeps(db, auth)[method]("a@b.co")).rejects.toThrow(); + } + } + }); +}); + +const authError = (code: string) => Object.assign(new Error(code), { code }); + +describe("firestoreProvisionDeps.createUser", () => { + it("returns the freshly minted account without consulting getUserByEmail", async () => { + const { auth, calls } = fakeAuth({}); + await expect(firestoreProvisionDeps(db, auth).createUser("a@b.co")).resolves.toMatchObject({ + uid: "new-a@b.co", + }); + expect(calls.createUser).toEqual(["a@b.co"]); + expect(calls.getUserByEmail).toEqual([]); + }); + + it("BLOCKING: rethrows a non-collision Auth error instead of masking it as a lookup", async () => { + // The catch used to be blanket. A quota, disabled-provider or invalid-email failure then + // fell through to getUserByEmail — which throws auth/user-not-found for an email no + // account was ever created for, and nullIfUserNotFound turns THAT into a null the relink + // guard reads as "the account was safely deleted". Wrong outcome, and the real cause was + // gone from the log. The fallback must fire for exactly one code. + for (const code of ["auth/quota-exceeded", "auth/operation-not-allowed"]) { + const { auth, calls } = fakeAuth({ createError: authError(code) }); + await expect(firestoreProvisionDeps(db, auth).createUser("a@b.co")).rejects.toMatchObject({ + code, + }); + expect(calls.getUserByEmail).toEqual([]); + } + }); + + // auth/invalid-email is the one non-collision code that does NOT stay raw. The shape screen + // in provisionMember is a pre-filter, not a guarantee: "a@.", ".a@b.co" and "a..b@c.co" each + // carry one @, no whitespace and no control characters, so they pass it AND the Admin SDK's + // own isEmail, and only Identity Toolkit rejects them. Rethrown raw that reaches the operator + // as an opaque `internal` with no details.reason — the generic "No se pudo…" dead end — on a + // member who is then unprovisionable with no hint. Tagged at the port closes the class + // however the regex evolves. + it("BLOCKING: tags an Identity-Toolkit invalid-email as the reason the UI can name", async () => { + for (const method of ["createUser", "getUserByEmail"] as const) { + const { auth } = fakeAuth( + method === "createUser" + ? { createError: authError("auth/invalid-email") } + : { byEmailError: authError("auth/invalid-email") }, + ); + await expect(firestoreProvisionDeps(db, auth)[method]("a@.")).rejects.toMatchObject({ + code: "failed-precondition", + details: { reason: "member-email-malformed" }, + }); + } + }); + + it("rethrows a codeless throw too — an unrecognized shape is not a collision", async () => { + const { auth, calls } = fakeAuth({ createError: new Error("socket hang up") }); + await expect(firestoreProvisionDeps(db, auth).createUser("a@b.co")).rejects.toThrow( + "socket hang up", + ); + expect(calls.getUserByEmail).toEqual([]); + }); + + it("falls back to getUserByEmail ONLY on a concurrent-create collision", async () => { + // The one tolerated case: a parallel invite already minted the account, so adopting it is + // the correct resolution rather than a failure. + const { auth, calls } = fakeAuth({ + createError: authError("auth/email-already-exists"), + byEmail: { "a@b.co": { uid: "u-existing" } }, + }); + await expect(firestoreProvisionDeps(db, auth).createUser("a@b.co")).resolves.toMatchObject({ + uid: "u-existing", + }); + expect(calls.createUser).toEqual(["a@b.co"]); + expect(calls.getUserByEmail).toEqual(["a@b.co"]); + }); + + it("propagates the lookup's own failure when the collision resolves to nothing", async () => { + // Not squashed to null: the caller's null contract means "no account exists", and this + // path just proved one does. + const { auth } = fakeAuth({ createError: authError("auth/email-already-exists") }); + await expect(firestoreProvisionDeps(db, auth).createUser("a@b.co")).rejects.toMatchObject({ + code: "auth/user-not-found", + }); + }); +}); diff --git a/apps/beacon/src/provision-deps.ts b/apps/beacon/src/provision-deps.ts index 5deef27d..32cd1437 100644 --- a/apps/beacon/src/provision-deps.ts +++ b/apps/beacon/src/provision-deps.ts @@ -1,13 +1,40 @@ import type { Auth } from "firebase-admin/auth"; import type { Firestore } from "firebase-admin/firestore"; import { readPositionGrants } from "./read-position-grants.js"; +import { logError } from "./firestore-util.js"; +import { memberEmailMalformed } from "./provision-errors.js"; import type { ProvisionDeps } from "./provision-member-login.js"; +function authCode(err: unknown): unknown { + return (err as { code?: unknown } | null)?.code; +} + +/** Identity Toolkit rejects addresses the SHAPE screen cannot: `a@.`, `.a@b.co`, `a..b@c.co` + * each carry one `@`, no whitespace and no control characters, so they pass + * `ADMIN_SDK_EMAIL_SHAPE` and the Admin SDK's own isEmail alike and only fail server-side. + * Rethrown raw, that is an opaque `internal` with no `details.reason` and a member nobody can + * provision without knowing why. Tagged HERE rather than by chasing regex precision: this + * closes the class whatever the pattern does next. + * + * Residual, deliberately not chased further: a rejection that maps to `auth/invalid-argument` + * rather than `auth/invalid-email` still reaches the client opaque. The shape screen plus this + * tag cover the reachable cases. */ +function tagInvalidEmail(err: unknown): never { + if (authCode(err) === "auth/invalid-email") { + // The HttpsError replaces the original, and firebase-functions treats a thrown HttpsError + // as an EXPECTED refusal — no "Unhandled error" line. Without this the failure class would + // leave zero trace in Cloud Logging (guardrail #4). Code only, never the address: PII. + logError("provision refused: Auth rejected the stored email", { code: authCode(err) }); + throw memberEmailMalformed(); + } + throw err; +} + // Null only for the "account does not exist" outcome — a transient Auth error // must propagate, not read as deleted (the relink guard trusts that contract). function nullIfUserNotFound(err: unknown): null { - if ((err as { code?: unknown } | null)?.code === "auth/user-not-found") return null; - throw err; + if (authCode(err) === "auth/user-not-found") return null; + return tagInvalidEmail(err); } export function firestoreProvisionDeps(db: Firestore, auth: Auth): ProvisionDeps { @@ -24,14 +51,17 @@ export function firestoreProvisionDeps(db: Firestore, auth: Auth): ProvisionDeps // auth/user-not-found, destroying the diagnostic. createUser: (email) => auth.createUser({ email }).catch((err: unknown) => { - if ((err as { code?: unknown } | null)?.code !== "auth/email-already-exists") throw err; + if (authCode(err) !== "auth/email-already-exists") return tagInvalidEmail(err); return auth.getUserByEmail(email); }), setClaims: (uid, claims) => auth.setCustomUserClaims(uid, claims), linkUid: async (id, uid) => { await db.doc(`members/${id}`).update({ uid }); }, + // Deliberately NOT routed through tagInvalidEmail: this is the only Auth call that runs + // AFTER createUser and linkUid, so tagging it would tell the operator to "corrige el correo" + // about an account that already exists and is already linked. passwordResetLink: (email) => auth.generatePasswordResetLink(email), - getPositionGrants: (cargoId) => readPositionGrants(db, cargoId), + getPositionGrants: (cargoId) => readPositionGrants(db, cargoId, logError), }; } diff --git a/apps/beacon/src/provision-errors.ts b/apps/beacon/src/provision-errors.ts new file mode 100644 index 00000000..9b0ff99e --- /dev/null +++ b/apps/beacon/src/provision-errors.ts @@ -0,0 +1,43 @@ +import { HttpsError } from "firebase-functions/v2/https"; +import type { ProvisionBlockReason } from "@luminova/types"; + +/** + * The tagged refusals `provisionMemberLogin` can be argued with, in a module BOTH the callable + * and its adapter can import. + * + * They live here rather than in `provision-member-login.ts` to dissolve a genuine import cycle: + * the port needs to raise the malformed-email refusal (Identity Toolkit rejects addresses the + * shape screen cannot anticipate), and the callable needs the port. That cycle was safe only + * because every cross-module reference sat inside a hoisted function body — one top-level + * `const` reading across it, which is an ordinary-looking edit, would throw at module + * evaluation, and `index.ts` pulls this graph into the shared entry, so it would take out every + * trigger in the bundle at cold start. Nothing in the repo lints for cycles. Same move + * `firestore-util.ts` already made for the log sinks. + */ + +/** A refusal the CLIENT can name. `reason` is a cross-boundary contract owned by + * `@luminova/types` (PROVISION_BLOCK_REASONS) and consumed by backstage's message table — + * routing every tagged throw through this helper is what makes renaming one a compile + * error on both ends instead of a silent degradation to the generic fallback. */ +export function provisionBlocked( + code: "failed-precondition" | "permission-denied", + message: string, + reason: ProvisionBlockReason, +): HttpsError { + return new HttpsError(code, message, { reason }); +} + +/** The stored email is unusable — absent, empty, wrong shape, or rejected by Identity Toolkit + * itself. One factory because the refusal is raised from TWO layers and must read identically + * from both: `provisionMember` screens the SHAPE up front, and the port tags the SEMANTIC + * rejection the shape screen cannot anticipate. `ADMIN_SDK_EMAIL_SHAPE` is a cheap pre-filter, + * never the sole guarantee — "a@.", ".a@b.co" and "a..b@c.co" each carry one `@`, no + * whitespace and no control characters, so they pass it AND the Admin SDK's own isEmail, reach + * the API, and come back auth/invalid-email. */ +export function memberEmailMalformed(): HttpsError { + return provisionBlocked( + "failed-precondition", + "member's stored email is missing or not a valid address; correct it before provisioning", + "member-email-malformed", + ); +} diff --git a/apps/beacon/src/provision-member-login.test.ts b/apps/beacon/src/provision-member-login.test.ts index 14e9f3c8..c2b13844 100644 --- a/apps/beacon/src/provision-member-login.test.ts +++ b/apps/beacon/src/provision-member-login.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect } from "vitest"; +import { HttpsError } from "firebase-functions/v2/https"; import type { Role } from "@luminova/auth/roles"; import { validateProvisionInput, @@ -9,13 +10,41 @@ import { } from "./provision-member-login.js"; describe("validateProvisionInput", () => { + function codeOf(data: unknown): string { + try { + validateProvisionInput(data); + return "no-throw"; + } catch (err) { + return err instanceof HttpsError ? err.code : "not-an-https-error"; + } + } + it("accepts a clean memberId", () => { expect(validateProvisionInput({ memberId: "m-1" })).toEqual({ memberId: "m-1" }); }); - it("rejects missing / empty / unclean memberId", () => { - expect(() => validateProvisionInput({})).toThrow(); - expect(() => validateProvisionInput({ memberId: "" })).toThrow(); - expect(() => validateProvisionInput({ memberId: "a/b" })).toThrow(); + + it("rejects every unusable memberId with invalid-argument, never a 500", () => { + // The CODE, not merely a throw: `.`, `..` and `__x__` BUILD a valid ref and fail LATER at + // get() with a permanent INVALID_ARGUMENT, which reaches the caller as `internal` — a 500 + // for what is a malformed request. A non-string had no typeof check at all before. Those + // are exactly the shapes moving to isSafeDocId added; the empty and "/"-bearing rows were + // already caught by the hand-rolled check this replaced, and stay as the regression floor. + const unusable: unknown[] = [ + undefined, + "", + "a/b", + ".", + "..", + "__name__", + 42, + { id: "m-1" }, + "x".repeat(1501), + ]; + for (const memberId of unusable) { + expect(codeOf({ memberId })).toBe("invalid-argument"); + } + expect(codeOf({})).toBe("invalid-argument"); + expect(codeOf(null)).toBe("invalid-argument"); }); }); @@ -173,6 +202,7 @@ describe("provisionMember", () => { }); await expect(provisionMember(deps, "m1", false)).rejects.toMatchObject({ code: "permission-denied", + details: { reason: "reprovision-requires-admin" }, }); // Nothing partial: no claim write, no uid link, no reset link generated. expect(calls.setClaims).toEqual([]); @@ -180,17 +210,6 @@ describe("provisionMember", () => { expect(calls.createUser).toEqual([]); }); - it("still lets a non-Admin caller mint a BRAND-NEW account", async () => { - // The delegation costs nothing on the path it is actually for: a genuinely new member - // has neither an Auth account nor a stored uid. - const fresh = fakeDeps({ member: active }); - await expect(provisionMember(fresh.deps, "m1", false)).resolves.toEqual({ - email: "a@b.co", - actionLink: "", - }); - expect(fresh.calls.createUser).toEqual(["a@b.co"]); - }); - it("BLOCKING: a non-Admin caller may NOT re-provision an ALREADY-LINKED member", async () => { // The resend path is an account-takeover primitive, not a convenience. passwordResetLink // is generatePasswordResetLink — it hands the oobCode URL to the CALLER, unlike the @@ -206,6 +225,7 @@ describe("provisionMember", () => { }); await expect(provisionMember(deps, "m1", false)).rejects.toMatchObject({ code: "permission-denied", + details: { reason: "reprovision-requires-admin" }, }); expect(calls.setClaims).toEqual([]); expect(calls.linkUid).toEqual([]); @@ -383,15 +403,23 @@ describe("provisionMember", () => { }); await expect(provisionMember(missing.deps, "m1", false)).rejects.toMatchObject({ code: "permission-denied", + details: { reason: "power-seat-requires-admin" }, }); + // The malformed half pins readCargoIds' `isSafeDocId(cargoId) ? cargoId : ""` mapping, + // which is why the fake ANSWERS "a/b" with a grant-free cargo: without that seed both + // halves would resolve through `opts.positions?.[…] ?? null` alike, and turning the + // mapping into a passthrough would leave this green. Seeded, a passthrough would read the + // grant-free entry and ALLOW. const malformed = fakeDeps({ member: { ...active, positions: { [TERM]: { cargoId: "a/b", comisionIds: [], assignedBy: "admin-uid" } }, }, + positions: { "a/b": [] }, }); await expect(provisionMember(malformed.deps, "m1", false)).rejects.toMatchObject({ code: "permission-denied", + details: { reason: "power-seat-requires-admin" }, }); }); @@ -399,6 +427,13 @@ describe("provisionMember", () => { // Seating plus inviting on a grant-free cargo mints nothing, and is exactly the enrolment // flow the delegation exists for. Without this pair the guard above would pass for a rule // that simply refused every seated member. + // + // NOT subsumed by the delegate half of "never receives the password-reset link", which is + // the claim this test was once deleted on. That fixture is `member: active` — no + // `positions` map at all — so `readCargoIds` yields nothing and the guard's loop body + // never executes. Only this test drives the loop to a resolved cargo and out the ALLOW + // side; a guard rewritten to refuse whenever ANY cargoId is present leaves the rest of + // this file green. const { deps, calls } = fakeDeps({ member: { ...active, @@ -414,11 +449,16 @@ describe("provisionMember", () => { // generatePasswordResetLink returns a bearer credential for the account. The client sends // the reset mail itself through the unprivileged sendPasswordResetEmail, so a delegate has // no need to hold it. Defence in depth behind the power-seat guard, not a substitute. + // + // The delegate half doubles as the paired ALLOW for every BLOCKING case above: the + // delegation costs nothing on the path it is actually for, since a genuinely new member + // has neither an Auth account nor a stored uid — hence the createUser assertion. const delegate = fakeDeps({ member: active }); await expect(provisionMember(delegate.deps, "m1", false)).resolves.toEqual({ email: "a@b.co", actionLink: "", }); + expect(delegate.calls.createUser).toEqual(["a@b.co"]); const admin = fakeDeps({ member: active }); await expect(provisionMember(admin.deps, "m1", true)).resolves.toEqual({ email: "a@b.co", @@ -433,6 +473,7 @@ describe("provisionMember", () => { const { deps, calls } = fakeDeps({ member: { ...active, uid: "dead-uid" } }); await expect(provisionMember(deps, "m1", false)).rejects.toMatchObject({ code: "permission-denied", + details: { reason: "reprovision-requires-admin" }, }); expect(calls.createUser).toEqual([]); }); @@ -443,7 +484,10 @@ describe("provisionMember", () => { member: active, usersByEmail: { "a@b.co": { uid: "u9", email: "a@b.co" } }, }); - await expect(provisionMember(deps, "m1")).rejects.toMatchObject({ code: "permission-denied" }); + await expect(provisionMember(deps, "m1")).rejects.toMatchObject({ + code: "permission-denied", + details: { reason: "reprovision-requires-admin" }, + }); }); it("rejects a missing / inactive / email-less member", async () => { @@ -453,9 +497,77 @@ describe("provisionMember", () => { await expect( provisionMember(fakeDeps({ member: { email: "a@b.co", active: false } }).deps, "m1"), ).rejects.toMatchObject({ code: "failed-precondition" }); - await expect( - provisionMember(fakeDeps({ member: { active: true } }).deps, "m1"), - ).rejects.toMatchObject({ code: "failed-precondition" }); + // BLOCKING: an absent or empty email is TAGGED, like every other refusal. It used to throw + // bare ("member has no email"), so `provisionRefusalMessage` returned null and the operator + // got the generic "No se pudo…" — the dead end PROVISION_BLOCK_REASONS exists to remove — + // and it shadowed the tagged malformed-email refusal for the "" case, which is the likelier + // one (memberDocSchema's `email` is a bare z.string()). + for (const member of [{ active: true }, { active: true, email: "" }]) { + await expect(provisionMember(fakeDeps({ member }).deps, "m1")).rejects.toMatchObject({ + code: "failed-precondition", + details: { reason: "member-email-malformed" }, + }); + } + }); + + it("BLOCKING: screens a malformed stored email instead of surfacing an opaque `internal`", async () => { + // `email` reaches auth.getUserByEmail / auth.createUser, which reject anything outside the + // Admin SDK's shape with a PERMANENT auth/invalid-email. nullIfUserNotFound only swallows + // auth/user-not-found, so it rethrows and the operator sees `internal` — with no hint that + // the fix is editing the member's stored email. firestore.rules does not shape-validate + // email on the admin write lane, so this shape is reachable. + const reached: string[] = []; + // The last four are the tightening over the SDK's own `/^[^@]+@[^@]+$/`: `[^@]` matches + // whitespace and control characters, so each of these passes that pattern AND the SDK's + // client-side check, reaches Identity Toolkit, and returns INVALID_EMAIL as an opaque + // `internal` — the exact failure this screen exists to prevent, one layer further out. + for (const email of [ + "not-an-email", + "@b.co", + "a@", + "a@b@c.co", + " ", + "pres@jci.bo\n", + "a b@jci.bo", + "a@b\t.bo", + "a@b.bo", + ]) { + const { deps, calls } = fakeDeps({ member: { email, active: true } }); + const spied: ProvisionDeps = { + ...deps, + getUserByEmail: async (value) => { + reached.push(value); + return deps.getUserByEmail(value); + }, + }; + await expect(provisionMember(spied, "m1", true)).rejects.toMatchObject({ + code: "failed-precondition", + details: { reason: "member-email-malformed" }, + }); + // Screened before the SDK sees it — that is the whole point of the check. + expect(reached).toEqual([]); + expect(calls.createUser).toEqual([]); + expect(calls.linkUid).toEqual([]); + } + }); + + it("does NOT refuse the unusual addresses the Admin SDK accepts", async () => { + // The screen is the SDK's own predicate plus a whitespace/control-character exclusion — + // NOT an RFC validator, and the distinction is the whole point of this row. Excluding + // characters Identity Toolkit rejects anyway costs nothing. Adding RFC STRUCTURE (dot + // placement, label rules, a TLD requirement) would start refusing addresses Firebase + // happily creates accounts for, which is the failure the screen exists to prevent pointed + // the other way — and that failure is now silent-proof from the other side too, since the + // port tags Identity Toolkit's own rejection rather than letting it surface as `internal`. + // A plus-tag, a bare hostname and a non-ASCII local part must all keep provisioning. + for (const email of ["ana+jci@sub.example.co", "root@localhost", "añez@ejemplo.bo"]) { + const { deps, calls } = fakeDeps({ member: { email, active: true } }); + await expect(provisionMember(deps, "m1", true)).resolves.toEqual({ + email, + actionLink: `link:${email}`, + }); + expect(calls.createUser).toEqual([email]); + } }); }); diff --git a/apps/beacon/src/provision-member-login.ts b/apps/beacon/src/provision-member-login.ts index 01c07b86..11274ab7 100644 --- a/apps/beacon/src/provision-member-login.ts +++ b/apps/beacon/src/provision-member-login.ts @@ -3,6 +3,7 @@ import { getFirestore } from "firebase-admin/firestore"; import { HttpsError, onCall } from "firebase-functions/v2/https"; import { isValidRole, type Role } from "@luminova/auth/roles"; import { isSafeDocId } from "./firestore-util.js"; +import { memberEmailMalformed, provisionBlocked } from "./provision-errors.js"; import { callerIsAdmin, requireAdminOrPerm } from "./callable-auth.js"; import { firestoreProvisionDeps } from "./provision-deps.js"; import { ensureApp } from "./runtime.js"; @@ -35,6 +36,28 @@ export function nextClaims(existing: RawClaims | undefined, role: Role): { roles return { roles }; } +// provisionBlocked / memberEmailMalformed live in ./provision-errors.js — the port raises the +// malformed-email refusal too, and keeping the factories here made provision-deps.ts and this +// module a two-node import cycle. + +/** The Admin SDK's OWN email predicate (`validator.isEmail`: `/^[^@]+@[^@]+$/`), plus the one + * tightening that is strictly safe: no whitespace, no control characters. + * + * Deliberately not an RFC-ish pattern — a stricter one would start rejecting addresses + * Firebase happily accepts, which is a worse failure than the one being fixed. But `[^@]` + * matches `\n`, `\r`, `\t`, spaces and NUL, so `"pres@jci.bo\n"` and `"a b@jci.bo"` pass BOTH + * this screen and the SDK's client-side check and reach Identity Toolkit, which rejects them. + * `firestore.rules` never constrains `members.email`, so a CSV paste or any `update:Member` + * holder can store one. + * + * This is a PRE-FILTER, not the guarantee. The port tags Identity Toolkit's own + * `auth/invalid-email` with the same reason (see ./provision-errors.js), so a shape that slips + * through — `a@.`, `.a@b.co` — no longer surfaces as an opaque `internal`. The screen still + * earns its keep: it refuses one round-trip earlier, never puts a junk address on the Auth + * API, and makes the refusal identical whether or not Identity Toolkit happens to reject that + * particular shape. */ +const ADMIN_SDK_EMAIL_SHAPE = /^[^@\s\p{C}]+@[^@\s\p{C}]+$/u; + export interface ProvisionUser { uid: string; email?: string; @@ -158,8 +181,21 @@ export async function provisionMember( const member = await deps.getMember(memberId); if (member === null) throw new HttpsError("not-found", "member not found"); if (member.active !== true) throw new HttpsError("failed-precondition", "member is not active"); - if (typeof member.email !== "string" || member.email.length === 0) { - throw new HttpsError("failed-precondition", "member has no email"); + // Shape-screened BEFORE it reaches the Auth SDK, for the same reason cargoId and assignedBy + // are screened in claims-sync: a stored value the SDK rejects throws a PERMANENT + // auth/invalid-email. That used to reach the caller as an opaque `internal`, leaving the + // member unprovisionable with no hint why; the port now tags that case with this same reason, + // so this check is the cheap first line rather than the only one. firestore.rules + // deliberately does not shape-validate `email` on the admin write lane, so the shape reaches + // here unchecked. + // + // ONE check, not a separate untagged "member has no email" above it. That one threw with no + // `details.reason`, so the UI degraded it to the generic "no se pudo" — verbatim the dead end + // PROVISION_BLOCK_REASONS exists to remove — and it SHADOWED this tagged one for the + // empty-string case, which is the likelier of the two (memberDocSchema's `email` is a bare + // z.string()). Absent, empty and malformed all have the same operator remedy: fix the ficha. + if (typeof member.email !== "string" || !ADMIN_SDK_EMAIL_SHAPE.test(member.email)) { + throw memberEmailMalformed(); } const email = member.email; const linkedUid = typeof member.uid === "string" && member.uid.length > 0 ? member.uid : null; @@ -170,10 +206,10 @@ export async function provisionMember( // orphaned; if it was deleted out-of-band, relinking by email is the // self-heal, not a conflict. if ((await deps.getUserByUid(linkedUid)) !== null) { - throw new HttpsError( + throw provisionBlocked( "failed-precondition", "member is already linked to a different login; unlink it explicitly before re-provisioning", - { reason: "linked-to-different-login" }, + "linked-to-different-login", ); } } @@ -204,10 +240,10 @@ export async function provisionMember( // member has neither — and leaves resend/adoption/self-heal to an Admin, plus the // client-side sendPasswordResetEmail any member can already use on their own address. if (!callerHoldsAdminRole && (user !== null || linkedUid !== null)) { - throw new HttpsError( + throw provisionBlocked( "permission-denied", "this member already has a login; only an Admin can re-provision or link one", - { reason: "reprovision-requires-admin" }, + "reprovision-requires-admin", ); } // POWER-SEAT GUARD. The check above asks whether this is a NEW login; it does not ask whose @@ -232,19 +268,19 @@ export async function provisionMember( if (!callerHoldsAdminRole) { // Direct grants first — no read required, and it is the half a cargo check cannot see. if (hasDirectGrants(member)) { - throw new HttpsError( + throw provisionBlocked( "permission-denied", "this member has been granted roles or permissions; only an Admin can provision their login", - { reason: "granted-member-requires-admin" }, + "granted-member-requires-admin", ); } for (const cargoId of readCargoIds(member)) { const grants = await deps.getPositionGrants(cargoId); if (grants === null || grants.length > 0) { - throw new HttpsError( + throw provisionBlocked( "permission-denied", "this member holds a cargo that confers permissions; only an Admin can provision their login", - { reason: "power-seat-requires-admin" }, + "power-seat-requires-admin", ); } } diff --git a/apps/beacon/src/read-position-grants.test.ts b/apps/beacon/src/read-position-grants.test.ts index 3f0a5e64..9db58155 100644 --- a/apps/beacon/src/read-position-grants.test.ts +++ b/apps/beacon/src/read-position-grants.test.ts @@ -41,6 +41,41 @@ describe("readPositionGrants", () => { await expect(readPositionGrants(db, 42)).resolves.toBeNull(); }); + it("logs every null through the injected sink — none is visible to either caller otherwise", async () => { + // Guardrail #4. `null` is fail-closed in BOTH directions (grant-free to the claims trust + // gate, power-conferring to the provisioning guard) and neither throws: the trust gate + // just mints nothing and the member is published on the Directiva with no roles. Every + // shape here is an anomaly no legitimate flow produces — a routine grant-free cargo + // returns [], not null — so an operator gets one line per occurrence or nothing at all. + const logged: { message: string; meta: Record }[] = []; + const log = (message: string, meta: Record) => logged.push({ message, meta }); + const db = fakeDb({ "positions/str": { grants: "Admin" } }); + await readPositionGrants(db, "a/b", log); + await readPositionGrants(db, 42, log); + await readPositionGrants(db, "ghost", log); + await readPositionGrants(db, "str", log); + expect(logged.map((l) => l.message)).toEqual([ + expect.stringMatching(/not a usable doc id/), + expect.stringMatching(/not a usable doc id/), + expect.stringMatching(/missing/), + expect.stringMatching(/not an array/), + ]); + // Ids, never the doc — and bounded, because Cloud Logging drops an over-large entry + // whole and isSafeDocId tolerates 1500 bytes. + await readPositionGrants(db, "x".repeat(1501), log); + expect(String(logged[4].meta.cargoId).length).toBeLessThanOrEqual(65); + }); + + it("stays quiet on the paths that resolve", async () => { + // The paired negative: a well-formed cargo — grant-bearing or grant-free — is the routine + // case on every member write, and logging it would drown the anomalies above. + const logged: string[] = []; + const db = fakeDb({ "positions/p1": { grants: ["Admin"] }, "positions/p2": {} }); + await readPositionGrants(db, "p1", (message) => logged.push(message)); + await readPositionGrants(db, "p2", (message) => logged.push(message)); + expect(logged).toEqual([]); + }); + it("BLOCKING: returns null instead of THROWING on a non-array grants field", async () => { // firestore.rules short-circuits every grants check on hasAnyRole(['Admin']) and never // type-checks the field, so a console edit or a migration can store one of these. A diff --git a/apps/beacon/src/read-position-grants.ts b/apps/beacon/src/read-position-grants.ts index 689d04bc..01362ffd 100644 --- a/apps/beacon/src/read-position-grants.ts +++ b/apps/beacon/src/read-position-grants.ts @@ -1,6 +1,6 @@ import type { Firestore } from "firebase-admin/firestore"; import { isValidRole, type Role } from "@luminova/auth/roles"; -import { isSafeDocId } from "./firestore-util.js"; +import { isSafeDocId, truncateForLog, type LogSink } from "./firestore-util.js"; /** A cargo's trusted grants, or null when the id is unusable or the doc is missing. * @@ -12,11 +12,35 @@ import { isSafeDocId } from "./firestore-util.js"; * `null` is the answer for an unusable id, a missing doc, or a malformed `grants` field, and * it is fail-closed for BOTH callers: an unreadable cargo is treated as * power-conferring by the guard and as grant-free by the trust gate, which is the safe - * direction in each. */ -export async function readPositionGrants(db: Firestore, id: unknown): Promise { - if (!isSafeDocId(id)) return null; + * direction in each. + * + * `logError` is injected rather than hard-coded to `console.error` so this shared read has + * one sink per port, the same way claims-sync's `deps.logError` does — a later change to + * where these lines go should not have to be made twice. */ +export async function readPositionGrants( + db: Firestore, + id: unknown, + logError?: LogSink, +): Promise { + // Every `null` below is logged (guardrail #4). Each one is an anomaly no legitimate flow + // produces — a routine grant-free cargo returns `[]`, not null — and each is invisible + // downstream: the trust gate mints nothing and the provisioning guard refuses, with no + // throw and no metric either side. Ids only, bounded — never the doc. + if (!isSafeDocId(id)) { + logError?.("positions: cargo id is not a usable doc id — cargo unresolvable", { + cargoIdType: typeof id, + cargoId: typeof id === "string" ? truncateForLog(id) : null, + cargoIdLength: typeof id === "string" ? id.length : null, + }); + return null; + } const snap = await db.doc(`positions/${id}`).get(); - if (!snap.exists) return null; + if (!snap.exists) { + logError?.("positions: cargo doc is missing — cargo unresolvable", { + cargoId: truncateForLog(id), + }); + return null; + } // A non-array `grants` returns null rather than throwing. firestore.rules short-circuits // every grants check on hasAnyRole(['Admin']) and never type-checks the field, so a console // edit or a migration can store a string or a map — and `.filter` on that is a TypeError. @@ -25,7 +49,13 @@ export async function readPositionGrants(db: Firestore, id: unknown): Promise isValidRole(g)); } diff --git a/apps/beacon/src/recompute-claims.ts b/apps/beacon/src/recompute-claims.ts index 3f776d28..9909fa0f 100644 --- a/apps/beacon/src/recompute-claims.ts +++ b/apps/beacon/src/recompute-claims.ts @@ -11,7 +11,7 @@ import { rawPermsFromRoleDoc, roleDocPermsMalformed, } from "./claims-sync/role-doc.js"; -import { firestoreClaimsDeps } from "./claims-sync/firestore-deps.js"; +import { firestoreClaimsDeps, logError } from "./claims-sync/firestore-deps.js"; import { syncMemberClaims } from "./claims-sync/sync.js"; import { parseMember, MEMBER_SYNC_FIELDS } from "./claims-sync/parse-member.js"; import { seedBuiltInRoles } from "./seed-roles.js"; @@ -74,7 +74,7 @@ export const recomputeAllClaims = onCall( let synced = 0; const failed: string[] = []; for (const doc of snap.docs) { - const member = parseMember(doc.data()); + const member = parseMember(doc.data(), { memberId: doc.id, logError }); if (!member.uid) continue; try { await syncMemberClaims(deps, member, termKey); diff --git a/docs/engineering-guardrails.md b/docs/engineering-guardrails.md index 13acd3ae..118b6376 100644 --- a/docs/engineering-guardrails.md +++ b/docs/engineering-guardrails.md @@ -146,7 +146,7 @@ splatting an unbounded ref list — fine at 40 docs, a cost/timeout bomb at 4,00 | Site | State | |---|---| | Spotlight `fetchFeatured` pulled the whole `showcase` collection, filtered client-side | Fixed (item 12): server-side `where("featured", "==", true)` — `apps/spotlight/src/showcase/showcase-firestore.ts:23` | -| `getRolesByIds` did `db.getAll(...refs)` on an uncapped `roleIds` list | Fixed (#145): chunk-at-300 via `chunk()` — `apps/beacon/src/claims-sync/firestore-deps.ts:169` batches over `apps/beacon/src/chunk.ts` | +| `getRolesByIds` did `db.getAll(...refs)` on an uncapped `roleIds` list | Fixed (#145): chunk-at-300 via `chunk()` — `getRolesByIds` in `apps/beacon/src/claims-sync/firestore-deps.ts` batches over `apps/beacon/src/chunk.ts` | | Beacon `onRoleWritten` built-in-role branch scans **all** members — `apps/beacon/src/index.ts:281` | Bounded (#145) by `roleClaimsChanged` early-return: a metadata-only edit skips the scan entirely; a real permission change still scans every holder **by design** — do NOT cap with `.limit()`, which would strand members beyond the cap with stale claims | | Backstage `RoleRepository.getAll()` reads the whole `roles` collection — no `where`, no `.limit` — `apps/backstage/src/features/permissions/repositories/role-repository.ts:35` | **Recorded exception**, not drift (see below) | | Beacon `getRoleDocsByBuiltInKeys` reads `roles` with `where("builtInKey","in",keys)` and no `.limit` — `apps/beacon/src/claims-sync/firestore-deps.ts` | **Recorded exception**, not drift (see below) — the mirror of the `RoleRepository.getAll()` one, on the trigger side | diff --git a/docs/plans/board-seat-delegation.md b/docs/plans/board-seat-delegation.md index 61207936..d705609d 100644 --- a/docs/plans/board-seat-delegation.md +++ b/docs/plans/board-seat-delegation.md @@ -2,6 +2,18 @@ Branch: `feat/board-seat-delegation` +> **SUPERSEDED — point-in-time artifact, not current guidance.** This plan was written before +> the design went through two adversarial reviews and 15 commits, and it was never updated as +> the design changed under it. `docs/specs/board-seat-delegation.md` is the authority on what +> actually shipped. Most importantly: **section 0b's G2 guard, below, describes a form of the +> self-assignment fix that was proposed, found to be wrong, and REJECTED** — it would have +> stripped the seeded bootstrap president's `Admin` claim on their very next member write, a +> near production outage. Do not implement G2 as written. See the correction inline at G2 and +> the spec's "Two earlier forms of this guard were wrong" for the full account. Kept in the repo +> as the commit-by-commit record of how the design evolved (per the "Fact-check corrections" +> table near the end, this document corrects itself in several other places too) — read it as +> history, not as a spec to implement from. + ## 0. Accepted decision The chapter owner has accepted that `update:BoardSeat` is a **claims-minting delegation**: a @@ -15,6 +27,14 @@ delegate can still seat any vacant cargo, including `Presidente`. ## 0b. Security guards (added after adversarial review — G1/G2/G3) +> **SUPERSEDED section.** G1 and G2 below describe the FIRST proposed shape of each guard, from +> before implementation. Neither is what shipped — G1's shipped guard is narrower, and G2's +> proposed fix was rejected outright (see the correction under G2). Section 0b also predates +> three guards that were added later, in response: the power-seat guard in `provisionMember`, +> fail-closed handling for a malformed `grants`/`positions` shape, and a narrowed `createUser` +> catch — listed under "Guards added after this section was written", below. Treat this whole +> section as the audit trail, and `docs/specs/board-seat-delegation.md` as the design authority. + Two independent reviews found three defects, all confirmed by reading code. Each guard below removes a capability the owner did **not** ask for, and none removes one they did. @@ -30,30 +50,83 @@ claims via `adoptedClaims` at `:109`, (c) `linkUid` the Admin's uid onto their o the admin SDK at `:111`, and (d) return a **password-reset link for the Admin's email** at `:112-114`. Full account takeover, unrelated to board seating. -**Guard:** thread the caller's privilege into `provisionMember`. For a non-Admin caller require +**Guard, as first proposed here — NARROWER in the shipped code, see correction:** thread the +caller's privilege into `provisionMember`. For a non-Admin caller require `user === null || user.uid === linkedUid` **before** `:97` — a delegate may create a brand-new Auth account or re-provision an already-linked one, never adopt a pre-existing account. Costs the delegate nothing: a genuinely new member has no existing account. +**Correction — what shipped is stricter than this.** The resend/re-provision half of this +proposal was itself found to be a hole and closed before merge: +`generatePasswordResetLink` (`passwordResetLink` in the `ProvisionDeps` port) returns the oobCode +URL **to the caller**, categorically unlike `sendPasswordResetEmail`, which delivers the secret +to the mailbox owner. A delegate permitted to "re-provision an already-linked" member could name +any member's id — including an Admin's — and receive a live reset link for that address. The +shipped guard in `provisionMember` (the ADOPTION GUARD in +`apps/beacon/src/provision-member-login.ts`) is therefore: + +```ts +if (!callerHoldsAdminRole && (user !== null || linkedUid !== null)) { + throw new HttpsError("permission-denied", /* ... */); +} +``` + +A non-Admin caller gets exactly one shape — mint a brand-new Auth account for a member that has +**neither** an existing Auth user for its email **nor** a stored `uid`. Resend, adoption, and the +deleted-account self-heal are all Admin-only; there is no re-provision path left for a delegate at +all. See `docs/specs/board-seat-delegation.md` § "`create:MemberLogin` — what is actually +privileged" for the full account, including the power-seat guard this section does not mention +(added later — see below). + Note also: the invite **email** is not the privileged part. `requestPasswordReset` (`apps/backstage/src/lib/auth/request-password-reset.ts`) is a plain client-side `sendPasswordResetEmail` any signed-in user can already call. The callable's privilege is account creation/adoption + uid linking + claim writing. -### G2 — the trust gate must be non-reflexive on self-assignment - -`sync.ts:69` + `compute-roles.ts:8-12`. A delegate self-seats `Presidente` in one write on the -positions-only lane; beacon mints `roles: ["Admin","Member"]`. Revoking `update:BoardSeat` then -re-fires `onMemberWritten`, the gate reads their **live** claims, finds the `Admin` role the -cargo just minted, and re-honors the grants. The claim satisfies the gate that minted it, so the -delegation is permanent. `recomputeAllClaims` runs the same code and does not break the loop. +### G2 — the trust gate must be non-reflexive on self-assignment — proposed fix REJECTED, do not implement as written + +`sync.ts:69` + `compute-roles.ts:8-12` (pre-implementation line refs; current shipped location is +`resolveTrustedGrants` in `apps/beacon/src/claims-sync/sync.ts`). A delegate self-seats +`Presidente` in one write on the positions-only lane; beacon mints `roles: ["Admin","Member"]`. +Revoking `update:BoardSeat` then re-fires `onMemberWritten`, the gate reads their **live** claims, +finds the `Admin` role the cargo just minted, and re-honors the grants. The claim satisfies the +gate that minted it, so the delegation is permanent. `recomputeAllClaims` runs the same code and +does not break the loop. This diagnosis stands; only the fix below was wrong. + +**Guard as first proposed here — REJECTED, DO NOT RE-PROPOSE:** when `assignedBy === member.uid`, +trust **only** `assigner.perms.includes("update:BoardSeat")`, never `assigner.roles.includes("Admin")`. + +**Why this is wrong, and is the exact inverse of what shipped:** the seeded bootstrap president +self-stamps `assignedBy` with their own uid (`tools/scripts/lib/seed-president.mjs`), and an +Admin's perms are `manage:all` — never the exact `update:BoardSeat` code. Keying the +self-assignment trust check on the perm, as proposed above, means the sitting president's own +`Admin` claim would be **stripped on their next member write**. Confirmed against the live +production member doc before this shipped; a regression test pins it. This form also does not +close the two-write puppet loop (a delegate seats a SECOND member, not themselves, on +`Presidente` — not a self-assignment, so it is still trusted) — see +`docs/specs/board-seat-delegation.md` § "Two earlier forms of this guard were wrong" for both +failure modes in full. + +**What shipped instead:** self-assignment is honored ONLY for an assigner holding the Admin +**role** — the opposite disjunct from the one proposed above — and the same rule extends to any +cargo whose `grants` include `Admin`, self-assigned or not: + +```ts +const assignerIsAdmin = assigner.roles.includes("Admin"); +const selfAssigned = assignedBy === memberUid; +const trusted = + position.grants.includes("Admin") || selfAssigned + ? assignerIsAdmin + : assignerIsAdmin || assigner.perms.includes("update:BoardSeat"); +``` -**Guard:** when `assignedBy === member.uid`, trust **only** `assigner.perms.includes("update:BoardSeat")`, -never `assigner.roles.includes("Admin")`. Cargo-derived Admin can no longer bootstrap its own -trust. An Admin seating *someone else* is unaffected; an Admin self-seating still works via the -perm if they hold it, and via a second Admin otherwise. +An Admin seating *someone else* on a non-Admin power cargo is unaffected either way; an Admin +self-seating still works because they hold the Admin role. Revoking `update:BoardSeat` from a +delegate is then real for everything they *can* confer, because no cargo-derived Admin can ever +originate from a delegate in the first place — there is no loop to close. -Test: assigner === target, `roles:["Admin"]`, `perms:[]` → grants dropped. +Test: assigner === target, `roles:["Admin"]`, `perms:[]` → grants **still honored** (assigner is +Admin) — the inverse of what the rejected proposal above would have asserted. ### G3 — `currentCargoGrantsEmpty()` stays Admin-only @@ -78,6 +151,35 @@ substitution. If the owner later wants de-elevation delegated too, that is a separate code with its own anti-lockout guard — not this one. +### Guards added after this section was written + +Later commits on this branch (`5f9408e`, `e02dbf1`, `d7cd564`, `2c328f5`, `58266d3`) found and +closed three more gaps this section does not mention. Cited by SYMBOL, never by line number: +every numeric citation this block originally carried was stale by 25-45 lines within the same +PR that wrote them — one of them, "the real trust computation, unchanged since", ended up +pointing at a logging block added afterwards, which is exactly how an auditor concludes a guard +was removed. Guardrail #6 is about claims that stay true, and a line number does not. + +- **The power-seat guard in `provisionMember`.** G1 above stops a delegate from re-provisioning + or adopting an ALREADY-linked account, but says nothing about an unlinked member who is already + granted or already power-seated — "unprovisioned" is not "enrolled by this delegate". Without a + further check, `provisionMemberLogin` would link a fresh Auth uid onto such a member, fire + `onMemberWritten`, and `resolveTrustedGrants` would read the *stored* `assignedBy` (a genuine + Admin) and mint the grants onto the account the delegate's call just created — a clean + escalation the delegate never had to forge. The shipped guard + (the `if (!callerHoldsAdminRole)` block in `apps/beacon/src/provision-member-login.ts`) checks + both claims-mint sources + `syncMemberClaims` reads: `hasDirectGrants()` (`roleIds`/`permissionOverrides`) and a per-term + cargo read via `readCargoIds()` (every term in `positions`, not just the current one — a future-term slate is invisible to claims-sync today but not to this guard). +- **Fail-closed handling for a malformed `grants` or `positions` shape.** `readCargoIds()` yields `""` — not skip — for a non-object term, a non-string `cargoId`, or one + `isSafeDocId` rejects; the caller then refuses on `grants === null`, so a shape it cannot parse + is treated as power-seated, never as "no cargo". `hasDirectGrants()` is symmetric for + `roleIds` / `permissionOverrides`: a present-but-unparseable value reads as granted, and only a + genuinely absent/null value reads as ungranted. +- **The narrowed `createUser` catch.** `createUser` in `apps/beacon/src/provision-deps.ts` swallows exactly + `auth/email-already-exists` (a benign race with a concurrent create) and rethrows everything + else — it does not swallow arbitrary Auth errors into a silent fallback. + ## The two codes | Subject | Spanish label | Live code | Grants | @@ -119,7 +221,7 @@ delegate whose write `firestore.rules` then rejects: the render-then-403 shape ` `checkIn:MemberPoints` / `checkIn:Notification` (20 chars). `checkIn:MemberLogin` is 19, `checkIn:BoardSeat` is 17. Worst case stays ~855 B. `PERMISSION_CAP` does not move. - Product consequence worth one spec line: two more codes compete for the same 30-slot effective-perm - budget, and a member breaching the cap gets `perms: []` fail-closed (`sync.ts:106-118`) — silently + budget, and a member breaching the cap gets `perms: []` fail-closed (the `perms.length > PERMISSION_CAP` block in `sync.ts`) — silently removing their `update:BoardSeat`. - `sync.test.ts:580` `distinctCodes(n)` walks `ACTIONS x SUBJECTS` in order; adding subjects changes which codes it picks, not their validity. No change. @@ -319,11 +421,27 @@ Rejected alternatives: return trusted ? [...new Set(position.grants)] : []; ``` + **This snippet is superseded — it is missing the self-assignment / Admin-granting-cargo branch + that G2 (above) required and that shipped.** The real trust computation, unchanged since, + reads (the trust computation at the end of `resolveTrustedGrants`, + `apps/beacon/src/claims-sync/sync.ts`): + + ```ts + const assignerIsAdmin = assigner.roles.includes("Admin"); + const selfAssigned = assignedBy === memberUid; + const trusted = + position.grants.includes("Admin") || selfAssigned + ? assignerIsAdmin + : assignerIsAdmin || assigner.perms.includes("update:BoardSeat"); + return trusted ? [...new Set(position.grants)] : []; + ``` + Extend the doc comment: name `update:BoardSeat` as the second trust source; state why (a delegate stamps their own uid into `assignedBy` via `assignedBySelf()`, so without this the seat publishes on the public Directiva and mints nothing — half-working, not safe); state the live-claims re-evaluation now also covers perm revocation; cross-reference `boardSeatDelegate()` in - `firestore.rules`. One line on the cap interaction (C2). + `firestore.rules`. One line on the cap interaction (C2). State the self-assignment / Admin-grant + exception too (G2) — it is not a detail, it is the guard the whole delegation rests on. 2. `apps/beacon/src/claims-sync/firestore-deps.ts` — rename the impl at `:287`, returning both, reusing the two module-local readers already there. No new read: `loadUser` is the same per-instance memo. diff --git a/docs/specs/board-seat-delegation.md b/docs/specs/board-seat-delegation.md index 9597dace..515a8c57 100644 --- a/docs/specs/board-seat-delegation.md +++ b/docs/specs/board-seat-delegation.md @@ -139,9 +139,20 @@ This costs the delegation nothing: a genuinely new member has neither an account `update:BoardSeat` override; an org-chart-only delegate wants `update:Position` instead. 2. **The delegate must sign out and back in after being granted or revoked.** All three gates — `firestore.rules`, `requireAdminOrPerm` and `useCan` — read the `perms` claim off the ID - token, and `auth-store.ts` calls `getIdTokenResult()` without `forceRefresh`. A freshly - granted code is invisible for up to an hour otherwise, and a freshly revoked one keeps - working for up to an hour. + token, and `auth-store.ts` calls `getIdTokenResult()` without `forceRefresh` — it is the only + place the app reads a token, there is no `onIdTokenChanged` listener anywhere, and no code + path forces a refresh. A freshly granted code is invisible for up to an hour otherwise, and a + freshly revoked one keeps working for up to an hour: the delegate's UI keeps its affordances + until they reload, and both `hasPerm()` in `firestore.rules` and `requireAdminOrPerm` in + beacon keep honoring the revoked code from their cached ID token until it expires. Signing out + and back in **does** mint a fresh token off the current (revoked) claims — but that is the + delegate's choice to make, not the Admin's; nothing forces it, and an Admin who revokes the + code has no lever to shorten the window on a delegate who simply keeps their existing session + open. The only immediate, Admin-side cut is `admin.auth().revokeRefreshTokens(uid)` on the + delegate's uid — it invalidates every outstanding session and forces re-auth on the next + request, regardless of what the delegate does. Nothing in this codebase currently calls it; it + is a console or one-off-script op, run by hand when "revoked" needs to mean "now" rather than + "within the hour". 3. **A delegate's failed provision escalates to an Admin.** If `provisionMemberLogin` fails after creating the Auth account (a transient `setClaims`/`linkUid` error), the retry is refused by the adoption guard — the account now exists, so `user !== null`. Finish it from @@ -167,6 +178,50 @@ This costs the delegation nothing: a genuinely new member has neither an account `resolveTrustedGrants` declines to mint the grants. The member is then published on the public Directiva with no claim — visible, powerless. Re-running the write after the token refreshes resolves it. +8. **The puppet path — say plainly what the three codes together can reach.** The + self-assignment guard (above) blocks the ONE-WRITE form of self-promotion: a delegate cannot + seat *themselves* on a power cargo and have it mint. It does **not** block the multi-write + form, and nothing else in the system does either. A holder of a member-creation capability + (`create:Member` or `manage:Member`) plus `update:BoardSeat` plus `create:MemberLogin` can: + create a second member on a mailbox they control, `provisionMemberLogin` it (the power-seat + guard does not fire — the puppet is brand new and unseated), sign in as the puppet, and then — + back as themselves — seat the *puppet* on any non-Admin power-granting cargo. That is not a + self-assignment (`assignedBy` is the delegate's uid, the seated member is the puppet's), so + `resolveTrustedGrants` trusts it on the `update:BoardSeat` perm alone. The delegate now + controls, through the puppet's session, every built-in role's capabilities short of `Admin`. + `Admin` itself stays closed — `resolveTrustedGrants` refuses an Admin-granting cargo from any + assigner who is not Admin-by-role, puppet or not. **Say the ceiling correctly: it is "everything + but Admin," not "nothing."** Granting all three codes to one person is materially different + from granting them to three different people, and nothing in the UI distinguishes that for the + Admin doing the granting. +9. **What survives a revocation — nothing removes the account, and nothing surfaces who a + delegate seated.** Two facts worth stating together, because they compound: + - No code path in `apps/beacon/src` calls `deleteUser` or `updateUser(..., { disabled: true + })` — confirmed by grep, there is no disable/delete of an Auth user anywhere in this + codebase. Revoking `update:BoardSeat` or `create:MemberLogin` never touches the Auth account + a delegate (or their puppet) created. + - `syncMemberClaims` (`apps/beacon/src/claims-sync/sync.ts`) never reads `active` or + `membershipStatus` on the member doc — only `positions`, `roleIds` and + `permissionOverrides`. Soft-deleting a member (`active: false` / `deletedAt` set) or marking + them `"Desafiliado"` does not drop their claims; their `Member` role, and any cargo-derived + role, survives until something else re-triggers `syncMemberClaims` with different inputs. + - Nothing in `apps/backstage` renders `assignedBy` — it is written by the repository layer + (`member-mapper.ts:53,92,102`) but read by no `.tsx` in the app. An Admin who revokes a + delegate's codes has **no in-app way** to find which members that delegate seated; the only + path is a manual Firestore query on `positions..assignedBy`. + + **Post-revocation checklist for an Admin** (do all four; none is optional): + 1. Query `members` for every `positions..assignedBy == ` across every term + key present (console query, or a one-off script — no in-app view does this). + 2. For each match, decide by hand whether the seat should stand; unseat or re-seat as an + Admin action if not (a delegate/former-delegate cannot fix this after their code is gone). + 3. Run `recomputeAllClaims` (or re-write each affected member doc) so cargo-derived claims + resolve against the delegate's now-revoked authority, closing the "recomputed on the next + write, which may be never" gap in note 4. + 4. If the delegate might have created a puppet member (note 8), find any member whose `email` + the delegate plausibly controls and whose account was provisioned in the delegation window; + it is not deleted or disabled by revocation and must be handled explicitly — `Member` role + and directory read access persist until removed by hand in the Firebase console. ## Out of scope diff --git a/docs/specs/position-assignment-lane.md b/docs/specs/position-assignment-lane.md index db7ffdb8..af09721c 100644 --- a/docs/specs/position-assignment-lane.md +++ b/docs/specs/position-assignment-lane.md @@ -72,6 +72,18 @@ assignable-only semantic this lane wants. `currentCargoGrantsEmpty()` must stay non-Admin branch: without it an `update:Position` holder could overwrite a president's power cargo with a grant-free one and silently strip Admin. +> **Superseded by one disjunct.** `docs/specs/board-seat-delegation.md` (branch +> `feat/board-seat-delegation`, after this PR) widens the NEW-side cargo conjunct from +> `cargoAssignableByNonAdmin()` to `boardSeatDelegate() || cargoAssignableByNonAdmin()` +> (`positionsAssignmentSafe()`, `firestore.rules:228-232`), so the "grant-free cargos only, on +> BOTH sides of a swap" claim +> above is no longer complete: an `update:BoardSeat` holder on this lane may now also assign a +> **power-granting** cargo (CEL or otherwise) to someone else — beacon's claims-sync trust gate +> honors that assignment too, unless it would mint `Admin` or the assignment is self-seating, +> both of which stay Admin-role-only. The OLD side, `currentCargoGrantsEmpty()`, is untouched by +> that branch — a delegate still cannot displace a sitting power-cargo holder. See the spec for +> the full trust-gate design. + `hasOnly(['positions'])` is the correct constraint for the production dot-path payload `{"positions.2026": {...}}` — `affectedKeys()` reports **top-level** keys, pinned by `rules.test.ts:2262-2270` and `:1310`. diff --git a/firestore.rules b/firestore.rules index c6f13eae..3c26dbeb 100644 --- a/firestore.rules +++ b/firestore.rules @@ -217,10 +217,14 @@ service cloud.firestore { // no Admin: setUserRoles is requireAdmin, and roles/* plus permissionOverrides // writes are all hasAnyRole(['Admin']). The chapter would be unrecoverable outside // the Firebase console. A delegate therefore seats VACANT and grant-free cargos and - // cannot displace a power-cargo holder; hand-over stays an Admin action. + // cannot displace a power-cargo holder; hand-over stays an Admin action. One caveat: + // currentCargoGrantsEmpty() reads positions[currentTermKey()] only, so in the window + // after a UTC-year rollover a sitting Admin's current-term slot is empty and a + // delegate CAN write into it — pre-existing, disclosed in + // docs/specs/board-seat-delegation.md, not something this feature introduced. // // A non-delegate is unchanged: both conjuncts still bind, so the deliberately - // asymmetric grant-free-CEL takedown below still works for an update:Position holder. + // asymmetric grant-free-CEL takedown above still works for an update:Position holder. function positionsAssignmentSafe() { return positionsDelta().hasOnly([currentTermKey()]) && assignedBySelf() diff --git a/packages/auth/CLAUDE.md b/packages/auth/CLAUDE.md index 8717dd55..51d305f8 100644 --- a/packages/auth/CLAUDE.md +++ b/packages/auth/CLAUDE.md @@ -110,6 +110,13 @@ a data change. - A role that ends up with an empty perm set is a **degenerate role**: it still passes `isValidRole` but grants nothing. Check the drop-safety of a role before removing its last permission. +- **`BoardSeat` and `MemberLogin` are exact-code gates, not `canDo` subjects.** The live codes + (`update:BoardSeat`, `create:MemberLogin`) are each checked with `hasPerm` — in + `firestore.rules`, in beacon's `requireAdminOrPerm`, and in backstage's `useCan` — never a + `canDo`-style expansion, so a `manage:all` holder does **not** satisfy either one. The other + four generated codes per subject (`manage:BoardSeat`, `read:MemberLogin`, …) are consequently + inert: valid `PermissionCode`s, assignable in `/permisos`, satisfying nothing anywhere. See + `docs/specs/board-seat-delegation.md`. ## Consumers (why changes here are wide-blast) diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 732a293e..28f410e8 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -23,6 +23,7 @@ export { permissionCodeSchema, type RoleDefinitionInput, } from "./role-definition-schema.js"; +export { PROVISION_BLOCK_REASONS, type ProvisionBlockReason } from "./provision-block-reason.js"; export type { Member, MemberStatus } from "./member.js"; export { MEMBER_STATUSES } from "./member.js"; export { MEMBER_GENDERS, type MemberGender } from "./member.js"; diff --git a/packages/types/src/provision-block-reason.test.ts b/packages/types/src/provision-block-reason.test.ts new file mode 100644 index 00000000..a22031dc --- /dev/null +++ b/packages/types/src/provision-block-reason.test.ts @@ -0,0 +1,19 @@ +import { describe, it, expect } from "vitest"; +import { PROVISION_BLOCK_REASONS } from "./provision-block-reason.js"; + +describe("PROVISION_BLOCK_REASONS", () => { + it("carries no duplicate tag", () => { + // A duplicate is invisible to both consumers: the union collapses it, so backstage's + // message table stays exhaustive with one message missing and beacon still compiles. + expect(new Set(PROVISION_BLOCK_REASONS).size).toBe(PROVISION_BLOCK_REASONS.length); + }); + + it("keeps every tag a stable wire token", () => { + // These cross the callable boundary inside `details.reason` and are compared byte-for-byte + // by the client. Lowercase kebab with no spaces or punctuation keeps them safe to log, + // JSON round-trip and eyeball in an error payload. + for (const reason of PROVISION_BLOCK_REASONS) { + expect(reason).toMatch(/^[a-z]+(?:-[a-z]+)*$/); + } + }); +}); diff --git a/packages/types/src/provision-block-reason.ts b/packages/types/src/provision-block-reason.ts new file mode 100644 index 00000000..1e290495 --- /dev/null +++ b/packages/types/src/provision-block-reason.ts @@ -0,0 +1,33 @@ +/** Why `provisionMemberLogin` refused. Beacon carries the tag on the HttpsError's + * `details.reason`; backstage's `provisionErrorMessage` keys its operator-facing message + * table on it. + * + * Declared here because that is a cross-boundary contract with nothing else coupling its + * two ends: renaming a literal in beacon used to leave the client's table silently + * unmatched, degrading every refusal to the generic "no se pudo" fallback — which is the + * dead-end ("a transient error the operator retries forever") the table exists to remove. + * Both apps already depend on `@luminova/types`, so the union turns that rename into a + * compile error on the consumer instead. Same reasoning as the Codegen-drift gate in + * CLAUDE.md; a shared union is the cheap form of it. + * + * Beacon imports the TYPE only — the root barrel pulls zod, which the functions bundle has + * no other reason to carry. The runtime array exists so the client's message table can be + * proved exhaustive by iterating the contract rather than re-listing it. */ +export const PROVISION_BLOCK_REASONS = [ + // The member's stored uid resolves to a different live Auth account than their email does + // (an out-of-band email change). Relinking is a deliberate console op. + "linked-to-different-login", + // A non-Admin caller reached adoption or resend: an account already exists for this email, + // or the member is already linked. Both are Admin-only. + "reprovision-requires-admin", + // A non-Admin caller reached a member carrying direct grants (roleIds / permissionOverrides). + "granted-member-requires-admin", + // A non-Admin caller reached a member seated on a cargo that confers roles. + "power-seat-requires-admin", + // The member's stored email is not an address the Auth SDK will accept, so the call would + // fail as an opaque `internal` and that member would stay unprovisionable until someone + // edits the doc. `firestore.rules` does not shape-validate email on the admin write lane. + "member-email-malformed", +] as const; + +export type ProvisionBlockReason = (typeof PROVISION_BLOCK_REASONS)[number]; diff --git a/packages/ui/src/components/combobox.tsx b/packages/ui/src/components/combobox.tsx index 838e3c5d..a22380cf 100644 --- a/packages/ui/src/components/combobox.tsx +++ b/packages/ui/src/components/combobox.tsx @@ -16,6 +16,11 @@ interface ComboboxProps { emptyText?: string; disabled?: boolean; id?: string; + /** Ids of the elements explaining this control — a note about why the list is empty, or why + * the control is locked. Without it those notes sit later in the reading order, so a + * screen-reader user reaches the trigger, hears only `emptyText`, and never meets the + * explanation. */ + "aria-describedby"?: string; } /** Single-select + search on Radix Popover + cmdk, JCI-token styled. Re-select clears. */ @@ -28,6 +33,7 @@ export function Combobox({ emptyText = "Sin resultados", disabled, id, + "aria-describedby": describedBy, }: ComboboxProps) { const [open, setOpen] = useState(false); const selected = options.find((o) => o.value === value) ?? null; @@ -39,6 +45,7 @@ export function Combobox({ type="button" id={id} disabled={disabled} + aria-describedby={describedBy} aria-haspopup="listbox" aria-expanded={open} className={cn( diff --git a/packages/ui/src/components/multi-select-field.tsx b/packages/ui/src/components/multi-select-field.tsx index a1ee4984..1e5a5718 100644 --- a/packages/ui/src/components/multi-select-field.tsx +++ b/packages/ui/src/components/multi-select-field.tsx @@ -16,6 +16,11 @@ interface MultiSelectProps { emptyText?: string; disabled?: boolean; id?: string; + /** Id of the element explaining this control — a permission note, a "why is this disabled" + * sentence. Same reason Combobox takes it: those notes sit AFTER the field in the DOM, so a + * screen-reader user reaching a disabled trigger otherwise hears a dead control with no + * explanation and cannot tell a permission ceiling from a broken widget. */ + "aria-describedby"?: string; } /** Multi-select + search on Radix Popover + cmdk; selected render as removable chips. */ @@ -28,6 +33,7 @@ export function MultiSelect({ emptyText = "Sin resultados", disabled, id, + "aria-describedby": describedBy, }: MultiSelectProps) { const [open, setOpen] = useState(false); const chosen = selectedOptions(options, value); @@ -41,6 +47,7 @@ export function MultiSelect({ disabled={disabled} aria-haspopup="listbox" aria-expanded={open} + aria-describedby={describedBy} className={cn( fieldControlClasses, "flex h-auto min-h-[52px] flex-wrap items-center gap-1.5 px-3 py-[7px] text-left disabled:opacity-60", diff --git a/tests/firestore-rules/cargo-assignment-parity.test.ts b/tests/firestore-rules/cargo-assignment-parity.test.ts new file mode 100644 index 00000000..669b6d61 --- /dev/null +++ b/tests/firestore-rules/cargo-assignment-parity.test.ts @@ -0,0 +1,511 @@ +import { fileURLToPath } from "node:url"; +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { + assertFails, + assertSucceeds, + initializeTestEnvironment, + type RulesTestEnvironment, +} from "@firebase/rules-unit-testing"; +import { doc, setDoc, updateDoc, type Firestore } from "firebase/firestore"; +import { buildAbility, type Action, type Subject } from "@luminova/auth/ability"; +import { ROLES, type AuthClaims } from "@luminova/auth/roles"; +import { permsForRoles } from "../../tools/scripts/lib/role-seed.mjs"; +// The SAME modules the two member forms render from. `assignable-cargo-core` is import-free +// and `member-edit-gate` / `probe` pull only `@luminova/auth`, so all three load here — +// `assignable-cargo.ts` itself does NOT (it needs `@luminova/types` for value, which this +// package cannot resolve), which is exactly why the predicates were split out. Same shape, +// and same reason, as `nav-equivalence.test.ts` importing `nav-config`. +import { + cargoSlotsForEditor, + cargoTakedownOnly, + heldCargo, + positionsLockedForEditor, + type CargoLike, +} from "../../apps/backstage/src/features/members/lib/assignable-cargo-core"; +import { memberEditMode } from "../../apps/backstage/src/features/members/lib/member-edit-gate"; +import { abilityAllows } from "../../apps/backstage/src/lib/authz/probe"; +import { capabilityFlags } from "../../apps/backstage/src/lib/authz/capability-flags"; + +// The contract: for every (principal, member fixture, cargo) triple, IF the backstage cargo +// editor would let this editor submit this cargo for this member, THEN the real rules engine +// must allow the corresponding members write — on both lanes, `positionsAssignmentSafe()` on +// update and `createPositionsSafe()` on create, with `assignedBy` self-stamped exactly as +// `member-repository.ts`'s `setPositions` does it. +// +// One principal fixture drives both sides: the claims the emulator context receives are the +// claims the client gates are built from, so an option list that admits a write the rules +// reject fails HERE rather than as a PERMISSION_DENIED toast in production. That has already +// happened twice on this branch — `cargoOptionsForEditor` offering a CEL cargo the rules deny, +// and `positionsLockedForEditor`'s flag being widened until the editor unlocked for a write +// the rules ALWAYS deny — and `assignable-cargo.test.ts`, a hand-written truth table that never +// reads `firestore.rules`, stays green through both. +// +// This is an IMPLICATION, not equality. The client is deliberately stricter in places (comisión +// cargos are filtered out of the picker though the rules would take a grant-free one; a member +// editor may clear a seat the rules would also let them keep), and flagging that curation would +// make the test an obstacle to it. It verifies nothing about the rules' OTHER conjuncts — +// `memberWriteInvariants`, the self lane, the takedown arm — which remain `rules.test.ts`'s. +// +// Per `docs/engineering-guardrails.md` this is the "parity test, never a hand-maintained +// mirror list" half only. The parser half is deliberately NOT built: these are BOOLEAN +// PREDICATES over `get()` fan-out and `resource` vs `request.resource`, which no shared TS +// description expresses — the emulator IS the parser. + +const PROJECT_ID = "demo-cargo-parity"; +let env: RulesTestEnvironment; + +// Rules derive the term from request.time.year() (UTC); compute it from the client clock so +// this suite cannot rot when the calendar year rolls over. +const TERM = String(new Date().getUTCFullYear()); +const PRIOR_TERM = Number(TERM) - 1; +/** Whoever seated the fixture member before the test's principal touches it. */ +const SEEDER_UID = "seed-admin-uid"; + +interface Principal { + label: string; + uid: string; + roles: string[]; + perms: string[]; +} +const canonical = (role: string): Principal => ({ + label: role, + uid: `${role.toLowerCase()}-uid`, + roles: [role], + perms: permsForRoles([role]), +}); +const custom = (label: string, perms: string[]): Principal => ({ + label: `custom(${label})`, + uid: `${label}-uid`, + roles: [], + perms, +}); + +const PRINCIPALS: Principal[] = [ + // Derived from ROLES, never hand-listed: a new built-in role is held to the implication from + // the moment it exists, rather than when someone remembers to add it here. + ...ROLES.map((role) => canonical(role)), + // Perms-only customs — the principals owner-op 1 describes, and the ones the delegation + // feature is actually about. Each names the exact capability the lane keys on: + custom("update-Member", ["update:Member"]), // institutional lane, no delegation + custom("update-Position", ["update:Position"]), // members-positions lane, ceiling = grant-free JDL + custom("update-BoardSeat", ["update:BoardSeat"]), // NON-VACUITY: the code widens which cargo an + // editor may seat, it does not make anyone an editor — this principal must reach no lane at all. + custom("position+boardseat", ["update:Position", "update:BoardSeat"]), // the spec's recommended pairing + custom("member+boardseat", ["update:Member", "update:BoardSeat"]), + custom("create-Member", ["create:Member"]), + custom("create+boardseat", ["create:Member", "update:BoardSeat"]), + // Escalation probe: `manage:all` satisfies the rules' canDo() but is NOT the exact code + // boardSeatDelegate()/canAssignBoardSeat ask for, so it must never be offered a CEL seat. + custom("manage-all", ["manage:all"]), +]; + +/** Bridge the seed producer's plain string arrays (role-seed.mjs) to the typed claim. */ +const claimsOf = (p: Principal): AuthClaims => + ({ roles: p.roles, perms: p.perms }) as unknown as AuthClaims; + +function as(p: Principal): Firestore { + return env.authenticatedContext(p.uid, { roles: p.roles, perms: p.perms }).firestore(); +} + +interface Gates { + /** Which member editor the profile page mounts (features/members/lib/member-edit-gate). */ + editMode: "full" | "positions" | "none"; + /** Whether the invite drawer's "Invitar miembro" affordance exists (``). */ + canCreate: boolean; + /** `Can.isAdmin` — the OLD-side flag (`allowReplacePowerCargo`), never delegated. */ + isAdmin: boolean; + /** `Can.canAssignBoardSeat` — the NEW-side flag (`allowPowerGrants`), which update:BoardSeat lifts. */ + allowPowerGrants: boolean; +} + +/** The claims → form-props mapping the four call sites make (`member-profile-page.tsx`, + * `member-drawer.tsx`, `member-invite-drawer.tsx`): `allowPowerGrants={canAssignBoardSeat}`, + * `allowReplacePowerCargo={isAdmin}`. + * + * Both flags come from `capabilityFlags` — the SAME function `buildCan` spreads — not from a + * local `hasAnyRole(...) || hasPerm(...)` re-derivation. That copy was the mirror class this + * whole test exists to delete, applied to the very flag whose widening caused the #224 + * regression: it would have kept agreeing with itself while `use-can.ts` drifted. The flags + * were split out of `use-can.ts` (a React module this package cannot load) for exactly this. + * `member-profile-page.test.tsx` is what pins the two PROPS to those two flags. */ +function gatesFor(p: Principal): Gates { + const claims = claimsOf(p); + const ability = buildAbility(claims, p.uid); + const can = (action: Action, subject: Subject) => abilityAllows(ability, action, subject); + const flags = capabilityFlags(claims); + return { + editMode: memberEditMode({ can }), + canCreate: can("create", "Member"), + isAdmin: flags.isAdmin, + allowPowerGrants: flags.canAssignBoardSeat, + }; +} + +interface Cargo extends CargoLike { + title: string; + description: string; + deletedAt: null; +} +// `category` is a literal union HERE. `CargoLike.category` is `PositionCategory` — NOT the +// widened `string` this comment used to claim — so the compiler already rejects a `"cel"` typo +// through the interface; the local union is a second, independent statement of the same thing +// and costs nothing. What neither catches is a RENAME of the category itself in +// POSITION_CATEGORIES: the same fixture object feeds the client predicate and the emulator, so +// the rules' `category != 'CEL'` and the client's `category !== "CEL"` would agree with each +// other, and agree wrongly, on the publication boundary, with the suite green. +const cargo = ( + id: string, + category: "CEL" | "JDL" | "Comision", + grants: string[], + extra: Partial = {}, +): Cargo => ({ + id, + title: id, + description: "", + category, + grants, + term: category === "JDL" ? Number(TERM) : null, + active: true, + deletedAt: null, + ...extra, +}); + +/** The positions catalog, seeded into the emulator AND handed to `cargoSlotsForEditor` — one + * fixture, both sides, so the option list and the `get()` the rules perform can never describe + * different cargos. Spans every axis the two rules conjuncts turn on: grants vs none, CEL vs + * JDL (the publication boundary), plus the three the CLIENT filters on its own (comisión, + * deactivated, past term) so the implication is exercised where the client is stricter. */ +const CATALOG: Cargo[] = [ + cargo("jdl_free", "JDL", []), + cargo("jdl_free_alt", "JDL", []), + cargo("cel_free", "CEL", []), // grant-free CEL: the deliberate takedown asymmetry + cargo("cel_power", "CEL", ["Admin"]), // Presidente-shaped: publishes at board rank 0 AND mints + cargo("jdl_power", "JDL", ["Secretary"]), + cargo("comision_free", "Comision", []), + cargo("jdl_retired", "JDL", [], { term: PRIOR_TERM }), + cargo("jdl_inactive", "JDL", [], { active: false }), +]; +const cargoById = (id: string | null) => CATALOG.find((c) => c.id === id); + +interface MemberFixture { + key: string; + /** The cargo this member already holds in the CURRENT term — the OLD side of the swap. */ + cargoId: string | null; +} +/** The states `currentCargoGrantsEmpty()` (the cargo being REPLACED) turns on. */ +const MEMBER_FIXTURES: MemberFixture[] = [ + { key: "unseated", cargoId: null }, + { key: "seated-jdl-free", cargoId: "jdl_free" }, + { key: "seated-cel-free", cargoId: "cel_free" }, // keeping it is denied, CLEARING it is not + { key: "seated-jdl-power", cargoId: "jdl_power" }, + { key: "seated-cel-power", cargoId: "cel_power" }, + { key: "seated-retired", cargoId: "jdl_retired" }, // held cargo outside the option list +]; + +/** What the cargo Combobox would let this editor SUBMIT for this member: every enabled option, + * plus `null` — clearing the slot, which the forms always offer while the slot is unlocked and + * which `cargoTakedownOnly` makes an explicit "Quitar cargo" action. A locked slot submits + * nothing at all. */ +function offeredCargoIds(g: Gates, assignedCargoId: string | null): (string | null)[] { + const held = heldCargo(CATALOG, assignedCargoId); + if (positionsLockedForEditor(held, g.isAdmin)) return []; + const enabled = cargoSlotsForEditor({ + positions: CATALOG, + allowPowerGrants: g.allowPowerGrants, + assignedCargoId, + term: TERM, + }) + .filter((slot) => !slot.disabled) + .map((slot) => slot.position.id); + return [null, ...enabled]; +} + +type Lane = "update" | "create"; +interface Triple { + principal: Principal; + lane: Lane; + fixture: MemberFixture; + cargoId: string | null; +} + +const NEW_MEMBER: MemberFixture = { key: "new-member", cargoId: null }; + +/** Every triple the UI offers, materialized up front so the structural assertions below can + * interrogate the matrix itself — an implication suite is vacuously green when nothing is + * offered, so "what did we actually probe" has to be assertable. */ +const MATRIX: Triple[] = PRINCIPALS.flatMap((principal) => { + const g = gatesFor(principal); + const update: Triple[] = + g.editMode === "none" + ? [] + : MEMBER_FIXTURES.flatMap((fixture) => + offeredCargoIds(g, fixture.cargoId).map((cargoId) => ({ + principal, + lane: "update" as const, + fixture, + cargoId, + })), + ); + const create: Triple[] = g.canCreate + ? offeredCargoIds(g, null).map((cargoId) => ({ + principal, + lane: "create" as const, + fixture: NEW_MEMBER, + cargoId, + })) + : []; + return [...update, ...create]; +}); + +const MEMBER_DOC = { name: "Ana Lopez", totalPoints: 0, active: true, deletedAt: null }; + +/** The exact payload `MemberRepository.setPositions` writes: the current term only, by dot + * path, with `assignedBy` self-stamped. Accepted by BOTH update arms — the institutional + * `update:Member` one and the `update:Position` members-positions lane, which additionally + * requires `affectedKeys().hasOnly(['positions'])`. */ +function writeUpdate(db: Firestore, id: string, cargoId: string | null, uid: string) { + return updateDoc(doc(db, `members/${id}`), { + [`positions.${TERM}`]: { cargoId, comisionIds: [], assignedBy: uid }, + }); +} + +/** The create-lane twin: a member born on the cargo, self-stamped, in the current term only. */ +function writeCreate(db: Firestore, id: string, cargoId: string | null, uid: string) { + return setDoc(doc(db, `members/${id}`), { + ...MEMBER_DOC, + positions: { [TERM]: { cargoId, comisionIds: [], assignedBy: uid } }, + }); +} + +let docCounter = 0; +/** A fresh member doc per assertion. An ALLOWED write mutates the fixture it lands on, so a + * shared doc would make every later triple depend on the order the earlier ones ran in. */ +async function seedMember(fixture: MemberFixture): Promise { + const id = `parity_${fixture.key}_${docCounter++}`; + await env.withSecurityRulesDisabled(async (ctx) => { + await setDoc(doc(ctx.firestore(), `members/${id}`), { + ...MEMBER_DOC, + ...(fixture.cargoId === null + ? {} + : { + positions: { + [TERM]: { cargoId: fixture.cargoId, comisionIds: [], assignedBy: SEEDER_UID }, + }, + }), + }); + }); + return id; +} + +const describeTriple = (t: Triple) => + `${t.principal.label} ${t.lane}s ${t.fixture.key} → cargo ${t.cargoId ?? "null (clear)"}`; + +async function assertRulesAllow(t: Triple): Promise { + const id = t.lane === "update" ? await seedMember(t.fixture) : `parity_created_${docCounter++}`; + const db = as(t.principal); + const write = + t.lane === "update" + ? writeUpdate(db, id, t.cargoId, t.principal.uid) + : writeCreate(db, id, t.cargoId, t.principal.uid); + try { + await assertSucceeds(write); + } catch (error) { + throw new Error( + `the cargo editor OFFERS "${describeTriple(t)}" but firestore.rules DENY that write — ` + + `the client mirror in assignable-cargo-core.ts has drifted from ` + + `positionsAssignmentSafe()/createPositionsSafe(). Cause: ${String(error)}`, + ); + } +} + +beforeAll(async () => { + const rulesPath = resolve(fileURLToPath(new URL("../../firestore.rules", import.meta.url))); + env = await initializeTestEnvironment({ + projectId: PROJECT_ID, + firestore: { + host: "127.0.0.1", + port: Number(process.env.FIRESTORE_EMULATOR_PORT ?? 4010), + rules: readFileSync(rulesPath, "utf8"), + }, + }); + await env.clearFirestore(); + await env.withSecurityRulesDisabled(async (ctx) => { + const db = ctx.firestore(); + // cargoAssignableByNonAdmin()/currentCargoGrantsEmpty() get() these docs; a missing one + // errors the rule (fail-closed), so the catalog must exist before any triple runs. + for (const c of CATALOG) await setDoc(doc(db, `positions/${c.id}`), c); + }); +}); + +afterAll(async () => { + await env.cleanup(); +}); + +describe("cargo assignment ⟷ rules: every OFFERED cargo is a write the emulator allows", () => { + // The implication is vacuously true wherever nothing is offered, so pin what the matrix + // actually probes. Each of these would have caught a predicate that regressed to offering + // nothing — which passes the implication while breaking the feature. + it("probes every member fixture, both lanes, and a cargo that is not just 'clear'", () => { + for (const fixture of MEMBER_FIXTURES) { + expect( + MATRIX.some((t) => t.fixture.key === fixture.key && t.cargoId !== null), + `no principal is offered any cargo for the ${fixture.key} fixture — it never probes`, + ).toBe(true); + } + for (const lane of ["update", "create"] as const) { + expect(MATRIX.some((t) => t.lane === lane && t.cargoId !== null)).toBe(true); + } + }); + + // The OLD-side conjunct is the Admin ROLE and nothing else — not the delegate, not manage:all + // (which is reachable as a perm without the role). Asserted over the matrix rather than left + // implicit in the absence of generated cases, so a widening shows up as a failure here and not + // as an extra green test nobody reads. + it("a power-cargo seat is submittable by the Admin role and by no one else", () => { + for (const key of ["seated-jdl-power", "seated-cel-power"]) { + const offered = MATRIX.filter((t) => t.fixture.key === key).map((t) => t.principal.label); + expect([...new Set(offered)]).toEqual(["Admin"]); + } + }); + + it("the delegation is live: only a board-seat delegate is offered CEL and power cargos", () => { + const celOffered = (label: string) => + MATRIX.some( + (t) => + t.principal.label === label && (t.cargoId === "cel_free" || t.cargoId === "cel_power"), + ); + expect(celOffered("custom(position+boardseat)")).toBe(true); + expect(celOffered("custom(create+boardseat)")).toBe(true); + expect(celOffered("Admin")).toBe(true); + // The ceiling the lane keeps for everyone else — including the manage:all escalation probe, + // which satisfies canDo() but not the exact code boardSeatDelegate() asks for. + expect(celOffered("custom(update-Position)")).toBe(false); + expect(celOffered("custom(update-Member)")).toBe(false); + expect(celOffered("custom(manage-all)")).toBe(false); + expect(celOffered("Membership")).toBe(false); + }); + + it("update:BoardSeat alone reaches no lane: the code widens a ceiling, it grants no entry", () => { + expect(MATRIX.some((t) => t.principal.label === "custom(update-BoardSeat)")).toBe(false); + }); + + for (const principal of PRINCIPALS) { + for (const lane of ["update", "create"] as const) { + for (const fixture of lane === "update" ? MEMBER_FIXTURES : [NEW_MEMBER]) { + const triples = MATRIX.filter( + (t) => t.principal === principal && t.lane === lane && t.fixture.key === fixture.key, + ); + if (triples.length === 0) continue; + const offered = triples.map((t) => t.cargoId ?? "null").join(", "); + it(`${principal.label} ${lane}s ${fixture.key}: offers [${offered}] ⟹ rules allow each`, async () => { + for (const triple of triples) await assertRulesAllow(triple); + }); + } + } + } + + // The takedown asymmetry, asserted as its own row because it is the one state where the + // client is stricter on one side and NOT the other: a non-delegate may not keep a grant-free + // CEL seat but must be able to clear it, so `null` has to be in the offered set (denying it + // would strand the takedown behind an Admin, which firestore.rules explicitly refuses to do). + it("a grant-free CEL seat is takedown-only for a non-delegate, and the takedown is offered", () => { + const g = gatesFor(PRINCIPALS.find((p) => p.label === "custom(update-Position)")!); + expect(cargoTakedownOnly(cargoById("cel_free"), g.allowPowerGrants)).toBe(true); + const offers = offeredCargoIds(g, "cel_free"); + expect(offers).toContain(null); + expect(offers).not.toContain("cel_free"); + }); + + // The counterfactual for drift #2, driven through the emulator rather than asserted about the + // client alone: feeding the NEW-side flag (`allowPowerGrants`, which update:BoardSeat lifts) + // into `positionsLockedForEditor` — whose conjunct, `currentCargoGrantsEmpty()`, is Admin-ROLE + // only — unlocks the slot for a delegate, and every write it would then submit is denied. This + // is what "the two flags are not the same one" costs when it is got wrong. + // THE OTHER DIRECTION. Everything above is `client offers ⟹ rules allow`, which catches a + // rules TIGHTENING and structurally cannot catch a rules LOOSENING: delete + // `&& cargo.category != 'CEL'` from nonAdminAssignable(), or widen boardSeatDelegate() from + // hasPerm('update:BoardSeat') to canDo('update','BoardSeat') so `manage:all` satisfies it, + // and every triple above stays green — the client would merely be stricter than the rules, + // which this file treats as legal curation, and legal curation is most of why the converse + // cannot be asserted wholesale (comisión cargos, retired seats, inactive ones). + // + // It CAN be asserted for the three cargos the delegation is about, and there the gap is not + // curation but the boundary itself. Driven through the emulator, so it is the ruleset that + // answers and not another read of the same client predicate. + // Each row proves its own lane reachability before asserting the denial. `assertFails` is + // REASON-BLIND: a later change that denies these writes for an unrelated reason — a tightened + // softDeleteSafe(), a lost perm, a renamed fixture — would keep every row green while the + // CEL/power boundary silently stopped being what denies them, which is the exact "green for + // the wrong reason" failure this file was written to end. The paired grant-free ALLOW is what + // makes the denial attributable: same principal, same lane, same fixture shape, one cargo + // apart, so the conjunct under test is the only difference between them. + // + // Both lanes, not just update: createPositionsSafe() applies the same + // `(boardSeatDelegate() || cargoAssignableByNonAdmin())` conjunct, and a principal that can + // create but not update (create-Member) reaches the boundary only there. + const WITHHELD = ["cel_free", "cel_power", "jdl_power"] as const; + const CONVERSE_PRINCIPALS = [ + "custom(update-Position)", + "custom(update-Member)", + "custom(create-Member)", + "custom(manage-all)", + ]; + // Vacuity guard, same class as the matrix pin above. `CONVERSE_PRINCIPALS` holds strings + // built by `custom()`, so a rename there would silently emit no `it` for that principal — and + // the create-Member row is the one this block exists for. A missing label must be a failure, + // never a skip. + it("every converse principal actually exists", () => { + const known = PRINCIPALS.map((p) => p.label); + for (const label of CONVERSE_PRINCIPALS) expect(known).toContain(label); + }); + + for (const label of CONVERSE_PRINCIPALS) { + const principal = PRINCIPALS.find((p) => p.label === label); + if (principal === undefined) continue; + const g = gatesFor(principal); + // Stated rather than derived from MATRIX on purpose: a MATRIX-derived lane list would + // silently DROP a lane that happens to offer nothing, which is exactly the state this block + // needs to probe. The cost is a second copy of the derivation, so the row count is asserted + // below — a principal that reaches no lane at all must fail, not vanish. + const lanes: Lane[] = [ + ...(g.editMode === "none" ? [] : (["update"] as const)), + ...(g.canCreate ? (["create"] as const) : []), + ]; + it(`${label} reaches at least one lane`, () => { + expect(lanes.length).toBeGreaterThan(0); + }); + for (const lane of lanes) { + const write = (id: string, cargoId: string | null) => + lane === "update" + ? writeUpdate(as(principal), id, cargoId, principal.uid) + : writeCreate(as(principal), id, cargoId, principal.uid); + const target = async () => + lane === "update" + ? await seedMember({ key: "unseated", cargoId: null }) + : `parity_converse_${docCounter++}`; + + it(`BLOCKING: the rules DENY ${label} on ${lane} every seat the client withholds`, async () => { + // Reachability, asserted rather than assumed: this principal really can write a seat on + // this lane, so the refusals below isolate the cargo conjuncts and nothing else. + await assertSucceeds(write(await target(), "jdl_free")); + for (const cargoId of WITHHELD) { + expect(offeredCargoIds(g, null)).not.toContain(cargoId); + await assertFails(write(await target(), cargoId)); + } + }); + } + } + + it("wiring allowPowerGrants into the OLD-side flag would offer writes the rules deny", async () => { + const delegate = PRINCIPALS.find((p) => p.label === "custom(position+boardseat)")!; + const g = gatesFor(delegate); + const held = heldCargo(CATALOG, "jdl_power"); + expect(positionsLockedForEditor(held, g.isAdmin)).toBe(true); + expect(positionsLockedForEditor(held, g.allowPowerGrants)).toBe(false); + const id = await seedMember({ key: "seated-jdl-power", cargoId: "jdl_power" }); + await assertFails(writeUpdate(as(delegate), id, "jdl_free", delegate.uid)); + await assertFails(writeUpdate(as(delegate), id, null, delegate.uid)); + }); +}); diff --git a/tests/firestore-rules/rules.test.ts b/tests/firestore-rules/rules.test.ts index 031564f0..7598706e 100644 --- a/tests/firestore-rules/rules.test.ts +++ b/tests/firestore-rules/rules.test.ts @@ -1169,6 +1169,16 @@ describe("firestore.rules — members", () => { // plus a next-term power cargo attributed to a real Admin. On the UTC-year rollover // claims-sync reads THAT entry and mints Admin onto a member whose login the creator // controls. Forged attribution, one term deferred. + // + // NOT a strict subset of the Admin variant above — the two principals differ in a way + // that matters. admin-uid's synthesized perms are ['manage:all'] (tools/scripts/lib/ + // role-seed.mjs), never the literal string 'update:BoardSeat', so hasPerm('update:BoardSeat') + // is false for it; it only satisfies boardSeatDelegate() via the separate hasAnyRole(['Admin']) + // arm. createDelegate() genuinely holds ['create:Member','update:BoardSeat'], so + // hasPerm('update:BoardSeat') is true for it. A mutation that grafts a hasPerm disjunct onto + // the term-key conjunct itself — `hasPerm('update:BoardSeat') || keys().hasOnly([currentTermKey()])` + // in createPositionsSafe() — leaves the Admin test denied (hasPerm still false there) while + // reopening the ride-along for this delegate. Only this test catches that mutation. await assertFails( setDoc(doc(createDelegate(), "members/new_delegate_ridealong"), { name: "Ximena Paz", @@ -1182,7 +1192,7 @@ describe("firestore.rules — members", () => { ); }); - it("BLOCKING: create:Member alone, and update:BoardSeat alone, each reach nothing", async () => { + it("BLOCKING: update:BoardSeat alone reaches nothing on the create lane", async () => { // The create-lane twin of the update-lane non-vacuity pin. update:BoardSeat widens WHICH // cargo a creator may use; it does not make anyone a creator. await assertFails( @@ -1194,7 +1204,7 @@ describe("firestore.rules — members", () => { ); }); - it("BLOCKING: the same create principal WITHOUT update:BoardSeat is still denied both", async () => { + it("BLOCKING: create:Member alone (no update:BoardSeat, a different principal) is still denied a CEL or power cargo", async () => { // The paired denial — otherwise the two ALLOWs above would pass for any create:Member // holder and prove nothing about the new disjunct. await assertFails( @@ -1255,6 +1265,20 @@ describe("firestore.rules — members", () => { updateDoc(doc(as("u", ["Membership"]), "members/m1"), { name: "Ana Rivas Paz" }), ); }); + // ACCEPTED EXPOSURE — pins the premise three beacon guards are built on + // (provisionMemberLogin's adoption guard and power-seat guard, in + // apps/beacon/src/provision-member-login.ts, plus their tests and the rules comments + // near currentCargoGrantsEmpty()): "firestore.rules never constrains members.email". + // memberWriteInvariants() locks totalPoints/uid/publicProfile and gates positions — + // email is not among them, so any canDo('update','Member') holder may rewrite ANY + // member's email, not just their own. "u" does not own members/m1 (owner-uid does). + // If a later PR pins email here, this test goes red first — the signal that the beacon + // guards it justifies are now over-strict, not that something silently broke. + it("ACCEPTED EXPOSURE: a manage:Member holder may rewrite another member's email (why beacon's adoption + power-seat guards exist)", async () => { + await assertSucceeds( + updateDoc(doc(as("u", ["Membership"]), "members/m1"), { email: "rewritten@example.com" }), + ); + }); // memberNameValid() binds EVERY lane, not just the member's own: boardShowcase publishes // the name world-read, so the bound belongs at the trust boundary. Leaving it to the admin // form's client-side zod would make it bypassable by any direct authenticated write. @@ -3271,7 +3295,7 @@ describe("firestore.rules — member positions assignment", () => { ); }); - it("denies a delegate a forged assignedBy, a past term, and a ride-along field", async () => { + it("denies a delegate a forged assignedBy, a non-current term, and a ride-along field", async () => { // assignedBySelf(), the current-term restriction and hasOnly(['positions']) all sit // OUTSIDE the substituted disjunction. Pin that the delegation did not loosen them — // a forged assignedBy is what the beacon trust gate reads to decide whether to mint.