diff --git a/changelog.d/1-api-changes/WPB-6189 b/changelog.d/1-api-changes/WPB-6189 new file mode 100644 index 00000000000..f5f0a99d2f1 --- /dev/null +++ b/changelog.d/1-api-changes/WPB-6189 @@ -0,0 +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 diff --git a/integration/integration.cabal b/integration/integration.cabal index 1335572e592..af26c40ba14 100644 --- a/integration/integration.cabal +++ b/integration/integration.cabal @@ -99,6 +99,7 @@ library API.Gundeck API.GundeckInternal API.Nginz + API.Spar MLS.Util Notifications RunAllTests diff --git a/integration/test/API/Brig.hs b/integration/test/API/Brig.hs index 514f5c8ec53..b8c0c462c67 100644 --- a/integration/test/API/Brig.hs +++ b/integration/test/API/Brig.hs @@ -109,6 +109,7 @@ addUser dom opts = do "password" .= fromMaybe defPassword opts.password ] +-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/get_users__uid_domain___uid_ getUser :: (HasCallStack, MakesValue user, MakesValue target) => user -> @@ -147,12 +148,6 @@ deleteUser user = do submit "DELETE" $ req & addJSONObject ["password" .= defPassword] -putHandle :: (HasCallStack, MakesValue user) => user -> String -> App Response -putHandle user handle = do - req <- baseRequest user Brig Versioned "/self/handle" - submit "PUT" $ - req & addJSONObject ["handle" .= handle] - data AddClient = AddClient { ctype :: String, internal :: Bool, @@ -398,12 +393,67 @@ replaceKeyPackages cid suites kps = do & addQueryParams [("ciphersuites", intercalate "," (map (.code) suites))] & addJSONObject ["key_packages" .= map (T.decodeUtf8 . Base64.encode) kps] -getSelf :: HasCallStack => String -> String -> App Response -getSelf domain uid = do +-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/get_self +getSelf :: (HasCallStack, MakesValue caller) => caller -> App Response +getSelf caller = do + req <- baseRequest caller Brig Versioned "/self" + submit "GET" req + +-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/get_self +-- this is a low-level version of `getSelf` for testing some error conditions. +getSelf' :: HasCallStack => String -> String -> App Response +getSelf' domain uid = do let user = object ["domain" .= domain, "id" .= uid] req <- baseRequest user Brig Versioned "/self" submit "GET" req +data PutSelf = PutSelf + { accent :: Maybe Int, + assets :: Maybe [Value], -- [{"key":"string", "size":"string", "type":"string"}] + name :: Maybe String, + picture :: Maybe [String] + } + +instance Default PutSelf where + def = PutSelf Nothing Nothing Nothing Nothing + +-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/put_self +putSelf :: (HasCallStack, MakesValue caller) => caller -> PutSelf -> App Response +putSelf caller body = do + req <- baseRequest caller Brig Versioned "/self" + submit "PUT" $ + req + & addJSONObject + [ "accent_id" .= body.accent, + "assets" .= body.assets, + "name" .= body.name, + "picture" .= body.picture + ] + +-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/put_self_locale +putSelfLocale :: (HasCallStack, MakesValue caller) => caller -> String -> App Response +putSelfLocale caller locale = do + req <- baseRequest caller Brig Versioned "/self/locale" + submit "PUT" $ req & addJSONObject ["locale" .= locale] + +-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/put_users__uid__email +-- +-- NOTE: the full process of changing (and confirming) the email address is more complicated. +-- see /services/brig/test/integration for details. +putSelfEmail :: (HasCallStack, MakesValue caller) => caller -> String -> App Response +putSelfEmail caller emailAddress = do + callerid <- asString $ caller %. "id" + req <- baseRequest caller Brig Versioned $ joinHttpPath ["users", callerid, "email"] + submit "PUT" $ req & addJSONObject ["email" .= emailAddress] + +-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/put_self_handle +-- FUTUREWORK: rename to putSelfHandle for consistency +putHandle :: (HasCallStack, MakesValue user) => user -> String -> App Response +putHandle user handle = do + req <- baseRequest user Brig Versioned "/self/handle" + submit "PUT" $ + req & addJSONObject ["handle" .= handle] + getUserSupportedProtocols :: (HasCallStack, MakesValue user, MakesValue target) => user -> diff --git a/integration/test/API/BrigInternal.hs b/integration/test/API/BrigInternal.hs index 9db84307a1b..03903bfdfee 100644 --- a/integration/test/API/BrigInternal.hs +++ b/integration/test/API/BrigInternal.hs @@ -52,6 +52,12 @@ createUser domain cu = do ] ) +-- | https://staging-nginz-https.zinfra.io/api-internal/swagger-ui/brig/#/brig/get_i_users +getUsersId :: (HasCallStack, MakesValue domain) => domain -> [String] -> App Response +getUsersId domain ids = do + req <- baseRequest domain Brig Unversioned "/i/users" + submit "GET" $ req & addQueryParams [("ids", intercalate "," ids)] + data FedConn = FedConn { domain :: String, searchStrategy :: String, diff --git a/integration/test/API/Spar.hs b/integration/test/API/Spar.hs new file mode 100644 index 00000000000..aff62ca6d5a --- /dev/null +++ b/integration/test/API/Spar.hs @@ -0,0 +1,10 @@ +module API.Spar where + +import GHC.Stack +import Testlib.Prelude + +-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/get_scim_auth_tokens +getScimTokens :: (HasCallStack, MakesValue caller) => caller -> App Response +getScimTokens caller = do + req <- baseRequest caller Spar Versioned "/scim/auth-tokens" + submit "GET" req diff --git a/integration/test/Test/Demo.hs b/integration/test/Test/Demo.hs index 9691212c5f9..681b5267d51 100644 --- a/integration/test/Test/Demo.hs +++ b/integration/test/Test/Demo.hs @@ -105,7 +105,7 @@ testDynamicBackend = do ownDomain <- objDomain OwnDomain user <- randomUser OwnDomain def uid <- objId user - bindResponse (BrigP.getSelf ownDomain uid) $ \resp -> do + bindResponse (BrigP.getSelf' ownDomain uid) $ \resp -> do resp.status `shouldMatchInt` 200 (resp.json %. "id") `shouldMatch` objId user @@ -117,18 +117,18 @@ testDynamicBackend = do resp.json %. "setRestrictUserCreation" `shouldMatch` False -- user created in own domain should not be found in dynamic backend - bindResponse (BrigP.getSelf dynDomain uid) $ \resp -> do + bindResponse (BrigP.getSelf' dynDomain uid) $ \resp -> do resp.status `shouldMatchInt` 404 -- now create a user in the dynamic backend userD1 <- randomUser dynDomain def uidD1 <- objId userD1 - bindResponse (BrigP.getSelf dynDomain uidD1) $ \resp -> do + bindResponse (BrigP.getSelf' dynDomain uidD1) $ \resp -> do resp.status `shouldMatchInt` 200 (resp.json %. "id") `shouldMatch` objId userD1 -- the d1 user should not be found in the own domain - bindResponse (BrigP.getSelf ownDomain uidD1) $ \resp -> do + bindResponse (BrigP.getSelf' ownDomain uidD1) $ \resp -> do resp.status `shouldMatchInt` 404 testStartMultipleDynamicBackends :: HasCallStack => App () diff --git a/integration/test/Test/User.hs b/integration/test/Test/User.hs index 5e5d1edb419..903de5a0724 100644 --- a/integration/test/Test/User.hs +++ b/integration/test/Test/User.hs @@ -1,7 +1,13 @@ +{-# OPTIONS_GHC -Wno-ambiguous-fields #-} + module Test.User where import API.Brig import API.BrigInternal +import API.GalleyInternal +import API.Spar +import qualified Data.UUID as UUID +import qualified Data.UUID.V4 as UUID import SetupHelpers import Testlib.Prelude @@ -47,3 +53,120 @@ testCreateUserSupportedProtocols = do bindResponse (createUser OwnDomain def {supportedProtocols = Just ["proteus", "mixed"]}) $ \resp -> do resp.status `shouldMatchInt` 400 resp.json %. "label" `shouldMatch` "bad-request" + +-- | For now this only tests attempts to update /self/handle in E2EId-enabled teams. More +-- tests can be found under `/services/brig/test/integration` (and should be moved here). +testUpdateHandle :: HasCallStack => App () +testUpdateHandle = do + -- create team with one member, without scim, but with `mlsE2EId` enabled. + (owner, team, [mem1]) <- createTeam OwnDomain 2 + mem1id <- asString $ mem1 %. "id" + + let featureName = "mlsE2EId" + bindResponse (getTeamFeature owner featureName team) $ \resp -> do + resp.status `shouldMatchInt` 200 + resp.json %. "status" `shouldMatch` "disabled" + setTeamFeatureStatus owner team featureName "enabled" + bindResponse (getTeamFeature owner featureName team) $ \resp -> do + resp.status `shouldMatchInt` 200 + resp.json %. "status" `shouldMatch` "enabled" + + -- all as expected here. (see the second time we check this at the end of the test for an + -- explanation why we care.) + bindResponse (getSelf mem1) $ \resp -> do + resp.status `shouldMatchInt` 200 + resp.json %. "managed_by" `shouldMatch` "wire" + bindResponse (getUsersId owner [mem1id]) $ \resp -> do + resp.status `shouldMatchInt` 200 + mb <- (assertOne =<< asList resp.json) %. "managed_by" + mb `shouldMatch` "wire" + + -- mem1 attempts to update handle for the first time => success + -- + -- this is desired, because without SCIM users need to pick their own handles initially. + -- moreover it is fine, because if `handle == NULL`, no mls E2Eid client certs can be + -- created. + handle <- UUID.toString <$> liftIO UUID.nextRandom + bindResponse (putHandle mem1 handle) $ \resp -> do + resp.status `shouldMatchInt` 200 + bindResponse (putHandle mem1 handle) $ \resp -> do + -- idempotency + resp.status `shouldMatchInt` 200 + + -- mem1 attempts to update handle again => failure + handle2 <- UUID.toString <$> liftIO UUID.nextRandom + bindResponse (putHandle mem1 handle2) $ \resp -> do + resp.status `shouldMatchInt` 403 + resp.json %. "label" `shouldMatch` "managed-by-scim" + + -- now self thinks it is managed by "scim", so clients can block change attempts to handle, + -- display name without adding E2EId-specific logic. this is just a hack, though: `GET + -- /self` is the only place where this is happening, other end-points still report the truth + -- that is still stored correctly in the DB. + -- + -- details: https://wearezeta.atlassian.net/browse/WPB-6189. + -- FUTUREWORK: figure out a better way for clients to detect E2EId (V6?) + bindResponse (getSelf mem1) $ \resp -> do + resp.status `shouldMatchInt` 200 + resp.json %. "managed_by" `shouldMatch` "scim" + bindResponse (getUsersId owner [mem1id]) $ \resp -> do + resp.status `shouldMatchInt` 200 + mb <- (assertOne =<< asList resp.json) %. "managed_by" + mb `shouldMatch` "wire" + bindResponse (getScimTokens owner) $ \resp -> do + resp.status `shouldMatchInt` 200 + resp.json %. "tokens" `shouldMatch` ([] @String) + +-- | For now this only tests attempts to update one's own display name, email address, or +-- language in E2EId-enabled teams (ie., everything except handle). More tests can be found +-- under `/services/brig/test/integration` (and should be moved here). +testUpdateSelf :: HasCallStack => TestUpdateSelfMode -> App () +testUpdateSelf mode = do + -- create team with one member, without scim, but with `mlsE2EId` enabled. + (owner, team, [mem1]) <- createTeam OwnDomain 2 + + let featureName = "mlsE2EId" + bindResponse (getTeamFeature owner featureName team) $ \resp -> do + resp.status `shouldMatchInt` 200 + resp.json %. "status" `shouldMatch` "disabled" + setTeamFeatureStatus owner team featureName "enabled" + bindResponse (getTeamFeature owner featureName team) $ \resp -> do + resp.status `shouldMatchInt` 200 + resp.json %. "status" `shouldMatch` "enabled" + + case mode of + TestUpdateDisplayName -> do + -- blocked unconditionally + someDisplayName <- UUID.toString <$> liftIO UUID.nextRandom + before <- getSelf mem1 + bindResponse (putSelf mem1 def {name = Just someDisplayName}) $ \resp -> do + resp.status `shouldMatchInt` 403 + resp.json %. "label" `shouldMatch` "managed-by-scim" + after <- getSelf mem1 + void $ (before.json %. "name") `shouldMatch` (after.json %. "name") + TestUpdateEmailAddress -> do + -- allowed unconditionally *for owner* (this is a bit off-topic: team members can't + -- change their email addresses themselves under any conditions) + someEmail <- (<> "@example.com") . UUID.toString <$> liftIO UUID.nextRandom + bindResponse (putSelfEmail owner someEmail) $ \resp -> do + resp.status `shouldMatchInt` 200 + TestUpdateLocale -> do + -- scim maps "User.preferredLanguage" to brig's locale field. allowed unconditionally. + -- we try two languages to make sure it doesn't work because it's already the active + -- locale. + forM_ ["uk", "he"] $ \someLocale -> + bindResponse (putSelfLocale mem1 someLocale) $ \resp -> do + resp.status `shouldMatchInt` 200 + +data TestUpdateSelfMode + = TestUpdateDisplayName + | TestUpdateEmailAddress + | TestUpdateLocale + deriving (Eq, Show, Bounded, Enum) + +instance HasTests x => HasTests (TestUpdateSelfMode -> x) where + mkTests m n s f x = + mconcat + [ mkTests m (n <> "[mode=" <> show mode <> "]") s f (x mode) + | mode <- [minBound ..] + ] diff --git a/services/brig/src/Brig/API/Internal.hs b/services/brig/src/Brig/API/Internal.hs index c10b4cad66b..72e6eaeb8ff 100644 --- a/services/brig/src/Brig/API/Internal.hs +++ b/services/brig/src/Brig/API/Internal.hs @@ -680,13 +680,13 @@ getRichInfoMultiH :: Maybe (CommaSeparatedList UserId) -> (Handler r) [(UserId, getRichInfoMultiH (maybe [] fromCommaSeparatedList -> uids) = lift $ wrapClient $ API.lookupRichInfoMultiUsers uids -updateHandleH :: UserId -> HandleUpdate -> (Handler r) NoContent +updateHandleH :: Member GalleyProvider r => UserId -> HandleUpdate -> (Handler r) NoContent updateHandleH uid (HandleUpdate handleUpd) = NoContent <$ do handle <- validateHandle handleUpd API.changeHandle uid Nothing handle API.AllowSCIMUpdates !>> changeHandleError -updateUserNameH :: UserId -> NameUpdate -> (Handler r) NoContent +updateUserNameH :: Member GalleyProvider r => UserId -> NameUpdate -> (Handler r) NoContent updateUserNameH uid (NameUpdate nameUpd) = NoContent <$ do name <- either (const $ throwStd (errorToWai @'E.InvalidUser)) pure $ mkName nameUpd diff --git a/services/brig/src/Brig/API/Public.hs b/services/brig/src/Brig/API/Public.hs index 0c026e06d0f..d61beffefa4 100644 --- a/services/brig/src/Brig/API/Public.hs +++ b/services/brig/src/Brig/API/Public.hs @@ -746,10 +746,11 @@ createUser (Public.NewUserPublic new) = lift . runExceptT $ do Public.NewTeamMemberSSO _ -> Team.sendMemberWelcomeMail e t n l -getSelf :: UserId -> (Handler r) Public.SelfProfile +getSelf :: Member GalleyProvider r => UserId -> (Handler r) Public.SelfProfile getSelf self = lift (API.lookupSelfProfile self) >>= ifNothing (errorToWai @'E.UserNotFound) + >>= lift . liftSem . API.hackForBlockingHandleChangeForE2EIdTeams getUserUnqualifiedH :: (Member GalleyProvider r) => @@ -861,7 +862,7 @@ newtype GetActivationCodeResp instance ToJSON GetActivationCodeResp where toJSON (GetActivationCodeResp (k, c)) = object ["key" .= k, "code" .= c] -updateUser :: UserId -> ConnId -> Public.UserUpdate -> (Handler r) (Maybe Public.UpdateProfileError) +updateUser :: Member GalleyProvider r => UserId -> ConnId -> Public.UserUpdate -> (Handler r) (Maybe Public.UpdateProfileError) updateUser uid conn uu = do eithErr <- lift $ runExceptT $ API.updateUser uid (Just conn) uu API.ForbidSCIMUpdates pure $ either Just (const Nothing) eithErr @@ -932,7 +933,7 @@ getHandleInfoUnqualifiedH self handle = do Public.UserHandleInfo . Public.profileQualifiedId <$$> Handle.getHandleInfo self (Qualified handle domain) -changeHandle :: UserId -> ConnId -> Public.HandleUpdate -> (Handler r) (Maybe Public.ChangeHandleError) +changeHandle :: Member GalleyProvider r => UserId -> ConnId -> Public.HandleUpdate -> (Handler r) (Maybe Public.ChangeHandleError) changeHandle u conn (Public.HandleUpdate h) = lift . exceptTToMaybe $ do handle <- maybe (throwError Public.ChangeHandleInvalid) pure $ parseHandle h API.changeHandle u (Just conn) handle API.ForbidSCIMUpdates diff --git a/services/brig/src/Brig/API/User.hs b/services/brig/src/Brig/API/User.hs index 73d8d188985..d9674b685a3 100644 --- a/services/brig/src/Brig/API/User.hs +++ b/services/brig/src/Brig/API/User.hs @@ -89,6 +89,7 @@ module Brig.API.User -- * Utilities fetchUserIdentity, + hackForBlockingHandleChangeForE2EIdTeams, ) where @@ -116,7 +117,7 @@ import Brig.Effects.BlacklistStore (BlacklistStore) import Brig.Effects.BlacklistStore qualified as BlacklistStore import Brig.Effects.CodeStore (CodeStore) import Brig.Effects.CodeStore qualified as E -import Brig.Effects.GalleyProvider (GalleyProvider) +import Brig.Effects.GalleyProvider import Brig.Effects.GalleyProvider qualified as GalleyProvider import Brig.Effects.PasswordResetStore (PasswordResetStore) import Brig.Effects.PasswordResetStore qualified as E @@ -176,7 +177,7 @@ import Wire.API.Password import Wire.API.Routes.Internal.Brig.Connection import Wire.API.Routes.Internal.Galley.TeamsIntra qualified as Team import Wire.API.Team hiding (newTeam) -import Wire.API.Team.Feature (forgetLock) +import Wire.API.Team.Feature import Wire.API.Team.Invitation import Wire.API.Team.Invitation qualified as Team import Wire.API.Team.Member (TeamMember, legalHoldStatus) @@ -575,7 +576,7 @@ checkRestrictedUserCreation new = do ------------------------------------------------------------------------------- -- Update Profile -updateUser :: UserId -> Maybe ConnId -> UserUpdate -> AllowSCIMUpdates -> ExceptT UpdateProfileError (AppT r) () +updateUser :: Member GalleyProvider r => UserId -> Maybe ConnId -> UserUpdate -> AllowSCIMUpdates -> ExceptT UpdateProfileError (AppT r) () updateUser uid mconn uu allowScim = do for_ (uupName uu) $ \newName -> do mbUser <- lift . wrapClient $ Data.lookupUser WithPendingInvitations uid @@ -586,6 +587,10 @@ updateUser uid mconn uu allowScim = do || allowScim == AllowSCIMUpdates ) $ throwE DisplayNameManagedByScim + hasE2EId <- lift . liftSem . userUnderE2EId $ uid + when (hasE2EId && newName /= userDisplayName user) $ + throwE DisplayNameManagedByScim + lift $ do wrapClient $ Data.updateUser uid uu wrapHttpClient $ Intra.onUserEvent uid mconn (profileUpdated uid uu) @@ -617,7 +622,7 @@ changeSupportedProtocols uid conn prots = do -------------------------------------------------------------------------------- -- Change Handle -changeHandle :: UserId -> Maybe ConnId -> Handle -> AllowSCIMUpdates -> ExceptT ChangeHandleError (AppT r) () +changeHandle :: Member GalleyProvider r => UserId -> Maybe ConnId -> Handle -> AllowSCIMUpdates -> ExceptT ChangeHandleError (AppT r) () changeHandle uid mconn hdl allowScim = do when (isBlacklistedHandle hdl) $ throwE ChangeHandleInvalid @@ -631,6 +636,9 @@ changeHandle uid mconn hdl allowScim = do || allowScim == AllowSCIMUpdates ) $ throwE ChangeHandleManagedByScim + hasE2EId <- lift . liftSem . userUnderE2EId . userId $ u + when (hasE2EId && userHandle u `notElem` [Nothing, Just hdl]) $ + throwE ChangeHandleManagedByScim claim u where claim u = do @@ -1615,3 +1623,24 @@ phonePrefixDelete = liftSem . BlacklistPhonePrefixStore.delete phonePrefixInsert :: Member BlacklistPhonePrefixStore r => ExcludedPrefix -> (AppT r) () phonePrefixInsert = liftSem . BlacklistPhonePrefixStore.insert + +userUnderE2EId :: Member GalleyProvider r => UserId -> Sem r Bool +userUnderE2EId uid = do + wsStatus . afcMlsE2EId <$> getAllFeatureConfigsForUser (Just uid) <&> \case + FeatureStatusEnabled -> True + FeatureStatusDisabled -> False + +-- | This is a hack! +-- +-- Background: +-- - https://wearezeta.atlassian.net/browse/WPB-6189. +-- - comments in `testUpdateHandle` in `/integration`. +-- +-- FUTUREWORK: figure out a better way for clients to detect E2EId (V6?) +hackForBlockingHandleChangeForE2EIdTeams :: Member GalleyProvider r => SelfProfile -> Sem r SelfProfile +hackForBlockingHandleChangeForE2EIdTeams (SelfProfile user) = do + hasE2EId <- userUnderE2EId . userId $ user + pure . SelfProfile $ + if (hasE2EId && isJust (userHandle user)) + then user {userManagedBy = ManagedByScim} + else user