Skip to content

fix(hub): add /api/v1/broker/callback route - #1436

Closed
ptone wants to merge 2 commits into
mainfrom
fix/dead-route-broker-callback
Closed

fix(hub): add /api/v1/broker/callback route#1436
ptone wants to merge 2 commits into
mainfrom
fix/dead-route-broker-callback

Conversation

@ptone

@ptone ptone commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add handleBrokerCallback handler for POST /api/v1/broker/callback — the endpoint the Teams broker plugin's HubClient.DeliverCallback() calls to deliver interactive card responses and action acknowledgements.
  • Register the route in server.go with broker HMAC auth (same pattern as /api/v1/broker/inbound), add route metadata and classification entries.
  • Delete the mock-based TestHubClient_DeliverCallback test that asserted against the non-existent route, and replace it with 5 server-side handler tests exercising the real route through the hub mux.

Design decision: Option A (new dedicated handler)

The callback payload (Data map[string]interface{}) is structurally different from the inbound message format (Topic + *messages.StructuredMessage). Routing callback data through handleBrokerInbound would fail validation, so a dedicated handler is the correct approach.

Test plan

  • go build ./... passes
  • go vet ./... passes
  • go test ./extras/scion-teams/... passes (mock test removed)
  • go test ./pkg/hub/ -run TestHandleBrokerCallback — 5 tests pass (success, no auth, method not allowed, nil data, invalid JSON)
  • go test ./pkg/hub/ -run TestHandleBrokerInbound — existing inbound tests still pass
  • go test ./pkg/hub/ -run TestRegisteredRoutesHavePermissionClassification — route metadata test passes

The Teams broker plugin's HubClient.DeliverCallback() sends callback
data (e.g. interactive card responses, action acknowledgements) to
POST /api/v1/broker/callback, but no handler existed on the hub server
— the delivery silently failed.

Add handleBrokerCallback with broker HMAC authentication (same auth
pattern as handleBrokerInbound), register the route, and add route
metadata. The callback payload (generic map[string]interface{} data) is
semantically different from inbound messages (structured Topic + Message),
so a dedicated handler is the correct approach.

Delete the mock-based TestHubClient_DeliverCallback test that asserted
against the non-existent route. Replace it with server-side handler tests
that exercise the real route through the hub mux, covering success,
auth failure, method not allowed, nil data, and invalid JSON cases.
@ptone

ptone commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Code Review: APPROVE ✅

Risk: LOW — Adds dedicated handleBrokerCallback handler for POST /api/v1/broker/callback.

(Posted as comment because the token cannot self-approve.)

Verified

  • ✅ Mock TestHubClient_DeliverCallback deleted in same commit as handler addition
  • ✅ Route registered in server.go with guarded() wrapper (same pattern as broker/inbound)
  • ✅ Auth via broker HMAC (GetBrokerIdentityFromContext) — matches handleBrokerInbound
  • ✅ POST-only enforcement with MethodNotAllowed for other methods
  • ✅ Route metadata and classification entries added, consistent with adjacent broker routes
  • ✅ Client callbackPayload{Data} matches server brokerCallbackRequest{Data} exactly
  • ✅ 5 handler tests through real mux: success, no-auth (401), method-not-allowed (405), nil data (400), invalid JSON (400)
  • go build and go vet pass
  • ✅ No cross-contamination with inbound message handling

Optional observation (non-blocking)

The handler currently logs the callback and returns {"received": true} without dispatching to any downstream consumer. Presumably intentional as a first step to stop 404s from the broker plugin. A follow-up to wire actual dispatch logic would be expected.

Clean change. Correct architectural decision to use a dedicated handler rather than overloading handleBrokerInbound.

@ptone ptone closed this Sep 4, 2026
@ptone
ptone deleted the fix/dead-route-broker-callback branch September 4, 2026 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant