Fix LDAP group sync using fix from 4d56877fe8a

Signed-off-by: Rémy Coutable <remy@rymai.me>

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

@@ -99,6 +99,8 @@ module EE
           def update_existing_group_membership(group, access_levels)
             logger.debug { "Updating existing membership for '#{group.name}' group" }
 
+            first_group_owner = group.members.owners.first.try(:user)
+
             select_and_preload_group_members(group).each do |member|
               user = member.user
               identity = user.identities.select(:id, :extern_uid)
@@ -137,7 +139,12 @@ module EE
 
                 next if member.ldap? && member.override?
 
-                add_or_update_user_membership(user, group, desired_access)
+                add_or_update_user_membership(
+                  user,
+                  group,
+                  desired_access,
+                  current_user: first_group_owner
+                )
               elsif group.last_owner?(user)
                 warn_cannot_remove_last_owner(user, group)
               else
@@ -149,11 +156,18 @@ module EE
           def add_new_members(group, access_levels)
             logger.debug { "Adding new members to '#{group.name}' group" }
 
+            first_group_owner = group.members.owners.first.try(:user)
+
             access_levels.each do |member_dn, access_level|
               user = ::Gitlab::LDAP::User.find_by_uid_and_provider(member_dn, provider)
 
               if user.present?
-                add_or_update_user_membership(user, group, access_level)
+                add_or_update_user_membership(
+                  user,
+                  group,
+                  access_level,
+                  current_user: first_group_owner
+                )
               else
                 logger.debug do
                   <<-MSG.strip_heredoc.tr("\n", ' ')
@@ -167,23 +181,19 @@ module EE
             end
           end
 
-          def add_or_update_user_membership(user, group, access)
+          def add_or_update_user_membership(user, group, access, current_user: nil)
             # Prevent the last owner of a group from being demoted
             if access < ::Gitlab::Access::OWNER && group.last_owner?(user)
               warn_cannot_remove_last_owner(user, group)
             else
-              # Temporarily handle access requests until
-              # gitlab-org/gitlab-ee#825 is properly resolved.
-              member = group.requesters.find_by(user_id: user.id)
-              if member.present?
-                member.access_level = access
-                member.requested_at = nil
-                member.save
-              else
-                # If you pass the user object, instead of just user ID,
-                # it saves an extra user database query.
-                group.add_users([user], access, skip_notification: true, ldap: true)
-              end
+              # If you pass the user object, instead of just user ID,
+              # it saves an extra user database query.
+              group.add_user(
+                user,
+                access,
+                current_user: current_user,
+                ldap: true
+              )
             end
           end
 

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

@@ -112,8 +112,12 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
   end
 
   describe '#update_permissions' do
-    before { group.start_ldap_sync }
-    after { group.finish_ldap_sync }
+    before do
+      group.start_ldap_sync
+    end
+    after do
+      group.finish_ldap_sync
+    end
 
     let(:group) do
       create(:group_with_ldap_group_link,
@@ -142,17 +146,15 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
         end
 
         it 'converts an existing membership access request to a real member' do
-          group.members.create(
-            user: user,
-            access_level: ::Gitlab::Access::MASTER,
-            requested_at: DateTime.now
-          )
+          group.add_owner(create(:user))
+          access_requester = group.request_access(user)
+          access_requester.update(access_level: ::Gitlab::Access::MASTER)
           # Validate that the user is properly created as a requester first.
-          expect(group.requesters.pluck(:user_id)).to include(user.id)
+          expect(group.requesters.pluck(:id)).to include(access_requester.id)
 
           sync_group.update_permissions
 
-          expect(group.members.pluck(:user_id)).to include(user.id)
+          expect(group.members.pluck(:id)).to include(access_requester.id)
           expect(group.members.find_by(user_id: user.id).access_level)
             .to eq(::Gitlab::Access::DEVELOPER)
         end
@@ -160,7 +162,7 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
         it 'downgrades existing member access' do
           # Create user with higher access
           group.add_users([user],
-                          ::Gitlab::Access::MASTER, skip_notification: true)
+                          ::Gitlab::Access::MASTER)
 
           sync_group.update_permissions
 
@@ -171,7 +173,7 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
         it 'upgrades existing member access' do
           # Create user with lower access
           group.add_users([user],
-                          ::Gitlab::Access::GUEST, skip_notification: true)
+                          ::Gitlab::Access::GUEST)
 
           sync_group.update_permissions
 
@@ -182,8 +184,7 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
         it 'sets an existing member ldap attribute to true' do
           group.add_users(
             [user],
-            ::Gitlab::Access::DEVELOPER,
-            skip_notification: true
+            ::Gitlab::Access::DEVELOPER
           )
 
           sync_group.update_permissions
@@ -213,7 +214,7 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
 
         it 'removes the user from the group' do
           group.add_users([user],
-                          Gitlab::Access::MASTER, skip_notification: true)
+                          Gitlab::Access::MASTER)
 
           sync_group.update_permissions
 
@@ -222,7 +223,7 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
 
         it 'refuses to delete the last owner' do
           group.add_users([user],
-                          Gitlab::Access::OWNER, skip_notification: true)
+                          Gitlab::Access::OWNER)
 
           sync_group.update_permissions
 
@@ -242,7 +243,7 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
           create(:identity, user: user1, extern_uid: user_dn(user1.username))
           create(:identity, user: user2, extern_uid: user_dn(user2.username))
           group.add_users([user1, user2],
-                          Gitlab::Access::OWNER, skip_notification: true)
+                          Gitlab::Access::OWNER)
 
           sync_group.update_permissions