From b41d75892e60e01fd3b2368e6286a9ecdc2febd1 Mon Sep 17 00:00:00 2001 From: Eugie Limpin Date: Wed, 22 Oct 2025 10:02:49 +0800 Subject: [PATCH 1/3] Remove Ci::Runner resource type handling from Authz::CustomAbility --- ee/app/models/authz/custom_ability.rb | 6 --- ee/app/policies/ee/ci/runner_policy.rb | 2 +- ee/spec/models/authz/custom_ability_spec.rb | 12 ----- .../regular/read_runners/request_spec.rb | 47 +++++++++++++++++++ 4 files changed, 48 insertions(+), 19 deletions(-) diff --git a/ee/app/models/authz/custom_ability.rb b/ee/app/models/authz/custom_ability.rb index c445ebadbe5996..15012a40d11772 100644 --- a/ee/app/models/authz/custom_ability.rb +++ b/ee/app/models/authz/custom_ability.rb @@ -68,12 +68,6 @@ def abilities_for abilities_for_projects([resource]).fetch(resource.id, []) when ::Group abilities_for_groups([resource]).fetch(resource.id, []) - when Ci::Runner - if resource.project_type? - abilities_for_projects(resource.projects) - else - abilities_for_groups(resource.groups) - end.flat_map { |(_id, abilities)| abilities } else [] end diff --git a/ee/app/policies/ee/ci/runner_policy.rb b/ee/app/policies/ee/ci/runner_policy.rb index 24159461f4e2cd..e9fba1e0ee063d 100644 --- a/ee/app/policies/ee/ci/runner_policy.rb +++ b/ee/app/policies/ee/ci/runner_policy.rb @@ -18,7 +18,7 @@ module RunnerPolicy end condition(:custom_role_enables_read_runners, score: 32) do - ::Authz::CustomAbility.allowed?(@user, :read_runners, @subject) + ::Authz::CustomAbility.allowed?(@user, :read_runners, @subject.owner) end rule { custom_role_enables_admin_runners }.policy do diff --git a/ee/spec/models/authz/custom_ability_spec.rb b/ee/spec/models/authz/custom_ability_spec.rb index 195bbb2367e843..25e52e84dae94b 100644 --- a/ee/spec/models/authz/custom_ability_spec.rb +++ b/ee/spec/models/authz/custom_ability_spec.rb @@ -33,8 +33,6 @@ let_it_be(:child_group) { create(:group, parent: group) } let_it_be(:project) { create(:project, group: group) } let_it_be(:child_project) { create(:project, group: child_group) } - let_it_be(:project_runner) { create(:ci_runner, :project, projects: [project]) } - let_it_be(:group_runner) { create(:ci_runner, :group, groups: [group]) } standard_abilities = Gitlab::CustomRoles::Definition.standard @@ -45,21 +43,16 @@ ref(:root_group) | name | ref(:child_group) | attrs[:group_ability] ref(:root_group) | name | ref(:project) | attrs[:project_ability] ref(:root_group) | name | ref(:child_project) | attrs[:project_ability] - ref(:root_group) | name | ref(:group_runner) | attrs[:group_ability] - ref(:root_group) | name | ref(:project_runner) | attrs[:project_ability] ref(:group) | name | ref(:group) | attrs[:group_ability] ref(:group) | name | ref(:child_group) | attrs[:group_ability] ref(:group) | name | ref(:project) | attrs[:project_ability] ref(:group) | name | ref(:child_project) | attrs[:project_ability] - ref(:group) | name | ref(:group_runner) | attrs[:group_ability] - ref(:group) | name | ref(:project_runner) | attrs[:project_ability] ref(:child_group) | name | ref(:child_group) | attrs[:group_ability] ref(:child_group) | name | ref(:child_project) | attrs[:project_ability] ref(:project) | name | ref(:project) | attrs[:project_ability] - ref(:project) | name | ref(:project_runner) | attrs[:project_ability] ref(:child_project) | name | ref(:child_project) | attrs[:project_ability] end @@ -105,8 +98,6 @@ it { is_expected.not_to be_allowed(user, ability, root_group) } it { is_expected.not_to be_allowed(user, ability, group) } it { is_expected.not_to be_allowed(user, ability, project) } - it { is_expected.not_to be_allowed(user, ability, group_runner) } - it { is_expected.not_to be_allowed(user, ability, project_runner) } end context 'with a membership on `project`' do @@ -116,7 +107,6 @@ it { is_expected.not_to be_allowed(user, ability, group) } it { is_expected.not_to be_allowed(user, ability, child_group) } it { is_expected.not_to be_allowed(user, ability, child_project) } - it { is_expected.not_to be_allowed(user, ability, group_runner) } end context 'with a membership on `child_project`' do @@ -128,8 +118,6 @@ it { is_expected.not_to be_allowed(user, ability, group) } it { is_expected.not_to be_allowed(user, ability, child_group) } it { is_expected.not_to be_allowed(user, ability, project) } - it { is_expected.not_to be_allowed(user, ability, group_runner) } - it { is_expected.not_to be_allowed(user, ability, project_runner) } end context 'with a user assigned to an admin custom role' do diff --git a/ee/spec/requests/custom_roles/regular/read_runners/request_spec.rb b/ee/spec/requests/custom_roles/regular/read_runners/request_spec.rb index 690948ecaa23a7..bef2094491bf44 100644 --- a/ee/spec/requests/custom_roles/regular/read_runners/request_spec.rb +++ b/ee/spec/requests/custom_roles/regular/read_runners/request_spec.rb @@ -67,6 +67,14 @@ expect(response).to redirect_to(project_settings_ci_cd_path(project, anchor: 'js-runners-settings')) end + it "#show" do + runner = create(:ci_runner, :project, projects: [project]) + + get project_runner_path(project, runner) + + expect(response).to have_gitlab_http_status(:ok) + end + it "#new" do get new_project_runner_path(project) @@ -103,4 +111,43 @@ expect(response).to have_gitlab_http_status(:not_found) end end + + describe API::Ci::Runners do + include ApiHelpers + + describe "GET /runners/:id" do + subject do + get api("/runners/#{runner.id}", user) + response + end + + context 'with a group runner' do + let_it_be(:runner) { create(:ci_runner, :group, groups: [project.group]) } + + it { is_expected.to have_gitlab_http_status(:forbidden) } + + context 'when user has the custom role' do + before do + create(:group_member, :guest, member_role: role, user: user, source: project.group) + end + + it { is_expected.to have_gitlab_http_status(:ok) } + end + end + + context 'with a project runner' do + let_it_be(:runner) { create(:ci_runner, :project, projects: [project]) } + + it { is_expected.to have_gitlab_http_status(:forbidden) } + + context 'when user has the custom role' do + before do + create(:project_member, :guest, member_role: role, user: user, source: project) + end + + it { is_expected.to have_gitlab_http_status(:ok) } + end + end + end + end end -- GitLab From bc203f1d7a9242d8f177f6f9ef075c2550df6a76 Mon Sep 17 00:00:00 2001 From: Eugie Limpin Date: Wed, 22 Oct 2025 10:31:14 +0800 Subject: [PATCH 2/3] Add spec to ensure user does not have access to other runners --- .../regular/read_runners/request_spec.rb | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/ee/spec/requests/custom_roles/regular/read_runners/request_spec.rb b/ee/spec/requests/custom_roles/regular/read_runners/request_spec.rb index bef2094491bf44..b22485400b6420 100644 --- a/ee/spec/requests/custom_roles/regular/read_runners/request_spec.rb +++ b/ee/spec/requests/custom_roles/regular/read_runners/request_spec.rb @@ -55,6 +55,7 @@ end describe Projects::RunnersController do + let_it_be(:runner) { create(:ci_runner, :project, projects: [project]) } let_it_be(:membership) { create(:project_member, :guest, member_role: role, user: user, source: project) } before do @@ -67,12 +68,22 @@ expect(response).to redirect_to(project_settings_ci_cd_path(project, anchor: 'js-runners-settings')) end - it "#show" do - runner = create(:ci_runner, :project, projects: [project]) + describe '#show' do + context 'with runner owned by project where user has the custom role' do + it 'returns 200' do + get project_runner_path(project, runner) - get project_runner_path(project, runner) + expect(response).to have_gitlab_http_status(:ok) + end + end - expect(response).to have_gitlab_http_status(:ok) + context 'with runner owned by another project' do + it 'returns 404' do + get project_runner_path(project, create(:ci_runner, :project, projects: [create(:project)])) + + expect(response).to have_gitlab_http_status(:not_found) + end + end end it "#new" do @@ -88,24 +99,18 @@ end it "#register" do - runner = create(:ci_runner, :project, projects: [project], registration_type: :authenticated_user) - get register_namespace_project_runner_path(group, project, runner) expect(response).to have_gitlab_http_status(:not_found) end it "#destroy" do - runner = create(:ci_runner, :project, projects: [project]) - delete project_runner_path(project, runner) expect(response).to have_gitlab_http_status(:not_found) end it "#pause" do - runner = create(:ci_runner, :project, projects: [project]) - post pause_project_runner_path(project, runner) expect(response).to have_gitlab_http_status(:not_found) -- GitLab From 8166eb53b6d0e37dde021e43f8144d552f3d92e6 Mon Sep 17 00:00:00 2001 From: Eugie Limpin Date: Wed, 22 Oct 2025 10:43:43 +0800 Subject: [PATCH 3/3] Check for presence of owner before using Authz::CustomAbility.allowed? --- ee/app/policies/ee/ci/runner_policy.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ee/app/policies/ee/ci/runner_policy.rb b/ee/app/policies/ee/ci/runner_policy.rb index e9fba1e0ee063d..eb03fac327de8e 100644 --- a/ee/app/policies/ee/ci/runner_policy.rb +++ b/ee/app/policies/ee/ci/runner_policy.rb @@ -18,7 +18,8 @@ module RunnerPolicy end condition(:custom_role_enables_read_runners, score: 32) do - ::Authz::CustomAbility.allowed?(@user, :read_runners, @subject.owner) + # Check for presence of owner to skip checks when runner is an instance runner + @subject.owner && ::Authz::CustomAbility.allowed?(@user, :read_runners, @subject.owner) end rule { custom_role_enables_admin_runners }.policy do -- GitLab