diff options
author | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-06-27 00:41:05 +0300 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-06-27 00:41:05 +0300 |
commit | 73dae02756b77e66ee66c462ab4b0efaa1ebf6ec (patch) | |
tree | 63e3fcb02cfadc7a999ab711febedcd4633753ed | |
parent | 55375a154eb4eac249c95d76674b29ba7422159b (diff) | |
parent | e0e49056f720a66f4d4f80f6bfb8074d087f12bf (diff) |
Merge branch 'security-notes-in-private-snippets-12-0' into '12-0-stable'
Ability to write a note in a private snippet
See merge request gitlab/gitlabhq!3142
-rw-r--r-- | app/controllers/concerns/notes_actions.rb | 14 | ||||
-rw-r--r-- | app/controllers/snippets/notes_controller.rb | 8 | ||||
-rw-r--r-- | changelogs/unreleased/security-notes-in-private-snippets.yml | 5 | ||||
-rw-r--r-- | spec/controllers/projects/notes_controller_spec.rb | 2 | ||||
-rw-r--r-- | spec/controllers/snippets/notes_controller_spec.rb | 113 |
5 files changed, 132 insertions, 10 deletions
diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index f96d1821095..0098c4cdf4c 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -203,17 +203,17 @@ module NotesActions # These params are also sent by the client but we need to set these based on # target_type and target_id because we're checking permissions based on that - create_params[:noteable_type] = params[:target_type].classify + create_params[:noteable_type] = noteable.class.name - case params[:target_type] - when 'commit' - create_params[:commit_id] = params[:target_id] - when 'merge_request' - create_params[:noteable_id] = params[:target_id] + case noteable + when Commit + create_params[:commit_id] = noteable.id + when MergeRequest + create_params[:noteable_id] = noteable.id # Notes on MergeRequest can have an extra `commit_id` context create_params[:commit_id] = params.dig(:note, :commit_id) else - create_params[:noteable_id] = params[:target_id] + create_params[:noteable_id] = noteable.id end end end diff --git a/app/controllers/snippets/notes_controller.rb b/app/controllers/snippets/notes_controller.rb index eee14b0faf4..612897f27e6 100644 --- a/app/controllers/snippets/notes_controller.rb +++ b/app/controllers/snippets/notes_controller.rb @@ -5,8 +5,8 @@ class Snippets::NotesController < ApplicationController include ToggleAwardEmoji skip_before_action :authenticate_user!, only: [:index] - before_action :snippet - before_action :authorize_read_snippet!, only: [:show, :index, :create] + before_action :authorize_read_snippet!, only: [:show, :index] + before_action :authorize_create_note!, only: [:create] private @@ -33,4 +33,8 @@ class Snippets::NotesController < ApplicationController def authorize_read_snippet! return render_404 unless can?(current_user, :read_personal_snippet, snippet) end + + def authorize_create_note! + access_denied! unless can?(current_user, :create_note, noteable) + end end diff --git a/changelogs/unreleased/security-notes-in-private-snippets.yml b/changelogs/unreleased/security-notes-in-private-snippets.yml new file mode 100644 index 00000000000..907d98cb16d --- /dev/null +++ b/changelogs/unreleased/security-notes-in-private-snippets.yml @@ -0,0 +1,5 @@ +--- +title: Correctly check permissions when creating snippet notes +merge_request: +author: +type: security 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 { |