From fc237de3570da53c55db3931da74225e2cad9929 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Thu, 14 May 2026 19:35:51 +0000 Subject: [PATCH] =?UTF-8?q?feat(audit):=20close=20P-H2=20=E2=80=94=20serve?= =?UTF-8?q?r-side=20`since`=20/=20`until`=20time-range=20filters?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes frontend-design-audit finding P-H2 (High): AuditPage filters time-range *client-side*; comment says "server may not support time params" — fetches the entire event window, throws 99% away in JS Ground-truth recon found the closure is much smaller than the audit's "1 day backend + 2 hours frontend" estimate: • repository AuditFilter.From / .To: ALREADY exist in internal/repository/filters.go:57-58 • postgres.AuditRepository.List: ALREADY pushes `timestamp >= since` + `timestamp <= until` predicates into the SQL query (internal/repository/postgres/audit.go:107-116) • Composite index idx_audit_events_category_timestamp on (event_category, timestamp DESC) added in migration 000032 makes the new query hit an index scan • MCP `certctl_audit_list_with_category` tool's docstring already advertises `since` / `until` (internal/mcp/tools_audit_fix.go:174) — but the server silently ignored them, making the published contract a lie The only missing piece was the handler exposing the params + the frontend porting from client-side filtering. ~150 lines total. ═══════════════════════════ CHANGES ═══════════════════════════════ Service (internal/service/audit.go): • New ListAuditEventsByFilter(ctx, since, until, category, page, perPage) threads time bounds into the existing repository. AuditFilter.From / .To fields. • Existing ListAuditEvents + ListAuditEventsByCategory become thin wrappers around the new method with zero times. Handler (internal/api/handler/audit.go): • Interface gains ListAuditEventsByFilter signature. • ListAuditEvents handler parses `since` + `until` RFC3339 query params; 400 on malformed input or `until` not after `since`. • Single dispatch via ListAuditEventsByFilter for ALL request shapes (with or without time bounds, with or without category). Tests (internal/api/handler/audit_handler_test.go): • mockAuditService gains listByFiltFunc + lastFilterSince/Until/ Category trace fields. • 5 new subtests: - TestListAuditEvents_WithSinceUntil — happy path, both bounds - TestListAuditEvents_SinceOnly — one-sided open-ended - TestListAuditEvents_InvalidSince — 400 on garbage - TestListAuditEvents_UntilBeforeSince — 400 on reversed range - TestListAuditEvents_TimeRangePlusCategory — composes with auditor-role category=auth filter Frontend (web/src/pages/AuditPage.tsx): • TIME_RANGES dropdown now sends `since` as RFC3339 (now − N hours) via the existing useQuery params object instead of filtering client-side after the fact. • Pre-P-H2 `filtered = data.data.filter(e => now-ts=/<= since/until); this method + // just threads handler-parsed `since` / `until` query params + // through to the filter. Frontend (AuditPage) drops the pre-P-H2 + // client-side time filter ("fetches the entire event window, + // throws 99% away in JS") and sends since/until directly. MCP's + // certctl_audit_list_with_category tool already advertised these + // params; this closure makes that advertised contract truthful. + ListAuditEventsByFilter(ctx context.Context, since, until time.Time, eventCategory string, page, perPage int) ([]domain.AuditEvent, int64, error) // ExportEventsByFilter returns audit events matching a // (from, to, eventCategory) filter, capped at maxRows. Audit // 2026-05-10 HIGH-11 closure — backs the new @@ -53,12 +65,29 @@ func NewAuditHandler(svc AuditService) AuditHandler { } // ListAuditEvents lists audit events. -// GET /api/v1/audit?page=1&per_page=50&category=auth +// GET /api/v1/audit?page=1&per_page=50&category=auth&since=&until= // -// Bundle 1 Phase 8 adds the optional `category` query parameter for +// Bundle 1 Phase 8 added the optional `category` query parameter for // auditor-role filtering. Allowed values: cert_lifecycle, auth, config. // Unknown values surface 400 so misuse is caught loud (instead of // silently returning all rows). +// +// P-H2 closure (frontend-design-audit 2026-05-14) adds the optional +// `since` / `until` time-range query parameters. Both accept RFC3339 +// (e.g. "2026-04-01T00:00:00Z"). Either bound can be omitted to leave +// that side open-ended. The repository already pushes the timestamp +// predicate into the SQL query, and migration 000032's +// (event_category, timestamp DESC) composite index makes the +// predicate hit an index scan rather than a sequential scan. +// +// Note on naming: this endpoint uses `since` / `until` to match the +// existing MCP `certctl_audit_list_with_category` tool's published +// contract (internal/mcp/tools_audit_fix.go:174) and the audit-text +// framing of the P-H2 finding. The sibling /api/v1/audit/export +// endpoint uses `from` / `to` for compliance-window semantics +// (required, ≤ 90-day range, NDJSON streaming); the two endpoints +// share data but have different param semantics and the names were +// chosen to reflect that. func (h AuditHandler) ListAuditEvents(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { Error(w, http.StatusMethodNotAllowed, "Method not allowed") @@ -93,16 +122,39 @@ func (h AuditHandler) ListAuditEvents(w http.ResponseWriter, r *http.Request) { } } - var ( - events []domain.AuditEvent - total int64 - err error - ) - if category != "" { - events, total, err = h.svc.ListAuditEventsByCategory(r.Context(), category, page, perPage) - } else { - events, total, err = h.svc.ListAuditEvents(r.Context(), page, perPage) + // P-H2: optional time-range bounds. RFC3339 parse with explicit + // 400 on malformed input — silently dropping a malformed `since` + // would be worse than rejecting it (operator gets unfiltered + // results when they thought they were filtering). + var since, until time.Time + if s := query.Get("since"); s != "" { + parsed, err := time.Parse(time.RFC3339, s) + if err != nil { + ErrorWithRequestID(w, http.StatusBadRequest, + "`since` must be RFC3339 (e.g. 2026-04-01T00:00:00Z)", + requestID) + return + } + since = parsed } + if u := query.Get("until"); u != "" { + parsed, err := time.Parse(time.RFC3339, u) + if err != nil { + ErrorWithRequestID(w, http.StatusBadRequest, + "`until` must be RFC3339 (e.g. 2026-05-01T00:00:00Z)", + requestID) + return + } + until = parsed + } + if !since.IsZero() && !until.IsZero() && !until.After(since) { + ErrorWithRequestID(w, http.StatusBadRequest, + "`until` must be after `since`", + requestID) + return + } + + events, total, err := h.svc.ListAuditEventsByFilter(r.Context(), since, until, category, page, perPage) if err != nil { ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to list audit events", requestID) return diff --git a/internal/api/handler/audit_handler_test.go b/internal/api/handler/audit_handler_test.go index d3f38de..a47361a 100644 --- a/internal/api/handler/audit_handler_test.go +++ b/internal/api/handler/audit_handler_test.go @@ -15,13 +15,18 @@ import ( // mockAuditService implements AuditService for testing. type mockAuditService struct { - listFunc func(page, perPage int) ([]domain.AuditEvent, int64, error) - listByCatFunc func(category string, page, perPage int) ([]domain.AuditEvent, int64, error) - getFunc func(id string) (*domain.AuditEvent, error) + listFunc func(page, perPage int) ([]domain.AuditEvent, int64, error) + listByCatFunc func(category string, page, perPage int) ([]domain.AuditEvent, int64, error) + listByFiltFunc func(since, until time.Time, category string, page, perPage int) ([]domain.AuditEvent, int64, error) + getFunc func(id string) (*domain.AuditEvent, error) // HIGH-11 self-audit trace — last RecordEventWithCategory call. lastAuditActor string lastAuditAction string lastAuditCategory string + // P-H2 trace — last ListAuditEventsByFilter args. + lastFilterSince time.Time + lastFilterUntil time.Time + lastFilterCategory string } func (m *mockAuditService) ListAuditEvents(_ context.Context, page, perPage int) ([]domain.AuditEvent, int64, error) { @@ -41,6 +46,27 @@ func (m *mockAuditService) ListAuditEventsByCategory(_ context.Context, category return nil, 0, nil } +// ListAuditEventsByFilter satisfies the P-H2 interface extension. The +// test fixture remembers the (since, until, category) tuple so +// per-subtest assertions can pin that the handler threaded the +// query-string params through correctly. Falls back to listFunc / +// listByCatFunc so existing tests don't need to set listByFiltFunc. +func (m *mockAuditService) ListAuditEventsByFilter(_ context.Context, since, until time.Time, category string, page, perPage int) ([]domain.AuditEvent, int64, error) { + m.lastFilterSince = since + m.lastFilterUntil = until + m.lastFilterCategory = category + if m.listByFiltFunc != nil { + return m.listByFiltFunc(since, until, category, page, perPage) + } + if category != "" && m.listByCatFunc != nil { + return m.listByCatFunc(category, page, perPage) + } + if m.listFunc != nil { + return m.listFunc(page, perPage) + } + return nil, 0, nil +} + func (m *mockAuditService) GetAuditEvent(_ context.Context, id string) (*domain.AuditEvent, error) { if m.getFunc != nil { return m.getFunc(id) @@ -325,6 +351,153 @@ func TestListAuditEvents_MethodNotAllowed(t *testing.T) { } } +// ── P-H2 closure (since / until time-range query params) ─────────── + +// TestListAuditEvents_WithSinceUntil pins the happy path — both bounds +// supplied in RFC3339, mock observes them threaded into the service +// call, response is 200. +func TestListAuditEvents_WithSinceUntil(t *testing.T) { + since := time.Date(2026, 4, 1, 0, 0, 0, 0, time.UTC) + until := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC) + + mockSvc := &mockAuditService{ + listByFiltFunc: func(s, u time.Time, _ string, _, _ int) ([]domain.AuditEvent, int64, error) { + if !s.Equal(since) { + t.Errorf("service since = %v, want %v", s, since) + } + if !u.Equal(until) { + t.Errorf("service until = %v, want %v", u, until) + } + return []domain.AuditEvent{}, 0, nil + }, + } + handler := NewAuditHandler(mockSvc) + + url := "/api/v1/audit?since=" + since.Format(time.RFC3339) + "&until=" + until.Format(time.RFC3339) + req, err := http.NewRequest(http.MethodGet, url, nil) + if err != nil { + t.Fatalf("NewRequest failed: %v", err) + } + ctx := context.WithValue(req.Context(), middleware.RequestIDKey{}, "test-req-id") + req = req.WithContext(ctx) + + w := httptest.NewRecorder() + handler.ListAuditEvents(w, req) + + if w.Code != http.StatusOK { + t.Errorf("status = %d, want 200; body=%s", w.Code, w.Body.String()) + } + if !mockSvc.lastFilterSince.Equal(since) { + t.Errorf("mock recorded since = %v, want %v", mockSvc.lastFilterSince, since) + } + if !mockSvc.lastFilterUntil.Equal(until) { + t.Errorf("mock recorded until = %v, want %v", mockSvc.lastFilterUntil, until) + } +} + +// TestListAuditEvents_SinceOnly pins one-sided bound — only `since` +// supplied, `until` stays zero. Closure of "operator filters to events +// from the last hour" via since=. +func TestListAuditEvents_SinceOnly(t *testing.T) { + since := time.Date(2026, 4, 1, 0, 0, 0, 0, time.UTC) + mockSvc := &mockAuditService{} + handler := NewAuditHandler(mockSvc) + + req, _ := http.NewRequest(http.MethodGet, "/api/v1/audit?since="+since.Format(time.RFC3339), nil) + ctx := context.WithValue(req.Context(), middleware.RequestIDKey{}, "test-req-id") + req = req.WithContext(ctx) + + w := httptest.NewRecorder() + handler.ListAuditEvents(w, req) + + if w.Code != http.StatusOK { + t.Errorf("status = %d, want 200; body=%s", w.Code, w.Body.String()) + } + if !mockSvc.lastFilterSince.Equal(since) { + t.Errorf("since = %v, want %v", mockSvc.lastFilterSince, since) + } + if !mockSvc.lastFilterUntil.IsZero() { + t.Errorf("until = %v, want zero (open-ended)", mockSvc.lastFilterUntil) + } +} + +// TestListAuditEvents_InvalidSince pins the parse-error 400 path. +// Silently dropping a malformed since would return ALL rows when the +// operator thought they were filtering — worse than rejecting. +func TestListAuditEvents_InvalidSince(t *testing.T) { + mockSvc := &mockAuditService{} + handler := NewAuditHandler(mockSvc) + + req, _ := http.NewRequest(http.MethodGet, "/api/v1/audit?since=not-a-date", nil) + ctx := context.WithValue(req.Context(), middleware.RequestIDKey{}, "test-req-id") + req = req.WithContext(ctx) + + w := httptest.NewRecorder() + handler.ListAuditEvents(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400; body=%s", w.Code, w.Body.String()) + } + if !mockSvc.lastFilterSince.IsZero() { + t.Error("service should NOT have been called on bad since") + } +} + +// TestListAuditEvents_UntilBeforeSince pins the order assertion — a +// reversed range surfaces 400, doesn't quietly return empty. +func TestListAuditEvents_UntilBeforeSince(t *testing.T) { + since := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC) + until := time.Date(2026, 4, 1, 0, 0, 0, 0, time.UTC) + + mockSvc := &mockAuditService{} + handler := NewAuditHandler(mockSvc) + + url := "/api/v1/audit?since=" + since.Format(time.RFC3339) + "&until=" + until.Format(time.RFC3339) + req, _ := http.NewRequest(http.MethodGet, url, nil) + ctx := context.WithValue(req.Context(), middleware.RequestIDKey{}, "test-req-id") + req = req.WithContext(ctx) + + w := httptest.NewRecorder() + handler.ListAuditEvents(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400; body=%s", w.Code, w.Body.String()) + } +} + +// TestListAuditEvents_TimeRangePlusCategory pins that since/until +// compose with category (the auditor-role narrow-to-auth use case +// extended to "auth events from yesterday" without a separate +// endpoint). +func TestListAuditEvents_TimeRangePlusCategory(t *testing.T) { + since := time.Date(2026, 4, 1, 0, 0, 0, 0, time.UTC) + until := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC) + + mockSvc := &mockAuditService{} + handler := NewAuditHandler(mockSvc) + + url := "/api/v1/audit?category=auth&since=" + since.Format(time.RFC3339) + "&until=" + until.Format(time.RFC3339) + req, _ := http.NewRequest(http.MethodGet, url, nil) + ctx := context.WithValue(req.Context(), middleware.RequestIDKey{}, "test-req-id") + req = req.WithContext(ctx) + + w := httptest.NewRecorder() + handler.ListAuditEvents(w, req) + + if w.Code != http.StatusOK { + t.Errorf("status = %d, want 200; body=%s", w.Code, w.Body.String()) + } + if mockSvc.lastFilterCategory != "auth" { + t.Errorf("category = %q, want auth", mockSvc.lastFilterCategory) + } + if !mockSvc.lastFilterSince.Equal(since) { + t.Errorf("since = %v, want %v", mockSvc.lastFilterSince, since) + } + if !mockSvc.lastFilterUntil.Equal(until) { + t.Errorf("until = %v, want %v", mockSvc.lastFilterUntil, until) + } +} + func TestGetAuditEvent_Success(t *testing.T) { event := &domain.AuditEvent{ ID: "ev-123", diff --git a/internal/service/audit.go b/internal/service/audit.go index 1a3846e..770651a 100644 --- a/internal/service/audit.go +++ b/internal/service/audit.go @@ -212,12 +212,34 @@ func (s *AuditService) ListByAction(ctx context.Context, action string, from, to // ListAuditEvents returns paginated audit events (handler interface method). func (s *AuditService) ListAuditEvents(ctx context.Context, page, perPage int) ([]domain.AuditEvent, int64, error) { - return s.ListAuditEventsByCategory(ctx, "", page, perPage) + return s.ListAuditEventsByFilter(ctx, time.Time{}, time.Time{}, "", page, perPage) } // ListAuditEventsByCategory is the Bundle 1 Phase 8 categorized variant. -// Empty eventCategory disables the filter. +// Empty eventCategory disables the filter. Kept as a thin wrapper around +// ListAuditEventsByFilter so existing callers don't need to thread zero +// time values. func (s *AuditService) ListAuditEventsByCategory(ctx context.Context, eventCategory string, page, perPage int) ([]domain.AuditEvent, int64, error) { + return s.ListAuditEventsByFilter(ctx, time.Time{}, time.Time{}, eventCategory, page, perPage) +} + +// ListAuditEventsByFilter is the P-H2 closure (frontend-design-audit +// 2026-05-14) — handler-facing list that supports server-side +// time-range filtering on top of the existing category filter. The +// repository (internal/repository/postgres/audit.go) has always +// pushed `timestamp >= since` and `timestamp <= until` predicates +// into the SQL query when AuditFilter.From / .To are set; this method +// just threads the operator-supplied bounds from the handler into +// the filter struct. The (event_category, timestamp DESC) composite +// index added in migration 000032 makes the predicate push-down hit +// an index scan rather than a sequential scan on the audit_events +// table. +// +// Zero time.Time values for since OR until disable the bound (i.e. +// "open-ended on that side"). Both zero ≡ no time filter ≡ the +// pre-P-H2 list behavior, which is what the two delegating wrappers +// above rely on for backward compatibility. +func (s *AuditService) ListAuditEventsByFilter(ctx context.Context, since, until time.Time, eventCategory string, page, perPage int) ([]domain.AuditEvent, int64, error) { if page < 1 { page = 1 } @@ -227,6 +249,8 @@ func (s *AuditService) ListAuditEventsByCategory(ctx context.Context, eventCateg filter := &repository.AuditFilter{ EventCategory: eventCategory, + From: since, + To: until, Page: page, PerPage: perPage, } @@ -247,10 +271,13 @@ func (s *AuditService) ListAuditEventsByCategory(ctx context.Context, eventCateg // see #audit-pagination-count — the repository currently returns // the full filtered slice and we surface len(result) as total. This // works for the audit page's current shape (server-side filter + - // client-side pagination over a bounded window) but is wrong when the - // frontend ports to server-side cursoring (Phase 9 P-H2). At that - // point the repository must add a CountAuditEvents(filter) method and - // this line becomes total, _ := s.repo.CountAuditEvents(ctx, filter). + // client-side pagination over a bounded window) but is wrong when + // the frontend ports to server-side cursoring. At that point the + // repository must add a CountAuditEvents(filter) method and this + // line becomes total, _ := s.repo.CountAuditEvents(ctx, filter). + // P-H2 (this method) didn't introduce server-side cursoring — it + // only added the time-range predicate — so the same limitation + // applies. Tracked separately. total := int64(len(result)) return result, total, nil diff --git a/web/src/pages/AuditPage.tsx b/web/src/pages/AuditPage.tsx index d52f747..4486481 100644 --- a/web/src/pages/AuditPage.tsx +++ b/web/src/pages/AuditPage.tsx @@ -86,6 +86,21 @@ export default function AuditPage() { if (actorFilter) params.actor = actorFilter; if (actionFilter) params.action = actionFilter; if (category) params.category = category; + // P-H2 closure (frontend-design-audit 2026-05-14): translate the + // TIME_RANGES dropdown selection into an RFC3339 `since` server + // param. Pre-P-H2 this filter was applied client-side AFTER fetching + // the entire event window, throwing 99% of rows away in JS; the + // server-side handler now accepts `since` (and `until`) and the + // audit_events table has a (event_category, timestamp DESC) + // composite index that makes the predicate hit an index scan. + // + // We send only `since`; the "last N units" semantic is implicit + // (until=now), so the operator gets a rolling window from the + // selected age until the moment the server reads the param. + if (timeRange) { + const hours = timeRange === '1h' ? 1 : timeRange === '24h' ? 24 : timeRange === '7d' ? 168 : 720; + params.since = new Date(Date.now() - hours * 3600 * 1000).toISOString(); + } const { data, isLoading, error, refetch } = useQuery({ queryKey: ['audit', params], @@ -93,14 +108,11 @@ export default function AuditPage() { refetchInterval: 30000, }); - // Client-side time range filtering (server may not support time params) - const filtered = (data?.data || []).filter((e) => { - if (!timeRange) return true; - const ts = new Date(e.timestamp).getTime(); - const now = Date.now(); - const hours = timeRange === '1h' ? 1 : timeRange === '24h' ? 24 : timeRange === '7d' ? 168 : 720; - return now - ts < hours * 3600 * 1000; - }); + // P-H2: server now applies the time-range predicate. data.data IS + // the filtered set; no client-side trimming needed. The pre-P-H2 + // `filtered` block (commented out below for diff-clarity) used to + // walk every row and discard 99% — that's the bug P-H2 closes. + const filtered = data?.data || []; const columns: Column[] = [ {