diff options
author | Alex Kalderimis <alex.kalderimis@gmail.com> | 2019-06-05 06:30:54 +0300 |
---|---|---|
committer | Alex Kalderimis <alex.kalderimis@gmail.com> | 2019-08-07 05:04:33 +0300 |
commit | d30a90a354f3dc015093d80f9de9dc15b38ff2a0 (patch) | |
tree | 3801475fb39956a6cc73598518b5bc6a7afbb3b1 /spec/policies | |
parent | 1dfbb27f6e8d01023564eededff2a0ba1a04badc (diff) |
Prevent unauthorised comments on merge requests
* Prevent creating notes on inaccessible MRs
This applies the notes rules at the MR scope. Rather than adding extra
rules to the Project level policy, preventing :create_note here is
better since it only prevents creating notes on MRs.
* Prevent creating notes in inaccessible Issues
without this policy, non-team-members are allowed to comment on issues
even when the project has the private-issues policy set. This means that
without this change, users are allowed to comment on issues that they
cannot read.
* Add CHANGELOG entry
Diffstat (limited to 'spec/policies')
-rw-r--r-- | spec/policies/issue_policy_spec.rb | 28 | ||||
-rw-r--r-- | spec/policies/merge_request_policy_spec.rb | 89 |
2 files changed, 117 insertions, 0 deletions
diff --git a/spec/policies/issue_policy_spec.rb b/spec/policies/issue_policy_spec.rb index b149dbcf871..25267d36ab8 100644 --- a/spec/policies/issue_policy_spec.rb +++ b/spec/policies/issue_policy_spec.rb @@ -172,6 +172,34 @@ describe IssuePolicy do expect(permissions(assignee, issue_locked)).to be_disallowed(:admin_issue, :reopen_issue) end + context 'when issues are private' do + before do + project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE) + end + let(:issue) { create(:issue, project: project, author: author) } + let(:visitor) { create(:user) } + let(:admin) { create(:user, :admin) } + + it 'forbids visitors from viewing issues' do + expect(permissions(visitor, issue)).to be_disallowed(:read_issue) + end + it 'forbids visitors from commenting' do + expect(permissions(visitor, issue)).to be_disallowed(:create_note) + end + it 'allows guests to view' do + expect(permissions(guest, issue)).to be_allowed(:read_issue) + end + it 'allows guests to comment' do + expect(permissions(guest, issue)).to be_allowed(:create_note) + end + it 'allows admins to view' do + expect(permissions(admin, issue)).to be_allowed(:read_issue) + end + it 'allows admins to comment' do + expect(permissions(admin, issue)).to be_allowed(:create_note) + end + end + context 'with confidential issues' do let(:confidential_issue) { create(:issue, :confidential, project: project, assignees: [assignee], author: author) } let(:confidential_issue_no_assignee) { create(:issue, :confidential, project: project) } diff --git a/spec/policies/merge_request_policy_spec.rb b/spec/policies/merge_request_policy_spec.rb index 81279225d61..87205f56589 100644 --- a/spec/policies/merge_request_policy_spec.rb +++ b/spec/policies/merge_request_policy_spec.rb @@ -6,6 +6,7 @@ describe MergeRequestPolicy do let(:guest) { create(:user) } let(:author) { create(:user) } let(:developer) { create(:user) } + let(:non_team_member) { create(:user) } let(:project) { create(:project, :public) } def permissions(user, merge_request) @@ -18,6 +19,78 @@ describe MergeRequestPolicy do project.add_developer(developer) end + MR_PERMS = %i[create_merge_request_in + create_merge_request_from + read_merge_request + create_note].freeze + + shared_examples_for 'a denied user' do + let(:perms) { permissions(subject, merge_request) } + + MR_PERMS.each do |thing| + it "cannot #{thing}" do + expect(perms).to be_disallowed(thing) + end + end + end + + shared_examples_for 'a user with access' do + let(:perms) { permissions(subject, merge_request) } + + MR_PERMS.each do |thing| + it "can #{thing}" do + expect(perms).to be_allowed(thing) + end + end + end + + context 'when merge requests have been disabled' do + let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) } + + before do + project.project_feature.update(merge_requests_access_level: ProjectFeature::DISABLED) + end + + describe 'the author' do + subject { author } + it_behaves_like 'a denied user' + end + + describe 'a guest' do + subject { guest } + it_behaves_like 'a denied user' + end + + describe 'a developer' do + subject { developer } + it_behaves_like 'a denied user' + end + + describe 'any other user' do + subject { non_team_member } + it_behaves_like 'a denied user' + end + end + + context 'when merge requests are private' do + let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: author) } + + before do + project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.project_feature.update(merge_requests_access_level: ProjectFeature::PRIVATE) + end + + describe 'a non-team-member' do + subject { non_team_member } + it_behaves_like 'a denied user' + end + + describe 'a developer' do + subject { developer } + it_behaves_like 'a user with access' + end + end + context 'when merge request is unlocked' do let(:merge_request) { create(:merge_request, :closed, source_project: project, target_project: project, author: author) } @@ -48,6 +121,22 @@ describe MergeRequestPolicy do it 'prevents guests from reopening merge request' do expect(permissions(guest, merge_request_locked)).to be_disallowed(:reopen_merge_request) end + + context 'when the user is not a project member' do + let(:user) { create(:user) } + + it 'cannot create a note' do + expect(permissions(user, merge_request_locked)).to be_disallowed(:create_note) + end + end + + context 'when the user is project member, with at least guest access' do + let(:user) { guest } + + it 'can create a note' do + expect(permissions(user, merge_request_locked)).to be_allowed(:create_note) + end + end end context 'with external authorization enabled' do |