fix(auth): optional user account; surface missing ADMIN_PASSWORD & JWT_SECRET as clear login errors#106
Merged
Merged
Conversation
… clearly
The login route previously required BOTH ADMIN_PASSWORD and USER_PASSWORD:
getAuthUsers() threw when either was unset. A deployment providing only admin
credentials could not log in at all, and the raw error surfaced as a misleading
"Invalid email or password" on the login screen (the form reads data.message,
but the generic error handler returned it under data.error).
- Make the lower-privilege user account optional: it exists only when
USER_PASSWORD is set. No default user password is ever assumed.
- Keep ADMIN_PASSWORD mandatory, but surface its absence as an actionable 503
("server not configured") that the login screen shows, instead of a generic
500 or a misleading 401.
- Move the local-provider credential logic (AuthUser, AuthConfigError,
getAuthUsers) out of the API route into src/lib/local-auth.ts, mirroring
src/lib/oidc.ts; the route is now a thin handler.
- Update docs (.env.example, README, DOCKERHUB, docker-compose.example,
API_DOCS) to state ADMIN_PASSWORD is required and USER_* is optional.
Tests: add tests/unit/lib/local-auth.test.ts and update the login route test to
assert the 503 config error. local-auth.ts and login/route.ts stay at 100%
line/function coverage, so SonarCloud coverage does not drop.
A correct login attempt on a server missing JWT_SECRET (or with one shorter than 32 chars) failed at token-signing time: auth.ts threw a generic Error, which the login route mapped to a generic 500, and the login form displayed the misleading "Invalid email or password" (the form reads data.message, but the generic handler returns the text under data.error). The misconfiguration only surfaced in the server console. - Promote AuthConfigError to a shared, dependency-free leaf module (src/lib/auth-errors.ts), like @/lib/db/errors, so both the shared auth layer (auth.ts) and the local provider (local-auth.ts) can throw it without a circular provider <-> shared dependency. - getJwtSecret() now throws AuthConfigError (with an actionable, display-ready message) for both the missing-in-production and too-short cases, still lazily so it does not crash module load. - The login route already translates AuthConfigError into a 503; it now forwards the error's own message, so JWT_SECRET and ADMIN_PASSWORD misconfigurations each show their specific, actionable text on the login screen. - Hoist the config-error messages to single-line module constants: bun's line coverage under-counts continuation lines of multi-line string concatenation, which would otherwise show as uncovered new code in SonarCloud. Tests: add tests/unit/lib/auth-jwt-config.test.ts (isolated process to avoid the JWT-secret memoization cache) covering the missing/too-short/dev-fallback cases, and a login route test asserting a JWT AuthConfigError becomes a 503 with its message. auth-errors.ts, local-auth.ts and login/route.ts stay at 100% coverage; auth.ts's only uncovered line is a pre-existing token-expired debug log.
cevheri
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Improves local-auth configuration handling so operator misconfiguration (missing ADMIN_PASSWORD or missing/invalid JWT_SECRET) surfaces as clear, actionable login errors, while making the lower-privilege user account optional when USER_PASSWORD is unset.
Changes:
- Introduces
AuthConfigErrorand uses it to turn missing/invalid auth config into specific 503 login responses. - Refactors local-provider credential resolution into
src/lib/local-auth.ts(admin required, user optional). - Adds/updates unit + API tests and updates docs/examples to reflect the new env-var behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/lib/local-auth.test.ts | Adds unit coverage for getAuthUsers() admin-required and optional-user behavior. |
| tests/unit/lib/auth-jwt-config.test.ts | Adds unit coverage for JWT_SECRET guard behavior in signJWT(). |
| tests/api/auth/login.test.ts | Updates login route tests to assert 503 + actionable messages for config errors and admin-only mode. |
| src/lib/local-auth.ts | New local auth helper extracting env-backed user list (admin required, user optional). |
| src/lib/auth.ts | Converts JWT secret misconfig errors to AuthConfigError with actionable messages; keeps lazy initialization. |
| src/lib/auth-errors.ts | New shared leaf error type to avoid circular deps and standardize config errors. |
| src/app/api/auth/login/route.ts | Uses getAuthUsers() and maps AuthConfigError to 503 with message for the login UI. |
| README.md | Documents required vs optional local-auth env vars and OIDC exemption. |
| docs/API_DOCS.md | Updates login endpoint notes to reflect optional USER_* behavior and required ADMIN_PASSWORD. |
| DOCKERHUB.md | Updates DockerHub usage notes for local auth env vars. |
| docker-compose.example.yml | Makes USER_PASSWORD optional in compose and documents behavior. |
| .env.example | Documents ADMIN_PASSWORD required and USER_* optional semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(data.message).toContain("ADMIN_PASSWORD"); | ||
| expect(data.message).not.toBe("Invalid email or password"); | ||
|
|
||
| process.env.ADMIN_PASSWORD = origAdminPassword!; |
| expect(data.success).toBe(true); | ||
| expect(data.role).toBe("admin"); | ||
|
|
||
| process.env.USER_PASSWORD = origUserPassword!; |
| expect(data.success).toBe(false); | ||
| expect(data.message).toBe("Invalid email or password"); | ||
|
|
||
| process.env.USER_PASSWORD = origUserPassword!; |
The login route tests restored ADMIN_PASSWORD/USER_PASSWORD via `origValue!`, which assigns the string "undefined" when the var was originally unset, leaking into later tests. Use a delete-on-undefined restore() helper, matching the pattern already used in local-auth.test.ts and auth-jwt-config.test.ts.
Contributor
Author
|
Addressed the Copilot review (all 3 comments): login route tests now restore |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tests/api/auth/login.test.ts:161
- This test mutates process.env but only restores it at the end of the test body. If an assertion throws (or the test is interrupted), the env var stays modified and can cascade failures into later tests. Wrap the mutated section in a try/finally (or move the restore into afterEach) so restoration is guaranteed.
test("returns 503 with an actionable message when ADMIN_PASSWORD is missing", async () => {
const origAdminPassword = process.env.ADMIN_PASSWORD;
delete process.env.ADMIN_PASSWORD;
const req = createMockRequest("/api/auth/login", {
Comment on lines
+205
to
+222
| test("still authenticates admin when USER_PASSWORD is not set", async () => { | ||
| const origUserPassword = process.env.USER_PASSWORD; | ||
| delete process.env.USER_PASSWORD; | ||
|
|
||
| const req = createMockRequest("/api/auth/login", { | ||
| method: "POST", | ||
| body: { email: "admin@libredb.org", password: "LibreDB.2026" }, | ||
| }); | ||
|
|
||
| const res = await POST(req as never); | ||
| const data = await parseResponseJSON<{ success: boolean; role: string }>(res); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| expect(data.success).toBe(true); | ||
| expect(data.role).toBe("admin"); | ||
|
|
||
| restore("USER_PASSWORD", origUserPassword); | ||
| }); |
Comment on lines
+224
to
+228
| test("rejects user login when USER_PASSWORD is not set (account is optional, no default)", async () => { | ||
| const origUserPassword = process.env.USER_PASSWORD; | ||
| delete process.env.USER_PASSWORD; | ||
|
|
||
| const req = createMockRequest("/api/auth/login", { |
…opilot review) Follow-up to the Copilot review: restoring env vars at the end of each test body is skipped when an earlier expect() throws, leaking state into later tests. Snapshot the mutated vars (ADMIN_PASSWORD, USER_PASSWORD) in beforeEach and restore them in afterEach (delete-on-undefined), so restoration always runs. Matches the afterEach pattern already used in local-auth.test.ts and auth-jwt-config.test.ts.
Contributor
Author
|
Addressed the second Copilot review round: the login tests now snapshot the mutated env vars ( |
|
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
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.
Problem
Two auth misconfigurations failed confusingly instead of clearly:
getAuthUsers()required bothADMIN_PASSWORDandUSER_PASSWORD, so an admin-only deployment could not log in.JWT_SECRETfailed at token-signing time after correct credentials, and both misconfigs surfaced on screen as the misleading "Invalid email or password" (the login form readsdata.message, but the generic error handler returned the text underdata.error). The real cause was only in the server console.Changes
USER_PASSWORDis set. No default user password is ever assumed.ADMIN_PASSWORD→ actionable 503 shown on the login screen ("...set the ADMIN_PASSWORD environment variable and restart..."), lazily (no boot-time guard), so embedded/OIDC modes are unaffected.JWT_SECRET→ actionable 503 on the login screen ("...set JWT_SECRET (at least 32 characters)..."), still thrown lazily.AuthConfigErrorlives in a dependency-free leaf modulesrc/lib/auth-errors.ts(like@/lib/db/errors); local-provider credential logic (AuthUser,getAuthUsers) lives insrc/lib/local-auth.ts(mirroringsrc/lib/oidc.ts). The login route is a thin handler that forwards each config error's specific message..env.example,README,DOCKERHUB,docker-compose.example.yml,docs/API_DOCS.md):ADMIN_PASSWORDrequired,USER_*optional.End-to-end verification (real Docker container, NODE_ENV=production)
Matrix over
ADMIN_PASSWORD(A),USER_PASSWORD(U),JWT_SECRET(J), unset and empty, positive and negative:All scenarios pass. Container boots healthy in every config (lazy), and OIDC / npm-embedded-in-platform modes are unaffected.
Testing & coverage
tests/unit/lib/local-auth.test.tsandtests/unit/lib/auth-jwt-config.test.ts(isolated process to sidestep the JWT-secret memoization cache); login route tests updated for the 503 config errors.auth-errors.ts,local-auth.ts,login/route.tsat 100% line/function coverage.auth.ts's only uncovered line is a pre-existing token-expired debug log, so SonarCloud new-code coverage does not drop. Config-error messages are hoisted to single-line module constants to avoid bun's multi-line-string coverage under-count.format,lint,typecheck,test— 486 pass,build).