diff --git a/.gitlab/issue_templates/Navigation - Left Sidebar Proposals.md b/.gitlab/issue_templates/Navigation - Left Sidebar Proposals.md index 57d6d12267c..e9e510da11e 100644 --- a/.gitlab/issue_templates/Navigation - Left Sidebar Proposals.md +++ b/.gitlab/issue_templates/Navigation - Left Sidebar Proposals.md @@ -8,8 +8,7 @@ - [ ] If your proposal includes changes to the top-level menu items within the left sidebar, engage the [Foundations Product Design Manager](https://about.gitlab.com/handbook/product/categories/#foundations-group) for approval. The Foundations DRI will work with UX partners in product design, research, and technical writing, as applicable. - [ ] Follow the [product development workflow](https://about.gitlab.com/handbook/product-development-flow/#validation-phase-2-problem-validation) validation process to ensure you are solving a well understood problem and that the proposed change is understandable and non-disruptive to users. Navigation-specific research is strongly encouraged. -- [ ] Engage the [Editor](https://about.gitlab.com/handbook/engineering/development/dev/create-editor/) team to ensure your proposal is in alignment with holistic changes happening to the left side bar. +- [ ] Engage the [Foundations](https://about.gitlab.com/handbook/product/categories/#foundations-group) team to ensure your proposal is in alignment with holistic changes happening to the left side bar. - [ ] Consider whether you need to communicate the change somehow, or if you will have an interim period in the UI where your nav item will live in more than one place. -- [ ] Once implemented, update this [navigation map in Mural](https://app.mural.co/t/gitlab2474/m/gitlab2474/1589571490215/261462d0beb3043979374623710d3f2d6cfec1cb) with your navigation change. /label ~UX ~"UI text" ~"documentation" ~"documentation" ~"Category:Navigation & Settings" ~"Category:Foundations" ~navigation diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 5ea1fc4ec18..b22167a3952 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -108,6 +108,12 @@ class ApplicationController < ActionController::Base head :forbidden, retry_after: Gitlab::Auth::UniqueIpsLimiter.config.unique_ips_limit_time_window end + rescue_from RateLimitedService::RateLimitedError do |e| + e.log_request(request, current_user) + response.headers.merge!(e.headers) + render plain: e.message, status: :too_many_requests + end + def redirect_back_or_default(default: root_path, options: {}) redirect_back(fallback_location: default, **options) end diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index f885ff9b45b..fd508d5f127 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -37,7 +37,7 @@ class Projects::IssuesController < Projects::ApplicationController before_action :authorize_download_code!, only: [:related_branches] # Limit the amount of issues created per minute - before_action :create_rate_limit, only: [:create] + before_action :create_rate_limit, only: [:create], if: -> { Feature.disabled?('rate_limited_service_issues_create', project, default_enabled: :yaml) } before_action do push_frontend_feature_flag(:tribute_autocomplete, @project) diff --git a/app/helpers/integrations_helper.rb b/app/helpers/integrations_helper.rb index 904508867d3..8819aa9e9cc 100644 --- a/app/helpers/integrations_helper.rb +++ b/app/helpers/integrations_helper.rb @@ -125,15 +125,6 @@ module IntegrationsHelper !Gitlab.com? end - def integration_tabs(integration:) - [ - { key: 'edit', text: _('Settings'), href: scoped_edit_integration_path(integration) }, - ( - { key: 'overrides', text: s_('Integrations|Projects using custom settings'), href: scoped_overrides_integration_path(integration) } if integration.instance_level? - ) - ].compact - end - def jira_issue_breadcrumb_link(issue_reference) link_to '', { class: 'gl-display-flex gl-align-items-center gl-white-space-nowrap' } do icon = image_tag image_path('illustrations/logos/jira.svg'), width: 15, height: 15, class: 'gl-mr-2' diff --git a/app/services/concerns/rate_limited_service.rb b/app/services/concerns/rate_limited_service.rb new file mode 100644 index 00000000000..87cba7814fe --- /dev/null +++ b/app/services/concerns/rate_limited_service.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +module RateLimitedService + extend ActiveSupport::Concern + + RateLimitedNotSetupError = Class.new(StandardError) + + class RateLimitedError < StandardError + def initialize(key:, rate_limiter:) + @key = key + @rate_limiter = rate_limiter + end + + def headers + # TODO: This will be fleshed out in https://gitlab.com/gitlab-org/gitlab/-/issues/342370 + {} + end + + def log_request(request, current_user) + rate_limiter.class.log_request(request, "#{key}_request_limit".to_sym, current_user) + end + + private + + attr_reader :key, :rate_limiter + end + + class RateLimiterScopedAndKeyed + attr_reader :key, :opts, :rate_limiter_klass + + def initialize(key:, opts:, rate_limiter_klass:) + @key = key + @opts = opts + @rate_limiter_klass = rate_limiter_klass + end + + def rate_limit!(service) + evaluated_scope = evaluated_scope_for(service) + return if feature_flag_disabled?(evaluated_scope[:project]) + + rate_limiter = new_rate_limiter(evaluated_scope) + if rate_limiter.throttled? + raise RateLimitedError.new(key: key, rate_limiter: rate_limiter), _('This endpoint has been requested too many times. Try again later.') + end + end + + private + + def users_allowlist + @users_allowlist ||= opts[:users_allowlist] ? opts[:users_allowlist].call : [] + end + + def evaluated_scope_for(service) + opts[:scope].each_with_object({}) do |var, all| + all[var] = service.public_send(var) # rubocop: disable GitlabSecurity/PublicSend + end + end + + def feature_flag_disabled?(project) + Feature.disabled?("rate_limited_service_#{key}", project, default_enabled: :yaml) + end + + def new_rate_limiter(evaluated_scope) + rate_limiter_klass.new(key, **opts.merge(scope: evaluated_scope.values, users_allowlist: users_allowlist)) + end + end + + prepended do + attr_accessor :rate_limiter_bypassed + cattr_accessor :rate_limiter_scoped_and_keyed + + def self.rate_limit(key:, opts:, rate_limiter_klass: ::Gitlab::ApplicationRateLimiter) + self.rate_limiter_scoped_and_keyed = RateLimiterScopedAndKeyed.new(key: key, + opts: opts, + rate_limiter_klass: rate_limiter_klass) + end + end + + def execute_without_rate_limiting(*args, **kwargs) + self.rate_limiter_bypassed = true + execute(*args, **kwargs) + ensure + self.rate_limiter_bypassed = false + end + + def execute(*args, **kwargs) + raise RateLimitedNotSetupError if rate_limiter_scoped_and_keyed.nil? + + rate_limiter_scoped_and_keyed.rate_limit!(self) unless rate_limiter_bypassed + + super + end +end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index b15b3e49c9a..fcedd1c1c8d 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -3,6 +3,10 @@ module Issues class CreateService < Issues::BaseService include ResolveDiscussions + prepend RateLimitedService + + rate_limit key: :issues_create, + opts: { scope: [:project, :current_user], users_allowlist: -> { [User.support_bot.username] } } # NOTE: For Issues::CreateService, we require the spam_params and do not default it to nil, because # spam_checking is likely to be necessary. However, if there is not a request available in scope diff --git a/app/views/shared/integrations/_tabs.html.haml b/app/views/shared/integrations/_tabs.html.haml index 553401e47bd..d6ca0bd7d1e 100644 --- a/app/views/shared/integrations/_tabs.html.haml +++ b/app/views/shared/integrations/_tabs.html.haml @@ -1,18 +1,11 @@ -- active_tab = local_assigns.fetch(:active_tab, 'edit') -- active_classes = 'gl-tab-nav-item-active gl-tab-nav-item-active-indigo active' -- tabs = integration_tabs(integration: integration) - -- if tabs.length <= 1 - = yield -- else +- if integration.instance_level? .tabs.gl-tabs %div - %ul.nav.gl-tabs-nav{ role: 'tablist' } - - tabs.each do |tab| - %li.nav-item{ role: 'presentation' } - %a.nav-link.gl-tab-nav-item{ role: 'tab', class: (active_classes if tab[:key] == active_tab), href: tab[:href] } - = tab[:text] + = gl_tabs_nav({ class: 'gl-mb-5' }) do + = gl_tab_link_to _('Settings'), scoped_edit_integration_path(integration) + = gl_tab_link_to s_('Integrations|Projects using custom settings'), scoped_overrides_integration_path(integration) - .tab-content.gl-tab-content - .tab-pane.gl-pt-3.active{ role: 'tabpanel' } - = yield + = yield + +- else + = yield diff --git a/config/feature_flags/development/rate_limited_service_issues_create.yml b/config/feature_flags/development/rate_limited_service_issues_create.yml new file mode 100644 index 00000000000..95ece10aa6c --- /dev/null +++ b/config/feature_flags/development/rate_limited_service_issues_create.yml @@ -0,0 +1,8 @@ +--- +name: rate_limited_service_issues_create +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68526 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/342677 +milestone: '14.4' +type: development +group: group::project management +default_enabled: false diff --git a/doc/development/testing_guide/end_to_end/best_practices.md b/doc/development/testing_guide/end_to_end/best_practices.md index baa4cddc306..9491c89c2a0 100644 --- a/doc/development/testing_guide/end_to_end/best_practices.md +++ b/doc/development/testing_guide/end_to_end/best_practices.md @@ -245,7 +245,11 @@ end ## Prefer `aggregate_failures` when there are back-to-back expectations -In cases where there must be multiple (back-to-back) expectations within a test case, it is preferable to use `aggregate_failures`. +See [Prefer aggregate failures when there are multiple expectations](#prefer-aggregate_failures-when-there-are-multiple-expectations) + +## Prefer `aggregate_failures` when there are multiple expectations + +In cases where there must be multiple expectations within a test case, it is preferable to use `aggregate_failures`. This allows you to group a set of expectations and see all the failures altogether, rather than having the test being aborted on the first failure. @@ -270,6 +274,32 @@ Page::Search::Results.perform do |search| end ``` +Attach the `:aggregate_failures` metadata to the example if multiple expectations are separated by statements. + +```ruby +#=> Good +it 'searches', :aggregate_failures do + Page::Search::Results.perform do |search| + expect(search).to have_file_in_project(template[:file_name], project.name) + + search.switch_to_code + + expect(search).to have_file_with_content(template[:file_name], content[0..33]) + end +end + +#=> Bad +it 'searches' do + Page::Search::Results.perform do |search| + expect(search).to have_file_in_project(template[:file_name], project.name) + + search.switch_to_code + + expect(search).to have_file_with_content(template[:file_name], content[0..33]) + end +end +``` + ## Prefer to split tests across multiple files Our framework includes a couple of parallelization mechanisms that work by executing spec files in parallel. diff --git a/doc/integration/saml.md b/doc/integration/saml.md index 9895d8a21d2..21ab73faa0d 100644 --- a/doc/integration/saml.md +++ b/doc/integration/saml.md @@ -52,7 +52,8 @@ in your SAML IdP: sudo -u git -H editor config/gitlab.yml ``` - + +1. See [Initial OmniAuth Configuration](omniauth.md#initial-omniauth-configuration) for initial settings. 1. To allow your users to use SAML to sign up without having to manually create an account first, add the following values to your configuration: diff --git a/doc/user/application_security/sast/index.md b/doc/user/application_security/sast/index.md index 993e5ee94dd..d182597942a 100644 --- a/doc/user/application_security/sast/index.md +++ b/doc/user/application_security/sast/index.md @@ -75,10 +75,11 @@ You can also [view our language roadmap](https://about.gitlab.com/direction/secu | .NET Core | [Security Code Scan](https://security-code-scan.github.io) | 11.0 | | .NET Framework | [Security Code Scan](https://security-code-scan.github.io) | 13.0 | | Apex (Salesforce) | [PMD](https://pmd.github.io/pmd/index.html) | 12.1 | -| C | [Semgrep](https://semgrep.dev) | 14.2 | +| C | [Semgrep](https://semgrep.dev) | 14.2 | | C/C++ | [Flawfinder](https://github.com/david-a-wheeler/flawfinder) | 10.7 | | Elixir (Phoenix) | [Sobelow](https://github.com/nccgroup/sobelow) | 11.1 | | Go | [Gosec](https://github.com/securego/gosec) | 10.7 | +| Go | [Semgrep](https://semgrep.dev) | 14.4 | | Groovy ([Ant](https://ant.apache.org/), [Gradle](https://gradle.org/), [Maven](https://maven.apache.org/), and [SBT](https://www.scala-sbt.org/)) | [SpotBugs](https://spotbugs.github.io/) with the [find-sec-bugs](https://find-sec-bugs.github.io/) plugin | 11.3 (Gradle) & 11.9 (Ant, Maven, SBT) | | Helm Charts | [Kubesec](https://github.com/controlplaneio/kubesec) | 13.1 | | Java ([Ant](https://ant.apache.org/), [Gradle](https://gradle.org/), [Maven](https://maven.apache.org/), and [SBT](https://www.scala-sbt.org/)) | [SpotBugs](https://spotbugs.github.io/) with the [find-sec-bugs](https://find-sec-bugs.github.io/) plugin | 10.6 (Maven), 10.8 (Gradle) & 11.9 (Ant, SBT) | diff --git a/doc/user/group/subgroups/img/create_new_group.png b/doc/user/group/subgroups/img/create_new_group.png deleted file mode 100644 index 9d011ec709a..00000000000 Binary files a/doc/user/group/subgroups/img/create_new_group.png and /dev/null differ diff --git a/doc/user/group/subgroups/img/create_subgroup_button_v13_6.png b/doc/user/group/subgroups/img/create_subgroup_button_v13_6.png deleted file mode 100644 index 013aee6b0b4..00000000000 Binary files a/doc/user/group/subgroups/img/create_subgroup_button_v13_6.png and /dev/null differ diff --git a/doc/user/group/subgroups/index.md b/doc/user/group/subgroups/index.md index 507db5c4a86..49032d0a2ef 100644 --- a/doc/user/group/subgroups/index.md +++ b/doc/user/group/subgroups/index.md @@ -73,51 +73,41 @@ subgroups, the behavior is the same as when performing these actions at the ## Creating a subgroup -To create a subgroup you must either be an Owner or a Maintainer of the -group, depending on the group's setting. - -By default, groups created in: - -- GitLab 12.2 or later allow both Owners and Maintainers to create subgroups. -- GitLab 12.1 or earlier only allow Owners to create subgroups. - -The setting can be changed for any group by: - -- A group owner: - 1. Select the group. - 1. On the left sidebar, select **Settings > General**. - 1. Expand the **Permissions, LFS, 2FA** section. -- An administrator: - 1. On the top bar, select **Menu > Admin**. - 1. On the left sidebar, select **Overview > Groups**. - 1. Select the group, and select **Edit**. - -For: - -- More information, check the [permissions table](../../permissions.md#group-members-permissions). -- A list of words that are not allowed to be used as group names, see the - [reserved names](../../reserved_names.md). - -Users can always create subgroups if they are explicitly added as an Owner (or +Users can create subgroups if they are explicitly added as an Owner (or Maintainer, if that setting is enabled) to an immediate parent group, even if group creation is disabled by an administrator in their settings. To create a subgroup: -1. In the group's dashboard click the **New subgroup** button. +1. On the top bar, select **Menu > Groups** and find the parent group. +1. In the top right, select **New subgroup**. +1. Select **Create group**. +1. Fill in the fields. View a list of [reserved names](../../reserved_names.md) + that cannot be used as group names. +1. Select **Create group**. - ![Subgroups page](img/create_subgroup_button_v13_6.png) +### Change who can create subgroups -1. Create a new group like you would normally do. Notice that the immediate parent group - namespace is fixed under **Group path**. The visibility level can differ from - the immediate parent group. +To create a subgroup you must either be an Owner or a Maintainer of the +group, depending on the group's setting. - ![Subgroups page](img/create_new_group.png) +By default: -1. Click the **Create group** button to be redirected to the new group's - dashboard page. +- In GitLab 12.2 or later, both Owners and Maintainers can create subgroups. +- In GitLab 12.1 or earlier, only Owners can create subgroups. -Follow the same process to create any subsequent groups. +You can change this setting: + +- As group owner: + 1. Select the group. + 1. On the left sidebar, select **Settings > General**. + 1. Expand **Permissions, LFS, 2FA**. +- As an administrator: + 1. On the top bar, select **Menu > Admin**. + 1. On the left sidebar, select **Overview > Groups**. + 1. Select the group, and select **Edit**. + +For more information, view the [permissions table](../../permissions.md#group-members-permissions). ## Membership diff --git a/lib/api/api.rb b/lib/api/api.rb index a5340d9baaf..9be515bcb09 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -124,6 +124,11 @@ module API handle_api_exception(exception) end + rescue_from RateLimitedService::RateLimitedError do |exception| + exception.log_request(context.request, context.current_user) + rack_response({ 'message' => { 'error' => exception.message } }.to_json, 429, exception.headers) + end + format :json formatter :json, Gitlab::Json::GrapeFormatter content_type :json, 'application/json' @@ -132,6 +137,7 @@ module API helpers ::API::Helpers helpers ::API::Helpers::CommonHelpers helpers ::API::Helpers::PerformanceBarHelpers + helpers ::API::Helpers::RateLimiter namespace do after do diff --git a/lib/api/group_export.rb b/lib/api/group_export.rb index 7e4fdba6033..25cc4e53bd2 100644 --- a/lib/api/group_export.rb +++ b/lib/api/group_export.rb @@ -2,8 +2,6 @@ module API class GroupExport < ::API::Base - helpers Helpers::RateLimiter - before do not_found! unless Feature.enabled?(:group_import_export, user_group, default_enabled: true) diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 87b576b92fb..43e83bd58fe 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -4,7 +4,6 @@ module API class Issues < ::API::Base include PaginationParams helpers Helpers::IssuesHelpers - helpers Helpers::RateLimiter before { authenticate_non_get! } @@ -263,7 +262,7 @@ module API post ':id/issues' do Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/21140') - check_rate_limit! :issues_create, [current_user] + check_rate_limit! :issues_create, [current_user] if Feature.disabled?("rate_limited_service_issues_create", user_project, default_enabled: :yaml) authorize! :create_issue, user_project diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 953c7feefbc..656eaa2b2bb 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -4,7 +4,6 @@ module API class Notes < ::API::Base include PaginationParams helpers ::API::Helpers::NotesHelpers - helpers Helpers::RateLimiter before { authenticate! } diff --git a/lib/api/project_export.rb b/lib/api/project_export.rb index 5a1c33fed30..e01c195dbc4 100644 --- a/lib/api/project_export.rb +++ b/lib/api/project_export.rb @@ -2,8 +2,6 @@ module API class ProjectExport < ::API::Base - helpers Helpers::RateLimiter - feature_category :importers before do diff --git a/lib/api/project_import.rb b/lib/api/project_import.rb index 039f7b4be41..d43184ff75d 100644 --- a/lib/api/project_import.rb +++ b/lib/api/project_import.rb @@ -6,7 +6,6 @@ module API helpers Helpers::ProjectsHelpers helpers Helpers::FileUploadHelpers - helpers Helpers::RateLimiter feature_category :importers diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index fbd55a695e8..7c37f67b766 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -11,6 +11,23 @@ module Gitlab # redirect_to(edit_project_path(@project), status: :too_many_requests) # end class ApplicationRateLimiter + def initialize(key, **options) + @key = key + @options = options + end + + def throttled? + self.class.throttled?(key, **options) + end + + def threshold_value + options[:threshold] || self.class.threshold(key) + end + + def interval_value + self.class.interval(key) + end + class << self # Application rate limits # @@ -154,5 +171,9 @@ module Gitlab scoped_user.username.downcase.in?(options[:users_allowlist]) end end + + private + + attr_reader :key, :options end end diff --git a/lib/quality/seeders/issues.rb b/lib/quality/seeders/issues.rb index 3eb0245f8a2..5d345dd30a1 100644 --- a/lib/quality/seeders/issues.rb +++ b/lib/quality/seeders/issues.rb @@ -30,7 +30,8 @@ module Quality labels: labels.join(',') } params[:closed_at] = params[:created_at] + rand(35).days if params[:state] == 'closed' - issue = ::Issues::CreateService.new(project: project, current_user: team.sample, params: params, spam_params: nil).execute + + issue = ::Issues::CreateService.new(project: project, current_user: team.sample, params: params, spam_params: nil).execute_without_rate_limiting if issue.persisted? created_issues_count += 1 diff --git a/scripts/static-analysis b/scripts/static-analysis index 7911e89b113..f50e4a24b58 100755 --- a/scripts/static-analysis +++ b/scripts/static-analysis @@ -66,7 +66,9 @@ class StaticAnalysis ].compact.freeze def run_tasks!(options = {}) - node_assignment = tasks_to_run((ENV['CI_NODE_TOTAL'] || 1).to_i)[(ENV['CI_NODE_INDEX'] || 1).to_i - 1] + total_nodes = (ENV['CI_NODE_TOTAL'] || 1).to_i + current_node_number = (ENV['CI_NODE_INDEX'] || 1).to_i + node_assignment = tasks_to_run(total_nodes)[current_node_number - 1] if options[:dry_run] puts "Dry-run mode!" diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 977879b453c..0b3bd4d78ac 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1411,39 +1411,42 @@ RSpec.describe Projects::IssuesController do stub_application_setting(issues_create_limit: 5) end - it 'prevents from creating more issues', :request_store do - 5.times { post_new_issue } + context 'when issue creation limits imposed' do + it 'prevents from creating more issues', :request_store do + 5.times { post_new_issue } - expect { post_new_issue } - .to change { Gitlab::GitalyClient.get_request_count }.by(1) # creates 1 projects and 0 issues + expect { post_new_issue } + .to change { Gitlab::GitalyClient.get_request_count }.by(1) # creates 1 projects and 0 issues - post_new_issue - expect(response.body).to eq(_('This endpoint has been requested too many times. Try again later.')) - expect(response).to have_gitlab_http_status(:too_many_requests) - end + post_new_issue - it 'logs the event on auth.log' do - attributes = { - message: 'Application_Rate_Limiter_Request', - env: :issues_create_request_limit, - remote_ip: '0.0.0.0', - request_method: 'POST', - path: "/#{project.full_path}/-/issues", - user_id: user.id, - username: user.username - } + expect(response.body).to eq(_('This endpoint has been requested too many times. Try again later.')) + expect(response).to have_gitlab_http_status(:too_many_requests) + end - expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once - - project.add_developer(user) - sign_in(user) - - 6.times do - post :create, params: { - namespace_id: project.namespace.to_param, - project_id: project, - issue: { title: 'Title', description: 'Description' } + it 'logs the event on auth.log' do + attributes = { + message: 'Application_Rate_Limiter_Request', + env: :issues_create_request_limit, + remote_ip: '0.0.0.0', + request_method: 'POST', + path: "/#{project.full_path}/-/issues", + user_id: user.id, + username: user.username } + + expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once + + project.add_developer(user) + sign_in(user) + + 6.times do + post :create, params: { + namespace_id: project.namespace.to_param, + project_id: project, + issue: { title: 'Title', description: 'Description' } + } + end end end end diff --git a/spec/graphql/mutations/issues/create_spec.rb b/spec/graphql/mutations/issues/create_spec.rb index 0e7ef0e55b9..825d04ff827 100644 --- a/spec/graphql/mutations/issues/create_spec.rb +++ b/spec/graphql/mutations/issues/create_spec.rb @@ -53,7 +53,11 @@ RSpec.describe Mutations::Issues::Create do stub_spam_services end - subject { mutation.resolve(**mutation_params) } + def resolve + mutation.resolve(**mutation_params) + end + + subject { resolve } context 'when the user does not have permission to create an issue' do it 'raises an error' do @@ -61,6 +65,15 @@ RSpec.describe Mutations::Issues::Create do end end + context 'when the user has exceeded the rate limit' do + it 'raises an error' do + allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true) + project.add_developer(user) + + expect { resolve }.to raise_error(RateLimitedService::RateLimitedError, _('This endpoint has been requested too many times. Try again later.')) + end + end + context 'when the user can create an issue' do context 'when creating an issue a developer' do before do diff --git a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb index dd230140b30..bd4f1d164a8 100644 --- a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb @@ -136,6 +136,36 @@ RSpec.describe Gitlab::Email::Handler::CreateIssueHandler do expect { handler.execute }.to raise_error(Gitlab::Email::ProjectNotFound) end end + + context 'rate limiting' do + let(:rate_limited_service_feature_enabled) { nil } + + before do + stub_feature_flags(rate_limited_service_issues_create: rate_limited_service_feature_enabled) + end + + context 'when :rate_limited_service Feature is disabled' do + let(:rate_limited_service_feature_enabled) { false } + + it 'does not attempt to throttle' do + expect(::Gitlab::ApplicationRateLimiter).not_to receive(:throttled?) + + setup_attachment + receiver.execute + end + end + + context 'when :rate_limited_service Feature is enabled' do + let(:rate_limited_service_feature_enabled) { true } + + it 'raises a RateLimitedService::RateLimitedError' do + allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true) + + setup_attachment + expect { receiver.execute }.to raise_error(RateLimitedService::RateLimitedError, _('This endpoint has been requested too many times. Try again later.')) + end + end + end end def email_fixture(path) diff --git a/spec/lib/gitlab/email/handler/service_desk_handler_spec.rb b/spec/lib/gitlab/email/handler/service_desk_handler_spec.rb index 2916e65528f..8cb1ccc065b 100644 --- a/spec/lib/gitlab/email/handler/service_desk_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/service_desk_handler_spec.rb @@ -243,6 +243,15 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do end end end + + context 'when rate limiting is in effect' do + it 'allows unlimited new issue creation' do + stub_application_setting(issues_create_limit: 1) + setup_attachment + + expect { 2.times { receiver.execute } }.to change { Issue.count }.by(2) + end + end end describe '#can_handle?' do diff --git a/spec/requests/api/issues/post_projects_issues_spec.rb b/spec/requests/api/issues/post_projects_issues_spec.rb index cc1d402dca9..82692366589 100644 --- a/spec/requests/api/issues/post_projects_issues_spec.rb +++ b/spec/requests/api/issues/post_projects_issues_spec.rb @@ -399,16 +399,15 @@ RSpec.describe API::Issues do end context 'when request exceeds the rate limit' do - before do - allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true) - end - it 'prevents users from creating more issues' do + allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true) + post api("/projects/#{project.id}/issues", user), params: { title: 'new issue', labels: 'label, label2', weight: 3, assignee_ids: [user2.id] } - expect(response).to have_gitlab_http_status(:too_many_requests) expect(json_response['message']['error']).to eq('This endpoint has been requested too many times. Try again later.') + + expect(response).to have_gitlab_http_status(:too_many_requests) end end end diff --git a/spec/services/concerns/rate_limited_service_spec.rb b/spec/services/concerns/rate_limited_service_spec.rb new file mode 100644 index 00000000000..f73871b7e44 --- /dev/null +++ b/spec/services/concerns/rate_limited_service_spec.rb @@ -0,0 +1,196 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe RateLimitedService do + let(:key) { :issues_create } + let(:scope) { [:project, :current_user] } + let(:opts) { { scope: scope, users_allowlist: -> { [User.support_bot.username] } } } + let(:rate_limiter_klass) { ::Gitlab::ApplicationRateLimiter } + let(:rate_limiter_instance) { rate_limiter_klass.new(key, **opts) } + + describe 'RateLimitedError' do + subject { described_class::RateLimitedError.new(key: key, rate_limiter: rate_limiter_instance) } + + describe '#headers' do + it 'returns a Hash of HTTP headers' do + # TODO: This will be fleshed out in https://gitlab.com/gitlab-org/gitlab/-/issues/342370 + expected_headers = {} + + expect(subject.headers).to eq(expected_headers) + end + end + + describe '#log_request' do + it 'logs the request' do + request = instance_double(Grape::Request) + user = instance_double(User) + + expect(rate_limiter_klass).to receive(:log_request).with(request, "#{key}_request_limit".to_sym, user) + + subject.log_request(request, user) + end + end + end + + describe 'RateLimiterScopedAndKeyed' do + subject { described_class::RateLimiterScopedAndKeyed.new(key: key, opts: opts, rate_limiter_klass: rate_limiter_klass) } + + describe '#rate_limit!' do + let(:project_with_feature_enabled) { create(:project) } + let(:project_without_feature_enabled) { create(:project) } + + let(:project) { nil } + + let(:current_user) { create(:user) } + let(:service) { instance_double(Issues::CreateService, project: project, current_user: current_user) } + let(:evaluated_scope) { [project, current_user] } + let(:evaluated_opts) { { scope: evaluated_scope, users_allowlist: %w[support-bot] } } + let(:rate_limited_service_issues_create_feature_enabled) { nil } + + before do + allow(rate_limiter_klass).to receive(:new).with(key, **evaluated_opts).and_return(rate_limiter_instance) + stub_feature_flags(rate_limited_service_issues_create: rate_limited_service_issues_create_feature_enabled) + end + + shared_examples 'a service that does not attempt to throttle' do + it 'does not attempt to throttle' do + expect(rate_limiter_instance).not_to receive(:throttled?) + + expect(subject.rate_limit!(service)).to be_nil + end + end + + shared_examples 'a service that does attempt to throttle' do + before do + allow(rate_limiter_instance).to receive(:throttled?).and_return(throttled) + end + + context 'when rate limiting is not in effect' do + let(:throttled) { false } + + it 'does not raise an exception' do + expect(subject.rate_limit!(service)).to be_nil + end + end + + context 'when rate limiting is in effect' do + let(:throttled) { true } + + it 'raises a RateLimitedError exception' do + expect { subject.rate_limit!(service) }.to raise_error(described_class::RateLimitedError, 'This endpoint has been requested too many times. Try again later.') + end + end + end + + context 'when :rate_limited_service_issues_create feature is globally disabled' do + let(:rate_limited_service_issues_create_feature_enabled) { false } + + it_behaves_like 'a service that does not attempt to throttle' + end + + context 'when :rate_limited_service_issues_create feature is globally enabled' do + let(:throttled) { nil } + let(:rate_limited_service_issues_create_feature_enabled) { true } + let(:project) { project_without_feature_enabled } + + it_behaves_like 'a service that does attempt to throttle' + end + + context 'when :rate_limited_service_issues_create feature is enabled for project_with_feature_enabled' do + let(:throttled) { nil } + let(:rate_limited_service_issues_create_feature_enabled) { project_with_feature_enabled } + + context 'for project_without_feature_enabled' do + let(:project) { project_without_feature_enabled } + + it_behaves_like 'a service that does not attempt to throttle' + end + + context 'for project_with_feature_enabled' do + let(:project) { project_with_feature_enabled } + + it_behaves_like 'a service that does attempt to throttle' + end + end + end + end + + describe '#execute_without_rate_limiting' do + let(:rate_limiter_scoped_and_keyed) { instance_double(RateLimitedService::RateLimiterScopedAndKeyed) } + let(:subject) do + local_key = key + local_opts = opts + + Class.new do + prepend RateLimitedService + + rate_limit key: local_key, opts: local_opts + + def execute(*args, **kwargs) + 'main logic here' + end + end.new + end + + before do + allow(RateLimitedService::RateLimiterScopedAndKeyed).to receive(:new).with(key: key, opts: opts, rate_limiter_klass: rate_limiter_klass).and_return(rate_limiter_scoped_and_keyed) + end + + context 'bypasses rate limiting' do + it 'calls super' do + expect(rate_limiter_scoped_and_keyed).not_to receive(:rate_limit!).with(subject) + + expect(subject.execute_without_rate_limiting).to eq('main logic here') + end + end + end + + describe '#execute' do + context 'when rate_limit has not been called' do + let(:subject) { Class.new { prepend RateLimitedService }.new } + + it 'raises an RateLimitedNotSetupError exception' do + expect { subject.execute }.to raise_error(described_class::RateLimitedNotSetupError) + end + end + + context 'when rate_limit has been called' do + let(:rate_limiter_scoped_and_keyed) { instance_double(RateLimitedService::RateLimiterScopedAndKeyed) } + let(:subject) do + local_key = key + local_opts = opts + + Class.new do + prepend RateLimitedService + + rate_limit key: local_key, opts: local_opts + + def execute(*args, **kwargs) + 'main logic here' + end + end.new + end + + before do + allow(RateLimitedService::RateLimiterScopedAndKeyed).to receive(:new).with(key: key, opts: opts, rate_limiter_klass: rate_limiter_klass).and_return(rate_limiter_scoped_and_keyed) + end + + context 'and applies rate limiting' do + it 'raises an RateLimitedService::RateLimitedError exception' do + expect(rate_limiter_scoped_and_keyed).to receive(:rate_limit!).with(subject).and_raise(RateLimitedService::RateLimitedError.new(key: key, rate_limiter: rate_limiter_instance)) + + expect { subject.execute }.to raise_error(RateLimitedService::RateLimitedError) + end + end + + context 'but does not apply rate limiting' do + it 'calls super' do + expect(rate_limiter_scoped_and_keyed).to receive(:rate_limit!).with(subject).and_return(nil) + + expect(subject.execute).to eq('main logic here') + end + end + end + end +end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 3988069d83a..1887be4896e 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -10,6 +10,25 @@ RSpec.describe Issues::CreateService do let(:spam_params) { double } + describe '.rate_limiter_scoped_and_keyed' do + it 'is set via the rate_limit call' do + expect(described_class.rate_limiter_scoped_and_keyed).to be_a(RateLimitedService::RateLimiterScopedAndKeyed) + + expect(described_class.rate_limiter_scoped_and_keyed.key).to eq(:issues_create) + expect(described_class.rate_limiter_scoped_and_keyed.opts[:scope]).to eq(%i[project current_user]) + expect(described_class.rate_limiter_scoped_and_keyed.opts[:users_allowlist].call).to eq(%w[support-bot]) + expect(described_class.rate_limiter_scoped_and_keyed.rate_limiter_klass).to eq(Gitlab::ApplicationRateLimiter) + end + end + + describe '#rate_limiter_bypassed' do + let(:subject) { described_class.new(project: project, spam_params: {}) } + + it 'is nil by default' do + expect(subject.rate_limiter_bypassed).to be_nil + end + end + describe '#execute' do let_it_be(:assignee) { create(:user) } let_it_be(:milestone) { create(:milestone, project: project) } diff --git a/spec/support/database/cross-join-allowlist.yml b/spec/support/database/cross-join-allowlist.yml index 8006113ce8c..7eb29c6ea7c 100644 --- a/spec/support/database/cross-join-allowlist.yml +++ b/spec/support/database/cross-join-allowlist.yml @@ -9,11 +9,9 @@ - "./ee/spec/finders/ee/namespaces/projects_finder_spec.rb" - "./ee/spec/graphql/ee/resolvers/namespace_projects_resolver_spec.rb" - "./ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb" -- "./ee/spec/lib/ee/gitlab/background_migration/migrate_security_scans_spec.rb" - "./ee/spec/lib/ee/gitlab/background_migration/populate_latest_pipeline_ids_spec.rb" - "./ee/spec/lib/ee/gitlab/background_migration/populate_resolved_on_default_branch_column_spec.rb" - "./ee/spec/lib/ee/gitlab/background_migration/populate_uuids_for_security_findings_spec.rb" -- "./ee/spec/lib/ee/gitlab/background_migration/populate_vulnerability_feedback_pipeline_id_spec.rb" - "./ee/spec/lib/ee/gitlab/usage_data_spec.rb" - "./ee/spec/migrations/schedule_populate_resolved_on_default_branch_column_spec.rb" - "./ee/spec/models/ci/build_spec.rb" diff --git a/spec/workers/email_receiver_worker_spec.rb b/spec/workers/email_receiver_worker_spec.rb index d26c08fb221..83e13ded7b3 100644 --- a/spec/workers/email_receiver_worker_spec.rb +++ b/spec/workers/email_receiver_worker_spec.rb @@ -80,6 +80,21 @@ RSpec.describe EmailReceiverWorker, :mailer do expect(email).to be_nil end end + + context 'when the error is RateLimitedService::RateLimitedError' do + let(:error) { RateLimitedService::RateLimitedError.new(key: :issues_create, rate_limiter: Gitlab::ApplicationRateLimiter) } + + it 'does not report the error to the sender' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with(error).and_call_original + + perform_enqueued_jobs do + described_class.new.perform(raw_message) + end + + email = ActionMailer::Base.deliveries.last + expect(email).to be_nil + end + end end end