diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-03-18 20:02:30 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-03-18 20:02:30 +0000 |
commit | 41fe97390ceddf945f3d967b8fdb3de4c66b7dea (patch) | |
tree | 9c8d89a8624828992f06d892cd2f43818ff5dcc8 /app/services | |
parent | 0804d2dc31052fb45a1efecedc8e06ce9bc32862 (diff) | |
download | gitlab-ce-41fe97390ceddf945f3d967b8fdb3de4c66b7dea.tar.gz |
Add latest changes from gitlab-org/gitlab@14-9-stable-eev14.9.0-rc42
Diffstat (limited to 'app/services')
112 files changed, 1329 insertions, 684 deletions
diff --git a/app/services/alert_management/create_alert_issue_service.rb b/app/services/alert_management/create_alert_issue_service.rb index ab8d1176b9e..34c2003bd01 100644 --- a/app/services/alert_management/create_alert_issue_service.rb +++ b/app/services/alert_management/create_alert_issue_service.rb @@ -22,9 +22,7 @@ module AlertManagement return result unless result.success? issue = result.payload[:issue] - update_title_for(issue) - - SystemNoteService.new_alert_issue(alert, issue, user) + perform_after_create_tasks(issue) result end @@ -56,6 +54,12 @@ module AlertManagement issue.update!(title: "#{DEFAULT_INCIDENT_TITLE} #{issue.iid}") end + def perform_after_create_tasks(issue) + update_title_for(issue) + + SystemNoteService.new_alert_issue(alert, issue, user) + end + def error(message, issue = nil) ServiceResponse.error(payload: { issue: issue }, message: message) end @@ -75,3 +79,5 @@ module AlertManagement end end end + +AlertManagement::CreateAlertIssueService.prepend_mod diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb index 84518fd6b0e..bb6a52eb2f4 100644 --- a/app/services/auth/container_registry_authentication_service.rb +++ b/app/services/auth/container_registry_authentication_service.rb @@ -59,12 +59,7 @@ module Auth token.expire_time = token_expire_at token[:access] = names.map do |name| - { - type: type, - name: name, - actions: actions, - migration_eligible: type == 'repository' ? migration_eligible(repository_path: name) : nil - }.compact + { type: type, name: name, actions: actions } end token.encoded @@ -136,12 +131,7 @@ module Auth # ensure_container_repository!(path, authorized_actions) - { - type: type, - name: path.to_s, - actions: authorized_actions, - migration_eligible: self.class.migration_eligible(project: requested_project) - }.compact + { type: type, name: path.to_s, actions: authorized_actions } end def actively_importing?(actions, path) @@ -153,28 +143,6 @@ module Auth container_repository.migration_importing? end - def self.migration_eligible(project: nil, repository_path: nil) - return unless Feature.enabled?(:container_registry_migration_phase1) - - # project has precedence over repository_path. If only the latter is provided, we find the corresponding Project. - unless project - return unless repository_path - - project = ContainerRegistry::Path.new(repository_path).repository_project - end - - # The migration process will start by allowing only specific test and gitlab-org projects using the - # `container_registry_migration_phase1_allow` FF. We'll then move on to a percentage rollout using this same FF. - # To remove the risk of impacting enterprise customers that rely heavily on the registry during the percentage - # rollout, we'll add their top-level group/namespace to the `container_registry_migration_phase1_deny` FF. Later, - # we'll remove them manually from this deny list, and their new repositories will become eligible. - Feature.disabled?(:container_registry_migration_phase1_deny, project.root_ancestor) && - Feature.enabled?(:container_registry_migration_phase1_allow, project) - rescue ContainerRegistry::Path::InvalidRegistryPathError => ex - Gitlab::ErrorTracking.track_and_raise_for_dev_exception(ex, **Gitlab::ApplicationContext.current) - false - end - ## # Because we do not have two way communication with registry yet, # we create a container repository image resource when push to the diff --git a/app/services/boards/base_items_list_service.rb b/app/services/boards/base_items_list_service.rb index a3e24844587..01fad14d036 100644 --- a/app/services/boards/base_items_list_service.rb +++ b/app/services/boards/base_items_list_service.rb @@ -12,21 +12,32 @@ module Boards end # rubocop: disable CodeReuse/ActiveRecord - def metadata - issuables = item_model.arel_table - keys = metadata_fields.keys + def metadata(required_fields = [:issue_count, :total_issue_weight]) + fields = metadata_fields(required_fields) + keys = fields.keys # TODO: eliminate need for SQL literal fragment - columns = Arel.sql(metadata_fields.values_at(*keys).join(', ')) - results = item_model.where(id: init_collection.select(issuables[:id])).pluck(columns) + columns = Arel.sql(fields.values_at(*keys).join(', ')) + results = item_model.where(id: collection_ids) + results = query_additions(results, required_fields) + results = results.select(columns) - Hash[keys.zip(results.flatten)] + Hash[keys.zip(results.pluck(columns).flatten)] end # rubocop: enable CodeReuse/ActiveRecord private - def metadata_fields - { size: 'COUNT(*)' } + # override if needed + def query_additions(items, required_fields) + items + end + + def collection_ids + @collection_ids ||= init_collection.select(item_model.arel_table[:id]) + end + + def metadata_fields(required_fields) + required_fields&.include?(:issue_count) ? { size: 'COUNT(*)' } : {} end def order(items) diff --git a/app/services/bulk_create_integration_service.rb b/app/services/bulk_create_integration_service.rb index a7fe4c776b7..3a214122ed3 100644 --- a/app/services/bulk_create_integration_service.rb +++ b/app/services/bulk_create_integration_service.rb @@ -32,11 +32,7 @@ class BulkCreateIntegrationService end def integration_hash - if integration.template? - integration.to_integration_hash - else - integration.to_integration_hash.tap { |json| json['inherit_from_id'] = integration.inherit_from_id || integration.id } - end + integration.to_integration_hash.tap { |json| json['inherit_from_id'] = integration.inherit_from_id || integration.id } end def data_fields_hash diff --git a/app/services/ci/after_requeue_job_service.rb b/app/services/ci/after_requeue_job_service.rb index 097b29cf143..bc70dd3bea4 100644 --- a/app/services/ci/after_requeue_job_service.rb +++ b/app/services/ci/after_requeue_job_service.rb @@ -22,9 +22,15 @@ module Ci end def dependent_jobs - stage_dependent_jobs - .or(needs_dependent_jobs.except(:preload)) + dependent_jobs = stage_dependent_jobs + .or(needs_dependent_jobs) .ordered_by_stage + + if ::Feature.enabled?(:ci_fix_order_of_subsequent_jobs, @processable.pipeline.project, default_enabled: :yaml) + dependent_jobs = ordered_by_dag(dependent_jobs) + end + + dependent_jobs end def process(job) @@ -44,5 +50,23 @@ module Ci def skipped_jobs @skipped_jobs ||= @processable.pipeline.processables.skipped end + + # rubocop: disable CodeReuse/ActiveRecord + def ordered_by_dag(jobs) + sorted_job_names = sort_jobs(jobs).each_with_index.to_h + + jobs.preload(:needs).group_by(&:stage_idx).flat_map do |_, stage_jobs| + stage_jobs.sort_by { |job| sorted_job_names.fetch(job.name) } + end + end + + def sort_jobs(jobs) + Gitlab::Ci::YamlProcessor::Dag.order( + jobs.to_h do |job| + [job.name, job.needs.map(&:name)] + end + ) + end + # rubocop: enable CodeReuse/ActiveRecord end end diff --git a/app/services/ci/create_downstream_pipeline_service.rb b/app/services/ci/create_downstream_pipeline_service.rb index a2e53cbf9b8..034bab93108 100644 --- a/app/services/ci/create_downstream_pipeline_service.rb +++ b/app/services/ci/create_downstream_pipeline_service.rb @@ -120,15 +120,13 @@ module Ci def has_cyclic_dependency? return false if @bridge.triggers_child_pipeline? - if Feature.enabled?(:ci_drop_cyclical_triggered_pipelines, @bridge.project, default_enabled: :yaml) - pipeline_checksums = @bridge.pipeline.self_and_upstreams.filter_map do |pipeline| - config_checksum(pipeline) unless pipeline.child? - end - - # To avoid false positives we allow 1 cycle in the ancestry and - # fail when 2 cycles are detected: A -> B -> A -> B -> A - pipeline_checksums.tally.any? { |_checksum, occurrences| occurrences > 2 } + pipeline_checksums = @bridge.pipeline.self_and_upstreams.filter_map do |pipeline| + config_checksum(pipeline) unless pipeline.child? end + + # To avoid false positives we allow 1 cycle in the ancestry and + # fail when 2 cycles are detected: A -> B -> A -> B -> A + pipeline_checksums.tally.any? { |_checksum, occurrences| occurrences > 2 } end def has_max_descendants_depth? diff --git a/app/services/ci/destroy_secure_file_service.rb b/app/services/ci/destroy_secure_file_service.rb new file mode 100644 index 00000000000..7145ace7f31 --- /dev/null +++ b/app/services/ci/destroy_secure_file_service.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Ci + class DestroySecureFileService < BaseService + def execute(secure_file) + raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_secure_files, secure_file.project) + + secure_file.destroy! + end + end +end diff --git a/app/services/ci/pipeline_processing/atomic_processing_service/status_collection.rb b/app/services/ci/pipeline_processing/atomic_processing_service/status_collection.rb index 883a70c9795..4d1b2e07d7f 100644 --- a/app/services/ci/pipeline_processing/atomic_processing_service/status_collection.rb +++ b/app/services/ci/pipeline_processing/atomic_processing_service/status_collection.rb @@ -91,17 +91,13 @@ module Ci def all_statuses_by_id strong_memoize(:all_statuses_by_id) do - all_statuses.to_h do |row| - [row[:id], row] - end + all_statuses.index_by { |row| row[:id] } end end def all_statuses_by_name strong_memoize(:statuses_by_name) do - all_statuses.to_h do |row| - [row[:name], row] - end + all_statuses.index_by { |row| row[:name] } end end diff --git a/app/services/ci/register_runner_service.rb b/app/services/ci/register_runner_service.rb deleted file mode 100644 index 7c6cd82565d..00000000000 --- a/app/services/ci/register_runner_service.rb +++ /dev/null @@ -1,58 +0,0 @@ -# frozen_string_literal: true - -module Ci - class RegisterRunnerService - def execute(registration_token, attributes) - runner_type_attrs = extract_runner_type_attrs(registration_token) - - return unless runner_type_attrs - - ::Ci::Runner.create(attributes.merge(runner_type_attrs)) - end - - private - - def extract_runner_type_attrs(registration_token) - @attrs_from_token ||= check_token(registration_token) - - return unless @attrs_from_token - - attrs = @attrs_from_token.clone - case attrs[:runner_type] - when :project_type - attrs[:projects] = [attrs.delete(:scope)] - when :group_type - attrs[:groups] = [attrs.delete(:scope)] - end - - attrs - end - - def check_token(registration_token) - if runner_registration_token_valid?(registration_token) - # Create shared runner. Requires admin access - { runner_type: :instance_type } - elsif runner_registrar_valid?('project') && project = ::Project.find_by_runners_token(registration_token) - # Create a specific runner for the project - { runner_type: :project_type, scope: project } - elsif runner_registrar_valid?('group') && group = ::Group.find_by_runners_token(registration_token) - # Create a specific runner for the group - { runner_type: :group_type, scope: group } - end - end - - def runner_registration_token_valid?(registration_token) - ActiveSupport::SecurityUtils.secure_compare(registration_token, Gitlab::CurrentSettings.runners_registration_token) - end - - def runner_registrar_valid?(type) - Feature.disabled?(:runner_registration_control) || Gitlab::CurrentSettings.valid_runner_registrars.include?(type) - end - - def token_scope - @attrs_from_token[:scope] - end - end -end - -Ci::RegisterRunnerService.prepend_mod diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb index 73c5d0163da..906e5cec4f3 100644 --- a/app/services/ci/retry_build_service.rb +++ b/app/services/ci/retry_build_service.rb @@ -65,7 +65,7 @@ module Ci def check_access!(build) unless can?(current_user, :update_build, build) - raise Gitlab::Access::AccessDeniedError + raise Gitlab::Access::AccessDeniedError, '403 Forbidden' end end diff --git a/app/services/ci/retry_pipeline_service.rb b/app/services/ci/retry_pipeline_service.rb index 9ad46ca7585..d40643e1513 100644 --- a/app/services/ci/retry_pipeline_service.rb +++ b/app/services/ci/retry_pipeline_service.rb @@ -5,9 +5,8 @@ module Ci include Gitlab::OptimisticLocking def execute(pipeline) - unless can?(current_user, :update_pipeline, pipeline) - raise Gitlab::Access::AccessDeniedError - end + access_response = check_access(pipeline) + return access_response if access_response.error? pipeline.ensure_scheduling_type! @@ -30,6 +29,18 @@ module Ci Ci::ProcessPipelineService .new(pipeline) .execute + + ServiceResponse.success + rescue Gitlab::Access::AccessDeniedError => e + ServiceResponse.error(message: e.message, http_status: :forbidden) + end + + def check_access(pipeline) + if can?(current_user, :update_pipeline, pipeline) + ServiceResponse.success + else + ServiceResponse.error(message: '403 Forbidden', http_status: :forbidden) + end end private diff --git a/app/services/ci/runners/assign_runner_service.rb b/app/services/ci/runners/assign_runner_service.rb new file mode 100644 index 00000000000..886cd3a4e44 --- /dev/null +++ b/app/services/ci/runners/assign_runner_service.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Ci + module Runners + class AssignRunnerService + # @param [Ci::Runner] runner: the runner to assign to a project + # @param [Project] project: the new project to assign the runner to + # @param [User] user: the user performing the operation + def initialize(runner, project, user) + @runner = runner + @project = project + @user = user + end + + def execute + return false unless @user.present? && @user.can?(:assign_runner, @runner) + + @runner.assign_to(@project, @user) + end + + private + + attr_reader :runner, :project, :user + end + end +end + +Ci::Runners::AssignRunnerService.prepend_mod diff --git a/app/services/ci/runners/register_runner_service.rb b/app/services/ci/runners/register_runner_service.rb new file mode 100644 index 00000000000..7978d094d9b --- /dev/null +++ b/app/services/ci/runners/register_runner_service.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +module Ci + module Runners + class RegisterRunnerService + def execute(registration_token, attributes) + runner_type_attrs = extract_runner_type_attrs(registration_token) + + return unless runner_type_attrs + + ::Ci::Runner.create(attributes.merge(runner_type_attrs)) + end + + private + + def extract_runner_type_attrs(registration_token) + @attrs_from_token ||= check_token(registration_token) + + return unless @attrs_from_token + + attrs = @attrs_from_token.clone + case attrs[:runner_type] + when :project_type + attrs[:projects] = [attrs.delete(:scope)] + when :group_type + attrs[:groups] = [attrs.delete(:scope)] + end + + attrs + end + + def check_token(registration_token) + if runner_registration_token_valid?(registration_token) + # Create shared runner. Requires admin access + { runner_type: :instance_type } + elsif runner_registrar_valid?('project') && project = ::Project.find_by_runners_token(registration_token) + # Create a specific runner for the project + { runner_type: :project_type, scope: project } + elsif runner_registrar_valid?('group') && group = ::Group.find_by_runners_token(registration_token) + # Create a specific runner for the group + { runner_type: :group_type, scope: group } + end + end + + def runner_registration_token_valid?(registration_token) + ActiveSupport::SecurityUtils.secure_compare(registration_token, Gitlab::CurrentSettings.runners_registration_token) + end + + def runner_registrar_valid?(type) + Feature.disabled?(:runner_registration_control, default_enabled: :yaml) || Gitlab::CurrentSettings.valid_runner_registrars.include?(type) + end + + def token_scope + @attrs_from_token[:scope] + end + end + end +end + +Ci::Runners::RegisterRunnerService.prepend_mod diff --git a/app/services/ci/runners/reset_registration_token_service.rb b/app/services/ci/runners/reset_registration_token_service.rb new file mode 100644 index 00000000000..bbe49c04644 --- /dev/null +++ b/app/services/ci/runners/reset_registration_token_service.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Ci + module Runners + class ResetRegistrationTokenService + # @param [ApplicationSetting, Project, Group] scope: the scope of the reset operation + # @param [User] user: the user performing the operation + def initialize(scope, user) + @scope = scope + @user = user + end + + def execute + return unless @user.present? && @user.can?(:update_runners_registration_token, scope) + + case scope + when ::ApplicationSetting + scope.reset_runners_registration_token! + ApplicationSetting.current_without_cache.runners_registration_token + when ::Group, ::Project + scope.reset_runners_token! + scope.runners_token + end + end + + private + + attr_reader :scope, :user + end + end +end diff --git a/app/services/ci/runners/unassign_runner_service.rb b/app/services/ci/runners/unassign_runner_service.rb new file mode 100644 index 00000000000..1e46cf6add8 --- /dev/null +++ b/app/services/ci/runners/unassign_runner_service.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Ci + module Runners + class UnassignRunnerService + # @param [Ci::RunnerProject] runner_project the runner/project association to destroy + # @param [User] user the user performing the operation + def initialize(runner_project, user) + @runner_project = runner_project + @runner = runner_project.runner + @project = runner_project.project + @user = user + end + + def execute + return false unless @user.present? && @user.can?(:assign_runner, @runner) + + @runner_project.destroy + end + + private + + attr_reader :runner, :project, :user + end + end +end + +Ci::Runners::UnassignRunnerService.prepend_mod diff --git a/app/services/ci/runners/unregister_runner_service.rb b/app/services/ci/runners/unregister_runner_service.rb new file mode 100644 index 00000000000..4ee1e73c458 --- /dev/null +++ b/app/services/ci/runners/unregister_runner_service.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Ci + module Runners + class UnregisterRunnerService + attr_reader :runner, :author + + # @param [Ci::Runner] runner the runner to unregister/destroy + # @param [User, authentication token String] author the user or the authentication token that authorizes the removal + def initialize(runner, author) + @runner = runner + @author = author + end + + def execute + @runner&.destroy + end + end + end +end + +Ci::Runners::UnregisterRunnerService.prepend_mod diff --git a/app/services/ci/runners/update_runner_service.rb b/app/services/ci/runners/update_runner_service.rb new file mode 100644 index 00000000000..6cc080f81c2 --- /dev/null +++ b/app/services/ci/runners/update_runner_service.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Ci + module Runners + class UpdateRunnerService + attr_reader :runner + + def initialize(runner) + @runner = runner + end + + def update(params) + params[:active] = !params.delete(:paused) if params.include?(:paused) + + runner.update(params).tap do |updated| + runner.tick_runner_queue if updated + end + end + end + end +end diff --git a/app/services/ci/test_failure_history_service.rb b/app/services/ci/test_failure_history_service.rb index a3f45c1b9cd..7323ad417ea 100644 --- a/app/services/ci/test_failure_history_service.rb +++ b/app/services/ci/test_failure_history_service.rb @@ -17,6 +17,7 @@ module Ci MAX_TRACKABLE_FAILURES = 200 attr_reader :pipeline + delegate :project, to: :pipeline def initialize(pipeline) diff --git a/app/services/ci/unregister_runner_service.rb b/app/services/ci/unregister_runner_service.rb deleted file mode 100644 index 97d9852b7ed..00000000000 --- a/app/services/ci/unregister_runner_service.rb +++ /dev/null @@ -1,16 +0,0 @@ -# frozen_string_literal: true - -module Ci - class UnregisterRunnerService - attr_reader :runner - - # @param [Ci::Runner] runner the runner to unregister/destroy - def initialize(runner) - @runner = runner - end - - def execute - @runner&.destroy - end - end -end diff --git a/app/services/ci/update_runner_service.rb b/app/services/ci/update_runner_service.rb deleted file mode 100644 index 4a17e25c0cc..00000000000 --- a/app/services/ci/update_runner_service.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -module Ci - class UpdateRunnerService - attr_reader :runner - - def initialize(runner) - @runner = runner - end - - def update(params) - params[:active] = !params.delete(:paused) if params.include?(:paused) - - runner.update(params).tap do |updated| - runner.tick_runner_queue if updated - end - end - end -end diff --git a/app/services/concerns/members/bulk_create_users.rb b/app/services/concerns/members/bulk_create_users.rb index 9cfef96311e..3f8971dde74 100644 --- a/app/services/concerns/members/bulk_create_users.rb +++ b/app/services/concerns/members/bulk_create_users.rb @@ -47,16 +47,15 @@ module Members end end - if user_ids.present? - # we should handle the idea of existing members where users are passed as users - https://gitlab.com/gitlab-org/gitlab/-/issues/352617 - # the below will automatically discard invalid user_ids - users.concat(User.id_in(user_ids)) + # the below will automatically discard invalid user_ids + users.concat(User.id_in(user_ids)) if user_ids.present? + users.uniq! # de-duplicate just in case as there is no controlling if user records and ids are sent multiple times + + if users.present? # helps not have to perform another query per user id to see if the member exists later on when fetching - existing_members = source.members_and_requesters.where(user_id: user_ids).index_by(&:user_id) # rubocop:disable CodeReuse/ActiveRecord + existing_members = source.members_and_requesters.where(user_id: users).index_by(&:user_id) # rubocop:disable CodeReuse/ActiveRecord end - users.uniq! # de-duplicate just in case as there is no controlling if user records and ids are sent multiple times - [emails, users, existing_members] end end diff --git a/app/services/concerns/rate_limited_service.rb b/app/services/concerns/rate_limited_service.rb index c8dc60355cf..5d7247a5b99 100644 --- a/app/services/concerns/rate_limited_service.rb +++ b/app/services/concerns/rate_limited_service.rb @@ -36,7 +36,6 @@ module RateLimitedService def rate_limit!(service) evaluated_scope = evaluated_scope_for(service) - return if feature_flag_disabled?(evaluated_scope[:project]) if rate_limiter.throttled?(key, **opts.merge(scope: evaluated_scope.values, users_allowlist: users_allowlist)) raise RateLimitedError.new(key: key, rate_limiter: rate_limiter), _('This endpoint has been requested too many times. Try again later.') @@ -54,14 +53,11 @@ module RateLimitedService all[var] = service.public_send(var) # rubocop: disable GitlabSecurity/PublicSend end end - - def feature_flag_disabled?(project) - Feature.disabled?("rate_limited_service_#{key}", project, default_enabled: :yaml) - end end prepended do attr_accessor :rate_limiter_bypassed + cattr_accessor :rate_limiter_scoped_and_keyed def self.rate_limit(key:, opts:, rate_limiter: ::Gitlab::ApplicationRateLimiter) diff --git a/app/services/concerns/update_repository_storage_methods.rb b/app/services/concerns/update_repository_storage_methods.rb index cbcd0b7f56b..b21d05f4178 100644 --- a/app/services/concerns/update_repository_storage_methods.rb +++ b/app/services/concerns/update_repository_storage_methods.rb @@ -6,6 +6,7 @@ module UpdateRepositoryStorageMethods Error = Class.new(StandardError) attr_reader :repository_storage_move + delegate :container, :source_storage_name, :destination_storage_name, to: :repository_storage_move def initialize(repository_storage_move) diff --git a/app/services/error_tracking/base_service.rb b/app/services/error_tracking/base_service.rb index 289c125b9d1..598621f70e1 100644 --- a/app/services/error_tracking/base_service.rb +++ b/app/services/error_tracking/base_service.rb @@ -1,7 +1,13 @@ # frozen_string_literal: true module ErrorTracking - class BaseService < ::BaseService + class BaseService < ::BaseProjectService + include Gitlab::Utils::UsageData + + def initialize(project, user = nil, params = {}) + super(project: project, current_user: user, params: params.dup) + end + def execute return unauthorized if unauthorized @@ -21,6 +27,8 @@ module ErrorTracking yield if block_given? + track_usage_event(params[:tracking_event], current_user.id) if params[:tracking_event] + success(parse_response(response)) end diff --git a/app/services/error_tracking/collect_error_service.rb b/app/services/error_tracking/collect_error_service.rb index 50508c9810a..6376b743255 100644 --- a/app/services/error_tracking/collect_error_service.rb +++ b/app/services/error_tracking/collect_error_service.rb @@ -60,7 +60,7 @@ module ErrorTracking end def actor - return event['transaction'] if event['transaction'] + return event['transaction'] if event['transaction'].present? # Some SDKs do not have a transaction attribute. # So we build it by combining function name and module name from diff --git a/app/services/google_cloud/create_service_accounts_service.rb b/app/services/google_cloud/create_service_accounts_service.rb index e360b3a8e4e..51d08cc5b55 100644 --- a/app/services/google_cloud/create_service_accounts_service.rb +++ b/app/services/google_cloud/create_service_accounts_service.rb @@ -12,7 +12,7 @@ module GoogleCloud service_account.project_id, service_account.to_json, service_account_key.to_json, - environment_protected? + ProtectedBranch.protected?(project, environment_name) || ProtectedTag.protected?(project, environment_name) ) ServiceResponse.success(message: _('Service account generated successfully'), payload: { @@ -50,11 +50,6 @@ module GoogleCloud def service_account_desc "GitLab generated service account for project '#{project.name}' and environment '#{environment_name}'" end - - # Overridden in EE - def environment_protected? - false - end end end diff --git a/app/services/google_cloud/gcp_region_add_or_replace_service.rb b/app/services/google_cloud/gcp_region_add_or_replace_service.rb new file mode 100644 index 00000000000..467f818bcc7 --- /dev/null +++ b/app/services/google_cloud/gcp_region_add_or_replace_service.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module GoogleCloud + class GcpRegionAddOrReplaceService < ::BaseService + def execute(environment, region) + gcp_region_key = Projects::GoogleCloudController::GCP_REGION_CI_VAR_KEY + + change_params = { variable_params: { key: gcp_region_key, value: region, environment_scope: environment } } + filter_params = { key: gcp_region_key, filter: { environment_scope: environment } } + + existing_variable = ::Ci::VariablesFinder.new(project, filter_params).execute.first + + if existing_variable + change_params[:action] = :update + change_params[:variable] = existing_variable + else + change_params[:action] = :create + end + + ::Ci::ChangeVariableService.new(container: project, current_user: current_user, params: change_params).execute + end + end +end diff --git a/app/services/google_cloud/service_accounts_service.rb b/app/services/google_cloud/service_accounts_service.rb index 3014daf08e2..b791f07cd65 100644 --- a/app/services/google_cloud/service_accounts_service.rb +++ b/app/services/google_cloud/service_accounts_service.rb @@ -14,12 +14,12 @@ module GoogleCloud # # This method looks up GitLab project's CI vars # and returns Google Cloud Service Accounts combinations - # aligning GitLab project and environment to GCP projects + # aligning GitLab project and ref to GCP projects def find_for_project - group_vars_by_environment.map do |environment_scope, value| + group_vars_by_ref.map do |environment_scope, value| { - environment: environment_scope, + ref: environment_scope, gcp_project: value['GCP_PROJECT_ID'], service_account_exists: value['GCP_SERVICE_ACCOUNT'].present?, service_account_key_exists: value['GCP_SERVICE_ACCOUNT_KEY'].present? @@ -27,21 +27,21 @@ module GoogleCloud end end - def add_for_project(environment, gcp_project_id, service_account, service_account_key, is_protected) + def add_for_project(ref, gcp_project_id, service_account, service_account_key, is_protected) project_var_create_or_replace( - environment, + ref, 'GCP_PROJECT_ID', gcp_project_id, is_protected ) project_var_create_or_replace( - environment, + ref, 'GCP_SERVICE_ACCOUNT', service_account, is_protected ) project_var_create_or_replace( - environment, + ref, 'GCP_SERVICE_ACCOUNT_KEY', service_account_key, is_protected @@ -50,7 +50,7 @@ module GoogleCloud private - def group_vars_by_environment + def group_vars_by_ref filtered_vars = project.variables.filter { |variable| GCP_KEYS.include? variable.key } filtered_vars.each_with_object({}) do |variable, grouped| grouped[variable.environment_scope] ||= {} @@ -59,10 +59,19 @@ module GoogleCloud end def project_var_create_or_replace(environment_scope, key, value, is_protected) - params = { key: key, filter: { environment_scope: environment_scope } } - existing_variable = ::Ci::VariablesFinder.new(project, params).execute.first - existing_variable.destroy if existing_variable - project.variables.create!(key: key, value: value, environment_scope: environment_scope, protected: is_protected) + change_params = { variable_params: { key: key, value: value, environment_scope: environment_scope, protected: is_protected } } + filter_params = { key: key, filter: { environment_scope: environment_scope } } + + existing_variable = ::Ci::VariablesFinder.new(project, filter_params).execute.first + + if existing_variable + change_params[:action] = :update + change_params[:variable] = existing_variable + else + change_params[:action] = :create + end + + ::Ci::ChangeVariableService.new(container: project, current_user: current_user, params: change_params).execute end end end diff --git a/app/services/groups/deploy_tokens/create_service.rb b/app/services/groups/deploy_tokens/create_service.rb index aee423659ef..4b0541e78a1 100644 --- a/app/services/groups/deploy_tokens/create_service.rb +++ b/app/services/groups/deploy_tokens/create_service.rb @@ -13,3 +13,5 @@ module Groups end end end + +Groups::DeployTokens::CreateService.prepend_mod diff --git a/app/services/groups/deploy_tokens/destroy_service.rb b/app/services/groups/deploy_tokens/destroy_service.rb index 6dae22f29d2..4745d00ed7f 100644 --- a/app/services/groups/deploy_tokens/destroy_service.rb +++ b/app/services/groups/deploy_tokens/destroy_service.rb @@ -11,3 +11,5 @@ module Groups end end end + +Groups::DeployTokens::DestroyService.prepend_mod diff --git a/app/services/groups/deploy_tokens/revoke_service.rb b/app/services/groups/deploy_tokens/revoke_service.rb new file mode 100644 index 00000000000..cf91d3b27fa --- /dev/null +++ b/app/services/groups/deploy_tokens/revoke_service.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module Groups + module DeployTokens + class RevokeService < BaseService + attr_accessor :token + + def execute + @token = group.deploy_tokens.find(params[:id]) + @token.revoke! + end + end + end +end + +Groups::DeployTokens::RevokeService.prepend_mod diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb index 5ffa746e109..c88c139a22e 100644 --- a/app/services/groups/destroy_service.rb +++ b/app/services/groups/destroy_service.rb @@ -11,11 +11,15 @@ module Groups # rubocop: disable CodeReuse/ActiveRecord def execute + # TODO - add a policy check here https://gitlab.com/gitlab-org/gitlab/-/issues/353082 + raise DestroyError, "You can't delete this group because you're blocked." if current_user.blocked? + group.prepare_for_destroy group.projects.includes(:project_feature).each do |project| # Execute the destruction of the models immediately to ensure atomic cleanup. success = ::Projects::DestroyService.new(project, current_user).execute + raise DestroyError, "Project #{project.id} can't be deleted" unless success end diff --git a/app/services/import/gitlab_projects/create_project_from_remote_file_service.rb b/app/services/import/gitlab_projects/create_project_from_remote_file_service.rb deleted file mode 100644 index edb9dc8ad91..00000000000 --- a/app/services/import/gitlab_projects/create_project_from_remote_file_service.rb +++ /dev/null @@ -1,91 +0,0 @@ -# frozen_string_literal: true - -module Import - module GitlabProjects - class CreateProjectFromRemoteFileService < CreateProjectFromUploadedFileService - FILE_SIZE_LIMIT = 10.gigabytes - ALLOWED_CONTENT_TYPES = [ - 'application/gzip', # most common content-type when fetching a tar.gz - 'application/x-tar' # aws-s3 uses x-tar for tar.gz files - ].freeze - - validate :valid_remote_import_url? - validate :validate_file_size - validate :validate_content_type - - private - - def required_params - [:path, :namespace, :remote_import_url] - end - - def project_params - super - .except(:file) - .merge(import_export_upload: ::ImportExportUpload.new( - remote_import_url: params[:remote_import_url] - )) - end - - def valid_remote_import_url? - ::Gitlab::UrlBlocker.validate!( - params[:remote_import_url], - allow_localhost: allow_local_requests?, - allow_local_network: allow_local_requests?, - schemes: %w(http https) - ) - - true - rescue ::Gitlab::UrlBlocker::BlockedUrlError => e - errors.add(:base, e.message) - - false - end - - def allow_local_requests? - ::Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? - end - - def validate_content_type - # AWS-S3 presigned URLs don't respond to HTTP HEAD requests, - # so file type cannot be validated - # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/75170#note_748059103 - return if amazon_s3? - - if headers['content-type'].blank? - errors.add(:base, "Missing 'ContentType' header") - elsif !ALLOWED_CONTENT_TYPES.include?(headers['content-type']) - errors.add(:base, "Remote file content type '%{content_type}' not allowed. (Allowed content types: %{allowed})" % { - content_type: headers['content-type'], - allowed: ALLOWED_CONTENT_TYPES.join(', ') - }) - end - end - - def validate_file_size - # AWS-S3 presigned URLs don't respond to HTTP HEAD requests, - # so file size cannot be validated - # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/75170#note_748059103 - return if amazon_s3? - - if headers['content-length'].to_i == 0 - errors.add(:base, "Missing 'ContentLength' header") - elsif headers['content-length'].to_i > FILE_SIZE_LIMIT - errors.add(:base, 'Remote file larger than limit. (limit %{limit})' % { - limit: ActiveSupport::NumberHelper.number_to_human_size(FILE_SIZE_LIMIT) - }) - end - end - - def amazon_s3? - headers['Server'] == 'AmazonS3' && headers['x-amz-request-id'].present? - end - - def headers - return {} if params[:remote_import_url].blank? || !valid_remote_import_url? - - @headers ||= Gitlab::HTTP.head(params[:remote_import_url]).headers - end - end - end -end diff --git a/app/services/import/gitlab_projects/create_project_from_uploaded_file_service.rb b/app/services/import/gitlab_projects/create_project_from_uploaded_file_service.rb deleted file mode 100644 index 35d52a11288..00000000000 --- a/app/services/import/gitlab_projects/create_project_from_uploaded_file_service.rb +++ /dev/null @@ -1,65 +0,0 @@ -# frozen_string_literal: true - -module Import - module GitlabProjects - class CreateProjectFromUploadedFileService - include ActiveModel::Validations - include ::Services::ReturnServiceResponses - - validate :required_params_presence - - def initialize(current_user, params = {}) - @current_user = current_user - @params = params.dup - end - - def execute - return error(errors.full_messages.first) unless valid? - return error(project.errors.full_messages&.first) unless project.saved? - - success(project) - rescue StandardError => e - error(e.message) - end - - private - - attr_reader :current_user, :params - - def error(message) - super(message, :bad_request) - end - - def project - @project ||= ::Projects::GitlabProjectsImportService.new( - current_user, - project_params, - params[:override] - ).execute - end - - def project_params - { - name: params[:name], - path: params[:path], - namespace_id: params[:namespace].id, - file: params[:file], - overwrite: params[:overwrite], - import_type: 'gitlab_project' - } - end - - def required_params - [:path, :namespace, :file] - end - - def required_params_presence - required_params - .select { |key| params[key].blank? } - .each do |missing_parameter| - errors.add(:base, "Parameter '#{missing_parameter}' is required") - end - end - end - end -end diff --git a/app/services/import/gitlab_projects/create_project_service.rb b/app/services/import/gitlab_projects/create_project_service.rb new file mode 100644 index 00000000000..1613c4dde25 --- /dev/null +++ b/app/services/import/gitlab_projects/create_project_service.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +# Creates a new project with an associated project export file to be imported +# The associated project export file might be associated with different strategies +# to acquire the file to be imported, the default file_acquisition_strategy +# is uploading a file (Import::GitlabProjects::FileAcquisitionStrategies::FileUpload) +module Import + module GitlabProjects + class CreateProjectService + include ActiveModel::Validations + include ::Services::ReturnServiceResponses + + validates_presence_of :path, :namespace + + # Creates a new CreateProjectService. + # + # @param [User] current_user + # @param [Hash] :params + # @param [Import::GitlabProjects::FileAcquisitionStrategies::*] :file_acquisition_strategy + def initialize(current_user, params:, file_acquisition_strategy: FileAcquisitionStrategies::FileUpload) + @current_user = current_user + @params = params.dup + @strategy = file_acquisition_strategy.new(current_user: current_user, params: params) + end + + # Creates a project with the strategy parameters + # + # @return [Services::ServiceReponse] + def execute + return error(errors.full_messages) unless valid? + return error(project.errors.full_messages) unless project.saved? + + success(project) + rescue StandardError => e + error(e.message) + end + + # Cascade the validation to strategy + def valid? + super && strategy.valid? + end + + # Merge with strategy's errors + def errors + super.tap { _1.merge!(strategy.errors) } + end + + def read_attribute_for_validation(key) + params[key] + end + + private + + attr_reader :current_user, :params, :strategy + + def error(messages) + messages = Array.wrap(messages) + message = messages.shift + super(message, :bad_request, pass_back: { other_errors: messages }) + end + + def project + @project ||= ::Projects::GitlabProjectsImportService.new( + current_user, + project_params, + params[:override] + ).execute + end + + def project_params + { + name: params[:name], + path: params[:path], + namespace_id: params[:namespace].id, + overwrite: params[:overwrite], + import_type: 'gitlab_project' + }.merge(strategy.project_params) + end + end + end +end diff --git a/app/services/import/gitlab_projects/file_acquisition_strategies/file_upload.rb b/app/services/import/gitlab_projects/file_acquisition_strategies/file_upload.rb new file mode 100644 index 00000000000..8bee3067d6c --- /dev/null +++ b/app/services/import/gitlab_projects/file_acquisition_strategies/file_upload.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Import + module GitlabProjects + module FileAcquisitionStrategies + class FileUpload + include ActiveModel::Validations + + validate :uploaded_file + + def initialize(current_user: nil, params:) + @params = params + end + + def project_params + @project_params ||= @params.slice(:file) + end + + def file + @file ||= @params[:file] + end + + private + + def uploaded_file + return if file.present? && file.is_a?(UploadedFile) + + errors.add(:file, 'must be uploaded') + end + end + end + end +end diff --git a/app/services/import/gitlab_projects/file_acquisition_strategies/remote_file.rb b/app/services/import/gitlab_projects/file_acquisition_strategies/remote_file.rb new file mode 100644 index 00000000000..ae9a450660c --- /dev/null +++ b/app/services/import/gitlab_projects/file_acquisition_strategies/remote_file.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +module Import + module GitlabProjects + module FileAcquisitionStrategies + class RemoteFile + include ActiveModel::Validations + + def self.allow_local_requests? + ::Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? + end + + validates :file_url, addressable_url: { + schemes: %w(https), + allow_localhost: allow_local_requests?, + allow_local_network: allow_local_requests?, + dns_rebind_protection: true + } + validate :aws_s3, if: :validate_aws_s3? + # When removing the import_project_from_remote_file_s3 remove the + # whole condition of this validation: + validates_with RemoteFileValidator, if: -> { validate_aws_s3? || !s3_request? } + + def initialize(current_user: nil, params:) + @params = params + end + + def project_params + @project_parms ||= { + import_export_upload: ::ImportExportUpload.new(remote_import_url: file_url) + } + end + + def file_url + @file_url ||= params[:remote_import_url] + end + + def content_type + @content_type ||= headers['content-type'] + end + + def content_length + @content_length ||= headers['content-length'].to_i + end + + private + + attr_reader :params + + def aws_s3 + if s3_request? + errors.add(:base, 'To import from AWS S3 use `projects/remote-import-s3`') + end + end + + def s3_request? + headers['Server'] == 'AmazonS3' && headers['x-amz-request-id'].present? + end + + def validate_aws_s3? + ::Feature.enabled?(:import_project_from_remote_file_s3, default_enabled: :yaml) + end + + def headers + return {} if file_url.blank? + + @headers ||= Gitlab::HTTP.head(file_url, timeout: 1.second).headers + rescue StandardError => e + errors.add(:base, "Failed to retrive headers: #{e.message}") + + {} + end + end + end + end +end diff --git a/app/services/import/gitlab_projects/file_acquisition_strategies/remote_file_s3.rb b/app/services/import/gitlab_projects/file_acquisition_strategies/remote_file_s3.rb new file mode 100644 index 00000000000..5cbca53582d --- /dev/null +++ b/app/services/import/gitlab_projects/file_acquisition_strategies/remote_file_s3.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +module Import + module GitlabProjects + module FileAcquisitionStrategies + class RemoteFileS3 + include ActiveModel::Validations + include Gitlab::Utils::StrongMemoize + + def self.allow_local_requests? + ::Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services? + end + + validates_presence_of :region, :bucket_name, :file_key, :access_key_id, :secret_access_key + validates :file_url, addressable_url: { + schemes: %w(https), + allow_localhost: allow_local_requests?, + allow_local_network: allow_local_requests?, + dns_rebind_protection: true + } + + validates_with RemoteFileValidator + + # The import itself has a limit of 24h, since the URL is created before the import starts + # we add an expiration a bit longer to ensure it won't expire during the import. + URL_EXPIRATION = 28.hours.seconds + + def initialize(current_user: nil, params:) + @params = params + end + + def project_params + @project_parms ||= { + import_export_upload: ::ImportExportUpload.new(remote_import_url: file_url) + } + end + + def file_url + @file_url ||= s3_object&.presigned_url(:get, expires_in: URL_EXPIRATION.to_i) + end + + def content_type + @content_type ||= s3_object&.content_type + end + + def content_length + @content_length ||= s3_object&.content_length.to_i + end + + # Make the validated params/methods accessible + def read_attribute_for_validation(key) + return file_url if key == :file_url + + params[key] + end + + private + + attr_reader :params + + def s3_object + strong_memoize(:s3_object) do + build_s3_options + end + end + + def build_s3_options + object = Aws::S3::Object.new( + params[:bucket_name], + params[:file_key], + client: Aws::S3::Client.new( + region: params[:region], + access_key_id: params[:access_key_id], + secret_access_key: params[:secret_access_key] + ) + ) + + # Force validate if the object exists and is accessible + # Some exceptions are only raised when trying to access the object data + unless object.exists? + errors.add(:base, "File not found '#{params[:file_key]}' in '#{params[:bucket_name]}'") + return + end + + object + rescue StandardError => e + errors.add(:base, "Failed to open '#{params[:file_key]}' in '#{params[:bucket_name]}': #{e.message}") + nil + end + end + end + end +end diff --git a/app/services/incident_management/pager_duty/process_webhook_service.rb b/app/services/incident_management/pager_duty/process_webhook_service.rb index ccbca671b37..a49e639ea62 100644 --- a/app/services/incident_management/pager_duty/process_webhook_service.rb +++ b/app/services/incident_management/pager_duty/process_webhook_service.rb @@ -2,7 +2,7 @@ module IncidentManagement module PagerDuty - class ProcessWebhookService + class ProcessWebhookService < ::BaseProjectService include Gitlab::Utils::StrongMemoize include IncidentManagement::Settings @@ -13,7 +13,8 @@ module IncidentManagement PAGER_DUTY_PROCESSABLE_EVENT_TYPES = %w(incident.trigger).freeze def initialize(project, payload) - @project = project + super(project: project) + @payload = payload end @@ -29,7 +30,7 @@ module IncidentManagement private - attr_reader :project, :payload + attr_reader :payload def process_incidents pager_duty_processable_events.each do |event| diff --git a/app/services/integrations/propagate_template_service.rb b/app/services/integrations/propagate_template_service.rb deleted file mode 100644 index 85a82ba4c8e..00000000000 --- a/app/services/integrations/propagate_template_service.rb +++ /dev/null @@ -1,10 +0,0 @@ -# frozen_string_literal: true - -module Integrations - # TODO: Remove this as part of https://gitlab.com/gitlab-org/gitlab/-/issues/335178 - class PropagateTemplateService - def self.propagate(_integration) - # no-op - end - end -end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 95093b88155..a63c54df4a6 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -160,7 +160,7 @@ class IssuableBaseService < ::BaseProjectService params.delete(:escalation_status) ).execute - return unless result.success? && result.payload.present? + return unless result.success? && result[:escalation_status].present? @escalation_status_change_reason = result[:escalation_status].delete(:status_change_reason) @@ -486,7 +486,10 @@ class IssuableBaseService < ::BaseProjectService associations[:description] = issuable.description associations[:reviewers] = issuable.reviewers.to_a if issuable.allows_reviewers? associations[:severity] = issuable.severity if issuable.supports_severity? - associations[:escalation_status] = issuable.escalation_status&.slice(:status, :policy_id) if issuable.supports_escalation? + + if issuable.supports_escalation? && issuable.escalation_status + associations[:escalation_status] = issuable.escalation_status.status_name + end associations end diff --git a/app/services/issuable_links/create_service.rb b/app/services/issuable_links/create_service.rb index 81685f81afa..802260c8fae 100644 --- a/app/services/issuable_links/create_service.rb +++ b/app/services/issuable_links/create_service.rb @@ -17,7 +17,7 @@ module IssuableLinks # otherwise create issue links for the issues which # are still not assigned and return success message. if render_conflict_error? - return error(issuables_assigned_message, 409) + return error(issuables_already_assigned_message, 409) end if render_not_found_error? @@ -36,6 +36,20 @@ module IssuableLinks success end + # rubocop: disable CodeReuse/ActiveRecord + def relate_issuables(referenced_issuable) + link = link_class.find_or_initialize_by(source: issuable, target: referenced_issuable) + + set_link_type(link) + + if link.changed? && link.save + create_notes(referenced_issuable) + end + + link + end + # rubocop: enable CodeReuse/ActiveRecord + private def render_conflict_error? @@ -96,6 +110,23 @@ module IssuableLinks {} end + def issuables_already_assigned_message + _('%{issuable}(s) already assigned' % { issuable: target_issuable_type.capitalize }) + end + + def issuables_not_found_message + _('No matching %{issuable} found. Make sure that you are adding a valid %{issuable} URL.' % { issuable: target_issuable_type }) + end + + def target_issuable_type + :issue + end + + def create_notes(referenced_issuable) + SystemNoteService.relate_issuable(issuable, referenced_issuable, current_user) + SystemNoteService.relate_issuable(referenced_issuable, issuable, current_user) + end + def linkable_issuables(objects) raise NotImplementedError end @@ -104,16 +135,12 @@ module IssuableLinks raise NotImplementedError end - def relate_issuables(referenced_object) + def link_class raise NotImplementedError end - def issuables_assigned_message - _("Issue(s) already assigned") - end - - def issuables_not_found_message - _("No matching issue found. Make sure that you are adding a valid issue URL.") + def set_link_type(_link) + # no-op end end end diff --git a/app/services/issuable_links/destroy_service.rb b/app/services/issuable_links/destroy_service.rb index 28035bbb291..19edd008b0a 100644 --- a/app/services/issuable_links/destroy_service.rb +++ b/app/services/issuable_links/destroy_service.rb @@ -4,11 +4,13 @@ module IssuableLinks class DestroyService < BaseService include IncidentManagement::UsageData - attr_reader :link, :current_user + attr_reader :link, :current_user, :source, :target def initialize(link, user) @link = link @current_user = user + @source = link.source + @target = link.target end def execute @@ -22,6 +24,11 @@ module IssuableLinks private + def create_notes + SystemNoteService.unrelate_issuable(source, target, current_user) + SystemNoteService.unrelate_issuable(target, source, current_user) + end + def after_destroy create_notes track_event diff --git a/app/services/issue_links/create_service.rb b/app/services/issue_links/create_service.rb index a022d3e0bcf..1c6621ce0a1 100644 --- a/app/services/issue_links/create_service.rb +++ b/app/services/issue_links/create_service.rb @@ -2,44 +2,25 @@ module IssueLinks class CreateService < IssuableLinks::CreateService - # rubocop: disable CodeReuse/ActiveRecord - def relate_issuables(referenced_issue) - link = IssueLink.find_or_initialize_by(source: issuable, target: referenced_issue) - - set_link_type(link) - - if link.changed? && link.save - create_notes(referenced_issue) - end - - link - end - # rubocop: enable CodeReuse/ActiveRecord - def linkable_issuables(issues) @linkable_issuables ||= begin issues.select { |issue| can?(current_user, :admin_issue_link, issue) } end end - def create_notes(referenced_issue) - SystemNoteService.relate_issue(issuable, referenced_issue, current_user) - SystemNoteService.relate_issue(referenced_issue, issuable, current_user) - end - def previous_related_issuables @related_issues ||= issuable.related_issues(current_user).to_a end private - def set_link_type(_link) - # EE only - end - def track_event track_incident_action(current_user, issuable, :incident_relate) end + + def link_class + IssueLink + end end end diff --git a/app/services/issue_links/destroy_service.rb b/app/services/issue_links/destroy_service.rb index 25a45fc697b..e2422ecaca9 100644 --- a/app/services/issue_links/destroy_service.rb +++ b/app/services/issue_links/destroy_service.rb @@ -4,23 +4,10 @@ module IssueLinks class DestroyService < IssuableLinks::DestroyService private - def source - @source ||= link.source - end - - def target - @target ||= link.target - end - def permission_to_remove_relation? can?(current_user, :admin_issue_link, source) && can?(current_user, :admin_issue_link, target) end - def create_notes - SystemNoteService.unrelate_issue(source, target, current_user) - SystemNoteService.unrelate_issue(target, source, current_user) - end - def track_event track_incident_action(current_user, target, :incident_unrelate) end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 7fbf7c6af58..7ab663718db 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -23,6 +23,7 @@ module Issues handle_move_between_ids(@issue) + @add_related_issue ||= params.delete(:add_related_issue) filter_resolve_discussion_params create(@issue, skip_system_notes: skip_system_notes) @@ -52,6 +53,7 @@ module Issues # Add new items to Issues::AfterCreateService if they can be performed in Sidekiq def after_create(issue) user_agent_detail_service.create + handle_add_related_issue(issue) resolve_discussions_with_issue(issue) create_escalation_status(issue) @@ -91,6 +93,12 @@ module Issues def user_agent_detail_service UserAgentDetailService.new(spammable: @issue, spam_params: spam_params) end + + def handle_add_related_issue(issue) + return unless @add_related_issue + + IssueLinks::CreateService.new(issue, issue.author, { target_issuable: @add_related_issue }).execute + end end end diff --git a/app/services/issues/export_csv_service.rb b/app/services/issues/export_csv_service.rb index 3809d8bc347..7076e858155 100644 --- a/app/services/issues/export_csv_service.rb +++ b/app/services/issues/export_csv_service.rb @@ -23,11 +23,11 @@ module Issues def header_to_value_hash { + 'Title' => 'title', + 'Description' => 'description', 'Issue ID' => 'iid', 'URL' => -> (issue) { issue_url(issue) }, - 'Title' => 'title', 'State' => -> (issue) { issue.closed? ? 'Closed' : 'Open' }, - 'Description' => 'description', 'Author' => 'author_name', 'Author Username' => -> (issue) { issue.author&.username }, 'Assignee' => -> (issue) { issue.assignees.map(&:name).join(', ') }, @@ -52,7 +52,7 @@ module Issues # rubocop: disable CodeReuse/ActiveRecord def issue_time_spent(issue) - issue.timelogs.map(&:time_spent).sum + issue.timelogs.sum(&:time_spent) end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/app/services/issues/set_crm_contacts_service.rb b/app/services/issues/set_crm_contacts_service.rb index 2edc944435b..5836097f1fd 100644 --- a/app/services/issues/set_crm_contacts_service.rb +++ b/app/services/issues/set_crm_contacts_service.rb @@ -52,7 +52,7 @@ module Issues end def add_by_email - contact_ids = ::CustomerRelations::Contact.find_ids_by_emails(project_group, emails(:add_emails)) + contact_ids = ::CustomerRelations::Contact.find_ids_by_emails(project_group.root_ancestor, emails(:add_emails)) add_by_id(contact_ids) end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 8372cd919e5..88c4ff1a8bb 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -51,7 +51,6 @@ module Issues old_mentioned_users = old_associations.fetch(:mentioned_users, []) old_assignees = old_associations.fetch(:assignees, []) old_severity = old_associations[:severity] - old_escalation_status = old_associations[:escalation_status] if has_changes?(issue, old_labels: old_labels, old_assignees: old_assignees) todo_service.resolve_todos_for_target(issue, current_user) @@ -68,7 +67,7 @@ module Issues handle_milestone_change(issue) handle_added_mentions(issue, old_mentioned_users) handle_severity_change(issue, old_severity) - handle_escalation_status_change(issue, old_escalation_status) + handle_escalation_status_change(issue) handle_issue_type_change(issue) end @@ -80,9 +79,7 @@ module Issues todo_service.reassigned_assignable(issue, current_user, old_assignees) track_incident_action(current_user, issue, :incident_assigned) - if Feature.enabled?(:broadcast_issue_updates, issue.project, default_enabled: :yaml) - GraphqlTriggers.issuable_assignees_updated(issue) - end + GraphqlTriggers.issuable_assignees_updated(issue) end def handle_task_changes(issuable) @@ -196,9 +193,8 @@ module Issues ::IncidentManagement::AddSeveritySystemNoteWorker.perform_async(issue.id, current_user.id) end - def handle_escalation_status_change(issue, old_escalation_status) - return unless old_escalation_status.present? - return if issue.escalation_status&.slice(:status, :policy_id) == old_escalation_status + def handle_escalation_status_change(issue) + return unless issue.supports_escalation? && issue.escalation_status ::IncidentManagement::IssuableEscalationStatuses::AfterUpdateService.new( issue, diff --git a/app/services/labels/base_service.rb b/app/services/labels/base_service.rb index ead7f2ea607..f694e6d47a0 100644 --- a/app/services/labels/base_service.rb +++ b/app/services/labels/base_service.rb @@ -2,162 +2,8 @@ module Labels class BaseService < ::BaseService - COLOR_NAME_TO_HEX = { - black: '#000000', - silver: '#C0C0C0', - gray: '#808080', - white: '#FFFFFF', - maroon: '#800000', - red: '#FF0000', - purple: '#800080', - fuchsia: '#FF00FF', - green: '#008000', - lime: '#00FF00', - olive: '#808000', - yellow: '#FFFF00', - navy: '#000080', - blue: '#0000FF', - teal: '#008080', - aqua: '#00FFFF', - orange: '#FFA500', - aliceblue: '#F0F8FF', - antiquewhite: '#FAEBD7', - aquamarine: '#7FFFD4', - azure: '#F0FFFF', - beige: '#F5F5DC', - bisque: '#FFE4C4', - blanchedalmond: '#FFEBCD', - blueviolet: '#8A2BE2', - brown: '#A52A2A', - burlywood: '#DEB887', - cadetblue: '#5F9EA0', - chartreuse: '#7FFF00', - chocolate: '#D2691E', - coral: '#FF7F50', - cornflowerblue: '#6495ED', - cornsilk: '#FFF8DC', - crimson: '#DC143C', - darkblue: '#00008B', - darkcyan: '#008B8B', - darkgoldenrod: '#B8860B', - darkgray: '#A9A9A9', - darkgreen: '#006400', - darkgrey: '#A9A9A9', - darkkhaki: '#BDB76B', - darkmagenta: '#8B008B', - darkolivegreen: '#556B2F', - darkorange: '#FF8C00', - darkorchid: '#9932CC', - darkred: '#8B0000', - darksalmon: '#E9967A', - darkseagreen: '#8FBC8F', - darkslateblue: '#483D8B', - darkslategray: '#2F4F4F', - darkslategrey: '#2F4F4F', - darkturquoise: '#00CED1', - darkviolet: '#9400D3', - deeppink: '#FF1493', - deepskyblue: '#00BFFF', - dimgray: '#696969', - dimgrey: '#696969', - dodgerblue: '#1E90FF', - firebrick: '#B22222', - floralwhite: '#FFFAF0', - forestgreen: '#228B22', - gainsboro: '#DCDCDC', - ghostwhite: '#F8F8FF', - gold: '#FFD700', - goldenrod: '#DAA520', - greenyellow: '#ADFF2F', - grey: '#808080', - honeydew: '#F0FFF0', - hotpink: '#FF69B4', - indianred: '#CD5C5C', - indigo: '#4B0082', - ivory: '#FFFFF0', - khaki: '#F0E68C', - lavender: '#E6E6FA', - lavenderblush: '#FFF0F5', - lawngreen: '#7CFC00', - lemonchiffon: '#FFFACD', - lightblue: '#ADD8E6', - lightcoral: '#F08080', - lightcyan: '#E0FFFF', - lightgoldenrodyellow: '#FAFAD2', - lightgray: '#D3D3D3', - lightgreen: '#90EE90', - lightgrey: '#D3D3D3', - lightpink: '#FFB6C1', - lightsalmon: '#FFA07A', - lightseagreen: '#20B2AA', - lightskyblue: '#87CEFA', - lightslategray: '#778899', - lightslategrey: '#778899', - lightsteelblue: '#B0C4DE', - lightyellow: '#FFFFE0', - limegreen: '#32CD32', - linen: '#FAF0E6', - mediumaquamarine: '#66CDAA', - mediumblue: '#0000CD', - mediumorchid: '#BA55D3', - mediumpurple: '#9370DB', - mediumseagreen: '#3CB371', - mediumslateblue: '#7B68EE', - mediumspringgreen: '#00FA9A', - mediumturquoise: '#48D1CC', - mediumvioletred: '#C71585', - midnightblue: '#191970', - mintcream: '#F5FFFA', - mistyrose: '#FFE4E1', - moccasin: '#FFE4B5', - navajowhite: '#FFDEAD', - oldlace: '#FDF5E6', - olivedrab: '#6B8E23', - orangered: '#FF4500', - orchid: '#DA70D6', - palegoldenrod: '#EEE8AA', - palegreen: '#98FB98', - paleturquoise: '#AFEEEE', - palevioletred: '#DB7093', - papayawhip: '#FFEFD5', - peachpuff: '#FFDAB9', - peru: '#CD853F', - pink: '#FFC0CB', - plum: '#DDA0DD', - powderblue: '#B0E0E6', - rosybrown: '#BC8F8F', - royalblue: '#4169E1', - saddlebrown: '#8B4513', - salmon: '#FA8072', - sandybrown: '#F4A460', - seagreen: '#2E8B57', - seashell: '#FFF5EE', - sienna: '#A0522D', - skyblue: '#87CEEB', - slateblue: '#6A5ACD', - slategray: '#708090', - slategrey: '#708090', - snow: '#FFFAFA', - springgreen: '#00FF7F', - steelblue: '#4682B4', - tan: '#D2B48C', - thistle: '#D8BFD8', - tomato: '#FF6347', - turquoise: '#40E0D0', - violet: '#EE82EE', - wheat: '#F5DEB3', - whitesmoke: '#F5F5F5', - yellowgreen: '#9ACD32', - rebeccapurple: '#663399' - }.freeze - def convert_color_name_to_hex - color = params[:color] - color_name = color.strip.downcase - - return color if color_name.start_with?('#') - - COLOR_NAME_TO_HEX[color_name.to_sym] || color + ::Gitlab::Color.of(params[:color]) end end end diff --git a/app/services/loose_foreign_keys/batch_cleaner_service.rb b/app/services/loose_foreign_keys/batch_cleaner_service.rb index f3db2037911..b89de15a568 100644 --- a/app/services/loose_foreign_keys/batch_cleaner_service.rb +++ b/app/services/loose_foreign_keys/batch_cleaner_service.rb @@ -54,7 +54,7 @@ module LooseForeignKeys attr_reader :parent_table, :loose_foreign_key_definitions, :deleted_parent_records, :modification_tracker, :deleted_records_counter, :deleted_records_rescheduled_count, :deleted_records_incremented_count def handle_over_limit - return if Feature.disabled?(:lfk_fair_queueing) + return if Feature.disabled?(:lfk_fair_queueing, default_enabled: :yaml) records_to_reschedule = [] records_to_increment = [] diff --git a/app/services/members/projects/creator_service.rb b/app/services/members/projects/creator_service.rb index 2e974177075..4dba81acf73 100644 --- a/app/services/members/projects/creator_service.rb +++ b/app/services/members/projects/creator_service.rb @@ -4,7 +4,7 @@ module Members module Projects class CreatorService < Members::CreatorService def self.access_levels - Gitlab::Access.sym_options + Gitlab::Access.sym_options_with_owner end private diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb index 3f39b2742c6..37c2676e51c 100644 --- a/app/services/merge_requests/approval_service.rb +++ b/app/services/merge_requests/approval_service.rb @@ -11,10 +11,11 @@ module MergeRequests reset_approvals_cache(merge_request) create_event(merge_request) + stream_audit_event(merge_request) create_approval_note(merge_request) mark_pending_todos_as_done(merge_request) execute_approval_hooks(merge_request, current_user) - remove_attention_requested(merge_request, current_user) + remove_attention_requested(merge_request) merge_request_activity_counter.track_approve_mr_action(user: current_user) success @@ -52,6 +53,10 @@ module MergeRequests def create_event(merge_request) event_service.approve_mr(merge_request, current_user) end + + def stream_audit_event(merge_request) + # Defined in EE + end end end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 3363fc90997..2ab623bacf8 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -60,7 +60,9 @@ module MergeRequests merge_request_activity_counter.track_reviewers_changed_action(user: current_user) unless new_reviewers.include?(current_user) - remove_attention_requested(merge_request, current_user) + remove_attention_requested(merge_request) + + merge_request.merge_request_reviewers_with(new_reviewers).update_all(updated_state_by_user_id: current_user.id) end end @@ -251,10 +253,10 @@ module MergeRequests ::MergeRequests::BulkRemoveAttentionRequestedService.new(project: merge_request.project, current_user: current_user, merge_request: merge_request, users: users.uniq).execute end - def remove_attention_requested(merge_request, user) + def remove_attention_requested(merge_request) return unless merge_request.attention_requested_enabled? - ::MergeRequests::RemoveAttentionRequestedService.new(project: merge_request.project, current_user: current_user, merge_request: merge_request, user: user).execute + ::MergeRequests::RemoveAttentionRequestedService.new(project: merge_request.project, current_user: current_user, merge_request: merge_request).execute end end end diff --git a/app/services/merge_requests/bulk_remove_attention_requested_service.rb b/app/services/merge_requests/bulk_remove_attention_requested_service.rb index 6573b623779..774f2c2ee35 100644 --- a/app/services/merge_requests/bulk_remove_attention_requested_service.rb +++ b/app/services/merge_requests/bulk_remove_attention_requested_service.rb @@ -19,6 +19,8 @@ module MergeRequests merge_request.merge_request_assignees.where(user_id: users).update_all(state: :reviewed) merge_request.merge_request_reviewers.where(user_id: users).update_all(state: :reviewed) + users.each { |user| user.invalidate_attention_requested_count } + success end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index c1292d924b2..9c525ae8489 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -31,6 +31,14 @@ module MergeRequests private + def before_create(merge_request) + # If the fetching of the source branch occurs in an ActiveRecord + # callback (e.g. after_create), a database transaction will be + # open while the Gitaly RPC waits. To avoid an idle in transaction + # timeout, we do this before we attempt to save the merge request. + merge_request.eager_fetch_ref! + end + def set_projects! # @project is used to determine whether the user can set the merge request's # assignee, milestone and labels. Whether they can depends on their diff --git a/app/services/merge_requests/export_csv_service.rb b/app/services/merge_requests/export_csv_service.rb index 8f2a70575e5..1f8dec69ef0 100644 --- a/app/services/merge_requests/export_csv_service.rb +++ b/app/services/merge_requests/export_csv_service.rb @@ -13,11 +13,11 @@ module MergeRequests def header_to_value_hash { + 'Title' => 'title', + 'Description' => 'description', 'MR IID' => 'iid', 'URL' => -> (merge_request) { merge_request_url(merge_request) }, - 'Title' => 'title', 'State' => 'state', - 'Description' => 'description', 'Source Branch' => 'source_branch', 'Target Branch' => 'target_branch', 'Source Project ID' => 'source_project_id', diff --git a/app/services/merge_requests/handle_assignees_change_service.rb b/app/services/merge_requests/handle_assignees_change_service.rb index 97be9fe8d9f..a169a6dc0b6 100644 --- a/app/services/merge_requests/handle_assignees_change_service.rb +++ b/app/services/merge_requests/handle_assignees_change_service.rb @@ -21,10 +21,12 @@ module MergeRequests merge_request_activity_counter.track_users_assigned_to_mr(users: new_assignees) merge_request_activity_counter.track_assignees_changed_action(user: current_user) + merge_request.merge_request_assignees_with(new_assignees).update_all(updated_state_by_user_id: current_user.id) + execute_assignees_hooks(merge_request, old_assignees) if options[:execute_hooks] unless new_assignees.include?(current_user) - remove_attention_requested(merge_request, current_user) + remove_attention_requested(merge_request) end end diff --git a/app/services/merge_requests/merge_orchestration_service.rb b/app/services/merge_requests/merge_orchestration_service.rb index 24341ef1145..5f3d2174840 100644 --- a/app/services/merge_requests/merge_orchestration_service.rb +++ b/app/services/merge_requests/merge_orchestration_service.rb @@ -26,7 +26,7 @@ module MergeRequests def can_merge_immediately?(merge_request) merge_request.can_be_merged_by?(current_user) && - merge_request.mergeable_state? + merge_request.mergeable? end def can_merge_automatically?(merge_request) diff --git a/app/services/merge_requests/mergeability/check_broken_status_service.rb b/app/services/merge_requests/mergeability/check_broken_status_service.rb new file mode 100644 index 00000000000..9a54a4292c8 --- /dev/null +++ b/app/services/merge_requests/mergeability/check_broken_status_service.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true +module MergeRequests + module Mergeability + class CheckBrokenStatusService < CheckBaseService + def execute + if merge_request.broken? + failure + else + success + end + end + + def skip? + false + end + + def cacheable? + false + end + end + end +end diff --git a/app/services/merge_requests/mergeability/check_discussions_status_service.rb b/app/services/merge_requests/mergeability/check_discussions_status_service.rb new file mode 100644 index 00000000000..9b4eab9d399 --- /dev/null +++ b/app/services/merge_requests/mergeability/check_discussions_status_service.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true +module MergeRequests + module Mergeability + class CheckDiscussionsStatusService < CheckBaseService + def execute + if merge_request.mergeable_discussions_state? + success + else + failure + end + end + + def skip? + params[:skip_discussions_check].present? + end + + def cacheable? + false + end + end + end +end diff --git a/app/services/merge_requests/mergeability/check_draft_status_service.rb b/app/services/merge_requests/mergeability/check_draft_status_service.rb new file mode 100644 index 00000000000..bc940e2116d --- /dev/null +++ b/app/services/merge_requests/mergeability/check_draft_status_service.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module MergeRequests + module Mergeability + class CheckDraftStatusService < CheckBaseService + def execute + if merge_request.draft? + failure + else + success + end + end + + def skip? + false + end + + def cacheable? + false + end + end + end +end diff --git a/app/services/merge_requests/mergeability/check_open_status_service.rb b/app/services/merge_requests/mergeability/check_open_status_service.rb new file mode 100644 index 00000000000..361af946e3f --- /dev/null +++ b/app/services/merge_requests/mergeability/check_open_status_service.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module MergeRequests + module Mergeability + class CheckOpenStatusService < CheckBaseService + def execute + if merge_request.open? + success + else + failure + end + end + + def skip? + false + end + + def cacheable? + false + end + end + end +end diff --git a/app/services/merge_requests/mergeability/run_checks_service.rb b/app/services/merge_requests/mergeability/run_checks_service.rb index c1d65fb65cc..03c6d985c23 100644 --- a/app/services/merge_requests/mergeability/run_checks_service.rb +++ b/app/services/merge_requests/mergeability/run_checks_service.rb @@ -7,6 +7,10 @@ module MergeRequests # We want to have the cheapest checks first in the list, # that way we can fail fast before running the more expensive ones CHECKS = [ + CheckOpenStatusService, + CheckDraftStatusService, + CheckBrokenStatusService, + CheckDiscussionsStatusService, CheckCiStatusService ].freeze diff --git a/app/services/merge_requests/reload_merge_head_diff_service.rb b/app/services/merge_requests/reload_merge_head_diff_service.rb index f02a9bd3139..4724dd1c068 100644 --- a/app/services/merge_requests/reload_merge_head_diff_service.rb +++ b/app/services/merge_requests/reload_merge_head_diff_service.rb @@ -9,7 +9,6 @@ module MergeRequests 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 @@ -23,10 +22,6 @@ module MergeRequests attr_reader :merge_request - def enabled? - Feature.enabled?(:default_merge_ref_for_diffs, merge_request.project, default_enabled: :yaml) - end - def recreate_merge_head_diff merge_request.merge_head_diff&.destroy! diff --git a/app/services/merge_requests/remove_approval_service.rb b/app/services/merge_requests/remove_approval_service.rb index 198a21884b8..c7bc3532264 100644 --- a/app/services/merge_requests/remove_approval_service.rb +++ b/app/services/merge_requests/remove_approval_service.rb @@ -17,7 +17,7 @@ module MergeRequests reset_approvals_cache(merge_request) create_note(merge_request) merge_request_activity_counter.track_unapprove_mr_action(user: current_user) - remove_attention_requested(merge_request, current_user) + remove_attention_requested(merge_request) end success diff --git a/app/services/merge_requests/remove_attention_requested_service.rb b/app/services/merge_requests/remove_attention_requested_service.rb index b727c24415e..a32a8071471 100644 --- a/app/services/merge_requests/remove_attention_requested_service.rb +++ b/app/services/merge_requests/remove_attention_requested_service.rb @@ -2,13 +2,12 @@ module MergeRequests class RemoveAttentionRequestedService < MergeRequests::BaseService - attr_accessor :merge_request, :user + attr_accessor :merge_request - def initialize(project:, current_user:, merge_request:, user:) + def initialize(project:, current_user:, merge_request:) super(project: project, current_user: current_user) @merge_request = merge_request - @user = user end def execute @@ -18,6 +17,8 @@ module MergeRequests update_state(reviewer) update_state(assignee) + current_user.invalidate_attention_requested_count + success else error("User is not a reviewer or assignee of the merge request") @@ -27,11 +28,11 @@ module MergeRequests private def assignee - merge_request.find_assignee(user) + merge_request.find_assignee(current_user) end def reviewer - merge_request.find_reviewer(user) + merge_request.find_reviewer(current_user) end def update_state(reviewer_or_assignee) diff --git a/app/services/merge_requests/reopen_service.rb b/app/services/merge_requests/reopen_service.rb index 35c50d63da0..4612688f78b 100644 --- a/app/services/merge_requests/reopen_service.rb +++ b/app/services/merge_requests/reopen_service.rb @@ -6,6 +6,8 @@ module MergeRequests return merge_request unless can?(current_user, :reopen_merge_request, merge_request) if merge_request.reopen + users = merge_request.assignees | merge_request.reviewers + create_event(merge_request) create_note(merge_request, 'reopened') merge_request_activity_counter.track_reopen_mr_action(user: current_user) @@ -13,11 +15,13 @@ module MergeRequests execute_hooks(merge_request, 'reopen') merge_request.reload_diff(current_user) merge_request.mark_as_unchecked - invalidate_cache_counts(merge_request, users: merge_request.assignees | merge_request.reviewers) + invalidate_cache_counts(merge_request, users: users) merge_request.update_project_counter_caches merge_request.cache_merge_request_closes_issues!(current_user) merge_request.cleanup_schedule&.destroy merge_request.update_column(:merge_ref_sha, nil) + + users.each { |user| user.invalidate_attention_requested_count } end merge_request diff --git a/app/services/merge_requests/toggle_attention_requested_service.rb b/app/services/merge_requests/toggle_attention_requested_service.rb index d9f81ac310f..64cdcd725a2 100644 --- a/app/services/merge_requests/toggle_attention_requested_service.rb +++ b/app/services/merge_requests/toggle_attention_requested_service.rb @@ -18,12 +18,14 @@ module MergeRequests update_state(reviewer) update_state(assignee) + user.invalidate_attention_requested_count + if reviewer&.attention_requested? || assignee&.attention_requested? create_attention_request_note notity_user if current_user.id != user.id - remove_attention_requested(merge_request, current_user) + remove_attention_requested(merge_request) end else create_remove_attention_request_note @@ -59,7 +61,8 @@ module MergeRequests end def update_state(reviewer_or_assignee) - reviewer_or_assignee&.update(state: reviewer_or_assignee&.attention_requested? ? :reviewed : :attention_requested) + reviewer_or_assignee&.update(state: reviewer_or_assignee&.attention_requested? ? :reviewed : :attention_requested, + updated_state_by: current_user) end end end diff --git a/app/services/notification_recipients/builder/merge_request_unmergeable.rb b/app/services/notification_recipients/builder/merge_request_unmergeable.rb index 24d96b98002..b9facf07a3a 100644 --- a/app/services/notification_recipients/builder/merge_request_unmergeable.rb +++ b/app/services/notification_recipients/builder/merge_request_unmergeable.rb @@ -4,6 +4,7 @@ module NotificationRecipients module Builder class MergeRequestUnmergeable < Base attr_reader :target + def initialize(merge_request) @target = merge_request end diff --git a/app/services/notification_recipients/builder/new_note.rb b/app/services/notification_recipients/builder/new_note.rb index 17e4728d352..dcf6d23298a 100644 --- a/app/services/notification_recipients/builder/new_note.rb +++ b/app/services/notification_recipients/builder/new_note.rb @@ -4,6 +4,7 @@ module NotificationRecipients module Builder class NewNote < Base attr_reader :note + def initialize(note) @note = note end diff --git a/app/services/notification_recipients/builder/new_review.rb b/app/services/notification_recipients/builder/new_review.rb index 3b1296f6967..84598c3d4ad 100644 --- a/app/services/notification_recipients/builder/new_review.rb +++ b/app/services/notification_recipients/builder/new_review.rb @@ -4,6 +4,7 @@ module NotificationRecipients module Builder class NewReview < Base attr_reader :review + def initialize(review) @review = review end diff --git a/app/services/notification_recipients/builder/project_maintainers.rb b/app/services/notification_recipients/builder/project_maintainers.rb index e8f22c00a83..a295929a1a9 100644 --- a/app/services/notification_recipients/builder/project_maintainers.rb +++ b/app/services/notification_recipients/builder/project_maintainers.rb @@ -14,6 +14,7 @@ module NotificationRecipients return [] unless project add_recipients(project.team.maintainers, :mention, nil) + add_recipients(project.team.owners, :mention, nil) end def acting_user diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 5b1733422d0..aa7e636b8a4 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -18,6 +18,7 @@ class NotificationService class Async attr_reader :parent + delegate :respond_to_missing, to: :parent def initialize(parent) @@ -64,6 +65,13 @@ class NotificationService end end + # Notify the owner of the account when a new personal access token is created + def access_token_created(user, token_name) + return unless user.can?(:receive_notifications) + + mailer.access_token_created_email(user, token_name).deliver_later + end + # Notify the owner of the personal access token, when it is about to expire # And mark the token with about_to_expire_delivered def access_token_about_to_expire(user, token_names) diff --git a/app/services/packages/pypi/create_package_service.rb b/app/services/packages/pypi/create_package_service.rb index b988c191734..5d7e967ceb0 100644 --- a/app/services/packages/pypi/create_package_service.rb +++ b/app/services/packages/pypi/create_package_service.rb @@ -9,7 +9,7 @@ module Packages ::Packages::Package.transaction do meta = Packages::Pypi::Metadatum.new( package: created_package, - required_python: params[:requires_python] + required_python: params[:requires_python] || '' ) unless meta.valid? diff --git a/app/services/personal_access_tokens/create_service.rb b/app/services/personal_access_tokens/create_service.rb index 7555ba26768..e2f2e220750 100644 --- a/app/services/personal_access_tokens/create_service.rb +++ b/app/services/personal_access_tokens/create_service.rb @@ -16,6 +16,7 @@ module PersonalAccessTokens if token.persisted? log_event(token) + notification_service.access_token_created(target_user, token.name) ServiceResponse.success(payload: { personal_access_token: token }) else ServiceResponse.error(message: token.errors.full_messages.to_sentence, payload: { personal_access_token: token }) diff --git a/app/services/post_receive_service.rb b/app/services/post_receive_service.rb index f5638b0aa40..15c978e6763 100644 --- a/app/services/post_receive_service.rb +++ b/app/services/post_receive_service.rb @@ -86,7 +86,7 @@ class PostReceiveService banner = nil if project - scoped_messages = BroadcastMessage.current_banner_messages(project.full_path).select do |message| + scoped_messages = BroadcastMessage.current_banner_messages(current_path: project.full_path).select do |message| message.target_path.present? && message.matches_current_path(project.full_path) end diff --git a/app/services/projects/base_move_relations_service.rb b/app/services/projects/base_move_relations_service.rb index 3a159cef58b..bd5a39d3b59 100644 --- a/app/services/projects/base_move_relations_service.rb +++ b/app/services/projects/base_move_relations_service.rb @@ -3,6 +3,7 @@ module Projects class BaseMoveRelationsService < BaseService attr_reader :source_project + def execute(source_project, remove_remaining_elements: true) return if source_project.blank? diff --git a/app/services/projects/container_repository/cleanup_tags_service.rb b/app/services/projects/container_repository/cleanup_tags_service.rb index 1a788abac12..72f3fddb4c3 100644 --- a/app/services/projects/container_repository/cleanup_tags_service.rb +++ b/app/services/projects/container_repository/cleanup_tags_service.rb @@ -145,12 +145,14 @@ module Projects end def caching_enabled? - container_expiration_policy && - older_than.present? + result = ::Gitlab::CurrentSettings.current_application_settings.container_registry_expiration_policies_caching && + container_expiration_policy && + older_than.present? + !!result end def throttling_enabled? - Feature.enabled?(:container_registry_expiration_policies_throttling) + Feature.enabled?(:container_registry_expiration_policies_throttling, default_enabled: :yaml) end def max_list_size diff --git a/app/services/projects/container_repository/gitlab/delete_tags_service.rb b/app/services/projects/container_repository/gitlab/delete_tags_service.rb index 589aac5c3ac..f109cb0ca20 100644 --- a/app/services/projects/container_repository/gitlab/delete_tags_service.rb +++ b/app/services/projects/container_repository/gitlab/delete_tags_service.rb @@ -54,7 +54,7 @@ module Projects def throttling_enabled? strong_memoize(:feature_flag) do - Feature.enabled?(:container_registry_expiration_policies_throttling) + Feature.enabled?(:container_registry_expiration_policies_throttling, default_enabled: :yaml) end end diff --git a/app/services/projects/container_repository/third_party/delete_tags_service.rb b/app/services/projects/container_repository/third_party/delete_tags_service.rb index 404642acf72..4184c676fc3 100644 --- a/app/services/projects/container_repository/third_party/delete_tags_service.rb +++ b/app/services/projects/container_repository/third_party/delete_tags_service.rb @@ -41,14 +41,12 @@ module Projects # update the manifests of the tags with the new dummy image def replace_tag_manifests(dummy_manifest) - deleted_tags = {} - - @tag_names.each do |name| + deleted_tags = @tag_names.map do |name| digest = @container_repository.client.put_tag(@container_repository.path, name, dummy_manifest) next unless digest - deleted_tags[name] = digest - end + [name, digest] + end.compact.to_h # make sure the digests are the same (it should always be) digests = deleted_tags.values.uniq diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index c885369dfec..252e1d76bef 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -147,7 +147,7 @@ module Projects priority: UserProjectAccessChangedService::LOW_PRIORITY ) else - @project.add_maintainer(@project.namespace.owner, current_user: current_user) + @project.add_owner(@project.namespace.owner, current_user: current_user) end end diff --git a/app/services/projects/deploy_tokens/create_service.rb b/app/services/projects/deploy_tokens/create_service.rb index 592198ef241..2486544b150 100644 --- a/app/services/projects/deploy_tokens/create_service.rb +++ b/app/services/projects/deploy_tokens/create_service.rb @@ -13,3 +13,5 @@ module Projects end end end + +Projects::DeployTokens::CreateService.prepend_mod diff --git a/app/services/projects/deploy_tokens/destroy_service.rb b/app/services/projects/deploy_tokens/destroy_service.rb index e063f86a65c..7ac1b52b0af 100644 --- a/app/services/projects/deploy_tokens/destroy_service.rb +++ b/app/services/projects/deploy_tokens/destroy_service.rb @@ -11,3 +11,5 @@ module Projects end end end + +Projects::DeployTokens::DestroyService.prepend_mod diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 95af5a6863f..a73244c6971 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -37,7 +37,7 @@ module Projects system_hook_service.execute_hooks_for(project, :destroy) log_info("Project \"#{project.full_path}\" was deleted") - publish_project_deleted_event_for(project) if Feature.enabled?(:publish_project_deleted_event, default_enabled: :yaml) + publish_project_deleted_event_for(project) current_user.invalidate_personal_projects_count @@ -72,7 +72,13 @@ module Projects end def remove_snippets - response = ::Snippets::BulkDestroyService.new(current_user, project.snippets).execute + # We're setting the hard_delete param because we dont need to perform the access checks within the service since + # the user has enough access rights to remove the project and its resources. + response = ::Snippets::BulkDestroyService.new(current_user, project.snippets).execute(hard_delete: true) + + if response.error? + log_error("Snippet deletion failed on #{project.full_path} with the following message: #{response.message}") + end response.success? end @@ -194,6 +200,10 @@ module Projects ::Ci::DestroyPipelineService.new(project, current_user).execute(pipeline) end + project.secure_files.find_each(batch_size: BATCH_SIZE) do |secure_file| # rubocop: disable CodeReuse/ActiveRecord + ::Ci::DestroySecureFileService.new(project, current_user).execute(secure_file) + end + deleted_count = ::CommitStatus.for_project(project).delete_all Gitlab::AppLogger.info( diff --git a/app/services/projects/lfs_pointers/lfs_download_service.rb b/app/services/projects/lfs_pointers/lfs_download_service.rb index 9da72d9300e..76005a1c96e 100644 --- a/app/services/projects/lfs_pointers/lfs_download_service.rb +++ b/app/services/projects/lfs_pointers/lfs_download_service.rb @@ -11,6 +11,7 @@ module Projects LARGE_FILE_SIZE = 1.megabytes attr_reader :lfs_download_object + delegate :oid, :size, :credentials, :sanitized_url, :headers, to: :lfs_download_object, prefix: :lfs def initialize(project, lfs_download_object) diff --git a/app/services/projects/refresh_build_artifacts_size_statistics_service.rb b/app/services/projects/refresh_build_artifacts_size_statistics_service.rb new file mode 100644 index 00000000000..794c042ea39 --- /dev/null +++ b/app/services/projects/refresh_build_artifacts_size_statistics_service.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Projects + class RefreshBuildArtifactsSizeStatisticsService + BATCH_SIZE = 1000 + + def execute + refresh = Projects::BuildArtifactsSizeRefresh.process_next_refresh! + return unless refresh + + batch = refresh.next_batch(limit: BATCH_SIZE).to_a + + if batch.any? + # We are doing the sum in ruby because the query takes too long when done in SQL + total_artifacts_size = batch.sum(&:size) + + Projects::BuildArtifactsSizeRefresh.transaction do + # Mark the refresh ready for another worker to pick up and process the next batch + refresh.requeue!(batch.last.id) + + refresh.project.statistics.delayed_increment_counter(:build_artifacts_size, total_artifacts_size) + end + else + # Remove the refresh job from the table if there are no more + # remaining job artifacts to calculate for the given project. + refresh.destroy! + end + + refresh + end + end +end diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 0000e713cb4..2ec965fe2f4 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -22,8 +22,8 @@ module Projects register_attempt # Create status notifying the deployment of pages - @status = build_commit_status - ::Ci::Pipelines::AddJobService.new(@build.pipeline).execute!(@status) do |job| + @commit_status = build_commit_status + ::Ci::Pipelines::AddJobService.new(@build.pipeline).execute!(@commit_status) do |job| job.enqueue! job.run! end @@ -46,17 +46,17 @@ module Projects private def success - @status.success - @project.mark_pages_as_deployed(artifacts_archive: build.job_artifacts_archive) + @commit_status.success + @project.mark_pages_as_deployed super end def error(message) register_failure log_error("Projects::UpdatePagesService: #{message}") - @status.allow_failure = !latest? - @status.description = message - @status.drop(:script_failure) + @commit_status.allow_failure = !latest? + @commit_status.description = message + @commit_status.drop(:script_failure) super end diff --git a/app/services/repositories/base_service.rb b/app/services/repositories/base_service.rb index efb6f6de8db..13ad126f8f0 100644 --- a/app/services/repositories/base_service.rb +++ b/app/services/repositories/base_service.rb @@ -18,8 +18,6 @@ class Repositories::BaseService < BaseService end def mv_repository(from_path, to_path) - return true unless repo_exists?(from_path) - gitlab_shell.mv_repository(repository.shard, from_path, to_path) end diff --git a/app/services/repositories/destroy_rollback_service.rb b/app/services/repositories/destroy_rollback_service.rb index 5ef4e11bf55..a19e305607f 100644 --- a/app/services/repositories/destroy_rollback_service.rb +++ b/app/services/repositories/destroy_rollback_service.rb @@ -12,8 +12,14 @@ class Repositories::DestroyRollbackService < Repositories::BaseService log_info(%Q{Repository "#{removal_path}" moved to "#{disk_path}" for repository "#{full_path}"}) success - else + elsif repo_exists?(removal_path) + # If the repo does not exist, there is no need to return an + # error because there was nothing to do. move_error(removal_path) + else + success end + rescue Gitlab::Git::Repository::NoRepository + success end end diff --git a/app/services/repositories/destroy_service.rb b/app/services/repositories/destroy_service.rb index 1e34dfbe398..c5a0af56066 100644 --- a/app/services/repositories/destroy_service.rb +++ b/app/services/repositories/destroy_service.rb @@ -30,8 +30,12 @@ class Repositories::DestroyService < Repositories::BaseService log_info("Repository \"#{full_path}\" was removed") success - else + elsif repo_exists?(disk_path) move_error(disk_path) + else + success end + rescue Gitlab::Git::Repository::NoRepository + success end end diff --git a/app/services/security/ci_configuration/base_create_service.rb b/app/services/security/ci_configuration/base_create_service.rb index ea77cd98ba3..7f3b66d40e1 100644 --- a/app/services/security/ci_configuration/base_create_service.rb +++ b/app/services/security/ci_configuration/base_create_service.rb @@ -41,7 +41,7 @@ module Security end def existing_gitlab_ci_content - @gitlab_ci_yml ||= project.repository.gitlab_ci_yml_for(project.repository.root_ref_sha) + @gitlab_ci_yml ||= project.ci_config_for(project.repository.root_ref_sha) YAML.safe_load(@gitlab_ci_yml) if @gitlab_ci_yml end diff --git a/app/services/security/ci_configuration/container_scanning_create_service.rb b/app/services/security/ci_configuration/container_scanning_create_service.rb index 788533575e6..da2f1ac0981 100644 --- a/app/services/security/ci_configuration/container_scanning_create_service.rb +++ b/app/services/security/ci_configuration/container_scanning_create_service.rb @@ -6,7 +6,8 @@ module Security private def action - Security::CiConfiguration::ContainerScanningBuildAction.new(project.auto_devops_enabled?, existing_gitlab_ci_content).generate + Security::CiConfiguration::ContainerScanningBuildAction.new(project.auto_devops_enabled?, existing_gitlab_ci_content, + project.ci_config_path).generate end def next_branch diff --git a/app/services/security/ci_configuration/dependency_scanning_create_service.rb b/app/services/security/ci_configuration/dependency_scanning_create_service.rb index 71e8d5025ae..b11eccc680c 100644 --- a/app/services/security/ci_configuration/dependency_scanning_create_service.rb +++ b/app/services/security/ci_configuration/dependency_scanning_create_service.rb @@ -6,7 +6,8 @@ module Security private def action - Security::CiConfiguration::DependencyScanningBuildAction.new(project.auto_devops_enabled?, existing_gitlab_ci_content).generate + Security::CiConfiguration::DependencyScanningBuildAction.new(project.auto_devops_enabled?, existing_gitlab_ci_content, + project.ci_config_path).generate end def next_branch diff --git a/app/services/security/ci_configuration/sast_create_service.rb b/app/services/security/ci_configuration/sast_create_service.rb index 47e01847b17..d78e22f1fe1 100644 --- a/app/services/security/ci_configuration/sast_create_service.rb +++ b/app/services/security/ci_configuration/sast_create_service.rb @@ -26,7 +26,7 @@ module Security nil end - Security::CiConfiguration::SastBuildAction.new(project.auto_devops_enabled?, params, existing_content).generate + Security::CiConfiguration::SastBuildAction.new(project.auto_devops_enabled?, params, existing_content, project.ci_config_path).generate end def next_branch diff --git a/app/services/security/ci_configuration/sast_iac_create_service.rb b/app/services/security/ci_configuration/sast_iac_create_service.rb index 80e9cf963da..fbc65484216 100644 --- a/app/services/security/ci_configuration/sast_iac_create_service.rb +++ b/app/services/security/ci_configuration/sast_iac_create_service.rb @@ -6,7 +6,8 @@ module Security private def action - Security::CiConfiguration::SastIacBuildAction.new(project.auto_devops_enabled?, existing_gitlab_ci_content).generate + Security::CiConfiguration::SastIacBuildAction.new(project.auto_devops_enabled?, existing_gitlab_ci_content, + project.ci_config_path).generate end def next_branch diff --git a/app/services/security/ci_configuration/secret_detection_create_service.rb b/app/services/security/ci_configuration/secret_detection_create_service.rb index ff3458d36fc..ca5138b6ed6 100644 --- a/app/services/security/ci_configuration/secret_detection_create_service.rb +++ b/app/services/security/ci_configuration/secret_detection_create_service.rb @@ -6,7 +6,8 @@ module Security private def action - Security::CiConfiguration::SecretDetectionBuildAction.new(project.auto_devops_enabled?, existing_gitlab_ci_content).generate + Security::CiConfiguration::SecretDetectionBuildAction.new(project.auto_devops_enabled?, existing_gitlab_ci_content, + project.ci_config_path).generate end def next_branch diff --git a/app/services/security/merge_reports_service.rb b/app/services/security/merge_reports_service.rb index 5f6f98a3c39..a982ec7efe2 100644 --- a/app/services/security/merge_reports_service.rb +++ b/app/services/security/merge_reports_service.rb @@ -21,7 +21,10 @@ module Security source_reports.first.type, source_reports.first.pipeline, source_reports.first.created_at - ).tap { |report| report.errors = source_reports.flat_map(&:errors) } + ).tap do |report| + report.errors = source_reports.flat_map(&:errors) + report.warnings = source_reports.flat_map(&:warnings) + end end def copy_resources_to_target_report diff --git a/app/services/spam/spam_action_service.rb b/app/services/spam/spam_action_service.rb index 2a28b66f09b..4fa9c0e4993 100644 --- a/app/services/spam/spam_action_service.rb +++ b/app/services/spam/spam_action_service.rb @@ -65,22 +65,19 @@ module Spam # ask the SpamVerdictService what to do with the target. spam_verdict_service.execute.tap do |result| case result - when CONDITIONAL_ALLOW - # at the moment, this means "ask for reCAPTCHA" - create_spam_log - - break if target.allow_possible_spam? - - target.needs_recaptcha! - when DISALLOW - # TODO: remove `unless target.allow_possible_spam?` once this flag has been passed to `SpamVerdictService` - # https://gitlab.com/gitlab-org/gitlab/-/issues/214739 - target.spam! unless target.allow_possible_spam? - create_spam_log when BLOCK_USER # TODO: improve BLOCK_USER handling, non-existent until now # https://gitlab.com/gitlab-org/gitlab/-/issues/329666 - target.spam! unless target.allow_possible_spam? + target.spam! + create_spam_log + when DISALLOW + target.spam! + create_spam_log + when CONDITIONAL_ALLOW + # This means "require a CAPTCHA to be solved" + target.needs_recaptcha! + create_spam_log + when OVERRIDE_VIA_ALLOW_POSSIBLE_SPAM create_spam_log when ALLOW target.clear_spam_flags! diff --git a/app/services/spam/spam_constants.rb b/app/services/spam/spam_constants.rb index b654fbbbcc8..d300525710c 100644 --- a/app/services/spam/spam_constants.rb +++ b/app/services/spam/spam_constants.rb @@ -2,11 +2,12 @@ module Spam module SpamConstants - CONDITIONAL_ALLOW = "conditional_allow" - DISALLOW = "disallow" - ALLOW = "allow" - BLOCK_USER = "block" - NOOP = "noop" + BLOCK_USER = 'block' + DISALLOW = 'disallow' + CONDITIONAL_ALLOW = 'conditional_allow' + OVERRIDE_VIA_ALLOW_POSSIBLE_SPAM = 'override_via_allow_possible_spam' + ALLOW = 'allow' + NOOP = 'noop' SUPPORTED_VERDICTS = { BLOCK_USER => { @@ -18,11 +19,14 @@ module Spam CONDITIONAL_ALLOW => { priority: 3 }, - ALLOW => { + OVERRIDE_VIA_ALLOW_POSSIBLE_SPAM => { priority: 4 }, - NOOP => { + ALLOW => { priority: 5 + }, + NOOP => { + priority: 6 } }.freeze end diff --git a/app/services/spam/spam_params.rb b/app/services/spam/spam_params.rb index ccc17a42f01..81db6b390b2 100644 --- a/app/services/spam/spam_params.rb +++ b/app/services/spam/spam_params.rb @@ -25,6 +25,7 @@ module Spam # then the spam check may fail, or the SpamLog or UserAgentDetail may have missing fields. class SpamParams def self.new_from_request(request:) + self.normalize_grape_request_headers(request: request) self.new( captcha_response: request.headers['X-GitLab-Captcha-Response'], spam_log_id: request.headers['X-GitLab-Spam-Log-Id'], @@ -52,5 +53,14 @@ module Spam other.user_agent == user_agent && other.referer == referer end + + def self.normalize_grape_request_headers(request:) + # If needed, make a normalized copy of Grape headers with the case of 'GitLab' (with an + # uppercase 'L') instead of 'Gitlab' (with a lowercase 'l'), because Grape header helper keys + # are "coerced into a capitalized kebab case". See https://github.com/ruby-grape/grape#request + %w[X-Gitlab-Captcha-Response X-Gitlab-Spam-Log-Id].each do |header| + request.headers[header.gsub('Gitlab', 'GitLab')] = request.headers[header] if request.headers.key?(header) + end + end end end diff --git a/app/services/spam/spam_verdict_service.rb b/app/services/spam/spam_verdict_service.rb index c8bdcf4310b..e73b2666c02 100644 --- a/app/services/spam/spam_verdict_service.rb +++ b/app/services/spam/spam_verdict_service.rb @@ -39,21 +39,24 @@ module Spam return ALLOW unless valid_results.any? # Favour the most restrictive result. - final_verdict = valid_results.min_by { |v| SUPPORTED_VERDICTS[v][:priority] } + verdict = valid_results.min_by { |v| SUPPORTED_VERDICTS[v][:priority] } + + # The target can override the verdict via the `allow_possible_spam` feature flag + verdict = OVERRIDE_VIA_ALLOW_POSSIBLE_SPAM if override_via_allow_possible_spam?(verdict: verdict) logger.info(class: self.class.name, akismet_verdict: akismet_verdict, spam_check_verdict: original_spamcheck_result, extra_attributes: spamcheck_attribs, spam_check_rtt: external_spam_check_round_trip_time.real, - final_verdict: final_verdict, + final_verdict: verdict, username: user.username, user_id: user.id, target_type: target.class.to_s, project_id: target.project_id ) - final_verdict + verdict end private @@ -87,6 +90,14 @@ module Spam end end + def override_via_allow_possible_spam?(verdict:) + # If the verdict is already going to allow (because current verdict's priority value is greater + # than the override verdict's priority value), then we don't need to override it. + return false if SUPPORTED_VERDICTS[verdict][:priority] > SUPPORTED_VERDICTS[OVERRIDE_VIA_ALLOW_POSSIBLE_SPAM][:priority] + + target.allow_possible_spam? + end + def spamcheck_client @spamcheck_client ||= Gitlab::Spamcheck::Client.new end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 1f1edad7a69..9db39a5e174 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -49,12 +49,12 @@ module SystemNoteService ::SystemNotes::IssuablesService.new(noteable: issuable, project: project, author: author).change_issuable_contacts(added_count, removed_count) end - def relate_issue(noteable, noteable_ref, user) - ::SystemNotes::IssuablesService.new(noteable: noteable, project: noteable.project, author: user).relate_issue(noteable_ref) + def relate_issuable(noteable, noteable_ref, user) + ::SystemNotes::IssuablesService.new(noteable: noteable, project: noteable.project, author: user).relate_issuable(noteable_ref) end - def unrelate_issue(noteable, noteable_ref, user) - ::SystemNotes::IssuablesService.new(noteable: noteable, project: noteable.project, author: user).unrelate_issue(noteable_ref) + def unrelate_issuable(noteable, noteable_ref, user) + ::SystemNotes::IssuablesService.new(noteable: noteable, project: noteable.project, author: user).unrelate_issuable(noteable_ref) end # Called when the due_date of a Noteable is changed diff --git a/app/services/system_notes/issuables_service.rb b/app/services/system_notes/issuables_service.rb index 09f36bb6501..89212288a6b 100644 --- a/app/services/system_notes/issuables_service.rb +++ b/app/services/system_notes/issuables_service.rb @@ -10,8 +10,9 @@ module SystemNotes # "marked this issue as related to gitlab-foss#9001" # # Returns the created Note object - def relate_issue(noteable_ref) - body = "marked this issue as related to #{noteable_ref.to_reference(noteable.project)}" + def relate_issuable(noteable_ref) + issuable_type = noteable.to_ability_name.humanize(capitalize: false) + body = "marked this #{issuable_type} as related to #{noteable_ref.to_reference(noteable.resource_parent)}" issue_activity_counter.track_issue_related_action(author: author) if noteable.is_a?(Issue) @@ -26,8 +27,8 @@ module SystemNotes # "removed the relation with gitlab-foss#9001" # # Returns the created Note object - def unrelate_issue(noteable_ref) - body = "removed the relation with #{noteable_ref.to_reference(noteable.project)}" + def unrelate_issuable(noteable_ref) + body = "removed the relation with #{noteable_ref.to_reference(noteable.resource_parent)}" issue_activity_counter.track_issue_unrelated_action(author: author) if noteable.is_a?(Issue) @@ -160,6 +161,7 @@ module SystemNotes body = "changed title from **#{marked_old_title}** to **#{marked_new_title}**" issue_activity_counter.track_issue_title_changed_action(author: author) if noteable.is_a?(Issue) + work_item_activity_counter.track_work_item_title_changed_action(author: author) if noteable.is_a?(WorkItem) create_note(NoteSummary.new(noteable, project, author, body, action: 'title')) end @@ -484,6 +486,10 @@ module SystemNotes Gitlab::UsageDataCounters::IssueActivityUniqueCounter end + def work_item_activity_counter + Gitlab::UsageDataCounters::WorkItemActivityUniqueCounter + end + def track_cross_reference_action issue_activity_counter.track_issue_cross_referenced_action(author: author) if noteable.is_a?(Issue) end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 091f441831a..64309c7f786 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -9,6 +9,7 @@ # class TodoService include Gitlab::Utils::UsageData + # When create an issue we should: # # * create a todo for assignee if issue is assigned @@ -229,8 +230,24 @@ class TodoService return if users.empty? - users_with_pending_todos = pending_todos(users, attributes).distinct_user_ids - users.reject! { |user| users_with_pending_todos.include?(user.id) && Feature.disabled?(:multiple_todos, user) } + users_single_todos, users_multiple_todos = users.partition { |u| Feature.disabled?(:multiple_todos, u) } + excluded_user_ids = [] + + if users_single_todos.present? + excluded_user_ids += pending_todos( + users_single_todos, + attributes.slice(:project_id, :target_id, :target_type, :commit_id, :discussion) + ).distinct_user_ids + end + + if users_multiple_todos.present? && !Todo::ACTIONS_MULTIPLE_ALLOWED.include?(attributes.fetch(:action)) + excluded_user_ids += pending_todos( + users_multiple_todos, + attributes.slice(:project_id, :target_id, :target_type, :commit_id, :discussion, :action) + ).distinct_user_ids + end + + users.reject! { |user| excluded_user_ids.include?(user.id) } todos = users.map do |user| issue_type = attributes.delete(:issue_type) diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb index 575614e8743..604b83f621f 100644 --- a/app/services/users/migrate_to_ghost_user_service.rb +++ b/app/services/users/migrate_to_ghost_user_service.rb @@ -66,20 +66,20 @@ module Users # rubocop: disable CodeReuse/ActiveRecord def migrate_issues - user.issues.update_all(author_id: ghost_user.id) - Issue.where(last_edited_by_id: user.id).update_all(last_edited_by_id: ghost_user.id) + batched_migrate(Issue, :author_id) + batched_migrate(Issue, :last_edited_by_id) end # rubocop: enable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord def migrate_merge_requests - user.merge_requests.update_all(author_id: ghost_user.id) - MergeRequest.where(merge_user_id: user.id).update_all(merge_user_id: ghost_user.id) + batched_migrate(MergeRequest, :author_id) + batched_migrate(MergeRequest, :merge_user_id) end # rubocop: enable CodeReuse/ActiveRecord def migrate_notes - user.notes.update_all(author_id: ghost_user.id) + batched_migrate(Note, :author_id) end def migrate_abuse_reports @@ -96,8 +96,17 @@ module Users end def migrate_reviews - user.reviews.update_all(author_id: ghost_user.id) + batched_migrate(Review, :author_id) end + + # rubocop:disable CodeReuse/ActiveRecord + def batched_migrate(base_scope, column) + loop do + update_count = base_scope.where(column => user.id).limit(100).update_all(column => ghost_user.id) + break if update_count == 0 + end + end + # rubocop:enable CodeReuse/ActiveRecord end end diff --git a/app/services/users/saved_replies/create_service.rb b/app/services/users/saved_replies/create_service.rb new file mode 100644 index 00000000000..21378ec4435 --- /dev/null +++ b/app/services/users/saved_replies/create_service.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Users + module SavedReplies + class CreateService + def initialize(current_user:, name:, content:) + @current_user = current_user + @name = name + @content = content + end + + def execute + saved_reply = saved_replies.build(name: name, content: content) + + if saved_reply.save + ServiceResponse.success(payload: { saved_reply: saved_reply }) + else + ServiceResponse.error(message: saved_reply.errors.full_messages) + end + end + + private + + attr_reader :current_user, :name, :content + + delegate :saved_replies, to: :current_user + end + end +end diff --git a/app/services/users/saved_replies/update_service.rb b/app/services/users/saved_replies/update_service.rb new file mode 100644 index 00000000000..ab0a3eaf87d --- /dev/null +++ b/app/services/users/saved_replies/update_service.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Users + module SavedReplies + class UpdateService + def initialize(current_user:, saved_reply:, name:, content:) + @current_user = current_user + @saved_reply = saved_reply + @name = name + @content = content + end + + def execute + if saved_reply.update(name: name, content: content) + ServiceResponse.success(payload: { saved_reply: saved_reply.reset }) + else + ServiceResponse.error(message: saved_reply.errors.full_messages) + end + end + + private + + attr_reader :current_user, :saved_reply, :name, :content + end + end +end diff --git a/app/services/web_hooks/log_execution_service.rb b/app/services/web_hooks/log_execution_service.rb index 6e58e15f093..0ee7c41469f 100644 --- a/app/services/web_hooks/log_execution_service.rb +++ b/app/services/web_hooks/log_execution_service.rb @@ -2,34 +2,86 @@ module WebHooks class LogExecutionService + include ::Gitlab::ExclusiveLeaseHelpers + + LOCK_TTL = 15.seconds.freeze + LOCK_SLEEP = 0.25.seconds.freeze + LOCK_RETRY = 65 + attr_reader :hook, :log_data, :response_category def initialize(hook:, log_data:, response_category:) @hook = hook - @log_data = log_data + @log_data = log_data.transform_keys(&:to_sym) @response_category = response_category + @prev_state = hook.active_state(ignore_flag: true) end def execute - update_hook_executability + update_hook_failure_state log_execution end private def log_execution - WebHookLog.create!(web_hook: hook, **log_data.transform_keys(&:to_sym)) + WebHookLog.create!(web_hook: hook, **log_data) end - def update_hook_executability - case response_category - when :ok - hook.enable! - when :error - hook.backoff! - when :failed - hook.failed! + # Perform this operation within an `Gitlab::ExclusiveLease` lock to make it + # safe to be called concurrently from different workers. + def update_hook_failure_state + in_lock(lock_name, ttl: LOCK_TTL, sleep_sec: LOCK_SLEEP, retries: LOCK_RETRY) do |retried| + hook.reset # Reload within the lock so properties are guaranteed to be current. + + case response_category + when :ok + hook.enable! + when :error + hook.backoff! + when :failed + hook.failed! + end + + log_state_change end + rescue Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError + raise if raise_lock_error? + end + + def log_state_change + new_state = hook.active_state(ignore_flag: true) + + return if @prev_state == new_state + + Gitlab::AuthLogger.info( + message: 'WebHook change active_state', + # identification + hook_id: hook.id, + hook_type: hook.type, + project_id: hook.project_id, + group_id: hook.group_id, + # relevant data + prev_state: @prev_state, + new_state: new_state, + duration: log_data[:execution_duration], + response_status: log_data[:response_status], + recent_hook_failures: hook.recent_failures, + # context + **Gitlab::ApplicationContext.current + ) + end + + def lock_name + "web_hooks:update_hook_failure_state:#{hook.id}" + end + + # Allow an error to be raised after failing to obtain a lease only if the hook + # is not already in the correct failure state. + def raise_lock_error? + hook.reset # Reload so properties are guaranteed to be current. + + hook.executable? != (response_category == :ok) end end end diff --git a/app/services/work_items/create_and_link_service.rb b/app/services/work_items/create_and_link_service.rb new file mode 100644 index 00000000000..534d220a846 --- /dev/null +++ b/app/services/work_items/create_and_link_service.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module WorkItems + # Create and link operations are not run inside a transaction in this class + # because CreateFromTaskService also creates a transaction. + # This class should always be run inside a transaction as we could end up with + # new work items that were never associated with other work items as expected. + class CreateAndLinkService + def initialize(project:, current_user: nil, params: {}, spam_params:, link_params: {}) + @create_service = CreateService.new( + project: project, + current_user: current_user, + params: params, + spam_params: spam_params + ) + @project = project + @current_user = current_user + @link_params = link_params + end + + def execute + create_result = @create_service.execute + return create_result if create_result.error? + + work_item = create_result[:work_item] + return ::ServiceResponse.success(payload: payload(work_item)) if @link_params.blank? + + result = IssueLinks::CreateService.new(work_item, @current_user, @link_params).execute + + if result[:status] == :success + ::ServiceResponse.success(payload: payload(work_item)) + else + ::ServiceResponse.error(message: result[:message], http_status: 404) + end + end + + private + + def payload(work_item) + { work_item: work_item } + end + end +end diff --git a/app/services/work_items/create_from_task_service.rb b/app/services/work_items/create_from_task_service.rb new file mode 100644 index 00000000000..4203c96e676 --- /dev/null +++ b/app/services/work_items/create_from_task_service.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module WorkItems + class CreateFromTaskService + def initialize(work_item:, current_user: nil, work_item_params: {}, spam_params:) + @work_item = work_item + @current_user = current_user + @work_item_params = work_item_params + @spam_params = spam_params + @errors = [] + end + + def execute + transaction_result = ApplicationRecord.transaction do + create_and_link_result = CreateAndLinkService.new( + project: @work_item.project, + current_user: @current_user, + params: @work_item_params.slice(:title, :work_item_type_id), + spam_params: @spam_params, + link_params: { target_issuable: @work_item } + ).execute + + if create_and_link_result.error? + @errors += create_and_link_result.errors + raise ActiveRecord::Rollback + end + + replacement_result = TaskListReferenceReplacementService.new( + work_item: @work_item, + work_item_reference: create_and_link_result[:work_item].to_reference, + line_number_start: @work_item_params[:line_number_start], + line_number_end: @work_item_params[:line_number_end], + title: @work_item_params[:title], + lock_version: @work_item_params[:lock_version] + ).execute + + if replacement_result.error? + @errors += replacement_result.errors + raise ActiveRecord::Rollback + end + + create_and_link_result + end + + return transaction_result if transaction_result + + ::ServiceResponse.error(message: @errors, http_status: 422) + end + end +end diff --git a/app/services/work_items/task_list_reference_replacement_service.rb b/app/services/work_items/task_list_reference_replacement_service.rb new file mode 100644 index 00000000000..1044a4feb88 --- /dev/null +++ b/app/services/work_items/task_list_reference_replacement_service.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module WorkItems + class TaskListReferenceReplacementService + STALE_OBJECT_MESSAGE = 'Stale work item. Check lock version' + + def initialize(work_item:, work_item_reference:, line_number_start:, line_number_end:, title:, lock_version:) + @work_item = work_item + @work_item_reference = work_item_reference + @line_number_start = line_number_start + @line_number_end = line_number_end + @title = title + @lock_version = lock_version + end + + def execute + return ::ServiceResponse.error(message: STALE_OBJECT_MESSAGE) if @work_item.lock_version > @lock_version + return ::ServiceResponse.error(message: 'line_number_start must be greater than 0') if @line_number_start < 1 + return ::ServiceResponse.error(message: 'line_number_end must be greater or equal to line_number_start') if @line_number_end < @line_number_start + return ::ServiceResponse.error(message: "Work item description can't be blank") if @work_item.description.blank? + + source_lines = @work_item.description.split("\n") + markdown_task_first_line = source_lines[@line_number_start - 1] + task_line = Taskable::ITEM_PATTERN.match(markdown_task_first_line) + + return ::ServiceResponse.error(message: "Unable to detect a task on line #{@line_number_start}") unless task_line + + captures = task_line.captures + + markdown_task_first_line.sub!(Taskable::ITEM_PATTERN, "#{captures[0]} #{captures[1]} #{@work_item_reference}+") + + source_lines[@line_number_start - 1] = markdown_task_first_line + remove_additional_lines!(source_lines) + + @work_item.update!(description: source_lines.join("\n")) + + ::ServiceResponse.success + rescue ActiveRecord::StaleObjectError + ::ServiceResponse.error(message: STALE_OBJECT_MESSAGE) + end + + private + + def remove_additional_lines!(source_lines) + return if @line_number_end <= @line_number_start + + source_lines.delete_if.each_with_index do |_line, index| + index >= @line_number_start && index < @line_number_end + end + end + end +end |