From 61ba66c033f54421a4cab01a4a7c77c393a7d5e2 Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Tue, 10 Sep 2013 11:54:37 +0300
Subject: [PATCH] Refactor LDAP::Access

Select only LDAP groups that are activated inside GitLab
Make LDAP::Access more readable
---
 lib/gitlab/ldap/access.rb  | 42 +++++++++++++++++++++++++-------------
 lib/gitlab/ldap/adapter.rb |  4 ++++
 lib/gitlab/ldap/group.rb   | 18 +++++++++++++++-
 lib/gitlab/ldap/person.rb  | 10 ---------
 4 files changed, 49 insertions(+), 25 deletions(-)

diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb
index 1b4d92faca..442dcea1ff 100644
--- a/lib/gitlab/ldap/access.rb
+++ b/lib/gitlab/ldap/access.rb
@@ -12,27 +12,41 @@ module Gitlab
         # if instance does not use group_base setting
         return true unless Gitlab.config.ldap['group_base'].present?
 
+        # Get LDAP user entry
         ldap_user = Gitlab::LDAP::Person.find_by_dn(user.extern_uid)
-        ldap_groups = ldap_user.groups
-        ldap_groups_cn = ldap_groups.map(&:name)
-        groups = ::Group.where(ldap_cn: ldap_groups_cn)
 
-        # First lets add user to new groups
-        groups.each do |group|
-          group.add_users([user.id], group.ldap_access) if group.ldap_access.present?
-        end
+        # Get all GitLab groups with activated LDAP
+        groups = ::Group.where('ldap_cn IS NOT NULL')
 
-        # Remove groups with LDAP if user lost access to it
-        user.authorized_groups.where('ldap_cn IS NOT NULL').each do |group|
-          if ldap_groups_cn.include?(group.ldap_cn)
-            # ok user still in group
+        # Get LDAP groups based on cn from GitLab groups
+        ldap_groups = groups.pluck(:ldap_cn).map { |cn| Gitlab::LDAP::Group.find_by_cn(cn) }
+        ldap_groups = ldap_groups.compact.uniq
+
+        # Iterate over ldap groups and check user membership
+        ldap_groups.each do |ldap_group|
+          if ldap_group.has_member?(ldap_user)
+            # If user present in LDAP group -> add him to GitLab groups
+            add_user_to_groups(user.id, ldap_group.cn)
           else
-            # user lost access to this group in ldap
-            membership = group.users_groups.where(user_id: user.id).last
-            membership.destroy if membership
+            # If not - remove him from GitLab groups
+            remove_user_from_groups(user.id, ldap_group.cn)
           end
         end
       end
+
+      def add_user_to_groups(user_id, group_cn)
+        groups = ::Group.where(ldap_cn: group_cn)
+        groups.each do |group|
+          group.add_users([user_id], group.ldap_access) if group.ldap_access.present?
+        end
+      end
+
+      def remove_user_from_groups(user_id, group_cn)
+        groups = ::Group.where(ldap_cn: group_cn)
+        groups.each do |group|
+          group.users_groups.where(user_id: user_id).destroy_all
+        end
+      end
     end
   end
 end
diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb
index 999b690b0a..a9ebfa86b5 100644
--- a/lib/gitlab/ldap/adapter.rb
+++ b/lib/gitlab/ldap/adapter.rb
@@ -51,6 +51,10 @@ module Gitlab
         end
       end
 
+      def group(*args)
+        groups(*args).first
+      end
+
       def users(field, value)
         if field.to_sym == :dn
           options = {
diff --git a/lib/gitlab/ldap/group.rb b/lib/gitlab/ldap/group.rb
index 138af8a3f6..299a28f95d 100644
--- a/lib/gitlab/ldap/group.rb
+++ b/lib/gitlab/ldap/group.rb
@@ -7,14 +7,22 @@
 module Gitlab
   module LDAP
     class Group
+      def self.find_by_cn(cn)
+        Gitlab::LDAP::Adapter.new.group(cn)
+      end
+
       def initialize(entry)
         @entry = entry
       end
 
-      def name
+      def cn
         entry.cn.join(" ")
       end
 
+      def name
+        cn
+      end
+
       def path
         name.parameterize
       end
@@ -27,6 +35,14 @@ module Gitlab
         entry.memberuid
       end
 
+      def has_member?(user)
+        if memberuid?
+          member_uids.include?(user.uid)
+        else
+          member_dns.include?(user.dn)
+        end
+      end
+
       def member_dns
         if entry.respond_to? :member
           entry.member
diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb
index 3060859493..13cb3b7d2d 100644
--- a/lib/gitlab/ldap/person.rb
+++ b/lib/gitlab/ldap/person.rb
@@ -35,16 +35,6 @@ module Gitlab
         entry.dn
       end
 
-      def groups
-        adapter.groups.select do |group|
-          if group.memberuid?
-            group.member_uids.include?(uid)
-          else
-            group.member_dns.include?(dn)
-          end
-        end
-      end
-
       private
 
       def entry