multi-ingress: cross-IdP SSO#5212
Open
supersven wants to merge 4 commits into
Open
Conversation
zebot
added
the
ok-to-test
supersven
force-pushed
the
sventennie/multi-ingress-sso_cross_idp
branch
3 times, most recently
from
70c63ab to
63c5b7d
Compare
supersven
force-pushed
the
sventennie/multi-ingress-sso_cross_idp
branch
3 times, most recently
from
c70220c to
e7f3bf2
Compare
supersven
changed the title
Allow users to login with all team IdPs. This is required because there's usually one IdP per ingress in a team and users should be able to login on all ingresses.
supersven
force-pushed
the
sventennie/multi-ingress-sso_cross_idp
branch
from
e7f3bf2 to
d27326c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for multi-ingress “cross-IdP” SSO logins in Spar by falling back to a team-wide lookup (by email NameID) when the primary (issuer, NameID) lookup fails, and updates /sso/get-by-email discovery to work for any SSO user (not only SCIM-managed).
Changes:
- Extend Spar SAML finalize-login handling to support cross-IdP migration in multi-ingress mode (and add a dedicated config error for non-email NameIDs).
- Relax
/sso/get-by-emaillookup condition from “SCIM+SSO” to “SSO user”. - Add integration and unit tests plus documentation/changelog updates covering the new behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| services/spar/test-integration/Test/Spar/AppSpec.hs | Updates integration test helper to call updated verdictHandler signature with SAML config. |
| services/spar/src/Spar/Error.hs | Adds and renders a new error for invalid multi-ingress cross-IdP configuration (non-email NameID). |
| services/spar/src/Spar/App.hs | Implements multi-ingress cross-IdP fallback flow + structured logging changes. |
| services/spar/src/Spar/API.hs | Plumbs SAML config/host info into finalize-login handling and updates logger constraints. |
| libs/wire-subsystems/test/unit/Wire/IdPSubsystem/InterpreterSpec.hs | Adjusts unit test expectation to “non SSO user” behavior for get-by-email. |
| libs/wire-subsystems/src/Wire/IdPSubsystem/Interpreter.hs | Changes /sso/get-by-email eligibility check from SCIM+SSO to SSO. |
| integration/test/Test/Spar/MultiIngressCrossIdpSso.hs | Adds new integration test suite for cross-IdP migration and related error cases. |
| integration/integration.cabal | Registers new integration test module. |
| docs/src/developer/reference/config-options.md | Documents cross-IdP fallback algorithm and updates get-by-email criteria. |
| changelog.d/2-features/multi-ingress-cross-IdP-SSO | Adds changelog entry describing cross-IdP SSO behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
supersven
commented
May 12, 2026
Comment on lines
+124
to
+128
| -- This used to check if the user is SCIM AND SSO! The RFC ("2025-05-12 | ||
| -- RFC: Default SSO flow for team by host domain") is not really | ||
| -- unambiguous about this. The customer currently provisions non-SCIM. | ||
| isSsoUser :: User -> Bool | ||
| isSsoUser = isJust . userSSOId |
Contributor
Author
There was a problem hiding this comment.
This might be subject to change in case the customer decides to go for SCIM. Then, we could make this condition stricter (requiring SSO AND SCIM).
fisx
reviewed
May 13, 2026
Contributor
fisx
left a comment
fisx
left a comment
There was a problem hiding this comment.
I read the docs and have some questions, more feedback coming up!
Comment on lines
+1522
to
+1524
| `(issuer, NameID)` lookup finds nothing. Using a shared static issuer across | ||
| all domains is not an option because each external identity provider has its | ||
| own issuer URI that spar cannot control. |
Contributor
There was a problem hiding this comment.
I'm confused by this sentence. How about:
Suggested change
| `(issuer, NameID)` lookup finds nothing. Using a shared static issuer across | |
| all domains is not an option because each external identity provider has its | |
| own issuer URI that spar cannot control. | |
| `(issuer, NameID)` lookup finds nothing. Two IdPs can't have the same Issuer ID because those must be globally unique. Sharing the IdP for all multi-ingress domains is not an option because that would reveal the connection between the domains to the attacker. |
If yours is more correct, I want to understand how.
Comment on lines
+1529
to
+1530
| 1. **NameID must be an email address.** Username-based `NameID`s are rejected | ||
| to avoid ambiguity across authenticating IdPs. |
Contributor
There was a problem hiding this comment.
[nit] admins need to manually guarantee that email addresses are unique, so why can't they do the same thing here?
Comment on lines
+1533
to
+1535
| assertion's issuer and the incoming `Z-Host` header (exact match). If no exact | ||
| match is found and the team has exactly one IdP configuration, that one is used | ||
| unconditionally (no issuer or domain check). If neither condition is met, the |
Contributor
There was a problem hiding this comment.
Why pick a non-matching IdP, and not just reject right away?
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.
I've covered this usecase in the documentation. To avoid duplication, please look at the docs in this PR.
Ticket: https://wearezeta.atlassian.net/browse/WPB-25161
Security notes:
/sso/get-by-emailfrom SCIM SSO to SSO userFurther reading:
Checklist
changelog.d