notifications (3/5): notification:read and notification:manage permissions

[illegalprime]

Stack 3/5 — base: `eden/notifications-2-history-store`

Adds authz for the notifications surface.

• New `notification` resource with `notification:read` / `notification:manage` permissions in the authz catalog
• Maps every notifications RPC to a permission: reads on `:read`, all mutations (incl. `TestChannel`, which triggers an outbound delivery) on `:manage`
• Seeds the permissions via migration

Stack

1. proto + generated code
2. notification_history keyset listing
3. this PR — notification:read / notification:manage permissions
4. Grafana-backed service, handlers, server wiring
5. notifications UI

🤖 Generated with Claude Code

[github-actions]

🔐 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 (b89405f73516fc9f89396a4197ef438f272e1855...8cd5d9a236df947116be263b82f3ac00ee5b9ceb, exact PR three-dot diff)
• Model: gpt-5.5

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

---

Review Summary

Overall Risk: LOW

Findings

[LOW] Notification RPCs Are Classified Before They Are Registered or Enforced

• Category: Auth | gRPC
• Location: server/internal/handlers/middleware/rpc_permissions.go:252
• Description: The PR adds notification RPCs to ProcedurePermissions, but this commit does not register the notification Connect services in server/cmd/fleetd/main.go and I found no concrete notification handler package enforcing RequirePermission. Since ProcedurePermissions is a test/audit classification map rather than runtime enforcement, these entries currently describe authorization coverage for unreachable procedures.
• Impact: No current reachable auth bypass in this exact commit, but this weakens the RBAC guardrail. A later PR could mount notification services and pass the procedure classification check because the entries already exist, even if the new handlers forget to call RequirePermission.
• Recommendation: Either remove these notification entries until the services and handlers are mounted with handler-level RequirePermission calls, or add a contract test that every ProcedurePermissions entry corresponds to a procedure registered on the production mux. Add targeted handler authorization tests when the notification services become reachable.

Notes

The new permission catalog entries and 000088 migration use static SQL and follow the existing ADMIN backfill pattern. I did not find SQL injection, cryptostealing/pool hijack behavior, command injection, or protobuf wire-format issues in the reviewed diff. git diff --check passed.

---

_{Generated by Codex Security Review |
Triggered by: @illegalprime |
Review workflow run}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 507a4e593d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Renumber the notification seed after existing 000088

This stack's base already contains 000088_notification_history_keyset_index, and runMigrations advances the DB with m.Up(). Any environment that has already run that parent stack will have schema_migrations at version 88, so adding this backfill as a new lower-numbered 000087 migration means it will not run there. In that upgrade path the new notification:* gates are present, but existing ADMIN roles never receive these grants, so the notifications RPCs/UI will be denied for current org admins; make this the next migration after 000088 instead of inserting it below the current version.

Useful? React with 👍 / 👎.

[illegalprime]

Codex finding [MEDIUM] services in RBAC contract but not mounted is a split artifact: this PR adds the permission map + registeredServices entries, and #455 mounts the four notificationsv1connect handlers in main.go. The contract and the mount land in adjacent PRs in the stack; nothing is missing once #455 merges. No change needed here.

[illegalprime]

Re the re-review:

• [MEDIUM] out-of-order migration won't backfill upgraded DBs — flagging as a merge-timing note, not a present bug. main's current max migration is 000086; this stack's 000087/000088 are the next sequential versions, so they apply cleanly on any main-based DB. Also, the permission rows are upserted at every startup by authz.Reconcile (main.go:204 → upsertCatalog), so they're never at risk. Migration 087's unique job is the one-time grant of notification:* onto existing ADMIN roles (the reconciler is additive by design and won't re-assert onto already-seeded roles). That backfill is only skipped if 087 ends up numbered below an already-applied version — i.e. only if other migrations land on main numbered ≥087 before this stack merges. Merge-time action: confirm 087/088 are still above main's max at merge; renumber if not. Renumbering now would be premature.
• [MEDIUM] services registered but not mounted — unchanged disposition: split artifact, notifications (4/5): Grafana-backed service, handlers, and server wiring #455 mounts the four handlers in main.go. No change here.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 66e55db8f5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Drop unmounted notification services from contract list

These entries make TestRPCContract_RegisteredServicesMatchMainMux fail in this stack because that test compares registeredServices against the handlers actually mounted in cmd/fleetd/main.go, and the current mux has no notificationsv1connect.New...ServiceHandler registrations (the commit message says wiring lands in stack 4). Until the notification handlers are mounted in main.go, listing them here leaves missingFromMux populated for all four services and blocks the middleware package tests.

Useful? React with 👍 / 👎.

[illegalprime]

codex-connector triage (both fixed):

• [P1] Renumber the seed after 000088 — fixed: seed is now 000088 (above the keyset index, now 000087 in notifications (2/5): notification_history keyset listing #453), so numeric order matches merge order and golang-migrate won't skip it.
• [P2] Drop unmounted services from the contract list — fixed: the registeredServices entries (which TestRPCContract_RegisteredServicesMatchMainMux compares against the main.go mux) moved to notifications (4/5): Grafana-backed service, handlers, and server wiring #455 where the handlers are mounted. The ProcedurePermissions policy map stays here. go test ./internal/handlers/middleware is green on both PRs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d2685fc9dd

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Wire notification permissions into the role builder

When the ListPermissions response includes these new ResourceNotification entries, the ProtoFleet role builder currently drops any resource not listed in RESOURCE_TO_GROUP and iterates only GROUP_ORDER (client/src/protoFleet/features/settings/utils/permissionCatalog.ts:29-62); CreateEditRoleModal then builds visibleKeys from those groups and filters role permissions through it before saving (client/src/protoFleet/features/settings/components/CreateEditRoleModal.tsx:121-125). As a result, custom roles cannot be granted notification:read/notification:manage in the UI, and editing an existing custom role with those keys will remove them on save; add notification to the client group mapping/order with this catalog addition.

Useful? React with 👍 / 👎.