From 31c778187b8c7c0dbcfa32c9c8412bdac354cb86 Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 18:30:34 -0400 Subject: [PATCH 01/15] feat(types): add BoardSeat and MemberLogin permission subjects Two delegable capabilities an Admin can grant temporarily and revoke: update:BoardSeat (seat a member on any vacant cargo, CEL and power-granting alike) and create:MemberLogin (provision a member's Auth login). Both gates are exact hasPerm code tests, so manage:all does not satisfy them and the sibling codes the cross-product generates stay inert. Spec and plan record the three guards adversarial review required: the trust gate must be non-reflexive, currentCargoGrantsEmpty stays Admin-only, and a non-Admin provision caller may only mint a new Auth account. Co-Authored-By: Claude Opus 5 (1M context) --- .../permissions/lib/permission-matrix.ts | 2 + docs/plans/board-seat-delegation.md | 527 ++++++++++++++++++ docs/specs/board-seat-delegation.md | 99 ++++ packages/types/src/permission.test.ts | 26 + packages/types/src/permission.ts | 15 + 5 files changed, 669 insertions(+) create mode 100644 docs/plans/board-seat-delegation.md create mode 100644 docs/specs/board-seat-delegation.md diff --git a/apps/backstage/src/features/permissions/lib/permission-matrix.ts b/apps/backstage/src/features/permissions/lib/permission-matrix.ts index 1a7356a2..7c8e7cff 100644 --- a/apps/backstage/src/features/permissions/lib/permission-matrix.ts +++ b/apps/backstage/src/features/permissions/lib/permission-matrix.ts @@ -38,6 +38,8 @@ export const SUBJECT_LABELS: Record, string> = Lead: "Prospectos", Notification: "Notificaciones", Showcase: "Destacados públicos", + BoardSeat: "Asientos de directiva", + MemberLogin: "Acceso de miembros", }; /** Human label for a single code, e.g. "Editar Miembros". */ diff --git a/docs/plans/board-seat-delegation.md b/docs/plans/board-seat-delegation.md new file mode 100644 index 00000000..61207936 --- /dev/null +++ b/docs/plans/board-seat-delegation.md @@ -0,0 +1,527 @@ +# Implementation Plan — Board-seat + member-login delegation + +Branch: `feat/board-seat-delegation` + +## 0. Accepted decision + +The chapter owner has accepted that `update:BoardSeat` is a **claims-minting delegation**: a +delegate may seat a member on a cargo whose `grants` include `Admin`, and beacon's trust gate +will mint that claim. Deliberate; no guard is added against *that*. The delegation is meant to +be granted temporarily and revoked. + +The acceptance is explicitly premised on **revocability**. Three guards below exist to make that +premise true; without them it is false. They are not a narrowing of the accepted decision — a +delegate can still seat any vacant cargo, including `Presidente`. + +## 0b. Security guards (added after adversarial review — G1/G2/G3) + +Two independent reviews found three defects, all confirmed by reading code. Each guard below +removes a capability the owner did **not** ask for, and none removes one they did. + +### G1 — `provisionMemberLogin`: a non-Admin caller may only mint a NEW Auth account + +`provision-member-login.ts:82-114`. The relink refusal at `:85` is guarded by +`linkedUid !== null`, so it does not run for a member doc with no `uid`. Rules never constrain +`members.email` (`firestore.rules:428-435` pins `totalPoints`/`uid`/`publicProfile`/`name`/ +`roleIds`/`positions`, not `email`), and no uniqueness check exists. So a +`create:Member` + `create:MemberLogin` holder could create `members/evil` carrying a sitting +Admin's email, call the callable, and have it (a) adopt the Admin's Auth account, (b) strip its +claims via `adoptedClaims` at `:109`, (c) `linkUid` the Admin's uid onto their own doc through +the admin SDK at `:111`, and (d) return a **password-reset link for the Admin's email** at +`:112-114`. Full account takeover, unrelated to board seating. + +**Guard:** thread the caller's privilege into `provisionMember`. For a non-Admin caller require +`user === null || user.uid === linkedUid` **before** `:97` — a delegate may create a brand-new +Auth account or re-provision an already-linked one, never adopt a pre-existing account. Costs +the delegate nothing: a genuinely new member has no existing account. + +Note also: the invite **email** is not the privileged part. `requestPasswordReset` +(`apps/backstage/src/lib/auth/request-password-reset.ts`) is a plain client-side +`sendPasswordResetEmail` any signed-in user can already call. The callable's privilege is +account creation/adoption + uid linking + claim writing. + +### G2 — the trust gate must be non-reflexive on self-assignment + +`sync.ts:69` + `compute-roles.ts:8-12`. A delegate self-seats `Presidente` in one write on the +positions-only lane; beacon mints `roles: ["Admin","Member"]`. Revoking `update:BoardSeat` then +re-fires `onMemberWritten`, the gate reads their **live** claims, finds the `Admin` role the +cargo just minted, and re-honors the grants. The claim satisfies the gate that minted it, so the +delegation is permanent. `recomputeAllClaims` runs the same code and does not break the loop. + +**Guard:** when `assignedBy === member.uid`, trust **only** `assigner.perms.includes("update:BoardSeat")`, +never `assigner.roles.includes("Admin")`. Cargo-derived Admin can no longer bootstrap its own +trust. An Admin seating *someone else* is unaffected; an Admin self-seating still works via the +perm if they hold it, and via a second Admin otherwise. + +Test: assigner === target, `roles:["Admin"]`, `perms:[]` → grants dropped. + +### G3 — `currentCargoGrantsEmpty()` stays Admin-only + +`computeMemberRoles` derives the `roles` claim **exclusively** from cargo grants; directly +assigned `roleIds` feed `perms`, never `roles`. So bypassing `currentCargoGrantsEmpty()` lets a +delegate clear every Admin's cargo and strip them all. After the last one there is no Admin: +`setUserRoles` is `requireAdmin`, `roles/*` and `permissionOverrides` writes are Admin-only. +Unrecoverable outside the Firebase console. + +**Guard:** substitute `boardSeatDelegate()` only on the NEW-side conjunct. `positionsAssignmentSafe()` +becomes: + +``` +(boardSeatDelegate() || cargoAssignableByNonAdmin()) + && (hasAnyRole(['Admin']) || currentCargoGrantsEmpty()) +``` + +A delegate seats any **vacant** cargo but cannot displace a sitting power-cargo holder. Hand-over +stays an Admin action. `createPositionsSafe()` has no old side, so it takes the plain +substitution. + +If the owner later wants de-elevation delegated too, that is a separate code with its own +anti-lockout guard — not this one. + +## The two codes + +| Subject | Spanish label | Live code | Grants | +|---|---|---|---| +| `BoardSeat` | "Asientos de directiva" | `update:BoardSeat` | assign/clear ANY cargo — CEL category and power-granting alike — on the member CREATE and UPDATE lanes | +| `MemberLogin` | "Acceso de miembros" | `create:MemberLogin` | run `provisionMemberLogin`: link a Firebase Auth user and return the password-reset action link (the "Enviar acceso" invite email). Cargo-agnostic — applies to every new member, board seat or not. | + +Independent by construction: an Admin can grant emailing without board seating and vice versa. + +## Corrections to the original brief + +### C1 (critical). `canAssignPowerGrants` also governs the `/positions` CATALOG + +Five consumers, one of which is the catalog: + +- `member-invite-drawer.tsx:180` -> `MemberForm allowPowerGrants` (create lane) — in scope +- `member-drawer.tsx:153` -> `MemberForm allowPowerGrants` (update lane) — in scope +- `member-profile-page.tsx:148` -> `MemberForm allowPowerGrants` — in scope +- `member-profile-page.tsx:176` -> `MemberPositionsForm allowPowerGrants` — in scope +- `positions-page.tsx:178` -> `PositionForm canEditGrants` — **OUT of scope** + +`PositionForm.canEditGrants` unlocks the `grants` editor, the `category` select and the +board-cargo `title`/`titleFemale` fields (`position-form.tsx:88,134,157,179`) — exactly the +`/positions` arms that stay Admin-only. Widening the shared flag would render that editor for a +delegate whose write `firestore.rules` then rejects: the render-then-403 shape `use-can.ts`'s own +`canFeatureInitiatives` comment exists to prevent. + +**Resolution:** split the flag. + +- `canAssignBoardSeat` = `hasAnyRole(["Admin"]) || hasPerm("update:BoardSeat")` — the four member-lane call sites. +- `canEditCargoCatalog` = `hasAnyRole(["Admin"])` — `positions-page.tsx` only. +- `canAssignPowerGrants` is deleted. Both replacements are `Can` members, so every call site is compiler-guided. + +### C2. `PERMISSION_CAP` and `ALL_PERMISSION_CODES` — no test breaks + +- `SUBJECTS` 14 -> 16, `ALL_PERMISSION_CODES` 84 -> 96. `permission.test.ts:34` asserts + `ACTIONS.length * SUBJECTS.length`, self-adjusting. +- The 1000-byte claim test (`permission.test.ts:38`) keys on the longest code, still + `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 + 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. + +### C3. Two permission surfaces; neither needs a component change + +- `MATRIX_SUBJECTS` is a runtime `SUBJECTS.filter(...)` (`permission-matrix.ts:6`). Both new subjects + appear as checkbox rows in the `/permisos` role editor grid automatically. The only compiler-guided + edit is `SUBJECT_LABELS`. +- The per-member surface — `member-roles-panel.tsx`, Admin-only, on `/members/$memberId`, a MultiSelect + of chips — derives options from `ALL_PERMISSION_CODES` + `permissionLabel()` (`:17-21`), so it picks + the codes up for free. Without the `SUBJECT_LABELS` entries the chip would read "Crear MemberLogin" + (raw-subject fallback at `permission-matrix.ts:46`). + +### C4. `firestore.rules`: exactly TWO substitution sites + +`cargoAssignableByNonAdmin()` and `currentCargoGrantsEmpty()` contain no role check of their own — +they are pure cargo predicates. + +| Function | Current | After | +|---|---|---| +| `positionsAssignmentSafe()` | `hasAnyRole(['Admin']) \|\| (cargoAssignableByNonAdmin() && currentCargoGrantsEmpty())` | `boardSeatDelegate() \|\| (...unchanged...)` | +| `createPositionsSafe()` | `assignedBySelf() && (hasAnyRole(['Admin']) \|\| cargoAssignableByNonAdmin())` | `assignedBySelf() && (boardSeatDelegate() \|\| ...)` | + +Two further facts: + +- **`update:BoardSeat` on its own opens nothing.** Both functions are conjuncts inside an arm whose + entry condition is `canDo('update','Member')`, `canDo('update','Position') && hasOnly(['positions'])`, + or `canDo('create','Member')`. Rules tests must use a principal holding one of those, or they pass + for the wrong reason. +- **The delegate also gains de-elevation.** Bypassing `currentCargoGrantsEmpty()` means a delegate can + clear or replace an existing power cargo, not just assign one. Say it in the rules comment. + +Explicitly UNCHANGED (Admin-role-only): the `/positions` create arm's +`hasAnyRole(['Admin']) || (grants == [] && !boardSurfacingCategory())`; the `/positions` update arm's +`hasAnyRole(['Admin']) || (unchanged('grants') && unchanged('category') && ...)`; +`createPermissionAssignmentSafe()` / `updatePermissionAssignmentSafe()`; the members Admin takedown arm. + +### C5. `permsFromClaims` exists but is not reusable from `callable-auth.ts` + +`firestore-deps.ts:16` defines it module-local, not exported, and its contract differs: it validates +through `isValidPermissionCode` and returns `PermissionCode[] | undefined`, where `undefined` means "no +`perms` key" — which `getExistingClaims` uses for its claim diff. Importing it would pull +`firebase-admin/firestore`, `chunk`, `role-doc` and `resolve-member-perms` into the callable trust +boundary for a boolean membership test. + +Instead: extract the genuinely shared three lines — `stringArrayClaim(request, key)` — so `callerRoles` +and the new perms reader share one implementation. Satisfies guardrail #1 for the logic actually +duplicated, without coupling the trust boundary to the claims-sync Firestore port. + +### C6. `member-drawer.tsx` has NO provision affordance + +Complete list of `isAdmin`-gated provision affordances: + +| File | Line | Gate today | Move to | +|---|---|---|---| +| `member-invite-drawer.tsx` | 41, 43, 51, 56, 184 | `isAdmin` | `canProvisionLogin` | +| `member-row-menu.tsx` | 50 | `` | `` | +| `member-profile-page.tsx` | 132 | `` | `` | + +`members-page.tsx` owns the mutation and passes `onProvision` down unconditionally — no change. +`ActionGate` needs no change: `role` and `when` are ANDed and `role` is optional (`action-gate.tsx:21`). + +### C7. Only ONE test file implements `ClaimsSyncDeps` + +`apps/beacon/src/claims-sync/sync.test.ts` — the `fakeDeps` factory (`:43-85`), the `spied` +spread-override (`:154-160`), and the standalone rejecting deps (`:536-547`). + +### C8. `assignable-cargo.ts` — no change needed + +Fully parameterized on `allowPowerGrants` (`:41-115`). Widening the source of that boolean is the +entire change. Its doc comment needs one sentence ("Admin, or an `update:BoardSeat` delegate") so the +file stops claiming the branch is Admin-only. + +### C9. `BUILT_IN_ROLE_PERMS` — no change needed + +Neither code is seeded onto any built-in role. `role-seed.mjs` mirrors only `BUILT_IN_ROLE_PERMS` / +`ROLE_LABELS` / `ROLE_DESCRIPTIONS`, cross-checked by `role-definition.mirror.test.ts`. That table is +untouched. `seed-contract.test.ts` never enumerates `SUBJECTS`. + +--- + +## Slice 1 — Vocabulary, labels, spec + +1. `docs/specs/board-seat-delegation.md` (new) — the two codes; the accepted escalation decision; the + "`update:BoardSeat` alone opens nothing" dependency; the "revoking de-elevates on next write" note; + the out-of-scope list from C4; the cap note from C2. +2. `packages/types/src/permission.ts` — add `"BoardSeat"` and `"MemberLogin"` to `SUBJECTS`, before + `"all"`. Each gets a comment in the shape of the existing `Showcase` block: name the ONE live code, + state the gate is an exact `hasPerm`, state the sibling codes are inert *because* the gate is exact. +3. `packages/types/src/permission.test.ts` — subject in `SUBJECTS`; live code validates; inert siblings + validate too (the matrix renders the full grid and the role editor's write validation would reject an + assignable-but-unvalidatable code). +4. `apps/backstage/src/features/permissions/lib/permission-matrix.ts` — `SUBJECT_LABELS` gains + `BoardSeat: "Asientos de directiva"` and `MemberLogin: "Acceso de miembros"`. No `MATRIX_SUBJECTS` + edit (C3). + +Verify: `pnpm --filter @luminova/types run build && pnpm --filter @luminova/auth run build`, +`pnpm --filter @luminova/types run ci`, `pnpm --filter backstage run typecheck`. + +Commit: `feat(types): add BoardSeat and MemberLogin permission subjects` + +## Slice 2 — `firestore.rules` + rules tests + +1. `firestore.rules` — add above `cargoAssignableByNonAdmin()` (define-before-use is this file's + convention at that point): + + ``` + function boardSeatDelegate() { + return hasAnyRole(['Admin']) || hasPerm('update:BoardSeat'); + } + ``` + + Comment in the shape of `canCurateFeatured()`'s: Admin by ROLE (locked, undeactivatable); everyone + else by exact PERM so revoking the code revokes the authority, which a surviving role NAME would not; + `hasPerm` not `canDo` so `manage:all` cannot satisfy it and the sibling codes stay inert; and the + accepted decision that a delegate may seat a power-granting cargo, with beacon's trust gate widened + to match. + + Substitute at the two C4 sites. Extend the existing comments rather than replacing: the non-Admin + branch is unchanged, so a non-delegate is still held to + `cargoAssignableByNonAdmin() && currentCargoGrantsEmpty()` and the asymmetric grant-free-CEL takedown + survives; a delegate bypasses both conjuncts and therefore also gains replace/clear of a power cargo. + + Leave the `/positions` arms, the permission-assignment arms and the takedown arm alone. + +2. `tests/firestore-rules/rules.test.ts` — module-scoped principals beside `ORG_CHART`/`orgChart`: + + ```ts + const seatDelegate = () => as(SEAT_DELEGATE, [], ["update:Position", "update:BoardSeat"]); + const plainDelegate = () => as("seatonly-uid", [], ["update:BoardSeat"]); + ``` + + `update:Position` is load-bearing — without it the delegate never reaches an arm and every ALLOW + below would pass for the wrong reason. + + New fixtures in the once-only `beforeAll` seed: `members/m_delegate`, `members/m_delegate_power` + (seeded holding `pos1` in the current term), `members/m_delegate_cel`. + + Cases in `describe("firestore.rules — member positions assignment")`, before the terminal + "allows Admin to assign a power-conferring cargo" test: + - delegate assigns a grant-free CEL cargo (`pos_cel_free`) to `m_delegate_cel`, self-stamped — the + case `orgChart()` is denied twelve lines above. Pair them in the comment. + - delegate assigns a power-granting cargo (`pos1`) to `m_delegate`, self-stamped. + - delegate replaces the power cargo on `m_delegate_power` with `pos_soft` — the + `currentCargoGrantsEmpty()` bypass, i.e. de-elevation authority. + - `plainDelegate()` (has `update:BoardSeat`, lacks `update:Position`/`update:Member`) is denied any + positions write. Non-vacuity pin for the whole feature. + - delegate denied a forged `assignedBy` — `assignedBySelf()` is outside the substituted disjunction. + - delegate denied a non-current-term write — `positionsDelta().hasOnly([currentTermKey()])`. + - delegate denied any ride-along non-positions field on the `hasOnly(['positions'])` lane. + - regression pin: `orgChart()` still denied `pos1` and `pos_cel_free`, still allowed the grant-free + JDL `pos_soft`. Extend the existing comments to name the new disjunct. + - takedown pin: a non-delegate `update:Position` holder may still CLEAR a member off a grant-free CEL + seat. **No test today** — `pos_cel_free` appears only in assign-side assertions. Most important + regression guard in the slice, because the delegate bypass touches the same expression. + + Create lane in `describe("firestore.rules — members")`: a delegate holding + `create:Member` + `update:BoardSeat` may create a member born on `pos_cel_free` and on `pos1`, + self-stamped; the same principal without `update:BoardSeat` is still denied both. Mirrors the + existing `new_cel_free` / `new_cel_admin` pair. + + Catalog-UNCHANGED in `describe("firestore.rules — positions")`: a catalog delegate holding + `["create:Position","update:Position","update:BoardSeat"]` is still denied minting a CEL cargo and a + JDL dirección, still denied setting `grants`, still denied changing `category`, still denied + retitling a board cargo. Without the catalog codes these pass vacuously. + +Verify: `pnpm --filter @luminova/firestore-rules-tests run ci` + +Commit: `feat(rules): delegate board-seat assignment via update:BoardSeat` + +## Slice 3 — beacon claims-sync trust gate + +**Port shape.** Rename `getUserRoles(uid): Promise` to +`getAssignerClaims(uid): Promise<{ roles: Role[]; perms: PermissionCode[] }>`. + +Rejected alternatives: + +- *A second method `getUserPerms(uid)`* — two calls to answer one question is two chances for a future + edit to consult one and not the other, precisely where that must not happen (guardrail #1). Free in + production (`loadUser` memo) but not in the fakes, which would need two hand-consistent maps. +- *Reuse `getExistingClaims(uid)` for the assigner* — structurally smallest, but it would blunt the + sharpest test in `sync.test.ts`: the `spied` case at `:154` asserts `assignerLookups).toEqual([])` to + prove `resolveTrustedGrants` short-circuits on `grants.length === 0` **before** consulting the + assigner. Merged, the same spy would also record the target's own claim read, degrading the assertion + to a uid-filtering heuristic. Also `getExistingClaims` returns `perms?:` optional by design (absence + vs. empty is meaningful for its diff) — the wrong contract for a membership test. + +1. `apps/beacon/src/claims-sync/sync.ts` — port change; `resolveTrustedGrants` keeps the `isSafeDocId` + screen and the `grants.length === 0` early return exactly as they are, then: + + ```ts + const assigner = assignedBy ? await deps.getAssignerClaims(assignedBy) : null; + const trusted = + assigner !== null && + (assigner.roles.includes("Admin") || assigner.perms.includes("update:BoardSeat")); + return trusted ? [...new Set(position.grants)] : []; + ``` + + Extend the doc comment: name `update:BoardSeat` as the second trust source; state why (a delegate + stamps their own uid into `assignedBy` via `assignedBySelf()`, so without this the seat publishes on + the public Directiva and mints nothing — half-working, not safe); state the live-claims + re-evaluation now also covers perm revocation; cross-reference `boardSeatDelegate()` in + `firestore.rules`. One line on the cap interaction (C2). + +2. `apps/beacon/src/claims-sync/firestore-deps.ts` — rename the impl at `:287`, returning both, reusing + the two module-local readers already there. No new read: `loadUser` is the same per-instance memo. + +3. `apps/beacon/src/claims-sync/sync.test.ts` — update the three port literals (C7). `fakeDeps` gains a + `userPerms` option map. New cases: + - honors power grants when the assigner holds `update:BoardSeat` and NOT the Admin role — mirror of + the existing "honors power grants when assignedBy is Admin" at `:109`. + - a `manage:all` perm holder who is neither Admin-by-role nor an `update:BoardSeat` holder does NOT + satisfy the gate — the exact-code property, asserted server-side too. + - revocation — same fixture with the assigner's perms emptied; the target recomputes down to a plain + `Member` claim. + - the existing `:129` positive-and-inert spy case: update the override, keep both assertions verbatim. + +Verify: `pnpm --filter beacon exec vitest run src/claims-sync/sync.test.ts`, then +`pnpm --filter beacon run ci`. + +Commit: `feat(beacon): honor cargo grants from an update:BoardSeat assigner` + +## Slice 4 — beacon callable auth + +1. `apps/beacon/src/callable-auth.ts` — extract the shared reader (C5), then: + + ```ts + export function requireAdminOrPerm(request: CallableRequest, code: PermissionCode): void { + if (!request.auth) throw new HttpsError("unauthenticated", "sign-in required"); + if (callerRoles(request).includes("Admin")) return; + if (stringArrayClaim(request, "perms").includes(code)) return; + throw new HttpsError("permission-denied", `Admin role or ${code} required`); + } + ``` + + Comment: exact-code match mirroring the rules' `hasPerm()` and `use-can`'s `hasPerm` — deliberately + NOT a `canDo`-style expansion, so `manage:all` does not satisfy it. `requireAdmin` keeps its exact + current behaviour and message. + +2. `apps/beacon/src/callable-auth.test.ts` (new) — this trust boundary is unasserted today. + `requireAdmin`: unauthenticated / non-Admin / Admin. `requireAdminOrPerm`: unauthenticated; Admin with + no `perms` claim at all; `{roles:["Member"], perms:["create:MemberLogin"]}` passes; + `{roles:["Member"], perms:["manage:all"]}` **throws** (wildcard-must-not-satisfy); + `{roles:["Member"], perms:["update:BoardSeat"]}` asked for `create:MemberLogin` throws (independence); + a non-array `perms` claim throws (fail-closed on a malformed token). + +3. `apps/beacon/src/provision-member-login.ts` — `requireAdmin(request)` -> + `requireAdminOrPerm(request, "create:MemberLogin")` at `:118`. Comment: the callable links an Auth + account and returns a password-reset action link, delegable per the owner decision; the `adoptedClaims` + de-elevation and the different-uid refusal are unchanged and still bind on every caller. + + Do not touch `seed-roles.ts`, `recompute-claims.ts:22,65,267`, `set-user-roles.ts:86`. + +Verify: `pnpm --filter beacon run ci` + +Commit: `feat(beacon): allow create:MemberLogin to call provisionMemberLogin` + +## Slice 5 — backstage gate flags (the C1 split) + +1. `apps/backstage/src/lib/authz/use-can.ts` — delete `canAssignPowerGrants`; add + `canAssignBoardSeat`, `canEditCargoCatalog`, `canProvisionLogin` with doc comments. Both perm-based + flags use `hasPerm` from `@luminova/auth/roles`, never `abilityAllows` — one line referencing the + existing `canFeatureInitiatives` comment. `canEditCargoCatalog`'s comment must state that this is what + the seat delegation deliberately does NOT widen, or the next person will "unify" the two flags and + reopen C1. +2. `apps/backstage/src/lib/authz/use-can.test.ts` — mirror the six existing `canFeatureInitiatives` cases + per flag, plus `canEditCargoCatalog === false` for an `update:BoardSeat` holder (the C1 pin) and + `manage:all -> false` for all three. +3. `positions-page.tsx` — `canAssignPowerGrants` -> `canEditCargoCatalog` (`:37`, `:178`). +4. `member-invite-drawer.tsx` — -> `canAssignBoardSeat` (`:41`, `:180`). +5. `member-drawer.tsx` — same (`:139`, `:153`). +6. `member-profile-page.tsx` — `gate.canAssignPowerGrants` -> `gate.canAssignBoardSeat` (`:148`, `:176`). +7. `assignable-cargo.ts` — no logic change (C8); one sentence added to the `allowPowerGrants` doc + comments and the two form prop JSDocs: the branch is "Admin, or an `update:BoardSeat` delegate". + +Verify: `pnpm --filter backstage run ci` + +Commit: `feat(backstage): split the seat, catalog and login-provision gates` + +## Slice 6 — provision affordances move to `canProvisionLogin` + +1. `member-invite-drawer.tsx` — `canProvisionLogin` drives `useState` (`:43`), the open-resync + `useEffect` (`:50-52`), `reset()` (`:56`) and the checkbox render guard (`:184`). **Keep the + `useEffect`** — its reason (the drawer mounts before the token's claims decode; the store emits empty + claims first and re-emits) applies harder to a perms-derived flag, since `perms` is minted by + claims-sync and arrives in the same late token. Rewrite the comment to say "Admin-role or + `create:MemberLogin`". +2. `member-invite-drawer.test.tsx` — parameterize the claims wrapper (`:10-18`); add: a delegate + `{roles:["Member"], perms:["create:Member","create:MemberLogin"]}` sees the checkbox, defaults it ON, + reaches `onProvision`; a `create:Member`-only creator does not see it and `onProvision` is never + called; a `manage:all` holder does not see it. +3. `member-row-menu.tsx` — `` at `:50` -> `when={canProvisionLogin}`; hoist + the `useCan()` call to the component body (hook rules). +4. `member-row-menu.test.tsx` — add a delegate-sees-it case and a `manage:all`-does-not case. +5. `member-profile-page.tsx` — `` at `:131-133` -> `when={gate.canProvisionLogin}`. + **Do not touch** the `` at `:203` around `` — `roleIds` / + `permissionOverrides` writes stay Admin-only, and that is the panel where the delegation is granted. + +Verify: `pnpm --filter backstage run ci` + +Commit: `feat(backstage): gate the invite affordances on create:MemberLogin` + +## Slice 7 — honest empty state for the Cargo combobox + +One shared place: both forms render the same `` + `Combobox options={cargoOptions}` +shape (`member-form.tsx:230-264`, `member-positions-form.tsx:76-104`). Their `locked` / `takedownOnly` +notes legitimately differ in wording, so extract only the new note. + +1. `apps/backstage/src/features/members/components/no-assignable-cargos-note.tsx` (new) — props-free, + in the established `role="note" className="text-ui-xs text-ink-3"` pattern. The quoted permission + name must equal `permissionLabel("update:BoardSeat")` = "Editar Asientos de directiva"; one comment + line says so, since the strings live in different features and nothing enforces the match. +2. `member-form.tsx` — render when `!positionsLocked && !allowPowerGrants && cargoOptions.length === 0`, + in the note stack after `cargoTakedown` (`:306`). +3. `member-positions-form.tsx` — same condition, after the `takedownOnly` note (`:130`). +4. `member-form.test.tsx` — with `allowPowerGrants={false}` and a CEL/power-only positions list, the note + renders and the combobox has no selectable option; with `allowPowerGrants` the note is absent and the + CEL option is present. +5. `member-positions-form.test.tsx` — the same pair, plus: when `locked` is true the locked note renders + and the empty note does not (mutually exclusive today because `cargoOptionsForEditor` appends the held + cargo disabled — pin it so a future change cannot produce two notes). +6. `assignable-cargo.ts` — no code change; one sentence noting an empty return for a non-delegate is a + real, expected state the forms explain. + +Residual, stated in the PR body and deliberately not fixed: an Admin/delegate facing a genuinely empty +catalog still sees the bare "Sin resultados" from `packages/ui/src/components/combobox.tsx:28`. That is +an empty-catalog problem, not a permissions problem; giving it the delegation copy would be a lie. + +Verify: `pnpm --filter backstage run ci` + +Commit: `feat(backstage): explain an empty cargo list to a non-delegate` + +## Slice 8 — docs, route, review, PR + +`docs/specs/position-assignment-lane.md` gains a cross-reference (its "who may assign" narrative is now +one disjunct out of date). `packages/auth/CLAUDE.md`'s Gotchas gains one line naming `BoardSeat` / +`MemberLogin` as hand-granted codes deliberately absent from `BUILT_IN_ROLE_PERMS`. + +Then, per the binding review-routing contract: + +```bash +.claude/hooks/route.sh +pnpm pr-tests +``` + +Run whatever the router prints, stamp with the exact command it emits (trailer in the final paragraph), +mirror the token list under `## Reviews` in the PR body. + +## Fact-check corrections (applied — cite these, not the originals) + +| Plan said | Actually | +|---|---| +| `sync.ts:106-118` cap block | `sync.ts:111-122` | +| "six existing `canFeatureInitiatives` cases" | five, at `use-can.test.ts:44,54,58,64,72` | +| `member-profile-page.tsx:203` roles-panel gate | `:202-206` | +| "define-before-use is this file's convention" | false — `cargoAssignableByNonAdmin()` calls `nonAdminAssignable()` defined *after* it (`firestore.rules:169-175`). Place `boardSeatDelegate()` next to `canCurateFeatured()` (`:323`) instead | +| "worst case ~855 B" | 830 B; longest code unchanged at 20 chars, so the byte count does not move at all | +| insert note after `member-form.tsx:306` | after `:312` (`:306-312` is the whole `cargoTakedown` block). `member-positions-form.tsx:130` is correct as written | +| "`callable-auth` is unasserted today" | no *direct* unit test; indirect coverage at `reseed-role-perms.emulator.test.ts:54` | +| C5: importing `permsFromClaims` pulls in `firebase-admin/firestore` | those imports are **type-only**; the real runtime pull-in is `../chunk.js`, `../firestore-util.js`, `./role-doc.js` (`firestore-deps.ts:3-8`). Argument stands, evidence did not | +| "`members-page.tsx` owns the mutation" | two owners — also a local `InviteAccess` in `member-profile-page.tsx:224-225` | +| "hoist the `useCan()` call" in `member-row-menu.tsx` | nothing to hoist; the component calls no hook. Add the import and the call | +| C7 rejecting-deps literal `:536-547` | `:537-548`. Also unstated: `fakeDeps`' **opts parameter type at `:34-41`** needs the `userPerms` map | +| C8 "fully parameterized (`:41-115`)" | file is 116 lines; `positionsLockedForNonAdmin` (`:41-45`) takes only the cargo — the flag is applied externally at `member-form.tsx:121` / `member-positions-form.tsx:52`. "No change needed" still holds | +| C4 quotes `createPositionsSafe()` | omits the leading `!('positions' in request.resource.data) ||` disjunct (`firestore.rules:224-227`) | +| `member-form.tsx:230-264` Cargo Field | `:230-265` | +| "denied twelve lines above" | ~70 — denial at `rules.test.ts:2941`, insertion point `:3012`, describe runs `:2741-3019` | + +**Resolved open question (was Risk 5): token refresh does NOT happen.** `claims.ts` is a pure +14-line decoder; the token comes from `auth-store.ts:54` calling `getIdTokenResult()` with no +`forceRefresh`. A newly granted `update:BoardSeat` / `create:MemberLogin` is invisible to +`firestore.rules`, `requireAdminOrPerm` and `useCan` until the hourly refresh or a re-login. +**Operator note for the spec: after granting or revoking either code, the delegate must sign out +and back in.** Not fixed here — a force-refresh on every load costs a network round trip on the +critical path. + +## Risks and open questions + +1. **C1 is the review's centre of gravity.** A single widened `canAssignPowerGrants` silently delegates + the `/positions` catalog editor. Verify `positions-page.tsx` reads `canEditCargoCatalog` and that + `use-can.test.ts` pins `canEditCargoCatalog === false` for an `update:BoardSeat` holder. +2. **`update:BoardSeat` grants nothing alone, and the UI does not say so.** An Admin ticking only that + box produces a delegate who can still do nothing — they also need `update:Position` or + `update:Member`. Neither surface hints at the dependency. Documented in the spec; not otherwise fixed. +3. **Revocation is retroactive and silent.** Removing the perm de-elevates the people they seated on the + *next write* to each member doc — possibly never. Inherited from the existing Admin behaviour, but + "revoke the delegation" is not "undo what they did". No operator sweep short of `recomputeAllClaims`. +4. **The cap can silently revoke the delegation.** A delegate exceeding `PERMISSION_CAP = 30` is written + `perms: []` fail-closed, taking `update:BoardSeat` with it. +5. **Token freshness on grant.** Rules, `requireAdminOrPerm` and `useCan` all read `perms` off the ID + token. A newly granted code does not take effect until the token refreshes. Verify whether + `apps/backstage/src/lib/authz/claims.ts` force-refreshes; if not, the delegate sees "no access" for up + to an hour with no explanation. Not addressed here. +6. **`stringArrayClaim` vs. reusing `permsFromClaims` (C5).** A reviewer applying guardrail #1 + mechanically will call the new reader a copy. Counter-argument is in C5; cheap to switch to a shared + `apps/beacon/src/claims-read.ts` if a reviewer insists. +7. **Rules-test non-vacuity.** Three new rules tests pass for the wrong reason if their principal is + under-permissioned. Check each `as(uid, [], [...])` literal by hand. +8. **The grant-free-CEL takedown has no test today.** Slice 2 adds it at the same time as the change to + the expression it depends on, so it proves post-change behaviour, not preservation. Stronger evidence + would land that test on `main` first. +9. **No end-to-end cross-product test.** `use-can.test.ts` and `callable-auth.test.ts` each pin + independence at their own layer; nothing tests a `create:MemberLogin`-only holder seeing the invite + button and an empty cargo list simultaneously. Deliberate omission. diff --git a/docs/specs/board-seat-delegation.md b/docs/specs/board-seat-delegation.md new file mode 100644 index 00000000..d9825243 --- /dev/null +++ b/docs/specs/board-seat-delegation.md @@ -0,0 +1,99 @@ +# Board-seat and member-login delegation + +Two explicitly grantable permission codes that let an Admin **temporarily** delegate work that +was previously hardcoded to the `Admin` role, then revoke it. Both are granted per member in the +`/members/$memberId` overrides panel (Admin-only) or per role in the `/permisos` matrix. + +## The two codes + +| Subject | Label in the matrix | Live code | Confers | +|---|---|---|---| +| `BoardSeat` | Asientos de directiva | `update:BoardSeat` | seat a member on **any vacant cargo** — CEL category and power-granting alike | +| `MemberLogin` | Acceso de miembros | `create:MemberLogin` | call `provisionMemberLogin`: create the member's Auth account, link their uid, return the password-reset link | + +They are independent by construction. An Admin can grant emailing without board seating and vice +versa. The other five codes each subject generates (`manage:BoardSeat`, `read:MemberLogin`, …) +are inert: every gate is an exact `hasPerm` code test, never a `canDo` expansion, so `manage:all` +does not satisfy either one. + +## What `update:BoardSeat` does NOT do + +- **It confers nothing on its own.** It only widens the cargo set for an editor who *already* + holds `update:Member`, `create:Member`, or `update:Position`. A member holding only + `update:BoardSeat` still cannot write anything. Granting it alone is a no-op; the UI does not + say so. +- **It does not unseat anyone.** `currentCargoGrantsEmpty()` stays Admin-only, so a delegate + cannot displace a member sitting on a power-granting cargo. Hand-over is an Admin action. + Rationale: the `roles` claim is derived *exclusively* from cargo grants + (`compute-roles.ts`), so clearing every Admin's cargo would strip every Admin claim in the + chapter — and `setUserRoles`, `roles/*` writes and `permissionOverrides` writes are all + Admin-only, making that state unrecoverable outside the Firebase console. +- **It does not reach the `/positions` catalog.** Creating a CEL or JDL cargo, and editing any + cargo's `grants`, `category` or board `title`/`titleFemale`, stay Admin-only + (`boardSurfacingCategory()` and the update-arm pins). The delegate seats members on cargos + that already exist. +- **It does not touch `roleIds` or `permissionOverrides`.** `createPermissionAssignmentSafe()` / + `updatePermissionAssignmentSafe()` are unchanged, so a delegate cannot re-grant the delegation + to themselves or anyone else. + +## Accepted decision — claims-minting delegation + +The chapter owner has explicitly accepted that a delegate may seat a member on a cargo whose +`grants` include `Admin`, and that beacon's trust gate will mint that claim. That is the feature, +not a defect. The acceptance is premised on the delegation being **revocable**, which required +one guard: + +**The trust gate is non-reflexive.** When `assignedBy === member.uid` (a self-assignment), +`resolveTrustedGrants` trusts only `update:BoardSeat` in the assigner's `perms` — never an +`Admin` role in their `roles`. Without this, a delegate who self-seats `Presidente` is minted +`Admin`, and that minted `Admin` then satisfies the very gate that minted it: revoking +`update:BoardSeat` re-fires the trigger, the Admin disjunct passes, and the grants are re-honored +forever. `recomputeAllClaims` runs the same code and would not break the loop either. An Admin +seating *someone else* is unaffected. + +## `create:MemberLogin` — what is actually privileged + +The invite **email** is not. `apps/backstage/src/lib/auth/request-password-reset.ts` is a plain +client-side `sendPasswordResetEmail` that any signed-in user can already call. What the callable +owns is Auth account creation, uid linking (the only path that can write `members.uid` at all, +since the rules forbid it on every client lane) and the initial claim write. + +**A non-Admin caller may only mint a NEW Auth account, or re-provision one already linked to that +same member.** It may never adopt a pre-existing unlinked account. Without that restriction the +code would be an account-takeover primitive rather than an invite helper: `members.email` is +unconstrained by the rules and has no uniqueness check, so a delegate could create a member doc +carrying a sitting Admin's email and have the callable adopt that Admin's account, strip its +claims, link its uid onto the attacker's member doc, and return a password-reset link for it. +An Admin caller keeps the full adoption path — it is the documented recovery op. + +This restriction costs the delegation nothing: a genuinely new member has no Auth account. + +## Operator notes + +1. **Grant `update:BoardSeat` together with a member-editing capability.** On its own it does + nothing. The usual pairing is the `Membresía` role (which carries `manage:Member`) plus the + `update:BoardSeat` override; an org-chart-only delegate wants `update:Position` instead. +2. **The delegate must sign out and back in after being granted or revoked.** All three gates — + `firestore.rules`, `requireAdminOrPerm` and `useCan` — read the `perms` claim off the ID + token, and `auth-store.ts` calls `getIdTokenResult()` without `forceRefresh`. A freshly + granted code is invisible for up to an hour otherwise, and a freshly revoked one keeps + working for up to an hour. +3. **Revocation is not an undo.** Removing the code stops future seating. Members already seated + keep their cargo, and their cargo-derived claims are recomputed on the *next write* to their + member doc — which may be much later. To force it, re-write the member docs or run + `recomputeAllClaims`. +4. **The `PERMISSION_CAP` interaction.** A member whose resolved perms exceed 30 is written + `perms: []` fail-closed, which silently takes `update:BoardSeat` with it. Two more subjects + in the vocabulary make the 30-slot budget marginally tighter. +5. **There is a consistency window.** `firestore.rules` reads the token while beacon reads stored + claims. If a delegate's own perms are dropped (cap breach, or revocation) their cached token + still passes the rules for up to an hour, so a seat write can succeed while + `resolveTrustedGrants` declines to mint the grants. The member is then published on the public + Directiva with no claim — visible, powerless. Re-running the write after the token refreshes + resolves it. + +## Out of scope + +Creating or editing cargo docs; `roleIds` / `permissionOverrides` assignment; unseating a sitting +power-cargo holder; delegating `setUserRoles`, `seedRoles`, `recomputeAllClaims` or +`reseedBuiltInRolePerms`. All stay Admin-role-only. diff --git a/packages/types/src/permission.test.ts b/packages/types/src/permission.test.ts index e6ac2346..b6183c6a 100644 --- a/packages/types/src/permission.test.ts +++ b/packages/types/src/permission.test.ts @@ -62,6 +62,32 @@ describe("Showcase subject", () => { }); }); +describe("BoardSeat subject", () => { + it("is a known subject", () => { + expect(SUBJECTS).toContain("BoardSeat"); + }); + // Same shape as Showcase: only update:BoardSeat is read (firestore.rules' boardSeatDelegate, + // beacon's resolveTrustedGrants, backstage's canAssignBoardSeat). The siblings gate nothing + // but must stay VALID — the /permisos matrix renders the full grid and the per-member + // override panel offers every code, so an unvalidatable one would fail the write. + it("accepts update:BoardSeat and the inert siblings the matrix will render", () => { + expect(isValidPermissionCode("update:BoardSeat")).toBe(true); + expect(isValidPermissionCode("manage:BoardSeat")).toBe(true); + expect(isValidPermissionCode("read:BoardSeat")).toBe(true); + }); +}); + +describe("MemberLogin subject", () => { + it("is a known subject", () => { + expect(SUBJECTS).toContain("MemberLogin"); + }); + it("accepts create:MemberLogin and the inert siblings the matrix will render", () => { + expect(isValidPermissionCode("create:MemberLogin")).toBe(true); + expect(isValidPermissionCode("manage:MemberLogin")).toBe(true); + expect(isValidPermissionCode("read:MemberLogin")).toBe(true); + }); +}); + describe("Notification subject", () => { it("is a known subject", () => { expect(SUBJECTS).toContain("Notification"); diff --git a/packages/types/src/permission.ts b/packages/types/src/permission.ts index b691741d..71f2ba40 100644 --- a/packages/types/src/permission.ts +++ b/packages/types/src/permission.ts @@ -27,6 +27,21 @@ export const SUBJECTS = [ // the /permisos matrix renders the full actions × subjects grid, so `checkIn:Member` and // dozens like it are already assignable and equally inert. "Showcase", + // Delegable board seating. Only `update:BoardSeat` is live: firestore.rules' + // boardSeatDelegate() is `hasAnyRole(['Admin']) || hasPerm('update:BoardSeat')` — an EXACT + // code match, not canDo(), so `manage:all` cannot satisfy it and the other five codes are + // inert. It confers no authority ALONE: it only widens which cargo an editor who already + // holds update:Member / create:Member / update:Position may assign, from "grant-free and + // non-CEL" to "any vacant cargo". Displacing a sitting power-cargo holder stays Admin-only + // (currentCargoGrantsEmpty), and the /positions CATALOG stays Admin-only. + "BoardSeat", + // Delegable login provisioning. Only `create:MemberLogin` is live, read by beacon's + // requireAdminOrPerm on provisionMemberLogin. NOT the invite email itself — that is a + // client-side sendPasswordResetEmail any signed-in user can already call. What this gates is + // Auth account creation + uid linking + the initial claim write. A non-Admin holder may only + // mint a NEW account, never adopt a pre-existing one (see provisionMember's callerIsAdmin + // branch), so it cannot be turned on an existing privileged account. + "MemberLogin", "all", ] as const; export type Subject = (typeof SUBJECTS)[number]; From 8470f78c1c08efe6619e08d6cc8b53f3f02b803a Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 18:34:33 -0400 Subject: [PATCH 02/15] feat(rules): delegate board-seat assignment via update:BoardSeat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit boardSeatDelegate() lifts the NEW-side cargo conjunct so an update:BoardSeat holder may seat a member on a CEL or power-granting cargo. It confers nothing alone — the principal still needs update:Member, create:Member or update:Position to reach an arm at all. The OLD side (currentCargoGrantsEmpty) stays Admin-role-only. computeMemberRoles derives the roles claim exclusively from cargo grants, so a principal able to overwrite a sitting Admin's cargo could strip every Admin claim in the chapter, and setUserRoles / roles/* / permissionOverrides are all Admin-only — the state would be unrecoverable outside the Firebase console. Falsified: neutralizing boardSeatDelegate() to Admin-only turns exactly the three new ALLOW tests red, 481 others green. Co-Authored-By: Claude Opus 5 (1M context) --- firestore.rules | 46 +++++- tests/firestore-rules/rules.test.ts | 222 ++++++++++++++++++++++++++++ 2 files changed, 265 insertions(+), 3 deletions(-) diff --git a/firestore.rules b/firestore.rules index af78c354..297f8170 100644 --- a/firestore.rules +++ b/firestore.rules @@ -203,11 +203,29 @@ service cloud.firestore { // also closes a ride-along: a non-Admin can no longer sneak a power cargo + forged // assignedBy under a different term key in the same write. Comisión power grants are // not loop-checkable here — the beacon claims-sync trust gate is their backstop. + // The two cargo conjuncts are gated SEPARATELY, and the asymmetry is the whole point. + // + // NEW side (cargoAssignableByNonAdmin, the cargo being written IN) — delegable. + // boardSeatDelegate() lifts it, so an update:BoardSeat holder may seat a CEL or + // power-granting cargo. That is the feature. + // + // OLD side (currentCargoGrantsEmpty, the cargo being REPLACED) — Admin role ONLY. + // NOT delegated, deliberately. computeMemberRoles derives the `roles` claim + // EXCLUSIVELY from cargo grants — directly-assigned roleIds feed `perms`, never + // `roles` — so a principal who can overwrite a sitting Admin's cargo can strip + // every Admin claim in the chapter one write at a time. After the last one there is + // no Admin: setUserRoles is requireAdmin, and roles/* plus permissionOverrides + // writes are all hasAnyRole(['Admin']). The chapter would be unrecoverable outside + // the Firebase console. A delegate therefore seats VACANT and grant-free cargos and + // cannot displace a power-cargo holder; hand-over stays an Admin action. + // + // A non-delegate is unchanged: both conjuncts still bind, so the deliberately + // asymmetric grant-free-CEL takedown below still works for an update:Position holder. function positionsAssignmentSafe() { return positionsDelta().hasOnly([currentTermKey()]) && assignedBySelf() - && (hasAnyRole(['Admin']) - || (cargoAssignableByNonAdmin() && currentCargoGrantsEmpty())); + && (boardSeatDelegate() || cargoAssignableByNonAdmin()) + && (hasAnyRole(['Admin']) || currentCargoGrantsEmpty()); } // Create has no prior resource to diff, so it can't use positionsDelta(); it applies the // same self-stamp + Admin-only-cargo gate to any positions it writes. Without this a @@ -221,9 +239,12 @@ service cloud.firestore { // Admin provisionMemberLogin then writes the uid, after which projectBoard publishes an // attacker-composed account at board rank 0 as Presidente. currentCargoGrantsEmpty() is // the only half a create cannot ask (no prior resource), and it has no old side to guard. + // Plain substitution, unlike positionsAssignmentSafe(): a create has no prior resource, + // so there is no old side to keep Admin-only — currentCargoGrantsEmpty() has never + // applied here, and a member born on a cargo displaces nobody. function createPositionsSafe() { return !('positions' in request.resource.data) - || (assignedBySelf() && (hasAnyRole(['Admin']) || cargoAssignableByNonAdmin())); + || (assignedBySelf() && (boardSeatDelegate() || cargoAssignableByNonAdmin())); } // roleIds + permissionOverrides feed Auth custom claims via the beacon trigger. @@ -323,6 +344,25 @@ service cloud.firestore { function canCurateFeatured() { return hasAnyRole(['Admin']) || hasPerm('update:Showcase'); } + // Who may seat a member on a cargo the non-Admin lane otherwise refuses — a + // power-granting one, or a CEL one. Split the same way as canCurateFeatured(), for the + // same two reasons: Admin by ROLE (locked and undeactivatable, so its name carries none + // of the staleness this fixes), everyone else by the exact PERM, so revoking the code + // revokes the authority where a surviving role NAME in the claim would not. + // + // hasPerm, deliberately NOT canDo: manage:all must not answer this, and the other five + // *:BoardSeat codes the cross-product generates stay inert BECAUSE the gate is exact. + // + // ACCEPTED, per docs/specs/board-seat-delegation.md: a delegate may seat a cargo whose + // grants confer Admin, and beacon's claims-sync mints it. What that acceptance is + // premised on — revocability — needs beacon's trust gate to be non-reflexive on a + // SELF-assignment, or the minted Admin satisfies the gate that minted it and the + // delegation can never be revoked. See resolveTrustedGrants in apps/beacon. + // + // Deliberately NOT widened to currentCargoGrantsEmpty(): see positionsAssignmentSafe(). + function boardSeatDelegate() { + return hasAnyRole(['Admin']) || hasPerm('update:BoardSeat'); + } function initiativeCreateAllowed(subject) { return canDo('create', subject) && request.resource.data.get('directionUids', []) == [] diff --git a/tests/firestore-rules/rules.test.ts b/tests/firestore-rules/rules.test.ts index 035f981d..c747c580 100644 --- a/tests/firestore-rules/rules.test.ts +++ b/tests/firestore-rules/rules.test.ts @@ -47,6 +47,21 @@ function anon() { const ORG_CHART = "orgchart-uid"; const orgChart = () => as(ORG_CHART, [], ["update:Position"]); +/** The board-seat delegate: an org-chart editor who ALSO holds update:BoardSeat, so + * boardSeatDelegate() lifts the new-side cargo conjunct for them. `update:Position` is + * load-bearing, not decoration — without an entry capability the delegate never reaches the + * arm at all and every ALLOW below would pass for the wrong reason. */ +const SEAT_DELEGATE = "seatdelegate-uid"; +const seatDelegate = () => as(SEAT_DELEGATE, [], ["update:Position", "update:BoardSeat"]); + +/** update:BoardSeat and NOTHING else. The non-vacuity pin for the whole feature: the code + * widens which cargo an editor may assign, it does not make anyone an editor. */ +const plainDelegate = () => as("seatonly-uid", [], ["update:BoardSeat"]); + +/** A delegate on the CREATE lane. create:Member is the entry capability there. */ +const createDelegate = () => as("createdelegate-uid", [], ["create:Member", "update:BoardSeat"]); +const createOnly = () => as("createonly-uid", [], ["create:Member"]); + const MEMBER_DOC = { name: "Ana", totalPoints: 0, uid: "owner-uid", active: true, deletedAt: null }; /** The birth state every members/positions/allies create arm now requires (B2): born @@ -587,6 +602,43 @@ beforeAll(async () => { active: true, deletedAt: null, }); + // Board-seat delegation targets. Each assertion that SUCCEEDS needs its own doc: the + // suite seeds once and never resets, so a shared target would carry the previous test's + // cargo into the next one's currentCargoGrantsEmpty() check. + await setDoc(doc(db, "members/m_delegate"), { + name: "Delegado", + totalPoints: 0, + uid: "delegate-target-uid", + active: true, + deletedAt: null, + }); + await setDoc(doc(db, "members/m_delegate_cel"), { + name: "Delegado CEL", + totalPoints: 0, + uid: "delegate-cel-uid", + active: true, + deletedAt: null, + }); + // Seated on a POWER cargo, for the G3 pin: a delegate must NOT be able to displace this. + await setDoc(doc(db, "members/m_delegate_power"), { + name: "Delegado Poder", + totalPoints: 0, + uid: "delegate-power-uid", + active: true, + deletedAt: null, + positions: { [TERM]: { cargoId: "pos1", comisionIds: [], assignedBy: "admin-uid" } }, + }); + // Seated on a GRANT-FREE CEL cargo, for the takedown pin. Seeded directly rather than + // produced by an earlier test, so currentCargoGrantsEmpty() actually evaluates its get() + // branch instead of short-circuiting on `prior == null` and passing vacuously. + await setDoc(doc(db, "members/m_cel_takedown"), { + name: "Takedown", + totalPoints: 0, + uid: "cel-takedown-uid", + active: true, + deletedAt: null, + positions: { [TERM]: { cargoId: "pos_cel_free", comisionIds: [], assignedBy: "admin-uid" } }, + }); // A member whose POWER cargo sits under a PRIOR term key, leaving the CURRENT term slot // empty. Pins the term-rollover residual (docs/specs/position-assignment-lane.md, // "Residual: the term-rollover window"): currentCargoGrantsEmpty() reads only @@ -1039,6 +1091,56 @@ describe("firestore.rules — members", () => { }), ); }); + it("allows an update:BoardSeat delegate to CREATE a member on a CEL or power cargo", async () => { + // createPositionsSafe() takes the plain substitution — a create has no prior resource, so + // there is no old side to keep Admin-only and a member born on a cargo displaces nobody. + await assertSucceeds( + setDoc(doc(createDelegate(), "members/new_delegate_cel"), { + name: "Ximena Paz", + totalPoints: 0, + ...BORN_LIVE, + positions: { + [TERM]: { cargoId: "pos_cel_free", comisionIds: [], assignedBy: "createdelegate-uid" }, + }, + }), + ); + await assertSucceeds( + setDoc(doc(createDelegate(), "members/new_delegate_pow"), { + name: "Ximena Paz", + totalPoints: 0, + ...BORN_LIVE, + positions: { + [TERM]: { cargoId: "pos1", comisionIds: [], assignedBy: "createdelegate-uid" }, + }, + }), + ); + }); + + it("BLOCKING: the same create principal WITHOUT update:BoardSeat is still denied both", async () => { + // The paired denial — otherwise the two ALLOWs above would pass for any create:Member + // holder and prove nothing about the new disjunct. + await assertFails( + setDoc(doc(createOnly(), "members/new_createonly_cel"), { + name: "Ximena Paz", + totalPoints: 0, + ...BORN_LIVE, + positions: { + [TERM]: { cargoId: "pos_cel_free", comisionIds: [], assignedBy: "createonly-uid" }, + }, + }), + ); + await assertFails( + setDoc(doc(createOnly(), "members/new_createonly_pow"), { + name: "Ximena Paz", + totalPoints: 0, + ...BORN_LIVE, + positions: { + [TERM]: { cargoId: "pos1", comisionIds: [], assignedBy: "createonly-uid" }, + }, + }), + ); + }); + it("allows Admin creating a member on a grant-free CEL cargo (the authority, not the delegate)", async () => { // The paired ALLOW: the CEL conjunct lives inside the non-Admin branch of the create arm // too, so seating the CEL at create stays possible — for an Admin only. Without this, @@ -2372,6 +2474,30 @@ describe("firestore.rules — positions", () => { setDoc(doc(as("admin-uid", ["Admin"]), "positions/mint_cel_admin"), boardCargo("CEL")), ); }); + it("BLOCKING: update:BoardSeat does NOT reach the positions CATALOG", async () => { + // The delegation is a SEATING authority, never an authoring one. It lifts the cargo + // conjunct on members/{id}.positions and nothing else — boardSurfacingCategory() on + // create, and the grants/category/title pins on update, all stay hasAnyRole(['Admin']). + // Without that boundary the delegation would be self-serving: mint a grant-free CEL + // 'Presidente', then seat yourself on it, landing at public board rank 0. + // The principal deliberately holds the CATALOG capabilities too, so each denial is the + // Admin-only pin firing and not a missing create:Position / update:Position. + const catalogDelegate = () => + as("catalogdelegate-uid", [], ["create:Position", "update:Position", "update:BoardSeat"]); + await assertFails( + setDoc(doc(catalogDelegate(), "positions/mint_cel_delegate"), boardCargo("CEL")), + ); + await assertFails( + setDoc(doc(catalogDelegate(), "positions/mint_jdl_delegate"), boardCargo("JDL")), + ); + await assertFails( + updateDoc(doc(catalogDelegate(), "positions/pos_payload"), { grants: ["Admin"] }), + ); + await assertFails(updateDoc(doc(catalogDelegate(), "positions/pos_cat"), { category: "CEL" })); + await assertFails( + updateDoc(doc(catalogDelegate(), "positions/pos_payload"), { title: "Presidente" }), + ); + }); // Deliberate fail-closed: a legacy power comisión (possible before the // invariant) is client-unwritable — even soft-delete — until an admin-SDK/ // console repair empties its grants. Documented in the design spec. @@ -3006,6 +3132,102 @@ describe("firestore.rules — member positions assignment", () => { ); }); + // --- update:BoardSeat delegation (docs/specs/board-seat-delegation.md) --- + + it("allows an update:BoardSeat delegate to assign a GRANT-FREE CEL cargo", async () => { + // The exact write orgChart() is denied above, same cargo, same lane, differing only by + // the delegate's update:BoardSeat. That pairing is what proves the new disjunct is what + // opened it, rather than the CEL conjunct having quietly disappeared. + await assertSucceeds( + updateDoc(doc(seatDelegate(), "members/m_delegate_cel"), { + [`positions.${TERM}`]: { + cargoId: "pos_cel_free", + comisionIds: [], + assignedBy: SEAT_DELEGATE, + }, + }), + ); + }); + + it("allows an update:BoardSeat delegate to assign a POWER-conferring cargo", async () => { + // The accepted claims-minting delegation: pos1 confers grants, and beacon's claims-sync + // will mint them because the delegate holds update:BoardSeat. Owner-accepted; the + // revocability that acceptance rests on is enforced in beacon (non-reflexive trust gate), + // not here. + await assertSucceeds( + updateDoc(doc(seatDelegate(), "members/m_delegate"), { + [`positions.${TERM}`]: { cargoId: "pos1", comisionIds: [], assignedBy: SEAT_DELEGATE }, + }), + ); + }); + + it("G3 BLOCKING: denies a delegate DISPLACING a member who already holds a power cargo", async () => { + // currentCargoGrantsEmpty() is deliberately NOT delegated. computeMemberRoles derives the + // `roles` claim exclusively from cargo grants, so a principal who can overwrite a sitting + // Admin's cargo can strip every Admin claim in the chapter one write at a time — and + // setUserRoles, roles/* and permissionOverrides are all Admin-role-only, so the result is + // unrecoverable outside the Firebase console. If this goes green, the chapter is one + // delegate away from having no Admin. + await assertFails( + updateDoc(doc(seatDelegate(), "members/m_delegate_power"), { + [`positions.${TERM}`]: { cargoId: "pos_soft", comisionIds: [], assignedBy: SEAT_DELEGATE }, + }), + ); + // Clearing it is the same authority and equally denied — the guard is about the cargo + // being REPLACED, not about what replaces it. + await assertFails( + updateDoc(doc(seatDelegate(), "members/m_delegate_power"), { + [`positions.${TERM}`]: { cargoId: null, comisionIds: [], assignedBy: SEAT_DELEGATE }, + }), + ); + }); + + it("BLOCKING: denies update:BoardSeat ALONE any positions write", async () => { + // Non-vacuity pin for the whole feature. The code widens WHICH cargo an editor may + // assign; it does not make anyone an editor. Without update:Position / update:Member the + // principal never reaches an arm — which is also why every ALLOW above pairs it with one. + await assertFails( + updateDoc(doc(plainDelegate(), "members/m_positions"), { + [`positions.${TERM}`]: { cargoId: "pos_soft", comisionIds: [], assignedBy: "seatonly-uid" }, + }), + ); + }); + + it("denies a delegate a forged assignedBy, a past term, and a ride-along field", async () => { + // assignedBySelf(), the current-term restriction and hasOnly(['positions']) all sit + // OUTSIDE the substituted disjunction. Pin that the delegation did not loosen them — + // a forged assignedBy is what the beacon trust gate reads to decide whether to mint. + await assertFails( + updateDoc(doc(seatDelegate(), "members/m_delegate"), { + [`positions.${TERM}`]: { cargoId: "pos_soft", comisionIds: [], assignedBy: "admin-uid" }, + }), + ); + await assertFails( + updateDoc(doc(seatDelegate(), "members/m_delegate"), { + "positions.2099": { cargoId: "pos1", comisionIds: [], assignedBy: SEAT_DELEGATE }, + }), + ); + await assertFails( + updateDoc(doc(seatDelegate(), "members/m_delegate"), { + [`positions.${TERM}`]: { cargoId: "pos_soft", comisionIds: [], assignedBy: SEAT_DELEGATE }, + name: "Renombrado", + }), + ); + }); + + it("REGRESSION: a non-delegate may still CLEAR a member off a grant-free CEL seat", async () => { + // The deliberate asymmetry (firestore.rules' currentCargoGrantsEmpty comment): keeping a + // grant-free CEL seat is denied to a non-Admin, but clearing one is allowed, or a takedown + // would be stranded behind an Admin. This is the guard on the expression the delegation + // touches, and m_cel_takedown is SEEDED holding pos_cel_free so currentCargoGrantsEmpty() + // reaches its get() instead of short-circuiting on a null prior. + await assertSucceeds( + updateDoc(doc(orgChart(), "members/m_cel_takedown"), { + [`positions.${TERM}`]: { cargoId: null, comisionIds: [], assignedBy: ORG_CHART }, + }), + ); + }); + // LAST in this block on purpose: the suite seeds once and never resets, and this write // leaves members/m1 holding a power cargo. A Membership success case running after it // would be denied by currentCargoGrantsEmpty() — the C1 guard — not by its own subject. From 19546b32f8cab7c9a4fdac98891d411fc42f4c85 Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 18:39:29 -0400 Subject: [PATCH 03/15] feat(beacon): honor update:BoardSeat assigners and delegate provisionMemberLogin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trust gate: resolveTrustedGrants now honors a cargo's grants when the assigner holds the Admin role OR the exact update:BoardSeat perm. Without the second disjunct a delegate's seat would publish on the public Directiva and mint nothing, since the rules make them stamp their own uid into assignedBy. The gate is NON-REFLEXIVE on a self-assignment: when the assigner is the member being seated, only the perm is trusted. Otherwise a delegate self-seats Presidente, gets minted Admin, and that minted Admin satisfies the gate that minted it — revoking the perm would re-fire the trigger and re-honor the grants forever. recomputeAllClaims runs the same code and would not break the loop. provisionMemberLogin moves to requireAdminOrPerm(create:MemberLogin), with a hard adoption guard: a non-Admin caller may only mint a brand-new Auth account or re-provision one already linked to that member. members.email is not constrained by firestore.rules and has no uniqueness check, so without the guard a create:Member + create:MemberLogin holder could file a member doc carrying an Admin's email and have the callable strip that Admin's claims, bind their uid via the admin SDK, and return a password-reset link for their mailbox. callerIsAdmin defaults to false so a new call site opts into adoption. callable-auth gains its first direct unit tests, incl. the manage:all and manage: wildcards failing the exact-code gate. Co-Authored-By: Claude Opus 5 (1M context) --- apps/beacon/src/callable-auth.test.ts | 121 ++++++++++++++++++ apps/beacon/src/callable-auth.ts | 41 +++++- apps/beacon/src/claims-sync/firestore-deps.ts | 7 +- apps/beacon/src/claims-sync/sync.test.ts | 107 +++++++++++++++- apps/beacon/src/claims-sync/sync.ts | 50 ++++++-- .../beacon/src/provision-member-login.test.ts | 75 ++++++++++- apps/beacon/src/provision-member-login.ts | 42 +++++- 7 files changed, 414 insertions(+), 29 deletions(-) create mode 100644 apps/beacon/src/callable-auth.test.ts diff --git a/apps/beacon/src/callable-auth.test.ts b/apps/beacon/src/callable-auth.test.ts new file mode 100644 index 00000000..9a9357e0 --- /dev/null +++ b/apps/beacon/src/callable-auth.test.ts @@ -0,0 +1,121 @@ +import { describe, expect, it } from "vitest"; +import { HttpsError, type CallableRequest } from "firebase-functions/v2/https"; +import { callerIsAdmin, requireAdmin, requireAdminOrPerm } from "./callable-auth.js"; + +/** A callable request carrying just the claim shape the gates read. The cast is test-only + * and justified: CallableRequest carries rawRequest/acceptsStreaming/etc. that no gate + * touches, and building them would assert nothing. */ +function req(token?: Record): CallableRequest { + return (token === undefined ? {} : { auth: { uid: "u", token } }) as unknown as CallableRequest; +} + +function codeOf(fn: () => void): string { + try { + fn(); + } catch (err) { + return err instanceof HttpsError ? err.code : "not-an-https-error"; + } + return "no-throw"; +} + +describe("requireAdmin", () => { + it("rejects an unauthenticated caller", () => { + expect(codeOf(() => requireAdmin(req()))).toBe("unauthenticated"); + }); + it("rejects a signed-in non-Admin", () => { + expect(codeOf(() => requireAdmin(req({ roles: ["Member"] })))).toBe("permission-denied"); + }); + it("accepts an Admin", () => { + expect(codeOf(() => requireAdmin(req({ roles: ["Admin", "Member"] })))).toBe("no-throw"); + }); + it("rejects a manage:all perm holder who is not Admin by role", () => { + // requireAdmin is a ROLE gate; the wildcard perm has never satisfied it and must not + // start now that a sibling gate reads perms from the same token. + expect(codeOf(() => requireAdmin(req({ roles: ["Member"], perms: ["manage:all"] })))).toBe( + "permission-denied", + ); + }); +}); + +describe("requireAdminOrPerm", () => { + it("rejects an unauthenticated caller", () => { + expect(codeOf(() => requireAdminOrPerm(req(), "create:MemberLogin"))).toBe("unauthenticated"); + }); + + it("accepts an Admin carrying no perms claim at all", () => { + // The role disjunct must stand alone — an Admin whose perms claim has not been minted + // yet (or was fail-closed to empty by the cap) still passes. + expect(codeOf(() => requireAdminOrPerm(req({ roles: ["Admin"] }), "create:MemberLogin"))).toBe( + "no-throw", + ); + }); + + it("accepts a non-Admin holding the exact code", () => { + expect( + codeOf(() => + requireAdminOrPerm( + req({ roles: ["Member"], perms: ["create:MemberLogin"] }), + "create:MemberLogin", + ), + ), + ).toBe("no-throw"); + }); + + it("BLOCKING: manage:all does NOT satisfy it", () => { + // Exact-code, mirroring firestore.rules' hasPerm(). A canDo-style expansion here would + // hand the delegation to every wildcard holder silently. + expect( + codeOf(() => + requireAdminOrPerm(req({ roles: ["Member"], perms: ["manage:all"] }), "create:MemberLogin"), + ), + ).toBe("permission-denied"); + }); + + it("BLOCKING: manage:MemberLogin does NOT satisfy create:MemberLogin", () => { + // The subject wildcard is equally inert — the gate is the literal code, not canDo(). + expect( + codeOf(() => + requireAdminOrPerm( + req({ roles: ["Member"], perms: ["manage:MemberLogin"] }), + "create:MemberLogin", + ), + ), + ).toBe("permission-denied"); + }); + + it("keeps the two delegations independent", () => { + // A board-seat delegate is not a login provisioner and vice versa. Pinned because both + // codes ship together and the obvious future mistake is to conflate them. + expect( + codeOf(() => + requireAdminOrPerm( + req({ roles: ["Member"], perms: ["update:BoardSeat"] }), + "create:MemberLogin", + ), + ), + ).toBe("permission-denied"); + }); + + it("fails closed on a malformed perms claim", () => { + // A string (or anything non-array) reads as empty rather than throwing — a malformed + // token must deny, not 500. + expect( + codeOf(() => + requireAdminOrPerm( + req({ roles: ["Member"], perms: "create:MemberLogin" }), + "create:MemberLogin", + ), + ), + ).toBe("permission-denied"); + }); +}); + +describe("callerIsAdmin", () => { + it("is false for an unauthenticated caller and for a wildcard perm holder", () => { + expect(callerIsAdmin(req())).toBe(false); + expect(callerIsAdmin(req({ roles: ["Member"], perms: ["manage:all"] }))).toBe(false); + }); + it("is true only for the Admin role", () => { + expect(callerIsAdmin(req({ roles: ["Admin"] }))).toBe(true); + }); +}); diff --git a/apps/beacon/src/callable-auth.ts b/apps/beacon/src/callable-auth.ts index d84932f4..1c87adf1 100644 --- a/apps/beacon/src/callable-auth.ts +++ b/apps/beacon/src/callable-auth.ts @@ -1,10 +1,23 @@ import { HttpsError, type CallableRequest } from "firebase-functions/v2/https"; +import type { PermissionCode } from "@luminova/types"; -function callerRoles(request: CallableRequest): string[] { - const token = request.auth?.token as { roles?: unknown } | undefined; - return Array.isArray(token?.roles) - ? (token.roles as unknown[]).filter((role): role is string => typeof role === "string") - : []; +/** One reader for both string-array claims. `roles` and `perms` are read identically and + * had drifted into two copies of the same three lines the moment a second gate needed one. + * + * The `as` narrows `DecodedIdToken`'s `[key: string]: any` index signature to `unknown`, + * which is a tightening — every value is still filtered before use. Deliberately NOT + * `permsFromClaims` from claims-sync: that returns `PermissionCode[] | undefined` because + * `getExistingClaims` needs absence and empty to differ for its claim diff, and importing + * it would pull the Firestore port's runtime graph (chunk, role-doc, resolve-member-perms) + * into the callable trust boundary for a membership test. */ +function stringArrayClaim(request: CallableRequest, key: "roles" | "perms"): string[] { + const token = request.auth?.token as Record | undefined; + const raw = token?.[key]; + return Array.isArray(raw) ? raw.filter((v): v is string => typeof v === "string") : []; +} + +export function callerIsAdmin(request: CallableRequest): boolean { + return stringArrayClaim(request, "roles").includes("Admin"); } /** Reject anyone who isn't a signed-in Admin. Shared by every admin-only callable. */ @@ -12,7 +25,23 @@ export function requireAdmin(request: CallableRequest): void { if (!request.auth) { throw new HttpsError("unauthenticated", "sign-in required"); } - if (!callerRoles(request).includes("Admin")) { + if (!callerIsAdmin(request)) { throw new HttpsError("permission-denied", "Admin role required"); } } + +/** Admin by ROLE, or the exact permission code — the callable-side mirror of + * firestore.rules' `hasAnyRole(['Admin']) || hasPerm(code)`. + * + * Exact-code, deliberately not a `canDo`-style expansion: `manage:all` must not satisfy a + * delegation gate, or every wildcard holder silently becomes a delegate. Same discipline as + * the rules' `hasPerm()` and backstage's `hasPerm`. A malformed `perms` claim (non-array, + * string, absent) reads as empty and therefore denies. */ +export function requireAdminOrPerm(request: CallableRequest, code: PermissionCode): void { + if (!request.auth) { + throw new HttpsError("unauthenticated", "sign-in required"); + } + if (callerIsAdmin(request)) return; + if (stringArrayClaim(request, "perms").includes(code)) return; + throw new HttpsError("permission-denied", `Admin role or ${code} required`); +} diff --git a/apps/beacon/src/claims-sync/firestore-deps.ts b/apps/beacon/src/claims-sync/firestore-deps.ts index 2fa90a1f..0883df40 100644 --- a/apps/beacon/src/claims-sync/firestore-deps.ts +++ b/apps/beacon/src/claims-sync/firestore-deps.ts @@ -284,9 +284,12 @@ export function firestoreClaimsDeps(db: Firestore, auth: Auth): FirestoreClaimsD const grants = (snap.data()?.grants ?? []) as unknown[]; return { grants: grants.filter((g): g is Role => isValidRole(g)) }; }, - getUserRoles: async (uid) => { + getAssignerClaims: async (uid) => { + // Same per-instance loadUser memo getExistingClaims uses — reading perms alongside + // roles costs no extra Auth read. const user = await loadUser(uid); - return user ? rolesFromClaims(user.customClaims as Record | undefined) : []; + const claims = user?.customClaims as Record | undefined; + return { roles: rolesFromClaims(claims), perms: permsFromClaims(claims) ?? [] }; }, getExistingClaims: async (uid) => { const user = await loadUser(uid); diff --git a/apps/beacon/src/claims-sync/sync.test.ts b/apps/beacon/src/claims-sync/sync.test.ts index 48fd7220..c30cdbf2 100644 --- a/apps/beacon/src/claims-sync/sync.test.ts +++ b/apps/beacon/src/claims-sync/sync.test.ts @@ -34,6 +34,9 @@ const customRole = (id: string, permissions: PermissionCode[]): RoleDefinition = function fakeDeps(opts: { positions: Record; userRoles: Record; + /** The assigner's `perms` claim — the second trust source alongside the Admin role, and + * the ONLY one honored on a self-assignment (see resolveTrustedGrants). */ + userPerms?: Record; existing: Record; builtInDocs?: RoleDefinition[]; customRoles?: Record; @@ -42,7 +45,10 @@ function fakeDeps(opts: { const writes: Record = {}; const deps: ClaimsSyncDeps = { getPosition: async (id) => opts.positions[id] ?? null, - getUserRoles: async (uid) => opts.userRoles[uid] ?? [], + getAssignerClaims: async (uid) => ({ + roles: opts.userRoles[uid] ?? [], + perms: opts.userPerms?.[uid] ?? [], + }), getExistingClaims: async (uid) => opts.existing[uid] ?? { roles: [] }, // COVERAGE is preserved (no liveness filter — a deactivated built-in must still reach // resolveMemberPerms so it COVERS its key), but `live` is COMPUTED with the production @@ -126,6 +132,99 @@ describe("syncMemberClaims", () => { }); }); + it("honors power grants when the assigner holds update:BoardSeat and NOT the Admin role", async () => { + // The mirror of the Admin case above, and the reason Slice 3 exists: a delegate stamps + // their own uid into assignedBy, so without this disjunct the seat would land and mint + // nothing. + const { deps, writes } = fakeDeps({ + positions: { "pos-pres": { grants: ["Admin"] } }, + userRoles: { "delegate-uid": ["Member"] }, + userPerms: { "delegate-uid": ["update:BoardSeat"] }, + existing: { "target-uid": { roles: ["Member"] } }, + }); + await syncMemberClaims( + deps, + { + uid: "target-uid", + positions: { "2026": { cargoId: "pos-pres", comisionIds: [], assignedBy: "delegate-uid" } }, + }, + "2026", + ); + expect(writes["target-uid"]).toEqual({ + roles: ["Admin", "Member"], + perms: permsFor(["Admin", "Member"]), + }); + }); + + it("BLOCKING: manage:all does NOT satisfy the trust gate", async () => { + // The gate is an exact code test, matching firestore.rules' hasPerm(). The CASL wildcard + // must not answer it on the server any more than it does in the client gate — otherwise + // any manage:all holder silently becomes a seat delegate. + const { deps, writes } = fakeDeps({ + positions: { "pos-pres": { grants: ["Admin"] } }, + userRoles: { "wildcard-uid": ["Member"] }, + userPerms: { "wildcard-uid": ["manage:all"] }, + existing: { "target-uid": { roles: ["Member"], perms: permsFor(["Member"]) } }, + }); + await syncMemberClaims( + deps, + { + uid: "target-uid", + positions: { "2026": { cargoId: "pos-pres", comisionIds: [], assignedBy: "wildcard-uid" } }, + }, + "2026", + ); + expect(writes["target-uid"]).toBeUndefined(); + }); + + it("BLOCKING: a SELF-assignment is non-reflexive — a cargo-derived Admin cannot re-trust itself", async () => { + // The guard the whole delegation rests on. Without it: a delegate self-seats Presidente, + // this function mints them Admin, and the minted Admin then satisfies the gate that + // minted it — so revoking update:BoardSeat re-fires the trigger, finds the Admin role, + // and re-honors the grants forever. The delegation would be permanent. + // Fixture is the post-revocation state exactly: assigner IS the target, holds Admin by + // role (from the cargo), and no longer holds the perm. + const { deps, writes } = fakeDeps({ + positions: { "pos-pres": { grants: ["Admin"] } }, + userRoles: { "self-uid": ["Admin", "Member"] }, + userPerms: { "self-uid": [] }, + existing: { "self-uid": { roles: ["Admin", "Member"] } }, + }); + await syncMemberClaims( + deps, + { + uid: "self-uid", + positions: { "2026": { cargoId: "pos-pres", comisionIds: [], assignedBy: "self-uid" } }, + }, + "2026", + ); + // De-elevated to a plain Member: revocation actually revokes. + expect(writes["self-uid"]).toEqual({ roles: ["Member"], perms: permsFor(["Member"]) }); + }); + + it("still honors a SELF-assignment backed by the perm itself", async () => { + // The paired ALLOW — otherwise the test above would pass for a rule that simply refused + // every self-assignment, which would break an Admin seating their own cargo. + const { deps, writes } = fakeDeps({ + positions: { "pos-pres": { grants: ["Admin"] } }, + userRoles: { "self-uid": ["Member"] }, + userPerms: { "self-uid": ["update:BoardSeat"] }, + existing: { "self-uid": { roles: ["Member"] } }, + }); + await syncMemberClaims( + deps, + { + uid: "self-uid", + positions: { "2026": { cargoId: "pos-pres", comisionIds: [], assignedBy: "self-uid" } }, + }, + "2026", + ); + expect(writes["self-uid"]).toEqual({ + roles: ["Admin", "Member"], + perms: permsFor(["Admin", "Member"]), + }); + }); + it("BLOCKING: positive-and-inert — a grant-free cargo from a NON-Admin assigner mints nothing", async () => { // The members-positions lane (firestore.rules' fourth members update arm, keyed on // update:Position) lets an org-chart editor who is NOT an Admin assign GRANT-FREE cargos. @@ -153,9 +252,9 @@ describe("syncMemberClaims", () => { }); const spied: ClaimsSyncDeps = { ...deps, - getUserRoles: async (uid) => { + getAssignerClaims: async (uid) => { assignerLookups.push(uid); - return deps.getUserRoles(uid); + return deps.getAssignerClaims(uid); }, }; await syncMemberClaims( @@ -536,7 +635,7 @@ describe("syncMemberClaims", () => { const writes: Record = {}; const deps: ClaimsSyncDeps = { getPosition: async () => ({ grants: ["Admin"] }), - getUserRoles: async () => { + getAssignerClaims: async () => { throw new Error("auth lookup failed"); }, getExistingClaims: async () => ({ roles: ["Member"] }), diff --git a/apps/beacon/src/claims-sync/sync.ts b/apps/beacon/src/claims-sync/sync.ts index c1177ac4..41b623d6 100644 --- a/apps/beacon/src/claims-sync/sync.ts +++ b/apps/beacon/src/claims-sync/sync.ts @@ -13,8 +13,13 @@ export interface MemberClaims { export interface ClaimsSyncDeps extends RolePermsDeps { /** Catalog position by id, or null if missing/deleted. */ getPosition(id: string): Promise<{ grants: Role[] } | null>; - /** The assigner's current claim roles (for the power-grant trust gate). */ - getUserRoles(uid: string): Promise; + /** The assigner's current claim roles AND perms (for the power-grant trust gate). + * Both, from one call: the gate asks a single question and two accessors would be two + * chances for a later edit to consult one and not the other. Deliberately not folded into + * `getExistingClaims` — the spy in sync.test.ts proves the gate is never REACHED on a + * grant-free cargo by asserting no assigner lookup happened, and a shared accessor would + * degrade that assertion to a uid-filtering heuristic. */ + getAssignerClaims(uid: string): Promise<{ roles: Role[]; perms: PermissionCode[] }>; /** The target member's existing custom claims. */ getExistingClaims(uid: string): Promise<{ roles: Role[]; perms?: PermissionCode[] }>; setClaims(uid: string, claims: MemberClaims): Promise; @@ -43,14 +48,30 @@ type MemberLike = { * claim true; do not re-loosen it without re-reading this comment. * * The assigner lookup runs only when the cargo actually confers power. - * `getUserRoles` reads the assigner's LIVE claims, so a later Firestore write + * `getAssignerClaims` reads the assigner's LIVE claims, so a later Firestore write * that re-invokes this function re-evaluates trust: if the assigner has since * lost Admin, their previously granted power cargo is revoked and claims - * reflect current org state (by design). */ + * reflect current org state (by design). + * + * TWO trust sources, mirroring firestore.rules' boardSeatDelegate(): the Admin ROLE, or + * the exact `update:BoardSeat` PERM. The perm is not optional politeness — a delegate + * stamps their OWN uid into `assignedBy` (the rules' assignedBySelf()), so without it the + * seat lands, the member is published on the world-readable Directiva, and no claim is + * minted. Visible, powerless, and silent: half-working rather than safe. + * + * NON-REFLEXIVE on a self-assignment, and this is the guard the whole delegation rests on. + * When the assigner IS the member being seated, only the PERM is trusted — never the Admin + * role. Otherwise: a delegate self-seats Presidente, this function mints them Admin, and + * from then on the minted Admin satisfies the gate that minted it. Revoking + * `update:BoardSeat` would re-fire this very trigger, find the Admin role, and re-honor the + * grants forever; `recomputeAllClaims` runs the same code and would not break the loop + * either. The delegation would be permanent, which is exactly what its acceptance was + * premised on NOT being. An Admin seating someone ELSE is untouched. */ async function resolveTrustedGrants( deps: ClaimsSyncDeps, cargoId: string | null, assignedBy: string | undefined, + memberUid: string, ): Promise { // FULL screening, not just the empty-string half this used to check. `cargoId` comes // straight off the member doc and every implementation of `getPosition` interpolates it @@ -65,10 +86,13 @@ async function resolveTrustedGrants( if (!isSafeDocId(cargoId)) return []; const position = await deps.getPosition(cargoId); if (!position || position.grants.length === 0) return []; - const assignerIsAdmin = assignedBy - ? (await deps.getUserRoles(assignedBy)).includes("Admin") - : false; - return assignerIsAdmin ? [...new Set(position.grants)] : []; + if (!assignedBy) return []; + const assigner = await deps.getAssignerClaims(assignedBy); + const selfAssigned = assignedBy === memberUid; + const trusted = selfAssigned + ? assigner.perms.includes("update:BoardSeat") + : assigner.roles.includes("Admin") || assigner.perms.includes("update:BoardSeat"); + return trusted ? [...new Set(position.grants)] : []; } function sameList(a: readonly string[], b: readonly string[]): boolean { @@ -96,7 +120,12 @@ export async function syncMemberClaims( ): Promise { if (!member.uid) return; const term = member.positions?.[termKey]; - const trustedGrants = await resolveTrustedGrants(deps, term?.cargoId ?? null, term?.assignedBy); + const trustedGrants = await resolveTrustedGrants( + deps, + term?.cargoId ?? null, + term?.assignedBy, + member.uid, + ); const existing = await deps.getExistingClaims(member.uid); const hadScanner = existing.roles.includes("Scanner"); @@ -113,6 +142,9 @@ export async function syncMemberClaims( // keep a stale grant while dropping a revoke). We still write the recomputed // `roles` + empty `perms` so a concurrent role revocation always lands — // never leave the member on stale, possibly-elevated claims. + // Note for update:BoardSeat holders: this takes their delegation with it, silently. A + // delegate over the cap keeps seating (their cached token still passes the rules) while + // this function stops honoring the grants — the seat publishes, no claim is minted. deps.logError?.("effective perms exceed cap; writing empty perms (fail-closed)", { uid: member.uid, count: perms.length, diff --git a/apps/beacon/src/provision-member-login.test.ts b/apps/beacon/src/provision-member-login.test.ts index 05dc2ef2..ef6c82ca 100644 --- a/apps/beacon/src/provision-member-login.test.ts +++ b/apps/beacon/src/provision-member-login.test.ts @@ -89,13 +89,18 @@ describe("provisionMember", () => { }); it("self-heals a stale link when the linked account was deleted — adopts by email, de-elevated", async () => { + // ADMIN caller. The self-heal is an adoption too — it binds an account this member was + // never linked to — so it sits behind the same guard, and deliberately: `email` is not + // pinned on the members update arm either, so an update:Member delegate could retarget an + // already-linked member at an Admin's mailbox and reach this branch whenever the stale + // link happens to be dead. Recovery from a deleted account stays an Admin op. const { deps, calls } = fakeDeps({ member: { ...active, uid: "dead-uid" }, usersByEmail: { "a@b.co": { uid: "u2", email: "a@b.co", customClaims: { roles: ["Admin"] } }, }, }); - const result = await provisionMember(deps, "m1"); + const result = await provisionMember(deps, "m1", true); expect(result.email).toBe("a@b.co"); expect(calls.createUser).toEqual([]); expect(calls.linkUid).toEqual(["u2"]); @@ -130,17 +135,74 @@ describe("provisionMember", () => { }); it("reuses an existing auth user for an unlinked member (pre-created account)", async () => { + // ADMIN caller: adoption is the documented recovery op and stays open for the Admin role. + // The `true` is load-bearing — the same call with `false` is the takedown case below. const { deps, calls } = fakeDeps({ member: active, usersByEmail: { "a@b.co": { uid: "u9", email: "a@b.co", customClaims: { roles: ["Scanner"] } }, }, }); - await provisionMember(deps, "m1"); + await provisionMember(deps, "m1", true); expect(calls.createUser).toEqual([]); expect(calls.linkUid).toEqual(["u9"]); }); + it("BLOCKING: a non-Admin caller may NOT adopt a pre-existing unlinked account", async () => { + // The takeover this guard closes: firestore.rules never constrains members.email and + // there is no uniqueness check, so a create:Member + create:MemberLogin delegate could + // file a member doc carrying a sitting Admin's email. Reaching the writes below would + // strip that Admin's claims (adoptedClaims), bind their uid to the attacker's member doc + // through the admin SDK, and hand back a password-reset link for their mailbox. + // Same fixture as "reuses an existing auth user for an unlinked member" one test above — + // the ONLY difference is the caller's privilege. + const { deps, calls } = fakeDeps({ + member: active, + usersByEmail: { + "a@b.co": { uid: "u9", email: "a@b.co", customClaims: { roles: ["Admin"] } }, + }, + }); + await expect(provisionMember(deps, "m1", false)).rejects.toMatchObject({ + code: "permission-denied", + }); + // Nothing partial: no claim write, no uid link, no reset link generated. + expect(calls.setClaims).toEqual([]); + expect(calls.linkUid).toEqual([]); + expect(calls.createUser).toEqual([]); + }); + + it("still lets a non-Admin caller mint a BRAND-NEW account, and resend to its own link", async () => { + // The delegation costs nothing on the path it is actually for: a genuinely new member + // has no Auth account. + const fresh = fakeDeps({ member: active }); + await expect(provisionMember(fresh.deps, "m1", false)).resolves.toEqual({ + email: "a@b.co", + actionLink: "link:a@b.co", + }); + expect(fresh.calls.createUser).toEqual(["a@b.co"]); + // And re-provisioning a member ALREADY linked to that account stays open (resend invite): + // user.uid === linkedUid, so it is not an adoption. + const resend = fakeDeps({ + member: { ...active, uid: "u1" }, + usersByEmail: { "a@b.co": { uid: "u1", email: "a@b.co" } }, + }); + await expect(provisionMember(resend.deps, "m1", false)).resolves.toEqual({ + email: "a@b.co", + actionLink: "link:a@b.co", + }); + expect(resend.calls.createUser).toEqual([]); + expect(resend.calls.linkUid).toEqual(["u1"]); + }); + + it("defaults callerIsAdmin to false — a new call site must opt into adoption", async () => { + // The parameter defaults closed so an added caller that forgets it gets the SAFE path. + const { deps } = fakeDeps({ + member: active, + usersByEmail: { "a@b.co": { uid: "u9", email: "a@b.co" } }, + }); + await expect(provisionMember(deps, "m1")).rejects.toMatchObject({ code: "permission-denied" }); + }); + it("rejects a missing / inactive / email-less member", async () => { await expect(provisionMember(fakeDeps({ member: null }).deps, "m1")).rejects.toMatchObject({ code: "not-found", @@ -154,6 +216,9 @@ describe("provisionMember", () => { }); }); +// Every case here exercises the ADOPTION path, which is Admin-only — hence the explicit +// `true` third argument throughout. A non-Admin caller is refused before any of this runs +// (see "a non-Admin caller may NOT adopt a pre-existing unlinked account"). describe("provisionMember — stale-claims bootstrap (fresh adopt)", () => { it("strips stale org roles when adopting a pre-existing auth account, keeping Scanner", async () => { const claimsWrites: Record[] = []; @@ -173,7 +238,7 @@ describe("provisionMember — stale-claims bootstrap (fresh adopt)", () => { claimsWrites.push(claims); }, }; - await provisionMember(spied, "m1"); + await provisionMember(spied, "m1", true); expect(claimsWrites).toEqual([{ roles: ["Scanner", "Member"] }]); }); @@ -191,7 +256,7 @@ describe("provisionMember — stale-claims bootstrap (fresh adopt)", () => { claimsWrites.push(claims); }, }; - await provisionMember(spied, "m1"); + await provisionMember(spied, "m1", true); expect(claimsWrites).toEqual([{ roles: ["Member"] }]); }); @@ -209,7 +274,7 @@ describe("provisionMember — stale-claims bootstrap (fresh adopt)", () => { claimsWrites.push(claims); }, }; - await provisionMember(spied, "m1"); + await provisionMember(spied, "m1", true); expect(claimsWrites).toEqual([{ roles: ["Admin", "Member"] }]); }); }); diff --git a/apps/beacon/src/provision-member-login.ts b/apps/beacon/src/provision-member-login.ts index 7a602884..959e1023 100644 --- a/apps/beacon/src/provision-member-login.ts +++ b/apps/beacon/src/provision-member-login.ts @@ -2,7 +2,7 @@ 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 { requireAdmin } from "./callable-auth.js"; +import { callerIsAdmin, requireAdminOrPerm } from "./callable-auth.js"; import { firestoreProvisionDeps } from "./provision-deps.js"; import { ensureApp } from "./runtime.js"; @@ -71,6 +71,10 @@ function adoptedClaims(existing: RawClaims | undefined): RawClaims { export async function provisionMember( deps: ProvisionDeps, memberId: string, + /** Whether the CALLER holds the Admin role. A `create:MemberLogin` delegate does not, and + * is confined to the new-account path below — see the adoption guard. Defaults to false: + * a new call site must opt INTO the privileged path, never inherit it by omission. */ + callerIsAdmin = false, ): Promise<{ email: string; actionLink: string }> { const member = await deps.getMember(memberId); if (member === null) throw new HttpsError("not-found", "member not found"); @@ -94,6 +98,30 @@ export async function provisionMember( ); } } + // ADOPTION GUARD — the boundary that makes create:MemberLogin delegable at all. + // + // Adoption is the branch where an Auth account already exists for this email and is not + // the one this member is linked to. For an Admin it is the documented recovery op. For a + // delegate it would be an account-takeover primitive, because NOTHING upstream ties + // members.email to the person: firestore.rules constrains totalPoints, uid, publicProfile, + // name, roleIds and positions on the create arm, never `email`, and no uniqueness check + // exists anywhere. So a create:Member + create:MemberLogin holder could file a member doc + // carrying a sitting Admin's email and reach the three writes below — adoptedClaims() + // stripping that Admin's claims, linkUid() binding the Admin's uid to the attacker's + // member doc through the admin SDK (the only path that can write members.uid at all), and + // passwordResetLink() handing back a reset link for the Admin's mailbox. + // + // A non-Admin therefore gets exactly two shapes: mint a brand-new account, or re-provision + // one already linked to THIS member (resend invite). That costs the delegation nothing — + // a genuinely new member has no Auth account — and is why the guard is a hard refusal + // rather than a claims-only restriction. + if (!callerIsAdmin && user !== null && user.uid !== linkedUid) { + throw new HttpsError( + "permission-denied", + "this email already has a login; only an Admin can link an existing account", + { reason: "adoption-requires-admin" }, + ); + } if (!user) user = await deps.createUser(email); const targetEmail = user.email ?? email; @@ -114,9 +142,17 @@ export async function provisionMember( return { email: targetEmail, actionLink } as const; } +// Delegable per docs/specs/board-seat-delegation.md: an Admin may hand `create:MemberLogin` +// to whoever is enrolling members, then revoke it. What the code gates is Auth account +// creation, uid linking and the initial claim write — NOT the invite email, which is a plain +// client-side sendPasswordResetEmail any signed-in user can already call. export const provisionMemberLogin = onCall(async (request) => { - requireAdmin(request); + requireAdminOrPerm(request, "create:MemberLogin"); const { memberId } = validateProvisionInput(request.data); ensureApp(); - return provisionMember(firestoreProvisionDeps(getFirestore(), getAuth()), memberId); + return provisionMember( + firestoreProvisionDeps(getFirestore(), getAuth()), + memberId, + callerIsAdmin(request), + ); }); From 450219a045fbc814c26cc482545f9fdd66d26c30 Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 18:44:31 -0400 Subject: [PATCH 04/15] feat(backstage): split the seat, catalog and login gates; explain the empty cargo list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit canAssignPowerGrants is replaced by three flags. It had five consumers and one of them — positions-page's PositionForm canEditGrants — governs the /positions CATALOG, not member seating. Widening it in place would have handed a seat delegate the ability to mint a grant-free CEL 'Presidente' and then seat itself on it at public board rank 0. canAssignBoardSeat = Admin role || update:BoardSeat (member create+update lanes) canEditCargoCatalog = Admin role (/positions, unchanged) canProvisionLogin = Admin role || create:MemberLogin (invite affordances) The invite affordances (drawer checkbox, row menu, profile header) move off the Admin role onto canProvisionLogin. All three flags use the exact-code hasPerm, never CASL, so manage:all cannot satisfy them and produce a render-then-403. NoAssignableCargosNote explains a Cargo picker the permission ceiling emptied — previously a bare "Sin resultados" indistinguishable from an empty catalog, which is the permanent state of a chapter whose every cargo carries grants. Co-Authored-By: Claude Opus 5 (1M context) --- .../members/components/member-drawer.tsx | 4 +- .../members/components/member-form.test.tsx | 46 +++++++++++++++ .../members/components/member-form.tsx | 4 ++ .../components/member-invite-drawer.test.tsx | 57 +++++++++++++++++-- .../components/member-invite-drawer.tsx | 26 +++++---- .../components/member-positions-form.test.tsx | 49 ++++++++++++++++ .../components/member-positions-form.tsx | 2 + .../components/member-profile-page.tsx | 9 +-- .../components/member-row-menu.test.tsx | 19 +++++++ .../members/components/member-row-menu.tsx | 7 ++- .../components/no-assignable-cargos-note.tsx | 20 +++++++ .../positions/components/positions-page.tsx | 4 +- apps/backstage/src/lib/authz/use-can.test.ts | 50 ++++++++++++++++ apps/backstage/src/lib/authz/use-can.ts | 35 +++++++++--- 14 files changed, 298 insertions(+), 34 deletions(-) create mode 100644 apps/backstage/src/features/members/components/no-assignable-cargos-note.tsx diff --git a/apps/backstage/src/features/members/components/member-drawer.tsx b/apps/backstage/src/features/members/components/member-drawer.tsx index 5cd39678..4dbbf5bd 100644 --- a/apps/backstage/src/features/members/components/member-drawer.tsx +++ b/apps/backstage/src/features/members/components/member-drawer.tsx @@ -136,7 +136,7 @@ function EditBody({ onSubmit: (data: MemberInput) => Promise; }) { const { onUpload, onRemove } = useMemberPhoto(member.id); - const { canAssignPowerGrants } = useCan(); + const { canAssignBoardSeat } = useCan(); return (
diff --git a/apps/backstage/src/features/members/components/member-form.test.tsx b/apps/backstage/src/features/members/components/member-form.test.tsx index 0fa5bdde..c2252593 100644 --- a/apps/backstage/src/features/members/components/member-form.test.tsx +++ b/apps/backstage/src/features/members/components/member-form.test.tsx @@ -105,6 +105,52 @@ describe("MemberForm", () => { expect(screen.queryByText("Comisión de Eventos")).not.toBeInTheDocument(); }); + // The empty-state the delegation exists to explain. A chapter whose every cargo carries + // grants (which is the real production shape) leaves a non-delegate with zero options, and + // the bare Combobox "Sin resultados" cannot be told apart from an empty catalog. + it("explains an empty cargo list to a non-delegate, and stays silent for a delegate", async () => { + const gatedCargo = ( + id: string, + category: Position["category"], + grants: Position["grants"], + ): Position => ({ + id, + title: id, + titleFemale: id, + category, + grants, + term: null, + sigla: null, + description: "", + active: true, + deletedAt: null, + }); + const celOnly: Position[] = [ + gatedCargo("pos-cel", "CEL", []), + gatedCargo("pos-power", "JDL", ["Membership"]), + ]; + const { unmount } = render( + , + ); + expect(screen.getByRole("note")).toHaveTextContent(/Asientos de directiva/); + await userEvent.click(screen.getByLabelText("Cargo")); + expect(await screen.findByText("Sin resultados")).toBeInTheDocument(); + unmount(); + + // The delegate sees the very same catalog as assignable, and no note. + render( + , + ); + expect(screen.queryByRole("note")).not.toBeInTheDocument(); + await userEvent.click(screen.getByLabelText("Cargo")); + expect(screen.queryByText("Sin resultados")).not.toBeInTheDocument(); + }); + // The admin half of memberSchemaFor: a member enrolled before memberNameValid() existed // must stay editable. Without the per-member schema the form blocks on a name the admin // never touched, making the rules' touched('name') affordance unreachable. diff --git a/apps/backstage/src/features/members/components/member-form.tsx b/apps/backstage/src/features/members/components/member-form.tsx index e6ee820a..a3ad59a6 100644 --- a/apps/backstage/src/features/members/components/member-form.tsx +++ b/apps/backstage/src/features/members/components/member-form.tsx @@ -28,6 +28,7 @@ import { cargoTakedownOnly, positionsLockedForNonAdmin, } from "../lib/assignable-cargo"; +import { NoAssignableCargosNote } from "./no-assignable-cargos-note"; interface MemberFormProps { positions: Position[]; @@ -310,6 +311,9 @@ export function MemberForm({ igual.

)} + {!positionsLocked && !allowPowerGrants && cargoOptions.length === 0 && ( + + )} ( - + {children} ), @@ -142,4 +146,49 @@ describe("MemberInviteDrawer", () => { ); expect(screen.getByRole("button", { name: "Copiar enlace de acceso" })).toBeInTheDocument(); }); + + // --- create:MemberLogin delegation --- + + const drawer = ( + onProvision = vi.fn().mockResolvedValue({ email: "a@b.co", actionLink: "l" }), + ) => ({ + node: ( + {}} + onCreate={async () => "idD"} + onProvision={onProvision} + /> + ), + onProvision, + }); + + it("shows 'Enviar acceso' to a create:MemberLogin delegate, defaulted ON, and provisions", async () => { + const { node, onProvision } = drawer(); + renderWithAbility(node, { roles: ["Member"], perms: ["create:Member", "create:MemberLogin"] }); + const checkbox = screen.getByLabelText("Enviar acceso a la app"); + expect(checkbox).toBeChecked(); + await fill(); + fireEvent.click(screen.getByRole("button", { name: "Enviar invitación" })); + await waitFor(() => expect(onProvision).toHaveBeenCalledWith("idD")); + }); + + it("hides it from a member creator without the code, and never calls onProvision", async () => { + const { node, onProvision } = drawer(); + renderWithAbility(node, { roles: ["Member"], perms: ["create:Member"] }); + expect(screen.queryByLabelText("Enviar acceso a la app")).not.toBeInTheDocument(); + await fill(); + fireEvent.click(screen.getByRole("button", { name: "Enviar invitación" })); + await waitFor(() => expect(screen.queryByRole("alert")).not.toBeInTheDocument()); + expect(onProvision).not.toHaveBeenCalled(); + }); + + it("BLOCKING: hides it from a manage:all perm holder without the Admin role", async () => { + // The render-then-403 this gate exists to stop: beacon's requireAdminOrPerm is an exact + // code test, so the wildcard would fail server-side after the member was already created. + const { node } = drawer(); + renderWithAbility(node, { roles: ["Member"], perms: ["manage:all"] }); + expect(screen.queryByLabelText("Enviar acceso a la app")).not.toBeInTheDocument(); + }); }); diff --git a/apps/backstage/src/features/members/components/member-invite-drawer.tsx b/apps/backstage/src/features/members/components/member-invite-drawer.tsx index e36ac1ba..1bce8c2c 100644 --- a/apps/backstage/src/features/members/components/member-invite-drawer.tsx +++ b/apps/backstage/src/features/members/components/member-invite-drawer.tsx @@ -34,26 +34,28 @@ export function MemberInviteDrawer({ onCreate, onProvision, }: MemberInviteDrawerProps) { - // Provisioning login is Admin-role-only (provisionMemberLogin → requireAdmin). A - // non-Admin may still create the member; they just can't send access here, so hide - // the option and default it off — otherwise the provision step fails silently after - // the member is already created. - const { isAdmin, canAssignPowerGrants } = useCan(); + // Provisioning login is the Admin role OR the create:MemberLogin perm + // (provisionMemberLogin → requireAdminOrPerm). A member creator without either may still + // create the member; they just can't send access here, so hide the option and default it + // off — otherwise the provision step fails silently after the member is already created. + const { canProvisionLogin, canAssignBoardSeat } = useCan(); const [done, setDone] = useState(null); - const [sendAccess, setSendAccess] = useState(isAdmin); + const [sendAccess, setSendAccess] = useState(canProvisionLogin); const [copyState, setCopyState] = useState<"idle" | "copied" | "failed">("idle"); // The drawer mounts with the page, before the auth token's claims decode (the store // emits with empty claims first, then re-emits). Re-sync the default each time it - // OPENS — by then isAdmin is resolved — so an Admin's first invite doesn't silently + // OPENS — by then the flag is resolved — so a provisioner's first invite doesn't silently // default "Enviar acceso" off. Won't clobber a manual toggle (deps stable while open). + // This matters MORE now than it did for a role gate: `perms` is minted by claims-sync and + // lands in the same late token, so a perm-derived flag is false for exactly as long. useEffect(() => { - if (open) setSendAccess(isAdmin); - }, [open, isAdmin]); + if (open) setSendAccess(canProvisionLogin); + }, [open, canProvisionLogin]); const reset = () => { setDone(null); - setSendAccess(isAdmin); + setSendAccess(canProvisionLogin); setCopyState("idle"); }; @@ -177,11 +179,11 @@ export function MemberInviteDrawer({ submitLabel="Enviar invitación" pendingLabel="Enviando…" showPreview - allowPowerGrants={canAssignPowerGrants} + allowPowerGrants={canAssignBoardSeat} defaultValues={{ joinDate: today(), status: "Activo", cargoId: null, comisionIds: [] }} onSubmit={handleSubmit} > - {isAdmin && ( + {canProvisionLogin && ( { expect(onSubmit).toHaveBeenCalledWith({ cargoId: null, comisionIds: [] }); }); + it("explains an empty cargo list to a non-delegate, and never doubles up with the locked note", async () => { + // A catalog of only CEL / power-granting cargos — the real production shape, and the + // state that made the picker silently empty. + const gated: Position[] = [ + { ...pos("presi", "CEL"), grants: [] }, + { ...pos("power", "JDL"), grants: ["Membership"] }, + ]; + const { unmount } = render( + , + ); + expect(screen.getByRole("note")).toHaveTextContent(/Asientos de directiva/); + unmount(); + + // A delegate assigns the same catalog: no note. + const asDelegate = render( + , + ); + expect(screen.queryByRole("note")).not.toBeInTheDocument(); + asDelegate.unmount(); + + // Locked (seated on a power cargo) renders the LOCKED note and not this one. They are + // mutually exclusive today only because cargoOptionsForEditor appends the held cargo, + // making the list non-empty — pin it so a change there cannot produce two notes. + render( + , + ); + const notes = screen.getAllByRole("note"); + expect(notes).toHaveLength(1); + expect(notes[0]).toHaveTextContent(/Solo un Admin/); + }); + it("submits selected cargo and comisiones", async () => { const onSubmit = vi.fn().mockResolvedValue(undefined); render( 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 bc02f796..f322d95d 100644 --- a/apps/backstage/src/features/members/components/member-positions-form.tsx +++ b/apps/backstage/src/features/members/components/member-positions-form.tsx @@ -9,6 +9,7 @@ import { cargoTakedownOnly, positionsLockedForNonAdmin, } from "../lib/assignable-cargo"; +import { NoAssignableCargosNote } from "./no-assignable-cargos-note"; const positionsSchema = z.object({ cargoId: z.string().min(1).nullable(), @@ -128,6 +129,7 @@ export function MemberPositionsForm({ con «Quitar cargo» y guardar, o elegir otro cargo.

)} + {!locked && !allowPowerGrants && cargoOptions.length === 0 && } {formError && (
{formError} diff --git a/apps/backstage/src/features/members/components/member-profile-page.tsx b/apps/backstage/src/features/members/components/member-profile-page.tsx index 61476ea8..24e2c333 100644 --- a/apps/backstage/src/features/members/components/member-profile-page.tsx +++ b/apps/backstage/src/features/members/components/member-profile-page.tsx @@ -128,8 +128,9 @@ export function MemberProfilePage() { actions={
{member.status && {member.status}} - {/* provisionMemberLogin is requireAdmin (role), not the manage:all perm. */} - + {/* provisionMemberLogin is requireAdminOrPerm(create:MemberLogin) — the Admin + role or that exact code, never the manage:all perm. */} +
@@ -145,7 +146,7 @@ export function MemberProfilePage() { defaultValues={memberFormDefaults(member)} submitLabel="Guardar cambios" pendingLabel="Guardando…" - allowPowerGrants={gate.canAssignPowerGrants} + allowPowerGrants={gate.canAssignBoardSeat} onSubmit={handleEdit} avatarSeed={member.name} /> @@ -173,7 +174,7 @@ export function MemberProfilePage() { { expect(screen.queryByText("Desafiliar")).not.toBeInTheDocument(); expect(screen.queryByText("Invitar a la app")).not.toBeInTheDocument(); }); + + it("shows the invite item to a create:MemberLogin delegate with no privileged role", async () => { + // The affordance moved off the Admin ROLE onto canProvisionLogin, mirroring beacon's + // requireAdminOrPerm. update:Member is what keeps the menu itself reachable. + renderMenu(member({ status: "Activo" }), { + roles: ["Member"], + perms: ["update:Member", "create:MemberLogin"], + }); + await userEvent.click(screen.getByLabelText(/Acciones para Ana/)); + expect(screen.getByText("Invitar a la app")).toBeInTheDocument(); + }); + + it("BLOCKING: hides the invite item from a manage:all perm holder without the Admin role", async () => { + // Exact-code gate, matching the callable. A wildcard holder clicking this would get a + // permission-denied from beacon after the fact. + renderMenu(member({ status: "Activo" }), { roles: ["Member"], perms: ["manage:all"] }); + await userEvent.click(screen.getByLabelText(/Acciones para Ana/)); + expect(screen.queryByText("Invitar a la app")).not.toBeInTheDocument(); + }); }); diff --git a/apps/backstage/src/features/members/components/member-row-menu.tsx b/apps/backstage/src/features/members/components/member-row-menu.tsx index 89e0cece..bc2811b2 100644 --- a/apps/backstage/src/features/members/components/member-row-menu.tsx +++ b/apps/backstage/src/features/members/components/member-row-menu.tsx @@ -2,6 +2,7 @@ import { Menu, MenuItem, MenuSeparator } from "@luminova/ui"; import type { Member, MemberStatus } from "@luminova/types"; import { Can } from "../../../lib/authz/ability-context"; import { ActionGate } from "../../../lib/authz/action-gate"; +import { useCan } from "../../../lib/authz/use-can"; interface MemberRowMenuProps { member: Member; @@ -20,6 +21,7 @@ export function MemberRowMenu({ onSetStatus, onUnpublish, }: MemberRowMenuProps) { + const { canProvisionLogin } = useCan(); return ( onEdit(member)}>Editar miembro - {/* provisionMemberLogin is requireAdmin (role), not the manage:all perm. */} - + {/* provisionMemberLogin is requireAdminOrPerm(create:MemberLogin) — the Admin role or + that exact code, never the manage:all perm. */} + onProvision(member)}> {member.uid ? "Reenviar invitación" : "Invitar a la app"} diff --git a/apps/backstage/src/features/members/components/no-assignable-cargos-note.tsx b/apps/backstage/src/features/members/components/no-assignable-cargos-note.tsx new file mode 100644 index 00000000..fe0b830f --- /dev/null +++ b/apps/backstage/src/features/members/components/no-assignable-cargos-note.tsx @@ -0,0 +1,20 @@ +/** Why the Cargo picker is empty, for an editor who is not a board-seat delegate. + * + * Without it the Combobox renders its bare "Sin resultados" and the editor cannot tell a + * permission ceiling from an empty catalog — which is the state a chapter whose every cargo + * carries grants lands in permanently. + * + * Shared by both member forms rather than typed into each: the `locked` and `takedownOnly` + * notes legitimately differ in wording between them, this one does not. + * + * The quoted permission name must stay equal to `permissionLabel("update:BoardSeat")` — + * ACTION_LABELS.update + SUBJECT_LABELS.BoardSeat, in features/permissions. Nothing enforces + * the match across the two features, so it is stated here. */ +export function NoAssignableCargosNote() { + return ( +

+ Ningún cargo del catálogo es asignable con tus permisos. Los cargos del Comité Ejecutivo Local + y los que otorgan permisos requieren un Admin o el permiso «Editar Asientos de directiva». +

+ ); +} diff --git a/apps/backstage/src/features/positions/components/positions-page.tsx b/apps/backstage/src/features/positions/components/positions-page.tsx index b28bb549..84a76aab 100644 --- a/apps/backstage/src/features/positions/components/positions-page.tsx +++ b/apps/backstage/src/features/positions/components/positions-page.tsx @@ -34,7 +34,7 @@ export function PositionsPage() { const seedPositions = useSeedPositions(); // Grants + seed are power-grant writes the rules gate on the Admin *role* // (hasAnyRole(['Admin'])), not the manage:all perm — use the role-based capability. - const { isAdmin, canAssignPowerGrants } = useCan(); + const { isAdmin, canEditCargoCatalog } = useCan(); const [editing, setEditing] = useState(null); const [deactivateTarget, setDeactivateTarget] = useState(null); @@ -175,7 +175,7 @@ export function PositionsPage() { key={editing === "new" ? "new" : editing.id} defaultValues={editing === "new" ? undefined : positionToInput(editing)} submitLabel={editing === "new" ? "Crear" : "Guardar"} - canEditGrants={canAssignPowerGrants} + canEditGrants={canEditCargoCatalog} onSubmit={handleSubmit} /> )} diff --git a/apps/backstage/src/lib/authz/use-can.test.ts b/apps/backstage/src/lib/authz/use-can.test.ts index 52557627..bf4069f2 100644 --- a/apps/backstage/src/lib/authz/use-can.test.ts +++ b/apps/backstage/src/lib/authz/use-can.test.ts @@ -73,6 +73,56 @@ describe("buildCan", () => { expect(can({ roles: ["Member"], perms: ["manage:all"] }).canFeatureInitiatives).toBe(false); }); + // Mirrors firestore.rules' boardSeatDelegate(): Admin by ROLE, everyone else by the exact + // update:BoardSeat PERM. + it("canAssignBoardSeat holds for Admin by role or an update:BoardSeat perm", () => { + expect(can({ roles: ["Admin"] }).canAssignBoardSeat).toBe(true); + expect(can({ roles: ["Admin"], perms: [] }).canAssignBoardSeat).toBe(true); + expect(can({ roles: ["Member"], perms: ["update:BoardSeat"] }).canAssignBoardSeat).toBe(true); + expect(can({ roles: ["Membership"], perms: ["manage:Member"] }).canAssignBoardSeat).toBe(false); + }); + + it("canAssignBoardSeat is false for a manage:all perm holder without the Admin role", () => { + expect(can({ roles: ["Member"], perms: ["manage:all"] }).canAssignBoardSeat).toBe(false); + }); + + // THE C1 PIN. These were one flag while both were Admin-role-only; the delegation splits + // them, and re-unifying would hand a seat delegate the cargo CATALOG — the door round the + // back, since minting a grant-free CEL 'Presidente' and then seating yourself on it lands + // you at public board rank 0. + it("canEditCargoCatalog stays Admin-role-only — update:BoardSeat does NOT reach it", () => { + expect(can({ roles: ["Admin"] }).canEditCargoCatalog).toBe(true); + expect(can({ roles: ["Member"], perms: ["update:BoardSeat"] }).canEditCargoCatalog).toBe(false); + expect( + can({ roles: ["Member"], perms: ["update:Position", "update:BoardSeat"] }) + .canEditCargoCatalog, + ).toBe(false); + expect(can({ roles: ["Member"], perms: ["manage:all"] }).canEditCargoCatalog).toBe(false); + }); + + // Mirrors beacon's requireAdminOrPerm(request, "create:MemberLogin"). + it("canProvisionLogin holds for Admin by role or a create:MemberLogin perm", () => { + expect(can({ roles: ["Admin"] }).canProvisionLogin).toBe(true); + expect(can({ roles: ["Admin"], perms: [] }).canProvisionLogin).toBe(true); + expect(can({ roles: ["Member"], perms: ["create:MemberLogin"] }).canProvisionLogin).toBe(true); + expect(can({ roles: ["Membership"], perms: ["manage:Member"] }).canProvisionLogin).toBe(false); + }); + + it("canProvisionLogin is false for a manage:all perm holder without the Admin role", () => { + expect(can({ roles: ["Member"], perms: ["manage:all"] }).canProvisionLogin).toBe(false); + }); + + // The two delegations are independent by construction — an Admin may grant emailing + // without board seating and vice versa. Pinned because they ship together. + it("the two delegations do not imply each other", () => { + const seat = can({ roles: ["Member"], perms: ["update:BoardSeat"] }); + expect(seat.canAssignBoardSeat).toBe(true); + expect(seat.canProvisionLogin).toBe(false); + const login = can({ roles: ["Member"], perms: ["create:MemberLogin"] }); + expect(login.canProvisionLogin).toBe(true); + expect(login.canAssignBoardSeat).toBe(false); + }); + // Same invariant the `` gate carries: a conditional own-doc grant answers only the // per-document question, never the collection one. it("a plain Member's own-doc grant does not answer the collection question", () => { diff --git a/apps/backstage/src/lib/authz/use-can.ts b/apps/backstage/src/lib/authz/use-can.ts index 94dfa074..da7d3439 100644 --- a/apps/backstage/src/lib/authz/use-can.ts +++ b/apps/backstage/src/lib/authz/use-can.ts @@ -30,13 +30,28 @@ export interface Can { /** 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; - /** The Admin-only half of the positions authority — one flag because firestore.rules keys - * every part of it on the same `hasAnyRole(['Admin'])`: assigning a power-granting cargo - * (`cargoAssignableByNonAdmin` / `currentCargoGrantsEmpty` / `createPositionsSafe`) or a CEL cargo - * at all, creating a board-surfacing cargo (`boardSurfacingCategory()`), and editing a - * stored cargo's `grants`, `category` or — on a board cargo — `title`/`titleFemale`. - * Named so the policy isn't a bare `isAdmin` at each grant site. */ - readonly canAssignPowerGrants: 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; } /** Pure builder — no React — so the gate logic is unit-testable. */ @@ -66,7 +81,11 @@ export function buildCan(ability: AppAbility, claims: AuthClaims): Can { // 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: hasAnyRole(claims, ["Admin"]) || hasPerm(claims, "update:Showcase"), - canAssignPowerGrants: hasAnyRole(claims, ["Admin"]), + // 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: hasAnyRole(claims, ["Admin"]) || hasPerm(claims, "update:BoardSeat"), + canEditCargoCatalog: hasAnyRole(claims, ["Admin"]), + canProvisionLogin: hasAnyRole(claims, ["Admin"]) || hasPerm(claims, "create:MemberLogin"), }; } From 427f29e126da707dcf0f9b465af8f58a41e74f5d Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 18:50:56 -0400 Subject: [PATCH 05/15] refactor: /simplify pass over the branch diff - extract noAssignableCargos() into assignable-cargo.ts. The three-clause render guard was typed out at both member forms, immediately next to the two sibling predicates that file exists to hold so the forms cannot disagree. - extract adminOrPerm() in use-can.ts at its third occurrence. - rename provisionMember's callerIsAdmin parameter to callerHoldsAdminRole: it shadowed the imported callerIsAdmin function in the same module. Skipped: hoisting canProvisionLogin out of member-row-menu into member-table as a prop. It saves one buildCan() per row, bounded by the page size (max 32) and each a trivial object construction, at the cost of prop plumbing. Co-Authored-By: Claude Opus 5 (1M context) --- .../members/components/member-form.tsx | 3 ++- .../components/member-positions-form.tsx | 3 ++- .../features/members/lib/assignable-cargo.ts | 18 ++++++++++++++++++ apps/backstage/src/lib/authz/use-can.ts | 15 ++++++++++++--- apps/beacon/src/provision-member-login.ts | 4 ++-- 5 files changed, 36 insertions(+), 7 deletions(-) diff --git a/apps/backstage/src/features/members/components/member-form.tsx b/apps/backstage/src/features/members/components/member-form.tsx index a3ad59a6..10ea19f4 100644 --- a/apps/backstage/src/features/members/components/member-form.tsx +++ b/apps/backstage/src/features/members/components/member-form.tsx @@ -26,6 +26,7 @@ import { avatarColor } from "../lib/member-display"; import { cargoOptionsForEditor, cargoTakedownOnly, + noAssignableCargos, positionsLockedForNonAdmin, } from "../lib/assignable-cargo"; import { NoAssignableCargosNote } from "./no-assignable-cargos-note"; @@ -311,7 +312,7 @@ export function MemberForm({ igual.

)} - {!positionsLocked && !allowPowerGrants && cargoOptions.length === 0 && ( + {noAssignableCargos({ cargoOptions, allowPowerGrants, locked: positionsLocked }) && ( )} )} - {!locked && !allowPowerGrants && cargoOptions.length === 0 && } + {noAssignableCargos({ cargoOptions, allowPowerGrants, locked }) && } {formError && (
{formError} diff --git a/apps/backstage/src/features/members/lib/assignable-cargo.ts b/apps/backstage/src/features/members/lib/assignable-cargo.ts index 97c1b5b0..982c985c 100644 --- a/apps/backstage/src/features/members/lib/assignable-cargo.ts +++ b/apps/backstage/src/features/members/lib/assignable-cargo.ts @@ -71,6 +71,24 @@ export function cargoTakedownOnly( export type CargoOption = { value: string; label: string; disabled?: boolean }; +/** + * The third derived render-state, alongside `positionsLockedForNonAdmin` and + * `cargoTakedownOnly`: this editor may assign, but the ceiling filtered every option away. + * + * Lives here for the same reason as its two siblings — both member forms ask it, and typing + * the three-clause condition at each call site is how the two forms drift. `locked` is passed + * in rather than recomputed because the forms derive it differently (one from the stored + * cargo, one from `defaultValues`), and it must be excluded: a locked slot renders its own + * note, and `cargoOptionsForEditor` appends the held cargo, so the two states cannot co-fire. + */ +export function noAssignableCargos(input: { + cargoOptions: readonly CargoOption[]; + allowPowerGrants: boolean; + locked: boolean; +}): boolean { + return !input.locked && !input.allowPowerGrants && input.cargoOptions.length === 0; +} + /** * The cargo Combobox options for one editor, shared by both member forms so they cannot * disagree about the same rules predicate (they did: one dropped the held CEL seat, the other diff --git a/apps/backstage/src/lib/authz/use-can.ts b/apps/backstage/src/lib/authz/use-can.ts index da7d3439..b6505fa0 100644 --- a/apps/backstage/src/lib/authz/use-can.ts +++ b/apps/backstage/src/lib/authz/use-can.ts @@ -1,6 +1,7 @@ import { useMemo } from "react"; import { hasAnyRole, hasPerm, 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 { isNavItemVisible, type NavItem } from "../../components/nav-config"; import { canRemoveEntry } from "../../features/check-in/lib/can-remove-entry"; @@ -54,6 +55,14 @@ export interface Can { 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. */ export function buildCan(ability: AppAbility, claims: AuthClaims): Can { return { @@ -80,12 +89,12 @@ export function buildCan(ability: AppAbility, claims: AuthClaims): Can { // 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: hasAnyRole(claims, ["Admin"]) || hasPerm(claims, "update:Showcase"), + 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: hasAnyRole(claims, ["Admin"]) || hasPerm(claims, "update:BoardSeat"), + canAssignBoardSeat: adminOrPerm(claims, "update:BoardSeat"), canEditCargoCatalog: hasAnyRole(claims, ["Admin"]), - canProvisionLogin: hasAnyRole(claims, ["Admin"]) || hasPerm(claims, "create:MemberLogin"), + canProvisionLogin: adminOrPerm(claims, "create:MemberLogin"), }; } diff --git a/apps/beacon/src/provision-member-login.ts b/apps/beacon/src/provision-member-login.ts index 959e1023..a8ccd7aa 100644 --- a/apps/beacon/src/provision-member-login.ts +++ b/apps/beacon/src/provision-member-login.ts @@ -74,7 +74,7 @@ export async function provisionMember( /** Whether the CALLER holds the Admin role. A `create:MemberLogin` delegate does not, and * is confined to the new-account path below — see the adoption guard. Defaults to false: * a new call site must opt INTO the privileged path, never inherit it by omission. */ - callerIsAdmin = false, + callerHoldsAdminRole = false, ): Promise<{ email: string; actionLink: string }> { const member = await deps.getMember(memberId); if (member === null) throw new HttpsError("not-found", "member not found"); @@ -115,7 +115,7 @@ export async function provisionMember( // one already linked to THIS member (resend invite). That costs the delegation nothing — // a genuinely new member has no Auth account — and is why the guard is a hard refusal // rather than a claims-only restriction. - if (!callerIsAdmin && user !== null && user.uid !== linkedUid) { + if (!callerHoldsAdminRole && user !== null && user.uid !== linkedUid) { throw new HttpsError( "permission-denied", "this email already has a login; only an Admin can link an existing account", From 6abd7924f3a57eae64ff0f68def509091fd97834 Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 19:01:49 -0400 Subject: [PATCH 06/15] fix(beacon,rules): conferring Admin stays Admin-only; close the create-lane ride-along MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review found the first form of the trust guard was both unsafe and a production outage. Replaced, not patched. Trust gate: a cargo whose grants include Admin is honored only for an assigner holding the Admin ROLE; every other cargo is honored for an update:BoardSeat delegate too. The previous self-assignment form was wrong twice over: - it missed the two-write puppet loop (delegate creates a second member on a mailbox they control, seats IT on Presidente — not a self-assignment, so the perm was trusted — and that puppet is Admin forever), so revocation, the premise the whole delegation was accepted on, did not work; and - it stripped the SITTING PRESIDENT. seed-president.mjs stamps assignedBy with the president's own uid and an Admin's perms are manage:all, never the exact code — verified against the live production member doc, which is self-assigned on an Admin-granting cargo. Pinned by a regression test. provisionMemberLogin: a non-Admin may provision only a member with NO login yet. The resend path was the hole — passwordResetLink is generatePasswordResetLink, which returns the oobCode URL to the CALLER, so a delegate could pass the president's memberId and receive a live reset link for their address. createPositionsSafe: restrict to the current term key, mirroring the update arm. assignedBySelf() and cargoAssignableByNonAdmin() read only the current term, so a second term key rode along unvalidated — a next-term power cargo attributed to a real Admin, minted on the year rollover. Pre-existing, in the function this branch rewrites. Tests: retargeted three delegate pins that were silently denied by G3 rather than by the conjunct they named; added rules coverage for hasPerm-vs-canDo, the create-lane self-stamp and the create-lane non-vacuity pin. Every new guard mutation-tested: neutralizing each turns exactly its own test red and nothing else. Co-Authored-By: Claude Opus 5 (1M context) --- apps/beacon/src/claims-sync/sync.test.ts | 95 +++++++++++------- apps/beacon/src/claims-sync/sync.ts | 48 +++++---- .../beacon/src/provision-member-login.test.ts | 49 ++++++--- apps/beacon/src/provision-member-login.ts | 23 +++-- docs/specs/board-seat-delegation.md | 67 +++++++++---- firestore.rules | 24 ++++- tests/firestore-rules/rules.test.ts | 99 ++++++++++++++++++- 7 files changed, 300 insertions(+), 105 deletions(-) diff --git a/apps/beacon/src/claims-sync/sync.test.ts b/apps/beacon/src/claims-sync/sync.test.ts index c30cdbf2..fece24f2 100644 --- a/apps/beacon/src/claims-sync/sync.test.ts +++ b/apps/beacon/src/claims-sync/sync.test.ts @@ -34,8 +34,8 @@ const customRole = (id: string, permissions: PermissionCode[]): RoleDefinition = function fakeDeps(opts: { positions: Record; userRoles: Record; - /** The assigner's `perms` claim — the second trust source alongside the Admin role, and - * the ONLY one honored on a self-assignment (see resolveTrustedGrants). */ + /** The assigner's `perms` claim — the second trust source alongside the Admin role, and the + * one that does NOT extend to conferring Admin (see resolveTrustedGrants). */ userPerms?: Record; existing: Record; builtInDocs?: RoleDefinition[]; @@ -132,12 +132,12 @@ describe("syncMemberClaims", () => { }); }); - it("honors power grants when the assigner holds update:BoardSeat and NOT the Admin role", async () => { - // The mirror of the Admin case above, and the reason Slice 3 exists: a delegate stamps - // their own uid into assignedBy, so without this disjunct the seat would land and mint - // nothing. + it("honors NON-Admin power grants when the assigner holds update:BoardSeat", async () => { + // The mirror of the Admin case above, and the reason the perm disjunct exists: a delegate + // stamps their own uid into assignedBy, so without it the seat would land and mint nothing + // — the member published on the world-readable Directiva, powerless and silent. const { deps, writes } = fakeDeps({ - positions: { "pos-pres": { grants: ["Admin"] } }, + positions: { "pos-dir": { grants: ["Membership"] } }, userRoles: { "delegate-uid": ["Member"] }, userPerms: { "delegate-uid": ["update:BoardSeat"] }, existing: { "target-uid": { roles: ["Member"] } }, @@ -146,13 +146,13 @@ describe("syncMemberClaims", () => { deps, { uid: "target-uid", - positions: { "2026": { cargoId: "pos-pres", comisionIds: [], assignedBy: "delegate-uid" } }, + positions: { "2026": { cargoId: "pos-dir", comisionIds: [], assignedBy: "delegate-uid" } }, }, "2026", ); expect(writes["target-uid"]).toEqual({ - roles: ["Admin", "Member"], - perms: permsFor(["Admin", "Member"]), + roles: ["Membership", "Member"], + perms: permsFor(["Membership", "Member"]), }); }); @@ -161,7 +161,7 @@ describe("syncMemberClaims", () => { // must not answer it on the server any more than it does in the client gate — otherwise // any manage:all holder silently becomes a seat delegate. const { deps, writes } = fakeDeps({ - positions: { "pos-pres": { grants: ["Admin"] } }, + positions: { "pos-dir": { grants: ["Membership"] } }, userRoles: { "wildcard-uid": ["Member"] }, userPerms: { "wildcard-uid": ["manage:all"] }, existing: { "target-uid": { roles: ["Member"], perms: permsFor(["Member"]) } }, @@ -170,59 +170,80 @@ describe("syncMemberClaims", () => { deps, { uid: "target-uid", - positions: { "2026": { cargoId: "pos-pres", comisionIds: [], assignedBy: "wildcard-uid" } }, + positions: { "2026": { cargoId: "pos-dir", comisionIds: [], assignedBy: "wildcard-uid" } }, }, "2026", ); expect(writes["target-uid"]).toBeUndefined(); }); - it("BLOCKING: a SELF-assignment is non-reflexive — a cargo-derived Admin cannot re-trust itself", async () => { - // The guard the whole delegation rests on. Without it: a delegate self-seats Presidente, - // this function mints them Admin, and the minted Admin then satisfies the gate that - // minted it — so revoking update:BoardSeat re-fires the trigger, finds the Admin role, - // and re-honors the grants forever. The delegation would be permanent. - // Fixture is the post-revocation state exactly: assigner IS the target, holds Admin by - // role (from the cargo), and no longer holds the perm. + it("BLOCKING: a delegate may NOT confer ADMIN — the guard the delegation rests on", async () => { + // Conferring Admin is reserved to the Admin ROLE. Without this a delegate mints an Admin, + // that Admin is itself a trust source, and the delegation can never be revoked — via the + // one-write self-loop OR the two-write puppet loop (seat a second member you control; it + // is not a self-assignment, so a reflexivity check would miss it entirely). const { deps, writes } = fakeDeps({ positions: { "pos-pres": { grants: ["Admin"] } }, - userRoles: { "self-uid": ["Admin", "Member"] }, - userPerms: { "self-uid": [] }, - existing: { "self-uid": { roles: ["Admin", "Member"] } }, + userRoles: { "delegate-uid": ["Member"] }, + userPerms: { "delegate-uid": ["update:BoardSeat"] }, + existing: { "target-uid": { roles: ["Member"], perms: permsFor(["Member"]) } }, }); await syncMemberClaims( deps, { - uid: "self-uid", - positions: { "2026": { cargoId: "pos-pres", comisionIds: [], assignedBy: "self-uid" } }, + uid: "target-uid", + positions: { "2026": { cargoId: "pos-pres", comisionIds: [], assignedBy: "delegate-uid" } }, }, "2026", ); - // De-elevated to a plain Member: revocation actually revokes. - expect(writes["self-uid"]).toEqual({ roles: ["Member"], perms: permsFor(["Member"]) }); + expect(writes["target-uid"]).toBeUndefined(); }); - it("still honors a SELF-assignment backed by the perm itself", async () => { - // The paired ALLOW — otherwise the test above would pass for a rule that simply refused - // every self-assignment, which would break an Admin seating their own cargo. + it("REGRESSION: an Admin who SELF-assigned their Admin cargo keeps it", async () => { + // The seeded bootstrap president: seed-president.mjs stamps assignedBy = their own uid, + // and their perms are manage:all — never the exact update:BoardSeat code. An earlier form + // of this guard keyed on `assignedBy === member.uid` and would have stripped exactly this + // member on their next write, in production, with no in-app recovery. Verified against the + // live member doc before it shipped; this pins it. const { deps, writes } = fakeDeps({ positions: { "pos-pres": { grants: ["Admin"] } }, - userRoles: { "self-uid": ["Member"] }, - userPerms: { "self-uid": ["update:BoardSeat"] }, - existing: { "self-uid": { roles: ["Member"] } }, + userRoles: { "president-uid": ["Admin", "Member"] }, + userPerms: { "president-uid": ["manage:all"] }, + existing: { + "president-uid": { roles: ["Admin", "Member"], perms: permsFor(["Admin", "Member"]) }, + }, }); await syncMemberClaims( deps, { - uid: "self-uid", - positions: { "2026": { cargoId: "pos-pres", comisionIds: [], assignedBy: "self-uid" } }, + uid: "president-uid", + positions: { + "2026": { cargoId: "pos-pres", comisionIds: [], assignedBy: "president-uid" }, + }, }, "2026", ); - expect(writes["self-uid"]).toEqual({ - roles: ["Admin", "Member"], - perms: permsFor(["Admin", "Member"]), + // Idempotent no-op: the claims are already correct, so no write — and crucially not a strip. + expect(writes["president-uid"]).toBeUndefined(); + }); + + it("de-elevates a delegate-conferred NON-Admin grant once the perm is revoked", async () => { + // Revocation is real for everything a delegate can actually confer. + const { deps, writes } = fakeDeps({ + positions: { "pos-dir": { grants: ["Membership"] } }, + userRoles: { "delegate-uid": ["Member"] }, + userPerms: { "delegate-uid": [] }, + existing: { "target-uid": { roles: ["Membership", "Member"] } }, }); + await syncMemberClaims( + deps, + { + uid: "target-uid", + positions: { "2026": { cargoId: "pos-dir", comisionIds: [], assignedBy: "delegate-uid" } }, + }, + "2026", + ); + expect(writes["target-uid"]).toEqual({ roles: ["Member"], perms: permsFor(["Member"]) }); }); it("BLOCKING: positive-and-inert — a grant-free cargo from a NON-Admin assigner mints nothing", async () => { diff --git a/apps/beacon/src/claims-sync/sync.ts b/apps/beacon/src/claims-sync/sync.ts index 41b623d6..75bfe78f 100644 --- a/apps/beacon/src/claims-sync/sync.ts +++ b/apps/beacon/src/claims-sync/sync.ts @@ -59,19 +59,32 @@ type MemberLike = { * seat lands, the member is published on the world-readable Directiva, and no claim is * minted. Visible, powerless, and silent: half-working rather than safe. * - * NON-REFLEXIVE on a self-assignment, and this is the guard the whole delegation rests on. - * When the assigner IS the member being seated, only the PERM is trusted — never the Admin - * role. Otherwise: a delegate self-seats Presidente, this function mints them Admin, and - * from then on the minted Admin satisfies the gate that minted it. Revoking - * `update:BoardSeat` would re-fire this very trigger, find the Admin role, and re-honor the - * grants forever; `recomputeAllClaims` runs the same code and would not break the loop - * either. The delegation would be permanent, which is exactly what its acceptance was - * premised on NOT being. An Admin seating someone ELSE is untouched. */ + * CONFERRING ADMIN IS ADMIN-ONLY, and this is the guard the whole delegation rests on. A + * cargo whose grants include `Admin` is honored only for an assigner holding the Admin ROLE; + * every other cargo is honored for an `update:BoardSeat` delegate too. + * + * Why the split rather than a self-assignment check (which is what this first shipped as): + * the danger is not reflexivity, it is that a delegate can mint an Admin AT ALL, because a + * minted Admin is itself a trust source and the delegation then cannot be revoked. Blocking + * only `assignedBy === memberUid` stops the one-write self-loop and not the two-write puppet + * loop — a delegate creates a second member on a mailbox they control, seats IT on + * Presidente (not a self-assignment, so the perm is trusted), and that puppet is Admin + * forever; revoking the delegate's code de-elevates nobody. It also had a worse problem: the + * seeded bootstrap president self-stamps `assignedBy` (tools/scripts/lib/seed-president.mjs) + * and their perms are `manage:all`, never the exact `update:BoardSeat` code — so the + * self-assignment form stripped the sitting president's Admin on their next member write. + * + * Keying on the GRANT fixes both: no cargo-derived Admin can ever originate from a delegate, + * so there is no loop to close and no anchor to special-case, and an Admin seating anyone — + * including themselves — is untouched. Revocation is then real for everything a delegate + * CAN confer: strip the perm and the next write to that member drops the grants. + * + * The cost, stated so nobody reads it as a bug: a delegate seating a member on an + * Admin-granting cargo publishes the seat but mints no claim. An Admin must re-stamp it. */ async function resolveTrustedGrants( deps: ClaimsSyncDeps, cargoId: string | null, assignedBy: string | undefined, - memberUid: string, ): Promise { // FULL screening, not just the empty-string half this used to check. `cargoId` comes // straight off the member doc and every implementation of `getPosition` interpolates it @@ -88,10 +101,12 @@ async function resolveTrustedGrants( if (!position || position.grants.length === 0) return []; if (!assignedBy) return []; const assigner = await deps.getAssignerClaims(assignedBy); - const selfAssigned = assignedBy === memberUid; - const trusted = selfAssigned - ? assigner.perms.includes("update:BoardSeat") - : assigner.roles.includes("Admin") || assigner.perms.includes("update:BoardSeat"); + const assignerIsAdmin = assigner.roles.includes("Admin"); + // Conferring ADMIN is reserved to an Admin, whoever assigned the cargo. Everything else a + // board-seat delegate may confer. + const trusted = position.grants.includes("Admin") + ? assignerIsAdmin + : assignerIsAdmin || assigner.perms.includes("update:BoardSeat"); return trusted ? [...new Set(position.grants)] : []; } @@ -120,12 +135,7 @@ export async function syncMemberClaims( ): Promise { if (!member.uid) return; const term = member.positions?.[termKey]; - const trustedGrants = await resolveTrustedGrants( - deps, - term?.cargoId ?? null, - term?.assignedBy, - member.uid, - ); + const trustedGrants = await resolveTrustedGrants(deps, term?.cargoId ?? null, term?.assignedBy); const existing = await deps.getExistingClaims(member.uid); const hadScanner = existing.roles.includes("Scanner"); diff --git a/apps/beacon/src/provision-member-login.test.ts b/apps/beacon/src/provision-member-login.test.ts index ef6c82ca..a33e4743 100644 --- a/apps/beacon/src/provision-member-login.test.ts +++ b/apps/beacon/src/provision-member-login.test.ts @@ -107,18 +107,22 @@ describe("provisionMember", () => { }); it("self-heals a stale link by minting a fresh account when the email resolves nothing", async () => { + // ADMIN caller: a stored uid means this member was provisioned once already, so recovery + // is an Admin op. A delegate hits the reprovision guard instead (test below). const { deps, calls } = fakeDeps({ member: { ...active, uid: "dead-uid" } }); - await provisionMember(deps, "m1"); + await provisionMember(deps, "m1", true); expect(calls.createUser).toEqual(["a@b.co"]); expect(calls.linkUid).toEqual(["new-a@b.co"]); }); it("re-provisions idempotently when the stored uid matches the resolved user (resend invite)", async () => { + // ADMIN caller. Resend returns a live password-reset link for the member's address, so it + // is Admin-only — see the non-Admin BLOCKING case below. const { deps, calls } = fakeDeps({ member: { ...active, uid: "u1" }, usersByEmail: { "a@b.co": { uid: "u1", email: "a@b.co" } }, }); - const result = await provisionMember(deps, "m1"); + const result = await provisionMember(deps, "m1", true); expect(result).toEqual({ email: "a@b.co", actionLink: "link:a@b.co" }); expect(calls.createUser).toEqual([]); expect(calls.setClaims).toEqual(["u1"]); @@ -171,27 +175,46 @@ describe("provisionMember", () => { expect(calls.createUser).toEqual([]); }); - it("still lets a non-Admin caller mint a BRAND-NEW account, and resend to its own link", async () => { + it("still lets a non-Admin caller mint a BRAND-NEW account", async () => { // The delegation costs nothing on the path it is actually for: a genuinely new member - // has no Auth account. + // has neither an Auth account nor a stored uid. const fresh = fakeDeps({ member: active }); await expect(provisionMember(fresh.deps, "m1", false)).resolves.toEqual({ email: "a@b.co", actionLink: "link:a@b.co", }); expect(fresh.calls.createUser).toEqual(["a@b.co"]); - // And re-provisioning a member ALREADY linked to that account stays open (resend invite): - // user.uid === linkedUid, so it is not an adoption. - const resend = fakeDeps({ + }); + + it("BLOCKING: a non-Admin caller may NOT re-provision an ALREADY-LINKED member", async () => { + // The resend path is an account-takeover primitive, not a convenience. passwordResetLink + // is generatePasswordResetLink — it hands the oobCode URL to the CALLER, unlike the + // client-side sendPasswordResetEmail which delivers it to the mailbox owner. So without + // this, a create:MemberLogin holder could pass the president's memberId, receive a live + // reset link for their address, and sign in as them. No adoption, no forged email — + // every other guard satisfied. + const { deps, calls } = fakeDeps({ member: { ...active, uid: "u1" }, - usersByEmail: { "a@b.co": { uid: "u1", email: "a@b.co" } }, + usersByEmail: { + "a@b.co": { uid: "u1", email: "a@b.co", customClaims: { roles: ["Admin"] } }, + }, }); - await expect(provisionMember(resend.deps, "m1", false)).resolves.toEqual({ - email: "a@b.co", - actionLink: "link:a@b.co", + await expect(provisionMember(deps, "m1", false)).rejects.toMatchObject({ + code: "permission-denied", }); - expect(resend.calls.createUser).toEqual([]); - expect(resend.calls.linkUid).toEqual(["u1"]); + expect(calls.setClaims).toEqual([]); + expect(calls.linkUid).toEqual([]); + }); + + it("BLOCKING: a non-Admin caller may NOT provision a member whose uid is set but account is gone", async () => { + // The self-heal branch: linkedUid points at a deleted account, so getUserByEmail may + // return null and the adoption half alone would let this through. A stored uid means an + // Admin already provisioned this member once — recovery is theirs. + const { deps, calls } = fakeDeps({ member: { ...active, uid: "dead-uid" } }); + await expect(provisionMember(deps, "m1", false)).rejects.toMatchObject({ + code: "permission-denied", + }); + expect(calls.createUser).toEqual([]); }); it("defaults callerIsAdmin to false — a new call site must opt into adoption", async () => { diff --git a/apps/beacon/src/provision-member-login.ts b/apps/beacon/src/provision-member-login.ts index a8ccd7aa..c4512161 100644 --- a/apps/beacon/src/provision-member-login.ts +++ b/apps/beacon/src/provision-member-login.ts @@ -111,15 +111,24 @@ export async function provisionMember( // member doc through the admin SDK (the only path that can write members.uid at all), and // passwordResetLink() handing back a reset link for the Admin's mailbox. // - // A non-Admin therefore gets exactly two shapes: mint a brand-new account, or re-provision - // one already linked to THIS member (resend invite). That costs the delegation nothing — - // a genuinely new member has no Auth account — and is why the guard is a hard refusal - // rather than a claims-only restriction. - if (!callerHoldsAdminRole && user !== null && user.uid !== linkedUid) { + // A non-Admin therefore gets exactly ONE shape: mint a brand-new Auth account for a member + // that has none. Not "anything but adoption" — the RESEND path is just as dangerous and was + // the first draft's hole. `passwordResetLink` below is `generatePasswordResetLink`, which + // returns the oobCode URL TO THE CALLER; that is categorically different from + // `sendPasswordResetEmail`, which delivers the secret to the mailbox owner and is already + // unprivileged. So a delegate allowed to "resend" an invite for an ALREADY-LINKED member + // could name the president's memberId, take the returned link, set a password and sign in + // as them — no adoption involved, every existing guard satisfied. + // + // Hence both halves: no pre-existing account for this email (`user === null`) AND no + // pre-existing link (`linkedUid === null`). Costs the delegation nothing — a genuinely new + // member has neither — and leaves resend/adoption/self-heal to an Admin, plus the + // client-side sendPasswordResetEmail any member can already use on their own address. + if (!callerHoldsAdminRole && (user !== null || linkedUid !== null)) { throw new HttpsError( "permission-denied", - "this email already has a login; only an Admin can link an existing account", - { reason: "adoption-requires-admin" }, + "this member already has a login; only an Admin can re-provision or link one", + { reason: "reprovision-requires-admin" }, ); } if (!user) user = await deps.createUser(email); diff --git a/docs/specs/board-seat-delegation.md b/docs/specs/board-seat-delegation.md index d9825243..b3835e14 100644 --- a/docs/specs/board-seat-delegation.md +++ b/docs/specs/board-seat-delegation.md @@ -9,7 +9,7 @@ was previously hardcoded to the `Admin` role, then revoke it. Both are granted p | Subject | Label in the matrix | Live code | Confers | |---|---|---|---| | `BoardSeat` | Asientos de directiva | `update:BoardSeat` | seat a member on **any vacant cargo** — CEL category and power-granting alike | -| `MemberLogin` | Acceso de miembros | `create:MemberLogin` | call `provisionMemberLogin`: create the member's Auth account, link their uid, return the password-reset link | +| `MemberLogin` | Acceso de miembros | `create:MemberLogin` | call `provisionMemberLogin` for a member who has **no login yet**: create their Auth account, link their uid, return the password-reset link | They are independent by construction. An Admin can grant emailing without board seating and vice versa. The other five codes each subject generates (`manage:BoardSeat`, `read:MemberLogin`, …) @@ -22,8 +22,13 @@ does not satisfy either one. holds `update:Member`, `create:Member`, or `update:Position`. A member holding only `update:BoardSeat` still cannot write anything. Granting it alone is a no-op; the UI does not say so. +- **It does not confer Admin.** See the section below — an Admin-granting cargo mints nothing + unless the assigner holds the Admin role. - **It does not unseat anyone.** `currentCargoGrantsEmpty()` stays Admin-only, so a delegate cannot displace a member sitting on a power-granting cargo. Hand-over is an Admin action. + One caveat, pre-existing and pinned by a rules test: that predicate reads only the CURRENT + term, so in the window after a UTC-year rollover an Admin's next-term slot is empty and any + `update:Position` holder can write into it. Rationale: the `roles` claim is derived *exclusively* from cargo grants (`compute-roles.ts`), so clearing every Admin's cargo would strip every Admin claim in the chapter — and `setUserRoles`, `roles/*` writes and `permissionOverrides` writes are all @@ -36,20 +41,35 @@ does not satisfy either one. `updatePermissionAssignmentSafe()` are unchanged, so a delegate cannot re-grant the delegation to themselves or anyone else. -## Accepted decision — claims-minting delegation +## Conferring Admin stays Admin-only -The chapter owner has explicitly accepted that a delegate may seat a member on a cargo whose -`grants` include `Admin`, and that beacon's trust gate will mint that claim. That is the feature, -not a defect. The acceptance is premised on the delegation being **revocable**, which required -one guard: +The chapter owner accepted that a delegate may seat power-granting cargos, on the explicit +premise that the delegation is **revocable**. Delivering that premise turned out to require one +rule, and it is the load-bearing guard of the whole feature: -**The trust gate is non-reflexive.** When `assignedBy === member.uid` (a self-assignment), -`resolveTrustedGrants` trusts only `update:BoardSeat` in the assigner's `perms` — never an -`Admin` role in their `roles`. Without this, a delegate who self-seats `Presidente` is minted -`Admin`, and that minted `Admin` then satisfies the very gate that minted it: revoking -`update:BoardSeat` re-fires the trigger, the Admin disjunct passes, and the grants are re-honored -forever. `recomputeAllClaims` runs the same code and would not break the loop either. An Admin -seating *someone else* is unaffected. +**A cargo whose `grants` include `Admin` is honored only when the assigner holds the Admin +ROLE.** Every other cargo is honored for an `update:BoardSeat` delegate too. + +Why, precisely: a minted Admin is itself a trust source, so a delegate who can mint one has +made the delegation permanent. Revoking their code de-elevates nobody, and neither +`recomputeAllClaims` nor any in-app path recovers it. + +Two earlier forms of this guard were wrong and are recorded so they are not re-proposed: + +- **Blocking only self-assignment** (`assignedBy === member.uid`) stops the one-write self-loop + and misses the two-write puppet loop: the delegate creates a second member on a mailbox they + control, seats *it* on Presidente — not a self-assignment, so the perm is trusted — and that + puppet is Admin forever. +- Worse, that same form **strips the sitting president**. `seed-president.mjs` stamps + `assignedBy` with the president's own uid, and an Admin's perms are `manage:all`, never the + exact `update:BoardSeat` code — so the president's own Admin claim would be dropped on the + next write to their member doc. Confirmed against the live production member doc before it + shipped, and pinned by a regression test. + +**The cost, so it is not read as a bug:** a delegate seating someone on an Admin-granting cargo +publishes the seat but mints no claim. An Admin must re-stamp it. Everything a delegate *can* +confer is genuinely revocable — strip the perm and the next write to that member drops the +grants. ## `create:MemberLogin` — what is actually privileged @@ -58,15 +78,20 @@ client-side `sendPasswordResetEmail` that any signed-in user can already call. W owns is Auth account creation, uid linking (the only path that can write `members.uid` at all, since the rules forbid it on every client lane) and the initial claim write. -**A non-Admin caller may only mint a NEW Auth account, or re-provision one already linked to that -same member.** It may never adopt a pre-existing unlinked account. Without that restriction the -code would be an account-takeover primitive rather than an invite helper: `members.email` is -unconstrained by the rules and has no uniqueness check, so a delegate could create a member doc -carrying a sitting Admin's email and have the callable adopt that Admin's account, strip its -claims, link its uid onto the attacker's member doc, and return a password-reset link for it. -An Admin caller keeps the full adoption path — it is the documented recovery op. +**A non-Admin caller may provision a member only when that member has NO login yet** — no +existing Auth account for their email, and no stored `uid`. Adoption, re-provision/resend, and +the deleted-account self-heal are all Admin-only. + +The resend path is restricted for the same reason as adoption, and it is the less obvious one: +`passwordResetLink` is `generatePasswordResetLink`, which returns the oobCode URL **to the +caller** — categorically different from `sendPasswordResetEmail`, which delivers the secret to +the mailbox owner. So a delegate permitted to "resend" could pass the president's `memberId`, +receive a live reset link for their address, set a password and sign in as them, with every +other guard satisfied. Adoption is worse still: `members.email` is unconstrained by the rules +and has no uniqueness check, so a delegate could file a member doc carrying an Admin's email and +have the callable adopt that account, strip its claims and link its uid onto their own doc. -This restriction costs the delegation nothing: a genuinely new member has no Auth account. +This costs the delegation nothing: a genuinely new member has neither an account nor a uid. ## Operator notes diff --git a/firestore.rules b/firestore.rules index 297f8170..8f81d747 100644 --- a/firestore.rules +++ b/firestore.rules @@ -239,12 +239,28 @@ service cloud.firestore { // Admin provisionMemberLogin then writes the uid, after which projectBoard publishes an // attacker-composed account at board rank 0 as Presidente. currentCargoGrantsEmpty() is // the only half a create cannot ask (no prior resource), and it has no old side to guard. - // Plain substitution, unlike positionsAssignmentSafe(): a create has no prior resource, - // so there is no old side to keep Admin-only — currentCargoGrantsEmpty() has never - // applied here, and a member born on a cargo displaces nobody. + // Plain substitution on the cargo conjunct, unlike positionsAssignmentSafe(): a create has + // no prior resource, so there is no old side to keep Admin-only — currentCargoGrantsEmpty() + // has never applied here, and a member born on a cargo displaces nobody. + // + // The current-term restriction, though, is NOT optional and was missing: assignedBySelf() + // and cargoAssignableByNonAdmin() both read ONLY positions[currentTermKey()] (via + // assignedTerm()), so every OTHER term key in a created map was entirely unvalidated. The + // update arm closes this — see positionsDelta().hasOnly() and its comment — and the create + // arm has to say the same thing or the ride-along just moves to birth: + // positions: { "<this year>": <grant-free, self-stamped: passes>, + // "<next year>": { cargoId: <Presidente>, assignedBy: <a real Admin uid> } } + // Nothing inspects the second entry. On the UTC-year rollover claims-sync reads it, sees a + // genuine Admin as assignedBy, and mints Admin onto a member whose email — and therefore + // whose login — the creator controls. Forged attribution, one term deferred; the trust gate + // cannot help, because the assigner really is an Admin. + // Symmetric with the update arm: Admin is already current-term-only there, so this adds no + // Admin restriction that did not already exist on the other lane. function createPositionsSafe() { return !('positions' in request.resource.data) - || (assignedBySelf() && (boardSeatDelegate() || cargoAssignableByNonAdmin())); + || (request.resource.data.get('positions', {}).keys().hasOnly([currentTermKey()]) + && assignedBySelf() + && (boardSeatDelegate() || cargoAssignableByNonAdmin())); } // roleIds + permissionOverrides feed Auth custom claims via the beacon trigger. diff --git a/tests/firestore-rules/rules.test.ts b/tests/firestore-rules/rules.test.ts index c747c580..ca5f1737 100644 --- a/tests/firestore-rules/rules.test.ts +++ b/tests/firestore-rules/rules.test.ts @@ -612,6 +612,15 @@ beforeAll(async () => { active: true, deletedAt: null, }); + // Target for the conjunct pins: never seated, so currentCargoGrantsEmpty() short-circuits + // true and each assertion is denied by the conjunct it actually names. + await setDoc(doc(db, "members/m_delegate_pins"), { + name: "Pins", + totalPoints: 0, + uid: "delegate-pins-uid", + active: true, + deletedAt: null, + }); await setDoc(doc(db, "members/m_delegate_cel"), { name: "Delegado CEL", totalPoints: 0, @@ -1116,6 +1125,56 @@ describe("firestore.rules — members", () => { ); }); + it("BLOCKING: a delegate may NOT create with a forged assignedBy", async () => { + // pos_soft is grant-free and non-CEL, so the cargo conjunct passes for ANY principal and + // assignedBySelf() is the sole denier. Without this the create lane's self-stamp had zero + // mutation coverage — every existing forgery case paired the forged uid with a power cargo, + // so the cargo conjunct denied it and the self-stamp could have been deleted silently. + // What it stops: a member born on a power cargo with a real Admin's uid as assignedBy, + // which the beacon trust gate would honor permanently — surviving revocation of the + // delegate's own code, because the assigner really is an Admin. + await assertFails( + setDoc(doc(createDelegate(), "members/new_delegate_forged"), { + name: "Ximena Paz", + totalPoints: 0, + ...BORN_LIVE, + positions: { [TERM]: { cargoId: "pos_soft", comisionIds: [], assignedBy: "admin-uid" } }, + }), + ); + }); + + it("BLOCKING: a delegate may NOT create with a ride-along NON-current term key", async () => { + // The create-lane twin of positionsDelta().hasOnly([currentTermKey()]). assignedBySelf() + // and cargoAssignableByNonAdmin() both read only positions[currentTermKey()], so a second + // term key rode along completely unvalidated: a clean current-term entry to pass the arm, + // plus a next-term power cargo attributed to a real Admin. On the UTC-year rollover + // claims-sync reads THAT entry and mints Admin onto a member whose login the creator + // controls. Forged attribution, one term deferred. + await assertFails( + setDoc(doc(createDelegate(), "members/new_delegate_ridealong"), { + name: "Ximena Paz", + totalPoints: 0, + ...BORN_LIVE, + positions: { + [TERM]: { cargoId: "pos_soft", comisionIds: [], assignedBy: "createdelegate-uid" }, + "2099": { cargoId: "pos1", comisionIds: [], assignedBy: "admin-uid" }, + }, + }), + ); + }); + + it("BLOCKING: create:Member alone, and update:BoardSeat alone, each reach nothing", async () => { + // The create-lane twin of the update-lane non-vacuity pin. update:BoardSeat widens WHICH + // cargo a creator may use; it does not make anyone a creator. + await assertFails( + setDoc(doc(plainDelegate(), "members/new_plaindelegate"), { + name: "Ximena Paz", + totalPoints: 0, + ...BORN_LIVE, + }), + ); + }); + it("BLOCKING: the same create principal WITHOUT update:BoardSeat is still denied both", async () => { // The paired denial — otherwise the two ALLOWs above would pass for any create:Member // holder and prove nothing about the new disjunct. @@ -3197,24 +3256,56 @@ describe("firestore.rules — member positions assignment", () => { // assignedBySelf(), the current-term restriction and hasOnly(['positions']) all sit // OUTSIDE the substituted disjunction. Pin that the delegation did not loosen them — // a forged assignedBy is what the beacon trust gate reads to decide whether to mint. + // + // m_delegate_pins, NOT m_delegate: the suite seeds once and never resets, and the + // power-cargo ALLOW above leaves m_delegate holding pos1. Every assertion here would then + // be denied by currentCargoGrantsEmpty() — the G3 guard — instead of by the conjunct it + // names, i.e. all three would pass with their own conjunct deleted. await assertFails( - updateDoc(doc(seatDelegate(), "members/m_delegate"), { + updateDoc(doc(seatDelegate(), "members/m_delegate_pins"), { [`positions.${TERM}`]: { cargoId: "pos_soft", comisionIds: [], assignedBy: "admin-uid" }, }), ); await assertFails( - updateDoc(doc(seatDelegate(), "members/m_delegate"), { - "positions.2099": { cargoId: "pos1", comisionIds: [], assignedBy: SEAT_DELEGATE }, + updateDoc(doc(seatDelegate(), "members/m_delegate_pins"), { + "positions.2099": { cargoId: "pos_soft", comisionIds: [], assignedBy: SEAT_DELEGATE }, }), ); await assertFails( - updateDoc(doc(seatDelegate(), "members/m_delegate"), { + updateDoc(doc(seatDelegate(), "members/m_delegate_pins"), { [`positions.${TERM}`]: { cargoId: "pos_soft", comisionIds: [], assignedBy: SEAT_DELEGATE }, name: "Renombrado", }), ); }); + it("BLOCKING: hasPerm is EXACT — manage:all and manage:BoardSeat are not board-seat delegates", async () => { + // boardSeatDelegate() uses hasPerm, not canDo, and until now nothing in the RULES pinned + // that — the property was tested at the client and callable layers only. Mutating it to + // canDo('update','BoardSeat') would hand board seating to every manage:all holder, which + // is reachable without the Admin role through a custom role doc or permissionOverrides. + const mgrAll = () => as("mgrall-uid", [], ["update:Position", "manage:all"]); + const mgrSeat = () => as("mgrseat-uid", [], ["update:Position", "manage:BoardSeat"]); + await assertFails( + updateDoc(doc(mgrAll(), "members/m_delegate_pins"), { + [`positions.${TERM}`]: { + cargoId: "pos_cel_free", + comisionIds: [], + assignedBy: "mgrall-uid", + }, + }), + ); + await assertFails( + updateDoc(doc(mgrSeat(), "members/m_delegate_pins"), { + [`positions.${TERM}`]: { + cargoId: "pos_cel_free", + comisionIds: [], + assignedBy: "mgrseat-uid", + }, + }), + ); + }); + it("REGRESSION: a non-delegate may still CLEAR a member off a grant-free CEL seat", async () => { // The deliberate asymmetry (firestore.rules' currentCargoGrantsEmpty comment): keeping a // grant-free CEL seat is denied to a non-Admin, but clearing one is allowed, or a takedown From 5f9408e5b48995df214521d123124fb64ea18baa Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 19:13:55 -0400 Subject: [PATCH 07/15] fix(beacon,backstage): refuse a delegate provisioning a power-seated member MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /security-review found a third takeover path, and the strongest: the adoption guard asked whether this was a NEW login, never WHOSE member doc it was. Every uid-less member is reachable by memberId — including one an Admin already seated on an Admin-granting cargo, which is the normal state between being seated and being invited. The delegate forges nothing. linkUid fires onMemberWritten, resolveTrustedGrants reads the STORED assignedBy (a genuine Admin), honors the grants, and mints Admin onto the account the call just created; the attacker then reaches it through the invite. So the mint is refused at the source: a non-Admin provision is denied when the member's current-term cargo carries grants, and fails closed when that cargo cannot be read. Suppressing the returned link would NOT have been sufficient on its own — with manage:Member the attacker rewrites members.email first, which the rules never pin, and the ordinary reset mail lands in their inbox. The link is withheld from non-Admin callers anyway as defence in depth; the drawer's copy-link fallback is hidden when there is no link rather than offering to copy "". Both guards mutation-tested: neutralizing each turns exactly its own tests red. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/member-invite-drawer.tsx | 46 +++++----- apps/beacon/src/provision-deps.ts | 9 ++ .../beacon/src/provision-member-login.test.ts | 86 ++++++++++++++++++- apps/beacon/src/provision-member-login.ts | 52 ++++++++++- docs/specs/board-seat-delegation.md | 15 ++++ 5 files changed, 185 insertions(+), 23 deletions(-) diff --git a/apps/backstage/src/features/members/components/member-invite-drawer.tsx b/apps/backstage/src/features/members/components/member-invite-drawer.tsx index 1bce8c2c..9641b177 100644 --- a/apps/backstage/src/features/members/components/member-invite-drawer.tsx +++ b/apps/backstage/src/features/members/components/member-invite-drawer.tsx @@ -79,7 +79,7 @@ export function MemberInviteDrawer({ try { const result = await onProvision(id); provisioned = true; - actionLink = result.actionLink; + actionLink = result.actionLink || null; try { await requestPasswordReset(data.email); emailSent = true; @@ -123,29 +123,35 @@ export function MemberInviteDrawer({ ) : done.provisioned && !done.emailSent ? ( <>

- El correo no se pudo enviar. Comparte el enlace de acceso manualmente. + {done.actionLink + ? "El correo no se pudo enviar. Comparte el enlace de acceso manualmente." + : "El correo no se pudo enviar. Pídele a un Admin que reenvíe la invitación."}

{done.errorDetail && (

Detalle: {done.errorDetail}

)} - - {copyState === "failed" && ( - - {done.actionLink} - + {done.actionLink && ( + <> + + {copyState === "failed" && ( + + {done.actionLink} + + )} + )} ) : ( diff --git a/apps/beacon/src/provision-deps.ts b/apps/beacon/src/provision-deps.ts index 15aa1175..14c52721 100644 --- a/apps/beacon/src/provision-deps.ts +++ b/apps/beacon/src/provision-deps.ts @@ -1,5 +1,7 @@ import type { Auth } from "firebase-admin/auth"; import type { Firestore } from "firebase-admin/firestore"; +import { isValidRole, type Role } from "@luminova/auth/roles"; +import { isSafeDocId } from "./firestore-util.js"; import type { ProvisionDeps } from "./provision-member-login.js"; // Null only for the "account does not exist" outcome — a transient Auth error @@ -25,5 +27,12 @@ export function firestoreProvisionDeps(db: Firestore, auth: Auth): ProvisionDeps await db.doc(`members/${id}`).update({ uid }); }, passwordResetLink: (email) => auth.generatePasswordResetLink(email), + getPositionGrants: async (cargoId) => { + if (!isSafeDocId(cargoId)) return null; + const snap = await db.doc(`positions/${cargoId}`).get(); + if (!snap.exists) return null; + const grants = (snap.data()?.grants ?? []) as unknown[]; + return grants.filter((g): g is Role => isValidRole(g)); + }, }; } diff --git a/apps/beacon/src/provision-member-login.test.ts b/apps/beacon/src/provision-member-login.test.ts index a33e4743..45973acf 100644 --- a/apps/beacon/src/provision-member-login.test.ts +++ b/apps/beacon/src/provision-member-login.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect } from "vitest"; +import type { Role } from "@luminova/auth/roles"; import { validateProvisionInput, nextClaims, @@ -35,6 +36,7 @@ describe("nextClaims", () => { function fakeDeps(opts: { member?: Record | null; usersByEmail?: Record; + positions?: Record; }) { const calls = { createUser: [] as string[], setClaims: [] as string[], linkUid: [] as string[] }; const users = opts.usersByEmail ?? {}; @@ -55,12 +57,14 @@ function fakeDeps(opts: { }, getUserByUid: async (uid) => Object.values(users).find((u) => u.uid === uid) ?? null, passwordResetLink: async (email) => `link:${email}`, + getPositionGrants: async (cargoId) => opts.positions?.[cargoId] ?? null, }; return { deps, calls }; } describe("provisionMember", () => { const active = { email: "a@b.co", active: true }; + const TERM = String(new Date().getUTCFullYear()); it("rejects when the member is already linked to a DIFFERENT live auth user (email changed)", async () => { const { deps, calls } = fakeDeps({ @@ -130,8 +134,9 @@ describe("provisionMember", () => { }); it("provisions an unlinked member, creating the auth user when absent", async () => { + // ADMIN caller: only an Admin receives the action link (see the delegate pair below). const { deps, calls } = fakeDeps({ member: active }); - const result = await provisionMember(deps, "m1"); + const result = await provisionMember(deps, "m1", true); expect(result).toEqual({ email: "a@b.co", actionLink: "link:a@b.co" }); expect(calls.createUser).toEqual(["a@b.co"]); expect(calls.setClaims).toEqual(["new-a@b.co"]); @@ -181,7 +186,7 @@ describe("provisionMember", () => { const fresh = fakeDeps({ member: active }); await expect(provisionMember(fresh.deps, "m1", false)).resolves.toEqual({ email: "a@b.co", - actionLink: "link:a@b.co", + actionLink: "", }); expect(fresh.calls.createUser).toEqual(["a@b.co"]); }); @@ -206,6 +211,83 @@ describe("provisionMember", () => { expect(calls.linkUid).toEqual([]); }); + it("BLOCKING: a non-Admin caller may NOT provision a POWER-SEATED member", async () => { + // The escalation this closes, and the delegate forges nothing to get it: any uid-less + // member is reachable — including one an Admin already seated on an Admin-granting cargo, + // which is the normal state between being seated and being invited. linkUid() fires + // onMemberWritten, resolveTrustedGrants reads the STORED assignedBy (a genuine Admin), + // honors the grants, and mints Admin onto the uid this call just created. The attacker + // then reaches that uid through the invite mail — which lands in THEIR inbox if they also + // hold manage:Member and rewrote members.email first, since the rules never pin it. + const { deps, calls } = fakeDeps({ + member: { + ...active, + positions: { [TERM]: { cargoId: "pos-pres", comisionIds: [], assignedBy: "admin-uid" } }, + }, + positions: { "pos-pres": ["Admin"] }, + }); + await expect(provisionMember(deps, "m1", false)).rejects.toMatchObject({ + code: "permission-denied", + details: { reason: "power-seat-requires-admin" }, + }); + expect(calls.createUser).toEqual([]); + expect(calls.linkUid).toEqual([]); + }); + + it("fails closed when the seated cargo cannot be read", async () => { + // A missing or malformed cargo must not read as "no cargo" — that would be the guard's + // own bypass. + const missing = fakeDeps({ + member: { + ...active, + positions: { [TERM]: { cargoId: "pos-ghost", comisionIds: [], assignedBy: "admin-uid" } }, + }, + }); + await expect(provisionMember(missing.deps, "m1", false)).rejects.toMatchObject({ + code: "permission-denied", + }); + const malformed = fakeDeps({ + member: { + ...active, + positions: { [TERM]: { cargoId: "a/b", comisionIds: [], assignedBy: "admin-uid" } }, + }, + }); + await expect(provisionMember(malformed.deps, "m1", false)).rejects.toMatchObject({ + code: "permission-denied", + }); + }); + + it("still lets a delegate provision a member seated on a GRANT-FREE cargo", async () => { + // Seating plus inviting on a grant-free cargo mints nothing, and is exactly the enrolment + // flow the delegation exists for. Without this pair the guard above would pass for a rule + // that simply refused every seated member. + const { deps, calls } = fakeDeps({ + member: { + ...active, + positions: { [TERM]: { cargoId: "pos-dir", comisionIds: [], assignedBy: "delegate-uid" } }, + }, + positions: { "pos-dir": [] }, + }); + await expect(provisionMember(deps, "m1", false)).resolves.toMatchObject({ email: "a@b.co" }); + expect(calls.createUser).toEqual(["a@b.co"]); + }); + + it("BLOCKING: a delegate never receives the password-reset link", async () => { + // generatePasswordResetLink returns a bearer credential for the account. The client sends + // the reset mail itself through the unprivileged sendPasswordResetEmail, so a delegate has + // no need to hold it. Defence in depth behind the power-seat guard, not a substitute. + const delegate = fakeDeps({ member: active }); + await expect(provisionMember(delegate.deps, "m1", false)).resolves.toEqual({ + email: "a@b.co", + actionLink: "", + }); + const admin = fakeDeps({ member: active }); + await expect(provisionMember(admin.deps, "m1", true)).resolves.toEqual({ + email: "a@b.co", + actionLink: "link:a@b.co", + }); + }); + it("BLOCKING: a non-Admin caller may NOT provision a member whose uid is set but account is gone", async () => { // The self-heal branch: linkedUid points at a deleted account, so getUserByEmail may // return null and the adoption half alone would let this through. A stored uid means an diff --git a/apps/beacon/src/provision-member-login.ts b/apps/beacon/src/provision-member-login.ts index c4512161..9077aa8b 100644 --- a/apps/beacon/src/provision-member-login.ts +++ b/apps/beacon/src/provision-member-login.ts @@ -2,6 +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 { currentTermKey } from "@luminova/types"; +import { isSafeDocId } from "./firestore-util.js"; import { callerIsAdmin, requireAdminOrPerm } from "./callable-auth.js"; import { firestoreProvisionDeps } from "./provision-deps.js"; import { ensureApp } from "./runtime.js"; @@ -47,6 +49,9 @@ export interface ProvisionDeps { setClaims(uid: string, claims: ReturnType): Promise; linkUid(memberId: string, uid: string): Promise; passwordResetLink(email: string): Promise; + /** The cargo's grants, or null if missing. Only consulted to refuse a non-Admin + * provisioning of a POWER-SEATED member — see the power-seat guard. */ + getPositionGrants(cargoId: string): Promise; } /** Claims carried over when adopting an Auth account not currently linked to @@ -61,6 +66,18 @@ function adoptedClaims(existing: RawClaims | undefined): RawClaims { return { roles }; } +/** The member's CURRENT-term cargo id, or null when absent/unusable. Fails closed on a + * malformed id: an unreadable cargo must not read as "no cargo". */ +function readCurrentCargoId(member: Record): string | null { + const positions = member.positions; + if (typeof positions !== "object" || positions === null) return null; + const term = (positions as Record)[currentTermKey()]; + if (typeof term !== "object" || term === null) return null; + const cargoId = (term as { cargoId?: unknown }).cargoId; + if (typeof cargoId !== "string" || cargoId.length === 0) return null; + return isSafeDocId(cargoId) ? cargoId : ""; +} + /** Provision (or re-provision) a member's login. Refuses to relink a member whose * stored uid does not match the Auth user its email resolves to — silently * overwriting would orphan the old Auth account with its claims (possibly Admin) @@ -131,6 +148,34 @@ export async function provisionMember( { reason: "reprovision-requires-admin" }, ); } + // POWER-SEAT GUARD. The check above asks whether this is a NEW login; it does not ask whose + // member doc it is, and "unprovisioned" does not mean "enrolled by this delegate". Any + // uid-less member is reachable, including one an Admin already seated on an Admin-granting + // cargo — the normal state between being seated and being invited. + // + // Without this guard that is a clean escalation, and the delegate forges nothing: linkUid() + // below fires onMemberWritten, resolveTrustedGrants reads the STORED assignedBy (a genuine + // Admin), honors the grants, and mints Admin onto the uid this call just created. The + // attacker then reaches that uid through the invite — either the returned actionLink, or, + // if they also hold manage:Member, by rewriting members.email first (the rules do not pin + // it) so the ordinary reset mail lands in their own inbox. Suppressing the link alone + // therefore does NOT close it; the mint has to be refused at the source. + // + // Grant-free cargos stay open: they mint nothing, so seating plus inviting is exactly the + // enrolment flow this delegation exists for. + if (!callerHoldsAdminRole) { + const cargoId = readCurrentCargoId(member); + if (cargoId !== null) { + const grants = await deps.getPositionGrants(cargoId); + if (grants === null || grants.length > 0) { + throw new HttpsError( + "permission-denied", + "this member holds a cargo that confers permissions; only an Admin can provision their login", + { reason: "power-seat-requires-admin" }, + ); + } + } + } if (!user) user = await deps.createUser(email); const targetEmail = user.email ?? email; @@ -146,7 +191,12 @@ export async function provisionMember( nextClaims(user.uid === linkedUid ? existingClaims : adoptedClaims(existingClaims), "Member"), ); await deps.linkUid(memberId, user.uid); - const actionLink = await deps.passwordResetLink(targetEmail); + // The link is generatePasswordResetLink's oobCode URL — a bearer credential for this + // account. An Admin gets it as the manual fallback the invite drawer offers when the mail + // fails; a delegate does not need it (the client sends the reset mail itself through the + // unprivileged sendPasswordResetEmail) and must not hold it. Defence in depth behind the + // power-seat guard, not a substitute for it. + const actionLink = callerHoldsAdminRole ? await deps.passwordResetLink(targetEmail) : ""; return { email: targetEmail, actionLink } as const; } diff --git a/docs/specs/board-seat-delegation.md b/docs/specs/board-seat-delegation.md index b3835e14..24491370 100644 --- a/docs/specs/board-seat-delegation.md +++ b/docs/specs/board-seat-delegation.md @@ -82,6 +82,21 @@ since the rules forbid it on every client lane) and the initial claim write. existing Auth account for their email, and no stored `uid`. Adoption, re-provision/resend, and the deleted-account self-heal are all Admin-only. +**And only when that member is not POWER-SEATED.** "Unprovisioned" does not mean "enrolled by +this delegate": every uid-less member is reachable by `memberId`, including one an Admin already +seated on an Admin-granting cargo — the normal state between being seated and being invited. +Linking a uid fires `onMemberWritten`, and `resolveTrustedGrants` reads the *stored* `assignedBy` +— a genuine Admin — so the grants are honored and Admin is minted onto the account this call +just created. The delegate forges nothing. So a non-Admin provision is refused whenever the +member's current-term cargo carries any grants, and fails closed when that cargo cannot be read. + +**A non-Admin caller never receives the `actionLink`.** `generatePasswordResetLink` returns a +bearer credential for the account; the client sends the invite itself through the unprivileged +`sendPasswordResetEmail`, so a delegate has no need to hold it. This is defence in depth behind +the power-seat guard, not a substitute for it — with `manage:Member` an attacker can rewrite +`members.email` first (the rules do not pin it) and receive the ordinary reset mail in their own +inbox, which suppressing the link alone would not stop. + The resend path is restricted for the same reason as adoption, and it is the less obvious one: `passwordResetLink` is `generatePasswordResetLink`, which returns the oobCode URL **to the caller** — categorically different from `sendPasswordResetEmail`, which delivers the secret to From b3533e53e20421d9e54af536186010afe6c2cce5 Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 19:41:32 -0400 Subject: [PATCH 08/15] fix(beacon): fail closed on a malformed positions shape; share the cargo read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verification pass found readCurrentCargoId treated a PRESENT but unreadable shape as "no cargo" and therefore ALLOWED a non-Admin provision — a non-object term entry, a non-string cargoId, or an empty one. That is the power-seat guard's own bypass, and it contradicted the docstring's fail-closed promise. Three outcomes now, explicitly: null for a genuinely absent cargo (allow), the id for a usable one, "" for present-but-unreadable (refuse, since "" fails isSafeDocId at the port too). readPositionGrants extracted: getPositionGrants and claims-sync's getPosition were the same isSafeDocId + read + isValidRole filter with different wrappers, and they must not drift — both decide whether a cargo confers power, one before minting claims and one before creating a login. Mutation-tested: restoring the pre-fix null returns turns the malformed-shape test red, and the paired "genuinely absent cargo is unseated" test keeps the guard from passing for a rule that just refuses every delegate. Co-Authored-By: Claude Opus 5 (1M context) --- apps/beacon/src/claims-sync/firestore-deps.ts | 13 +++---- apps/beacon/src/provision-deps.ts | 11 +----- .../beacon/src/provision-member-login.test.ts | 39 +++++++++++++++++++ apps/beacon/src/provision-member-login.ts | 24 +++++++++--- apps/beacon/src/read-position-grants.ts | 21 ++++++++++ docs/specs/board-seat-delegation.md | 10 ++++- 6 files changed, 96 insertions(+), 22 deletions(-) create mode 100644 apps/beacon/src/read-position-grants.ts diff --git a/apps/beacon/src/claims-sync/firestore-deps.ts b/apps/beacon/src/claims-sync/firestore-deps.ts index 0883df40..f81c4958 100644 --- a/apps/beacon/src/claims-sync/firestore-deps.ts +++ b/apps/beacon/src/claims-sync/firestore-deps.ts @@ -4,6 +4,7 @@ import { isValidRole, type Role } from "@luminova/auth/roles"; import { isValidPermissionCode, type PermissionCode } from "@luminova/types/permission"; import { chunk } from "../chunk.js"; import { isSafeDocId } from "../firestore-util.js"; +import { readPositionGrants } from "../read-position-grants.js"; import { isActiveRoleDoc, permsFromRoleDoc } from "./role-doc.js"; import type { LiveBuiltInRoleDoc } from "./resolve-member-perms.js"; import type { ClaimsSyncDeps } from "./sync.js"; @@ -276,13 +277,11 @@ export function firestoreClaimsDeps(db: Firestore, auth: Auth): FirestoreClaimsD }, getPosition: async (id) => { // Belt-and-braces: resolveTrustedGrants in sync.ts already screens cargoId (that is - // the port-independent gate the test fakes inherit), but this is the site where an - // unscreened id becomes a permanent db.doc() throw, so it does not rely on its caller. - if (!isSafeDocId(id)) return null; - const snap = await db.doc(`positions/${id}`).get(); - if (!snap.exists) return null; - const grants = (snap.data()?.grants ?? []) as unknown[]; - return { grants: grants.filter((g): g is Role => isValidRole(g)) }; + // the port-independent gate the test fakes inherit), but readPositionGrants screens + // again at the site where an unscreened id becomes a permanent db.doc() throw, so this + // does not rely on its caller. + const grants = await readPositionGrants(db, id); + return grants === null ? null : { grants }; }, getAssignerClaims: async (uid) => { // Same per-instance loadUser memo getExistingClaims uses — reading perms alongside diff --git a/apps/beacon/src/provision-deps.ts b/apps/beacon/src/provision-deps.ts index 14c52721..c030da31 100644 --- a/apps/beacon/src/provision-deps.ts +++ b/apps/beacon/src/provision-deps.ts @@ -1,7 +1,6 @@ import type { Auth } from "firebase-admin/auth"; import type { Firestore } from "firebase-admin/firestore"; -import { isValidRole, type Role } from "@luminova/auth/roles"; -import { isSafeDocId } from "./firestore-util.js"; +import { readPositionGrants } from "./read-position-grants.js"; import type { ProvisionDeps } from "./provision-member-login.js"; // Null only for the "account does not exist" outcome — a transient Auth error @@ -27,12 +26,6 @@ export function firestoreProvisionDeps(db: Firestore, auth: Auth): ProvisionDeps await db.doc(`members/${id}`).update({ uid }); }, passwordResetLink: (email) => auth.generatePasswordResetLink(email), - getPositionGrants: async (cargoId) => { - if (!isSafeDocId(cargoId)) return null; - const snap = await db.doc(`positions/${cargoId}`).get(); - if (!snap.exists) return null; - const grants = (snap.data()?.grants ?? []) as unknown[]; - return grants.filter((g): g is Role => isValidRole(g)); - }, + getPositionGrants: (cargoId) => readPositionGrants(db, cargoId), }; } diff --git a/apps/beacon/src/provision-member-login.test.ts b/apps/beacon/src/provision-member-login.test.ts index 45973acf..728d357b 100644 --- a/apps/beacon/src/provision-member-login.test.ts +++ b/apps/beacon/src/provision-member-login.test.ts @@ -234,6 +234,45 @@ describe("provisionMember", () => { expect(calls.linkUid).toEqual([]); }); + it("fails closed on a PRESENT but malformed positions shape", async () => { + // The guard's own bypass if these read as "no cargo". None is produced by a client write + // path — assignedBySelf() errors on a non-object term and the rules deny — but a console + // edit or a partial migration reaches them, and the whole point of the guard is that an + // unreadable cargo is not an absent one. + const shapes: Record[] = [ + { positions: "not-an-object" }, + { positions: { [TERM]: "not-an-object" } }, + { positions: { [TERM]: { cargoId: 42, comisionIds: [] } } }, + { positions: { [TERM]: { cargoId: "", comisionIds: [] } } }, + ]; + for (const positions of shapes) { + const { deps, calls } = fakeDeps({ member: { ...active, ...positions } }); + await expect(provisionMember(deps, "m1", false)).rejects.toMatchObject({ + code: "permission-denied", + details: { reason: "power-seat-requires-admin" }, + }); + expect(calls.createUser).toEqual([]); + } + }); + + it("treats a genuinely ABSENT cargo as unseated, not as malformed", async () => { + // The paired ALLOW. Without it the fail-closed test above would pass for a guard that + // simply refused every non-Admin provision, which is the whole delegation. + const shapes: Record[] = [ + {}, + { positions: {} }, + { positions: { [TERM]: { comisionIds: [] } } }, + { positions: { [TERM]: { cargoId: null, comisionIds: [] } } }, + { positions: { "1999": { cargoId: "pos-pres", comisionIds: [] } } }, + ]; + for (const positions of shapes) { + const { deps } = fakeDeps({ member: { ...active, ...positions } }); + await expect(provisionMember(deps, "m1", false)).resolves.toMatchObject({ + email: "a@b.co", + }); + } + }); + it("fails closed when the seated cargo cannot be read", async () => { // A missing or malformed cargo must not read as "no cargo" — that would be the guard's // own bypass. diff --git a/apps/beacon/src/provision-member-login.ts b/apps/beacon/src/provision-member-login.ts index 9077aa8b..a74656ce 100644 --- a/apps/beacon/src/provision-member-login.ts +++ b/apps/beacon/src/provision-member-login.ts @@ -66,15 +66,29 @@ function adoptedClaims(existing: RawClaims | undefined): RawClaims { return { roles }; } -/** The member's CURRENT-term cargo id, or null when absent/unusable. Fails closed on a - * malformed id: an unreadable cargo must not read as "no cargo". */ +/** The member's CURRENT-term cargo id for the power-seat guard. + * + * Three outcomes, and the middle one is the point: + * null — genuinely NO current-term cargo (no positions map, no entry for this term, or an + * entry whose cargoId is absent/null). The guard allows: an unseated member is + * exactly who a delegate is meant to be enrolling. + * id — a usable cargo id; the guard reads its grants. + * "" — PRESENT but unreadable: a term entry that is not an object, a cargoId that is not + * a string, or an id `isSafeDocId` rejects. Deliberately NOT null — `""` fails + * `isSafeDocId` at the port too, so `getPositionGrants` returns null and the guard + * refuses. A malformed shape must never read as "no cargo": that is the guard's own + * bypass, and these docs are reachable by console edit and legacy migration even + * though no client write path produces them. */ function readCurrentCargoId(member: Record): string | null { const positions = member.positions; - if (typeof positions !== "object" || positions === null) return null; + if (positions === undefined || positions === null) return null; + if (typeof positions !== "object") return ""; const term = (positions as Record)[currentTermKey()]; - if (typeof term !== "object" || term === null) return null; + if (term === undefined || term === null) return null; + if (typeof term !== "object") return ""; const cargoId = (term as { cargoId?: unknown }).cargoId; - if (typeof cargoId !== "string" || cargoId.length === 0) return null; + if (cargoId === undefined || cargoId === null) return null; + if (typeof cargoId !== "string" || cargoId.length === 0) return ""; return isSafeDocId(cargoId) ? cargoId : ""; } diff --git a/apps/beacon/src/read-position-grants.ts b/apps/beacon/src/read-position-grants.ts new file mode 100644 index 00000000..4af30da1 --- /dev/null +++ b/apps/beacon/src/read-position-grants.ts @@ -0,0 +1,21 @@ +import type { Firestore } from "firebase-admin/firestore"; +import { isValidRole, type Role } from "@luminova/auth/roles"; +import { isSafeDocId } from "./firestore-util.js"; + +/** A cargo's trusted grants, or null when the id is unusable or the doc is missing. + * + * Two ports need exactly this — the claims-sync trust gate (`getPosition`) and the callable + * power-seat guard (`getPositionGrants`) — and they must not drift: both decide whether a + * cargo confers power, one before minting claims and one before creating a login. They + * differ only in their wrapper shape, so the read lives here and each adapter wraps it. + * + * `null` is the fail-closed answer for BOTH callers: an unreadable cargo is treated as + * power-conferring by the guard and as grant-free by the trust gate, which is the safe + * direction in each. */ +export async function readPositionGrants(db: Firestore, id: unknown): Promise { + if (!isSafeDocId(id)) return null; + const snap = await db.doc(`positions/${id}`).get(); + if (!snap.exists) return null; + const grants = (snap.data()?.grants ?? []) as unknown[]; + return grants.filter((g): g is Role => isValidRole(g)); +} diff --git a/docs/specs/board-seat-delegation.md b/docs/specs/board-seat-delegation.md index 24491370..52ebbd14 100644 --- a/docs/specs/board-seat-delegation.md +++ b/docs/specs/board-seat-delegation.md @@ -125,7 +125,15 @@ This costs the delegation nothing: a genuinely new member has neither an account 4. **The `PERMISSION_CAP` interaction.** A member whose resolved perms exceed 30 is written `perms: []` fail-closed, which silently takes `update:BoardSeat` with it. Two more subjects in the vocabulary make the 30-slot budget marginally tighter. -5. **There is a consistency window.** `firestore.rules` reads the token while beacon reads stored +5. **Verify who you seat.** One residual no guard can close, and it is not specific to this + delegation — it is why enrolment and seating should not both be delegated blindly. A member + creator controls the `email` on the doc they file, and the invite goes to that address. So if + an Admin later seats a FABRICATED member on an Admin-granting cargo, the claim is minted onto + an account the fabricator controls. The chain needs an Admin to seat someone they did not + verify; it existed before this feature (an Admin provisioning the same fabricated doc sends + the invite to the same attacker address), and the power-seat guard means a delegate cannot + complete it alone. Treat the members list as the thing you verify before seating. +6. **There is a consistency window.** `firestore.rules` reads the token while beacon reads stored claims. If a delegate's own perms are dropped (cap breach, or revocation) their cached token still passes the rules for up to an hour, so a seat write can succeed while `resolveTrustedGrants` declines to mint the grants. The member is then published on the public From 89921aa70bc1f3daf3368d8c6c132d7628ae5c6b Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 19:45:10 -0400 Subject: [PATCH 09/15] test(rules): make the term-restriction pins load-bearing; de-escape a comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Delta re-review found the retargeted "past term" pin was still over-determined: on an unseated fixture a lone 2099 write is denied by assignedBySelf() (the post-merge current term is {}), so the term conjunct was not the denier and the comment claiming otherwise was a guardrail #6 claim with nothing behind it. Now paired with a valid current-term entry so only positionsDelta().hasOnly() denies. Added the missing Admin pin on the create arm. The term restriction binds every principal, and the tempting future edit is `hasAnyRole(['Admin']) ||` in front of it the first time a migration looks blocked — which silently reopens the ride-along for anyone who can get an Admin to run a create. Nothing went red on that mutation before; now exactly this test does. Also de-escaped HTML entities that leaked into a firestore.rules comment. Co-Authored-By: Claude Opus 5 (1M context) --- firestore.rules | 4 ++-- tests/firestore-rules/rules.test.ts | 27 ++++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/firestore.rules b/firestore.rules index 8f81d747..9c54e93f 100644 --- a/firestore.rules +++ b/firestore.rules @@ -248,8 +248,8 @@ service cloud.firestore { // assignedTerm()), so every OTHER term key in a created map was entirely unvalidated. The // update arm closes this — see positionsDelta().hasOnly() and its comment — and the create // arm has to say the same thing or the ride-along just moves to birth: - // positions: { "<this year>": <grant-free, self-stamped: passes>, - // "<next year>": { cargoId: <Presidente>, assignedBy: <a real Admin uid> } } + // positions: { "": , + // "": { cargoId: , assignedBy: } } // Nothing inspects the second entry. On the UTC-year rollover claims-sync reads it, sees a // genuine Admin as assignedBy, and mints Admin onto a member whose email — and therefore // whose login — the creator controls. Forged attribution, one term deferred; the trust gate diff --git a/tests/firestore-rules/rules.test.ts b/tests/firestore-rules/rules.test.ts index ca5f1737..031564f0 100644 --- a/tests/firestore-rules/rules.test.ts +++ b/tests/firestore-rules/rules.test.ts @@ -1143,6 +1143,25 @@ describe("firestore.rules — members", () => { ); }); + it("BLOCKING: the create-lane term restriction binds an ADMIN too", async () => { + // Symmetric with the update arm, which has denied non-current-term writes to everyone + // including Admin since before this branch. Stated as its own test because the tempting + // future edit is `hasAnyRole(['Admin']) ||` in front of the conjunct the first time a + // migration looks blocked — and that silently reopens the ride-along for anyone who can + // get an Admin to run a create. + await assertFails( + setDoc(doc(as("admin-uid", ["Admin"]), "members/new_admin_ridealong"), { + name: "Ximena Paz", + totalPoints: 0, + ...BORN_LIVE, + positions: { + [TERM]: { cargoId: "pos_soft", comisionIds: [], assignedBy: "admin-uid" }, + "2099": { cargoId: "pos1", comisionIds: [], assignedBy: "admin-uid" }, + }, + }), + ); + }); + it("BLOCKING: a delegate may NOT create with a ride-along NON-current term key", async () => { // The create-lane twin of positionsDelta().hasOnly([currentTermKey()]). assignedBySelf() // and cargoAssignableByNonAdmin() both read only positions[currentTermKey()], so a second @@ -3266,9 +3285,15 @@ describe("firestore.rules — member positions assignment", () => { [`positions.${TERM}`]: { cargoId: "pos_soft", comisionIds: [], assignedBy: "admin-uid" }, }), ); + // Paired with a VALID current-term entry on purpose. A lone 2099 write on an unseated doc + // is already denied by assignedBySelf() — the post-merge current term is {} — so the term + // conjunct would not be the denier and the assertion would pin nothing. With both keys + // present, assignedBySelf(), cargoAssignableByNonAdmin() and currentCargoGrantsEmpty() all + // pass and only positionsDelta().hasOnly([currentTermKey()]) denies. await assertFails( updateDoc(doc(seatDelegate(), "members/m_delegate_pins"), { - "positions.2099": { cargoId: "pos_soft", comisionIds: [], assignedBy: SEAT_DELEGATE }, + [`positions.${TERM}`]: { cargoId: "pos_soft", comisionIds: [], assignedBy: SEAT_DELEGATE }, + "positions.2099": { cargoId: "pos1", comisionIds: [], assignedBy: "admin-uid" }, }), ); await assertFails( From e02dbf1a96192733dd6506db80952fa70891b51b Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 19:54:44 -0400 Subject: [PATCH 10/15] fix(beacon): refuse a delegate provisioning a member carrying direct grants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final security pass found the power-seat guard closed only one of the two claims-mint sources. syncMemberClaims mints `roles` from trusted cargo grants AND `perms` from roleIds + permissionOverrides via resolveMemberPerms — the second path needs no cargo at all, and it is precisely what the Admin-only roles panel writes, so "granted but not yet invited" is as ordinary a state as "seated but not yet invited". Exploit it closes: a manage:Member + create:MemberLogin holder finds an un-invited member an Admin granted a custom role or an override to, rewrites their email (the rules never pin it), provisions them, and the account they now control is minted that member's entire granted perm set — which may itself include update:BoardSeat, chaining into the seating lane. hasDirectGrants fails closed on any shape that is not a clean empty, while absent / null / [] stay ungranted: the rules' unchanged()/touched() gap admits an explicit null and parseMember resolves it to [], so treating null as malformed would make ordinary members un-invitable by a delegate. Mutation-tested both ways: dropping the guard and softening its fail-closed branches each turn exactly their own tests red. Co-Authored-By: Claude Opus 5 (1M context) --- .../beacon/src/provision-member-login.test.ts | 60 +++++++++++++++++++ apps/beacon/src/provision-member-login.ts | 43 ++++++++++++- docs/specs/board-seat-delegation.md | 8 +++ 3 files changed, 109 insertions(+), 2 deletions(-) diff --git a/apps/beacon/src/provision-member-login.test.ts b/apps/beacon/src/provision-member-login.test.ts index 728d357b..4255f55a 100644 --- a/apps/beacon/src/provision-member-login.test.ts +++ b/apps/beacon/src/provision-member-login.test.ts @@ -234,6 +234,66 @@ describe("provisionMember", () => { expect(calls.linkUid).toEqual([]); }); + it("BLOCKING: a non-Admin caller may NOT provision a member carrying DIRECT grants", async () => { + // The other half of the claims-mint surface. syncMemberClaims mints `roles` from trusted + // cargo grants AND `perms` from roleIds + permissionOverrides — the second path needs no + // cargo at all, and "granted but not yet invited" is exactly what the Admin-only roles + // panel produces. Without this, a manage:Member + create:MemberLogin holder rewrites such + // a member's email (the rules never pin it), provisions them, and the account they now + // control is minted that member's whole granted perm set — which may itself include + // update:BoardSeat, chaining into the seating lane. + const granted: Record[] = [ + { roleIds: ["custom-role"] }, + { permissionOverrides: { grant: ["update:BoardSeat"], revoke: [] } }, + ]; + for (const fields of granted) { + const { deps, calls } = fakeDeps({ member: { ...active, ...fields } }); + await expect(provisionMember(deps, "m1", false)).rejects.toMatchObject({ + code: "permission-denied", + details: { reason: "granted-member-requires-admin" }, + }); + expect(calls.createUser).toEqual([]); + expect(calls.linkUid).toEqual([]); + } + }); + + it("fails closed on a PRESENT but malformed grants shape", async () => { + const malformed: Record[] = [ + { roleIds: "custom-role" }, + { roleIds: {} }, + { permissionOverrides: "nope" }, + { permissionOverrides: { grant: "update:BoardSeat" } }, + ]; + for (const fields of malformed) { + const { deps } = fakeDeps({ member: { ...active, ...fields } }); + await expect(provisionMember(deps, "m1", false)).rejects.toMatchObject({ + code: "permission-denied", + details: { reason: "granted-member-requires-admin" }, + }); + } + }); + + it("treats empty / absent / explicitly-null grants as ungranted", async () => { + // The paired ALLOW. The rules' unchanged()/touched() gap admits an explicit null, and + // parseMember resolves that to [] — so null must read as "no grants", not as malformed, + // or ordinary members become un-invitable by a delegate. + const ungranted: Record[] = [ + {}, + { roleIds: [] }, + { roleIds: null }, + { permissionOverrides: null }, + { permissionOverrides: { grant: [], revoke: [] } }, + { permissionOverrides: { revoke: ["read:Member"] } }, + { roleIds: [], permissionOverrides: { grant: [], revoke: [] } }, + ]; + for (const fields of ungranted) { + const { deps } = fakeDeps({ member: { ...active, ...fields } }); + await expect(provisionMember(deps, "m1", false)).resolves.toMatchObject({ + email: "a@b.co", + }); + } + }); + it("fails closed on a PRESENT but malformed positions shape", async () => { // The guard's own bypass if these read as "no cargo". None is produced by a client write // path — assignedBySelf() errors on a non-object term and the rules deny — but a console diff --git a/apps/beacon/src/provision-member-login.ts b/apps/beacon/src/provision-member-login.ts index a74656ce..7f7093f0 100644 --- a/apps/beacon/src/provision-member-login.ts +++ b/apps/beacon/src/provision-member-login.ts @@ -66,6 +66,33 @@ function adoptedClaims(existing: RawClaims | undefined): RawClaims { return { roles }; } +/** Whether this member carries DIRECT grants — a custom role or a per-member override. + * + * The second half of the privileged-member question. syncMemberClaims mints from two + * independent sources: trusted cargo grants become `roles`, and `roleIds` + + * `permissionOverrides` become `perms` (resolveMemberPerms). A guard that reads only the + * cargo half leaves the other wide open — and `roleIds`/`permissionOverrides` are exactly + * what the Admin-only panel writes, so "granted but not yet invited" is as ordinary a state + * as "seated but not yet invited". + * + * Fail-closed on any shape that is not a clean empty: a present-but-unparseable `roleIds` + * must refuse, not read as "no grants". Absent and null are the genuine empties — the rules' + * unchanged()/touched() gap admits an explicit null, and parseMember resolves that to []. */ +function hasDirectGrants(member: Record): boolean { + const roleIds = member.roleIds; + if (roleIds !== undefined && roleIds !== null) { + if (!Array.isArray(roleIds)) return true; + if (roleIds.length > 0) return true; + } + const overrides = member.permissionOverrides; + if (overrides === undefined || overrides === null) return false; + if (typeof overrides !== "object") return true; + const grant = (overrides as { grant?: unknown }).grant; + if (grant === undefined || grant === null) return false; + if (!Array.isArray(grant)) return true; + return grant.length > 0; +} + /** The member's CURRENT-term cargo id for the power-seat guard. * * Three outcomes, and the middle one is the point: @@ -175,9 +202,21 @@ export async function provisionMember( // it) so the ordinary reset mail lands in their own inbox. Suppressing the link alone // therefore does NOT close it; the mint has to be refused at the source. // - // Grant-free cargos stay open: they mint nothing, so seating plus inviting is exactly the - // enrolment flow this delegation exists for. + // Both halves of the claims-mint surface are checked, mirroring how syncMemberClaims splits + // it: hasDirectGrants() for the roleIds/permissionOverrides -> perms path, and the cargo + // read below for the grants -> roles path. Closing only one leaves the other reachable. + // + // Grant-free, un-granted members stay open: they mint nothing, so enrolling and inviting + // them is exactly the flow this delegation exists for. if (!callerHoldsAdminRole) { + // Direct grants first — no read required, and it is the half a cargo check cannot see. + if (hasDirectGrants(member)) { + throw new HttpsError( + "permission-denied", + "this member has been granted roles or permissions; only an Admin can provision their login", + { reason: "granted-member-requires-admin" }, + ); + } const cargoId = readCurrentCargoId(member); if (cargoId !== null) { const grants = await deps.getPositionGrants(cargoId); diff --git a/docs/specs/board-seat-delegation.md b/docs/specs/board-seat-delegation.md index 52ebbd14..5acee82d 100644 --- a/docs/specs/board-seat-delegation.md +++ b/docs/specs/board-seat-delegation.md @@ -82,6 +82,14 @@ since the rules forbid it on every client lane) and the initial claim write. existing Auth account for their email, and no stored `uid`. Adoption, re-provision/resend, and the deleted-account self-heal are all Admin-only. +**And only when that member carries no grants of either kind.** `syncMemberClaims` mints from +two independent sources, so the guard has to ask about both: + +- *Direct grants* — `roleIds` and `permissionOverrides` become `perms`, with no cargo involved. + These are exactly what the Admin-only roles panel writes, so "granted but not yet invited" is + as ordinary a state as "seated but not yet invited". +- *Cargo grants* — the current-term cargo's `grants` become `roles`. + **And only when that member is not POWER-SEATED.** "Unprovisioned" does not mean "enrolled by this delegate": every uid-less member is reachable by `memberId`, including one an Admin already seated on an Admin-granting cargo — the normal state between being seated and being invited. From d7cd564b45725f145b8301ee07b9a061c72bbc0f Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 20:01:54 -0400 Subject: [PATCH 11/15] fix(beacon): power-seat guard checks every term, not just the current one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final verification found the last gap: syncMemberClaims reads positions[currentTermKey()] at TRIGGER time, so a future-term power cargo was invisible to the guard and would mint on the UTC-year rollover — a genuine Admin in assignedBy, the cargo's grants honored, onto an account a delegate caused to exist and whose mailbox they control. Every client write lane is term-pinned (positionsDelta().hasOnly() on update, keys().hasOnly() on create, both binding Admins), so the shape needs a console edit, an admin-SDK write or a legacy migration — the same reachability this file already fail-closes on for a malformed cargoId, and a console-authored next-term board slate is the more plausible of the two. readCargoIds now yields every cargo in the map, deduped, with "" for any present-but-unreadable entry so the port's isSafeDocId still refuses it. Mutation-tested: restricting the loop back to the current term turns exactly the future-term test red. Co-Authored-By: Claude Opus 5 (1M context) --- .../beacon/src/provision-member-login.test.ts | 42 ++++++++++++- apps/beacon/src/provision-member-login.ts | 62 +++++++++++-------- docs/specs/board-seat-delegation.md | 4 +- 3 files changed, 80 insertions(+), 28 deletions(-) diff --git a/apps/beacon/src/provision-member-login.test.ts b/apps/beacon/src/provision-member-login.test.ts index 4255f55a..8e273687 100644 --- a/apps/beacon/src/provision-member-login.test.ts +++ b/apps/beacon/src/provision-member-login.test.ts @@ -323,16 +323,54 @@ describe("provisionMember", () => { { positions: {} }, { positions: { [TERM]: { comisionIds: [] } } }, { positions: { [TERM]: { cargoId: null, comisionIds: [] } } }, - { positions: { "1999": { cargoId: "pos-pres", comisionIds: [] } } }, + { positions: { "1999": { cargoId: "pos-unknown-but-grantfree", comisionIds: [] } } }, ]; for (const positions of shapes) { - const { deps } = fakeDeps({ member: { ...active, ...positions } }); + const { deps } = fakeDeps({ + member: { ...active, ...positions }, + positions: { "pos-unknown-but-grantfree": [] }, + }); await expect(provisionMember(deps, "m1", false)).resolves.toMatchObject({ email: "a@b.co", }); } }); + it("BLOCKING: a FUTURE-term power cargo is refused too, not just the current term", async () => { + // syncMemberClaims reads positions[currentTermKey()] at TRIGGER time, so a next-term entry + // is invisible today and mints on the UTC-year rollover — a genuine Admin in assignedBy, + // the cargo's grants honored, onto an account a delegate caused to exist. Every client + // lane is term-pinned so the shape needs a console edit or a migration, which is the same + // reachability this file already fail-closes on for a malformed cargoId. + const { deps, calls } = fakeDeps({ + member: { + ...active, + positions: { "2099": { cargoId: "pos-pres", comisionIds: [], assignedBy: "admin-uid" } }, + }, + positions: { "pos-pres": ["Admin"] }, + }); + await expect(provisionMember(deps, "m1", false)).rejects.toMatchObject({ + code: "permission-denied", + details: { reason: "power-seat-requires-admin" }, + }); + expect(calls.createUser).toEqual([]); + }); + + it("still allows a delegate when EVERY term's cargo is grant-free", async () => { + const { deps, calls } = fakeDeps({ + member: { + ...active, + positions: { + [TERM]: { cargoId: "pos-dir", comisionIds: [], assignedBy: "delegate-uid" }, + "2099": { cargoId: "pos-dir2", comisionIds: [], assignedBy: "delegate-uid" }, + }, + }, + positions: { "pos-dir": [], "pos-dir2": [] }, + }); + await expect(provisionMember(deps, "m1", false)).resolves.toMatchObject({ email: "a@b.co" }); + expect(calls.createUser).toEqual(["a@b.co"]); + }); + it("fails closed when the seated cargo cannot be read", async () => { // A missing or malformed cargo must not read as "no cargo" — that would be the guard's // own bypass. diff --git a/apps/beacon/src/provision-member-login.ts b/apps/beacon/src/provision-member-login.ts index 7f7093f0..fd6cd4ef 100644 --- a/apps/beacon/src/provision-member-login.ts +++ b/apps/beacon/src/provision-member-login.ts @@ -2,7 +2,6 @@ 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 { currentTermKey } from "@luminova/types"; import { isSafeDocId } from "./firestore-util.js"; import { callerIsAdmin, requireAdminOrPerm } from "./callable-auth.js"; import { firestoreProvisionDeps } from "./provision-deps.js"; @@ -93,30 +92,44 @@ function hasDirectGrants(member: Record): boolean { return grant.length > 0; } -/** The member's CURRENT-term cargo id for the power-seat guard. +/** Every cargo id in the member's positions map, for the power-seat guard. * - * Three outcomes, and the middle one is the point: - * null — genuinely NO current-term cargo (no positions map, no entry for this term, or an - * entry whose cargoId is absent/null). The guard allows: an unseated member is - * exactly who a delegate is meant to be enrolling. - * id — a usable cargo id; the guard reads its grants. - * "" — PRESENT but unreadable: a term entry that is not an object, a cargoId that is not - * a string, or an id `isSafeDocId` rejects. Deliberately NOT null — `""` fails - * `isSafeDocId` at the port too, so `getPositionGrants` returns null and the guard - * refuses. A malformed shape must never read as "no cargo": that is the guard's own - * bypass, and these docs are reachable by console edit and legacy migration even - * though no client write path produces them. */ -function readCurrentCargoId(member: Record): string | null { + * EVERY term, not just the current one — and that is the point. `syncMemberClaims` reads + * `positions[currentTermKey()]` at TRIGGER time, so a future-term entry is invisible today + * and mints on the UTC-year rollover. All client write lanes are term-pinned + * (`positionsDelta().hasOnly()` on update, `keys().hasOnly()` on create, both binding Admins + * too), so such a map takes a console edit, an admin-SDK write or a legacy migration — the + * same reachability this file already fail-closes on for a malformed cargoId, and a + * console-authored next-term board slate is the more plausible of the two. + * + * Yields: + * a usable id — read its grants. + * "" — present but unreadable (a non-object entry, a non-string or empty + * cargoId, or an id `isSafeDocId` rejects). Deliberately NOT skipped: "" + * fails `isSafeDocId` at the port too, so the guard refuses. A malformed + * shape must never read as "no cargo" — that is the guard's own bypass. + * A genuinely absent cargo (no map, no entry, or `cargoId` absent/null) yields nothing, so an + * unseated member produces an empty list and the delegate may enrol them. */ +function readCargoIds(member: Record): string[] { const positions = member.positions; - if (positions === undefined || positions === null) return null; - if (typeof positions !== "object") return ""; - const term = (positions as Record)[currentTermKey()]; - if (term === undefined || term === null) return null; - if (typeof term !== "object") return ""; - const cargoId = (term as { cargoId?: unknown }).cargoId; - if (cargoId === undefined || cargoId === null) return null; - if (typeof cargoId !== "string" || cargoId.length === 0) return ""; - return isSafeDocId(cargoId) ? cargoId : ""; + if (positions === undefined || positions === null) return []; + if (typeof positions !== "object") return [""]; + const ids: string[] = []; + for (const term of Object.values(positions as Record)) { + if (term === undefined || term === null) continue; + if (typeof term !== "object") { + ids.push(""); + continue; + } + const cargoId = (term as { cargoId?: unknown }).cargoId; + if (cargoId === undefined || cargoId === null) continue; + if (typeof cargoId !== "string" || cargoId.length === 0) { + ids.push(""); + continue; + } + ids.push(isSafeDocId(cargoId) ? cargoId : ""); + } + return [...new Set(ids)]; } /** Provision (or re-provision) a member's login. Refuses to relink a member whose @@ -217,8 +230,7 @@ export async function provisionMember( { reason: "granted-member-requires-admin" }, ); } - const cargoId = readCurrentCargoId(member); - if (cargoId !== null) { + for (const cargoId of readCargoIds(member)) { const grants = await deps.getPositionGrants(cargoId); if (grants === null || grants.length > 0) { throw new HttpsError( diff --git a/docs/specs/board-seat-delegation.md b/docs/specs/board-seat-delegation.md index 5acee82d..4972e194 100644 --- a/docs/specs/board-seat-delegation.md +++ b/docs/specs/board-seat-delegation.md @@ -88,7 +88,9 @@ two independent sources, so the guard has to ask about both: - *Direct grants* — `roleIds` and `permissionOverrides` become `perms`, with no cargo involved. These are exactly what the Admin-only roles panel writes, so "granted but not yet invited" is as ordinary a state as "seated but not yet invited". -- *Cargo grants* — the current-term cargo's `grants` become `roles`. +- *Cargo grants* — a cargo's `grants` become `roles`. The guard checks EVERY term in the map, + not just the current one: `syncMemberClaims` reads the current term at trigger time, so a + future-term entry is invisible today and mints on the UTC-year rollover. **And only when that member is not POWER-SEATED.** "Unprovisioned" does not mean "enrolled by this delegate": every uid-less member is reachable by `memberId`, including one an Admin already From 2c328f551f4f58f4d0d4949810965d88912171a1 Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 20:12:06 -0400 Subject: [PATCH 12/15] fix(beacon,backstage): don't die on a malformed grants field; wire the delegate invite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Functions re-review, both Mediums: M1 — readPositionGrants called .filter on snap.data().grants, which is a TypeError when that field is a string or a map. firestore.rules short-circuits every grants check on hasAnyRole(['Admin']) and never type-checks the field, so a console edit or a migration can store one. onMemberWritten is retry:false and the bad value persists, so the throw would kill claims sync for every member seated on that cargo, permanently and silently. Returns null now — fail-closed for both callers. The module had no test file; it has one. M2 — member-profile-page's InviteAccess still assumed an actionLink. For a delegate (who is withheld one) it opened a dialog over an empty code block with a copy button that copied nothing, having already created the account. It now sends the reset mail itself, exactly as the invite drawer does. Also: validateProvisionInput uses isSafeDocId rather than a weaker hand-rolled subset; createUser's catch is narrowed to auth/email-already-exists instead of swallowing quota and invalid-email errors; the post-createUser recovery comment now says a delegate's retry is refused; a duplicated comment fragment removed. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/member-profile-page.tsx | 28 ++++++++- apps/beacon/src/claims-sync/sync.ts | 2 +- apps/beacon/src/provision-deps.ts | 10 +++- apps/beacon/src/provision-member-login.ts | 11 +++- apps/beacon/src/read-position-grants.test.ts | 60 +++++++++++++++++++ apps/beacon/src/read-position-grants.ts | 14 ++++- docs/specs/board-seat-delegation.md | 12 ++-- 7 files changed, 123 insertions(+), 14 deletions(-) create mode 100644 apps/beacon/src/read-position-grants.test.ts diff --git a/apps/backstage/src/features/members/components/member-profile-page.tsx b/apps/backstage/src/features/members/components/member-profile-page.tsx index 24e2c333..f4d39087 100644 --- a/apps/backstage/src/features/members/components/member-profile-page.tsx +++ b/apps/backstage/src/features/members/components/member-profile-page.tsx @@ -16,6 +16,7 @@ import { useActivitiesByTerm } from "../../activities/hooks/use-activities-by-te import { useInitiativesByTerm } from "../../initiatives/hooks/use-initiatives-by-term"; import { summarizeParticipations } from "../lib/participation-summary"; import { pointsRank } from "../../../lib/points-rank"; +import { requestPasswordReset } from "../../../lib/auth/request-password-reset"; import { useProvisionMemberLogin } from "../hooks/use-provision-member-login"; import { useUpdateMember } from "../hooks/use-update-member"; import { useSetMemberPositions } from "../hooks/use-set-member-positions"; @@ -226,15 +227,33 @@ function InviteAccess({ member }: { member: Member }) { const provision = useProvisionMemberLogin(); const [open, setOpen] = useState(false); const [link, setLink] = useState(null); + const [sent, setSent] = useState(false); const [error, setError] = useState(null); const label = member.uid ? "Reenviar acceso" : "Invitar acceso"; + // beacon withholds the action link from a non-Admin caller (it is a bearer credential for + // the account). The client then does what the invite drawer already does — send the reset + // mail itself through the unprivileged sendPasswordResetEmail — so a delegate's invite + // still lands. Without this the delegate got an empty code block and a copy button that + // copied nothing, with the account already created and no way to set a password. const invite = () => { setError(null); provision.mutate(member.id, { onSuccess: (result) => { - setLink(result.actionLink); - setOpen(true); + if (result.actionLink) { + setLink(result.actionLink); + setOpen(true); + return; + } + setSent(false); + void requestPasswordReset(result.email) + .then(() => setSent(true)) + .catch((err: unknown) => { + console.error("No se pudo enviar el correo de acceso", err); + setError( + "Se creó el acceso, pero no se pudo enviar el correo. Pídele a un Admin que lo reenvíe.", + ); + }); }, onError: (err) => setError(provisionErrorMessage(err, "No se pudo generar el acceso.")), }); @@ -256,6 +275,11 @@ function InviteAccess({ member }: { member: Member }) { {error}

)} + {sent && ( +

+ Invitación enviada por correo. +

+ )}

diff --git a/apps/beacon/src/claims-sync/sync.ts b/apps/beacon/src/claims-sync/sync.ts index 75bfe78f..ee45c793 100644 --- a/apps/beacon/src/claims-sync/sync.ts +++ b/apps/beacon/src/claims-sync/sync.ts @@ -39,7 +39,7 @@ type MemberLike = { * `comisionIds` is the one slot rules cannot grant-check (no array iteration), so * honoring it would let a console-written power comisión — or a power cargo's id * smuggled into comisionIds — mint claims. Ignoring it entirely also means a - * Ignoring it entirely also means a permitted non-Admin positions edit (which restamps + * permitted non-Admin positions edit (which restamps * the shared `assignedBy`) can no longer strip Admin-granted power. That last part was * NOT true of the rules until currentCargoGrantsEmpty() landed: the rules denied * ASSIGNING a power cargo, never OVERWRITING one, so a manage:Member holder could diff --git a/apps/beacon/src/provision-deps.ts b/apps/beacon/src/provision-deps.ts index c030da31..5deef27d 100644 --- a/apps/beacon/src/provision-deps.ts +++ b/apps/beacon/src/provision-deps.ts @@ -19,8 +19,14 @@ export function firestoreProvisionDeps(db: Firestore, auth: Auth): ProvisionDeps getUserByEmail: (email) => auth.getUserByEmail(email).catch(nullIfUserNotFound), getUserByUid: (uid) => auth.getUser(uid).catch(nullIfUserNotFound), // Tolerate a concurrent create (a parallel invite would otherwise throw - // auth/email-already-exists). - createUser: (email) => auth.createUser({ email }).catch(() => auth.getUserByEmail(email)), + // auth/email-already-exists) — and ONLY that. A blanket catch also swallowed quota, + // disabled-provider and invalid-email errors and re-surfaced them as an unrelated + // 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; + return auth.getUserByEmail(email); + }), setClaims: (uid, claims) => auth.setCustomUserClaims(uid, claims), linkUid: async (id, uid) => { await db.doc(`members/${id}`).update({ uid }); diff --git a/apps/beacon/src/provision-member-login.ts b/apps/beacon/src/provision-member-login.ts index fd6cd4ef..2412f342 100644 --- a/apps/beacon/src/provision-member-login.ts +++ b/apps/beacon/src/provision-member-login.ts @@ -17,7 +17,10 @@ interface RawClaims { export function validateProvisionInput(data: unknown): ProvisionInput { const raw = (data ?? {}) as { memberId?: unknown }; - if (typeof raw.memberId !== "string" || raw.memberId.length === 0 || raw.memberId.includes("/")) { + // isSafeDocId, not a hand-rolled subset: `..` and `__x__` build a valid ref and then fail at + // get() with a permanent INVALID_ARGUMENT, which surfaces as `internal` (a 500) instead of + // the invalid-argument this is meant to return. + if (!isSafeDocId(raw.memberId)) { throw new HttpsError("invalid-argument", "memberId is required"); } return { memberId: raw.memberId }; @@ -137,8 +140,10 @@ function readCargoIds(member: Record): string[] { * overwriting would orphan the old Auth account with its claims (possibly Admin) * still live and no member doc backing them. Relinking after an email change is a * deliberate console op. Same-uid re-provision stays allowed (resend invite). - * A failure after createUser leaves an unlinked Auth user — no compensation - * needed: the next run resolves it by email and adopts it (linkedUid null). */ + * A failure after createUser leaves an unlinked Auth user — no compensation needed FOR AN + * ADMIN: the next run resolves it by email and adopts it (linkedUid null). A delegate's retry + * is refused by the adoption guard below (`user !== null`), so a partial failure escalates + * that member to an Admin-only fix. Stated in the spec's operator notes too. */ export async function provisionMember( deps: ProvisionDeps, memberId: string, diff --git a/apps/beacon/src/read-position-grants.test.ts b/apps/beacon/src/read-position-grants.test.ts new file mode 100644 index 00000000..3f0a5e64 --- /dev/null +++ b/apps/beacon/src/read-position-grants.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; +import type { Firestore } from "firebase-admin/firestore"; +import { readPositionGrants } from "./read-position-grants.js"; + +/** A Firestore stub narrow enough to drive `db.doc(path).get()`. The cast is test-only and + * justified: `readPositionGrants` touches exactly that one call chain. */ +function fakeDb(docs: Record | undefined>): Firestore { + return { + doc: (path: string) => ({ + get: async () => { + const data = docs[path]; + return { exists: data !== undefined, data: () => data }; + }, + }), + } as unknown as Firestore; +} + +describe("readPositionGrants", () => { + it("returns the valid roles from a well-formed cargo", async () => { + const db = fakeDb({ "positions/p1": { grants: ["Admin", "Membership"] } }); + await expect(readPositionGrants(db, "p1")).resolves.toEqual(["Admin", "Membership"]); + }); + + it("drops grant entries that are not valid roles", async () => { + const db = fakeDb({ "positions/p1": { grants: ["Admin", "NotARole", 42, null] } }); + await expect(readPositionGrants(db, "p1")).resolves.toEqual(["Admin"]); + }); + + it("returns [] for an absent or empty grants field", async () => { + const db = fakeDb({ "positions/p1": {}, "positions/p2": { grants: [] } }); + await expect(readPositionGrants(db, "p1")).resolves.toEqual([]); + await expect(readPositionGrants(db, "p2")).resolves.toEqual([]); + }); + + it("returns null for a missing doc and for an unsafe id", async () => { + const db = fakeDb({}); + await expect(readPositionGrants(db, "ghost")).resolves.toBeNull(); + await expect(readPositionGrants(db, "a/b")).resolves.toBeNull(); + await expect(readPositionGrants(db, "")).resolves.toBeNull(); + await expect(readPositionGrants(db, "__name__")).resolves.toBeNull(); + await expect(readPositionGrants(db, 42)).resolves.toBeNull(); + }); + + it("BLOCKING: returns null instead of THROWING on a non-array grants field", async () => { + // firestore.rules short-circuits every grants check on hasAnyRole(['Admin']) and never + // type-checks the field, so a console edit or a migration can store one of these. A + // TypeError here would be permanent per member: onMemberWritten is retry:false and the bad + // value persists in positions/, so every later write to any member seated on that cargo + // would re-throw and their claims would never sync again — silently. + const db = fakeDb({ + "positions/str": { grants: "Admin" }, + "positions/map": { grants: { 0: "Admin" } }, + "positions/num": { grants: 7 }, + "positions/bool": { grants: true }, + }); + for (const id of ["str", "map", "num", "bool"]) { + await expect(readPositionGrants(db, id)).resolves.toBeNull(); + } + }); +}); diff --git a/apps/beacon/src/read-position-grants.ts b/apps/beacon/src/read-position-grants.ts index 4af30da1..689d04bc 100644 --- a/apps/beacon/src/read-position-grants.ts +++ b/apps/beacon/src/read-position-grants.ts @@ -9,13 +9,23 @@ import { isSafeDocId } from "./firestore-util.js"; * cargo confers power, one before minting claims and one before creating a login. They * differ only in their wrapper shape, so the read lives here and each adapter wraps it. * - * `null` is the fail-closed answer for BOTH callers: an unreadable cargo is treated as + * `null` is the answer for an unusable id, a missing doc, or a malformed `grants` field, and + * it is fail-closed for BOTH callers: an unreadable cargo is treated as * power-conferring by the guard and as grant-free by the trust gate, which is the safe * direction in each. */ export async function readPositionGrants(db: Firestore, id: unknown): Promise { if (!isSafeDocId(id)) return null; const snap = await db.doc(`positions/${id}`).get(); if (!snap.exists) return null; - const grants = (snap.data()?.grants ?? []) as unknown[]; + // A non-array `grants` returns null rather than throwing. firestore.rules short-circuits + // every grants check on hasAnyRole(['Admin']) and never type-checks the field, so a console + // edit or a migration can store a string or a map — and `.filter` on that is a TypeError. + // onMemberWritten is retry:false and the bad value PERSISTS, so the throw would kill claims + // sync for every member seated on that cargo, permanently and silently. null is fail-closed + // for both callers: the provisioning guard reads it as power-conferring, the trust gate as + // grant-free. + const raw = snap.data()?.grants; + if (raw !== undefined && raw !== null && !Array.isArray(raw)) return null; + const grants = (raw ?? []) as unknown[]; return grants.filter((g): g is Role => isValidRole(g)); } diff --git a/docs/specs/board-seat-delegation.md b/docs/specs/board-seat-delegation.md index 4972e194..452f7a39 100644 --- a/docs/specs/board-seat-delegation.md +++ b/docs/specs/board-seat-delegation.md @@ -128,14 +128,18 @@ This costs the delegation nothing: a genuinely new member has neither an account token, and `auth-store.ts` calls `getIdTokenResult()` without `forceRefresh`. A freshly granted code is invisible for up to an hour otherwise, and a freshly revoked one keeps working for up to an hour. -3. **Revocation is not an undo.** Removing the code stops future seating. Members already seated +3. **A delegate's failed provision escalates to an Admin.** If `provisionMemberLogin` fails + after creating the Auth account (a transient `setClaims`/`linkUid` error), the retry is + refused by the adoption guard — the account now exists, so `user !== null`. Finish it from + an Admin account. +4. **Revocation is not an undo.** Removing the code stops future seating. Members already seated keep their cargo, and their cargo-derived claims are recomputed on the *next write* to their member doc — which may be much later. To force it, re-write the member docs or run `recomputeAllClaims`. -4. **The `PERMISSION_CAP` interaction.** A member whose resolved perms exceed 30 is written +5. **The `PERMISSION_CAP` interaction.** A member whose resolved perms exceed 30 is written `perms: []` fail-closed, which silently takes `update:BoardSeat` with it. Two more subjects in the vocabulary make the 30-slot budget marginally tighter. -5. **Verify who you seat.** One residual no guard can close, and it is not specific to this +6. **Verify who you seat.** One residual no guard can close, and it is not specific to this delegation — it is why enrolment and seating should not both be delegated blindly. A member creator controls the `email` on the doc they file, and the invite goes to that address. So if an Admin later seats a FABRICATED member on an Admin-granting cargo, the claim is minted onto @@ -143,7 +147,7 @@ This costs the delegation nothing: a genuinely new member has neither an account verify; it existed before this feature (an Admin provisioning the same fabricated doc sends the invite to the same attacker address), and the power-seat guard means a delegate cannot complete it alone. Treat the members list as the thing you verify before seating. -6. **There is a consistency window.** `firestore.rules` reads the token while beacon reads stored +7. **There is a consistency window.** `firestore.rules` reads the token while beacon reads stored claims. If a delegate's own perms are dropped (cap breach, or revocation) their cached token still passes the rules for up to an hour, so a seat write can succeed while `resolveTrustedGrants` declines to mint the grants. The member is then published on the public From 1e6f4d72946edefa964eb6091b0064f69f30997c Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 20:12:29 -0400 Subject: [PATCH 13/15] chore: reviews Reviews: 2c328f551f4f58f4d0d4949810965d88912171a1 security-review,firestore-security-reviewer,firebase-functions-reviewer,code-review,simplify,react-best-practices,bundle-budget-watcher From 58266d33e0cd76d75044ddf38c875876666e79af Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 20:45:07 -0400 Subject: [PATCH 14/15] fix(beacon,rules,backstage): a delegate may confer power on others, never on themselves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /code-review (the real skill, run by the user) found a HIGH the subagent passes missed, plus four stale-claim/dead-affordance issues. HIGH — update:BoardSeat was a self-service grant of every built-in role but Admin. The spec's own recommended pairing (update:Position + update:BoardSeat) could write a Secretario/ProjectManager cargo onto its OWN member doc through the positions-only lane — one write, no puppet — and claims-sync minted those roles onto the author. resolveTrustedGrants now requires the Admin ROLE for a self-assignment of any granting cargo, alongside the existing Admin-grant rule. The seeded president is unaffected (both halves defer to the Admin role, which he holds), and seating someone ELSE still works, which is the feature. Also: - firestore.rules' positions-only lane still claimed "CEL is NOT in that grant" and "the power-cargo restriction is NOT relaxed" — false since this branch lifted the new-side conjunct. boardSeatDelegate()'s comment still described the abandoned non-reflexive design, which would have led a reader to re-introduce the check that broke the president. - The invite drawer offered "Enviar acceso" on a cargo whose provisioning beacon always refuses: it created the member, 403'd, and pointed at a row action that failed identically forever. It now decides before writing anything and says so. - "Reenviar invitación/acceso" was shown to delegates, whom the adoption guard rejects on every click. Gated on isAdmin || !member.uid. - hasDirectGrants failed OPEN on an array permissionOverrides (typeof [] is "object"), contradicting its own fail-closed docblock. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/member-invite-drawer.test.tsx | 40 +++++++++++ .../components/member-invite-drawer.tsx | 20 +++++- .../components/member-profile-page.tsx | 4 +- .../members/components/member-row-menu.tsx | 8 ++- apps/beacon/src/claims-sync/sync.test.ts | 66 +++++++++++++++++++ apps/beacon/src/claims-sync/sync.ts | 51 +++++++++----- .../beacon/src/provision-member-login.test.ts | 1 + apps/beacon/src/provision-member-login.ts | 5 +- docs/specs/board-seat-delegation.md | 20 +++++- firestore.rules | 38 ++++++----- 10 files changed, 213 insertions(+), 40 deletions(-) diff --git a/apps/backstage/src/features/members/components/member-invite-drawer.test.tsx b/apps/backstage/src/features/members/components/member-invite-drawer.test.tsx index c79c2aba..ee0ceda1 100644 --- a/apps/backstage/src/features/members/components/member-invite-drawer.test.tsx +++ b/apps/backstage/src/features/members/components/member-invite-drawer.test.tsx @@ -1,4 +1,5 @@ import { render, screen, fireEvent, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; import { describe, it, expect, vi, beforeEach } from "vitest"; import type { ReactElement, ReactNode } from "react"; import { MemberInviteDrawer } from "./member-invite-drawer"; @@ -191,4 +192,43 @@ describe("MemberInviteDrawer", () => { renderWithAbility(node, { roles: ["Member"], perms: ["manage:all"] }); expect(screen.queryByLabelText("Enviar acceso a la app")).not.toBeInTheDocument(); }); + + it("does not attempt the invite when the cargo confers permissions and the caller is a delegate", async () => { + // beacon's power-seat guard would refuse it, so attempting it would create the member, + // 403, and point the user at a row action that fails identically forever. + const onProvision = vi.fn(); + const powerCargo = [ + { + id: "pos-power", + title: "Secretario", + titleFemale: null, + category: "CEL" as const, + grants: ["Secretary"] as never, + term: null, + sigla: null, + description: "", + active: true, + deletedAt: null, + }, + ]; + renderWithAbility( + {}} + onCreate={async () => "idB"} + onProvision={onProvision} + />, + { roles: ["Member"], perms: ["create:Member", "create:MemberLogin", "update:BoardSeat"] }, + ); + await fill(); + await userEvent.click(screen.getByLabelText("Cargo")); + // positionTitle derives the female variant from the title when titleFemale is null, and + // fill() picks "Femenino" — so the rendered label is "Secretaria". + await userEvent.click(await screen.findByText(/Secretari[ao]/)); + fireEvent.click(screen.getByRole("button", { name: "Enviar invitación" })); + await waitFor(() => expect(screen.getByRole("alert")).toBeInTheDocument()); + expect(screen.getByRole("alert")).toHaveTextContent(/solo un Admin puede enviarle el acceso/); + expect(onProvision).not.toHaveBeenCalled(); + }); }); diff --git a/apps/backstage/src/features/members/components/member-invite-drawer.tsx b/apps/backstage/src/features/members/components/member-invite-drawer.tsx index 9641b177..28a79537 100644 --- a/apps/backstage/src/features/members/components/member-invite-drawer.tsx +++ b/apps/backstage/src/features/members/components/member-invite-drawer.tsx @@ -15,6 +15,9 @@ interface MemberInviteDrawerProps { } interface DoneState { + /** The invite was skipped because the member's cargo confers permissions and the caller is + * not an Admin — beacon would refuse it, so nothing was attempted. */ + blockedByCargo: boolean; name: string; email: string; provisioned: boolean; @@ -38,7 +41,7 @@ export function MemberInviteDrawer({ // (provisionMemberLogin → requireAdminOrPerm). A member creator without either may still // create the member; they just can't send access here, so hide the option and default it // off — otherwise the provision step fails silently after the member is already created. - const { canProvisionLogin, canAssignBoardSeat } = useCan(); + const { canProvisionLogin, canAssignBoardSeat, isAdmin } = useCan(); const [done, setDone] = useState(null); const [sendAccess, setSendAccess] = useState(canProvisionLogin); const [copyState, setCopyState] = useState<"idle" | "copied" | "failed">("idle"); @@ -70,7 +73,15 @@ export function MemberInviteDrawer({ let emailSent = false; let actionLink: string | null = null; let errorDetail: string | null = null; - if (sendAccess) { + // beacon refuses a non-Admin provisioning a member seated on a granting cargo (the + // power-seat guard). The rules DO let that member be created, so without this check the + // drawer would create them, 403 on the invite, and send the user to a row action that + // fails the same way on every retry. Decide before writing anything. + const seatedCargo = data.cargoId ? positions.find((p) => p.id === data.cargoId) : undefined; + const provisionBlocked = !isAdmin && (seatedCargo?.grants.length ?? 0) > 0; + if (sendAccess && provisionBlocked) { + errorDetail = null; + } else if (sendAccess) { // The member is already created; if provisioning fails, fall through to the // done screen with provisioned=false ("aún no tiene acceso, invítalo desde su // fila") instead of throwing — a thrown error reads as a create failure and @@ -93,6 +104,7 @@ export function MemberInviteDrawer({ } } setDone({ + blockedByCargo: sendAccess && provisionBlocked, name: data.name, email: data.email, provisioned, @@ -120,6 +132,10 @@ export function MemberInviteDrawer({

{`Invitación enviada a ${done.email}. Recibirá un correo para crear su contraseña y acceder a la app.`}

+ ) : done.blockedByCargo ? ( +

+ {`${done.name} fue creado, pero su cargo otorga permisos: solo un Admin puede enviarle el acceso. Pídeselo para completar la invitación.`} +

) : done.provisioned && !done.emailSent ? ( <>

diff --git a/apps/backstage/src/features/members/components/member-profile-page.tsx b/apps/backstage/src/features/members/components/member-profile-page.tsx index f4d39087..fd4da41e 100644 --- a/apps/backstage/src/features/members/components/member-profile-page.tsx +++ b/apps/backstage/src/features/members/components/member-profile-page.tsx @@ -131,7 +131,9 @@ export function MemberProfilePage() { {member.status && {member.status}} {/* provisionMemberLogin is requireAdminOrPerm(create:MemberLogin) — the Admin role or that exact code, never the manage:all perm. */} - + {/* `!member.uid` mirrors beacon's adoption guard: a delegate may only mint a NEW + login, so "Reenviar acceso" would 403 on every click for them. */} +

diff --git a/apps/backstage/src/features/members/components/member-row-menu.tsx b/apps/backstage/src/features/members/components/member-row-menu.tsx index bc2811b2..03d9a6f9 100644 --- a/apps/backstage/src/features/members/components/member-row-menu.tsx +++ b/apps/backstage/src/features/members/components/member-row-menu.tsx @@ -21,7 +21,7 @@ export function MemberRowMenu({ onSetStatus, onUnpublish, }: MemberRowMenuProps) { - const { canProvisionLogin } = useCan(); + const { canProvisionLogin, isAdmin } = useCan(); return ( {/* provisionMemberLogin is requireAdminOrPerm(create:MemberLogin) — the Admin role or - that exact code, never the manage:all perm. */} - + that exact code, never the manage:all perm. The `!member.uid` half is the adoption + guard mirrored: a delegate may only mint a NEW login, so offering "Reenviar" to one + would be an item that 403s on every click. */} + onProvision(member)}> {member.uid ? "Reenviar invitación" : "Invitar a la app"} diff --git a/apps/beacon/src/claims-sync/sync.test.ts b/apps/beacon/src/claims-sync/sync.test.ts index fece24f2..b5bef8f0 100644 --- a/apps/beacon/src/claims-sync/sync.test.ts +++ b/apps/beacon/src/claims-sync/sync.test.ts @@ -177,6 +177,72 @@ describe("syncMemberClaims", () => { expect(writes["target-uid"]).toBeUndefined(); }); + it("BLOCKING: a delegate may NOT confer power on THEMSELVES", async () => { + // One write, no puppet: the spec's own recommended pairing (update:Position + + // update:BoardSeat) can seat itself on a Secretario/ProjectManager cargo through the + // positions-only lane, and without this that mints those roles onto the author — + // update:BoardSeat becomes a self-service grant of every built-in role but Admin. + // Conferring power on others is the delegation; on yourself it is self-promotion. + const { deps, writes } = fakeDeps({ + positions: { "pos-sec": { grants: ["Secretary"] } }, + userRoles: { "delegate-uid": ["Member"] }, + userPerms: { "delegate-uid": ["update:BoardSeat"] }, + existing: { "delegate-uid": { roles: ["Member"], perms: permsFor(["Member"]) } }, + }); + await syncMemberClaims( + deps, + { + uid: "delegate-uid", + positions: { "2026": { cargoId: "pos-sec", comisionIds: [], assignedBy: "delegate-uid" } }, + }, + "2026", + ); + expect(writes["delegate-uid"]).toBeUndefined(); + }); + + it("still lets a delegate confer a non-Admin cargo on SOMEONE ELSE", async () => { + // The paired ALLOW — the restriction is on self-dealing, not on the delegation itself. + const { deps, writes } = fakeDeps({ + positions: { "pos-sec": { grants: ["Secretary"] } }, + userRoles: { "delegate-uid": ["Member"] }, + userPerms: { "delegate-uid": ["update:BoardSeat"] }, + existing: { "target-uid": { roles: ["Member"] } }, + }); + await syncMemberClaims( + deps, + { + uid: "target-uid", + positions: { "2026": { cargoId: "pos-sec", comisionIds: [], assignedBy: "delegate-uid" } }, + }, + "2026", + ); + expect(writes["target-uid"]).toEqual({ + roles: ["Secretary", "Member"], + perms: permsFor(["Secretary", "Member"]), + }); + }); + + it("still lets an ADMIN seat themselves on a non-Admin cargo", async () => { + // The self-assignment half defers to the Admin ROLE, so it costs an Admin nothing. + const { deps, writes } = fakeDeps({ + positions: { "pos-sec": { grants: ["Secretary"] } }, + userRoles: { "admin-uid": ["Admin", "Member"] }, + existing: { "admin-uid": { roles: ["Admin", "Member"] } }, + }); + await syncMemberClaims( + deps, + { + uid: "admin-uid", + positions: { "2026": { cargoId: "pos-sec", comisionIds: [], assignedBy: "admin-uid" } }, + }, + "2026", + ); + expect(writes["admin-uid"]).toEqual({ + roles: ["Secretary", "Member"], + perms: permsFor(["Secretary", "Member"]), + }); + }); + it("BLOCKING: a delegate may NOT confer ADMIN — the guard the delegation rests on", async () => { // Conferring Admin is reserved to the Admin ROLE. Without this a delegate mints an Admin, // that Admin is itself a trust source, and the delegation can never be revoked — via the diff --git a/apps/beacon/src/claims-sync/sync.ts b/apps/beacon/src/claims-sync/sync.ts index ee45c793..a0b861a7 100644 --- a/apps/beacon/src/claims-sync/sync.ts +++ b/apps/beacon/src/claims-sync/sync.ts @@ -59,15 +59,27 @@ type MemberLike = { * seat lands, the member is published on the world-readable Directiva, and no claim is * minted. Visible, powerless, and silent: half-working rather than safe. * - * CONFERRING ADMIN IS ADMIN-ONLY, and this is the guard the whole delegation rests on. A - * cargo whose grants include `Admin` is honored only for an assigner holding the Admin ROLE; - * every other cargo is honored for an `update:BoardSeat` delegate too. + * TWO restrictions, and together they are the guard the whole delegation rests on: + * - a cargo whose grants include `Admin` is honored only for an assigner holding the Admin + * ROLE; and + * - a SELF-assignment is honored only for an Admin, whatever the cargo grants. + * Everything else — a delegate seating SOMEONE ELSE on a non-Admin power cargo — is honored + * for an `update:BoardSeat` holder, and that is the feature. * - * Why the split rather than a self-assignment check (which is what this first shipped as): - * the danger is not reflexivity, it is that a delegate can mint an Admin AT ALL, because a - * minted Admin is itself a trust source and the delegation then cannot be revoked. Blocking - * only `assignedBy === memberUid` stops the one-write self-loop and not the two-write puppet - * loop — a delegate creates a second member on a mailbox they control, seats IT on + * The self-assignment half is not the discredited reflexivity check (see below); it is + * narrower and it closes a different hole. Without it `update:BoardSeat` is a self-service + * grant of every built-in role but Admin: the spec's own recommended pairing + * (`update:Position` + `update:BoardSeat`) can write `positions. = { cargoId: + * , assignedBy: }` onto its OWN member doc + * through the positions-only lane — one write, no puppet needed — and claims-sync would mint + * those roles onto the author. Conferring power on others is the delegation; conferring it on + * yourself is self-promotion. + * + * Why the ADMIN half keys on the grant rather than on reflexivity (which is what this first + * shipped as): for Admin the danger is not reflexivity, it is that a delegate can mint an + * Admin AT ALL, because a minted Admin is itself a trust source and the delegation then + * cannot be revoked. Blocking only `assignedBy === memberUid` stops the one-write self-loop + * and not the two-write puppet loop — a delegate creates a second member on a mailbox they control, seats IT on * Presidente (not a self-assignment, so the perm is trusted), and that puppet is Admin * forever; revoking the delegate's code de-elevates nobody. It also had a worse problem: the * seeded bootstrap president self-stamps `assignedBy` (tools/scripts/lib/seed-president.mjs) @@ -76,7 +88,8 @@ type MemberLike = { * * Keying on the GRANT fixes both: no cargo-derived Admin can ever originate from a delegate, * so there is no loop to close and no anchor to special-case, and an Admin seating anyone — - * including themselves — is untouched. Revocation is then real for everything a delegate + * including themselves — is untouched. The self-assignment half rides alongside it and is + * safe for the same reason: it too defers to the Admin ROLE, which the seeded president has. Revocation is then real for everything a delegate * CAN confer: strip the perm and the next write to that member drops the grants. * * The cost, stated so nobody reads it as a bug: a delegate seating a member on an @@ -85,6 +98,7 @@ async function resolveTrustedGrants( deps: ClaimsSyncDeps, cargoId: string | null, assignedBy: string | undefined, + memberUid: string, ): Promise { // FULL screening, not just the empty-string half this used to check. `cargoId` comes // straight off the member doc and every implementation of `getPosition` interpolates it @@ -102,11 +116,13 @@ async function resolveTrustedGrants( if (!assignedBy) return []; const assigner = await deps.getAssignerClaims(assignedBy); const assignerIsAdmin = assigner.roles.includes("Admin"); - // Conferring ADMIN is reserved to an Admin, whoever assigned the cargo. Everything else a - // board-seat delegate may confer. - const trusted = position.grants.includes("Admin") - ? assignerIsAdmin - : assignerIsAdmin || assigner.perms.includes("update:BoardSeat"); + // A delegate may confer power on OTHERS, never on themselves, and never Admin at all. + // Both halves need the Admin role; only the third case honors the perm. + const selfAssigned = assignedBy === memberUid; + const trusted = + position.grants.includes("Admin") || selfAssigned + ? assignerIsAdmin + : assignerIsAdmin || assigner.perms.includes("update:BoardSeat"); return trusted ? [...new Set(position.grants)] : []; } @@ -135,7 +151,12 @@ export async function syncMemberClaims( ): Promise { if (!member.uid) return; const term = member.positions?.[termKey]; - const trustedGrants = await resolveTrustedGrants(deps, term?.cargoId ?? null, term?.assignedBy); + const trustedGrants = await resolveTrustedGrants( + deps, + term?.cargoId ?? null, + term?.assignedBy, + member.uid, + ); const existing = await deps.getExistingClaims(member.uid); const hadScanner = existing.roles.includes("Scanner"); diff --git a/apps/beacon/src/provision-member-login.test.ts b/apps/beacon/src/provision-member-login.test.ts index 8e273687..14e9f3c8 100644 --- a/apps/beacon/src/provision-member-login.test.ts +++ b/apps/beacon/src/provision-member-login.test.ts @@ -263,6 +263,7 @@ describe("provisionMember", () => { { roleIds: {} }, { permissionOverrides: "nope" }, { permissionOverrides: { grant: "update:BoardSeat" } }, + { permissionOverrides: ["manage:all"] }, ]; for (const fields of malformed) { const { deps } = fakeDeps({ member: { ...active, ...fields } }); diff --git a/apps/beacon/src/provision-member-login.ts b/apps/beacon/src/provision-member-login.ts index 2412f342..01c07b86 100644 --- a/apps/beacon/src/provision-member-login.ts +++ b/apps/beacon/src/provision-member-login.ts @@ -88,7 +88,10 @@ function hasDirectGrants(member: Record): boolean { } const overrides = member.permissionOverrides; if (overrides === undefined || overrides === null) return false; - if (typeof overrides !== "object") return true; + // Array before the typeof: `typeof [] === "object"`, so a legacy/console + // `permissionOverrides: ["manage:all"]` would reach `.grant === undefined` and read as + // ungranted — failing OPEN, which is what the roleIds branch above refuses to do. + if (Array.isArray(overrides) || typeof overrides !== "object") return true; const grant = (overrides as { grant?: unknown }).grant; if (grant === undefined || grant === null) return false; if (!Array.isArray(grant)) return true; diff --git a/docs/specs/board-seat-delegation.md b/docs/specs/board-seat-delegation.md index 452f7a39..9597dace 100644 --- a/docs/specs/board-seat-delegation.md +++ b/docs/specs/board-seat-delegation.md @@ -22,8 +22,9 @@ does not satisfy either one. holds `update:Member`, `create:Member`, or `update:Position`. A member holding only `update:BoardSeat` still cannot write anything. Granting it alone is a no-op; the UI does not say so. -- **It does not confer Admin.** See the section below — an Admin-granting cargo mints nothing - unless the assigner holds the Admin role. +- **It does not confer Admin, and it does not confer anything on the delegate themselves.** + See the section below: an Admin-granting cargo mints nothing unless the assigner holds the + Admin role, and a self-assignment mints nothing unless the assigner is an Admin. - **It does not unseat anyone.** `currentCargoGrantsEmpty()` stays Admin-only, so a delegate cannot displace a member sitting on a power-granting cargo. Hand-over is an Admin action. One caveat, pre-existing and pinned by a rules test: that predicate reads only the CURRENT @@ -33,6 +34,10 @@ does not satisfy either one. (`compute-roles.ts`), so clearing every Admin's cargo would strip every Admin claim in the chapter — and `setUserRoles`, `roles/*` writes and `permissionOverrides` writes are all Admin-only, making that state unrecoverable outside the Firebase console. +- **It DOES raise the publication ceiling to CEL.** A plain `update:Position` holder can seat + only grant-free non-CEL cargos; adding this code lets them seat a CEL cargo, so "Presidente" + at public board rank 0 is no longer an Admin-only decision for a delegate. Publication and + authority are separate ceilings — the claims half still refuses to mint Admin. - **It does not reach the `/positions` catalog.** Creating a CEL or JDL cargo, and editing any cargo's `grants`, `category` or board `title`/`titleFemale`, stay Admin-only (`boardSurfacingCategory()` and the update-arm pins). The delegate seats members on cargos @@ -48,7 +53,16 @@ premise that the delegation is **revocable**. Delivering that premise turned out rule, and it is the load-bearing guard of the whole feature: **A cargo whose `grants` include `Admin` is honored only when the assigner holds the Admin -ROLE.** Every other cargo is honored for an `update:BoardSeat` delegate too. +ROLE**, and **a SELF-assignment is honored only for an Admin, whatever the cargo grants.** +Everything else — a delegate seating *someone else* on a non-Admin power cargo — is honored for +an `update:BoardSeat` holder, and that is the feature. + +The self-assignment half closes a separate, one-write hole: without it `update:BoardSeat` is a +self-service grant of every built-in role but Admin. The recommended pairing below +(`update:Position` + `update:BoardSeat`) can write a Secretario or ProjectManager cargo onto its +own member doc through the positions-only lane and be minted those roles. Conferring power on +others is the delegation; conferring it on yourself is self-promotion. A delegate seating +themselves still *publishes* the seat — it just confers nothing. Why, precisely: a minted Admin is itself a trust source, so a delegate who can mint one has made the delegation permanent. Revoking their code de-elevates nobody, and neither diff --git a/firestore.rules b/firestore.rules index 9c54e93f..c6f13eae 100644 --- a/firestore.rules +++ b/firestore.rules @@ -369,11 +369,13 @@ service cloud.firestore { // hasPerm, deliberately NOT canDo: manage:all must not answer this, and the other five // *:BoardSeat codes the cross-product generates stay inert BECAUSE the gate is exact. // - // ACCEPTED, per docs/specs/board-seat-delegation.md: a delegate may seat a cargo whose - // grants confer Admin, and beacon's claims-sync mints it. What that acceptance is - // premised on — revocability — needs beacon's trust gate to be non-reflexive on a - // SELF-assignment, or the minted Admin satisfies the gate that minted it and the - // delegation can never be revoked. See resolveTrustedGrants in apps/beacon. + // What this rule does and does NOT buy, per docs/specs/board-seat-delegation.md: it lifts + // the cargo conjunct, so a delegate may SEAT anyone on a CEL or power-granting cargo and + // the seat publishes on the world-readable Directiva. It does not follow that the cargo's + // grants are MINTED — beacon's resolveTrustedGrants refuses an Admin-conferring cargo + // from a delegate outright, and refuses any granting cargo on a SELF-assignment. So a + // delegate seating themselves publishes and confers nothing, and seating someone else + // confers everything but Admin. Do not restate the mint rule here; read it there. // // Deliberately NOT widened to currentCargoGrantsEmpty(): see positionsAssignmentSafe(). function boardSeatDelegate() { @@ -524,17 +526,23 @@ service cloud.firestore { && softDeleteSafe(); // Positions-only lane: an org-chart editor who is NOT a member editor. Keyed on // update:Position — the same capability that governs the positions CATALOG — and - // confined to the positions map. The power-cargo restriction is NOT relaxed: - // positionsAssignmentSafe()'s non-Admin branch still demands - // cargoAssignableByNonAdmin() && currentCargoGrantsEmpty(), so this principal assigns - // and clears grant-free cargos only, on BOTH sides of a swap. + // confined to the positions map. A PLAIN update:Position holder is still held to + // cargoAssignableByNonAdmin() && currentCargoGrantsEmpty(), so they assign and clear + // grant-free non-CEL cargos only, on BOTH sides of a swap. Adding update:BoardSeat lifts + // the NEW side only: that principal may seat a CEL or power-granting cargo, but still + // cannot displace a sitting power-cargo holder (the old side stays Admin-role-only). // - // This lane IS a public publication authority, deliberately, and its ceiling is JDL: - // grant-free JDL direcciones are board cargos, so its holder can put a member — - // including themselves — on the world-readable Directiva under a dirección. No claim - // is minted (claims-sync returns early on grants.length == 0). Accepted, and pinned by - // a rules test. CEL is NOT in that grant — cargoAssignableByNonAdmin() refuses it - // whatever its grants, so 'Presidente' at public rank 0 stays an Admin decision. + // This lane IS a public publication authority, deliberately. For a PLAIN update:Position + // holder its ceiling is JDL: grant-free JDL direcciones are board cargos, so they can put + // a member — including themselves — on the world-readable Directiva under a dirección, + // minting nothing (claims-sync returns early on grants.length == 0). Accepted, pinned by + // a rules test, and CEL stays out of reach — cargoAssignableByNonAdmin() refuses it + // whatever its grants. + // Adding update:BoardSeat RAISES that ceiling to CEL, so 'Presidente' at public rank 0 is + // no longer an Admin-only decision for a delegate. The claims half is what still holds: + // an Admin-conferring cargo mints nothing from a delegate, and neither does a + // self-assignment. Publication and authority are separate ceilings here — do not read + // one as the other. // // memberWriteInvariants() is implied by hasOnly(['positions']) TODAY and is stated // anyway: if hasOnly is ever widened — the obvious future edit is adding a second From b968b46491c0616bdd452324dc11021c15852c27 Mon Sep 17 00:00:00 2001 From: Arnold Gandarillas Castillo Date: Thu, 27 Aug 2026 20:45:07 -0400 Subject: [PATCH 15/15] chore: reviews Reviews: 58266d33e0cd76d75044ddf38c875876666e79af security-review,firestore-security-reviewer,firebase-functions-reviewer,code-review,simplify,react-best-practices,bundle-budget-watcher