Skip to content

Record an identity provider being registered, not only removed - #462

Open
zopeVaibhav wants to merge 1 commit into
CopilotKit:mainfrom
zopeVaibhav:fix/audit-identity-provider-registration
Open

Record an identity provider being registered, not only removed#462
zopeVaibhav wants to merge 1 commit into
CopilotKit:mainfrom
zopeVaibhav:fix/audit-identity-provider-registration

Conversation

@zopeVaibhav

Copy link
Copy Markdown
Contributor

What this changes

Registering an identity provider now leaves an audit row, the way removing one already did. Both are
written in the catch-all that forwards /api/auth/* to the sign-in library, at the point where this
deployment already checks the caller is an administrator (server/src/app.ts:280-283).

  • identity_provider.registered and identity_provider.removed for POST /api/auth/sso/register
    and POST /api/auth/sso/delete-provider. The first was declared in the vocabulary with no writer
    behind it (server/src/audit.ts:325).
  • The body is read from a clone before auth.handler runs. The handler consumes the stream, so a
    clone taken afterwards is of a request whose body is gone and the row would name no provider.
  • The row is written only when the library answered ok, so a refused registration is not recorded as
    one.
  • The actor comes from auth.api.getSession, the same call the role check on those routes makes.

The admin screen's own removal keeps its existing row (server/src/app.ts:735-742); this adds the
path that went around it.

Two things worth a reviewer's eye

Payload key. The existing removal row uses { removedBy: <email> }. These use
{ providerId, by: <email> }, because one writer serves both events and removedBy reads wrongly on
a registration. That leaves two shapes for one target type; happy to split into per-event keys if
uniformity is preferred.

update-provider is untouched. It has no event type declared, so recording it means adding
vocabulary rather than adding a writer. Left out deliberately.

Where it runs

  • New state that outlives a request? None — existing audit_events table, existing store.
  • What happens on the second replica? Whichever process answers writes the row from that
    request's session. Nothing here claims shared work, so no coordination is needed.
  • Anything serialised? Nothing new. audit_events is append-only and rows are independent.
  • Anything fanned out to a browser? No. The Audit screen reads the table on its own query.
  • New listener, port, or schedule? None.

Boundary and audit

  • Every acting call still goes through the gateway. This is the audit step for two acting calls
    that had none.
  • New refusals and failures each write a row — with one deliberate exception: a refused
    registration writes nothing, because the library's refusal is not something this deployment
    did, and recording it as a registration would be worse than the gap being fixed.
  • Nothing new is trusted from the client. The actor comes from the session; the only
    request-supplied value is providerId, which is what the library itself acts on.

Cost: one getSession on two administrator-only routes that already make one, and one insert per
accepted call.

Changelog

  • Under Unreleased: "The trail says when an identity provider was added, not only when one was
    taken away".

Proof

Run against the bug first. On main three of the five new tests fail with real assertions — the
registration row missing, the actor and provider missing from it, and a removal through the library's
endpoint missing. The two that pass are the ones asserting those routes stay administrator-only. All
five pass here.

The tests stand a stub in for the sign-in library, so the two things that stub could get wrong were
checked against the installed package rather than assumed. @better-auth/sso@1.7.1 declares
/sso/register and /sso/delete-provider — what the two keys name once mounted under /api/auth
and both are POST with providerId in a JSON body (delete-provider as
z.object({ providerId: z.string() })), which is what the clone-and-read depends on.

Not covered: no registration was driven through the real library end to end. /api/auth/*
answers 503 unless an identity provider is configured (server/src/app.ts:286-288), and configuring
one needs credentials from a real directory. Driving the admin screen on a live local deployment
confirms exactly that boundary: the form answers "No identity provider is configured", the request is
POST /api/auth/sso/register → 503, and no audit row and no sso_providers row appear. Wiring,
ordering and refusal behaviour are covered by tests; the library's 200 path is not.

Gates: typecheck clean across app, server and worker; lint 596 files, format 592, no diff.
drizzle-kit generate reports no schema changes — no migration in this PR.

Suite against 06633a4, with the two agent-handoff-*.integration files excluded because they fail
4-5 of their 5 tests on bare main and make any comparison meaningless: branch 2538 pass / 2 fail,
main 2533 / 2, each identical across three consecutive runs.
Same two failures on both sides,
neither belonging to this change. The delta is exactly this PR's one new file — 2560 tests across 212
files on main, 2565 across 213 here.

Closes #461

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.

An identity provider being registered leaves no audit row, though removing one does

1 participant