fix(auth): address SSO/SCIM review feedback#2933
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughThis PR tightens SSO enforcement and access controls, adds SCIM-driven role recomputation, reworks org-scoped SCIM user and group provisioning, updates SSO route cleanup and token audits, and changes the login and SSO admin UI to use dynamic redirects, async copy, and confirmation dialogs. ChangesBackend SSO/SCIM enforcement and role recomputation
UI SSO sign-in and connection management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LoginRequest
participant AuthConfig
participant AuthHandler
participant DB
LoginRequest->>AuthConfig: check email for enforced SSO
AuthConfig->>DB: load matching enforced providers and organizations
DB-->>AuthConfig: provider and organization rows
AuthConfig-->>LoginRequest: enforce only for active enterprise orgs
LoginRequest->>AuthHandler: request /auth/*
AuthHandler->>AuthHandler: block sensitive SSO plugin paths
AuthHandler-->>LoginRequest: 404 or delegate to apiAuth.handler
sequenceDiagram
participant SCIMClient
participant ScimRoute
participant SsoRolesLib
participant DB
SCIMClient->>ScimRoute: PUT or PATCH /Groups/:id
ScimRoute->>DB: check org-scoped displayName uniqueness
DB-->>ScimRoute: unique or conflict
ScimRoute->>DB: rename group when allowed
ScimRoute->>SsoRolesLib: recomputeRoleForGroupName(orgId, groupName)
SsoRolesLib->>DB: load matching groups and members
SsoRolesLib->>DB: update affected userOrganization roles
ScimRoute-->>SCIMClient: group response or 409
sequenceDiagram
participant User
participant SsoClient
participant AlertDialog
participant API
User->>SsoClient: click delete/rotate/revoke
SsoClient->>SsoClient: set confirmAction with run callback
SsoClient->>AlertDialog: render dialog
User->>AlertDialog: confirm
AlertDialog->>SsoClient: call confirmAction.run()
SsoClient->>API: execute delete/rotate/revoke
API-->>SsoClient: result
SsoClient->>SsoClient: clear confirmAction
Related issues: None found. Related PRs: None found. Suggested labels: security, backend, sso, scim Suggested reviewers: Not enough information to determine. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/api/src/lib/sso-roles.ts (1)
93-100: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winN+1 recomputation for large groups.
Each
recomputeUserRolecall issues several queries (membership, group memberships, groups, mappings). Iterating serially over every member (Line 98-100) makes this scale linearly in query count, which can be slow for large IdP-pushed groups. Consider batching the shared lookups (group→role mapping resolution) once per group and updating memberships in bulk, or at least resolving the org'sssoRoleMappingset a single time and passing it into per-user computation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/lib/sso-roles.ts` around lines 93 - 100, The recomputation loop in sso-roles.ts is doing per-user work with repeated shared lookups, which creates an N+1 pattern for large SCIM groups. Refactor the logic around the members/userIds loop to resolve the organization’s ssoRoleMapping/group-role data once, then reuse it while recomputing roles, or batch the membership updates so recomputeUserRole is not called serially for every user. Keep the fix centered on the member aggregation and recomputation flow in this module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/routes/scim.ts`:
- Around line 267-295: The SCIM search path in scim.ts is only checking one
arbitrary userOrganization row when emailFilter is present, so `userName eq` can
miss an existing user in email-only lookups. Update the lookup logic in the
membership query to constrain by email at the database level, or first resolve
the user by email and then verify organization membership before building the
SCIM response. Keep the fix centered around the existing
`db.query.userOrganization.findFirst` and `toScimUser` flow so the filter
returns the correct existing user instead of an empty result.
In `@apps/api/src/routes/sso.ts`:
- Around line 386-399: Normalize the SCIM token’s ssoProviderId before
persisting it so it always matches the provider slug used by the delete path.
Update generateScim in sso.ts to resolve the incoming identifier to the provider
slug (or otherwise store the canonical providerId value) before writing to
tables.scimToken, and keep the cleanup query using the same identifier shape so
the stale token is cleared when the provider is deleted.
In `@apps/ui/src/app/login/page.tsx`:
- Around line 219-225: Preserve the validated redirect target for SSO error
handling in the login flow. In `page.tsx` within the `signIn.sso` call, make
`errorCallbackURL` use the same validated `redirectTarget` as `callbackURL`
instead of always sending users to `/login`, so retries keep the intended
post-login destination.
---
Nitpick comments:
In `@apps/api/src/lib/sso-roles.ts`:
- Around line 93-100: The recomputation loop in sso-roles.ts is doing per-user
work with repeated shared lookups, which creates an N+1 pattern for large SCIM
groups. Refactor the logic around the members/userIds loop to resolve the
organization’s ssoRoleMapping/group-role data once, then reuse it while
recomputing roles, or batch the membership updates so recomputeUserRole is not
called serially for every user. Keep the fix centered on the member aggregation
and recomputation flow in this module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 715002dd-0283-440f-9b16-a72e0d91b3d8
📒 Files selected for processing (7)
apps/api/src/auth/config.tsapps/api/src/auth/handler.tsapps/api/src/lib/sso-roles.tsapps/api/src/routes/scim.tsapps/api/src/routes/sso.tsapps/ui/src/app/login/page.tsxapps/ui/src/components/sso/sso-client.tsx
steebchen
force-pushed
the
fix/sso-scim-review-feedback
branch
2 times, most recently
from
b716314 to
7bfc64f
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/api/src/routes/scim.ts (1)
306-334: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEmail-only
userName eqlookup still returns an arbitrary member, not the matching one.When
emailFilteris present butexternalIdFilteris not, the query constrains onlyorganizationId, sofindFirstreturns an arbitrary membership row; the email is then compared in JS against that single row. For any org with more than one member, a lookup for a non-first user resolves tonull, so the IdP concludes the user doesn't exist and retries a create against the 409 path.Since
user.emailis globally unique, resolve the user by email first, then verify org membership.This is the same defect raised previously and marked resolved, but the email-only branch still exhibits it.
🔧 Proposed fix for the email-only branch
if (emailFilter || externalIdFilter) { - const membership = await db.query.userOrganization.findFirst({ - where: { - organizationId: { eq: orgId }, - ...(externalIdFilter - ? { scimExternalId: { eq: externalIdFilter } } - : {}), - }, - columns: { scimExternalId: true }, - with: { user: { columns: { id: true, email: true, name: true } } }, - }); - const match = - membership?.user && - (!emailFilter || - membership.user.email.toLowerCase() === emailFilter.toLowerCase()) - ? membership - : null; + let match: + | { scimExternalId: string | null; user: ScimUserRow } + | null = null; + if (externalIdFilter) { + const membership = await db.query.userOrganization.findFirst({ + where: { + organizationId: { eq: orgId }, + scimExternalId: { eq: externalIdFilter }, + }, + columns: { scimExternalId: true }, + with: { user: { columns: { id: true, email: true, name: true } } }, + }); + if ( + membership?.user && + (!emailFilter || + membership.user.email.toLowerCase() === emailFilter.toLowerCase()) + ) { + match = { scimExternalId: membership.scimExternalId, user: membership.user }; + } + } else if (emailFilter) { + const user = await db.query.user.findFirst({ + where: { email: { eq: emailFilter.toLowerCase() } }, + columns: { id: true, email: true, name: true }, + }); + const membership = user ? await getMembership(user.id, orgId) : null; + if (user && membership) { + match = { scimExternalId: membership.scimExternalId, user }; + } + } const resources = - match && match.user + match ? [toScimUser(match.user, true, match.scimExternalId)] : [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/scim.ts` around lines 306 - 334, The email-only `userName eq` path in `scim.ts` is still selecting an arbitrary membership because `findFirst` only filters by `organizationId`, then checks `membership.user.email` in memory. Update the lookup logic in this branch to resolve the user by the unique email first, then confirm they belong to the org before building the SCIM response. Use the existing `emailFilter`, `externalIdFilter`, `db.query.userOrganization.findFirst`, and `toScimUser` flow to keep the fix localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@apps/api/src/routes/scim.ts`:
- Around line 306-334: The email-only `userName eq` path in `scim.ts` is still
selecting an arbitrary membership because `findFirst` only filters by
`organizationId`, then checks `membership.user.email` in memory. Update the
lookup logic in this branch to resolve the user by the unique email first, then
confirm they belong to the org before building the SCIM response. Use the
existing `emailFilter`, `externalIdFilter`,
`db.query.userOrganization.findFirst`, and `toScimUser` flow to keep the fix
localized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 15bd1ac1-4cfd-41b7-8bf0-f09b6ba7f775
📒 Files selected for processing (7)
apps/api/src/auth/config.tsapps/api/src/auth/handler.tsapps/api/src/lib/sso-roles.tsapps/api/src/routes/scim.tsapps/api/src/routes/sso.tsapps/ui/src/app/login/page.tsxapps/ui/src/components/sso/sso-client.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/api/src/auth/handler.ts
- apps/api/src/auth/config.ts
- apps/api/src/lib/sso-roles.ts
- apps/ui/src/app/login/page.tsx
- apps/ui/src/components/sso/sso-client.tsx
steebchen
force-pushed
the
fix/sso-scim-review-feedback
branch
2 times, most recently
from
e3a3a1b to
41e2b61
Compare
Follow-up to #2924. Implements the actionable review feedback from that PR: SCIM (apps/api/src/routes/scim.ts) - Scope GET/PUT/PATCH/DELETE /Users/:id and group membership to the token's org so a token can't read or mutate users outside its tenant. - Honor `active: false` on user create (staged/suspended assignments no longer get org membership). - Recompute roles when a user is reactivated via PUT/PATCH and when a group is renamed, so mapped admin/owner members aren't left as developer. - Guard against removing the org's last owner during deprovisioning. - Link the SSO provider account for pre-existing users, not just new ones. - Enforce displayName uniqueness on group rename (PUT/PATCH). - Push /Users and /Groups filtering + pagination into the database. SSO management (apps/api/src/routes/sso.ts) - Block plugin bypass: the raw Better Auth /auth/sso management endpoints are now rejected at the HTTP handler (they only require a session). - Prevent admins (non-owners) from creating owner role mappings. - Recompute affected members when a role mapping is added or removed. - Guard against a missing provider row after registration and clean up orphans. - Clear SCIM tokens' stale provider link when a connection is deleted. - Derive the SCIM token's provider link from the org's connection. - Audit SCIM revoke against the token id, not the org id. Auth enforcement (apps/api/src/auth/config.ts) - Gate SSO enforcement on SSO_ENABLED and on an active, enterprise org. - On a blocked non-SSO login, delete only that session, not all of the user's. UI (apps/ui) - Preserve the ?redirect= target and surface thrown errors on SSO sign-in. - Confirm before destructive SSO/SCIM actions; only toast on successful copy; show a loading state until the org plan resolves. Shared role-recompute logic extracted to apps/api/src/lib/sso-roles.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address CodeRabbit review on the follow-up PR: - SCIM `GET /Users?filter=userName eq "…"` resolved an arbitrary org member and compared its email, so in multi-member orgs an existing user could be missed and the IdP would retry against the 409 create path. Resolve the user by email first, then confirm org membership. externalId lookups keep their DB-level filter. - Login: carry the validated `?redirect=` target through `errorCallbackURL` so a failed SSO attempt returns to /login with the intended destination. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add handler.spec.ts asserting the raw Better Auth SSO management endpoints (/auth/sso/register, update-provider, delete-provider, get-provider, providers, request-domain-verification, verify-domain) return the 404 not_found block for both GET and POST (and with a query string), while the login-flow paths (/auth/sign-in/sso, /auth/sso/callback, /auth/sso/saml2/sp/metadata) pass through to Better Auth. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
steebchen
force-pushed
the
fix/sso-scim-review-feedback
branch
from
0aa5127 to
bba6be1
Compare
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.
Summary
Follow-up to #2924 (SAML SSO + SCIM), which merged with a large batch of automated review comments (Copilot, CodeRabbit, Codex). This PR works through each one, keeps the ones worth having, and implements them.
Security / correctness (SCIM —
apps/api/src/routes/scim.ts)GET/PUT/PATCH/DELETE /Users/:idand group-member adds now require the target user to belong to the token's org, so a token for one tenant can no longer read or mutate another tenant's users (was leakingemail/nameand allowing cross-org renames).active: falseon create. A staged/suspended Okta assignment no longer gets an active org membership (and thus SSO eligibility) until a later activation.PUT/PATCH active=trueand groupdisplayNamechanges now re-apply group→role mappings, so an admin/owner-group member isn't silently left asdeveloper.displayNameuniqueness enforced on rename (PUT/PATCH), not just create.GET /UsersandGET /Groupsinstead of loading the whole directory into memory and slicing.Security / correctness (SSO management —
apps/api/src/routes/sso.ts,apps/api/src/auth/handler.ts)/auth/sso/register|update-provider|delete-provider|…endpoints only require a session, so any authenticated user could sidestep the enterprise/admin-gated/ssowrapper. They're now rejected at the HTTP handler; our server-sideapiAuth.api.registerSSOProvidercall bypasses the handler and is unaffected.ownerrole mappings.registerSSOProviderand clean up an orphaned unassigned row on failure.ssoProviderIdwhen the connection is deleted; derive the token's provider link from the org's connection when not supplied.Auth enforcement (
apps/api/src/auth/config.ts)SSO_ENABLEDand on an active, enterprise org (a soft-deleted org can no longer be managed to turn enforcement off).UI (
apps/ui)?redirect=target for SSO sign-in and surface thrown errors via toast.Refactor
apps/api/src/lib/sso-roles.tsand reused by both routers.Deliberately skipped
PUT— theuserrow is shared across orgs by email; letting one org's SCIM rewrite the global email risks cross-tenant identity hijack.spMetadatafor Better Auth — it's.optional()in@better-auth/sso@1.6.18;issueralready serves as the SP entityID that matches what admins paste. Not a defect in this version.scim_token.ssoProviderId— cosmetic; already documented; a rename needs a migration and ref churn for no behavior change.Testing
pnpm turbo run build --filter=api --filter=ui,pnpm format,pnpm lintall pass.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes