diff options
author | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-08-26 13:24:08 +0000 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-08-26 13:24:08 +0000 |
commit | c6b4fa0c9605ad45703248281c895aba1ed0d771 (patch) | |
tree | d91d192f2fc5aa9c741643ff62636ce99c5873f3 | |
parent | 55426415aaa324819cce3dcf39bdd88efa47f194 (diff) | |
parent | b0ebfa3d46084dc2b876d62ab8c6a06e84c4da8e (diff) | |
download | gitlab-ce-c6b4fa0c9605ad45703248281c895aba1ed0d771.tar.gz |
Merge branch 'security-64711-fix-commit-todos-12-1' into '12-1-stable'
Send TODOs for comments on commits correctly
See merge request gitlab/gitlabhq!3366
-rw-r--r-- | app/services/todo_service.rb | 6 | ||||
-rw-r--r-- | changelogs/unreleased/security-64711-fix-commit-todos.yml | 5 | ||||
-rw-r--r-- | spec/services/todo_service_spec.rb | 121 |
3 files changed, 112 insertions, 20 deletions
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 0ea230a44a1..b1256df35d6 100644 --- a/app/services/todo_service.rb +++ b/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) diff --git a/changelogs/unreleased/security-64711-fix-commit-todos.yml b/changelogs/unreleased/security-64711-fix-commit-todos.yml new file mode 100644 index 00000000000..ce4b3cdeeaf --- /dev/null +++ b/changelogs/unreleased/security-64711-fix-commit-todos.yml @@ -0,0 +1,5 @@ +--- +title: Send TODOs for comments on commits correctly +merge_request: +author: +type: security diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 9ee23f3eb48..bdf2f59704c 100644 --- a/spec/services/todo_service_spec.rb +++ b/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 |