Extract internal/access.Checker (H-11) #105

Merged

qwc merged 1 commit from refactor/access-checker into main

2026-05-18 20:02:11 +02:00

qwc commented

2026-05-18 19:53:51 +02:00

Owner

Summary

Workstream item #10. Audit finding H-11 (High) — authz helpers can't be unit-tested in isolation.

canViewProject, canUpload, filterAccessibleProjects lived as methods on Handler and took *Handler as receiver. Each consulted the stores directly. Testing required a full Handler with five stores + a logger, and the rules had already drifted across these three methods plus the inline copies in project.go (the #94 root cause and the audit's H-1/H-2).

Changes

New internal/access package with Checker:
- CanView(ctx, user, project) bool
- CanUpload(ctx, user, project) bool
- FilterAccessible(ctx, user, []Project) []Project
Pure code-move: rules preserved byte-for-byte. Including the M-2 upload-without-view asymmetry (documented in CanUpload's godoc; no product change here).
Handler wiring: Checker constructed in handler.New from existing Deps. The three handler methods become one-line delegations to h.checker.CanView / h.checker.CanUpload / h.checker.FilterAccessible, so all 17 existing call sites are untouched.
24 test cases across TestCanView, TestCanUpload, TestFilterAccessible. Anonymous + every role/visibility/grant combination, including the per-project-grant-on-private edge that PR #95 wants to flip (currently pinned to pre-#95 behavior so the test will alert when #95 lands).

Test plan

go test ./... passes
New Checker tests cover the full matrix
Existing handler tests still pass — the thin forwarders preserve every call site's behavior

Follow-up that this unblocks

H-1: handleDeleteVersion can call h.canUpload(...) instead of the inline ad-hoc check (one-line fix).
H-2: handleProjectDetail's CanUpload/CanDelete template flag becomes the same call.
PR #95: still applies cleanly — the rule change lives in internal/access/checker.go now (one place, not three).

🤖 Generated with Claude Code

## Summary Workstream item #10. Audit finding **H-11 (High)** — authz helpers can't be unit-tested in isolation. `canViewProject`, `canUpload`, `filterAccessibleProjects` lived as methods on `Handler` and took `*Handler` as receiver. Each consulted the stores directly. Testing required a full Handler with five stores + a logger, and the rules had already drifted across these three methods plus the inline copies in `project.go` (the #94 root cause and the audit's H-1/H-2). ## Changes - **New `internal/access` package** with `Checker`: - `CanView(ctx, user, project) bool` - `CanUpload(ctx, user, project) bool` - `FilterAccessible(ctx, user, []Project) []Project` - **Pure code-move**: rules preserved byte-for-byte. Including the M-2 upload-without-view asymmetry (documented in CanUpload's godoc; no product change here). - **Handler wiring**: `Checker` constructed in `handler.New` from existing `Deps`. The three handler methods become one-line delegations to `h.checker.CanView` / `h.checker.CanUpload` / `h.checker.FilterAccessible`, so all 17 existing call sites are untouched. - **24 test cases** across `TestCanView`, `TestCanUpload`, `TestFilterAccessible`. Anonymous + every role/visibility/grant combination, including the per-project-grant-on-private edge that PR #95 wants to flip (currently pinned to pre-#95 behavior so the test will alert when #95 lands). ## Test plan - [x] `go test ./...` passes - [x] New Checker tests cover the full matrix - [x] Existing handler tests still pass — the thin forwarders preserve every call site's behavior ## Follow-up that this unblocks - **H-1**: `handleDeleteVersion` can call `h.canUpload(...)` instead of the inline ad-hoc check (one-line fix). - **H-2**: `handleProjectDetail`'s `CanUpload`/`CanDelete` template flag becomes the same call. - **PR #95**: still applies cleanly — the rule change lives in `internal/access/checker.go` now (one place, not three). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

qwc added 1 commit

2026-05-18 19:53:51 +02:00

Extract internal/access.Checker (H-11)

CI / test (pull_request) Successful in 1m18s

Details

CI / build (pull_request) Successful in 56s

Details

CI / docker (pull_request) Has been skipped

Details

a8b1712527

Audit finding H-11. The three authorization helpers — canViewProject,
canUpload, filterAccessibleProjects — lived as methods on Handler and
took *Handler as receiver. Each consulted ProjectAccessStore +
GlobalAccessStore directly. Three consequences:

  - Tests of the authorization rules needed a full Handler with five
    stores + a logger to construct, which discouraged exhaustive case
    coverage.
  - The view / upload rules drifted across the three methods plus
    inline copies in project.go (#94 and the audit's H-1/H-2).
  - Future structural splits (PublicHandler vs AdminHandler from the
    architecture audit) had no clean seam.

Move all three to a new internal/access package:

  type Checker struct { access, globalAccess, logger }
  func NewChecker(...)
  func (c *Checker) CanView(ctx, user, project) bool
  func (c *Checker) CanUpload(ctx, user, project) bool
  func (c *Checker) FilterAccessible(ctx, user, []Project) []Project

This is a pure code-move: rules preserved byte-for-byte from the prior
implementation, including the upload-without-view asymmetry the audit
flagged as M-2 (it's documented in CanUpload's godoc). PR #95 still
applies on top.

Wiring: Handler.checker is constructed in handler.New from the existing
Deps (Access + GlobalAccess + Logger). The three handler methods
(h.canViewProject, h.canUpload, h.filterAccessibleProjects) become
one-line delegations, keeping all 17 existing call sites unchanged and
the diff small.

Tests:
  - TestCanView (13 cases) covers anonymous + every role/visibility/
    grant combination, including the per-project-grant-on-private edge
    that PR #95 wants to flip (test currently pins the pre-#95
    behavior).
  - TestCanUpload (10 cases) covers the same matrix from the upload
    side, with explicit notes on the M-2 asymmetry.
  - TestFilterAccessible verifies the batch form returns the same
    subset CanView would on each project.

Net handler diff: -85 / +12 lines.

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

qwc merged commit 81379f4a83 into main

2026-05-18 20:02:11 +02:00

qwc deleted branch refactor/access-checker

2026-05-18 20:02:11 +02:00

qwc referenced this pull request from a commit

2026-05-18 20:02:12 +02:00

Merge pull request 'Extract internal/access.Checker (H-11)' (#105) from refactor/access-checker into main

qwc referenced this pull request from a commit

2026-05-18 20:07:14 +02:00

Fix H-1/H-2 inline authz copies; H-3 reject project-scoped tokens on POST /api/projects

qwc referenced this pull request

2026-05-18 20:07:28 +02:00