Add latest changes from gitlab-org/security/gitlab@15-2-stable-ee

16 files changed, 253 insertions, 58 deletions

diff --git a/app/models/error_tracking/project_error_tracking_setting.rb b/app/models/error_tracking/project_error_tracking_setting.rb
index 30382a1c205..4953f24755c 100644
--- a/app/models/error_tracking/project_error_tracking_setting.rb
+++ b/app/models/error_tracking/project_error_tracking_setting.rb
@@ -44,6 +44,8 @@ module ErrorTracking
       key: Settings.attr_encrypted_db_key_base_32,
       algorithm: 'aes-256-gcm'
 
+    before_validation :reset_token
+
     after_save :clear_reactive_cache!
 
     # When a user enables the integrated error tracking
@@ -182,6 +184,12 @@ module ErrorTracking
 
     private
 
+    def reset_token
+      if api_url_changed? && !encrypted_token_changed?
+        self.token = nil
+      end
+    end
+
     def ensure_issue_belongs_to_project!(project_id_from_api)
       raise 'The Sentry issue appers to be outside of the configured Sentry project' if Integer(project_id_from_api) != ensure_sentry_project_id!
     end
diff --git a/app/models/grafana_integration.rb b/app/models/grafana_integration.rb
index 00213732fee..0358e37c58b 100644
--- a/app/models/grafana_integration.rb
+++ b/app/models/grafana_integration.rb
@@ -18,6 +18,8 @@ class GrafanaIntegration < ApplicationRecord
 
   validates :enabled, inclusion: { in: [true, false] }
 
+  before_validation :reset_token
+
   scope :enabled, -> { where(enabled: true) }
 
   def client
@@ -36,6 +38,12 @@ class GrafanaIntegration < ApplicationRecord
 
   private
 
+  def reset_token
+    if grafana_url_changed? && !encrypted_token_changed?
+      self.token = nil
+    end
+  end
+
   def token
     decrypt(:token, encrypted_token)
   end
diff --git a/app/models/todo.rb b/app/models/todo.rb
index cff7a93f72f..c698783d750 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -74,6 +74,7 @@ class Todo < ApplicationRecord
   scope :for_commit, -> (id) { where(commit_id: id) }
   scope :with_entity_associations, -> { preload(:target, :author, :note, group: :route, project: [:route, { namespace: [:route, :owner] }]) }
   scope :joins_issue_and_assignees, -> { left_joins(issue: :assignees) }
+  scope :for_internal_notes, -> { joins(:note).where(note: { confidential: true }) }
 
   enum resolved_by_action: { system_done: 0, api_all_done: 1, api_done: 2, mark_all_done: 3, mark_done: 4 }, _prefix: :resolved_by
 
diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb
index c88c139a22e..bcf3110ca21 100644
--- a/app/services/groups/destroy_service.rb
+++ b/app/services/groups/destroy_service.rb
@@ -35,6 +35,8 @@ module Groups
 
       user_ids_for_project_authorizations_refresh = obtain_user_ids_for_project_authorizations_refresh
 
+      destroy_group_bots
+
       group.destroy
 
       if user_ids_for_project_authorizations_refresh.present?
@@ -76,6 +78,19 @@ module Groups
 
       group.users_ids_of_direct_members
     end
+
+    # rubocop:disable CodeReuse/ActiveRecord
+    def destroy_group_bots
+      bot_ids = group.members_and_requesters.joins(:user).merge(User.project_bot).pluck(:user_id)
+      current_user_id = current_user.id
+
+      group.run_after_commit do
+        bot_ids.each do |user_id|
+          DeleteUserWorker.perform_async(current_user_id, user_id, skip_authorization: true)
+        end
+      end
+    end
+    # rubocop:enable CodeReuse/ActiveRecord
   end
 end
 
diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb
index 1fe397d24e7..5b04d2fd3af 100644
--- a/app/services/todos/destroy/entity_leave_service.rb
+++ b/app/services/todos/destroy/entity_leave_service.rb
@@ -41,11 +41,20 @@ module Todos
       end
 
       def remove_confidential_resource_todos
+        # Deletes todos for confidential issues
         Todo
           .for_target(confidential_issues.select(:id))
           .for_type(Issue.name)
           .for_user(user)
           .delete_all
+
+        # Deletes todos for internal notes on unauthorized projects
+        Todo
+          .for_type(Issue.name)
+          .for_internal_notes
+          .for_project(non_authorized_reporter_projects) # Only Reporter+ can read internal notes
+          .for_user(user)
+          .delete_all
       end
 
       def remove_project_todos
diff --git a/db/post_migrate/20220712175029_add_index_with_target_type_to_todos.rb b/db/post_migrate/20220712175029_add_index_with_target_type_to_todos.rb
new file mode 100644
index 00000000000..077e8ed4bbe
--- /dev/null
+++ b/db/post_migrate/20220712175029_add_index_with_target_type_to_todos.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+class AddIndexWithTargetTypeToTodos < Gitlab::Database::Migration[2.0]
+  disable_ddl_transaction!
+
+  INDEX_FOR_PROJECTS_NAME = 'index_requirements_project_id_user_id_id_and_target_type'
+  INDEX_FOR_TARGET_TYPE_NAME = 'index_requirements_user_id_and_target_type'
+
+  def up
+    add_concurrent_index :todos, [:project_id, :user_id, :id, :target_type], name: INDEX_FOR_PROJECTS_NAME
+    add_concurrent_index :todos, [:user_id, :target_type], name: INDEX_FOR_TARGET_TYPE_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :todos, INDEX_FOR_PROJECTS_NAME
+    remove_concurrent_index_by_name :todos, INDEX_FOR_TARGET_TYPE_NAME
+  end
+end
diff --git a/db/post_migrate/20220712181304_remove_deprecated_indexes_from_todos.rb b/db/post_migrate/20220712181304_remove_deprecated_indexes_from_todos.rb
new file mode 100644
index 00000000000..4f99caa10a4
--- /dev/null
+++ b/db/post_migrate/20220712181304_remove_deprecated_indexes_from_todos.rb
@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+class RemoveDeprecatedIndexesFromTodos < Gitlab::Database::Migration[2.0]
+  disable_ddl_transaction!
+
+  PROJECTS_INDEX = 'index_todos_on_project_id_and_user_id_and_id'
+  USERS_INDEX = 'index_todos_on_user_id'
+
+  # These indexes are deprecated in favor of two new ones
+  # added in a previous migration:
+  #
+  # * index_requirements_project_id_user_id_target_type_and_id
+  # * index_requirements_user_id_and_target_type
+  def up
+    remove_concurrent_index_by_name :todos, PROJECTS_INDEX
+    remove_concurrent_index_by_name :todos, USERS_INDEX
+  end
+
+  def down
+    add_concurrent_index :todos, [:project_id, :user_id, :id], name: PROJECTS_INDEX
+    add_concurrent_index :todos, :user_id, name: USERS_INDEX
+  end
+end
diff --git a/db/schema_migrations/20220712175029 b/db/schema_migrations/20220712175029
new file mode 100644
index 00000000000..bb7fdca340f
--- /dev/null
+++ b/db/schema_migrations/20220712175029
@@ -0,0 +1 @@
+f6638435457f57f5c566e107de4f4557a1d87b5dd27acc9e5345999197d18e6e
\ No newline at end of file
diff --git a/db/schema_migrations/20220712181304 b/db/schema_migrations/20220712181304
new file mode 100644
index 00000000000..ff111fe7c28
--- /dev/null
+++ b/db/schema_migrations/20220712181304
@@ -0,0 +1 @@
+ff9ad44a43be82867da8e0f51e68a2284065cab6b2eb7cf6496108dce1cdd657
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index cb0d4696931..28082885574 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -29549,6 +29549,10 @@ CREATE INDEX index_requirements_on_title_trigram ON requirements USING gin (titl
 
 CREATE INDEX index_requirements_on_updated_at ON requirements USING btree (updated_at);
 
+CREATE INDEX index_requirements_project_id_user_id_id_and_target_type ON todos USING btree (project_id, user_id, id, target_type);
+
+CREATE INDEX index_requirements_user_id_and_target_type ON todos USING btree (user_id, target_type);
+
 CREATE INDEX index_resource_iteration_events_on_issue_id ON resource_iteration_events USING btree (issue_id);
 
 CREATE INDEX index_resource_iteration_events_on_iteration_id ON resource_iteration_events USING btree (iteration_id);
@@ -29867,12 +29871,8 @@ CREATE INDEX index_todos_on_note_id ON todos USING btree (note_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);
 
-CREATE INDEX index_todos_on_user_id ON todos USING btree (user_id);
-
 CREATE INDEX index_todos_on_user_id_and_id_done ON todos USING btree (user_id, id) WHERE ((state)::text = 'done'::text);
 
 CREATE INDEX index_todos_on_user_id_and_id_pending ON todos USING btree (user_id, id) WHERE ((state)::text = 'pending'::text);
diff --git a/spec/models/error_tracking/project_error_tracking_setting_spec.rb b/spec/models/error_tracking/project_error_tracking_setting_spec.rb
index 15b6b45eaba..ebfd9f04f6a 100644
--- a/spec/models/error_tracking/project_error_tracking_setting_spec.rb
+++ b/spec/models/error_tracking/project_error_tracking_setting_spec.rb
@@ -123,6 +123,38 @@ RSpec.describe ErrorTracking::ProjectErrorTrackingSetting do
     end
   end
 
+  describe 'before_validation :reset_token' do
+    context 'when a token was previously set' do
+      subject { create(:project_error_tracking_setting, project: project) }
+
+      it 'resets token if url changed' do
+        subject.api_url = 'http://sentry.com/api/0/projects/org-slug/proj-slug/'
+
+        expect(subject).not_to be_valid
+        expect(subject.token).to be_nil
+      end
+
+      it "does not reset token if new url is set together with the same token" do
+        subject.api_url = 'http://sentrytest.com/api/0/projects/org-slug/proj-slug/'
+        current_token = subject.token
+        subject.token = current_token
+
+        expect(subject).to be_valid
+        expect(subject.token).to eq(current_token)
+        expect(subject.api_url).to eq('http://sentrytest.com/api/0/projects/org-slug/proj-slug/')
+      end
+
+      it 'does not reset token if new url is set together with a new token' do
+        subject.api_url = 'http://sentrytest.com/api/0/projects/org-slug/proj-slug/'
+        subject.token = 'token'
+
+        expect(subject).to be_valid
+        expect(subject.token).to eq('token')
+        expect(subject.api_url).to eq('http://sentrytest.com/api/0/projects/org-slug/proj-slug/')
+      end
+    end
+  end
+
   describe '.extract_sentry_external_url' do
     subject { described_class.extract_sentry_external_url(sentry_url) }
 
diff --git a/spec/models/grafana_integration_spec.rb b/spec/models/grafana_integration_spec.rb
index bb822187e0c..73ec2856c05 100644
--- a/spec/models/grafana_integration_spec.rb
+++ b/spec/models/grafana_integration_spec.rb
@@ -86,4 +86,38 @@ RSpec.describe GrafanaIntegration do
       end
     end
   end
+
+  describe 'Callbacks' do
+    describe 'before_validation :reset_token' do
+      context 'when a token was previously set' do
+        subject(:grafana_integration) { create(:grafana_integration) }
+
+        it 'resets token if url changed' do
+          grafana_integration.grafana_url = 'http://gitlab1.com'
+
+          expect(grafana_integration).not_to be_valid
+          expect(grafana_integration.send(:token)).to be_nil
+        end
+
+        it "does not reset token if new url is set together with the same token" do
+          grafana_integration.grafana_url = 'http://gitlab_edited.com'
+          current_token = grafana_integration.send(:token)
+          grafana_integration.token = current_token
+
+          expect(grafana_integration).to be_valid
+          expect(grafana_integration.send(:token)).to eq(current_token)
+          expect(grafana_integration.grafana_url).to eq('http://gitlab_edited.com')
+        end
+
+        it 'does not reset token if new url is set together with a new token' do
+          grafana_integration.grafana_url = 'http://gitlab_edited.com'
+          grafana_integration.token = 'token'
+
+          expect(grafana_integration).to be_valid
+          expect(grafana_integration.send(:token)).to eq('token')
+          expect(grafana_integration.grafana_url).to eq('http://gitlab_edited.com')
+        end
+      end
+    end
+  end
 end
diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb
index 7df22078c6d..18b0cb36cc6 100644
--- a/spec/models/todo_spec.rb
+++ b/spec/models/todo_spec.rb
@@ -495,4 +495,14 @@ RSpec.describe Todo do
 
     it { is_expected.to contain_exactly(user1.id, user2.id) }
   end
+
+  describe '.for_internal_notes' do
+    it 'returns todos created from internal notes' do
+      internal_note = create(:note, confidential: true )
+      todo = create(:todo, note: internal_note)
+      create(:todo)
+
+      expect(described_class.for_internal_notes).to contain_exactly(todo)
+    end
+  end
 end
diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb
index 57a151efda6..f43f64fdf89 100644
--- a/spec/services/groups/destroy_service_spec.rb
+++ b/spec/services/groups/destroy_service_spec.rb
@@ -35,6 +35,20 @@ RSpec.describe Groups::DestroyService do
       it { expect(NotificationSetting.unscoped.all).not_to include(notification_setting) }
     end
 
+    context 'bot tokens', :sidekiq_might_not_need_inline do
+      it 'removes group bot', :aggregate_failures do
+        bot = create(:user, :project_bot)
+        group.add_developer(bot)
+        token = create(:personal_access_token, user: bot)
+
+        destroy_group(group, user, async)
+
+        expect(PersonalAccessToken.find_by(id: token.id)).to be_nil
+        expect(User.find_by(id: bot.id)).to be_nil
+        expect(User.find_by(id: user.id)).not_to be_nil
+      end
+    end
+
     context 'mattermost team', :sidekiq_might_not_need_inline do
       let!(:chat_team) { create(:chat_team, namespace: group) }
 
diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb
index bee91c358ce..95f2176dbc0 100644
--- a/spec/services/projects/operations/update_service_spec.rb
+++ b/spec/services/projects/operations/update_service_spec.rb
@@ -306,6 +306,11 @@ RSpec.describe Projects::Operations::UpdateService do
         let(:params) do
           {
             error_tracking_setting_attributes: {
+              api_host: 'https://sentrytest.gitlab.com/',
+              project: {
+                slug: 'sentry-project',
+                organization_slug: 'sentry-org'
+              },
               enabled: false,
               token: '*' * 8
             }
@@ -313,7 +318,7 @@ RSpec.describe Projects::Operations::UpdateService do
         end
 
         before do
-          create(:project_error_tracking_setting, project: project, token: 'token')
+          create(:project_error_tracking_setting, project: project, token: 'token', api_url: 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project/')
         end
 
         it 'does not update token' do
diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb
index 03fa2482bbf..225e7933d79 100644
--- a/spec/services/todos/destroy/entity_leave_service_spec.rb
+++ b/spec/services/todos/destroy/entity_leave_service_spec.rb
@@ -3,21 +3,24 @@
 require 'spec_helper'
 
 RSpec.describe Todos::Destroy::EntityLeaveService do
-  let_it_be(:user, reload: true)    { create(:user) }
-  let_it_be(:user2, reload: true)   { create(:user) }
-
-  let(:group)   { create(:group, :private) }
-  let(:project) { create(:project, :private, group: group) }
-  let(:issue)                 { create(:issue, project: project) }
-  let(:issue_c)               { create(:issue, project: project, confidential: true) }
-  let!(:todo_group_user)       { create(:todo, user: user, group: group) }
-  let!(:todo_group_user2)      { create(:todo, user: user2, group: group) }
-
-  let(:mr)                  { create(:merge_request, source_project: project) }
-  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_issue_c_user)  { create(:todo, user: user, target: issue_c, project: project) }
+  let_it_be(:user, reload: true) { create(:user) }
+  let_it_be(:user2, reload: true) { create(:user) }
+  let_it_be_with_refind(:group) { create(:group, :private) }
+  let_it_be(:project) { create(:project, :private, group: group) }
+
+  let(:issue) { create(:issue, project: project) }
+  let(:issue_c) { create(:issue, project: project, confidential: true) }
+  let!(:todo_group_user) { create(:todo, user: user, group: group) }
+  let!(:todo_group_user2) { create(:todo, user: user2, group: group) }
+  let(:mr) { create(:merge_request, source_project: project) }
+  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_issue_c_user) { create(:todo, user: user, target: issue_c, project: project) }
   let!(:todo_issue_c_user2) { create(:todo, user: user2, target: issue_c, project: project) }
+  let(:internal_note) { create(:note, noteable: issue, project: project, confidential: true ) }
+  let!(:todo_for_internal_note) do
+    create(:todo, user: user, target: issue, project: project, note: internal_note)
+  end
 
   shared_examples 'using different access permissions' do
     before do
@@ -34,20 +37,28 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
     it { does_not_remove_any_todos }
   end
 
-  shared_examples 'removes only confidential issues todos' do
-    it { removes_only_confidential_issues_todos }
+  shared_examples 'removes confidential issues and internal notes todos' do
+    it { removes_confidential_issues_and_internal_notes_todos }
+  end
+
+  shared_examples 'removes only internal notes todos' do
+    it { removes_only_internal_notes_todos }
   end
 
   def does_not_remove_any_todos
     expect { subject }.not_to change { Todo.count }
   end
 
-  def removes_only_confidential_issues_todos
-    expect { subject }.to change { Todo.count }.from(6).to(5)
+  def removes_only_internal_notes_todos
+    expect { subject }.to change { Todo.count }.from(7).to(6)
+  end
+
+  def removes_confidential_issues_and_internal_notes_todos
+    expect { subject }.to change { Todo.count }.from(7).to(5)
   end
 
-  def removes_confidential_issues_and_merge_request_todos
-    expect { subject }.to change { Todo.count }.from(6).to(4)
+  def removes_confidential_issues_and_internal_notes_and_merge_request_todos
+    expect { subject }.to change { Todo.count }.from(7).to(4)
     expect(user.todos).to match_array([todo_issue_user, todo_group_user])
   end
 
@@ -70,7 +81,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
       context 'when project is private' do
         context 'when user is not a member of the project' do
           it 'removes project todos for the provided user' do
-            expect { subject }.to change { Todo.count }.from(6).to(3)
+            expect { subject }.to change { Todo.count }.from(7).to(3)
 
             expect(user.todos).to match_array([todo_group_user])
             expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2])
@@ -81,11 +92,11 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
           where(:group_access, :project_access, :method_name) do
             [
               [nil,       :reporter, :does_not_remove_any_todos],
-              [nil,       :guest,    :removes_confidential_issues_and_merge_request_todos],
+              [nil,       :guest,    :removes_confidential_issues_and_internal_notes_and_merge_request_todos],
               [:reporter, nil,       :does_not_remove_any_todos],
-              [:guest,    nil,       :removes_confidential_issues_and_merge_request_todos],
+              [:guest,    nil,       :removes_confidential_issues_and_internal_notes_and_merge_request_todos],
               [:guest,    :reporter, :does_not_remove_any_todos],
-              [:guest,    :guest,    :removes_confidential_issues_and_merge_request_todos]
+              [:guest,    :guest,    :removes_confidential_issues_and_internal_notes_and_merge_request_todos]
             ]
           end
 
@@ -97,11 +108,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
 
       # a private project in an internal/public group is valid
       context 'when project is private in an internal/public group' do
-        let(:group) { create(:group, :internal) }
+        let_it_be(:group) { create(:group, :internal) }
+        let_it_be(:project) { create(:project, :private, group: group) }
 
         context 'when user is not a member of the project' do
           it 'removes project todos for the provided user' do
-            expect { subject }.to change { Todo.count }.from(6).to(3)
+            expect { subject }.to change { Todo.count }.from(7).to(3)
 
             expect(user.todos).to match_array([todo_group_user])
             expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2])
@@ -112,11 +124,11 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
           where(:group_access, :project_access, :method_name) do
             [
               [nil,       :reporter, :does_not_remove_any_todos],
-              [nil,       :guest,    :removes_confidential_issues_and_merge_request_todos],
+              [nil,       :guest,    :removes_confidential_issues_and_internal_notes_and_merge_request_todos],
               [:reporter, nil,       :does_not_remove_any_todos],
-              [:guest,    nil,       :removes_confidential_issues_and_merge_request_todos],
+              [:guest,    nil,       :removes_confidential_issues_and_internal_notes_and_merge_request_todos],
               [:guest,    :reporter, :does_not_remove_any_todos],
-              [:guest,    :guest,    :removes_confidential_issues_and_merge_request_todos]
+              [:guest,    :guest,    :removes_confidential_issues_and_internal_notes_and_merge_request_todos]
             ]
           end
 
@@ -142,7 +154,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
 
         context 'confidential issues' do
           context 'when a user is not an author of confidential issue' do
-            it_behaves_like 'removes only confidential issues todos'
+            it_behaves_like 'removes confidential issues and internal notes todos'
           end
 
           context 'when a user is an author of confidential issue' do
@@ -150,7 +162,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
               issue_c.update!(author: user)
             end
 
-            it_behaves_like 'does not remove any todos'
+            it_behaves_like 'removes only internal notes todos'
           end
 
           context 'when a user is an assignee of confidential issue' do
@@ -158,18 +170,18 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
               issue_c.assignees << user
             end
 
-            it_behaves_like 'does not remove any todos'
+            it_behaves_like 'removes only internal notes todos'
           end
 
           context 'access permissions' do
             where(:group_access, :project_access, :method_name) do
               [
                 [nil,       :reporter, :does_not_remove_any_todos],
-                [nil,       :guest,    :removes_only_confidential_issues_todos],
+                [nil,       :guest,    :removes_confidential_issues_and_internal_notes_todos],
                 [:reporter, nil,       :does_not_remove_any_todos],
-                [:guest,    nil,       :removes_only_confidential_issues_todos],
+                [:guest,    nil,       :removes_confidential_issues_and_internal_notes_todos],
                 [:guest,    :reporter, :does_not_remove_any_todos],
-                [:guest,    :guest,    :removes_only_confidential_issues_todos]
+                [:guest,    :guest,    :removes_confidential_issues_and_internal_notes_todos]
               ]
             end
 
@@ -186,7 +198,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
             end
 
             it 'removes only users issue todos' do
-              expect { subject }.to change { Todo.count }.from(6).to(5)
+              expect { subject }.to change { Todo.count }.from(7).to(5)
             end
           end
         end
@@ -199,7 +211,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
       context 'when group is private' do
         context 'when a user leaves a group' do
           it 'removes group and subproject todos for the user' do
-            expect { subject }.to change { Todo.count }.from(6).to(2)
+            expect { subject }.to change { Todo.count }.from(7).to(2)
 
             expect(user.todos).to be_empty
             expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2])
@@ -210,11 +222,11 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
           where(:group_access, :project_access, :method_name) do
             [
               [nil,       :reporter, :does_not_remove_any_todos],
-              [nil,       :guest,    :removes_confidential_issues_and_merge_request_todos],
+              [nil,       :guest,    :removes_confidential_issues_and_internal_notes_and_merge_request_todos],
               [:reporter, nil,       :does_not_remove_any_todos],
-              [:guest,    nil,       :removes_confidential_issues_and_merge_request_todos],
+              [:guest,    nil,       :removes_confidential_issues_and_internal_notes_and_merge_request_todos],
               [:guest,    :reporter, :does_not_remove_any_todos],
-              [:guest,    :guest,    :removes_confidential_issues_and_merge_request_todos]
+              [:guest,    :guest,    :removes_confidential_issues_and_internal_notes_and_merge_request_todos]
             ]
           end
 
@@ -224,12 +236,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
         end
 
         context 'with nested groups' do
-          let(:parent_group) { create(:group, :public) }
-          let(:parent_subgroup) { create(:group)}
-          let(:subgroup) { create(:group, :private, parent: group) }
-          let(:subgroup2) { create(:group, :private, parent: group) }
-          let(:subproject) { create(:project, group: subgroup) }
-          let(:subproject2) { create(:project, group: subgroup2) }
+          let_it_be_with_refind(:parent_group) { create(:group, :public) }
+          let_it_be_with_refind(:parent_subgroup) { create(:group) }
+          let_it_be(:subgroup) { create(:group, :private, parent: group) }
+          let_it_be(:subgroup2) { create(:group, :private, parent: group) }
+          let_it_be(:subproject) { create(:project, group: subgroup) }
+          let_it_be(:subproject2) { create(:project, group: subgroup2) }
 
           let!(:todo_subproject_user) { create(:todo, user: user, project: subproject) }
           let!(:todo_subproject2_user) { create(:todo, user: user, project: subproject2) }
@@ -238,6 +250,10 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
           let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) }
           let!(:todo_subpgroup_user2)  { create(:todo, user: user2, group: subgroup) }
           let!(:todo_parent_group_user) { create(:todo, user: user, group: parent_group) }
+          let(:subproject_internal_note) { create(:note, noteable: issue, project: project, confidential: true ) }
+          let!(:todo_for_internal_subproject_note) do
+            create(:todo, user: user, target: issue, project: project, note: subproject_internal_note)
+          end
 
           before do
             group.update!(parent: parent_group)
@@ -245,7 +261,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
 
           context 'when the user is not a member of any groups/projects' do
             it 'removes todos for the user including subprojects todos' do
-              expect { subject }.to change { Todo.count }.from(13).to(5)
+              expect { subject }.to change { Todo.count }.from(15).to(5)
 
               expect(user.todos).to eq([todo_parent_group_user])
               expect(user2.todos)
@@ -269,7 +285,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
             end
 
             it 'does not remove group and subproject todos' do
-              expect { subject }.to change { Todo.count }.from(13).to(8)
+              expect { subject }.to change { Todo.count }.from(15).to(8)
 
               expect(user.todos)
                 .to match_array(
@@ -288,7 +304,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
             end
 
             it 'does not remove subproject and group todos' do
-              expect { subject }.to change { Todo.count }.from(13).to(8)
+              expect { subject }.to change { Todo.count }.from(15).to(8)
 
               expect(user.todos)
                 .to match_array(
@@ -319,13 +335,13 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
         context 'access permissions' do
           where(:group_access, :project_access, :method_name) do
             [
-              [nil,       nil,       :removes_only_confidential_issues_todos],
+              [nil,       nil,       :removes_confidential_issues_and_internal_notes_todos],
               [nil,       :reporter, :does_not_remove_any_todos],
-              [nil,       :guest,    :removes_only_confidential_issues_todos],
+              [nil,       :guest,    :removes_confidential_issues_and_internal_notes_todos],
               [:reporter, nil,       :does_not_remove_any_todos],
-              [:guest,    nil,       :removes_only_confidential_issues_todos],
+              [:guest,    nil,       :removes_confidential_issues_and_internal_notes_todos],
               [:guest,    :reporter, :does_not_remove_any_todos],
-              [:guest,    :guest,    :removes_only_confidential_issues_todos]
+              [:guest,    :guest,    :removes_confidential_issues_and_internal_notes_todos]
             ]
           end