diff --git a/changelog.d/3-bug-fixes/more-spar-tests b/changelog.d/3-bug-fixes/more-spar-tests new file mode 100644 index 0000000000..be3a1593b0 --- /dev/null +++ b/changelog.d/3-bug-fixes/more-spar-tests @@ -0,0 +1 @@ +Always create spar credentials during SCIM provisioning when applicable diff --git a/libs/wire-api/src/Wire/API/Team/Feature.hs b/libs/wire-api/src/Wire/API/Team/Feature.hs index 92aa3b062c..2c7b97a880 100644 --- a/libs/wire-api/src/Wire/API/Team/Feature.hs +++ b/libs/wire-api/src/Wire/API/Team/Feature.hs @@ -263,7 +263,7 @@ typeTeamFeatureName = Doc.string . Doc.enum $ cs . toByteString' <$> [(minBound data TeamFeatureStatusValue = TeamFeatureEnabled | TeamFeatureDisabled - deriving stock (Eq, Show, Generic) + deriving stock (Eq, Ord, Enum, Bounded, Show, Generic) deriving (Arbitrary) via (GenericUniform TeamFeatureStatusValue) deriving (ToJSON, FromJSON, S.ToSchema) via (Schema TeamFeatureStatusValue) diff --git a/services/spar/src/Spar/Scim/User.hs b/services/spar/src/Spar/Scim/User.hs index cf57fdd0e3..8ce22b5035 100644 --- a/services/spar/src/Spar/Scim/User.hs +++ b/services/spar/src/Spar/Scim/User.hs @@ -167,11 +167,7 @@ instance $ do mIdpConfig <- maybe (pure Nothing) (lift . IdPConfigStore.getConfig) stiIdP let notfound = Scim.notFound "User" (idToText uid) - brigUser <- lift (BrigAccess.getAccount Brig.WithPendingInvitations uid) >>= maybe (throwError notfound) pure - unless (userTeam (accountUser brigUser) == Just stiTeam) (throwError notfound) - case Brig.veidFromBrigUser (accountUser brigUser) ((^. SAML.idpMetadata . SAML.edIssuer) <$> mIdpConfig) of - Right veid -> synthesizeStoredUser brigUser veid - Left _ -> throwError notfound + runMaybeT (getUserById mIdpConfig stiTeam uid) >>= maybe (throwError notfound) pure postUser :: ScimTokenInfo -> @@ -439,6 +435,9 @@ createValidScimUser tokeninfo@ScimTokenInfo {stiTeam} vsu@(ST.ValidScimUser veid ST.runValidExternalId ( \uref -> do + -- FUTUREWORK: outsource this and some other fragments from + -- `createValidScimUser` into a function `createValidScimUserBrig` similar + -- to `createValidScimUserSpar`? uid <- Id <$> Random.uuid BrigAccess.createSAML uref uid stiTeam name ManagedByScim ) @@ -474,18 +473,13 @@ createValidScimUser tokeninfo@ScimTokenInfo {stiTeam} vsu@(ST.ValidScimUser veid lift $ Logger.debug ("createValidScimUser: spar says " <> show storedUser) -- {(arianvp): these two actions we probably want to make transactional.} - lift $ do - -- Store scim timestamps, saml credentials, scim externalId locally in spar. - ScimUserTimesStore.write storedUser - ST.runValidExternalId - (`SAMLUserStore.insert` buid) - (\email -> ScimExternalIdStore.insert stiTeam email buid) - veid + createValidScimUserSpar stiTeam buid storedUser veid -- If applicable, trigger email validation procedure on brig. lift $ ST.runValidExternalId (validateEmailIfExists buid) (\_ -> pure ()) veid - -- {suspension via scim: if we don't reach the following line, the user will be active.} + -- TODO: suspension via scim is brittle, and may leave active users behind: if we don't + -- reach the following line due to a crash, the user will be active. lift $ do old <- BrigAccess.getStatus buid let new = ST.scimActiveFlagToAccountStatus old (Scim.unScimBool <$> active) @@ -493,6 +487,28 @@ createValidScimUser tokeninfo@ScimTokenInfo {stiTeam} vsu@(ST.ValidScimUser veid when (new /= old) $ BrigAccess.setStatus buid new pure storedUser +-- | Store scim timestamps, saml credentials, scim externalId locally in spar. Table +-- `spar.scim_external` gets an entry iff there is no `UserRef`: if there is, we don't do a +-- lookup in that table either, but compute the `externalId` from the `UserRef`. +createValidScimUserSpar :: + forall m r. + ( (m ~ Scim.ScimHandler (Sem r)), + Member ScimExternalIdStore r, + Member ScimUserTimesStore r, + Member SAMLUserStore r + ) => + TeamId -> + UserId -> + Scim.StoredUser ST.SparTag -> + ST.ValidExternalId -> + m () +createValidScimUserSpar stiTeam uid storedUser veid = lift $ do + ScimUserTimesStore.write storedUser + ST.runValidExternalId + ((`SAMLUserStore.insert` uid)) + (\email -> ScimExternalIdStore.insert stiTeam email uid) + veid + -- TODO(arianvp): how do we get this safe w.r.t. race conditions / crashes? updateValidScimUser :: forall m r. @@ -872,15 +888,46 @@ synthesizeScimUser info = Scim.active = Just . Scim.ScimBool $ info ^. ST.vsuActive } +getUserById :: + forall r. + ( Member BrigAccess r, + Member (Input Opts) r, + Member (Logger (Msg -> Msg)) r, + Member Now r, + Member SAMLUserStore r, + Member ScimExternalIdStore r, + Member ScimUserTimesStore r + ) => + Maybe IdP -> + TeamId -> + UserId -> + MaybeT (Scim.ScimHandler (Sem r)) (Scim.StoredUser ST.SparTag) +getUserById midp stiTeam uid = do + brigUser <- MaybeT . lift $ BrigAccess.getAccount Brig.WithPendingInvitations uid + let mbveid = + Brig.veidFromBrigUser + (accountUser brigUser) + ((^. SAML.idpMetadata . SAML.edIssuer) <$> midp) + case mbveid of + Right veid | userTeam (accountUser brigUser) == Just stiTeam -> lift $ do + storedUser :: Scim.StoredUser ST.SparTag <- synthesizeStoredUser brigUser veid + -- if we get a user from brig that hasn't been touched by scim yet, we call this + -- function to move it under scim control. + assertExternalIdNotUsedElsewhere stiTeam veid uid + createValidScimUserSpar stiTeam uid storedUser veid + pure storedUser + _ -> Applicative.empty + scimFindUserByHandle :: - Members - '[ Input Opts, - Now, - Logger (Msg -> Msg), - BrigAccess, - ScimUserTimesStore - ] - r => + forall r. + ( Member BrigAccess r, + Member (Input Opts) r, + Member (Logger (Msg -> Msg)) r, + Member Now r, + Member SAMLUserStore r, + Member ScimExternalIdStore r, + Member ScimUserTimesStore r + ) => Maybe IdP -> TeamId -> Text -> @@ -888,10 +935,7 @@ scimFindUserByHandle :: scimFindUserByHandle mIdpConfig stiTeam hndl = do handle <- MaybeT . pure . parseHandle . Text.toLower $ hndl brigUser <- MaybeT . lift . BrigAccess.getByHandle $ handle - guard $ userTeam (accountUser brigUser) == Just stiTeam - case Brig.veidFromBrigUser (accountUser brigUser) ((^. SAML.idpMetadata . SAML.edIssuer) <$> mIdpConfig) of - Right veid -> lift $ synthesizeStoredUser brigUser veid - Left _ -> Applicative.empty + getUserById mIdpConfig stiTeam . userId . accountUser $ brigUser -- | Construct a 'ValidExternalid'. If it an 'Email', find the non-SAML SCIM user in spar; if -- that fails, find the user by email in brig. If it is a 'UserRef', find the SAML user. @@ -901,16 +945,14 @@ scimFindUserByHandle mIdpConfig stiTeam hndl = do -- successful authentication with their SAML credentials. scimFindUserByEmail :: forall r. - Members - '[ Input Opts, - Now, - Logger (Msg -> Msg), - BrigAccess, - ScimExternalIdStore, - ScimUserTimesStore, - SAMLUserStore - ] - r => + ( Member BrigAccess r, + Member (Input Opts) r, + Member (Logger (Msg -> Msg)) r, + Member Now r, + Member SAMLUserStore r, + Member ScimExternalIdStore r, + Member ScimUserTimesStore r + ) => Maybe IdP -> TeamId -> Text -> @@ -925,8 +967,7 @@ scimFindUserByEmail mIdpConfig stiTeam email = do veid <- MaybeT (either (const Nothing) Just <$> runExceptT (mkValidExternalId mIdpConfig (pure email))) uid <- MaybeT . lift $ ST.runValidExternalId withUref withEmailOnly veid brigUser <- MaybeT . lift . BrigAccess.getAccount Brig.WithPendingInvitations $ uid - guard $ userTeam (accountUser brigUser) == Just stiTeam - lift $ synthesizeStoredUser brigUser veid + getUserById mIdpConfig stiTeam . userId . accountUser $ brigUser where withUref :: SAML.UserRef -> Sem r (Maybe UserId) withUref uref = do diff --git a/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs b/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs index a2efd0acfd..3d323d9e5c 100644 --- a/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs +++ b/services/spar/test-integration/Test/Spar/Scim/UserSpec.hs @@ -30,7 +30,7 @@ where import Bilge import Bilge.Assert -import Brig.Types.Intra (AccountStatus (Active, PendingInvitation, Suspended), accountStatus, accountUser) +import Brig.Types.Intra (AccountStatus (Active, PendingInvitation, Suspended), UserAccount (..), accountStatus, accountUser) import Brig.Types.User as Brig import qualified Control.Exception import Control.Lens @@ -46,7 +46,7 @@ import Data.Aeson.Types (fromJSON, toJSON) import Data.ByteString.Conversion import qualified Data.CaseInsensitive as CI import qualified Data.Csv as Csv -import Data.Handle (Handle (Handle), fromHandle) +import Data.Handle (Handle (Handle), fromHandle, parseHandleEither) import Data.Id (TeamId, UserId, randomId) import Data.Ix (inRange) import Data.Misc (HttpsUrl, mkHttpsUrl) @@ -58,6 +58,8 @@ import Imports import qualified Network.Wai.Utilities.Error as Wai import qualified SAML2.WebSSO as SAML import qualified SAML2.WebSSO.Test.MockResponse as SAML +import SAML2.WebSSO.Test.Util.TestSP (makeSampleIdPMetadata) +import qualified SAML2.WebSSO.Test.Util.Types as SAML import qualified Spar.Intra.BrigApp as Intra import Spar.Scim import Spar.Scim.Types (normalizeLikeStored) @@ -78,6 +80,7 @@ import qualified Web.Scim.Schema.User as Scim.User import qualified Wire.API.Team.Export as CsvExport import qualified Wire.API.Team.Feature as Feature import Wire.API.Team.Invitation (Invitation (..)) +import Wire.API.User.Identity (emailToSAMLNameID) import Wire.API.User.IdentityProvider (IdP) import qualified Wire.API.User.IdentityProvider as User import Wire.API.User.RichInfo @@ -98,6 +101,8 @@ spec = do specAzureQuirks specEmailValidation specSuspend + specImportToScimFromSAML + specImportToScimFromInvitation specSCIMManaged describe "CRUD operations maintain invariants in mapScimToBrig, mapBrigToScim." $ do it "..." $ do @@ -106,6 +111,199 @@ spec = do it "works" $ do pendingWith "write a list of unit tests here that make the mapping explicit, exhaustive, and easy to read." +specImportToScimFromSAML :: SpecWith TestEnv +specImportToScimFromSAML = + describe "Create with SAML autoprovisioning; then re-provision with SCIM" $ do + forM_ ((,,) <$> [minBound ..] <*> [minBound ..] <*> [minBound ..]) $ \(x, y, z) -> check x y z + where + check :: Bool -> Bool -> Feature.TeamFeatureStatusValue -> SpecWith TestEnv + check sameHandle sameDisplayName valemail = it (show (sameHandle, sameDisplayName, valemail)) $ do + (_ownerid, teamid, idp, (_, privCreds)) <- registerTestIdPWithMeta + setSamlEmailValidation teamid valemail + + -- saml-auto-provision a new user + (usr :: Scim.User.User SparTag, email :: Email) <- do + (usr, email) <- randomScimUserWithEmail + pure + ( -- when auto-provisioning via saml, user display name is set to saml name id. + usr {Scim.User.displayName = Just $ fromEmail email}, + email + ) + + (uref :: SAML.UserRef, uid :: UserId) <- do + let uref = SAML.UserRef tenant subj + subj = emailToSAMLNameID email + tenant = idp ^. SAML.idpMetadata . SAML.edIssuer + !(Just !uid) <- createViaSaml idp privCreds uref + samlUserShouldSatisfy uref isJust + pure (uref, uid) + + let handle = fromRight undefined . parseHandleEither $ Scim.User.userName usr + runSpar (BrigAccess.setHandle uid handle) + + assertSparCassandraUref (uref, Just uid) + assertSparCassandraScim ((teamid, email), Nothing) + assertBrigCassandra uid uref usr (valemail, False) ManagedByWire + + -- activate email + case valemail of + Feature.TeamFeatureEnabled -> do + asks (view teBrig) >>= \brig -> call (activateEmail brig email) + assertBrigCassandra uid uref usr (valemail, True) ManagedByWire + Feature.TeamFeatureDisabled -> do + pure () + + -- now import to scim + tok :: ScimToken <- do + -- this can only happen now, since it turns off saml-autoprovisioning. + registerScimToken teamid (Just (idp ^. SAML.idpId)) + + storedUserGot :: Scim.UserC.StoredUser SparTag <- do + resp <- + aFewTimes (getUser_ (Just tok) uid =<< view teSpar) ((== 200) . statusCode) + u {Scim.User.userName = Scim.User.userName usr_}) + & (if sameDisplayName then id else \u -> u {Scim.User.displayName = Scim.User.displayName usr_}) + & pure + + storedUserUpdated :: Scim.UserC.StoredUser SparTag <- do + resp <- + aFewTimes (updateUser_ (Just tok) (Just uid) usr' =<< view teSpar) ((== 200) . statusCode) + TestSpar (UserId, TeamId) + createTeam = do + env <- ask + call $ createUserWithTeam (env ^. teBrig) (env ^. teGalley) + + invite :: HasCallStack => UserId -> TeamId -> TestSpar (UserId, Email) + invite owner teamid = do + env <- ask + memberInvited <- call (inviteAndRegisterUser (env ^. teBrig) owner teamid) + let memberIdInvited = userId memberInvited + emailInvited = maybe (error "must have email") id (userEmail memberInvited) + pure (memberIdInvited, emailInvited) + + addSamlIdP :: HasCallStack => UserId -> TestSpar (SAML.IdPConfig User.WireIdP, SAML.SignPrivCreds) + addSamlIdP userid = do + env <- ask + apiVersion <- view teWireIdPAPIVersion + SAML.SampleIdP idpmeta privkey _ _ <- makeSampleIdPMetadata + idp <- call $ callIdpCreate apiVersion (env ^. teSpar) (Just userid) idpmeta + pure (idp, privkey) + + reProvisionWithScim :: HasCallStack => Bool -> Maybe (SAML.IdPConfig User.WireIdP) -> TeamId -> UserId -> ReaderT TestEnv IO () + reProvisionWithScim changeHandle mbidp teamid userid = do + tok :: ScimToken <- do + registerScimToken teamid ((^. SAML.idpId) <$> mbidp) + + storedUserGot :: Scim.UserC.StoredUser SparTag <- do + resp <- + aFewTimes (getUser_ (Just tok) userid =<< view teSpar) ((== 200) . statusCode) + (SAML.IdPConfig User.WireIdP, SAML.SignPrivCreds) -> Email -> TestSpar () + signInWithSaml (idp, privCreds) email = do + let uref = SAML.UserRef tenant subj + subj = emailToSAMLNameID email + tenant = idp ^. SAML.idpMetadata . SAML.edIssuer + void $ createViaSaml idp privCreds uref + + check :: Bool -> SpecWith TestEnv + check changeHandle = it (show changeHandle) $ do + (ownerid, teamid) <- createTeam + (userid, email) <- invite ownerid teamid + idp <- addSamlIdP ownerid + reProvisionWithScim changeHandle (Just $ fst idp) teamid userid + signInWithSaml idp email + +assertSparCassandraUref :: HasCallStack => (SAML.UserRef, Maybe UserId) -> TestSpar () +assertSparCassandraUref (uref, urefAnswer) = do + liftIO . (`shouldBe` urefAnswer) + =<< runSpar (SAMLUserStore.get uref) + +assertSparCassandraScim :: HasCallStack => ((TeamId, Email), Maybe UserId) -> TestSpar () +assertSparCassandraScim ((teamid, email), scimAnswer) = do + liftIO . (`shouldBe` scimAnswer) + =<< runSpar (ScimExternalIdStore.lookup teamid email) + +assertBrigCassandra :: + HasCallStack => + UserId -> + SAML.UserRef -> + Scim.User.User SparTag -> + (Feature.TeamFeatureStatusValue, Bool) -> + ManagedBy -> + TestSpar () +assertBrigCassandra uid uref usr (valemail, emailValidated) managedBy = do + runSpar (BrigAccess.getAccount NoPendingInvitations uid) >>= \(Just acc) -> liftIO $ do + let handle = fromRight errmsg . parseHandleEither $ Scim.User.userName usr + where + errmsg = error . show . Scim.User.userName $ usr + + name = Name . fromMaybe (error "name") $ Scim.User.displayName usr + + email = case (valemail, emailValidated) of + (Feature.TeamFeatureEnabled, True) -> + Just . fromJust . parseEmail . fromJust . Scim.User.externalId $ usr + _ -> + Nothing + + accountStatus acc `shouldBe` Active + userId (accountUser acc) `shouldBe` uid + userHandle (accountUser acc) `shouldBe` Just handle + userDisplayName (accountUser acc) `shouldBe` name + userManagedBy (accountUser acc) `shouldBe` managedBy + + userIdentity (accountUser acc) + `shouldBe` Just (SSOIdentity (UserSSOId uref) email Nothing) + specSuspend :: SpecWith TestEnv specSuspend = do describe "suspend" $ do @@ -686,42 +884,42 @@ testScimCreateVsUserRef = do subj' = either (error . show) id $ SAML.mkNameID uname' Nothing Nothing Nothing tenant' = idp ^. SAML.idpMetadata . SAML.edIssuer createViaSamlFails idp privCreds uref' - where - samlUserShouldSatisfy :: HasCallStack => SAML.UserRef -> (Maybe UserId -> Bool) -> TestSpar () - samlUserShouldSatisfy uref property = do - muid <- getUserIdViaRef' uref - liftIO $ muid `shouldSatisfy` property - - createViaSamlResp :: HasCallStack => IdP -> SAML.SignPrivCreds -> SAML.UserRef -> TestSpar ResponseLBS - createViaSamlResp idp privCreds (SAML.UserRef _ subj) = do - authnReq <- negotiateAuthnRequest idp - let tid = idp ^. SAML.idpExtraInfo . User.wiTeam - spmeta <- getTestSPMetadata tid - authnResp <- - runSimpleSP $ - SAML.mkAuthnResponseWithSubj subj privCreds idp spmeta authnReq True - submitAuthnResponse tid authnResp IdP -> SAML.SignPrivCreds -> SAML.UserRef -> TestSpar () - createViaSamlFails idp privCreds uref = do - resp <- createViaSamlResp idp privCreds uref - liftIO $ do - maybe (error "no body") cs (responseBody resp) - `shouldNotContain` "