diff --git a/changelog.d/0-release-notes/pr-1763 b/changelog.d/0-release-notes/pr-1763 new file mode 100644 index 0000000000..80f3c3a16b --- /dev/null +++ b/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. \ No newline at end of file diff --git a/changelog.d/3-bug-fixes/pr-1763 b/changelog.d/3-bug-fixes/pr-1763 new file mode 100644 index 0000000000..0fe1a26a91 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/changelog.d/3-bug-fixes/pr-1763-2 b/changelog.d/3-bug-fixes/pr-1763-2 new file mode 100644 index 0000000000..93dd86ef48 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/changelog.d/4-docs/pr-1763 b/changelog.d/4-docs/pr-1763 new file mode 100644 index 0000000000..9ad084f071 --- /dev/null +++ b/changelog.d/4-docs/pr-1763 @@ -0,0 +1 @@ +Document how to use IdP issuers for multiple teams diff --git a/changelog.d/5-internal/pr-1763 b/changelog.d/5-internal/pr-1763 new file mode 100644 index 0000000000..361a2a7424 --- /dev/null +++ b/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. diff --git a/docs/reference/spar-braindump.md b/docs/reference/spar-braindump.md index d6fe65a414..f92775beca 100644 --- a/docs/reference/spar-braindump.md +++ b/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=` 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) diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Spar.hs b/libs/wire-api/src/Wire/API/Routes/Public/Spar.hs index 5492a0be76..de7886cbf5 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Spar.hs +++ b/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 = diff --git a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs b/libs/wire-api/src/Wire/API/User/IdentityProvider.hs index f3d88cfd95..6cca0754de 100644 --- a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs +++ b/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 diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 9b5b6923de..19969954c5 100644 --- a/services/spar/src/Spar/API.hs +++ b/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 :) diff --git a/services/spar/src/Spar/App.hs b/services/spar/src/Spar/App.hs index 2e243d33d6..79e8ae0474 100644 --- a/services/spar/src/Spar/App.hs +++ b/services/spar/src/Spar/App.hs @@ -26,8 +26,9 @@ module Spar.App wrapMonadClientWithEnv, wrapMonadClient, verdictHandler, - getUserByUref, - getUserByScimExternalId, + GetUserResult (..), + getUserIdByUref, + getUserIdByScimExternalId, insertUser, validateEmailIfExists, errorPage, @@ -35,7 +36,7 @@ module Spar.App where import Bilge -import Brig.Types (ManagedBy (..), userTeam) +import Brig.Types (ManagedBy (..), User, userId, userTeam) import Brig.Types.Intra (AccountStatus (..), accountStatus, accountUser) import Cassandra import qualified Cassandra as Cas @@ -199,19 +200,39 @@ insertUser uref uid = wrapMonadClient $ Data.insertSAMLUser uref uid -- the team with valid SAML credentials. -- -- FUTUREWORK: Remove and reinstatate getUser, in AuthID refactoring PR. (in https://github.com/wireapp/wire-server/pull/1410, undo https://github.com/wireapp/wire-server/pull/1418) -getUserByUref :: SAML.UserRef -> Spar (Maybe UserId) -getUserByUref uref = do +getUserIdByUref :: Maybe TeamId -> SAML.UserRef -> Spar (GetUserResult UserId) +getUserIdByUref mbteam uref = userId <$$> getUserByUref mbteam uref + +getUserByUref :: Maybe TeamId -> SAML.UserRef -> Spar (GetUserResult User) +getUserByUref mbteam uref = do muid <- wrapMonadClient $ Data.getSAMLUser uref case muid of - Nothing -> pure Nothing + Nothing -> pure GetUserNotFound Just uid -> do let withpending = Intra.WithPendingInvitations -- see haddocks above - itis <- isJust <$> Intra.getBrigUserTeam withpending uid - pure $ if itis then Just uid else Nothing + Intra.getBrigUser withpending uid >>= \case + Nothing -> pure GetUserNotFound + Just user + | isNothing (userTeam user) -> pure GetUserNoTeam + | isJust mbteam && mbteam /= userTeam user -> pure GetUserWrongTeam + | otherwise -> pure $ GetUserFound user + +data GetUserResult usr + = GetUserFound usr + | GetUserNotFound + | GetUserNoTeam + | GetUserWrongTeam + deriving (Eq, Show) + +instance Functor GetUserResult where + fmap f (GetUserFound usr) = GetUserFound (f usr) + fmap _ GetUserNotFound = GetUserNotFound + fmap _ GetUserNoTeam = GetUserNoTeam + fmap _ GetUserWrongTeam = GetUserWrongTeam -- FUTUREWORK: Remove and reinstatate getUser, in AuthID refactoring PR -getUserByScimExternalId :: TeamId -> Email -> Spar (Maybe UserId) -getUserByScimExternalId tid email = do +getUserIdByScimExternalId :: TeamId -> Email -> Spar (Maybe UserId) +getUserIdByScimExternalId tid email = do muid <- wrapMonadClient $ (Data.lookupScimExternalId tid email) case muid of Nothing -> pure Nothing @@ -236,9 +257,8 @@ getUserByScimExternalId tid email = do -- FUTUREWORK: once we support , 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 +278,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. @@ -306,7 +326,7 @@ bindUser buid userref = do Data.GetIdPFound idp -> pure $ idp ^. idpExtraInfo . wiTeam Data.GetIdPNotFound -> err Data.GetIdPDanglingId _ -> err -- database inconsistency - Data.GetIdPNonUnique is -> throwSpar $ SparUserRefInMultipleTeams (cs $ show (buid, is)) + Data.GetIdPNonUnique is -> throwSpar $ SparUserRefInNoOrMultipleTeams (cs $ show (buid, is)) Data.GetIdPWrongTeam _ -> err -- impossible acc <- Intra.getBrigUserAccount Intra.WithPendingInvitations buid >>= maybe err pure teamid' :: TeamId <- userTeam (accountUser acc) & maybe err pure @@ -407,15 +427,15 @@ catchVerdictErrors = (`catchError` hndlr) -- | If a user attempts to login presenting a new IdP issuer, but there is no entry in -- @"spar.user"@ for her: lookup @"old_issuers"@ from @"spar.idp"@ for the new IdP, and -- traverse the old IdPs in search for the old entry. Return that old entry. -findUserWithOldIssuer :: Maybe TeamId -> SAML.UserRef -> Spar (Maybe (SAML.UserRef, UserId)) -findUserWithOldIssuer mbteam (SAML.UserRef issuer subject) = do +findUserIdWithOldIssuer :: Maybe TeamId -> SAML.UserRef -> Spar (GetUserResult (SAML.UserRef, UserId)) +findUserIdWithOldIssuer mbteam (SAML.UserRef issuer subject) = do idp <- getIdPConfigByIssuerOptionalSPId issuer mbteam - let tryFind :: Maybe (SAML.UserRef, UserId) -> Issuer -> Spar (Maybe (SAML.UserRef, UserId)) - tryFind found@(Just _) _ = pure found - tryFind Nothing oldIssuer = (uref,) <$$> getUserByUref uref + let tryFind :: GetUserResult (SAML.UserRef, UserId) -> Issuer -> Spar (GetUserResult (SAML.UserRef, UserId)) + tryFind found@(GetUserFound _) _ = pure found + tryFind _ oldIssuer = (uref,) <$$> getUserIdByUref mbteam uref where uref = SAML.UserRef oldIssuer subject - foldM tryFind Nothing (idp ^. idpExtraInfo . wiOldIssuers) + foldM tryFind GetUserNotFound (idp ^. idpExtraInfo . wiOldIssuers) -- | After a user has been found using 'findUserWithOldIssuer', update it everywhere so that -- the old IdP is not needed any more next time. @@ -432,35 +452,42 @@ verdictHandlerResultCore bindCky mbteam = \case SAML.AccessGranted userref -> do uid :: UserId <- do viaBindCookie <- maybe (pure Nothing) (wrapMonadClient . Data.lookupBindCookie) bindCky - viaSparCassandra <- getUserByUref userref + viaSparCassandra <- getUserIdByUref mbteam userref -- race conditions: if the user has been created on spar, but not on brig, 'getUser' -- returns 'Nothing'. this is ok assuming 'createUser', 'bindUser' (called below) are -- idempotent. viaSparCassandraOldIssuer <- - if isJust viaSparCassandra - then pure Nothing - else findUserWithOldIssuer mbteam userref + case viaSparCassandra of + GetUserFound _ -> pure GetUserNotFound + _ -> findUserIdWithOldIssuer mbteam userref + let err = + SparUserRefInNoOrMultipleTeams . cs $ + show (userref, viaBindCookie, viaSparCassandra, viaSparCassandraOldIssuer) case (viaBindCookie, viaSparCassandra, viaSparCassandraOldIssuer) of + (_, GetUserNoTeam, _) -> throwSpar err + (_, GetUserWrongTeam, _) -> throwSpar err + (_, _, GetUserNoTeam) -> throwSpar err + (_, _, GetUserWrongTeam) -> throwSpar err -- This is the first SSO authentication, so we auto-create a user. We know the user -- has not been created via SCIM because then we would've ended up in the -- "reauthentication" branch. - (Nothing, Nothing, Nothing) -> autoprovisionSamlUser mbteam userref + (Nothing, GetUserNotFound, GetUserNotFound) -> autoprovisionSamlUser mbteam userref -- If the user is only found under an old (previous) issuer, move it here. - (Nothing, Nothing, Just (oldUserRef, uid)) -> moveUserToNewIssuer oldUserRef userref uid >> pure uid + (Nothing, GetUserNotFound, GetUserFound (oldUserRef, uid)) -> moveUserToNewIssuer oldUserRef userref uid >> pure uid -- SSO re-authentication (the most common case). - (Nothing, Just uid, _) -> pure uid + (Nothing, GetUserFound uid, _) -> pure uid -- Bind existing user (non-SSO or SSO) to ssoid - (Just uid, Nothing, Nothing) -> bindUser uid userref - (Just uid, Just uid', Nothing) + (Just uid, GetUserNotFound, GetUserNotFound) -> bindUser uid userref + (Just uid, GetUserFound uid', GetUserNotFound) -- Redundant binding (no change to Brig or Spar) | uid == uid' -> pure uid -- Attempt to use ssoid for a second Wire user | otherwise -> throwSpar SparBindUserRefTaken -- same two cases as above, but between last login and bind there was an issuer update. - (Just uid, Nothing, Just (oldUserRef, uid')) + (Just uid, GetUserNotFound, GetUserFound (oldUserRef, uid')) | uid == uid' -> moveUserToNewIssuer oldUserRef userref uid >> pure uid | otherwise -> throwSpar SparBindUserRefTaken - (Just _, Just _, Just _) -> + (Just _, GetUserFound _, GetUserFound _) -> -- to see why, consider the condition on the call to 'findUserWithOldIssuer' above. error "impossible." SAML.logger SAML.Debug ("granting sso login for " <> show uid) diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index bbe5c2ca8c..d523149563 100644 --- a/services/spar/src/Spar/Data.hs +++ b/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 = ?" diff --git a/services/spar/src/Spar/Error.hs b/services/spar/src/Spar/Error.hs index ddbc57321c..fb96e017b7 100644 --- a/services/spar/src/Spar/Error.hs +++ b/services/spar/src/Spar/Error.hs @@ -84,7 +84,7 @@ data SparCustomError | SparBindFromWrongOrNoTeam LT | SparBindFromBadAccountStatus LT | SparBindUserRefTaken - | SparUserRefInMultipleTeams LT + | SparUserRefInNoOrMultipleTeams LT | SparBadUserName LT | SparCannotCreateUsersOnReplacedIdP LT | SparCouldNotParseRfcResponse LT LT @@ -141,7 +141,7 @@ renderSparError (SAML.CustomError (SparBadInitiateLoginQueryParams label)) = Rig renderSparError (SAML.CustomError (SparBindFromWrongOrNoTeam msg)) = Right $ Wai.mkError status403 "bad-team" ("Forbidden: wrong user team " <> msg) renderSparError (SAML.CustomError (SparBindFromBadAccountStatus msg)) = Right $ Wai.mkError status403 "bad-account-status" ("Forbidden: user has account status " <> msg <> "; only Active, PendingInvitation are supported") renderSparError (SAML.CustomError SparBindUserRefTaken) = Right $ Wai.mkError status403 "subject-id-taken" "Forbidden: SubjectID is used by another wire user. If you have an old user bound to this IdP, unbind or delete that user." -renderSparError (SAML.CustomError (SparUserRefInMultipleTeams msg)) = Right $ Wai.mkError status403 "bad-team" ("Forbidden: multiple teams for same UserRef " <> msg) +renderSparError (SAML.CustomError (SparUserRefInNoOrMultipleTeams msg)) = Right $ Wai.mkError status403 "bad-team" ("Forbidden: multiple teams or no team for same UserRef " <> msg) renderSparError (SAML.CustomError (SparBadUserName msg)) = Right $ Wai.mkError status400 "bad-username" ("Bad UserName in SAML response, except len [1, 128]: " <> msg) renderSparError (SAML.CustomError (SparCannotCreateUsersOnReplacedIdP replacingIdPId)) = Right $ Wai.mkError status400 "cannont-provision-on-replaced-idp" ("This IdP has been replaced, users can only be auto-provisioned on the replacing IdP " <> replacingIdPId) -- RFC-specific errors diff --git a/services/spar/src/Spar/Scim/User.hs b/services/spar/src/Spar/Scim/User.hs index ee3d938404..72a4e8b3fc 100644 --- a/services/spar/src/Spar/Scim/User.hs +++ b/services/spar/src/Spar/Scim/User.hs @@ -64,7 +64,7 @@ import qualified Data.UUID.V4 as UUID import Imports import Network.URI (URI, parseURI) import qualified SAML2.WebSSO as SAML -import Spar.App (Spar, getUserByScimExternalId, getUserByUref, sparCtxOpts, validateEmailIfExists, wrapMonadClient) +import Spar.App (GetUserResult (..), Spar, getUserIdByScimExternalId, getUserIdByUref, sparCtxOpts, validateEmailIfExists, wrapMonadClient) import qualified Spar.Data as Data import qualified Spar.Intra.Brig as Brig import Spar.Scim.Auth () @@ -633,9 +633,11 @@ calculateVersion uid usr = Scim.Weak (Text.pack (show h)) -- to a single `externalId`. assertExternalIdUnused :: TeamId -> ST.ValidExternalId -> Scim.ScimHandler Spar () assertExternalIdUnused tid veid = do - mExistingUserId <- lift $ ST.runValidExternalId (getUserByUref) (getUserByScimExternalId tid) veid - unless (isNothing mExistingUserId) $ - throwError Scim.conflict {Scim.detail = Just "externalId is already taken"} + assertExternalIdInAllowedValues + [Nothing] + "externalId is already taken" + tid + veid -- | -- Check that the UserRef is not taken any user other than the passed 'UserId' @@ -645,9 +647,28 @@ assertExternalIdUnused tid veid = do -- to a single `externalId`. assertExternalIdNotUsedElsewhere :: TeamId -> ST.ValidExternalId -> UserId -> Scim.ScimHandler Spar () assertExternalIdNotUsedElsewhere tid veid wireUserId = do - mExistingUserId <- lift $ ST.runValidExternalId getUserByUref (getUserByScimExternalId tid) veid - unless (mExistingUserId `elem` [Nothing, Just wireUserId]) $ do - throwError Scim.conflict {Scim.detail = Just "externalId already in use by another Wire user"} + assertExternalIdInAllowedValues + [Nothing, Just wireUserId] + "externalId already in use by another Wire user" + tid + veid + +assertExternalIdInAllowedValues :: [Maybe UserId] -> Text -> TeamId -> ST.ValidExternalId -> Scim.ScimHandler Spar () +assertExternalIdInAllowedValues allowedValues errmsg tid veid = do + isGood <- + lift $ + ST.runValidExternalId + ( \uref -> + getUserIdByUref (Just tid) uref <&> \case + (Spar.App.GetUserFound uid) -> Just uid `elem` allowedValues + Spar.App.GetUserNotFound -> Nothing `elem` allowedValues + Spar.App.GetUserNoTeam -> False -- this is never allowed (and also hopefully impossible) + Spar.App.GetUserWrongTeam -> False -- this can happen, but it's violating all our assertions + ) + (fmap (`elem` allowedValues) . getUserIdByScimExternalId tid) + veid + unless isGood $ + throwError Scim.conflict {Scim.detail = Just errmsg} assertHandleUnused :: Handle -> Scim.ScimHandler Spar () assertHandleUnused = assertHandleUnused' "userName is already taken" diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index e932ffa42d..0a26514b2a 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -235,6 +235,22 @@ specFinalizeLogin = do bdy `shouldContain` "wire:sso:success" bdy `shouldContain` "window.opener.postMessage({type: 'AUTH_SUCCESS'}, receiverOrigin)" hasPersistentCookieHeader sparresp `shouldBe` Right () + + let loginFailure :: HasCallStack => ResponseLBS -> TestSpar () + loginFailure sparresp = liftIO $ do + statusCode sparresp `shouldBe` 200 + let bdy = maybe "" (cs @LBS @String) (responseBody sparresp) + bdy `shouldContain` "" + bdy `shouldContain` "" + bdy `shouldNotContain` "wire:sso:error:success" + bdy `shouldContain` "wire:sso:error:bad-team" + bdy `shouldContain` "window.opener.postMessage({" + bdy `shouldContain` "\"type\":\"AUTH_ERROR\"" + bdy `shouldContain` "\"payload\":{" + bdy `shouldContain` "\"label\":\"forbidden\"" + bdy `shouldContain` "}, receiverOrigin)" + hasPersistentCookieHeader sparresp `shouldBe` Left "no set-cookie header" + context "happy flow" $ do it "responds with a very peculiar 'allowed' HTTP response" $ do env <- ask @@ -249,6 +265,7 @@ specFinalizeLogin = do liftIO $ authnreq ^. rqIssuer . fromIssuer . to URI.uriPath `shouldBe` audiencePath authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp spmeta authnreq True loginSuccess =<< submitAuthnResponse tid authnresp + context "happy flow (two teams, fixed IdP entityID)" $ do it "works" $ do skipIdPAPIVersions @@ -261,6 +278,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 +292,36 @@ 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 "idp sends user to two teams with same issuer, nameid" $ do + it "fails" $ do + skipIdPAPIVersions + [ WireIdPAPIV1 + -- (In fact, to get this to work was the reason to introduce 'WireIdPAPIVesion'.) + ] + env <- ask + (_, tid1, idp1, (IdPMetadataValue _ metadata, privcreds)) <- registerTestIdPWithMeta + (tid2, idp2) <- liftIO . runHttpT (env ^. teMgr) $ do + (owner2, tid2) <- createUserWithTeam (env ^. teBrig) (env ^. teGalley) + idp2 :: IdP <- callIdpCreate (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner2) metadata + pure (tid2, idp2) + subj <- liftIO $ SAML.unspecifiedNameID . UUID.toText <$> UUID.nextRandom + do + spmeta <- getTestSPMetadata tid1 + authnreq <- negotiateAuthnRequest idp1 + authnresp <- runSimpleSP $ mkAuthnResponseWithSubj subj privcreds idp1 spmeta authnreq True + loginSuccess =<< submitAuthnResponse tid1 authnresp + do + spmeta <- getTestSPMetadata tid2 + authnreq <- negotiateAuthnRequest idp2 + authnresp <- runSimpleSP $ mkAuthnResponseWithSubj subj privcreds idp2 spmeta authnreq True + loginFailure =<< submitAuthnResponse tid2 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 @@ -313,18 +364,22 @@ specFinalizeLogin = do authnreq <- negotiateAuthnRequest idp authnresp <- runSimpleSP $ mkAuthnResponseWithSubj subj privcreds idp spmeta authnreq True loginSuccess =<< submitAuthnResponse teamid authnresp + context "unknown user" $ do it "creates the user" $ do pending + context "known user A, but client device (probably a browser?) is already authenticated as another (probably non-sso) user B" $ do it "logs out user B, logs in user A" $ do pending -- TODO(arianvp): Ask Matthias what this even means + context "more than one dsig cert" $ do it "accepts the first of two certs for signatures" $ do pending it "accepts the second of two certs for signatures" $ do pending + context "unknown IdP Issuer" $ do it "rejects" $ do (_, teamid, idp, (_, privcreds)) <- registerTestIdPWithMeta diff --git a/services/spar/test-integration/Util/Core.hs b/services/spar/test-integration/Util/Core.hs index 03bda6dc4b..d876a3735c 100644 --- a/services/spar/test-integration/Util/Core.hs +++ b/services/spar/test-integration/Util/Core.hs @@ -1079,7 +1079,7 @@ 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 $ @@ -1087,8 +1087,8 @@ callIdpCreate' apiversion sparreq_ muid metadata = do . 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,7 +1117,7 @@ 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 $ @@ -1125,7 +1125,7 @@ callIdpCreateReplace' apiversion sparreq_ muid metadata idpid = do . 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" diff --git a/services/spar/test-integration/Util/Types.hs b/services/spar/test-integration/Util/Types.hs index 67b5631d62..be65f43344 100644 --- a/services/spar/test-integration/Util/Types.hs +++ b/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