Skip to content
Merged
Show file tree
Hide file tree
Changes from 14 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: 2 additions & 0 deletions libs/galley-types/src/Galley/Types/Teams.hs
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ data HiddenPerm
-- efficient this end-point is. better not let all team members
-- play with it unless we have to.
DownloadTeamMembersCsv
| ChangeTeamMemberProfiles
deriving (Eq, Ord, Show)

-- | See Note [hidden team roles]
Expand Down Expand Up @@ -367,6 +368,7 @@ roleHiddenPermissions role = HiddenPermissions p p
ChangeTeamFeature TeamFeatureFileSharing,
ChangeTeamFeature TeamFeatureClassifiedDomains {- the features not listed here can only be changed in stern -},
ChangeTeamFeature TeamFeatureSelfDeletingMessages,
ChangeTeamMemberProfiles,
ReadIdp,
CreateUpdateDeleteIdp,
CreateReadDeleteScimToken,
Expand Down
10 changes: 10 additions & 0 deletions libs/wire-api/src/Wire/API/Routes/Public/Brig.hs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,16 @@ data Api routes = Api
:> "self"
:> ReqBody '[JSON] DeleteUser
:> MultiVerb 'DELETE '[JSON] DeleteSelfResponses (Maybe Timeout),
updateUserEmail ::
routes
:- Summary "Resend email address validation email."
:> Description "If the user has a pending email validation, the validation email will be resent."
:> ZUser
:> "users"
:> CaptureUserId "uid"
Comment thread
fisx marked this conversation as resolved.
:> "email"
:> ReqBody '[JSON] EmailUpdate
:> Put '[JSON] (),
getHandleInfoUnqualified ::
routes
:- Summary "(deprecated, use /search/contacts) Get information on a user handle"
Expand Down
7 changes: 7 additions & 0 deletions libs/wire-api/src/Wire/API/User.hs
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,13 @@ instance FromJSON LocaleUpdate where
newtype EmailUpdate = EmailUpdate {euEmail :: Email}
deriving stock (Eq, Show, Generic)
deriving newtype (Arbitrary)
deriving (S.ToSchema) via (Schema EmailUpdate)

instance ToSchema EmailUpdate where
schema =
object "EmailUpdate" $
EmailUpdate
<$> euEmail .= field "email" schema

modelEmailUpdate :: Doc.Model
modelEmailUpdate = Doc.defineModel "EmailUpdate" $ do
Expand Down
3 changes: 3 additions & 0 deletions services/brig/src/Brig/API/Error.hs
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,9 @@ newPasswordMustDiffer = Wai.mkError status409 "password-must-differ" "For provid
notFound :: LText -> Wai.Error
notFound = Wai.mkError status404 "not-found"

conflict :: LText -> Wai.Error
conflict = Wai.mkError status409 "conflict"

Comment thread
battermann marked this conversation as resolved.
Outdated
invalidAccountStatus :: Wai.Error
invalidAccountStatus = Wai.mkError status400 "invalid-status" "The specified account status cannot be set."

Expand Down
22 changes: 22 additions & 0 deletions services/brig/src/Brig/API/Public.hs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import Brig.App
import qualified Brig.Calling.API as Calling
import qualified Brig.Data.Connection as Data
import qualified Brig.Data.User as Data
import qualified Brig.IO.Intra as Intra
import Brig.Options hiding (internalEvents, sesQueue)
import qualified Brig.Provider.API as Provider
import qualified Brig.Team.API as Team
Expand Down Expand Up @@ -80,6 +81,7 @@ import qualified Data.Text.Ascii as Ascii
import Data.Text.Encoding (decodeLatin1)
import Data.Text.Lazy (pack)
import qualified Data.ZAuth.Token as ZAuth
import Galley.Types.Teams (HiddenPerm (..), hasPermission)
import Imports hiding (head)
import Network.HTTP.Types.Status
import Network.Wai (Response, lazyRequestBody)
Expand Down Expand Up @@ -251,6 +253,7 @@ servantSitemap =
BrigAPI.getUserQualified = getUser,
BrigAPI.getSelf = getSelf,
BrigAPI.deleteSelf = deleteUser,
BrigAPI.updateUserEmail = updateUserEmail,
BrigAPI.getHandleInfoUnqualified = getHandleInfoUnqualifiedH,
BrigAPI.getUserByHandleQualified = Handle.getHandleInfo,
BrigAPI.listUsersByUnqualifiedIdsOrHandles = listUsersByUnqualifiedIdsOrHandles,
Expand Down Expand Up @@ -1193,6 +1196,25 @@ verifyDeleteUserH (r ::: _) = do
API.verifyDeleteUser body !>> deleteUserError
return (setStatus status200 empty)

updateUserEmail :: UserId -> UserId -> Public.EmailUpdate -> Handler ()
updateUserEmail zuserId emailOwnerId (Public.EmailUpdate email) = do
maybeZuserTeamId <- lift $ Data.lookupUserTeam zuserId
Comment thread
battermann marked this conversation as resolved.
whenM (not . fromMaybe False <$> runMaybeT (checkPerm maybeZuserTeamId)) $ throwStd insufficientTeamPermissions
maybeEmailOwnerTeamId <- lift $ Data.lookupUserTeam emailOwnerId
checkSameTeam maybeZuserTeamId maybeEmailOwnerTeamId
void $ API.changeSelfEmail emailOwnerId email API.AllowSCIMUpdates
where
checkSameTeam :: Maybe TeamId -> Maybe TeamId -> Handler ()
checkSameTeam (Just zuserTeamId) maybeEmailOwnerTeamId =
when (Just zuserTeamId /= maybeEmailOwnerTeamId) $ throwStd $ notFound "user not found"
checkSameTeam Nothing _ = throwStd insufficientTeamPermissions

checkPerm :: Maybe TeamId -> MaybeT Handler Bool
checkPerm maybeTeamId = do
Comment thread
battermann marked this conversation as resolved.
Outdated
teamId <- hoistMaybe maybeTeamId
teamMember <- MaybeT $ lift $ Intra.getTeamMember zuserId teamId
pure $ teamMember `hasPermission` ChangeTeamMemberProfiles

-- activation

data ActivationRespWithStatus
Expand Down
5 changes: 2 additions & 3 deletions services/brig/src/Brig/API/User.hs
Original file line number Diff line number Diff line change
Expand Up @@ -1232,9 +1232,8 @@ getEmailForProfile profileOwner EmailVisibleIfOnTeam' =
then userEmail profileOwner
else Nothing
getEmailForProfile profileOwner (EmailVisibleIfOnSameTeam' (Just (viewerTeamId, viewerTeamMember))) =
if ( Just viewerTeamId == userTeam profileOwner
&& Team.hasPermission viewerTeamMember Team.ViewSameTeamEmails
)
if Just viewerTeamId == userTeam profileOwner
&& Team.hasPermission viewerTeamMember Team.ViewSameTeamEmails
then userEmail profileOwner
else Nothing
getEmailForProfile _ (EmailVisibleIfOnSameTeam' Nothing) = Nothing
Expand Down
4 changes: 2 additions & 2 deletions services/brig/src/Brig/Data/User.hs
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ lookupRichInfoMultiUsers users = do
-- successful login.
lookupUserTeam :: UserId -> AppIO (Maybe TeamId)
lookupUserTeam u =
join . fmap runIdentity
(runIdentity =<<)
<$> retry x1 (query1 teamSelect (params LocalQuorum (Identity u)))

lookupAuth :: (MonadClient m) => UserId -> m (Maybe (Maybe Password, AccountStatus))
Expand Down Expand Up @@ -471,7 +471,7 @@ lookupFeatureConferenceCalling uid = do
pure $ ApiFt.TeamFeatureStatusNoConfig <$> mStatusValue
where
select :: PrepQuery R (Identity UserId) (Identity (Maybe ApiFt.TeamFeatureStatusValue))
select = fromString $ "select feature_conference_calling from user where id = ?"
select = fromString "select feature_conference_calling from user where id = ?"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally, these changes would go into a separate PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I know, but this makes the barrier for improving the code on the fly much higher.


-------------------------------------------------------------------------------
-- Queries
Expand Down
16 changes: 16 additions & 0 deletions services/brig/test/integration/API/Team/Util.hs
Original file line number Diff line number Diff line change
Expand Up @@ -476,3 +476,19 @@ setTeamSearchVisibility galley tid typ =
)
!!! do
const 204 === statusCode

setUserEmail :: Brig -> UserId -> UserId -> Email -> Http ResponseLBS
setUserEmail brig from uid email = do
put
( brig
. paths ["users", toByteString' uid, "email"]
. zUser from
. zConn "conn"
. contentJson
. body
( RequestBodyLBS . encode $
object
[ "email" .= show email
]
Comment thread
battermann marked this conversation as resolved.
Outdated
)
)
50 changes: 50 additions & 0 deletions services/brig/test/integration/API/User/Account.hs
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,59 @@ tests _ at opts p b c ch g aws =
testGroup
"temporary customer extensions"
[ test' aws p "domains blocked for registration" $ testDomainsBlockedForRegistration opts b
],
testGroup
"re-send activation email"
[ test' aws p "put /users/:uid/email" $ testReSendActivationEmail b
]
]

testReSendActivationEmail :: Brig -> Http ()
Comment thread
battermann marked this conversation as resolved.
Outdated
testReSendActivationEmail brig = do
(_, teamOwner, emailOwner : otherTeamMember : _) <- createPopulatedBindingTeamWithNamesAndHandles brig 2
Comment thread
battermann marked this conversation as resolved.
(teamOwnerDifferentTeam, _) <- createUserWithTeam' brig
newEmail <- randomEmail
initiateEmailUpdateNoSend brig newEmail (userId emailOwner) !!! (const 202 === statusCode)
checkActivationCode newEmail True
checkLetActivationExpire newEmail
checkSetUserEmail teamOwner emailOwner newEmail 200
checkActivationCode newEmail True
checkLetActivationExpire newEmail
checkUnauthorizedRequests emailOwner otherTeamMember teamOwnerDifferentTeam newEmail
checkActivationCode newEmail False
checkSetUserEmail teamOwner emailOwner newEmail 200
checkActivationCode newEmail True
activateEmail brig newEmail
-- apparently activating an email does not invalidate the activation code
Comment thread
battermann marked this conversation as resolved.
Outdated
checkLetActivationExpire newEmail
checkActivationCode newEmail False
Comment thread
battermann marked this conversation as resolved.
Outdated
checkSetUserEmail teamOwner emailOwner newEmail 200
Comment thread
battermann marked this conversation as resolved.
checkActivationCode newEmail False
checkUnauthorizedRequests emailOwner otherTeamMember teamOwnerDifferentTeam newEmail

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we remove this line? ff not, can you add a comment helping me to find out what this is testing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Line 168: Ideally if the email has been verified the team owner can call the endpoint again and nothing will happen (no new activation code will be generated). In order to test this specifically we would have to wait until the activation code expires. This is too costly for the test, but at least we can test that the request still responds with status code 200 in this case.

Line: 169: Similar, if the request is made with insufficient permissions, the responses should be the same as before (regardless of the state of the activation)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe these are not the most useful tests, but they don't hurt.

@battermann battermann Dec 2, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As discussed in another conversation, I added the expiration of the token again, so this conversation is obsolete, I think.

checkActivationCode newEmail False
where
checkLetActivationExpire :: Email -> Http ()
checkLetActivationExpire email = do
-- assumption: email activation timeout is set to 5 sec, so we wait until it expires
Comment thread
battermann marked this conversation as resolved.
Outdated
threadDelay (5100 * 1000)
checkActivationCode email False

checkActivationCode :: Email -> Bool -> Http ()
checkActivationCode email shouldExist = do
maybeActivationCode <- Util.getActivationCode brig (Left email)
if shouldExist
then void $ lift $ assertBool "activation code should exists" (isJust maybeActivationCode)
else void $ lift $ assertBool "activation code should not exists" (isNothing maybeActivationCode)
Comment thread
battermann marked this conversation as resolved.
Outdated

checkSetUserEmail :: User -> User -> Email -> Int -> Http ()
checkSetUserEmail teamOwner emailOwner email expectedStatusCode =
setUserEmail brig (userId teamOwner) (userId emailOwner) email !!! (const expectedStatusCode === statusCode)

checkUnauthorizedRequests :: User -> User -> User -> Email -> Http ()
checkUnauthorizedRequests emailOwner otherTeamMember teamOwnerDifferentTeam email = do
setUserEmail brig (userId teamOwnerDifferentTeam) (userId emailOwner) email !!! (const 404 === statusCode)
setUserEmail brig (userId otherTeamMember) (userId emailOwner) email !!! (const 403 === statusCode)

testCreateUserWithPreverified :: Opt.Opts -> Brig -> AWS.Env -> Http ()
testCreateUserWithPreverified opts brig aws = do
-- Register (pre verified) user with phone
Expand Down