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

Merged

zombor merged 2 commits from bd-bookshelf-r5dix into main 2026-08-09 13:34:50 +00:00

Conversation 2 Commits 2 Files changed 2 +77

zombor commented 2026-08-09 01:59:04 +00:00

Owner

Copy link

Summary

internal/app/app.go's GetOIDCConfig closure built appwire.OIDCRuntimeConfig with an explicit field list that dropped rc.DefaultPermissions and rc.DefaultLibraryIDs. Both fields exist on the source (settings.OIDCRuntimeConfig) and target (appwire.OIDCRuntimeConfig) types and are fully consumed downstream (internal/users/wire.go -> applyJITDefaults), but they never reached the production struct literal — so admin-configured OIDC JIT default permissions/default library access round-tripped through the DB but were silently dropped and never applied to newly auto-provisioned OIDC users (they got nil permissions/nil library access instead). Same bug class as the #1395 SessionDuration BLOCKER.

Fix

• internal/app/app.go: add DefaultPermissions: rc.DefaultPermissions and DefaultLibraryIDs: rc.DefaultLibraryIDs to the struct literal.
• e2e/api/journey_9_oidc_login_test.go: add a regression step mirroring the #1395 SessionDuration guard. internal/app is untested wiring per project convention, so the only way to exercise the real GetOIDCConfig closure is booting the real app.New server end-to-end. The new step configures default_permissions/default_library_ids via PUT /settings/oidc, logs in a fresh OIDC user with no matching group mapping, and asserts the new user actually received the configured permission and library access. This step fails against the unfixed app.go.

Test plan

• go build ./... and go build -tags e2e ./e2e/... compile clean
• make test — all unit tests pass
• New e2e Journey-9 step added; will run in CI (make e2e)
• Docs: N/A because this is an internal wiring bugfix with no new user-facing surface — the OIDC settings page already documents default permissions/library access; this fix makes that existing setting actually take effect.

Closes bead bookshelf-r5dix on merge.

## Summary internal/app/app.go's GetOIDCConfig closure built appwire.OIDCRuntimeConfig with an explicit field list that dropped `rc.DefaultPermissions` and `rc.DefaultLibraryIDs`. Both fields exist on the source (`settings.OIDCRuntimeConfig`) and target (`appwire.OIDCRuntimeConfig`) types and are fully consumed downstream (`internal/users/wire.go` -> `applyJITDefaults`), but they never reached the production struct literal — so admin-configured OIDC JIT default permissions/default library access round-tripped through the DB but were silently dropped and never applied to newly auto-provisioned OIDC users (they got nil permissions/nil library access instead). Same bug class as the #1395 SessionDuration BLOCKER. ## Fix - `internal/app/app.go`: add `DefaultPermissions: rc.DefaultPermissions` and `DefaultLibraryIDs: rc.DefaultLibraryIDs` to the struct literal. - `e2e/api/journey_9_oidc_login_test.go`: add a regression step mirroring the #1395 SessionDuration guard. `internal/app` is untested wiring per project convention, so the only way to exercise the real `GetOIDCConfig` closure is booting the real `app.New` server end-to-end. The new step configures `default_permissions`/`default_library_ids` via `PUT /settings/oidc`, logs in a fresh OIDC user with no matching group mapping, and asserts the new user actually received the configured permission and library access. This step fails against the unfixed app.go. ## Test plan - [x] `go build ./...` and `go build -tags e2e ./e2e/...` compile clean - [x] `make test` — all unit tests pass - [x] New e2e Journey-9 step added; will run in CI (`make e2e`) - Docs: N/A because this is an internal wiring bugfix with no new user-facing surface — the OIDC settings page already documents default permissions/library access; this fix makes that existing setting actually take effect. Closes bead bookshelf-r5dix on merge.

zombor added 1 commit 2026-08-09 01:59:04 +00:00

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

internal/app/app.go's GetOIDCConfig closure built appwire.OIDCRuntimeConfig
with an explicit field list that dropped rc.DefaultPermissions and
rc.DefaultLibraryIDs, so admin-configured OIDC JIT defaults never reached
new auto-provisioned users at runtime (round-tripped in the DB but ignored
by createJITUser/applyJITDefaults). Same bug class as the #1395
SessionDuration BLOCKER.

Add a regression e2e journey step (Journey-9) mirroring the #1395
SessionDuration guard: boots the real app via app.New, configures
default_permissions + default_library_ids via PUT /settings/oidc, logs in
a fresh OIDC user with no matching group mapping, and asserts the new
user actually has the configured permission and library access. Fails
against the unfixed app.go.

Docs: N/A because this is an internal wiring bugfix with no new
user-facing surface (the OIDC settings UI/behavior already documents
default permissions/library access; this fix makes the existing setting
actually take effect).

zombor commented 2026-08-09 02:32:21 +00:00

Author

Owner

Copy link

Code review for PR #1401 (bd-bookshelf-r5dix)

Scope checked: correctness of the GetOIDCConfig closure fix in internal/app/app.go, completeness of the field-copy (no other silently-dropped fields), and whether the new e2e step in e2e/api/journey_9_oidc_login_test.go genuinely proves the regression against the real app.New wiring.

Findings:

1. internal/app/app.go:373-374 — adds DefaultPermissions: rc.DefaultPermissions and DefaultLibraryIDs: rc.DefaultLibraryIDs to the struct literal. Diffed both internal/settings/oidc_settings.go and internal/appwire/appwire.go OIDCRuntimeConfig definitions field-by-field against the literal in app.go — all 17 fields (Enabled … RPInitiatedLogout) are now copied 1:1. No other field is silently dropped in the same literal.

2. e2e/api/journey_9_oidc_login_test.go:887-960 (new Step 9b) — appended inside the existing Ordered "Journey-9" Describe (not a new top-level container), reusing the shared BeforeAll-booted real app/DB/fake-IdP from the rest of the file — complies with the Ordered-journey policy (no new BeforeEach(ResetDB), no isolated single-endpoint Describe). The step:

   • PUTs /settings/oidc with DefaultPermissions: ["permission_upload"] and DefaultLibraryIDs: [libraryID], matching the same PUT pattern already used earlier in the file (Step 9, for the analogous SessionDuration regression) — consistent with precedent, and this endpoint shape is proven by the file's own earlier green step.
   • Runs a real JIT-provisioning login (oidcFullFlow) for a brand-new subject with "groups": [] (no group mapping in play, isolating the default-permissions/libraries path specifically).
   • Asserts against real DB rows (user_permissions.permission_upload, user_library_mapping) rather than an in-memory stub — this exercises the actual GetOIDCConfig closure end-to-end. If either field were dropped from the literal again, rc.DefaultPermissions/rc.DefaultLibraryIDs would never reach appwire.OIDCRuntimeConfig, provisioning would apply no defaults, and this test would fail (hasUpload false / libraryCount 0) — a genuine regression guard, not a tautology.
   • Justification comment (lines 887-895) correctly explains why this can't be caught at unit level (internal/app is untested wiring per project convention; only booting real app.New exercises the literal).

3. Multi-Expect-per-It in the new step is allowed under the e2e journey policy's relaxation for a single action spanning multiple observations — no violation.

4. CI is green (success on the head SHA) and the PR is mergeable: true.

No correctness, security, scope, or convention issues found. This is a minimal, complete, well-targeted fix with a regression test that would have caught the original bug.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Code review for PR #1401 (bd-bookshelf-r5dix) **Scope checked:** correctness of the `GetOIDCConfig` closure fix in `internal/app/app.go`, completeness of the field-copy (no other silently-dropped fields), and whether the new e2e step in `e2e/api/journey_9_oidc_login_test.go` genuinely proves the regression against the real `app.New` wiring. **Findings:** 1. `internal/app/app.go:373-374` — adds `DefaultPermissions: rc.DefaultPermissions` and `DefaultLibraryIDs: rc.DefaultLibraryIDs` to the struct literal. Diffed both `internal/settings/oidc_settings.go` and `internal/appwire/appwire.go` `OIDCRuntimeConfig` definitions field-by-field against the literal in `app.go` — all 17 fields (`Enabled` … `RPInitiatedLogout`) are now copied 1:1. No other field is silently dropped in the same literal. 2. `e2e/api/journey_9_oidc_login_test.go:887-960` (new Step 9b) — appended inside the existing `Ordered` "Journey-9" `Describe` (not a new top-level container), reusing the shared `BeforeAll`-booted real app/DB/fake-IdP from the rest of the file — complies with the `Ordered`-journey policy (no new `BeforeEach(ResetDB)`, no isolated single-endpoint `Describe`). The step: - PUTs `/settings/oidc` with `DefaultPermissions: ["permission_upload"]` and `DefaultLibraryIDs: [libraryID]`, matching the same PUT pattern already used earlier in the file (Step 9, for the analogous `SessionDuration` regression) — consistent with precedent, and this endpoint shape is proven by the file's own earlier green step. - Runs a real JIT-provisioning login (`oidcFullFlow`) for a brand-new subject with `"groups": []` (no group mapping in play, isolating the default-permissions/libraries path specifically). - Asserts against real DB rows (`user_permissions.permission_upload`, `user_library_mapping`) rather than an in-memory stub — this exercises the actual `GetOIDCConfig` closure end-to-end. If either field were dropped from the literal again, `rc.DefaultPermissions`/`rc.DefaultLibraryIDs` would never reach `appwire.OIDCRuntimeConfig`, provisioning would apply no defaults, and this test would fail (`hasUpload` false / `libraryCount` 0) — a genuine regression guard, not a tautology. - Justification comment (lines 887-895) correctly explains why this can't be caught at unit level (`internal/app` is untested wiring per project convention; only booting real `app.New` exercises the literal). 3. Multi-`Expect`-per-`It` in the new step is allowed under the e2e journey policy's relaxation for a single action spanning multiple observations — no violation. 4. CI is green (`success` on the head SHA) and the PR is `mergeable: true`. No correctness, security, scope, or convention issues found. This is a minimal, complete, well-targeted fix with a regression test that would have caught the original bug. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-09 02:32:27 +00:00

Author

Owner

Copy link

Security review of PR #1401 (bd-bookshelf-r5dix) — read-only diff review.

Summary

Minimal 2-line wiring fix in internal/app/app.go (GetOIDCConfig closure): adds DefaultPermissions: rc.DefaultPermissions and DefaultLibraryIDs: rc.DefaultLibraryIDs to the appwire.OIDCRuntimeConfig struct literal, plus a Journey-9 e2e test asserting the fix end-to-end. Same class of bug as the earlier SessionDuration field-drop regression — a field silently missing from an explicit struct literal.

Verification performed

• Traced the full config path: settings.GetOIDCConfig (DB) → app.go GetOIDCConfig closure (the fix) → internal/users/wire.go getOIDCConfig adapter → OIDCConfig passed into applyJITDefaults (internal/users/oidc_service.go). Confirmed wire.go already correctly mapped rc.DefaultPermissions/rc.DefaultLibraryIDs — the only break was the missing fields in app.go's literal, now fixed.
• applyJITDefaults (pre-existing, untouched by this diff) explicitly strips permission_admin before building the upsert map:

  for _, key := range cfg.DefaultPermissions {
      if key != "permission_admin" {
          permMap[key] = true
      }
  }
  

  So admin cannot be auto-granted via JIT defaults even if an operator configures it — no privilege-escalation path introduced by this fix.
• PUT /settings/oidc (where DefaultPermissions/DefaultLibraryIDs are configured) is adminRequired-gated (internal/settings/routes.go) — only an admin can set these defaults; a non-admin cannot self-grant broader defaults for future JIT users.
• The JIT-provisioned target user ID is generated server-side at user-creation time (provisionOIDCUser → applyDefaults(ctx, userID)), never sourced from request/claims data — no cross-user injection vector.
• DefaultLibraryIDs are admin-configured values applied via setLibraries(ctx, userID, cfg.DefaultLibraryIDs), not attacker-influenced; the new user's library grants are limited to exactly the admin-configured set, not a wildcard.
• The new e2e test (journey_9_oidc_login_test.go) configures a single non-admin permission (permission_upload) and one library, then asserts via direct DB read that exactly that permission and exactly that library landed on the new user — a reasonable regression guard scoped to the same failure mode as the SessionDuration bug (config silently dropped by an explicit-field struct literal in untested wiring code, internal/app is exempt from unit tests per project convention, so an e2e boot of the real app.New closure is the only way to catch this).
• No SQL injection, SSRF, secret-logging, or CSP issues touched by this diff — no new SQL, no inline style=, no external fetch, no token/secret handling in the changed lines.

Findings

No blockers, majors, or minors found. This is a narrowly-scoped, well-justified regression fix with an appropriate test.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Security review of PR #1401 (bd-bookshelf-r5dix) — read-only diff review. ## Summary Minimal 2-line wiring fix in `internal/app/app.go` (`GetOIDCConfig` closure): adds `DefaultPermissions: rc.DefaultPermissions` and `DefaultLibraryIDs: rc.DefaultLibraryIDs` to the `appwire.OIDCRuntimeConfig` struct literal, plus a Journey-9 e2e test asserting the fix end-to-end. Same class of bug as the earlier `SessionDuration` field-drop regression — a field silently missing from an explicit struct literal. ## Verification performed - Traced the full config path: `settings.GetOIDCConfig` (DB) → `app.go` `GetOIDCConfig` closure (the fix) → `internal/users/wire.go` `getOIDCConfig` adapter → `OIDCConfig` passed into `applyJITDefaults` (`internal/users/oidc_service.go`). Confirmed `wire.go` already correctly mapped `rc.DefaultPermissions`/`rc.DefaultLibraryIDs` — the only break was the missing fields in `app.go`'s literal, now fixed. - `applyJITDefaults` (pre-existing, untouched by this diff) explicitly strips `permission_admin` before building the upsert map: ```go for _, key := range cfg.DefaultPermissions { if key != "permission_admin" { permMap[key] = true } } ``` So admin cannot be auto-granted via JIT defaults even if an operator configures it — no privilege-escalation path introduced by this fix. - `PUT /settings/oidc` (where `DefaultPermissions`/`DefaultLibraryIDs` are configured) is `adminRequired`-gated (`internal/settings/routes.go`) — only an admin can set these defaults; a non-admin cannot self-grant broader defaults for future JIT users. - The JIT-provisioned target user ID is generated server-side at user-creation time (`provisionOIDCUser` → `applyDefaults(ctx, userID)`), never sourced from request/claims data — no cross-user injection vector. - `DefaultLibraryIDs` are admin-configured values applied via `setLibraries(ctx, userID, cfg.DefaultLibraryIDs)`, not attacker-influenced; the new user's library grants are limited to exactly the admin-configured set, not a wildcard. - The new e2e test (`journey_9_oidc_login_test.go`) configures a single non-admin permission (`permission_upload`) and one library, then asserts via direct DB read that exactly that permission and exactly that library landed on the new user — a reasonable regression guard scoped to the same failure mode as the `SessionDuration` bug (config silently dropped by an explicit-field struct literal in untested wiring code, `internal/app` is exempt from unit tests per project convention, so an e2e boot of the real `app.New` closure is the only way to catch this). - No SQL injection, SSRF, secret-logging, or CSP issues touched by this diff — no new SQL, no inline `style=`, no external fetch, no token/secret handling in the changed lines. ## Findings No blockers, majors, or minors found. This is a narrowly-scoped, well-justified regression fix with an appropriate test. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor added 1 commit 2026-08-09 13:29:56 +00:00

Merge branch 'main' into bd-bookshelf-r5dix 032bdb07b4

zombor merged commit 9b7ed31ab5 into main 2026-08-09 13:34:50 +00:00

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

Merge pull request 'fix(oidc): wire DefaultPermissions/DefaultLibraryIDs into JIT provisioning (bookshelf-r5dix)' (#1401) from bd-bookshelf-r5dix into main

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!1401

No description provided.