Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion changelog.d/5-internal/WPB-8881
Original file line number Diff line number Diff line change
@@ -1 +1 @@
Move email update and remove operations to effects
Move email update and remove operations to effects (#4316, ##)
1 change: 1 addition & 0 deletions libs/wire-api/src/Wire/API/User.hs
Original file line number Diff line number Diff line change
Expand Up @@ -1580,6 +1580,7 @@ instance ToSchema NameUpdate where
data ChangeEmailResponse
= ChangeEmailResponseIdempotent
| ChangeEmailResponseNeedsActivation
deriving (Eq, Show)

instance
AsUnion
Expand Down
5 changes: 5 additions & 0 deletions libs/wire-api/src/Wire/API/User/Identity.hs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
module Wire.API.User.Identity
( -- * UserIdentity
UserIdentity (..),
isUserSSOId,
isSSOIdentity,
newIdentity,
emailIdentity,
Expand Down Expand Up @@ -150,6 +151,10 @@ data UserSSOId
deriving stock (Eq, Show, Generic)
deriving (Arbitrary) via (GenericUniform UserSSOId)

isUserSSOId :: UserSSOId -> Bool
isUserSSOId (UserSSOId _) = True
isUserSSOId (UserScimExternalId _) = False

instance C.Cql UserSSOId where
ctype = C.Tagged C.TextColumn

Expand Down
4 changes: 3 additions & 1 deletion libs/wire-subsystems/src/Wire/ActivationCodeStore.hs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ import Wire.API.User.Activation
import Wire.UserKeyStore

data ActivationCodeStore :: Effect where
LookupActivationCode :: EmailKey -> ActivationCodeStore m (Maybe (Maybe UserId, ActivationCode))
LookupActivationCode ::
EmailKey ->
ActivationCodeStore m (Maybe (Maybe UserId, ActivationCode))
-- | Create a code for a new pending activation for a given 'EmailKey'
NewActivationCode ::
EmailKey ->
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
module Wire.ActivationCodeStore.InterpreterSpec (spec) where

import Data.Default
import Data.Map qualified as Map
import Imports
import Test.Hspec
import Test.Hspec.QuickCheck
import Test.QuickCheck
import Wire.API.User.Activation
import Wire.ActivationCodeStore
import Wire.MiniBackend
import Wire.MockInterpreters.ActivationCodeStore

spec :: Spec
spec = do
describe "ActivationCodeStore effect" $ do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered to add a test where the lookup fails? E.g. there is a code, but the key does not match?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added it! I've rebased and pushed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks! (The more test code coverage we can add, the better - as long as the task doesn't grow out of bounds, of course 😄 )

prop "a code can be looked up" $ \emailKey config ->
let c = code emailKey
localBackend =
def {activationCodes = Map.singleton emailKey (Nothing, c)}
result =
runNoFederationStack localBackend Nothing config $
lookupActivationCode emailKey
in result === Just (Nothing, c)
prop "a code not found in the store" $ \emailKey config ->
let localBackend = def
result =
runNoFederationStack localBackend Nothing config $
lookupActivationCode emailKey
in result === Nothing
prop "newly added code can be looked up" $ \emailKey mUid config ->
let c = code emailKey
localBackend = def
(actCode, lookupRes) =
runNoFederationStack localBackend Nothing config $ do
ac <-
(.activationCode)
<$> newActivationCode emailKey undefined mUid
(ac,) <$> lookupActivationCode emailKey
in actCode === c .&&. lookupRes === Just (mUid, c)
18 changes: 18 additions & 0 deletions libs/wire-subsystems/test/unit/Wire/MiniBackend.hs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ module Wire.MiniBackend
NotPendingStoredUser (..),
NotPendingEmptyIdentityStoredUser (..),
PendingNotEmptyIdentityStoredUser (..),
NotPendingSSOIdWithEmailStoredUser (..),
PendingStoredUser (..),
)
where
Expand Down Expand Up @@ -126,6 +127,23 @@ instance Arbitrary NotPendingStoredUser where
notPendingStatus <- elements (Nothing : map Just [Active, Suspended, Ephemeral])
pure $ NotPendingStoredUser (user {status = notPendingStatus})

newtype NotPendingSSOIdWithEmailStoredUser = NotPendingSSOIdWithEmailStoredUser StoredUser
deriving (Show, Eq)

instance Arbitrary NotPendingSSOIdWithEmailStoredUser where
arbitrary = do
user <- arbitrary `suchThat` \user -> fmap isUserSSOId user.ssoId == Just True
notPendingStatus <- elements (Nothing : map Just [Active, Suspended, Ephemeral])
e <- arbitrary
pure $
NotPendingSSOIdWithEmailStoredUser
( user
{ activated = True,
status = notPendingStatus,
email = Just e
}
)

type AllErrors =
[ Error UserSubsystemError,
Error FederationError,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,15 @@ import Wire.API.User.Activation
import Wire.ActivationCodeStore (ActivationCodeStore (..))
import Wire.UserKeyStore

code :: EmailKey -> ActivationCode
code =
ActivationCode
. Ascii.unsafeFromText
. pack
. printf "%06d"
. length
. show

inMemoryActivationCodeStoreInterpreter ::
( Member (State (Map EmailKey (Maybe UserId, ActivationCode))) r
) =>
Expand All @@ -26,12 +35,5 @@ inMemoryActivationCodeStoreInterpreter = interpret \case
. T.encodeUtf8
. emailKeyUniq
$ ek
code =
ActivationCode
. Ascii.unsafeFromText
. pack
. printf "%06d"
. length
. show
$ ek
modify (insert ek (uid, code)) $> Activation key code
c = code ek
modify (insert ek (uid, c)) $> Activation key c
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ inMemoryUserStoreInterpreter = interpret $ \case
GetActivityTimestamps _ -> pure []
GetRichInfo _ -> error "rich info not implemented"
GetUserAuthenticationInfo _uid -> error "Not implemented"
DeleteEmail _uid -> error "Not implemented"
DeleteEmail uid -> modify (map doUpdate)
where
doUpdate :: StoredUser -> StoredUser
doUpdate u = if u.id == uid then u {email = Nothing} else u

storedUserToIndexUser :: StoredUser -> IndexUser
storedUserToIndexUser storedUser =
Expand Down
22 changes: 22 additions & 0 deletions libs/wire-subsystems/test/unit/Wire/UserStoreSpec.hs
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
module Wire.UserStoreSpec (spec) where

import Data.Default
import Imports
import Polysemy.State
import Test.Hspec
import Test.Hspec.QuickCheck
import Test.QuickCheck
import Wire.API.User
import Wire.MiniBackend
import Wire.StoredUser
import Wire.UserStore

spec :: Spec
spec = do
Expand Down Expand Up @@ -33,3 +37,21 @@ spec = do
in if (isJust storedUser.language)
then user.userLocale === Locale (fromJust storedUser.language) storedUser.country
else user.userLocale === defaultLocale

describe "UserStore effect" $ do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it wouldn't be useful to also test what happens when the user does not exist. Or, can't this case not happen? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that can happen. I'd argue it's out of scope for this ticket. Lots of stuff hasn't been tested, and I've added tests for the actions that I introduced in that original PR. Looking up a user in the storage is not an action that was in that PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Then, I'm approving. 👍

prop "user self email deleted" $ \user1 user2' email2 config ->
let user2 = user2' {email = Just email2}
localBackend = def {users = [user1, user2]}
result =
runNoFederationStack localBackend Nothing config $ do
deleteEmail (user1.id)
gets users
in result === [user1 {email = Nothing}, user2]
prop "update unvalidated email" $ \user1 user2 email1 config ->
let updatedUser1 = user1 {emailUnvalidated = Just email1}
localBackend = def {users = [user1, user2]}
result =
runNoFederationStack localBackend Nothing config $ do
updateEmailUnvalidated (user1.id) email1
gets users
in result === [updatedUser1, user2]
Original file line number Diff line number Diff line change
Expand Up @@ -814,3 +814,67 @@ spec = describe "UserSubsystem.Interpreter" do
. interpretNoFederationStack localBackend Nothing def config
$ getLocalUserAccountByUserKey (toLocalUnsafe localDomain userKey)
in retrievedUser === Nothing
describe "Removing an email address" do
prop "Cannot remove an email of a non-existing user" $ \lusr config ->
let localBackend = def
result =
runNoFederationStack localBackend Nothing config $
removeEmailEither lusr
in result === Left UserSubsystemProfileNotFound
prop "Cannot remove an email of a no-identity user" $
\(locx :: Local ()) (NotPendingEmptyIdentityStoredUser user) config ->
let localBackend = def {users = [user]}
lusr = qualifyAs locx user.id
result =
runNoFederationStack localBackend Nothing config $
removeEmailEither lusr
in result === Left UserSubsystemNoIdentity
prop "Cannot remove an email of a last-identity user" $
\(locx :: Local ()) user' email sso config ->
let user =
user'
{ activated = True,
email = email,
ssoId = if isNothing email then Just sso else Nothing
}
localBackend = def {users = [user]}
lusr = qualifyAs locx user.id
result =
runNoFederationStack localBackend Nothing config $
removeEmailEither lusr
in result === Left UserSubsystemLastIdentity
prop "Successfully remove an email from an SSOId user" $
\(locx :: Local ()) (NotPendingSSOIdWithEmailStoredUser user) config ->
let localBackend = def {users = [user]}
lusr = qualifyAs locx user.id
result =
runNoFederationStack localBackend Nothing config $ do
remRes <- removeEmailEither lusr
(remRes,) <$> gets users
in result === (Right (), [user {email = Nothing}])
describe "Changing an email address" $ do
prop "Idempotent email change" $
\(locx :: Local ()) (NotPendingStoredUser user') email config ->
let user = user' {email = Just email}
localBackend = def {users = [user]}
lusr = qualifyAs locx user.id
result =
runNoFederationStack localBackend Nothing config $ do
c <- requestEmailChange lusr email UpdateOriginWireClient
(c,) <$> gets users
in result === (ChangeEmailResponseIdempotent, [user])
prop "Email change needing activation" $
\(locx :: Local ()) (NotPendingStoredUser user') config ->
let email = unsafeEmailAddress "me" "example.com"
updatedEmail = unsafeEmailAddress "you" "example.com"
user = user' {email = Just email, managedBy = Nothing}
localBackend = def {users = [user]}
lusr = qualifyAs locx user.id
result =
runNoFederationStack localBackend Nothing config $ do
c <- requestEmailChange lusr updatedEmail UpdateOriginWireClient
(c,) <$> gets users
in result
=== ( ChangeEmailResponseNeedsActivation,
[user {emailUnvalidated = Just updatedEmail}]
)
1 change: 1 addition & 0 deletions libs/wire-subsystems/wire-subsystems.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ test-suite wire-subsystems-tests
-- cabal-fmt: expand test/unit
other-modules:
Spec
Wire.ActivationCodeStore.InterpreterSpec
Wire.AuthenticationSubsystem.InterpreterSpec
Wire.MiniBackend
Wire.MockInterpreters
Expand Down