Restrict reopening locked issues for issue authors39665-restrict-issue-reopen

9 files changed, 53 insertions, 24 deletions

diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb
index 198bb168d85..6d8b575102e 100644
--- a/app/policies/issuable_policy.rb
+++ b/app/policies/issuable_policy.rb
@@ -14,6 +14,7 @@ class IssuablePolicy < BasePolicy
   rule { assignee_or_author }.policy do
     enable :read_issue
     enable :update_issue
+    enable :reopen_issue
     enable :read_merge_request
     enable :update_merge_request
   end
diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb
index 94b5f37c682..2d379e7eecf 100644
--- a/app/policies/issue_policy.rb
+++ b/app/policies/issue_policy.rb
@@ -19,4 +19,8 @@ class IssuePolicy < IssuablePolicy
     prevent :update_issue
     prevent :admin_issue
   end
+
+  rule { locked  }.policy do
+    prevent :reopen_issue
+  end
 end
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index f52a3bad77d..50d39d826a2 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -176,6 +176,7 @@ class ProjectPolicy < BasePolicy
     enable :fork_project
     enable :create_project_snippet
     enable :update_issue
+    enable :reopen_issue
     enable :admin_issue
     enable :admin_label
     enable :admin_list
diff --git a/app/services/issues/reopen_service.rb b/app/services/issues/reopen_service.rb
index 3bd53f9ccdc..56d59b235a7 100644
--- a/app/services/issues/reopen_service.rb
+++ b/app/services/issues/reopen_service.rb
@@ -3,7 +3,7 @@
 module Issues
   class ReopenService < Issues::BaseService
     def execute(issue)
-      return issue unless can?(current_user, :update_issue, issue)
+      return issue unless can?(current_user, :reopen_issue, issue)
 
       if issue.reopen
         event_service.reopen_issue(issue, current_user)
diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml
index 2d036bd4e3e..b81d1a188f0 100644
--- a/app/views/projects/issues/show.html.haml
+++ b/app/views/projects/issues/show.html.haml
@@ -6,6 +6,7 @@
 - page_card_attributes @issue.card_attributes
 
 - can_update_issue = can?(current_user, :update_issue, @issue)
+- can_reopen_issue = can?(current_user, :reopen_issue, @issue)
 - can_report_spam = @issue.submittable_as_spam_by?(current_user)
 - can_create_issue = show_new_issue_link?(@project)
 
@@ -40,6 +41,7 @@
             %li= link_to 'Report abuse', new_abuse_report_path(user_id: @issue.author.id, ref_url: issue_url(@issue))
           - if can_update_issue
             %li= link_to 'Close issue', issue_path(@issue, issue: { state_event: :close }, format: 'json'), class: "btn-close js-btn-issue-action #{issue_button_visibility(@issue, true)}", title: 'Close issue'
+          - if can_reopen_issue
             %li= link_to 'Reopen issue', issue_path(@issue, issue: { state_event: :reopen }, format: 'json'), class: "btn-reopen js-btn-issue-action #{issue_button_visibility(@issue, false)}", title: 'Reopen issue'
           - if can_report_spam
             %li= link_to 'Submit as spam', mark_as_spam_project_issue_path(@project, @issue), method: :post, class: 'btn-spam', title: 'Submit as spam'
@@ -48,7 +50,7 @@
               %li.divider
             %li= link_to 'New issue', new_project_issue_path(@project), title: 'New issue', id: 'new_issue_link'
 
-    = render 'shared/issuable/close_reopen_button', issuable: @issue, can_update: can_update_issue
+    = render 'shared/issuable/close_reopen_button', issuable: @issue, can_update: can_update_issue, can_reopen: can_reopen_issue
 
     - if can_report_spam
       = link_to 'Submit as spam', mark_as_spam_project_issue_path(@project, @issue), method: :post, class: 'd-none d-sm-none d-md-block btn btn-grouped btn-spam', title: 'Submit as spam'
diff --git a/app/views/projects/merge_requests/_mr_title.html.haml b/app/views/projects/merge_requests/_mr_title.html.haml
index a58179091ae..1bf42ded97a 100644
--- a/app/views/projects/merge_requests/_mr_title.html.haml
+++ b/app/views/projects/merge_requests/_mr_title.html.haml
@@ -39,4 +39,4 @@
       - 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"
 
-      = render 'shared/issuable/close_reopen_button', issuable: @merge_request, can_update: can_update_merge_request
+      = render 'shared/issuable/close_reopen_button', issuable: @merge_request, can_update: can_update_merge_request, can_reopen: can_update_merge_request
diff --git a/app/views/shared/issuable/_close_reopen_button.html.haml b/app/views/shared/issuable/_close_reopen_button.html.haml
index 933d4b2ea65..70e05eb1c8c 100644
--- a/app/views/shared/issuable/_close_reopen_button.html.haml
+++ b/app/views/shared/issuable/_close_reopen_button.html.haml
@@ -2,13 +2,15 @@
 - display_issuable_type = issuable_display_type(issuable)
 - button_method = issuable_close_reopen_button_method(issuable)
 
-- if can_update && is_current_user
-  = link_to "Close #{display_issuable_type}", close_issuable_path(issuable), method: button_method,
-            class: "d-none d-sm-none d-md-block btn btn-grouped btn-close js-btn-issue-action #{issuable_button_visibility(issuable, true)}", title: "Close #{display_issuable_type}"
-  = link_to "Reopen #{display_issuable_type}", reopen_issuable_path(issuable), method: button_method,
-            class: "d-none d-sm-none d-md-block btn btn-grouped btn-reopen js-btn-issue-action #{issuable_button_visibility(issuable, false)}", title: "Reopen #{display_issuable_type}"
-- elsif can_update && !is_current_user
-  = render 'shared/issuable/close_reopen_report_toggle', issuable: issuable
+- if can_update
+  - if is_current_user
+    = link_to "Close #{display_issuable_type}", close_issuable_path(issuable), method: button_method,
+              class: "d-none d-sm-none d-md-block btn btn-grouped btn-close js-btn-issue-action #{issuable_button_visibility(issuable, true)}", title: "Close #{display_issuable_type}"
+  - else
+    = render 'shared/issuable/close_reopen_report_toggle', issuable: issuable
+  - if can_reopen && is_current_user
+    = link_to "Reopen #{display_issuable_type}", reopen_issuable_path(issuable), method: button_method,
+              class: "d-none d-sm-none d-md-block btn btn-grouped btn-reopen js-btn-issue-action #{issuable_button_visibility(issuable, false)}", title: "Reopen #{display_issuable_type}"
 - else
   = link_to 'Report abuse', new_abuse_report_path(user_id: issuable.author.id, ref_url: issuable_url(issuable)),
             class: 'd-none d-sm-none d-md-block btn btn-grouped btn-close-color', title: 'Report abuse'
diff --git a/changelogs/unreleased/39665-restrict-issue-reopen.yml b/changelogs/unreleased/39665-restrict-issue-reopen.yml
new file mode 100644
index 00000000000..204baafb700
--- /dev/null
+++ b/changelogs/unreleased/39665-restrict-issue-reopen.yml
@@ -0,0 +1,5 @@
+---
+title: Restrict reopening locked issues for non authorized issue authors
+merge_request: 21299
+author:
+type: changed
diff --git a/spec/policies/issue_policy_spec.rb b/spec/policies/issue_policy_spec.rb
index 793b724bfca..93e85b3a6fa 100644
--- a/spec/policies/issue_policy_spec.rb
+++ b/spec/policies/issue_policy_spec.rb
@@ -112,6 +112,7 @@ describe IssuePolicy do
     let(:project) { create(:project, :public) }
     let(:issue) { create(:issue, project: project, assignees: [assignee], author: author) }
     let(:issue_no_assignee) { create(:issue, project: project) }
+    let(:issue_locked) { create(:issue, project: project, discussion_locked: true, author: author, assignees: [assignee]) }
 
     before do
       project.add_guest(guest)
@@ -124,36 +125,49 @@ describe IssuePolicy do
 
     it 'allows guests to read issues' do
       expect(permissions(guest, issue)).to be_allowed(:read_issue, :read_issue_iid)
-      expect(permissions(guest, issue)).to be_disallowed(:update_issue, :admin_issue)
+      expect(permissions(guest, issue)).to be_disallowed(:update_issue, :admin_issue, :reopen_issue)
 
       expect(permissions(guest, issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid)
-      expect(permissions(guest, issue_no_assignee)).to be_disallowed(:update_issue, :admin_issue)
+      expect(permissions(guest, issue_no_assignee)).to be_disallowed(:update_issue, :admin_issue, :reopen_issue)
+
+      expect(permissions(guest, issue_locked)).to be_allowed(:read_issue, :read_issue_iid)
+      expect(permissions(guest, issue_locked)).to be_disallowed(:update_issue, :admin_issue, :reopen_issue)
     end
 
-    it 'allows reporters to read, update, and admin issues' do
-      expect(permissions(reporter, issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue)
-      expect(permissions(reporter, issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue)
+    it 'allows reporters to read, update, reopen, and admin issues' do
+      expect(permissions(reporter, issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :reopen_issue)
+      expect(permissions(reporter, issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :reopen_issue)
+      expect(permissions(reporter, issue_locked)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue)
+      expect(permissions(reporter, issue_locked)).to be_disallowed(:reopen_issue)
     end
 
-    it 'allows reporters from group links to read, update, and admin issues' do
-      expect(permissions(reporter_from_group_link, issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue)
-      expect(permissions(reporter_from_group_link, issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue)
+    it 'allows reporters from group links to read, update, reopen and admin issues' do
+      expect(permissions(reporter_from_group_link, issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :reopen_issue)
+      expect(permissions(reporter_from_group_link, issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :reopen_issue)
+      expect(permissions(reporter_from_group_link, issue_locked)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue)
+      expect(permissions(reporter_from_group_link, issue_locked)).to be_disallowed(:reopen_issue)
     end
 
-    it 'allows issue authors to read and update their issues' do
-      expect(permissions(author, issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue)
+    it 'allows issue authors to read, reopen and update their issues' do
+      expect(permissions(author, issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :reopen_issue)
       expect(permissions(author, issue)).to be_disallowed(:admin_issue)
 
       expect(permissions(author, issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid)
-      expect(permissions(author, issue_no_assignee)).to be_disallowed(:update_issue, :admin_issue)
+      expect(permissions(author, issue_no_assignee)).to be_disallowed(:update_issue, :admin_issue, :reopen_issue)
+
+      expect(permissions(author, issue_locked)).to be_allowed(:read_issue, :read_issue_iid, :update_issue)
+      expect(permissions(author, issue_locked)).to be_disallowed(:admin_issue, :reopen_issue)
     end
 
-    it 'allows issue assignees to read and update their issues' do
-      expect(permissions(assignee, issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue)
+    it 'allows issue assignees to read, reopen and update their issues' do
+      expect(permissions(assignee, issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :reopen_issue)
       expect(permissions(assignee, issue)).to be_disallowed(:admin_issue)
 
       expect(permissions(assignee, issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid)
-      expect(permissions(assignee, issue_no_assignee)).to be_disallowed(:update_issue, :admin_issue)
+      expect(permissions(assignee, issue_no_assignee)).to be_disallowed(:update_issue, :admin_issue, :reopen_issue)
+
+      expect(permissions(assignee, issue_locked)).to be_allowed(:read_issue, :read_issue_iid, :update_issue)
+      expect(permissions(assignee, issue_locked)).to be_disallowed(:admin_issue, :reopen_issue)
     end
 
     context 'with confidential issues' do