From 1987e0d93aa9612a490acf406abb1e185115a9ed Mon Sep 17 00:00:00 2001 From: Sandy Maguire Date: Thu, 13 Oct 2022 00:17:15 -0700 Subject: [PATCH] Refactor Scim.Scim.UserSpec tests to use composable pieces (#2762) * refactor: build UserSpec tests out of more composable pieces --- changelog.d/5-internal/cleanup-scim-tests | 1 + services/spar/test/Test/Spar/Scim/UserSpec.hs | 102 ++++++------------ 2 files changed, 35 insertions(+), 68 deletions(-) create mode 100644 changelog.d/5-internal/cleanup-scim-tests diff --git a/changelog.d/5-internal/cleanup-scim-tests b/changelog.d/5-internal/cleanup-scim-tests new file mode 100644 index 00000000000..66e5e76ab6d --- /dev/null +++ b/changelog.d/5-internal/cleanup-scim-tests @@ -0,0 +1 @@ +Refactor some internal Scim user tests diff --git a/services/spar/test/Test/Spar/Scim/UserSpec.hs b/services/spar/test/Test/Spar/Scim/UserSpec.hs index 93918199e5d..271712d6ed2 100644 --- a/services/spar/test/Test/Spar/Scim/UserSpec.hs +++ b/services/spar/test/Test/Spar/Scim/UserSpec.hs @@ -6,16 +6,15 @@ import Brig.Types.User import Control.Monad.Except (runExceptT) import Data.Handle (parseHandle) import Data.Id -import qualified Data.Json.Util import Imports import Polysemy import Polysemy.TinyLog import Spar.Scim.User (deleteScimUser) import Spar.Sem.BrigAccess import Spar.Sem.IdPConfigStore -import Spar.Sem.IdPConfigStore.Mem (TypedState, idPToMem) +import Spar.Sem.IdPConfigStore.Mem (idPToMem) import Spar.Sem.SAMLUserStore -import Spar.Sem.SAMLUserStore.Mem (UserRefOrd, samlUserStoreToMem) +import Spar.Sem.SAMLUserStore.Mem (samlUserStoreToMem) import qualified Spar.Sem.ScimExternalIdStore as ScimExternalIdStore import Spar.Sem.ScimExternalIdStore.Mem (scimExternalIdStoreToMem) import Spar.Sem.ScimUserTimesStore @@ -25,7 +24,6 @@ import Test.Hspec import Test.QuickCheck import Web.Scim.Schema.Error import Wire.API.User -import qualified Wire.API.User.Identity import Wire.API.User.Scim import Wire.Sem.Logger.TinyLog (discardTinyLogs) @@ -36,33 +34,33 @@ spec = describe "deleteScimUser" $ do acc <- someActiveUser tokenInfo r <- interpretWithBrigAccessMock - (mockBrigForActiveUser acc AccountDeleted) + (mockBrig (withActiveUser acc) AccountDeleted) (deleteUserAndAssertDeletionInSpar acc tokenInfo) - handlerResult r `shouldBe` Right () + r `shouldBe` Right () it "returns an error when the account was deleted before" $ do tokenInfo <- generate arbitrary acc <- someActiveUser tokenInfo r <- interpretWithBrigAccessMock - (mockBrigForActiveUser acc AccountAlreadyDeleted) + (mockBrig (withActiveUser acc) AccountAlreadyDeleted) (deleteUserAndAssertDeletionInSpar acc tokenInfo) - handlerResult r `shouldBe` Left (notFound "user" ((idToText . userId . accountUser) acc)) + r `shouldBe` Left (notFound "user" ((idToText . userId . accountUser) acc)) it "returns an error when there never was an account" $ do uid <- generate arbitrary tokenInfo <- generate arbitrary r <- interpretWithBrigAccessMock - mockBrigForNonExistendUser + (mockBrig (const Nothing) NoUser) (runExceptT $ deleteScimUser tokenInfo uid) - handlerResult r `shouldBe` Left (notFound "user" (idToText uid)) + r `shouldBe` Left (notFound "user" (idToText uid)) it "returns no error when there was a partially deleted account" $ do uid <- generate arbitrary tokenInfo <- generate arbitrary r <- interpretWithBrigAccessMock - mockBrigForPartiallyDeletedUser + (mockBrig (const Nothing) AccountDeleted) (runExceptT $ deleteScimUser tokenInfo uid) - handlerResult r `shouldBe` Right () + r `shouldBe` Right () deleteUserAndAssertDeletionInSpar :: forall (r :: EffectRow). @@ -99,77 +97,45 @@ type EffsWithoutBrigAccess = Final IO ] -type Effs = BrigAccess ': EffsWithoutBrigAccess - -type InterpreterState = - ( Map (Data.Id.TeamId, Wire.API.User.Identity.Email) Data.Id.UserId, - ( Map Data.Id.UserId (Data.Json.Util.UTCTimeMillis, Data.Json.Util.UTCTimeMillis), - ( Map UserRefOrd UserId, - (Spar.Sem.IdPConfigStore.Mem.TypedState, Either ScimError ()) - ) - ) - ) - -handlerResult :: InterpreterState -> Either ScimError () -handlerResult = snd . snd . snd . snd - interpretWithBrigAccessMock :: - ( Sem Effs (Either ScimError ()) -> - Sem EffsWithoutBrigAccess (Either ScimError ()) + ( Sem (BrigAccess ': EffsWithoutBrigAccess) a -> + Sem EffsWithoutBrigAccess a ) -> - Sem Effs (Either ScimError ()) -> - IO InterpreterState + Sem (BrigAccess ': EffsWithoutBrigAccess) a -> + IO a interpretWithBrigAccessMock mock = runFinal . embedToFinal @IO . discardTinyLogs - . scimExternalIdStoreToMem - . scimUserTimesStoreToMem - . samlUserStoreToMem - . idPToMem + . ignoringState scimExternalIdStoreToMem + . ignoringState scimUserTimesStoreToMem + . ignoringState samlUserStoreToMem + . ignoringState idPToMem . mock -mockBrigForNonExistendUser :: - forall (r :: EffectRow). - Members '[Embed IO] r => - Sem (BrigAccess ': r) (Either ScimError ()) -> - Sem r (Either ScimError ()) -mockBrigForNonExistendUser = interpret $ \case - (GetAccount WithPendingInvitations _) -> pure Nothing - (Spar.Sem.BrigAccess.DeleteUser _) -> pure NoUser - _ -> do - liftIO $ expectationFailure $ "Unexpected effect (call to brig)" - error "Throw error here to avoid implementation of all cases." - -mockBrigForPartiallyDeletedUser :: - forall (r :: EffectRow). - Members '[Embed IO] r => - Sem (BrigAccess ': r) (Either ScimError ()) -> - Sem r (Either ScimError ()) -mockBrigForPartiallyDeletedUser = interpret $ \case - (GetAccount WithPendingInvitations _) -> pure Nothing - (Spar.Sem.BrigAccess.DeleteUser _) -> pure AccountDeleted - _ -> do - liftIO $ expectationFailure $ "Unexpected effect (call to brig)" - error "Throw error here to avoid implementation of all cases." +ignoringState :: Functor f => (a -> f (c, b)) -> a -> f b +ignoringState f = fmap snd . f -mockBrigForActiveUser :: - forall (r :: EffectRow). +mockBrig :: + forall (r :: EffectRow) a. Members '[Embed IO] r => - UserAccount -> + (UserId -> Maybe UserAccount) -> DeleteUserResult -> - Sem (BrigAccess ': r) (Either ScimError ()) -> - Sem r (Either ScimError ()) -mockBrigForActiveUser acc deletionResult = interpret $ \case - (GetAccount WithPendingInvitations uid) -> - if uid == (userId . accountUser) acc - then pure $ Just acc - else pure Nothing - (Spar.Sem.BrigAccess.DeleteUser _) -> pure deletionResult + Sem (BrigAccess ': r) a -> + Sem r a +mockBrig lookup_user delete_response = interpret $ \case + (GetAccount WithPendingInvitations uid) -> pure $ lookup_user uid + (Spar.Sem.BrigAccess.DeleteUser _) -> pure delete_response _ -> do liftIO $ expectationFailure $ "Unexpected effect (call to brig)" error "Throw error here to avoid implementation of all cases." +withActiveUser :: UserAccount -> UserId -> Maybe UserAccount +withActiveUser acc uid = + if uid == (userId . accountUser) acc + then Just acc + else Nothing + someActiveUser :: ScimTokenInfo -> IO UserAccount someActiveUser tokenInfo = do user <- generate arbitrary