diff options
author | Heinrich Lee Yu <heinrich@gitlab.com> | 2019-02-14 20:22:51 +0800 |
---|---|---|
committer | Heinrich Lee Yu <heinrich@gitlab.com> | 2019-02-23 09:38:58 +0800 |
commit | d03dee26b9d15440ba3d52d319b11474176143c4 (patch) | |
tree | 5e4359ed56bdd13da6b00d56ed55930626f1d2d8 | |
parent | 48d31abc97528a36614b545f0b61fa14053ce4a8 (diff) | |
download | gitlab-ce-d03dee26b9d15440ba3d52d319b11474176143c4.tar.gz |
Refactor params for notes_actions
Removes unneeded params from permitted list
This also fixes commenting on commits with a hash starting with a large
number
-rw-r--r-- | app/controllers/concerns/notes_actions.rb | 57 | ||||
-rw-r--r-- | app/controllers/snippets/notes_controller.rb | 4 | ||||
-rw-r--r-- | changelogs/unreleased/54924-refactor-notes-actions-params.yml | 5 | ||||
-rw-r--r-- | spec/controllers/projects/notes_controller_spec.rb | 20 | ||||
-rw-r--r-- | spec/support/helpers/test_env.rb | 3 |
5 files changed, 52 insertions, 37 deletions
diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index 0319948a12f..b4fee93713b 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -6,7 +6,6 @@ module NotesActions extend ActiveSupport::Concern included do - prepend_before_action :normalize_create_params, only: [:create] before_action :set_polling_interval_header, only: [:index] before_action :require_noteable!, only: [:index, :create] before_action :authorize_admin_note!, only: [:update, :destroy] @@ -44,12 +43,7 @@ module NotesActions # rubocop:disable Gitlab/ModuleWithInstanceVariables def create - create_params = note_params.merge( - merge_request_diff_head_sha: params[:merge_request_diff_head_sha], - in_reply_to_discussion_id: params[:in_reply_to_discussion_id] - ) - - @note = Notes::CreateService.new(note_project, current_user, create_params).execute + @note = Notes::CreateService.new(note_project, current_user, create_note_params).execute respond_to do |format| format.json do @@ -78,7 +72,7 @@ module NotesActions # rubocop:disable Gitlab/ModuleWithInstanceVariables def update - @note = Notes::UpdateService.new(project, current_user, note_params).execute(note) + @note = Notes::UpdateService.new(project, current_user, update_note_params).execute(note) prepare_notes_for_rendering([@note]) respond_to do |format| @@ -196,24 +190,36 @@ module NotesActions return access_denied! unless can?(current_user, :admin_note, note) end - def note_params + def create_note_params params.require(:note).permit( - :project_id, - :noteable_type, - :noteable_id, - :commit_id, - :noteable, :type, - :note, - :attachment, + :line_code, # LegacyDiffNote + :position # DiffNote + ).tap do |create_params| + create_params.merge!( + params.permit(:merge_request_diff_head_sha, :in_reply_to_discussion_id) + ) - # LegacyDiffNote - :line_code, + # 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 + + case params[:target_type] + when 'commit' + create_params[:commit_id] = params[:target_id] + when 'merge_request' + create_params[:noteable_id] = params[:target_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] + end + end + end - # DiffNote - :position - ) + def update_note_params + params.require(:note).permit(:note) end def set_polling_interval_header @@ -248,15 +254,6 @@ module NotesActions DiscussionSerializer.new(project: project, noteable: noteable, current_user: current_user, note_entity: ProjectNoteEntity) end - # Avoids checking permissions in the wrong object - this ensures that the object we checked permissions for - # is the object we're actually creating a note in. - def normalize_create_params - params[:note].try do |note| - note[:noteable_id] = params[:target_id] - note[:noteable_type] = params[:target_type].classify - end - end - def note_project strong_memoize(:note_project) do next nil unless project diff --git a/app/controllers/snippets/notes_controller.rb b/app/controllers/snippets/notes_controller.rb index 091bcb1253d..eee14b0faf4 100644 --- a/app/controllers/snippets/notes_controller.rb +++ b/app/controllers/snippets/notes_controller.rb @@ -26,10 +26,6 @@ class Snippets::NotesController < ApplicationController # rubocop: enable CodeReuse/ActiveRecord alias_method :noteable, :snippet - def note_params - super.merge(noteable_id: params[:snippet_id]) - end - def finder_params params.merge(last_fetched_at: last_fetched_at, target_id: snippet.id, target_type: 'personal_snippet') end diff --git a/changelogs/unreleased/54924-refactor-notes-actions-params.yml b/changelogs/unreleased/54924-refactor-notes-actions-params.yml new file mode 100644 index 00000000000..b6083820401 --- /dev/null +++ b/changelogs/unreleased/54924-refactor-notes-actions-params.yml @@ -0,0 +1,5 @@ +--- +title: Fix commenting on commits having SHA1 starting with a large number +merge_request: 25278 +author: +type: fixed diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 81892575889..0b0f5117784 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -252,8 +252,8 @@ describe Projects::NotesController do note: 'some note', noteable_id: merge_request.id.to_s, noteable_type: 'MergeRequest', - merge_request_diff_head_sha: 'sha', - in_reply_to_discussion_id: nil + commit_id: nil, + merge_request_diff_head_sha: 'sha' }).permit! expect(Notes::CreateService).to receive(:new).with(project, user, service_params).and_return(double(execute: true)) @@ -266,6 +266,22 @@ describe Projects::NotesController do end end + context 'when creating a comment on a commit with SHA1 starting with a large number' do + let(:commit) { create(:commit, project: project, id: '842616594688d2351480dfebd67b3d8d15571e6d') } + + it 'creates a note successfully' do + expect do + post :create, params: { + note: { note: 'some note', commit_id: commit.id }, + namespace_id: project.namespace, + project_id: project, + target_type: 'commit', + target_id: commit.id + } + end.to change { Note.count }.by(1) + end + end + context 'when creating a commit comment from an MR fork' do let(:project) { create(:project, :repository) } diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb index f485eb7b0eb..06bcf4f8013 100644 --- a/spec/support/helpers/test_env.rb +++ b/spec/support/helpers/test_env.rb @@ -63,7 +63,8 @@ module TestEnv 'after-create-delete-modify-move' => 'ba3faa7', 'with-codeowners' => '219560e', 'submodule_inside_folder' => 'b491b92', - 'png-lfs' => 'fe42f41' + 'png-lfs' => 'fe42f41', + 'sha-starting-with-large-number' => '8426165' }.freeze # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily |