refactor group base checks

ee/lib/ee/gitlab/ldap/sync/group.rb

@@ -74,12 +74,7 @@ module EE
             access_levels = AccessLevels.new
             # Only iterate over group links for the current provider
             group.ldap_group_links.with_provider(provider).each do |group_link|
-              if member_dns = get_member_dns(group_link)
-                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
+              update_access_levels(access_levels, group_link)
             end
 
             update_existing_group_membership(group, access_levels)
@@ -88,6 +83,18 @@ module EE
 
           private
 
+          def update_access_levels(access_levels, group_link)
+            if config.group_base.blank? && group_link.cn
+              logger.debug { "No `group_base` configured for '#{provider}' provider and group link CN #{group_link.cn}. Skipping" }
+            elsif member_dns = get_member_dns(group_link)
+              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
+
           def get_member_dns(group_link)
             group_link.cn ? dns_for_group_cn(group_link.cn) : UserFilter.filter(@proxy, group_link.filter)
           end
@@ -213,6 +220,10 @@ module EE
           def logger
             Rails.logger
           end
+
+          def config
+            @proxy.adapter.config
+          end
         end
       end
     end

ee/lib/ee/gitlab/ldap/sync/groups.rb

@@ -26,11 +26,6 @@ module EE
           end
 
           def update_permissions
-            unless config.group_base.present?
-              logger.debug { "No `group_base` configured for '#{provider}' provider. Skipping" }
-              return nil
-            end
-
             logger.debug { "Performing LDAP group sync for '#{provider}' provider" }
             sync_groups
             logger.debug { "Finished LDAP group sync for '#{provider}' provider" }

spec/ee/spec/lib/ee/gitlab/ldap/sync/group_spec.rb

@@ -299,6 +299,14 @@ describe EE::Gitlab::LDAP::Sync::Group do
           expect(group.members.pluck(:access_level))
             .to match_array([::Gitlab::Access::MASTER, ::Gitlab::Access::OWNER])
         end
+
+        it 'does not update permissions when group base is missing' do
+          stub_ldap_config(group_base: nil)
+
+          sync_group.update_permissions
+
+          expect(group.members.pluck(:user_id)).not_to include(user.id)
+        end
       end
     end
 
@@ -414,6 +422,14 @@ describe EE::Gitlab::LDAP::Sync::Group do
             expect(group.members.pluck(:user_id)).to include(user.id)
             expect(group.members.find_by(user_id: user.id).ldap?).to be_truthy
           end
+
+          it 'updates permissions when group base is missing' do
+            stub_ldap_config(group_base: nil)
+
+            sync_group.update_permissions
+
+            expect(group.members.pluck(:user_id)).to include(user.id)
+          end
         end
       end
     end

spec/ee/spec/lib/ee/gitlab/ldap/sync/groups_spec.rb

@@ -24,10 +24,6 @@ describe EE::Gitlab::LDAP::Sync::Groups do
         stub_ldap_config(group_base: nil)
       end
 
-      it 'does not call EE::Gitlab::LDAP::Sync::Group#execute' do
-        expect(EE::Gitlab::LDAP::Sync::Group).not_to receive(:execute)
-      end
-
       it 'does not call EE::Gitlab::LDAP::Sync::AdminUsers#execute' do
         expect(EE::Gitlab::LDAP::Sync::AdminUsers).not_to receive(:execute)
       end