Skip to content

Commit c715642

Browse files
fisxfisx
committed
Block changes to some user data in mlsE2EId teams (WPB-6189)
- Integration tests - block changes in the backend. - lie about managed_by in `GET /self`, but only there.
1 parent 4cedeeb commit c715642

File tree

10 files changed

+242
-21
lines changed

10 files changed

+242
-21
lines changed

‎changelog.d/1-api-changes/WPB-6189‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Block changes of userDisplayName, userHandle in mlsE2EI-enabled teams on the backend without SCIM; report `"managed_by" == "scim"` in `GET /self`, but only there

‎integration/integration.cabal‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ library
9999
API.Gundeck
100100
API.GundeckInternal
101101
API.Nginz
102+
API.Spar
102103
MLS.Util
103104
Notifications
104105
RunAllTests

‎integration/test/API/Brig.hs‎

Lines changed: 58 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ addUser dom opts = do
109109
"password" .= fromMaybe defPassword opts.password
110110
]
111111

112+
-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/get_users__uid_domain___uid_
112113
getUser ::
113114
(HasCallStack, MakesValue user, MakesValue target) =>
114115
user ->
@@ -147,12 +148,6 @@ deleteUser user = do
147148
submit "DELETE" $
148149
req & addJSONObject ["password" .= defPassword]
149150

150-
putHandle :: (HasCallStack, MakesValue user) => user -> String -> App Response
151-
putHandle user handle = do
152-
req <- baseRequest user Brig Versioned "/self/handle"
153-
submit "PUT" $
154-
req & addJSONObject ["handle" .= handle]
155-
156151
data AddClient = AddClient
157152
{ ctype :: String,
158153
internal :: Bool,
@@ -398,12 +393,67 @@ replaceKeyPackages cid suites kps = do
398393
& addQueryParams [("ciphersuites", intercalate "," (map (.code) suites))]
399394
& addJSONObject ["key_packages" .= map (T.decodeUtf8 . Base64.encode) kps]
400395

401-
getSelf :: HasCallStack => String -> String -> App Response
402-
getSelf domain uid = do
396+
-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/get_self
397+
getSelf :: (HasCallStack, MakesValue caller) => caller -> App Response
398+
getSelf caller = do
399+
req <- baseRequest caller Brig Versioned "/self"
400+
submit "GET" req
401+
402+
-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/get_self
403+
-- this is a low-level version of `getSelf` for testing some error conditions.
404+
getSelf' :: HasCallStack => String -> String -> App Response
405+
getSelf' domain uid = do
403406
let user = object ["domain" .= domain, "id" .= uid]
404407
req <- baseRequest user Brig Versioned "/self"
405408
submit "GET" req
406409

410+
data PutSelf = PutSelf
411+
{ accent :: Maybe Int,
412+
assets :: Maybe [Value], -- [{"key":"string", "size":"string", "type":"string"}]
413+
name :: Maybe String,
414+
picture :: Maybe [String]
415+
}
416+
417+
instance Default PutSelf where
418+
def = PutSelf Nothing Nothing Nothing Nothing
419+
420+
-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/put_self
421+
putSelf :: (HasCallStack, MakesValue caller) => caller -> PutSelf -> App Response
422+
putSelf caller body = do
423+
req <- baseRequest caller Brig Versioned "/self"
424+
submit "PUT" $
425+
req
426+
& addJSONObject
427+
[ "accent_id" .= body.accent,
428+
"assets" .= body.assets,
429+
"name" .= body.name,
430+
"picture" .= body.picture
431+
]
432+
433+
-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/put_self_locale
434+
putSelfLocale :: (HasCallStack, MakesValue caller) => caller -> String -> App Response
435+
putSelfLocale caller locale = do
436+
req <- baseRequest caller Brig Versioned "/self/locale"
437+
submit "PUT" $ req & addJSONObject ["locale" .= locale]
438+
439+
-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/put_users__uid__email
440+
--
441+
-- NOTE: the full process of changing (and confirming) the email address is more complicated.
442+
-- see /services/brig/test/integration for details.
443+
putSelfEmail :: (HasCallStack, MakesValue caller) => caller -> String -> App Response
444+
putSelfEmail caller emailAddress = do
445+
callerid <- asString $ caller %. "id"
446+
req <- baseRequest caller Brig Versioned $ joinHttpPath ["users", callerid, "email"]
447+
submit "PUT" $ req & addJSONObject ["email" .= emailAddress]
448+
449+
-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/put_self_handle
450+
-- FUTUREWORK: rename to putSelfHandle for consistency
451+
putHandle :: (HasCallStack, MakesValue user) => user -> String -> App Response
452+
putHandle user handle = do
453+
req <- baseRequest user Brig Versioned "/self/handle"
454+
submit "PUT" $
455+
req & addJSONObject ["handle" .= handle]
456+
407457
getUserSupportedProtocols ::
408458
(HasCallStack, MakesValue user, MakesValue target) =>
409459
user ->

‎integration/test/API/BrigInternal.hs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,12 @@ createUser domain cu = do
5252
]
5353
)
5454

55+
-- | https://staging-nginz-https.zinfra.io/api-internal/swagger-ui/brig/#/brig/get_i_users
56+
getUsersId :: (HasCallStack, MakesValue domain) => domain -> [String] -> App Response
57+
getUsersId domain ids = do
58+
req <- baseRequest domain Brig Unversioned "/i/users"
59+
submit "GET" $ req & addQueryParams [("ids", intercalate "," ids)]
60+
5561
data FedConn = FedConn
5662
{ domain :: String,
5763
searchStrategy :: String,

‎integration/test/API/Spar.hs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
module API.Spar where
2+
3+
import GHC.Stack
4+
import Testlib.Prelude
5+
6+
-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/get_scim_auth_tokens
7+
getScimTokens :: (HasCallStack, MakesValue caller) => caller -> App Response
8+
getScimTokens caller = do
9+
req <- baseRequest caller Spar Versioned "/scim/auth-tokens"
10+
submit "GET" req

‎integration/test/Test/Demo.hs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ testDynamicBackend = do
105105
ownDomain <- objDomain OwnDomain
106106
user <- randomUser OwnDomain def
107107
uid <- objId user
108-
bindResponse (BrigP.getSelf ownDomain uid) $ \resp -> do
108+
bindResponse (BrigP.getSelf' ownDomain uid) $ \resp -> do
109109
resp.status `shouldMatchInt` 200
110110
(resp.json %. "id") `shouldMatch` objId user
111111

@@ -117,18 +117,18 @@ testDynamicBackend = do
117117
resp.json %. "setRestrictUserCreation" `shouldMatch` False
118118

119119
-- user created in own domain should not be found in dynamic backend
120-
bindResponse (BrigP.getSelf dynDomain uid) $ \resp -> do
120+
bindResponse (BrigP.getSelf' dynDomain uid) $ \resp -> do
121121
resp.status `shouldMatchInt` 404
122122

123123
-- now create a user in the dynamic backend
124124
userD1 <- randomUser dynDomain def
125125
uidD1 <- objId userD1
126-
bindResponse (BrigP.getSelf dynDomain uidD1) $ \resp -> do
126+
bindResponse (BrigP.getSelf' dynDomain uidD1) $ \resp -> do
127127
resp.status `shouldMatchInt` 200
128128
(resp.json %. "id") `shouldMatch` objId userD1
129129

130130
-- the d1 user should not be found in the own domain
131-
bindResponse (BrigP.getSelf ownDomain uidD1) $ \resp -> do
131+
bindResponse (BrigP.getSelf' ownDomain uidD1) $ \resp -> do
132132
resp.status `shouldMatchInt` 404
133133

134134
testStartMultipleDynamicBackends :: HasCallStack => App ()

‎integration/test/Test/User.hs‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
1+
{-# OPTIONS_GHC -Wno-ambiguous-fields #-}
2+
13
module Test.User where
24

35
import API.Brig
46
import API.BrigInternal
7+
import API.GalleyInternal
8+
import API.Spar
9+
import qualified Data.UUID as UUID
10+
import qualified Data.UUID.V4 as UUID
511
import SetupHelpers
612
import Testlib.Prelude
713

@@ -47,3 +53,120 @@ testCreateUserSupportedProtocols = do
4753
bindResponse (createUser OwnDomain def {supportedProtocols = Just ["proteus", "mixed"]}) $ \resp -> do
4854
resp.status `shouldMatchInt` 400
4955
resp.json %. "label" `shouldMatch` "bad-request"
56+
57+
-- | For now this only tests attempts to update /self/handle in E2EId-enabled teams. More
58+
-- tests can be found under `/services/brig/test/integration` (and should be moved here).
59+
testUpdateHandle :: HasCallStack => App ()
60+
testUpdateHandle = do
61+
-- create team with one member, without scim, but with `mlsE2EId` enabled.
62+
(owner, team, [mem1]) <- createTeam OwnDomain 2
63+
mem1id <- asString $ mem1 %. "id"
64+
65+
let featureName = "mlsE2EId"
66+
bindResponse (getTeamFeature owner featureName team) $ \resp -> do
67+
resp.status `shouldMatchInt` 200
68+
resp.json %. "status" `shouldMatch` "disabled"
69+
setTeamFeatureStatus owner team featureName "enabled"
70+
bindResponse (getTeamFeature owner featureName team) $ \resp -> do
71+
resp.status `shouldMatchInt` 200
72+
resp.json %. "status" `shouldMatch` "enabled"
73+
74+
-- all as expected here. (see the second time we check this at the end of the test for an
75+
-- explanation why we care.)
76+
bindResponse (getSelf mem1) $ \resp -> do
77+
resp.status `shouldMatchInt` 200
78+
resp.json %. "managed_by" `shouldMatch` "wire"
79+
bindResponse (getUsersId owner [mem1id]) $ \resp -> do
80+
resp.status `shouldMatchInt` 200
81+
mb <- (assertOne =<< asList resp.json) %. "managed_by"
82+
mb `shouldMatch` "wire"
83+
84+
-- mem1 attempts to update handle for the first time => success
85+
--
86+
-- this is desired, because without SCIM users need to pick their own handles initially.
87+
-- moreover it is fine, because if `handle == NULL`, no mls E2Eid client certs can be
88+
-- created.
89+
handle <- UUID.toString <$> liftIO UUID.nextRandom
90+
bindResponse (putHandle mem1 handle) $ \resp -> do
91+
resp.status `shouldMatchInt` 200
92+
bindResponse (putHandle mem1 handle) $ \resp -> do
93+
-- idempotency
94+
resp.status `shouldMatchInt` 200
95+
96+
-- mem1 attempts to update handle again => failure
97+
handle2 <- UUID.toString <$> liftIO UUID.nextRandom
98+
bindResponse (putHandle mem1 handle2) $ \resp -> do
99+
resp.status `shouldMatchInt` 403
100+
resp.json %. "label" `shouldMatch` "managed-by-scim"
101+
102+
-- now self thinks it is managed by "scim", so clients can block change attempts to handle,
103+
-- display name without adding E2EId-specific logic. this is just a hack, though: `GET
104+
-- /self` is the only place where this is happening, other end-points still report the truth
105+
-- that is still stored correctly in the DB.
106+
--
107+
-- details: https://wearezeta.atlassian.net/browse/WPB-6189.
108+
-- FUTUREWORK: figure out a better way for clients to detect E2EId (V6?)
109+
bindResponse (getSelf mem1) $ \resp -> do
110+
resp.status `shouldMatchInt` 200
111+
resp.json %. "managed_by" `shouldMatch` "scim"
112+
bindResponse (getUsersId owner [mem1id]) $ \resp -> do
113+
resp.status `shouldMatchInt` 200
114+
mb <- (assertOne =<< asList resp.json) %. "managed_by"
115+
mb `shouldMatch` "wire"
116+
bindResponse (getScimTokens owner) $ \resp -> do
117+
resp.status `shouldMatchInt` 200
118+
resp.json %. "tokens" `shouldMatch` ([] @String)
119+
120+
-- | For now this only tests attempts to update one's own display name, email address, or
121+
-- language in E2EId-enabled teams (ie., everything except handle). More tests can be found
122+
-- under `/services/brig/test/integration` (and should be moved here).
123+
testUpdateSelf :: HasCallStack => TestUpdateSelfMode -> App ()
124+
testUpdateSelf mode = do
125+
-- create team with one member, without scim, but with `mlsE2EId` enabled.
126+
(owner, team, [mem1]) <- createTeam OwnDomain 2
127+
128+
let featureName = "mlsE2EId"
129+
bindResponse (getTeamFeature owner featureName team) $ \resp -> do
130+
resp.status `shouldMatchInt` 200
131+
resp.json %. "status" `shouldMatch` "disabled"
132+
setTeamFeatureStatus owner team featureName "enabled"
133+
bindResponse (getTeamFeature owner featureName team) $ \resp -> do
134+
resp.status `shouldMatchInt` 200
135+
resp.json %. "status" `shouldMatch` "enabled"
136+
137+
case mode of
138+
TestUpdateDisplayName -> do
139+
-- blocked unconditionally
140+
someDisplayName <- UUID.toString <$> liftIO UUID.nextRandom
141+
before <- getSelf mem1
142+
bindResponse (putSelf mem1 def {name = Just someDisplayName}) $ \resp -> do
143+
resp.status `shouldMatchInt` 403
144+
resp.json %. "label" `shouldMatch` "managed-by-scim"
145+
after <- getSelf mem1
146+
void $ (before.json %. "name") `shouldMatch` (after.json %. "name")
147+
TestUpdateEmailAddress -> do
148+
-- allowed unconditionally *for owner* (this is a bit off-topic: team members can't
149+
-- change their email addresses themselves under any conditions)
150+
someEmail <- (<> "@example.com") . UUID.toString <$> liftIO UUID.nextRandom
151+
bindResponse (putSelfEmail owner someEmail) $ \resp -> do
152+
resp.status `shouldMatchInt` 200
153+
TestUpdateLocale -> do
154+
-- scim maps "User.preferredLanguage" to brig's locale field. allowed unconditionally.
155+
-- we try two languages to make sure it doesn't work because it's already the active
156+
-- locale.
157+
forM_ ["uk", "he"] $ \someLocale ->
158+
bindResponse (putSelfLocale mem1 someLocale) $ \resp -> do
159+
resp.status `shouldMatchInt` 200
160+
161+
data TestUpdateSelfMode
162+
= TestUpdateDisplayName
163+
| TestUpdateEmailAddress
164+
| TestUpdateLocale
165+
deriving (Eq, Show, Bounded, Enum)
166+
167+
instance HasTests x => HasTests (TestUpdateSelfMode -> x) where
168+
mkTests m n s f x =
169+
mconcat
170+
[ mkTests m (n <> "[mode=" <> show mode <> "]") s f (x mode)
171+
| mode <- [minBound ..]
172+
]

‎services/brig/src/Brig/API/Internal.hs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -680,13 +680,13 @@ getRichInfoMultiH :: Maybe (CommaSeparatedList UserId) -> (Handler r) [(UserId,
680680
getRichInfoMultiH (maybe [] fromCommaSeparatedList -> uids) =
681681
lift $ wrapClient $ API.lookupRichInfoMultiUsers uids
682682

683-
updateHandleH :: UserId -> HandleUpdate -> (Handler r) NoContent
683+
updateHandleH :: Member GalleyProvider r => UserId -> HandleUpdate -> (Handler r) NoContent
684684
updateHandleH uid (HandleUpdate handleUpd) =
685685
NoContent <$ do
686686
handle <- validateHandle handleUpd
687687
API.changeHandle uid Nothing handle API.AllowSCIMUpdates !>> changeHandleError
688688

689-
updateUserNameH :: UserId -> NameUpdate -> (Handler r) NoContent
689+
updateUserNameH :: Member GalleyProvider r => UserId -> NameUpdate -> (Handler r) NoContent
690690
updateUserNameH uid (NameUpdate nameUpd) =
691691
NoContent <$ do
692692
name <- either (const $ throwStd (errorToWai @'E.InvalidUser)) pure $ mkName nameUpd

‎services/brig/src/Brig/API/Public.hs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -746,10 +746,11 @@ createUser (Public.NewUserPublic new) = lift . runExceptT $ do
746746
Public.NewTeamMemberSSO _ ->
747747
Team.sendMemberWelcomeMail e t n l
748748

749-
getSelf :: UserId -> (Handler r) Public.SelfProfile
749+
getSelf :: Member GalleyProvider r => UserId -> (Handler r) Public.SelfProfile
750750
getSelf self =
751751
lift (API.lookupSelfProfile self)
752752
>>= ifNothing (errorToWai @'E.UserNotFound)
753+
>>= lift . liftSem . API.hackForBlockingHandleChangeForE2EIdTeams
753754

754755
getUserUnqualifiedH ::
755756
(Member GalleyProvider r) =>
@@ -861,7 +862,7 @@ newtype GetActivationCodeResp
861862
instance ToJSON GetActivationCodeResp where
862863
toJSON (GetActivationCodeResp (k, c)) = object ["key" .= k, "code" .= c]
863864

864-
updateUser :: UserId -> ConnId -> Public.UserUpdate -> (Handler r) (Maybe Public.UpdateProfileError)
865+
updateUser :: Member GalleyProvider r => UserId -> ConnId -> Public.UserUpdate -> (Handler r) (Maybe Public.UpdateProfileError)
865866
updateUser uid conn uu = do
866867
eithErr <- lift $ runExceptT $ API.updateUser uid (Just conn) uu API.ForbidSCIMUpdates
867868
pure $ either Just (const Nothing) eithErr
@@ -932,7 +933,7 @@ getHandleInfoUnqualifiedH self handle = do
932933
Public.UserHandleInfo . Public.profileQualifiedId
933934
<$$> Handle.getHandleInfo self (Qualified handle domain)
934935

935-
changeHandle :: UserId -> ConnId -> Public.HandleUpdate -> (Handler r) (Maybe Public.ChangeHandleError)
936+
changeHandle :: Member GalleyProvider r => UserId -> ConnId -> Public.HandleUpdate -> (Handler r) (Maybe Public.ChangeHandleError)
936937
changeHandle u conn (Public.HandleUpdate h) = lift . exceptTToMaybe $ do
937938
handle <- maybe (throwError Public.ChangeHandleInvalid) pure $ parseHandle h
938939
API.changeHandle u (Just conn) handle API.ForbidSCIMUpdates

0 commit comments

Comments
 (0)