From b55c3a7bc4c23618860916738702b5d62820c351 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Tue, 12 Sep 2017 11:34:34 +0200 Subject: [PATCH] support group runners in existing API endpoints --- lib/api/entities.rb | 18 ++- lib/api/runner.rb | 5 +- lib/api/runners.rb | 1 + lib/api/v3/runners.rb | 1 + spec/requests/api/runner_spec.rb | 15 +- spec/requests/api/runners_spec.rb | 219 +++++++++++++++++++-------- spec/requests/api/v3/runners_spec.rb | 57 +++++-- 7 files changed, 239 insertions(+), 77 deletions(-) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 8aad320e376..f28c4bcc784 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -242,13 +242,18 @@ module API expose :requested_at end - class Group < Grape::Entity - expose :id, :name, :path, :description, :visibility + class BasicGroupDetails < Grape::Entity + expose :id + expose :web_url + expose :name + end + + class Group < BasicGroupDetails + expose :path, :description, :visibility expose :lfs_enabled?, as: :lfs_enabled expose :avatar_url do |group, options| group.avatar_url(only_path: false) end - expose :web_url expose :request_access_enabled expose :full_name, :full_path @@ -965,6 +970,13 @@ module API options[:current_user].authorized_projects.where(id: runner.projects) end end + expose :groups, with: Entities::BasicGroupDetails do |runner, options| + if options[:current_user].admin? + runner.groups + else + options[:current_user].authorized_groups.where(id: runner.groups) + end + end end class RunnerRegistrationDetails < Grape::Entity diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 4d4fbe50f9f..49d9b0b1b4f 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -25,8 +25,11 @@ module API # Create shared runner. Requires admin access Ci::Runner.create(attributes.merge(is_shared: true)) elsif project = Project.find_by(runners_token: params[:token]) - # Create a specific runner for project. + # Create a specific runner for the project project.runners.create(attributes) + elsif group = Group.find_by(runners_token: params[:token]) + # Create a specific runner for the group + group.runners.create(attributes) end break forbidden! unless runner diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 5f2a9567605..ef4ec3f4800 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -199,6 +199,7 @@ module API 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 that one group") if runner.groups.count > 1 forbidden!("No access granted") unless user_can_access_runner?(runner) end diff --git a/lib/api/v3/runners.rb b/lib/api/v3/runners.rb index c6d9957d452..24e10128b79 100644 --- a/lib/api/v3/runners.rb +++ b/lib/api/v3/runners.rb @@ -54,6 +54,7 @@ module API 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 that one group") if runner.groups.count > 1 forbidden!("No access granted") unless user_can_access_runner?(runner) end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 17c7a511857..5ea110b4d82 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -45,7 +45,7 @@ describe API::Runner do context 'when project token is used' do let(:project) { create(:project) } - it 'creates runner' do + it 'creates project runner' do post api('/runners'), token: project.runners_token expect(response).to have_gitlab_http_status 201 @@ -54,6 +54,19 @@ describe API::Runner do expect(Ci::Runner.first.token).not_to eq(project.runners_token) end end + + context 'when group token is used' do + let(:group) { create(:group) } + + it 'creates a group runner' do + post api('/runners'), token: group.runners_token + + expect(response).to have_http_status 201 + expect(group.runners.size).to eq(1) + expect(Ci::Runner.first.token).not_to eq(registration_token) + expect(Ci::Runner.first.token).not_to eq(group.runners_token) + end + end end context 'when runner description is provided' do diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index d30f0cf36e2..5a2d607960e 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -8,22 +8,29 @@ describe API::Runners do let(:project) { create(:project, creator_id: user.id) } let(:project2) { create(:project, creator_id: user.id) } - let!(:shared_runner) { create(:ci_runner, :shared) } - let!(:unused_specific_runner) { create(:ci_runner) } + let(:group) { create(:group).tap { |group| group.add_owner(user) } } + let(:group2) { create(:group).tap { |group| group.add_owner(user) } } - let!(:specific_runner) do - create(:ci_runner).tap do |runner| + let!(:shared_runner) { create(:ci_runner, :shared, description: 'Shared runner') } + let!(:unused_project_runner) { create(:ci_runner) } + + let!(:project_runner) do + create(:ci_runner, description: 'Project runner').tap do |runner| create(:ci_runner_project, runner: runner, project: project) end end let!(:two_projects_runner) do - create(:ci_runner).tap do |runner| + create(:ci_runner, description: 'Two projects runner').tap do |runner| create(:ci_runner_project, runner: runner, project: project) create(:ci_runner_project, runner: runner, project: project2) end end + let!(:group_runner) { create(:ci_runner, description: 'Group runner', groups: [group]) } + + let!(:two_groups_runner) { create(:ci_runner, description: 'Two groups runner', groups: [group, group2]) } + before do # Set project access for users create(:project_member, :master, user: user, project: project) @@ -37,9 +44,13 @@ describe API::Runners do get api('/runners', user) shared = json_response.any? { |r| r['is_shared'] } + descriptions = json_response.map { |runner| runner['description'] } expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array + expect(descriptions).to contain_exactly( + 'Project runner', 'Group runner', 'Two projects runner', 'Two groups runner' + ) expect(shared).to be_falsey end @@ -129,10 +140,26 @@ describe API::Runners do context 'when runner is not shared' do it "returns runner's details" do - get api("/runners/#{specific_runner.id}", admin) + get api("/runners/#{project_runner.id}", admin) expect(response).to have_gitlab_http_status(200) - expect(json_response['description']).to eq(specific_runner.description) + expect(json_response['description']).to eq(project_runner.description) + end + + it "returns the project's details for a project runner" do + get api("/runners/#{project_runner.id}", admin) + + expect(json_response['projects'].first['id']).to eq(project.id) + end + + it "returns the group's details for a group runner" do + get api("/runners/#{group_runner.id}", admin) + + expect(json_response['groups'].first).to eq( + 'id' => group.id, + 'web_url' => group.web_url, + 'name' => group.name + ) end end @@ -146,10 +173,10 @@ describe API::Runners do context "runner project's administrative user" do context 'when runner is not shared' do it "returns runner's details" do - get api("/runners/#{specific_runner.id}", user) + get api("/runners/#{project_runner.id}", user) expect(response).to have_gitlab_http_status(200) - expect(json_response['description']).to eq(specific_runner.description) + expect(json_response['description']).to eq(project_runner.description) end end @@ -163,17 +190,40 @@ describe API::Runners do end end + context "runner group's administrative user" do + context 'when runner is not shared' do + it "returns runner's details" do + get api("/runners/#{group_runner.id}", user) + + expect(response).to have_http_status(200) + expect(json_response['id']).to eq(group_runner.id) + end + end + end + context 'other authorized user' do - it "does not return runner's details" do - get api("/runners/#{specific_runner.id}", user2) + it "does not return project runner's details" do + get api("/runners/#{project_runner.id}", user2) + + expect(response).to have_http_status(403) + end + + it "does not return group runner's details" do + get api("/runners/#{group_runner.id}", user2) expect(response).to have_gitlab_http_status(403) end end context 'unauthorized user' do - it "does not return runner's details" do - get api("/runners/#{specific_runner.id}") + it "does not return project runner's details" do + get api("/runners/#{project_runner.id}") + + expect(response).to have_http_status(401) + end + + it "does not return group runner's details" do + get api("/runners/#{group_runner.id}") expect(response).to have_gitlab_http_status(401) end @@ -212,16 +262,16 @@ describe API::Runners do context 'when runner is not shared' do it 'updates runner' do - description = specific_runner.description - runner_queue_value = specific_runner.ensure_runner_queue_value + description = project_runner.description + runner_queue_value = project_runner.ensure_runner_queue_value - update_runner(specific_runner.id, admin, description: 'test') - specific_runner.reload + update_runner(project_runner.id, admin, description: 'test') + project_runner.reload expect(response).to have_gitlab_http_status(200) - expect(specific_runner.description).to eq('test') - expect(specific_runner.description).not_to eq(description) - expect(specific_runner.ensure_runner_queue_value) + expect(project_runner.description).to eq('test') + expect(project_runner.description).not_to eq(description) + expect(project_runner.ensure_runner_queue_value) .not_to eq(runner_queue_value) end end @@ -247,27 +297,49 @@ describe API::Runners do end context 'when runner is not shared' do - it 'does not update runner without access to it' do - put api("/runners/#{specific_runner.id}", user2), description: 'test' + it 'does not update project runner without access to it' do + put api("/runners/#{project_runner.id}", user2), description: 'test' + + expect(response).to have_http_status(403) + end + + it 'does not update group runner without access to it' do + put api("/runners/#{group_runner.id}", user2), description: 'test' expect(response).to have_gitlab_http_status(403) end - it 'updates runner with access to it' do - description = specific_runner.description - put api("/runners/#{specific_runner.id}", admin), description: 'test' - specific_runner.reload + it 'updates project runner with access to it' do + description = project_runner.description + put api("/runners/#{project_runner.id}", admin), description: 'test' + project_runner.reload expect(response).to have_gitlab_http_status(200) - expect(specific_runner.description).to eq('test') - expect(specific_runner.description).not_to eq(description) + expect(project_runner.description).to eq('test') + expect(project_runner.description).not_to eq(description) + end + + it 'updates group runner with access to it' do + description = group_runner.description + put api("/runners/#{group_runner.id}", admin), description: 'test' + group_runner.reload + + expect(response).to have_gitlab_http_status(200) + expect(group_runner.description).to eq('test') + expect(group_runner.description).not_to eq(description) end end end context 'unauthorized user' do - it 'does not delete runner' do - put api("/runners/#{specific_runner.id}") + it 'does not delete project runner' do + put api("/runners/#{project_runner.id}") + + expect(response).to have_http_status(401) + end + + it 'does not delete group runner' do + put api("/runners/#{group_runner.id}") expect(response).to have_gitlab_http_status(401) end @@ -293,15 +365,23 @@ describe API::Runners do context 'when runner is not shared' do it 'deletes unused runner' do expect do - delete api("/runners/#{unused_specific_runner.id}", admin) + delete api("/runners/#{unused_project_runner.id}", admin) expect(response).to have_gitlab_http_status(204) end.to change { Ci::Runner.specific.count }.by(-1) end - it 'deletes used runner' do + it 'deletes used project runner' do expect do - delete api("/runners/#{specific_runner.id}", admin) + delete api("/runners/#{project_runner.id}", admin) + + expect(response).to have_http_status(204) + end.to change { Ci::Runner.specific.count }.by(-1) + end + + it 'deletes used group runner' do + expect do + delete api("/runners/#{group_runner.id}", admin) expect(response).to have_gitlab_http_status(204) end.to change { Ci::Runner.specific.count }.by(-1) @@ -325,32 +405,51 @@ describe API::Runners do context 'when runner is not shared' do it 'does not delete runner without access to it' do - delete api("/runners/#{specific_runner.id}", user2) + delete api("/runners/#{project_runner.id}", user2) expect(response).to have_gitlab_http_status(403) end - it 'does not delete runner with more than one associated project' do + it 'does not delete project runner with more than one associated project' do delete api("/runners/#{two_projects_runner.id}", user) expect(response).to have_gitlab_http_status(403) end - it 'deletes runner for one owned project' do + it 'deletes project runner for one owned project' do expect do - delete api("/runners/#{specific_runner.id}", user) + delete api("/runners/#{project_runner.id}", user) + + expect(response).to have_http_status(204) + end.to change { Ci::Runner.specific.count }.by(-1) + end + + it 'does not delete group runner with more than one associated group' do + delete api("/runners/#{two_groups_runner.id}", user) + expect(response).to have_http_status(403) + end + + it 'deletes group runner for one owned group' do + expect do + delete api("/runners/#{group_runner.id}", user) expect(response).to have_gitlab_http_status(204) end.to change { Ci::Runner.specific.count }.by(-1) end it_behaves_like '412 response' do - let(:request) { api("/runners/#{specific_runner.id}", user) } + let(:request) { api("/runners/#{project_runner.id}", user) } end end end context 'unauthorized user' do - it 'does not delete runner' do - delete api("/runners/#{specific_runner.id}") + it 'does not delete project runner' do + delete api("/runners/#{project_runner.id}") + + expect(response).to have_http_status(401) + end + + it 'does not delete group runner' do + delete api("/runners/#{group_runner.id}") expect(response).to have_gitlab_http_status(401) end @@ -361,8 +460,8 @@ describe API::Runners do set(:job_1) { create(:ci_build) } let!(:job_2) { create(:ci_build, :running, runner: shared_runner, project: project) } let!(:job_3) { create(:ci_build, :failed, runner: shared_runner, project: project) } - let!(:job_4) { create(:ci_build, :running, runner: specific_runner, project: project) } - let!(:job_5) { create(:ci_build, :failed, runner: specific_runner, project: project) } + let!(:job_4) { create(:ci_build, :running, runner: project_runner, project: project) } + let!(:job_5) { create(:ci_build, :failed, runner: project_runner, project: project) } context 'admin user' do context 'when runner exists' do @@ -380,7 +479,7 @@ describe API::Runners do context 'when runner is specific' do it 'return jobs' do - get api("/runners/#{specific_runner.id}/jobs", admin) + get api("/runners/#{project_runner.id}/jobs", admin) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -392,7 +491,7 @@ describe API::Runners do context 'when valid status is provided' do it 'return filtered jobs' do - get api("/runners/#{specific_runner.id}/jobs?status=failed", admin) + get api("/runners/#{project_runner.id}/jobs?status=failed", admin) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -405,7 +504,7 @@ describe API::Runners do context 'when invalid status is provided' do it 'return 400' do - get api("/runners/#{specific_runner.id}/jobs?status=non-existing", admin) + get api("/runners/#{project_runner.id}/jobs?status=non-existing", admin) expect(response).to have_gitlab_http_status(400) end @@ -433,7 +532,7 @@ describe API::Runners do context 'when runner is specific' do it 'return jobs' do - get api("/runners/#{specific_runner.id}/jobs", user) + get api("/runners/#{project_runner.id}/jobs", user) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -445,7 +544,7 @@ describe API::Runners do context 'when valid status is provided' do it 'return filtered jobs' do - get api("/runners/#{specific_runner.id}/jobs?status=failed", user) + get api("/runners/#{project_runner.id}/jobs?status=failed", user) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -458,7 +557,7 @@ describe API::Runners do context 'when invalid status is provided' do it 'return 400' do - get api("/runners/#{specific_runner.id}/jobs?status=non-existing", user) + get api("/runners/#{project_runner.id}/jobs?status=non-existing", user) expect(response).to have_gitlab_http_status(400) end @@ -476,7 +575,7 @@ describe API::Runners do context 'other authorized user' do it 'does not return jobs' do - get api("/runners/#{specific_runner.id}/jobs", user2) + get api("/runners/#{project_runner.id}/jobs", user2) expect(response).to have_gitlab_http_status(403) end @@ -484,7 +583,7 @@ describe API::Runners do context 'unauthorized user' do it 'does not return jobs' do - get api("/runners/#{specific_runner.id}/jobs") + get api("/runners/#{project_runner.id}/jobs") expect(response).to have_gitlab_http_status(401) end @@ -523,7 +622,7 @@ describe API::Runners do describe 'POST /projects/:id/runners' do context 'authorized user' do - let(:specific_runner2) do + let(:project_runner2) do create(:ci_runner).tap do |runner| create(:ci_runner_project, runner: runner, project: project2) end @@ -531,23 +630,23 @@ describe API::Runners do it 'enables specific runner' do expect do - post api("/projects/#{project.id}/runners", user), runner_id: specific_runner2.id + post api("/projects/#{project.id}/runners", user), runner_id: project_runner2.id end.to change { project.runners.count }.by(+1) expect(response).to have_gitlab_http_status(201) end it 'avoids changes when enabling already enabled runner' do expect do - post api("/projects/#{project.id}/runners", user), runner_id: specific_runner.id + post api("/projects/#{project.id}/runners", user), runner_id: project_runner.id end.to change { project.runners.count }.by(0) expect(response).to have_gitlab_http_status(409) end it 'does not enable locked runner' do - specific_runner2.update(locked: true) + project_runner2.update(locked: true) expect do - post api("/projects/#{project.id}/runners", user), runner_id: specific_runner2.id + post api("/projects/#{project.id}/runners", user), runner_id: project_runner2.id end.to change { project.runners.count }.by(0) expect(response).to have_gitlab_http_status(403) @@ -562,7 +661,7 @@ describe API::Runners do context 'user is admin' do it 'enables any specific runner' do expect do - post api("/projects/#{project.id}/runners", admin), runner_id: unused_specific_runner.id + post api("/projects/#{project.id}/runners", admin), runner_id: unused_project_runner.id end.to change { project.runners.count }.by(+1) expect(response).to have_gitlab_http_status(201) end @@ -570,7 +669,7 @@ describe API::Runners do context 'user is not admin' do it 'does not enable runner without access to' do - post api("/projects/#{project.id}/runners", user), runner_id: unused_specific_runner.id + post api("/projects/#{project.id}/runners", user), runner_id: unused_project_runner.id expect(response).to have_gitlab_http_status(403) end @@ -619,7 +718,7 @@ describe API::Runners do context 'when runner have one associated projects' do it "does not disable project's runner" do expect do - delete api("/projects/#{project.id}/runners/#{specific_runner.id}", user) + delete api("/projects/#{project.id}/runners/#{project_runner.id}", user) end.to change { project.runners.count }.by(0) expect(response).to have_gitlab_http_status(403) end @@ -634,7 +733,7 @@ describe API::Runners do context 'authorized user without permissions' do it "does not disable project's runner" do - delete api("/projects/#{project.id}/runners/#{specific_runner.id}", user2) + delete api("/projects/#{project.id}/runners/#{project_runner.id}", user2) expect(response).to have_gitlab_http_status(403) end @@ -642,7 +741,7 @@ describe API::Runners do context 'unauthorized user' do it "does not disable project's runner" do - delete api("/projects/#{project.id}/runners/#{specific_runner.id}") + delete api("/projects/#{project.id}/runners/#{project_runner.id}") expect(response).to have_gitlab_http_status(401) end diff --git a/spec/requests/api/v3/runners_spec.rb b/spec/requests/api/v3/runners_spec.rb index c91b097a3c7..c9a05407857 100644 --- a/spec/requests/api/v3/runners_spec.rb +++ b/spec/requests/api/v3/runners_spec.rb @@ -8,10 +8,16 @@ describe API::V3::Runners do 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(:group2) { create(:group).tap { |group| group.add_owner(user) } } + + let!(:group_runner) { create(:ci_runner, description: 'Group runner', groups: [group]) } + let!(:two_groups_runner) { create(:ci_runner, description: 'Two groups runner', groups: [group, group2]) } + let!(:shared_runner) { create(:ci_runner, :shared) } let!(:unused_specific_runner) { create(:ci_runner) } - let!(:specific_runner) do + let!(:project_runner) do create(:ci_runner).tap do |runner| create(:ci_runner_project, runner: runner, project: project) end @@ -51,9 +57,17 @@ describe API::V3::Runners do end.to change { Ci::Runner.specific.count }.by(-1) end - it 'deletes used runner' do + it 'deletes used project runner' do expect do - delete v3_api("/runners/#{specific_runner.id}", admin) + delete v3_api("/runners/#{project_runner.id}", admin) + + expect(response).to have_http_status(200) + end.to change { Ci::Runner.specific.count }.by(-1) + end + + it 'deletes used group runner' do + expect do + delete v3_api("/runners/#{group_runner.id}", admin) expect(response).to have_gitlab_http_status(200) end.to change { Ci::Runner.specific.count }.by(-1) @@ -77,18 +91,31 @@ describe API::V3::Runners do context 'when runner is not shared' do it 'does not delete runner without access to it' do - delete v3_api("/runners/#{specific_runner.id}", user2) + delete v3_api("/runners/#{project_runner.id}", user2) expect(response).to have_gitlab_http_status(403) end - it 'does not delete runner with more than one associated project' do + it 'does not delete project runner with more than one associated project' do delete v3_api("/runners/#{two_projects_runner.id}", user) expect(response).to have_gitlab_http_status(403) end - it 'deletes runner for one owned project' do + it 'deletes project runner for one owned project' do expect do - delete v3_api("/runners/#{specific_runner.id}", user) + delete v3_api("/runners/#{group_runner.id}", user) + + expect(response).to have_http_status(200) + end.to change { Ci::Runner.specific.count }.by(-1) + end + + it 'does not delete group runner with more than one associated project' do + delete v3_api("/runners/#{two_groups_runner.id}", user) + expect(response).to have_http_status(403) + end + + it 'deletes group runner for one owned project' do + expect do + delete v3_api("/runners/#{project_runner.id}", user) expect(response).to have_gitlab_http_status(200) end.to change { Ci::Runner.specific.count }.by(-1) @@ -97,8 +124,14 @@ describe API::V3::Runners do end context 'unauthorized user' do - it 'does not delete runner' do - delete v3_api("/runners/#{specific_runner.id}") + it 'does not delete project runner' do + delete v3_api("/runners/#{project_runner.id}") + + expect(response).to have_http_status(401) + end + + it 'does not delete group runner' do + delete v3_api("/runners/#{group_runner.id}") expect(response).to have_gitlab_http_status(401) end @@ -120,7 +153,7 @@ describe API::V3::Runners do context 'when runner have one associated projects' do it "does not disable project's runner" do expect do - delete v3_api("/projects/#{project.id}/runners/#{specific_runner.id}", user) + delete v3_api("/projects/#{project.id}/runners/#{project_runner.id}", user) end.to change { project.runners.count }.by(0) expect(response).to have_gitlab_http_status(403) end @@ -135,7 +168,7 @@ describe API::V3::Runners do context 'authorized user without permissions' do it "does not disable project's runner" do - delete v3_api("/projects/#{project.id}/runners/#{specific_runner.id}", user2) + delete v3_api("/projects/#{project.id}/runners/#{project_runner.id}", user2) expect(response).to have_gitlab_http_status(403) end @@ -143,7 +176,7 @@ describe API::V3::Runners do context 'unauthorized user' do it "does not disable project's runner" do - delete v3_api("/projects/#{project.id}/runners/#{specific_runner.id}") + delete v3_api("/projects/#{project.id}/runners/#{project_runner.id}") expect(response).to have_gitlab_http_status(401) end