Commit edb4d1ce authored by Nacho Otal's avatar Nacho Otal Committed by Mayra Cabrera

Issue #26426 Drafting a solution for the retrieval of runners that belong to...

Issue #26426 Drafting a solution for the retrieval of runners that belong to subgroups for the owner of the parent group

Issue #26426 : Using the owned_groups with self_and_descendants to get
all the groups that a user owns even per inheritance of ownership.
Fixing the test case so that it behaves as expected.

Issue #26426 : No need to have two separate cases for group_runners and
subgroup_runners since the subgroups inherit the ownership from the
groups.

Issue #26426 : Improving description of the example.

Issue #26426 : As discussed, get rid of the example extraction and
inline it in the test case.

Issue #26426 : As discussed, adding a test case for inclusion of runners
that belong to groups in which the user is maintainer (and developer).

issue #26426 : Adding reporter and guest to shared examples for
completion.

Issue #26426 : Adding multi-owner, group owner and project owner as
shared examples.

Issue #26426 : Refining the case of group owner.

Issue #26426 : Making all let imperative. Reviewing the cases so that
they're complete.

Issue #26426 : Improving wording on the descriptions of the test. Adding
completion cases tests.
parent 7207dcb1
......@@ -1405,7 +1405,7 @@ class User < ApplicationRecord
.select('ci_runners.*')
group_runners = Ci::RunnerNamespace
.where(namespace_id: owned_groups.select(:id))
.where(namespace_id: Gitlab::ObjectHierarchy.new(owned_groups).base_and_descendants.select(:id))
.joins(:runner)
.select('ci_runners.*')
......
---
title: 'Fix for issue 26426: Details of runners of nested groups of an owned group
are now available for users with enough permissions'
merge_request: 24169
author: nachootal@gmail.com
type: changed
......@@ -2835,61 +2835,88 @@ describe User, :do_not_mock_admin_mode do
describe '#ci_owned_runners' do
let(:user) { create(:user) }
let!(:project) { create(:project) }
let(:runner) { create(:ci_runner, :project, projects: [project]) }
context 'without any projects nor groups' do
it 'does not load' do
expect(user.ci_owned_runners).to be_empty
shared_examples :nested_groups_owner do
context 'when the user is the owner of a multi-level group' do
before do
set_permissions_for_users
end
it 'loads all the runners in the tree of groups' do
expect(user.ci_owned_runners).to contain_exactly(runner, group_runner)
end
end
end
context 'with personal projects runners' do
let(:namespace) { create(:namespace, owner: user) }
let!(:project) { create(:project, namespace: namespace) }
shared_examples :group_owner do
context 'when the user is the owner of a one level group' do
before do
group.add_owner(user)
end
it 'loads' do
expect(user.ci_owned_runners).to contain_exactly(runner)
it 'loads the runners in the group' do
expect(user.ci_owned_runners).to contain_exactly(group_runner)
end
end
end
context 'with personal group runner' do
let!(:project) { create(:project) }
let(:group_runner) { create(:ci_runner, :group, groups: [group]) }
let!(:group) do
create(:group).tap do |group|
group.add_owner(user)
shared_examples :project_owner do
context 'when the user is the owner of a project' do
it 'loads the runner belonging to the project' do
expect(user.ci_owned_runners).to contain_exactly(runner)
end
end
end
it 'loads' do
expect(user.ci_owned_runners).to contain_exactly(group_runner)
shared_examples :project_member do
context 'when the user is a maintainer' do
before do
add_user(:maintainer)
end
it 'loads the runners of the project' do
expect(user.ci_owned_runners).to contain_exactly(project_runner)
end
end
end
context 'with personal project and group runner' do
let(:namespace) { create(:namespace, owner: user) }
let!(:project) { create(:project, namespace: namespace) }
let!(:group_runner) { create(:ci_runner, :group, groups: [group]) }
context 'when the user is a developer' do
before do
add_user(:developer)
end
let!(:group) do
create(:group).tap do |group|
group.add_owner(user)
it 'does not load any runner' do
expect(user.ci_owned_runners).to be_empty
end
end
context 'when the user is a reporter' do
before do
add_user(:reporter)
end
it 'does not load any runner' do
expect(user.ci_owned_runners).to be_empty
end
end
it 'loads' do
expect(user.ci_owned_runners).to contain_exactly(runner, group_runner)
context 'when the user is a guest' do
before do
add_user(:guest)
end
it 'does not load any runner' do
expect(user.ci_owned_runners).to be_empty
end
end
end
shared_examples :member do
shared_examples :group_member do
context 'when the user is a maintainer' do
before do
add_user(:maintainer)
end
it 'does not load' do
it 'does not load the runners of the group' do
expect(user.ci_owned_runners).to be_empty
end
end
......@@ -2899,29 +2926,49 @@ describe User, :do_not_mock_admin_mode do
add_user(:developer)
end
it 'does not load' do
it 'does not load any runner' do
expect(user.ci_owned_runners).to be_empty
end
end
end
shared_examples :group_member do
context 'when the user is owner' do
context 'when the user is a reporter' do
before do
add_user(:owner)
add_user(:reporter)
end
it 'loads' do
expect(user.ci_owned_runners).to contain_exactly(runner)
it 'does not load any runner' do
expect(user.ci_owned_runners).to be_empty
end
end
it_behaves_like :member
context 'when the user is a guest' do
before do
add_user(:guest)
end
it 'does not load any runner' do
expect(user.ci_owned_runners).to be_empty
end
end
end
context 'with groups projects runners' do
let(:group) { create(:group) }
let!(:project) { create(:project, group: group) }
context 'without any projects nor groups' do
it 'does not load any runner' do
expect(user.ci_owned_runners).to be_empty
end
end
context 'with runner in a personal project' do
let!(:namespace) { create(:namespace, owner: user) }
let!(:project) { create(:project, namespace: namespace) }
let!(:runner) { create(:ci_runner, :project, projects: [project]) }
it_behaves_like :project_owner
end
context 'with group runner in a non owned group' do
let!(:group) { create(:group) }
let!(:runner) { create(:ci_runner, :group, groups: [group]) }
def add_user(access)
group.add_user(user, access)
......@@ -2930,37 +2977,114 @@ describe User, :do_not_mock_admin_mode do
it_behaves_like :group_member
end
context 'with groups runners' do
context 'with group runner in an owned group' do
let!(:group) { create(:group) }
let!(:group_runner) { create(:ci_runner, :group, groups: [group]) }
it_behaves_like :group_owner
end
context 'with group runner in an owned group and group runner in a different owner subgroup' do
let!(:group) { create(:group) }
let!(:runner) { create(:ci_runner, :group, groups: [group]) }
let!(:subgroup) { create(:group, parent: group) }
let!(:group_runner) { create(:ci_runner, :group, groups: [subgroup]) }
let!(:another_user) { create(:user) }
def set_permissions_for_users
group.add_owner(user)
subgroup.add_owner(another_user)
end
it_behaves_like :nested_groups_owner
end
context 'with personal project runner in an an owned group and a group runner in that same group' do
let!(:group) { create(:group) }
let!(:group_runner) { create(:ci_runner, :group, groups: [group]) }
let!(:project) { create(:project, group: group) }
let!(:runner) { create(:ci_runner, :project, projects: [project]) }
def add_user(access)
group.add_user(user, access)
def set_permissions_for_users
group.add_owner(user)
end
it_behaves_like :group_member
it_behaves_like :nested_groups_owner
end
context 'with other projects runners' do
let!(:project) { create(:project) }
context 'with personal project runner in an owned group and a group runner in a subgroup' do
let!(:group) { create(:group) }
let!(:subgroup) { create(:group, parent: group) }
let!(:group_runner) { create(:ci_runner, :group, groups: [subgroup]) }
let!(:project) { create(:project, group: group) }
let!(:runner) { create(:ci_runner, :project, projects: [project]) }
def set_permissions_for_users
group.add_owner(user)
end
it_behaves_like :nested_groups_owner
end
context 'with personal project runner in an owned group in an owned namespace and a group runner in that group' do
let!(:namespace) { create(:namespace, owner: user) }
let!(:group) { create(:group) }
let!(:group_runner) { create(:ci_runner, :group, groups: [group]) }
let!(:project) { create(:project, namespace: namespace, group: group) }
let!(:runner) { create(:ci_runner, :project, projects: [project]) }
def set_permissions_for_users
group.add_owner(user)
end
it_behaves_like :nested_groups_owner
end
context 'with personal project runner in an owned namespace, an owned group, a subgroup and a group runner in that subgroup' do
let!(:namespace) { create(:namespace, owner: user) }
let!(:group) { create(:group) }
let!(:subgroup) { create(:group, parent: group) }
let!(:group_runner) { create(:ci_runner, :group, groups: [subgroup]) }
let!(:project) { create(:project, namespace: namespace, group: group) }
let!(:runner) { create(:ci_runner, :project, projects: [project]) }
def set_permissions_for_users
group.add_owner(user)
end
it_behaves_like :nested_groups_owner
end
context 'with a project runner that belong to projects that belong to a not owned group' do
let!(:group) { create(:group) }
let!(:project) { create(:project, group: group) }
let!(:project_runner) { create(:ci_runner, :project, projects: [project]) }
def add_user(access)
project.add_user(user, access)
end
it_behaves_like :member
it_behaves_like :project_member
end
context 'with subgroup with different owner for project runner' do
let(:group) { create(:group) }
let(:another_user) { create(:user) }
let(:subgroup) { create(:group, parent: group) }
let!(:project) { create(:project, group: subgroup) }
context 'with project runners that belong to projects that do not belong to any group' do
let!(:project) { create(:project) }
let!(:runner) { create(:ci_runner, :project, projects: [project]) }
it 'does not load any runner' do
expect(user.ci_owned_runners).to be_empty
end
end
context 'with a group runner that belongs to a subgroup of a group owned by another user' do
let!(:group) { create(:group) }
let!(:subgroup) { create(:group, parent: group) }
let!(:runner) { create(:ci_runner, :group, groups: [subgroup]) }
let!(:another_user) { create(:user) }
def add_user(access)
group.add_user(user, access)
subgroup.add_user(user, access)
group.add_user(another_user, :owner)
subgroup.add_user(another_user, :owner)
end
it_behaves_like :group_member
......
......@@ -6,20 +6,28 @@ describe API::Runners do
let(:admin) { create(:user, :admin) }
let(:user) { create(:user) }
let(:user2) { create(:user) }
let(:group_guest) { create(:user) }
let(:group_reporter) { create(:user) }
let(:group_developer) { create(:user) }
let(:group_maintainer) { create(:user) }
let(:project) { create(:project, creator_id: user.id) }
let(:project2) { create(:project, creator_id: user.id) }
let(:group) { create(:group).tap { |group| group.add_owner(user) } }
let(:subgroup) { create(:group, parent: group) }
let!(:shared_runner) { create(:ci_runner, :instance, description: 'Shared runner') }
let!(:project_runner) { create(:ci_runner, :project, description: 'Project runner', projects: [project]) }
let!(:two_projects_runner) { create(:ci_runner, :project, description: 'Two projects runner', projects: [project, project2]) }
let!(:group_runner) { create(:ci_runner, :group, description: 'Group runner', groups: [group]) }
let!(:group_runner_a) { create(:ci_runner, :group, description: 'Group runner A', groups: [group]) }
let!(:group_runner_b) { create(:ci_runner, :group, description: 'Group runner B', groups: [subgroup]) }
before do
# Set project access for users
create(:group_member, :guest, user: group_guest, group: group)
create(:group_member, :reporter, user: group_reporter, group: group)
create(:group_member, :developer, user: group_developer, group: group)
create(:group_member, :maintainer, user: group_maintainer, group: group)
create(:project_member, :maintainer, user: user, project: project)
create(:project_member, :maintainer, user: user, project: project2)
......@@ -41,7 +49,8 @@ describe API::Runners do
expect(json_response).to match_array [
a_hash_including('description' => 'Project runner'),
a_hash_including('description' => 'Two projects runner'),
a_hash_including('description' => 'Group runner')
a_hash_including('description' => 'Group runner A'),
a_hash_including('description' => 'Group runner B')
]
end
......@@ -131,7 +140,8 @@ describe API::Runners do
expect(json_response).to match_array [
a_hash_including('description' => 'Project runner'),
a_hash_including('description' => 'Two projects runner'),
a_hash_including('description' => 'Group runner'),
a_hash_including('description' => 'Group runner A'),
a_hash_including('description' => 'Group runner B'),
a_hash_including('description' => 'Shared runner')
]
end
......@@ -156,7 +166,8 @@ describe API::Runners do
expect(json_response).to match_array [
a_hash_including('description' => 'Project runner'),
a_hash_including('description' => 'Two projects runner'),
a_hash_including('description' => 'Group runner')
a_hash_including('description' => 'Group runner A'),
a_hash_including('description' => 'Group runner B')
]
end
......@@ -165,7 +176,7 @@ describe API::Runners do
expect(response).to have_gitlab_http_status(:bad_request)
end
it 'filters runners by type' do
it 'filters runners by project type' do
get api('/runners/all?type=project_type', admin)
expect(json_response).to match_array [
......@@ -174,6 +185,15 @@ describe API::Runners do
]
end
it 'filters runners by group type' do
get api('/runners/all?type=group_type', admin)
expect(json_response).to match_array [
a_hash_including('description' => 'Group runner A'),
a_hash_including('description' => 'Group runner B')
]
end
it 'does not filter by invalid type' do
get api('/runners/all?type=bogus', admin)
......@@ -526,15 +546,41 @@ describe API::Runners do
end.to change { Ci::Runner.project_type.count }.by(-1)
end
it 'does not delete group runner with guest access' do
delete api("/runners/#{group_runner_a.id}", group_guest)
expect(response).to have_gitlab_http_status(:forbidden)
end
it 'does not delete group runner with reporter access' do
delete api("/runners/#{group_runner_a.id}", group_reporter)
expect(response).to have_gitlab_http_status(:forbidden)
end
it 'does not delete group runner with developer access' do
delete api("/runners/#{group_runner_a.id}", group_developer)
expect(response).to have_gitlab_http_status(:forbidden)
end
it 'does not delete group runner with maintainer access' do
delete api("/runners/#{group_runner.id}", group_maintainer)
delete api("/runners/#{group_runner_a.id}", group_maintainer)
expect(response).to have_gitlab_http_status(:forbidden)
end
it 'deletes group runner with owner access' do
it 'deletes owned group runner with owner access' do
expect do
delete api("/runners/#{group_runner_a.id}", user)
expect(response).to have_gitlab_http_status(:no_content)
end.to change { Ci::Runner.group_type.count }.by(-1)
end
it 'deletes inherited group runner with owner access' do
expect do
delete api("/runners/#{group_runner.id}", user)
delete api("/runners/#{group_runner_b.id}", user)
expect(response).to have_gitlab_http_status(:no_content)
end.to change { Ci::Runner.group_type.count }.by(-1)
......@@ -842,7 +888,7 @@ describe API::Runners do
get api("/groups/#{group.id}/runners", user)
expect(json_response).to match_array([
a_hash_including('description' => 'Group runner')
a_hash_including('description' => 'Group runner A')
])
end
......@@ -851,7 +897,7 @@ describe API::Runners do
get api("/groups/#{group.id}/runners?type=group_type", user)
expect(json_response).to match_array([
a_hash_including('description' => 'Group runner')
a_hash_including('description' => 'Group runner A')
])
end
......@@ -939,7 +985,7 @@ describe API::Runners do
end
it 'does not enable group runner' do
post api("/projects/#{project.id}/runners", user), params: { runner_id: group_runner.id }
post api("/projects/#{project.id}/runners", user), params: { runner_id: group_runner_a.id }
expect(response).to have_gitlab_http_status(:forbidden)
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