From adab5dba43660a22118ea77038ef03fe0ded19f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 27 Sep 2016 11:29:06 +0200 Subject: [PATCH] Fix permission for setting an issue's due date MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- CHANGELOG | 1 + app/services/issuable_base_service.rb | 1 + .../slash_commands/interpret_service.rb | 4 +- .../issues/user_uses_slash_commands_spec.rb | 72 +++++++++++--- spec/services/issues/create_service_spec.rb | 36 +++++-- spec/services/issues/update_service_spec.rb | 95 ++++++++++++------- .../slash_commands/interpret_service_spec.rb | 71 ++++++++++++-- ...issuable_slash_commands_shared_examples.rb | 0 ..._service_slash_commands_shared_examples.rb | 0 9 files changed, 215 insertions(+), 65 deletions(-) rename spec/support/{ => features}/issuable_slash_commands_shared_examples.rb (100%) rename spec/support/{ => services}/issuable_create_service_slash_commands_shared_examples.rb (100%) diff --git a/CHANGELOG b/CHANGELOG index f92fcff951a..4512d7d0e75 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -7,6 +7,7 @@ v 8.13.0 (unreleased) - Log LDAP lookup errors and don't swallow unrelated exceptions. !6103 (Markus Koller) - Add more tests for calendar contribution (ClemMakesApps) - Avoid database queries on Banzai::ReferenceParser::BaseParser for nodes without references + - Fix permission for setting an issue's due date - Fix robots.txt disallowing access to groups starting with "s" (Matt Harrison) - Only update issuable labels if they have been changed - Revoke button in Applications Settings underlines on hover. diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index fbce46769f7..57d521f2fea 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -50,6 +50,7 @@ class IssuableBaseService < BaseService params.delete(:remove_label_ids) params.delete(:label_ids) params.delete(:assignee_id) + params.delete(:due_date) end end diff --git a/app/services/slash_commands/interpret_service.rb b/app/services/slash_commands/interpret_service.rb index 9ac1124abc1..ffcad5b3a87 100644 --- a/app/services/slash_commands/interpret_service.rb +++ b/app/services/slash_commands/interpret_service.rb @@ -195,7 +195,7 @@ module SlashCommands params '' condition do issuable.respond_to?(:due_date) && - current_user.can?(:"update_#{issuable.to_ability_name}", issuable) + current_user.can?(:"admin_#{issuable.to_ability_name}", project) end command :due do |due_date_param| due_date = Chronic.parse(due_date_param).try(:to_date) @@ -208,7 +208,7 @@ module SlashCommands issuable.persisted? && issuable.respond_to?(:due_date) && issuable.due_date? && - current_user.can?(:"update_#{issuable.to_ability_name}", issuable) + current_user.can?(:"admin_#{issuable.to_ability_name}", project) end command :remove_due_date do @updates[:due_date] = nil diff --git a/spec/features/issues/user_uses_slash_commands_spec.rb b/spec/features/issues/user_uses_slash_commands_spec.rb index 105629c485a..bf2b93c92fb 100644 --- a/spec/features/issues/user_uses_slash_commands_spec.rb +++ b/spec/features/issues/user_uses_slash_commands_spec.rb @@ -25,32 +25,78 @@ feature 'Issues > User uses slash commands', feature: true, js: true do describe 'adding a due date from note' do let(:issue) { create(:issue, project: project) } - it 'does not create a note, and sets the due date accordingly' do - write_note("/due 2016-08-28") + context 'when the current user can update the due date' do + it 'does not create a note, and sets the due date accordingly' do + write_note("/due 2016-08-28") - expect(page).not_to have_content '/due 2016-08-28' - expect(page).to have_content 'Your commands have been executed!' + expect(page).not_to have_content '/due 2016-08-28' + expect(page).to have_content 'Your commands have been executed!' - issue.reload + issue.reload - expect(issue.due_date).to eq Date.new(2016, 8, 28) + expect(issue.due_date).to eq Date.new(2016, 8, 28) + end + end + + context 'when the current user cannot update the due date' do + let(:guest) { create(:user) } + before do + project.team << [guest, :guest] + logout + login_with(guest) + visit namespace_project_issue_path(project.namespace, project, issue) + end + + it 'does not create a note, and sets the due date accordingly' do + write_note("/due 2016-08-28") + + expect(page).to have_content '/due 2016-08-28' + expect(page).not_to have_content 'Your commands have been executed!' + + issue.reload + + expect(issue.due_date).to be_nil + end end end describe 'removing a due date from note' do let(:issue) { create(:issue, project: project, due_date: Date.new(2016, 8, 28)) } - it 'does not create a note, and removes the due date accordingly' do - expect(issue.due_date).to eq Date.new(2016, 8, 28) + context 'when the current user can update the due date' do + it 'does not create a note, and removes the due date accordingly' do + expect(issue.due_date).to eq Date.new(2016, 8, 28) - write_note("/remove_due_date") + write_note("/remove_due_date") - expect(page).not_to have_content '/remove_due_date' - expect(page).to have_content 'Your commands have been executed!' + expect(page).not_to have_content '/remove_due_date' + expect(page).to have_content 'Your commands have been executed!' - issue.reload + issue.reload - expect(issue.due_date).to be_nil + expect(issue.due_date).to be_nil + end + end + + context 'when the current user cannot update the due date' do + let(:guest) { create(:user) } + before do + project.team << [guest, :guest] + logout + login_with(guest) + visit namespace_project_issue_path(project.namespace, project, issue) + end + + it 'does not create a note, and sets the due date accordingly' do + write_note("/remove_due_date") + + expect(page).to have_content '/remove_due_date' + expect(page).not_to have_content 'Your commands have been executed!' + + issue.reload + + expect(issue.due_date).to eq Date.new(2016, 8, 28) + end end end end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 58569ba96c3..1050502fa19 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -20,16 +20,38 @@ describe Issues::CreateService, services: true do let(:opts) do { title: 'Awesome issue', description: 'please fix', - assignee: assignee, + assignee_id: assignee.id, label_ids: labels.map(&:id), - milestone_id: milestone.id } + milestone_id: milestone.id, + due_date: Date.tomorrow } end - it { expect(issue).to be_valid } - it { expect(issue.title).to eq('Awesome issue') } - it { expect(issue.assignee).to eq assignee } - it { expect(issue.labels).to match_array labels } - it { expect(issue.milestone).to eq milestone } + it 'creates the issue with the given params' do + expect(issue).to be_persisted + expect(issue.title).to eq('Awesome issue') + expect(issue.assignee).to eq assignee + expect(issue.labels).to match_array labels + expect(issue.milestone).to eq milestone + expect(issue.due_date).to eq Date.tomorrow + end + + context 'when current user cannot admin issues in the project' do + let(:guest) { create(:user) } + before do + project.team << [guest, :guest] + end + + it 'filters out params that cannot be set without the :admin_issue permission' do + issue = described_class.new(project, guest, opts).execute + + expect(issue).to be_persisted + expect(issue.title).to eq('Awesome issue') + expect(issue.assignee).to be_nil + expect(issue.labels).to be_empty + expect(issue.milestone).to be_nil + expect(issue.due_date).to be_nil + end + end it 'creates a pending todo for new assignee' do attributes = { diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 4f5375a3583..1638a46ed51 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -32,55 +32,84 @@ describe Issues::UpdateService, services: true do described_class.new(project, user, opts).execute(issue) end - context "valid params" do - before do - opts = { + context 'valid params' do + let(:opts) do + { title: 'New title', description: 'Also please fix', assignee_id: user2.id, state_event: 'close', - label_ids: [label.id] + label_ids: [label.id], + due_date: Date.tomorrow } + end - perform_enqueued_jobs do - update_issue(opts) + it 'updates the issue with the given params' do + update_issue(opts) + + expect(issue).to be_valid + expect(issue.title).to eq 'New title' + expect(issue.description).to eq 'Also please fix' + expect(issue.assignee).to eq user2 + expect(issue).to be_closed + expect(issue.labels).to match_array [label] + expect(issue.due_date).to eq Date.tomorrow + end + + context 'when current user cannot admin issues in the project' do + let(:guest) { create(:user) } + before do + project.team << [guest, :guest] + end + + it 'filters out params that cannot be set without the :admin_issue permission' do + described_class.new(project, guest, opts).execute(issue) + + expect(issue).to be_valid + expect(issue.title).to eq 'New title' + expect(issue.description).to eq 'Also please fix' + expect(issue.assignee).to eq user3 + expect(issue.labels).to be_empty + expect(issue.milestone).to be_nil + expect(issue.due_date).to be_nil end end - it { expect(issue).to be_valid } - it { expect(issue.title).to eq('New title') } - it { expect(issue.assignee).to eq(user2) } - it { expect(issue).to be_closed } - it { expect(issue.labels.count).to eq(1) } - it { expect(issue.labels.first.title).to eq(label.name) } + context 'with background jobs processed' do + before do + perform_enqueued_jobs do + update_issue(opts) + end + end - it 'sends email to user2 about assign of new issue and email to user3 about issue unassignment' do - deliveries = ActionMailer::Base.deliveries - email = deliveries.last - recipients = deliveries.last(2).map(&:to).flatten - expect(recipients).to include(user2.email, user3.email) - expect(email.subject).to include(issue.title) - end + it 'sends email to user2 about assign of new issue and email to user3 about issue unassignment' do + deliveries = ActionMailer::Base.deliveries + email = deliveries.last + recipients = deliveries.last(2).map(&:to).flatten + expect(recipients).to include(user2.email, user3.email) + expect(email.subject).to include(issue.title) + end - it 'creates system note about issue reassign' do - note = find_note('Reassigned to') + it 'creates system note about issue reassign' do + note = find_note('Reassigned to') - expect(note).not_to be_nil - expect(note.note).to include "Reassigned to \@#{user2.username}" - end + expect(note).not_to be_nil + expect(note.note).to include "Reassigned to \@#{user2.username}" + end - it 'creates system note about issue label edit' do - note = find_note('Added ~') + it 'creates system note about issue label edit' do + note = find_note('Added ~') - expect(note).not_to be_nil - expect(note.note).to include "Added ~#{label.id} label" - end + expect(note).not_to be_nil + expect(note.note).to include "Added ~#{label.id} label" + end - it 'creates system note about title change' do - note = find_note('Changed title:') + it 'creates system note about title change' do + note = find_note('Changed title:') - expect(note).not_to be_nil - expect(note.note).to eq 'Changed title: **{-Old-} title** → **{+New+} title**' + expect(note).not_to be_nil + expect(note.note).to eq 'Changed title: **{-Old-} title** → **{+New+} title**' + end end end diff --git a/spec/services/slash_commands/interpret_service_spec.rb b/spec/services/slash_commands/interpret_service_spec.rb index a616275e883..5b1edba87a1 100644 --- a/spec/services/slash_commands/interpret_service_spec.rb +++ b/spec/services/slash_commands/interpret_service_spec.rb @@ -1,19 +1,19 @@ require 'spec_helper' describe SlashCommands::InterpretService, services: true do - let(:project) { create(:project) } - let(:user) { create(:user) } + let(:project) { create(:empty_project, :public) } + let(:developer) { create(:user) } let(:issue) { create(:issue, project: project) } let(:milestone) { create(:milestone, project: project, title: '9.10') } let(:inprogress) { create(:label, project: project, title: 'In Progress') } let(:bug) { create(:label, project: project, title: 'Bug') } before do - project.team << [user, :developer] + project.team << [developer, :developer] end describe '#execute' do - let(:service) { described_class.new(project, user) } + let(:service) { described_class.new(project, developer) } let(:merge_request) { create(:merge_request, source_project: project) } shared_examples 'reopen command' do @@ -45,13 +45,13 @@ describe SlashCommands::InterpretService, services: true do it 'fetches assignee and populates assignee_id if content contains /assign' do _, updates = service.execute(content, issuable) - expect(updates).to eq(assignee_id: user.id) + expect(updates).to eq(assignee_id: developer.id) end end shared_examples 'unassign command' do it 'populates assignee_id: nil if content contains /unassign' do - issuable.update(assignee_id: user.id) + issuable.update(assignee_id: developer.id) _, updates = service.execute(content, issuable) expect(updates).to eq(assignee_id: nil) @@ -124,7 +124,7 @@ describe SlashCommands::InterpretService, services: true do shared_examples 'done command' do it 'populates todo_event: "done" if content contains /done' do - TodoService.new.mark_todo(issuable, user) + TodoService.new.mark_todo(issuable, developer) _, updates = service.execute(content, issuable) expect(updates).to eq(todo_event: 'done') @@ -141,7 +141,7 @@ describe SlashCommands::InterpretService, services: true do shared_examples 'unsubscribe command' do it 'populates subscription_event: "unsubscribe" if content contains /unsubscribe' do - issuable.subscribe(user) + issuable.subscribe(developer) _, updates = service.execute(content, issuable) expect(updates).to eq(subscription_event: 'unsubscribe') @@ -209,12 +209,12 @@ describe SlashCommands::InterpretService, services: true do end it_behaves_like 'assign command' do - let(:content) { "/assign @#{user.username}" } + let(:content) { "/assign @#{developer.username}" } let(:issuable) { issue } end it_behaves_like 'assign command' do - let(:content) { "/assign @#{user.username}" } + let(:content) { "/assign @#{developer.username}" } let(:issuable) { merge_request } end @@ -380,5 +380,56 @@ describe SlashCommands::InterpretService, services: true do let(:content) { '/remove_due_date' } let(:issuable) { merge_request } end + + context 'when current_user cannot :admin_issue' do + let(:visitor) { create(:user) } + let(:issue) { create(:issue, project: project, author: visitor) } + let(:service) { described_class.new(project, visitor) } + + it_behaves_like 'empty command' do + let(:content) { "/assign @#{developer.username}" } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { '/unassign' } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { "/milestone %#{milestone.title}" } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { '/remove_milestone' } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { %(/label ~"#{inprogress.title}" ~#{bug.title} ~unknown) } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { %(/unlabel ~"#{inprogress.title}") } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { %(/relabel ~"#{inprogress.title}") } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { '/due tomorrow' } + let(:issuable) { issue } + end + + it_behaves_like 'empty command' do + let(:content) { '/remove_due_date' } + let(:issuable) { issue } + end + end end end diff --git a/spec/support/issuable_slash_commands_shared_examples.rb b/spec/support/features/issuable_slash_commands_shared_examples.rb similarity index 100% rename from spec/support/issuable_slash_commands_shared_examples.rb rename to spec/support/features/issuable_slash_commands_shared_examples.rb diff --git a/spec/support/issuable_create_service_slash_commands_shared_examples.rb b/spec/support/services/issuable_create_service_slash_commands_shared_examples.rb similarity index 100% rename from spec/support/issuable_create_service_slash_commands_shared_examples.rb rename to spec/support/services/issuable_create_service_slash_commands_shared_examples.rb