fix(sso): link Entra sign-ins to existing accounts and enforce unique provider IDs - #6311
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview The register route now rejects globally taken provider IDs with 409 and a suggested id, re-checks conflicts before write, grants trust under a Verified domain delete and verify APIs revoke or restore Enterprise SSO UI adds attribute mapping, OIDC advanced endpoints, SAML NameID format, SP entity ID, and clearer provider-id guidance; docs cover prerequisites, Entra steps, and the Security settings path. Reviewed by Cursor Bugbot for commit 7b735e4. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThe PR links SSO identities through verified-domain trust, enforces globally unique provider IDs, and coordinates trust revocation/restoration with domain lifecycle changes. It also expands SSO configuration and documentation.
Confidence Score: 4/5The PR is not yet safe to merge because a failed domain-trust grant can overwrite a newer successful SSO provider update. The rollback writes an old provider snapshot by row ID without a lock, version check, or comparison against this request's update, so an overlapping successful save can be silently replaced and have its trust cleared. Files Needing Attention: apps/sim/app/api/auth/sso/register/route.ts
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/auth/sso/register/route.ts | Adds provider collision handling and domain-trust grant/rollback logic, but the unguarded compensating rollback can overwrite a concurrent successful provider edit. |
| apps/sim/app/api/organizations/[id]/domains/[domainId]/route.ts | Deletes domain proof and revokes matching provider trust in one transaction with wildcard-compatible normalization. |
| apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts | Restores matching provider trust transactionally when pending domain verification succeeds. |
| packages/db/migrations/0284_sso_provider_domain_verified.sql | Adds the domain-verification flag and replaces the provider-ID index with a replay-aware global unique index. |
| apps/sim/ee/sso/components/sso-settings.tsx | Adds advanced OIDC endpoints, claim mappings, SAML NameID selection, and globally unique provider-ID guidance. |
Sequence Diagram
sequenceDiagram
participant A as Admin request A
participant B as Admin request B
participant P as SSO provider
participant D as Domain proof
A->>P: Read rollback snapshot
A->>P: Save configuration A
B->>P: Read current configuration
B->>P: Save configuration B
B->>D: Lock verified proof
B->>P: Grant domain trust
D-->>A: Proof unavailable
A->>P: Restore stale snapshot and clear trust
Note over P: Successful configuration B is lost
Reviews (24): Last reviewed commit: "fix(sso): revert a rejected SSO update i..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ca48eb5. Configure here.
… provider IDs Better Auth 1.6.23 calls the account-linking handler with trustProviderByName: false, which disables the trustedProviders allowlist for SSO entirely. Trust now comes only from the provider's domainVerified flag, which Sim never set — so any user who already had a Sim account was stranded on "account not linked". Entra never sends email_verified, so this hit every Microsoft tenant. Sim already proves domain ownership via sso_domain before a provider can be registered, so the register route mirrors that decision onto domainVerified. The column defaults to true so existing providers keep signing in across the deploy, since enabling the option turns sign-in into a hard gate. Also enforces the providerId uniqueness Better Auth already assumes: it rejects any id that exists in any tenant and resolves providers by that column alone, so a second customer picking "azure-ad" could not register at all and got an opaque 422. Sim now returns a 409 naming a free id, and a unique index makes the duplicate-row state unreachable.
The create path re-checks domain ownership after Better Auth persists the provider and rolls the row back if the verified sso_domain row disappeared in that window. The update path had no equivalent, so deleting the verified domain while updateSSOProvider was in flight still set domainVerified, restoring same-email account-linking trust for a domain the org no longer proves it owns. The update path has no newly-created row to roll back, so it clears the flag instead: that denies linking and blocks sign-in on the provider until the domain is verified again.
Greptile flagged that the ownership check and the domainVerified write were separate statements, so a domain deleted between them still ended with trust granted. Two changes close it from both sides. The grant now folds the ownership test into the UPDATE's WHERE clause, so Postgres evaluates both in one statement and the write matches nothing once the proof is gone. Removing a verified domain now clears domainVerified for providers on that domain, in the same transaction as the delete. This was a standing gap, not just a race: deleting a domain previously left linking trust set indefinitely. Together the provider cannot end up trusted without current ownership in either commit order — if the grant lands first the delete clears it, and if the delete lands first the grant no-ops.
ca48eb5 to
a0bd392
Compare
|
@cursor review |
…ccess The conditional grant could match zero rows if the verified domain was deleted between the pre-write check and the write. The route ignored that and returned 200, leaving a provider that cannot sign anyone in while telling the admin it saved. The grant now reports whether it matched, and that result is the single decision point on both paths: the create path rolls the provider back, the update path clears the flag, and both return SSO_DOMAIN_NOT_VERIFIED. This also drops the separate post-write ownership read, since the UPDATE re-tests ownership itself.
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 4dea2d4. Configure here.
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 4dea2d4. Configure here.
Identity providers disagree on which claim carries each value — Entra can send the address as `upn` rather than `email` — and the mapping was hardcoded, so a mismatch had no fix in the UI at all. Adds an Attribute mapping section for both protocols, defaulting to each protocol's standard claim names shown as placeholders, so the common case still needs no input. Editing an existing provider now loads its stored mapping and only treats a value as an override when it differs from the default, so a saved custom mapping is never silently rewritten.
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ee5fd2d. Configure here.
|
@cursor review |
|
@greptile — acknowledging the summary, and explaining why I am not changing this, since it is the sole reason for the 3/5 and it keeps recurring each round. Your description of the code is accurate. The org-less branch does set if (!orgId) {
await setProviderDomainVerified(!isHosted)
return true
}Two things narrow it to the point where it is not a merge blocker for this PR. On hosted it is inert, not merely unlikely. Org-less registration is not reachable from the product. The only caller is So the residual exposure is: a self-hosted instance, with more than one untrusted user, reached by a direct API call that bypasses the UI. That is real, and you are right that an org-less provider can never obtain DNS proof, because Scoring this 3/5 on a hosted-inert, UI-unreachable, self-host-only path understates a change whose actual effect is to restore account linking that has been broken in production since the better-auth 1.6.23 bump on 2026-07-22, and to make domain trust earned via DNS proof where it previously was not gated at all. Happy to be shown a concrete hosted path to the bypass if you have one — that would change my assessment. |
|
Your argument holds. I had the hosted case wrong — because The 3/5 was wrong. The PR restores production account linking that has been broken since the 1.6.23 bump, adds the |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9fff2d3. Configure here.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 27ade42. Configure here.
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7b735e4. Configure here.
1 participant
Summary
domainVerifiedon SSO providers so an SSO sign-in links to an existing same-email account. Better Auth 1.6.23 passestrustProviderByName: false, which disables thetrustedProvidersallowlist for SSO entirely — trust now comes only from that flag, which Sim never set. Entra never sendsemail_verified, so every Microsoft tenant hit "account not linked".sso_domainbefore a provider can be registered, so the register route mirrors that decision onto the flag.registerSSOProvideralways writesfalseandupdateSSOProviderresets it on a domain change, so it is re-applied after every write.true: enabling the option turns sign-in into a hard gate, so existing providers must already satisfy it or they'd be locked out mid-deploy.providerIduniqueness Better Auth already assumes. It rejects any id present in any tenant and resolves providers by that column alone, so a second customer pickingazure-adcould not register at all and got an opaque 422. Now returns a 409 naming a free id, plus a unique index.emailoptional claim, app assignment), and correct the settings path — it said Enterprise, the section is under Security.Type of Change
Testing
280 auth/org tests pass, including 6 new cases covering the providerId collision, the suggested replacement id, and the
domainVerifiedwrite on both create and update paths. Verified each new test fails when the fix is reverted. Typecheck, biome,check:migrations, andcheck:api-validationall pass.Migration is replay-safe: idempotent before the
COMMIT,IF NOT EXISTS/IF EXISTSon the concurrent index operations, and it builds the unique index before dropping the old one soprovider_idis never unindexed. It fails loudly inside the transaction if duplicateprovider_idvalues exist rather than stranding an INVALID index.Checklist