Merge branch...

Merge branch '22592-can-set-due-date-through-slash-commands-even-though-i-m-not-authorized-to' into 'master'

Fix permission for setting an issue's due date

## What does this MR do?

This merge request ensure the current user can `:admin_issue` in order to change the issue's `due_date`, in `BaseIssuableService` and in `SlashCommands::InterpretService`.

Closes #22592 

## Are there points in the code the reviewer needs to double check?

No.

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [ ] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !6539

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.

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
 

app/services/slash_commands/interpret_service.rb

@@ -195,7 +195,7 @@ module SlashCommands
     params '<in 2 days | this Friday | December 31st>'
     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

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")
+
+          expect(page).not_to have_content '/remove_due_date'
+          expect(page).to have_content 'Your commands have been executed!'
+
+          issue.reload
+
+          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
 
-        write_note("/remove_due_date")
+        it 'does not create a note, and sets the due date accordingly' do
+          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).to have_content '/remove_due_date'
+          expect(page).not_to have_content 'Your commands have been executed!'
 
-        issue.reload
+          issue.reload
 
-        expect(issue.due_date).to be_nil
+          expect(issue.due_date).to eq Date.new(2016, 8, 28)
+        end
       end
     end
   end

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 = {

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
         }
-
-        perform_enqueued_jobs do
-          update_issue(opts)
-        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) }
-
-      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)
+      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
 
-      it 'creates system note about issue reassign' do
-        note = find_note('Reassigned to')
+      context 'when current user cannot admin issues in the project' do
+        let(:guest) { create(:user) }
+        before do
+          project.team << [guest, :guest]
+        end
 
-        expect(note).not_to be_nil
-        expect(note.note).to include "Reassigned to \@#{user2.username}"
+        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 'creates system note about issue label edit' do
-        note = find_note('Added ~')
+      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
 
-        expect(note).not_to be_nil
-        expect(note.note).to include "Added ~#{label.id} label"
-      end
+        it 'creates system note about issue reassign' do
+          note = find_note('Reassigned to')
 
-      it 'creates system note about title change' do
-        note = find_note('Changed title:')
+          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 eq 'Changed title: **{-Old-} title** → **{+New+} title**'
+        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
+
+        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**'
+        end
       end
     end
 

spec/services/slash_commands/interpret_service_spec.rb

 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