Merge branch 'refactor_group_sync' into 'master'

Refactor group sync to pull access level logic to its own class

A continuation of !229; Fixes #398 

Pull access level logic to its own class to clean up the `GroupSync` class.

See merge request !306

CHANGELOG-EE

@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.7.0 (unreleased)
   - Update GitLab Pages to 0.2.1: support user-defined 404 pages
+  - Refactor group sync to pull access level logic to its own class. !306
 
 v 8.6.3 (unreleased)
   - Exit ElasticIndexerWorker's job happily if record cannot be found. !311

lib/gitlab/ldap/access_levels.rb

+module Gitlab
+  module LDAP
+    # Create a hash map of member DNs to access levels. The highest
+    # access level is retained in cases where `set` is called multiple times
+    # for the same DN.
+    class AccessLevels < Hash
+      def set(dns, to:)
+        dns.each do |dn|
+          current = self[dn]
+
+          # Keep the higher of the access values.
+          self[dn] = to if current.nil? || to > current
+        end
+      end
+    end
+  end
+end

lib/gitlab/ldap/group_sync.rb

@@ -64,21 +64,19 @@ module Gitlab
           next unless lease.try_obtain
 
           logger.debug { "Syncing '#{group.name}' group" }
-          access_hash = {}
 
           # Only iterate over group links for the current provider
           group.ldap_group_links.with_provider(provider).each do |group_link|
             if member_dns = dns_for_group_cn(group_link.cn)
-              members_to_access_hash(
-                access_hash, member_dns, group_link.group_access
-              )
-
-              logger.debug { "Resolved '#{group.name}' group member access: #{access_hash}" }
+              access_levels.set(member_dns, to: group_link.group_access)
+              logger.debug do
+                "Resolved '#{group.name}' group member access: #{access_levels.to_hash}"
+              end
             end
           end
 
-          update_existing_group_membership(group, access_hash)
-          add_new_members(group, access_hash)
+          update_existing_group_membership(group)
+          add_new_members(group)
 
           group.update(last_ldap_sync_at: Time.now)
 
@@ -120,18 +118,6 @@ module Gitlab
         end
       end
 
-      def members_to_access_hash(access_hash, member_dns, group_access)
-        member_dns.each do |member_dn|
-          current_access = access_hash[member_dn]
-
-          # Keep the higher of the access values.
-          if current_access.nil? || group_access > current_access
-            access_hash[member_dn] = group_access
-          end
-        end
-        access_hash
-      end
-
       private
 
       # Cache LDAP group member DNs so we don't query LDAP groups more than once.
@@ -154,6 +140,10 @@ module Gitlab
         @config ||= Gitlab::LDAP::Config.new(provider)
       end
 
+      def access_levels
+        @access_levels ||= Gitlab::LDAP::AccessLevels.new
+      end
+
       def group_base
         config.group_base
       end
@@ -213,7 +203,7 @@ module Gitlab
         identity.save
       end
 
-      def update_existing_group_membership(group, access_hash)
+      def update_existing_group_membership(group)
         logger.debug { "Updating existing membership for '#{group.name}' group" }
 
         select_and_preload_group_members(group).each do |member|
@@ -229,15 +219,15 @@ module Gitlab
           # of two LDAP groups from different providers linked to the same
           # GitLab group. This is not ideal, but preserves existing behavior.
           if user.ldap_identity.id != identity.id
-            access_hash.delete(member_dn)
+            access_levels.delete(member_dn)
             next
           end
 
-          desired_access = access_hash[member_dn]
+          desired_access = access_levels[member_dn]
 
           # Don't do anything if the user already has the desired access level
           if member.access_level == desired_access
-            access_hash.delete(member_dn)
+            access_levels.delete(member_dn)
             next
           end
 
@@ -247,7 +237,7 @@ module Gitlab
             add_or_update_user_membership(user, group, desired_access)
 
             # Delete this entry from the hash now that we've acted on it
-            access_hash.delete(member_dn)
+            access_levels.delete(member_dn)
           elsif group.last_owner?(user)
             warn_cannot_remove_last_owner(user, group)
           else
@@ -256,10 +246,10 @@ module Gitlab
         end
       end
 
-      def add_new_members(group, access_hash)
+      def add_new_members(group)
         logger.debug { "Adding new members to '#{group.name}' group" }
 
-        access_hash.each do |member_dn, access_level|
+        access_levels.each do |member_dn, access_level|
           user = Gitlab::LDAP::User.find_by_uid_and_provider(member_dn, provider)
 
           if user.present?

spec/lib/gitlab/ldap/access_levels_spec.rb

+require 'spec_helper'
+
+describe Gitlab::LDAP::AccessLevels, lib: true do
+  describe '#set' do
+    let(:access_levels) { Gitlab::LDAP::AccessLevels.new }
+    let(:dns) do
+      %w(
+      uid=johndoe,ou=users,dc=example,dc=com
+      uid=janedoe,ou=users,dc=example,dc=com
+    )
+    end
+    subject { access_levels }
+
+    context 'when access_levels is empty' do
+      before { access_levels.set(dns, to: Gitlab::Access::DEVELOPER) }
+
+      it do
+        is_expected
+          .to eq({
+            'uid=janedoe,ou=users,dc=example,dc=com' => Gitlab::Access::DEVELOPER,
+            'uid=johndoe,ou=users,dc=example,dc=com' => Gitlab::Access::DEVELOPER
+          })
+      end
+    end
+
+    context 'when access_hash has existing entries' do
+      let(:developer_dns) do
+        %w{
+          uid=janedoe,ou=users,dc=example,dc=com
+          uid=jamesdoe,ou=users,dc=example,dc=com
+        }
+      end
+      let(:master_dns) do
+        %w{
+          uid=johndoe,ou=users,dc=example,dc=com
+          uid=janedoe,ou=users,dc=example,dc=com
+        }
+      end
+      before do
+        access_levels.set(master_dns, to: Gitlab::Access::MASTER)
+        access_levels.set(developer_dns, to: Gitlab::Access::DEVELOPER)
+      end
+
+      it 'keeps the higher of all access values' do
+        is_expected
+          .to eq({
+             'uid=janedoe,ou=users,dc=example,dc=com'  => Gitlab::Access::MASTER,
+             'uid=johndoe,ou=users,dc=example,dc=com'  => Gitlab::Access::MASTER,
+             'uid=jamesdoe,ou=users,dc=example,dc=com' => Gitlab::Access::DEVELOPER
+          })
+      end
+    end
+  end
+end

spec/lib/gitlab/ldap/group_sync_spec.rb

@@ -498,47 +498,4 @@ describe Gitlab::LDAP::GroupSync, lib: true do
         .not_to change { User.admins.where(id: user3.id).any? }
     end
   end
-
-  describe '#members_to_access_hash' do
-    let(:group_access) { Gitlab::Access::DEVELOPER }
-    let(:member_dns) do
-      %w(
-        uid=johndoe,ou=users,dc=example,dc=com
-        uid=janedoe,ou=users,dc=example,dc=com
-      )
-    end
-
-    subject { group_sync.members_to_access_hash(access_hash, member_dns, group_access) }
-
-    context 'when access_hash is empty' do
-      let(:access_hash) { Hash.new }
-
-      it do
-        is_expected
-          .to eq({
-            'uid=janedoe,ou=users,dc=example,dc=com' => 30,
-            'uid=johndoe,ou=users,dc=example,dc=com' => 30
-          })
-      end
-    end
-
-    context 'when access_hash has existing entries' do
-      let(:access_hash) do
-        {
-          'uid=janedoe,ou=users,dc=example,dc=com'  => 40,
-          'uid=johndoe,ou=users,dc=example,dc=com'  => 20,
-          'uid=jamesdoe,ou=users,dc=example,dc=com' => 40,
-        }
-      end
-
-      it 'keeps the higher of all access values' do
-        is_expected
-          .to eq({
-             'uid=janedoe,ou=users,dc=example,dc=com'  => 40,
-             'uid=johndoe,ou=users,dc=example,dc=com'  => 30,
-             'uid=jamesdoe,ou=users,dc=example,dc=com' => 40
-          })
-      end
-    end
-  end
 end