Merge branch '325422-cleanup-unaccessible-todos-when-user-leaves-public-project' into 'master'

Cleanup unaccessible todos when user leaves public project

See merge request gitlab-org/gitlab!74829

app/models/issue.rb

@@ -434,8 +434,6 @@ class Issue < ApplicationRecord
   # Returns `true` if the current issue can be viewed by either a logged in User
   # or an anonymous user.
   def visible_to_user?(user = nil)
-    return false unless project && project.feature_available?(:issues, user)
-
     return publicly_visible? unless user
 
     return false unless readable_by?(user)
@@ -563,10 +561,10 @@ class Issue < ApplicationRecord
       project.team.member?(user, Gitlab::Access::REPORTER)
     elsif hidden?
       false
+    elsif project.public? || (project.internal? && !user.external?)
+      project.feature_available?(:issues, user)
     else
-      project.public? ||
-        project.internal? && !user.external? ||
-        project.team.member?(user)
+      project.team.member?(user)
     end
   end
 

app/models/todo.rb

@@ -3,6 +3,7 @@
 class Todo < ApplicationRecord
   include Sortable
   include FromUnion
+  include EachBatch
 
   # Time to wait for todos being removed when not visible for user anymore.
   # Prevents TODOs being removed by mistake, for example, removing access from a user

app/services/todos/destroy/private_features_service.rb

-# frozen_string_literal: true
-
-module Todos
-  module Destroy
-    class PrivateFeaturesService < ::Todos::Destroy::BaseService
-      attr_reader :project_ids, :user_id
-
-      def initialize(project_ids, user_id = nil)
-        @project_ids = project_ids
-        @user_id = user_id
-      end
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def execute
-        ProjectFeature.where(project_id: project_ids).each do |project_features|
-          target_types = []
-          target_types << Issue.name if private?(project_features.issues_access_level)
-          target_types << MergeRequest.name if private?(project_features.merge_requests_access_level)
-          target_types << Commit.name if private?(project_features.repository_access_level)
-
-          next if target_types.empty?
-
-          remove_todos(project_features.project_id, target_types)
-        end
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-
-      private
-
-      def private?(feature_level)
-        feature_level == ProjectFeature::PRIVATE
-      end
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def remove_todos(project_id, target_types)
-        items = Todo.where(project_id: project_id)
-        items = items.where(user_id: user_id) if user_id
-
-        items.where.not(user_id: authorized_users)
-          .where(target_type: target_types)
-          .delete_all
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-    end
-  end
-end

app/services/todos/destroy/unauthorized_features_service.rb

+# frozen_string_literal: true
+
+module Todos
+  module Destroy
+    class UnauthorizedFeaturesService < ::Todos::Destroy::BaseService
+      attr_reader :project_id, :user_id
+
+      BATCH_SIZE = 1000
+
+      def initialize(project_id, user_id = nil)
+        @project_id = project_id
+        @user_id = user_id
+      end
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      def execute
+        return if user_id && authorized_users.where(user_id: user_id).exists?
+
+        related_todos.each_batch(of: BATCH_SIZE) do |batch|
+          pending_delete = without_authorized(batch).includes(:target, :user).reject do |todo|
+            Ability.allowed?(todo.user, :read_todo, todo, scope: :user)
+          end
+          Todo.where(id: pending_delete).delete_all if pending_delete.present?
+        end
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      private
+
+      def related_todos
+        base_scope = Todo.for_project(project_id)
+        base_scope = base_scope.for_user(user_id) if user_id
+        base_scope
+      end
+
+      # Compatibility for #authorized_users in this class we always work
+      # with 1 project for queries efficiency
+      def project_ids
+        [project_id]
+      end
+    end
+  end
+end

app/workers/todos_destroyer/private_features_worker.rb

@@ -10,7 +10,7 @@ module TodosDestroyer
     include TodosDestroyerQueue
 
     def perform(project_id, user_id = nil)
-      ::Todos::Destroy::PrivateFeaturesService.new(project_id, user_id).execute
+      ::Todos::Destroy::UnauthorizedFeaturesService.new(project_id, user_id).execute
     end
   end
 end

db/post_migrate/20211125120444_add_index_todos_project_id_user_id.rb

+# frozen_string_literal: true
+
+class AddIndexTodosProjectIdUserId < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_todos_on_project_id_and_user_id_and_id'
+
+  def up
+    add_concurrent_index :todos, [:project_id, :user_id, :id], name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :todos, INDEX_NAME
+  end
+end

db/post_migrate/20211202145237_add_todos_project_and_id_index.rb

+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddTodosProjectAndIdIndex < Gitlab::Database::Migration[1.0]
+  # When using the methods "add_concurrent_index" or "remove_concurrent_index"
+  # you must disable the use of transactions
+  # as these methods can not run in an existing transaction.
+  # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure
+  # that either of them is the _only_ method called in the migration,
+  # any other changes should go in a separate migration.
+  # This ensures that upon failure _only_ the index creation or removing fails
+  # and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  disable_ddl_transaction!
+
+  NEW_INDEX_NAME = 'index_todos_on_project_id_and_id'
+  OLD_INDEX_NAME = 'index_todos_on_project_id'
+
+  def up
+    add_concurrent_index :todos, [:project_id, :id], name: NEW_INDEX_NAME
+    remove_concurrent_index_by_name :todos, OLD_INDEX_NAME
+  end
+
+  def down
+    add_concurrent_index :todos, :project_id, name: OLD_INDEX_NAME
+    remove_concurrent_index_by_name :todos, NEW_INDEX_NAME
+  end
+end

db/schema_migrations/20211125120444

+19062282d022e5d93cd525cff44c67f1fbc5557f1201e523a57725dc0b6ecd70
\ No newline at end of file

db/schema_migrations/20211202145237

+d109142aa838faedcd307f6cd235c969ca265813493eef50d63cbc2fe5d203b3
\ No newline at end of file

db/structure.sql

@@ -27543,7 +27543,9 @@ CREATE INDEX index_todos_on_group_id ON todos USING btree (group_id);
 
 CREATE INDEX index_todos_on_note_id ON todos USING btree (note_id);
 
-CREATE INDEX index_todos_on_project_id ON todos USING btree (project_id);
+CREATE INDEX index_todos_on_project_id_and_id ON todos USING btree (project_id, id);
+
+CREATE INDEX index_todos_on_project_id_and_user_id_and_id ON todos USING btree (project_id, user_id, id);
 
 CREATE INDEX index_todos_on_target_type_and_target_id ON todos USING btree (target_type, target_id);
 

spec/models/issue_spec.rb

@@ -987,6 +987,7 @@ RSpec.describe Issue do
           issue = build(:issue, project: project)
           user = build(:user)
 
+          allow(::Gitlab::ExternalAuthorization).to receive(:access_allowed?).with(user, 'a-label', project.full_path).and_call_original
           expect(::Gitlab::ExternalAuthorization).to receive(:access_allowed?).with(user, 'a-label') { false }
           expect(issue.visible_to_user?(user)).to be_falsy
         end
@@ -1020,6 +1021,7 @@ RSpec.describe Issue do
               issue = build(:issue, project: project)
               user = build(:admin)
 
+              allow(::Gitlab::ExternalAuthorization).to receive(:access_allowed?).with(user, 'a-label', project.full_path).and_call_original
               expect(::Gitlab::ExternalAuthorization).to receive(:access_allowed?).with(user, 'a-label') { false }
               expect(issue.visible_to_user?(user)).to be_falsy
             end

spec/services/todos/destroy/private_features_service_spec.rb → spec/services/todos/destroy/unauthorized_features_service_spec.rb

@@ -2,13 +2,17 @@
 
 require 'spec_helper'
 
-RSpec.describe Todos::Destroy::PrivateFeaturesService do
-  let(:project)        { create(:project, :public) }
-  let(:user)           { create(:user) }
-  let(:another_user)   { create(:user) }
-  let(:project_member) { create(:user) }
-  let(:issue)          { create(:issue, project: project) }
-  let(:mr)             { create(:merge_request, source_project: project) }
+RSpec.describe Todos::Destroy::UnauthorizedFeaturesService do
+  let_it_be(:project, reload: true) { create(:project, :public, :repository) }
+  let_it_be(:issue)          { create(:issue, project: project) }
+  let_it_be(:mr)             { create(:merge_request, source_project: project) }
+  let_it_be(:user)           { create(:user) }
+  let_it_be(:another_user)   { create(:user) }
+  let_it_be(:project_member) do
+    create(:user).tap do |user|
+      project.add_developer(user)
+    end
+  end
 
   let!(:todo_mr_non_member)      { create(:todo, user: user, target: mr, project: project) }
   let!(:todo_mr_non_member2)     { create(:todo, user: another_user, target: mr, project: project) }
@@ -20,10 +24,6 @@ RSpec.describe Todos::Destroy::PrivateFeaturesService do
   let!(:commit_todo_non_member2) { create(:on_commit_todo, user: another_user, project: project) }
   let!(:commit_todo_member)      { create(:on_commit_todo, user: project_member, project: project) }
 
-  before do
-    project.add_developer(project_member)
-  end
-
   context 'when user_id is provided' do
     subject { described_class.new(project.id, user.id).execute }
 

spec/workers/todos_destroyer/private_features_worker_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe TodosDestroyer::PrivateFeaturesWorker do
   it "calls the Todos::Destroy::PrivateFeaturesService with the params it was given" do
     service = double
 
-    expect(::Todos::Destroy::PrivateFeaturesService).to receive(:new).with(100, nil).and_return(service)
+    expect(::Todos::Destroy::UnauthorizedFeaturesService).to receive(:new).with(100, nil).and_return(service)
     expect(service).to receive(:execute)
 
     described_class.new.perform(100)