mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-08 22:58:57 +00:00
f0865bb051
Two audit findings, both category cat-l, both rooted in
web/src/pages/CertificatesPage.tsx. Pre-L-1 the GUI looped per-cert
HTTP calls — 100 selected certs = 100 sequential round-trips × ~50–200
ms each = a 5–20-second wedge during which the operator stared at a
progress bar. Post-L-1 each workflow is a single POST.
cat-l-fa0c1ac07ab5 [P1, primary] — bulk renew loop
handleBulkRenewal: for/await triggerRenewal(id)
cat-l-8a1fb258a38a [P2] — bulk reassign loop
handleReassign: for/await updateCertificate(id, {owner_id})
The bulk-revoke endpoint (POST /api/v1/certificates/bulk-revoke +
BulkRevocationCriteria/Result) already existed as the canonical shape
in v2.0.x — L-1 ports that pattern to renew + reassign with per-action
twists.
Backend (Go)
- internal/domain/bulk_renewal.go: BulkRenewalCriteria mirrors
BulkRevocationCriteria (criteria + IDs modes); BulkRenewalResult
envelope adds EnqueuedJobs[] for per-cert {certificate_id, job_id};
shared BulkOperationError type for all bulk paths.
- internal/domain/bulk_reassignment.go: narrower shape — IDs-only,
owner_id required, team_id optional.
- internal/service/bulk_renewal.go::BulkRenewalService.BulkRenew:
resolves criteria → status filter (Archived/Revoked/Expired/
RenewalInProgress all silent-skip) → per-cert status flip + job
create. Keygen-mode-aware so jobs land in the same initial status
as single-cert TriggerRenewal. Single bulk audit event per call,
not N.
- internal/service/bulk_reassignment.go::BulkReassignmentService.
BulkReassign: validates owner_id upfront via the
ErrBulkReassignOwnerNotFound typed sentinel — non-existent owner
returns 400 before any cert is touched. Already-owned-by-target
is silent-skip. Single bulk audit event.
- internal/api/handler/{bulk_renewal,bulk_reassignment}.go: HTTP
shape mirrors bulk_revocation.go. NOT admin-gated (renew is non-
destructive; reassign is a common-case workflow). Sentinel-error
→ 400 mapping for OwnerNotFound.
- internal/api/router/router.go: three bulk-* routes registered as a
block before the {id} routes. HandlerRegistry gains BulkRenewal +
BulkReassignment fields.
- cmd/server/main.go: NewBulkRenewalService threads cfg.Keygen.Mode
so bulk-renew jobs land in same initial state as single-cert path.
Frontend
- web/src/api/client.ts: bulkRenewCertificates(criteria) +
bulkReassignCertificates(request) functions with full TS types.
- web/src/pages/CertificatesPage.tsx: handleBulkRenewal + handleReassign
rewritten from N-call loops to single calls. Result envelope drives
progress UI; first-error message surfaced when total_failed > 0.
Stale triggerRenewal + updateCertificate imports removed.
MCP
- internal/mcp/types.go: BulkRenewCertificatesInput +
BulkReassignCertificatesInput.
- internal/mcp/tools.go: certctl_bulk_renew_certificates +
certctl_bulk_reassign_certificates tools mirroring the existing
certctl_bulk_revoke_certificates pattern.
OpenAPI
- api/openapi.yaml: two new operations (bulkRenewCertificates,
bulkReassignCertificates) under Certificates tag. Four new schemas
(BulkRenewRequest, BulkRenewResult, BulkEnqueuedJob,
BulkReassignRequest, BulkReassignResult).
Tests
- Domain: BulkRenewalCriteria.IsEmpty + BulkReassignmentRequest.IsEmpty
IsEmpty contracts; JSON round-trip shape pinning.
- Service: 7 BulkRenew tests (happy/criteria-mode/skips-RenewalInProgress/
skips-revoked-archived/empty-criteria-error/partial-failure/
audit-event-emitted) + 8 BulkReassign tests (happy/skips-already-
owned/owner-required/empty-IDs/owner-not-found-sentinel/team-id-
optional/team-id-provided/partial-failure/audit-event-emitted).
- Handler: 5 BulkRenew handler tests (happy/empty-body-400/wrong-
method-405/actor-attribution/service-error-500) + 6 BulkReassign
handler tests (happy/empty-IDs-400/missing-owner-400/owner-not-
found-400-via-sentinel/wrong-method-405/generic-error-500).
CI guardrail
- .github/workflows/ci.yml: 'Forbidden client-side bulk-action loop
regression guard (L-1)'. Greps web/src/pages/CertificatesPage.tsx
for 'for(...) await triggerRenewal(...)' and 'for(...) await
updateCertificate(...)' patterns; comment lines exempt; test files
exempt. Verified locally (passes against post-fix tree, fires
against synthetic regression).
Counts (deltas)
- Routes: 119 → 121 (+2)
- OpenAPI operations: 123 → 125 (+2)
- MCP tools: 83 → 85 (+2)
Performance
- 100-cert bulk-renew: ~10s of sequential HTTP → ~100ms (99% latency
reduction on the canonical operator workflow).
- Audit event volume: 1 + N per operation → 1.
Out of scope (deferred follow-ups)
- cat-b-31ceb6aaa9f1: updateOwner/updateTeam/updateAgentGroup orphan
(different shape — wire existing PUT to GUI, not new bulk endpoint).
- cat-k-e85d1099b2d7: CertificatesPage no pagination UI.
- cat-i-b0924b6675f8: MCP missing claim/dismiss/acknowledge (L-1 added
2 new tools but does not close that finding).
Verification
- go build / vet / test -short / test -short -race all clean.
- web tsc --noEmit + vitest run all clean (296 tests passing).
- OpenAPI YAML parses (89 paths, 125 ops).
- L-1 CI guardrail passes against post-fix tree, fires against
synthetic regression.
No push.
222 lines
8.1 KiB
Go
222 lines
8.1 KiB
Go
package service
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"log/slog"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/shankar0123/certctl/internal/domain"
|
|
)
|
|
|
|
// newBulkRenewalTestService spins up a BulkRenewalService wired against
|
|
// the in-memory mocks used by every other service test in this package.
|
|
// keygenMode defaults to "agent" — production-like routing where renewal
|
|
// jobs start as AwaitingCSR.
|
|
func newBulkRenewalTestService() (*BulkRenewalService, *mockCertRepo, *mockJobRepo, *mockAuditRepo) {
|
|
certRepo := newMockCertificateRepository()
|
|
jobRepo := &mockJobRepo{Jobs: map[string]*domain.Job{}}
|
|
auditRepo := newMockAuditRepository()
|
|
auditService := NewAuditService(auditRepo)
|
|
svc := NewBulkRenewalService(certRepo, jobRepo, auditService, slog.Default(), "agent")
|
|
return svc, certRepo, jobRepo, auditRepo
|
|
}
|
|
|
|
// addRenewableCert seeds a cert that is eligible for renewal (Active
|
|
// status, future expiry).
|
|
func addRenewableCert(repo *mockCertRepo, id string) {
|
|
cert := &domain.ManagedCertificate{
|
|
ID: id,
|
|
CommonName: id + ".example.com",
|
|
Status: domain.CertificateStatusActive,
|
|
ExpiresAt: time.Now().AddDate(0, 1, 0),
|
|
IssuerID: "iss-test",
|
|
}
|
|
repo.AddCert(cert)
|
|
}
|
|
|
|
// TestBulkRenew_ByExplicitIDs — happy path. N IDs in, N jobs enqueued,
|
|
// EnqueuedJobs slice carries the {certificate_id, job_id} pairs.
|
|
func TestBulkRenew_ByExplicitIDs(t *testing.T) {
|
|
svc, certRepo, jobRepo, _ := newBulkRenewalTestService()
|
|
addRenewableCert(certRepo, "mc-1")
|
|
addRenewableCert(certRepo, "mc-2")
|
|
addRenewableCert(certRepo, "mc-3")
|
|
|
|
res, err := svc.BulkRenew(context.Background(),
|
|
domain.BulkRenewalCriteria{CertificateIDs: []string{"mc-1", "mc-2", "mc-3"}},
|
|
"alice")
|
|
if err != nil {
|
|
t.Fatalf("BulkRenew failed: %v", err)
|
|
}
|
|
if res.TotalMatched != 3 || res.TotalEnqueued != 3 || res.TotalSkipped != 0 || res.TotalFailed != 0 {
|
|
t.Errorf("counts = matched:%d enqueued:%d skipped:%d failed:%d, want 3/3/0/0",
|
|
res.TotalMatched, res.TotalEnqueued, res.TotalSkipped, res.TotalFailed)
|
|
}
|
|
if len(res.EnqueuedJobs) != 3 {
|
|
t.Fatalf("EnqueuedJobs len = %d, want 3", len(res.EnqueuedJobs))
|
|
}
|
|
if len(jobRepo.Jobs) != 3 {
|
|
t.Errorf("jobRepo got %d jobs, want 3 (one per renewable cert)", len(jobRepo.Jobs))
|
|
}
|
|
for _, j := range res.EnqueuedJobs {
|
|
if j.JobID == "" {
|
|
t.Errorf("EnqueuedJob missing job_id for cert %s", j.CertificateID)
|
|
}
|
|
}
|
|
}
|
|
|
|
// TestBulkRenew_ByOwnerCriteria — criteria-mode resolution. The
|
|
// criteria-routing path must call resolveCertificates with the filter
|
|
// branch (not the explicit-IDs branch). Mocking convention in this
|
|
// package: mockCertRepo.List ignores the filter and returns all certs,
|
|
// so the test seeds only certs that should match (mirrors
|
|
// TestBulkRevoke_ByOwner shape in bulk_revocation_test.go).
|
|
func TestBulkRenew_ByOwnerCriteria(t *testing.T) {
|
|
svc, certRepo, _, _ := newBulkRenewalTestService()
|
|
for _, id := range []string{"mc-a1", "mc-a2"} {
|
|
cert := &domain.ManagedCertificate{
|
|
ID: id, CommonName: id, Status: domain.CertificateStatusActive,
|
|
OwnerID: "o-alice", ExpiresAt: time.Now().AddDate(0, 1, 0),
|
|
}
|
|
certRepo.AddCert(cert)
|
|
}
|
|
|
|
res, err := svc.BulkRenew(context.Background(),
|
|
domain.BulkRenewalCriteria{OwnerID: "o-alice"}, "alice")
|
|
if err != nil {
|
|
t.Fatalf("BulkRenew failed: %v", err)
|
|
}
|
|
if res.TotalEnqueued != 2 {
|
|
t.Errorf("TotalEnqueued = %d, want 2 (alice's 2 certs)", res.TotalEnqueued)
|
|
}
|
|
}
|
|
|
|
// TestBulkRenew_SkipsRenewalInProgress — a cert already in the renewal
|
|
// flow must NOT get a second job. This is the no-double-enqueue
|
|
// contract: dispatch the bulk-renew button twice in quick succession
|
|
// and the second call cleanly skips.
|
|
func TestBulkRenew_SkipsRenewalInProgress(t *testing.T) {
|
|
svc, certRepo, jobRepo, _ := newBulkRenewalTestService()
|
|
cert := &domain.ManagedCertificate{
|
|
ID: "mc-rip", Status: domain.CertificateStatusRenewalInProgress,
|
|
ExpiresAt: time.Now().AddDate(0, 1, 0),
|
|
}
|
|
certRepo.AddCert(cert)
|
|
|
|
res, err := svc.BulkRenew(context.Background(),
|
|
domain.BulkRenewalCriteria{CertificateIDs: []string{"mc-rip"}}, "alice")
|
|
if err != nil {
|
|
t.Fatalf("BulkRenew failed: %v", err)
|
|
}
|
|
if res.TotalSkipped != 1 || res.TotalEnqueued != 0 {
|
|
t.Errorf("counts wrong: skipped=%d enqueued=%d, want 1/0",
|
|
res.TotalSkipped, res.TotalEnqueued)
|
|
}
|
|
if len(jobRepo.Jobs) != 0 {
|
|
t.Errorf("no job should be created for already-in-progress cert; got %d jobs", len(jobRepo.Jobs))
|
|
}
|
|
}
|
|
|
|
// TestBulkRenew_SkipsRevokedAndArchived — terminal states are silent
|
|
// no-ops, not errors. Operator selecting a mix of live and revoked certs
|
|
// shouldn't see "ERROR: revoked cert can't be renewed" 50 times.
|
|
func TestBulkRenew_SkipsRevokedAndArchived(t *testing.T) {
|
|
svc, certRepo, _, _ := newBulkRenewalTestService()
|
|
addRenewableCert(certRepo, "mc-live")
|
|
for _, st := range []domain.CertificateStatus{
|
|
domain.CertificateStatusRevoked,
|
|
domain.CertificateStatusArchived,
|
|
domain.CertificateStatusExpired,
|
|
} {
|
|
cert := &domain.ManagedCertificate{
|
|
ID: "mc-" + string(st), Status: st, ExpiresAt: time.Now().AddDate(0, 1, 0),
|
|
}
|
|
certRepo.AddCert(cert)
|
|
}
|
|
|
|
res, err := svc.BulkRenew(context.Background(),
|
|
domain.BulkRenewalCriteria{CertificateIDs: []string{
|
|
"mc-live", "mc-Revoked", "mc-Archived", "mc-Expired",
|
|
}}, "alice")
|
|
if err != nil {
|
|
t.Fatalf("BulkRenew failed: %v", err)
|
|
}
|
|
if res.TotalEnqueued != 1 || res.TotalSkipped != 3 {
|
|
t.Errorf("counts = enqueued:%d skipped:%d, want 1/3 (only mc-live qualifies)",
|
|
res.TotalEnqueued, res.TotalSkipped)
|
|
}
|
|
if len(res.Errors) != 0 {
|
|
t.Errorf("status-skip should NOT populate Errors; got %v", res.Errors)
|
|
}
|
|
}
|
|
|
|
// TestBulkRenew_EmptyCriteria_Error — defensive contract. Mirrors
|
|
// BulkRevocationCriteria.IsEmpty rejection so a stray empty POST
|
|
// doesn't try to renew the entire fleet.
|
|
func TestBulkRenew_EmptyCriteria_Error(t *testing.T) {
|
|
svc, _, _, _ := newBulkRenewalTestService()
|
|
_, err := svc.BulkRenew(context.Background(),
|
|
domain.BulkRenewalCriteria{}, "alice")
|
|
if err == nil {
|
|
t.Fatal("expected error for empty criteria, got nil")
|
|
}
|
|
}
|
|
|
|
// TestBulkRenew_PartialFailure — N=3, jobRepo.Create injected to fail
|
|
// on one of them. Response carries 2 enqueued + 1 error; no panic, no
|
|
// abort.
|
|
func TestBulkRenew_PartialFailure(t *testing.T) {
|
|
svc, certRepo, jobRepo, _ := newBulkRenewalTestService()
|
|
addRenewableCert(certRepo, "mc-1")
|
|
addRenewableCert(certRepo, "mc-2")
|
|
addRenewableCert(certRepo, "mc-3")
|
|
|
|
// Make Create fail uniformly. Two of the three certs will still
|
|
// have their status flipped (because Update happened first), so
|
|
// the failure manifests as "I tried to enqueue, the job-create
|
|
// failed". Per-cert error string surfaced.
|
|
jobRepo.CreateErr = errors.New("simulated DB outage")
|
|
|
|
res, err := svc.BulkRenew(context.Background(),
|
|
domain.BulkRenewalCriteria{CertificateIDs: []string{"mc-1", "mc-2", "mc-3"}},
|
|
"alice")
|
|
if err != nil {
|
|
t.Fatalf("BulkRenew should not propagate per-cert errors as a top-level error; got: %v", err)
|
|
}
|
|
if res.TotalFailed != 3 || res.TotalEnqueued != 0 {
|
|
t.Errorf("counts = failed:%d enqueued:%d, want 3/0", res.TotalFailed, res.TotalEnqueued)
|
|
}
|
|
if len(res.Errors) != 3 {
|
|
t.Errorf("Errors len = %d, want 3", len(res.Errors))
|
|
}
|
|
}
|
|
|
|
// TestBulkRenew_AuditEventEmitted — exactly ONE bulk audit event for
|
|
// the operation, NOT N. This is the audit-volume contract that makes
|
|
// bulk endpoints scalable.
|
|
func TestBulkRenew_AuditEventEmitted(t *testing.T) {
|
|
svc, certRepo, _, auditRepo := newBulkRenewalTestService()
|
|
addRenewableCert(certRepo, "mc-1")
|
|
addRenewableCert(certRepo, "mc-2")
|
|
addRenewableCert(certRepo, "mc-3")
|
|
|
|
_, err := svc.BulkRenew(context.Background(),
|
|
domain.BulkRenewalCriteria{CertificateIDs: []string{"mc-1", "mc-2", "mc-3"}},
|
|
"alice")
|
|
if err != nil {
|
|
t.Fatalf("BulkRenew failed: %v", err)
|
|
}
|
|
|
|
// audit_events count must be exactly 1 — the bulk-renewal envelope.
|
|
// Per-cert renewal events come from CertificateService.TriggerRenewal,
|
|
// which the bulk path bypasses for exactly this reason.
|
|
if len(auditRepo.Events) != 1 {
|
|
t.Errorf("audit events count = %d, want exactly 1 (one bulk event, NOT N per-cert events)", len(auditRepo.Events))
|
|
}
|
|
if len(auditRepo.Events) > 0 && auditRepo.Events[0].Action != "bulk_renewal_initiated" {
|
|
t.Errorf("audit action = %q, want 'bulk_renewal_initiated'", auditRepo.Events[0].Action)
|
|
}
|
|
}
|