feat(oidc): configurable per-login session duration override (bookshelf-tm38.11) #1395

Merged

zombor merged 2 commits from bd-bookshelf-tm38.11 into main 2026-08-08 15:53:41 +00:00

Conversation 4 Commits 2 Files changed 18 +378 -24

zombor commented 2026-08-08 02:22:55 +00:00

Owner

Copy link

Summary

• Adds an admin-configurable OIDC_SESSION_DURATION_HOURS app_settings value (mirrors Grimmory's setting of the same name) that overrides the refresh-token (session) lifetime for OIDC/SSO logins only.
• When unset (0), OIDC logins fall back to the global --refresh-token-ttl default, same as today. Local username/password logins are always unaffected.
• Value is clamped to [1 hour, 1 year] on save; persisted inside the existing OIDC_PROVIDER_DETAILS app_settings JSON blob — no new Grimmory-table column.
• New "Session duration (hours)" field under Settings → OIDC → Session Settings, wired through oidc_settings_controller.js (reuses canonical .provider-settings-key-row/.provider-settings-key-input classes, no inline style=).
• Docs updated: docs/content/docs/administering/oidc.md.

Test plan

• internal/settings: SaveOIDCSettings/GetOIDCConfig round-trip tests — override applied when set, zero (default) when unset, clamped at the 1-year ceiling; handler pass-through test.
• internal/users: OIDCHandleCallback full-flow test asserts the refresh-token expiry reflects the override when configured, and the global default when not; access-token expiry is unaffected by the override.
• static/js/test/oidc_settings_controller.test.js: new SESSION DURATION describe block (parsed int, blank→0, non-numeric→0) + no-target false-arm coverage.
• e2e/browser/journey_settings_test.go (existing Ordered OIDC journey, no new Describe): extended to assert the new field renders, round-trips through save+reload, and is captured in the existing OIDC screenshots.
• go build ./..., make lint, make test, make coverage (100% on internal/), npm run coverage (100% on JS) all green locally.

Closes bead bookshelf-tm38.11 on merge.

## Summary - Adds an admin-configurable `OIDC_SESSION_DURATION_HOURS` app_settings value (mirrors Grimmory's setting of the same name) that overrides the refresh-token (session) lifetime for OIDC/SSO logins only. - When unset (0), OIDC logins fall back to the global `--refresh-token-ttl` default, same as today. Local username/password logins are always unaffected. - Value is clamped to [1 hour, 1 year] on save; persisted inside the existing `OIDC_PROVIDER_DETAILS` app_settings JSON blob — no new Grimmory-table column. - New "Session duration (hours)" field under Settings → OIDC → Session Settings, wired through `oidc_settings_controller.js` (reuses canonical `.provider-settings-key-row`/`.provider-settings-key-input` classes, no inline `style=`). - Docs updated: `docs/content/docs/administering/oidc.md`. ## Test plan - [x] `internal/settings`: SaveOIDCSettings/GetOIDCConfig round-trip tests — override applied when set, zero (default) when unset, clamped at the 1-year ceiling; handler pass-through test. - [x] `internal/users`: OIDCHandleCallback full-flow test asserts the refresh-token expiry reflects the override when configured, and the global default when not; access-token expiry is unaffected by the override. - [x] `static/js/test/oidc_settings_controller.test.js`: new SESSION DURATION describe block (parsed int, blank→0, non-numeric→0) + no-target false-arm coverage. - [x] `e2e/browser/journey_settings_test.go` (existing Ordered OIDC journey, no new Describe): extended to assert the new field renders, round-trips through save+reload, and is captured in the existing OIDC screenshots. - [x] `go build ./...`, `make lint`, `make test`, `make coverage` (100% on `internal/`), `npm run coverage` (100% on JS) all green locally. Closes bead bookshelf-tm38.11 on merge.

zombor added 1 commit 2026-08-08 02:22:56 +00:00

feat(oidc): configurable per-login session duration override (bookshelf-tm38.11) ... 5a27fcede2

Add an OIDC_SESSION_DURATION_HOURS app_settings value (mirrors Grimmory's
setting of the same name) that overrides the refresh-token (session)
lifetime for OIDC logins only, in place of the fixed global
--refresh-token-ttl default. Local username/password logins are unaffected.

- settings.OIDCProviderDetails/OIDCSettingsPage/OIDCSettingsUpdate/
  OIDCRuntimeConfig gain SessionDurationHours/SessionDuration; the value is
  clamped to [1 hour, 1 year] on save and stored in the existing
  OIDC_PROVIDER_DETAILS app_settings JSON blob (no new Grimmory column).
- users.OIDCConfig carries the resolved SessionDuration; OIDCHandleCallback
  applies it in place of the global refresh-token TTL when > 0.
- New "Session duration (hours)" field on Settings → OIDC → Session
  Settings, wired through oidc_settings_controller.js.
- docs/content/docs/administering/oidc.md documents the new setting.

Closes bead bookshelf-tm38.11 on merge.

zombor commented 2026-08-08 02:31:47 +00:00

Author

Owner

Copy link

Security review of #1395 (bookshelf-tm38.11) — OIDC session/token TTL configurable.

Scope reviewed: internal/settings/oidc_settings.go, oidc_handler.go, internal/users/oidc_service.go, internal/users/wire.go, internal/appwire/appwire.go, templates/pages/settings_shell.html, static/js/controllers/oidc_settings_controller.js, docs/content/docs/administering/oidc.md, e2e/browser/journey_settings_test.go.

Checks performed:

• AuthZ: PUT /settings/oidc (routes.go:62) is wrapped in adminRequired(...), unchanged by this PR — the new session_duration_hours field rides the existing admin-gated route. No new unauthenticated/non-admin surface.
• Clamp enforcement: clampOIDCSessionDurationHours (oidc_settings.go) is applied both at save time (validateSaveOIDC) AND again at read time in GetOIDCConfig before converting to time.Duration — defense in depth, so a value written before the clamp existed (or via direct DB edit) is still re-clamped on every issuance read, not just on save. Negative/zero → treated as "unset" (0, falls back to global default), not clamped up to the 1h floor — verified by test "clamps a below-minimum value up to the minimum" (which actually asserts negative → zero/unset, correctly documented). Upper bound clamps to 24*365 hours. No overflow risk (int hours converted to time.Duration via multiplication by time.Hour; max value 8760 is far below overflow range).
• Revocation/session-lifetime interaction: the override only changes SessionDuration/effectiveRefreshTTL, which feeds the refresh-token row TTL (createRefreshToken/CreateRefreshTokenParams.ExpiryDate) — a DB-backed, revocable token. The access-token TTL (DefaultAccessTokenTTL = 15 min, JWT) is untouched by this change (confirmed in oidc_service.go: accessTTL passed through unchanged; test "does not affect the access-token expiry" pins this). So a disabled/logged-out user's exposure window after revocation stays bounded by the 15-minute access-token TTL regardless of how long the admin sets the session/refresh-token duration — extending session length does not meaningfully undermine revocation.
• Scoping: SessionDurationHours lives in the global OIDC_PROVIDER_DETAILS app_settings JSON blob (admin-only settings surface), not derived from any request/session value at issuance time (GetOIDCConfig reads only from stored app_settings). Local (non-OIDC) logins are explicitly unaffected (Login() always uses the global TTL directly per the code comment).
• Logging: no secrets or PII newly logged; the existing oidc settings save started/completed log lines (enabled/provider_name/issuer_uri) are unchanged.
• CSP: no inline style= introduced in the template diff.

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Security review of #1395 (bookshelf-tm38.11) — OIDC session/token TTL configurable. Scope reviewed: internal/settings/oidc_settings.go, oidc_handler.go, internal/users/oidc_service.go, internal/users/wire.go, internal/appwire/appwire.go, templates/pages/settings_shell.html, static/js/controllers/oidc_settings_controller.js, docs/content/docs/administering/oidc.md, e2e/browser/journey_settings_test.go. Checks performed: - AuthZ: `PUT /settings/oidc` (routes.go:62) is wrapped in `adminRequired(...)`, unchanged by this PR — the new `session_duration_hours` field rides the existing admin-gated route. No new unauthenticated/non-admin surface. - Clamp enforcement: `clampOIDCSessionDurationHours` (oidc_settings.go) is applied both at save time (`validateSaveOIDC`) AND again at read time in `GetOIDCConfig` before converting to `time.Duration` — defense in depth, so a value written before the clamp existed (or via direct DB edit) is still re-clamped on every issuance read, not just on save. Negative/zero → treated as "unset" (0, falls back to global default), not clamped up to the 1h floor — verified by test `"clamps a below-minimum value up to the minimum"` (which actually asserts negative → zero/unset, correctly documented). Upper bound clamps to `24*365` hours. No overflow risk (`int` hours converted to `time.Duration` via multiplication by `time.Hour`; max value 8760 is far below overflow range). - Revocation/session-lifetime interaction: the override only changes `SessionDuration`/`effectiveRefreshTTL`, which feeds the refresh-token row TTL (`createRefreshToken`/`CreateRefreshTokenParams.ExpiryDate`) — a DB-backed, revocable token. The access-token TTL (`DefaultAccessTokenTTL` = 15 min, JWT) is untouched by this change (confirmed in oidc_service.go: `accessTTL` passed through unchanged; test `"does not affect the access-token expiry"` pins this). So a disabled/logged-out user's exposure window after revocation stays bounded by the 15-minute access-token TTL regardless of how long the admin sets the session/refresh-token duration — extending session length does not meaningfully undermine revocation. - Scoping: `SessionDurationHours` lives in the global `OIDC_PROVIDER_DETAILS` app_settings JSON blob (admin-only settings surface), not derived from any request/session value at issuance time (`GetOIDCConfig` reads only from stored app_settings). Local (non-OIDC) logins are explicitly unaffected (`Login()` always uses the global TTL directly per the code comment). - Logging: no secrets or PII newly logged; the existing `oidc settings save started/completed` log lines (enabled/provider_name/issuer_uri) are unchanged. - CSP: no inline `style=` introduced in the template diff. No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-08 02:33:41 +00:00

Author

Owner

Copy link

Code Review — bookshelf-tm38.11 (configurable OIDC session duration)

[BLOCKER] internal/app/app.go:359-372 — SessionDuration override is never wired into production, feature is dead code
This PR adds SessionDuration time.Duration to appwire.OIDCRuntimeConfig (internal/appwire/appwire.go:854-856) and correctly threads it through internal/settings/oidc_settings.go's GetOIDCConfig and internal/users/wire.go:123 (SessionDuration: rc.SessionDuration). But the one production call site that constructs appwire.OIDCRuntimeConfig — the GetOIDCConfig closure in internal/app/app.go (lines 354-373) — builds the struct with an explicit field list (Enabled, Issuer, ClientID, ... OIDCOnlyMode) and does not copy rc.SessionDuration from settings.OIDCRuntimeConfig into it. Since this diff does not touch internal/app/app.go at all, the field silently defaults to zero at runtime, so effectiveRefreshTTL (internal/users/oidc_service.go:452) always falls back to the global TTL — an admin who sets a session duration override in the settings UI has it saved and re-displayed (round-trips through the DB/UI, per the new e2e journey step), but it never actually changes any issued OIDC session's expiry.
This is uncaught because every test that exercises the new behavior stubs GetOIDCConfig/cfg.SessionDuration directly (internal/settings/oidc_settings_test.go, internal/users/oidc_coverage_test.go) and the new browser e2e journey only asserts the form value persists (e2e/browser/journey_settings_test.go:417-419) — none of them go through the real app.New wiring closure in app.go that is the only place this actually gets consumed.
Fix: add SessionDuration: rc.SessionDuration, to the struct literal in internal/app/app.go's GetOIDCConfig closure (~line 372, alongside OIDCOnlyMode). Also worth flagging (pre-existing, out of scope for this bead but same shape of bug): DefaultPermissions / DefaultLibraryIDs are also missing from that same struct literal — a prior feature (JIT auto-provision defaults) may have the identical wiring gap; suggest filing a follow-up bead to audit/complete that mapping and consider adding a genuine end-to-end (real app.New) e2e/integration assertion that an OIDC login actually respects a configured session-duration override, so this class of gap can't recur silently.

[MAJOR] internal/users/oidc_coverage_test.go:2338-2361 — new wall-clock (time.Now()) assertions in the test's It bodies
Three new It blocks assert capturedRefreshTTL.ExpiryDate / result.AccessTokenExpiry against time.Now().Add(...) computed at assertion time (not injected/frozen), with a 5-second BeTemporally("~", ..., 5*time.Second) tolerance. Per .claude/rules/review-standard.md → "Flake prevention" this is exactly the banned pattern: asserting on real elapsed time between the action (JustBeforeEach invoking handleCallback, which internally computes now.Add(refreshTTL)) and the assertion. 5s is generous for a fast unit test, but under CI load/parallel test contention it is not impossible to exceed, and the fix is cheap. The file already has access to logger/config plumbed by closures — prefer injecting a fixed now (mirroring the pattern already used elsewhere in this codebase, e.g. internal/users/device_store_test.go:48 fakeNow, and internal/users/service_test.go:257 fixedNow) rather than a wall-clock tolerance window.
Fix: thread a fixed now time.Time into whatever computes ExpiryDate/AccessTokenExpiry in this flow (or, if that's out of reach without touching production code in this bead, at minimum capture time.Now() once before JustBeforeEach runs and assert against that captured value with a tight tolerance, removing the skew between per-It time.Now() calls and the actual event).

[MINOR] templates/pages/settings_shell.html:1361 — session-duration input has min="1" but no max attribute
The clamp is enforced server-side (oidcSessionDurationMaxHours = 8760h/1yr) so this is not a correctness issue, but adding max="8760" to the <input type="number"> would give the admin instant client-side feedback consistent with the min="1" already present, rather than only discovering the clamp after a round-trip save+reload.

REVIEW VERDICT: 1 blocker, 1 major, 1 minor

## Code Review — bookshelf-tm38.11 (configurable OIDC session duration) [BLOCKER] internal/app/app.go:359-372 — SessionDuration override is never wired into production, feature is dead code This PR adds `SessionDuration time.Duration` to `appwire.OIDCRuntimeConfig` (internal/appwire/appwire.go:854-856) and correctly threads it through `internal/settings/oidc_settings.go`'s `GetOIDCConfig` and `internal/users/wire.go:123` (`SessionDuration: rc.SessionDuration`). But the **one production call site** that constructs `appwire.OIDCRuntimeConfig` — the `GetOIDCConfig` closure in `internal/app/app.go` (lines 354-373) — builds the struct with an explicit field list (`Enabled`, `Issuer`, `ClientID`, ... `OIDCOnlyMode`) and does **not** copy `rc.SessionDuration` from `settings.OIDCRuntimeConfig` into it. Since this diff does not touch `internal/app/app.go` at all, the field silently defaults to zero at runtime, so `effectiveRefreshTTL` (internal/users/oidc_service.go:452) always falls back to the global TTL — an admin who sets a session duration override in the settings UI has it saved and re-displayed (round-trips through the DB/UI, per the new e2e journey step), but it **never actually changes any issued OIDC session's expiry**. This is uncaught because every test that exercises the new behavior stubs `GetOIDCConfig`/`cfg.SessionDuration` directly (internal/settings/oidc_settings_test.go, internal/users/oidc_coverage_test.go) and the new browser e2e journey only asserts the form value persists (e2e/browser/journey_settings_test.go:417-419) — none of them go through the real `app.New` wiring closure in app.go that is the only place this actually gets consumed. Fix: add `SessionDuration: rc.SessionDuration,` to the struct literal in `internal/app/app.go`'s `GetOIDCConfig` closure (~line 372, alongside `OIDCOnlyMode`). Also worth flagging (pre-existing, out of scope for this bead but same shape of bug): `DefaultPermissions` / `DefaultLibraryIDs` are also missing from that same struct literal — a prior feature (JIT auto-provision defaults) may have the identical wiring gap; suggest filing a follow-up bead to audit/complete that mapping and consider adding a genuine end-to-end (real `app.New`) e2e/integration assertion that an OIDC login actually respects a configured session-duration override, so this class of gap can't recur silently. [MAJOR] internal/users/oidc_coverage_test.go:2338-2361 — new wall-clock (`time.Now()`) assertions in the test's `It` bodies Three new `It` blocks assert `capturedRefreshTTL.ExpiryDate` / `result.AccessTokenExpiry` against `time.Now().Add(...)` computed *at assertion time* (not injected/frozen), with a 5-second `BeTemporally("~", ..., 5*time.Second)` tolerance. Per `.claude/rules/review-standard.md` → "Flake prevention" this is exactly the banned pattern: asserting on real elapsed time between the action (`JustBeforeEach` invoking `handleCallback`, which internally computes `now.Add(refreshTTL)`) and the assertion. 5s is generous for a fast unit test, but under CI load/parallel test contention it is not impossible to exceed, and the fix is cheap. The file already has access to `logger`/config plumbed by closures — prefer injecting a fixed `now` (mirroring the pattern already used elsewhere in this codebase, e.g. `internal/users/device_store_test.go:48` `fakeNow`, and `internal/users/service_test.go:257` `fixedNow`) rather than a wall-clock tolerance window. Fix: thread a fixed `now time.Time` into whatever computes `ExpiryDate`/`AccessTokenExpiry` in this flow (or, if that's out of reach without touching production code in this bead, at minimum capture `time.Now()` once *before* `JustBeforeEach` runs and assert against that captured value with a tight tolerance, removing the skew between per-`It` `time.Now()` calls and the actual event). [MINOR] templates/pages/settings_shell.html:1361 — session-duration input has `min="1"` but no `max` attribute The clamp is enforced server-side (`oidcSessionDurationMaxHours` = 8760h/1yr) so this is not a correctness issue, but adding `max="8760"` to the `<input type="number">` would give the admin instant client-side feedback consistent with the `min="1"` already present, rather than only discovering the clamp after a round-trip save+reload. REVIEW VERDICT: 1 blocker, 1 major, 1 minor

zombor added 1 commit 2026-08-08 02:44:42 +00:00

fix(review): wire SessionDuration into app.go, replace wall-clock asserts, add max hint ... 5cfd066ec6

- BLOCKER: internal/app/app.go's GetOIDCConfig closure was constructed with an
  explicit field list that dropped rc.SessionDuration, so the admin-configured
  OIDC session-duration override never reached production token issuance
  (round-tripped in the DB but changed nothing at runtime). Wire it through.
- Add a regression e2e journey step (Journey-9) that boots the real app via
  app.New, sets a 2h session-duration override via PUT /settings/oidc, and
  asserts the issued refresh_token.expiry_date reflects it rather than the
  720h global default. Fails against the unfixed app.go.
- MAJOR: thread an injected `now func() time.Time` clock through
  oidcIssueTokens/oidcFinalizeLogin/OIDCHandleCallback (mirroring the existing
  DeviceStore clock-injection pattern) so internal/users/oidc_coverage_test.go
  can assert exact refresh/access-token expiry timestamps against a frozen
  clock instead of a 5s wall-clock tolerance window.
- MINOR: add max="8760" (1 year) to the OIDC session-duration input in
  settings_shell.html to match the server-side clamp.

zombor commented 2026-08-08 02:53:27 +00:00

Author

Owner

Copy link

Security re-review of PR #1395 (bookshelf-tm38.11) — now that SessionDuration is actually wired into internal/app/app.go's GetOIDCConfig closure and the clock is injected through token issuance.

Re-verified the posture that was previously reviewed only in the abstract, now that it is live at runtime:

• Clamp enforced on the live path. internal/settings/oidc_settings.go clampOIDCSessionDurationHours (≤0 → 0/"unset", >8760 → clamped to 8760) is applied both at save time (validateSaveOIDC) and again defensively at read time (GetOIDCConfig), so OIDCRuntimeConfig.SessionDuration reaching appwire.OIDCRuntimeConfig / users.OIDCConfig via internal/app/app.go:374 and internal/users/wire.go:126 is always already in [0, 8760h] — no unbounded/absurd TTL is reachable through the newly-live wiring.
• Scope confirmed: refresh token only. internal/users/oidc_service.go effectiveRefreshTTL(cfg.SessionDuration, refreshTTL) is applied only to the refreshTTL argument passed into oidcFinalizeLogin/oidcIssueTokens (oidc_service.go:450); accessTTL is threaded through unchanged from wire.go's fixed d.AccessTokenTTL (15 min) with no cfg.SessionDuration involvement anywhere in that path. Verified by the new table test at oidc_coverage_test.go ("does not affect the access-token expiry"). A long session override still cannot defeat logout/disable/revocation, since the short-lived access JWT still expires on its fixed schedule and the revocable refresh token is the only thing lengthened.
• Clock injection is safe. now func() time.Time is threaded from internal/users/wire.go:215 as plain time.Now in production — not derived from any request header/claim/DB value. Only test callers substitute a fixed clock. No request can influence issued-token expiry via this seam.
• AuthZ unchanged. PUT /settings/oidc remains behind adminRequired (internal/settings/routes.go:88); the override value is never request-derived at issuance — it flows from app_settings (admin-controlled) through GetOIDCConfig, never from the login request itself.
• No new logging of secrets/PII — the diff logs no token/claim values; SessionDurationHours (a plain integer duration setting, not a secret) is the only new field surfaced, and only via existing settings JSON, not logs.

No regressions found from making the override live. The BLOCKER from the prior review (dead wiring) is resolved without reintroducing any bound-bypass or revocation-defeat.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Security re-review of PR #1395 (bookshelf-tm38.11) — now that `SessionDuration` is actually wired into `internal/app/app.go`'s `GetOIDCConfig` closure and the clock is injected through token issuance. Re-verified the posture that was previously reviewed only in the abstract, now that it is live at runtime: - **Clamp enforced on the live path.** `internal/settings/oidc_settings.go` `clampOIDCSessionDurationHours` (≤0 → 0/"unset", >8760 → clamped to 8760) is applied both at save time (`validateSaveOIDC`) and again defensively at read time (`GetOIDCConfig`), so `OIDCRuntimeConfig.SessionDuration` reaching `appwire.OIDCRuntimeConfig` / `users.OIDCConfig` via `internal/app/app.go:374` and `internal/users/wire.go:126` is always already in `[0, 8760h]` — no unbounded/absurd TTL is reachable through the newly-live wiring. - **Scope confirmed: refresh token only.** `internal/users/oidc_service.go` `effectiveRefreshTTL(cfg.SessionDuration, refreshTTL)` is applied only to the `refreshTTL` argument passed into `oidcFinalizeLogin`/`oidcIssueTokens` (`oidc_service.go:450`); `accessTTL` is threaded through unchanged from `wire.go`'s fixed `d.AccessTokenTTL` (15 min) with no `cfg.SessionDuration` involvement anywhere in that path. Verified by the new table test at `oidc_coverage_test.go` ("does not affect the access-token expiry"). A long session override still cannot defeat logout/disable/revocation, since the short-lived access JWT still expires on its fixed schedule and the revocable refresh token is the only thing lengthened. - **Clock injection is safe.** `now func() time.Time` is threaded from `internal/users/wire.go:215` as plain `time.Now` in production — not derived from any request header/claim/DB value. Only test callers substitute a fixed clock. No request can influence issued-token expiry via this seam. - **AuthZ unchanged.** `PUT /settings/oidc` remains behind `adminRequired` (`internal/settings/routes.go:88`); the override value is never request-derived at issuance — it flows from `app_settings` (admin-controlled) through `GetOIDCConfig`, never from the login request itself. - **No new logging of secrets/PII** — the diff logs no token/claim values; `SessionDurationHours` (a plain integer duration setting, not a secret) is the only new field surfaced, and only via existing settings JSON, not logs. No regressions found from making the override live. The BLOCKER from the prior review (dead wiring) is resolved without reintroducing any bound-bypass or revocation-defeat. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-08 02:57:19 +00:00

Author

Owner

Copy link

Code Re-Review of PR #1395 (bookshelf-tm38.11) — verifying BLOCKER+MAJOR fix

BLOCKER re-check (SessionDuration dead-wiring): RESOLVED.
internal/app/app.go (real app.New closure, GetOIDCConfig) now sets SessionDuration: rc.SessionDuration and threads it all the way to users.Wire -> OIDCHandleCallback -> oidcFinalizeLogin -> oidcIssueTokens, confirmed on origin/bd-bookshelf-tm38.11 (not the stale local checkout). The new e2e/api/journey_9_oidc_login_test.go "applies the configured OIDC session duration..." spec boots the real server via app.New (e2e/testutil/server.go:818), PUTs a live session_duration_hours=2 through /settings/oidc, drives a full OIDC login against the fake IdP, and reads refresh_token.expiry_date straight from MySQL, asserting ttl between 90-150 min. This is a genuine regression guard: it exercises the real struct literal in app.go, not a stub, and would fail against the unfixed code (which would produce ~720h). Good catch-proof test.

MAJOR re-check (wall-clock assertions): RESOLVED. oidcIssueTokens/OIDCHandleCallback/oidcFinalizeLogin now take an injected now func() time.Time (internal/users/oidc_service.go lines ~341, 413, 483), wired from production via time.Now in internal/users/wire.go:215 (confirmed correct positional order against the OIDCHandleCallback signature: secret, accessTTL, refreshTTL, now, logger). All 18 call-site updates (17 test stubs + 1 production wiring) are present and consistently ordered, verified by diffing every +time.Now, insertion against every OIDCHandleCallback( call site in oidc_service_test.go, oidc_coverage_test.go, oidc_userinfo_test.go, oidc_cache_test.go. The "OIDCHandleCallback full flow" describe block now freezes fixedNow and asserts capturedRefreshTTL.ExpiryDate with exact Equal(fixedNow.Add(...)), no tolerance window, fully deterministic. A sibling It also confirms the access-token expiry is unaffected by the session-duration override (result.AccessTokenExpiry still fixedNow.Add(DefaultAccessTokenTTL)), so the clock threading did not regress access-token TTL behavior.

MINOR re-check (missing max attr): RESOLVED. templates/pages/settings_shell.html now has max="8760" on the session-duration input, matching oidcSessionDurationMaxHours = 24*365 in internal/settings/oidc_settings.go.

Conventions: all touched test files are black-box (package settings_test / package users_test); e2e journey uses the allowed multi-Expect-per-It relaxation; no .golangci.yml or scripts/check-coverage.sh changes; docs updated in the same PR (docs/content/docs/administering/oidc.md).

New finding (not present in the original review, spotted in the fix's new test)

[MINOR] internal/settings/oidc_settings_test.go:1303 - misleading It title for negative-value clamp test
The test is titled It("clamps a below-minimum value up to the minimum", ...) but the body's own comment and assertion say the opposite: a negative value (-5) is treated as "unset" and returns 0, it is NOT clamped to a 1-hour minimum. There is in fact no minimum-clamp code path (clampOIDCSessionDurationHours only clamps hours <= 0 to 0 and hours > max to max), so the title describes behavior the code doesn't implement. Fix: rename to something like It("treats a negative value as unset (0), not clamped to a minimum", ...) so a future reader isn't misled into thinking a minimum-clamp exists.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Re-Review of PR #1395 (bookshelf-tm38.11) — verifying BLOCKER+MAJOR fix **BLOCKER re-check (SessionDuration dead-wiring):** RESOLVED. `internal/app/app.go` (real `app.New` closure, `GetOIDCConfig`) now sets `SessionDuration: rc.SessionDuration` and threads it all the way to `users.Wire` -> `OIDCHandleCallback` -> `oidcFinalizeLogin` -> `oidcIssueTokens`, confirmed on `origin/bd-bookshelf-tm38.11` (not the stale local checkout). The new `e2e/api/journey_9_oidc_login_test.go` "applies the configured OIDC session duration..." spec boots the real server via `app.New` (`e2e/testutil/server.go:818`), PUTs a live `session_duration_hours=2` through `/settings/oidc`, drives a full OIDC login against the fake IdP, and reads `refresh_token.expiry_date` straight from MySQL, asserting `ttl` between 90-150 min. This is a genuine regression guard: it exercises the real struct literal in `app.go`, not a stub, and would fail against the unfixed code (which would produce ~720h). Good catch-proof test. **MAJOR re-check (wall-clock assertions):** RESOLVED. `oidcIssueTokens`/`OIDCHandleCallback`/`oidcFinalizeLogin` now take an injected `now func() time.Time` (`internal/users/oidc_service.go` lines ~341, 413, 483), wired from production via `time.Now` in `internal/users/wire.go:215` (confirmed correct positional order against the `OIDCHandleCallback` signature: `secret, accessTTL, refreshTTL, now, logger`). All 18 call-site updates (17 test stubs + 1 production wiring) are present and consistently ordered, verified by diffing every `+time.Now,` insertion against every `OIDCHandleCallback(` call site in `oidc_service_test.go`, `oidc_coverage_test.go`, `oidc_userinfo_test.go`, `oidc_cache_test.go`. The "OIDCHandleCallback full flow" describe block now freezes `fixedNow` and asserts `capturedRefreshTTL.ExpiryDate` with exact `Equal(fixedNow.Add(...))`, no tolerance window, fully deterministic. A sibling `It` also confirms the access-token expiry is unaffected by the session-duration override (`result.AccessTokenExpiry` still `fixedNow.Add(DefaultAccessTokenTTL)`), so the clock threading did not regress access-token TTL behavior. **MINOR re-check (missing max attr):** RESOLVED. `templates/pages/settings_shell.html` now has `max="8760"` on the session-duration input, matching `oidcSessionDurationMaxHours = 24*365` in `internal/settings/oidc_settings.go`. **Conventions:** all touched test files are black-box (`package settings_test` / `package users_test`); e2e journey uses the allowed multi-Expect-per-It relaxation; no `.golangci.yml` or `scripts/check-coverage.sh` changes; docs updated in the same PR (`docs/content/docs/administering/oidc.md`). ### New finding (not present in the original review, spotted in the fix's new test) [MINOR] internal/settings/oidc_settings_test.go:1303 - misleading It title for negative-value clamp test The test is titled `It("clamps a below-minimum value up to the minimum", ...)` but the body's own comment and assertion say the opposite: a negative value (-5) is treated as "unset" and returns 0, it is NOT clamped to a 1-hour minimum. There is in fact no minimum-clamp code path (`clampOIDCSessionDurationHours` only clamps `hours <= 0` to 0 and `hours > max` to max), so the title describes behavior the code doesn't implement. Fix: rename to something like `It("treats a negative value as unset (0), not clamped to a minimum", ...)` so a future reader isn't misled into thinking a minimum-clamp exists. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor force-pushed bd-bookshelf-tm38.11 from 5cfd066ec6 to 50dc8f9605 2026-08-08 15:45:02 +00:00 Compare

zombor merged commit 4fefc21eef into main 2026-08-08 15:53:41 +00:00

zombor referenced this pull request from a commit 2026-08-08 15:53:45 +00:00

Merge pull request 'feat(oidc): configurable per-login session duration override (bookshelf-tm38.11)' (#1395) from bd-bookshelf-tm38.11 into main

zombor referenced this pull request from a commit 2026-08-09 01:58:53 +00:00

fix(oidc): wire DefaultPermissions/DefaultLibraryIDs into JIT provisioning

zombor referenced this pull request 2026-08-09 01:59:04 +00:00

fix(oidc): wire DefaultPermissions/DefaultLibraryIDs into JIT provisioning (bookshelf-r5dix) #1401

zombor referenced this pull request 2026-08-12 20:00:32 +00:00

fix(review): address batch MINOR/def-in-depth follow-ups (bookshelf-syyzy) #1437

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1395

No description provided.