From b2ef83856de8c175d384688d09023d16dcfef0c6 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 26 Feb 2018 13:32:42 +0100 Subject: 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. --- app/models/project.rb | 29 ++++++++++++++++ app/policies/project_policy.rb | 16 +++++++++ spec/lib/gitlab/checks/change_access_spec.rb | 6 ++-- spec/lib/gitlab/import_export/all_models.yml | 1 + spec/models/project_spec.rb | 52 ++++++++++++++++++++++++++++ spec/policies/project_policy_spec.rb | 37 ++++++++++++++++++++ 6 files changed, 138 insertions(+), 3 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 5cd1da43645..0128967e038 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -150,6 +150,7 @@ class Project < ActiveRecord::Base # Merge Requests for target project should be removed with it 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 :labels, class_name: 'ProjectLabel' has_many :services @@ -1799,6 +1800,14 @@ class Project < ActiveRecord::Base Badge.where("id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection 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 def storage @@ -1921,4 +1930,24 @@ class Project < ActiveRecord::Base raise ex 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 diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 3b0550b4dd6..ce94a25a20b 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -61,6 +61,11 @@ class ProjectPolicy < BasePolicy desc "Project has 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[ merge_requests issues @@ -240,6 +245,7 @@ class ProjectPolicy < BasePolicy rule { repository_disabled }.policy do prevent :push_code + prevent :push_single_branch prevent :download_code prevent :fork_project prevent :read_commit_status @@ -291,6 +297,16 @@ class ProjectPolicy < BasePolicy prevent :read_issue 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 def team_member? diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb index b49ddbfc780..2f173d2e75c 100644 --- a/spec/lib/gitlab/checks/change_access_spec.rb +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -30,9 +30,9 @@ describe Gitlab::Checks::ChangeAccess do 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 - 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.') end @@ -42,7 +42,7 @@ describe Gitlab::Checks::ChangeAccess do let(:ref) { 'refs/tags/v1.0.0' } 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 { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to change existing tags on this project.') diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index b20cc34dd5c..bece82e531a 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -278,6 +278,7 @@ project: - custom_attributes - lfs_file_locks - project_badges +- source_of_merge_requests award_emoji: - awardable - user diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index b1c9e6754b9..a95a4637234 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe Project do + include ProjectForksHelper + describe 'associations' do it { is_expected.to belong_to(:group) } it { is_expected.to belong_to(:namespace) } @@ -3378,4 +3380,54 @@ describe Project do 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 diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 129344f105f..f449fbd6b52 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -308,4 +308,41 @@ describe ProjectPolicy do it_behaves_like 'project policies as master' it_behaves_like 'project policies as owner' 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 -- cgit v1.2.1