Forbid manual updates to email, handle, display for SCIM-managed users (#1320)

Co-authored-by: fisx <[email protected]>

Author: 2 people

services/brig/src/Brig/API/Error.hs

@@ -116,6 +116,7 @@ changeEmailError :: ChangeEmailError -> Error
 changeEmailError (InvalidNewEmail _ _) = StdError invalidEmail
 changeEmailError (EmailExists _) = StdError userKeyExists
 changeEmailError (ChangeBlacklistedEmail _) = StdError blacklistedEmail
+changeEmailError EmailManagedByScim = StdError $ propertyManagedByScim "email"
 
 changePhoneError :: ChangePhoneError -> Error
 changePhoneError (InvalidNewPhone _) = StdError invalidPhone
@@ -130,6 +131,7 @@ changeHandleError :: ChangeHandleError -> Error
 changeHandleError ChangeHandleNoIdentity = StdError (noIdentity 2)
 changeHandleError ChangeHandleExists = StdError handleExists
 changeHandleError ChangeHandleInvalid = StdError invalidHandle
+changeHandleError ChangeHandleManagedByScim = StdError $ propertyManagedByScim "handle"
 
 legalHoldLoginError :: LegalHoldLoginError -> Error
 legalHoldLoginError LegalHoldLoginNoBindingTeam = StdError noBindingTeam
@@ -207,6 +209,10 @@ phoneError PhoneNumberUnreachable = StdError invalidPhone
 phoneError PhoneNumberBarred = StdError blacklistedPhone
 phoneError (PhoneBudgetExhausted t) = RichError phoneBudgetExhausted (PhoneBudgetTimeout t) []
 
+updateProfileError :: UpdateProfileError -> Error
+updateProfileError DisplayNameManagedByScim = StdError (propertyManagedByScim "name")
+updateProfileError (ProfileNotFound _) = StdError userNotFound
+
 -- WAI Errors -----------------------------------------------------------------
 
 tooManyProperties :: Wai.Error
@@ -427,6 +433,9 @@ insufficientTeamPermissions = Wai.Error status403 "insufficient-permissions" "In
 noBindingTeam :: Wai.Error
 noBindingTeam = Wai.Error status403 "no-binding-team" "Operation allowed only on binding teams"
 
+propertyManagedByScim :: LText -> Wai.Error
+propertyManagedByScim prop = Wai.Error status403 "managed-by-scim" $ "Updating \"" <> prop <> "\" is not allowed, because it is managed by SCIM"
+
 sameBindingTeamUsers :: Wai.Error
 sameBindingTeamUsers = Wai.Error status403 "same-binding-team-users" "Operation not allowed to binding team users."
 

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

@@ -322,17 +322,17 @@ deleteUserNoVerify uid = do
 changeSelfEmailMaybeSendH :: UserId ::: Bool ::: JsonRequest EmailUpdate -> Handler Response
 changeSelfEmailMaybeSendH (u ::: validate ::: req) = do
   email <- euEmail <$> parseJsonBody req
-  changeSelfEmailMaybeSend u (if validate then ActuallySendEmail else DoNotSendEmail) email >>= \case
+  changeSelfEmailMaybeSend u (if validate then ActuallySendEmail else DoNotSendEmail) email API.AllowSCIMUpdates >>= \case
     ChangeEmailResponseIdempotent -> pure (setStatus status204 empty)
     ChangeEmailResponseNeedsActivation -> pure (setStatus status202 empty)
 
 data MaybeSendEmail = ActuallySendEmail | DoNotSendEmail
 
-changeSelfEmailMaybeSend :: UserId -> MaybeSendEmail -> Email -> Handler ChangeEmailResponse
-changeSelfEmailMaybeSend u ActuallySendEmail email = do
-  API.changeSelfEmail u email
-changeSelfEmailMaybeSend u DoNotSendEmail email = do
-  API.changeEmail u email !>> changeEmailError >>= \case
+changeSelfEmailMaybeSend :: UserId -> MaybeSendEmail -> Email -> API.AllowSCIMUpdates -> Handler ChangeEmailResponse
+changeSelfEmailMaybeSend u ActuallySendEmail email allowScim = do
+  API.changeSelfEmail u email allowScim
+changeSelfEmailMaybeSend u DoNotSendEmail email allowScim = do
+  API.changeEmail u email allowScim !>> changeEmailError >>= \case
     ChangeEmailIdempotent -> pure ChangeEmailResponseIdempotent
     ChangeEmailNeedsActivation _ -> pure ChangeEmailResponseNeedsActivation
 
@@ -518,7 +518,7 @@ updateHandleH (uid ::: _ ::: body) = empty <$ (updateHandle uid =<< parseJsonBod
 updateHandle :: UserId -> HandleUpdate -> Handler ()
 updateHandle uid (HandleUpdate handleUpd) = do
   handle <- validateHandle handleUpd
-  API.changeHandle uid Nothing handle !>> changeHandleError
+  API.changeHandle uid Nothing handle API.AllowSCIMUpdates !>> changeHandleError
 
 updateUserNameH :: UserId ::: JSON ::: JsonRequest NameUpdate -> Handler Response
 updateUserNameH (uid ::: _ ::: body) = empty <$ (updateUserName uid =<< parseJsonBody body)
@@ -534,7 +534,7 @@ updateUserName uid (NameUpdate nameUpd) = do
             uupAccentId = Nothing
           }
   lift (Data.lookupUser WithPendingInvitations uid) >>= \case
-    Just _ -> lift $ API.updateUser uid Nothing uu
+    Just _ -> API.updateUser uid Nothing uu API.AllowSCIMUpdates !>> updateProfileError
     Nothing -> throwStd invalidUser
 
 checkHandleInternalH :: Text -> Handler Response
@@ -547,7 +547,7 @@ checkHandleInternalH =
 getContactListH :: JSON ::: UserId -> Handler Response
 getContactListH (_ ::: uid) = do
   contacts <- lift $ API.lookupContactList uid
-  return $ json $ (UserIds contacts)
+  return $ json $ UserIds contacts
 
 -- Deprecated
 

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

@@ -1290,7 +1290,7 @@ instance ToJSON GetActivationCodeResp where
 updateUserH :: UserId ::: ConnId ::: JsonRequest Public.UserUpdate -> Handler Response
 updateUserH (uid ::: conn ::: req) = do
   uu <- parseJsonBody req
-  lift $ API.updateUser uid (Just conn) uu
+  API.updateUser uid (Just conn) uu API.ForbidSCIMUpdates !>> updateProfileError
   return empty
 
 changePhoneH :: UserId ::: ConnId ::: JsonRequest Public.PhoneUpdate -> Handler Response
@@ -1383,7 +1383,8 @@ changeHandleH (u ::: conn ::: req) = do
 changeHandle :: UserId -> ConnId -> Public.HandleUpdate -> Handler ()
 changeHandle u conn (Public.HandleUpdate h) = do
   handle <- API.validateHandle h
-  API.changeHandle u (Just conn) handle !>> changeHandleError
+  -- TODO check here
+  API.changeHandle u (Just conn) handle API.ForbidSCIMUpdates !>> changeHandleError
 
 beginPasswordResetH :: JSON ::: JsonRequest Public.NewPasswordReset -> Handler Response
 beginPasswordResetH (_ ::: req) = do
@@ -1434,7 +1435,7 @@ customerExtensionCheckBlockedDomains email = do
 changeSelfEmailH :: UserId ::: ConnId ::: JsonRequest Public.EmailUpdate -> Handler Response
 changeSelfEmailH (u ::: _ ::: req) = do
   email <- Public.euEmail <$> parseJsonBody req
-  API.changeSelfEmail u email >>= \case
+  API.changeSelfEmail u email API.ForbidSCIMUpdates >>= \case
     ChangeEmailResponseIdempotent -> pure (setStatus status204 empty)
     ChangeEmailResponseNeedsActivation -> pure (setStatus status202 empty)
 

services/brig/src/Brig/API/Types.hs

@@ -103,6 +103,10 @@ data CreateUserError
   | -- | Some precondition on another Wire service failed. We propagate this error.
     ExternalPreconditionFailed Wai.Error
 
+data UpdateProfileError
+  = DisplayNameManagedByScim
+  | ProfileNotFound UserId
+
 data InvitationError
   = InviteeEmailExists UserId
   | InviteInvalidEmail Email
@@ -163,11 +167,13 @@ data ChangeEmailError
   = InvalidNewEmail !Email !String
   | EmailExists !Email
   | ChangeBlacklistedEmail !Email
+  | EmailManagedByScim
 
 data ChangeHandleError
   = ChangeHandleNoIdentity
   | ChangeHandleExists
   | ChangeHandleInvalid
+  | ChangeHandleManagedByScim
 
 data SendActivationCodeError
   = InvalidRecipient UserKey

services/brig/src/Brig/API/User.hs

@@ -56,6 +56,7 @@ module Brig.API.User
     checkHandles,
     isBlacklistedHandle,
     Data.reauthenticate,
+    AllowSCIMUpdates (..),
 
     -- * Activation
     sendActivationCode,
@@ -147,6 +148,11 @@ import Network.Wai.Utilities
 import qualified System.Logger.Class as Log
 import System.Logger.Message
 
+data AllowSCIMUpdates
+  = AllowSCIMUpdates
+  | ForbidSCIMUpdates
+  deriving (Show, Eq, Ord)
+
 -------------------------------------------------------------------------------
 -- Create User
 
@@ -394,13 +400,20 @@ checkRestrictedUserCreation new = do
 -------------------------------------------------------------------------------
 -- Update Profile
 
--- FUTUREWORK: this and other functions should refuse to modify a ManagedByScim user. See
--- {#SparBrainDump}  https://github.com/zinfra/backend-issues/issues/1632
-
-updateUser :: UserId -> Maybe ConnId -> UserUpdate -> AppIO ()
-updateUser uid mconn uu = do
-  Data.updateUser uid uu
-  Intra.onUserEvent uid mconn (profileUpdated uid uu)
+updateUser :: UserId -> Maybe ConnId -> UserUpdate -> AllowSCIMUpdates -> ExceptT UpdateProfileError AppIO ()
+updateUser uid mconn uu allowScim = do
+  for_ (uupName uu) $ \newName -> do
+    mbUser <- lift $ Data.lookupUser WithPendingInvitations uid
+    user <- maybe (throwE (ProfileNotFound uid)) pure mbUser
+    unless
+      ( userManagedBy user /= ManagedByScim
+          || userDisplayName user == newName
+          || allowScim == AllowSCIMUpdates
+      )
+      $ throwE DisplayNameManagedByScim
+  lift $ do
+    Data.updateUser uid uu
+    Intra.onUserEvent uid mconn (profileUpdated uid uu)
 
 -------------------------------------------------------------------------------
 -- Update Locale
@@ -421,14 +434,21 @@ changeManagedBy uid conn (ManagedByUpdate mb) = do
 --------------------------------------------------------------------------------
 -- Change Handle
 
-changeHandle :: UserId -> Maybe ConnId -> Handle -> ExceptT ChangeHandleError AppIO ()
-changeHandle uid mconn hdl = do
+changeHandle :: UserId -> Maybe ConnId -> Handle -> AllowSCIMUpdates -> ExceptT ChangeHandleError AppIO ()
+changeHandle uid mconn hdl allowScim = do
   when (isBlacklistedHandle hdl) $
     throwE ChangeHandleInvalid
   usr <- lift $ Data.lookupUser WithPendingInvitations uid
   case usr of
     Nothing -> throwE ChangeHandleNoIdentity
-    Just u -> claim u
+    Just u -> do
+      unless
+        ( userManagedBy u /= ManagedByScim
+            || Just hdl == userHandle u
+            || allowScim == AllowSCIMUpdates
+        )
+        $ throwE ChangeHandleManagedByScim
+      claim u
   where
     claim u = do
       unless (isJust (userIdentity u)) $
@@ -487,9 +507,9 @@ checkHandles check num = reverse <$> collectFree [] check num
 
 -- | Call 'changeEmail' and process result: if email changes to itself, succeed, if not, send
 -- validation email.
-changeSelfEmail :: UserId -> Email -> ExceptT Error.Error AppIO ChangeEmailResponse
-changeSelfEmail u email = do
-  changeEmail u email !>> Error.changeEmailError >>= \case
+changeSelfEmail :: UserId -> Email -> AllowSCIMUpdates -> ExceptT Error.Error AppIO ChangeEmailResponse
+changeSelfEmail u email allowScim = do
+  changeEmail u email allowScim !>> Error.changeEmailError >>= \case
     ChangeEmailIdempotent ->
       pure ChangeEmailResponseIdempotent
     ChangeEmailNeedsActivation (usr, adata, en) -> do
@@ -505,8 +525,8 @@ changeSelfEmail u email = do
         (userIdentity usr)
 
 -- | Prepare changing the email (checking a number of invariants).
-changeEmail :: UserId -> Email -> ExceptT ChangeEmailError AppIO ChangeEmailResult
-changeEmail u email = do
+changeEmail :: UserId -> Email -> AllowSCIMUpdates -> ExceptT ChangeEmailError AppIO ChangeEmailResult
+changeEmail u email allowScim = do
   em <-
     either
       (throwE . InvalidNewEmail email)
@@ -525,6 +545,11 @@ changeEmail u email = do
     -- The user already has an email address and the new one is exactly the same
     Just current | current == em -> return ChangeEmailIdempotent
     _ -> do
+      unless
+        ( userManagedBy usr /= ManagedByScim
+            || allowScim == AllowSCIMUpdates
+        )
+        $ throwE EmailManagedByScim
       timeout <- setActivationTimeout <$> view settings
       act <- lift $ Data.newActivation ek timeout (Just u)
       return $ ChangeEmailNeedsActivation (usr, act, em)

services/spar/test-integration/Test/Spar/Scim/UserSpec.hs

@@ -34,6 +34,7 @@ import Brig.Types.Intra (AccountStatus (Active, PendingInvitation, Suspended), a
 import Brig.Types.User as Brig
 import qualified Control.Exception
 import Control.Lens
+import Control.Monad.Random (Random (randomRIO))
 import Control.Monad.Trans.Except
 import Control.Monad.Trans.Maybe
 import Control.Retry (exponentialBackoff, limitRetries, recovering)
@@ -47,6 +48,7 @@ import Data.Ix (inRange)
 import Data.String.Conversions (cs)
 import Data.Text.Encoding (encodeUtf8)
 import Imports
+import qualified Network.Wai.Utilities.Error as Wai
 import qualified SAML2.WebSSO.Test.MockResponse as SAML
 import qualified SAML2.WebSSO.Types as SAML
 import qualified Spar.Data as Data
@@ -81,6 +83,7 @@ spec = do
   specAzureQuirks
   specEmailValidation
   specSuspend
+  specSCIMManaged
   describe "CRUD operations maintain invariants in mapScimToBrig, mapBrigToScim." $ do
     it "..." $ do
       pendingWith "this is a job for quickcheck-state-machine"
@@ -193,7 +196,7 @@ specCreateUser = describe "POST /Users" $ do
   context "team has one SAML IdP" $ do
     it "creates a user in an existing team" $ do
       testCreateUserWithSamlIdP
-  it "adds a Wire scheme to the user record" $ testSchemaIsAdded
+    it "adds a Wire scheme to the user record" $ testSchemaIsAdded
   it "requires externalId to be present" $ testExternalIdIsRequired
   it "rejects invalid handle" $ testCreateRejectsInvalidHandle
   it "rejects occupied handle" $ testCreateRejectsTakenHandle
@@ -1594,3 +1597,42 @@ specEmailValidation = do
     context "not enabled in team" . it "does not give user email" $ do
       (uid, _) <- setup False
       eventually $ checkEmail uid Nothing
+
+specSCIMManaged :: SpecWith TestEnv
+specSCIMManaged = do
+  describe "SCIM-managed users" $ do
+    it "cannot manually update their email, handle or name" $ do
+      env <- ask
+      let brig = env ^. teBrig
+
+      (tok, _) <- registerIdPAndScimToken
+      user <- randomScimUser
+      storedUser <- createUser tok user
+      let uid = Scim.id . Scim.thing $ storedUser
+
+      do
+        email <- randomEmail
+        call $
+          changeEmailBrig brig uid email !!! do
+            (fmap Wai.label . responseJsonEither @Wai.Error) === const (Right "managed-by-scim")
+            statusCode === const 403
+
+      do
+        handleTxt <- randomAlphaNum
+        call $
+          changeHandleBrig brig uid handleTxt !!! do
+            (fmap Wai.label . responseJsonEither @Wai.Error) === const (Right "managed-by-scim")
+            statusCode === const 403
+
+      do
+        displayName <- Name <$> randomAlphaNum
+        let uupd = UserUpdate (Just displayName) Nothing Nothing Nothing
+        call $
+          updateProfileBrig brig uid uupd !!! do
+            (fmap Wai.label . responseJsonEither @Wai.Error) === const (Right "managed-by-scim")
+            statusCode === const 403
+  where
+    randomAlphaNum :: MonadIO m => m Text
+    randomAlphaNum = liftIO $ do
+      nrs <- replicateM 21 (randomRIO (97, 122)) -- a-z
+      return (cs (map chr nrs))

services/spar/test-integration/Util/Core.hs

@@ -52,6 +52,8 @@ module Util.Core
     randomEmail,
     defPassword,
     getUserBrig,
+    changeHandleBrig,
+    updateProfileBrig,
     createUserWithTeam,
     createUserWithTeamDisableSSO,
     getSSOEnabledInternal,
@@ -71,6 +73,7 @@ module Util.Core
     nextUserRef,
     createRandomPhoneUser,
     zUser,
+    zConn,
     ping,
     makeTestIdP,
     getTestSPMetadata,
@@ -192,6 +195,7 @@ import qualified Web.Cookie as Web
 import Wire.API.Team.Feature (TeamFeatureStatusValue (..))
 import qualified Wire.API.Team.Feature as Public
 import qualified Wire.API.Team.Invitation as TeamInvitation
+import Wire.API.User (HandleUpdate (HandleUpdate), UserUpdate)
 import qualified Wire.API.User as User
 
 -- | Call 'mkEnv' with options from config files.
@@ -1203,3 +1207,35 @@ stdInvitationRequest = stdInvitationRequest' Nothing Nothing
 stdInvitationRequest' :: Maybe User.Locale -> Maybe Galley.Role -> User.Email -> TeamInvitation.InvitationRequest
 stdInvitationRequest' loc role email =
   TeamInvitation.InvitationRequest loc role Nothing email Nothing
+
+changeHandleBrig ::
+  (MonadCatch m, MonadIO m, MonadHttp m, HasCallStack) =>
+  BrigReq ->
+  UserId ->
+  Text ->
+  m ResponseLBS
+changeHandleBrig brig uid handlTxt = do
+  put
+    ( brig
+        . path "/self/handle"
+        . zUser uid
+        . zConn "user"
+        . contentJson
+        . json (HandleUpdate handlTxt)
+    )
+
+updateProfileBrig ::
+  (MonadCatch m, MonadIO m, MonadHttp m, HasCallStack) =>
+  BrigReq ->
+  UserId ->
+  UserUpdate ->
+  m ResponseLBS
+updateProfileBrig brig uid uupd =
+  put
+    ( brig
+        . path "/self"
+        . zUser uid
+        . zConn "user"
+        . contentJson
+        . json uupd
+    )