Merge branch 'security-pb-locked-mr-drafts-publish' into 'master'

Don't publish drafts if user can't create notes

Closes #3

See merge request gitlab-org/security/gitlab!9

ee/app/controllers/projects/merge_requests/drafts_controller.rb

@@ -6,7 +6,7 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
   respond_to :json
 
   before_action :check_draft_notes_available!, except: [:index]
-  before_action :authorize_create_draft!, only: [:create]
+  before_action :authorize_create_note!, only: [:create, :publish]
   before_action :authorize_admin_draft!, only: [:update, :destroy]
   before_action :authorize_admin_draft!, if: -> { action_name == 'publish' && params[:id].present? }
 
@@ -128,7 +128,7 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
     access_denied! unless can?(current_user, :admin_note, draft_note)
   end
 
-  def authorize_create_draft!
+  def authorize_create_note!
     access_denied! unless can?(current_user, :create_note, merge_request)
   end
 

ee/app/services/draft_notes/publish_service.rb

@@ -3,6 +3,8 @@
 module DraftNotes
   class PublishService < DraftNotes::BaseService
     def execute(draft = nil)
+      return error('Not allowed to create notes') unless can?(current_user, :create_note, merge_request)
+
       if draft
         publish_draft_note(draft)
       else

ee/changelogs/unreleased/security-pb-locked-mr-drafts-publish.yml

+---
+title: Don't publish drafts if user can't create notes
+merge_request:
+author:
+type: security

ee/spec/controllers/projects/merge_requests/drafts_controller_spec.rb

@@ -215,18 +215,42 @@ describe Projects::MergeRequests::DraftsController do
 
   describe 'POST #publish' do
     context 'without permissions' do
+      shared_examples_for 'action that does not allow publishing draft note' do
+        it 'does not allow publishing draft note' do
+          expect { action }
+            .to not_change { Note.count }
+            .and not_change { DraftNote.count }
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+
       before do
         sign_in(user2)
-        project.add_developer(user2)
       end
 
-      it 'does not allow publishing draft note belonging to someone else' do
-        draft = create(:draft_note, merge_request: merge_request, author: user)
+      context 'when note belongs to someone else' do
+        before do
+          project.add_developer(user2)
+        end
 
-        expect { post :publish, params: params.merge(id: draft.id) }.to change { Note.count }.by(0)
-          .and change { DraftNote.count }.by(0)
+        it_behaves_like 'action that does not allow publishing draft note' do
+          let!(:draft) { create(:draft_note, merge_request: merge_request, author: user) }
+          let(:action) { post :publish, params: params.merge(id: draft.id) }
+        end
+      end
 
-        expect(response).to have_gitlab_http_status(404)
+      context 'when merge request discussion is locked' do
+        let(:project) { create(:project, :public, :merge_requests_public, :repository) }
+
+        before do
+          create(:draft_note, merge_request: merge_request, author: user2)
+          merge_request.update!(discussion_locked: true)
+        end
+
+        it_behaves_like 'action that does not allow publishing draft note' do
+          let(:action) { post :publish, params: params }
+        end
       end
     end
 

ee/spec/services/draft_notes/publish_service_spec.rb

@@ -246,4 +246,14 @@ describe DraftNotes::PublishService do
       publish
     end
   end
+
+  context 'user cannot create notes' do
+    before do
+      allow(Ability).to receive(:allowed?).with(user, :create_note, merge_request).and_return(false)
+    end
+
+    it 'returns an error' do
+      expect(publish[:status]).to eq(:error)
+    end
+  end
 end