mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 16:21:30 +00:00
fix(gui,api): close C-001 + C-002 — ownership + agent FK contract
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 <select> elements fed by getOwners/getTeams/
getPolicies queries; mark all three required; gate the Create
button on owner_id + team_id + renewal_policy_id being set.
- internal/api/handler/certificates.go: ValidateRequired for
owner_id, team_id, renewal_policy_id on CreateCertificate so the
handler returns HTTP 400 with the offending field name before the
service layer is reached.
- internal/mcp/types.go: drop ',omitempty' from
CreateCertificateInput.RenewalPolicyID so the MCP schema reflects
the required contract; Update inputs keep partial-update semantics.
- api/openapi.yaml: 'required: [name, common_name, renewal_policy_id,
issuer_id, owner_id, team_id]' was already present on the Create
schema; clarified DeploymentTarget.agent_id description to note the
FK contract.
C-002 — CreateTargetWizard accepted an empty or bogus agent_id and the
service inserted directly, producing a Postgres 23503 FK-violation that
bubbled out as a generic HTTP 500. The FK itself (migration 000001 line
104: agent_id TEXT NOT NULL REFERENCES agents(id)) is correct; we keep
the schema strict and add validation at three layers:
- internal/service/target.go: introduce
ErrAgentNotFound sentinel and pre-validate agent_id in
TargetService.CreateTarget — empty string returns
'agent_id is required'; a nonexistent id returns the full
'referenced agent does not exist: <id>' error. Both wrap
ErrAgentNotFound via fmt.Errorf %w so callers can use errors.Is.
- internal/api/handler/targets.go: ValidateRequired on agent_id; map
errors.Is(err, service.ErrAgentNotFound) to HTTP 400 instead of
letting it fall through to the generic 500 branch.
- internal/mcp/types.go: drop ',omitempty' from
CreateTargetInput.AgentID to match the required contract.
- web/src/pages/TargetsPage.tsx: replace the free-text Agent ID input
with a <select> 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.
This commit is contained in:
@@ -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
|
||||
</select>
|
||||
</div>
|
||||
<div>
|
||||
<label className="text-xs text-ink-muted block mb-1">Policy</label>
|
||||
<input value={form.renewal_policy_id} onChange={e => setForm(f => ({ ...f, renewal_policy_id: e.target.value }))}
|
||||
className={inputClass}
|
||||
placeholder="rp-standard" />
|
||||
<label className="text-xs text-ink-muted block mb-1">Policy *</label>
|
||||
<select value={form.renewal_policy_id} onChange={e => setForm(f => ({ ...f, renewal_policy_id: e.target.value }))}
|
||||
className={selectClass}>
|
||||
<option value="">Select policy...</option>
|
||||
{policies.map(p => (
|
||||
<option key={p.id} value={p.id}>{p.name}</option>
|
||||
))}
|
||||
</select>
|
||||
</div>
|
||||
</div>
|
||||
<div className="grid grid-cols-2 gap-3">
|
||||
<div>
|
||||
<label className="text-xs text-ink-muted block mb-1">Owner</label>
|
||||
<input value={form.owner_id} onChange={e => setForm(f => ({ ...f, owner_id: e.target.value }))}
|
||||
className={inputClass}
|
||||
placeholder="o-alice" />
|
||||
<label className="text-xs text-ink-muted block mb-1">Owner *</label>
|
||||
<select value={form.owner_id} onChange={e => setForm(f => ({ ...f, owner_id: e.target.value }))}
|
||||
className={selectClass}>
|
||||
<option value="">Select owner...</option>
|
||||
{owners.map(o => (
|
||||
<option key={o.id} value={o.id}>{o.name} ({o.email})</option>
|
||||
))}
|
||||
</select>
|
||||
</div>
|
||||
<div>
|
||||
<label className="text-xs text-ink-muted block mb-1">Team</label>
|
||||
<input value={form.team_id} onChange={e => setForm(f => ({ ...f, team_id: e.target.value }))}
|
||||
className={inputClass}
|
||||
placeholder="t-platform" />
|
||||
<label className="text-xs text-ink-muted block mb-1">Team *</label>
|
||||
<select value={form.team_id} onChange={e => setForm(f => ({ ...f, team_id: e.target.value }))}
|
||||
className={selectClass}>
|
||||
<option value="">Select team...</option>
|
||||
{teams.map(t => (
|
||||
<option key={t.id} value={t.id}>{t.name}</option>
|
||||
))}
|
||||
</select>
|
||||
</div>
|
||||
</div>
|
||||
<div>
|
||||
@@ -175,7 +206,15 @@ function CreateCertificateModal({ onClose, onSuccess }: { onClose: () => void; o
|
||||
<button onClick={onClose} className="btn btn-ghost text-sm">Cancel</button>
|
||||
<button
|
||||
onClick={() => mutation.mutate()}
|
||||
disabled={!form.name || !form.common_name || !form.issuer_id || mutation.isPending}
|
||||
disabled={
|
||||
!form.name ||
|
||||
!form.common_name ||
|
||||
!form.issuer_id ||
|
||||
!form.owner_id ||
|
||||
!form.team_id ||
|
||||
!form.renewal_policy_id ||
|
||||
mutation.isPending
|
||||
}
|
||||
className="btn btn-primary text-sm disabled:opacity-50"
|
||||
>
|
||||
{mutation.isPending ? 'Creating...' : 'Create Certificate'}
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { useState } from 'react';
|
||||
import { Link } from 'react-router-dom';
|
||||
import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query';
|
||||
import { getTargets, createTarget, deleteTarget } from '../api/client';
|
||||
import { getTargets, createTarget, deleteTarget, getAgents } from '../api/client';
|
||||
import PageHeader from '../components/PageHeader';
|
||||
import DataTable from '../components/DataTable';
|
||||
import type { Column } from '../components/DataTable';
|
||||
@@ -180,6 +180,16 @@ function CreateTargetWizard({ onClose, onSuccess }: { onClose: () => void; onSuc
|
||||
const [config, setConfig] = useState<Record<string, string>>({});
|
||||
const [error, setError] = useState('');
|
||||
|
||||
// C-002: agent_id is a NOT NULL FK in deployment_targets (migration 000001
|
||||
// line 104). Load registered agents so the user picks a valid FK instead of
|
||||
// typing a free-text ID that would 400 at the service layer (or, pre-fix,
|
||||
// bubble up as a Postgres 23503 foreign-key violation → 500).
|
||||
const { data: agentsResp } = useQuery({
|
||||
queryKey: ['agents', 'form'],
|
||||
queryFn: () => getAgents({ per_page: '500' }),
|
||||
});
|
||||
const agents = agentsResp?.data || [];
|
||||
|
||||
// Fields that backends expect as boolean (Go bool)
|
||||
const BOOL_FIELDS = new Set([
|
||||
'sni', 'insecure', 'sds_config', 'remove_expired', 'create_keystore',
|
||||
@@ -244,7 +254,7 @@ function CreateTargetWizard({ onClose, onSuccess }: { onClose: () => void; onSuc
|
||||
});
|
||||
|
||||
const fields = CONFIG_FIELDS[targetType] || [];
|
||||
const canProceedToReview = name && targetType && fields.filter(f => f.required).every(f => config[f.key]);
|
||||
const canProceedToReview = name && targetType && agentId && fields.filter(f => f.required).every(f => config[f.key]);
|
||||
|
||||
return (
|
||||
<div className="fixed inset-0 bg-black/40 flex items-center justify-center z-50" onClick={onClose}>
|
||||
@@ -314,10 +324,16 @@ function CreateTargetWizard({ onClose, onSuccess }: { onClose: () => void; onSuc
|
||||
placeholder="web-server-1" />
|
||||
</div>
|
||||
<div>
|
||||
<label className="text-xs text-ink-muted block mb-1">Agent ID</label>
|
||||
<input value={agentId} onChange={e => setAgentId(e.target.value)}
|
||||
className="w-full bg-white border border-surface-border rounded px-3 py-2 text-sm text-ink focus:outline-none focus:border-brand-400"
|
||||
placeholder="agent-web1" />
|
||||
<label className="text-xs text-ink-muted block mb-1">Agent *</label>
|
||||
<select value={agentId} onChange={e => setAgentId(e.target.value)}
|
||||
className="w-full bg-white border border-surface-border rounded px-3 py-2 text-sm text-ink focus:outline-none focus:border-brand-400">
|
||||
<option value="">Select an agent...</option>
|
||||
{agents.map(a => (
|
||||
<option key={a.id} value={a.id}>
|
||||
{a.hostname || a.id} ({a.id})
|
||||
</option>
|
||||
))}
|
||||
</select>
|
||||
</div>
|
||||
{fields.map(f => (
|
||||
<div key={f.key}>
|
||||
|
||||
Reference in New Issue
Block a user