From f1320e4f938a5f508628b21a012db1617eab8946 Mon Sep 17 00:00:00 2001 From: manojmj Date: Mon, 23 Sep 2019 13:12:26 +0530 Subject: Require maintainer permission to transfer projects --- app/policies/group_policy.rb | 2 + app/policies/namespace_policy.rb | 2 + app/services/projects/transfer_service.rb | 2 +- .../security-developer-transfer-project.yml | 5 ++ spec/policies/group_policy_spec.rb | 82 ++++++++++++++++++++++ spec/policies/namespace_policy_spec.rb | 3 +- spec/requests/api/projects_spec.rb | 16 +++++ spec/services/projects/transfer_service_spec.rb | 18 +++++ 8 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/security-developer-transfer-project.yml diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 9e8ee3acf00..13e5b4ae41a 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -131,6 +131,8 @@ class GroupPolicy < BasePolicy rule { owner | admin }.enable :read_statistics + rule { maintainer & can?(:create_projects) }.enable :transfer_projects + def access_level return GroupMember::NO_ACCESS if @user.nil? diff --git a/app/policies/namespace_policy.rb b/app/policies/namespace_policy.rb index fd3bdddded6..350dd208499 100644 --- a/app/policies/namespace_policy.rb +++ b/app/policies/namespace_policy.rb @@ -15,6 +15,8 @@ class NamespacePolicy < BasePolicy end rule { personal_project & ~can_create_personal_project }.prevent :create_projects + + rule { (owner | admin) & can?(:create_projects) }.enable :transfer_projects end NamespacePolicy.prepend_if_ee('EE::NamespacePolicy') diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index 4b3aca58dd7..525fc18b312 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -98,7 +98,7 @@ module Projects @new_namespace && can?(current_user, :change_namespace, project) && @new_namespace.id != project.namespace_id && - current_user.can?(:create_projects, @new_namespace) + current_user.can?(:transfer_projects, @new_namespace) end def update_namespace_and_visibility(to_namespace) diff --git a/changelogs/unreleased/security-developer-transfer-project.yml b/changelogs/unreleased/security-developer-transfer-project.yml new file mode 100644 index 00000000000..fe533fc099a --- /dev/null +++ b/changelogs/unreleased/security-developer-transfer-project.yml @@ -0,0 +1,5 @@ +--- +title: Require Maintainer permission on group where project is transferred to +merge_request: +author: +type: security diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 603e7e874c9..aeb09c1dc3a 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -354,6 +354,88 @@ describe GroupPolicy do end end + context 'transfer_projects' do + shared_examples_for 'allowed to transfer projects' do + before do + group.update(project_creation_level: project_creation_level) + end + + it { is_expected.to be_allowed(:transfer_projects) } + end + + shared_examples_for 'not allowed to transfer projects' do + before do + group.update(project_creation_level: project_creation_level) + end + + it { is_expected.to be_disallowed(:transfer_projects) } + end + + context 'reporter' do + let(:current_user) { reporter } + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::NO_ONE_PROJECT_ACCESS } + end + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS } + end + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS } + end + end + + context 'developer' do + let(:current_user) { developer } + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::NO_ONE_PROJECT_ACCESS } + end + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS } + end + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS } + end + end + + context 'maintainer' do + let(:current_user) { maintainer } + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::NO_ONE_PROJECT_ACCESS } + end + + it_behaves_like 'allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS } + end + + it_behaves_like 'allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS } + end + end + + context 'owner' do + let(:current_user) { owner } + + it_behaves_like 'not allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::NO_ONE_PROJECT_ACCESS } + end + + it_behaves_like 'allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS } + end + + it_behaves_like 'allowed to transfer projects' do + let(:project_creation_level) { ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS } + end + end + end + context "create_projects" do context 'when group has no project creation level set' do before_all do diff --git a/spec/policies/namespace_policy_spec.rb b/spec/policies/namespace_policy_spec.rb index 216aaae70ee..909c17fe8b5 100644 --- a/spec/policies/namespace_policy_spec.rb +++ b/spec/policies/namespace_policy_spec.rb @@ -6,7 +6,7 @@ describe NamespacePolicy do let(:admin) { create(:admin) } let(:namespace) { create(:namespace, owner: owner) } - let(:owner_permissions) { [:create_projects, :admin_namespace, :read_namespace, :read_statistics] } + let(:owner_permissions) { [:create_projects, :admin_namespace, :read_namespace, :read_statistics, :transfer_projects] } subject { described_class.new(current_user, namespace) } @@ -31,6 +31,7 @@ describe NamespacePolicy do let(:owner) { create(:user, projects_limit: 0) } it { is_expected.to be_disallowed(:create_projects) } + it { is_expected.to be_disallowed(:transfer_projects) } end end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 2d8ef9c06dc..99d2a68ef53 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -2648,6 +2648,22 @@ describe API::Projects do expect(response).to have_gitlab_http_status(400) end end + + context 'when authenticated as developer' do + before do + group.add_developer(user) + end + + context 'target namespace allows developers to create projects' do + let(:group) { create(:group, project_creation_level: ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS) } + + it 'fails transferring the project to the target namespace' do + put api("/projects/#{project.id}/transfer", user), params: { namespace: group.id } + + expect(response).to have_gitlab_http_status(400) + end + end + end end it_behaves_like 'custom attributes endpoints', 'projects' do diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 26d8ac9b479..298867f483b 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -222,6 +222,24 @@ describe Projects::TransferService do it { expect(project.errors[:new_namespace]).to include('Project with same name or path in target namespace already exists') } end + context 'target namespace allows developers to create projects' do + let(:group) { create(:group, project_creation_level: ::Gitlab::Access::DEVELOPER_MAINTAINER_PROJECT_ACCESS) } + + context 'the user is a member of the target namespace with developer permissions' do + subject(:transfer_project_result) { transfer_project(project, user, group) } + + before do + group.add_developer(user) + end + + it 'does not allow project transfer to the target namespace' do + expect(transfer_project_result).to eq false + expect(project.namespace).to eq(user.namespace) + expect(project.errors[:new_namespace]).to include('Transfer failed, please contact an admin.') + end + end + end + def transfer_project(project, user, new_namespace) service = Projects::TransferService.new(project, user) -- cgit v1.2.1