Skip to content

Add multi-ingress domains to SCIM IdPs#4778

Merged
supersven merged 25 commits intodevelopfrom
sventennie/scim-idp-with-domain
Dec 4, 2025
Merged

Add multi-ingress domains to SCIM IdPs#4778
supersven merged 25 commits intodevelopfrom
sventennie/scim-idp-with-domain

Conversation

@supersven
Copy link
Contributor

@supersven supersven commented Sep 17, 2025 •
edited
Loading

Ticket: https://wearezeta.atlassian.net/browse/WPB-20052

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@supersven supersven force-pushed the sventennie/scim-idp-with-domain branch from 5b2b73f to 559a318 Compare September 17, 2025 13:20
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 17, 2025
@supersven supersven changed the title Sventennie/SCIM idp with domain Add multi-ingress domains to SCIM IdPs Sep 17, 2025
@supersven supersven force-pushed the sventennie/scim-idp-with-domain branch from 6a8da79 to c2457d1 Compare September 19, 2025 06:35
@supersven supersven marked this pull request as ready for review September 19, 2025 06:36
@supersven supersven requested review from a team as code owners September 19, 2025 06:36
@supersven supersven requested a review from fisx September 19, 2025 06:36
@supersven
Copy link
Contributor Author

supersven commented Sep 19, 2025

Hey @fisx 👋

May I ask you to review this? This PR touches sensitive areas and you surely have most knowledge/context here...

fisx
fisx reviewed Sep 25, 2025
View reviewed changes
changelog.d/2-features/multi-ingress-idp-domains
in later lookups, at most one IdP can be configured per multi-ingress domain.
If multi-ingress is not configured or it's not configured for the specific
domain, no `domain` field gets added to the IdP. This guards against creating
multiple IdPs and then assigning them to multi-ingress domains. Thus, users who
Copy link
Contributor

@fisx fisx Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean "guards against a team having no multi-ingress, but several idps that are only distinguished by domain (because domain is part of the database key)?

we do allow several idps per team, though, right? except there is some complication with scim and saml, and associating the two for one ipd, not sure what the current implementation status there is.

not sure this makes sense, but i'll leave it as a note to self so i can clarify later.

Copy link
Contributor Author

@supersven supersven Nov 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For your future self:

  • We allow several IdPs per team (v2 API, not in v1) in general
    • But, you cannot have the same IdP twice in one team.
  • For multi-ingress, we have to restrict that because we want to provide one SSO code for an email per domain. So, the mapping (domain, team) -> IdP has to be bijective.
    • Technically, this is implemented with assertions in the /identity-providers endpoints (not at database level).
  • multi-ingress is configured on domain level. If there's a domain that is not configured, the team may have many IdPs for it. If it is configured, there can only be one.

Sorry for the late answer. However, I hope it helps 😅

@supersven supersven force-pushed the sventennie/scim-idp-with-domain branch from 79ef8f4 to 533140c Compare November 27, 2025 10:36
@battermann battermann requested a review from Copilot November 28, 2025 07:30
battermann
battermann approved these changes Nov 28, 2025
View reviewed changes
Copy link
Contributor

@battermann battermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copilot AI reviewed Nov 28, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@supersven
Copy link
Contributor Author

supersven commented Nov 28, 2025

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@copilot please restart

Copilot AI mentioned this pull request Nov 28, 2025
Closed
2 tasks
Copy link
Contributor

Copilot AI commented Nov 28, 2025

@supersven I've opened a new pull request, #4880, to work on those changes. Once the pull request is ready, I'll request review from you.

supersven
supersven commented Nov 28, 2025
View reviewed changes
@supersven supersven force-pushed the sventennie/scim-idp-with-domain branch from c2b397d to d97317d Compare December 4, 2025 09:41
@supersven
Copy link
Contributor Author

supersven commented Dec 4, 2025

Rebased on latest develop - Just to be sure... (This PR was stale for quite some time.)

@supersven supersven merged commit c0c2b35 into develop Dec 4, 2025
10 of 12 checks passed
@supersven supersven deleted the sventennie/scim-idp-with-domain branch December 4, 2025 11:04
@zebot zebot mentioned this pull request Jan 13, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@battermann battermann battermann approved these changes

@fisx fisx fisx approved these changes

Assignees

No one assigned

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@supersven @battermann @fisx @zebot