diff --git a/changelog.d/3-bug-fixes/fix-saml-corner-case b/changelog.d/3-bug-fixes/fix-saml-corner-case new file mode 100644 index 0000000000..00f26d06fd --- /dev/null +++ b/changelog.d/3-bug-fixes/fix-saml-corner-case @@ -0,0 +1 @@ +saml-auto-provisioned users don't get to change email address diff --git a/services/brig/src/Brig/API/User.hs b/services/brig/src/Brig/API/User.hs index 4272fb3cb8..0fc21c80b3 100644 --- a/services/brig/src/Brig/API/User.hs +++ b/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 diff --git a/services/brig/src/Brig/Data/User.hs b/services/brig/src/Brig/Data/User.hs index 10fe01409d..c89811da92 100644 --- a/services/brig/src/Brig/Data/User.hs +++ b/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 => diff --git a/services/brig/src/Brig/User/Auth.hs b/services/brig/src/Brig/User/Auth.hs index 3088a6eb61..b09ec22b83 100644 --- a/services/brig/src/Brig/User/Auth.hs +++ b/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 --------------------------------------------------------------------------------