Merge branch 'security-282445-read-todo-target' into 'master'

Filter todos whose target users no longer have access to [RUN AS-IF-FOSS]

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

app/controllers/dashboard/todos_controller.rb

@@ -16,6 +16,8 @@ class Dashboard::TodosController < Dashboard::ApplicationController
     @todos = @todos.with_entity_associations
 
     return if redirect_out_of_range(@todos, todos_page_count(@todos))
+
+    @allowed_todos = ::Todos::AllowedTargetFilterService.new(@todos, current_user).execute
   end
 
   def destroy

app/graphql/resolvers/todo_resolver.rb

@@ -34,7 +34,7 @@ module Resolvers
       return Todo.none unless current_user.present? && target.present?
       return Todo.none if target.is_a?(User) && target != current_user
 
-      TodosFinder.new(current_user, todo_finder_params(args)).execute
+      TodosFinder.new(current_user, todo_finder_params(args)).execute.with_entity_associations
     end
 
     private

app/graphql/types/alert_management/alert_type.rb

@@ -115,11 +115,9 @@ module Types
             null: true,
             description: 'Runbook for the alert as defined in alert details.'
 
-      field :todos,
-            Types::TodoType.connection_type,
-            null: true,
-            description: 'To-do items of the current user for the alert.',
-            resolver: Resolvers::TodoResolver
+      field :todos, description: 'To-do items of the current user for the alert.', resolver: Resolvers::TodoResolver do
+        extension(::Gitlab::Graphql::TodosProjectPermissionPreloader::FieldExtension)
+      end
 
       field :details_url,
             GraphQL::Types::String,

app/graphql/types/user_interface.rb

@@ -55,9 +55,6 @@ module Types
           type: GraphQL::Types::String,
           null: false,
           description: 'Web path of the user.'
-    field :todos,
-          resolver: Resolvers::TodoResolver,
-          description: 'To-do items of the user.'
     field :group_memberships,
           type: Types::GroupMemberType.connection_type,
           null: true,
@@ -81,6 +78,10 @@ module Types
           description: 'Projects starred by the user.',
           resolver: Resolvers::UserStarredProjectsResolver
 
+    field :todos, resolver: Resolvers::TodoResolver, description: 'To-do items of the user.' do
+      extension(::Gitlab::Graphql::TodosProjectPermissionPreloader::FieldExtension)
+    end
+
     # Merge request field: MRs can be authored, assigned, or assigned-for-review:
     field :authored_merge_requests,
           resolver: Resolvers::AuthoredMergeRequestsResolver,

app/models/alert_management/alert.rb

@@ -266,6 +266,10 @@ module AlertManagement
       end
     end
 
+    def to_ability_name
+      'alert_management_alert'
+    end
+
     private
 
     def hook_data

app/models/application_record.rb

@@ -103,4 +103,12 @@ class ApplicationRecord < ActiveRecord::Base
   def self.cached_column_list
     self.column_names.map { |column_name| self.arel_table[column_name] }
   end
+
+  def readable_by?(user)
+    Ability.allowed?(user, "read_#{to_ability_name}".to_sym, self)
+  end
+
+  def to_ability_name
+    model_name.element
+  end
 end

app/models/commit.rb

@@ -550,6 +550,10 @@ class Commit
     expire_note_etag_cache_for_related_mrs
   end
 
+  def readable_by?(user)
+    Ability.allowed?(user, :read_commit, self)
+  end
+
   private
 
   def expire_note_etag_cache_for_related_mrs

app/models/design_management/design.rb

@@ -182,10 +182,6 @@ module DesignManagement
       File.join(DesignManagement.designs_directory, "issue-#{issue.iid}", design.filename)
     end
 
-    def to_ability_name
-      'design'
-    end
-
     def description
       ''
     end

app/models/group.rb

@@ -714,10 +714,6 @@ class Group < Namespace
     Gitlab::ServiceDesk.supported? && all_projects.service_desk_enabled.exists?
   end
 
-  def to_ability_name
-    model_name.singular
-  end
-
   def activity_path
     Gitlab::Routing.url_helpers.activity_group_path(self)
   end

app/models/issue.rb

@@ -543,6 +543,25 @@ class Issue < ApplicationRecord
     self.update_column(:upvotes_count, self.upvotes)
   end
 
+  # Returns `true` if the given User can read the current Issue.
+  #
+  # This method duplicates the same check of issue_policy.rb
+  # for performance reasons, check commit: 002ad215818450d2cbbc5fa065850a953dc7ada8
+  # Make sure to sync this method with issue_policy.rb
+  def readable_by?(user)
+    if user.can_read_all_resources?
+      true
+    elsif project.owner == user
+      true
+    elsif confidential? && !assignee_or_author?(user)
+      project.team.member?(user, Gitlab::Access::REPORTER)
+    else
+      project.public? ||
+        project.internal? && !user.external? ||
+        project.team.member?(user)
+    end
+  end
+
   private
 
   def spammable_attribute_changed?
@@ -568,25 +587,6 @@ class Issue < ApplicationRecord
     Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_created_action(author: author)
   end
 
-  # Returns `true` if the given User can read the current Issue.
-  #
-  # This method duplicates the same check of issue_policy.rb
-  # for performance reasons, check commit: 002ad215818450d2cbbc5fa065850a953dc7ada8
-  # Make sure to sync this method with issue_policy.rb
-  def readable_by?(user)
-    if user.can_read_all_resources?
-      true
-    elsif project.owner == user
-      true
-    elsif confidential? && !assignee_or_author?(user)
-      project.team.member?(user, Gitlab::Access::REPORTER)
-    else
-      project.public? ||
-        project.internal? && !user.external? ||
-        project.team.member?(user)
-    end
-  end
-
   # Returns `true` if this Issue is visible to everybody.
   def publicly_visible?
     project.public? && !confidential? && !::Gitlab::ExternalAuthorization.enabled?

app/models/note.rb

@@ -384,12 +384,6 @@ class Note < ApplicationRecord
     super
   end
 
-  # This method is to be used for checking read permissions on a note instead of `system_note_with_references_visible_for?`
-  def readable_by?(user)
-    # note_policy accounts for #system_note_with_references_visible_for?(user) check when granting read access
-    Ability.allowed?(user, :read_note, self)
-  end
-
   def award_emoji?
     can_be_award_emoji? && contains_emoji_only?
   end
@@ -406,10 +400,6 @@ class Note < ApplicationRecord
     note =~ /\A#{Banzai::Filter::EmojiFilter.emoji_pattern}\s?\Z/
   end
 
-  def to_ability_name
-    model_name.singular
-  end
-
   def noteable_ability_name
     if for_snippet?
       'snippet'

app/models/project.rb

@@ -1494,10 +1494,6 @@ class Project < ApplicationRecord
     end
   end
 
-  def to_ability_name
-    model_name.singular
-  end
-
   # rubocop: disable CodeReuse/ServiceClass
   def execute_hooks(data, hooks_scope = :push_hooks)
     run_after_commit_or_now do

app/policies/todo_policy.rb

@@ -5,7 +5,10 @@ class TodoPolicy < BasePolicy
   condition(:own_todo) do
     @user && @subject.user_id == @user.id
   end
+  condition(:can_read_target) do
+    @user && @subject.target&.readable_by?(@user)
+  end
 
-  rule { own_todo }.enable :read_todo
-  rule { own_todo }.enable :update_todo
+  rule { own_todo & can_read_target }.enable :read_todo
+  rule { own_todo & can_read_target }.enable :update_todo
 end

app/services/todos/allowed_target_filter_service.rb

+# frozen_string_literal: true
+
+module Todos
+  class AllowedTargetFilterService
+    include Gitlab::Allowable
+
+    def initialize(todos, current_user)
+      @todos = todos
+      @current_user = current_user
+    end
+
+    def execute
+      Preloaders::UserMaxAccessLevelInProjectsPreloader.new(@todos.map(&:project).compact, @current_user).execute
+
+      @todos.select { |todo| can?(@current_user, :read_todo, todo) }
+    end
+  end
+end

app/views/dashboard/todos/index.html.haml

@@ -25,7 +25,7 @@
             = number_with_delimiter(todos_done_count)
 
     .nav-controls
-      - if @todos.any?(&:pending?)
+      - if @allowed_todos.any?(&:pending?)
         .gl-mr-3
           = link_to destroy_all_dashboard_todos_path(todos_filter_params), class: 'gl-button btn btn-default btn-loading align-items-center js-todos-mark-all', method: :delete, data: { href: destroy_all_dashboard_todos_path(todos_filter_params) } do
             Mark all as done
@@ -82,11 +82,11 @@
                   = sort_title_oldest_created
 
 .row.js-todos-all
-  - if @todos.any?
+  - if @allowed_todos.any?
     .col.js-todos-list-container{ data: { qa_selector: "todos_list_container" } }
-      .js-todos-options{ data: { per_page: @todos.limit_value, current_page: @todos.current_page, total_pages: @todos.total_pages } }
+      .js-todos-options{ data: { per_page: @allowed_todos.count, current_page: @todos.current_page, total_pages: @todos.total_pages } }
         %ul.content-list.todos-list
-          = render @todos
+          = render @allowed_todos
       = paginate @todos, theme: "gitlab"
     .js-nothing-here-container.empty-state.hidden
       .svg-content

ee/spec/requests/api/graphql/current_user/todos_query_spec.rb

@@ -6,7 +6,9 @@ RSpec.describe 'getting project information' do
   include GraphqlHelpers
 
   let_it_be(:current_user) { create(:user) }
-  let_it_be(:epic_todo) { create(:todo, user: current_user, target: create(:epic)) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:epic) { create(:epic, group: group) }
+  let_it_be(:epic_todo) { create(:todo, user: current_user, target: epic) }
 
   let(:fields) do
     <<~QUERY
@@ -22,7 +24,13 @@ RSpec.describe 'getting project information' do
 
   subject { graphql_data.dig('currentUser', 'todos', 'nodes') }
 
+  before_all do
+    group.add_developer(current_user)
+  end
+
   before do
+    stub_licensed_features(epics: true)
+
     post_graphql(query, current_user: current_user)
   end
 

ee/spec/requests/api/todos_spec.rb

@@ -31,6 +31,8 @@ RSpec.describe API::Todos do
       let!(:epic_todo) { create_todo_for_new_epic }
 
       before do
+        stub_licensed_features(epics: true)
+
         get api('/todos', personal_access_token: pat)
       end
 
@@ -45,7 +47,8 @@ RSpec.describe API::Todos do
 
         create_todo_for_new_epic
 
-        expect { get api('/todos', personal_access_token: pat) }.not_to exceed_query_limit(control)
+        # Additional query due to authorization check on new group
+        expect { get api('/todos', personal_access_token: pat) }.not_to exceed_query_limit(control).with_threshold(1)
       end
 
       it 'includes the Epic Todo in the response' do

ee/spec/services/todos/allowed_target_filter_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Todos::AllowedTargetFilterService do
+  let_it_be(:authorized_group) { create(:group, :private) }
+  let_it_be(:authorized_project) { create(:project, group: authorized_group) }
+  let_it_be(:unauthorized_group) { create(:group, :private) }
+  let_it_be(:unauthorized_project) { create(:project, group: unauthorized_group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:authorized_epic) { create(:epic, group: authorized_group) }
+  let_it_be(:authorized_epic_todo) { create(:todo, group: authorized_group, target: authorized_epic, user: user) }
+  let_it_be(:unauthorized_epic) { create(:epic, group: unauthorized_group) }
+  let_it_be(:unauthorized_epic_todo) { create(:todo, group: unauthorized_group, target: unauthorized_epic, user: user) }
+
+  before_all do
+    authorized_group.add_developer(user)
+  end
+
+  describe '#execute' do
+    subject(:execute_service) { described_class.new(all_todos, user).execute }
+
+    let!(:all_todos) { authorized_todos + unauthorized_todos }
+
+    let(:authorized_todos) do
+      [
+        authorized_epic_todo
+      ]
+    end
+
+    let(:unauthorized_todos) do
+      [
+        unauthorized_epic_todo
+      ]
+    end
+
+    before do
+      stub_licensed_features(epics: true)
+    end
+
+    it { is_expected.to match_array(authorized_todos) }
+  end
+end

lib/api/todos.rb

@@ -92,6 +92,7 @@ module API
       end
       get do
         todos = paginate(find_todos.with_entity_associations)
+        todos = ::Todos::AllowedTargetFilterService.new(todos, current_user).execute
         options = { with: Entities::Todo, current_user: current_user }
         batch_load_issuable_metadata(todos, options)
 

lib/gitlab/graphql/todos_project_permission_preloader/field_extension.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module TodosProjectPermissionPreloader
+      class FieldExtension < ::GraphQL::Schema::FieldExtension
+        def after_resolve(value:, memo:, **rest)
+          todos = value.to_a
+
+          Preloaders::UserMaxAccessLevelInProjectsPreloader.new(
+            todos.map(&:project).compact,
+            current_user(rest)
+          ).execute
+
+          value
+        end
+
+        private
+
+        def current_user(options)
+          options.dig(:context, :current_user)
+        end
+      end
+    end
+  end
+end

spec/features/dashboard/todos/target_state_spec.rb

@@ -3,16 +3,20 @@
 require 'spec_helper'
 
 RSpec.describe 'Dashboard > Todo target states' do
-  let(:user)    { create(:user) }
-  let(:author)  { create(:user) }
-  let(:project) { create(:project, :public) }
+  let_it_be(:user)    { create(:user) }
+  let_it_be(:author)  { create(:user) }
+  let_it_be(:project) { create(:project, :public) }
+
+  before_all do
+    project.add_developer(user)
+  end
 
   before do
     sign_in(user)
   end
 
   it 'on a closed issue todo has closed label' do
-    issue_closed = create(:issue, state: 'closed')
+    issue_closed = create(:issue, state: 'closed', project: project)
     create_todo issue_closed
     visit dashboard_todos_path
 
@@ -22,7 +26,7 @@ RSpec.describe 'Dashboard > Todo target states' do
   end
 
   it 'on an open issue todo does not have an open label' do
-    issue_open = create(:issue)
+    issue_open = create(:issue, project: project)
     create_todo issue_open
     visit dashboard_todos_path
 
@@ -32,7 +36,7 @@ RSpec.describe 'Dashboard > Todo target states' do
   end
 
   it 'on a merged merge request todo has merged label' do
-    mr_merged = create(:merge_request, :simple, :merged, author: user)
+    mr_merged = create(:merge_request, :simple, :merged, author: user, source_project: project)
     create_todo mr_merged
     visit dashboard_todos_path
 
@@ -42,7 +46,7 @@ RSpec.describe 'Dashboard > Todo target states' do
   end
 
   it 'on a closed merge request todo has closed label' do
-    mr_closed = create(:merge_request, :simple, :closed, author: user)
+    mr_closed = create(:merge_request, :simple, :closed, author: user, source_project: project)
     create_todo mr_closed
     visit dashboard_todos_path
 
@@ -52,7 +56,7 @@ RSpec.describe 'Dashboard > Todo target states' do
   end
 
   it 'on an open merge request todo does not have an open label' do
-    mr_open = create(:merge_request, :simple, author: user)
+    mr_open = create(:merge_request, :simple, author: user, source_project: project)
     create_todo mr_open
     visit dashboard_todos_path
 

spec/features/dashboard/todos/todos_filtering_spec.rb

@@ -128,7 +128,7 @@ RSpec.describe 'Dashboard > User filters todos', :js do
 
   describe 'filter by action' do
     before do
-      create(:todo, :build_failed, user: user_1, author: user_2, project: project_1)
+      create(:todo, :build_failed, user: user_1, author: user_2, project: project_1, target: merge_request)
       create(:todo, :marked, user: user_1, author: user_2, project: project_1, target: issue1)
       create(:todo, :review_requested, user: user_1, author: user_2, project: project_1, target: issue1)
     end

spec/features/dashboard/todos/todos_spec.rb

@@ -3,10 +3,16 @@
 require 'spec_helper'
 
 RSpec.describe 'Dashboard Todos' do
+  include DesignManagementTestHelpers
+
   let_it_be(:user)    { create(:user, username: 'john') }
   let_it_be(:author)  { create(:user) }
   let_it_be(:project) { create(:project, :public) }
-  let_it_be(:issue)   { create(:issue, due_date: Date.today, title: "Fix bug") }
+  let_it_be(:issue)   { create(:issue, project: project, due_date: Date.today, title: "Fix bug") }
+
+  before_all do
+    project.add_developer(user)
+  end
 
   context 'User does not have todos' do
     before do
@@ -21,8 +27,8 @@ RSpec.describe 'Dashboard Todos' do
 
   context 'when the todo references a merge request' do
     let(:referenced_mr) { create(:merge_request, source_project: project) }
-    let(:note) { create(:note, project: project, note: "Check out #{referenced_mr.to_reference}") }
-    let!(:todo) { create(:todo, :mentioned, user: user, project: project, author: author, note: note) }
+    let(:note) { create(:note, project: project, note: "Check out #{referenced_mr.to_reference}", noteable: create(:issue, project: project)) }
+    let!(:todo) { create(:todo, :mentioned, user: user, project: project, author: author, note: note, target: note.noteable) }
 
     before do
       sign_in(user)
@@ -39,9 +45,26 @@ RSpec.describe 'Dashboard Todos' do
     end
   end
 
-  context 'User has a todo', :js do
+  context 'user has an unauthorized todo' do
     before do
+      sign_in(user)
+    end
+
+    it 'does not render the todo' do
+      unauthorized_issue = create(:issue)
+      create(:todo, :mentioned, user: user, project: unauthorized_issue.project, target: unauthorized_issue, author: author)
       create(:todo, :mentioned, user: user, project: project, target: issue, author: author)
+
+      visit dashboard_todos_path
+
+      expect(page).to have_selector('.todos-list .todo', count: 1)
+    end
+  end
+
+  context 'User has a todo', :js do
+    let_it_be(:user_todo) { create(:todo, :mentioned, user: user, project: project, target: issue, author: author) }
+
+    before do
       sign_in(user)
 
       visit dashboard_todos_path
@@ -183,7 +206,7 @@ RSpec.describe 'Dashboard Todos' do
     end
 
     context 'approval todo' do
-      let(:merge_request) { create(:merge_request, title: "Fixes issue") }
+      let(:merge_request) { create(:merge_request, title: "Fixes issue", source_project: project) }
 
       before do
         create(:todo, :approval_required, user: user, project: project, target: merge_request, author: user)
@@ -199,7 +222,7 @@ RSpec.describe 'Dashboard Todos' do
     end
 
     context 'review request todo' do
-      let(:merge_request) { create(:merge_request, title: "Fixes issue") }
+      let(:merge_request) { create(:merge_request, title: "Fixes issue", source_project: project) }
 
       before do
         create(:todo, :review_requested, user: user, project: project, target: merge_request, author: user)
@@ -355,7 +378,7 @@ RSpec.describe 'Dashboard Todos' do
   end
 
   context 'User has a Build Failed todo' do
-    let!(:todo) { create(:todo, :build_failed, user: user, project: project, author: author) }
+    let!(:todo) { create(:todo, :build_failed, user: user, project: project, author: author, target: create(:merge_request, source_project: project)) }
 
     before do
       sign_in(user)
@@ -386,6 +409,7 @@ RSpec.describe 'Dashboard Todos' do
     end
 
     before do
+      enable_design_management
       project.add_developer(user)
       sign_in(user)
 

spec/graphql/mutations/todos/mark_done_spec.rb

@@ -5,17 +5,23 @@ require 'spec_helper'
 RSpec.describe Mutations::Todos::MarkDone do
   include GraphqlHelpers
 
+  let_it_be(:project) { create(:project) }
+  let_it_be(:issue) { create(:issue, project: project) }
   let_it_be(:current_user) { create(:user) }
   let_it_be(:author) { create(:user) }
   let_it_be(:other_user) { create(:user) }
 
-  let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending) }
-  let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done) }
+  let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending, target: issue) }
+  let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
 
   let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :pending) }
 
   let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) }
 
+  before_all do
+    project.add_developer(current_user)
+  end
+
   specify { expect(described_class).to require_graphql_authorizations(:update_todo) }
 
   describe '#resolve' do

spec/graphql/mutations/todos/restore_spec.rb

@@ -5,17 +5,23 @@ require 'spec_helper'
 RSpec.describe Mutations::Todos::Restore do
   include GraphqlHelpers
 
+  let_it_be(:project) { create(:project) }
+  let_it_be(:issue) { create(:issue, project: project) }
   let_it_be(:current_user) { create(:user) }
   let_it_be(:author) { create(:user) }
   let_it_be(:other_user) { create(:user) }
 
-  let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done) }
-  let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :pending) }
+  let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
+  let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :pending, target: issue) }
 
   let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :done) }
 
   let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) }
 
+  before_all do
+    project.add_developer(current_user)
+  end
+
   specify { expect(described_class).to require_graphql_authorizations(:update_todo) }
 
   describe '#resolve' do

spec/graphql/resolvers/todo_resolver_spec.rb

@@ -4,19 +4,28 @@ require 'spec_helper'
 
 RSpec.describe Resolvers::TodoResolver do
   include GraphqlHelpers
+  include DesignManagementTestHelpers
 
   specify do
     expect(described_class).to have_nullable_graphql_type(Types::TodoType.connection_type)
   end
 
   describe '#resolve' do
+    let_it_be(:project) { create(:project) }
     let_it_be(:current_user) { create(:user) }
+    let_it_be(:issue) { create(:issue, project: project) }
     let_it_be(:author1) { create(:user) }
     let_it_be(:author2) { create(:user) }
 
-    let_it_be(:merge_request_todo_pending) { create(:todo, user: current_user, target_type: 'MergeRequest', state: :pending, action: Todo::MENTIONED, author: author1) }
-    let_it_be(:issue_todo_done) { create(:todo, user: current_user, state: :done, action: Todo::ASSIGNED, author: author2) }
-    let_it_be(:issue_todo_pending) { create(:todo, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) }
+    let_it_be(:issue_todo_done) { create(:todo, user: current_user, state: :done, action: Todo::ASSIGNED, author: author2, target: issue) }
+    let_it_be(:issue_todo_pending) { create(:todo, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: issue) }
+
+    let(:merge_request) { create(:merge_request, source_project: project) }
+    let!(:merge_request_todo_pending) { create(:todo, user: current_user, target: merge_request, state: :pending, action: Todo::MENTIONED, author: author1) }
+
+    before_all do
+      project.add_developer(current_user)
+    end
 
     it 'calls TodosFinder' do
       expect_next_instance_of(TodosFinder) do |finder|
@@ -40,7 +49,9 @@ RSpec.describe Resolvers::TodoResolver do
       end
 
       it 'returns the todos for multiple filters' do
-        design_todo_pending = create(:todo, target_type: 'DesignManagement::Design', user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
+        enable_design_management
+        design = create(:design, issue: issue)
+        design_todo_pending = create(:todo, target: design, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
 
         todos = resolve_todos(type: ['MergeRequest', 'DesignManagement::Design'])
 
@@ -59,11 +70,15 @@ RSpec.describe Resolvers::TodoResolver do
         group3 = create(:group)
 
         group1.add_developer(current_user)
+        issue1 = create(:issue, project: create(:project, group: group1))
         group2.add_developer(current_user)
+        issue2 = create(:issue, project: create(:project, group: group2))
+        group3.add_developer(current_user)
+        issue3 = create(:issue, project: create(:project, group: group3))
 
-        todo4 = create(:todo, group: group1, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
-        todo5 = create(:todo, group: group2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
-        create(:todo, group: group3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
+        todo4 = create(:todo, group: group1, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: issue1)
+        todo5 = create(:todo, group: group2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: issue2)
+        create(:todo, group: group3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: issue3)
 
         todos = resolve_todos(group_id: [group2.id, group1.id])
 
@@ -93,9 +108,13 @@ RSpec.describe Resolvers::TodoResolver do
         project2 = create(:project)
         project3 = create(:project)
 
-        todo4 = create(:todo, project: project1, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
-        todo5 = create(:todo, project: project2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
-        create(:todo, project: project3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
+        project1.add_developer(current_user)
+        project2.add_developer(current_user)
+        project3.add_developer(current_user)
+
+        todo4 = create(:todo, project: project1, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: create(:issue, project: project1))
+        todo5 = create(:todo, project: project2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: create(:issue, project: project2))
+        create(:todo, project: project3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1, target: create(:issue, project: project3))
 
         todos = resolve_todos(project_id: [project2.id, project1.id])
 

spec/models/diff_note_spec.rb

@@ -552,4 +552,10 @@ RSpec.describe DiffNote do
       expect(subject.on_image?).to be_truthy
     end
   end
+
+  describe '#to_ability_name' do
+    subject { described_class.new.to_ability_name }
+
+    it { is_expected.to eq('note') }
+  end
 end

spec/models/discussion_note_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe DiscussionNote do
+  describe '#to_ability_name' do
+    subject { described_class.new.to_ability_name }
+
+    it { is_expected.to eq('note') }
+  end
+end

spec/models/legacy_diff_note_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe LegacyDiffNote do
+  describe '#to_ability_name' do
+    subject { described_class.new.to_ability_name }
+
+    it { is_expected.to eq('note') }
+  end
+end

spec/models/synthetic_note_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe SyntheticNote do
+  describe '#to_ability_name' do
+    subject { described_class.new.to_ability_name }
+
+    it { is_expected.to eq('note') }
+  end
+end

spec/policies/todo_policy_spec.rb

@@ -9,22 +9,28 @@ RSpec.describe TodoPolicy do
   let_it_be(:user2) { create(:user) }
   let_it_be(:user3) { create(:user) }
 
-  let_it_be(:todo1) { create(:todo, author: author, user: user1) }
-  let_it_be(:todo2) { create(:todo, author: author, user: user2) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:issue) { create(:issue, project: project) }
+
+  let_it_be(:todo1) { create(:todo, author: author, user: user1, issue: issue) }
+  let_it_be(:todo2) { create(:todo, author: author, user: user2, issue: issue) }
   let_it_be(:todo3) { create(:todo, author: author, user: user2) }
-  let_it_be(:todo4) { create(:todo, author: author, user: user3) }
+  let_it_be(:todo4) { create(:todo, author: author, user: user3, issue: issue) }
 
   def permissions(user, todo)
     described_class.new(user, todo)
   end
 
+  before_all do
+    project.add_developer(user1)
+    project.add_developer(user2)
+  end
+
   describe 'own_todo' do
-    it 'allows owners to access their own todos' do
+    it 'allows owners to access their own todos if they can read todo target' do
       [
         [user1, todo1],
-        [user2, todo2],
-        [user2, todo3],
-        [user3, todo4]
+        [user2, todo2]
       ].each do |user, todo|
         expect(permissions(user, todo)).to be_allowed(:read_todo)
       end
@@ -38,7 +44,9 @@ RSpec.describe TodoPolicy do
         [user2, todo4],
         [user3, todo1],
         [user3, todo2],
-        [user3, todo3]
+        [user3, todo3],
+        [user2, todo3],
+        [user3, todo4]
       ].each do |user, todo|
         expect(permissions(user, todo)).to be_disallowed(:read_todo)
       end

spec/requests/api/graphql/current_user/todos_query_spec.rb

@@ -4,12 +4,17 @@ require 'spec_helper'
 
 RSpec.describe 'Query current user todos' do
   include GraphqlHelpers
+  include DesignManagementTestHelpers
 
   let_it_be(:current_user) { create(:user) }
-  let_it_be(:commit_todo) { create(:on_commit_todo, user: current_user, project: create(:project, :repository)) }
-  let_it_be(:issue_todo) { create(:todo, user: current_user, target: create(:issue)) }
-  let_it_be(:merge_request_todo) { create(:todo, user: current_user, target: create(:merge_request)) }
-  let_it_be(:design_todo) { create(:todo, user: current_user, target: create(:design)) }
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:unauthorize_project) { create(:project) }
+  let_it_be(:commit_todo) { create(:on_commit_todo, user: current_user, project: project) }
+  let_it_be(:issue) { create(:issue, project: project) }
+  let_it_be(:issue_todo) { create(:todo, project: project, user: current_user, target: issue) }
+  let_it_be(:merge_request_todo) { create(:todo, project: project, user: current_user, target: create(:merge_request, source_project: project)) }
+  let_it_be(:design_todo) { create(:todo, project: project, user: current_user, target: create(:design, issue: issue)) }
+  let_it_be(:unauthorized_todo) { create(:todo, user: current_user, project: unauthorize_project, target: create(:issue, project: unauthorize_project)) }
 
   let(:fields) do
     <<~QUERY
@@ -23,16 +28,22 @@ RSpec.describe 'Query current user todos' do
     graphql_query_for('currentUser', {}, query_graphql_field('todos', {}, fields))
   end
 
+  before_all do
+    project.add_developer(current_user)
+  end
+
   subject { graphql_data.dig('currentUser', 'todos', 'nodes') }
 
   before do
+    enable_design_management
+
     post_graphql(query, current_user: current_user)
   end
 
   it_behaves_like 'a working graphql query'
 
   it 'contains the expected ids' do
-    is_expected.to include(
+    is_expected.to contain_exactly(
       a_hash_including('id' => commit_todo.to_global_id.to_s),
       a_hash_including('id' => issue_todo.to_global_id.to_s),
       a_hash_including('id' => merge_request_todo.to_global_id.to_s),
@@ -41,11 +52,33 @@ RSpec.describe 'Query current user todos' do
   end
 
   it 'returns Todos for all target types' do
-    is_expected.to include(
+    is_expected.to contain_exactly(
       a_hash_including('targetType' => 'COMMIT'),
       a_hash_including('targetType' => 'ISSUE'),
       a_hash_including('targetType' => 'MERGEREQUEST'),
       a_hash_including('targetType' => 'DESIGN')
     )
   end
+
+  context 'when requesting a single field' do
+    let(:fields) do
+      <<~QUERY
+      nodes {
+        id
+      }
+      QUERY
+    end
+
+    it 'avoids N+1 queries', :request_store do
+      control = ActiveRecord::QueryRecorder.new { post_graphql(query, current_user: current_user) }
+
+      project2 = create(:project)
+      project2.add_developer(current_user)
+      issue2 = create(:issue, project: project2)
+      create(:todo, user: current_user, target: issue2, project: project2)
+
+      # An additional query is made for each different group that owns a todo through a project
+      expect { post_graphql(query, current_user: current_user) }.not_to exceed_query_limit(control).with_threshold(2)
+    end
+  end
 end

spec/requests/api/graphql/mutations/todos/mark_all_done_spec.rb

@@ -5,14 +5,16 @@ require 'spec_helper'
 RSpec.describe 'Marking all todos done' do
   include GraphqlHelpers
 
+  let_it_be(:project) { create(:project) }
+  let_it_be(:issue) { create(:issue, project: project) }
   let_it_be(:current_user) { create(:user) }
   let_it_be(:author) { create(:user) }
   let_it_be(:other_user) { create(:user) }
   let_it_be(:other_user2) { create(:user) }
 
-  let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending) }
-  let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done) }
-  let_it_be(:todo3) { create(:todo, user: current_user, author: author, state: :pending) }
+  let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending, target: issue) }
+  let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
+  let_it_be(:todo3) { create(:todo, user: current_user, author: author, state: :pending, target: issue) }
 
   let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :pending) }
 
@@ -28,6 +30,10 @@ RSpec.describe 'Marking all todos done' do
     )
   end
 
+  before_all do
+    project.add_developer(current_user)
+  end
+
   def mutation_response
     graphql_mutation_response(:todos_mark_all_done)
   end

spec/requests/api/graphql/mutations/todos/mark_done_spec.rb

@@ -5,12 +5,14 @@ require 'spec_helper'
 RSpec.describe 'Marking todos done' do
   include GraphqlHelpers
 
+  let_it_be(:project) { create(:project) }
+  let_it_be(:issue) { create(:issue, project: project) }
   let_it_be(:current_user) { create(:user) }
   let_it_be(:author) { create(:user) }
   let_it_be(:other_user) { create(:user) }
 
-  let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending) }
-  let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done) }
+  let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :pending, target: issue) }
+  let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
 
   let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :pending) }
 
@@ -29,6 +31,10 @@ RSpec.describe 'Marking todos done' do
     )
   end
 
+  before_all do
+    project.add_developer(current_user)
+  end
+
   def mutation_response
     graphql_mutation_response(:todo_mark_done)
   end

spec/requests/api/graphql/mutations/todos/restore_many_spec.rb

@@ -5,12 +5,14 @@ require 'spec_helper'
 RSpec.describe 'Restoring many Todos' do
   include GraphqlHelpers
 
+  let_it_be(:project) { create(:project) }
+  let_it_be(:issue) { create(:issue, project: project) }
   let_it_be(:current_user) { create(:user) }
   let_it_be(:author) { create(:user) }
   let_it_be(:other_user) { create(:user) }
 
-  let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done) }
-  let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done) }
+  let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
+  let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
 
   let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :done) }
 
@@ -30,6 +32,10 @@ RSpec.describe 'Restoring many Todos' do
     )
   end
 
+  before_all do
+    project.add_developer(current_user)
+  end
+
   def mutation_response
     graphql_mutation_response(:todo_restore_many)
   end

spec/requests/api/graphql/mutations/todos/restore_spec.rb

@@ -5,12 +5,14 @@ require 'spec_helper'
 RSpec.describe 'Restoring Todos' do
   include GraphqlHelpers
 
+  let_it_be(:project) { create(:project) }
+  let_it_be(:issue) { create(:issue, project: project) }
   let_it_be(:current_user) { create(:user) }
   let_it_be(:author) { create(:user) }
   let_it_be(:other_user) { create(:user) }
 
-  let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done) }
-  let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :pending) }
+  let_it_be(:todo1) { create(:todo, user: current_user, author: author, state: :done, target: issue) }
+  let_it_be(:todo2) { create(:todo, user: current_user, author: author, state: :pending, target: issue) }
 
   let_it_be(:other_user_todo) { create(:todo, user: other_user, author: author, state: :done) }
 
@@ -29,6 +31,10 @@ RSpec.describe 'Restoring Todos' do
     )
   end
 
+  before_all do
+    project.add_developer(current_user)
+  end
+
   def mutation_response
     graphql_mutation_response(:todo_restore)
   end

spec/requests/api/todos_spec.rb

@@ -3,18 +3,22 @@
 require 'spec_helper'
 
 RSpec.describe API::Todos do
+  include DesignManagementTestHelpers
+
   let_it_be(:group) { create(:group) }
   let_it_be(:project_1) { create(:project, :repository, group: group) }
   let_it_be(:project_2) { create(:project) }
   let_it_be(:author_1) { create(:user) }
   let_it_be(:author_2) { create(:user) }
   let_it_be(:john_doe) { create(:user, username: 'john_doe') }
+  let_it_be(:issue) { create(:issue, project: project_1) }
   let_it_be(:merge_request) { create(:merge_request, source_project: project_1) }
   let_it_be(:merge_request_todo) { create(:todo, project: project_1, author: author_2, user: john_doe, target: merge_request) }
-  let_it_be(:pending_1) { create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe) }
-  let_it_be(:pending_2) { create(:todo, project: project_2, author: author_2, user: john_doe) }
+  let_it_be(:pending_1) { create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe, target: issue) }
+  let_it_be(:pending_2) { create(:todo, project: project_2, author: author_2, user: john_doe, target: issue) }
   let_it_be(:pending_3) { create(:on_commit_todo, project: project_1, author: author_2, user: john_doe) }
-  let_it_be(:done) { create(:todo, :done, project: project_1, author: author_1, user: john_doe) }
+  let_it_be(:pending_4) { create(:on_commit_todo, project: project_1, author: author_2, user: john_doe, commit_id: 'invalid_id') }
+  let_it_be(:done) { create(:todo, :done, project: project_1, author: author_1, user: john_doe, target: issue) }
   let_it_be(:award_emoji_1) { create(:award_emoji, awardable: merge_request, user: author_1, name: 'thumbsup') }
   let_it_be(:award_emoji_2) { create(:award_emoji, awardable: pending_1.target, user: author_1, name: 'thumbsup') }
   let_it_be(:award_emoji_3) { create(:award_emoji, awardable: pending_2.target, user: author_2, name: 'thumbsdown') }
@@ -77,13 +81,13 @@ RSpec.describe API::Todos do
         expect(json_response[0]['target_type']).to eq('Commit')
 
         expect(json_response[1]['target_type']).to eq('Issue')
-        expect(json_response[1]['target']['upvotes']).to eq(0)
+        expect(json_response[1]['target']['upvotes']).to eq(1)
         expect(json_response[1]['target']['downvotes']).to eq(1)
         expect(json_response[1]['target']['merge_requests_count']).to eq(0)
 
         expect(json_response[2]['target_type']).to eq('Issue')
         expect(json_response[2]['target']['upvotes']).to eq(1)
-        expect(json_response[2]['target']['downvotes']).to eq(0)
+        expect(json_response[2]['target']['downvotes']).to eq(1)
         expect(json_response[2]['target']['merge_requests_count']).to eq(0)
 
         expect(json_response[3]['target_type']).to eq('MergeRequest')
@@ -93,6 +97,19 @@ RSpec.describe API::Todos do
         expect(json_response[3]['target']['downvotes']).to eq(0)
       end
 
+      context "when current user does not have access to one of the TODO's target" do
+        it 'filters out unauthorized todos' do
+          no_access_project = create(:project, :repository, group: group)
+          no_access_merge_request = create(:merge_request, source_project: no_access_project)
+          no_access_todo = create(:todo, project: no_access_project, author: author_2, user: john_doe, target: no_access_merge_request)
+
+          get api('/todos', john_doe)
+
+          expect(json_response.count).to eq(4)
+          expect(json_response.map { |t| t['id'] }).not_to include(no_access_todo.id, pending_4.id)
+        end
+      end
+
       context 'and using the author filter' do
         it 'filters based on author_id param' do
           get api('/todos', john_doe), params: { author_id: author_2.id }
@@ -163,23 +180,31 @@ RSpec.describe API::Todos do
     end
 
     it 'avoids N+1 queries', :request_store do
+      create_issue_todo_for(john_doe)
       create(:todo, project: project_1, author: author_2, user: john_doe, target: merge_request)
 
       get api('/todos', john_doe)
 
-      control = ActiveRecord::QueryRecorder.new { get api('/todos', john_doe) }
+      control1 = ActiveRecord::QueryRecorder.new { get api('/todos', john_doe) }
+
+      create_issue_todo_for(john_doe)
+      create_mr_todo_for(john_doe, project_2)
+      create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe, target: merge_request)
+      new_todo = create_mr_todo_for(john_doe)
+      merge_request_3 = create(:merge_request, :jira_branch, source_project: new_todo.project)
+      create(:on_commit_todo, project: new_todo.project, author: author_1, user: john_doe, target: merge_request_3)
+      create(:todo, project: new_todo.project, author: author_2, user: john_doe, target: merge_request_3)
 
-      merge_request_2 = create(:merge_request, source_project: project_2)
-      create(:todo, project: project_2, author: author_2, user: john_doe, target: merge_request_2)
+      expect { get api('/todos', john_doe) }.not_to exceed_query_limit(control1).with_threshold(4)
+      control2 = ActiveRecord::QueryRecorder.new { get api('/todos', john_doe) }
 
-      project_3 = create(:project, :repository)
-      project_3.add_developer(john_doe)
-      merge_request_3 = create(:merge_request, source_project: project_3)
-      create(:todo, project: project_3, author: author_2, user: john_doe, target: merge_request_3)
-      create(:todo, :mentioned, project: project_1, author: author_1, user: john_doe)
-      create(:on_commit_todo, project: project_3, author: author_1, user: john_doe)
+      create_issue_todo_for(john_doe)
+      create_issue_todo_for(john_doe, project_1)
+      create_issue_todo_for(john_doe, project_1)
+
+      # Additional query only when target belongs to project from different group
+      expect { get api('/todos', john_doe) }.not_to exceed_query_limit(control2).with_threshold(1)
 
-      expect { get api('/todos', john_doe) }.not_to exceed_query_limit(control)
       expect(response).to have_gitlab_http_status(:ok)
     end
 
@@ -201,6 +226,8 @@ RSpec.describe API::Todos do
       end
 
       before do
+        enable_design_management
+
         api_request
       end
 
@@ -222,6 +249,20 @@ RSpec.describe API::Todos do
         )
       end
     end
+
+    def create_mr_todo_for(user, project = nil)
+      new_project = project || create(:project, group: create(:group))
+      new_project.add_developer(user) if project.blank?
+      new_merge_request = create(:merge_request, source_project: new_project)
+      create(:todo, project: new_project, author: user, user: user, target: new_merge_request)
+    end
+
+    def create_issue_todo_for(user, project = nil)
+      new_project = project || create(:project, group: create(:group))
+      new_project.group.add_developer(user) if project.blank?
+      issue = create(:issue, project: new_project)
+      create(:todo, project: new_project, target: issue, author: user, user: user)
+    end
   end
 
   describe 'POST /todos/:id/mark_as_done' do

spec/services/todos/allowed_target_filter_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Todos::AllowedTargetFilterService do
+  include DesignManagementTestHelpers
+
+  let_it_be(:authorized_group) { create(:group, :private) }
+  let_it_be(:authorized_project) { create(:project, group: authorized_group) }
+  let_it_be(:unauthorized_group) { create(:group, :private) }
+  let_it_be(:unauthorized_project) { create(:project, group: unauthorized_group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:authorized_issue) { create(:issue, project: authorized_project) }
+  let_it_be(:authorized_issue_todo) { create(:todo, project: authorized_project, target: authorized_issue, user: user) }
+  let_it_be(:unauthorized_issue) { create(:issue, project: unauthorized_project) }
+  let_it_be(:unauthorized_issue_todo) { create(:todo, project: unauthorized_project, target: unauthorized_issue, user: user) }
+  let_it_be(:authorized_design) { create(:design, issue: authorized_issue) }
+  let_it_be(:authorized_design_todo) { create(:todo, project: authorized_project, target: authorized_design, user: user) }
+  let_it_be(:unauthorized_design) { create(:design, issue: unauthorized_issue) }
+  let_it_be(:unauthorized_design_todo) { create(:todo, project: unauthorized_project, target: unauthorized_design, user: user) }
+
+  # Cannot use let_it_be with MRs
+  let(:authorized_mr) { create(:merge_request, source_project: authorized_project) }
+  let(:authorized_mr_todo) { create(:todo, project: authorized_project, user: user, target: authorized_mr) }
+  let(:unauthorized_mr) { create(:merge_request, source_project: unauthorized_project) }
+  let(:unauthorized_mr_todo) { create(:todo, project: unauthorized_project, user: user, target: unauthorized_mr) }
+
+  before_all do
+    authorized_group.add_developer(user)
+  end
+
+  describe '#execute' do
+    subject(:execute_service) { described_class.new(all_todos, user).execute }
+
+    let!(:all_todos) { authorized_todos + unauthorized_todos }
+
+    let(:authorized_todos) do
+      [
+        authorized_mr_todo,
+        authorized_issue_todo,
+        authorized_design_todo
+      ]
+    end
+
+    let(:unauthorized_todos) do
+      [
+        unauthorized_mr_todo,
+        unauthorized_issue_todo,
+        unauthorized_design_todo
+      ]
+    end
+
+    before do
+      enable_design_management
+    end
+
+    it { is_expected.to match_array(authorized_todos) }
+  end
+end