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

: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

- [x] :bomb:  app/finders/notes_finder.rb:17
- [x] :warning:  app/views/layouts/nav/_project.html.haml:80 [`.count`]
- [x] :bomb:  app/controllers/concerns/creates_commit.rb:84
- [x] :traffic_light:  app/controllers/projects/commits_controller.rb:24
- [x] :traffic_light:  app/controllers/projects/compare_controller.rb:56
- [x] :vertical_traffic_light:  app/controllers/projects/discussions_controller.rb:29
- [x] :white_check_mark:  app/controllers/projects/todos_controller.rb:27
- [x] :vertical_traffic_light:  app/models/commit.rb:268
- [x] :white_check_mark: 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

1 files changed, 26 insertions, 15 deletions

diff --git a/app/models/commit.rb b/app/models/commit.rb
index 248140f421b..1831cc7e175 100644
--- a/app/models/commit.rb
+++ b/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