From cecad1cb50bd2c5f73b8f612c175f9834d1fead0 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 3 May 2023 16:58:33 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-11-stable-ee --- .../set_runner_associated_projects_service.rb | 35 ++++--- spec/graphql/mutations/ci/runner/update_spec.rb | 108 +++++++++------------ .../set_runner_associated_projects_service_spec.rb | 77 ++++++++++----- 3 files changed, 118 insertions(+), 102 deletions(-) diff --git a/app/services/ci/runners/set_runner_associated_projects_service.rb b/app/services/ci/runners/set_runner_associated_projects_service.rb index 5e33fdae2f4..3608fdfac71 100644 --- a/app/services/ci/runners/set_runner_associated_projects_service.rb +++ b/app/services/ci/runners/set_runner_associated_projects_service.rb @@ -33,15 +33,9 @@ module Ci current_project_ids = runner.projects.ids # rubocop:enable CodeReuse/ActiveRecord - unless associate_new_projects(new_project_ids, current_project_ids) - response = ServiceResponse.error(message: 'failed to assign projects to runner') - raise ActiveRecord::Rollback, response.errors - end - - unless disassociate_old_projects(new_project_ids, current_project_ids) - response = ServiceResponse.error(message: 'failed to destroy runner project') - raise ActiveRecord::Rollback, response.errors - end + response = associate_new_projects(new_project_ids, current_project_ids) + response = disassociate_old_projects(new_project_ids, current_project_ids) if response.success? + raise ActiveRecord::Rollback, response.errors unless response.success? end response @@ -49,16 +43,29 @@ module Ci def associate_new_projects(new_project_ids, current_project_ids) missing_projects = Project.id_in(new_project_ids - current_project_ids) - missing_projects.all? { |project| runner.assign_to(project, current_user) } + + unless missing_projects.all? { |project| current_user.can?(:register_project_runners, project) } + return ServiceResponse.error(message: 'user is not authorized to add runners to project') + end + + unless missing_projects.all? { |project| runner.assign_to(project, current_user) } + return ServiceResponse.error(message: 'failed to assign projects to runner') + end + + ServiceResponse.success end def disassociate_old_projects(new_project_ids, current_project_ids) projects_to_be_deleted = current_project_ids - new_project_ids - return true if projects_to_be_deleted.empty? + return ServiceResponse.success if projects_to_be_deleted.empty? + + all_destroyed = + Ci::RunnerProject + .destroy_by(project_id: projects_to_be_deleted) + .all?(&:destroyed?) + return ServiceResponse.success if all_destroyed - Ci::RunnerProject - .destroy_by(project_id: projects_to_be_deleted) - .all?(&:destroyed?) + ServiceResponse.error(message: 'failed to destroy runner project') end attr_reader :runner, :current_user, :project_ids diff --git a/spec/graphql/mutations/ci/runner/update_spec.rb b/spec/graphql/mutations/ci/runner/update_spec.rb index e0c8219e0f6..50351321be8 100644 --- a/spec/graphql/mutations/ci/runner/update_spec.rb +++ b/spec/graphql/mutations/ci/runner/update_spec.rb @@ -46,10 +46,17 @@ RSpec.describe Mutations::Ci::Runner::Update, feature_category: :runner_fleet do end end - context 'when user can update runner', :enable_admin_mode do - let_it_be(:admin_user) { create(:user, :admin) } + context 'when user can update runner' do + let_it_be(:user) { create(:user) } - let(:current_ctx) { { current_user: admin_user } } + let(:original_projects) { [project1, project2] } + let(:projects_with_maintainer_access) { original_projects } + + let(:current_ctx) { { current_user: user } } + + before do + projects_with_maintainer_access.each { |project| project.add_maintainer(user) } + end context 'with valid arguments' do let(:mutation_params) do @@ -82,27 +89,22 @@ RSpec.describe Mutations::Ci::Runner::Update, feature_category: :runner_fleet do context 'with associatedProjects argument' do let_it_be(:project3) { create(:project) } + let_it_be(:project4) { create(:project) } + + let(:new_projects) { [project3, project4] } + let(:mutation_params) do + { + id: runner.to_global_id, + description: 'updated description', + associated_projects: new_projects.map { |project| project.to_global_id.to_s } + } + end context 'with id set to project runner' do - let(:mutation_params) do - { - id: runner.to_global_id, - description: 'updated description', - associated_projects: [project3.to_global_id.to_s] - } - end + let(:projects_with_maintainer_access) { original_projects + new_projects } it 'updates runner attributes and project relationships', :aggregate_failures do - expect_next_instance_of( - ::Ci::Runners::SetRunnerAssociatedProjectsService, - { - runner: runner, - current_user: admin_user, - project_ids: [project3.id] - } - ) do |service| - expect(service).to receive(:execute).and_call_original - end + setup_service_expectations expected_attributes = mutation_params.except(:id, :associated_projects) @@ -112,57 +114,32 @@ RSpec.describe Mutations::Ci::Runner::Update, feature_category: :runner_fleet do expect(response[:runner]).to be_an_instance_of(Ci::Runner) expect(response[:runner]).to have_attributes(expected_attributes) expect(runner.reload).to have_attributes(expected_attributes) - expect(runner.projects).to match_array([project1, project3]) + expect(runner.projects).to match_array([project1] + new_projects) end - context 'with user not allowed to assign runner' do - before do - allow(admin_user).to receive(:can?).with(:assign_runner, runner).and_return(false) - end + context 'with missing permissions on one of the new projects' do + let(:projects_with_maintainer_access) { original_projects + [project3] } it 'does not update runner', :aggregate_failures do - expect_next_instance_of( - ::Ci::Runners::SetRunnerAssociatedProjectsService, - { - runner: runner, - current_user: admin_user, - project_ids: [project3.id] - } - ) do |service| - expect(service).to receive(:execute).and_call_original - end + setup_service_expectations expected_attributes = mutation_params.except(:id, :associated_projects) response - expect(response[:errors]).to match_array(['user not allowed to assign runner']) + expect(response[:errors]).to match_array(['user is not authorized to add runners to project']) expect(response[:runner]).to be_nil expect(runner.reload).not_to have_attributes(expected_attributes) - expect(runner.projects).to match_array([project1, project2]) + expect(runner.projects).to match_array(original_projects) end end end context 'with an empty list of projects' do - let(:mutation_params) do - { - id: runner.to_global_id, - associated_projects: [] - } - end + let(:new_projects) { [] } it 'removes project relationships', :aggregate_failures do - expect_next_instance_of( - ::Ci::Runners::SetRunnerAssociatedProjectsService, - { - runner: runner, - current_user: admin_user, - project_ids: [] - } - ) do |service| - expect(service).to receive(:execute).and_call_original - end + setup_service_expectations response @@ -172,15 +149,9 @@ RSpec.describe Mutations::Ci::Runner::Update, feature_category: :runner_fleet do end end - context 'with id set to instance runner' do - let(:instance_runner) { create(:ci_runner, :instance) } - let(:mutation_params) do - { - id: instance_runner.to_global_id, - description: 'updated description', - associated_projects: [project2.to_global_id.to_s] - } - end + context 'with id set to instance runner', :enable_admin_mode do + let_it_be(:user) { create(:user, :admin) } + let_it_be(:runner) { create(:ci_runner, :instance) } it 'raises error', :aggregate_failures do expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ArgumentError) do @@ -188,6 +159,19 @@ RSpec.describe Mutations::Ci::Runner::Update, feature_category: :runner_fleet do end end end + + def setup_service_expectations + expect_next_instance_of( + ::Ci::Runners::SetRunnerAssociatedProjectsService, + { + runner: runner, + current_user: user, + project_ids: new_projects.map(&:id) + } + ) do |service| + expect(service).to receive(:execute).and_call_original + end + end end context 'with non-existing project ID in associatedProjects argument' do diff --git a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb index 9921f9322bd..d952fca25a5 100644 --- a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb +++ b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb @@ -3,17 +3,19 @@ require 'spec_helper' RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', feature_category: :runner_fleet do - subject(:execute) { described_class.new(runner: runner, current_user: user, project_ids: project_ids).execute } + subject(:execute) do + described_class.new(runner: runner, current_user: user, project_ids: new_projects.map(&:id)).execute + end let_it_be(:owner_project) { create(:project) } let_it_be(:project2) { create(:project) } - let_it_be(:original_projects) { [owner_project, project2] } + let(:original_projects) { [owner_project, project2] } let(:runner) { create(:ci_runner, :project, projects: original_projects) } context 'without user' do let(:user) { nil } - let(:project_ids) { [project2.id] } + let(:new_projects) { [project2] } it 'does not call assign_to on runner and returns error response', :aggregate_failures do expect(runner).not_to receive(:assign_to) @@ -24,8 +26,8 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', fe end context 'with unauthorized user' do - let(:user) { build(:user) } - let(:project_ids) { [project2.id] } + let(:user) { create(:user) } + let(:new_projects) { [project2] } it 'does not call assign_to on runner and returns error message' do expect(runner).not_to receive(:assign_to) @@ -35,15 +37,19 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', fe end end - context 'with admin user', :enable_admin_mode do - let_it_be(:user) { create(:user, :admin) } + context 'with authorized user' do + let_it_be(:project3) { create(:project) } + let_it_be(:project4) { create(:project) } + + let(:projects_with_maintainer_access) { original_projects } - let(:project3) { create(:project) } - let(:project4) { create(:project) } + before do + projects_with_maintainer_access.each { |project| project.add_maintainer(user) } + end - context 'with successful requests' do + shared_context 'with successful requests' do context 'when disassociating a project' do - let(:project_ids) { [project3.id, project4.id] } + let(:new_projects) { [project3, project4] } it 'reassigns associated projects and returns success response' do expect(execute).to be_success @@ -51,12 +57,12 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', fe runner.reload expect(runner.owner_project).to eq(owner_project) - expect(runner.projects.ids).to match_array([owner_project.id] + project_ids) + expect(runner.projects.ids).to match_array([owner_project.id] + new_projects.map(&:id)) end end context 'when disassociating no projects' do - let(:project_ids) { [project2.id, project3.id] } + let(:new_projects) { [project2, project3] } it 'reassigns associated projects and returns success response' do expect(execute).to be_success @@ -64,12 +70,12 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', fe runner.reload expect(runner.owner_project).to eq(owner_project) - expect(runner.projects.ids).to match_array([owner_project.id] + project_ids) + expect(runner.projects.ids).to match_array([owner_project.id] + new_projects.map(&:id)) end end context 'when disassociating all projects' do - let(:project_ids) { [] } + let(:new_projects) { [] } it 'reassigns associated projects and returns success response' do expect(execute).to be_success @@ -82,28 +88,47 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute', fe end end - context 'with failing assign_to requests' do - let(:project_ids) { [project3.id, project4.id] } + shared_context 'with failing destroy calls' do + let(:new_projects) { [project3, project4] } it 'returns error response and rolls back transaction' do - expect(runner).to receive(:assign_to).with(project4, user).once.and_return(false) + allow_next_found_instance_of(Ci::RunnerProject) do |runner_project| + allow(runner_project).to receive(:destroy).and_return(false) + end expect(execute).to be_error expect(runner.reload.projects).to eq(original_projects) end end - context 'with failing destroy calls' do - let(:project_ids) { [project3.id, project4.id] } + context 'with maintainer user' do + let(:user) { create(:user) } + let(:projects_with_maintainer_access) { original_projects + new_projects } - it 'returns error response and rolls back transaction' do - allow_next_found_instance_of(Ci::RunnerProject) do |runner_project| - allow(runner_project).to receive(:destroy).and_return(false) - end + it_behaves_like 'with successful requests' + it_behaves_like 'with failing destroy calls' - expect(execute).to be_error - expect(runner.reload.projects).to eq(original_projects) + context 'when associating new projects' do + let(:new_projects) { [project3, project4] } + + context 'with missing permissions on one of the new projects' do + let(:projects_with_maintainer_access) { original_projects + [project3] } + + it 'returns error response and rolls back transaction' do + expect(execute).to be_error + expect(execute.errors).to contain_exactly('user is not authorized to add runners to project') + expect(runner.reload.projects).to eq(original_projects) + end + end end end + + context 'with admin user', :enable_admin_mode do + let(:user) { create(:user, :admin) } + let(:projects_with_maintainer_access) { original_projects + new_projects } + + it_behaves_like 'with successful requests' + it_behaves_like 'with failing destroy calls' + end end end -- cgit v1.2.1 From 501a86395e6c0d8f93e033e9148db0799b76a16f Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Wed, 3 May 2023 18:30:07 +0000 Subject: Update VERSION files [ci skip] --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 3da231b3aea..5635cb5c26f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -15.11.1 \ No newline at end of file +15.11.2 \ No newline at end of file -- cgit v1.2.1 From f17ae0d7169f4fedd4c1557218371252d18e187d Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 3 May 2023 18:32:18 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@15-11-stable-ee --- CHANGELOG.md | 7 +++++++ GITALY_SERVER_VERSION | 2 +- GITLAB_PAGES_VERSION | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9dd6dde9ce3..e7bc2cee3b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 15.11.2 (2023-05-03) + +### Security (2 changes) + +- [Only maintainers of projects should be able to assign runners to them](gitlab-org/security/gitlab@c52abfffad2c06c2a49788e3db473f14923c3926) ([merge request](gitlab-org/security/gitlab!3234)) +- [Authorize access to vulnerabilitiesCountByDay resolver](gitlab-org/security/gitlab@8e78aecb9a6c248099a043f181de3c8f6d4417ce) + ## 15.11.1 (2023-05-01) ### Fixed (2 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 3da231b3aea..5635cb5c26f 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -15.11.1 \ No newline at end of file +15.11.2 \ No newline at end of file diff --git a/GITLAB_PAGES_VERSION b/GITLAB_PAGES_VERSION index 3da231b3aea..5635cb5c26f 100644 --- a/GITLAB_PAGES_VERSION +++ b/GITLAB_PAGES_VERSION @@ -1 +1 @@ -15.11.1 \ No newline at end of file +15.11.2 \ No newline at end of file -- cgit v1.2.1