Fix SAML SSO Enforcement Implementation #2926
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@steven-tey has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughReplaces per-domain SSO enforcement with a timestamp field ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin (Dashboard)
participant UI as SAML Settings UI
participant API as PATCH /api/workspaces/:idOrSlug
participant DB as Prisma (Project)
Admin->>UI: Toggle "Enforce SAML"
UI->>API: { enforceSAML: true|false }
API->>DB: Update Project.ssoEnforcedAt = now() or null
DB-->>API: Updated Project { ssoEnforcedAt }
API-->>UI: { ssoEnforcedAt }
UI-->>Admin: Reflect toggle and badge (domain text from workspace.ssoEmailDomain)
sequenceDiagram
autonumber
actor Admin as Admin
participant UI as SAML Settings UI
participant SAMLAPI as POST/DELETE /api/workspaces/:idOrSlug/saml
participant Sess as Session
participant DB as Prisma (Project)
rect rgba(200,255,200,0.12)
note over Admin,SAMLAPI: Create SAML connection
Admin->>UI: Connect SAML
UI->>SAMLAPI: POST (includes session)
SAMLAPI->>Sess: Read session.user.email
SAMLAPI->>DB: Update Project.ssoEmailDomain = domain(session.email)
SAMLAPI-->>UI: 201 Created
end
rect rgba(255,230,200,0.12)
note over Admin,SAMLAPI: Delete SAML connection
Admin->>UI: Remove SAML
UI->>SAMLAPI: DELETE
SAMLAPI->>DB: Set ssoEmailDomain = null, ssoEnforcedAt = null
SAMLAPI-->>UI: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/api/workspaces/[idOrSlug]/route.ts (1)
109-130: Verify SAML configuration before allowing enforcement.The code validates that SAML connections exist when
enforceSAMLis true (lines 117-129), which is good. However, there's no verification thatssoEmailDomainhas been set on the workspace. Since the email domain is now managed separately (set insaml/route.tsduring POST), there's a potential race condition or misconfiguration scenario:
- Admin creates SAML connection (sets
ssoEmailDomain)- Admin deletes SAML connection (clears
ssoEmailDomainandssoEnforcedAt)- Admin creates a new SAML connection with a different email domain
- Meanwhile, another admin could enable enforcement before step 3 completes
This could result in
ssoEnforcedAtbeing set whilessoEmailDomainis null or stale.Consider adding a check to verify
ssoEmailDomainis set:if (connections.length === 0) { throw new DubApiError({ code: "forbidden", message: "SAML SSO is not configured for this workspace.", }); } + + if (!workspace.ssoEmailDomain) { + throw new DubApiError({ + code: "forbidden", + message: "SAML SSO email domain is not configured for this workspace.", + }); + } }
🧹 Nitpick comments (5)
apps/web/scripts/migrations/backfill-saml-sso.ts (2)
36-41: Detect unique collisions before writingAvoid attempting updates that will violate unique(ssoEmailDomain). Deduplicate and log collisions.
Apply:
- workspacesToUpdate.push({ - id: workspace.id, - name: workspace.name, - ssoEmailDomain: emailDomain, - }); + workspacesToUpdate.push({ + id: workspace.id, + name: workspace.name, + ssoEmailDomain: emailDomain, + }); + + // Collapse by domain and flag collisions + const byDomain = new Map<string, { id: string; name: string }[]>(); + for (const w of workspacesToUpdate) { + const arr = byDomain.get(w.ssoEmailDomain) ?? []; + arr.push({ id: w.id, name: w.name }); + byDomain.set(w.ssoEmailDomain, arr); + } + for (const [domain, owners] of byDomain) { + if (owners.length > 1) { + console.warn( + `Collision on domain "${domain}" across ${owners.length} workspaces:`, + owners.map((o) => o.name).join(", "), + ); + } + }
45-56: Add an --apply switch and safe update loopProvide a dry-run by default. On --apply, update sequentially and handle unique violations gracefully.
Apply:
- // await Promise.allSettled( - // workspacesToUpdate.map((workspace) => - // prisma.project.update({ - // where: { - // id: workspace.id, - // }, - // data: { - // ssoEmailDomain: workspace.ssoEmailDomain, - // }, - // }), - // ), - // ); + if (process.argv.includes("--apply")) { + for (const workspace of workspacesToUpdate) { + try { + await prisma.project.update({ + where: { id: workspace.id }, + data: { ssoEmailDomain: workspace.ssoEmailDomain }, + }); + console.log( + `Updated ${workspace.name} (${workspace.id}) -> ${workspace.ssoEmailDomain}`, + ); + } catch (err: any) { + console.error( + `Failed to update ${workspace.name} (${workspace.id}): ${err?.code || err?.message}`, + ); + } + } + } else { + console.log("Dry-run. Pass --apply to write changes."); + }apps/web/lib/api/workspaces/is-saml-enforced-for-email-domain.ts (1)
9-9: Normalize domains with toLowerCase and simplify return
- Use locale-invariant lowercasing for email domains to avoid locale pitfalls and to be consistent across code.
- Simplify the final return.
Apply:
- const emailDomain = email.split("@")[1].toLocaleLowerCase(); + const emailDomain = email.split("@")[1]?.toLowerCase();- if (workspace?.ssoEnforcedAt) { - return true; - } - - return false; + return Boolean(workspace?.ssoEnforcedAt);Also applies to: 29-34
apps/web/lib/auth/options.ts (1)
430-441: Safer domain parsing and locale‑invariant comparisonUse optional chaining and toLowerCase to avoid locale issues; bail out if parsing fails.
Apply:
- const emailDomain = user.email.split("@")[1]; + const emailDomain = user.email?.split("@")[1]?.toLowerCase(); @@ - if ( - emailDomain.toLocaleLowerCase() !== - ssoEmailDomain.toLocaleLowerCase() - ) { + if (!emailDomain) { + return false; + } + if (emailDomain !== ssoEmailDomain.toLowerCase()) { return false; }apps/web/lib/zod/schemas/workspaces.ts (1)
125-127: Add descriptions for new fields; confirm public exposure is intendedAdd .describe for API docs clarity. Also confirm that exposing ssoEmailDomain on the base (public) schema is intended.
Apply:
- ssoEmailDomain: z.string().nullable(), - ssoEnforcedAt: z.date().nullable(), + ssoEmailDomain: z + .string() + .nullable() + .describe("Email domain associated with SAML SSO for this workspace."), + ssoEnforcedAt: z + .date() + .nullable() + .describe("Timestamp when SAML SSO enforcement was enabled."),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/app/api/workspaces/[idOrSlug]/route.ts(2 hunks)apps/web/app/api/workspaces/[idOrSlug]/saml/route.ts(3 hunks)apps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/security/saml.tsx(4 hunks)apps/web/lib/api/workspaces/is-saml-enforced-for-email-domain.ts(2 hunks)apps/web/lib/auth/options.ts(1 hunks)apps/web/lib/zod/schemas/workspaces.ts(1 hunks)apps/web/scripts/migrations/backfill-saml-sso.ts(1 hunks)packages/prisma/schema/workspace.prisma(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/security/saml.tsx (1)
apps/web/lib/openapi/workspaces/update-workspace.ts (1)
updateWorkspace(9-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
packages/prisma/schema/workspace.prisma (1)
44-52: Deprecate unused ssoEnabled; confirm ssoEmailDomain uniqueness
- ssoEnabled is never referenced—add a deprecation doc comment or remove it.
- ssoEmailDomain has a @unique constraint, disallowing multiple workspaces per domain; verify this business rule before backfilling or enabling SAML.
- ssoEnabled Boolean @default(false) // TODO: this is not used + /// @deprecated Not used; scheduled for removal. + ssoEnabled Boolean @default(false)apps/web/app/api/workspaces/[idOrSlug]/route.ts (2)
146-148: Setting ssoEnforcedAt looks correct.The conditional logic properly sets
ssoEnforcedAtto the current timestamp when enforcement is enabled and clears it when disabled. This aligns with the new timestamp-based enforcement model.
213-217: Only slug conflicts possible here; conflict message is accurate
Project has two unique fields (slug, inviteCode) but this route only updates slug, so a P2002 can only stem from slug.apps/web/app/api/workspaces/[idOrSlug]/saml/route.ts (1)
107-108: Correctly clearing both SAML fields on deletion.The DELETE handler properly clears both
ssoEmailDomainandssoEnforcedAtwhen removing a SAML connection, ensuring consistent state.apps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/security/saml.tsx (3)
23-23: Good: Using ssoEmailDomain from workspace context.Correctly retrieving
ssoEmailDomainfrom the workspace hook, which ensures it's available for display purposes even though enforcement is now tracked separately viassoEnforcedAt.
208-215: UI correctly uses ssoEnforcedAt for conditional rendering.The badge display is properly gated by
ssoEnforcedAtand shows thessoEmailDomainfrom the workspace context. This provides a clear indication of enforcement status and affected domain.
219-219: Switch state correctly reflects enforcement status.The switch correctly uses
ssoEnforcedAt !== nullto determine if SAML enforcement is active, aligning with the new timestamp-based approach.
apps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/security/saml.tsx
Show resolved
Hide resolved
apps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/security/saml.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/lib/auth/options.ts (1)
430-442: Guard against malformed email domains before lowercasingIf
user.emaillacks a second@segment,.split("@")[1]isundefined, so calling.toLocaleLowerCase()will throw and reject the SAML sign-in. Deriving the domain with.pop()?.trim()and short-circuiting when it’s missing makes this path resilient while still enforcing the match.- const emailDomain = user.email.split("@")[1]; + const emailDomain = user.email.split("@").pop()?.trim(); + + if (!emailDomain) { + return false; + } // ssoEmailDomain should be required for all SAML enabled workspace // this should not happen if (!ssoEmailDomain) { return false; } if ( - emailDomain.toLocaleLowerCase() !== - ssoEmailDomain.toLocaleLowerCase() + emailDomain.toLowerCase() !== + ssoEmailDomain.trim().toLowerCase() ) { return false; }
Summary by CodeRabbit
New Features
Bug Fixes
Chores