Merge branch '56864-reopen-locked-mr' into 'master'

Disallow reopening of locked merge request

Closes #56864

See merge request gitlab-org/gitlab-ce!24882

9 files changed, 68 insertions, 6 deletions

diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb
index ecb2797d1d9..537319addc2 100644
--- a/app/policies/issuable_policy.rb
+++ b/app/policies/issuable_policy.rb
@@ -17,6 +17,7 @@ class IssuablePolicy < BasePolicy
     enable :reopen_issue
     enable :read_merge_request
     enable :update_merge_request
+    enable :reopen_merge_request
   end
 
   rule { locked & ~is_project_member }.policy do
diff --git a/app/policies/merge_request_policy.rb b/app/policies/merge_request_policy.rb
index a2950951d03..a3692857ff4 100644
--- a/app/policies/merge_request_policy.rb
+++ b/app/policies/merge_request_policy.rb
@@ -1,4 +1,7 @@
 # frozen_string_literal: true
 
 class MergeRequestPolicy < IssuablePolicy
+  rule { locked }.policy do
+    prevent :reopen_merge_request
+  end
 end
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index cf257ed47c8..9f9f5230040 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -231,6 +231,7 @@ class ProjectPolicy < BasePolicy
     enable :admin_merge_request
     enable :admin_milestone
     enable :update_merge_request
+    enable :reopen_merge_request
     enable :create_commit_status
     enable :update_commit_status
     enable :create_build
diff --git a/app/services/merge_requests/reopen_service.rb b/app/services/merge_requests/reopen_service.rb
index f6cbe769ef4..f87005bcb6c 100644
--- a/app/services/merge_requests/reopen_service.rb
+++ b/app/services/merge_requests/reopen_service.rb
@@ -3,7 +3,7 @@
 module MergeRequests
   class ReopenService < MergeRequests::BaseService
     def execute(merge_request)
-      return merge_request unless can?(current_user, :update_merge_request, merge_request)
+      return merge_request unless can?(current_user, :reopen_merge_request, merge_request)
 
       if merge_request.reopen
         create_event(merge_request)
diff --git a/app/views/projects/merge_requests/_mr_title.html.haml b/app/views/projects/merge_requests/_mr_title.html.haml
index 3cd83feb842..70011d58c8a 100644
--- a/app/views/projects/merge_requests/_mr_title.html.haml
+++ b/app/views/projects/merge_requests/_mr_title.html.haml
@@ -1,4 +1,5 @@
 - can_update_merge_request = can?(current_user, :update_merge_request, @merge_request)
+- can_reopen_merge_request = can?(current_user, :reopen_merge_request, @merge_request)
 
 - if @merge_request.closed_without_fork?
   .alert.alert-danger
@@ -33,10 +34,11 @@
           - if can_update_merge_request
             %li{ class: [merge_request_button_visibility(@merge_request, true), 'js-close-item'] }
               = link_to 'Close', merge_request_path(@merge_request, merge_request: { state_event: :close }), method: :put, title: 'Close merge request'
+          - if can_reopen_merge_request
             %li{ class: merge_request_button_visibility(@merge_request, false) }
               = link_to 'Reopen', merge_request_path(@merge_request, merge_request: { state_event: :reopen }), method: :put, class: 'reopen-mr-link', title: 'Reopen merge request'
 
       - if can_update_merge_request
         = link_to 'Edit', edit_project_merge_request_path(@project, @merge_request), class: "d-none d-sm-none d-md-block btn btn-grouped js-issuable-edit qa-edit-button"
 
-      = render 'shared/issuable/close_reopen_button', issuable: @merge_request, can_update: can_update_merge_request, can_reopen: can_update_merge_request
+      = render 'shared/issuable/close_reopen_button', issuable: @merge_request, can_update: can_update_merge_request, can_reopen: can_reopen_merge_request
diff --git a/changelogs/unreleased/56864-reopen-locked-mr.yml b/changelogs/unreleased/56864-reopen-locked-mr.yml
new file mode 100644
index 00000000000..d1d71531ac8
--- /dev/null
+++ b/changelogs/unreleased/56864-reopen-locked-mr.yml
@@ -0,0 +1,5 @@
+---
+title: Disallow reopening of a locked merge request
+merge_request: 24882
+author: Jan Beckmann
+type: fixed
diff --git a/doc/user/discussions/index.md b/doc/user/discussions/index.md
index 90936034fea..a15e1a59921 100644
--- a/doc/user/discussions/index.md
+++ b/doc/user/discussions/index.md
@@ -276,7 +276,7 @@ edit existing comments. Non-team members are restricted from adding or editing c
 | :-----------: | :----------: |
 | ![Comment form member](img/lock_form_member.png) | ![Comment form non-member](img/lock_form_non_member.png) |
 
-Additionally locked issues can not be reopened.
+Additionally, locked issues and merge requests can not be reopened.
 
 ## Filtering notes
 
diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb
index db3df760472..6d34b0a8b4b 100644
--- a/spec/policies/issuable_policy_spec.rb
+++ b/spec/policies/issuable_policy_spec.rb
@@ -13,7 +13,7 @@ describe IssuablePolicy, models: true do
 
       context 'when user is able to read project' do
         it 'enables user to read and update issuables' do
-          expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request)
+          expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request, :reopen_merge_request)
         end
       end
 
@@ -24,12 +24,12 @@ describe IssuablePolicy, models: true do
           it 'enables user to read and update issuables' do
             project.add_maintainer(user)
 
-            expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request)
+            expect(policies).to be_allowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request, :reopen_merge_request)
           end
         end
 
         it 'disallows user from reading and updating issuables from that project' do
-          expect(policies).to be_disallowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request)
+          expect(policies).to be_disallowed(:read_issue, :update_issue, :reopen_issue, :read_merge_request, :update_merge_request, :reopen_merge_request)
         end
       end
     end
diff --git a/spec/policies/merge_request_policy_spec.rb b/spec/policies/merge_request_policy_spec.rb
new file mode 100644
index 00000000000..1efa70addc2
--- /dev/null
+++ b/spec/policies/merge_request_policy_spec.rb
@@ -0,0 +1,50 @@
+require 'spec_helper'
+
+describe MergeRequestPolicy do
+  let(:guest) { create(:user) }
+  let(:author) { create(:user) }
+  let(:developer) { create(:user) }
+  let(:project) { create(:project, :public) }
+
+  def permissions(user, merge_request)
+    described_class.new(user, merge_request)
+  end
+
+  before do
+    project.add_guest(guest)
+    project.add_guest(author)
+    project.add_developer(developer)
+  end
+
+  context 'when merge request is unlocked' do
+    let(:merge_request) { create(:merge_request, :closed, source_project: project, target_project: project, author: author) }
+
+    it 'allows author to reopen merge request' do
+      expect(permissions(author, merge_request)).to be_allowed(:reopen_merge_request)
+    end
+
+    it 'allows developer to reopen merge request' do
+      expect(permissions(developer, merge_request)).to be_allowed(:reopen_merge_request)
+    end
+
+    it 'prevents guest from reopening merge request' do
+      expect(permissions(guest, merge_request)).to be_disallowed(:reopen_merge_request)
+    end
+  end
+
+  context 'when merge request is locked' do
+    let(:merge_request_locked) { create(:merge_request, :closed, discussion_locked: true, source_project: project, target_project: project, author: author) }
+
+    it 'prevents author from reopening merge request' do
+      expect(permissions(author, merge_request_locked)).to be_disallowed(:reopen_merge_request)
+    end
+
+    it 'prevents developer from reopening merge request' do
+      expect(permissions(developer, merge_request_locked)).to be_disallowed(:reopen_merge_request)
+    end
+
+    it 'prevents guests from reopening merge request' do
+      expect(permissions(guest, merge_request_locked)).to be_disallowed(:reopen_merge_request)
+    end
+  end
+end