fix(platform): Enterprise SSO + SCIM hardening#2118
Open
yannickmonney wants to merge 10 commits into
Open
Conversation
8 tasks
POST /scim/v2/Users resolved an existing user globally by email and reused that account — grafting a cross-tenant membership onto, and renaming, a user owned by another org. provisionUser now classifies ownership: a user owned only by another org is rejected as a 409 uniqueness conflict (coded ConvexError mapped in the SCIM HTTP layer), and the global user row is renamed only when this org already owns the membership. Also fixes a Group PATCH that combined an `add` with a value-less `remove` silently dropping the adds (the clear-all now composes with adds/removes), and converts the SCIM auth-gate throws to coded ConvexError so production no longer redacts them to a generic "Server Error". Refs #2036, #2085, #2049
OIDC/OAuth2 sign-in ignored the user's email and always used the first enabled connection, so on a multi-org deployment a user landed at the wrong org's IdP. The login page now resolves the org from the typed email via /api/sso/discover and threads the resolved organizationId through /authorize, the signed state and the callback (and ?org= for SAML) so the code is exchanged against the same connection. With no email it falls back to the single-connection flow. The trusted-headers authenticate endpoint reflected ?redirect= into navigation with no origin validation (open redirect). A shared sanitizeInternalRedirect util restricts it to a same-origin path, applied in the handler and defensively on the login-page forward. SSO config auth gates now throw coded ConvexError. Refs #2082, #2037, #2049
Switching a configured connection's protocol to OIDC with a blank client secret passed client validation, then the backend threw an untranslated raw Error. The secret requirement is now gated on whether an OIDC secret is actually stored (so a SAML->OIDC switch requires one), and the backend raises a coded, localized ConvexError instead. The SSO settings form's Select/Switch controls no longer mount uncontrolled (their values default), removing the controlled/uncontrolled console warnings. The standing 2FA grace / low-backup-code banners use a polite role="status" instead of assertive role="alert", and the password-expiry gate matches the /forced-change-password/$id route by inclusion so it stops re-navigating to it. Refs #2057, #2095, #2085
Convert the client-facing raw `throw new Error(...)` sites in the users (password), task-metrics, tasks (assignee), and conversations modules to coded ConvexError so production no longer redacts them to a generic "Server Error". getProjectTaskMetrics now mirrors getProject and returns null for a missing or forbidden project (the metrics page already renders the empty state). Adds a normalizeAssignee regression test asserting the coded ConvexError. Completes the #2049 sweep (the SSO/SCIM auth gates landed earlier). Refs #2049
Adds a role-mapping-rules editor to the Enterprise SSO settings so the "auto-assign roles from the IdP" toggle is actually functional — map IdP groups, app roles, job titles, or a claim path to a platform role; the editor seeds from and saves into the connection's provisioning policy. The /2fa-enroll wall now navigates away when the user has no reason to be there (already enrolled, or 2FA isn't enforced), matching the forced-change-password guard. Disabling 2FA on the Account page now warns when the org enforces it. Completes the remaining #2085 items ([04] enroll guard, [12] role mapping, [19] enforced-disable warning; [05]/[06]/[13] landed earlier). Refs #2085
The #2085[04] guard navigated away whenever 2FA was not enforced, which bounced users off /2fa-enroll even when they came to enroll voluntarily, and could interrupt an in-progress enrollment (verifyTotp flips twoFactorEnabled before the backup codes are shown). Restrict the auto-exit to an already-enrolled user still on the initial password step, so the standalone enrollment route stays usable. Caught by auth-account.spec. Refs #2085
The Entra ID adapter's getUserInfo() reads the signed-in user from Microsoft Graph /me, which requires the User.Read delegated permission. When an org's configured scopes omitted it (e.g. only GroupMember.Read.All), the access token came back without Graph user access and the userinfo call 403'd, breaking the whole OIDC/Entra sign-in after a successful token exchange. buildAuthorizeUrl now always ensures https://graph.microsoft.com/User.Read is requested, and the settings form's default Entra scope set lists it explicitly. Verified live against a real Entra tenant over a tunnel: sign-in now completes through /me (200) to a provisioned session.
…ct owner A SCIM HTTP DELETE was mapped to a soft-deactivate (PATCH active:false): it returned 204 but the user stayed retrievable via GET (200) and kept appearing in GET /Users. RFC 7644 §3.6 requires a deleted resource to no longer be returned, and it left User DELETE inconsistent with Group DELETE (which truly removes). Add deprovisionUser: it removes the org membership, its mirror, and the provisioning link so a subsequent GET/PATCH returns 404, symmetric with deleteGroup. The global Better Auth user row is preserved (the person may belong to other orgs; a SCIM token never mutates an account it doesn't own — #2036). The IdP's usual de-provisioning signal, PATCH active:false, still soft- deactivates (membership kept, role restorable) — that path is unchanged. The sole owner is never removed (that would orphan the org): DELETE on the owner returns 403 (scimType "mutability"). New pure classifyDeprovision() covers the 404 / owner-protected / remove decision; verified live against a real Entra tenant (DELETE 204 then GET 404; owner DELETE 403).
Pure whitespace reformat (a short operations array collapsed to one line) so the repo-wide oxfmt --check passes under the CI toolchain version. No workflow semantics or version field changed. Surfaced by merging main, where oxfmt 0.44.0 flags the file the committed formatting predates.
yannickmonney
force-pushed
the
fix/enterprise-sso-scim-hardening
branch
from
d8e136e to
822a11c
Compare
…ch login "Auto-assign roles from the IdP" only set a member's role at first provision: findOrCreateSsoUser computed the mapped role but, for an existing membership, discarded it and returned. So an IdP promotion/demotion (e.g. adding a user to an admin group) never propagated — the role stuck at whatever the user's first login produced, a correctness and security gap (stale elevated roles). handle_sso_login now passes syncRole = autoProvisionRole; when set, findOrCreateSsoUser re-applies the mapped role to an existing membership. The org owner is never touched (would orphan the org) and no-op writes are skipped (new pure shouldSyncMemberRole, unit-tested). Found live against a real Entra tenant: a user in an "All Company" group with an "All Company -> admin" rule stayed `member` across logins; with the fix the re-login promotes them to `admin`. Group->team sync verified in the same run (teams created from the user's Entra groups).
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
1 participant
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
Consolidated, complete fix for every open Enterprise SSO + SCIM issue tracked by Epic #2116, plus the broader error-handling sweep they pulled in. Validated end-to-end against a real Microsoft Entra ID tenant over a cloudflared tunnel (OIDC sign-in with SMS MFA + full SCIM provisioning) — that live run found and fixed two further defects. Full report on Epic #2116.
Fixed
POST /Userscross-tenant graft:provisionUserclassifies ownership; another tenant's account is rejected as a 409 uniqueness conflict (codedConvexError); the globaluser.nameis rewritten only when this org owns the membership. Validated live (cross-tenant → 409, new email → 201)./api/trusted-headers/authenticate: sharedsanitizeInternalRedirect(root-relative same-origin only), applied in the handler + login forward. Unit tests./api/sso/discoverand threadsorganizationIdthrough/authorize→ signedstate→ callback. Validated live (login_hint+organizationIdin signed state).ConvexError.throw new Erroracross auth/SSO/tasks/conversations/project-metrics) #2049 — ConvexError sweep, complete. SSO config + SCIM auth gates, users-password, task-metrics, tasks-assignee, conversations — every client-facing rawthrowis now a codedConvexError(SCIM 409 surfaces coded live)./2fa-enrollparity guard, [05] polite banners, [06] password-expiry gate, [12] functional role-mapping-rules editor (validated live — renders + persists a rule), [13] SCIM Group PATCHadd+ value-lessremovecompose (validated live), [19] enforced-2FA disable warning.userName eqfilter, deactivate/reactivate, de-provision, and the negative paths (401/404/409).Found & fixed during the live run
User.Readfor Entra userinfo (c2bf8ea84) — userinfo/me403'd when the org's scopes omittedUser.Read; the authorize request now always includes it. OIDC sign-in completes live.DELETEde-provisions (RFC 7644 §3.6) (d08946ec4) —DELETEreturned 204 but left the user GET-able; it now removes the membership/mirror/link (GET → 404), keeps Azure'sactive:falsesoft-disable path intact, and protects the org owner (403). NewclassifyDeprovision()unit tests; validated live.Definition of Done
typecheck,lint(oxlint),oxfmt --check,knipclassifyDeprovision+ i18n parity)en/de/frGroupMember.Read.Allconsent; available on request.Closes #2036, #2037, #2049, #2057, #2082, #2085, #2095
Closes #1921
Part of #2116