zombor/pergamum

Fork 0

refactor(users): burn down OIDC/admin lint exclusions (bookshelf-bbsd.2) #916

Open

zombor wants to merge 1 commit from bd-bookshelf-bbsd.2 into main

zombor commented

2026-07-03 20:38:58 +00:00

Owner

Summary

Brings the grandfathered internal/users OIDC and admin god-functions under the now-live lint gates (funlen ≤60L/40stmt, gocyclo ≤15, nestif ≤5) and deletes their .golangci.yml exclusion entries.

Functions refactored:

OIDCHandleCallback — extracted oidcValidateCallbackState, oidcExchangeCode, oidcVerifyAndProvision, oidcRecordSession, oidcIssueTokens, buildOAuthConfig
provisionOIDCUser — extracted deriveDisplayName, deriveUsername
syncOIDCGroupMappings — extracted mergeGroupMappings
applyPermissionsJSON — replaced 27 individual if blocks with a package-level permSetterTable []permSetter
verifyIDToken — extracted buildJWKKeySet, resolveJWKSKey, validateTokenAudience
adminUsersListHandler — extracted parseListParams, buildUserListRows
adminUserUpdateHandler — extracted parseUpdateLibraryIDs, validateLibraryIDs, adminUpdateUserError

.golangci.yml lines removed: all internal/users/ entries from funlen, gocyclo, and nestif exclude-rules sections. Zero new exclusions added.

Coverage: Added test for deriveDisplayName's usernameClaim fallback branch (name absent, preferred_username present) to maintain 100% coverage gate.

Test plan

make lint — 0 issues
make test — pass
make coverage — check-coverage: OK
OIDC group→role mapping behavior unchanged (black-box tests preserved)

Closes bead bookshelf-bbsd.2 on merge.

## Summary Brings the grandfathered `internal/users` OIDC and admin god-functions under the now-live lint gates (funlen ≤60L/40stmt, gocyclo ≤15, nestif ≤5) and **deletes** their `.golangci.yml` exclusion entries. **Functions refactored:** - `OIDCHandleCallback` — extracted `oidcValidateCallbackState`, `oidcExchangeCode`, `oidcVerifyAndProvision`, `oidcRecordSession`, `oidcIssueTokens`, `buildOAuthConfig` - `provisionOIDCUser` — extracted `deriveDisplayName`, `deriveUsername` - `syncOIDCGroupMappings` — extracted `mergeGroupMappings` - `applyPermissionsJSON` — replaced 27 individual `if` blocks with a package-level `permSetterTable []permSetter` - `verifyIDToken` — extracted `buildJWKKeySet`, `resolveJWKSKey`, `validateTokenAudience` - `adminUsersListHandler` — extracted `parseListParams`, `buildUserListRows` - `adminUserUpdateHandler` — extracted `parseUpdateLibraryIDs`, `validateLibraryIDs`, `adminUpdateUserError` **`.golangci.yml` lines removed:** all `internal/users/` entries from `funlen`, `gocyclo`, and `nestif` exclude-rules sections. Zero new exclusions added. **Coverage:** Added test for `deriveDisplayName`'s `usernameClaim` fallback branch (`name` absent, `preferred_username` present) to maintain 100% coverage gate. ## Test plan - [x] `make lint` — 0 issues - [x] `make test` — pass - [x] `make coverage` — `check-coverage: OK` - [x] OIDC group→role mapping behavior unchanged (black-box tests preserved) Closes bead bookshelf-bbsd.2 on merge.

wf-detail-older-execution.png

56 KiB

recompute-match-score-kebab-open.png

109 KiB

zombor added 1 commit

2026-07-03 20:38:58 +00:00

refactor(users): bring OIDC/admin god-functions under lint gates and remove .golangci.yml exclusions (bookshelf-bbsd.2)

/ JS Unit Tests (pull_request) Successful in 1m5s

Details

/ E2E API (pull_request) Successful in 2m12s

Details

/ Lint (pull_request) Successful in 2m49s

Details

/ Integration (pull_request) Successful in 3m5s

Details

/ E2E Browser (pull_request) Successful in 3m23s

Details

/ Test (pull_request) Successful in 3m48s

Details

1eb66358f1

Refactored OIDCHandleCallback, provisionOIDCUser, syncOIDCGroupMappings,
applyPermissionsJSON (oidc_service.go), verifyIDToken (oidc_jwks.go), and
adminUsersListHandler/adminUserUpdateHandler (admin_handler.go) to pass
funlen ≤60L/40stmt, gocyclo ≤15, nestif ≤5 without any exclusions.

Extractions:
- oidc_service.go: buildOAuthConfig, oidcExchangeCode, oidcValidateCallbackState,
  oidcVerifyAndProvision, oidcRecordSession, oidcIssueTokens, deriveDisplayName,
  deriveUsername, mergeGroupMappings, permSetterTable (replaces 27 if-blocks in
  applyPermissionsJSON)
- oidc_jwks.go: buildJWKKeySet, resolveJWKSKey, validateTokenAudience
- admin_handler.go: parseListParams, buildUserListRows, parseUpdateLibraryIDs,
  validateLibraryIDs, adminUpdateUserError

Deleted all internal/users/* entries from .golangci.yml funlen/gocyclo/nestif
exclude-rules. No new exclusions added.

Added coverage test for deriveDisplayName usernameClaim fallback branch
(preferred_username present, name absent).

make lint: 0 issues
make coverage: OK (100%)
make test: pass

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

forgejo-actions commented

2026-07-03 20:41:24 +00:00

Workflow Detail page screenshot (wf-detail-older-execution)

Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent.

**Workflow Detail page screenshot** (wf-detail-older-execution) Older completed ContinueAsNew epoch detail — execution ID and state visible, Cancel absent. ![wf-detail-older-execution](/attachments/21cfcb39-0ac7-437b-a2e1-e30bbc2b7e51)

forgejo-actions commented

2026-07-03 20:41:36 +00:00

Recompute Match Score — kebab open screenshot (recompute-match-score-kebab-open)

**Recompute Match Score — kebab open screenshot** (recompute-match-score-kebab-open) ![recompute-match-score-kebab-open](/attachments/145548dd-5d50-4e87-a99c-4a92a1c626dc)

zombor commented

2026-07-03 20:48:49 +00:00

Author

Owner

CODE REVIEW: APPROVED

Reviewed diff of bd-bookshelf-bbsd.2 (refactor internal/users OIDC/admin god-functions + burn .golangci.yml exclusions).

Phase 0: DEMO Verification

This is a behavior-preserving internal refactor with no user-visible surface change. There is no DEMO block, which is appropriate — the CI gate and lint gate together are the truth signal for a pure refactor. CI: success. Lint exclusions removed: verified in diff. Proceeding to Phase 1.

Phase 1: Spec Compliance

Bead description: refactor OIDCHandleCallback, provisionOIDCUser, syncOIDCGroupMappings, applyPermissionsJSON, verifyIDToken, adminUsersListHandler, adminUserUpdateHandler; delete all internal/users/ .golangci.yml exclusions (funlen, gocyclo, nestif).

All seven functions refactored: confirmed in diff.

.golangci.yml burn-down — 7 exclusion blocks removed, 0 added:

funlen: admin_handler (adminUsersListHandler|adminUserUpdateHandler), oidc_jwks (verifyIDToken), oidc_service (OIDCHandleCallback|provisionOIDCUser|syncOIDCGroupMappings|applyPermissionsJSON) — all gone.
gocyclo: same three paths — all gone.
nestif: oidc_service — gone.

Phase 2: Behavior Preservation

Traced each extraction against the original control flow:

admin_handler.go

parseListParams (lines 130-153): afterID defaults to 0 (zero value, unchanged when parse fails), limit defaults to adminUsersDefaultLimit, capped at adminUsersMaxLimit. Identical to original inline logic.

buildUserListRows (lines 157-165): pre-allocates with make([]AdminUserListRow, len(pg.Users)), same loop body.

adminUsersListHandler error-param simplification (line 216): original was a switch with one case "self-delete"; new is an if == "self-delete". Behavior identical.

parseUpdateLibraryIDs / validateLibraryIDs / adminUpdateUserError: extracted verbatim. The UpdateUser error path (JSON branch: sets 422 + JSON body; HTML branch: re-fetches user, sets 422 + renders form) is preserved exactly in adminUpdateUserError. Call site returns adminUpdateUserError(...) so the early-return semantics are unchanged.

oidc_jwks.go

buildJWKKeySet: removed the keyEntry struct wrapper; stores pub any directly. resolveJWKSKey: reproduces the original kid-lookup with len==1 fallback faithfully.

validateTokenAudience: string/[]any/default cases match the original switch. The string case: on mismatch returns error, on match falls through to return nil. The []any case: early-return on first match, error after loop (replaces found bool). Functionally identical.

oidc_service.go

OIDCHandleCallback pipeline: getConfig -> oidcValidateCallbackState (Enabled check + state CSRF check) -> discoverMeta -> oidcExchangeCode (Exchange + extract id_token; ErrInvalidCredentials on Exchange failure) -> oidcVerifyAndProvision (fetchJWKS + verifyIDToken + provisionOIDCUser) -> oidcRecordSession (non-fatal insertOIDCSession) -> syncOIDCGroupMappings -> oidcIssueTokens. Every step maps 1:1 to the original inlined code. No branch dropped or reordered.

oidcRecordSession: cfg.Issuer passed as issuer argument at call site; OidcIssuer field receives the same value. Non-fatal error handling preserved.

deriveDisplayName: nameClaim -> usernameClaim -> email local-part -> "OIDC User" fallback chain matches the original if-cascade exactly.

deriveUsername: usernameClaim -> emailClaim -> sub matches original.

provisionOIDCUser: emailNull hoisted before the ErrNoRows branch — safe because the computation is identical in both branches (same email variable, same NullString construction).

mergeGroupMappings: groupSet construction, isAdmin accumulation, applyPermissionsJSON call, parseLibraryIDs + warn-and-continue on error — all preserved.

syncOIDCGroupMappings: anyMatched gating of setLibraries (both anyMatched==true with empty libIDs and anyMatched==false paths preserved).

applyPermissionsJSON: table-driven replacement covers all 27 permission keys. Verified count and field mappings against original if-chain.

Phase 3: oidc_coverage_test.go Anti-Gaming Audit

internal/users/oidc_coverage_test.go lines 435-461 — new Context "name derived from preferred_username when name claim is absent":

Supplies claims with "preferred_username": "charlie" and no "name" key.
Captures p.Name in the createOIDCUser stub via capturedName.
Asserts Expect(capturedName, err).To(Equal("charlie")) — checks both err==nil AND the derived name equals "charlie".

This is a genuine behavioral assertion covering the deriveDisplayName usernameClaim branch. Not coverage padding.

Package declaration confirmed: package users_test. ExportProvisionOIDCUser pre-exists on main (export_test.go: var ExportProvisionOIDCUser = provisionOIDCUser); not a new export introduced by this PR.

One-Expect-per-It: confirmed (single Expect in the new It block; err folded in via trailing-actual form).

Findings

[MINOR] internal/users/oidc_service.go — oidcVerifyAndProvision, oidcIssueTokens parameter count
Both functions exceed the project metric of <5 parameters (use object for >5): oidcVerifyAndProvision has 9, oidcIssueTokens has 8. They are unexported helpers extracted from an existing large function; adding a parameter struct is out of scope for this refactor bead. Note for a follow-up pass.

[MINOR] internal/users/admin_handler.go:387 — adminUpdateUserError parameter count
9 parameters (w, r, d, id, perms, libraryIDs, contentRestrictions, allLibraries, updateErr). Same reasoning as above.

[MINOR] internal/users/admin_handler.go — var-at-top preference not applied in refactored handlers
adminUsersListHandler and adminUserUpdateHandler use inline := throughout rather than a top-of-block var block. Per project conventions this is a stated preference, not absolute, and the functions are short post-extraction. Acceptable here.

REVIEW VERDICT: 0 blocker, 0 major, 3 minor

CODE REVIEW: APPROVED Reviewed diff of bd-bookshelf-bbsd.2 (refactor internal/users OIDC/admin god-functions + burn .golangci.yml exclusions). --- ## Phase 0: DEMO Verification This is a behavior-preserving internal refactor with no user-visible surface change. There is no DEMO block, which is appropriate — the CI gate and lint gate together are the truth signal for a pure refactor. CI: success. Lint exclusions removed: verified in diff. Proceeding to Phase 1. --- ## Phase 1: Spec Compliance Bead description: refactor OIDCHandleCallback, provisionOIDCUser, syncOIDCGroupMappings, applyPermissionsJSON, verifyIDToken, adminUsersListHandler, adminUserUpdateHandler; delete all internal/users/ .golangci.yml exclusions (funlen, gocyclo, nestif). All seven functions refactored: confirmed in diff. .golangci.yml burn-down — 7 exclusion blocks removed, 0 added: - funlen: admin_handler (adminUsersListHandler|adminUserUpdateHandler), oidc_jwks (verifyIDToken), oidc_service (OIDCHandleCallback|provisionOIDCUser|syncOIDCGroupMappings|applyPermissionsJSON) — all gone. - gocyclo: same three paths — all gone. - nestif: oidc_service — gone. --- ## Phase 2: Behavior Preservation Traced each extraction against the original control flow: **admin_handler.go** parseListParams (lines 130-153): afterID defaults to 0 (zero value, unchanged when parse fails), limit defaults to adminUsersDefaultLimit, capped at adminUsersMaxLimit. Identical to original inline logic. buildUserListRows (lines 157-165): pre-allocates with make([]AdminUserListRow, len(pg.Users)), same loop body. adminUsersListHandler error-param simplification (line 216): original was a switch with one case "self-delete"; new is an if == "self-delete". Behavior identical. parseUpdateLibraryIDs / validateLibraryIDs / adminUpdateUserError: extracted verbatim. The UpdateUser error path (JSON branch: sets 422 + JSON body; HTML branch: re-fetches user, sets 422 + renders form) is preserved exactly in adminUpdateUserError. Call site returns adminUpdateUserError(...) so the early-return semantics are unchanged. **oidc_jwks.go** buildJWKKeySet: removed the keyEntry struct wrapper; stores pub any directly. resolveJWKSKey: reproduces the original kid-lookup with len==1 fallback faithfully. validateTokenAudience: string/[]any/default cases match the original switch. The string case: on mismatch returns error, on match falls through to return nil. The []any case: early-return on first match, error after loop (replaces found bool). Functionally identical. **oidc_service.go** OIDCHandleCallback pipeline: getConfig -> oidcValidateCallbackState (Enabled check + state CSRF check) -> discoverMeta -> oidcExchangeCode (Exchange + extract id_token; ErrInvalidCredentials on Exchange failure) -> oidcVerifyAndProvision (fetchJWKS + verifyIDToken + provisionOIDCUser) -> oidcRecordSession (non-fatal insertOIDCSession) -> syncOIDCGroupMappings -> oidcIssueTokens. Every step maps 1:1 to the original inlined code. No branch dropped or reordered. oidcRecordSession: cfg.Issuer passed as issuer argument at call site; OidcIssuer field receives the same value. Non-fatal error handling preserved. deriveDisplayName: nameClaim -> usernameClaim -> email local-part -> "OIDC User" fallback chain matches the original if-cascade exactly. deriveUsername: usernameClaim -> emailClaim -> sub matches original. provisionOIDCUser: emailNull hoisted before the ErrNoRows branch — safe because the computation is identical in both branches (same email variable, same NullString construction). mergeGroupMappings: groupSet construction, isAdmin accumulation, applyPermissionsJSON call, parseLibraryIDs + warn-and-continue on error — all preserved. syncOIDCGroupMappings: anyMatched gating of setLibraries (both anyMatched==true with empty libIDs and anyMatched==false paths preserved). applyPermissionsJSON: table-driven replacement covers all 27 permission keys. Verified count and field mappings against original if-chain. --- ## Phase 3: oidc_coverage_test.go Anti-Gaming Audit internal/users/oidc_coverage_test.go lines 435-461 — new Context "name derived from preferred_username when name claim is absent": - Supplies claims with "preferred_username": "charlie" and no "name" key. - Captures p.Name in the createOIDCUser stub via capturedName. - Asserts Expect(capturedName, err).To(Equal("charlie")) — checks both err==nil AND the derived name equals "charlie". This is a genuine behavioral assertion covering the deriveDisplayName usernameClaim branch. Not coverage padding. Package declaration confirmed: package users_test. ExportProvisionOIDCUser pre-exists on main (export_test.go: var ExportProvisionOIDCUser = provisionOIDCUser); not a new export introduced by this PR. One-Expect-per-It: confirmed (single Expect in the new It block; err folded in via trailing-actual form). --- ## Findings [MINOR] internal/users/oidc_service.go — oidcVerifyAndProvision, oidcIssueTokens parameter count Both functions exceed the project metric of <5 parameters (use object for >5): oidcVerifyAndProvision has 9, oidcIssueTokens has 8. They are unexported helpers extracted from an existing large function; adding a parameter struct is out of scope for this refactor bead. Note for a follow-up pass. [MINOR] internal/users/admin_handler.go:387 — adminUpdateUserError parameter count 9 parameters (w, r, d, id, perms, libraryIDs, contentRestrictions, allLibraries, updateErr). Same reasoning as above. [MINOR] internal/users/admin_handler.go — var-at-top preference not applied in refactored handlers adminUsersListHandler and adminUserUpdateHandler use inline := throughout rather than a top-of-block var block. Per project conventions this is a stated preference, not absolute, and the functions are short post-extraction. Acceptable here. --- REVIEW VERDICT: 0 blocker, 0 major, 3 minor

zombor commented

2026-07-03 20:48:57 +00:00

Author

Owner

Security Review — bookshelf-bbsd.2 (PR #916)

Refactor of OIDC/admin god-functions into extracted helpers. This is auth code; the bar is high. Traced every extracted helper against the original monolithic function.

1. verifyIDToken — all token checks survive

Signature validation: jwt.ParseWithClaims with the JWKS-keyed function is unchanged. buildJWKKeySet + resolveJWKSKey produce the same key lookup as the original keyEntry map with identical fall-back-to-sole-key logic.

Expiry / not-before / issued-at: jwt.MapClaims.Valid() (called internally by ParseWithClaims in golang-jwt v5) validates exp, nbf, and iat. This was true before and after the refactor.

Issuer: manually validated after ParseWithClaims at the same position as original — claims["iss"] != expectedIssuer → error.

Audience: extracted into validateTokenAudience(claims jwt.MapClaims, expectedAudience string) error. Logic is byte-equivalent to the original inline switch: string case rejects mismatch; []any case iterates with early-return nil on match or error if exhausted; default rejects unknown type. No path change.

Nonce: never implemented before this PR (no nonce parameter in OIDCBeginLogin, no nonce check in callback). Not dropped by this refactor — pre-existing gap outside this diff's scope.

2. OIDCHandleCallback — state/CSRF check survives and order preserved

oidcValidateCallbackState(cfg, state, cookieState, logger) is called first after getConfig, before discoverMeta, before oidcExchangeCode, before oidcVerifyAndProvision. Sequence: state check → discover → exchange code → verify token → provision user → record session (non-fatal) → sync groups (fatal) → issue tokens. Identical to original.

oidcValidateCallbackState checks: OIDC disabled → ErrOIDCNotEnabled; state == "" || state != cookieState → ErrInvalidCredentials. Same predicates, same error values.

3. provisionOIDCUser / syncOIDCGroupMappings / applyPermissionsJSON — mapping logic identical

applyPermissionsJSON / permSetterTable: the 27-entry table covers the exact same permission keys as the original 27 if m["permission_*"] blocks in the same order. Iterating the table with if m[s.key] { s.set(dst) } is semantically identical to the original if-chain. Verified by inspection: count matches.

mergeGroupMappings: extracted loop body is identical — groupSet membership test, IsAdmin OR-accumulation, applyPermissionsJSON for permissions, parseLibraryIDs for library IDs with the same warning-and-continue on parse error.

syncOIDCGroupMappings: the unconditional upsertPermissions (zero-permissions when no groups match, revoking stale grants) and the setLibraries([]int64{}) empty-revoke path both survive.

Admin assignment: merged.Perms.PermissionAdmin = true when merged.IsAdmin — unchanged.

emailNull hoisting: original declared emailNull separately in both the create-branch and update-branch. New code declares it once before the errors.Is(err, sql.ErrNoRows) branch. Functionally identical — same value used in both paths.

4. adminUserUpdateHandler / adminUsersListHandler — authz intact

claims := ClaimsFromContext(r.Context()) nil-guard is present and identical. claims.UserID (from the authenticated session, not from the request body) is still passed as the first argument to UpdateUser, CreateUser, and DeleteUser. Route-level AdminRequired middleware remains the outer gate (not changed in this diff). id comes from r.PathValue("id") (URL path), not from the body. Library ID validation via validateLibraryIDs is equivalent to the original inline set-membership check — same logic, same ErrValidation error type.

5. No auth bypass via error handling

Every extracted helper that can fail returns a non-nil error that the caller propagates, blocking login:

oidcValidateCallbackState → caller returns immediately
oidcExchangeCode → returns ErrInvalidCredentials on exchange failure, preserving the original "redirect gracefully" behaviour
oidcVerifyAndProvision → every error in fetchJWKS, verifyIDToken, or provisionOIDCUser returns non-nil; no success value is produced on error
oidcIssueTokens → every error propagates

oidcRecordSession remains non-fatal (session-tracking failure logs at Error but does not block login) — same as original.

6. Secrets / PII — no new exposure

rawIDToken continues to be stored in the DB as IDTokenHint (pre-existing OIDC logout-hint behaviour) but is not newly logged. ClientSecret is consumed inside buildOAuthConfig passed to oauth2.Config and is never logged. JWKS bytes are not logged. The warn-on-exchange-failure log entry records the OAuth error string from the provider (not the authorization code), same as before.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — bookshelf-bbsd.2 (PR #916) Refactor of OIDC/admin god-functions into extracted helpers. This is auth code; the bar is high. Traced every extracted helper against the original monolithic function. --- ### 1. verifyIDToken — all token checks survive **Signature validation:** `jwt.ParseWithClaims` with the JWKS-keyed function is unchanged. `buildJWKKeySet` + `resolveJWKSKey` produce the same key lookup as the original `keyEntry` map with identical fall-back-to-sole-key logic. **Expiry / not-before / issued-at:** `jwt.MapClaims.Valid()` (called internally by `ParseWithClaims` in golang-jwt v5) validates `exp`, `nbf`, and `iat`. This was true before and after the refactor. **Issuer:** manually validated after `ParseWithClaims` at the same position as original — `claims["iss"] != expectedIssuer` → error. **Audience:** extracted into `validateTokenAudience(claims jwt.MapClaims, expectedAudience string) error`. Logic is byte-equivalent to the original inline switch: string case rejects mismatch; `[]any` case iterates with early-return nil on match or error if exhausted; default rejects unknown type. No path change. **Nonce:** never implemented before this PR (no `nonce` parameter in `OIDCBeginLogin`, no nonce check in callback). Not dropped by this refactor — pre-existing gap outside this diff's scope. ### 2. OIDCHandleCallback — state/CSRF check survives and order preserved `oidcValidateCallbackState(cfg, state, cookieState, logger)` is called **first** after `getConfig`, before `discoverMeta`, before `oidcExchangeCode`, before `oidcVerifyAndProvision`. Sequence: state check → discover → exchange code → verify token → provision user → record session (non-fatal) → sync groups (fatal) → issue tokens. Identical to original. `oidcValidateCallbackState` checks: OIDC disabled → `ErrOIDCNotEnabled`; `state == "" || state != cookieState` → `ErrInvalidCredentials`. Same predicates, same error values. ### 3. provisionOIDCUser / syncOIDCGroupMappings / applyPermissionsJSON — mapping logic identical **applyPermissionsJSON / permSetterTable:** the 27-entry table covers the exact same permission keys as the original 27 `if m["permission_*"]` blocks in the same order. Iterating the table with `if m[s.key] { s.set(dst) }` is semantically identical to the original if-chain. Verified by inspection: count matches. **mergeGroupMappings:** extracted loop body is identical — `groupSet` membership test, `IsAdmin` OR-accumulation, `applyPermissionsJSON` for permissions, `parseLibraryIDs` for library IDs with the same warning-and-continue on parse error. **syncOIDCGroupMappings:** the unconditional `upsertPermissions` (zero-permissions when no groups match, revoking stale grants) and the `setLibraries([]int64{})` empty-revoke path both survive. **Admin assignment:** `merged.Perms.PermissionAdmin = true` when `merged.IsAdmin` — unchanged. **emailNull hoisting:** original declared `emailNull` separately in both the create-branch and update-branch. New code declares it once before the `errors.Is(err, sql.ErrNoRows)` branch. Functionally identical — same value used in both paths. ### 4. adminUserUpdateHandler / adminUsersListHandler — authz intact `claims := ClaimsFromContext(r.Context())` nil-guard is present and identical. `claims.UserID` (from the authenticated session, not from the request body) is still passed as the first argument to `UpdateUser`, `CreateUser`, and `DeleteUser`. Route-level `AdminRequired` middleware remains the outer gate (not changed in this diff). `id` comes from `r.PathValue("id")` (URL path), not from the body. Library ID validation via `validateLibraryIDs` is equivalent to the original inline set-membership check — same logic, same `ErrValidation` error type. ### 5. No auth bypass via error handling Every extracted helper that can fail returns a non-nil error that the caller propagates, blocking login: - `oidcValidateCallbackState` → caller returns immediately - `oidcExchangeCode` → returns `ErrInvalidCredentials` on exchange failure, preserving the original "redirect gracefully" behaviour - `oidcVerifyAndProvision` → every error in `fetchJWKS`, `verifyIDToken`, or `provisionOIDCUser` returns non-nil; no success value is produced on error - `oidcIssueTokens` → every error propagates `oidcRecordSession` remains non-fatal (session-tracking failure logs at Error but does not block login) — same as original. ### 6. Secrets / PII — no new exposure `rawIDToken` continues to be stored in the DB as `IDTokenHint` (pre-existing OIDC logout-hint behaviour) but is not newly logged. `ClientSecret` is consumed inside `buildOAuthConfig` passed to `oauth2.Config` and is never logged. JWKS bytes are not logged. The warn-on-exchange-failure log entry records the OAuth error string from the provider (not the authorization code), same as before. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

/ JS Unit Tests (pull_request) Successful in 1m5s

Details

/ E2E API (pull_request) Successful in 2m12s

Required

Details

/ Lint (pull_request) Successful in 2m49s

Required

Details

/ Integration (pull_request) Successful in 3m5s

Required

Details

/ E2E Browser (pull_request) Successful in 3m23s

Required

Details

/ Test (pull_request) Successful in 3m48s

Required

Details

This pull request is blocked because it's outdated.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin bd-bookshelf-bbsd.2:bd-bookshelf-bbsd.2

git switch bd-bookshelf-bbsd.2

No reviewers

No labels

No milestone

No project

No assignees

2 participants

Notifications

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

No description provided.

Rows
Columns