Merge branch 'security-64711-fix-commit-todos-ee' into 'master'

Send TODOs for comments on commits correctly

See merge request gitlab/gitlab-ee!1219

app/services/todo_service.rb

@@ -314,11 +314,9 @@ class TodoService
   end
 
   def reject_users_without_access(users, parent, target)
-    if target.is_a?(Note) && target.for_issuable?
-      target = target.noteable
-    end
+    target = target.noteable if target.is_a?(Note)
 
-    if target.is_a?(Issuable)
+    if target.respond_to?(:to_ability_name)
       select_users(users, :"read_#{target.to_ability_name}", target)
     else
       select_users(users, :read_project, parent)

changelogs/unreleased/security-64711-fix-commit-todos.yml

+---
+title: Send TODOs for comments on commits correctly
+merge_request:
+author:
+type: security

ee/app/models/design_management/design.rb

@@ -71,6 +71,10 @@ module DesignManagement
       filename
     end
 
+    def to_ability_name
+      'design'
+    end
+
     def description
       ''
     end

ee/spec/models/design_management/design_spec.rb

@@ -130,6 +130,10 @@ describe DesignManagement::Design do
     end
   end
 
+  describe '#to_ability_name' do
+    it { expect(described_class.new.to_ability_name).to eq('design') }
+  end
+
   describe '#status' do
     context 'the design is new' do
       subject { build(:design) }

ee/spec/services/todo_service_spec.rb

 require 'spec_helper'
 
 describe TodoService do
+  include DesignManagementTestHelpers
+
   let(:author) { create(:user, username: 'author') }
   let(:non_member) { create(:user, username: 'non_member') }
   let(:member) { create(:user, username: 'member') }
@@ -309,6 +311,8 @@ describe TodoService do
     let(:design) { create(:design, issue: issue) }
 
     before do
+      enable_design_management
+
       project.add_guest(author)
       project.add_developer(john_doe)
     end

spec/services/todo_service_spec.rb

@@ -436,25 +436,114 @@ describe TodoService do
         should_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_confidential_issue)
       end
 
-      context 'on commit' do
-        let(:project) { create(:project, :repository) }
-
-        it 'creates a todo for each valid mentioned user when leaving a note on commit' do
-          service.new_note(note_on_commit, john_doe)
-
-          should_create_todo(user: member, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
-          should_create_todo(user: author, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
-          should_create_todo(user: john_doe, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
-          should_not_create_todo(user: non_member, target_id: nil, target_type: 'Commit', commit_id: note_on_commit.commit_id, author: john_doe, action: Todo::MENTIONED, note: note_on_commit)
+      context 'commits' do
+        let(:base_commit_todo_attrs) { { target_id: nil, target_type: 'Commit', author: john_doe } }
+
+        context 'leaving a note on a commit in a public project' do
+          let(:project) { create(:project, :repository, :public) }
+          it 'creates a todo for each valid mentioned user' do
+            expected_todo = base_commit_todo_attrs.merge(
+              action: Todo::MENTIONED,
+              note: note_on_commit,
+              commit_id: note_on_commit.commit_id
+            )
+
+            service.new_note(note_on_commit, john_doe)
+
+            should_create_todo(expected_todo.merge(user: member))
+            should_create_todo(expected_todo.merge(user: author))
+            should_create_todo(expected_todo.merge(user: john_doe))
+            should_create_todo(expected_todo.merge(user: guest))
+            should_create_todo(expected_todo.merge(user: non_member))
+          end
+
+          it 'creates a directly addressed todo for each valid mentioned user' do
+            expected_todo = base_commit_todo_attrs.merge(
+              action: Todo::DIRECTLY_ADDRESSED,
+              note: addressed_note_on_commit,
+              commit_id: addressed_note_on_commit.commit_id
+            )
+
+            service.new_note(addressed_note_on_commit, john_doe)
+
+            should_create_todo(expected_todo.merge(user: member))
+            should_create_todo(expected_todo.merge(user: author))
+            should_create_todo(expected_todo.merge(user: john_doe))
+            should_create_todo(expected_todo.merge(user: guest))
+            should_create_todo(expected_todo.merge(user: non_member))
+          end
         end
 
-        it 'creates a directly addressed todo for each valid mentioned user when leaving a note on commit' do
-          service.new_note(addressed_note_on_commit, john_doe)
+        context 'leaving a note on a commit in a public project with private code' do
+          let(:project) { create(:project, :repository, :public, :repository_private) }
+
+          it 'creates a todo for each valid mentioned user' do
+            expected_todo = base_commit_todo_attrs.merge(
+              action: Todo::MENTIONED,
+              note: note_on_commit,
+              commit_id: note_on_commit.commit_id
+            )
+
+            service.new_note(note_on_commit, john_doe)
+
+            should_create_todo(expected_todo.merge(user: member))
+            should_create_todo(expected_todo.merge(user: author))
+            should_create_todo(expected_todo.merge(user: john_doe))
+            should_create_todo(expected_todo.merge(user: guest))
+            should_not_create_todo(expected_todo.merge(user: non_member))
+          end
+
+          it 'creates a directly addressed todo for each valid mentioned user' do
+            expected_todo = base_commit_todo_attrs.merge(
+              action: Todo::DIRECTLY_ADDRESSED,
+              note: addressed_note_on_commit,
+              commit_id: addressed_note_on_commit.commit_id
+            )
+
+            service.new_note(addressed_note_on_commit, john_doe)
+
+            should_create_todo(expected_todo.merge(user: member))
+            should_create_todo(expected_todo.merge(user: author))
+            should_create_todo(expected_todo.merge(user: john_doe))
+            should_create_todo(expected_todo.merge(user: guest))
+            should_not_create_todo(expected_todo.merge(user: non_member))
+          end
+        end
 
-          should_create_todo(user: member, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit)
-          should_create_todo(user: author, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit)
-          should_create_todo(user: john_doe, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit)
-          should_not_create_todo(user: non_member, target_id: nil, target_type: 'Commit', commit_id: addressed_note_on_commit.commit_id, author: john_doe, action: Todo::DIRECTLY_ADDRESSED, note: addressed_note_on_commit)
+        context 'leaving a note on a commit in a private project' do
+          let(:project) { create(:project, :repository, :private) }
+
+          it 'creates a todo for each valid mentioned user' do
+            expected_todo = base_commit_todo_attrs.merge(
+              action: Todo::MENTIONED,
+              note: note_on_commit,
+              commit_id: note_on_commit.commit_id
+            )
+
+            service.new_note(note_on_commit, john_doe)
+
+            should_create_todo(expected_todo.merge(user: member))
+            should_create_todo(expected_todo.merge(user: author))
+            should_create_todo(expected_todo.merge(user: john_doe))
+            should_not_create_todo(expected_todo.merge(user: guest))
+            should_not_create_todo(expected_todo.merge(user: non_member))
+          end
+
+          it 'creates a directly addressed todo for each valid mentioned user' do
+            expected_todo = base_commit_todo_attrs.merge(
+              action: Todo::DIRECTLY_ADDRESSED,
+              note: addressed_note_on_commit,
+              commit_id: addressed_note_on_commit.commit_id
+            )
+
+            service.new_note(addressed_note_on_commit, john_doe)
+
+            should_create_todo(expected_todo.merge(user: member))
+            should_create_todo(expected_todo.merge(user: author))
+            should_create_todo(expected_todo.merge(user: john_doe))
+            should_not_create_todo(expected_todo.merge(user: guest))
+            should_not_create_todo(expected_todo.merge(user: non_member))
+          end
         end
       end