Enforce permissions in `{Issues,MergeRequests}::{Close,Reopen}Service`

Signed-off-by: Rémy Coutable <remy@rymai.me>

9 files changed, 178 insertions, 21 deletions

diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb
index 859c934ea3b..45cca216ccc 100644
--- a/app/services/issues/close_service.rb
+++ b/app/services/issues/close_service.rb
@@ -1,6 +1,8 @@
 module Issues
   class CloseService < Issues::BaseService
     def execute(issue, commit: nil, notifications: true, system_note: true)
+      return issue unless can?(current_user, :update_issue, issue)
+
       if project.jira_tracker? && project.jira_service.active
         project.jira_service.execute(commit, issue)
         todo_service.close_issue(issue, current_user)
diff --git a/app/services/issues/reopen_service.rb b/app/services/issues/reopen_service.rb
index e48ca359f4f..40fbe354492 100644
--- a/app/services/issues/reopen_service.rb
+++ b/app/services/issues/reopen_service.rb
@@ -1,6 +1,8 @@
 module Issues
   class ReopenService < Issues::BaseService
     def execute(issue)
+      return issue unless can?(current_user, :update_issue, issue)
+
       if issue.reopen
         event_service.reopen_issue(issue, current_user)
         create_note(issue)
diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb
index 27ee81fe3e7..f2053bda83a 100644
--- a/app/services/merge_requests/close_service.rb
+++ b/app/services/merge_requests/close_service.rb
@@ -1,6 +1,8 @@
 module MergeRequests
   class CloseService < MergeRequests::BaseService
     def execute(merge_request, commit = nil)
+      return merge_request unless can?(current_user, :update_merge_request, merge_request)
+
       # If we close MergeRequest we want to ignore validation
       # so we can close broken one (Ex. fork project removed)
       merge_request.allow_broken = true
diff --git a/app/services/merge_requests/reopen_service.rb b/app/services/merge_requests/reopen_service.rb
index eb88ae9d11c..fadcce5d9b6 100644
--- a/app/services/merge_requests/reopen_service.rb
+++ b/app/services/merge_requests/reopen_service.rb
@@ -1,6 +1,8 @@
 module MergeRequests
   class ReopenService < MergeRequests::BaseService
     def execute(merge_request)
+      return merge_request unless can?(current_user, :update_merge_request, merge_request)
+
       if merge_request.reopen
         event_service.reopen_mr(merge_request, current_user)
         create_note(merge_request)
diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb
index 1318607a388..aff022a573e 100644
--- a/spec/services/issues/close_service_spec.rb
+++ b/spec/services/issues/close_service_spec.rb
@@ -3,6 +3,7 @@ require 'spec_helper'
 describe Issues::CloseService, services: true do
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
+  let(:guest) { create(:user) }
   let(:issue) { create(:issue, assignee: user2) }
   let(:project) { issue.project }
   let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) }
@@ -10,13 +11,14 @@ describe Issues::CloseService, services: true do
   before do
     project.team << [user, :master]
     project.team << [user2, :developer]
+    project.team << [guest, :guest]
   end
 
   describe '#execute' do
     context "valid params" do
       before do
         perform_enqueued_jobs do
-          @issue = Issues::CloseService.new(project, user, {}).execute(issue)
+          @issue = described_class.new(project, user, {}).execute(issue)
         end
       end
 
@@ -39,10 +41,22 @@ describe Issues::CloseService, services: true do
       end
     end
 
+    context 'current user is not authorized to close issue' do
+      before do
+        perform_enqueued_jobs do
+          @issue = described_class.new(project, guest).execute(issue)
+        end
+      end
+
+      it 'does not close the issue' do
+        expect(@issue).to be_open
+      end
+    end
+
     context "external issue tracker" do
       before do
         allow(project).to receive(:default_issues_tracker?).and_return(false)
-        @issue = Issues::CloseService.new(project, user, {}).execute(issue)
+        @issue = described_class.new(project, user, {}).execute(issue)
       end
 
       it { expect(@issue).to be_valid }
diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb
new file mode 100644
index 00000000000..34a89fcd4e1
--- /dev/null
+++ b/spec/services/issues/reopen_service_spec.rb
@@ -0,0 +1,25 @@
+require 'spec_helper'
+
+describe Issues::ReopenService, services: true do
+  let(:guest) { create(:user) }
+  let(:issue) { create(:issue, :closed) }
+  let(:project) { issue.project }
+
+  before do
+    project.team << [guest, :guest]
+  end
+
+  describe '#execute' do
+    context 'current user is not authorized to reopen issue' do
+      before do
+        perform_enqueued_jobs do
+          @issue = described_class.new(project, guest).execute(issue)
+        end
+      end
+
+      it 'does not reopen the issue' do
+        expect(@issue).to be_closed
+      end
+    end
+  end
+end
diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb
index 403533be5d9..24c25e4350f 100644
--- a/spec/services/merge_requests/close_service_spec.rb
+++ b/spec/services/merge_requests/close_service_spec.rb
@@ -3,6 +3,7 @@ require 'spec_helper'
 describe MergeRequests::CloseService, services: true do
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
+  let(:guest) { create(:user) }
   let(:merge_request) { create(:merge_request, assignee: user2) }
   let(:project) { merge_request.project }
   let!(:todo) { create(:todo, :assigned, user: user, project: project, target: merge_request, author: user2) }
@@ -10,11 +11,12 @@ describe MergeRequests::CloseService, services: true do
   before do
     project.team << [user, :master]
     project.team << [user2, :developer]
+    project.team << [guest, :guest]
   end
 
   describe '#execute' do
     context 'valid params' do
-      let(:service) { MergeRequests::CloseService.new(project, user, {}) }
+      let(:service) { described_class.new(project, user, {}) }
 
       before do
         allow(service).to receive(:execute_hooks)
@@ -47,5 +49,17 @@ describe MergeRequests::CloseService, services: true do
         expect(todo.reload).to be_done
       end
     end
+
+    context 'current user is not authorized to close merge request' do
+      before do
+        perform_enqueued_jobs do
+          @merge_request = described_class.new(project, guest).execute(merge_request)
+        end
+      end
+
+      it 'does not close the merge request' do
+        expect(@merge_request).to be_open
+      end
+    end
   end
 end
diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb
index 3419b8bf5e6..af7424a76a9 100644
--- a/spec/services/merge_requests/reopen_service_spec.rb
+++ b/spec/services/merge_requests/reopen_service_spec.rb
@@ -3,22 +3,23 @@ require 'spec_helper'
 describe MergeRequests::ReopenService, services: true do
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
-  let(:merge_request) { create(:merge_request, assignee: user2) }
+  let(:guest) { create(:user) }
+  let(:merge_request) { create(:merge_request, :closed, assignee: user2) }
   let(:project) { merge_request.project }
 
   before do
     project.team << [user, :master]
     project.team << [user2, :developer]
+    project.team << [guest, :guest]
   end
 
   describe '#execute' do
     context 'valid params' do
-      let(:service) { MergeRequests::ReopenService.new(project, user, {}) }
+      let(:service) { described_class.new(project, user, {}) }
 
       before do
         allow(service).to receive(:execute_hooks)
 
-        merge_request.state = :closed
         perform_enqueued_jobs do
           service.execute(merge_request)
         end
@@ -43,5 +44,17 @@ describe MergeRequests::ReopenService, services: true do
         expect(note.note).to include 'Status changed to reopened'
       end
     end
+
+    context 'current user is not authorized to reopen merge request' do
+      before do
+        perform_enqueued_jobs do
+          @merge_request = described_class.new(project, guest).execute(merge_request)
+        end
+      end
+
+      it 'does not reopen the merge request' do
+        expect(@merge_request).to be_closed
+      end
+    end
   end
 end
diff --git a/spec/support/issuable_slash_commands_shared_examples.rb b/spec/support/issuable_slash_commands_shared_examples.rb
index 0c8bd69add6..3b8783cb053 100644
--- a/spec/support/issuable_slash_commands_shared_examples.rb
+++ b/spec/support/issuable_slash_commands_shared_examples.rb
@@ -2,8 +2,9 @@
 # It takes a `issuable_type`, and expect an `issuable`.
 
 shared_examples 'issuable record that supports slash commands in its description and notes' do |issuable_type|
-  let(:user) { create(:user) }
+  let(:master) { create(:user) }
   let(:assignee) { create(:user, username: 'bob') }
+  let(:guest) { create(:user) }
   let(:project) { create(:project, :public) }
   let!(:milestone) { create(:milestone, project: project, title: 'ASAP') }
   let!(:label_bug) { create(:label, project: project, title: 'bug') }
@@ -11,9 +12,10 @@ shared_examples 'issuable record that supports slash commands in its description
   let(:new_url_opts) { {} }
 
   before do
-    project.team << [user, :master]
+    project.team << [master, :master]
     project.team << [assignee, :developer]
-    login_with(user)
+    project.team << [guest, :guest]
+    login_with(master)
   end
 
   describe "new #{issuable_type}" do
@@ -83,6 +85,87 @@ shared_examples 'issuable record that supports slash commands in its description
       end
     end
 
+    context "with a note closing the #{issuable_type}" do
+      before do
+        expect(issuable).to be_open
+      end
+
+      context "when current user can close #{issuable_type}" do
+        it "closes the #{issuable_type}" do
+          page.within('.js-main-target-form') do
+            fill_in 'note[note]', with: "/close"
+            click_button 'Comment'
+          end
+
+          expect(page).not_to have_content '/close'
+          expect(page).to have_content 'Your commands are being executed.'
+
+          expect(issuable.reload).to be_closed
+        end
+      end
+
+      context "when current user cannot close #{issuable_type}" do
+        before do
+          logout
+          login_with(guest)
+          visit public_send("namespace_project_#{issuable_type}_path", project.namespace, project, issuable)
+        end
+
+        it "does not close the #{issuable_type}" do
+          page.within('.js-main-target-form') do
+            fill_in 'note[note]', with: "/close"
+            click_button 'Comment'
+          end
+
+          expect(page).not_to have_content '/close'
+          expect(page).to have_content 'Your commands are being executed.'
+
+          expect(issuable).to be_open
+        end
+      end
+    end
+
+    context "with a note reopening the #{issuable_type}" do
+      before do
+        issuable.close
+        expect(issuable).to be_closed
+      end
+
+      context "when current user can reopen #{issuable_type}" do
+        it "reopens the #{issuable_type}" do
+          page.within('.js-main-target-form') do
+            fill_in 'note[note]', with: "/reopen"
+            click_button 'Comment'
+          end
+
+          expect(page).not_to have_content '/reopen'
+          expect(page).to have_content 'Your commands are being executed.'
+
+          expect(issuable.reload).to be_open
+        end
+      end
+
+      context "when current user cannot reopen #{issuable_type}" do
+        before do
+          logout
+          login_with(guest)
+          visit public_send("namespace_project_#{issuable_type}_path", project.namespace, project, issuable)
+        end
+
+        it "does not reopen the #{issuable_type}" do
+          page.within('.js-main-target-form') do
+            fill_in 'note[note]', with: "/reopen"
+            click_button 'Comment'
+          end
+
+          expect(page).not_to have_content '/reopen'
+          expect(page).to have_content 'Your commands are being executed.'
+
+          expect(issuable).to be_closed
+        end
+      end
+    end
+
     context "with a note marking the #{issuable_type} as todo" do
       it "creates a new todo for the #{issuable_type}" do
         page.within('.js-main-target-form') do
@@ -93,31 +176,31 @@ shared_examples 'issuable record that supports slash commands in its description
         expect(page).not_to have_content '/todo'
         expect(page).to have_content 'Your commands are being executed.'
 
-        todos = TodosFinder.new(user).execute
+        todos = TodosFinder.new(master).execute
         todo = todos.first
 
         expect(todos.size).to eq 1
         expect(todo).to be_pending
         expect(todo.target).to eq issuable
-        expect(todo.author).to eq user
-        expect(todo.user).to eq user
+        expect(todo.author).to eq master
+        expect(todo.user).to eq master
       end
     end
 
     context "with a note marking the #{issuable_type} as done" do
       before do
-        TodoService.new.mark_todo(issuable, user)
+        TodoService.new.mark_todo(issuable, master)
       end
 
       it "creates a new todo for the #{issuable_type}" do
-        todos = TodosFinder.new(user).execute
+        todos = TodosFinder.new(master).execute
         todo = todos.first
 
         expect(todos.size).to eq 1
         expect(todos.first).to be_pending
         expect(todo.target).to eq issuable
-        expect(todo.author).to eq user
-        expect(todo.user).to eq user
+        expect(todo.author).to eq master
+        expect(todo.user).to eq master
 
         page.within('.js-main-target-form') do
           fill_in 'note[note]', with: "/done"
@@ -133,7 +216,7 @@ shared_examples 'issuable record that supports slash commands in its description
 
     context "with a note subscribing to the #{issuable_type}" do
       it "creates a new todo for the #{issuable_type}" do
-        expect(issuable.subscribed?(user)).to be_falsy
+        expect(issuable.subscribed?(master)).to be_falsy
 
         page.within('.js-main-target-form') do
           fill_in 'note[note]', with: "/subscribe"
@@ -143,17 +226,17 @@ shared_examples 'issuable record that supports slash commands in its description
         expect(page).not_to have_content '/subscribe'
         expect(page).to have_content 'Your commands are being executed.'
 
-        expect(issuable.subscribed?(user)).to be_truthy
+        expect(issuable.subscribed?(master)).to be_truthy
       end
     end
 
     context "with a note unsubscribing to the #{issuable_type} as done" do
       before do
-        issuable.subscribe(user)
+        issuable.subscribe(master)
       end
 
       it "creates a new todo for the #{issuable_type}" do
-        expect(issuable.subscribed?(user)).to be_truthy
+        expect(issuable.subscribed?(master)).to be_truthy
 
         page.within('.js-main-target-form') do
           fill_in 'note[note]', with: "/unsubscribe"
@@ -163,7 +246,7 @@ shared_examples 'issuable record that supports slash commands in its description
         expect(page).not_to have_content '/unsubscribe'
         expect(page).to have_content 'Your commands are being executed.'
 
-        expect(issuable.subscribed?(user)).to be_falsy
+        expect(issuable.subscribed?(master)).to be_falsy
       end
     end
   end