feat(api/v2): gate admin routes by feature + instance admin#2817
Conversation
github-actions
Bot
added
area/api
Preview DeploymentPreview deployments for this PR are available at:
The preview environment will start automatically on first visit. Subsequent pushes to this PR will update the Run locally with Dockerdocker pull ghcr.io/go-vikunja/vikunja:pr-2817
docker run -p 3456:3456 ghcr.io/go-vikunja/vikunja:pr-2817Last updated for commit 6076102 |
|
auggie review |
🤖 Augment PR SummarySummary: Adds an admin/license gate for v2 admin routes and introduces the first gated v2 admin endpoint. Changes:
Technical Notes: The gate reuses the existing v1 middleware unchanged to preserve semantics (feature check first, then admin check) and keep admin endpoints in the unified v2 OpenAPI spec. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| // The admin gate must run after the token middleware (it reads the | ||
| // authenticated user from the JWT claims) and is scoped by path so only | ||
| // /api/v2/admin/* is gated. | ||
| a.Use(gateV2AdminRoutes()) |
There was a problem hiding this comment.
pkg/routes/routes.go:419: gateV2AdminRoutes() is registered before setupRateLimit/setupMetricsMiddleware, so requests rejected by the admin/license gate won’t be rate-limited or measured (unlike v1 admin routes which sit behind the parent group’s rate limit). Was that intentional given RequireInstanceAdmin does a DB read per request?
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Implemented in fce8de0 — reordered the v2 middleware so the admin gate runs after setupRateLimit and setupMetricsMiddleware (still after the token middleware): token, rate limit, metrics, gate. A flood to /api/v2/admin/* is now rate-limited and measured before the per-request RequireInstanceAdmin DB read.
| assert.Contains(t, body, `"total":`) | ||
| // Project 6 is owned by user6, not shared with user1 — the admin list | ||
| // surfaces it regardless of ownership. | ||
| assert.Contains(t, body, `"id":6`) |
There was a problem hiding this comment.
pkg/webtests/huma_admin_test.go:81: assert.Contains(body, "\"id\":6") can false-pass (e.g. it also matches "id":60) and doesn’t guarantee the ID appears in the items list. A structured JSON assertion would make this test stricter and less brittle.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Implemented in 0eb4350 — replaced the substring check with a structured assertion that unmarshals the paginated envelope and confirms id 6 (and 22) appear in items.
| t.Run("admin without the feature gets 404", func(t *testing.T) { | ||
| e, err := setupTestEnv() | ||
| require.NoError(t, err) | ||
| license.ResetForTests() |
There was a problem hiding this comment.
pkg/webtests/huma_admin_test.go:57: this subtest calls license.ResetForTests() without a deferred reset, so future early exits or parallelization could let license state bleed into other tests. Keeping the same set/defer ResetForTests() pattern as the other subtests would make isolation clearer.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Implemented in 1c2b2db — switched to the sibling pattern (license.SetForTests(...) + defer license.ResetForTests()) so license state cannot bleed into other tests.
tink-bot
force-pushed
the
feat-v2-admin-gate
branch
from
0d79624 to
78e38d9
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Add the admin + license gate for /api/v2 and ship the first gated resource, GET /api/v2/admin/projects (AdminProjectList). The gate reuses the existing v1 middleware functions unchanged — RequireFeature(license.FeatureAdminPanel) and RequireInstanceAdmin(), both of which serve 404 on failure. Rather than splitting the single v2 Huma API into a separate gated sub-group (which would split the OpenAPI spec and drop admin operations from /api/v2/openapi.json), the gate is applied as a path-scoped Echo middleware on the shared /api/v2 group, firing only for /api/v2/admin/* and after the token middleware. This preserves v1's 404-not-403 semantics and keeps admin routes in the unified v2 spec and Scalar docs. AdminProjectList lists every project on the instance (archived included), behind the gate. Adds doc:/readOnly: tags to the shared Project model so it documents correctly as a v2 schema. Tests in pkg/webtests/huma_admin_test.go (TestHumaAdminProjects) cover all three personas: non-admin -> 404, admin without feature -> 404, admin with feature -> 200 list, plus unauthenticated -> 401.
tink-bot
force-pushed
the
feat-v2-admin-gate
branch
from
78e38d9 to
bf64abc
Compare
2 participants
What this implements
The admin + license gate for
/api/v2(which did not exist yet) and the first gated resource,GET /api/v2/admin/projects(AdminProjectList).pkg/routes/routes.go): reuses the existing v1 middleware functions unchanged —RequireFeature(license.FeatureAdminPanel)(pkg/routes/feature_gate.go) andRequireInstanceAdmin()(pkg/routes/admin_gate.go). Both serve 404 on failure, so gated routes are indistinguishable from unregistered ones. v1 semantics preserved exactly.pkg/routes/api/v2/admin_projects.go):GET /api/v2/admin/projectslists every project on the instance (archived included), behind the gate. Standard v2 list handler mirroringlabels.go— type-assertsDoReadAll's result to[]*models.Projectwith anokcheck, wraps inNewPaginated, routes errors throughtranslateDomainError. No permission re-check in the handler (the gate owns authorization;AdminProjectList.ReadAllalready returns all projects).pkg/models/project.go): addeddoc:/readOnly:tags to the sharedProjectstruct so it documents correctly as a v2 schema (the first timeProjectis exposed via v2). Inert for v1 (swaggo/XORM/govalidator ignore the tags).pkg/webtests/huma_admin_test.go,TestHumaAdminProjects): non-admin -> 404; admin without the feature -> 404; admin with the feature -> 200 list (asserts a non-owned project Importing from Trello does not work properly #6 and an archived project Special characters in VIKUNJA_DATABASE_PASSWORD #22 appear); unauthenticated -> 401. RFC 9457 error body is not re-asserted (covered globally inTestHuma_ErrorShapeIsRFC9457).Plan:
plans/v2-phase1-pr3-admin-license-gate.mdKey architectural DECISION (the plan left this open)
Where does the gate live? The plan posed two options: (a) an Echo sub-group under
/api/v2/admincarrying the existing middlewares, with admin Huma operations mounted onto it; (b) a Huma-side middleware/resolver.Recon of
pkg/modules/humaecho5/humaecho5.goshowed the adapter mounts a Huma API onto a single Echo router/group; per-operation Echo middleware is not threaded throughHandle.NewWithGroupcould mount a second Huma API on a gated/api/v2/adminsub-group — but that splits the OpenAPI spec: admin operations would no longer appear in the main/api/v2/openapi.json/ Scalar docs, and would need their own spec-serving wiring.Chosen approach: a single Huma API, with the gate applied as a path-scoped Echo middleware on the shared
/api/v2group (gateV2AdminRoutes). It composes the two existing middleware functions and fires only when the request path starts with/api/v2/admin, otherwise passing through. It is registered afterSetupTokenMiddleware()so the authenticated user is available toRequireInstanceAdmin(which re-readsis_adminfrom the DB).Tradeoffs:
registerAPIRoutesV2.Flag for reviewers: please confirm the path-scoped-middleware choice over a separate gated Huma API. This is the load-bearing call and the riskiest part of the PR.
Test output
TestHumaAdminProjects— all 4 subtests pass.mage lintclean. (Run directly viago test -run '^TestHumaAdminProjects$' ./pkg/webtests/becausemage test:filterpasses-short, whichpkg/webtests/TestMainuses to skip the whole package.)Autonomous first-pass implementation (v2 Phase-1 pattern PR). Draft for review.