diff options
Diffstat (limited to 'app/services')
82 files changed, 1759 insertions, 469 deletions
diff --git a/app/services/alert_management/http_integrations/update_service.rb b/app/services/alert_management/http_integrations/update_service.rb index 220c4e759f0..af079f670b8 100644 --- a/app/services/alert_management/http_integrations/update_service.rb +++ b/app/services/alert_management/http_integrations/update_service.rb @@ -9,7 +9,7 @@ module AlertManagement def initialize(integration, current_user, params) @integration = integration @current_user = current_user - @params = params + @params = params.with_indifferent_access end def execute @@ -17,7 +17,7 @@ module AlertManagement params[:token] = nil if params.delete(:regenerate_token) - if integration.update(params) + if integration.update(permitted_params) success else error(integration.errors.full_messages.to_sentence) @@ -32,6 +32,15 @@ module AlertManagement current_user&.can?(:admin_operations, integration) end + def permitted_params + params.slice(*permitted_params_keys) + end + + # overriden in EE + def permitted_params_keys + %i[name active token] + end + def error(message) ServiceResponse.error(message: message) end @@ -46,3 +55,5 @@ module AlertManagement end end end + +::AlertManagement::HttpIntegrations::UpdateService.prepend_if_ee('::EE::AlertManagement::HttpIntegrations::UpdateService') diff --git a/app/services/alert_management/process_prometheus_alert_service.rb b/app/services/alert_management/process_prometheus_alert_service.rb index 753162bfdbf..545c5581f72 100644 --- a/app/services/alert_management/process_prometheus_alert_service.rb +++ b/app/services/alert_management/process_prometheus_alert_service.rb @@ -2,9 +2,8 @@ module AlertManagement class ProcessPrometheusAlertService - include BaseServiceUtility - include Gitlab::Utils::StrongMemoize - include ::IncidentManagement::Settings + extend ::Gitlab::Utils::Override + include ::AlertManagement::AlertProcessing def initialize(project, payload) @project = project @@ -14,11 +13,10 @@ module AlertManagement def execute return bad_request unless incoming_payload.has_required_attributes? - process_alert_management_alert + process_alert return bad_request unless alert.persisted? - process_incident_issues if process_issues? - send_alert_email if send_email? + complete_post_processing_tasks ServiceResponse.success end @@ -27,110 +25,31 @@ module AlertManagement attr_reader :project, :payload - def process_alert_management_alert - if incoming_payload.resolved? - process_resolved_alert_management_alert - else - process_firing_alert_management_alert - end - end - - def process_firing_alert_management_alert - if alert.persisted? - alert.register_new_event! - reset_alert_management_alert_status - else - create_alert_management_alert - end - end + override :process_new_alert + def process_new_alert + return if resolving_alert? - def reset_alert_management_alert_status - return if alert.trigger - - logger.warn( - message: 'Unable to update AlertManagement::Alert status to triggered', - project_id: project.id, - alert_id: alert.id - ) + super end - def create_alert_management_alert - if alert.save - alert.execute_services - SystemNoteService.create_new_alert(alert, Gitlab::AlertManagement::Payload::MONITORING_TOOLS[:prometheus]) - return - end + override :process_firing_alert + def process_firing_alert + super - logger.warn( - message: 'Unable to create AlertManagement::Alert', - project_id: project.id, - alert_errors: alert.errors.messages - ) + reset_alert_status end - def process_resolved_alert_management_alert - return unless alert.persisted? - return unless auto_close_incident? - - if alert.resolve(incoming_payload.ends_at) - close_issue(alert.issue) - return - end + def reset_alert_status + return if alert.trigger logger.warn( - message: 'Unable to update AlertManagement::Alert status to resolved', + message: 'Unable to update AlertManagement::Alert status to triggered', project_id: project.id, alert_id: alert.id ) end - def close_issue(issue) - return if issue.blank? || issue.closed? - - Issues::CloseService - .new(project, User.alert_bot) - .execute(issue, system_note: false) - - SystemNoteService.auto_resolve_prometheus_alert(issue, project, User.alert_bot) if issue.reset.closed? - end - - def process_incident_issues - return if alert.issue || alert.resolved? - - IncidentManagement::ProcessAlertWorker.perform_async(nil, nil, alert.id) - end - - def send_alert_email - notification_service - .async - .prometheus_alerts_fired(project, [alert]) - end - - def logger - @logger ||= Gitlab::AppLogger - end - - def alert - strong_memoize(:alert) do - existing_alert || new_alert - end - end - - def existing_alert - strong_memoize(:existing_alert) do - AlertManagement::Alert.not_resolved.for_fingerprint(project, incoming_payload.gitlab_fingerprint).first - end - end - - def new_alert - strong_memoize(:new_alert) do - AlertManagement::Alert.new( - **incoming_payload.alert_params, - ended_at: nil - ) - end - end - + override :incoming_payload def incoming_payload strong_memoize(:incoming_payload) do Gitlab::AlertManagement::Payload.parse( @@ -141,6 +60,11 @@ module AlertManagement end end + override :resolving_alert? + def resolving_alert? + incoming_payload.resolved? + end + def bad_request ServiceResponse.error(message: 'Bad Request', http_status: :bad_request) end diff --git a/app/services/application_settings/update_service.rb b/app/services/application_settings/update_service.rb index 7792b811b4e..5e5c8ae2177 100644 --- a/app/services/application_settings/update_service.rb +++ b/app/services/application_settings/update_service.rb @@ -6,7 +6,7 @@ module ApplicationSettings attr_reader :params, :application_setting - MARKDOWN_CACHE_INVALIDATING_PARAMS = %w(asset_proxy_enabled asset_proxy_url asset_proxy_secret_key asset_proxy_whitelist).freeze + MARKDOWN_CACHE_INVALIDATING_PARAMS = %w(asset_proxy_enabled asset_proxy_url asset_proxy_secret_key asset_proxy_allowlist).freeze def execute result = update_settings diff --git a/app/services/authorized_project_update/recalculate_for_user_range_service.rb b/app/services/authorized_project_update/recalculate_for_user_range_service.rb index 14b0f5d6117..f300c45f019 100644 --- a/app/services/authorized_project_update/recalculate_for_user_range_service.rb +++ b/app/services/authorized_project_update/recalculate_for_user_range_service.rb @@ -9,7 +9,7 @@ module AuthorizedProjectUpdate def execute User.where(id: start_user_id..end_user_id).select(:id).find_each do |user| # rubocop: disable CodeReuse/ActiveRecord - Users::RefreshAuthorizedProjectsService.new(user).execute + Users::RefreshAuthorizedProjectsService.new(user, source: self.class.name).execute end end diff --git a/app/services/boards/create_service.rb b/app/services/boards/create_service.rb index 2ccaea64d14..54dab581686 100644 --- a/app/services/boards/create_service.rb +++ b/app/services/boards/create_service.rb @@ -13,11 +13,11 @@ module Boards private def can_create_board? - parent.boards.empty? || parent.multiple_issue_boards_available? + parent_board_collection.empty? || parent.multiple_issue_boards_available? end def create_board! - board = parent.boards.create(params) + board = parent_board_collection.create(params) unless board.persisted? return ServiceResponse.error(message: "There was an error when creating a board.", payload: board) @@ -30,6 +30,10 @@ module Boards ServiceResponse.success(payload: board) end + + def parent_board_collection + parent.boards + end end end diff --git a/app/services/bulk_create_integration_service.rb b/app/services/bulk_create_integration_service.rb index df78c3645c7..ae756d0856e 100644 --- a/app/services/bulk_create_integration_service.rb +++ b/app/services/bulk_create_integration_service.rb @@ -11,8 +11,6 @@ class BulkCreateIntegrationService service_list = ServiceList.new(batch, service_hash, association).to_array Service.transaction do - run_callbacks(batch) if association == 'project' - results = bulk_insert(*service_list) if integration.data_fields_present? @@ -33,14 +31,6 @@ class BulkCreateIntegrationService klass.insert_all(items_to_insert, returning: [:id]) end - # rubocop: disable CodeReuse/ActiveRecord - def run_callbacks(batch) - if integration.external_issue_tracker? - Project.where(id: batch.select(:id)).update_all(has_external_issue_tracker: true) - end - end - # rubocop: enable CodeReuse/ActiveRecord - def service_hash if integration.template? integration.to_service_hash diff --git a/app/services/captcha/captcha_verification_service.rb b/app/services/captcha/captcha_verification_service.rb new file mode 100644 index 00000000000..45a5a52367c --- /dev/null +++ b/app/services/captcha/captcha_verification_service.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module Captcha + ## + # Encapsulates logic of checking captchas. + # + class CaptchaVerificationService + include Recaptcha::Verify + + ## + # 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 + + @request = request + + Gitlab::Recaptcha.load_configurations! + + # NOTE: We could pass the model and let the recaptcha gem automatically add errors to it, + # but we do not, for two reasons: + # + # 1. We want control over when the errors are added + # 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) + end + + private + + # 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 + end +end diff --git a/app/services/ci/generate_codequality_mr_diff_report_service.rb b/app/services/ci/generate_codequality_mr_diff_report_service.rb new file mode 100644 index 00000000000..3b1bd319a4f --- /dev/null +++ b/app/services/ci/generate_codequality_mr_diff_report_service.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Ci + # TODO: a couple of points with this approach: + # + reuses existing architecture and reactive caching + # - it's not a report comparison and some comparing features must be turned off. + # see CompareReportsBaseService for more notes. + # issue: https://gitlab.com/gitlab-org/gitlab/issues/34224 + class GenerateCodequalityMrDiffReportService < CompareReportsBaseService + def execute(base_pipeline, head_pipeline) + merge_request = MergeRequest.find_by_id(params[:id]) + { + status: :parsed, + key: key(base_pipeline, head_pipeline), + data: head_pipeline.pipeline_artifacts.find_by_file_type(:code_quality_mr_diff).present.for_files(merge_request.new_paths) + } + rescue => e + Gitlab::ErrorTracking.track_exception(e, project_id: project.id) + { + status: :error, + key: key(base_pipeline, head_pipeline), + status_reason: _('An error occurred while fetching codequality mr diff reports.') + } + end + + def latest?(base_pipeline, head_pipeline, data) + data&.fetch(:key, nil) == key(base_pipeline, head_pipeline) + end + end +end diff --git a/app/services/ci/generate_coverage_reports_service.rb b/app/services/ci/generate_coverage_reports_service.rb index 063fb966183..b3aa7b3091b 100644 --- a/app/services/ci/generate_coverage_reports_service.rb +++ b/app/services/ci/generate_coverage_reports_service.rb @@ -12,7 +12,7 @@ module Ci { status: :parsed, key: key(base_pipeline, head_pipeline), - data: head_pipeline.pipeline_artifacts.find_with_code_coverage.present.for_files(merge_request.new_paths) + data: head_pipeline.pipeline_artifacts.find_by_file_type(:code_coverage).present.for_files(merge_request.new_paths) } rescue => e Gitlab::ErrorTracking.track_exception(e, project_id: project.id) diff --git a/app/services/ci/pipeline_artifacts/create_code_quality_mr_diff_report_service.rb b/app/services/ci/pipeline_artifacts/create_code_quality_mr_diff_report_service.rb new file mode 100644 index 00000000000..8a4ba039fa3 --- /dev/null +++ b/app/services/ci/pipeline_artifacts/create_code_quality_mr_diff_report_service.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true +module Ci + module PipelineArtifacts + class CreateCodeQualityMrDiffReportService + def execute(pipeline) + return unless pipeline.can_generate_codequality_reports? + return if pipeline.has_codequality_mr_diff_report? + + file = build_carrierwave_file(pipeline) + + pipeline.pipeline_artifacts.create!( + project_id: pipeline.project_id, + file_type: :code_quality_mr_diff, + file_format: :raw, + size: file["tempfile"].size, + file: file, + expire_at: Ci::PipelineArtifact::EXPIRATION_DATE.from_now + ) + end + + private + + def build_carrierwave_file(pipeline) + CarrierWaveStringFile.new_file( + file_content: build_quality_mr_diff_report(pipeline), + filename: Ci::PipelineArtifact::DEFAULT_FILE_NAMES.fetch(:code_quality_mr_diff), + content_type: 'application/json' + ) + end + + def build_quality_mr_diff_report(pipeline) + mr_diff_report = Gitlab::Ci::Reports::CodequalityMrDiff.new(pipeline.codequality_reports) + + Ci::CodequalityMrDiffReportSerializer.new.represent(mr_diff_report).to_json # rubocop: disable CodeReuse/Serializer + end + end + end +end diff --git a/app/services/ci/process_build_service.rb b/app/services/ci/process_build_service.rb index dd7b562cdb7..733aa75f255 100644 --- a/app/services/ci/process_build_service.rb +++ b/app/services/ci/process_build_service.rb @@ -26,7 +26,7 @@ module Ci end def valid_statuses_for_build(build) - if ::Feature.enabled?(:skip_dag_manual_and_delayed_jobs, default_enabled: :yaml) + if ::Feature.enabled?(:skip_dag_manual_and_delayed_jobs, build.project, default_enabled: :yaml) current_valid_statuses_for_build(build) else legacy_valid_statuses_for_build(build) diff --git a/app/services/ci/process_pipeline_service.rb b/app/services/ci/process_pipeline_service.rb index e511e26adfe..678b386fbbf 100644 --- a/app/services/ci/process_pipeline_service.rb +++ b/app/services/ci/process_pipeline_service.rb @@ -38,10 +38,15 @@ module Ci # mark builds that are retried if latest_statuses.any? - pipeline.latest_statuses - .where(name: latest_statuses.map(&:second)) - .where.not(id: latest_statuses.map(&:first)) - .update_all(retried: true) + updated_count = pipeline.latest_statuses + .where(name: latest_statuses.map(&:second)) + .where.not(id: latest_statuses.map(&:first)) + .update_all(retried: true) + + # This counter is temporary. It will be used to check whether if we still use this method or not + # after setting correct value of `GenericCommitStatus#retried`. + # More info: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/50465#note_491657115 + metrics.legacy_update_jobs_counter.increment if updated_count > 0 end end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/services/ci/prometheus_metrics/observe_histograms_service.rb b/app/services/ci/prometheus_metrics/observe_histograms_service.rb new file mode 100644 index 00000000000..ee22ea75df9 --- /dev/null +++ b/app/services/ci/prometheus_metrics/observe_histograms_service.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +module Ci + module PrometheusMetrics + class ObserveHistogramsService + class << self + def available_histograms + @available_histograms ||= [ + histogram(:pipeline_graph_link_calculation_duration_seconds, 'Total time spent calculating links, in seconds', {}, [0.05, 0.1, 0.2, 0.3, 0.4, 0.5, 0.8, 1, 2]), + histogram(:pipeline_graph_links_total, 'Number of links per graph', {}, [1, 5, 10, 25, 50, 100, 200]), + histogram(:pipeline_graph_links_per_job_ratio, 'Ratio of links to job per graph', {}, [0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1]) + ].to_h + end + + private + + def histogram(name, *attrs) + [name.to_s, proc { Gitlab::Metrics.histogram(name, *attrs) }] + end + end + + def initialize(project, params) + @project = project + @params = params + end + + def execute + return ServiceResponse.success(http_status: :accepted) unless enabled? + + params + .fetch(:histograms, []) + .each(&method(:observe)) + + ServiceResponse.success(http_status: :created) + end + + private + + attr_reader :project, :params + + def observe(data) + histogram = find_histogram(data[:name]) + histogram.observe({ project: project.full_path }, data[:value].to_f) + end + + def find_histogram(name) + self.class.available_histograms + .fetch(name) { raise ActiveRecord::RecordNotFound } + .call + end + + def enabled? + ::Feature.enabled?(:ci_accept_frontend_prometheus_metrics, project, default_enabled: :yaml) + end + end + end +end diff --git a/app/services/ci/resource_groups/assign_resource_from_resource_group_service.rb b/app/services/ci/resource_groups/assign_resource_from_resource_group_service.rb index a4bcca8e8b3..9e3e6de3928 100644 --- a/app/services/ci/resource_groups/assign_resource_from_resource_group_service.rb +++ b/app/services/ci/resource_groups/assign_resource_from_resource_group_service.rb @@ -7,8 +7,8 @@ module Ci def execute(resource_group) free_resources = resource_group.resources.free.count - resource_group.builds.waiting_for_resource.take(free_resources).each do |build| - build.enqueue_waiting_for_resource + resource_group.processables.waiting_for_resource.take(free_resources).each do |processable| + processable.enqueue_waiting_for_resource end end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/services/clusters/kubernetes/configure_istio_ingress_service.rb b/app/services/clusters/kubernetes/configure_istio_ingress_service.rb index 53c3c686f07..3b7e094bc97 100644 --- a/app/services/clusters/kubernetes/configure_istio_ingress_service.rb +++ b/app/services/clusters/kubernetes/configure_istio_ingress_service.rb @@ -60,7 +60,7 @@ module Clusters cert.public_key = key.public_key cert.subject = name cert.issuer = name - cert.sign(key, OpenSSL::Digest::SHA256.new) + cert.sign(key, OpenSSL::Digest.new('SHA256')) serverless_domain_cluster.update!( key: key.to_pem, diff --git a/app/services/concerns/alert_management/alert_processing.rb b/app/services/concerns/alert_management/alert_processing.rb new file mode 100644 index 00000000000..4143a4668f5 --- /dev/null +++ b/app/services/concerns/alert_management/alert_processing.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +module AlertManagement + # Module to support the processing of new alert payloads + # from various sources. Payloads may be for new alerts, + # existing alerts, or acting as a resolving alert. + # + # Performs processing-related tasks, such as creating system + # notes, creating or resolving related issues, and notifying + # stakeholders of the alert. + # + # Requires #project [Project] and #payload [Hash] methods + # to be defined. + module AlertProcessing + include BaseServiceUtility + include Gitlab::Utils::StrongMemoize + include ::IncidentManagement::Settings + + # Updates or creates alert from payload for project + # including system notes + def process_alert + if alert.persisted? + process_existing_alert + else + process_new_alert + end + end + + # Creates or closes issue for alert and notifies stakeholders + def complete_post_processing_tasks + process_incident_issues if process_issues? + send_alert_email if send_email? + end + + def process_existing_alert + if resolving_alert? + process_resolved_alert + else + process_firing_alert + end + end + + def process_resolved_alert + return unless auto_close_incident? + return close_issue(alert.issue) if alert.resolve(incoming_payload.ends_at) + + logger.warn( + message: 'Unable to update AlertManagement::Alert status to resolved', + project_id: project.id, + alert_id: alert.id + ) + end + + def process_firing_alert + alert.register_new_event! + end + + def close_issue(issue) + return if issue.blank? || issue.closed? + + ::Issues::CloseService + .new(project, User.alert_bot) + .execute(issue, system_note: false) + + SystemNoteService.auto_resolve_prometheus_alert(issue, project, User.alert_bot) if issue.reset.closed? + end + + def process_new_alert + if alert.save + alert.execute_services + SystemNoteService.create_new_alert(alert, alert_source) + else + logger.warn( + message: "Unable to create AlertManagement::Alert from #{alert_source}", + project_id: project.id, + alert_errors: alert.errors.messages + ) + end + end + + def process_incident_issues + return if alert.issue || alert.resolved? + + ::IncidentManagement::ProcessAlertWorker.perform_async(nil, nil, alert.id) + end + + def send_alert_email + notification_service + .async + .prometheus_alerts_fired(project, [alert]) + end + + def incoming_payload + strong_memoize(:incoming_payload) do + Gitlab::AlertManagement::Payload.parse(project, payload.to_h) + end + end + + def alert + strong_memoize(:alert) do + find_existing_alert || build_new_alert + end + end + + def find_existing_alert + return unless incoming_payload.gitlab_fingerprint + + AlertManagement::Alert.not_resolved.for_fingerprint(project, incoming_payload.gitlab_fingerprint).first + end + + def build_new_alert + AlertManagement::Alert.new(**incoming_payload.alert_params, ended_at: nil) + end + + def resolving_alert? + incoming_payload.ends_at.present? + end + + def alert_source + alert.monitoring_tool + end + + def logger + @logger ||= Gitlab::AppLogger + end + end +end diff --git a/app/services/concerns/integrations/project_test_data.rb b/app/services/concerns/integrations/project_test_data.rb index 72c12cfb394..11eb2cd4ca7 100644 --- a/app/services/concerns/integrations/project_test_data.rb +++ b/app/services/concerns/integrations/project_test_data.rb @@ -8,22 +8,41 @@ module Integrations Gitlab::DataBuilder::Push.build_sample(project, current_user) end + def use_optimal_query? + Feature.enabled?(:integrations_test_webhook_optimizations, project) + end + def note_events_data - note = project.notes.first + note = if use_optimal_query? + NotesFinder.new(current_user, project: project, target: project).execute.reorder(nil).last # rubocop: disable CodeReuse/ActiveRecord + else + project.notes.first + end + return { error: s_('TestHooks|Ensure the project has notes.') } unless note.present? Gitlab::DataBuilder::Note.build(note, current_user) end def issues_events_data - issue = project.issues.first + issue = if use_optimal_query? + IssuesFinder.new(current_user, project_id: project.id, sort: 'created_desc').execute.first + else + project.issues.first + end + return { error: s_('TestHooks|Ensure the project has issues.') } unless issue.present? issue.to_hook_data(current_user) end def merge_requests_events_data - merge_request = project.merge_requests.first + merge_request = if use_optimal_query? + MergeRequestsFinder.new(current_user, project_id: project.id, sort: 'created_desc').execute.first + else + project.merge_requests.first + end + return { error: s_('TestHooks|Ensure the project has merge requests.') } unless merge_request.present? merge_request.to_hook_data(current_user) diff --git a/app/services/concerns/spam_check_methods.rb b/app/services/concerns/spam_check_methods.rb deleted file mode 100644 index 939f8f183ab..00000000000 --- a/app/services/concerns/spam_check_methods.rb +++ /dev/null @@ -1,39 +0,0 @@ -# frozen_string_literal: true - -# SpamCheckMethods -# -# Provide helper methods for checking if a given spammable object has -# potential spam data. -# -# Dependencies: -# - params with :request - -module SpamCheckMethods - # rubocop:disable Gitlab/ModuleWithInstanceVariables - def filter_spam_check_params - @request = params.delete(:request) - @api = params.delete(:api) - @recaptcha_verified = params.delete(:recaptcha_verified) - @spam_log_id = params.delete(:spam_log_id) - end - # rubocop:enable Gitlab/ModuleWithInstanceVariables - - # In order to be proceed to the spam check process, @spammable has to be - # a dirty instance, which means it should be already assigned with the new - # attribute values. - # rubocop:disable Gitlab/ModuleWithInstanceVariables - def spam_check(spammable, user, action:) - raise ArgumentError.new('Please provide an action, such as :create') unless action - - Spam::SpamActionService.new( - spammable: spammable, - request: @request, - user: user, - context: { action: action } - ).execute( - api: @api, - recaptcha_verified: @recaptcha_verified, - spam_log_id: @spam_log_id) - end - # rubocop:enable Gitlab/ModuleWithInstanceVariables -end diff --git a/app/services/concerns/update_repository_storage_methods.rb b/app/services/concerns/update_repository_storage_methods.rb index c3a55e9379e..6e4824bd784 100644 --- a/app/services/concerns/update_repository_storage_methods.rb +++ b/app/services/concerns/update_repository_storage_methods.rb @@ -1,8 +1,9 @@ # frozen_string_literal: true module UpdateRepositoryStorageMethods + include Gitlab::Utils::StrongMemoize + Error = Class.new(StandardError) - SameFilesystemError = Class.new(Error) attr_reader :repository_storage_move delegate :container, :source_storage_name, :destination_storage_name, to: :repository_storage_move @@ -18,9 +19,7 @@ module UpdateRepositoryStorageMethods repository_storage_move.start! end - raise SameFilesystemError if same_filesystem?(source_storage_name, destination_storage_name) - - mirror_repositories + mirror_repositories unless same_filesystem? repository_storage_move.transaction do repository_storage_move.finish_replication! @@ -28,8 +27,10 @@ module UpdateRepositoryStorageMethods track_repository(destination_storage_name) end - remove_old_paths - enqueue_housekeeping + unless same_filesystem? + remove_old_paths + enqueue_housekeeping + end repository_storage_move.finish_cleanup! @@ -80,8 +81,10 @@ module UpdateRepositoryStorageMethods end end - def same_filesystem?(old_storage, new_storage) - Gitlab::GitalyClient.filesystem_id(old_storage) == Gitlab::GitalyClient.filesystem_id(new_storage) + def same_filesystem? + strong_memoize(:same_filesystem) do + Gitlab::GitalyClient.filesystem_id(source_storage_name) == Gitlab::GitalyClient.filesystem_id(destination_storage_name) + end end def remove_old_paths diff --git a/app/services/dependency_proxy/find_or_create_manifest_service.rb b/app/services/dependency_proxy/find_or_create_manifest_service.rb index 6b46f5e4c59..ee608d715aa 100644 --- a/app/services/dependency_proxy/find_or_create_manifest_service.rb +++ b/app/services/dependency_proxy/find_or_create_manifest_service.rb @@ -13,7 +13,7 @@ module DependencyProxy def execute @manifest = @group.dependency_proxy_manifests - .find_or_initialize_by_file_name(@file_name) + .find_or_initialize_by_file_name_or_digest(file_name: @file_name, digest: @tag) head_result = DependencyProxy::HeadManifestService.new(@image, @tag, @token).execute @@ -30,6 +30,7 @@ module DependencyProxy def pull_new_manifest DependencyProxy::PullManifestService.new(@image, @tag, @token).execute_with_manifest do |new_manifest| @manifest.update!( + content_type: new_manifest[:content_type], digest: new_manifest[:digest], file: new_manifest[:file], size: new_manifest[:file].size @@ -38,7 +39,9 @@ module DependencyProxy end def cached_manifest_matches?(head_result) - @manifest && @manifest.digest == head_result[:digest] + return false if head_result[:status] == :error + + @manifest && @manifest.digest == head_result[:digest] && @manifest.content_type == head_result[:content_type] end def respond diff --git a/app/services/dependency_proxy/head_manifest_service.rb b/app/services/dependency_proxy/head_manifest_service.rb index 87d9c417c98..ecc3eb77399 100644 --- a/app/services/dependency_proxy/head_manifest_service.rb +++ b/app/services/dependency_proxy/head_manifest_service.rb @@ -2,6 +2,8 @@ module DependencyProxy class HeadManifestService < DependencyProxy::BaseService + ACCEPT_HEADERS = ::ContainerRegistry::Client::ACCEPTED_TYPES.join(',') + def initialize(image, tag, token) @image = image @tag = tag @@ -9,10 +11,10 @@ module DependencyProxy end def execute - response = Gitlab::HTTP.head(manifest_url, headers: auth_headers) + response = Gitlab::HTTP.head(manifest_url, headers: auth_headers.merge(Accept: ACCEPT_HEADERS)) if response.success? - success(digest: response.headers['docker-content-digest']) + success(digest: response.headers['docker-content-digest'], content_type: response.headers['content-type']) else error(response.body, response.code) end diff --git a/app/services/dependency_proxy/pull_manifest_service.rb b/app/services/dependency_proxy/pull_manifest_service.rb index 5c804489fd1..737414c396e 100644 --- a/app/services/dependency_proxy/pull_manifest_service.rb +++ b/app/services/dependency_proxy/pull_manifest_service.rb @@ -11,7 +11,7 @@ module DependencyProxy def execute_with_manifest raise ArgumentError, 'Block must be provided' unless block_given? - response = Gitlab::HTTP.get(manifest_url, headers: auth_headers) + response = Gitlab::HTTP.get(manifest_url, headers: auth_headers.merge(Accept: ::ContainerRegistry::Client::ACCEPTED_TYPES.join(','))) if response.success? file = Tempfile.new @@ -20,7 +20,7 @@ module DependencyProxy file.write(response) file.flush - yield(success(file: file, digest: response.headers['docker-content-digest'])) + yield(success(file: file, digest: response.headers['docker-content-digest'], content_type: response.headers['content-type'])) ensure file.close file.unlink diff --git a/app/services/deployments/create_service.rb b/app/services/deployments/create_service.rb index 7355747d778..ebf2b077bca 100644 --- a/app/services/deployments/create_service.rb +++ b/app/services/deployments/create_service.rb @@ -11,6 +11,8 @@ module Deployments end def execute + return last_deployment if last_deployment&.equal_to?(params) + environment.deployments.build(deployment_attributes).tap do |deployment| # Deployment#change_status already saves the model, so we only need to # call #save ourselves if no status is provided. @@ -36,5 +38,11 @@ module Deployments on_stop: params[:on_stop] } end + + private + + def last_deployment + @environment.last_deployment + end end end diff --git a/app/services/discussions/resolve_service.rb b/app/services/discussions/resolve_service.rb index cd5925cd9be..91c3cf136a4 100644 --- a/app/services/discussions/resolve_service.rb +++ b/app/services/discussions/resolve_service.rb @@ -40,7 +40,13 @@ module Discussions discussion.resolve!(current_user) @resolved_count += 1 - MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request) if merge_request + if merge_request + Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter + .track_resolve_thread_action(user: current_user) + + MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request) + end + SystemNoteService.discussion_continued_in_issue(discussion, project, current_user, follow_up_issue) if follow_up_issue end diff --git a/app/services/discussions/unresolve_service.rb b/app/services/discussions/unresolve_service.rb new file mode 100644 index 00000000000..fbd96ceafe7 --- /dev/null +++ b/app/services/discussions/unresolve_service.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Discussions + class UnresolveService < Discussions::BaseService + include Gitlab::Utils::StrongMemoize + + def initialize(discussion, user) + @discussion = discussion + @user = user + + super + end + + def execute + @discussion.unresolve! + + Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter + .track_unresolve_thread_action(user: @user) + end + end +end diff --git a/app/services/draft_notes/publish_service.rb b/app/services/draft_notes/publish_service.rb index 316abff4552..82917241347 100644 --- a/app/services/draft_notes/publish_service.rb +++ b/app/services/draft_notes/publish_service.rb @@ -38,6 +38,8 @@ module DraftNotes end draft_notes.delete_all + set_reviewed + notification_service.async.new_review(review) MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request) end @@ -64,5 +66,9 @@ module DraftNotes discussion.unresolve! end end + + def set_reviewed + ::MergeRequests::MarkReviewerReviewedService.new(project, current_user).execute(merge_request) + end end end diff --git a/app/services/feature_flags/base_service.rb b/app/services/feature_flags/base_service.rb index c11c465252e..f48f95e2550 100644 --- a/app/services/feature_flags/base_service.rb +++ b/app/services/feature_flags/base_service.rb @@ -41,7 +41,6 @@ module FeatureFlags def sync_to_jira(feature_flag) return unless feature_flag.present? - return unless Feature.enabled?(:jira_sync_feature_flags, feature_flag.project) seq_id = ::Atlassian::JiraConnect::Client.generate_update_sequence_id feature_flag.run_after_commit do diff --git a/app/services/git/branch_hooks_service.rb b/app/services/git/branch_hooks_service.rb index 4edcff0e3d0..19b9b439fed 100644 --- a/app/services/git/branch_hooks_service.rb +++ b/app/services/git/branch_hooks_service.rb @@ -44,11 +44,7 @@ module Git def invalidated_file_types return super unless default_branch? && !creating_branch? - paths = limited_commits.each_with_object(Set.new) do |commit, set| - commit.raw_deltas.each do |diff| - set << diff.new_path - end - end + paths = commit_paths.values.reduce(&:merge) || Set.new Gitlab::FileDetector.types_in_paths(paths) end @@ -77,6 +73,7 @@ module Git enqueue_process_commit_messages enqueue_jira_connect_sync_messages enqueue_metrics_dashboard_sync + track_ci_config_change_event end def branch_remove_hooks @@ -89,6 +86,18 @@ module Git ::Metrics::Dashboard::SyncDashboardsWorker.perform_async(project.id) end + def track_ci_config_change_event + return unless Gitlab::CurrentSettings.usage_ping_enabled? + return unless ::Feature.enabled?(:usage_data_unique_users_committing_ciconfigfile, project, default_enabled: :yaml) + return unless default_branch? + + commits_changing_ci_config.each do |commit| + Gitlab::UsageDataCounters::HLLRedisCounter.track_event( + 'o_pipeline_authoring_unique_users_committing_ciconfigfile', values: commit.author&.id + ) + end + end + # Schedules processing of commit messages def enqueue_process_commit_messages referencing_commits = limited_commits.select(&:matches_cross_reference_regex?) @@ -190,6 +199,23 @@ module Git set end + + def commits_changing_ci_config + commit_paths.select do |commit, paths| + next if commit.merge_commit? + + paths.include?(project.ci_config_path_or_default) + end.keys + end + + def commit_paths + strong_memoize(:commit_paths) do + limited_commits.map do |commit| + paths = Set.new(commit.raw_deltas.map(&:new_path)) + [commit, paths] + end.to_h + end + end end end diff --git a/app/services/git/wiki_push_service.rb b/app/services/git/wiki_push_service.rb index 87e2be858c0..99659bc8ab2 100644 --- a/app/services/git/wiki_push_service.rb +++ b/app/services/git/wiki_push_service.rb @@ -16,6 +16,7 @@ module Git wiki.after_post_receive process_changes + perform_housekeeping if Feature.enabled?(:wiki_housekeeping, wiki.container) end private @@ -72,6 +73,14 @@ module Git def default_branch_changes @default_branch_changes ||= changes.select { |change| on_default_branch?(change) } end + + def perform_housekeeping + housekeeping = Repositories::HousekeepingService.new(wiki) + housekeeping.increment! + housekeeping.execute if housekeeping.needed? + rescue Repositories::HousekeepingService::LeaseTaken + # no-op + end end end diff --git a/app/services/groups/import_export/export_service.rb b/app/services/groups/import_export/export_service.rb index abac0ffc5d9..a436aec1b39 100644 --- a/app/services/groups/import_export/export_service.rb +++ b/app/services/groups/import_export/export_service.rb @@ -12,40 +12,44 @@ module Groups end def async_execute - GroupExportWorker.perform_async(@current_user.id, @group.id, @params) + GroupExportWorker.perform_async(current_user.id, group.id, params) end def execute validate_user_permissions - remove_existing_export! if @group.export_file_exists? + remove_existing_export! if group.export_file_exists? save! ensure - remove_base_tmp_dir + remove_archive_tmp_dir end private + attr_reader :group, :current_user, :params attr_accessor :shared def validate_user_permissions - unless @current_user.can?(:admin_group, @group) - @shared.error(::Gitlab::ImportExport::Error.permission_error(@current_user, @group)) + unless current_user.can?(:admin_group, group) + shared.error(::Gitlab::ImportExport::Error.permission_error(current_user, group)) notify_error! end end def remove_existing_export! - import_export_upload = @group.import_export_upload + import_export_upload = group.import_export_upload import_export_upload.remove_export_file! import_export_upload.save end def save! - if savers.all?(&:save) + # We cannot include the file_saver with the other savers because + # it removes the tmp dir. This means that if we want to add new savers + # in EE the data won't be available. + if savers.all?(&:save) && file_saver.save notify_success else notify_error! @@ -53,36 +57,40 @@ module Groups end def savers - [version_saver, tree_exporter, file_saver] + [version_saver, tree_exporter] end def tree_exporter tree_exporter_class.new( - group: @group, - current_user: @current_user, - shared: @shared, - params: @params + group: group, + current_user: current_user, + shared: shared, + params: params ) end def tree_exporter_class - if ::Feature.enabled?(:group_export_ndjson, @group&.parent, default_enabled: true) + if ndjson? Gitlab::ImportExport::Group::TreeSaver else Gitlab::ImportExport::Group::LegacyTreeSaver end end + def ndjson? + ::Feature.enabled?(:group_export_ndjson, group&.parent, default_enabled: :yaml) + end + def version_saver Gitlab::ImportExport::VersionSaver.new(shared: shared) end def file_saver - Gitlab::ImportExport::Saver.new(exportable: @group, shared: @shared) + Gitlab::ImportExport::Saver.new(exportable: group, shared: shared) end - def remove_base_tmp_dir - FileUtils.rm_rf(shared.base_path) if shared&.base_path + def remove_archive_tmp_dir + FileUtils.rm_rf(shared.archive_path) if shared&.archive_path end def notify_error! @@ -94,22 +102,22 @@ module Groups def notify_success @logger.info( message: 'Group Export succeeded', - group_id: @group.id, - group_name: @group.name + group_id: group.id, + group_name: group.name ) - notification_service.group_was_exported(@group, @current_user) + notification_service.group_was_exported(group, current_user) end def notify_error @logger.error( message: 'Group Export failed', - group_id: @group.id, - group_name: @group.name, - errors: @shared.errors.join(', ') + group_id: group.id, + group_name: group.name, + errors: shared.errors.join(', ') ) - notification_service.group_was_not_exported(@group, @current_user, @shared.errors) + notification_service.group_was_not_exported(group, current_user, shared.errors) end def notification_service @@ -118,3 +126,5 @@ module Groups end end end + +Groups::ImportExport::ExportService.prepend_if_ee('EE::Groups::ImportExport::ExportService') diff --git a/app/services/groups/import_export/import_service.rb b/app/services/groups/import_export/import_service.rb index a0ddc50e5e0..bf3f09f22d4 100644 --- a/app/services/groups/import_export/import_service.rb +++ b/app/services/groups/import_export/import_service.rb @@ -3,7 +3,7 @@ module Groups module ImportExport class ImportService - attr_reader :current_user, :group, :params + attr_reader :current_user, :group, :shared def initialize(group:, user:) @group = group @@ -26,10 +26,10 @@ module Groups end def execute - if valid_user_permissions? && import_file && restorer.restore + if valid_user_permissions? && import_file && restorers.all?(&:restore) notify_success - @group + group else notify_error! end @@ -43,37 +43,41 @@ module Groups def import_file @import_file ||= Gitlab::ImportExport::FileImporter.import( - importable: @group, + importable: group, archive_file: nil, - shared: @shared + shared: shared ) end - def restorer - @restorer ||= + def restorers + [tree_restorer] + end + + def tree_restorer + @tree_restorer ||= if ndjson? Gitlab::ImportExport::Group::TreeRestorer.new( - user: @current_user, - shared: @shared, - group: @group + user: current_user, + shared: shared, + group: group ) else Gitlab::ImportExport::Group::LegacyTreeRestorer.new( - user: @current_user, - shared: @shared, - group: @group, + user: current_user, + shared: shared, + group: group, group_hash: nil ) end end def ndjson? - ::Feature.enabled?(:group_import_ndjson, @group&.parent, default_enabled: true) && - File.exist?(File.join(@shared.export_path, 'tree/groups/_all.ndjson')) + ::Feature.enabled?(:group_import_ndjson, group&.parent, default_enabled: true) && + File.exist?(File.join(shared.export_path, 'tree/groups/_all.ndjson')) end def remove_import_file - upload = @group.import_export_upload + upload = group.import_export_upload return unless upload&.import_file&.file @@ -85,7 +89,7 @@ module Groups if current_user.can?(:admin_group, group) true else - @shared.error(::Gitlab::ImportExport::Error.permission_error(current_user, group)) + shared.error(::Gitlab::ImportExport::Error.permission_error(current_user, group)) false end @@ -93,16 +97,16 @@ module Groups def notify_success @logger.info( - group_id: @group.id, - group_name: @group.name, + group_id: group.id, + group_name: group.name, message: 'Group Import/Export: Import succeeded' ) end def notify_error @logger.error( - group_id: @group.id, - group_name: @group.name, + group_id: group.id, + group_name: group.name, message: "Group Import/Export: Errors occurred, see '#{Gitlab::ErrorTracking::Logger.file_name}' for details" ) end @@ -110,12 +114,14 @@ module Groups def notify_error! notify_error - raise Gitlab::ImportExport::Error.new(@shared.errors.to_sentence) + raise Gitlab::ImportExport::Error.new(shared.errors.to_sentence) end def remove_base_tmp_dir - FileUtils.rm_rf(@shared.base_path) + FileUtils.rm_rf(shared.base_path) end end end end + +Groups::ImportExport::ImportService.prepend_if_ee('EE::Groups::ImportExport::ImportService') diff --git a/app/services/groups/open_issues_count_service.rb b/app/services/groups/open_issues_count_service.rb new file mode 100644 index 00000000000..db1ca09212a --- /dev/null +++ b/app/services/groups/open_issues_count_service.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +module Groups + # Service class for counting and caching the number of open issues of a group. + class OpenIssuesCountService < BaseCountService + include Gitlab::Utils::StrongMemoize + + VERSION = 1 + PUBLIC_COUNT_KEY = 'group_public_open_issues_count' + TOTAL_COUNT_KEY = 'group_total_open_issues_count' + CACHED_COUNT_THRESHOLD = 1000 + EXPIRATION_TIME = 24.hours + + attr_reader :group, :user + + def initialize(group, user = nil) + @group = group + @user = user + end + + # Reads count value from cache and return it if present. + # If empty or expired, #uncached_count will calculate the issues count for the group and + # compare it with the threshold. If it is greater, it will be written to the cache and returned. + # If below, it will be returned without being cached. + # This results in only caching large counts and calculating the rest with every call to maintain + # accuracy. + def count + cached_count = Rails.cache.read(cache_key) + return cached_count unless cached_count.blank? + + refreshed_count = uncached_count + update_cache_for_key(cache_key) { refreshed_count } if refreshed_count > CACHED_COUNT_THRESHOLD + refreshed_count + end + + def cache_key(key = nil) + ['groups', 'open_issues_count_service', VERSION, group.id, cache_key_name] + end + + private + + def cache_options + super.merge({ expires_in: EXPIRATION_TIME }) + end + + def cache_key_name + public_only? ? PUBLIC_COUNT_KEY : TOTAL_COUNT_KEY + end + + def public_only? + !user_is_at_least_reporter? + end + + def user_is_at_least_reporter? + strong_memoize(:user_is_at_least_reporter) do + group.member?(user, Gitlab::Access::REPORTER) + end + end + + def relation_for_count + IssuesFinder.new(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true, public_only: public_only?).execute + end + end +end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 6d41d449683..7c508237c8d 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -190,11 +190,7 @@ class IssuableBaseService < BaseService change_additional_attributes(issuable) old_associations = associations_before_update(issuable) - label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids) - if labels_changing?(issuable.label_ids, label_ids) - params[:label_ids] = label_ids - issuable.touch - end + assign_requested_labels(issuable) if issuable.changed? || params.present? issuable.assign_attributes(params) @@ -297,10 +293,6 @@ class IssuableBaseService < BaseService update_task(issuable) end - def labels_changing?(old_label_ids, new_label_ids) - old_label_ids.sort != new_label_ids.sort - end - def has_title_or_description_changed?(issuable) issuable.title_changed? || issuable.description_changed? end @@ -349,6 +341,20 @@ class IssuableBaseService < BaseService end # rubocop: enable CodeReuse/ActiveRecord + def assign_requested_labels(issuable) + label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids) + return unless ids_changing?(issuable.label_ids, label_ids) + + params[:label_ids] = label_ids + issuable.touch + end + + # Arrays of ids are used, but we should really use sets of ids, so + # let's have an helper to properly check if some ids are changing + def ids_changing?(old_array, new_array) + old_array.sort != new_array.sort + end + def toggle_award(issuable) award = params.delete(:emoji_award) AwardEmojis::ToggleService.new(issuable, award, current_user).execute if award diff --git a/app/services/issue_rebalancing_service.rb b/app/services/issue_rebalancing_service.rb index 4138c6441c8..849afc4edb8 100644 --- a/app/services/issue_rebalancing_service.rb +++ b/app/services/issue_rebalancing_service.rb @@ -17,8 +17,21 @@ class IssueRebalancingService start = RelativePositioning::START_POSITION - (gaps / 2) * gap_size - Issue.transaction do - indexed_ids.each_slice(100) { |pairs| assign_positions(start, pairs) } + if Feature.enabled?(:issue_rebalancing_optimization) + Issue.transaction do + assign_positions(start, indexed_ids) + .sort_by(&:first) + .each_slice(100) do |pairs_with_position| + update_positions(pairs_with_position, 'rebalance issue positions in batches ordered by id') + end + end + else + Issue.transaction do + indexed_ids.each_slice(100) do |pairs| + pairs_with_position = assign_positions(start, pairs) + update_positions(pairs_with_position, 'rebalance issue positions') + end + end end end @@ -32,13 +45,22 @@ class IssueRebalancingService end # rubocop: enable CodeReuse/ActiveRecord - # rubocop: disable CodeReuse/ActiveRecord - def assign_positions(start, positions) - values = positions.map do |id, index| - "(#{id}, #{start + (index * gap_size)})" + def assign_positions(start, pairs) + pairs.map do |id, index| + [id, start + (index * gap_size)] + end + end + + def update_positions(pairs_with_position, query_name) + values = pairs_with_position.map do |id, index| + "(#{id}, #{index})" end.join(', ') - Issue.connection.exec_query(<<~SQL, "rebalance issue positions") + run_update_query(values, query_name) + end + + def run_update_query(values, query_name) + Issue.connection.exec_query(<<~SQL, query_name) WITH cte(cte_id, new_pos) AS ( SELECT * FROM (VALUES #{values}) as t (id, pos) @@ -49,7 +71,6 @@ class IssueRebalancingService WHERE cte_id = id SQL end - # rubocop: enable CodeReuse/ActiveRecord def issue_count @issue_count ||= base.count diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 44de8eb6389..d2285a375a1 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -2,20 +2,26 @@ module Issues class CreateService < Issues::BaseService - include SpamCheckMethods include ResolveDiscussions def execute(skip_system_notes: false) + @request = params.delete(:request) + @spam_params = Spam::SpamActionService.filter_spam_params!(params) + @issue = BuildService.new(project, current_user, params).execute - filter_spam_check_params filter_resolve_discussion_params create(@issue, skip_system_notes: skip_system_notes) end def before_create(issue) - spam_check(issue, current_user, action: :create) + Spam::SpamActionService.new( + spammable: issue, + request: request, + user: current_user, + action: :create + ).execute(spam_params: spam_params) # current_user (defined in BaseService) is not available within run_after_commit block user = current_user @@ -46,8 +52,10 @@ module Issues private + attr_reader :request, :spam_params + def user_agent_detail_service - UserAgentDetailService.new(@issue, @request) + UserAgentDetailService.new(@issue, request) end # Applies label "incident" (creates it if missing) to incident issues. diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 127ed04cf51..2906bdf62a7 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -2,12 +2,14 @@ module Issues class UpdateService < Issues::BaseService - include SpamCheckMethods extend ::Gitlab::Utils::Override def execute(issue) handle_move_between_ids(issue) - filter_spam_check_params + + @request = params.delete(:request) + @spam_params = Spam::SpamActionService.filter_spam_params!(params) + change_issue_duplicate(issue) move_issue_to_new_project(issue) || clone_issue(issue) || update_task_event(issue) || update(issue) end @@ -30,7 +32,14 @@ module Issues end def before_update(issue, skip_spam_check: false) - spam_check(issue, current_user, action: :update) unless skip_spam_check + return if skip_spam_check + + Spam::SpamActionService.new( + spammable: issue, + request: request, + user: current_user, + action: :update + ).execute(spam_params: spam_params) end def after_update(issue) @@ -126,6 +135,8 @@ module Issues private + attr_reader :request, :spam_params + def clone_issue(issue) target_project = params.delete(:target_clone_project) with_notes = params.delete(:clone_with_notes) diff --git a/app/services/jira/requests/base.rb b/app/services/jira/requests/base.rb index 098aae9284c..bae8298d5c8 100644 --- a/app/services/jira/requests/base.rb +++ b/app/services/jira/requests/base.rb @@ -18,15 +18,15 @@ module Jira request end + private + + attr_reader :jira_service, :project + # We have to add the context_path here because the Jira client is not taking it into account def base_api_url "#{context_path}/rest/api/#{api_version}" end - private - - attr_reader :jira_service, :project - def context_path client.options[:context_path].to_s end diff --git a/app/services/members/update_service.rb b/app/services/members/update_service.rb index b5c27caafa2..5c6e51201c2 100644 --- a/app/services/members/update_service.rb +++ b/app/services/members/update_service.rb @@ -16,7 +16,11 @@ module Members enqueue_delete_todos(member) if downgrading_to_guest? end - member + if member.errors.any? + error(member.errors.full_messages.to_sentence, pass_back: { member: member }) + else + success(member: member) + end end private diff --git a/app/services/merge_requests/add_context_service.rb b/app/services/merge_requests/add_context_service.rb index bb82fa23468..b693f8509a2 100644 --- a/app/services/merge_requests/add_context_service.rb +++ b/app/services/merge_requests/add_context_service.rb @@ -66,7 +66,8 @@ module MergeRequests relative_order: index, sha: sha, authored_date: Gitlab::Database.sanitize_timestamp(commit_hash[:authored_date]), - committed_date: Gitlab::Database.sanitize_timestamp(commit_hash[:committed_date]) + committed_date: Gitlab::Database.sanitize_timestamp(commit_hash[:committed_date]), + trailers: commit_hash.fetch(:trailers, {}).to_json ) end end diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 80991657688..12c901aa1a1 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -16,30 +16,17 @@ module MergeRequests merge_request.source_project = find_source_project merge_request.target_project = find_target_project - # Source project sets the default source branch removal setting - merge_request.merge_params['force_remove_source_branch'] = - if params.key?(:force_remove_source_branch) - params.delete(:force_remove_source_branch) - else - merge_request.source_project.remove_source_branch_after_merge? - end + # Force remove the source branch? + merge_request.merge_params['force_remove_source_branch'] = force_remove_source_branch + # Only assign merge requests params that are allowed self.params = assign_allowed_merge_params(merge_request, params) + # Filter out params that are either not allowed or invalid filter_params(merge_request) - # merge_request.assign_attributes(...) below is a Rails - # method that only work if all the params it is passed have - # corresponding fields in the database. As there are no fields - # in the database for :add_label_ids and :remove_label_ids, we - # need to remove them from the params before the call to - # merge_request.assign_attributes(...) - # - # IssuableBaseService#process_label_ids takes care - # of the removal. - params[:label_ids] = process_label_ids(params, extra_label_ids: merge_request.label_ids.to_a) - - merge_request.assign_attributes(params.to_h.compact) + # Filter out :add_label_ids and :remove_label_ids params + filter_label_id_params merge_request.compare_commits = [] set_merge_request_target_branch @@ -74,6 +61,29 @@ module MergeRequests :errors, to: :merge_request + def force_remove_source_branch + if params.key?(:force_remove_source_branch) + params.delete(:force_remove_source_branch) + else + merge_request.source_project.remove_source_branch_after_merge? + end + end + + def filter_label_id_params + # merge_request.assign_attributes(...) below is a Rails + # method that only work if all the params it is passed have + # corresponding fields in the database. As there are no fields + # in the database for :add_label_ids and :remove_label_ids, we + # need to remove them from the params before the call to + # merge_request.assign_attributes(...) + # + # IssuableBaseService#process_label_ids takes care + # of the removal. + params[:label_ids] = process_label_ids(params, extra_label_ids: merge_request.label_ids.to_a) + + merge_request.assign_attributes(params.to_h.compact) + end + def find_source_project source_project = project_from_params(:source_project) return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project) diff --git a/app/services/merge_requests/mark_reviewer_reviewed_service.rb b/app/services/merge_requests/mark_reviewer_reviewed_service.rb new file mode 100644 index 00000000000..766a4ca0a49 --- /dev/null +++ b/app/services/merge_requests/mark_reviewer_reviewed_service.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module MergeRequests + class MarkReviewerReviewedService < MergeRequests::BaseService + def execute(merge_request) + return error("Invalid permissions") unless can?(current_user, :update_merge_request, merge_request) + + reviewer = merge_request.find_reviewer(current_user) + + if reviewer + return error("Failed to update reviewer") unless reviewer.update(state: :reviewed) + + success + else + error("Reviewer not found") + end + end + end +end diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb index 96a2322f6a0..9fecab85cc1 100644 --- a/app/services/merge_requests/mergeability_check_service.rb +++ b/app/services/merge_requests/mergeability_check_service.rb @@ -114,6 +114,7 @@ module MergeRequests merge_to_ref_success = merge_to_ref + reload_merge_head_diff update_diff_discussion_positions! if merge_to_ref_success if merge_to_ref_success && can_git_merge? @@ -123,6 +124,10 @@ module MergeRequests end end + def reload_merge_head_diff + MergeRequests::ReloadMergeHeadDiffService.new(merge_request).execute + end + def update_diff_discussion_positions! Discussions::CaptureDiffNotePositionsService.new(merge_request).execute end @@ -153,6 +158,7 @@ module MergeRequests def merge_to_ref params = { allow_conflicts: Feature.enabled?(:display_merge_conflicts_in_diff, project) } result = MergeRequests::MergeToRefService.new(project, merge_request.author, params).execute(merge_request) + result[:status] == :success end diff --git a/app/services/merge_requests/reload_merge_head_diff_service.rb b/app/services/merge_requests/reload_merge_head_diff_service.rb new file mode 100644 index 00000000000..66fcb5c022b --- /dev/null +++ b/app/services/merge_requests/reload_merge_head_diff_service.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module MergeRequests + class ReloadMergeHeadDiffService + include BaseServiceUtility + + def initialize(merge_request) + @merge_request = merge_request + end + + def execute + return error("default_merge_ref_for_diffs feature flag is disabled") unless enabled? + return error("Merge request has no merge ref head.") unless merge_request.merge_ref_head.present? + + error_msg = recreate_merge_head_diff + + return error(error_msg) if error_msg + + success + end + + private + + attr_reader :merge_request + + def enabled? + Feature.enabled?(:default_merge_ref_for_diffs, merge_request.project) + end + + def recreate_merge_head_diff + merge_request.merge_head_diff&.destroy! + + # n+1: https://gitlab.com/gitlab-org/gitlab/-/issues/19377 + Gitlab::GitalyClient.allow_n_plus_1_calls do + merge_request.create_merge_head_diff! + end + + # Reset the merge request so it won't load the merge head diff as the + # MergeRequest#merge_request_diff. + merge_request.reset + + nil + rescue StandardError => e + message = "Failed to recreate merge head diff: #{e.message}" + + Gitlab::AppLogger.error(message: message, merge_request_id: merge_request.id) + message + end + end +end diff --git a/app/services/merge_requests/request_review_service.rb b/app/services/merge_requests/request_review_service.rb new file mode 100644 index 00000000000..b061ed45fee --- /dev/null +++ b/app/services/merge_requests/request_review_service.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module MergeRequests + class RequestReviewService < MergeRequests::BaseService + def execute(merge_request, user) + return error("Invalid permissions") unless can?(current_user, :update_merge_request, merge_request) + + reviewer = merge_request.find_reviewer(user) + + if reviewer + return error("Failed to update reviewer") unless reviewer.update(state: :unreviewed) + + notify_reviewer(merge_request, user) + + success + else + error("Reviewer not found") + end + end + + private + + def notify_reviewer(merge_request, reviewer) + notification_service.async.review_requested_of_merge_request(merge_request, current_user, reviewer) + todo_service.create_request_review_todo(merge_request, current_user, reviewer) + end + end +end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index d2e5a2a1619..45f81d972db 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -109,6 +109,9 @@ module MergeRequests create_assignee_note(merge_request, old_assignees) notification_service.async.reassigned_merge_request(merge_request, current_user, old_assignees) todo_service.reassigned_assignable(merge_request, current_user, old_assignees) + + new_assignees = merge_request.assignees - old_assignees + merge_request_activity_counter.track_users_assigned_to_mr(users: new_assignees) end def handle_reviewers_change(merge_request, old_reviewers) @@ -117,6 +120,9 @@ module MergeRequests notification_service.async.changed_reviewer_of_merge_request(merge_request, current_user, old_reviewers) todo_service.reassigned_reviewable(merge_request, current_user, old_reviewers) invalidate_cache_counts(merge_request, users: affected_reviewers.compact) + + new_reviewers = merge_request.reviewers - old_reviewers + merge_request_activity_counter.track_users_review_requested(users: new_reviewers) end def create_branch_change_note(issuable, branch_type, old_branch, new_branch) diff --git a/app/services/namespaces/in_product_marketing_emails_service.rb b/app/services/namespaces/in_product_marketing_emails_service.rb new file mode 100644 index 00000000000..45b4619ddbe --- /dev/null +++ b/app/services/namespaces/in_product_marketing_emails_service.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +module Namespaces + class InProductMarketingEmailsService + include Gitlab::Experimentation::GroupTypes + + INTERVAL_DAYS = [1, 5, 10].freeze + TRACKS = { + create: :git_write, + verify: :pipeline_created, + trial: :trial_started, + team: :user_added + }.freeze + + def self.send_for_all_tracks_and_intervals + TRACKS.each_key do |track| + INTERVAL_DAYS.each do |interval| + new(track, interval).execute + end + end + end + + def initialize(track, interval) + @track = track + @interval = interval + @sent_email_user_ids = [] + end + + def execute + groups_for_track.each_batch do |groups| + groups.each do |group| + send_email_for_group(group) + end + end + end + + private + + attr_reader :track, :interval, :sent_email_user_ids + + def send_email_for_group(group) + experiment_enabled_for_group = experiment_enabled_for_group?(group) + experiment_add_group(group, experiment_enabled_for_group) + return unless experiment_enabled_for_group + + users_for_group(group).each do |user| + send_email(user, group) if can_perform_action?(user, group) + end + 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 + + # rubocop: disable CodeReuse/ActiveRecord + def groups_for_track + onboarding_progress_scope = OnboardingProgress + .completed_actions_with_latest_in_range(completed_actions, range) + .incomplete_actions(incomplete_action) + + Group.joins(:onboarding_progress).merge(onboarding_progress_scope) + end + + def users_for_group(group) + group.users.where(email_opted_in: true) + .where.not(id: sent_email_user_ids) + end + # rubocop: enable CodeReuse/ActiveRecord + + def can_perform_action?(user, group) + case track + when :create + user.can?(:create_projects, group) + when :verify + user.can?(:create_projects, group) + when :trial + user.can?(:start_trial, group) + when :team + user.can?(:admin_group_member, group) + else + raise NotImplementedError, "No ability defined for track #{track}" + end + end + + def send_email(user, group) + NotificationService.new.in_product_marketing(user.id, group.id, track, series) + sent_email_user_ids << user.id + end + + def completed_actions + index = TRACKS.keys.index(track) + index == 0 ? [:created] : TRACKS.values[0..index - 1] + end + + def range + (interval + 1).days.ago.beginning_of_day..(interval + 1).days.ago.end_of_day + end + + def incomplete_action + TRACKS[track] + end + + def series + INTERVAL_DAYS.index(interval) + end + end +end diff --git a/app/services/notification_recipients/build_service.rb b/app/services/notification_recipients/build_service.rb index 040ecc29d3a..52070abbad7 100644 --- a/app/services/notification_recipients/build_service.rb +++ b/app/services/notification_recipients/build_service.rb @@ -36,5 +36,9 @@ module NotificationRecipients def self.build_new_review_recipients(*args) ::NotificationRecipients::Builder::NewReview.new(*args).notification_recipients end + + def self.build_requested_review_recipients(*args) + ::NotificationRecipients::Builder::RequestReview.new(*args).notification_recipients + end end end diff --git a/app/services/notification_recipients/builder/request_review.rb b/app/services/notification_recipients/builder/request_review.rb new file mode 100644 index 00000000000..911d89c6a8e --- /dev/null +++ b/app/services/notification_recipients/builder/request_review.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module NotificationRecipients + module Builder + class RequestReview < Base + attr_reader :merge_request, :current_user, :reviewer + + def initialize(merge_request, current_user, reviewer) + @merge_request, @current_user, @reviewer = merge_request, current_user, reviewer + end + + def target + merge_request + end + + def build! + add_recipients(reviewer, :mention, NotificationReason::REVIEW_REQUESTED) + end + end + end +end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 5a71e0eac7c..50247532f69 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -265,6 +265,14 @@ class NotificationService end end + def review_requested_of_merge_request(merge_request, current_user, reviewer) + recipients = NotificationRecipients::BuildService.build_requested_review_recipients(merge_request, current_user, reviewer) + + recipients.each do |recipient| + mailer.request_review_merge_request_email(recipient.user.id, merge_request.id, current_user.id, recipient.reason).deliver_later + end + end + # When we add labels to a merge request we should send an email to: # # * watchers of the mr's labels @@ -664,6 +672,10 @@ class NotificationService end end + def in_product_marketing(user_id, group_id, track, series) + mailer.in_product_marketing_email(user_id, group_id, track, series).deliver_later + end + protected def new_resource_email(target, method) diff --git a/app/services/packages/debian/destroy_distribution_service.rb b/app/services/packages/debian/destroy_distribution_service.rb new file mode 100644 index 00000000000..bef1127fece --- /dev/null +++ b/app/services/packages/debian/destroy_distribution_service.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Packages + module Debian + class DestroyDistributionService + def initialize(distribution) + @distribution = distribution + end + + def execute + destroy_distribution + end + + private + + def destroy_distribution + if @distribution.destroy + success + else + error("Unable to destroy Debian #{@distribution.model_name.human.downcase}") + end + end + + def success + ServiceResponse.success + end + + def error(message) + ServiceResponse.error(message: message, payload: { distribution: @distribution }) + end + end + end +end diff --git a/app/services/packages/debian/update_distribution_service.rb b/app/services/packages/debian/update_distribution_service.rb new file mode 100644 index 00000000000..5bb59b854e9 --- /dev/null +++ b/app/services/packages/debian/update_distribution_service.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +module Packages + module Debian + class UpdateDistributionService + def initialize(distribution, params) + @distribution, @params = distribution, params + + @components = params.delete(:components) + + @architectures = params.delete(:architectures) + @architectures += ['all'] unless @architectures.nil? + + @errors = [] + end + + def execute + update_distribution + end + + private + + attr_reader :distribution, :params, :components, :architectures, :errors + + def append_errors(record, prefix = '') + return if record.valid? + + prefix = "#{prefix} " unless prefix.empty? + @errors += record.errors.full_messages.map { |message| "#{prefix}#{message}" } + end + + def update_distribution + distribution.transaction do + if distribution.update(params) + update_components if components + update_architectures if architectures + + success + else + append_errors(distribution) + error + end + end || error + end + + def update_components + update_objects(distribution.components, components, error_label: 'Component') + end + + def update_architectures + update_objects(distribution.architectures, architectures, error_label: 'Architecture') + end + + def update_objects(objects, object_names_from_params, error_label: ) + current_object_names = objects.map(&:name) + missing_object_names = object_names_from_params - current_object_names + extra_object_names = current_object_names - object_names_from_params + + missing_object_names.each do |name| + new_object = objects.create(name: name) + append_errors(new_object, error_label) + raise ActiveRecord::Rollback unless new_object.persisted? + end + + extra_object_names.each do |name| + object = objects.with_name(name).first + raise ActiveRecord::Rollback unless object.destroy + end + end + + def success + ServiceResponse.success(payload: { distribution: distribution }) + end + + def error + ServiceResponse.error(message: errors.to_sentence, payload: { distribution: distribution }) + end + end + end +end diff --git a/app/services/packages/maven/find_or_create_package_service.rb b/app/services/packages/maven/find_or_create_package_service.rb index 8ee449cbfdc..6e0346058e8 100644 --- a/app/services/packages/maven/find_or_create_package_service.rb +++ b/app/services/packages/maven/find_or_create_package_service.rb @@ -11,12 +11,7 @@ module Packages .execute unless Namespace::PackageSetting.duplicates_allowed?(package) - files = package&.package_files || [] - current_maven_files = files.map { |file| extname(file.file_name) } - - if current_maven_files.compact.include?(extname(params[:file_name])) - return ServiceResponse.error(message: 'Duplicate package is not allowed') - end + return ServiceResponse.error(message: 'Duplicate package is not allowed') if target_package_is_duplicate?(package) end unless package @@ -67,6 +62,17 @@ module Packages File.extname(filename) end + + def target_package_is_duplicate?(package) + # duplicate metadata files can be uploaded multiple times + return false if package.version.nil? + + package + .package_files + .map { |file| extname(file.file_name) } + .compact + .include?(extname(params[:file_name])) + end end end end diff --git a/app/services/pages/migrate_from_legacy_storage_service.rb b/app/services/pages/migrate_from_legacy_storage_service.rb new file mode 100644 index 00000000000..d805ae2418c --- /dev/null +++ b/app/services/pages/migrate_from_legacy_storage_service.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +module Pages + class MigrateFromLegacyStorageService + def initialize(logger, migration_threads, batch_size) + @logger = logger + @migration_threads = migration_threads + @batch_size = batch_size + + @migrated = 0 + @errored = 0 + @counters_lock = Mutex.new + end + + def execute + @queue = SizedQueue.new(1) + + threads = start_migration_threads + + ProjectPagesMetadatum.only_on_legacy_storage.each_batch(of: @batch_size) do |batch| + @queue.push(batch) + end + + @queue.close + + @logger.info("Waiting for threads to finish...") + threads.each(&:join) + + { migrated: @migrated, errored: @errored } + end + + def start_migration_threads + Array.new(@migration_threads) do + Thread.new do + while batch = @queue.pop + Rails.application.executor.wrap do + process_batch(batch) + end + end + end + end + end + + def process_batch(batch) + batch.with_project_route_and_deployment.each do |metadatum| + project = metadatum.project + + migrate_project(project) + end + + @logger.info("#{@migrated} projects are migrated successfully, #{@errored} projects failed to be migrated") + rescue => e + # This method should never raise exception otherwise all threads might be killed + # and this will result in queue starving (and deadlock) + Gitlab::ErrorTracking.track_exception(e) + @logger.error("failed processing a batch: #{e.message}") + end + + def migrate_project(project) + result = nil + time = Benchmark.realtime do + result = ::Pages::MigrateLegacyStorageToDeploymentService.new(project).execute + end + + if result[:status] == :success + @logger.info("project_id: #{project.id} #{project.pages_path} has been migrated in #{time} seconds") + @counters_lock.synchronize { @migrated += 1 } + else + @logger.error("project_id: #{project.id} #{project.pages_path} failed to be migrated in #{time} seconds: #{result[:message]}") + @counters_lock.synchronize { @errored += 1 } + end + rescue => e + @counters_lock.synchronize { @errored += 1 } + @logger.error("#{e.message} project_id: #{project&.id}") + Gitlab::ErrorTracking.track_exception(e, project_id: project&.id) + end + end +end diff --git a/app/services/projects/alerting/notify_service.rb b/app/services/projects/alerting/notify_service.rb index 014fb0e3ed3..2ba64b73699 100644 --- a/app/services/projects/alerting/notify_service.rb +++ b/app/services/projects/alerting/notify_service.rb @@ -3,9 +3,8 @@ module Projects module Alerting class NotifyService - include BaseServiceUtility - include Gitlab::Utils::StrongMemoize - include ::IncidentManagement::Settings + extend ::Gitlab::Utils::Override + include ::AlertManagement::AlertProcessing def initialize(project, payload) @project = project @@ -22,8 +21,7 @@ module Projects process_alert return bad_request unless alert.persisted? - process_incident_issues if process_issues? - send_alert_email if send_email? + complete_post_processing_tasks ServiceResponse.success end @@ -32,93 +30,15 @@ module Projects attr_reader :project, :payload, :integration - def process_alert - if alert.persisted? - process_existing_alert - else - create_alert - end - end - - def process_existing_alert - if incoming_payload.ends_at.present? - process_resolved_alert - else - alert.register_new_event! - end - - alert - end - - def process_resolved_alert - return unless auto_close_incident? - - if alert.resolve(incoming_payload.ends_at) - close_issue(alert.issue) - end - - alert - end - - def close_issue(issue) - return if issue.blank? || issue.closed? - - ::Issues::CloseService - .new(project, User.alert_bot) - .execute(issue, system_note: false) - - SystemNoteService.auto_resolve_prometheus_alert(issue, project, User.alert_bot) if issue.reset.closed? - end - - def create_alert - return unless alert.save - - alert.execute_services - SystemNoteService.create_new_alert(alert, notification_source) - end - - def process_incident_issues - return if alert.issue || alert.resolved? - - ::IncidentManagement::ProcessAlertWorker.perform_async(nil, nil, alert.id) - end - - def send_alert_email - notification_service - .async - .prometheus_alerts_fired(project, [alert]) - end - - def alert - strong_memoize(:alert) do - existing_alert || new_alert - end - end - - def existing_alert - return unless incoming_payload.gitlab_fingerprint - - AlertManagement::Alert.not_resolved.for_fingerprint(project, incoming_payload.gitlab_fingerprint).first - end - - def new_alert - AlertManagement::Alert.new(**incoming_payload.alert_params, ended_at: nil) - end - - def incoming_payload - strong_memoize(:incoming_payload) do - Gitlab::AlertManagement::Payload.parse(project, payload.to_h) - end + def valid_payload_size? + Gitlab::Utils::DeepSize.new(payload).valid? end - def notification_source + override :alert_source + def alert_source alert.monitoring_tool || integration&.name || 'Generic Alert Endpoint' end - def valid_payload_size? - Gitlab::Utils::DeepSize.new(payload).valid? - end - def active_integration? integration&.active? end diff --git a/app/services/projects/cleanup_service.rb b/app/services/projects/cleanup_service.rb index 6e3b320afbe..7bcaee75813 100644 --- a/app/services/projects/cleanup_service.rb +++ b/app/services/projects/cleanup_service.rb @@ -40,7 +40,7 @@ module Projects apply_bfg_object_map! # Remove older objects that are no longer referenced - GitGarbageCollectWorker.new.perform(project.id, :prune, "project_cleanup:gc:#{project.id}") + Projects::GitGarbageCollectWorker.new.perform(project.id, :prune, "project_cleanup:gc:#{project.id}") # The cache may now be inaccurate, and holding onto it could prevent # bugs assuming the presence of some object from manifesting for some diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index a01db4b498c..08f569662a8 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -127,7 +127,7 @@ module Projects access_level: group_access_level) end - if Feature.enabled?(:specialized_project_authorization_workers) + if Feature.enabled?(:specialized_project_authorization_workers, default_enabled: :yaml) AuthorizedProjectUpdate::ProjectCreateWorker.perform_async(@project.id) # AuthorizedProjectsWorker uses an exclusive lease per user but # specialized workers might have synchronization issues. Until we @@ -210,16 +210,22 @@ module Projects end def set_project_name_from_path - # Set project name from path - if @project.name.present? && @project.path.present? - # if both name and path set - everything is ok - elsif @project.path.present? + # if both name and path set - everything is ok + return if @project.name.present? && @project.path.present? + + if @project.path.present? # Set project name from path @project.name = @project.path.dup elsif @project.name.present? # For compatibility - set path from name - # TODO: remove this in 8.0 - @project.path = @project.name.dup.parameterize + @project.path = @project.name.dup + + # TODO: Retained for backwards compatibility. Remove in API v5. + # When removed, validation errors will get bubbled up automatically. + # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52725 + unless @project.path.match?(Gitlab::PathRegex.project_path_format_regex) + @project.path = @project.path.parameterize + end end end diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb index 050bfdd862d..fd9b64a4ee0 100644 --- a/app/services/projects/fork_service.rb +++ b/app/services/projects/fork_service.rb @@ -43,8 +43,8 @@ module Projects def new_fork_params new_params = { forked_from_project: @project, - visibility_level: allowed_visibility_level, - description: @project.description, + visibility_level: target_visibility_level, + description: target_description, name: target_name, path: target_path, shared_runners_enabled: @project.shared_runners_enabled, @@ -107,6 +107,10 @@ module Projects @target_name ||= @params[:name] || @project.name end + def target_description + @target_description ||= @params[:description] || @project.description + end + def target_namespace @target_namespace ||= @params[:namespace] || current_user.namespace end @@ -115,8 +119,9 @@ module Projects @skip_disk_validation ||= @params[:skip_disk_validation] || false end - def allowed_visibility_level + def target_visibility_level target_level = [@project.visibility_level, target_namespace.visibility_level].min + target_level = [target_level, Gitlab::VisibilityLevel.level_value(params[:visibility])].min if params.key?(:visibility) Gitlab::VisibilityLevel.closest_allowed_level(target_level) end diff --git a/app/services/projects/import_export/export_service.rb b/app/services/projects/import_export/export_service.rb index 031b99753c3..c2a8db7b657 100644 --- a/app/services/projects/import_export/export_service.rb +++ b/app/services/projects/import_export/export_service.rb @@ -86,11 +86,11 @@ module Projects end def repo_saver - Gitlab::ImportExport::RepoSaver.new(project: project, shared: shared) + Gitlab::ImportExport::RepoSaver.new(exportable: project, shared: shared) end def wiki_repo_saver - Gitlab::ImportExport::WikiRepoSaver.new(project: project, shared: shared) + Gitlab::ImportExport::WikiRepoSaver.new(exportable: project, shared: shared) end def lfs_saver @@ -102,7 +102,7 @@ module Projects end def design_repo_saver - Gitlab::ImportExport::DesignRepoSaver.new(project: project, shared: shared) + Gitlab::ImportExport::DesignRepoSaver.new(exportable: project, shared: shared) end def cleanup diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 25d46ada885..29e92d725e2 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -80,6 +80,10 @@ module Projects end def deploy_to_legacy_storage(artifacts_path) + # path today used by one project can later be used by another + # so we can't really scope this feature flag by project or group + return unless Feature.enabled?(:pages_update_legacy_storage, default_enabled: true) + # Create temporary directory in which we will extract the artifacts make_secure_tmp_dir(tmp_path) do |tmp_path| extract_archive!(artifacts_path, tmp_path) diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index 50a544ed1a5..8384bfa813f 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -56,11 +56,25 @@ module Projects raise ValidationError.new(s_('UpdateProject|Cannot rename project because it contains container registry tags!')) end - if changing_default_branch? - raise ValidationError.new(s_("UpdateProject|Could not set the default branch")) unless project.change_head(params[:default_branch]) + validate_default_branch_change + end + + def validate_default_branch_change + return unless changing_default_branch? + + previous_default_branch = project.default_branch + + if project.change_head(params[:default_branch]) + after_default_branch_change(previous_default_branch) + else + raise ValidationError.new(s_("UpdateProject|Could not set the default branch")) end end + def after_default_branch_change(previous_default_branch) + # overridden by EE module + end + def remove_unallowed_params params.delete(:emails_disabled) unless can?(current_user, :set_emails_disabled, project) end diff --git a/app/services/quick_actions/interpret_service.rb b/app/services/quick_actions/interpret_service.rb index de1cd7cd981..ea90d8e3dd8 100644 --- a/app/services/quick_actions/interpret_service.rb +++ b/app/services/quick_actions/interpret_service.rb @@ -164,6 +164,7 @@ module QuickActions next unless definition definition.execute(self, arg) + usage_ping_tracking(name, arg) end end @@ -178,6 +179,14 @@ module QuickActions ext.references(type) end # rubocop: enable CodeReuse/ActiveRecord + + def usage_ping_tracking(quick_action_name, arg) + Gitlab::UsageDataCounters::QuickActionActivityUniqueCounter.track_unique_action( + quick_action_name, + args: arg&.strip, + user: current_user + ) + end end end diff --git a/app/services/repositories/changelog_service.rb b/app/services/repositories/changelog_service.rb new file mode 100644 index 00000000000..f30b64b9b32 --- /dev/null +++ b/app/services/repositories/changelog_service.rb @@ -0,0 +1,99 @@ +# frozen_string_literal: true + +module Repositories + # A service class for generating a changelog section. + class ChangelogService + DEFAULT_TRAILER = 'Changelog' + DEFAULT_FILE = 'CHANGELOG.md' + + # The `project` specifies the `Project` to generate the changelog section + # for. + # + # The `user` argument specifies a `User` to use for committing the changes + # to the Git repository. + # + # The `version` arguments must be a version `String` using semantic + # versioning as the format. + # + # The arguments `from` and `to` must specify a Git ref or SHA to use for + # fetching the commits to include in the changelog. The SHA/ref set in the + # `from` argument isn't included in the list. + # + # The `date` argument specifies the date of the release, and defaults to the + # current time/date. + # + # The `branch` argument specifies the branch to commit the changes to. The + # branch must already exist. + # + # The `trailer` argument is the Git trailer to use for determining what + # commits to include in the changelog. + # + # The `file` arguments specifies the name/path of the file to commit the + # changes to. If the file doesn't exist, it's created automatically. + # + # The `message` argument specifies the commit message to use when committing + # the changelog changes. + # + # rubocop: disable Metrics/ParameterLists + def initialize( + project, + user, + version:, + from:, + to:, + date: DateTime.now, + branch: project.default_branch_or_master, + trailer: DEFAULT_TRAILER, + file: DEFAULT_FILE, + message: "Add changelog for version #{version}" + ) + @project = project + @user = user + @version = version + @from = from + @to = to + @date = date + @branch = branch + @trailer = trailer + @file = file + @message = message + end + # rubocop: enable Metrics/ParameterLists + + def execute + # For every entry we want to only include the merge request that + # originally introduced the commit, which is the oldest merge request that + # contains the commit. We fetch there merge requests in batches, reducing + # the number of SQL queries needed to get this data. + mrs_finder = MergeRequests::OldestPerCommitFinder.new(@project) + config = Gitlab::Changelog::Config.from_git(@project) + release = Gitlab::Changelog::Release + .new(version: @version, date: @date, config: config) + + commits = + CommitsWithTrailerFinder.new(project: @project, from: @from, to: @to) + + commits.each_page(@trailer) do |page| + mrs = mrs_finder.execute(page) + + # Preload the authors. This ensures we only need a single SQL query per + # batch of commits, instead of needing a query for every commit. + page.each(&:lazy_author) + + page.each do |commit| + release.add_entry( + title: commit.title, + commit: commit, + category: commit.trailers.fetch(@trailer), + author: commit.author, + merge_request: mrs[commit.id] + ) + end + end + + Gitlab::Changelog::Committer + .new(@project, @user) + .commit(release: release, file: @file, branch: @branch, message: @message) + end + end +end diff --git a/app/services/repositories/housekeeping_service.rb b/app/services/repositories/housekeeping_service.rb index 6a2fa95d25f..de80390e60b 100644 --- a/app/services/repositories/housekeeping_service.rb +++ b/app/services/repositories/housekeeping_service.rb @@ -45,7 +45,7 @@ module Repositories private def execute_gitlab_shell_gc(lease_uuid) - GitGarbageCollectWorker.perform_async(@resource.id, task, lease_key, lease_uuid) + @resource.git_garbage_collect_worker_klass.perform_async(@resource.id, task, lease_key, lease_uuid) ensure if pushes_since_gc >= gc_period Gitlab::Metrics.measure(:reset_pushes_since_gc) do diff --git a/app/services/resource_access_tokens/create_service.rb b/app/services/resource_access_tokens/create_service.rb index 70e09be9407..36858f33b49 100644 --- a/app/services/resource_access_tokens/create_service.rb +++ b/app/services/resource_access_tokens/create_service.rb @@ -10,7 +10,7 @@ module ResourceAccessTokens end def execute - return error("User does not have permission to create #{resource_type} Access Token") unless has_permission_to_create? + return error("User does not have permission to create #{resource_type} access token") unless has_permission_to_create? user = create_user @@ -26,6 +26,7 @@ module ResourceAccessTokens token_response = create_personal_access_token(user) if token_response.success? + log_event(token_response.payload[:personal_access_token]) success(token_response.payload[:personal_access_token]) else delete_failed_user(user) @@ -105,6 +106,10 @@ module ResourceAccessTokens resource.add_user(user, :maintainer, expires_at: params[:expires_at]) end + def log_event(token) + ::Gitlab::AppLogger.info "PROJECT ACCESS TOKEN CREATION: created_by: #{current_user.username}, project_id: #{resource.id}, token_user: #{token.user.name}, token_id: #{token.id}" + end + def error(message) ServiceResponse.error(message: message) end @@ -114,3 +119,5 @@ module ResourceAccessTokens end end end + +ResourceAccessTokens::CreateService.prepend_if_ee('EE::ResourceAccessTokens::CreateService') diff --git a/app/services/resource_access_tokens/revoke_service.rb b/app/services/resource_access_tokens/revoke_service.rb index ece928dac31..59402701ddc 100644 --- a/app/services/resource_access_tokens/revoke_service.rb +++ b/app/services/resource_access_tokens/revoke_service.rb @@ -21,6 +21,8 @@ module ResourceAccessTokens destroy_bot_user + log_event + success("Access token #{access_token.name} has been revoked and the bot user has been scheduled for deletion.") rescue StandardError => error log_error("Failed to revoke access token for #{bot_user.name}: #{error.message}") @@ -57,6 +59,10 @@ module ResourceAccessTokens end end + def log_event + ::Gitlab::AppLogger.info "PROJECT ACCESS TOKEN REVOCATION: revoked_by: #{current_user.username}, project_id: #{resource.id}, token_user: #{access_token.user.name}, token_id: #{access_token.id}" + end + def error(message) ServiceResponse.error(message: message) end @@ -66,3 +72,5 @@ module ResourceAccessTokens end end end + +ResourceAccessTokens::RevokeService.prepend_if_ee('EE::ResourceAccessTokens::RevokeService') diff --git a/app/services/resource_events/base_change_timebox_service.rb b/app/services/resource_events/base_change_timebox_service.rb index 5c83f7b12f7..d802bbee107 100644 --- a/app/services/resource_events/base_change_timebox_service.rb +++ b/app/services/resource_events/base_change_timebox_service.rb @@ -2,12 +2,11 @@ module ResourceEvents class BaseChangeTimeboxService - attr_reader :resource, :user, :event_created_at + attr_reader :resource, :user - def initialize(resource, user, created_at: Time.current) + def initialize(resource, user) @resource = resource @user = user - @event_created_at = created_at end def execute @@ -27,7 +26,7 @@ module ResourceEvents { user_id: user.id, - created_at: event_created_at, + created_at: resource.system_note_timestamp, key => resource.id } end diff --git a/app/services/resource_events/change_milestone_service.rb b/app/services/resource_events/change_milestone_service.rb index dcdf87599ac..24935a3327a 100644 --- a/app/services/resource_events/change_milestone_service.rb +++ b/app/services/resource_events/change_milestone_service.rb @@ -4,8 +4,8 @@ module ResourceEvents class ChangeMilestoneService < BaseChangeTimeboxService attr_reader :milestone, :old_milestone - def initialize(resource, user, created_at: Time.current, old_milestone:) - super(resource, user, created_at: created_at) + def initialize(resource, user, old_milestone:) + super(resource, user) @milestone = resource&.milestone @old_milestone = old_milestone diff --git a/app/services/security/ci_configuration/sast_create_service.rb b/app/services/security/ci_configuration/sast_create_service.rb new file mode 100644 index 00000000000..8fc3b8d078c --- /dev/null +++ b/app/services/security/ci_configuration/sast_create_service.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +module Security + module CiConfiguration + class SastCreateService < ::BaseService + def initialize(project, current_user, params) + @project = project + @current_user = current_user + @params = params + @branch_name = @project.repository.next_branch('set-sast-config') + end + + def execute + attributes_for_commit = attributes + result = ::Files::MultiService.new(@project, @current_user, attributes_for_commit).execute + + if result[:status] == :success + result[:success_path] = successful_change_path + track_event(attributes_for_commit) + else + result[:errors] = result[:message] + end + + result + + rescue Gitlab::Git::PreReceiveError => e + { status: :error, errors: e.message } + end + + private + + def attributes + actions = Security::CiConfiguration::SastBuildActions.new(@project.auto_devops_enabled?, @params, existing_gitlab_ci_content).generate + + @project.repository.add_branch(@current_user, @branch_name, @project.default_branch) + message = _('Set .gitlab-ci.yml to enable or configure SAST') + + { + commit_message: message, + branch_name: @branch_name, + start_branch: @branch_name, + actions: actions + } + end + + def existing_gitlab_ci_content + gitlab_ci_yml = @project.repository.gitlab_ci_yml_for(@project.repository.root_ref_sha) + YAML.safe_load(gitlab_ci_yml) if gitlab_ci_yml + end + + def successful_change_path + description = _('Set .gitlab-ci.yml to enable or configure SAST security scanning using the GitLab managed template. You can [add variable overrides](https://docs.gitlab.com/ee/user/application_security/sast/#customizing-the-sast-settings) to customize SAST settings.') + merge_request_params = { source_branch: @branch_name, description: description } + Gitlab::Routing.url_helpers.project_new_merge_request_url(@project, merge_request: merge_request_params) + end + + def track_event(attributes_for_commit) + action = attributes_for_commit[:actions].first + + Gitlab::Tracking.event( + self.class.to_s, action[:action], label: action[:default_values_overwritten].to_s + ) + end + end + end +end diff --git a/app/services/security/ci_configuration/sast_parser_service.rb b/app/services/security/ci_configuration/sast_parser_service.rb new file mode 100644 index 00000000000..a8fe5764d19 --- /dev/null +++ b/app/services/security/ci_configuration/sast_parser_service.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +module Security + module CiConfiguration + # This class parses SAST template file and .gitlab-ci.yml to populate default and current values into the JSON + # read from app/validators/json_schemas/security_ci_configuration_schemas/sast_ui_schema.json + class SastParserService < ::BaseService + include Gitlab::Utils::StrongMemoize + + SAST_UI_SCHEMA_PATH = 'app/validators/json_schemas/security_ci_configuration_schemas/sast_ui_schema.json' + + def initialize(project) + @project = project + end + + def configuration + result = Gitlab::Json.parse(File.read(Rails.root.join(SAST_UI_SCHEMA_PATH))).with_indifferent_access + populate_default_value_for(result, :global) + populate_default_value_for(result, :pipeline) + fill_current_value_with_default_for(result, :global) + fill_current_value_with_default_for(result, :pipeline) + populate_current_value_for(result, :global) + populate_current_value_for(result, :pipeline) + + fill_current_value_with_default_for_analyzers(result) + populate_current_value_for_analyzers(result) + + result + end + + private + + def sast_template_content + Gitlab::Template::GitlabCiYmlTemplate.find('SAST').content + end + + def populate_default_value_for(config, level) + set_each(config[level], key: :default_value, with: sast_template_attributes) + end + + def populate_current_value_for(config, level) + set_each(config[level], key: :value, with: gitlab_ci_yml_attributes) + end + + def fill_current_value_with_default_for(config, level) + set_each(config[level], key: :value, with: sast_template_attributes) + end + + def set_each(config_attributes, key:, with:) + config_attributes.each do |entity| + entity[key] = with[entity[:field]] if with[entity[:field]] + end + end + + def fill_current_value_with_default_for_analyzers(result) + result[:analyzers].each do |analyzer| + analyzer[:variables].each do |entity| + entity[:value] = entity[:default_value] if entity[:default_value] + end + end + end + + def populate_current_value_for_analyzers(result) + result[:analyzers].each do |analyzer| + analyzer[:enabled] = analyzer_enabled?(analyzer[:name]) + populate_current_value_for(analyzer, :variables) + end + end + + def analyzer_enabled?(analyzer_name) + # Unless explicitly listed in the excluded analyzers, consider it enabled + sast_excluded_analyzers.exclude?(analyzer_name) + end + + def sast_excluded_analyzers + strong_memoize(:sast_excluded_analyzers) do + all_analyzers = Security::CiConfiguration::SastBuildActions::SAST_DEFAULT_ANALYZERS.split(', ') rescue [] + enabled_analyzers = sast_default_analyzers.split(',').map(&:strip) rescue [] + + excluded_analyzers = gitlab_ci_yml_attributes["SAST_EXCLUDED_ANALYZERS"] || sast_template_attributes["SAST_EXCLUDED_ANALYZERS"] + excluded_analyzers = excluded_analyzers.split(',').map(&:strip) rescue [] + ((all_analyzers - enabled_analyzers) + excluded_analyzers).uniq + end + end + + def sast_default_analyzers + @sast_default_analyzers ||= gitlab_ci_yml_attributes["SAST_DEFAULT_ANALYZERS"] || sast_template_attributes["SAST_DEFAULT_ANALYZERS"] + end + + def sast_template_attributes + @sast_template_attributes ||= build_sast_attributes(sast_template_content) + end + + def gitlab_ci_yml_attributes + @gitlab_ci_yml_attributes ||= begin + config_content = @project.repository.blob_data_at(@project.repository.root_ref_sha, ci_config_file) + return {} unless config_content + + build_sast_attributes(config_content) + end + end + + def ci_config_file + '.gitlab-ci.yml' + end + + def build_sast_attributes(content) + options = { project: @project, user: current_user, sha: @project.repository.commit.sha } + yaml_result = Gitlab::Ci::YamlProcessor.new(content, options).execute + return {} unless yaml_result.valid? + + sast_attributes = yaml_result.build_attributes(:sast) + extract_required_attributes(sast_attributes) + end + + def extract_required_attributes(attributes) + result = {} + attributes[:yaml_variables].each do |variable| + result[variable[:key]] = variable[:value] + end + + result[:stage] = attributes[:stage] + result.with_indifferent_access + end + end + end +end diff --git a/app/services/snippets/base_service.rb b/app/services/snippets/base_service.rb index 278857b7933..415cfcb7d8f 100644 --- a/app/services/snippets/base_service.rb +++ b/app/services/snippets/base_service.rb @@ -2,8 +2,6 @@ module Snippets class BaseService < ::BaseService - include SpamCheckMethods - UPDATE_COMMIT_MSG = 'Update snippet' INITIAL_COMMIT_MSG = 'Initial commit' @@ -18,8 +16,6 @@ module Snippets input_actions = Array(@params.delete(:snippet_actions).presence) @snippet_actions = SnippetInputActionCollection.new(input_actions, allowed_actions: restricted_files_actions) - - filter_spam_check_params end private diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index 0881be73eaf..802bfd813dc 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -3,20 +3,32 @@ 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) + @snippet = build_from_params return invalid_params_error(@snippet) unless valid_params? - unless visibility_allowed?(@snippet, @snippet.visibility_level) - return forbidden_visibility_error(@snippet) + unless visibility_allowed?(snippet, snippet.visibility_level) + return forbidden_visibility_error(snippet) end @snippet.author = current_user - spam_check(@snippet, current_user, action: :create) + unless disable_spam_action_service + Spam::SpamActionService.new( + spammable: @snippet, + request: request, + user: current_user, + action: :create + ).execute(spam_params: spam_params) + end if save_and_commit - UserAgentDetailService.new(@snippet, @request).create + UserAgentDetailService.new(@snippet, request).create Gitlab::UsageDataCounters::SnippetCounter.count(:create) move_temporary_files @@ -29,6 +41,8 @@ module Snippets private + attr_reader :snippet, :request, :spam_params + def build_from_params if project project.snippets.build(create_params) diff --git a/app/services/snippets/update_service.rb b/app/services/snippets/update_service.rb index b982ff98747..5b427817a02 100644 --- a/app/services/snippets/update_service.rb +++ b/app/services/snippets/update_service.rb @@ -7,6 +7,11 @@ 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) + return invalid_params_error(snippet) unless valid_params? if visibility_changed?(snippet) && !visibility_allowed?(snippet, visibility_level) @@ -14,12 +19,20 @@ module Snippets end update_snippet_attributes(snippet) - spam_check(snippet, current_user, action: :update) + + unless disable_spam_action_service + Spam::SpamActionService.new( + spammable: snippet, + request: request, + user: current_user, + action: :update + ).execute(spam_params: spam_params) + end if save_and_commit(snippet) Gitlab::UsageDataCounters::SnippetCounter.count(:update) - ServiceResponse.success(payload: { snippet: snippet } ) + ServiceResponse.success(payload: { snippet: snippet }) else snippet_error_response(snippet, 400) end @@ -27,6 +40,8 @@ module Snippets private + attr_reader :request, :spam_params + def visibility_changed?(snippet) visibility_level && visibility_level.to_i != snippet.visibility_level end diff --git a/app/services/spam/spam_action_service.rb b/app/services/spam/spam_action_service.rb index b3d617256ff..ff32bc32d93 100644 --- a/app/services/spam/spam_action_service.rb +++ b/app/services/spam/spam_action_service.rb @@ -4,37 +4,69 @@ 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). + # + # 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) + # 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 + captcha_response = params.delete(:captcha_response) + + SpamParams.new( + api: params.delete(:api), + captcha_response: captcha_response, + spam_log_id: params.delete(:spam_log_id) + ) + end + attr_accessor :target, :request, :options attr_reader :spam_log - def initialize(spammable:, request:, user:, context: {}) + def initialize(spammable:, request:, user:, action:) @target = spammable @request = request @user = user - @context = context + @action = action @options = {} + end - if @request - @options[:ip_address] = @request.env['action_dispatch.remote_ip'].to_s - @options[:user_agent] = @request.env['HTTP_USER_AGENT'] - @options[:referrer] = @request.env['HTTP_REFERRER'] + 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[:referrer] = request.env['HTTP_REFERRER'] else - @options[:ip_address] = @target.ip_address - @options[:user_agent] = @target.user_agent + # 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 - end - def execute(api: false, recaptcha_verified:, spam_log_id:) + recaptcha_verified = Captcha::CaptchaVerificationService.new.execute( + captcha_response: spam_params.captcha_response, + request: request + ) + if recaptcha_verified - # If it's a request which is already verified through reCAPTCHA, + # If it's a request which is already verified through captcha, # update the spam log accordingly. - SpamLog.verify_recaptcha!(user_id: user.id, id: spam_log_id) + SpamLog.verify_recaptcha!(user_id: user.id, id: spam_params.spam_log_id) + ServiceResponse.success(message: "Captcha was successfully verified") else - return if allowlisted?(user) - return unless request - return unless check_for_spam? + 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(api) + perform_spam_service_check(spam_params.api) + ServiceResponse.success(message: "Spam check performed, check #{target.class.name} spammable model for any errors or captcha requirement") end end @@ -42,13 +74,27 @@ module Spam private - attr_reader :user, :context + attr_reader :user, :action + + ## + # In order to be proceed to the spam check process, the target must be + # a dirty instance, which means it should be already assigned with the new + # attribute values. + def ensure_target_is_dirty + msg = "Target instance of #{target.class.name} must be dirty (must have changes to save)" + raise(msg) unless target.has_changes_to_save? + end def allowlisted?(user) user.try(:gitlab_employee?) || user.try(:gitlab_bot?) || user.try(:gitlab_service_user?) end + ## + # 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) + ensure_target_is_dirty + # since we can check for spam, and recaptcha is not verified, # ask the SpamVerdictService what to do with the target. spam_verdict_service.execute.tap do |result| @@ -79,7 +125,7 @@ module Spam description: target.spam_description, source_ip: options[:ip_address], user_agent: options[:user_agent], - noteable_type: notable_type, + noteable_type: noteable_type, via_api: api } ) @@ -88,14 +134,19 @@ module Spam end def spam_verdict_service + context = { + action: action, + target_type: noteable_type + } + SpamVerdictService.new(target: target, user: user, - request: @request, + request: request, options: options, - context: context.merge(target_type: notable_type)) + context: context) end - def notable_type + def noteable_type @notable_type ||= target.class.to_s end end diff --git a/app/services/spam/spam_params.rb b/app/services/spam/spam_params.rb new file mode 100644 index 00000000000..fef5355c7f3 --- /dev/null +++ b/app/services/spam/spam_params.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +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. + # + # 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. + class SpamParams + attr_reader :api, :captcha_response, :spam_log_id + + def initialize(api:, captcha_response:, spam_log_id:) + @api = api.present? + @captcha_response = captcha_response + @spam_log_id = spam_log_id + end + + def ==(other) + other.class == self.class && + other.api == self.api && + other.captcha_response == self.captcha_response && + other.spam_log_id == self.spam_log_id + end + end +end diff --git a/app/services/suggestions/apply_service.rb b/app/services/suggestions/apply_service.rb index ab80b23a37b..f9783f4271f 100644 --- a/app/services/suggestions/apply_service.rb +++ b/app/services/suggestions/apply_service.rb @@ -2,8 +2,9 @@ module Suggestions class ApplyService < ::BaseService - def initialize(current_user, *suggestions) + def initialize(current_user, *suggestions, message: nil) @current_user = current_user + @message = message @suggestion_set = Gitlab::Suggestions::SuggestionSet.new(suggestions) end @@ -30,6 +31,9 @@ module Suggestions Suggestion.id_in(suggestion_set.suggestions) .update_all(commit_id: result[:result], applied: true) + + Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter + .track_apply_suggestion_action(user: current_user) end def multi_service @@ -44,7 +48,7 @@ module Suggestions end def commit_message - Gitlab::Suggestions::CommitMessage.new(current_user, suggestion_set).message + Gitlab::Suggestions::CommitMessage.new(current_user, suggestion_set, @message).message end end end diff --git a/app/services/suggestions/create_service.rb b/app/services/suggestions/create_service.rb index 93d2bd11426..a97c36fa0ca 100644 --- a/app/services/suggestions/create_service.rb +++ b/app/services/suggestions/create_service.rb @@ -27,6 +27,8 @@ module Suggestions rows.in_groups_of(100, false) do |rows| Gitlab::Database.bulk_insert('suggestions', rows) # rubocop:disable Gitlab/BulkInsert end + + Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter.track_add_suggestion_action(user: @note.author) end end end diff --git a/app/services/system_hooks_service.rb b/app/services/system_hooks_service.rb index 881a139437a..5273dedb56f 100644 --- a/app/services/system_hooks_service.rb +++ b/app/services/system_hooks_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class SystemHooksService - BUILDER_DRIVEN_EVENT_DATA_AVAILABLE_FOR_CLASSES = [GroupMember].freeze + BUILDER_DRIVEN_EVENT_DATA_AVAILABLE_FOR_CLASSES = [GroupMember, Group].freeze def execute_hooks_for(model, event) data = build_event_data(model, event) @@ -58,15 +58,6 @@ class SystemHooksService end when ProjectMember data.merge!(project_member_data(model)) - when Group - data.merge!(group_data(model)) - - if event == :rename - data.merge!( - old_path: model.path_before_last_save, - old_full_path: model.full_path_before_last_save - ) - end end data @@ -114,19 +105,6 @@ class SystemHooksService } end - def group_data(model) - owner = model.owner - - { - name: model.name, - path: model.path, - full_path: model.full_path, - group_id: model.id, - owner_name: owner.try(:name), - owner_email: owner.try(:email) - } - end - def user_data(model) { name: model.name, @@ -141,10 +119,14 @@ class SystemHooksService end def builder_driven_event_data(model, event) - case model - when GroupMember - Gitlab::HookData::GroupMemberBuilder.new(model).build(event) - end + builder_class = case model + when GroupMember + Gitlab::HookData::GroupMemberBuilder + when Group + Gitlab::HookData::GroupBuilder + end + + builder_class.new(model).build(event) end end diff --git a/app/services/terraform/remote_state_handler.rb b/app/services/terraform/remote_state_handler.rb index 7e79cb9e007..9500a821071 100644 --- a/app/services/terraform/remote_state_handler.rb +++ b/app/services/terraform/remote_state_handler.rb @@ -68,12 +68,14 @@ module Terraform find_params = { project: project, name: params[:name] } - if find_only - Terraform::State.find_by(find_params) || # rubocop: disable CodeReuse/ActiveRecord - raise(ActiveRecord::RecordNotFound.new("Couldn't find state")) - else - Terraform::State.create_or_find_by(find_params) - end + return find_state!(find_params) if find_only + + state = Terraform::State.create_or_find_by(find_params) + + # https://github.com/rails/rails/issues/36027 + return state unless state.errors.of_kind? :name, :taken + + find_state(find_params) end def lock_matches?(state) @@ -86,5 +88,13 @@ module Terraform def can_modify_state? current_user.can?(:admin_terraform_state, project) end + + def find_state(find_params) + Terraform::State.find_by(find_params) # rubocop: disable CodeReuse/ActiveRecord + end + + def find_state!(find_params) + find_state(find_params) || raise(ActiveRecord::RecordNotFound.new("Couldn't find state")) + end end end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 12d26fe890b..dea116c8546 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -212,6 +212,11 @@ class TodoService current_user.update_todos_count_cache end + def create_request_review_todo(target, author, reviewers) + attributes = attributes_for_todo(target.project, target, author, Todo::REVIEW_REQUESTED) + create_todos(reviewers, attributes) + end + private def create_todos(users, attributes) @@ -266,8 +271,7 @@ class TodoService def create_reviewer_todo(target, author, old_reviewers = []) if target.reviewers.any? reviewers = target.reviewers - old_reviewers - attributes = attributes_for_todo(target.project, target, author, Todo::REVIEW_REQUESTED) - create_todos(reviewers, attributes) + create_request_review_todo(target, author, reviewers) end end diff --git a/app/services/users/approve_service.rb b/app/services/users/approve_service.rb index debd1e8cd17..fea7fc55d90 100644 --- a/app/services/users/approve_service.rb +++ b/app/services/users/approve_service.rb @@ -8,8 +8,7 @@ module Users def execute(user) return error(_('You are not allowed to approve a user'), :forbidden) unless allowed? - return error(_('The user you are trying to approve is not pending an approval'), :conflict) if user.active? - return error(_('The user you are trying to approve is not pending an approval'), :conflict) unless approval_required?(user) + return error(_('The user you are trying to approve is not pending approval'), :conflict) if user.active? || !approval_required?(user) if user.activate # Resends confirmation email if the user isn't confirmed yet. @@ -18,6 +17,7 @@ module Users user.accept_pending_invitations! if user.active_for_authentication? DeviseMailer.user_admin_approval(user).deliver_later + log_event(user) after_approve_hook(user) success(message: 'Success', http_status: :created) else @@ -40,6 +40,10 @@ module Users def approval_required?(user) user.blocked_pending_approval? end + + def log_event(user) + Gitlab::AppLogger.info(message: "User instance access request approved", user: "#{user.username}", email: "#{user.email}", approved_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + end end end diff --git a/app/services/users/refresh_authorized_projects_service.rb b/app/services/users/refresh_authorized_projects_service.rb index d0939d5a542..24e3fb73370 100644 --- a/app/services/users/refresh_authorized_projects_service.rb +++ b/app/services/users/refresh_authorized_projects_service.rb @@ -14,13 +14,14 @@ module Users # service = Users::RefreshAuthorizedProjectsService.new(some_user) # service.execute class RefreshAuthorizedProjectsService - attr_reader :user + attr_reader :user, :source LEASE_TIMEOUT = 1.minute.to_i # user - The User for which to refresh the authorized projects. - def initialize(user, incorrect_auth_found_callback: nil, missing_auth_found_callback: nil) + def initialize(user, source: nil, incorrect_auth_found_callback: nil, missing_auth_found_callback: nil) @user = user + @source = source @incorrect_auth_found_callback = incorrect_auth_found_callback @missing_auth_found_callback = missing_auth_found_callback @@ -91,6 +92,8 @@ module Users # remove - The IDs of the authorization rows to remove. # add - Rows to insert in the form `[user id, project id, access level]` def update_authorizations(remove = [], add = []) + log_refresh_details(remove.length, add.length) + User.transaction do user.remove_project_authorizations(remove) unless remove.empty? ProjectAuthorization.insert_authorizations(add) unless add.empty? @@ -101,6 +104,13 @@ module Users user.reset end + def log_refresh_details(rows_deleted, rows_added) + Gitlab::AppJsonLogger.info(event: 'authorized_projects_refresh', + 'authorized_projects_refresh.source': source, + 'authorized_projects_refresh.rows_deleted': rows_deleted, + 'authorized_projects_refresh.rows_added': rows_added) + end + def fresh_access_levels_per_project fresh_authorizations.each_with_object({}) do |row, hash| hash[row.project_id] = row.access_level diff --git a/app/services/users/reject_service.rb b/app/services/users/reject_service.rb index dd72547c688..0e3eb3e5dde 100644 --- a/app/services/users/reject_service.rb +++ b/app/services/users/reject_service.rb @@ -12,8 +12,12 @@ module Users user.delete_async(deleted_by: current_user, params: { hard_delete: true }) + after_reject_hook(user) + NotificationService.new.user_admin_rejection(user.name, user.email) + log_event(user) + success end @@ -24,5 +28,15 @@ module Users def allowed? can?(current_user, :reject_user) end + + def after_reject_hook(user) + # overridden by EE module + end + + def log_event(user) + Gitlab::AppLogger.info(message: "User instance access request rejected", user: "#{user.username}", email: "#{user.email}", rejected_by: "#{current_user.username}", ip_address: "#{current_user.current_sign_in_ip}") + end end end + +Users::RejectService.prepend_if_ee('EE::Users::RejectService') |