increment 2 done #38

Merged

navicore merged 3 commits from increment-2 into main

2026-05-29 14:25:51 +00:00

navicore commented

2026-05-29 13:47:24 +00:00

Owner

┌─────────────┬─────────────────────────────────┬──────────────────────────────────────────────────────────┐
│ Piece │ File │ Notes │
├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤
│ JSON │ src/admin_api.rs (new │ RealmDto, UserDto, ClientDto, SigningKeyDto, SessionDto, │
│ contract │ top-level) │ ListResponse. Pure data — no axum, no rusqlite — so │
│ DTOs │ │ the future CLI imports it unchanged. │
├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤
│ Auth module │ src/server/admin/auth.rs │ The extractor + authorize_admin + audit-on-denial, │
│ │ │ relocated from mod.rs; behavior unchanged. │
├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤
│ List │ src/server/admin/lists.rs │ Five GET routes; each is a thin domain-model → DTO │
│ handlers │ │ mapper over existing db:: query functions. │
├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤
│ Shared test │ src/server/admin/testing.rs │ mint, router_with, status, response, MASTER_ISSUER — │
│ helpers │ (#[cfg(test)]) │ reused across auth and lists test suites. │
├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤
│ Lean module │ src/server/admin/mod.rs │ Module declarations + admin_router (now wiring 7 routes) │
│ root │ │ + health/whoami + AdminContext re-export. ~67 lines. │
└─────────────┴─────────────────────────────────┴──────────────────────────────────────────────────────────┘

Routes added:

GET /admin/realms
GET /admin/realms/{realm}/users
GET /admin/realms/{realm}/clients
GET /admin/realms/{realm}/keys
GET /admin/realms/{realm}/users/{username}/sessions

Security posture verified by tests (every contract failure would be the kind of thing a reviewer would catch):

password_hash never appears in UserDto
client_secret_hash is replaced by has_secret: bool — the hash is never on the wire
signing-key responses carry only metadata (kid, algorithm, active, created_at); no PEM material
session responses are row-id + timestamps only; session tokens stay SHA-256-hashed in the DB
every list endpoint goes through AdminContext (asserted at the route level by list_realms_requires_admin)
unknown realm → 404; unknown user → 404

Same merge recommendation: ship this as its own PR. The CLI doesn't change in this increment — the DTOs are the
contract that Increment 4's CLI backend will consume — so review surface stays tight and the wire format gets
locked in main before write endpoints land in Increment 3.

┌─────────────┬─────────────────────────────────┬──────────────────────────────────────────────────────────┐ │ Piece │ File │ Notes │ ├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤ │ JSON │ src/admin_api.rs (new │ RealmDto, UserDto, ClientDto, SigningKeyDto, SessionDto, │ │ contract │ top-level) │ ListResponse<T>. Pure data — no axum, no rusqlite — so │ │ DTOs │ │ the future CLI imports it unchanged. │ ├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤ │ Auth module │ src/server/admin/auth.rs │ The extractor + authorize_admin + audit-on-denial, │ │ │ │ relocated from mod.rs; behavior unchanged. │ ├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤ │ List │ src/server/admin/lists.rs │ Five GET routes; each is a thin domain-model → DTO │ │ handlers │ │ mapper over existing db:: query functions. │ ├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤ │ Shared test │ src/server/admin/testing.rs │ mint, router_with, status, response, MASTER_ISSUER — │ │ helpers │ (#[cfg(test)]) │ reused across auth and lists test suites. │ ├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤ │ Lean module │ src/server/admin/mod.rs │ Module declarations + admin_router (now wiring 7 routes) │ │ root │ │ + health/whoami + AdminContext re-export. ~67 lines. │ └─────────────┴─────────────────────────────────┴──────────────────────────────────────────────────────────┘ Routes added: - GET /admin/realms - GET /admin/realms/{realm}/users - GET /admin/realms/{realm}/clients - GET /admin/realms/{realm}/keys - GET /admin/realms/{realm}/users/{username}/sessions Security posture verified by tests (every contract failure would be the kind of thing a reviewer would catch): - password_hash never appears in UserDto - client_secret_hash is replaced by has_secret: bool — the hash is never on the wire - signing-key responses carry only metadata (kid, algorithm, active, created_at); no PEM material - session responses are row-id + timestamps only; session tokens stay SHA-256-hashed in the DB - every list endpoint goes through AdminContext (asserted at the route level by list_realms_requires_admin) - unknown realm → 404; unknown user → 404 Same merge recommendation: ship this as its own PR. The CLI doesn't change in this increment — the DTOs are the contract that Increment 4's CLI backend will consume — so review surface stays tight and the wire format gets locked in main before write endpoints land in Increment 3.

navicore added 1 commit

2026-05-29 13:47:25 +00:00

increment 2 done

CI / ci (pull_request) Failing after 4s

Details

17bd7b3f11

┌─────────────┬─────────────────────────────────┬──────────────────────────────────────────────────────────┐
  │    Piece    │              File               │                          Notes                           │
  ├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤
  │ JSON        │ src/admin_api.rs (new           │ RealmDto, UserDto, ClientDto, SigningKeyDto, SessionDto, │
  │ contract    │ top-level)                      │  ListResponse<T>. Pure data — no axum, no rusqlite — so  │
  │ DTOs        │                                 │ the future CLI imports it unchanged.                     │
  ├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤
  │ Auth module │ src/server/admin/auth.rs        │ The extractor + authorize_admin + audit-on-denial,       │
  │             │                                 │ relocated from mod.rs; behavior unchanged.               │
  ├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤
  │ List        │ src/server/admin/lists.rs       │ Five GET routes; each is a thin domain-model → DTO       │
  │ handlers    │                                 │ mapper over existing db:: query functions.               │
  ├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤
  │ Shared test │ src/server/admin/testing.rs     │ mint, router_with, status, response, MASTER_ISSUER —     │
  │  helpers    │ (#[cfg(test)])                  │ reused across auth and lists test suites.                │
  ├─────────────┼─────────────────────────────────┼──────────────────────────────────────────────────────────┤
  │ Lean module │ src/server/admin/mod.rs         │ Module declarations + admin_router (now wiring 7 routes) │
  │  root       │                                 │  + health/whoami + AdminContext re-export. ~67 lines.    │
  └─────────────┴─────────────────────────────────┴──────────────────────────────────────────────────────────┘

  Routes added:
  - GET /admin/realms
  - GET /admin/realms/{realm}/users
  - GET /admin/realms/{realm}/clients
  - GET /admin/realms/{realm}/keys
  - GET /admin/realms/{realm}/users/{username}/sessions

  Security posture verified by tests (every contract failure would be the kind of thing a reviewer would catch):
  - password_hash never appears in UserDto
  - client_secret_hash is replaced by has_secret: bool — the hash is never on the wire
  - signing-key responses carry only metadata (kid, algorithm, active, created_at); no PEM material
  - session responses are row-id + timestamps only; session tokens stay SHA-256-hashed in the DB
  - every list endpoint goes through AdminContext (asserted at the route level by list_realms_requires_admin)
  - unknown realm → 404; unknown user → 404

  Same merge recommendation: ship this as its own PR. The CLI doesn't change in this increment — the DTOs are the
  contract that Increment 4's CLI backend will consume — so review surface stays tight and the wire format gets
  locked in main before write endpoints land in Increment 3.

navicore commented

2026-05-29 14:15:19 +00:00

Author

Owner

Review: Increment 2 — shared DTOs + read-only admin endpoints

Verdict: solid, ready to merge. Clean refactor + five read-only endpoints, with the wire contract locked in src/admin_api.rs before write endpoints land. Verified locally: clippy clean with -D warnings, 134 tests pass. Both notes from the Increment 1 review are addressed (see below).

Increment 1 follow-ups — both resolved

Audit on denial. authorize_admin now emits admin_access_denied on every 401/403 with a detail reason (no_bearer / invalid_token / foreign_realm / disabled / no_admin_group / unknown_subject) and the caller IP (via ConnectInfo, gracefully None under oneshot). Locked by denials_are_audited_with_reason.
Misconfig vs. authz. A missing admin realm now returns 500 Internal with a tracing::error!, instead of a 403 — no longer conflating server misconfiguration with an authorization denial, and audit logs aren't polluted with bogus "denials".

Verified against the code

Secret stripping is real, not just intent. password_hash is absent from UserDto; client_secret_hash → has_secret: bool; SigningKeyDto is metadata-only (no PEM); SessionDto is row-id + timestamps (session tokens stay SHA-256-hashed in storage). Each is asserted by a dedicated test that checks the field is absent from the JSON, not merely empty.
Timestamps are RFC 3339 end-to-end. realm/user/client map via .to_rfc3339(); signing_key/session pass their column through directly, and those columns are themselves written with .to_rfc3339() at insert — so the contract holds across both paths.
list_sessions "active sessions" wording is accurate — the underlying query filters expires_at > now, so expired rows don't leak into the response.
DTO module is genuinely server-agnostic — no axum, no rusqlite — so the Increment 4 CLI can import it unchanged. Domain → DTO conversion stays inline in the handlers.
Uniform authz — every list handler takes AdminContext, so auth runs before the body. ListResponse<T> envelope is a good forward-compat choice (pagination/totals later without a breaking change).

Worth noting (none blocking — forward-looking)

Lists are unbounded. No pagination yet — a large realm's users or a busy user's sessions returns everything in one response. The ListResponse<T> envelope exists precisely so pagination can be added without breaking the contract; just flagging that the cap isn't there yet.
Admin-gate coverage is representative, not per-route. list_realms_requires_admin asserts the 401 path on one endpoint. Since all five share the same AdminContext extractor that's reasonable, but a one-line unauth assertion per route would make the guarantee explicit and catch a future copy-paste that forgets the extractor.

## Review: Increment 2 — shared DTOs + read-only admin endpoints **Verdict: solid, ready to merge.** Clean refactor + five read-only endpoints, with the wire contract locked in `src/admin_api.rs` before write endpoints land. Verified locally: clippy clean with `-D warnings`, 134 tests pass. Both notes from the Increment 1 review are addressed (see below). ### Increment 1 follow-ups — both resolved - **Audit on denial.** `authorize_admin` now emits `admin_access_denied` on every 401/403 with a `detail` reason (`no_bearer` / `invalid_token` / `foreign_realm` / `disabled` / `no_admin_group` / `unknown_subject`) and the caller IP (via `ConnectInfo`, gracefully `None` under `oneshot`). Locked by `denials_are_audited_with_reason`. - **Misconfig vs. authz.** A missing admin realm now returns **500 Internal** with a `tracing::error!`, instead of a 403 — no longer conflating server misconfiguration with an authorization denial, and audit logs aren't polluted with bogus "denials". ### Verified against the code - **Secret stripping is real, not just intent.** `password_hash` is absent from `UserDto`; `client_secret_hash` → `has_secret: bool`; `SigningKeyDto` is metadata-only (no PEM); `SessionDto` is row-id + timestamps (session tokens stay SHA-256-hashed in storage). Each is asserted by a dedicated test that checks the field is *absent* from the JSON, not merely empty. - **Timestamps are RFC 3339 end-to-end.** `realm`/`user`/`client` map via `.to_rfc3339()`; `signing_key`/`session` pass their column through directly, and those columns are themselves written with `.to_rfc3339()` at insert — so the contract holds across both paths. - **`list_sessions` "active sessions" wording is accurate** — the underlying query filters `expires_at > now`, so expired rows don't leak into the response. - **DTO module is genuinely server-agnostic** — no axum, no rusqlite — so the Increment 4 CLI can import it unchanged. Domain → DTO conversion stays inline in the handlers. - **Uniform authz** — every list handler takes `AdminContext`, so auth runs before the body. `ListResponse<T>` envelope is a good forward-compat choice (pagination/totals later without a breaking change). ### Worth noting (none blocking — forward-looking) 1. **Lists are unbounded.** No pagination yet — a large realm's `users` or a busy user's `sessions` returns everything in one response. The `ListResponse<T>` envelope exists precisely so pagination can be added without breaking the contract; just flagging that the cap isn't there yet. 2. **Admin-gate coverage is representative, not per-route.** `list_realms_requires_admin` asserts the 401 path on one endpoint. Since all five share the same `AdminContext` extractor that's reasonable, but a one-line unauth assertion per route would make the guarantee explicit and catch a future copy-paste that forgets the extractor.

navicore added 1 commit

2026-05-29 14:20:13 +00:00

Summary of the response:

CI / ci (pull_request) Failing after 4s

Details

8401026d8c

- Nit #2 (per-route auth) — addressed. list_realms_requires_admin replaced with
  every_list_endpoint_requires_admin, which iterates all five list paths and asserts 401 on each, with the failing
   path quoted in the assertion message. The comment explicitly flags it as the list to update when new routes
  land — so when I add write endpoints in Increment 3, this is the canary that fires if I forget the extractor.
  - Nit #1 (pagination) — explicitly deferred. The ListResponse<T> envelope is the forward-compat seam; adding
  pagination is a real design pass (cursor vs offset, default page size, total counts) that deserves its own
  focused increment.

navicore added 1 commit

2026-05-29 14:21:43 +00:00

fmt

CI / ci (pull_request) Successful in 3m41s

Details

5c163c23e7

navicore merged commit e105d8c9bb into main

2026-05-29 14:25:51 +00:00

navicore deleted branch increment-2

2026-05-29 14:25:51 +00:00

navicore referenced this pull request from a commit

2026-05-29 14:25:52 +00:00