#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/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 :)

services/spar/src/Spar/App.hs

@@ -236,9 +236,8 @@ getUserByScimExternalId tid email = do
 -- FUTUREWORK: once we support <https://github.com/wireapp/hscim scim>, brig will refuse to delete
 -- users that have an sso id, unless the request comes from spar.  then we can make users
 -- undeletable in the team admin page, and ask admins to go talk to their IdP system.
-createSamlUserWithId :: IdP -> UserId -> SAML.UserRef -> Spar ()
-createSamlUserWithId idp buid suid = do
-  let teamid = idp ^. idpExtraInfo . wiTeam
+createSamlUserWithId :: TeamId -> UserId -> SAML.UserRef -> Spar ()
+createSamlUserWithId teamid buid suid = do
   uname <- either (throwSpar . SparBadUserName . cs) pure $ Intra.mkUserName Nothing (UrefOnly suid)
   buid' <- Intra.createBrigUserSAML suid buid teamid uname ManagedByWire
   assert (buid == buid') $ pure ()
@@ -258,7 +257,7 @@ autoprovisionSamlUserWithId mbteam buid suid = do
   idp <- getIdPConfigByIssuerOptionalSPId (suid ^. uidTenant) mbteam
   guardReplacedIdP idp
   guardScimTokens idp
-  createSamlUserWithId idp buid suid
+  createSamlUserWithId (idp ^. idpExtraInfo . wiTeam) buid suid
   validateEmailIfExists buid suid
   where
     -- Replaced IdPs are not allowed to create new wire accounts.

services/spar/src/Spar/Data.hs

@@ -445,9 +445,9 @@ storeIdPConfig idp = retry x5 . batch $ do
     )
   addPrepQuery
     byIssuer
-    ( idp ^. SAML.idpId,
+    ( idp ^. SAML.idpMetadata . SAML.edIssuer,
       idp ^. SAML.idpExtraInfo . wiTeam,
-      idp ^. SAML.idpMetadata . SAML.edIssuer
+      idp ^. SAML.idpId
     )
   addPrepQuery
     byTeam
@@ -459,8 +459,8 @@ storeIdPConfig idp = retry x5 . batch $ do
     ins = "INSERT INTO idp (idp, issuer, request_uri, public_key, extra_public_keys, team, api_version, old_issuers, replaced_by) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)"
 
     -- FUTUREWORK: migrate `spar.issuer_idp` away, `spar.issuer_idp_v2` is enough.
-    byIssuer :: PrepQuery W (SAML.IdPId, TeamId, SAML.Issuer) ()
-    byIssuer = "INSERT INTO issuer_idp_v2 (idp, team, issuer) VALUES (?, ?, ?)"
+    byIssuer :: PrepQuery W (SAML.Issuer, TeamId, SAML.IdPId) ()
+    byIssuer = "INSERT INTO issuer_idp_v2 (issuer, team, idp) VALUES (?, ?, ?)"
 
     byTeam :: PrepQuery W (SAML.IdPId, TeamId) ()
     byTeam = "INSERT INTO team_idp (idp, team) VALUES (?, ?)"
@@ -593,20 +593,22 @@ getIdPIdByIssuerAllowOld issuer mbteam = do
               else mbv1v2
     _ -> pure mbv1v2
 
--- | Find 'IdPId' without team.  Search both `issuer_idp` and `issuer_idp_v2`; in the latter,
+-- | Find 'IdPId' without team.  Search both `issuer_idp_v2` and `issuer_idp`; in the former,
 -- make sure the result is unique (no two IdPs for two different teams).
 getIdPIdByIssuerWithoutTeam ::
   (HasCallStack, MonadClient m) =>
   SAML.Issuer ->
   m (GetIdPResult SAML.IdPId)
 getIdPIdByIssuerWithoutTeam issuer = do
-  (runIdentity <$$> retry x1 (query1 sel $ params Quorum (Identity issuer))) >>= \case
-    Just idpid -> pure $ GetIdPFound idpid
-    Nothing ->
-      (runIdentity <$$> retry x1 (query selv2 $ params Quorum (Identity issuer))) >>= \case
-        [] -> pure GetIdPNotFound
-        [idpid] -> pure $ GetIdPFound idpid
-        idpids@(_ : _ : _) -> pure $ GetIdPNonUnique idpids
+  (runIdentity <$$> retry x1 (query selv2 $ params Quorum (Identity issuer))) >>= \case
+    [] ->
+      (runIdentity <$$> retry x1 (query1 sel $ params Quorum (Identity issuer))) >>= \case
+        Just idpid -> pure $ GetIdPFound idpid
+        Nothing -> pure GetIdPNotFound
+    [idpid] ->
+      pure $ GetIdPFound idpid
+    idpids@(_ : _ : _) ->
+      pure $ GetIdPNonUnique idpids
   where
     sel :: PrepQuery R (Identity SAML.Issuer) (Identity SAML.IdPId)
     sel = "SELECT idp FROM issuer_idp WHERE issuer = ?"

services/spar/test-integration/Test/Spar/APISpec.hs

@@ -261,6 +261,10 @@ specFinalizeLogin = do
             (owner2, tid2) <- createUserWithTeam (env ^. teBrig) (env ^. teGalley)
             idp2 :: IdP <- callIdpCreate (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner2) metadata
             pure (tid2, idp2)
+          (tid3, idp3) <- liftIO . runHttpT (env ^. teMgr) $ do
+            (owner3, tid3) <- createUserWithTeam (env ^. teBrig) (env ^. teGalley)
+            idp3 :: IdP <- callIdpCreate (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner3) metadata
+            pure (tid3, idp3)
           do
             spmeta <- getTestSPMetadata tid1
             authnreq <- negotiateAuthnRequest idp1
@@ -271,6 +275,11 @@ specFinalizeLogin = do
             authnreq <- negotiateAuthnRequest idp2
             authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp2 spmeta authnreq True
             loginSuccess =<< submitAuthnResponse tid2 authnresp
+          do
+            spmeta <- getTestSPMetadata tid3
+            authnreq <- negotiateAuthnRequest idp3
+            authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp3 spmeta authnreq True
+            loginSuccess =<< submitAuthnResponse tid3 authnresp
       context "user is created once, then deleted in team settings, then can login again." $ do
         it "responds with 'allowed'" $ do
           (ownerid, teamid, idp, (_, privcreds)) <- registerTestIdPWithMeta

services/spar/test-integration/Util/Core.hs

@@ -1079,16 +1079,16 @@ callIdpCreate apiversion sparreq_ muid metadata = do
 callIdpCreate' :: (MonadIO m, MonadHttp m) => WireIdPAPIVersion -> SparReq -> Maybe UserId -> SAML.IdPMetadata -> m ResponseLBS
 callIdpCreate' apiversion sparreq_ muid metadata = do
   explicitQueryParam <- do
-    -- `&api-version=v1` is implicit and can be omitted from the query, but we want to test
+    -- `&api_version=v1` is implicit and can be omitted from the query, but we want to test
     -- both, and not spend extra time on it.
     liftIO $ randomRIO (True, False)
   post $
     sparreq_
       . maybe id zUser muid
       . path "/identity-providers/"
       . ( case apiversion of
-            WireIdPAPIV1 -> Bilge.query [("api-version", Just "v1") | explicitQueryParam]
-            WireIdPAPIV2 -> Bilge.query [("api-version", Just "v2")]
+            WireIdPAPIV1 -> Bilge.query [("api_version", Just "v1") | explicitQueryParam]
+            WireIdPAPIV2 -> Bilge.query [("api_version", Just "v2")]
         )
       . body (RequestBodyLBS . cs $ SAML.encode metadata)
       . header "Content-Type" "application/xml"
@@ -1117,15 +1117,15 @@ callIdpCreateReplace apiversion sparreq_ muid metadata idpid = do
 callIdpCreateReplace' :: (HasCallStack, MonadIO m, MonadHttp m) => WireIdPAPIVersion -> SparReq -> Maybe UserId -> IdPMetadata -> IdPId -> m ResponseLBS
 callIdpCreateReplace' apiversion sparreq_ muid metadata idpid = do
   explicitQueryParam <- do
-    -- `&api-version=v1` is implicit and can be omitted from the query, but we want to test
+    -- `&api_version=v1` is implicit and can be omitted from the query, but we want to test
     -- both, and not spend extra time on it.
     liftIO $ randomRIO (True, False)
   post $
     sparreq_
       . maybe id zUser muid
       . path "/identity-providers/"
       . Bilge.query
-        ( [ ( "api-version",
+        ( [ ( "api_version",
               case apiversion of
                 WireIdPAPIV1 -> if explicitQueryParam then Just "v1" else Nothing
                 WireIdPAPIV2 -> Just "v2"

services/spar/test-integration/Util/Types.hs

@@ -120,6 +120,16 @@ _unitTestTestErrorLabel = do
     throwIO . ErrorCall . show $
       val
 
+-- | FUTUREWORK(fisx): we're running all tests for all constructors of `WireIdPAPIVersion`,
+-- which sometimes makes little sense.  'skipIdPAPIVersions' can be used to pend individual
+-- tests that do not even work in both versions (most of them should), or ones that aren't
+-- that interesting to run twice (like if SAML is not involved at all).  A more scalable
+-- solution would be to pass the versions that a test should be run on as an argument to
+-- describe ('skipIdPAPIVersions' only works on individual leafs of the test tree, not on
+-- sub-trees), but that would be slightly (only slightly) more involved than I would like.
+-- so, some other time.  (Context: `make -C services/spar i` takes currently takes 3m22.476s
+-- on my laptop, including all the uninteresting tests.  So this is the maximum time
+-- improvement that we can get out of this.)
 skipIdPAPIVersions :: (MonadIO m, MonadReader TestEnv m) => [WireIdPAPIVersion] -> m ()
 skipIdPAPIVersions skip = do
   asks (^. teWireIdPAPIVersion) >>= \vers -> when (vers `elem` skip) . liftIO $ do