Skip to content

Commit 7b735e4

Browse files
committed
fix(sso): revert a rejected SSO update instead of leaving it stored
1 parent 27ade42 commit 7b735e4

2 files changed

Lines changed: 48 additions & 25 deletions

File tree

apps/sim/app/api/auth/sso/register/route.test.ts

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -279,27 +279,41 @@ describe('POST /api/auth/sso/register', () => {
279279

280280
/**
281281
* The create path rolls the provider back when verification is revoked during the
282-
* write; the update path has no row to roll back, so it must instead strip the
283-
* trust flag. Leaving it set would re-authorize same-email account linking for a
284-
* domain the org no longer proves it owns.
282+
* write. The update path has no new row to delete, so it restores the pre-update
283+
* config and clears the trust flag together. Clearing alone would leave the
284+
* rejected config stored, and re-verifying the domain regrants trust
285+
* automatically — silently activating a config the caller was told had failed.
285286
*/
286-
it('revokes domain trust when verification is removed during an update', async () => {
287+
it('reverts the config and revokes trust when verification is removed mid-update', async () => {
287288
queueMembers([{ organizationId: 'org1', role: 'owner' }])
288289
resetDbChainMock()
289290
queueMembers([{ organizationId: 'org1', role: 'owner' }])
290291
queueTableRows(schemaMock.ssoDomain, [{ id: 'v' }]) // entry gate
291292
queueTableRows(schemaMock.ssoDomain, [{ id: 'v' }]) // pre-write re-check
292293
queueTableRows(schemaMock.ssoDomain, []) // locking read in the grant: proof gone
293294
queueProviders([])
294-
queueTableRows(schemaMock.ssoProvider, [{ id: 'p1' }]) // provider already owned → update path
295+
queueTableRows(schemaMock.ssoProvider, [
296+
{
297+
id: 'p1',
298+
issuer: 'https://old-issuer.example.com',
299+
domain: 'acme.com',
300+
oidcConfig: '{"stored":"oidc"}',
301+
samlConfig: null,
302+
},
303+
]) // provider already owned → update path
295304

296305
const res = await POST(request({ ...OIDC_BODY, orgId: 'org1' }))
297306
expect(res.status).toBe(403)
298307
expect(mockUpdateSSOProvider).toHaveBeenCalledTimes(1)
299-
// The conditional UPDATE is still issued — it simply matches no rows once the
300-
// proof is gone — so the signal is the explicit clear plus the 403, not the
301-
// absence of the grant statement.
302-
expect(dbChainMockFns.set).toHaveBeenCalledWith({ domainVerified: false })
308+
// The conditional grant UPDATE is still issued — it simply matches no rows once
309+
// the proof is gone — so the signal is the restoring write plus the 403.
310+
expect(dbChainMockFns.set).toHaveBeenCalledWith({
311+
issuer: 'https://old-issuer.example.com',
312+
domain: 'acme.com',
313+
oidcConfig: '{"stored":"oidc"}',
314+
samlConfig: null,
315+
domainVerified: false,
316+
})
303317
})
304318

305319
it('does not mark domain-verified when the registration is rolled back', async () => {

apps/sim/app/api/auth/sso/register/route.ts

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -614,22 +614,21 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
614614
eq(ssoProvider.userId, session.user.id),
615615
isNull(ssoProvider.organizationId)
616616
)
617+
// Config columns are captured, not just the id: an update whose trust grant is
618+
// refused has to be undone, or the rejected config stays stored and goes live
619+
// the moment the domain is verified again.
617620
const [existingOwnedProvider] = await db
618-
.select({ id: ssoProvider.id })
621+
.select({
622+
id: ssoProvider.id,
623+
issuer: ssoProvider.issuer,
624+
domain: ssoProvider.domain,
625+
oidcConfig: ssoProvider.oidcConfig,
626+
samlConfig: ssoProvider.samlConfig,
627+
})
619628
.from(ssoProvider)
620629
.where(ownerClause)
621630
.limit(1)
622631

623-
/**
624-
* Unconditional write of Better Auth's `domainVerified` flag, which Sim
625-
* mirrors from its own DNS proof. Used to withdraw trust; granting on an
626-
* org-scoped provider goes through {@link grantProviderDomainTrust}, which
627-
* re-tests ownership in the write itself.
628-
*/
629-
const setProviderDomainVerified = async (verified: boolean) => {
630-
await db.update(ssoProvider).set({ domainVerified: verified }).where(ownerClause)
631-
}
632-
633632
/**
634633
* Grants domain trust only while the proof is held under a row lock.
635634
*
@@ -644,7 +643,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
644643
*/
645644
const grantProviderDomainTrust = async (): Promise<boolean> => {
646645
if (!orgId) {
647-
await setProviderDomainVerified(!isHosted)
646+
await db.update(ssoProvider).set({ domainVerified: !isHosted }).where(ownerClause)
648647
return true
649648
}
650649
return db.transaction(async (tx) => {
@@ -683,11 +682,21 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
683682
headers,
684683
})
685684

686-
// Nothing to roll back on update, so clear the flag: `updateSSOProvider`
687-
// resets it only when the domain changes, leaving same-domain edits stale.
685+
// Restore the pre-update config and clear the flag together. Clearing alone
686+
// is not enough: re-verifying the domain now regrants trust automatically,
687+
// which would activate the very config this request reported as rejected.
688688
if (!(await grantProviderDomainTrust())) {
689-
await setProviderDomainVerified(false)
690-
logger.warn('Revoked SSO domain trust: verification was removed mid-update', {
689+
await db
690+
.update(ssoProvider)
691+
.set({
692+
issuer: existingOwnedProvider.issuer,
693+
domain: existingOwnedProvider.domain,
694+
oidcConfig: existingOwnedProvider.oidcConfig,
695+
samlConfig: existingOwnedProvider.samlConfig,
696+
domainVerified: false,
697+
})
698+
.where(eq(ssoProvider.id, existingOwnedProvider.id))
699+
logger.warn('Reverted SSO update: domain verification was removed mid-write', {
691700
domain,
692701
orgId,
693702
providerId,

0 commit comments

Comments
 (0)