Merge branch 'jej-use-issuable-finder-instead-of-access-check' into 'security'

Replace issue access checks with use of IssuableFinder

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

### Issue lookup with access check

Using `visible_to_user` likely makes these security issues too. See [Code smells](#code-smells).

- [x] :vertical_traffic_light: app/finders/notes_finder.rb:15 [`visible_to_user`]
- [x] :traffic_light: app/views/layouts/nav/_project.html.haml:73 [`visible_to_user`] [`.count`]
- [x] :white_check_mark: app/services/merge_requests/build_service.rb:84 [`issue.try(:confidential?)`]
- [x] :white_check_mark: lib/api/issues.rb:112 [`visible_to_user`]
  - CHANGELOG: Prevented API returning issues set to 'Only team members' to everyone
- [x] :white_check_mark: lib/api/helpers.rb:126 [`can?(current_user, :read_issue, issue)`] Maybe here too?
- [x] :white_check_mark: lib/gitlab/search_results.rb:53 [`visible_to_user`]

### Previous discussions
- [ ] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#b2ff264eddf9819d7693c14ae213d941494fe2b3_128_126
- [ ] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#7b6375270d22f880bdcb085e47b519b426a5c6c7_87_87

See merge request !2031

5 files changed, 6 insertions, 6 deletions

diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 36005035f68..9a74e36870b 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -21,7 +21,7 @@ class IssuableFinder
 
   attr_accessor :current_user, :params
 
-  def initialize(current_user, params)
+  def initialize(current_user, params = {})
     @current_user = current_user
     @params = params
   end
diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb
index 0b7832e6583..a653a6d59c6 100644
--- a/app/finders/notes_finder.rb
+++ b/app/finders/notes_finder.rb
@@ -12,7 +12,7 @@ class NotesFinder
       when "commit"
         project.notes.for_commit_id(target_id).non_diff_notes
       when "issue"
-        project.issues.visible_to_user(current_user).find(target_id).notes.inc_author
+        IssuesFinder.new(current_user, project_id: project.id).find(target_id).notes.inc_author
       when "merge_request"
         project.merge_requests.find(target_id).mr_and_commit_notes.inc_author
       when "snippet", "project_snippet"
diff --git a/app/models/project.rb b/app/models/project.rb
index c61e63461e0..f01cb613b85 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -687,9 +687,9 @@ class Project < ActiveRecord::Base
     self.id
   end
 
-  def get_issue(issue_id)
+  def get_issue(issue_id, current_user)
     if default_issues_tracker?
-      issues.find_by(iid: issue_id)
+      IssuesFinder.new(current_user, project_id: id).find_by(iid: issue_id)
     else
       ExternalIssue.new(issue_id, self)
     end
diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb
index dd0d738674e..bebfca7537b 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -81,7 +81,7 @@ module MergeRequests
         commit = commits.first
         merge_request.title = commit.title
         merge_request.description ||= commit.description.try(:strip)
-      elsif iid && (issue = merge_request.target_project.get_issue(iid)) && !issue.try(:confidential?)
+      elsif iid && issue = merge_request.target_project.get_issue(iid, current_user)
         case issue
         when Issue
           merge_request.title = "Resolve \"#{issue.title}\""
diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml
index 99a58bbb676..701bcd3ab71 100644
--- a/app/views/layouts/nav/_project.html.haml
+++ b/app/views/layouts/nav/_project.html.haml
@@ -70,7 +70,7 @@
           %span
             Issues
             - if @project.default_issues_tracker?
-              %span.badge.count.issue_counter= number_with_delimiter(@project.issues.visible_to_user(current_user).opened.count)
+              %span.badge.count.issue_counter= number_with_delimiter(IssuesFinder.new(current_user, project_id: @project.id).execute.opened.count)
 
     - if project_nav_tab? :merge_requests
       = nav_link(controller: :merge_requests) do