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

Replace MR access checks with use of MergeRequestsFinder

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

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

- [x]   app/finders/notes_finder.rb:17
- [x]   app/views/layouts/nav/_project.html.haml:80 [`.count`]
- [x]   app/controllers/concerns/creates_commit.rb:84
- [x]   app/controllers/projects/commits_controller.rb:24
- [x]   app/controllers/projects/compare_controller.rb:56
- [x]   app/controllers/projects/discussions_controller.rb:29
- [x]   app/controllers/projects/todos_controller.rb:27
- [x]   app/models/commit.rb:268
- [x]  lib/gitlab/search_results.rb:71

- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#d1c10892daedb4d4dd3d4b12b6d071091eea83df_267_266 Memoize ` merged_merge_request(current_user)`
- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#d1c10892daedb4d4dd3d4b12b6d071091eea83df_248_247 Expected side effect for `merged_merge_request!`, consider `skip_authorization: true`.
- [x] https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2024/diffs#d1c10892daedb4d4dd3d4b12b6d071091eea83df_269_269 Scary use  of unchecked `merged_merge_request?`

See merge request !2033

app/controllers/concerns/creates_commit.rb

@@ -81,10 +81,8 @@ module CreatesCommit
   def merge_request_exists?
     return @merge_request if defined?(@merge_request)
-    @merge_request = @mr_target_project.merge_requests.opened.find_by(
-      source_branch: @mr_source_branch,
-      target_branch: @mr_target_branch
-    )
+    @merge_request = MergeRequestsFinder.new(current_user, project_id: @mr_target_project.id).execute.opened.
+      find_by(source_branch: @mr_source_branch, target_branch: @mr_target_branch)
   end
   def different_project?

app/controllers/projects/commit_controller.rb

@@ -65,7 +65,7 @@ class Projects::CommitController < Projects::ApplicationController
     return render_404 if @target_branch.blank?
-    create_commit(Commits::RevertService, success_notice: "The #{@commit.change_type_title} has been successfully reverted.",
+    create_commit(Commits::RevertService, success_notice: "The #{@commit.change_type_title(current_user)} has been successfully reverted.",
                                           success_path: successful_change_path, failure_path: failed_change_path)
   end
@@ -74,26 +74,24 @@ class Projects::CommitController < Projects::ApplicationController
     return render_404 if @target_branch.blank?
-    create_commit(Commits::CherryPickService, success_notice: "The #{@commit.change_type_title} has been successfully cherry-picked.",
+    create_commit(Commits::CherryPickService, success_notice: "The #{@commit.change_type_title(current_user)} has been successfully cherry-picked.",
                                               success_path: successful_change_path, failure_path: failed_change_path)
   end
   private
   def successful_change_path
-    return referenced_merge_request_url if @commit.merged_merge_request
-
-    namespace_project_commits_url(@project.namespace, @project, @target_branch)
+    referenced_merge_request_url || namespace_project_commits_url(@project.namespace, @project, @target_branch)
   end
   def failed_change_path
-    return referenced_merge_request_url if @commit.merged_merge_request
-
-    namespace_project_commit_url(@project.namespace, @project, params[:id])
+    referenced_merge_request_url || namespace_project_commit_url(@project.namespace, @project, params[:id])
   end
   def referenced_merge_request_url
-    namespace_project_merge_request_url(@project.namespace, @project, @commit.merged_merge_request)
+    if merge_request = @commit.merged_merge_request(current_user)
+      namespace_project_merge_request_url(@project.namespace, @project, merge_request)
+    end
   end
   def commit

app/controllers/projects/commits_controller.rb

@@ -21,7 +21,7 @@ class Projects::CommitsController < Projects::ApplicationController
     @note_counts = project.notes.where(commit_id: @commits.map(&:id)).
       group(:commit_id).count
-    @merge_request = @project.merge_requests.opened.
+    @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened.
       find_by(source_project: @project, source_branch: @ref, target_branch: @repository.root_ref)
     respond_to do |format|

app/controllers/projects/compare_controller.rb

@@ -53,7 +53,7 @@ class Projects::CompareController < Projects::ApplicationController
   end
   def merge_request
-    @merge_request ||= @project.merge_requests.opened.
+    @merge_request ||= MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened.
       find_by(source_project: @project, source_branch: @head_ref, target_branch: @start_ref)
   end
 end

app/controllers/projects/discussions_controller.rb

@@ -24,7 +24,7 @@ class Projects::DiscussionsController < Projects::ApplicationController
   private
   def merge_request
-    @merge_request ||= @project.merge_requests.find_by!(iid: params[:merge_request_id])
+    @merge_request ||= MergeRequestsFinder.new(current_user, project_id: @project.id).find_by!(iid: params[:merge_request_id])
   end
   def discussion

app/controllers/projects/todos_controller.rb

@@ -18,7 +18,7 @@ class Projects::TodosController < Projects::ApplicationController
       when "issue"
         IssuesFinder.new(current_user, project_id: @project.id).find(params[:issuable_id])
       when "merge_request"
-        @project.merge_requests.find(params[:issuable_id])
+        MergeRequestsFinder.new(current_user, project_id: @project.id).find(params[:issuable_id])
       end
     end
   end

app/finders/issuable_finder.rb

@@ -77,6 +77,10 @@ class IssuableFinder
     counts
   end
+  def find_by!(*params)
+    execute.find_by!(*params)
+  end
+
   def group
     return @group if defined?(@group)

app/finders/notes_finder.rb

@@ -14,7 +14,7 @@ class NotesFinder
       when "issue"
         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
+        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
       else

app/helpers/commits_helper.rb

@@ -130,7 +130,7 @@ module CommitsHelper
   def revert_commit_link(commit, continue_to_path, btn_class: nil, has_tooltip: true)
     return unless current_user
-    tooltip = "Revert this #{commit.change_type_title} in a new merge request" if has_tooltip
+    tooltip = "Revert this #{commit.change_type_title(current_user)} in a new merge request" if has_tooltip
     if can_collaborate_with_project?
       btn_class = "btn btn-warning btn-#{btn_class}" unless btn_class.nil?
@@ -154,7 +154,7 @@ module CommitsHelper
   def cherry_pick_commit_link(commit, continue_to_path, btn_class: nil, has_tooltip: true)
     return unless current_user
-    tooltip = "Cherry-pick this #{commit.change_type_title} in a new merge request"
+    tooltip = "Cherry-pick this #{commit.change_type_title(current_user)} in a new merge request"
     if can_collaborate_with_project?
       btn_class = "btn btn-default btn-#{btn_class}" unless btn_class.nil?

app/models/commit.rb

@@ -245,44 +245,47 @@ class Commit
     project.repository.next_branch("cherry-pick-#{short_id}", mild: true)
   end
-  def revert_description
-    if merged_merge_request
-      "This reverts merge request #{merged_merge_request.to_reference}"
+  def revert_description(user)
+    if merged_merge_request?(user)
+      "This reverts merge request #{merged_merge_request(user).to_reference}"
     else
       "This reverts commit #{sha}"
     end
   end
-  def revert_message
-    %Q{Revert "#{title.strip}"\n\n#{revert_description}}
+  def revert_message(user)
+    %Q{Revert "#{title.strip}"\n\n#{revert_description(user)}}
   end
-  def reverts_commit?(commit)
-    description? && description.include?(commit.revert_description)
+  def reverts_commit?(commit, user)
+    description? && description.include?(commit.revert_description(user))
   end
   def merge_commit?
     parents.size > 1
   end
-  def merged_merge_request
-    return @merged_merge_request if defined?(@merged_merge_request)
-
-    @merged_merge_request = project.merge_requests.find_by(merge_commit_sha: id) if merge_commit?
+  def merged_merge_request(current_user)
+    # Memoize with per-user access check
+    @merged_merge_request_hash ||= Hash.new do |hash, user|
+      hash[user] = merged_merge_request_no_cache(user)
+    end
+    
+    @merged_merge_request_hash[current_user]
   end
-  def has_been_reverted?(current_user = nil, noteable = self)
+  def has_been_reverted?(current_user, noteable = self)
     ext = all_references(current_user)
     noteable.notes_with_associations.system.each do |note|
       note.all_references(current_user, extractor: ext)
     end
-    ext.commits.any? { |commit_ref| commit_ref.reverts_commit?(self) }
+    ext.commits.any? { |commit_ref| commit_ref.reverts_commit?(self, current_user) }
   end
-  def change_type_title
-    merged_merge_request ? 'merge request' : 'commit'
+  def change_type_title(user)
+    merged_merge_request?(user) ? 'merge request' : 'commit'
   end
   # Get the URI type of the given path
@@ -350,4 +353,12 @@ class Commit
     changes
   end
+
+  def merged_merge_request?(user)
+    !!merged_merge_request(user)
+  end
+
+  def merged_merge_request_no_cache(user)
+    MergeRequestsFinder.new(user, project_id: project.id).find_by(merge_commit_sha: id) if merge_commit?
+  end
 end

app/models/concerns/milestoneish.rb

 module Milestoneish
-  def closed_items_count(user = nil)
+  def closed_items_count(user)
     issues_visible_to_user(user).closed.size + merge_requests.closed_and_merged.size
   end
-  def total_items_count(user = nil)
+  def total_items_count(user)
     issues_visible_to_user(user).size + merge_requests.size
   end
-  def complete?(user = nil)
+  def complete?(user)
     total_items_count(user) > 0 && total_items_count(user) == closed_items_count(user)
   end
-  def percent_complete(user = nil)
+  def percent_complete(user)
     ((closed_items_count(user) * 100) / total_items_count(user)).abs
   rescue ZeroDivisionError
     0
@@ -29,7 +29,7 @@ module Milestoneish
     (Date.today - start_date).to_i
   end
-  def issues_visible_to_user(user = nil)
+  def issues_visible_to_user(user)
     issues.visible_to_user(user)
   end

app/models/merge_request.rb

@@ -805,7 +805,7 @@ class MergeRequest < ActiveRecord::Base
     @merge_commit ||= project.commit(merge_commit_sha) if merge_commit_sha
   end
-  def can_be_reverted?(current_user = nil)
+  def can_be_reverted?(current_user)
     merge_commit && !merge_commit.has_been_reverted?(current_user, self)
   end

app/models/repository.rb

@@ -950,7 +950,7 @@ class Repository
     update_branch_with_hooks(user, base_branch) do
       committer = user_to_committer(user)
       source_sha = Rugged::Commit.create(rugged,
-        message: commit.revert_message,
+        message: commit.revert_message(user),
         author: committer,
         committer: committer,
         tree: revert_tree_id,

app/services/commits/change_service.rb

@@ -34,7 +34,7 @@ module Commits
         repository.public_send(action, current_user, @commit, into, tree_id)
         success
       else
-        error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title} automatically.
+        error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title(current_user)} automatically.
                      It may have already been #{action.to_s.dasherize}, or a more recent commit may have updated some of its content."
         raise ChangeError, error_msg
       end

app/views/layouts/nav/_project.html.haml

@@ -77,7 +77,7 @@
         = link_to namespace_project_merge_requests_path(@project.namespace, @project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do
           %span
             Merge Requests
-            %span.badge.count.merge_counter= number_with_delimiter(@project.merge_requests.opened.count)
+            %span.badge.count.merge_counter= number_with_delimiter(MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened.count)
     - if project_nav_tab? :wiki
       = nav_link(controller: :wikis) do

app/views/projects/commit/_change.html.haml

@@ -11,7 +11,7 @@
     .modal-content
       .modal-header
         %a.close{href: "#", "data-dismiss" => "modal"} ×
-        %h3.page-title== #{label} this #{commit.change_type_title}
+        %h3.page-title== #{label} this #{commit.change_type_title(current_user)}
       .modal-body
         = form_tag send("#{type.underscore}_namespace_project_commit_path", @project.namespace, @project, commit.id), method: :post, remote: false, class: 'form-horizontal js-#{type}-form js-requires-input' do
           .form-group.branch

changelogs/unreleased/jej-23867-use-mr-finder-instead-of-access-check.yml

+---
+title: Replace MR access checks with use of MergeRequestsFinder
+merge_request: 
+author: 

lib/gitlab/search_results.rb

@@ -68,7 +68,7 @@ module Gitlab
     end
     def merge_requests
-      merge_requests = MergeRequest.in_projects(project_ids_relation)
+      merge_requests = MergeRequestsFinder.new(current_user).execute.in_projects(project_ids_relation)
       if query =~ /[#!](\d+)\z/
         merge_requests = merge_requests.where(iid: $1)
       else

spec/controllers/projects/todo_controller_spec.rb

@@ -110,7 +110,7 @@ describe Projects::TodosController do
         end
       end
-      context 'when not authorized' do
+      context 'when not authorized for project' do
         it 'does not create todo for merge request user has no access to' do
           sign_in(user)
           expect do
@@ -128,6 +128,19 @@ describe Projects::TodosController do
           expect(response).to have_http_status(302)
         end
       end
+
+      context 'when not authorized for merge_request' do
+        before do
+          project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+          project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+          sign_in(user)
+        end
+
+        it "doesn't create todo" do
+          expect{ go }.not_to change { user.todos.count }
+          expect(response).to have_http_status(404)
+        end
+      end
     end
   end
 end

spec/lib/gitlab/search_results_spec.rb

@@ -40,6 +40,15 @@ describe Gitlab::SearchResults do
         expect(results.milestones_count).to eq(1)
       end
     end
+
+    it 'includes merge requests from source and target projects' do
+      forked_project = create(:empty_project, forked_from_project: project)
+      merge_request_2 = create(:merge_request, target_project: project, source_project: forked_project, title: 'foo')
+
+      results = described_class.new(user, Project.where(id: forked_project.id), 'foo')
+
+      expect(results.objects('merge_requests')).to include merge_request_2
+    end
   end
   it 'does not list issues on private projects' do
@@ -152,4 +161,11 @@ describe Gitlab::SearchResults do
       expect(results.issues_count).to eq 5
     end
   end
+
+  it 'does not list merge requests on projects with limited access' do
+    project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+    project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+
+    expect(results.objects('merge_requests')).not_to include merge_request
+  end
 end