diff --git a/enterprise/app/builders/saml_user_builder.rb b/enterprise/app/builders/saml_user_builder.rb index 97a287462..568225201 100644 --- a/enterprise/app/builders/saml_user_builder.rb +++ b/enterprise/app/builders/saml_user_builder.rb @@ -1,4 +1,6 @@ class SamlUserBuilder + class AuthenticationFailed < StandardError; end + def initialize(auth_hash, account_id) @auth_hash = auth_hash @account_id = account_id @@ -16,13 +18,20 @@ class SamlUserBuilder def find_or_create_user user = User.from_email(auth_attribute('email')) - if user - confirm_user_if_required(user) - convert_existing_user_to_saml(user) - return user - end + return create_user unless user + return existing_user_for_account(user) if user_belongs_to_account?(user) - create_user + raise AuthenticationFailed, I18n.t('auth.saml.authentication_failed') + end + + def existing_user_for_account(user) + confirm_user_if_required(user) + convert_existing_user_to_saml(user) + user + end + + def user_belongs_to_account?(user) + user.account_users.exists?(account_id: @account_id) end def confirm_user_if_required(user) diff --git a/enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb b/enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb index 4856ca443..3dea12888 100644 --- a/enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb +++ b/enterprise/app/controllers/enterprise/devise_overrides/omniauth_callbacks_controller.rb @@ -39,7 +39,7 @@ module Enterprise::DeviseOverrides::OmniauthCallbacksController error = params[:message] || 'authentication-failed' if for_mobile?(relay_state) - redirect_to_mobile_error(error, relay_state) + redirect_to_mobile_error(error) else redirect_to login_page_url(error: "saml-#{error}") end @@ -51,23 +51,15 @@ module Enterprise::DeviseOverrides::OmniauthCallbacksController account_id = extract_saml_account_id relay_state = saml_relay_state - unless saml_enabled_for_account?(account_id) - return redirect_to_mobile_error('saml-not-enabled') if for_mobile?(relay_state) - - return redirect_to login_page_url(error: 'saml-not-enabled') - end + return handle_saml_auth_error(relay_state, 'saml-not-enabled') unless saml_enabled_for_account?(account_id) @resource = SamlUserBuilder.new(auth_hash, account_id).perform - if @resource.persisted? - return sign_in_user_on_mobile if for_mobile?(relay_state) + return sign_in_saml_user(relay_state) if @resource.persisted? - sign_in_user - else - return redirect_to_mobile_error('saml-authentication-failed') if for_mobile?(relay_state) - - redirect_to login_page_url(error: 'saml-authentication-failed') - end + handle_saml_auth_error(relay_state, 'saml-authentication-failed') + rescue SamlUserBuilder::AuthenticationFailed + handle_saml_auth_error(relay_state, 'saml-authentication-failed') end def extract_saml_account_id @@ -82,6 +74,18 @@ module Enterprise::DeviseOverrides::OmniauthCallbacksController relay_state.to_s.casecmp('mobile').zero? end + def sign_in_saml_user(relay_state) + return sign_in_user_on_mobile if for_mobile?(relay_state) + + sign_in_user + end + + def handle_saml_auth_error(relay_state, error) + return redirect_to_mobile_error(error) if for_mobile?(relay_state) + + redirect_to login_page_url(error: error) + end + def redirect_to_mobile_error(error) mobile_deep_link_base = GlobalConfigService.load('MOBILE_DEEP_LINK_BASE', 'chatwootapp') redirect_to "#{mobile_deep_link_base}://auth/saml?error=#{ERB::Util.url_encode(error)}", allow_other_host: true diff --git a/spec/enterprise/builders/saml_user_builder_spec.rb b/spec/enterprise/builders/saml_user_builder_spec.rb index 434beda16..9ebaf3a55 100644 --- a/spec/enterprise/builders/saml_user_builder_spec.rb +++ b/spec/enterprise/builders/saml_user_builder_spec.rb @@ -74,7 +74,7 @@ RSpec.describe SamlUserBuilder do end context 'when user already exists' do - let!(:existing_user) { create(:user, email: email) } + let!(:existing_user) { create(:user, email: email, account: account) } it 'does not create a new user' do expect { builder.perform }.not_to change(User, :count) @@ -85,7 +85,7 @@ RSpec.describe SamlUserBuilder do expect(user).to eq(existing_user) end - it 'adds existing user to the account if not already added' do + it 'keeps the existing user in the account' do user = builder.perform expect(user.accounts).to include(account) end @@ -105,11 +105,38 @@ RSpec.describe SamlUserBuilder do end it 'does not duplicate account association' do - existing_user.account_users.create!(account: account, role: 'agent') - expect { builder.perform }.not_to change(AccountUser, :count) end + context 'when the user does not belong to the target account' do + let!(:other_account) { create(:account) } + let!(:existing_user) { create(:user, email: email, account: other_account) } + + it 'raises an authentication failure' do + expect { builder.perform }.to raise_error do |error| + expect(error.class.name).to eq('SamlUserBuilder::AuthenticationFailed') + expect(error.message).to eq(I18n.t('auth.saml.authentication_failed')) + end + end + + it 'does not add the user to the target account' do + expect do + builder.perform + rescue SamlUserBuilder::AuthenticationFailed + nil + end.not_to change(AccountUser, :count) + expect(existing_user.reload.accounts).not_to include(account) + end + + it 'does not convert the user provider to saml' do + expect do + builder.perform + rescue SamlUserBuilder::AuthenticationFailed + nil + end.not_to(change { existing_user.reload.provider }) + end + end + context 'when user is not confirmed' do let(:unconfirmed_email) { 'unconfirmed_saml_user@example.com' } let(:unconfirmed_auth_hash) do @@ -131,7 +158,7 @@ RSpec.describe SamlUserBuilder do end let(:unconfirmed_builder) { described_class.new(unconfirmed_auth_hash, account.id) } let!(:existing_user) do - user = build(:user, email: unconfirmed_email) + user = build(:user, email: unconfirmed_email, account: account) user.confirmed_at = nil user.save!(validate: false) user @@ -147,7 +174,7 @@ RSpec.describe SamlUserBuilder do end context 'when user is already confirmed' do - let!(:existing_user) { create(:user, email: email, confirmed_at: Time.current) } + let!(:existing_user) { create(:user, email: email, account: account, confirmed_at: Time.current) } it 'keeps already confirmed user confirmed' do expect(existing_user.confirmed?).to be true diff --git a/spec/enterprise/controllers/enterprise/devise_overrides/omniauth_callbacks_controller_spec.rb b/spec/enterprise/controllers/enterprise/devise_overrides/omniauth_callbacks_controller_spec.rb index af67e3009..8eb61fd8f 100644 --- a/spec/enterprise/controllers/enterprise/devise_overrides/omniauth_callbacks_controller_spec.rb +++ b/spec/enterprise/controllers/enterprise/devise_overrides/omniauth_callbacks_controller_spec.rb @@ -57,5 +57,22 @@ RSpec.describe 'Enterprise SAML OmniAuth Callbacks', type: :request do expect(response).to redirect_to(%r{/app/login\?email=.+&sso_auth_token=.+$}) end end + + it 'rejects an existing user from another account' do + with_modified_env FRONTEND_URL: 'http://www.example.com' do + other_account = create(:account) + existing_user = create(:user, email: 'existing@example.com', account: other_account, provider: 'email') + set_saml_config('existing@example.com') + + get "/omniauth/saml/callback?account_id=#{account.id}" + + expect(response).to redirect_to('http://www.example.com/auth/saml/callback') + follow_redirect! + + expect(response).to redirect_to('http://www.example.com/app/login?error=saml-authentication-failed') + expect(existing_user.reload.provider).to eq('email') + expect(existing_user.accounts).not_to include(account) + end + end end end