admin_group authorization for Groups::RunnersController

- Use authorize_admin_group! instead of authorize_admin_pipeline!
- Added role-based permission specs for Groups::RunnersController

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

changelogs/unreleased/security-group-runners-permissions.yml

+---
+title: Use admin_group authorization in Groups::RunnersController
+merge_request:
+author:
+type: security

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