Automatic merge of gitlab-org/gitlab master

app/assets/javascripts/diffs/components/image_diff_overlay.vue

@@ -4,8 +4,8 @@ import { isArray } from 'lodash';
 import { mapActions, mapGetters } from 'vuex';
 import imageDiffMixin from 'ee_else_ce/diffs/mixins/image_diff';
 
-function calcPercent(pos, size, renderedSize) {
-  return (((pos / size) * 100) / ((renderedSize / size) * 100)) * 100;
+function calcPercent(pos, renderedSize) {
+  return (100 * pos) / renderedSize;
 }
 
 export default {
@@ -65,8 +65,8 @@ export default {
     ...mapActions('diffs', ['openDiffFileCommentForm']),
     getImageDimensions() {
       return {
-        width: this.$parent.width,
-        height: this.$parent.height,
+        width: Math.round(this.$parent.width),
+        height: Math.round(this.$parent.height),
       };
     },
     getPositionForObject(meta) {
@@ -87,15 +87,15 @@ export default {
     },
     clickedImage(x, y) {
       const { width, height } = this.getImageDimensions();
-      const xPercent = calcPercent(x, width, this.renderedWidth);
-      const yPercent = calcPercent(y, height, this.renderedHeight);
+      const xPercent = calcPercent(x, this.renderedWidth);
+      const yPercent = calcPercent(y, this.renderedHeight);
 
       this.openDiffFileCommentForm({
         fileHash: this.fileHash,
         width,
         height,
-        x: width * (xPercent / 100),
-        y: height * (yPercent / 100),
+        x: Math.round(width * (xPercent / 100)),
+        y: Math.round(height * (yPercent / 100)),
         xPercent,
         yPercent,
       });

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

doc/user/permissions.md

@@ -47,7 +47,7 @@ The following table lists project permissions available for each role:
 <!-- Keep this table sorted: By topic first, then by minimum role, then alphabetically. -->
 
 | Action                                                                                                                                                                                    | Guest    | Reporter | Developer | Maintainer | Owner |
-|-------------------------------------------------------------------------------------------------------------------------|----------|----------|-----------|------------|-------|
+|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------|----------|-----------|------------|-------|
 | [Analytics](analytics/index.md):<br>View issue analytics **(PREMIUM)**                                                                                                                    | ✓        | ✓        | ✓         | ✓          | ✓     |
 | [Analytics](analytics/index.md):<br>View [merge request analytics](analytics/merge_request_analytics.md) **(PREMIUM)**                                                                    | ✓        | ✓        | ✓         | ✓          | ✓     |
 | [Analytics](analytics/index.md):<br>View value stream analytics                                                                                                                           | ✓        | ✓        | ✓         | ✓          | ✓     |
@@ -73,7 +73,8 @@ The following table lists project permissions available for each role:
 | [CI/CD](../ci/index.md):<br>View a job with [debug logging](../ci/variables/index.md#debug-logging)                                                                                       |          |          | ✓         | ✓          | ✓     |
 | [CI/CD](../ci/index.md):<br>Manage CI/CD variables                                                                                                                                        |          |          |           | ✓          | ✓     |
 | [CI/CD](../ci/index.md):<br>Manage job triggers                                                                                                                                           |          |          |           | ✓          | ✓     |
-| [CI/CD](../ci/index.md):<br>Manage runners                                                                             |          |          |           | ✓          | ✓     |
+| [CI/CD](../ci/index.md):<br>Manage group runners                                                                                                                                          |          |          |           |            | ✓     |
+| [CI/CD](../ci/index.md):<br>Manage project runners                                                                                                                                        |          |          |           | ✓          | ✓     |
 | [CI/CD](../ci/index.md):<br>Run Web IDE's Interactive Web Terminals **(ULTIMATE ONLY)**                                                                                                   |          |          |           | ✓          | ✓     |
 | [CI/CD](../ci/index.md):<br>Use [environment terminals](../ci/environments/index.md#web-terminals-deprecated)                                                                             |          |          |           | ✓          | ✓     |
 | [CI/CD](../ci/index.md):<br>Delete pipelines                                                                                                                                              |          |          |           |            | ✓     |
@@ -94,7 +95,7 @@ The following table lists project permissions available for each role:
 | [Incident Management](../operations/incident_management/index.md):<br>Participate in on-call rotation                                                                                     | ✓| ✓ | ✓ | ✓          | ✓ |
 | [Incident Management](../operations/incident_management/index.md):<br>View [escalation policies](../operations/incident_management/escalation_policies.md)                                |  | ✓ | ✓ | ✓          | ✓ |
 | [Incident Management](../operations/incident_management/index.md):<br>Manage [on-call schedules](../operations/incident_management/oncall_schedules.md)                                   |  |   |   | ✓          | ✓ |
-| [Incident Management](../operations/incident_management/index.md):<br>Manage [escalation policies](../operations/incident_management/escalation_policies.md)|  |   |   | ✓ | ✓ |
+| [Incident Management](../operations/incident_management/index.md):<br>Manage [escalation policies](../operations/incident_management/escalation_policies.md)                              |  |   |   | ✓          | ✓ |
 | [Issues](project/issues/index.md):<br>Add Labels                                                                                                                                          | ✓ (*16*) | ✓        | ✓         | ✓          | ✓     |
 | [Issues](project/issues/index.md):<br>Assign                                                                                                                                              | ✓ (*16*) | ✓        | ✓         | ✓          | ✓     |
 | [Issues](project/issues/index.md):<br>Create                                                                                                                                              | ✓        | ✓        | ✓         | ✓          | ✓     |

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/frontend/diffs/components/image_diff_overlay_spec.js

@@ -6,8 +6,8 @@ import { imageDiffDiscussions } from '../mock_data/diff_discussions';
 
 describe('Diffs image diff overlay component', () => {
   const dimensions = {
-    width: 100,
-    height: 200,
+    width: 99.9,
+    height: 199.5,
   };
   let wrapper;
   let dispatch;
@@ -38,7 +38,6 @@ describe('Diffs image diff overlay component', () => {
 
   afterEach(() => {
     wrapper.destroy();
-    wrapper = null;
   });
 
   it('renders comment badges', () => {
@@ -81,17 +80,21 @@ describe('Diffs image diff overlay component', () => {
 
   it('dispatches openDiffFileCommentForm when clicking overlay', () => {
     createComponent({ canComment: true });
-    wrapper.find('.js-add-image-diff-note-button').trigger('click', { offsetX: 0, offsetY: 0 });
+    wrapper.find('.js-add-image-diff-note-button').trigger('click', { offsetX: 1.2, offsetY: 3.8 });
 
     expect(dispatch).toHaveBeenCalledWith('diffs/openDiffFileCommentForm', {
       fileHash: 'ABC',
-      x: 0,
-      y: 0,
+      x: 1,
+      y: 4,
       width: 100,
       height: 200,
-      xPercent: 0,
-      yPercent: 0,
+      xPercent: expect.any(Number),
+      yPercent: expect.any(Number),
     });
+
+    const { xPercent, yPercent } = dispatch.mock.calls[0][1];
+    expect(xPercent).toBeCloseTo(0.6);
+    expect(yPercent).toBeCloseTo(1.9);
   });
 
   describe('toggle discussion', () => {

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] }