Commit 00d489bf 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

 - 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
parent 51764c07
......@@ -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?
......
......@@ -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
......
......@@ -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|
......
......@@ -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
......@@ -26,7 +26,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
......
......@@ -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
......
......@@ -77,6 +77,10 @@ class IssuableFinder
counts
end
def find_by!(*params)
execute.find_by!(*params)
end
def group
return @group if defined?(@group)
......
......@@ -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
......
......@@ -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?
......
......@@ -253,44 +253,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)
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 = project.merge_requests.find_by(merge_commit_sha: id) if merge_commit?
@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
......@@ -348,4 +351,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
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
......@@ -23,7 +23,7 @@ module Milestoneish
(due_date - Date.today).to_i
end
def issues_visible_to_user(user = nil)
def issues_visible_to_user(user)
issues.visible_to_user(user)
end
end
......@@ -804,7 +804,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
......
......@@ -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,
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
---
title: Replace MR access checks with use of MergeRequestsFinder
merge_request:
author:
......@@ -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
......
......@@ -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
......@@ -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
......@@ -137,26 +137,25 @@ describe CommitRange, models: true do
end
describe '#has_been_reverted?' do
it 'returns true if the commit has been reverted' do
issue = create(:issue)
let(:issue) { create(:issue) }
let(:user) { issue.author }
it 'returns true if the commit has been reverted' do
create(:note_on_issue,
noteable: issue,
system: true,
note: commit1.revert_description,
note: commit1.revert_description(user),
project: issue.project)
expect_any_instance_of(Commit).to receive(:reverts_commit?).
with(commit1).
with(commit1, user).
and_return(true)
expect(commit1.has_been_reverted?(nil, issue)).to eq(true)
expect(commit1.has_been_reverted?(user, issue)).to eq(true)
end
it 'returns false a commit has not been reverted' do
issue = create(:issue)
expect(commit1.has_been_reverted?(nil, issue)).to eq(false)
expect(commit1.has_been_reverted?(user, issue)).to eq(false)
end
end
end
......@@ -173,25 +173,26 @@ eos
describe '#reverts_commit?' do
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
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
context "another_commit's description does not revert commit" do
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
context "another_commit's description reverts commit" do
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
context "another_commit's description reverts merged merge request" do
......@@ -201,7 +202,7 @@ eos
allow(commit).to receive(:description).and_return("Foo #{another_commit.revert_description} Bar")
end
it { expect(commit.reverts_commit?(another_commit)).to be_truthy }
it { expect(commit.reverts_commit?(another_commit, user)).to be_truthy }
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