Merge branch 'security-2779-fix-email-comment-permissions-check' into 'master'

[master] Fix discussion replies permissions check

Closes #2779

See merge request gitlab/gitlabhq!2794

app/policies/issue_policy.rb

@@ -18,6 +18,7 @@ class IssuePolicy < IssuablePolicy
     prevent :read_issue_iid
     prevent :update_issue
     prevent :admin_issue
+    prevent :create_note
   end
 
   rule { locked }.policy do

app/policies/personal_snippet_policy.rb

@@ -28,5 +28,8 @@ class PersonalSnippetPolicy < BasePolicy
 
   rule { anonymous }.prevent :comment_personal_snippet
 
-  rule { can?(:comment_personal_snippet) }.enable :award_emoji
+  rule { can?(:comment_personal_snippet) }.policy do
+    enable :create_note
+    enable :award_emoji
+  end
 end

app/policies/project_snippet_policy.rb

@@ -43,4 +43,6 @@ class ProjectSnippetPolicy < BasePolicy
     enable :update_project_snippet
     enable :admin_project_snippet
   end
+
+  rule { ~can?(:read_project_snippet) }.prevent :create_note
 end

app/services/notes/build_service.rb

@@ -9,7 +9,7 @@ module Notes
       if in_reply_to_discussion_id.present?
         discussion = find_discussion(in_reply_to_discussion_id)
 
-        unless discussion
+        unless discussion && can?(current_user, :create_note, discussion.noteable)
           note = Note.new
           note.errors.add(:base, 'Discussion to reply to cannot be found')
           return note
@@ -34,19 +34,8 @@ module Notes
       if project
         project.notes.find_discussion(discussion_id)
       else
-        discussion = Note.find_discussion(discussion_id)
-        noteable = discussion.noteable
-
-        return nil unless noteable_without_project?(noteable)
-
-        discussion
+        Note.find_discussion(discussion_id)
       end
     end
-
-    def noteable_without_project?(noteable)
-      return true if noteable.is_a?(PersonalSnippet) && can?(current_user, :comment_personal_snippet, noteable)
-
-      false
-    end
   end
 end

changelogs/unreleased/security-2779-fix-email-comment-permissions-check.yml

+---
+title: Prevent unauthorized replies when discussion is locked or confidential
+merge_request:
+author:
+type: security

lib/gitlab/email/handler/reply_processing.rb

@@ -56,7 +56,7 @@ module Gitlab
             raise ProjectNotFound unless author.can?(:read_project, project)
           end
 
-          raise UserNotAuthorizedError unless author.can?(permission, project || noteable)
+          raise UserNotAuthorizedError unless author.can?(permission, try(:noteable) || project)
         end
 
         def verify_record!(record:, invalid_exception:, record_name:)

spec/lib/gitlab/email/handler/create_note_handler_spec.rb

@@ -118,6 +118,43 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
     end
   end
 
+  shared_examples "checks permissions on noteable" do
+    context "when user has access" do
+      before do
+        project.add_reporter(user)
+      end
+
+      it "creates a comment" do
+        expect { receiver.execute }.to change { noteable.notes.count }.by(1)
+      end
+    end
+
+    context "when user does not have access" do
+      it "raises UserNotAuthorizedError" do
+        expect { receiver.execute }.to raise_error(Gitlab::Email::UserNotAuthorizedError)
+      end
+    end
+  end
+
+  context "when discussion is locked" do
+    before do
+      noteable.update_attribute(:discussion_locked, true)
+    end
+
+    it_behaves_like "checks permissions on noteable"
+  end
+
+  context "when issue is confidential" do
+    let(:issue) { create(:issue, project: project) }
+    let(:note) { create(:note, noteable: issue, project: project) }
+
+    before do
+      issue.update_attribute(:confidential, true)
+    end
+
+    it_behaves_like "checks permissions on noteable"
+  end
+
   context "when everything is fine" do
     before do
       setup_attachment

spec/models/sent_notification_spec.rb

@@ -48,7 +48,7 @@ describe SentNotification do
     let(:note) { create(:diff_note_on_merge_request) }
 
     it 'creates a new SentNotification' do
-      expect { described_class.record_note(note, user.id) }.to change { described_class.count }.by(1)
+      expect { described_class.record_note(note, note.author.id) }.to change { described_class.count }.by(1)
     end
   end
 

spec/policies/personal_snippet_policy_spec.rb

@@ -14,6 +14,13 @@ describe PersonalSnippetPolicy do
     ]
   end
 
+  let(:comment_permissions) do
+    [
+      :comment_personal_snippet,
+      :create_note
+    ]
+  end
+
   def permissions(user)
     described_class.new(user, snippet)
   end
@@ -26,7 +33,7 @@ describe PersonalSnippetPolicy do
 
       it do
         is_expected.to be_allowed(:read_personal_snippet)
-        is_expected.to be_disallowed(:comment_personal_snippet)
+        is_expected.to be_disallowed(*comment_permissions)
         is_expected.to be_disallowed(:award_emoji)
         is_expected.to be_disallowed(*author_permissions)
       end
@@ -37,7 +44,7 @@ describe PersonalSnippetPolicy do
 
       it do
         is_expected.to be_allowed(:read_personal_snippet)
-        is_expected.to be_allowed(:comment_personal_snippet)
+        is_expected.to be_allowed(*comment_permissions)
         is_expected.to be_allowed(:award_emoji)
         is_expected.to be_disallowed(*author_permissions)
       end
@@ -48,7 +55,7 @@ describe PersonalSnippetPolicy do
 
       it do
         is_expected.to be_allowed(:read_personal_snippet)
-        is_expected.to be_allowed(:comment_personal_snippet)
+        is_expected.to be_allowed(*comment_permissions)
         is_expected.to be_allowed(:award_emoji)
         is_expected.to be_allowed(*author_permissions)
       end
@@ -63,7 +70,7 @@ describe PersonalSnippetPolicy do
 
       it do
         is_expected.to be_disallowed(:read_personal_snippet)
-        is_expected.to be_disallowed(:comment_personal_snippet)
+        is_expected.to be_disallowed(*comment_permissions)
         is_expected.to be_disallowed(:award_emoji)
         is_expected.to be_disallowed(*author_permissions)
       end
@@ -74,7 +81,7 @@ describe PersonalSnippetPolicy do
 
       it do
         is_expected.to be_allowed(:read_personal_snippet)
-        is_expected.to be_allowed(:comment_personal_snippet)
+        is_expected.to be_allowed(*comment_permissions)
         is_expected.to be_allowed(:award_emoji)
         is_expected.to be_disallowed(*author_permissions)
       end
@@ -85,7 +92,7 @@ describe PersonalSnippetPolicy do
 
       it do
         is_expected.to be_disallowed(:read_personal_snippet)
-        is_expected.to be_disallowed(:comment_personal_snippet)
+        is_expected.to be_disallowed(*comment_permissions)
         is_expected.to be_disallowed(:award_emoji)
         is_expected.to be_disallowed(*author_permissions)
       end
@@ -96,7 +103,7 @@ describe PersonalSnippetPolicy do
 
       it do
         is_expected.to be_allowed(:read_personal_snippet)
-        is_expected.to be_allowed(:comment_personal_snippet)
+        is_expected.to be_allowed(*comment_permissions)
         is_expected.to be_allowed(:award_emoji)
         is_expected.to be_allowed(*author_permissions)
       end
@@ -111,7 +118,7 @@ describe PersonalSnippetPolicy do
 
       it do
         is_expected.to be_disallowed(:read_personal_snippet)
-        is_expected.to be_disallowed(:comment_personal_snippet)
+        is_expected.to be_disallowed(*comment_permissions)
         is_expected.to be_disallowed(:award_emoji)
         is_expected.to be_disallowed(*author_permissions)
       end
@@ -122,7 +129,7 @@ describe PersonalSnippetPolicy do
 
       it do
         is_expected.to be_disallowed(:read_personal_snippet)
-        is_expected.to be_disallowed(:comment_personal_snippet)
+        is_expected.to be_disallowed(*comment_permissions)
         is_expected.to be_disallowed(:award_emoji)
         is_expected.to be_disallowed(*author_permissions)
       end
@@ -133,7 +140,7 @@ describe PersonalSnippetPolicy do
 
       it do
         is_expected.to be_disallowed(:read_personal_snippet)
-        is_expected.to be_disallowed(:comment_personal_snippet)
+        is_expected.to be_disallowed(*comment_permissions)
         is_expected.to be_disallowed(:award_emoji)
         is_expected.to be_disallowed(*author_permissions)
       end
@@ -144,7 +151,7 @@ describe PersonalSnippetPolicy do
 
       it do
         is_expected.to be_allowed(:read_personal_snippet)
-        is_expected.to be_allowed(:comment_personal_snippet)
+        is_expected.to be_allowed(*comment_permissions)
         is_expected.to be_allowed(:award_emoji)
         is_expected.to be_allowed(*author_permissions)
       end

spec/policies/project_snippet_policy_spec.rb

@@ -41,7 +41,7 @@ describe ProjectSnippetPolicy do
       subject { abilities(regular_user, :public) }
 
       it do
-        expect_allowed(:read_project_snippet)
+        expect_allowed(:read_project_snippet, :create_note)
         expect_disallowed(*author_permissions)
       end
     end
@@ -50,7 +50,7 @@ describe ProjectSnippetPolicy do
       subject { abilities(external_user, :public) }
 
       it do
-        expect_allowed(:read_project_snippet)
+        expect_allowed(:read_project_snippet, :create_note)
         expect_disallowed(*author_permissions)
       end
     end
@@ -70,7 +70,7 @@ describe ProjectSnippetPolicy do
       subject { abilities(regular_user, :internal) }
 
       it do
-        expect_allowed(:read_project_snippet)
+        expect_allowed(:read_project_snippet, :create_note)
         expect_disallowed(*author_permissions)
       end
     end
@@ -79,7 +79,7 @@ describe ProjectSnippetPolicy do
       subject { abilities(external_user, :internal) }
 
       it do
-        expect_disallowed(:read_project_snippet)
+        expect_disallowed(:read_project_snippet, :create_note)
         expect_disallowed(*author_permissions)
       end
     end
@@ -92,7 +92,7 @@ describe ProjectSnippetPolicy do
       end
 
       it do
-        expect_allowed(:read_project_snippet)
+        expect_allowed(:read_project_snippet, :create_note)
         expect_disallowed(*author_permissions)
       end
     end
@@ -112,7 +112,7 @@ describe ProjectSnippetPolicy do
       subject { abilities(regular_user, :private) }
 
       it do
-        expect_disallowed(:read_project_snippet)
+        expect_disallowed(:read_project_snippet, :create_note)
         expect_disallowed(*author_permissions)
       end
     end
@@ -123,7 +123,7 @@ describe ProjectSnippetPolicy do
       subject { described_class.new(regular_user, snippet) }
 
       it do
-        expect_allowed(:read_project_snippet)
+        expect_allowed(:read_project_snippet, :create_note)
         expect_allowed(*author_permissions)
       end
     end
@@ -136,7 +136,7 @@ describe ProjectSnippetPolicy do
       end
 
       it do
-        expect_allowed(:read_project_snippet)
+        expect_allowed(:read_project_snippet, :create_note)
         expect_disallowed(*author_permissions)
       end
     end
@@ -149,7 +149,7 @@ describe ProjectSnippetPolicy do
       end
 
       it do
-        expect_allowed(:read_project_snippet)
+        expect_allowed(:read_project_snippet, :create_note)
         expect_disallowed(*author_permissions)
       end
     end
@@ -158,7 +158,7 @@ describe ProjectSnippetPolicy do
       subject { abilities(create(:admin), :private) }
 
       it do
-        expect_allowed(:read_project_snippet)
+        expect_allowed(:read_project_snippet, :create_note)
         expect_allowed(*author_permissions)
       end
     end

spec/services/notes/build_service_spec.rb

@@ -45,6 +45,15 @@ describe Notes::BuildService do
         end
       end
 
+      context 'when user has no access to discussion' do
+        it 'sets an error' do
+          another_user = create(:user)
+          new_note = described_class.new(project, another_user, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute
+
+          expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found')
+        end
+      end
+
       context 'personal snippet note' do
         def reply(note, user = nil)
           user ||= create(:user)

spec/services/notes/create_service_spec.rb

@@ -127,6 +127,10 @@ describe Notes::CreateService do
         create(:diff_note_on_merge_request, noteable: merge_request, project: project_with_repo)
       end
 
+      before do
+        project_with_repo.add_maintainer(user)
+      end
+
       context 'when eligible to have a note diff file' do
         let(:new_opts) do
           opts.merge(in_reply_to_discussion_id: nil,