fix(users): OIDCEnabled missing from failed-login render struct (bookshelf-xhqy) #608
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-xhqy"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Regression from bookshelf-4op.6 (PR #605, OIDC):
POST /loginwith wrong credentials re-renderslogin.htmlvia an anonymous struct that was missingOIDCEnabled. Sincelogin.html:46references{{if .OIDCEnabled}}, template execution fails withcan't evaluate field OIDCEnabled→ 500 instead of the correct 401 + error page.Fix: add
OIDCEnabled boolandOIDCEnabled: d.OIDCEnabledto the failed-login error-render struct inloginHandler, matching theloginPageHandler(GET) pattern exactly.Tests:
testAuthRendererlogin template stub to reference.OIDCEnabled, so any future regression is caught at unit-test time"with invalid credentials"→It("renders oidc=false ...")to pin the default case"with invalid credentials and OIDCEnabled=true"context asserting 401 +oidc=truein re-rendered bodyTest plan
make buildgreenmake lintclean (users package)make coverage100% gate passesCloses bead bookshelf-xhqy on merge.
The POST /login error path (ErrInvalidCredentials) was re-rendering login.html with an anonymous struct that had no OIDCEnabled field, while the GET /login happy-path struct did include it. Since login.html:46 references {{if .OIDCEnabled}}, any failed login with OIDC enabled caused a template execute error → 500 instead of 401. Fix: add OIDCEnabled bool + OIDCEnabled: d.OIDCEnabled to the failed-login struct, matching the GET handler pattern. Tests: update testAuthRenderer to reference .OIDCEnabled in the login template stub (ensures future regressions are caught) and add two new test contexts — OIDCEnabled=false and OIDCEnabled=true — that assert the re-rendered login page carries the correct field value. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Security Review — bookshelf-xhqy (bd-bookshelf-xhqy @
4e6a8dfe)Reviewed diff only; CI green, tests not re-run.
No security findings.
Auth semantics are unchanged:
w.WriteHeader(http.StatusUnauthorized)was already on that path before the fix; the diff does not touch it. Failed login still returns 401."Invalid username or password."regardless of whether the username or password is wrong. No change introduced.OIDCEnabledis a server-side configuration boolean already exposed on the GET/loginpage. Populating it on the POST error path brings the error-render into parity with the initial page render — no new information surface.http.Redirector302is issued on bad credentials.isJSONRequestbranch (unchanged) returns{"error": "Invalid credentials"}with 401;OIDCEnabledis not included in that response (correct — it is only a template rendering concern).OIDCEnabledis non-secret. It controls only whether the OIDC button is rendered; no credential, key, or token is involved.Itblocks assert 401 status and correctoidc=rendering for bothOIDCEnabled=false(existing suite) andOIDCEnabled=true(new context), using the curried-function pattern.REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Code Review — PR #608 (bookshelf-xhqy)
Phase 0: DEMO Verification
No DEMO block required for a pure regression-fix bead; the test additions ARE the verification.
Phase 1: Spec Compliance
The bug:
login.htmluses{{.OIDCEnabled}}but the error-render struct inloginHandlerdid not carry that field → template execution panic → 500.All
login.htmlrender sites audited (grep -rn "login.html" internal/ --include="*.go"):loginPageHandler(GET)handler.go:61loginHandlerErrInvalidCredentials HTML branch (POST error)handler.go:116The two other
WriteHeader(http.StatusUnauthorized)sites inrefreshHandler(lines 161, 168) return a bare 401 with no body and do NOT callRenderer.Render— not affected.Phase 2: Code Quality
The test guard is sound.
testAuthRenderer()now includesoidc={{.OIDCEnabled}}in the login template literal, so a missingOIDCEnabledfield on the data struct would cause a template execution error and fail the test. The new OIDC=true context correctly:oidc=trueis present in the re-rendered body.One minor style note: the new
OIDCEnabled=truecontext places a fullJustBeforeEachinside a nestedContextthat already has an outerJustBeforeEach. Ginkgo runs both in order (outer first, inner second), so the outer one does a redundant server spin-up + HTTP POST before the inner one overwritesresp/body. It works correctly — theItblocks see the right values — but it is slightly wasteful. No correctness impact.defer localSrv.Close()insideJustBeforeEachis safe:bodyStr(resp)reads the response body before the defer fires, so no use-after-close.No issues in the non-test code. The fix is minimal and correct.
[MINOR]
internal/users/handler_test.go(~line 395) — newOIDCEnabled=truecontext duplicates the outerJustBeforeEachserver spin-upOuter
JustBeforeEachfires first (buildssrvwithOIDCEnabled=false, posts, writesresp/body), then inner fires (rebuilds withOIDCEnabled=true, overwrites). The test asserts correctly but wastes one round-trip. Could be refactored to use aBeforeEachthat setsOIDCEnabled=trueon aDepsvar and have the outerJustBeforeEachconsume it. Not a blocker.REVIEW VERDICT: 0 blocker, 0 major, 1 minor