Compare commits

..

10 Commits

Author SHA1 Message Date
shankar0123 36e722ba12 WIP: M-1 handler sentinel error mapping (checkpoint before branch cleanup)
Uncommitted migration work at the time of branch cleanup. Tagged as
checkpoint/m1-migration-wip so the commit survives git gc --prune=now.

Session context: Phase 3 Part B+C of the M-1 sentinel error migration
was in progress. 38 modified files, 4 new files (errors.go + errors_test.go
in internal/service/ and internal/api/handler/). Resume from this commit
via 'git checkout checkpoint/m1-migration-wip'.
2026-04-24 00:35:12 +00:00
shankar0123 d6959a75c1 Merge branch 'test/l1-repo-integration-coverage' 2026-04-20 20:39:10 +00:00
shankar0123 97b23e98d9 test(repository): close L-1 integration-coverage gap for HealthCheck + RenewalPolicy
The coverage-gap audit flagged L-1 (P2): `HealthCheckRepository` (453 LOC,
11 methods) and `RenewalPolicyRepository` (289 LOC, 5 methods post-G-1 —
the audit's "92 lines, 2 methods" figure was stale) ship to production
with zero live-DB integration coverage. The existing `repo_test.go`
header self-documents the gap: "15 of 17 PostgreSQL repository files".

Operationally load-bearing piece: M48's scheduler calls
`HealthCheckRepository.ListDueForCheck` every tick to drive continuous
TLS health monitoring. A silent SQL regression there — wrong INTERVAL
math, NULL-handling slip, lost ORDER BY — would fail open: operator
adds endpoint → scheduler never picks it up → endpoint degrades in
production → no alert. The loop continues ticking and logs "processed
0 endpoints" normally, so the failure mode is operationally invisible.

Closure shape (test-only; no production code touched):

- internal/repository/postgres/health_check_test.go (new file, 7 tests)
  · TestHealthCheckRepository_CRUD
  · TestHealthCheckRepository_GetByEndpoint
  · TestHealthCheckRepository_List_Filters
  · TestHealthCheckRepository_ListDueForCheck  (the load-bearing one —
    seeds four rows with differing last_checked_at+interval
    relationships to NOW() plus one NULL-last_checked_at row,
    asserts the correct subset returns and ORDER BY last_checked_at
    ASC NULLS FIRST holds)
  · TestHealthCheckRepository_RecordHistory_GetHistory
  · TestHealthCheckRepository_PurgeHistory
  · TestHealthCheckRepository_GetSummary

- internal/repository/postgres/renewal_policy_test.go (new file, 3 tests)
  · TestRenewalPolicyRepository_CRUD  (exercises auto-generated
    rp-<slug(name)> PK, JSONB round-trip of [30,14,7,0] thresholds,
    UpdatedAt monotonic advance, ORDER BY name for List)
  · TestRenewalPolicyRepository_DuplicateName  (asserts
    errors.Is(err, repository.ErrRenewalPolicyDuplicateName) on both
    Create-name-unique and Update-name-unique collision paths, the pg
    23505 sentinel mapping)
  · TestRenewalPolicyRepository_DeleteInUse  (raw-INSERTs a
    managed_certificates row FK'ing the policy, asserts
    errors.Is(err, repository.ErrRenewalPolicyInUse) from pg 23503
    ON DELETE RESTRICT, cleans up, then asserts not-found surfaces
    distinctly)

- internal/repository/postgres/repo_test.go (one-line header flip)
  "covering 15 of 17 ... repository files" → "17 of 17"; added
  cross-reference pointing readers at the two sibling files.

Both new files use the existing getTestDB(t) + schema-per-test-isolation
convention and skip via testing.Short() in CI, matching M26 TICKET-003
scaffolding byte-for-byte. Repository/postgres is not in the CI
coverage-gate path (grep -nE "internal/repository/postgres"
.github/workflows/ci.yml → no hits), so adding test-only files cannot
regress gated coverage elsewhere.

Verification gates run locally (sandbox without Docker, so the -short
skip gate itself is what's exercised; operator runs the testcontainer
path locally):

  1.  go vet ./...                                              — clean
  2.  go build ./...                                            — clean
  3.  go test -short -count=1 ./...                             — clean
  4.  go test -race -short ./internal/repository/postgres/...   — clean
  5.  staticcheck                         — absent; CI checkset holds
  6.  govulncheck                         — skipped; test-only, no deps
  7.  per-layer coverage no-regression    — N/A; repo/pg not gated
  8.  tsc --noEmit                        — N/A; no frontend change
  9.  vitest run                          — N/A; no frontend change
  10. vite build                          — N/A; no frontend change
  11. OpenAPI lint                        — N/A; no spec change

No migration, no interface change, no production code diff. The
RenewalPolicyRepository drift between audit ("92 lines, 2 methods")
and HEAD (289 lines, 5 methods post-G-1) is documented honestly in
the audit report's Resolution Log, not papered over.

Closes: coverage-gap-audit L-1 (P2)
2026-04-20 20:39:06 +00:00
shankar0123 4cf5fcdb4f Merge branch 'fix/d1-cli-status-endpoint' 2026-04-20 19:41:03 +00:00
shankar0123 1ee67b7792 D-1: correct certctl-cli status endpoint path (/api/v1/health -> /health)
The CLI's GetStatus() was issuing GET /api/v1/health, but the real
liveness route is GET /health at internal/api/router/router.go:76
(mounted at root, not under /api/v1/). Every 'certctl-cli status'
invocation 404'd since M16b.

The regression was masked because TestClient_GetStatus encoded the
same wrong path on both sides of the contract -- the mock server
also dispatched on /api/v1/health -- so the production request
matched the test's buggy dispatch and the green bar hid the bug.

Two-line fix:
  - internal/cli/client.go:615: "/api/v1/health" -> "/health"
  - internal/cli/client_test.go:296: mock dispatch to match

Red receipt captured before the green fix: with the test fixture
corrected but production still wrong, TestClient_GetStatus fails
'parsing response: unexpected end of JSON input' (the client falls
through the mock's if/else to the default 200 OK empty body and
the JSON decoder chokes). After the production edit the test
passes.

GetStatus()'s response decoder is already compatible with the real
/health shape (graceful 'ok' check on health["status"], optional
health["timestamp"]). No interface change. No migration. No
frontend change. No OpenAPI delta -- /health is a root-level
liveness probe, not part of the /api/v1/ surface.
2026-04-20 19:40:58 +00:00
shankar0123 128d0eeaa8 Merge branch 'fix/g1-renewal-policies-api'
G-1: renewal-policies API + frontend FK-drift fix. Adds /api/v1/renewal-policies
CRUD backing the dropdown that managed_certificates.renewal_policy_id FKs into.
Three frontend call sites swapped from getPolicies() (pol-*, compliance rules)
to getRenewalPolicies() (rp-*, lifecycle policies). Validation bounds, pg
23503/23505 error mapping to HTTP 409, OpenAPI coverage, test suite.

No migration — renewal_policies table already exists from schema 000001.
2026-04-20 18:53:09 +00:00
shankar0123 9834b4e4a4 G-1: renewal-policies API + frontend FK-drift fix
Three frontend call sites (OnboardingWizard.tsx:603, CertificatesPage.tsx:52,
CertificateDetailPage.tsx:169) populated the renewal_policy_id dropdown from
getPolicies() — the compliance-rule endpoint returning pol-* IDs — which
violated the FK managed_certificates.renewal_policy_id REFERENCES
renewal_policies(id) ON DELETE RESTRICT. Create would fail pg 23503 at insert.

Backend (new):
- RenewalPolicyRepository CRUD + ListAll/ExistsByID (pg 23503 → ErrRenewalPolicyInUse
  → HTTP 409; pg 23505 → ErrRenewalPolicyDuplicateName → HTTP 409)
- RenewalPolicyService with repo-only constructor. Service sentinels
  var-alias the repo sentinels so errors.Is walks across layers.
- RenewalPolicyHandler with validation bounds: name 1–255;
  renewal_window_days [1,365] default 30; max_retries [0,10] not defaulted;
  retry_interval_seconds [60,86400] default 3600; alert_thresholds_days
  [0,365] default [30,14,7,0]. Auto-generated IDs rp-<slug(name)>.
- Router registers 5 routes under /api/v1/renewal-policies[/{id}].

Frontend:
- CertificatesPage/CertificateDetailPage/OnboardingWizard now call
  getRenewalPolicies() and render rp-* IDs.
- client.ts adds getRenewalPolicies/createRenewalPolicy/updateRenewalPolicy/
  deleteRenewalPolicy. types.ts adds the RenewalPolicy shape.

OpenAPI: RenewalPolicies tag + 5 operations + 3 schemas (RenewalPolicy,
RenewalPolicyCreateRequest, RenewalPolicyUpdateRequest). 409 responses
on create/update duplicate-name and delete FK-in-use.

No migration — renewal_policies table already exists from the initial
schema (000001).

Tests:
- internal/service/renewal_policy_test.go: CRUD + validation + sentinel
  error wrapping.
- internal/api/handler/renewal_policy_handler_test.go: handler endpoint
  contracts including 400/404/409.
- web/src/api/client.test.ts: 4 subtests covering the 4 new API functions.

Phase 3 gates all green: go vet, build, short tests, race tests (service/
handler/router/scheduler), staticcheck (G-1 packages), govulncheck (0
reachable), coverage (service 69.7%, handler 79.0%, domain 86.9%,
middleware 80.6% — all above thresholds), tsc, vitest (256 passed),
vite build, OpenAPI structural validation.
2026-04-20 18:53:01 +00:00
shankar0123 cab579368b Merge branch 'fix/audit-f001-f002-f003'
Closes F-001 (CRL scoped query via composite index), F-002 (digest error
body sanitization), and F-003 (ctx-aware sleep at three sites).

Verification: build, vet, race-short test sweep across all packages green.
govulncheck clean. golangci-lint run deferred — local environment's
golangci-lint is v1.64.8 built with go1.24 and rejects the go1.25.9
project; fresh install blocked by disk constraints. CI lane will cover it.
2026-04-20 16:52:00 +00:00
shankar0123 4e5522a999 F-001/F-002/F-003: CRL prefix-scan, digest error sanitization, ctx-aware sleeps
F-001 (P3): GenerateDERCRL scoped to issuer via composite index
  - Add RevocationRepository.ListByIssuer leveraging migration 000012's
    idx_certificate_revocations_issuer_serial composite index as a
    prefix-scan target. Previously CAOperationsSvc.GenerateDERCRL called
    ListAll() and filtered by IssuerID in Go — O(total revocations)
    regardless of how many revocations belonged to the target issuer.
  - Rewrite GenerateDERCRL to call ListByIssuer(ctx, issuerID) so PostgreSQL
    drives a prefix scan of the composite index. Drops the in-memory filter.
  - New regression test in ca_operations_test.go asserts the CRL hot path
    invokes ListByIssuer exactly once and never ListAll, and that the
    issuerID is threaded through correctly.

F-002 (P3): digest.go admin-auth endpoints no longer leak internal errors
  - PreviewDigest (GET /api/v1/digest/preview) and SendDigest
    (POST /api/v1/digest/send) previously wrote err.Error() into the HTTP
    response body on 500s. Replace with slog.Error server-side logging plus
    a generic "internal error" response body, matching the house pattern
    in certificates.go and export.go.

F-003 (P4): three blocking time.Sleep sites now honor ctx cancellation
  - internal/connector/issuer/acme/acme.go:672 (DNS-01 propagation wait)
    now runs under a select{case <-ctx.Done(): CleanUp + return ctx.Err();
    case <-time.After(d):} so graceful shutdown doesn't get stuck behind
    the propagation delay.
  - internal/connector/issuer/acme/acme.go:786 (dns-persist-01 propagation
    wait) same pattern, returns ctx.Err() on cancel.
  - cmd/agent/main.go:272 (polling backoff inside the heartbeat loop) now
    wraps the sleep in select{case <-ctx.Done(): continue; case <-time.After(backoff):}
    so the outer <-ctx.Done() case on the parent loop fires cleanly.

Verification: build, vet, and race-enabled short tests green across all
55+ packages. govulncheck reports zero vulnerabilities in the code path.
No migration needed — F-001 reuses the existing 000012 composite index.
No frontend changes.
2026-04-20 16:51:52 +00:00
shankar0123 55ce86b132 v2.0.48: swap self-signed TLS bootstrap algorithm ed25519 → ECDSA-P256
Follow-up to v2.0.47 (HTTPS-Everywhere). The Phase-3 self-signed
bootstrap sidecar shipped an ed25519 server cert. Apple's TLS stack —
Safari Network Framework and the macOS-bundled LibreSSL 3.3.6
/usr/bin/curl — does not advertise ed25519 in the ClientHello
signature_algorithms extension for server certs, so the handshake fails
with the server-side log line:

  tls: peer doesn't support any of the certificate's signature algorithms

Homebrew OpenSSL 3.x, Chrome, Firefox, and Linux curl all accept
ed25519 server certs fine. Apple is the outlier. Rather than gate the
demo stack behind "install Homebrew OpenSSL first," swap the bootstrap
algorithm to ECDSA-P256 with SHA-256 — universally supported, including
on the Apple stack.

Changes
- deploy/docker-compose.yml: certctl-tls-init openssl invocation swapped
  to `-newkey ec -pkeyopt ec_paramgen_curve:P-256 -nodes`; header comment
  + echo line updated; multi-line rationale paragraph added.
- deploy/docker-compose.test.yml: same openssl swap + echo update for
  the test harness sidecar that writes to the bind-mounted ./test/certs
  directory the Go integration_test.go pins via CERTCTL_TEST_CA_BUNDLE.
- docs/tls.md: Pattern 1 description + code block updated;
  "Why ECDSA-P256 and not ed25519" rationale paragraph added covering
  pre-v2.0.48 history, the Apple diagnosis, accepting clients, and
  the operator migration command. Patterns 2 (existing Secret) and 3
  (cert-manager) explicitly called out as unaffected.
- docs/upgrade-to-tls.md: docker-compose procedure sentence updated
  with cross-reference to tls.md Pattern 1.
- docs/test-env.md: "Get the CA bundle for curl" sentence updated.

Migration
Existing demo installs must tear the `certs` named volume down to pick
up the new algorithm:

  docker compose -f deploy/docker-compose.yml down -v
  docker compose -f deploy/docker-compose.yml up -d --build

Not touched
- cmd/server/tls.go: algorithm-agnostic. TLS 1.3 min version with
  [X25519, P-256] curve preferences for key exchange is orthogonal to
  the server cert's signature algorithm. No Go code change needed.
- Helm chart: Patterns 2 and 3 operators supply their own cert; this
  patch does not affect them.
- Unrelated ed25519 uses (agent key algorithm detection, profile
  algorithm options, SSH key path examples, tlsprobe key metadata,
  cloud discovery key-algo display): all orthogonal to the server TLS
  bootstrap cert.

Incidental cleanup
- .gitignore: dropped dangling `strategy.md` entry (file doesn't exist
  in repo; entry was cruft).
2026-04-20 04:17:05 +00:00
72 changed files with 4261 additions and 354 deletions
-1
View File
@@ -66,7 +66,6 @@ certctl-cli
/mcp-server
# Private strategy docs
strategy.md
SECURITY_REMEDIATION.md
# OS
+259
View File
@@ -42,6 +42,8 @@ tags:
description: Job queue — issuance, renewal, deployment, validation
- name: Policies
description: Policy rules and violation tracking
- name: RenewalPolicies
description: Lifecycle renewal policies (distinct from compliance policy rules above)
- name: Profiles
description: Certificate enrollment profiles with crypto constraints
- name: Teams
@@ -1528,6 +1530,137 @@ paths:
"500":
$ref: "#/components/responses/InternalError"
# ─── Renewal Policies ────────────────────────────────────────────────
# G-1: lifecycle policies (rp-* ids, table renewal_policies). DISTINCT from
# /api/v1/policies above, which returns compliance rules (pol-* ids, table
# policy_rules). `managed_certificates.renewal_policy_id` FK points at
# renewal_policies(id) — populating that dropdown from /api/v1/policies
# caused 23503 FK violations; hence this endpoint.
/api/v1/renewal-policies:
get:
tags: [RenewalPolicies]
summary: List renewal policies
operationId: listRenewalPolicies
parameters:
- $ref: "#/components/parameters/page"
- $ref: "#/components/parameters/per_page"
responses:
"200":
description: Paginated list of renewal policies
content:
application/json:
schema:
allOf:
- $ref: "#/components/schemas/PaginationEnvelope"
- type: object
properties:
data:
type: array
items:
$ref: "#/components/schemas/RenewalPolicy"
"500":
$ref: "#/components/responses/InternalError"
post:
tags: [RenewalPolicies]
summary: Create renewal policy
operationId: createRenewalPolicy
requestBody:
required: true
content:
application/json:
schema:
$ref: "#/components/schemas/RenewalPolicyCreateRequest"
responses:
"201":
description: Renewal policy created
content:
application/json:
schema:
$ref: "#/components/schemas/RenewalPolicy"
"400":
$ref: "#/components/responses/BadRequest"
"409":
description: Duplicate policy name
content:
application/json:
schema:
$ref: "#/components/schemas/Error"
"500":
$ref: "#/components/responses/InternalError"
/api/v1/renewal-policies/{id}:
get:
tags: [RenewalPolicies]
summary: Get renewal policy
operationId: getRenewalPolicy
parameters:
- $ref: "#/components/parameters/resourceId"
responses:
"200":
description: Renewal policy details
content:
application/json:
schema:
$ref: "#/components/schemas/RenewalPolicy"
"400":
$ref: "#/components/responses/BadRequest"
"404":
$ref: "#/components/responses/NotFound"
"500":
$ref: "#/components/responses/InternalError"
put:
tags: [RenewalPolicies]
summary: Update renewal policy
operationId: updateRenewalPolicy
parameters:
- $ref: "#/components/parameters/resourceId"
requestBody:
required: true
content:
application/json:
schema:
$ref: "#/components/schemas/RenewalPolicyUpdateRequest"
responses:
"200":
description: Renewal policy updated
content:
application/json:
schema:
$ref: "#/components/schemas/RenewalPolicy"
"400":
$ref: "#/components/responses/BadRequest"
"404":
$ref: "#/components/responses/NotFound"
"409":
description: Duplicate policy name
content:
application/json:
schema:
$ref: "#/components/schemas/Error"
"500":
$ref: "#/components/responses/InternalError"
delete:
tags: [RenewalPolicies]
summary: Delete renewal policy
operationId: deleteRenewalPolicy
parameters:
- $ref: "#/components/parameters/resourceId"
responses:
"204":
description: Renewal policy deleted
"400":
$ref: "#/components/responses/BadRequest"
"404":
$ref: "#/components/responses/NotFound"
"409":
description: Policy in use by one or more certificates (FK restrict)
content:
application/json:
schema:
$ref: "#/components/schemas/Error"
"500":
$ref: "#/components/responses/InternalError"
# ─── Profiles ────────────────────────────────────────────────────────
/api/v1/profiles:
get:
@@ -3765,6 +3898,132 @@ components:
type: string
format: date-time
# ─── Renewal Policies ─────────────────────────────────────────────
# G-1: renewal_policies table — lifecycle policies, referenced by
# managed_certificates.renewal_policy_id ON DELETE RESTRICT. Distinct
# from PolicyRule above (compliance rules, table policy_rules).
RenewalPolicy:
type: object
required:
- id
- name
- renewal_window_days
- auto_renew
- max_retries
- retry_interval_seconds
- alert_thresholds_days
- created_at
- updated_at
properties:
id:
type: string
description: Human-readable ID, prefixed `rp-` (e.g., `rp-default`).
name:
type: string
description: Unique display name (UNIQUE in DB).
renewal_window_days:
type: integer
minimum: 1
maximum: 365
description: Days before expiry to trigger renewal.
auto_renew:
type: boolean
description: Whether renewal is triggered automatically by the scheduler.
max_retries:
type: integer
minimum: 0
maximum: 10
description: Maximum renewal retry attempts on failure.
retry_interval_seconds:
type: integer
minimum: 60
maximum: 86400
description: Seconds to wait between retry attempts.
alert_thresholds_days:
type: array
items:
type: integer
minimum: 0
maximum: 365
description: Days-before-expiry thresholds at which to emit alerts.
certificate_profile_id:
type: string
nullable: true
description: Optional certificate profile binding. Read-only at this endpoint; UI does not currently edit this field.
created_at:
type: string
format: date-time
updated_at:
type: string
format: date-time
RenewalPolicyCreateRequest:
type: object
required:
- name
properties:
id:
type: string
description: Optional human-readable ID. Auto-generated from name when omitted.
name:
type: string
minLength: 1
maxLength: 255
renewal_window_days:
type: integer
minimum: 1
maximum: 365
default: 30
auto_renew:
type: boolean
default: true
max_retries:
type: integer
minimum: 0
maximum: 10
description: Required. Not defaulted — 0 is a valid operator choice.
retry_interval_seconds:
type: integer
minimum: 60
maximum: 86400
default: 3600
alert_thresholds_days:
type: array
items:
type: integer
minimum: 0
maximum: 365
default: [30, 14, 7, 0]
RenewalPolicyUpdateRequest:
type: object
description: Partial update. Omitted fields are left unchanged.
properties:
name:
type: string
minLength: 1
maxLength: 255
renewal_window_days:
type: integer
minimum: 1
maximum: 365
auto_renew:
type: boolean
max_retries:
type: integer
minimum: 0
maximum: 10
retry_interval_seconds:
type: integer
minimum: 60
maximum: 86400
alert_thresholds_days:
type: array
items:
type: integer
minimum: 0
maximum: 365
# ─── Profiles ────────────────────────────────────────────────────
CertificateProfile:
type: object
+8 -1
View File
@@ -269,7 +269,14 @@ func (a *Agent) Run(ctx context.Context) error {
a.logger.Warn("backing off due to consecutive failures",
"failures", a.consecutiveFailures,
"backoff", backoff.String())
time.Sleep(backoff)
// F-003: ctx-aware wait so graceful shutdown does not stall on
// a long backoff. If ctx cancels mid-backoff, return to the
// outer loop so the <-ctx.Done() case can trigger clean exit.
select {
case <-ctx.Done():
continue
case <-time.After(backoff):
}
}
a.pollForWork(ctx)
+10
View File
@@ -147,6 +147,11 @@ func main() {
auditService := service.NewAuditService(auditRepo)
policyService := service.NewPolicyService(policyRepo, auditService)
policyService.SetCertRepo(certificateRepo) // D-008: CertificateLifetime arm needs CertificateVersion.NotBefore/NotAfter
// G-1: RenewalPolicyService — distinct from PolicyService (compliance rules).
// Drives /api/v1/renewal-policies CRUD; the service layer owns slugify + validation,
// the repo layer owns sentinel translation for 23505 (name UNIQUE) and 23503
// (FK-RESTRICT against managed_certificates.renewal_policy_id).
renewalPolicyService := service.NewRenewalPolicyService(renewalPolicyRepo)
certificateService := service.NewCertificateService(certificateRepo, policyService, auditService)
notifierRegistry := make(map[string]service.Notifier)
@@ -368,6 +373,10 @@ func main() {
agentHandler := handler.NewAgentHandler(agentService)
jobHandler := handler.NewJobHandler(jobService)
policyHandler := handler.NewPolicyHandler(policyService)
// G-1: RenewalPolicyHandler — /api/v1/renewal-policies CRUD. Value-returning
// constructor matches the house pattern (PolicyHandler, IssuerHandler etc.);
// the registry stores it by value in HandlerRegistry.RenewalPolicies.
renewalPolicyHandler := handler.NewRenewalPolicyHandler(renewalPolicyService)
profileHandler := handler.NewProfileHandler(profileService)
teamHandler := handler.NewTeamHandler(teamService)
ownerHandler := handler.NewOwnerHandler(ownerService)
@@ -508,6 +517,7 @@ func main() {
Agents: agentHandler,
Jobs: jobHandler,
Policies: policyHandler,
RenewalPolicies: renewalPolicyHandler,
Profiles: profileHandler,
Teams: teamHandler,
Owners: ownerHandler,
+4 -2
View File
@@ -65,14 +65,16 @@ services:
echo "TLS cert already present at $$CERT — skipping generation"
else
mkdir -p /etc/certctl/tls
openssl req -x509 -newkey ed25519 -nodes \
openssl req -x509 -newkey ec \
-pkeyopt ec_paramgen_curve:P-256 \
-nodes \
-keyout "$$KEY" \
-out "$$CERT" \
-days 3650 \
-subj "/CN=certctl-server" \
-addext "subjectAltName=DNS:certctl-server,DNS:localhost,IP:127.0.0.1,IP:::1"
cp "$$CERT" "$$CA"
echo "Generated self-signed TLS cert for certctl-test-server (ed25519, 3650d, CN=certctl-server)"
echo "Generated self-signed TLS cert for certctl-test-server (ECDSA-P256/SHA-256, 3650d, CN=certctl-server)"
fi
# The test server container runs as root (see `user: "0:0"` below)
# because setup-trust.sh needs to update the system trust store, so
+19 -9
View File
@@ -1,12 +1,20 @@
services:
# HTTPS-Everywhere Phase 3 — self-signed TLS bootstrap (init container).
# Generates a CN=certctl-server ed25519 cert with the SAN list locked by
# milestone §3.6 on first boot; subsequent boots see the cert already
# present in the `certs` named volume and no-op out. Server + agent mount
# the volume read-only. Destroy via `docker compose down -v` to force
# regeneration. This bootstrap is for docker-compose demos and local dev
# only; Helm operators supply a Secret / cert-manager Certificate per
# docs/tls.md.
# Generates a CN=certctl-server ECDSA-P256 (SHA-256 signature) cert with
# the SAN list locked by milestone §3.6 on first boot; subsequent boots
# see the cert already present in the `certs` named volume and no-op out.
# Server + agent mount the volume read-only. Destroy via `docker compose
# down -v` to force regeneration. This bootstrap is for docker-compose
# demos and local dev only; Helm operators supply a Secret / cert-manager
# Certificate per docs/tls.md.
#
# Rationale for ECDSA-P256 (was ed25519 pre-v2.0.48): Apple's TLS stack
# — Safari Network Framework and the macOS-bundled LibreSSL 3.3.6
# /usr/bin/curl — does not advertise ed25519 in the ClientHello
# signature_algorithms extension for server certs, yielding "tls: peer
# doesn't support any of the certificate's signature algorithms" at
# handshake. ECDSA-P256 with SHA-256 is universally supported. See
# docs/tls.md Pattern 1.
certctl-tls-init:
image: alpine/openssl:latest
container_name: certctl-tls-init
@@ -23,14 +31,16 @@ services:
echo "TLS cert already present at $$CERT — skipping generation"
else
mkdir -p /etc/certctl/tls
openssl req -x509 -newkey ed25519 -nodes \
openssl req -x509 -newkey ec \
-pkeyopt ec_paramgen_curve:P-256 \
-nodes \
-keyout "$$KEY" \
-out "$$CERT" \
-days 3650 \
-subj "/CN=certctl-server" \
-addext "subjectAltName=DNS:certctl-server,DNS:localhost,IP:127.0.0.1,IP:::1"
cp "$$CERT" "$$CA"
echo "Generated self-signed TLS cert for certctl-server (ed25519, 3650d, CN=certctl-server)"
echo "Generated self-signed TLS cert for certctl-server (ECDSA-P256/SHA-256, 3650d, CN=certctl-server)"
fi
# certctl binary runs as UID 1000 inside the server container per
# Dockerfile:64-65; the cert + key must be readable by that UID.
+1 -1
View File
@@ -161,7 +161,7 @@ certctl-test-stepca Up (healthy)
### Get the CA bundle for curl
The test harness runs HTTPS-only (the `certctl-tls-init` init container self-signs an ed25519 server cert into a bind-mounted directory before the server starts — see `docker-compose.test.yml` §`certctl-tls-init` for details). The CA cert that signed it is materialized on the host at `./test/certs/ca.crt` (relative to the `deploy/` directory). Every `curl` in the rest of this doc expects it in `$CA`:
The test harness runs HTTPS-only (the `certctl-tls-init` init container self-signs an ECDSA-P256 server cert with a SHA-256 signature into a bind-mounted directory before the server starts — see `docker-compose.test.yml` §`certctl-tls-init` for details). The CA cert that signed it is materialized on the host at `./test/certs/ca.crt` (relative to the `deploy/` directory). Every `curl` in the rest of this doc expects it in `$CA`:
```bash
export CA=$PWD/test/certs/ca.crt
+6 -2
View File
@@ -19,10 +19,12 @@ Both paths are read during a fail-loud preflight in `cmd/server/main.go` (see `p
This is the default for the `deploy/docker-compose.yml` stack. It exists so `docker compose up -d --build` just works on a laptop without the operator standing up a CA first. It is not appropriate for any non-demo environment.
An init container named `certctl-tls-init` runs once before the server starts. It uses the `alpine/openssl` image and generates an ed25519 self-signed cert:
An init container named `certctl-tls-init` runs once before the server starts. It uses the `alpine/openssl` image and generates an ECDSA-P256 self-signed cert (SHA-256 signature):
```
openssl req -x509 -newkey ed25519 -nodes \
openssl req -x509 -newkey ec \
-pkeyopt ec_paramgen_curve:P-256 \
-nodes \
-keyout /etc/certctl/tls/server.key \
-out /etc/certctl/tls/server.crt \
-days 3650 \
@@ -30,6 +32,8 @@ openssl req -x509 -newkey ed25519 -nodes \
-addext "subjectAltName=DNS:certctl-server,DNS:localhost,IP:127.0.0.1,IP:::1"
```
**Why ECDSA-P256 and not ed25519.** The pre-v2.0.48 demo bootstrap used ed25519 (small keys, fast signatures). Apple's TLS stack — Safari Network Framework and the macOS-bundled LibreSSL 3.3.6 `/usr/bin/curl` — does not advertise ed25519 in the ClientHello `signature_algorithms` extension for server certs, so an ed25519 server cert was rejected at handshake with `tls: peer doesn't support any of the certificate's signature algorithms` on the server side (and the generic TLS handshake error on the client side). Homebrew OpenSSL 3.x, Chrome, Firefox, and Linux curl all accepted ed25519 — Apple was the outlier. ECDSA-P256 with SHA-256 is universally supported, so the demo bootstrap uses it by default. To pick up the new algorithm on an existing demo install, tear the volume down and rebuild: `docker compose -f deploy/docker-compose.yml down -v && docker compose -f deploy/docker-compose.yml up -d --build`. **Helm and operator-supplied-Secret users (Patterns 2 and 3) are unaffected** — they bring their own cert, and `cmd/server/tls.go` is algorithm-agnostic (TLS 1.3 with curve preference `[X25519, P-256]` for key exchange — no constraint on the server cert's signature algorithm).
The cert, its matching key, and a copy of the cert published as `ca.crt` land in a named volume (`certs`) mounted at `/etc/certctl/tls/` in the server container (read-only) and the agent container (read-only). The bootstrap is idempotent — if `server.crt`, `server.key`, and `ca.crt` are already present on the volume, the init container logs `TLS cert already present at …` and exits cleanly.
Single-cert design. CN is `certctl-server` to match the Docker-network hostname. The SAN list is `[certctl-server, localhost, 127.0.0.1, ::1]`, which covers both container-internal agent→server traffic and operator browser/curl access to `https://localhost:8443`. There is no separate intermediate/root chain — the server cert and the CA bundle are the same PEM. This is the whole point of a demo bootstrap.
+1 -1
View File
@@ -22,7 +22,7 @@ There is no schema migration tied to this release; the only at-rest state that c
## Procedure — docker-compose operators
The shipped `deploy/docker-compose.yml` includes a `certctl-tls-init` init container that self-signs an ed25519 cert on first boot and drops `server.crt`, `server.key`, and `ca.crt` into a named volume mounted read-only at `/etc/certctl/tls/` on the server and agent containers. No manual cert provisioning is required for the default stack.
The shipped `deploy/docker-compose.yml` includes a `certctl-tls-init` init container that self-signs an ECDSA-P256 (SHA-256 signature) cert on first boot and drops `server.crt`, `server.key`, and `ca.crt` into a named volume mounted read-only at `/etc/certctl/tls/` on the server and agent containers. No manual cert provisioning is required for the default stack. (Pre-v2.0.48 this was an ed25519 cert; see [`tls.md`](tls.md) Pattern 1 for the rationale and the `down -v && up --build` migration note.)
1. **Pull the HTTPS-everywhere release.** From the repo root:
+58 -9
View File
@@ -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"
)
// AgentGroupService defines the service interface for agent group operations.
@@ -22,6 +24,19 @@ type AgentGroupService interface {
}
// AgentGroupHandler handles HTTP requests for agent group operations.
//
// Error dispatch (post-M-1): every service error routes through the [errToStatus]
// choke point via `errors.Is` walking the wrap chain, with one explicit
// [service.ErrValidation] arm on the write paths (Create, Update) so the
// composed "validation: <field-specific reason>" message the service layer
// attaches via `fmt.Errorf("%w: ...", ErrValidation)` can be passed through to
// the 400 response body. Before M-1, the Create handler branched on
// `strings.Contains(err.Error(), "invalid"|"required")` — fragile because a
// single reword in [service.validateAgentGroup] would demote the 400 to 500
// with no compile-time signal — and the Update/Delete handlers branched on
// `strings.Contains(err.Error(), "not found")`, coupling HTTP classification
// to repository human-readable strings. Both now ride the typed
// [repository.ErrNotFound] wrap through errToStatus. Mirrors ProfileHandler.
type AgentGroupHandler struct {
svc AgentGroupService
}
@@ -89,7 +104,18 @@ func (h AgentGroupHandler) GetAgentGroup(w http.ResponseWriter, r *http.Request)
group, err := h.svc.GetAgentGroup(r.Context(), id)
if err != nil {
ErrorWithRequestID(w, http.StatusNotFound, "Agent group not found", requestID)
// M-1: route through errToStatus so a repo-level `sql.ErrNoRows`
// (wrapped as repository.ErrNotFound) becomes 404, but a transient DB
// failure no longer masquerades as 404 — it correctly surfaces 500. The
// pre-M-1 "any error → 404" shortcut was plausible when Get's only
// expected failure was "not found", but the choke point now gives us
// correct dispatch for free. Mirrors GetProfile.
status := errToStatus(err)
msg := "Failed to get agent group"
if status == http.StatusNotFound {
msg = "Agent group not found"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -123,7 +149,15 @@ func (h AgentGroupHandler) CreateAgentGroup(w http.ResponseWriter, r *http.Reque
created, err := h.svc.CreateAgentGroup(r.Context(), group)
if err != nil {
if strings.Contains(err.Error(), "invalid") || strings.Contains(err.Error(), "required") {
// M-1: replace the 2-term substring net (`"invalid"|"required"`) with a
// single `errors.Is(err, service.ErrValidation)` arm. validateAgentGroup
// wraps every field-specific failure via `fmt.Errorf("%w: <reason>",
// ErrValidation)`, so `err.Error()` still contains the human-readable
// reason (e.g., "agent group name is required") and can be safely passed
// to the 400 body — but the status decision no longer depends on the
// exact wording. Other errors redact to a generic 500. Mirrors
// CreateProfile.
if errors.Is(err, service.ErrValidation) {
ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID)
return
}
@@ -160,11 +194,22 @@ func (h AgentGroupHandler) UpdateAgentGroup(w http.ResponseWriter, r *http.Reque
updated, err := h.svc.UpdateAgentGroup(r.Context(), id, group)
if err != nil {
if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Agent group not found", requestID)
// M-1: explicit ErrValidation arm preserves the user-facing reason in
// the 400 body (validateAgentGroup wraps every failure via
// `fmt.Errorf("%w: ...", ErrValidation)`); every other error — including
// repo-layer ErrNotFound on a missing row — routes through errToStatus
// so the 404/500 decision no longer depends on substring matching.
// Mirrors UpdateProfile.
if errors.Is(err, service.ErrValidation) {
ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID)
return
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to update agent group", requestID)
status := errToStatus(err)
msg := "Failed to update agent group"
if status == http.StatusNotFound {
msg = "Agent group not found"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -188,11 +233,15 @@ func (h AgentGroupHandler) DeleteAgentGroup(w http.ResponseWriter, r *http.Reque
}
if err := h.svc.DeleteAgentGroup(r.Context(), id); err != nil {
if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Agent group not found", requestID)
return
// M-1: sentinel dispatch replaces the substring 404 check — see the
// parallel comment block in UpdateAgentGroup for the rationale. Mirrors
// DeleteProfile.
status := errToStatus(err)
msg := "Failed to delete agent group"
if status == http.StatusNotFound {
msg = "Agent group not found"
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to delete agent group", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
+53 -6
View File
@@ -108,7 +108,19 @@ func (h AgentHandler) GetAgent(w http.ResponseWriter, r *http.Request) {
agent, err := h.svc.GetAgent(r.Context(), id)
if err != nil {
ErrorWithRequestID(w, http.StatusNotFound, "Agent not found", requestID)
// M-1 (P2): route through errToStatus so a repo-level
// sql.ErrNoRows (wrapped as repository.ErrNotFound) becomes 404,
// but a transient DB failure no longer masquerades as 404 — it
// correctly surfaces 500. The pre-M-1 "any error → 404" shortcut
// was plausible when Get's only expected failure was "not found",
// but the choke point now gives us correct dispatch for free.
// Mirrors GetAgentGroup / GetProfile.
status := errToStatus(err)
msg := "Failed to get agent"
if status == http.StatusNotFound {
msg = "Agent not found"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -147,11 +159,20 @@ func (h AgentHandler) RegisterAgent(w http.ResponseWriter, r *http.Request) {
created, err := h.svc.RegisterAgent(r.Context(), agent)
if err != nil {
errMsg := err.Error()
if strings.Contains(errMsg, "unique") || strings.Contains(errMsg, "duplicate") || strings.Contains(errMsg, "already exists") {
// M-1 (P2): replace the 3-term substring net
// (`"unique"|"duplicate"|"already exists"`) with a typed
// errors.Is(err, service.ErrConflict) arm. The service layer now
// wraps pg SQLSTATE 23505 duplicate-name violations via
// fmt.Errorf("%w: agent name already exists", ErrConflict), so
// classification no longer depends on the exact driver wording.
// Other errors redact to a generic 500 with slog.Error server-
// side diagnostic capture (F-002). Mirrors CreateProfile's
// ErrValidation arm pattern, adapted for the conflict case.
if errors.Is(err, service.ErrConflict) {
ErrorWithRequestID(w, http.StatusConflict, "Agent with this name already exists", requestID)
return
}
slog.Error("RegisterAgent failed", "name", agent.Name, "error", err.Error())
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to register agent", requestID)
return
}
@@ -211,7 +232,15 @@ func (h AgentHandler) Heartbeat(w http.ResponseWriter, r *http.Request) {
ErrorWithRequestID(w, http.StatusGone, "Agent has been retired", requestID)
return
}
if strings.Contains(err.Error(), "not found") {
// M-1 (P2): the pre-M-1 `strings.Contains(err.Error(), "not
// found")` branch now rides the errToStatus choke point, which
// recognizes repository.ErrNotFound via errors.Is. The retired-
// agent sentinel is still checked FIRST above so the 410 Gone
// short-circuit is never masked by the 404 arm. Any other error
// redacts to a generic 500 with slog.Error server-side diagnostic
// capture (F-002). Mirrors GetAgentGroup.
status := errToStatus(err)
if status == http.StatusNotFound {
ErrorWithRequestID(w, http.StatusNotFound, "Agent not found", requestID)
return
}
@@ -308,7 +337,16 @@ func (h AgentHandler) AgentCertificatePickup(w http.ResponseWriter, r *http.Requ
certPEM, err := h.svc.CertificatePickup(r.Context(), agentID, certID)
if err != nil {
ErrorWithRequestID(w, http.StatusNotFound, "Certificate not found or not ready", requestID)
// M-1 (P2): route through errToStatus so a repo-level
// sql.ErrNoRows (wrapped as repository.ErrNotFound) becomes 404,
// but a transient DB failure no longer masquerades as 404 — it
// correctly surfaces 500. Mirrors GetAgent.
status := errToStatus(err)
msg := "Failed to retrieve certificate"
if status == http.StatusNotFound {
msg = "Certificate not found or not ready"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -491,7 +529,16 @@ func (h AgentHandler) RetireAgent(w http.ResponseWriter, r *http.Request) {
JSON(w, http.StatusConflict, body)
return
}
if strings.Contains(err.Error(), "not found") {
// M-1 (P2): the pre-M-1 `strings.Contains(err.Error(), "not
// found")` branch now rides the errToStatus choke point, which
// recognizes repository.ErrNotFound via errors.Is. The sentinel
// (ErrAgentIsSentinel, ErrForceReasonRequired) and typed
// (*BlockedByDependenciesError) checks above still run FIRST so
// the 403/400/409 structural refusals are never masked by the
// 404 arm. Any other error redacts to a generic 500 with
// slog.Error server-side diagnostic capture (F-002).
status := errToStatus(err)
if status == http.StatusNotFound {
ErrorWithRequestID(w, http.StatusNotFound, "Agent not found", requestID)
return
}
+19 -1
View File
@@ -2,6 +2,7 @@ package handler
import (
"context"
"log/slog"
"net/http"
"strconv"
"strings"
@@ -86,7 +87,24 @@ func (h AuditHandler) GetAuditEvent(w http.ResponseWriter, r *http.Request) {
event, err := h.svc.GetAuditEvent(r.Context(), id)
if err != nil {
ErrorWithRequestID(w, http.StatusNotFound, "Audit event not found", requestID)
// M-1 (P2): dispatch routes through errToStatus. Pre-M-1 this branch was
// a blanket `any error → 404 Audit event not found` shortcut — which
// silently demoted transient DB failures from the service's auditRepo.List
// wrap to 404 Not Found with no observable external signal. Post-M-1:
// service/audit.go GetAuditEvent only wraps the genuine zero-events path
// with service.ErrNotFound via %w, and the repo.List wrap surfaces without
// that sentinel — so errors.Is(err, service.ErrNotFound) picks up the real
// 404s and everything else correctly surfaces as 500 with server-side
// slog.Error capture (F-002 redacted-500 pattern preserved).
status := errToStatus(err)
if status == http.StatusInternalServerError {
slog.Error("GetAuditEvent failed", "audit_event_id", id, "error", err.Error())
}
msg := "Failed to get audit event"
if status == http.StatusNotFound {
msg = "Audit event not found"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
+48 -62
View File
@@ -298,12 +298,13 @@ func (h CertificateHandler) UpdateCertificate(w http.ResponseWriter, r *http.Req
updated, err := h.svc.UpdateCertificate(r.Context(), id, cert)
if err != nil {
if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Certificate not found", requestID)
return
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
slog.Error("UpdateCertificate failed", "cert_id", id, "error", err)
msg = "internal error"
}
slog.Error("UpdateCertificate failed", "cert_id", id, "error", err.Error())
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to update certificate", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -327,11 +328,13 @@ func (h CertificateHandler) ArchiveCertificate(w http.ResponseWriter, r *http.Re
}
if err := h.svc.ArchiveCertificate(r.Context(), id); err != nil {
if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Certificate not found", requestID)
return
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
slog.Error("ArchiveCertificate failed", "cert_id", id, "error", err)
msg = "internal error"
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to archive certificate", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -373,12 +376,13 @@ func (h CertificateHandler) GetCertificateVersions(w http.ResponseWriter, r *htt
versions, total, err := h.svc.GetCertificateVersions(r.Context(), certID, page, perPage)
if err != nil {
if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Certificate not found", requestID)
return
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
slog.Error("GetCertificateVersions failed", "cert_id", certID, "error", err)
msg = "internal error"
}
slog.Error("GetCertificateVersions failed", "cert_id", certID, "error", err.Error())
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to get certificate versions", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -414,20 +418,13 @@ func (h CertificateHandler) TriggerRenewal(w http.ResponseWriter, r *http.Reques
actor := resolveActor(r.Context())
if err := h.svc.TriggerRenewal(r.Context(), certID, actor); err != nil {
errMsg := err.Error()
if strings.Contains(errMsg, "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Certificate not found", requestID)
return
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
slog.Error("TriggerRenewal failed", "cert_id", certID, "error", err)
msg = "internal error"
}
if strings.Contains(errMsg, "cannot renew") {
ErrorWithRequestID(w, http.StatusBadRequest, errMsg, requestID)
return
}
if strings.Contains(errMsg, "already in progress") {
ErrorWithRequestID(w, http.StatusConflict, errMsg, requestID)
return
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to trigger renewal", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -516,19 +513,13 @@ func (h CertificateHandler) RevokeCertificate(w http.ResponseWriter, r *http.Req
actor := resolveActor(r.Context())
if err := h.svc.RevokeCertificate(r.Context(), certID, req.Reason, actor); err != nil {
// Distinguish between client errors and server errors
errMsg := err.Error()
if strings.Contains(errMsg, "already revoked") ||
strings.Contains(errMsg, "cannot revoke") ||
strings.Contains(errMsg, "invalid revocation reason") {
ErrorWithRequestID(w, http.StatusBadRequest, errMsg, requestID)
return
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
slog.Error("RevokeCertificate failed", "cert_id", certID, "error", err)
msg = "internal error"
}
if strings.Contains(errMsg, "not found") || strings.Contains(errMsg, "failed to fetch") || strings.Contains(errMsg, "failed to get") {
ErrorWithRequestID(w, http.StatusNotFound, "Certificate not found", requestID)
return
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to revoke certificate", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -557,16 +548,13 @@ func (h CertificateHandler) GetDERCRL(w http.ResponseWriter, r *http.Request) {
derBytes, err := h.svc.GenerateDERCRL(r.Context(), issuerID)
if err != nil {
errMsg := err.Error()
if strings.Contains(errMsg, "not found") {
ErrorWithRequestID(w, http.StatusNotFound, errMsg, requestID)
return
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
slog.Error("GenerateDERCRL failed", "issuer_id", issuerID, "error", err)
msg = "internal error"
}
if strings.Contains(errMsg, "do not support") || strings.Contains(errMsg, "does not support") {
ErrorWithRequestID(w, http.StatusNotImplemented, errMsg, requestID)
return
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to generate CRL", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -602,16 +590,13 @@ func (h CertificateHandler) HandleOCSP(w http.ResponseWriter, r *http.Request) {
derBytes, err := h.svc.GetOCSPResponse(r.Context(), issuerID, serialHex)
if err != nil {
errMsg := err.Error()
if strings.Contains(errMsg, "not found") {
ErrorWithRequestID(w, http.StatusNotFound, errMsg, requestID)
return
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
slog.Error("GetOCSPResponse failed", "issuer_id", issuerID, "serial", serialHex, "error", err)
msg = "internal error"
}
if strings.Contains(errMsg, "do not support") || strings.Contains(errMsg, "does not support") {
ErrorWithRequestID(w, http.StatusNotImplemented, errMsg, requestID)
return
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to generate OCSP response", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -642,12 +627,13 @@ func (h CertificateHandler) GetCertificateDeployments(w http.ResponseWriter, r *
deployments, err := h.svc.GetCertificateDeployments(r.Context(), certID)
if err != nil {
errMsg := err.Error()
if strings.Contains(errMsg, "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Certificate not found", requestID)
return
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
slog.Error("GetCertificateDeployments failed", "cert_id", certID, "error", err)
msg = "internal error"
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to get deployments", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
+5 -2
View File
@@ -3,6 +3,7 @@ package handler
import (
"context"
"encoding/json"
"log/slog"
"net/http"
)
@@ -39,7 +40,8 @@ func (h *DigestHandler) PreviewDigest(w http.ResponseWriter, r *http.Request) {
html, err := h.service.PreviewDigest(r.Context())
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
slog.Error("digest preview failed", "error", err.Error())
http.Error(w, "internal error", http.StatusInternalServerError)
return
}
@@ -64,9 +66,10 @@ func (h *DigestHandler) SendDigest(w http.ResponseWriter, r *http.Request) {
}
if err := h.service.SendDigest(r.Context()); err != nil {
slog.Error("digest send failed", "error", err.Error())
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusInternalServerError)
json.NewEncoder(w).Encode(map[string]string{"error": err.Error()})
json.NewEncoder(w).Encode(map[string]string{"error": "internal error"})
return
}
+117
View File
@@ -0,0 +1,117 @@
package handler
import (
"errors"
"net/http"
"github.com/lib/pq"
"github.com/shankar0123/certctl/internal/repository"
"github.com/shankar0123/certctl/internal/service"
)
// errToStatus is the single choke point that maps a service-layer or
// repository-layer error to its HTTP status code. Before M-1 (P2), 42 switch
// branches across 11 handler files classified errors via
// `strings.Contains(err.Error(), ...)` substring matching — a pattern that
// made every HTTP status mapping one sentinel-message reword away from silent
// regression (see M-003 self-approval privilege boundary: a reword of
// ErrSelfApproval.Error() would have demoted 403 Forbidden to 500 Internal
// Server Error with no compile-time error, no test failure, and no observable
// external signal).
//
// All handler branches now route through this function via errors.Is and
// errors.As, which walks the wrap chain built by fmt.Errorf("%w: ...", ...).
// The generic sentinels live in internal/service/errors.go; domain-specific
// sentinels (ErrSelfApproval, ErrAgentIsSentinel, ErrBlockedByDependencies,
// ErrForceReasonRequired, ErrAgentNotFound) wrap those generics via %w so both
// errors.Is(err, ErrSelfApproval) and errors.Is(err, ErrForbidden) succeed on
// the same wrapped error.
//
// # Dispatch order
//
// 1. ErrAgentRetired → 410 Gone. Tested FIRST. It is deliberately NOT wrapped
// under any generic sentinel — 410 Gone is semantically distinct from
// 403/404/409 (permanently-terminated resource identity that drives
// deterministic agent-binary shutdown at cmd/agent/main.go:1291). Must
// short-circuit before any generic check so wrapping can never demote it.
// 2. ErrNotFound → 404 Not Found. Both service.ErrNotFound and
// repository.ErrNotFound route here — repositories wrap sql.ErrNoRows with
// repository.ErrNotFound so a "row not found" escapes the repo layer as a
// typed sentinel rather than an untyped fmt.Errorf string. Tested BEFORE
// ErrForbidden so RFC 7235's preference for hiding resource existence from
// unauthorized callers is preserved (a caller who cannot see a resource
// should get 404, not 403).
// 3. ErrUnauthenticated → 401 Unauthorized. SCEP challenge-password mismatch
// and similar credential failures.
// 4. ErrForbidden → 403 Forbidden. M-003 gate. Tested BEFORE ErrValidation so
// double-wrapping (e.g., a future fmt.Errorf("%w: ctx", ErrSelfApproval)
// in a wrapping call site) cannot demote 403 to 400.
// 5. ErrConflict / repository.ErrRenewalPolicyDuplicateName /
// repository.ErrRenewalPolicyInUse → 409 Conflict. The repo-layer sentinels
// are routed here explicitly so handlers do not need their own dispatch
// tree for G-1's renewal-policy FK + unique-name violations.
// 6. ErrValidation → 400 Bad Request. Generic input validation / malformed
// request bodies / invalid state transitions that the caller could correct
// by changing their request.
// 7. ErrUnprocessable → 422 Unprocessable Entity. Distinct from
// ErrValidation: ErrValidation is "caller sent bad input" (400), while
// ErrUnprocessable is "caller's input was fine but our stored data can't
// satisfy the operation" — e.g., an X.509 PEM in the inventory that fails
// to decode. The pre-M-1 ExportPKCS12 handler pinned 422 on
// strings.Contains(err.Error(), "cannot be parsed"); the sentinel makes
// that dispatch survive message rewording.
// 8. ErrNotImplemented → 501 Not Implemented. Reserved for feature-flag-gated
// code paths.
// 9. *pq.Error fallback on SQLSTATE 23503 (FK violation) / 23505 (unique
// violation) → 409 Conflict. Final branch before the default 500. Anything
// that reaches here is technically a code smell (the repository layer
// should normally wrap driver errors into a typed sentinel) but the status
// mapping is still correct.
//
// # Why a function, not a middleware
//
// Handlers must continue to call [Error] / [ErrorWithRequestID] with a
// caller-chosen human-readable message (sometimes the wrapped err.Error(),
// sometimes a redacted "internal error" for 500s per F-002). This function
// gives handlers the status code; the handler keeps control of the body.
func errToStatus(err error) int {
if err == nil {
return http.StatusOK
}
switch {
case errors.Is(err, service.ErrAgentRetired):
return http.StatusGone // 410 — must short-circuit before generic dispatch
case errors.Is(err, service.ErrNotFound),
errors.Is(err, repository.ErrNotFound):
return http.StatusNotFound // 404 — before ErrForbidden (RFC 7235 existence hiding)
case errors.Is(err, service.ErrUnauthenticated):
return http.StatusUnauthorized // 401
case errors.Is(err, service.ErrForbidden):
return http.StatusForbidden // 403 — before ErrValidation (preserves M-003 gate under double-wrap)
case errors.Is(err, service.ErrConflict),
errors.Is(err, repository.ErrRenewalPolicyDuplicateName),
errors.Is(err, repository.ErrRenewalPolicyInUse):
return http.StatusConflict // 409
case errors.Is(err, service.ErrValidation):
return http.StatusBadRequest // 400
case errors.Is(err, service.ErrUnprocessable):
return http.StatusUnprocessableEntity // 422 — stored-data-unparseable, not caller-input-bad
case errors.Is(err, service.ErrNotImplemented):
return http.StatusNotImplemented // 501
}
// Driver-level fallback. Raw *pq.Error escaping the repository layer is a
// code smell but a real escape hatch today — we still want a correct 409
// instead of a generic 500 for FK/unique violations.
var pgErr *pq.Error
if errors.As(err, &pgErr) {
switch pgErr.Code {
case "23503", "23505":
return http.StatusConflict
}
}
return http.StatusInternalServerError
}
+120
View File
@@ -0,0 +1,120 @@
package handler
import (
"errors"
"fmt"
"net/http"
"testing"
"github.com/lib/pq"
"github.com/shankar0123/certctl/internal/repository"
"github.com/shankar0123/certctl/internal/service"
)
// TestErrToStatus_DispatchMatrix pins the handler's single error → HTTP
// status choke point. Each row covers one branch of the dispatch switch and
// the dispatch order invariants documented in errors.go:
//
// - ErrAgentRetired FIRST (410 short-circuits before generic checks)
// - ErrNotFound before ErrForbidden (RFC 7235 existence hiding)
// - ErrForbidden before ErrValidation (preserves M-003 gate under double-wrap)
// - Repo sentinels route to 409 alongside ErrConflict
// - *pq.Error on 23503 / 23505 routes to 409 as the driver-level fallback
// - Default path is 500
func TestErrToStatus_DispatchMatrix(t *testing.T) {
cases := []struct {
name string
err error
want int
}{
{"nil → 200", nil, http.StatusOK},
// Each generic sentinel resolves to its documented status code.
{"ErrNotFound → 404", service.ErrNotFound, http.StatusNotFound},
{"ErrValidation → 400", service.ErrValidation, http.StatusBadRequest},
{"ErrConflict → 409", service.ErrConflict, http.StatusConflict},
{"ErrForbidden → 403", service.ErrForbidden, http.StatusForbidden},
{"ErrUnauthenticated → 401", service.ErrUnauthenticated, http.StatusUnauthorized},
{"ErrNotImplemented → 501", service.ErrNotImplemented, http.StatusNotImplemented},
// Wrapped domain sentinels route through their generic wrap.
{"ErrSelfApproval → 403 (via ErrForbidden)", service.ErrSelfApproval, http.StatusForbidden},
{"ErrAgentIsSentinel → 403 (via ErrForbidden)", service.ErrAgentIsSentinel, http.StatusForbidden},
{"ErrBlockedByDependencies → 409 (via ErrConflict)", service.ErrBlockedByDependencies, http.StatusConflict},
{"ErrForceReasonRequired → 400 (via ErrValidation)", service.ErrForceReasonRequired, http.StatusBadRequest},
{"ErrAgentNotFound → 400 (via ErrValidation)", service.ErrAgentNotFound, http.StatusBadRequest},
// ErrAgentRetired is standalone — 410 Gone must fire before any
// generic dispatch. This locks in the semantic-distinct short-circuit.
{"ErrAgentRetired → 410", service.ErrAgentRetired, http.StatusGone},
// Repository-layer sentinels (G-1 + M-1).
{"repo.ErrNotFound → 404", repository.ErrNotFound, http.StatusNotFound},
{"wrapped repo.ErrNotFound → 404",
fmt.Errorf("%w: renewal policy rp-foo", repository.ErrNotFound),
http.StatusNotFound},
{"repo.ErrRenewalPolicyDuplicateName → 409", repository.ErrRenewalPolicyDuplicateName, http.StatusConflict},
{"repo.ErrRenewalPolicyInUse → 409", repository.ErrRenewalPolicyInUse, http.StatusConflict},
// Wrapped errors with additional context survive the dispatch.
{"wrapped ErrNotFound with context → 404",
fmt.Errorf("lookup failed: %w", service.ErrNotFound),
http.StatusNotFound},
{"wrapped ErrSelfApproval with context → 403",
fmt.Errorf("approval gate: %w", service.ErrSelfApproval),
http.StatusForbidden},
// Driver-level fallback: raw *pq.Error escaping repo layer.
{"*pq.Error 23503 → 409", &pq.Error{Code: "23503"}, http.StatusConflict},
{"*pq.Error 23505 → 409", &pq.Error{Code: "23505"}, http.StatusConflict},
{"*pq.Error 08006 → 500", &pq.Error{Code: "08006"}, http.StatusInternalServerError},
// Default path.
{"unknown error → 500", errors.New("something arbitrary"), http.StatusInternalServerError},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
got := errToStatus(c.err)
if got != c.want {
t.Errorf("errToStatus(%v) = %d, want %d", c.err, got, c.want)
}
})
}
}
// TestErrToStatus_AgentRetiredShortCircuit is a dedicated regression guard
// for the most fragile dispatch invariant: ErrAgentRetired's 410 Gone must
// fire FIRST. If a future commit wraps it under ErrForbidden (e.g., to
// include it in a generic "agent operations forbidden" bucket), this test
// goes red and the agent-binary shutdown at cmd/agent/main.go:1291 would
// silently stop triggering.
func TestErrToStatus_AgentRetiredShortCircuit(t *testing.T) {
if got := errToStatus(service.ErrAgentRetired); got != http.StatusGone {
t.Fatalf("ErrAgentRetired → %d, want 410 Gone (short-circuit must fire before any generic dispatch)", got)
}
}
// TestErrToStatus_NotFoundBeforeForbidden locks the RFC 7235 existence-
// hiding dispatch order. If someone were to reorder the switch arms to put
// ErrForbidden first, an authorization failure on a nonexistent resource
// would leak existence via a 403 instead of masking it with a 404.
func TestErrToStatus_NotFoundBeforeForbidden(t *testing.T) {
// A hypothetical wrapping where both would match — contrived but the
// ordering guarantee is what we're testing.
both := fmt.Errorf("%w: layered with %w", service.ErrNotFound, service.ErrForbidden)
if got := errToStatus(both); got != http.StatusNotFound {
t.Errorf("dual-wrapped err → %d, want 404 (ErrNotFound must dispatch before ErrForbidden)", got)
}
}
// TestErrToStatus_ForbiddenBeforeValidation guards the M-003 self-approval
// gate against a future call site that double-wraps ErrSelfApproval under
// ErrValidation (intentionally or accidentally). The dispatch must pick
// 403, not 400.
func TestErrToStatus_ForbiddenBeforeValidation(t *testing.T) {
doubled := fmt.Errorf("%w: %w", service.ErrSelfApproval, service.ErrValidation)
if got := errToStatus(doubled); got != http.StatusForbidden {
t.Errorf("double-wrapped err → %d, want 403 (ErrForbidden must dispatch before ErrValidation — M-003 gate)", got)
}
}
+43 -13
View File
@@ -46,12 +46,26 @@ func (h ExportHandler) ExportPEM(w http.ResponseWriter, r *http.Request) {
result, err := h.svc.ExportPEM(r.Context(), id)
if err != nil {
if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Certificate not found", requestID)
return
// M-1 (P2): dispatch routes through errToStatus. Pre-M-1 this branch
// classified 404 via strings.Contains(err.Error(), "not found"), which
// gave false positives on any error whose rendered text happened to
// contain "not found" — notably a transient DB failure when the service
// layer wrapped every certRepo.Get error with "certificate not found".
// Post-M-1: service/export.go now wraps with "failed to get certificate"
// and only the genuine sql.ErrNoRows path surfaces repository.ErrNotFound
// through the wrap chain, so errors.Is(err, repository.ErrNotFound) picks
// up the real 404s and everything else — including transient DB errors —
// correctly surfaces as 500 with server-side slog.Error capture (F-002
// redacted-500 pattern preserved).
status := errToStatus(err)
if status == http.StatusInternalServerError {
slog.Error("ExportPEM failed", "cert_id", id, "error", err.Error())
}
slog.Error("ExportPEM failed", "cert_id", id, "error", err.Error())
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to export certificate", requestID)
msg := "Failed to export certificate"
if status == http.StatusNotFound {
msg = "Certificate not found"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -94,16 +108,32 @@ func (h ExportHandler) ExportPKCS12(w http.ResponseWriter, r *http.Request) {
pfxData, err := h.svc.ExportPKCS12(r.Context(), id, req.Password)
if err != nil {
if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Certificate not found", requestID)
return
// M-1 (P2): dispatch routes through errToStatus. The pre-M-1 3-term
// substring net (`"not found"|"cannot be parsed"|"no certificates
// found"`) is replaced with sentinel dispatch:
// - repository.ErrNotFound (from certificate.go Get/GetLatestVersion
// sql.ErrNoRows wrap) → 404
// - service.ErrUnprocessable (from service/export.go ExportPKCS12's
// parsePEMCertificates-failure and empty-chain wraps) → 422 —
// semantically correct because the caller's request is fine; our
// stored PEM chain is what cannot be processed
// - everything else → 500 with slog.Error capture (F-002 redacted-500
// pattern preserved)
// A transient DB failure that pre-M-1 would have been swept into the
// 404 substring branch (because the service wrapped every certRepo.Get
// error with "certificate not found") now correctly surfaces as 500.
status := errToStatus(err)
if status == http.StatusInternalServerError {
slog.Error("ExportPKCS12 failed", "cert_id", id, "error", err.Error())
}
if strings.Contains(err.Error(), "cannot be parsed") || strings.Contains(err.Error(), "no certificates found") {
ErrorWithRequestID(w, http.StatusUnprocessableEntity, "Certificate data cannot be parsed as X.509", requestID)
return
msg := "Failed to export PKCS#12"
switch status {
case http.StatusNotFound:
msg = "Certificate not found"
case http.StatusUnprocessableEntity:
msg = "Certificate data cannot be parsed as X.509"
}
slog.Error("ExportPKCS12 failed", "cert_id", id, "error", err.Error())
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to export PKCS#12", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
+37 -2
View File
@@ -108,9 +108,17 @@ func TestExportPEM_Download(t *testing.T) {
}
func TestExportPEM_NotFound(t *testing.T) {
// M-1 (P2): wrap with service.ErrNotFound via %w so the handler's
// errToStatus choke point dispatches to 404 via errors.Is. Pre-M-1 this
// test used a raw `fmt.Errorf("certificate not found")` string and relied
// on the handler's strings.Contains(err.Error(), "not found") classifier
// — which was the same mechanism that silently misclassified transient DB
// failures whose text happened to include "not found" (see docblock on
// ExportPEM handler). Pinning the sentinel contract makes this test
// regression-proof against wrap-text changes.
mockSvc := &MockExportService{
ExportPEMFn: func(_ context.Context, _ string) (*service.ExportPEMResult, error) {
return nil, fmt.Errorf("certificate not found")
return nil, fmt.Errorf("%w: certificate", service.ErrNotFound)
},
}
h := NewExportHandler(mockSvc)
@@ -214,9 +222,11 @@ func TestExportPKCS12_EmptyPassword(t *testing.T) {
}
func TestExportPKCS12_NotFound(t *testing.T) {
// M-1 (P2): same sentinel migration as TestExportPEM_NotFound — see
// rationale there.
mockSvc := &MockExportService{
ExportPKCS12Fn: func(_ context.Context, _ string, _ string) ([]byte, error) {
return nil, fmt.Errorf("certificate not found")
return nil, fmt.Errorf("%w: certificate", service.ErrNotFound)
},
}
h := NewExportHandler(mockSvc)
@@ -231,6 +241,31 @@ func TestExportPKCS12_NotFound(t *testing.T) {
}
}
// TestExportPKCS12_Unprocessable pins the M-1 (P2) 422 contract: when the
// service layer wraps a parse failure with service.ErrUnprocessable, the
// handler's errToStatus choke point must dispatch to 422 Unprocessable
// Entity. Pre-M-1 this was classified via a 2-term substring net
// (`"cannot be parsed"|"no certificates found"`) at export.go:101, which
// would have been silently broken by a message reword in service/export.go.
// The new sentinel makes the dispatch survive message rewording.
func TestExportPKCS12_Unprocessable(t *testing.T) {
mockSvc := &MockExportService{
ExportPKCS12Fn: func(_ context.Context, _ string, _ string) ([]byte, error) {
return nil, fmt.Errorf("%w: certificate data cannot be parsed as X.509: asn1 decode error", service.ErrUnprocessable)
},
}
h := NewExportHandler(mockSvc)
req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/mc-test-1/export/pkcs12", nil)
w := httptest.NewRecorder()
h.ExportPKCS12(w, req)
if w.Code != http.StatusUnprocessableEntity {
t.Fatalf("expected 422, got %d", w.Code)
}
}
func TestExportPKCS12_ServiceError(t *testing.T) {
mockSvc := &MockExportService{
ExportPKCS12Fn: func(_ context.Context, _ string, _ string) ([]byte, error) {
+18 -24
View File
@@ -135,16 +135,13 @@ func (h IssuerHandler) CreateIssuer(w http.ResponseWriter, r *http.Request) {
created, err := h.svc.CreateIssuer(r.Context(), issuer)
if err != nil {
h.logger.Error("failed to create issuer", "error", err, "name", issuer.Name, "type", issuer.Type)
errMsg := err.Error()
switch {
case strings.Contains(errMsg, "unique") || strings.Contains(errMsg, "duplicate"):
ErrorWithRequestID(w, http.StatusConflict, "An issuer with this name already exists", requestID)
case strings.Contains(errMsg, "unsupported issuer type"):
ErrorWithRequestID(w, http.StatusBadRequest, errMsg, requestID)
default:
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to create issuer", requestID)
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
h.logger.Error("failed to create issuer", "error", err, "name", issuer.Name, "type", issuer.Type)
msg = "internal error"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -177,16 +174,13 @@ func (h IssuerHandler) UpdateIssuer(w http.ResponseWriter, r *http.Request) {
updated, err := h.svc.UpdateIssuer(r.Context(), id, issuer)
if err != nil {
h.logger.Error("failed to update issuer", "error", err, "id", id)
errMsg := err.Error()
switch {
case strings.Contains(errMsg, "unique") || strings.Contains(errMsg, "duplicate"):
ErrorWithRequestID(w, http.StatusConflict, "An issuer with this name already exists", requestID)
case strings.Contains(errMsg, "not found"):
ErrorWithRequestID(w, http.StatusNotFound, "Issuer not found", requestID)
default:
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to update issuer", requestID)
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
h.logger.Error("failed to update issuer", "error", err, "id", id)
msg = "internal error"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -210,13 +204,13 @@ func (h IssuerHandler) DeleteIssuer(w http.ResponseWriter, r *http.Request) {
}
if err := h.svc.DeleteIssuer(r.Context(), id); err != nil {
if strings.Contains(err.Error(), "violates foreign key") || strings.Contains(err.Error(), "RESTRICT") {
ErrorWithRequestID(w, http.StatusConflict, "Cannot delete issuer: certificates are still using this issuer", requestID)
} else if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Issuer not found", requestID)
} else {
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to delete issuer", requestID)
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
h.logger.Error("DeleteIssuer failed", "issuer_id", id, "error", err)
msg = "internal error"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
+13 -25
View File
@@ -3,15 +3,14 @@ package handler
import (
"context"
"encoding/json"
"errors"
"io"
"log/slog"
"net/http"
"strconv"
"strings"
"github.com/shankar0123/certctl/internal/api/middleware"
"github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/service"
)
// JobService defines the service interface for job operations.
@@ -160,22 +159,13 @@ func (h JobHandler) ApproveJob(w http.ResponseWriter, r *http.Request) {
actor := resolveActor(r.Context())
if err := h.svc.ApproveJob(r.Context(), jobID, actor); err != nil {
// M-003: self-approval by the certificate owner is forbidden.
if errors.Is(err, service.ErrSelfApproval) {
ErrorWithRequestID(w, http.StatusForbidden,
"Self-approval is forbidden: the certificate owner cannot approve their own renewal",
requestID)
return
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
slog.Error("ApproveJob failed", "job_id", jobID, "error", err)
msg = "internal error"
}
if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Job not found", requestID)
return
}
if strings.Contains(err.Error(), "cannot approve") {
ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID)
return
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to approve job", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -213,15 +203,13 @@ func (h JobHandler) RejectJob(w http.ResponseWriter, r *http.Request) {
actor := resolveActor(r.Context())
if err := h.svc.RejectJob(r.Context(), jobID, body.Reason, actor); err != nil {
if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Job not found", requestID)
return
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
slog.Error("RejectJob failed", "job_id", jobID, "error", err)
msg = "internal error"
}
if strings.Contains(err.Error(), "cannot reject") {
ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID)
return
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to reject job", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
+39 -5
View File
@@ -2,6 +2,7 @@ package handler
import (
"context"
"log/slog"
"net/http"
"strconv"
"strings"
@@ -107,7 +108,25 @@ func (h NotificationHandler) GetNotification(w http.ResponseWriter, r *http.Requ
notification, err := h.svc.GetNotification(r.Context(), id)
if err != nil {
ErrorWithRequestID(w, http.StatusNotFound, "Notification not found", requestID)
// M-1 (P2): dispatch routes through errToStatus. Pre-M-1 this branch was
// a blanket `any error → 404 Notification not found` shortcut — which
// silently demoted transient DB failures from the service's List() wrap
// (notification.go:386 pre-M-1) to 404 Not Found with no observable
// external signal. Post-M-1: service/notification.go GetNotification only
// wraps the genuine "not found" path with service.ErrNotFound via %w, and
// every other error (including the List() wrap) surfaces without that
// sentinel — so errors.Is(err, service.ErrNotFound) picks up the real
// 404s and everything else correctly surfaces as 500 with server-side
// slog.Error capture (F-002 redacted-500 pattern preserved).
status := errToStatus(err)
if status == http.StatusInternalServerError {
slog.Error("GetNotification failed", "notification_id", id, "error", err.Error())
}
msg := "Failed to get notification"
if status == http.StatusNotFound {
msg = "Notification not found"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -170,11 +189,26 @@ func (h NotificationHandler) RequeueNotification(w http.ResponseWriter, r *http.
notificationID := parts[0]
if err := h.svc.RequeueNotification(r.Context(), notificationID); err != nil {
if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Notification not found", requestID)
return
// M-1 (P2): dispatch routes through errToStatus. Pre-M-1 this branch
// classified 404 via strings.Contains(err.Error(), "not found"), which
// would have given false positives on any error whose rendered text
// happened to contain "not found" — notably a transient DB failure whose
// driver message mentioned a missing relation or column. Post-M-1: the
// repo-layer Requeue (postgres/notification.go) wraps zero-rows-affected
// with repository.ErrNotFound via %w, and the service layer forwards
// that error with %w — so errors.Is(err, repository.ErrNotFound) walks
// the full wrap chain and picks up the real 404s while everything else
// (including transient DB errors) correctly surfaces as 500 with
// server-side slog.Error capture (F-002 redacted-500 pattern preserved).
status := errToStatus(err)
if status == http.StatusInternalServerError {
slog.Error("RequeueNotification failed", "notification_id", notificationID, "error", err.Error())
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to requeue notification", requestID)
msg := "Failed to requeue notification"
if status == http.StatusNotFound {
msg = "Notification not found"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
+7 -6
View File
@@ -3,6 +3,7 @@ package handler
import (
"context"
"encoding/json"
"log/slog"
"net/http"
"strconv"
"strings"
@@ -184,13 +185,13 @@ func (h OwnerHandler) DeleteOwner(w http.ResponseWriter, r *http.Request) {
id = parts[0]
if err := h.svc.DeleteOwner(r.Context(), id); err != nil {
if strings.Contains(err.Error(), "violates foreign key") || strings.Contains(err.Error(), "RESTRICT") {
ErrorWithRequestID(w, http.StatusConflict, "Cannot delete owner: certificates are still assigned to this owner", requestID)
} else if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Owner not found", requestID)
} else {
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to delete owner", requestID)
status := errToStatus(err)
msg := err.Error()
if status == http.StatusInternalServerError {
slog.Error("DeleteOwner failed", "owner_id", id, "error", err)
msg = "internal error"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
+56 -15
View File
@@ -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"
)
// ProfileService defines the service interface for certificate profile operations.
@@ -21,6 +23,20 @@ type ProfileService interface {
}
// ProfileHandler handles HTTP requests for certificate profile operations.
//
// Error dispatch (post-M-1): every service error routes through the [errToStatus]
// choke point via `errors.Is` walking the wrap chain, with one explicit
// [service.ErrValidation] arm on the write paths (Create, Update) so the
// composed "validation: <field-specific reason>" message the service layer
// attaches via `fmt.Errorf("%w: ...", ErrValidation)` can be passed through to
// the 400 response body. Before M-1, the Create and Update handlers branched on
// `strings.Contains(err.Error(), "invalid"|"required"|"must be"|"cannot")` — a
// fragile pattern where a single reword in [service.validateProfile] would
// demote the 400 to 500 with no compile-time signal. The substring-based 404
// branches on Update and Delete likewise depended on the repository's
// human-readable "profile not found" message surviving forever; both now ride
// the same [repository.ErrNotFound] wrap that G-1's renewal-policy and M-1's
// other repositories use.
type ProfileHandler struct {
svc ProfileService
}
@@ -88,7 +104,18 @@ func (h ProfileHandler) GetProfile(w http.ResponseWriter, r *http.Request) {
profile, err := h.svc.GetProfile(r.Context(), id)
if err != nil {
ErrorWithRequestID(w, http.StatusNotFound, "Profile not found", requestID)
// M-1: route through errToStatus so a repo-level `sql.ErrNoRows`
// (wrapped as repository.ErrNotFound) becomes 404, but a transient DB
// failure no longer masquerades as 404 — it correctly surfaces 500. The
// pre-M-1 "any error → 404" shortcut was plausible when Get's only
// expected failure was "not found", but the choke point now gives us
// correct dispatch for free. Mirrors GetRenewalPolicy.
status := errToStatus(err)
msg := "Failed to get profile"
if status == http.StatusNotFound {
msg = "Profile not found"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -123,9 +150,15 @@ func (h ProfileHandler) CreateProfile(w http.ResponseWriter, r *http.Request) {
created, err := h.svc.CreateProfile(r.Context(), profile)
if err != nil {
// Check if it's a validation error from the service
if strings.Contains(err.Error(), "invalid") || strings.Contains(err.Error(), "required") ||
strings.Contains(err.Error(), "must be") || strings.Contains(err.Error(), "cannot") {
// M-1: replace the 4-term substring net
// (`"invalid"|"required"|"must be"|"cannot"`) with a single
// `errors.Is(err, service.ErrValidation)` arm. validateProfile wraps
// every field-specific failure via `fmt.Errorf("%w: <reason>",
// ErrValidation)`, so `err.Error()` still contains the human-readable
// reason (e.g., "RSA minimum key size must be at least 2048") and can be
// safely passed to the 400 body — but the status decision no longer
// depends on the exact wording. Other errors redact to a generic 500.
if errors.Is(err, service.ErrValidation) {
ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID)
return
}
@@ -162,16 +195,21 @@ func (h ProfileHandler) UpdateProfile(w http.ResponseWriter, r *http.Request) {
updated, err := h.svc.UpdateProfile(r.Context(), id, profile)
if err != nil {
if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Profile not found", requestID)
return
}
if strings.Contains(err.Error(), "invalid") || strings.Contains(err.Error(), "required") ||
strings.Contains(err.Error(), "must be") || strings.Contains(err.Error(), "cannot") {
// M-1: explicit ErrValidation arm preserves the user-facing reason in
// the 400 body (validateProfile wraps every failure via
// `fmt.Errorf("%w: ...", ErrValidation)`); every other error — including
// repo-layer ErrNotFound on a missing row — routes through errToStatus
// so the 404/500 decision no longer depends on substring matching.
if errors.Is(err, service.ErrValidation) {
ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID)
return
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to update profile", requestID)
status := errToStatus(err)
msg := "Failed to update profile"
if status == http.StatusNotFound {
msg = "Profile not found"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
@@ -195,11 +233,14 @@ func (h ProfileHandler) DeleteProfile(w http.ResponseWriter, r *http.Request) {
}
if err := h.svc.DeleteProfile(r.Context(), id); err != nil {
if strings.Contains(err.Error(), "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Profile not found", requestID)
return
// M-1: sentinel dispatch replaces the substring 404 check — see the
// parallel comment block in UpdateProfile for the rationale.
status := errToStatus(err)
msg := "Failed to delete profile"
if status == http.StatusNotFound {
msg = "Profile not found"
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to delete profile", requestID)
ErrorWithRequestID(w, status, msg, requestID)
return
}
+273
View File
@@ -0,0 +1,273 @@
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"
)
// RenewalPolicyService defines the service interface for renewal policy
// operations. G-1: all methods take ctx so the handler can propagate
// request-scoped cancellation/deadlines through the full stack.
type RenewalPolicyService interface {
ListRenewalPolicies(ctx context.Context, page, perPage int) ([]domain.RenewalPolicy, int64, error)
GetRenewalPolicy(ctx context.Context, id string) (*domain.RenewalPolicy, error)
CreateRenewalPolicy(ctx context.Context, rp domain.RenewalPolicy) (*domain.RenewalPolicy, error)
UpdateRenewalPolicy(ctx context.Context, id string, rp domain.RenewalPolicy) (*domain.RenewalPolicy, error)
DeleteRenewalPolicy(ctx context.Context, id string) error
}
// RenewalPolicyHandler serves /api/v1/renewal-policies CRUD endpoints.
//
// Error dispatch (post-M-1): every service error routes through the [errToStatus]
// choke point via `errors.Is` walking the wrap chain. Three sentinel identities
// cover the full dispatch surface:
//
// - [service.ErrRenewalPolicyDuplicateName] / [service.ErrRenewalPolicyInUse]
// are `var`-aliased to the repository-layer sentinels of the same name (G-1),
// so handler-side `errors.Is` succeeds against a sentinel raised three layers
// deep in [internal/repository/postgres.RenewalPolicyRepository] without the
// service layer having to translate. [errToStatus] routes both to 409.
// - [repository.ErrNotFound] is wrapped around `sql.ErrNoRows` inside the
// repo's Get/Update/Delete methods via `fmt.Errorf("%w: renewal policy %s",
// repository.ErrNotFound, id)` (M-1). [errToStatus] routes that to 404 in
// the same switch arm as [service.ErrNotFound], preserving the existing
// 404-on-missing behavior that the pre-M-1 substring check provided —
// without the rewording-regression risk that motivated the migration.
//
// The handler layer keeps two explicit `errors.Is` arms for the
// duplicate-name / in-use sentinels so each 409 response can carry a
// constraint-specific human-readable message ("with that name" vs. "still
// referenced by managed certificates"); every other error path — including
// not-found — delegates the status decision to [errToStatus] and provides a
// generic body via the F-002 redacted-500 pattern.
type RenewalPolicyHandler struct {
svc RenewalPolicyService
}
// NewRenewalPolicyHandler constructs the handler with its service dependency.
// Returned by value to match the house pattern (PolicyHandler, IssuerHandler
// etc.) — the registry stores handlers by value in router.HandlerRegistry.
func NewRenewalPolicyHandler(svc RenewalPolicyService) RenewalPolicyHandler {
return RenewalPolicyHandler{svc: svc}
}
// ListRenewalPolicies lists all renewal policies (paginated).
// GET /api/v1/renewal-policies?page=1&per_page=50
func (h RenewalPolicyHandler) ListRenewalPolicies(w http.ResponseWriter, r *http.Request) {
if r.Method != http.MethodGet {
Error(w, http.StatusMethodNotAllowed, "Method not allowed")
return
}
requestID := middleware.GetRequestID(r.Context())
page := 1
perPage := 50
query := r.URL.Query()
if p := query.Get("page"); p != "" {
if parsed, err := strconv.Atoi(p); err == nil && parsed > 0 {
page = parsed
}
}
if pp := query.Get("per_page"); pp != "" {
if parsed, err := strconv.Atoi(pp); err == nil && parsed > 0 && parsed <= 500 {
perPage = parsed
}
}
policies, total, err := h.svc.ListRenewalPolicies(r.Context(), page, perPage)
if err != nil {
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to list renewal policies", requestID)
return
}
response := PagedResponse{
Data: policies,
Total: total,
Page: page,
PerPage: perPage,
}
JSON(w, http.StatusOK, response)
}
// GetRenewalPolicy retrieves a single renewal policy by ID.
// GET /api/v1/renewal-policies/{id}
func (h RenewalPolicyHandler) GetRenewalPolicy(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 := strings.TrimPrefix(r.URL.Path, "/api/v1/renewal-policies/")
parts := strings.Split(id, "/")
if len(parts) == 0 || parts[0] == "" {
ErrorWithRequestID(w, http.StatusBadRequest, "Renewal policy ID is required", requestID)
return
}
id = parts[0]
policy, err := h.svc.GetRenewalPolicy(r.Context(), id)
if err != nil {
// M-1: route through errToStatus so a repo-level `sql.ErrNoRows`
// (wrapped as repository.ErrNotFound) becomes 404, but a transient DB
// failure no longer masquerades as 404 — it correctly surfaces 500.
// The pre-M-1 "any error → 404" shortcut was plausible when Get's only
// expected failure was "not found", but the choke point now gives us
// correct dispatch for free.
status := errToStatus(err)
msg := "Failed to get renewal policy"
if status == http.StatusNotFound {
msg = "Renewal policy not found"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
JSON(w, http.StatusOK, policy)
}
// CreateRenewalPolicy inserts a new renewal policy.
// POST /api/v1/renewal-policies
//
// Error mapping:
// - invalid JSON / missing name → 400
// - ErrRenewalPolicyDuplicateName (pg 23505 on name UNIQUE) → 409
// - anything else → 500
func (h RenewalPolicyHandler) CreateRenewalPolicy(w http.ResponseWriter, r *http.Request) {
if r.Method != http.MethodPost {
Error(w, http.StatusMethodNotAllowed, "Method not allowed")
return
}
requestID := middleware.GetRequestID(r.Context())
var rp domain.RenewalPolicy
if err := json.NewDecoder(r.Body).Decode(&rp); err != nil {
ErrorWithRequestID(w, http.StatusBadRequest, "Invalid request body", requestID)
return
}
if err := ValidateRequired("name", rp.Name); err != nil {
ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID)
return
}
created, err := h.svc.CreateRenewalPolicy(r.Context(), rp)
if err != nil {
if errors.Is(err, service.ErrRenewalPolicyDuplicateName) {
ErrorWithRequestID(w, http.StatusConflict, "A renewal policy with that name already exists", requestID)
return
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to create renewal policy", requestID)
return
}
JSON(w, http.StatusCreated, created)
}
// UpdateRenewalPolicy replaces the fields of an existing renewal policy.
// PUT /api/v1/renewal-policies/{id}
//
// Error mapping (post-M-1, sentinel-driven):
// - invalid JSON / empty ID → 400
// - ErrRenewalPolicyDuplicateName (pg 23505) → 409 (explicit arm, custom msg)
// - ErrNotFound (wrapping sql.ErrNoRows) → 404 (via errToStatus)
// - anything else → 500 (via errToStatus, body redacted)
func (h RenewalPolicyHandler) UpdateRenewalPolicy(w http.ResponseWriter, r *http.Request) {
if r.Method != http.MethodPut {
Error(w, http.StatusMethodNotAllowed, "Method not allowed")
return
}
requestID := middleware.GetRequestID(r.Context())
id := strings.TrimPrefix(r.URL.Path, "/api/v1/renewal-policies/")
parts := strings.Split(id, "/")
if len(parts) == 0 || parts[0] == "" {
ErrorWithRequestID(w, http.StatusBadRequest, "Renewal policy ID is required", requestID)
return
}
id = parts[0]
var rp domain.RenewalPolicy
if err := json.NewDecoder(r.Body).Decode(&rp); err != nil {
ErrorWithRequestID(w, http.StatusBadRequest, "Invalid request body", requestID)
return
}
updated, err := h.svc.UpdateRenewalPolicy(r.Context(), id, rp)
if err != nil {
if errors.Is(err, service.ErrRenewalPolicyDuplicateName) {
ErrorWithRequestID(w, http.StatusConflict, "A renewal policy with that name already exists", requestID)
return
}
// M-1: drop the `strings.Contains(err.Error(), "not found")` branch.
// [repository.ErrNotFound] now wraps sql.ErrNoRows at the three
// renewal-policy repo methods (Get/Update/Delete), so errToStatus
// routes a missing row to 404 via errors.Is without depending on the
// repo's fmt.Errorf format string surviving a future reword.
status := errToStatus(err)
msg := "Failed to update renewal policy"
if status == http.StatusNotFound {
msg = "Renewal policy not found"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
JSON(w, http.StatusOK, updated)
}
// DeleteRenewalPolicy removes a renewal policy.
// DELETE /api/v1/renewal-policies/{id}
//
// Error mapping (post-M-1, sentinel-driven):
// - empty ID (trailing slash) → 400
// - ErrRenewalPolicyInUse (pg 23503 FK-RESTRICT) → 409 (explicit arm, custom msg)
// - ErrNotFound (wrapping sql.ErrNoRows) → 404 (via errToStatus)
// - anything else → 500 (via errToStatus, body redacted)
func (h RenewalPolicyHandler) DeleteRenewalPolicy(w http.ResponseWriter, r *http.Request) {
if r.Method != http.MethodDelete {
Error(w, http.StatusMethodNotAllowed, "Method not allowed")
return
}
requestID := middleware.GetRequestID(r.Context())
id := strings.TrimPrefix(r.URL.Path, "/api/v1/renewal-policies/")
parts := strings.Split(id, "/")
if len(parts) == 0 || parts[0] == "" {
ErrorWithRequestID(w, http.StatusBadRequest, "Renewal policy ID is required", requestID)
return
}
id = parts[0]
if err := h.svc.DeleteRenewalPolicy(r.Context(), id); err != nil {
if errors.Is(err, service.ErrRenewalPolicyInUse) {
ErrorWithRequestID(w, http.StatusConflict, "Renewal policy is still referenced by managed certificates", requestID)
return
}
// M-1: sentinel dispatch replaces the substring check — see the
// parallel comment block in UpdateRenewalPolicy for the rationale.
status := errToStatus(err)
msg := "Failed to delete renewal policy"
if status == http.StatusNotFound {
msg = "Renewal policy not found"
}
ErrorWithRequestID(w, status, msg, requestID)
return
}
w.WriteHeader(http.StatusNoContent)
}
@@ -0,0 +1,434 @@
package handler
import (
"bytes"
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"testing"
"time"
"github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/service"
)
// G-1 red tests: lock in the HTTP surface of /api/v1/renewal-policies before
// the production code exists. Every subtest here references a symbol that
// Phase 2b must introduce:
//
// - NewRenewalPolicyHandler(svc) (constructor)
// - RenewalPolicyService (service-layer interface, in this package)
// - handler.ListRenewalPolicies / GetRenewalPolicy / CreateRenewalPolicy /
// UpdateRenewalPolicy / DeleteRenewalPolicy
// - service.ErrRenewalPolicyDuplicateName (pg 23505 → 409 mapping)
// - service.ErrRenewalPolicyInUse (pg 23503 → 409 mapping)
// MockRenewalPolicyService is a mock implementation of RenewalPolicyService.
type MockRenewalPolicyService struct {
ListRenewalPoliciesFn func(page, perPage int) ([]domain.RenewalPolicy, int64, error)
GetRenewalPolicyFn func(id string) (*domain.RenewalPolicy, error)
CreateRenewalPolicyFn func(rp domain.RenewalPolicy) (*domain.RenewalPolicy, error)
UpdateRenewalPolicyFn func(id string, rp domain.RenewalPolicy) (*domain.RenewalPolicy, error)
DeleteRenewalPolicyFn func(id string) error
}
func (m *MockRenewalPolicyService) ListRenewalPolicies(_ context.Context, page, perPage int) ([]domain.RenewalPolicy, int64, error) {
if m.ListRenewalPoliciesFn != nil {
return m.ListRenewalPoliciesFn(page, perPage)
}
return nil, 0, nil
}
func (m *MockRenewalPolicyService) GetRenewalPolicy(_ context.Context, id string) (*domain.RenewalPolicy, error) {
if m.GetRenewalPolicyFn != nil {
return m.GetRenewalPolicyFn(id)
}
return nil, nil
}
func (m *MockRenewalPolicyService) CreateRenewalPolicy(_ context.Context, rp domain.RenewalPolicy) (*domain.RenewalPolicy, error) {
if m.CreateRenewalPolicyFn != nil {
return m.CreateRenewalPolicyFn(rp)
}
return nil, nil
}
func (m *MockRenewalPolicyService) UpdateRenewalPolicy(_ context.Context, id string, rp domain.RenewalPolicy) (*domain.RenewalPolicy, error) {
if m.UpdateRenewalPolicyFn != nil {
return m.UpdateRenewalPolicyFn(id, rp)
}
return nil, nil
}
func (m *MockRenewalPolicyService) DeleteRenewalPolicy(_ context.Context, id string) error {
if m.DeleteRenewalPolicyFn != nil {
return m.DeleteRenewalPolicyFn(id)
}
return nil
}
// ----- List -----
func TestListRenewalPolicies_Success(t *testing.T) {
now := time.Now()
rp1 := domain.RenewalPolicy{
ID: "rp-default", Name: "Default", RenewalWindowDays: 30,
MaxRetries: 3, RetryInterval: 3600, AutoRenew: true,
CreatedAt: now, UpdatedAt: now,
}
rp2 := domain.RenewalPolicy{
ID: "rp-urgent", Name: "Urgent", RenewalWindowDays: 7,
MaxRetries: 5, RetryInterval: 600, AutoRenew: true,
CreatedAt: now, UpdatedAt: now,
}
mock := &MockRenewalPolicyService{
ListRenewalPoliciesFn: func(page, perPage int) ([]domain.RenewalPolicy, int64, error) {
return []domain.RenewalPolicy{rp1, rp2}, 2, nil
},
}
handler := NewRenewalPolicyHandler(mock)
req := httptest.NewRequest(http.MethodGet, "/api/v1/renewal-policies", nil)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.ListRenewalPolicies(w, req)
if w.Code != http.StatusOK {
t.Fatalf("expected status 200, got %d", w.Code)
}
var resp PagedResponse
if err := json.NewDecoder(w.Body).Decode(&resp); err != nil {
t.Fatalf("failed to decode response: %v", err)
}
if resp.Total != 2 {
t.Errorf("expected total 2, got %d", resp.Total)
}
}
func TestListRenewalPolicies_ServiceError(t *testing.T) {
mock := &MockRenewalPolicyService{
ListRenewalPoliciesFn: func(page, perPage int) ([]domain.RenewalPolicy, int64, error) {
return nil, 0, ErrMockServiceFailed
},
}
handler := NewRenewalPolicyHandler(mock)
req := httptest.NewRequest(http.MethodGet, "/api/v1/renewal-policies", nil)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.ListRenewalPolicies(w, req)
if w.Code != http.StatusInternalServerError {
t.Fatalf("expected status 500, got %d", w.Code)
}
}
func TestListRenewalPolicies_MethodNotAllowed(t *testing.T) {
handler := NewRenewalPolicyHandler(&MockRenewalPolicyService{})
req := httptest.NewRequest(http.MethodDelete, "/api/v1/renewal-policies", nil)
w := httptest.NewRecorder()
handler.ListRenewalPolicies(w, req)
if w.Code != http.StatusMethodNotAllowed {
t.Fatalf("expected status 405, got %d", w.Code)
}
}
// ----- Get -----
func TestGetRenewalPolicy_Success(t *testing.T) {
now := time.Now()
mock := &MockRenewalPolicyService{
GetRenewalPolicyFn: func(id string) (*domain.RenewalPolicy, error) {
return &domain.RenewalPolicy{
ID: id, Name: "Default", RenewalWindowDays: 30,
MaxRetries: 3, RetryInterval: 3600, AutoRenew: true,
CreatedAt: now, UpdatedAt: now,
}, nil
},
}
handler := NewRenewalPolicyHandler(mock)
req := httptest.NewRequest(http.MethodGet, "/api/v1/renewal-policies/rp-default", nil)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.GetRenewalPolicy(w, req)
if w.Code != http.StatusOK {
t.Fatalf("expected status 200, got %d", w.Code)
}
}
func TestGetRenewalPolicy_NotFound(t *testing.T) {
mock := &MockRenewalPolicyService{
GetRenewalPolicyFn: func(id string) (*domain.RenewalPolicy, error) {
return nil, ErrMockNotFound
},
}
handler := NewRenewalPolicyHandler(mock)
req := httptest.NewRequest(http.MethodGet, "/api/v1/renewal-policies/nonexistent", nil)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.GetRenewalPolicy(w, req)
if w.Code != http.StatusNotFound {
t.Fatalf("expected status 404, got %d", w.Code)
}
}
// ----- Create -----
func TestCreateRenewalPolicy_Success(t *testing.T) {
now := time.Now()
mock := &MockRenewalPolicyService{
CreateRenewalPolicyFn: func(rp domain.RenewalPolicy) (*domain.RenewalPolicy, error) {
rp.ID = "rp-new"
rp.CreatedAt = now
rp.UpdatedAt = now
return &rp, nil
},
}
body := map[string]interface{}{
"name": "New Policy",
"renewal_window_days": 30,
"max_retries": 3,
"retry_interval_seconds": 3600,
"auto_renew": true,
}
bodyBytes, _ := json.Marshal(body)
handler := NewRenewalPolicyHandler(mock)
req := httptest.NewRequest(http.MethodPost, "/api/v1/renewal-policies", bytes.NewReader(bodyBytes))
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.CreateRenewalPolicy(w, req)
if w.Code != http.StatusCreated {
t.Fatalf("expected status 201, got %d", w.Code)
}
}
func TestCreateRenewalPolicy_MissingName(t *testing.T) {
body := map[string]interface{}{
"renewal_window_days": 30,
"max_retries": 3,
"retry_interval_seconds": 3600,
}
bodyBytes, _ := json.Marshal(body)
handler := NewRenewalPolicyHandler(&MockRenewalPolicyService{})
req := httptest.NewRequest(http.MethodPost, "/api/v1/renewal-policies", bytes.NewReader(bodyBytes))
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.CreateRenewalPolicy(w, req)
if w.Code != http.StatusBadRequest {
t.Fatalf("expected status 400, got %d", w.Code)
}
}
func TestCreateRenewalPolicy_InvalidJSON(t *testing.T) {
handler := NewRenewalPolicyHandler(&MockRenewalPolicyService{})
req := httptest.NewRequest(http.MethodPost, "/api/v1/renewal-policies", bytes.NewReader([]byte("not json")))
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.CreateRenewalPolicy(w, req)
if w.Code != http.StatusBadRequest {
t.Fatalf("expected status 400, got %d", w.Code)
}
}
func TestCreateRenewalPolicy_DuplicateName(t *testing.T) {
// Service bubbles up ErrRenewalPolicyDuplicateName (pg 23505) → handler maps to 409.
mock := &MockRenewalPolicyService{
CreateRenewalPolicyFn: func(rp domain.RenewalPolicy) (*domain.RenewalPolicy, error) {
return nil, service.ErrRenewalPolicyDuplicateName
},
}
body := map[string]interface{}{
"name": "Duplicate",
"renewal_window_days": 30,
"max_retries": 3,
"retry_interval_seconds": 3600,
}
bodyBytes, _ := json.Marshal(body)
handler := NewRenewalPolicyHandler(mock)
req := httptest.NewRequest(http.MethodPost, "/api/v1/renewal-policies", bytes.NewReader(bodyBytes))
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.CreateRenewalPolicy(w, req)
if w.Code != http.StatusConflict {
t.Fatalf("expected status 409 on duplicate name, got %d", w.Code)
}
}
func TestCreateRenewalPolicy_MethodNotAllowed(t *testing.T) {
handler := NewRenewalPolicyHandler(&MockRenewalPolicyService{})
req := httptest.NewRequest(http.MethodGet, "/api/v1/renewal-policies", nil)
w := httptest.NewRecorder()
handler.CreateRenewalPolicy(w, req)
if w.Code != http.StatusMethodNotAllowed {
t.Fatalf("expected status 405, got %d", w.Code)
}
}
// ----- Update -----
func TestUpdateRenewalPolicy_Success(t *testing.T) {
now := time.Now()
mock := &MockRenewalPolicyService{
UpdateRenewalPolicyFn: func(id string, rp domain.RenewalPolicy) (*domain.RenewalPolicy, error) {
return &domain.RenewalPolicy{
ID: id, Name: rp.Name, RenewalWindowDays: rp.RenewalWindowDays,
MaxRetries: rp.MaxRetries, RetryInterval: rp.RetryInterval,
AutoRenew: rp.AutoRenew,
CreatedAt: now, UpdatedAt: now,
}, nil
},
}
body := map[string]interface{}{
"name": "Updated Policy",
"renewal_window_days": 45,
"max_retries": 5,
"retry_interval_seconds": 1800,
"auto_renew": true,
}
bodyBytes, _ := json.Marshal(body)
handler := NewRenewalPolicyHandler(mock)
req := httptest.NewRequest(http.MethodPut, "/api/v1/renewal-policies/rp-default", bytes.NewReader(bodyBytes))
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.UpdateRenewalPolicy(w, req)
if w.Code != http.StatusOK {
t.Fatalf("expected status 200, got %d", w.Code)
}
}
func TestUpdateRenewalPolicy_NotFound(t *testing.T) {
mock := &MockRenewalPolicyService{
UpdateRenewalPolicyFn: func(id string, rp domain.RenewalPolicy) (*domain.RenewalPolicy, error) {
return nil, ErrMockNotFound
},
}
body := map[string]interface{}{
"name": "Updated",
"renewal_window_days": 30,
"max_retries": 3,
"retry_interval_seconds": 3600,
}
bodyBytes, _ := json.Marshal(body)
handler := NewRenewalPolicyHandler(mock)
req := httptest.NewRequest(http.MethodPut, "/api/v1/renewal-policies/rp-missing", bytes.NewReader(bodyBytes))
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.UpdateRenewalPolicy(w, req)
if w.Code != http.StatusNotFound {
t.Fatalf("expected status 404, got %d", w.Code)
}
}
// ----- Delete -----
func TestDeleteRenewalPolicy_Success(t *testing.T) {
var deletedID string
mock := &MockRenewalPolicyService{
DeleteRenewalPolicyFn: func(id string) error {
deletedID = id
return nil
},
}
handler := NewRenewalPolicyHandler(mock)
req := httptest.NewRequest(http.MethodDelete, "/api/v1/renewal-policies/rp-default", nil)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.DeleteRenewalPolicy(w, req)
if w.Code != http.StatusNoContent {
t.Fatalf("expected status 204, got %d", w.Code)
}
if deletedID != "rp-default" {
t.Errorf("expected deleted ID 'rp-default', got '%s'", deletedID)
}
}
func TestDeleteRenewalPolicy_NotFound(t *testing.T) {
mock := &MockRenewalPolicyService{
DeleteRenewalPolicyFn: func(id string) error {
return ErrMockNotFound
},
}
handler := NewRenewalPolicyHandler(mock)
req := httptest.NewRequest(http.MethodDelete, "/api/v1/renewal-policies/rp-missing", nil)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.DeleteRenewalPolicy(w, req)
if w.Code != http.StatusNotFound {
t.Fatalf("expected status 404, got %d", w.Code)
}
}
func TestDeleteRenewalPolicy_InUseConflict(t *testing.T) {
// Service bubbles up ErrRenewalPolicyInUse (pg 23503 FK-RESTRICT) → handler maps to 409.
mock := &MockRenewalPolicyService{
DeleteRenewalPolicyFn: func(id string) error {
return service.ErrRenewalPolicyInUse
},
}
handler := NewRenewalPolicyHandler(mock)
req := httptest.NewRequest(http.MethodDelete, "/api/v1/renewal-policies/rp-active", nil)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.DeleteRenewalPolicy(w, req)
if w.Code != http.StatusConflict {
t.Fatalf("expected status 409 on in-use conflict, got %d", w.Code)
}
}
func TestDeleteRenewalPolicy_EmptyID(t *testing.T) {
handler := NewRenewalPolicyHandler(&MockRenewalPolicyService{})
req := httptest.NewRequest(http.MethodDelete, "/api/v1/renewal-policies/", nil)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.DeleteRenewalPolicy(w, req)
if w.Code != http.StatusBadRequest {
t.Fatalf("expected status 400, got %d", w.Code)
}
}
+19 -3
View File
@@ -6,6 +6,7 @@ import (
"encoding/asn1"
"encoding/base64"
"encoding/pem"
"errors"
"fmt"
"io"
"net/http"
@@ -14,6 +15,7 @@ import (
"github.com/shankar0123/certctl/internal/api/middleware"
"github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/pkcs7"
"github.com/shankar0123/certctl/internal/service"
)
// SCEPService defines the service interface for SCEP enrollment operations.
@@ -171,11 +173,25 @@ func (h SCEPHandler) pkiOperation(w http.ResponseWriter, r *http.Request) {
result, err := h.svc.PKCSReq(r.Context(), csrPEM, challengePassword, transactionID)
if err != nil {
if strings.Contains(err.Error(), "challenge password") {
ErrorWithRequestID(w, http.StatusForbidden, "Invalid challenge password", requestID)
// M-1 (P2): typed-sentinel dispatch replaces the pre-M-1 substring
// branch `strings.Contains(err.Error(), "challenge password")`. The
// service layer now wraps both challenge-password failure modes (server
// misconfigured / client credential wrong) via `fmt.Errorf("%w: ...",
// ErrUnauthenticated)`, so errors.Is walks the wrap chain without
// depending on the exact wording of the error string. This also
// corrects the HTTP status: pre-M-1 returned 403 Forbidden, but RFC
// 7235 classifies this as 401 Unauthorized (authentication failure, not
// authorization denial). The errToStatus doc block enumerates this as
// the canonical ErrUnauthenticated call site.
if errors.Is(err, service.ErrUnauthenticated) {
ErrorWithRequestID(w, http.StatusUnauthorized, "Invalid challenge password", requestID)
return
}
ErrorWithRequestID(w, http.StatusInternalServerError, fmt.Sprintf("Enrollment failed: %v", err), requestID)
// F-002 redacted-500: every other enrollment failure (CSR parse errors,
// issuer-layer failures, audit-layer failures) returns an opaque body
// so we don't leak internal state through the SCEP response. The
// logger already captured the real error at the service layer.
ErrorWithRequestID(w, http.StatusInternalServerError, "Enrollment failed", requestID)
return
}
+17 -3
View File
@@ -4,12 +4,14 @@ import (
"context"
"encoding/pem"
"errors"
"fmt"
"net/http"
"net/http/httptest"
"strings"
"testing"
"github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/service"
)
// mockSCEPService implements SCEPService for testing.
@@ -214,9 +216,21 @@ func TestSCEP_PKIOperation_Success_RawCSR(t *testing.T) {
}
}
// TestSCEP_PKIOperation_ChallengePasswordRejected pins the M-1 (P2) dispatch
// contract: when the service wraps the failure via `fmt.Errorf("%w: ...",
// service.ErrUnauthenticated)` the handler's errToStatus choke point must
// return 401 Unauthorized, NOT 403 Forbidden.
//
// This is a deliberate semantic correction. Pre-M-1 the handler inspected
// err.Error() for the "challenge password" substring and returned 403, which
// misclassified the RFC 7235 condition (the caller presented no valid
// application-layer credential — that is auth failure, not authorization
// denial). The errToStatus doc explicitly cites this code path as the
// canonical ErrUnauthenticated consumer; see handler/errors.go and the
// symmetric M-1 comment block at handler/scep.go in the pkiOperation arm.
func TestSCEP_PKIOperation_ChallengePasswordRejected(t *testing.T) {
svc := &mockSCEPService{
EnrollErr: errors.New("invalid challenge password"),
EnrollErr: fmt.Errorf("%w: invalid challenge password", service.ErrUnauthenticated),
}
h := NewSCEPHandler(svc)
@@ -230,8 +244,8 @@ func TestSCEP_PKIOperation_ChallengePasswordRejected(t *testing.T) {
w := httptest.NewRecorder()
h.HandleSCEP(w, req)
if w.Code != http.StatusForbidden {
t.Errorf("expected 403, got %d: %s", w.Code, w.Body.String())
if w.Code != http.StatusUnauthorized {
t.Errorf("expected 401 Unauthorized (M-1 dispatch of service.ErrUnauthenticated), got %d: %s", w.Code, w.Body.String())
}
}
+23 -7
View File
@@ -1,11 +1,27 @@
package handler
import "errors"
import (
"errors"
"fmt"
var (
// Mock errors for testing
ErrMockServiceFailed = errors.New("mock service error")
ErrMockNotFound = errors.New("mock not found error")
ErrMockUnauthorized = errors.New("mock unauthorized error")
ErrMockConflict = errors.New("mock conflict error")
"github.com/shankar0123/certctl/internal/service"
)
// Mock errors for testing.
//
// M-1: Since the handler layer now classifies errors via the typed-sentinel
// dispatch in [errToStatus] (errors.Is on service + repository sentinels rather
// than substring matching on err.Error()), handler mocks MUST wrap the
// appropriate generic sentinel so `errors.Is(err, service.ErrNotFound)` etc.
// succeed. Using raw errors.New() breaks the dispatch and degrades every
// mock-driven negative-path test to a 500 Internal Server Error — the same
// silent-regression trap the migration was designed to eliminate.
//
// ErrMockServiceFailed deliberately stays untyped so it continues to exercise
// the default 500 path.
var (
ErrMockServiceFailed = errors.New("mock service error")
ErrMockNotFound = fmt.Errorf("%w: mock not found", service.ErrNotFound)
ErrMockUnauthorized = fmt.Errorf("%w: mock unauthenticated", service.ErrUnauthenticated)
ErrMockConflict = fmt.Errorf("%w: mock conflict", service.ErrConflict)
)
+15 -2
View File
@@ -65,8 +65,9 @@ type HandlerRegistry struct {
Verification handler.VerificationHandler
Export handler.ExportHandler
Digest handler.DigestHandler
HealthChecks *handler.HealthCheckHandler
BulkRevocation handler.BulkRevocationHandler
HealthChecks *handler.HealthCheckHandler
BulkRevocation handler.BulkRevocationHandler
RenewalPolicies handler.RenewalPolicyHandler
}
// RegisterHandlers sets up all API routes with their handlers.
@@ -167,6 +168,18 @@ func (r *Router) RegisterHandlers(reg HandlerRegistry) {
r.Register("DELETE /api/v1/policies/{id}", http.HandlerFunc(reg.Policies.DeletePolicy))
r.Register("GET /api/v1/policies/{id}/violations", http.HandlerFunc(reg.Policies.ListViolations))
// Renewal Policies routes: /api/v1/renewal-policies
// G-1: fixes frontend FK drift — OnboardingWizard + CertificatesPage dropdowns
// were previously populating renewal_policy_id from /api/v1/policies (compliance
// rules, pol-* IDs), violating FK managed_certificates.renewal_policy_id →
// renewal_policies(id) ON DELETE RESTRICT. This block is the backend half; the
// frontend half swaps getPolicies → getRenewalPolicies at 3 call sites.
r.Register("GET /api/v1/renewal-policies", http.HandlerFunc(reg.RenewalPolicies.ListRenewalPolicies))
r.Register("POST /api/v1/renewal-policies", http.HandlerFunc(reg.RenewalPolicies.CreateRenewalPolicy))
r.Register("GET /api/v1/renewal-policies/{id}", http.HandlerFunc(reg.RenewalPolicies.GetRenewalPolicy))
r.Register("PUT /api/v1/renewal-policies/{id}", http.HandlerFunc(reg.RenewalPolicies.UpdateRenewalPolicy))
r.Register("DELETE /api/v1/renewal-policies/{id}", http.HandlerFunc(reg.RenewalPolicies.DeleteRenewalPolicy))
// Profiles routes: /api/v1/profiles
r.Register("GET /api/v1/profiles", http.HandlerFunc(reg.Profiles.ListProfiles))
r.Register("POST /api/v1/profiles", http.HandlerFunc(reg.Profiles.CreateProfile))
+1 -1
View File
@@ -612,7 +612,7 @@ func (c *Client) CancelJob(id string) error {
// GetStatus retrieves server health and summary stats.
func (c *Client) GetStatus() error {
resp, err := c.do("GET", "/api/v1/health", nil, nil)
resp, err := c.do("GET", "/health", nil, nil)
if err != nil {
return err
}
+1 -1
View File
@@ -293,7 +293,7 @@ func TestClient_GetStatus(t *testing.T) {
w.Header().Set("Content-Type", "application/json")
if r.URL.Path == "/api/v1/health" {
if r.URL.Path == "/health" {
json.NewEncoder(w).Encode(map[string]interface{}{
"status": "healthy",
"timestamp": time.Now().Format(time.RFC3339),
+13 -4
View File
@@ -664,12 +664,17 @@ func (c *Connector) solveAuthorizationsDNS01(ctx context.Context, authzURLs []st
return fmt.Errorf("failed to present DNS record for %s: %w", domain, err)
}
// Wait for DNS propagation
// Wait for DNS propagation (ctx-aware so graceful shutdown can interrupt — F-003)
propagationWait := time.Duration(c.config.DNSPropagationWait) * time.Second
c.logger.Info("waiting for DNS propagation",
"domain", domain,
"wait_seconds", c.config.DNSPropagationWait)
time.Sleep(propagationWait)
select {
case <-ctx.Done():
_ = c.dnsSolver.CleanUp(ctx, domain, dnsChallenge.Token, keyAuth)
return ctx.Err()
case <-time.After(propagationWait):
}
// Tell the CA we're ready
if _, err := c.client.Accept(ctx, dnsChallenge); err != nil {
@@ -773,12 +778,16 @@ func (c *Connector) solveAuthorizationsDNSPersist01(ctx context.Context, authzUR
return fmt.Errorf("failed to create persistent DNS record for %s: %w", domain, err)
}
// Wait for DNS propagation
// Wait for DNS propagation (ctx-aware so graceful shutdown can interrupt — F-003)
propagationWait := time.Duration(c.config.DNSPropagationWait) * time.Second
c.logger.Info("waiting for DNS propagation",
"domain", domain,
"wait_seconds", c.config.DNSPropagationWait)
time.Sleep(propagationWait)
select {
case <-ctx.Done():
return ctx.Err()
case <-time.After(propagationWait):
}
// Tell the CA we're ready
if _, err := c.client.Accept(ctx, persistChallenge); err != nil {
+35 -2
View File
@@ -1151,6 +1151,25 @@ func (m *mockRenewalPolicyRepository) List(ctx context.Context) ([]*domain.Renew
return policies, nil
}
// Create/Update/Delete satisfy the G-1 interface extension. The integration
// harness never drives the CRUD endpoints directly — these methods exist
// purely for interface compliance so the binary still builds.
func (m *mockRenewalPolicyRepository) Create(ctx context.Context, policy *domain.RenewalPolicy) error {
m.policies[policy.ID] = policy
return nil
}
func (m *mockRenewalPolicyRepository) Update(ctx context.Context, id string, policy *domain.RenewalPolicy) error {
policy.ID = id
m.policies[id] = policy
return nil
}
func (m *mockRenewalPolicyRepository) Delete(ctx context.Context, id string) error {
delete(m.policies, id)
return nil
}
type mockIssuerRepository struct {
issuers map[string]*domain.Issuer
}
@@ -1299,7 +1318,10 @@ func (m *mockProfileService) ListProfiles(_ context.Context, page, perPage int)
}
func (m *mockProfileService) GetProfile(_ context.Context, id string) (*domain.CertificateProfile, error) {
return nil, fmt.Errorf("profile not found")
// M-1: wrap service.ErrNotFound so the handler's errToStatus choke point
// routes this to 404 via errors.Is. The Error() message still contains
// "not found" so any pre-migration substring assertions continue to pass.
return nil, fmt.Errorf("%w: profile not found", service.ErrNotFound)
}
func (m *mockProfileService) CreateProfile(_ context.Context, profile domain.CertificateProfile) (*domain.CertificateProfile, error) {
@@ -1322,7 +1344,8 @@ func (m *mockAgentGroupService) ListAgentGroups(_ context.Context, page, perPage
}
func (m *mockAgentGroupService) GetAgentGroup(_ context.Context, id string) (*domain.AgentGroup, error) {
return nil, fmt.Errorf("agent group not found")
// M-1: wrap service.ErrNotFound — see GetProfile above for rationale.
return nil, fmt.Errorf("%w: agent group not found", service.ErrNotFound)
}
func (m *mockAgentGroupService) CreateAgentGroup(_ context.Context, group domain.AgentGroup) (*domain.AgentGroup, error) {
@@ -1371,6 +1394,16 @@ func (m *mockRevocationRepository) ListAll(ctx context.Context) ([]*domain.Certi
return m.revocations, nil
}
func (m *mockRevocationRepository) ListByIssuer(ctx context.Context, issuerID string) ([]*domain.CertificateRevocation, error) {
var result []*domain.CertificateRevocation
for _, r := range m.revocations {
if r.IssuerID == issuerID {
result = append(result, r)
}
}
return result, nil
}
func (m *mockRevocationRepository) ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error) {
var result []*domain.CertificateRevocation
for _, r := range m.revocations {
+59 -2
View File
@@ -2,11 +2,37 @@ package repository
import (
"context"
"errors"
"time"
"github.com/shankar0123/certctl/internal/domain"
)
// Repository-level sentinel errors. Repositories (primarily the postgres
// implementation) translate RDBMS-specific errors into these typed envelopes
// so the service/handler layers can branch with errors.Is without importing
// lib/pq or care about SQLSTATE codes.
//
// G-1: renewal-policy sentinels — DuplicateName → HTTP 409 (pg 23505 on
// renewal_policies.name UNIQUE), InUse → HTTP 409 (pg 23503 on the FK from
// managed_certificates.renewal_policy_id to renewal_policies.id with ON
// DELETE RESTRICT). Both map onto the same 409 status but with distinct
// messages so operators can tell them apart.
//
// M-1: ErrNotFound is the repo-layer "row not found" sentinel. Repositories
// that historically returned fmt.Errorf("... not found: %s", id) without
// wrapping sql.ErrNoRows now wrap ErrNotFound via fmt.Errorf("%w: ...", so
// the handler layer's single errToStatus choke point can route them to HTTP
// 404 via errors.Is without substring-matching the message text. Existing
// service-level service.ErrNotFound stays a distinct value — both map to 404
// through explicit branches in handler/errors.go (mirrors the G-1 treatment
// of the repo-level 409 sentinels).
var (
ErrNotFound = errors.New("repository: not found")
ErrRenewalPolicyDuplicateName = errors.New("renewal policy name already exists")
ErrRenewalPolicyInUse = errors.New("renewal policy is still referenced by managed certificates")
)
// CertificateRepository defines operations for managing certificates.
type CertificateRepository interface {
// List returns a paginated list of certificates matching the filter criteria.
@@ -47,8 +73,15 @@ type RevocationRepository interface {
// protocol endpoints carry it in the request path; RFC 5280 §5.2.3 guarantees
// uniqueness only within a single issuer.
GetByIssuerAndSerial(ctx context.Context, issuerID, serial string) (*domain.CertificateRevocation, error)
// ListAll returns all revocations, ordered by revocation time (for CRL generation).
// ListAll returns all revocations, ordered by revocation time (for global
// revocation admin views). CRL generation should prefer ListByIssuer to
// avoid loading and discarding rows that belong to other issuers.
ListAll(ctx context.Context) ([]*domain.CertificateRevocation, error)
// ListByIssuer returns revocations for a single issuer, ordered by revocation
// time (for CRL generation). Pushing the issuer filter into the query lets
// the migration 000012 composite index (issuer_id, serial_number) drive a
// prefix scan instead of a full table read + in-memory filter.
ListByIssuer(ctx context.Context, issuerID string) ([]*domain.CertificateRevocation, error)
// ListByCertificate returns all revocations for a certificate.
ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error)
// MarkIssuerNotified updates the issuer_notified flag for a revocation.
@@ -251,11 +284,35 @@ type JobRepository interface {
}
// RenewalPolicyRepository defines operations for managing renewal policies.
//
// G-1: extended with Create/Update/Delete so the new /api/v1/renewal-policies
// CRUD surface has a repo contract to lean on. Delete must map the PostgreSQL
// 23503 (foreign_key_violation on managed_certificates.renewal_policy_id
// REFERENCES renewal_policies(id) ON DELETE RESTRICT) onto the typed
// ErrRenewalPolicyInUse sentinel so the handler can emit a 409 Conflict
// instead of an opaque 500. Create/Update map PostgreSQL 23505
// (unique_violation on renewal_policies.name) onto ErrRenewalPolicyDuplicateName
// for the same 409 Conflict reason.
//
// List stays single-shot (no pagination params) because the production row
// count is in the single digits — the service layer paginates/sorts in Go.
// Changing the signature would churn every mock without functional benefit.
type RenewalPolicyRepository interface {
// Get retrieves a renewal policy by ID.
Get(ctx context.Context, id string) (*domain.RenewalPolicy, error)
// List returns all renewal policies.
// List returns all renewal policies, ordered by name.
List(ctx context.Context) ([]*domain.RenewalPolicy, error)
// Create inserts a new renewal policy. The caller is responsible for
// populating Name; Create auto-generates ID (as rp-<slug(name)>) if empty.
// Returns ErrRenewalPolicyDuplicateName on pg 23505.
Create(ctx context.Context, policy *domain.RenewalPolicy) error
// Update modifies an existing renewal policy in-place. Returns
// sql.ErrNoRows-wrapped error when id is unknown, or
// ErrRenewalPolicyDuplicateName on pg 23505 (name collision with another row).
Update(ctx context.Context, id string, policy *domain.RenewalPolicy) error
// Delete removes a renewal policy. Returns ErrRenewalPolicyInUse when the
// policy is still referenced by rows in managed_certificates (pg 23503).
Delete(ctx context.Context, id string) error
}
// PolicyRepository defines operations for managing compliance policies and violations.
+30 -7
View File
@@ -3,11 +3,13 @@ package postgres
import (
"context"
"database/sql"
"errors"
"fmt"
"time"
"github.com/google/uuid"
"github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/repository"
)
// AgentRepository implements repository.AgentRepository
@@ -72,8 +74,13 @@ func (r *AgentRepository) Get(ctx context.Context, id string) (*domain.Agent, er
agent, err := scanAgent(row)
if err != nil {
if err == sql.ErrNoRows {
return nil, fmt.Errorf("agent not found")
// M-1 (P2): wrap sql.ErrNoRows with repository.ErrNotFound via %w so
// the handler's errToStatus choke point dispatches to 404 via
// errors.Is instead of the pre-M-1 strings.Contains(err.Error(),
// "not found") branch at handler/agents.go. Mirrors agent_group and
// profile repositories.
if errors.Is(err, sql.ErrNoRows) {
return nil, fmt.Errorf("%w: agent %s", repository.ErrNotFound, id)
}
return nil, fmt.Errorf("failed to query agent: %w", err)
}
@@ -169,8 +176,12 @@ func (r *AgentRepository) Update(ctx context.Context, agent *domain.Agent) error
return fmt.Errorf("failed to get rows affected: %w", err)
}
// M-1 (P2): wrap the zero-rows-affected condition with
// repository.ErrNotFound so the handler's errToStatus dispatches to 404
// via errors.Is without substring matching. Mirrors agent_group and
// profile repositories.
if rows == 0 {
return fmt.Errorf("agent not found")
return fmt.Errorf("%w: agent %s", repository.ErrNotFound, agent.ID)
}
return nil
@@ -189,8 +200,10 @@ func (r *AgentRepository) Delete(ctx context.Context, id string) error {
return fmt.Errorf("failed to get rows affected: %w", err)
}
// M-1 (P2): zero-rows-affected → repository.ErrNotFound wrap. Mirrors
// agent_group and profile repositories.
if rows == 0 {
return fmt.Errorf("agent not found")
return fmt.Errorf("%w: agent %s", repository.ErrNotFound, id)
}
return nil
@@ -236,8 +249,13 @@ func (r *AgentRepository) UpdateHeartbeat(ctx context.Context, id string, metada
return fmt.Errorf("failed to get rows affected: %w", err)
}
// M-1 (P2): zero-rows-affected → repository.ErrNotFound wrap. Note the
// UPDATE filters on `retired_at IS NULL`, so a retired agent row also
// returns zero-rows-affected here. The service layer short-circuits with
// ErrAgentRetired (410) BEFORE reaching this path via Heartbeat's
// explicit Get check, so the 404 vs 410 distinction is drawn upstream.
if rows == 0 {
return fmt.Errorf("agent not found")
return fmt.Errorf("%w: agent %s", repository.ErrNotFound, id)
}
return nil
@@ -258,8 +276,13 @@ func (r *AgentRepository) GetByAPIKey(ctx context.Context, keyHash string) (*dom
agent, err := scanAgent(row)
if err != nil {
if err == sql.ErrNoRows {
return nil, fmt.Errorf("agent not found")
// M-1 (P2): wrap sql.ErrNoRows with repository.ErrNotFound via %w.
// The auth middleware calls this on every request; a missing row
// must surface as 404 (well, 401 upstream — the middleware treats
// "no agent matched" as auth failure) via the errToStatus choke
// point, not via substring matching.
if errors.Is(err, sql.ErrNoRows) {
return nil, fmt.Errorf("%w: agent with api key not found", repository.ErrNotFound)
}
return nil, fmt.Errorf("failed to query agent: %w", err)
}
+16 -4
View File
@@ -3,10 +3,12 @@ package postgres
import (
"context"
"database/sql"
"errors"
"fmt"
"time"
"github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/repository"
)
// AgentGroupRepository implements agent group CRUD with PostgreSQL.
@@ -49,8 +51,12 @@ func (r *AgentGroupRepository) Get(ctx context.Context, id string) (*domain.Agen
g := &domain.AgentGroup{}
err := row.Scan(&g.ID, &g.Name, &g.Description, &g.MatchOS, &g.MatchArchitecture,
&g.MatchIPCIDR, &g.MatchVersion, &g.Enabled, &g.CreatedAt, &g.UpdatedAt)
if err == sql.ErrNoRows {
return nil, fmt.Errorf("agent group not found: %s", id)
// M-1 (P2): wrap sql.ErrNoRows with repository.ErrNotFound via %w so the
// handler's errToStatus choke point dispatches to 404 via errors.Is
// instead of the pre-M-1 strings.Contains(err.Error(), "not found")
// branch at handler/agent_groups.go. Mirrors profile repository.
if errors.Is(err, sql.ErrNoRows) {
return nil, fmt.Errorf("%w: agent group %s", repository.ErrNotFound, id)
}
if err != nil {
return nil, fmt.Errorf("failed to get agent group: %w", err)
@@ -83,8 +89,11 @@ func (r *AgentGroupRepository) Update(ctx context.Context, group *domain.AgentGr
return fmt.Errorf("failed to update agent group: %w", err)
}
rows, _ := result.RowsAffected()
// M-1 (P2): wrap the zero-rows-affected condition with
// repository.ErrNotFound so the handler's errToStatus dispatches to 404
// via errors.Is without substring matching. Mirrors profile repository.
if rows == 0 {
return fmt.Errorf("agent group not found: %s", group.ID)
return fmt.Errorf("%w: agent group %s", repository.ErrNotFound, group.ID)
}
return nil
}
@@ -96,8 +105,11 @@ func (r *AgentGroupRepository) Delete(ctx context.Context, id string) error {
return fmt.Errorf("failed to delete agent group: %w", err)
}
rows, _ := result.RowsAffected()
// M-1 (P2): wrap zero-rows-affected with repository.ErrNotFound so the
// handler's errToStatus dispatches to 404 via errors.Is. Mirrors profile
// repository.
if rows == 0 {
return fmt.Errorf("agent group not found: %s", id)
return fmt.Errorf("%w: agent group %s", repository.ErrNotFound, id)
}
return nil
}
+25 -4
View File
@@ -264,8 +264,14 @@ func (r *CertificateRepository) Get(ctx context.Context, id string) (*domain.Man
cert, err := r.scanCertificate(ctx, row)
if err != nil {
if err == sql.ErrNoRows {
return nil, fmt.Errorf("certificate not found")
// M-1 (P2): wrap sql.ErrNoRows with repository.ErrNotFound via %w so
// the handler's errToStatus choke point dispatches to 404 via
// errors.Is instead of the pre-M-1 strings.Contains(err.Error(),
// "not found") branch at handler/export.go (ExportPEM + ExportPKCS12).
// scanCertificate already wraps sql.ErrNoRows via %w, so errors.Is
// walks through. Mirrors profile and agent_group repositories.
if errors.Is(err, sql.ErrNoRows) {
return nil, fmt.Errorf("%w: certificate %s", repository.ErrNotFound, id)
}
return nil, fmt.Errorf("failed to query certificate: %w", err)
}
@@ -396,8 +402,11 @@ func (r *CertificateRepository) Update(ctx context.Context, cert *domain.Managed
return fmt.Errorf("failed to get rows affected: %w", err)
}
// M-1 (P2): zero-rows-affected → repository.ErrNotFound wrap so the
// handler's errToStatus dispatches to 404 via errors.Is. Mirrors profile
// and agent_group repositories.
if rows == 0 {
return fmt.Errorf("certificate not found")
return fmt.Errorf("%w: certificate %s", repository.ErrNotFound, cert.ID)
}
return nil
@@ -418,8 +427,10 @@ func (r *CertificateRepository) Archive(ctx context.Context, id string) error {
return fmt.Errorf("failed to get rows affected: %w", err)
}
// M-1 (P2): zero-rows-affected → repository.ErrNotFound wrap. Mirrors
// profile and agent_group repositories.
if rows == 0 {
return fmt.Errorf("certificate not found")
return fmt.Errorf("%w: certificate %s", repository.ErrNotFound, id)
}
return nil
@@ -583,6 +594,16 @@ func (r *CertificateRepository) GetLatestVersion(ctx context.Context, certID str
v.KeySize = int(keySize.Int64)
if err != nil {
// M-1 (P2): wrap sql.ErrNoRows with repository.ErrNotFound via %w so
// the handler's errToStatus choke point dispatches to 404 via
// errors.Is. The export service surfaces "no certificate version
// found" on this path — pre-M-1 the handler branched on
// strings.Contains(err.Error(), "not found"), now it walks the wrap
// chain. Non-ErrNoRows errors preserve their "failed to get latest
// certificate version" framing.
if errors.Is(err, sql.ErrNoRows) {
return nil, fmt.Errorf("%w: certificate version for %s", repository.ErrNotFound, certID)
}
return nil, fmt.Errorf("failed to get latest certificate version: %w", err)
}
@@ -0,0 +1,453 @@
package postgres_test
// Integration tests for HealthCheckRepository (M48). Closes the L-1
// coverage gap flagged in coverage-gap-audit.md: the 453-line repository
// shipped in M48 had zero live-DB tests, leaving 11 methods — including
// the time-sensitive ListDueForCheck, PurgeHistory, and GetSummary —
// without migration-pinned regression protection. These tests exercise
// every method against a real Postgres 16 container through the same
// schema-per-test harness used by repo_test.go.
import (
"context"
"testing"
"time"
"github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/repository"
"github.com/shankar0123/certctl/internal/repository/postgres"
)
// newHealthCheck builds a minimal EndpointHealthCheck the repository will
// accept. All time-pointer fields are left nil so callers can override
// exactly the bits each subtest cares about — Create stores nil pointers
// as NULL, which is what ListDueForCheck's `last_checked_at IS NULL`
// branch relies on.
func newHealthCheck(id, endpoint string, status domain.HealthStatus, enabled bool) *domain.EndpointHealthCheck {
now := time.Now().UTC().Truncate(time.Microsecond)
return &domain.EndpointHealthCheck{
ID: id,
Endpoint: endpoint,
Status: status,
DegradedThreshold: 2,
DownThreshold: 5,
CheckIntervalSecs: 300,
Enabled: enabled,
CreatedAt: now,
UpdatedAt: now,
}
}
// TestHealthCheckRepository_CRUD covers Create → Get → Update → Delete on
// the nominal path. Also verifies the sql.NullTime round-trip: a check
// created without timestamps comes back with nil pointers (not
// zero-valued time.Time) so downstream Go code can distinguish "never
// probed" from "probed at epoch".
func TestHealthCheckRepository_CRUD(t *testing.T) {
tdb := getTestDB(t)
db := tdb.freshSchema(t)
repo := postgres.NewHealthCheckRepository(db)
ctx := context.Background()
check := newHealthCheck("hc-crud", "example.com:443", domain.HealthStatusHealthy, true)
check.ExpectedFingerprint = "sha256:expected"
check.ResponseTimeMs = 42
if err := repo.Create(ctx, check); err != nil {
t.Fatalf("Create failed: %v", err)
}
got, err := repo.Get(ctx, "hc-crud")
if err != nil {
t.Fatalf("Get failed: %v", err)
}
if got.Endpoint != "example.com:443" {
t.Errorf("Endpoint = %q, want example.com:443", got.Endpoint)
}
if got.Status != domain.HealthStatusHealthy {
t.Errorf("Status = %q, want %q", got.Status, domain.HealthStatusHealthy)
}
if got.ExpectedFingerprint != "sha256:expected" {
t.Errorf("ExpectedFingerprint = %q, want sha256:expected", got.ExpectedFingerprint)
}
if got.CheckIntervalSecs != 300 {
t.Errorf("CheckIntervalSecs = %d, want 300", got.CheckIntervalSecs)
}
if got.LastCheckedAt != nil {
t.Errorf("LastCheckedAt = %v, want nil (never probed)", got.LastCheckedAt)
}
// Update: status transition + observed fingerprint assignment.
// Update() rewrites UpdatedAt to time.Now() regardless of what we
// send, so record the pre-call timestamp to assert monotonic advance.
preUpdate := got.UpdatedAt
time.Sleep(2 * time.Millisecond) // ensure a measurable delta
got.Status = domain.HealthStatusDegraded
got.ObservedFingerprint = "sha256:observed"
got.ConsecutiveFailures = 2
if err := repo.Update(ctx, got); err != nil {
t.Fatalf("Update failed: %v", err)
}
got2, err := repo.Get(ctx, "hc-crud")
if err != nil {
t.Fatalf("Get after Update failed: %v", err)
}
if got2.Status != domain.HealthStatusDegraded {
t.Errorf("Status after Update = %q, want %q", got2.Status, domain.HealthStatusDegraded)
}
if got2.ObservedFingerprint != "sha256:observed" {
t.Errorf("ObservedFingerprint after Update = %q, want sha256:observed", got2.ObservedFingerprint)
}
if got2.ConsecutiveFailures != 2 {
t.Errorf("ConsecutiveFailures after Update = %d, want 2", got2.ConsecutiveFailures)
}
if !got2.UpdatedAt.After(preUpdate) {
t.Errorf("UpdatedAt did not advance: pre=%v post=%v", preUpdate, got2.UpdatedAt)
}
if err := repo.Delete(ctx, "hc-crud"); err != nil {
t.Fatalf("Delete failed: %v", err)
}
if _, err := repo.Get(ctx, "hc-crud"); err == nil {
t.Errorf("Get after Delete returned nil error, want not-found")
}
}
// TestHealthCheckRepository_GetByEndpoint verifies the secondary lookup
// path used by AutoCreateFromDeployment to decide whether to INSERT or
// UPDATE. Missing endpoints return an error (not a nil cert) so the
// service layer can branch safely.
func TestHealthCheckRepository_GetByEndpoint(t *testing.T) {
tdb := getTestDB(t)
db := tdb.freshSchema(t)
repo := postgres.NewHealthCheckRepository(db)
ctx := context.Background()
check := newHealthCheck("hc-byep", "svc.internal:443", domain.HealthStatusHealthy, true)
if err := repo.Create(ctx, check); err != nil {
t.Fatalf("Create failed: %v", err)
}
got, err := repo.GetByEndpoint(ctx, "svc.internal:443")
if err != nil {
t.Fatalf("GetByEndpoint failed: %v", err)
}
if got.ID != "hc-byep" {
t.Errorf("ID = %q, want hc-byep", got.ID)
}
if _, err := repo.GetByEndpoint(ctx, "never-seen.example.com:443"); err == nil {
t.Errorf("GetByEndpoint on unknown endpoint returned nil error")
}
}
// TestHealthCheckRepository_List_Filters seeds rows across the filter
// axes (status, certificate_id, enabled) and asserts each branch of the
// WHERE builder, plus the Page/PerPage pagination shim.
func TestHealthCheckRepository_List_Filters(t *testing.T) {
tdb := getTestDB(t)
db := tdb.freshSchema(t)
repo := postgres.NewHealthCheckRepository(db)
ctx := context.Background()
// Create prereq managed certificate so certificate_id FK can be
// populated on one row — proves the filter path joins on a real ID.
ownerID, teamID, issuerID, policyID := insertCertPrereqsRaw(t, db, ctx, "hclist")
certID := "mc-hc-list"
now := time.Now().UTC().Truncate(time.Microsecond)
if _, err := db.ExecContext(ctx, `
INSERT INTO managed_certificates (id, name, common_name, sans, environment, owner_id, team_id, issuer_id, renewal_policy_id, status, expires_at, tags, created_at, updated_at)
VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14)`,
certID, "hc-list-cert", "hc.example.com", "{}", "production",
ownerID, teamID, issuerID, policyID,
string(domain.CertificateStatusActive), now.Add(90*24*time.Hour), "{}",
now, now); err != nil {
t.Fatalf("seed managed_certificate: %v", err)
}
// 4 rows: healthy+enabled+cert, degraded+enabled, down+disabled, unknown+enabled.
rows := []*domain.EndpointHealthCheck{
newHealthCheck("hc-list-1", "a.example.com:443", domain.HealthStatusHealthy, true),
newHealthCheck("hc-list-2", "b.example.com:443", domain.HealthStatusDegraded, true),
newHealthCheck("hc-list-3", "c.example.com:443", domain.HealthStatusDown, false),
newHealthCheck("hc-list-4", "d.example.com:443", domain.HealthStatusUnknown, true),
}
rows[0].CertificateID = &certID
for _, r := range rows {
if err := repo.Create(ctx, r); err != nil {
t.Fatalf("Create %s: %v", r.ID, err)
}
}
// Filter: status=healthy → 1 result.
got, total, err := repo.List(ctx, &repository.HealthCheckFilter{Status: string(domain.HealthStatusHealthy)})
if err != nil {
t.Fatalf("List status=healthy: %v", err)
}
if total != 1 || len(got) != 1 || got[0].ID != "hc-list-1" {
t.Errorf("status=healthy: total=%d rows=%d want 1/1 with hc-list-1", total, len(got))
}
// Filter: certificate_id → 1 result.
got, total, err = repo.List(ctx, &repository.HealthCheckFilter{CertificateID: certID})
if err != nil {
t.Fatalf("List certificate_id: %v", err)
}
if total != 1 || len(got) != 1 || got[0].ID != "hc-list-1" {
t.Errorf("certificate_id filter: total=%d rows=%d want 1/1", total, len(got))
}
// Filter: enabled=false → 1 result.
disabled := false
got, total, err = repo.List(ctx, &repository.HealthCheckFilter{Enabled: &disabled})
if err != nil {
t.Fatalf("List enabled=false: %v", err)
}
if total != 1 || len(got) != 1 || got[0].ID != "hc-list-3" {
t.Errorf("enabled=false: total=%d rows=%d want 1/1 with hc-list-3", total, len(got))
}
// Pagination: per_page=2 → first page has 2, total reflects all 4.
got, total, err = repo.List(ctx, &repository.HealthCheckFilter{Page: 1, PerPage: 2})
if err != nil {
t.Fatalf("List paginated: %v", err)
}
if total != 4 {
t.Errorf("paginated total = %d, want 4", total)
}
if len(got) != 2 {
t.Errorf("paginated rows = %d, want 2", len(got))
}
}
// TestHealthCheckRepository_ListDueForCheck seeds all four branches of
// the WHERE clause — (a) enabled+null → due, (b) enabled+past-due → due,
// (c) enabled+recent → not due, (d) disabled+null → excluded — and
// asserts the ORDER BY last_checked_at NULLS FIRST, ASC ordering.
//
// This is the hot path the scheduler's 8th loop hits every 60 seconds;
// a correctness regression here silently fails every probe.
func TestHealthCheckRepository_ListDueForCheck(t *testing.T) {
tdb := getTestDB(t)
db := tdb.freshSchema(t)
repo := postgres.NewHealthCheckRepository(db)
ctx := context.Background()
now := time.Now().UTC().Truncate(time.Microsecond)
pastDue := now.Add(-10 * time.Minute) // > 300s ago, enabled → due
recent := now.Add(-30 * time.Second) // < 300s ago, enabled → not due
// (a) enabled + null last_checked_at — NULLS FIRST puts this first
a := newHealthCheck("hc-due-a", "a.example.com:443", domain.HealthStatusUnknown, true)
// (b) enabled + past-due last_checked_at
b := newHealthCheck("hc-due-b", "b.example.com:443", domain.HealthStatusHealthy, true)
b.LastCheckedAt = &pastDue
// (c) enabled + recent last_checked_at — must NOT appear
c := newHealthCheck("hc-due-c", "c.example.com:443", domain.HealthStatusHealthy, true)
c.LastCheckedAt = &recent
// (d) disabled + null last_checked_at — must NOT appear
d := newHealthCheck("hc-due-d", "d.example.com:443", domain.HealthStatusUnknown, false)
for _, r := range []*domain.EndpointHealthCheck{a, b, c, d} {
if err := repo.Create(ctx, r); err != nil {
t.Fatalf("Create %s: %v", r.ID, err)
}
}
due, err := repo.ListDueForCheck(ctx)
if err != nil {
t.Fatalf("ListDueForCheck: %v", err)
}
if len(due) != 2 {
ids := make([]string, 0, len(due))
for _, r := range due {
ids = append(ids, r.ID)
}
t.Fatalf("due rows = %d (%v), want exactly 2 (hc-due-a, hc-due-b)", len(due), ids)
}
// NULLS FIRST: variant (a) should precede variant (b).
if due[0].ID != "hc-due-a" {
t.Errorf("due[0].ID = %q, want hc-due-a (NULLS FIRST)", due[0].ID)
}
if due[1].ID != "hc-due-b" {
t.Errorf("due[1].ID = %q, want hc-due-b", due[1].ID)
}
// Sanity: neither excluded row leaked through.
for _, r := range due {
if r.ID == "hc-due-c" {
t.Errorf("recent-probed row hc-due-c leaked into due set")
}
if r.ID == "hc-due-d" {
t.Errorf("disabled row hc-due-d leaked into due set")
}
}
}
// TestHealthCheckRepository_RecordHistory_GetHistory asserts FIFO
// insertion with DESC retrieval (most-recent-first) and the explicit
// limit clamp.
func TestHealthCheckRepository_RecordHistory_GetHistory(t *testing.T) {
tdb := getTestDB(t)
db := tdb.freshSchema(t)
repo := postgres.NewHealthCheckRepository(db)
ctx := context.Background()
parent := newHealthCheck("hc-hist", "hist.example.com:443", domain.HealthStatusHealthy, true)
if err := repo.Create(ctx, parent); err != nil {
t.Fatalf("Create parent: %v", err)
}
base := time.Now().UTC().Truncate(time.Microsecond)
entries := []*domain.HealthHistoryEntry{
{ID: "hh-1", HealthCheckID: "hc-hist", Status: string(domain.HealthStatusHealthy), ResponseTimeMs: 10, CheckedAt: base.Add(-3 * time.Minute)},
{ID: "hh-2", HealthCheckID: "hc-hist", Status: string(domain.HealthStatusDegraded), ResponseTimeMs: 20, CheckedAt: base.Add(-2 * time.Minute)},
{ID: "hh-3", HealthCheckID: "hc-hist", Status: string(domain.HealthStatusHealthy), ResponseTimeMs: 30, CheckedAt: base.Add(-1 * time.Minute)},
}
for _, e := range entries {
if err := repo.RecordHistory(ctx, e); err != nil {
t.Fatalf("RecordHistory %s: %v", e.ID, err)
}
}
// limit=2 → newest 2 in DESC order: hh-3, hh-2.
got, err := repo.GetHistory(ctx, "hc-hist", 2)
if err != nil {
t.Fatalf("GetHistory: %v", err)
}
if len(got) != 2 {
t.Fatalf("rows = %d, want 2", len(got))
}
if got[0].ID != "hh-3" || got[1].ID != "hh-2" {
t.Errorf("order = [%s, %s], want [hh-3, hh-2]", got[0].ID, got[1].ID)
}
// limit=0 → default 100 → returns all 3.
got, err = repo.GetHistory(ctx, "hc-hist", 0)
if err != nil {
t.Fatalf("GetHistory limit=0: %v", err)
}
if len(got) != 3 {
t.Errorf("limit=0 rows = %d, want 3", len(got))
}
}
// TestHealthCheckRepository_PurgeHistory exercises the retention-sweep
// hot path (scheduler calls this once/day). 5 past + 5 future straddling
// the cutoff exposes both sides of the < comparator — an off-by-one
// regression here would either nuke live data or skip rows the retention
// policy was meant to remove.
func TestHealthCheckRepository_PurgeHistory(t *testing.T) {
tdb := getTestDB(t)
db := tdb.freshSchema(t)
repo := postgres.NewHealthCheckRepository(db)
ctx := context.Background()
parent := newHealthCheck("hc-purge", "purge.example.com:443", domain.HealthStatusHealthy, true)
if err := repo.Create(ctx, parent); err != nil {
t.Fatalf("Create parent: %v", err)
}
cutoff := time.Now().UTC().Truncate(time.Microsecond)
// 5 rows BEFORE cutoff (should be purged).
for i := 0; i < 5; i++ {
e := &domain.HealthHistoryEntry{
ID: "hh-past-" + string(rune('0'+i)),
HealthCheckID: "hc-purge",
Status: string(domain.HealthStatusHealthy),
CheckedAt: cutoff.Add(time.Duration(-10-i) * time.Minute),
}
if err := repo.RecordHistory(ctx, e); err != nil {
t.Fatalf("RecordHistory past %d: %v", i, err)
}
}
// 5 rows AFTER cutoff (should remain).
for i := 0; i < 5; i++ {
e := &domain.HealthHistoryEntry{
ID: "hh-future-" + string(rune('0'+i)),
HealthCheckID: "hc-purge",
Status: string(domain.HealthStatusHealthy),
CheckedAt: cutoff.Add(time.Duration(1+i) * time.Minute),
}
if err := repo.RecordHistory(ctx, e); err != nil {
t.Fatalf("RecordHistory future %d: %v", i, err)
}
}
deleted, err := repo.PurgeHistory(ctx, cutoff)
if err != nil {
t.Fatalf("PurgeHistory: %v", err)
}
if deleted != 5 {
t.Errorf("deleted = %d, want 5", deleted)
}
remaining, err := repo.GetHistory(ctx, "hc-purge", 100)
if err != nil {
t.Fatalf("GetHistory after purge: %v", err)
}
if len(remaining) != 5 {
t.Errorf("remaining = %d, want 5", len(remaining))
}
}
// TestHealthCheckRepository_GetSummary seeds all 5 HealthStatus values
// in a non-uniform distribution so the GROUP BY status branch-table gets
// exercised on each arm. The Total field is computed inside the
// aggregator — its drift would not surface unless we assert it too.
func TestHealthCheckRepository_GetSummary(t *testing.T) {
tdb := getTestDB(t)
db := tdb.freshSchema(t)
repo := postgres.NewHealthCheckRepository(db)
ctx := context.Background()
seed := map[domain.HealthStatus]int{
domain.HealthStatusHealthy: 3,
domain.HealthStatusDegraded: 2,
domain.HealthStatusDown: 2,
domain.HealthStatusCertMismatch: 1,
domain.HealthStatusUnknown: 1,
}
idx := 0
for status, count := range seed {
for i := 0; i < count; i++ {
check := newHealthCheck(
"hc-sum-"+string(rune('a'+idx)),
"sum.example.com:443",
status,
true,
)
// Endpoint uniqueness isn't enforced by the schema but making
// it unique documents intent and rules out false-positives.
check.Endpoint = check.ID + "-" + check.Endpoint
if err := repo.Create(ctx, check); err != nil {
t.Fatalf("Create %s: %v", check.ID, err)
}
idx++
}
}
summary, err := repo.GetSummary(ctx)
if err != nil {
t.Fatalf("GetSummary: %v", err)
}
if summary.Healthy != 3 {
t.Errorf("Healthy = %d, want 3", summary.Healthy)
}
if summary.Degraded != 2 {
t.Errorf("Degraded = %d, want 2", summary.Degraded)
}
if summary.Down != 2 {
t.Errorf("Down = %d, want 2", summary.Down)
}
if summary.CertMismatch != 1 {
t.Errorf("CertMismatch = %d, want 1", summary.CertMismatch)
}
if summary.Unknown != 1 {
t.Errorf("Unknown = %d, want 1", summary.Unknown)
}
if summary.Total != 9 {
t.Errorf("Total = %d, want 9 (3+2+2+1+1)", summary.Total)
}
}
+20 -8
View File
@@ -138,7 +138,15 @@ func (r *NotificationRepository) List(ctx context.Context, filter *repository.No
return notifs, nil
}
// UpdateStatus updates a notification's delivery status
// UpdateStatus updates a notification's delivery status.
//
// M-1 (P2): zero-rows-affected now wraps repository.ErrNotFound via %w so the
// handler's errToStatus choke point dispatches to 404 via errors.Is. Pre-M-1
// the return was a raw fmt.Errorf("notification not found") string that the
// service layer re-wrapped and the handler classified via strings.Contains —
// one sentinel-message reword away from silently demoting 404 to 500. The
// behavior (error on concurrent-delete / bad-id) is unchanged; only the error
// identity is now type-safe.
func (r *NotificationRepository) UpdateStatus(ctx context.Context, id string, status string, sentAt time.Time) error {
result, err := r.db.ExecContext(ctx, `
UPDATE notification_events SET status = $1, sent_at = $2 WHERE id = $3
@@ -154,7 +162,7 @@ func (r *NotificationRepository) UpdateStatus(ctx context.Context, id string, st
}
if rows == 0 {
return fmt.Errorf("notification not found")
return fmt.Errorf("%w: notification %s", repository.ErrNotFound, id)
}
return nil
@@ -307,10 +315,12 @@ func (r *NotificationRepository) RecordFailedAttempt(ctx context.Context, id str
return fmt.Errorf("failed to get rows affected: %w", err)
}
if rows == 0 {
// Same "not found" error shape as UpdateStatus above. The scheduler
// logs-and-continues on this so a concurrently-deleted row doesn't
// break the sweep.
return fmt.Errorf("notification not found")
// M-1 (P2): wrap repository.ErrNotFound (was raw
// fmt.Errorf("notification not found")). Same "not found" shape as
// UpdateStatus — the scheduler logs-and-continues on a concurrently
// deleted row, but callers that surface the error (the handler) now
// discriminate via errors.Is instead of substring matching.
return fmt.Errorf("%w: notification %s", repository.ErrNotFound, id)
}
return nil
}
@@ -342,7 +352,8 @@ func (r *NotificationRepository) MarkAsDead(ctx context.Context, id string, last
return fmt.Errorf("failed to get rows affected: %w", err)
}
if rows == 0 {
return fmt.Errorf("notification not found")
// M-1 (P2): wrap repository.ErrNotFound. See UpdateStatus rationale.
return fmt.Errorf("%w: notification %s", repository.ErrNotFound, id)
}
return nil
}
@@ -379,7 +390,8 @@ func (r *NotificationRepository) Requeue(ctx context.Context, id string) error {
return fmt.Errorf("failed to get rows affected: %w", err)
}
if rows == 0 {
return fmt.Errorf("notification not found")
// M-1 (P2): wrap repository.ErrNotFound. See UpdateStatus rationale.
return fmt.Errorf("%w: notification %s", repository.ErrNotFound, id)
}
return nil
}
+11 -4
View File
@@ -4,11 +4,13 @@ import (
"context"
"database/sql"
"encoding/json"
"errors"
"fmt"
"time"
"github.com/google/uuid"
"github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/repository"
)
// ProfileRepository implements repository.CertificateProfileRepository
@@ -63,8 +65,11 @@ func (r *ProfileRepository) Get(ctx context.Context, id string) (*domain.Certifi
p, err := scanProfile(row)
if err != nil {
if err == sql.ErrNoRows {
return nil, fmt.Errorf("profile not found")
if errors.Is(err, sql.ErrNoRows) {
// M-1: wrap repository.ErrNotFound so the handler's errToStatus
// choke point can route this to HTTP 404 via errors.Is without
// substring-matching the "not found" message text.
return nil, fmt.Errorf("%w: profile %s", repository.ErrNotFound, id)
}
return nil, fmt.Errorf("failed to query profile: %w", err)
}
@@ -159,7 +164,8 @@ func (r *ProfileRepository) Update(ctx context.Context, profile *domain.Certific
}
if rows == 0 {
return fmt.Errorf("profile not found")
// M-1: wrap repository.ErrNotFound — see Get for rationale.
return fmt.Errorf("%w: profile %s", repository.ErrNotFound, profile.ID)
}
return nil
@@ -178,7 +184,8 @@ func (r *ProfileRepository) Delete(ctx context.Context, id string) error {
}
if rows == 0 {
return fmt.Errorf("profile not found")
// M-1: wrap repository.ErrNotFound — see Get for rationale.
return fmt.Errorf("%w: profile %s", repository.ErrNotFound, id)
}
return nil
+242 -40
View File
@@ -4,46 +4,61 @@ import (
"context"
"database/sql"
"encoding/json"
"errors"
"fmt"
"regexp"
"strings"
"github.com/lib/pq"
"github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/repository"
)
// RenewalPolicyRepository implements repository.RenewalPolicyRepository
// RenewalPolicyRepository implements repository.RenewalPolicyRepository.
type RenewalPolicyRepository struct {
db *sql.DB
}
// NewRenewalPolicyRepository creates a new RenewalPolicyRepository
// NewRenewalPolicyRepository creates a new RenewalPolicyRepository.
func NewRenewalPolicyRepository(db *sql.DB) *RenewalPolicyRepository {
return &RenewalPolicyRepository{db: db}
}
// Get retrieves a renewal policy by ID
func (r *RenewalPolicyRepository) Get(ctx context.Context, id string) (*domain.RenewalPolicy, error) {
// SELECT column order is the shared contract between scanRenewalPolicy and
// every SELECT/RETURNING in this file. Keep them in lockstep; if you add a
// new column, add it to all SELECTs, all scan calls, and scanRenewalPolicy.
//
// Note: certificate_profile_id and agent_group_id live on renewal_policies
// (migrations 000003 and 000004) but are deliberately NOT read here — that
// pre-existing drift is out of G-1's minimum-viable-delta and is tracked in
// the design doc §8. Introducing them would change struct shapes / JSON tags
// and require domain-layer churn we're not taking on in this change.
const renewalPolicyColumns = `
id, name, renewal_window_days, auto_renew, max_retries,
retry_interval_minutes, alert_thresholds_days, created_at, updated_at
`
// scanRenewalPolicy decodes one renewal_policies row from a Row or Rows
// scanner, unmarshaling alert_thresholds_days JSONB into the domain slice.
// Malformed JSONB silently falls back to DefaultAlertThresholds() — same
// behavior as the pre-G-1 code so we don't change observable semantics.
func scanRenewalPolicy(scanner interface {
Scan(dest ...any) error
}) (*domain.RenewalPolicy, error) {
var policy domain.RenewalPolicy
var thresholdsJSON []byte
err := r.db.QueryRowContext(ctx, `
SELECT id, name, renewal_window_days, auto_renew, max_retries,
retry_interval_minutes, alert_thresholds_days, created_at, updated_at
FROM renewal_policies
WHERE id = $1
`, id).Scan(&policy.ID, &policy.Name, &policy.RenewalWindowDays, &policy.AutoRenew,
if err := scanner.Scan(
&policy.ID, &policy.Name, &policy.RenewalWindowDays, &policy.AutoRenew,
&policy.MaxRetries, &policy.RetryInterval, &thresholdsJSON,
&policy.CreatedAt, &policy.UpdatedAt)
if err != nil {
if err == sql.ErrNoRows {
return nil, fmt.Errorf("renewal policy not found: %s", id)
}
return nil, fmt.Errorf("failed to query renewal policy: %w", err)
&policy.CreatedAt, &policy.UpdatedAt,
); err != nil {
return nil, err
}
// Parse alert thresholds from JSONB
if len(thresholdsJSON) > 0 {
if err := json.Unmarshal(thresholdsJSON, &policy.AlertThresholdsDays); err != nil {
// Fall back to defaults if JSON is malformed
policy.AlertThresholdsDays = domain.DefaultAlertThresholds()
}
}
@@ -51,14 +66,26 @@ func (r *RenewalPolicyRepository) Get(ctx context.Context, id string) (*domain.R
return &policy, nil
}
// List returns all renewal policies
// Get retrieves a renewal policy by ID.
func (r *RenewalPolicyRepository) Get(ctx context.Context, id string) (*domain.RenewalPolicy, error) {
row := r.db.QueryRowContext(ctx, `SELECT `+renewalPolicyColumns+` FROM renewal_policies WHERE id = $1`, id)
policy, err := scanRenewalPolicy(row)
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
// M-1: wrap repository.ErrNotFound so the handler's errToStatus
// choke point can route this to HTTP 404 via errors.Is without
// substring-matching the "not found" message text.
return nil, fmt.Errorf("%w: renewal policy %s", repository.ErrNotFound, id)
}
return nil, fmt.Errorf("failed to query renewal policy: %w", err)
}
return policy, nil
}
// List returns all renewal policies, ordered by name (matches the index on
// renewal_policies.name from migration 000001 so ORDER BY is index-served).
func (r *RenewalPolicyRepository) List(ctx context.Context) ([]*domain.RenewalPolicy, error) {
rows, err := r.db.QueryContext(ctx, `
SELECT id, name, renewal_window_days, auto_renew, max_retries,
retry_interval_minutes, alert_thresholds_days, created_at, updated_at
FROM renewal_policies
ORDER BY name
`)
rows, err := r.db.QueryContext(ctx, `SELECT `+renewalPolicyColumns+` FROM renewal_policies ORDER BY name`)
if err != nil {
return nil, fmt.Errorf("failed to query renewal policies: %w", err)
}
@@ -66,22 +93,11 @@ func (r *RenewalPolicyRepository) List(ctx context.Context) ([]*domain.RenewalPo
var policies []*domain.RenewalPolicy
for rows.Next() {
var policy domain.RenewalPolicy
var thresholdsJSON []byte
if err := rows.Scan(&policy.ID, &policy.Name, &policy.RenewalWindowDays, &policy.AutoRenew,
&policy.MaxRetries, &policy.RetryInterval, &thresholdsJSON,
&policy.CreatedAt, &policy.UpdatedAt); err != nil {
policy, err := scanRenewalPolicy(rows)
if err != nil {
return nil, fmt.Errorf("failed to scan renewal policy: %w", err)
}
if len(thresholdsJSON) > 0 {
if err := json.Unmarshal(thresholdsJSON, &policy.AlertThresholdsDays); err != nil {
policy.AlertThresholdsDays = domain.DefaultAlertThresholds()
}
}
policies = append(policies, &policy)
policies = append(policies, policy)
}
if err := rows.Err(); err != nil {
@@ -90,3 +106,189 @@ func (r *RenewalPolicyRepository) List(ctx context.Context) ([]*domain.RenewalPo
return policies, nil
}
// slugRegex matches non-alphanumeric characters that slugifyPolicyName strips.
var slugRegex = regexp.MustCompile(`[^a-z0-9-]+`)
// slugifyPolicyName produces `rp-<slug>` for an auto-generated policy ID.
// Slug: lowercase, spaces→hyphens, non-alphanumeric stripped, trimmed to 64
// chars. Matches the existing seed convention (rp-default, rp-standard,
// rp-urgent). Collision resolution is handled by Create's retry loop.
func slugifyPolicyName(name string) string {
slug := strings.ToLower(strings.TrimSpace(name))
slug = strings.ReplaceAll(slug, " ", "-")
slug = slugRegex.ReplaceAllString(slug, "")
slug = strings.Trim(slug, "-")
if slug == "" {
slug = "policy"
}
if len(slug) > 64 {
slug = slug[:64]
}
return "rp-" + slug
}
// isUniqueViolation reports whether err is a PostgreSQL 23505 unique_violation.
// Used by Create/Update to translate name-collision errors onto the typed
// ErrRenewalPolicyDuplicateName sentinel.
func isUniqueViolation(err error) bool {
var pqErr *pq.Error
return errors.As(err, &pqErr) && pqErr.Code == "23505"
}
// isForeignKeyViolation reports whether err is a PostgreSQL 23503
// foreign_key_violation. Used by Delete to translate ON DELETE RESTRICT
// failures onto the typed ErrRenewalPolicyInUse sentinel.
func isForeignKeyViolation(err error) bool {
var pqErr *pq.Error
return errors.As(err, &pqErr) && pqErr.Code == "23503"
}
// Create inserts a new renewal policy. If policy.ID is empty, auto-generates
// `rp-<slug(name)>` with -2/-3/... suffixes on collision (up to 10 attempts).
// Returns ErrRenewalPolicyDuplicateName on pg 23505 (name collision).
//
// alert_thresholds_days is marshaled to JSONB here rather than relying on the
// DB default because the service layer already applies DefaultAlertThresholds
// for empty input — the DB default is a safety net, not the primary path.
func (r *RenewalPolicyRepository) Create(ctx context.Context, policy *domain.RenewalPolicy) error {
if policy == nil {
return errors.New("renewal policy is nil")
}
thresholdsJSON, err := json.Marshal(policy.AlertThresholdsDays)
if err != nil {
return fmt.Errorf("failed to marshal alert thresholds: %w", err)
}
// ID auto-generation with collision retry. We attempt up to 10 suffix
// variants (rp-foo, rp-foo-2, ..., rp-foo-10) before giving up — the
// 23505 error the caller gets back past that point is on Name (their
// job to fix) rather than on a slug-collision we swallowed.
baseID := policy.ID
if baseID == "" {
baseID = slugifyPolicyName(policy.Name)
}
insertSQL := `
INSERT INTO renewal_policies (
id, name, renewal_window_days, auto_renew, max_retries,
retry_interval_minutes, alert_thresholds_days, created_at, updated_at
) VALUES ($1, $2, $3, $4, $5, $6, $7, NOW(), NOW())
RETURNING ` + renewalPolicyColumns
maxAttempts := 10
if policy.ID != "" {
// Caller supplied a specific ID — no collision-retry, just one shot.
maxAttempts = 1
}
for attempt := 1; attempt <= maxAttempts; attempt++ {
candidateID := baseID
if attempt > 1 {
candidateID = fmt.Sprintf("%s-%d", baseID, attempt)
}
row := r.db.QueryRowContext(ctx, insertSQL,
candidateID, policy.Name, policy.RenewalWindowDays, policy.AutoRenew,
policy.MaxRetries, policy.RetryInterval, thresholdsJSON,
)
inserted, scanErr := scanRenewalPolicy(row)
if scanErr == nil {
*policy = *inserted
return nil
}
if isUniqueViolation(scanErr) {
// Determine which unique constraint — if it's the name UNIQUE
// we can't recover (caller has to pick a new name); if it's the
// primary-key slug collision we loop to the next suffix.
var pqErr *pq.Error
errors.As(scanErr, &pqErr)
// Postgres reports the constraint name in pqErr.Constraint;
// renewal_policies_name_key is the name UNIQUE, renewal_policies_pkey
// is the PK. Name collision is terminal, PK collision is retryable.
if pqErr.Constraint != "" && !strings.Contains(pqErr.Constraint, "pkey") {
return repository.ErrRenewalPolicyDuplicateName
}
// PK collision — try next suffix.
continue
}
return fmt.Errorf("failed to insert renewal policy: %w", scanErr)
}
// Exhausted retry budget on PK collisions — surface as duplicate so the
// caller at least gets a 409 rather than a mysterious 500.
return repository.ErrRenewalPolicyDuplicateName
}
// Update modifies an existing renewal policy by ID. Returns an error wrapping
// sql.ErrNoRows when id is unknown (detected by RETURNING returning zero rows),
// or ErrRenewalPolicyDuplicateName on pg 23505 (name collision with another row).
func (r *RenewalPolicyRepository) Update(ctx context.Context, id string, policy *domain.RenewalPolicy) error {
if policy == nil {
return errors.New("renewal policy is nil")
}
thresholdsJSON, err := json.Marshal(policy.AlertThresholdsDays)
if err != nil {
return fmt.Errorf("failed to marshal alert thresholds: %w", err)
}
row := r.db.QueryRowContext(ctx, `
UPDATE renewal_policies SET
name = $2,
renewal_window_days = $3,
auto_renew = $4,
max_retries = $5,
retry_interval_minutes = $6,
alert_thresholds_days = $7,
updated_at = NOW()
WHERE id = $1
RETURNING `+renewalPolicyColumns,
id, policy.Name, policy.RenewalWindowDays, policy.AutoRenew,
policy.MaxRetries, policy.RetryInterval, thresholdsJSON,
)
updated, err := scanRenewalPolicy(row)
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
// M-1: wrap repository.ErrNotFound — see Get for rationale.
return fmt.Errorf("%w: renewal policy %s", repository.ErrNotFound, id)
}
if isUniqueViolation(err) {
return repository.ErrRenewalPolicyDuplicateName
}
return fmt.Errorf("failed to update renewal policy: %w", err)
}
*policy = *updated
return nil
}
// Delete removes a renewal policy by ID. Returns ErrRenewalPolicyInUse when
// the policy is still referenced by rows in managed_certificates (pg 23503
// foreign_key_violation against the ON DELETE RESTRICT FK from
// managed_certificates.renewal_policy_id). Returns an error wrapping
// sql.ErrNoRows when id is unknown.
func (r *RenewalPolicyRepository) Delete(ctx context.Context, id string) error {
result, err := r.db.ExecContext(ctx, `DELETE FROM renewal_policies WHERE id = $1`, id)
if err != nil {
if isForeignKeyViolation(err) {
return repository.ErrRenewalPolicyInUse
}
return fmt.Errorf("failed to delete renewal policy: %w", err)
}
rows, err := result.RowsAffected()
if err != nil {
return fmt.Errorf("failed to read RowsAffected for delete: %w", err)
}
if rows == 0 {
// M-1: wrap repository.ErrNotFound — see Get for rationale.
return fmt.Errorf("%w: renewal policy %s", repository.ErrNotFound, id)
}
return nil
}
@@ -0,0 +1,317 @@
package postgres_test
// Integration tests for RenewalPolicyRepository (post-G-1, 289 lines, 5
// methods). Closes the L-1 coverage gap flagged in coverage-gap-audit.md:
// the repository's auto-generated-ID collision retry loop and its two
// typed error sentinels (ErrRenewalPolicyDuplicateName on pg 23505,
// ErrRenewalPolicyInUse on pg 23503) shipped with zero live-DB regression
// coverage — a mock-only test surface cannot exercise the PostgreSQL
// constraint semantics these paths depend on.
//
// The audit listed the file as "92 lines, 2 methods"; that was stale
// pre-G-1. Current state is 5 methods (Get/List/Create/Update/Delete),
// all covered below.
import (
"context"
"errors"
"strings"
"testing"
"time"
"github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/repository"
"github.com/shankar0123/certctl/internal/repository/postgres"
)
// TestRenewalPolicyRepository_CRUD exercises the happy path for all five
// interface methods. In particular it drives the slug-based ID
// auto-generation branch (policy.ID left empty → Create emits
// rp-<slug(name)>) so any regression to slugifyPolicyName or the retry
// loop surfaces immediately. The AlertThresholdsDays JSONB round-trip is
// asserted end-to-end: marshal on Create → store as JSONB → scan back on
// Get preserves the slice ordering and values.
func TestRenewalPolicyRepository_CRUD(t *testing.T) {
tdb := getTestDB(t)
db := tdb.freshSchema(t)
repo := postgres.NewRenewalPolicyRepository(db)
ctx := context.Background()
// Create: leave ID empty so the repository generates rp-<slug(name)>.
// "Prod TLS 90d" → rp-prod-tls-90d per slugifyPolicyName's rules
// (lowercase, spaces→hyphens, non-alphanumeric stripped).
policy := &domain.RenewalPolicy{
Name: "Prod TLS 90d",
RenewalWindowDays: 30,
AutoRenew: true,
MaxRetries: 5,
RetryInterval: 3600, // stored in retry_interval_minutes column; passthrough
AlertThresholdsDays: []int{30, 14, 7, 0},
}
if err := repo.Create(ctx, policy); err != nil {
t.Fatalf("Create failed: %v", err)
}
if policy.ID != "rp-prod-tls-90d" {
t.Errorf("auto-generated ID = %q, want %q", policy.ID, "rp-prod-tls-90d")
}
if policy.CreatedAt.IsZero() {
t.Error("Create did not populate CreatedAt (RETURNING clause regression?)")
}
if policy.UpdatedAt.IsZero() {
t.Error("Create did not populate UpdatedAt (RETURNING clause regression?)")
}
// Get: pull the just-created row back and confirm every stored field
// survives the scanRenewalPolicy path, including the JSONB unmarshal.
got, err := repo.Get(ctx, policy.ID)
if err != nil {
t.Fatalf("Get failed: %v", err)
}
if got.Name != "Prod TLS 90d" {
t.Errorf("Get: Name = %q, want %q", got.Name, "Prod TLS 90d")
}
if got.RenewalWindowDays != 30 {
t.Errorf("Get: RenewalWindowDays = %d, want 30", got.RenewalWindowDays)
}
if !got.AutoRenew {
t.Error("Get: AutoRenew = false, want true")
}
if got.MaxRetries != 5 {
t.Errorf("Get: MaxRetries = %d, want 5", got.MaxRetries)
}
if len(got.AlertThresholdsDays) != 4 {
t.Fatalf("Get: AlertThresholdsDays length = %d, want 4 (JSONB round-trip regression)", len(got.AlertThresholdsDays))
}
for i, want := range []int{30, 14, 7, 0} {
if got.AlertThresholdsDays[i] != want {
t.Errorf("Get: AlertThresholdsDays[%d] = %d, want %d", i, got.AlertThresholdsDays[i], want)
}
}
// Update: 3-arg signature is a house invariant — don't let it slip to
// 2-arg without the test catching the breakage. Tweak scalar + JSONB
// simultaneously so both SET branches exercise.
updated := *got
updated.Name = "Prod TLS 90d (tightened)"
updated.RenewalWindowDays = 45
updated.AlertThresholdsDays = []int{45, 30, 14, 7, 0}
// Sleep long enough that NOW() ticks past the Create timestamp so we
// can assert UpdatedAt monotonicity without a flaky equality check.
time.Sleep(2 * time.Millisecond)
if err := repo.Update(ctx, policy.ID, &updated); err != nil {
t.Fatalf("Update failed: %v", err)
}
if !updated.UpdatedAt.After(got.UpdatedAt) {
t.Errorf("Update: UpdatedAt %v not after Create's %v (RETURNING NOW() regression?)", updated.UpdatedAt, got.UpdatedAt)
}
refetched, err := repo.Get(ctx, policy.ID)
if err != nil {
t.Fatalf("Get after Update failed: %v", err)
}
if refetched.Name != "Prod TLS 90d (tightened)" {
t.Errorf("Get after Update: Name = %q, want %q", refetched.Name, "Prod TLS 90d (tightened)")
}
if refetched.RenewalWindowDays != 45 {
t.Errorf("Get after Update: RenewalWindowDays = %d, want 45", refetched.RenewalWindowDays)
}
if len(refetched.AlertThresholdsDays) != 5 {
t.Errorf("Get after Update: AlertThresholdsDays length = %d, want 5", len(refetched.AlertThresholdsDays))
}
// List: add a second policy so the ORDER BY name contract is non-vacuous.
second := &domain.RenewalPolicy{
Name: "Aa Earliest",
RenewalWindowDays: 14,
AutoRenew: false,
MaxRetries: 1,
RetryInterval: 60,
AlertThresholdsDays: []int{7, 0},
}
if err := repo.Create(ctx, second); err != nil {
t.Fatalf("Create second failed: %v", err)
}
all, err := repo.List(ctx)
if err != nil {
t.Fatalf("List failed: %v", err)
}
if len(all) != 2 {
t.Fatalf("List: len = %d, want 2", len(all))
}
// "Aa Earliest" sorts before "Prod TLS 90d (tightened)" under ORDER BY name ASC.
if all[0].Name != "Aa Earliest" {
t.Errorf("List[0].Name = %q, want %q (ORDER BY name regression?)", all[0].Name, "Aa Earliest")
}
// Delete: removes the policy and a follow-up Get surfaces "not found".
if err := repo.Delete(ctx, policy.ID); err != nil {
t.Fatalf("Delete failed: %v", err)
}
if _, err := repo.Get(ctx, policy.ID); err == nil {
t.Error("Get after Delete: err = nil, want not-found")
}
}
// TestRenewalPolicyRepository_DuplicateName verifies the pg 23505
// unique_violation translation. The name UNIQUE constraint is enforced
// on the renewal_policies.name column; Create's inner scanRenewalPolicy
// must see the pq.Error, call isUniqueViolation, check the constraint
// name, and return ErrRenewalPolicyDuplicateName. A non-sentinel error
// here would cause the handler to emit 500 instead of 409.
func TestRenewalPolicyRepository_DuplicateName(t *testing.T) {
tdb := getTestDB(t)
db := tdb.freshSchema(t)
repo := postgres.NewRenewalPolicyRepository(db)
ctx := context.Background()
first := &domain.RenewalPolicy{
ID: "rp-first",
Name: "Shared Name",
RenewalWindowDays: 30,
AutoRenew: true,
MaxRetries: 3,
RetryInterval: 300,
AlertThresholdsDays: domain.DefaultAlertThresholds(),
}
if err := repo.Create(ctx, first); err != nil {
t.Fatalf("Create first failed: %v", err)
}
// Second policy with a distinct ID but the same Name — the name UNIQUE
// constraint fires, Create's collision branch inspects pqErr.Constraint,
// and because it's NOT *_pkey, it returns ErrRenewalPolicyDuplicateName
// without retrying.
second := &domain.RenewalPolicy{
ID: "rp-second",
Name: "Shared Name",
RenewalWindowDays: 60,
AutoRenew: false,
MaxRetries: 1,
RetryInterval: 600,
AlertThresholdsDays: domain.DefaultAlertThresholds(),
}
err := repo.Create(ctx, second)
if err == nil {
t.Fatal("Create second: err = nil, want ErrRenewalPolicyDuplicateName")
}
if !errors.Is(err, repository.ErrRenewalPolicyDuplicateName) {
t.Errorf("Create second: err = %v, want ErrRenewalPolicyDuplicateName (via errors.Is)", err)
}
// Also verify Update surfaces the same sentinel when an existing row's
// name is changed to collide with another policy's name.
third := &domain.RenewalPolicy{
ID: "rp-third",
Name: "Third Name",
RenewalWindowDays: 90,
AutoRenew: true,
MaxRetries: 2,
RetryInterval: 1200,
AlertThresholdsDays: domain.DefaultAlertThresholds(),
}
if err := repo.Create(ctx, third); err != nil {
t.Fatalf("Create third failed: %v", err)
}
third.Name = "Shared Name" // collide with first
err = repo.Update(ctx, third.ID, third)
if err == nil {
t.Fatal("Update: err = nil, want ErrRenewalPolicyDuplicateName")
}
if !errors.Is(err, repository.ErrRenewalPolicyDuplicateName) {
t.Errorf("Update: err = %v, want ErrRenewalPolicyDuplicateName (via errors.Is)", err)
}
}
// TestRenewalPolicyRepository_DeleteInUse verifies the pg 23503
// foreign_key_violation translation. managed_certificates.renewal_policy_id
// REFERENCES renewal_policies(id) ON DELETE RESTRICT; attempting to Delete
// a policy while a certificate still references it must surface as
// ErrRenewalPolicyInUse so the handler can emit 409 Conflict. Any change
// to either the FK definition or the isForeignKeyViolation mapping breaks
// this.
func TestRenewalPolicyRepository_DeleteInUse(t *testing.T) {
tdb := getTestDB(t)
db := tdb.freshSchema(t)
repo := postgres.NewRenewalPolicyRepository(db)
ctx := context.Background()
// The policy under test — create via repo so ID auto-generation is
// also exercised end-to-end in this path.
policy := &domain.RenewalPolicy{
Name: "InUse Policy",
RenewalWindowDays: 30,
AutoRenew: true,
MaxRetries: 3,
RetryInterval: 300,
AlertThresholdsDays: domain.DefaultAlertThresholds(),
}
if err := repo.Create(ctx, policy); err != nil {
t.Fatalf("Create policy failed: %v", err)
}
// Create owner/team/issuer prerequisites, then raw-INSERT a
// managed_certificate row referencing the policy. Using raw SQL here
// (matching insertCertPrereqsRaw's idiom) keeps the test independent
// of the service layer.
ownerID, teamID, issuerID, _ := insertCertPrereqsRaw(t, db, ctx, "inuse")
now := time.Now().UTC().Truncate(time.Microsecond)
_, err := db.ExecContext(ctx, `
INSERT INTO managed_certificates (
id, name, common_name, sans, environment,
owner_id, team_id, issuer_id, renewal_policy_id,
status, expires_at, tags, created_at, updated_at
) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14)
`,
"mc-inuse", "inuse-cert", "inuse.example.com", []string{}, "production",
ownerID, teamID, issuerID, policy.ID,
string(domain.CertificateStatusActive), now.Add(90*24*time.Hour), "{}", now, now,
)
if err != nil {
t.Fatalf("INSERT managed_certificates failed: %v", err)
}
// Delete: the ON DELETE RESTRICT FK fires, the pg driver returns a
// *pq.Error with Code 23503, isForeignKeyViolation detects it, and
// the repository returns ErrRenewalPolicyInUse.
err = repo.Delete(ctx, policy.ID)
if err == nil {
t.Fatal("Delete: err = nil, want ErrRenewalPolicyInUse (ON DELETE RESTRICT should have fired)")
}
if !errors.Is(err, repository.ErrRenewalPolicyInUse) {
t.Errorf("Delete: err = %v, want ErrRenewalPolicyInUse (via errors.Is)", err)
}
// And the policy is still there — RESTRICT aborted the delete.
if _, err := repo.Get(ctx, policy.ID); err != nil {
t.Errorf("Get after failed Delete: err = %v, want nil (policy should still exist)", err)
}
// After removing the referencing cert, Delete succeeds — proves the
// RESTRICT was the only thing blocking the earlier Delete and rules
// out any unrelated failure mode.
if _, err := db.ExecContext(ctx, `DELETE FROM managed_certificates WHERE id = $1`, "mc-inuse"); err != nil {
t.Fatalf("cleanup DELETE managed_certificates failed: %v", err)
}
if err := repo.Delete(ctx, policy.ID); err != nil {
t.Errorf("Delete after cleanup: err = %v, want nil", err)
}
// Also verify Delete on a non-existent ID returns a not-found error
// (not nil, not the InUse sentinel) — guards against a silent no-op
// regression in the RowsAffected check.
err = repo.Delete(ctx, "rp-does-not-exist")
if err == nil {
t.Fatal("Delete(non-existent): err = nil, want not-found")
}
if errors.Is(err, repository.ErrRenewalPolicyInUse) {
t.Errorf("Delete(non-existent): err = %v, should not be ErrRenewalPolicyInUse", err)
}
if !strings.Contains(err.Error(), "not found") {
t.Errorf("Delete(non-existent): err = %v, want substring %q", err, "not found")
}
}
+4 -2
View File
@@ -1,6 +1,8 @@
// Package postgres_test provides repository integration tests covering 15 of 17
// Package postgres_test provides repository integration tests covering 17 of 17
// PostgreSQL repository files. Each test function exercises CRUD operations,
// edge cases, and deduplication logic against a real database.
// edge cases, and deduplication logic against a real database. HealthCheck
// and RenewalPolicy integration tests live in sibling *_test.go files in this
// package (see health_check_test.go and renewal_policy_test.go).
package postgres_test
import (
@@ -81,6 +81,29 @@ func (r *RevocationRepository) ListAll(ctx context.Context) ([]*domain.Certifica
return scanRevocations(rows)
}
// ListByIssuer returns all revocations for a single issuer, ordered by revocation time.
//
// This is the hot path for CRL generation. Pushing the issuer filter into the
// SQL query lets the composite index `idx_certificate_revocations_issuer_serial`
// (migration 000012) drive a prefix scan on issuer_id rather than forcing
// callers to load every row in the table and discard the ones belonging to
// other issuers.
func (r *RevocationRepository) ListByIssuer(ctx context.Context, issuerID string) ([]*domain.CertificateRevocation, error) {
rows, err := r.db.QueryContext(ctx, `
SELECT id, certificate_id, serial_number, reason, revoked_by, revoked_at,
issuer_id, issuer_notified, created_at
FROM certificate_revocations
WHERE issuer_id = $1
ORDER BY revoked_at ASC
`, issuerID)
if err != nil {
return nil, fmt.Errorf("failed to list revocations by issuer: %w", err)
}
defer rows.Close()
return scanRevocations(rows)
}
// ListByCertificate returns all revocations for a certificate.
func (r *RevocationRepository) ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error) {
rows, err := r.db.QueryContext(ctx, `
+12
View File
@@ -6,10 +6,12 @@ import (
"crypto/sha256"
"encoding/base64"
"encoding/hex"
"errors"
"fmt"
"log/slog"
"time"
"github.com/lib/pq"
"github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/repository"
)
@@ -409,6 +411,16 @@ func (s *AgentService) RegisterAgent(ctx context.Context, agent domain.Agent) (*
agent.LastHeartbeatAt = &now
if err := s.agentRepo.Create(ctx, &agent); err != nil {
// M-1 (P2): explicit pg-23505 dispatch wraps unique-constraint
// violations on the agent name with ErrConflict so the handler's
// errors.Is(err, service.ErrConflict) arm fires 409 deterministically
// instead of relying on the errToStatus *pq.Error SQLSTATE fallback.
// Mirrors the duplicate-name paths in IssuerService and
// RenewalPolicyService.
var pgErr *pq.Error
if errors.As(err, &pgErr) && pgErr.Code == "23505" {
return nil, fmt.Errorf("%w: agent name already exists", ErrConflict)
}
return nil, fmt.Errorf("failed to register agent: %w", err)
}
return &agent, nil
+10 -2
View File
@@ -143,12 +143,20 @@ func (s *AgentGroupService) ListMembers(ctx context.Context, id string) ([]domai
}
// validateAgentGroup checks that an agent group's configuration is valid.
//
// M-1 (P2): every return wraps ErrValidation via %w so the handler's
// errToStatus choke point dispatches these to HTTP 400 via errors.Is without
// the substring-matching on "invalid"/"required" that the pre-M-1
// agent_groups handler relied on at handler/agent_groups.go:126. The composed
// Error() string still contains the original human-readable text, so the
// handler safely passes err.Error() through to the response body on the 400
// arm. Mirrors validateProfile.
func validateAgentGroup(g *domain.AgentGroup) error {
if g.Name == "" {
return fmt.Errorf("agent group name is required")
return fmt.Errorf("%w: agent group name is required", ErrValidation)
}
if len(g.Name) > 255 {
return fmt.Errorf("agent group name exceeds 255 characters")
return fmt.Errorf("%w: agent group name exceeds 255 characters", ErrValidation)
}
return nil
}
+24 -7
View File
@@ -26,8 +26,12 @@ import (
// three cloud secret-manager discovery sources; retiring any of them orphans
// its subsystem. The guard fires unconditionally — force=true does not bypass
// it, because a sentinel is a structural invariant of the deployment, not
// a piece of fleet state the operator owns. Handler maps this to HTTP 403.
var ErrAgentIsSentinel = errors.New("agent is a reserved sentinel and cannot be retired")
// a piece of fleet state the operator owns.
//
// M-1 (P2): wraps [ErrForbidden] so the handler-layer errToStatus choke point
// resolves HTTP 403 via errors.Is. Call sites can still errors.Is against
// ErrAgentIsSentinel for domain-specific branches.
var ErrAgentIsSentinel = fmt.Errorf("%w: agent is a reserved sentinel and cannot be retired", ErrForbidden)
// ErrBlockedByDependencies is returned by RetireAgent when at least one of
// (active targets, active certificates, pending jobs) referencing the agent
@@ -35,15 +39,22 @@ var ErrAgentIsSentinel = errors.New("agent is a reserved sentinel and cannot be
// a *BlockedByDependenciesError (see below), so handlers doing errors.As
// can surface the per-bucket counts in the 409 body for operator
// troubleshooting. Tests use errors.Is; handlers use errors.As.
var ErrBlockedByDependencies = errors.New("agent has active downstream dependencies")
//
// M-1 (P2): wraps [ErrConflict] so errToStatus resolves HTTP 409 via
// errors.Is, keeping the per-bucket errors.As path unchanged for the
// detailed 409 body. BlockedByDependenciesError.Unwrap still returns the
// domain-specific sentinel for chained Is checks.
var ErrBlockedByDependencies = fmt.Errorf("%w: agent has active downstream dependencies", ErrConflict)
// ErrForceReasonRequired is returned when force=true is supplied without a
// non-empty reason. The force escape hatch is deliberately chatty: operators
// pulling the emergency cord must leave an auditable breadcrumb explaining
// why a cascade was justified. Handler maps this to HTTP 400 so the operator
// retries with --reason rather than silently skipping the guard. Checked
// before any DB mutation to keep the no-reason path transactionally clean.
var ErrForceReasonRequired = errors.New("force=true requires a non-empty reason")
// why a cascade was justified. Operators must retry with --reason rather
// than silently skipping the guard. Checked before any DB mutation to keep
// the no-reason path transactionally clean.
//
// M-1 (P2): wraps [ErrValidation] so errToStatus resolves HTTP 400.
var ErrForceReasonRequired = fmt.Errorf("%w: force=true requires a non-empty reason", ErrValidation)
// ErrAgentRetired is returned by Heartbeat (and any future agent-authenticated
// call site) when a retired agent is still polling. The handler layer maps
@@ -52,6 +63,12 @@ var ErrForceReasonRequired = errors.New("force=true requires a non-empty reason"
// forever on a soft-retired identity. IsRetired() on the domain model is
// the single source of truth; the sentinel exists so service and handler
// callers can errors.Is against one symbol.
//
// M-1 (P2): deliberately NOT wrapped under any generic sentinel. 410 Gone is
// semantically distinct from 403/404/409 — it signals a permanently-terminated
// resource identity and drives deterministic agent-binary shutdown (see
// cmd/agent/main.go). errToStatus tests it FIRST so it cannot be demoted by
// the generic cascade.
var ErrAgentRetired = errors.New("agent has been retired")
// BlockedByDependenciesError wraps ErrBlockedByDependencies and carries the
+10 -1
View File
@@ -143,6 +143,15 @@ func (s *AuditService) ListAuditEvents(ctx context.Context, page, perPage int) (
}
// GetAuditEvent returns a single audit event (handler interface method).
//
// M-1 (P2): the pre-M-1 zero-events path returned a bare
// `fmt.Errorf("audit event not found")` and the handler dispatched via a
// blanket `any error → 404 Audit event not found` shortcut. That silently
// demoted transient DB failures from the auditRepo.List wrap (line 154 above)
// to 404 Not Found. Now the zero-events path wraps ErrNotFound via %w so
// errors.Is(err, service.ErrNotFound) picks up the real 404 at the handler's
// errToStatus choke point, and the repo.List wrap surfaces as 500 with
// server-side slog.Error capture (F-002 redacted-500 pattern preserved).
func (s *AuditService) GetAuditEvent(ctx context.Context, id string) (*domain.AuditEvent, error) {
filter := &repository.AuditFilter{
ResourceID: id,
@@ -155,7 +164,7 @@ func (s *AuditService) GetAuditEvent(ctx context.Context, id string) (*domain.Au
}
if len(events) == 0 {
return nil, fmt.Errorf("audit event not found")
return nil, fmt.Errorf("%w: audit event not found", ErrNotFound)
}
return events[0], nil
+24 -10
View File
@@ -43,6 +43,15 @@ func (s *CAOperationsSvc) SetIssuerRegistry(registry *IssuerRegistry) {
// GenerateDERCRL generates a DER-encoded X.509 CRL for the given issuer.
// Short-lived certificates (profile TTL < 1 hour) are excluded from the CRL.
//
// M-1 (P2): the pre-M-1 issuer-not-found path returned a bare
// `fmt.Errorf("issuer not found: %s", issuerID)` with no sentinel wrap. A
// pre-M-1 handler strings.Contains(err.Error(), "not found") classifier would
// sweep both this path and any transient error whose text happened to mention
// "not found" into 404. Post-M-1 this path wraps service.ErrNotFound via %w so
// errors.Is picks up the genuine 404 at errToStatus, and truly-unexpected
// errors (e.g., registry misconfigured) surface as 500 via the generic-error
// fallback.
func (s *CAOperationsSvc) GenerateDERCRL(ctx context.Context, issuerID string) ([]byte, error) {
if s.revocationRepo == nil {
return nil, fmt.Errorf("revocation repository not configured")
@@ -53,22 +62,22 @@ func (s *CAOperationsSvc) GenerateDERCRL(ctx context.Context, issuerID string) (
issuerConn, ok := s.issuerRegistry.Get(issuerID)
if !ok {
return nil, fmt.Errorf("issuer not found: %s", issuerID)
return nil, fmt.Errorf("%w: issuer %s", ErrNotFound, issuerID)
}
revocations, err := s.revocationRepo.ListAll(ctx)
// Scope the query to this issuer so the migration 000012 composite index
// drives a prefix scan; previously this path read every revocation in the
// table and filtered in Go, which did not scale as the revocation table
// grew across many issuers (F-001).
revocations, err := s.revocationRepo.ListByIssuer(ctx, issuerID)
if err != nil {
return nil, fmt.Errorf("failed to list revocations: %w", err)
return nil, fmt.Errorf("failed to list revocations for issuer %s: %w", issuerID, err)
}
// Filter to this issuer and convert to CRL entries.
// Short-lived certificates (profile TTL < 1 hour) are excluded — expiry is sufficient revocation.
// Convert revocations to CRL entries. Short-lived certificates (profile
// TTL < 1 hour) are excluded — expiry is sufficient revocation.
var entries []CRLEntry
for _, rev := range revocations {
if rev.IssuerID != issuerID {
continue
}
// Check short-lived exemption: look up the cert's profile
if s.profileRepo != nil && s.certRepo != nil {
cert, err := s.certRepo.Get(ctx, rev.CertificateID)
@@ -98,6 +107,11 @@ func (s *CAOperationsSvc) GenerateDERCRL(ctx context.Context, issuerID string) (
}
// GetOCSPResponse generates a signed OCSP response for the given certificate serial.
//
// M-1 (P2): see GenerateDERCRL above — same sentinel-wrap rationale applies.
// The issuer-not-found path wraps service.ErrNotFound via %w so errors.Is
// picks up the genuine 404 at errToStatus; transient/misconfiguration errors
// surface as 500.
func (s *CAOperationsSvc) GetOCSPResponse(ctx context.Context, issuerID string, serialHex string) ([]byte, error) {
if s.revocationRepo == nil {
return nil, fmt.Errorf("revocation repository not configured")
@@ -108,7 +122,7 @@ func (s *CAOperationsSvc) GetOCSPResponse(ctx context.Context, issuerID string,
issuerConn, ok := s.issuerRegistry.Get(issuerID)
if !ok {
return nil, fmt.Errorf("issuer not found: %s", issuerID)
return nil, fmt.Errorf("%w: issuer %s", ErrNotFound, issuerID)
}
serial := new(big.Int)
+67
View File
@@ -75,6 +75,73 @@ func TestCAOperationsSvc_GenerateDERCRL_Success(t *testing.T) {
t.Logf("DER CRL generated successfully: %d bytes", len(crl))
}
// TestCAOperationsSvc_GenerateDERCRL_UsesListByIssuer_NotListAll guards F-001.
// Before the fix, GenerateDERCRL called revocationRepo.ListAll(ctx) and filtered
// results in Go (if rev.IssuerID != issuerID { continue }). That was O(N) in the
// size of the entire revocation table and did not scale as revocations piled up
// across many issuers. Migration 000012 added the composite index
// idx_certificate_revocations_issuer_serial(issuer_id, serial_number), which is
// a prefix scan target — so the hot path must now call ListByIssuer(ctx, id) to
// drive an indexed query. This regression test asserts the hot path invokes
// ListByIssuer exactly once and never falls back to the full-table ListAll scan,
// and also double-checks that cross-issuer revocations are correctly excluded
// from the generated CRL (no in-Go filter left to catch them).
func TestCAOperationsSvc_GenerateDERCRL_UsesListByIssuer_NotListAll(t *testing.T) {
caSvc, revocationRepo, _ := newCAOperationsSvcTest()
// Pre-populate with revocations from TWO issuers. If the hot path regresses
// and calls ListAll instead of ListByIssuer, the generated CRL would either
// include the wrong rows or — with the in-Go filter gone — pull in both
// issuers' revocations. ListByIssuer scopes at the query level so only
// iss-local rows come back.
now := time.Now()
revocationRepo.Revocations = []*domain.CertificateRevocation{
{
SerialNumber: "LOCAL-001",
CertificateID: "cert-local-1",
IssuerID: "iss-local",
Reason: "keyCompromise",
RevokedAt: now.Add(-24 * time.Hour),
RevokedBy: "admin",
},
{
SerialNumber: "LOCAL-002",
CertificateID: "cert-local-2",
IssuerID: "iss-local",
Reason: "superseded",
RevokedAt: now.Add(-12 * time.Hour),
RevokedBy: "admin",
},
{
SerialNumber: "OTHER-001",
CertificateID: "cert-other-1",
IssuerID: "iss-other",
Reason: "keyCompromise",
RevokedAt: now.Add(-6 * time.Hour),
RevokedBy: "admin",
},
}
crl, err := caSvc.GenerateDERCRL(context.Background(), "iss-local")
if err != nil {
t.Fatalf("expected no error, got: %v", err)
}
if len(crl) == 0 {
t.Fatal("expected non-empty CRL")
}
// The contractual assertion: the CRL hot path MUST use the scoped query.
if got, want := revocationRepo.ListByIssuerCalls, 1; got != want {
t.Errorf("ListByIssuerCalls = %d, want %d — CRL hot path must call the scoped query driven by migration 000012 index", got, want)
}
if got := revocationRepo.ListAllCalls; got != 0 {
t.Errorf("ListAllCalls = %d, want 0 — CRL hot path must NOT fall back to the full-table scan after F-001", got)
}
if got, want := revocationRepo.LastListIssuerID, "iss-local"; got != want {
t.Errorf("LastListIssuerID = %q, want %q — issuer scoping argument lost", got, want)
}
}
func TestCAOperationsSvc_GenerateDERCRL_EmptyCRL(t *testing.T) {
caSvc, revocationRepo, _ := newCAOperationsSvcTest()
+19 -4
View File
@@ -376,10 +376,21 @@ func (s *CertificateService) CreateCertificate(ctx context.Context, cert domain.
// UpdateCertificate modifies a certificate (handler interface method).
func (s *CertificateService) UpdateCertificate(ctx context.Context, id string, patch domain.ManagedCertificate) (*domain.ManagedCertificate, error) {
// Fetch existing certificate so partial updates don't zero out fields
// Fetch existing certificate so partial updates don't zero out fields.
//
// M-1 (P2): the pre-M-1 wrap was `"certificate not found: %w"` on every
// certRepo.Get error — which coupled to the handler's strings.Contains
// substring classifier on "not found" and gave false positives on transient
// DB failures (connection refused, context deadline, etc.), demoting a 500
// to a 404. Now the repo wraps only the genuine sql.ErrNoRows path with
// repository.ErrNotFound (certificate.go Get), so the errors.Is walk
// through the handler's errToStatus choke point discriminates correctly:
// truly-missing → 404, everything else → 500 (the intended outcome). The
// wrap text is changed from "certificate not found" to "failed to get
// certificate" to match the semantic.
existing, err := s.certRepo.Get(ctx, id)
if err != nil {
return nil, fmt.Errorf("certificate not found: %w", err)
return nil, fmt.Errorf("failed to get certificate: %w", err)
}
// Merge non-zero fields from patch into existing
@@ -501,10 +512,14 @@ func (s *CertificateService) GetOCSPResponse(ctx context.Context, issuerID strin
// GetCertificateDeployments returns all deployment targets for a certificate (M20).
func (s *CertificateService) GetCertificateDeployments(ctx context.Context, certID string) ([]domain.DeploymentTarget, error) {
// Verify certificate exists
// Verify certificate exists. See M-1 (P2) note on UpdateCertificate for
// the wrap-text correction rationale — same treatment applies here: the
// repo's sql.ErrNoRows wrap with repository.ErrNotFound escapes cleanly
// through fmt.Errorf("%w", ...) so errors.Is picks up the genuine 404
// while transient DB errors correctly surface as 500.
_, err := s.certRepo.Get(ctx, certID)
if err != nil {
return nil, fmt.Errorf("certificate not found: %w", err)
return nil, fmt.Errorf("failed to get certificate: %w", err)
}
if s.targetRepo == nil {
+13 -2
View File
@@ -166,9 +166,20 @@ func (s *DiscoveryService) ClaimDiscovered(ctx context.Context, id string, manag
return fmt.Errorf("failed to get discovered certificate: %w", err)
}
// Verify the managed cert exists
// Verify the managed cert exists.
//
// M-1 (P2): the pre-M-1 wrap was a bare `fmt.Errorf("managed certificate not
// found: %s", managedCertID)` with no %w and no sentinel. That threw away the
// underlying error entirely — a transient DB failure from certRepo.Get (e.g.,
// connection dropped, query timeout) would be flattened into the same "not
// found" string as a genuine sql.ErrNoRows, which the pre-M-1 handler
// strings.Contains classifier then swept into 404. Post-M-1 we propagate the
// underlying error via %w so errors.Is walks the chain correctly:
// certRepo.Get wraps genuine sql.ErrNoRows with repository.ErrNotFound (see
// postgres/certificate.go Get), and errToStatus dispatches that to 404;
// transient errors surface as 500 via the generic-error fallback.
if _, err := s.certRepo.Get(ctx, managedCertID); err != nil {
return fmt.Errorf("managed certificate not found: %s", managedCertID)
return fmt.Errorf("failed to get managed certificate %s: %w", managedCertID, err)
}
if err := s.discoveryRepo.UpdateDiscoveredStatus(ctx, id, domain.DiscoveryStatusManaged, managedCertID); err != nil {
+73
View File
@@ -0,0 +1,73 @@
package service
import "errors"
// M-1 (P2) coverage-gap closure: generic service-layer error sentinels.
//
// Before M-1, API handlers classified service errors by substring-matching the
// wrapped message text (`strings.Contains(err.Error(), "not found")` and
// friends). That made every HTTP status mapping one `fmt.Errorf` reword away
// from silently regressing — including the M-003 self-approval privilege
// boundary, where `handler/jobs.go:174/:220` ignored the already-defined
// ErrSelfApproval sentinel and relied on the literal string "cannot approve".
//
// These six generic sentinels form the type-safe surface the handler layer
// dispatches against via `errors.Is`. Domain-specific sentinels (ErrSelfApproval,
// ErrAgentIsSentinel, ErrBlockedByDependencies, ErrForceReasonRequired,
// ErrAgentNotFound) are declared in their own topical files (job.go,
// agent_retire.go, target.go) and wrap one of these generics via
// `fmt.Errorf("%w: ...", ErrForbidden)`. The wrap chain lets call sites continue
// to `errors.Is(err, ErrSelfApproval)` for domain-specific logic while the
// handler's single choke point in `api/handler/errors.go` can match on the
// generic sentinel to pick the HTTP status.
//
// Dispatch order in errToStatus matters — see the doc block at the top of
// `internal/api/handler/errors.go`.
//
// ErrAgentRetired is deliberately NOT wrapped here. 410 Gone is semantically
// distinct from 403/404/409 and must short-circuit the generic dispatch. Keep
// its standalone declaration in agent_retire.go untouched; errToStatus tests
// it first.
var (
// ErrNotFound indicates a lookup for a resource that does not exist.
// Handlers translate this to HTTP 404.
ErrNotFound = errors.New("not found")
// ErrValidation indicates malformed, missing, or out-of-range input from
// the caller. Handlers translate this to HTTP 400.
ErrValidation = errors.New("validation failed")
// ErrConflict indicates a state conflict: unique-constraint violation,
// foreign-key dependency, or a state machine transition that is not
// allowed from the current state. Handlers translate this to HTTP 409.
ErrConflict = errors.New("conflict")
// ErrForbidden indicates an authorization / privilege-boundary denial.
// The caller is authenticated but is not permitted to perform the action.
// Handlers translate this to HTTP 403.
ErrForbidden = errors.New("forbidden")
// ErrUnauthenticated indicates the caller failed to authenticate — most
// commonly a SCEP challenge-password mismatch, where the transport itself
// is valid but the application-layer credential is wrong. Handlers
// translate this to HTTP 401.
ErrUnauthenticated = errors.New("unauthenticated")
// ErrNotImplemented indicates the requested operation is defined but not
// yet wired up — reserved for feature-flag-gated code paths. Handlers
// translate this to HTTP 501.
ErrNotImplemented = errors.New("not implemented")
// ErrUnprocessable indicates the request was well-formed and the
// referenced resource exists, but server-side stored data could not be
// processed — e.g., a certificate PEM in inventory that fails X.509
// decoding because the stored blob is corrupt or was inserted with the
// wrong encoding. Distinct from ErrValidation: ErrValidation means the
// caller sent bad input (400), while ErrUnprocessable means the caller's
// input was fine but our own data cannot satisfy the operation (422
// Unprocessable Entity). Today the only call site is ExportPKCS12's parse
// path in internal/service/export.go; keeping the sentinel generic so
// other "stored-data-unparseable" paths can reuse it without inventing a
// second variant.
ErrUnprocessable = errors.New("unprocessable")
)
+110
View File
@@ -0,0 +1,110 @@
package service
import (
"errors"
"fmt"
"testing"
)
// TestGenericSentinels_IdentityDistinct guards against an accidental
// `var ErrX = ErrY` alias where two generic sentinels share identity. Each
// must be a distinct error value so errors.Is dispatch in errToStatus can
// route them to different HTTP status codes.
func TestGenericSentinels_IdentityDistinct(t *testing.T) {
sentinels := []struct {
name string
err error
}{
{"ErrNotFound", ErrNotFound},
{"ErrValidation", ErrValidation},
{"ErrConflict", ErrConflict},
{"ErrForbidden", ErrForbidden},
{"ErrUnauthenticated", ErrUnauthenticated},
{"ErrNotImplemented", ErrNotImplemented},
}
for i := range sentinels {
for j := range sentinels {
if i == j {
continue
}
if errors.Is(sentinels[i].err, sentinels[j].err) {
t.Errorf("%s and %s alias the same error value — each generic sentinel must be distinct",
sentinels[i].name, sentinels[j].name)
}
}
}
}
// TestWrappedSentinels_ChainWalk is the core M-1 invariant: wrapping a
// domain-specific sentinel under a generic sentinel via fmt.Errorf("%w: ...")
// must preserve BOTH identities on the wrap chain. Call sites that check
// errors.Is(err, ErrSelfApproval) for domain logic AND the handler-layer
// errToStatus that checks errors.Is(err, ErrForbidden) for the HTTP status
// both need to succeed on the same error value.
//
// If this test fails, every handler dispatch that routes through errToStatus
// is silently broken.
func TestWrappedSentinels_ChainWalk(t *testing.T) {
cases := []struct {
name string
err error
generic error
}{
{"ErrSelfApproval wraps ErrForbidden", ErrSelfApproval, ErrForbidden},
{"ErrAgentIsSentinel wraps ErrForbidden", ErrAgentIsSentinel, ErrForbidden},
{"ErrBlockedByDependencies wraps ErrConflict", ErrBlockedByDependencies, ErrConflict},
{"ErrForceReasonRequired wraps ErrValidation", ErrForceReasonRequired, ErrValidation},
{"ErrAgentNotFound wraps ErrValidation", ErrAgentNotFound, ErrValidation},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
if !errors.Is(c.err, c.generic) {
t.Errorf("errors.Is(%v, %v) = false; want true", c.err, c.generic)
}
if !errors.Is(c.err, c.err) {
t.Errorf("errors.Is(err, err) = false; want true — domain sentinel lost self-identity after wrap")
}
})
}
}
// TestErrAgentRetired_StandaloneGone locks the 410 Gone semantics in place.
// ErrAgentRetired MUST NOT wrap any generic sentinel — 410 is semantically
// distinct from 403/404/409 (permanently-terminated resource identity) and
// the errToStatus dispatch tests it FIRST before any generic check. If this
// test goes red because someone wrapped it under ErrForbidden or ErrNotFound,
// the agent-binary shutdown behavior at cmd/agent/main.go:1291 silently
// regresses.
func TestErrAgentRetired_StandaloneGone(t *testing.T) {
if errors.Is(ErrAgentRetired, ErrForbidden) {
t.Error("ErrAgentRetired must NOT wrap ErrForbidden — 410 Gone would demote to 403")
}
if errors.Is(ErrAgentRetired, ErrNotFound) {
t.Error("ErrAgentRetired must NOT wrap ErrNotFound — 410 Gone would demote to 404")
}
if errors.Is(ErrAgentRetired, ErrConflict) {
t.Error("ErrAgentRetired must NOT wrap ErrConflict — 410 Gone would demote to 409")
}
if errors.Is(ErrAgentRetired, ErrValidation) {
t.Error("ErrAgentRetired must NOT wrap ErrValidation — 410 Gone would demote to 400")
}
if !errors.Is(ErrAgentRetired, ErrAgentRetired) {
t.Error("ErrAgentRetired lost self-identity")
}
}
// TestSentinels_SurviveDoubleWrap verifies that wrapping a sentinel-wrapped
// error a SECOND time (e.g., a call site doing fmt.Errorf("%w: ctx-info",
// ErrSelfApproval)) preserves both the domain sentinel and the generic
// sentinel. This is critical because the errToStatus dispatch order places
// ErrForbidden BEFORE ErrValidation — if double-wrapping broke the chain,
// the M-003 gate would demote to 400.
func TestSentinels_SurviveDoubleWrap(t *testing.T) {
doubled := fmt.Errorf("%w: additional context from call site", ErrSelfApproval)
if !errors.Is(doubled, ErrSelfApproval) {
t.Error("double-wrapped ErrSelfApproval lost domain identity")
}
if !errors.Is(doubled, ErrForbidden) {
t.Error("double-wrapped ErrSelfApproval lost ErrForbidden wrap — M-003 gate would demote to 500")
}
}
+37 -11
View File
@@ -38,16 +38,31 @@ type ExportPEMResult struct {
// ExportPEM returns the PEM-encoded certificate and chain for the latest version.
func (s *ExportService) ExportPEM(ctx context.Context, certID string) (*ExportPEMResult, error) {
// Verify certificate exists
// Verify certificate exists.
//
// M-1 (P2): the pre-M-1 wrap was `"certificate not found: %w"` on every
// certRepo.Get error — which gave the handler's strings.Contains(err.Error(),
// "not found") check a false positive on transient DB failures (connection
// refused, context deadline, etc.), demoting a 500 to a 404. Now the repo
// wraps only the genuine sql.ErrNoRows path with repository.ErrNotFound
// (certificate.go Get), so the errors.Is walk through the handler's
// errToStatus discriminates correctly: truly-missing → 404, everything else
// → 500 (the intended outcome). The wrap text is changed from "certificate
// not found" to "failed to get certificate" to match the semantic.
cert, err := s.certRepo.Get(ctx, certID)
if err != nil {
return nil, fmt.Errorf("certificate not found: %w", err)
return nil, fmt.Errorf("failed to get certificate: %w", err)
}
// Get latest version (contains the PEM chain)
// Get latest version (contains the PEM chain).
//
// M-1 (P2): same wrap-text correction as above — pre-M-1 any
// GetLatestVersion error surfaced as "no certificate version found",
// which bled into the handler's substring classifier. Now the repo wraps
// sql.ErrNoRows with repository.ErrNotFound; the wrap chain walks cleanly.
version, err := s.certRepo.GetLatestVersion(ctx, certID)
if err != nil {
return nil, fmt.Errorf("no certificate version found: %w", err)
return nil, fmt.Errorf("failed to get latest certificate version: %w", err)
}
// Split PEM chain into leaf cert + chain
@@ -73,26 +88,37 @@ func (s *ExportService) ExportPEM(ctx context.Context, certID string) (*ExportPE
// The private key is NOT included — it lives on the agent and never touches the control plane.
// The PKCS#12 bundle is encrypted with the provided password (can be empty for cert-only bundles).
func (s *ExportService) ExportPKCS12(ctx context.Context, certID string, password string) ([]byte, error) {
// Verify certificate exists
// Verify certificate exists. See M-1 (P2) note on ExportPEM for the wrap-text
// correction — same rationale applies here.
cert, err := s.certRepo.Get(ctx, certID)
if err != nil {
return nil, fmt.Errorf("certificate not found: %w", err)
return nil, fmt.Errorf("failed to get certificate: %w", err)
}
// Get latest version
// Get latest version. Same wrap-text correction as ExportPEM.
version, err := s.certRepo.GetLatestVersion(ctx, certID)
if err != nil {
return nil, fmt.Errorf("no certificate version found: %w", err)
return nil, fmt.Errorf("failed to get latest certificate version: %w", err)
}
// Parse PEM chain into x509.Certificate objects
// Parse PEM chain into x509.Certificate objects.
//
// M-1 (P2): wrap both parse-failure paths with ErrUnprocessable so the
// handler's errToStatus choke point dispatches to 422 Unprocessable Entity
// via errors.Is instead of the pre-M-1 two-term substring net
// (`"cannot be parsed"|"no certificates found"`) at handler/export.go:101.
// 422 is the correct status here — the caller's request is syntactically
// fine; the stored PEM chain is what can't be processed. The composed
// Error() string still carries the "certificate data cannot be parsed as
// X.509"/"no certificates found in PEM chain" wording so server-side
// slog.Error capture and any future 422 body propagation stay readable.
certs, err := parsePEMCertificates(version.PEMChain)
if err != nil {
return nil, fmt.Errorf("certificate data cannot be parsed as X.509: %w", err)
return nil, fmt.Errorf("%w: certificate data cannot be parsed as X.509: %v", ErrUnprocessable, err)
}
if len(certs) == 0 {
return nil, fmt.Errorf("no certificates found in PEM chain")
return nil, fmt.Errorf("%w: no certificates found in PEM chain", ErrUnprocessable)
}
// Build PKCS#12 bundle: leaf cert + CA chain (no private key)
+9 -1
View File
@@ -262,10 +262,18 @@ func (s *IssuerService) Delete(ctx context.Context, id string, actor string) err
// TestConnection tests the connection to an issuer by instantiating a throwaway
// connector and calling ValidateConfig. Records the result in the database.
//
// M-1 (P2): the pre-M-1 wrap was `"issuer not found: %w"` on every
// issuerRepo.Get error — which coupled to handler substring classifiers on
// "not found" and could have demoted transient DB failures to 404. Now the
// repo wraps only the genuine sql.ErrNoRows path with repository.ErrNotFound
// (postgres/issuer.go Get), so errors.Is walks the wrap chain correctly:
// truly-missing → 404, everything else → 500. The wrap text is changed from
// "issuer not found" to "failed to get issuer" to match the semantic.
func (s *IssuerService) TestConnection(ctx context.Context, id string) error {
iss, err := s.issuerRepo.Get(ctx, id)
if err != nil {
return fmt.Errorf("issuer not found: %w", err)
return fmt.Errorf("failed to get issuer: %w", err)
}
// Get the decrypted config
+24 -5
View File
@@ -3,7 +3,6 @@ package service
import (
"time"
"context"
"errors"
"fmt"
"log/slog"
"strings"
@@ -16,8 +15,16 @@ import (
// approve a renewal job is the same person listed as the owner of the
// underlying certificate. M-003 enforces separation of duties: the owner who
// requested (or benefits from) the renewal must not be the same identity that
// approves it. Handlers map this sentinel to HTTP 403 Forbidden.
var ErrSelfApproval = errors.New("self-approval forbidden: actor is the owner of the certificate")
// approves it.
//
// M-1 (P2): wraps [ErrForbidden] via fmt.Errorf("%w", ...) so the single
// handler-layer errToStatus choke point resolves HTTP 403 via
// errors.Is(err, ErrForbidden). Call sites can continue to
// errors.Is(err, ErrSelfApproval) for domain-specific logging — both succeed
// because %w builds a wrap chain. Pre-M-1, handler/jobs.go classified the
// failure by substring-matching "cannot approve" in the message; a reword
// would have silently demoted 403 to 500 with no observable signal.
var ErrSelfApproval = fmt.Errorf("%w: self-approval forbidden: actor is the owner of the certificate", ErrForbidden)
// JobService manages job processing and status tracking.
// It coordinates between the scheduler and various job-specific services.
@@ -403,9 +410,17 @@ func (s *JobService) GetJob(ctx context.Context, id string) (*domain.Job, error)
// duties. Callers must pass a non-empty actor; empty actor is treated as an
// anonymous system caller and permitted (internal/system paths).
func (s *JobService) ApproveJob(ctx context.Context, id, actor string) error {
// M-1 (P2): the pre-M-1 wrap was `"job not found: %w"` on every jobRepo.Get
// error — which coupled to the handler's strings.Contains substring classifier
// on "not found" and gave false positives on transient DB failures, demoting a
// 500 to a 404. Now the repo wraps only the genuine sql.ErrNoRows path with
// repository.ErrNotFound (postgres/job.go Get), so errors.Is walks the wrap
// chain correctly: truly-missing → 404, everything else → 500. The wrap text
// is changed from "job not found" to "failed to get job" to match the
// semantic.
job, err := s.jobRepo.Get(ctx, id)
if err != nil {
return fmt.Errorf("job not found: %w", err)
return fmt.Errorf("failed to get job: %w", err)
}
if job.Status != domain.JobStatusAwaitingApproval {
@@ -435,9 +450,13 @@ func (s *JobService) ApproveJob(ctx context.Context, id, actor string) error {
// owner is permitted to cancel their own pending renewal. actor is recorded
// on the log line for audit attribution.
func (s *JobService) RejectJob(ctx context.Context, id, reason, actor string) error {
// M-1 (P2): wrap-text correction — see ApproveJob for rationale. Same
// repo.Get sql.ErrNoRows → repository.ErrNotFound propagation; text changed
// from "job not found" to "failed to get job" so transient DB errors stay
// 500 while genuine 404s are picked up via errors.Is at errToStatus.
job, err := s.jobRepo.Get(ctx, id)
if err != nil {
return fmt.Errorf("job not found: %w", err)
return fmt.Errorf("failed to get job: %w", err)
}
if job.Status != domain.JobStatusAwaitingApproval {
+17 -1
View File
@@ -383,6 +383,15 @@ func (s *NotificationService) ListNotifications(ctx context.Context, page, perPa
}
// GetNotification returns a single notification (handler interface method).
//
// M-1 (P2): the not-found branch now wraps service.ErrNotFound via %w so the
// handler's errToStatus choke point dispatches to 404 via errors.Is. Pre-M-1
// this returned a raw fmt.Errorf("notification not found") and the handler
// relied on a blanket "any error from this service call → 404" shortcut —
// which silently demoted transient DB failures (the List() error wrap above)
// to 404. Post-M-1 the handler uses errToStatus; the List() error wrap still
// surfaces as 500 (no sentinel attached), and only this explicit not-found
// branch maps to 404.
func (s *NotificationService) GetNotification(ctx context.Context, id string) (*domain.NotificationEvent, error) {
filter := &repository.NotificationFilter{
PerPage: 1,
@@ -400,7 +409,7 @@ func (s *NotificationService) GetNotification(ctx context.Context, id string) (*
}
}
return nil, fmt.Errorf("notification not found")
return nil, fmt.Errorf("%w: notification %s", ErrNotFound, id)
}
// MarkAsRead marks a notification as read (handler interface method).
@@ -568,6 +577,13 @@ func (s *NotificationService) RetryFailedNotifications(ctx context.Context) erro
// "pg: deadlock detected" surfaces in the handler response and the
// operator UI. The service has no fallback — a silent "success" that
// didn't actually mutate the row would be worse than a loud error.
//
// M-1 (P2): the repo's Requeue (postgres/notification.go) now wraps
// repository.ErrNotFound on zero-rows-affected. Because this method wraps
// the repo error with %w, errors.Is(err, repository.ErrNotFound) walks
// cleanly through both wrap layers — so the handler's errToStatus choke
// point dispatches a genuine missing-row Requeue to 404 and a transient
// "pg: deadlock detected" to 500 without substring matching.
func (s *NotificationService) RequeueNotification(ctx context.Context, id string) error {
if err := s.notifRepo.Requeue(ctx, id); err != nil {
return fmt.Errorf("failed to requeue notification: %w", err)
+17 -8
View File
@@ -139,42 +139,51 @@ func (s *ProfileService) Get(ctx context.Context, id string) (*domain.Certificat
}
// validateProfile checks that a profile's configuration is valid.
//
// M-1: every return here wraps ErrValidation via %w so the handler's
// errToStatus choke point dispatches these to HTTP 400 via errors.Is without
// the substring-matching on "invalid"/"required"/"must be"/"cannot" that the
// pre-M-1 profile handler relied on. The composed Error() string still
// contains the original human-readable text (e.g. "RSA minimum key size must
// be at least 2048"), so the handler safely passes err.Error() through to the
// response body on the 400 arm. Substring assertions in profile_test.go
// continue to match for the same reason.
func validateProfile(p *domain.CertificateProfile) error {
if p.Name == "" {
return fmt.Errorf("profile name is required")
return fmt.Errorf("%w: profile name is required", ErrValidation)
}
if len(p.Name) > 255 {
return fmt.Errorf("profile name exceeds 255 characters")
return fmt.Errorf("%w: profile name exceeds 255 characters", ErrValidation)
}
// Validate key algorithms
for _, alg := range p.AllowedKeyAlgorithms {
if !domain.ValidKeyAlgorithms[alg.Algorithm] {
return fmt.Errorf("invalid key algorithm: %s (allowed: RSA, ECDSA, Ed25519)", alg.Algorithm)
return fmt.Errorf("%w: invalid key algorithm: %s (allowed: RSA, ECDSA, Ed25519)", ErrValidation, alg.Algorithm)
}
if alg.Algorithm == domain.KeyAlgorithmRSA && alg.MinSize < 2048 {
return fmt.Errorf("RSA minimum key size must be at least 2048, got %d", alg.MinSize)
return fmt.Errorf("%w: RSA minimum key size must be at least 2048, got %d", ErrValidation, alg.MinSize)
}
if alg.Algorithm == domain.KeyAlgorithmECDSA && alg.MinSize < 256 {
return fmt.Errorf("ECDSA minimum key size must be at least 256, got %d", alg.MinSize)
return fmt.Errorf("%w: ECDSA minimum key size must be at least 256, got %d", ErrValidation, alg.MinSize)
}
}
// Validate EKUs
for _, eku := range p.AllowedEKUs {
if !domain.ValidEKUs[eku] {
return fmt.Errorf("invalid EKU: %s", eku)
return fmt.Errorf("%w: invalid EKU: %s", ErrValidation, eku)
}
}
// Validate max TTL
if p.MaxTTLSeconds < 0 {
return fmt.Errorf("max_ttl_seconds cannot be negative")
return fmt.Errorf("%w: max_ttl_seconds cannot be negative", ErrValidation)
}
// Validate short-lived consistency
if p.AllowShortLived && p.MaxTTLSeconds >= 3600 {
return fmt.Errorf("allow_short_lived is true but max_ttl_seconds (%d) is >= 3600; short-lived certs must have TTL under 1 hour", p.MaxTTLSeconds)
return fmt.Errorf("%w: allow_short_lived is true but max_ttl_seconds (%d) is >= 3600; short-lived certs must have TTL under 1 hour", ErrValidation, p.MaxTTLSeconds)
}
return nil
+211
View File
@@ -0,0 +1,211 @@
package service
import (
"context"
"fmt"
"regexp"
"strings"
"time"
"github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/repository"
)
// G-1: service-level sentinels alias the repository sentinels so errors.Is
// walks transparently across layers. Do NOT errors.New a fresh copy — the
// handler's `errors.Is(err, repository.ErrRenewalPolicyInUse)` branch and
// the service-layer tests' `errors.Is(err, service.ErrRenewalPolicyInUse)`
// branch need to match the same sentinel var identity.
var (
ErrRenewalPolicyDuplicateName = repository.ErrRenewalPolicyDuplicateName
ErrRenewalPolicyInUse = repository.ErrRenewalPolicyInUse
)
// RenewalPolicyService implements the /api/v1/renewal-policies CRUD surface.
//
// G-1 scope note: the red-test contract pins NewRenewalPolicyService to a
// repo-only signature (no auditService). Renewal-policy CRUD does not emit
// audit events in this change — if audit coverage is needed later, add a
// SetAuditService setter rather than churning the constructor signature.
type RenewalPolicyService struct {
repo repository.RenewalPolicyRepository
}
// NewRenewalPolicyService constructs the service bound to its repository.
func NewRenewalPolicyService(repo repository.RenewalPolicyRepository) *RenewalPolicyService {
return &RenewalPolicyService{repo: repo}
}
// rpSlugRegex matches non-alphanumeric characters that slugifyRenewalPolicyName strips.
// Mirrors the identical regex in internal/repository/postgres/renewal_policy.go —
// the service owns the rp-<slug> convention so the repo's retry loop is a
// pure PK-collision safety net, not the primary ID generator.
var rpSlugRegex = regexp.MustCompile(`[^a-z0-9-]+`)
// slugifyRenewalPolicyName produces `rp-<slug>` for an auto-generated policy
// ID. Slug: lowercase, spaces→hyphens, non-alphanumeric stripped, trimmed to
// 64 chars. Matches the seed convention (rp-default, rp-standard, rp-urgent)
// and the repo's slugifyPolicyName byte-for-byte.
func slugifyRenewalPolicyName(name string) string {
slug := strings.ToLower(strings.TrimSpace(name))
slug = strings.ReplaceAll(slug, " ", "-")
slug = rpSlugRegex.ReplaceAllString(slug, "")
slug = strings.Trim(slug, "-")
if slug == "" {
slug = "policy"
}
if len(slug) > 64 {
slug = slug[:64]
}
return "rp-" + slug
}
// ListRenewalPolicies returns a single page of renewal policies sorted by
// name (the repo's ORDER BY name is index-served via idx_renewal_policies_name).
// Pagination is done in Go rather than SQL — the expected row count is in the
// single digits so LIMIT/OFFSET would be premature optimization and would
// churn the repo contract for no measurable benefit (design doc §Known
// Caller Audit).
//
// Bounds: page defaults to 1, per_page defaults to 50, caps at 500 to match
// the /api/v1/policies handler's behavior. Past-end slices return an empty
// slice with no error — callers use `total` to detect end of pagination.
func (s *RenewalPolicyService) ListRenewalPolicies(ctx context.Context, page, perPage int) ([]domain.RenewalPolicy, int64, error) {
if page < 1 {
page = 1
}
if perPage < 1 {
perPage = 50
}
if perPage > 500 {
perPage = 500
}
items, err := s.repo.List(ctx)
if err != nil {
return nil, 0, fmt.Errorf("failed to list renewal policies: %w", err)
}
total := int64(len(items))
start := (page - 1) * perPage
if start >= int(total) {
return nil, total, nil
}
end := start + perPage
if end > int(total) {
end = int(total)
}
out := make([]domain.RenewalPolicy, 0, end-start)
for _, p := range items[start:end] {
if p != nil {
out = append(out, *p)
}
}
return out, total, nil
}
// GetRenewalPolicy retrieves one renewal policy by ID. Not-found errors
// surface from the repo verbatim; the handler translates them to 404.
func (s *RenewalPolicyService) GetRenewalPolicy(ctx context.Context, id string) (*domain.RenewalPolicy, error) {
return s.repo.Get(ctx, id)
}
// validateBounds enforces the design doc §Validation Bounds invariants:
// - name required, ≤ 255 chars
// - renewal_window_days in [1, 365]
// - max_retries in [0, 10]
// - retry_interval_seconds in [60, 86400]
// - alert_thresholds_days each in [0, 365]
//
// Called after applyCreateDefaults so zero-value fields that the caller
// expects to be defaulted don't trip the range checks.
func (s *RenewalPolicyService) validateBounds(rp *domain.RenewalPolicy) error {
if strings.TrimSpace(rp.Name) == "" {
return fmt.Errorf("name is required")
}
if len(rp.Name) > 255 {
return fmt.Errorf("name must be 255 characters or fewer, got %d", len(rp.Name))
}
if rp.RenewalWindowDays < 1 || rp.RenewalWindowDays > 365 {
return fmt.Errorf("renewal_window_days must be between 1 and 365, got %d", rp.RenewalWindowDays)
}
if rp.MaxRetries < 0 || rp.MaxRetries > 10 {
return fmt.Errorf("max_retries must be between 0 and 10, got %d", rp.MaxRetries)
}
if rp.RetryInterval < 60 || rp.RetryInterval > 86400 {
return fmt.Errorf("retry_interval_seconds must be between 60 and 86400, got %d", rp.RetryInterval)
}
for i, t := range rp.AlertThresholdsDays {
if t < 0 || t > 365 {
return fmt.Errorf("alert_thresholds_days[%d]=%d must be between 0 and 365", i, t)
}
}
return nil
}
// applyCreateDefaults fills in zero-valued optional fields with the design
// doc defaults. Name is never defaulted — missing name fails validation.
// MaxRetries=0 is a legal explicit value (no retries), so it is NOT
// defaulted; the DB default column handles that path if needed.
func (s *RenewalPolicyService) applyCreateDefaults(rp *domain.RenewalPolicy) {
if rp.RenewalWindowDays == 0 {
rp.RenewalWindowDays = 30
}
if rp.RetryInterval == 0 {
rp.RetryInterval = 3600
}
if len(rp.AlertThresholdsDays) == 0 {
rp.AlertThresholdsDays = domain.DefaultAlertThresholds()
}
}
// CreateRenewalPolicy inserts a new renewal policy. Auto-generates
// `rp-<slug(name)>` for ID if empty. Defaults are applied before bounds
// validation so a caller can omit RenewalWindowDays / RetryInterval and
// still pass bounds. Returns ErrRenewalPolicyDuplicateName unwrapped from
// the repo when a name collision occurs (pg 23505 on the UNIQUE constraint);
// the handler surfaces that as 409 Conflict.
func (s *RenewalPolicyService) CreateRenewalPolicy(ctx context.Context, rp domain.RenewalPolicy) (*domain.RenewalPolicy, error) {
s.applyCreateDefaults(&rp)
if err := s.validateBounds(&rp); err != nil {
return nil, err
}
if rp.ID == "" {
rp.ID = slugifyRenewalPolicyName(rp.Name)
}
if rp.CreatedAt.IsZero() {
rp.CreatedAt = time.Now()
}
if err := s.repo.Create(ctx, &rp); err != nil {
// Propagate repository sentinels verbatim — service-level sentinels
// alias repo sentinels (same var identity), so errors.Is walks
// through without any translation.
return nil, err
}
return &rp, nil
}
// UpdateRenewalPolicy replaces the fields of an existing renewal policy.
// Applies the same defaults+bounds as Create so partial updates do not slip
// an invalid row past validation via zero-value fields. id in the path wins
// over any id the caller supplied in the body.
func (s *RenewalPolicyService) UpdateRenewalPolicy(ctx context.Context, id string, rp domain.RenewalPolicy) (*domain.RenewalPolicy, error) {
s.applyCreateDefaults(&rp)
if err := s.validateBounds(&rp); err != nil {
return nil, err
}
rp.ID = id
if err := s.repo.Update(ctx, id, &rp); err != nil {
return nil, err
}
return &rp, nil
}
// DeleteRenewalPolicy removes a renewal policy. Returns ErrRenewalPolicyInUse
// when the policy is still referenced by rows in managed_certificates (the
// repo translates pg 23503 FK_RESTRICT violations onto that sentinel). The
// handler surfaces that as 409 Conflict.
func (s *RenewalPolicyService) DeleteRenewalPolicy(ctx context.Context, id string) error {
return s.repo.Delete(ctx, id)
}
+341
View File
@@ -0,0 +1,341 @@
package service
import (
"context"
"errors"
"strings"
"testing"
"time"
"github.com/shankar0123/certctl/internal/domain"
)
// G-1 red tests: lock in the behavior of RenewalPolicyService before
// the production code exists. Every subtest here references a type or
// method that Phase 2b must introduce:
//
// - NewRenewalPolicyService(repo) (constructor)
// - svc.ListRenewalPolicies(ctx, page, pp) ([]RenewalPolicy, int64, error)
// - svc.GetRenewalPolicy(ctx, id) (*RenewalPolicy, error)
// - svc.CreateRenewalPolicy(ctx, rp) (*RenewalPolicy, error)
// - svc.UpdateRenewalPolicy(ctx, id, rp) (*RenewalPolicy, error)
// - svc.DeleteRenewalPolicy(ctx, id) error
// - ErrRenewalPolicyDuplicateName sentinel (pg 23505 → 409)
// - ErrRenewalPolicyInUse sentinel (pg 23503 → 409)
//
// Once Phase 2b lands, these should all turn green without modification.
func TestRenewalPolicyService_List_Success(t *testing.T) {
ctx := context.Background()
now := time.Now()
repo := &mockRenewalPolicyRepo{
Policies: map[string]*domain.RenewalPolicy{
"rp-default": {
ID: "rp-default", Name: "Default", RenewalWindowDays: 30,
MaxRetries: 3, RetryInterval: 3600, AutoRenew: true,
CreatedAt: now, UpdatedAt: now,
},
"rp-urgent": {
ID: "rp-urgent", Name: "Urgent", RenewalWindowDays: 7,
MaxRetries: 5, RetryInterval: 600, AutoRenew: true,
CreatedAt: now, UpdatedAt: now,
},
},
}
svc := NewRenewalPolicyService(repo)
items, total, err := svc.ListRenewalPolicies(ctx, 1, 50)
if err != nil {
t.Fatalf("ListRenewalPolicies failed: %v", err)
}
if total != 2 {
t.Errorf("expected total 2, got %d", total)
}
if len(items) != 2 {
t.Errorf("expected 2 items, got %d", len(items))
}
}
func TestRenewalPolicyService_List_Empty(t *testing.T) {
ctx := context.Background()
repo := &mockRenewalPolicyRepo{Policies: map[string]*domain.RenewalPolicy{}}
svc := NewRenewalPolicyService(repo)
items, total, err := svc.ListRenewalPolicies(ctx, 1, 50)
if err != nil {
t.Fatalf("ListRenewalPolicies failed: %v", err)
}
if total != 0 {
t.Errorf("expected total 0, got %d", total)
}
if len(items) != 0 {
t.Errorf("expected 0 items, got %d", len(items))
}
}
func TestRenewalPolicyService_List_Pagination(t *testing.T) {
ctx := context.Background()
now := time.Now()
repo := &mockRenewalPolicyRepo{Policies: map[string]*domain.RenewalPolicy{}}
// Seed 5 policies, names A..E so the mock's sort.Slice yields a deterministic
// ordering that pagination boundaries can assert against.
for _, name := range []string{"A", "B", "C", "D", "E"} {
p := &domain.RenewalPolicy{
ID: "rp-" + strings.ToLower(name), Name: name,
RenewalWindowDays: 30, MaxRetries: 3, RetryInterval: 3600, AutoRenew: true,
CreatedAt: now, UpdatedAt: now,
}
repo.Policies[p.ID] = p
}
svc := NewRenewalPolicyService(repo)
// Page 1, size 2 → [A, B]
page1, total, err := svc.ListRenewalPolicies(ctx, 1, 2)
if err != nil {
t.Fatalf("page 1 failed: %v", err)
}
if total != 5 {
t.Errorf("expected total 5, got %d", total)
}
if len(page1) != 2 || page1[0].Name != "A" || page1[1].Name != "B" {
t.Errorf("unexpected page 1 slice: %+v", page1)
}
// Page 3, size 2 → [E] (single-item last page)
page3, _, err := svc.ListRenewalPolicies(ctx, 3, 2)
if err != nil {
t.Fatalf("page 3 failed: %v", err)
}
if len(page3) != 1 || page3[0].Name != "E" {
t.Errorf("unexpected page 3 slice: %+v", page3)
}
// Page 4, size 2 → [] (past the end, no error)
page4, _, err := svc.ListRenewalPolicies(ctx, 4, 2)
if err != nil {
t.Fatalf("page 4 failed: %v", err)
}
if len(page4) != 0 {
t.Errorf("expected empty past-end slice, got %+v", page4)
}
}
func TestRenewalPolicyService_List_RepoError(t *testing.T) {
ctx := context.Background()
repo := &mockRenewalPolicyRepo{
Policies: map[string]*domain.RenewalPolicy{},
ListErr: errors.New("boom"),
}
svc := NewRenewalPolicyService(repo)
_, _, err := svc.ListRenewalPolicies(ctx, 1, 50)
if err == nil {
t.Fatal("expected error, got nil")
}
}
func TestRenewalPolicyService_Get_Success(t *testing.T) {
ctx := context.Background()
now := time.Now()
rp := &domain.RenewalPolicy{
ID: "rp-default", Name: "Default", RenewalWindowDays: 30,
MaxRetries: 3, RetryInterval: 3600, AutoRenew: true,
CreatedAt: now, UpdatedAt: now,
}
repo := &mockRenewalPolicyRepo{Policies: map[string]*domain.RenewalPolicy{"rp-default": rp}}
svc := NewRenewalPolicyService(repo)
got, err := svc.GetRenewalPolicy(ctx, "rp-default")
if err != nil {
t.Fatalf("GetRenewalPolicy failed: %v", err)
}
if got.Name != "Default" {
t.Errorf("expected name Default, got %s", got.Name)
}
}
func TestRenewalPolicyService_Get_NotFound(t *testing.T) {
ctx := context.Background()
repo := &mockRenewalPolicyRepo{Policies: map[string]*domain.RenewalPolicy{}}
svc := NewRenewalPolicyService(repo)
_, err := svc.GetRenewalPolicy(ctx, "rp-missing")
if err == nil {
t.Fatal("expected error for missing policy, got nil")
}
}
func TestRenewalPolicyService_Create_Success(t *testing.T) {
ctx := context.Background()
repo := &mockRenewalPolicyRepo{Policies: map[string]*domain.RenewalPolicy{}}
svc := NewRenewalPolicyService(repo)
rp := domain.RenewalPolicy{
Name: "Weekly Renewal",
RenewalWindowDays: 7,
MaxRetries: 3,
RetryInterval: 3600,
AutoRenew: true,
}
created, err := svc.CreateRenewalPolicy(ctx, rp)
if err != nil {
t.Fatalf("CreateRenewalPolicy failed: %v", err)
}
if created.ID == "" {
t.Fatal("expected auto-generated ID, got empty")
}
// ID convention: rp-<slug(name)> matches seed rows rp-default/rp-standard/rp-urgent.
if !strings.HasPrefix(created.ID, "rp-") {
t.Errorf("expected ID prefix rp-, got %s", created.ID)
}
if created.CreatedAt.IsZero() {
t.Error("expected CreatedAt to be populated")
}
}
func TestRenewalPolicyService_Create_MissingName(t *testing.T) {
ctx := context.Background()
repo := &mockRenewalPolicyRepo{Policies: map[string]*domain.RenewalPolicy{}}
svc := NewRenewalPolicyService(repo)
_, err := svc.CreateRenewalPolicy(ctx, domain.RenewalPolicy{
RenewalWindowDays: 30, MaxRetries: 3, RetryInterval: 3600,
})
if err == nil {
t.Fatal("expected validation error for missing name, got nil")
}
}
func TestRenewalPolicyService_Create_BoundsViolation(t *testing.T) {
ctx := context.Background()
repo := &mockRenewalPolicyRepo{Policies: map[string]*domain.RenewalPolicy{}}
svc := NewRenewalPolicyService(repo)
// RenewalWindowDays out of range [1, 365]
_, err := svc.CreateRenewalPolicy(ctx, domain.RenewalPolicy{
Name: "Bad Window",
RenewalWindowDays: 999,
MaxRetries: 3,
RetryInterval: 3600,
})
if err == nil {
t.Fatal("expected bounds violation on RenewalWindowDays, got nil")
}
}
func TestRenewalPolicyService_Create_DuplicateName(t *testing.T) {
ctx := context.Background()
repo := &mockRenewalPolicyRepo{
Policies: map[string]*domain.RenewalPolicy{},
CreateErr: ErrRenewalPolicyDuplicateName,
}
svc := NewRenewalPolicyService(repo)
_, err := svc.CreateRenewalPolicy(ctx, domain.RenewalPolicy{
Name: "Duplicate",
RenewalWindowDays: 30,
MaxRetries: 3,
RetryInterval: 3600,
})
if err == nil {
t.Fatal("expected duplicate-name error, got nil")
}
if !errors.Is(err, ErrRenewalPolicyDuplicateName) {
t.Errorf("expected ErrRenewalPolicyDuplicateName, got %v", err)
}
}
func TestRenewalPolicyService_Update_Success(t *testing.T) {
ctx := context.Background()
now := time.Now()
rp := &domain.RenewalPolicy{
ID: "rp-default", Name: "Default", RenewalWindowDays: 30,
MaxRetries: 3, RetryInterval: 3600, AutoRenew: true,
CreatedAt: now, UpdatedAt: now,
}
repo := &mockRenewalPolicyRepo{Policies: map[string]*domain.RenewalPolicy{"rp-default": rp}}
svc := NewRenewalPolicyService(repo)
updated, err := svc.UpdateRenewalPolicy(ctx, "rp-default", domain.RenewalPolicy{
Name: "Default Renamed",
RenewalWindowDays: 45,
MaxRetries: 5,
RetryInterval: 1800,
AutoRenew: true,
})
if err != nil {
t.Fatalf("UpdateRenewalPolicy failed: %v", err)
}
if updated.Name != "Default Renamed" {
t.Errorf("expected updated name, got %s", updated.Name)
}
if updated.RenewalWindowDays != 45 {
t.Errorf("expected window 45, got %d", updated.RenewalWindowDays)
}
}
func TestRenewalPolicyService_Update_NotFound(t *testing.T) {
ctx := context.Background()
repo := &mockRenewalPolicyRepo{Policies: map[string]*domain.RenewalPolicy{}}
svc := NewRenewalPolicyService(repo)
_, err := svc.UpdateRenewalPolicy(ctx, "rp-missing", domain.RenewalPolicy{
Name: "X", RenewalWindowDays: 30, MaxRetries: 3, RetryInterval: 3600,
})
if err == nil {
t.Fatal("expected error for missing policy, got nil")
}
}
func TestRenewalPolicyService_Delete_Success(t *testing.T) {
ctx := context.Background()
now := time.Now()
rp := &domain.RenewalPolicy{
ID: "rp-default", Name: "Default", RenewalWindowDays: 30,
MaxRetries: 3, RetryInterval: 3600, AutoRenew: true,
CreatedAt: now, UpdatedAt: now,
}
repo := &mockRenewalPolicyRepo{Policies: map[string]*domain.RenewalPolicy{"rp-default": rp}}
svc := NewRenewalPolicyService(repo)
if err := svc.DeleteRenewalPolicy(ctx, "rp-default"); err != nil {
t.Fatalf("DeleteRenewalPolicy failed: %v", err)
}
if _, exists := repo.Policies["rp-default"]; exists {
t.Error("expected policy to be removed from repo")
}
}
func TestRenewalPolicyService_Delete_NotFound(t *testing.T) {
ctx := context.Background()
repo := &mockRenewalPolicyRepo{Policies: map[string]*domain.RenewalPolicy{}}
svc := NewRenewalPolicyService(repo)
err := svc.DeleteRenewalPolicy(ctx, "rp-missing")
if err == nil {
t.Fatal("expected error for missing policy, got nil")
}
}
func TestRenewalPolicyService_Delete_InUseConflict(t *testing.T) {
ctx := context.Background()
now := time.Now()
rp := &domain.RenewalPolicy{
ID: "rp-active", Name: "Active", RenewalWindowDays: 30,
MaxRetries: 3, RetryInterval: 3600, AutoRenew: true,
CreatedAt: now, UpdatedAt: now,
}
repo := &mockRenewalPolicyRepo{
Policies: map[string]*domain.RenewalPolicy{"rp-active": rp},
DeleteErr: ErrRenewalPolicyInUse,
}
svc := NewRenewalPolicyService(repo)
err := svc.DeleteRenewalPolicy(ctx, "rp-active")
if err == nil {
t.Fatal("expected in-use conflict error, got nil")
}
if !errors.Is(err, ErrRenewalPolicyInUse) {
t.Errorf("expected ErrRenewalPolicyInUse, got %v", err)
}
}
+14 -2
View File
@@ -84,10 +84,22 @@ func (s *SCEPService) PKCSReq(ctx context.Context, csrPEM string, challengePassw
// this branch also protects future call sites (tests, library reuse, a
// future REST-over-HTTPS wrapper) from silently accepting unauthenticated
// CSRs.
//
// M-1 (P2): both failure modes now wrap service.ErrUnauthenticated via %w so
// the handler's errToStatus choke point dispatches them to HTTP 401 via
// errors.Is instead of relying on a `strings.Contains(err.Error(), "challenge
// password")` substring branch at handler/scep.go:174. This is a deliberate
// semantic correction: the pre-M-1 substring branch returned 403 Forbidden,
// but SCEP challenge-password failure is an authentication failure (the
// caller has no valid credential at the application layer), not an
// authorization denial (the caller has a credential but is not permitted),
// and 401 Unauthorized is the correct RFC 7235 status. The errToStatus doc
// block explicitly cites this site as the canonical ErrUnauthenticated use
// case.
if s.challengePassword == "" {
s.logger.Warn("SCEP enrollment rejected: server has no challenge password configured",
"transaction_id", transactionID)
return nil, fmt.Errorf("SCEP challenge password not configured on server")
return nil, fmt.Errorf("%w: SCEP challenge password not configured on server", ErrUnauthenticated)
}
// Constant-time compare avoids leaking the configured secret through
// response-time variance. ConstantTimeCompare returns 1 only when both
@@ -96,7 +108,7 @@ func (s *SCEPService) PKCSReq(ctx context.Context, csrPEM string, challengePassw
if subtle.ConstantTimeCompare([]byte(challengePassword), []byte(s.challengePassword)) != 1 {
s.logger.Warn("SCEP enrollment rejected: invalid challenge password",
"transaction_id", transactionID)
return nil, fmt.Errorf("invalid challenge password")
return nil, fmt.Errorf("%w: invalid challenge password", ErrUnauthenticated)
}
return s.processEnrollment(ctx, csrPEM, transactionID, "scep_pkcsreq")
+19
View File
@@ -148,6 +148,13 @@ func TestSCEPService_PKCSReq_ChallengePassword_Invalid(t *testing.T) {
if err == nil {
t.Fatal("expected error for invalid challenge password")
}
// M-1 (P2): pin the sentinel wrap so the handler's errToStatus dispatch
// (errors.Is → 401 Unauthorized) stays contract-stable. The parallel
// substring check is retained because it also pins the user-facing reason
// that service.ErrUnauthenticated composes with via fmt.Errorf("%w: ...").
if !errors.Is(err, ErrUnauthenticated) {
t.Errorf("expected errors.Is(err, ErrUnauthenticated), got: %v", err)
}
if !strings.Contains(err.Error(), "challenge password") {
t.Errorf("expected 'challenge password' in error, got: %v", err)
}
@@ -171,6 +178,12 @@ func TestSCEPService_PKCSReq_ChallengePassword_EmptyServerConfigRejected(t *test
if err == nil {
t.Fatalf("expected rejection when server challenge password is empty (client=%q)", clientPassword)
}
// M-1 (P2): pin the sentinel wrap on the "no shared secret configured"
// branch so both H-2 failure modes route to the same 401 arm at the
// handler. Preserves the explanatory substring assertion alongside.
if !errors.Is(err, ErrUnauthenticated) {
t.Errorf("expected errors.Is(err, ErrUnauthenticated) for client=%q, got: %v", clientPassword, err)
}
if !strings.Contains(err.Error(), "not configured") {
t.Errorf("expected 'not configured' in error, got: %v", err)
}
@@ -193,6 +206,12 @@ func TestSCEPService_PKCSReq_ChallengePassword_ConstantTimeLengthIndependence(t
if err == nil {
t.Fatalf("expected rejection for bad password %q", bad)
}
// M-1 (P2): pin sentinel-wrap on every bad-password input so both the
// length-mismatch and content-mismatch ConstantTimeCompare paths map to
// the same 401 arm.
if !errors.Is(err, ErrUnauthenticated) {
t.Errorf("expected errors.Is(err, ErrUnauthenticated) for %q, got: %v", bad, err)
}
if !strings.Contains(err.Error(), "invalid challenge password") {
t.Errorf("expected 'invalid challenge password' for %q, got: %v", bad, err)
}
+21 -4
View File
@@ -3,7 +3,6 @@ package service
import (
"context"
"encoding/json"
"errors"
"fmt"
"log/slog"
"time"
@@ -18,7 +17,15 @@ import (
// 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")
//
// M-1 (P2): wraps [ErrValidation] via fmt.Errorf("%w", ...) so the single
// handler-layer errToStatus choke point resolves HTTP 400 via
// errors.Is(err, ErrValidation). "Referenced FK does not exist in POST body"
// is invalid input — not a missing-resource lookup — which is why this wraps
// ErrValidation (400) rather than ErrNotFound (404). Pre-M-1, target handlers
// classified this via strings.Contains(err.Error(), "not found"); a message
// reword would have silently promoted 400 to 500 with no observable signal.
var ErrAgentNotFound = fmt.Errorf("%w: referenced agent does not exist", ErrValidation)
// validTargetTypes is the set of allowed target types for validation.
var validTargetTypes = map[domain.TargetType]bool{
@@ -215,10 +222,20 @@ func (s *TargetService) Delete(ctx context.Context, id string, actor string) err
// TestConnection tests a target's connectivity by checking the assigned agent's heartbeat status.
// Target connectors run on agents, not on the server, so we can't instantiate a connector here.
// Instead, we verify the agent is online and reachable.
//
// M-1 (P2): the pre-M-1 wraps were `"target not found: %w"` and
// `"assigned agent not found: %w"` on every targetRepo.Get / agentRepo.Get error
// — which coupled to pre-M-1 handler strings.Contains substring classifiers on
// "not found" and gave false positives on transient DB failures. Now both
// repos wrap only the genuine sql.ErrNoRows path with repository.ErrNotFound,
// so errors.Is walks the chain correctly: truly-missing → 404, everything else
// → 500. The wrap text is changed to "failed to get target" / "failed to get
// agent" to match the semantic (agent-misconfigured stays 500 — "assigned
// agent" is a data-integrity issue, not a consumer 404).
func (s *TargetService) TestConnection(ctx context.Context, id string) error {
target, err := s.targetRepo.Get(ctx, id)
if err != nil {
return fmt.Errorf("target not found: %w", err)
return fmt.Errorf("failed to get target: %w", err)
}
if target.AgentID == "" {
@@ -229,7 +246,7 @@ func (s *TargetService) TestConnection(ctx context.Context, id string) error {
agent, err := s.agentRepo.Get(ctx, target.AgentID)
if err != nil {
s.updateTestStatus(ctx, target, "failed")
return fmt.Errorf("assigned agent not found: %w", err)
return fmt.Errorf("failed to get assigned agent: %w", err)
}
// I-004: AgentRepository.Get intentionally surfaces retired rows (the banner
+77 -4
View File
@@ -749,11 +749,21 @@ func (m *mockPolicyRepo) AddRule(rule *domain.PolicyRule) {
m.Rules[rule.ID] = rule
}
// mockRenewalPolicyRepo is a test implementation of RenewalPolicyRepository
// mockRenewalPolicyRepo is a test implementation of RenewalPolicyRepository.
//
// G-1: repo contract extended with Create/Update/Delete to support the
// /api/v1/renewal-policies CRUD endpoints. Per-method *Err fields let tests
// force specific repo failures (duplicate name → 23505, FK RESTRICT on Delete
// → 23503) without standing up a real Postgres connection. The sentinel
// errors `ErrRenewalPolicyDuplicateName` and `ErrRenewalPolicyInUse` are the
// typed envelopes the service / handler layers translate into 409 Conflict.
type mockRenewalPolicyRepo struct {
Policies map[string]*domain.RenewalPolicy
GetErr error
ListErr error
Policies map[string]*domain.RenewalPolicy
GetErr error
ListErr error
CreateErr error
UpdateErr error
DeleteErr error
}
func (m *mockRenewalPolicyRepo) Get(ctx context.Context, id string) (*domain.RenewalPolicy, error) {
@@ -775,9 +785,49 @@ func (m *mockRenewalPolicyRepo) List(ctx context.Context) ([]*domain.RenewalPoli
for _, p := range m.Policies {
policies = append(policies, p)
}
// Deterministic ordering mirrors the production repo's ORDER BY name,
// so pagination-boundary assertions don't become flaky under map
// iteration randomness.
sort.Slice(policies, func(i, j int) bool {
return policies[i].Name < policies[j].Name
})
return policies, nil
}
func (m *mockRenewalPolicyRepo) Create(ctx context.Context, policy *domain.RenewalPolicy) error {
if m.CreateErr != nil {
return m.CreateErr
}
if _, exists := m.Policies[policy.ID]; exists {
return m.CreateErr
}
m.Policies[policy.ID] = policy
return nil
}
func (m *mockRenewalPolicyRepo) Update(ctx context.Context, id string, policy *domain.RenewalPolicy) error {
if m.UpdateErr != nil {
return m.UpdateErr
}
if _, exists := m.Policies[id]; !exists {
return errNotFound
}
policy.ID = id
m.Policies[id] = policy
return nil
}
func (m *mockRenewalPolicyRepo) Delete(ctx context.Context, id string) error {
if m.DeleteErr != nil {
return m.DeleteErr
}
if _, exists := m.Policies[id]; !exists {
return errNotFound
}
delete(m.Policies, id)
return nil
}
func (m *mockRenewalPolicyRepo) AddPolicy(policy *domain.RenewalPolicy) {
m.Policies[policy.ID] = policy
}
@@ -1385,6 +1435,13 @@ type mockRevocationRepo struct {
Revocations []*domain.CertificateRevocation
CreateErr error
ListErr error
// F-001 regression instrumentation: track which list method was invoked
// so tests can assert that the CRL generation hot path uses the scoped
// ListByIssuer query (migration 000012 composite index) rather than
// ListAll followed by in-Go filtering.
ListAllCalls int
ListByIssuerCalls int
LastListIssuerID string
}
func (m *mockRevocationRepo) Create(ctx context.Context, revocation *domain.CertificateRevocation) error {
@@ -1405,12 +1462,28 @@ func (m *mockRevocationRepo) GetByIssuerAndSerial(ctx context.Context, issuerID,
}
func (m *mockRevocationRepo) ListAll(ctx context.Context) ([]*domain.CertificateRevocation, error) {
m.ListAllCalls++
if m.ListErr != nil {
return nil, m.ListErr
}
return m.Revocations, nil
}
func (m *mockRevocationRepo) ListByIssuer(ctx context.Context, issuerID string) ([]*domain.CertificateRevocation, error) {
m.ListByIssuerCalls++
m.LastListIssuerID = issuerID
if m.ListErr != nil {
return nil, m.ListErr
}
var result []*domain.CertificateRevocation
for _, r := range m.Revocations {
if r.IssuerID == issuerID {
result = append(result, r)
}
}
return result, nil
}
func (m *mockRevocationRepo) ListByCertificate(ctx context.Context, certID string) ([]*domain.CertificateRevocation, error) {
var result []*domain.CertificateRevocation
for _, r := range m.Revocations {
+58
View File
@@ -33,6 +33,10 @@ import {
updatePolicy,
deletePolicy,
getPolicyViolations,
getRenewalPolicies,
createRenewalPolicy,
updateRenewalPolicy,
deleteRenewalPolicy,
getIssuers,
createIssuer,
testIssuerConnection,
@@ -575,6 +579,60 @@ describe('API Client', () => {
});
});
// ─── Renewal Policies (G-1) ─────────────────────────
// Distinct from compliance Policies above. Populates the
// `renewal_policy_id` dropdown on OnboardingWizard + CertificatesPage +
// CertificateDetailPage.InlinePolicyEditor. Hits `/api/v1/renewal-policies`.
describe('RenewalPolicies', () => {
it('getRenewalPolicies sends GET', async () => {
mockFetch.mockReturnValueOnce(mockJsonResponse({ data: [], total: 0, page: 1, per_page: 50 }));
await getRenewalPolicies();
expect(mockFetch.mock.calls[0][0]).toContain('/api/v1/renewal-policies');
});
it('createRenewalPolicy sends POST with body', async () => {
mockFetch.mockReturnValueOnce(
mockJsonResponse({
id: 'rp-new',
name: 'New Policy',
renewal_window_days: 30,
max_retries: 3,
retry_interval_seconds: 3600,
auto_renew: true,
}),
);
await createRenewalPolicy({
name: 'New Policy',
renewal_window_days: 30,
max_retries: 3,
retry_interval_seconds: 3600,
auto_renew: true,
});
const [url, init] = mockFetch.mock.calls[0];
expect(url).toBe('/api/v1/renewal-policies');
expect(init.method).toBe('POST');
expect(JSON.parse(init.body).name).toBe('New Policy');
});
it('updateRenewalPolicy sends PUT with partial data', async () => {
mockFetch.mockReturnValueOnce(mockJsonResponse({ id: 'rp-default', name: 'Renamed' }));
await updateRenewalPolicy('rp-default', { name: 'Renamed' });
const [url, init] = mockFetch.mock.calls[0];
expect(url).toBe('/api/v1/renewal-policies/rp-default');
expect(init.method).toBe('PUT');
expect(JSON.parse(init.body)).toEqual({ name: 'Renamed' });
});
it('deleteRenewalPolicy sends DELETE', async () => {
mockFetch.mockReturnValueOnce(mockJsonResponse({ message: 'deleted' }));
await deleteRenewalPolicy('rp-default');
const [url, init] = mockFetch.mock.calls[0];
expect(url).toBe('/api/v1/renewal-policies/rp-default');
expect(init.method).toBe('DELETE');
});
});
// ─── Issuers ────────────────────────────────────────
describe('Issuers', () => {
+24 -1
View File
@@ -1,4 +1,4 @@
import type { Certificate, CertificateVersion, Agent, Job, Notification, AuditEvent, PolicyRule, PolicyViolation, Issuer, Target, CertificateProfile, Owner, Team, AgentGroup, PaginatedResponse, DashboardSummary, CertificateStatusCount, ExpirationBucket, JobTrendDataPoint, IssuanceRateDataPoint, MetricsResponse, DiscoveredCertificate, DiscoveryScan, DiscoverySummary, NetworkScanTarget, EndpointHealthCheck, HealthHistoryEntry, HealthCheckSummary, AgentDependencyCounts, RetireAgentResponse, BlockedByDependenciesResponse } from './types';
import type { Certificate, CertificateVersion, Agent, Job, Notification, AuditEvent, PolicyRule, PolicyViolation, RenewalPolicy, Issuer, Target, CertificateProfile, Owner, Team, AgentGroup, PaginatedResponse, DashboardSummary, CertificateStatusCount, ExpirationBucket, JobTrendDataPoint, IssuanceRateDataPoint, MetricsResponse, DiscoveredCertificate, DiscoveryScan, DiscoverySummary, NetworkScanTarget, EndpointHealthCheck, HealthHistoryEntry, HealthCheckSummary, AgentDependencyCounts, RetireAgentResponse, BlockedByDependenciesResponse } from './types';
const BASE = '/api/v1';
@@ -344,6 +344,29 @@ export const deletePolicy = (id: string) =>
export const getPolicyViolations = (id: string) =>
fetchJSON<PaginatedResponse<PolicyViolation>>(`${BASE}/policies/${id}/violations`);
// G-1: Renewal Policies (/api/v1/renewal-policies) — lifecycle policies with
// rp-* IDs in the renewal_policies table. Distinct from getPolicies() above
// which hits /api/v1/policies and returns PolicyRule (compliance, pol-* IDs).
// OnboardingWizard, CertificatesPage, and CertificateDetailPage populate the
// `renewal_policy_id` dropdown from this endpoint; populating it from
// getPolicies() produced FK violations on certificate insert/update.
export const getRenewalPolicies = (page = 1, perPage = 50) => {
const qs = new URLSearchParams({ page: String(page), per_page: String(perPage) }).toString();
return fetchJSON<PaginatedResponse<RenewalPolicy>>(`${BASE}/renewal-policies?${qs}`);
};
export const getRenewalPolicy = (id: string) =>
fetchJSON<RenewalPolicy>(`${BASE}/renewal-policies/${id}`);
export const createRenewalPolicy = (data: Partial<RenewalPolicy>) =>
fetchJSON<RenewalPolicy>(`${BASE}/renewal-policies`, { method: 'POST', body: JSON.stringify(data) });
export const updateRenewalPolicy = (id: string, data: Partial<RenewalPolicy>) =>
fetchJSON<RenewalPolicy>(`${BASE}/renewal-policies/${id}`, { method: 'PUT', body: JSON.stringify(data) });
export const deleteRenewalPolicy = (id: string) =>
fetchJSON<void>(`${BASE}/renewal-policies/${id}`, { method: 'DELETE' });
// Issuers
export const getIssuers = (params: Record<string, string> = {}) => {
const qs = new URLSearchParams({ page: '1', per_page: '50', ...params }).toString();
+25
View File
@@ -228,6 +228,31 @@ export interface PolicyViolation {
created_at: string;
}
/**
* G-1: RenewalPolicy is the lifecycle policy attached to managed certificates
* via `managed_certificates.renewal_policy_id` (FK ON DELETE RESTRICT `rp-*`
* IDs in the `renewal_policies` table). Distinct from `PolicyRule` above, which
* models compliance rules in the `policy_rules` table with `pol-*` IDs. The
* OnboardingWizard + CertificatesPage + CertificateDetailPage dropdowns populate
* `renewal_policy_id` from this interface previously they mis-populated it
* from `getPolicies()` which returned `pol-*` IDs and produced FK violations on
* certificate insert/update.
*
* JSON tags mirror internal/domain/renewal_policy.go.
*/
export interface RenewalPolicy {
id: string;
name: string;
renewal_window_days: number;
auto_renew: boolean;
max_retries: number;
retry_interval_seconds: number;
alert_thresholds_days: number[];
certificate_profile_id?: string | null;
created_at: string;
updated_at: string;
}
export interface Issuer {
id: string;
name: string;
+12 -4
View File
@@ -1,7 +1,7 @@
import { useState } from 'react';
import { useParams, useNavigate } from 'react-router-dom';
import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query';
import { getCertificate, getCertificateVersions, triggerRenewal, triggerDeployment, archiveCertificate, revokeCertificate, updateCertificate, getTargets, getJobs, getPolicies, getProfiles, getProfile, downloadCertificatePEM, exportCertificatePKCS12 } from '../api/client';
import { getCertificate, getCertificateVersions, triggerRenewal, triggerDeployment, archiveCertificate, revokeCertificate, updateCertificate, getTargets, getJobs, getRenewalPolicies, getProfiles, getProfile, downloadCertificatePEM, exportCertificatePKCS12 } from '../api/client';
import { REVOCATION_REASONS } from '../api/types';
import PageHeader from '../components/PageHeader';
import StatusBadge from '../components/StatusBadge';
@@ -164,9 +164,14 @@ function InlinePolicyEditor({ certId, currentPolicyId, currentProfileId }: { cer
const [policyId, setPolicyId] = useState(currentPolicyId);
const [profileId, setProfileId] = useState(currentProfileId);
// G-1: swap from getPolicies (compliance rules, pol-*) to getRenewalPolicies
// (lifecycle policies, rp-*). managed_certificates.renewal_policy_id FK
// points at renewal_policies(id); the previous getPolicies call populated
// the dropdown with pol-* IDs that would 400/23503 at the server. See also
// OnboardingWizard.tsx:603 and CertificatesPage.tsx:53 for the sibling fixes.
const { data: policies } = useQuery({
queryKey: ['policies'],
queryFn: () => getPolicies(),
queryKey: ['renewal-policies'],
queryFn: () => getRenewalPolicies(1, 500),
enabled: editing,
});
@@ -227,7 +232,10 @@ function InlinePolicyEditor({ certId, currentPolicyId, currentProfileId }: { cer
className="w-full bg-white border border-surface-border rounded px-3 py-2 text-sm text-ink">
<option value="">None</option>
{policies?.data?.map(p => (
<option key={p.id} value={p.id}>{p.name} ({p.type})</option>
// G-1: RenewalPolicy has no `type` field (that was PolicyRule).
// Show the human-readable name + renewal window so operators can
// pick the correct lifecycle policy at a glance.
<option key={p.id} value={p.id}>{p.name} ({p.renewal_window_days}d window)</option>
))}
</select>
</div>
+8 -2
View File
@@ -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, getTeams, getPolicies, getProfiles, getIssuers, bulkRevokeCertificates } from '../api/client';
import { getCertificates, createCertificate, triggerRenewal, revokeCertificate, updateCertificate, getOwners, getTeams, getRenewalPolicies, getProfiles, getIssuers, bulkRevokeCertificates } from '../api/client';
import { useAuth } from '../components/AuthProvider';
import { REVOCATION_REASONS } from '../api/types';
import PageHeader from '../components/PageHeader';
@@ -48,9 +48,15 @@ function CreateCertificateModal({ onClose, onSuccess }: { onClose: () => void; o
queryKey: ['teams', 'form'],
queryFn: () => getTeams({ per_page: '500' }),
});
// G-1: swap from getPolicies (compliance rules, pol-*) to getRenewalPolicies
// (lifecycle policies, rp-*). managed_certificates.renewal_policy_id FK
// points at renewal_policies(id), so the dropdown must pull from that table
// — the previous getPolicies call populated the dropdown with pol-* IDs that
// would 400/23503 at the server. See also OnboardingWizard.tsx:603 and
// CertificateDetailPage.tsx:169 for the sibling fixes.
const { data: policiesResp } = useQuery({
queryKey: ['renewal-policies', 'form'],
queryFn: () => getPolicies({ per_page: '500' }),
queryFn: () => getRenewalPolicies(1, 500),
});
const profiles = profilesResp?.data || [];
const issuers = issuersResp?.data || [];
+7 -2
View File
@@ -39,7 +39,10 @@ vi.mock('../api/client', () => ({
getProfiles: vi.fn(),
getOwners: vi.fn(),
getTeams: vi.fn(),
getPolicies: vi.fn(),
// G-1: wizard populates the renewal_policy_id dropdown from
// getRenewalPolicies (rp-* ids), not getPolicies (which returns compliance
// rules with pol-* ids and violates the FK).
getRenewalPolicies: vi.fn(),
createIssuer: vi.fn(),
testIssuerConnection: vi.fn(),
createCertificate: vi.fn(),
@@ -85,7 +88,9 @@ function stubAllQueriesEmpty() {
vi.mocked(client.getTeams).mockResolvedValue({
data: [], total: 0, page: 1, per_page: 500,
} as never);
vi.mocked(client.getPolicies).mockResolvedValue({
// G-1: wizard populates renewal_policy_id from getRenewalPolicies, not
// getPolicies. See comment on the mock factory above.
vi.mocked(client.getRenewalPolicies).mockResolvedValue({
data: [], total: 0, page: 1, per_page: 500,
} as never);
}
+6 -2
View File
@@ -2,7 +2,7 @@ import { useState } from 'react';
import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query';
import { useNavigate, Link } from 'react-router-dom';
import {
getIssuers, getAgents, getProfiles, getOwners, getTeams, getPolicies,
getIssuers, getAgents, getProfiles, getOwners, getTeams, getRenewalPolicies,
createIssuer, testIssuerConnection,
createCertificate, triggerRenewal,
createTeam, createOwner,
@@ -600,7 +600,11 @@ function CertificateStep({ onNext, onSkip, createdIssuerId }: {
const { data: agents } = useQuery({ queryKey: ['agents'], queryFn: () => getAgents() });
const { data: owners } = useQuery({ queryKey: ['owners'], queryFn: () => getOwners({ per_page: '500' }) });
const { data: teams } = useQuery({ queryKey: ['teams'], queryFn: () => getTeams({ per_page: '500' }) });
const { data: policies } = useQuery({ queryKey: ['renewal-policies'], queryFn: () => getPolicies({ per_page: '500' }) });
// G-1: bind renewal_policy_id dropdown to /api/v1/renewal-policies (rp-* IDs
// from the renewal_policies table). Previously populated from getPolicies()
// which returned compliance rules (pol-* IDs) and violated the FK
// managed_certificates.renewal_policy_id → renewal_policies(id) on submit.
const { data: policies } = useQuery({ queryKey: ['renewal-policies'], queryFn: () => getRenewalPolicies(1, 500) });
const hasAgents = (agents?.data?.length ?? 0) > 0;