Fix saml corner case

changelog.d/3-bug-fixes/fix-saml-corner-case

@@ -0,0 +1 @@
+saml-auto-provisioned users don't get to change email address

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

@@ -607,32 +607,42 @@ changeSelfEmail u email allowScim = do
 -- | Prepare changing the email (checking a number of invariants).
 changeEmail :: UserId -> Email -> AllowSCIMUpdates -> ExceptT ChangeEmailError (AppT r) ChangeEmailResult
 changeEmail u email allowScim = do
-  em <-
-    either
-      (throwE . InvalidNewEmail email)
-      pure
-      (validateEmail email)
-  let ek = userEmailKey em
-  blacklisted <- lift . wrapClient $ Blacklist.exists ek
+  eml <- either (throwE . InvalidNewEmail email) pure (validateEmail email)
+  let eky = userEmailKey eml
+
+  blacklisted <- lift . wrapClient $ Blacklist.exists eky
   when blacklisted $
     throwE (ChangeBlacklistedEmail email)
-  available <- lift . wrapClient $ Data.keyAvailable ek (Just u)
+
+  available <- lift . wrapClient $ Data.keyAvailable eky (Just u)
   unless available $
-    throwE $
-      EmailExists email
-  usr <- maybe (throwM $ UserProfileNotFound u) pure =<< lift (wrapClient $ Data.lookupUser WithPendingInvitations u)
-  case emailIdentity =<< userIdentity usr of
-    -- The user already has an email address and the new one is exactly the same
-    Just current | current == em -> pure ChangeEmailIdempotent
-    _ -> do
-      unless
-        ( userManagedBy usr /= ManagedByScim
-            || allowScim == AllowSCIMUpdates
-        )
-        $ throwE EmailManagedByScim
+    throwE (EmailExists email)
+
+  usr <-
+    maybe (throwM $ UserProfileNotFound u) pure
+      =<< lift (wrapClient $ Data.lookupUser WithPendingInvitations u)
+
+  if (emailIdentity =<< userIdentity usr) == Just eml
+    then do
+      -- The user already has an email address and the new one is exactly the same
+      pure ChangeEmailIdempotent
+    else do
+      -- case 2: No or different old email address
+      let changeAllowed =
+            ( userManagedBy usr /= ManagedByScim
+                || allowScim == AllowSCIMUpdates -- spar is always allowed to call this function (from the scim handlers)
+            )
+              && not
+                ( -- user is auto-provisioned by saml (deprecated use case)
+                  userManagedBy usr /= ManagedByScim && userHasSAML usr
+                )
+
+      unless changeAllowed $
+        throwE EmailManagedByScim
+
       timeout <- setActivationTimeout <$> view settings
-      act <- lift . wrapClient $ Data.newActivation ek timeout (Just u)
-      pure $ ChangeEmailNeedsActivation (usr, act, em)
+      act <- lift . wrapClient $ Data.newActivation eky timeout (Just u)
+      pure $ ChangeEmailNeedsActivation (usr, act, eml)
 
 -------------------------------------------------------------------------------
 -- Change Phone

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

@@ -31,7 +31,8 @@ module Brig.Data.User
     reauthenticate,
     filterActive,
     isActivated,
-    isSamlUser,
+    userIdHasSAML,
+    userHasSAML,
 
     -- * Lookups
     lookupAccount,
@@ -212,17 +213,20 @@ reauthenticate u pw =
     Just (Just pw', Ephemeral) -> maybeReAuth pw'
   where
     maybeReAuth pw' = case pw of
-      Nothing -> unlessM (isSamlUser u) $ throwE ReAuthMissingPassword
+      Nothing -> unlessM (userIdHasSAML u) $ throwE ReAuthMissingPassword
       Just p ->
         unless (verifyPassword p pw') $
           throwE (ReAuthError AuthInvalidCredentials)
 
-isSamlUser :: (MonadClient m, MonadReader Env m) => UserId -> m Bool
-isSamlUser uid = do
-  account <- lookupAccount uid
-  case userIdentity . accountUser =<< account of
-    Just (SSOIdentity (UserSSOId _) _ _) -> pure True
-    _ -> pure False
+userIdHasSAML :: (MonadClient m, MonadReader Env m) => UserId -> m Bool
+userIdHasSAML uid = do
+  maybe False (userHasSAML . accountUser) <$> lookupAccount uid
+
+userHasSAML :: User -> Bool
+userHasSAML user = do
+  case userIdentity user of
+    Just (SSOIdentity (UserSSOId _) _ _) -> True
+    _ -> False
 
 insertAccount ::
   MonadClient m =>

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

@@ -288,7 +288,7 @@ revokeAccess ::
   ExceptT AuthError m ()
 revokeAccess u pw cc ll = do
   lift $ Log.debug $ field "user" (toByteString u) . field "action" (Log.val "User.revokeAccess")
-  unlessM (Data.isSamlUser u) $ Data.authenticate u pw
+  unlessM (Data.userIdHasSAML u) $ Data.authenticate u pw
   lift $ revokeCookies u cc ll
 
 --------------------------------------------------------------------------------