Skip to content
Merged
Show file tree
Hide file tree
Changes from 20 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
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
A new endpoint is added to Brig (`put /users/:uid/email`) that allows a team owner to change/set a user email. It can also be used to (re-) send the activation email.
Comment thread
battermann marked this conversation as resolved.
Outdated
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
24 changes: 24 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,27 @@ 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 <$> assertHasPerm 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

assertHasPerm :: Maybe TeamId -> Handler Bool
assertHasPerm maybeTeamId = fromMaybe False <$> check
where
check = runMaybeT $ do
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
12 changes: 12 additions & 0 deletions services/brig/test/integration/API/Team/Util.hs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import Util
import Web.Cookie (parseSetCookie, setCookieName)
import Wire.API.Team.Feature (TeamFeatureStatusValue (..))
import qualified Wire.API.Team.Feature as Public
import qualified Wire.API.User as Public

-- | FUTUREWORK: Remove 'createPopulatedBindingTeam', 'createPopulatedBindingTeamWithNames',
-- and rename 'createPopulatedBindingTeamWithNamesAndHandles' to 'createPopulatedBindingTeam'.
Expand Down Expand Up @@ -476,3 +477,14 @@ 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 $ Public.EmailUpdate email)
)
48 changes: 48 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,57 @@ tests _ at opts p b c ch g aws =
testGroup
"temporary customer extensions"
[ test' aws p "domains blocked for registration" $ testDomainsBlockedForRegistration opts b
],
testGroup
"update user email by team owner"
[ test' aws p "put /users/:uid/email" $ testUpdateUserEmailByTeamOwner b
]
]

testUpdateUserEmailByTeamOwner :: Brig -> Http ()
testUpdateUserEmailByTeamOwner 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
checkActivationCode newEmail False
checkSetUserEmail teamOwner emailOwner newEmail 200
checkActivationCode newEmail True
checkUnauthorizedRequests emailOwner otherTeamMember teamOwnerDifferentTeam newEmail
activateEmail brig newEmail
-- apparently activating the email does not invalidate the activation code
-- ideally we would let the activation code expire again (which is too expensive time-wise)
-- and then assert that if the email is verified there is no new activation code created
-- when the set email function is called again (idempotency)
Comment thread
battermann marked this conversation as resolved.
Outdated
checkSetUserEmail teamOwner emailOwner newEmail 200
Comment thread
battermann marked this conversation as resolved.
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.

where
checkLetActivationExpire :: Email -> Http ()
checkLetActivationExpire email = do
-- assumption: `optSettings.setActivationTimeout = 5` in `brig.yaml`
threadDelay (5100 * 1000)
checkActivationCode email False

checkActivationCode :: Email -> Bool -> Http ()
checkActivationCode email shouldExist = do
maybeActivationCode <- Util.getActivationCode brig (Left email)
void $
lift $
if shouldExist
then assertBool "activation code should exists" (isJust maybeActivationCode)
else assertBool "activation code should not exists" (isNothing maybeActivationCode)

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