feat(users): add external_id seed for JWT auth (Phase 1.d.1-PR1) - #11
Merged
Conversation
First sub-PR of Phase 1.d.1 — widens the users table + POST endpoint
to carry the `sub` claim that Better Auth-issued JWTs will reference.
The JWT verification machinery itself lands in PR2; wiring lands in
PR3. This PR is no-ops at runtime (the existing dev shim path keeps
working with NULL external_id) but stages the schema + handler so
the next two PRs are pure additions.
- Migration `20260530000005_users_external_id.sql`: ALTER TABLE
users ADD COLUMN external_id TEXT UNIQUE. Nullable on purpose so
the dev shim can keep minting users without an upstream account
during the 1.d transition; once Better Auth is the only auth path
(1.d.2) an ALTER COLUMN ... SET NOT NULL after a backfill closes
the slot. UNIQUE doubles as a free lookup index for the JWT
middleware's `SELECT id FROM users WHERE external_id = $1`.
- db::users::create signature gains `external_id: Option<&str>` and
binds it into the INSERT.
- POST /api/v1/users accepts an optional `CreateUserRequest { external_id }`
body; bare-bones POST with no body still works (Option<Json<T>>
defaults to empty struct). Trims + rejects blank with 400.
Postgres unique-violation (SQLSTATE 23505) surfaces as 409 so a
duplicate sub gets a distinct error from a transient 500.
- tests/profiles.rs: 4 new tests covering external_id accept path,
blank rejection, duplicate 409, explicit-null no-op. Existing
"no body" test still passes.
- tests/ready.rs: column existence canary, same belt-and-braces
pattern as the per-table canaries from 1.b.5.
OpenAPI doc updated with the new request body shape + 400 / 409
responses.
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
📝 WalkthroughWalkthroughCette PR ajoute ChangesIntégration external_id dans les users
Sequence DiagramsequenceDiagram
participant Client
participant CreateUserAPI
participant Validation
participant DB
Client->>CreateUserAPI: POST /api/v1/users { external_id }
CreateUserAPI->>Validation: trim/extract external_id
alt external_id blank après trim
Validation-->>Client: 400 Bad Request
else external_id valide ou null
CreateUserAPI->>DB: db::users::create(created_at, external_id)
alt constraint UNIQUE violation (SQLSTATE 23505)
DB-->>CreateUserAPI: SQLSTATE 23505
CreateUserAPI-->>Client: 409 Conflict
else succès
DB-->>CreateUserAPI: returning user id
CreateUserAPI-->>Client: 201 Created { id }
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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.
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 `@migrations/20260530000005_users_external_id.sql`:
- Around line 20-21: La colonne external_id ajoutée à la table users doit
interdire les valeurs vides/blanches au niveau de la BD ; modifie la migration
qui contient ALTER TABLE users ADD COLUMN external_id TEXT UNIQUE pour ajouter
une contrainte CHECK (par exemple nommée users_external_id_non_blank) qui
autorise NULL mais refuse les chaînes vides ou constituées uniquement d’espaces
(p.ex. en testant trim(external_id) <> '' ou équivalent), afin de verrouiller
l’invariant côté base et empêcher l’écriture d’external_id non exploitables.
In `@src/api/users.rs`:
- Line 57: L’annotation utoipa pour le handler create_user est incorrecte : le
handler accepte body: Option<Json<CreateUserRequest>> mais l’attribut currently
indique request_body = CreateUserRequest (corp requis). Mettez l’annotation en
accord avec le type optionnel en remplaçant request_body = CreateUserRequest par
une forme optionnelle (par ex. request_body = Option<CreateUserRequest> ou en
ajoutant required = false selon le style utilisé dans vos autres
#[utoipa::path(...)]), afin que la doc reflète correctement
Option<Json<CreateUserRequest>>; modifiez l’attribut #[utoipa::path(...
request_body = ...)] situé sur la fonction create_user et gardez le nom de type
CreateUserRequest.
🪄 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: 1c7273c1-b239-4c83-baa5-3dec24cbe105
📒 Files selected for processing (5)
migrations/20260530000005_users_external_id.sqlsrc/api/users.rssrc/db.rstests/profiles.rstests/ready.rs
- Migration: add `CHECK (external_id IS NULL OR length(trim(external_id)) > 0)` constraint named `users_external_id_non_blank`. Locks the boundary invariant at the storage layer — defense in depth on top of the handler's trim+reject. A future code path that bypasses the endpoint (manual SQL, internal job, backfill script) can't sit blank rows the JWT middleware could never match. NULL stays allowed for the dev shim path. Same pattern as `track.rating BETWEEN 0 AND 255` in 20260530000003_track.sql. Migration hasn't merged yet, so in-place edit doesn't violate the "immutable once merged" rule. - OpenAPI: wrap as `request_body = Option<CreateUserRequest>` so utoipa emits `requestBody.required: false`. The bare `request_body = CreateUserRequest` shorthand would generate `required: true` and misrepresent the contract (handler extractor is `Option<Json<CreateUserRequest>>`). utoipa-axum 0.2 doesn't accept an explicit `required = false` attribute on `request_body(...)` — only `content`, `description`, `content_type`, `example`, `examples`, `extensions` — so the type-level Option<…> wrap is the only knob for optionality. - tests/ready.rs: `users_external_id_check_rejects_blank` probe exercises a direct INSERT to prove the CHECK fires on `''` / whitespace and stays out of the way for NULL. SQLSTATE 23514 (check_violation) verified explicitly so a future regression mapping the error to a different status is caught. - tests/openapi.rs: assert the POST /api/v1/users requestBody is NOT `required: true`. Locks the contract so a future refactor dropping the `Option<…>` wrapper trips here instead of in prod. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
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 `@migrations/20260530000005_users_external_id.sql`:
- Around line 21-23: La contrainte users_external_id_non_blank laisse passer des
chaînes composées uniquement de tabulations/sauts de ligne car trim n’enlève que
les espaces ; remplace le CHECK actuel par un test qui retire tous les
caractères d’espacement via regexp_replace, par exemple: CHECK (external_id IS
NULL OR length(regexp_replace(external_id, '\s', '', 'g')) > 0), afin que values
comme "\t\n " soient considérés vides et déclenchent l'erreur attendue (SQLSTATE
23514).
In `@tests/openapi.rs`:
- Around line 66-84: Add an explicit check that the OpenAPI `paths` contains the
"/api/v1/users" path before indexing into it: ensure
`paths.get("/api/v1/users")` (or equivalent) is asserted like the existing
checks for "/health" and "/ready" so the test fails with a clear message if the
path is removed; then proceed to access `users_post` and the `requestBody` as
currently done (symbols to locate: the `paths` variable, the `users_post`
binding and the `request_body` retrieval in tests/openapi.rs).
🪄 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: e6528bff-8ac0-4430-9062-7c6759b73294
📒 Files selected for processing (4)
migrations/20260530000005_users_external_id.sqlsrc/api/users.rstests/openapi.rstests/ready.rs
CI caught a latent bug in the previous CHECK: Postgres' bare trim(…)
only strips ASCII space by default, so '\t\n ' (tab + newline +
space) survived the predicate and the INSERT slipped through.
Rust's str::trim() strips every Unicode whitespace char by default
— the handler-side rejection still worked, but the storage canary
the constraint was supposed to be didn't fire.
Migration (still on this PR's branch — in-place edit OK per the
"immutable once merged" rule):
- Swap `length(trim(external_id)) > 0` for
`length(regexp_replace(external_id, '\s', '', 'g')) > 0`. The
'\s' character class strips space / tab / newline / CR / form-feed /
vertical-tab, matching Rust's str::trim() definition so the
storage CHECK and the handler stay in lockstep.
- NULL passing path is unchanged (explicit `IS NULL` branch).
tests/openapi.rs:
- Explicit `paths.contains_key("/api/v1/users")` assertion before
indexing, matching the existing /health and /ready style. A
future refactor that drops the path now fails with a clear
"missing /api/v1/users in spec" message instead of an opaque
panic on the index lookup.
users_external_id_check_rejects_blank now passes for all three
inputs (`""`, `" "`, `"\t\n "`) — verified locally:
- cargo check --all-targets
- cargo clippy --all-targets -- -D warnings
- cargo fmt --all --check
Signed-off-by: InstaZDLL <github.105mh@8shield.net>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@migrations/20260530000005_users_external_id.sql`:
- Around line 23-26: The CHECK constraint using regexp_replace(external_id,
'\s', '', 'g') will work under default standard_conforming_strings, but to make
the escape explicit and avoid dependence on server string settings update the
CHECK to use a double-escaped backslash in the regex
(regexp_replace(external_id, '\\s', '', 'g')) so the regex engine reliably
receives \s; modify the CHECK around external_id accordingly.
🪄 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: e75ee327-29ec-415e-88ab-d93cb5218b25
📒 Files selected for processing (2)
migrations/20260530000005_users_external_id.sqltests/openapi.rs
4 tasks
2 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
First of 3 sub-PRs landing Phase 1.d.1 — JWT auth verification machinery. This PR is the staging step: schema + handler accept an
external_id(the upstream auth provider'ssubclaim), but the JWT middleware that actually consumes it lands in PR2/PR3. The existing devX-User-Idshim path keeps working untouched.Why
Better Auth (or any other JWKS-issuing auth provider) hands the server a verified JWT whose
subclaim is a stable per-user identifier. The server needs a column onusersto resolve thatsubback to our internalusers.idBIGINT — choosingBIGINT id+TEXT external_idover migrating every FK to TEXT keeps the existing CRUD chain intact. The per-request indirection is a single indexed lookup (the UNIQUE constraint doubles as the index).Changes
20260530000005_users_external_id.sql—ALTER TABLE users ADD COLUMN external_id TEXT UNIQUE. Nullable on purpose so the dev shim keeps minting users without an upstream account during the 1.d transition; 1.d.2 willALTER COLUMN … SET NOT NULLafter a backfill once Better Auth is the only auth path.db::users::create— signature widened to(pool, created_at, external_id: Option<&str>).POST /api/v1/users— acceptsCreateUserRequest { external_id: Option<String> }. Bare-bones POST with no body still works (Option<Json<T>>defaults to empty struct). Trims + rejects blank with 400. Postgres unique-violation (SQLSTATE23505) surfaces as 409 so a duplicatesubgets a distinct error from a transient 500.tests/profiles.rs— 4 new tests:create_user_accepts_external_id— happy pathcreate_user_rejects_blank_external_id— 400 on""," ","\t\n "create_user_rejects_duplicate_external_id— 2nd POST with same sub → 409create_user_accepts_explicit_null_external_id— locks in thenull = omittedcontracttests/ready.rs—users.external_idcolumn canary.OpenAPI annotations updated with
request_body = CreateUserRequest+ 400 / 409 responses.What this is NOT
Test plan
cargo check --all-targetscargo fmt --all --checkcargo clippy --all-targets -- -D warningscargo test --all(CI runs this against a Postgres service container)Refs: RFC-001 §6.6, follows the boundary-validation + 4xx-discrimination patterns from Phase 1.b.5.
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests