diff options
54 files changed, 947 insertions, 662 deletions
diff --git a/.gitpod.yml b/.gitpod.yml index dc1dbc1472d..a28a11d6612 100644 --- a/.gitpod.yml +++ b/.gitpod.yml @@ -13,7 +13,10 @@ tasks: ( set -e cd /workspace/gitlab-development-kit - [[ ! -L /workspace/gitlab-development-kit/gitlab ]] && ln -fs /workspace/gitlab /workspace/gitlab-development-kit/gitlab + # GitLab FOSS + [[ -d /workspace/gitlab-foss ]] && ln -fs /workspace/gitlab-foss /workspace/gitlab-development-kit/gitlab + # GitLab + [[ -d /workspace/gitlab ]] && ln -fs /workspace/gitlab /workspace/gitlab-development-kit/gitlab mv /workspace/gitlab-development-kit/secrets.yml /workspace/gitlab-development-kit/gitlab/config # reconfigure GDK echo "$(date) – Reconfiguring GDK" | tee -a /workspace/startup.log @@ -47,9 +50,11 @@ tasks: if [ "$GITLAB_RUN_DB_MIGRATIONS" == true ]; then make gitlab-db-migrate fi - cd ../gitlab + cd /workspace/gitlab-development-kit/gitlab + # Install Lefthook + bundle exec lefthook install git checkout db/structure.sql - cd ../gitlab-development-kit + cd /workspace/gitlab-development-kit # Waiting for GitLab ... gp await-port 3000 printf "Waiting for GitLab at $(gp url 3000) ..." diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index 4ec561014a8..b23cc4a302d 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -3,9 +3,6 @@ module SpammableActions extend ActiveSupport::Concern - include Recaptcha::Verify - include Gitlab::Utils::StrongMemoize - included do before_action :authorize_submit_spammable!, only: :mark_as_spam end @@ -21,9 +18,7 @@ module SpammableActions private def ensure_spam_config_loaded! - strong_memoize(:spam_config_loaded) do - Gitlab::Recaptcha.load_configurations! - end + Gitlab::Recaptcha.load_configurations! end def recaptcha_check_with_fallback(should_redirect = true, &fallback) @@ -50,33 +45,30 @@ module SpammableActions end def spammable_params - default_params = { request: request } - - recaptcha_check = recaptcha_response && - ensure_spam_config_loaded! && - verify_recaptcha(response: recaptcha_response) - - return default_params unless recaptcha_check - - { recaptcha_verified: true, - spam_log_id: params[:spam_log_id] }.merge(default_params) - end - - def recaptcha_response - # NOTE: This field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the recaptcha - # gem, which is called from the HAML `_recaptcha_form.html.haml` form. + # NOTE: For the legacy reCAPTCHA implementation based on the HTML/HAML form, the + # 'g-recaptcha-response' field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the + # recaptcha gem, which is called from the HAML `_recaptcha_form.html.haml` form. # - # It is used in the `Recaptcha::Verify#verify_recaptcha` if the `response` option is not - # passed explicitly. + # It is used in the `Recaptcha::Verify#verify_recaptcha` to extract the value from `params`, + # if the `response` option is not passed explicitly. # # Instead of relying on this behavior, we are extracting and passing it explicitly. This will # make it consistent with the newer, modern reCAPTCHA verification process as it will be # implemented via the GraphQL API and in Vue components via the native reCAPTCHA Javascript API, # which requires that the recaptcha response param be obtained and passed explicitly. # - # After this newer GraphQL/JS API process is fully supported by the backend, we can remove this - # (and other) HAML-specific support. - params['g-recaptcha-response'] + # It can also be expanded to multiple fields when we move to future alternative captcha + # implementations such as FriendlyCaptcha. See https://gitlab.com/gitlab-org/gitlab/-/issues/273480 + + # After this newer GraphQL/JS API process is fully supported by the backend, we can remove the + # check for the 'g-recaptcha-response' field and other HTML/HAML form-specific support. + captcha_response = params['g-recaptcha-response'] + + { + request: request, + spam_log_id: params[:spam_log_id], + captcha_response: captcha_response + } end def spammable diff --git a/app/finders/merge_request/metrics_finder.rb b/app/finders/merge_request/metrics_finder.rb new file mode 100644 index 00000000000..d93e53d1636 --- /dev/null +++ b/app/finders/merge_request/metrics_finder.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +class MergeRequest::MetricsFinder + include Gitlab::Allowable + + def initialize(current_user, params = {}) + @current_user = current_user + @params = params + end + + def execute + return klass.none if target_project.blank? || user_not_authorized? + + items = init_collection + items = by_target_project(items) + items = by_merged_after(items) + items = by_merged_before(items) + + items + end + + private + + attr_reader :current_user, :params + + def by_target_project(items) + items.by_target_project(target_project) + end + + def by_merged_after(items) + return items unless merged_after + + items.merged_after(merged_after) + end + + def by_merged_before(items) + return items unless merged_before + + items.merged_before(merged_before) + end + + def user_not_authorized? + !can?(current_user, :read_merge_request, target_project) + end + + def init_collection + klass.all + end + + def klass + MergeRequest::Metrics + end + + def target_project + params[:target_project] + end + + def merged_after + params[:merged_after] + end + + def merged_before + params[:merged_before] + end +end diff --git a/app/graphql/resolvers/project_merge_requests_resolver.rb b/app/graphql/resolvers/project_merge_requests_resolver.rb index 830649d5e52..21d9afc31ab 100644 --- a/app/graphql/resolvers/project_merge_requests_resolver.rb +++ b/app/graphql/resolvers/project_merge_requests_resolver.rb @@ -6,5 +6,38 @@ module Resolvers accept_assignee accept_author accept_reviewer + + def resolve(**args) + scope = super + + if only_count_is_selected_with_merged_at_filter?(args) && Feature.enabled?(:optimized_merge_request_count_with_merged_at_filter) + MergeRequest::MetricsFinder + .new(current_user, args.merge(target_project: project)) + .execute + else + scope + end + end + + def only_count_is_selected_with_merged_at_filter?(args) + return unless lookahead + + argument_names = args.except(:lookahead, :sort, :merged_before, :merged_after).keys + + # no extra filtering arguments are provided + return unless argument_names.empty? + return unless args[:merged_after] || args[:merged_before] + + # Detecting a specific query pattern: + # mergeRequests(mergedAfter: "X", mergedBefore: "Y") { + # count + # totalTimeToMerge + # } + allowed_selected_fields = [:count, :total_time_to_merge] + selected_fields = lookahead.selections.map(&:field).map(&:original_name) + + # only the allowed_selected_fields are present + (selected_fields - allowed_selected_fields).empty? + end end end diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index e44d3d96334..3bc8bd2661f 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -676,7 +676,7 @@ module Ci def number_of_warnings BatchLoader.for(id).batch(default_value: 0) do |pipeline_ids, loader| - ::Ci::Build.where(commit_id: pipeline_ids) + ::CommitStatus.where(commit_id: pipeline_ids) .latest .failed_but_allowed .group(:commit_id) diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index cc6bd1870b9..ae80692d598 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -118,7 +118,7 @@ module Ci def number_of_warnings BatchLoader.for(id).batch(default_value: 0) do |stage_ids, loader| - ::Ci::Build.where(stage_id: stage_ids) + ::CommitStatus.where(stage_id: stage_ids) .latest .failed_but_allowed .group(:stage_id) diff --git a/app/models/merge_request/metrics.rb b/app/models/merge_request/metrics.rb index d3fe256fb1b..5c611da0684 100644 --- a/app/models/merge_request/metrics.rb +++ b/app/models/merge_request/metrics.rb @@ -5,12 +5,14 @@ class MergeRequest::Metrics < ApplicationRecord belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :pipeline_id belongs_to :latest_closed_by, class_name: 'User' belongs_to :merged_by, class_name: 'User' + belongs_to :target_project, class_name: 'Project', inverse_of: :merge_requests before_save :ensure_target_project_id scope :merged_after, ->(date) { where(arel_table[:merged_at].gteq(date)) } scope :merged_before, ->(date) { where(arel_table[:merged_at].lteq(date)) } scope :with_valid_time_to_merge, -> { where(arel_table[:merged_at].gt(arel_table[:created_at])) } + scope :by_target_project, ->(project) { where(target_project_id: project) } def self.time_to_merge_expression Arel.sql('EXTRACT(epoch FROM SUM(AGE(merge_request_metrics.merged_at, merge_request_metrics.created_at)))') @@ -21,6 +23,12 @@ class MergeRequest::Metrics < ApplicationRecord def ensure_target_project_id self.target_project_id ||= merge_request.target_project_id end + + def self.total_time_to_merge + with_valid_time_to_merge + .pluck(time_to_merge_expression) + .first + end end MergeRequest::Metrics.prepend_if_ee('EE::MergeRequest::Metrics') diff --git a/app/models/project.rb b/app/models/project.rb index 3f46773bb8c..aad2e7d2259 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -218,6 +218,7 @@ class Project < ApplicationRecord # Merge Requests for target project should be removed with it has_many :merge_requests, foreign_key: 'target_project_id', inverse_of: :target_project + has_many :merge_request_metrics, foreign_key: 'target_project', class_name: 'MergeRequest::Metrics', inverse_of: :target_project has_many :source_of_merge_requests, foreign_key: 'source_project_id', class_name: 'MergeRequest' has_many :issues has_many :labels, class_name: 'ProjectLabel' diff --git a/app/services/captcha/captcha_verification_service.rb b/app/services/captcha/captcha_verification_service.rb new file mode 100644 index 00000000000..45a5a52367c --- /dev/null +++ b/app/services/captcha/captcha_verification_service.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module Captcha + ## + # Encapsulates logic of checking captchas. + # + class CaptchaVerificationService + include Recaptcha::Verify + + ## + # Performs verification of a captcha response. + # + # 'captcha_response' parameter is the response from the user solving a client-side captcha. + # + # 'request' parameter is the request which submitted the captcha. + # + # NOTE: Currently only supports reCAPTCHA, and is not yet used in all places of the app in which + # captchas are verified, but these can be addressed in future MRs. See: + # https://gitlab.com/gitlab-org/gitlab/-/issues/273480 + def execute(captcha_response: nil, request:) + return false unless captcha_response + + @request = request + + Gitlab::Recaptcha.load_configurations! + + # NOTE: We could pass the model and let the recaptcha gem automatically add errors to it, + # but we do not, for two reasons: + # + # 1. We want control over when the errors are added + # 2. We want control over the wording and i18n of the message + # 3. We want a consistent interface and behavior when adding support for other captcha + # libraries which may not support automatically adding errors to the model. + verify_recaptcha(response: captcha_response) + end + + private + + # The recaptcha library's Recaptcha::Verify#verify_recaptcha method requires that + # 'request' be a readable attribute - it doesn't support passing it as an options argument. + attr_reader :request + end +end diff --git a/app/services/concerns/spam_check_methods.rb b/app/services/concerns/spam_check_methods.rb deleted file mode 100644 index 939f8f183ab..00000000000 --- a/app/services/concerns/spam_check_methods.rb +++ /dev/null @@ -1,39 +0,0 @@ -# frozen_string_literal: true - -# SpamCheckMethods -# -# Provide helper methods for checking if a given spammable object has -# potential spam data. -# -# Dependencies: -# - params with :request - -module SpamCheckMethods - # rubocop:disable Gitlab/ModuleWithInstanceVariables - def filter_spam_check_params - @request = params.delete(:request) - @api = params.delete(:api) - @recaptcha_verified = params.delete(:recaptcha_verified) - @spam_log_id = params.delete(:spam_log_id) - end - # rubocop:enable Gitlab/ModuleWithInstanceVariables - - # In order to be proceed to the spam check process, @spammable has to be - # a dirty instance, which means it should be already assigned with the new - # attribute values. - # rubocop:disable Gitlab/ModuleWithInstanceVariables - def spam_check(spammable, user, action:) - raise ArgumentError.new('Please provide an action, such as :create') unless action - - Spam::SpamActionService.new( - spammable: spammable, - request: @request, - user: user, - context: { action: action } - ).execute( - api: @api, - recaptcha_verified: @recaptcha_verified, - spam_log_id: @spam_log_id) - end - # rubocop:enable Gitlab/ModuleWithInstanceVariables -end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 44de8eb6389..d2285a375a1 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -2,20 +2,26 @@ module Issues class CreateService < Issues::BaseService - include SpamCheckMethods include ResolveDiscussions def execute(skip_system_notes: false) + @request = params.delete(:request) + @spam_params = Spam::SpamActionService.filter_spam_params!(params) + @issue = BuildService.new(project, current_user, params).execute - filter_spam_check_params filter_resolve_discussion_params create(@issue, skip_system_notes: skip_system_notes) end def before_create(issue) - spam_check(issue, current_user, action: :create) + Spam::SpamActionService.new( + spammable: issue, + request: request, + user: current_user, + action: :create + ).execute(spam_params: spam_params) # current_user (defined in BaseService) is not available within run_after_commit block user = current_user @@ -46,8 +52,10 @@ module Issues private + attr_reader :request, :spam_params + def user_agent_detail_service - UserAgentDetailService.new(@issue, @request) + UserAgentDetailService.new(@issue, request) end # Applies label "incident" (creates it if missing) to incident issues. diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 127ed04cf51..2906bdf62a7 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -2,12 +2,14 @@ module Issues class UpdateService < Issues::BaseService - include SpamCheckMethods extend ::Gitlab::Utils::Override def execute(issue) handle_move_between_ids(issue) - filter_spam_check_params + + @request = params.delete(:request) + @spam_params = Spam::SpamActionService.filter_spam_params!(params) + change_issue_duplicate(issue) move_issue_to_new_project(issue) || clone_issue(issue) || update_task_event(issue) || update(issue) end @@ -30,7 +32,14 @@ module Issues end def before_update(issue, skip_spam_check: false) - spam_check(issue, current_user, action: :update) unless skip_spam_check + return if skip_spam_check + + Spam::SpamActionService.new( + spammable: issue, + request: request, + user: current_user, + action: :update + ).execute(spam_params: spam_params) end def after_update(issue) @@ -126,6 +135,8 @@ module Issues private + attr_reader :request, :spam_params + def clone_issue(issue) target_project = params.delete(:target_clone_project) with_notes = params.delete(:clone_with_notes) diff --git a/app/services/snippets/base_service.rb b/app/services/snippets/base_service.rb index 278857b7933..415cfcb7d8f 100644 --- a/app/services/snippets/base_service.rb +++ b/app/services/snippets/base_service.rb @@ -2,8 +2,6 @@ module Snippets class BaseService < ::BaseService - include SpamCheckMethods - UPDATE_COMMIT_MSG = 'Update snippet' INITIAL_COMMIT_MSG = 'Initial commit' @@ -18,8 +16,6 @@ module Snippets input_actions = Array(@params.delete(:snippet_actions).presence) @snippet_actions = SnippetInputActionCollection.new(input_actions, allowed_actions: restricted_files_actions) - - filter_spam_check_params end private diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index 0881be73eaf..9b0042c988a 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -3,20 +3,28 @@ module Snippets class CreateService < Snippets::BaseService def execute + @request = params.delete(:request) + @spam_params = Spam::SpamActionService.filter_spam_params!(params) + @snippet = build_from_params return invalid_params_error(@snippet) unless valid_params? - unless visibility_allowed?(@snippet, @snippet.visibility_level) - return forbidden_visibility_error(@snippet) + unless visibility_allowed?(snippet, snippet.visibility_level) + return forbidden_visibility_error(snippet) end @snippet.author = current_user - spam_check(@snippet, current_user, action: :create) + Spam::SpamActionService.new( + spammable: @snippet, + request: request, + user: current_user, + action: :create + ).execute(spam_params: spam_params) if save_and_commit - UserAgentDetailService.new(@snippet, @request).create + UserAgentDetailService.new(@snippet, request).create Gitlab::UsageDataCounters::SnippetCounter.count(:create) move_temporary_files @@ -29,6 +37,8 @@ module Snippets private + attr_reader :snippet, :request, :spam_params + def build_from_params if project project.snippets.build(create_params) diff --git a/app/services/snippets/update_service.rb b/app/services/snippets/update_service.rb index b982ff98747..a723cf763d9 100644 --- a/app/services/snippets/update_service.rb +++ b/app/services/snippets/update_service.rb @@ -7,6 +7,9 @@ module Snippets UpdateError = Class.new(StandardError) def execute(snippet) + @request = params.delete(:request) + @spam_params = Spam::SpamActionService.filter_spam_params!(params) + return invalid_params_error(snippet) unless valid_params? if visibility_changed?(snippet) && !visibility_allowed?(snippet, visibility_level) @@ -14,7 +17,12 @@ module Snippets end update_snippet_attributes(snippet) - spam_check(snippet, current_user, action: :update) + Spam::SpamActionService.new( + spammable: snippet, + request: request, + user: current_user, + action: :update + ).execute(spam_params: spam_params) if save_and_commit(snippet) Gitlab::UsageDataCounters::SnippetCounter.count(:update) @@ -27,6 +35,8 @@ module Snippets private + attr_reader :request, :spam_params + def visibility_changed?(snippet) visibility_level && visibility_level.to_i != snippet.visibility_level end diff --git a/app/services/spam/spam_action_service.rb b/app/services/spam/spam_action_service.rb index b3d617256ff..ff32bc32d93 100644 --- a/app/services/spam/spam_action_service.rb +++ b/app/services/spam/spam_action_service.rb @@ -4,37 +4,69 @@ module Spam class SpamActionService include SpamConstants + ## + # Utility method to filter SpamParams from parameters, which will later be passed to #execute + # after the spammable is created/updated based on the remaining parameters. + # + # Takes a hash of parameters from an incoming request to modify a model (via a controller, + # service, or GraphQL mutation). + # + # Deletes the parameters which are related to spam and captcha processing, and returns + # them in a SpamParams parameters object. See: + # https://refactoring.com/catalog/introduceParameterObject.html + def self.filter_spam_params!(params) + # NOTE: The 'captcha_response' field can be expanded to multiple fields when we move to future + # alternative captcha implementations such as FriendlyCaptcha. See + # https://gitlab.com/gitlab-org/gitlab/-/issues/273480 + captcha_response = params.delete(:captcha_response) + + SpamParams.new( + api: params.delete(:api), + captcha_response: captcha_response, + spam_log_id: params.delete(:spam_log_id) + ) + end + attr_accessor :target, :request, :options attr_reader :spam_log - def initialize(spammable:, request:, user:, context: {}) + def initialize(spammable:, request:, user:, action:) @target = spammable @request = request @user = user - @context = context + @action = action @options = {} + end - if @request - @options[:ip_address] = @request.env['action_dispatch.remote_ip'].to_s - @options[:user_agent] = @request.env['HTTP_USER_AGENT'] - @options[:referrer] = @request.env['HTTP_REFERRER'] + def execute(spam_params:) + if request + options[:ip_address] = request.env['action_dispatch.remote_ip'].to_s + options[:user_agent] = request.env['HTTP_USER_AGENT'] + options[:referrer] = request.env['HTTP_REFERRER'] else - @options[:ip_address] = @target.ip_address - @options[:user_agent] = @target.user_agent + # TODO: This code is never used, because we do not perform a verification if there is not a + # request. Why? Should it be deleted? Or should we check even if there is no request? + options[:ip_address] = target.ip_address + options[:user_agent] = target.user_agent end - end - def execute(api: false, recaptcha_verified:, spam_log_id:) + recaptcha_verified = Captcha::CaptchaVerificationService.new.execute( + captcha_response: spam_params.captcha_response, + request: request + ) + if recaptcha_verified - # If it's a request which is already verified through reCAPTCHA, + # If it's a request which is already verified through captcha, # update the spam log accordingly. - SpamLog.verify_recaptcha!(user_id: user.id, id: spam_log_id) + SpamLog.verify_recaptcha!(user_id: user.id, id: spam_params.spam_log_id) + ServiceResponse.success(message: "Captcha was successfully verified") else - return if allowlisted?(user) - return unless request - return unless check_for_spam? + return ServiceResponse.success(message: 'Skipped spam check because user was allowlisted') if allowlisted?(user) + return ServiceResponse.success(message: 'Skipped spam check because request was not present') unless request + return ServiceResponse.success(message: 'Skipped spam check because it was not required') unless check_for_spam? - perform_spam_service_check(api) + perform_spam_service_check(spam_params.api) + ServiceResponse.success(message: "Spam check performed, check #{target.class.name} spammable model for any errors or captcha requirement") end end @@ -42,13 +74,27 @@ module Spam private - attr_reader :user, :context + attr_reader :user, :action + + ## + # In order to be proceed to the spam check process, the target must be + # a dirty instance, which means it should be already assigned with the new + # attribute values. + def ensure_target_is_dirty + msg = "Target instance of #{target.class.name} must be dirty (must have changes to save)" + raise(msg) unless target.has_changes_to_save? + end def allowlisted?(user) user.try(:gitlab_employee?) || user.try(:gitlab_bot?) || user.try(:gitlab_service_user?) end + ## + # Performs the spam check using the spam verdict service, and modifies the target model + # accordingly based on the result. def perform_spam_service_check(api) + ensure_target_is_dirty + # since we can check for spam, and recaptcha is not verified, # ask the SpamVerdictService what to do with the target. spam_verdict_service.execute.tap do |result| @@ -79,7 +125,7 @@ module Spam description: target.spam_description, source_ip: options[:ip_address], user_agent: options[:user_agent], - noteable_type: notable_type, + noteable_type: noteable_type, via_api: api } ) @@ -88,14 +134,19 @@ module Spam end def spam_verdict_service + context = { + action: action, + target_type: noteable_type + } + SpamVerdictService.new(target: target, user: user, - request: @request, + request: request, options: options, - context: context.merge(target_type: notable_type)) + context: context) end - def notable_type + def noteable_type @notable_type ||= target.class.to_s end end diff --git a/app/services/spam/spam_params.rb b/app/services/spam/spam_params.rb new file mode 100644 index 00000000000..fef5355c7f3 --- /dev/null +++ b/app/services/spam/spam_params.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Spam + ## + # This class is a Parameter Object (https://refactoring.com/catalog/introduceParameterObject.html) + # which acts as an container abstraction for multiple parameter values related to spam and + # captcha processing for a request. + # + # Values contained are: + # + # api: A boolean flag indicating if the request was submitted via the REST or GraphQL API + # captcha_response: The response resulting from the user solving a captcha. Currently it is + # a scalar reCAPTCHA response string, but it can be expanded to an object in the future to + # support other captcha implementations such as FriendlyCaptcha. + # spam_log_id: The id of a SpamLog record. + class SpamParams + attr_reader :api, :captcha_response, :spam_log_id + + def initialize(api:, captcha_response:, spam_log_id:) + @api = api.present? + @captcha_response = captcha_response + @spam_log_id = spam_log_id + end + + def ==(other) + other.class == self.class && + other.api == self.api && + other.captcha_response == self.captcha_response && + other.spam_log_id == self.spam_log_id + end + end +end diff --git a/app/views/admin/application_settings/_ci_cd.html.haml b/app/views/admin/application_settings/_ci_cd.html.haml index 9f384519c3a..351c13d5ad1 100644 --- a/app/views/admin/application_settings/_ci_cd.html.haml +++ b/app/views/admin/application_settings/_ci_cd.html.haml @@ -58,6 +58,6 @@ = f.text_field :default_ci_config_path, class: 'form-control', placeholder: '.gitlab-ci.yml' %p.form-text.text-muted = _("The default CI configuration path for new projects.").html_safe - = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'custom-ci-configuration-path'), target: '_blank' + = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'custom-cicd-configuration-path'), target: '_blank' = f.submit _('Save changes'), class: "gl-button btn btn-success" diff --git a/app/views/groups/settings/ci_cd/_form.html.haml b/app/views/groups/settings/ci_cd/_form.html.haml index 8fad73f1249..635e3b64e39 100644 --- a/app/views/groups/settings/ci_cd/_form.html.haml +++ b/app/views/groups/settings/ci_cd/_form.html.haml @@ -4,10 +4,10 @@ = form_errors(group) %fieldset.builds-feature .form-group - = f.label :max_artifacts_size, _('Maximum artifacts size (MB)'), class: 'label-bold' + = f.label :max_artifacts_size, _('Maximum artifacts size'), class: 'label-bold' = f.number_field :max_artifacts_size, class: 'form-control' %p.form-text.text-muted - = _("Set the maximum file size for each job's artifacts") + = _("The maximum file size in megabytes for individual job artifacts.") = link_to sprite_icon('question-o'), help_page_path('user/admin_area/settings/continuous_integration', anchor: 'maximum-artifacts-size'), target: '_blank' = f.submit _('Save changes'), class: "btn btn-success" diff --git a/app/views/projects/settings/ci_cd/_form.html.haml b/app/views/projects/settings/ci_cd/_form.html.haml index d247e73a5b4..5e52eb1023c 100644 --- a/app/views/projects/settings/ci_cd/_form.html.haml +++ b/app/views/projects/settings/ci_cd/_form.html.haml @@ -4,10 +4,46 @@ = form_errors(@project) %fieldset.builds-feature .form-group + .form-check + = f.check_box :public_builds, { class: 'form-check-input' } + = f.label :public_builds, class: 'form-check-label' do + %strong= _("Public pipelines") + .form-text.text-muted + = _("Allow public access to pipelines and job details, including output logs and artifacts.") + = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'visibility-of-pipelines'), target: '_blank' + + .form-group + .form-check + = f.check_box :auto_cancel_pending_pipelines, { class: 'form-check-input' }, 'enabled', 'disabled' + = f.label :auto_cancel_pending_pipelines, class: 'form-check-label' do + %strong= _("Auto-cancel redundant pipelines") + .form-text.text-muted + = _("New pipelines cause older pending pipelines on the same branch to be cancelled.") + = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'auto-cancel-redundant-pipelines'), target: '_blank' + + .form-group + .form-check + = f.fields_for :ci_cd_settings_attributes, @project.ci_cd_settings do |form| + = form.check_box :forward_deployment_enabled, { class: 'form-check-input' } + = form.label :forward_deployment_enabled, class: 'form-check-label' do + %strong= _("Skip outdated deployment jobs") + .form-text.text-muted + = _("When a deployment job is successful, skip older deployment jobs that are still pending.") + = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'skip-outdated-deployment-jobs'), target: '_blank' + + .form-group + = f.label :ci_config_path, _('CI/CD configuration file'), class: 'label-bold' + = f.text_field :ci_config_path, class: 'form-control', placeholder: '.gitlab-ci.yml' + %p.form-text.text-muted + = html_escape(_("The name of the CI/CD configuration file. A path relative to the root directory is optional (for example %{code_open}my/path/.myfile.yml%{code_close}).")) % { code_open: '<code>'.html_safe, code_close: '</code>'.html_safe } + = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'custom-cicd-configuration-path'), target: '_blank' + + %hr + .form-group %h5.gl-mt-0 - = _("Git strategy for pipelines") + = _("Git strategy") %p - = html_escape(_("Choose between %{code_open}clone%{code_close} or %{code_open}fetch%{code_close} to get the recent application code")) % { code_open: '<code>'.html_safe, code_close: '</code>'.html_safe } + = _("Choose which Git strategy to use when fetching the project.") = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'git-strategy'), target: '_blank' .form-check = f.radio_button :build_allow_git_fetch, 'false', { class: 'form-check-input' } @@ -15,90 +51,39 @@ %strong git clone %br %span - = _("Slower but makes sure the project workspace is pristine as it clones the repository from scratch for every job") + = _("For each job, clone the repository.") .form-check = f.radio_button :build_allow_git_fetch, 'true', { class: 'form-check-input' } = f.label :build_allow_git_fetch_true, class: 'form-check-label' do %strong git fetch %br %span - = _("Faster as it re-uses the project workspace (falling back to clone if it doesn't exist)") + = html_escape(_("For each job, re-use the project workspace. If the workspace doesn't exist, use %{code_open}git clone%{code_close}.")) % { code_open: '<code>'.html_safe, code_close: '</code>'.html_safe } - %hr .form-group = f.fields_for :ci_cd_settings_attributes, @project.ci_cd_settings do |form| = form.label :default_git_depth, _('Git shallow clone'), class: 'label-bold' = form.number_field :default_git_depth, { class: 'form-control', min: 0, max: 1000 } %p.form-text.text-muted - = _('The number of changes to be fetched from GitLab when cloning a repository. This can speed up Pipelines execution. Keep empty or set to 0 to disable shallow clone by default and make GitLab CI fetch all branches and tags each time.') + = html_escape(_('The number of changes to fetch from GitLab when cloning a repository. Lower values can speed up pipeline execution. Set to %{code_open}0%{code_close} or blank to fetch all branches and tags for each job')) % { code_open: '<code>'.html_safe, code_close: '</code>'.html_safe } + = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'git-shallow-clone'), target: '_blank' %hr .form-group = f.label :build_timeout_human_readable, _('Timeout'), class: 'label-bold' = f.text_field :build_timeout_human_readable, class: 'form-control' %p.form-text.text-muted - = _('If any job surpasses this timeout threshold, it will be marked as failed. Human readable time input language is accepted like "1 hour". Values without specification represent seconds.') + = html_escape(_('Jobs fail if they run longer than the timeout time. Input value is in seconds by default. Human readable input is also accepted, for example %{code_open}1 hour%{code_close}.')) % { code_open: '<code>'.html_safe, code_close: '</code>'.html_safe } = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'timeout'), target: '_blank' - if can?(current_user, :update_max_artifacts_size, @project) - %hr .form-group - = f.label :max_artifacts_size, _('Maximum artifacts size (MB)'), class: 'label-bold' + = f.label :max_artifacts_size, _('Maximum artifacts size'), class: 'label-bold' = f.number_field :max_artifacts_size, class: 'form-control' %p.form-text.text-muted - = _("Set the maximum file size for each job's artifacts") + = _("The maximum file size in megabytes for individual job artifacts.") = link_to sprite_icon('question-o'), help_page_path('user/admin_area/settings/continuous_integration', anchor: 'maximum-artifacts-size'), target: '_blank' - %hr - .form-group - = f.label :ci_config_path, _('Custom CI configuration path'), class: 'label-bold' - = f.text_field :ci_config_path, class: 'form-control', placeholder: '.gitlab-ci.yml' - %p.form-text.text-muted - = html_escape(_("The path to the CI configuration file. Defaults to %{code_open}.gitlab-ci.yml%{code_close}")) % { code_open: '<code>'.html_safe, code_close: '</code>'.html_safe } - = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'custom-ci-configuration-path'), target: '_blank' - - %hr - .form-group - .form-check - = f.check_box :public_builds, { class: 'form-check-input' } - = f.label :public_builds, class: 'form-check-label' do - %strong= _("Public pipelines") - .form-text.text-muted - = _("Allow public access to pipelines and job details, including output logs and artifacts") - = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'visibility-of-pipelines'), target: '_blank' - .bs-callout.bs-callout-info - %p #{_("If enabled")}: - %ul - %li - = _("For public projects, anyone can view pipelines and access job details (output logs and artifacts)") - %li - = _("For internal projects, any logged in user except external users can view pipelines and access job details (output logs and artifacts)") - %li - = _("For private projects, any member (guest or higher) can view pipelines and access job details (output logs and artifacts)") - %p - = _("If disabled, the access level will depend on the user's permissions in the project.") - - %hr - .form-group - .form-check - = f.check_box :auto_cancel_pending_pipelines, { class: 'form-check-input' }, 'enabled', 'disabled' - = f.label :auto_cancel_pending_pipelines, class: 'form-check-label' do - %strong= _("Auto-cancel redundant, pending pipelines") - .form-text.text-muted - = _("New pipelines will cancel older, pending pipelines on the same branch") - = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'auto-cancel-pending-pipelines'), target: '_blank' - - .form-group - .form-check - = f.fields_for :ci_cd_settings_attributes, @project.ci_cd_settings do |form| - = form.check_box :forward_deployment_enabled, { class: 'form-check-input' } - = form.label :forward_deployment_enabled, class: 'form-check-label' do - %strong= _("Skip outdated deployment jobs") - .form-text.text-muted - = _("When a deployment job is successful, skip older deployment jobs that are still pending") - = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'skip-outdated-deployment-jobs'), target: '_blank' - - %hr .form-group = f.label :build_coverage_regex, _("Test coverage parsing"), class: 'label-bold' .input-group @@ -108,44 +93,8 @@ %span.input-group-append .input-group-text / %p.form-text.text-muted - = _("A regular expression that will be used to find the test coverage output in the job log. Leave blank to disable") + = html_escape(_('The regular expression used to find test coverage output in the job log. For example, use %{regex} for Simplecov (Ruby). Leave blank to disable.')) % { regex: '<code>\(\d+.\d+%\)</code>'.html_safe } = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'test-coverage-parsing'), target: '_blank' - .bs-callout.bs-callout-info - %p= _("Below are examples of regex for existing tools:") - %ul - %li - Simplecov (Ruby) - - %code \(\d+.\d+\%\) covered - %li - pytest-cov (Python) - - %code ^TOTAL.+?(\d+\%)$ - %li - Scoverage (Scala) - - %code Statement coverage[A-Za-z\.*]\s*:\s*([^%]+) - %li - phpunit --coverage-text --colors=never (PHP) - - %code ^\s*Lines:\s*\d+.\d+\% - %li - gcovr (C/C++) - - %code ^TOTAL.*\s+(\d+\%)$ - %li - tap --coverage-report=text-summary (NodeJS) - - %code ^Statements\s*:\s*([^%]+) - %li - nyc npm test (NodeJS) - - %code All files[^|]*\|[^|]*\s+([\d\.]+) - %li - excoveralls (Elixir) - - %code \[TOTAL\]\s+(\d+\.\d+)% - %li - mix test --cover (Elixir) - - %code \d+.\d+\%\s+\|\s+Total - %li - JaCoCo (Java/Kotlin) - %code Total.*?([0-9]{1,3})% - %li - go test -cover (Go) - %code coverage: \d+.\d+% of statements = f.submit _('Save changes'), class: "btn btn-success", data: { qa_selector: 'save_general_pipelines_changes_button' } diff --git a/app/views/projects/settings/ci_cd/show.html.haml b/app/views/projects/settings/ci_cd/show.html.haml index 157a87b4868..f7053561dfa 100644 --- a/app/views/projects/settings/ci_cd/show.html.haml +++ b/app/views/projects/settings/ci_cd/show.html.haml @@ -12,7 +12,7 @@ %button.btn.js-settings-toggle{ type: 'button' } = expanded ? _('Collapse') : _('Expand') %p - = _("Customize your pipeline configuration, view your pipeline status and coverage report.") + = _("Customize your pipeline configuration and coverage report.") .settings-content = render 'form' diff --git a/changelogs/unreleased/298822-ci-cd-general-pipelines-settings-ui-text.yml b/changelogs/unreleased/298822-ci-cd-general-pipelines-settings-ui-text.yml new file mode 100644 index 00000000000..279be572394 --- /dev/null +++ b/changelogs/unreleased/298822-ci-cd-general-pipelines-settings-ui-text.yml @@ -0,0 +1,5 @@ +--- +title: Update CI general pipeline settings UI text +merge_request: 51806 +author: +type: other diff --git a/changelogs/unreleased/fix-pipeline-and-stage-status.yml b/changelogs/unreleased/fix-pipeline-and-stage-status.yml new file mode 100644 index 00000000000..6a3f4e7e31d --- /dev/null +++ b/changelogs/unreleased/fix-pipeline-and-stage-status.yml @@ -0,0 +1,5 @@ +--- +title: Fix pipeline and stage show success without considering bridge status +merge_request: 52192 +author: Cong Chen @gentcys +type: fixed diff --git a/config/feature_flags/development/optimized_merge_request_count_with_merged_at_filter.yml b/config/feature_flags/development/optimized_merge_request_count_with_merged_at_filter.yml new file mode 100644 index 00000000000..f27e14aab6c --- /dev/null +++ b/config/feature_flags/development/optimized_merge_request_count_with_merged_at_filter.yml @@ -0,0 +1,8 @@ +--- +name: optimized_merge_request_count_with_merged_at_filter +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52113 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/299347 +milestone: '13.9' +type: development +group: group::optimize +default_enabled: false diff --git a/doc/administration/compliance.md b/doc/administration/compliance.md index 6079e838ac6..09fe447fd53 100644 --- a/doc/administration/compliance.md +++ b/doc/administration/compliance.md @@ -26,4 +26,4 @@ relevant compliance standards. |**[Audit events](audit_events.md)**<br>To maintain the integrity of your code, GitLab Enterprise Edition Premium gives admins the ability to view any modifications made within the GitLab server in an advanced audit events system, so you can control, analyze, and track every change.|Premium+|✓|Instance, Group, Project| |**[Auditor users](auditor_users.md)**<br>Auditor users are users who are given read-only access to all projects, groups, and other resources on the GitLab instance.|Premium+||Instance| |**[Credentials inventory](../user/admin_area/credentials_inventory.md)**<br>With a credentials inventory, GitLab administrators can keep track of the credentials used by all of the users in their GitLab instance. |Ultimate||Instance| -|**Separation of Duties using [Protected branches](../user/project/protected_branches.md#protected-branches-approval-by-code-owners) and [custom CI Configuration Paths](../ci/pipelines/settings.md#custom-ci-configuration-path)**<br> GitLab Silver and Premium users can leverage the GitLab cross-project YAML configurations to define deployers of code and developers of code. View the [Separation of Duties Deploy Project](https://gitlab.com/guided-explorations/separation-of-duties-deploy/blob/master/README.md) and [Separation of Duties Project](https://gitlab.com/guided-explorations/separation-of-duties/blob/master/README.md) to see how to use this set up to define these roles.|Premium+|✓|Project| +|**Separation of Duties using [Protected branches](../user/project/protected_branches.md#protected-branches-approval-by-code-owners) and [custom CI Configuration Paths](../ci/pipelines/settings.md#custom-cicd-configuration-path)**<br> GitLab Silver and Premium users can leverage the GitLab cross-project YAML configurations to define deployers of code and developers of code. View the [Separation of Duties Deploy Project](https://gitlab.com/guided-explorations/separation-of-duties-deploy/blob/master/README.md) and [Separation of Duties Project](https://gitlab.com/guided-explorations/separation-of-duties/blob/master/README.md) to see how to use this set up to define these roles.|Premium+|✓|Project| diff --git a/doc/ci/README.md b/doc/ci/README.md index 1a3b685eac6..3aca4e1d63d 100644 --- a/doc/ci/README.md +++ b/doc/ci/README.md @@ -105,7 +105,7 @@ GitLab CI/CD supports numerous configuration options: | Configuration | Description | |:----------------------------------------------------------------------------------------|:------------------------------------------------------------------------------------------| | [Schedule pipelines](pipelines/schedules.md) | Schedule pipelines to run as often as you need. | -| [Custom path for `.gitlab-ci.yml`](pipelines/settings.md#custom-ci-configuration-path) | Define a custom path for the CI/CD configuration file. | +| [Custom path for `.gitlab-ci.yml`](pipelines/settings.md#custom-cicd-configuration-path) | Define a custom path for the CI/CD configuration file. | | [Git submodules for CI/CD](git_submodules.md) | Configure jobs for using Git submodules. | | [SSH keys for CI/CD](ssh_keys/README.md) | Using SSH keys in your CI pipelines. | | [Pipeline triggers](triggers/README.md) | Trigger pipelines through the API. | diff --git a/doc/ci/environments/deployment_safety.md b/doc/ci/environments/deployment_safety.md index 4dac076ffb7..9ae79ef40e8 100644 --- a/doc/ci/environments/deployment_safety.md +++ b/doc/ci/environments/deployment_safety.md @@ -141,7 +141,7 @@ reference a file in another project with a completely different set of permissio In this scenario, the `gitlab-ci.yml` is publicly accessible, but can only be edited by users with appropriate permissions in the other project. -For more information, see [Custom CI configuration path](../pipelines/settings.md#custom-ci-configuration-path). +For more information, see [Custom CI/CD configuration path](../pipelines/settings.md#custom-cicd-configuration-path). ## Troubleshooting diff --git a/doc/ci/pipelines/settings.md b/doc/ci/pipelines/settings.md index 5a758bccd62..36eb343a5db 100644 --- a/doc/ci/pipelines/settings.md +++ b/doc/ci/pipelines/settings.md @@ -68,7 +68,7 @@ Project defined timeout (either specific timeout set by user or the default For information about setting a maximum artifact size for a project, see [Maximum artifacts size](../../user/admin_area/settings/continuous_integration.md#maximum-artifacts-size). -## Custom CI configuration path +## Custom CI/CD configuration path > - [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/12509) in GitLab 9.4. > - [Support for external `.gitlab-ci.yml` locations](https://gitlab.com/gitlab-org/gitlab/-/issues/14376) introduced in GitLab 12.6. @@ -80,7 +80,7 @@ To customize the path: 1. Go to the project's **Settings > CI / CD**. 1. Expand the **General pipelines** section. -1. Provide a value in the **Custom CI configuration path** field. +1. Provide a value in the **CI/CD configuration file** field. 1. Click **Save changes**. If the CI configuration is stored within the repository in a non-default @@ -131,8 +131,19 @@ averaged.  -A few examples of known coverage tools for a variety of languages can be found -in the pipelines settings page. +| Coverage Tool | Sample regular expression | +|------------------------------------------------|---------------------------| +| Simplecov (Ruby) | `\(\d+.\d+\%\) covered` | +| pytest-cov (Python) | `^TOTAL.+?(\d+\%)$` | +| Scoverage (Scala) | `Statement coverage[A-Za-z\.*]\s*:\s*([^%]+)` | +| `phpunit --coverage-text --colors=never` (PHP) | `^\s*Lines:\s*\d+.\d+\%` | +| gcovr (C/C++) | `^TOTAL.*\s+(\d+\%)$` | +| `tap --coverage-report=text-summary` (NodeJS) | `^Statements\s*:\s*([^%]+)` | +| `nyc npm test` (NodeJS) | `All files[^|]*\|[^|]*\s+([\d\.]+)` | +| excoveralls (Elixir) | `\[TOTAL\]\s+(\d+\.\d+)%` | +| `mix test --cover` (Elixir) | `\d+.\d+\%\s+\|\s+Total` | +| JaCoCo (Java/Kotlin) | `Total.*?([0-9]{1,3})%` | +| `go test -cover` (Go) | `coverage: \d+.\d+% of statements` | ### Code Coverage history @@ -198,7 +209,7 @@ If **Public pipelines** is disabled: - For **private** projects, only project members (reporter or higher) can view the pipelines or access the related features. -## Auto-cancel pending pipelines +## Auto-cancel redundant pipelines > [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/9362) in GitLab 9.1. @@ -206,7 +217,7 @@ You can set pending or running pipelines to cancel automatically when a new pipe 1. Go to **Settings > CI / CD**. 1. Expand **General Pipelines**. -1. Check the **Auto-cancel redundant, pending pipelines** checkbox. +1. Check the **Auto-cancel redundant pipelines** checkbox. 1. Click **Save changes**. Use the [`interruptible`](../yaml/README.md#interruptible) keyword to indicate if a diff --git a/doc/ci/unit_test_reports.md b/doc/ci/unit_test_reports.md index 1fec1f77bc3..58308f71217 100644 --- a/doc/ci/unit_test_reports.md +++ b/doc/ci/unit_test_reports.md @@ -290,6 +290,22 @@ javascript: - junit.xml ``` +### Flutter / Dart example + +This example `.gitlab-ci.yml` file uses the [JUnit Report](https://pub.dev/packages/junitreport) package to convert the `flutter test` output into JUnit report XML format: + +```yaml +test: + stage: test + script: + - flutter test --machine | tojunit -o report.xml + artifacts: + when: always + reports: + junit: + - report.xml +``` + ## Viewing Unit test reports on GitLab > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/24792) in GitLab 12.5 behind a feature flag (`junit_pipeline_view`), disabled by default. diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index 3928bca4c60..37335bb7781 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -618,7 +618,7 @@ variables, an `Insufficient permissions to set pipeline variables` error occurs. The setting is `disabled` by default. -If you [store your CI configurations in a different repository](../../ci/pipelines/settings.md#custom-ci-configuration-path), +If you [store your CI configurations in a different repository](../../ci/pipelines/settings.md#custom-cicd-configuration-path), use this setting for strict control over all aspects of the environment the pipeline runs in. diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index 61280a15585..56634ade3f4 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -3845,7 +3845,7 @@ The trigger token is different than the [`trigger`](#trigger) keyword. Use `interruptible` to indicate that a running job should be canceled if made redundant by a newer pipeline run. Defaults to `false` (uninterruptible). Jobs that have not started yet (pending) are considered interruptible and safe to be cancelled. -This value is used only if the [automatic cancellation of redundant pipelines feature](../pipelines/settings.md#auto-cancel-pending-pipelines) +This value is used only if the [automatic cancellation of redundant pipelines feature](../pipelines/settings.md#auto-cancel-redundant-pipelines) is enabled. When enabled, a pipeline is immediately canceled when a new pipeline starts on the same branch if either of the following is true: diff --git a/doc/development/module_with_instance_variables.md b/doc/development/module_with_instance_variables.md index 75575105178..16c7a807053 100644 --- a/doc/development/module_with_instance_variables.md +++ b/doc/development/module_with_instance_variables.md @@ -121,71 +121,7 @@ module Gitlab end ``` -Now the cop doesn't complain. Here's a bad example which we could rewrite: - -``` ruby -module SpamCheckService - def filter_spam_check_params - @request = params.delete(:request) - @api = params.delete(:api) - @recaptcha_verified = params.delete(:recaptcha_verified) - @spam_log_id = params.delete(:spam_log_id) - end - - def spam_check(spammable, user) - spam_service = SpamService.new(spammable, @request) - - spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do - user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true) - end - end -end -``` - -There are several implicit dependencies here. First, `params` should be -defined before use. Second, `filter_spam_check_params` should be called -before `spam_check`. These are all implicit and the includer could be using -those instance variables without awareness. - -This should be rewritten like: - -``` ruby -class SpamCheckService - def initialize(request:, api:, recaptcha_verified:, spam_log_id:) - @request = request - @api = api - @recaptcha_verified = recaptcha_verified - @spam_log_id = spam_log_id - end - - def spam_check(spammable, user) - spam_service = SpamService.new(spammable, @request) - - spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do - user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true) - end - end -end -``` - -And use it like: - -``` ruby -class UpdateSnippetService < BaseService - def execute - # ... - spam = SpamCheckService.new(params.slice!(:request, :api, :recaptcha_verified, :spam_log_id)) - - spam.check(snippet, current_user) - # ... - end -end -``` - -This way, all those instance variables are isolated in `SpamCheckService` -rather than whatever includes the module, and those modules which were also -included, making it much easier to track down any issues, -and reducing the chance of having name conflicts. +Now the cop doesn't complain. ## How to disable this cop diff --git a/doc/user/admin_area/settings/continuous_integration.md b/doc/user/admin_area/settings/continuous_integration.md index 6418be13ee9..5f65277813d 100644 --- a/doc/user/admin_area/settings/continuous_integration.md +++ b/doc/user/admin_area/settings/continuous_integration.md @@ -156,7 +156,7 @@ Area of your GitLab instance (`.gitlab-ci.yml` if not set): 1. Input the new path in the **Default CI configuration path** field. 1. Hit **Save changes** for the changes to take effect. -It is also possible to specify a [custom CI configuration path for a specific project](../../../ci/pipelines/settings.md#custom-ci-configuration-path). +It is also possible to specify a [custom CI/CD configuration path for a specific project](../../../ci/pipelines/settings.md#custom-cicd-configuration-path). <!-- ## Troubleshooting diff --git a/lib/gitlab/recaptcha.rb b/lib/gitlab/recaptcha.rb index f3cbe1db901..a08cea5a435 100644 --- a/lib/gitlab/recaptcha.rb +++ b/lib/gitlab/recaptcha.rb @@ -2,8 +2,10 @@ module Gitlab module Recaptcha + extend Gitlab::Utils::StrongMemoize + def self.load_configurations! - if Gitlab::CurrentSettings.recaptcha_enabled || enabled_on_login? + if enabled? || enabled_on_login? ::Recaptcha.configure do |config| config.site_key = Gitlab::CurrentSettings.recaptcha_site_key config.secret_key = Gitlab::CurrentSettings.recaptcha_private_key diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 484a4cf94b6..b427233886f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1337,9 +1337,6 @@ msgstr "" msgid "A rebase is already in progress." msgstr "" -msgid "A regular expression that will be used to find the test coverage output in the job log. Leave blank to disable" -msgstr "" - msgid "A secure token that identifies an external storage request." msgstr "" @@ -2913,7 +2910,7 @@ msgstr "" msgid "Allow projects within this group to use Git LFS" msgstr "" -msgid "Allow public access to pipelines and job details, including output logs and artifacts" +msgid "Allow public access to pipelines and job details, including output logs and artifacts." msgstr "" msgid "Allow rendering of PlantUML diagrams in Asciidoc documents." @@ -4187,7 +4184,7 @@ msgstr "" msgid "Auto stop successfully canceled." msgstr "" -msgid "Auto-cancel redundant, pending pipelines" +msgid "Auto-cancel redundant pipelines" msgstr "" msgid "Auto-close referenced issues on default branch" @@ -4436,9 +4433,6 @@ msgstr "" msgid "Begin with the selected commit" msgstr "" -msgid "Below are examples of regex for existing tools:" -msgstr "" - msgid "Below are the fingerprints for the current instance SSH host keys." msgstr "" @@ -4466,6 +4460,9 @@ msgstr "" msgid "BillingPlans|Congratulations, your free trial is activated." msgstr "" +msgid "BillingPlans|End of availability for the Bronze Plan" +msgstr "" + msgid "BillingPlans|Free upgrade!" msgstr "" @@ -4493,6 +4490,9 @@ msgstr "" msgid "BillingPlans|To manage the plan for this group, visit the billing section of %{parent_billing_page_link}." msgstr "" +msgid "BillingPlans|While GitLab is ending availability of the Bronze plan, you can still renew your Bronze subscription one additional time before %{eoa_bronze_plan_end_date}. We are also offering a limited time free upgrade to our Premium Plan (up to 25 users)! Learn more about the changes and offers in our %{announcement_link}." +msgstr "" + msgid "BillingPlans|Your GitLab.com %{plan} trial will %{strong_open}expire after %{expiration_date}%{strong_close}. You can retain access to the %{plan} features by upgrading below." msgstr "" @@ -4981,6 +4981,9 @@ msgstr "" msgid "CI/CD configuration" msgstr "" +msgid "CI/CD configuration file" +msgstr "" + msgid "CI/CD for external repo" msgstr "" @@ -5635,9 +5638,6 @@ msgstr "" msgid "Choose any color. Or you can choose one of the suggested colors below" msgstr "" -msgid "Choose between %{code_open}clone%{code_close} or %{code_open}fetch%{code_close} to get the recent application code" -msgstr "" - msgid "Choose file…" msgstr "" @@ -5656,6 +5656,9 @@ msgstr "" msgid "Choose what content you want to see on a group’s overview page." msgstr "" +msgid "Choose which Git strategy to use when fetching the project." +msgstr "" + msgid "Choose which repositories you want to connect and run CI/CD pipelines." msgstr "" @@ -8567,9 +8570,6 @@ msgstr "" msgid "Custom Attributes" msgstr "" -msgid "Custom CI configuration path" -msgstr "" - msgid "Custom Git clone URL for HTTP(S)" msgstr "" @@ -8642,7 +8642,7 @@ msgstr "" msgid "Customize name" msgstr "" -msgid "Customize your pipeline configuration, view your pipeline status and coverage report." +msgid "Customize your pipeline configuration and coverage report." msgstr "" msgid "Customize your pipeline configuration." @@ -12183,9 +12183,6 @@ msgstr "" msgid "Fast-forward merge without a merge commit" msgstr "" -msgid "Faster as it re-uses the project workspace (falling back to clone if it doesn't exist)" -msgstr "" - msgid "Faster releases. Better code. Less pain." msgstr "" @@ -12707,13 +12704,16 @@ msgstr "" msgid "For additional information, review your group membership: %{link_to} or contact your group owner." msgstr "" -msgid "For help setting up the Service Desk for your instance, please contact an administrator." +msgid "For each job, clone the repository." msgstr "" -msgid "For individual use, create a separate account under your personal email address, not tied to the Enterprise email domain or group." +msgid "For each job, re-use the project workspace. If the workspace doesn't exist, use %{code_open}git clone%{code_close}." +msgstr "" + +msgid "For help setting up the Service Desk for your instance, please contact an administrator." msgstr "" -msgid "For internal projects, any logged in user except external users can view pipelines and access job details (output logs and artifacts)" +msgid "For individual use, create a separate account under your personal email address, not tied to the Enterprise email domain or group." msgstr "" msgid "For more info, read the documentation." @@ -12737,12 +12737,6 @@ msgstr "" msgid "For more information, see the documentation on %{link_start}disabling Seat Link%{link_end}." msgstr "" -msgid "For private projects, any member (guest or higher) can view pipelines and access job details (output logs and artifacts)" -msgstr "" - -msgid "For public projects, anyone can view pipelines and access job details (output logs and artifacts)" -msgstr "" - msgid "Forgot your password?" msgstr "" @@ -13283,7 +13277,7 @@ msgstr "" msgid "Git shallow clone" msgstr "" -msgid "Git strategy for pipelines" +msgid "Git strategy" msgstr "" msgid "Git transfer in progress" @@ -14673,9 +14667,6 @@ msgstr "" msgid "If any indexed field exceeds this limit it will be truncated to this number of characters and the rest will not be indexed or searchable. This does not apply to repository and wiki indexing. Setting this to 0 means it is unlimited." msgstr "" -msgid "If any job surpasses this timeout threshold, it will be marked as failed. Human readable time input language is accepted like \"1 hour\". Values without specification represent seconds." -msgstr "" - msgid "If blank, set allowable lifetime to %{instance_level_policy_in_words}, as defined by the instance admin. Once set, existing tokens for users in this group may be revoked." msgstr "" @@ -14691,12 +14682,6 @@ msgstr "" msgid "If disabled, only admins will be able to configure repository mirroring." msgstr "" -msgid "If disabled, the access level will depend on the user's permissions in the project." -msgstr "" - -msgid "If enabled" -msgstr "" - msgid "If enabled, GitLab will handle Object Storage replication using Geo. %{linkStart}More information%{linkEnd}" msgstr "" @@ -16274,6 +16259,9 @@ msgstr "" msgid "Jobs" msgstr "" +msgid "Jobs fail if they run longer than the timeout time. Input value is in seconds by default. Human readable input is also accepted, for example %{code_open}1 hour%{code_close}." +msgstr "" + msgid "Jobs|Are you sure you want to proceed?" msgstr "" @@ -17475,6 +17463,9 @@ msgstr "" msgid "Maximum allowable lifetime for personal access token (days)" msgstr "" +msgid "Maximum artifacts size" +msgstr "" + msgid "Maximum artifacts size (MB)" msgstr "" @@ -19195,7 +19186,7 @@ msgstr "" msgid "New password" msgstr "" -msgid "New pipelines will cancel older, pending pipelines on the same branch" +msgid "New pipelines cause older pending pipelines on the same branch to be cancelled." msgstr "" msgid "New project" @@ -26410,9 +26401,6 @@ msgstr "" msgid "SlackService|This service allows users to perform common operations on this project by entering slash commands in Slack." msgstr "" -msgid "Slower but makes sure the project workspace is pristine as it clones the repository from scratch for every job" -msgstr "" - msgid "Smartcard" msgstr "" @@ -28352,6 +28340,9 @@ msgstr "" msgid "The maximum file size allowed is %{size}." msgstr "" +msgid "The maximum file size in megabytes for individual job artifacts." +msgstr "" + msgid "The maximum file size is %{size}." msgstr "" @@ -28370,7 +28361,10 @@ msgstr "" msgid "The name \"%{name}\" is already taken in this directory." msgstr "" -msgid "The number of changes to be fetched from GitLab when cloning a repository. This can speed up Pipelines execution. Keep empty or set to 0 to disable shallow clone by default and make GitLab CI fetch all branches and tags each time." +msgid "The name of the CI/CD configuration file. A path relative to the root directory is optional (for example %{code_open}my/path/.myfile.yml%{code_close})." +msgstr "" + +msgid "The number of changes to fetch from GitLab when cloning a repository. Lower values can speed up pipeline execution. Set to %{code_open}0%{code_close} or blank to fetch all branches and tags for each job" msgstr "" msgid "The number of merge requests merged by month." @@ -28388,9 +28382,6 @@ msgstr "" msgid "The passphrase required to decrypt the private key. This is optional and the value is encrypted at rest." msgstr "" -msgid "The path to the CI configuration file. Defaults to %{code_open}.gitlab-ci.yml%{code_close}" -msgstr "" - msgid "The phase of the development lifecycle." msgstr "" @@ -28436,6 +28427,9 @@ msgstr "" msgid "The pseudonymizer data collection is disabled. When enabled, GitLab will run a background job that will produce pseudonymized CSVs of the GitLab database that will be uploaded to your configured object storage directory." msgstr "" +msgid "The regular expression used to find test coverage output in the job log. For example, use %{regex} for Simplecov (Ruby). Leave blank to disable." +msgstr "" + msgid "The remote mirror took to long to complete." msgstr "" @@ -32031,7 +32025,7 @@ msgstr "" msgid "When Kroki is enabled, GitLab sends diagrams to an instance of Kroki to display them as images. You can use the free public cloud instance %{kroki_public_url} or you can %{install_link} on your own infrastructure. Once you've installed Kroki, make sure to update the server URL to point to your instance." msgstr "" -msgid "When a deployment job is successful, skip older deployment jobs that are still pending" +msgid "When a deployment job is successful, skip older deployment jobs that are still pending." msgstr "" msgid "When a runner is locked, it cannot be assigned to other projects" diff --git a/spec/controllers/concerns/spammable_actions_spec.rb b/spec/controllers/concerns/spammable_actions_spec.rb index 3b5b4d11a9b..21732dba282 100644 --- a/spec/controllers/concerns/spammable_actions_spec.rb +++ b/spec/controllers/concerns/spammable_actions_spec.rb @@ -6,21 +6,8 @@ RSpec.describe SpammableActions do controller(ActionController::Base) do include SpammableActions - # #create is used to test spammable_params - # for testing purposes - def create - spam_params = spammable_params - - # replace the actual request with a string in the JSON response, all we care is that it got set - spam_params[:request] = 'this is the request' if spam_params[:request] - - # just return the params in the response so they can be verified in this fake controller spec. - # Normally, they are processed further by the controller action - render json: spam_params.to_json, status: :ok - end - - # #update is used to test recaptcha_check_with_fallback - # for testing purposes + # #update is used here to test #recaptcha_check_with_fallback, but it could be invoked + # from #create or any other action which mutates a spammable via a controller. def update should_redirect = params[:should_redirect] == 'true' @@ -35,80 +22,7 @@ RSpec.describe SpammableActions do end before do - # Ordinarily we would not stub a method on the class under test, but :ensure_spam_config_loaded! - # returns false in the test environment, and is also strong_memoized, so we need to stub it - allow(controller).to receive(:ensure_spam_config_loaded!) { true } - end - - describe '#spammable_params' do - subject { post :create, format: :json, params: params } - - shared_examples 'expects request param only' do - it do - subject - - expect(response).to be_successful - expect(json_response).to eq({ 'request' => 'this is the request' }) - end - end - - shared_examples 'expects all spammable params' do - it do - subject - - expect(response).to be_successful - expect(json_response['request']).to eq('this is the request') - expect(json_response['recaptcha_verified']).to eq(true) - expect(json_response['spam_log_id']).to eq('1') - end - end - - let(:recaptcha_response) { nil } - let(:spam_log_id) { nil } - - context 'when recaptcha response is not present' do - let(:params) do - { - spam_log_id: spam_log_id - } - end - - it_behaves_like 'expects request param only' - end - - context 'when recaptcha response is present' do - let(:recaptcha_response) { 'abd123' } - let(:params) do - { - 'g-recaptcha-response': recaptcha_response, - spam_log_id: spam_log_id - } - end - - context 'when verify_recaptcha returns falsey' do - before do - expect(controller).to receive(:verify_recaptcha).with( - { - response: recaptcha_response - }) { false } - end - - it_behaves_like 'expects request param only' - end - - context 'when verify_recaptcha returns truthy' do - let(:spam_log_id) { 1 } - - before do - expect(controller).to receive(:verify_recaptcha).with( - { - response: recaptcha_response - }) { true } - end - - it_behaves_like 'expects all spammable params' - end - end + allow(Gitlab::Recaptcha).to receive(:load_configurations!) { true } end describe '#recaptcha_check_with_fallback' do diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 52ad7efff12..81ffd2c4512 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1281,11 +1281,13 @@ RSpec.describe Projects::IssuesController do let!(:last_spam_log) { spam_logs.last } def post_verified_issue - post_new_issue({}, { spam_log_id: last_spam_log.id, 'g-recaptcha-response': true } ) + post_new_issue({}, { spam_log_id: last_spam_log.id, 'g-recaptcha-response': 'abc123' } ) end before do - expect(controller).to receive_messages(verify_recaptcha: true) + expect_next_instance_of(Captcha::CaptchaVerificationService) do |instance| + expect(instance).to receive(:execute) { true } + end end it 'accepts an issue after reCAPTCHA is verified' do diff --git a/spec/factories/ci/bridge.rb b/spec/factories/ci/bridge.rb index 7727a468633..7258b3367d3 100644 --- a/spec/factories/ci/bridge.rb +++ b/spec/factories/ci/bridge.rb @@ -53,6 +53,11 @@ FactoryBot.define do finished_at { '2013-10-29 09:53:28 CET' } end + trait :success do + finished + status { 'success' } + end + trait :failed do finished status { 'failed' } @@ -75,5 +80,9 @@ FactoryBot.define do trait :playable do manual end + + trait :allowed_to_fail do + allow_failure { true } + end end end diff --git a/spec/features/projects/settings/pipelines_settings_spec.rb b/spec/features/projects/settings/pipelines_settings_spec.rb index c087237fd7c..39c4315bf0f 100644 --- a/spec/features/projects/settings/pipelines_settings_spec.rb +++ b/spec/features/projects/settings/pipelines_settings_spec.rb @@ -46,7 +46,7 @@ RSpec.describe "Projects > Settings > Pipelines settings" do it 'updates auto_cancel_pending_pipelines' do visit project_settings_ci_cd_path(project) - page.check('Auto-cancel redundant, pending pipelines') + page.check('Auto-cancel redundant pipelines') page.within '#js-general-pipeline-settings' do click_on 'Save changes' end diff --git a/spec/finders/merge_request/metrics_finder_spec.rb b/spec/finders/merge_request/metrics_finder_spec.rb new file mode 100644 index 00000000000..ea039462e66 --- /dev/null +++ b/spec/finders/merge_request/metrics_finder_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequest::MetricsFinder do + let_it_be(:current_user) { create(:user) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:merge_request_not_merged) { create(:merge_request, :unique_branches, source_project: project) } + let_it_be(:merged_at) { Time.new(2020, 5, 1) } + let_it_be(:merge_request_merged) do + create(:merge_request, :unique_branches, :merged, source_project: project).tap do |mr| + mr.metrics.update!(merged_at: merged_at) + end + end + + let(:params) do + { + target_project: project, + merged_after: merged_at - 10.days, + merged_before: merged_at + 10.days + } + end + + subject { described_class.new(current_user, params).execute.to_a } + + context 'when target project is missing' do + before do + params.delete(:target_project) + end + + it { is_expected.to be_empty } + end + + context 'when the user is not part of the project' do + it { is_expected.to be_empty } + end + + context 'when user is part of the project' do + before do + project.add_developer(current_user) + end + + it 'returns merge request records' do + is_expected.to eq([merge_request_merged.metrics]) + end + + it 'excludes not merged records' do + is_expected.not_to eq([merge_request_not_merged.metrics]) + end + + context 'when only merged_before is given' do + before do + params.delete(:merged_after) + end + + it { is_expected.to eq([merge_request_merged.metrics]) } + end + + context 'when only merged_after is given' do + before do + params.delete(:merged_before) + end + + it { is_expected.to eq([merge_request_merged.metrics]) } + end + + context 'when no records matching the date range' do + before do + params[:merged_before] = merged_at - 1.year + params[:merged_after] = merged_at - 2.years + end + + it { is_expected.to be_empty } + end + end +end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 825513bdfc5..ab9b62e02f0 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -561,6 +561,7 @@ project: - exported_protected_branches - incident_management_oncall_schedules - debian_distributions +- merge_request_metrics award_emoji: - awardable - user @@ -589,6 +590,7 @@ lfs_file_locks: project_badges: - project metrics: +- target_project - merge_request - latest_closed_by - merged_by diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 79eb016bc41..3b155517a31 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1996,13 +1996,34 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do is_expected.to be_falsey end end + + context 'bridge which is allowed to fail fails' do + before do + create :ci_bridge, :allowed_to_fail, :failed, pipeline: pipeline, name: 'rubocop' + end + + it 'returns true' do + is_expected.to be_truthy + end + end + + context 'bridge which is allowed to fail is successful' do + before do + create :ci_bridge, :allowed_to_fail, :success, pipeline: pipeline, name: 'rubocop' + end + + it 'returns false' do + is_expected.to be_falsey + end + end end describe '#number_of_warnings' do it 'returns the number of warnings' do create(:ci_build, :allowed_to_fail, :failed, pipeline: pipeline, name: 'rubocop') + create(:ci_bridge, :allowed_to_fail, :failed, pipeline: pipeline, name: 'rubocop') - expect(pipeline.number_of_warnings).to eq(1) + expect(pipeline.number_of_warnings).to eq(2) end it 'supports eager loading of the number of warnings' do diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb index 3d873a1b9c1..0afc491dc73 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -288,6 +288,7 @@ RSpec.describe Ci::Stage, :models do context 'when stage has warnings' do before do create(:ci_build, :failed, :allowed_to_fail, stage_id: stage.id) + create(:ci_bridge, :failed, :allowed_to_fail, stage_id: stage.id) end describe '#has_warnings?' do @@ -310,7 +311,7 @@ RSpec.describe Ci::Stage, :models do expect(synced_queries.count).to eq 1 expect(stage.number_of_warnings.inspect).to include 'BatchLoader' - expect(stage.number_of_warnings).to eq 1 + expect(stage.number_of_warnings).to eq 2 end end end diff --git a/spec/models/merge_request/metrics_spec.rb b/spec/models/merge_request/metrics_spec.rb index 760eaf1ac7f..13ff239a306 100644 --- a/spec/models/merge_request/metrics_spec.rb +++ b/spec/models/merge_request/metrics_spec.rb @@ -5,6 +5,7 @@ require 'spec_helper' RSpec.describe MergeRequest::Metrics do describe 'associations' do it { is_expected.to belong_to(:merge_request) } + it { is_expected.to belong_to(:target_project).class_name('Project') } it { is_expected.to belong_to(:latest_closed_by).class_name('User') } it { is_expected.to belong_to(:merged_by).class_name('User') } end @@ -36,5 +37,15 @@ RSpec.describe MergeRequest::Metrics do is_expected.not_to include([metrics_2]) end end + + describe '.by_target_project' do + let(:target_project) { metrics_1.target_project } + + subject { described_class.by_target_project(target_project) } + + it 'finds metrics record with the associated target project' do + is_expected.to eq([metrics_1]) + end + end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 67a80924cbc..1b24c1f5436 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -21,6 +21,7 @@ RSpec.describe Project, factory_default: :keep do it { is_expected.to have_many(:services) } it { is_expected.to have_many(:events) } it { is_expected.to have_many(:merge_requests) } + it { is_expected.to have_many(:merge_request_metrics).class_name('MergeRequest::Metrics') } it { is_expected.to have_many(:issues) } it { is_expected.to have_many(:milestones) } it { is_expected.to have_many(:iterations) } diff --git a/spec/requests/api/graphql/project/merge_requests_spec.rb b/spec/requests/api/graphql/project/merge_requests_spec.rb index c85cb8b2ffe..11d04db0011 100644 --- a/spec/requests/api/graphql/project/merge_requests_spec.rb +++ b/spec/requests/api/graphql/project/merge_requests_spec.rb @@ -396,4 +396,85 @@ RSpec.describe 'getting merge request listings nested in a project' do end end end + + context 'when only the count is requested' do + context 'when merged at filter is present' do + let_it_be(:merge_request) do + create(:merge_request, :unique_branches, source_project: project).tap do |mr| + mr.metrics.update!(merged_at: Time.new(2020, 1, 3)) + end + end + + let(:query) do + graphql_query_for(:project, { full_path: project.full_path }, + <<~QUERY + mergeRequests(mergedAfter: "2020-01-01", mergedBefore: "2020-01-05", first: 0) { + count + } + QUERY + ) + end + + shared_examples 'count examples' do + it 'returns the correct count' do + post_graphql(query, current_user: current_user) + + count = graphql_data.dig('project', 'mergeRequests', 'count') + expect(count).to eq(1) + end + end + + context 'when "optimized_merge_request_count_with_merged_at_filter" feature flag is enabled' do + before do + stub_feature_flags(optimized_merge_request_count_with_merged_at_filter: true) + end + + it 'does not query the merge requests table for the count' do + query_recorder = ActiveRecord::QueryRecorder.new { post_graphql(query, current_user: current_user) } + + queries = query_recorder.data.each_value.first[:occurrences] + expect(queries).not_to include(match(/SELECT COUNT\(\*\) FROM "merge_requests"/)) + expect(queries).to include(match(/SELECT COUNT\(\*\) FROM "merge_request_metrics"/)) + end + + context 'when total_time_to_merge and count is queried' do + let(:query) do + graphql_query_for(:project, { full_path: project.full_path }, + <<~QUERY + mergeRequests(mergedAfter: "2020-01-01", mergedBefore: "2020-01-05", first: 0) { + totalTimeToMerge + count + } + QUERY + ) + end + + it 'does not query the merge requests table for the total_time_to_merge' do + query_recorder = ActiveRecord::QueryRecorder.new { post_graphql(query, current_user: current_user) } + + queries = query_recorder.data.each_value.first[:occurrences] + expect(queries).to include(match(/SELECT.+SUM.+FROM "merge_request_metrics" WHERE/)) + end + end + + it_behaves_like 'count examples' + + context 'when "optimized_merge_request_count_with_merged_at_filter" feature flag is disabled' do + before do + stub_feature_flags(optimized_merge_request_count_with_merged_at_filter: false) + end + + it 'queries the merge requests table for the count' do + query_recorder = ActiveRecord::QueryRecorder.new { post_graphql(query, current_user: current_user) } + + queries = query_recorder.data.each_value.first[:occurrences] + expect(queries).to include(match(/SELECT COUNT\(\*\) FROM "merge_requests"/)) + expect(queries).not_to include(match(/SELECT COUNT\(\*\) FROM "merge_request_metrics"/)) + end + + it_behaves_like 'count examples' + end + end + end + end end diff --git a/spec/services/captcha/captcha_verification_service_spec.rb b/spec/services/captcha/captcha_verification_service_spec.rb new file mode 100644 index 00000000000..245e06703f5 --- /dev/null +++ b/spec/services/captcha/captcha_verification_service_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Captcha::CaptchaVerificationService do + describe '#execute' do + let(:captcha_response) { nil } + let(:request) { double(:request) } + let(:service) { described_class.new } + + subject { service.execute(captcha_response: captcha_response, request: request) } + + context 'when there is no captcha_response' do + it 'returns false' do + expect(subject).to eq(false) + end + end + + context 'when there is a captcha_response' do + let(:captcha_response) { 'abc123' } + + before do + expect(Gitlab::Recaptcha).to receive(:load_configurations!) + end + + it 'returns false' do + expect(service).to receive(:verify_recaptcha).with(response: captcha_response) { true } + + expect(subject).to eq(true) + end + + it 'has a request method which returns the request' do + subject + + expect(service.send(:request)).to eq(request) + end + end + end +end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index cc6a49fc4cf..e42e9722297 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -452,162 +452,50 @@ RSpec.describe Issues::CreateService do end context 'checking spam' do - include_context 'includes Spam constants' + let(:request) { double(:request) } + let(:api) { true } + let(:captcha_response) { 'abc123' } + let(:spam_log_id) { 1 } - let(:title) { 'Legit issue' } - let(:description) { 'please fix' } - let(:opts) do + let(:params) do { - title: title, - description: description, - request: double(:request, env: {}) + title: 'Spam issue', + request: request, + api: api, + captcha_response: captcha_response, + spam_log_id: spam_log_id } end - subject { described_class.new(project, user, opts) } - - before do - stub_feature_flags(allow_possible_spam: false) + subject do + described_class.new(project, user, params) end - context 'when reCAPTCHA was verified' do - let(:log_user) { user } - let(:spam_logs) { create_list(:spam_log, 2, user: log_user, title: title) } - let(:target_spam_log) { spam_logs.last } - - before do - opts[:recaptcha_verified] = true - opts[:spam_log_id] = target_spam_log.id - - expect(Spam::SpamVerdictService).not_to receive(:new) - end - - it 'does not mark an issue as spam' do - expect(issue).not_to be_spam - end - - it 'creates a valid issue' do - expect(issue).to be_valid - end - - it 'does not assign a spam_log to the issue' do - expect(issue.spam_log).to be_nil - end - - it 'marks related spam_log as recaptcha_verified' do - expect { issue }.to change { target_spam_log.reload.recaptcha_verified }.from(false).to(true) - end - - context 'when spam log does not belong to a user' do - let(:log_user) { create(:user) } - - it 'does not mark spam_log as recaptcha_verified' do - expect { issue }.not_to change { target_spam_log.reload.recaptcha_verified } - end + before do + allow_next_instance_of(UserAgentDetailService) do |instance| + allow(instance).to receive(:create) end end - context 'when reCAPTCHA was not verified' do - before do - expect_next_instance_of(Spam::SpamActionService) do |spam_service| - expect(spam_service).to receive_messages(check_for_spam?: true) - end - end - - context 'when SpamVerdictService requires reCAPTCHA' do - before do - expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service| - expect(verdict_service).to receive(:execute).and_return(CONDITIONAL_ALLOW) - end - end - - it 'does not mark the issue as spam' do - expect(issue).not_to be_spam - end - - it 'marks the issue as needing reCAPTCHA' do - expect(issue.needs_recaptcha?).to be_truthy - end - - it 'invalidates the issue' do - expect(issue).to be_invalid - end - - it 'creates a new spam_log' do - expect { issue } - .to have_spam_log(title: title, description: description, user_id: user.id, noteable_type: 'Issue') - end - end - - context 'when SpamVerdictService disallows creation' do - before do - expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service| - expect(verdict_service).to receive(:execute).and_return(DISALLOW) - end - end - - context 'when allow_possible_spam feature flag is false' do - it 'marks the issue as spam' do - expect(issue).to be_spam - end - - it 'does not mark the issue as needing reCAPTCHA' do - expect(issue.needs_recaptcha?).to be_falsey - end - - it 'invalidates the issue' do - expect(issue).to be_invalid - end - - it 'creates a new spam_log' do - expect { issue } - .to have_spam_log(title: title, description: description, user_id: user.id, noteable_type: 'Issue') - end - end - - context 'when allow_possible_spam feature flag is true' do - before do - stub_feature_flags(allow_possible_spam: true) - end - - it 'does not mark the issue as spam' do - expect(issue).not_to be_spam - end - - it 'does not mark the issue as needing reCAPTCHA' do - expect(issue.needs_recaptcha?).to be_falsey - end - - it 'creates a valid issue' do - expect(issue).to be_valid - end - - it 'creates a new spam_log' do - expect { issue } - .to have_spam_log(title: title, description: description, user_id: user.id, noteable_type: 'Issue') - end - end + it 'executes SpamActionService' do + spam_params = Spam::SpamParams.new( + api: api, + captcha_response: captcha_response, + spam_log_id: spam_log_id + ) + expect_next_instance_of( + Spam::SpamActionService, + { + spammable: an_instance_of(Issue), + request: request, + user: user, + action: :create + } + ) do |instance| + expect(instance).to receive(:execute).with(spam_params: spam_params) end - context 'when the SpamVerdictService allows creation' do - before do - expect_next_instance_of(Spam::SpamVerdictService) do |verdict_service| - expect(verdict_service).to receive(:execute).and_return(ALLOW) - end - end - - it 'does not mark an issue as spam' do - expect(issue).not_to be_spam - end - - it 'creates a valid issue' do - expect(issue).to be_valid - end - - it 'does not assign a spam_log to an issue' do - expect(issue.spam_log).to be_nil - end - end + subject.execute end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 06a6a52bc41..fd42a84e405 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -711,7 +711,7 @@ RSpec.describe Issues::UpdateService, :mailer do } service = described_class.new(project, user, params) - expect(service).not_to receive(:spam_check) + expect(Spam::SpamActionService).not_to receive(:new) service.execute(issue) end diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index 96807fd629f..32a09e1afc8 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Snippets::CreateService do describe '#execute' do let_it_be(:user) { create(:user) } let_it_be(:admin) { create(:user, :admin) } + let(:action) { :create } let(:opts) { base_opts.merge(extra_opts) } let(:base_opts) do { @@ -309,7 +310,7 @@ RSpec.describe Snippets::CreateService do it_behaves_like 'a service that creates a snippet' it_behaves_like 'public visibility level restrictions apply' - it_behaves_like 'snippets spam check is performed' + it_behaves_like 'checking spam' it_behaves_like 'snippet create data is tracked' it_behaves_like 'an error service response when save fails' it_behaves_like 'creates repository and files' @@ -337,7 +338,7 @@ RSpec.describe Snippets::CreateService do it_behaves_like 'a service that creates a snippet' it_behaves_like 'public visibility level restrictions apply' - it_behaves_like 'snippets spam check is performed' + it_behaves_like 'checking spam' it_behaves_like 'snippet create data is tracked' it_behaves_like 'an error service response when save fails' it_behaves_like 'creates repository and files' diff --git a/spec/services/snippets/update_service_spec.rb b/spec/services/snippets/update_service_spec.rb index a2341dc71b2..e737c00ae67 100644 --- a/spec/services/snippets/update_service_spec.rb +++ b/spec/services/snippets/update_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Snippets::UpdateService do describe '#execute', :aggregate_failures do let_it_be(:user) { create(:user) } let_it_be(:admin) { create :user, admin: true } + let(:action) { :update } let(:visibility_level) { Gitlab::VisibilityLevel::PRIVATE } let(:base_opts) do { @@ -738,11 +739,7 @@ RSpec.describe Snippets::UpdateService do it_behaves_like 'only file_name is present' it_behaves_like 'only content is present' it_behaves_like 'invalid params error response' - it_behaves_like 'snippets spam check is performed' do - before do - subject - end - end + it_behaves_like 'checking spam' context 'when snippet does not have a repository' do let!(:snippet) { create(:project_snippet, author: user, project: project) } @@ -766,11 +763,7 @@ RSpec.describe Snippets::UpdateService do it_behaves_like 'only file_name is present' it_behaves_like 'only content is present' it_behaves_like 'invalid params error response' - it_behaves_like 'snippets spam check is performed' do - before do - subject - end - end + it_behaves_like 'checking spam' context 'when snippet does not have a repository' do let!(:snippet) { create(:personal_snippet, author: user, project: project) } diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb index 8edd9406bce..371923f1518 100644 --- a/spec/services/spam/spam_action_service_spec.rb +++ b/spec/services/spam/spam_action_service_spec.rb @@ -24,41 +24,16 @@ RSpec.describe Spam::SpamActionService do issue.spam = false end - describe '#initialize' do - subject { described_class.new(spammable: issue, request: request, user: user) } - - context 'when the request is nil' do - let(:request) { nil } - - it 'assembles the options with information from the spammable' do - aggregate_failures do - expect(subject.options[:ip_address]).to eq(issue.ip_address) - expect(subject.options[:user_agent]).to eq(issue.user_agent) - expect(subject.options.key?(:referrer)).to be_falsey - end - end - end - - context 'when the request is present' do - let(:request) { double(:request, env: env) } - - it 'assembles the options with information from the spammable' do - aggregate_failures do - expect(subject.options[:ip_address]).to eq(fake_ip) - expect(subject.options[:user_agent]).to eq(fake_user_agent) - expect(subject.options[:referrer]).to eq(fake_referrer) - end - end - end - end - shared_examples 'only checks for spam if a request is provided' do context 'when request is missing' do - subject { described_class.new(spammable: issue, request: nil, user: user) } + let(:request) { nil } it "doesn't check as spam" do - subject + expect(fake_verdict_service).not_to receive(:execute) + + response = subject + expect(response.message).to match(/request was not present/) expect(issue).not_to be_spam end end @@ -66,34 +41,88 @@ RSpec.describe Spam::SpamActionService do context 'when request exists' do it 'creates a spam log' do expect { subject } - .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue') + .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue') end end end + shared_examples 'creates a spam log' do + it do + expect { subject }.to change { SpamLog.count }.by(1) + + new_spam_log = SpamLog.last + expect(new_spam_log.user_id).to eq(user.id) + expect(new_spam_log.title).to eq(issue.title) + expect(new_spam_log.description).to eq(issue.description) + expect(new_spam_log.source_ip).to eq(fake_ip) + expect(new_spam_log.user_agent).to eq(fake_user_agent) + expect(new_spam_log.noteable_type).to eq('Issue') + expect(new_spam_log.via_api).to eq(false) + end + end + describe '#execute' do let(:request) { double(:request, env: env) } + let(:fake_captcha_verification_service) { double(:captcha_verification_service) } let(:fake_verdict_service) { double(:spam_verdict_service) } let(:allowlisted) { false } + let(:api) { nil } + let(:captcha_response) { 'abc123' } + let(:spam_log_id) { existing_spam_log.id } + let(:spam_params) do + Spam::SpamActionService.filter_spam_params!( + api: api, + captcha_response: captcha_response, + spam_log_id: spam_log_id + ) + end + + let(:verdict_service_opts) do + { + ip_address: fake_ip, + user_agent: fake_user_agent, + referrer: fake_referrer + } + end + + let(:verdict_service_args) do + { + target: issue, + user: user, + request: request, + options: verdict_service_opts, + context: { + action: :create, + target_type: 'Issue' + } + } + end let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false) } subject do - described_service = described_class.new(spammable: issue, request: request, user: user) + described_service = described_class.new(spammable: issue, request: request, user: user, action: :create) allow(described_service).to receive(:allowlisted?).and_return(allowlisted) - described_service.execute(api: nil, recaptcha_verified: recaptcha_verified, spam_log_id: existing_spam_log.id) + described_service.execute(spam_params: spam_params) end before do - allow(Spam::SpamVerdictService).to receive(:new).and_return(fake_verdict_service) + allow(Captcha::CaptchaVerificationService).to receive(:new) { fake_captcha_verification_service } + allow(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args).and_return(fake_verdict_service) end - context 'when reCAPTCHA was already verified' do - let(:recaptcha_verified) { true } + context 'when captcha response verification returns true' do + before do + expect(fake_captcha_verification_service) + .to receive(:execute).with(captcha_response: captcha_response, request: request) { true } + end it "doesn't check with the SpamVerdictService" do aggregate_failures do - expect(SpamLog).to receive(:verify_recaptcha!) + expect(SpamLog).to receive(:verify_recaptcha!).with( + user_id: user.id, + id: spam_log_id + ) expect(fake_verdict_service).not_to receive(:execute) end @@ -105,8 +134,11 @@ RSpec.describe Spam::SpamActionService do end end - context 'when reCAPTCHA was not verified' do - let(:recaptcha_verified) { false } + context 'when captcha response verification returns false' do + before do + expect(fake_captcha_verification_service) + .to receive(:execute).with(captcha_response: captcha_response, request: request) { false } + end context 'when spammable attributes have not changed' do before do @@ -120,6 +152,10 @@ RSpec.describe Spam::SpamActionService do end context 'when spammable attributes have changed' do + let(:expected_service_check_response_message) do + /check Issue spammable model for any errors or captcha requirement/ + end + before do issue.description = 'SPAM!' end @@ -130,7 +166,9 @@ RSpec.describe Spam::SpamActionService do it 'does not perform spam check' do expect(Spam::SpamVerdictService).not_to receive(:new) - subject + response = subject + + expect(response.message).to match(/user was allowlisted/) end end @@ -147,8 +185,9 @@ RSpec.describe Spam::SpamActionService do it_behaves_like 'only checks for spam if a request is provided' it 'marks as spam' do - subject + response = subject + expect(response.message).to match(expected_service_check_response_message) expect(issue).to be_spam end end @@ -157,8 +196,9 @@ RSpec.describe Spam::SpamActionService do it_behaves_like 'only checks for spam if a request is provided' it 'does not mark as spam' do - subject + response = subject + expect(response.message).to match(expected_service_check_response_message) expect(issue).not_to be_spam end end @@ -176,15 +216,19 @@ RSpec.describe Spam::SpamActionService do it_behaves_like 'only checks for spam if a request is provided' + it_behaves_like 'creates a spam log' + it 'does not mark as spam' do - subject + response = subject + expect(response.message).to match(expected_service_check_response_message) expect(issue).not_to be_spam end it 'marks as needing reCAPTCHA' do - subject + response = subject + expect(response.message).to match(expected_service_check_response_message) expect(issue.needs_recaptcha?).to be_truthy end end @@ -192,9 +236,12 @@ RSpec.describe Spam::SpamActionService do context 'when allow_possible_spam feature flag is true' do it_behaves_like 'only checks for spam if a request is provided' + it_behaves_like 'creates a spam log' + it 'does not mark as needing reCAPTCHA' do - subject + response = subject + expect(response.message).to match(expected_service_check_response_message) expect(issue.needs_recaptcha).to be_falsey end end @@ -209,6 +256,51 @@ RSpec.describe Spam::SpamActionService do expect { subject } .not_to change { SpamLog.count } end + + it 'clears spam flags' do + expect(issue).to receive(:clear_spam_flags!) + + subject + end + end + + context 'spam verdict service options' do + before do + allow(fake_verdict_service).to receive(:execute) { ALLOW } + end + + context 'when the request is nil' do + let(:request) { nil } + let(:issue_ip_address) { '1.2.3.4' } + let(:issue_user_agent) { 'lynx' } + let(:verdict_service_opts) do + { + ip_address: issue_ip_address, + user_agent: issue_user_agent + } + end + + before do + allow(issue).to receive(:ip_address) { issue_ip_address } + allow(issue).to receive(:user_agent) { issue_user_agent } + end + + it 'assembles the options with information from the spammable' do + # TODO: This code untestable, because we do not perform a verification if there is not a + # request. See corresponding comment in code + # expect(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args) + + subject + end + end + + context 'when the request is present' do + it 'assembles the options with information from the request' do + expect(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args) + + subject + end + end end end end diff --git a/spec/support/shared_examples/graphql/mutations/spammable_mutation_fields_examples.rb b/spec/support/shared_examples/graphql/mutations/spammable_mutation_fields_examples.rb index 8678b23ad31..bc9cb98cd6c 100644 --- a/spec/support/shared_examples/graphql/mutations/spammable_mutation_fields_examples.rb +++ b/spec/support/shared_examples/graphql/mutations/spammable_mutation_fields_examples.rb @@ -21,10 +21,10 @@ RSpec.shared_examples 'can raise spam flag' do context 'when the snippet is detected as spam' do it 'raises spam flag' do - allow_next_instance_of(service) do |instance| - allow(instance).to receive(:spam_check) do |snippet, user, _| - snippet.spam! - end + allow_next_instance_of(Spam::SpamActionService) do |instance| + allow(instance).to receive(:execute) { true } + instance.target.spam! + instance.target.unrecoverable_spam_error! end subject diff --git a/spec/support/shared_examples/services/snippets_shared_examples.rb b/spec/support/shared_examples/services/snippets_shared_examples.rb index 4a08c0d4365..5847f6c886b 100644 --- a/spec/support/shared_examples/services/snippets_shared_examples.rb +++ b/spec/support/shared_examples/services/snippets_shared_examples.rb @@ -1,43 +1,45 @@ # frozen_string_literal: true -RSpec.shared_examples 'snippets spam check is performed' do - shared_examples 'marked as spam' do - it 'marks a snippet as spam' do - expect(snippet).to be_spam - end - - it 'invalidates the snippet' do - expect(snippet).to be_invalid - end - - it 'creates a new spam_log' do - expect { snippet } - .to have_spam_log(title: snippet.title, noteable_type: snippet.class.name) - end - - it 'assigns a spam_log to an issue' do - expect(snippet.spam_log).to eq(SpamLog.last) - end - end +RSpec.shared_examples 'checking spam' do + let(:request) { double(:request) } + let(:api) { true } + let(:captcha_response) { 'abc123' } + let(:spam_log_id) { 1 } let(:extra_opts) do - { visibility_level: Gitlab::VisibilityLevel::PUBLIC, request: double(:request, env: {}) } + { + request: request, + api: api, + captcha_response: captcha_response, + spam_log_id: spam_log_id + } end before do - expect_next_instance_of(Spam::AkismetService) do |akismet_service| - expect(akismet_service).to receive_messages(spam?: true) + allow_next_instance_of(UserAgentDetailService) do |instance| + allow(instance).to receive(:create) end end - [true, false, nil].each do |allow_possible_spam| - context "when allow_possible_spam flag is #{allow_possible_spam.inspect}" do - before do - stub_feature_flags(allow_possible_spam: allow_possible_spam) unless allow_possible_spam.nil? - end - - it_behaves_like 'marked as spam' - end + it 'executes SpamActionService' do + spam_params = Spam::SpamParams.new( + api: api, + captcha_response: captcha_response, + spam_log_id: spam_log_id + ) + expect_next_instance_of( + Spam::SpamActionService, + { + spammable: kind_of(Snippet), + request: request, + user: an_instance_of(User), + action: action + } + ) do |instance| + expect(instance).to receive(:execute).with(spam_params: spam_params) + end + + subject end end |