Merge branch 'security-bypass-email-verification-using-salesforce-12-1' into '12-1-stable'

Prevent Bypassing Email Verification using Salesforce

See merge request gitlab/gitlabhq!3407

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

changelogs/unreleased/security-bypass-email-verification-using-salesforce.yml

+---
+title: Prevent bypassing email verification using Salesforce
+merge_request:
+author:
+type: security

locale/gitlab.pot

@@ -3901,6 +3901,9 @@ msgstr ""
 msgid "Email"
 msgstr ""
 
+msgid "Email not verified. Please verify your email in Salesforce."
+msgstr ""
+
 msgid "Email patch"
 msgstr ""
 

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
 

spec/features/oauth_login_spec.rb

@@ -20,8 +20,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
 
@@ -31,6 +31,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)
@@ -38,7 +39,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
@@ -48,7 +49,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
@@ -59,7 +60,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
 
@@ -72,7 +73,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
 
@@ -85,7 +86,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
 
@@ -98,7 +99,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
 

spec/support/helpers/login_helpers.rb

@@ -77,8 +77,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')
 
@@ -97,9 +97,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,
@@ -122,11 +123,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]