feat(auth): RBAC permission middleware + nav gating + bookdrop gating (bookshelf-4op.3.1) #458

Merged

zombor merged 2 commits from bd-bookshelf-4op.3.1 into main 2026-06-09 13:41:41 +00:00

Conversation 2 Commits 2 Files changed 13 +743 -53

zombor commented 2026-06-09 13:09:31 +00:00

Owner

Copy link

Summary

• Add PermissionRequired(getPerms, check, logger) middleware in internal/users/: admin short-circuits via JWT isAdmin (zero DB calls), non-admin does a single PK-indexed SELECT on user_permissions, result cached in request context so repeated checks share one round-trip. Fail-closed: missing row → 403, DB error → 403.
• Add GetPermissions public helper so the extractUser closure in app.go reuses the same context cache when populating CurrentUser.CanAccessBookdrop on every page render.
• Expose CanAccessBookdrop bool on tmpl.CurrentUser/BaseData for template use (bookshelf-v1at).
• Gate nav links in base.html: BookDrop wrapped in {{if .CurrentUser.CanAccessBookdrop}}; Field Priority and Metadata Providers moved inside existing {{if .CurrentUser.IsAdmin}} block (bookshelf-v1at).
• Apply BookdropRequired middleware to all 6 bookdrop routes via bookdrop.RegisterRoutes — GET /bookdrop + 5 POSTs (bookshelf-f8so).
• Wire BookdropRequired in appwire.Deps and app.go.
• Add GetUserPermissions :one sqlc query + regenerate internal/db/sqlc/.
• 100% unit test coverage on all new code.

Test plan

• make test — all unit tests pass
• make coverage — 100% coverage gate passes
• Admin user: no DB lookup, all routes accessible
• Non-admin with flag set: DB lookup, route accessible
• Non-admin with flag unset: 403
• Non-admin with no user_permissions row: 403 (fail-closed)
• DB error: 403 (fail-closed)
• No claims at all: 401 (delegated to denyRequest)
• JSON client gets {"error":"Forbidden"} on 403
• Context cache: second check in same request skips DB
• Bookdrop route test: passthrough middleware → 200, forbid middleware → 403

Closes bead bookshelf-4op.3.1 on merge.
Also resolves bookshelf-v1at and bookshelf-f8so (folded into this slice).

## Summary - Add `PermissionRequired(getPerms, check, logger)` middleware in `internal/users/`: admin short-circuits via JWT `isAdmin` (zero DB calls), non-admin does a single PK-indexed `SELECT` on `user_permissions`, result cached in request context so repeated checks share one round-trip. Fail-closed: missing row → 403, DB error → 403. - Add `GetPermissions` public helper so the `extractUser` closure in `app.go` reuses the same context cache when populating `CurrentUser.CanAccessBookdrop` on every page render. - Expose `CanAccessBookdrop bool` on `tmpl.CurrentUser`/`BaseData` for template use (bookshelf-v1at). - Gate nav links in `base.html`: BookDrop wrapped in `{{if .CurrentUser.CanAccessBookdrop}}`; Field Priority and Metadata Providers moved inside existing `{{if .CurrentUser.IsAdmin}}` block (bookshelf-v1at). - Apply `BookdropRequired` middleware to all 6 bookdrop routes via `bookdrop.RegisterRoutes` — GET /bookdrop + 5 POSTs (bookshelf-f8so). - Wire `BookdropRequired` in `appwire.Deps` and `app.go`. - Add `GetUserPermissions :one` sqlc query + regenerate `internal/db/sqlc/`. - 100% unit test coverage on all new code. ## Test plan - [x] `make test` — all unit tests pass - [x] `make coverage` — 100% coverage gate passes - [x] Admin user: no DB lookup, all routes accessible - [x] Non-admin with flag set: DB lookup, route accessible - [x] Non-admin with flag unset: 403 - [x] Non-admin with no `user_permissions` row: 403 (fail-closed) - [x] DB error: 403 (fail-closed) - [x] No claims at all: 401 (delegated to `denyRequest`) - [x] JSON client gets `{"error":"Forbidden"}` on 403 - [x] Context cache: second check in same request skips DB - [x] Bookdrop route test: passthrough middleware → 200, forbid middleware → 403 Closes bead bookshelf-4op.3.1 on merge. Also resolves bookshelf-v1at and bookshelf-f8so (folded into this slice).

zombor added 1 commit 2026-06-09 13:09:31 +00:00

feat(auth): RBAC permission middleware + nav visibility + bookdrop gating (bookshelf-4op.3.1) ... b67aa97929

Add PermissionRequired middleware with per-request DB-lookup hybrid:
- Admin short-circuits via JWT isAdmin claim (no DB hit)
- Non-admin: single PK-indexed SELECT on user_permissions, cached in
  request context so multiple checks per request share one round-trip
- Fail-closed: missing row → 403; DB error → 403

Wire GetUserPermissions sqlc query (28 Grimmory permission columns).
Expose CanAccessBookdrop on tmpl.CurrentUser for template use.

Nav visibility (v1at):
- Field Priority and Metadata Providers links gated to IsAdmin
- BookDrop link gated to CanAccessBookdrop (admin OR flag-set user)

Bookdrop route gating (f8so):
- All 6 bookdrop routes (GET /bookdrop + 5 POSTs) wrapped with
  BookdropRequired middleware in RegisterRoutes

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

zombor commented 2026-06-09 13:29:26 +00:00

Author

Owner

Copy link

Code Review (bot)

PR #458 — RBAC foundation: PermissionRequired middleware + bookdrop gating

---

Phase 0: DEMO Verification

No DEMO block found in bead comments (bd comments bookshelf-4op.3.1) or PR description. Per review standard, this is a NOT APPROVED gate. The PR has a test plan (make test / make coverage) but no runnable DEMO command.

---

Phase 1: Spec Compliance

All stated requirements are present:

• PermissionRequired middleware with admin short-circuit, per-request DB lookup, context cache, fail-closed — implemented in internal/users/permissions.go
• GetPermissions public helper for the extractUser closure — present
• CurrentUser.CanAccessBookdrop surfaced on tmpl.CurrentUser — present
• Nav gating in base.html: BookDrop wrapped in {{if .CurrentUser.CanAccessBookdrop}}, Field Priority + Metadata Providers inside {{if .CurrentUser.IsAdmin}} — correct and matches existing route-level AdminRequired enforcement in internal/settings/routes.go
• All 6 bookdrop routes gated: GET /bookdrop, POST /bookdrop/accept, POST /bookdrop/reject, POST /bookdrop/refresh, POST /bookdrop/{id}/reject, POST /bookdrop/{id}/accept — verified in internal/bookdrop/routes.go
• BookdropRequired wired through appwire.Deps and app.go — present
• GetUserPermissions :one sqlc query + regenerated internal/db/sqlc/ — present

---

Phase 2: Code Quality

[MINOR] internal/users/permissions.go:20 — UserPermissions struct is defined but never used
The struct type UserPermissions struct { AccessBookdrop bool } was likely a design artifact from an earlier iteration. The actual cache and all callers use dbsqlc.GetUserPermissionsRow directly. The comment on line 14 ("permsKey is the unexported context key used to cache resolved UserPermissions") is also misleading — it caches GetUserPermissionsRow, not UserPermissions. Remove the struct and fix the comment to avoid confusion when the next permission is added.

[MINOR] internal/bookdrop/routes_test.go — mutation routes not covered by gate test
The test suite covers GET /bookdrop gating (passthrough → 200, forbid → 403) but none of the five mutation routes (POST /bookdrop/accept, POST /bookdrop/reject, POST /bookdrop/refresh, POST /bookdrop/{id}/reject, POST /bookdrop/{id}/accept) have a gating assertion. The implementation is correct (all routes use the same gate variable), but a test that verifies at least one mutation route under forbidMiddleware returns 403 would give stronger regression protection.

Positive observations:

• Admin short-circuit is correctly implemented: the noPermsRow trick in the admin test proves the DB stub is never called.
• ErrNoRows detection is correct: resolvePermissions wraps with %w and errors.Is unwraps properly.
• Context cache is request-scoped (context.WithValue on r.Context()), not cross-request — correct.
• contextKey typed int prevents collision between claimsKey=1 and permsKey=2.
• GetUserPermissions SQL query uses COALESCE(..., 0) so a partial row (NULL columns) still returns false, not a scan error.
• forbidRequest (vs denyRequest) distinction is correct: missing claims → 401 redirect, missing permission → 403.
• Ginkgo test structure is correct: BeforeEach for setup, separate It blocks for each assertion.

---

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

NOT APPROVED — no DEMO block provided (review standard Phase 0 gate).

## Code Review (bot) **PR #458 — RBAC foundation: PermissionRequired middleware + bookdrop gating** --- ### Phase 0: DEMO Verification No DEMO block found in bead comments (`bd comments bookshelf-4op.3.1`) or PR description. Per review standard, this is a NOT APPROVED gate. The PR has a test plan (make test / make coverage) but no runnable DEMO command. --- ### Phase 1: Spec Compliance All stated requirements are present: - `PermissionRequired` middleware with admin short-circuit, per-request DB lookup, context cache, fail-closed — implemented in `internal/users/permissions.go` - `GetPermissions` public helper for the `extractUser` closure — present - `CurrentUser.CanAccessBookdrop` surfaced on `tmpl.CurrentUser` — present - Nav gating in `base.html`: BookDrop wrapped in `{{if .CurrentUser.CanAccessBookdrop}}`, Field Priority + Metadata Providers inside `{{if .CurrentUser.IsAdmin}}` — correct and matches existing route-level `AdminRequired` enforcement in `internal/settings/routes.go` - All 6 bookdrop routes gated: GET /bookdrop, POST /bookdrop/accept, POST /bookdrop/reject, POST /bookdrop/refresh, POST /bookdrop/{id}/reject, POST /bookdrop/{id}/accept — verified in `internal/bookdrop/routes.go` - `BookdropRequired` wired through `appwire.Deps` and `app.go` — present - `GetUserPermissions :one` sqlc query + regenerated `internal/db/sqlc/` — present --- ### Phase 2: Code Quality [MINOR] internal/users/permissions.go:20 — `UserPermissions` struct is defined but never used The struct `type UserPermissions struct { AccessBookdrop bool }` was likely a design artifact from an earlier iteration. The actual cache and all callers use `dbsqlc.GetUserPermissionsRow` directly. The comment on line 14 ("permsKey is the unexported context key used to cache resolved UserPermissions") is also misleading — it caches `GetUserPermissionsRow`, not `UserPermissions`. Remove the struct and fix the comment to avoid confusion when the next permission is added. [MINOR] internal/bookdrop/routes_test.go — mutation routes not covered by gate test The test suite covers `GET /bookdrop` gating (passthrough → 200, forbid → 403) but none of the five mutation routes (`POST /bookdrop/accept`, `POST /bookdrop/reject`, `POST /bookdrop/refresh`, `POST /bookdrop/{id}/reject`, `POST /bookdrop/{id}/accept`) have a gating assertion. The implementation is correct (all routes use the same `gate` variable), but a test that verifies at least one mutation route under `forbidMiddleware` returns 403 would give stronger regression protection. **Positive observations:** - Admin short-circuit is correctly implemented: the `noPermsRow` trick in the admin test proves the DB stub is never called. - ErrNoRows detection is correct: `resolvePermissions` wraps with `%w` and `errors.Is` unwraps properly. - Context cache is request-scoped (context.WithValue on `r.Context()`), not cross-request — correct. - `contextKey` typed int prevents collision between `claimsKey=1` and `permsKey=2`. - `GetUserPermissions` SQL query uses `COALESCE(..., 0)` so a partial row (NULL columns) still returns false, not a scan error. - `forbidRequest` (vs `denyRequest`) distinction is correct: missing claims → 401 redirect, missing permission → 403. - Ginkgo test structure is correct: `BeforeEach` for setup, separate `It` blocks for each assertion. --- **REVIEW VERDICT: 0 blocker, 0 major, 2 minor** NOT APPROVED — no DEMO block provided (review standard Phase 0 gate).

zombor commented 2026-06-09 13:29:49 +00:00

Author

Owner

Copy link

Security Review (bot)

PR #458 — RBAC permission middleware + nav visibility + bookdrop gating (bookshelf-4op.3.1)

---

Fail-Closed Analysis

All failure modes deny access (403/401), never allow:

• claims == nil → denyRequest (401) ✓
• sql.ErrNoRows (no permissions row) → forbidRequest (403) ✓ — explicitly tested
• Any other DB error → forbidRequest (403) ✓ — explicitly tested
• check(perms) returns false → forbidRequest (403) ✓

No fail-open path exists. The if err == nil guard in extractUser (app.go:191) for the nav-visible CanAccessBookdrop flag also fails closed — on error the flag stays false (nav link hidden). Correct for a display-only signal that is NOT the gate.

---

UserID / IsAdmin Source

• claims.UserID and claims.IsAdmin are extracted exclusively from ClaimsFromContext(r.Context()), which is populated by AuthMiddleware after verifying the HS256 JWT signature against the server secret.
• isAdmin is embedded in the token at login time (issueAccessToken) sourced from a DB lookup (getUserIsAdmin). There is no path that reads IsAdmin from a request header, body, or query parameter.
• Admin short-circuit in PermissionRequired: reads claims.IsAdmin (JWT-bound), calls next.ServeHTTP directly. Cannot be spoofed without forging the JWT signature.

---

Per-Request Cache Isolation

• Cache key is permsKey contextKey = 2 (unexported typed constant) — cannot be set from outside the users package.
• context.WithValue is immutable; each HTTP request in net/http gets a fresh context.Background()-rooted context. No cross-request state sharing is possible.
• The perms row is written to context only after check(perms) passes (line 77 of permissions.go). A denied request never caches its zero-value row, so there is no "false allow on second check in same request" path.
• Admin short-circuit does NOT write permsKey to context. Downstream GetPermissions(isAdmin=true) returns the fully-set synthetic row without a DB call — correct.

---

Bookdrop Route Coverage

All 6 registered routes are wrapped with gate := d.BookdropRequired:

Route	Gated
GET /bookdrop	✓
POST /bookdrop/accept (bulk)	✓
POST /bookdrop/reject (bulk)	✓
POST /bookdrop/refresh	✓
POST /bookdrop/{id}/reject	✓
POST /bookdrop/{id}/accept	✓

No bookdrop routes are registered outside internal/bookdrop/routes.go. Verified: wfengine and app register no additional bookdrop HTTP routes.

---

Nav Hiding vs Route Authz Alignment

• /bookdrop nav: {{if .CurrentUser.CanAccessBookdrop}} ← matches BookdropRequired (admin OR perm flag). Defense-in-depth correct.
• /settings/metadata-field-priority and /settings/metadata-providers nav: {{if .CurrentUser.IsAdmin}} ← both routes are gated by adminRequired in internal/settings/routes.go. Defense-in-depth correct.
• IsAdmin in CurrentUser comes from JWT claims — same trust anchor as the middleware.

---

Findings

[MINOR] internal/users/permissions.go:17 — UserPermissions struct is defined but never used
The type UserPermissions struct { AccessBookdrop bool } is declared but referenced nowhere in production code. The actual API surface uses dbsqlc.GetUserPermissionsRow throughout. This is dead code that will cause a lint warning and reader confusion about whether a projection type is intended. Remove it, or document why it is reserved for future use.

---

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Security Review (bot) **PR #458 — RBAC permission middleware + nav visibility + bookdrop gating (bookshelf-4op.3.1)** --- ### Fail-Closed Analysis All failure modes deny access (403/401), never allow: - `claims == nil` → `denyRequest` (401) ✓ - `sql.ErrNoRows` (no permissions row) → `forbidRequest` (403) ✓ — explicitly tested - Any other DB error → `forbidRequest` (403) ✓ — explicitly tested - `check(perms)` returns false → `forbidRequest` (403) ✓ No fail-open path exists. The `if err == nil` guard in `extractUser` (app.go:191) for the nav-visible `CanAccessBookdrop` flag also fails closed — on error the flag stays `false` (nav link hidden). Correct for a display-only signal that is NOT the gate. --- ### UserID / IsAdmin Source - `claims.UserID` and `claims.IsAdmin` are extracted exclusively from `ClaimsFromContext(r.Context())`, which is populated by `AuthMiddleware` after verifying the HS256 JWT signature against the server secret. - `isAdmin` is embedded in the token at login time (`issueAccessToken`) sourced from a DB lookup (`getUserIsAdmin`). There is no path that reads `IsAdmin` from a request header, body, or query parameter. - Admin short-circuit in `PermissionRequired`: reads `claims.IsAdmin` (JWT-bound), calls `next.ServeHTTP` directly. Cannot be spoofed without forging the JWT signature. --- ### Per-Request Cache Isolation - Cache key is `permsKey contextKey = 2` (unexported typed constant) — cannot be set from outside the `users` package. - `context.WithValue` is immutable; each HTTP request in `net/http` gets a fresh `context.Background()`-rooted context. No cross-request state sharing is possible. - The perms row is written to context **only after `check(perms)` passes** (line 77 of permissions.go). A denied request never caches its zero-value row, so there is no "false allow on second check in same request" path. - Admin short-circuit does NOT write `permsKey` to context. Downstream `GetPermissions(isAdmin=true)` returns the fully-set synthetic row without a DB call — correct. --- ### Bookdrop Route Coverage All 6 registered routes are wrapped with `gate := d.BookdropRequired`: | Route | Gated | |-------|-------| | `GET /bookdrop` | ✓ | | `POST /bookdrop/accept` (bulk) | ✓ | | `POST /bookdrop/reject` (bulk) | ✓ | | `POST /bookdrop/refresh` | ✓ | | `POST /bookdrop/{id}/reject` | ✓ | | `POST /bookdrop/{id}/accept` | ✓ | No bookdrop routes are registered outside `internal/bookdrop/routes.go`. Verified: `wfengine` and `app` register no additional bookdrop HTTP routes. --- ### Nav Hiding vs Route Authz Alignment - `/bookdrop` nav: `{{if .CurrentUser.CanAccessBookdrop}}` ← matches `BookdropRequired` (admin OR perm flag). Defense-in-depth correct. - `/settings/metadata-field-priority` and `/settings/metadata-providers` nav: `{{if .CurrentUser.IsAdmin}}` ← both routes are gated by `adminRequired` in `internal/settings/routes.go`. Defense-in-depth correct. - `IsAdmin` in `CurrentUser` comes from JWT claims — same trust anchor as the middleware. --- ### Findings [MINOR] internal/users/permissions.go:17 — `UserPermissions` struct is defined but never used The type `UserPermissions struct { AccessBookdrop bool }` is declared but referenced nowhere in production code. The actual API surface uses `dbsqlc.GetUserPermissionsRow` throughout. This is dead code that will cause a lint warning and reader confusion about whether a projection type is intended. Remove it, or document why it is reserved for future use. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor force-pushed bd-bookshelf-4op.3.1 from b67aa97929 to 746e93554f 2026-06-09 13:32:19 +00:00 Compare

zombor merged commit 6b1e7523ed into main 2026-06-09 13:41:41 +00:00

zombor referenced this pull request from a commit 2026-06-09 13:41:41 +00:00

Merge pull request 'feat(auth): RBAC permission middleware + nav gating + bookdrop gating (bookshelf-4op.3.1)' (#458) from bd-bookshelf-4op.3.1 into main

zombor referenced this pull request 2026-06-09 14:01:37 +00:00

RBAC: book write gating (bookshelf-4op.3.2) #460

zombor referenced this pull request from a commit 2026-06-09 17:52:44 +00:00

feat(books): gate POST /books/{id}/files upload route with CAN_UPLOAD

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

No description provided.