feat(auth): lazy auto-provision users on first jwt request - #16
Merged
Conversation
Previously a valid JWT for a sub with no matching users row got 401, forcing every fresh Better Auth signup to first POST /api/v1/users via the dev shim before the JWT path would let it through. That defeats the point of removing the shim in 1.d.2 and adds a second auth channel for the bootstrap. A signed JWT from the configured Better Auth issuer IS the authoritative statement that the sub is a real user. The middleware now does an UPSERT on users(external_id) the first time a new sub shows up, using INSERT ... ON CONFLICT DO UPDATE ... RETURNING id so the resolution is one round-trip whether the row is new or pre-existing. The UNIQUE constraint on external_id keeps concurrent first requests from the same user collapsed cleanly to one row. Drops the now-unused find_by_external_id helper - the only caller was the middleware path above. Easy to re-add when an admin/read-only endpoint actually wants lookup without provisioning. Tests: bearer_with_unknown_sub_returns_401 becomes bearer_with_unknown_sub_lazy_provisions_user - asserts both the 200 status, the empty profile list (tenant-scoped to the new user, not someone else's profiles), and the COUNT(*)=1 invariant on a second request to prove idempotence. cargo check passes. Integration tests need a live Postgres so CI will validate the runtime behavior. 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)
📝 WalkthroughWalkthroughLe PR transforme l’authentification JWT en un flux "JWT-first" avec provisioning paresseux : claims.sub est résolu via une fonction find-or-provision (SELECT puis INSERT...ON CONFLICT), middleware adapté, doc mise à jour et test vérifiant l'idempotence sous concurrence. ChangesJWT Lazy User Provisioning
Sequence DiagramsequenceDiagram
participant Client
participant JWTMiddleware as JWTMiddleware
participant PostgreSQL as PostgreSQL
participant ProfilesAPI as ProfilesAPI
Client->>JWTMiddleware: GET /api/v1/profiles + Bearer JWT
JWTMiddleware->>JWTMiddleware: Validate JWT signature & extract sub
JWTMiddleware->>PostgreSQL: SELECT id FROM users WHERE external_id = sub
PostgreSQL-->>JWTMiddleware: id OR empty
alt id exists
JWTMiddleware->>ProfilesAPI: Request with user_id
else missing
JWTMiddleware->>PostgreSQL: INSERT ... ON CONFLICT (external_id) DO UPDATE ... RETURNING id
PostgreSQL-->>JWTMiddleware: user_id (provisioned)
JWTMiddleware->>ProfilesAPI: Request with user_id
end
ProfilesAPI->>PostgreSQL: Query profiles for user_id
PostgreSQL-->>ProfilesAPI: profiles (empty for new user)
ProfilesAPI-->>Client: 200 OK, profiles list
Possibly Related PRs
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~40 minutes 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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/db.rs`:
- Around line 127-129: La requête SQL actuelle qui utilise "INSERT INTO users
... ON CONFLICT ... DO UPDATE SET external_id = EXCLUDED.external_id RETURNING
id" provoque un write-amplification sur chaque authentification ; remplace ce
comportement par un INSERT ... ON CONFLICT DO NOTHING RETURNING id and fallback
SELECT id in the same database call: try the INSERT with "DO NOTHING" and if it
returns no row, SELECT id FROM users WHERE external_id = $2; update the code
path that builds/executes this SQL (the SQL string used to return the user id in
src/db.rs) to perform that two-branch logic so conflicts do not produce writes.
In `@tests/jwt_middleware.rs`:
- Around line 128-177: Update the test
bearer_with_unknown_sub_lazy_provisions_user to exercise concurrent UPSERTs:
instead of sending the two GETs sequentially, issue two GET /api/v1/profiles
requests in parallel (e.g. spawn two async tasks or use
futures::join!/tokio::join!) both using the same token created by harness.mint,
await both responses, assert both statuses are StatusCode::OK, then run the sqlx
COUNT(*) query against users (as done currently) and assert it equals 1; keep
the existing setup helpers (JwksHarness::spawn, spawn_app_with_jwt_and_shim,
harness.mint) and all existing assertions for profile contents but change the
second-request section to perform the parallel requests and checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 76030502-c460-4089-9494-bb6b033ad9eb
📒 Files selected for processing (4)
CLAUDE.mdsrc/db.rssrc/middleware.rstests/jwt_middleware.rs
CR concerns on PR #16: - src/db.rs::find_or_provision_by_external_id: switch from unconditional ON CONFLICT DO UPDATE to a read-first pattern. The hot path (every JWT request after the first) is now a SELECT only, zero writes, no heap-tuple churn. The miss path falls through to the same ON CONFLICT DO UPDATE UPSERT so two concurrent first-request races still collapse atomically to one row - the loser's UPDATE is a no-op that still fires RETURNING id. CR proposed the CTE pattern (INSERT ON CONFLICT DO NOTHING RETURNING id UNION ALL SELECT) but that has a documented race window in READ COMMITTED: both CTE branches share the statement's snapshot, so a concurrent commit between snapshot and INSERT can return zero rows. The two-query read-first pattern is race-safe via the UPSERT fallback and matches the same throughput profile (the existing-user case is one read, the new-user case is one read + one write). - tests/jwt_middleware.rs::bearer_with_unknown_sub_lazy_provisions_user: fire two GET /api/v1/profiles requests in parallel via tokio::join! with the same fresh token. Asserts both 200, both empty profile lists (tenant-scoped to the new user), and COUNT(*)=1 in users - actually exercises the race the UPSERT fallback exists to handle, rather than the previous sequential check. - cargo fmt --all noise that broke CI. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
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)
78-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDoc obsolète : ce PR change ce comportement.
Ce bloc décrit toujours «
subdoesn't resolve to ausersrow → 401 ». Orresolve_bearerprovisionne désormais l'utilisateur à la volée (200, plus 401). Comme c'est de la doc de sécurité, mieux vaut la corriger pour éviter d'induire en erreur sur le flux d'auth réel.📝 Correction suggérée
-/// - `sub` doesn't resolve to a `users` row → 401. Hides the -/// existence of foreign sub values (same no-leak rationale as -/// the resource-level 404s). +/// - `sub` has no `users` row yet → lazy-provision it (first +/// request from a fresh Better Auth signup), then attach the +/// new [`UserId`]. A provisioning failure maps to 500.🤖 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 78 - 80, La documentation dans le commentaire décrivant le comportement d'authentification est obsolète : le texte qui dit « `sub` doesn't resolve to a `users` row → 401 » doit être mis à jour pour refléter que resolve_bearer provisionne désormais l'utilisateur à la volée et retourne 200 (plutôt que 401). Ouvre le bloc de commentaire dans src/middleware.rs près de la description du flux d'auth et remplace la phrase par une formulation précisant que si le `sub` n'existe pas encore, resolve_bearer crée/provisionne un utilisateur automatiquement et poursuit l'authentification (200), en conservant la justification sécurité/no-leak adaptée; mentionne la fonction resolve_bearer pour repérer l'endroit exact.
🤖 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 78-80: La documentation dans le commentaire décrivant le
comportement d'authentification est obsolète : le texte qui dit « `sub` doesn't
resolve to a `users` row → 401 » doit être mis à jour pour refléter que
resolve_bearer provisionne désormais l'utilisateur à la volée et retourne 200
(plutôt que 401). Ouvre le bloc de commentaire dans src/middleware.rs près de la
description du flux d'auth et remplace la phrase par une formulation précisant
que si le `sub` n'existe pas encore, resolve_bearer crée/provisionne un
utilisateur automatiquement et poursuit l'authentification (200), en conservant
la justification sécurité/no-leak adaptée; mentionne la fonction resolve_bearer
pour repérer l'endroit exact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f2f3a5e9-e558-47c2-bfd8-bd485b58e71d
📒 Files selected for processing (3)
src/db.rssrc/middleware.rstests/jwt_middleware.rs
The authenticate doc still claimed 'sub doesn't resolve to a users row -> 401'. That was true through 1.d.1 but 1.c.3a inverted the behavior: a verified JWT for an unknown sub now lazy-provisions the row and returns 200. Update the flow bullet to match. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
Owner
Author
|
@coderabbitai finding valide — la bullet flow #2 dans le doc-comment d |
|
Tip For best results, initiate chat on the files or code changes.
(ﾐ^ ^ﾐ) |
3 tasks
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
Phase 1.c.3a — first half of the cross-repo bridge between
waveflow-web(Better Auth, JWT-issuer) andwaveflow-server(JWT-verifier). The web client opens PRs against this once 1.c.3b lands.Problem. The 1.d.1 JWT middleware returned 401 on
sub-unknown-in-users — forcing every fresh Better Auth signup to first hitPOST /api/v1/usersvia the devX-User-Idshim before the JWT path would accept it. That defeats the point of removing the shim in 1.d.2 and adds a second auth channel for bootstrap that has its own auth-of-the-bootstrap chicken-and-egg.Fix. A signed JWT from the configured Better Auth issuer IS the authoritative statement that the
subis a real user. The middleware now UPSERTsusers(external_id)the first time it sees a newsub, usingINSERT … ON CONFLICT DO UPDATE … RETURNING idso the resolution is one round-trip whether the row is new or pre-existing.Changes
src/db.rsusers::find_or_provision_by_external_id(pool, sub, now_ms). Dropsfind_by_external_id(now-unused).src/middleware.rs::resolve_bearerfind_by_external_id. No more 401 on unknown sub.tests/jwt_middleware.rsbearer_with_unknown_sub_returns_401→bearer_with_unknown_sub_lazy_provisions_user. Asserts 200, empty profile list (tenant-scoped), andCOUNT(*)=1after a second request (idempotence).CLAUDE.mdWhy an UPSERT instead of read-then-insert
Two reasons:
subwithin milliseconds. A naive SELECT-then-INSERT races; an UPSERT against theUNIQUE(external_id)index doesn't.DO UPDATE SET external_id = EXCLUDED.external_id RETURNING idis the idiomatic Postgres trick for "always give me the id, insert-or-existing."DO NOTHINGwould skip theRETURNINGclause on conflict and force a separateSELECT.Test plan
cargo check --workspace --all-targets— cleancargo test --test jwt_middleware— needs live Postgres, CI will validaterequire_user_id, prod-gate 503) untouched — only the JWT branch behavior changed.What this is NOT
require_user_idstill 401s on missing/malformedX-User-Id). 1.d.2 deletes the shim entirely.POST /api/v1/users— the endpoint stays as the explicit-onboarding path for admin tooling / scripts that don't want to mint a JWT first.kidmatching, or the algorithm allowlist (all unchanged from 1.d.1).Follow-up
After this lands →
waveflow-web1.c.3b ships theAuthorization: Bearerfetch wrapper + the first wired UI page. Then 1.d.2 retires the shim everywhere.Summary by CodeRabbit
New Features
Documentation
Tests