Merge branch 'fix/oauth_user_ldap_person' into 'master'ldap-person-fix-7-12

Fix behavior of ldap_person method in Gitlab::OAuth::User

Code tweaks in 45e9150a caused the ldap_person method to not return expected results.
Improved tests to cover the ldap_person method, which was previously stubbed.

Restored the previous implementation of ldap_person, as I could not find a more concise to write it that still works with all the test cases.

See merge request !837

2 files changed, 61 insertions, 50 deletions

diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index c4971b5bcc6..17ce4d4b174 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -87,12 +87,13 @@ module Gitlab
       def ldap_person
         return @ldap_person if defined?(@ldap_person)
 
-        # looks for a corresponding person with same uid in any of the configured LDAP providers
-        @ldap_person = Gitlab::LDAP::Config.providers.find do |provider|
+        # Look for a corresponding person with same uid in any of the configured LDAP providers
+        Gitlab::LDAP::Config.providers.each do |provider|
           adapter = Gitlab::LDAP::Adapter.new(provider)
-
-          Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter)
+          @ldap_person = Gitlab::LDAP::Person.find_by_uid(auth_hash.uid, adapter)
+          break if @ldap_person
         end
+        @ldap_person
       end
 
       def ldap_config
diff --git a/spec/lib/gitlab/o_auth/user_spec.rb b/spec/lib/gitlab/o_auth/user_spec.rb
index 2a982e8b107..d383ea2d051 100644
--- a/spec/lib/gitlab/o_auth/user_spec.rb
+++ b/spec/lib/gitlab/o_auth/user_spec.rb
@@ -62,55 +62,65 @@ describe Gitlab::OAuth::User do
 
       context "with auto_link_ldap_user enabled" do
         before { Gitlab.config.omniauth.stub auto_link_ldap_user: true }
-
-        context "and a corresponding LDAP person" do
-          before do
-            ldap_user.stub(:uid) { uid }
-            ldap_user.stub(:username) { uid }
-            ldap_user.stub(:email) { ['johndoe@example.com','john2@example.com'] }
-            ldap_user.stub(:dn) { 'uid=user1,ou=People,dc=example' }
-            allow(oauth_user).to receive(:ldap_person).and_return(ldap_user)
-          end
-
-          context "and no account for the LDAP user" do
-
-            it "creates a user with dual LDAP and omniauth identities" do
-              oauth_user.save
-
-              expect(gl_user).to be_valid
-              expect(gl_user.username).to eql uid
-              expect(gl_user.email).to eql 'johndoe@example.com'
-              expect(gl_user.identities.length).to eql 2
-              identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
-              expect(identities_as_hash).to match_array(
-                [ { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
-                  { provider: 'twitter', extern_uid: uid }
-                ])
+        
+        context "and no LDAP provider defined" do
+          before { allow(Gitlab::LDAP::Config).to receive(:providers).and_return([]) }
+          
+          include_examples "to verify compliance with allow_single_sign_on"
+        end
+        
+        context "and at least one LDAP provider is defined" do
+          before { allow(Gitlab::LDAP::Config).to receive(:providers).and_return(['ldapmain']) }
+
+          context "and a corresponding LDAP person" do
+            before do
+              ldap_user.stub(:uid) { uid }
+              ldap_user.stub(:username) { uid }
+              ldap_user.stub(:email) { ['johndoe@example.com','john2@example.com'] }
+              ldap_user.stub(:dn) { 'uid=user1,ou=People,dc=example' }
+              allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
             end
-          end
-
-          context "and LDAP user has an account already" do
-            let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'ldapmain', username: 'john') }
-            it "adds the omniauth identity to the LDAP account" do
-              oauth_user.save
-
-              expect(gl_user).to be_valid
-              expect(gl_user.username).to eql 'john'
-              expect(gl_user.email).to eql 'john@example.com'
-              expect(gl_user.identities.length).to eql 2
-              identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
-              expect(identities_as_hash).to match_array(
-                [ { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
-                  { provider: 'twitter', extern_uid: uid }
-                ])
+  
+            context "and no account for the LDAP user" do
+  
+              it "creates a user with dual LDAP and omniauth identities" do
+                oauth_user.save
+  
+                expect(gl_user).to be_valid
+                expect(gl_user.username).to eql uid
+                expect(gl_user.email).to eql 'johndoe@example.com'
+                expect(gl_user.identities.length).to eql 2
+                identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
+                expect(identities_as_hash).to match_array(
+                  [ { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
+                    { provider: 'twitter', extern_uid: uid }
+                  ])
+              end
+            end
+  
+            context "and LDAP user has an account already" do
+              let!(:existing_user) { create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=user1,ou=People,dc=example', provider: 'ldapmain', username: 'john') }
+              it "adds the omniauth identity to the LDAP account" do
+                oauth_user.save
+  
+                expect(gl_user).to be_valid
+                expect(gl_user.username).to eql 'john'
+                expect(gl_user.email).to eql 'john@example.com'
+                expect(gl_user.identities.length).to eql 2
+                identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
+                expect(identities_as_hash).to match_array(
+                  [ { provider: 'ldapmain', extern_uid: 'uid=user1,ou=People,dc=example' },
+                    { provider: 'twitter', extern_uid: uid }
+                  ])
+              end
             end
           end
-        end
-
-        context "and no corresponding LDAP person" do
-          before { allow(oauth_user).to receive(:ldap_person).and_return(nil) }
-
-          include_examples "to verify compliance with allow_single_sign_on"
+  
+          context "and no corresponding LDAP person" do
+            before { allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(nil) }
+  
+            include_examples "to verify compliance with allow_single_sign_on"
+          end
         end
       end