Improvements to LDAP::User model

* method #changed? also tracks changes of identites (fixes issue with email mapping)
* find ldap identity before initialize one

2 files changed, 23 insertions, 2 deletions

diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb
index 3ef494ba137..cfa8692659d 100644
--- a/lib/gitlab/ldap/user.rb
+++ b/lib/gitlab/ldap/user.rb
@@ -40,12 +40,16 @@ module Gitlab
 
       def update_user_attributes
         gl_user.email = auth_hash.email
-        gl_user.identities.build(provider: auth_hash.provider, extern_uid: auth_hash.uid)
+
+        # Build new identity only if we dont have have same one
+        gl_user.identities.find_or_initialize_by(provider: auth_hash.provider,
+                                                 extern_uid: auth_hash.uid)
+
         gl_user
       end
 
       def changed?
-        gl_user.changed?
+        gl_user.changed? || gl_user.identities.any?(&:changed?)
       end
 
       def needs_blocking?
diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb
index f73884e6441..63ffc21ba3b 100644
--- a/spec/lib/gitlab/ldap/user_spec.rb
+++ b/spec/lib/gitlab/ldap/user_spec.rb
@@ -13,6 +13,23 @@ describe Gitlab::LDAP::User do
     double(uid: 'my-uid', provider: 'ldapmain', info: double(info))
   end
 
+  describe :changed? do
+    it "marks existing ldap user as changed" do
+      existing_user = create(:omniauth_user, extern_uid: 'my-uid', provider: 'ldapmain')
+      expect(gl_user.changed?).to be_true
+    end
+
+    it "marks existing non-ldap user if the email matches as changed" do
+      existing_user = create(:user, email: 'john@example.com')
+      expect(gl_user.changed?).to be_true
+    end
+
+    it "dont marks existing ldap user as changed" do
+      existing_user = create(:omniauth_user, email: 'john@example.com', extern_uid: 'my-uid', provider: 'ldapmain')
+      expect(gl_user.changed?).to be_false
+    end
+  end
+
   describe :find_or_create do
     it "finds the user if already existing" do
       existing_user = create(:omniauth_user, extern_uid: 'my-uid', provider: 'ldapmain')