diff options
Diffstat (limited to 'spec/services')
109 files changed, 2105 insertions, 767 deletions
diff --git a/spec/services/alert_management/alerts/update_service_spec.rb b/spec/services/alert_management/alerts/update_service_spec.rb index 4b47efca9ed..35697ac79a0 100644 --- a/spec/services/alert_management/alerts/update_service_spec.rb +++ b/spec/services/alert_management/alerts/update_service_spec.rb @@ -235,6 +235,59 @@ RSpec.describe AlertManagement::Alerts::UpdateService do it_behaves_like 'adds a system note' end + + context 'with an associated issue' do + let_it_be(:issue, reload: true) { create(:issue, project: project) } + + before do + alert.update!(issue: issue) + end + + shared_examples 'does not sync with the incident status' do + specify do + expect(::Issues::UpdateService).not_to receive(:new) + expect { response }.to change { alert.acknowledged? }.to(true) + end + end + + it_behaves_like 'does not sync with the incident status' + + context 'when the issue is an incident' do + before do + issue.update!(issue_type: Issue.issue_types[:incident]) + end + + it_behaves_like 'does not sync with the incident status' + + context 'when the incident has an escalation status' do + let_it_be(:escalation_status, reload: true) { create(:incident_management_issuable_escalation_status, issue: issue) } + + it 'updates the incident escalation status with the new alert status' do + expect(::Issues::UpdateService).to receive(:new).once.and_call_original + expect(described_class).to receive(:new).once.and_call_original + + expect { response }.to change { escalation_status.reload.acknowledged? }.to(true) + .and change { alert.reload.acknowledged? }.to(true) + end + + context 'when the statuses match' do + before do + escalation_status.update!(status_event: :acknowledge) + end + + it_behaves_like 'does not sync with the incident status' + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(incident_escalations: false) + end + + it_behaves_like 'does not sync with the incident status' + end + end + end + end end end end diff --git a/spec/services/audit_event_service_spec.rb b/spec/services/audit_event_service_spec.rb index ce7b43972da..0379fd3f05c 100644 --- a/spec/services/audit_event_service_spec.rb +++ b/spec/services/audit_event_service_spec.rb @@ -60,17 +60,18 @@ RSpec.describe AuditEventService do ip_address: user.current_sign_in_ip, result: AuthenticationEvent.results[:success], provider: 'standard' - ) + ).and_call_original audit_service.for_authentication.security_event end it 'tracks exceptions when the event cannot be created' do - allow(user).to receive_messages(current_sign_in_ip: 'invalid IP') + allow_next_instance_of(AuditEvent) do |event| + allow(event).to receive(:valid?).and_return(false) + end expect(Gitlab::ErrorTracking).to( - receive(:track_exception) - .with(ActiveRecord::RecordInvalid, audit_event_type: 'AuthenticationEvent').and_call_original + receive(:track_and_raise_for_dev_exception) ) audit_service.for_authentication.security_event @@ -93,7 +94,7 @@ RSpec.describe AuditEventService do end specify do - expect(AuthenticationEvent).to receive(:new).with(hash_including(ip_address: output)) + expect(AuthenticationEvent).to receive(:new).with(hash_including(ip_address: output)).and_call_original audit_service.for_authentication.security_event end diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index 46cc027fcb3..83f77780b80 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -92,6 +92,35 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'a modified token' end + + context 'with a project with a path with trailing underscore' do + let(:bad_project) { create(:project) } + + before do + bad_project.update!(path: bad_project.path + '_') + bad_project.add_developer(current_user) + end + + describe '#full_access_token' do + let(:token) { described_class.full_access_token(bad_project.full_path) } + let(:access) do + [{ 'type' => 'repository', + 'name' => bad_project.full_path, + 'actions' => ['*'], + 'migration_eligible' => false }] + end + + subject { { token: token } } + + it 'logs an exception and returns a valid access token' do + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception) + + expect(token).to be_present + expect(payload).to be_a(Hash) + expect(payload).to include('access' => access) + end + end + end end context 'when not in migration mode' do @@ -116,4 +145,28 @@ RSpec.describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'an unmodified token' end end + + context 'CDN redirection' do + include_context 'container registry auth service context' + + let_it_be(:current_user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:current_params) { { scopes: ["repository:#{project.full_path}:pull"] } } + + before do + project.add_developer(current_user) + end + + it_behaves_like 'a valid token' + it { expect(payload['access']).to include(include('cdn_redirect' => true)) } + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(container_registry_cdn_redirect: false) + end + + it_behaves_like 'a valid token' + it { expect(payload['access']).not_to include(have_key('cdn_redirect')) } + end + end end diff --git a/spec/services/branches/delete_merged_service_spec.rb b/spec/services/branches/delete_merged_service_spec.rb index 2cf0f53c8c3..46611670fe1 100644 --- a/spec/services/branches/delete_merged_service_spec.rb +++ b/spec/services/branches/delete_merged_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Branches::DeleteMergedService do include ProjectForksHelper - subject(:service) { described_class.new(project, project.owner) } + subject(:service) { described_class.new(project, project.first_owner) } let(:project) { create(:project, :repository) } diff --git a/spec/services/bulk_imports/archive_extraction_service_spec.rb b/spec/services/bulk_imports/archive_extraction_service_spec.rb index aa823d88010..da9df31cde9 100644 --- a/spec/services/bulk_imports/archive_extraction_service_spec.rb +++ b/spec/services/bulk_imports/archive_extraction_service_spec.rb @@ -34,9 +34,9 @@ RSpec.describe BulkImports::ArchiveExtractionService do context 'when dir is not in tmpdir' do it 'raises an error' do - ['/etc', '/usr', '/', '/home', '', '/some/other/path', Rails.root].each do |path| + ['/etc', '/usr', '/', '/home', '/some/other/path', Rails.root.to_s].each do |path| expect { described_class.new(tmpdir: path, filename: 'filename').execute } - .to raise_error(BulkImports::Error, 'Invalid target directory') + .to raise_error(StandardError, "path #{path} is not allowed") end end end @@ -52,7 +52,7 @@ RSpec.describe BulkImports::ArchiveExtractionService do context 'when filepath is being traversed' do it 'raises an error' do - expect { described_class.new(tmpdir: File.join(tmpdir, '../../../'), filename: 'name').execute } + expect { described_class.new(tmpdir: File.join(Dir.mktmpdir, 'test', '..'), filename: 'name').execute } .to raise_error(Gitlab::Utils::PathTraversalAttackError, 'Invalid path') end end diff --git a/spec/services/bulk_imports/file_decompression_service_spec.rb b/spec/services/bulk_imports/file_decompression_service_spec.rb index 4e8f78c8243..1d6aa79a37f 100644 --- a/spec/services/bulk_imports/file_decompression_service_spec.rb +++ b/spec/services/bulk_imports/file_decompression_service_spec.rb @@ -18,7 +18,7 @@ RSpec.describe BulkImports::FileDecompressionService do FileUtils.remove_entry(tmpdir) end - subject { described_class.new(dir: tmpdir, filename: gz_filename) } + subject { described_class.new(tmpdir: tmpdir, filename: gz_filename) } describe '#execute' do it 'decompresses specified file' do @@ -55,10 +55,18 @@ RSpec.describe BulkImports::FileDecompressionService do end context 'when dir is not in tmpdir' do - subject { described_class.new(dir: '/etc', filename: 'filename') } + subject { described_class.new(tmpdir: '/etc', filename: 'filename') } it 'raises an error' do - expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid target directory') + expect { subject.execute }.to raise_error(StandardError, 'path /etc is not allowed') + end + end + + context 'when path is being traversed' do + subject { described_class.new(tmpdir: File.join(Dir.mktmpdir, 'test', '..'), filename: 'filename') } + + it 'raises an error' do + expect { subject.execute }.to raise_error(Gitlab::Utils::PathTraversalAttackError, 'Invalid path') end end @@ -69,7 +77,7 @@ RSpec.describe BulkImports::FileDecompressionService do FileUtils.ln_s(File.join(tmpdir, gz_filename), symlink) end - subject { described_class.new(dir: tmpdir, filename: 'symlink.gz') } + subject { described_class.new(tmpdir: tmpdir, filename: 'symlink.gz') } it 'raises an error and removes the file' do expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid file') @@ -87,7 +95,7 @@ RSpec.describe BulkImports::FileDecompressionService do subject.instance_variable_set(:@decompressed_filepath, symlink) end - subject { described_class.new(dir: tmpdir, filename: gz_filename) } + subject { described_class.new(tmpdir: tmpdir, filename: gz_filename) } it 'raises an error and removes the file' do expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid file') diff --git a/spec/services/bulk_imports/file_download_service_spec.rb b/spec/services/bulk_imports/file_download_service_spec.rb index a24af9ae64d..bd664d6e996 100644 --- a/spec/services/bulk_imports/file_download_service_spec.rb +++ b/spec/services/bulk_imports/file_download_service_spec.rb @@ -33,7 +33,7 @@ RSpec.describe BulkImports::FileDownloadService do described_class.new( configuration: config, relative_url: '/test', - dir: tmpdir, + tmpdir: tmpdir, filename: filename, file_size_limit: file_size_limit, allowed_content_types: allowed_content_types @@ -72,7 +72,7 @@ RSpec.describe BulkImports::FileDownloadService do service = described_class.new( configuration: double, relative_url: '/test', - dir: tmpdir, + tmpdir: tmpdir, filename: filename, file_size_limit: file_size_limit, allowed_content_types: allowed_content_types @@ -157,7 +157,7 @@ RSpec.describe BulkImports::FileDownloadService do described_class.new( configuration: config, relative_url: '/test', - dir: tmpdir, + tmpdir: tmpdir, filename: 'symlink', file_size_limit: file_size_limit, allowed_content_types: allowed_content_types @@ -179,7 +179,7 @@ RSpec.describe BulkImports::FileDownloadService do described_class.new( configuration: config, relative_url: '/test', - dir: '/etc', + tmpdir: '/etc', filename: filename, file_size_limit: file_size_limit, allowed_content_types: allowed_content_types @@ -188,8 +188,28 @@ RSpec.describe BulkImports::FileDownloadService do it 'raises an error' do expect { subject.execute }.to raise_error( - described_class::ServiceError, - 'Invalid target directory' + StandardError, + 'path /etc is not allowed' + ) + end + end + + context 'when dir path is being traversed' do + subject do + described_class.new( + configuration: config, + relative_url: '/test', + tmpdir: File.join(Dir.mktmpdir('bulk_imports'), 'test', '..'), + filename: filename, + file_size_limit: file_size_limit, + allowed_content_types: allowed_content_types + ) + end + + it 'raises an error' do + expect { subject.execute }.to raise_error( + Gitlab::Utils::PathTraversalAttackError, + 'Invalid path' ) end end diff --git a/spec/services/bulk_imports/file_export_service_spec.rb b/spec/services/bulk_imports/file_export_service_spec.rb index 0d129c75384..94efceff6c6 100644 --- a/spec/services/bulk_imports/file_export_service_spec.rb +++ b/spec/services/bulk_imports/file_export_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe BulkImports::FileExportService do let_it_be(:project) { create(:project) } let_it_be(:export_path) { Dir.mktmpdir } - let_it_be(:relation) { 'uploads' } + let_it_be(:relation) { BulkImports::FileTransfer::BaseConfig::UPLOADS_RELATION } subject(:service) { described_class.new(project, export_path, relation) } @@ -20,6 +20,20 @@ RSpec.describe BulkImports::FileExportService do subject.execute end + context 'when relation is lfs objects' do + let_it_be(:relation) { BulkImports::FileTransfer::ProjectConfig::LFS_OBJECTS_RELATION } + + it 'executes lfs objects export service' do + expect_next_instance_of(BulkImports::LfsObjectsExportService) do |service| + expect(service).to receive(:execute) + end + + expect(subject).to receive(:tar_cf).with(archive: File.join(export_path, 'lfs_objects.tar'), dir: export_path) + + subject.execute + end + end + context 'when unsupported relation is passed' do it 'raises an error' do service = described_class.new(project, export_path, 'unsupported') diff --git a/spec/services/bulk_imports/lfs_objects_export_service_spec.rb b/spec/services/bulk_imports/lfs_objects_export_service_spec.rb new file mode 100644 index 00000000000..5ae54ed309b --- /dev/null +++ b/spec/services/bulk_imports/lfs_objects_export_service_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BulkImports::LfsObjectsExportService do + let_it_be(:project) { create(:project) } + let_it_be(:lfs_json_filename) { "#{BulkImports::FileTransfer::ProjectConfig::LFS_OBJECTS_RELATION}.json" } + let_it_be(:remote_url) { 'http://my-object-storage.local' } + + let(:export_path) { Dir.mktmpdir } + let(:lfs_object) { create(:lfs_object, :with_file) } + + subject(:service) { described_class.new(project, export_path) } + + before do + stub_lfs_object_storage + + %w(wiki design).each do |repository_type| + create( + :lfs_objects_project, + project: project, + repository_type: repository_type, + lfs_object: lfs_object + ) + end + + project.lfs_objects << lfs_object + end + + after do + FileUtils.remove_entry(export_path) if Dir.exist?(export_path) + end + + describe '#execute' do + it 'exports lfs objects and their repository types' do + filepath = File.join(export_path, lfs_json_filename) + + service.execute + + expect(File).to exist(File.join(export_path, lfs_object.oid)) + expect(File).to exist(filepath) + + lfs_json = Gitlab::Json.parse(File.read(filepath)) + + expect(lfs_json).to eq( + { + lfs_object.oid => [ + LfsObjectsProject.repository_types['wiki'], + LfsObjectsProject.repository_types['design'], + nil + ] + } + ) + end + + context 'when lfs object is remotely stored' do + let(:lfs_object) { create(:lfs_object, :object_storage) } + + it 'downloads lfs object from object storage' do + expect_next_instance_of(LfsObjectUploader) do |instance| + expect(instance).to receive(:url).and_return(remote_url) + end + + expect(subject).to receive(:download).with(remote_url, File.join(export_path, lfs_object.oid)) + + service.execute + end + end + end +end diff --git a/spec/services/chat_names/authorize_user_service_spec.rb b/spec/services/chat_names/authorize_user_service_spec.rb index b0bb741564d..53d90c7f100 100644 --- a/spec/services/chat_names/authorize_user_service_spec.rb +++ b/spec/services/chat_names/authorize_user_service_spec.rb @@ -4,10 +4,10 @@ require 'spec_helper' RSpec.describe ChatNames::AuthorizeUserService do describe '#execute' do - subject { described_class.new(service, params) } - + let(:integration) { create(:integration) } let(:result) { subject.execute } - let(:service) { create(:service) } + + subject { described_class.new(integration, params) } context 'when all parameters are valid' do let(:params) { { team_id: 'T0001', team_domain: 'myteam', user_id: 'U0001', user_name: 'user' } } diff --git a/spec/services/chat_names/find_user_service_spec.rb b/spec/services/chat_names/find_user_service_spec.rb index 9bbad09cd0d..4b0a1204558 100644 --- a/spec/services/chat_names/find_user_service_spec.rb +++ b/spec/services/chat_names/find_user_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe ChatNames::FindUserService, :clean_gitlab_redis_shared_state do describe '#execute' do - let(:integration) { create(:service) } + let(:integration) { create(:integration) } subject { described_class.new(integration, params).execute } diff --git a/spec/services/ci/after_requeue_job_service_spec.rb b/spec/services/ci/after_requeue_job_service_spec.rb index 2465bac7d10..d2acf3ad2f1 100644 --- a/spec/services/ci/after_requeue_job_service_spec.rb +++ b/spec/services/ci/after_requeue_job_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Ci::AfterRequeueJobService do let_it_be(:project) { create(:project) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:pipeline) { create(:ci_pipeline, project: project) } diff --git a/spec/services/ci/archive_trace_service_spec.rb b/spec/services/ci/archive_trace_service_spec.rb index b08ba6fd5e5..bf2e5302d2e 100644 --- a/spec/services/ci/archive_trace_service_spec.rb +++ b/spec/services/ci/archive_trace_service_spec.rb @@ -15,6 +15,25 @@ RSpec.describe Ci::ArchiveTraceService, '#execute' do expect(job.trace_metadata.trace_artifact).to eq(job.job_artifacts_trace) end + context 'integration hooks' do + it do + stub_feature_flags(datadog_integration_logs_collection: [job.project]) + + expect(job.project).to receive(:execute_integrations) do |data, hook_type| + expect(data).to eq Gitlab::DataBuilder::ArchiveTrace.build(job) + expect(hook_type).to eq :archive_trace_hooks + end + expect { subject }.not_to raise_error + end + + it 'with feature flag disabled' do + stub_feature_flags(datadog_integration_logs_collection: false) + + expect(job.project).not_to receive(:execute_integrations) + expect { subject }.not_to raise_error + end + end + context 'when trace is already archived' do let!(:job) { create(:ci_build, :success, :trace_artifact) } diff --git a/spec/services/ci/create_downstream_pipeline_service_spec.rb b/spec/services/ci/create_downstream_pipeline_service_spec.rb index 2237fd76d07..d61abf6a6ee 100644 --- a/spec/services/ci/create_downstream_pipeline_service_spec.rb +++ b/spec/services/ci/create_downstream_pipeline_service_spec.rb @@ -604,7 +604,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do context 'when configured with bridge job rules' do before do stub_ci_pipeline_yaml_file(config) - downstream_project.add_maintainer(upstream_project.owner) + downstream_project.add_maintainer(upstream_project.first_owner) end let(:config) do @@ -622,13 +622,13 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do end let(:primary_pipeline) do - Ci::CreatePipelineService.new(upstream_project, upstream_project.owner, { ref: 'master' }) + Ci::CreatePipelineService.new(upstream_project, upstream_project.first_owner, { ref: 'master' }) .execute(:push, save_on_errors: false) .payload end let(:bridge) { primary_pipeline.processables.find_by(name: 'bridge-job') } - let(:service) { described_class.new(upstream_project, upstream_project.owner) } + let(:service) { described_class.new(upstream_project, upstream_project.first_owner) } context 'that include the bridge job' do it 'creates the downstream pipeline' do diff --git a/spec/services/ci/create_pipeline_service/cache_spec.rb b/spec/services/ci/create_pipeline_service/cache_spec.rb index f5f162e4578..ca85a8cce64 100644 --- a/spec/services/ci/create_pipeline_service/cache_spec.rb +++ b/spec/services/ci/create_pipeline_service/cache_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do context 'cache' do let(:project) { create(:project, :custom_repo, files: files) } - let(:user) { project.owner } + let(:user) { project.first_owner } let(:ref) { 'refs/heads/master' } let(:source) { :push } let(:service) { described_class.new(project, user, { ref: ref }) } diff --git a/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb b/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb index c69c91593ae..e62a94b6df8 100644 --- a/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb +++ b/spec/services/ci/create_pipeline_service/creation_errors_and_warnings_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do describe 'creation errors and warnings' do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:ref) { 'refs/heads/master' } let(:source) { :push } diff --git a/spec/services/ci/create_pipeline_service/custom_config_content_spec.rb b/spec/services/ci/create_pipeline_service/custom_config_content_spec.rb index f150a4f8b51..a0cbf14d936 100644 --- a/spec/services/ci/create_pipeline_service/custom_config_content_spec.rb +++ b/spec/services/ci/create_pipeline_service/custom_config_content_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:ref) { 'refs/heads/master' } let(:service) { described_class.new(project, user, { ref: ref }) } diff --git a/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb b/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb index 026111d59f1..716a929830e 100644 --- a/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb +++ b/spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do describe '!reference tags' do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:ref) { 'refs/heads/master' } let(:source) { :push } diff --git a/spec/services/ci/create_pipeline_service/dry_run_spec.rb b/spec/services/ci/create_pipeline_service/dry_run_spec.rb index ae43c63b516..9a7e97fb12b 100644 --- a/spec/services/ci/create_pipeline_service/dry_run_spec.rb +++ b/spec/services/ci/create_pipeline_service/dry_run_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:ref) { 'refs/heads/master' } let(:service) { described_class.new(project, user, { ref: ref }) } diff --git a/spec/services/ci/create_pipeline_service/include_spec.rb b/spec/services/ci/create_pipeline_service/include_spec.rb index aa01977272a..3116801d50c 100644 --- a/spec/services/ci/create_pipeline_service/include_spec.rb +++ b/spec/services/ci/create_pipeline_service/include_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do context 'include:' do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:ref) { 'refs/heads/master' } let(:variables_attributes) { [{ key: 'MYVAR', secret_value: 'hello' }] } diff --git a/spec/services/ci/create_pipeline_service/logger_spec.rb b/spec/services/ci/create_pipeline_service/logger_spec.rb index dfe0859015d..53e5f0dd7f2 100644 --- a/spec/services/ci/create_pipeline_service/logger_spec.rb +++ b/spec/services/ci/create_pipeline_service/logger_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do context 'pipeline logger' do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:ref) { 'refs/heads/master' } let(:service) { described_class.new(project, user, { ref: ref }) } @@ -35,7 +35,10 @@ RSpec.describe Ci::CreatePipelineService do 'pipeline_creation_service_duration_s' => a_kind_of(Numeric), 'pipeline_creation_duration_s' => counters, 'pipeline_size_count' => counters, - 'pipeline_step_gitlab_ci_pipeline_chain_seed_duration_s' => counters + 'pipeline_step_gitlab_ci_pipeline_chain_seed_duration_s' => counters, + 'pipeline_seed_build_inclusion_duration_s' => counters, + 'pipeline_builds_tags_count' => a_kind_of(Numeric), + 'pipeline_builds_distinct_tags_count' => a_kind_of(Numeric) } end @@ -81,7 +84,6 @@ RSpec.describe Ci::CreatePipelineService do { 'pipeline_creation_caller' => 'Ci::CreatePipelineService', 'pipeline_source' => 'push', - 'pipeline_id' => nil, 'pipeline_persisted' => false, 'project_id' => project.id, 'pipeline_creation_service_duration_s' => a_kind_of(Numeric), diff --git a/spec/services/ci/create_pipeline_service/merge_requests_spec.rb b/spec/services/ci/create_pipeline_service/merge_requests_spec.rb index a1f85faa69f..de19ef363fb 100644 --- a/spec/services/ci/create_pipeline_service/merge_requests_spec.rb +++ b/spec/services/ci/create_pipeline_service/merge_requests_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do context 'merge requests handling' do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:ref) { 'refs/heads/feature' } let(:source) { :push } diff --git a/spec/services/ci/create_pipeline_service/needs_spec.rb b/spec/services/ci/create_pipeline_service/needs_spec.rb index 9070d86f7f6..abd17ccdd6a 100644 --- a/spec/services/ci/create_pipeline_service/needs_spec.rb +++ b/spec/services/ci/create_pipeline_service/needs_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do context 'needs' do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:ref) { 'refs/heads/master' } let(:source) { :push } diff --git a/spec/services/ci/create_pipeline_service/parallel_spec.rb b/spec/services/ci/create_pipeline_service/parallel_spec.rb index 6b455bf4874..ae28b74fef5 100644 --- a/spec/services/ci/create_pipeline_service/parallel_spec.rb +++ b/spec/services/ci/create_pipeline_service/parallel_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:service) { described_class.new(project, user, { ref: 'master' }) } let(:pipeline) { service.execute(:push).payload } diff --git a/spec/services/ci/create_pipeline_service/parameter_content_spec.rb b/spec/services/ci/create_pipeline_service/parameter_content_spec.rb index 761504ffb58..c28bc9d8c13 100644 --- a/spec/services/ci/create_pipeline_service/parameter_content_spec.rb +++ b/spec/services/ci/create_pipeline_service/parameter_content_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:service) { described_class.new(project, user, { ref: 'refs/heads/master' }) } let(:content) do diff --git a/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb b/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb index 5e34eeb99db..c6e69862422 100644 --- a/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb +++ b/spec/services/ci/create_pipeline_service/pre_post_stages_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do describe '.pre/.post stages' do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:source) { :push } let(:service) { described_class.new(project, user, { ref: ref }) } diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index d0915f099de..d0ce1c5aba8 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do let(:project) { create(:project, :repository) } - let(:user) { project.owner } + let(:user) { project.first_owner } let(:ref) { 'refs/heads/master' } let(:source) { :push } let(:service) { described_class.new(project, user, { ref: ref }) } diff --git a/spec/services/ci/create_pipeline_service/tags_spec.rb b/spec/services/ci/create_pipeline_service/tags_spec.rb index cbbeb870c5f..61c2415fa33 100644 --- a/spec/services/ci/create_pipeline_service/tags_spec.rb +++ b/spec/services/ci/create_pipeline_service/tags_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Ci::CreatePipelineService do describe 'tags:' do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let(:ref) { 'refs/heads/master' } let(:service) { described_class.new(project, user, { ref: ref }) } diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index ef879d536c3..a7026f5062e 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Ci::CreatePipelineService do include ProjectForksHelper let_it_be_with_refind(:project) { create(:project, :repository) } - let_it_be_with_reload(:user) { project.owner } + let_it_be_with_reload(:user) { project.first_owner } let(:ref_name) { 'refs/heads/master' } @@ -146,140 +146,22 @@ RSpec.describe Ci::CreatePipelineService do end context 'when merge requests already exist for this source branch' do - let(:merge_request_1) do + let!(:merge_request_1) do create(:merge_request, source_branch: 'feature', target_branch: "master", source_project: project) end - let(:merge_request_2) do + let!(:merge_request_2) do create(:merge_request, source_branch: 'feature', target_branch: "v1.1.0", source_project: project) end - context 'when related merge request is already merged' do - let!(:merged_merge_request) do - create(:merge_request, source_branch: 'master', target_branch: "branch_2", source_project: project, state: 'merged') - end - - it 'does not schedule update head pipeline job' do - expect(UpdateHeadPipelineForMergeRequestWorker).not_to receive(:perform_async).with(merged_merge_request.id) - - execute_service - end - end - context 'when the head pipeline sha equals merge request sha' do it 'updates head pipeline of each merge request', :sidekiq_might_not_need_inline do - merge_request_1 - merge_request_2 - head_pipeline = execute_service(ref: 'feature', after: nil).payload expect(merge_request_1.reload.head_pipeline).to eq(head_pipeline) expect(merge_request_2.reload.head_pipeline).to eq(head_pipeline) end end - - context 'when the head pipeline sha does not equal merge request sha' do - it 'does not update the head piepeline of MRs' do - merge_request_1 - merge_request_2 - - allow_any_instance_of(Ci::Pipeline).to receive(:latest?).and_return(true) - - expect { execute_service(after: 'ae73cb07c9eeaf35924a10f713b364d32b2dd34f') }.not_to raise_error - - last_pipeline = Ci::Pipeline.last - - expect(merge_request_1.reload.head_pipeline).not_to eq(last_pipeline) - expect(merge_request_2.reload.head_pipeline).not_to eq(last_pipeline) - end - end - - context 'when there is no pipeline for source branch' do - it "does not update merge request head pipeline" do - merge_request = create(:merge_request, source_branch: 'feature', - target_branch: "branch_1", - source_project: project) - - head_pipeline = execute_service.payload - - expect(merge_request.reload.head_pipeline).not_to eq(head_pipeline) - end - end - - context 'when merge request target project is different from source project' do - let!(:project) { fork_project(target_project, nil, repository: true) } - let!(:target_project) { create(:project, :repository) } - let!(:user) { create(:user) } - - before do - project.add_developer(user) - end - - it 'updates head pipeline for merge request', :sidekiq_might_not_need_inline do - merge_request = create(:merge_request, source_branch: 'feature', - target_branch: "master", - source_project: project, - target_project: target_project) - - head_pipeline = execute_service(ref: 'feature', after: nil).payload - - expect(merge_request.reload.head_pipeline).to eq(head_pipeline) - end - end - - context 'when the pipeline is not the latest for the branch' do - it 'does not update merge request head pipeline' do - merge_request = create(:merge_request, source_branch: 'master', - target_branch: "branch_1", - source_project: project) - - allow_any_instance_of(MergeRequest) - .to receive(:find_actual_head_pipeline) { } - - execute_service - - expect(merge_request.reload.head_pipeline).to be_nil - end - end - - context 'when pipeline has errors' do - before do - stub_ci_pipeline_yaml_file('some invalid syntax') - end - - it 'updates merge request head pipeline reference', :sidekiq_might_not_need_inline do - merge_request = create(:merge_request, source_branch: 'master', - target_branch: 'feature', - source_project: project) - - head_pipeline = execute_service.payload - - expect(head_pipeline).to be_persisted - expect(head_pipeline.yaml_errors).to be_present - expect(head_pipeline.messages).to be_present - expect(merge_request.reload.head_pipeline).to eq head_pipeline - end - end - - context 'when pipeline has been skipped' do - before do - allow_any_instance_of(Ci::Pipeline) - .to receive(:git_commit_message) - .and_return('some commit [ci skip]') - end - - it 'updates merge request head pipeline', :sidekiq_might_not_need_inline do - merge_request = create(:merge_request, source_branch: 'master', - target_branch: 'feature', - source_project: project) - - head_pipeline = execute_service.payload - - expect(head_pipeline).to be_skipped - expect(head_pipeline).to be_persisted - expect(merge_request.reload.head_pipeline).to eq head_pipeline - end - end end context 'auto-cancel enabled' do @@ -1655,7 +1537,7 @@ RSpec.describe Ci::CreatePipelineService do expect(pipeline.target_sha).to be_nil end - it 'schedules update for the head pipeline of the merge request' do + it 'schedules update for the head pipeline of the merge request', :sidekiq_inline do expect(UpdateHeadPipelineForMergeRequestWorker) .to receive(:perform_async).with(merge_request.id) diff --git a/spec/services/ci/destroy_pipeline_service_spec.rb b/spec/services/ci/destroy_pipeline_service_spec.rb index 6c1c02b2875..045051c7152 100644 --- a/spec/services/ci/destroy_pipeline_service_spec.rb +++ b/spec/services/ci/destroy_pipeline_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe ::Ci::DestroyPipelineService do subject { described_class.new(project, user).execute(pipeline) } context 'user is owner' do - let(:user) { project.owner } + let(:user) { project.first_owner } it 'destroys the pipeline' do subject @@ -66,6 +66,28 @@ RSpec.describe ::Ci::DestroyPipelineService do expect { subject }.to change { Ci::DeletedObject.count } end end + + context 'when job has trace chunks' do + let(:connection_params) { Gitlab.config.artifacts.object_store.connection.symbolize_keys } + let(:connection) { ::Fog::Storage.new(connection_params) } + + before do + stub_object_storage(connection_params: connection_params, remote_directory: 'artifacts') + stub_artifacts_object_storage + end + + let!(:trace_chunk) { create(:ci_build_trace_chunk, :fog_with_data, build: build) } + + it 'destroys associated trace chunks' do + subject + + expect { trace_chunk.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'removes data from object store' do + expect { subject }.to change { Ci::BuildTraceChunks::Fog.new.data(trace_chunk) } + end + end end context 'when pipeline is in cancelable state' do diff --git a/spec/services/ci/job_artifacts/delete_project_artifacts_service_spec.rb b/spec/services/ci/job_artifacts/delete_project_artifacts_service_spec.rb new file mode 100644 index 00000000000..74fa42962f3 --- /dev/null +++ b/spec/services/ci/job_artifacts/delete_project_artifacts_service_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::JobArtifacts::DeleteProjectArtifactsService do + let_it_be(:project) { create(:project) } + + subject { described_class.new(project: project) } + + describe '#execute' do + it 'enqueues a Ci::ExpireProjectBuildArtifactsWorker' do + expect(Ci::JobArtifacts::ExpireProjectBuildArtifactsWorker).to receive(:perform_async).with(project.id).and_call_original + + subject.execute + end + end +end diff --git a/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb b/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb index e71f1a4266a..e95a449d615 100644 --- a/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb +++ b/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb @@ -20,7 +20,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s context 'with preloaded relationships' do before do - stub_const("#{described_class}::LOOP_LIMIT", 1) + stub_const("#{described_class}::LARGE_LOOP_LIMIT", 1) end context 'with ci_destroy_unlocked_job_artifacts feature flag disabled' do @@ -53,46 +53,6 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s log = ActiveRecord::QueryRecorder.new { subject } expect(log.count).to be_within(1).of(8) end - - context 'with several locked-unknown artifact records' do - before do - stub_const("#{described_class}::LOOP_LIMIT", 10) - stub_const("#{described_class}::BATCH_SIZE", 2) - end - - let!(:lockable_artifact_records) do - [ - create(:ci_job_artifact, :metadata, :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: locked_job), - create(:ci_job_artifact, :junit, :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: locked_job), - create(:ci_job_artifact, :sast, :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: locked_job), - create(:ci_job_artifact, :cobertura, :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: locked_job), - create(:ci_job_artifact, :trace, :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: locked_job) - ] - end - - let!(:unlockable_artifact_records) do - [ - create(:ci_job_artifact, :metadata, :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: job), - create(:ci_job_artifact, :junit, :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: job), - create(:ci_job_artifact, :sast, :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: job), - create(:ci_job_artifact, :cobertura, :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: job), - create(:ci_job_artifact, :trace, :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: job), - artifact - ] - end - - it 'updates the locked status of job artifacts from artifacts-locked pipelines' do - subject - - expect(lockable_artifact_records).to be_all(&:persisted?) - expect(lockable_artifact_records).to be_all { |artifact| artifact.reload.artifact_artifacts_locked? } - end - - it 'unlocks and then destroys job artifacts from artifacts-unlocked pipelines' do - expect { subject }.to change { Ci::JobArtifact.count }.by(-6) - expect(Ci::JobArtifact.where(id: unlockable_artifact_records.map(&:id))).to be_empty - end - end end end @@ -159,7 +119,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s let!(:artifact) { create(:ci_job_artifact, :expired, job: job, locked: job.pipeline.locked) } before do - stub_const("#{described_class}::LOOP_LIMIT", 10) + stub_const("#{described_class}::LARGE_LOOP_LIMIT", 10) end context 'when the import fails' do @@ -229,7 +189,8 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s context 'when loop reached loop limit' do before do - stub_const("#{described_class}::LOOP_LIMIT", 1) + stub_feature_flags(ci_artifact_fast_removal_large_loop_limit: false) + stub_const("#{described_class}::SMALL_LOOP_LIMIT", 1) end it 'destroys one artifact' do diff --git a/spec/services/ci/job_artifacts/expire_project_build_artifacts_service_spec.rb b/spec/services/ci/job_artifacts/expire_project_build_artifacts_service_spec.rb new file mode 100644 index 00000000000..fb9dd6b876b --- /dev/null +++ b/spec/services/ci/job_artifacts/expire_project_build_artifacts_service_spec.rb @@ -0,0 +1,157 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::JobArtifacts::ExpireProjectBuildArtifactsService do + let_it_be(:project) { create(:project) } + let_it_be(:pipeline, reload: true) { create(:ci_pipeline, :unlocked, project: project) } + + let(:expiry_time) { Time.current } + + RSpec::Matchers.define :have_locked_status do |expected_status| + match do |job_artifacts| + predicate = "#{expected_status}?".to_sym + job_artifacts.all? { |artifact| artifact.__send__(predicate) } + end + end + + RSpec::Matchers.define :expire_at do |expected_expiry| + match do |job_artifacts| + job_artifacts.all? { |artifact| artifact.expire_at.to_i == expected_expiry.to_i } + end + end + + RSpec::Matchers.define :have_no_expiry do + match do |job_artifacts| + job_artifacts.all? { |artifact| artifact.expire_at.nil? } + end + end + + describe '#execute' do + subject(:execute) { described_class.new(project.id, expiry_time).execute } + + context 'with job containing erasable artifacts' do + let_it_be(:job, reload: true) { create(:ci_build, :erasable, pipeline: pipeline) } + + it 'unlocks erasable job artifacts' do + execute + + expect(job.job_artifacts).to have_locked_status(:artifact_unlocked) + end + + it 'expires erasable job artifacts' do + execute + + expect(job.job_artifacts).to expire_at(expiry_time) + end + end + + context 'with job containing trace artifacts' do + let_it_be(:job, reload: true) { create(:ci_build, :trace_artifact, pipeline: pipeline) } + + it 'does not unlock trace artifacts' do + execute + + expect(job.job_artifacts).to have_locked_status(:artifact_unknown) + end + + it 'does not expire trace artifacts' do + execute + + expect(job.job_artifacts).to have_no_expiry + end + end + + context 'with job from artifact locked pipeline' do + let_it_be(:job, reload: true) { create(:ci_build, pipeline: pipeline) } + let_it_be(:locked_artifact, reload: true) { create(:ci_job_artifact, :locked, job: job) } + + before do + pipeline.artifacts_locked! + end + + it 'does not unlock locked artifacts' do + execute + + expect(job.job_artifacts).to have_locked_status(:artifact_artifacts_locked) + end + + it 'does not expire locked artifacts' do + execute + + expect(job.job_artifacts).to have_no_expiry + end + end + + context 'with job containing both erasable and trace artifacts' do + let_it_be(:job, reload: true) { create(:ci_build, pipeline: pipeline) } + let_it_be(:erasable_artifact, reload: true) { create(:ci_job_artifact, :archive, job: job) } + let_it_be(:trace_artifact, reload: true) { create(:ci_job_artifact, :trace, job: job) } + + it 'unlocks erasable artifacts' do + execute + + expect(erasable_artifact.artifact_unlocked?).to be_truthy + end + + it 'expires erasable artifacts' do + execute + + expect(erasable_artifact.expire_at.to_i).to eq(expiry_time.to_i) + end + + it 'does not unlock trace artifacts' do + execute + + expect(trace_artifact.artifact_unlocked?).to be_falsey + end + + it 'does not expire trace artifacts' do + execute + + expect(trace_artifact.expire_at).to be_nil + end + end + + context 'with multiple pipelines' do + let_it_be(:job, reload: true) { create(:ci_build, :erasable, pipeline: pipeline) } + + let_it_be(:pipeline2, reload: true) { create(:ci_pipeline, :unlocked, project: project) } + let_it_be(:job2, reload: true) { create(:ci_build, :erasable, pipeline: pipeline) } + + it 'unlocks artifacts across pipelines' do + execute + + expect(job.job_artifacts).to have_locked_status(:artifact_unlocked) + expect(job2.job_artifacts).to have_locked_status(:artifact_unlocked) + end + + it 'expires artifacts across pipelines' do + execute + + expect(job.job_artifacts).to expire_at(expiry_time) + expect(job2.job_artifacts).to expire_at(expiry_time) + end + end + + context 'with artifacts belonging to another project' do + let_it_be(:job, reload: true) { create(:ci_build, :erasable, pipeline: pipeline) } + + let_it_be(:another_project, reload: true) { create(:project) } + let_it_be(:another_pipeline, reload: true) { create(:ci_pipeline, project: another_project) } + let_it_be(:another_job, reload: true) { create(:ci_build, :erasable, pipeline: another_pipeline) } + + it 'does not unlock erasable artifacts in other projects' do + execute + + expect(another_job.job_artifacts).to have_locked_status(:artifact_unknown) + end + + it 'does not expire erasable artifacts in other projects' do + execute + + expect(another_job.job_artifacts).to have_no_expiry + end + end + end +end diff --git a/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb b/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb index 31e1b0a896d..26bc6f747e1 100644 --- a/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb +++ b/spec/services/ci/pipeline_processing/atomic_processing_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do describe 'Pipeline Processing Service Tests With Yaml' do let_it_be(:project) { create(:project, :repository) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } where(:test_file_path) do Dir.glob(Rails.root.join('spec/services/ci/pipeline_processing/test_cases/*.yml')) @@ -65,7 +65,7 @@ RSpec.describe Ci::PipelineProcessing::AtomicProcessingService do describe 'Pipeline Processing Service' do let(:project) { create(:project, :repository) } - let(:user) { project.owner } + let(:user) { project.first_owner } let(:pipeline) do create(:ci_empty_pipeline, ref: 'master', project: project) diff --git a/spec/services/ci/pipelines/add_job_service_spec.rb b/spec/services/ci/pipelines/add_job_service_spec.rb index 709a840c644..560724a1c6a 100644 --- a/spec/services/ci/pipelines/add_job_service_spec.rb +++ b/spec/services/ci/pipelines/add_job_service_spec.rb @@ -31,7 +31,7 @@ RSpec.describe Ci::Pipelines::AddJobService do execute end.to change { job.slice(:pipeline, :project, :ref) }.to( pipeline: pipeline, project: pipeline.project, ref: pipeline.ref - ) + ).and change { job.metadata.project }.to(pipeline.project) end it 'returns a service response with the job as payload' do diff --git a/spec/services/ci/play_build_service_spec.rb b/spec/services/ci/play_build_service_spec.rb index 34f77260334..85ef8b60af4 100644 --- a/spec/services/ci/play_build_service_spec.rb +++ b/spec/services/ci/play_build_service_spec.rb @@ -122,7 +122,7 @@ RSpec.describe Ci::PlayBuildService, '#execute' do end context 'when build is not a playable manual action' do - let(:build) { create(:ci_build, when: :manual, pipeline: pipeline) } + let(:build) { create(:ci_build, :success, pipeline: pipeline) } let!(:branch) { create(:protected_branch, :developers_can_merge, name: build.ref, project: project) } it 'duplicates the build' do @@ -138,6 +138,18 @@ RSpec.describe Ci::PlayBuildService, '#execute' do expect(build.user).not_to eq user expect(duplicate.user).to eq user end + + context 'and is not retryable' do + let(:build) { create(:ci_build, :deployment_rejected, pipeline: pipeline) } + + it 'does not duplicate the build' do + expect { service.execute(build) }.not_to change { Ci::Build.count } + end + + it 'does not enqueue the build' do + expect { service.execute(build) }.not_to change { build.status } + end + end end context 'when build is not action' do diff --git a/spec/services/ci/process_sync_events_service_spec.rb b/spec/services/ci/process_sync_events_service_spec.rb index 00b670ff54f..8b7717fe4bf 100644 --- a/spec/services/ci/process_sync_events_service_spec.rb +++ b/spec/services/ci/process_sync_events_service_spec.rb @@ -28,10 +28,10 @@ RSpec.describe Ci::ProcessSyncEventsService do it 'consumes events' do expect { execute }.to change(Projects::SyncEvent, :count).from(2).to(0) - expect(project1.ci_project_mirror).to have_attributes( + expect(project1.reload.ci_project_mirror).to have_attributes( namespace_id: parent_group_1.id ) - expect(project2.ci_project_mirror).to have_attributes( + expect(project2.reload.ci_project_mirror).to have_attributes( namespace_id: parent_group_2.id ) end @@ -71,6 +71,24 @@ RSpec.describe Ci::ProcessSyncEventsService do expect { execute }.not_to change(Projects::SyncEvent, :count) end end + + it 'does not delete non-executed events' do + new_project = create(:project) + sync_event_class.delete_all + + project1.update!(group: parent_group_2) + new_project.update!(group: parent_group_1) + project2.update!(group: parent_group_1) + + new_project_sync_event = new_project.sync_events.last + + allow(sync_event_class).to receive(:preload_synced_relation).and_return( + sync_event_class.where.not(id: new_project_sync_event) + ) + + expect { execute }.to change(Projects::SyncEvent, :count).from(3).to(1) + expect(new_project_sync_event.reload).to be_persisted + end end context 'for Namespaces::SyncEvent' do @@ -88,10 +106,10 @@ RSpec.describe Ci::ProcessSyncEventsService do it 'consumes events' do expect { execute }.to change(Namespaces::SyncEvent, :count).from(2).to(0) - expect(group.ci_namespace_mirror).to have_attributes( + expect(group.reload.ci_namespace_mirror).to have_attributes( traversal_ids: [parent_group_1.id, parent_group_2.id, group.id] ) - expect(parent_group_2.ci_namespace_mirror).to have_attributes( + expect(parent_group_2.reload.ci_namespace_mirror).to have_attributes( traversal_ids: [parent_group_1.id, parent_group_2.id] ) end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 866015aa523..251159864f5 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -827,11 +827,17 @@ module Ci end context 'when project already has running jobs' do - let!(:build2) { create(:ci_build, :running, pipeline: pipeline, runner: shared_runner) } - let!(:build3) { create(:ci_build, :running, pipeline: pipeline, runner: shared_runner) } + let(:build2) { create(:ci_build, :running, pipeline: pipeline, runner: shared_runner) } + let(:build3) { create(:ci_build, :running, pipeline: pipeline, runner: shared_runner) } + + before do + ::Ci::RunningBuild.upsert_shared_runner_build!(build2) + ::Ci::RunningBuild.upsert_shared_runner_build!(build3) + end it 'counts job queuing time histogram with expected labels' do allow(attempt_counter).to receive(:increment) + expect(job_queue_duration_seconds).to receive(:observe) .with({ shared_runner: expected_shared_runner, jobs_running_for_project: expected_jobs_running_for_project_third_job, @@ -845,6 +851,14 @@ module Ci shared_examples 'metrics collector' do it_behaves_like 'attempt counter collector' it_behaves_like 'jobs queueing time histogram collector' + + context 'when using denormalized data is disabled' do + before do + stub_feature_flags(ci_pending_builds_maintain_denormalized_data: false) + end + + it_behaves_like 'jobs queueing time histogram collector' + end end context 'when shared runner is used' do @@ -875,6 +889,16 @@ module Ci it_behaves_like 'metrics collector' end + context 'when max running jobs bucket size is exceeded' do + before do + stub_const('Gitlab::Ci::Queue::Metrics::JOBS_RUNNING_FOR_PROJECT_MAX_BUCKET', 1) + end + + let(:expected_jobs_running_for_project_third_job) { '1+' } + + it_behaves_like 'metrics collector' + end + context 'when pending job with queued_at=nil is used' do before do pending_job.update!(queued_at: nil) diff --git a/spec/services/ci/register_runner_service_spec.rb b/spec/services/ci/register_runner_service_spec.rb new file mode 100644 index 00000000000..e813a1d8b31 --- /dev/null +++ b/spec/services/ci/register_runner_service_spec.rb @@ -0,0 +1,226 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::RegisterRunnerService do + let(:registration_token) { 'abcdefg123456' } + + before do + stub_feature_flags(runner_registration_control: false) + stub_application_setting(runners_registration_token: registration_token) + stub_application_setting(valid_runner_registrars: ApplicationSetting::VALID_RUNNER_REGISTRAR_TYPES) + end + + describe '#execute' do + let(:token) { } + let(:args) { {} } + + subject { described_class.new.execute(token, args) } + + context 'when no token is provided' do + let(:token) { '' } + + it 'returns nil' do + is_expected.to be_nil + end + end + + context 'when invalid token is provided' do + let(:token) { 'invalid' } + + it 'returns nil' do + is_expected.to be_nil + end + end + + context 'when valid token is provided' do + context 'with a registration token' do + let(:token) { registration_token } + + it 'creates runner with default values' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.persisted?).to be_truthy + expect(subject.run_untagged).to be true + expect(subject.active).to be true + expect(subject.token).not_to eq(registration_token) + expect(subject).to be_instance_type + end + + context 'with non-default arguments' do + let(:args) do + { + description: 'some description', + active: false, + locked: true, + run_untagged: false, + tag_list: %w(tag1 tag2), + access_level: 'ref_protected', + maximum_timeout: 600, + name: 'some name', + version: 'some version', + revision: 'some revision', + platform: 'some platform', + architecture: 'some architecture', + ip_address: '10.0.0.1', + config: { + gpus: 'some gpu config' + } + } + end + + it 'creates runner with specified values', :aggregate_failures do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.active).to eq args[:active] + expect(subject.locked).to eq args[:locked] + expect(subject.run_untagged).to eq args[:run_untagged] + expect(subject.tags).to contain_exactly( + an_object_having_attributes(name: 'tag1'), + an_object_having_attributes(name: 'tag2') + ) + expect(subject.access_level).to eq args[:access_level] + expect(subject.maximum_timeout).to eq args[:maximum_timeout] + expect(subject.name).to eq args[:name] + expect(subject.version).to eq args[:version] + expect(subject.revision).to eq args[:revision] + expect(subject.platform).to eq args[:platform] + expect(subject.architecture).to eq args[:architecture] + expect(subject.ip_address).to eq args[:ip_address] + end + end + end + + context 'when project token is used' do + let(:project) { create(:project) } + let(:token) { project.runners_token } + + it 'creates project runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(project.runners.size).to eq(1) + is_expected.to eq(project.runners.first) + expect(subject.token).not_to eq(registration_token) + expect(subject.token).not_to eq(project.runners_token) + expect(subject).to be_project_type + end + + context 'when it exceeds the application limits' do + before do + create(:ci_runner, runner_type: :project_type, projects: [project], contacted_at: 1.second.ago) + create(:plan_limits, :default_plan, ci_registered_project_runners: 1) + end + + it 'does not create runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.persisted?).to be_falsey + expect(subject.errors.messages).to eq('runner_projects.base': ['Maximum number of ci registered project runners (1) exceeded']) + expect(project.runners.reload.size).to eq(1) + end + end + + context 'when abandoned runners cause application limits to not be exceeded' do + before do + create(:ci_runner, runner_type: :project_type, projects: [project], created_at: 14.months.ago, contacted_at: 13.months.ago) + create(:plan_limits, :default_plan, ci_registered_project_runners: 1) + end + + it 'creates runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.errors).to be_empty + expect(project.runners.reload.size).to eq(2) + expect(project.runners.recent.size).to eq(1) + end + end + + context 'when valid runner registrars do not include project' do + before do + stub_application_setting(valid_runner_registrars: ['group']) + end + + context 'when feature flag is enabled' do + before do + stub_feature_flags(runner_registration_control: true) + end + + it 'returns 403 error' do + is_expected.to be_nil + end + end + + context 'when feature flag is disabled' do + it 'registers the runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.errors).to be_empty + expect(subject.active).to be true + end + end + end + end + + context 'when group token is used' do + let(:group) { create(:group) } + let(:token) { group.runners_token } + + it 'creates a group runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.errors).to be_empty + expect(group.runners.reload.size).to eq(1) + expect(subject.token).not_to eq(registration_token) + expect(subject.token).not_to eq(group.runners_token) + expect(subject).to be_group_type + end + + context 'when it exceeds the application limits' do + before do + create(:ci_runner, runner_type: :group_type, groups: [group], contacted_at: nil, created_at: 1.month.ago) + create(:plan_limits, :default_plan, ci_registered_group_runners: 1) + end + + it 'does not create runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.persisted?).to be_falsey + expect(subject.errors.messages).to eq('runner_namespaces.base': ['Maximum number of ci registered group runners (1) exceeded']) + expect(group.runners.reload.size).to eq(1) + end + end + + context 'when abandoned runners cause application limits to not be exceeded' do + before do + create(:ci_runner, runner_type: :group_type, groups: [group], created_at: 4.months.ago, contacted_at: 3.months.ago) + create(:ci_runner, runner_type: :group_type, groups: [group], contacted_at: nil, created_at: 4.months.ago) + create(:plan_limits, :default_plan, ci_registered_group_runners: 1) + end + + it 'creates runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.errors).to be_empty + expect(group.runners.reload.size).to eq(3) + expect(group.runners.recent.size).to eq(1) + end + end + + context 'when valid runner registrars do not include group' do + before do + stub_application_setting(valid_runner_registrars: ['project']) + end + + context 'when feature flag is enabled' do + before do + stub_feature_flags(runner_registration_control: true) + end + + it 'returns nil' do + is_expected.to be_nil + end + end + + context 'when feature flag is disabled' do + it 'registers the runner' do + is_expected.to be_an_instance_of(::Ci::Runner) + expect(subject.errors).to be_empty + expect(subject.active).to be true + end + end + end + end + end + end +end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 5d56084faa8..4e8e41ca6e6 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -188,13 +188,6 @@ RSpec.describe Ci::RetryBuildService do expect(new_build).to be_pending end - it 'resolves todos for old build that failed' do - expect(::MergeRequests::AddTodoWhenBuildFailsService) - .to receive_message_chain(:new, :close) - - service.execute(build) - end - context 'when there are subsequent processables that are skipped' do let!(:subsequent_build) do create(:ci_build, :skipped, stage_idx: 2, @@ -272,6 +265,17 @@ RSpec.describe Ci::RetryBuildService do expect(bridge.reload).to be_pending end end + + context 'when there is a failed job todo for the MR' do + let!(:merge_request) { create(:merge_request, source_project: project, author: user, head_pipeline: pipeline) } + let!(:todo) { create(:todo, :build_failed, user: user, project: project, author: user, target: merge_request) } + + it 'resolves the todo for the old failed build' do + expect do + service.execute(build) + end.to change { todo.reload.state }.from('pending').to('done') + end + end end context 'when user does not have ability to execute build' do @@ -367,6 +371,14 @@ RSpec.describe Ci::RetryBuildService do it_behaves_like 'when build with dynamic environment is retried' context 'when create_deployment_in_separate_transaction feature flag is disabled' do + let(:new_build) do + travel_to(1.second.from_now) do + ::Gitlab::Database::QueryAnalyzers::PreventCrossDatabaseModification.allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/345668') do + service.clone!(build) + end + end + end + before do stub_feature_flags(create_deployment_in_separate_transaction: false) end diff --git a/spec/services/clusters/agent_tokens/track_usage_service_spec.rb b/spec/services/clusters/agent_tokens/track_usage_service_spec.rb new file mode 100644 index 00000000000..3350b15a5ce --- /dev/null +++ b/spec/services/clusters/agent_tokens/track_usage_service_spec.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Clusters::AgentTokens::TrackUsageService do + let_it_be(:agent) { create(:cluster_agent) } + + describe '#execute', :clean_gitlab_redis_cache do + let(:agent_token) { create(:cluster_agent_token, agent: agent) } + + subject { described_class.new(agent_token).execute } + + context 'when last_used_at was updated recently' do + before do + agent_token.update!(last_used_at: 10.minutes.ago) + end + + it 'updates cache but not database' do + expect { subject }.not_to change { agent_token.reload.read_attribute(:last_used_at) } + + expect_redis_update + end + end + + context 'when last_used_at was not updated recently' do + it 'updates cache and database' do + does_db_update + expect_redis_update + end + + context 'with invalid token' do + before do + agent_token.description = SecureRandom.hex(2000) + end + + it 'still updates caches and database' do + expect(agent_token).to be_invalid + + does_db_update + expect_redis_update + end + end + + context 'agent is not connected' do + before do + allow(agent).to receive(:connected?).and_return(false) + end + + it 'creates an activity event' do + expect { subject }.to change { agent.activity_events.count } + + event = agent.activity_events.last + expect(event).to have_attributes( + kind: 'agent_connected', + level: 'info', + recorded_at: agent_token.reload.read_attribute(:last_used_at), + agent_token: agent_token + ) + end + end + + context 'agent is connected' do + before do + allow(agent).to receive(:connected?).and_return(true) + end + + it 'does not create an activity event' do + expect { subject }.not_to change { agent.activity_events.count } + end + end + end + + def expect_redis_update + Gitlab::Redis::Cache.with do |redis| + redis_key = "cache:#{agent_token.class}:#{agent_token.id}:attributes" + expect(redis.get(redis_key)).to be_present + end + end + + def does_db_update + expect { subject }.to change { agent_token.reload.read_attribute(:last_used_at) } + end + end +end diff --git a/spec/services/clusters/agents/create_activity_event_service_spec.rb b/spec/services/clusters/agents/create_activity_event_service_spec.rb new file mode 100644 index 00000000000..7a8f0e16d60 --- /dev/null +++ b/spec/services/clusters/agents/create_activity_event_service_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Clusters::Agents::CreateActivityEventService do + let_it_be(:agent) { create(:cluster_agent) } + let_it_be(:token) { create(:cluster_agent_token, agent: agent) } + let_it_be(:user) { create(:user) } + + describe '#execute' do + let(:params) do + { + kind: :token_created, + level: :info, + recorded_at: token.created_at, + user: user, + agent_token: token + } + end + + subject { described_class.new(agent, **params).execute } + + it 'creates an activity event record' do + expect { subject }.to change(agent.activity_events, :count).from(0).to(1) + + event = agent.activity_events.last + + expect(event).to have_attributes( + kind: 'token_created', + level: 'info', + recorded_at: token.reload.created_at, + user: user, + agent_token_id: token.id + ) + end + + it 'schedules the cleanup worker' do + expect(Clusters::Agents::DeleteExpiredEventsWorker).to receive(:perform_at) + .with(1.hour.from_now.change(min: agent.id % 60), agent.id) + + subject + end + end +end diff --git a/spec/services/clusters/agents/delete_expired_events_service_spec.rb b/spec/services/clusters/agents/delete_expired_events_service_spec.rb new file mode 100644 index 00000000000..3dc166f54eb --- /dev/null +++ b/spec/services/clusters/agents/delete_expired_events_service_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Clusters::Agents::DeleteExpiredEventsService do + let_it_be(:agent) { create(:cluster_agent) } + + describe '#execute' do + let_it_be(:event1) { create(:agent_activity_event, agent: agent, recorded_at: 1.hour.ago) } + let_it_be(:event2) { create(:agent_activity_event, agent: agent, recorded_at: 2.hours.ago) } + let_it_be(:event3) { create(:agent_activity_event, agent: agent, recorded_at: 3.hours.ago) } + let_it_be(:event4) { create(:agent_activity_event, agent: agent, recorded_at: 4.hours.ago) } + let_it_be(:event5) { create(:agent_activity_event, agent: agent, recorded_at: 5.hours.ago) } + + let(:deletion_cutoff) { 1.day.ago } + + subject { described_class.new(agent).execute } + + before do + allow(agent).to receive(:activity_event_deletion_cutoff).and_return(deletion_cutoff) + end + + it 'does not delete events if the limit has not been reached' do + expect { subject }.not_to change(agent.activity_events, :count) + end + + context 'there are more events than the limit' do + let(:deletion_cutoff) { event3.recorded_at } + + it 'removes events to remain at the limit, keeping the most recent' do + expect { subject }.to change(agent.activity_events, :count).from(5).to(3) + expect(agent.activity_events).to contain_exactly(event1, event2, event3) + end + end + end +end diff --git a/spec/services/clusters/integrations/create_service_spec.rb b/spec/services/clusters/integrations/create_service_spec.rb index 14653236ab1..6dac97ebf8f 100644 --- a/spec/services/clusters/integrations/create_service_spec.rb +++ b/spec/services/clusters/integrations/create_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Clusters::Integrations::CreateService, '#execute' do let_it_be_with_reload(:cluster) { create(:cluster, :provided_by_gcp, projects: [project]) } let(:service) do - described_class.new(container: project, cluster: cluster, current_user: project.owner, params: params) + described_class.new(container: project, cluster: cluster, current_user: project.first_owner, params: params) end shared_examples_for 'a cluster integration' do |application_type| diff --git a/spec/services/customer_relations/contacts/create_service_spec.rb b/spec/services/customer_relations/contacts/create_service_spec.rb index 71eb447055e..567e1c91e78 100644 --- a/spec/services/customer_relations/contacts/create_service_spec.rb +++ b/spec/services/customer_relations/contacts/create_service_spec.rb @@ -12,7 +12,7 @@ RSpec.describe CustomerRelations::Contacts::CreateService do subject(:response) { described_class.new(group: group, current_user: user, params: params).execute } context 'when user does not have permission' do - let_it_be(:group) { create(:group) } + let_it_be(:group) { create(:group, :crm_enabled) } before_all do group.add_reporter(user) @@ -25,7 +25,7 @@ RSpec.describe CustomerRelations::Contacts::CreateService do end context 'when user has permission' do - let_it_be(:group) { create(:group) } + let_it_be(:group) { create(:group, :crm_enabled) } before_all do group.add_developer(user) diff --git a/spec/services/customer_relations/contacts/update_service_spec.rb b/spec/services/customer_relations/contacts/update_service_spec.rb index 7c5fbabb600..253bbc23226 100644 --- a/spec/services/customer_relations/contacts/update_service_spec.rb +++ b/spec/services/customer_relations/contacts/update_service_spec.rb @@ -11,7 +11,7 @@ RSpec.describe CustomerRelations::Contacts::UpdateService do describe '#execute' do context 'when the user has no permission' do - let_it_be(:group) { create(:group) } + let_it_be(:group) { create(:group, :crm_enabled) } let(:params) { { first_name: 'Gary' } } @@ -24,7 +24,7 @@ RSpec.describe CustomerRelations::Contacts::UpdateService do end context 'when user has permission' do - let_it_be(:group) { create(:group) } + let_it_be(:group) { create(:group, :crm_enabled) } before_all do group.add_developer(user) diff --git a/spec/services/customer_relations/organizations/create_service_spec.rb b/spec/services/customer_relations/organizations/create_service_spec.rb index d8985d8d90b..18eefdd716e 100644 --- a/spec/services/customer_relations/organizations/create_service_spec.rb +++ b/spec/services/customer_relations/organizations/create_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe CustomerRelations::Organizations::CreateService do describe '#execute' do let_it_be(:user) { create(:user) } - let(:group) { create(:group) } + let(:group) { create(:group, :crm_enabled) } let(:params) { attributes_for(:organization, group: group) } subject(:response) { described_class.new(group: group, current_user: user, params: params).execute } diff --git a/spec/services/customer_relations/organizations/update_service_spec.rb b/spec/services/customer_relations/organizations/update_service_spec.rb index bc40cb3e8e7..8461c98ef0e 100644 --- a/spec/services/customer_relations/organizations/update_service_spec.rb +++ b/spec/services/customer_relations/organizations/update_service_spec.rb @@ -11,7 +11,7 @@ RSpec.describe CustomerRelations::Organizations::UpdateService do describe '#execute' do context 'when the user has no permission' do - let_it_be(:group) { create(:group) } + let_it_be(:group) { create(:group, :crm_enabled) } let(:params) { { name: 'GitLab' } } @@ -24,7 +24,7 @@ RSpec.describe CustomerRelations::Organizations::UpdateService do end context 'when user has permission' do - let_it_be(:group) { create(:group) } + let_it_be(:group) { create(:group, :crm_enabled) } before_all do group.add_developer(user) diff --git a/spec/services/dependency_proxy/download_blob_service_spec.rb b/spec/services/dependency_proxy/download_blob_service_spec.rb deleted file mode 100644 index 2f293b8a46b..00000000000 --- a/spec/services/dependency_proxy/download_blob_service_spec.rb +++ /dev/null @@ -1,59 +0,0 @@ -# frozen_string_literal: true -require 'spec_helper' - -RSpec.describe DependencyProxy::DownloadBlobService do - include DependencyProxyHelpers - - let(:image) { 'alpine' } - let(:token) { Digest::SHA256.hexdigest('123') } - let(:blob_sha) { Digest::SHA256.hexdigest('ruby:2.7.0') } - - subject(:download_blob) { described_class.new(image, blob_sha, token).execute } - - context 'remote request is successful' do - before do - stub_blob_download(image, blob_sha) - end - - it { expect(subject[:status]).to eq(:success) } - it { expect(subject[:file]).to be_a(Tempfile) } - it { expect(subject[:file].size).to eq(6) } - - it 'streams the download' do - expected_options = { headers: anything, stream_body: true } - - expect(Gitlab::HTTP).to receive(:perform_request).with(Net::HTTP::Get, anything, expected_options) - - download_blob - end - - it 'skips read_total_timeout', :aggregate_failures do - stub_const('GitLab::HTTP::DEFAULT_READ_TOTAL_TIMEOUT', 0) - - expect(Gitlab::Metrics::System).not_to receive(:monotonic_time) - expect(download_blob).to include(status: :success) - end - end - - context 'remote request is not found' do - before do - stub_blob_download(image, blob_sha, 404) - end - - it { expect(subject[:status]).to eq(:error) } - it { expect(subject[:http_status]).to eq(404) } - it { expect(subject[:message]).to eq('Non-success response code on downloading blob fragment') } - end - - context 'net timeout exception' do - before do - blob_url = DependencyProxy::Registry.blob_url(image, blob_sha) - - stub_full_request(blob_url).to_timeout - end - - it { expect(subject[:status]).to eq(:error) } - it { expect(subject[:http_status]).to eq(599) } - it { expect(subject[:message]).to eq('execution expired') } - end -end diff --git a/spec/services/dependency_proxy/find_cached_manifest_service_spec.rb b/spec/services/dependency_proxy/find_cached_manifest_service_spec.rb index 29bdf1f11c3..607d67d8efe 100644 --- a/spec/services/dependency_proxy/find_cached_manifest_service_spec.rb +++ b/spec/services/dependency_proxy/find_cached_manifest_service_spec.rb @@ -91,9 +91,9 @@ RSpec.describe DependencyProxy::FindCachedManifestService do it_behaves_like 'returning no manifest' end - context 'when the cached manifest is expired' do + context 'when the cached manifest is pending destruction' do before do - dependency_proxy_manifest.update_column(:status, DependencyProxy::Manifest.statuses[:expired]) + dependency_proxy_manifest.update_column(:status, DependencyProxy::Manifest.statuses[:pending_destruction]) stub_manifest_head(image, tag, headers: headers) stub_manifest_download(image, tag, headers: headers) end diff --git a/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb b/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb deleted file mode 100644 index 5f7afdf699a..00000000000 --- a/spec/services/dependency_proxy/find_or_create_blob_service_spec.rb +++ /dev/null @@ -1,71 +0,0 @@ -# frozen_string_literal: true -require 'spec_helper' - -RSpec.describe DependencyProxy::FindOrCreateBlobService do - include DependencyProxyHelpers - - let_it_be_with_reload(:blob) { create(:dependency_proxy_blob) } - - let(:group) { blob.group } - let(:image) { 'alpine' } - let(:tag) { '3.9' } - let(:token) { Digest::SHA256.hexdigest('123') } - let(:blob_sha) { '40bd001563085fc35165329ea1ff5c5ecbdbbeef' } - - subject { described_class.new(group, image, token, blob_sha).execute } - - before do - stub_registry_auth(image, token) - end - - shared_examples 'downloads the remote blob' do - it 'downloads blob from remote registry if there is no cached one' do - expect(subject[:status]).to eq(:success) - expect(subject[:blob]).to be_a(DependencyProxy::Blob) - expect(subject[:blob]).to be_persisted - expect(subject[:from_cache]).to eq false - end - end - - context 'no cache' do - before do - stub_blob_download(image, blob_sha) - end - - it_behaves_like 'downloads the remote blob' - end - - context 'cached blob' do - let(:blob_sha) { blob.file_name.sub('.gz', '') } - - it 'uses cached blob instead of downloading one' do - expect { subject }.to change { blob.reload.read_at } - - expect(subject[:status]).to eq(:success) - expect(subject[:blob]).to be_a(DependencyProxy::Blob) - expect(subject[:blob]).to eq(blob) - expect(subject[:from_cache]).to eq true - end - - context 'when the cached blob is expired' do - before do - blob.update_column(:status, DependencyProxy::Blob.statuses[:expired]) - stub_blob_download(image, blob_sha) - end - - it_behaves_like 'downloads the remote blob' - end - end - - context 'no such blob exists remotely' do - before do - stub_blob_download(image, blob_sha, 404) - end - - it 'returns error message and http status' do - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Failed to download the blob') - expect(subject[:http_status]).to eq(404) - end - end -end diff --git a/spec/services/deployments/archive_in_project_service_spec.rb b/spec/services/deployments/archive_in_project_service_spec.rb index d4039ee7b4a..a316c210d64 100644 --- a/spec/services/deployments/archive_in_project_service_spec.rb +++ b/spec/services/deployments/archive_in_project_service_spec.rb @@ -50,17 +50,6 @@ RSpec.describe Deployments::ArchiveInProjectService do end end - context 'when deployments_archive feature flag is disabled' do - before do - stub_feature_flags(deployments_archive: false) - end - - it 'does not do anything' do - expect(subject[:status]).to eq(:error) - expect(subject[:message]).to eq('Feature flag is not enabled') - end - end - def deployment_refs_exist? deployment_refs.map { |path| project.repository.ref_exists?(path) } end diff --git a/spec/services/deployments/create_for_build_service_spec.rb b/spec/services/deployments/create_for_build_service_spec.rb new file mode 100644 index 00000000000..6fc7c9e56a6 --- /dev/null +++ b/spec/services/deployments/create_for_build_service_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Deployments::CreateForBuildService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + + let(:service) { described_class.new } + + describe '#execute' do + subject { service.execute(build) } + + context 'with a deployment job' do + let!(:build) { create(:ci_build, :start_review_app, project: project) } + let!(:environment) { create(:environment, project: project, name: build.expanded_environment_name) } + + it 'creates a deployment record' do + expect { subject }.to change { Deployment.count }.by(1) + + build.reset + expect(build.deployment.project).to eq(build.project) + expect(build.deployment.ref).to eq(build.ref) + expect(build.deployment.sha).to eq(build.sha) + expect(build.deployment.deployable).to eq(build) + expect(build.deployment.deployable_type).to eq('CommitStatus') + expect(build.deployment.environment).to eq(build.persisted_environment) + end + + context 'when creation failure occures' do + before do + allow(build).to receive(:create_deployment!) { raise ActiveRecord::RecordInvalid } + end + + it 'trackes the exception' do + expect { subject }.to raise_error(described_class::DeploymentCreationError) + + expect(Deployment.count).to eq(0) + end + end + + context 'when the corresponding environment does not exist' do + let!(:environment) { } + + it 'does not create a deployment record' do + expect { subject }.not_to change { Deployment.count } + + expect(build.deployment).to be_nil + end + end + end + + context 'with a teardown job' do + let!(:build) { create(:ci_build, :stop_review_app, project: project) } + let!(:environment) { create(:environment, name: build.expanded_environment_name) } + + it 'does not create a deployment record' do + expect { subject }.not_to change { Deployment.count } + + expect(build.deployment).to be_nil + end + end + + context 'with a normal job' do + let!(:build) { create(:ci_build, project: project) } + + it 'does not create a deployment record' do + expect { subject }.not_to change { Deployment.count } + + expect(build.deployment).to be_nil + end + end + + context 'with a bridge' do + let!(:build) { create(:ci_bridge, project: project) } + + it 'does not create a deployment record' do + expect { subject }.not_to change { Deployment.count } + end + end + end +end diff --git a/spec/services/discussions/update_diff_position_service_spec.rb b/spec/services/discussions/update_diff_position_service_spec.rb index 85020e95c83..e7a3505bbd4 100644 --- a/spec/services/discussions/update_diff_position_service_spec.rb +++ b/spec/services/discussions/update_diff_position_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Discussions::UpdateDiffPositionService do let(:project) { create(:project, :repository) } - let(:current_user) { project.owner } + let(:current_user) { project.first_owner } let(:create_commit) { project.commit("913c66a37b4a45b9769037c55c2d238bd0942d2e") } let(:modify_commit) { project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e") } let(:edit_commit) { project.commit("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") } diff --git a/spec/services/error_tracking/collect_error_service_spec.rb b/spec/services/error_tracking/collect_error_service_spec.rb index 52d095148c8..2b16612dac3 100644 --- a/spec/services/error_tracking/collect_error_service_spec.rb +++ b/spec/services/error_tracking/collect_error_service_spec.rb @@ -4,8 +4,9 @@ require 'spec_helper' RSpec.describe ErrorTracking::CollectErrorService do let_it_be(:project) { create(:project) } - let_it_be(:parsed_event_file) { 'error_tracking/parsed_event.json' } - let_it_be(:parsed_event) { Gitlab::Json.parse(fixture_file(parsed_event_file)) } + + let(:parsed_event_file) { 'error_tracking/parsed_event.json' } + let(:parsed_event) { parse_valid_event(parsed_event_file) } subject { described_class.new(project, nil, event: parsed_event) } @@ -43,7 +44,7 @@ RSpec.describe ErrorTracking::CollectErrorService do end context 'python sdk event' do - let(:parsed_event) { Gitlab::Json.parse(fixture_file('error_tracking/python_event.json')) } + let(:parsed_event_file) { 'error_tracking/python_event.json' } it 'creates a valid event' do expect { subject.execute }.to change { ErrorTracking::ErrorEvent.count }.by(1) @@ -75,7 +76,7 @@ RSpec.describe ErrorTracking::CollectErrorService do end context 'go payload' do - let(:parsed_event) { Gitlab::Json.parse(fixture_file('error_tracking/go_parsed_event.json')) } + let(:parsed_event_file) { 'error_tracking/go_parsed_event.json' } it 'has correct values set' do subject.execute @@ -92,6 +93,38 @@ RSpec.describe ErrorTracking::CollectErrorService do expect(event.environment).to eq 'Accumulate' expect(event.payload).to eq parsed_event end + + context 'with two exceptions' do + let(:parsed_event_file) { 'error_tracking/go_two_exception_event.json' } + + it 'reports using second exception', :aggregate_failures do + subject.execute + + event = ErrorTracking::ErrorEvent.last + error = event.error + + expect(error.name).to eq '*url.Error' + expect(error.description).to eq(%(Get \"foobar\": unsupported protocol scheme \"\")) + expect(error.platform).to eq 'go' + expect(error.actor).to eq('main(main)') + + expect(event.description).to eq(%(Get \"foobar\": unsupported protocol scheme \"\")) + expect(event.payload).to eq parsed_event + end + end end end + + private + + def parse_valid_event(parsed_event_file) + parsed_event = Gitlab::Json.parse(fixture_file(parsed_event_file)) + + validator = ErrorTracking::Collector::PayloadValidator.new + # This a precondition for all specs to verify that + # submitted JSON payload is valid. + expect(validator).to be_valid(parsed_event) + + parsed_event + end end diff --git a/spec/services/events/destroy_service_spec.rb b/spec/services/events/destroy_service_spec.rb index 8dcbb83eb1d..8b07852c040 100644 --- a/spec/services/events/destroy_service_spec.rb +++ b/spec/services/events/destroy_service_spec.rb @@ -30,16 +30,28 @@ RSpec.describe Events::DestroyService do expect(unrelated_event.reload).to be_present end + context 'batch delete' do + before do + stub_const("#{described_class}::BATCH_SIZE", 2) + end + + it 'splits delete queries into batches' do + expect(project).to receive(:events).twice.and_call_original + + subject.execute + end + end + context 'when an error is raised while deleting the records' do before do - allow(project).to receive_message_chain(:events, :all, :delete_all).and_raise(ActiveRecord::ActiveRecordError) + allow(project).to receive_message_chain(:events, :limit, :delete_all).and_raise(ActiveRecord::ActiveRecordError, 'custom error') end it 'returns error' do response = subject.execute expect(response).to be_error - expect(response.message).to eq 'Failed to remove events.' + expect(response.message).to eq 'custom error' end it 'does not delete events' do diff --git a/spec/services/feature_flags/hook_service_spec.rb b/spec/services/feature_flags/hook_service_spec.rb index 02cdbbd86ac..19c935e43f3 100644 --- a/spec/services/feature_flags/hook_service_spec.rb +++ b/spec/services/feature_flags/hook_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe FeatureFlags::HookService do let_it_be(:namespace) { create(:namespace) } let_it_be(:project) { create(:project, :repository, namespace: namespace) } let_it_be(:feature_flag) { create(:operations_feature_flag, project: project) } - let_it_be(:user) { namespace.owner } + let_it_be(:user) { namespace.first_owner } let!(:hook) { create(:project_hook, project: project) } let(:hook_data) { double } diff --git a/spec/services/git/process_ref_changes_service_spec.rb b/spec/services/git/process_ref_changes_service_spec.rb index f52df9b0073..05c1f898cab 100644 --- a/spec/services/git/process_ref_changes_service_spec.rb +++ b/spec/services/git/process_ref_changes_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Git::ProcessRefChangesService do let(:project) { create(:project, :repository) } - let(:user) { project.owner } + let(:user) { project.first_owner } let(:params) { { changes: git_changes } } subject { described_class.new(project, user, params) } @@ -34,7 +34,7 @@ RSpec.describe Git::ProcessRefChangesService do it "calls #{push_service_class}" do expect(push_service_class) .to receive(:new) - .with(project, project.owner, hash_including(execute_project_hooks: true, create_push_event: true)) + .with(project, project.first_owner, hash_including(execute_project_hooks: true, create_push_event: true)) .exactly(changes.count).times .and_return(service) @@ -58,7 +58,7 @@ RSpec.describe Git::ProcessRefChangesService do it "calls #{push_service_class} with execute_project_hooks set to false" do expect(push_service_class) .to receive(:new) - .with(project, project.owner, hash_including(execute_project_hooks: false)) + .with(project, project.first_owner, hash_including(execute_project_hooks: false)) .exactly(changes.count).times .and_return(service) @@ -86,7 +86,7 @@ RSpec.describe Git::ProcessRefChangesService do it "calls #{push_service_class} with create_push_event set to false" do expect(push_service_class) .to receive(:new) - .with(project, project.owner, hash_including(create_push_event: false)) + .with(project, project.first_owner, hash_including(create_push_event: false)) .exactly(changes.count).times .and_return(service) @@ -170,7 +170,7 @@ RSpec.describe Git::ProcessRefChangesService do allow(push_service_class) .to receive(:new) - .with(project, project.owner, hash_including(execute_project_hooks: true, create_push_event: true)) + .with(project, project.first_owner, hash_including(execute_project_hooks: true, create_push_event: true)) .exactly(changes.count).times .and_return(service) end diff --git a/spec/services/google_cloud/create_service_accounts_service_spec.rb b/spec/services/google_cloud/create_service_accounts_service_spec.rb new file mode 100644 index 00000000000..190e1a8098c --- /dev/null +++ b/spec/services/google_cloud/create_service_accounts_service_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +# Mock Types +MockGoogleOAuth2Credentials = Struct.new(:app_id, :app_secret) +MockServiceAccount = Struct.new(:project_id, :unique_id) + +RSpec.describe GoogleCloud::CreateServiceAccountsService do + describe '#execute' do + before do + allow(Gitlab::Auth::OAuth::Provider).to receive(:config_for) + .with('google_oauth2') + .and_return(MockGoogleOAuth2Credentials.new('mock-app-id', 'mock-app-secret')) + + allow_next_instance_of(GoogleApi::CloudPlatform::Client) do |client| + allow(client).to receive(:create_service_account) + .and_return(MockServiceAccount.new('mock-project-id', 'mock-unique-id')) + allow(client).to receive(:create_service_account_key) + .and_return('mock-key') + end + end + + it 'creates unprotected vars', :aggregate_failures do + project = create(:project) + + service = described_class.new( + project, + nil, + google_oauth2_token: 'mock-token', + gcp_project_id: 'mock-gcp-project-id', + environment_name: '*' + ) + + response = service.execute + + expect(response.status).to eq(:success) + expect(response.message).to eq('Service account generated successfully') + expect(project.variables.count).to eq(3) + expect(project.variables.first.protected).to eq(false) + expect(project.variables.second.protected).to eq(false) + expect(project.variables.third.protected).to eq(false) + end + end +end diff --git a/spec/services/google_cloud/service_accounts_service_spec.rb b/spec/services/google_cloud/service_accounts_service_spec.rb index 505c623c02a..17c1f61a96e 100644 --- a/spec/services/google_cloud/service_accounts_service_spec.rb +++ b/spec/services/google_cloud/service_accounts_service_spec.rb @@ -60,8 +60,8 @@ RSpec.describe GoogleCloud::ServiceAccountsService do let_it_be(:project) { create(:project) } it 'saves GCP creds as project CI vars' do - service.add_for_project('env_1', 'gcp_prj_id_1', 'srv_acc_1', 'srv_acc_key_1') - service.add_for_project('env_2', 'gcp_prj_id_2', 'srv_acc_2', 'srv_acc_key_2') + service.add_for_project('env_1', 'gcp_prj_id_1', 'srv_acc_1', 'srv_acc_key_1', true) + service.add_for_project('env_2', 'gcp_prj_id_2', 'srv_acc_2', 'srv_acc_key_2', false) list = service.find_for_project @@ -81,7 +81,7 @@ RSpec.describe GoogleCloud::ServiceAccountsService do end it 'replaces previously stored CI vars with new CI vars' do - service.add_for_project('env_1', 'new_project', 'srv_acc_1', 'srv_acc_key_1') + service.add_for_project('env_1', 'new_project', 'srv_acc_1', 'srv_acc_key_1', false) list = service.find_for_project @@ -101,9 +101,16 @@ RSpec.describe GoogleCloud::ServiceAccountsService do end end - it 'underlying project CI vars must be protected' do - expect(project.variables.first.protected).to eq(true) - expect(project.variables.second.protected).to eq(true) + it 'underlying project CI vars must be protected as per value' do + service.add_for_project('env_1', 'gcp_prj_id_1', 'srv_acc_1', 'srv_acc_key_1', true) + service.add_for_project('env_2', 'gcp_prj_id_2', 'srv_acc_2', 'srv_acc_key_2', false) + + expect(project.variables[0].protected).to eq(true) + expect(project.variables[1].protected).to eq(true) + expect(project.variables[2].protected).to eq(true) + expect(project.variables[3].protected).to eq(false) + expect(project.variables[4].protected).to eq(false) + expect(project.variables[5].protected).to eq(false) end end end diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index e1bd3732820..46c5e2a9818 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -163,6 +163,70 @@ RSpec.describe Groups::UpdateService do expect(updated_group.parent_id).to be_nil end end + + context 'crm_enabled param' do + context 'when no existing crm_settings' do + it 'when param not present, leave crm disabled' do + params = {} + + described_class.new(public_group, user, params).execute + updated_group = public_group.reload + + expect(updated_group.crm_enabled?).to be_falsey + end + + it 'when param set true, enables crm' do + params = { crm_enabled: true } + + described_class.new(public_group, user, params).execute + updated_group = public_group.reload + + expect(updated_group.crm_enabled?).to be_truthy + end + end + + context 'with existing crm_settings' do + it 'when param set true, enables crm' do + params = { crm_enabled: true } + create(:crm_settings, group: public_group) + + described_class.new(public_group, user, params).execute + + updated_group = public_group.reload + expect(updated_group.crm_enabled?).to be_truthy + end + + it 'when param set false, disables crm' do + params = { crm_enabled: false } + create(:crm_settings, group: public_group, enabled: true) + + described_class.new(public_group, user, params).execute + + updated_group = public_group.reload + expect(updated_group.crm_enabled?).to be_falsy + end + + it 'when param not present, crm remains disabled' do + params = {} + create(:crm_settings, group: public_group) + + described_class.new(public_group, user, params).execute + + updated_group = public_group.reload + expect(updated_group.crm_enabled?).to be_falsy + end + + it 'when param not present, crm remains enabled' do + params = {} + create(:crm_settings, group: public_group, enabled: true) + + described_class.new(public_group, user, params).execute + + updated_group = public_group.reload + expect(updated_group.crm_enabled?).to be_truthy + end + end + end end context "unauthorized visibility_level validation" do diff --git a/spec/services/import/gitlab_projects/create_project_from_remote_file_service_spec.rb b/spec/services/import/gitlab_projects/create_project_from_remote_file_service_spec.rb index 3c461c91ff0..92c46cf7052 100644 --- a/spec/services/import/gitlab_projects/create_project_from_remote_file_service_spec.rb +++ b/spec/services/import/gitlab_projects/create_project_from_remote_file_service_spec.rb @@ -18,24 +18,29 @@ RSpec.describe ::Import::GitlabProjects::CreateProjectFromRemoteFileService do subject { described_class.new(user, params) } - it 'creates a project and returns a successful response' do - stub_headers_for(remote_url, { - 'content-type' => 'application/gzip', - 'content-length' => '10' - }) - - response = nil - expect { response = subject.execute } - .to change(Project, :count).by(1) + shared_examples 'successfully import' do |content_type| + it 'creates a project and returns a successful response' do + stub_headers_for(remote_url, { + 'content-type' => content_type, + 'content-length' => '10' + }) - expect(response).to be_success - expect(response.http_status).to eq(:ok) - expect(response.payload).to be_instance_of(Project) - expect(response.payload.name).to eq('name') - expect(response.payload.path).to eq('path') - expect(response.payload.namespace).to eq(user.namespace) + response = nil + expect { response = subject.execute } + .to change(Project, :count).by(1) + + expect(response).to be_success + expect(response.http_status).to eq(:ok) + expect(response.payload).to be_instance_of(Project) + expect(response.payload.name).to eq('name') + expect(response.payload.path).to eq('path') + expect(response.payload.namespace).to eq(user.namespace) + end end + it_behaves_like 'successfully import', 'application/gzip' + it_behaves_like 'successfully import', 'application/x-tar' + context 'when the file url is invalid' do it 'returns an erred response with the reason of the failure' do stub_application_setting(allow_local_requests_from_web_hooks_and_services: false) @@ -79,7 +84,7 @@ RSpec.describe ::Import::GitlabProjects::CreateProjectFromRemoteFileService do expect(response).not_to be_success expect(response.http_status).to eq(:bad_request) expect(response.message) - .to eq("Remote file content type 'application/js' not allowed. (Allowed content types: application/gzip)") + .to eq("Remote file content type 'application/js' not allowed. (Allowed content types: application/gzip, application/x-tar)") end end @@ -130,6 +135,20 @@ RSpec.describe ::Import::GitlabProjects::CreateProjectFromRemoteFileService do end end + it 'does not validate content-type or content-length when the file is stored in AWS-S3' do + stub_headers_for(remote_url, { + 'Server' => 'AmazonS3', + 'x-amz-request-id' => 'Something' + }) + + response = nil + expect { response = subject.execute } + .to change(Project, :count) + + expect(response).to be_success + expect(response.http_status).to eq(:ok) + end + context 'when required parameters are not provided' do let(:params) { {} } diff --git a/spec/services/import/validate_remote_git_endpoint_service_spec.rb b/spec/services/import/validate_remote_git_endpoint_service_spec.rb index fbd8a3cb323..9dc862b6ca3 100644 --- a/spec/services/import/validate_remote_git_endpoint_service_spec.rb +++ b/spec/services/import/validate_remote_git_endpoint_service_spec.rb @@ -24,6 +24,17 @@ RSpec.describe Import::ValidateRemoteGitEndpointService do expect(Gitlab::HTTP).to have_received(:get).with(endpoint_url, basic_auth: nil, stream_body: true, follow_redirects: false) end + context 'when uri is using git:// protocol' do + subject { described_class.new(url: 'git://demo.host/repo')} + + it 'returns success' do + result = subject.execute + + expect(result).to be_a(ServiceResponse) + expect(result.success?).to be(true) + end + end + context 'when receiving HTTP response' do subject { described_class.new(url: base_url) } diff --git a/spec/services/incident_management/incidents/create_service_spec.rb b/spec/services/incident_management/incidents/create_service_spec.rb index 0f32a4b5425..47ce9d01f66 100644 --- a/spec/services/incident_management/incidents/create_service_spec.rb +++ b/spec/services/incident_management/incidents/create_service_spec.rb @@ -39,7 +39,7 @@ RSpec.describe IncidentManagement::Incidents::CreateService do let(:issue) { new_issue } - include_examples 'has incident label' + include_examples 'does not have incident label' end context 'with default severity' do @@ -71,8 +71,8 @@ RSpec.describe IncidentManagement::Incidents::CreateService do end context 'when incident label does not exists' do - it 'creates incident label' do - expect { create_incident }.to change { project.labels.where(title: label_title).count }.by(1) + it 'does not create incident label' do + expect { create_incident }.to not_change { project.labels.where(title: label_title).count } end end diff --git a/spec/services/incident_management/issuable_escalation_statuses/after_update_service_spec.rb b/spec/services/incident_management/issuable_escalation_statuses/after_update_service_spec.rb new file mode 100644 index 00000000000..e9db6ba8d28 --- /dev/null +++ b/spec/services/incident_management/issuable_escalation_statuses/after_update_service_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IncidentManagement::IssuableEscalationStatuses::AfterUpdateService do + let_it_be(:current_user) { create(:user) } + let_it_be(:escalation_status, reload: true) { create(:incident_management_issuable_escalation_status, :triggered) } + let_it_be(:issue, reload: true) { escalation_status.issue } + let_it_be(:project) { issue.project } + let_it_be(:alert) { create(:alert_management_alert, issue: issue, project: project) } + + let(:status_event) { :acknowledge } + let(:update_params) { { incident_management_issuable_escalation_status_attributes: { status_event: status_event } } } + let(:service) { IncidentManagement::IssuableEscalationStatuses::AfterUpdateService.new(issue, current_user) } + + subject(:result) do + issue.update!(update_params) + service.execute + end + + before do + issue.project.add_developer(current_user) + end + + shared_examples 'does not attempt to update the alert' do + specify do + expect(::AlertManagement::Alerts::UpdateService).not_to receive(:new) + + expect(result).to be_success + end + end + + context 'with status attributes' do + it 'updates the alert with the new alert status' do + expect(::AlertManagement::Alerts::UpdateService).to receive(:new).once.and_call_original + expect(described_class).to receive(:new).once.and_call_original + + expect { result }.to change { escalation_status.reload.acknowledged? }.to(true) + .and change { alert.reload.acknowledged? }.to(true) + end + + context 'when incident is not associated with an alert' do + before do + alert.destroy! + end + + it_behaves_like 'does not attempt to update the alert' + end + + context 'when new status matches the current status' do + let(:status_event) { :trigger } + + it_behaves_like 'does not attempt to update the alert' + end + end +end diff --git a/spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb b/spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb new file mode 100644 index 00000000000..bfed5319028 --- /dev/null +++ b/spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateService do + let_it_be(:escalation_status) { create(:incident_management_issuable_escalation_status, :triggered) } + let_it_be(:user_with_permissions) { create(:user) } + + let(:current_user) { user_with_permissions } + let(:issue) { escalation_status.issue } + let(:status) { :acknowledged } + let(:params) { { status: status } } + let(:service) { IncidentManagement::IssuableEscalationStatuses::PrepareUpdateService.new(issue, current_user, params) } + + subject(:result) { service.execute } + + before do + issue.project.add_developer(user_with_permissions) + end + + shared_examples 'successful response' do |payload| + it 'returns valid parameters which can be used to update the issue' do + expect(result).to be_success + expect(result.payload).to eq(escalation_status: payload) + end + end + + shared_examples 'error response' do |message| + specify do + expect(result).to be_error + expect(result.message).to eq(message) + end + end + + shared_examples 'availability error response' do + include_examples 'error response', 'Escalation status updates are not available for this issue, user, or project.' + end + + shared_examples 'invalid params error response' do + include_examples 'error response', 'Invalid value was provided for a parameter.' + end + + it_behaves_like 'successful response', { status_event: :acknowledge } + + context 'when feature flag is disabled' do + before do + stub_feature_flags(incident_escalations: false) + end + + it_behaves_like 'availability error response' + end + + context 'when user is anonymous' do + let(:current_user) { nil } + + it_behaves_like 'availability error response' + end + + context 'when user does not have permissions' do + let(:current_user) { create(:user) } + + it_behaves_like 'availability error response' + end + + context 'when called with an unsupported issue type' do + let(:issue) { create(:issue) } + + it_behaves_like 'availability error response' + end + + context 'when an IssuableEscalationStatus record for the issue does not exist' do + let(:issue) { create(:incident) } + + it_behaves_like 'availability error response' + end + + context 'when called without params' do + let(:params) { nil } + + it_behaves_like 'successful response', {} + end + + context 'when called with unsupported params' do + let(:params) { { escalations_started_at: Time.current } } + + it_behaves_like 'successful response', {} + end + + context 'with status param' do + context 'when status matches the current status' do + let(:params) { { status: :triggered } } + + it_behaves_like 'successful response', {} + end + + context 'when status is unsupported' do + let(:params) { { status: :mitigated } } + + it_behaves_like 'invalid params error response' + end + + context 'when status is a String' do + let(:params) { { status: 'acknowledged' } } + + it_behaves_like 'successful response', { status_event: :acknowledge } + end + end +end diff --git a/spec/services/integrations/test/project_service_spec.rb b/spec/services/integrations/test/project_service_spec.rb index 32f9f632d7a..74833686283 100644 --- a/spec/services/integrations/test/project_service_spec.rb +++ b/spec/services/integrations/test/project_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Integrations::Test::ProjectService do let_it_be(:project) { create(:project) } let(:integration) { create(:integrations_slack, project: project) } - let(:user) { project.owner } + let(:user) { project.first_owner } let(:event) { nil } let(:sample_data) { { data: 'sample' } } let(:success_result) { { success: true, result: {} } } diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb index cf75efb5c57..304e4bb3ebb 100644 --- a/spec/services/issues/build_service_spec.rb +++ b/spec/services/issues/build_service_spec.rb @@ -172,9 +172,9 @@ RSpec.describe Issues::BuildService do end describe 'setting issue type' do - context 'with a corresponding WorkItem::Type' do - let_it_be(:type_issue_id) { WorkItem::Type.default_issue_type.id } - let_it_be(:type_incident_id) { WorkItem::Type.default_by_type(:incident).id } + context 'with a corresponding WorkItems::Type' do + let_it_be(:type_issue_id) { WorkItems::Type.default_issue_type.id } + let_it_be(:type_incident_id) { WorkItems::Type.default_by_type(:incident).id } where(:issue_type, :current_user, :work_item_type_id, :resulting_issue_type) do nil | ref(:guest) | ref(:type_issue_id) | 'issue' diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 8496bd31e00..b2dcfb5c6d3 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Issues::CreateService do include AfterNextHelpers - let_it_be(:group) { create(:group) } + let_it_be(:group) { create(:group, :crm_enabled) } let_it_be_with_reload(:project) { create(:project, group: group) } let_it_be(:user) { create(:user) } @@ -61,6 +61,7 @@ RSpec.describe Issues::CreateService do expect(Issuable::CommonSystemNotesService).to receive_message_chain(:new, :execute) expect(issue).to be_persisted + expect(issue).to be_a(::Issue) expect(issue.title).to eq('Awesome issue') expect(issue.assignees).to eq([assignee]) expect(issue.labels).to match_array(labels) @@ -69,6 +70,18 @@ RSpec.describe Issues::CreateService do expect(issue.work_item_type.base_type).to eq('issue') end + context 'when a build_service is provided' do + let(:issue) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params, build_service: build_service).execute } + + let(:issue_from_builder) { WorkItem.new(project: project, title: 'Issue from builder') } + let(:build_service) { double(:build_service, execute: issue_from_builder) } + + it 'uses the provided service to build the issue' do + expect(issue).to be_persisted + expect(issue).to be_a(WorkItem) + end + end + context 'when skip_system_notes is true' do let(:issue) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute(skip_system_notes: true) } @@ -101,11 +114,11 @@ RSpec.describe Issues::CreateService do end it_behaves_like 'incident issue' - it_behaves_like 'has incident label' + it_behaves_like 'does not have incident label' - it 'does create an incident label' do + it 'does not create an incident label' do expect { subject } - .to change { Label.where(incident_label_attributes).count }.by(1) + .to not_change { Label.where(incident_label_attributes).count } end it 'calls IncidentManagement::Incidents::CreateEscalationStatusService' do diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index 36af38aef18..ef501f47f0d 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -259,6 +259,16 @@ RSpec.describe Issues::MoveService do it_behaves_like 'copy or reset relative position' end + + context 'issue with escalation status' do + it 'keeps the escalation status' do + escalation_status = create(:incident_management_issuable_escalation_status, issue: old_issue) + + move_service.execute(old_issue, new_project) + + expect(escalation_status.reload.issue).to eq(old_issue) + end + end end describe 'move permissions' do diff --git a/spec/services/issues/set_crm_contacts_service_spec.rb b/spec/services/issues/set_crm_contacts_service_spec.rb index 628f70efad6..2418f317551 100644 --- a/spec/services/issues/set_crm_contacts_service_spec.rb +++ b/spec/services/issues/set_crm_contacts_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Issues::SetCrmContactsService do let_it_be(:user) { create(:user) } - let_it_be(:group) { create(:group) } + let_it_be(:group) { create(:group, :crm_enabled) } let_it_be(:project) { create(:project, group: group) } let_it_be(:contacts) { create_list(:contact, 4, group: group) } diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 4739b7e0f28..11ed47b84d9 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Issues::UpdateService, :mailer do let_it_be(:user2) { create(:user) } let_it_be(:user3) { create(:user) } let_it_be(:guest) { create(:user) } - let_it_be(:group) { create(:group, :public) } + let_it_be(:group) { create(:group, :public, :crm_enabled) } let_it_be(:project, reload: true) { create(:project, :repository, group: group) } let_it_be(:label) { create(:label, project: project) } let_it_be(:label2) { create(:label, project: project) } @@ -22,10 +22,10 @@ RSpec.describe Issues::UpdateService, :mailer do end before_all do - project.add_maintainer(user) - project.add_developer(user2) - project.add_developer(user3) - project.add_guest(guest) + group.add_maintainer(user) + group.add_developer(user2) + group.add_developer(user3) + group.add_guest(guest) end describe 'execute' do @@ -191,11 +191,6 @@ RSpec.describe Issues::UpdateService, :mailer do end end - it 'adds a `incident` label if one does not exist' do - expect { update_issue(issue_type: 'incident') }.to change(issue.labels, :count).by(1) - expect(issue.labels.pluck(:title)).to eq(['incident']) - end - it 'creates system note about issue type' do update_issue(issue_type: 'incident') @@ -204,6 +199,13 @@ RSpec.describe Issues::UpdateService, :mailer do expect(note).not_to eq(nil) end + it 'creates an escalation status' do + expect { update_issue(issue_type: 'incident') } + .to change { issue.reload.incident_management_issuable_escalation_status } + .from(nil) + .to(a_kind_of(IncidentManagement::IssuableEscalationStatus)) + end + context 'for an issue with multiple labels' do let(:issue) { create(:incident, project: project, labels: [label_1]) } @@ -215,18 +217,6 @@ RSpec.describe Issues::UpdateService, :mailer do expect(issue.labels).to eq([label_1]) end end - - context 'filtering the incident label' do - let(:params) { { add_label_ids: [] } } - - before do - update_issue(issue_type: 'incident') - end - - it 'creates and add a incident label id to add_label_ids' do - expect(issue.label_ids).to contain_exactly(label_1.id) - end - end end context 'from incident to issue' do @@ -241,10 +231,8 @@ RSpec.describe Issues::UpdateService, :mailer do context 'for an incident with multiple labels' do let(:issue) { create(:incident, project: project, labels: [label_1, label_2]) } - it 'removes an `incident` label if one exists on the incident' do - expect { update_issue(issue_type: 'issue') }.to change(issue, :label_ids) - .from(containing_exactly(label_1.id, label_2.id)) - .to([label_2.id]) + it 'does not remove an `incident` label if one exists on the incident' do + expect { update_issue(issue_type: 'issue') }.to not_change(issue, :label_ids) end end @@ -252,10 +240,8 @@ RSpec.describe Issues::UpdateService, :mailer do let(:issue) { create(:incident, project: project, labels: [label_1, label_2]) } let(:params) { { label_ids: [label_1.id, label_2.id], remove_label_ids: [] } } - it 'adds an incident label id to remove_label_ids for it to be removed' do - expect { update_issue(issue_type: 'issue') }.to change(issue, :label_ids) - .from(containing_exactly(label_1.id, label_2.id)) - .to([label_2.id]) + it 'does not add an incident label id to remove_label_ids for it to be removed' do + expect { update_issue(issue_type: 'issue') }.to not_change(issue, :label_ids) end end end @@ -1157,6 +1143,83 @@ RSpec.describe Issues::UpdateService, :mailer do end end + context 'updating escalation status' do + let(:opts) { { escalation_status: { status: 'acknowledged' } } } + let(:escalation_update_class) { ::IncidentManagement::IssuableEscalationStatuses::AfterUpdateService } + + shared_examples 'updates the escalation status record' do |expected_status| + let(:service_double) { instance_double(escalation_update_class) } + + it 'has correct value' do + expect(escalation_update_class).to receive(:new).with(issue, user).and_return(service_double) + expect(service_double).to receive(:execute) + + update_issue(opts) + + expect(issue.escalation_status.status_name).to eq(expected_status) + end + end + + shared_examples 'does not change the status record' do + it 'retains the original value' do + expected_status = issue.escalation_status&.status_name + + update_issue(opts) + + expect(issue.escalation_status&.status_name).to eq(expected_status) + end + + it 'does not trigger side-effects' do + expect(escalation_update_class).not_to receive(:new) + + update_issue(opts) + end + end + + context 'when issue is an incident' do + let(:issue) { create(:incident, project: project) } + + context 'with an escalation status record' do + before do + create(:incident_management_issuable_escalation_status, issue: issue) + end + + it_behaves_like 'updates the escalation status record', :acknowledged + + context 'with associated alert' do + let!(:alert) { create(:alert_management_alert, issue: issue, project: project) } + + it 'syncs the update back to the alert' do + update_issue(opts) + + expect(issue.escalation_status.status_name).to eq(:acknowledged) + expect(alert.reload.status_name).to eq(:acknowledged) + end + end + + context 'with unsupported status value' do + let(:opts) { { escalation_status: { status: 'unsupported-status' } } } + + it_behaves_like 'does not change the status record' + end + + context 'with status value defined but unchanged' do + let(:opts) { { escalation_status: { status: issue.escalation_status.status_name } } } + + it_behaves_like 'does not change the status record' + end + end + + context 'without an escalation status record' do + it_behaves_like 'does not change the status record' + end + end + + context 'when issue type is not incident' do + it_behaves_like 'does not change the status record' + end + end + context 'duplicate issue' do let(:canonical_issue) { create(:issue, project: project) } diff --git a/spec/services/labels/transfer_service_spec.rb b/spec/services/labels/transfer_service_spec.rb index 05190accb33..e67ab6025a5 100644 --- a/spec/services/labels/transfer_service_spec.rb +++ b/spec/services/labels/transfer_service_spec.rb @@ -109,15 +109,5 @@ RSpec.describe Labels::TransferService do end end - context 'with use_optimized_group_labels_query FF on' do - it_behaves_like 'transfer labels' - end - - context 'with use_optimized_group_labels_query FF off' do - before do - stub_feature_flags(use_optimized_group_labels_query: false) - end - - it_behaves_like 'transfer labels' - end + it_behaves_like 'transfer labels' end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index b9f382d3cd8..1a1283b1078 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -424,7 +424,7 @@ RSpec.describe Members::DestroyService do end context 'deletion of invitations created by deleted project member' do - let(:user) { project.owner } + let(:user) { project.first_owner } let(:member_user) { create(:user) } let(:project) { create(:project) } diff --git a/spec/services/members/invite_service_spec.rb b/spec/services/members/invite_service_spec.rb index 7b9ae19f038..8ceb9979f33 100644 --- a/spec/services/members/invite_service_spec.rb +++ b/spec/services/members/invite_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_shared_state, :sidekiq_inline do let_it_be(:project, reload: true) { create(:project) } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let_it_be(:project_user) { create(:user) } let_it_be(:namespace) { project.namespace } diff --git a/spec/services/merge_requests/base_service_spec.rb b/spec/services/merge_requests/base_service_spec.rb index 7911392ef19..6eeba3029ae 100644 --- a/spec/services/merge_requests/base_service_spec.rb +++ b/spec/services/merge_requests/base_service_spec.rb @@ -17,7 +17,7 @@ RSpec.describe MergeRequests::BaseService do } end - subject { MergeRequests::CreateService.new(project: project, current_user: project.owner, params: params) } + subject { MergeRequests::CreateService.new(project: project, current_user: project.first_owner, params: params) } describe '#execute_hooks' do shared_examples 'enqueues Jira sync worker' do diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index 4b21812503e..e5bea0e7b14 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -6,7 +6,7 @@ RSpec.describe MergeRequests::SquashService do include GitHelpers let(:service) { described_class.new(project: project, current_user: user, params: { merge_request: merge_request }) } - let(:user) { project.owner } + let(:user) { project.first_owner } let(:project) { create(:project, :repository) } let(:repository) { project.repository.raw } let(:log_error) { "Failed to squash merge request #{merge_request.to_reference(full: true)}:" } diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 6ec2b158d30..2925dad7f6b 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -1132,7 +1132,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do context 'updating `force_remove_source_branch`' do let(:target_project) { create(:project, :repository, :public) } let(:source_project) { fork_project(target_project, nil, repository: true) } - let(:user) { target_project.owner } + let(:user) { target_project.first_owner } let(:merge_request) do create(:merge_request, source_project: source_project, diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 793e9ed9848..1fb50b07b3b 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -402,7 +402,7 @@ RSpec.describe Notes::CreateService do let_it_be(:design) { create(:design, :with_file) } let_it_be(:project) { design.project } - let_it_be(:user) { project.owner } + let_it_be(:user) { project.first_owner } let_it_be(:params) do { type: 'DiffNote', diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 24775ce06a4..9cbc16f0c95 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -2885,7 +2885,7 @@ RSpec.describe NotificationService, :mailer do let(:member) { create(:user) } before do - project.add_developer(member, current_user: project.owner) + project.add_developer(member, current_user: project.first_owner) end it do @@ -3287,7 +3287,7 @@ RSpec.describe NotificationService, :mailer do let_it_be(:domain, reload: true) { create(:pages_domain, project: project) } let_it_be(:u_blocked) { create(:user, :blocked) } let_it_be(:u_silence) { create_user_with_notification(:disabled, 'silent', project) } - let_it_be(:u_owner) { project.owner } + let_it_be(:u_owner) { project.first_owner } let_it_be(:u_maintainer1) { create(:user) } let_it_be(:u_maintainer2) { create(:user) } let_it_be(:u_developer) { create(:user) } @@ -3395,7 +3395,7 @@ RSpec.describe NotificationService, :mailer do let(:remote_mirror) { create(:remote_mirror, project: project) } let(:u_blocked) { create(:user, :blocked) } let(:u_silence) { create_user_with_notification(:disabled, 'silent-maintainer', project) } - let(:u_owner) { project.owner } + let(:u_owner) { project.first_owner } let(:u_maintainer1) { create(:user) } let(:u_maintainer2) { create(:user) } let(:u_developer) { create(:user) } @@ -3489,7 +3489,7 @@ RSpec.describe NotificationService, :mailer do it 'sends the email to owners and masters' do expect(Notify).to receive(:prometheus_alert_fired_email).with(project, master, alert).and_call_original - expect(Notify).to receive(:prometheus_alert_fired_email).with(project, project.owner, alert).and_call_original + expect(Notify).to receive(:prometheus_alert_fired_email).with(project, project.first_owner, alert).and_call_original expect(Notify).not_to receive(:prometheus_alert_fired_email).with(project, developer, alert) subject.prometheus_alerts_fired(project, [alert]) diff --git a/spec/services/packages/create_event_service_spec.rb b/spec/services/packages/create_event_service_spec.rb index 122f1e88ad0..58fa68b11fe 100644 --- a/spec/services/packages/create_event_service_spec.rb +++ b/spec/services/packages/create_event_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' RSpec.describe Packages::CreateEventService do - let(:scope) { 'container' } + let(:scope) { 'generic' } let(:event_name) { 'push_package' } let(:params) do @@ -75,24 +75,24 @@ RSpec.describe Packages::CreateEventService do context 'with a user' do let(:user) { create(:user) } - it_behaves_like 'db package event creation', 'user', 'container' - it_behaves_like 'redis package unique event creation', 'user', 'container' - it_behaves_like 'redis package count event creation', 'user', 'container' + it_behaves_like 'db package event creation', 'user', 'generic' + it_behaves_like 'redis package unique event creation', 'user', 'generic' + it_behaves_like 'redis package count event creation', 'user', 'generic' end context 'with a deploy token' do let(:user) { create(:deploy_token) } - it_behaves_like 'db package event creation', 'deploy_token', 'container' - it_behaves_like 'redis package unique event creation', 'deploy_token', 'container' - it_behaves_like 'redis package count event creation', 'deploy_token', 'container' + it_behaves_like 'db package event creation', 'deploy_token', 'generic' + it_behaves_like 'redis package unique event creation', 'deploy_token', 'generic' + it_behaves_like 'redis package count event creation', 'deploy_token', 'generic' end context 'with no user' do let(:user) { nil } - it_behaves_like 'db package event creation', 'guest', 'container' - it_behaves_like 'redis package count event creation', 'guest', 'container' + it_behaves_like 'db package event creation', 'guest', 'generic' + it_behaves_like 'redis package count event creation', 'guest', 'generic' end context 'with a package as scope' do diff --git a/spec/services/packages/maven/metadata/sync_service_spec.rb b/spec/services/packages/maven/metadata/sync_service_spec.rb index 30ddb48207a..a736ed281f0 100644 --- a/spec/services/packages/maven/metadata/sync_service_spec.rb +++ b/spec/services/packages/maven/metadata/sync_service_spec.rb @@ -265,4 +265,22 @@ RSpec.describe ::Packages::Maven::Metadata::SyncService do end end end + + # TODO When cleaning up packages_installable_package_files, consider adding a + # dummy package file pending for destruction on L10/11 and remove this context + context 'with package files pending destruction' do + let_it_be(:package_file_pending_destruction) { create(:package_file, :pending_destruction, package: versionless_package_for_versions, file_name: Packages::Maven::Metadata.filename) } + + subject { service.send(:metadata_package_file_for, versionless_package_for_versions) } + + it { is_expected.not_to eq(package_file_pending_destruction) } + + context 'with packages_installable_package_files disabled' do + before do + stub_feature_flags(packages_installable_package_files: false) + end + + it { is_expected.to eq(package_file_pending_destruction) } + end + end end diff --git a/spec/services/packages/terraform_module/create_package_service_spec.rb b/spec/services/packages/terraform_module/create_package_service_spec.rb index f911bb5b82c..e172aa726fd 100644 --- a/spec/services/packages/terraform_module/create_package_service_spec.rb +++ b/spec/services/packages/terraform_module/create_package_service_spec.rb @@ -37,7 +37,7 @@ RSpec.describe Packages::TerraformModule::CreatePackageService do let!(:existing_package) { create(:terraform_module_package, project: project2, name: 'foo/bar', version: '1.0.0') } it { expect(subject[:http_status]).to eq 403 } - it { expect(subject[:message]).to be 'Package already exists.' } + it { expect(subject[:message]).to be 'Access Denied' } end context 'version already exists' do diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 2aa9be5066f..d5fbf96ce74 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -119,7 +119,7 @@ RSpec.describe Projects::CreateService, '#execute' do project = create_project(user, opts) expect(project).to be_valid - expect(project.owner).to eq(user) + expect(project.first_owner).to eq(user) expect(project.team.maintainers).to include(user) expect(project.namespace).to eq(user.namespace) expect(project.project_namespace).to be_in_sync_with_project(project) @@ -154,6 +154,7 @@ RSpec.describe Projects::CreateService, '#execute' do expect(project).to be_persisted expect(project.owner).to eq(user) + expect(project.first_owner).to eq(user) expect(project.team.maintainers).to contain_exactly(user) expect(project.namespace).to eq(user.namespace) expect(project.project_namespace).to be_in_sync_with_project(project) diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index b22f276ee1f..9475f562d71 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -64,7 +64,7 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do create(:ci_pipeline_artifact, pipeline: pipeline) create_list(:ci_build_trace_chunk, 3, build: builds[0]) - expect { destroy_project(project, project.owner, {}) }.not_to exceed_query_limit(recorder) + expect { destroy_project(project, project.first_owner, {}) }.not_to exceed_query_limit(recorder) end it_behaves_like 'deleting the project' @@ -78,6 +78,11 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do end.not_to raise_error end + it 'reports the error' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).and_call_original + destroy_project(project, user, {}) + end + it 'unmarks the project as "pending deletion"' do destroy_project(project, user, {}) diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 3f58fa46806..ce30a20edf4 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -61,7 +61,7 @@ RSpec.describe Projects::ForkService do it { expect(to_project).to be_persisted } it { expect(to_project.errors).to be_empty } - it { expect(to_project.owner).to eq(@to_user) } + it { expect(to_project.first_owner).to eq(@to_user) } it { expect(to_project.namespace).to eq(@to_user.namespace) } it { expect(to_project.star_count).to be_zero } it { expect(to_project.description).to eq(@from_project.description) } @@ -274,7 +274,7 @@ RSpec.describe Projects::ForkService do expect(to_project).to be_persisted expect(to_project.errors).to be_empty - expect(to_project.owner).to eq(@group) + expect(to_project.first_owner).to eq(@group_owner) expect(to_project.namespace).to eq(@group) expect(to_project.name).to eq(@project.name) expect(to_project.path).to eq(@project.path) diff --git a/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 3bd96ad19bc..0d0bb317df2 100644 --- a/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -224,6 +224,78 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do end end end + + context 'when payload exceeds max amount of processable alerts' do + # We are defining 2 alerts in payload_raw above + let(:max_alerts) { 1 } + + before do + stub_const("#{described_class}::PROCESS_MAX_ALERTS", max_alerts) + + create(:prometheus_integration, project: project) + create(:project_alerting_setting, project: project, token: token) + + allow(Gitlab::AppLogger).to receive(:warn) + end + + shared_examples 'process truncated alerts' do + it 'returns 200 but skips processing and logs a warning', :aggregate_failures do + expect(subject).to be_success + expect(subject.payload[:alerts].size).to eq(max_alerts) + expect(Gitlab::AppLogger) + .to have_received(:warn) + .with( + message: 'Prometheus payload exceeded maximum amount of alerts. Truncating alerts.', + project_id: project.id, + alerts: { + total: 2, + max: max_alerts + }) + end + end + + shared_examples 'process all alerts' do + it 'returns 200 and process alerts without warnings', :aggregate_failures do + expect(subject).to be_success + expect(subject.payload[:alerts].size).to eq(2) + expect(Gitlab::AppLogger).not_to have_received(:warn) + end + end + + context 'with feature flag globally enabled' do + before do + stub_feature_flags(prometheus_notify_max_alerts: true) + end + + include_examples 'process truncated alerts' + end + + context 'with feature flag enabled on project' do + before do + stub_feature_flags(prometheus_notify_max_alerts: project) + end + + include_examples 'process truncated alerts' + end + + context 'with feature flag enabled on unrelated project' do + let(:another_project) { create(:project) } + + before do + stub_feature_flags(prometheus_notify_max_alerts: another_project) + end + + include_examples 'process all alerts' + end + + context 'with feature flag disabled' do + before do + stub_feature_flags(prometheus_notify_max_alerts: false) + end + + include_examples 'process all alerts' + end + end end context 'with invalid payload' do diff --git a/spec/services/projects/repository_languages_service_spec.rb b/spec/services/projects/repository_languages_service_spec.rb index cb61a7a1a3e..50d5fba6b84 100644 --- a/spec/services/projects/repository_languages_service_spec.rb +++ b/spec/services/projects/repository_languages_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Projects::RepositoryLanguagesService do - let(:service) { described_class.new(project, project.owner) } + let(:service) { described_class.new(project, project.first_owner) } context 'when detected_repository_languages flag is set' do let(:project) { create(:project) } diff --git a/spec/services/projects/update_pages_configuration_service_spec.rb b/spec/services/projects/update_pages_configuration_service_spec.rb deleted file mode 100644 index 58939ef4ada..00000000000 --- a/spec/services/projects/update_pages_configuration_service_spec.rb +++ /dev/null @@ -1,76 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Projects::UpdatePagesConfigurationService do - let(:service) { described_class.new(project) } - - describe "#execute" do - subject { service.execute } - - context 'when pages are deployed' do - let_it_be(:project) do - create(:project).tap(&:mark_pages_as_deployed) - end - - let(:file) { Tempfile.new('pages-test') } - - before do - allow(service).to receive(:pages_config_file).and_return(file.path) - end - - after do - file.close - file.unlink - end - - context 'when configuration changes' do - it 'updates the config and reloads the daemon' do - expect(service).to receive(:update_file).with(file.path, an_instance_of(String)) - .and_call_original - allow(service).to receive(:update_file).with(File.join(::Settings.pages.path, '.update'), - an_instance_of(String)).and_call_original - - expect(subject).to include(status: :success) - end - - it "doesn't update configuration files if updates on legacy storage are disabled" do - allow(Settings.pages.local_store).to receive(:enabled).and_return(false) - - expect(service).not_to receive(:update_file) - - expect(subject).to include(status: :success) - end - end - - context 'when configuration does not change' do - before do - # we set the configuration - service.execute - end - - it 'does not update anything' do - expect(service).not_to receive(:update_file) - - expect(subject).to include(status: :success) - end - end - end - - context 'when pages are not deployed' do - let_it_be(:project) do - create(:project).tap(&:mark_pages_as_not_deployed) - end - - it 'returns successfully' do - expect(subject).to eq(status: :success) - end - - it 'does not update the config' do - expect(service).not_to receive(:update_file) - - subject - end - end - end -end diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index f4a6d1b19e7..547641867bc 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -15,7 +15,7 @@ RSpec.describe Projects::UpdateRemoteMirrorService do subject(:execute!) { service.execute(remote_mirror, retries) } before do - project.repository.add_branch(project.owner, 'existing-branch', 'master') + project.repository.add_branch(project.first_owner, 'existing-branch', 'master') allow(remote_mirror) .to receive(:update_repository) @@ -131,32 +131,82 @@ RSpec.describe Projects::UpdateRemoteMirrorService do expect_next_instance_of(Lfs::PushService) do |service| expect(service).to receive(:execute) end + expect(Gitlab::AppJsonLogger).not_to receive(:info) execute! + + expect(remote_mirror.update_status).to eq('finished') + expect(remote_mirror.last_error).to be_nil end - it 'does nothing to an SSH repository' do - remote_mirror.update!(url: 'ssh://example.com') + context 'when LFS objects fail to push' do + before do + expect_next_instance_of(Lfs::PushService) do |service| + expect(service).to receive(:execute).and_return({ status: :error, message: 'unauthorized' }) + end + end + + context 'when remote_mirror_fail_on_lfs feature flag enabled' do + it 'fails update' do + expect(Gitlab::AppJsonLogger).to receive(:info).with( + hash_including(message: "Error synching remote mirror")).and_call_original - expect_any_instance_of(Lfs::PushService).not_to receive(:execute) + execute! - execute! - end + expect(remote_mirror.update_status).to eq('failed') + expect(remote_mirror.last_error).to eq("Error synchronizing LFS files:\n\nunauthorized\n\n") + end + end - it 'does nothing if LFS is disabled' do - expect(project).to receive(:lfs_enabled?) { false } + context 'when remote_mirror_fail_on_lfs feature flag is disabled' do + before do + stub_feature_flags(remote_mirror_fail_on_lfs: false) + end - expect_any_instance_of(Lfs::PushService).not_to receive(:execute) + it 'does not fail update' do + expect(Gitlab::AppJsonLogger).to receive(:info).with( + hash_including(message: "Error synching remote mirror")).and_call_original - execute! + execute! + + expect(remote_mirror.update_status).to eq('finished') + expect(remote_mirror.last_error).to be_nil + end + end end - it 'does nothing if non-password auth is specified' do - remote_mirror.update!(auth_method: 'ssh_public_key') + context 'with SSH repository' do + let(:ssh_mirror) { create(:remote_mirror, project: project, enabled: true) } + + before do + allow(ssh_mirror) + .to receive(:update_repository) + .and_return(double(divergent_refs: [])) + end + + it 'does nothing to an SSH repository' do + ssh_mirror.update!(url: 'ssh://example.com') - expect_any_instance_of(Lfs::PushService).not_to receive(:execute) + expect_any_instance_of(Lfs::PushService).not_to receive(:execute) - execute! + service.execute(ssh_mirror, retries) + end + + it 'does nothing if LFS is disabled' do + expect(project).to receive(:lfs_enabled?) { false } + + expect_any_instance_of(Lfs::PushService).not_to receive(:execute) + + service.execute(ssh_mirror, retries) + end + + it 'does nothing if non-password auth is specified' do + ssh_mirror.update!(auth_method: 'ssh_public_key') + + expect_any_instance_of(Lfs::PushService).not_to receive(:execute) + + service.execute(ssh_mirror, retries) + end end end end diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 4923ef169e8..7b5bf1db030 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -149,7 +149,7 @@ RSpec.describe Projects::UpdateService do describe 'when updating project that has forks' do let(:project) { create(:project, :internal) } - let(:user) { project.owner } + let(:user) { project.first_owner } let(:forked_project) { fork_project(project) } context 'and unlink forks feature flag is off' do @@ -379,52 +379,6 @@ RSpec.describe Projects::UpdateService do end end - shared_examples 'updating pages configuration' do - it 'schedules the `PagesUpdateConfigurationWorker` when pages are deployed' do - project.mark_pages_as_deployed - - expect(PagesUpdateConfigurationWorker).to receive(:perform_async).with(project.id) - - subject - end - - it "does not schedule a job when pages aren't deployed" do - project.mark_pages_as_not_deployed - - expect(PagesUpdateConfigurationWorker).not_to receive(:perform_async).with(project.id) - - subject - end - end - - context 'when updating #pages_https_only', :https_pages_enabled do - subject(:call_service) do - update_project(project, admin, pages_https_only: false) - end - - it 'updates the attribute' do - expect { call_service } - .to change { project.pages_https_only? } - .to(false) - end - - it_behaves_like 'updating pages configuration' - end - - context 'when updating #pages_access_level' do - subject(:call_service) do - update_project(project, admin, project_feature_attributes: { pages_access_level: ProjectFeature::ENABLED }) - end - - it 'updates the attribute' do - expect { call_service } - .to change { project.project_feature.pages_access_level } - .to(ProjectFeature::ENABLED) - end - - it_behaves_like 'updating pages configuration' - end - context 'when updating #emails_disabled' do it 'updates the attribute for the project owner' do expect { update_project(project, user, emails_disabled: true) } diff --git a/spec/services/protected_branches/create_service_spec.rb b/spec/services/protected_branches/create_service_spec.rb index 756c775be9b..0bea3edf203 100644 --- a/spec/services/protected_branches/create_service_spec.rb +++ b/spec/services/protected_branches/create_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe ProtectedBranches::CreateService do let(:project) { create(:project) } - let(:user) { project.owner } + let(:user) { project.first_owner } let(:params) do { name: name, diff --git a/spec/services/protected_branches/destroy_service_spec.rb b/spec/services/protected_branches/destroy_service_spec.rb index 47a048e7033..4e55c72f312 100644 --- a/spec/services/protected_branches/destroy_service_spec.rb +++ b/spec/services/protected_branches/destroy_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe ProtectedBranches::DestroyService do let(:protected_branch) { create(:protected_branch) } let(:project) { protected_branch.project } - let(:user) { project.owner } + let(:user) { project.first_owner } describe '#execute' do subject(:service) { described_class.new(project, user) } diff --git a/spec/services/protected_branches/update_service_spec.rb b/spec/services/protected_branches/update_service_spec.rb index b5cf1a54aff..3d9b77dcfc0 100644 --- a/spec/services/protected_branches/update_service_spec.rb +++ b/spec/services/protected_branches/update_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe ProtectedBranches::UpdateService do let(:protected_branch) { create(:protected_branch) } let(:project) { protected_branch.project } - let(:user) { project.owner } + let(:user) { project.first_owner } let(:params) { { name: new_name } } describe '#execute' do diff --git a/spec/services/protected_tags/create_service_spec.rb b/spec/services/protected_tags/create_service_spec.rb index 3d06cc9fb6c..31059d17f10 100644 --- a/spec/services/protected_tags/create_service_spec.rb +++ b/spec/services/protected_tags/create_service_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe ProtectedTags::CreateService do let(:project) { create(:project) } - let(:user) { project.owner } + let(:user) { project.first_owner } let(:params) do { name: name, diff --git a/spec/services/protected_tags/destroy_service_spec.rb b/spec/services/protected_tags/destroy_service_spec.rb index fbd1452a8d1..658a4f5557e 100644 --- a/spec/services/protected_tags/destroy_service_spec.rb +++ b/spec/services/protected_tags/destroy_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe ProtectedTags::DestroyService do let(:protected_tag) { create(:protected_tag) } let(:project) { protected_tag.project } - let(:user) { project.owner } + let(:user) { project.first_owner } describe '#execute' do subject(:service) { described_class.new(project, user) } diff --git a/spec/services/protected_tags/update_service_spec.rb b/spec/services/protected_tags/update_service_spec.rb index 22005bb9b89..8d301dcd825 100644 --- a/spec/services/protected_tags/update_service_spec.rb +++ b/spec/services/protected_tags/update_service_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe ProtectedTags::UpdateService do let(:protected_tag) { create(:protected_tag) } let(:project) { protected_tag.project } - let(:user) { project.owner } + let(:user) { project.first_owner } let(:params) { { name: new_name } } describe '#execute' do diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 77d263f4b70..e56e54db6f4 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe QuickActions::InterpretService do - let_it_be(:group) { create(:group) } + let_it_be(:group) { create(:group, :crm_enabled) } let_it_be(:public_project) { create(:project, :public, group: group) } let_it_be(:repository_project) { create(:project, :repository) } let_it_be(:project) { public_project } diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index 42520ea26b2..5a88929334b 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -7,10 +7,14 @@ RSpec.describe ResourceAccessTokens::CreateService do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project, :private) } + let_it_be(:group) { create(:group, :private) } let_it_be(:params) { {} } + before do + stub_config_setting(host: 'example.com') + end + describe '#execute' do - # Created shared_examples as it will easy to include specs for group bots in https://gitlab.com/gitlab-org/gitlab/-/issues/214046 shared_examples 'token creation fails' do let(:resource) { create(:project)} @@ -31,7 +35,7 @@ RSpec.describe ResourceAccessTokens::CreateService do access_token = response.payload[:access_token] - expect(access_token.user.reload.user_type).to eq("#{resource_type}_bot") + expect(access_token.user.reload.user_type).to eq("project_bot") expect(access_token.user.created_by_id).to eq(user.id) end @@ -88,6 +92,15 @@ RSpec.describe ResourceAccessTokens::CreateService do end end + context 'bot email' do + it 'check email domain' do + response = subject + access_token = response.payload[:access_token] + + expect(access_token.user.email).to end_with("@noreply.#{Gitlab.config.gitlab.host}") + end + end + context 'access level' do context 'when user does not specify an access level' do it 'adds the bot user as a maintainer in the resource' do @@ -112,10 +125,8 @@ RSpec.describe ResourceAccessTokens::CreateService do end context 'when user is external' do - let(:user) { create(:user, :external) } - before do - project.add_maintainer(user) + user.update!(external: true) end it 'creates resource bot user with external status' do @@ -162,7 +173,7 @@ RSpec.describe ResourceAccessTokens::CreateService do access_token = response.payload[:access_token] project_bot = access_token.user - expect(project.members.find_by(user_id: project_bot.id).expires_at).to eq(nil) + expect(resource.members.find_by(user_id: project_bot.id).expires_at).to eq(nil) end end end @@ -183,7 +194,7 @@ RSpec.describe ResourceAccessTokens::CreateService do access_token = response.payload[:access_token] project_bot = access_token.user - expect(project.members.find_by(user_id: project_bot.id).expires_at).to eq(params[:expires_at]) + expect(resource.members.find_by(user_id: project_bot.id).expires_at).to eq(params[:expires_at]) end end end @@ -234,24 +245,41 @@ RSpec.describe ResourceAccessTokens::CreateService do end end + shared_examples 'when user does not have permission to create a resource bot' do + it_behaves_like 'token creation fails' + + it 'returns the permission error message' do + response = subject + + expect(response.error?).to be true + expect(response.errors).to include("User does not have permission to create #{resource_type} access token") + end + end + context 'when resource is a project' do let_it_be(:resource_type) { 'project' } let_it_be(:resource) { project } - context 'when user does not have permission to create a resource bot' do - it_behaves_like 'token creation fails' - - it 'returns the permission error message' do - response = subject + it_behaves_like 'when user does not have permission to create a resource bot' - expect(response.error?).to be true - expect(response.errors).to include("User does not have permission to create #{resource_type} access token") + context 'user with valid permission' do + before_all do + resource.add_maintainer(user) end + + it_behaves_like 'allows creation of bot with valid params' end + end + + context 'when resource is a project' do + let_it_be(:resource_type) { 'group' } + let_it_be(:resource) { group } + + it_behaves_like 'when user does not have permission to create a resource bot' context 'user with valid permission' do before_all do - resource.add_maintainer(user) + resource.add_owner(user) end it_behaves_like 'allows creation of bot with valid params' diff --git a/spec/services/resource_access_tokens/revoke_service_spec.rb b/spec/services/resource_access_tokens/revoke_service_spec.rb index 4f4e2ab0c99..3d724a79fef 100644 --- a/spec/services/resource_access_tokens/revoke_service_spec.rb +++ b/spec/services/resource_access_tokens/revoke_service_spec.rb @@ -6,11 +6,12 @@ RSpec.describe ResourceAccessTokens::RevokeService do subject { described_class.new(user, resource, access_token).execute } let_it_be(:user) { create(:user) } + let_it_be(:user_non_priviledged) { create(:user) } + let_it_be(:resource_bot) { create(:user, :project_bot) } let(:access_token) { create(:personal_access_token, user: resource_bot) } describe '#execute', :sidekiq_inline do - # Created shared_examples as it will easy to include specs for group bots in https://gitlab.com/gitlab-org/gitlab/-/issues/214046 shared_examples 'revokes access token' do it { expect(subject.success?).to be true } @@ -79,71 +80,80 @@ RSpec.describe ResourceAccessTokens::RevokeService do end end - context 'when resource is a project' do - let_it_be(:resource) { create(:project, :private) } + shared_examples 'revoke fails' do |resource_type| + let_it_be(:other_user) { create(:user) } - let(:resource_bot) { create(:user, :project_bot) } + context "when access token does not belong to this #{resource_type}" do + it 'does not find the bot' do + other_access_token = create(:personal_access_token, user: other_user) - before do - resource.add_maintainer(user) - resource.add_maintainer(resource_bot) - end + response = described_class.new(user, resource, other_access_token).execute - it_behaves_like 'revokes access token' + expect(response.success?).to be false + expect(response.message).to eq("Failed to find bot user") + expect(access_token.reload.revoked?).to be false + end + end - context 'revoke fails' do - let_it_be(:other_user) { create(:user) } + context 'when user does not have permission to destroy bot' do + context "when non-#{resource_type} member tries to delete project bot" do + it 'does not allow other user to delete bot' do + response = described_class.new(other_user, resource, access_token).execute - context 'when access token does not belong to this project' do - it 'does not find the bot' do - other_access_token = create(:personal_access_token, user: other_user) + expect(response.success?).to be false + expect(response.message).to eq("#{other_user.name} cannot delete #{access_token.user.name}") + expect(access_token.reload.revoked?).to be false + end + end - response = described_class.new(user, resource, other_access_token).execute + context "when non-priviledged #{resource_type} member tries to delete project bot" do + it 'does not allow developer to delete bot' do + response = described_class.new(user_non_priviledged, resource, access_token).execute expect(response.success?).to be false - expect(response.message).to eq("Failed to find bot user") + expect(response.message).to eq("#{user_non_priviledged.name} cannot delete #{access_token.user.name}") expect(access_token.reload.revoked?).to be false end end + end - context 'when user does not have permission to destroy bot' do - context 'when non-project member tries to delete project bot' do - it 'does not allow other user to delete bot' do - response = described_class.new(other_user, resource, access_token).execute - - expect(response.success?).to be false - expect(response.message).to eq("#{other_user.name} cannot delete #{access_token.user.name}") - expect(access_token.reload.revoked?).to be false - end + context 'when deletion of bot user fails' do + before do + allow_next_instance_of(::ResourceAccessTokens::RevokeService) do |service| + allow(service).to receive(:execute).and_return(false) end + end + + it_behaves_like 'rollback revoke steps' + end + end - context 'when non-maintainer project member tries to delete project bot' do - let(:developer) { create(:user) } + context 'when resource is a project' do + let_it_be(:resource) { create(:project, :private) } - before do - resource.add_developer(developer) - end + before do + resource.add_maintainer(user) + resource.add_developer(user_non_priviledged) + resource.add_maintainer(resource_bot) + end - it 'does not allow developer to delete bot' do - response = described_class.new(developer, resource, access_token).execute + it_behaves_like 'revokes access token' - expect(response.success?).to be false - expect(response.message).to eq("#{developer.name} cannot delete #{access_token.user.name}") - expect(access_token.reload.revoked?).to be false - end - end - end + it_behaves_like 'revoke fails', 'project' + end - context 'when deletion of bot user fails' do - before do - allow_next_instance_of(::ResourceAccessTokens::RevokeService) do |service| - allow(service).to receive(:execute).and_return(false) - end - end + context 'when resource is a group' do + let_it_be(:resource) { create(:group, :private) } - it_behaves_like 'rollback revoke steps' - end + before do + resource.add_owner(user) + resource.add_maintainer(user_non_priviledged) + resource.add_maintainer(resource_bot) end + + it_behaves_like 'revokes access token' + + it_behaves_like 'revoke fails', 'group' end end end diff --git a/spec/services/service_ping/submit_service_ping_service_spec.rb b/spec/services/service_ping/submit_service_ping_service_spec.rb index ca387690e83..2971c9a9309 100644 --- a/spec/services/service_ping/submit_service_ping_service_spec.rb +++ b/spec/services/service_ping/submit_service_ping_service_spec.rb @@ -110,6 +110,7 @@ RSpec.describe ServicePing::SubmitService do context 'when product_intelligence_enabled is true' do before do stub_usage_data_connections + stub_database_flavor_check allow(ServicePing::ServicePingSettings).to receive(:product_intelligence_enabled?).and_return(true) end @@ -126,6 +127,7 @@ RSpec.describe ServicePing::SubmitService do context 'when usage ping is enabled' do before do stub_usage_data_connections + stub_database_flavor_check stub_application_setting(usage_ping_enabled: true) end diff --git a/spec/services/test_hooks/system_service_spec.rb b/spec/services/test_hooks/system_service_spec.rb index a13ae471b4b..48c8c24212a 100644 --- a/spec/services/test_hooks/system_service_spec.rb +++ b/spec/services/test_hooks/system_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe TestHooks::SystemService do let_it_be(:project) { create(:project, :repository) } let(:hook) { create(:system_hook) } - let(:service) { described_class.new(hook, project.owner, trigger) } + let(:service) { described_class.new(hook, project.first_owner, trigger) } let(:success_result) { { status: :success, http_status: 200, message: 'ok' } } before do diff --git a/spec/services/users/create_service_spec.rb b/spec/services/users/create_service_spec.rb index 74340bac055..ab9da82e91c 100644 --- a/spec/services/users/create_service_spec.rb +++ b/spec/services/users/create_service_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Users::CreateService do context 'when required parameters are provided' do let(:params) do - { name: 'John Doe', username: 'jduser', email: email, password: 'mydummypass' } + { name: 'John Doe', username: 'jduser', email: email, password: Gitlab::Password.test_default } end it 'returns a persisted user' do @@ -82,13 +82,13 @@ RSpec.describe Users::CreateService do context 'when force_random_password parameter is true' do let(:params) do - { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass', force_random_password: true } + { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: Gitlab::Password.test_default, force_random_password: true } end it 'generates random password' do user = service.execute - expect(user.password).not_to eq 'mydummypass' + expect(user.password).not_to eq Gitlab::Password.test_default expect(user.password).to be_present end end @@ -99,7 +99,7 @@ RSpec.describe Users::CreateService do name: 'John Doe', username: 'jduser', email: 'jd@example.com', - password: 'mydummypass', + password: Gitlab::Password.test_default, password_automatically_set: true } end @@ -121,7 +121,7 @@ RSpec.describe Users::CreateService do context 'when skip_confirmation parameter is true' do let(:params) do - { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass', skip_confirmation: true } + { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: Gitlab::Password.test_default, skip_confirmation: true } end it 'confirms the user' do @@ -131,7 +131,7 @@ RSpec.describe Users::CreateService do context 'when reset_password parameter is true' do let(:params) do - { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass', reset_password: true } + { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: Gitlab::Password.test_default, reset_password: true } end it 'resets password even if a password parameter is given' do @@ -152,7 +152,7 @@ RSpec.describe Users::CreateService do context 'with nil user' do let(:params) do - { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass', skip_confirmation: true } + { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: Gitlab::Password.test_default, skip_confirmation: true } end let(:service) { described_class.new(nil, params) } diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb index aa4df93a241..a31902c7f16 100644 --- a/spec/services/users/refresh_authorized_projects_service_spec.rb +++ b/spec/services/users/refresh_authorized_projects_service_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Users::RefreshAuthorizedProjectsService do # triggered twice. let!(:project) { create(:project) } - let(:user) { project.namespace.owner } + let(:user) { project.namespace.first_owner } let(:service) { described_class.new(user) } describe '#execute', :clean_gitlab_redis_shared_state do diff --git a/spec/services/users/upsert_credit_card_validation_service_spec.rb b/spec/services/users/upsert_credit_card_validation_service_spec.rb index 952d482f1bd..ac7e619612f 100644 --- a/spec/services/users/upsert_credit_card_validation_service_spec.rb +++ b/spec/services/users/upsert_credit_card_validation_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Users::UpsertCreditCardValidationService do - let_it_be(:user) { create(:user) } + let_it_be(:user) { create(:user, requires_credit_card_verification: true) } let(:user_id) { user.id } let(:credit_card_validated_time) { Time.utc(2020, 1, 1) } @@ -21,7 +21,7 @@ RSpec.describe Users::UpsertCreditCardValidationService do end describe '#execute' do - subject(:service) { described_class.new(params) } + subject(:service) { described_class.new(params, user) } context 'successfully set credit card validation record for the user' do context 'when user does not have credit card validation record' do @@ -42,6 +42,10 @@ RSpec.describe Users::UpsertCreditCardValidationService do expiration_date: Date.new(expiration_year, 1, 31) ) end + + it 'sets the requires_credit_card_verification attribute on the user to false' do + expect { service.execute }.to change { user.reload.requires_credit_card_verification }.to(false) + end end context 'when user has credit card validation record' do diff --git a/spec/services/verify_pages_domain_service_spec.rb b/spec/services/verify_pages_domain_service_spec.rb index 2a3b3814065..42f7ebc85f9 100644 --- a/spec/services/verify_pages_domain_service_spec.rb +++ b/spec/services/verify_pages_domain_service_spec.rb @@ -269,56 +269,6 @@ RSpec.describe VerifyPagesDomainService do end end - context 'pages configuration updates' do - context 'enabling a disabled domain' do - let(:domain) { create(:pages_domain, :disabled) } - - it 'schedules an update' do - stub_resolver(domain.domain => domain.verification_code) - - expect(domain).to receive(:update_daemon) - - service.execute - end - end - - context 'verifying an enabled domain' do - let(:domain) { create(:pages_domain) } - - it 'schedules an update' do - stub_resolver(domain.domain => domain.verification_code) - - expect(domain).not_to receive(:update_daemon) - - service.execute - end - end - - context 'disabling an expired domain' do - let(:domain) { create(:pages_domain, :expired) } - - it 'schedules an update' do - stub_resolver - - expect(domain).to receive(:update_daemon) - - service.execute - end - end - - context 'failing to verify a disabled domain' do - let(:domain) { create(:pages_domain, :disabled) } - - it 'does not schedule an update' do - stub_resolver - - expect(domain).not_to receive(:update_daemon) - - service.execute - end - end - end - context 'no verification code' do let(:domain) { create(:pages_domain) } diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 2aebd2adab9..7d933ea9c5c 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -2,20 +2,12 @@ require 'spec_helper' -RSpec.describe WebHookService do +RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state do include StubRequests let_it_be(:project) { create(:project) } let_it_be_with_reload(:project_hook) { create(:project_hook, project: project) } - let(:headers) do - { - 'Content-Type' => 'application/json', - 'User-Agent' => "GitLab/#{Gitlab::VERSION}", - 'X-Gitlab-Event' => 'Push Hook' - } - end - let(:data) do { before: 'oldrev', after: 'newrev', ref: 'ref' } end @@ -61,6 +53,21 @@ RSpec.describe WebHookService do end describe '#execute' do + let!(:uuid) { SecureRandom.uuid } + let(:headers) do + { + 'Content-Type' => 'application/json', + 'User-Agent' => "GitLab/#{Gitlab::VERSION}", + 'X-Gitlab-Event' => 'Push Hook', + 'X-Gitlab-Event-UUID' => uuid + } + end + + before do + # Set a stable value for the `X-Gitlab-Event-UUID` header. + Gitlab::WebHooks::RecursionDetection.set_request_uuid(uuid) + end + context 'when token is defined' do let_it_be(:project_hook) { create(:project_hook, :token) } @@ -127,11 +134,74 @@ RSpec.describe WebHookService do expect(service_instance.execute).to eq({ status: :error, message: 'Hook disabled' }) end + it 'executes and registers the hook with the recursion detection', :aggregate_failures do + stub_full_request(project_hook.url, method: :post) + cache_key = Gitlab::WebHooks::RecursionDetection.send(:cache_key_for_hook, project_hook) + + ::Gitlab::Redis::SharedState.with do |redis| + expect { service_instance.execute }.to change { + redis.sismember(cache_key, project_hook.id) + }.to(true) + end + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)) + .with(headers: headers) + .once + end + + it 'executes and logs if a recursive web hook is detected', :aggregate_failures do + stub_full_request(project_hook.url, method: :post) + Gitlab::WebHooks::RecursionDetection.register!(project_hook) + + expect(Gitlab::AuthLogger).to receive(:error).with( + include( + message: 'Webhook recursion detected and will be blocked in future', + hook_id: project_hook.id, + hook_type: 'ProjectHook', + hook_name: 'push_hooks', + recursion_detection: Gitlab::WebHooks::RecursionDetection.to_log(project_hook), + 'correlation_id' => kind_of(String) + ) + ) + + service_instance.execute + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)) + .with(headers: headers) + .once + end + + it 'executes and logs if the recursion count limit would be exceeded', :aggregate_failures do + stub_full_request(project_hook.url, method: :post) + stub_const("#{Gitlab::WebHooks::RecursionDetection.name}::COUNT_LIMIT", 3) + previous_hooks = create_list(:project_hook, 3) + previous_hooks.each { Gitlab::WebHooks::RecursionDetection.register!(_1) } + + expect(Gitlab::AuthLogger).to receive(:error).with( + include( + message: 'Webhook recursion detected and will be blocked in future', + hook_id: project_hook.id, + hook_type: 'ProjectHook', + hook_name: 'push_hooks', + recursion_detection: Gitlab::WebHooks::RecursionDetection.to_log(project_hook), + 'correlation_id' => kind_of(String) + ) + ) + + service_instance.execute + + expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)) + .with(headers: headers) + .once + end + it 'handles exceptions' do exceptions = Gitlab::HTTP::HTTP_ERRORS + [ Gitlab::Json::LimitedEncoder::LimitExceeded, URI::InvalidURIError ] + allow(Gitlab::WebHooks::RecursionDetection).to receive(:block?).and_return(false) + exceptions.each do |exception_class| exception = exception_class.new('Exception message') project_hook.enable! @@ -420,6 +490,57 @@ RSpec.describe WebHookService do end end + context 'recursion detection' do + before do + # Set a request UUID so `RecursionDetection.block?` will query redis. + Gitlab::WebHooks::RecursionDetection.set_request_uuid(SecureRandom.uuid) + end + + it 'queues a worker and logs an error if the call chain limit would be exceeded' do + stub_const("#{Gitlab::WebHooks::RecursionDetection.name}::COUNT_LIMIT", 3) + previous_hooks = create_list(:project_hook, 3) + previous_hooks.each { Gitlab::WebHooks::RecursionDetection.register!(_1) } + + expect(WebHookWorker).to receive(:perform_async) + expect(Gitlab::AuthLogger).to receive(:error).with( + include( + message: 'Webhook recursion detected and will be blocked in future', + hook_id: project_hook.id, + hook_type: 'ProjectHook', + hook_name: 'push_hooks', + recursion_detection: Gitlab::WebHooks::RecursionDetection.to_log(project_hook), + 'correlation_id' => kind_of(String), + 'meta.project' => project.full_path, + 'meta.related_class' => 'ProjectHook', + 'meta.root_namespace' => project.root_namespace.full_path + ) + ) + + service_instance.async_execute + end + + it 'queues a worker and logs an error if a recursive call chain is detected' do + Gitlab::WebHooks::RecursionDetection.register!(project_hook) + + expect(WebHookWorker).to receive(:perform_async) + expect(Gitlab::AuthLogger).to receive(:error).with( + include( + message: 'Webhook recursion detected and will be blocked in future', + hook_id: project_hook.id, + hook_type: 'ProjectHook', + hook_name: 'push_hooks', + recursion_detection: Gitlab::WebHooks::RecursionDetection.to_log(project_hook), + 'correlation_id' => kind_of(String), + 'meta.project' => project.full_path, + 'meta.related_class' => 'ProjectHook', + 'meta.root_namespace' => project.root_namespace.full_path + ) + ) + + service_instance.async_execute + end + end + context 'when hook has custom context attributes' do it 'includes the attributes in the worker context' do expect(WebHookWorker).to receive(:perform_async) do diff --git a/spec/services/work_items/build_service_spec.rb b/spec/services/work_items/build_service_spec.rb new file mode 100644 index 00000000000..6b2e2d8819e --- /dev/null +++ b/spec/services/work_items/build_service_spec.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::BuildService do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:guest) { create(:user) } + + let(:user) { guest } + + before_all do + project.add_guest(guest) + end + + describe '#execute' do + subject { described_class.new(project: project, current_user: user, params: {}).execute } + + it { is_expected.to be_a(::WorkItem) } + end +end diff --git a/spec/services/work_items/create_service_spec.rb b/spec/services/work_items/create_service_spec.rb new file mode 100644 index 00000000000..2c054ae59a0 --- /dev/null +++ b/spec/services/work_items/create_service_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::CreateService do + include AfterNextHelpers + + let_it_be(:group) { create(:group) } + let_it_be_with_reload(:project) { create(:project, group: group) } + let_it_be(:user) { create(:user) } + + let(:spam_params) { double } + + describe '#execute' do + let(:work_item) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute } + + before do + stub_spam_services + end + + context 'when params are valid' do + before_all do + project.add_guest(user) + end + + let(:opts) do + { + title: 'Awesome work_item', + description: 'please fix' + } + end + + it 'created instance is a WorkItem' do + expect(Issuable::CommonSystemNotesService).to receive_message_chain(:new, :execute) + + expect(work_item).to be_persisted + expect(work_item).to be_a(::WorkItem) + expect(work_item.title).to eq('Awesome work_item') + expect(work_item.description).to eq('please fix') + expect(work_item.work_item_type.base_type).to eq('issue') + end + end + + context 'checking spam' do + let(:params) do + { + title: 'Spam work_item' + } + end + + subject do + described_class.new(project: project, current_user: user, params: params, spam_params: spam_params) + end + + it 'executes SpamActionService' do + expect_next_instance_of( + Spam::SpamActionService, + { + spammable: kind_of(WorkItem), + spam_params: spam_params, + user: an_instance_of(User), + action: :create + } + ) do |instance| + expect(instance).to receive(:execute) + end + + subject.execute + end + end + end +end |