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

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