Merge branch 'ce-to-ee-2017-09-08-fix-user-access' into 'ce-to-ee-2017-09-08'

Fix user access due to having multiple identities

See merge request !2886

lib/gitlab/ldap/access.rb

@@ -6,7 +6,7 @@
 module Gitlab
   module LDAP
     class Access
-      attr_reader :adapter, :provider, :user, :ldap_user, :ldap_identity
+      attr_reader :provider, :user, :ldap_identity
 
       def self.open(user, &block)
         Gitlab::LDAP::Adapter.open(user.ldap_identity.provider) do |adapter|
@@ -32,8 +32,8 @@ module Gitlab
       def initialize(user, adapter = nil)
         @adapter = adapter
         @user = user
-        @provider = adapter&.provider || user.ldap_identity.provider
-        @ldap_identity = user.identities.find_by(provider: @provider)
+        @ldap_identity = user.ldap_identity
+        @provider = adapter&.provider || @ldap_identity&.provider
       end
 
       def allowed?
@@ -67,10 +67,12 @@ module Gitlab
       end
 
       def find_ldap_user
+        return unless provider
+
         found_user = Gitlab::LDAP::Person.find_by_dn(ldap_identity.extern_uid, adapter)
         return found_user if found_user
 
-        if user.external_email? && [nil, provider].include?(user.email_provider)
+        if ldap_identity
           Gitlab::LDAP::Person.find_by_email(user.email, adapter)
         end
       end
@@ -82,10 +84,17 @@ module Gitlab
       def block_user(user, reason)
         user.ldap_block
 
-        Gitlab::AppLogger.info(
-          "LDAP account \"#{ldap_identity.extern_uid}\" #{reason}, " \
-          "blocking Gitlab user \"#{user.name}\" (#{user.email})"
-        )
+        if provider
+          Gitlab::AppLogger.info(
+            "LDAP account \"#{ldap_identity.extern_uid}\" #{reason}, " \
+            "blocking Gitlab user \"#{user.name}\" (#{user.email})"
+          )
+        else
+          Gitlab::AppLogger.info(
+            "Account is not provided by LDAP, " \
+            "blocking Gitlab user \"#{user.name}\" (#{user.email})"
+          )
+        end
       end
 
       def unblock_user(user, reason)

spec/lib/gitlab/ldap/access_spec.rb

@@ -20,14 +20,12 @@ describe Gitlab::LDAP::Access do
   describe '#find_ldap_user' do
     it 'finds a user by dn first' do
       expect(Gitlab::LDAP::Person).to receive(:find_by_dn).and_return(:ldap_user)
-      expect(user).not_to receive(:external_email?)
 
       access.find_ldap_user
     end
 
     it 'finds a user by email if the email came from LDAP' do
       expect(Gitlab::LDAP::Person).to receive(:find_by_dn).and_return(nil)
-      expect(user).to receive(:external_email?).and_return(true)
       expect(Gitlab::LDAP::Person).to receive(:find_by_email)
 
       access.find_ldap_user
@@ -51,7 +49,7 @@ describe Gitlab::LDAP::Access do
       end
 
       context 'when looking for a user by email' do
-        let(:user) { create(:omniauth_user, external_email: true) }
+        let(:user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'my-provider') }
 
         it { is_expected.to be_falsey }
       end