Conversation
The in-memory IdPConfigStore resembles some constraints that would have been enforced on a higher level: The IdP issuer is (depending on IdP API version) either unique per backend (V1) or per team (V2).
Filling a small test gap.
Document the constraints of V1 vs. V2. Otherwise, this would be hard to grasp by client devs.
supersven
changed the title
Wire.IdPSubsystem.Interpreter.IdPSubsystem.Interpreter.getSsoCodeByEmail finds IdP for SCIM user by domain test
zebot
added
the
ok-to-test
There was a problem hiding this comment.
Pull request overview
This PR addresses a flaky unit test around SSO IdP lookup by ensuring test data respects issuer-uniqueness constraints enforced near the IdP creation endpoint, and expands coverage/documentation of the IdP API version (v1/v2) uniqueness rules—especially for multi-ingress setups.
Changes:
- Tighten the unit property test precondition to ensure the target IdP issuer is unique among generated configs.
- Add/extend integration tests covering IdP issuer uniqueness constraints for IdP API v1 (global) vs v2 (per-team) in multi-ingress scenarios.
- Add Swagger/OpenAPI description text for the IdP create endpoint and add an integration API helper to create IdPs with
api_version=v1.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| libs/wire-subsystems/test/unit/Wire/IdPSubsystem/InterpreterSpec.hs | Adjusts property precondition to avoid non-unique issuers that can cause flaky lookups. |
| libs/wire-api/src/Wire/API/Routes/Public/Spar.hs | Documents issuer-uniqueness behavior for api_version on IdP creation in generated Swagger/OpenAPI. |
| integration/test/Test/Spar/MultiIngressIdp.hs | Adds multi-ingress integration tests to validate v1/v2 issuer uniqueness constraints and updates domain config fixtures. |
| integration/test/API/Spar.hs | Adds createIdpWithZHostV1 helper to exercise v1 IdP creation in integration tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
integration/test/API/Spar.hs
Outdated
| submit "POST" (req & maybe id zHost mbZHost) | ||
|
|
||
| createIdpWithZHostV1 :: (HasCallStack, MakesValue user) => user -> Maybe String -> SAML.IdPMetadata -> App Response | ||
| createIdpWithZHostV1 user mbZHost metadata = do |
There was a problem hiding this comment.
Parameterize createIdPWithZHost above? I was briefly confused by the lack of support for v2 here.
| -- As issuers correspond to realms in Keycloak and realms are return-URL | ||
| -- specific, this is probably fine for now. Otherwise, we would have to | ||
| -- redesign spar's database schema. E.g there would be a race-condition on the | ||
| -- `spar.issuer_idp_v2` table. |
There was a problem hiding this comment.
I don't understand this comment. Are you debating if we should lift the uniqueness constraints altogether? I don't know how that would work, SAML defines the issuer to be the primary identifier of an IdP, so it has to be unique within some scope.
But maybe I misunderstood?
There was a problem hiding this comment.
I was debating this topic in the multi-ingress context. So, one might raise the question Why one can't use the same IdP for the same team on several domains?
The answer is basically: We don't want to deal with the complexity of existing Cassandra tables, now. And, as IdPs have URLs in their configuration at IdP-side (e.g. at the Keycloak server), so this might be of little benefit, anyways.
| testMultiIngressSameIdPsSameIssuerDifferentDomains :: (HasCallStack) => App () | ||
| testMultiIngressSameIdPsSameIssuerDifferentDomains = do |
There was a problem hiding this comment.
| testMultiIngressSameIdPsSameIssuerDifferentDomains :: (HasCallStack) => App () | |
| testMultiIngressSameIdPsSameIssuerDifferentDomains = do | |
| -- | A variant of `testMultiIngressSameIdPsDifferentDomains` where the metadata is recreated at random for the calls to `POST /identity-providers` and only the issuer field is conflicting. | |
| testMultiIngressSameIdPsSameIssuerDifferentDomains :: (HasCallStack) => App () | |
| testMultiIngressSameIdPsSameIssuerDifferentDomains = do |
If that's the only difference though, I wonder if we really need testMultiIngressSameIdPsDifferentDomains? What extra insights does it give us?
There was a problem hiding this comment.
Yeah, that's indeed debatable 🤔
The
IdPConfigStorein-memory interpreter resembles some restrictions that are enforced on a higher level / closer to the endpoint. Relevant here is, that - depending on the IdP API version (not to be confused with the Wire API version 😈 ) - the issuer has to be either unique per backend (V1) or per team (V2).So, to de-flake the test, we have to make sure that the IdP to be looked up has a unique issuer.
Because I didn't find it very straight forward to figure out, I also:
Rendered Swagger:
Checklist
Add a new entry in an appropriate subdirectory ofchangelog.d