Skip to content

refactor(audit): derive the audit event vocabulary from one table - #6983

Open
pfreixes wants to merge 3 commits into
masterfrom
pau/audit-event-vocabulary
Open

refactor(audit): derive the audit event vocabulary from one table#6983
pfreixes wants to merge 3 commits into
masterfrom
pau/audit-event-vocabulary

Conversation

@pfreixes

@pfreixes pfreixes commented Jul 31, 2026

Copy link
Copy Markdown
Contributor
  • 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

#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

  • ts-build clean, zero errors. packages/webapp is in tsconfig.build.json, so the dashboard is covered
  • 25 audit unit tests pass
  • prettier and oxlint clean
  • 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
  • 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
  • feat(audit): filter the audit log by resource, and by resource + action #6979 restacked onto this branch, with its filter list checked against AuditEventKey

`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>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@pfreixes
pfreixes marked this pull request as ready for review July 31, 2026 13:40
`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>
@pfreixes
pfreixes requested review from a team and TBonnin July 31, 2026 13:57
Comment thread packages/types/lib/audit-trail/event.ts Outdated
| 'billing'
| 'audit_log';
// Not exported: this package emits no JavaScript, so a const here is unusable at runtime — code needing
// the list keeps a twin checked against `AuditEventKey`, as `apiKeyScopes` does for scopes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you not achieve the same derivation without a const that's gonna be discarded at compile time

interface AuditEventTable {
    connection: 'created' | 'updated' | 'metadata_updated' | 'refreshed' | 'deleted';
    sync: 'enabled' | 'disabled' | ...
}

export type AuditResource = keyof AuditEventTable;
export type AuditActionOf<R extends AuditResource> = AuditEventTable[R];
export type AuditAction = AuditActionOf<AuditResource>;
export type AuditEventKey = { [R in AuditResource]: `${R}.${AuditEventTable[R]}` }[AuditResource];

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, easier.

@pfreixes pfreixes Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — interface AuditEventTable in 4c39dc5. Better than I had it: the const was discarded at compile time and needed two lines of comment explaining why it couldn't be exported, both of which are gone now. All five compile-time guards re-verified against the new derivation (endpoint declaration, policy builder, and both assertion directions).

Comment thread packages/audit/lib/event.ts Outdated
type InVocabularyButNotEmitted = Exclude<AuditEventKey, EmittedKey<AuditResourceAction>>;

true satisfies [EmittedButNotInVocabulary] extends [never] ? true : never;
true satisfies [InVocabularyButNotEmitted] extends [never] ? true : never;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, you can probably do it in one line

true satisfies [EmittedButNotInVocabulary, InVocabularyButNotEmitted] extends [never, never] ? true : never;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied. Verified it still catches a failure in either tuple position independently, since a single assertion covering two invariants is easy to get silently half-working — an event emitted but absent from the table, and a table entry nothing emits, each fail it on their own.

…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants