Award Emoji cannot be added to Merge Requests

Attempting to add an Award Emoji to a Merge Request causes a 500 error:

PG::UndefinedColumn: ERROR:  column merge_requests.project does not exist
LINE 1: ...e_requests"  WHERE "merge_requests"."id" = 50 AND "merge_req...
                                                             ^
: SELECT  "merge_requests".* FROM "merge_requests"  WHERE "merge_requests"."id" = 50 AND "merge_requests"."project" = 5  ORDER BY "merge_requests"."id" DESC LIMIT 1
Completed 500 Internal Server Error in 51ms (ActiveRecord: 6.4ms)

PG::UndefinedColumn - ERROR:  column merge_requests.project does not exist
LINE 1: ...e_requests"  WHERE "merge_requests"."id" = 50 AND "merge_req...
                                                             ^
:
  activerecord (4.1.12) lib/active_record/connection_adapters/postgresql_adapter.rb:822:in `block in exec_no_cache'
  activerecord (4.1.12) lib/active_record/connection_adapters/abstract_adapter.rb:378:in `block in log'
  activesupport (4.1.12) lib/active_support/notifications/instrumenter.rb:20:in `instrument'
  activerecord (4.1.12) lib/active_record/connection_adapters/abstract_adapter.rb:372:in `log'
  activerecord (4.1.12) lib/active_record/connection_adapters/postgresql_adapter.rb:822:in `exec_no_cache'
  activerecord (4.1.12) lib/active_record/connection_adapters/postgresql/database_statements.rb:137:in `exec_query'
  activerecord (4.1.12) lib/active_record/connection_adapters/postgresql_adapter.rb:954:in `select'
  activerecord (4.1.12) lib/active_record/connection_adapters/abstract/database_statements.rb:24:in `select_all'
  activerecord (4.1.12) lib/active_record/connection_adapters/abstract/query_cache.rb:68:in `block in select_all'
  activerecord (4.1.12) lib/active_record/connection_adapters/abstract/query_cache.rb:83:in `cache_sql'
  activerecord (4.1.12) lib/active_record/connection_adapters/abstract/query_cache.rb:68:in `select_all'
  activerecord (4.1.12) lib/active_record/querying.rb:39:in `find_by_sql'
  activerecord (4.1.12) lib/active_record/relation.rb:611:in `exec_queries'
  activerecord (4.1.12) lib/active_record/relation.rb:493:in `load'
  activerecord (4.1.12) lib/active_record/relation.rb:238:in `to_a'
  activerecord (4.1.12) lib/active_record/relation/finder_methods.rb:460:in `find_take'
  activerecord (4.1.12) lib/active_record/relation/finder_methods.rb:98:in `take'
  activerecord (4.1.12) lib/active_record/relation/finder_methods.rb:104:in `take!'
  activerecord (4.1.12) lib/active_record/relation/finder_methods.rb:87:in `find_by!'
  activerecord (4.1.12) lib/active_record/querying.rb:7:in `find_by!'
  app/controllers/projects/notes_controller.rb:63:in `award_toggle'
  ... rest of stack trace

This is because we're calling find_by! on MergeRequest with a project argument, which of course doesn't exist on MergeRequest.

This must be fixed for the official 8.2 release.

cc @dzaporozhets @vsizov

mentioned in issue #3530 (closed)

Simple hack-ish patch:

diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index 1e3f1d8..b4b8d2a 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/app/controllers/projects/notes_controller.rb
@@ -59,8 +59,13 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def award_toggle
-    noteable = note_params[:noteable_type] == "issue" ? Issue : MergeRequest
-    noteable = noteable.find_by!(id: note_params[:noteable_id], project: project)
+    case note_params[:noteable_type]
+    when 'issue'
+      noteable = Issue.find_by!(id: note_params[:noteable_id], project: project)
+    else
+      noteable = MergeRequest.find_by!(id: note_params[:noteable_id],
+                                       source_project: project)
+    end
 
     data = {
       author: current_user,

Unsure if we should be using source_project or target_project here.

In the bigger picture, I wonder if it would be worth defining an alias_attribute method for project to source_project. cc @DouweM

Is project even needed if issues and merge requests can be identified by the specific ID?

Since notes can also apply to commits, should there be a case that catches that?

Also, what prevents someone from tampering with the award toggle parameters? It looks like find_current_user_notes is called as a before_action, but are the results actually used?

Added ~128341 label

@rspeicher Yes, we can just remove project at all. @stanhu is right

@rspeicher here is a fix https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/1865

@stanhu What kind of tampering do you mean?

@vsizov Right now I can change the award emoji parameters to specify a issue/merge request ID to which I don't have access. NotesFinder finds the accessible notes for the given user, but it doesn't appear to be used for the award emoji.

yeah, it needs to be fixed. But NotesFinder will not help me, I need another solution.

@stanhu This is why project has been used in a condition. You could not remove someone else's emoji (in another project) and you could not add new emoji to another project because note create service is using project in a condition as well

but after last fix that's the case.

@vsizov What about using MergeRequestFinder and IssueFinder?

@stanhu Yes, probably it's a solution.

Well. I think my first implementation was good.. before code review and following fixes :) I did project.notes it guarantees a safety

@stanhu I think this is a solution:

noteable = if note_params[:noteable_type] == "issue"
                 project.issues.find(note_params[:noteable_id])
               else
                 project.merge_requests.find(note_params[:noteable_id])
               end

Status changed to closed by commit 78be7055

mentioned in commit 53c0b621

mentioned in commit 78be7055

mentioned in merge request !14785