Merge branch 'dblessing_saml_group_sync_fix' into 'master'

Ensure SAML Group Sync runs anytime SAML Group Links exist

See merge request gitlab-org/gitlab!67633

ee/lib/gitlab/auth/group_saml/membership_updater.rb

@@ -38,18 +38,30 @@ module Gitlab
         def sync_groups?
           return false unless user && group.saml_group_sync_available?
 
-          group_names_from_saml.any? && group_link_ids.any?
+          any_group_links_in_hierarchy?
         end
 
         # rubocop:disable CodeReuse/ActiveRecord
         def group_link_ids
           strong_memoize(:group_link_ids) do
+            next [] if group_names_from_saml.empty?
+
             SamlGroupLink
               .by_saml_group_name(group_names_from_saml)
-              .by_group_id(group.self_and_descendants.select(:id))
+              .by_group_id(group_ids_in_hierarchy)
               .pluck(:id)
           end
         end
+
+        def any_group_links_in_hierarchy?
+          strong_memoize(:group_ids_with_any_links) do
+            SamlGroupLink.by_group_id(group_ids_in_hierarchy).exists?
+          end
+        end
+
+        def group_ids_in_hierarchy
+          group.self_and_descendants.select(:id)
+        end
         # rubocop:enable CodeReuse/ActiveRecord
 
         def group_names_from_saml

ee/spec/lib/gitlab/auth/group_saml/membership_updater_spec.rb

@@ -64,8 +64,8 @@ RSpec.describe Gitlab::Auth::GroupSaml::MembershipUpdater do
       allow(group).to receive(:saml_group_sync_available?).and_return(enabled)
     end
 
-    let(:group_link) { create(:saml_group_link, saml_group_name: 'Owners', group: group) }
-    let(:subgroup_link) { create(:saml_group_link, saml_group_name: 'Developers', group: create(:group, parent: group)) }
+    let!(:group_link) { create(:saml_group_link, saml_group_name: 'Owners', group: group) }
+    let!(:subgroup_link) { create(:saml_group_link, saml_group_name: 'Developers', group: create(:group, parent: group)) }
 
     context 'when group sync is not available' do
       before do
@@ -74,6 +74,8 @@ RSpec.describe Gitlab::Auth::GroupSaml::MembershipUpdater do
 
       it 'does not enqueue group sync' do
         expect(GroupSamlGroupSyncWorker).not_to receive(:perform_async)
+
+        update_membership
       end
     end
 
@@ -99,6 +101,33 @@ RSpec.describe Gitlab::Auth::GroupSaml::MembershipUpdater do
           update_membership
         end
       end
+
+      context 'when auth hash contains no groups' do
+        let!(:auth_hash) do
+          Gitlab::Auth::GroupSaml::AuthHash.new(
+            OmniAuth::AuthHash.new(extra: { raw_info: OneLogin::RubySaml::Attributes.new })
+          )
+        end
+
+        it 'enqueues group sync' do
+          expect(GroupSamlGroupSyncWorker).to receive(:perform_async).with(user.id, group.id, [])
+
+          update_membership
+        end
+      end
+
+      context 'when auth hash groups do not match group links' do
+        before do
+          group_link.update!(saml_group_name: 'Web Developers')
+          subgroup_link.destroy!
+        end
+
+        it 'enqueues group sync' do
+          expect(GroupSamlGroupSyncWorker).to receive(:perform_async).with(user.id, group.id, [])
+
+          update_membership
+        end
+      end
     end
   end
 end

ee/spec/workers/group_saml_group_sync_worker_spec.rb

@@ -87,6 +87,19 @@ RSpec.describe GroupSamlGroupSyncWorker do
             perform([top_level_group_link.id, group_link.id])
           end
         end
+
+        context 'when the worker receives no group link ids' do
+          before do
+            group.add_user(user, Gitlab::Access::DEVELOPER)
+          end
+
+          it 'calls the sync service and removes existing users' do
+            expect_sync_service_call(group_links: [])
+            expect_metadata_logging_call({ added: 0, updated: 0, removed: 1 })
+
+            perform([])
+          end
+        end
       end
     end