From f53f9f9ca37ad43750ae148b42575c5a2bdd164f Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Mon, 4 May 2026 01:05:16 +0000 Subject: [PATCH] api, handler: 4 approval endpoints + handler RBAC integration tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/api/handler/approval.go | 201 +++++++++++++++++++++ internal/api/handler/approval_test.go | 244 ++++++++++++++++++++++++++ internal/api/router/router.go | 23 +++ 3 files changed, 468 insertions(+) create mode 100644 internal/api/handler/approval.go create mode 100644 internal/api/handler/approval_test.go diff --git a/internal/api/handler/approval.go b/internal/api/handler/approval.go new file mode 100644 index 0000000..9974b8c --- /dev/null +++ b/internal/api/handler/approval.go @@ -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], + }) +} diff --git a/internal/api/handler/approval_test.go b/internal/api/handler/approval_test.go new file mode 100644 index 0000000..fc73a40 --- /dev/null +++ b/internal/api/handler/approval_test.go @@ -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") + } + }) +} diff --git a/internal/api/router/router.go b/internal/api/router/router.go index 05d78cf..39a86d5 100644 --- a/internal/api/router/router.go +++ b/internal/api/router/router.go @@ -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))