Merge branch 'bvl-ldap-find-user-by-email' into 'master'

Find a user by email from LDAP

Closes gitlab-ce#22924 and #2522

See merge request !2003

changelogs/unreleased-ee/bvl-ldap-find-user-by-email.yml

+---
+title: Lookup users by email in LDAP if lookup by DN fails during sync
+merge_request: 2003
+author:

doc/administration/auth/ldap-ee.md

@@ -248,23 +248,10 @@ main: # 'main' is the GitLab 'provider ID' of this LDAP server
 ### User DN has changed
 
 When an LDAP user is created in GitLab, their LDAP DN is stored for later reference.
-If a user's DN changes, it can cause problems for LDAP sync. Administrators can
-manually update a user's stored DN in this case.
-
-> **Note:** If GitLab cannot find a user by their DN, it will attempt to fallback
-  to finding the user by their email. If the lookup is successful, GitLab will
-  update the stored DN to the new value.
-
-1. Sign in to GitLab as an administrator user.
-1. Navigate to **Admin area -> Users**.
-1. Search for the user
-1. Open the user, by clicking on their name. Do not click 'Edit'.
-1. Navigate to the **Identities** tab.
-1. Click 'Edit' next to the LDAP identity.
-1. Change the 'Identifier' to match the user's new LDAP DN.
-1. Save the identity.
-
-Now the user should sync correctly.
+
+If GitLab cannot find a user by their DN, it will attempt to fallback
+to finding the user by their email. If the lookup is successful, GitLab will
+update the stored DN to the new value.
 
 ### User is not being added to a group
 

lib/ee/gitlab/ldap/person.rb

@@ -2,6 +2,19 @@ module EE
   module Gitlab
     module LDAP
       module Person
+        extend ActiveSupport::Concern
+
+        class_methods do
+          def find_by_email(email, adapter)
+            email_attributes = Array(adapter.config.attributes['email'])
+
+            email_attributes.each do |possible_attribute|
+              found_user = adapter.user(possible_attribute, email)
+              return found_user if found_user
+            end
+          end
+        end
+
         def ssh_keys
           if config.sync_ssh_keys? && entry.respond_to?(config.sync_ssh_keys)
             entry[config.sync_ssh_keys.to_sym]

lib/gitlab/ldap/access.rb

@@ -6,7 +6,7 @@
 module Gitlab
   module LDAP
     class Access
-      attr_reader :adapter, :provider, :user, :ldap_user
+      attr_reader :adapter, :provider, :user, :ldap_user, :ldap_identity
 
       def self.open(user, &block)
         Gitlab::LDAP::Adapter.open(user.ldap_identity.provider) do |adapter|
@@ -32,7 +32,8 @@ module Gitlab
       def initialize(user, adapter = nil)
         @adapter = adapter
         @user = user
-        @provider = user.ldap_identity.provider
+        @provider = adapter&.provider || user.ldap_identity.provider
+        @ldap_identity = user.identities.find_by(provider: @provider)
       end
 
       def allowed?
@@ -43,7 +44,7 @@ module Gitlab
           end
 
           # Block user in GitLab if he/she was blocked in AD
-          if Gitlab::LDAP::Person.disabled_via_active_directory?(user.ldap_identity.extern_uid, adapter)
+          if Gitlab::LDAP::Person.disabled_via_active_directory?(ldap_identity.extern_uid, adapter)
             block_user(user, 'is disabled in Active Directory')
             false
           else
@@ -65,15 +66,24 @@ module Gitlab
         Gitlab::LDAP::Config.new(provider)
       end
 
+      def find_ldap_user
+        found_user = Gitlab::LDAP::Person.find_by_dn(ldap_identity.extern_uid, adapter)
+        return found_user if found_user
+
+        if user.ldap_email?
+          Gitlab::LDAP::Person.find_by_email(user.email, adapter)
+        end
+      end
+
       def ldap_user
-        @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter)
+        @ldap_user ||= find_ldap_user
       end
 
       def block_user(user, reason)
         user.ldap_block
 
         Gitlab::AppLogger.info(
-          "LDAP account \"#{user.ldap_identity.extern_uid}\" #{reason}, " \
+          "LDAP account \"#{ldap_identity.extern_uid}\" #{reason}, " \
           "blocking Gitlab user \"#{user.name}\" (#{user.email})"
         )
       end
@@ -82,7 +92,7 @@ module Gitlab
         user.activate
 
         Gitlab::AppLogger.info(
-          "LDAP account \"#{user.ldap_identity.extern_uid}\" #{reason}, " \
+          "LDAP account \"#{ldap_identity.extern_uid}\" #{reason}, " \
           "unblocking Gitlab user \"#{user.name}\" (#{user.email})"
         )
       end
@@ -90,6 +100,7 @@ module Gitlab
       def update_user
         update_email
         update_memberships
+        update_identity
         update_ssh_keys if sync_ssh_keys?
         update_kerberos_identity if import_kerberos_identities?
       end
@@ -156,6 +167,15 @@ module Gitlab
         user.update(email: ldap_email)
       end
 
+      def update_identity
+        return if ldap_user.dn.empty? || ldap_user.dn == ldap_identity.extern_uid
+
+        unless ldap_identity.update(extern_uid: ldap_user.dn)
+          Rails.logger.error "Could not update DN for #{user.name} (#{user.id})\n"\
+                             "error messages: #{user.ldap_identity.errors.messages}"
+        end
+      end
+
       delegate :sync_ssh_keys?, to: :ldap_config
 
       def import_kerberos_identities?

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

 require 'spec_helper'
 
 describe Gitlab::LDAP::Person do
+  include LdapHelpers
+
   it 'includes the EE module' do
     expect(described_class).to include(EE::Gitlab::LDAP::Person)
   end
 
+  describe '.find_by_email' do
+    it 'tries finding for each configured email attribute' do
+      adapter = ldap_adapter
+
+      expect(adapter).to receive(:user).with('mail', 'jane@gitlab.com')
+      expect(adapter).to receive(:user).with('email', 'jane@gitlab.com')
+      expect(adapter).to receive(:user).with('userPrincipalName', 'jane@gitlab.com')
+
+      described_class.find_by_email('jane@gitlab.com', adapter)
+    end
+  end
+
   describe '#kerberos_principal' do
     let(:entry) do
       ldif = "dn: cn=foo, dc=bar, dc=com\n"

spec/lib/gitlab/ldap/access_spec.rb

@@ -5,6 +5,23 @@ describe Gitlab::LDAP::Access, lib: true do
   let(:access) { Gitlab::LDAP::Access.new user }
   let(:user) { create(:omniauth_user) }
 
+  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(:ldap_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(:ldap_email?).and_return(true)
+      expect(Gitlab::LDAP::Person).to receive(:find_by_email)
+
+      access.find_ldap_user
+    end
+  end
+
   describe '#allowed?' do
     subject { access.allowed? }
 
@@ -193,6 +210,12 @@ describe Gitlab::LDAP::Access, lib: true do
 
       subject
     end
+
+    it 'updates the ldap identity' do
+      expect(access).to receive(:update_identity)
+
+      subject
+    end
   end
 
   describe '#update_kerberos_identity' do
@@ -358,4 +381,19 @@ describe Gitlab::LDAP::Access, lib: true do
       access.update_memberships
     end
   end
+
+  describe '#update_identity' do
+    it 'updates the external UID if it changed in the entry' do
+      entry = ldap_user_entry('another uid')
+      provider = user.ldap_identity.provider
+      person = Gitlab::LDAP::Person.new(entry, provider)
+
+      allow(access).to receive(:ldap_user).and_return(person)
+
+      access.update_identity
+
+      expect(user.ldap_identity.reload.extern_uid)
+        .to eq('uid=another uid,ou=users,dc=example,dc=com')
+    end
+  end
 end