Record an identity provider being registered, not only removed - #462
Open
zopeVaibhav wants to merge 1 commit into
Open
Record an identity provider being registered, not only removed#462zopeVaibhav wants to merge 1 commit into
zopeVaibhav wants to merge 1 commit into
Conversation
zopeVaibhav
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
September 9, 2026 08:59
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.
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 thisdeployment already checks the caller is an administrator (
server/src/app.ts:280-283).identity_provider.registeredandidentity_provider.removedforPOST /api/auth/sso/registerand
POST /api/auth/sso/delete-provider. The first was declared in the vocabulary with no writerbehind it (
server/src/audit.ts:325).auth.handlerruns. The handler consumes the stream, so aclone taken afterwards is of a request whose body is gone and the row would name no provider.
one.
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 thepath 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 andremovedByreads wrongly ona registration. That leaves two shapes for one target type; happy to split into per-event keys if
uniformity is preferred.
update-provideris untouched. It has no event type declared, so recording it means addingvocabulary rather than adding a writer. Left out deliberately.
Where it runs
audit_eventstable, existing store.request's session. Nothing here claims shared work, so no coordination is needed.
audit_eventsis append-only and rows are independent.Boundary and audit
that had none.
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.
request-supplied value is
providerId, which is what the library itself acts on.Cost: one
getSessionon two administrator-only routes that already make one, and one insert peraccepted call.
Changelog
Unreleased: "The trail says when an identity provider was added, not only when one wastaken away".
Proof
Run against the bug first. On
mainthree of the five new tests fail with real assertions — theregistration 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.1declares/sso/registerand/sso/delete-provider— what the two keys name once mounted under/api/auth—and both are
POSTwithproviderIdin a JSON body (delete-providerasz.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 configuringone 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 nosso_providersrow 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 generatereports no schema changes — no migration in this PR.Suite against
06633a4, with the twoagent-handoff-*.integrationfiles excluded because they fail4-5 of their 5 tests on bare
mainand make any comparison meaningless: branch 2538 pass / 2 fail,main2533 / 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