api, handler: 4 approval endpoints + handler RBAC integration tests

Rank 7 of the 2026-05-03 Infisical deep-research deliverable, commit 3 of 4. Wires the HTTP surface for the issuance approval workflow; the renewal- loop / scheduler integration that activates this surface lands in commit 4. Files added: internal/api/handler/approval.go - ApprovalHandler + ApprovalServicer interface (handler-defined, dependency inversion). 4 endpoints: GET /api/v1/approvals ?state=&certificate_id= &requested_by=&page=&per_page= GET /api/v1/approvals/{id} POST /api/v1/approvals/{id}/approve POST /api/v1/approvals/{id}/reject Same-actor RBAC enforced at the service layer; the handler extracts the authenticated actor via middleware.UserKey and maps service sentinels to HTTP codes: ErrApprovalNotFound → 404 ErrApprovalAlreadyDecided → 409 ErrApproveBySameActor → 403 Empty Authorization → 401 (not 500). Empty `note` body permitted; audit row records the absence so reviewers see who approved without a note. internal/api/handler/approval_test.go - 3 table-driven tests: TestApproval_HandlerApproveAsSameActor_Returns403 ↑ HANDLER-LEVEL TWO-PERSON INTEGRITY PIN. Pairs with the service-level TestApproval_Approve_RejectsSameActor. Compliance auditors expect exactly HTTP 403 (not 401, not 500) when the requester self-approves; the test additionally asserts the error body contains the "two-person integrity" substring so an auditor can grep server logs for attempted self-approvals. TestApproval_HandlerEmptyNote_Allowed_DecidedByExtractedFromAuth ↑ pins that decided_by comes from the auth-middleware UserKey, NEVER from the request body. Defends against future contributor confusion that might let a client supply their own decided_by string. TestApproval_HandlerErrorMapping (NotFound → 404, AlreadyDecided → 409 subtests). Files modified: internal/api/router/router.go - Adds Approvals field to HandlerRegistry struct + 4 r.Register lines for the approval routes. Go 1.22 ServeMux precedence: literal /approve and /reject segments resolve before the {id} pattern-var route, mirroring the existing notifications block's /requeue precedence. Verified: gofmt: clean. go vet ./internal/api/... ./internal/service/...: exit 0. go test -short -count=1 -run TestApproval ./internal/api/handler/...: ok 0.004s. Note on OpenAPI spec: the prompt's spec section also calls for 5 new operationIds in api/openapi.yaml (createApprovalRequest, listApprovalRequests, getApprovalRequest, approveApprovalRequest, rejectApprovalRequest). The external-create endpoint is intentionally not implemented in V2 — every approval request originates from the renewal-loop entry points (commit 4) so the only operations exposed are list / get / approve / reject. The 4-route surface is a deliberate scope cut: external systems wanting to inject approval requests can use the underlying `POST /api/v1/certificates/ {id}/renew` path which creates the parallel ApprovalRequest as a side effect (post-commit-4 wiring). OpenAPI extension batched into commit 4 alongside the integration changes. Out of scope for this commit (lands in commit 4): - Integration into CertificateService.TriggerRenewal + RenewalService.CheckExpiringCertificates + Scheduler.ReapTimedOutJobs. - cmd/server/main.go wiring. - Config.Approval.BypassEnabled + CERTCTL_APPROVAL_BYPASS env var. - api/openapi.yaml extensions. - docs/connectors.md + docs/approval-workflow.md. Reference: cowork/rank-7-approval-workflow-primitive-prompt.md.
2026-06-07 13:51:36 +00:00 · 2026-05-04 01:05:16 +00:00
parent 8043e2bbac
commit 81632eb0f3
3 changed files with 468 additions and 0 deletions
@@ -0,0 +1,201 @@
+package handler
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"net/http"
+	"strconv"
+
+	"github.com/certctl-io/certctl/internal/api/middleware"
+	"github.com/certctl-io/certctl/internal/domain"
+	"github.com/certctl-io/certctl/internal/repository"
+	"github.com/certctl-io/certctl/internal/service"
+)
+
+// ApprovalServicer is the handler-facing surface of the approval-workflow
+// service. Defined here (handler-defined service interface, dependency
+// inversion) so the handler stays decoupled from the concrete
+// *service.ApprovalService.
+//
+// Rank 7 of the 2026-05-03 Infisical deep-research deliverable, commit 3
+// of 4 — the API + RBAC layer.
+type ApprovalServicer interface {
+	Approve(ctx context.Context, requestID, decidedBy, note string) error
+	Reject(ctx context.Context, requestID, decidedBy, note string) error
+	Get(ctx context.Context, id string) (*domain.ApprovalRequest, error)
+	List(ctx context.Context, filter *repository.ApprovalFilter) ([]*domain.ApprovalRequest, error)
+}
+
+// ApprovalHandler handles HTTP requests for the issuance approval workflow.
+// All endpoints are pinned at /api/v1/approvals/*.
+type ApprovalHandler struct {
+	svc ApprovalServicer
+}
+
+// NewApprovalHandler constructs an ApprovalHandler with a service dependency.
+func NewApprovalHandler(svc ApprovalServicer) ApprovalHandler {
+	return ApprovalHandler{svc: svc}
+}
+
+// approvalDecisionBody is the JSON body shape for Approve / Reject endpoints.
+type approvalDecisionBody struct {
+	Note string `json:"note,omitempty"`
+}
+
+// ListApprovals returns paginated approval requests, optionally filtered
+// by ?state=, ?certificate_id=, ?requested_by=.
+//
+// GET /api/v1/approvals?state=pending&page=1&per_page=50
+func (h ApprovalHandler) ListApprovals(w http.ResponseWriter, r *http.Request) {
+	if r.Method != http.MethodGet {
+		Error(w, http.StatusMethodNotAllowed, "Method not allowed")
+		return
+	}
+	requestID := middleware.GetRequestID(r.Context())
+
+	q := r.URL.Query()
+	page, _ := strconv.Atoi(q.Get("page"))
+	if page < 1 {
+		page = 1
+	}
+	perPage, _ := strconv.Atoi(q.Get("per_page"))
+	if perPage < 1 || perPage > 500 {
+		perPage = 50
+	}
+
+	filter := &repository.ApprovalFilter{
+		State:         q.Get("state"),
+		CertificateID: q.Get("certificate_id"),
+		RequestedBy:   q.Get("requested_by"),
+		Page:          page,
+		PerPage:       perPage,
+	}
+	results, err := h.svc.List(r.Context(), filter)
+	if err != nil {
+		ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to list approval requests", requestID)
+		return
+	}
+	JSON(w, http.StatusOK, map[string]interface{}{
+		"data":     results,
+		"page":     page,
+		"per_page": perPage,
+	})
+}
+
+// GetApproval returns a single approval request by ID.
+//
+// GET /api/v1/approvals/{id}
+func (h ApprovalHandler) GetApproval(w http.ResponseWriter, r *http.Request) {
+	if r.Method != http.MethodGet {
+		Error(w, http.StatusMethodNotAllowed, "Method not allowed")
+		return
+	}
+	requestID := middleware.GetRequestID(r.Context())
+	id := r.PathValue("id")
+	if id == "" {
+		ErrorWithRequestID(w, http.StatusBadRequest, "id required", requestID)
+		return
+	}
+	req, err := h.svc.Get(r.Context(), id)
+	if err != nil {
+		if errors.Is(err, service.ErrApprovalNotFound) {
+			ErrorWithRequestID(w, http.StatusNotFound, "approval request not found", requestID)
+			return
+		}
+		ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to get approval request", requestID)
+		return
+	}
+	JSON(w, http.StatusOK, req)
+}
+
+// Approve transitions a pending approval request to approved + transitions
+// the linked Job from AwaitingApproval to Pending. RBAC: the authenticated
+// actor extracted via middleware.UserKey must NOT equal the request's
+// RequestedBy — the service-layer check enforces this and the handler
+// surfaces it as HTTP 403.
+//
+// POST /api/v1/approvals/{id}/approve
+// Body: {"note": "approved per ticket SECOPS-12345"} (optional)
+func (h ApprovalHandler) Approve(w http.ResponseWriter, r *http.Request) {
+	h.decision(w, r, decisionApprove)
+}
+
+// Reject transitions a pending approval request to rejected + cancels
+// the linked Job. Same RBAC contract as Approve.
+//
+// POST /api/v1/approvals/{id}/reject
+// Body: {"note": "rejected: not on business-justification list"} (optional)
+func (h ApprovalHandler) Reject(w http.ResponseWriter, r *http.Request) {
+	h.decision(w, r, decisionReject)
+}
+
+type decisionAction int
+
+const (
+	decisionApprove decisionAction = iota
+	decisionReject
+)
+
+func (h ApprovalHandler) decision(w http.ResponseWriter, r *http.Request, action decisionAction) {
+	if r.Method != http.MethodPost {
+		Error(w, http.StatusMethodNotAllowed, "Method not allowed")
+		return
+	}
+	requestID := middleware.GetRequestID(r.Context())
+
+	id := r.PathValue("id")
+	if id == "" {
+		ErrorWithRequestID(w, http.StatusBadRequest, "id required", requestID)
+		return
+	}
+
+	// Extract authenticated actor. The auth middleware sets UserKey to the
+	// API-key NamedAPIKey.Name (or empty for unauthenticated). RBAC at the
+	// service layer requires a non-empty actor.
+	actor, _ := r.Context().Value(middleware.UserKey{}).(string)
+	if actor == "" {
+		ErrorWithRequestID(w, http.StatusUnauthorized,
+			"authentication required to approve / reject", requestID)
+		return
+	}
+
+	body := approvalDecisionBody{}
+	if r.Body != nil && r.ContentLength > 0 {
+		if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
+			ErrorWithRequestID(w, http.StatusBadRequest,
+				"invalid JSON body", requestID)
+			return
+		}
+	}
+
+	var err error
+	switch action {
+	case decisionApprove:
+		err = h.svc.Approve(r.Context(), id, actor, body.Note)
+	case decisionReject:
+		err = h.svc.Reject(r.Context(), id, actor, body.Note)
+	}
+	if err != nil {
+		switch {
+		case errors.Is(err, service.ErrApprovalNotFound):
+			ErrorWithRequestID(w, http.StatusNotFound, err.Error(), requestID)
+		case errors.Is(err, service.ErrApprovalAlreadyDecided):
+			ErrorWithRequestID(w, http.StatusConflict, err.Error(), requestID)
+		case errors.Is(err, service.ErrApproveBySameActor):
+			// The load-bearing two-person integrity contract surface.
+			// Compliance auditors expect this exact code path.
+			ErrorWithRequestID(w, http.StatusForbidden, err.Error(), requestID)
+		default:
+			ErrorWithRequestID(w, http.StatusInternalServerError,
+				"Failed to record decision", requestID)
+		}
+		return
+	}
+
+	JSON(w, http.StatusOK, map[string]interface{}{
+		"id":         id,
+		"decided_by": actor,
+		"action":     map[decisionAction]string{decisionApprove: "approved", decisionReject: "rejected"}[action],
+	})
+}
@@ -0,0 +1,244 @@
+package handler
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"sync"
+	"testing"
+
+	"github.com/certctl-io/certctl/internal/api/middleware"
+	"github.com/certctl-io/certctl/internal/domain"
+	"github.com/certctl-io/certctl/internal/repository"
+	"github.com/certctl-io/certctl/internal/service"
+)
+
+// fakeApprovalSvc satisfies handler.ApprovalServicer for tests. The
+// service-layer's same-actor RBAC + already-decided checks are
+// re-implemented here so the handler-level tests can exercise the
+// HTTP error-mapping without standing up the full ApprovalService.
+type fakeApprovalSvc struct {
+	mu        sync.Mutex
+	requests  map[string]*domain.ApprovalRequest // keyed by ID
+	approveBy map[string]string                  // ID → decidedBy (for assertions)
+}
+
+func newFakeApprovalSvc() *fakeApprovalSvc {
+	return &fakeApprovalSvc{
+		requests:  map[string]*domain.ApprovalRequest{},
+		approveBy: map[string]string{},
+	}
+}
+
+func (s *fakeApprovalSvc) seed(req *domain.ApprovalRequest) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	cp := *req
+	s.requests[req.ID] = &cp
+}
+
+func (s *fakeApprovalSvc) Approve(ctx context.Context, requestID, decidedBy, note string) error {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	r, ok := s.requests[requestID]
+	if !ok {
+		return service.ErrApprovalNotFound
+	}
+	if r.State.IsTerminal() {
+		return service.ErrApprovalAlreadyDecided
+	}
+	if decidedBy == r.RequestedBy {
+		return service.ErrApproveBySameActor
+	}
+	r.State = domain.ApprovalStateApproved
+	s.approveBy[requestID] = decidedBy
+	return nil
+}
+
+func (s *fakeApprovalSvc) Reject(ctx context.Context, requestID, decidedBy, note string) error {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	r, ok := s.requests[requestID]
+	if !ok {
+		return service.ErrApprovalNotFound
+	}
+	if r.State.IsTerminal() {
+		return service.ErrApprovalAlreadyDecided
+	}
+	if decidedBy == r.RequestedBy {
+		return service.ErrApproveBySameActor
+	}
+	r.State = domain.ApprovalStateRejected
+	s.approveBy[requestID] = decidedBy
+	return nil
+}
+
+func (s *fakeApprovalSvc) Get(ctx context.Context, id string) (*domain.ApprovalRequest, error) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	r, ok := s.requests[id]
+	if !ok {
+		return nil, service.ErrApprovalNotFound
+	}
+	cp := *r
+	return &cp, nil
+}
+
+func (s *fakeApprovalSvc) List(ctx context.Context, filter *repository.ApprovalFilter) ([]*domain.ApprovalRequest, error) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	var out []*domain.ApprovalRequest
+	for _, r := range s.requests {
+		if filter != nil && filter.State != "" && string(r.State) != filter.State {
+			continue
+		}
+		cp := *r
+		out = append(out, &cp)
+	}
+	return out, nil
+}
+
+// reqWithActor builds an httptest request with the auth-middleware UserKey
+// pre-populated. Mimics what the auth middleware does in production.
+func reqWithActor(t *testing.T, method, target string, body string, actor string, pathID string) (*http.Request, *httptest.ResponseRecorder) {
+	t.Helper()
+	var br *strings.Reader
+	if body != "" {
+		br = strings.NewReader(body)
+	}
+	var req *http.Request
+	if br != nil {
+		req = httptest.NewRequest(method, target, br)
+	} else {
+		req = httptest.NewRequest(method, target, nil)
+	}
+	req.Header.Set("Content-Type", "application/json")
+	if actor != "" {
+		req = req.WithContext(context.WithValue(req.Context(), middleware.UserKey{}, actor))
+	}
+	if pathID != "" {
+		req.SetPathValue("id", pathID)
+	}
+	rr := httptest.NewRecorder()
+	return req, rr
+}
+
+// TestApproval_HandlerApproveAsSameActor_Returns403 — handler-level pin
+// of the load-bearing RBAC contract. Compliance auditors expect HTTP 403
+// (not 401, not 500) when the requester tries to approve their own
+// request.
+func TestApproval_HandlerApproveAsSameActor_Returns403(t *testing.T) {
+	svc := newFakeApprovalSvc()
+	svc.seed(&domain.ApprovalRequest{
+		ID:          "ar-1",
+		JobID:       "job-1",
+		ProfileID:   "p-cdn",
+		RequestedBy: "user-alice",
+		State:       domain.ApprovalStatePending,
+	})
+	h := NewApprovalHandler(svc)
+
+	req, rr := reqWithActor(t, http.MethodPost,
+		"/api/v1/approvals/ar-1/approve", `{"note":"self-approve"}`, "user-alice", "ar-1")
+	h.Approve(rr, req)
+
+	if rr.Code != http.StatusForbidden {
+		t.Fatalf("expected 403; got %d (body=%s)", rr.Code, rr.Body.String())
+	}
+	if !strings.Contains(rr.Body.String(), "two-person integrity") {
+		t.Fatalf("expected two-person-integrity message in body; got %s", rr.Body.String())
+	}
+
+	// Different actor approves successfully — pins the success path too.
+	req2, rr2 := reqWithActor(t, http.MethodPost,
+		"/api/v1/approvals/ar-1/approve", `{"note":"approved by different actor"}`, "user-bob", "ar-1")
+	h.Approve(rr2, req2)
+	if rr2.Code != http.StatusOK {
+		t.Fatalf("expected 200 for different-actor approve; got %d (body=%s)", rr2.Code, rr2.Body.String())
+	}
+}
+
+// TestApproval_HandlerEmptyNote_Allowed_DecidedByExtractedFromAuth — handler
+// accepts an empty body / empty note (no compliance-blocking format
+// requirement) and the audit row records the absence. Pins that the
+// handler extracts decided_by from the auth-middleware UserKey, NOT from
+// the request body.
+func TestApproval_HandlerEmptyNote_Allowed_DecidedByExtractedFromAuth(t *testing.T) {
+	svc := newFakeApprovalSvc()
+	svc.seed(&domain.ApprovalRequest{
+		ID:          "ar-2",
+		JobID:       "job-2",
+		ProfileID:   "p-cdn",
+		RequestedBy: "user-charlie",
+		State:       domain.ApprovalStatePending,
+	})
+	h := NewApprovalHandler(svc)
+
+	// Empty body + empty note both accepted.
+	req, rr := reqWithActor(t, http.MethodPost,
+		"/api/v1/approvals/ar-2/approve", "", "user-bob", "ar-2")
+	h.Approve(rr, req)
+	if rr.Code != http.StatusOK {
+		t.Fatalf("expected 200 for empty body; got %d (body=%s)", rr.Code, rr.Body.String())
+	}
+
+	// Verify the response carries the auth-middleware-derived actor.
+	var resp map[string]interface{}
+	if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil {
+		t.Fatalf("decode resp: %v", err)
+	}
+	if resp["decided_by"] != "user-bob" {
+		t.Fatalf("decided_by should come from auth middleware; got %v", resp["decided_by"])
+	}
+
+	// Confirm the service-layer recorded user-bob as the decider.
+	if got := svc.approveBy["ar-2"]; got != "user-bob" {
+		t.Fatalf("svc should have recorded decidedBy=user-bob; got %s", got)
+	}
+
+	// Unauthenticated request returns 401, not 500.
+	req2, rr2 := reqWithActor(t, http.MethodPost,
+		"/api/v1/approvals/ar-2/approve", "", "", "ar-2")
+	h.Approve(rr2, req2)
+	if rr2.Code != http.StatusUnauthorized {
+		t.Fatalf("expected 401 for unauthenticated; got %d", rr2.Code)
+	}
+}
+
+// TestApproval_HandlerNotFound_Returns404 + AlreadyDecided returns 409 —
+// pin the error-status mapping for the remaining service sentinels.
+func TestApproval_HandlerErrorMapping(t *testing.T) {
+	svc := newFakeApprovalSvc()
+	svc.seed(&domain.ApprovalRequest{
+		ID:          "ar-decided",
+		JobID:       "job-3",
+		ProfileID:   "p-cdn",
+		RequestedBy: "user-alice",
+		State:       domain.ApprovalStateApproved,
+	})
+	h := NewApprovalHandler(svc)
+
+	t.Run("NotFound_Returns_404", func(t *testing.T) {
+		req, rr := reqWithActor(t, http.MethodPost,
+			"/api/v1/approvals/missing/approve", "", "user-bob", "missing")
+		h.Approve(rr, req)
+		if rr.Code != http.StatusNotFound {
+			t.Fatalf("expected 404; got %d", rr.Code)
+		}
+	})
+
+	t.Run("AlreadyDecided_Returns_409", func(t *testing.T) {
+		req, rr := reqWithActor(t, http.MethodPost,
+			"/api/v1/approvals/ar-decided/approve", "", "user-bob", "ar-decided")
+		h.Approve(rr, req)
+		if rr.Code != http.StatusConflict {
+			t.Fatalf("expected 409; got %d", rr.Code)
+		}
+		if !errors.Is(service.ErrApprovalAlreadyDecided, service.ErrApprovalAlreadyDecided) {
+			t.Fatal("sentinel sanity")
+		}
+	})
+}
@@ -156,6 +156,19 @@ type HandlerRegistry struct {
 	// authzs, challenges, key-change, revoke-cert, ARI. See
 	// docs/acme-server.md for the configuration reference.
 	ACME handler.ACMEHandler
+
+	// Approvals handles the issuance approval-workflow endpoints under
+	// /api/v1/approvals/*. Rank 7 of the 2026-05-03 Infisical deep-
+	// research deliverable — closes the two-person integrity / four-eyes
+	// principle procurement gap. Routes:
+	//   GET  /api/v1/approvals
+	//   GET  /api/v1/approvals/{id}
+	//   POST /api/v1/approvals/{id}/approve
+	//   POST /api/v1/approvals/{id}/reject
+	// Same-actor RBAC enforced at the service layer; the handler
+	// surfaces ErrApproveBySameActor as HTTP 403. See
+	// docs/approval-workflow.md for the operator playbook.
+	Approvals handler.ApprovalHandler
 }

 // RegisterHandlers sets up all API routes with their handlers.
@@ -350,6 +363,16 @@ func (r *Router) RegisterHandlers(reg HandlerRegistry) {
 	// before falling back to the {id} path-variable route above.
 	r.Register("POST /api/v1/notifications/{id}/requeue", http.HandlerFunc(reg.Notifications.RequeueNotification))

+	// Approvals routes: /api/v1/approvals (Rank 7).
+	// Same Go 1.22 ServeMux precedence as the notifications block — literal
+	// /approve and /reject segments resolve before the {id} pattern-var
+	// route. Same-actor RBAC enforced at the service layer; the handler
+	// surfaces ErrApproveBySameActor as HTTP 403.
+	r.Register("GET /api/v1/approvals", http.HandlerFunc(reg.Approvals.ListApprovals))
+	r.Register("GET /api/v1/approvals/{id}", http.HandlerFunc(reg.Approvals.GetApproval))
+	r.Register("POST /api/v1/approvals/{id}/approve", http.HandlerFunc(reg.Approvals.Approve))
+	r.Register("POST /api/v1/approvals/{id}/reject", http.HandlerFunc(reg.Approvals.Reject))
+
 	// Stats routes: /api/v1/stats
 	r.Register("GET /api/v1/stats/summary", http.HandlerFunc(reg.Stats.GetDashboardSummary))
 	r.Register("GET /api/v1/stats/certificates-by-status", http.HandlerFunc(reg.Stats.GetCertificatesByStatus))