feat: tickets endpoint + N-scopes-per-key auth refactor (v2.2.0)#53
Merged
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- ApiKeyResult.ScopeRead -> Scopes map[string]struct{} + HasScope()
- ValidateApiKey rewritten to JOIN xf_cav7_api_key_scope_def and _scope
- gRPC interceptor attaches *ApiKeyResult to handler ctx, drops ScopeRead gate
- HTTP middleware drops ScopeRead gate (validation still fail-fast on bad tokens)
- All milpacs handlers explicitly RequireScope(ctx, "read")
- Adds RequireScope + KeyFromContext + ContextWithKey helpers under servers/grpc/
Coordinated with Cav7/ApiKeyManager 1.1.0; see addon migration notes.
Holds status/priority/prefix phrase names + the category tree. Refresh is loader-driven (the loader implementation lives in the datastore package next). Cache miss returns empty/nil — never an error — because the cache can be cold relative to addon-side additions.
Reads ticket status/priority/prefix phrase names from xf_phrase and the full category tree from xf_nf_tickets_category. Caller (referencecache.Cache) calls these at startup and every refresh tick.
Fails fast if the initial load errors (DB unreachable, schema mismatch). After startup the refresh goroutine ticks every 15min (configurable via REFERENCE_CACHE_REFRESH_INTERVAL env var); refresh failures log but never blank the cache.
- Replace fmt.Sscanf with strconv.ParseUint for stricter id parse - Add cycle protection to CategoryAncestors traversal - Thread context.Context through refresh goroutine for future cancellation
Mirrors milpacs.proto convention so Swagger UI renders the Authorize button for tickets and each RPC shows a summary + description + tag group instead of just the operationId.
- gorm.ErrRecordNotFound -> codes.NotFound (matches MilpacsService convention) - Other datastore errors -> codes.Internal with operation context - Strengthen TestListTickets_HappyPath to assert all 11 filter fields - Add TestGetTicket_NotFoundReturns404Code
- servers/server.go const version + proto/milpacs.proto openapiv2_swagger.info.version bumped lockstep per CLAUDE.md - proto/tickets.proto already declares 2.2.0 (set in Phase 3) - make generate output includes catch-up bytes for milpacs.pb.go + third_party/OpenAPI/milpacs.swagger.json which had pre-existing drift (stuck at 1.7.x) — the catch-up isn't new work, just the regeneration finally syncing them to the proto source
Adds a case-insensitive Bearer-scheme parser in datastores so the gRPC interceptor and HTTP middleware can share one implementation. RFC 7235 says auth-scheme tokens are case-insensitive; lowercase 'bearer' previously returned 401 (caught as G7 in the 2026-05-16 staging smoke test). Call-site swap follows in the next commit.
Replaces the inline TrimPrefix with the case-insensitive helper from the previous commit. No functional change beyond G7's case-sensitivity fix.
Closes G7 from the 2026-05-16 staging smoke test — lowercase 'bearer' Authorization headers now authenticate correctly. Both auth surfaces share one parser, eliminating the lockstep-or-drift footgun called out in CLAUDE.md.
decodeCursor now wraps a sentinel error so the handler layer can map malformed cursors to codes.InvalidArgument. Handler mapping follows in the next commit.
Closes C3 from the 2026-05-16 staging smoke test — malformed after_cursor on /api/v1/tickets now returns HTTP 400 instead of 500. Detected via errors.Is on the ErrInvalidCursor sentinel introduced in the previous commit.
Adds doc-comments to all six repeated filters on ListTicketsRequest so the OpenAPI page surfaces the supported multi-select form. Closes A4 from the 2026-05-16 staging smoke test — comma-separated form (?status_id=1,3) was never wired and the parent spec wrongly promised it would work. Aligning the contract with reality is the YAGNI fix; a custom unmarshaller is deferred until a hand-typed-URL consumer asks for it.
Closes E1 from the 2026-05-16 staging smoke test. - Adds total_message_count to Ticket (derived from reply_count + 1, no extra query). Both ListTickets and GetTicket responses carry it. - Deprecates GetTicketResponse.total_message_count (use ticket.total_message_count). Removal target: v2.3.0. - Switches ListTicketMessages cursor from uint32 after_position to opaque string after_cursor + base64-encoded position. Closes the proto3-zero-value gap that made position=0 messages unreachable (smoke run: ticket 6899 paginated 80/81 messages). - Maps ErrInvalidCursor → codes.InvalidArgument on ListTicketMessages to match ListTickets. Field-number reuse on ListTicketMessagesRequest.after_cursor and ListTicketMessagesResponse.next_cursor is intentional and safe: the previous uint32 types never shipped — v2.2.0 is unreleased.
fmt.Sscanf accepts a leading numeric prefix and silently discards the
rest, so base64('42abc') decoded to (pos=42, nil) instead of erroring.
Replaces both decodeCursor and decodeMessageCursor with strconv.ParseUint
which is strict.
Adds trailing-garbage test cases to both garbage suites and a
position=0 round-trip test that pins the regression target from
smoke ticket 6899 (the original reason for the opaque-cursor switch
in the previous commit).
Smoke-test on staging caught that the prior opaque-cursor fix only addressed the wire-format zero-value collapse; the underlying SQL boundary `position > afterPos` still excluded position=0 on the empty- cursor first page. Confirmed on ticket 6899 — paginated 80/81 again. - SQL boundary changed to `position >= ?` so afterPos=0 (empty cursor) includes the starter post. - Cursor semantic shifts: encodes the NEXT position to include (last_returned + 1) rather than the last position returned. Round- trip property unchanged; consumers treat the cursor as opaque. - Adds TestListTicketMessages_EmptyCursorReachesPositionZero pinning the regression target.
This was referenced May 19, 2026
SyniRon
added a commit
that referenced
this pull request
May 20, 2026
#81) Eliminates the manual two-place version bump that's been a recurring near-miss at release time (the 2.2.0 deploy nearly shipped with the const unbumped; only caught because PR #53 happened to include it). Changes: - servers/server.go: `const version = "2.2.0"` → `var version = "dev"` so -ldflags can override at link time. - proto/milpacs.proto + proto/tickets.proto: openapiv2_swagger info.version literal switched to "dev" sentinel. - Dockerfile: accepts `ARG VERSION=dev`, passes it through `-ldflags="-X github.com/7cav/api/servers.version=${VERSION}"`. - .github/workflows/build_and_push.yml: new step that sed-substitutes `${{ github.ref_name }}` into both .proto files before docker build, and passes the same value through as a build-arg. Both the runtime server log and the OpenAPI spec at `/` will report the released tag on production builds; local dev builds continue to report "dev". Mirrors the ldflags-injection pattern cavbot2 adopted in 0.7.4. Verified locally: - `go build` → "Starting 7Cav API version: dev" - `go build -ldflags=...` → "Starting 7Cav API version: 2.2.1-test" - `make generate` clean with the "dev" sentinel; openapi JSONs render `"version": "dev"` as expected. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ships v2.2.0 of the 7Cav API: scope-based auth aligned with
Cav7/ApiKeyManager1.1.0 + a new read-only tickets endpoint backed by thexf_nf_tickets_*XenForo addon.GET /api/v1/tickets/*) backed by the nixfifty XenForo addon. Five RPCs:ListTickets,GetTicket,GetTicketByRef,ListTicketMessages,ListCategories.Cav7/ApiKeyManager1.1.0 auth refactor:xf_cav7_api_key.scope_readboolean is replaced with a scope catalog (xf_cav7_api_key_scope_def+xf_cav7_api_key_scopejunction). Each handler explicitly declares its required scope viaRequireScope(ctx, "<name>")—"read"for milpacs,"read:tickets"for the new service.REFERENCE_CACHE_REFRESH_INTERVAL).monitoredTablesadditions — intentional; the spec covers why)._test.gofiles in the repo — 28 tests covering the new code paths (auth refactor + reference cache + tickets datastore + tickets handlers + smoke-fix coverage).FORUM_BASE_URL(optional; populatesTicket.forum_url) andREFERENCE_CACHE_REFRESH_INTERVAL(optional, Go duration, default15m).Smoke-fix batch (added 2026-05-17)
After the initial PR opened, the branch was deployed to api-staging on traycer and a full smoke matrix was run. Four code/spec gaps surfaced; this PR now also contains the fixes for all of them, plus two reviewer-driven fixups.
Authorization: bearer <token>(lowercase scheme) returned 401datastores.ParseBearerTokenhelper, case-insensitive per RFC 7235. Both gRPC interceptor and HTTP middleware use it.after_cursorreturned HTTP 500datastores.ErrInvalidCursorsentinel; handlers map viaerrors.Is→codes.InvalidArgument→ HTTP 400 with message"invalid after_cursor".ListTicketsRequestcarry doc-comments surfaced as OpenAPI parameter descriptions. Custom unmarshaller deferred.total_message_countonTicket; messages atposition=0were unreachableTicket.total_message_count = reply_count + 1(no extra query).ListTicketMessagesswitched to opaque base64 string cursor. SQL boundary tightened fromposition > ?toposition >= ?(caught during re-smoke) with cursor semantic shifted to "next position to include".Reviewer-driven fixups
decodeCursoranddecodeMessageCursorusestrconv.ParseUintinstead offmt.Sscanf(rejects trailing garbage likebase64("42abc")).GetTicketResponse.total_message_count(field 3) marked[deprecated = true]with removal target v2.3.0; still populated through v2.2.0 as a one-release migration runway. Consumers should switch toTicket.total_message_count(newly available on every Ticket-returning endpoint).API contract caveats
ListTicketMessagescursor type change (uint32 after_position→string after_cursor, both request and response). Wire-incompat in isolation but v2.2.0 is unreleased and the known consumers (7cav-cavbot2,7cav-adr) don't call this RPC. Field-number reuse on request field 3 and response field 2 is intentional and safe — the previous types never shipped.Authorization: bearer <token>now succeeds where previously it returned 401. Tightening side-effect: a bareAuthorization: <token>with NO scheme prefix is now rejected (the old code silently no-op'dTrimPrefixand accepted bare tokens). No known consumer sent bare tokens.after_cursormalformed input → HTTP 400 instead of 500. Behavioral change in the wrong-input direction; consumers that special-cased 500 will now miss the InvalidArgument signal (none known to do this).Coordination — must read before deploying
This PR is deploy-gated against
Cav7/ApiKeyManager1.1.0. The addon's migration drops thescope_readcolumn and creates the two new scope tables. The auth query in this PR references the post-migration schema; the old query referenced the column that's about to be dropped. There is no overlap window where both queries work.Recommended sequence:
develop; let the next release cut produce the v2.2.0 image.Cav7/ApiKeyManagerto 1.1.0 on the XF board (xf-addon:rebuild Cav7/ApiKeyManager). Migration: scope_def + junction tables created, every active key backfilled withread,scope_readcolumn dropped,read:ticketsscope row seeded with permission gating.read:ticketsto the MCP server's API key via the addon ACP.Brief downtime alternative: skip the gate, do steps 2+3 back-to-back.
Test plan
go test -vet=off ./...green locally (pre-existingFatalfformat-string warnings inservers/server.go:139/183/191are out of scope; will file a separate maintenance ticket)make lintgreen (buf lint+buf breaking --against develop, exit 0)~/repos/xenforo-db: ListTickets with category subtree expansion returns S6 tickets, GetTicketByRef populatesforum_url, scope rejection returns 403 for read-only keys on tickets routes, milpacs routes still work for both full-access and read-only keys, cache bypass confirmed via server log diffd9212d4):bearer, ALL-CAPSBEARER, canonicalBearerall → 200; no-scheme prefix → 401?after_cursor=garbage-not-base64→ 400{"code":3,"message":"invalid after_cursor"}; trailing-garbage cursor → 400GET /tickets/6899returnstotal_message_count: 81,reply_count: 80;/messagesfirst page positions[0, 1, 2]; full pagination yields exactly 81 messages (previously stopped at 80 due to unreachableposition=0)Notes for reviewers
go.sumhas a 422-line reduction fromgo mod tidycleaning up stale transitive entries left by previous PRs. Not part of this PR's work; happened during dep refresh in the first commit (77117f8).proto/milpacs.pb.goandthird_party/OpenAPI/milpacs.swagger.jsonwhich had pre-existing drift (stuck at 1.7.x while the proto declared 2.1.1). The catch-up isn't new work — just the regeneration finally syncing to the proto source.TicketsServiceerror mapping (errors.Is(err, gorm.ErrRecordNotFound)→codes.NotFound, others →codes.Internal) is stricter thanMilpacsService, which currently maps any non-nil error tocodes.NotFound. Filed as a follow-up to align milpacs to the new pattern; not addressed in this PR to keep the diff focused.Fatalfformat-string vet warnings atservers/server.go:139,183,191are flagged for a separate maintenance follow-up; they are NOT a regression from this PR.🤖 Generated with Claude Code