diff --git a/ee/app/models/authz/custom_ability.rb b/ee/app/models/authz/custom_ability.rb index c445ebadbe599613c3782ee7598cc2547bf33e26..15012a40d117721a0162148ad61a17993e303606 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 24159461f4e2cd53a056361b47ddb1124b5c3122..eb03fac327de8e991cd5fb6cfc46145e9dd5cb15 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) + # 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 diff --git a/ee/spec/models/authz/custom_ability_spec.rb b/ee/spec/models/authz/custom_ability_spec.rb index 195bbb2367e8435a8b3c16cb0c8024386f0594ac..25e52e84dae94b8986459ff1d743c6473e38386f 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 690948ecaa23a78f4e66f1874a03dd1d63334d89..b22485400b6420e592147750e6a7339c5664236a 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,6 +68,24 @@ expect(response).to redirect_to(project_settings_ci_cd_path(project, anchor: 'js-runners-settings')) end + 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) + + expect(response).to have_gitlab_http_status(:ok) + end + end + + 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 get new_project_runner_path(project) @@ -80,27 +99,60 @@ 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) 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