Refactor Kerberos simple LDAP linking

Refactor to make code more clear and add tests for the
simple LDAP linking feature.

ee/changelogs/unreleased/2906-make-kerberos-mapping-configurable.yml

 ---
-title: Add simple_ldap_linking kerberos options to make the mapping between ldap and
-  kerberos configureable
-merge_request:
+title: Make mapping between LDAP and Kerberos configurable
+merge_request: 9962
 author: Christopher Schenk
 type: added

ee/lib/ee/gitlab/auth/ldap/person.rb

@@ -30,23 +30,23 @@ module EE
             def find_by_kerberos_principal(principal, adapter)
               uid, domain = principal.split('@', 2)
               return unless uid && domain
+              return unless allowed_realm?(domain, adapter)
 
-              if ::Gitlab.config.kerberos.simple_ldap_linking_allowed_realms.blank?
+              find_by_uid(uid, adapter)
+            end
 
-                # In multi-forest setups, there may be several users with matching
-                # uids but differing DNs, so skip adapters configured to connect to
-                # non-matching domains
-                return unless domain.casecmp(domain_from_dn(adapter.config.base)) == 0
+            def allowed_realm?(domain, adapter)
+              return domain.casecmp(domain_from_dn(adapter.config.base)) == 0 unless simple_ldap_linking?
 
-                find_by_uid(uid, adapter)
+              simple_ldap_linking_allowed_realms.select { |realm| domain.casecmp(realm) == 0 }.any?
+            end
 
-              else
-                ::Gitlab.config.kerberos.simple_ldap_linking_allowed_realms.each do |realm|
-                  if domain.casecmp(realm) == 0
-                    return find_by_uid(uid, adapter)
-                  end
-                end
-              end
+            def simple_ldap_linking_allowed_realms
+              ::Gitlab.config.kerberos.simple_ldap_linking_allowed_realms
+            end
+
+            def simple_ldap_linking?
+              simple_ldap_linking_allowed_realms.present?
             end
 
             # Extracts the rightmost unbroken set of domain components from an

ee/spec/lib/gitlab/auth/ldap/person_spec.rb

@@ -60,32 +60,63 @@ RSpec.describe Gitlab::Auth::Ldap::Person do
   describe '.find_by_kerberos_principal' do
     let(:adapter) { ldap_adapter }
     let(:username) { 'foo' }
-    let(:principal) { username + '@' + kerberos_realm }
     let(:ldap_server) { 'ad.example.com' }
 
-    subject { described_class.find_by_kerberos_principal(principal, adapter) }
+    subject(:ldap_person) { described_class.find_by_kerberos_principal(principal, adapter) }
 
     before do
       stub_ldap_config(uid: 'sAMAccountName', base: 'ou=foo,dc=' + ldap_server.gsub('.', ',dc='))
     end
 
-    context 'LDAP server is not for kerberos realm' do
-      let(:kerberos_realm) { 'kerberos.example.com' }
+    context 'when simple LDAP linking is not configured' do
+      let(:principal) { username + '@' + kerberos_realm }
 
-      it 'returns nil without searching' do
-        expect(adapter).not_to receive(:user)
+      context 'LDAP server is not for kerberos realm' do
+        let(:kerberos_realm) { 'kerberos.example.com' }
 
-        is_expected.to be_nil
+        it 'returns nil without searching' do
+          expect(adapter).not_to receive(:user)
+
+          is_expected.to be_nil
+        end
+      end
+
+      context 'LDAP server is for kerberos realm' do
+        let(:kerberos_realm) { ldap_server }
+
+        it 'searches by configured uid attribute' do
+          expect(adapter).to receive(:user).with('sAMAccountName', username).and_return(:fake_user)
+
+          is_expected.to eq(:fake_user)
+        end
       end
     end
 
-    context 'LDAP server is for kerberos realm' do
-      let(:kerberos_realm) { ldap_server }
+    context 'when simple LDAP linking is enabled' do
+      let(:allowed_realms) { ['kerberos.example.com', ldap_server] }
+
+      before do
+        stub_config(kerberos: { simple_ldap_linking_allowed_realms: allowed_realms })
+      end
+
+      context 'principal domain matches an allowed realm' do
+        let(:principal) { "#{username}@#{allowed_realms[0]}" }
+
+        it 'searches by configured uid attribute' do
+          expect(adapter).to receive(:user).with('sAMAccountName', username).and_return(:fake_user)
+
+          expect(ldap_person).to eq(:fake_user)
+        end
+      end
+
+      context 'principal domain does not match an allowed realm' do
+        let(:principal) { "#{username}@alternate.example.com" }
 
-      it 'searches by configured uid attribute' do
-        expect(adapter).to receive(:user).with('sAMAccountName', username).and_return(:fake_user)
+        it 'returns nil without searching' do
+          expect(adapter).not_to receive(:user)
 
-        is_expected.to eq(:fake_user)
+          is_expected.to be_nil
+        end
       end
     end
   end