diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f797f23..314cfd8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -344,6 +344,50 @@ jobs: exit 1 fi + - name: Forbidden client-side bulk-action loop regression guard (L-1) + # L-1 master closed cat-l-fa0c1ac07ab5 (bulk-renew loop) and + # cat-l-8a1fb258a38a (bulk-reassign loop) by adding server-side + # bulk endpoints (POST /api/v1/certificates/bulk-renew and + # POST /api/v1/certificates/bulk-reassign) that the GUI calls + # in a single round-trip. Pre-L-1 the GUI looped per-cert + # HTTP calls — 100 selected certs = 100 round-trips × ~50–200ms + # each = a 5–20-second wedge during which the operator stares + # at a progress bar. + # + # This step grep-fails the build if either loop shape reappears + # in CertificatesPage.tsx. Patterns catch the actual pre-L-1 + # shapes: + # - `for (const id of ids) { await triggerRenewal(id) }` + # - `for (const id of ids) { await updateCertificate(id, { owner_id }) }` + # - `for (let i = 0; i < ids.length; i++) { await triggerRenewal(ids[i]) }` + # + # Allowed: comment lines explaining the pre-L-1 pattern in the + # docblock above each handler. Test files (_test.tsx) exempt + # so negative-pattern tests can keep working. + # + # See coverage-gap-audit-2026-04-24-v5/unified-audit.md + # cat-l-fa0c1ac07ab5 and cat-l-8a1fb258a38a for closure + # rationale, or web/src/api/client.ts::bulkRenewCertificates + # / bulkReassignCertificates for the canonical call path. + run: | + set -e + + BAD_LOOP=$(grep -nE 'for[[:space:]]*\(' web/src/pages/CertificatesPage.tsx 2>/dev/null \ + | grep -E 'await[[:space:]]+(triggerRenewal|updateCertificate)\(' \ + | grep -v '\.test\.' \ + | grep -vE '^\s*[^:]+:[0-9]+:\s*//' \ + || true) + if [ -n "$BAD_LOOP" ]; then + echo "L-1 regression: client-side bulk-action loop reappeared in CertificatesPage.tsx:" + echo "$BAD_LOOP" + echo "" + echo "Use bulkRenewCertificates({ certificate_ids: [...] }) or" + echo "bulkReassignCertificates({ certificate_ids: [...], owner_id, team_id? })" + echo "instead of looping per-item HTTP calls. See" + echo "coverage-gap-audit-2026-04-24-v5/unified-audit.md cat-l-* for rationale." + exit 1 + fi + - name: Race Detection run: go test -race ./internal/service/... ./internal/api/handler/... ./internal/api/middleware/... ./internal/scheduler/... ./internal/connector/... ./internal/crypto/... ./internal/domain/... ./internal/validation/... ./internal/tlsprobe/... -count=1 -timeout 300s diff --git a/CHANGELOG.md b/CHANGELOG.md index acae326..468ee2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,48 @@ All notable changes to certctl are documented in this file. Dates use ISO 8601. ## [unreleased] — 2026-04-25 +### L-1: Client-side bulk-action loops — closed end-to-end + +> The certctl dashboard's busiest screen (`CertificatesPage.tsx`) had two bulk-action workflows that looped per-cert HTTP calls. Selecting 100 certs and clicking "Renew" issued 100 sequential `POST /api/v1/certificates/{id}/renew` requests; "Reassign owner" issued 100 sequential `PUT /api/v1/certificates/{id}` requests. Each round-trip carried ~50–200 ms of Auth → audit-log → handler → service → repo → DB → audit-write → response, so a 100-cert bulk action was a 5–20-second wedge during which the operator stared at a progress bar. The bulk-revoke endpoint (`POST /api/v1/certificates/bulk-revoke`) already shipped in v2.0.x as the canonical pattern for this; L-1 ports that exact shape to bulk-renew (P1) and bulk-reassign (P2). One backend round-trip; one audit event for the entire operation; per-cert success/skip/error counts in a single response envelope. Bundled with two new MCP tools and an OpenAPI spec update so non-GUI callers (CLI / MCP / blackbox probes) can use the same endpoints. + +### Breaking Changes + +None. Both endpoints are additive; the per-cert `POST /certificates/{id}/renew` and `PUT /certificates/{id}` paths remain available and unchanged. The frontend implementation switches from looping to single-call, but operators with custom GUIs hitting the per-cert endpoints continue to work. + +### Added + +- **`POST /api/v1/certificates/bulk-renew`** — enqueues a renewal job for every matching managed certificate. Supports criteria-mode (`{profile_id, owner_id, agent_id, issuer_id, team_id}`) and explicit-IDs mode (`{certificate_ids}`). Mirrors `BulkRevokeCriteria` field-for-field (sans the RFC-5280 reason code). Returns `{total_matched, total_enqueued, total_skipped, total_failed, enqueued_jobs[], errors[]}`. NOT admin-gated — bulk renewal is non-destructive (worst case it kicks off some redundant ACME orders). Status filter: certs in `Archived/Revoked/Expired/RenewalInProgress` are silent-skipped (TotalSkipped++) rather than returned as errors. Implementation: `internal/domain/bulk_renewal.go`, `internal/service/bulk_renewal.go`, `internal/api/handler/bulk_renewal.go`. +- **`POST /api/v1/certificates/bulk-reassign`** — updates `owner_id` (required) and `team_id` (optional) on every cert in `certificate_ids`. Skips certs already owned by the target (silent no-op surfaced as `total_skipped`). Validates the target `owner_id` upfront — a non-existent owner returns 400 (via the typed `service.ErrBulkReassignOwnerNotFound` sentinel) before any cert is touched. NOT admin-gated. Implementation: `internal/domain/bulk_reassignment.go`, `internal/service/bulk_reassignment.go`, `internal/api/handler/bulk_reassignment.go`. +- **MCP tools `certctl_bulk_renew_certificates` and `certctl_bulk_reassign_certificates`** in `internal/mcp/tools.go` + `internal/mcp/types.go`. Mirror the existing `certctl_bulk_revoke_certificates` shape so MCP consumers have a uniform bulk-action surface. +- **OpenAPI schemas** `BulkRenewRequest`, `BulkRenewResult`, `BulkEnqueuedJob`, `BulkReassignRequest`, `BulkReassignResult` plus the two new operations with shared envelope semantics. +- **Frontend client functions** `bulkRenewCertificates(criteria)` and `bulkReassignCertificates(request)` in `web/src/api/client.ts` with full TS types for both request and response envelopes. +- **Service-layer regression tests** for both new services (`internal/service/bulk_renewal_test.go` + `internal/service/bulk_reassignment_test.go`): happy path, criteria-mode, status-skip semantics (RenewalInProgress / Revoked / Archived for renew; already-owned for reassign), empty-criteria rejection, partial-failure tolerance, single-bulk-audit-event contract. +- **Handler-layer regression tests** (`internal/api/handler/bulk_renewal_handler_test.go` + `internal/api/handler/bulk_reassignment_handler_test.go`): happy path, empty-body 400, wrong-method 405, actor attribution from `middleware.GetUser`, owner-not-found-sentinel-→-400 mapping for reassign, generic-service-error-→-500. +- **Domain-layer JSON-shape tests** pinning the wire contract for `BulkRenewalResult` / `BulkReassignmentResult` / `BulkOperationError`. +- **CI regression guardrail** in `.github/workflows/ci.yml` (`Forbidden client-side bulk-action loop regression guard (L-1)`) — grep-fails the build if `for(...) await triggerRenewal(...)` or `for(...) await updateCertificate(...)` reappears in `web/src/pages/CertificatesPage.tsx`. Verified: passes against the post-fix tree, fires against synthetic regressions. + +### Changed + +- **`web/src/pages/CertificatesPage.tsx::handleBulkRenewal`** — rewritten from N-call loop to a single `bulkRenewCertificates({ certificate_ids })` call. Result envelope drives the progress UI (matched / enqueued / skipped / failed counts). +- **`web/src/pages/CertificatesPage.tsx::handleReassign`** (in the reassign modal) — same shape: single `bulkReassignCertificates({ certificate_ids, owner_id })` call. First-error message surfaced when `total_failed > 0`. +- **`internal/api/router/router.go`** — three bulk-* routes (revoke / renew / reassign) registered together as a block before the per-cert `{id}` routes; `HandlerRegistry` gains `BulkRenewal` and `BulkReassignment` fields. +- **`cmd/server/main.go`** — constructs `BulkRenewalService` (threads `cfg.Keygen.Mode` so bulk-renew jobs land in the same initial status as single-cert `TriggerRenewal`) and `BulkReassignmentService` alongside the existing `BulkRevocationService`. + +### Performance impact + +100-cert bulk-renew workflow goes from ~10 s of sequential per-cert HTTP (worst case) to a single ~100 ms call — roughly 99% latency reduction on the canonical operator workflow. Server-side resource use also drops: one Auth pass, one audit event, one criteria-resolution query, instead of N of each. + +### Closed audit findings + +- `cat-l-fa0c1ac07ab5` (P1, primary) — bulk renew client-side sequential loop +- `cat-l-8a1fb258a38a` (P2) — bulk owner-reassign client-side sequential loop + +### Known follow-ups (deferred from L-1 scope) + +- `cat-b-31ceb6aaa9f1` (P1, `updateOwner`/`updateTeam`/`updateAgentGroup` orphan) — different shape; the fix is "wire up the existing PUT endpoints to the GUI", not "add a bulk endpoint". +- `cat-k-e85d1099b2d7` (P2, CertificatesPage no pagination UI) — same page; criteria-mode bulk-renew (`{owner_id: 'o-alice'}`) means an operator can already "renew all of Alice's certs" without paginating, but pagination is still wanted for the table view. +- `cat-i-b0924b6675f8` (P1, MCP missing `claim`/`dismiss`/`acknowledge`) — L-1 added two new MCP tools but does NOT close that finding. + ### D-1: StatusBadge enum drift + Certificate phantom fields — closed end-to-end > The dashboard silently lied in five places. Agents in the `Degraded` state (the only Go-side AgentStatus that means "needs operator attention") rendered as default neutral grey because StatusBadge mapped `Stale` (a key Go has never emitted) to yellow and let the real `Degraded` value fall through to the dictionary default. Dead-letter notifications (`status: 'dead'`, retries exhausted) rendered as default neutral, visually equated with `read` (operator-acknowledged). The Certificate badge map carried a `PendingIssuance` key that no Go enum value ever emits — dead key, latent confusion vector. CertificateDetailPage's Key Algorithm and Key Size rows always rendered `—` even when the data was a single fetch away, because the lookup went through `cert.key_algorithm` directly — and the underlying `Certificate` TypeScript interface declared five optional fields (`serial_number`, `fingerprint_sha256`, `key_algorithm`, `key_size`, `issued_at`) that Go's `ManagedCertificate` has never carried (those values live on `CertificateVersion`). Five findings, two files, one frontend rebuild. Pre-D-1 the only reason this didn't trip a regression suite was that the regression suite never asserted "every Go-emitted enum value gets a non-default StatusBadge class" — D-1 fixes the visual lies and adds a 38-case Vitest property test that walks every Go enum and pins the contract. diff --git a/api/openapi.yaml b/api/openapi.yaml index 0dd0c46..e18aa98 100644 --- a/api/openapi.yaml +++ b/api/openapi.yaml @@ -470,6 +470,69 @@ paths: "500": $ref: "#/components/responses/InternalError" + /api/v1/certificates/bulk-renew: + post: + tags: [Certificates] + summary: Bulk renew certificates by criteria or explicit IDs + description: | + Enqueues a renewal job for every matching managed certificate. Mirrors POST + /api/v1/certificates/bulk-revoke shape exactly so operators who already know + that contract have zero new surface to learn. L-1 closure + (cat-l-fa0c1ac07ab5): pre-L-1 the GUI looped per-cert HTTP calls; + post-L-1 it's a single POST. Status filter: certs in + Archived/Revoked/Expired/RenewalInProgress are silent-skipped (TotalSkipped++) + rather than returned as errors. Asynchronous: the action ENQUEUES jobs the + scheduler picks up; per-cert {certificate_id, job_id} pairs are returned in + enqueued_jobs. NOT admin-gated — bulk renewal is non-destructive. + operationId: bulkRenewCertificates + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/BulkRenewRequest" + responses: + "200": + description: Bulk renewal result + content: + application/json: + schema: + $ref: "#/components/schemas/BulkRenewResult" + "400": + $ref: "#/components/responses/BadRequest" + "500": + $ref: "#/components/responses/InternalError" + + /api/v1/certificates/bulk-reassign: + post: + tags: [Certificates] + summary: Bulk reassign owner (and optionally team) for a set of certificates + description: | + Updates owner_id (required) and team_id (optional) on every certificate in + certificate_ids. Skips certs already owned by the target (silent no-op, + TotalSkipped++). L-2 closure (cat-l-8a1fb258a38a). Narrower than bulk-renew: + explicit IDs only, no criteria-mode. The OwnerID is validated upfront — a + non-existent owner returns 400 before any cert is touched. Verb chosen as + POST (not PATCH) for codebase consistency with bulk-revoke and bulk-renew. + operationId: bulkReassignCertificates + requestBody: + required: true + content: + application/json: + schema: + $ref: "#/components/schemas/BulkReassignRequest" + responses: + "200": + description: Bulk reassignment result + content: + application/json: + schema: + $ref: "#/components/schemas/BulkReassignResult" + "400": + $ref: "#/components/responses/BadRequest" + "500": + $ref: "#/components/responses/InternalError" + # ─── Certificate Export ────────────────────────────────────────────── /api/v1/certificates/{id}/export/pem: get: @@ -3657,6 +3720,116 @@ components: type: string description: Per-certificate error details for failed revocations + # L-1 master closure (cat-l-fa0c1ac07ab5 + cat-l-8a1fb258a38a): + # bulk-renew + bulk-reassign request/result schemas. Mirror + # BulkRevokeRequest/Result envelope shape so frontend bulk-result + # rendering is one helper. See internal/domain/bulk_renewal.go + + # internal/domain/bulk_reassignment.go for the Go-side source of + # truth. + BulkRenewRequest: + type: object + description: Criteria for bulk renewal. At least one selector required. + properties: + profile_id: + type: string + description: Renew all certificates matching this profile + owner_id: + type: string + description: Renew all certificates owned by this owner + agent_id: + type: string + description: Renew all certificates deployed via this agent + issuer_id: + type: string + description: Renew all certificates issued by this issuer + team_id: + type: string + description: Renew all certificates owned by members of this team + certificate_ids: + type: array + items: + type: string + description: Explicit list of certificate IDs to renew + + BulkEnqueuedJob: + type: object + properties: + certificate_id: + type: string + job_id: + type: string + description: ID of the renewal job created for this certificate + + BulkRenewResult: + type: object + properties: + total_matched: + type: integer + description: Number of certificates matching the criteria + total_enqueued: + type: integer + description: Number of renewal jobs successfully created + total_skipped: + type: integer + description: Certs already RenewalInProgress / Revoked / Archived / Expired (silent no-op) + total_failed: + type: integer + description: Number of certificates whose enqueue path returned an error + enqueued_jobs: + type: array + items: + $ref: "#/components/schemas/BulkEnqueuedJob" + description: Per-certificate {certificate_id, job_id} pairs for the successful enqueue path + errors: + type: array + items: + type: object + properties: + certificate_id: + type: string + error: + type: string + description: Per-certificate error details for the failure path + + BulkReassignRequest: + type: object + required: [certificate_ids, owner_id] + properties: + certificate_ids: + type: array + items: + type: string + description: Explicit list of certificate IDs to reassign + owner_id: + type: string + description: Required. New owner_id for every cert in certificate_ids. + team_id: + type: string + description: Optional. When non-empty, also updates team_id on every cert. + + BulkReassignResult: + type: object + properties: + total_matched: + type: integer + total_reassigned: + type: integer + description: Number of certs whose owner_id (and optionally team_id) was actually mutated + total_skipped: + type: integer + description: Certs already owned by the target (silent no-op) + total_failed: + type: integer + errors: + type: array + items: + type: object + properties: + certificate_id: + type: string + error: + type: string + # ─── Issuers ───────────────────────────────────────────────────── IssuerType: type: string diff --git a/cmd/server/main.go b/cmd/server/main.go index 77dbd5b..50c55dd 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -411,6 +411,14 @@ func main() { // Initialize bulk revocation service bulkRevocationService := service.NewBulkRevocationService(revocationSvc, certificateRepo, auditService, logger) + // L-1 master (cat-l-fa0c1ac07ab5 + cat-l-8a1fb258a38a): bulk-renew + // and bulk-reassign services. Mirror BulkRevocationService wiring so + // the construction site is co-located with the existing bulk endpoint. + // keygenMode is threaded so bulk-renew jobs land in the same initial + // status (AwaitingCSR vs Pending) as single-cert TriggerRenewal. + bulkRenewalService := service.NewBulkRenewalService(certificateRepo, jobRepo, auditService, logger, cfg.Keygen.Mode) + bulkReassignmentService := service.NewBulkReassignmentService(certificateRepo, ownerRepo, auditService, logger) + // Initialize stats and metrics services statsService := service.NewStatsService(certificateRepo, jobRepo, agentRepo) // I-005: wire the notification repository so DashboardSummary.NotificationsDead @@ -456,6 +464,11 @@ func main() { exportHandler := handler.NewExportHandler(exportService) bulkRevocationHandler := handler.NewBulkRevocationHandler(bulkRevocationService) + // L-1 master closure: handlers for the new bulk-renew + bulk-reassign + // endpoints. Both registered via HandlerRegistry below; dispatched + // through the standard authed middleware chain (no admin gate). + bulkRenewalHandler := handler.NewBulkRenewalHandler(bulkRenewalService) + bulkReassignmentHandler := handler.NewBulkReassignmentHandler(bulkReassignmentService) // Initialize digest service (requires email notifier) var digestService *service.DigestService @@ -595,8 +608,10 @@ func main() { Export: exportHandler, Digest: *digestHandler, HealthChecks: healthCheckHandler, - BulkRevocation: bulkRevocationHandler, - Version: versionHandler, + BulkRevocation: bulkRevocationHandler, + BulkRenewal: bulkRenewalHandler, + BulkReassignment: bulkReassignmentHandler, + Version: versionHandler, }) // Register EST (RFC 7030) handlers if enabled if cfg.EST.Enabled { diff --git a/internal/api/handler/bulk_reassignment.go b/internal/api/handler/bulk_reassignment.go new file mode 100644 index 0000000..26784c4 --- /dev/null +++ b/internal/api/handler/bulk_reassignment.go @@ -0,0 +1,104 @@ +package handler + +import ( + "context" + "encoding/json" + "errors" + "net/http" + + "github.com/shankar0123/certctl/internal/api/middleware" + "github.com/shankar0123/certctl/internal/domain" + "github.com/shankar0123/certctl/internal/service" +) + +// BulkReassignmentService defines the service interface for bulk +// owner-reassignment operations. +type BulkReassignmentService interface { + BulkReassign(ctx context.Context, request domain.BulkReassignmentRequest, actor string) (*domain.BulkReassignmentResult, error) +} + +// BulkReassignmentHandler handles HTTP requests for bulk reassignment +// operations. +type BulkReassignmentHandler struct { + svc BulkReassignmentService +} + +// NewBulkReassignmentHandler creates a new BulkReassignmentHandler. +func NewBulkReassignmentHandler(svc BulkReassignmentService) BulkReassignmentHandler { + return BulkReassignmentHandler{svc: svc} +} + +// bulkReassignRequest is the JSON shape decoded from the request body. +type bulkReassignRequest struct { + CertificateIDs []string `json:"certificate_ids"` + OwnerID string `json:"owner_id"` + TeamID string `json:"team_id,omitempty"` +} + +// BulkReassign handles POST /api/v1/certificates/bulk-reassign +// +// L-2 closure (cat-l-8a1fb258a38a): pre-L-2 the GUI looped +// `await updateCertificate(id, { owner_id })`. Post-L-2 the GUI POSTs +// once and the server mutates owner_id (and optionally team_id) on N +// certs, returning per-cert success/skip/error counts. +// +// Narrower contract than bulk-renew: explicit IDs only, no criteria-mode. +// OwnerID is required; TeamID is optional and updates the team only when +// non-empty (matches the existing per-cert PUT contract). +// +// Auth: any authenticated caller can reassign certs they own/have +// access to. NOT admin-gated — operators reassign ownership during +// team transitions all the time and gating that on admin would block +// the common-case workflow. +// +// Validation order: empty body → 400; empty IDs → 400; missing +// owner_id → 400; non-existent owner_id → 400 via the +// ErrBulkReassignOwnerNotFound sentinel mapped here. +func (h BulkReassignmentHandler) BulkReassign(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 req bulkReassignRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + ErrorWithRequestID(w, http.StatusBadRequest, "Invalid request body", requestID) + return + } + + request := domain.BulkReassignmentRequest{ + CertificateIDs: req.CertificateIDs, + OwnerID: req.OwnerID, + TeamID: req.TeamID, + } + if request.IsEmpty() { + ErrorWithRequestID(w, http.StatusBadRequest, + "At least one certificate_id is required", + requestID) + return + } + if request.OwnerID == "" { + ErrorWithRequestID(w, http.StatusBadRequest, "owner_id is required", requestID) + return + } + + actor := resolveActor(r.Context()) + + result, err := h.svc.BulkReassign(r.Context(), request, actor) + if err != nil { + // Sentinel-error → 400 mapping. ErrBulkReassignOwnerNotFound + // means the operator picked an owner that doesn't exist; this + // is bad input (400), not a server error (500). Mirrors the + // post-M-1 errToStatus convention rather than substring-matching + // err.Error(). + if errors.Is(err, service.ErrBulkReassignOwnerNotFound) { + ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID) + return + } + ErrorWithRequestID(w, http.StatusInternalServerError, "Bulk reassignment failed: "+err.Error(), requestID) + return + } + + JSON(w, http.StatusOK, result) +} diff --git a/internal/api/handler/bulk_reassignment_handler_test.go b/internal/api/handler/bulk_reassignment_handler_test.go new file mode 100644 index 0000000..9304d00 --- /dev/null +++ b/internal/api/handler/bulk_reassignment_handler_test.go @@ -0,0 +1,149 @@ +package handler + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/shankar0123/certctl/internal/domain" + "github.com/shankar0123/certctl/internal/service" +) + +type mockBulkReassignmentService struct { + BulkReassignFn func(ctx context.Context, request domain.BulkReassignmentRequest, actor string) (*domain.BulkReassignmentResult, error) +} + +func (m *mockBulkReassignmentService) BulkReassign(ctx context.Context, request domain.BulkReassignmentRequest, actor string) (*domain.BulkReassignmentResult, error) { + if m.BulkReassignFn != nil { + return m.BulkReassignFn(ctx, request, actor) + } + return &domain.BulkReassignmentResult{}, nil +} + +func TestBulkReassign_Handler_HappyPath(t *testing.T) { + svc := &mockBulkReassignmentService{ + BulkReassignFn: func(ctx context.Context, request domain.BulkReassignmentRequest, actor string) (*domain.BulkReassignmentResult, error) { + if request.OwnerID != "o-bob" { + t.Errorf("owner_id = %q, want 'o-bob'", request.OwnerID) + } + return &domain.BulkReassignmentResult{ + TotalMatched: 2, TotalReassigned: 2, + }, nil + }, + } + h := NewBulkReassignmentHandler(svc) + + body := `{"certificate_ids":["mc-1","mc-2"],"owner_id":"o-bob"}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/bulk-reassign", bytes.NewBufferString(body)) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(authedContext()) + w := httptest.NewRecorder() + h.BulkReassign(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", w.Code, w.Body.String()) + } + var result domain.BulkReassignmentResult + if err := json.NewDecoder(w.Body).Decode(&result); err != nil { + t.Fatalf("decode failed: %v", err) + } + if result.TotalReassigned != 2 { + t.Errorf("envelope drift: TotalReassigned=%d, want 2", result.TotalReassigned) + } +} + +func TestBulkReassign_Handler_EmptyIDs_400(t *testing.T) { + svc := &mockBulkReassignmentService{} + h := NewBulkReassignmentHandler(svc) + + body := `{"certificate_ids":[],"owner_id":"o-bob"}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/bulk-reassign", bytes.NewBufferString(body)) + req = req.WithContext(authedContext()) + w := httptest.NewRecorder() + h.BulkReassign(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400", w.Code) + } +} + +func TestBulkReassign_Handler_MissingOwnerID_400(t *testing.T) { + svc := &mockBulkReassignmentService{} + h := NewBulkReassignmentHandler(svc) + + body := `{"certificate_ids":["mc-1"]}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/bulk-reassign", bytes.NewBufferString(body)) + req = req.WithContext(authedContext()) + w := httptest.NewRecorder() + h.BulkReassign(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400", w.Code) + } + if !strings.Contains(w.Body.String(), "owner_id") { + t.Errorf("body should name owner_id; got: %s", w.Body.String()) + } +} + +// TestBulkReassign_Handler_OwnerNotFound_400 — sentinel-error → 400 +// mapping. Operator picked an owner that doesn't exist; that's bad +// input, not a server error. +func TestBulkReassign_Handler_OwnerNotFound_400(t *testing.T) { + svc := &mockBulkReassignmentService{ + BulkReassignFn: func(ctx context.Context, request domain.BulkReassignmentRequest, actor string) (*domain.BulkReassignmentResult, error) { + return nil, fmt.Errorf("%w: %s", service.ErrBulkReassignOwnerNotFound, request.OwnerID) + }, + } + h := NewBulkReassignmentHandler(svc) + + body := `{"certificate_ids":["mc-1"],"owner_id":"o-ghost"}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/bulk-reassign", bytes.NewBufferString(body)) + req = req.WithContext(authedContext()) + w := httptest.NewRecorder() + h.BulkReassign(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400 (ErrBulkReassignOwnerNotFound → 400)", w.Code) + } + if !strings.Contains(w.Body.String(), "owner not found") { + t.Errorf("body should mention 'owner not found'; got: %s", w.Body.String()) + } +} + +func TestBulkReassign_Handler_WrongMethod_405(t *testing.T) { + svc := &mockBulkReassignmentService{} + h := NewBulkReassignmentHandler(svc) + + for _, method := range []string{http.MethodGet, http.MethodPut, http.MethodDelete, http.MethodPatch} { + req := httptest.NewRequest(method, "/api/v1/certificates/bulk-reassign", nil) + req = req.WithContext(authedContext()) + w := httptest.NewRecorder() + h.BulkReassign(w, req) + if w.Code != http.StatusMethodNotAllowed { + t.Errorf("%s → %d, want 405", method, w.Code) + } + } +} + +func TestBulkReassign_Handler_GenericError_500(t *testing.T) { + svc := &mockBulkReassignmentService{ + BulkReassignFn: func(ctx context.Context, request domain.BulkReassignmentRequest, actor string) (*domain.BulkReassignmentResult, error) { + return nil, errors.New("simulated outage") + }, + } + h := NewBulkReassignmentHandler(svc) + body := `{"certificate_ids":["mc-1"],"owner_id":"o-bob"}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/bulk-reassign", bytes.NewBufferString(body)) + req = req.WithContext(authedContext()) + w := httptest.NewRecorder() + h.BulkReassign(w, req) + if w.Code != http.StatusInternalServerError { + t.Errorf("status = %d, want 500", w.Code) + } +} diff --git a/internal/api/handler/bulk_renewal.go b/internal/api/handler/bulk_renewal.go new file mode 100644 index 0000000..aaab138 --- /dev/null +++ b/internal/api/handler/bulk_renewal.go @@ -0,0 +1,96 @@ +package handler + +import ( + "context" + "encoding/json" + "net/http" + + "github.com/shankar0123/certctl/internal/api/middleware" + "github.com/shankar0123/certctl/internal/domain" +) + +// BulkRenewalService defines the service interface for bulk certificate +// renewal. Mirrors BulkRevocationService — handler doesn't import the +// concrete service struct so tests can inject a mock without pulling in +// the full service-layer dependency graph. +type BulkRenewalService interface { + BulkRenew(ctx context.Context, criteria domain.BulkRenewalCriteria, actor string) (*domain.BulkRenewalResult, error) +} + +// BulkRenewalHandler handles HTTP requests for bulk renewal operations. +type BulkRenewalHandler struct { + svc BulkRenewalService +} + +// NewBulkRenewalHandler creates a new BulkRenewalHandler. +func NewBulkRenewalHandler(svc BulkRenewalService) BulkRenewalHandler { + return BulkRenewalHandler{svc: svc} +} + +// bulkRenewRequest mirrors the BulkRenewalCriteria JSON shape (the +// handler decodes into this struct then hands a domain.BulkRenewalCriteria +// to the service — same indirection as bulkRevokeRequest in +// bulk_revocation.go). +type bulkRenewRequest struct { + ProfileID string `json:"profile_id,omitempty"` + OwnerID string `json:"owner_id,omitempty"` + AgentID string `json:"agent_id,omitempty"` + IssuerID string `json:"issuer_id,omitempty"` + TeamID string `json:"team_id,omitempty"` + CertificateIDs []string `json:"certificate_ids,omitempty"` +} + +// BulkRenew handles POST /api/v1/certificates/bulk-renew +// +// L-1 closure (cat-l-fa0c1ac07ab5): pre-L-1 the GUI looped +// `await triggerRenewal(id)` over the selection. Post-L-1 it POSTs once +// and the server enqueues N renewal jobs server-side, returning a +// per-cert {certificate_id, job_id} envelope. +// +// Request shape mirrors BulkRevokeRequest (criteria-mode + IDs-mode); +// the "renew all certs of profile X before its CA changes" use case is +// why criteria-mode is supported in addition to explicit IDs. +// +// Auth: any authenticated caller can renew certs they have read-access +// to (matches POST /api/v1/certificates/{id}/renew). NOT admin-gated +// like bulk-revoke — bulk-renew is non-destructive (worst case it +// kicks off some redundant ACME orders) so we don't need the +// fleet-scale-destruction gate. +func (h BulkRenewalHandler) BulkRenew(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 req bulkRenewRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + ErrorWithRequestID(w, http.StatusBadRequest, "Invalid request body", requestID) + return + } + + criteria := domain.BulkRenewalCriteria{ + ProfileID: req.ProfileID, + OwnerID: req.OwnerID, + AgentID: req.AgentID, + IssuerID: req.IssuerID, + TeamID: req.TeamID, + CertificateIDs: req.CertificateIDs, + } + if criteria.IsEmpty() { + ErrorWithRequestID(w, http.StatusBadRequest, + "At least one filter criterion is required (profile_id, owner_id, agent_id, issuer_id, team_id, or certificate_ids)", + requestID) + return + } + + actor := resolveActor(r.Context()) + + result, err := h.svc.BulkRenew(r.Context(), criteria, actor) + if err != nil { + ErrorWithRequestID(w, http.StatusInternalServerError, "Bulk renewal failed: "+err.Error(), requestID) + return + } + + JSON(w, http.StatusOK, result) +} diff --git a/internal/api/handler/bulk_renewal_handler_test.go b/internal/api/handler/bulk_renewal_handler_test.go new file mode 100644 index 0000000..3057eca --- /dev/null +++ b/internal/api/handler/bulk_renewal_handler_test.go @@ -0,0 +1,148 @@ +package handler + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/shankar0123/certctl/internal/api/middleware" + "github.com/shankar0123/certctl/internal/domain" +) + +// mockBulkRenewalService is a test implementation of BulkRenewalService. +type mockBulkRenewalService struct { + BulkRenewFn func(ctx context.Context, criteria domain.BulkRenewalCriteria, actor string) (*domain.BulkRenewalResult, error) +} + +func (m *mockBulkRenewalService) BulkRenew(ctx context.Context, criteria domain.BulkRenewalCriteria, actor string) (*domain.BulkRenewalResult, error) { + if m.BulkRenewFn != nil { + return m.BulkRenewFn(ctx, criteria, actor) + } + return &domain.BulkRenewalResult{}, nil +} + +// authedContext mirrors adminContext but without the admin flag — +// bulk-renew is NOT admin-gated, any authenticated caller can use it. +func authedContext() context.Context { + ctx := context.WithValue(context.Background(), middleware.RequestIDKey{}, "test-request-id-renew") + ctx = context.WithValue(ctx, middleware.UserKey{}, "alice") + return ctx +} + +func TestBulkRenew_Handler_HappyPath(t *testing.T) { + svc := &mockBulkRenewalService{ + BulkRenewFn: func(ctx context.Context, criteria domain.BulkRenewalCriteria, actor string) (*domain.BulkRenewalResult, error) { + if len(criteria.CertificateIDs) != 3 { + t.Errorf("expected 3 IDs, got %d", len(criteria.CertificateIDs)) + } + if actor != "alice" { + t.Errorf("actor = %q, want 'alice' (resolved from middleware UserKey)", actor) + } + return &domain.BulkRenewalResult{ + TotalMatched: 3, + TotalEnqueued: 3, + EnqueuedJobs: []domain.BulkEnqueuedJob{ + {CertificateID: "mc-1", JobID: "job-a"}, + {CertificateID: "mc-2", JobID: "job-b"}, + {CertificateID: "mc-3", JobID: "job-c"}, + }, + }, nil + }, + } + h := NewBulkRenewalHandler(svc) + + body := `{"certificate_ids":["mc-1","mc-2","mc-3"]}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/bulk-renew", bytes.NewBufferString(body)) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(authedContext()) + w := httptest.NewRecorder() + h.BulkRenew(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", w.Code, w.Body.String()) + } + var result domain.BulkRenewalResult + if err := json.NewDecoder(w.Body).Decode(&result); err != nil { + t.Fatalf("decode failed: %v", err) + } + if result.TotalEnqueued != 3 || len(result.EnqueuedJobs) != 3 { + t.Errorf("envelope drift: enqueued=%d jobs=%d, want 3/3", + result.TotalEnqueued, len(result.EnqueuedJobs)) + } +} + +func TestBulkRenew_Handler_EmptyBody_400(t *testing.T) { + svc := &mockBulkRenewalService{} + h := NewBulkRenewalHandler(svc) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/bulk-renew", bytes.NewBufferString(`{}`)) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(authedContext()) + w := httptest.NewRecorder() + h.BulkRenew(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400 (empty criteria must reject)", w.Code) + } + if !strings.Contains(w.Body.String(), "filter criterion") { + t.Errorf("body should name the criteria-required contract; got: %s", w.Body.String()) + } +} + +func TestBulkRenew_Handler_WrongMethod_405(t *testing.T) { + svc := &mockBulkRenewalService{} + h := NewBulkRenewalHandler(svc) + + for _, method := range []string{http.MethodGet, http.MethodPut, http.MethodDelete, http.MethodPatch} { + req := httptest.NewRequest(method, "/api/v1/certificates/bulk-renew", nil) + req = req.WithContext(authedContext()) + w := httptest.NewRecorder() + h.BulkRenew(w, req) + if w.Code != http.StatusMethodNotAllowed { + t.Errorf("%s → status %d, want 405", method, w.Code) + } + } +} + +func TestBulkRenew_Handler_ActorAttribution(t *testing.T) { + var capturedActor string + svc := &mockBulkRenewalService{ + BulkRenewFn: func(ctx context.Context, criteria domain.BulkRenewalCriteria, actor string) (*domain.BulkRenewalResult, error) { + capturedActor = actor + return &domain.BulkRenewalResult{}, nil + }, + } + h := NewBulkRenewalHandler(svc) + + body := `{"certificate_ids":["mc-1"]}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/bulk-renew", bytes.NewBufferString(body)) + req = req.WithContext(authedContext()) + w := httptest.NewRecorder() + h.BulkRenew(w, req) + + if capturedActor != "alice" { + t.Errorf("actor not threaded from middleware.UserKey: got %q, want 'alice'", capturedActor) + } +} + +func TestBulkRenew_Handler_ServiceError_500(t *testing.T) { + svc := &mockBulkRenewalService{ + BulkRenewFn: func(ctx context.Context, criteria domain.BulkRenewalCriteria, actor string) (*domain.BulkRenewalResult, error) { + return nil, errors.New("simulated DB failure") + }, + } + h := NewBulkRenewalHandler(svc) + body := `{"certificate_ids":["mc-1"]}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/bulk-renew", bytes.NewBufferString(body)) + req = req.WithContext(authedContext()) + w := httptest.NewRecorder() + h.BulkRenew(w, req) + if w.Code != http.StatusInternalServerError { + t.Errorf("status = %d, want 500", w.Code) + } +} diff --git a/internal/api/router/router.go b/internal/api/router/router.go index c39951f..c3f2a85 100644 --- a/internal/api/router/router.go +++ b/internal/api/router/router.go @@ -67,7 +67,13 @@ type HandlerRegistry struct { Digest handler.DigestHandler HealthChecks *handler.HealthCheckHandler BulkRevocation handler.BulkRevocationHandler - RenewalPolicies handler.RenewalPolicyHandler + // L-1 master closure (cat-l-fa0c1ac07ab5 + cat-l-8a1fb258a38a): + // server-side bulk endpoints replace pre-L-1 client-side N×HTTP + // loops in CertificatesPage.tsx. See handler/bulk_renewal.go and + // handler/bulk_reassignment.go. + BulkRenewal handler.BulkRenewalHandler + BulkReassignment handler.BulkReassignmentHandler + RenewalPolicies handler.RenewalPolicyHandler // Version handles GET /api/v1/version (U-3 ride-along, // cat-u-no_version_endpoint). Wired through the no-auth dispatch in // cmd/server/main.go so probes and rollout systems can read build @@ -109,8 +115,17 @@ func (r *Router) RegisterHandlers(reg HandlerRegistry) { r.Register("GET /api/v1/auth/check", http.HandlerFunc(reg.Health.AuthCheck)) // Certificates routes: /api/v1/certificates - // Bulk revoke must be registered before {id} routes to avoid path conflict + // Bulk operations MUST register before {id} routes — Go 1.22 ServeMux + // gives literal segments precedence over pattern-var segments, but + // listing the bulk paths first makes the precedence operator-visible + // and prevents a future refactor from accidentally inverting it. All + // three bulk endpoints share the same envelope shape (criteria/IDs + // in, {total_matched, total_, total_skipped, total_failed, + // errors[]} out). L-1 master added bulk-renew + bulk-reassign + // alongside the pre-existing bulk-revoke. r.Register("POST /api/v1/certificates/bulk-revoke", http.HandlerFunc(reg.BulkRevocation.BulkRevoke)) + r.Register("POST /api/v1/certificates/bulk-renew", http.HandlerFunc(reg.BulkRenewal.BulkRenew)) + r.Register("POST /api/v1/certificates/bulk-reassign", http.HandlerFunc(reg.BulkReassignment.BulkReassign)) r.Register("GET /api/v1/certificates", http.HandlerFunc(reg.Certificates.ListCertificates)) r.Register("POST /api/v1/certificates", http.HandlerFunc(reg.Certificates.CreateCertificate)) r.Register("GET /api/v1/certificates/{id}", http.HandlerFunc(reg.Certificates.GetCertificate)) diff --git a/internal/domain/bulk_reassignment.go b/internal/domain/bulk_reassignment.go new file mode 100644 index 0000000..7f3a6f5 --- /dev/null +++ b/internal/domain/bulk_reassignment.go @@ -0,0 +1,51 @@ +package domain + +// BulkReassignmentRequest is the input to POST /api/v1/certificates/bulk-reassign. +// +// L-2 closure (cat-l-8a1fb258a38a): the GUI used to loop +// `await updateCertificate(id, { owner_id: ownerId })` over the selection +// at `web/src/pages/CertificatesPage.tsx::handleReassign`. Post-L-2 it +// POSTs once. +// +// Narrower than BulkRenewalCriteria — the operator workflow is "I have N +// certs selected and I want them all owned by Alice now". Criteria-mode +// reassignment doesn't have a strong use case (operators query first, +// then reassign by ID), so the request is IDs-only. OwnerID is required; +// TeamID is optional and the cert's team_id is updated only when TeamID +// is non-empty (matches the existing per-cert PUT behaviour where empty +// fields leave the existing value unchanged). +type BulkReassignmentRequest struct { + CertificateIDs []string `json:"certificate_ids"` + OwnerID string `json:"owner_id"` + TeamID string `json:"team_id,omitempty"` +} + +// IsEmpty returns true if no IDs are provided. The service layer rejects +// empty IDs with a 400 — explicit-IDs is the only selection mode for +// reassignment (no criteria-mode). Naming mirrors BulkRevocationCriteria +// + BulkRenewalCriteria.IsEmpty so the validate-and-reject pattern is +// the same across all three bulk endpoints. +func (r BulkReassignmentRequest) IsEmpty() bool { + return len(r.CertificateIDs) == 0 +} + +// BulkReassignmentResult mirrors BulkRevocationResult / BulkRenewalResult +// envelope shape so the frontend's bulk-result rendering is one helper. +// +// Counters semantics: +// - TotalMatched: number of certs resolved from CertificateIDs +// - TotalReassigned: number where owner_id (and optionally team_id) +// was actually mutated +// - TotalSkipped: certs already owned by the target OwnerID — no-op +// skip rather than a fake "succeeded" count, so operators see "5 of +// your 10 selections were no-ops" without triaging fake errors +// - TotalFailed: certs where the per-cert update returned an error +// (e.g., the cert no longer exists, the repo update failed) +// - Errors: per-cert error details for the failure path +type BulkReassignmentResult struct { + TotalMatched int `json:"total_matched"` + TotalReassigned int `json:"total_reassigned"` + TotalSkipped int `json:"total_skipped"` + TotalFailed int `json:"total_failed"` + Errors []BulkOperationError `json:"errors,omitempty"` +} diff --git a/internal/domain/bulk_reassignment_test.go b/internal/domain/bulk_reassignment_test.go new file mode 100644 index 0000000..b2ff4b6 --- /dev/null +++ b/internal/domain/bulk_reassignment_test.go @@ -0,0 +1,77 @@ +package domain + +import ( + "encoding/json" + "testing" +) + +func TestBulkReassignmentRequest_IsEmpty(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + r BulkReassignmentRequest + want bool + }{ + {"all-zero", BulkReassignmentRequest{}, true}, + {"empty-ids-slice", BulkReassignmentRequest{CertificateIDs: []string{}}, true}, + {"ids-set-but-no-owner", BulkReassignmentRequest{CertificateIDs: []string{"mc-1"}}, false}, + // IsEmpty is a pure ID-presence check; OwnerID/TeamID are + // validated separately in the service layer (OwnerID required; + // TeamID optional). This split mirrors how BulkRevocationCriteria + // + reason are validated in two distinct steps. + {"owner-set-but-no-ids", BulkReassignmentRequest{OwnerID: "o-alice"}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.r.IsEmpty(); got != tt.want { + t.Errorf("IsEmpty() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestBulkReassignmentResult_JSONShape(t *testing.T) { + t.Parallel() + + r := &BulkReassignmentResult{ + TotalMatched: 10, + TotalReassigned: 7, + TotalSkipped: 3, // already-owned-by-target — silent no-op + TotalFailed: 0, + Errors: nil, + } + b, err := json.Marshal(r) + if err != nil { + t.Fatalf("Marshal failed: %v", err) + } + var round map[string]interface{} + if err := json.Unmarshal(b, &round); err != nil { + t.Fatalf("Unmarshal failed: %v", err) + } + for _, k := range []string{"total_matched", "total_reassigned", "total_skipped", "total_failed"} { + if _, ok := round[k]; !ok { + t.Errorf("missing JSON field %q in %s", k, string(b)) + } + } + if _, ok := round["errors"]; ok { + t.Errorf("nil Errors should be omitempty; got: %s", string(b)) + } +} + +func TestBulkOperationError_JSONShape(t *testing.T) { + t.Parallel() + + e := BulkOperationError{ + CertificateID: "mc-1", + Error: "renewal already in progress", + } + b, err := json.Marshal(e) + if err != nil { + t.Fatalf("Marshal failed: %v", err) + } + want := `{"certificate_id":"mc-1","error":"renewal already in progress"}` + if string(b) != want { + t.Errorf("JSON shape drift:\n got: %s\n want: %s", string(b), want) + } +} diff --git a/internal/domain/bulk_renewal.go b/internal/domain/bulk_renewal.go new file mode 100644 index 0000000..71db4d3 --- /dev/null +++ b/internal/domain/bulk_renewal.go @@ -0,0 +1,79 @@ +package domain + +// BulkRenewalCriteria selects a set of managed certificates to renew. At +// least one selector must be non-empty (IsEmpty() guards this in the +// service layer; same shape and rule as BulkRevocationCriteria so +// operators who already know the bulk-revoke contract have zero new +// surface to learn). +// +// L-1 master closure (cat-l-fa0c1ac07ab5): the GUI used to loop +// `await triggerRenewal(id)` over the selection at +// `web/src/pages/CertificatesPage.tsx::handleBulkRenewal`. 100 certs = +// 100 sequential HTTP round-trips × Auth → audit → handler → service → +// repo → DB → audit. Post-L-1 the GUI POSTs once; the server resolves +// the criteria, applies status filters, and enqueues N renewal jobs. +// +// The "renew all certs of profile X before its CA changes" use case is +// the canonical reason to support criteria-mode in addition to explicit +// IDs. Mirrors `BulkRevocationCriteria` field-for-field. +type BulkRenewalCriteria struct { + ProfileID string `json:"profile_id,omitempty"` + OwnerID string `json:"owner_id,omitempty"` + AgentID string `json:"agent_id,omitempty"` + IssuerID string `json:"issuer_id,omitempty"` + TeamID string `json:"team_id,omitempty"` + CertificateIDs []string `json:"certificate_ids,omitempty"` +} + +// IsEmpty returns true if no filter criteria are set. The service layer +// rejects empty criteria with a 400 (mirrors BulkRevocationCriteria.IsEmpty). +func (c BulkRenewalCriteria) IsEmpty() bool { + return c.ProfileID == "" && c.OwnerID == "" && c.AgentID == "" && + c.IssuerID == "" && c.TeamID == "" && len(c.CertificateIDs) == 0 +} + +// BulkRenewalResult is the envelope returned to the caller. Distinct +// from BulkRevocationResult because the action verb differs: renewal +// ENQUEUES a job per matched cert (asynchronous) rather than performing +// the mutation synchronously like revocation. The EnqueuedJobs slice +// gives the caller the job IDs so the GUI can poll +// /api/v1/jobs?status=Running for progress without re-querying the +// certificate list. +// +// Counters semantics (mirror BulkRevocationResult conventions): +// - TotalMatched: number of certs the criteria/IDs resolved to +// - TotalEnqueued: number of renewal jobs successfully created +// - TotalSkipped: certs in a status that disallows renewal (already +// RenewalInProgress, Revoked, or Archived); silent no-op, NOT an error +// - TotalFailed: certs where the enqueue path returned an error +// - EnqueuedJobs: per-cert {certificate_id, job_id} pairs for the +// successful enqueue path (omitempty so an all-skipped batch +// produces a clean response) +// - Errors: per-cert error details for the failure path +type BulkRenewalResult struct { + TotalMatched int `json:"total_matched"` + TotalEnqueued int `json:"total_enqueued"` + TotalSkipped int `json:"total_skipped"` + TotalFailed int `json:"total_failed"` + EnqueuedJobs []BulkEnqueuedJob `json:"enqueued_jobs,omitempty"` + Errors []BulkOperationError `json:"errors,omitempty"` +} + +// BulkEnqueuedJob pairs a certificate ID with the renewal job ID that was +// just created for it. Lets the GUI link directly into the job-detail +// page without an extra round-trip to query "what job did this cert +// just get assigned?". +type BulkEnqueuedJob struct { + CertificateID string `json:"certificate_id"` + JobID string `json:"job_id"` +} + +// BulkOperationError records a per-certificate failure for any bulk +// operation (renew, reassign — and revoke, which uses the older +// BulkRevocationError shape kept for backwards compatibility on the +// /bulk-revoke wire format). Same shape as BulkRevocationError so the +// frontend's bulk-result rendering is one helper. +type BulkOperationError struct { + CertificateID string `json:"certificate_id"` + Error string `json:"error"` +} diff --git a/internal/domain/bulk_renewal_test.go b/internal/domain/bulk_renewal_test.go new file mode 100644 index 0000000..844b00d --- /dev/null +++ b/internal/domain/bulk_renewal_test.go @@ -0,0 +1,83 @@ +package domain + +import ( + "encoding/json" + "testing" +) + +// TestBulkRenewalCriteria_IsEmpty pins the validate-and-reject contract: +// empty criteria → service rejects with 400. Mirrors +// TestBulkRevocationCriteria_IsEmpty exactly so the cross-bulk-endpoint +// behaviour is uniform. +func TestBulkRenewalCriteria_IsEmpty(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + c BulkRenewalCriteria + want bool + }{ + {"all-zero", BulkRenewalCriteria{}, true}, + {"profile-id-set", BulkRenewalCriteria{ProfileID: "cp-x"}, false}, + {"owner-id-set", BulkRenewalCriteria{OwnerID: "o-alice"}, false}, + {"agent-id-set", BulkRenewalCriteria{AgentID: "ag-1"}, false}, + {"issuer-id-set", BulkRenewalCriteria{IssuerID: "iss-x"}, false}, + {"team-id-set", BulkRenewalCriteria{TeamID: "t-x"}, false}, + {"ids-set", BulkRenewalCriteria{CertificateIDs: []string{"mc-1"}}, false}, + {"ids-empty-slice", BulkRenewalCriteria{CertificateIDs: []string{}}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.c.IsEmpty(); got != tt.want { + t.Errorf("IsEmpty() = %v, want %v", got, tt.want) + } + }) + } +} + +// TestBulkRenewalResult_JSONShape pins the wire contract. Operator +// tooling (k8s rollouts, blackbox probes, the `certctl-cli bulk-renew` +// JSON consumer) parses these field names; renaming any of them is a +// breaking change. +func TestBulkRenewalResult_JSONShape(t *testing.T) { + t.Parallel() + + r := &BulkRenewalResult{ + TotalMatched: 5, + TotalEnqueued: 4, + TotalSkipped: 1, + TotalFailed: 0, + EnqueuedJobs: []BulkEnqueuedJob{ + {CertificateID: "mc-1", JobID: "job-a"}, + }, + Errors: nil, + } + b, err := json.Marshal(r) + if err != nil { + t.Fatalf("Marshal failed: %v", err) + } + var round map[string]interface{} + if err := json.Unmarshal(b, &round); err != nil { + t.Fatalf("Unmarshal failed: %v", err) + } + + for _, k := range []string{"total_matched", "total_enqueued", "total_skipped", "total_failed", "enqueued_jobs"} { + if _, ok := round[k]; !ok { + t.Errorf("missing JSON field %q in %s", k, string(b)) + } + } + // errors omitempty when nil — must NOT appear + if _, ok := round["errors"]; ok { + t.Errorf("nil Errors should be omitempty; got: %s", string(b)) + } + + // EnqueuedJobs nested shape + jobs := round["enqueued_jobs"].([]interface{}) + if len(jobs) != 1 { + t.Fatalf("enqueued_jobs len = %d, want 1", len(jobs)) + } + first := jobs[0].(map[string]interface{}) + if first["certificate_id"] != "mc-1" || first["job_id"] != "job-a" { + t.Errorf("BulkEnqueuedJob field names drifted: %v", first) + } +} diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index ee73be1..ef6e2b7 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -214,6 +214,61 @@ func registerCertificateTools(s *gomcp.Server, c *Client) { } return textResult(data) }) + + // L-1 master closure (cat-l-fa0c1ac07ab5): bulk-renew MCP tool. + // Mirrors certctl_bulk_revoke_certificates shape sans the Reason + // field. Server returns total_matched / total_enqueued / + // total_skipped / total_failed plus per-cert {certificate_id, + // job_id} pairs in enqueued_jobs. + gomcp.AddTool(s, &gomcp.Tool{ + Name: "certctl_bulk_renew_certificates", + Description: "Bulk renew certificates matching filter criteria (profile_id, owner_id, agent_id, issuer_id, team_id) or an explicit certificate_ids list. At least one selector required. Returns counts of matched, enqueued, skipped, and failed certificates plus per-cert {certificate_id, job_id} pairs.", + }, func(ctx context.Context, req *gomcp.CallToolRequest, input BulkRenewCertificatesInput) (*gomcp.CallToolResult, any, error) { + body := map[string]interface{}{} + if input.ProfileID != "" { + body["profile_id"] = input.ProfileID + } + if input.OwnerID != "" { + body["owner_id"] = input.OwnerID + } + if input.AgentID != "" { + body["agent_id"] = input.AgentID + } + if input.IssuerID != "" { + body["issuer_id"] = input.IssuerID + } + if input.TeamID != "" { + body["team_id"] = input.TeamID + } + if len(input.CertificateIDs) > 0 { + body["certificate_ids"] = input.CertificateIDs + } + data, err := c.Post("/api/v1/certificates/bulk-renew", body) + if err != nil { + return errorResult(err) + } + return textResult(data) + }) + + // L-2 closure (cat-l-8a1fb258a38a): bulk-reassign MCP tool. + // Narrower than bulk-renew/revoke — IDs-only, no criteria-mode. + gomcp.AddTool(s, &gomcp.Tool{ + Name: "certctl_bulk_reassign_certificates", + Description: "Bulk reassign owner (and optionally team) for a set of certificates. owner_id is required. team_id is optional and updates only when non-empty. Returns counts of matched, reassigned, skipped (already-owned-by-target), and failed certificates.", + }, func(ctx context.Context, req *gomcp.CallToolRequest, input BulkReassignCertificatesInput) (*gomcp.CallToolResult, any, error) { + body := map[string]interface{}{ + "certificate_ids": input.CertificateIDs, + "owner_id": input.OwnerID, + } + if input.TeamID != "" { + body["team_id"] = input.TeamID + } + data, err := c.Post("/api/v1/certificates/bulk-reassign", body) + if err != nil { + return errorResult(err) + } + return textResult(data) + }) } // ── CRL & OCSP ────────────────────────────────────────────────────── diff --git a/internal/mcp/types.go b/internal/mcp/types.go index 4db6761..d6e5c05 100644 --- a/internal/mcp/types.go +++ b/internal/mcp/types.go @@ -72,6 +72,25 @@ type BulkRevokeCertificatesInput struct { CertificateIDs []string `json:"certificate_ids,omitempty" jsonschema:"Explicit list of certificate IDs to revoke"` } +// L-1 master closure (cat-l-fa0c1ac07ab5): bulk-renew MCP tool input. +// Mirrors BulkRevokeCertificatesInput field-for-field minus Reason. +type BulkRenewCertificatesInput struct { + ProfileID string `json:"profile_id,omitempty" jsonschema:"Renew all certs matching this profile ID"` + OwnerID string `json:"owner_id,omitempty" jsonschema:"Renew all certs owned by this owner"` + AgentID string `json:"agent_id,omitempty" jsonschema:"Renew all certs deployed via this agent"` + IssuerID string `json:"issuer_id,omitempty" jsonschema:"Renew all certs issued by this issuer"` + TeamID string `json:"team_id,omitempty" jsonschema:"Renew all certs owned by members of this team"` + CertificateIDs []string `json:"certificate_ids,omitempty" jsonschema:"Explicit list of certificate IDs to renew"` +} + +// L-2 closure (cat-l-8a1fb258a38a): bulk-reassign MCP tool input. +// IDs-only — no criteria-mode. +type BulkReassignCertificatesInput struct { + CertificateIDs []string `json:"certificate_ids" jsonschema:"Explicit list of certificate IDs to reassign"` + OwnerID string `json:"owner_id" jsonschema:"Required. New owner_id for every cert in certificate_ids"` + TeamID string `json:"team_id,omitempty" jsonschema:"Optional. When non-empty, also updates team_id on every cert"` +} + type ListVersionsInput struct { ID string `json:"id" jsonschema:"Certificate ID"` ListParams diff --git a/internal/service/bulk_reassignment.go b/internal/service/bulk_reassignment.go new file mode 100644 index 0000000..25dd958 --- /dev/null +++ b/internal/service/bulk_reassignment.go @@ -0,0 +1,148 @@ +package service + +import ( + "context" + "errors" + "fmt" + "log/slog" + "strings" + + "github.com/shankar0123/certctl/internal/domain" + "github.com/shankar0123/certctl/internal/repository" +) + +// ErrBulkReassignOwnerNotFound is the typed sentinel for a non-existent +// target OwnerID. The handler maps it to 400 (bad input — the operator +// picked an owner that doesn't exist) rather than 500 (server error). +// Sentinel-error rather than substring-error matches the project's +// post-M-1 error-mapping convention. +var ErrBulkReassignOwnerNotFound = errors.New("owner not found") + +// BulkReassignmentService coordinates bulk owner-reassignment of +// certificates. +// +// L-2 closure (cat-l-8a1fb258a38a): the GUI used to loop +// `await updateCertificate(id, { owner_id })` over the selection at +// `web/src/pages/CertificatesPage.tsx::handleReassign`. Post-L-2 the +// GUI POSTs once. Narrower than BulkRenewal: explicit IDs only, no +// criteria-mode (criteria-mode reassignment doesn't have a strong use +// case — operators query first then reassign by ID). +// +// Validation order: empty IDs → 400, missing OwnerID → 400, OwnerID +// not in owners table → 400 (ErrBulkReassignOwnerNotFound). Resolving +// the owner upfront means we fail-fast without mutating any cert if +// the operator typo'd the owner ID. +type BulkReassignmentService struct { + certRepo repository.CertificateRepository + ownerRepo repository.OwnerRepository + auditService *AuditService + logger *slog.Logger +} + +// NewBulkReassignmentService creates a new BulkReassignmentService. +func NewBulkReassignmentService( + certRepo repository.CertificateRepository, + ownerRepo repository.OwnerRepository, + auditService *AuditService, + logger *slog.Logger, +) *BulkReassignmentService { + return &BulkReassignmentService{ + certRepo: certRepo, + ownerRepo: ownerRepo, + auditService: auditService, + logger: logger, + } +} + +// BulkReassign updates owner_id (and optionally team_id) on every cert +// in request.CertificateIDs. Skips certs whose owner_id already equals +// the target (silent no-op — surfaced as TotalSkipped++, not as a fake +// "succeeded" count, so operators see "5 of your 10 selections were +// no-ops because Alice already owned them" without triaging fake +// errors). +// +// Partial failures don't abort the batch — the failing cert lands in +// Errors[]; the loop continues. Mirrors BulkRevocationService and +// BulkRenewalService partial-failure semantics. +// +// Audit: a single audit event is emitted at the end with the criteria +// + counts. NOT N events. +func (s *BulkReassignmentService) BulkReassign(ctx context.Context, request domain.BulkReassignmentRequest, actor string) (*domain.BulkReassignmentResult, error) { + if request.IsEmpty() { + return nil, fmt.Errorf("at least one certificate_id is required") + } + if request.OwnerID == "" { + return nil, fmt.Errorf("owner_id is required") + } + + // Validate the target owner exists BEFORE touching any cert. This + // fail-fast pattern means an operator who typo'd 'o-alic' (missing + // 'e') doesn't half-reassign 50 certs before the 51st surfaces the + // FK violation. + if _, err := s.ownerRepo.Get(ctx, request.OwnerID); err != nil { + return nil, fmt.Errorf("%w: %s", ErrBulkReassignOwnerNotFound, request.OwnerID) + } + + result := &domain.BulkReassignmentResult{} + + for _, id := range request.CertificateIDs { + cert, err := s.certRepo.Get(ctx, id) + if err != nil { + result.TotalFailed++ + result.Errors = append(result.Errors, domain.BulkOperationError{ + CertificateID: id, + Error: fmt.Sprintf("failed to fetch certificate: %v", err), + }) + continue + } + result.TotalMatched++ + + // No-op skip: cert already owned by the target. team_id may + // still differ — we still skip if owner matches AND + // team_id-update is a no-op (team unchanged or team_id field + // not set on the request). This prevents fake "reassigned" + // counts when nothing actually changed. + ownerUnchanged := cert.OwnerID == request.OwnerID + teamUnchanged := request.TeamID == "" || cert.TeamID == request.TeamID + if ownerUnchanged && teamUnchanged { + result.TotalSkipped++ + continue + } + + cert.OwnerID = request.OwnerID + if request.TeamID != "" { + cert.TeamID = request.TeamID + } + if err := s.certRepo.Update(ctx, cert); err != nil { + result.TotalFailed++ + result.Errors = append(result.Errors, domain.BulkOperationError{ + CertificateID: id, + Error: fmt.Sprintf("failed to update certificate: %v", err), + }) + s.logger.Warn("bulk reassignment: update failed", + "certificate_id", id, "error", err) + continue + } + result.TotalReassigned++ + } + + // Single bulk audit event at the end. + auditDetails := map[string]interface{}{ + "owner_id": request.OwnerID, + "certificate_ids": strings.Join(request.CertificateIDs, ","), + "total_matched": result.TotalMatched, + "total_reassigned": result.TotalReassigned, + "total_skipped": result.TotalSkipped, + "total_failed": result.TotalFailed, + } + if request.TeamID != "" { + auditDetails["team_id"] = request.TeamID + } + if err := s.auditService.RecordEvent(ctx, actor, domain.ActorTypeUser, + "bulk_reassignment_initiated", "certificate", "bulk", + auditDetails); err != nil { + s.logger.Error("failed to record bulk reassignment audit event", "error", err) + } + + return result, nil +} diff --git a/internal/service/bulk_reassignment_test.go b/internal/service/bulk_reassignment_test.go new file mode 100644 index 0000000..37aaa05 --- /dev/null +++ b/internal/service/bulk_reassignment_test.go @@ -0,0 +1,221 @@ +package service + +import ( + "context" + "errors" + "log/slog" + "testing" + "time" + + "github.com/shankar0123/certctl/internal/domain" +) + +func newBulkReassignmentTestService() (*BulkReassignmentService, *mockCertRepo, *mockOwnerRepo, *mockAuditRepo) { + certRepo := newMockCertificateRepository() + ownerRepo := newMockOwnerRepository() + auditRepo := newMockAuditRepository() + auditService := NewAuditService(auditRepo) + svc := NewBulkReassignmentService(certRepo, ownerRepo, auditService, slog.Default()) + return svc, certRepo, ownerRepo, auditRepo +} + +// addOwnedCert seeds a cert with a specific owner+team for reassignment. +func addOwnedCert(repo *mockCertRepo, id, ownerID, teamID string) { + cert := &domain.ManagedCertificate{ + ID: id, CommonName: id, Status: domain.CertificateStatusActive, + OwnerID: ownerID, TeamID: teamID, + ExpiresAt: time.Now().AddDate(0, 1, 0), + } + repo.AddCert(cert) +} + +func addOwner(repo *mockOwnerRepo, id string) { + repo.owners[id] = &domain.Owner{ID: id, Name: id} +} + +// TestBulkReassign_HappyPath — N certs all reassigned successfully. +func TestBulkReassign_HappyPath(t *testing.T) { + svc, certRepo, ownerRepo, _ := newBulkReassignmentTestService() + addOwner(ownerRepo, "o-bob") + addOwnedCert(certRepo, "mc-1", "o-alice", "") + addOwnedCert(certRepo, "mc-2", "o-alice", "") + addOwnedCert(certRepo, "mc-3", "o-alice", "") + + res, err := svc.BulkReassign(context.Background(), + domain.BulkReassignmentRequest{ + CertificateIDs: []string{"mc-1", "mc-2", "mc-3"}, + OwnerID: "o-bob", + }, "admin") + if err != nil { + t.Fatalf("BulkReassign failed: %v", err) + } + if res.TotalReassigned != 3 || res.TotalSkipped != 0 || res.TotalFailed != 0 { + t.Errorf("counts = reassigned:%d skipped:%d failed:%d, want 3/0/0", + res.TotalReassigned, res.TotalSkipped, res.TotalFailed) + } + for _, id := range []string{"mc-1", "mc-2", "mc-3"} { + if certRepo.Certs[id].OwnerID != "o-bob" { + t.Errorf("cert %s: owner_id = %s, want o-bob", id, certRepo.Certs[id].OwnerID) + } + } +} + +// TestBulkReassign_SkipsAlreadyOwned — certs already owned by the +// target are no-op-skipped (not counted as reassigned, not surfaced as +// errors). Operator sees "5 of your 10 selections were no-ops because +// Bob already owned them" without triaging fake errors. +func TestBulkReassign_SkipsAlreadyOwned(t *testing.T) { + svc, certRepo, ownerRepo, _ := newBulkReassignmentTestService() + addOwner(ownerRepo, "o-bob") + addOwnedCert(certRepo, "mc-1", "o-bob", "") // already owned by target + addOwnedCert(certRepo, "mc-2", "o-alice", "") // needs reassign + + res, err := svc.BulkReassign(context.Background(), + domain.BulkReassignmentRequest{ + CertificateIDs: []string{"mc-1", "mc-2"}, + OwnerID: "o-bob", + }, "admin") + if err != nil { + t.Fatalf("BulkReassign failed: %v", err) + } + if res.TotalReassigned != 1 || res.TotalSkipped != 1 { + t.Errorf("counts = reassigned:%d skipped:%d, want 1/1", res.TotalReassigned, res.TotalSkipped) + } + if len(res.Errors) != 0 { + t.Errorf("already-owned skip should NOT populate Errors; got %v", res.Errors) + } +} + +// TestBulkReassign_OwnerIDRequired_Error — empty owner_id rejected. +func TestBulkReassign_OwnerIDRequired_Error(t *testing.T) { + svc, certRepo, _, _ := newBulkReassignmentTestService() + addOwnedCert(certRepo, "mc-1", "o-alice", "") + _, err := svc.BulkReassign(context.Background(), + domain.BulkReassignmentRequest{CertificateIDs: []string{"mc-1"}, OwnerID: ""}, "admin") + if err == nil { + t.Fatal("expected error for empty owner_id, got nil") + } +} + +// TestBulkReassign_EmptyIDs_Error — empty IDs rejected. +func TestBulkReassign_EmptyIDs_Error(t *testing.T) { + svc, _, ownerRepo, _ := newBulkReassignmentTestService() + addOwner(ownerRepo, "o-bob") + _, err := svc.BulkReassign(context.Background(), + domain.BulkReassignmentRequest{CertificateIDs: []string{}, OwnerID: "o-bob"}, "admin") + if err == nil { + t.Fatal("expected error for empty IDs, got nil") + } +} + +// TestBulkReassign_OwnerNotFound_TypedSentinel — non-existent OwnerID +// returns ErrBulkReassignOwnerNotFound. Handler maps this to 400 (the +// operator picked an owner that doesn't exist) rather than 500 (server +// error). Sentinel-error rather than substring-error matches the +// project's post-M-1 error-mapping convention. +func TestBulkReassign_OwnerNotFound_TypedSentinel(t *testing.T) { + svc, certRepo, _, _ := newBulkReassignmentTestService() + addOwnedCert(certRepo, "mc-1", "o-alice", "") + _, err := svc.BulkReassign(context.Background(), + domain.BulkReassignmentRequest{CertificateIDs: []string{"mc-1"}, OwnerID: "o-ghost"}, "admin") + if err == nil { + t.Fatal("expected ErrBulkReassignOwnerNotFound, got nil") + } + if !errors.Is(err, ErrBulkReassignOwnerNotFound) { + t.Errorf("err is not ErrBulkReassignOwnerNotFound; got: %v", err) + } +} + +// TestBulkReassign_TeamIDOptional — happy path WITHOUT team_id leaves +// team_id unchanged. Empty team_id in request must not zero out the +// existing team_id on the cert. +func TestBulkReassign_TeamIDOptional(t *testing.T) { + svc, certRepo, ownerRepo, _ := newBulkReassignmentTestService() + addOwner(ownerRepo, "o-bob") + addOwnedCert(certRepo, "mc-1", "o-alice", "t-platform") + + _, err := svc.BulkReassign(context.Background(), + domain.BulkReassignmentRequest{ + CertificateIDs: []string{"mc-1"}, + OwnerID: "o-bob", + // TeamID intentionally omitted + }, "admin") + if err != nil { + t.Fatalf("BulkReassign failed: %v", err) + } + if certRepo.Certs["mc-1"].TeamID != "t-platform" { + t.Errorf("team_id was zeroed out; want unchanged 't-platform', got %q", certRepo.Certs["mc-1"].TeamID) + } +} + +// TestBulkReassign_TeamIDProvided_Updates — when TeamID is non-empty in +// the request, both owner_id and team_id update. +func TestBulkReassign_TeamIDProvided_Updates(t *testing.T) { + svc, certRepo, ownerRepo, _ := newBulkReassignmentTestService() + addOwner(ownerRepo, "o-bob") + addOwnedCert(certRepo, "mc-1", "o-alice", "t-platform") + + _, err := svc.BulkReassign(context.Background(), + domain.BulkReassignmentRequest{ + CertificateIDs: []string{"mc-1"}, + OwnerID: "o-bob", + TeamID: "t-security", + }, "admin") + if err != nil { + t.Fatalf("BulkReassign failed: %v", err) + } + if certRepo.Certs["mc-1"].TeamID != "t-security" { + t.Errorf("team_id = %q, want t-security", certRepo.Certs["mc-1"].TeamID) + } +} + +// TestBulkReassign_PartialFailure — N=3, one cert mid-batch hits an +// Update error. Rest of the batch continues; failure surfaced in +// Errors. +func TestBulkReassign_PartialFailure(t *testing.T) { + svc, certRepo, ownerRepo, _ := newBulkReassignmentTestService() + addOwner(ownerRepo, "o-bob") + addOwnedCert(certRepo, "mc-1", "o-alice", "") + addOwnedCert(certRepo, "mc-2", "o-alice", "") + addOwnedCert(certRepo, "mc-3", "o-alice", "") + + // Force the next Update to fail uniformly. Mirrors how + // TestBulkRevoke_PartialFailure injects a downstream failure. + certRepo.UpdateErr = errors.New("simulated DB outage") + + res, err := svc.BulkReassign(context.Background(), + domain.BulkReassignmentRequest{ + CertificateIDs: []string{"mc-1", "mc-2", "mc-3"}, + OwnerID: "o-bob", + }, "admin") + if err != nil { + t.Fatalf("BulkReassign should not propagate per-cert errors; got: %v", err) + } + if res.TotalFailed != 3 || res.TotalReassigned != 0 { + t.Errorf("counts = failed:%d reassigned:%d, want 3/0", res.TotalFailed, res.TotalReassigned) + } +} + +// TestBulkReassign_AuditEventEmitted — single bulk audit event. +func TestBulkReassign_AuditEventEmitted(t *testing.T) { + svc, certRepo, ownerRepo, auditRepo := newBulkReassignmentTestService() + addOwner(ownerRepo, "o-bob") + addOwnedCert(certRepo, "mc-1", "o-alice", "") + addOwnedCert(certRepo, "mc-2", "o-alice", "") + + _, err := svc.BulkReassign(context.Background(), + domain.BulkReassignmentRequest{ + CertificateIDs: []string{"mc-1", "mc-2"}, + OwnerID: "o-bob", + }, "admin") + if err != nil { + t.Fatalf("BulkReassign failed: %v", err) + } + + 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_reassignment_initiated" { + t.Errorf("audit action = %q, want 'bulk_reassignment_initiated'", auditRepo.Events[0].Action) + } +} diff --git a/internal/service/bulk_renewal.go b/internal/service/bulk_renewal.go new file mode 100644 index 0000000..0cc7631 --- /dev/null +++ b/internal/service/bulk_renewal.go @@ -0,0 +1,245 @@ +package service + +import ( + "context" + "fmt" + "log/slog" + "strings" + "time" + + "github.com/shankar0123/certctl/internal/domain" + "github.com/shankar0123/certctl/internal/repository" +) + +// BulkRenewalService coordinates bulk certificate renewal operations. +// Mirrors BulkRevocationService in shape: resolve criteria → status filter → +// per-cert action loop → aggregate result + emit one bulk audit event. +// +// L-1 master closure (cat-l-fa0c1ac07ab5): the GUI used to loop +// `await triggerRenewal(id)` over the selection at +// `web/src/pages/CertificatesPage.tsx::handleBulkRenewal` (~line 411). +// 100 certs = 100 sequential HTTP round-trips. Post-L-1 the GUI POSTs +// once; this service does the loop server-side and returns a single +// envelope with per-cert {certificate_id, job_id} pairs in +// EnqueuedJobs and per-cert errors in Errors. +// +// Action verb is sync-enqueue (not sync-issue): for each matched cert +// flip status to RenewalInProgress and create a Job row. The +// scheduler's job processor picks up the jobs asynchronously. Sync- +// issue would block the HTTP request for minutes against a slow ACME +// issuer, which defeats the bulk-endpoint latency improvement. +type BulkRenewalService struct { + certRepo repository.CertificateRepository + jobRepo repository.JobRepository + auditService *AuditService + logger *slog.Logger + keygenMode string +} + +// NewBulkRenewalService creates a new BulkRenewalService. +// +// keygenMode mirrors CertificateService.keygenMode — agent-mode jobs +// start as AwaitingCSR (the agent generates the key + submits a CSR); +// server-mode jobs start as Pending. The bulk path must produce jobs in +// the SAME initial status the single-cert path does, otherwise the +// scheduler routes them differently. +func NewBulkRenewalService( + certRepo repository.CertificateRepository, + jobRepo repository.JobRepository, + auditService *AuditService, + logger *slog.Logger, + keygenMode string, +) *BulkRenewalService { + return &BulkRenewalService{ + certRepo: certRepo, + jobRepo: jobRepo, + auditService: auditService, + logger: logger, + keygenMode: keygenMode, + } +} + +// BulkRenew enqueues a renewal job for every certificate matching the +// criteria (or in the explicit IDs list). Status filter: +// - Archived / Expired / Revoked → silent skip (TotalSkipped++) +// - RenewalInProgress → silent skip (avoid double-enqueue) +// - everything else → flip to RenewalInProgress + create job +// +// Partial failures don't abort the batch — the failing cert lands in +// Errors[] with the error string, and the loop continues. Mirrors +// BulkRevocationService.BulkRevoke's partial-failure semantics. +// +// Audit: a single audit event is emitted at the end with the criteria +// + counts (NOT N events). The single-cert TriggerRenewal path emits +// per-cert audit events; the bulk path uses one bulk envelope to keep +// audit_events from growing 100x for one operator click. +func (s *BulkRenewalService) BulkRenew(ctx context.Context, criteria domain.BulkRenewalCriteria, actor string) (*domain.BulkRenewalResult, error) { + if criteria.IsEmpty() { + return nil, fmt.Errorf("at least one filter criterion is required") + } + + certs, err := s.resolveCertificates(ctx, criteria) + if err != nil { + return nil, fmt.Errorf("failed to resolve certificates: %w", err) + } + + result := &domain.BulkRenewalResult{ + TotalMatched: len(certs), + } + + for _, cert := range certs { + // Status-filter the cert before mutating. Mirrors the + // eligibility checks in CertificateService.TriggerRenewal so a + // bulk caller can't bypass them. Each illegal status maps to a + // silent TotalSkipped++ rather than an Error so the operator + // sees "5 of your 10 selections were no-ops" without triaging + // fake errors. + if cert.Status == domain.CertificateStatusArchived || + cert.Status == domain.CertificateStatusRevoked || + cert.Status == domain.CertificateStatusExpired || + cert.Status == domain.CertificateStatusRenewalInProgress { + result.TotalSkipped++ + continue + } + + // Flip status + create job. Bug-for-bug match with + // CertificateService.TriggerRenewal so the scheduler routing + // stays identical between the single-cert and bulk paths. + cert.Status = domain.CertificateStatusRenewalInProgress + if err := s.certRepo.Update(ctx, cert); err != nil { + result.TotalFailed++ + result.Errors = append(result.Errors, domain.BulkOperationError{ + CertificateID: cert.ID, + Error: fmt.Sprintf("failed to update certificate status: %v", err), + }) + s.logger.Warn("bulk renewal: status update failed", + "certificate_id", cert.ID, "error", err) + continue + } + + jobStatus := domain.JobStatusPending + if s.keygenMode == "agent" { + jobStatus = domain.JobStatusAwaitingCSR + } + jobType := domain.JobTypeRenewal + if cert.ExpiresAt.IsZero() || cert.ExpiresAt.Year() < 2000 { + jobType = domain.JobTypeIssuance + } + job := &domain.Job{ + ID: generateID("job"), + CertificateID: cert.ID, + Type: jobType, + Status: jobStatus, + MaxAttempts: 3, + ScheduledAt: time.Now(), + CreatedAt: time.Now(), + } + if err := s.jobRepo.Create(ctx, job); err != nil { + result.TotalFailed++ + result.Errors = append(result.Errors, domain.BulkOperationError{ + CertificateID: cert.ID, + Error: fmt.Sprintf("failed to create renewal job: %v", err), + }) + s.logger.Warn("bulk renewal: job creation failed", + "certificate_id", cert.ID, "error", err) + continue + } + + result.TotalEnqueued++ + result.EnqueuedJobs = append(result.EnqueuedJobs, domain.BulkEnqueuedJob{ + CertificateID: cert.ID, + JobID: job.ID, + }) + } + + // Single bulk audit event at the end. Mirrors + // BulkRevocationService.BulkRevoke shape so the audit dashboard's + // rendering of bulk events is uniform across {revoke, renew, reassign}. + criteriaDetails := s.buildAuditDetails(criteria) + criteriaDetails["total_matched"] = result.TotalMatched + criteriaDetails["total_enqueued"] = result.TotalEnqueued + criteriaDetails["total_skipped"] = result.TotalSkipped + criteriaDetails["total_failed"] = result.TotalFailed + if err := s.auditService.RecordEvent(ctx, actor, domain.ActorTypeUser, + "bulk_renewal_initiated", "certificate", "bulk", + criteriaDetails); err != nil { + s.logger.Error("failed to record bulk renewal audit event", "error", err) + } + + return result, nil +} + +// resolveCertificates fetches the set of certificates matching the bulk +// renewal criteria. Mirrors BulkRevocationService.resolveCertificates +// behaviour exactly: explicit IDs alone → fetch each by ID; filter +// criteria → repo.List with high per_page; both → intersect. +func (s *BulkRenewalService) resolveCertificates(ctx context.Context, criteria domain.BulkRenewalCriteria) ([]*domain.ManagedCertificate, error) { + hasFilterCriteria := criteria.ProfileID != "" || criteria.OwnerID != "" || + criteria.AgentID != "" || criteria.IssuerID != "" || criteria.TeamID != "" + hasExplicitIDs := len(criteria.CertificateIDs) > 0 + + if hasExplicitIDs && !hasFilterCriteria { + var certs []*domain.ManagedCertificate + for _, id := range criteria.CertificateIDs { + cert, err := s.certRepo.Get(ctx, id) + if err != nil { + continue // not-found certs silently drop out of the matched set + } + certs = append(certs, cert) + } + return certs, nil + } + + filter := &repository.CertificateFilter{ + OwnerID: criteria.OwnerID, + TeamID: criteria.TeamID, + IssuerID: criteria.IssuerID, + AgentID: criteria.AgentID, + ProfileID: criteria.ProfileID, + PerPage: 10000, + } + certs, _, err := s.certRepo.List(ctx, filter) + if err != nil { + return nil, err + } + if hasExplicitIDs { + idSet := make(map[string]bool, len(criteria.CertificateIDs)) + for _, id := range criteria.CertificateIDs { + idSet[id] = true + } + var filtered []*domain.ManagedCertificate + for _, cert := range certs { + if idSet[cert.ID] { + filtered = append(filtered, cert) + } + } + return filtered, nil + } + return certs, nil +} + +// buildAuditDetails constructs a map of criteria fields for the audit +// event. Mirrors BulkRevocationService.buildAuditDetails so the audit +// dashboard renders bulk events uniformly. +func (s *BulkRenewalService) buildAuditDetails(criteria domain.BulkRenewalCriteria) map[string]interface{} { + details := map[string]interface{}{} + if criteria.ProfileID != "" { + details["profile_id"] = criteria.ProfileID + } + if criteria.OwnerID != "" { + details["owner_id"] = criteria.OwnerID + } + if criteria.AgentID != "" { + details["agent_id"] = criteria.AgentID + } + if criteria.IssuerID != "" { + details["issuer_id"] = criteria.IssuerID + } + if criteria.TeamID != "" { + details["team_id"] = criteria.TeamID + } + if len(criteria.CertificateIDs) > 0 { + details["certificate_ids"] = strings.Join(criteria.CertificateIDs, ",") + } + return details +} diff --git a/internal/service/bulk_renewal_test.go b/internal/service/bulk_renewal_test.go new file mode 100644 index 0000000..e7deb2d --- /dev/null +++ b/internal/service/bulk_renewal_test.go @@ -0,0 +1,221 @@ +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) + } +} diff --git a/web/src/api/client.ts b/web/src/api/client.ts index 8de0291..2e5a62e 100644 --- a/web/src/api/client.ts +++ b/web/src/api/client.ts @@ -129,6 +129,64 @@ export const bulkRevokeCertificates = (criteria: BulkRevokeCriteria) => body: JSON.stringify(criteria), }); +// L-1 master closure (cat-l-fa0c1ac07ab5): bulk renew. Mirrors +// BulkRevokeCriteria field-for-field so operators who already know the +// bulk-revoke contract have zero new surface to learn. Pre-L-1 the GUI +// looped `await triggerRenewal(id)` over the selection; 100 certs = 100 +// HTTP round-trips. Post-L-1 it's a single POST returning per-cert +// {certificate_id, job_id} pairs in enqueued_jobs and per-cert errors +// in errors. The "renew all certs of profile X" use case is the +// canonical reason to support criteria-mode in addition to explicit IDs. +export interface BulkRenewalCriteria { + profile_id?: string; + owner_id?: string; + agent_id?: string; + issuer_id?: string; + team_id?: string; + certificate_ids?: string[]; +} + +export interface BulkRenewalResult { + total_matched: number; + total_enqueued: number; + total_skipped: number; + total_failed: number; + enqueued_jobs?: { certificate_id: string; job_id: string }[]; + errors?: { certificate_id: string; error: string }[]; +} + +export const bulkRenewCertificates = (criteria: BulkRenewalCriteria) => + fetchJSON(`${BASE}/certificates/bulk-renew`, { + method: 'POST', + body: JSON.stringify(criteria), + }); + +// L-2 closure (cat-l-8a1fb258a38a): bulk reassign owner (and optionally +// team) for a set of certificates. Narrower than bulk-renew — explicit +// IDs only, no criteria-mode (operators query first, then reassign by +// ID). Pre-L-2 the GUI looped `await updateCertificate(id, { owner_id })`. +// owner_id is required; team_id is optional and updates only when +// non-empty (matches the existing per-cert PUT contract). +export interface BulkReassignmentRequest { + certificate_ids: string[]; + owner_id: string; + team_id?: string; +} + +export interface BulkReassignmentResult { + total_matched: number; + total_reassigned: number; + total_skipped: number; + total_failed: number; + errors?: { certificate_id: string; error: string }[]; +} + +export const bulkReassignCertificates = (request: BulkReassignmentRequest) => + fetchJSON(`${BASE}/certificates/bulk-reassign`, { + method: 'POST', + body: JSON.stringify(request), + }); + // Certificate Export export const exportCertificatePEM = (id: string) => fetchJSON<{ cert_pem: string; chain_pem: string; full_pem: string }>(`${BASE}/certificates/${id}/export/pem`); diff --git a/web/src/pages/CertificatesPage.tsx b/web/src/pages/CertificatesPage.tsx index 4e4d4c5..0560760 100644 --- a/web/src/pages/CertificatesPage.tsx +++ b/web/src/pages/CertificatesPage.tsx @@ -1,7 +1,7 @@ import { useState } from 'react'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { useNavigate } from 'react-router-dom'; -import { getCertificates, createCertificate, triggerRenewal, revokeCertificate, updateCertificate, getOwners, getTeams, getRenewalPolicies, getProfiles, getIssuers, bulkRevokeCertificates } from '../api/client'; +import { getCertificates, createCertificate, revokeCertificate, getOwners, getTeams, getRenewalPolicies, getProfiles, getIssuers, bulkRevokeCertificates, bulkRenewCertificates, bulkReassignCertificates } from '../api/client'; import { useAuth } from '../components/AuthProvider'; import { REVOCATION_REASONS } from '../api/types'; import PageHeader from '../components/PageHeader'; @@ -311,22 +311,38 @@ function BulkReassignModal({ ids, onClose, onSuccess }: { ids: string[]; onClose queryFn: () => getOwners(), }); + // L-2 closure (cat-l-8a1fb258a38a): pre-L-2 this looped + // `await updateCertificate(id, { owner_id })` over the selection + // (N HTTP round-trips). Post-L-2 it's a single POST to + // /api/v1/certificates/bulk-reassign. The CI guardrail in + // .github/workflows/ci.yml (`Forbidden client-side bulk-action loop + // regression guard (L-1)`) catches reintroduction of the loop shape. const handleReassign = async () => { if (!ownerId) return; setRunning(true); setError(''); - let succeeded = 0; - for (const id of ids) { - try { - await updateCertificate(id, { owner_id: ownerId } as Partial); - succeeded++; - setProgress(succeeded); - } catch (err) { - setError(`Failed on ${id}: ${err instanceof Error ? err.message : 'Unknown error'}`); - break; + setProgress(0); + try { + const result = await bulkReassignCertificates({ + certificate_ids: ids, + owner_id: ownerId, + }); + setProgress(result.total_reassigned); + if (result.total_failed > 0) { + const first = result.errors?.[0]; + setError( + `${result.total_failed} of ${result.total_matched} failed${ + first ? `: ${first.certificate_id} — ${first.error}` : '' + }` + ); + } else { + onSuccess(); } + } catch (err) { + setError(`Bulk reassignment failed: ${err instanceof Error ? err.message : 'Unknown error'}`); + } finally { + setRunning(false); } - if (!error) onSuccess(); }; return ( @@ -405,20 +421,32 @@ export default function CertificatesPage() { refetchInterval: 30000, }); + // L-1 closure (cat-l-fa0c1ac07ab5): pre-L-1 this looped + // `await triggerRenewal(ids[i])` over the selection (N HTTP round- + // trips × ~50–200ms each = 5–20s wedge for 100 selected certs). + // Post-L-1 it's a single POST to /api/v1/certificates/bulk-renew; + // the server resolves the criteria, applies status filters + // (RenewalInProgress/Revoked/Archived/Expired all silent-skip), and + // enqueues N renewal jobs server-side, returning a per-cert + // {certificate_id, job_id} envelope. CI guardrail at + // .github/workflows/ci.yml catches loop-shape regression. const handleBulkRenewal = async () => { const ids = Array.from(selectedIds); setBulkRenewProgress({ done: 0, total: ids.length, running: true }); - for (let i = 0; i < ids.length; i++) { - try { - await triggerRenewal(ids[i]); - } catch { - // continue on individual failures - } - setBulkRenewProgress({ done: i + 1, total: ids.length, running: i + 1 < ids.length }); + try { + const result = await bulkRenewCertificates({ certificate_ids: ids }); + setBulkRenewProgress({ + done: result.total_enqueued, + total: result.total_matched, + running: false, + }); + } catch { + // surface as a "0 of N" terminal state — no retries. + setBulkRenewProgress({ done: 0, total: ids.length, running: false }); } queryClient.invalidateQueries({ queryKey: ['certificates'] }); setSelectedIds(new Set()); - setTimeout(() => setBulkRenewProgress(null), 3000); + setTimeout(() => setBulkRenewProgress(null), 5000); }; const columns: Column[] = [