Commit bd0a12be authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch '46010-allow-managing-group-runners-via-api' into 'master'

API support + Improved policies for group runners

Closes #45894 and #38979

See merge request gitlab-org/gitlab-ce!18851
parents 357aaafb 1cfa5ed0
...@@ -69,7 +69,7 @@ module Projects ...@@ -69,7 +69,7 @@ module Projects
@project_runners = @project.runners.ordered @project_runners = @project.runners.ordered
@assignable_runners = current_user @assignable_runners = current_user
.ci_authorized_runners .ci_owned_runners
.assignable_for(project) .assignable_for(project)
.ordered .ordered
.page(params[:page]).per(20) .page(params[:page]).per(20)
......
...@@ -52,7 +52,7 @@ module Ci ...@@ -52,7 +52,7 @@ module Ci
# Without that, placeholders would miss one and couldn't match. # Without that, placeholders would miss one and couldn't match.
where(locked: false) where(locked: false)
.where.not("ci_runners.id IN (#{project.runners.select(:id).to_sql})") .where.not("ci_runners.id IN (#{project.runners.select(:id).to_sql})")
.specific .project_type
end end
validate :tag_constraints validate :tag_constraints
......
...@@ -999,12 +999,19 @@ class User < ActiveRecord::Base ...@@ -999,12 +999,19 @@ class User < ActiveRecord::Base
!solo_owned_groups.present? !solo_owned_groups.present?
end end
def ci_authorized_runners def ci_owned_runners
@ci_authorized_runners ||= begin @ci_owned_runners ||= begin
runner_ids = Ci::RunnerProject project_runner_ids = Ci::RunnerProject
.where(project: authorized_projects(Gitlab::Access::MASTER)) .where(project: authorized_projects(Gitlab::Access::MASTER))
.select(:runner_id) .select(:runner_id)
Ci::Runner.specific.where(id: runner_ids)
group_runner_ids = Ci::RunnerNamespace
.where(namespace_id: owned_or_masters_groups.select(:id))
.select(:runner_id)
union = Gitlab::SQL::Union.new([project_runner_ids, group_runner_ids])
Ci::Runner.specific.where("ci_runners.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
end end
end end
...@@ -1205,6 +1212,11 @@ class User < ActiveRecord::Base ...@@ -1205,6 +1212,11 @@ class User < ActiveRecord::Base
!terms_accepted? !terms_accepted?
end end
def owned_or_masters_groups
union = Gitlab::SQL::Union.new([owned_groups, masters_groups])
Group.from("(#{union.to_sql}) namespaces")
end
protected protected
# override, from Devise::Validatable # override, from Devise::Validatable
......
module Ci module Ci
class RunnerPolicy < BasePolicy class RunnerPolicy < BasePolicy
with_options scope: :subject, score: 0
condition(:shared) { @subject.is_shared? }
with_options scope: :subject, score: 0 with_options scope: :subject, score: 0
condition(:locked, scope: :subject) { @subject.locked? } condition(:locked, scope: :subject) { @subject.locked? }
condition(:authorized_runner) { @user.ci_authorized_runners.include?(@subject) } condition(:owned_runner) { @user.ci_owned_runners.exists?(@subject.id) }
rule { anonymous }.prevent_all rule { anonymous }.prevent_all
rule { admin | authorized_runner }.enable :assign_runner
rule { ~admin & shared }.prevent :assign_runner rule { admin | owned_runner }.policy do
enable :assign_runner
enable :read_runner
enable :update_runner
enable :delete_runner
end
rule { ~admin & locked }.prevent :assign_runner rule { ~admin & locked }.prevent :assign_runner
end end
end end
...@@ -14,7 +14,7 @@ module API ...@@ -14,7 +14,7 @@ module API
use :pagination use :pagination
end end
get do get do
runners = filter_runners(current_user.ci_authorized_runners, params[:scope], without: %w(specific shared)) runners = filter_runners(current_user.ci_owned_runners, params[:scope], without: %w(specific shared))
present paginate(runners), with: Entities::Runner present paginate(runners), with: Entities::Runner
end end
...@@ -184,40 +184,35 @@ module API ...@@ -184,40 +184,35 @@ module API
def authenticate_show_runner!(runner) def authenticate_show_runner!(runner)
return if runner.is_shared || current_user.admin? return if runner.is_shared || current_user.admin?
forbidden!("No access granted") unless user_can_access_runner?(runner) forbidden!("No access granted") unless can?(current_user, :read_runner, runner)
end end
def authenticate_update_runner!(runner) def authenticate_update_runner!(runner)
return if current_user.admin? return if current_user.admin?
forbidden!("Runner is shared") if runner.is_shared? forbidden!("No access granted") unless can?(current_user, :update_runner, runner)
forbidden!("No access granted") unless user_can_access_runner?(runner)
end end
def authenticate_delete_runner!(runner) def authenticate_delete_runner!(runner)
return if current_user.admin? return if current_user.admin?
forbidden!("Runner is shared") if runner.is_shared?
forbidden!("Runner associated with more than one project") if runner.projects.count > 1 forbidden!("Runner associated with more than one project") if runner.projects.count > 1
forbidden!("No access granted") unless user_can_access_runner?(runner) forbidden!("No access granted") unless can?(current_user, :delete_runner, runner)
end end
def authenticate_enable_runner!(runner) def authenticate_enable_runner!(runner)
forbidden!("Runner is shared") if runner.is_shared? forbidden!("Runner is a group runner") if runner.group_type?
forbidden!("Runner is locked") if runner.locked?
return if current_user.admin? return if current_user.admin?
forbidden!("No access granted") unless user_can_access_runner?(runner) forbidden!("Runner is locked") if runner.locked?
forbidden!("No access granted") unless can?(current_user, :assign_runner, runner)
end end
def authenticate_list_runners_jobs!(runner) def authenticate_list_runners_jobs!(runner)
return if current_user.admin? return if current_user.admin?
forbidden!("No access granted") unless user_can_access_runner?(runner) forbidden!("No access granted") unless can?(current_user, :read_runner, runner)
end
def user_can_access_runner?(runner)
current_user.ci_authorized_runners.exists?(runner.id)
end end
end end
end end
......
...@@ -58,7 +58,7 @@ module API ...@@ -58,7 +58,7 @@ module API
end end
def user_can_access_runner?(runner) def user_can_access_runner?(runner)
current_user.ci_authorized_runners.exists?(runner.id) current_user.ci_owned_runners.exists?(runner.id)
end end
end end
end end
......
...@@ -19,11 +19,11 @@ describe Projects::Settings::CiCdController do ...@@ -19,11 +19,11 @@ describe Projects::Settings::CiCdController do
end end
context 'with group runners' do context 'with group runners' do
let(:group_runner) { create(:ci_runner) } let(:group_runner) { create(:ci_runner, runner_type: :group_type) }
let(:parent_group) { create(:group) } let(:parent_group) { create(:group) }
let(:group) { create(:group, runners: [group_runner], parent: parent_group) } let(:group) { create(:group, runners: [group_runner], parent: parent_group) }
let(:other_project) { create(:project, group: group) } let(:other_project) { create(:project, group: group) }
let!(:project_runner) { create(:ci_runner, projects: [other_project]) } let!(:project_runner) { create(:ci_runner, projects: [other_project], runner_type: :project_type) }
let!(:shared_runner) { create(:ci_runner, :shared) } let!(:shared_runner) { create(:ci_runner, :shared) }
it 'sets assignable project runners only' do it 'sets assignable project runners only' do
...@@ -31,7 +31,7 @@ describe Projects::Settings::CiCdController do ...@@ -31,7 +31,7 @@ describe Projects::Settings::CiCdController do
get :show, namespace_id: project.namespace, project_id: project get :show, namespace_id: project.namespace, project_id: project
expect(assigns(:assignable_runners)).to eq [project_runner] expect(assigns(:assignable_runners)).to contain_exactly(project_runner)
end end
end end
end end
......
...@@ -626,62 +626,26 @@ describe Ci::Runner do ...@@ -626,62 +626,26 @@ describe Ci::Runner do
end end
describe '.assignable_for' do describe '.assignable_for' do
let(:runner) { create(:ci_runner) } let!(:unlocked_project_runner) { create(:ci_runner, runner_type: :project_type, projects: [project]) }
let!(:locked_project_runner) { create(:ci_runner, runner_type: :project_type, locked: true, projects: [project]) }
let!(:group_runner) { create(:ci_runner, runner_type: :group_type) }
let!(:instance_runner) { create(:ci_runner, :shared) }
let(:project) { create(:project) } let(:project) { create(:project) }
let(:another_project) { create(:project) } let(:another_project) { create(:project) }
before do context 'with already assigned project' do
project.runners << runner
end
context 'with shared runners' do
before do
runner.update(is_shared: true)
end
context 'does not give owned runner' do
subject { described_class.assignable_for(project) }
it { is_expected.to be_empty }
end
context 'does not give shared runner' do
subject { described_class.assignable_for(another_project) }
it { is_expected.to be_empty }
end
end
context 'with unlocked runner' do
context 'does not give owned runner' do
subject { described_class.assignable_for(project) } subject { described_class.assignable_for(project) }
it { is_expected.to be_empty } it { is_expected.to be_empty }
end end
context 'does give a specific runner' do context 'with a different project' do
subject { described_class.assignable_for(another_project) } subject { described_class.assignable_for(another_project) }
it { is_expected.to contain_exactly(runner) } it { is_expected.to include(unlocked_project_runner) }
end it { is_expected.not_to include(group_runner) }
end it { is_expected.not_to include(locked_project_runner) }
it { is_expected.not_to include(instance_runner) }
context 'with locked runner' do
before do
runner.update(locked: true)
end
context 'does not give owned runner' do
subject { described_class.assignable_for(project) }
it { is_expected.to be_empty }
end
context 'does not give a locked runner' do
subject { described_class.assignable_for(another_project) }
it { is_expected.to be_empty }
end
end end
end end
......
...@@ -1786,28 +1786,54 @@ describe User do ...@@ -1786,28 +1786,54 @@ describe User do
end end
end end
describe '#ci_authorized_runners' do describe '#ci_owned_runners' do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:runner) { create(:ci_runner) } let(:runner_1) { create(:ci_runner) }
let(:runner_2) { create(:ci_runner) }
before do context 'without any projects nor groups' do
project.runners << runner let!(:project) { create(:project, runners: [runner_1]) }
end let!(:group) { create(:group) }
context 'without any projects' do
let(:project) { create(:project) }
it 'does not load' do it 'does not load' do
expect(user.ci_authorized_runners).to be_empty expect(user.ci_owned_runners).to be_empty
end end
end end
context 'with personal projects runners' do context 'with personal projects runners' do
let(:namespace) { create(:namespace, owner: user) } let(:namespace) { create(:namespace, owner: user) }
let(:project) { create(:project, namespace: namespace) } let!(:project) { create(:project, namespace: namespace, runners: [runner_1]) }
it 'loads' do
expect(user.ci_owned_runners).to contain_exactly(runner_1)
end
end
context 'with personal group runner' do
let!(:project) { create(:project, runners: [runner_1]) }
let!(:group) do
create(:group, runners: [runner_2]).tap do |group|
group.add_owner(user)
end
end
it 'loads' do
expect(user.ci_owned_runners).to contain_exactly(runner_2)
end
end
context 'with personal project and group runner' do
let(:namespace) { create(:namespace, owner: user) }
let!(:project) { create(:project, namespace: namespace, runners: [runner_1]) }
let!(:group) do
create(:group, runners: [runner_2]).tap do |group|
group.add_owner(user)
end
end
it 'loads' do it 'loads' do
expect(user.ci_authorized_runners).to contain_exactly(runner) expect(user.ci_owned_runners).to contain_exactly(runner_1, runner_2)
end end
end end
...@@ -1818,7 +1844,7 @@ describe User do ...@@ -1818,7 +1844,7 @@ describe User do
end end
it 'loads' do it 'loads' do
expect(user.ci_authorized_runners).to contain_exactly(runner) expect(user.ci_owned_runners).to contain_exactly(runner_1)
end end
end end
...@@ -1828,14 +1854,28 @@ describe User do ...@@ -1828,14 +1854,28 @@ describe User do
end end
it 'does not load' do it 'does not load' do
expect(user.ci_authorized_runners).to be_empty expect(user.ci_owned_runners).to be_empty
end end
end end
end end
context 'with groups projects runners' do context 'with groups projects runners' do
let(:group) { create(:group) } let(:group) { create(:group) }
let(:project) { create(:project, group: group) } let!(:project) { create(:project, group: group, runners: [runner_1]) }
def add_user(access)
group.add_user(user, access)
end
it_behaves_like :member
end
context 'with groups runners' do
let!(:group) do
create(:group, runners: [runner_1]).tap do |group|
group.add_owner(user)
end
end
def add_user(access) def add_user(access)
group.add_user(user, access) group.add_user(user, access)
...@@ -1845,7 +1885,7 @@ describe User do ...@@ -1845,7 +1885,7 @@ describe User do
end end
context 'with other projects runners' do context 'with other projects runners' do
let(:project) { create(:project) } let!(:project) { create(:project, runners: [runner_1]) }
def add_user(access) def add_user(access)
project.add_role(user, access) project.add_role(user, access)
...@@ -1858,7 +1898,7 @@ describe User do ...@@ -1858,7 +1898,7 @@ describe User do
let(:group) { create(:group) } let(:group) { create(:group) }
let(:another_user) { create(:user) } let(:another_user) { create(:user) }
let(:subgroup) { create(:group, parent: group) } let(:subgroup) { create(:group, parent: group) }
let(:project) { create(:project, group: subgroup) } let!(:project) { create(:project, group: subgroup, runners: [runner_1]) }
def add_user(access) def add_user(access)
group.add_user(user, access) group.add_user(user, access)
......
...@@ -27,7 +27,7 @@ describe API::Runners do ...@@ -27,7 +27,7 @@ describe API::Runners do
end end
end end
let!(:group_runner) { create(:ci_runner, description: 'Group runner', groups: [group]) } let!(:group_runner) { create(:ci_runner, description: 'Group runner', groups: [group], runner_type: :group_type) }
before do before do
# Set project access for users # Set project access for users
...@@ -48,7 +48,7 @@ describe API::Runners do ...@@ -48,7 +48,7 @@ describe API::Runners do
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response[0]).to have_key('ip_address') expect(json_response[0]).to have_key('ip_address')
expect(descriptions).to contain_exactly( expect(descriptions).to contain_exactly(
'Project runner', 'Two projects runner' 'Project runner', 'Two projects runner', 'Group runner'
) )
expect(shared).to be_falsey expect(shared).to be_falsey
end end
...@@ -592,6 +592,15 @@ describe API::Runners do ...@@ -592,6 +592,15 @@ describe API::Runners do
end.to change { project.runners.count }.by(+1) end.to change { project.runners.count }.by(+1)
expect(response).to have_gitlab_http_status(201) expect(response).to have_gitlab_http_status(201)
end end
it 'enables a shared runner' do
expect do
post api("/projects/#{project.id}/runners", admin), runner_id: shared_runner.id
end.to change { project.runners.count }.by(1)
expect(shared_runner.reload).not_to be_shared
expect(response).to have_gitlab_http_status(201)
end
end end
context 'user is not admin' do context 'user is not admin' do
......
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