diff options
| -rw-r--r-- | app/controllers/groups/runners_controller.rb | 6 | ||||
| -rw-r--r-- | changelogs/unreleased/security-group-runners-permissions.yml | 5 | ||||
| -rw-r--r-- | spec/controllers/groups/runners_controller_spec.rb | 205 |
3 files changed, 173 insertions, 43 deletions
diff --git a/app/controllers/groups/runners_controller.rb b/app/controllers/groups/runners_controller.rb index f8e32451b02..af2b2cbd1fd 100644 --- a/app/controllers/groups/runners_controller.rb +++ b/app/controllers/groups/runners_controller.rb @@ -3,7 +3,7 @@ class Groups::RunnersController < Groups::ApplicationController # Proper policies should be implemented per # https://gitlab.com/gitlab-org/gitlab-ce/issues/45894 - before_action :authorize_admin_pipeline! + before_action :authorize_admin_group! before_action :runner, only: [:edit, :update, :destroy, :pause, :resume, :show] @@ -50,10 +50,6 @@ class Groups::RunnersController < Groups::ApplicationController @runner ||= @group.runners.find(params[:id]) end - def authorize_admin_pipeline! - return render_404 unless can?(current_user, :admin_pipeline, group) - end - def runner_params params.require(:runner).permit(Ci::Runner::FORM_EDITABLE) end diff --git a/changelogs/unreleased/security-group-runners-permissions.yml b/changelogs/unreleased/security-group-runners-permissions.yml new file mode 100644 index 00000000000..6c74be30b6d --- /dev/null +++ b/changelogs/unreleased/security-group-runners-permissions.yml @@ -0,0 +1,5 @@ +--- +title: Use admin_group authorization in Groups::RunnersController +merge_request: +author: +type: security diff --git a/spec/controllers/groups/runners_controller_spec.rb b/spec/controllers/groups/runners_controller_spec.rb index 91f9e2c7832..14b0cf959b3 100644 --- a/spec/controllers/groups/runners_controller_spec.rb +++ b/spec/controllers/groups/runners_controller_spec.rb @@ -3,73 +3,202 @@ require 'spec_helper' describe Groups::RunnersController do - let(:user) { create(:user) } - let(:group) { create(:group) } + let(:user) { create(:user) } + let(:group) { create(:group) } let(:runner) { create(:ci_runner, :group, groups: [group]) } - - let(:params) do - { - group_id: group, - id: runner - } - end + let(:params) { { group_id: group, id: runner } } before do sign_in(user) - group.add_maintainer(user) + end + + describe '#show' do + context 'when user is owner' do + before do + group.add_owner(user) + end + + it 'renders show with 200 status code' do + get :show, params: { group_id: group, id: runner } + + expect(response).to have_gitlab_http_status(200) + expect(response).to render_template(:show) + end + end + + context 'when user is not owner' do + before do + group.add_maintainer(user) + end + + it 'renders a 404' do + get :show, params: { group_id: group, id: runner } + + expect(response).to have_gitlab_http_status(404) + end + end + end + + describe '#edit' do + context 'when user is owner' do + before do + group.add_owner(user) + end + + it 'renders show with 200 status code' do + get :edit, params: { group_id: group, id: runner } + + expect(response).to have_gitlab_http_status(200) + expect(response).to render_template(:edit) + end + end + + context 'when user is not owner' do + before do + group.add_maintainer(user) + end + + it 'renders a 404' do + get :edit, params: { group_id: group, id: runner } + + expect(response).to have_gitlab_http_status(404) + end + end end describe '#update' do - it 'updates the runner and ticks the queue' do - new_desc = runner.description.swapcase + context 'when user is an owner' do + before do + group.add_owner(user) + end - expect do - post :update, params: params.merge(runner: { description: new_desc } ) - end.to change { runner.ensure_runner_queue_value } + it 'updates the runner, ticks the queue, and redirects' do + new_desc = runner.description.swapcase - runner.reload + expect do + post :update, params: params.merge(runner: { description: new_desc } ) + end.to change { runner.ensure_runner_queue_value } - expect(response).to have_gitlab_http_status(302) - expect(runner.description).to eq(new_desc) + expect(response).to have_gitlab_http_status(302) + expect(runner.reload.description).to eq(new_desc) + end + end + + context 'when user is not an owner' do + before do + group.add_maintainer(user) + end + + it 'rejects the update and responds 404' do + old_desc = runner.description + + expect do + post :update, params: params.merge(runner: { description: old_desc.swapcase } ) + end.not_to change { runner.ensure_runner_queue_value } + + expect(response).to have_gitlab_http_status(404) + expect(runner.reload.description).to eq(old_desc) + end end end describe '#destroy' do - it 'destroys the runner' do - delete :destroy, params: params + context 'when user is an owner' do + before do + group.add_owner(user) + end + + it 'destroys the runner and redirects' do + delete :destroy, params: params + + expect(response).to have_gitlab_http_status(302) + expect(Ci::Runner.find_by(id: runner.id)).to be_nil + end + end + + context 'when user is not an owner' do + before do + group.add_maintainer(user) + end + + it 'responds 404 and does not destroy the runner' do + delete :destroy, params: params - expect(response).to have_gitlab_http_status(302) - expect(Ci::Runner.find_by(id: runner.id)).to be_nil + expect(response).to have_gitlab_http_status(404) + expect(Ci::Runner.find_by(id: runner.id)).to be_present + end end end describe '#resume' do - it 'marks the runner as active and ticks the queue' do - runner.update(active: false) + context 'when user is an owner' do + before do + group.add_owner(user) + end - expect do - post :resume, params: params - end.to change { runner.ensure_runner_queue_value } + it 'marks the runner as active, ticks the queue, and redirects' do + runner.update(active: false) - runner.reload + expect do + post :resume, params: params + end.to change { runner.ensure_runner_queue_value } - expect(response).to have_gitlab_http_status(302) - expect(runner.active).to eq(true) + expect(response).to have_gitlab_http_status(302) + expect(runner.reload.active).to eq(true) + end + end + + context 'when user is not an owner' do + before do + group.add_maintainer(user) + end + + it 'responds 404 and does not activate the runner' do + runner.update(active: false) + + expect do + post :resume, params: params + end.not_to change { runner.ensure_runner_queue_value } + + expect(response).to have_gitlab_http_status(404) + expect(runner.reload.active).to eq(false) + end end end describe '#pause' do - it 'marks the runner as inactive and ticks the queue' do - runner.update(active: true) + context 'when user is an owner' do + before do + group.add_owner(user) + end + + it 'marks the runner as inactive, ticks the queue, and redirects' do + runner.update(active: true) + + expect do + post :pause, params: params + end.to change { runner.ensure_runner_queue_value } + + expect(response).to have_gitlab_http_status(302) + expect(runner.reload.active).to eq(false) + end + end + + context 'when user is not an owner' do + before do + group.add_maintainer(user) + end - expect do - post :pause, params: params - end.to change { runner.ensure_runner_queue_value } + it 'responds 404 and does not update the runner or queue' do + runner.update(active: true) - runner.reload + expect do + post :pause, params: params + end.not_to change { runner.ensure_runner_queue_value } - expect(response).to have_gitlab_http_status(302) - expect(runner.active).to eq(false) + expect(response).to have_gitlab_http_status(404) + expect(runner.reload.active).to eq(true) + end end end end |