Merge branch 'jej-note-search-uses-finder' into 'security'

Fix missing Note access checks in by moving Note#search to updated NoteFinder

Split from !2024 to partially solve https://gitlab.com/gitlab-org/gitlab-ce/issues/23867

## Which fixes are in this MR?

:warning: - Potentially untested  
:bomb: - No test coverage  
:traffic_light: - Test coverage of some sort exists (a test failed when error raised)  
:vertical_traffic_light: - Test coverage of return value (a test failed when nil used)  
:white_check_mark: - Permissions check tested

### Note lookup without access check

- [x] :white_check_mark: app/finders/notes_finder.rb:13 :download_code check
- [x] :white_check_mark: app/finders/notes_finder.rb:19 `SnippetsFinder`
- [x] :white_check_mark: app/models/note.rb:121 [`Issue#visible_to_user`]
- [x] :white_check_mark: lib/gitlab/project_search_results.rb:113
  - This is the only use of `app/models/note.rb:121` above, but importantly has no access checks at all. This means it leaks MR comments and snippets when those features are `team-only` in addition to the issue comments which would be fixed by `app/models/note.rb:121`.
  - It is only called from SearchController where `can?(current_user, :download_code, @project)` is checked, so commit comments are not leaked.

### Previous discussions
- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#b915c5267a63628b0bafd23d37792ae73ceae272_13_13 `: download_code` check on commit
- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#b915c5267a63628b0bafd23d37792ae73ceae272_19_19 `SnippetsFinder` should be used
  - `SnippetsFinder` should check if the snippets feature is enabled -> https://gitlab.com/gitlab-org/gitlab-ce/issues/25223

###  Acceptance criteria met?
- [x] Tests added for new code
- [x] TODO comments removed
- [x] Squashed and removed skipped tests
- [x] Changelog entry
- [ ] State Gitlab versions affected and issue severity in description
- [ ] Create technical debt issue for NotesFinder.
  - Either split into `NotesFinder::ForTarget` and `NotesFinder::Search` or consider object per notable type such as `NotesFinder::OnIssue`. For the first option could create `NotesFinder::Base` which is either inherited from or which can be included in the other two.
  - Avoid case statement anti-pattern in this finder with use of `NotesFinder::OnCommit` etc. Consider something on the finder for this? `Model.finder(user, project)`
  - Move `inc_author` to the controller, and implement `related_notes` to replace `non_diff_notes`/`mr_and_commit_notes`

See merge request !2035

6 files changed, 99 insertions, 41 deletions

diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index 15ca080c696..b71509f2c9b 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/app/controllers/projects/notes_controller.rb
@@ -217,6 +217,6 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def find_current_user_notes
-    @notes = NotesFinder.new.execute(project, current_user, params)
+    @notes = NotesFinder.new(project, current_user, params).execute.inc_author
   end
 end
diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb
index 2484339e3a4..4bd8c83081a 100644
--- a/app/finders/notes_finder.rb
+++ b/app/finders/notes_finder.rb
@@ -1,27 +1,102 @@
 class NotesFinder
   FETCH_OVERLAP = 5.seconds
 
-  def execute(project, current_user, params)
-    target_type = params[:target_type]
-    target_id   = params[:target_id]
-    # Default to 0 to remain compatible with old clients
-    last_fetched_at = Time.at(params.fetch(:last_fetched_at, 0).to_i)
-
-    notes =
-      case target_type
-      when "commit"
-        project.notes.for_commit_id(target_id).non_diff_notes
-      when "issue"
-        IssuesFinder.new(current_user, project_id: project.id).find(target_id).notes.inc_author
-      when "merge_request"
-        MergeRequestsFinder.new(current_user, project_id: project.id).find(target_id).mr_and_commit_notes.inc_author
-      when "snippet", "project_snippet"
-        project.snippets.find(target_id).notes
+  # Used to filter Notes
+  # When used with target_type and target_id this returns notes specifically for the controller
+  #
+  # Arguments:
+  #   current_user - which user check authorizations with
+  #   project - which project to look for notes on
+  #   params:
+  #     target_type: string
+  #     target_id: integer
+  #     last_fetched_at: time
+  #     search: string
+  #
+  def initialize(project, current_user, params = {})
+    @project = project
+    @current_user = current_user
+    @params = params
+    init_collection
+  end
+
+  def execute
+    @notes = since_fetch_at(@params[:last_fetched_at]) if @params[:last_fetched_at]
+    @notes
+  end
+
+  private
+
+  def init_collection
+    if @params[:target_id]
+      @notes = on_target(@params[:target_type], @params[:target_id])
+    else
+      @notes = notes_of_any_type
+    end
+  end
+
+  def notes_of_any_type
+    types = %w(commit issue merge_request snippet)
+    note_relations = types.map { |t| notes_for_type(t) }
+    note_relations.map!{ |notes| search(@params[:search], notes) } if @params[:search]
+    UnionFinder.new.find_union(note_relations, Note)
+  end
+
+  def noteables_for_type(noteable_type)
+    case noteable_type
+    when "issue"
+      IssuesFinder.new(@current_user, project_id: @project.id).execute
+    when "merge_request"
+      MergeRequestsFinder.new(@current_user, project_id: @project.id).execute
+    when "snippet", "project_snippet"
+      SnippetsFinder.new.execute(@current_user, filter: :by_project, project: @project)
+    else
+      raise 'invalid target_type'
+    end
+  end
+
+  def notes_for_type(noteable_type)
+    if noteable_type == "commit"
+      if Ability.allowed?(@current_user, :download_code, @project)
+        @project.notes.where(noteable_type: 'Commit')
+      else
+        Note.none
+      end
+    else
+      finder = noteables_for_type(noteable_type)
+      @project.notes.where(noteable_type: finder.base_class.name, noteable_id: finder.reorder(nil))
+    end
+  end
+
+  def on_target(target_type, target_id)
+    if target_type == "commit"
+      notes_for_type('commit').for_commit_id(target_id)
+    else
+      target = noteables_for_type(target_type).find(target_id)
+
+      if target.respond_to?(:related_notes)
+        target.related_notes
       else
-        raise 'invalid target_type'
+        target.notes
       end
+    end
+  end
+
+  # Searches for notes matching the given query.
+  #
+  # This method uses ILIKE on PostgreSQL and LIKE on MySQL.
+  #
+  def search(query, notes_relation = @notes)
+    pattern = "%#{query}%"
+    notes_relation.where(Note.arel_table[:note].matches(pattern))
+  end
+
+  # Notes changed since last fetch
+  # Uses overlapping intervals to avoid worrying about race conditions
+  def since_fetch_at(fetch_time)
+    # Default to 0 to remain compatible with old clients
+    last_fetched_at = Time.at(@params.fetch(:last_fetched_at, 0).to_i)
 
-    # Use overlapping intervals to avoid worrying about race conditions
-    notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh
+    @notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh
   end
 end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index ea3cf1cdaac..b73d7acefea 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -452,7 +452,7 @@ class MergeRequest < ActiveRecord::Base
     should_remove_source_branch? || force_remove_source_branch?
   end
 
-  def mr_and_commit_notes
+  def related_notes
     # Fetch comments only from last 100 commits
     commits_for_notes_limit = 100
     commit_ids = commits.last(commits_for_notes_limit).map(&:id)
@@ -468,7 +468,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def discussions
-    @discussions ||= self.mr_and_commit_notes.
+    @discussions ||= self.related_notes.
       inc_relations_for_view.
       fresh.
       discussions
diff --git a/app/models/note.rb b/app/models/note.rb
index 08bd08743ef..0c1b05dabf2 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -107,23 +107,6 @@ class Note < ActiveRecord::Base
       Discussion.for_diff_notes(active_notes).
         map { |d| [d.line_code, d] }.to_h
     end
-
-    # Searches for notes matching the given query.
-    #
-    # This method uses ILIKE on PostgreSQL and LIKE on MySQL.
-    #
-    # query   - The search query as a String.
-    # as_user - Limit results to those viewable by a specific user
-    #
-    # Returns an ActiveRecord::Relation.
-    def search(query, as_user: nil)
-      table   = arel_table
-      pattern = "%#{query}%"
-
-      Note.joins('LEFT JOIN issues ON issues.id = noteable_id').
-        where(table[:note].matches(pattern)).
-        merge(Issue.visible_to_user(as_user))
-    end
   end
 
   def cross_reference?
diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml
index fa189ae62d8..959c796ecec 100644
--- a/app/views/projects/merge_requests/_merge_request.html.haml
+++ b/app/views/projects/merge_requests/_merge_request.html.haml
@@ -39,7 +39,7 @@
           = icon('thumbs-down')
           = downvotes
 
-      - note_count = merge_request.mr_and_commit_notes.user.count
+      - note_count = merge_request.related_notes.user.count
       %li
         = link_to merge_request_path(merge_request, anchor: 'notes'), class: ('no-comments' if note_count.zero?) do
           = icon('comments')
diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml
index 896f10104fa..5febe806bdd 100644
--- a/app/views/projects/merge_requests/_show.html.haml
+++ b/app/views/projects/merge_requests/_show.html.haml
@@ -53,7 +53,7 @@
             %li.notes-tab
               = link_to namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#notes', action: 'notes', toggle: 'tab' } do
                 Discussion
-                %span.badge= @merge_request.mr_and_commit_notes.user.count
+                %span.badge= @merge_request.related_notes.user.count
             - if @merge_request.source_project
               %li.commits-tab
                 = link_to commits_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#commits', action: 'commits', toggle: 'tab' } do