diff --git a/api/openapi.yaml b/api/openapi.yaml index df890d2..65140e8 100644 --- a/api/openapi.yaml +++ b/api/openapi.yaml @@ -3480,6 +3480,9 @@ components: description: Policy-specific configuration (varies by type) enabled: type: boolean + severity: + $ref: "#/components/schemas/PolicySeverity" + description: Severity level applied to violations of this rule. Defaults to Warning on create when omitted. created_at: type: string format: date-time diff --git a/internal/api/handler/policies.go b/internal/api/handler/policies.go index 9c74076..8acfd64 100644 --- a/internal/api/handler/policies.go +++ b/internal/api/handler/policies.go @@ -127,6 +127,17 @@ func (h PolicyHandler) CreatePolicy(w http.ResponseWriter, r *http.Request) { ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID) return } + // Severity is optional on create; default matches the DB default. + // Any explicit value must pass the TitleCase allowlist; the DB CHECK + // constraint enforces the same set, but catching it here gives a 400 + // with a clear message instead of a 500 on constraint violation. + if policy.Severity == "" { + policy.Severity = domain.PolicySeverityWarning + } + if err := ValidatePolicySeverity(policy.Severity); err != nil { + ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID) + return + } created, err := h.svc.CreatePolicy(r.Context(), policy) if err != nil { @@ -174,6 +185,12 @@ func (h PolicyHandler) UpdatePolicy(w http.ResponseWriter, r *http.Request) { return } } + if policy.Severity != "" { + if err := ValidatePolicySeverity(policy.Severity); err != nil { + ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID) + return + } + } updated, err := h.svc.UpdatePolicy(r.Context(), id, policy) if err != nil { diff --git a/internal/domain/policy.go b/internal/domain/policy.go index aae73c0..312a24c 100644 --- a/internal/domain/policy.go +++ b/internal/domain/policy.go @@ -12,6 +12,7 @@ type PolicyRule struct { Type PolicyType `json:"type"` Config json.RawMessage `json:"config"` Enabled bool `json:"enabled"` + Severity PolicySeverity `json:"severity"` CreatedAt time.Time `json:"created_at"` UpdatedAt time.Time `json:"updated_at"` } diff --git a/internal/integration/e2e_test.go b/internal/integration/e2e_test.go index 91656ff..2b1dc20 100644 --- a/internal/integration/e2e_test.go +++ b/internal/integration/e2e_test.go @@ -158,7 +158,7 @@ func TestCrossResourceWorkflow(t *testing.T) { payload := map[string]interface{}{ "name": "Allowed Domains Policy", "type": "AllowedDomains", - "severity": "High", + "severity": "Error", "config": json.RawMessage(`{"domains": ["example.com", "*.example.com"]}`), "description": "Restrict issuance to example.com domains", } diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index d960be0..1ab0197 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -610,7 +610,7 @@ func registerPolicyTools(s *gomcp.Server, c *Client) { gomcp.AddTool(s, &gomcp.Tool{ Name: "certctl_create_policy", - Description: "Create a new policy rule. Requires name and type.", + Description: "Create a new policy rule. Requires name and type. Optional severity (Warning, Error, Critical) defaults to Warning.", }, func(ctx context.Context, req *gomcp.CallToolRequest, input CreatePolicyInput) (*gomcp.CallToolResult, any, error) { data, err := c.Post("/api/v1/policies", input) if err != nil { @@ -621,7 +621,7 @@ func registerPolicyTools(s *gomcp.Server, c *Client) { gomcp.AddTool(s, &gomcp.Tool{ Name: "certctl_update_policy", - Description: "Update a policy rule's name, type, configuration, or enabled status.", + Description: "Update a policy rule's name, type, configuration, enabled status, or severity.", }, func(ctx context.Context, req *gomcp.CallToolRequest, input UpdatePolicyInput) (*gomcp.CallToolResult, any, error) { data, err := c.Put("/api/v1/policies/"+input.ID, input) if err != nil { diff --git a/internal/mcp/types.go b/internal/mcp/types.go index ac4d228..16e0503 100644 --- a/internal/mcp/types.go +++ b/internal/mcp/types.go @@ -168,19 +168,21 @@ type RejectJobInput struct { // ── Policies ──────────────────────────────────────────────────────── type CreatePolicyInput struct { - ID string `json:"id,omitempty" jsonschema:"Policy ID"` - Name string `json:"name" jsonschema:"Policy display name"` - Type string `json:"type" jsonschema:"Policy type: AllowedIssuers, AllowedDomains, RequiredMetadata, AllowedEnvironments, RenewalLeadTime"` - Config interface{} `json:"config,omitempty" jsonschema:"Policy-specific configuration"` - Enabled bool `json:"enabled,omitempty" jsonschema:"Whether the policy is enabled"` + ID string `json:"id,omitempty" jsonschema:"Policy ID"` + Name string `json:"name" jsonschema:"Policy display name"` + Type string `json:"type" jsonschema:"Policy type: AllowedIssuers, AllowedDomains, RequiredMetadata, AllowedEnvironments, RenewalLeadTime"` + Config interface{} `json:"config,omitempty" jsonschema:"Policy-specific configuration"` + Enabled bool `json:"enabled,omitempty" jsonschema:"Whether the policy is enabled"` + Severity string `json:"severity,omitempty" jsonschema:"Violation severity: Warning, Error, or Critical (default: Warning)"` } type UpdatePolicyInput struct { - ID string `json:"id" jsonschema:"Policy ID to update"` - Name string `json:"name,omitempty" jsonschema:"Policy display name"` - Type string `json:"type,omitempty" jsonschema:"Policy type"` - Config interface{} `json:"config,omitempty" jsonschema:"Policy-specific configuration"` - Enabled *bool `json:"enabled,omitempty" jsonschema:"Whether the policy is enabled"` + ID string `json:"id" jsonschema:"Policy ID to update"` + Name string `json:"name,omitempty" jsonschema:"Policy display name"` + Type string `json:"type,omitempty" jsonschema:"Policy type"` + Config interface{} `json:"config,omitempty" jsonschema:"Policy-specific configuration"` + Enabled *bool `json:"enabled,omitempty" jsonschema:"Whether the policy is enabled"` + Severity string `json:"severity,omitempty" jsonschema:"Violation severity: Warning, Error, or Critical"` } type ListViolationsInput struct { diff --git a/internal/repository/postgres/policy.go b/internal/repository/postgres/policy.go index 46a0c61..1d828bf 100644 --- a/internal/repository/postgres/policy.go +++ b/internal/repository/postgres/policy.go @@ -24,7 +24,7 @@ func NewPolicyRepository(db *sql.DB) *PolicyRepository { // ListRules returns all policy rules func (r *PolicyRepository) ListRules(ctx context.Context) ([]*domain.PolicyRule, error) { rows, err := r.db.QueryContext(ctx, ` - SELECT id, name, type, config, enabled, created_at, updated_at + SELECT id, name, type, config, enabled, severity, created_at, updated_at FROM policy_rules ORDER BY created_at DESC `) @@ -38,7 +38,7 @@ func (r *PolicyRepository) ListRules(ctx context.Context) ([]*domain.PolicyRule, for rows.Next() { var rule domain.PolicyRule if err := rows.Scan(&rule.ID, &rule.Name, &rule.Type, &rule.Config, - &rule.Enabled, &rule.CreatedAt, &rule.UpdatedAt); err != nil { + &rule.Enabled, &rule.Severity, &rule.CreatedAt, &rule.UpdatedAt); err != nil { return nil, fmt.Errorf("failed to scan policy rule: %w", err) } rules = append(rules, &rule) @@ -55,11 +55,11 @@ func (r *PolicyRepository) ListRules(ctx context.Context) ([]*domain.PolicyRule, func (r *PolicyRepository) GetRule(ctx context.Context, id string) (*domain.PolicyRule, error) { var rule domain.PolicyRule err := r.db.QueryRowContext(ctx, ` - SELECT id, name, type, config, enabled, created_at, updated_at + SELECT id, name, type, config, enabled, severity, created_at, updated_at FROM policy_rules WHERE id = $1 `, id).Scan(&rule.ID, &rule.Name, &rule.Type, &rule.Config, - &rule.Enabled, &rule.CreatedAt, &rule.UpdatedAt) + &rule.Enabled, &rule.Severity, &rule.CreatedAt, &rule.UpdatedAt) if err != nil { if err == sql.ErrNoRows { @@ -78,11 +78,11 @@ func (r *PolicyRepository) CreateRule(ctx context.Context, rule *domain.PolicyRu } err := r.db.QueryRowContext(ctx, ` - INSERT INTO policy_rules (id, name, type, config, enabled, created_at, updated_at) - VALUES ($1, $2, $3, $4, $5, $6, $7) + INSERT INTO policy_rules (id, name, type, config, enabled, severity, created_at, updated_at) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8) RETURNING id `, rule.ID, rule.Name, rule.Type, rule.Config, rule.Enabled, - rule.CreatedAt, rule.UpdatedAt).Scan(&rule.ID) + rule.Severity, rule.CreatedAt, rule.UpdatedAt).Scan(&rule.ID) if err != nil { return fmt.Errorf("failed to create policy rule: %w", err) @@ -99,9 +99,10 @@ func (r *PolicyRepository) UpdateRule(ctx context.Context, rule *domain.PolicyRu type = $2, config = $3, enabled = $4, - updated_at = $5 - WHERE id = $6 - `, rule.Name, rule.Type, rule.Config, rule.Enabled, rule.UpdatedAt, rule.ID) + severity = $5, + updated_at = $6 + WHERE id = $7 + `, rule.Name, rule.Type, rule.Config, rule.Enabled, rule.Severity, rule.UpdatedAt, rule.ID) if err != nil { return fmt.Errorf("failed to update policy rule: %w", err) diff --git a/internal/service/policy.go b/internal/service/policy.go index 45ba127..e411791 100644 --- a/internal/service/policy.go +++ b/internal/service/policy.go @@ -288,6 +288,20 @@ func (s *PolicyService) UpdatePolicy(ctx context.Context, id string, policy doma policy.ID = id policy.UpdatedAt = time.Now() + // Severity is NOT NULL with a CHECK constraint at the DB level + // (migration 000013). If the client omits severity on a PUT (zero-value + // empty string after json.Decode), preserve the existing severity rather + // than letting the CHECK reject the write. Preserves partial-update + // semantics for the new column without changing the pre-existing behavior + // for Name/Type, which is out of scope for D-005/D-006. + if policy.Severity == "" { + existing, err := s.policyRepo.GetRule(ctx, id) + if err != nil { + return nil, fmt.Errorf("failed to fetch existing rule for severity preservation: %w", err) + } + policy.Severity = existing.Severity + } + if err := s.policyRepo.UpdateRule(ctx, &policy); err != nil { return nil, fmt.Errorf("failed to update policy: %w", err) } diff --git a/migrations/000013_policy_rule_severity.down.sql b/migrations/000013_policy_rule_severity.down.sql new file mode 100644 index 0000000..5cd6a56 --- /dev/null +++ b/migrations/000013_policy_rule_severity.down.sql @@ -0,0 +1,8 @@ +-- Rollback migration 000013: remove per-rule severity. +-- +-- DROP COLUMN removes the column, its CHECK constraint, and the default in +-- one statement. Any downstream code still referencing severity after +-- rollback will fail at query time — that's intentional, since running this +-- rollback implies severity as a concept is being abandoned. + +ALTER TABLE policy_rules DROP COLUMN IF EXISTS severity; diff --git a/migrations/000013_policy_rule_severity.up.sql b/migrations/000013_policy_rule_severity.up.sql new file mode 100644 index 0000000..8799203 --- /dev/null +++ b/migrations/000013_policy_rule_severity.up.sql @@ -0,0 +1,24 @@ +-- Migration 000013: Per-Rule Severity on policy_rules +-- +-- Prior to this migration, PolicyRule had no severity column. The TypeScript +-- frontend (PoliciesPage.tsx) sent a `severity` field on create/update, but +-- Go's json.Decoder silently dropped it (no matching struct field) and the +-- value never reached PostgreSQL. Reloading the page always showed severity +-- reverting to a default — the classic "silent drop" bug. +-- +-- This migration adds severity as a first-class column on policy_rules. +-- Default `'Warning'` covers pre-existing rows; the CHECK constraint gives +-- defense-in-depth against casing drift (the application-layer validator in +-- internal/api/handler/validation.go already enforces the TitleCase allowlist, +-- but the DB should reject a bypassed write too). +-- +-- No index: three-value column on a table that stays in the low thousands of +-- rows. The planner will seq-scan regardless; write cost without read benefit. +-- If measurements later justify it, add the index then. +-- +-- PG 11+ makes ADD COLUMN with a literal DEFAULT a metadata-only operation +-- (no table rewrite), so this is safe to run on a live server. + +ALTER TABLE policy_rules + ADD COLUMN IF NOT EXISTS severity VARCHAR(50) NOT NULL DEFAULT 'Warning' + CHECK (severity IN ('Warning', 'Error', 'Critical')); diff --git a/migrations/seed.sql b/migrations/seed.sql index 63065ab..06d5a63 100644 --- a/migrations/seed.sql +++ b/migrations/seed.sql @@ -13,41 +13,51 @@ VALUES ( ) ON CONFLICT (id) DO NOTHING; -- Policy rules: Require owner assignment -INSERT INTO policy_rules (id, name, type, config, enabled) +-- Severity differentiated per rule to demonstrate the field means something +-- (D-006). The backend CHECK constraint (migration 000013) enforces the +-- TitleCase allowlist Warning/Error/Critical. Type-value drift +-- (ownership/environment/lifetime/renewal_window vs. the engine's TitleCase +-- canonicals) is tracked separately in d/D-008 and intentionally left +-- unchanged in this commit. +INSERT INTO policy_rules (id, name, type, config, enabled, severity) VALUES ( 'pr-require-owner', 'require-owner', 'ownership', '{"requirement": "owner_id must be set"}'::jsonb, - true + true, + 'Warning' ) ON CONFLICT (id) DO NOTHING; -- Policy rules: Allowed environments -INSERT INTO policy_rules (id, name, type, config, enabled) +INSERT INTO policy_rules (id, name, type, config, enabled, severity) VALUES ( 'pr-allowed-environments', 'allowed-environments', 'environment', '{"allowed": ["production", "staging", "development"]}'::jsonb, - true + true, + 'Error' ) ON CONFLICT (id) DO NOTHING; -- Policy rules: Maximum certificate lifetime -INSERT INTO policy_rules (id, name, type, config, enabled) +INSERT INTO policy_rules (id, name, type, config, enabled, severity) VALUES ( 'pr-max-certificate-lifetime', 'max-certificate-lifetime', 'lifetime', '{"max_days": 90}'::jsonb, - true + true, + 'Critical' ) ON CONFLICT (id) DO NOTHING; -- Policy rules: Minimum renewal window -INSERT INTO policy_rules (id, name, type, config, enabled) +INSERT INTO policy_rules (id, name, type, config, enabled, severity) VALUES ( 'pr-min-renewal-window', 'min-renewal-window', 'renewal_window', '{"min_days": 14}'::jsonb, - true + true, + 'Warning' ) ON CONFLICT (id) DO NOTHING; diff --git a/web/src/api/types.test.ts b/web/src/api/types.test.ts new file mode 100644 index 0000000..957305f --- /dev/null +++ b/web/src/api/types.test.ts @@ -0,0 +1,59 @@ +import { describe, it, expect } from 'vitest'; +import { POLICY_TYPES, POLICY_SEVERITIES } from './types'; + +/** + * Regression tests for the policy enum tuples. + * + * These tuples are the GUI's source of truth for the policy type and severity + * dropdowns. They MUST stay in lockstep with the backend enum values: + * - internal/domain/policy.go defines the PolicyType / PolicySeverity consts + * - internal/api/handler/validators.go rejects anything outside the allowlist + * - migration 000013 enforces the severity allowlist at the DB level via CHECK + * + * Audit history (D-005, D-006): + * - The GUI previously sent lowercase values (e.g. 'key_algorithm', + * 'ownership'), which the backend validator rejected with a 400. Every + * attempt to create a policy from the "+ New Policy" button silently + * failed until the modal was closed. + * - The severity dropdown carried a four-value `low/medium/high/critical` + * tuple that shared zero values with the backend's + * `Warning/Error/Critical` — the `medium` option has no backend analog + * and is removed. + * + * If these tests fail because a backend enum changed, DO NOT update the + * expected arrays without also updating the backend consts and the migration. + * Frontend/backend drift on these tuples is precisely what this regression + * guards against. + */ + +describe('POLICY_TYPES', () => { + it('matches the backend PolicyType TitleCase allowlist exactly', () => { + expect(POLICY_TYPES).toEqual([ + 'AllowedIssuers', + 'AllowedDomains', + 'RequiredMetadata', + 'AllowedEnvironments', + 'RenewalLeadTime', + ]); + }); + + it('has no duplicate entries', () => { + expect(new Set(POLICY_TYPES).size).toBe(POLICY_TYPES.length); + }); +}); + +describe('POLICY_SEVERITIES', () => { + it('matches the backend PolicySeverity TitleCase allowlist exactly', () => { + expect(POLICY_SEVERITIES).toEqual(['Warning', 'Error', 'Critical']); + }); + + it('has no duplicate entries', () => { + expect(new Set(POLICY_SEVERITIES).size).toBe(POLICY_SEVERITIES.length); + }); + + it('does not include the removed pre-fix `medium` value', () => { + // Explicit negative assertion. Pre-fix the GUI offered four severities + // (low/medium/high/critical); `medium` never had a backend analog. + expect(POLICY_SEVERITIES as readonly string[]).not.toContain('medium'); + }); +}); diff --git a/web/src/api/types.ts b/web/src/api/types.ts index 46fcea5..c58cb70 100644 --- a/web/src/api/types.ts +++ b/web/src/api/types.ts @@ -112,11 +112,37 @@ export interface AuditEvent { timestamp: string; } +/** + * Policy rule type enum — pinned to the backend's TitleCase constants in + * internal/domain/policy.go. Historical note (D-005): the GUI previously sent + * lowercase values (`ownership`, `environment`, etc.) that the handler's + * ValidatePolicyType rejected with a 400. These tuples are the canonical + * source of truth for the dropdown options; the regression test in + * types.test.ts pins them so future drift is caught at CI time. + */ +export const POLICY_TYPES = [ + 'AllowedIssuers', + 'AllowedDomains', + 'RequiredMetadata', + 'AllowedEnvironments', + 'RenewalLeadTime', +] as const; +export type PolicyType = (typeof POLICY_TYPES)[number]; + +/** + * Policy severity enum — pinned to the backend's PolicySeverity constants. + * The backend CHECK constraint on policy_rules.severity enforces the same + * allowlist (migration 000013). The 4-value `medium` option that used to + * appear in the GUI was never a valid backend value and has been removed. + */ +export const POLICY_SEVERITIES = ['Warning', 'Error', 'Critical'] as const; +export type PolicySeverity = (typeof POLICY_SEVERITIES)[number]; + export interface PolicyRule { id: string; name: string; - type: string; - severity: string; + type: PolicyType; + severity: PolicySeverity; config: Record; enabled: boolean; created_at: string; @@ -127,7 +153,7 @@ export interface PolicyViolation { id: string; rule_id: string; certificate_id: string; - severity: string; + severity: PolicySeverity; message: string; created_at: string; } diff --git a/web/src/pages/PoliciesPage.tsx b/web/src/pages/PoliciesPage.tsx index 483c305..073d521 100644 --- a/web/src/pages/PoliciesPage.tsx +++ b/web/src/pages/PoliciesPage.tsx @@ -6,22 +6,40 @@ import DataTable from '../components/DataTable'; import type { Column } from '../components/DataTable'; import ErrorState from '../components/ErrorState'; import { formatDateTime } from '../api/utils'; -import type { PolicyRule } from '../api/types'; +import { + POLICY_TYPES, + POLICY_SEVERITIES, + type PolicyRule, + type PolicyType, + type PolicySeverity, +} from '../api/types'; -const severityStyles: Record = { - low: 'badge-info', - medium: 'badge-warning', - high: 'badge-danger', - critical: 'badge-danger', +/** + * Severity → badge style. Keyed on the backend's TitleCase PolicySeverity + * enum values (D-006). The pre-fix map keyed on `low`/`medium`/`high`/`critical` + * which never matched the backend's `Warning`/`Error`/`Critical`, so every + * existing rule fell through to the `badge-neutral` default. + */ +const severityStyles: Record = { + Warning: 'badge-warning', + Error: 'badge-danger', + Critical: 'badge-danger', }; -const severityDots: Record = { - low: 'bg-emerald-500', - medium: 'bg-amber-500', - high: 'bg-orange-500', - critical: 'bg-red-500', +const severityDots: Record = { + Warning: 'bg-amber-500', + Error: 'bg-orange-500', + Critical: 'bg-red-500', }; +/** + * Convert TitleCase enum value to a human-readable label for display. + * "AllowedIssuers" → "Allowed Issuers" + */ +function humanize(s: string): string { + return s.replace(/([A-Z])/g, ' $1').trim(); +} + interface CreatePolicyModalProps { isOpen: boolean; onClose: () => void; @@ -32,8 +50,8 @@ interface CreatePolicyModalProps { function CreatePolicyModal({ isOpen, onClose, onSuccess, isLoading, error }: CreatePolicyModalProps) { const [name, setName] = useState(''); - const [type, setType] = useState('key_algorithm'); - const [severity, setSeverity] = useState('medium'); + const [type, setType] = useState(POLICY_TYPES[0]); + const [severity, setSeverity] = useState('Warning'); const [configStr, setConfigStr] = useState('{}'); const [enabled, setEnabled] = useState(true); @@ -43,8 +61,8 @@ function CreatePolicyModal({ isOpen, onClose, onSuccess, isLoading, error }: Cre const config = JSON.parse(configStr); await createPolicy({ name: name.trim(), type, severity, config, enabled }); setName(''); - setType('key_algorithm'); - setSeverity('medium'); + setType(POLICY_TYPES[0]); + setSeverity('Warning'); setConfigStr('{}'); setEnabled(true); onSuccess(); @@ -72,27 +90,24 @@ function CreatePolicyModal({ isOpen, onClose, onSuccess, isLoading, error }: Cre
@@ -182,7 +197,7 @@ export default function PoliciesPage() {
), }, - { key: 'type', label: 'Type', render: (p) => {p.type.replace(/_/g, ' ')} }, + { key: 'type', label: 'Type', render: (p) => {humanize(p.type)} }, { key: 'severity', label: 'Severity', @@ -248,8 +263,8 @@ export default function PoliciesPage() { {Object.entries(bySeverity).map(([sev, count]) => (
-
- {sev} +
+ {sev} {count}
))}