[codex] Fix IdP-initiated SSO org provisioning#1913
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralized SSO provisioning: added a frontend Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Browser
participant Guard as Auth Guard
participant Service as provisionSsoUser (frontend)
participant API as Backend /private/sso/provision-user
participant DB as Database
Client->>Guard: Navigate with session
Guard->>Guard: Detect SSO user
Guard->>Service: provisionSsoUser(session)
Service->>API: POST /private/sso/provision-user (Authorization: Bearer token)
API->>DB: Lookup provider, org, org_users
alt invite_* membership exists
API->>DB: Update user_right (promote invite_* → non-invite)
else no membership
API->>DB: Insert org_users (fallback role), retry on duplicate
end
API-->>Service: { success, merged?, already_member? }
Service-->>Guard: { merged, alreadyMember, error }
alt merged == true
Guard->>Client: signOut and redirect to /login?message=sso_account_linked
else error != null
Guard->>Client: abort navigation (next(false))
else
Guard->>Client: continue navigation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/sso.test.ts (1)
739-926: Please add the negative merge-case regression too.This suite now covers merge success, but not the new branch that must fail the merge when the org/provider link cannot be guaranteed. A test where the duplicate SSO user shares the email but no active provider can be resolved for the domain would lock in the hardening added in
supabase/functions/_backend/private/sso/provision-user.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sso.test.ts` around lines 739 - 926, Add a negative test mirroring the successful merge case that verifies provision-user refuses to merge when there is no active SSO provider linked for the email domain: create original password user and duplicate SSO auth user (same email), but do NOT insert an active sso_provider for the domain (or insert with status !== 'active' or different org_id), then call fetchWithRetry on '/private/sso/provision-user' using the duplicateAuthHeaders and assert response.status === 200 and responseBody.success === true but responseBody.merged === false; also assert the duplicate auth user still exists via getSupabaseClient().auth.admin.getUserById(duplicateUser.user.id) and that org_users membership was not transferred to originalUser; use the existing helpers and DB updates (getSupabaseClient(), pool queries, identity manipulation, provision-user fetch) to mirror the successful test flow but expect no merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/auth.ts`:
- Around line 100-104: The guard currently logs result.error and then returns
'continue', which allows subsequent onboarding logic (invoked after the
/private/sso/provision-user call) to run; change the early error path so
provisioning failures short-circuit the guard instead of falling through: inside
the auth guard where result.error is checked, log the error and return a
non-'continue' outcome (or throw) to stop further processing (ensure you update
the branch handling that inspects the guard's return value if needed);
reference: the result.error check and the guard's current return 'continue' plus
the /private/sso/provision-user provisioning flow.
---
Nitpick comments:
In `@tests/sso.test.ts`:
- Around line 739-926: Add a negative test mirroring the successful merge case
that verifies provision-user refuses to merge when there is no active SSO
provider linked for the email domain: create original password user and
duplicate SSO auth user (same email), but do NOT insert an active sso_provider
for the domain (or insert with status !== 'active' or different org_id), then
call fetchWithRetry on '/private/sso/provision-user' using the
duplicateAuthHeaders and assert response.status === 200 and responseBody.success
=== true but responseBody.merged === false; also assert the duplicate auth user
still exists via
getSupabaseClient().auth.admin.getUserById(duplicateUser.user.id) and that
org_users membership was not transferred to originalUser; use the existing
helpers and DB updates (getSupabaseClient(), pool queries, identity
manipulation, provision-user fetch) to mirror the successful test flow but
expect no merge.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02221fe4-257e-4b87-ac74-db80a9173fa0
📒 Files selected for processing (6)
src/composables/useSSOProvisioning.tssrc/modules/auth.tssrc/services/ssoProvisioning.tssupabase/functions/_backend/private/sso/provision-user.tstests/auth-sso-provisioning.unit.test.tstests/sso.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50107ccbfc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the auth-guard review note in 1a3b435: provisioning errors now abort navigation instead of falling through to org onboarding, and the unit test suite covers that branch. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/auth-sso-provisioning.unit.test.ts (1)
167-257: Add coverage for the merged and already-authenticated SSO branches.This suite only drives the
!hadAuthpath, and the success case never returnsmerged: true. That leaves the two highest-risk branches from this change untested: the merged-account sign-out redirect and thehasAuth && main.authreprovision flow atsrc/modules/auth.tsLines 305-317.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth-sso-provisioning.unit.test.ts` around lines 167 - 257, Tests miss coverage for the merged-account and already-authenticated SSO branches in the auth guard: add unit tests that simulate the provisioning response including { merged: true } to assert the sign-out/redirect behavior and a test where the session indicates hadAuth/hasAuth (the "main.auth" reprovision flow referenced in src/modules/auth.ts around the provisioning logic) to assert reprovisioning is attempted and navigation proceeds correctly; use getGuard to create the guard, stub mockGetSession to return a session with app_metadata showing SSO, stub fetch to return ok:true with merged:true for the merged-account case (and verify the expected sign-out redirect/next calls), and for the hadAuth case stub session with prior auth and fetch to return success to verify reprovisioning and continued navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/auth.ts`:
- Around line 95-97: The code currently calls supabase.auth.signOut() when
result.merged is true and immediately returns 'redirect_login' even if signOut
fails; wrap the signOut call in a try/catch (or check its error return) inside
the same merged branch, and only return 'redirect_login' when signOut completes
successfully; on error, log the failure (including the error object) and return
a distinct outcome (e.g., 'signout_failed' or propagate the error) so the caller
can avoid redirecting with a stale SSO session; update the branch around
result.merged and supabase.auth.signOut() accordingly.
---
Nitpick comments:
In `@tests/auth-sso-provisioning.unit.test.ts`:
- Around line 167-257: Tests miss coverage for the merged-account and
already-authenticated SSO branches in the auth guard: add unit tests that
simulate the provisioning response including { merged: true } to assert the
sign-out/redirect behavior and a test where the session indicates
hadAuth/hasAuth (the "main.auth" reprovision flow referenced in
src/modules/auth.ts around the provisioning logic) to assert reprovisioning is
attempted and navigation proceeds correctly; use getGuard to create the guard,
stub mockGetSession to return a session with app_metadata showing SSO, stub
fetch to return ok:true with merged:true for the merged-account case (and verify
the expected sign-out redirect/next calls), and for the hadAuth case stub
session with prior auth and fetch to return success to verify reprovisioning and
continued navigation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 406bd95e-c189-4282-80aa-97b51f6ee3d9
📒 Files selected for processing (2)
src/modules/auth.tstests/auth-sso-provisioning.unit.test.ts
|
Addressed the merged-session sign-out edge case in 4a318b1: the auth guard now aborts navigation if sign-out fails, and the unit tests cover that path. |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a318b1b71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .from('org_users') | ||
| .select('id, user_right') | ||
| .eq('user_id', userId) | ||
| .eq('org_id', orgId) | ||
| .maybeSingle() |
There was a problem hiding this comment.
ensureOrgMembership() assumes (user_id, org_id) is unique by calling .maybeSingle(), but org_users supports scoped rows (app_id/channel_id) and can legitimately have multiple records for the same user/org. In that case PostgREST returns a multiple-rows error, this function throws membership_check_failed, and the new merge path now returns provision_failed before identity transfer, blocking SSO account merges for affected users. Please scope this query to the org-level row (for example app_id IS NULL and channel_id IS NULL) or otherwise handle multiple rows deterministically.
Useful? React with 👍 / 👎.
1 participant
Summary (AI generated)
/sso-callbackand the auth guard/private/sso/provision-userso invite-only memberships are promoted to real memberships and merge flows fail when the org link cannot be guaranteedMotivation (AI generated)
Some customers entering through IdP-initiated SSO were bypassing the
/sso-callbackpage, so the org-linking call never ran. The auth guard then saw no selectable org and redirected the user to create an organization even though their managed SSO org already existed. The backend also treatedinvite_*org memberships as completed membership and could silently continue a merge flow without proving the canonical account was linked to the managed org.Business Impact (AI generated)
This removes a broken first-login path for SSO customers, which reduces onboarding friction for enterprise users and lowers support load on managed SSO setups. It also makes SSO account linking safer by preventing silent partial merges that would strand users without access to their organization.
Test Plan (AI generated)
bunx eslint src/services/ssoProvisioning.ts src/composables/useSSOProvisioning.ts src/modules/auth.ts tests/auth-sso-provisioning.unit.test.ts tests/sso.test.ts supabase/functions/_backend/private/sso/provision-user.tsbunx vitest run tests/auth-sso-provisioning.unit.test.tsbun run typecheckbun run supabase:with-env -- bunx vitest run tests/sso.test.tsGenerated with AI
Summary by CodeRabbit
New Features
Bug Fixes
Tests