Commit ef4a03f8 authored by Patrick Bajao's avatar Patrick Bajao

Prevent infinite loop when checking if collaboration is allowed

When there are merge requests in the same project that have their
source/target branches to each other and collaboration is allowed,
it can result to an infinite loop when a Reporter/Guest views a
project.

This fix adds a `skip_collaboration_check` to `Gitlab::UserAccess`
so when `Project#fetch_branch_allows_collaboration` calls
`MergeRequest#can_be_merged_by?` (which calls `Gitlab::UserAccess`
again), it will not check if collaboration is allowed.
parent ffcbbe06
......@@ -1337,8 +1337,8 @@ class MergeRequest < ApplicationRecord
has_no_commits? || branch_missing? || cannot_be_merged?
end
def can_be_merged_by?(user)
access = ::Gitlab::UserAccess.new(user, container: project)
def can_be_merged_by?(user, skip_collaboration_check: false)
access = ::Gitlab::UserAccess.new(user, container: project, skip_collaboration_check: skip_collaboration_check)
access.can_update_branch?(target_branch)
end
......
......@@ -2704,7 +2704,7 @@ class Project < ApplicationRecord
# Issue for N+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/49322
Gitlab::GitalyClient.allow_n_plus_1_calls do
merge_requests_allowing_collaboration(branch_name).any? do |merge_request|
merge_request.can_be_merged_by?(user)
merge_request.can_be_merged_by?(user, skip_collaboration_check: true)
end
end
end
......
---
title: Prevent infinite loop when checking if collaboration is allowed
merge_request:
author:
type: security
......@@ -11,10 +11,11 @@ module Gitlab
attr_reader :user, :push_ability
attr_accessor :container
def initialize(user, container: nil, push_ability: :push_code)
def initialize(user, container: nil, push_ability: :push_code, skip_collaboration_check: false)
@user = user
@container = container
@push_ability = push_ability
@skip_collaboration_check = skip_collaboration_check
end
def can_do_action?(action)
......@@ -87,6 +88,8 @@ module Gitlab
private
attr_reader :skip_collaboration_check
def can_push?
user.can?(push_ability, container)
end
......@@ -98,6 +101,8 @@ module Gitlab
end
def branch_allows_collaboration_for?(ref)
return false if skip_collaboration_check
# Checking for an internal project or group to prevent an infinite loop:
# https://gitlab.com/gitlab-org/gitlab/issues/36805
(!project.internal? && project.branch_allows_collaboration?(user, ref))
......
......@@ -216,6 +216,15 @@ RSpec.describe Gitlab::UserAccess do
expect(access.can_merge_to_branch?(@branch.name)).to be_falsey
end
end
context 'when skip_collaboration_check is true' do
let(:access) { described_class.new(user, container: project, skip_collaboration_check: true) }
it 'does not call Project#branch_allows_collaboration?' do
expect(project).not_to receive(:branch_allows_collaboration?)
expect(access.can_push_to_branch?('master')).to be_falsey
end
end
end
describe '#can_create_tag?' do
......
......@@ -5291,6 +5291,64 @@ RSpec.describe Project, factory_default: :keep do
end
end
describe '#branch_allows_collaboration?' do
context 'when there are open merge requests that have their source/target branches point to each other' do
let_it_be(:project) { create(:project, :repository) }
let_it_be(:developer) { create(:user) }
let_it_be(:reporter) { create(:user) }
let_it_be(:guest) { create(:user) }
before_all do
create(
:merge_request,
target_project: project,
target_branch: 'master',
source_project: project,
source_branch: 'merge-test',
allow_collaboration: true
)
create(
:merge_request,
target_project: project,
target_branch: 'merge-test',
source_project: project,
source_branch: 'master',
allow_collaboration: true
)
project.add_developer(developer)
project.add_reporter(reporter)
project.add_guest(guest)
end
shared_examples_for 'successful check' do
it 'does not go into an infinite loop' do
expect { project.branch_allows_collaboration?(user, 'master') }
.not_to raise_error
end
end
context 'when user is a developer' do
let(:user) { developer }
it_behaves_like 'successful check'
end
context 'when user is a reporter' do
let(:user) { reporter }
it_behaves_like 'successful check'
end
context 'when user is a guest' do
let(:user) { guest }
it_behaves_like 'successful check'
end
end
end
context 'with cross project merge requests' do
let(:user) { create(:user) }
let(:target_project) { create(:project, :repository) }
......
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