diff options
Diffstat (limited to 'app/services')
110 files changed, 1466 insertions, 534 deletions
diff --git a/app/services/alert_management/alerts/update_service.rb b/app/services/alert_management/alerts/update_service.rb index 464d5f2ecea..089715a42fb 100644 --- a/app/services/alert_management/alerts/update_service.rb +++ b/app/services/alert_management/alerts/update_service.rb @@ -37,7 +37,6 @@ module AlertManagement private attr_reader :alert, :current_user, :params, :param_errors, :status - delegate :resolved?, to: :alert def allowed? current_user&.can?(:update_alert_management_alert, alert) @@ -129,7 +128,7 @@ module AlertManagement def handle_status_change add_status_change_system_note - resolve_todos if resolved? + resolve_todos if alert.resolved? end def add_status_change_system_note @@ -177,3 +176,5 @@ module AlertManagement end end end + +AlertManagement::Alerts::UpdateService.prepend_mod diff --git a/app/services/application_settings/update_service.rb b/app/services/application_settings/update_service.rb index 7728982779e..0f2099793ea 100644 --- a/app/services/application_settings/update_service.rb +++ b/app/services/application_settings/update_service.rb @@ -67,10 +67,8 @@ module ApplicationSettings end def update_terms(terms) - return unless terms.present? - # Avoid creating a new terms record if the text is exactly the same. - terms = terms.strip + terms = terms&.strip return if terms == @application_setting.terms ApplicationSetting::Term.create(terms: terms) diff --git a/app/services/audit_event_service.rb b/app/services/audit_event_service.rb index 60421f61007..558798c830d 100644 --- a/app/services/audit_event_service.rb +++ b/app/services/audit_event_service.rb @@ -16,7 +16,7 @@ class AuditEventService @author = build_author(author) @entity = entity @details = details - @ip_address = resolve_ip_address(@details, @author) + @ip_address = resolve_ip_address(@author) end # Builds the @details attribute for authentication @@ -64,9 +64,8 @@ class AuditEventService end end - def resolve_ip_address(details, author) - details[:ip_address].presence || - Gitlab::RequestContext.instance.client_ip || + def resolve_ip_address(author) + Gitlab::RequestContext.instance.client_ip || author.current_sign_in_ip end diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb index 5fde346c4ab..d42dcb2fd00 100644 --- a/app/services/auth/container_registry_authentication_service.rb +++ b/app/services/auth/container_registry_authentication_service.rb @@ -115,7 +115,25 @@ module Auth # ensure_container_repository!(path, authorized_actions) - { type: type, name: path.to_s, actions: authorized_actions } + { + type: type, + name: path.to_s, + actions: authorized_actions, + migration_eligible: migration_eligible(requested_project, authorized_actions) + }.compact + end + + def migration_eligible(project, actions) + return unless actions.include?('push') + return unless Feature.enabled?(:container_registry_migration_phase1) + + # The migration process will start by allowing only specific test and gitlab-org projects using the + # `container_registry_migration_phase1_allow` FF. We'll then move on to a percentage rollout using this same FF. + # To remove the risk of impacting enterprise customers that rely heavily on the registry during the percentage + # rollout, we'll add their top-level group/namespace to the `container_registry_migration_phase1_deny` FF. Later, + # we'll remove them manually from this deny list, and their new repositories will become eligible. + Feature.disabled?(:container_registry_migration_phase1_deny, project.root_ancestor) && + Feature.enabled?(:container_registry_migration_phase1_allow, project) end ## diff --git a/app/services/base_service.rb b/app/services/base_service.rb index 7ab87a1af09..3030287e035 100644 --- a/app/services/base_service.rb +++ b/app/services/base_service.rb @@ -14,6 +14,7 @@ # or, create a new base class and update this comment. class BaseService include BaseServiceUtility + include Gitlab::Experiment::Dsl attr_accessor :project, :current_user, :params diff --git a/app/services/boards/issues/create_service.rb b/app/services/boards/issues/create_service.rb index 0639acfb399..e3d4da7fb07 100644 --- a/app/services/boards/issues/create_service.rb +++ b/app/services/boards/issues/create_service.rb @@ -30,7 +30,9 @@ module Boards end def create_issue(params) - ::Issues::CreateService.new(project: project, current_user: current_user, params: params).execute + # NOTE: We are intentionally not doing a spam/CAPTCHA check for issues created via boards. + # See https://gitlab.com/gitlab-org/gitlab/-/issues/29400#note_598479184 for more context. + ::Issues::CreateService.new(project: project, current_user: current_user, params: params, spam_params: nil).execute end end end diff --git a/app/services/branches/create_service.rb b/app/services/branches/create_service.rb index 848e6aaa65a..b5faf2ec281 100644 --- a/app/services/branches/create_service.rb +++ b/app/services/branches/create_service.rb @@ -9,12 +9,16 @@ module Branches return result if result[:status] == :error - new_branch = repository.add_branch(current_user, branch_name, ref) + begin + new_branch = repository.add_branch(current_user, branch_name, ref) + rescue Gitlab::Git::CommandError => e + return error("Failed to create branch '#{branch_name}': #{e}") + end if new_branch success(new_branch) else - error("Invalid reference name: #{ref}") + error("Failed to create branch '#{branch_name}': invalid reference name '#{ref}'") end rescue Gitlab::Git::PreReceiveError => e Gitlab::ErrorTracking.track_exception(e, pre_receive_message: e.raw_message, branch_name: branch_name, ref: ref) diff --git a/app/services/bulk_create_integration_service.rb b/app/services/bulk_create_integration_service.rb index adb989be218..a7fe4c776b7 100644 --- a/app/services/bulk_create_integration_service.rb +++ b/app/services/bulk_create_integration_service.rb @@ -8,7 +8,7 @@ class BulkCreateIntegrationService end def execute - service_list = ServiceList.new(batch, service_hash, association).to_array + service_list = ServiceList.new(batch, integration_hash, association).to_array Integration.transaction do results = bulk_insert(*service_list) @@ -31,11 +31,11 @@ class BulkCreateIntegrationService klass.insert_all(items_to_insert, returning: [:id]) end - def service_hash + def integration_hash if integration.template? - integration.to_service_hash + integration.to_integration_hash else - integration.to_service_hash.tap { |json| json['inherit_from_id'] = integration.inherit_from_id || integration.id } + integration.to_integration_hash.tap { |json| json['inherit_from_id'] = integration.inherit_from_id || integration.id } end end diff --git a/app/services/bulk_imports/file_download_service.rb b/app/services/bulk_imports/file_download_service.rb index c5a1241e0a4..9a301c260a9 100644 --- a/app/services/bulk_imports/file_download_service.rb +++ b/app/services/bulk_imports/file_download_service.rb @@ -1,18 +1,20 @@ # frozen_string_literal: true +# Downloads a remote file. If no filename is given, it'll use the remote filename module BulkImports class FileDownloadService - FILE_SIZE_LIMIT = 5.gigabytes - ALLOWED_CONTENT_TYPES = %w(application/gzip application/octet-stream).freeze - ServiceError = Class.new(StandardError) - def initialize(configuration:, relative_url:, dir:, filename:) + REMOTE_FILENAME_PATTERN = %r{filename="(?<filename>[^"]+)"}.freeze + FILENAME_SIZE_LIMIT = 255 # chars before the extension + + def initialize(configuration:, relative_url:, dir:, file_size_limit:, allowed_content_types:, filename: nil) @configuration = configuration @relative_url = relative_url @filename = filename @dir = dir - @filepath = File.join(@dir, @filename) + @file_size_limit = file_size_limit + @allowed_content_types = allowed_content_types end def execute @@ -30,7 +32,7 @@ module BulkImports private - attr_reader :configuration, :relative_url, :dir, :filename, :filepath + attr_reader :configuration, :relative_url, :dir, :file_size_limit, :allowed_content_types def download_file File.open(filepath, 'wb') do |file| @@ -39,7 +41,7 @@ module BulkImports http_client.stream(relative_url) do |chunk| bytes_downloaded += chunk.size - raise(ServiceError, 'Invalid downloaded file') if bytes_downloaded > FILE_SIZE_LIMIT + validate_size!(bytes_downloaded) raise(ServiceError, "File download error #{chunk.code}") unless chunk.code == 200 file.write(chunk) @@ -53,7 +55,7 @@ module BulkImports def http_client @http_client ||= BulkImports::Clients::HTTP.new( - uri: configuration.url, + url: configuration.url, token: configuration.access_token ) end @@ -88,15 +90,59 @@ module BulkImports end def validate_content_length - content_size = headers['content-length'] + validate_size!(headers['content-length']) + end - raise(ServiceError, 'Invalid content length') if content_size.blank? || content_size.to_i > FILE_SIZE_LIMIT + def validate_size!(size) + if size.blank? + raise ServiceError, 'Missing content-length header' + elsif size.to_i > file_size_limit + raise ServiceError, "File size %{size} exceeds limit of %{limit}" % { + size: ActiveSupport::NumberHelper.number_to_human_size(size), + limit: ActiveSupport::NumberHelper.number_to_human_size(file_size_limit) + } + end end def validate_content_type content_type = headers['content-type'] - raise(ServiceError, 'Invalid content type') if content_type.blank? || ALLOWED_CONTENT_TYPES.exclude?(content_type) + raise(ServiceError, 'Invalid content type') if content_type.blank? || allowed_content_types.exclude?(content_type) + end + + def filepath + @filepath ||= File.join(@dir, filename) + end + + def filename + @filename.presence || remote_filename + end + + # Fetch the remote filename information from the request content-disposition header + # - Raises if the filename does not exist + # - If the filename is longer then 255 chars truncate it + # to be a total of 255 chars (with the extension) + def remote_filename + @remote_filename ||= + headers['content-disposition'].to_s + .match(REMOTE_FILENAME_PATTERN) # matches the filename pattern + .then { |match| match&.named_captures || {} } # ensures the match is a hash + .fetch('filename') # fetches the 'filename' key or raise KeyError + .then(&File.method(:basename)) # Ensures to remove path from the filename (../ for instance) + .then(&method(:ensure_filename_size)) # Ensures the filename is within the FILENAME_SIZE_LIMIT + rescue KeyError + raise ServiceError, 'Remote filename not provided in content-disposition header' + end + + def ensure_filename_size(filename) + if filename.length <= FILENAME_SIZE_LIMIT + filename + else + extname = File.extname(filename) + basename = File.basename(filename, extname)[0, FILENAME_SIZE_LIMIT] + + "#{basename}#{extname}" + end end end end diff --git a/app/services/bulk_update_integration_service.rb b/app/services/bulk_update_integration_service.rb index 29cfd824c12..fc1580ab880 100644 --- a/app/services/bulk_update_integration_service.rb +++ b/app/services/bulk_update_integration_service.rb @@ -9,7 +9,7 @@ class BulkUpdateIntegrationService # rubocop: disable CodeReuse/ActiveRecord def execute Integration.transaction do - Integration.where(id: batch.select(:id)).update_all(service_hash) + Integration.where(id: batch.select(:id)).update_all(integration_hash) if integration.data_fields_present? integration.data_fields.class.where(service_id: batch.select(:id)).update_all(data_fields_hash) @@ -22,8 +22,8 @@ class BulkUpdateIntegrationService attr_reader :integration, :batch - def service_hash - integration.to_service_hash.tap { |json| json['inherit_from_id'] = integration.inherit_from_id || integration.id } + def integration_hash + integration.to_integration_hash.tap { |json| json['inherit_from_id'] = integration.inherit_from_id || integration.id } end def data_fields_hash diff --git a/app/services/captcha/captcha_verification_service.rb b/app/services/captcha/captcha_verification_service.rb index 45a5a52367c..3ed8ea12f3a 100644 --- a/app/services/captcha/captcha_verification_service.rb +++ b/app/services/captcha/captcha_verification_service.rb @@ -7,20 +7,27 @@ module Captcha class CaptchaVerificationService include Recaptcha::Verify + # Currently the only value that is used out of the request by the reCAPTCHA library + # is 'remote_ip'. Therefore, we just create a struct to avoid passing the full request + # object through all the service layer objects, and instead just rely on passing only + # the required remote_ip value. This eliminates the need to couple the service layer + # to the HTTP request (for the purpose of this service, at least). + RequestStruct = Struct.new(:remote_ip) + + def initialize(spam_params:) + @spam_params = spam_params + end + ## # Performs verification of a captcha response. # - # 'captcha_response' parameter is the response from the user solving a client-side captcha. - # - # 'request' parameter is the request which submitted the captcha. - # # NOTE: Currently only supports reCAPTCHA, and is not yet used in all places of the app in which # captchas are verified, but these can be addressed in future MRs. See: # https://gitlab.com/gitlab-org/gitlab/-/issues/273480 - def execute(captcha_response: nil, request:) - return false unless captcha_response + def execute + return false unless spam_params.captcha_response - @request = request + @request = RequestStruct.new(spam_params.ip_address) Gitlab::Recaptcha.load_configurations! @@ -31,11 +38,13 @@ module Captcha # 2. We want control over the wording and i18n of the message # 3. We want a consistent interface and behavior when adding support for other captcha # libraries which may not support automatically adding errors to the model. - verify_recaptcha(response: captcha_response) + verify_recaptcha(response: spam_params.captcha_response) end private + attr_reader :spam_params + # The recaptcha library's Recaptcha::Verify#verify_recaptcha method requires that # 'request' be a readable attribute - it doesn't support passing it as an options argument. attr_reader :request diff --git a/app/services/ci/after_requeue_job_service.rb b/app/services/ci/after_requeue_job_service.rb index 2b611c857c7..b422e57baad 100644 --- a/app/services/ci/after_requeue_job_service.rb +++ b/app/services/ci/after_requeue_job_service.rb @@ -10,8 +10,16 @@ module Ci private def process_subsequent_jobs(processable) - processable.pipeline.processables.skipped.after_stage(processable.stage_idx).find_each do |processable| - process(processable) + if Feature.enabled?(:ci_same_stage_job_needs, processable.project, default_enabled: :yaml) + (stage_dependent_jobs(processable) | needs_dependent_jobs(processable)) + .each do |processable| + process(processable) + end + else + skipped_jobs(processable).after_stage(processable.stage_idx) + .find_each do |job| + process(job) + end end end @@ -24,5 +32,17 @@ module Ci processable.process(current_user) end end + + def skipped_jobs(processable) + processable.pipeline.processables.skipped + end + + def stage_dependent_jobs(processable) + skipped_jobs(processable).scheduling_type_stage.after_stage(processable.stage_idx) + end + + def needs_dependent_jobs(processable) + skipped_jobs(processable).scheduling_type_dag.with_needs([processable.name]) + end end end diff --git a/app/services/ci/append_build_trace_service.rb b/app/services/ci/append_build_trace_service.rb index 602f8c5030d..8200f9790ee 100644 --- a/app/services/ci/append_build_trace_service.rb +++ b/app/services/ci/append_build_trace_service.rb @@ -24,6 +24,12 @@ module Ci body_start = content_range[0].to_i body_end = body_start + body_data.bytesize + if trace_size_exceeded?(body_end) + build.drop(:trace_size_exceeded) + + return Result.new(status: 403) + end + stream_size = build.trace.append(body_data, body_start) unless stream_size == body_end @@ -37,6 +43,8 @@ module Ci private + delegate :project, to: :build + def stream_range params.fetch(:content_range) end @@ -61,5 +69,10 @@ module Ci ::Gitlab::ErrorTracking .log_exception(TraceRangeError.new, extra) end + + def trace_size_exceeded?(size) + Feature.enabled?(:ci_jobs_trace_size_limit, project, default_enabled: :yaml) && + project.actual_limits.exceeded?(:ci_jobs_trace_size_limit, size / 1.megabyte) + end end end diff --git a/app/services/ci/create_downstream_pipeline_service.rb b/app/services/ci/create_downstream_pipeline_service.rb index 1eff76c2e5d..e9ec2338171 100644 --- a/app/services/ci/create_downstream_pipeline_service.rb +++ b/app/services/ci/create_downstream_pipeline_service.rb @@ -120,7 +120,7 @@ module Ci return false if @bridge.triggers_child_pipeline? if Feature.enabled?(:ci_drop_cyclical_triggered_pipelines, @bridge.project, default_enabled: :yaml) - pipeline_checksums = @bridge.pipeline.base_and_ancestors.filter_map do |pipeline| + pipeline_checksums = @bridge.pipeline.self_and_upstreams.filter_map do |pipeline| config_checksum(pipeline) unless pipeline.child? end @@ -131,7 +131,7 @@ module Ci def has_max_descendants_depth? return false unless @bridge.triggers_child_pipeline? - ancestors_of_new_child = @bridge.pipeline.base_and_ancestors(same_project: true) + ancestors_of_new_child = @bridge.pipeline.self_and_ancestors ancestors_of_new_child.count > MAX_DESCENDANTS_DEPTH end diff --git a/app/services/ci/destroy_pipeline_service.rb b/app/services/ci/destroy_pipeline_service.rb index 1d9533ed76f..494fcb23a06 100644 --- a/app/services/ci/destroy_pipeline_service.rb +++ b/app/services/ci/destroy_pipeline_service.rb @@ -7,7 +7,9 @@ module Ci Ci::ExpirePipelineCacheService.new.execute(pipeline, delete: true) - pipeline.destroy! + pipeline.cancel_running if pipeline.cancelable? && ::Feature.enabled?(:cancel_pipelines_prior_to_destroy, default_enabled: :yaml) + + pipeline.reset.destroy! ServiceResponse.success(message: 'Pipeline not found') rescue ActiveRecord::RecordNotFound diff --git a/app/services/ci/expire_pipeline_cache_service.rb b/app/services/ci/expire_pipeline_cache_service.rb index 80c83818d0b..48a6344f576 100644 --- a/app/services/ci/expire_pipeline_cache_service.rb +++ b/app/services/ci/expire_pipeline_cache_service.rb @@ -77,7 +77,7 @@ module Ci store.touch(path) end - pipeline.self_with_ancestors_and_descendants.each do |relative_pipeline| + pipeline.self_with_upstreams_and_downstreams.each do |relative_pipeline| store.touch(project_pipeline_path(relative_pipeline.project, relative_pipeline)) store.touch(graphql_pipeline_path(relative_pipeline)) store.touch(graphql_pipeline_sha_path(relative_pipeline.sha)) diff --git a/app/services/ci/job_token_scope/add_project_service.rb b/app/services/ci/job_token_scope/add_project_service.rb new file mode 100644 index 00000000000..d03ae434b69 --- /dev/null +++ b/app/services/ci/job_token_scope/add_project_service.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Ci + module JobTokenScope + class AddProjectService < ::BaseService + include EditScopeValidations + + def execute(target_project) + validate_edit!(project, target_project, current_user) + + link = add_project!(target_project) + ServiceResponse.success(payload: { project_link: link }) + + rescue ActiveRecord::RecordNotUnique + ServiceResponse.error(message: "Target project is already in the job token scope") + rescue ActiveRecord::RecordInvalid => e + ServiceResponse.error(message: e.message) + rescue EditScopeValidations::ValidationError => e + ServiceResponse.error(message: e.message) + end + + def add_project!(target_project) + ::Ci::JobToken::ProjectScopeLink.create!( + source_project: project, + target_project: target_project, + added_by: current_user + ) + end + end + end +end diff --git a/app/services/ci/job_token_scope/remove_project_service.rb b/app/services/ci/job_token_scope/remove_project_service.rb new file mode 100644 index 00000000000..15644e529d9 --- /dev/null +++ b/app/services/ci/job_token_scope/remove_project_service.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Ci + module JobTokenScope + class RemoveProjectService < ::BaseService + include EditScopeValidations + + def execute(target_project) + validate_edit!(project, target_project, current_user) + + if project == target_project + return ServiceResponse.error(message: "Source project cannot be removed from the job token scope") + end + + link = ::Ci::JobToken::ProjectScopeLink.for_source_and_target(project, target_project) + + unless link + return ServiceResponse.error(message: "Target project is not in the job token scope") + end + + if link.destroy + ServiceResponse.success + else + ServiceResponse.error(message: link.errors.full_messages.to_sentence, payload: { project_link: link }) + end + rescue EditScopeValidations::ValidationError => e + ServiceResponse.error(message: e.message) + end + end + end +end diff --git a/app/services/ci/pipeline_schedules/calculate_next_run_service.rb b/app/services/ci/pipeline_schedules/calculate_next_run_service.rb index 9978b2d4775..9c8f6b47288 100644 --- a/app/services/ci/pipeline_schedules/calculate_next_run_service.rb +++ b/app/services/ci/pipeline_schedules/calculate_next_run_service.rb @@ -12,15 +12,16 @@ module Ci return fallback_method.call unless plan_cron&.cron_valid? now = Time.zone.now + plan_min_run = plan_cron.next_time_from(now) schedule_next_run = schedule_cron.next_time_from(now) - return schedule_next_run if worker_cron.match?(schedule_next_run) && plan_cron.match?(schedule_next_run) + return schedule_next_run if worker_cron.match?(schedule_next_run) && plan_min_run <= schedule_next_run - plan_next_run = plan_cron.next_time_from(now) + plan_next_run = plan_cron.next_time_from(schedule_next_run) return plan_next_run if worker_cron.match?(plan_next_run) - worker_next_run = worker_cron.next_time_from(now) - return worker_next_run if plan_cron.match?(worker_next_run) + worker_next_run = worker_cron.next_time_from(schedule_next_run) + return worker_next_run if plan_min_run <= worker_next_run worker_cron.next_time_from(plan_next_run) end diff --git a/app/services/ci/pipelines/add_job_service.rb b/app/services/ci/pipelines/add_job_service.rb new file mode 100644 index 00000000000..03bdb491200 --- /dev/null +++ b/app/services/ci/pipelines/add_job_service.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Ci + module Pipelines + class AddJobService + attr_reader :pipeline + + def initialize(pipeline) + @pipeline = pipeline + + raise ArgumentError, "Pipeline must be persisted for this service to be used" unless @pipeline.persisted? + end + + def execute!(job, &block) + assign_pipeline_attributes(job) + + Ci::Pipeline.transaction do + yield(job) + + job.update_older_statuses_retried! if Feature.enabled?(:ci_fix_commit_status_retried, @pipeline.project, default_enabled: :yaml) + end + + ServiceResponse.success(payload: { job: job }) + rescue StandardError => e + ServiceResponse.error(message: e.message, payload: { job: job }) + end + + private + + def assign_pipeline_attributes(job) + job.pipeline = @pipeline + job.project = @pipeline.project + job.ref = @pipeline.ref + end + end + end +end diff --git a/app/services/ci/play_bridge_service.rb b/app/services/ci/play_bridge_service.rb index 2f0bcece9e3..a719467253e 100644 --- a/app/services/ci/play_bridge_service.rb +++ b/app/services/ci/play_bridge_service.rb @@ -9,8 +9,6 @@ module Ci bridge.user = current_user bridge.enqueue! - next unless ::Feature.enabled?(:ci_fix_pipeline_status_for_dag_needs_manual, project, default_enabled: :yaml) - AfterRequeueJobService.new(project, current_user).execute(bridge) end end diff --git a/app/services/ci/play_build_service.rb b/app/services/ci/play_build_service.rb index 073c1a2d0e0..c1cf06a4631 100644 --- a/app/services/ci/play_build_service.rb +++ b/app/services/ci/play_build_service.rb @@ -11,8 +11,6 @@ module Ci build.tap do |build| build.update(user: current_user, job_variables_attributes: job_variables_attributes || []) - next unless ::Feature.enabled?(:ci_fix_pipeline_status_for_dag_needs_manual, project, default_enabled: :yaml) - AfterRequeueJobService.new(project, current_user).execute(build) end else diff --git a/app/services/ci/queue/build_queue_service.rb b/app/services/ci/queue/build_queue_service.rb new file mode 100644 index 00000000000..99408d529b2 --- /dev/null +++ b/app/services/ci/queue/build_queue_service.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +module Ci + module Queue + class BuildQueueService + include ::Gitlab::Utils::StrongMemoize + + attr_reader :runner + + def initialize(runner) + @runner = runner + end + + def new_builds + strategy.new_builds + end + + ## + # This is overridden in EE + # + def builds_for_shared_runner + strategy.builds_for_shared_runner + end + + # rubocop:disable CodeReuse/ActiveRecord + def builds_for_group_runner + # Workaround for weird Rails bug, that makes `runner.groups.to_sql` to return `runner_id = NULL` + groups = ::Group.joins(:runner_namespaces).merge(runner.runner_namespaces) + + hierarchy_groups = Gitlab::ObjectHierarchy + .new(groups) + .base_and_descendants + + projects = Project.where(namespace_id: hierarchy_groups) + .with_group_runners_enabled + .with_builds_enabled + .without_deleted + + relation = new_builds.where(project: projects) + + order(relation) + end + + def builds_for_project_runner + relation = new_builds + .where(project: runner.projects.without_deleted.with_builds_enabled) + + order(relation) + end + + def builds_queued_before(relation, time) + relation.queued_before(time) + end + + def builds_for_protected_runner(relation) + relation.ref_protected + end + + def builds_matching_tag_ids(relation, ids) + strategy.builds_matching_tag_ids(relation, ids) + end + + def builds_with_any_tags(relation) + strategy.builds_with_any_tags(relation) + end + + def order(relation) + strategy.order(relation) + end + + def execute(relation) + strategy.build_ids(relation) + end + + private + + def strategy + strong_memoize(:strategy) do + if ::Feature.enabled?(:ci_pending_builds_queue_source, runner, default_enabled: :yaml) + Queue::PendingBuildsStrategy.new(runner) + else + Queue::BuildsTableStrategy.new(runner) + end + end + end + end + end +end + +Ci::Queue::BuildQueueService.prepend_mod_with('Ci::Queue::BuildQueueService') diff --git a/app/services/ci/queue/builds_table_strategy.rb b/app/services/ci/queue/builds_table_strategy.rb new file mode 100644 index 00000000000..c941734ac40 --- /dev/null +++ b/app/services/ci/queue/builds_table_strategy.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +module Ci + module Queue + class BuildsTableStrategy + attr_reader :runner + + def initialize(runner) + @runner = runner + end + + # rubocop:disable CodeReuse/ActiveRecord + def builds_for_shared_runner + relation = new_builds + # don't run projects which have not enabled shared runners and builds + .joins('INNER JOIN projects ON ci_builds.project_id = projects.id') + .where(projects: { shared_runners_enabled: true, pending_delete: false }) + .joins('LEFT JOIN project_features ON ci_builds.project_id = project_features.project_id') + .where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0') + + if Feature.enabled?(:ci_queueing_disaster_recovery_disable_fair_scheduling, runner, type: :ops, default_enabled: :yaml) + # if disaster recovery is enabled, we fallback to FIFO scheduling + relation.order('ci_builds.id ASC') + else + # Implement fair scheduling + # this returns builds that are ordered by number of running builds + # we prefer projects that don't use shared runners at all + relation + .joins("LEFT JOIN (#{running_builds_for_shared_runners.to_sql}) AS project_builds ON ci_builds.project_id = project_builds.project_id") + .order(Arel.sql('COALESCE(project_builds.running_builds, 0) ASC'), 'ci_builds.id ASC') + end + end + + def builds_matching_tag_ids(relation, ids) + # pick builds that does not have other tags than runner's one + relation.matches_tag_ids(ids) + end + + def builds_with_any_tags(relation) + # pick builds that have at least one tag + relation.with_any_tags + end + + def order(relation) + relation.order('id ASC') + end + + def new_builds + ::Ci::Build.pending.unstarted + end + + def build_ids(relation) + relation.pluck(:id) + end + + private + + def running_builds_for_shared_runners + ::Ci::Build.running + .where(runner: ::Ci::Runner.instance_type) + .group(:project_id) + .select(:project_id, 'COUNT(*) AS running_builds') + end + # rubocop:enable CodeReuse/ActiveRecord + end + end +end diff --git a/app/services/ci/queue/pending_builds_strategy.rb b/app/services/ci/queue/pending_builds_strategy.rb new file mode 100644 index 00000000000..55d5cb96a0a --- /dev/null +++ b/app/services/ci/queue/pending_builds_strategy.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +module Ci + module Queue + class PendingBuildsStrategy + attr_reader :runner + + def initialize(runner) + @runner = runner + end + + # rubocop:disable CodeReuse/ActiveRecord + def builds_for_shared_runner + relation = new_builds + # don't run projects which have not enabled shared runners and builds + .joins('INNER JOIN projects ON ci_pending_builds.project_id = projects.id') + .where(projects: { shared_runners_enabled: true, pending_delete: false }) + .joins('LEFT JOIN project_features ON ci_pending_builds.project_id = project_features.project_id') + .where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0') + + if Feature.enabled?(:ci_queueing_disaster_recovery_disable_fair_scheduling, runner, type: :ops, default_enabled: :yaml) + # if disaster recovery is enabled, we fallback to FIFO scheduling + relation.order('ci_pending_builds.build_id ASC') + else + # Implement fair scheduling + # this returns builds that are ordered by number of running builds + # we prefer projects that don't use shared runners at all + relation + .with(running_builds_for_shared_runners_cte.to_arel) + .joins("LEFT JOIN project_builds ON ci_pending_builds.project_id = project_builds.project_id") + .order(Arel.sql('COALESCE(project_builds.running_builds, 0) ASC'), 'ci_pending_builds.build_id ASC') + end + end + + def builds_matching_tag_ids(relation, ids) + relation.merge(CommitStatus.matches_tag_ids(ids, table: 'ci_pending_builds', column: 'build_id')) + end + + def builds_with_any_tags(relation) + relation.merge(CommitStatus.with_any_tags(table: 'ci_pending_builds', column: 'build_id')) + end + + def order(relation) + relation.order('build_id ASC') + end + + def new_builds + ::Ci::PendingBuild.all + end + + def build_ids(relation) + relation.pluck(:build_id) + end + + private + + def running_builds_for_shared_runners_cte + running_builds = ::Ci::RunningBuild + .instance_type + .group(:project_id) + .select(:project_id, 'COUNT(*) AS running_builds') + + ::Gitlab::SQL::CTE + .new(:project_builds, running_builds, materialized: true) + end + # rubocop:enable CodeReuse/ActiveRecord + end + end +end diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 6280bf4c986..dc046e1d164 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -103,35 +103,40 @@ module Ci # rubocop: disable CodeReuse/ActiveRecord def each_build(params, &blk) - builds = + queue = ::Ci::Queue::BuildQueueService.new(runner) + + builds = begin if runner.instance_type? - builds_for_shared_runner + queue.builds_for_shared_runner elsif runner.group_type? - builds_for_group_runner + queue.builds_for_group_runner else - builds_for_project_runner + queue.builds_for_project_runner end + end + + if runner.ref_protected? + builds = queue.builds_for_protected_runner(builds) + end # pick builds that does not have other tags than runner's one - builds = builds.matches_tag_ids(runner.tags.ids) + builds = queue.builds_matching_tag_ids(builds, runner.tags.ids) # pick builds that have at least one tag unless runner.run_untagged? - builds = builds.with_any_tags + builds = queue.builds_with_any_tags(builds) end # pick builds that older than specified age if params.key?(:job_age) - builds = builds.queued_before(params[:job_age].seconds.ago) + builds = queue.builds_queued_before(builds, params[:job_age].seconds.ago) end - build_ids = retrieve_queue(-> { builds.pluck(:id) }) + build_ids = retrieve_queue(-> { queue.execute(builds) }) @metrics.observe_queue_size(-> { build_ids.size }, @runner.runner_type) - build_ids.each do |build_id| - yield Ci::Build.find(build_id) - end + build_ids.each { |build_id| yield Ci::Build.find(build_id) } end # rubocop: enable CodeReuse/ActiveRecord @@ -204,7 +209,7 @@ module Ci # We need to use the presenter here because Gitaly calls in the presenter # may fail, and we need to ensure the response has been generated. presented_build = ::Ci::BuildRunnerPresenter.new(build) # rubocop:disable CodeReuse/Presenter - build_json = ::API::Entities::JobRequest::Response.new(presented_build).to_json + build_json = ::API::Entities::Ci::JobRequest::Response.new(presented_build).to_json Result.new(build, build_json, true) end @@ -259,63 +264,6 @@ module Ci ) end - # rubocop: disable CodeReuse/ActiveRecord - def builds_for_shared_runner - relation = new_builds. - # don't run projects which have not enabled shared runners and builds - joins(:project).where(projects: { shared_runners_enabled: true, pending_delete: false }) - .joins('LEFT JOIN project_features ON ci_builds.project_id = project_features.project_id') - .where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0') - - if Feature.enabled?(:ci_queueing_disaster_recovery, runner, type: :ops, default_enabled: :yaml) - # if disaster recovery is enabled, we fallback to FIFO scheduling - relation.order('ci_builds.id ASC') - else - # Implement fair scheduling - # this returns builds that are ordered by number of running builds - # we prefer projects that don't use shared runners at all - relation - .joins("LEFT JOIN (#{running_builds_for_shared_runners.to_sql}) AS project_builds ON ci_builds.project_id=project_builds.project_id") - .order(Arel.sql('COALESCE(project_builds.running_builds, 0) ASC'), 'ci_builds.id ASC') - end - end - - def builds_for_project_runner - new_builds.where(project: runner.projects.without_deleted.with_builds_enabled).order('id ASC') - end - - def builds_for_group_runner - # Workaround for weird Rails bug, that makes `runner.groups.to_sql` to return `runner_id = NULL` - groups = ::Group.joins(:runner_namespaces).merge(runner.runner_namespaces) - - hierarchy_groups = Gitlab::ObjectHierarchy.new(groups, options: { use_distinct: Feature.enabled?(:use_distinct_in_register_job_object_hierarchy) }).base_and_descendants - projects = Project.where(namespace_id: hierarchy_groups) - .with_group_runners_enabled - .with_builds_enabled - .without_deleted - new_builds.where(project: projects).order('id ASC') - end - - def running_builds_for_shared_runners - Ci::Build.running.where(runner: Ci::Runner.instance_type) - .group(:project_id).select(:project_id, 'count(*) AS running_builds') - end - - def all_builds - if Feature.enabled?(:ci_pending_builds_queue_join, runner, default_enabled: :yaml) - Ci::Build.joins(:queuing_entry) - else - Ci::Build.all - end - end - # rubocop: enable CodeReuse/ActiveRecord - - def new_builds - builds = all_builds.pending.unstarted - builds = builds.ref_protected if runner.ref_protected? - builds - end - def pre_assign_runner_checks { missing_dependency_failure: -> (build, _) { !build.has_valid_build_dependencies? }, diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb index ea76771b80a..08520c9514c 100644 --- a/app/services/ci/retry_build_service.rb +++ b/app/services/ci/retry_build_service.rb @@ -10,10 +10,17 @@ module Ci resource_group scheduling_type].freeze end + def self.extra_accessors + [] + end + def execute(build) build.ensure_scheduling_type! reprocess!(build).tap do |new_build| + check_assignable_runners!(new_build) + next if new_build.failed? + Gitlab::OptimisticLocking.retry_lock(new_build, name: 'retry_build', &:enqueue) AfterRequeueJobService.new(project, current_user).execute(build) @@ -27,18 +34,15 @@ module Ci def reprocess!(build) check_access!(build) - attributes = self.class.clone_accessors.to_h do |attribute| - [attribute, build.public_send(attribute)] # rubocop:disable GitlabSecurity/PublicSend - end - - attributes[:user] = current_user - - Ci::Build.transaction do - create_build!(attributes).tap do |new_build| - new_build.update_older_statuses_retried! - build.reset # refresh the data to get new values of `retried` and `processed`. + new_build = clone_build(build) + ::Ci::Pipelines::AddJobService.new(build.pipeline).execute!(new_build) do |job| + BulkInsertableAssociations.with_bulk_insert do + job.save! end end + build.reset # refresh the data to get new values of `retried` and `processed`. + + new_build end # rubocop: enable CodeReuse/ActiveRecord @@ -50,13 +54,21 @@ module Ci end end - def create_build!(attributes) - build = project.builds.new(attributes) - build.assign_attributes(::Gitlab::Ci::Pipeline::Seed::Build.environment_attributes_for(build)) - BulkInsertableAssociations.with_bulk_insert do - build.save! + def check_assignable_runners!(build); end + + def clone_build(build) + project.builds.new(build_attributes(build)).tap do |new_build| + new_build.assign_attributes(::Gitlab::Ci::Pipeline::Seed::Build.environment_attributes_for(new_build)) end - build + end + + def build_attributes(build) + attributes = self.class.clone_accessors.to_h do |attribute| + [attribute, build.public_send(attribute)] # rubocop:disable GitlabSecurity/PublicSend + end + + attributes[:user] = current_user + attributes end end end diff --git a/app/services/ci/retry_pipeline_service.rb b/app/services/ci/retry_pipeline_service.rb index 5cc6b89bfef..02ee40d2cf6 100644 --- a/app/services/ci/retry_pipeline_service.rb +++ b/app/services/ci/retry_pipeline_service.rb @@ -13,8 +13,8 @@ module Ci pipeline.ensure_scheduling_type! - pipeline.retryable_builds.preload_needs.find_each do |build| - next unless can?(current_user, :update_build, build) + builds_relation(pipeline).find_each do |build| + next unless can_be_retried?(build) Ci::RetryBuildService.new(project, current_user) .reprocess!(build) @@ -36,5 +36,17 @@ module Ci .new(pipeline) .execute end + + private + + def builds_relation(pipeline) + pipeline.retryable_builds.preload_needs + end + + def can_be_retried?(build) + can?(current_user, :update_build, build) + end end end + +Ci::RetryPipelineService.prepend_mod_with('Ci::RetryPipelineService') diff --git a/app/services/concerns/alert_management/alert_processing.rb b/app/services/concerns/alert_management/alert_processing.rb index 98d255dec27..2b556a4339d 100644 --- a/app/services/concerns/alert_management/alert_processing.rb +++ b/app/services/concerns/alert_management/alert_processing.rb @@ -64,7 +64,7 @@ module AlertManagement def process_new_alert if alert.save - alert.execute_services + alert.execute_integrations SystemNoteService.create_new_alert(alert, alert_source) process_resolved_alert if resolving_alert? diff --git a/app/services/concerns/ci/job_token_scope/edit_scope_validations.rb b/app/services/concerns/ci/job_token_scope/edit_scope_validations.rb new file mode 100644 index 00000000000..23053975313 --- /dev/null +++ b/app/services/concerns/ci/job_token_scope/edit_scope_validations.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Ci + module JobTokenScope + module EditScopeValidations + ValidationError = Class.new(StandardError) + + TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND = "The target_project that you are attempting to access does " \ + "not exist or you don't have permission to perform this action" + + def validate_edit!(source_project, target_project, current_user) + unless source_project.ci_job_token_scope_enabled? + raise ValidationError, "Job token scope is disabled for this project" + end + + unless can?(current_user, :admin_project, source_project) + raise ValidationError, "Insufficient permissions to modify the job token scope" + end + + unless can?(current_user, :read_project, target_project) + raise ValidationError, TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND + end + end + end + end +end diff --git a/app/services/concerns/update_repository_storage_methods.rb b/app/services/concerns/update_repository_storage_methods.rb index 6e4824bd784..cbcd0b7f56b 100644 --- a/app/services/concerns/update_repository_storage_methods.rb +++ b/app/services/concerns/update_repository_storage_methods.rb @@ -38,11 +38,7 @@ module UpdateRepositoryStorageMethods rescue StandardError => e repository_storage_move.do_fail! - Gitlab::ErrorTracking.track_exception(e, container_klass: container.class.to_s, container_path: container.full_path) - - ServiceResponse.error( - message: s_("UpdateRepositoryStorage|Error moving repository storage for %{container_full_path} - %{message}") % { container_full_path: container.full_path, message: e.message } - ) + Gitlab::ErrorTracking.track_and_raise_exception(e, container_klass: container.class.to_s, container_path: container.full_path) end private diff --git a/app/services/design_management/save_designs_service.rb b/app/services/design_management/save_designs_service.rb index 44ebd45f76e..a1fce45434b 100644 --- a/app/services/design_management/save_designs_service.rb +++ b/app/services/design_management/save_designs_service.rb @@ -119,7 +119,7 @@ module DesignManagement # Returns the latest blobs for the designs as a Hash of `{ Design => Blob }` def existing_blobs @existing_blobs ||= begin - items = designs.map { |d| ['HEAD', d.full_path] } + items = designs.map { |d| [target_branch, d.full_path] } repository.blobs_at(items).each_with_object({}) do |blob, h| design = designs.find { |d| d.full_path == blob.path } diff --git a/app/services/error_tracking/collect_error_service.rb b/app/services/error_tracking/collect_error_service.rb new file mode 100644 index 00000000000..bc1f238d81f --- /dev/null +++ b/app/services/error_tracking/collect_error_service.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module ErrorTracking + class CollectErrorService < ::BaseService + def execute + # Error is a way to group events based on common data like name or cause + # of exception. We need to keep a sane balance here between taking too little + # and too much data into group logic. + error = project.error_tracking_errors.report_error( + name: exception['type'], # Example: ActionView::MissingTemplate + description: exception['value'], # Example: Missing template posts/show in... + actor: event['transaction'], # Example: PostsController#show + platform: event['platform'], # Example: ruby + timestamp: event['timestamp'] + ) + + # The payload field contains all the data on error including stacktrace in jsonb. + # Together with occured_at these are 2 main attributes that we need to save here. + error.events.create!( + environment: event['environment'], + description: exception['type'], + level: event['level'], + occurred_at: event['timestamp'], + payload: event + ) + end + + private + + def event + params[:event] + end + + def exception + event['exception']['values'].first + end + end +end diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb index 1ca1bfa0c05..1eb54e13522 100644 --- a/app/services/git/base_hooks_service.rb +++ b/app/services/git/base_hooks_service.rb @@ -69,7 +69,7 @@ module Git # Creating push_data invokes one CommitDelta RPC per commit. Only # build this data if we actually need it. project.execute_hooks(push_data, hook_name) if project.has_active_hooks?(hook_name) - project.execute_services(push_data, hook_name) if project.has_active_services?(hook_name) + project.execute_integrations(push_data, hook_name) if project.has_active_integrations?(hook_name) end def enqueue_invalidate_cache diff --git a/app/services/git/wiki_push_service/change.rb b/app/services/git/wiki_push_service/change.rb index 9109a7f9d58..8fc821b59fc 100644 --- a/app/services/git/wiki_push_service/change.rb +++ b/app/services/git/wiki_push_service/change.rb @@ -66,7 +66,8 @@ module Git def strip_extension(filename) return unless filename - File.basename(filename, File.extname(filename)) + encoded_filename = Gitlab::EncodingHelper.encode_utf8(filename.dup) + File.basename(encoded_filename, File.extname(encoded_filename)) end end end diff --git a/app/services/gpg_keys/create_service.rb b/app/services/gpg_keys/create_service.rb index e41444b2a82..ab8b12732d7 100644 --- a/app/services/gpg_keys/create_service.rb +++ b/app/services/gpg_keys/create_service.rb @@ -3,9 +3,17 @@ module GpgKeys class CreateService < Keys::BaseService def execute - key = user.gpg_keys.create(params) + key = create(params) notification_service.new_gpg_key(key) if key.persisted? key end + + private + + def create(params) + user.gpg_keys.create(params) + end end end + +GpgKeys::CreateService.prepend_mod diff --git a/app/services/gpg_keys/destroy_service.rb b/app/services/gpg_keys/destroy_service.rb index cecbfe26611..2e82509897e 100644 --- a/app/services/gpg_keys/destroy_service.rb +++ b/app/services/gpg_keys/destroy_service.rb @@ -7,3 +7,5 @@ module GpgKeys end end end + +GpgKeys::DestroyService.prepend_mod diff --git a/app/services/groups/group_links/create_service.rb b/app/services/groups/group_links/create_service.rb index 5f81e5972b0..8c3ba0a63f2 100644 --- a/app/services/groups/group_links/create_service.rb +++ b/app/services/groups/group_links/create_service.rb @@ -24,7 +24,7 @@ module Groups ) if link.save - shared_with_group.refresh_members_authorized_projects(direct_members_only: true) + shared_with_group.refresh_members_authorized_projects(blocking: false, direct_members_only: true) success(link: link) else error(link.errors.full_messages.to_sentence, 409) diff --git a/app/services/groups/group_links/destroy_service.rb b/app/services/groups/group_links/destroy_service.rb index 05504a80f46..0e7fd7e0817 100644 --- a/app/services/groups/group_links/destroy_service.rb +++ b/app/services/groups/group_links/destroy_service.rb @@ -16,7 +16,7 @@ module Groups groups_to_refresh = links.map(&:shared_with_group) groups_to_refresh.uniq.each do |group| - group.refresh_members_authorized_projects(direct_members_only: true) + group.refresh_members_authorized_projects(blocking: false, direct_members_only: true) end else Gitlab::AppLogger.info( diff --git a/app/services/groups/group_links/update_service.rb b/app/services/groups/group_links/update_service.rb index 3703d535482..a1411de36d6 100644 --- a/app/services/groups/group_links/update_service.rb +++ b/app/services/groups/group_links/update_service.rb @@ -13,7 +13,7 @@ module Groups group_link.update!(group_link_params) if requires_authorization_refresh?(group_link_params) - group_link.shared_with_group.refresh_members_authorized_projects(direct_members_only: true) + group_link.shared_with_group.refresh_members_authorized_projects(blocking: false, direct_members_only: true) end end diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb index 518d061c654..966d04ceb70 100644 --- a/app/services/groups/transfer_service.rb +++ b/app/services/groups/transfer_service.rb @@ -46,6 +46,7 @@ module Groups def ensure_allowed_transfer raise_transfer_error(:group_is_already_root) if group_is_already_root? raise_transfer_error(:same_parent_as_current) if same_parent? + raise_transfer_error(:has_subscription) if has_subscription? raise_transfer_error(:invalid_policies) unless valid_policies? raise_transfer_error(:namespace_with_same_path) if namespace_with_same_path? raise_transfer_error(:group_contains_images) if group_projects_contain_registry_images? @@ -73,6 +74,10 @@ module Groups @new_parent_group && @new_parent_group.id == @group.parent_id end + def has_subscription? + @group.paid? + end + def transfer_to_subgroup? @new_parent_group && \ @group.self_and_descendants.pluck_primary_key.include?(@new_parent_group.id) diff --git a/app/services/incident_management/incidents/create_service.rb b/app/services/incident_management/incidents/create_service.rb index 7497ee00d74..f8437290d9b 100644 --- a/app/services/incident_management/incidents/create_service.rb +++ b/app/services/incident_management/incidents/create_service.rb @@ -20,14 +20,14 @@ module IncidentManagement params: { title: title, description: description, - issue_type: ISSUE_TYPE - } + issue_type: ISSUE_TYPE, + severity: severity + }, + spam_params: nil ).execute return error(issue.errors.full_messages.to_sentence, issue) unless issue.valid? - update_severity_for(issue) - success(issue) end @@ -42,10 +42,6 @@ module IncidentManagement def error(message, issue = nil) ServiceResponse.error(payload: { issue: issue }, message: message) end - - def update_severity_for(issue) - ::IncidentManagement::Incidents::UpdateSeverityService.new(issue, current_user, severity).execute - end end end end diff --git a/app/services/incident_management/incidents/update_severity_service.rb b/app/services/incident_management/incidents/update_severity_service.rb deleted file mode 100644 index faa9277c469..00000000000 --- a/app/services/incident_management/incidents/update_severity_service.rb +++ /dev/null @@ -1,38 +0,0 @@ -# frozen_string_literal: true - -module IncidentManagement - module Incidents - class UpdateSeverityService < BaseService - def initialize(issuable, current_user, severity) - super(issuable.project, current_user) - - @issuable = issuable - @severity = severity.to_s.downcase - @severity = IssuableSeverity::DEFAULT unless IssuableSeverity.severities.key?(@severity) - end - - def execute - return unless issuable.supports_severity? - - update_severity! - add_system_note - end - - private - - attr_reader :issuable, :severity - - def issuable_severity - issuable.issuable_severity || issuable.build_issuable_severity(issue_id: issuable.id) - end - - def update_severity! - issuable_severity.update!(severity: severity) - end - - def add_system_note - ::IncidentManagement::AddSeveritySystemNoteWorker.perform_async(issuable.id, current_user.id) - end - end - end -end diff --git a/app/services/issuable/clone/base_service.rb b/app/services/issuable/clone/base_service.rb index f8a9eb3ece5..574fe85b466 100644 --- a/app/services/issuable/clone/base_service.rb +++ b/app/services/issuable/clone/base_service.rb @@ -66,7 +66,7 @@ module Issuable def close_issue close_service = Issues::CloseService.new(project: old_project, current_user: current_user) - close_service.execute(original_entity, notifications: false, system_note: false) + close_service.execute(original_entity, notifications: false, system_note: true) end def new_parent diff --git a/app/services/issuable/import_csv/base_service.rb b/app/services/issuable/import_csv/base_service.rb index 27dbc8b3cc4..4a6b7540ded 100644 --- a/app/services/issuable/import_csv/base_service.rb +++ b/app/services/issuable/import_csv/base_service.rb @@ -68,7 +68,10 @@ module Issuable end def create_issuable(attributes) - create_issuable_class.new(project: @project, current_user: @user, params: attributes).execute + # NOTE: CSV imports are performed by workers, so we do not have a request context in order + # to create a SpamParams object to pass to the issuable create service. + spam_params = nil + create_issuable_class.new(project: @project, current_user: @user, params: attributes, spam_params: spam_params).execute end def email_results_to_user diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 02c1f078a40..8d65865e7da 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -57,6 +57,7 @@ class IssuableBaseService < ::BaseProjectService filter_assignees(issuable) filter_milestone filter_labels + filter_severity(issuable) end def filter_assignees(issuable) @@ -135,6 +136,16 @@ class IssuableBaseService < ::BaseProjectService @labels_service ||= ::Labels::AvailableLabelsService.new(current_user, parent, params) end + def filter_severity(issuable) + severity = params.delete(:severity) + return unless severity && issuable.supports_severity? + + severity = IssuableSeverity::DEFAULT unless IssuableSeverity.severities.key?(severity) + return if severity == issuable.severity + + params[:issuable_severity_attributes] = { severity: severity } + end + def process_label_ids(attributes, existing_label_ids: nil, extra_label_ids: []) label_ids = attributes.delete(:label_ids) add_label_ids = attributes.delete(:add_label_ids) @@ -352,7 +363,6 @@ class IssuableBaseService < ::BaseProjectService def change_additional_attributes(issuable) change_state(issuable) - change_severity(issuable) change_subscription(issuable) change_todo(issuable) toggle_award(issuable) @@ -371,12 +381,6 @@ class IssuableBaseService < ::BaseProjectService end end - def change_severity(issuable) - if severity = params.delete(:severity) - ::IncidentManagement::Incidents::UpdateSeverityService.new(issuable, current_user, severity).execute - end - end - def change_subscription(issuable) case params.delete(:subscription_event) when 'subscribe' @@ -443,6 +447,7 @@ class IssuableBaseService < ::BaseProjectService associations[:time_change] = issuable.time_change if issuable.respond_to?(:time_change) associations[:description] = issuable.description associations[:reviewers] = issuable.reviewers.to_a if issuable.allows_reviewers? + associations[:severity] = issuable.severity if issuable.supports_severity? associations end diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index 1c50bb74176..bf66a33a7b2 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -60,7 +60,7 @@ module Issues issue_data = Gitlab::Lazy.new { hook_data(issue, action, old_associations: old_associations) } hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks issue.project.execute_hooks(issue_data, hooks_scope) - issue.project.execute_services(issue_data, hooks_scope) + issue.project.execute_integrations(issue_data, hooks_scope) end def update_project_counter_caches?(issue) diff --git a/app/services/issues/clone_service.rb b/app/services/issues/clone_service.rb index 6df32f1104c..cb42334fe32 100644 --- a/app/services/issues/clone_service.rb +++ b/app/services/issues/clone_service.rb @@ -55,9 +55,13 @@ module Issues new_params = original_entity.serializable_hash.symbolize_keys.merge(new_params) + # spam checking is not necessary, as no new content is being created. Passing nil for + # spam_params will cause SpamActionService to skip checking and return a success response. + spam_params = nil + # Skip creation of system notes for existing attributes of the issue. The system notes of the old # issue are copied over so we don't want to end up with duplicate notes. - CreateService.new(project: target_project, current_user: current_user, params: new_params).execute(skip_system_notes: true) + CreateService.new(project: target_project, current_user: current_user, params: new_params, spam_params: spam_params).execute(skip_system_notes: true) end def queue_copy_designs diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 53f3dc39ba3..30d081996b1 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -4,10 +4,21 @@ module Issues class CreateService < Issues::BaseService include ResolveDiscussions - def execute(skip_system_notes: false) - @request = params.delete(:request) - @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request) + # NOTE: For Issues::CreateService, we require the spam_params and do not default it to nil, because + # spam_checking is likely to be necessary. However, if there is not a request available in scope + # in the caller (for example, an issue created via email) and the required arguments to the + # SpamParams constructor are not otherwise available, spam_params: must be explicitly passed as nil. + def initialize(project:, current_user: nil, params: {}, spam_params:) + # Temporary check to ensure we are no longer passing request in params now that we have + # introduced spam_params. Raise an exception if it is present. + # Remove after https://gitlab.com/gitlab-org/gitlab/-/merge_requests/58603 is complete. + raise if params[:request] + + super(project: project, current_user: current_user, params: params) + @spam_params = spam_params + end + def execute(skip_system_notes: false) @issue = BuildService.new(project: project, current_user: current_user, params: params).execute filter_resolve_discussion_params @@ -18,10 +29,10 @@ module Issues def before_create(issue) Spam::SpamActionService.new( spammable: issue, - request: request, + spam_params: spam_params, user: current_user, action: :create - ).execute(spam_params: spam_params) + ).execute # current_user (defined in BaseService) is not available within run_after_commit block user = current_user @@ -64,10 +75,10 @@ module Issues private - attr_reader :request, :spam_params + attr_reader :spam_params def user_agent_detail_service - UserAgentDetailService.new(@issue, request) + UserAgentDetailService.new(spammable: @issue, spam_params: spam_params) end end end diff --git a/app/services/issues/duplicate_service.rb b/app/services/issues/duplicate_service.rb index d150f0e5917..9547698d916 100644 --- a/app/services/issues/duplicate_service.rb +++ b/app/services/issues/duplicate_service.rb @@ -28,6 +28,7 @@ module Issues def relate_two_issues(duplicate_issue, canonical_issue) params = { target_issuable: canonical_issue } + IssueLinks::CreateService.new(duplicate_issue, current_user, params).execute end end diff --git a/app/services/issues/move_service.rb b/app/services/issues/move_service.rb index e49123a2993..ff78221c941 100644 --- a/app/services/issues/move_service.rb +++ b/app/services/issues/move_service.rb @@ -58,10 +58,13 @@ module Issues } new_params = original_entity.serializable_hash.symbolize_keys.merge(new_params) + # spam checking is not necessary, as no new content is being created. Passing nil for + # spam_params will cause SpamActionService to skip checking and return a success response. + spam_params = nil # Skip creation of system notes for existing attributes of the issue. The system notes of the old # issue are copied over so we don't want to end up with duplicate notes. - CreateService.new(project: @target_project, current_user: @current_user, params: new_params).execute(skip_system_notes: true) + CreateService.new(project: @target_project, current_user: @current_user, params: new_params, spam_params: spam_params).execute(skip_system_notes: true) end def queue_copy_designs diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index cf2892bf413..9ede5ef728b 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -4,12 +4,17 @@ module Issues class UpdateService < Issues::BaseService extend ::Gitlab::Utils::Override + # NOTE: For Issues::UpdateService, we default the spam_params to nil, because spam_checking is not + # necessary in many cases, and we don't want to require every caller to explicitly pass it as nil + # to disable spam checking. + def initialize(project:, current_user: nil, params: {}, spam_params: nil) + super(project: project, current_user: current_user, params: params) + @spam_params = spam_params + end + def execute(issue) handle_move_between_ids(issue) - @request = params.delete(:request) - @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request) - change_issue_duplicate(issue) move_issue_to_new_project(issue) || clone_issue(issue) || update_task_event(issue) || update(issue) end @@ -25,10 +30,10 @@ module Issues Spam::SpamActionService.new( spammable: issue, - request: request, + spam_params: spam_params, user: current_user, action: :update - ).execute(spam_params: spam_params) + ).execute end def handle_changes(issue, options) @@ -37,6 +42,7 @@ module Issues old_labels = old_associations.fetch(:labels, []) old_mentioned_users = old_associations.fetch(:mentioned_users, []) old_assignees = old_associations.fetch(:assignees, []) + old_severity = old_associations[:severity] if has_changes?(issue, old_labels: old_labels, old_assignees: old_assignees) todo_service.resolve_todos_for_target(issue, current_user) @@ -69,6 +75,8 @@ module Issues if added_mentions.present? notification_service.async.new_mentions_in_issue(issue, added_mentions, current_user) end + + handle_severity_change(issue, old_severity) end def handle_assignee_changes(issue, old_assignees) @@ -127,7 +135,7 @@ module Issues private - attr_reader :request, :spam_params + attr_reader :spam_params def clone_issue(issue) target_project = params.delete(:target_clone_project) @@ -176,6 +184,12 @@ module Issues end end + def handle_severity_change(issue, old_severity) + return unless old_severity && issue.severity != old_severity + + ::IncidentManagement::AddSeveritySystemNoteWorker.perform_async(issue.id, current_user.id) + end + # rubocop: disable CodeReuse/ActiveRecord def issuable_for_positioning(id, board_group_id = nil) return unless id diff --git a/app/services/jira/requests/base.rb b/app/services/jira/requests/base.rb index bae8298d5c8..e4e2736ca2f 100644 --- a/app/services/jira/requests/base.rb +++ b/app/services/jira/requests/base.rb @@ -7,20 +7,20 @@ module Jira JIRA_API_VERSION = 2 - def initialize(jira_service, params = {}) - @project = jira_service&.project - @jira_service = jira_service + def initialize(jira_integration, params = {}) + @project = jira_integration&.project + @jira_integration = jira_integration end def execute - return ServiceResponse.error(message: _('Jira service not configured.')) unless jira_service&.active? + return ServiceResponse.error(message: _('Jira service not configured.')) unless jira_integration&.active? request end private - attr_reader :jira_service, :project + attr_reader :jira_integration, :project # We have to add the context_path here because the Jira client is not taking it into account def base_api_url @@ -37,7 +37,7 @@ module Jira end def client - @client ||= jira_service.client + @client ||= jira_integration.client end def request diff --git a/app/services/jira/requests/projects/list_service.rb b/app/services/jira/requests/projects/list_service.rb index 373c536974a..ac9e9bf0be9 100644 --- a/app/services/jira/requests/projects/list_service.rb +++ b/app/services/jira/requests/projects/list_service.rb @@ -6,8 +6,8 @@ module Jira class ListService < Base extend ::Gitlab::Utils::Override - def initialize(jira_service, params = {}) - super(jira_service, params) + def initialize(jira_integration, params = {}) + super(jira_integration, params) @query = params[:query] end diff --git a/app/services/jira_connect_installations/destroy_service.rb b/app/services/jira_connect_installations/destroy_service.rb new file mode 100644 index 00000000000..cfe58575dcf --- /dev/null +++ b/app/services/jira_connect_installations/destroy_service.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module JiraConnectInstallations + class DestroyService + def self.execute(installation, jira_connect_base_path, jira_connect_uninstalled_event_path) + new(installation, jira_connect_base_path, jira_connect_uninstalled_event_path).execute + end + + def initialize(installation, jira_connect_base_path, jira_connect_uninstalled_event_path) + @installation = installation + @jira_connect_base_path = jira_connect_base_path + @jira_connect_uninstalled_event_path = jira_connect_uninstalled_event_path + end + + def execute + if @installation.instance_url.present? + JiraConnect::ForwardEventWorker.perform_async(@installation.id, @jira_connect_base_path, @jira_connect_uninstalled_event_path) + return true + end + + @installation.destroy + end + end +end diff --git a/app/services/jira_import/users_importer.rb b/app/services/jira_import/users_importer.rb index 5b2f91efc38..667a2836acc 100644 --- a/app/services/jira_import/users_importer.rb +++ b/app/services/jira_import/users_importer.rb @@ -32,9 +32,9 @@ module JiraImport end def user_mapper_service_factory - if project.jira_service.data_fields.deployment_server? + if project.jira_integration.data_fields.deployment_server? ServerUsersMapperService.new(user, project, start_at) - elsif project.jira_service.data_fields.deployment_cloud? + elsif project.jira_integration.data_fields.deployment_cloud? CloudUsersMapperService.new(user, project, start_at) else raise ArgumentError diff --git a/app/services/jira_import/users_mapper_service.rb b/app/services/jira_import/users_mapper_service.rb index 6c8610bfbf3..13e0dd5120e 100644 --- a/app/services/jira_import/users_mapper_service.rb +++ b/app/services/jira_import/users_mapper_service.rb @@ -13,7 +13,7 @@ module JiraImport def initialize(current_user, project, start_at) @current_user = current_user @project = project - @jira_service = project.jira_service + @jira_integration = project.jira_integration @start_at = start_at end @@ -29,14 +29,14 @@ module JiraImport private - attr_reader :current_user, :project, :jira_service, :start_at + attr_reader :current_user, :project, :jira_integration, :start_at def jira_users @jira_users ||= client.get(url) end def client - @client ||= jira_service.client + @client ||= jira_integration.client end def url @@ -77,7 +77,7 @@ module JiraImport end def project_member_ids - @project_member_ids ||= MembersFinder.new(project, current_user).execute.select(:user_id) + @project_member_ids ||= MembersFinder.new(project, current_user).execute.reselect(:user_id) end end end diff --git a/app/services/keys/destroy_service.rb b/app/services/keys/destroy_service.rb index eaf5eb35f58..c7ebb484e94 100644 --- a/app/services/keys/destroy_service.rb +++ b/app/services/keys/destroy_service.rb @@ -3,14 +3,22 @@ module Keys class DestroyService < ::Keys::BaseService def execute(key) - key.destroy if destroy_possible?(key) + return unless destroy_possible?(key) + + destroy(key) end + private + # overridden in EE::Keys::DestroyService def destroy_possible?(key) true end + + def destroy(key) + key.destroy + end end end -Keys::DestroyService.prepend_mod_with('Keys::DestroyService') +Keys::DestroyService.prepend_mod diff --git a/app/services/members/creator_service.rb b/app/services/members/creator_service.rb new file mode 100644 index 00000000000..f6972f81162 --- /dev/null +++ b/app/services/members/creator_service.rb @@ -0,0 +1,172 @@ +# frozen_string_literal: true + +module Members + # This class serves as more of an app-wide way we add/create members + # All roads to add members should take this path. + class CreatorService + class << self + def parsed_access_level(access_level) + access_levels.fetch(access_level) { access_level.to_i } + end + + def access_levels + raise NotImplementedError + end + + def add_users(source, users, access_level, current_user: nil, expires_at: nil) + return [] unless users.present? + + emails, users, existing_members = parse_users_list(source, users) + + Member.transaction do + (emails + users).map! do |user| + new(source, + user, + access_level, + existing_members: existing_members, + current_user: current_user, + expires_at: expires_at) + .execute + end + end + end + + private + + def parse_users_list(source, list) + emails = [] + user_ids = [] + users = [] + existing_members = {} + + list.each do |item| + case item + when User + users << item + when Integer + user_ids << item + when /\A\d+\Z/ + user_ids << item.to_i + when Devise.email_regexp + emails << item + end + end + + if user_ids.present? + users.concat(User.id_in(user_ids)) + # the below will automatically discard invalid user_ids + existing_members = source.members_and_requesters.where(user_id: user_ids).index_by(&:user_id) # rubocop:todo CodeReuse/ActiveRecord + end + + [emails, users, existing_members] + end + end + + def initialize(source, user, access_level, **args) + @source = source + @user = user + @access_level = self.class.parsed_access_level(access_level) + @args = args + end + + def execute + find_or_build_member + update_member + + member + end + + private + + attr_reader :source, :user, :access_level, :member, :args + + def update_member + return unless can_update_member? + + member.attributes = member_attributes + + if member.request? + approve_request + else + member.save + end + end + + def can_update_member? + # There is no current user for bulk actions, in which case anything is allowed + !current_user # inheriting classes will add more logic + end + + # Populates the attributes of a member. + # + # This logic resides in a separate method so that EE can extend this logic, + # without having to patch the `add_user` method directly. + def member_attributes + { + created_by: member.created_by || current_user, + access_level: access_level, + expires_at: args[:expires_at] + } + end + + def approve_request + ::Members::ApproveAccessRequestService.new(current_user, + access_level: access_level) + .execute( + member, + skip_authorization: ldap, + skip_log_audit_event: ldap + ) + end + + def current_user + args[:current_user] + end + + def find_or_build_member + @user = parse_user_param + + @member = if user.is_a?(User) + find_or_initialize_member_by_user + else + source.members.build(invite_email: user) + end + end + + # This method is used to find users that have been entered into the "Add members" field. + # These can be the User objects directly, their IDs, their emails, or new emails to be invited. + def parse_user_param + case user + when User + user + when Integer + # might not return anything - this needs enhancement + User.find_by(id: user) # rubocop:todo CodeReuse/ActiveRecord + else + # must be an email or at least we'll consider it one + User.find_by_any_email(user) || user + end + end + + def find_or_initialize_member_by_user + if existing_members + # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/334062 + # i'm not so sure this is needed as the parse_users_list looks at members_and_requesters... + # so it is like we could just do a find or initialize by here and be fine + existing_members[user.id] || source.members.build(user_id: user.id) + else + source.members_and_requesters.find_or_initialize_by(user_id: user.id) # rubocop:todo CodeReuse/ActiveRecord + end + end + + def existing_members + args[:existing_members] + end + + def ldap + args[:ldap] || false + end + end +end + +Members::CreatorService.prepend_mod_with('Members::CreatorService') diff --git a/app/services/members/groups/creator_service.rb b/app/services/members/groups/creator_service.rb new file mode 100644 index 00000000000..df4d3f59d3b --- /dev/null +++ b/app/services/members/groups/creator_service.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Members + module Groups + class CreatorService < Members::CreatorService + def self.access_levels + Gitlab::Access.sym_options_with_owner + end + + private + + def can_update_member? + super || current_user.can?(:update_group_member, member) + end + end + end +end diff --git a/app/services/members/invite_service.rb b/app/services/members/invite_service.rb index 48010f9c8e7..6298943977b 100644 --- a/app/services/members/invite_service.rb +++ b/app/services/members/invite_service.rb @@ -21,7 +21,7 @@ module Members def validate_invites! super - # we need the below due to add_users hitting Member#parse_users_list and ignoring invalid emails + # we need the below due to add_users hitting Members::CreatorService.parse_users_list and ignoring invalid emails # ideally we wouldn't need this, but we can't really change the add_users method valid, invalid = invites.partition { |email| Member.valid_email?(email) } @invites = valid diff --git a/app/services/members/projects/creator_service.rb b/app/services/members/projects/creator_service.rb new file mode 100644 index 00000000000..2e974177075 --- /dev/null +++ b/app/services/members/projects/creator_service.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Members + module Projects + class CreatorService < Members::CreatorService + def self.access_levels + Gitlab::Access.sym_options + end + + private + + def can_update_member? + super || current_user.can?(:update_project_member, member) || adding_a_new_owner? + end + + def adding_a_new_owner? + # this condition is reached during testing setup a lot due to use of `.add_user` + member.owner? && member.new_record? + end + end + end +end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 7ebdbf94932..099ab1d26e9 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -22,7 +22,7 @@ module MergeRequests def execute_hooks(merge_request, action = 'open', old_rev: nil, old_associations: {}) merge_data = hook_data(merge_request, action, old_rev: old_rev, old_associations: old_associations) merge_request.project.execute_hooks(merge_data, :merge_request_hooks) - merge_request.project.execute_services(merge_data, :merge_request_hooks) + merge_request.project.execute_integrations(merge_data, :merge_request_hooks) execute_external_hooks(merge_request, merge_data) diff --git a/app/services/merge_requests/push_options_handler_service.rb b/app/services/merge_requests/push_options_handler_service.rb index cc1e08e1606..79b7eb8d9d8 100644 --- a/app/services/merge_requests/push_options_handler_service.rb +++ b/app/services/merge_requests/push_options_handler_service.rb @@ -142,6 +142,11 @@ module MergeRequests params[:add_assignee_ids] = params.delete(:assign).keys if params.has_key?(:assign) params[:remove_assignee_ids] = params.delete(:unassign).keys if params.has_key?(:unassign) + if push_options[:milestone] + milestone = Milestone.for_projects_and_groups(@project, @project.ancestors_upto)&.find_by_name(push_options[:milestone]) + params[:milestone] = milestone if milestone + end + params end diff --git a/app/services/merge_requests/rebase_service.rb b/app/services/merge_requests/rebase_service.rb index ae8398e2335..9423194c01d 100644 --- a/app/services/merge_requests/rebase_service.rb +++ b/app/services/merge_requests/rebase_service.rb @@ -18,12 +18,6 @@ module MergeRequests end def rebase - # Ensure Gitaly isn't already running a rebase - if source_project.repository.rebase_in_progress?(merge_request.id) - log_error(exception: nil, message: 'Rebase task canceled: Another rebase is already in progress', save_message_on_model: true) - return false - end - repository.rebase(current_user, merge_request, skip_ci: @skip_ci) true diff --git a/app/services/metrics_service.rb b/app/services/metrics_service.rb index 222a5c8c79c..d27328f89cd 100644 --- a/app/services/metrics_service.rb +++ b/app/services/metrics_service.rb @@ -4,7 +4,7 @@ require 'prometheus/client/formats/text' class MetricsService def prometheus_metrics_text - Prometheus::Client::Formats::Text.marshal_multiprocess(multiprocess_metrics_path) + ::Prometheus::Client::Formats::Text.marshal_multiprocess(multiprocess_metrics_path) end def metrics_text diff --git a/app/services/namespace_settings/update_service.rb b/app/services/namespace_settings/update_service.rb index 80f15f7cc22..25525265e1c 100644 --- a/app/services/namespace_settings/update_service.rb +++ b/app/services/namespace_settings/update_service.rb @@ -14,7 +14,15 @@ module NamespaceSettings def execute validate_resource_access_token_creation_allowed_param - validate_prevent_sharing_groups_outside_hierarchy_param + + validate_settings_param_for_root_group( + param_key: :prevent_sharing_groups_outside_hierarchy, + user_policy: :change_prevent_sharing_groups_outside_hierarchy + ) + validate_settings_param_for_root_group( + param_key: :new_user_signups_cap, + user_policy: :change_new_user_signups_cap + ) if group.namespace_settings group.namespace_settings.attributes = settings_params @@ -34,12 +42,17 @@ module NamespaceSettings end end - def validate_prevent_sharing_groups_outside_hierarchy_param - return if settings_params[:prevent_sharing_groups_outside_hierarchy].nil? + def validate_settings_param_for_root_group(param_key:, user_policy:) + return if settings_params[param_key].nil? + + unless can?(current_user, user_policy, group) + settings_params.delete(param_key) + group.namespace_settings.errors.add(param_key, _('can only be changed by a group admin.')) + end - unless can?(current_user, :change_prevent_sharing_groups_outside_hierarchy, group) - settings_params.delete(:prevent_sharing_groups_outside_hierarchy) - group.namespace_settings.errors.add(:prevent_sharing_groups_outside_hierarchy, _('can only be changed by a group admin.')) + unless group.root? + settings_params.delete(param_key) + group.namespace_settings.errors.add(param_key, _('only available on top-level groups.')) end end end diff --git a/app/services/namespaces/in_product_marketing_emails_service.rb b/app/services/namespaces/in_product_marketing_emails_service.rb index 3461362b48c..f7f0cf9abe8 100644 --- a/app/services/namespaces/in_product_marketing_emails_service.rb +++ b/app/services/namespaces/in_product_marketing_emails_service.rb @@ -2,8 +2,6 @@ module Namespaces class InProductMarketingEmailsService - include Gitlab::Experimentation::GroupTypes - TRACKS = { create: { interval_days: [1, 5, 10], @@ -61,12 +59,6 @@ module Namespaces attr_reader :track, :interval, :in_product_marketing_email_records def send_email_for_group(group) - if Gitlab.com? - experiment_enabled_for_group = experiment_enabled_for_group?(group) - experiment_add_group(group, experiment_enabled_for_group) - return unless experiment_enabled_for_group - end - users_for_group(group).each do |user| if can_perform_action?(user, group) send_email(user, group) @@ -77,15 +69,6 @@ module Namespaces save_tracked_emails! end - def experiment_enabled_for_group?(group) - Gitlab::Experimentation.in_experiment_group?(:in_product_marketing_emails, subject: group) - end - - def experiment_add_group(group, experiment_enabled_for_group) - variant = experiment_enabled_for_group ? GROUP_EXPERIMENTAL : GROUP_CONTROL - Experiment.add_group(:in_product_marketing_emails, variant: variant, group: group) - end - def groups_for_track onboarding_progress_scope = OnboardingProgress .completed_actions_with_latest_in_range(completed_actions, range) diff --git a/app/services/notes/post_process_service.rb b/app/services/notes/post_process_service.rb index b7ccdbc1cff..c9375fe14a1 100644 --- a/app/services/notes/post_process_service.rb +++ b/app/services/notes/post_process_service.rb @@ -39,7 +39,7 @@ module Notes hooks_scope = note.confidential?(include_noteable: true) ? :confidential_note_hooks : :note_hooks note.project.execute_hooks(note_data, hooks_scope) - note.project.execute_services(note_data, hooks_scope) + note.project.execute_integrations(note_data, hooks_scope) end end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 9dfcfe748da..afc9015e758 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -396,6 +396,7 @@ class NotificationService recipients.each do |recipient| mailer.service_desk_new_note_email(issue.id, note.id, recipient).deliver_later + Gitlab::Metrics::BackgroundTransaction.current&.add_event(:service_desk_new_note_email) end end @@ -427,6 +428,10 @@ class NotificationService mailer.user_admin_rejection_email(name, email).deliver_later end + def user_deactivated(name, email) + mailer.user_deactivated_email(name, email).deliver_later + end + # Members def new_access_request(member) return true unless member.notifiable?(:subscription) diff --git a/app/services/packages/conan/search_service.rb b/app/services/packages/conan/search_service.rb index 143fd8a627b..31ee9bea084 100644 --- a/app/services/packages/conan/search_service.rb +++ b/app/services/packages/conan/search_service.rb @@ -41,7 +41,7 @@ module Packages end def search_for_single_package(query) - name, version, username, _ = query.split(/[@\/]/) + name, version, username, _ = query.split(%r{[@/]}) full_path = Packages::Conan::Metadatum.full_path_from(package_username: username) project = Project.find_by_full_path(full_path) return unless Ability.allowed?(current_user, :read_package, project) diff --git a/app/services/packages/destroy_package_service.rb b/app/services/packages/destroy_package_service.rb new file mode 100644 index 00000000000..697f1fa3ac8 --- /dev/null +++ b/app/services/packages/destroy_package_service.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Packages + class DestroyPackageService < BaseContainerService + alias_method :package, :container + + def execute + return service_response_error("You don't have access to this package", 403) unless user_can_delete_package? + + package.destroy! + + package.sync_maven_metadata(current_user) + + service_response_success('Package was successfully deleted') + rescue StandardError + service_response_error('Failed to remove the package', 400) + end + + private + + def service_response_error(message, http_status) + ServiceResponse.error(message: message, http_status: http_status) + end + + def service_response_success(message) + ServiceResponse.success(message: message) + end + + def user_can_delete_package? + can?(current_user, :destroy_package, package.project) + end + end +end diff --git a/app/services/packages/nuget/metadata_extraction_service.rb b/app/services/packages/nuget/metadata_extraction_service.rb index 63da98dde43..66abd189153 100644 --- a/app/services/packages/nuget/metadata_extraction_service.rb +++ b/app/services/packages/nuget/metadata_extraction_service.rb @@ -18,6 +18,7 @@ module Packages XPATH_DEPENDENCIES = '//xmlns:package/xmlns:metadata/xmlns:dependencies/xmlns:dependency' XPATH_DEPENDENCY_GROUPS = '//xmlns:package/xmlns:metadata/xmlns:dependencies/xmlns:group' XPATH_TAGS = '//xmlns:package/xmlns:metadata/xmlns:tags' + XPATH_PACKAGE_TYPES = '//xmlns:package/xmlns:metadata/xmlns:packageTypes/xmlns:packageType' MAX_FILE_SIZE = 4.megabytes.freeze @@ -57,6 +58,7 @@ module Packages .tap do |metadata| metadata[:package_dependencies] = extract_dependencies(doc) metadata[:package_tags] = extract_tags(doc) + metadata[:package_types] = extract_package_types(doc) end end @@ -85,6 +87,10 @@ module Packages }.compact end + def extract_package_types(doc) + doc.xpath(XPATH_PACKAGE_TYPES).map { |node| node.attr('name') }.uniq + end + def extract_tags(doc) tags = doc.xpath(XPATH_TAGS).text diff --git a/app/services/packages/nuget/update_package_from_metadata_service.rb b/app/services/packages/nuget/update_package_from_metadata_service.rb index 8210072eab3..2d1733421fd 100644 --- a/app/services/packages/nuget/update_package_from_metadata_service.rb +++ b/app/services/packages/nuget/update_package_from_metadata_service.rb @@ -8,6 +8,7 @@ module Packages # used by ExclusiveLeaseGuard DEFAULT_LEASE_TIMEOUT = 1.hour.to_i.freeze + SYMBOL_PACKAGE_IDENTIFIER = 'SymbolsPackage' InvalidMetadataError = Class.new(StandardError) @@ -20,7 +21,13 @@ module Packages try_obtain_lease do @package_file.transaction do - package = existing_package ? link_to_existing_package : update_linked_package + if existing_package + package = link_to_existing_package + elsif symbol_package? + raise InvalidMetadataError, 'symbol package is invalid, matching package does not exist' + else + package = update_linked_package + end update_package(package) @@ -39,6 +46,8 @@ module Packages private def update_package(package) + return if symbol_package? + ::Packages::Nuget::SyncMetadatumService .new(package, metadata.slice(:project_url, :license_url, :icon_url)) .execute @@ -103,6 +112,14 @@ module Packages metadata.fetch(:package_tags, []) end + def package_types + metadata.fetch(:package_types, []) + end + + def symbol_package? + package_types.include?(SYMBOL_PACKAGE_IDENTIFIER) + end + def metadata strong_memoize(:metadata) do ::Packages::Nuget::MetadataExtractionService.new(@package_file.id).execute @@ -110,7 +127,7 @@ module Packages end def package_filename - "#{package_name.downcase}.#{package_version.downcase}.nupkg" + "#{package_name.downcase}.#{package_version.downcase}.#{symbol_package? ? 'snupkg' : 'nupkg'}" end # used by ExclusiveLeaseGuard diff --git a/app/services/post_receive_service.rb b/app/services/post_receive_service.rb index faacabbb16c..a6d49f03c0b 100644 --- a/app/services/post_receive_service.rb +++ b/app/services/post_receive_service.rb @@ -32,11 +32,11 @@ class PostReceiveService response.add_alert_message(broadcast_message) response.add_merge_request_urls(merge_request_urls) - # Neither User nor Project are guaranteed to be returned; an orphaned write deploy + # Neither User nor Repository are guaranteed to be returned; an orphaned write deploy # key could be used - if user && project - redirect_message = Gitlab::Checks::ProjectMoved.fetch_message(user.id, project.id) - project_created_message = Gitlab::Checks::ProjectCreated.fetch_message(user.id, project.id) + if user && repository + redirect_message = Gitlab::Checks::ContainerMoved.fetch_message(user, repository) + project_created_message = Gitlab::Checks::ProjectCreated.fetch_message(user, repository) response.add_basic_message(redirect_message) response.add_basic_message(project_created_message) @@ -94,6 +94,8 @@ class PostReceiveService end def record_onboarding_progress + return unless project + OnboardingProgressService.new(project.namespace).execute(action: :git_write) end end diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 7dd9280e5b1..9a5c260e488 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -108,11 +108,7 @@ module Projects current_user.invalidate_personal_projects_count - if Feature.enabled?(:projects_post_creation_worker, current_user, default_enabled: :yaml) - Projects::PostCreationWorker.perform_async(@project.id) - else - create_prometheus_service - end + Projects::PostCreationWorker.perform_async(@project.id) create_readme if @initialize_with_readme end @@ -151,7 +147,7 @@ module Projects branch_name: @default_branch.presence || @project.default_branch_or_main, commit_message: 'Initial commit', file_path: 'README.md', - file_content: "# #{@project.name}\n\n#{@project.description}" + file_content: experiment(:new_project_readme_content, namespace: @project.namespace).run_with(@project) } Files::CreateService.new(@project, current_user, commit_attrs).execute @@ -191,25 +187,6 @@ module Projects @project end - # Deprecated: https://gitlab.com/gitlab-org/gitlab/-/issues/326665 - def create_prometheus_service - service = @project.find_or_initialize_service(::PrometheusService.to_param) - - # If the service has already been inserted in the database, that - # means it came from a template, and there's nothing more to do. - return if service.persisted? - - if service.prometheus_available? - service.save! - else - @project.prometheus_service = nil - end - - rescue ActiveRecord::RecordInvalid => e - Gitlab::ErrorTracking.track_exception(e, extra: { project_id: project.id }) - @project.prometheus_service = nil - end - def set_project_name_from_path # if both name and path set - everything is ok return if @project.name.present? && @project.path.present? diff --git a/app/services/projects/group_links/create_service.rb b/app/services/projects/group_links/create_service.rb index fc5c936b378..a0232779c97 100644 --- a/app/services/projects/group_links/create_service.rb +++ b/app/services/projects/group_links/create_service.rb @@ -13,7 +13,7 @@ module Projects ) if link.save - setup_authorizations(group, link.group_access) + setup_authorizations(group) success(link: link) else error(link.errors.full_messages.to_sentence, 409) @@ -22,9 +22,8 @@ module Projects private - def setup_authorizations(group, group_access = nil) - AuthorizedProjectUpdate::ProjectGroupLinkCreateWorker.perform_async( - project.id, group.id, group_access) + def setup_authorizations(group) + AuthorizedProjectUpdate::ProjectRecalculateWorker.perform_async(project.id) # AuthorizedProjectsWorker uses an exclusive lease per user but # specialized workers might have synchronization issues. Until we diff --git a/app/services/projects/group_links/update_service.rb b/app/services/projects/group_links/update_service.rb index 7de4b7a211d..475ab17f1a1 100644 --- a/app/services/projects/group_links/update_service.rb +++ b/app/services/projects/group_links/update_service.rb @@ -12,15 +12,29 @@ module Projects def execute(group_link_params) group_link.update!(group_link_params) - if requires_authorization_refresh?(group_link_params) - group_link.group.refresh_members_authorized_projects - end + refresh_authorizations if requires_authorization_refresh?(group_link_params) end private attr_reader :group_link + def refresh_authorizations + if Feature.enabled?(:specialized_worker_for_project_share_update_auth_recalculation) + AuthorizedProjectUpdate::ProjectRecalculateWorker.perform_async(project.id) + + # Until we compare the inconsistency rates of the new specialized worker and + # the old approach, we still run AuthorizedProjectsWorker + # but with some delay and lower urgency as a safety net. + group_link.group.refresh_members_authorized_projects( + blocking: false, + priority: UserProjectAccessChangedService::LOW_PRIORITY + ) + else + group_link.group.refresh_members_authorized_projects + end + end + def requires_authorization_refresh?(params) params.include?(:group_access) end diff --git a/app/services/projects/lfs_pointers/lfs_download_service.rb b/app/services/projects/lfs_pointers/lfs_download_service.rb index 9e2edf7c4ef..fe9dce26029 100644 --- a/app/services/projects/lfs_pointers/lfs_download_service.rb +++ b/app/services/projects/lfs_pointers/lfs_download_service.rb @@ -22,7 +22,7 @@ module Projects def execute return unless project&.lfs_enabled? && lfs_download_object return error("LFS file with oid #{lfs_oid} has invalid attributes") unless lfs_download_object.valid? - return link_existing_lfs_object! if lfs_size > LARGE_FILE_SIZE && lfs_object + return link_existing_lfs_object! if Feature.enabled?(:lfs_link_existing_object, project, default_enabled: :yaml) && lfs_size > LARGE_FILE_SIZE && lfs_object wrap_download_errors do download_lfs_file! @@ -61,8 +61,10 @@ module Projects def download_and_save_file!(file) digester = Digest::SHA256.new fetch_file do |fragment| - digester << fragment - file.write(fragment) + if digest_fragment?(fragment) + digester << fragment + file.write(fragment) + end raise_size_error! if file.size > lfs_size end @@ -71,6 +73,10 @@ module Projects raise_oid_error! if digester.hexdigest != lfs_oid end + def digest_fragment?(fragment) + fragment.http_response.is_a?(Net::HTTPSuccess) + end + def download_options http_options = { headers: lfs_headers, stream_body: true } diff --git a/app/services/projects/operations/update_service.rb b/app/services/projects/operations/update_service.rb index c0734171ee5..2cc6bcdf57c 100644 --- a/app/services/projects/operations/update_service.rb +++ b/app/services/projects/operations/update_service.rb @@ -102,10 +102,10 @@ module Projects def prometheus_integration_params return {} unless attrs = params[:prometheus_integration_attributes] - service = project.find_or_initialize_service(::PrometheusService.to_param) - service.assign_attributes(attrs) + integration = project.find_or_initialize_integration(::Integrations::Prometheus.to_param) + integration.assign_attributes(attrs) - { prometheus_service_attributes: service.attributes.except(*%w(id project_id created_at updated_at)) } + { prometheus_integration_attributes: integration.attributes.except(*%w[id project_id created_at updated_at]) } end def incident_management_setting_params diff --git a/app/services/projects/overwrite_project_service.rb b/app/services/projects/overwrite_project_service.rb index e681b5643ee..6be3b1b5a6f 100644 --- a/app/services/projects/overwrite_project_service.rb +++ b/app/services/projects/overwrite_project_service.rb @@ -20,7 +20,7 @@ module Projects rescue Exception => e # rubocop:disable Lint/RescueException attempt_restore_repositories(source_project) - if e.class == Exception + if e.instance_of?(Exception) raise StandardError, e.message else raise diff --git a/app/services/projects/prometheus/alerts/notify_service.rb b/app/services/projects/prometheus/alerts/notify_service.rb index e1eb1374d14..c1bf2e68436 100644 --- a/app/services/projects/prometheus/alerts/notify_service.rb +++ b/app/services/projects/prometheus/alerts/notify_service.rb @@ -67,7 +67,7 @@ module Projects end def valid_for_manual?(token) - prometheus = project.find_or_initialize_service('prometheus') + prometheus = project.find_or_initialize_integration('prometheus') return false unless prometheus.manual_configuration? if setting = project.alerting_setting diff --git a/app/services/projects/protect_default_branch_service.rb b/app/services/projects/protect_default_branch_service.rb index 1d3fb523448..0111b9e377a 100644 --- a/app/services/projects/protect_default_branch_service.rb +++ b/app/services/projects/protect_default_branch_service.rb @@ -22,7 +22,7 @@ module Projects # Ensure HEAD points to the default branch in case it is not master project.change_head(default_branch) - create_protected_branch if protect_branch? + create_protected_branch if protect_branch? && !protected_branch_exists? end def create_protected_branch @@ -44,6 +44,10 @@ module Projects !ProtectedBranch.protected?(project, default_branch) end + def protected_branch_exists? + project.protected_branches.find_by_name(default_branch).present? + end + def default_branch project.default_branch end diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index d9e49dfae61..fb0fea756bc 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -139,7 +139,19 @@ module Projects user_ids = @old_namespace.user_ids_for_project_authorizations | @new_namespace.user_ids_for_project_authorizations - UserProjectAccessChangedService.new(user_ids).execute + if Feature.enabled?(:specialized_worker_for_project_transfer_auth_recalculation) + AuthorizedProjectUpdate::ProjectRecalculateWorker.perform_async(project.id) + + # Until we compare the inconsistency rates of the new specialized worker and + # the old approach, we still run AuthorizedProjectsWorker + # but with some delay and lower urgency as a safety net. + UserProjectAccessChangedService.new(user_ids).execute( + blocking: false, + priority: UserProjectAccessChangedService::LOW_PRIORITY + ) + else + UserProjectAccessChangedService.new(user_ids).execute + end end def rollback_side_effects diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 8ea35131339..a90c22c7de5 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -31,10 +31,11 @@ module Projects register_attempt # Create status notifying the deployment of pages - @status = create_status - @status.update_older_statuses_retried! if Feature.enabled?(:ci_fix_commit_status_retried, project, default_enabled: :yaml) - @status.enqueue! - @status.run! + @status = build_commit_status + ::Ci::Pipelines::AddJobService.new(@build.pipeline).execute!(@status) do |job| + job.enqueue! + job.run! + end raise InvalidStateError, 'missing pages artifacts' unless build.artifacts? raise InvalidStateError, 'build SHA is outdated for this ref' unless latest? @@ -70,12 +71,9 @@ module Projects super end - def create_status + def build_commit_status GenericCommitStatus.new( - project: project, - pipeline: build.pipeline, user: build.user, - ref: build.ref, stage: 'deploy', name: 'pages:deploy' ) diff --git a/app/services/projects/update_remote_mirror_service.rb b/app/services/projects/update_remote_mirror_service.rb index eac84337967..6c29ba81910 100644 --- a/app/services/projects/update_remote_mirror_service.rb +++ b/app/services/projects/update_remote_mirror_service.rb @@ -65,7 +65,7 @@ module Projects # TODO: Support LFS sync over SSH # https://gitlab.com/gitlab-org/gitlab/-/issues/249587 - return unless remote_mirror.url =~ /\Ahttps?:\/\//i + return unless remote_mirror.url =~ %r{\Ahttps?://}i return unless remote_mirror.password_auth? Lfs::PushService.new( diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 4351a66351d..d6e7f165d72 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -66,6 +66,8 @@ module Projects previous_default_branch = project.default_branch if project.change_head(params[:default_branch]) + params[:previous_default_branch] = previous_default_branch + after_default_branch_change(previous_default_branch) else raise ValidationError, s_("UpdateProject|Could not set the default branch") diff --git a/app/services/releases/base_service.rb b/app/services/releases/base_service.rb index 9dd0c9a007a..b4b493624e7 100644 --- a/app/services/releases/base_service.rb +++ b/app/services/releases/base_service.rb @@ -5,6 +5,8 @@ module Releases include BaseServiceUtility include Gitlab::Utils::StrongMemoize + ReleaseProtectedTagAccessError = Class.new(StandardError) + attr_accessor :project, :current_user, :params def initialize(project, user = nil, params = {}) @@ -81,6 +83,15 @@ module Releases release.execute_hooks(action) end + def track_protected_tag_access_error! + unless ::Gitlab::UserAccess.new(current_user, container: project).can_create_tag?(tag_name) + Gitlab::ErrorTracking.log_exception( + ReleaseProtectedTagAccessError.new, + project_id: project.id, + user_id: current_user.id) + end + end + # overridden in EE def project_group_id; end end diff --git a/app/services/releases/create_service.rb b/app/services/releases/create_service.rb index 1096e207e02..2aac5644b84 100644 --- a/app/services/releases/create_service.rb +++ b/app/services/releases/create_service.rb @@ -7,6 +7,8 @@ module Releases return error('Release already exists', 409) if release return error("Milestone(s) not found: #{inexistent_milestones.join(', ')}", 400) if inexistent_milestones.any? + track_protected_tag_access_error! + # should be found before the creation of new tag # because tag creation can spawn new pipeline # which won't have any data for evidence yet @@ -42,7 +44,13 @@ module Releases end def allowed? - Ability.allowed?(current_user, :create_release, project) + Ability.allowed?(current_user, :create_release, project) && can_create_tag? + end + + def can_create_tag? + return true unless ::Feature.enabled?(:evalute_protected_tag_for_release_permissions, project, default_enabled: :yaml) + + ::Gitlab::UserAccess.new(current_user, container: project).can_create_tag?(tag_name) end def create_release(tag, evidence_pipeline) diff --git a/app/services/releases/destroy_service.rb b/app/services/releases/destroy_service.rb index 8abf9308689..36cf29c955d 100644 --- a/app/services/releases/destroy_service.rb +++ b/app/services/releases/destroy_service.rb @@ -6,6 +6,8 @@ module Releases return error('Release does not exist', 404) unless release return error('Access Denied', 403) unless allowed? + track_protected_tag_access_error! + if release.destroy success(tag: existing_tag, release: release) else diff --git a/app/services/releases/update_service.rb b/app/services/releases/update_service.rb index 4e78120ac05..eda4b7102c0 100644 --- a/app/services/releases/update_service.rb +++ b/app/services/releases/update_service.rb @@ -7,6 +7,8 @@ module Releases return error end + track_protected_tag_access_error! + if param_for_milestone_titles_provided? previous_milestones = release.milestones.map(&:title) params[:milestones] = milestones diff --git a/app/services/resource_access_tokens/create_service.rb b/app/services/resource_access_tokens/create_service.rb index 6ff8767a525..34aa414de8f 100644 --- a/app/services/resource_access_tokens/create_service.rb +++ b/app/services/resource_access_tokens/create_service.rb @@ -16,11 +16,12 @@ module ResourceAccessTokens return error(user.errors.full_messages.to_sentence) unless user.persisted? - member = create_membership(resource, user) + access_level = params[:access_level] || Gitlab::Access::MAINTAINER + member = create_membership(resource, user, access_level) unless member.persisted? delete_failed_user(user) - return error("Could not provision maintainer access to project access token") + return error("Could not provision #{Gitlab::Access.human_access(access_level).downcase} access to project access token") end token_response = create_personal_access_token(user) @@ -102,8 +103,8 @@ module ResourceAccessTokens Gitlab::Auth.resource_bot_scopes end - def create_membership(resource, user) - resource.add_user(user, :maintainer, expires_at: params[:expires_at]) + def create_membership(resource, user, access_level) + resource.add_user(user, access_level, expires_at: params[:expires_at]) end def log_event(token) diff --git a/app/services/security/ci_configuration/dependency_scanning_create_service.rb b/app/services/security/ci_configuration/dependency_scanning_create_service.rb new file mode 100644 index 00000000000..71e8d5025ae --- /dev/null +++ b/app/services/security/ci_configuration/dependency_scanning_create_service.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Security + module CiConfiguration + class DependencyScanningCreateService < ::Security::CiConfiguration::BaseCreateService + private + + def action + Security::CiConfiguration::DependencyScanningBuildAction.new(project.auto_devops_enabled?, existing_gitlab_ci_content).generate + end + + def next_branch + 'set-dependency-scanning-config' + end + + def message + _('Configure Dependency Scanning in `.gitlab-ci.yml`, creating this file if it does not already exist') + end + + def description + _('Configure Dependency Scanning in `.gitlab-ci.yml` using the GitLab managed template. You can [add variable overrides](https://docs.gitlab.com/ee/user/application_security/dependency_scanning/#customizing-the-dependency-scanning-settings) to customize Dependency Scanning settings.') + end + end + end +end diff --git a/app/services/service_ping/build_payload_service.rb b/app/services/service_ping/build_payload_service.rb new file mode 100644 index 00000000000..2bef3d32103 --- /dev/null +++ b/app/services/service_ping/build_payload_service.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module ServicePing + class BuildPayloadService + def execute + return {} unless allowed_to_report? + + raw_payload + end + + private + + def allowed_to_report? + product_intelligence_enabled? && !User.single_user&.requires_usage_stats_consent? + end + + def product_intelligence_enabled? + ::Gitlab::CurrentSettings.usage_ping_enabled? + end + + def raw_payload + @raw_payload ||= ::Gitlab::UsageData.data(force_refresh: true) + end + end +end + +ServicePing::BuildPayloadService.prepend_mod_with('ServicePing::BuildPayloadService') diff --git a/app/services/service_ping/permit_data_categories_service.rb b/app/services/service_ping/permit_data_categories_service.rb new file mode 100644 index 00000000000..ff48c022b56 --- /dev/null +++ b/app/services/service_ping/permit_data_categories_service.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module ServicePing + class PermitDataCategoriesService + STANDARD_CATEGORY = 'Standard' + SUBSCRIPTION_CATEGORY = 'Subscription' + OPERATIONAL_CATEGORY = 'Operational' + OPTIONAL_CATEGORY = 'Optional' + CATEGORIES = [ + STANDARD_CATEGORY, + SUBSCRIPTION_CATEGORY, + OPERATIONAL_CATEGORY, + OPTIONAL_CATEGORY + ].to_set.freeze + + def execute + return [] unless product_intelligence_enabled? + + CATEGORIES + end + + def product_intelligence_enabled? + pings_enabled? && !User.single_user&.requires_usage_stats_consent? + end + + private + + def pings_enabled? + ::Gitlab::CurrentSettings.usage_ping_enabled? + end + end +end + +ServicePing::PermitDataCategoriesService.prepend_mod_with('ServicePing::PermitDataCategoriesService') diff --git a/app/services/service_ping/submit_service.rb b/app/services/service_ping/submit_service.rb new file mode 100644 index 00000000000..5c03aa46e18 --- /dev/null +++ b/app/services/service_ping/submit_service.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +module ServicePing + class SubmitService + PRODUCTION_URL = 'https://version.gitlab.com/usage_data' + STAGING_URL = 'https://gitlab-services-version-gitlab-com-staging.gs-staging.gitlab.org/usage_data' + + METRICS = %w[leader_issues instance_issues percentage_issues leader_notes instance_notes + percentage_notes leader_milestones instance_milestones percentage_milestones + leader_boards instance_boards percentage_boards leader_merge_requests + instance_merge_requests percentage_merge_requests leader_ci_pipelines + instance_ci_pipelines percentage_ci_pipelines leader_environments instance_environments + percentage_environments leader_deployments instance_deployments percentage_deployments + leader_projects_prometheus_active instance_projects_prometheus_active + percentage_projects_prometheus_active leader_service_desk_issues instance_service_desk_issues + percentage_service_desk_issues].freeze + + SubmissionError = Class.new(StandardError) + + def execute + return unless ServicePing::PermitDataCategoriesService.new.product_intelligence_enabled? + + begin + usage_data = BuildPayloadService.new.execute + raw_usage_data, response = submit_usage_data_payload(usage_data) + rescue StandardError + return unless Gitlab::CurrentSettings.usage_ping_enabled? + + usage_data = Gitlab::UsageData.data(force_refresh: true) + raw_usage_data, response = submit_usage_data_payload(usage_data) + end + + version_usage_data_id = response.dig('conv_index', 'usage_data_id') || response.dig('dev_ops_score', 'usage_data_id') + + unless version_usage_data_id.is_a?(Integer) && version_usage_data_id > 0 + raise SubmissionError, "Invalid usage_data_id in response: #{version_usage_data_id}" + end + + raw_usage_data.update_version_metadata!(usage_data_id: version_usage_data_id) + + store_metrics(response) + end + + private + + def submit_payload(usage_data) + Gitlab::HTTP.post( + url, + body: usage_data.to_json, + allow_local_requests: true, + headers: { 'Content-type' => 'application/json' } + ) + end + + def submit_usage_data_payload(usage_data) + raise SubmissionError, 'Usage data is blank' if usage_data.blank? + + raw_usage_data = save_raw_usage_data(usage_data) + + response = submit_payload(usage_data) + + raise SubmissionError, "Unsuccessful response code: #{response.code}" unless response.success? + + [raw_usage_data, response] + end + + def save_raw_usage_data(usage_data) + RawUsageData.safe_find_or_create_by(recorded_at: usage_data[:recorded_at]) do |record| + record.payload = usage_data + end + end + + def store_metrics(response) + metrics = response['conv_index'] || response['dev_ops_score'] # leaving dev_ops_score here, as the response data comes from the gitlab-version-com + + return unless metrics.present? + + DevOpsReport::Metric.create!( + metrics.slice(*METRICS) + ) + end + + # See https://gitlab.com/gitlab-org/gitlab/-/issues/233615 for details + def url + if Rails.env.production? + PRODUCTION_URL + else + STAGING_URL + end + end + end +end + +ServicePing::SubmitService.prepend_mod diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index 8f1b481d307..6d3b63de9fd 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -2,12 +2,14 @@ module Snippets class CreateService < Snippets::BaseService - def execute - # NOTE: disable_spam_action_service can be removed when the ':snippet_spam' feature flag is removed. - disable_spam_action_service = params.delete(:disable_spam_action_service) == true - @request = params.delete(:request) - @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request) + # NOTE: For Issues::CreateService, we require the spam_params and do not default it to nil, because + # spam_checking is likely to be necessary. + def initialize(project:, current_user: nil, params: {}, spam_params:) + super(project: project, current_user: current_user, params: params) + @spam_params = spam_params + end + def execute @snippet = build_from_params return invalid_params_error(@snippet) unless valid_params? @@ -18,17 +20,15 @@ module Snippets @snippet.author = current_user - unless disable_spam_action_service - Spam::SpamActionService.new( - spammable: @snippet, - request: request, - user: current_user, - action: :create - ).execute(spam_params: spam_params) - end + Spam::SpamActionService.new( + spammable: @snippet, + spam_params: spam_params, + user: current_user, + action: :create + ).execute if save_and_commit - UserAgentDetailService.new(@snippet, request).create + UserAgentDetailService.new(spammable: @snippet, spam_params: spam_params).create Gitlab::UsageDataCounters::SnippetCounter.count(:create) move_temporary_files @@ -41,7 +41,7 @@ module Snippets private - attr_reader :snippet, :request, :spam_params + attr_reader :snippet, :spam_params def build_from_params if project diff --git a/app/services/snippets/update_service.rb b/app/services/snippets/update_service.rb index 8571bc9c869..d83b21271c0 100644 --- a/app/services/snippets/update_service.rb +++ b/app/services/snippets/update_service.rb @@ -6,12 +6,15 @@ module Snippets UpdateError = Class.new(StandardError) - def execute(snippet) - # NOTE: disable_spam_action_service can be removed when the ':snippet_spam' feature flag is removed. - disable_spam_action_service = params.delete(:disable_spam_action_service) == true - @request = params.delete(:request) - @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request) + # NOTE: For Snippets::UpdateService, we default the spam_params to nil, because spam_checking is not + # necessary in many cases, and we don't want every caller to have to explicitly pass it as nil + # to disable spam checking. + def initialize(project:, current_user: nil, params: {}, spam_params: nil) + super(project: project, current_user: current_user, params: params) + @spam_params = spam_params + end + def execute(snippet) return invalid_params_error(snippet) unless valid_params? if visibility_changed?(snippet) && !visibility_allowed?(visibility_level) @@ -20,14 +23,12 @@ module Snippets update_snippet_attributes(snippet) - unless disable_spam_action_service - Spam::SpamActionService.new( - spammable: snippet, - request: request, - user: current_user, - action: :update - ).execute(spam_params: spam_params) - end + Spam::SpamActionService.new( + spammable: snippet, + spam_params: spam_params, + user: current_user, + action: :update + ).execute if save_and_commit(snippet) Gitlab::UsageDataCounters::SnippetCounter.count(:update) @@ -40,7 +41,7 @@ module Snippets private - attr_reader :request, :spam_params + attr_reader :spam_params def visibility_changed?(snippet) visibility_level && visibility_level.to_i != snippet.visibility_level diff --git a/app/services/spam/akismet_service.rb b/app/services/spam/akismet_service.rb index e9843497dd7..d31b904f549 100644 --- a/app/services/spam/akismet_service.rb +++ b/app/services/spam/akismet_service.rb @@ -20,6 +20,7 @@ module Spam created_at: DateTime.current, author: owner_name, author_email: owner_email, + # NOTE: The akismet_client needs the option to be named `:referrer`, not `:referer` referrer: options[:referer] } diff --git a/app/services/spam/spam_action_service.rb b/app/services/spam/spam_action_service.rb index 3ae5111b994..ec16ce19cf6 100644 --- a/app/services/spam/spam_action_service.rb +++ b/app/services/spam/spam_action_service.rb @@ -4,67 +4,22 @@ module Spam class SpamActionService include SpamConstants - ## - # Utility method to filter SpamParams from parameters, which will later be passed to #execute - # after the spammable is created/updated based on the remaining parameters. - # - # Takes a hash of parameters from an incoming request to modify a model (via a controller, - # service, or GraphQL mutation). The parameters will either be camelCase (if they are - # received directly via controller params) or underscore_case (if they have come from - # a GraphQL mutation which has converted them to underscore), or in the - # headers when using the header based flow. - # - # Deletes the parameters which are related to spam and captcha processing, and returns - # them in a SpamParams parameters object. See: - # https://refactoring.com/catalog/introduceParameterObject.html - def self.filter_spam_params!(params, request) - # NOTE: The 'captcha_response' field can be expanded to multiple fields when we move to future - # alternative captcha implementations such as FriendlyCaptcha. See - # https://gitlab.com/gitlab-org/gitlab/-/issues/273480 - headers = request&.headers || {} - api = params.delete(:api) - captcha_response = read_parameter(:captcha_response, params, headers) - spam_log_id = read_parameter(:spam_log_id, params, headers)&.to_i - - SpamParams.new(api: api, captcha_response: captcha_response, spam_log_id: spam_log_id) - end - - def self.read_parameter(name, params, headers) - [ - params.delete(name), - params.delete(name.to_s.camelize(:lower).to_sym), - headers["X-GitLab-#{name.to_s.titlecase(keep_id_suffix: true).tr(' ', '-')}"] - ].compact.first - end - - attr_accessor :target, :request, :options - attr_reader :spam_log - - def initialize(spammable:, request:, user:, action:) + def initialize(spammable:, spam_params:, user:, action:) @target = spammable - @request = request + @spam_params = spam_params @user = user @action = action - @options = {} end # rubocop:disable Metrics/AbcSize - def execute(spam_params:) - if request - options[:ip_address] = request.env['action_dispatch.remote_ip'].to_s - options[:user_agent] = request.env['HTTP_USER_AGENT'] - options[:referer] = request.env['HTTP_REFERER'] - else - # TODO: This code is never used, because we do not perform a verification if there is not a - # request. Why? Should it be deleted? Or should we check even if there is no request? - options[:ip_address] = target.ip_address - options[:user_agent] = target.user_agent - end + def execute + # If spam_params is passed as `nil`, no check will be performed. This is the easiest way to allow + # composed services which may not need to do spam checking to "opt out". For example, when + # MoveService is calling CreateService, spam checking is not necessary, as no new content is + # being created. + return ServiceResponse.success(message: 'Skipped spam check because spam_params was not present') unless spam_params - recaptcha_verified = Captcha::CaptchaVerificationService.new.execute( - captcha_response: spam_params.captcha_response, - request: request - ) + recaptcha_verified = Captcha::CaptchaVerificationService.new(spam_params: spam_params).execute if recaptcha_verified # If it's a request which is already verified through CAPTCHA, @@ -73,10 +28,9 @@ module Spam ServiceResponse.success(message: "CAPTCHA successfully verified") else return ServiceResponse.success(message: 'Skipped spam check because user was allowlisted') if allowlisted?(user) - return ServiceResponse.success(message: 'Skipped spam check because request was not present') unless request return ServiceResponse.success(message: 'Skipped spam check because it was not required') unless check_for_spam? - perform_spam_service_check(spam_params.api) + perform_spam_service_check ServiceResponse.success(message: "Spam check performed. Check #{target.class.name} spammable model for any errors or CAPTCHA requirement") end end @@ -86,7 +40,7 @@ module Spam private - attr_reader :user, :action + attr_reader :user, :action, :target, :spam_params, :spam_log ## # In order to be proceed to the spam check process, the target must be @@ -104,7 +58,7 @@ module Spam ## # Performs the spam check using the spam verdict service, and modifies the target model # accordingly based on the result. - def perform_spam_service_check(api) + def perform_spam_service_check ensure_target_is_dirty # since we can check for spam, and recaptcha is not verified, @@ -113,7 +67,7 @@ module Spam case result when CONDITIONAL_ALLOW # at the moment, this means "ask for reCAPTCHA" - create_spam_log(api) + create_spam_log break if target.allow_possible_spam? @@ -122,12 +76,12 @@ module Spam # TODO: remove `unless target.allow_possible_spam?` once this flag has been passed to `SpamVerdictService` # https://gitlab.com/gitlab-org/gitlab/-/issues/214739 target.spam! unless target.allow_possible_spam? - create_spam_log(api) + create_spam_log when BLOCK_USER # TODO: improve BLOCK_USER handling, non-existent until now # https://gitlab.com/gitlab-org/gitlab/-/issues/329666 target.spam! unless target.allow_possible_spam? - create_spam_log(api) + create_spam_log when ALLOW target.clear_spam_flags! when NOOP @@ -137,16 +91,21 @@ module Spam end end - def create_spam_log(api) + def create_spam_log @spam_log = SpamLog.create!( { user_id: target.author_id, title: target.spam_title, description: target.spam_description, - source_ip: options[:ip_address], - user_agent: options[:user_agent], + source_ip: spam_params.ip_address, + user_agent: spam_params.user_agent, noteable_type: noteable_type, - via_api: api + # Now, all requests are via the API, so hardcode it to true to simplify the logic and API + # of this service. See https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/2266 + # for original introduction of `via_api` field. + # See discussion here about possibly deprecating this field: + # https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/2266#note_542527450 + via_api: true } ) @@ -159,9 +118,14 @@ module Spam target_type: noteable_type } + options = { + ip_address: spam_params.ip_address, + user_agent: spam_params.user_agent, + referer: spam_params.referer + } + SpamVerdictService.new(target: target, user: user, - request: request, options: options, context: context) end diff --git a/app/services/spam/spam_params.rb b/app/services/spam/spam_params.rb index 3420748822d..ccc17a42f01 100644 --- a/app/services/spam/spam_params.rb +++ b/app/services/spam/spam_params.rb @@ -3,30 +3,54 @@ module Spam ## # This class is a Parameter Object (https://refactoring.com/catalog/introduceParameterObject.html) - # which acts as an container abstraction for multiple parameter values related to spam and - # captcha processing for a request. + # which acts as an container abstraction for multiple values related to spam and + # captcha processing for a provided HTTP request object. + # + # It is used to encapsulate these values and allow them to be passed from the Controller/GraphQL + # layers down into to the Service layer, without needing to pass the entire request and therefore + # unnecessarily couple the Service layer to the HTTP request. # # Values contained are: # - # api: A boolean flag indicating if the request was submitted via the REST or GraphQL API # captcha_response: The response resulting from the user solving a captcha. Currently it is # a scalar reCAPTCHA response string, but it can be expanded to an object in the future to - # support other captcha implementations such as FriendlyCaptcha. - # spam_log_id: The id of a SpamLog record. + # support other captcha implementations such as FriendlyCaptcha. Obtained from + # request.headers['X-GitLab-Captcha-Response'] + # spam_log_id: The id of a SpamLog record. Obtained from request.headers['X-GitLab-Spam-Log-Id'] + # ip_address = The remote IP. Obtained from request.env['action_dispatch.remote_ip'] + # user_agent = The user agent. Obtained from request.env['HTTP_USER_AGENT'] + # referer = The HTTP referer. Obtained from request.env['HTTP_REFERER'] + # + # NOTE: The presence of these values in the request is not currently enforced. If they are missing, + # then the spam check may fail, or the SpamLog or UserAgentDetail may have missing fields. class SpamParams - attr_reader :api, :captcha_response, :spam_log_id + def self.new_from_request(request:) + self.new( + captcha_response: request.headers['X-GitLab-Captcha-Response'], + spam_log_id: request.headers['X-GitLab-Spam-Log-Id'], + ip_address: request.env['action_dispatch.remote_ip'].to_s, + user_agent: request.env['HTTP_USER_AGENT'], + referer: request.env['HTTP_REFERER'] + ) + end + + attr_reader :captcha_response, :spam_log_id, :ip_address, :user_agent, :referer - def initialize(api:, captcha_response:, spam_log_id:) - @api = api.present? + def initialize(captcha_response:, spam_log_id:, ip_address:, user_agent:, referer:) @captcha_response = captcha_response @spam_log_id = spam_log_id + @ip_address = ip_address + @user_agent = user_agent + @referer = referer end def ==(other) other.class <= self.class && - other.api == api && other.captcha_response == captcha_response && - other.spam_log_id == spam_log_id + other.spam_log_id == spam_log_id && + other.ip_address == ip_address && + other.user_agent == user_agent && + other.referer == referer end end end diff --git a/app/services/spam/spam_verdict_service.rb b/app/services/spam/spam_verdict_service.rb index 7155017b73f..8d995631db6 100644 --- a/app/services/spam/spam_verdict_service.rb +++ b/app/services/spam/spam_verdict_service.rb @@ -5,9 +5,8 @@ module Spam include AkismetMethods include SpamConstants - def initialize(user:, target:, request:, options:, context: {}) + def initialize(user:, target:, options:, context: {}) @target = target - @request = request @user = user @options = options @context = context @@ -59,7 +58,7 @@ module Spam private - attr_reader :user, :target, :request, :options, :context + attr_reader :user, :target, :options, :context def akismet_verdict if akismet.spam? diff --git a/app/services/submit_usage_ping_service.rb b/app/services/submit_usage_ping_service.rb deleted file mode 100644 index 4942dd0e913..00000000000 --- a/app/services/submit_usage_ping_service.rb +++ /dev/null @@ -1,77 +0,0 @@ -# frozen_string_literal: true - -class SubmitUsagePingService - PRODUCTION_URL = 'https://version.gitlab.com/usage_data' - STAGING_URL = 'https://gitlab-services-version-gitlab-com-staging.gs-staging.gitlab.org/usage_data' - - METRICS = %w[leader_issues instance_issues percentage_issues leader_notes instance_notes - percentage_notes leader_milestones instance_milestones percentage_milestones - leader_boards instance_boards percentage_boards leader_merge_requests - instance_merge_requests percentage_merge_requests leader_ci_pipelines - instance_ci_pipelines percentage_ci_pipelines leader_environments instance_environments - percentage_environments leader_deployments instance_deployments percentage_deployments - leader_projects_prometheus_active instance_projects_prometheus_active - percentage_projects_prometheus_active leader_service_desk_issues instance_service_desk_issues - percentage_service_desk_issues].freeze - - SubmissionError = Class.new(StandardError) - - def execute - return unless Gitlab::CurrentSettings.usage_ping_enabled? - return if User.single_user&.requires_usage_stats_consent? - - usage_data = Gitlab::UsageData.data(force_refresh: true) - - raise SubmissionError, 'Usage data is blank' if usage_data.blank? - - raw_usage_data = save_raw_usage_data(usage_data) - - response = Gitlab::HTTP.post( - url, - body: usage_data.to_json, - allow_local_requests: true, - headers: { 'Content-type' => 'application/json' } - ) - - raise SubmissionError, "Unsuccessful response code: #{response.code}" unless response.success? - - version_usage_data_id = response.dig('conv_index', 'usage_data_id') || response.dig('dev_ops_score', 'usage_data_id') - - unless version_usage_data_id.is_a?(Integer) && version_usage_data_id > 0 - raise SubmissionError, "Invalid usage_data_id in response: #{version_usage_data_id}" - end - - raw_usage_data.update_version_metadata!(usage_data_id: version_usage_data_id) - - store_metrics(response) - end - - private - - def save_raw_usage_data(usage_data) - RawUsageData.safe_find_or_create_by(recorded_at: usage_data[:recorded_at]) do |record| - record.payload = usage_data - end - end - - def store_metrics(response) - metrics = response['conv_index'] || response['dev_ops_score'] # leaving dev_ops_score here, as the response data comes from the gitlab-version-com - - return unless metrics.present? - - DevOpsReport::Metric.create!( - metrics.slice(*METRICS) - ) - end - - # See https://gitlab.com/gitlab-org/gitlab/-/issues/233615 for details - def url - if Rails.env.production? - PRODUCTION_URL - else - STAGING_URL - end - end -end - -SubmitUsagePingService.prepend_mod diff --git a/app/services/user_agent_detail_service.rb b/app/services/user_agent_detail_service.rb index 9302c86d3e6..01a98a15869 100644 --- a/app/services/user_agent_detail_service.rb +++ b/app/services/user_agent_detail_service.rb @@ -1,16 +1,21 @@ # frozen_string_literal: true class UserAgentDetailService - attr_accessor :spammable, :request - - def initialize(spammable, request) + def initialize(spammable:, spam_params:) @spammable = spammable - @request = request + @spam_params = spam_params end def create - return unless request + unless spam_params&.user_agent && spam_params&.ip_address + messasge = 'Skipped UserAgentDetail creation because necessary spam_params were not provided' + return ServiceResponse.success(message: messasge) + end - spammable.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s) + spammable.create_user_agent_detail(user_agent: spam_params.user_agent, ip_address: spam_params.ip_address) end + + private + + attr_reader :spammable, :spam_params end diff --git a/app/services/user_project_access_changed_service.rb b/app/services/user_project_access_changed_service.rb index f52502e0379..5f48f410bf7 100644 --- a/app/services/user_project_access_changed_service.rb +++ b/app/services/user_project_access_changed_service.rb @@ -20,8 +20,13 @@ class UserProjectAccessChangedService if priority == HIGH_PRIORITY AuthorizedProjectsWorker.bulk_perform_async(bulk_args) # rubocop:disable Scalability/BulkPerformWithContext else - AuthorizedProjectUpdate::UserRefreshFromReplicaWorker.bulk_perform_in( # rubocop:disable Scalability/BulkPerformWithContext - DELAY, bulk_args, batch_size: 100, batch_delay: 30.seconds) + with_related_class_context do + # We wrap the execution in `with_related_class_context`so as to obtain + # the location of the original caller + # in jobs enqueued from within `AuthorizedProjectUpdate::UserRefreshFromReplicaWorker` + AuthorizedProjectUpdate::UserRefreshFromReplicaWorker.bulk_perform_in( # rubocop:disable Scalability/BulkPerformWithContext + DELAY, bulk_args, batch_size: 100, batch_delay: 30.seconds) + end end end @@ -29,4 +34,11 @@ class UserProjectAccessChangedService result end + + private + + def with_related_class_context(&block) + current_caller_id = Gitlab::ApplicationContext.current_context_attribute('meta.caller_id').presence + Gitlab::ApplicationContext.with_context(related_class: current_caller_id, &block) + end end diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 1d5b38575bb..79bdf34392f 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -177,7 +177,6 @@ class WebHookService end def rate_limited? - return false unless Feature.enabled?(:web_hooks_rate_limit, default_enabled: :yaml) return false if rate_limit.nil? Gitlab::ApplicationRateLimiter.throttled?( diff --git a/app/services/wiki_pages/base_service.rb b/app/services/wiki_pages/base_service.rb index 4ec884469eb..891e18c0acc 100644 --- a/app/services/wiki_pages/base_service.rb +++ b/app/services/wiki_pages/base_service.rb @@ -12,7 +12,7 @@ module WikiPages def execute_hooks(page) page_data = payload(page) container.execute_hooks(page_data, :wiki_page_hooks) - container.execute_services(page_data, :wiki_page_hooks) + container.execute_integrations(page_data, :wiki_page_hooks) increment_usage create_wiki_event(page) end diff --git a/app/services/wiki_pages/create_service.rb b/app/services/wiki_pages/create_service.rb index 9702876effa..d14d94d77df 100644 --- a/app/services/wiki_pages/create_service.rb +++ b/app/services/wiki_pages/create_service.rb @@ -6,11 +6,12 @@ module WikiPages wiki = Wiki.for_container(container, current_user) page = WikiPage.new(wiki) - if page.create(@params) - execute_hooks(page) + wiki.capture_git_error(event_action) do + page.create(@params) end if page.persisted? + execute_hooks(page) ServiceResponse.success(payload: { page: page }) else ServiceResponse.error(message: _('Could not create wiki page'), payload: { page: page }) diff --git a/app/services/wiki_pages/update_service.rb b/app/services/wiki_pages/update_service.rb index 88275f8c417..12b2cf87d5d 100644 --- a/app/services/wiki_pages/update_service.rb +++ b/app/services/wiki_pages/update_service.rb @@ -8,7 +8,7 @@ module WikiPages # this class is not thread safe! @old_slug = page.slug - if page.update(@params) + if page.wiki.capture_git_error(event_action) { page.update(@params) } execute_hooks(page) ServiceResponse.success(payload: { page: page }) else diff --git a/app/services/wikis/create_attachment_service.rb b/app/services/wikis/create_attachment_service.rb index 82179459345..88a593cce48 100644 --- a/app/services/wikis/create_attachment_service.rb +++ b/app/services/wikis/create_attachment_service.rb @@ -21,7 +21,11 @@ module Wikis end def create_commit! + wiki.create_wiki_repository + commit_result(create_transformed_commit(@file_content)) + rescue Wiki::CouldNotCreateWikiError + raise_error("Error creating the wiki repository") end private |