fix: restrict existing user sign-in to account members (#13793)

SAML sign-in now only links an existing user when that user already
belongs to the account that initiated SSO. New users can still be
created for SAML-enabled accounts, and invited members can continue to
sign in through their IdP, but SAML will no longer auto-attach an
unrelated existing user record during login.

**What changed**
- Added an account-membership check before SAML reuses an existing user
by email.
- Kept first-time SAML user creation unchanged for valid new users.
- Added builder and request specs covering the allowed and rejected
login paths.
This commit is contained in:
Shivam Mishra
2026-03-13 12:22:25 +05:30
committed by GitHub
parent b103747584
commit 550b408656
4 changed files with 83 additions and 26 deletions

View File

@@ -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

View File

@@ -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