Correctly check permissions when creating snippet notes

In the Snippets::NotesController the noteable was resolved and
authorized through the :snippet_id, so by passing a :target_id for a
different snippet it was possible to create a note on a snippet
where the user would be unauthorized to do so otherwise.

This fixes the problem by ignoring the :target_id and :target_type from
the request, and using the same noteable for creation and authorization.

2 files changed, 114 insertions, 1 deletions

diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb
index 6ec84f5c528..1db1963476c 100644
--- a/spec/controllers/projects/notes_controller_spec.rb
+++ b/spec/controllers/projects/notes_controller_spec.rb
@@ -252,7 +252,7 @@ describe Projects::NotesController do
       before do
         service_params = ActionController::Parameters.new({
           note: 'some note',
-          noteable_id: merge_request.id.to_s,
+          noteable_id: merge_request.id,
           noteable_type: 'MergeRequest',
           commit_id: nil,
           merge_request_diff_head_sha: 'sha'
diff --git a/spec/controllers/snippets/notes_controller_spec.rb b/spec/controllers/snippets/notes_controller_spec.rb
index 936d7c7dae4..586d59c2d09 100644
--- a/spec/controllers/snippets/notes_controller_spec.rb
+++ b/spec/controllers/snippets/notes_controller_spec.rb
@@ -119,6 +119,119 @@ describe Snippets::NotesController do
     end
   end
 
+  describe 'POST create' do
+    context 'when a snippet is public' do
+      let(:request_params) do
+        {
+          note: attributes_for(:note_on_personal_snippet, noteable: public_snippet),
+          snippet_id: public_snippet.id
+        }
+      end
+
+      before do
+        sign_in user
+      end
+
+      it 'returns status 302' do
+        post :create, params: request_params
+
+        expect(response).to have_gitlab_http_status(302)
+      end
+
+      it 'creates the note' do
+        expect { post :create, params: request_params }.to change { Note.count }.by(1)
+      end
+    end
+
+    context 'when a snippet is internal' do
+      let(:request_params) do
+        {
+          note: attributes_for(:note_on_personal_snippet, noteable: internal_snippet),
+          snippet_id: internal_snippet.id
+        }
+      end
+
+      before do
+        sign_in user
+      end
+
+      it 'returns status 302' do
+        post :create, params: request_params
+
+        expect(response).to have_gitlab_http_status(302)
+      end
+
+      it 'creates the note' do
+        expect { post :create, params: request_params }.to change { Note.count }.by(1)
+      end
+    end
+
+    context 'when a snippet is private' do
+      let(:request_params) do
+        {
+          note: attributes_for(:note_on_personal_snippet, noteable: private_snippet),
+          snippet_id: private_snippet.id
+        }
+      end
+
+      before do
+        sign_in user
+      end
+
+      context 'when user is not the author' do
+        before do
+          sign_in(user)
+        end
+
+        it 'returns status 404' do
+          post :create, params: request_params
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+
+        it 'does not create the note' do
+          expect { post :create, params: request_params }.not_to change { Note.count }
+        end
+
+        context 'when user sends a snippet_id for a public snippet' do
+          let(:request_params) do
+            {
+              note: attributes_for(:note_on_personal_snippet, noteable: private_snippet),
+              snippet_id: public_snippet.id
+            }
+          end
+
+          it 'returns status 302' do
+            post :create, params: request_params
+
+            expect(response).to have_gitlab_http_status(302)
+          end
+
+          it 'creates the note on the public snippet' do
+            expect { post :create, params: request_params }.to change { Note.count }.by(1)
+            expect(Note.last.noteable).to eq public_snippet
+          end
+        end
+      end
+
+      context 'when user is the author' do
+        before do
+          sign_in(private_snippet.author)
+        end
+
+        it 'returns status 302' do
+          post :create, params: request_params
+
+          expect(response).to have_gitlab_http_status(302)
+        end
+
+        it 'creates the note' do
+          expect { post :create, params: request_params }.to change { Note.count }.by(1)
+        end
+      end
+    end
+  end
+
   describe 'DELETE destroy' do
     let(:request_params) do
       {