diff options
author | Kamil Trzciński <ayufan@ayufan.eu> | 2018-05-16 20:28:42 +0000 |
---|---|---|
committer | Kamil Trzciński <ayufan@ayufan.eu> | 2018-05-16 20:28:42 +0000 |
commit | bd0a12be77840ca49cdd296e4a6d701df99024b4 (patch) | |
tree | 1dd558f653a37ec30e7302c66424163bdf9e41ee | |
parent | 357aaafb73a6d8c4e0185f193d67a06ed1b79832 (diff) | |
parent | 1cfa5ed07065f04531900fe0931deaaaef3e69d2 (diff) | |
download | gitlab-ce-bd0a12be77840ca49cdd296e4a6d701df99024b4.tar.gz |
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
-rw-r--r-- | app/controllers/projects/settings/ci_cd_controller.rb | 2 | ||||
-rw-r--r-- | app/models/ci/runner.rb | 2 | ||||
-rw-r--r-- | app/models/user.rb | 20 | ||||
-rw-r--r-- | app/policies/ci/runner_policy.rb | 15 | ||||
-rw-r--r-- | lib/api/runners.rb | 23 | ||||
-rw-r--r-- | lib/api/v3/runners.rb | 2 | ||||
-rw-r--r-- | spec/controllers/projects/settings/ci_cd_controller_spec.rb | 6 | ||||
-rw-r--r-- | spec/models/ci/runner_spec.rb | 62 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 72 | ||||
-rw-r--r-- | spec/requests/api/runners_spec.rb | 13 |
10 files changed, 120 insertions, 97 deletions
diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index 177c8a54099..1d850baf012 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -69,7 +69,7 @@ module Projects @project_runners = @project.runners.ordered @assignable_runners = current_user - .ci_authorized_runners + .ci_owned_runners .assignable_for(project) .ordered .page(params[:page]).per(20) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index bda69f85a78..e6f1ed519be 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -52,7 +52,7 @@ module Ci # Without that, placeholders would miss one and couldn't match. where(locked: false) .where.not("ci_runners.id IN (#{project.runners.select(:id).to_sql})") - .specific + .project_type end validate :tag_constraints diff --git a/app/models/user.rb b/app/models/user.rb index 173ab38e20c..226a4489261 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -999,12 +999,19 @@ class User < ActiveRecord::Base !solo_owned_groups.present? end - def ci_authorized_runners - @ci_authorized_runners ||= begin - runner_ids = Ci::RunnerProject + def ci_owned_runners + @ci_owned_runners ||= begin + project_runner_ids = Ci::RunnerProject .where(project: authorized_projects(Gitlab::Access::MASTER)) .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 @@ -1205,6 +1212,11 @@ class User < ActiveRecord::Base !terms_accepted? end + def owned_or_masters_groups + union = Gitlab::SQL::Union.new([owned_groups, masters_groups]) + Group.from("(#{union.to_sql}) namespaces") + end + protected # override, from Devise::Validatable diff --git a/app/policies/ci/runner_policy.rb b/app/policies/ci/runner_policy.rb index 7dff8470e23..895abe87d86 100644 --- a/app/policies/ci/runner_policy.rb +++ b/app/policies/ci/runner_policy.rb @@ -1,16 +1,19 @@ module Ci class RunnerPolicy < BasePolicy with_options scope: :subject, score: 0 - condition(:shared) { @subject.is_shared? } - - with_options scope: :subject, score: 0 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 { 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 end end diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 5f2a9567605..5cb96d467c0 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -14,7 +14,7 @@ module API use :pagination end 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 end @@ -184,40 +184,35 @@ module API def authenticate_show_runner!(runner) 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 def authenticate_update_runner!(runner) return if current_user.admin? - forbidden!("Runner is shared") if runner.is_shared? - forbidden!("No access granted") unless user_can_access_runner?(runner) + forbidden!("No access granted") unless can?(current_user, :update_runner, runner) end def authenticate_delete_runner!(runner) 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!("No access granted") unless user_can_access_runner?(runner) + forbidden!("No access granted") unless can?(current_user, :delete_runner, runner) end def authenticate_enable_runner!(runner) - forbidden!("Runner is shared") if runner.is_shared? - forbidden!("Runner is locked") if runner.locked? + forbidden!("Runner is a group runner") if runner.group_type? + 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 def authenticate_list_runners_jobs!(runner) return if current_user.admin? - forbidden!("No access granted") unless user_can_access_runner?(runner) - end - - def user_can_access_runner?(runner) - current_user.ci_authorized_runners.exists?(runner.id) + forbidden!("No access granted") unless can?(current_user, :read_runner, runner) end end end diff --git a/lib/api/v3/runners.rb b/lib/api/v3/runners.rb index c6d9957d452..8a5c46805bd 100644 --- a/lib/api/v3/runners.rb +++ b/lib/api/v3/runners.rb @@ -58,7 +58,7 @@ module API end 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 diff --git a/spec/controllers/projects/settings/ci_cd_controller_spec.rb b/spec/controllers/projects/settings/ci_cd_controller_spec.rb index a91c868cbaf..f1810763d2d 100644 --- a/spec/controllers/projects/settings/ci_cd_controller_spec.rb +++ b/spec/controllers/projects/settings/ci_cd_controller_spec.rb @@ -19,11 +19,11 @@ describe Projects::Settings::CiCdController do end 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(:group) { create(:group, runners: [group_runner], parent: parent_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) } it 'sets assignable project runners only' do @@ -31,7 +31,7 @@ describe Projects::Settings::CiCdController do 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 diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index e2b212f4f4c..0fbc934f669 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -626,62 +626,26 @@ describe Ci::Runner do end 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(:another_project) { create(:project) } - before 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) } - - it { is_expected.to be_empty } - end + context 'with already assigned project' do + subject { described_class.assignable_for(project) } - context 'does give a specific runner' do - subject { described_class.assignable_for(another_project) } - - it { is_expected.to contain_exactly(runner) } - end + it { is_expected.to be_empty } end - 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) } + context 'with a different project' do + subject { described_class.assignable_for(another_project) } - it { is_expected.to be_empty } - end + it { is_expected.to include(unlocked_project_runner) } + it { is_expected.not_to include(group_runner) } + it { is_expected.not_to include(locked_project_runner) } + it { is_expected.not_to include(instance_runner) } end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index bb5308221f0..de15e0e62aa 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1786,28 +1786,54 @@ describe User do end end - describe '#ci_authorized_runners' do + describe '#ci_owned_runners' do let(:user) { create(:user) } - let(:runner) { create(:ci_runner) } + let(:runner_1) { create(:ci_runner) } + let(:runner_2) { create(:ci_runner) } - before do - project.runners << runner - end - - context 'without any projects' do - let(:project) { create(:project) } + context 'without any projects nor groups' do + let!(:project) { create(:project, runners: [runner_1]) } + let!(:group) { create(:group) } it 'does not load' do - expect(user.ci_authorized_runners).to be_empty + expect(user.ci_owned_runners).to be_empty end end context 'with personal projects runners' do 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_authorized_runners).to contain_exactly(runner) + 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 + expect(user.ci_owned_runners).to contain_exactly(runner_1, runner_2) end end @@ -1818,7 +1844,7 @@ describe User do end it 'loads' do - expect(user.ci_authorized_runners).to contain_exactly(runner) + expect(user.ci_owned_runners).to contain_exactly(runner_1) end end @@ -1828,14 +1854,28 @@ describe User do end it 'does not load' do - expect(user.ci_authorized_runners).to be_empty + 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) } + 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) group.add_user(user, access) @@ -1845,7 +1885,7 @@ describe User do end context 'with other projects runners' do - let(:project) { create(:project) } + let!(:project) { create(:project, runners: [runner_1]) } def add_user(access) project.add_role(user, access) @@ -1858,7 +1898,7 @@ describe User do let(:group) { create(:group) } let(:another_user) { create(:user) } 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) group.add_user(user, access) diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 981ac768e3a..c7587c877fc 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -27,7 +27,7 @@ describe API::Runners do 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 # Set project access for users @@ -48,7 +48,7 @@ describe API::Runners do expect(json_response).to be_an Array expect(json_response[0]).to have_key('ip_address') expect(descriptions).to contain_exactly( - 'Project runner', 'Two projects runner' + 'Project runner', 'Two projects runner', 'Group runner' ) expect(shared).to be_falsey end @@ -592,6 +592,15 @@ describe API::Runners do end.to change { project.runners.count }.by(+1) expect(response).to have_gitlab_http_status(201) 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 context 'user is not admin' do |