From 1c2d9e5a11a39121c4abe7bbed2e0836755e8817 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 8 Sep 2021 13:12:37 +0200 Subject: [PATCH 01/52] Allow two teams with same IdP (entityID). --- .../src/Wire/API/Routes/Public/Spar.hs | 26 ++++++++++--- services/spar/schema/src/Main.hs | 4 +- services/spar/schema/src/V15.hs | 38 +++++++++++++++++++ services/spar/spar.cabal | 3 +- services/spar/src/Spar/API.hs | 14 ++++--- services/spar/src/Spar/App.hs | 5 ++- services/spar/src/Spar/Data.hs | 36 ++++++++++++++---- .../test-integration/Test/Spar/APISpec.hs | 17 ++++++++- 8 files changed, 120 insertions(+), 23 deletions(-) create mode 100644 services/spar/schema/src/V15.hs 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 ad54f2aa87..5e9a110e91 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Spar.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Spar.hs @@ -53,8 +53,10 @@ type API = type APISSO = "metadata" :> SAML.APIMeta + :<|> "metadata" :> Capture "team" TeamId :> SAML.APIMeta :<|> "initiate-login" :> APIAuthReqPrecheck :<|> "initiate-login" :> APIAuthReq + :<|> APIAuthRespLegacy :<|> APIAuthResp :<|> "settings" :> SsoSettingsGet @@ -131,8 +133,16 @@ data DoInitiate = DoInitiateLogin | DoInitiateBind type WithSetBindCookie = Headers '[Servant.Header "Set-Cookie" SetBindCookie] +type APIAuthRespLegacy = + "finalize-login" + :> Header "Cookie" ST + -- (SAML.APIAuthResp from here on, except for response) + :> MultipartForm Mem SAML.AuthnResponseBody + :> Post '[PlainText] Void + type APIAuthResp = "finalize-login" + :> Capture "team" TeamId :> Header "Cookie" ST -- (SAML.APIAuthResp from here on, except for response) :> MultipartForm Mem SAML.AuthnResponseBody @@ -176,11 +186,17 @@ type APIINTERNAL = :<|> "teams" :> Capture "team" TeamId :> DeleteNoContent :<|> "sso" :> "settings" :> ReqBody '[JSON] SsoSettings :> Put '[JSON] NoContent -sparSPIssuer :: SAML.HasConfig m => m SAML.Issuer -sparSPIssuer = SAML.Issuer <$> SAML.getSsoURI (Proxy @APISSO) (Proxy @APIAuthResp) - -sparResponseURI :: SAML.HasConfig m => m URI.URI -sparResponseURI = SAML.getSsoURI (Proxy @APISSO) (Proxy @APIAuthResp) +sparSPIssuer :: SAML.HasConfig m => Maybe TeamId -> m SAML.Issuer +sparSPIssuer Nothing = + SAML.Issuer <$> SAML.getSsoURI (Proxy @APISSO) (Proxy @APIAuthRespLegacy) +sparSPIssuer (Just tid) = + SAML.Issuer <$> SAML.getSsoURI' (Proxy @APISSO) (Proxy @APIAuthResp) tid + +sparResponseURI :: SAML.HasConfig m => Maybe TeamId -> m URI.URI +sparResponseURI Nothing = + SAML.getSsoURI (Proxy @APISSO) (Proxy @APIAuthRespLegacy) +sparResponseURI (Just tid) = + SAML.getSsoURI' (Proxy @APISSO) (Proxy @APIAuthResp) tid -- SCIM diff --git a/services/spar/schema/src/Main.hs b/services/spar/schema/src/Main.hs index d039eb3154..4084b5c10a 100644 --- a/services/spar/schema/src/Main.hs +++ b/services/spar/schema/src/Main.hs @@ -29,6 +29,7 @@ import qualified V11 import qualified V12 import qualified V13 import qualified V14 +import qualified V15 import qualified V2 import qualified V3 import qualified V4 @@ -61,7 +62,8 @@ main = do V11.migration, V12.migration, V13.migration, - V14.migration + V14.migration, + V15.migration -- When adding migrations here, don't forget to update -- 'schemaVersion' in Spar.Data diff --git a/services/spar/schema/src/V15.hs b/services/spar/schema/src/V15.hs new file mode 100644 index 0000000000..ca7290cfbf --- /dev/null +++ b/services/spar/schema/src/V15.hs @@ -0,0 +1,38 @@ +-- This file is part of the Wire Server implementation. +-- +-- Copyright (C) 2020 Wire Swiss GmbH +-- +-- This program is free software: you can redistribute it and/or modify it under +-- the terms of the GNU Affero General Public License as published by the Free +-- Software Foundation, either version 3 of the License, or (at your option) any +-- later version. +-- +-- This program is distributed in the hope that it will be useful, but WITHOUT +-- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS +-- FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +-- details. +-- +-- You should have received a copy of the GNU Affero General Public License along +-- with this program. If not, see . + +module V15 + ( migration, + ) +where + +import Cassandra.Schema +import Imports +import Text.RawString.QQ + +migration :: Migration +migration = Migration 15 "Optionally index IdP by teamid (add. to entityID)" $ do + void $ + schema' + [r| + CREATE TABLE if not exists issuer_idp_v2 + ( issuer text + , team uuid + , idp uuid + , PRIMARY KEY (issuer, team) + ) with compaction = {'class': 'LeveledCompactionStrategy'}; + |] diff --git a/services/spar/spar.cabal b/services/spar/spar.cabal index 884c3ef17e..ec6623400f 100644 --- a/services/spar/spar.cabal +++ b/services/spar/spar.cabal @@ -4,7 +4,7 @@ cabal-version: 1.12 -- -- see: https://github.com/sol/hpack -- --- hash: 2afa3a03f475aac5d63ab87ded5843f404fee3d8506ac70390ee37dde18f22d4 +-- hash: 6a8deefc6739b8a56d89eae84bd4333254d31f2b1bf1ab22b830d7d120992bbf name: spar version: 0.1 @@ -364,6 +364,7 @@ executable spar-schema V12 V13 V14 + V15 V2 V3 V4 diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 7afadc45b3..3a2a38f42e 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -85,10 +85,12 @@ api opts = apiSSO :: Opts -> ServerT APISSO Spar apiSSO opts = - SAML.meta appName sparSPIssuer sparResponseURI + SAML.meta appName (sparSPIssuer Nothing) (sparResponseURI Nothing) + :<|> (\tid -> SAML.meta appName (sparSPIssuer (Just tid)) (sparResponseURI (Just tid))) :<|> authreqPrecheck :<|> authreq (maxttlAuthreqDiffTime opts) DoInitiateLogin - :<|> authresp + :<|> authresp Nothing + :<|> authresp . Just :<|> ssoSettings apiIDP :: ServerT APIIDP Spar @@ -130,7 +132,9 @@ authreq _ DoInitiateLogin (Just _) _ _ _ = throwSpar SparInitLoginWithAuth authreq _ DoInitiateBind Nothing _ _ _ = throwSpar SparInitBindWithoutAuth authreq authreqttl _ zusr msucc merr idpid = do vformat <- validateAuthreqParams msucc merr - form@(SAML.FormRedirect _ ((^. SAML.rqID) -> reqid)) <- SAML.authreq authreqttl sparSPIssuer idpid + form@(SAML.FormRedirect _ ((^. SAML.rqID) -> reqid)) <- do + mbidp :: Maybe IdP <- wrapMonadClient (Data.getIdPConfig idpid) + SAML.authreq authreqttl (sparSPIssuer (mbidp <&> view (SAML.idpExtraInfo . wiTeam))) idpid wrapMonadClient $ Data.storeVerdictFormat authreqttl reqid vformat cky <- initializeBindCookie zusr authreqttl SAML.logger SAML.Debug $ "setting bind cookie: " <> show cky @@ -168,8 +172,8 @@ validateRedirectURL uri = do unless ((SBS.length $ URI.serializeURIRef' uri) <= redirectURLMaxLength) $ do throwSpar $ SparBadInitiateLoginQueryParams "url-too-long" -authresp :: Maybe ST -> SAML.AuthnResponseBody -> Spar Void -authresp ckyraw arbody = logErrors $ SAML.authresp sparSPIssuer sparResponseURI go arbody +authresp :: Maybe TeamId -> Maybe ST -> SAML.AuthnResponseBody -> Spar Void +authresp mbtid ckyraw arbody = logErrors $ SAML.authresp (sparSPIssuer mbtid) (sparResponseURI mbtid) go arbody where cky :: Maybe BindCookie cky = ckyraw >>= bindCookieFromHeader diff --git a/services/spar/src/Spar/App.hs b/services/spar/src/Spar/App.hs index c3914ab363..2b703cec66 100644 --- a/services/spar/src/Spar/App.hs +++ b/services/spar/src/Spar/App.hs @@ -146,6 +146,7 @@ instance SPStoreID Assertion Spar where instance SPStoreIdP SparError Spar where type IdPConfigExtra Spar = WireIdP + type IdPConfigSPId Spar = TeamId storeIdPConfig :: IdP -> Spar () storeIdPConfig idp = wrapMonadClient $ Data.storeIdPConfig idp @@ -153,8 +154,8 @@ instance SPStoreIdP SparError Spar where getIdPConfig :: IdPId -> Spar IdP getIdPConfig = (>>= maybe (throwSpar SparIdPNotFound) pure) . wrapMonadClientWithEnv . Data.getIdPConfig - getIdPConfigByIssuer :: Issuer -> Spar IdP - getIdPConfigByIssuer = (>>= maybe (throwSpar SparIdPNotFound) pure) . wrapMonadClientWithEnv . Data.getIdPConfigByIssuer + getIdPConfigByIssuer :: Issuer -> Maybe TeamId -> Spar IdP + getIdPConfigByIssuer issuer = (>>= maybe (throwSpar SparIdPNotFound) pure) . wrapMonadClientWithEnv . Data.getIdPConfigByIssuer issuer -- | 'wrapMonadClient' with an 'Env' in a 'ReaderT', and exceptions. If you -- don't need either of those, 'wrapMonadClient' will suffice. diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index 3f0658518b..f3e979c78a 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -120,7 +120,7 @@ import qualified Prelude -- | A lower bound: @schemaVersion <= whatWeFoundOnCassandra@, not @==@. schemaVersion :: Int32 -schemaVersion = 14 +schemaVersion = 15 ---------------------------------------------------------------------- -- helpers @@ -510,24 +510,46 @@ getIdPConfig idpid = getIdPConfigByIssuer :: (HasCallStack, MonadClient m) => SAML.Issuer -> + Maybe TeamId -> m (Maybe IdP) -getIdPConfigByIssuer issuer = do - getIdPIdByIssuer issuer >>= \case +getIdPConfigByIssuer issuer mbteam = do + getIdPIdByIssuer issuer mbteam >>= \case Nothing -> pure Nothing Just idpid -> getIdPConfig idpid +-- | Lookup idp in table `issuer_idp_v2` (using both issuer entityID and teamid); if nothing +-- is found there or if teamid is 'Nothing', lookup under issuer in `issuer_idp`. getIdPIdByIssuer :: (HasCallStack, MonadClient m) => SAML.Issuer -> + Maybe TeamId -> m (Maybe SAML.IdPId) -getIdPIdByIssuer issuer = do - retry x1 (query1 sel $ params Quorum (Identity issuer)) <&> \case - Nothing -> Nothing - Just (Identity idpid) -> Just idpid +getIdPIdByIssuer issuer mbteam = do + mbv2 <- maybe (pure Nothing) (getIdPIdByIssuerV2 issuer) mbteam + mbboth <- maybe (getIdPIdByIssuerOld issuer) (pure . Just) mbv2 + pure mbboth + +getIdPIdByIssuerOld :: + (HasCallStack, MonadClient m) => + SAML.Issuer -> + m (Maybe SAML.IdPId) +getIdPIdByIssuerOld issuer = do + runIdentity <$$> retry x1 (query1 sel $ params Quorum (Identity issuer)) where sel :: PrepQuery R (Identity SAML.Issuer) (Identity SAML.IdPId) sel = "SELECT idp FROM issuer_idp WHERE issuer = ?" +getIdPIdByIssuerV2 :: + (HasCallStack, MonadClient m) => + SAML.Issuer -> + TeamId -> + m (Maybe SAML.IdPId) +getIdPIdByIssuerV2 issuer tid = do + runIdentity <$$> retry x1 (query1 sel $ params Quorum (issuer, tid)) + where + sel :: PrepQuery R (SAML.Issuer, TeamId) (Identity SAML.IdPId) + sel = "SELECT idp FROM issuer_idp_v2 WHERE issuer = ? and team = ?" + getIdPConfigsByTeam :: (HasCallStack, MonadClient m) => TeamId -> diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 1103a4ce24..bc0a996ea6 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -130,7 +130,7 @@ specMisc = do specMetadata :: SpecWith TestEnv specMetadata = do describe "metadata" $ do - it "metadata" $ do + it "metadata (legacy)" $ do env <- ask get ((env ^. teSpar) . path "/sso/metadata" . expect2xx) `shouldRespondWith` ( \(responseBody -> Just (cs -> bdy)) -> @@ -138,7 +138,20 @@ specMetadata = do (`isInfixOf` bdy) [ "md:SPSSODescriptor", "validUntil", - "WantAssertionsSigned=\"true\"" + "WantAssertionsSigned=\"true\"", + "" + ] + ) + it "metadata (with team-id)" $ do + env <- ask + get ((env ^. teSpar) . path "/sso/metadata/3f1a01e2-1092-11ec-846d-3344b58840fe" . expect2xx) + `shouldRespondWith` ( \(responseBody -> Just (cs -> bdy)) -> + all + (`isInfixOf` bdy) + [ "md:SPSSODescriptor", + "validUntil", + "WantAssertionsSigned=\"true\"", + "" ] ) From 2a3d79b5b039d877ad2e34fb5406e779199cd90a Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 8 Sep 2021 20:46:15 +0200 Subject: [PATCH 02/52] RESET_PLS --- docs/reference/spar-braindump.md | 59 +++++++++++++++++++ services/spar/src/Spar/API.hs | 8 +-- services/spar/src/Spar/App.hs | 8 +-- services/spar/src/Spar/Data.hs | 2 + services/spar/src/Spar/Options.hs | 2 +- .../test-integration/Test/Spar/APISpec.hs | 12 +++- .../test-integration/Test/Spar/DataSpec.hs | 10 ++-- stack.yaml | 3 +- stack.yaml.lock | 11 ---- 9 files changed, 86 insertions(+), 29 deletions(-) diff --git a/docs/reference/spar-braindump.md b/docs/reference/spar-braindump.md index d6fe65a414..b1957e4d3f 100644 --- a/docs/reference/spar-braindump.md +++ b/docs/reference/spar-braindump.md @@ -326,3 +326,62 @@ 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 for fresh +apps. The way... + + +changes: + +- a few end-points have been make more flexible by adding an optional + teamid to the path: ... +- /sso/initiate-login returns an AuthnReq with the response url that + contains the teamid. + + +we make sure that it doesn't matter whether an IdP calls the +end-point(s) *with* teamid or *without*. but we may confuse some bad +idps by sending them a url with the teamid init, where they expect one +without it. + +we could change /sso/initiate-login to contain the teamid based on +which table the idp comes from, and make sure that we insert idps into +the old table if the metadata sais that's what the idp expects. it's +annoying and awkward, though. + +ya, we need to make sure idps set up without teamid will still present with the old urls everywhere, especially in the initiate-login resp body. + +this leaves the complication that people may create new idps based on the metadata they pulled from /metadata (without teamid). can we accomodate this, too? + + + +yet another complication: we need to pull in the fold-case stuff. + - look at old PRs that are related. (none?) + - decide whether we want to pull this in. (makes sense to me.) + - do it before finishing up the 2-sp-change. + - this shouldn't be too hard: if we relax case matching, we shouldn't get any extra matches (emails only distinguished by case are weird). also, no extra errors, because every input behavior that worked before will work after (it's *relaxing* the matching). + -> is this reasoning sound? + + + +save and lookup differently: +- if found in the old table, use the old api +- if found in the new table, use the new api +- store in the new table only if query param is given. this "ensures" that people do it only when they got the new metadata file. (no typesafe way to accomplish that, it happens outside of our power.) + + + + +it's called an `Audience mismatch`, and it's a real thing! +- but perhaps it is allowed to add multiple finalize urls to the authreq? is the idp required to match against all, and be happy if one of them matches? + + + + +next steps: +- write integration tests. make them fail for good reasons. +- read the above ideas. +- finish the implementation. diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 3a2a38f42e..c25db14f93 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -173,7 +173,7 @@ validateRedirectURL uri = do throwSpar $ SparBadInitiateLoginQueryParams "url-too-long" authresp :: Maybe TeamId -> Maybe ST -> SAML.AuthnResponseBody -> Spar Void -authresp mbtid ckyraw arbody = logErrors $ SAML.authresp (sparSPIssuer mbtid) (sparResponseURI mbtid) go arbody +authresp mbtid ckyraw arbody = logErrors $ SAML.authresp mbtid (sparSPIssuer mbtid) (sparResponseURI mbtid) go arbody where cky :: Maybe BindCookie cky = ckyraw >>= bindCookieFromHeader @@ -273,7 +273,7 @@ idpDelete zusr idpid (fromMaybe False -> purge) = withDebugLog "idpDelete" (cons updateReplacingIdP :: IdP -> Spar () updateReplacingIdP idp = forM_ (idp ^. SAML.idpExtraInfo . wiOldIssuers) $ \oldIssuer -> do wrapMonadClient $ do - iid <- Data.getIdPIdByIssuer oldIssuer + iid <- Data.getIdPIdByIssuer oldIssuer Nothing mapM_ (Data.clearReplacedBy . Data.Replaced) iid -- | This handler only does the json parsing, and leaves all authorization checks and @@ -340,7 +340,7 @@ validateNewIdP _idpMetadata teamId mReplaces = do let requri = _idpMetadata ^. SAML.edRequestURI _idpExtraInfo = WireIdP teamId oldIssuers Nothing enforceHttps requri - wrapMonadClient (Data.getIdPIdByIssuer (_idpMetadata ^. SAML.edIssuer)) >>= \case + wrapMonadClient (Data.getIdPIdByIssuer (_idpMetadata ^. SAML.edIssuer) Nothing) >>= \case Nothing -> pure () Just _ -> throwSpar SparNewIdPAlreadyInUse pure SAML.IdPConfig {..} @@ -387,7 +387,7 @@ validateIdPUpdate zusr _idpMetadata _idpId = do if previousIssuer == newIssuer then pure $ previousIdP ^. SAML.idpExtraInfo else do - foundConfig <- wrapMonadClient (Data.getIdPConfigByIssuer newIssuer) + foundConfig <- wrapMonadClient (Data.getIdPConfigByIssuer newIssuer Nothing) let notInUseByOthers = case foundConfig of Nothing -> True Just c -> c ^. SAML.idpId == _idpId diff --git a/services/spar/src/Spar/App.hs b/services/spar/src/Spar/App.hs index 2b703cec66..f73dfb9866 100644 --- a/services/spar/src/Spar/App.hs +++ b/services/spar/src/Spar/App.hs @@ -232,7 +232,7 @@ getUserByScimExternalId tid email = do -- undeletable in the team admin page, and ask admins to go talk to their IdP system. createSamlUserWithId :: UserId -> SAML.UserRef -> Spar () createSamlUserWithId buid suid = do - teamid <- (^. idpExtraInfo . wiTeam) <$> getIdPConfigByIssuer (suid ^. uidTenant) + teamid <- (^. idpExtraInfo . wiTeam) <$> getIdPConfigByIssuer (suid ^. uidTenant) Nothing uname <- either (throwSpar . SparBadUserName . cs) pure $ Intra.mkUserName Nothing (UrefOnly suid) buid' <- Intra.createBrigUserSAML suid buid teamid uname ManagedByWire assert (buid == buid') $ pure () @@ -249,7 +249,7 @@ autoprovisionSamlUser suid = do -- | Like 'autoprovisionSamlUser', but for an already existing 'UserId'. autoprovisionSamlUserWithId :: UserId -> SAML.UserRef -> Spar () autoprovisionSamlUserWithId buid suid = do - idp <- getIdPConfigByIssuer (suid ^. uidTenant) + idp <- getIdPConfigByIssuer (suid ^. uidTenant) Nothing guardReplacedIdP idp guardScimTokens idp createSamlUserWithId buid suid @@ -295,7 +295,7 @@ bindUser buid userref = do oldStatus <- do let err :: Spar a err = throwSpar . SparBindFromWrongOrNoTeam . cs . show $ buid - teamid :: TeamId <- (^. idpExtraInfo . wiTeam) <$> getIdPConfigByIssuer (userref ^. uidTenant) + teamid :: TeamId <- (^. idpExtraInfo . wiTeam) <$> getIdPConfigByIssuer (userref ^. uidTenant) Nothing acc <- Intra.getBrigUserAccount Intra.WithPendingInvitations buid >>= maybe err pure teamid' :: TeamId <- userTeam (accountUser acc) & maybe err pure unless (teamid' == teamid) err @@ -393,7 +393,7 @@ catchVerdictErrors = (`catchError` hndlr) -- traverse the old IdPs in search for the old entry. Return that old entry. findUserWithOldIssuer :: SAML.UserRef -> Spar (Maybe (SAML.UserRef, UserId)) findUserWithOldIssuer (SAML.UserRef issuer subject) = do - idp <- getIdPConfigByIssuer issuer + idp <- getIdPConfigByIssuer issuer Nothing let tryFind :: Maybe (SAML.UserRef, UserId) -> Issuer -> Spar (Maybe (SAML.UserRef, UserId)) tryFind found@(Just _) _ = pure found tryFind Nothing oldIssuer = (uref,) <$$> getUserByUref uref diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index f3e979c78a..011b13e8f1 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -447,6 +447,8 @@ storeIdPConfig idp = retry x5 . batch $ do where ins :: PrepQuery W IdPConfigRow () ins = "INSERT INTO idp (idp, issuer, request_uri, public_key, extra_public_keys, team, old_issuers, replaced_by) VALUES (?, ?, ?, ?, ?, ?, ?, ?)" + -- TODO: only insert into issuer_idp_v2, not into the old table! + -- FUTUREWORK: migrate the old table away, we don't need it any more. byIssuer :: PrepQuery W (SAML.IdPId, SAML.Issuer) () byIssuer = "INSERT INTO issuer_idp (idp, issuer) VALUES (?, ?)" byTeam :: PrepQuery W (SAML.IdPId, TeamId) () diff --git a/services/spar/src/Spar/Options.hs b/services/spar/src/Spar/Options.hs index 27bfdff16b..e83b487ffd 100644 --- a/services/spar/src/Spar/Options.hs +++ b/services/spar/src/Spar/Options.hs @@ -53,7 +53,7 @@ getOpts = do deriveOpts :: OptsRaw -> IO Opts deriveOpts raw = do derived <- do - let respuri = runWithConfig raw sparResponseURI + let respuri = runWithConfig raw (sparResponseURI Nothing) -- TODO: where is this used? derivedOptsBindCookiePath = URI.uriPath respuri -- We could also make this selectable in the config file, but it seems easier to derive it from -- the SAML base uri. diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index bc0a996ea6..d3dfad6cd2 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -129,7 +129,7 @@ specMisc = do specMetadata :: SpecWith TestEnv specMetadata = do - describe "metadata" $ do + focus . describe "metadata" $ do it "metadata (legacy)" $ do env <- ask get ((env ^. teSpar) . path "/sso/metadata" . expect2xx) @@ -235,13 +235,21 @@ specFinalizeLogin = do bdy `shouldContain` "wire:sso:success" bdy `shouldContain` "window.opener.postMessage({type: 'AUTH_SUCCESS'}, receiverOrigin)" hasPersistentCookieHeader sparresp `shouldBe` Right () - context "happy flow" $ do + focus . context "happy flow (legacy, each EntityID is mapped on at most one team)" $ do it "responds with a very peculiar 'allowed' HTTP response" $ do (_, _, idp, (_, privcreds)) <- registerTestIdPWithMeta spmeta <- getTestSPMetadata authnreq <- negotiateAuthnRequest idp authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp spmeta authnreq True loginSuccess =<< submitAuthnResponse authnresp + focus . context "happy flow (allow one IdP EntityID to be shared by two teams)" $ do + it "responds with a very peculiar 'allowed' HTTP response" $ do + () <- error "TODO" + (_, _, idp, (_, privcreds)) <- registerTestIdPWithMeta + spmeta <- getTestSPMetadata + authnreq <- negotiateAuthnRequest idp + authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp spmeta authnreq True + loginSuccess =<< submitAuthnResponse 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 diff --git a/services/spar/test-integration/Test/Spar/DataSpec.hs b/services/spar/test-integration/Test/Spar/DataSpec.hs index bc4d8f418a..a99e54eeec 100644 --- a/services/spar/test-integration/Test/Spar/DataSpec.hs +++ b/services/spar/test-integration/Test/Spar/DataSpec.hs @@ -170,12 +170,12 @@ spec = do it "getIdPConfigByIssuer works" $ do idp <- makeTestIdP () <- runSparCass $ Data.storeIdPConfig idp - midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) + midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) Nothing liftIO $ midp `shouldBe` Just idp it "getIdPIdByIssuer works" $ do idp <- makeTestIdP () <- runSparCass $ Data.storeIdPConfig idp - midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) + midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) Nothing liftIO $ midp `shouldBe` Just (idp ^. idpId) it "getIdPConfigsByTeam works" $ do teamid <- nextWireId @@ -195,10 +195,10 @@ spec = do midp <- runSparCass $ Data.getIdPConfig (idp ^. idpId) liftIO $ midp `shouldBe` Nothing do - midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) + midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) Nothing liftIO $ midp `shouldBe` Nothing do - midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) + midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) Nothing liftIO $ midp `shouldBe` Nothing do idps <- runSparCass $ Data.getIdPConfigsByTeam teamid @@ -302,7 +302,7 @@ testDeleteTeam = it "cleans up all the right tables after deletion" $ do -- The config from 'issuer_idp': do let issuer = idp ^. SAML.idpMetadata . SAML.edIssuer - mbIdp <- runSparCass $ Data.getIdPIdByIssuer issuer + mbIdp <- runSparCass $ Data.getIdPIdByIssuer issuer Nothing liftIO $ mbIdp `shouldBe` Nothing -- The config from 'team_idp': do diff --git a/stack.yaml b/stack.yaml index 7eb20da383..873b684a05 100644 --- a/stack.yaml +++ b/stack.yaml @@ -69,8 +69,7 @@ extra-deps: # wai-middleware-prometheus can be pulled from hackage once the # a version > 1.0.0 of wai-middleware-prometheus is available # (required: https://github.com/fimad/prometheus-haskell/pull/45) -- git: https://github.com/wireapp/saml2-web-sso - commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c # https://github.com/wireapp/saml2-web-sso/pull/74 (Apr 30, 2021) +- ../saml2-web-sso - git: https://github.com/kim/hs-collectd commit: 885da222be2375f78c7be36127620ed772b677c9 diff --git a/stack.yaml.lock b/stack.yaml.lock index 3cd6d32d24..85c0ca3ebe 100644 --- a/stack.yaml.lock +++ b/stack.yaml.lock @@ -17,17 +17,6 @@ packages: subdir: wai-middleware-prometheus git: https://github.com/fimad/prometheus-haskell commit: 2e3282e5fb27ba8d989c271a0a989823fad7ec43 -- completed: - name: saml2-web-sso - version: '0.18' - git: https://github.com/wireapp/saml2-web-sso - pantry-tree: - size: 4887 - sha256: 12be9a699749b9ebe63fb2e04113c2f2160a63494e8a2ba005792a02d0571f47 - commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c - original: - git: https://github.com/wireapp/saml2-web-sso - commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c - completed: name: collectd version: 0.0.0.2 From 3b229e9b9187935c256ad4d392937c129ac829b8 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 8 Sep 2021 20:46:19 +0200 Subject: [PATCH 03/52] Revert "RESET_PLS" This reverts commit 17c22336940529a014d649e3d17eed6de6f1c05d. --- docs/reference/spar-braindump.md | 59 ------------------- services/spar/src/Spar/API.hs | 8 +-- services/spar/src/Spar/App.hs | 8 +-- services/spar/src/Spar/Data.hs | 2 - services/spar/src/Spar/Options.hs | 2 +- .../test-integration/Test/Spar/APISpec.hs | 12 +--- .../test-integration/Test/Spar/DataSpec.hs | 10 ++-- stack.yaml | 3 +- stack.yaml.lock | 11 ++++ 9 files changed, 29 insertions(+), 86 deletions(-) diff --git a/docs/reference/spar-braindump.md b/docs/reference/spar-braindump.md index b1957e4d3f..d6fe65a414 100644 --- a/docs/reference/spar-braindump.md +++ b/docs/reference/spar-braindump.md @@ -326,62 +326,3 @@ 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 for fresh -apps. The way... - - -changes: - -- a few end-points have been make more flexible by adding an optional - teamid to the path: ... -- /sso/initiate-login returns an AuthnReq with the response url that - contains the teamid. - - -we make sure that it doesn't matter whether an IdP calls the -end-point(s) *with* teamid or *without*. but we may confuse some bad -idps by sending them a url with the teamid init, where they expect one -without it. - -we could change /sso/initiate-login to contain the teamid based on -which table the idp comes from, and make sure that we insert idps into -the old table if the metadata sais that's what the idp expects. it's -annoying and awkward, though. - -ya, we need to make sure idps set up without teamid will still present with the old urls everywhere, especially in the initiate-login resp body. - -this leaves the complication that people may create new idps based on the metadata they pulled from /metadata (without teamid). can we accomodate this, too? - - - -yet another complication: we need to pull in the fold-case stuff. - - look at old PRs that are related. (none?) - - decide whether we want to pull this in. (makes sense to me.) - - do it before finishing up the 2-sp-change. - - this shouldn't be too hard: if we relax case matching, we shouldn't get any extra matches (emails only distinguished by case are weird). also, no extra errors, because every input behavior that worked before will work after (it's *relaxing* the matching). - -> is this reasoning sound? - - - -save and lookup differently: -- if found in the old table, use the old api -- if found in the new table, use the new api -- store in the new table only if query param is given. this "ensures" that people do it only when they got the new metadata file. (no typesafe way to accomplish that, it happens outside of our power.) - - - - -it's called an `Audience mismatch`, and it's a real thing! -- but perhaps it is allowed to add multiple finalize urls to the authreq? is the idp required to match against all, and be happy if one of them matches? - - - - -next steps: -- write integration tests. make them fail for good reasons. -- read the above ideas. -- finish the implementation. diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index c25db14f93..3a2a38f42e 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -173,7 +173,7 @@ validateRedirectURL uri = do throwSpar $ SparBadInitiateLoginQueryParams "url-too-long" authresp :: Maybe TeamId -> Maybe ST -> SAML.AuthnResponseBody -> Spar Void -authresp mbtid ckyraw arbody = logErrors $ SAML.authresp mbtid (sparSPIssuer mbtid) (sparResponseURI mbtid) go arbody +authresp mbtid ckyraw arbody = logErrors $ SAML.authresp (sparSPIssuer mbtid) (sparResponseURI mbtid) go arbody where cky :: Maybe BindCookie cky = ckyraw >>= bindCookieFromHeader @@ -273,7 +273,7 @@ idpDelete zusr idpid (fromMaybe False -> purge) = withDebugLog "idpDelete" (cons updateReplacingIdP :: IdP -> Spar () updateReplacingIdP idp = forM_ (idp ^. SAML.idpExtraInfo . wiOldIssuers) $ \oldIssuer -> do wrapMonadClient $ do - iid <- Data.getIdPIdByIssuer oldIssuer Nothing + iid <- Data.getIdPIdByIssuer oldIssuer mapM_ (Data.clearReplacedBy . Data.Replaced) iid -- | This handler only does the json parsing, and leaves all authorization checks and @@ -340,7 +340,7 @@ validateNewIdP _idpMetadata teamId mReplaces = do let requri = _idpMetadata ^. SAML.edRequestURI _idpExtraInfo = WireIdP teamId oldIssuers Nothing enforceHttps requri - wrapMonadClient (Data.getIdPIdByIssuer (_idpMetadata ^. SAML.edIssuer) Nothing) >>= \case + wrapMonadClient (Data.getIdPIdByIssuer (_idpMetadata ^. SAML.edIssuer)) >>= \case Nothing -> pure () Just _ -> throwSpar SparNewIdPAlreadyInUse pure SAML.IdPConfig {..} @@ -387,7 +387,7 @@ validateIdPUpdate zusr _idpMetadata _idpId = do if previousIssuer == newIssuer then pure $ previousIdP ^. SAML.idpExtraInfo else do - foundConfig <- wrapMonadClient (Data.getIdPConfigByIssuer newIssuer Nothing) + foundConfig <- wrapMonadClient (Data.getIdPConfigByIssuer newIssuer) let notInUseByOthers = case foundConfig of Nothing -> True Just c -> c ^. SAML.idpId == _idpId diff --git a/services/spar/src/Spar/App.hs b/services/spar/src/Spar/App.hs index f73dfb9866..2b703cec66 100644 --- a/services/spar/src/Spar/App.hs +++ b/services/spar/src/Spar/App.hs @@ -232,7 +232,7 @@ getUserByScimExternalId tid email = do -- undeletable in the team admin page, and ask admins to go talk to their IdP system. createSamlUserWithId :: UserId -> SAML.UserRef -> Spar () createSamlUserWithId buid suid = do - teamid <- (^. idpExtraInfo . wiTeam) <$> getIdPConfigByIssuer (suid ^. uidTenant) Nothing + teamid <- (^. idpExtraInfo . wiTeam) <$> getIdPConfigByIssuer (suid ^. uidTenant) uname <- either (throwSpar . SparBadUserName . cs) pure $ Intra.mkUserName Nothing (UrefOnly suid) buid' <- Intra.createBrigUserSAML suid buid teamid uname ManagedByWire assert (buid == buid') $ pure () @@ -249,7 +249,7 @@ autoprovisionSamlUser suid = do -- | Like 'autoprovisionSamlUser', but for an already existing 'UserId'. autoprovisionSamlUserWithId :: UserId -> SAML.UserRef -> Spar () autoprovisionSamlUserWithId buid suid = do - idp <- getIdPConfigByIssuer (suid ^. uidTenant) Nothing + idp <- getIdPConfigByIssuer (suid ^. uidTenant) guardReplacedIdP idp guardScimTokens idp createSamlUserWithId buid suid @@ -295,7 +295,7 @@ bindUser buid userref = do oldStatus <- do let err :: Spar a err = throwSpar . SparBindFromWrongOrNoTeam . cs . show $ buid - teamid :: TeamId <- (^. idpExtraInfo . wiTeam) <$> getIdPConfigByIssuer (userref ^. uidTenant) Nothing + teamid :: TeamId <- (^. idpExtraInfo . wiTeam) <$> getIdPConfigByIssuer (userref ^. uidTenant) acc <- Intra.getBrigUserAccount Intra.WithPendingInvitations buid >>= maybe err pure teamid' :: TeamId <- userTeam (accountUser acc) & maybe err pure unless (teamid' == teamid) err @@ -393,7 +393,7 @@ catchVerdictErrors = (`catchError` hndlr) -- traverse the old IdPs in search for the old entry. Return that old entry. findUserWithOldIssuer :: SAML.UserRef -> Spar (Maybe (SAML.UserRef, UserId)) findUserWithOldIssuer (SAML.UserRef issuer subject) = do - idp <- getIdPConfigByIssuer issuer Nothing + idp <- getIdPConfigByIssuer issuer let tryFind :: Maybe (SAML.UserRef, UserId) -> Issuer -> Spar (Maybe (SAML.UserRef, UserId)) tryFind found@(Just _) _ = pure found tryFind Nothing oldIssuer = (uref,) <$$> getUserByUref uref diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index 011b13e8f1..f3e979c78a 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -447,8 +447,6 @@ storeIdPConfig idp = retry x5 . batch $ do where ins :: PrepQuery W IdPConfigRow () ins = "INSERT INTO idp (idp, issuer, request_uri, public_key, extra_public_keys, team, old_issuers, replaced_by) VALUES (?, ?, ?, ?, ?, ?, ?, ?)" - -- TODO: only insert into issuer_idp_v2, not into the old table! - -- FUTUREWORK: migrate the old table away, we don't need it any more. byIssuer :: PrepQuery W (SAML.IdPId, SAML.Issuer) () byIssuer = "INSERT INTO issuer_idp (idp, issuer) VALUES (?, ?)" byTeam :: PrepQuery W (SAML.IdPId, TeamId) () diff --git a/services/spar/src/Spar/Options.hs b/services/spar/src/Spar/Options.hs index e83b487ffd..27bfdff16b 100644 --- a/services/spar/src/Spar/Options.hs +++ b/services/spar/src/Spar/Options.hs @@ -53,7 +53,7 @@ getOpts = do deriveOpts :: OptsRaw -> IO Opts deriveOpts raw = do derived <- do - let respuri = runWithConfig raw (sparResponseURI Nothing) -- TODO: where is this used? + let respuri = runWithConfig raw sparResponseURI derivedOptsBindCookiePath = URI.uriPath respuri -- We could also make this selectable in the config file, but it seems easier to derive it from -- the SAML base uri. diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index d3dfad6cd2..bc0a996ea6 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -129,7 +129,7 @@ specMisc = do specMetadata :: SpecWith TestEnv specMetadata = do - focus . describe "metadata" $ do + describe "metadata" $ do it "metadata (legacy)" $ do env <- ask get ((env ^. teSpar) . path "/sso/metadata" . expect2xx) @@ -235,21 +235,13 @@ specFinalizeLogin = do bdy `shouldContain` "wire:sso:success" bdy `shouldContain` "window.opener.postMessage({type: 'AUTH_SUCCESS'}, receiverOrigin)" hasPersistentCookieHeader sparresp `shouldBe` Right () - focus . context "happy flow (legacy, each EntityID is mapped on at most one team)" $ do + context "happy flow" $ do it "responds with a very peculiar 'allowed' HTTP response" $ do (_, _, idp, (_, privcreds)) <- registerTestIdPWithMeta spmeta <- getTestSPMetadata authnreq <- negotiateAuthnRequest idp authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp spmeta authnreq True loginSuccess =<< submitAuthnResponse authnresp - focus . context "happy flow (allow one IdP EntityID to be shared by two teams)" $ do - it "responds with a very peculiar 'allowed' HTTP response" $ do - () <- error "TODO" - (_, _, idp, (_, privcreds)) <- registerTestIdPWithMeta - spmeta <- getTestSPMetadata - authnreq <- negotiateAuthnRequest idp - authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp spmeta authnreq True - loginSuccess =<< submitAuthnResponse 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 diff --git a/services/spar/test-integration/Test/Spar/DataSpec.hs b/services/spar/test-integration/Test/Spar/DataSpec.hs index a99e54eeec..bc4d8f418a 100644 --- a/services/spar/test-integration/Test/Spar/DataSpec.hs +++ b/services/spar/test-integration/Test/Spar/DataSpec.hs @@ -170,12 +170,12 @@ spec = do it "getIdPConfigByIssuer works" $ do idp <- makeTestIdP () <- runSparCass $ Data.storeIdPConfig idp - midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) Nothing + midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) liftIO $ midp `shouldBe` Just idp it "getIdPIdByIssuer works" $ do idp <- makeTestIdP () <- runSparCass $ Data.storeIdPConfig idp - midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) Nothing + midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) liftIO $ midp `shouldBe` Just (idp ^. idpId) it "getIdPConfigsByTeam works" $ do teamid <- nextWireId @@ -195,10 +195,10 @@ spec = do midp <- runSparCass $ Data.getIdPConfig (idp ^. idpId) liftIO $ midp `shouldBe` Nothing do - midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) Nothing + midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) liftIO $ midp `shouldBe` Nothing do - midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) Nothing + midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) liftIO $ midp `shouldBe` Nothing do idps <- runSparCass $ Data.getIdPConfigsByTeam teamid @@ -302,7 +302,7 @@ testDeleteTeam = it "cleans up all the right tables after deletion" $ do -- The config from 'issuer_idp': do let issuer = idp ^. SAML.idpMetadata . SAML.edIssuer - mbIdp <- runSparCass $ Data.getIdPIdByIssuer issuer Nothing + mbIdp <- runSparCass $ Data.getIdPIdByIssuer issuer liftIO $ mbIdp `shouldBe` Nothing -- The config from 'team_idp': do diff --git a/stack.yaml b/stack.yaml index 873b684a05..7eb20da383 100644 --- a/stack.yaml +++ b/stack.yaml @@ -69,7 +69,8 @@ extra-deps: # wai-middleware-prometheus can be pulled from hackage once the # a version > 1.0.0 of wai-middleware-prometheus is available # (required: https://github.com/fimad/prometheus-haskell/pull/45) -- ../saml2-web-sso +- git: https://github.com/wireapp/saml2-web-sso + commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c # https://github.com/wireapp/saml2-web-sso/pull/74 (Apr 30, 2021) - git: https://github.com/kim/hs-collectd commit: 885da222be2375f78c7be36127620ed772b677c9 diff --git a/stack.yaml.lock b/stack.yaml.lock index 85c0ca3ebe..3cd6d32d24 100644 --- a/stack.yaml.lock +++ b/stack.yaml.lock @@ -17,6 +17,17 @@ packages: subdir: wai-middleware-prometheus git: https://github.com/fimad/prometheus-haskell commit: 2e3282e5fb27ba8d989c271a0a989823fad7ec43 +- completed: + name: saml2-web-sso + version: '0.18' + git: https://github.com/wireapp/saml2-web-sso + pantry-tree: + size: 4887 + sha256: 12be9a699749b9ebe63fb2e04113c2f2160a63494e8a2ba005792a02d0571f47 + commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c + original: + git: https://github.com/wireapp/saml2-web-sso + commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c - completed: name: collectd version: 0.0.0.2 From 05e269d203ccb2c2af1a84203203257d638eec0e Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 9 Sep 2021 10:03:06 +0200 Subject: [PATCH 04/52] Tests. --- services/spar/test-integration/Spec.hs | 29 +++++-- .../test-integration/Test/Spar/APISpec.hs | 81 ++++++++++--------- .../test-integration/Test/Spar/AppSpec.hs | 3 +- .../test-integration/Test/Spar/DataSpec.hs | 10 +-- .../Test/Spar/Scim/AuthSpec.hs | 5 +- .../Test/Spar/Scim/UserSpec.hs | 6 +- services/spar/test-integration/Util/Core.hs | 55 ++++++++----- services/spar/test-integration/Util/Types.hs | 19 ++++- 8 files changed, 131 insertions(+), 77 deletions(-) diff --git a/services/spar/test-integration/Spec.hs b/services/spar/test-integration/Spec.hs index 32c0b1bf92..82a06df838 100644 --- a/services/spar/test-integration/Spec.hs +++ b/services/spar/test-integration/Spec.hs @@ -29,7 +29,7 @@ -- the solution: https://github.com/hspec/hspec/pull/397. module Main where -import Control.Lens ((^.)) +import Control.Lens ((.~), (^.)) import Data.String.Conversions import Data.Text (pack) import Imports @@ -53,10 +53,16 @@ import Wire.API.User.Scim main :: IO () main = do (wireArgs, hspecArgs) <- partitionArgs <$> getArgs - env <- withArgs wireArgs mkEnvFromOptions + let env = withArgs wireArgs mkEnvFromOptions withArgs hspecArgs . hspec $ do - beforeAll (pure env) . afterAll destroyEnv $ mkspec - mkspec' env + beforeAll env . afterAll destroyEnv $ do + mkspecMisc + mkspecSaml + mkspecScim + beforeAll (env <&> teLegacySAMLEndPoints .~ True) . afterAll destroyEnv $ do + mkspecSaml + mkspecScim + mkspecHscimAcceptance env destroyEnv partitionArgs :: [String] -> ([String], [String]) partitionArgs = go [] [] @@ -66,23 +72,30 @@ partitionArgs = go [] [] go wireArgs hspecArgs (x : xs) = go wireArgs (hspecArgs <> [x]) xs go wireArgs hspecArgs [] = (wireArgs, hspecArgs) -mkspec :: SpecWith TestEnv -mkspec = do +mkspecMisc :: SpecWith TestEnv +mkspecMisc = do describe "Logging" Test.LoggingSpec.spec describe "Metrics" Test.MetricsSpec.spec + +mkspecSaml :: SpecWith TestEnv +mkspecSaml = do describe "Spar.API" Test.Spar.APISpec.spec describe "Spar.App" Test.Spar.AppSpec.spec describe "Spar.Data" Test.Spar.DataSpec.spec describe "Spar.Intra.Brig" Test.Spar.Intra.BrigSpec.spec + +mkspecScim :: SpecWith TestEnv +mkspecScim = do describe "Spar.Scim.Auth" Test.Spar.Scim.AuthSpec.spec describe "Spar.Scim.User" Test.Spar.Scim.UserSpec.spec -mkspec' :: TestEnv -> Spec -mkspec' env = do +mkspecHscimAcceptance :: IO TestEnv -> (TestEnv -> IO ()) -> Spec +mkspecHscimAcceptance mkenv _destroyenv = do describe "hscim acceptance tests" $ microsoftAzure @SparTag AcceptanceConfig {..} where scimAppAndConfig = do + env <- mkenv (app, _) <- mkApp (env ^. teOpts) scimAuthToken <- toHeader . fst <$> registerIdPAndScimToken `runReaderT` env let queryConfig = AcceptanceQueryConfig {..} diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index bc0a996ea6..8f05fa135f 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -122,7 +122,7 @@ specMisc = do pure $ nonfresh & edIssuer .~ issuer env <- ask uid <- fst <$> call (createUserWithTeam (env ^. teBrig) (env ^. teGalley)) - resp <- call $ callIdpCreate' (env ^. teSpar) (Just uid) somemeta + resp <- call $ callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid) somemeta liftIO $ statusCode resp `shouldBe` if isHttps then 201 else 400 it "does not trigger on https urls" $ check True it "does trigger on http urls" $ check False @@ -199,11 +199,11 @@ specFinalizeLogin = do describe "POST /sso/finalize-login" $ do context "access denied" $ do it "responds with a very peculiar 'forbidden' HTTP response" $ do - (_, _, idp, (_, privcreds)) <- registerTestIdPWithMeta + (_, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta authnreq <- negotiateAuthnRequest idp - spmeta <- getTestSPMetadata + spmeta <- getTestSPMetadata tid authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp spmeta authnreq False - sparresp <- submitAuthnResponse authnresp + sparresp <- submitAuthnResponse tid authnresp liftIO $ do -- import Text.XML -- putStrLn $ unlines @@ -237,20 +237,20 @@ specFinalizeLogin = do hasPersistentCookieHeader sparresp `shouldBe` Right () context "happy flow" $ do it "responds with a very peculiar 'allowed' HTTP response" $ do - (_, _, idp, (_, privcreds)) <- registerTestIdPWithMeta - spmeta <- getTestSPMetadata + (_, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta + spmeta <- getTestSPMetadata tid authnreq <- negotiateAuthnRequest idp authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp spmeta authnreq True - loginSuccess =<< submitAuthnResponse authnresp + loginSuccess =<< submitAuthnResponse tid 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 - spmeta <- getTestSPMetadata + spmeta <- getTestSPMetadata (idp ^. idpExtraInfo . wiTeam) -- first login newUserAuthnResp :: SignedAuthnResponse <- do authnreq <- negotiateAuthnRequest idp authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp spmeta authnreq True - loginSuccess =<< submitAuthnResponse authnresp + loginSuccess =<< submitAuthnResponse teamid authnresp pure $ authnresp let newUserRef@(UserRef _ subj) = either (error . show) (^. userRefL) $ @@ -283,7 +283,7 @@ specFinalizeLogin = do do authnreq <- negotiateAuthnRequest idp authnresp <- runSimpleSP $ mkAuthnResponseWithSubj subj privcreds idp spmeta authnreq True - loginSuccess =<< submitAuthnResponse authnresp + loginSuccess =<< submitAuthnResponse teamid authnresp context "unknown user" $ do it "creates the user" $ do pending @@ -298,9 +298,9 @@ specFinalizeLogin = do pending context "unknown IdP Issuer" $ do it "rejects" $ do - (_, _, idp, (_, privcreds)) <- registerTestIdPWithMeta + (_, teamid, idp, (_, privcreds)) <- registerTestIdPWithMeta authnreq <- negotiateAuthnRequest idp - spmeta <- getTestSPMetadata + spmeta <- getTestSPMetadata (idp ^. idpExtraInfo . wiTeam) authnresp <- runSimpleSP $ mkAuthnResponse @@ -309,7 +309,7 @@ specFinalizeLogin = do spmeta authnreq True - sparresp <- submitAuthnResponse authnresp + sparresp <- submitAuthnResponse teamid authnresp let shouldContainInBase64 :: String -> String -> Expectation shouldContainInBase64 hay needle = cs hay'' `shouldContain` needle where @@ -340,7 +340,7 @@ specFinalizeLogin = do context "IdP changes response format" $ do it "treats NameId case-insensitively" $ do (_ownerid, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta - spmeta <- getTestSPMetadata + spmeta <- getTestSPMetadata tid let loginSuccess :: HasCallStack => ResponseLBS -> TestSpar () loginSuccess sparresp = liftIO $ do @@ -349,7 +349,7 @@ specFinalizeLogin = do let loginWithSubject subj = do authnreq <- negotiateAuthnRequest idp authnresp <- runSimpleSP $ mkAuthnResponseWithSubj subj privcreds idp spmeta authnreq True - loginSuccess =<< submitAuthnResponse authnresp + loginSuccess =<< submitAuthnResponse tid authnresp ssoid <- getSsoidViaAuthResp authnresp ssoToUidSpar tid ssoid @@ -495,20 +495,21 @@ specBindingUsers = describe "binding existing users to sso identities" $ do reBindSame' tweakcookies uid idp privCreds subj = do (authnReq, Just (SetBindCookie (SimpleSetCookie bindCky))) <- do negotiateAuthnRequest' DoInitiateBind idp (header "Z-User" $ toByteString' uid) - spmeta <- getTestSPMetadata + let tid = idp ^. idpExtraInfo . wiTeam + spmeta <- getTestSPMetadata tid authnResp <- runSimpleSP $ mkAuthnResponseWithSubj subj privCreds idp spmeta authnReq True let cookiehdr = case tweakcookies [(Cky.setCookieName bindCky, Cky.setCookieValue bindCky)] of Just val -> header "Cookie" . cs . LB.toLazyByteString . Cky.renderCookies $ val Nothing -> id sparAuthnResp :: ResponseLBS <- - submitAuthnResponse' cookiehdr authnResp + submitAuthnResponse' cookiehdr tid authnResp pure (authnResp, sparAuthnResp) reBindDifferent :: HasCallStack => UserId -> TestSpar (SignedAuthnResponse, ResponseLBS) reBindDifferent uid = do env <- ask (SampleIdP metadata privcreds _ _) <- makeSampleIdPMetadata - idp <- call $ callIdpCreate (env ^. teSpar) (Just uid) metadata + idp <- call $ callIdpCreate (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid) metadata (_, authnResp, sparAuthnResp) <- initialBind uid idp privcreds pure (authnResp, sparAuthnResp) context "initial bind" $ do @@ -613,10 +614,10 @@ testGetPutDelete whichone = do -- the team, which is the original owner.) mkSsoOwner :: UserId -> TeamId -> IdP -> SignPrivCreds -> TestSpar UserId mkSsoOwner firstOwner tid idp privcreds = do - spmeta <- getTestSPMetadata + spmeta <- getTestSPMetadata tid authnreq <- negotiateAuthnRequest idp authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp spmeta authnreq True - loginresp <- submitAuthnResponse authnresp + loginresp <- submitAuthnResponse tid authnresp liftIO $ responseStatus loginresp `shouldBe` status200 [ssoOwner] <- filter (/= firstOwner) <$> getTeamMembers firstOwner tid promoteTeamMember firstOwner tid ssoOwner @@ -767,7 +768,7 @@ specCRUDIdentityProvider = do env <- ask (owner1, _, (^. idpId) -> idpid1, (IdPMetadataValue _ idpmeta1, _)) <- registerTestIdPWithMeta (SampleIdP idpmeta2 _ _ _) <- makeSampleIdPMetadata - _ <- call $ callIdpCreate (env ^. teSpar) (Just owner1) idpmeta2 + _ <- call $ callIdpCreate (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just owner1) idpmeta2 let idpmeta1' = idpmeta1 & edIssuer .~ (idpmeta2 ^. edIssuer) callIdpUpdate' (env ^. teSpar) (Just owner1) idpid1 (IdPMetadataValue (cs $ SAML.encode idpmeta1') undefined) `shouldRespondWith` checkErrHspec 400 "idp-issuer-in-use" @@ -917,12 +918,13 @@ specCRUDIdentityProvider = do check :: HasCallStack => Bool -> Int -> String -> Either String () -> TestSpar () check useNewPrivKey expectedStatus expectedHtmlTitle expectedCookie = do (idp, oldPrivKey, newPrivKey) <- initidp + let tid = idp ^. idpExtraInfo . wiTeam env <- ask (_, authnreq) <- call $ callAuthnReq (env ^. teSpar) (idp ^. idpId) - spmeta <- getTestSPMetadata + spmeta <- getTestSPMetadata tid let privkey = if useNewPrivKey then newPrivKey else oldPrivKey idpresp <- runSimpleSP $ mkAuthnResponse privkey idp spmeta authnreq True - sparresp <- submitAuthnResponse idpresp + sparresp <- submitAuthnResponse tid idpresp liftIO $ do statusCode sparresp `shouldBe` expectedStatus let bdy = maybe "" (cs @LBS @String) (responseBody sparresp) @@ -946,7 +948,7 @@ specCRUDIdentityProvider = do env <- ask (uid, _tid) <- call $ createUserWithTeamDisableSSO (env ^. teBrig) (env ^. teGalley) (SampleIdP metadata _ _ _) <- makeSampleIdPMetadata - callIdpCreate' (env ^. teSpar) (Just uid) metadata + callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid) metadata `shouldRespondWith` checkErrHspec 403 "sso-disabled" context "bad xml" $ do it "responds with a 'client error'" $ do @@ -957,14 +959,14 @@ specCRUDIdentityProvider = do it "responds with 'client error'" $ do env <- ask (SampleIdP idpmeta _ _ _) <- makeSampleIdPMetadata - callIdpCreate' (env ^. teSpar) Nothing idpmeta + callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) Nothing idpmeta `shouldRespondWith` checkErrHspec 400 "client-error" context "zuser has no team" $ do it "responds with 'no team member'" $ do env <- ask (uid, _) <- call $ createRandomPhoneUser (env ^. teBrig) (SampleIdP idpmeta _ _ _) <- makeSampleIdPMetadata - callIdpCreate' (env ^. teSpar) (Just uid) idpmeta + callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid) idpmeta `shouldRespondWith` checkErrHspec 403 "no-team-member" context "zuser is a team member, but not a team owner" $ do it "responds with 'insufficient-permissions' and a helpful message" $ do @@ -973,7 +975,7 @@ specCRUDIdentityProvider = do newmember <- let perms = Galley.noPermissions in call $ createTeamMember (env ^. teBrig) (env ^. teGalley) tid perms - callIdpCreate' (env ^. teSpar) (Just newmember) (idp ^. idpMetadata) + callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just newmember) (idp ^. idpMetadata) `shouldRespondWith` checkErrHspec 403 "insufficient-permissions" context "idp (identified by issuer) is in use by other team" $ do it "rejects" $ do @@ -981,9 +983,9 @@ specCRUDIdentityProvider = do (SampleIdP newMetadata _ _ _) <- makeSampleIdPMetadata (uid1, _) <- call $ createUserWithTeam (env ^. teBrig) (env ^. teGalley) (uid2, _) <- call $ createUserWithTeam (env ^. teBrig) (env ^. teGalley) - resp1 <- call $ callIdpCreate' (env ^. teSpar) (Just uid1) newMetadata - resp2 <- call $ callIdpCreate' (env ^. teSpar) (Just uid1) newMetadata - resp3 <- call $ callIdpCreate' (env ^. teSpar) (Just uid2) newMetadata + resp1 <- call $ callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid1) newMetadata + resp2 <- call $ callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid1) newMetadata + resp3 <- call $ callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid2) newMetadata liftIO $ do statusCode resp1 `shouldBe` 201 statusCode resp2 `shouldBe` 400 @@ -995,7 +997,7 @@ specCRUDIdentityProvider = do env <- ask (owner, _) <- call $ createUserWithTeam (env ^. teBrig) (env ^. teGalley) (SampleIdP metadata _ _ _) <- makeSampleIdPMetadata - idp <- call $ callIdpCreate (env ^. teSpar) (Just owner) metadata + idp <- call $ callIdpCreate (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just owner) metadata idp' <- call $ callIdpGet (env ^. teSpar) (Just owner) (idp ^. idpId) rawmeta <- call $ callIdpGetRaw (env ^. teSpar) (Just owner) (idp ^. idpId) liftIO $ do @@ -1179,10 +1181,11 @@ specDeleteCornerCases = describe "delete corner cases" $ do createViaSamlResp :: HasCallStack => IdP -> SignPrivCreds -> SAML.UserRef -> TestSpar ResponseLBS createViaSamlResp idp privCreds (SAML.UserRef _ subj) = do + let tid = idp ^. idpExtraInfo . wiTeam authnReq <- negotiateAuthnRequest idp - spmeta <- getTestSPMetadata + spmeta <- getTestSPMetadata tid authnResp <- runSimpleSP $ mkAuthnResponseWithSubj subj privCreds idp spmeta authnReq True - createResp <- submitAuthnResponse authnResp + createResp <- submitAuthnResponse tid authnResp liftIO $ responseStatus createResp `shouldBe` status200 pure createResp @@ -1205,7 +1208,7 @@ specScimAndSAML = do it "SCIM and SAML work together and SCIM-created users can login" $ do env <- ask -- create a user via scim - (tok, (_, _, idp, (_, privcreds))) <- ScimT.registerIdPAndScimTokenWithMeta + (tok, (_, tid, idp, (_, privcreds))) <- ScimT.registerIdPAndScimTokenWithMeta (usr, subj) <- ScimT.randomScimUserWithSubject scimStoredUser <- ScimT.createUser tok usr let userid :: UserId = ScimT.scimUserId scimStoredUser @@ -1221,9 +1224,9 @@ specScimAndSAML = do liftIO $ ('r', preview veidUref <$$> (Intra.veidFromUserSSOId <$> userssoid)) `shouldBe` ('r', Just (Right (Just userref))) -- login a user for the first time with the scim-supplied credentials authnreq <- negotiateAuthnRequest idp - spmeta <- getTestSPMetadata + spmeta <- getTestSPMetadata tid authnresp :: SignedAuthnResponse <- runSimpleSP $ mkAuthnResponseWithSubj subject privcreds idp spmeta authnreq True - sparresp :: ResponseLBS <- submitAuthnResponse authnresp + sparresp :: ResponseLBS <- submitAuthnResponse tid authnresp -- user should receive a cookie liftIO $ statusCode sparresp `shouldBe` 200 setcky :: SAML.SimpleSetCookie "zuid" <- @@ -1263,7 +1266,7 @@ specScimAndSAML = do mkNameID subj (Just "https://federation.foobar.com/nidp/saml2/metadata") (Just "https://prod-nginz-https.wire.com/sso/finalize-login") Nothing authnreq <- negotiateAuthnRequest idp - spmeta <- getTestSPMetadata + spmeta <- getTestSPMetadata (idp ^. idpExtraInfo . wiTeam) authnresp :: SignedAuthnResponse <- runSimpleSP $ mkAuthnResponseWithSubj subjectWithQualifier privcreds idp spmeta authnreq True ssoid <- getSsoidViaAuthResp authnresp @@ -1398,7 +1401,7 @@ specSparUserMigration = do env <- ask (_ownerid, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta - spmeta <- getTestSPMetadata + spmeta <- getTestSPMetadata tid (issuer, subject) <- do suffix <- cs <$> replicateM 7 (getRandomR ('a', 'z')) @@ -1421,7 +1424,7 @@ specSparUserMigration = do mbUserId <- do authnreq <- negotiateAuthnRequest idp authnresp <- runSimpleSP $ mkAuthnResponseWithSubj subject privcreds idp spmeta authnreq True - sparresp <- submitAuthnResponse authnresp + sparresp <- submitAuthnResponse tid authnresp liftIO $ statusCode sparresp `shouldBe` 200 ssoid <- getSsoidViaAuthResp authnresp ssoToUidSpar tid ssoid diff --git a/services/spar/test-integration/Test/Spar/AppSpec.hs b/services/spar/test-integration/Test/Spar/AppSpec.hs index 4e8b84468c..a2abfd28d0 100644 --- a/services/spar/test-integration/Test/Spar/AppSpec.hs +++ b/services/spar/test-integration/Test/Spar/AppSpec.hs @@ -43,6 +43,7 @@ import URI.ByteString.QQ (uri) import Util import Web.Cookie import Wire.API.User.IdentityProvider (IdP) +import qualified Wire.API.User.IdentityProvider as User spec :: SpecWith TestEnv spec = describe "accessVerdict" $ do @@ -152,7 +153,7 @@ requestAccessVerdict idp isGranted mkAuthnReq = do raw <- mkAuthnReq (idp ^. SAML.idpId) bdy <- maybe (error "authreq") pure $ responseBody raw either (error . show) pure $ Servant.mimeUnrender (Servant.Proxy @SAML.HTML) bdy - spmeta <- getTestSPMetadata + spmeta <- getTestSPMetadata (idp ^. idpExtraInfo . User.wiTeam) (privKey, _, _) <- DSig.mkSignCredsWithCert Nothing 96 authnresp :: SAML.AuthnResponse <- do case authnreq of diff --git a/services/spar/test-integration/Test/Spar/DataSpec.hs b/services/spar/test-integration/Test/Spar/DataSpec.hs index bc4d8f418a..d0291d1979 100644 --- a/services/spar/test-integration/Test/Spar/DataSpec.hs +++ b/services/spar/test-integration/Test/Spar/DataSpec.hs @@ -170,12 +170,12 @@ spec = do it "getIdPConfigByIssuer works" $ do idp <- makeTestIdP () <- runSparCass $ Data.storeIdPConfig idp - midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) + midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) (Just $ idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Just idp it "getIdPIdByIssuer works" $ do idp <- makeTestIdP () <- runSparCass $ Data.storeIdPConfig idp - midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) + midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (Just $ idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Just (idp ^. idpId) it "getIdPConfigsByTeam works" $ do teamid <- nextWireId @@ -195,10 +195,10 @@ spec = do midp <- runSparCass $ Data.getIdPConfig (idp ^. idpId) liftIO $ midp `shouldBe` Nothing do - midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) + midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) (Just $ idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Nothing do - midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) + midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (Just $ idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Nothing do idps <- runSparCass $ Data.getIdPConfigsByTeam teamid @@ -302,7 +302,7 @@ testDeleteTeam = it "cleans up all the right tables after deletion" $ do -- The config from 'issuer_idp': do let issuer = idp ^. SAML.idpMetadata . SAML.edIssuer - mbIdp <- runSparCass $ Data.getIdPIdByIssuer issuer + mbIdp <- runSparCass $ Data.getIdPIdByIssuer issuer (Just $ idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ mbIdp `shouldBe` Nothing -- The config from 'team_idp': do diff --git a/services/spar/test-integration/Test/Spar/Scim/AuthSpec.hs b/services/spar/test-integration/Test/Spar/Scim/AuthSpec.hs index 2e9056cea2..017c9fb0a3 100644 --- a/services/spar/test-integration/Test/Spar/Scim/AuthSpec.hs +++ b/services/spar/test-integration/Test/Spar/Scim/AuthSpec.hs @@ -133,9 +133,10 @@ testNumIdPs = do let addSomeIdP :: TestSpar () addSomeIdP = do - spar <- asks (view teSpar) + let spar = env ^. teSpar + legacyMode = env ^. teLegacySAMLEndPoints SAML.SampleIdP metadata _ _ _ <- SAML.makeSampleIdPMetadata - void $ call $ Util.callIdpCreate spar (Just owner) metadata + void $ call $ Util.callIdpCreate legacyMode spar (Just owner) metadata createToken owner (CreateScimToken "eins" (Just defPassword)) >>= deleteToken owner . stiId . createScimTokenResponseInfo diff --git a/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs b/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs index 8b742be0a6..4ade02f3eb 100644 --- a/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs +++ b/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs @@ -78,6 +78,7 @@ import qualified Web.Scim.Schema.User as Scim.User import qualified Wire.API.Team.Export as CsvExport import Wire.API.Team.Invitation (Invitation (..)) import Wire.API.User.IdentityProvider (IdP) +import qualified Wire.API.User.IdentityProvider as User import Wire.API.User.RichInfo import qualified Wire.API.User.Saml as Spar.Types import qualified Wire.API.User.Scim as Spar.Types @@ -685,11 +686,12 @@ testScimCreateVsUserRef = do createViaSamlResp :: HasCallStack => IdP -> SAML.SignPrivCreds -> SAML.UserRef -> TestSpar ResponseLBS createViaSamlResp idp privCreds (SAML.UserRef _ subj) = do authnReq <- negotiateAuthnRequest idp - spmeta <- getTestSPMetadata + let tid = idp ^. SAML.idpExtraInfo . User.wiTeam + spmeta <- getTestSPMetadata tid authnResp <- runSimpleSP $ SAML.mkAuthnResponseWithSubj subj privCreds idp spmeta authnReq True - submitAuthnResponse authnResp IdP -> SAML.SignPrivCreds -> SAML.UserRef -> TestSpar () createViaSamlFails idp privCreds uref = do diff --git a/services/spar/test-integration/Util/Core.hs b/services/spar/test-integration/Util/Core.hs index a31d8cab8b..a517f93ec6 100644 --- a/services/spar/test-integration/Util/Core.hs +++ b/services/spar/test-integration/Util/Core.hs @@ -253,6 +253,7 @@ mkEnv _teTstOpts _teOpts = do _teGalley = endpointToReq (cfgGalley _teTstOpts) _teSpar = endpointToReq (cfgSpar _teTstOpts) _teSparEnv = Spar.Env {..} + _teLegacySAMLEndPoints = False sparCtxOpts = _teOpts sparCtxCas = _teCql sparCtxHttpManager = _teMgr @@ -744,10 +745,17 @@ makeTestIdP = do <*> (pure md) <*> nextWireIdP -getTestSPMetadata :: (HasCallStack, MonadReader TestEnv m, MonadIO m) => m SPMetadata -getTestSPMetadata = do +getTestSPMetadata :: (HasCallStack, MonadReader TestEnv m, MonadIO m) => TeamId -> m SPMetadata +getTestSPMetadata tid = do env <- ask - resp <- call . get $ (env ^. teSpar) . path "/sso/metadata" . expect2xx + resp <- + call . get $ + (env ^. teSpar) + . ( if env ^. teLegacySAMLEndPoints + then path "/sso/metadata" + else paths ["/sso/metadata", toByteString' tid] + ) + . expect2xx raw <- maybe (crash_ "no body") (pure . cs) $ responseBody resp either (crash_ . show) pure (SAML.decode raw) where @@ -774,7 +782,7 @@ registerTestIdPWithMeta = do -- | Helper for 'registerTestIdP'. registerTestIdPFrom :: - (HasCallStack, MonadIO m) => + (HasCallStack, MonadIO m, MonadReader TestEnv m) => IdPMetadata -> Manager -> BrigReq -> @@ -782,9 +790,10 @@ registerTestIdPFrom :: SparReq -> m (UserId, TeamId, IdP) registerTestIdPFrom metadata mgr brig galley spar = do + legacyMode <- asks (^. teLegacySAMLEndPoints) liftIO . runHttpT mgr $ do (uid, tid) <- createUserWithTeam brig galley - (uid,tid,) <$> callIdpCreate spar (Just uid) metadata + (uid,tid,) <$> callIdpCreate legacyMode spar (Just uid) metadata getCookie :: KnownSymbol name => proxy name -> ResponseLBS -> Either String (SAML.SimpleSetCookie name) getCookie proxy rsp = do @@ -838,10 +847,11 @@ isSetBindCookie (SetBindCookie (SimpleSetCookie cky)) = do tryLogin :: HasCallStack => SignPrivCreds -> IdP -> NameID -> TestSpar SAML.UserRef tryLogin privkey idp userSubject = do env <- ask - spmeta <- getTestSPMetadata + let tid = idp ^. idpExtraInfo . wiTeam + spmeta <- getTestSPMetadata tid (_, authnreq) <- call $ callAuthnReq (env ^. teSpar) (idp ^. SAML.idpId) idpresp <- runSimpleSP $ mkAuthnResponseWithSubj userSubject privkey idp spmeta authnreq True - sparresp <- submitAuthnResponse idpresp + sparresp <- submitAuthnResponse tid idpresp liftIO $ do statusCode sparresp `shouldBe` 200 let bdy = maybe "" (cs @LBS @String) (responseBody sparresp) @@ -852,10 +862,11 @@ tryLogin privkey idp userSubject = do tryLoginFail :: HasCallStack => SignPrivCreds -> IdP -> NameID -> String -> TestSpar () tryLoginFail privkey idp userSubject bodyShouldContain = do env <- ask - spmeta <- getTestSPMetadata + let tid = idp ^. idpExtraInfo . wiTeam + spmeta <- getTestSPMetadata tid (_, authnreq) <- call $ callAuthnReq (env ^. teSpar) (idp ^. SAML.idpId) idpresp <- runSimpleSP $ mkAuthnResponseWithSubj userSubject privkey idp spmeta authnreq True - sparresp <- submitAuthnResponse idpresp + sparresp <- submitAuthnResponse tid idpresp liftIO $ do let bdy = maybe "" (cs @LBS @String) (responseBody sparresp) bdy `shouldContain` bodyShouldContain @@ -899,6 +910,7 @@ negotiateAuthnRequest' (doInitiatePath -> doInit) idp modreq = do submitAuthnResponse :: (HasCallStack, MonadIO m, MonadReader TestEnv m) => + TeamId -> SignedAuthnResponse -> m ResponseLBS submitAuthnResponse = submitAuthnResponse' id @@ -906,13 +918,18 @@ submitAuthnResponse = submitAuthnResponse' id submitAuthnResponse' :: (HasCallStack, MonadIO m, MonadReader TestEnv m) => (Request -> Request) -> + TeamId -> SignedAuthnResponse -> m ResponseLBS -submitAuthnResponse' reqmod (SignedAuthnResponse authnresp) = do +submitAuthnResponse' reqmod tid (SignedAuthnResponse authnresp) = do env <- ask req :: Request <- formDataBody [partLBS "SAMLResponse" . EL.encode . XML.renderLBS XML.def $ authnresp] empty - call $ post' req (reqmod . (env ^. teSpar) . path "/sso/finalize-login/") + let p = + if env ^. teLegacySAMLEndPoints + then path "/sso/finalize-login/" + else paths ["/sso/finalize-login", toByteString' tid] + call $ post' req (reqmod . (env ^. teSpar) . p) loginSsoUserFirstTime :: (HasCallStack, MonadIO m, MonadReader TestEnv m) => @@ -938,10 +955,11 @@ loginCreatedSsoUser :: m (UserId, Cookie) loginCreatedSsoUser nameid idp privCreds = do env <- ask + let tid = idp ^. idpExtraInfo . wiTeam authnReq <- negotiateAuthnRequest idp - spmeta <- getTestSPMetadata + spmeta <- getTestSPMetadata tid authnResp <- runSimpleSP $ mkAuthnResponseWithSubj nameid privCreds idp spmeta authnReq True - sparAuthnResp <- submitAuthnResponse authnResp + sparAuthnResp <- submitAuthnResponse tid authnResp let wireCookie = maybe (error (show sparAuthnResp)) id . lookup "Set-Cookie" $ responseHeaders sparAuthnResp accessResp :: ResponseLBS <- @@ -1051,18 +1069,19 @@ callIdpGetAll' :: (MonadIO m, MonadHttp m) => SparReq -> Maybe UserId -> m Respo callIdpGetAll' sparreq_ muid = do get $ sparreq_ . maybe id zUser muid . path "/identity-providers" -callIdpCreate :: (MonadIO m, MonadHttp m) => SparReq -> Maybe UserId -> SAML.IdPMetadata -> m IdP -callIdpCreate sparreq_ muid metadata = do - resp <- callIdpCreate' (sparreq_ . expect2xx) muid metadata +callIdpCreate :: (MonadIO m, MonadHttp m) => Bool -> SparReq -> Maybe UserId -> SAML.IdPMetadata -> m IdP +callIdpCreate legacyMode sparreq_ muid metadata = do + resp <- callIdpCreate' legacyMode (sparreq_ . expect2xx) muid metadata either (liftIO . throwIO . ErrorCall . show) pure $ responseJsonEither @IdP resp -callIdpCreate' :: (MonadIO m, MonadHttp m) => SparReq -> Maybe UserId -> SAML.IdPMetadata -> m ResponseLBS -callIdpCreate' sparreq_ muid metadata = do +callIdpCreate' :: (MonadIO m, MonadHttp m) => Bool -> SparReq -> Maybe UserId -> SAML.IdPMetadata -> m ResponseLBS +callIdpCreate' legacyMode sparreq_ muid metadata = do post $ sparreq_ . maybe id zUser muid . path "/identity-providers/" + . Bilge.query [("legacy", Just "true") | legacyMode] . body (RequestBodyLBS . cs $ SAML.encode metadata) . header "Content-Type" "application/xml" diff --git a/services/spar/test-integration/Util/Types.hs b/services/spar/test-integration/Util/Types.hs index a727e80382..377888038b 100644 --- a/services/spar/test-integration/Util/Types.hs +++ b/services/spar/test-integration/Util/Types.hs @@ -31,6 +31,7 @@ module Util.Types teSparEnv, teOpts, teTstOpts, + teLegacySAMLEndPoints, Select, ResponseLBS, IntegrationConfig (..), @@ -41,7 +42,7 @@ where import Bilge import Cassandra as Cas import Control.Exception -import Control.Lens (makeLenses) +import Control.Lens (makeLenses, (^.)) import Crypto.Random.Types (MonadRandom (..)) import Data.Aeson import qualified Data.Aeson as Aeson @@ -51,6 +52,7 @@ import Imports import SAML2.WebSSO.Types.TH (deriveJSONOptions) import Spar.API () import qualified Spar.App as Spar +import Test.Hspec (pendingWith) import Util.Options import Wire.API.User.Saml @@ -76,7 +78,14 @@ data TestEnv = TestEnv -- | spar config _teOpts :: Opts, -- | integration test config - _teTstOpts :: IntegrationConfig + _teTstOpts :: IntegrationConfig, + -- | If True, run tests against legacy SAML API where team is derived from idp issuer + -- instead of teamid. See Section "using the same IdP (same entityID, or Issuer) with + -- different teams" in "/docs/reference/spar-braindump.md" for more details. + -- + -- NB: this has no impact on the tested spar code; the rest API supports both legacy and + -- multi-sp mode. this falg merely determines how the rest API is used. + _teLegacySAMLEndPoints :: Bool } type Select = TestEnv -> (Request -> Request) @@ -108,3 +117,9 @@ _unitTestTestErrorLabel = do unless (val == Right "not-found") $ throwIO . ErrorCall . show $ val + +skipInLegacyRun :: (MonadIO m, MonadReader TestEnv m) => m () +skipInLegacyRun = do + asks (^. teLegacySAMLEndPoints) >>= \case + True -> liftIO $ pendingWith "skipping this test case in legacy run (makes no difference)" + False -> pure () From fc588ebae273ad81068226ff4fd7f95d6a205b49 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 9 Sep 2021 11:25:45 +0200 Subject: [PATCH 05/52] ... --- libs/wire-api/src/Wire/API/User/IdentityProvider.hs | 12 ++++++++++++ services/spar/src/Spar/API.hs | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs b/libs/wire-api/src/Wire/API/User/IdentityProvider.hs index 868c59ab6f..933769e519 100644 --- a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs +++ b/libs/wire-api/src/Wire/API/User/IdentityProvider.hs @@ -43,6 +43,7 @@ data WireIdP = WireIdP -- | list of issuer names that this idp has replaced, most recent first. this is used -- for finding users that are still stored under the old issuer, see -- 'findUserWithOldIssuer', 'moveUserToNewIssuer'. + _wiApiVersion :: Maybe WireIdPAPIVersion, _wiOldIssuers :: [SAML.Issuer], -- | the issuer that has replaced this one. this is set iff a new issuer is created -- with the @"replaces"@ query parameter, and it is used to decide whether users not @@ -51,8 +52,16 @@ data WireIdP = WireIdP } deriving (Eq, Show, Generic) +data WireIdPAPIVersion + = -- | initial API + WireIdPAPIV1 + | -- | support for different SP entityIDs per team + WireIdPAPIV2 + deriving (Eq, Show, Generic) + makeLenses ''WireIdP +deriveJSON deriveJSONOptions ''WireIdPAPIVersion deriveJSON deriveJSONOptions ''WireIdP -- | A list of 'IdP's, returned by some endpoints. Wrapped into an object to @@ -104,6 +113,9 @@ instance ToJSON IdPMetadataInfo where instance ToSchema IdPList where declareNamedSchema = genericDeclareNamedSchema samlSchemaOptions +instance ToSchema WireIdPAPIVersion where + declareNamedSchema = genericDeclareNamedSchema samlSchemaOptions + instance ToSchema WireIdP where declareNamedSchema = genericDeclareNamedSchema samlSchemaOptions diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 3a2a38f42e..3e6ac7f25d 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -173,7 +173,7 @@ validateRedirectURL uri = do throwSpar $ SparBadInitiateLoginQueryParams "url-too-long" authresp :: Maybe TeamId -> Maybe ST -> SAML.AuthnResponseBody -> Spar Void -authresp mbtid ckyraw arbody = logErrors $ SAML.authresp (sparSPIssuer mbtid) (sparResponseURI mbtid) go arbody +authresp mbtid ckyraw arbody = logErrors $ SAML.authresp mbtid (sparSPIssuer mbtid) (sparResponseURI mbtid) go arbody where cky :: Maybe BindCookie cky = ckyraw >>= bindCookieFromHeader From 182af838ede8b9c11baf68fbb7c44bd6e4418fd0 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 9 Sep 2021 11:51:32 +0200 Subject: [PATCH 06/52] ... --- services/spar/schema/src/V15.hs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/services/spar/schema/src/V15.hs b/services/spar/schema/src/V15.hs index ca7290cfbf..6ead409623 100644 --- a/services/spar/schema/src/V15.hs +++ b/services/spar/schema/src/V15.hs @@ -25,7 +25,7 @@ import Imports import Text.RawString.QQ migration :: Migration -migration = Migration 15 "Optionally index IdP by teamid (add. to entityID)" $ do +migration = Migration 15 "Optionally index IdP by teamid (in addition to entityID); add idp api version." $ do void $ schema' [r| @@ -36,3 +36,8 @@ migration = Migration 15 "Optionally index IdP by teamid (add. to entityID)" $ d , PRIMARY KEY (issuer, team) ) with compaction = {'class': 'LeveledCompactionStrategy'}; |] + void $ + schema' + [r| + ALTER TABLE idp ADD api_version int; + |] From 47c377ea93191584ed12e67ee114261facb40abb Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 9 Sep 2021 13:43:32 +0200 Subject: [PATCH 07/52] ... --- .../src/Wire/API/User/IdentityProvider.hs | 19 +++++- services/spar/src/Spar/API.hs | 2 +- services/spar/src/Spar/App.hs | 5 +- services/spar/src/Spar/Data.hs | 63 ++++++++++++++++--- services/spar/test-integration/Spec.hs | 12 ++-- .../test-integration/Test/Spar/APISpec.hs | 22 +++---- .../test-integration/Test/Spar/DataSpec.hs | 15 ++--- .../Test/Spar/Scim/AuthSpec.hs | 4 +- services/spar/test-integration/Util/Core.hs | 40 ++++++------ services/spar/test-integration/Util/Types.hs | 15 ++--- services/spar/test/Arbitrary.hs | 5 +- 11 files changed, 136 insertions(+), 66 deletions(-) diff --git a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs b/libs/wire-api/src/Wire/API/User/IdentityProvider.hs index 933769e519..f1a897fa26 100644 --- a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs +++ b/libs/wire-api/src/Wire/API/User/IdentityProvider.hs @@ -17,6 +17,8 @@ module Wire.API.User.IdentityProvider where +import qualified Cassandra as Cql +import Control.Exception (assert) import Control.Lens (makeLenses, (.~), (?~)) import Control.Monad.Except import Data.Aeson @@ -57,13 +59,28 @@ data WireIdPAPIVersion WireIdPAPIV1 | -- | support for different SP entityIDs per team WireIdPAPIV2 - deriving (Eq, Show, Generic) + deriving (Eq, Show, Enum, Bounded, Generic) + +defWireIdPAPIVersion :: WireIdPAPIVersion +defWireIdPAPIVersion = WireIdPAPIV2 makeLenses ''WireIdP deriveJSON deriveJSONOptions ''WireIdPAPIVersion deriveJSON deriveJSONOptions ''WireIdP +instance Cql.Cql WireIdPAPIVersion where + ctype = Cql.Tagged Cql.IntColumn + + toCql WireIdPAPIV1 = Cql.CqlInt 1 + toCql WireIdPAPIV2 = Cql.CqlInt 2 + + fromCql (Cql.CqlInt i) = case i of + 1 -> return WireIdPAPIV1 + 2 -> return WireIdPAPIV2 + n -> Left $ "Unexpected ClientCapability value: " ++ show n + fromCql _ = Left "ClientCapability value: int expected" + -- | A list of 'IdP's, returned by some endpoints. Wrapped into an object to -- allow extensibility later on. data IdPList = IdPList diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 3e6ac7f25d..1e7fd6c0ad 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -273,7 +273,7 @@ idpDelete zusr idpid (fromMaybe False -> purge) = withDebugLog "idpDelete" (cons updateReplacingIdP :: IdP -> Spar () updateReplacingIdP idp = forM_ (idp ^. SAML.idpExtraInfo . wiOldIssuers) $ \oldIssuer -> do wrapMonadClient $ do - iid <- Data.getIdPIdByIssuer oldIssuer + iid <- Data.getIdPIdByIssuerAllowOld oldIssuer (idp ^. SAML.idpExtraInfo . wiTeam) mapM_ (Data.clearReplacedBy . Data.Replaced) iid -- | This handler only does the json parsing, and leaves all authorization checks and diff --git a/services/spar/src/Spar/App.hs b/services/spar/src/Spar/App.hs index 2b703cec66..d2ccb2ab01 100644 --- a/services/spar/src/Spar/App.hs +++ b/services/spar/src/Spar/App.hs @@ -154,9 +154,12 @@ instance SPStoreIdP SparError Spar where getIdPConfig :: IdPId -> Spar IdP getIdPConfig = (>>= maybe (throwSpar SparIdPNotFound) pure) . wrapMonadClientWithEnv . Data.getIdPConfig - getIdPConfigByIssuer :: Issuer -> Maybe TeamId -> Spar IdP + getIdPConfigByIssuer :: Issuer -> TeamId -> Spar IdP getIdPConfigByIssuer issuer = (>>= maybe (throwSpar SparIdPNotFound) pure) . wrapMonadClientWithEnv . Data.getIdPConfigByIssuer issuer + getIdPConfigByIssuerOptionalSPId :: Issuer -> Maybe TeamId -> Spar IdP + getIdPConfigByIssuerOptionalSPId issuer = (>>= maybe (throwSpar SparIdPNotFound) pure) . wrapMonadClientWithEnv . Data.getIdPConfigByIssuerAllowOld issuer + -- | 'wrapMonadClient' with an 'Env' in a 'ReaderT', and exceptions. If you -- don't need either of those, 'wrapMonadClient' will suffice. wrapMonadClientWithEnv :: forall a. ReaderT Data.Env (ExceptT TTLError Cas.Client) a -> Spar a diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index f3e979c78a..6f2144aa44 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -58,7 +58,9 @@ module Spar.Data clearReplacedBy, getIdPConfig, getIdPConfigByIssuer, + getIdPConfigByIssuerAllowOld, getIdPIdByIssuer, + getIdPIdByIssuerAllowOld, getIdPConfigsByTeam, deleteIdPConfig, deleteTeam, @@ -409,7 +411,7 @@ lookupBindCookie (cs . fromBindCookie -> ckyval :: ST) = ---------------------------------------------------------------------- -- idp -type IdPConfigRow = (SAML.IdPId, SAML.Issuer, URI, SignedCertificate, [SignedCertificate], TeamId, [SAML.Issuer], Maybe SAML.IdPId) +type IdPConfigRow = (SAML.IdPId, SAML.Issuer, URI, SignedCertificate, [SignedCertificate], TeamId, Maybe WireIdPAPIVersion, [SAML.Issuer], Maybe SAML.IdPId) -- FUTUREWORK: should be called 'insertIdPConfig' for consistency. -- FUTUREWORK: enforce that wiReplacedby is Nothing, or throw an error. there is no @@ -431,12 +433,14 @@ storeIdPConfig idp = retry x5 . batch $ do NL.tail (idp ^. SAML.idpMetadata . SAML.edCertAuthnResponse), -- (the 'List1' is split up into head and tail to make migration from one-element-only easier.) idp ^. SAML.idpExtraInfo . wiTeam, + idp ^. SAML.idpExtraInfo . wiApiVersion, idp ^. SAML.idpExtraInfo . wiOldIssuers, idp ^. SAML.idpExtraInfo . wiReplacedBy ) addPrepQuery byIssuer ( idp ^. SAML.idpId, + idp ^. SAML.idpExtraInfo . wiTeam, idp ^. SAML.idpMetadata . SAML.edIssuer ) addPrepQuery @@ -446,9 +450,12 @@ storeIdPConfig idp = retry x5 . batch $ do ) where ins :: PrepQuery W IdPConfigRow () - ins = "INSERT INTO idp (idp, issuer, request_uri, public_key, extra_public_keys, team, old_issuers, replaced_by) VALUES (?, ?, ?, ?, ?, ?, ?, ?)" - byIssuer :: PrepQuery W (SAML.IdPId, SAML.Issuer) () - byIssuer = "INSERT INTO issuer_idp (idp, issuer) VALUES (?, ?)" + 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 (?, ? ?)" + byTeam :: PrepQuery W (SAML.IdPId, TeamId) () byTeam = "INSERT INTO team_idp (idp, team) VALUES (?, ?)" @@ -497,48 +504,84 @@ getIdPConfig idpid = certsTail, -- extras teamId, + apiVersion, oldIssuers, replacedBy ) = do let _edCertAuthnResponse = certsHead NL.:| certsTail _idpMetadata = SAML.IdPMetadata {..} - _idpExtraInfo = WireIdP teamId oldIssuers replacedBy + _idpExtraInfo = WireIdP teamId apiVersion oldIssuers replacedBy pure $ SAML.IdPConfig {..} sel :: PrepQuery R (Identity SAML.IdPId) IdPConfigRow - sel = "SELECT idp, issuer, request_uri, public_key, extra_public_keys, team, old_issuers, replaced_by FROM idp WHERE idp = ?" + sel = "SELECT idp, issuer, request_uri, public_key, extra_public_keys, team, api_version, old_issuers, replaced_by FROM idp WHERE idp = ?" +-- | See 'getIdPIdByIssuer'. getIdPConfigByIssuer :: + (HasCallStack, MonadClient m) => + SAML.Issuer -> + TeamId -> + m (Maybe IdP) +getIdPConfigByIssuer issuer team = do + getIdPIdByIssuer issuer team >>= \case + Nothing -> pure Nothing + Just idpid -> getIdPConfig idpid + +-- | See 'getIdPIdByIssuerAllowOld'. +getIdPConfigByIssuerAllowOld :: (HasCallStack, MonadClient m) => SAML.Issuer -> Maybe TeamId -> m (Maybe IdP) -getIdPConfigByIssuer issuer mbteam = do - getIdPIdByIssuer issuer mbteam >>= \case +getIdPConfigByIssuerAllowOld issuer mbteam = do + getIdPIdByIssuerAllowOld issuer mbteam >>= \case Nothing -> pure Nothing Just idpid -> getIdPConfig idpid -- | Lookup idp in table `issuer_idp_v2` (using both issuer entityID and teamid); if nothing -- is found there or if teamid is 'Nothing', lookup under issuer in `issuer_idp`. getIdPIdByIssuer :: + (HasCallStack, MonadClient m) => + SAML.Issuer -> + TeamId -> + m (Maybe SAML.IdPId) +getIdPIdByIssuer issuer team = do + mbv2 <- getIdPIdByIssuerV2 issuer team + mbboth <- maybe (getIdPIdByIssuerOld issuer) (pure . Just) mbv2 + pure mbboth + +-- | Like 'getIdPIdByIssuer', but do not require a 'TeamId'. If none is provided, see if a +-- single solution can be found without. +getIdPIdByIssuerAllowOld :: (HasCallStack, MonadClient m) => SAML.Issuer -> Maybe TeamId -> m (Maybe SAML.IdPId) -getIdPIdByIssuer issuer mbteam = do +getIdPIdByIssuerAllowOld issuer mbteam = do mbv2 <- maybe (pure Nothing) (getIdPIdByIssuerV2 issuer) mbteam mbboth <- maybe (getIdPIdByIssuerOld issuer) (pure . Just) mbv2 pure mbboth +-- | Find 'IdPId' without team. Search both `issuer_idp` and `issuer_idp_v2`; in the latter, +-- make sure the result is unique (no two IdPs for two different teams). getIdPIdByIssuerOld :: (HasCallStack, MonadClient m) => SAML.Issuer -> m (Maybe SAML.IdPId) getIdPIdByIssuerOld issuer = do - runIdentity <$$> retry x1 (query1 sel $ params Quorum (Identity issuer)) + (runIdentity <$$> retry x1 (query1 sel $ params Quorum (Identity issuer))) >>= \case + Just idpid -> pure $ Just idpid + Nothing -> + (runIdentity <$$> retry x1 (query selv2 $ params Quorum (Identity issuer))) >>= \case + [] -> pure Nothing + [idpid] -> pure $ Just idpid + (_ : _ : _) -> pure Nothing -- TODO: this should produce a better error! where sel :: PrepQuery R (Identity SAML.Issuer) (Identity SAML.IdPId) sel = "SELECT idp FROM issuer_idp WHERE issuer = ?" + selv2 :: PrepQuery R (Identity SAML.Issuer) (Identity SAML.IdPId) + selv2 = "SELECT idp FROM issuer_idp_v2 WHERE issuer = ?" + getIdPIdByIssuerV2 :: (HasCallStack, MonadClient m) => SAML.Issuer -> diff --git a/services/spar/test-integration/Spec.hs b/services/spar/test-integration/Spec.hs index 82a06df838..f74c9dc513 100644 --- a/services/spar/test-integration/Spec.hs +++ b/services/spar/test-integration/Spec.hs @@ -55,13 +55,11 @@ main = do (wireArgs, hspecArgs) <- partitionArgs <$> getArgs let env = withArgs wireArgs mkEnvFromOptions withArgs hspecArgs . hspec $ do - beforeAll env . afterAll destroyEnv $ do - mkspecMisc - mkspecSaml - mkspecScim - beforeAll (env <&> teLegacySAMLEndPoints .~ True) . afterAll destroyEnv $ do - mkspecSaml - mkspecScim + for_ [minBound ..] $ \idpApiVersion -> do + describe (show idpApiVersion) . beforeAll (env <&> teWireIdPAPIVersion .~ idpApiVersion) . afterAll destroyEnv $ do + mkspecMisc + mkspecSaml + mkspecScim mkspecHscimAcceptance env destroyEnv partitionArgs :: [String] -> ([String], [String]) diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 8f05fa135f..c638c67567 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -122,7 +122,7 @@ specMisc = do pure $ nonfresh & edIssuer .~ issuer env <- ask uid <- fst <$> call (createUserWithTeam (env ^. teBrig) (env ^. teGalley)) - resp <- call $ callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid) somemeta + resp <- call $ callIdpCreate' (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just uid) somemeta liftIO $ statusCode resp `shouldBe` if isHttps then 201 else 400 it "does not trigger on https urls" $ check True it "does trigger on http urls" $ check False @@ -509,7 +509,7 @@ specBindingUsers = describe "binding existing users to sso identities" $ do reBindDifferent uid = do env <- ask (SampleIdP metadata privcreds _ _) <- makeSampleIdPMetadata - idp <- call $ callIdpCreate (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid) metadata + idp <- call $ callIdpCreate (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just uid) metadata (_, authnResp, sparAuthnResp) <- initialBind uid idp privcreds pure (authnResp, sparAuthnResp) context "initial bind" $ do @@ -768,7 +768,7 @@ specCRUDIdentityProvider = do env <- ask (owner1, _, (^. idpId) -> idpid1, (IdPMetadataValue _ idpmeta1, _)) <- registerTestIdPWithMeta (SampleIdP idpmeta2 _ _ _) <- makeSampleIdPMetadata - _ <- call $ callIdpCreate (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just owner1) idpmeta2 + _ <- call $ callIdpCreate (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner1) idpmeta2 let idpmeta1' = idpmeta1 & edIssuer .~ (idpmeta2 ^. edIssuer) callIdpUpdate' (env ^. teSpar) (Just owner1) idpid1 (IdPMetadataValue (cs $ SAML.encode idpmeta1') undefined) `shouldRespondWith` checkErrHspec 400 "idp-issuer-in-use" @@ -948,7 +948,7 @@ specCRUDIdentityProvider = do env <- ask (uid, _tid) <- call $ createUserWithTeamDisableSSO (env ^. teBrig) (env ^. teGalley) (SampleIdP metadata _ _ _) <- makeSampleIdPMetadata - callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid) metadata + callIdpCreate' (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just uid) metadata `shouldRespondWith` checkErrHspec 403 "sso-disabled" context "bad xml" $ do it "responds with a 'client error'" $ do @@ -959,14 +959,14 @@ specCRUDIdentityProvider = do it "responds with 'client error'" $ do env <- ask (SampleIdP idpmeta _ _ _) <- makeSampleIdPMetadata - callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) Nothing idpmeta + callIdpCreate' (env ^. teWireIdPAPIVersion) (env ^. teSpar) Nothing idpmeta `shouldRespondWith` checkErrHspec 400 "client-error" context "zuser has no team" $ do it "responds with 'no team member'" $ do env <- ask (uid, _) <- call $ createRandomPhoneUser (env ^. teBrig) (SampleIdP idpmeta _ _ _) <- makeSampleIdPMetadata - callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid) idpmeta + callIdpCreate' (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just uid) idpmeta `shouldRespondWith` checkErrHspec 403 "no-team-member" context "zuser is a team member, but not a team owner" $ do it "responds with 'insufficient-permissions' and a helpful message" $ do @@ -975,7 +975,7 @@ specCRUDIdentityProvider = do newmember <- let perms = Galley.noPermissions in call $ createTeamMember (env ^. teBrig) (env ^. teGalley) tid perms - callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just newmember) (idp ^. idpMetadata) + callIdpCreate' (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just newmember) (idp ^. idpMetadata) `shouldRespondWith` checkErrHspec 403 "insufficient-permissions" context "idp (identified by issuer) is in use by other team" $ do it "rejects" $ do @@ -983,9 +983,9 @@ specCRUDIdentityProvider = do (SampleIdP newMetadata _ _ _) <- makeSampleIdPMetadata (uid1, _) <- call $ createUserWithTeam (env ^. teBrig) (env ^. teGalley) (uid2, _) <- call $ createUserWithTeam (env ^. teBrig) (env ^. teGalley) - resp1 <- call $ callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid1) newMetadata - resp2 <- call $ callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid1) newMetadata - resp3 <- call $ callIdpCreate' (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just uid2) newMetadata + resp1 <- call $ callIdpCreate' (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just uid1) newMetadata + resp2 <- call $ callIdpCreate' (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just uid1) newMetadata + resp3 <- call $ callIdpCreate' (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just uid2) newMetadata liftIO $ do statusCode resp1 `shouldBe` 201 statusCode resp2 `shouldBe` 400 @@ -997,7 +997,7 @@ specCRUDIdentityProvider = do env <- ask (owner, _) <- call $ createUserWithTeam (env ^. teBrig) (env ^. teGalley) (SampleIdP metadata _ _ _) <- makeSampleIdPMetadata - idp <- call $ callIdpCreate (env ^. teLegacySAMLEndPoints) (env ^. teSpar) (Just owner) metadata + idp <- call $ callIdpCreate (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner) metadata idp' <- call $ callIdpGet (env ^. teSpar) (Just owner) (idp ^. idpId) rawmeta <- call $ callIdpGetRaw (env ^. teSpar) (Just owner) (idp ^. idpId) liftIO $ do diff --git a/services/spar/test-integration/Test/Spar/DataSpec.hs b/services/spar/test-integration/Test/Spar/DataSpec.hs index d0291d1979..8a5d4a625f 100644 --- a/services/spar/test-integration/Test/Spar/DataSpec.hs +++ b/services/spar/test-integration/Test/Spar/DataSpec.hs @@ -170,22 +170,23 @@ spec = do it "getIdPConfigByIssuer works" $ do idp <- makeTestIdP () <- runSparCass $ Data.storeIdPConfig idp - midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) (Just $ idp ^. SAML.idpExtraInfo . wiTeam) + midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Just idp it "getIdPIdByIssuer works" $ do idp <- makeTestIdP () <- runSparCass $ Data.storeIdPConfig idp - midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (Just $ idp ^. SAML.idpExtraInfo . wiTeam) + midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Just (idp ^. idpId) it "getIdPConfigsByTeam works" $ do teamid <- nextWireId - idp <- makeTestIdP <&> idpExtraInfo .~ (WireIdP teamid [] Nothing) + idp <- makeTestIdP <&> idpExtraInfo .~ (WireIdP teamid Nothing [] Nothing) () <- runSparCass $ Data.storeIdPConfig idp idps <- runSparCass $ Data.getIdPConfigsByTeam teamid liftIO $ idps `shouldBe` [idp] it "deleteIdPConfig works" $ do teamid <- nextWireId - idp <- makeTestIdP <&> idpExtraInfo .~ (WireIdP teamid [] Nothing) + idpApiVersion <- asks (^. teWireIdPAPIVersion) + idp <- makeTestIdP <&> idpExtraInfo .~ (WireIdP teamid (Just idpApiVersion) [] Nothing) () <- runSparCass $ Data.storeIdPConfig idp do midp <- runSparCass $ Data.getIdPConfig (idp ^. idpId) @@ -195,10 +196,10 @@ spec = do midp <- runSparCass $ Data.getIdPConfig (idp ^. idpId) liftIO $ midp `shouldBe` Nothing do - midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) (Just $ idp ^. SAML.idpExtraInfo . wiTeam) + midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Nothing do - midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (Just $ idp ^. SAML.idpExtraInfo . wiTeam) + midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Nothing do idps <- runSparCass $ Data.getIdPConfigsByTeam teamid @@ -302,7 +303,7 @@ testDeleteTeam = it "cleans up all the right tables after deletion" $ do -- The config from 'issuer_idp': do let issuer = idp ^. SAML.idpMetadata . SAML.edIssuer - mbIdp <- runSparCass $ Data.getIdPIdByIssuer issuer (Just $ idp ^. SAML.idpExtraInfo . wiTeam) + mbIdp <- runSparCass $ Data.getIdPIdByIssuer issuer (idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ mbIdp `shouldBe` Nothing -- The config from 'team_idp': do diff --git a/services/spar/test-integration/Test/Spar/Scim/AuthSpec.hs b/services/spar/test-integration/Test/Spar/Scim/AuthSpec.hs index 017c9fb0a3..5be4bc24a2 100644 --- a/services/spar/test-integration/Test/Spar/Scim/AuthSpec.hs +++ b/services/spar/test-integration/Test/Spar/Scim/AuthSpec.hs @@ -134,9 +134,9 @@ testNumIdPs = do let addSomeIdP :: TestSpar () addSomeIdP = do let spar = env ^. teSpar - legacyMode = env ^. teLegacySAMLEndPoints + apiversion = env ^. teWireIdPAPIVersion SAML.SampleIdP metadata _ _ _ <- SAML.makeSampleIdPMetadata - void $ call $ Util.callIdpCreate legacyMode spar (Just owner) metadata + void $ call $ Util.callIdpCreate apiversion spar (Just owner) metadata createToken owner (CreateScimToken "eins" (Just defPassword)) >>= deleteToken owner . stiId . createScimTokenResponseInfo diff --git a/services/spar/test-integration/Util/Core.hs b/services/spar/test-integration/Util/Core.hs index a517f93ec6..6bad490497 100644 --- a/services/spar/test-integration/Util/Core.hs +++ b/services/spar/test-integration/Util/Core.hs @@ -253,7 +253,7 @@ mkEnv _teTstOpts _teOpts = do _teGalley = endpointToReq (cfgGalley _teTstOpts) _teSpar = endpointToReq (cfgSpar _teTstOpts) _teSparEnv = Spar.Env {..} - _teLegacySAMLEndPoints = False + _teWireIdPAPIVersion = WireIdPAPIV2 sparCtxOpts = _teOpts sparCtxCas = _teCql sparCtxHttpManager = _teMgr @@ -521,8 +521,8 @@ deleteUserNoWait brigreq uid = nextWireId :: MonadIO m => m (Id a) nextWireId = Id <$> liftIO UUID.nextRandom -nextWireIdP :: MonadIO m => m WireIdP -nextWireIdP = WireIdP <$> (Id <$> liftIO UUID.nextRandom) <*> pure [] <*> pure Nothing +nextWireIdP :: MonadIO m => WireIdPAPIVersion -> m WireIdP +nextWireIdP version = WireIdP <$> (Id <$> liftIO UUID.nextRandom) <*> pure (Just version) <*> pure [] <*> pure Nothing nextSAMLID :: MonadIO m => m (ID a) nextSAMLID = mkID . UUID.toText <$> liftIO UUID.nextRandom @@ -737,13 +737,14 @@ call req = ask >>= \env -> liftIO $ runHttpT (env ^. teMgr) req ping :: (Request -> Request) -> Http () ping req = void . get $ req . path "/i/status" . expect2xx -makeTestIdP :: (HasCallStack, MonadRandom m, MonadIO m) => m (IdPConfig WireIdP) +makeTestIdP :: (HasCallStack, MonadReader TestEnv m, MonadRandom m, MonadIO m) => m (IdPConfig WireIdP) makeTestIdP = do + apiversion <- asks (^. teWireIdPAPIVersion) SampleIdP md _ _ _ <- makeSampleIdPMetadata IdPConfig <$> (IdPId <$> liftIO UUID.nextRandom) <*> (pure md) - <*> nextWireIdP + <*> nextWireIdP apiversion getTestSPMetadata :: (HasCallStack, MonadReader TestEnv m, MonadIO m) => TeamId -> m SPMetadata getTestSPMetadata tid = do @@ -751,9 +752,9 @@ getTestSPMetadata tid = do resp <- call . get $ (env ^. teSpar) - . ( if env ^. teLegacySAMLEndPoints - then path "/sso/metadata" - else paths ["/sso/metadata", toByteString' tid] + . ( case env ^. teWireIdPAPIVersion of + WireIdPAPIV1 -> path "/sso/metadata" + WireIdPAPIV2 -> paths ["/sso/metadata", toByteString' tid] ) . expect2xx raw <- maybe (crash_ "no body") (pure . cs) $ responseBody resp @@ -790,7 +791,7 @@ registerTestIdPFrom :: SparReq -> m (UserId, TeamId, IdP) registerTestIdPFrom metadata mgr brig galley spar = do - legacyMode <- asks (^. teLegacySAMLEndPoints) + legacyMode <- asks (^. teWireIdPAPIVersion) liftIO . runHttpT mgr $ do (uid, tid) <- createUserWithTeam brig galley (uid,tid,) <$> callIdpCreate legacyMode spar (Just uid) metadata @@ -926,9 +927,9 @@ submitAuthnResponse' reqmod tid (SignedAuthnResponse authnresp) = do req :: Request <- formDataBody [partLBS "SAMLResponse" . EL.encode . XML.renderLBS XML.def $ authnresp] empty let p = - if env ^. teLegacySAMLEndPoints - then path "/sso/finalize-login/" - else paths ["/sso/finalize-login", toByteString' tid] + case env ^. teWireIdPAPIVersion of + WireIdPAPIV1 -> path "/sso/finalize-login/" + WireIdPAPIV2 -> paths ["/sso/finalize-login", toByteString' tid] call $ post' req (reqmod . (env ^. teSpar) . p) loginSsoUserFirstTime :: @@ -1069,19 +1070,22 @@ callIdpGetAll' :: (MonadIO m, MonadHttp m) => SparReq -> Maybe UserId -> m Respo callIdpGetAll' sparreq_ muid = do get $ sparreq_ . maybe id zUser muid . path "/identity-providers" -callIdpCreate :: (MonadIO m, MonadHttp m) => Bool -> SparReq -> Maybe UserId -> SAML.IdPMetadata -> m IdP -callIdpCreate legacyMode sparreq_ muid metadata = do - resp <- callIdpCreate' legacyMode (sparreq_ . expect2xx) muid metadata +callIdpCreate :: (MonadIO m, MonadHttp m) => WireIdPAPIVersion -> SparReq -> Maybe UserId -> SAML.IdPMetadata -> m IdP +callIdpCreate apiversion sparreq_ muid metadata = do + resp <- callIdpCreate' apiversion (sparreq_ . expect2xx) muid metadata either (liftIO . throwIO . ErrorCall . show) pure $ responseJsonEither @IdP resp -callIdpCreate' :: (MonadIO m, MonadHttp m) => Bool -> SparReq -> Maybe UserId -> SAML.IdPMetadata -> m ResponseLBS -callIdpCreate' legacyMode sparreq_ muid metadata = do +callIdpCreate' :: (MonadIO m, MonadHttp m) => WireIdPAPIVersion -> SparReq -> Maybe UserId -> SAML.IdPMetadata -> m ResponseLBS +callIdpCreate' apiversion sparreq_ muid metadata = do post $ sparreq_ . maybe id zUser muid . path "/identity-providers/" - . Bilge.query [("legacy", Just "true") | legacyMode] + . ( case apiversion of + WireIdPAPIV1 -> Bilge.query [("api", Just "v1")] + WireIdPAPIV2 -> id + ) . body (RequestBodyLBS . cs $ SAML.encode metadata) . header "Content-Type" "application/xml" diff --git a/services/spar/test-integration/Util/Types.hs b/services/spar/test-integration/Util/Types.hs index 377888038b..67b5631d62 100644 --- a/services/spar/test-integration/Util/Types.hs +++ b/services/spar/test-integration/Util/Types.hs @@ -31,11 +31,12 @@ module Util.Types teSparEnv, teOpts, teTstOpts, - teLegacySAMLEndPoints, + teWireIdPAPIVersion, Select, ResponseLBS, IntegrationConfig (..), TestErrorLabel (..), + skipIdPAPIVersions, ) where @@ -54,6 +55,7 @@ import Spar.API () import qualified Spar.App as Spar import Test.Hspec (pendingWith) import Util.Options +import Wire.API.User.IdentityProvider (WireIdPAPIVersion) import Wire.API.User.Saml type BrigReq = Request -> Request @@ -85,7 +87,7 @@ data TestEnv = TestEnv -- -- NB: this has no impact on the tested spar code; the rest API supports both legacy and -- multi-sp mode. this falg merely determines how the rest API is used. - _teLegacySAMLEndPoints :: Bool + _teWireIdPAPIVersion :: WireIdPAPIVersion } type Select = TestEnv -> (Request -> Request) @@ -118,8 +120,7 @@ _unitTestTestErrorLabel = do throwIO . ErrorCall . show $ val -skipInLegacyRun :: (MonadIO m, MonadReader TestEnv m) => m () -skipInLegacyRun = do - asks (^. teLegacySAMLEndPoints) >>= \case - True -> liftIO $ pendingWith "skipping this test case in legacy run (makes no difference)" - False -> pure () +skipIdPAPIVersions :: (MonadIO m, MonadReader TestEnv m) => [WireIdPAPIVersion] -> m () +skipIdPAPIVersions skip = do + asks (^. teWireIdPAPIVersion) >>= \vers -> when (vers `elem` skip) . liftIO $ do + pendingWith $ "skipping " <> show vers <> " for this test case (behavior covered by other versions)" diff --git a/services/spar/test/Arbitrary.hs b/services/spar/test/Arbitrary.hs index f80bac9baa..db741ce4c3 100644 --- a/services/spar/test/Arbitrary.hs +++ b/services/spar/test/Arbitrary.hs @@ -41,7 +41,10 @@ instance Arbitrary IdPList where pure $ IdPList {..} instance Arbitrary WireIdP where - arbitrary = WireIdP <$> arbitrary <*> arbitrary <*> arbitrary + arbitrary = WireIdP <$> arbitrary <*> arbitrary <*> arbitrary <*> arbitrary + +instance Arbitrary WireIdPAPIVersion where + arbitrary = elements [minBound ..] deriving instance Arbitrary ScimToken From d2c04673f3e7aa193863798dd7cd1b8c26b99f52 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 9 Sep 2021 14:50:34 +0200 Subject: [PATCH 08/52] ... --- .../src/Wire/API/Routes/Public/Spar.hs | 1 + .../src/Wire/API/User/IdentityProvider.hs | 1 - services/spar/src/Spar/API.hs | 21 ++++++++++--------- services/spar/src/Spar/App.hs | 14 ++++++------- services/spar/src/Spar/Options.hs | 5 ++++- services/spar/test-integration/Util/Core.hs | 2 +- 6 files changed, 24 insertions(+), 20 deletions(-) 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 5e9a110e91..5492a0be76 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Spar.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Spar.hs @@ -166,6 +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 :> 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 f1a897fa26..af9c2a25dd 100644 --- a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs +++ b/libs/wire-api/src/Wire/API/User/IdentityProvider.hs @@ -18,7 +18,6 @@ module Wire.API.User.IdentityProvider where import qualified Cassandra as Cql -import Control.Exception (assert) import Control.Lens (makeLenses, (.~), (?~)) import Control.Monad.Except import Data.Aeson diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 1e7fd6c0ad..971e68b257 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -273,21 +273,21 @@ idpDelete zusr idpid (fromMaybe False -> purge) = withDebugLog "idpDelete" (cons updateReplacingIdP :: IdP -> Spar () updateReplacingIdP idp = forM_ (idp ^. SAML.idpExtraInfo . wiOldIssuers) $ \oldIssuer -> do wrapMonadClient $ do - iid <- Data.getIdPIdByIssuerAllowOld oldIssuer (idp ^. SAML.idpExtraInfo . wiTeam) + iid <- Data.getIdPIdByIssuer oldIssuer (idp ^. SAML.idpExtraInfo . wiTeam) mapM_ (Data.clearReplacedBy . Data.Replaced) iid -- | This handler only does the json parsing, and leaves all authorization checks and -- application logic to 'idpCreateXML'. -idpCreate :: Maybe UserId -> IdPMetadataInfo -> Maybe SAML.IdPId -> Spar IdP -idpCreate zusr (IdPMetadataValue raw xml) midpid = idpCreateXML zusr raw xml midpid +idpCreate :: Maybe UserId -> IdPMetadataInfo -> Maybe SAML.IdPId -> Maybe WireIdPAPIVersion -> Spar IdP +idpCreate zusr (IdPMetadataValue raw xml) midpid apiversion = idpCreateXML zusr raw xml midpid apiversion -- | We generate a new UUID for each IdP used as IdPConfig's path, thereby ensuring uniqueness. -idpCreateXML :: Maybe UserId -> Text -> SAML.IdPMetadata -> Maybe SAML.IdPId -> Spar IdP -idpCreateXML zusr raw idpmeta mReplaces = withDebugLog "idpCreate" (Just . show . (^. SAML.idpId)) $ do +idpCreateXML :: Maybe UserId -> Text -> SAML.IdPMetadata -> Maybe SAML.IdPId -> Maybe WireIdPAPIVersion -> Spar IdP +idpCreateXML zusr raw idpmeta mReplaces (fromMaybe defWireIdPAPIVersion -> apiversion) = withDebugLog "idpCreate" (Just . show . (^. SAML.idpId)) $ do teamid <- Brig.getZUsrCheckPerm zusr CreateUpdateDeleteIdp Galley.assertSSOEnabled teamid assertNoScimOrNoIdP teamid - idp <- validateNewIdP idpmeta teamid mReplaces + idp <- validateNewIdP apiversion idpmeta teamid mReplaces wrapMonadClient $ Data.storeIdPRawMetadata (idp ^. SAML.idpId) raw SAML.storeIdPConfig idp forM_ mReplaces $ \replaces -> wrapMonadClient $ do @@ -326,11 +326,12 @@ assertNoScimOrNoIdP teamid = do validateNewIdP :: forall m. (HasCallStack, m ~ Spar) => + WireIdPAPIVersion -> SAML.IdPMetadata -> TeamId -> Maybe SAML.IdPId -> m IdP -validateNewIdP _idpMetadata teamId mReplaces = do +validateNewIdP apiversion _idpMetadata teamId mReplaces = do _idpId <- SAML.IdPId <$> SAML.createUUID oldIssuers :: [SAML.Issuer] <- case mReplaces of Nothing -> pure [] @@ -338,9 +339,9 @@ validateNewIdP _idpMetadata teamId mReplaces = do idp <- wrapMonadClient (Data.getIdPConfig replaces) >>= maybe (throwSpar SparIdPNotFound) pure pure $ (idp ^. SAML.idpMetadata . SAML.edIssuer) : (idp ^. SAML.idpExtraInfo . wiOldIssuers) let requri = _idpMetadata ^. SAML.edRequestURI - _idpExtraInfo = WireIdP teamId oldIssuers Nothing + _idpExtraInfo = WireIdP teamId (Just apiversion) oldIssuers Nothing enforceHttps requri - wrapMonadClient (Data.getIdPIdByIssuer (_idpMetadata ^. SAML.edIssuer)) >>= \case + wrapMonadClient (Data.getIdPIdByIssuer (_idpMetadata ^. SAML.edIssuer) teamId) >>= \case Nothing -> pure () Just _ -> throwSpar SparNewIdPAlreadyInUse pure SAML.IdPConfig {..} @@ -387,7 +388,7 @@ validateIdPUpdate zusr _idpMetadata _idpId = do if previousIssuer == newIssuer then pure $ previousIdP ^. SAML.idpExtraInfo else do - foundConfig <- wrapMonadClient (Data.getIdPConfigByIssuer newIssuer) + foundConfig <- wrapMonadClient (Data.getIdPConfigByIssuerAllowOld newIssuer (Just teamId)) let notInUseByOthers = case foundConfig of Nothing -> True Just c -> c ^. SAML.idpId == _idpId diff --git a/services/spar/src/Spar/App.hs b/services/spar/src/Spar/App.hs index d2ccb2ab01..784372dd5b 100644 --- a/services/spar/src/Spar/App.hs +++ b/services/spar/src/Spar/App.hs @@ -233,9 +233,9 @@ 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 :: UserId -> SAML.UserRef -> Spar () -createSamlUserWithId buid suid = do - teamid <- (^. idpExtraInfo . wiTeam) <$> getIdPConfigByIssuer (suid ^. uidTenant) +createSamlUserWithId :: IdP -> UserId -> SAML.UserRef -> Spar () +createSamlUserWithId idp buid suid = do + let teamid = idp ^. idpExtraInfo . wiTeam uname <- either (throwSpar . SparBadUserName . cs) pure $ Intra.mkUserName Nothing (UrefOnly suid) buid' <- Intra.createBrigUserSAML suid buid teamid uname ManagedByWire assert (buid == buid') $ pure () @@ -252,10 +252,10 @@ autoprovisionSamlUser suid = do -- | Like 'autoprovisionSamlUser', but for an already existing 'UserId'. autoprovisionSamlUserWithId :: UserId -> SAML.UserRef -> Spar () autoprovisionSamlUserWithId buid suid = do - idp <- getIdPConfigByIssuer (suid ^. uidTenant) + idp <- getIdPConfigByIssuerOptionalSPId (suid ^. uidTenant) Nothing guardReplacedIdP idp guardScimTokens idp - createSamlUserWithId buid suid + createSamlUserWithId idp buid suid validateEmailIfExists buid suid where -- Replaced IdPs are not allowed to create new wire accounts. @@ -298,7 +298,7 @@ bindUser buid userref = do oldStatus <- do let err :: Spar a err = throwSpar . SparBindFromWrongOrNoTeam . cs . show $ buid - teamid :: TeamId <- (^. idpExtraInfo . wiTeam) <$> getIdPConfigByIssuer (userref ^. uidTenant) + teamid :: TeamId <- (^. idpExtraInfo . wiTeam) <$> getIdPConfigByIssuerOptionalSPId (userref ^. uidTenant) Nothing acc <- Intra.getBrigUserAccount Intra.WithPendingInvitations buid >>= maybe err pure teamid' :: TeamId <- userTeam (accountUser acc) & maybe err pure unless (teamid' == teamid) err @@ -396,7 +396,7 @@ catchVerdictErrors = (`catchError` hndlr) -- traverse the old IdPs in search for the old entry. Return that old entry. findUserWithOldIssuer :: SAML.UserRef -> Spar (Maybe (SAML.UserRef, UserId)) findUserWithOldIssuer (SAML.UserRef issuer subject) = do - idp <- getIdPConfigByIssuer issuer + idp <- getIdPConfigByIssuerOptionalSPId issuer Nothing let tryFind :: Maybe (SAML.UserRef, UserId) -> Issuer -> Spar (Maybe (SAML.UserRef, UserId)) tryFind found@(Just _) _ = pure found tryFind Nothing oldIssuer = (uref,) <$$> getUserByUref uref diff --git a/services/spar/src/Spar/Options.hs b/services/spar/src/Spar/Options.hs index 27bfdff16b..55d040520d 100644 --- a/services/spar/src/Spar/Options.hs +++ b/services/spar/src/Spar/Options.hs @@ -53,7 +53,10 @@ getOpts = do deriveOpts :: OptsRaw -> IO Opts deriveOpts raw = do derived <- do - let respuri = runWithConfig raw sparResponseURI + let respuri = + -- respuri is only needed for 'derivedOptsBindCookiePath'; we want the prefix of the + -- V2 path that includes the team id. + runWithConfig raw (sparResponseURI Nothing) derivedOptsBindCookiePath = URI.uriPath respuri -- We could also make this selectable in the config file, but it seems easier to derive it from -- the SAML base uri. diff --git a/services/spar/test-integration/Util/Core.hs b/services/spar/test-integration/Util/Core.hs index 6bad490497..63a300ed38 100644 --- a/services/spar/test-integration/Util/Core.hs +++ b/services/spar/test-integration/Util/Core.hs @@ -1083,7 +1083,7 @@ callIdpCreate' apiversion sparreq_ muid metadata = do . maybe id zUser muid . path "/identity-providers/" . ( case apiversion of - WireIdPAPIV1 -> Bilge.query [("api", Just "v1")] + WireIdPAPIV1 -> Bilge.query [("api-version", Just "v1")] WireIdPAPIV2 -> id ) . body (RequestBodyLBS . cs $ SAML.encode metadata) From d09283eb27eaef91d39eed15467e2d0e24e3e7bb Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 9 Sep 2021 15:26:47 +0200 Subject: [PATCH 09/52] ... --- .../src/Wire/API/User/IdentityProvider.hs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs b/libs/wire-api/src/Wire/API/User/IdentityProvider.hs index af9c2a25dd..49d1f0e5bc 100644 --- a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs +++ b/libs/wire-api/src/Wire/API/User/IdentityProvider.hs @@ -22,6 +22,8 @@ import Control.Lens (makeLenses, (.~), (?~)) import Control.Monad.Except import Data.Aeson import Data.Aeson.TH +import qualified Data.Binary.Builder as BSB +import qualified Data.ByteString.Conversion as BSC import Data.HashMap.Strict.InsOrd (InsOrdHashMap) import qualified Data.HashMap.Strict.InsOrd as InsOrdHashMap import Data.Id (TeamId) @@ -68,6 +70,29 @@ makeLenses ''WireIdP deriveJSON deriveJSONOptions ''WireIdPAPIVersion deriveJSON deriveJSONOptions ''WireIdP +instance BSC.ToByteString WireIdPAPIVersion where + builder = + BSB.fromByteString . \case + WireIdPAPIV1 -> "v1" + WireIdPAPIV2 -> "v2" + +instance BSC.FromByteString WireIdPAPIVersion where + parser = _ + +instance FromHttpApiData WireIdPAPIVersion where + parseQueryParam = maybe (Left _) Right . BSC.fromByteString' . cs + +instance ToHttpApiData WireIdPAPIVersion where + toQueryParam = cs . BSC.toByteString' + +instance ToParamSchema WireIdPAPIVersion where + toParamSchema Proxy = + mempty + { _paramSchemaDefault = Just "v2", + _paramSchemaType = Just SwaggerString, + _paramSchemaEnum = Just (String . toQueryParam <$> [(minBound :: WireIdPAPIVersion) ..]) + } + instance Cql.Cql WireIdPAPIVersion where ctype = Cql.Tagged Cql.IntColumn From e4c9097c7da552f26ded77346d65447975983435 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 9 Sep 2021 15:28:09 +0200 Subject: [PATCH 10/52] RESET_PLS --- docs/reference/spar-braindump.md | 52 +++++++++++++++++++ services/spar/spar.integration.yaml | 2 +- services/spar/src/Spar/Data.hs | 4 ++ .../test-integration/Test/Spar/APISpec.hs | 4 +- .../test-integration/Test/Spar/DataSpec.hs | 3 +- stack.yaml | 3 +- stack.yaml.lock | 11 ---- 7 files changed, 62 insertions(+), 17 deletions(-) diff --git a/docs/reference/spar-braindump.md b/docs/reference/spar-braindump.md index d6fe65a414..3ee8a45333 100644 --- a/docs/reference/spar-braindump.md +++ b/docs/reference/spar-braindump.md @@ -326,3 +326,55 @@ 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 for fresh +apps. The way... + + +changes: + +- a few end-points have been make more flexible by adding an optional + teamid to the path: ... +- /sso/initiate-login returns an AuthnReq with the response url that + contains the teamid. + + +we make sure that it doesn't matter whether an IdP calls the +end-point(s) *with* teamid or *without*. but we may confuse some bad +idps by sending them a url with the teamid init, where they expect one +without it. + +we could change /sso/initiate-login to contain the teamid based on +which table the idp comes from, and make sure that we insert idps into +the old table if the metadata sais that's what the idp expects. it's +annoying and awkward, though. + +ya, we need to make sure idps set up without teamid will still present with the old urls everywhere, especially in the initiate-login resp body. + +this leaves the complication that people may create new idps based on the metadata they pulled from /metadata (without teamid). can we accomodate this, too? + + + + + +- authnreq needs to have an sp issuer that matches the expectation of the idp. +- store in old table to state that legacy api should be used, this is set via a query param in `POST /identity-provider`. this "ensures" that people do it only when they got the new metadata file. (no typesafe way to accomplish that, it happens outside of our power.) +- add flag to IdP type to state which api is used. + + + + + +next steps: +- write integration tests. make them fail for good reasons. +- read the above ideas. +- finish the implementation. + + + +TODO: +- in 'getIdPIdByIssuerOld', provide a better error to the UI if two idps are found for different teams. +- call `skipIdPAPIVersions [WireIdPAPIV1]` everywhere /a diff --git a/services/spar/spar.integration.yaml b/services/spar/spar.integration.yaml index 6b40d80950..ff0d855085 100644 --- a/services/spar/spar.integration.yaml +++ b/services/spar/spar.integration.yaml @@ -1,6 +1,6 @@ saml: version: SAML2.0 - logLevel: Info + logLevel: Debug spHost: 0.0.0.0 spPort: 8088 diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index 6f2144aa44..eb55b62741 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -354,6 +354,10 @@ getSAMLUser uref = do sel :: PrepQuery R (SAML.Issuer, SAML.NameID) (Identity UserId) sel = "SELECT uid FROM user WHERE issuer = ? AND sso_id = ?" +-- TODO: can the same Issuer send the same NameID to two different teams? we should assume it +-- can, and handle it properly. (in that case, we will either have the team id from the HTTP +-- request, or we won't have made it here because the issuer cannot be uniquely determined.) + deleteSAMLUsersByIssuer :: (HasCallStack, MonadClient m) => SAML.Issuer -> m () deleteSAMLUsersByIssuer issuer = retry x5 . write del $ params Quorum (Identity issuer) where diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index c638c67567..1267531704 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -129,7 +129,7 @@ specMisc = do specMetadata :: SpecWith TestEnv specMetadata = do - describe "metadata" $ do + focus . describe "metadata" $ do it "metadata (legacy)" $ do env <- ask get ((env ^. teSpar) . path "/sso/metadata" . expect2xx) @@ -235,7 +235,7 @@ specFinalizeLogin = do bdy `shouldContain` "wire:sso:success" bdy `shouldContain` "window.opener.postMessage({type: 'AUTH_SUCCESS'}, receiverOrigin)" hasPersistentCookieHeader sparresp `shouldBe` Right () - context "happy flow" $ do + focus . context "happy flow" $ do it "responds with a very peculiar 'allowed' HTTP response" $ do (_, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta spmeta <- getTestSPMetadata tid diff --git a/services/spar/test-integration/Test/Spar/DataSpec.hs b/services/spar/test-integration/Test/Spar/DataSpec.hs index 8a5d4a625f..832f8f5940 100644 --- a/services/spar/test-integration/Test/Spar/DataSpec.hs +++ b/services/spar/test-integration/Test/Spar/DataSpec.hs @@ -177,7 +177,8 @@ spec = do () <- runSparCass $ Data.storeIdPConfig idp midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Just (idp ^. idpId) - it "getIdPConfigsByTeam works" $ do + focus . it "getIdPConfigsByTeam works" $ do + skipIdPAPIVersions [WireIdPAPIV1] teamid <- nextWireId idp <- makeTestIdP <&> idpExtraInfo .~ (WireIdP teamid Nothing [] Nothing) () <- runSparCass $ Data.storeIdPConfig idp diff --git a/stack.yaml b/stack.yaml index 7eb20da383..873b684a05 100644 --- a/stack.yaml +++ b/stack.yaml @@ -69,8 +69,7 @@ extra-deps: # wai-middleware-prometheus can be pulled from hackage once the # a version > 1.0.0 of wai-middleware-prometheus is available # (required: https://github.com/fimad/prometheus-haskell/pull/45) -- git: https://github.com/wireapp/saml2-web-sso - commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c # https://github.com/wireapp/saml2-web-sso/pull/74 (Apr 30, 2021) +- ../saml2-web-sso - git: https://github.com/kim/hs-collectd commit: 885da222be2375f78c7be36127620ed772b677c9 diff --git a/stack.yaml.lock b/stack.yaml.lock index 3cd6d32d24..85c0ca3ebe 100644 --- a/stack.yaml.lock +++ b/stack.yaml.lock @@ -17,17 +17,6 @@ packages: subdir: wai-middleware-prometheus git: https://github.com/fimad/prometheus-haskell commit: 2e3282e5fb27ba8d989c271a0a989823fad7ec43 -- completed: - name: saml2-web-sso - version: '0.18' - git: https://github.com/wireapp/saml2-web-sso - pantry-tree: - size: 4887 - sha256: 12be9a699749b9ebe63fb2e04113c2f2160a63494e8a2ba005792a02d0571f47 - commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c - original: - git: https://github.com/wireapp/saml2-web-sso - commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c - completed: name: collectd version: 0.0.0.2 From da36da6a77e7342042f4c98725c215769ae20fda Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 9 Sep 2021 15:28:42 +0200 Subject: [PATCH 11/52] Revert "RESET_PLS" This reverts commit 8c4316a034f3d248e8eeb4f1060a3ade21c9d467. --- docs/reference/spar-braindump.md | 52 ------------------- services/spar/spar.integration.yaml | 2 +- services/spar/src/Spar/Data.hs | 4 -- .../test-integration/Test/Spar/APISpec.hs | 4 +- .../test-integration/Test/Spar/DataSpec.hs | 3 +- stack.yaml | 3 +- stack.yaml.lock | 11 ++++ 7 files changed, 17 insertions(+), 62 deletions(-) diff --git a/docs/reference/spar-braindump.md b/docs/reference/spar-braindump.md index 3ee8a45333..d6fe65a414 100644 --- a/docs/reference/spar-braindump.md +++ b/docs/reference/spar-braindump.md @@ -326,55 +326,3 @@ 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 for fresh -apps. The way... - - -changes: - -- a few end-points have been make more flexible by adding an optional - teamid to the path: ... -- /sso/initiate-login returns an AuthnReq with the response url that - contains the teamid. - - -we make sure that it doesn't matter whether an IdP calls the -end-point(s) *with* teamid or *without*. but we may confuse some bad -idps by sending them a url with the teamid init, where they expect one -without it. - -we could change /sso/initiate-login to contain the teamid based on -which table the idp comes from, and make sure that we insert idps into -the old table if the metadata sais that's what the idp expects. it's -annoying and awkward, though. - -ya, we need to make sure idps set up without teamid will still present with the old urls everywhere, especially in the initiate-login resp body. - -this leaves the complication that people may create new idps based on the metadata they pulled from /metadata (without teamid). can we accomodate this, too? - - - - - -- authnreq needs to have an sp issuer that matches the expectation of the idp. -- store in old table to state that legacy api should be used, this is set via a query param in `POST /identity-provider`. this "ensures" that people do it only when they got the new metadata file. (no typesafe way to accomplish that, it happens outside of our power.) -- add flag to IdP type to state which api is used. - - - - - -next steps: -- write integration tests. make them fail for good reasons. -- read the above ideas. -- finish the implementation. - - - -TODO: -- in 'getIdPIdByIssuerOld', provide a better error to the UI if two idps are found for different teams. -- call `skipIdPAPIVersions [WireIdPAPIV1]` everywhere /a diff --git a/services/spar/spar.integration.yaml b/services/spar/spar.integration.yaml index ff0d855085..6b40d80950 100644 --- a/services/spar/spar.integration.yaml +++ b/services/spar/spar.integration.yaml @@ -1,6 +1,6 @@ saml: version: SAML2.0 - logLevel: Debug + logLevel: Info spHost: 0.0.0.0 spPort: 8088 diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index eb55b62741..6f2144aa44 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -354,10 +354,6 @@ getSAMLUser uref = do sel :: PrepQuery R (SAML.Issuer, SAML.NameID) (Identity UserId) sel = "SELECT uid FROM user WHERE issuer = ? AND sso_id = ?" --- TODO: can the same Issuer send the same NameID to two different teams? we should assume it --- can, and handle it properly. (in that case, we will either have the team id from the HTTP --- request, or we won't have made it here because the issuer cannot be uniquely determined.) - deleteSAMLUsersByIssuer :: (HasCallStack, MonadClient m) => SAML.Issuer -> m () deleteSAMLUsersByIssuer issuer = retry x5 . write del $ params Quorum (Identity issuer) where diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 1267531704..c638c67567 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -129,7 +129,7 @@ specMisc = do specMetadata :: SpecWith TestEnv specMetadata = do - focus . describe "metadata" $ do + describe "metadata" $ do it "metadata (legacy)" $ do env <- ask get ((env ^. teSpar) . path "/sso/metadata" . expect2xx) @@ -235,7 +235,7 @@ specFinalizeLogin = do bdy `shouldContain` "wire:sso:success" bdy `shouldContain` "window.opener.postMessage({type: 'AUTH_SUCCESS'}, receiverOrigin)" hasPersistentCookieHeader sparresp `shouldBe` Right () - focus . context "happy flow" $ do + context "happy flow" $ do it "responds with a very peculiar 'allowed' HTTP response" $ do (_, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta spmeta <- getTestSPMetadata tid diff --git a/services/spar/test-integration/Test/Spar/DataSpec.hs b/services/spar/test-integration/Test/Spar/DataSpec.hs index 832f8f5940..8a5d4a625f 100644 --- a/services/spar/test-integration/Test/Spar/DataSpec.hs +++ b/services/spar/test-integration/Test/Spar/DataSpec.hs @@ -177,8 +177,7 @@ spec = do () <- runSparCass $ Data.storeIdPConfig idp midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Just (idp ^. idpId) - focus . it "getIdPConfigsByTeam works" $ do - skipIdPAPIVersions [WireIdPAPIV1] + it "getIdPConfigsByTeam works" $ do teamid <- nextWireId idp <- makeTestIdP <&> idpExtraInfo .~ (WireIdP teamid Nothing [] Nothing) () <- runSparCass $ Data.storeIdPConfig idp diff --git a/stack.yaml b/stack.yaml index 873b684a05..7eb20da383 100644 --- a/stack.yaml +++ b/stack.yaml @@ -69,7 +69,8 @@ extra-deps: # wai-middleware-prometheus can be pulled from hackage once the # a version > 1.0.0 of wai-middleware-prometheus is available # (required: https://github.com/fimad/prometheus-haskell/pull/45) -- ../saml2-web-sso +- git: https://github.com/wireapp/saml2-web-sso + commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c # https://github.com/wireapp/saml2-web-sso/pull/74 (Apr 30, 2021) - git: https://github.com/kim/hs-collectd commit: 885da222be2375f78c7be36127620ed772b677c9 diff --git a/stack.yaml.lock b/stack.yaml.lock index 85c0ca3ebe..3cd6d32d24 100644 --- a/stack.yaml.lock +++ b/stack.yaml.lock @@ -17,6 +17,17 @@ packages: subdir: wai-middleware-prometheus git: https://github.com/fimad/prometheus-haskell commit: 2e3282e5fb27ba8d989c271a0a989823fad7ec43 +- completed: + name: saml2-web-sso + version: '0.18' + git: https://github.com/wireapp/saml2-web-sso + pantry-tree: + size: 4887 + sha256: 12be9a699749b9ebe63fb2e04113c2f2160a63494e8a2ba005792a02d0571f47 + commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c + original: + git: https://github.com/wireapp/saml2-web-sso + commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c - completed: name: collectd version: 0.0.0.2 From c9d3a71ef3bad68c5de0df652ff42ecc21dfd800 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 9 Sep 2021 22:51:29 +0200 Subject: [PATCH 12/52] ... --- libs/wire-api/src/Wire/API/User/IdentityProvider.hs | 13 ++++++++++--- .../test/unit/Test/Wire/API/Roundtrip/ByteString.hs | 4 +++- services/spar/test/Arbitrary.hs | 3 --- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs b/libs/wire-api/src/Wire/API/User/IdentityProvider.hs index 49d1f0e5bc..a0fdfa525e 100644 --- a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs +++ b/libs/wire-api/src/Wire/API/User/IdentityProvider.hs @@ -22,6 +22,7 @@ import Control.Lens (makeLenses, (.~), (?~)) import Control.Monad.Except import Data.Aeson import Data.Aeson.TH +import qualified Data.Attoparsec.ByteString as AP import qualified Data.Binary.Builder as BSB import qualified Data.ByteString.Conversion as BSC import Data.HashMap.Strict.InsOrd (InsOrdHashMap) @@ -36,6 +37,7 @@ import SAML2.WebSSO (IdPConfig) import qualified SAML2.WebSSO as SAML import SAML2.WebSSO.Types.TH (deriveJSONOptions) import Servant.API as Servant hiding (MkLink, URI (..)) +import Wire.API.Arbitrary (Arbitrary, GenericUniform (GenericUniform)) import Wire.API.User.Orphans (samlSchemaOptions) -- | The identity provider type used in Spar. @@ -60,7 +62,8 @@ data WireIdPAPIVersion WireIdPAPIV1 | -- | support for different SP entityIDs per team WireIdPAPIV2 - deriving (Eq, Show, Enum, Bounded, Generic) + deriving stock (Eq, Show, Enum, Bounded, Generic) + deriving (Arbitrary) via (GenericUniform WireIdPAPIVersion) defWireIdPAPIVersion :: WireIdPAPIVersion defWireIdPAPIVersion = WireIdPAPIV2 @@ -77,10 +80,14 @@ instance BSC.ToByteString WireIdPAPIVersion where WireIdPAPIV2 -> "v2" instance BSC.FromByteString WireIdPAPIVersion where - parser = _ + parser = + (AP.string "v1" >> pure WireIdPAPIV1) + <|> (AP.string "v2" >> pure WireIdPAPIV2) instance FromHttpApiData WireIdPAPIVersion where - parseQueryParam = maybe (Left _) Right . BSC.fromByteString' . cs + parseQueryParam txt = maybe err Right $ BSC.fromByteString' (cs txt) + where + err = Left $ "FromHttpApiData WireIdPAPIVersion: " <> txt instance ToHttpApiData WireIdPAPIVersion where toQueryParam = cs . BSC.toByteString' diff --git a/libs/wire-api/test/unit/Test/Wire/API/Roundtrip/ByteString.hs b/libs/wire-api/test/unit/Test/Wire/API/Roundtrip/ByteString.hs index 642dfe1f87..d79ed2a670 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Roundtrip/ByteString.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Roundtrip/ByteString.hs @@ -40,6 +40,7 @@ import qualified Wire.API.User as User import qualified Wire.API.User.Activation as User.Activation import qualified Wire.API.User.Auth as User.Auth import qualified Wire.API.User.Identity as User.Identity +import qualified Wire.API.User.IdentityProvider as User.IdentityProvider import qualified Wire.API.User.Password as User.Password import qualified Wire.API.User.Profile as User.Profile import qualified Wire.API.User.Search as User.Search @@ -84,7 +85,8 @@ tests = testRoundTrip @Team.Role.Role, testRoundTrip @User.Search.TeamUserSearchSortBy, testRoundTrip @User.Search.TeamUserSearchSortOrder, - testRoundTrip @User.Search.RoleFilter + testRoundTrip @User.Search.RoleFilter, + testRoundTrip @User.IdentityProvider.WireIdPAPIVersion -- FUTUREWORK: -- testCase "Call.Config.TurnUsername (doesn't have FromByteString)" ... -- testCase "User.Activation.ActivationTarget (doesn't have FromByteString)" ... diff --git a/services/spar/test/Arbitrary.hs b/services/spar/test/Arbitrary.hs index db741ce4c3..f48e5722fc 100644 --- a/services/spar/test/Arbitrary.hs +++ b/services/spar/test/Arbitrary.hs @@ -43,9 +43,6 @@ instance Arbitrary IdPList where instance Arbitrary WireIdP where arbitrary = WireIdP <$> arbitrary <*> arbitrary <*> arbitrary <*> arbitrary -instance Arbitrary WireIdPAPIVersion where - arbitrary = elements [minBound ..] - deriving instance Arbitrary ScimToken instance Arbitrary ScimTokenHash where From 31ff727aabd77a4b42df58e4f729b116dfd4e214 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 9 Sep 2021 23:10:19 +0200 Subject: [PATCH 13/52] ... --- services/spar/src/Spar/Data.hs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index 6f2144aa44..76dc1501b4 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -324,6 +324,18 @@ getSAMLSomeUsersByIssuer issuer = sel :: PrepQuery R (Identity SAML.Issuer) (SAML.NameID, UserId) sel = "SELECT sso_id, uid FROM user_v2 WHERE issuer = ? LIMIT 2000" +-- | ... +-- +-- NB: It is not allowed for two distinct wire users from two different teams to ahave the +-- same 'UserRef'. Rationale: with saml auto-provisioning, the users would be required to +-- add distinguishing handles themselves, which would be at best confusing. with scim +-- provisioning, the two accounts have to be distinct in the user data source, so it +-- /should/ be straight-forward to give the two accounts different 'UserRef' values, even +-- if the 'Issuer' is the same. +-- +-- ... this means we can get away with `user_v2`: pull `UserId`, assume it's always unique, +-- get the `TeamId` from brig, pull all `TeamId`s associated with the given `Issuer` from +-- `issuer_idp_v2`, and make sure the one from brig is in there. (if not: what's the error?) getSAMLUser :: (HasCallStack, MonadClient m) => SAML.UserRef -> m (Maybe UserId) getSAMLUser uref = do mbUid <- getSAMLUserNew uref From 57d06aebb12db00cec35d8f40b7e3189c64539db Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 9 Sep 2021 23:14:42 +0200 Subject: [PATCH 14/52] Fix typo. --- services/spar/src/Spar/Data.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index 76dc1501b4..f70be179a8 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -466,7 +466,7 @@ storeIdPConfig idp = retry x5 . batch $ do -- 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 = "INSERT INTO issuer_idp_v2 (idp, team, issuer) VALUES (?, ?, ?)" byTeam :: PrepQuery W (SAML.IdPId, TeamId) () byTeam = "INSERT INTO team_idp (idp, team) VALUES (?, ?)" From f944be2d86f2d55d381db2149e005a42c434a208 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 10 Sep 2021 08:39:04 +0200 Subject: [PATCH 15/52] ... --- libs/wire-api/src/Wire/API/User/IdentityProvider.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs b/libs/wire-api/src/Wire/API/User/IdentityProvider.hs index a0fdfa525e..f3d88cfd95 100644 --- a/libs/wire-api/src/Wire/API/User/IdentityProvider.hs +++ b/libs/wire-api/src/Wire/API/User/IdentityProvider.hs @@ -66,7 +66,7 @@ data WireIdPAPIVersion deriving (Arbitrary) via (GenericUniform WireIdPAPIVersion) defWireIdPAPIVersion :: WireIdPAPIVersion -defWireIdPAPIVersion = WireIdPAPIV2 +defWireIdPAPIVersion = WireIdPAPIV1 makeLenses ''WireIdP From 188626b55882355e8b5a70a0a7d4fe31d92b76c9 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 10 Sep 2021 08:39:17 +0200 Subject: [PATCH 16/52] RESET_PLS --- docs/reference/spar-braindump.md | 41 +++++++++++++++++++ services/spar/spar.integration.yaml | 2 +- .../test-integration/Test/Spar/APISpec.hs | 4 +- .../test-integration/Test/Spar/DataSpec.hs | 3 +- .../Test/Spar/Scim/UserSpec.hs | 2 +- stack.yaml | 3 +- stack.yaml.lock | 11 ----- 7 files changed, 48 insertions(+), 18 deletions(-) diff --git a/docs/reference/spar-braindump.md b/docs/reference/spar-braindump.md index d6fe65a414..d22b3264c3 100644 --- a/docs/reference/spar-braindump.md +++ b/docs/reference/spar-braindump.md @@ -326,3 +326,44 @@ 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 for fresh +apps. The way... + + +changes: + +- a few end-points have been make more flexible by adding an optional + teamid to the path: ... +- /sso/initiate-login returns an AuthnReq with the response url that + contains the teamid. +- schema changes: ... + +we make sure that it doesn't matter whether an IdP calls the +end-point(s) *with* teamid or *without*. but the url of the +finalize-login end-point in the authnreq must match the one the idp +knows, or there will be an "audience mismatch" error. + +we solve this by introducing an IdP API version that is associated +with every IdP. the default is V1 (IdP issuer must be unique accross +the wire instance); V2 (IdP issuer must be unique in the scope of one +team only, and the API carries the team-id in the places where it's +needed; see above) can be set actively in `POST /identity-providers` +by adding query param `api-version=v2`. this "ensures" that people do +it only when they got the new metadata file. (no typesafe way to +accomplish that, it happens outside of our power.) + + + +TODO: +- finish haddocks above 'Spar.Data.getSAMLUser'. + - make sure that we implement the check that user is always a team belonging to the idp. + - make sure that we fail with a good error if the idp attempts to provide the user to two teams (or can that even happen? what *does* happen now in that case?) +- fix all spar integration tests. +- add integration test: register same issuer for two teams, provision *different* nameids to both teams. run full auth flow. +- in 'getIdPIdByIssuerOld', provide a better error to the UI if two idps are found for different teams. +- write this section. +- NTH: call `skipIdPAPIVersions [WireIdPAPIV1]` everywhere /a diff --git a/services/spar/spar.integration.yaml b/services/spar/spar.integration.yaml index 6b40d80950..ff0d855085 100644 --- a/services/spar/spar.integration.yaml +++ b/services/spar/spar.integration.yaml @@ -1,6 +1,6 @@ saml: version: SAML2.0 - logLevel: Info + logLevel: Debug spHost: 0.0.0.0 spPort: 8088 diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index c638c67567..1267531704 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -129,7 +129,7 @@ specMisc = do specMetadata :: SpecWith TestEnv specMetadata = do - describe "metadata" $ do + focus . describe "metadata" $ do it "metadata (legacy)" $ do env <- ask get ((env ^. teSpar) . path "/sso/metadata" . expect2xx) @@ -235,7 +235,7 @@ specFinalizeLogin = do bdy `shouldContain` "wire:sso:success" bdy `shouldContain` "window.opener.postMessage({type: 'AUTH_SUCCESS'}, receiverOrigin)" hasPersistentCookieHeader sparresp `shouldBe` Right () - context "happy flow" $ do + focus . context "happy flow" $ do it "responds with a very peculiar 'allowed' HTTP response" $ do (_, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta spmeta <- getTestSPMetadata tid diff --git a/services/spar/test-integration/Test/Spar/DataSpec.hs b/services/spar/test-integration/Test/Spar/DataSpec.hs index 8a5d4a625f..832f8f5940 100644 --- a/services/spar/test-integration/Test/Spar/DataSpec.hs +++ b/services/spar/test-integration/Test/Spar/DataSpec.hs @@ -177,7 +177,8 @@ spec = do () <- runSparCass $ Data.storeIdPConfig idp midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Just (idp ^. idpId) - it "getIdPConfigsByTeam works" $ do + focus . it "getIdPConfigsByTeam works" $ do + skipIdPAPIVersions [WireIdPAPIV1] teamid <- nextWireId idp <- makeTestIdP <&> idpExtraInfo .~ (WireIdP teamid Nothing [] Nothing) () <- runSparCass $ Data.storeIdPConfig idp diff --git a/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs b/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs index 4ade02f3eb..a2d3d3499d 100644 --- a/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs +++ b/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs @@ -776,7 +776,7 @@ specListUsers :: SpecWith TestEnv specListUsers = describe "GET /Users" $ do it "lists all SCIM users in a team" $ testListProvisionedUsers context "1 SAML IdP" $ do - it "finds a SCIM-provisioned user by userName or externalId" $ testFindProvisionedUser + focus . it "finds a SCIM-provisioned user by userName or externalId" $ testFindProvisionedUser it "finds a user autoprovisioned via saml by externalId via email" $ testFindSamlAutoProvisionedUserMigratedWithEmailInTeamWithSSO it "finds a user invited via team settings by externalId via email" $ testFindTeamSettingsInvitedUserMigratedWithEmailInTeamWithSSO it "finds a user invited via team settings by UserId" $ testFindTeamSettingsInvitedUserMigratedWithEmailInTeamWithSSOViaUserId diff --git a/stack.yaml b/stack.yaml index 7eb20da383..873b684a05 100644 --- a/stack.yaml +++ b/stack.yaml @@ -69,8 +69,7 @@ extra-deps: # wai-middleware-prometheus can be pulled from hackage once the # a version > 1.0.0 of wai-middleware-prometheus is available # (required: https://github.com/fimad/prometheus-haskell/pull/45) -- git: https://github.com/wireapp/saml2-web-sso - commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c # https://github.com/wireapp/saml2-web-sso/pull/74 (Apr 30, 2021) +- ../saml2-web-sso - git: https://github.com/kim/hs-collectd commit: 885da222be2375f78c7be36127620ed772b677c9 diff --git a/stack.yaml.lock b/stack.yaml.lock index 3cd6d32d24..85c0ca3ebe 100644 --- a/stack.yaml.lock +++ b/stack.yaml.lock @@ -17,17 +17,6 @@ packages: subdir: wai-middleware-prometheus git: https://github.com/fimad/prometheus-haskell commit: 2e3282e5fb27ba8d989c271a0a989823fad7ec43 -- completed: - name: saml2-web-sso - version: '0.18' - git: https://github.com/wireapp/saml2-web-sso - pantry-tree: - size: 4887 - sha256: 12be9a699749b9ebe63fb2e04113c2f2160a63494e8a2ba005792a02d0571f47 - commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c - original: - git: https://github.com/wireapp/saml2-web-sso - commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c - completed: name: collectd version: 0.0.0.2 From 83acf32353912dcf8578df90f671ebda7025e6b7 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 10 Sep 2021 17:28:14 +0200 Subject: [PATCH 17/52] Revert "RESET_PLS" This reverts commit e0853d3a4a4a6ddbaf189eeff5c7633f62b016cf. --- docs/reference/spar-braindump.md | 41 ------------------- services/spar/spar.integration.yaml | 2 +- .../test-integration/Test/Spar/APISpec.hs | 4 +- .../test-integration/Test/Spar/DataSpec.hs | 3 +- .../Test/Spar/Scim/UserSpec.hs | 2 +- stack.yaml | 3 +- stack.yaml.lock | 11 +++++ 7 files changed, 18 insertions(+), 48 deletions(-) diff --git a/docs/reference/spar-braindump.md b/docs/reference/spar-braindump.md index d22b3264c3..d6fe65a414 100644 --- a/docs/reference/spar-braindump.md +++ b/docs/reference/spar-braindump.md @@ -326,44 +326,3 @@ 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 for fresh -apps. The way... - - -changes: - -- a few end-points have been make more flexible by adding an optional - teamid to the path: ... -- /sso/initiate-login returns an AuthnReq with the response url that - contains the teamid. -- schema changes: ... - -we make sure that it doesn't matter whether an IdP calls the -end-point(s) *with* teamid or *without*. but the url of the -finalize-login end-point in the authnreq must match the one the idp -knows, or there will be an "audience mismatch" error. - -we solve this by introducing an IdP API version that is associated -with every IdP. the default is V1 (IdP issuer must be unique accross -the wire instance); V2 (IdP issuer must be unique in the scope of one -team only, and the API carries the team-id in the places where it's -needed; see above) can be set actively in `POST /identity-providers` -by adding query param `api-version=v2`. this "ensures" that people do -it only when they got the new metadata file. (no typesafe way to -accomplish that, it happens outside of our power.) - - - -TODO: -- finish haddocks above 'Spar.Data.getSAMLUser'. - - make sure that we implement the check that user is always a team belonging to the idp. - - make sure that we fail with a good error if the idp attempts to provide the user to two teams (or can that even happen? what *does* happen now in that case?) -- fix all spar integration tests. -- add integration test: register same issuer for two teams, provision *different* nameids to both teams. run full auth flow. -- in 'getIdPIdByIssuerOld', provide a better error to the UI if two idps are found for different teams. -- write this section. -- NTH: call `skipIdPAPIVersions [WireIdPAPIV1]` everywhere /a diff --git a/services/spar/spar.integration.yaml b/services/spar/spar.integration.yaml index ff0d855085..6b40d80950 100644 --- a/services/spar/spar.integration.yaml +++ b/services/spar/spar.integration.yaml @@ -1,6 +1,6 @@ saml: version: SAML2.0 - logLevel: Debug + logLevel: Info spHost: 0.0.0.0 spPort: 8088 diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 1267531704..c638c67567 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -129,7 +129,7 @@ specMisc = do specMetadata :: SpecWith TestEnv specMetadata = do - focus . describe "metadata" $ do + describe "metadata" $ do it "metadata (legacy)" $ do env <- ask get ((env ^. teSpar) . path "/sso/metadata" . expect2xx) @@ -235,7 +235,7 @@ specFinalizeLogin = do bdy `shouldContain` "wire:sso:success" bdy `shouldContain` "window.opener.postMessage({type: 'AUTH_SUCCESS'}, receiverOrigin)" hasPersistentCookieHeader sparresp `shouldBe` Right () - focus . context "happy flow" $ do + context "happy flow" $ do it "responds with a very peculiar 'allowed' HTTP response" $ do (_, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta spmeta <- getTestSPMetadata tid diff --git a/services/spar/test-integration/Test/Spar/DataSpec.hs b/services/spar/test-integration/Test/Spar/DataSpec.hs index 832f8f5940..8a5d4a625f 100644 --- a/services/spar/test-integration/Test/Spar/DataSpec.hs +++ b/services/spar/test-integration/Test/Spar/DataSpec.hs @@ -177,8 +177,7 @@ spec = do () <- runSparCass $ Data.storeIdPConfig idp midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Just (idp ^. idpId) - focus . it "getIdPConfigsByTeam works" $ do - skipIdPAPIVersions [WireIdPAPIV1] + it "getIdPConfigsByTeam works" $ do teamid <- nextWireId idp <- makeTestIdP <&> idpExtraInfo .~ (WireIdP teamid Nothing [] Nothing) () <- runSparCass $ Data.storeIdPConfig idp diff --git a/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs b/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs index a2d3d3499d..4ade02f3eb 100644 --- a/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs +++ b/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs @@ -776,7 +776,7 @@ specListUsers :: SpecWith TestEnv specListUsers = describe "GET /Users" $ do it "lists all SCIM users in a team" $ testListProvisionedUsers context "1 SAML IdP" $ do - focus . it "finds a SCIM-provisioned user by userName or externalId" $ testFindProvisionedUser + it "finds a SCIM-provisioned user by userName or externalId" $ testFindProvisionedUser it "finds a user autoprovisioned via saml by externalId via email" $ testFindSamlAutoProvisionedUserMigratedWithEmailInTeamWithSSO it "finds a user invited via team settings by externalId via email" $ testFindTeamSettingsInvitedUserMigratedWithEmailInTeamWithSSO it "finds a user invited via team settings by UserId" $ testFindTeamSettingsInvitedUserMigratedWithEmailInTeamWithSSOViaUserId diff --git a/stack.yaml b/stack.yaml index 873b684a05..7eb20da383 100644 --- a/stack.yaml +++ b/stack.yaml @@ -69,7 +69,8 @@ extra-deps: # wai-middleware-prometheus can be pulled from hackage once the # a version > 1.0.0 of wai-middleware-prometheus is available # (required: https://github.com/fimad/prometheus-haskell/pull/45) -- ../saml2-web-sso +- git: https://github.com/wireapp/saml2-web-sso + commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c # https://github.com/wireapp/saml2-web-sso/pull/74 (Apr 30, 2021) - git: https://github.com/kim/hs-collectd commit: 885da222be2375f78c7be36127620ed772b677c9 diff --git a/stack.yaml.lock b/stack.yaml.lock index 85c0ca3ebe..3cd6d32d24 100644 --- a/stack.yaml.lock +++ b/stack.yaml.lock @@ -17,6 +17,17 @@ packages: subdir: wai-middleware-prometheus git: https://github.com/fimad/prometheus-haskell commit: 2e3282e5fb27ba8d989c271a0a989823fad7ec43 +- completed: + name: saml2-web-sso + version: '0.18' + git: https://github.com/wireapp/saml2-web-sso + pantry-tree: + size: 4887 + sha256: 12be9a699749b9ebe63fb2e04113c2f2160a63494e8a2ba005792a02d0571f47 + commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c + original: + git: https://github.com/wireapp/saml2-web-sso + commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c - completed: name: collectd version: 0.0.0.2 From edfb77dba7b8212e3a7417921001082d737a1906 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 10 Sep 2021 17:34:31 +0200 Subject: [PATCH 18/52] Try out skipAPIVersions. --- services/spar/test-integration/Test/Spar/DataSpec.hs | 1 + 1 file changed, 1 insertion(+) diff --git a/services/spar/test-integration/Test/Spar/DataSpec.hs b/services/spar/test-integration/Test/Spar/DataSpec.hs index 8a5d4a625f..2a911bc3ad 100644 --- a/services/spar/test-integration/Test/Spar/DataSpec.hs +++ b/services/spar/test-integration/Test/Spar/DataSpec.hs @@ -178,6 +178,7 @@ spec = do midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) liftIO $ midp `shouldBe` Just (idp ^. idpId) it "getIdPConfigsByTeam works" $ do + skipIdPAPIVersions [WireIdPAPIV1] teamid <- nextWireId idp <- makeTestIdP <&> idpExtraInfo .~ (WireIdP teamid Nothing [] Nothing) () <- runSparCass $ Data.storeIdPConfig idp From a8675c9d5a3396c758a27f81dd5de73ffcf18492 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 10 Sep 2021 21:00:38 +0200 Subject: [PATCH 19/52] Update stack.yaml. --- stack.yaml | 2 +- stack.yaml.lock | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/stack.yaml b/stack.yaml index 7eb20da383..33dd374754 100644 --- a/stack.yaml +++ b/stack.yaml @@ -70,7 +70,7 @@ extra-deps: # a version > 1.0.0 of wai-middleware-prometheus is available # (required: https://github.com/fimad/prometheus-haskell/pull/45) - git: https://github.com/wireapp/saml2-web-sso - commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c # https://github.com/wireapp/saml2-web-sso/pull/74 (Apr 30, 2021) + commit: 60398f375987b74d6b855b5d225e45dc3a96ac06 # https://github.com/wireapp/saml2-web-sso/pull/75 (Sep 10, 2021) - git: https://github.com/kim/hs-collectd commit: 885da222be2375f78c7be36127620ed772b677c9 diff --git a/stack.yaml.lock b/stack.yaml.lock index 3cd6d32d24..6fc195e0df 100644 --- a/stack.yaml.lock +++ b/stack.yaml.lock @@ -23,11 +23,11 @@ packages: git: https://github.com/wireapp/saml2-web-sso pantry-tree: size: 4887 - sha256: 12be9a699749b9ebe63fb2e04113c2f2160a63494e8a2ba005792a02d0571f47 - commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c + sha256: 29e0138ca6bc33500b87cd6c06bbd899fe4ddaadcfd5117b211e7769f9f80161 + commit: 60398f375987b74d6b855b5d225e45dc3a96ac06 original: git: https://github.com/wireapp/saml2-web-sso - commit: f56b5ffc10ec5ceab9a508cbb9f8fbaa017bbf2c + commit: 60398f375987b74d6b855b5d225e45dc3a96ac06 - completed: name: collectd version: 0.0.0.2 From 3eca40eb51aff911ee642d09951813451a3ab8d7 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 10 Sep 2021 21:01:04 +0200 Subject: [PATCH 20/52] Fix stale comment. --- services/spar/src/Spar/App.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/spar/src/Spar/App.hs b/services/spar/src/Spar/App.hs index 784372dd5b..f0a16cc3e2 100644 --- a/services/spar/src/Spar/App.hs +++ b/services/spar/src/Spar/App.hs @@ -430,7 +430,7 @@ verdictHandlerResultCore bindCky = \case case (viaBindCookie, viaSparCassandra, viaSparCassandraOldIssuer) of -- 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, so we pass 'ManagedByWire'. + -- "reauthentication" branch. (Nothing, Nothing, Nothing) -> autoprovisionSamlUser 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 From 1124443d48bbb82c9d4d4b5a5e51649f829f4e47 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 10 Sep 2021 21:37:57 +0200 Subject: [PATCH 21/52] better test. --- services/spar/test-integration/Test/Spar/APISpec.hs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index c638c67567..ed3cd7035e 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -54,6 +54,7 @@ import SAML2.WebSSO edCertAuthnResponse, edIssuer, edRequestURI, + fromIssuer, getUserRef, idPIdToST, idpExtraInfo, @@ -61,6 +62,7 @@ import SAML2.WebSSO idpMetadata, mkNameID, parseFromDocument, + rqIssuer, (-/), ) import qualified SAML2.WebSSO as SAML @@ -70,6 +72,7 @@ import SAML2.WebSSO.Test.Util import qualified Spar.Data as Data import qualified Spar.Intra.Brig as Intra import Text.XML.DSig (SignPrivCreds, mkSignCredsWithCert) +import qualified URI.ByteString as URI import URI.ByteString.QQ (uri) import Util.Core import qualified Util.Scim as ScimT @@ -240,6 +243,11 @@ specFinalizeLogin = do (_, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta spmeta <- getTestSPMetadata tid authnreq <- negotiateAuthnRequest idp + audiencePath <- do + asks (^. teWireIdPAPIVersion) <&> \case + WireIdPAPIV1 -> "/sso/finalize-login/" + WireIdPAPIV2 -> "/sso/finalize-login/" <> toByteString' tid + liftIO $ authnreq ^. rqIssuer . fromIssuer . to URI.uriPath `shouldBe` audiencePath authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp spmeta authnreq True loginSuccess =<< submitAuthnResponse tid authnresp context "user is created once, then deleted in team settings, then can login again." $ do From 24c783f15b9f5fc0b562cdbd2925da40221e1830 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 10 Sep 2021 21:38:15 +0200 Subject: [PATCH 22/52] fix authnreq issuer computation. --- services/spar/src/Spar/API.hs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 971e68b257..5b82c69269 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -133,8 +133,12 @@ authreq _ DoInitiateBind Nothing _ _ _ = throwSpar SparInitBindWithoutAuth authreq authreqttl _ zusr msucc merr idpid = do vformat <- validateAuthreqParams msucc merr form@(SAML.FormRedirect _ ((^. SAML.rqID) -> reqid)) <- do - mbidp :: Maybe IdP <- wrapMonadClient (Data.getIdPConfig idpid) - SAML.authreq authreqttl (sparSPIssuer (mbidp <&> view (SAML.idpExtraInfo . wiTeam))) idpid + idp :: IdP <- wrapMonadClient (Data.getIdPConfig idpid) >>= maybe (throwSpar SparIdPNotFound) pure + let mbtid :: Maybe TeamId + mbtid = case fromMaybe defWireIdPAPIVersion (idp ^. SAML.idpExtraInfo . wiApiVersion) of + WireIdPAPIV1 -> Nothing + WireIdPAPIV2 -> Just $ idp ^. SAML.idpExtraInfo . wiTeam + SAML.authreq authreqttl (sparSPIssuer mbtid) idpid wrapMonadClient $ Data.storeVerdictFormat authreqttl reqid vformat cky <- initializeBindCookie zusr authreqttl SAML.logger SAML.Debug $ "setting bind cookie: " <> show cky From 23e5e9c7ea109aaab412c1d71c14c3be7d30c348 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 10 Sep 2021 22:08:11 +0200 Subject: [PATCH 23/52] fix authnreq issuer computation *test*. --- services/spar/test-integration/Test/Spar/APISpec.hs | 4 +++- services/spar/test-integration/Util/Core.hs | 12 ++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index ed3cd7035e..e1d5a445ba 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -241,11 +241,13 @@ specFinalizeLogin = do context "happy flow" $ do it "responds with a very peculiar 'allowed' HTTP response" $ do (_, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta + asks (^. teWireIdPAPIVersion) >>= \apiVersion -> do + liftIO $ fromMaybe defWireIdPAPIVersion (idp ^. idpExtraInfo . wiApiVersion) `shouldBe` apiVersion spmeta <- getTestSPMetadata tid authnreq <- negotiateAuthnRequest idp audiencePath <- do asks (^. teWireIdPAPIVersion) <&> \case - WireIdPAPIV1 -> "/sso/finalize-login/" + WireIdPAPIV1 -> "/sso/finalize-login" WireIdPAPIV2 -> "/sso/finalize-login/" <> toByteString' tid liftIO $ authnreq ^. rqIssuer . fromIssuer . to URI.uriPath `shouldBe` audiencePath authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp spmeta authnreq True diff --git a/services/spar/test-integration/Util/Core.hs b/services/spar/test-integration/Util/Core.hs index 63a300ed38..0a41a522c7 100644 --- a/services/spar/test-integration/Util/Core.hs +++ b/services/spar/test-integration/Util/Core.hs @@ -791,10 +791,10 @@ registerTestIdPFrom :: SparReq -> m (UserId, TeamId, IdP) registerTestIdPFrom metadata mgr brig galley spar = do - legacyMode <- asks (^. teWireIdPAPIVersion) + apiVersion <- asks (^. teWireIdPAPIVersion) liftIO . runHttpT mgr $ do (uid, tid) <- createUserWithTeam brig galley - (uid,tid,) <$> callIdpCreate legacyMode spar (Just uid) metadata + (uid,tid,) <$> callIdpCreate apiVersion spar (Just uid) metadata getCookie :: KnownSymbol name => proxy name -> ResponseLBS -> Either String (SAML.SimpleSetCookie name) getCookie proxy rsp = do @@ -1078,13 +1078,17 @@ 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 + -- 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")] - WireIdPAPIV2 -> id + 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" From cf3600058084cc968a5280be62d8c243c2cee527 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 10 Sep 2021 22:24:42 +0200 Subject: [PATCH 24/52] Test multi-team idp issuer (failing). --- .../test-integration/Test/Spar/APISpec.hs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index e1d5a445ba..74ca975fdf 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -252,6 +252,28 @@ 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 + [ 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) + do + spmeta <- getTestSPMetadata tid1 + authnreq <- negotiateAuthnRequest idp1 + authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp1 spmeta authnreq True + loginSuccess =<< submitAuthnResponse tid1 authnresp + do + spmeta <- getTestSPMetadata tid2 + authnreq <- negotiateAuthnRequest idp2 + authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp2 spmeta authnreq True + loginSuccess =<< 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 From fe57c6cb335541f20edbcd96a2d94f73ce8ec2ff Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 10 Sep 2021 22:28:21 +0200 Subject: [PATCH 25/52] Fix: it's ok to have two teams with the same IdP issuer/entityID... ... as long as they really live in different teams! --- services/spar/src/Spar/API.hs | 11 +++++++++-- services/spar/src/Spar/Error.hs | 4 ++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 5b82c69269..099a86fa1d 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -345,9 +345,16 @@ validateNewIdP apiversion _idpMetadata teamId mReplaces = do let requri = _idpMetadata ^. SAML.edRequestURI _idpExtraInfo = WireIdP teamId (Just apiversion) oldIssuers Nothing enforceHttps requri - wrapMonadClient (Data.getIdPIdByIssuer (_idpMetadata ^. SAML.edIssuer) teamId) >>= \case + wrapMonadClient (Data.getIdPConfigByIssuer (_idpMetadata ^. SAML.edIssuer) teamId) >>= \case Nothing -> pure () - Just _ -> throwSpar SparNewIdPAlreadyInUse + Just idp' -> case apiversion of + WireIdPAPIV1 -> do + throwSpar $ SparNewIdPAlreadyInUse "you can't create an IdP with api-version v1 if the issuer is already in use on the wire instance." + WireIdPAPIV2 -> do + when (fromMaybe defWireIdPAPIVersion (idp' ^. SAML.idpExtraInfo . wiApiVersion) == WireIdPAPIV1) $ do + throwSpar $ SparNewIdPAlreadyInUse "only allow all-new IdPs, no combination of old and new IdPs." + 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." pure SAML.IdPConfig {..} -- | FUTUREWORK: 'idpUpdateXML' is only factored out of this function for symmetry with diff --git a/services/spar/src/Spar/Error.hs b/services/spar/src/Spar/Error.hs index 960b7de809..f82d7257b3 100644 --- a/services/spar/src/Spar/Error.hs +++ b/services/spar/src/Spar/Error.hs @@ -93,7 +93,7 @@ data SparCustomError | SparCassandraTTLError TTLError | SparNewIdPBadMetadata LT | SparNewIdPPubkeyMismatch - | SparNewIdPAlreadyInUse + | SparNewIdPAlreadyInUse LT | SparNewIdPWantHttps LT | SparIdPHasBoundUsers | SparIdPIssuerInUse @@ -172,7 +172,7 @@ renderSparError (SAML.InvalidCert msg) = Right $ Wai.mkError status500 "invalid- -- Errors related to IdP creation renderSparError (SAML.CustomError (SparNewIdPBadMetadata msg)) = Right $ Wai.mkError status400 "invalid-metadata" msg renderSparError (SAML.CustomError SparNewIdPPubkeyMismatch) = Right $ Wai.mkError status400 "key-mismatch" "public keys in body, metadata do not match" -renderSparError (SAML.CustomError SparNewIdPAlreadyInUse) = Right $ Wai.mkError status400 "idp-already-in-use" "an idp issuer can only be used within one team" +renderSparError (SAML.CustomError (SparNewIdPAlreadyInUse msg)) = Right $ Wai.mkError status400 "idp-already-in-use" msg renderSparError (SAML.CustomError (SparNewIdPWantHttps msg)) = Right $ Wai.mkError status400 "idp-must-be-https" ("an idp request uri must be https, not http or other: " <> msg) renderSparError (SAML.CustomError SparIdPHasBoundUsers) = Right $ Wai.mkError status412 "idp-has-bound-users" "an idp can only be deleted if it is empty" renderSparError (SAML.CustomError SparIdPIssuerInUse) = Right $ Wai.mkError status400 "idp-issuer-in-use" "The issuer of your IdP is already in use. Remove the entry in the team that uses it, or construct a new IdP issuer." From f0111fc409257614eaf6e6276440fed483fe87cc Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 10 Sep 2021 23:47:00 +0200 Subject: [PATCH 26/52] The fix is becoming more invovled... More error info when looking up idps. --- services/spar/src/Spar/Data.hs | 77 ++++++++++++++++++++++++---------- 1 file changed, 54 insertions(+), 23 deletions(-) diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index f70be179a8..0c0671d7cb 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -56,6 +56,7 @@ module Spar.Data Replacing (..), setReplacedBy, clearReplacedBy, + GetIdPResult (..), getIdPConfig, getIdPConfigByIssuer, getIdPConfigByIssuerAllowOld, @@ -527,27 +528,53 @@ getIdPConfig idpid = sel :: PrepQuery R (Identity SAML.IdPId) IdPConfigRow sel = "SELECT idp, issuer, request_uri, public_key, extra_public_keys, team, api_version, old_issuers, replaced_by FROM idp WHERE idp = ?" +data GetIdPResult a + = GetIdPFound a + | GetIdPNotFound + | -- | you were looking for an idp by just providing issuer, not teamid, and `issuer_idp_v2` + -- has more than one entry (for different teams). + GetIdPNonUniqueAnyTeam [a] + | -- | If you provide a teamid, and the idp retrieved is found just by issuer, and lives in + -- another team. this should be handled similarly to NotFound in most cases. + GetIdPWrongTeam a + deriving (Eq, Show) + +-- | we don't need the functor property, this is just for fun. +instance Functor GetIdPResult where + fmap f (GetIdPFound a) = GetIdPFound (f a) + fmap _ GetIdPNotFound = GetIdPNotFound + fmap f (GetIdPNonUniqueAnyTeam as) = GetIdPNonUniqueAnyTeam (f <$> as) + fmap f (GetIdPWrongTeam a) = GetIdPWrongTeam (f a) + +_mapGetIdPResult_simple :: (HasCallStack, MonadClient m) => (a -> m b) -> GetIdPResult a -> m (GetIdPResult b) +_mapGetIdPResult_simple f (GetIdPFound a) = GetIdPFound <$> f a +_mapGetIdPResult_simple _ GetIdPNotFound = pure GetIdPNotFound +_mapGetIdPResult_simple f (GetIdPNonUniqueAnyTeam as) = GetIdPNonUniqueAnyTeam <$> (f `mapM` as) +_mapGetIdPResult_simple f (GetIdPWrongTeam a) = GetIdPWrongTeam <$> f a + +mapGetIdPResult :: (HasCallStack, MonadClient m) => (a -> m (Maybe b)) -> GetIdPResult a -> m (GetIdPResult b) +mapGetIdPResult = do + -- TODO: the simple one doesn't work, but this one is stupid. we want to throw something if + -- we have an id, but can't find the idp for it. + undefined + -- | See 'getIdPIdByIssuer'. getIdPConfigByIssuer :: (HasCallStack, MonadClient m) => SAML.Issuer -> TeamId -> - m (Maybe IdP) -getIdPConfigByIssuer issuer team = do - getIdPIdByIssuer issuer team >>= \case - Nothing -> pure Nothing - Just idpid -> getIdPConfig idpid + m (GetIdPResult IdP) +getIdPConfigByIssuer issuer = + getIdPIdByIssuer issuer >=> mapGetIdPResult getIdPConfig -- | See 'getIdPIdByIssuerAllowOld'. getIdPConfigByIssuerAllowOld :: (HasCallStack, MonadClient m) => SAML.Issuer -> Maybe TeamId -> - m (Maybe IdP) -getIdPConfigByIssuerAllowOld issuer mbteam = do - getIdPIdByIssuerAllowOld issuer mbteam >>= \case - Nothing -> pure Nothing - Just idpid -> getIdPConfig idpid + m (GetIdPResult IdP) +getIdPConfigByIssuerAllowOld issuer = do + getIdPIdByIssuerAllowOld issuer >=> mapGetIdPResult getIdPConfig -- | Lookup idp in table `issuer_idp_v2` (using both issuer entityID and teamid); if nothing -- is found there or if teamid is 'Nothing', lookup under issuer in `issuer_idp`. @@ -555,11 +582,8 @@ getIdPIdByIssuer :: (HasCallStack, MonadClient m) => SAML.Issuer -> TeamId -> - m (Maybe SAML.IdPId) -getIdPIdByIssuer issuer team = do - mbv2 <- getIdPIdByIssuerV2 issuer team - mbboth <- maybe (getIdPIdByIssuerOld issuer) (pure . Just) mbv2 - pure mbboth + m (GetIdPResult SAML.IdPId) +getIdPIdByIssuer issuer = getIdPIdByIssuerAllowOld issuer . Just -- | Like 'getIdPIdByIssuer', but do not require a 'TeamId'. If none is provided, see if a -- single solution can be found without. @@ -567,26 +591,33 @@ getIdPIdByIssuerAllowOld :: (HasCallStack, MonadClient m) => SAML.Issuer -> Maybe TeamId -> - m (Maybe SAML.IdPId) + m (GetIdPResult SAML.IdPId) getIdPIdByIssuerAllowOld issuer mbteam = do mbv2 <- maybe (pure Nothing) (getIdPIdByIssuerV2 issuer) mbteam - mbboth <- maybe (getIdPIdByIssuerOld issuer) (pure . Just) mbv2 - pure mbboth + mbv1v2 <- maybe (getIdPIdByIssuerOld issuer) (pure . GetIdPFound) mbv2 + case (mbv1v2, mbteam) of + (GetIdPFound idpid, Just team) -> do + idp <- getIdPConfig idpid >>= error "TODO: handle nothing better" -- maybe (error "TODO: database inconsistency") pure + pure $ + if idp ^. SAML.idpExtraInfo . wiTeam /= team + then GetIdPWrongTeam idpid + else mbv1v2 + _ -> pure mbv1v2 -- | Find 'IdPId' without team. Search both `issuer_idp` and `issuer_idp_v2`; in the latter, -- make sure the result is unique (no two IdPs for two different teams). getIdPIdByIssuerOld :: (HasCallStack, MonadClient m) => SAML.Issuer -> - m (Maybe SAML.IdPId) + m (GetIdPResult SAML.IdPId) getIdPIdByIssuerOld issuer = do (runIdentity <$$> retry x1 (query1 sel $ params Quorum (Identity issuer))) >>= \case - Just idpid -> pure $ Just idpid + Just idpid -> pure $ GetIdPFound idpid Nothing -> (runIdentity <$$> retry x1 (query selv2 $ params Quorum (Identity issuer))) >>= \case - [] -> pure Nothing - [idpid] -> pure $ Just idpid - (_ : _ : _) -> pure Nothing -- TODO: this should produce a better error! + [] -> pure GetIdPNotFound + [idpid] -> pure $ GetIdPFound idpid + idpids@(_ : _ : _) -> pure $ GetIdPNonUniqueAnyTeam idpids where sel :: PrepQuery R (Identity SAML.Issuer) (Identity SAML.IdPId) sel = "SELECT idp FROM issuer_idp WHERE issuer = ?" From 42865d346b6182cbd464cca5aeb32070ce03cc43 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Sat, 11 Sep 2021 13:17:05 +0200 Subject: [PATCH 27/52] ... --- services/spar/src/Spar/Data.hs | 48 ++++++++++++++++------------------ 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index 0c0671d7cb..0ec5ec17b7 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -531,32 +531,25 @@ getIdPConfig idpid = data GetIdPResult a = GetIdPFound a | GetIdPNotFound + | -- | IdPId has been found, but no IdPConfig matching that Id. (Database + -- inconsistency or race condition.) + GetIdPDanglingId SAML.IdPId | -- | you were looking for an idp by just providing issuer, not teamid, and `issuer_idp_v2` -- has more than one entry (for different teams). - GetIdPNonUniqueAnyTeam [a] + GetIdPNonUnique [SAML.IdPId] | -- | If you provide a teamid, and the idp retrieved is found just by issuer, and lives in -- another team. this should be handled similarly to NotFound in most cases. - GetIdPWrongTeam a + GetIdPWrongTeam SAML.IdPId deriving (Eq, Show) --- | we don't need the functor property, this is just for fun. -instance Functor GetIdPResult where - fmap f (GetIdPFound a) = GetIdPFound (f a) - fmap _ GetIdPNotFound = GetIdPNotFound - fmap f (GetIdPNonUniqueAnyTeam as) = GetIdPNonUniqueAnyTeam (f <$> as) - fmap f (GetIdPWrongTeam a) = GetIdPWrongTeam (f a) - -_mapGetIdPResult_simple :: (HasCallStack, MonadClient m) => (a -> m b) -> GetIdPResult a -> m (GetIdPResult b) -_mapGetIdPResult_simple f (GetIdPFound a) = GetIdPFound <$> f a -_mapGetIdPResult_simple _ GetIdPNotFound = pure GetIdPNotFound -_mapGetIdPResult_simple f (GetIdPNonUniqueAnyTeam as) = GetIdPNonUniqueAnyTeam <$> (f `mapM` as) -_mapGetIdPResult_simple f (GetIdPWrongTeam a) = GetIdPWrongTeam <$> f a - -mapGetIdPResult :: (HasCallStack, MonadClient m) => (a -> m (Maybe b)) -> GetIdPResult a -> m (GetIdPResult b) -mapGetIdPResult = do - -- TODO: the simple one doesn't work, but this one is stupid. we want to throw something if - -- we have an id, but can't find the idp for it. - undefined +-- | (There are probably category theoretical models for what we're doing here, but it's more +-- straight-forward to just handle the one instance we need.) +mapGetIdPResult :: (HasCallStack, MonadClient m) => (SAML.IdPId -> m (Maybe IdP)) -> GetIdPResult SAML.IdPId -> m (GetIdPResult IdP) +mapGetIdPResult f (GetIdPFound i) = f i <&> maybe (GetIdPDanglingId i) GetIdPFound +mapGetIdPResult _ GetIdPNotFound = pure GetIdPNotFound +mapGetIdPResult _ (GetIdPDanglingId i) = pure (GetIdPDanglingId i) +mapGetIdPResult _ (GetIdPNonUnique is) = pure (GetIdPNonUnique is) +mapGetIdPResult _ (GetIdPWrongTeam i) = pure (GetIdPWrongTeam i) -- | See 'getIdPIdByIssuer'. getIdPConfigByIssuer :: @@ -597,11 +590,14 @@ getIdPIdByIssuerAllowOld issuer mbteam = do mbv1v2 <- maybe (getIdPIdByIssuerOld issuer) (pure . GetIdPFound) mbv2 case (mbv1v2, mbteam) of (GetIdPFound idpid, Just team) -> do - idp <- getIdPConfig idpid >>= error "TODO: handle nothing better" -- maybe (error "TODO: database inconsistency") pure - pure $ - if idp ^. SAML.idpExtraInfo . wiTeam /= team - then GetIdPWrongTeam idpid - else mbv1v2 + getIdPConfig idpid >>= \case + Nothing -> do + pure $ GetIdPDanglingId idpid + Just idp -> + pure $ + if idp ^. SAML.idpExtraInfo . wiTeam /= team + then GetIdPWrongTeam idpid + else mbv1v2 _ -> pure mbv1v2 -- | Find 'IdPId' without team. Search both `issuer_idp` and `issuer_idp_v2`; in the latter, @@ -617,7 +613,7 @@ getIdPIdByIssuerOld issuer = do (runIdentity <$$> retry x1 (query selv2 $ params Quorum (Identity issuer))) >>= \case [] -> pure GetIdPNotFound [idpid] -> pure $ GetIdPFound idpid - idpids@(_ : _ : _) -> pure $ GetIdPNonUniqueAnyTeam idpids + idpids@(_ : _ : _) -> pure $ GetIdPNonUnique idpids where sel :: PrepQuery R (Identity SAML.Issuer) (Identity SAML.IdPId) sel = "SELECT idp FROM issuer_idp WHERE issuer = ?" From 689caa21bfae78e15a1028c393d5910fc73ea244 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Sat, 11 Sep 2021 20:54:55 +0200 Subject: [PATCH 28/52] ... --- services/spar/src/Spar/API.hs | 29 ++++++++++++------- services/spar/src/Spar/App.hs | 13 +++++---- services/spar/src/Spar/Error.hs | 5 ++-- .../test-integration/Test/Spar/DataSpec.hs | 10 +++---- 4 files changed, 34 insertions(+), 23 deletions(-) diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 099a86fa1d..d08b6f5e8e 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -133,7 +133,7 @@ authreq _ DoInitiateBind Nothing _ _ _ = throwSpar SparInitBindWithoutAuth authreq authreqttl _ zusr msucc merr idpid = do vformat <- validateAuthreqParams msucc merr form@(SAML.FormRedirect _ ((^. SAML.rqID) -> reqid)) <- do - idp :: IdP <- wrapMonadClient (Data.getIdPConfig idpid) >>= maybe (throwSpar SparIdPNotFound) pure + idp :: IdP <- wrapMonadClient (Data.getIdPConfig idpid) >>= maybe (throwSpar (SparIdPNotFound (cs $ show idpid))) pure let mbtid :: Maybe TeamId mbtid = case fromMaybe defWireIdPAPIVersion (idp ^. SAML.idpExtraInfo . wiApiVersion) of WireIdPAPIV1 -> Nothing @@ -216,7 +216,7 @@ idpGetRaw zusr idpid = do _ <- authorizeIdP zusr idp wrapMonadClient (Data.getIdPRawMetadata idpid) >>= \case Just txt -> pure $ RawIdPMetadata txt - Nothing -> throwSpar SparIdPNotFound + Nothing -> throwSpar $ SparIdPNotFound (cs $ show idpid) idpGetAll :: Maybe UserId -> Spar IdPList idpGetAll zusr = withDebugLog "idpGetAll" (const Nothing) $ do @@ -277,8 +277,9 @@ idpDelete zusr idpid (fromMaybe False -> purge) = withDebugLog "idpDelete" (cons updateReplacingIdP :: IdP -> Spar () updateReplacingIdP idp = forM_ (idp ^. SAML.idpExtraInfo . wiOldIssuers) $ \oldIssuer -> do wrapMonadClient $ do - iid <- Data.getIdPIdByIssuer oldIssuer (idp ^. SAML.idpExtraInfo . wiTeam) - mapM_ (Data.clearReplacedBy . Data.Replaced) iid + Data.getIdPIdByIssuer oldIssuer (idp ^. SAML.idpExtraInfo . wiTeam) >>= \case + Data.GetIdPFound iid -> Data.clearReplacedBy $ Data.Replaced iid + _ -> pure () -- | This handler only does the json parsing, and leaves all authorization checks and -- application logic to 'idpCreateXML'. @@ -340,14 +341,13 @@ validateNewIdP apiversion _idpMetadata teamId mReplaces = do oldIssuers :: [SAML.Issuer] <- case mReplaces of Nothing -> pure [] Just replaces -> do - idp <- wrapMonadClient (Data.getIdPConfig replaces) >>= maybe (throwSpar SparIdPNotFound) pure + idp <- wrapMonadClient (Data.getIdPConfig replaces) >>= maybe (throwSpar (SparIdPNotFound (cs $ show mReplaces))) pure pure $ (idp ^. SAML.idpMetadata . SAML.edIssuer) : (idp ^. SAML.idpExtraInfo . wiOldIssuers) let requri = _idpMetadata ^. SAML.edRequestURI _idpExtraInfo = WireIdP teamId (Just apiversion) oldIssuers Nothing enforceHttps requri wrapMonadClient (Data.getIdPConfigByIssuer (_idpMetadata ^. SAML.edIssuer) teamId) >>= \case - Nothing -> pure () - Just idp' -> case apiversion of + Data.GetIdPFound idp' -> case apiversion of WireIdPAPIV1 -> do throwSpar $ SparNewIdPAlreadyInUse "you can't create an IdP with api-version v1 if the issuer is already in use on the wire instance." WireIdPAPIV2 -> do @@ -355,6 +355,10 @@ validateNewIdP apiversion _idpMetadata teamId mReplaces = do throwSpar $ SparNewIdPAlreadyInUse "only allow all-new IdPs, no combination of old and new IdPs." 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." + Data.GetIdPNotFound -> pure () + res@(Data.GetIdPDanglingId _) -> throwSpar . SparIdPNotFound . cs . show $ res + res@(Data.GetIdPNonUnique _) -> throwSpar . SparIdPNotFound . cs . show $ res + res@(Data.GetIdPWrongTeam _) -> throwSpar . SparIdPNotFound . cs . show $ res pure SAML.IdPConfig {..} -- | FUTUREWORK: 'idpUpdateXML' is only factored out of this function for symmetry with @@ -400,9 +404,12 @@ validateIdPUpdate zusr _idpMetadata _idpId = do then pure $ previousIdP ^. SAML.idpExtraInfo else do foundConfig <- wrapMonadClient (Data.getIdPConfigByIssuerAllowOld newIssuer (Just teamId)) - let notInUseByOthers = case foundConfig of - Nothing -> True - Just c -> c ^. SAML.idpId == _idpId + notInUseByOthers <- case foundConfig of + Data.GetIdPFound c -> pure $ c ^. SAML.idpId == _idpId + Data.GetIdPNotFound -> pure $ True + res@(Data.GetIdPDanglingId _) -> throwSpar . SparIdPNotFound . cs . show $ res + res@(Data.GetIdPNonUnique _) -> throwSpar . SparIdPNotFound . cs . show $ res + res@(Data.GetIdPWrongTeam _) -> throwSpar . SparIdPNotFound . cs . show $ res if notInUseByOthers then pure $ (previousIdP ^. SAML.idpExtraInfo) & wiOldIssuers %~ nub . (previousIssuer :) else throwSpar SparIdPIssuerInUse @@ -463,7 +470,7 @@ internalPutSsoSettings SsoSettings {defaultSsoCode = Just code} = do -- this will return a 404, which is not quite right, -- but it's an internal endpoint and the message clearly says -- "Could not find IdP". - throwSpar SparIdPNotFound + throwSpar $ SparIdPNotFound mempty Just _ -> do wrapMonadClient $ Data.storeDefaultSsoCode code pure NoContent diff --git a/services/spar/src/Spar/App.hs b/services/spar/src/Spar/App.hs index f0a16cc3e2..8c65acda7a 100644 --- a/services/spar/src/Spar/App.hs +++ b/services/spar/src/Spar/App.hs @@ -152,13 +152,16 @@ instance SPStoreIdP SparError Spar where storeIdPConfig idp = wrapMonadClient $ Data.storeIdPConfig idp getIdPConfig :: IdPId -> Spar IdP - getIdPConfig = (>>= maybe (throwSpar SparIdPNotFound) pure) . wrapMonadClientWithEnv . Data.getIdPConfig - - getIdPConfigByIssuer :: Issuer -> TeamId -> Spar IdP - getIdPConfigByIssuer issuer = (>>= maybe (throwSpar SparIdPNotFound) pure) . wrapMonadClientWithEnv . Data.getIdPConfigByIssuer issuer + getIdPConfig = (>>= maybe (throwSpar (SparIdPNotFound mempty)) pure) . wrapMonadClientWithEnv . Data.getIdPConfig getIdPConfigByIssuerOptionalSPId :: Issuer -> Maybe TeamId -> Spar IdP - getIdPConfigByIssuerOptionalSPId issuer = (>>= maybe (throwSpar SparIdPNotFound) pure) . wrapMonadClientWithEnv . Data.getIdPConfigByIssuerAllowOld issuer + getIdPConfigByIssuerOptionalSPId issuer mbteam = do + wrapMonadClientWithEnv (Data.getIdPConfigByIssuerAllowOld issuer mbteam) >>= \case + Data.GetIdPFound idp -> pure idp + Data.GetIdPNotFound -> throwSpar $ SparIdPNotFound mempty + res@(Data.GetIdPDanglingId _) -> throwSpar $ SparIdPNotFound (cs $ show res) + res@(Data.GetIdPNonUnique _) -> throwSpar $ SparIdPNotFound (cs $ show res) + res@(Data.GetIdPWrongTeam _) -> throwSpar $ SparIdPNotFound (cs $ show res) -- | 'wrapMonadClient' with an 'Env' in a 'ReaderT', and exceptions. If you -- don't need either of those, 'wrapMonadClient' will suffice. diff --git a/services/spar/src/Spar/Error.hs b/services/spar/src/Spar/Error.hs index f82d7257b3..6b982e19ec 100644 --- a/services/spar/src/Spar/Error.hs +++ b/services/spar/src/Spar/Error.hs @@ -68,7 +68,7 @@ throwSpar :: MonadError SparError m => SparCustomError -> m a throwSpar = throwError . SAML.CustomError data SparCustomError - = SparIdPNotFound + = SparIdPNotFound LT | SparSamlCredentialsNotFound | SparMissingZUsr | SparNotInTeam @@ -158,7 +158,8 @@ renderSparError SAML.BadSamlResponseIssuerMissing = Right $ Wai.mkError status40 renderSparError SAML.BadSamlResponseNoAssertions = Right $ Wai.mkError status400 "bad-response-saml" ("Bad response: no assertions in AuthnResponse") renderSparError SAML.BadSamlResponseAssertionWithoutID = Right $ Wai.mkError status400 "bad-response-saml" ("Bad response: assertion without ID") renderSparError (SAML.BadSamlResponseInvalidSignature msg) = Right $ Wai.mkError status400 "bad-response-signature" (cs msg) -renderSparError (SAML.CustomError SparIdPNotFound) = Right $ Wai.mkError status404 "not-found" "Could not find IdP." +renderSparError (SAML.CustomError (SparIdPNotFound "")) = Right $ Wai.mkError status404 "not-found" "Could not find IdP." +renderSparError (SAML.CustomError (SparIdPNotFound msg)) = Right $ Wai.mkError status404 "not-found" ("Could not find IdP: " <> msg) renderSparError (SAML.CustomError SparSamlCredentialsNotFound) = Right $ Wai.mkError status404 "not-found" "Could not find SAML credentials, and auto-provisioning is disabled." renderSparError (SAML.CustomError SparMissingZUsr) = Right $ Wai.mkError status400 "client-error" "[header] 'Z-User' required" renderSparError (SAML.CustomError SparNotInTeam) = Right $ Wai.mkError status403 "no-team-member" "Requesting user is not a team member or not a member of this team." diff --git a/services/spar/test-integration/Test/Spar/DataSpec.hs b/services/spar/test-integration/Test/Spar/DataSpec.hs index 2a911bc3ad..0662956fc6 100644 --- a/services/spar/test-integration/Test/Spar/DataSpec.hs +++ b/services/spar/test-integration/Test/Spar/DataSpec.hs @@ -171,12 +171,12 @@ spec = do idp <- makeTestIdP () <- runSparCass $ Data.storeIdPConfig idp midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) - liftIO $ midp `shouldBe` Just idp + liftIO $ midp `shouldBe` GetIdPFound idp it "getIdPIdByIssuer works" $ do idp <- makeTestIdP () <- runSparCass $ Data.storeIdPConfig idp midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) - liftIO $ midp `shouldBe` Just (idp ^. idpId) + liftIO $ midp `shouldBe` GetIdPFound (idp ^. idpId) it "getIdPConfigsByTeam works" $ do skipIdPAPIVersions [WireIdPAPIV1] teamid <- nextWireId @@ -198,10 +198,10 @@ spec = do liftIO $ midp `shouldBe` Nothing do midp <- runSparCass $ Data.getIdPConfigByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) - liftIO $ midp `shouldBe` Nothing + liftIO $ midp `shouldBe` GetIdPNotFound do midp <- runSparCass $ Data.getIdPIdByIssuer (idp ^. idpMetadata . edIssuer) (idp ^. SAML.idpExtraInfo . wiTeam) - liftIO $ midp `shouldBe` Nothing + liftIO $ midp `shouldBe` GetIdPNotFound do idps <- runSparCass $ Data.getIdPConfigsByTeam teamid liftIO $ idps `shouldBe` [] @@ -305,7 +305,7 @@ testDeleteTeam = it "cleans up all the right tables after deletion" $ do do let issuer = idp ^. SAML.idpMetadata . SAML.edIssuer mbIdp <- runSparCass $ Data.getIdPIdByIssuer issuer (idp ^. SAML.idpExtraInfo . wiTeam) - liftIO $ mbIdp `shouldBe` Nothing + liftIO $ mbIdp `shouldBe` GetIdPNotFound -- The config from 'team_idp': do idps <- runSparCass $ Data.getIdPConfigsByTeam tid From 67cc4208707d2e59e81f1e0d0566d8b43cb0fbb6 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Sat, 11 Sep 2021 21:09:06 +0200 Subject: [PATCH 29/52] ... --- services/spar/src/Spar/API.hs | 6 +++--- services/spar/src/Spar/Data.hs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index d08b6f5e8e..36627710c3 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -356,9 +356,9 @@ validateNewIdP apiversion _idpMetadata teamId mReplaces = 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." Data.GetIdPNotFound -> pure () - res@(Data.GetIdPDanglingId _) -> throwSpar . SparIdPNotFound . cs . show $ res - res@(Data.GetIdPNonUnique _) -> throwSpar . SparIdPNotFound . cs . show $ res - res@(Data.GetIdPWrongTeam _) -> throwSpar . SparIdPNotFound . cs . show $ res + res@(Data.GetIdPDanglingId _) -> throwSpar . SparIdPNotFound . cs . show $ res -- database inconsistency + res@(Data.GetIdPNonUnique _) -> throwSpar . SparIdPNotFound . cs . show $ res -- impossible + Data.GetIdPWrongTeam _ -> pure () -- (it's ok to use the same IdP issuer / entityID in different teams.) pure SAML.IdPConfig {..} -- | FUTUREWORK: 'idpUpdateXML' is only factored out of this function for symmetry with diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index 0ec5ec17b7..fb4c3fc293 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -534,11 +534,11 @@ data GetIdPResult a | -- | IdPId has been found, but no IdPConfig matching that Id. (Database -- inconsistency or race condition.) GetIdPDanglingId SAML.IdPId - | -- | you were looking for an idp by just providing issuer, not teamid, and `issuer_idp_v2` + | -- | You were looking for an idp by just providing issuer, not teamid, and `issuer_idp_v2` -- has more than one entry (for different teams). GetIdPNonUnique [SAML.IdPId] - | -- | If you provide a teamid, and the idp retrieved is found just by issuer, and lives in - -- another team. this should be handled similarly to NotFound in most cases. + | -- | An IdP was found, but it lives in another team than the one you were looking for. + -- This should be handled similarly to NotFound in most cases. GetIdPWrongTeam SAML.IdPId deriving (Eq, Show) From c230589533b95e15d2236b54795cf8ccec4bafe9 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 09:15:36 +0200 Subject: [PATCH 30/52] More debug logging. --- services/spar/src/Spar/App.hs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/services/spar/src/Spar/App.hs b/services/spar/src/Spar/App.hs index 8c65acda7a..04dd1b676d 100644 --- a/services/spar/src/Spar/App.hs +++ b/services/spar/src/Spar/App.hs @@ -360,16 +360,19 @@ verdictHandler cky aresp verdict = do -- [3/4.1.4.2] -- [...] If the containing message is in response to an , then -- the InResponseTo attribute MUST match the request's ID. + SAML.logger SAML.Debug $ "entering verdictHandler: " <> show (fromBindCookie <$> cky, aresp, verdict) reqid <- either (throwSpar . SparNoRequestRefInResponse . cs) pure $ SAML.rspInResponseTo aresp format :: Maybe VerdictFormat <- wrapMonadClient $ Data.getVerdictFormat reqid - case format of + resp <- case format of Just (VerdictFormatWeb) -> verdictHandlerResult cky verdict >>= verdictHandlerWeb Just (VerdictFormatMobile granted denied) -> verdictHandlerResult cky verdict >>= verdictHandlerMobile granted denied - Nothing -> throwSpar SparNoSuchRequest - --- (this shouldn't happen too often, see 'storeVerdictFormat') + Nothing -> + -- (this shouldn't happen too often, see 'storeVerdictFormat') + throwSpar SparNoSuchRequest + SAML.logger SAML.Debug $ "leaving verdictHandler: " <> show resp + pure resp data VerdictHandlerResult = VerifyHandlerGranted {_vhrCookie :: SetCookie, _vhrUserId :: UserId} @@ -379,8 +382,9 @@ data VerdictHandlerResult verdictHandlerResult :: HasCallStack => Maybe BindCookie -> SAML.AccessVerdict -> Spar VerdictHandlerResult verdictHandlerResult bindCky verdict = do + SAML.logger SAML.Debug $ "entering verdictHandlerResult: " <> show (fromBindCookie <$> bindCky) result <- catchVerdictErrors $ verdictHandlerResultCore bindCky verdict - SAML.logger SAML.Debug (show result) + SAML.logger SAML.Debug $ "leaving verdictHandlerResult" <> show result pure result catchVerdictErrors :: Spar VerdictHandlerResult -> Spar VerdictHandlerResult From 0c0e29290308b891df27772e9830e73aa21e4d60 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 11:26:35 +0200 Subject: [PATCH 31/52] Fix: pass teamid from path to all the functions in finalize-login. --- services/spar/src/Spar/API.hs | 2 +- services/spar/src/Spar/App.hs | 40 +++++++++---------- .../test-integration/Test/Spar/AppSpec.hs | 7 +++- 3 files changed, 27 insertions(+), 22 deletions(-) diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 36627710c3..a9334de15b 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -184,7 +184,7 @@ authresp mbtid ckyraw arbody = logErrors $ SAML.authresp mbtid (sparSPIssuer mbt go :: SAML.AuthnResponse -> SAML.AccessVerdict -> Spar Void go resp verdict = do - result :: SAML.ResponseVerdict <- verdictHandler cky resp verdict + result :: SAML.ResponseVerdict <- verdictHandler cky mbtid resp verdict throwError $ SAML.CustomServant result logErrors :: Spar Void -> Spar Void diff --git a/services/spar/src/Spar/App.hs b/services/spar/src/Spar/App.hs index 04dd1b676d..123bafc110 100644 --- a/services/spar/src/Spar/App.hs +++ b/services/spar/src/Spar/App.hs @@ -246,16 +246,16 @@ createSamlUserWithId idp buid suid = do -- | If the team has no scim token, call 'createSamlUser'. Otherwise, raise "invalid -- credentials". -autoprovisionSamlUser :: SAML.UserRef -> Spar UserId -autoprovisionSamlUser suid = do +autoprovisionSamlUser :: Maybe TeamId -> SAML.UserRef -> Spar UserId +autoprovisionSamlUser mbteam suid = do buid <- Id <$> liftIO UUID.nextRandom - autoprovisionSamlUserWithId buid suid + autoprovisionSamlUserWithId mbteam buid suid pure buid -- | Like 'autoprovisionSamlUser', but for an already existing 'UserId'. -autoprovisionSamlUserWithId :: UserId -> SAML.UserRef -> Spar () -autoprovisionSamlUserWithId buid suid = do - idp <- getIdPConfigByIssuerOptionalSPId (suid ^. uidTenant) Nothing +autoprovisionSamlUserWithId :: Maybe TeamId -> UserId -> SAML.UserRef -> Spar () +autoprovisionSamlUserWithId mbteam buid suid = do + idp <- getIdPConfigByIssuerOptionalSPId (suid ^. uidTenant) mbteam guardReplacedIdP idp guardScimTokens idp createSamlUserWithId idp buid suid @@ -355,8 +355,8 @@ instance Intra.MonadSparToGalley Spar where -- signed in-response-to info in the assertions matches the unsigned in-response-to field in the -- 'SAML.Response', and fills in the response id in the header if missing, we can just go for the -- latter. -verdictHandler :: HasCallStack => Maybe BindCookie -> SAML.AuthnResponse -> SAML.AccessVerdict -> Spar SAML.ResponseVerdict -verdictHandler cky aresp verdict = do +verdictHandler :: HasCallStack => Maybe BindCookie -> Maybe TeamId -> SAML.AuthnResponse -> SAML.AccessVerdict -> Spar SAML.ResponseVerdict +verdictHandler cky mbteam aresp verdict = do -- [3/4.1.4.2] -- [...] If the containing message is in response to an , then -- the InResponseTo attribute MUST match the request's ID. @@ -365,9 +365,9 @@ verdictHandler cky aresp verdict = do format :: Maybe VerdictFormat <- wrapMonadClient $ Data.getVerdictFormat reqid resp <- case format of Just (VerdictFormatWeb) -> - verdictHandlerResult cky verdict >>= verdictHandlerWeb + verdictHandlerResult cky mbteam verdict >>= verdictHandlerWeb Just (VerdictFormatMobile granted denied) -> - verdictHandlerResult cky verdict >>= verdictHandlerMobile granted denied + verdictHandlerResult cky mbteam verdict >>= verdictHandlerMobile granted denied Nothing -> -- (this shouldn't happen too often, see 'storeVerdictFormat') throwSpar SparNoSuchRequest @@ -380,10 +380,10 @@ data VerdictHandlerResult | VerifyHandlerError {_vhrLabel :: ST, _vhrMessage :: ST} deriving (Eq, Show) -verdictHandlerResult :: HasCallStack => Maybe BindCookie -> SAML.AccessVerdict -> Spar VerdictHandlerResult -verdictHandlerResult bindCky verdict = do +verdictHandlerResult :: HasCallStack => Maybe BindCookie -> Maybe TeamId -> SAML.AccessVerdict -> Spar VerdictHandlerResult +verdictHandlerResult bindCky mbteam verdict = do SAML.logger SAML.Debug $ "entering verdictHandlerResult: " <> show (fromBindCookie <$> bindCky) - result <- catchVerdictErrors $ verdictHandlerResultCore bindCky verdict + result <- catchVerdictErrors $ verdictHandlerResultCore bindCky mbteam verdict SAML.logger SAML.Debug $ "leaving verdictHandlerResult" <> show result pure result @@ -401,9 +401,9 @@ 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 :: SAML.UserRef -> Spar (Maybe (SAML.UserRef, UserId)) -findUserWithOldIssuer (SAML.UserRef issuer subject) = do - idp <- getIdPConfigByIssuerOptionalSPId issuer Nothing +findUserWithOldIssuer :: Maybe TeamId -> SAML.UserRef -> Spar (Maybe (SAML.UserRef, UserId)) +findUserWithOldIssuer 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 @@ -419,8 +419,8 @@ moveUserToNewIssuer oldUserRef newUserRef uid = do Intra.setBrigUserVeid uid (UrefOnly newUserRef) wrapMonadClient $ Data.deleteSAMLUser uid oldUserRef -verdictHandlerResultCore :: HasCallStack => Maybe BindCookie -> SAML.AccessVerdict -> Spar VerdictHandlerResult -verdictHandlerResultCore bindCky = \case +verdictHandlerResultCore :: HasCallStack => Maybe BindCookie -> Maybe TeamId -> SAML.AccessVerdict -> Spar VerdictHandlerResult +verdictHandlerResultCore bindCky mbteam = \case SAML.AccessDenied reasons -> do pure $ VerifyHandlerDenied reasons SAML.AccessGranted userref -> do @@ -433,12 +433,12 @@ verdictHandlerResultCore bindCky = \case viaSparCassandraOldIssuer <- if isJust viaSparCassandra then pure Nothing - else findUserWithOldIssuer userref + else findUserWithOldIssuer mbteam userref case (viaBindCookie, viaSparCassandra, viaSparCassandraOldIssuer) of -- 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 userref + (Nothing, Nothing, Nothing) -> 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 -- SSO re-authentication (the most common case). diff --git a/services/spar/test-integration/Test/Spar/AppSpec.hs b/services/spar/test-integration/Test/Spar/AppSpec.hs index a2abfd28d0..a357d2b409 100644 --- a/services/spar/test-integration/Test/Spar/AppSpec.hs +++ b/services/spar/test-integration/Test/Spar/AppSpec.hs @@ -166,7 +166,12 @@ requestAccessVerdict idp isGranted mkAuthnReq = do if isGranted then SAML.AccessGranted uref else SAML.AccessDenied [DeniedNoBearerConfSubj, DeniedNoAuthnStatement] - outcome :: ResponseVerdict <- runSpar $ Spar.verdictHandler Nothing authnresp verdict + outcome :: ResponseVerdict <- do + mbteam <- + asks (^. teWireIdPAPIVersion) <&> \case + User.WireIdPAPIV1 -> Nothing + User.WireIdPAPIV2 -> Just (idp ^. SAML.idpExtraInfo . User.wiTeam) + runSpar $ Spar.verdictHandler Nothing mbteam authnresp verdict let loc :: URI.URI loc = maybe (error "no location") (either error id . SAML.parseURI' . cs) From eb1fdf6ee2a1478562f784d24e62363fabf3050c Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 12:47:19 +0200 Subject: [PATCH 32/52] Fix: delete now hits issuer_idp_v2. --- services/spar/src/Spar/Data.hs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index fb4c3fc293..35bb957897 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -655,14 +655,21 @@ deleteIdPConfig idp issuer team = retry x5 . batch $ do addPrepQuery delDefaultIdp (Identity idp) addPrepQuery delIdp (Identity idp) addPrepQuery delIssuerIdp (Identity issuer) + addPrepQuery delIssuerIdpV2 (Identity issuer) addPrepQuery delTeamIdp (team, idp) where delDefaultIdp :: PrepQuery W (Identity SAML.IdPId) () delDefaultIdp = "DELETE FROM default_idp WHERE partition_key_always_default = 'default' AND idp = ?" + delIdp :: PrepQuery W (Identity SAML.IdPId) () delIdp = "DELETE FROM idp WHERE idp = ?" + delIssuerIdp :: PrepQuery W (Identity SAML.Issuer) () delIssuerIdp = "DELETE FROM issuer_idp WHERE issuer = ?" + + delIssuerIdpV2 :: PrepQuery W (Identity SAML.Issuer) () + delIssuerIdpV2 = "DELETE FROM issuer_idp_v2 WHERE issuer = ?" + delTeamIdp :: PrepQuery W (TeamId, SAML.IdPId) () delTeamIdp = "DELETE FROM team_idp WHERE team = ? and idp = ?" From 7203c7f4de0faed73d03820803f6a32830b4d82a Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 12:52:29 +0200 Subject: [PATCH 33/52] Fix tests: replacing idps. --- .../test-integration/Test/Spar/APISpec.hs | 12 ++++----- services/spar/test-integration/Util/Core.hs | 26 ++++++++++++++----- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 74ca975fdf..9d2fd860a2 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -1064,7 +1064,7 @@ specCRUDIdentityProvider = do issuer2 <- makeIssuer idp2 <- let idpmeta2 = idpmeta1 & edIssuer .~ issuer2 - in call $ callIdpCreateReplace (env ^. teSpar) (Just owner1) idpmeta2 (idp1 ^. SAML.idpId) + in call $ callIdpCreateReplace (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner1) idpmeta2 (idp1 ^. SAML.idpId) idp1' <- call $ callIdpGet (env ^. teSpar) (Just owner1) (idp1 ^. SAML.idpId) idp2' <- call $ callIdpGet (env ^. teSpar) (Just owner1) (idp2 ^. SAML.idpId) liftIO $ do @@ -1094,7 +1094,7 @@ specCRUDIdentityProvider = do issuer2 <- makeIssuer _ <- let idpmeta2 = idpmeta1 & edIssuer .~ issuer2 - in call $ callIdpCreateReplace (env ^. teSpar) (Just owner1) idpmeta2 (idp1 ^. SAML.idpId) + in call $ callIdpCreateReplace (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner1) idpmeta2 (idp1 ^. SAML.idpId) newuref <- tryLogin privkey1 idp1 userSubject newuid <- getUserIdViaRef' newuref liftIO $ do @@ -1113,7 +1113,7 @@ specCRUDIdentityProvider = do issuer2 <- makeIssuer idp2 <- let idpmeta2 = idpmeta1 & edIssuer .~ issuer2 - in call $ callIdpCreateReplace (env ^. teSpar) (Just owner1) idpmeta2 (idp1 ^. SAML.idpId) + in call $ callIdpCreateReplace (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner1) idpmeta2 (idp1 ^. SAML.idpId) newuref <- tryLogin privkey2 idp2 userSubject newuid <- getUserIdViaRef' newuref liftIO $ do @@ -1130,7 +1130,7 @@ specCRUDIdentityProvider = do issuer2 <- makeIssuer idp2 <- let idpmeta2 = idpmeta1 & edIssuer .~ issuer2 - in call $ callIdpCreateReplace (env ^. teSpar) (Just owner1) idpmeta2 (idp1 ^. SAML.idpId) + in call $ callIdpCreateReplace (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner1) idpmeta2 (idp1 ^. SAML.idpId) newuref <- tryLogin privkey2 idp2 userSubject newuid <- getUserIdViaRef' newuref liftIO $ do @@ -1152,7 +1152,7 @@ specDeleteCornerCases = describe "delete corner cases" $ do uref `shouldBe` (SAML.UserRef issuer1 userSubject) idp2 <- let idpmeta2 = idpmeta1 & edIssuer .~ issuer2 - in call $ callIdpCreateReplace (env ^. teSpar) (Just owner1) idpmeta2 (idp1 ^. SAML.idpId) + in call $ callIdpCreateReplace (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner1) idpmeta2 (idp1 ^. SAML.idpId) call $ callIdpDelete (env ^. teSpar) (pure owner1) (idp2 ^. idpId) uref' <- tryLogin privkey1 idp1 userSubject uid' <- getUserIdViaRef' uref' @@ -1166,7 +1166,7 @@ specDeleteCornerCases = describe "delete corner cases" $ do issuer2 <- makeIssuer idp2 <- let idpmeta2 = idpmeta1 & edIssuer .~ issuer2 - in call $ callIdpCreateReplace (env ^. teSpar) (Just owner1) idpmeta2 (idp1 ^. SAML.idpId) + in call $ callIdpCreateReplace (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner1) idpmeta2 (idp1 ^. SAML.idpId) call $ callIdpDelete (env ^. teSpar) (pure owner1) (idp2 ^. idpId) let userSubject = SAML.unspecifiedNameID "bloob" uref <- tryLogin privkey1 idp1 userSubject diff --git a/services/spar/test-integration/Util/Core.hs b/services/spar/test-integration/Util/Core.hs index 0a41a522c7..ed1f34343b 100644 --- a/services/spar/test-integration/Util/Core.hs +++ b/services/spar/test-integration/Util/Core.hs @@ -1108,20 +1108,34 @@ callIdpCreateRaw' sparreq_ muid ctyp metadata = do . body (RequestBodyLBS metadata) . header "Content-Type" ctyp -callIdpCreateReplace :: (MonadIO m, MonadHttp m) => SparReq -> Maybe UserId -> IdPMetadata -> IdPId -> m IdP -callIdpCreateReplace sparreq_ muid metadata idpid = do - resp <- callIdpCreateReplace' (sparreq_ . expect2xx) muid metadata idpid +callIdpCreateReplace :: (MonadIO m, MonadHttp m) => WireIdPAPIVersion -> SparReq -> Maybe UserId -> IdPMetadata -> IdPId -> m IdP +callIdpCreateReplace apiversion sparreq_ muid metadata idpid = do + resp <- callIdpCreateReplace' apiversion (sparreq_ . expect2xx) muid metadata idpid either (liftIO . throwIO . ErrorCall . show) pure $ responseJsonEither @IdP resp -callIdpCreateReplace' :: (MonadIO m, MonadHttp m) => SparReq -> Maybe UserId -> IdPMetadata -> IdPId -> m ResponseLBS -callIdpCreateReplace' 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 + -- 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", + case apiversion of + WireIdPAPIV1 -> if explicitQueryParam then Just "v1" else Nothing + WireIdPAPIV2 -> Just "v2" + ), + ( "replaces", + Just . cs . idPIdToST $ idpid + ) + ] + ) . body (RequestBodyLBS . cs $ SAML.encode metadata) - . queryItem "replaces" (cs $ idPIdToST idpid) . header "Content-Type" "application/xml" callIdpUpdate' :: (MonadIO m, MonadHttp m) => SparReq -> Maybe UserId -> IdPId -> IdPMetadataInfo -> m ResponseLBS From efafe7a37dc180ea4016ef2e343117c0c716d001 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 13:09:41 +0200 Subject: [PATCH 34/52] Fix: post several idps with same entityID. --- services/spar/src/Spar/API.hs | 36 ++++++++++++------- .../test-integration/Test/Spar/APISpec.hs | 18 +++++++--- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index a9334de15b..e68264ab29 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -336,7 +336,7 @@ validateNewIdP :: TeamId -> Maybe SAML.IdPId -> m IdP -validateNewIdP apiversion _idpMetadata teamId mReplaces = do +validateNewIdP apiversion _idpMetadata teamId mReplaces = withDebugLog "validateNewIdP" (Just . show . (^. SAML.idpId)) $ do _idpId <- SAML.IdPId <$> SAML.createUUID oldIssuers :: [SAML.Issuer] <- case mReplaces of Nothing -> pure [] @@ -346,19 +346,31 @@ validateNewIdP apiversion _idpMetadata teamId mReplaces = do let requri = _idpMetadata ^. SAML.edRequestURI _idpExtraInfo = WireIdP teamId (Just apiversion) oldIssuers Nothing enforceHttps requri - wrapMonadClient (Data.getIdPConfigByIssuer (_idpMetadata ^. SAML.edIssuer) teamId) >>= \case - Data.GetIdPFound idp' -> case apiversion of - WireIdPAPIV1 -> do - throwSpar $ SparNewIdPAlreadyInUse "you can't create an IdP with api-version v1 if the issuer is already in use on the wire instance." - WireIdPAPIV2 -> do - when (fromMaybe defWireIdPAPIVersion (idp' ^. SAML.idpExtraInfo . wiApiVersion) == WireIdPAPIV1) $ do - throwSpar $ SparNewIdPAlreadyInUse "only allow all-new IdPs, no combination of old and new IdPs." - 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." + idp <- wrapMonadClient (Data.getIdPConfigByIssuer (_idpMetadata ^. SAML.edIssuer) teamId) + SAML.logger SAML.Debug $ show (apiversion, _idpMetadata, teamId, mReplaces) + SAML.logger SAML.Debug $ show (_idpId, oldIssuers, idp) + + let handleIdPClash :: Either SAML.IdPId IdP -> m () + handleIdPClash (Right idp') = case apiversion of + WireIdPAPIV1 -> do + throwSpar $ SparNewIdPAlreadyInUse "you can't create an IdP with api-version v1 if the issuer is already in use on the wire instance." + WireIdPAPIV2 -> do + when (fromMaybe defWireIdPAPIVersion (idp' ^. SAML.idpExtraInfo . wiApiVersion) == WireIdPAPIV1) $ do + throwSpar $ SparNewIdPAlreadyInUse "only allow all-new IdPs, no combination of old and new IdPs." + 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." + handleIdPClash (Left id') = do + let err = throwSpar . SparIdPNotFound . cs . show $ id' -- database inconsistency + idp' <- wrapMonadClient (Data.getIdPConfig id') >>= maybe err pure + handleIdPClash (Right idp') + + 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 - Data.GetIdPWrongTeam _ -> pure () -- (it's ok to use the same IdP issuer / entityID in different teams.) + Data.GetIdPWrongTeam id' {- different team -} -> handleIdPClash (Left id') + pure SAML.IdPConfig {..} -- | FUTUREWORK: 'idpUpdateXML' is only factored out of this function for symmetry with @@ -389,7 +401,7 @@ validateIdPUpdate :: SAML.IdPMetadata -> SAML.IdPId -> m (TeamId, IdP) -validateIdPUpdate zusr _idpMetadata _idpId = do +validateIdPUpdate zusr _idpMetadata _idpId = withDebugLog "validateNewIdP" (Just . show . (_2 %~ (^. SAML.idpId))) $ do previousIdP <- wrapMonadClient (Data.getIdPConfig _idpId) >>= \case Nothing -> throwError errUnknownIdPId diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 9d2fd860a2..3653c36f44 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -1015,15 +1015,25 @@ specCRUDIdentityProvider = do (SampleIdP newMetadata _ _ _) <- makeSampleIdPMetadata (uid1, _) <- call $ createUserWithTeam (env ^. teBrig) (env ^. teGalley) (uid2, _) <- call $ createUserWithTeam (env ^. teBrig) (env ^. teGalley) + -- first idp resp1 <- call $ callIdpCreate' (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just uid1) newMetadata + -- same idp issuer, same team resp2 <- call $ callIdpCreate' (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just uid1) newMetadata + -- same idp issuer, different team resp3 <- call $ callIdpCreate' (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just uid2) newMetadata liftIO $ do statusCode resp1 `shouldBe` 201 - statusCode resp2 `shouldBe` 400 - responseJsonEither resp2 `shouldBe` Right (TestErrorLabel "idp-already-in-use") - statusCode resp3 `shouldBe` 400 - responseJsonEither resp3 `shouldBe` Right (TestErrorLabel "idp-already-in-use") + do + -- always fail if we're trying same (SP entityID, IdP entityID, team) + statusCode resp2 `shouldBe` 400 + responseJsonEither resp2 `shouldBe` Right (TestErrorLabel "idp-already-in-use") + case env ^. teWireIdPAPIVersion of + -- fail in the old api only if we're trying same (SP entityID, IdP entityID) on different teams + WireIdPAPIV1 -> do + statusCode resp3 `shouldBe` 400 + responseJsonEither resp3 `shouldBe` Right (TestErrorLabel "idp-already-in-use") + WireIdPAPIV2 -> do + statusCode resp3 `shouldBe` 201 context "client is owner with email" $ do it "responds with 2xx; makes IdP available for GET /identity-providers/" $ do env <- ask From 97cf8c77f436b1fe850e3032ebe1a98143b169a7 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 14:38:52 +0200 Subject: [PATCH 35/52] Fix test case. --- services/spar/test-integration/Test/Spar/APISpec.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 3653c36f44..2d816f1141 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -359,7 +359,7 @@ specFinalizeLogin = do statusCode sparresp `shouldBe` 404 -- body should contain the error label in the title, the verbatim haskell error, and the request: (cs . fromJust . responseBody $ sparresp) `shouldContain` "wire:sso:error:not-found" - (cs . fromJust . responseBody $ sparresp) `shouldContainInBase64` "CustomError SparIdPNotFound" + (cs . fromJust . responseBody $ sparresp) `shouldContainInBase64` "CustomError (SparIdPNotFound" (cs . fromJust . responseBody $ sparresp) `shouldContainInBase64` "Input {iName = \"SAMLResponse\"" -- TODO(arianvp): Ask Matthias what this even means context "AuthnResponse does not match any request" $ do From a3ed0fb56bfa10c79620d8f1438a797de201fe39 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 14:51:38 +0200 Subject: [PATCH 36/52] Another fix. --- services/spar/src/Spar/API.hs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index e68264ab29..8ed8cf106a 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -418,10 +418,10 @@ validateIdPUpdate zusr _idpMetadata _idpId = withDebugLog "validateNewIdP" (Just foundConfig <- wrapMonadClient (Data.getIdPConfigByIssuerAllowOld newIssuer (Just teamId)) notInUseByOthers <- case foundConfig of Data.GetIdPFound c -> pure $ c ^. SAML.idpId == _idpId - Data.GetIdPNotFound -> pure $ True - res@(Data.GetIdPDanglingId _) -> throwSpar . SparIdPNotFound . cs . show $ res - res@(Data.GetIdPNonUnique _) -> throwSpar . SparIdPNotFound . cs . show $ res - res@(Data.GetIdPWrongTeam _) -> throwSpar . SparIdPNotFound . cs . show $ res + 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) + Data.GetIdPWrongTeam _ -> pure False if notInUseByOthers then pure $ (previousIdP ^. SAML.idpExtraInfo) & wiOldIssuers %~ nub . (previousIssuer :) else throwSpar SparIdPIssuerInUse From ad211e8a24b9e6e4160692ed38d3a5ad00e33608 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 14:58:08 +0200 Subject: [PATCH 37/52] Tweak race condition mitigation in test case. --- services/spar/test-integration/Util/Core.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/spar/test-integration/Util/Core.hs b/services/spar/test-integration/Util/Core.hs index ed1f34343b..03bda6dc4b 100644 --- a/services/spar/test-integration/Util/Core.hs +++ b/services/spar/test-integration/Util/Core.hs @@ -498,7 +498,7 @@ deleteUserOnBrig :: m () deleteUserOnBrig brigreq uid = do deleteUserNoWait brigreq uid - recoverAll (exponentialBackoff 30000 <> limitRetries 5) $ \_ -> do + recoverAll (exponentialBackoff 500000 <> limitRetries 5) $ \_ -> do profile <- getSelfProfile brigreq uid liftIO $ selfUser profile `shouldSatisfy` Brig.userDeleted From 333ff5480315460eae4f6085afa985a5ff7e27aa Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 16:00:46 +0200 Subject: [PATCH 38/52] Tweak. --- services/spar/test-integration/Test/Spar/APISpec.hs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 2d816f1141..6166508f67 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -240,15 +240,15 @@ specFinalizeLogin = do hasPersistentCookieHeader sparresp `shouldBe` Right () context "happy flow" $ do it "responds with a very peculiar 'allowed' HTTP response" $ do + env <- ask + let apiVersion = env ^. teWireIdPAPIVersion (_, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta - asks (^. teWireIdPAPIVersion) >>= \apiVersion -> do - liftIO $ fromMaybe defWireIdPAPIVersion (idp ^. idpExtraInfo . wiApiVersion) `shouldBe` apiVersion + liftIO $ fromMaybe defWireIdPAPIVersion (idp ^. idpExtraInfo . wiApiVersion) `shouldBe` apiVersion spmeta <- getTestSPMetadata tid authnreq <- negotiateAuthnRequest idp - audiencePath <- do - asks (^. teWireIdPAPIVersion) <&> \case - WireIdPAPIV1 -> "/sso/finalize-login" - WireIdPAPIV2 -> "/sso/finalize-login/" <> toByteString' tid + let audiencePath = case apiVersion of + WireIdPAPIV1 -> "/sso/finalize-login" + WireIdPAPIV2 -> "/sso/finalize-login/" <> toByteString' tid liftIO $ authnreq ^. rqIssuer . fromIssuer . to URI.uriPath `shouldBe` audiencePath authnresp <- runSimpleSP $ mkAuthnResponse privcreds idp spmeta authnreq True loginSuccess =<< submitAuthnResponse tid authnresp From 1d15c4e30865fda4a3c15bd6a3053704980b7695 Mon Sep 17 00:00:00 2001 From: fisx Date: Mon, 13 Sep 2021 16:04:24 +0200 Subject: [PATCH 39/52] Update services/spar/test-integration/Test/Spar/APISpec.hs --- services/spar/test-integration/Test/Spar/APISpec.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 6166508f67..04e2da3076 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -277,7 +277,7 @@ specFinalizeLogin = do 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 - spmeta <- getTestSPMetadata (idp ^. idpExtraInfo . wiTeam) + spmeta <- getTestSPMetadata teamid -- first login newUserAuthnResp :: SignedAuthnResponse <- do authnreq <- negotiateAuthnRequest idp From 3950ce66738ab90c45765042ed2033484329aab9 Mon Sep 17 00:00:00 2001 From: fisx Date: Mon, 13 Sep 2021 16:05:06 +0200 Subject: [PATCH 40/52] Update services/spar/test-integration/Test/Spar/APISpec.hs --- services/spar/test-integration/Test/Spar/APISpec.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 04e2da3076..8217d11369 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -332,7 +332,7 @@ specFinalizeLogin = do it "rejects" $ do (_, teamid, idp, (_, privcreds)) <- registerTestIdPWithMeta authnreq <- negotiateAuthnRequest idp - spmeta <- getTestSPMetadata (idp ^. idpExtraInfo . wiTeam) + spmeta <- getTestSPMetadata teamid authnresp <- runSimpleSP $ mkAuthnResponse From 178528fd9aec63c80150bc5006cac9db4db3f75f Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 16:28:25 +0200 Subject: [PATCH 41/52] ... --- services/spar/src/Spar/API.hs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 8ed8cf106a..27c36fd1db 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -279,7 +279,10 @@ idpDelete zusr idpid (fromMaybe False -> purge) = withDebugLog "idpDelete" (cons wrapMonadClient $ do Data.getIdPIdByIssuer oldIssuer (idp ^. SAML.idpExtraInfo . wiTeam) >>= \case Data.GetIdPFound iid -> Data.clearReplacedBy $ Data.Replaced iid - _ -> pure () + Data.GetIdPNotFound -> pure () + Data.GetIdPDanglingId _ -> pure () + Data.GetIdPNonUnique _ -> pure () + Data.GetIdPWrongTeam _ -> pure () -- | This handler only does the json parsing, and leaves all authorization checks and -- application logic to 'idpCreateXML'. From 5ede16ff2290febbacc77e19a563301ca45e0d4c Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 16:29:54 +0200 Subject: [PATCH 42/52] ... --- services/spar/src/Spar/API.hs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 27c36fd1db..8752531137 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -353,7 +353,9 @@ 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 -- FUTUREWORK: some database lookups here are unnecessary, this could be made more + -- efficient rather easily. + handleIdPClash :: Either SAML.IdPId IdP -> m () handleIdPClash (Right idp') = case apiversion of WireIdPAPIV1 -> do throwSpar $ SparNewIdPAlreadyInUse "you can't create an IdP with api-version v1 if the issuer is already in use on the wire instance." From 83f3bc5a161e96824737eef5b0b895a7053e2d5f Mon Sep 17 00:00:00 2001 From: Julia Longtin Date: Mon, 13 Sep 2021 18:39:50 +0100 Subject: [PATCH 43/52] add a changelog entry. --- changelog.d/2-features/pr-1755 | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog.d/2-features/pr-1755 diff --git a/changelog.d/2-features/pr-1755 b/changelog.d/2-features/pr-1755 new file mode 100644 index 0000000000..4030326e1d --- /dev/null +++ b/changelog.d/2-features/pr-1755 @@ -0,0 +1,3 @@ +Support using a single IDP with a single EntityID to set up two teams. +Sets up a migration, and makes teamID + EntityID unique, rather than relying on EntityID to be unique. +Required to support multiple teams in environments where the IDP software cannot present anything but one EntityID (E.G.: DualShield). \ No newline at end of file From a7bab5da07b494c713ae1e061d4a314d4077e2b9 Mon Sep 17 00:00:00 2001 From: jschaul Date: Mon, 13 Sep 2021 20:18:13 +0200 Subject: [PATCH 44/52] cassandra-schema docs --- docs/reference/cassandra-schema.cql | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/docs/reference/cassandra-schema.cql b/docs/reference/cassandra-schema.cql index cd0b9d85d5..e9383b3618 100644 --- a/docs/reference/cassandra-schema.cql +++ b/docs/reference/cassandra-schema.cql @@ -1513,6 +1513,7 @@ CREATE TABLE spar_test.issuer_idp ( CREATE TABLE spar_test.idp ( idp uuid PRIMARY KEY, + api_version int, extra_public_keys list, issuer text, old_issuers list, @@ -1681,6 +1682,27 @@ CREATE TABLE spar_test.team_idp ( AND read_repair_chance = 0.0 AND speculative_retry = '99PERCENTILE'; +CREATE TABLE spar_test.issuer_idp_v2 ( + issuer text, + team uuid, + idp uuid, + PRIMARY KEY (issuer, team) +) WITH CLUSTERING ORDER BY (team ASC) + AND bloom_filter_fp_chance = 0.1 + AND caching = {'keys': 'ALL', 'rows_per_partition': 'NONE'} + AND comment = '' + AND compaction = {'class': 'org.apache.cassandra.db.compaction.LeveledCompactionStrategy'} + AND compression = {'chunk_length_in_kb': '64', 'class': 'org.apache.cassandra.io.compress.LZ4Compressor'} + AND crc_check_chance = 1.0 + AND dclocal_read_repair_chance = 0.1 + AND default_time_to_live = 0 + AND gc_grace_seconds = 864000 + AND max_index_interval = 2048 + AND memtable_flush_period_in_ms = 0 + AND min_index_interval = 128 + AND read_repair_chance = 0.0 + AND speculative_retry = '99PERCENTILE'; + CREATE TABLE spar_test.scim_user_times ( uid uuid PRIMARY KEY, created_at timestamp, From 91de317a6b2ed1533198094dbb9bd5ea71feffd0 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 21:45:31 +0200 Subject: [PATCH 45/52] Nit-pick. --- changelog.d/2-features/pr-1755 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/2-features/pr-1755 b/changelog.d/2-features/pr-1755 index 4030326e1d..81624c4022 100644 --- a/changelog.d/2-features/pr-1755 +++ b/changelog.d/2-features/pr-1755 @@ -1,3 +1,3 @@ -Support using a single IDP with a single EntityID to set up two teams. +Support using a single IDP with a single EntityID (aka issuer ID) to set up two teams. Sets up a migration, and makes teamID + EntityID unique, rather than relying on EntityID to be unique. Required to support multiple teams in environments where the IDP software cannot present anything but one EntityID (E.G.: DualShield). \ No newline at end of file From 6f91c5788218c759f6c825afbbb364fe265d1f6e Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 22:00:43 +0200 Subject: [PATCH 46/52] Reduce code duplication. --- .../test-integration/Test/Spar/APISpec.hs | 41 ++++++++----------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 8217d11369..4b253837b6 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -133,30 +133,23 @@ specMisc = do specMetadata :: SpecWith TestEnv specMetadata = do describe "metadata" $ do - it "metadata (legacy)" $ do - env <- ask - get ((env ^. teSpar) . path "/sso/metadata" . expect2xx) - `shouldRespondWith` ( \(responseBody -> Just (cs -> bdy)) -> - all - (`isInfixOf` bdy) - [ "md:SPSSODescriptor", - "validUntil", - "WantAssertionsSigned=\"true\"", - "" - ] - ) - it "metadata (with team-id)" $ do - env <- ask - get ((env ^. teSpar) . path "/sso/metadata/3f1a01e2-1092-11ec-846d-3344b58840fe" . expect2xx) - `shouldRespondWith` ( \(responseBody -> Just (cs -> bdy)) -> - all - (`isInfixOf` bdy) - [ "md:SPSSODescriptor", - "validUntil", - "WantAssertionsSigned=\"true\"", - "" - ] - ) + let mkit :: String -> String -> SpecWith TestEnv + mkit mdpath finalizepath = do + it ("metadata (" <> mdpath <> ")") $ do + env <- ask + get ((env ^. teSpar) . path (cs mdpath) . expect2xx) + `shouldRespondWith` ( \(responseBody -> Just (cs -> bdy)) -> + all + (`isInfixOf` bdy) + [ "md:SPSSODescriptor", + "validUntil", + "WantAssertionsSigned=\"true\"", + " finalizepath <> "\" index=\"0\" isDefault=\"true\"/>" + ] + ) + + mkit "/sso/metadata" "http://localhost:8088/sso/finalize-login" + mkit "/sso/metadata/208f5cc4-14cd-11ec-b969-db4fdf0173d5" "http://localhost:8088/sso/finalize-login/208f5cc4-14cd-11ec-b969-db4fdf0173d5" specInitiateLogin :: SpecWith TestEnv specInitiateLogin = do From 8cfa08b02c7c4d5e384d406aab684dd7f1d9c2b0 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 22:08:25 +0200 Subject: [PATCH 47/52] Fix: test against sso-url from config. (not one that we falsely assume will always be configured.) --- services/spar/test-integration/Test/Spar/APISpec.hs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 4b253837b6..350b567039 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -82,6 +82,7 @@ import qualified Web.Scim.Schema.User as Scim import Wire.API.Cookie import Wire.API.Routes.Public.Spar import Wire.API.User.IdentityProvider +import qualified Wire.API.User.Saml as WireAPI (saml) import Wire.API.User.Scim spec :: SpecWith TestEnv @@ -137,6 +138,7 @@ specMetadata = do mkit mdpath finalizepath = do it ("metadata (" <> mdpath <> ")") $ do env <- ask + let sparHost = env ^. teOpts . to WireAPI.saml . SAML.cfgSPSsoURI . to (cs . SAML.renderURI) get ((env ^. teSpar) . path (cs mdpath) . expect2xx) `shouldRespondWith` ( \(responseBody -> Just (cs -> bdy)) -> all @@ -144,12 +146,12 @@ specMetadata = do [ "md:SPSSODescriptor", "validUntil", "WantAssertionsSigned=\"true\"", - " finalizepath <> "\" index=\"0\" isDefault=\"true\"/>" + " sparHost <> finalizepath <> "\" index=\"0\" isDefault=\"true\"/>" ] ) - mkit "/sso/metadata" "http://localhost:8088/sso/finalize-login" - mkit "/sso/metadata/208f5cc4-14cd-11ec-b969-db4fdf0173d5" "http://localhost:8088/sso/finalize-login/208f5cc4-14cd-11ec-b969-db4fdf0173d5" + mkit "/sso/metadata" "/finalize-login" + mkit "/sso/metadata/208f5cc4-14cd-11ec-b969-db4fdf0173d5" "/finalize-login/208f5cc4-14cd-11ec-b969-db4fdf0173d5" specInitiateLogin :: SpecWith TestEnv specInitiateLogin = do From 234f95a91e352a9092128a19effac749f79540d9 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 22:11:57 +0200 Subject: [PATCH 48/52] ormolu --- .../test-integration/Test/Spar/APISpec.hs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 350b567039..486c67cda2 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -139,16 +139,17 @@ specMetadata = do it ("metadata (" <> mdpath <> ")") $ do env <- ask let sparHost = env ^. teOpts . to WireAPI.saml . SAML.cfgSPSsoURI . to (cs . SAML.renderURI) + fragments = + [ "md:SPSSODescriptor", + "validUntil", + "WantAssertionsSigned=\"true\"", + " sparHost + <> finalizepath + <> "\" index=\"0\" isDefault=\"true\"/>" + ] get ((env ^. teSpar) . path (cs mdpath) . expect2xx) - `shouldRespondWith` ( \(responseBody -> Just (cs -> bdy)) -> - all - (`isInfixOf` bdy) - [ "md:SPSSODescriptor", - "validUntil", - "WantAssertionsSigned=\"true\"", - " sparHost <> finalizepath <> "\" index=\"0\" isDefault=\"true\"/>" - ] - ) + `shouldRespondWith` (\(responseBody -> Just (cs -> bdy)) -> all (`isInfixOf` bdy) fragments) mkit "/sso/metadata" "/finalize-login" mkit "/sso/metadata/208f5cc4-14cd-11ec-b969-db4fdf0173d5" "/finalize-login/208f5cc4-14cd-11ec-b969-db4fdf0173d5" From b9498f42d084c38609c0a9169a449d0c06c0dee3 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 22:18:18 +0200 Subject: [PATCH 49/52] ... --- services/spar/src/Spar/Data.hs | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index 35bb957897..e0203a83d6 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -544,12 +544,12 @@ data GetIdPResult a -- | (There are probably category theoretical models for what we're doing here, but it's more -- straight-forward to just handle the one instance we need.) -mapGetIdPResult :: (HasCallStack, MonadClient m) => (SAML.IdPId -> m (Maybe IdP)) -> GetIdPResult SAML.IdPId -> m (GetIdPResult IdP) -mapGetIdPResult f (GetIdPFound i) = f i <&> maybe (GetIdPDanglingId i) GetIdPFound -mapGetIdPResult _ GetIdPNotFound = pure GetIdPNotFound -mapGetIdPResult _ (GetIdPDanglingId i) = pure (GetIdPDanglingId i) -mapGetIdPResult _ (GetIdPNonUnique is) = pure (GetIdPNonUnique is) -mapGetIdPResult _ (GetIdPWrongTeam i) = pure (GetIdPWrongTeam i) +mapGetIdPResult :: (HasCallStack, MonadClient m) => GetIdPResult SAML.IdPId -> m (GetIdPResult IdP) +mapGetIdPResult (GetIdPFound i) = getIdPConfig i <&> maybe (GetIdPDanglingId i) GetIdPFound +mapGetIdPResult GetIdPNotFound = pure GetIdPNotFound +mapGetIdPResult (GetIdPDanglingId i) = pure (GetIdPDanglingId i) +mapGetIdPResult (GetIdPNonUnique is) = pure (GetIdPNonUnique is) +mapGetIdPResult (GetIdPWrongTeam i) = pure (GetIdPWrongTeam i) -- | See 'getIdPIdByIssuer'. getIdPConfigByIssuer :: @@ -558,7 +558,7 @@ getIdPConfigByIssuer :: TeamId -> m (GetIdPResult IdP) getIdPConfigByIssuer issuer = - getIdPIdByIssuer issuer >=> mapGetIdPResult getIdPConfig + getIdPIdByIssuer issuer >=> mapGetIdPResult -- | See 'getIdPIdByIssuerAllowOld'. getIdPConfigByIssuerAllowOld :: @@ -567,7 +567,7 @@ getIdPConfigByIssuerAllowOld :: Maybe TeamId -> m (GetIdPResult IdP) getIdPConfigByIssuerAllowOld issuer = do - getIdPIdByIssuerAllowOld issuer >=> mapGetIdPResult getIdPConfig + getIdPIdByIssuerAllowOld issuer >=> mapGetIdPResult -- | Lookup idp in table `issuer_idp_v2` (using both issuer entityID and teamid); if nothing -- is found there or if teamid is 'Nothing', lookup under issuer in `issuer_idp`. @@ -586,8 +586,8 @@ getIdPIdByIssuerAllowOld :: Maybe TeamId -> m (GetIdPResult SAML.IdPId) getIdPIdByIssuerAllowOld issuer mbteam = do - mbv2 <- maybe (pure Nothing) (getIdPIdByIssuerV2 issuer) mbteam - mbv1v2 <- maybe (getIdPIdByIssuerOld issuer) (pure . GetIdPFound) mbv2 + mbv2 <- maybe (pure Nothing) (getIdPIdByIssuerWithTeam issuer) mbteam + mbv1v2 <- maybe (getIdPIdByIssuerWithoutTeam issuer) (pure . GetIdPFound) mbv2 case (mbv1v2, mbteam) of (GetIdPFound idpid, Just team) -> do getIdPConfig idpid >>= \case @@ -602,11 +602,11 @@ getIdPIdByIssuerAllowOld issuer mbteam = do -- | Find 'IdPId' without team. Search both `issuer_idp` and `issuer_idp_v2`; in the latter, -- make sure the result is unique (no two IdPs for two different teams). -getIdPIdByIssuerOld :: +getIdPIdByIssuerWithoutTeam :: (HasCallStack, MonadClient m) => SAML.Issuer -> m (GetIdPResult SAML.IdPId) -getIdPIdByIssuerOld issuer = do +getIdPIdByIssuerWithoutTeam issuer = do (runIdentity <$$> retry x1 (query1 sel $ params Quorum (Identity issuer))) >>= \case Just idpid -> pure $ GetIdPFound idpid Nothing -> @@ -621,12 +621,12 @@ getIdPIdByIssuerOld issuer = do selv2 :: PrepQuery R (Identity SAML.Issuer) (Identity SAML.IdPId) selv2 = "SELECT idp FROM issuer_idp_v2 WHERE issuer = ?" -getIdPIdByIssuerV2 :: +getIdPIdByIssuerWithTeam :: (HasCallStack, MonadClient m) => SAML.Issuer -> TeamId -> m (Maybe SAML.IdPId) -getIdPIdByIssuerV2 issuer tid = do +getIdPIdByIssuerWithTeam issuer tid = do runIdentity <$$> retry x1 (query1 sel $ params Quorum (issuer, tid)) where sel :: PrepQuery R (SAML.Issuer, TeamId) (Identity SAML.IdPId) From 8cad1b2cd28bbbefcaabe6fda3b27159b0bf712b Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 22:24:51 +0200 Subject: [PATCH 50/52] ... --- services/spar/src/Spar/Data.hs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/services/spar/src/Spar/Data.hs b/services/spar/src/Spar/Data.hs index e0203a83d6..bbe5c2ca8c 100644 --- a/services/spar/src/Spar/Data.hs +++ b/services/spar/src/Spar/Data.hs @@ -325,18 +325,11 @@ getSAMLSomeUsersByIssuer issuer = sel :: PrepQuery R (Identity SAML.Issuer) (SAML.NameID, UserId) sel = "SELECT sso_id, uid FROM user_v2 WHERE issuer = ? LIMIT 2000" --- | ... +-- | Lookup a brig 'UserId' by IdP issuer and NameID. -- --- NB: It is not allowed for two distinct wire users from two different teams to ahave the --- same 'UserRef'. Rationale: with saml auto-provisioning, the users would be required to --- add distinguishing handles themselves, which would be at best confusing. with scim --- provisioning, the two accounts have to be distinct in the user data source, so it --- /should/ be straight-forward to give the two accounts different 'UserRef' values, even --- if the 'Issuer' is the same. --- --- ... this means we can get away with `user_v2`: pull `UserId`, assume it's always unique, --- get the `TeamId` from brig, pull all `TeamId`s associated with the given `Issuer` from --- `issuer_idp_v2`, and make sure the one from brig is in there. (if not: what's the error?) +-- NB: It is not allowed for two distinct wire users from two different teams to have the same +-- 'UserRef'. 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). getSAMLUser :: (HasCallStack, MonadClient m) => SAML.UserRef -> m (Maybe UserId) getSAMLUser uref = do mbUid <- getSAMLUserNew uref From 79488d5a824d6bdbc7fdc949db82c2c5294b7cbe Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 22:49:31 +0200 Subject: [PATCH 51/52] More explicit error case handling. --- services/spar/src/Spar/App.hs | 8 +++++++- services/spar/src/Spar/Error.hs | 2 ++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/services/spar/src/Spar/App.hs b/services/spar/src/Spar/App.hs index 123bafc110..2e243d33d6 100644 --- a/services/spar/src/Spar/App.hs +++ b/services/spar/src/Spar/App.hs @@ -301,7 +301,13 @@ bindUser buid userref = do oldStatus <- do let err :: Spar a err = throwSpar . SparBindFromWrongOrNoTeam . cs . show $ buid - teamid :: TeamId <- (^. idpExtraInfo . wiTeam) <$> getIdPConfigByIssuerOptionalSPId (userref ^. uidTenant) Nothing + teamid :: TeamId <- + wrapMonadClient (Data.getIdPConfigByIssuerAllowOld (userref ^. uidTenant) Nothing) >>= \case + 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.GetIdPWrongTeam _ -> err -- impossible acc <- Intra.getBrigUserAccount Intra.WithPendingInvitations buid >>= maybe err pure teamid' :: TeamId <- userTeam (accountUser acc) & maybe err pure unless (teamid' == teamid) err diff --git a/services/spar/src/Spar/Error.hs b/services/spar/src/Spar/Error.hs index 6b982e19ec..ddbc57321c 100644 --- a/services/spar/src/Spar/Error.hs +++ b/services/spar/src/Spar/Error.hs @@ -84,6 +84,7 @@ data SparCustomError | SparBindFromWrongOrNoTeam LT | SparBindFromBadAccountStatus LT | SparBindUserRefTaken + | SparUserRefInMultipleTeams LT | SparBadUserName LT | SparCannotCreateUsersOnReplacedIdP LT | SparCouldNotParseRfcResponse LT LT @@ -140,6 +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 (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 From 379c56c865ff36e3f5112882d5217bf26be17565 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 13 Sep 2021 23:04:42 +0200 Subject: [PATCH 52/52] ... --- services/spar/src/Spar/API.hs | 41 +++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 8752531137..9b5b6923de 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -353,21 +353,34 @@ 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 -- FUTUREWORK: some database lookups here are unnecessary, this could be made more - -- efficient rather easily. - handleIdPClash :: Either SAML.IdPId IdP -> m () - handleIdPClash (Right idp') = case apiversion of - WireIdPAPIV1 -> do + let handleIdPClash :: Either SAML.IdPId IdP -> m () + 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." - WireIdPAPIV2 -> do - when (fromMaybe defWireIdPAPIVersion (idp' ^. SAML.idpExtraInfo . wiApiVersion) == WireIdPAPIV1) $ do - throwSpar $ SparNewIdPAlreadyInUse "only allow all-new IdPs, no combination of old and new IdPs." - 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." - handleIdPClash (Left id') = do - let err = throwSpar . SparIdPNotFound . cs . show $ id' -- database inconsistency - idp' <- wrapMonadClient (Data.getIdPConfig id') >>= maybe err pure - handleIdPClash (Right idp') + 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)) case idp of Data.GetIdPFound idp' {- same team -} -> handleIdPClash (Right idp')