#1755 follow-up

changelog.d/0-release-notes/pr-1763

@@ -0,0 +1 @@
+*Only if you are an early adopter of multi-team IdP issuers on release* [2021-09-14](https://github.com/wireapp/wire-server/releases/tag/v2021-09-14): that the [query parameter for IdP creation has changed](https://github.com/wireapp/wire-server/pull/1763/files#diff-bd66bf2f3a2445e08650535a431fc33cc1f6a9e0763c7afd9c9d3f2d67fac196).  This only affects future calls to this one end-point.

changelog.d/3-bug-fixes/pr-1763

@@ -0,0 +1 @@
+An attempt to create a 3rd IdP with the same issuer was triggering an exception.

changelog.d/3-bug-fixes/pr-1763-2

@@ -0,0 +1 @@
+When a user was auto-provisioned into two teams under the same pair of `Issuer` and `NameID`, they where directed into the wrong team, and not rejected.

changelog.d/4-docs/pr-1763

@@ -0,0 +1 @@
+Document how to use IdP issuers for multiple teams

changelog.d/5-internal/pr-1763

@@ -0,0 +1,6 @@
+Minor changes around SAML and multi-team Issuers.
+
+- Change query param to not contain `-`, but `_`.  (This is considered an internal change because the feature has been release in the last release, but only been documented in this one.)
+- Haddocks.
+- Simplify code.
+- Remove unnecessary calls to cassandra.

docs/reference/spar-braindump.md

@@ -326,3 +326,80 @@ TODO (probably little difference between this and "user deletes herself"?)
 #### delete via scim
 
 TODO
+
+
+## using the same IdP (same entityID, or Issuer) with different teams
+
+Some SAML IdP vendors do not allow to set up fresh entityIDs (issuers)
+for fresh apps; instead, all apps controlled by the IdP are receiving
+SAML credentials from the same issuer.
+
+In the past, wire has used the a tuple of IdP issuer and 'NameID'
+(Haskell type 'UserRef') to uniquely identity users (tables
+`spar.user_v2` and `spar.issuer_idp`).
+
+In order to allow one IdP to serve more than one team, this has been
+changed: we now allow to identity an IdP by a combination of
+entityID/issuer and wire `TeamId`.  The necessary tweaks to the
+protocol are listed here.
+
+For everybody using IdPs that do not have this limitation, we have
+taken great care to not change the behavior.
+
+
+### what you need to know when operating a team or an instance
+
+No instance-level configuration is required.
+
+If your IdP supports different entityID / issuer for different apps,
+you don't need to change anything.  We hope to deprecate the old
+flavor of the SAML protocol eventually, but we will keep you posted in
+the release notes, and give you time to react.
+
+If your IdP does not support different entityID / issuer for different
+apps, keep reading.  At the time of writing this section, there is no
+support for multi-team IdP issuers in team-settings, so you have two
+options: (1) use the rest API directly; or (2) contact our customer
+support and send them the link to this section.
+
+If you feel up to calling the rest API, try the following:
+
+- Use the above end-point `GET /sso/metadata/:tid` with your `TeamId`
+  for pulling the SP metadata.
+- When calling `POST /identity-provider`, make sure to add
+  `?api_version=v2`.  (`?api_version=v1` or no omission of the query
+  param both invoke the old behavior.)
+
+NB: Neither version of the API allows you to provision a user with the
+same Issuer and same NamdID.  RATIONALE: this allows us to implement
+'getSAMLUser' without adding 'TeamId' to 'UserRef', which in turn
+would break the (admittedly leaky) abstarctions of saml2-web-sso.
+
+
+### API changes in more detail
+
+- New query param `api_version=<v1|v2>` for `POST
+  /identity-providers`.  The version is stored in `spar.idp` together
+  with the rest of the IdP setup, and is used by `GET
+  /sso/initiate-login` (see below).
+- `GET /sso/initiate-login` sends audience based on api_version stored
+  in `spar.idp`: for v1, the audience is `/sso/finalize-login`; for
+  v2, it's `/sso/finalize-login/:tid`.
+- New end-point `POST /sso/finalize-login/:tid` that behaves
+  indistinguishable from `POST /sso/finalize-login`, except when more
+  than one IdP with the same issuer, but different teams are
+  registered.  In that case, this end-point can process the
+  credentials by discriminating on the `TeamId`.
+- `POST /sso/finalize-login/:tid` remains unchanged.
+- New end-point `GET /sso/metadata/:tid` returns the same SP metadata as
+  `GET /sso/metadata`, with the exception that it lists
+  `"/sso/finalize-login/:tid"` as the path of the
+  `AssertionConsumerService` (rather than `"/sso/finalize-login"` as
+  before).
+- `GET /sso/metadata` remains unchanged, and still returns the old SP
+  metadata, without the `TeamId` in the paths.
+
+
+### database schema changes
+
+[V15](https://github.com/wireapp/wire-server/blob/b97439756cfe0721164934db1f80658b60de1e5e/services/spar/schema/src/V15.hs#L29-L43)

libs/wire-api/src/Wire/API/Routes/Public/Spar.hs

@@ -166,7 +166,7 @@ type IdpGetAll = Get '[JSON] IdPList
 type IdpCreate =
   ReqBodyCustomError '[RawXML, JSON] "wai-error" IdPMetadataInfo
     :> QueryParam' '[Optional, Strict] "replaces" SAML.IdPId
-    :> QueryParam' '[Optional, Strict] "api-version" WireIdPAPIVersion
+    :> QueryParam' '[Optional, Strict] "api_version" WireIdPAPIVersion
     :> PostCreated '[JSON] IdP
 
 type IdpUpdate =

libs/wire-api/src/Wire/API/User/IdentityProvider.hs

@@ -65,6 +65,7 @@ data WireIdPAPIVersion
   deriving stock (Eq, Show, Enum, Bounded, Generic)
   deriving (Arbitrary) via (GenericUniform WireIdPAPIVersion)
 
+-- | (Internal issue for making v2 the default: https://wearezeta.atlassian.net/browse/SQSERVICES-781)
 defWireIdPAPIVersion :: WireIdPAPIVersion
 defWireIdPAPIVersion = WireIdPAPIV1
 

services/spar/src/Spar/API.hs

@@ -315,19 +315,23 @@ assertNoScimOrNoIdP teamid = do
       SparProvisioningMoreThanOneIdP
         "Teams with SCIM tokens can only have at most one IdP"
 
--- | Check that issuer is not used for any team in the system (it is a database keys for
--- finding IdPs), and request URI is https.
+-- | Check that issuer is not used anywhere in the system ('WireIdPAPIV1', here it is a
+-- database keys for finding IdPs), or anywhere in this team ('WireIdPAPIV2'), that request
+-- URI is https, that the replacement IdPId, if present, points to our team, and possibly
+-- other things (see source code for the definitive answer).
 --
 -- About the @mReplaces@ argument: the information whether the idp is replacing an old one is
 -- in query parameter, because the body can be both XML and JSON.  The JSON body could carry
 -- the replaced idp id fine, but the XML is defined in the SAML standard and cannot be
--- changed.
+-- changed.  NB: if you want to replace an IdP by one with the same issuer, you probably
+-- want to use `PUT` instead of `POST`.
 --
 -- FUTUREWORK: find out if anybody uses the XML body type and drop it if not.
 --
--- FUTUREWORK: using the same issuer for two teams may be possible, but only if we stop
--- supporting implicit user creating via SAML.  If unknown users present IdP credentials, the
--- issuer is our only way of finding the team in which the user must be created.
+-- FUTUREWORK: using the same issuer for two teams even in `WireIdPAPIV1` may be possible, but
+-- only if we stop supporting implicit user creating via SAML.  If unknown users present IdP
+-- credentials, the issuer is our only way of finding the team in which the user must be
+-- created.
 --
 -- FUTUREWORK: move this to the saml2-web-sso package.  (same probably goes for get, create,
 -- update, delete of idps.)
@@ -353,40 +357,25 @@ validateNewIdP apiversion _idpMetadata teamId mReplaces = withDebugLog "validate
   SAML.logger SAML.Debug $ show (apiversion, _idpMetadata, teamId, mReplaces)
   SAML.logger SAML.Debug $ show (_idpId, oldIssuers, idp)
 
-  let handleIdPClash :: Either SAML.IdPId IdP -> m ()
+  let handleIdPClash :: Either id idp -> m ()
+      -- (HINT: using type vars above instead of the actual types constitutes a proof that
+      -- we're not using any properties of the arguments in this function.)
       handleIdPClash = case apiversion of
         WireIdPAPIV1 -> const $ do
-          throwSpar $ SparNewIdPAlreadyInUse "you can't create an IdP with api-version v1 if the issuer is already in use on the wire instance."
+          throwSpar $ SparNewIdPAlreadyInUse "you can't create an IdP with api_version v1 if the issuer is already in use on the wire instance."
         WireIdPAPIV2 -> \case
-          (Right idp') -> do
-            guardSameTeam idp'
-            guardReplaceeV2
-          (Left id') -> do
-            idp' <- do
-              let err = throwSpar $ SparIdPNotFound (cs $ show id') -- database inconsistency
-              wrapMonadClient (Data.getIdPConfig id') >>= maybe err pure
-            handleIdPClash (Right idp')
-
-      guardSameTeam :: IdP -> m ()
-      guardSameTeam idp' = do
-        when ((idp' ^. SAML.idpExtraInfo . wiTeam) == teamId) $ do
-          throwSpar $ SparNewIdPAlreadyInUse "if the exisitng IdP is registered for a team, the new one can't have it."
-
-      guardReplaceeV2 :: m ()
-      guardReplaceeV2 = forM_ mReplaces $ \rid -> do
-        ridp <- do
-          let err = throwSpar $ SparIdPNotFound (cs $ show rid) -- database inconsistency
-          wrapMonadClient (Data.getIdPConfig rid) >>= maybe err pure
-        when (fromMaybe defWireIdPAPIVersion (ridp ^. SAML.idpExtraInfo . wiApiVersion) /= WireIdPAPIV2) $ do
-          throwSpar $
-            SparNewIdPAlreadyInUse
-              (cs $ "api-version mismatch: " <> show ((ridp ^. SAML.idpExtraInfo . wiApiVersion), WireIdPAPIV2))
+          (Right _) -> do
+            -- idp' was found by lookup with teamid, so it's in the same team.
+            throwSpar $ SparNewIdPAlreadyInUse "if the exisitng IdP is registered for a team, the new one can't have it."
+          (Left _) -> do
+            -- this idp *id* is from a different team, and we're in the 'WireIdPAPIV2' case, so this is fine.
+            pure ()
 
   case idp of
     Data.GetIdPFound idp' {- same team -} -> handleIdPClash (Right idp')
     Data.GetIdPNotFound -> pure ()
-    res@(Data.GetIdPDanglingId _) -> throwSpar . SparIdPNotFound . cs . show $ res -- database inconsistency
-    res@(Data.GetIdPNonUnique _) -> throwSpar . SparIdPNotFound . cs . show $ res -- impossible
+    res@(Data.GetIdPDanglingId _) -> throwSpar . SparIdPNotFound . ("validateNewIdP: " <>) . cs . show $ res -- database inconsistency
+    Data.GetIdPNonUnique ids' {- same team didn't yield anything, but there are at least two other teams with this issuer already -} -> handleIdPClash (Left ids')
     Data.GetIdPWrongTeam id' {- different team -} -> handleIdPClash (Left id')
 
   pure SAML.IdPConfig {..}
@@ -437,8 +426,8 @@ validateIdPUpdate zusr _idpMetadata _idpId = withDebugLog "validateNewIdP" (Just
         notInUseByOthers <- case foundConfig of
           Data.GetIdPFound c -> pure $ c ^. SAML.idpId == _idpId
           Data.GetIdPNotFound -> pure True
-          res@(Data.GetIdPDanglingId _) -> throwSpar . SparIdPNotFound . cs . show $ res -- impossible
-          res@(Data.GetIdPNonUnique _) -> throwSpar . SparIdPNotFound . cs . show $ res -- impossible (because team id was used in lookup)
+          res@(Data.GetIdPDanglingId _) -> throwSpar . SparIdPNotFound . ("validateIdPUpdate: " <>) . cs . show $ res -- impossible
+          res@(Data.GetIdPNonUnique _) -> throwSpar . SparIdPNotFound . ("validateIdPUpdate: " <>) . cs . show $ res -- impossible (because team id was used in lookup)
           Data.GetIdPWrongTeam _ -> pure False
         if notInUseByOthers
           then pure $ (previousIdP ^. SAML.idpExtraInfo) & wiOldIssuers %~ nub . (previousIssuer :)