chore(sso): trim verbose comments

Author: waleedlatif1

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

@@ -41,12 +41,9 @@ function selectTokenEndpointAuthMethod(
 }
 
 /**
- * Proposes a free, tenant-scoped provider ID by suffixing the domain's first
- * label (`azure-ad` + `acme.com` -> `azure-ad-acme`), so a caller who hit the
- * global-uniqueness collision is handed something concrete to type rather than
- * being asked to invent a name. Callers pass a domain that already went through
- * `normalizeSSODomain`, whose `^[a-z0-9-]+(\.[a-z0-9-]+)+$` shape guarantees a
- * non-empty first label needing no further sanitizing.
+ * Proposes a free provider ID by suffixing the domain's first label
+ * (`azure-ad` + `acme.com` -> `azure-ad-acme`). Callers pass a domain already
+ * through `normalizeSSODomain`, whose shape guarantees a non-empty first label.
  */
 function suggestProviderId(providerId: string, domain: string): string {
   return `${providerId}-${domain.split('.')[0]}`
@@ -135,12 +132,11 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
       )
     }
 
-    // Security gate: configuring org SSO for a domain requires the org to have
-    // proven ownership of it (DNS TXT verification). Without this, the old
-    // first-come claim let any org wire another company's domain to their own
-    // IdP — an account-takeover primitive. Existing domains were grandfathered
-    // as verified by migration 0266, so live tenants are unaffected. Personal
-    // (org-less) SSO is not gated.
+    /**
+     * Configuring org SSO for a domain requires DNS-proven ownership; without it
+     * a first-come claim lets any org wire another company's domain to their own
+     * IdP. Migration 0266 grandfathered existing domains. Org-less SSO is not gated.
+     */
     const isOrgDomainVerified = async (): Promise<boolean> => {
       if (!orgId) return true
       const [verified] = await db
@@ -166,9 +162,8 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
         { status: 403 }
       )
 
-    // Fail fast before the expensive OIDC discovery. Re-checked immediately
-    // before the provider write below to close the TOCTOU window (the verified
-    // row could be removed while discovery is in flight).
+    // Fail fast before OIDC discovery; re-checked before the write to close the
+    // window where the proof is removed while discovery is in flight.
     if (!(await isOrgDomainVerified())) return domainNotVerifiedResponse()
 
     const isOwnedByCaller = (provider: {
@@ -200,12 +195,9 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
       )
 
     /**
-     * Better Auth treats `providerId` as globally unique, not per-tenant:
-     * `registerSSOProvider` rejects any id already present regardless of owner,
-     * and `checkProviderAccess` resolves providers by that column alone. Catch
-     * the cross-tenant collision here so the caller gets an actionable 409 that
-     * names a free id, instead of Better Auth's opaque 422 ("SSO provider with
-     * this providerId already exists") that gives no hint anything can be done.
+     * Better Auth treats `providerId` as globally unique, not per-tenant, and
+     * resolves providers by that column alone. Catching the cross-tenant
+     * collision here turns its opaque 422 into a 409 naming a free id.
      */
     const findProviderIdConflict = async () =>
       (
@@ -529,16 +521,13 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
       if (digestAlgorithm) samlConfig.digestAlgorithm = digestAlgorithm
 
       /**
-       * These two are always written, empty when unset, rather than omitted.
-       * Better Auth merges SAML config with `??`, so an omitted key silently keeps
-       * whatever was stored — clearing either field would never take effect. Both
-       * are falsy-guarded downstream: `createIdP` falls back to
-       * issuer/entryPoint/cert without metadata, and `createSP` omits nameIDFormat.
+       * Always written, empty when unset: Better Auth merges SAML config with
+       * `??`, so an omitted key keeps whatever was stored and clearing either
+       * field would never take effect. Both are falsy-guarded downstream.
        *
-       * Metadata in particular must not be generated here. Storing a document built
-       * from cert + entryPoint made re-saving destructive, because the form loads it
-       * back, resends it, and it then outranks the certificate — so rotating a SAML
-       * cert appeared to succeed and changed nothing.
+       * Metadata must not be generated here — a document built from cert +
+       * entryPoint outranks the certificate on re-save, silently defeating
+       * SAML cert rotation.
        */
       samlConfig.idpMetadata = { metadata: idpMetadata ?? '' }
       samlConfig.identifierFormat = identifierFormat ?? ''
@@ -632,12 +621,10 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
       .limit(1)
 
     /**
-     * Unconditional write of Better Auth's `domainVerified` flag — the value Sim
-     * mirrors from its own DNS proof, and what lets an SSO sign-in auto-link to an
-     * existing same-email account. Used to withdraw trust, and to set the org-less
-     * (personal) verdict, which {@link grantProviderDomainTrust} decides from the
-     * deployment rather than from a domain. Granting on an org-scoped provider goes
-     * through that same helper, which re-tests ownership in the write itself.
+     * Unconditional write of Better Auth's `domainVerified` flag, which Sim
+     * mirrors from its own DNS proof. Used to withdraw trust; granting on an
+     * org-scoped provider goes through {@link grantProviderDomainTrust}, which
+     * re-tests ownership in the write itself.
      */
     const setProviderDomainVerified = async (verified: boolean) => {
       await db.update(ssoProvider).set({ domainVerified: verified }).where(ownerClause)
@@ -646,19 +633,14 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
     /**
      * Grants domain trust only while the proof is held under a row lock.
      *
-     * Folding the ownership test into the UPDATE's WHERE clause is not sufficient:
-     * under READ COMMITTED the EXISTS subquery is evaluated against the statement's
-     * original snapshot, so a delete committing while the UPDATE waits on the
-     * provider row can still leave the subquery seeing the removed sso_domain row —
-     * granting trust after ownership is gone. Taking `FOR SHARE` on that row inside
-     * a transaction makes the two operations order properly: the delete's removal of
-     * sso_domain blocks until this commits, and if it committed first the SELECT
-     * finds nothing and no trust is written.
+     * A WHERE-clause EXISTS test is not enough: under READ COMMITTED the subquery
+     * sees the statement's original snapshot, so a delete committing while the
+     * UPDATE waits can still grant trust after ownership is gone. `FOR SHARE`
+     * orders the two — the delete blocks until this commits, and if it committed
+     * first the SELECT finds nothing.
      *
-     * Org-less (personal) SSO is a self-host-only path — Sim's UI always registers
-     * org-scoped. It has no verified domain behind it, so it is trusted only when
-     * self-hosted, where the operator is the sole tenant. On the hosted deployment
-     * that trust would let anyone claim a domain they do not own.
+     * Org-less SSO is self-host-only (Sim's UI always registers org-scoped) and
+     * has no proof behind it, so it is trusted only when self-hosted.
      */
     const grantProviderDomainTrust = async (): Promise<boolean> => {
       if (!orgId) {
@@ -701,9 +683,8 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
         headers,
       })
 
-      // No newly-created row to roll back here, so clear the flag instead:
-      // `updateSSOProvider` only resets it when the domain changes, so a
-      // same-domain edit would otherwise leave stale trust standing.
+      // Nothing to roll back on update, so clear the flag: `updateSSOProvider`
+      // resets it only when the domain changes, leaving same-domain edits stale.
       if (!(await grantProviderDomainTrust())) {
         await setProviderDomainVerified(false)
         logger.warn('Revoked SSO domain trust: verification was removed mid-update', {
@@ -729,12 +710,9 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
       headers,
     })
 
-    // A refused grant means the verified sso_domain row was removed between the
-    // pre-write check and Better Auth persisting the provider, leaving a provider
-    // on a domain the org no longer proves — roll it back. registerSSOProvider is
-    // create-only, so a successful call always created a brand-new row; we delete
-    // by its primary-key `id`, not the logical providerId, which a concurrent
-    // delete+recreate could point at a different row.
+    // A refused grant means the proof vanished mid-write, leaving a provider on a
+    // domain the org no longer proves — roll it back. Deleted by primary key, not
+    // providerId, which a concurrent delete+recreate could point at another row.
     if (!(await grantProviderDomainTrust())) {
       // registerSSOProvider spreads the created row's `id` at runtime, but the
       // typed return omits it — read it defensively and only delete when it's a

apps/sim/app/api/organizations/[id]/domains/[domainId]/route.ts

@@ -61,11 +61,8 @@ export const DELETE = withRouteHandler(
       )
     }
 
-    // Removing the proof must also withdraw the trust it granted. `domainVerified`
-    // on a provider is what authorizes auto-linking an SSO sign-in to an existing
-    // same-email account, so leaving it set would keep that authorization alive
-    // indefinitely after ownership was revoked. Both writes share a transaction so
-    // a domain can never be gone while its provider still claims to be verified.
+    // Removing the proof withdraws the trust it granted, in the same transaction
+    // so a domain can never be gone while its provider still claims verification.
     const removed = await db.transaction(async (tx) => {
       const [deleted] = await tx
         .delete(ssoDomain)
@@ -74,10 +71,8 @@ export const DELETE = withRouteHandler(
 
       if (!deleted) return null
 
-      // Match the provider domain the same way migration 0268 normalized it when it
-      // grandfathered these rows: lower, trimmed, leading `*.` stripped. A provider
-      // stored as `*.acme.com` against a verified row holding `acme.com` would
-      // otherwise keep its trust after the proof was deleted.
+      // Normalize as migration 0268 did when grandfathering these rows (lower,
+      // trimmed, leading `*.` stripped), so `*.acme.com` matches proof `acme.com`.
       await tx
         .update(ssoProvider)
         .set({ domainVerified: false })

apps/sim/app/api/organizations/[id]/domains/[domainId]/verify/route.ts

@@ -116,12 +116,10 @@ export const POST = withRouteHandler(
           )
           .returning()
 
-        // Restore trust on any provider this proof covers, mirroring the revocation
-        // performed when a verified domain is deleted. Without this, a delete followed
-        // by a re-verification leaves the provider untrusted — and because that flag
-        // gates sign-in, not just linking, the org would sit in a silent SSO outage
-        // until someone re-saved the SSO config. Wildcard-tolerant, matching the
-        // revoking comparison exactly so the two stay symmetric.
+        // Restore trust this proof covers, mirroring the revocation on delete.
+        // Without it a delete-then-reverify leaves the provider untrusted, and
+        // since that flag gates sign-in the org sits in a silent SSO outage. The
+        // comparison matches the revoking one exactly so the two stay symmetric.
         if (flipped.length > 0) {
           await tx
             .update(ssoProvider)

apps/sim/ee/sso/components/sso-settings.tsx

@@ -363,8 +363,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
       let wantAssertionsSigned = true
       let idpMetadata = ''
       // Blank means "use the protocol default", so only carry over a stored value
-      // that actually differs — otherwise editing would rewrite a default as an
-      // explicit override, and a stored custom mapping must never silently reset.
+      // that differs — otherwise editing rewrites a default as an explicit override.
       let mapping: { id?: string; email?: string; name?: string } = {}
       let identifierFormat = ''
       let authorizationEndpoint = ''
@@ -387,10 +386,9 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
         callbackUrl = config.callbackUrl || ''
         audience = config.audience || ''
         wantAssertionsSigned = config.wantAssertionsSigned ?? true
-        // Two stored shapes: `{ metadata }` from the route, and a bare string from
-        // older rows. Narrow on the type rather than truthiness — `{ metadata: '' }`
-        // is falsy at `.metadata` but truthy as an object, which would put an object
-        // into this string field and fail validation on the next save.
+        // Two stored shapes: `{ metadata }` from the route, a bare string from older
+        // rows. Narrow on type, not truthiness — `{ metadata: '' }` is falsy at
+        // `.metadata` but truthy as an object, putting an object in a string field.
         idpMetadata =
           typeof config.idpMetadata === 'string'
             ? config.idpMetadata
@@ -532,8 +530,7 @@ function OrganizationSsoSettings({ organizationId }: SSOProps) {
             dirty: hasChanges,
             saving: configureSSOMutation.isPending,
             // Never disabled on validation errors: showErrors is only set by
-            // handleSubmit, so a disabled Save left the admin with a greyed out
-            // button and no message. Clicking now reveals what is wrong.
+            // handleSubmit, so disabling Save left a greyed button and no message.
             saveLabel: isEditing ? 'Update' : 'Save',
             savingLabel: isEditing ? 'Updating...' : 'Saving...',
             onSave: () => void handleSubmit(),

apps/sim/lib/auth/auth.ts

@@ -109,15 +109,11 @@ const additionalTrustedOrigins = parseOriginList(env.TRUSTED_ORIGINS, (value) =>
 
 /**
  * Extra provider IDs appended to `trustedProviders`, from `SSO_PROVIDER_ID` and
- * `SSO_TRUSTED_PROVIDER_IDS`. Empty when SSO is disabled, so `trustedProviders`
- * is unchanged for non-SSO deployments.
+ * `SSO_TRUSTED_PROVIDER_IDS`. Empty when SSO is disabled.
  *
- * NOTE: these no longer affect SSO sign-in. Better Auth's SSO plugin calls the
- * account-linking handler with `trustProviderByName: false`, which disables the
- * `trustedProviders.includes(providerId)` branch entirely — SSO trust now comes
- * only from the provider's `domainVerified` flag (see the `sso()` config below).
- * They still apply to non-SSO providers that link by name, so the env vars are
- * kept rather than removed.
+ * These no longer affect SSO sign-in: the plugin passes `trustProviderByName:
+ * false`, disabling the name-based branch, so SSO trust comes only from
+ * `domainVerified`. Kept because non-SSO providers still link by name.
  */
 const additionalTrustedSsoProviders = isSsoEnabled
   ? [env.SSO_PROVIDER_ID, ...(env.SSO_TRUSTED_PROVIDER_IDS?.split(',') ?? [])]
@@ -1092,35 +1088,27 @@ export const auth = betterAuth({
       ? [
           sso({
             /**
-             * Honor the IdP's verified-email claim, so an SSO sign-in from an IdP
-             * that asserts `email_verified` produces a verified local account
-             * rather than one forced to `emailVerified: false`.
+             * Honor the IdP's `email_verified` claim so the local account is
+             * verified rather than forced to false.
              *
-             * This is NOT what enables account linking — many IdPs never send the
-             * claim at all (Microsoft Entra omits it, and Better Auth's own Entra
-             * provider defaults it to false), and the SAML path ignores it unless
-             * an explicit `mapping.emailVerified` is configured. `domainVerification`
-             * below is what actually establishes linking trust.
+             * This is not what enables linking — Entra omits the claim entirely,
+             * and SAML ignores it without an explicit `mapping.emailVerified`.
+             * `domainVerification` below establishes linking trust.
              */
             trustEmailVerified: true,
             /**
-             * Marks a provider as authoritative for its domain, which is what makes
-             * Better Auth auto-link an SSO sign-in to an existing same-email account.
-             * Without it `isTrustedProvider` is always false — the plugin passes
-             * `trustProviderByName: false`, so the `trustedProviders` allowlist never
-             * applies to SSO — and every user who already had a Sim account would be
-             * stranded on "account not linked".
+             * Marks a provider authoritative for its domain, which is what lets an
+             * SSO sign-in auto-link to an existing same-email account. Without it
+             * `isTrustedProvider` is always false and every user who already had a
+             * Sim account is stranded on "account not linked".
              *
-             * Sim does not use Better Auth's own DNS challenge endpoints: ownership is
-             * proven by Sim's `sso_domain` flow before a provider can be registered,
-             * and the register route mirrors that decision onto this flag.
+             * Sim does not use Better Auth's DNS challenge endpoints: ownership is
+             * proven by the `sso_domain` flow before registration, and the register
+             * route mirrors that decision onto this flag.
              *
-             * This path is constrained to emails whose domain matches the provider's
-             * (`validateEmailDomain`). It is NOT the only path: `link-account.mjs`
-             * blocks on `!isTrustedProvider && !userInfo.emailVerified`, so an IdP
-             * that asserts `email_verified` links regardless of domain — see the
-             * note on `trustEmailVerified` above. This flag narrows nothing on its
-             * own; it exists so linking survives IdPs that omit the claim.
+             * It narrows nothing on its own — an IdP asserting `email_verified`
+             * links regardless of domain (see `trustEmailVerified` above). It
+             * exists so linking survives IdPs that omit the claim.
              */
             domainVerification: { enabled: true },
             organizationProvisioning: {