- {/* The dialog opens as soon as the link arrives, before the reset mail settles, so
- its copy has to track that outcome. A fixed "ya le enviamos el correo" reads as a
- flat contradiction of the failure alert behind it — and worse, the modal's
- aria-hidden takes that alert out of the accessibility tree, so a screen-reader
- user would hear ONLY the false sentence. For the same reason the mail FAILURE is
- repeated inside the dialog rather than left to the header alert. */}
+ {/* 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. */}
- {sent
- ? "Ya le enviamos el correo para crear su contraseña. Si no le llega, comparte este enlace con el miembro."
- : "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.
- {error && (
-
- {error}
-
- )}
{link}
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/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 }),
});
}
From c00fefe5eeefd27a105f50d238528cc393b9c4fc Mon Sep 17 00:00:00 2001
From: Arnold Gandarillas Castillo
Date: Fri, 28 Aug 2026 16:21:56 -0400
Subject: [PATCH 18/25] fix(backstage,ui): fail closed on a cargo the editor
cannot resolve
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
positionsLockedForEditor took `Position | undefined` and asked only whether it
confers power, so a held cargo whose id resolves to NOTHING read as "no cargo" and
unlocked the picker. The rules have no such gap: currentCargoGrantsEmpty() get()s the
real doc and a missing one errors the rule, which denies. So the client offered a full
picker and an enabled Guardar for a write firestore.rules always rejects — the
render-then-die shape this module exists to prevent, one input short.
Reachable without a console edit. The catalog is parseDocs(positionDocSchema, …),
which DROPS any doc failing the schema, so the very corruption that makes a cargo's
power unknowable is what removes it from the array the client searches.
`heldCargo(positions, cargoId)` is now the one way to build that input — it keeps the
id alongside the lookup, so "seated on something unreadable" and "not seated" stop
being the same value. It also replaces the `positions.find(...)` each form had typed
for itself. `cargoSlotsForEditor` and `draftProvisionBlocked` lose their truthiness
tests on the id for the same reason: "" is an id that resolves to nothing, not "no
cargo", and beacon's readCargoIds manufactures exactly that value so its own guard
refuses it. The provision-gate test enshrined that divergence as deliberate; the
reasoning it gave ("the draft schema cannot produce one") makes the case unreachable,
not the fail-open answer correct.
MemberForm's four authority props are REQUIRED now. They were optional with `= false`
defaults, which are not safe in the same direction: `isSelfAssignment = false`
suppresses the mint-pending warning, `allowReplacePowerCargo = false` locks an Admin's
picker, and a call site that forgot either compiled clean.
The BLOCKING test named for the #224 flag conflation could not fail on flag
conflation — one assertion duplicated an earlier test verbatim and the other was
decided by the cargo alone. It now pins that the two flags DISAGREE for the same
principal, which is the property collapsing them destroys. cargoTakedownOnly gets the
truth table it never had.
ui: MultiSelect takes aria-describedby, like Combobox. Both forms disable the
comisiones picker on the same `locked` flag, and only the cargo picker was explaining
itself — a screen-reader user met a dead control with no way to tell a permission
ceiling from a broken widget.
Mutation-tested: dropping the unresolvable clause turns exactly the two new BLOCKING
rows red.
Co-Authored-By: Claude Opus 5 (1M context)
---
.../members/components/member-form.test.tsx | 97 ++++++++++--
.../members/components/member-form.tsx | 35 +++--
.../components/member-positions-form.tsx | 14 +-
.../no-assignable-cargos-note.test.tsx | 4 +
.../members/lib/assignable-cargo-core.ts | 53 ++++++-
.../members/lib/assignable-cargo.test.ts | 142 +++++++++++++-----
.../members/lib/provision-gate.test.ts | 17 ++-
.../features/members/lib/provision-gate.ts | 7 +-
.../ui/src/components/multi-select-field.tsx | 7 +
9 files changed, 303 insertions(+), 73 deletions(-)
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 78b8adfc..9163e9b2 100644
--- a/apps/backstage/src/features/members/components/member-form.test.tsx
+++ b/apps/backstage/src/features/members/components/member-form.test.tsx
@@ -24,6 +24,16 @@ const BOARD_SEAT_LABEL = permissionLabel("update:BoardSeat");
// 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[] = [
{
id: "pos-pres",
@@ -91,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();
@@ -99,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");
@@ -115,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"));
@@ -149,6 +169,7 @@ describe("MemberForm", () => {
];
const { unmount } = render(
{
// 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"));
@@ -176,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" }));
@@ -257,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");
@@ -276,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
,
);
@@ -293,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();
@@ -314,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();
@@ -342,6 +405,7 @@ describe("MemberForm", () => {
it("BLOCKING: does NOT lock a grant-free CEL seat — clearing it is the allowed takedown", () => {
render(
{
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(
{
it("BLOCKING: locks for a board-seat DELEGATE on a member seated on a power-granting cargo", () => {
render(
{
it("does NOT lock an Admin on that same power-granting seat", () => {
render(
{
it("BLOCKING: warns a delegate seating THEMSELVES on a non-Admin power cargo", async () => {
render(
{
// another member, so a note here would be false and would train users past the real one.
render(
{
// refusal for them. Without this cell the fix could be "warn on any self-assignment".
render(
{
it("defaults both new props to false rather than warning by accident", async () => {
render(
{
it("BLOCKING: associates the takedown note with the trigger", () => {
render(
{
it("renders comisión option as 'sigla — title' when sigla is present", async () => {
render(
;
@@ -48,20 +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;
+ 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;
+ 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;
+ isSelfAssignment: boolean;
children?: ReactNode;
}
@@ -91,10 +101,10 @@ export function MemberForm({
onSubmit,
showPreview,
avatarSeed,
- allowPowerGrants = false,
- allowReplacePowerCargo = false,
- assignerIsAdmin = false,
- isSelfAssignment = false,
+ allowPowerGrants,
+ allowReplacePowerCargo,
+ assignerIsAdmin,
+ isSelfAssignment,
children,
}: MemberFormProps) {
const [formError, setFormError] = useState(null);
@@ -140,8 +150,8 @@ export function MemberForm({
// 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 positionsLockedForEditor() / cargoTakedownOnly().
- const assignedCargo = positions.find((p) => p.id === assignedCargoId);
- const positionsLocked = positionsLockedForEditor(assignedCargo, allowReplacePowerCargo);
+ const held = heldCargo(positions, assignedCargoId);
+ const positionsLocked = positionsLockedForEditor(held, allowReplacePowerCargo);
const cargoTakedown = cargoTakedownOnly(selectedCargo, allowPowerGrants);
const cargoOptions = cargoOptionsForEditor({
positions,
@@ -324,6 +334,9 @@ export function MemberForm({
value={field.value}
onChange={field.onChange}
disabled={positionsLocked}
+ // Same flag disables it as the cargo picker, so it owes the same
+ // explanation — see the note in member-positions-form.
+ aria-describedby={positionsLocked ? NOTE_IDS.locked : undefined}
/>
)}
/>
diff --git a/apps/backstage/src/features/members/components/member-positions-form.tsx b/apps/backstage/src/features/members/components/member-positions-form.tsx
index 3ff393ea..26e272b2 100644
--- a/apps/backstage/src/features/members/components/member-positions-form.tsx
+++ b/apps/backstage/src/features/members/components/member-positions-form.tsx
@@ -12,7 +12,11 @@ import {
} from "../lib/assignable-cargo";
// Directly from the rules-mirroring module, not through assignable-cargo.ts: the file a
// predicate comes from is what says the emulator parity test holds it to firestore.rules.
-import { cargoTakedownOnly, positionsLockedForEditor } from "../lib/assignable-cargo-core";
+import {
+ cargoTakedownOnly,
+ heldCargo,
+ positionsLockedForEditor,
+} from "../lib/assignable-cargo-core";
import { cargoNoteIds, MintPendingNote, NoAssignableCargosNote } from "./no-assignable-cargos-note";
const NOTE_IDS = cargoNoteIds("positions");
@@ -71,8 +75,8 @@ export function MemberPositionsForm({
// denied, CLEARING it is allowed on purpose — so the form stays open, the seat renders as a
// disabled option (the trigger must not claim "Sin cargo" for a seated member) and only the
// takedown can be saved. See positionsLockedForEditor() / cargoTakedownOnly().
- const assignedCargo = positions.find((p) => p.id === defaultValues.cargoId);
- const locked = positionsLockedForEditor(assignedCargo, allowReplacePowerCargo);
+ const held = heldCargo(positions, defaultValues.cargoId);
+ const locked = positionsLockedForEditor(held, allowReplacePowerCargo);
const cargoOptions = cargoOptionsForEditor({
positions,
gender,
@@ -150,6 +154,10 @@ export function MemberPositionsForm({
value={field.value}
onChange={field.onChange}
disabled={locked}
+ // Disabled by the same flag as the cargo picker, so it owes the same
+ // explanation: a11y-wise a dead control with no reason is indistinguishable
+ // from a broken one. `locked` is the only state that disables it.
+ aria-describedby={locked ? NOTE_IDS.locked : undefined}
/>
)}
/>
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
index 56ed41ca..d5d89a39 100644
--- 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
@@ -49,6 +49,10 @@ function renderMemberForm(cargoId: string | null) {
positions={[POWER_CARGO]}
defaultValues={{ cargoId }}
submitLabel="Guardar"
+ allowPowerGrants={false}
+ allowReplacePowerCargo={false}
+ assignerIsAdmin={false}
+ isSelfAssignment={false}
onSubmit={vi.fn()}
/>,
).container;
diff --git a/apps/backstage/src/features/members/lib/assignable-cargo-core.ts b/apps/backstage/src/features/members/lib/assignable-cargo-core.ts
index e91ed6c0..4f984479 100644
--- a/apps/backstage/src/features/members/lib/assignable-cargo-core.ts
+++ b/apps/backstage/src/features/members/lib/assignable-cargo-core.ts
@@ -82,6 +82,46 @@ export function cargoConfersPower(cargo: Pick | undefined):
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. */
+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
@@ -90,6 +130,9 @@ export function cargoConfersPower(cargo: Pick | undefined):
* 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
@@ -112,10 +155,11 @@ export function cargoConfersPower(cargo: Pick | undefined):
* delegate silently unlocked the editor for a write the rules always deny.
*/
export function positionsLockedForEditor(
- cargo: Pick | undefined,
+ held: HeldCargo>,
allowReplacePowerCargo: boolean,
): boolean {
- return !allowReplacePowerCargo && cargoConfersPower(cargo);
+ if (allowReplacePowerCargo) return false;
+ return cargoConfersPower(held.cargo) || heldCargoUnresolvable(held);
}
/**
@@ -181,7 +225,10 @@ export function cargoSlotsForEditor({
.filter((p) => allowPowerGrants || cargoAssignableByNonAdmin(p))
.map((p) => ({ position: p, retired: false, disabled: false }));
- const held = assignedCargoId ? positions.find((p) => p.id === assignedCargoId) : undefined;
+ // 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,
diff --git a/apps/backstage/src/features/members/lib/assignable-cargo.test.ts b/apps/backstage/src/features/members/lib/assignable-cargo.test.ts
index 77099acd..66d34df6 100644
--- a/apps/backstage/src/features/members/lib/assignable-cargo.test.ts
+++ b/apps/backstage/src/features/members/lib/assignable-cargo.test.ts
@@ -21,6 +21,20 @@ 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`.
@@ -28,35 +42,94 @@ const JDL_FREE = cargo("JDL");
// 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(POWER, false)).toBe(true);
+ expect(positionsLockedForEditor(seated(POWER), false)).toBe(true);
});
it("does not lock a power-granting cargo for an Admin", () => {
- expect(positionsLockedForEditor(POWER, true)).toBe(false);
+ 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(CEL_FREE, flag)).toBe(false);
- expect(positionsLockedForEditor(JDL_FREE, flag)).toBe(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(undefined, false)).toBe(false);
- expect(positionsLockedForEditor(undefined, true)).toBe(false);
+ 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);
});
- it("BLOCKING: the flag is honored independently of the NEW-side one", () => {
- // The regression in one line. A board-seat delegate carries allowPowerGrants=true (the
- // NEW side, which update:BoardSeat lifts) while allowReplacePowerCargo stays false (the
- // OLD side, Admin-only). Folding the two into one flag unlocked a write the rules always
- // deny; this asserts the OLD side is the ONLY input here.
- expect(positionsLockedForEditor(POWER, false)).toBe(true);
- expect(cargoTakedownOnly(POWER, 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);
});
});
@@ -92,30 +165,27 @@ describe("noAssignableCargos", () => {
).toBe(false);
});
- // The `!locked` clause is DEFENSIVE, not reachable through either form today, and this pins
- // the invariant that makes it so — rather than deleting a clause whose redundancy depends on
- // a coincidence between two other functions.
+ // The `!locked` clause carries the two locked states differently, and BOTH are asserted
+ // below because only one of them is redundant.
//
- // Both forms derive `locked` and `cargoOptions` from the SAME (positions, assignedCargoId)
- // pair. locked === true therefore implies the id resolved (positionsLockedForEditor returns
- // false for an unresolved cargo) and carries grants, so cargoOptionsForEditor appends it as a
- // disabled option and the length clause alone already returns false. Break either half — give
- // the forms independent inputs, or stop appending the held cargo — and `!locked` becomes the
- // only thing keeping the locked note and the empty-catalog note from rendering together.
- it("BLOCKING: locked implies a non-empty option list, which is why !locked is defensive", () => {
- const held: Position = {
- id: "pos-power",
- title: "Secretario",
- titleFemale: null,
- category: "CEL",
- grants: ["Secretary"],
- term: null,
- sigla: null,
- description: "",
- active: true,
- deletedAt: null,
- };
- const locked = positionsLockedForEditor(held, false);
+ // 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],
diff --git a/apps/backstage/src/features/members/lib/provision-gate.test.ts b/apps/backstage/src/features/members/lib/provision-gate.test.ts
index 564d2f08..34b84f45 100644
--- a/apps/backstage/src/features/members/lib/provision-gate.test.ts
+++ b/apps/backstage/src/features/members/lib/provision-gate.test.ts
@@ -180,14 +180,15 @@ describe("draftProvisionBlocked", () => {
expect(draftProvisionBlocked(undefined, catalog, false)).toBe(false);
});
- // Deliberately NOT the member variant's answer, and pinned so the difference is a decision
- // rather than a leftover. `memberProvisionBlocked` reads a STORED doc, where a malformed ""
- // is reachable and must fail closed; this reads the draft the invite drawer is about to
- // create, whose cargoId comes from `z.string().min(1).nullable()` — "" cannot be produced,
- // and the create lane forbids a non-Admin the uid/roleIds/overrides halves anyway. If the
- // draft schema ever stops guaranteeing that, this line is the one that has to move.
- it("reads an empty-string draft cargoId as no cargo — the schema cannot produce one", () => {
- expect(draftProvisionBlocked("", 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", () => {
diff --git a/apps/backstage/src/features/members/lib/provision-gate.ts b/apps/backstage/src/features/members/lib/provision-gate.ts
index 5f606ea0..ccacd6d4 100644
--- a/apps/backstage/src/features/members/lib/provision-gate.ts
+++ b/apps/backstage/src/features/members/lib/provision-gate.ts
@@ -68,7 +68,7 @@ export function memberProvisionBlocked(
// `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,
+ (member.roleIds?.length ?? 0) > 0 || (member.permissionOverrides?.grant?.length ?? 0) > 0,
seatedCargos: cargoIds.map(cargo),
});
}
@@ -85,6 +85,9 @@ export function draftProvisionBlocked(
return provisionBlockedForNonAdmin({
hasLogin: false,
hasDirectGrants: false,
- seatedCargos: cargoId ? [cargo(cargoId)] : [],
+ // 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/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",
From 499253fde2f384d031e3f6a73c591cb9930deed5 Mon Sep 17 00:00:00 2001
From: Arnold Gandarillas Castillo
Date: Fri, 28 Aug 2026 16:22:11 -0400
Subject: [PATCH 19/25] test(rules): make the parity test catch a rules
LOOSENING, and stop mirroring buildCan
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Two defects in this branch's headline deliverable.
It asserted `client offers => rules allow` and nothing else, which catches a rules
TIGHTENING and structurally cannot catch a LOOSENING. Delete `&& category != 'CEL'`
from cargoAssignableByNonAdmin(), or widen boardSeatDelegate() from
hasPerm('update:BoardSeat') to canDo('update','BoardSeat') so manage:all satisfies it,
and all 429 lines stayed green — including the row named "the delegation is live",
which reads as a rules property and asserts only about the client. The converse cannot
be asserted wholesale (the client is deliberately stricter about comisiones, retired
and inactive cargos, and flagging that curation would make the test an obstacle to
it), but it can be for the three cargos the delegation is ABOUT, where the gap is not
curation but the boundary. Both loosenings now turn those rows red — verified by
making each mutation against firestore.rules and re-running the suite.
And `gatesFor` hand-re-implemented buildCan's claims -> flags mapping: the exact
mirror class this file exists to delete, of the exact flag whose widening caused the
#224 regression. It would have kept agreeing with itself while use-can.ts drifted. The
derivation is now `capabilityFlags()`, split out of use-can.ts (React, so this package
cannot load it) and spread back into buildCan — one function, both sides.
Also corrects a comment that justified the local category union by claiming CargoLike
widens it to `string`. It does not; assignable-cargo-core declares PositionCategory
and documents at length why.
Co-Authored-By: Claude Opus 5 (1M context)
---
.../src/lib/authz/capability-flags.ts | 66 +++++++++++++++++++
apps/backstage/src/lib/authz/use-can.ts | 64 ++----------------
.../cargo-assignment-parity.test.ts | 65 +++++++++++++-----
3 files changed, 122 insertions(+), 73 deletions(-)
create mode 100644 apps/backstage/src/lib/authz/capability-flags.ts
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/tests/firestore-rules/cargo-assignment-parity.test.ts b/tests/firestore-rules/cargo-assignment-parity.test.ts
index 65f63527..e0f19723 100644
--- a/tests/firestore-rules/cargo-assignment-parity.test.ts
+++ b/tests/firestore-rules/cargo-assignment-parity.test.ts
@@ -10,7 +10,7 @@ import {
} 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 { hasAnyRole, hasPerm, ROLES, type AuthClaims } from "@luminova/auth/roles";
+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 —
@@ -20,11 +20,13 @@ import { permsForRoles } from "../../tools/scripts/lib/role-seed.mjs";
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
@@ -120,20 +122,24 @@ interface Gates {
/** 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}`. `adminOrPerm` is re-derived from the same `hasAnyRole` /
- * `hasPerm` primitives `buildCan` uses rather than imported, because `use-can.ts` is a React
- * module this package cannot load; `member-profile-page.test.tsx` is what pins the props to
- * those two flags. `hasPerm`, never the ability, is the point — `manage:all` must not answer a
- * gate the rules key on an exact code. */
+ * `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: hasAnyRole(claims, ["Admin"]),
- allowPowerGrants: hasAnyRole(claims, ["Admin"]) || hasPerm(claims, "update:BoardSeat"),
+ isAdmin: flags.isAdmin,
+ allowPowerGrants: flags.canAssignBoardSeat,
};
}
@@ -142,12 +148,13 @@ interface Cargo extends CargoLike {
description: string;
deletedAt: null;
}
-// `category` is a literal union HERE even though CargoLike widens it to `string`. This is the
-// one error the parity test structurally cannot catch: the same fixture object is both fed to
-// the client predicate and seeded into the emulator, so a `"cel"` typo would make the rules'
-// `category != 'CEL'` and the client's `category !== "CEL"` agree with each other — and agree
-// wrongly, on the publication boundary, with the suite green. The compiler is the only guard
-// available for it, so give it one.
+// `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",
@@ -202,7 +209,7 @@ const MEMBER_FIXTURES: MemberFixture[] = [
* 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 = cargoById(assignedCargoId);
+ const held = heldCargo(CATALOG, assignedCargoId);
if (positionsLockedForEditor(held, g.isAdmin)) return [];
const enabled = cargoSlotsForEditor({
positions: CATALOG,
@@ -416,10 +423,36 @@ describe("cargo assignment ⟷ rules: every OFFERED cargo is a write the emulato
// 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.
+ const WITHHELD = ["cel_free", "cel_power", "jdl_power"] as const;
+ for (const label of ["custom(update-Position)", "custom(update-Member)", "custom(manage-all)"]) {
+ const principal = PRINCIPALS.find((p) => p.label === label);
+ if (principal === undefined) continue;
+ const g = gatesFor(principal);
+ if (g.editMode === "none") continue;
+ for (const cargoId of WITHHELD) {
+ it(`BLOCKING: the rules DENY ${label} the ${cargoId} seat the client withholds`, async () => {
+ expect(offeredCargoIds(g, null)).not.toContain(cargoId);
+ const id = await seedMember({ key: "unseated", cargoId: null });
+ await assertFails(writeUpdate(as(principal), id, cargoId, principal.uid));
+ });
+ }
+ }
+
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 = cargoById("jdl_power");
+ 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" });
From fec9b5655f121250af355c71f435221e89124b98 Mon Sep 17 00:00:00 2001
From: Arnold Gandarillas Castillo
Date: Fri, 28 Aug 2026 16:22:38 -0400
Subject: [PATCH 20/25] fix(beacon): tag the last untagged refusal; make two
log guards falsifiable
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
"member has no email" was the one refusal thrown with no details.reason — so the UI
degraded it to the generic "No se pudo…", verbatim the dead end PROVISION_BLOCK_REASONS
was created to remove, and it SHADOWED the tagged member-email-malformed refusal for
the empty-string case, which is the likelier of the two (memberDocSchema's email is a
bare z.string()). Absent, empty and malformed have the same operator remedy, so they
are one tagged check now.
ADMIN_SDK_EMAIL_SHAPE was copied verbatim from the Admin SDK on the argument that
tightening it would start rejecting addresses Firebase accepts. True in general, but
`[^@]` matches \n, \r, \t, spaces and NUL, and Identity Toolkit rejects those
server-side anyway — so "pres@jci.bo\n" passed this screen AND the SDK's own check,
reached the API, and came back as an opaque `internal` with no reason. That IS the
unprovisionable-with-no-hint failure the constant exists to prevent. \s and \p{C} are
excluded now; the three SDK-accepted fixtures still pass.
Two log guards could not fail:
- The BLOCKING "bounds the log" test appended its one oversized id LAST in a 10,001
entry array, outside sampleRejectedIds' .slice(0, 10) window. The per-entry length
assertion only ever saw 9-character ids, so deleting .map(truncateForLog) left it
green — while a member whose FIRST junk roleId is 1,500 bytes serializes raw, the
>256 KB entry-dropped failure the test's own comment names. It goes first now.
- "stays serializable when the cut lands mid-surrogate-pair" could not produce a lone
surrogate: an all-astral fixture is pairs on even indices and the cap is 64, so the
cut always landed ON a boundary. Both its assertions were unconditionally true
besides (JSON.stringify has not thrown on a lone surrogate since ES2019, and
toBeTruthy() holds for every object). A leading BMP char shifts the pairs, and
truncateForLog now drops the orphan rather than shipping an ill-formed log field.
And provision-deps.ts held a byte-identical second copy of the logError sink whose
sibling's doc comment reads "defining a second console.error wrapper at each of those
call sites would be the copy guardrail #1 forbids". Both now come from firestore-util,
so routing beacon's logs elsewhere is one edit that reaches every port.
Mutation-tested: neutralizing the surrogate trim or the sample truncation turns
exactly its own test red.
Co-Authored-By: Claude Opus 5 (1M context)
---
.../src/claims-sync/firestore-deps.test.ts | 14 ++++++--
apps/beacon/src/claims-sync/firestore-deps.ts | 13 ++++----
apps/beacon/src/firestore-util.test.ts | 28 ++++++++++++----
apps/beacon/src/firestore-util.ts | 18 +++++++++-
apps/beacon/src/provision-deps.ts | 4 +--
.../beacon/src/provision-member-login.test.ts | 30 ++++++++++++++---
apps/beacon/src/provision-member-login.ts | 33 ++++++++++++-------
7 files changed, 105 insertions(+), 35 deletions(-)
diff --git a/apps/beacon/src/claims-sync/firestore-deps.test.ts b/apps/beacon/src/claims-sync/firestore-deps.test.ts
index 28fc56ee..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 };
@@ -272,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 d3547995..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, truncateForLog, type LogSink } 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";
@@ -186,10 +186,11 @@ export interface FirestoreClaimsDeps extends ClaimsSyncDeps {
staleBuiltInRoleKeys(): Promise;
}
-/** Exported so the trigger/callable call sites can hand the SAME sink to `parseMember`, which
- * runs before any deps instance exists. Defining a second `console.error` wrapper at each of
- * those three call sites would be the copy this repo's guardrail #1 forbids. */
-export const logError: LogSink = (message, meta) => console.error(message, meta);
+/** 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>();
@@ -354,6 +355,6 @@ export function firestoreClaimsDeps(db: Firestore, auth: Auth): FirestoreClaimsD
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: (message, meta) => console.warn(message, meta),
+ logWarn,
};
}
diff --git a/apps/beacon/src/firestore-util.test.ts b/apps/beacon/src/firestore-util.test.ts
index cb47a14b..474e3a06 100644
--- a/apps/beacon/src/firestore-util.test.ts
+++ b/apps/beacon/src/firestore-util.test.ts
@@ -26,12 +26,26 @@ describe("truncateForLog", () => {
expect(truncateForLog("y".repeat(65))).toBe(`${"y".repeat(64)}…`);
});
- it("stays serializable when the cut lands mid-surrogate-pair", () => {
- // `.slice` can split an astral pair into a lone surrogate. JSON.stringify has been
- // well-formed since ES2019 and escapes it, so the log entry survives — this pins that the
- // helper never produces something the structured sink would reject.
- const astral = "𝒳".repeat(40); // 2 UTF-16 units each, so the 64-char cut lands mid-pair
- expect(() => JSON.stringify({ id: truncateForLog(astral) })).not.toThrow();
- expect(JSON.parse(JSON.stringify({ id: truncateForLog(astral) }))).toBeTruthy();
+ // 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 32a34755..37deda88 100644
--- a/apps/beacon/src/firestore-util.ts
+++ b/apps/beacon/src/firestore-util.ts
@@ -28,6 +28,14 @@ export function isSafeDocId(id: unknown): id is string {
/** 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
@@ -35,5 +43,13 @@ const LOG_ID_MAX_CHARS = 64;
* 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 {
- return value.length > LOG_ID_MAX_CHARS ? `${value.slice(0, LOG_ID_MAX_CHARS)}…` : value;
+ 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/provision-deps.ts b/apps/beacon/src/provision-deps.ts
index 4d6ff575..b219ddff 100644
--- a/apps/beacon/src/provision-deps.ts
+++ b/apps/beacon/src/provision-deps.ts
@@ -1,11 +1,9 @@
import type { Auth } from "firebase-admin/auth";
import type { Firestore } from "firebase-admin/firestore";
import { readPositionGrants } from "./read-position-grants.js";
-import type { LogSink } from "./firestore-util.js";
+import { logError } from "./firestore-util.js";
import type { ProvisionDeps } from "./provision-member-login.js";
-const logError: LogSink = (message, meta) => console.error(message, meta);
-
// 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 {
diff --git a/apps/beacon/src/provision-member-login.test.ts b/apps/beacon/src/provision-member-login.test.ts
index d72c87d6..3cd2a456 100644
--- a/apps/beacon/src/provision-member-login.test.ts
+++ b/apps/beacon/src/provision-member-login.test.ts
@@ -497,9 +497,17 @@ 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 () => {
@@ -509,7 +517,21 @@ describe("provisionMember", () => {
// 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[] = [];
- for (const email of ["not-an-email", "@b.co", "a@", "a@b@c.co", " "]) {
+ // 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,
diff --git a/apps/beacon/src/provision-member-login.ts b/apps/beacon/src/provision-member-login.ts
index c33883b5..0f343c59 100644
--- a/apps/beacon/src/provision-member-login.ts
+++ b/apps/beacon/src/provision-member-login.ts
@@ -48,11 +48,19 @@ function provisionBlocked(
return new HttpsError(code, message, { reason });
}
-/** The Admin SDK's OWN email predicate (`validator.isEmail`: `/^[^@]+@[^@]+$/`), copied
- * verbatim rather than tightened. The point is to refuse exactly what `getUserByEmail` /
- * `createUser` would refuse — a stricter RFC-ish pattern would start rejecting addresses
- * Firebase happily accepts, which is a worse failure than the one being fixed. */
-const ADMIN_SDK_EMAIL_SHAPE = /^[^@]+@[^@]+$/;
+/** 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, reach Identity Toolkit, and come back
+ * INVALID_EMAIL → `auth/invalid-email` → rethrown by `nullIfUserNotFound` as an opaque
+ * `internal` with no `details.reason`. That is exactly the unprovisionable-with-no-hint
+ * failure this constant exists to prevent, and `firestore.rules` never constrains
+ * `members.email`, so a CSV paste or any `update:Member` holder can store one. Rejecting them
+ * here costs nothing: the server rejects them anyway, and now with a reason the UI can name. */
+const ADMIN_SDK_EMAIL_SHAPE = /^[^@\s\p{C}]+@[^@\s\p{C}]+$/u;
export interface ProvisionUser {
uid: string;
@@ -177,23 +185,26 @@ 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");
- }
- const email = member.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, which nullIfUserNotFound rethrows and the caller receives as an opaque
// `internal`. That member is then unprovisionable through this callable — with no hint why —
// until someone edits the doc in the console. firestore.rules deliberately does not
// shape-validate `email` on the admin write lane, so the shape reaches here unchecked.
- if (!ADMIN_SDK_EMAIL_SHAPE.test(email)) {
+ //
+ // 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 provisionBlocked(
"failed-precondition",
- "member's stored email is not a valid address; correct it before provisioning",
+ "member's stored email is missing or not a valid address; correct it before provisioning",
"member-email-malformed",
);
}
+ const email = member.email;
const linkedUid = typeof member.uid === "string" && member.uid.length > 0 ? member.uid : null;
let user = await deps.getUserByEmail(email);
From 0ca459a071aa5269b93a9bbc0bad814ddce8e815 Mon Sep 17 00:00:00 2001
From: Arnold Gandarillas Castillo
Date: Fri, 28 Aug 2026 16:22:58 -0400
Subject: [PATCH 21/25] docs(plans): cite the shipped guards by symbol, not by
line number
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The block added under "Confirmed against the shipped code, not restated from memory"
had every one of its line citations invalidated by the same PR that wrote them — stale
by 25-45 lines. The worst was `sync.ts:126-134`, cited as "the real trust computation,
unchanged since", which now points at a logging block inserted afterwards: an auditor
following the doc to verify the power-seat guard lands on unrelated code and can
reasonably conclude the guard was removed.
Symbols instead. Guardrail #6 is about claims that stay true, and a line number in a
file under active change is not one. The pre-implementation refs elsewhere in the doc
are left alone — they are labelled as such and are historically accurate.
Co-Authored-By: Claude Opus 5 (1M context)
---
docs/plans/board-seat-delegation.md | 32 ++++++++++++++++-------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/docs/plans/board-seat-delegation.md b/docs/plans/board-seat-delegation.md
index 64fcd694..d705609d 100644
--- a/docs/plans/board-seat-delegation.md
+++ b/docs/plans/board-seat-delegation.md
@@ -62,7 +62,8 @@ proposal was itself found to be a hole and closed before merge:
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` (`apps/beacon/src/provision-member-login.ts:206`) is therefore:
+shipped guard in `provisionMember` (the ADOPTION GUARD in
+`apps/beacon/src/provision-member-login.ts`) is therefore:
```ts
if (!callerHoldsAdminRole && (user !== null || linkedUid !== null)) {
@@ -85,7 +86,7 @@ account creation/adoption + uid linking + claim writing.
### 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:97-135`). A delegate self-seats
+`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
@@ -153,8 +154,11 @@ 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. Confirmed against the shipped code, not
-restated from memory:
+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
@@ -163,17 +167,16 @@ restated from memory:
`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
- (`apps/beacon/src/provision-member-login.ts:213-251`) checks both claims-mint sources
- `syncMemberClaims` reads: `hasDirectGrants()` (`:83-99`, `roleIds`/`permissionOverrides`) and a
- per-term cargo read via `readCargoIds()` (`:101-139`, 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()`
- (`:119-139`) yields `""` — not skip — for a non-object term, a non-string `cargoId`, or one
+ (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()` (`:83-99`) is symmetric for
+ 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.** `apps/beacon/src/provision-deps.ts:25-29` swallows exactly
+- **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.
@@ -218,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.
@@ -420,7 +423,8 @@ Rejected alternatives:
**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 (`apps/beacon/src/claims-sync/sync.ts:126-134`):
+ reads (the trust computation at the end of `resolveTrustedGrants`,
+ `apps/beacon/src/claims-sync/sync.ts`):
```ts
const assignerIsAdmin = assigner.roles.includes("Admin");
From 3d933fe971cad976ff9bcae256b876b8467697ad Mon Sep 17 00:00:00 2001
From: Arnold Gandarillas Castillo
Date: Fri, 28 Aug 2026 17:22:26 -0400
Subject: [PATCH 22/25] fix(beacon): tag Identity Toolkit's own invalid-email,
not just the shape screen
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
firebase-functions-reviewer, Medium. Tightening ADMIN_SDK_EMAIL_SHAPE narrowed the
reason-less surface without closing it, because the screen is a SHAPE test and Identity
Toolkit's is a SEMANTIC one. "a@.", ".a@b.co", "a..b@c.co" each carry one @, no
whitespace and no control characters — so they pass this screen AND the Admin SDK's own
isEmail, reach the API, and come back auth/invalid-email, which nullIfUserNotFound
rethrew as an opaque `internal` with no details.reason. The operator got the generic
"No se pudo…" and the member stayed unprovisionable with no hint: the same dead end the
previous commit removed, reached by a different road.
Tagged at the PORT rather than by chasing regex precision, so the class is closed
however the pattern evolves — the regex is a cheap pre-filter now, not the sole
guarantee. Both Auth entry points route through it (getUserByEmail and createUser), and
one exported factory raises the refusal so the two layers cannot word it differently.
Also retargets two comments that this branch made false (guardrail #6):
- the "do NOT tighten this regex" test comment, written against the old pattern, now
distinguishes the tightening that is safe (whitespace + \p{C}, which the server
rejects anyway) from the one that is not (RFC structure).
- docs/engineering-guardrails.md cited firestore-deps.ts:169 for getRolesByIds, which
now lives at :310. Cited by symbol.
NOT fixed, recorded instead: the power-seat loop reads one cargo per distinct term
serially with no ceiling (needs a console-written positions map with hundreds of terms;
rules are term-pinned, and it fails closed), no logWarn names the orphaned uid when
linkUid fails after createUser, and no beacon callable sets enforceAppCheck.
Co-Authored-By: Claude Opus 5 (1M context)
---
apps/beacon/src/provision-deps.test.ts | 34 +++++++++++++++----
apps/beacon/src/provision-deps.ts | 23 ++++++++++---
.../beacon/src/provision-member-login.test.ts | 12 ++++---
apps/beacon/src/provision-member-login.ts | 23 ++++++++++---
docs/engineering-guardrails.md | 2 +-
5 files changed, 74 insertions(+), 20 deletions(-)
diff --git a/apps/beacon/src/provision-deps.test.ts b/apps/beacon/src/provision-deps.test.ts
index 6212d2ee..5252e9ff 100644
--- a/apps/beacon/src/provision-deps.test.ts
+++ b/apps/beacon/src/provision-deps.test.ts
@@ -10,7 +10,11 @@ const db = {} as Firestore;
* 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; byEmail?: Record }) {
+function fakeAuth(opts: {
+ createError?: unknown;
+ byEmailError?: unknown;
+ byEmail?: Record;
+}) {
const calls = { createUser: [] as string[], getUserByEmail: [] as string[] };
const auth = {
createUser: async ({ email }: { email: string }) => {
@@ -20,6 +24,7 @@ function fakeAuth(opts: { createError?: unknown; byEmail?: Record {
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;
@@ -46,11 +51,7 @@ describe("firestoreProvisionDeps.createUser", () => {
// 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",
- "auth/invalid-email",
- ]) {
+ 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,
@@ -59,6 +60,27 @@ describe("firestoreProvisionDeps.createUser", () => {
}
});
+ // 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(
diff --git a/apps/beacon/src/provision-deps.ts b/apps/beacon/src/provision-deps.ts
index b219ddff..6db6d88c 100644
--- a/apps/beacon/src/provision-deps.ts
+++ b/apps/beacon/src/provision-deps.ts
@@ -2,13 +2,28 @@ 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 type { ProvisionDeps } from "./provision-member-login.js";
+import { memberEmailMalformed, 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. */
+function tagInvalidEmail(err: unknown): never {
+ if (authCode(err) === "auth/invalid-email") 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 {
@@ -25,7 +40,7 @@ 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),
diff --git a/apps/beacon/src/provision-member-login.test.ts b/apps/beacon/src/provision-member-login.test.ts
index 3cd2a456..c2b13844 100644
--- a/apps/beacon/src/provision-member-login.test.ts
+++ b/apps/beacon/src/provision-member-login.test.ts
@@ -552,10 +552,14 @@ describe("provisionMember", () => {
});
it("does NOT refuse the unusual addresses the Admin SDK accepts", async () => {
- // The screen is the SDK's own predicate (`/^[^@]+@[^@]+$/`), not an RFC validator: a
- // plus-tag, a bare hostname and a non-ASCII local part all provision as before. Tightening
- // this regex would make members with legitimate addresses unprovisionable — the exact
- // failure the screen exists to prevent, pointed the other way.
+ // 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({
diff --git a/apps/beacon/src/provision-member-login.ts b/apps/beacon/src/provision-member-login.ts
index 0f343c59..b4459154 100644
--- a/apps/beacon/src/provision-member-login.ts
+++ b/apps/beacon/src/provision-member-login.ts
@@ -48,6 +48,23 @@ function provisionBlocked(
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: this module screens the SHAPE up front, and the port (provision-deps) 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. Without the port tagging that, it surfaced
+ * as an opaque `internal` and the operator got the generic "No se pudo…" — the dead end
+ * PROVISION_BLOCK_REASONS exists to remove, reached by a different road. */
+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",
+ );
+}
+
/** The Admin SDK's OWN email predicate (`validator.isEmail`: `/^[^@]+@[^@]+$/`), plus the one
* tightening that is strictly safe: no whitespace, no control characters.
*
@@ -198,11 +215,7 @@ export async function provisionMember(
// 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 provisionBlocked(
- "failed-precondition",
- "member's stored email is missing or not a valid address; correct it before provisioning",
- "member-email-malformed",
- );
+ throw memberEmailMalformed();
}
const email = member.email;
const linkedUid = typeof member.uid === "string" && member.uid.length > 0 ? member.uid : null;
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 |
From 8c4cf1c268a9640bedcbba2dd2f72d0b1ad78089 Mon Sep 17 00:00:00 2001
From: Arnold Gandarillas Castillo
Date: Fri, 28 Aug 2026 17:22:43 -0400
Subject: [PATCH 23/25] test(rules): make each converse row prove its own lane
reachability
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
firestore-security-reviewer, Low. `assertFails` is REASON-BLIND: the new converse rows
asserted only "denied", so a later change denying those 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. That is the
green-for-the-wrong-reason failure this whole file was written to end, reintroduced by
the fix for it.
Each row now writes a grant-free JDL seat FIRST and asserts it succeeds: same principal,
same lane, same fixture shape, one cargo apart, so the conjunct under test is the only
difference between the ALLOW and the DENY.
Extended to the create lane too. createPositionsSafe() applies the same
`(boardSeatDelegate() || cargoAssignableByNonAdmin())` conjunct, and create:Member alone
reaches the boundary ONLY there — it has no update lane at all, so the update-only loop
never probed it.
Re-ran both rules mutations against the hardened rows: dropping the CEL conjunct turns
10 tests red across both lanes, widening boardSeatDelegate() to canDo() turns 3 red
including the manage:all create row that did not exist before.
Also documents the one place this mirror is deliberately STRICTER than the rules, which
the reviewer surfaced and I am not fixing: "unresolvable" here means absent from the
PARSED catalog, and parseDocs drops a doc for any schema violation — so a cargo that
exists with grants: [] but a malformed `description` locks the editor out of a takedown
currentCargoGrantsEmpty() would have allowed. Availability only, needs a malformed doc,
Admin can still clear it, and resolving around the schema would mean reading positions
past the validation that keeps unparsed data out of the client. The corruption that
matters — a bad `grants` — locks on both sides.
Co-Authored-By: Claude Opus 5 (1M context)
---
.../members/lib/assignable-cargo-core.ts | 13 ++++-
.../cargo-assignment-parity.test.ts | 47 ++++++++++++++++---
2 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/apps/backstage/src/features/members/lib/assignable-cargo-core.ts b/apps/backstage/src/features/members/lib/assignable-cargo-core.ts
index 4f984479..3de24170 100644
--- a/apps/backstage/src/features/members/lib/assignable-cargo-core.ts
+++ b/apps/backstage/src/features/members/lib/assignable-cargo-core.ts
@@ -117,7 +117,18 @@ export function heldCargo(
};
}
-/** Whether the member is seated on SOMETHING whose power this editor cannot establish. */
+/** 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, needs a malformed catalog doc, and an Admin can still clear it.
+ * 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 bad `grants` — locks on BOTH sides,
+ * since a non-empty stored `grants` fails `size() == 0` too. */
function heldCargoUnresolvable(held: HeldCargo): boolean {
return held.cargoId !== null && held.cargoId !== undefined && held.cargo === undefined;
}
diff --git a/tests/firestore-rules/cargo-assignment-parity.test.ts b/tests/firestore-rules/cargo-assignment-parity.test.ts
index e0f19723..598248d5 100644
--- a/tests/firestore-rules/cargo-assignment-parity.test.ts
+++ b/tests/firestore-rules/cargo-assignment-parity.test.ts
@@ -434,17 +434,50 @@ describe("cargo assignment ⟷ rules: every OFFERED cargo is a write the emulato
// 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;
- for (const label of ["custom(update-Position)", "custom(update-Member)", "custom(manage-all)"]) {
+ const CONVERSE_PRINCIPALS = [
+ "custom(update-Position)",
+ "custom(update-Member)",
+ "custom(create-Member)",
+ "custom(manage-all)",
+ ];
+ for (const label of CONVERSE_PRINCIPALS) {
const principal = PRINCIPALS.find((p) => p.label === label);
if (principal === undefined) continue;
const g = gatesFor(principal);
- if (g.editMode === "none") continue;
- for (const cargoId of WITHHELD) {
- it(`BLOCKING: the rules DENY ${label} the ${cargoId} seat the client withholds`, async () => {
- expect(offeredCargoIds(g, null)).not.toContain(cargoId);
- const id = await seedMember({ key: "unseated", cargoId: null });
- await assertFails(writeUpdate(as(principal), id, cargoId, principal.uid));
+ const lanes: Lane[] = [
+ ...(g.editMode === "none" ? [] : (["update"] as const)),
+ ...(g.canCreate ? (["create"] as const) : []),
+ ];
+ 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));
+ }
});
}
}
From 0239ddf822442ba7a8af6f51d1b794f745e5f886 Mon Sep 17 00:00:00 2001
From: Arnold Gandarillas Castillo
Date: Fri, 28 Aug 2026 17:32:54 -0400
Subject: [PATCH 24/25] fix(beacon,rules): pin the null-vs-throw contract;
dissolve the import cycle
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Re-review of the previous commit. Verdict was fix-then-ship; this is the fix half.
M1 — `nullIfUserNotFound`'s contract ("null ONLY when the account does not exist —
a transient Auth error must throw, or a blip would misread a live linked account as
safely deleted") was asserted in two docblocks and pinned by NOTHING. `getUserByUid`,
the one the relink guard actually calls, had no coverage in this file at all, and
provision-member-login.test.ts drives hand-written fakes that bypass the port entirely.
The invisible mutation: widen the null branch to also swallow auth/internal-error and
the whole beacon suite stays green while an Identity Toolkit blip lets a caller
re-provision over a live account. Both lookups are now pinned both ways, and that exact
mutation turns the new BLOCKING row red.
L1 — the invalid-email tagging made provision-deps.ts and provision-member-login.ts a
real two-node import cycle, safe only because every cross-module reference sat inside a
hoisted function body. One top-level `const` reading across it — 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. The refusal factories move to provision-errors.ts and the cycle
is gone, not documented; the remaining edge is type-only and erased.
L2 — the tagged throw discards the underlying Auth error, and firebase-functions treats
a thrown HttpsError as an expected refusal, so it emits no "Unhandled error" line. That
left the failure class with zero trace in Cloud Logging (guardrail #4). Logged now, code
only, never the address.
Three comments corrected, all of them claims of mine this branch made false — the
mistake-class this whole PR is about:
- two present-tense comments still said auth/invalid-email "is rethrown as an opaque
internal", which is what the previous commit removed. They are the stated justification
for the shape screen, so a reader checking them would conclude one of the two guards is
redundant and delete the wrong one.
- "an Admin can still clear it" was false: an unresolvable cargo is absent from the
option list and Combobox clears by re-selecting the selected option, so the Admin's
remedy is reassignment.
- "a bad grants locks on BOTH sides" overreached: CEL defines .size() on strings and
maps, so `grants: ""` or `{}` reads as empty to the rules.
And two vacuity guards on the converse block: a renamed principal label would have
silently emitted no `it` for the create-Member row — the one that block exists for — and
a principal reaching no lane would vanish rather than fail.
Not taken, recorded: auth/invalid-uid still surfaces opaque (a different field, needs a
console-written uid), and the create-lane fixture is a minimal doc rather than the
mapper's full output (full-payload create parity lives in rules.test.ts).
Co-Authored-By: Claude Opus 5 (1M context)
---
.../members/lib/assignable-cargo-core.ts | 16 +++--
apps/beacon/src/provision-deps.test.ts | 51 +++++++++++++++-
apps/beacon/src/provision-deps.ts | 20 ++++++-
apps/beacon/src/provision-errors.ts | 43 ++++++++++++++
apps/beacon/src/provision-member-login.ts | 58 ++++++-------------
.../cargo-assignment-parity.test.ts | 16 +++++
6 files changed, 156 insertions(+), 48 deletions(-)
create mode 100644 apps/beacon/src/provision-errors.ts
diff --git a/apps/backstage/src/features/members/lib/assignable-cargo-core.ts b/apps/backstage/src/features/members/lib/assignable-cargo-core.ts
index 3de24170..b5103183 100644
--- a/apps/backstage/src/features/members/lib/assignable-cargo-core.ts
+++ b/apps/backstage/src/features/members/lib/assignable-cargo-core.ts
@@ -124,11 +124,17 @@ export function heldCargo(
* 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, needs a malformed catalog doc, and an Admin can still clear it.
- * 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 bad `grants` — locks on BOTH sides,
- * since a non-empty stored `grants` fails `size() == 0` too. */
+ * 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;
}
diff --git a/apps/beacon/src/provision-deps.test.ts b/apps/beacon/src/provision-deps.test.ts
index 5252e9ff..0763a52b 100644
--- a/apps/beacon/src/provision-deps.test.ts
+++ b/apps/beacon/src/provision-deps.test.ts
@@ -13,9 +13,15 @@ const db = {} as Firestore;
function fakeAuth(opts: {
createError?: unknown;
byEmailError?: unknown;
+ byUidError?: unknown;
byEmail?: Record;
+ byUid?: Record;
}) {
- const calls = { createUser: [] as string[], getUserByEmail: [] as string[] };
+ const calls = {
+ createUser: [] as string[],
+ getUserByEmail: [] as string[],
+ getUser: [] as string[],
+ };
const auth = {
createUser: async ({ email }: { email: string }) => {
calls.createUser.push(email);
@@ -29,10 +35,53 @@ function fakeAuth(opts: {
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", () => {
diff --git a/apps/beacon/src/provision-deps.ts b/apps/beacon/src/provision-deps.ts
index 6db6d88c..32cd1437 100644
--- a/apps/beacon/src/provision-deps.ts
+++ b/apps/beacon/src/provision-deps.ts
@@ -2,7 +2,8 @@ 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, type ProvisionDeps } from "./provision-member-login.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;
@@ -13,9 +14,19 @@ function authCode(err: unknown): unknown {
* `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. */
+ * 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") throw memberEmailMalformed();
+ 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;
}
@@ -47,6 +58,9 @@ export function firestoreProvisionDeps(db: Firestore, auth: Auth): ProvisionDeps
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, 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.ts b/apps/beacon/src/provision-member-login.ts
index b4459154..11274ab7 100644
--- a/apps/beacon/src/provision-member-login.ts
+++ b/apps/beacon/src/provision-member-login.ts
@@ -2,8 +2,8 @@ import { getAuth } from "firebase-admin/auth";
import { getFirestore } from "firebase-admin/firestore";
import { HttpsError, onCall } from "firebase-functions/v2/https";
import { isValidRole, type Role } from "@luminova/auth/roles";
-import type { ProvisionBlockReason } from "@luminova/types";
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";
@@ -36,34 +36,9 @@ export function nextClaims(existing: RawClaims | undefined, role: Role): { roles
return { roles };
}
-/** 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. */
-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: this module screens the SHAPE up front, and the port (provision-deps) 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. Without the port tagging that, it surfaced
- * as an opaque `internal` and the operator got the generic "No se pudo…" — the dead end
- * PROVISION_BLOCK_REASONS exists to remove, reached by a different road. */
-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",
- );
-}
+// 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.
@@ -71,12 +46,16 @@ export function memberEmailMalformed(): HttpsError {
* 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, reach Identity Toolkit, and come back
- * INVALID_EMAIL → `auth/invalid-email` → rethrown by `nullIfUserNotFound` as an opaque
- * `internal` with no `details.reason`. That is exactly the unprovisionable-with-no-hint
- * failure this constant exists to prevent, and `firestore.rules` never constrains
- * `members.email`, so a CSV paste or any `update:Member` holder can store one. Rejecting them
- * here costs nothing: the server rejects them anyway, and now with a reason the UI can name. */
+ * 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 {
@@ -204,10 +183,11 @@ export async function provisionMember(
if (member.active !== true) throw new HttpsError("failed-precondition", "member is not active");
// 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, which nullIfUserNotFound rethrows and the caller receives as an opaque
- // `internal`. That member is then unprovisionable through this callable — with no hint why —
- // until someone edits the doc in the console. firestore.rules deliberately does not
- // shape-validate `email` on the admin write lane, so the shape reaches here unchecked.
+ // 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
diff --git a/tests/firestore-rules/cargo-assignment-parity.test.ts b/tests/firestore-rules/cargo-assignment-parity.test.ts
index 598248d5..669b6d61 100644
--- a/tests/firestore-rules/cargo-assignment-parity.test.ts
+++ b/tests/firestore-rules/cargo-assignment-parity.test.ts
@@ -452,14 +452,30 @@ describe("cargo assignment ⟷ rules: every OFFERED cargo is a write the emulato
"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"
From 1f20cdeb9778645f8c27b35297d38366f8e3020a Mon Sep 17 00:00:00 2001
From: Arnold Gandarillas Castillo
Date: Fri, 28 Aug 2026 21:27:40 -0400
Subject: [PATCH 25/25] chore: reviews
Reviews: 0239ddf822442ba7a8af6f51d1b794f745e5f886 security-review,firestore-security-reviewer,firebase-functions-reviewer,code-review,simplify,react-best-practices,bundle-budget-watcher