feat/v0-3-dashboard-role-enforcement #293

Open

johnride wants to merge 2 commits from feat/v0-3-dashboard-role-enforcement into feat/smoke-test-contract

Author	SHA1	Message	Date
Jean-Gabriel Gill-Couture	e9522464af	refactor(zitadel-auth): typed Role/Roles, drop string-path role lookup All checks were successful Run Check Script / check (pull_request) Successful in 2m21s Details Removes the brittle pieces that landed in the original role-enforcement commit: `extract_roles(value, path)` walking `serde_json::Value` with a `path.contains("urn:")`-aware splitter, an env-driven `FLEET_AUTH_ROLES_CLAIM`, and `VerifiedSession.roles: Vec<String>` compared with `iter().any(\|r\| r == "fleet-admin")` in middleware. This is a security-relevant code path; runtime string heuristics inside it hide injection-shaped bugs (a misconfigured env or a custom Zitadel mapper shifts the lookup to a path the attacker controls). Replaced with end-to-end typed serde decoding: * `Role` — closed enum (one variant today: `FleetAdmin`). Adding a variant is a deliberate code change to the security path. Unknown role names emitted by the IdP cannot be represented and therefore cannot grant access. * `RoleClaims` — wire-side struct, flattened into the JWT `Claims`. The two well-known role-claim locations are matched verbatim by `#[serde(rename = "urn:zitadel:iam:org:project:roles")]` and `#[serde(rename = "roles")]`. No dotted-path navigation. No env string. If a future issuer adds a third location it is an additive `#[serde(rename = ...)]` field inside the security boundary. * `Roles` — domain value on `VerifiedSession`. Construction is restricted to `RoleClaims::into_roles` plus a typed `FromIterator<Role>` for test fixtures. `Roles::has(Role::FleetAdmin)` is the only check the middleware needs; no string comparison exists anywhere downstream. * Malformed shapes (scalar at the URN path, mixed-type array) now ERROR at decode rather than degrading to an empty-vec "closed door". Fail-loud is the security-correct default when the IdP misbehaves — the user re-logs in, the operator notices. Callout side: reverted the shared-extract_roles delegation. The callout retains its own, unchanged role-extraction logic. We do NOT need cross-crate sharing here, and the shared extractor was the entry point we were trying to delete — the callout's own behaviour was the status quo and is preserved verbatim. Dropped exports: `DEFAULT_ADMIN_ROLE`, `DEFAULT_ROLES_CLAIM`, `extract_roles`, `ROLES_CLAIM_ENV`, `ZitadelAuthConfig.roles_claim`. None had external consumers in the workspace. Tests: * 12 tests on `RoleClaims` deserialization: both shapes resolve, Zitadel URN wins precedence, unknown roles dropped, malformed scalar/mixed-type arrays error at decode, missing claim → empty, empty object/array → empty, extra unrelated claims are ignored, display matches wire spelling. * 4 middleware tests on the typed `require_role(Role::FleetAdmin, …)` path. Dropped the redundant "admin-only vs admin+other" test — with a single-variant enum it duplicated the positive case.	2026-05-25 06:45:34 -04:00
Jean-Gabriel Gill-Couture	aad14cd04d	feat(fleet-operator): require fleet-admin role on dashboard routes Previously the dashboard granted full access to any Zitadel user with a valid JWT — issuer, audience and signature were the only checks. Operators routinely give non-admin users an account in Zitadel for SSO into adjacent apps, so a valid JWT is not by itself evidence the user should manage a fleet. This change closes that gap. Behaviour: - Verified sessions now carry a `roles: Vec<String>` extracted from a configurable JWT claim (env `FLEET_AUTH_ROLES_CLAIM`, default the Zitadel role URN). The extractor accepts both OIDC array shape and Zitadel's object-map shape; missing/malformed claim yields empty vec (closed door, never wildcard). - `require_fleet_admin` middleware layers above `require_auth` on the private routes. A logged-in user without the role gets a human- readable 403 page that names the missing role and offers a sign-out link. - `/logout` is in `public_routes` so a non-admin can switch accounts without being trapped by the gate that rendered the 403. Shared role extraction: - `harmony_zitadel_auth::roles::extract_roles` is the single source of truth for Zitadel role parsing. - The NATS auth callout's `ZitadelValidator::extract_roles` now delegates to it. Any future quirk in Zitadel's role-emission shape is fixed once. Tests: - 9 unit tests on the role extractor: both claim shapes, empty/missing fail closed, non-string array entries skipped, dotted paths, Zitadel URN paths. - 5 axum middleware tests via `tower::ServiceExt::oneshot`: no session → unauthenticated redirect, role-less session → 403 with expected role name + logout link, fleet-admin session → handler reached, HTML (not JSON) content-type on 403. Defers to follow-ups: a `fleet-viewer` read-only role; unifying the dashboard role policy with the NATS callout's per-device scoping.	2026-05-24 16:39:37 -04:00

Author

SHA1

Message

Date

Jean-Gabriel Gill-Couture

e9522464af

refactor(zitadel-auth): typed Role/Roles, drop string-path role lookup

Run Check Script / check (pull_request) Successful in 2m21s

Details

Removes the brittle pieces that landed in the original role-enforcement
commit: `extract_roles(value, path)` walking `serde_json::Value` with
a `path.contains("urn:")`-aware splitter, an env-driven
`FLEET_AUTH_ROLES_CLAIM`, and `VerifiedSession.roles: Vec<String>`
compared with `iter().any(|r| r == "fleet-admin")` in middleware.

This is a security-relevant code path; runtime string heuristics inside
it hide injection-shaped bugs (a misconfigured env or a custom Zitadel
mapper shifts the lookup to a path the attacker controls).

Replaced with end-to-end typed serde decoding:

* `Role` — closed enum (one variant today: `FleetAdmin`). Adding a
  variant is a deliberate code change to the security path. Unknown
  role names emitted by the IdP cannot be represented and therefore
  cannot grant access.
* `RoleClaims` — wire-side struct, flattened into the JWT `Claims`.
  The two well-known role-claim locations are matched verbatim by
  `#[serde(rename = "urn:zitadel:iam:org:project:roles")]` and
  `#[serde(rename = "roles")]`. No dotted-path navigation. No env
  string. If a future issuer adds a third location it is an additive
  `#[serde(rename = ...)]` field inside the security boundary.
* `Roles` — domain value on `VerifiedSession`. Construction is
  restricted to `RoleClaims::into_roles` plus a typed
  `FromIterator<Role>` for test fixtures. `Roles::has(Role::FleetAdmin)`
  is the only check the middleware needs; no string comparison exists
  anywhere downstream.
* Malformed shapes (scalar at the URN path, mixed-type array) now
  ERROR at decode rather than degrading to an empty-vec "closed door".
  Fail-loud is the security-correct default when the IdP misbehaves
  — the user re-logs in, the operator notices.

Callout side: reverted the shared-extract_roles delegation. The
callout retains its own, unchanged role-extraction logic. We do NOT
need cross-crate sharing here, and the shared extractor was the entry
point we were trying to delete — the callout's own behaviour was the
status quo and is preserved verbatim.

Dropped exports: `DEFAULT_ADMIN_ROLE`, `DEFAULT_ROLES_CLAIM`,
`extract_roles`, `ROLES_CLAIM_ENV`, `ZitadelAuthConfig.roles_claim`.
None had external consumers in the workspace.

Tests:
* 12 tests on `RoleClaims` deserialization: both shapes resolve,
  Zitadel URN wins precedence, unknown roles dropped, malformed
  scalar/mixed-type arrays error at decode, missing claim → empty,
  empty object/array → empty, extra unrelated claims are ignored,
  display matches wire spelling.
* 4 middleware tests on the typed `require_role(Role::FleetAdmin, …)`
  path. Dropped the redundant "admin-only vs admin+other" test —
  with a single-variant enum it duplicated the positive case.

2026-05-25 06:45:34 -04:00

Jean-Gabriel Gill-Couture

aad14cd04d

feat(fleet-operator): require fleet-admin role on dashboard routes

Previously the dashboard granted full access to any Zitadel user with
a valid JWT — issuer, audience and signature were the only checks.
Operators routinely give non-admin users an account in Zitadel for SSO
into adjacent apps, so a valid JWT is not by itself evidence the user
should manage a fleet. This change closes that gap.

Behaviour:
- Verified sessions now carry a `roles: Vec<String>` extracted from a
  configurable JWT claim (env `FLEET_AUTH_ROLES_CLAIM`, default the
  Zitadel role URN). The extractor accepts both OIDC array shape and
  Zitadel's object-map shape; missing/malformed claim yields empty
  vec (closed door, never wildcard).
- `require_fleet_admin` middleware layers above `require_auth` on the
  private routes. A logged-in user without the role gets a human-
  readable 403 page that names the missing role and offers a sign-out
  link.
- `/logout` is in `public_routes` so a non-admin can switch accounts
  without being trapped by the gate that rendered the 403.

Shared role extraction:
- `harmony_zitadel_auth::roles::extract_roles` is the single source
  of truth for Zitadel role parsing.
- The NATS auth callout's `ZitadelValidator::extract_roles` now
  delegates to it. Any future quirk in Zitadel's role-emission shape
  is fixed once.

Tests:
- 9 unit tests on the role extractor: both claim shapes, empty/missing
  fail closed, non-string array entries skipped, dotted paths,
  Zitadel URN paths.
- 5 axum middleware tests via `tower::ServiceExt::oneshot`: no
  session → unauthenticated redirect, role-less session → 403 with
  expected role name + logout link, fleet-admin session → handler
  reached, HTML (not JSON) content-type on 403.

Defers to follow-ups: a `fleet-viewer` read-only role; unifying the
dashboard role policy with the NATS callout's per-device scoping.

2026-05-24 16:39:37 -04:00

feat/v0-3-dashboard-role-enforcement #293

2 Commits