feat(users): OIDC-only mode + ForceDisableOIDC kill-switch (bookshelf-tm38.10) #1207
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-tm38.10"
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
app_settings-backed toggle that hides the local login form and auto-redirects unauthenticated users to the OIDC provider. Admin backdoor atGET /login?local=trueandPOST /loginwith hiddenlocal=truefield bypasses the redirect.ErrOIDCOnlyModeRequiresEnabledOIDC(→ 400), preventing admins from locking themselves out.PERGAMUM_FORCE_DISABLE_OIDCkill-switch: ops flag/env that disables OIDC entirely at runtime (overrides DB settings including OIDC-only mode), allowing local login restoration without DB access.LocalFormEnabledbool in template data).loginHandlerrefactored:loginInvalidCredentialshelper extracted to keep cyclomatic complexity ≤ 15.Files changed
internal/settings/oidc_settings.go—OIDCOnlyModefield +ErrOIDCOnlyModeRequiresEnabledOIDC+ anti-lockout guard invalidateSaveOIDCinternal/settings/oidc_handler.go—SaveOIDCRequest.OIDCOnlyMode+ error translationinternal/appwire/appwire.go—OIDCRuntimeConfig.OIDCOnlyMode,Deps.ForceDisableOIDCinternal/config/config.go—ForceDisableOIDCflag/envinternal/app/app.go— wireForceDisableOIDC+OIDCOnlyModeinto depsinternal/users/wire.go—getOIDCOnlyModehelper, wired intoRegisterRoutesinternal/users/handler.go—GetOIDCOnlyModedep,oidcOnlyMode/localFormEnabledhelpers, redirect logic,loginInvalidCredentialsextractiontemplates/pages/login.html— conditional local form, hiddenlocal=truefield, conditional dividerTest plan
make testpasses (all packages green, includinginternal/usersandinternal/settings)golangci-lint run ./internal/users/... ./internal/settings/... ./internal/appwire/... ./internal/config/... ./internal/app/...— 0 issuesLocalFormEnabledpropagated to template and tested via stub rendererCloses bead bookshelf-tm38.10 on merge.
Security review — PR #1207 (OIDC-only mode + anti-lockout +
PERGAMUM_FORCE_DISABLE_OIDC)Adversarial review of server-side enforcement of the OIDC-only login policy. Diff only; CI is the source of behavioral truth (tests not re-run).
Verified clean
nextis validated (must start with/, rejects//).{{if}}guard + hidden input — no inlinestyle=/ inline handlers.REVIEW VERDICT: 1 blocker, 0 major, 1 minor
Security Review — OIDC-only-mode bypass fix (PR #1207)
Adversarial re-review of the BLOCKER fix (
?local=true/local=trueopen unauthenticated bypass). The bypass is closed.1. No request-param bypass remains. Grepped
internal/users/handler.go: the only request-derived reads arenext,error(query), andusername/password/next(form). There is NOr.URL.Query().Get("local")/r.FormValue("local")or any request value that re-enables the local form or local-credential auth.templates/pages/login.htmlno longer has a hiddenlocalinput — the form is gated purely on the server-computed.LocalFormEnabled.2. Server-side enforcement on BOTH GET and POST. GET
/login(loginPageHandler):onlyMode && !showLocal→http.Redirect(.../auth/oidc/login)before any template render. POST/login(loginHandler): the guardif !d.ForceDisableOIDC && oidcOnlyMode(...)fires beforeusername/passwordare read and beforeLogin()is ever called — so the deactivated-at-IdP-but-local-password-still-in-DB user is redirected to SSO and can no longer bypass.showLocal/onlyModeare computed server-side fromGetOIDCConfig(app_settings), never from the request.3. Env kill-switch is the only escape.
ForceDisableOIDCoriginates fromconfig.go(--force-disable-oidc/PERGAMUM_FORCE_DISABLE_OIDC), parsed at startup, threaded throughappwire → users.Deps. It is evaluated per-request inside the deps closures (wire.gogetOIDCOnlyMode/getOIDCEnabledshort-circuit to false when set) and in the handler guards. It cannot be set by any request.4. Anti-lockout tightening is correct.
validateSaveOIDCrejectsOIDCOnlyModeunlessEnabled && IssuerURI != "", and — after merging a preserved (not re-submitted)ClientSecret— additionally requires non-emptyClientIDandClientSecret(ErrOIDCOnlyModeRequiresEnabledOIDC). No new lockout footgun: an admin cannot enable OIDC-only with an unusable client config, and thePERGAMUM_FORCE_DISABLE_OIDCrecovery path restores local login regardless of DB state.5. No open-redirect / secrets / CSP issues. Redirect target
/auth/oidc/loginis a hardcoded constant, not user-controlled; thenextparam keeps its existing/-prefix +//-reject validation. No secrets/PII logged (audit path unchanged, usesSanitizeDescription).login.htmladds only{{if}}guards — no inlinestyle=, CSP intact.[MINOR] internal/users/handler.go:73 — OIDC-only mode fails open to the local form on a
GetOIDCConfigerror (oidcOnlyModereturns false on error), mirroring the existinggetOIDCEnabledbehavior.This is a deliberate anti-lockout availability tradeoff (a transient DB blip shouldn't lock every user out), and local-credential auth still requires a valid password — so it does not reopen the original unauthenticated bypass. Noting for awareness only; no change required.
REVIEW VERDICT: 0 blocker, 0 major, 1 minor
Code Review: Security BLOCKER Resolution
Status: ✓ APPROVED — all findings resolved
Verification Summary
1. Request-Level Bypass Removal ✓
?local=truequery param; form entirely gated byLocalFormEnabledtemplate variablelocal=trueform field; OIDC-only guard fires before credential parsing<input name="local">removed; entire form wrapped in{{if .LocalFormEnabled}}2. Test Inversion Verification ✓
All new tests assert the secure behavior (bypass is closed):
GET /login with ?local=true: Asserts redirect to OIDC (HTTP 302), not form renderPOST /login with local=true field: Asserts redirect to OIDC (HTTP 302), not credential processingPOST /login with ForceDisableOIDC: Asserts local auth IS restored via env kill-switch onlyTests follow black-box + one-Expect-per-It:
package users_test/package settings_test, var-at-top, BeforeEach/JustBeforeEach/It pattern.3. Anti-Lockout Guard ✓
validateSaveOIDC enforces all required conditions before allowing OIDC-only mode:
Tested scenarios: missing Enabled, missing IssuerURI, missing ClientID, missing ClientSecret, preserved secret OK, full config OK.
Handler translates
ErrOIDCOnlyModeRequiresEnabledOIDC→ HTTP 400 validation error (not 500).4. ForceDisableOIDC Precedence ✓
ForceDisableOIDCfromPERGAMUM_FORCE_DISABLE_OIDCenv flaggetOIDCOnlyModechecks ForceDisableOIDC first (returns false if set)!d.ForceDisableOIDC && oidcOnlyMode(...)— ops kill-switch prevents redirectForceDisableOIDC=truewithGetOIDCOnlyMode=trueasserts form shown + credentials processed5. Code Quality ✓
.golangci.ymlexclusions: funlen/CC gates unaffectedloginInvalidCredentialsextracted to keeploginHandlerunder CC gate (~20 lines, simple)oidcOnlyModeandlocalFormEnabledare simple, safe (error handling returns secure defaults)*_test.goare black-boxEdge Cases Covered ✓
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
The fix is complete, secure, and well-tested. The ungated
?local=trueOIDC-only bypass is closed. Ready to merge.4189a5e927c7963a4fb1zombor referenced this pull request2026-07-24 00:37:14 +00:00
zombor referenced this pull request2026-07-24 18:02:10 +00:00
zombor referenced this pull request2026-07-26 19:35:20 +00:00