Delete unauthorized Todos when project is privateissue_49897

Delete Todos for guest users when project visibility
level is updated to private.

9 files changed, 91 insertions, 31 deletions

diff --git a/app/models/todo.rb b/app/models/todo.rb
index 5dcc3e9945a..f1fc5e599eb 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -38,7 +38,9 @@ class Todo < ApplicationRecord
       self
     end
   }, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations
+
   belongs_to :user
+  belongs_to :issue, -> { where("target_type = 'Issue'") }, foreign_key: :target_id
 
   delegate :name, :email, to: :author, prefix: true, allow_nil: true
 
@@ -59,6 +61,7 @@ class Todo < ApplicationRecord
   scope :for_target, -> (id) { where(target_id: id) }
   scope :for_commit, -> (id) { where(commit_id: id) }
   scope :with_api_entity_associations, -> { preload(:target, :author, :note, group: :route, project: [:route, { namespace: :route }]) }
+  scope :joins_issue_and_assignees, -> { left_joins(issue: :assignees) }
 
   state_machine :state, initial: :pending do
     event :done do
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index dfa7bd20254..2bc04470342 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -64,6 +64,7 @@ module Projects
 
       if project.previous_changes.include?(:visibility_level) && project.private?
         # don't enqueue immediately to prevent todos removal in case of a mistake
+        TodosDestroyer::ConfidentialIssueWorker.perform_in(Todo::WAIT_FOR_DELETE, nil, project.id)
         TodosDestroyer::ProjectPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id)
       elsif (project_changed_feature_keys & todos_features_changes).present?
         TodosDestroyer::PrivateFeaturesWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id)
diff --git a/app/services/todos/destroy/base_service.rb b/app/services/todos/destroy/base_service.rb
index f3f1dbb5698..7378f10e7c4 100644
--- a/app/services/todos/destroy/base_service.rb
+++ b/app/services/todos/destroy/base_service.rb
@@ -13,7 +13,7 @@ module Todos
 
       # rubocop: disable CodeReuse/ActiveRecord
       def without_authorized(items)
-        items.where('user_id NOT IN (?)', authorized_users)
+        items.where('todos.user_id NOT IN (?)', authorized_users)
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
diff --git a/app/services/todos/destroy/confidential_issue_service.rb b/app/services/todos/destroy/confidential_issue_service.rb
index 6276e332448..6cdd8c16894 100644
--- a/app/services/todos/destroy/confidential_issue_service.rb
+++ b/app/services/todos/destroy/confidential_issue_service.rb
@@ -2,36 +2,55 @@
 
 module Todos
   module Destroy
+    # Service class for deleting todos that belongs to confidential issues.
+    # It deletes todos for users that are not at least reporters, issue author or assignee.
+    #
+    # Accepts issue_id or project_id as argument.
+    # When issue_id is passed it deletes matching todos for one confidential issue.
+    # When project_id is passed it deletes matching todos for all confidential issues of the project.
     class ConfidentialIssueService < ::Todos::Destroy::BaseService
       extend ::Gitlab::Utils::Override
 
-      attr_reader :issue
+      attr_reader :issues
 
       # rubocop: disable CodeReuse/ActiveRecord
-      def initialize(issue_id)
-        @issue = Issue.find_by(id: issue_id)
+      def initialize(issue_id: nil, project_id: nil)
+        @issues =
+          if issue_id
+            Issue.where(id: issue_id)
+          elsif project_id
+            project_confidential_issues(project_id)
+          end
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
       private
 
+      def project_confidential_issues(project_id)
+        project = Project.find(project_id)
+
+        project.issues.confidential_only
+      end
+
       override :todos
       # rubocop: disable CodeReuse/ActiveRecord
       def todos
-        Todo.where(target: issue)
-          .where('user_id != ?', issue.author_id)
-          .where('user_id NOT IN (?)', issue.assignees.select(:id))
+        Todo.joins_issue_and_assignees
+          .where(target: issues)
+          .where('issues.confidential = ?', true)
+          .where('todos.user_id != issues.author_id')
+          .where('todos.user_id != issue_assignees.user_id')
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
       override :todos_to_remove?
       def todos_to_remove?
-        issue&.confidential?
+        issues&.any?(&:confidential?)
       end
 
       override :project_ids
       def project_ids
-        issue.project_id
+        issues&.distinct&.select(:project_id)
       end
 
       override :authorized_users
diff --git a/app/workers/todos_destroyer/confidential_issue_worker.rb b/app/workers/todos_destroyer/confidential_issue_worker.rb
index 481fde8c83d..240a5f98ad5 100644
--- a/app/workers/todos_destroyer/confidential_issue_worker.rb
+++ b/app/workers/todos_destroyer/confidential_issue_worker.rb
@@ -5,8 +5,8 @@ module TodosDestroyer
     include ApplicationWorker
     include TodosDestroyerQueue
 
-    def perform(issue_id)
-      ::Todos::Destroy::ConfidentialIssueService.new(issue_id).execute
+    def perform(issue_id = nil, project_id = nil)
+      ::Todos::Destroy::ConfidentialIssueService.new(issue_id: issue_id, project_id: project_id).execute
     end
   end
 end
diff --git a/changelogs/unreleased/issue_49897.yml b/changelogs/unreleased/issue_49897.yml
new file mode 100644
index 00000000000..b630b5143c6
--- /dev/null
+++ b/changelogs/unreleased/issue_49897.yml
@@ -0,0 +1,5 @@
+---
+title: Delete unauthorized Todos when project is made private
+merge_request: 28560
+author:
+type: fixed
diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb
index 5ad30b58511..1dcfb739eb6 100644
--- a/spec/services/projects/update_service_spec.rb
+++ b/spec/services/projects/update_service_spec.rb
@@ -45,6 +45,7 @@ describe Projects::UpdateService do
 
         it 'updates the project to private' do
           expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id)
+          expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, nil, project.id)
 
           result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE)
 
diff --git a/spec/services/todos/destroy/confidential_issue_service_spec.rb b/spec/services/todos/destroy/confidential_issue_service_spec.rb
index 78b6744b426..9f7e656f7d3 100644
--- a/spec/services/todos/destroy/confidential_issue_service_spec.rb
+++ b/spec/services/todos/destroy/confidential_issue_service_spec.rb
@@ -9,36 +9,60 @@ describe Todos::Destroy::ConfidentialIssueService do
   let(:assignee)       { create(:user) }
   let(:guest)          { create(:user) }
   let(:project_member) { create(:user) }
-  let(:issue)          { create(:issue, project: project, author: author, assignees: [assignee]) }
-
-  let!(:todo_issue_non_member)   { create(:todo, user: user, target: issue, project: project) }
-  let!(:todo_issue_member)       { create(:todo, user: project_member, target: issue, project: project) }
-  let!(:todo_issue_author)       { create(:todo, user: author, target: issue, project: project) }
-  let!(:todo_issue_asignee)      { create(:todo, user: assignee, target: issue, project: project) }
-  let!(:todo_issue_guest)        { create(:todo, user: guest, target: issue, project: project) }
-  let!(:todo_another_non_member) { create(:todo, user: user, project: project) }
+  let(:issue_1)        { create(:issue, :confidential, project: project, author: author, assignees: [assignee]) }
 
   describe '#execute' do
     before do
       project.add_developer(project_member)
       project.add_guest(guest)
+
+      # todos not to be deleted
+      create(:todo, user: project_member, target: issue_1, project: project)
+      create(:todo, user: author, target: issue_1, project: project)
+      create(:todo, user: assignee, target: issue_1, project: project)
+      create(:todo, user: user, project: project)
+      # Todos to be deleted
+      create(:todo, user: guest, target: issue_1, project: project)
+      create(:todo, user: user, target: issue_1, project: project)
     end
 
-    subject { described_class.new(issue.id).execute }
+    subject { described_class.new(issue_id: issue_1.id).execute }
 
-    context 'when provided issue is confidential' do
-      before do
-        issue.update!(confidential: true)
+    context 'when issue_id parameter is present' do
+      context 'when provided issue is confidential' do
+        it 'removes issue todos for users who can not access the confidential issue' do
+          expect { subject }.to change { Todo.count }.from(6).to(4)
+        end
       end
 
-      it 'removes issue todos for users who can not access the confidential issue' do
-        expect { subject }.to change { Todo.count }.from(6).to(4)
+      context 'when provided issue is not confidential' do
+        it 'does not remove any todos' do
+          issue_1.update(confidential: false)
+
+          expect { subject }.not_to change { Todo.count }
+        end
       end
     end
 
-    context 'when provided issue is not confidential' do
-      it 'does not remove any todos' do
-        expect { subject }.not_to change { Todo.count }
+    context 'when project_id parameter is present' do
+      subject { described_class.new(issue_id: nil, project_id: project.id).execute }
+
+      it 'removes issues todos for users that cannot access confidential issues' do
+        issue_2 = create(:issue, :confidential, project: project)
+        issue_3 = create(:issue, :confidential, project: project, author: author, assignees: [assignee])
+        issue_4 = create(:issue, project: project)
+        # Todos not to be deleted
+        create(:todo, user: guest, target: issue_1, project: project)
+        create(:todo, user: assignee, target: issue_1, project: project)
+        create(:todo, user: project_member, target: issue_2, project: project)
+        create(:todo, user: author, target: issue_3, project: project)
+        create(:todo, user: user, target: issue_4, project: project)
+        create(:todo, user: user, project: project)
+        # Todos to be deleted
+        create(:todo, user: user, target: issue_1, project: project)
+        create(:todo, user: guest, target: issue_2, project: project)
+
+        expect { subject }.to change { Todo.count }.from(14).to(10)
       end
     end
   end
diff --git a/spec/workers/todos_destroyer/confidential_issue_worker_spec.rb b/spec/workers/todos_destroyer/confidential_issue_worker_spec.rb
index 18876b71615..0907e2768ba 100644
--- a/spec/workers/todos_destroyer/confidential_issue_worker_spec.rb
+++ b/spec/workers/todos_destroyer/confidential_issue_worker_spec.rb
@@ -3,12 +3,19 @@
 require 'spec_helper'
 
 describe TodosDestroyer::ConfidentialIssueWorker do
-  it "calls the Todos::Destroy::ConfidentialIssueService with the params it was given" do
-    service = double
+  let(:service) { double }
 
-    expect(::Todos::Destroy::ConfidentialIssueService).to receive(:new).with(100).and_return(service)
+  it "calls the Todos::Destroy::ConfidentialIssueService with issue_id parameter" do
+    expect(::Todos::Destroy::ConfidentialIssueService).to receive(:new).with(issue_id: 100, project_id: nil).and_return(service)
     expect(service).to receive(:execute)
 
     described_class.new.perform(100)
   end
+
+  it "calls the Todos::Destroy::ConfidentialIssueService with project_id parameter" do
+    expect(::Todos::Destroy::ConfidentialIssueService).to receive(:new).with(issue_id: nil, project_id: 100).and_return(service)
+    expect(service).to receive(:execute)
+
+    described_class.new.perform(nil, 100)
+  end
 end