Merge branch '54924-refactor-notes-actions-params' into 'master'

Refactor params for NotesActions

Closes #54924

See merge request gitlab-org/gitlab-ce!25278

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