From eba4b9404f152f6ad8c4be62116cbe5fd0662b0d Mon Sep 17 00:00:00 2001 From: Jan Beckmann Date: Fri, 8 Mar 2019 08:34:20 +0000 Subject: Disallow reopening of locked merge requests Fixes #56864 --- app/policies/issuable_policy.rb | 1 + app/policies/merge_request_policy.rb | 3 ++ app/policies/project_policy.rb | 1 + app/services/merge_requests/reopen_service.rb | 2 +- .../projects/merge_requests/_mr_title.html.haml | 4 +- changelogs/unreleased/56864-reopen-locked-mr.yml | 5 +++ doc/user/discussions/index.md | 2 +- spec/policies/issuable_policy_spec.rb | 6 +-- spec/policies/merge_request_policy_spec.rb | 50 ++++++++++++++++++++++ 9 files changed, 68 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/56864-reopen-locked-mr.yml create mode 100644 spec/policies/merge_request_policy_spec.rb 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 -- cgit v1.2.1