Merge branch 'security-master-mc-api-runner-owner-permissions' into 'master'

Return only runners for owned groups in User#ci_owned_runners

Closes #10

See merge request gitlab-org/security/gitlab!32

app/models/user.rb

@@ -1327,7 +1327,7 @@ class User < ApplicationRecord
         .select('ci_runners.*')
 
       group_runners = Ci::RunnerNamespace
-        .where(namespace_id: owned_or_maintainers_groups.select(:id))
+        .where(namespace_id: owned_groups.select(:id))
         .joins(:runner)
         .select('ci_runners.*')
 

changelogs/unreleased/security-master-mc-api-runner-owner-permissions.yml

+---
+title: Return only runners from groups where user is owner for user CI owned runners.
+merge_request:
+author:
+type: security

spec/models/user_spec.rb

@@ -2638,8 +2638,8 @@ describe User, :do_not_mock_admin_mode do
           add_user(:maintainer)
         end
 
-        it 'loads' do
-          expect(user.ci_owned_runners).to contain_exactly(runner)
+        it 'does not load' do
+          expect(user.ci_owned_runners).to be_empty
         end
       end
 
@@ -2654,6 +2654,20 @@ describe User, :do_not_mock_admin_mode do
       end
     end
 
+    shared_examples :group_member do
+      context 'when the user is owner' do
+        before do
+          add_user(:owner)
+        end
+
+        it 'loads' do
+          expect(user.ci_owned_runners).to contain_exactly(runner)
+        end
+      end
+
+      it_behaves_like :member
+    end
+
     context 'with groups projects runners' do
       let(:group) { create(:group) }
       let!(:project) { create(:project, group: group) }
@@ -2662,7 +2676,7 @@ describe User, :do_not_mock_admin_mode do
         group.add_user(user, access)
       end
 
-      it_behaves_like :member
+      it_behaves_like :group_member
     end
 
     context 'with groups runners' do
@@ -2673,14 +2687,14 @@ describe User, :do_not_mock_admin_mode do
         group.add_user(user, access)
       end
 
-      it_behaves_like :member
+      it_behaves_like :group_member
     end
 
     context 'with other projects runners' do
       let!(:project) { create(:project) }
 
       def add_user(access)
-        project.add_role(user, access)
+        project.add_user(user, access)
       end
 
       it_behaves_like :member
@@ -2698,7 +2712,7 @@ describe User, :do_not_mock_admin_mode do
         subgroup.add_user(another_user, :owner)
       end
 
-      it_behaves_like :member
+      it_behaves_like :group_member
     end
   end
 

spec/requests/api/runners_spec.rb

@@ -6,6 +6,7 @@ describe API::Runners do
   let(:admin) { create(:user, :admin) }
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
+  let(:group_maintainer) { create(:user) }
 
   let(:project) { create(:project, creator_id: user.id) }
   let(:project2) { create(:project, creator_id: user.id) }
@@ -20,6 +21,7 @@ describe API::Runners do
 
   before do
     # Set project access for users
+    create(:group_member, :maintainer, user: group_maintainer, group: group)
     create(:project_member, :maintainer, user: user, project: project)
     create(:project_member, :maintainer, user: user, project: project2)
     create(:project_member, :reporter, user: user2, project: project)
@@ -525,6 +527,20 @@ describe API::Runners do
           end.to change { Ci::Runner.project_type.count }.by(-1)
         end
 
+        it 'does not delete group runner with maintainer access' do
+          delete api("/runners/#{group_runner.id}", group_maintainer)
+
+          expect(response).to have_http_status(403)
+        end
+
+        it 'deletes group runner with owner access' do
+          expect do
+            delete api("/runners/#{group_runner.id}", user)
+
+            expect(response).to have_http_status(204)
+          end.to change { Ci::Runner.group_type.count }.by(-1)
+        end
+
         it_behaves_like '412 response' do
           let(:request) { api("/runners/#{project_runner.id}", user) }
         end