Remove group todos when a users looses access

8 files changed, 156 insertions, 18 deletions

diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
index 436a6b18cb1..fe47aa2f140 100644
--- a/app/services/groups/update_service.rb
+++ b/app/services/groups/update_service.rb
@@ -14,7 +14,9 @@ module Groups
       group.assign_attributes(params)
 
       begin
-        group.save
+        after_update if group.save
+
+        true
       rescue Gitlab::UpdatePathError => e
         group.errors.add(:base, e.message)
 
@@ -24,6 +26,13 @@ module Groups
 
     private
 
+    def after_update
+      if group.previous_changes.include?(:visibility_level) && group.private?
+        # don't enqueue immediately to prevent todos removal in case of a mistake
+        TodosDestroyer::GroupPrivateWorker.perform_in(1.hour, group.id)
+      end
+    end
+
     def reject_parent_id!
       params.except!(:parent_id)
     end
diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb
index 2ff9f94b718..b1c4eb95e87 100644
--- a/app/services/todos/destroy/entity_leave_service.rb
+++ b/app/services/todos/destroy/entity_leave_service.rb
@@ -19,7 +19,7 @@ module Todos
       override :todos
       def todos
         if entity.private?
-          Todo.where(project_id: project_ids, user_id: user_id)
+          Todo.where('(project_id IN (?) OR group_id IN (?)) AND user_id = ?', project_ids, group_ids, user_id)
         else
           project_ids.each do |project_id|
             TodosDestroyer::PrivateFeaturesWorker.perform_async(project_id, user_id)
@@ -37,7 +37,16 @@ module Todos
         when Project
           [entity.id]
         when Namespace
-          Project.select(:id).where(namespace_id: entity.self_and_descendants.select(:id))
+          Project.select(:id).where(namespace_id: group_ids)
+        end
+      end
+
+      def group_ids
+        case entity
+        when Project
+          []
+        when Namespace
+          entity.self_and_descendants.select(:id)
         end
       end
 
diff --git a/app/services/todos/destroy/group_private_service.rb b/app/services/todos/destroy/group_private_service.rb
new file mode 100644
index 00000000000..1b00bbe91e1
--- /dev/null
+++ b/app/services/todos/destroy/group_private_service.rb
@@ -0,0 +1,30 @@
+module Todos
+  module Destroy
+    class GroupPrivateService < ::Todos::Destroy::BaseService
+      extend ::Gitlab::Utils::Override
+
+      attr_reader :group
+
+      def initialize(group_id)
+        @group = Group.find_by(id: group_id)
+      end
+
+      private
+
+      override :todos
+      def todos
+        Todo.where(group_id: group.id)
+      end
+
+      override :authorized_users
+      def authorized_users
+        GroupMember.select(:user_id).where(source: group.id)
+      end
+
+      override :todos_to_remove?
+      def todos_to_remove?
+        group&.private?
+      end
+    end
+  end
+end
diff --git a/app/workers/todos_destroyer/group_private_worker.rb b/app/workers/todos_destroyer/group_private_worker.rb
new file mode 100644
index 00000000000..3e47eec7461
--- /dev/null
+++ b/app/workers/todos_destroyer/group_private_worker.rb
@@ -0,0 +1,10 @@
+module TodosDestroyer
+  class GroupPrivateWorker
+    include ApplicationWorker
+    include TodosDestroyerQueue
+
+    def perform(group_id)
+      ::Todos::Destroy::GroupPrivateService.new(group_id).execute
+    end
+  end
+end
diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb
index 48d689e11d4..7c5c7409cc1 100644
--- a/spec/services/groups/update_service_spec.rb
+++ b/spec/services/groups/update_service_spec.rb
@@ -12,13 +12,17 @@ describe Groups::UpdateService do
         let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL) }
 
         before do
-          public_group.add_user(user, Gitlab::Access::MAINTAINER)
+          public_group.add_user(user, Gitlab::Access::OWNER)
           create(:project, :public, group: public_group)
+
+          expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in)
         end
 
         it "does not change permission level" do
           service.execute
           expect(public_group.errors.count).to eq(1)
+
+          expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in)
         end
       end
 
@@ -26,8 +30,10 @@ describe Groups::UpdateService do
         let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) }
 
         before do
-          internal_group.add_user(user, Gitlab::Access::MAINTAINER)
+          internal_group.add_user(user, Gitlab::Access::OWNER)
           create(:project, :internal, group: internal_group)
+
+          expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in)
         end
 
         it "does not change permission level" do
@@ -35,6 +41,24 @@ describe Groups::UpdateService do
           expect(internal_group.errors.count).to eq(1)
         end
       end
+
+      context "internal group with private project" do
+        let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) }
+
+        before do
+          internal_group.add_user(user, Gitlab::Access::OWNER)
+          create(:project, :private, group: internal_group)
+
+          expect(TodosDestroyer::GroupPrivateWorker).to receive(:perform_in)
+            .with(1.hour, internal_group.id)
+        end
+
+        it "changes permission level to private" do
+          service.execute
+          expect(internal_group.visibility_level)
+            .to eq(Gitlab::VisibilityLevel::PRIVATE)
+        end
+      end
     end
 
     context "with parent_id user doesn't have permissions for" do
diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb
index bad408a314e..c554db5e024 100644
--- a/spec/services/todos/destroy/entity_leave_service_spec.rb
+++ b/spec/services/todos/destroy/entity_leave_service_spec.rb
@@ -10,18 +10,20 @@ describe Todos::Destroy::EntityLeaveService do
 
   let!(:todo_mr_user)     { create(:todo, user: user, target: mr, project: project) }
   let!(:todo_issue_user)  { create(:todo, user: user, target: issue, project: project) }
+  let!(:todo_group_user)  { create(:todo, user: user, group: group) }
   let!(:todo_issue_user2) { create(:todo, user: user2, target: issue, project: project) }
+  let!(:todo_group_user2) { create(:todo, user: user2, group: group) }
 
   describe '#execute' do
     context 'when a user leaves a project' do
       subject { described_class.new(user.id, project.id, 'Project').execute }
 
       context 'when project is private' do
-        it 'removes todos for the provided user' do
-          expect { subject }.to change { Todo.count }.from(3).to(1)
+        it 'removes project todos for the provided user' do
+          expect { subject }.to change { Todo.count }.from(5).to(3)
 
-          expect(user.todos).to be_empty
-          expect(user2.todos).to match_array([todo_issue_user2])
+          expect(user.todos).to match_array([todo_group_user])
+          expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2])
         end
       end
 
@@ -37,7 +39,7 @@ describe Todos::Destroy::EntityLeaveService do
           end
 
           it 'removes only confidential issues todos' do
-            expect { subject }.to change { Todo.count }.from(3).to(2)
+            expect { subject }.to change { Todo.count }.from(5).to(4)
           end
         end
 
@@ -69,7 +71,7 @@ describe Todos::Destroy::EntityLeaveService do
             end
 
             it 'removes only users issue todos' do
-              expect { subject }.to change { Todo.count }.from(3).to(2)
+              expect { subject }.to change { Todo.count }.from(5).to(4)
             end
           end
         end
@@ -80,26 +82,30 @@ describe Todos::Destroy::EntityLeaveService do
       subject { described_class.new(user.id, group.id, 'Group').execute }
 
       context 'when group is private' do
-        it 'removes todos for the user' do
-          expect { subject }.to change { Todo.count }.from(3).to(1)
+        it 'removes group and subproject todos for the user' do
+          expect { subject }.to change { Todo.count }.from(5).to(2)
 
           expect(user.todos).to be_empty
-          expect(user2.todos).to match_array([todo_issue_user2])
+          expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2])
         end
 
         context 'with nested groups', :nested_groups do
           let(:subgroup) { create(:group, :private, parent: group) }
           let(:subproject) { create(:project, group: subgroup) }
 
-          let!(:todo_subproject_user) { create(:todo, user: user, project: subproject) }
+          let!(:todo_subproject_user)  { create(:todo, user: user, project: subproject) }
+          let!(:todo_subgroup_user)    { create(:todo, user: user, group: subgroup) }
           let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) }
+          let!(:todo_subpgroup_user2)  { create(:todo, user: user2, group: subgroup) }
 
           it 'removes todos for the user including subprojects todos' do
-            expect { subject }.to change { Todo.count }.from(5).to(2)
+            expect { subject }.to change { Todo.count }.from(9).to(4)
 
             expect(user.todos).to be_empty
             expect(user2.todos)
-              .to match_array([todo_issue_user2, todo_subproject_user2])
+              .to match_array(
+                [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
+              )
           end
         end
       end
@@ -113,7 +119,7 @@ describe Todos::Destroy::EntityLeaveService do
         end
 
         it 'removes only confidential issues todos' do
-          expect { subject }.to change { Todo.count }.from(3).to(2)
+          expect { subject }.to change { Todo.count }.from(5).to(4)
         end
       end
     end
diff --git a/spec/services/todos/destroy/group_private_service_spec.rb b/spec/services/todos/destroy/group_private_service_spec.rb
new file mode 100644
index 00000000000..48e5a5a91eb
--- /dev/null
+++ b/spec/services/todos/destroy/group_private_service_spec.rb
@@ -0,0 +1,38 @@
+require 'spec_helper'
+
+describe Todos::Destroy::GroupPrivateService do
+  let(:group)        { create(:group, :public) }
+  let(:user)         { create(:user) }
+  let(:group_member) { create(:user) }
+
+  let!(:todo_non_member)   { create(:todo, user: user, group: group) }
+  let!(:todo_member)       { create(:todo, user: group_member, group: group) }
+  let!(:todo_another_non_member) { create(:todo, user: user, group: group) }
+
+  describe '#execute' do
+    before do
+      group.add_developer(group_member)
+    end
+
+    subject { described_class.new(group.id).execute }
+
+    context 'when a project set to private' do
+      before do
+        group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      it 'removes todos for a user who is not a member' do
+        expect { subject }.to change { Todo.count }.from(3).to(1)
+
+        expect(user.todos).to be_empty
+        expect(group_member.todos).to match_array([todo_member])
+      end
+    end
+
+    context 'when group is not private' do
+      it 'does not remove any todos' do
+        expect { subject }.not_to change { Todo.count }
+      end
+    end
+  end
+end
diff --git a/spec/workers/todos_destroyer/group_private_worker_spec.rb b/spec/workers/todos_destroyer/group_private_worker_spec.rb
new file mode 100644
index 00000000000..fcc38989ced
--- /dev/null
+++ b/spec/workers/todos_destroyer/group_private_worker_spec.rb
@@ -0,0 +1,12 @@
+require 'spec_helper'
+
+describe TodosDestroyer::GroupPrivateWorker do
+  it "calls the Todos::Destroy::GroupPrivateService with the params it was given" do
+    service = double
+
+    expect(::Todos::Destroy::GroupPrivateService).to receive(:new).with(100).and_return(service)
+    expect(service).to receive(:execute)
+
+    described_class.new.perform(100)
+  end
+end