Skip to content

feat(authz): Settings → Roles management UI (production wire-up)#383

Open
mcharles-square wants to merge 14 commits into
mainfrom
feat/rbac-roles-real-wire
Open

feat(authz): Settings → Roles management UI (production wire-up)#383
mcharles-square wants to merge 14 commits into
mainfrom
feat/rbac-roles-real-wire

Conversation

@mcharles-square
Copy link
Copy Markdown
Collaborator

Supersedes #358 — replaces every placeholder with a real RPC. Ready to merge to main.

Summary

U11 from docs/plans/2026-05-19-001-feat-granular-rbac-plan.md. New Settings → Roles page where admins (anyone holding role:manage) can manage built-in and custom roles, plus a role Select on AddTeamMemberModal so new members can be assigned a role at creation time.

What it does

Roles page (/settings/roles)

  • Lists built-in (Owner / Admin / Field Tech) and custom roles with member counts, permission counts, and last-updated timestamps — fetched from AuthzService.ListRoles.
  • Edit action hidden on all built-in roles (server unconditionally rejects built-in mutation); custom roles only.
  • Delete action on custom roles only, gated on memberCount === 0 so members can't be orphaned mid-deletion.
  • Nav entry + route gated on role:manage. Direct visits redirect to /settings/general for callers without the key.

Create/Edit role modal

  • Grouped checkbox builder over the live permission catalog (AuthzService.ListPermissions).
  • Description input (validated against the proto's max_len = 1024).
  • Search, collapse-by-group, "X selected" running total.
  • Automatic inclusion of read-pairing dependencies (mirrors the server's role-save validator: every action key requires its same-resource :read partner, and any miner action requires fleet:read).
  • Read keys still required by other selected actions render disabled with a (required) suffix via lockedReadKeys, so the UI never holds a selection the server would reject on save.
  • Loading + error fallback while the catalog is fetching.

AddTeamMemberModal

  • Adds a role Select populated from AuthzService.ListRoles.
  • Owner is excluded (ownership transfer is a deliberately separate flow, not a member default).
  • Defaults to Field Tech when present, falls back to the first assignable role.
  • roleId is forwarded end-to-end to auth.v1.CreateUser.

Server work in this PR

  • proto/auth/v1/auth.protoCreateUserRequest gains optional string role_id = 2.
  • server/internal/domain/auth/service.go — resolves the role for the new user: empty role_id keeps the legacy ADMIN-default; non-empty validates org membership, rejects SUPER_ADMIN (ownership transfer is a separate flow), and runs the same parity check used by authorizeCallerForNewUserWithRole before persisting the assignment through the existing CreateUserOrganizationRole write.
  • UserManagementStore gains GetRoleByID; concrete + mock + sqlc-generated implementations all in sync.
  • Two new test suites (TestParseInt64RoleID, TestResolveCreateUserRole_ValidationBranches) cover the resolver's validation branches.

What changed vs #358

The placeholders that #358 shipped as drafts are now real implementations:

#358 placeholder Now
useRoleManagement in-memory dataset Real authzClient.listRoles / createCustomRole / updateCustomRole / deleteCustomRole calls with a pbToRoleItem adapter (BuiltinKey enum → string union, Timestamp → Date, permission_keys → permissions).
Hand-maintained PERMISSION_CATALOG constant usePermissionCatalog hook that fetches authzClient.listPermissions on first mount, caches at module scope so every consumer shares one fetch. Pure helpers (requiredReadsFor, withRequiredReads, lockedReadKeys) are bound to the fetched catalog.
wireRoleId regex guard stripping non-numeric placeholder ids before forwarding Removed — real role ids are numeric int64 strings the server parses directly.
description workaround (optional + preserve-on-update) Real Description input in the role editor; description is now a required parameter.
TODO(rbac): markers across useRoleManagement, permissionCatalog, useUserManagement, CreateEditRoleModal Zero remaining.

Files

New:

  • client/src/protoFleet/api/useRoleManagement.ts — real AuthzService wire-up with pbToRoleItem adapter.
  • client/src/protoFleet/features/settings/components/Roles.tsx
  • client/src/protoFleet/features/settings/components/CreateEditRoleModal.tsx
  • client/src/protoFleet/features/settings/components/DeleteRoleDialog.tsx
  • client/src/protoFleet/features/settings/utils/permissionCatalog.tsusePermissionCatalog hook + pure helpers.

Modified:

  • proto/auth/v1/auth.protoCreateUserRequest.role_id.
  • server/internal/domain/auth/service.go (+ tests) — role resolution and validation.
  • server/internal/domain/stores/interfaces/user.go + sqlstores/user.go + mocks — GetRoleByID.
  • client/src/protoFleet/api/useUserManagement.ts — forwards roleId.
  • client/src/protoFleet/features/settings/components/AddTeamMemberModal.tsx — role Select.
  • client/src/protoFleet/features/settings/utils/formatRole.ts + test — FIELD_TECH → "Field Tech".
  • client/src/protoFleet/config/navItems.ts/settings/roles entry gated on role:manage.
  • client/src/protoFleet/routePrefetch.ts + router.tsx — lazy import + route.

Removed:

  • Roles.stories.tsx, CreateEditRoleModal.stories.tsx, DeleteRoleDialog.stories.tsx — authored against the deleted placeholder dataset; the repo has no MSW/Storybook RPC-mock infrastructure to rebuild them on. They were authoring tools, not production code.

Deferred from U11 plan

These U11 surfaces remain follow-up work — they need server RPCs that are still Unimplemented (AssignRole / UnassignRole / ListUserAssignments):

  • Per-user assignment editing on Team.tsx (gain a "Roles" cell with an add/remove modal).
  • Assignment scope picker (org / site).
  • Settings primary nav hidden when no sub-items match (FIELD_TECH).
  • Delete-role with "remove all assignments and delete" affordance (this PR disables Delete when memberCount > 0).

Test plan

  • tsc --noEmit clean.
  • eslint clean.
  • vitest run src/protoFleet/features/settings src/protoFleet/api — 331 passed.
  • go build ./... clean; go test ./internal/domain/auth/... and ./internal/handlers/auth/... pass.
  • Manual: log in as ADMIN, walk Create/Edit/Delete on a custom role; verify the read-pairing lock behavior; add a team member with a role and confirm the assignment lands server-side.

mcharles-square and others added 6 commits June 3, 2026 18:30
U11 from docs/plans/2026-05-19-001-feat-granular-rbac-plan.md. New
Settings → Roles page where admins (anyone holding role:manage) can:

  - List built-in and custom roles, with member counts and permission
    counts.
  - Edit any role except Owner (SUPER_ADMIN is immutable server-side).
  - Create custom roles via a grouped checkbox builder over the full
    permission catalog: search, collapse-by-resource, automatic
    inclusion of read-pairing dependencies, "Required" lock icons on
    reads still held by other selected actions.
  - Delete custom roles, gated on memberCount = 0 so members never end
    up orphaned mid-deletion.

Also wires role selection into Settings → Team:

  - AddTeamMemberModal grows a role Select populated from
    useRoleManagement. Owner is excluded (ownership transfer is a
    separate, deliberate flow). FIELD_TECH is the default when present.
  - useUserManagement.createUser accepts a roleId param (currently
    voided with a TODO; CreateUserRequest needs a role_id field — see
    inline comment).

Adds /settings/roles route + nav entry (gated on role:manage),
prefetch wiring, and stories for the three new components.

Notes on placeholders (each marked TODO inline):

  - api/useRoleManagement.ts serves an in-memory dataset derived from
    the catalog. The proto package authz.v1 currently exposes only the
    Permission / PermissionGroup messages; the ListRoles / CreateRole /
    UpdateRole / DeleteRole RPCs and AuthzService client land in a
    follow-up. The hook's signature mirrors useUserManagement so the
    settings components consume it identically.
  - features/settings/utils/permissionCatalog.ts is a hand-maintained
    mirror of server/internal/domain/authz/catalog.go, replaced by a
    AuthzService.GetPermissionCatalog fetch once that RPC ships.
  - useUserManagement.createUser accepts roleId but does not forward it
    yet — CreateUserRequest has no role_id field; add it then wire the
    forward in the authClient.createUser call.

formatRole gains a FIELD_TECH → "Field Tech" mapping so the role badge
on the Team table renders consistently with the role label in the
Roles list.
- CreateEditRoleModal: drop the prevKey/openKey render-phase reset
  pattern. Callers now remount the modal via key={role?.roleId ??
  "create"}, so the form state is seeded from useState defaults
  exactly once per open. Removes three inner key props on the input
  fields that existed only to remount them under the prior scheme.
- Roles.tsx: consolidate the two CreateEditRoleModal mounts (create vs
  edit) into one, gated on `open={showCreateModal || !!editRole}` with
  `role={editRole}`. Replaces the two handleX callbacks with a single
  handleEditor pair.
- useRoleManagement: extract a RoleCallbacks base for the shared
  onError/onFinally pair across the four *Props types. Drops ~8 lines
  of duplication.
- Type RoleItem.builtinKey as the union "SUPER_ADMIN" | "ADMIN" |
  "FIELD_TECH" (new BuiltinRoleKey export) instead of plain string —
  catches consumer typos at compile time.
- CreateEditRoleModal.toggleKey: when unchecking a locked read whose
  withRequiredReads expansion produces an identical set, return the
  previous reference so React doesn't schedule a no-op re-render of
  the permission tree.
…der UI

- Consolidate 12 permission groups into 6: hide fleet:read (auto-dependency),
  merge sites + racks into "Sites, buildings & racks", merge admin singletons
  (server logs, API keys, team, roles, fleet nodes) into "Administration"
- Simplify permission rows to description-only (remove action label + key)
- Remove lock badge UX; unchecking a read key now cascades removal
- Use standard modal width, design tokens, and textOnly Button for CTAs
- Drop role description field and built-in role callout banner
- Generalize edit title to "Edit role" with disabled name input for built-ins

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bug fixes visible today:
- CreateEditRoleModal no longer wipes role descriptions on save —
  description is optional in CreateRoleProps/UpdateRoleProps and
  updateRole preserves the existing value when omitted.
- Read-key checkboxes that still have action dependents render
  disabled with a "(required)" suffix via lockedReadKeys, replacing
  the misleading toggleKey cascade comment that promised behavior
  the code didn't implement.
- Roles page renders CreateEditRoleModal conditionally with a
  bump counter so two consecutive opens always get a fresh instance,
  fixing stale name/selection/query leak.
- Edit action is now hidden for all built-in roles (was only
  SUPER_ADMIN); server unconditionally rejects built-in mutation,
  so the UI must not expose it.

Catalog and copy alignment with server:
- Added activity:read to client permissionCatalog with the same
  description and resource as server catalog.go.
- role:manage description now matches the server: "Create, edit,
  and delete custom roles. Built-in roles cannot be modified."
- fleet is intentionally excluded from RESOURCE_TO_GROUP so
  fleet:read remains dependency-floor only — documented inline.

Land role_id end-to-end (was the unresolved P1 Codex finding):
- Added optional string role_id = 2 to auth.v1.CreateUserRequest;
  regenerated Go and TS.
- New domain helper resolveCreateUserRole: empty role_id keeps
  the legacy ADMIN-default; non-empty validates org membership,
  rejects SUPER_ADMIN, and runs the parity check used by
  authorizeCallerForNewUserWithRole before persisting the
  assignment via the existing CreateUserOrganizationRole path.
- useUserManagement.createUser forwards roleId on the wire.
- New TestParseInt64RoleID and TestResolveCreateUserRole_
  ValidationBranches cover the pre-resolver rejection paths.
- Extended UserManagementStore interface with GetRoleByID;
  regenerated mock.

Swap-target clarifications:
- TODO comments renamed to authzClient.createCustomRole /
  updateCustomRole / deleteCustomRole to match real method names.
- Added RoleItem-as-client-model comment plus pbToRoleItem
  adapter notes at swap sites: proto Role uses permissionKeys
  (not permissions), numeric BuiltinKey enum (not string union),
  and Timestamp (not Date | null) for updatedAt.

Maintainability:
- isImmutable(role) helper consolidates the SUPER_ADMIN / builtin
  predicate split.
- ALL_KEYS inlined into builtin-admin; ADMIN_KEYS alias removed.
- Migration-number comment in useRoleManagement replaced with
  intent-focused description.
The E2E teamAccounts spec failed every "add member" path with
"FleetError: invalid_argument (Common: 0) invalid role_id" because
the placeholder useRoleManagement emits non-numeric ids
("builtin-field-tech", "role-<ts>") that the server's int64 parser
rejects. Strip non-numeric roleId in useUserManagement.createUser so
the server applies its default role until the real AuthzService.ListRoles
swap provides numeric ids; the guard removes itself naturally once those
ids are on the wire.
Replace the placeholder useRoleManagement and hand-maintained permission
catalog with real Connect-RPC calls. No TODOs, no in-memory mocks.

- useRoleManagement now calls authzClient.listRoles / createCustomRole /
  updateCustomRole / deleteCustomRole through the same useAuthErrors +
  getErrorMessage wrapper the other hooks use. A pbToRoleItem adapter
  converts authz.v1.Role to the client RoleItem model — maps the
  BuiltinKey numeric enum to the existing BuiltinRoleKey string union and
  the Timestamp updated_at to Date. Local validation is dropped; server
  errors surface via getErrorMessage.
- permissionCatalog becomes a usePermissionCatalog hook that fetches
  authzClient.listPermissions on first mount, caches at module scope so
  every consumer shares one fetch, and returns bound requiredReadsFor /
  withRequiredReads / lockedReadKeys helpers along with permissionGroups
  derived from the fetched flat list. The hand-maintained PERMISSION_CATALOG
  constant is gone — the server's catalog-declaration order is now the
  source of truth.
- CreateEditRoleModal consumes the hook, shows a loading/error fallback
  while the catalog fetches, and adds a Description input (max 1024 chars
  per the proto validator). Description is now a required parameter on
  CreateRoleProps and UpdateRoleProps; the preserve-on-update workaround
  is gone.
- useUserManagement.createUser drops the wireRoleId regex guard. Real
  role ids are numeric int64 strings the server accepts directly.

Storybook stories for Roles, CreateEditRoleModal, and DeleteRoleDialog
deleted — they were authored against the in-memory placeholder dataset.
The repo has no MSW/Storybook RPC-mock infrastructure to rebuild them on
top of, and they were authoring tools, not production code.
Copilot AI review requested due to automatic review settings June 3, 2026 18:14
@mcharles-square mcharles-square requested a review from a team as a code owner June 3, 2026 18:14
@github-actions github-actions Bot added javascript Pull requests that update javascript code client server shared labels Jun 3, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 3, 2026

🔐 Codex Security Review

Note: This is an automated security-focused code review generated by Codex.
It should be used as a supplementary check alongside human review.
False positives are possible - use your judgment.

Scope summary

  • Reviewed pull request diff only (ac9bde7e680d2599298037ca5d2cb25b9a635251...759185577516a631fe14bab75a2dc79d7006c7ff, exact PR three-dot diff)
  • Model: gpt-5.5

💡 Click "edited" above to see previous reviews for this PR.


Review Summary

Overall Risk: MEDIUM

Findings

[MEDIUM] CreateUser Role Parity Check Can Race Permission Revocation

  • Category: Auth
  • Location: server/internal/domain/auth/service.go:643
  • Description: resolveCreateUserRole re-loads the caller’s permissions with s.permResolver.LoadEffective(...), which uses the base DB connection and takes no FOR UPDATE locks. This runs inside the CreateUser transaction, but the permission read is not transaction-bound or locked, unlike the authz role mutation path’s LoadEffectiveForUpdate.
  • Impact: A caller being demoted concurrently can pass the parity check using stale permissions, then still create a new account and assign a role after their authority should have been revoked. For example, a demoted role manager could race the demotion and mint another privileged account with the temporary password.
  • Recommendation: Re-check caller permissions using the same transaction and lock semantics as role mutations, e.g. expose a transaction-aware LoadEffectiveForUpdate path through the auth service/store layer and hold those locks until the user and role assignment rows are committed.

Notes

No PR-local issues found in the pool/stratum, command execution, discovery, plugin, infrastructure, Rust, or protobuf wire-format surfaces. I did not run tests; this was a static review of .git/codex-review.diff and targeted surrounding code.


Generated by Codex Security Review |
Triggered by: @mcharles-square |
Review workflow run

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fff29956af

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread client/src/protoFleet/features/settings/utils/permissionCatalog.ts
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR wires up the production Roles management UI in ProtoFleet (Settings → Roles) and extends user creation to optionally assign a role at creation time, with server-side validation/parity checks to prevent privilege escalation.

Changes:

  • Added /settings/roles page and supporting Create/Edit/Delete role UI backed by AuthzService RPCs (roles + live permission catalog).
  • Extended auth.v1.CreateUserRequest with optional role_id, and updated server CreateUser to resolve/validate the requested role (org scoping + SUPER_ADMIN rejection + parity check).
  • Updated user/role store interfaces + SQL store + mocks, and added focused unit tests for role-id parsing and validation branches.

Reviewed changes

Copilot reviewed 17 out of 20 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
client/src/protoFleet/api/useRoleManagement.ts Adds AuthzService RPC wiring + PB→UI adapter for roles.
client/src/protoFleet/features/settings/utils/permissionCatalog.ts Adds live permission-catalog fetch + cache and read-dependency helpers.
client/src/protoFleet/features/settings/components/Roles.tsx New Roles listing page with edit/delete actions and modals.
client/src/protoFleet/features/settings/components/CreateEditRoleModal.tsx New role builder modal using live permission catalog + dependency locking.
client/src/protoFleet/features/settings/components/DeleteRoleDialog.tsx New delete confirmation dialog with member-count guard messaging.
client/src/protoFleet/features/settings/components/AddTeamMemberModal.tsx Adds role select to user-creation modal and forwards roleId.
client/src/protoFleet/api/useUserManagement.ts Forwards roleId to authClient.createUser.
client/src/protoFleet/config/navItems.ts Adds gated Settings → Roles nav entry (role:manage).
client/src/protoFleet/router.tsx Adds /settings/roles route.
client/src/protoFleet/routePrefetch.ts Adds lazy import/prefetch for Roles route.
client/src/protoFleet/features/settings/utils/formatRole.ts Formats FIELD_TECH label.
client/src/protoFleet/features/settings/utils/formatRole.test.ts Adds unit coverage for FIELD_TECH formatting.
proto/auth/v1/auth.proto Adds optional string role_id = 2 to CreateUserRequest.
server/internal/domain/auth/service.go Adds resolveCreateUserRole + parseInt64RoleID and integrates into CreateUser.
server/internal/domain/auth/service_test.go Adds tests for role-id parsing and resolver validation branches.
server/internal/domain/stores/interfaces/user.go Extends Role shape + adds GetRoleByID to UserManagementStore.
server/internal/domain/stores/sqlstores/user.go Implements GetRoleByID and normalizes role row → domain mapping.
server/internal/domain/stores/interfaces/mocks/mock_user_store.go Updates mock for new store method.
server/generated/grpc/auth/v1/auth.pb.go Regenerated output for new proto field (not reviewed).
client/src/protoFleet/api/generated/auth/v1/auth_pb.ts Regenerated output for new proto field (not reviewed).
Files not reviewed (1)
  • server/internal/domain/stores/interfaces/mocks/mock_user_store.go: Language not supported

Comment thread client/src/protoFleet/features/settings/utils/permissionCatalog.ts
Comment thread client/src/protoFleet/api/useRoleManagement.ts Outdated
Two MEDIUM-severity findings from the automated security review:

1. Hidden fleet:read could persist after deselection — CreateEditRoleModal
   stored an additive `selected` set so the dependency-floor reads
   (miner:read, fleet:read) auto-injected when a miner action was checked
   stayed in state after the action was unchecked. A user toggling
   "miner:reboot" on then off would save a role they thought was empty
   but that actually granted fleet dashboard / miner-list / telemetry
   visibility.

   Switched the modal to track the user's explicit selection separately
   and derive `selected = withRequiredReads(explicit)` via useMemo on
   every render. Deselecting the last dependent action drops its derived
   reads automatically; explicit toggling of a read (a "miner:read only"
   role) still works as before.

2. AddTeamMemberModal could fail open to ADMIN — Save was enabled before
   listRoles finished and after listRoles errors. With the server's
   legacy empty-role-id fallback assigning ADMIN, that produced an
   accidental admin-account creation path.

   Modal now tracks isLoadingRoles and rolesError state. Save is disabled
   while listRoles is in flight, disabled if listRoles failed (with an
   inline Callout naming the error), and disabled until a non-empty
   roleId is selected. A defensive guard in handleCreateUser refuses
   submission if roleId is empty even when the gate is bypassed. Every
   CreateUser call from this surface now carries an explicit role.

   Test coverage updated: vi.mock for useRoleManagement, three new cases
   covering enabled-after-load / disabled-while-loading / disabled-on-error.
@mcharles-square
Copy link
Copy Markdown
Collaborator Author

Addressing both Codex Security Review MEDIUM findings:

[Codex Security Review — MEDIUM] Hidden fleet:read Permission Can Persist After Deselection

Addressed in 1bd88be7: switched CreateEditRoleModal to track the user's explicit selection separately; the effective selected set (the one written on save and used for the locked-read UX) is now derived via useMemo from explicit on every render. Deselecting the last dependent miner action drops its derived reads automatically since they're never persisted in state — miner:read and fleet:read no longer survive when their dependent action is unchecked. Explicit toggling of a read (a "miner:read only" role) still works as before.

[Codex Security Review — MEDIUM] Add-Member Flow Fails Open To ADMIN When Role Selection Is Empty

Addressed in 1bd88be7: AddTeamMemberModal now tracks isLoadingRoles and rolesError state. Save is disabled while listRoles is in flight, disabled when no roleId is selected, and disabled when listRoles failed (with an inline Callout naming the error so the user can dismiss/retry). A defensive guard in handleCreateUser returns early if roleId is unexpectedly empty even after the gate. The server's legacy fallback to ADMIN on empty role_id is now unreachable from this UI surface — every CreateUser call from the add-member flow carries an explicit role.

Validation: tsc --noEmit clean; vitest 334/334 pass (3 new cases covering enabled-after-load / disabled-while-loading / disabled-on-error).

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1bd88be733

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread client/src/protoFleet/features/settings/components/CreateEditRoleModal.tsx Outdated
Four follow-on findings from the Codex / Copilot re-review:

- Cancel in-flight `listRoles` when AddTeamMemberModal closes. A
  cleanup flag on the useEffect no-ops the success/error/finally
  callbacks once the modal has dismissed, preventing a stale roleId
  from being repopulated on the next open and silencing the
  state-update-on-hidden warning.
- Derive the fleet read key in `requiredReadsFor` from the catalog
  lookup rather than hardcoding `"fleet:read"`. If the server renames
  the key, the client mirror picks it up on the next ListPermissions
  response without a code change. Also reuses the resource→read map
  so the same lookup serves both same-resource and miner-floor reads.
- Update the `RoleItem.builtin` doc comment to state that all
  built-in roles are immutable from the client and rejected by the
  server. The previous comment singled out SUPER_ADMIN but the
  isImmutable helper and the UI hide Edit/Delete on every built-in.
- Migrate `Team.tsx` from `useRole() === "SUPER_ADMIN"` to
  `useHasPermission("user:manage")`. The TODO above the old gate
  called out exactly this migration; its prerequisite (CreateUser
  accepting role_id with server-side parity check) lands in this PR,
  so the SUPER_ADMIN-only restriction can lift. The modal's existing
  Owner-exclusion plus the server's parity check keep ADMINs from
  creating accounts that exceed their own permissions.
CreateEditRoleModal seeded `explicit` from every key in
`role.permissions`, which on an edit dragged in the server-required
auto-injected reads from non-rendered groups — `fleet:read` chief
among them, since `fleet` is intentionally excluded from
RESOURCE_TO_GROUP. With that hidden key in `explicit`, deselecting
every visible miner permission still left `fleet:read` in the
derived `selected` set; Save persisted a role that looked empty in
the UI but actually granted fleet dashboard visibility.

Filter the initial explicit seed to keys the user can see and toggle
(computed from `permissionGroups` already passed in as a prop). On
edit, the hidden `fleet:read` is now derived solely from
`withRequiredReads` over the visible miner actions, so deselecting
those actions drops it the same way it drops on create.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1c5b799079

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Two server-side findings:

- ListRoles gated on role:manage blocks user:manage callers from
  loading the assignable-role list, so the built-in ADMIN (which
  intentionally lacks role:manage) can open the Add team member
  modal but can't submit. Added middleware.RequireAnyPermission and
  switched the ListRoles handler to accept role:manage OR
  user:manage. The privilege gate against escalation is the parity
  check on CreateUser (and the role:manage-only gates on every
  mutation RPC, which stay as-is); ListRoles is a read-only surface
  whose information is legitimately useful to both permission
  holders. Proto comment on AuthzService documents the exception.
  Eight unit tests on RequireAnyPermission plus a handler
  integration test asserting ListRoles succeeds for a
  user:manage-only caller and denies a caller holding neither.

- CreateUser resolved and authorized the role outside RunInTx, then
  used the resolved role inside the tx for the assignment write. A
  concurrent DeleteCustomRole between resolution and assignment
  could leave the new user assigned to a soft-deleted role (the FK
  only enforces row existence, not deleted_at IS NULL). Moved
  resolveCreateUserRole inside the RunInTx block so it runs through
  the transactional *sqlc.Queries. GetRoleByID already filters
  deleted_at IS NULL, so a concurrent delete that commits first
  surfaces as InvalidArgument cleanly; a delete that commits after
  the assignment write lands on a non-deleted row at write time.
  Updated the resolver's doc to record the in-tx contract so a
  future caller doesn't reintroduce the race.
@mcharles-square
Copy link
Copy Markdown
Collaborator Author

Addressing both findings from the latest Codex Security Review:

[Codex Security Review — MEDIUM] Add-member flow is exposed to user:manage users but depends on role:manage

Addressed in e35d9bce: added middleware.RequireAnyPermission and switched the ListRoles handler to accept role:manage OR user:manage. The privilege gate against escalation is the parity check on CreateUser (which inspects the caller's effective permissions and refuses any role that exceeds them) plus the existing role:manage-only gates on every mutation RPC (CreateCustomRole/UpdateCustomRole/DeleteCustomRole/AssignRole/UnassignRole) — those stay as-is. ListPermissions also stays role:manage-only. New handler integration test covers ListRoles accepting a user:manage-only caller and denying a caller holding neither.

[Codex Security Review — LOW] Explicit role assignment is validated before the create-user transaction

Addressed in e35d9bce: moved resolveCreateUserRole into the RunInTx block in CreateUser so it runs through the transactional *sqlc.Queries. GetRoleByID already filters deleted_at IS NULL, so a concurrent DeleteCustomRole that commits before the in-tx resolver reads surfaces as InvalidArgument cleanly; a delete that commits after the assignment write lands on a non-deleted row at write time. The resolver's doc records the in-tx contract so a future caller doesn't reintroduce the race.

Validation: go build ./... clean; go vet ./... clean; go test ./internal/handlers/middleware/... ./internal/handlers/authz/... ./internal/domain/auth/... passes (including the new 8-case TestRequireAnyPermission and the user:manage-only handler integration test).

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e35d9bcef7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread server/internal/domain/auth/service.go
The previous commit added a service-level comment on AuthzService
documenting the ListRoles role:manage-or-user:manage exception.
Connect's TS generator embeds leading service comments as JSDoc on
the generated service descriptor, so authz_pb.ts went out of sync.
just gen brings it back in line; the corresponding pb.go file
doesn't embed service-level leading comments so it didn't change.
Comment thread server/internal/domain/auth/service.go Outdated
Comment thread server/internal/domain/auth/service.go Outdated
- Lock role row via GetRoleByIDForUpdate in resolveCreateUserRole and
  DeleteCustomRole so a concurrent delete↔create-assignment race
  serializes on the role row instead of orphaning the new user on a
  soft-deleted role.
- Extract ParseRoleID into the authz domain package and reuse it from
  the authz Connect handler and auth service.
- Reject empty role_id on CreateUser instead of defaulting to ADMIN —
  callers must pick a role explicitly so the server cannot silently
  mint an admin account.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0150a2f51

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread proto/auth/v1/auth.proto Outdated
The proto comment still advertised an optional role_id with a default,
but the server now rejects empty values with InvalidArgument. Update
the contract to match — drop the `optional` modifier, regenerate
auth.pb.go and auth_pb.ts, and update the doc comment so external
callers see the actual server behavior.
@mcharles-square
Copy link
Copy Markdown
Collaborator Author

[Codex Security Review – MEDIUM] Create-user role assignment can race with custom-role deletion (report)

Addressed in d0150a2. Added GetRoleByIDForUpdate (FOR UPDATE on the role row, still filtered by deleted_at IS NULL) and used it from both resolveCreateUserRole and DeleteCustomRole's new getRoleInOrgForUpdate helper. The two operations now serialize on the same role row: a racing delete either commits first (our locked re-read sees deleted_at set and we surface InvalidArgument) or blocks until the assignment commits (it then sees count > 0 and refuses). Either ordering, the new user is never bound to a soft-deleted role.

The Add team member modal now defaults the role picker to FIELD_TECH
(least privileged built-in) and the server rejects empty role_id, so
the "Add team member" test's implicit-default new user is created with
Field Tech rather than the legacy ADMIN. Update the role assertion to
match the new default.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7591855775

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// never renders as a manually toggleable checkbox in the role builder —
// it is the dependency floor for every miner action and is auto-included
// by `withRequiredReads` whenever a miner action is selected.
const RESOURCE_TO_GROUP: Record<string, string> = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep fleet:read editable in the role builder

When fleet is omitted from the rendered groups, the role UI has no way to grant the valid standalone fleet:read permission (the server catalog and validator allow read-only roles). Fresh evidence after the earlier hidden-permission fix is that editing any custom role that currently has fleet:read but only visible read permissions (for example an API-created dashboard/miner viewer with fleet:read + miner:read and no miner action) will seed only visible keys and save back without fleet:read, silently removing dashboard/telemetry access; render this read key or preserve standalone hidden reads instead of only auto-adding it for miner actions.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client javascript Pull requests that update javascript code server shared

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants