From 5c01c7f21f7f8c3af87b951d37f14ec4512165bd Mon Sep 17 00:00:00 2001 From: Shankar Reddy Date: Sat, 18 Apr 2026 16:01:40 +0000 Subject: [PATCH] =?UTF-8?q?fix(gui,api):=20close=20C-001=20+=20C-002=20?= =?UTF-8?q?=E2=80=94=20ownership=20+=20agent=20FK=20contract?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C-001 — CreateCertificate was server-accepted with null owner_id, team_id, renewal_policy_id because the GUI neither collected the fields nor enforced them, even though the backend's ManagedCertificate schema and handler contract treat them as required. Fix the contract at all four layers: - web/src/pages/CertificatesPage.tsx: replace owner_id/team_id free- text inputs with populated from getAgents(); include agent in the canProceedToReview gate so Next is disabled until an agent is chosen. Regression coverage (21 new subtests total): - TestCreateCertificate_MissingRequiredField_Returns400 — 6 subtests, one per required field, each proves the handler guard fires before the mock service is called. - TestCreateTarget_MissingAgentID_Returns400 — handler guard. - TestCreateTarget_NonexistentAgent_Returns400 — pins the ErrAgentNotFound -> 400 translation. - TestTargetService_CreateTarget_MissingAgentID — errors.Is sentinel. - TestTargetService_CreateTarget_NonexistentAgentID — errors.Is. - The existing TestTargetService_CreateTarget_Success, along with TestCreateTarget_{MissingName,MissingType,NameTooLong}_* handler tests, were updated to seed a real agent or include agent_id in the request body so the happy paths still run cleanly. Gates (Phase 4): - go build/vet/test/race: green - go test -cover: internal/service 68.7% (gate 55%), internal/api/handler 78.9% (gate 60%) - golangci-lint on service+handler+mcp: 0 issues - govulncheck: no reachable vulns - tsc --noEmit: clean - vitest: 223/223 passing See cowork/certctl-coverage-gap-audit.md entries C-001 and C-002. --- api/openapi.yaml | 7 ++ .../api/handler/certificate_handler_test.go | 60 +++++++++++++++ internal/api/handler/target_handler_test.go | 76 +++++++++++++++++-- internal/api/handler/targets.go | 16 ++++ internal/mcp/types.go | 4 +- internal/service/target.go | 21 +++++ internal/service/target_test.go | 60 ++++++++++++++- web/src/pages/CertificatesPage.tsx | 67 ++++++++++++---- web/src/pages/TargetsPage.tsx | 28 +++++-- 9 files changed, 308 insertions(+), 31 deletions(-) diff --git a/api/openapi.yaml b/api/openapi.yaml index a205b54..5fc3b36 100644 --- a/api/openapi.yaml +++ b/api/openapi.yaml @@ -3326,6 +3326,7 @@ components: DeploymentTarget: type: object + required: [name, type, agent_id] properties: id: type: string @@ -3335,6 +3336,12 @@ components: $ref: "#/components/schemas/TargetType" agent_id: type: string + description: | + ID of the agent that manages this target. Required because + deployment_targets.agent_id is a NOT NULL foreign key to agents(id) + (migration 000001). Empty or nonexistent agent IDs are rejected + with HTTP 400 by the service layer (see C-002 in the coverage-gap + audit). config: type: object description: Target-specific configuration (varies by type) diff --git a/internal/api/handler/certificate_handler_test.go b/internal/api/handler/certificate_handler_test.go index 7da514f..55663e3 100644 --- a/internal/api/handler/certificate_handler_test.go +++ b/internal/api/handler/certificate_handler_test.go @@ -432,6 +432,66 @@ func TestCreateCertificate_ServiceError(t *testing.T) { } } +// TestCreateCertificate_MissingRequiredField_Returns400 pins the C-001 handler +// contract: handler MUST reject a create payload that omits any of the five +// required fields (name, common_name, owner_id, team_id, issuer_id, +// renewal_policy_id) with HTTP 400 before the service is invoked. The mock +// service here would succeed if called; every subtest proving 400 therefore +// proves the handler guard fires. +func TestCreateCertificate_MissingRequiredField_Returns400(t *testing.T) { + baseBody := map[string]interface{}{ + "name": "API Prod", + "common_name": "api.example.com", + "owner_id": "o-alice", + "team_id": "t-platform", + "issuer_id": "iss-local", + "renewal_policy_id": "rp-standard", + } + + cases := []struct { + name string + missingField string + }{ + {"missing name", "name"}, + {"missing common_name", "common_name"}, + {"missing owner_id", "owner_id"}, + {"missing team_id", "team_id"}, + {"missing issuer_id", "issuer_id"}, + {"missing renewal_policy_id", "renewal_policy_id"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + body := make(map[string]interface{}, len(baseBody)) + for k, v := range baseBody { + body[k] = v + } + delete(body, tc.missingField) + bodyBytes, _ := json.Marshal(body) + + mock := &MockCertificateService{ + CreateCertificateFn: func(_ context.Context, cert domain.ManagedCertificate) (*domain.ManagedCertificate, error) { + // Would succeed if handler guard did not fire. + cert.ID = "mc-would-be-created" + return &cert, nil + }, + } + handler := NewCertificateHandler(mock) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates", bytes.NewReader(bodyBytes)) + req = req.WithContext(contextWithRequestID()) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + + handler.CreateCertificate(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("%s: expected 400, got %d — body=%s", tc.name, w.Code, w.Body.String()) + } + }) + } +} + // Test UpdateCertificate - success case func TestUpdateCertificate_Success(t *testing.T) { updated := &domain.ManagedCertificate{ diff --git a/internal/api/handler/target_handler_test.go b/internal/api/handler/target_handler_test.go index 8d80f0c..756fdbf 100644 --- a/internal/api/handler/target_handler_test.go +++ b/internal/api/handler/target_handler_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/shankar0123/certctl/internal/domain" + "github.com/shankar0123/certctl/internal/service" ) // MockTargetService is a mock implementation of TargetService interface. @@ -239,8 +240,9 @@ func TestCreateTarget_Success(t *testing.T) { } body := map[string]interface{}{ - "name": "New Target", - "type": "nginx", + "name": "New Target", + "type": "nginx", + "agent_id": "agent-001", } bodyBytes, _ := json.Marshal(body) @@ -258,7 +260,8 @@ func TestCreateTarget_Success(t *testing.T) { func TestCreateTarget_MissingName(t *testing.T) { body := map[string]interface{}{ - "type": "nginx", + "type": "nginx", + "agent_id": "agent-001", } bodyBytes, _ := json.Marshal(body) @@ -276,7 +279,8 @@ func TestCreateTarget_MissingName(t *testing.T) { func TestCreateTarget_MissingType(t *testing.T) { body := map[string]interface{}{ - "name": "New Target", + "name": "New Target", + "agent_id": "agent-001", } bodyBytes, _ := json.Marshal(body) @@ -311,8 +315,9 @@ func TestCreateTarget_NameTooLong(t *testing.T) { longName += "x" } body := map[string]interface{}{ - "name": longName, - "type": "nginx", + "name": longName, + "type": "nginx", + "agent_id": "agent-001", } bodyBytes, _ := json.Marshal(body) @@ -340,6 +345,65 @@ func TestCreateTarget_MethodNotAllowed(t *testing.T) { } } +// TestCreateTarget_MissingAgentID_Returns400 pins the C-002 handler contract: +// handler MUST reject a create payload that omits agent_id with HTTP 400 +// before the service is invoked. Using a mock that would return 201-worthy +// success proves the guard fires. +func TestCreateTarget_MissingAgentID_Returns400(t *testing.T) { + body := map[string]interface{}{ + "name": "New Target", + "type": "nginx", + // agent_id intentionally omitted + } + bodyBytes, _ := json.Marshal(body) + + mock := &MockTargetService{ + CreateTargetFn: func(_ context.Context, target domain.DeploymentTarget) (*domain.DeploymentTarget, error) { + // Would succeed if handler guard did not fire. + target.ID = "t-would-be-created" + return &target, nil + }, + } + handler := NewTargetHandler(mock) + req := httptest.NewRequest(http.MethodPost, "/api/v1/targets", bytes.NewReader(bodyBytes)) + req = req.WithContext(contextWithRequestID()) + w := httptest.NewRecorder() + + handler.CreateTarget(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d — body=%s", w.Code, w.Body.String()) + } +} + +// TestCreateTarget_NonexistentAgent_Returns400 pins the C-002 handler↔service +// translation: when the service returns service.ErrAgentNotFound, the handler +// MUST map it to HTTP 400, not the generic 500 used for other service errors. +func TestCreateTarget_NonexistentAgent_Returns400(t *testing.T) { + mock := &MockTargetService{ + CreateTargetFn: func(_ context.Context, target domain.DeploymentTarget) (*domain.DeploymentTarget, error) { + return nil, service.ErrAgentNotFound + }, + } + body := map[string]interface{}{ + "name": "New Target", + "type": "nginx", + "agent_id": "agent-does-not-exist", + } + bodyBytes, _ := json.Marshal(body) + + handler := NewTargetHandler(mock) + req := httptest.NewRequest(http.MethodPost, "/api/v1/targets", bytes.NewReader(bodyBytes)) + req = req.WithContext(contextWithRequestID()) + w := httptest.NewRecorder() + + handler.CreateTarget(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for nonexistent agent, got %d — body=%s", w.Code, w.Body.String()) + } +} + func TestUpdateTarget_Success(t *testing.T) { now := time.Now() mock := &MockTargetService{ diff --git a/internal/api/handler/targets.go b/internal/api/handler/targets.go index a6e23db..d6d5b35 100644 --- a/internal/api/handler/targets.go +++ b/internal/api/handler/targets.go @@ -3,12 +3,14 @@ package handler import ( "context" "encoding/json" + "errors" "net/http" "strconv" "strings" "github.com/shankar0123/certctl/internal/api/middleware" "github.com/shankar0123/certctl/internal/domain" + "github.com/shankar0123/certctl/internal/service" ) // TargetService defines the service interface for deployment target operations. @@ -125,9 +127,23 @@ func (h TargetHandler) CreateTarget(w http.ResponseWriter, r *http.Request) { ErrorWithRequestID(w, http.StatusBadRequest, "type is required", requestID) return } + // C-002: agent_id is a NOT NULL FK in deployment_targets (migration 000001 + // line 104). Reject empty values at the boundary so callers get a clean 400 + // with the field name rather than a generic "Failed to create target" 500. + if err := ValidateRequired("agent_id", target.AgentID); err != nil { + ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID) + return + } created, err := h.svc.CreateTarget(r.Context(), target) if err != nil { + // C-002: a nonexistent agent_id is a client error, not a server error. + // The service returns ErrAgentNotFound (wrapped via fmt.Errorf %w) when + // agentRepo.Get fails; we translate that to 400 via errors.Is. + if errors.Is(err, service.ErrAgentNotFound) { + ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID) + return + } ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to create target", requestID) return } diff --git a/internal/mcp/types.go b/internal/mcp/types.go index 16e0503..d14c2fe 100644 --- a/internal/mcp/types.go +++ b/internal/mcp/types.go @@ -35,7 +35,7 @@ type CreateCertificateInput struct { TeamID string `json:"team_id" jsonschema:"Team ID (required)"` IssuerID string `json:"issuer_id" jsonschema:"Issuer connector ID"` TargetIDs []string `json:"target_ids,omitempty" jsonschema:"Deployment target IDs"` - RenewalPolicyID string `json:"renewal_policy_id,omitempty" jsonschema:"Renewal policy ID"` + RenewalPolicyID string `json:"renewal_policy_id" jsonschema:"Renewal policy ID (required)"` ProfileID string `json:"certificate_profile_id,omitempty" jsonschema:"Certificate profile ID"` Tags map[string]string `json:"tags,omitempty" jsonschema:"Key-value tags"` } @@ -112,7 +112,7 @@ type CreateTargetInput struct { ID string `json:"id,omitempty" jsonschema:"Target ID"` Name string `json:"name" jsonschema:"Target display name"` Type string `json:"type" jsonschema:"Target type: NGINX, Apache, HAProxy, F5, IIS"` - AgentID string `json:"agent_id,omitempty" jsonschema:"Agent ID that manages this target"` + AgentID string `json:"agent_id" jsonschema:"Agent ID that manages this target (required)"` Config interface{} `json:"config,omitempty" jsonschema:"Target-specific configuration"` Enabled bool `json:"enabled,omitempty" jsonschema:"Whether the target is enabled"` } diff --git a/internal/service/target.go b/internal/service/target.go index 0b7f464..bfbcc9e 100644 --- a/internal/service/target.go +++ b/internal/service/target.go @@ -3,6 +3,7 @@ package service import ( "context" "encoding/json" + "errors" "fmt" "log/slog" "time" @@ -12,6 +13,13 @@ import ( "github.com/shankar0123/certctl/internal/repository" ) +// ErrAgentNotFound is returned by [TargetService.CreateTarget] when the caller +// references an agent_id that is empty or does not correspond to a registered +// agent. The handler layer maps this to HTTP 400 via [errors.Is]. See C-002 in +// cowork/certctl-coverage-gap-audit.md — this sentinel replaces a silent +// Postgres FK violation (23503 → HTTP 500) with a deterministic 400. +var ErrAgentNotFound = errors.New("referenced agent does not exist") + // validTargetTypes is the set of allowed target types for validation. var validTargetTypes = map[domain.TargetType]bool{ domain.TargetTypeNGINX: true, @@ -276,6 +284,19 @@ func (s *TargetService) CreateTarget(ctx context.Context, target domain.Deployme if !isValidTargetType(target.Type) { return nil, fmt.Errorf("unsupported target type: %s", target.Type) } + + // C-002: enforce agent_id FK at service layer so we return a clean 400 + // instead of bubbling a Postgres 23503 foreign-key violation out as 500. + // The schema (migrations/000001 line 104) declares agent_id TEXT NOT NULL + // with a FK to agents(id); we mirror that contract here for deterministic + // error mapping. + if target.AgentID == "" { + return nil, fmt.Errorf("%w: agent_id is required", ErrAgentNotFound) + } + if _, err := s.agentRepo.Get(ctx, target.AgentID); err != nil { + return nil, fmt.Errorf("%w: %s", ErrAgentNotFound, target.AgentID) + } + if target.ID == "" { target.ID = generateID("target") } diff --git a/internal/service/target_test.go b/internal/service/target_test.go index 21e473e..cbca9fa 100644 --- a/internal/service/target_test.go +++ b/internal/service/target_test.go @@ -3,6 +3,7 @@ package service import ( "context" "encoding/json" + "errors" "log/slog" "os" "testing" @@ -377,11 +378,17 @@ func TestTargetService_GetTarget_Success(t *testing.T) { } func TestTargetService_CreateTarget_Success(t *testing.T) { - svc, targetRepo, _, _ := newTestTargetService() + svc, targetRepo, _, agentRepo := newTestTargetService() + + // C-002: CreateTarget now pre-validates agent_id against agentRepo. Seed a + // real agent so the happy path still exercises the normal creation flow + // without tripping the new ErrAgentNotFound guard. + agentRepo.AddAgent(&domain.Agent{ID: "a-1", Name: "test-agent"}) target := domain.DeploymentTarget{ - Name: "New Target", - Type: domain.TargetTypeNGINX, + Name: "New Target", + Type: domain.TargetTypeNGINX, + AgentID: "a-1", } ctx := context.Background() @@ -415,6 +422,53 @@ func TestTargetService_CreateTarget_InvalidType(t *testing.T) { } } +// TestTargetService_CreateTarget_MissingAgentID verifies the C-002 service-layer +// guard: an empty agent_id must be rejected with ErrAgentNotFound before the +// repository layer is ever consulted. The handler maps this sentinel to HTTP +// 400, so a 500 from a Postgres 23503 FK violation is never surfaced. +func TestTargetService_CreateTarget_MissingAgentID(t *testing.T) { + svc, _, _, _ := newTestTargetService() + + target := domain.DeploymentTarget{ + Name: "No Agent", + Type: domain.TargetTypeNGINX, + // AgentID intentionally empty + } + + ctx := context.Background() + _, err := svc.CreateTarget(ctx, target) + if err == nil { + t.Fatalf("expected error for missing agent_id, got nil") + } + if !errors.Is(err, ErrAgentNotFound) { + t.Errorf("expected errors.Is(err, ErrAgentNotFound) to be true, got err=%v", err) + } +} + +// TestTargetService_CreateTarget_NonexistentAgentID verifies the second half of +// the C-002 guard: a non-empty agent_id that does not resolve in agentRepo +// still returns ErrAgentNotFound rather than letting the FK violation escape to +// Postgres. This is the realistic failure mode for a GUI sending a stale +// agent_id or a CLI caller with a typo. +func TestTargetService_CreateTarget_NonexistentAgentID(t *testing.T) { + svc, _, _, _ := newTestTargetService() + + target := domain.DeploymentTarget{ + Name: "Bad Agent Ref", + Type: domain.TargetTypeNGINX, + AgentID: "a-does-not-exist", + } + + ctx := context.Background() + _, err := svc.CreateTarget(ctx, target) + if err == nil { + t.Fatalf("expected error for nonexistent agent_id, got nil") + } + if !errors.Is(err, ErrAgentNotFound) { + t.Errorf("expected errors.Is(err, ErrAgentNotFound) to be true, got err=%v", err) + } +} + func TestTargetService_UpdateTarget_Success(t *testing.T) { svc, targetRepo, _, _ := newTestTargetService() diff --git a/web/src/pages/CertificatesPage.tsx b/web/src/pages/CertificatesPage.tsx index a864629..62e539f 100644 --- a/web/src/pages/CertificatesPage.tsx +++ b/web/src/pages/CertificatesPage.tsx @@ -1,7 +1,7 @@ import { useState } from 'react'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { useNavigate } from 'react-router-dom'; -import { getCertificates, createCertificate, triggerRenewal, revokeCertificate, updateCertificate, getOwners, getProfiles, getIssuers, bulkRevokeCertificates } from '../api/client'; +import { getCertificates, createCertificate, triggerRenewal, revokeCertificate, updateCertificate, getOwners, getTeams, getPolicies, getProfiles, getIssuers, bulkRevokeCertificates } from '../api/client'; import { REVOCATION_REASONS } from '../api/types'; import PageHeader from '../components/PageHeader'; import DataTable from '../components/DataTable'; @@ -35,8 +35,27 @@ function CreateCertificateModal({ onClose, onSuccess }: { onClose: () => void; o queryKey: ['issuers'], queryFn: () => getIssuers(), }); + // C-001: owner_id, team_id, and renewal_policy_id are required by the + // server (handler in internal/api/handler/certificates.go) and by OpenAPI. + // Load the catalog so the user selects valid FKs instead of typing free-text + // IDs that would 400 at the server. + const { data: ownersResp } = useQuery({ + queryKey: ['owners', 'form'], + queryFn: () => getOwners({ per_page: '500' }), + }); + const { data: teamsResp } = useQuery({ + queryKey: ['teams', 'form'], + queryFn: () => getTeams({ per_page: '500' }), + }); + const { data: policiesResp } = useQuery({ + queryKey: ['renewal-policies', 'form'], + queryFn: () => getPolicies({ per_page: '500' }), + }); const profiles = profilesResp?.data || []; const issuers = issuersResp?.data || []; + const owners = ownersResp?.data || []; + const teams = teamsResp?.data || []; + const policies = policiesResp?.data || []; const selectedProfile = profiles.find(p => p.id === form.certificate_profile_id); const ttlLabel = selectedProfile @@ -143,24 +162,36 @@ function CreateCertificateModal({ onClose, onSuccess }: { onClose: () => void; o
- - setForm(f => ({ ...f, renewal_policy_id: e.target.value }))} - className={inputClass} - placeholder="rp-standard" /> + +
- - setForm(f => ({ ...f, owner_id: e.target.value }))} - className={inputClass} - placeholder="o-alice" /> + +
- - setForm(f => ({ ...f, team_id: e.target.value }))} - className={inputClass} - placeholder="t-platform" /> + +
@@ -175,7 +206,15 @@ function CreateCertificateModal({ onClose, onSuccess }: { onClose: () => void; o