From 291b4f2a63fad35b557cda7b7380ebc73c0c7c46 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 24 Jan 2024 12:07:14 +0100 Subject: [PATCH] Revert "Block changes to some user data in mlsE2EId teams (WPB-6189)" This reverts commit c71564245153ccfcfee6e4e43f24b11f88e5d5f5. --- changelog.d/1-api-changes/WPB-6189 | 1 - integration/integration.cabal | 1 - integration/test/API/Brig.hs | 66 ++----------- integration/test/API/BrigInternal.hs | 6 -- integration/test/API/Spar.hs | 10 -- integration/test/Test/Demo.hs | 8 +- integration/test/Test/User.hs | 123 ------------------------- services/brig/src/Brig/API/Internal.hs | 4 +- services/brig/src/Brig/API/Public.hs | 7 +- services/brig/src/Brig/API/User.hs | 37 +------- 10 files changed, 21 insertions(+), 242 deletions(-) delete mode 100644 changelog.d/1-api-changes/WPB-6189 delete mode 100644 integration/test/API/Spar.hs diff --git a/changelog.d/1-api-changes/WPB-6189 b/changelog.d/1-api-changes/WPB-6189 deleted file mode 100644 index f5f0a99d2f1..00000000000 --- a/changelog.d/1-api-changes/WPB-6189 +++ /dev/null @@ -1 +0,0 @@ -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 af26c40ba14..1335572e592 100644 --- a/integration/integration.cabal +++ b/integration/integration.cabal @@ -99,7 +99,6 @@ 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 b8c0c462c67..514f5c8ec53 100644 --- a/integration/test/API/Brig.hs +++ b/integration/test/API/Brig.hs @@ -109,7 +109,6 @@ 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 -> @@ -148,6 +147,12 @@ 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, @@ -393,67 +398,12 @@ replaceKeyPackages cid suites kps = do & addQueryParams [("ciphersuites", intercalate "," (map (.code) suites))] & addJSONObject ["key_packages" .= map (T.decodeUtf8 . Base64.encode) kps] --- | 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 +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 03903bfdfee..9db84307a1b 100644 --- a/integration/test/API/BrigInternal.hs +++ b/integration/test/API/BrigInternal.hs @@ -52,12 +52,6 @@ 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 deleted file mode 100644 index aff62ca6d5a..00000000000 --- a/integration/test/API/Spar.hs +++ /dev/null @@ -1,10 +0,0 @@ -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 681b5267d51..9691212c5f9 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 903de5a0724..5e5d1edb419 100644 --- a/integration/test/Test/User.hs +++ b/integration/test/Test/User.hs @@ -1,13 +1,7 @@ -{-# 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 @@ -53,120 +47,3 @@ 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 72e6eaeb8ff..c10b4cad66b 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 :: Member GalleyProvider r => UserId -> HandleUpdate -> (Handler r) NoContent +updateHandleH :: UserId -> HandleUpdate -> (Handler r) NoContent updateHandleH uid (HandleUpdate handleUpd) = NoContent <$ do handle <- validateHandle handleUpd API.changeHandle uid Nothing handle API.AllowSCIMUpdates !>> changeHandleError -updateUserNameH :: Member GalleyProvider r => UserId -> NameUpdate -> (Handler r) NoContent +updateUserNameH :: 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 d61beffefa4..0c026e06d0f 100644 --- a/services/brig/src/Brig/API/Public.hs +++ b/services/brig/src/Brig/API/Public.hs @@ -746,11 +746,10 @@ createUser (Public.NewUserPublic new) = lift . runExceptT $ do Public.NewTeamMemberSSO _ -> Team.sendMemberWelcomeMail e t n l -getSelf :: Member GalleyProvider r => UserId -> (Handler r) Public.SelfProfile +getSelf :: UserId -> (Handler r) Public.SelfProfile getSelf self = lift (API.lookupSelfProfile self) >>= ifNothing (errorToWai @'E.UserNotFound) - >>= lift . liftSem . API.hackForBlockingHandleChangeForE2EIdTeams getUserUnqualifiedH :: (Member GalleyProvider r) => @@ -862,7 +861,7 @@ newtype GetActivationCodeResp instance ToJSON GetActivationCodeResp where toJSON (GetActivationCodeResp (k, c)) = object ["key" .= k, "code" .= c] -updateUser :: Member GalleyProvider r => UserId -> ConnId -> Public.UserUpdate -> (Handler r) (Maybe Public.UpdateProfileError) +updateUser :: 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 @@ -933,7 +932,7 @@ getHandleInfoUnqualifiedH self handle = do Public.UserHandleInfo . Public.profileQualifiedId <$$> Handle.getHandleInfo self (Qualified handle domain) -changeHandle :: Member GalleyProvider r => UserId -> ConnId -> Public.HandleUpdate -> (Handler r) (Maybe Public.ChangeHandleError) +changeHandle :: 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 d9674b685a3..73d8d188985 100644 --- a/services/brig/src/Brig/API/User.hs +++ b/services/brig/src/Brig/API/User.hs @@ -89,7 +89,6 @@ module Brig.API.User -- * Utilities fetchUserIdentity, - hackForBlockingHandleChangeForE2EIdTeams, ) where @@ -117,7 +116,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 +import Brig.Effects.GalleyProvider (GalleyProvider) import Brig.Effects.GalleyProvider qualified as GalleyProvider import Brig.Effects.PasswordResetStore (PasswordResetStore) import Brig.Effects.PasswordResetStore qualified as E @@ -177,7 +176,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 +import Wire.API.Team.Feature (forgetLock) import Wire.API.Team.Invitation import Wire.API.Team.Invitation qualified as Team import Wire.API.Team.Member (TeamMember, legalHoldStatus) @@ -576,7 +575,7 @@ checkRestrictedUserCreation new = do ------------------------------------------------------------------------------- -- Update Profile -updateUser :: Member GalleyProvider r => UserId -> Maybe ConnId -> UserUpdate -> AllowSCIMUpdates -> ExceptT UpdateProfileError (AppT r) () +updateUser :: 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 @@ -587,10 +586,6 @@ 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) @@ -622,7 +617,7 @@ changeSupportedProtocols uid conn prots = do -------------------------------------------------------------------------------- -- Change Handle -changeHandle :: Member GalleyProvider r => UserId -> Maybe ConnId -> Handle -> AllowSCIMUpdates -> ExceptT ChangeHandleError (AppT r) () +changeHandle :: UserId -> Maybe ConnId -> Handle -> AllowSCIMUpdates -> ExceptT ChangeHandleError (AppT r) () changeHandle uid mconn hdl allowScim = do when (isBlacklistedHandle hdl) $ throwE ChangeHandleInvalid @@ -636,9 +631,6 @@ 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 @@ -1623,24 +1615,3 @@ 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