Commit 27d2ecb8 authored by Douwe Maan's avatar Douwe Maan Committed by Alejandro Rodríguez

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

## Which fixes are in this MR?

 - 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

### MR lookup from project

- [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

### Previous discussions
- [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
parent f397f97f
...@@ -81,10 +81,8 @@ module CreatesCommit ...@@ -81,10 +81,8 @@ module CreatesCommit
def merge_request_exists? def merge_request_exists?
return @merge_request if defined?(@merge_request) return @merge_request if defined?(@merge_request)
@merge_request = @mr_target_project.merge_requests.opened.find_by( @merge_request = MergeRequestsFinder.new(current_user, project_id: @mr_target_project.id).execute.opened.
source_branch: @mr_source_branch, find_by(source_branch: @mr_source_branch, target_branch: @mr_target_branch)
target_branch: @mr_target_branch
)
end end
def different_project? def different_project?
......
...@@ -61,7 +61,7 @@ class Projects::CommitController < Projects::ApplicationController ...@@ -61,7 +61,7 @@ class Projects::CommitController < Projects::ApplicationController
return render_404 if @target_branch.blank? 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) success_path: successful_change_path, failure_path: failed_change_path)
end end
...@@ -70,26 +70,24 @@ class Projects::CommitController < Projects::ApplicationController ...@@ -70,26 +70,24 @@ class Projects::CommitController < Projects::ApplicationController
return render_404 if @target_branch.blank? 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) success_path: successful_change_path, failure_path: failed_change_path)
end end
private private
def successful_change_path def successful_change_path
return referenced_merge_request_url if @commit.merged_merge_request referenced_merge_request_url || namespace_project_commits_url(@project.namespace, @project, @target_branch)
namespace_project_commits_url(@project.namespace, @project, @target_branch)
end end
def failed_change_path def failed_change_path
return referenced_merge_request_url if @commit.merged_merge_request referenced_merge_request_url || namespace_project_commit_url(@project.namespace, @project, params[:id])
namespace_project_commit_url(@project.namespace, @project, params[:id])
end end
def referenced_merge_request_url 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 end
def commit def commit
......
...@@ -21,7 +21,7 @@ class Projects::CommitsController < Projects::ApplicationController ...@@ -21,7 +21,7 @@ class Projects::CommitsController < Projects::ApplicationController
@note_counts = project.notes.where(commit_id: @commits.map(&:id)). @note_counts = project.notes.where(commit_id: @commits.map(&:id)).
group(:commit_id).count 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) find_by(source_project: @project, source_branch: @ref, target_branch: @repository.root_ref)
respond_to do |format| respond_to do |format|
......
...@@ -53,7 +53,7 @@ class Projects::CompareController < Projects::ApplicationController ...@@ -53,7 +53,7 @@ class Projects::CompareController < Projects::ApplicationController
end end
def merge_request 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) find_by(source_project: @project, source_branch: @head_ref, target_branch: @start_ref)
end end
end end
...@@ -26,7 +26,7 @@ class Projects::DiscussionsController < Projects::ApplicationController ...@@ -26,7 +26,7 @@ class Projects::DiscussionsController < Projects::ApplicationController
private private
def merge_request 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 end
def discussion def discussion
......
...@@ -18,7 +18,7 @@ class Projects::TodosController < Projects::ApplicationController ...@@ -18,7 +18,7 @@ class Projects::TodosController < Projects::ApplicationController
when "issue" when "issue"
IssuesFinder.new(current_user, project_id: @project.id).find(params[:issuable_id]) IssuesFinder.new(current_user, project_id: @project.id).find(params[:issuable_id])
when "merge_request" 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 end
end end
......
...@@ -51,6 +51,10 @@ class IssuableFinder ...@@ -51,6 +51,10 @@ class IssuableFinder
execute.find_by(*params) execute.find_by(*params)
end end
def find_by!(*params)
execute.find_by!(*params)
end
def group def group
return @group if defined?(@group) return @group if defined?(@group)
......
...@@ -14,7 +14,7 @@ class NotesFinder ...@@ -14,7 +14,7 @@ class NotesFinder
when "issue" when "issue"
IssuesFinder.new(current_user, project_id: project.id).find(target_id).notes.inc_author IssuesFinder.new(current_user, project_id: project.id).find(target_id).notes.inc_author
when "merge_request" 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" when "snippet", "project_snippet"
project.snippets.find(target_id).notes project.snippets.find(target_id).notes
else else
......
...@@ -128,7 +128,7 @@ module CommitsHelper ...@@ -128,7 +128,7 @@ module CommitsHelper
def revert_commit_link(commit, continue_to_path, btn_class: nil, has_tooltip: true) def revert_commit_link(commit, continue_to_path, btn_class: nil, has_tooltip: true)
return unless current_user 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? if can_collaborate_with_project?
btn_class = "btn btn-warning btn-#{btn_class}" unless btn_class.nil? btn_class = "btn btn-warning btn-#{btn_class}" unless btn_class.nil?
...@@ -152,7 +152,7 @@ module CommitsHelper ...@@ -152,7 +152,7 @@ module CommitsHelper
def cherry_pick_commit_link(commit, continue_to_path, btn_class: nil, has_tooltip: true) def cherry_pick_commit_link(commit, continue_to_path, btn_class: nil, has_tooltip: true)
return unless current_user 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? if can_collaborate_with_project?
btn_class = "btn btn-default btn-#{btn_class}" unless btn_class.nil? btn_class = "btn btn-default btn-#{btn_class}" unless btn_class.nil?
......
...@@ -242,44 +242,47 @@ class Commit ...@@ -242,44 +242,47 @@ class Commit
project.repository.next_branch("cherry-pick-#{short_id}", mild: true) project.repository.next_branch("cherry-pick-#{short_id}", mild: true)
end end
def revert_description def revert_description(user)
if merged_merge_request if merged_merge_request?(user)
"This reverts merge request #{merged_merge_request.to_reference}" "This reverts merge request #{merged_merge_request(user).to_reference}"
else else
"This reverts commit #{sha}" "This reverts commit #{sha}"
end end
end end
def revert_message def revert_message(user)
%Q{Revert "#{title.strip}"\n\n#{revert_description}} %Q{Revert "#{title.strip}"\n\n#{revert_description(user)}}
end end
def reverts_commit?(commit) def reverts_commit?(commit, user)
description? && description.include?(commit.revert_description) description? && description.include?(commit.revert_description(user))
end end
def merge_commit? def merge_commit?
parents.size > 1 parents.size > 1
end end
def merged_merge_request def merged_merge_request(current_user)
return @merged_merge_request if defined?(@merged_merge_request) # Memoize with per-user access check
@merged_merge_request_hash ||= Hash.new do |hash, user|
@merged_merge_request = project.merge_requests.find_by(merge_commit_sha: id) if merge_commit? hash[user] = merged_merge_request_no_cache(user)
end
@merged_merge_request_hash[current_user]
end end
def has_been_reverted?(current_user = nil, noteable = self) def has_been_reverted?(current_user, noteable = self)
ext = all_references(current_user) ext = all_references(current_user)
noteable.notes_with_associations.system.each do |note| noteable.notes_with_associations.system.each do |note|
note.all_references(current_user, extractor: ext) note.all_references(current_user, extractor: ext)
end end
ext.commits.any? { |commit_ref| commit_ref.reverts_commit?(self) } ext.commits.any? { |commit_ref| commit_ref.reverts_commit?(self, current_user) }
end end
def change_type_title def change_type_title(user)
merged_merge_request ? 'merge request' : 'commit' merged_merge_request?(user) ? 'merge request' : 'commit'
end end
# Get the URI type of the given path # Get the URI type of the given path
...@@ -337,4 +340,12 @@ class Commit ...@@ -337,4 +340,12 @@ class Commit
changes changes
end 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 end
module Milestoneish 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 issues_visible_to_user(user).closed.size + merge_requests.closed_and_merged.size
end end
def total_items_count(user = nil) def total_items_count(user)
issues_visible_to_user(user).size + merge_requests.size issues_visible_to_user(user).size + merge_requests.size
end end
def complete?(user = nil) def complete?(user)
total_items_count(user) > 0 && total_items_count(user) == closed_items_count(user) total_items_count(user) > 0 && total_items_count(user) == closed_items_count(user)
end end
def percent_complete(user = nil) def percent_complete(user)
((closed_items_count(user) * 100) / total_items_count(user)).abs ((closed_items_count(user) * 100) / total_items_count(user)).abs
rescue ZeroDivisionError rescue ZeroDivisionError
0 0
...@@ -23,7 +23,7 @@ module Milestoneish ...@@ -23,7 +23,7 @@ module Milestoneish
(due_date - Date.today).to_i (due_date - Date.today).to_i
end end
def issues_visible_to_user(user = nil) def issues_visible_to_user(user)
issues.visible_to_user(user) issues.visible_to_user(user)
end end
end end
...@@ -791,7 +791,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -791,7 +791,7 @@ class MergeRequest < ActiveRecord::Base
@merge_commit ||= project.commit(merge_commit_sha) if merge_commit_sha @merge_commit ||= project.commit(merge_commit_sha) if merge_commit_sha
end end
def can_be_reverted?(current_user = nil) def can_be_reverted?(current_user)
merge_commit && !merge_commit.has_been_reverted?(current_user, self) merge_commit && !merge_commit.has_been_reverted?(current_user, self)
end end
......
...@@ -894,7 +894,7 @@ class Repository ...@@ -894,7 +894,7 @@ class Repository
update_branch_with_hooks(user, base_branch) do update_branch_with_hooks(user, base_branch) do
committer = user_to_committer(user) committer = user_to_committer(user)
source_sha = Rugged::Commit.create(rugged, source_sha = Rugged::Commit.create(rugged,
message: commit.revert_message, message: commit.revert_message(user),
author: committer, author: committer,
committer: committer, committer: committer,
tree: revert_tree_id, tree: revert_tree_id,
......
...@@ -34,7 +34,7 @@ module Commits ...@@ -34,7 +34,7 @@ module Commits
repository.public_send(action, current_user, @commit, into, tree_id) repository.public_send(action, current_user, @commit, into, tree_id)
success success
else 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." 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 raise ChangeError, error_msg
end end
......
...@@ -77,7 +77,7 @@ ...@@ -77,7 +77,7 @@
= link_to namespace_project_merge_requests_path(@project.namespace, @project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do = link_to namespace_project_merge_requests_path(@project.namespace, @project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do
%span %span
Merge Requests 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 - if project_nav_tab? :wiki
= nav_link(controller: :wikis) do = nav_link(controller: :wikis) do
......
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
.modal-content .modal-content
.modal-header .modal-header
%a.close{href: "#", "data-dismiss" => "modal"} × %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 .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_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 .form-group.branch
......
---
title: Replace MR access checks with use of MergeRequestsFinder
merge_request:
author:
...@@ -68,7 +68,7 @@ module Gitlab ...@@ -68,7 +68,7 @@ module Gitlab
end end
def merge_requests 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/ if query =~ /[#!](\d+)\z/
merge_requests = merge_requests.where(iid: $1) merge_requests = merge_requests.where(iid: $1)
else else
......
...@@ -110,7 +110,7 @@ describe Projects::TodosController do ...@@ -110,7 +110,7 @@ describe Projects::TodosController do
end end
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 it 'does not create todo for merge request user has no access to' do
sign_in(user) sign_in(user)
expect do expect do
...@@ -128,6 +128,19 @@ describe Projects::TodosController do ...@@ -128,6 +128,19 @@ describe Projects::TodosController do
expect(response).to have_http_status(302) expect(response).to have_http_status(302)
end end
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 end
end end
...@@ -40,6 +40,15 @@ describe Gitlab::SearchResults do ...@@ -40,6 +40,15 @@ describe Gitlab::SearchResults do
expect(results.milestones_count).to eq(1) expect(results.milestones_count).to eq(1)
end end
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 end
it 'does not list issues on private projects' do it 'does not list issues on private projects' do
...@@ -152,4 +161,11 @@ describe Gitlab::SearchResults do ...@@ -152,4 +161,11 @@ describe Gitlab::SearchResults do
expect(results.issues_count).to eq 5 expect(results.issues_count).to eq 5
end end
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 end
...@@ -137,26 +137,25 @@ describe CommitRange, models: true do ...@@ -137,26 +137,25 @@ describe CommitRange, models: true do
end end
describe '#has_been_reverted?' do describe '#has_been_reverted?' do
it 'returns true if the commit has been reverted' do let(:issue) { create(:issue) }
issue = create(:issue) let(:user) { issue.author }
it 'returns true if the commit has been reverted' do
create(:note_on_issue, create(:note_on_issue,
noteable: issue, noteable: issue,
system: true, system: true,
note: commit1.revert_description, note: commit1.revert_description(user),
project: issue.project) project: issue.project)
expect_any_instance_of(Commit).to receive(:reverts_commit?). expect_any_instance_of(Commit).to receive(:reverts_commit?).
with(commit1). with(commit1, user).
and_return(true) and_return(true)
expect(commit1.has_been_reverted?(nil, issue)).to eq(true) expect(commit1.has_been_reverted?(user, issue)).to eq(true)
end end
it 'returns false a commit has not been reverted' do it 'returns false a commit has not been reverted' do
issue = create(:issue) expect(commit1.has_been_reverted?(user, issue)).to eq(false)
expect(commit1.has_been_reverted?(nil, issue)).to eq(false)
end end
end end
end end
...@@ -173,25 +173,26 @@ eos ...@@ -173,25 +173,26 @@ eos
describe '#reverts_commit?' do describe '#reverts_commit?' do
let(:another_commit) { double(:commit, revert_description: "This reverts commit #{commit.sha}") } let(:another_commit) { double(:commit, revert_description: "This reverts commit #{commit.sha}") }
let(:user) { commit.author }
it { expect(commit.reverts_commit?(another_commit)).to be_falsy } it { expect(commit.reverts_commit?(another_commit, user)).to be_falsy }
context 'commit has no description' do context 'commit has no description' do
before { allow(commit).to receive(:description?).and_return(false) } before { allow(commit).to receive(:description?).and_return(false) }
it { expect(commit.reverts_commit?(another_commit)).to be_falsy } it { expect(commit.reverts_commit?(another_commit, user)).to be_falsy }
end end
context "another_commit's description does not revert commit" do context "another_commit's description does not revert commit" do
before { allow(commit).to receive(:description).and_return("Foo Bar") } before { allow(commit).to receive(:description).and_return("Foo Bar") }
it { expect(commit.reverts_commit?(another_commit)).to be_falsy } it { expect(commit.reverts_commit?(another_commit, user)).to be_falsy }
end end
context "another_commit's description reverts commit" do context "another_commit's description reverts commit" do
before { allow(commit).to receive(:description).and_return("Foo #{another_commit.revert_description} Bar") } before { allow(commit).to receive(:description).and_return("Foo #{another_commit.revert_description} Bar") }
it { expect(commit.reverts_commit?(another_commit)).to be_truthy } it { expect(commit.reverts_commit?(another_commit, user)).to be_truthy }
end end
context "another_commit's description reverts merged merge request" do context "another_commit's description reverts merged merge request" do
...@@ -201,7 +202,7 @@ eos ...@@ -201,7 +202,7 @@ eos
allow(commit).to receive(:description).and_return("Foo #{another_commit.revert_description} Bar") allow(commit).to receive(:description).and_return("Foo #{another_commit.revert_description} Bar")
end end
it { expect(commit.reverts_commit?(another_commit)).to be_truthy } it { expect(commit.reverts_commit?(another_commit, user)).to be_truthy }
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment