diff options
author | Małgorzata Ksionek <mksionek@gitlab.com> | 2019-09-04 16:18:36 +0300 |
---|---|---|
committer | Małgorzata Ksionek <mksionek@gitlab.com> | 2019-09-11 12:47:13 +0300 |
commit | c8c60560c057e543782f46bbdaa67fe02e61dd6c (patch) | |
tree | 96d044433cf489a9d9fa52a4809c5b130d85fda5 | |
parent | 39381519f294742e4083dfd6a50c0c8ceddecd5d (diff) |
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
-rw-r--r-- | app/controllers/omniauth_callbacks_controller.rb | 18 | ||||
-rw-r--r-- | changelogs/unreleased/security-bypass-email-verification-using-salesforce.yml | 5 | ||||
-rw-r--r-- | locale/gitlab.pot | 3 | ||||
-rw-r--r-- | spec/controllers/omniauth_callbacks_controller_spec.rb | 46 | ||||
-rw-r--r-- | spec/features/oauth_login_spec.rb | 17 | ||||
-rw-r--r-- | spec/support/helpers/login_helpers.rb | 13 |
6 files changed, 78 insertions, 24 deletions
diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index b1efa767154..11dbe12eaab 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -73,6 +73,14 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController end end + def salesforce + if oauth.dig('extra', 'email_verified') + handle_omniauth + else + fail_salesforce_login + end + end + private def omniauth_flow(auth_module, identity_linker: nil) @@ -173,7 +181,15 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController end 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 end diff --git a/changelogs/unreleased/security-bypass-email-verification-using-salesforce.yml b/changelogs/unreleased/security-bypass-email-verification-using-salesforce.yml new file mode 100644 index 00000000000..20b841b68f8 --- /dev/null +++ b/changelogs/unreleased/security-bypass-email-verification-using-salesforce.yml @@ -0,0 +1,5 @@ +--- +title: Prevent bypassing email verification using Salesforce +merge_request: +author: +type: security diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d6a1940e9a7..fcdeb49c84a 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4147,6 +4147,9 @@ msgstr "" msgid "Email" msgstr "" +msgid "Email not verified. Please verify your email in Salesforce." +msgstr "" + msgid "Email patch" msgstr "" diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index 6e374a8daa7..5536b7df64e 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -7,9 +7,10 @@ describe OmniauthCallbacksController, type: :controller do describe 'omniauth' do let(:user) { create(:omniauth_user, extern_uid: extern_uid, provider: provider) } + let(:additional_info) { {} } 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) end @@ -109,6 +110,14 @@ describe OmniauthCallbacksController, type: :controller do end 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 let(:extern_uid) { 'my-uid' } let(:provider) { :github } @@ -146,14 +155,6 @@ describe OmniauthCallbacksController, type: :controller do 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 include_context 'sign_up' @@ -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.') 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 diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index a47eaa9bda7..be9762c254a 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -22,8 +22,8 @@ describe 'OAuth Login', :js, :allow_forgery_protection do with_omniauth_full_host { example.run } end - def login_with_provider(provider, enter_two_factor: false) - login_via(provider.to_s, user, uid, remember_me: remember_me) + def login_with_provider(provider, enter_two_factor: false, additional_info: {}) + login_via(provider.to_s, user, uid, remember_me: remember_me, additional_info: additional_info) enter_code(user.current_otp) if enter_two_factor end @@ -33,6 +33,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do let(:remember_me) { false } 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) } + provider == :salesforce ? let(:additional_info) { { extra: { email_verified: true } } } : let(:additional_info) { {} } before do stub_omniauth_config(provider) @@ -41,7 +42,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do context 'when two-factor authentication is disabled' 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 end @@ -51,7 +52,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do let(:user) { two_factor_user } 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 end @@ -62,7 +63,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do context 'when two-factor authentication is disabled' 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 @@ -75,7 +76,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do let(:user) { two_factor_user } 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 @@ -88,7 +89,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do context 'when "remember me" is not checked' do context 'when two-factor authentication is disabled' 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 @@ -101,7 +102,7 @@ describe 'OAuth Login', :js, :allow_forgery_protection do let(:user) { two_factor_user } 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 diff --git a/spec/support/helpers/login_helpers.rb b/spec/support/helpers/login_helpers.rb index 2b508ee6f2c..ff5ed644329 100644 --- a/spec/support/helpers/login_helpers.rb +++ b/spec/support/helpers/login_helpers.rb @@ -79,8 +79,8 @@ module LoginHelpers click_button "Sign in" end - def login_via(provider, user, uid, remember_me: false) - mock_auth_hash(provider, uid, user.email) + def login_via(provider, user, uid, remember_me: false, additional_info: {}) + mock_auth_hash(provider, uid, user.email, additional_info: additional_info) visit new_user_session_path expect(page).to have_content('Sign in with') @@ -99,9 +99,10 @@ module LoginHelpers mock_auth_hash(provider, uid, email, response_object: response_object) 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) # authentication hashes to return during integration testing. + OmniAuth.config.mock_auth[provider.to_sym] = OmniAuth::AuthHash.new({ provider: provider, uid: uid, @@ -124,11 +125,11 @@ module LoginHelpers }, response_object: response_object } - }) + }).merge(additional_info) { |_, old_hash, new_hash| old_hash.merge(new_hash) } end - def mock_auth_hash(provider, uid, email, response_object: nil) - configure_mock_auth(provider, uid, email, response_object: response_object) + def mock_auth_hash(provider, uid, email, additional_info: {}, response_object: nil) + 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'] Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[provider.to_sym] |