Merge branch 'security-confepic-todo2' into 'master'

Cleanup confidential epic todos

See merge request gitlab-org/security/gitlab!1066

app/services/todos/destroy/entity_leave_service.rb

@@ -22,7 +22,7 @@ module Todos
         # if at least reporter, all entities including confidential issues can be accessed
         return if user_has_reporter_access?
 
-        remove_confidential_issue_todos
+        remove_confidential_resource_todos
 
         if entity.private?
           remove_project_todos
@@ -40,7 +40,7 @@ module Todos
         end
       end
 
-      def remove_confidential_issue_todos
+      def remove_confidential_resource_todos
         Todo
           .for_target(confidential_issues.select(:id))
           .for_type(Issue.name)
@@ -133,3 +133,5 @@ module Todos
     end
   end
 end
+
+Todos::Destroy::EntityLeaveService.prepend_if_ee('EE::Todos::Destroy::EntityLeaveService')

config/feature_categories.yml

@@ -43,6 +43,7 @@
 - dynamic_application_security_testing
 - editor_extension
 - epics
+- epic_tracking
 - error_tracking
 - feature_flags
 - five_minute_production_app

db/post_migrate/20201109114603_schedule_remove_inaccessible_epic_todos.rb

+# frozen_string_literal: true
+
+class ScheduleRemoveInaccessibleEpicTodos < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INTERVAL = 2.minutes
+  BATCH_SIZE = 10
+  MIGRATION = 'RemoveInaccessibleEpicTodos'
+
+  disable_ddl_transaction!
+
+  class Epic < ActiveRecord::Base
+    include EachBatch
+  end
+
+  def up
+    return unless Gitlab.ee?
+
+    relation = Epic.where(confidential: true)
+
+    queue_background_migration_jobs_by_range_at_intervals(
+      relation, MIGRATION, INTERVAL, batch_size: BATCH_SIZE)
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema_migrations/20201109114603

+ae8034ec52df47ce2ce3397715dd18347e4d297a963c17c7b26321f414dfa632
\ No newline at end of file

doc/user/todos.md

@@ -64,7 +64,7 @@ To-do item triggers aren't affected by [GitLab notification email settings](prof
 
 NOTE: **Note:**
 When a user no longer has access to a resource related to a to-do item (such as
-an issue, merge request, project, or group), for security reasons GitLab
+an issue, merge request, epic, project, or group), for security reasons GitLab
 deletes any related to-do items within the next hour. Deletion is delayed to
 prevent data loss, in the case where a user's access is accidentally revoked.
 

ee/app/services/ee/todos/destroy/entity_leave_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Todos
+    module Destroy
+      module EntityLeaveService
+        extend ActiveSupport::Concern
+        extend ::Gitlab::Utils::Override
+
+        override :remove_confidential_resource_todos
+        def remove_confidential_resource_todos
+          super
+
+          return unless entity.is_a?(Namespace)
+
+          ::Todo
+            .for_target(confidential_epics.select(:id))
+            .for_type(::Epic.name)
+            .for_user(user)
+            .delete_all
+        end
+
+        private
+
+        def confidential_epics
+          ::Epic
+            .in_selected_groups(non_authorized_reporter_groups)
+            .confidential
+        end
+      end
+    end
+  end
+end

ee/app/services/epics/update_service.rb

@@ -34,6 +34,11 @@ module Epics
       end
 
       todo_service.update_epic(epic, current_user, old_mentioned_users)
+
+      if epic.previous_changes.include?('confidential') && epic.confidential?
+        # don't enqueue immediately to prevent todos removal in case of a mistake
+        ::TodosDestroyer::ConfidentialEpicWorker.perform_in(::Todo::WAIT_FOR_DELETE, epic.id)
+      end
     end
 
     def handle_task_changes(epic)

ee/app/services/todos/destroy/confidential_epic_service.rb

+# frozen_string_literal: true
+
+module Todos
+  module Destroy
+    # Service class for deleting todos that belong to confidential epics.
+    # It deletes todos for users that are not at least reporters.
+    class ConfidentialEpicService < ::Todos::Destroy::BaseService
+      extend ::Gitlab::Utils::Override
+
+      attr_reader :epic
+
+      def initialize(epic_id:)
+        @epic = ::Epic.find_by_id(epic_id)
+      end
+
+      private
+
+      override :todos
+      def todos
+        epic.todos
+      end
+
+      override :todos_to_remove?
+      def todos_to_remove?
+        epic&.confidential?
+      end
+
+      override :authorized_users
+      def authorized_users
+        epic.group.members_with_parents.non_guests.select(:user_id)
+      end
+    end
+  end
+end

ee/app/workers/all_queues.yml

@@ -581,6 +581,14 @@
   :weight: 2
   :idempotent:
   :tags: []
+- :name: todos_destroyer:todos_destroyer_confidential_epic
+  :feature_category: :epic_tracking
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent:
+  :tags: []
 - :name: adjourned_project_deletion
   :feature_category: :authentication_and_authorization
   :has_external_dependencies:

ee/app/workers/todos_destroyer/confidential_epic_worker.rb

+# frozen_string_literal: true
+
+module TodosDestroyer
+  class ConfidentialEpicWorker # rubocop:disable Scalability/IdempotentWorker
+    include ApplicationWorker
+
+    queue_namespace :todos_destroyer
+    feature_category :epic_tracking
+
+    def perform(epic_id)
+      return unless epic_id
+
+      ::Todos::Destroy::ConfidentialEpicService.new(epic_id: epic_id).execute
+    end
+  end
+end

ee/changelogs/unreleased/security-confepic-todo2.yml

+---
+title: Cleanup todos for confidential epics that are no longer accessible by the user
+merge_request:
+author:
+type: security

ee/lib/ee/gitlab/background_migration/remove_inaccessible_epic_todos.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module EE
+  module Gitlab
+    module BackgroundMigration
+      module RemoveInaccessibleEpicTodos
+        extend ::Gitlab::Utils::Override
+
+        class User < ActiveRecord::Base
+        end
+
+        class Todo < ActiveRecord::Base
+          belongs_to :epic, foreign_key: :target_id
+          belongs_to :user
+        end
+
+        class Member < ActiveRecord::Base
+          include FromUnion
+
+          self.inheritance_column = :_type_disabled
+        end
+
+        class GroupGroupLink < ActiveRecord::Base
+        end
+
+        class Epic < ActiveRecord::Base
+          belongs_to :group
+
+          def can_read_confidential?(user)
+            group.max_member_access_for_user(user) >= ::Gitlab::Access::REPORTER
+          end
+        end
+
+        class Group < ActiveRecord::Base
+          self.table_name = 'namespaces'
+          self.inheritance_column = :_type_disabled
+
+          def max_member_access_for_user(user)
+            max_member_access = members_with_parents.where(user_id: user)
+                                  .reorder(access_level: :desc)
+                                  .first
+                                  &.access_level
+
+            max_member_access || ::Gitlab::Access::NO_ACCESS
+          end
+
+          def members_with_parents
+            group_hierarchy_members = Member.where(source_type: 'Namespace', source_id: source_ids)
+
+            Member.from_union([group_hierarchy_members,
+                               members_from_self_and_ancestor_group_shares])
+          end
+
+          # rubocop:disable Metrics/AbcSize
+          # this is taken from Group model, so instead of doing additional
+          # refactoring let's keep it close to the original
+          def members_from_self_and_ancestor_group_shares
+            group_group_link_table = GroupGroupLink.arel_table
+            group_member_table = Member.arel_table
+
+            group_group_links_query = GroupGroupLink.where(shared_group_id: source_ids)
+            cte = ::Gitlab::SQL::CTE.new(:group_group_links_cte, group_group_links_query)
+            cte_alias = cte.table.alias(GroupGroupLink.table_name)
+
+            # Instead of members.access_level, we need to maximize that access_level at
+            # the respective group_group_links.group_access.
+            member_columns = Member.attribute_names.map do |column_name|
+              if column_name == 'access_level'
+                smallest_value_arel([cte_alias[:group_access], group_member_table[:access_level]],
+                                    'access_level')
+              else
+                group_member_table[column_name]
+              end
+            end
+
+            Member
+              .with(cte.to_arel)
+              .select(*member_columns)
+              .from([group_member_table, cte.alias_to(group_group_link_table)])
+              .where(group_member_table[:requested_at].eq(nil))
+              .where(group_member_table[:source_id].eq(group_group_link_table[:shared_with_group_id]))
+              .where(group_member_table[:source_type].eq('Namespace'))
+          end
+          # rubocop:enable Metrics/AbcSize
+
+          def source_ids
+            return id unless parent_id
+
+            ::Gitlab::ObjectHierarchy
+              .new(self.class.where(id: id))
+              .base_and_ancestors
+              .reorder(nil).select(:id)
+          end
+
+          def smallest_value_arel(args, column_alias)
+            Arel::Nodes::As.new(
+              Arel::Nodes::NamedFunction.new('LEAST', args),
+              Arel::Nodes::SqlLiteral.new(column_alias))
+          end
+        end
+
+        override :perform
+        def perform(start_id, stop_id)
+          confidential_epic_ids = Epic.where(confidential: true).where(id: start_id..stop_id).ids
+          epic_todos = Todo
+            .where(target_type: 'Epic', target_id: confidential_epic_ids)
+            .includes(:epic, :user)
+          ids_to_delete = not_readable_epic_todo_ids(epic_todos)
+
+          logger.info(message: 'Deleting confidential epic todos', todo_ids: ids_to_delete)
+          Todo.where(id: ids_to_delete).delete_all
+        end
+
+        private
+
+        def not_readable_epic_todo_ids(todos)
+          todos.map do |todo|
+            next todo.id unless todo.epic
+            next if todo.epic.can_read_confidential?(todo.user)
+
+            todo.id
+          end.compact
+        end
+
+        def logger
+          @logger ||= ::Gitlab::BackgroundMigration::Logger.build
+        end
+      end
+    end
+  end
+end

ee/spec/lib/ee/gitlab/background_migration/remove_inaccessible_epic_todos_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::RemoveInaccessibleEpicTodos, schema: 20201109114603 do
+  include MigrationHelpers::NamespacesHelpers
+
+  let(:users) { table(:users) }
+  let(:todos) { table(:todos) }
+  let(:epics) { table(:epics) }
+  let(:members_table) { table(:members) }
+  let(:group_group_links) { table(:group_group_links) }
+
+  let(:author) { users.create!(email: 'author@example.com', projects_limit: 10) }
+  let(:user) { users.create!(email: 'user@example.com', projects_limit: 10) }
+
+  let(:group_root) { create_namespace('root', Gitlab::VisibilityLevel::PUBLIC) }
+  let(:group_level1) { create_namespace('level1', Gitlab::VisibilityLevel::PUBLIC, parent_id: group_root.id) }
+
+  let(:epic_conf1) { epics.create!(iid: 1, title: 'confidential1', title_html: 'confidential1', confidential: true, group_id: group_root.id, author_id: author.id) }
+  let(:epic_conf2) { epics.create!(iid: 1, title: 'confidential2', title_html: 'confidential2', confidential: true, group_id: group_level1.id, author_id: author.id) }
+  let(:epic_public1) { epics.create!(iid: 2, title: 'public1', title_html: 'epic_public1', group_id: group_root.id, author_id: author.id) }
+  let(:epic_public2) { epics.create!(iid: 2, title: 'public1', title_html: 'epic_public2', group_id: group_level1.id, author_id: author.id) }
+
+  let!(:todo1) { todos.create!(target_type: 'Epic', target_id: epic_conf1.id, user_id: user.id, author_id: user.id, action: 2, state: 0) }
+  let!(:todo2) { todos.create!(target_type: 'Epic', target_id: epic_conf2.id, user_id: user.id, author_id: user.id, action: 2, state: 0) }
+  let!(:todo3) { todos.create!(target_type: 'Epic', target_id: epic_public1.id, user_id: user.id, author_id: user.id, action: 2, state: 0) }
+  let!(:todo4) { todos.create!(target_type: 'Epic', target_id: epic_public2.id, user_id: user.id, author_id: user.id, action: 2, state: 0) }
+
+  describe '#perform' do
+    subject(:perform) { described_class.new.perform(epics.first.id, epics.last.id) }
+
+    def expect_todos(preserved:)
+      expect { subject }.to change { todos.count }.by(preserved.count - 4)
+
+      existing_ids = todos.pluck(:id)
+      expect(existing_ids).to match_array(preserved)
+    end
+
+    context 'when user is not member of related groups' do
+      it 'deletes only todos referencing confidential epics' do
+        expect_todos(preserved: [todo3.id, todo4.id])
+      end
+    end
+
+    context 'when user is only guest member of related groups' do
+      let!(:member) do
+        members_table.create!(user_id: user.id, source_id: group_root.id, source_type: 'Namespace',
+                              type: 'GroupMember', access_level: 10, notification_level: 3)
+      end
+
+      it 'deletes todos referencing confidential epics' do
+        expect_todos(preserved: [todo3.id, todo4.id])
+      end
+    end
+
+    context 'when user is member of subgroup' do
+      let!(:member) do
+        members_table.create!(user_id: user.id, source_id: group_level1.id, source_type: 'Namespace',
+                              type: 'GroupMember', access_level: 20, notification_level: 3)
+      end
+
+      it 'deletes only epic todos in the root group' do
+        expect_todos(preserved: [todo2.id, todo3.id, todo4.id])
+      end
+    end
+
+    context 'when user is member of root group' do
+      let!(:member) do
+        members_table.create!(user_id: user.id, source_id: group_root.id, source_type: 'Namespace',
+                              type: 'GroupMember', access_level: 20, notification_level: 3)
+      end
+
+      it 'does not delete any todos' do
+        expect_todos(preserved: [todo1.id, todo2.id, todo3.id, todo4.id])
+      end
+    end
+
+    context 'when user is only guest on root group' do
+      let!(:root_member) do
+        members_table.create!(user_id: user.id, source_id: group_root.id, source_type: 'Namespace',
+                              type: 'GroupMember', access_level: 10, notification_level: 3)
+      end
+
+      let!(:subgroup_member) do
+        members_table.create!(user_id: user.id, source_id: group_level1.id, source_type: 'Namespace',
+                              type: 'GroupMember', access_level: 20, notification_level: 3)
+      end
+
+      it 'deletes only root confidential epic todo' do
+        expect_todos(preserved: [todo2.id, todo3.id, todo4.id])
+      end
+    end
+
+    context 'when root group is shared with other group' do
+      let!(:other_group) { create_namespace('other_group', Gitlab::VisibilityLevel::PRIVATE) }
+      let!(:member) do
+        members_table.create!(user_id: user.id, source_id: other_group.id, source_type: 'Namespace',
+                              type: 'GroupMember', access_level: 20, notification_level: 3)
+      end
+
+      let!(:group_link) do
+        group_group_links.create!(shared_group_id: group_root.id,
+                                  shared_with_group_id: other_group.id, group_access: 20)
+      end
+
+      it 'does not delete any todos' do
+        expect_todos(preserved: [todo1.id, todo2.id, todo3.id, todo4.id])
+      end
+    end
+  end
+end

ee/spec/migrations/schedule_remove_inaccessible_epic_todos_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20201109114603_schedule_remove_inaccessible_epic_todos')
+
+RSpec.describe ScheduleRemoveInaccessibleEpicTodos do
+  let(:group) { table(:namespaces).create!(name: 'gitlab', path: 'gitlab-org') }
+  let(:user) { table(:users).create!(email: 'user@example.com', projects_limit: 10) }
+  let!(:epic1) { table(:epics).create!(iid: 1, title: 'foo', title_html: 'foo', group_id: group.id, author_id: user.id, confidential: true) }
+  let!(:epic2) { table(:epics).create!(iid: 2, title: 'foo', title_html: 'foo', group_id: group.id, author_id: user.id) }
+  let!(:epic3) { table(:epics).create!(iid: 3, title: 'foo', title_html: 'foo', group_id: group.id, author_id: user.id, confidential: true) }
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 1)
+  end
+
+  it 'schedules jobs for confidental epic todos' do
+    Sidekiq::Testing.fake! do
+      freeze_time do
+        migrate!
+
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(
+          2.minutes, epic1.id, epic1.id)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(
+          4.minutes, epic3.id, epic3.id)
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+      end
+    end
+  end
+end

ee/spec/services/ee/todos/destroy/entity_leave_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Todos::Destroy::EntityLeaveService do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:subgroup) { create(:group, parent: group) }
+
+  let_it_be(:epic1) { create(:epic, confidential: true, group: subgroup) }
+  let_it_be(:epic2) { create(:epic, group: subgroup) }
+  let!(:todo1) { create(:todo, target: epic1, user: user, group: subgroup) }
+  let!(:todo2) { create(:todo, target: epic2, user: user, group: subgroup) }
+
+  describe '#execute' do
+    subject { described_class.new(user.id, subgroup.id, 'Group').execute }
+
+    shared_examples 'removes only confidential epics todos' do
+      it 'removes todos targeting confidential epics in the group' do
+        expect { subject }.to change { Todo.count }.by(-1)
+        expect(user.reload.todos.ids).to match_array(todo2.id)
+      end
+    end
+
+    it_behaves_like 'removes only confidential epics todos'
+
+    context 'when user is still member of ancestor group' do
+      before do
+        group.add_reporter(user)
+      end
+
+      it 'does not remove todos targeting confidential epics in the group' do
+        expect { subject }.not_to change { Todo.count }
+      end
+    end
+
+    context 'when user role is downgraded to guest' do
+      before do
+        subgroup.add_guest(user)
+      end
+
+      it_behaves_like 'removes only confidential epics todos'
+    end
+  end
+end

ee/spec/services/epics/update_service_spec.rb

@@ -216,6 +216,12 @@ RSpec.describe Epics::UpdateService do
           end
         end
       end
+
+      it 'schedules deletion of todos when epic becomes confidential' do
+        expect(TodosDestroyer::ConfidentialEpicWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, epic.id)
+
+        update_epic(confidential: true)
+      end
     end
 
     context 'when Epic has tasks' do

ee/spec/services/todos/destroy/confidential_epic_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Todos::Destroy::ConfidentialEpicService do
+  let_it_be(:group) { create(:group, :public) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:author) { create(:user) }
+  let_it_be(:guest) { create(:user) }
+  let_it_be(:group_member) { create(:user) }
+  let_it_be(:shared_user) { create(:user) }
+  let_it_be(:group_link) { create(:group_group_link, shared_group: group) }
+  let_it_be(:epic_1, reload: true) { create(:epic, :confidential, group: group, author: author) }
+
+  let!(:todos) do
+    [
+      # todos not to be deleted
+      create(:todo, user: group_member, target: epic_1, group: group),
+      create(:todo, user: user, group: group),
+      create(:todo, user: shared_user, target: epic_1, group: group),
+      # Todos to be deleted
+      create(:todo, user: guest, target: epic_1, group: group),
+      create(:todo, user: user, target: epic_1, group: group)
+    ]
+  end
+
+  describe '#execute' do
+    before do
+      group.add_reporter(group_member)
+      group.add_guest(guest)
+      group_link.shared_with_group.add_reporter(shared_user)
+    end
+
+    subject { described_class.new(epic_id: epic_1.id).execute }
+
+    it 'removes epic todos for users who can not access the confidential epic' do
+      expect { subject }.to change { Todo.count }.by(-2)
+    end
+
+    context 'when provided epic is not confidential' do
+      before do
+        epic_1.update!(confidential: false)
+      end
+
+      it 'does not remove any todos' do
+        expect { subject }.not_to change { Todo.count }
+      end
+    end
+  end
+end

ee/spec/workers/todos_destroyer/confidential_epic_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe TodosDestroyer::ConfidentialEpicWorker do
+  let(:service) { double }
+
+  it 'calls the Todos::Destroy::ConfidentialEpicService with epic_id parameter' do
+    expect(::Todos::Destroy::ConfidentialEpicService).to receive(:new).with(epic_id: 100).and_return(service)
+    expect(service).to receive(:execute)
+
+    described_class.new.perform(100)
+  end
+end

lib/gitlab/background_migration/remove_inaccessible_epic_todos.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # rubocop:disable Style/Documentation
+    class RemoveInaccessibleEpicTodos
+      def perform(start_id, stop_id)
+      end
+    end
+  end
+end
+
+Gitlab::BackgroundMigration::RemoveInaccessibleEpicTodos.prepend_if_ee('EE::Gitlab::BackgroundMigration::RemoveInaccessibleEpicTodos')