Merge branch 'security-IDOR-note-drafts-publish' into 'master'

[master] Fix IDOR at /drafts/publish/

Closes #356

See merge request gitlab/gitlab-ee!710

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

@@ -8,6 +8,7 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
   before_action :check_draft_notes_available!, except: [:index]
   before_action :authorize_create_draft!, only: [:create]
   before_action :authorize_admin_draft!, only: [:update, :destroy]
+  before_action :authorize_admin_draft!, only: [:publish], if: -> { params[:id].present? }
 
   def index
     drafts = prepare_notes_for_rendering(draft_notes)
@@ -40,7 +41,7 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
   end
 
   def publish
-    DraftNotes::PublishService.new(merge_request, current_user).execute(params[:id])
+    DraftNotes::PublishService.new(merge_request, current_user).execute(draft_note(allow_nil: true))
 
     head :ok
   end
@@ -53,10 +54,13 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
 
   private
 
-  def draft_note
+  def draft_note(allow_nil: false)
     strong_memoize(:draft_note) do
-      draft_notes.try(:find, params[:id])
+      draft_notes.find(params[:id])
     end
+  rescue ActiveRecord::RecordNotFound => ex
+    # draft_note is allowed to be nil in #publish
+    raise ex unless allow_nil
   end
 
   def draft_notes

ee/app/services/draft_notes/publish_service.rb

@@ -2,9 +2,9 @@
 
 module DraftNotes
   class PublishService < DraftNotes::BaseService
-    def execute(draft_id = nil)
-      if draft_id
-        publish_draft_note(draft_id)
+    def execute(draft = nil)
+      if draft
+        publish_draft_note(draft)
       else
         publish_draft_notes
       end
@@ -12,9 +12,7 @@ module DraftNotes
 
     private
 
-    def publish_draft_note(draft_id)
-      draft = DraftNote.find(draft_id)
-
+    def publish_draft_note(draft)
       create_note_from_draft(draft)
       draft.delete
 

ee/changelogs/unreleased/security-IDOR-note-drafts-publish.yml

+---
+title: Fix IDOR at /drafts/publish
+merge_request:
+author:
+type: security

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

@@ -153,6 +153,7 @@ describe Projects::MergeRequests::DraftsController do
     context 'without permissions' do
       before do
         sign_in(user2)
+        project.add_developer(user2)
       end
 
       it 'does not allow editing draft note belonging to someone else' do
@@ -176,6 +177,22 @@ describe Projects::MergeRequests::DraftsController do
   end
 
   describe 'POST #publish' do
+    context 'without permissions' do
+      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)
+
+        expect { post :publish, params.merge(id: draft.id) }.to change { Note.count }.by(0)
+          .and change { DraftNote.count }.by(0)
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+
     it 'publishes draft notes with position' do
       diff_refs = project.commit(RepoHelpers.sample_commit.id).try(:diff_refs)
 

ee/spec/services/draft_notes/publish_service_spec.rb

@@ -6,14 +6,14 @@ describe DraftNotes::PublishService do
   let(:project) { merge_request.target_project }
   let(:user) { merge_request.author }
 
-  def publish(id: nil)
-    DraftNotes::PublishService.new(merge_request, user).execute(id)
+  def publish(draft: nil)
+    DraftNotes::PublishService.new(merge_request, user).execute(draft)
   end
 
   it 'publishes a single draft note' do
     drafts = create_list(:draft_note, 2, merge_request: merge_request, author: user)
 
-    expect { publish(id: drafts.first.id) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1)
+    expect { publish(draft: drafts.first) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1)
     expect(DraftNote.count).to eq(1)
   end
 
@@ -58,7 +58,7 @@ describe DraftNotes::PublishService do
     let(:draft_note) { create(:draft_note, merge_request: merge_request, author: user, resolve_discussion: true, discussion_id: note.discussion.reply_id) }
 
     it 'resolves the discussion' do
-      publish(id: draft_note.id)
+      publish(draft: draft_note)
 
       expect(note.discussion.resolved?).to be true
     end