feat(auth): wire JWT middleware alongside X-User-Id shim (Phase 1.d.1-PR3)#14
Merged
Conversation
…-PR3)
Final sub-PR of Phase 1.d.1. The JWT verifier from PR2 now sits in
the request path: every `/api/v1/profiles/*` (and its nested
resources) goes through a single `authenticate` middleware that
tries Bearer JWT first, falls back to the dev X-User-Id shim if
configured, and short-circuits to 503 when neither path is wired.
The dev shim stays alive on purpose for the 1.d.2 transition window
— so existing integration tests + dev workflows keep working while
Better Auth gets stood up. Phase 1.d.2 deletes the shim branch
entirely.
Config (src/config.rs):
- New optional triple: WAVEFLOW_JWT_JWKS_URL / _ISSUER / _AUDIENCE.
All three or none — a partial config fails boot fast rather than
silently building a verifier that rejects every token.
- Config::has_jwt_config() + auth_disabled_at_boot() helpers feed
both the main.rs boot path and the api/mod.rs router wiring.
State (src/lib.rs + src/main.rs):
- AppState gains `jwt_verifier: Option<Arc<JwtVerifier>>` and
`dev_auth_enabled: bool`. The verifier is built once at boot
(bad JWKS URL fails startup, not the first request).
- AppState drops `Debug` since JwtVerifier opts out of Debug to
keep raw key bytes off log sinks.
- Boot loudly warns when auth_disabled_at_boot() — operators who
flipped a wrong env var see the issue without having to probe.
Middleware (src/middleware.rs):
- New `authenticate(State<AppState>, …)` middleware:
- 503 when neither path is configured (production gate)
- JWT-first when verifier present + Authorization header present
- Shim fallback when dev_auth_enabled and no Bearer
- 401 when JWT configured but the request has no Authorization
- JwksFetchFailed / EmptyJwks → 503; every other AuthError → 401.
Response body stays opaque; the discriminated reason lands in
tracing::warn so operators can correlate 401 spikes with bad
client config.
- `sub` is resolved via db::users::find_by_external_id — token
signed for an unknown sub → 401 (not 404), hides which sub
values exist on the box.
- The legacy `require_user_id` middleware stays for back-compat
but factored into a shared `parse_x_user_id_header` helper that
`authenticate` reuses.
API wiring (src/api/mod.rs):
- The per-resource fork between require_user_id and
reject_dev_auth_disabled is replaced by a single
`middleware::from_fn_with_state(state, authenticate)` layer
shared across profiles / libraries / tracks / playlists.
- `/api/v1/users` keeps the shim-gated open / 503 toggle — the
JWT path doesn't gate user creation because production onboarding
happens at Better Auth, not at this endpoint.
DB helper (src/db.rs):
- New `users::find_by_external_id(pool, external_id)` — single
indexed lookup (UNIQUE constraint on external_id doubles as
the index). Returns Option<i64> so the caller can map a miss to
401.
Tests:
- New tests/jwks_harness.rs — shared mock JWKS + signed-token
factory used by both tests/auth.rs (already inlined) and the
new tests/jwt_middleware.rs. The harness lives at the top level
so each consumer pulls it in via `mod jwks_harness;` — Cargo
compiles it as an empty test binary too, hence the
`#![allow(dead_code)]`.
- New tests/jwt_middleware.rs — 7 integration tests:
- valid Bearer authenticates (and the same user_id underlies
Bearer + shim, proving they target the same row)
- missing Bearer with JWT-only mode → 401
- Bearer with unknown sub → 401 (verifier passes, lookup misses)
- Bearer with bad signature → 401 (two harnesses, sign with A,
verify against B)
- Bearer without kid header → 401
- Bearer with wrong scheme (Basic) → 401
- Invalid Bearer + valid X-User-Id together → 401 (proves the
attacker can't downgrade auth by sending both headers)
- No auth configured → 503 (generalised prod-gate)
- tests/support.rs gains SpawnOptions + spawn_app_with_jwt /
spawn_app_with_jwt_and_shim wrappers so future test files can
pick the auth mode they need.
Deps:
- No new runtime deps (jsonwebtoken + reqwest already landed in
PR2).
- No new dev deps (the JWKS harness reuses PR2's rsa / rand /
base64).
What this is NOT:
- Better Auth itself — that runs separately (probably alongside
waveflow-web), this PR just speaks JWT at it.
- Deletion of the X-User-Id shim — 1.d.2 ships that once Better
Auth is deployed and the operator flips the kill switch.
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughJWT Bearer authentication middleware unified across protected routes. Configuration validates JWT parameters (all-or-none contract), initializes JwtVerifier from JWKS during startup, middleware gates all ChangesJWT Authentication & Middleware
Sequence DiagramsequenceDiagram
participant Client
participant Router
participant authenticate as Middleware authenticate
participant JwtVerifier
participant Database
participant Handler
Client->>Router: GET /api/v1/profiles<br/>Authorization: Bearer JWT
Router->>authenticate: invoke middleware
alt auth_disabled_at_boot
authenticate-->>Client: 503 Service Unavailable
else has Authorization header
authenticate->>JwtVerifier: verify(Bearer JWT)
alt JWKS fetch/parse error
JwtVerifier-->>authenticate: AuthError::JwksUnavailable
authenticate-->>Client: 503 Service Unavailable
else signature/kid/exp invalid
JwtVerifier-->>authenticate: AuthError (signature/format)
authenticate-->>Client: 401 Unauthorized
else valid JWT
JwtVerifier-->>authenticate: sub claim
authenticate->>Database: find_by_external_id(sub)
alt user not found
Database-->>authenticate: None
authenticate-->>Client: 401 Unauthorized
else user found
Database-->>authenticate: Some(user_id)
authenticate->>Handler: extensions.insert(UserId)
Handler-->>Client: 200 + response
end
end
else no Authorization, dev_auth enabled
authenticate->>authenticate: parse X-User-Id header
authenticate->>Handler: extensions.insert(UserId)
Handler-->>Client: 200 + response
else no Authorization, dev_auth disabled
authenticate-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/middleware.rs (1)
47-68: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
require_user_idduplique la logique au lieu de réutiliserparse_x_user_id_header.Le commentaire (lignes 198-200) indique que
require_user_idetauthenticatepartagentparse_x_user_id_header, maisrequire_user_idconserve sa propre implémentation inline. Si Phase 1.d.2 supprime le shim, ce n'est pas bloquant, mais la refacto promise n'est pas complète.♻️ Refacto possible
pub async fn require_user_id(mut request: Request, next: Next) -> Result<Response, StatusCode> { - let value: &HeaderValue = request - .headers() - .get(X_USER_ID_HEADER) - .ok_or(StatusCode::UNAUTHORIZED)?; - - let user_id: i64 = value - .to_str() - .ok() - .and_then(|s| s.parse().ok()) - .ok_or(StatusCode::UNAUTHORIZED)?; - - // `0` and negative ids are reserved (0 is the desktop sentinel, - // negatives are conventionally invalid) — reject so a stray - // header from a confused dev client can't sneak through. - if user_id <= 0 { - return Err(StatusCode::UNAUTHORIZED); - } - - request.extensions_mut().insert(UserId(user_id)); + let user_id = parse_x_user_id_header(request.headers())?; + request.extensions_mut().insert(UserId(user_id)); Ok(next.run(request).await) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/middleware.rs` around lines 47 - 68, require_user_id duplicates header-parsing logic; replace the inline parsing in require_user_id with a call to the shared helper parse_x_user_id_header (used by authenticate) so the parsing/validation is centralized; call parse_x_user_id_header to obtain the i64 user id, keep the non-positive-id check (user_id <= 0) if that logic isn't already in the helper (or move it into the helper), insert UserId(user_id) into request.extensions_mut(), and then call next.run(request).await so behavior is unchanged while removing the duplicated parsing code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/middleware.rs`:
- Around line 47-68: require_user_id duplicates header-parsing logic; replace
the inline parsing in require_user_id with a call to the shared helper
parse_x_user_id_header (used by authenticate) so the parsing/validation is
centralized; call parse_x_user_id_header to obtain the i64 user id, keep the
non-positive-id check (user_id <= 0) if that logic isn't already in the helper
(or move it into the helper), insert UserId(user_id) into
request.extensions_mut(), and then call next.run(request).await so behavior is
unchanged while removing the duplicated parsing code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4bcfb30b-dee3-4ebe-92ea-395cc52f9d1a
📒 Files selected for processing (9)
src/api/mod.rssrc/config.rssrc/db.rssrc/lib.rssrc/main.rssrc/middleware.rstests/jwks_harness.rstests/jwt_middleware.rstests/support.rs
PR3's commit message said the legacy `require_user_id` middleware was "factored into a shared `parse_x_user_id_header` helper that `authenticate` reuses" — but the refactor on the require_user_id side was forgotten, leaving the inline `header parse → i64 > 0 check → 401` triple duplicated. Replace the inline body with a delegation to the same helper `authenticate` calls. Single source of truth for the shim's contract: when 1.d.2 retires the X-User-Id path, deleting `parse_x_user_id_header` is enough to take both call sites with it. No behaviour change: the helper carries the identical `parse` → `<= 0` → 401 sequence the inline version did. `require_user_id` itself is now only referenced from doc-comments in `api/profiles.rs` and `api/libraries.rs` — none of the live router branches use it after PR3 wired everything through `authenticate`. Keeping it `pub` for the 1.d.2 sweep instead of deleting in two passes. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
Owner
Author
|
@coderabbitai bien vu, oversight de ma part. Le commit message de #14 promettait la refacto mais je n'avais factorisé que côté ```rust Le helper porte déjà le check Note : `require_user_id` est maintenant only référencé depuis des doc-comments dans `api/profiles.rs` + `api/libraries.rs` — aucun router live ne l'utilise depuis PR3 a wire `authenticate` partout. Je le garde `pub` pour que 1.d.2 fasse la suppression en un sweep cohérent (`require_user_id` + `parse_x_user_id_header` + `X_USER_ID_HEADER` + le `dev_auth_enabled` field d'AppState), plutôt qu'en deux passes. |
|
Tip For best results, initiate chat on the files or code changes.
(ノ✿◕ᗜ◕)ノ |
This was referenced May 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Final sub-PR of Phase 1.d.1. The JWT verifier from #12 now sits in the request path: every
/api/v1/profiles/*(and its nested resources) goes through a singleauthenticatemiddleware that tries Bearer JWT first, falls back to the devX-User-Idshim if configured, and short-circuits to 503 when neither path is wired.The dev shim stays alive on purpose for the 1.d.2 transition window so existing tests + dev workflows keep working while Better Auth gets stood up. 1.d.2 deletes the shim branch entirely.
Auth resolution matrix
dev_auth_enabledjwt_verifierThe "no downgrade" row is the load-bearing security property — an invalid Bearer alongside a forged
X-User-IdMUST 401, otherwise an attacker could downgrade auth.Changes
Config (
src/config.rs)WAVEFLOW_JWT_JWKS_URL/_ISSUER/_AUDIENCE. All three or none — a partial config fails boot fast rather than silently building a verifier that rejects every token.Config::has_jwt_config()+auth_disabled_at_boot()helpers feed both the main.rs boot path and the api/mod.rs router wiring.State (
src/lib.rs+src/main.rs)AppStategainsjwt_verifier: Option<Arc<JwtVerifier>>anddev_auth_enabled: bool. Verifier built at boot — bad JWKS URL fails startup, not the first request.AppStatedropsDebugsinceJwtVerifieropts out to keep raw key bytes off log sinks.auth_disabled_at_boot()— operators see misconfigured deploys without probing.Middleware (
src/middleware.rs)authenticate(State<AppState>, …):verify_bearer→find_by_external_id→ attachUserIdJwksFetchFailed/EmptyJwks→ 503; otherAuthError→ 401sub→ 401 (not 404, hides which sub values exist on the box)require_user_idstays for now, factored to shareparse_x_user_id_headerwithauthenticate.API wiring (
src/api/mod.rs)require_user_idandreject_dev_auth_disabledis replaced by a singlemiddleware::from_fn_with_state(state, authenticate)layer shared across profiles / libraries / tracks / playlists./api/v1/userskeeps the shim-gated open/503 toggle — JWT doesn't gate user creation since prod onboarding happens at Better Auth.DB helper (
src/db.rs)users::find_by_external_id(pool, external_id)— single indexed lookup, returnsOption<i64>.Tests
tests/jwks_harness.rs— shared mock JWKS + signed-token factory. Hosted at top level viamod jwks_harness;from consumers; Cargo compiles it as an empty test binary too, hence#![allow(dead_code)].tests/jwt_middleware.rs— 8 integration tests:valid_bearer_authenticates_request— full round-trip + proves the same user_id underlies Bearer + shimmissing_bearer_with_jwt_only_returns_401bearer_with_unknown_sub_returns_401— verifier passes, lookup missesbearer_with_bad_signature_returns_401— sign with harness A, verify against harness Bbearer_with_no_kid_returns_401bearer_with_wrong_scheme_returns_401invalid_bearer_does_not_downgrade_to_shim— the critical security propertyno_auth_configured_returns_503tests/support.rsgainsSpawnOptionsbuilder +spawn_app_with_jwt/spawn_app_with_jwt_and_shimwrappers.Deps
No new runtime deps. No new dev deps. Everything reuses what landed in PR2.
What this is NOT
Test plan
cargo check --all-targetscargo fmt --all --checkcargo clippy --all-targets -- -D warningscargo test --all(CI runs against the Postgres service container; local check passed but no Postgres so the JWT middleware tests couldn't run locally)Closes Phase 1.d.1. Refs: #11, #12, RFC-001 §6.6.
Summary by CodeRabbit
New Features
Refactor
Tests