Allow abilities on forks while MR is open
When an MR is created using `allow_maintainer_to_push`, we enable some abilities while the MR is open. This should allow every user with developer abilities on the target project, to push to the source project.
This commit is contained in:
parent
792ab0631c
commit
b2ef83856d
|
@ -150,6 +150,7 @@ class Project < ActiveRecord::Base
|
||||||
|
|
||||||
# Merge Requests for target project should be removed with it
|
# Merge Requests for target project should be removed with it
|
||||||
has_many :merge_requests, foreign_key: 'target_project_id'
|
has_many :merge_requests, foreign_key: 'target_project_id'
|
||||||
|
has_many :source_of_merge_requests, foreign_key: 'source_project_id', class_name: 'MergeRequest'
|
||||||
has_many :issues
|
has_many :issues
|
||||||
has_many :labels, class_name: 'ProjectLabel'
|
has_many :labels, class_name: 'ProjectLabel'
|
||||||
has_many :services
|
has_many :services
|
||||||
|
@ -1799,6 +1800,14 @@ class Project < ActiveRecord::Base
|
||||||
Badge.where("id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
|
Badge.where("id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def branches_allowing_maintainer_access_to_user(user)
|
||||||
|
@branches_allowing_maintainer_access_to_user ||= Hash.new do |result, user|
|
||||||
|
result[user] = fetch_branches_allowing_maintainer_access_to_user(user)
|
||||||
|
end
|
||||||
|
|
||||||
|
@branches_allowing_maintainer_access_to_user[user]
|
||||||
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
def storage
|
def storage
|
||||||
|
@ -1921,4 +1930,24 @@ class Project < ActiveRecord::Base
|
||||||
|
|
||||||
raise ex
|
raise ex
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def fetch_branches_allowing_maintainer_access_to_user(user)
|
||||||
|
return [] unless user
|
||||||
|
|
||||||
|
projects_with_developer_access = user.project_authorizations
|
||||||
|
.where('access_level >= ? ', Gitlab::Access::DEVELOPER)
|
||||||
|
.select(:project_id)
|
||||||
|
merge_requests_allowing_push = source_of_merge_requests.opened
|
||||||
|
.where(allow_maintainer_to_push: true)
|
||||||
|
.where(target_project_id: projects_with_developer_access)
|
||||||
|
.select(:source_branch).uniq
|
||||||
|
|
||||||
|
if RequestStore.active?
|
||||||
|
RequestStore.fetch("project-#{id}:user-#{user.id}:branches_allowing_maintainer_access") do
|
||||||
|
merge_requests_allowing_push.pluck(:source_branch)
|
||||||
|
end
|
||||||
|
else
|
||||||
|
merge_requests_allowing_push.pluck(:source_branch)
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -61,6 +61,11 @@ class ProjectPolicy < BasePolicy
|
||||||
desc "Project has request access enabled"
|
desc "Project has request access enabled"
|
||||||
condition(:request_access_enabled, scope: :subject) { project.request_access_enabled }
|
condition(:request_access_enabled, scope: :subject) { project.request_access_enabled }
|
||||||
|
|
||||||
|
desc "The project has merge requests open that allow external users to push"
|
||||||
|
condition(:merge_request_allows_push, scope: :subject) do
|
||||||
|
project.branches_allowing_maintainer_access_to_user(@user).any?
|
||||||
|
end
|
||||||
|
|
||||||
features = %w[
|
features = %w[
|
||||||
merge_requests
|
merge_requests
|
||||||
issues
|
issues
|
||||||
|
@ -240,6 +245,7 @@ class ProjectPolicy < BasePolicy
|
||||||
|
|
||||||
rule { repository_disabled }.policy do
|
rule { repository_disabled }.policy do
|
||||||
prevent :push_code
|
prevent :push_code
|
||||||
|
prevent :push_single_branch
|
||||||
prevent :download_code
|
prevent :download_code
|
||||||
prevent :fork_project
|
prevent :fork_project
|
||||||
prevent :read_commit_status
|
prevent :read_commit_status
|
||||||
|
@ -291,6 +297,16 @@ class ProjectPolicy < BasePolicy
|
||||||
prevent :read_issue
|
prevent :read_issue
|
||||||
end
|
end
|
||||||
|
|
||||||
|
# These rules are included to allow maintainers of projects to push to certain
|
||||||
|
# branches of forks.
|
||||||
|
rule { can?(:public_access) & merge_request_allows_push }.policy do
|
||||||
|
enable :push_single_branch
|
||||||
|
enable :create_build
|
||||||
|
enable :update_build
|
||||||
|
enable :create_pipeline
|
||||||
|
enable :update_pipeline
|
||||||
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
def team_member?
|
def team_member?
|
||||||
|
|
|
@ -30,9 +30,9 @@ describe Gitlab::Checks::ChangeAccess do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'when the user is not allowed to push code' do
|
context 'when the user is not allowed to push to the repo' do
|
||||||
it 'raises an error' do
|
it 'raises an error' do
|
||||||
expect(user_access).to receive(:can_do_action?).with(:push_code).and_return(false)
|
expect(user_access).to receive(:can_do_action?).with(:push_to_repo).and_return(false)
|
||||||
|
|
||||||
expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.')
|
expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.')
|
||||||
end
|
end
|
||||||
|
@ -42,7 +42,7 @@ describe Gitlab::Checks::ChangeAccess do
|
||||||
let(:ref) { 'refs/tags/v1.0.0' }
|
let(:ref) { 'refs/tags/v1.0.0' }
|
||||||
|
|
||||||
it 'raises an error if the user is not allowed to update tags' do
|
it 'raises an error if the user is not allowed to update tags' do
|
||||||
allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true)
|
allow(user_access).to receive(:can_do_action?).with(:push_to_repo).and_return(true)
|
||||||
expect(user_access).to receive(:can_do_action?).with(:admin_project).and_return(false)
|
expect(user_access).to receive(:can_do_action?).with(:admin_project).and_return(false)
|
||||||
|
|
||||||
expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to change existing tags on this project.')
|
expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to change existing tags on this project.')
|
||||||
|
|
|
@ -278,6 +278,7 @@ project:
|
||||||
- custom_attributes
|
- custom_attributes
|
||||||
- lfs_file_locks
|
- lfs_file_locks
|
||||||
- project_badges
|
- project_badges
|
||||||
|
- source_of_merge_requests
|
||||||
award_emoji:
|
award_emoji:
|
||||||
- awardable
|
- awardable
|
||||||
- user
|
- user
|
||||||
|
|
|
@ -1,6 +1,8 @@
|
||||||
require 'spec_helper'
|
require 'spec_helper'
|
||||||
|
|
||||||
describe Project do
|
describe Project do
|
||||||
|
include ProjectForksHelper
|
||||||
|
|
||||||
describe 'associations' do
|
describe 'associations' do
|
||||||
it { is_expected.to belong_to(:group) }
|
it { is_expected.to belong_to(:group) }
|
||||||
it { is_expected.to belong_to(:namespace) }
|
it { is_expected.to belong_to(:namespace) }
|
||||||
|
@ -3378,4 +3380,54 @@ describe Project do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
describe '#branches_allowing_maintainer_access_to_user' do
|
||||||
|
let(:maintainer) { create(:user) }
|
||||||
|
let(:target_project) { create(:project) }
|
||||||
|
let(:project) { fork_project(target_project) }
|
||||||
|
let!(:merge_request) do
|
||||||
|
create(
|
||||||
|
:merge_request,
|
||||||
|
target_project: target_project,
|
||||||
|
source_project: project,
|
||||||
|
source_branch: 'awesome-feature-1',
|
||||||
|
allow_maintainer_to_push: true
|
||||||
|
)
|
||||||
|
end
|
||||||
|
|
||||||
|
before do
|
||||||
|
target_project.add_developer(maintainer)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'includes branch names for merge requests allowing maintainer access to a user' do
|
||||||
|
expect(project.branches_allowing_maintainer_access_to_user(maintainer))
|
||||||
|
.to include('awesome-feature-1')
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'does not include branches for closed MRs' do
|
||||||
|
create(:merge_request, :closed,
|
||||||
|
target_project: target_project,
|
||||||
|
source_project: project,
|
||||||
|
source_branch: 'rejected-feature-1',
|
||||||
|
allow_maintainer_to_push: true)
|
||||||
|
|
||||||
|
expect(project.branches_allowing_maintainer_access_to_user(maintainer))
|
||||||
|
.not_to include('rejected-feature-1')
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'only queries once per user' do
|
||||||
|
expect { 3.times { project.branches_allowing_maintainer_access_to_user(maintainer) } }
|
||||||
|
.not_to exceed_query_limit(1)
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when the requeststore is active', :request_store do
|
||||||
|
it 'only queries once per user accross project instances' do
|
||||||
|
# limiting to 3 queries:
|
||||||
|
# 2 times loading the project
|
||||||
|
# once loading the accessible branches
|
||||||
|
expect { 2.times { described_class.find(project.id).branches_allowing_maintainer_access_to_user(maintainer) } }
|
||||||
|
.not_to exceed_query_limit(3)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -308,4 +308,41 @@ describe ProjectPolicy do
|
||||||
it_behaves_like 'project policies as master'
|
it_behaves_like 'project policies as master'
|
||||||
it_behaves_like 'project policies as owner'
|
it_behaves_like 'project policies as owner'
|
||||||
it_behaves_like 'project policies as admin'
|
it_behaves_like 'project policies as admin'
|
||||||
|
|
||||||
|
context 'when a public project has merge requests allowing access' do
|
||||||
|
include ProjectForksHelper
|
||||||
|
let(:user) { create(:user) }
|
||||||
|
let(:target_project) { create(:project, :public) }
|
||||||
|
let(:project) { fork_project(target_project) }
|
||||||
|
let!(:merge_request) do
|
||||||
|
create(
|
||||||
|
:merge_request,
|
||||||
|
target_project: target_project,
|
||||||
|
source_project: project,
|
||||||
|
allow_maintainer_to_push: true
|
||||||
|
)
|
||||||
|
end
|
||||||
|
let(:maintainer_abilities) do
|
||||||
|
%w(push_single_branch create_build update_build create_pipeline update_pipeline)
|
||||||
|
end
|
||||||
|
|
||||||
|
subject { described_class.new(user, project) }
|
||||||
|
|
||||||
|
it 'does not allow pushing code' do
|
||||||
|
expect_disallowed(*maintainer_abilities)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'allows pushing if the user is a member with push access to the target project' do
|
||||||
|
target_project.add_developer(user)
|
||||||
|
|
||||||
|
expect_allowed(*maintainer_abilities)
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'dissallows abilities to a maintainer if the merge request was closed' do
|
||||||
|
target_project.add_developer(user)
|
||||||
|
merge_request.close!
|
||||||
|
|
||||||
|
expect_disallowed(*maintainer_abilities)
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in New Issue