Normalize LDAP DN when looking up identitydm-ldap-identity-normalize-dn

7 files changed, 33 insertions, 10 deletions

diff --git a/app/models/identity.rb b/app/models/identity.rb
index 920a25932b4..ac8094b610e 100644
--- a/app/models/identity.rb
+++ b/app/models/identity.rb
@@ -7,7 +7,10 @@ class Identity < ActiveRecord::Base
   validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider }
   validates :user_id, uniqueness: { scope: :provider }
 
-  scope :with_extern_uid, ->(provider, extern_uid) { where(extern_uid: extern_uid, provider: provider) }
+  scope :with_extern_uid, ->(provider, extern_uid) do
+    extern_uid = Gitlab::LDAP::Person.normalize_dn(extern_uid) if provider.starts_with?('ldap')
+    where(extern_uid: extern_uid, provider: provider)
+  end
 
   def ldap?
     provider.starts_with?('ldap')
diff --git a/changelogs/unreleased/dm-ldap-identity-normalize-dn.yml b/changelogs/unreleased/dm-ldap-identity-normalize-dn.yml
new file mode 100644
index 00000000000..7ab25f79143
--- /dev/null
+++ b/changelogs/unreleased/dm-ldap-identity-normalize-dn.yml
@@ -0,0 +1,5 @@
+---
+title: Normalize LDAP DN when looking up identity
+merge_request:
+author:
+type: fixed
diff --git a/lib/gitlab/ldap/auth_hash.rb b/lib/gitlab/ldap/auth_hash.rb
index 3123da17fd9..1bd0965679a 100644
--- a/lib/gitlab/ldap/auth_hash.rb
+++ b/lib/gitlab/ldap/auth_hash.rb
@@ -4,7 +4,7 @@ module Gitlab
   module LDAP
     class AuthHash < Gitlab::OAuth::AuthHash
       def uid
-        Gitlab::LDAP::Person.normalize_dn(super)
+        @uid ||= Gitlab::LDAP::Person.normalize_dn(super)
       end
 
       private
diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb
index 1793097363e..4d5c67ed892 100644
--- a/lib/gitlab/ldap/user.rb
+++ b/lib/gitlab/ldap/user.rb
@@ -9,10 +9,11 @@ module Gitlab
     class User < Gitlab::OAuth::User
       class << self
         def find_by_uid_and_provider(uid, provider)
-          # LDAP distinguished name is case-insensitive
+          uid = Gitlab::LDAP::Person.normalize_dn(uid)
+
           identity = ::Identity
             .where(provider: provider)
-            .iwhere(extern_uid: uid).last
+            .where(extern_uid: uid).last
           identity && identity.user
         end
       end
diff --git a/spec/lib/gitlab/ldap/authentication_spec.rb b/spec/lib/gitlab/ldap/authentication_spec.rb
index 01b6282af0c..9d57a46c12b 100644
--- a/spec/lib/gitlab/ldap/authentication_spec.rb
+++ b/spec/lib/gitlab/ldap/authentication_spec.rb
@@ -1,8 +1,8 @@
 require 'spec_helper'
 
 describe Gitlab::LDAP::Authentication do
-  let(:user)     { create(:omniauth_user, extern_uid: dn) }
-  let(:dn)       { 'uid=john,ou=people,dc=example,dc=com' }
+  let(:dn)       { 'uid=John Smith, ou=People, dc=example, dc=com' }
+  let(:user)     { create(:omniauth_user, extern_uid: Gitlab::LDAP::Person.normalize_dn(dn)) }
   let(:login)    { 'john' }
   let(:password) { 'password' }
 
diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb
index 9a4705d1cee..260df6e4dae 100644
--- a/spec/lib/gitlab/ldap/user_spec.rb
+++ b/spec/lib/gitlab/ldap/user_spec.rb
@@ -44,23 +44,25 @@ describe Gitlab::LDAP::User do
   end
 
   describe '.find_by_uid_and_provider' do
+    let(:dn) { 'CN=John Åström, CN=Users, DC=Example, DC=com' }
+
     it 'retrieves the correct user' do
       special_info = {
         name: 'John Åström',
         email: 'john@example.com',
         nickname: 'jastrom'
       }
-      special_hash = OmniAuth::AuthHash.new(uid: 'CN=John Åström,CN=Users,DC=Example,DC=com', provider: 'ldapmain', info: special_info)
+      special_hash = OmniAuth::AuthHash.new(uid: dn, provider: 'ldapmain', info: special_info)
       special_chars_user = described_class.new(special_hash)
       user = special_chars_user.save
 
-      expect(described_class.find_by_uid_and_provider(special_hash.uid, special_hash.provider)).to eq user
+      expect(described_class.find_by_uid_and_provider(dn, 'ldapmain')).to eq user
     end
   end
 
   describe 'find or create' do
     it "finds the user if already existing" do
-      create(:omniauth_user, extern_uid: 'uid=John Smith,ou=People,dc=example,dc=com', provider: 'ldapmain')
+      create(:omniauth_user, extern_uid: 'uid=john smith,ou=people,dc=example,dc=com', provider: 'ldapmain')
 
       expect { ldap_user.save }.not_to change { User.count }
     end
diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb
index 4ca6556d0f4..3ed048744de 100644
--- a/spec/models/identity_spec.rb
+++ b/spec/models/identity_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-RSpec.describe Identity do
+describe Identity do
   describe 'relations' do
     it { is_expected.to belong_to(:user) }
   end
@@ -22,4 +22,16 @@ RSpec.describe Identity do
       expect(other_identity.ldap?).to be_falsey
     end
   end
+
+  describe '.with_extern_uid' do
+    context 'LDAP identity' do
+      let!(:ldap_identity) { create(:identity, provider: 'ldapmain', extern_uid: 'uid=john smith,ou=people,dc=example,dc=com') }
+
+      it 'finds the identity when the DN is formatted differently' do
+        identity = described_class.with_extern_uid('ldapmain', 'uid=John Smith, ou=People, dc=example, dc=com').first
+
+        expect(identity).to eq(ldap_identity)
+      end
+    end
+  end
 end