Ensure last group owner isn't removed on expiry

2 files changed, 48 insertions, 13 deletions

diff --git a/app/services/members/authorized_destroy_service.rb b/app/services/members/authorized_destroy_service.rb
index c23f90a6a10..ca9db59cac7 100644
--- a/app/services/members/authorized_destroy_service.rb
+++ b/app/services/members/authorized_destroy_service.rb
@@ -7,6 +7,8 @@ module Members
     end
 
     def execute
+      return false if member.is_a?(GroupMember) && member.source.last_owner?(member.user)
+
       member.destroy
 
       if member.request? && member.user != user
diff --git a/spec/workers/remove_expired_members_worker_spec.rb b/spec/workers/remove_expired_members_worker_spec.rb
index 75a5bee7ab4..a6ce926b925 100644
--- a/spec/workers/remove_expired_members_worker_spec.rb
+++ b/spec/workers/remove_expired_members_worker_spec.rb
@@ -1,25 +1,58 @@
 require 'spec_helper'
 
 describe RemoveExpiredMembersWorker do
-  let!(:worker) { RemoveExpiredMembersWorker.new }
-  let!(:expired_member) { create(:project_member, expires_at: 1.hour.ago) }
-  let!(:member_expiring_in_future) { create(:project_member, expires_at: 10.days.from_now) }
-  let!(:non_expiring_member) { create(:project_member, expires_at: nil) }
+  let(:worker) { RemoveExpiredMembersWorker.new }
 
   describe '#perform' do
-    it 'removes expired members' do
-      expect { worker.perform }.to change { Member.count }.by(-1)
-      expect(Member.find_by(id: expired_member.id)).to be_nil
+    context 'project members' do
+      let!(:expired_project_member) { create(:project_member, expires_at: 1.hour.ago, access_level: GroupMember::DEVELOPER) }
+      let!(:project_member_expiring_in_future) { create(:project_member, expires_at: 10.days.from_now, access_level: GroupMember::DEVELOPER) }
+      let!(:non_expiring_project_member) { create(:project_member, expires_at: nil, access_level: GroupMember::DEVELOPER) }
+
+      it 'removes expired members' do
+        expect { worker.perform }.to change { Member.count }.by(-1)
+        expect(Member.find_by(id: expired_project_member.id)).to be_nil
+      end
+
+      it 'leaves members who expire in the future' do
+        worker.perform
+        expect(project_member_expiring_in_future.reload).to be_present
+      end
+
+      it 'leaves members who do not expire at all' do
+        worker.perform
+        expect(non_expiring_project_member.reload).to be_present
+      end
     end
 
-    it 'leaves members who expire in the future' do
-      worker.perform
-      expect(member_expiring_in_future.reload).to be_present
+    context 'group members' do
+      let!(:expired_group_member) { create(:group_member, expires_at: 1.hour.ago, access_level: GroupMember::DEVELOPER) }
+      let!(:group_member_expiring_in_future) { create(:group_member, expires_at: 10.days.from_now, access_level: GroupMember::DEVELOPER) }
+      let!(:non_expiring_group_member) { create(:group_member, expires_at: nil, access_level: GroupMember::DEVELOPER) }
+
+      it 'removes expired members' do
+        expect { worker.perform }.to change { Member.count }.by(-1)
+        expect(Member.find_by(id: expired_group_member.id)).to be_nil
+      end
+
+      it 'leaves members who expire in the future' do
+        worker.perform
+        expect(group_member_expiring_in_future.reload).to be_present
+      end
+
+      it 'leaves members who do not expire at all' do
+        worker.perform
+        expect(non_expiring_group_member.reload).to be_present
+      end
     end
 
-    it 'leaves members who do not expire at all' do
-      worker.perform
-      expect(non_expiring_member.reload).to be_present
+    context 'when the last group owner expires' do
+      let!(:expired_group_owner) { create(:group_member, expires_at: 1.hour.ago, access_level: GroupMember::OWNER) }
+
+      it 'does not delete the owner' do
+        worker.perform
+        expect(expired_group_owner.reload).to be_present
+      end
     end
   end
 end