Commit fe4ffa44 authored by Małgorzata Ksionek's avatar Małgorzata Ksionek

Add checking for email_verified key

Fix rubocop offences and add changelog

Add email_verified key for feature specs

Add code review remarks

Add code review remarks

Fix specs
parent 9735395f
...@@ -73,6 +73,14 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController ...@@ -73,6 +73,14 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
end end
end end
def salesforce
if oauth.dig('extra', 'email_verified')
handle_omniauth
else
fail_salesforce_login
end
end
private private
def omniauth_flow(auth_module, identity_linker: nil) def omniauth_flow(auth_module, identity_linker: nil)
...@@ -173,7 +181,15 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController ...@@ -173,7 +181,15 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
end end
def fail_auth0_login def fail_auth0_login
flash[:alert] = _('Wrong extern UID provided. Make sure Auth0 is configured correctly.') fail_login_with_message(_('Wrong extern UID provided. Make sure Auth0 is configured correctly.'))
end
def fail_salesforce_login
fail_login_with_message(_('Email not verified. Please verify your email in Salesforce.'))
end
def fail_login_with_message(message)
flash[:alert] = message
redirect_to new_user_session_path redirect_to new_user_session_path
end end
......
---
title: Prevent bypassing email verification using Salesforce
merge_request:
author:
type: security
...@@ -5551,6 +5551,9 @@ msgstr "" ...@@ -5551,6 +5551,9 @@ msgstr ""
msgid "Email domain is not editable in subgroups. Value inherited from top-level parent group." msgid "Email domain is not editable in subgroups. Value inherited from top-level parent group."
msgstr "" msgstr ""
msgid "Email not verified. Please verify your email in Salesforce."
msgstr ""
msgid "Email patch" msgid "Email patch"
msgstr "" msgstr ""
......
...@@ -7,9 +7,10 @@ describe OmniauthCallbacksController, type: :controller do ...@@ -7,9 +7,10 @@ describe OmniauthCallbacksController, type: :controller do
describe 'omniauth' do describe 'omniauth' do
let(:user) { create(:omniauth_user, extern_uid: extern_uid, provider: provider) } let(:user) { create(:omniauth_user, extern_uid: extern_uid, provider: provider) }
let(:additional_info) { {} }
before do before do
@original_env_config_omniauth_auth = mock_auth_hash(provider.to_s, +extern_uid, user.email) @original_env_config_omniauth_auth = mock_auth_hash(provider.to_s, extern_uid, user.email, additional_info: additional_info )
stub_omniauth_provider(provider, context: request) stub_omniauth_provider(provider, context: request)
end end
...@@ -109,6 +110,14 @@ describe OmniauthCallbacksController, type: :controller do ...@@ -109,6 +110,14 @@ describe OmniauthCallbacksController, type: :controller do
end end
context 'strategies' do context 'strategies' do
shared_context 'sign_up' do
let(:user) { double(email: 'new@example.com') }
before do
stub_omniauth_setting(block_auto_created_users: false)
end
end
context 'github' do context 'github' do
let(:extern_uid) { 'my-uid' } let(:extern_uid) { 'my-uid' }
let(:provider) { :github } let(:provider) { :github }
...@@ -146,14 +155,6 @@ describe OmniauthCallbacksController, type: :controller do ...@@ -146,14 +155,6 @@ describe OmniauthCallbacksController, type: :controller do
end end
end end
shared_context 'sign_up' do
let(:user) { double(email: +'new@example.com') }
before do
stub_omniauth_setting(block_auto_created_users: false)
end
end
context 'sign up' do context 'sign up' do
include_context 'sign_up' include_context 'sign_up'
...@@ -215,6 +216,33 @@ describe OmniauthCallbacksController, type: :controller do ...@@ -215,6 +216,33 @@ describe OmniauthCallbacksController, type: :controller do
expect(controller).to set_flash[:alert].to('Wrong extern UID provided. Make sure Auth0 is configured correctly.') expect(controller).to set_flash[:alert].to('Wrong extern UID provided. Make sure Auth0 is configured correctly.')
end end
end end
context 'salesforce' do
let(:extern_uid) { 'my-uid' }
let(:provider) { :salesforce }
let(:additional_info) { { extra: { email_verified: false } } }
context 'without verified email' do
it 'does not allow sign in' do
post 'salesforce'
expect(request.env['warden']).not_to be_authenticated
expect(response.status).to eq(302)
expect(controller).to set_flash[:alert].to('Email not verified. Please verify your email in Salesforce.')
end
end
context 'with verified email' do
include_context 'sign_up'
let(:additional_info) { { extra: { email_verified: true } } }
it 'allows sign in' do
post 'salesforce'
expect(request.env['warden']).to be_authenticated
end
end
end
end end
end end
......
...@@ -22,8 +22,8 @@ describe 'OAuth Login', :js, :allow_forgery_protection do ...@@ -22,8 +22,8 @@ describe 'OAuth Login', :js, :allow_forgery_protection do
with_omniauth_full_host { example.run } with_omniauth_full_host { example.run }
end end
def login_with_provider(provider, enter_two_factor: false) def login_with_provider(provider, enter_two_factor: false, additional_info: {})
login_via(provider.to_s, user, uid, remember_me: remember_me) login_via(provider.to_s, user, uid, remember_me: remember_me, additional_info: additional_info)
enter_code(user.current_otp) if enter_two_factor enter_code(user.current_otp) if enter_two_factor
end end
...@@ -33,6 +33,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do ...@@ -33,6 +33,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do
let(:remember_me) { false } let(:remember_me) { false }
let(:user) { create(:omniauth_user, extern_uid: uid, provider: provider.to_s) } let(:user) { create(:omniauth_user, extern_uid: uid, provider: provider.to_s) }
let(:two_factor_user) { create(:omniauth_user, :two_factor, extern_uid: uid, provider: provider.to_s) } let(:two_factor_user) { create(:omniauth_user, :two_factor, extern_uid: uid, provider: provider.to_s) }
provider == :salesforce ? let(:additional_info) { { extra: { email_verified: true } } } : let(:additional_info) { {} }
before do before do
stub_omniauth_config(provider) stub_omniauth_config(provider)
...@@ -41,7 +42,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do ...@@ -41,7 +42,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do
context 'when two-factor authentication is disabled' do context 'when two-factor authentication is disabled' do
it 'logs the user in' do it 'logs the user in' do
login_with_provider(provider) login_with_provider(provider, additional_info: additional_info)
expect(current_path).to eq root_path expect(current_path).to eq root_path
end end
...@@ -51,20 +52,20 @@ describe 'OAuth Login', :js, :allow_forgery_protection do ...@@ -51,20 +52,20 @@ describe 'OAuth Login', :js, :allow_forgery_protection do
let(:user) { two_factor_user } let(:user) { two_factor_user }
it 'logs the user in' do it 'logs the user in' do
login_with_provider(provider, enter_two_factor: true) login_with_provider(provider, additional_info: additional_info, enter_two_factor: true)
expect(current_path).to eq root_path expect(current_path).to eq root_path
end end
it 'when bypass-two-factor is enabled' do it 'when bypass-two-factor is enabled' do
allow(Gitlab.config.omniauth).to receive_messages(allow_bypass_two_factor: true) allow(Gitlab.config.omniauth).to receive_messages(allow_bypass_two_factor: true)
login_via(provider.to_s, user, uid, remember_me: false) login_via(provider.to_s, user, uid, remember_me: false, additional_info: additional_info)
expect(current_path).to eq root_path expect(current_path).to eq root_path
end end
it 'when bypass-two-factor is disabled' do it 'when bypass-two-factor is disabled' do
allow(Gitlab.config.omniauth).to receive_messages(allow_bypass_two_factor: false) allow(Gitlab.config.omniauth).to receive_messages(allow_bypass_two_factor: false)
login_with_provider(provider, enter_two_factor: true) login_with_provider(provider, enter_two_factor: true, additional_info: additional_info)
expect(current_path).to eq root_path expect(current_path).to eq root_path
end end
end end
...@@ -74,7 +75,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do ...@@ -74,7 +75,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do
context 'when two-factor authentication is disabled' do context 'when two-factor authentication is disabled' do
it 'remembers the user after a browser restart' do it 'remembers the user after a browser restart' do
login_with_provider(provider) login_with_provider(provider, additional_info: additional_info)
clear_browser_session clear_browser_session
...@@ -87,7 +88,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do ...@@ -87,7 +88,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do
let(:user) { two_factor_user } let(:user) { two_factor_user }
it 'remembers the user after a browser restart' do it 'remembers the user after a browser restart' do
login_with_provider(provider, enter_two_factor: true) login_with_provider(provider, enter_two_factor: true, additional_info: additional_info)
clear_browser_session clear_browser_session
...@@ -100,7 +101,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do ...@@ -100,7 +101,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do
context 'when "remember me" is not checked' do context 'when "remember me" is not checked' do
context 'when two-factor authentication is disabled' do context 'when two-factor authentication is disabled' do
it 'does not remember the user after a browser restart' do it 'does not remember the user after a browser restart' do
login_with_provider(provider) login_with_provider(provider, additional_info: additional_info)
clear_browser_session clear_browser_session
...@@ -113,7 +114,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do ...@@ -113,7 +114,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do
let(:user) { two_factor_user } let(:user) { two_factor_user }
it 'does not remember the user after a browser restart' do it 'does not remember the user after a browser restart' do
login_with_provider(provider, enter_two_factor: true) login_with_provider(provider, enter_two_factor: true, additional_info: additional_info)
clear_browser_session clear_browser_session
......
...@@ -79,8 +79,8 @@ module LoginHelpers ...@@ -79,8 +79,8 @@ module LoginHelpers
click_button "Sign in" click_button "Sign in"
end end
def login_via(provider, user, uid, remember_me: false) def login_via(provider, user, uid, remember_me: false, additional_info: {})
mock_auth_hash(provider, uid, user.email) mock_auth_hash(provider, uid, user.email, additional_info: additional_info)
visit new_user_session_path visit new_user_session_path
expect(page).to have_content('Sign in with') expect(page).to have_content('Sign in with')
...@@ -99,9 +99,10 @@ module LoginHelpers ...@@ -99,9 +99,10 @@ module LoginHelpers
mock_auth_hash(provider, uid, email, response_object: response_object) mock_auth_hash(provider, uid, email, response_object: response_object)
end end
def configure_mock_auth(provider, uid, email, response_object: nil) def configure_mock_auth(provider, uid, email, response_object: nil, additional_info: {})
# The mock_auth configuration allows you to set per-provider (or default) # The mock_auth configuration allows you to set per-provider (or default)
# authentication hashes to return during integration testing. # authentication hashes to return during integration testing.
OmniAuth.config.mock_auth[provider.to_sym] = OmniAuth::AuthHash.new({ OmniAuth.config.mock_auth[provider.to_sym] = OmniAuth::AuthHash.new({
provider: provider, provider: provider,
uid: uid, uid: uid,
...@@ -124,11 +125,11 @@ module LoginHelpers ...@@ -124,11 +125,11 @@ module LoginHelpers
}, },
response_object: response_object response_object: response_object
} }
}) }).merge(additional_info) { |_, old_hash, new_hash| old_hash.merge(new_hash) }
end end
def mock_auth_hash(provider, uid, email, response_object: nil) def mock_auth_hash(provider, uid, email, additional_info: {}, response_object: nil)
configure_mock_auth(provider, uid, email, response_object: response_object) configure_mock_auth(provider, uid, email, additional_info: additional_info, response_object: response_object)
original_env_config_omniauth_auth = Rails.application.env_config['omniauth.auth'] original_env_config_omniauth_auth = Rails.application.env_config['omniauth.auth']
Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[provider.to_sym] Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[provider.to_sym]
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment