Merge branch 'pedropombeiro/19819/add-read_group_runners-policy-rule' into 'master'

Add read_group_runners group policy rule

See merge request gitlab-org/gitlab!77253

app/controllers/groups/application_controller.rb

@@ -37,6 +37,18 @@ class Groups::ApplicationController < ApplicationController
     end
   end
 
+  def authorize_admin_group_runners!
+    unless can?(current_user, :admin_group_runners, group)
+      render_404
+    end
+  end
+
+  def authorize_read_group_runners!
+    unless can?(current_user, :read_group_runners, group)
+      render_404
+    end
+  end
+
   def authorize_create_deploy_token!
     unless can?(current_user, :create_deploy_token, group)
       render_404

app/controllers/groups/runners_controller.rb

 # frozen_string_literal: true
 
 class Groups::RunnersController < Groups::ApplicationController
-  # TODO Proper policies, such as `read_group_runners, should be implemented per
-  # https://gitlab.com/gitlab-org/gitlab/-/issues/334802
-  before_action :authorize_admin_group!
+  before_action :authorize_read_group_runners!, only: [:index, :show]
+  before_action :authorize_admin_group_runners!, only: [:edit, :update, :destroy, :pause, :resume]
   before_action :runner_list_group_view_vue_ui_enabled, only: [:index]
   before_action :runner, only: [:edit, :update, :destroy, :pause, :resume, :show]
 
@@ -17,7 +16,7 @@ class Groups::RunnersController < Groups::ApplicationController
   end
 
   def runner_list_group_view_vue_ui_enabled
-    return render_404 unless Feature.enabled?(:runner_list_group_view_vue_ui, group, default_enabled: :yaml)
+    render_404 unless Feature.enabled?(:runner_list_group_view_vue_ui, group, default_enabled: :yaml)
   end
 
   def show

app/finders/ci/runners_finder.rb

@@ -47,7 +47,7 @@ module Ci
     end
 
     def group_runners
-      raise Gitlab::Access::AccessDeniedError unless can?(@current_user, :admin_group, @group)
+      raise Gitlab::Access::AccessDeniedError unless can?(@current_user, :read_group_runners, @group)
 
       @runners = case @params[:membership]
                  when :direct

app/policies/group_policy.rb

@@ -165,7 +165,6 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
     enable :destroy_package
     enable :create_projects
     enable :admin_pipeline
-    enable :admin_group_runners
     enable :admin_build
     enable :read_cluster
     enable :add_cluster
@@ -183,6 +182,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
     enable :admin_group_member
     enable :change_visibility_level
 
+    enable :read_group_runners
+    enable :admin_group_runners
+    enable :register_group_runners
+
     enable :set_note_created_at
     enable :set_emails_disabled
     enable :change_prevent_sharing_groups_outside_hierarchy
@@ -208,10 +211,6 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
     enable :read_nested_project_resources
   end
 
-  rule { can?(:admin_group_runners) }.policy do
-    enable :register_group_runners
-  end
-
   rule { owner }.enable :create_subgroup
   rule { maintainer & maintainer_can_create_group }.enable :create_subgroup
 

app/views/projects/runners/_group_runners.html.haml

@@ -29,9 +29,9 @@
 
   - if can?(current_user, :admin_group_runners, @project.group)
     - group_link = link_to _("group's CI/CD settings."), group_settings_ci_cd_path(@project.group)
-    = _('Group maintainers can register group runners in the %{link}').html_safe % { link: group_link }
+    = _('Group owners can register group runners in the %{link}').html_safe % { link: group_link }
   - else
-    = _('Ask your group maintainer to set up a group runner.')
+    = _('Ask your group owner to set up a group runner.')
 
 - else
   %h4.underlined-title

lib/sidebars/groups/menus/ci_cd_menu.rb

@@ -34,10 +34,8 @@ module Sidebars
           )
         end
 
-        # TODO Proper policies, such as `read_group_runners`, should be implemented per
-        # See https://gitlab.com/gitlab-org/gitlab/-/issues/334802
         def show_runners?
-          can?(context.current_user, :admin_group, context.group) &&
+          can?(context.current_user, :read_group_runners, context.group) &&
             Feature.enabled?(:runner_list_group_view_vue_ui, context.group, default_enabled: :yaml)
         end
       end

locale/gitlab.pot

@@ -4793,7 +4793,7 @@ msgstr ""
 msgid "Ask someone with write access to resolve it."
 msgstr ""
 
-msgid "Ask your group maintainer to set up a group runner."
+msgid "Ask your group owner to set up a group runner."
 msgstr ""
 
 msgid "Assertion consumer service URL"
@@ -16754,9 +16754,6 @@ msgstr ""
 msgid "Group jobs by"
 msgstr ""
 
-msgid "Group maintainers can register group runners in the %{link}"
-msgstr ""
-
 msgid "Group members"
 msgstr ""
 
@@ -16781,6 +16778,9 @@ msgstr ""
 msgid "Group overview content"
 msgstr ""
 
+msgid "Group owners can register group runners in the %{link}"
+msgstr ""
+
 msgid "Group path is already taken. We've suggested one that is available."
 msgstr ""
 

spec/features/runners_spec.rb

@@ -268,10 +268,27 @@ RSpec.describe 'Runners' do
         it 'group runners are not available' do
           visit project_runners_path(project)
 
+          expect(page).not_to have_content 'Group owners can register group runners in the group\'s CI/CD settings.'
+          expect(page).to have_content 'Ask your group owner to set up a group runner'
+        end
+      end
+    end
+
+    context 'as project maintainer and group owner' do
+      before do
+        group.add_owner(user)
+      end
+
+      context 'project with a group but no group runner' do
+        let(:project) { create :project, group: group }
+
+        it 'group runners are available' do
+          visit project_runners_path(project)
+
           expect(page).to have_content 'This group does not have any group runners yet.'
 
-          expect(page).to have_content 'Group maintainers can register group runners in the group\'s CI/CD settings.'
-          expect(page).not_to have_content 'Ask your group maintainer to set up a group runner'
+          expect(page).to have_content 'Group owners can register group runners in the group\'s CI/CD settings.'
+          expect(page).not_to have_content 'Ask your group owner to set up a group runner'
         end
       end
     end
@@ -296,8 +313,8 @@ RSpec.describe 'Runners' do
 
           expect(page).to have_content 'This group does not have any group runners yet.'
 
-          expect(page).not_to have_content 'Group maintainers can register group runners in the group\'s CI/CD settings.'
-          expect(page).to have_content 'Ask your group maintainer to set up a group runner.'
+          expect(page).not_to have_content 'Group owners can register group runners in the group\'s CI/CD settings.'
+          expect(page).to have_content 'Ask your group owner to set up a group runner.'
         end
       end
 

spec/policies/group_policy_spec.rb

@@ -36,6 +36,7 @@ RSpec.describe GroupPolicy do
     it { expect_disallowed(:read_crm_organization) }
     it { expect_disallowed(:read_crm_contact) }
     it { expect_disallowed(:read_counts) }
+    it { expect_disallowed(:read_group_runners) }
     it { expect_disallowed(*read_group_permissions) }
   end
 
@@ -51,6 +52,7 @@ RSpec.describe GroupPolicy do
     it { expect_disallowed(:read_crm_organization) }
     it { expect_disallowed(:read_crm_contact) }
     it { expect_disallowed(:read_counts) }
+    it { expect_disallowed(:read_group_runners) }
     it { expect_disallowed(*read_group_permissions) }
   end
 
@@ -1126,9 +1128,7 @@ RSpec.describe GroupPolicy do
     context 'with maintainer' do
       let(:current_user) { maintainer }
 
-      it { is_expected.to be_allowed(:register_group_runners) }
-
-      it_behaves_like 'expected outcome based on runner registration control'
+      it { is_expected.to be_disallowed(:register_group_runners) }
     end
 
     context 'with reporter' do

spec/support/shared_contexts/policies/group_policy_shared_context.rb

@@ -48,22 +48,24 @@ RSpec.shared_context 'GroupPolicy context' do
       destroy_package
       create_projects
       read_cluster create_cluster update_cluster admin_cluster add_cluster
-      admin_group_runners
     ]
   end
 
   let(:owner_permissions) do
-    [
-      :owner_access,
-      :admin_group,
-      :admin_namespace,
-      :admin_group_member,
-      :change_visibility_level,
-      :set_note_created_at,
-      :create_subgroup,
-      :read_statistics,
-      :update_default_branch_protection
-    ].compact
+    %i[
+      owner_access
+      admin_group
+      admin_namespace
+      admin_group_member
+      change_visibility_level
+      set_note_created_at
+      create_subgroup
+      read_statistics
+      update_default_branch_protection
+      read_group_runners
+      admin_group_runners
+      register_group_runners
+    ]
   end
 
   let(:admin_permissions) { %i[read_confidential_issues] }