feat(audit): filter the audit log by resource, and by resource + action - #6979
Open
pfreixes wants to merge 21 commits into
Open
feat(audit): filter the audit log by resource, and by resource + action#6979pfreixes wants to merge 21 commits into
pfreixes wants to merge 21 commits into
Conversation
Adds `resource` and `resource_action` materialized search columns (each with a `set` skip index) to `audit_trail_events`, exposes them as repeated `resources` / `actions` query params on GET /api/v1/audit-trail, and wires two selectors into the dashboard audit log. An action is only meaningful attached to a resource, so the Action selector stays disabled until exactly one resource is picked, and the endpoint rejects `actions` sent without `resources`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sync command work landed paused, started, triggered and cancelled on master after this branch was cut. The dashboard's action list is a hand-kept mirror of the emit-side union, so they were silently unfilterable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Preview Deploy
|
Cut the header to the three facts that stop someone undoing a deliberate choice; the rationale behind them is in the PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Confidence score: 4/5
- In
packages/webapp/src/pages/Audit/Show.tsx, theConditionalTooltiptrigger is attached to a non-focusable<span>, so keyboard users can’t access the disabled Action filter explanation, which creates an accessibility gap and can block understanding of why the control is unavailable — make the trigger focusable (or use a semantic focusable element) so tooltip content is reachable via keyboard.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/webapp/src/pages/Audit/Show.tsx">
<violation number="1" location="packages/webapp/src/pages/Audit/Show.tsx:107">
P2: Keyboard users cannot reach the explanation for the disabled Action filter. `ConditionalTooltip` is configured with `asChild`, so its trigger semantics are applied to this `<span>`, but the span is not focusable and the nested `FilterMultiSelect` renders a disabled native button; the tooltip therefore only appears on pointer hover. Making the wrapper focusable allows the existing tooltip to be announced on focus as well.
(Based on your team's feedback about focusable tooltip triggers.) [FEEDBACK_USED].</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| <div className="flex gap-2"> | ||
| <FilterMultiSelect label="Resource" options={resourceOptions} selected={resources} defaultSelect={[ALL]} onChange={onResourcesChange} /> | ||
| <ConditionalTooltip condition={!singleResource} content="Select a single resource to filter by action" asChild> | ||
| <span> |
Contributor
There was a problem hiding this comment.
P2: Keyboard users cannot reach the explanation for the disabled Action filter. ConditionalTooltip is configured with asChild, so its trigger semantics are applied to this <span>, but the span is not focusable and the nested FilterMultiSelect renders a disabled native button; the tooltip therefore only appears on pointer hover. Making the wrapper focusable allows the existing tooltip to be announced on focus as well.
(Based on your team's feedback about focusable tooltip triggers.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/webapp/src/pages/Audit/Show.tsx, line 107:
<comment>Keyboard users cannot reach the explanation for the disabled Action filter. `ConditionalTooltip` is configured with `asChild`, so its trigger semantics are applied to this `<span>`, but the span is not focusable and the nested `FilterMultiSelect` renders a disabled native button; the tooltip therefore only appears on pointer hover. Making the wrapper focusable allows the existing tooltip to be announced on focus as well.
(Based on your team's feedback about focusable tooltip triggers.) .</comment>
<file context>
@@ -83,9 +99,22 @@ export const AuditShow: React.FC = () => {
<div className="flex gap-2">
+ <FilterMultiSelect label="Resource" options={resourceOptions} selected={resources} defaultSelect={[ALL]} onChange={onResourcesChange} />
+ <ConditionalTooltip condition={!singleResource} content="Select a single resource to filter by action" asChild>
+ <span>
+ <FilterMultiSelect
+ label="Action"
</file context>
Suggested change
| <span> | |
| <span tabIndex={0}> |
The cap comment said "the two caps multiply" when there is one constant applied twice; say what the resulting bound actually is. Point the repeated param comment at why these values skip vocabulary validation, which is the difference from the enum query params elsewhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comma-separated is the more common encoding for multi-value query params in this codebase, and both vocabularies are snake_case identifiers so splitting on a comma is safe. Types the querystring as the string that actually goes on the wire rather than the array it denotes, so the contract can't drift from the encoding the way the existing CSV params have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cap bounds the cross product the store builds, so it has to count values rather than characters; assert both sides of the boundary. Verified red with the cap removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cap is the number of values in one param; the cross product is why 50 is the ceiling, not what the cap measures. The test name already says it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bounding a query param is the standing rule, so the cap needs no rationale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It read "No MATERIALIZE" directly above two columns declared MATERIALIZED — a different keyword the comment never distinguished — and justified the omission with a large-table cost the empty table doesn't have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keep what a caller needs — the encoding, the vocabulary and the actions-requires-resources constraint — and drop the defence of the typing choice, which the endpoint tests already enforce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`AuditResource` and `AuditAction` were two independently hand-listed unions, so nothing tied a resource to the actions it actually records. They are now derived from a single table of recorded events, which also yields `AuditEventKey` (`resource.action`) for consumers that need to enumerate the vocabulary — the dashboard's filter list restates it today. Three places name an event and each is now checked against that table: an endpoint's `AuditPolicy` constrains its action by its resource, and two assertions tie the emit-side metadata union to the vocabulary in both directions. Follows the `apiKeyScopes` pattern in @nangohq/utils. Drops `audit_log`, `exported` and `reauthorized`, which no event emits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 31, 2026
… pau/audit-resource-action-filter
`Audit.auditable` took its action parameter independently of its resource, so it accepted a pair no event can emit and only failed later, where the spec is checked against the endpoint's declaration — reporting an assignability mismatch rather than the invalid pair. It now fails on the argument itself. Also derives `AuditAction` from `AuditActionOf` rather than repeating the indexed access, so the table is read in one place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the hand-maintained list with a runtime twin of the vocabulary introduced on the base branch. @nangohq/types emits no JavaScript, so the table can't cross the package boundary as a value; two compile-time checks keep the twin in step, as apiKeyScopes does in @nangohq/utils. Fills in the 14 recorded pairs the hand-kept list was missing — every lifecycle and authentication event from the coverage PRs that merged after this branch was cut. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… pau/audit-resource-action-filter
Renames every user-facing occurrence of "Audit log": the page heading, the profile menu entry, the breadcrumb, the browser tab titles and the not-enabled and access-denied copy. The route and the query keys keep their existing paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tions The table was a const in a package that emits no JavaScript, so it was discarded at compile time and needed a comment explaining why it couldn't be exported. As an interface it derives everything the same way with no runtime artifact and nothing to explain. Both vocabulary assertions fit in one, checking each direction as a tuple position. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… pau/audit-resource-action-filter
The vocabulary table is a type now, so there is no value to export and the "emits no JavaScript" reasoning no longer describes why the dashboard keeps its own copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pull Bot
pushed a commit
to admariner/Pizzly
that referenced
this pull request
Aug 3, 2026
…ngoHQ#6983) - **The audit event vocabulary is one table instead of two independent unions.** `AuditResource` and `AuditAction` were hand-listed separately, so nothing tied a resource to the actions it records: `team` and `deleted` were each valid, which made `team.deleted` declarable even though no such event exists. Both now derive from a single table of recorded events, along with `AuditEventKey` (`resource.action`) for consumers that need to enumerate the vocabulary. - **All three places that name an event are checked against that table.** An endpoint's audit policy and the middleware's policy builder each constrain their action by their resource, so an impossible pair fails where it is written. Two assertions tie the emit-side metadata union to the vocabulary in both directions — rejecting an event emitted with no vocabulary entry, and a vocabulary entry nothing emits. - **Drops `audit_log`, `exported` and `reauthorized`**, none of which any event emits. The table can't express a resource with no actions or an action belonging to no resource, so they go by construction. Type-only. No runtime behaviour changes. ### Why the table isn't exported `@nangohq/types` is `emitDeclarationOnly` — it ships declarations and no JavaScript, so an exported const there is visible to the type checker and absent at runtime. The table stays module-local and only its type projections cross the package boundary. Code that needs the list at runtime keeps a twin checked against `AuditEventKey`. That is what `apiKeyScopes` in `@nangohq/utils` already does for API key scopes: the canonical list lives in `@nangohq/types`, the runtime twin lives in a package that emits JavaScript, and two compile-time assertions keep them in step. This PR adds no twin, because nothing on master consumes the vocabulary at runtime yet. Constraining the policy's action by its resource follows the correlated-parameter pattern already used by `APIEndpointsPicker` in `packages/types/lib/api.endpoints.ts`, `Extract<Event, { subject: TSubject }>` in `packages/pubsub/lib/publisher.ts`, and the task payload lookup in `packages/tasks/lib/types.ts`. ### The builder was the gap Narrowing the policy type alone was not enough. `Audit.auditable` declared its action parameter independently of its resource, so it accepted an impossible pair and the mistake only surfaced downstream, where the middleware spec is checked against the endpoint's declaration — which reports an assignability mismatch between two policy types rather than the invalid pair. Constraining the builder makes it fail on the argument itself. ### What this unblocks NangoHQ#6979 hand-maintains the dashboard's audit filter list against the emit union, which lives in a package the webapp cannot import. Measured against master, that list is missing 14 of the 51 emitted pairs — every lifecycle and authentication event from the coverage PRs that merged after the branch was cut, `app_auth.login` included. `AuditEventKey` lets the list be verified at compile time, so an omission fails the build instead of silently making those events unfilterable. ### Adding an audited event Still four files: the vocabulary table, the emit union's metadata member, the middleware spec, and the endpoint's policy declaration. This PR doesn't reduce that count — it makes the four checked against each other rather than kept in step by hand. ## Test plan - [x] `ts-build` clean, zero errors. `packages/webapp` is in `tsconfig.build.json`, so the dashboard is covered - [x] 25 audit unit tests pass - [x] prettier and oxlint clean - [x] Every existing endpoint declaration and all 56 builder call sites already named a valid pair — both constraints compiled with no change to any of them, so the loose constraints were hiding no live bug - [x] Five deliberate breaks proven red: an action removed from the vocabulary (fails at every endpoint declaration naming it), an event emitted with no vocabulary entry, a vocabulary entry nothing emits, an endpoint declaring `team.deleted`, and the builder called with `team.deleted` - [ ] NangoHQ#6979 restacked onto this branch, with its filter list checked against `AuditEventKey` --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…action-filter The vocabulary and account-zero PRs both landed. Resolves the audit migration ledger assertion: three migrations are applied now, so both new names are listed and the no-re-apply count is 3.
TBonnin
approved these changes
Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
#6983 must be merged first. This PR is stacked on it — the base branch is
pau/audit-event-vocabulary, notmaster. GitHub retargets this tomasterautomatically once that lands.Demo: https://www.loom.com/share/412ec2bf2dc94ccfb1e1c72dc350fa8e
resourceandresource_actionmaterialized search columns toaudit_trail_events, each with asetskip index.resource_actionis a column of its own rather than two conditions because twosetindexes AND-ed prune per column, not per pair — a granule holdingconnection.createdandapi_key.deletedsatisfies bothresource='connection'andaction='deleted'while containing noconnection.deleted. Measured on 200k local rows,resource_action='api_key.deleted'prunes 25 granules to 2.GET /api/v1/audit-trailas comma-separatedresources/actionsquery params.actionswithoutresourcesis rejected with a 400 rather than silently widened, since a match is against a singleresource.actionvalue and needs both halves.On the query param encoding
Both encodings exist in the tree, so I counted: comma-separated is used by four query params (
integrationIdson both connections endpoints,filteron records,syncson sync status) against two for repeated (idson records,includeon integrations). The deciding factor in the existing code is whether a value can contain a comma —getRecordsuses both in one schema and says so forids. Resources and actions aresnake_caseidentifiers that can't, so CSV it is.One deliberate difference from the existing CSV params: the querystring is typed as the string that goes on the wire, not as the array it denotes.
GetConnections['Querystring']declaresintegrationIds?: string[]while sending a comma-joined string, which is untested and simply wrong; typing it honestly means the contract can't drift from the encoding.The filter list is a checked twin, not a hand-kept copy
@nangohq/typesemits no JavaScript, so the vocabulary table can't cross the package boundary as a value. The dashboard keeps a runtime twin inpackages/webapp/src/pages/Audit/constants.ts, kept in step exactly asapiKeyScopesin@nangohq/utilsis:satisfies { [R in AuditResource]: readonly AuditActionOf<R>[] }rejects an action its resource doesn't record, and anExclude<AuditEventKey, …> extends [never]assertion rejects a recorded event missing from the list.That is what #6983 unblocks and why this PR is now stacked on it. The list was previously hand-maintained against a union the dashboard couldn't import, and it had already drifted — 14 of the 51 recorded pairs were missing, every lifecycle and authentication event from the coverage PRs that merged after this branch was cut. Adding an audited event now fails the build until the list is updated.
Notes
There is no standalone
actioncolumn: filtering by action alone isn't offered, so it would index nothing anyone queries.The ALTER only fills new parts. Older parts evaluate the expression on read — correct results, just unpruned — and pick up the column when they next merge. No
MATERIALIZEstep, because the table is empty at this migration and on a large one it would be a blocking mutation at metering boot.Test plan
actionswithoutresources; verified red by making the split a no-opsetindexEXPLAIN indexes=1ts-build, oxlint and prettier clean; audit + audit-trail suites pass (32 tests)auditdatabase on dev