Harden signer frontend test coverage

Author: archbilesherman

.github/workflows/web-verify.yml

@@ -75,3 +75,46 @@ jobs:
 
       - name: Production build
         run: pnpm build
+
+  signer-e2e:
+    name: Signer Playwright
+    runs-on: ubuntu-latest
+    timeout-minutes: 15
+    defaults:
+      run:
+        working-directory: web
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+
+      - name: Set up pnpm
+        uses: pnpm/action-setup@v4
+        with:
+          package_json_file: web/package.json
+
+      - name: Set up Node
+        uses: actions/setup-node@v4
+        with:
+          node-version: 22
+          cache: pnpm
+          cache-dependency-path: web/pnpm-lock.yaml
+
+      - name: Install
+        run: pnpm install --frozen-lockfile
+
+      - name: Install Chromium
+        run: pnpm exec playwright install --with-deps chromium
+
+      - name: Run signer end-to-end tests
+        run: pnpm test:e2e:signer
+
+      - name: Upload Playwright artifacts
+        if: failure()
+        uses: actions/upload-artifact@v4
+        with:
+          name: signer-playwright-artifacts
+          path: |
+            web/playwright-report/
+            web/test-results/
+          if-no-files-found: ignore
+          retention-days: 7

web/package.json

@@ -17,6 +17,7 @@
     "test": "vitest run --passWithNoTests",
     "test:watch": "vitest",
     "test:e2e": "playwright test",
+    "test:e2e:signer": "playwright test tests/signer-certificates.e2e.ts --project=chromium --workers=1",
     "clean": "rm -rf .next dist coverage",
     "gen:api": "tsx scripts/gen-api.mts",
     "gen:api:check": "pnpm gen:api && git diff --quiet src/generated || (echo 'OpenAPI codegen drift — re-run pnpm gen:api' && exit 1)",

web/playwright.config.ts

@@ -40,7 +40,7 @@ export default defineConfig({
     },
   ],
   webServer: {
-    command: `pnpm dev --port ${port}`,
+    command: `corepack pnpm dev --port ${port}`,
     url: baseURL,
     reuseExistingServer: false,
     timeout: 120_000,

web/src/app/api/v1/[...path]/__tests__/route.test.ts

@@ -58,7 +58,7 @@ describe("api v1 proxy route", () => {
     fetchMock.mockResolvedValueOnce(
       new Response(JSON.stringify({ certificates: [] }), {
         status: 200,
-        headers: { "content-type": "application/json" },
+        headers: { "content-type": "application/json", "x-trace-id": "trace-signer-1" },
       }),
     );
 
@@ -71,6 +71,32 @@ describe("api v1 proxy route", () => {
     const [url, init] = fetchMock.mock.calls[0] ?? [];
     expect(String(url)).toBe("https://signer.example.org/api/v1/admin/certificates?limit=20");
     expect(new Headers(init?.headers).get("authorization")).toBe("Bearer access-token-abc");
+    expect(response.headers.get("x-trace-id")).toBe("trace-signer-1");
+  });
+
+  it("forwards signer request bodies and query parameters", async () => {
+    fetchMock.mockResolvedValueOnce(
+      new Response(JSON.stringify({ success: true }), {
+        status: 200,
+        headers: { "content-type": "application/json" },
+      }),
+    );
+
+    await POST(
+      new NextRequest("http://localhost:3000/api/v1/signer/admin/certificates/42/revoke?audit=true", {
+        method: "POST",
+        body: JSON.stringify({ reason: "compromised" }),
+        headers: { "content-type": "application/json", accept: "application/json" },
+      }),
+      { params: Promise.resolve({ path: ["signer", "admin", "certificates", "42", "revoke"] }) },
+    );
+
+    const [url, init] = fetchMock.mock.calls[0] ?? [];
+    expect(String(url)).toBe(
+      "https://signer.example.org/api/v1/admin/certificates/42/revoke?audit=true",
+    );
+    expect(init?.body).toBe(JSON.stringify({ reason: "compromised" }));
+    expect(new Headers(init?.headers).get("content-type")).toBe("application/json");
   });
 
   it("proxies no-content backend responses without constructing a response body", async () => {
@@ -87,5 +113,8 @@ describe("api v1 proxy route", () => {
 
     expect(response.status).toBe(204);
     expect(await response.text()).toBe("");
+    expect(String(fetchMock.mock.calls[0]?.[0])).toBe(
+      "https://core.example.org/roles/role-1/privileges",
+    );
   });
 });

web/src/features/core/signer/__tests__/CertificateDetail.test.tsx

@@ -1,41 +1,42 @@
-// Licensed to the Apache Software Foundation (ASF) under one
-// or more contributor license agreements.  See the NOTICE file
-// distributed with this work for additional information
-// regarding copyright ownership.  The ASF licenses this file
-// to you under the Apache License, Version 2.0 (the
-// "License"); you may not use this file except in compliance
-// with the License.  You may obtain a copy of the License at
-//
-//   http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing,
-// software distributed under the License is distributed on an
-// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-// KIND, either express or implied.  See the License for the
-// specific language governing permissions and limitations
-// under the License.
-
-import { render, screen } from "@testing-library/react";
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0.
+
+import { fireEvent, render, screen } from "@testing-library/react";
 import { beforeEach, describe, expect, it, vi } from "vitest";
+import { ApiError } from "@/shared/api/client";
 import type { Certificate } from "../schemas";
 
-// Presentation gating only — backend enforces the real authorization.
+const baseCertificate: Certificate = {
+  tenant_id: "tenant-1",
+  client_id: "signer-client",
+  serial_number: 42,
+  key_id: "key-42",
+  principal: "someone-else",
+  user_email: "admin@example.org",
+  public_key_fingerprint: "SHA256:pk",
+  ca_fingerprint: "SHA256:ca",
+  valid_after: 1_700_000_000,
+  valid_before: 4_102_444_800,
+  issued_at: 1_700_000_000,
+  source_ip: "192.0.2.42",
+  granted_extensions: ["permit-pty", "permit-user-rc"],
+  force_command: "/usr/bin/id",
+  revoked: false,
+};
+
 const state = vi.hoisted(() => ({
   canManage: true,
-  cert: {
-    tenant_id: "tenant-1",
-    client_id: "signer-client",
-    serial_number: 42,
-    key_id: "k",
-    principal: "someone-else",
-    user_email: "admin@example.org",
-    public_key_fingerprint: "SHA256:pk",
-    ca_fingerprint: "SHA256:ca",
-    valid_after: 1_700_000_000,
-    valid_before: 4_102_444_800,
-    issued_at: 1_700_000_000,
-    revoked: false,
-  } as Certificate,
+  query: {} as {
+    data?: Certificate;
+    isLoading: boolean;
+    error: Error | null;
+    refetch: ReturnType<typeof vi.fn>;
+  },
+  mutate: vi.fn(),
+  isPending: false,
+  toastSuccess: vi.fn(),
 }));
 
 vi.mock("@/shared/casl/AbilityProvider", () => ({
@@ -45,52 +46,139 @@ vi.mock("@/shared/casl/AbilityProvider", () => ({
   }),
 }));
 
+vi.mock("sonner", () => ({ toast: { success: state.toastSuccess } }));
+
 vi.mock("../queries", () => ({
-  useCertificate: () => ({ data: state.cert, isLoading: false, error: null, refetch: vi.fn() }),
-  useRevokeCertificate: () => ({ mutate: vi.fn(), isPending: false }),
+  useCertificate: () => state.query,
+  useRevokeCertificate: () => ({ mutate: state.mutate, isPending: state.isPending }),
 }));
 
 import { CertificateDetail } from "../components/CertificateDetail";
 
 beforeEach(() => {
   state.canManage = true;
-  state.cert = { ...state.cert, revoked: false };
+  state.query = {
+    data: { ...baseCertificate },
+    isLoading: false,
+    error: null,
+    refetch: vi.fn(),
+  };
+  state.mutate.mockReset();
+  state.toastSuccess.mockReset();
+  state.isPending = false;
 });
 
-describe("<CertificateDetail /> revoke gating", () => {
-  it("shows the Revoke button for an admin with the signer write privilege", () => {
-    state.canManage = true;
+function submitRevoke(reason = "compromised") {
+  fireEvent.click(screen.getByRole("button", { name: /^Revoke$/ }));
+  fireEvent.change(screen.getByLabelText(/reason/i), { target: { value: reason } });
+  fireEvent.click(screen.getByRole("button", { name: /^Confirm revoke$/ }));
+}
+
+describe("<CertificateDetail />", () => {
+  it("renders complete issuance metadata", () => {
+    render(<CertificateDetail serial="42" />);
+    for (const value of [
+      "tenant-1",
+      "signer-client",
+      "admin@example.org",
+      "key-42",
+      "SHA256:pk",
+      "SHA256:ca",
+      "192.0.2.42",
+      "permit-pty, permit-user-rc",
+      "/usr/bin/id",
+    ]) {
+      expect(screen.getByText(value)).toBeInTheDocument();
+    }
+  });
+
+  it("renders a loading skeleton", () => {
+    state.query = { ...state.query, data: undefined, isLoading: true };
+    const { container } = render(<CertificateDetail serial="42" />);
+    expect(container.querySelector(".animate-pulse")).toBeInTheDocument();
+  });
+
+  it("renders not-found handling for a 404", () => {
+    state.query = {
+      ...state.query,
+      data: undefined,
+      error: new ApiError(404, "/certificate/99", { error: "not_found" }),
+    };
+    render(<CertificateDetail serial="99" />);
+    expect(screen.getByRole("heading", { name: /certificate not found/i })).toBeInTheDocument();
+  });
+
+  it("renders a retryable detail failure", () => {
+    state.query = { ...state.query, data: undefined, error: new Error("signer unavailable") };
+    render(<CertificateDetail serial="42" />);
+    fireEvent.click(screen.getByRole("button", { name: /try again/i }));
+    expect(state.query.refetch).toHaveBeenCalledOnce();
+  });
+
+  it("shows the Revoke button only for an active writer", () => {
     render(<CertificateDetail serial="42" />);
     expect(screen.getByRole("button", { name: /^Revoke$/ })).toBeInTheDocument();
   });
 
-  it("hides the Revoke button for a user without the privilege (ownership is irrelevant)", () => {
+  it("hides Revoke without write privilege", () => {
     state.canManage = false;
     render(<CertificateDetail serial="42" />);
     expect(screen.queryByRole("button", { name: /^Revoke$/ })).not.toBeInTheDocument();
   });
 
-  it("hides the Revoke button when the certificate is already revoked", () => {
-    state.canManage = true;
-    state.cert = {
-      ...state.cert,
+  it.each([
+    ["revoked", { revoked: true, revoked_at: 1_700_500_000, revocation_reason: "old" }],
+    ["expired", { valid_before: 1 }],
+    ["not yet valid", { valid_after: 4_102_444_800 }],
+  ])("hides Revoke when the certificate is %s", (_label, overrides) => {
+    state.query.data = { ...baseCertificate, ...overrides };
+    render(<CertificateDetail serial="42" />);
+    expect(screen.queryByRole("button", { name: /^Revoke$/ })).not.toBeInTheDocument();
+  });
+
+  it("renders authoritative revocation metadata", () => {
+    state.query.data = {
+      ...baseCertificate,
       revoked: true,
       revoked_at: 1_700_500_000,
-      revocation_reason: "old",
+      revocation_reason: "original reason",
+      revoked_by: "admin-id",
     };
     render(<CertificateDetail serial="42" />);
-    expect(screen.queryByRole("button", { name: /^Revoke$/ })).not.toBeInTheDocument();
+    expect(screen.getByText("original reason")).toBeInTheDocument();
+    expect(screen.getByText("admin-id")).toBeInTheDocument();
   });
 
-  it("hides the Revoke button when the certificate is expired", () => {
-    state.cert = { ...state.cert, valid_before: 1 };
+  it.each([
+    [403, "You no longer have permission to revoke certificates."],
+    [409, "This certificate is no longer active and cannot be revoked."],
+  ])("keeps the dialog open for API %s", (status, expected) => {
+    state.mutate.mockImplementation((_variables, options) => {
+      options.onError(new ApiError(status, "/revoke", { error: "failure" }));
+    });
     render(<CertificateDetail serial="42" />);
-    expect(screen.queryByRole("button", { name: /^Revoke$/ })).not.toBeInTheDocument();
+    submitRevoke();
+    expect(screen.getByRole("dialog")).toBeInTheDocument();
+    expect(screen.getByText(expected)).toBeInTheDocument();
   });
 
-  it("hides the Revoke button when the certificate is not yet valid", () => {
-    state.cert = { ...state.cert, valid_after: 4_102_444_800 };
+  it("keeps the dialog open after a retryable network failure", () => {
+    state.mutate.mockImplementation((_variables, options) => {
+      options.onError(new Error("Network request failed"));
+    });
     render(<CertificateDetail serial="42" />);
-    expect(screen.queryByRole("button", { name: /^Revoke$/ })).not.toBeInTheDocument();
+    submitRevoke();
+    expect(screen.getByRole("dialog")).toBeInTheDocument();
+    expect(screen.getByText("Network request failed")).toBeInTheDocument();
+  });
+
+  it("treats an already-revoked response as informational success", () => {
+    state.mutate.mockImplementation((_variables, options) => {
+      options.onSuccess({ already_revoked: true });
+    });
+    render(<CertificateDetail serial="42" />);
+    submitRevoke();
+    expect(state.toastSuccess).toHaveBeenCalledWith("Certificate was already revoked");
+    expect(screen.queryByRole("dialog")).not.toBeInTheDocument();
   });
 });