Merge pull request #9739 from opf/fix/39120/synchronized-group-saving

[39120] Ensure synchronized group is saved with the filter

modules/ldap_groups/app/services/ldap_groups/synchronize_filter_service.rb

@@ -69,7 +69,8 @@ module LdapGroups
     ##
     # Create or update the synchronized group item
     def create_or_update_sync_group(dn)
-      LdapGroups::SynchronizedGroup.find_or_initialize_by(dn: dn).tap do |sync|
+      group = LdapGroups::SynchronizedGroup
+        .find_or_initialize_by(dn: dn).tap do |sync|
         # Always set the filter and auth source, in case multiple filters match the same group
         # they are simply being re-assigned to the latest one
         sync.filter_id = filter.id
@@ -78,6 +79,8 @@ module LdapGroups
         # Tell the group to synchronize users if the filter has requested it to
         sync.sync_users = filter.sync_users
       end
+
+      group.tap { group.save! if group.persisted? }
     end
 
     ##

modules/ldap_groups/spec/factories/synchronized_filter_factory.rb

@@ -5,5 +5,6 @@ FactoryBot.define do
     group_name_attribute { 'cn' }
     base_dn { 'dc=example,dc=com' }
     auth_source factory: :ldap_auth_source
+    sync_users { true }
   end
 end

modules/ldap_groups/spec/factories/synchronized_group_factory.rb

@@ -3,5 +3,6 @@ FactoryBot.define do
     dn { 'cn=foo,ou=groups,dc=example,dc=com' }
     group factory: :group
     auth_source factory: :ldap_auth_source
+    sync_users { true }
   end
 end

modules/ldap_groups/spec/services/synchronize_filter_spec.rb

@@ -29,11 +29,11 @@ describe LdapGroups::SynchronizeFilterService, with_ee: %i[ldap_groups] do
 
   let(:synced_foo) do
     FactoryBot.create :ldap_synchronized_group, dn: 'cn=foo,ou=groups,dc=example,dc=com', group: group_foo,
-                                                auth_source: auth_source
+                      auth_source: auth_source
   end
   let(:synced_bar) do
     FactoryBot.create :ldap_synchronized_group, dn: 'cn=bar,ou=groups,dc=example,dc=com', group: group_bar,
-                                                auth_source: auth_source
+                      auth_source: auth_source
   end
 
   let(:filter_foo_bar) { FactoryBot.create :ldap_synchronized_filter, auth_source: auth_source }
@@ -83,6 +83,33 @@ describe LdapGroups::SynchronizeFilterService, with_ee: %i[ldap_groups] do
     end
   end
 
+  describe 'when one group already exists with different settings' do
+    let(:synced_foo) do
+      FactoryBot.create :ldap_synchronized_group,
+                        dn: 'cn=foo,ou=groups,dc=example,dc=com',
+                        group: group_foo,
+                        sync_users: false,
+                        auth_source: auth_source
+    end
+    let(:filter_foo_bar) do
+      FactoryBot.create :ldap_synchronized_filter,
+                        sync_users: true,
+                        auth_source: auth_source
+    end
+
+    before do
+      synced_foo
+    end
+
+    it 'the group receives the value of the filter' do
+      expect(synced_foo.sync_users).to eq false
+      expect { subject }.not_to raise_error
+
+      synced_foo.reload
+      expect(synced_foo.sync_users).to eq true
+    end
+  end
+
   describe 'when it has a group that no longer exists in ldap' do
     let!(:group_doesnotexist) { FactoryBot.create :group, lastname: 'doesnotexist' }
     let!(:synced_doesnotexist) do