Merge branch 'ee-jej/refactor-omniauth-controller' into 'master'

[EE] Refactor OmniauthCallbacksController to remove duplication

See merge request gitlab-org/gitlab-ee!5416

app/controllers/concerns/authenticates_with_two_factor.rb

@@ -23,6 +23,9 @@ module AuthenticatesWithTwoFactor
   #
   # Returns nil
   def prompt_for_two_factor(user)
+    # Set @user for Devise views
+    @user = user # rubocop:disable Gitlab/ModuleWithInstanceVariables
+
     return locked_user_redirect(user) unless user.can?(:log_in)
 
     session[:otp_user_id] = user.id

app/controllers/ldap/omniauth_callbacks_controller.rb

+class Ldap::OmniauthCallbacksController < OmniauthCallbacksController
+  extend ::Gitlab::Utils::Override
+  prepend EE::OmniauthCallbacksController
+  prepend EE::Ldap::OmniauthCallbacksController
+
+  def self.define_providers!
+    return unless Gitlab::Auth::LDAP::Config.enabled?
+
+    Gitlab::Auth::LDAP::Config.available_servers.each do |server|
+      alias_method server['provider_name'], :ldap
+    end
+  end
+
+  # We only find ourselves here
+  # if the authentication to LDAP was successful.
+  def ldap
+    sign_in_user_flow(Gitlab::Auth::LDAP::User)
+  end
+
+  define_providers!
+
+  override :set_remember_me
+  def set_remember_me(user)
+    user.remember_me = params[:remember_me] if user.persisted?
+  end
+
+  override :fail_login
+  def fail_login(user)
+    flash[:alert] = 'Access denied for your LDAP account.'
+
+    redirect_to new_user_session_path
+  end
+end

app/controllers/omniauth_callbacks_controller.rb

@@ -5,18 +5,12 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
 
   protect_from_forgery except: [:kerberos, :saml, :cas3]
 
-  Gitlab.config.omniauth.providers.each do |provider|
-    define_method provider['name'] do
-      handle_omniauth
-    end
+  def handle_omniauth
+    omniauth_flow(Gitlab::Auth::OAuth)
   end
 
-  if Gitlab::Auth::LDAP::Config.enabled?
-    Gitlab::Auth::LDAP::Config.available_servers.each do |server|
-      define_method server['provider_name'] do
-        ldap
-      end
-    end
+  Gitlab.config.omniauth.providers.each do |provider|
+    alias_method provider['name'], :handle_omniauth
   end
 
   # Extend the standard implementation to also increment
@@ -38,53 +32,12 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     error ||= exception.error        if exception.respond_to?(:error)
     error ||= exception.message      if exception.respond_to?(:message)
     error ||= env["omniauth.error.type"].to_s
-    error.to_s.humanize if error
-  end
-
-  # We only find ourselves here
-  # if the authentication to LDAP was successful.
-  def ldap
-    ldap_user = Gitlab::Auth::LDAP::User.new(oauth)
-    ldap_user.save if ldap_user.changed? # will also save new users
 
-    @user = ldap_user.gl_user
-    @user.remember_me = params[:remember_me] if ldap_user.persisted?
-
-    # Do additional LDAP checks for the user filter and EE features
-    if ldap_user.allowed?
-      if @user.two_factor_enabled?
-        prompt_for_two_factor(@user)
-      else
-        log_audit_event(@user, with: oauth['provider'])
-        # The counter only gets incremented in `sign_in_and_redirect`
-        show_ldap_sync_flash if @user.sign_in_count == 0
-        sign_in_and_redirect(@user)
-      end
-    else
-      fail_ldap_login
-    end
+    error.to_s.humanize if error
   end
 
   def saml
-    if current_user
-      log_audit_event(current_user, with: :saml)
-      # Update SAML identity if data has changed.
-      identity = current_user.identities.with_extern_uid(:saml, oauth['uid']).take
-      if identity.nil?
-        current_user.identities.create(extern_uid: oauth['uid'], provider: :saml)
-        redirect_to profile_account_path, notice: 'Authentication method updated'
-      else
-        redirect_to after_sign_in_path_for(current_user)
-      end
-    else
-      saml_user = Gitlab::Auth::Saml::User.new(oauth)
-      saml_user.save if saml_user.changed?
-      @user = saml_user.gl_user
-
-      continue_login_process
-    end
-  rescue Gitlab::Auth::OAuth::User::SignupDisabledError
-    handle_signup_error
+    omniauth_flow(Gitlab::Auth::Saml)
   end
 
   def omniauth_error
@@ -130,25 +83,36 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
 
   private
 
-  def handle_omniauth
+  def omniauth_flow(auth_module, identity_linker: nil)
     if current_user
-      # Add new authentication method
-      current_user.identities
-                  .with_extern_uid(oauth['provider'], oauth['uid'])
-                  .first_or_create(extern_uid: oauth['uid'])
       log_audit_event(current_user, with: oauth['provider'])
-      redirect_to profile_account_path, notice: 'Authentication method updated'
+
+      identity_linker ||= auth_module::IdentityLinker.new(current_user, oauth)
+
+      identity_linker.link
+
+      if identity_linker.changed?
+        redirect_identity_linked
+      elsif identity_linker.error_message.present?
+        redirect_identity_link_failed(identity_linker.error_message)
       else
-      oauth_user = Gitlab::Auth::OAuth::User.new(oauth)
-      oauth_user.save
-      @user = oauth_user.gl_user
+        redirect_identity_exists
+      end
+    else
+      sign_in_user_flow(auth_module::User)
+    end
+  end
 
-      continue_login_process
+  def redirect_identity_exists
+    redirect_to after_sign_in_path_for(current_user)
   end
-  rescue Gitlab::Auth::OAuth::User::SigninDisabledForProviderError
-    handle_disabled_provider
-  rescue Gitlab::Auth::OAuth::User::SignupDisabledError
-    handle_signup_error
+
+  def redirect_identity_link_failed(error_message)
+    redirect_to profile_account_path, notice: "Authentication failed: #{error_message}"
+  end
+
+  def redirect_identity_linked
+    redirect_to profile_account_path, notice: 'Authentication method updated'
   end
 
   def handle_service_ticket(provider, ticket)
@@ -157,21 +121,27 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     session[:service_tickets][provider] = ticket
   end
 
-  def continue_login_process
-    # Only allow properly saved users to login.
-    if @user.persisted? && @user.valid?
-      log_audit_event(@user, with: oauth['provider'])
+  def sign_in_user_flow(auth_user_class)
+    auth_user = auth_user_class.new(oauth)
+    user = auth_user.find_and_update!
+
+    if auth_user.valid_sign_in?
+      log_audit_event(user, with: oauth['provider'])
+
+      set_remember_me(user)
 
-      if @user.two_factor_enabled?
-        params[:remember_me] = '1' if remember_me?
-        prompt_for_two_factor(@user)
+      if user.two_factor_enabled?
+        prompt_for_two_factor(user)
       else
-        remember_me(@user) if remember_me?
-        sign_in_and_redirect(@user)
+        sign_in_and_redirect(user)
       end
     else
-      fail_login
+      fail_login(user)
     end
+  rescue Gitlab::Auth::OAuth::User::SigninDisabledForProviderError
+    handle_disabled_provider
+  rescue Gitlab::Auth::OAuth::User::SignupDisabledError
+    handle_signup_error
   end
 
   def handle_signup_error
@@ -191,18 +161,12 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     @oauth ||= request.env['omniauth.auth']
   end
 
-  def fail_login
-    error_message = @user.errors.full_messages.to_sentence
+  def fail_login(user)
+    error_message = user.errors.full_messages.to_sentence
 
     return redirect_to omniauth_error_path(oauth['provider'], error: error_message)
   end
 
-  def fail_ldap_login
-    flash[:alert] = 'Access denied for your LDAP account.'
-
-    redirect_to new_user_session_path
-  end
-
   def fail_auth0_login
     flash[:alert] = 'Wrong extern UID provided. Make sure Auth0 is configured correctly.'
 
@@ -221,13 +185,18 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
       .for_authentication.security_event
   end
 
+  def set_remember_me(user)
+    return unless remember_me?
+
+    if user.two_factor_enabled?
+      params[:remember_me] = '1'
+    else
+      remember_me(user)
+    end
+  end
+
   def remember_me?
     request_params = request.env['omniauth.params']
     (request_params['remember_me'] == '1') if request_params.present?
   end
-
-  def show_ldap_sync_flash
-    flash[:notice] = 'LDAP sync in progress. This could take a few minutes. '\
-                     'Refresh the page to see the changes.'
-  end
 end

config/routes/user.rb

@@ -3,6 +3,24 @@ get  'unsubscribes/:email', to: 'unsubscribes#show', as: :unsubscribe
 post 'unsubscribes/:email', to: 'unsubscribes#create'
 ## EE-specific
 
+# Allows individual providers to be directed to a chosen controller
+# Call from inside devise_scope
+def override_omniauth(provider, controller, path_prefix = '/users/auth')
+  match "#{path_prefix}/#{provider}/callback",
+    to: "#{controller}##{provider}",
+    as: "#{provider}_omniauth_callback",
+    via: [:get, :post]
+end
+
+# Use custom controller for LDAP omniauth callback
+if Gitlab::Auth::LDAP::Config.enabled?
+  devise_scope :user do
+    Gitlab::Auth::LDAP::Config.available_servers.each do |server|
+      override_omniauth(server['provider_name'], 'ldap/omniauth_callbacks')
+    end
+  end
+end
+
 devise_for :users, controllers: { omniauth_callbacks: :omniauth_callbacks,
                                   registrations: :registrations,
                                   passwords: :passwords,

ee/app/controllers/ee/ldap/omniauth_callbacks_controller.rb

+module EE
+  module Ldap
+    module OmniauthCallbacksController
+      extend ::Gitlab::Utils::Override
+
+      override :sign_in_and_redirect
+      def sign_in_and_redirect(user)
+        # The counter gets incremented in `sign_in_and_redirect`
+        show_ldap_sync_flash if user.sign_in_count == 0
+
+        super
+      end
+
+      private
+
+      def show_ldap_sync_flash
+        flash[:notice] = 'LDAP sync in progress. This could take a few minutes. '\
+                         'Refresh the page to see the changes.'
+      end
+    end
+  end
+end

ee/app/controllers/ee/omniauth_callbacks_controller.rb

 module EE
   module OmniauthCallbacksController
+    extend ::Gitlab::Utils::Override
+
     protected
 
-    def fail_login
-      log_failed_login(@user.username, oauth['provider']) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+    override :fail_login
+    def fail_login(user)
+      log_failed_login(user.username, oauth['provider'])
 
       super
     end
 
-    alias_method :fail_ldap_login, :fail_login
-
     private
 
     def log_failed_login(author, provider)

ee/spec/controllers/ldap/omniauth_callbacks_controller_spec.rb

+require 'spec_helper'
+
+describe Ldap::OmniauthCallbacksController do
+  include_context 'Ldap::OmniauthCallbacksController'
+
+  it "displays LDAP sync flash on first sign in" do
+    post provider
+
+    expect(flash[:notice]).to match(/LDAP sync in progress*/)
+  end
+
+  it "skips LDAP sync flash on subsequent sign ins" do
+    user.update!(sign_in_count: 1)
+
+    post provider
+
+    expect(flash[:notice]).to eq nil
+  end
+
+  context 'access denied' do
+    let(:valid_login?) { false }
+
+    it 'logs a failure event' do
+      stub_licensed_features(extended_audit_events: true)
+
+      expect { post provider }.to change(SecurityEvent, :count).by(1)
+    end
+  end
+end

lib/gitlab/auth/ldap/config.rb

@@ -141,7 +141,7 @@ module Gitlab
         end
 
         def external_groups
-          options['external_groups']
+          options['external_groups'] || []
         end
 
         def has_auth?

lib/gitlab/auth/ldap/user.rb

@@ -8,6 +8,7 @@ module Gitlab
   module Auth
     module LDAP
       class User < Gitlab::Auth::OAuth::User
+        extend ::Gitlab::Utils::Override
         prepend ::EE::Gitlab::Auth::LDAP::User
 
         class << self
@@ -31,7 +32,8 @@ module Gitlab
           self.class.find_by_uid_and_provider(auth_hash.uid, auth_hash.provider)
         end
 
-        def changed?
+        override :should_save?
+        def should_save?
           gl_user.changed? || gl_user.identities.any?(&:changed?)
         end
 
@@ -43,6 +45,10 @@ module Gitlab
           Gitlab::Auth::LDAP::Access.allowed?(gl_user)
         end
 
+        def valid_sign_in?
+          allowed? && super
+        end
+
         def ldap_config
           Gitlab::Auth::LDAP::Config.new(auth_hash.provider)
         end

lib/gitlab/auth/o_auth/identity_linker.rb

+module Gitlab
+  module Auth
+    module OAuth
+      class IdentityLinker < OmniauthIdentityLinkerBase
+      end
+    end
+  end
+end

lib/gitlab/auth/o_auth/user.rb

@@ -32,6 +32,10 @@ module Gitlab
           gl_user.try(:valid?)
         end
 
+        def valid_sign_in?
+          valid? && persisted?
+        end
+
         def save(provider = 'OAuth')
           raise SigninDisabledForProviderError if oauth_provider_disabled?
           raise SignupDisabledError unless gl_user
@@ -66,8 +70,18 @@ module Gitlab
           user
         end
 
+        def find_and_update!
+          save if should_save?
+
+          gl_user
+        end
+
         protected
 
+        def should_save?
+          true
+        end
+
         def add_or_update_user_identities
           return unless gl_user
 

lib/gitlab/auth/omniauth_identity_linker_base.rb

+module Gitlab
+  module Auth
+    class OmniauthIdentityLinkerBase
+      attr_reader :current_user, :oauth
+
+      def initialize(current_user, oauth)
+        @current_user = current_user
+        @oauth = oauth
+        @changed = false
+      end
+
+      def link
+        save if identity.new_record?
+      end
+
+      def changed?
+        @changed
+      end
+
+      def error_message
+        identity.validate
+
+        identity.errors.full_messages.join(', ')
+      end
+
+      private
+
+      def save
+        @changed = identity.save
+      end
+
+      def identity
+        @identity ||= current_user.identities
+                                  .with_extern_uid(provider, uid)
+                                  .first_or_initialize(extern_uid: uid)
+      end
+
+      def provider
+        oauth['provider']
+      end
+
+      def uid
+        oauth['uid']
+      end
+    end
+  end
+end

lib/gitlab/auth/saml/identity_linker.rb

+module Gitlab
+  module Auth
+    module Saml
+      class IdentityLinker < OmniauthIdentityLinkerBase
+      end
+    end
+  end
+end

lib/gitlab/auth/saml/user.rb

@@ -7,6 +7,8 @@ module Gitlab
   module Auth
     module Saml
       class User < Gitlab::Auth::OAuth::User
+        extend ::Gitlab::Utils::Override
+
         def save
           super('SAML')
         end
@@ -27,14 +29,15 @@ module Gitlab
           end
 
           if user
-            user.external = !(auth_hash.groups & Gitlab::Auth::Saml::Config.external_groups).empty? if external_users_enabled?
-            user.admin = !(auth_hash.groups & Gitlab::Auth::Saml::Config.admin_groups).empty? if admin_groups_enabled?
+            user.external = !(auth_hash.groups & saml_config.external_groups).empty? if external_users_enabled?
+            user.admin = !(auth_hash.groups & saml_config.admin_groups).empty? if admin_groups_enabled?
           end
 
           user
         end
 
-        def changed?
+        override :should_save?
+        def should_save?
           return true unless gl_user
 
           gl_user.changed? || gl_user.identities.any?(&:changed?)
@@ -42,6 +45,10 @@ module Gitlab
 
         protected
 
+        def saml_config
+          Gitlab::Auth::Saml::Config
+        end
+
         def block_user(user, reason)
           user.ldap_block
           log_user_changes(user, "#{reason}, blocking")
@@ -60,7 +67,7 @@ module Gitlab
         end
 
         def user_in_required_group?
-          required_groups = Gitlab::Auth::Saml::Config.required_groups
+          required_groups = saml_config.required_groups
           required_groups.empty? || !(auth_hash.groups & required_groups).empty?
         end
 
@@ -69,7 +76,7 @@ module Gitlab
         end
 
         def external_users_enabled?
-          !Gitlab::Auth::Saml::Config.external_groups.nil?
+          !saml_config.external_groups.nil?
         end
 
         def auth_hash=(auth_hash)
@@ -77,7 +84,7 @@ module Gitlab
         end
 
         def admin_groups_enabled?
-          !Gitlab::Auth::Saml::Config.admin_groups.nil?
+          !saml_config.admin_groups.nil?
         end
       end
     end

spec/controllers/ldap/omniauth_callbacks_controller_spec.rb

+require 'spec_helper'
+
+describe Ldap::OmniauthCallbacksController do
+  include_context 'Ldap::OmniauthCallbacksController'
+
+  it 'allows sign in' do
+    post provider
+
+    expect(request.env['warden']).to be_authenticated
+  end
+
+  it 'respects remember me checkbox' do
+    expect do
+      post provider, remember_me: '1'
+    end.to change { user.reload.remember_created_at }.from(nil)
+  end
+
+  context 'with 2FA' do
+    let(:user) { create(:omniauth_user, :two_factor_via_otp, extern_uid: uid, provider: provider) }
+
+    it 'passes remember_me to the Devise view' do
+      post provider, remember_me: '1'
+
+      expect(assigns[:user].remember_me).to eq '1'
+    end
+  end
+
+  context 'access denied' do
+    let(:valid_login?) { false }
+
+    it 'warns the user' do
+      post provider
+
+      expect(flash[:alert]).to match(/Access denied for your LDAP account*/)
+    end
+
+    it "doesn't authenticate user" do
+      post provider
+
+      expect(request.env['warden']).not_to be_authenticated
+      expect(response).to redirect_to(new_user_session_path)
+    end
+  end
+
+  context 'sign up' do
+    let(:user) { double(email: 'new@example.com') }
+
+    before do
+      stub_omniauth_setting(block_auto_created_users: false)
+    end
+
+    it 'is allowed' do
+      post provider
+
+      expect(request.env['warden']).to be_authenticated
+    end
+  end
+end

spec/features/oauth_login_spec.rb

@@ -28,35 +28,46 @@ feature 'OAuth Login', :js, :allow_forgery_protection do
     OmniAuth.config.full_host = @omniauth_config_full_host
   end
 
+  def login_with_provider(provider, enter_two_factor: false)
+    login_via(provider.to_s, user, uid, remember_me: remember_me)
+    enter_code(user.current_otp) if enter_two_factor
+  end
+
   providers.each do |provider|
     context "when the user logs in using the #{provider} provider" do
+      let(:uid) { 'my-uid' }
+      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) }
+
+      before do
+        stub_omniauth_config(provider)
+      end
+
       context 'when two-factor authentication is disabled' do
         it 'logs the user in' do
-          stub_omniauth_config(provider)
-          user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s)
-          login_via(provider.to_s, user, 'my-uid')
+          login_with_provider(provider)
 
           expect(current_path).to eq root_path
         end
       end
 
       context 'when two-factor authentication is enabled' do
+        let(:user) { two_factor_user }
+
         it 'logs the user in' do
-          stub_omniauth_config(provider)
-          user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s)
-          login_via(provider.to_s, user, 'my-uid')
+          login_with_provider(provider, enter_two_factor: true)
 
-          enter_code(user.current_otp)
           expect(current_path).to eq root_path
         end
       end
 
       context 'when "remember me" is checked' do
+        let(:remember_me) { true }
+
         context 'when two-factor authentication is disabled' do
           it 'remembers the user after a browser restart' do
-            stub_omniauth_config(provider)
-            user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s)
-            login_via(provider.to_s, user, 'my-uid', remember_me: true)
+            login_with_provider(provider)
 
             clear_browser_session
 
@@ -66,11 +77,10 @@ feature 'OAuth Login', :js, :allow_forgery_protection do
         end
 
         context 'when two-factor authentication is enabled' do
+          let(:user) { two_factor_user }
+
           it 'remembers the user after a browser restart' do
-            stub_omniauth_config(provider)
-            user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s)
-            login_via(provider.to_s, user, 'my-uid', remember_me: true)
-            enter_code(user.current_otp)
+            login_with_provider(provider, enter_two_factor: true)
 
             clear_browser_session
 
@@ -83,9 +93,7 @@ feature '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
-            stub_omniauth_config(provider)
-            user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s)
-            login_via(provider.to_s, user, 'my-uid', remember_me: false)
+            login_with_provider(provider)
 
             clear_browser_session
 
@@ -95,11 +103,10 @@ feature 'OAuth Login', :js, :allow_forgery_protection do
         end
 
         context 'when two-factor authentication is enabled' do
+          let(:user) { two_factor_user }
+
           it 'does not remember the user after a browser restart' do
-            stub_omniauth_config(provider)
-            user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s)
-            login_via(provider.to_s, user, 'my-uid', remember_me: false)
-            enter_code(user.current_otp)
+            login_with_provider(provider, enter_two_factor: true)
 
             clear_browser_session
 

spec/lib/gitlab/auth/ldap/user_spec.rb

@@ -27,20 +27,20 @@ describe Gitlab::Auth::LDAP::User do
     OmniAuth::AuthHash.new(uid: 'uid=John Smith,ou=People,dc=example,dc=com', provider: 'ldapmain', info: info_upper_case)
   end
 
-  describe '#changed?' do
+  describe '#should_save?' do
     it "marks existing ldap user as changed" do
       create(:omniauth_user, extern_uid: 'uid=John Smith,ou=People,dc=example,dc=com', provider: 'ldapmain')
-      expect(ldap_user.changed?).to be_truthy
+      expect(ldap_user.should_save?).to be_truthy
     end
 
     it "marks existing non-ldap user if the email matches as changed" do
       create(:user, email: 'john@example.com')
-      expect(ldap_user.changed?).to be_truthy
+      expect(ldap_user.should_save?).to be_truthy
     end
 
     it "does not mark existing ldap user as changed" do
       create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=john smith,ou=people,dc=example,dc=com', provider: 'ldapmain')
-      expect(ldap_user.changed?).to be_falsey
+      expect(ldap_user.should_save?).to be_falsey
     end
   end
 

spec/lib/gitlab/auth/o_auth/identity_linker_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Auth::OAuth::IdentityLinker do
+  let(:user) { create(:user) }
+  let(:provider) { 'twitter' }
+  let(:uid) { user.email }
+  let(:oauth) { { 'provider' => provider, 'uid' => uid } }
+
+  subject { described_class.new(user, oauth) }
+
+  context 'linked identity exists' do
+    let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) }
+
+    it "doesn't create new identity" do
+      expect { subject.link }.not_to change { Identity.count }
+    end
+
+    it "sets #changed? to false" do
+      subject.link
+
+      expect(subject).not_to be_changed
+    end
+  end
+
+  context 'identity already linked to different user' do
+    let!(:identity) {  create(:identity, provider: provider, extern_uid: uid) }
+
+    it "#changed? returns false" do
+      subject.link
+
+      expect(subject).not_to be_changed
+    end
+
+    it 'exposes error message' do
+      expect(subject.error_message).to eq 'Extern uid has already been taken'
+    end
+  end
+
+  context 'identity needs to be created' do
+    it 'creates linked identity' do
+      expect { subject.link }.to change { user.identities.count }
+    end
+
+    it 'sets identity provider' do
+      subject.link
+
+      expect(user.identities.last.provider).to eq provider
+    end
+
+    it 'sets identity extern_uid' do
+      subject.link
+
+      expect(user.identities.last.extern_uid).to eq uid
+    end
+
+    it 'sets #changed? to true' do
+      subject.link
+
+      expect(subject).to be_changed
+    end
+  end
+end

spec/lib/gitlab/auth/saml/identity_linker_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Auth::Saml::IdentityLinker do
+  let(:user) { create(:user) }
+  let(:provider) { 'saml' }
+  let(:uid) { user.email }
+  let(:oauth) { { 'provider' => provider, 'uid' => uid } }
+
+  subject { described_class.new(user, oauth) }
+
+  context 'linked identity exists' do
+    let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) }
+
+    it "doesn't create new identity" do
+      expect { subject.link }.not_to change { Identity.count }
+    end
+
+    it "sets #changed? to false" do
+      subject.link
+
+      expect(subject).not_to be_changed
+    end
+  end
+
+  context 'identity needs to be created' do
+    it 'creates linked identity' do
+      expect { subject.link }.to change { user.identities.count }
+    end
+
+    it 'sets identity provider' do
+      subject.link
+
+      expect(user.identities.last.provider).to eq provider
+    end
+
+    it 'sets identity extern_uid' do
+      subject.link
+
+      expect(user.identities.last.extern_uid).to eq uid
+    end
+
+    it 'sets #changed? to true' do
+      subject.link
+
+      expect(subject).to be_changed
+    end
+  end
+end

spec/lib/gitlab/auth/saml/user_spec.rb

@@ -24,10 +24,6 @@ describe Gitlab::Auth::Saml::User do
       allow(Gitlab.config.omniauth).to receive_messages(messages)
     end
 
-    def stub_ldap_config(messages)
-      allow(Gitlab::Auth::LDAP::Config).to receive_messages(messages)
-    end
-
     def stub_basic_saml_config
       allow(Gitlab::Auth::Saml::Config).to receive_messages({ options: { name: 'saml', args: {} } })
     end

spec/support/controllers/ldap_omniauth_callbacks_controller_shared_context.rb

+require 'spec_helper'
+
+shared_context 'Ldap::OmniauthCallbacksController' do
+  include LoginHelpers
+  include LdapHelpers
+
+  let(:uid) { 'my-uid' }
+  let(:provider) { 'ldapmain' }
+  let(:valid_login?) { true }
+  let(:user) { create(:omniauth_user, extern_uid: uid, provider: provider) }
+  let(:ldap_server_config) do
+    { main: ldap_config_defaults(:main) }
+  end
+
+  def ldap_config_defaults(key, hash = {})
+    {
+      provider_name: "ldap#{key}",
+      attributes: {},
+      encryption: 'plain'
+    }.merge(hash)
+  end
+
+  before do
+    stub_ldap_setting(enabled: true, servers: ldap_server_config)
+    described_class.define_providers!
+    Rails.application.reload_routes!
+
+    mock_auth_hash(provider.to_s, uid, user.email)
+    stub_omniauth_provider(provider, context: request)
+
+    allow(Gitlab::Auth::LDAP::Access).to receive(:allowed?).and_return(valid_login?)
+  end
+end

spec/support/helpers/ldap_helpers.rb

@@ -26,6 +26,10 @@ module LdapHelpers
     allow_any_instance_of(::Gitlab::Auth::LDAP::Config).to receive_messages(messages)
   end
 
+  def stub_ldap_setting(messages)
+    allow(Gitlab.config.ldap).to receive_messages(to_settings(messages))
+  end
+
   # Stub an LDAP person search and provide the return entry. Specify `nil` for
   # `entry` to simulate when an LDAP person is not found
   #

spec/support/helpers/login_helpers.rb

@@ -112,7 +112,7 @@ module LoginHelpers
         }
       }
     })
-    Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[:saml]
+    Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[provider.to_sym]
   end
 
   def mock_saml_config
@@ -129,7 +129,7 @@ module LoginHelpers
     env = env_from_context(context)
 
     set_devise_mapping(context: context)
-    env['omniauth.auth'] = OmniAuth.config.mock_auth[provider]
+    env['omniauth.auth'] = OmniAuth.config.mock_auth[provider.to_sym]
   end
 
   def stub_omniauth_saml_config(messages)