diff options
37 files changed, 204 insertions, 76 deletions
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index e733be39c52..86c60401bdf 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -965c2fedd2d11761d3f0c00b13b98e3f7e50b34e +3e152f5c73c61d6d0a17a5103077de4b049f233d diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index ee5caf63703..4bcd1be9f53 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -1,8 +1,10 @@ # frozen_string_literal: true class AutocompleteController < ApplicationController + include SearchRateLimitable + skip_before_action :authenticate_user!, only: [:users, :award_emojis, :merge_request_target_branches] - before_action :check_email_search_rate_limit!, only: [:users] + before_action :check_search_rate_limit!, only: [:users, :projects] feature_category :users, [:users, :user] feature_category :projects, [:projects] @@ -72,12 +74,6 @@ class AutocompleteController < ApplicationController def target_branch_params params.permit(:group_id, :project_id).select { |_, v| v.present? } end - - def check_email_search_rate_limit! - search_params = Gitlab::Search::Params.new(params) - - check_rate_limit!(:user_email_lookup, scope: [current_user]) if search_params.email_lookup? - end end AutocompleteController.prepend_mod_with('AutocompleteController') diff --git a/app/controllers/concerns/search_rate_limitable.rb b/app/controllers/concerns/search_rate_limitable.rb new file mode 100644 index 00000000000..a77ebd276b6 --- /dev/null +++ b/app/controllers/concerns/search_rate_limitable.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module SearchRateLimitable + extend ActiveSupport::Concern + + private + + def check_search_rate_limit! + if current_user + check_rate_limit!(:search_rate_limit, scope: [current_user]) + else + check_rate_limit!(:search_rate_limit_unauthenticated, scope: [request.ip]) + end + end +end diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 0c3d400875d..817da658f14 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -4,6 +4,7 @@ class SearchController < ApplicationController include ControllerWithCrossProjectAccessCheck include SearchHelper include RedisTracking + include SearchRateLimitable RESCUE_FROM_TIMEOUT_ACTIONS = [:count, :show, :autocomplete].freeze @@ -17,7 +18,7 @@ class SearchController < ApplicationController search_term_present = params[:search].present? || params[:term].present? search_term_present && !params[:project_id].present? end - before_action :check_email_search_rate_limit!, only: [:show, :count, :autocomplete] + before_action :check_search_rate_limit!, only: [:show, :count, :autocomplete] rescue_from ActiveRecord::QueryCanceled, with: :render_timeout @@ -202,12 +203,6 @@ class SearchController < ApplicationController render status: :request_timeout end end - - def check_email_search_rate_limit! - return unless search_service.params.email_lookup? - - check_rate_limit!(:user_email_lookup, scope: [current_user]) - end end SearchController.prepend_mod_with('SearchController') diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 1c148029e47..d9a1731e820 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -424,7 +424,8 @@ module ApplicationSettingsHelper :sidekiq_job_limiter_compression_threshold_bytes, :sidekiq_job_limiter_limit_bytes, :suggest_pipeline_enabled, - :user_email_lookup_limit, + :search_rate_limit, + :search_rate_limit_unauthenticated, :users_get_by_id_limit, :users_get_by_id_limit_allowlist_raw, :runner_token_expiration_interval, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 9941f76a51f..3453518fea8 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -11,6 +11,7 @@ class ApplicationSetting < ApplicationRecord ignore_columns %i[elasticsearch_shards elasticsearch_replicas], remove_with: '14.4', remove_after: '2021-09-22' ignore_columns %i[static_objects_external_storage_auth_token], remove_with: '14.9', remove_after: '2022-03-22' ignore_column %i[max_package_files_for_package_destruction], remove_with: '14.9', remove_after: '2022-03-22' + ignore_column :user_email_lookup_limit, remove_with: '15.0', remove_after: '2022-04-18' INSTANCE_REVIEW_MIN_USERS = 50 GRAFANA_URL_ERROR_MESSAGE = 'Please check your Grafana URL setting in ' \ @@ -519,9 +520,12 @@ class ApplicationSetting < ApplicationRecord validates :notes_create_limit, numericality: { only_integer: true, greater_than_or_equal_to: 0 } - validates :user_email_lookup_limit, + validates :search_rate_limit, numericality: { only_integer: true, greater_than_or_equal_to: 0 } + validates :search_rate_limit_unauthenticated, + numericality: { only_integer: true, greater_than_or_equal_to: 0 } + validates :notes_create_limit_allowlist, length: { maximum: 100, message: N_('is too long (maximum is 100 entries)') }, allow_nil: false diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index 73448461826..42049713883 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -233,7 +233,8 @@ module ApplicationSettingImplementation rate_limiting_response_text: nil, whats_new_variant: 0, user_deactivation_emails_enabled: true, - user_email_lookup_limit: 60, + search_rate_limit: 30, + search_rate_limit_unauthenticated: 10, users_get_by_id_limit: 300, users_get_by_id_limit_allowlist: [] } diff --git a/app/models/concerns/timebox.rb b/app/models/concerns/timebox.rb index 943ef3fa59f..d53594eb5af 100644 --- a/app/models/concerns/timebox.rb +++ b/app/models/concerns/timebox.rb @@ -44,7 +44,6 @@ module Timebox validates :group, presence: true, unless: :project validates :project, presence: true, unless: :group - validates :title, presence: true validate :timebox_type_check validate :start_date_should_be_less_than_due_date, if: proc { |m| m.start_date.present? && m.due_date.present? } diff --git a/app/models/instance_configuration.rb b/app/models/instance_configuration.rb index 2016024b2f4..00e55d0fd89 100644 --- a/app/models/instance_configuration.rb +++ b/app/models/instance_configuration.rb @@ -118,7 +118,8 @@ class InstanceConfiguration group_export_download: application_setting_limit_per_minute(:group_download_export_limit), group_import: application_setting_limit_per_minute(:group_import_limit), raw_blob: application_setting_limit_per_minute(:raw_blob_request_limit), - user_email_lookup: application_setting_limit_per_minute(:user_email_lookup_limit), + search_rate_limit: application_setting_limit_per_minute(:search_rate_limit), + search_rate_limit_unauthenticated: application_setting_limit_per_minute(:search_rate_limit_unauthenticated), users_get_by_id: { enabled: application_settings[:users_get_by_id_limit] > 0, requests_per_period: application_settings[:users_get_by_id_limit], diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 2c95cc2672c..86da29dd27a 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -35,6 +35,7 @@ class Milestone < ApplicationRecord scope :with_api_entity_associations, -> { preload(project: [:project_feature, :route, namespace: :route]) } scope :order_by_dates_and_title, -> { order(due_date: :asc, start_date: :asc, title: :asc) } + validates :title, presence: true validates_associated :milestone_releases, message: -> (_, obj) { obj[:value].map(&:errors).map(&:full_messages).join(",") } validate :uniqueness_of_title, if: :title_changed? diff --git a/app/views/admin/application_settings/_search_limits.html.haml b/app/views/admin/application_settings/_search_limits.html.haml new file mode 100644 index 00000000000..24403fe8fd3 --- /dev/null +++ b/app/views/admin/application_settings/_search_limits.html.haml @@ -0,0 +1,16 @@ += form_for @application_setting, url: network_admin_application_settings_path(anchor: 'js-search-limits-settings'), html: { class: 'fieldset-form' } do |f| + = form_errors(@application_setting) + + %fieldset + .form-group + = f.label :search_rate_limit, _('Maximum authenticated requests by a user per minute'), class: 'label-bold' + .form-text.gl-text-gray-600 + = _("Set this number to 0 to disable the limit.") + + = f.label :search_rate_limit, _('Maximum number of requests per minute for an authenticated user'), class: 'label-bold' + .form-group + = f.label :search_rate_limit_unauthenticated, _('Maximum number of requests per minute for an unauthenticated IP address'), class: 'label-bold' + = f.number_field :search_rate_limit_unauthenticated, class: 'form-control gl-form-input' + + + = f.submit _('Save changes'), class: "gl-button btn btn-confirm", data: { qa_selector: 'save_changes_button' } diff --git a/app/views/admin/application_settings/network.html.haml b/app/views/admin/application_settings/network.html.haml index 90183b028f0..b0e3f8182f6 100644 --- a/app/views/admin/application_settings/network.html.haml +++ b/app/views/admin/application_settings/network.html.haml @@ -48,6 +48,17 @@ .settings-content = render partial: 'network_rate_limits', locals: { anchor: 'js-files-limits-settings', setting_fragment: 'files_api' } +%section.settings.as-note-limits.no-animate#js-search-limits-settings{ class: ('expanded' if expanded_by_default?) } + .settings-header + %h4 + = _('Search rate limits') + %button.btn.gl-button.btn-default.js-settings-toggle{ type: 'button' } + = expanded_by_default? ? _('Collapse') : _('Expand') + %p + = _('Set rate limits for searches performed by web or API requests.') + .settings-content + = render 'search_limits' + %section.settings.as-deprecated-limits.no-animate#js-deprecated-limits-settings{ class: ('expanded' if expanded_by_default?) } .settings-header %h4 diff --git a/app/views/admin/runners/edit.html.haml b/app/views/admin/runners/edit.html.haml index b65fead49ab..55fd09ac203 100644 --- a/app/views/admin/runners/edit.html.haml +++ b/app/views/admin/runners/edit.html.haml @@ -25,15 +25,12 @@ - if project %tr %td - .gl-alert.gl-alert-danger - .gl-alert-container - = sprite_icon('error', size: 16, css_class: 'gl-icon gl-alert-icon gl-alert-icon-no-title') - .gl-alert-content - .gl-alert-body - %strong - = project.full_name - .gl-alert-actions - = link_to _('Disable'), admin_namespace_project_runner_project_path(project.namespace, project, runner_project), method: :delete, class: 'btn gl-alert-action btn-confirm btn-md gl-button' + = render 'shared/global_alert', + variant: :danger, + dismissible: false, + title: project.full_name do + .gl-alert-actions + = link_to _('Disable'), admin_namespace_project_runner_project_path(project.namespace, project, runner_project), method: :delete, class: 'btn gl-alert-action btn-confirm btn-md gl-button' %table.table{ data: { testid: 'unassigned-projects' } } %thead diff --git a/config/feature_flags/development/iteration_cadences.yml b/config/feature_flags/development/iteration_cadences.yml index 2a496449a6a..c90743020d6 100644 --- a/config/feature_flags/development/iteration_cadences.yml +++ b/config/feature_flags/development/iteration_cadences.yml @@ -1,7 +1,7 @@ --- name: iteration_cadences introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/54822 -rollout_issue_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/354878 milestone: '13.10' type: development group: group::project management diff --git a/db/migrate/20220307203458_rename_user_email_lookup_limit_setting_to_search_settings.rb b/db/migrate/20220307203458_rename_user_email_lookup_limit_setting_to_search_settings.rb new file mode 100644 index 00000000000..62fe55b22f2 --- /dev/null +++ b/db/migrate/20220307203458_rename_user_email_lookup_limit_setting_to_search_settings.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class RenameUserEmailLookupLimitSettingToSearchSettings < Gitlab::Database::Migration[1.0] + def up + add_column :application_settings, :search_rate_limit, :integer, null: false, default: 30 + add_column :application_settings, :search_rate_limit_unauthenticated, :integer, null: false, default: 10 + end + + def down + remove_column :application_settings, :search_rate_limit + remove_column :application_settings, :search_rate_limit_unauthenticated + end +end diff --git a/db/post_migrate/20220307203459_rename_user_email_lookup_limit_setting_to_search_settings_cleanup.rb b/db/post_migrate/20220307203459_rename_user_email_lookup_limit_setting_to_search_settings_cleanup.rb new file mode 100644 index 00000000000..2d01374780d --- /dev/null +++ b/db/post_migrate/20220307203459_rename_user_email_lookup_limit_setting_to_search_settings_cleanup.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class RenameUserEmailLookupLimitSettingToSearchSettingsCleanup < Gitlab::Database::Migration[1.0] + class ApplicationSetting < ActiveRecord::Base + self.table_name = :application_settings + end + + def up + ApplicationSetting.update_all 'search_rate_limit=user_email_lookup_limit' + end + + def down + ApplicationSetting.update_all 'user_email_lookup_limit=search_rate_limit' + end +end diff --git a/db/schema_migrations/20220307203458 b/db/schema_migrations/20220307203458 new file mode 100644 index 00000000000..3063be46503 --- /dev/null +++ b/db/schema_migrations/20220307203458 @@ -0,0 +1 @@ +d4bf5f7c695c9833a07722d724b7a6363f0ebcb7f6d8a15bcf8148bdae5e1b32
\ No newline at end of file diff --git a/db/schema_migrations/20220307203459 b/db/schema_migrations/20220307203459 new file mode 100644 index 00000000000..2220fd3cb61 --- /dev/null +++ b/db/schema_migrations/20220307203459 @@ -0,0 +1 @@ +74f6687c0793a2596467338d8b4904bef712e6ff3ad3561e3ab2546eed5cd24d
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 42798928a6f..8cb77345afb 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -11251,6 +11251,8 @@ CREATE TABLE application_settings ( users_get_by_id_limit integer DEFAULT 300 NOT NULL, users_get_by_id_limit_allowlist text[] DEFAULT '{}'::text[] NOT NULL, container_registry_expiration_policies_caching boolean DEFAULT true NOT NULL, + search_rate_limit integer DEFAULT 30 NOT NULL, + search_rate_limit_unauthenticated integer DEFAULT 10 NOT NULL, CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)), CONSTRAINT app_settings_dep_proxy_ttl_policies_worker_capacity_positive CHECK ((dependency_proxy_ttl_group_policy_worker_capacity >= 0)), CONSTRAINT app_settings_ext_pipeline_validation_service_url_text_limit CHECK ((char_length(external_pipeline_validation_service_url) <= 255)), diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 8f89deb5b62..1b979b68745 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -12164,7 +12164,7 @@ Represents an iteration object. | <a id="iterationsequence"></a>`sequence` | [`Int!`](#int) | Sequence number for the iteration when you sort the containing cadence's iterations by the start and end date. The earliest starting and ending iteration is assigned 1. | | <a id="iterationstartdate"></a>`startDate` | [`Time`](#time) | Timestamp of the iteration start date. | | <a id="iterationstate"></a>`state` | [`IterationState!`](#iterationstate) | State of the iteration. | -| <a id="iterationtitle"></a>`title` | [`String!`](#string) | Title of the iteration. | +| <a id="iterationtitle"></a>`title` | [`String`](#string) | Title of the iteration. Title must be specified unless iteration_cadences feature flag is enabled. | | <a id="iterationupdatedat"></a>`updatedAt` | [`Time!`](#time) | Timestamp of last iteration update. | | <a id="iterationwebpath"></a>`webPath` | [`String!`](#string) | Web path of the iteration. | | <a id="iterationweburl"></a>`webUrl` | [`String!`](#string) | Web URL of the iteration. | diff --git a/doc/api/settings.md b/doc/api/settings.md index 5913569a0bb..b9c52ff01fd 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -390,7 +390,9 @@ listed in the descriptions of the relevant settings. | `push_event_hooks_limit` | integer | no | Number of changes (branches or tags) in a single push to determine whether webhooks and services fire or not. Webhooks and services aren't submitted if it surpasses that value. | | `rate_limiting_response_text` | string | no | When rate limiting is enabled via the `throttle_*` settings, send this plain text response when a rate limit is exceeded. 'Retry later' is sent if this is blank. | | `raw_blob_request_limit` | integer | no | Max number of requests per minute for each raw path. Default: 300. To disable throttling set to 0.| -| `user_email_lookup_limit` | integer | no | Max number of requests per minute for email lookup. Default: 60. To disable throttling set to 0.| +| `user_email_lookup_limit` | integer | no | **{warning}** **[Removed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80631/)** in GitLab 14.9. Replaced by `search_rate_limit`. Max number of requests per minute for email lookup. Default: 60. To disable throttling set to 0.| +| `search_rate_limit` | integer | no | Max number of requests per minute for performing a search while authenticated. Default: 30. To disable throttling set to 0.| +| `search_rate_limit_unauthenticated` | integer | no | Max number of requests per minute for performing a search while unauthenticated. Default: 10. To disable throttling set to 0.| | `recaptcha_enabled` | boolean | no | (**If enabled, requires:** `recaptcha_private_key` and `recaptcha_site_key`) Enable reCAPTCHA. | | `recaptcha_private_key` | string | required by: `recaptcha_enabled` | Private key for reCAPTCHA. | | `recaptcha_site_key` | string | required by: `recaptcha_enabled` | Site key for reCAPTCHA. | diff --git a/doc/user/profile/account/delete_account.md b/doc/user/profile/account/delete_account.md index 5a57f8902ca..3f5554b80ac 100644 --- a/doc/user/profile/account/delete_account.md +++ b/doc/user/profile/account/delete_account.md @@ -32,8 +32,10 @@ As an administrator, to delete a user account: 1. On the left sidebar, select **Overview > Users**. 1. Select a user. 1. Under the **Account** tab, select: - - **Delete user** to delete only the user but maintain their [associated records](#associated-records). - - **Delete user and contributions** to delete the user and their associated records. + - **Delete user** to delete only the user but maintain their [associated records](#associated-records). You can't use this option if + the selected user is the sole owner of any groups. + - **Delete user and contributions** to delete the user and their associated records. This option also removes all groups (and + projects within these groups) where the user is the sole direct Owner of a group. Inherited ownership doesn't apply. WARNING: Using the **Delete user and contributions** option may result in removing more data than intended. See @@ -60,7 +62,7 @@ When deleting users, you can either: An alternative to deleting is [blocking a user](../../admin_area/moderate_users.md#block-a-user). When a user is deleted from an [abuse report](../../admin_area/review_abuse_reports.md) or spam log, these associated -records are always removed. This includes any groups of which the user is the only user with the Owner role. +records are always removed. The deleting associated records option can be requested in the [API](../../../api/users.md#user-deletion) as well as the Admin Area. diff --git a/lib/api/search.rb b/lib/api/search.rb index 60a7e944b43..4ef8fef329c 100644 --- a/lib/api/search.rb +++ b/lib/api/search.rb @@ -7,7 +7,7 @@ module API before do authenticate! - check_rate_limit!(:user_email_lookup, scope: [current_user]) if search_service.params.email_lookup? + check_rate_limit!(:search_rate_limit, scope: [current_user]) end feature_category :global_search diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index d2a31938e89..0b0aaacbaff 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -39,7 +39,8 @@ module Gitlab profile_update_username: { threshold: 10, interval: 1.minute }, update_environment_canary_ingress: { threshold: 1, interval: 1.minute }, auto_rollback_deployment: { threshold: 1, interval: 3.minutes }, - user_email_lookup: { threshold: -> { application_settings.user_email_lookup_limit }, interval: 1.minute }, + search_rate_limit: { threshold: -> { application_settings.search_rate_limit }, interval: 1.minute }, + search_rate_limit_unauthenticated: { threshold: -> { application_settings.search_rate_limit_unauthenticated }, interval: 1.minute }, gitlab_shell_operation: { threshold: 600, interval: 1.minute } }.freeze end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 3d714035d5d..b4a0ff10965 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -22645,6 +22645,9 @@ msgstr "" msgid "Maximum authenticated API requests per rate limit period per user" msgstr "" +msgid "Maximum authenticated requests by a user per minute" +msgstr "" + msgid "Maximum authenticated web requests per rate limit period per user" msgstr "" @@ -22738,6 +22741,12 @@ msgstr "" msgid "Maximum number of projects." msgstr "" +msgid "Maximum number of requests per minute for an authenticated user" +msgstr "" + +msgid "Maximum number of requests per minute for an unauthenticated IP address" +msgstr "" + msgid "Maximum number of requests per minute for each raw path (default is 300). Set to 0 to disable throttling." msgstr "" @@ -32287,6 +32296,9 @@ msgstr "" msgid "Search projects..." msgstr "" +msgid "Search rate limits" +msgstr "" + msgid "Search refs" msgstr "" @@ -33707,6 +33719,9 @@ msgstr "" msgid "Set rate limits for package registry API requests that supersede the general user and IP rate limits." msgstr "" +msgid "Set rate limits for searches performed by web or API requests." +msgstr "" + msgid "Set severity" msgstr "" @@ -33752,6 +33767,9 @@ msgstr "" msgid "Set the timeout in seconds to send a secondary site status to the primary and IPs allowed for the secondary sites." msgstr "" +msgid "Set this number to 0 to disable the limit." +msgstr "" + msgid "Set time estimate" msgstr "" diff --git a/spec/controllers/autocomplete_controller_spec.rb b/spec/controllers/autocomplete_controller_spec.rb index 533d3896ee6..0a809e80fcd 100644 --- a/spec/controllers/autocomplete_controller_spec.rb +++ b/spec/controllers/autocomplete_controller_spec.rb @@ -235,7 +235,7 @@ RSpec.describe AutocompleteController do end end - it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do let(:current_user) { user } def request diff --git a/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb index 0f1501d4c3c..4778d4a77b8 100644 --- a/spec/controllers/search_controller_spec.rb +++ b/spec/controllers/search_controller_spec.rb @@ -291,7 +291,7 @@ RSpec.describe SearchController do end end - it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do let(:current_user) { user } def request @@ -355,7 +355,7 @@ RSpec.describe SearchController do expect(json_response).to eq({ 'count' => '0' }) end - it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do let(:current_user) { user } def request @@ -375,7 +375,7 @@ RSpec.describe SearchController do expect(json_response).to match_array([]) end - it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do let(:current_user) { user } def request @@ -445,6 +445,26 @@ RSpec.describe SearchController do end context 'unauthorized user' do + describe 'search rate limits' do + using RSpec::Parameterized::TableSyntax + + let(:project) { create(:project, :public) } + + where(:endpoint, :params) do + :show | { search: 'hello', scope: 'projects' } + :count | { search: 'hello', scope: 'projects' } + :autocomplete | { term: 'hello', scope: 'projects' } + end + + with_them do + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit_unauthenticated do + def request + get endpoint, params: params.merge(project_id: project.id) + end + end + end + end + describe 'GET #opensearch' do render_views diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index a33e6c04317..68e39e25282 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -388,6 +388,10 @@ FactoryBot.define do service_desk_enabled { true } end + trait :with_error_tracking_setting do + error_tracking_setting { association :project_error_tracking_setting } + end + # Project with empty repository # # This is a case when you just created a project diff --git a/spec/fixtures/api/schemas/list.json b/spec/fixtures/api/schemas/list.json index 65e140f9e28..0985874a500 100644 --- a/spec/fixtures/api/schemas/list.json +++ b/spec/fixtures/api/schemas/list.json @@ -34,7 +34,7 @@ "priority": { "type": ["integer", "null"] } } }, - "title": { "type": "string" }, + "title": { "type": ["string", "null"] }, "position": { "type": ["integer", "null"] }, "max_issue_count": { "type": "integer" }, "max_issue_weight": { "type": "integer" }, diff --git a/spec/helpers/application_settings_helper_spec.rb b/spec/helpers/application_settings_helper_spec.rb index 169b1c75995..c352fe1bdca 100644 --- a/spec/helpers/application_settings_helper_spec.rb +++ b/spec/helpers/application_settings_helper_spec.rb @@ -51,7 +51,7 @@ RSpec.describe ApplicationSettingsHelper do issues_create_limit notes_create_limit project_export_limit project_download_export_limit project_export_limit project_import_limit raw_blob_request_limit group_export_limit group_download_export_limit - group_import_limit users_get_by_id_limit user_email_lookup_limit + group_import_limit users_get_by_id_limit search_rate_limit search_rate_limit_unauthenticated )) end diff --git a/spec/helpers/projects/error_tracking_helper_spec.rb b/spec/helpers/projects/error_tracking_helper_spec.rb index 9f6b9241cbd..f49458be40d 100644 --- a/spec/helpers/projects/error_tracking_helper_spec.rb +++ b/spec/helpers/projects/error_tracking_helper_spec.rb @@ -5,8 +5,8 @@ require 'spec_helper' RSpec.describe Projects::ErrorTrackingHelper do include Gitlab::Routing.url_helpers - let_it_be(:project, reload: true) { create(:project) } - let_it_be(:current_user) { create(:user) } + let(:project) { build_stubbed(:project) } + let(:current_user) { build_stubbed(:user) } describe '#error_tracking_data' do let(:can_enable_error_tracking) { true } @@ -41,14 +41,14 @@ RSpec.describe Projects::ErrorTrackingHelper do end context 'with error_tracking_setting' do - let(:error_tracking_setting) do - create(:project_error_tracking_setting, project: project) + let(:project) { build_stubbed(:project, :with_error_tracking_setting) } + + before do + project.error_tracking_setting.enabled = enabled end context 'when enabled' do - before do - error_tracking_setting.update!(enabled: true) - end + let(:enabled) { true } it 'show error tracking enabled' do expect(helper.error_tracking_data(current_user, project)).to include( @@ -58,9 +58,7 @@ RSpec.describe Projects::ErrorTrackingHelper do end context 'when disabled' do - before do - error_tracking_setting.update!(enabled: false) - end + let(:enabled) { false } it 'show error tracking not enabled' do expect(helper.error_tracking_data(current_user, project)).to include( @@ -86,10 +84,11 @@ RSpec.describe Projects::ErrorTrackingHelper do with_them do before do stub_feature_flags(integrated_error_tracking: feature_flag) - error_tracking_setting.update_columns( + + project.error_tracking_setting.attributes = { enabled: enabled, integrated: integrated - ) + } end specify do diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index eb4428ca04a..70331e8d78a 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -143,7 +143,7 @@ RSpec.describe ApplicationSetting do it { is_expected.not_to allow_value('default' => 101).for(:repository_storages_weighted).with_message("value for 'default' must be between 0 and 100") } it { is_expected.not_to allow_value('default' => 100, shouldntexist: 50).for(:repository_storages_weighted).with_message("can't include: shouldntexist") } - %i[notes_create_limit user_email_lookup_limit users_get_by_id_limit].each do |setting| + %i[notes_create_limit search_rate_limit search_rate_limit_unauthenticated users_get_by_id_limit].each do |setting| it { is_expected.to allow_value(400).for(setting) } it { is_expected.not_to allow_value('two').for(setting) } it { is_expected.not_to allow_value(nil).for(setting) } diff --git a/spec/models/instance_configuration_spec.rb b/spec/models/instance_configuration_spec.rb index 6b0d8d7ca4a..3af717798c3 100644 --- a/spec/models/instance_configuration_spec.rb +++ b/spec/models/instance_configuration_spec.rb @@ -206,7 +206,8 @@ RSpec.describe InstanceConfiguration do group_download_export_limit: 1019, group_import_limit: 1020, raw_blob_request_limit: 1021, - user_email_lookup_limit: 1022, + search_rate_limit: 1022, + search_rate_limit_unauthenticated: 1000, users_get_by_id_limit: 1023 ) end @@ -230,7 +231,8 @@ RSpec.describe InstanceConfiguration do expect(rate_limits[:group_export_download]).to eq({ enabled: true, requests_per_period: 1019, period_in_seconds: 60 }) expect(rate_limits[:group_import]).to eq({ enabled: true, requests_per_period: 1020, period_in_seconds: 60 }) expect(rate_limits[:raw_blob]).to eq({ enabled: true, requests_per_period: 1021, period_in_seconds: 60 }) - expect(rate_limits[:user_email_lookup]).to eq({ enabled: true, requests_per_period: 1022, period_in_seconds: 60 }) + expect(rate_limits[:search_rate_limit]).to eq({ enabled: true, requests_per_period: 1022, period_in_seconds: 60 }) + expect(rate_limits[:search_rate_limit_unauthenticated]).to eq({ enabled: true, requests_per_period: 1000, period_in_seconds: 60 }) expect(rate_limits[:users_get_by_id]).to eq({ enabled: true, requests_per_period: 1023, period_in_seconds: 600 }) end end diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index bc592acc80f..06044cf53cc 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -65,6 +65,17 @@ RSpec.describe Milestone do allow(subject).to receive(:set_iid).and_return(false) end + describe 'title' do + it { is_expected.to validate_presence_of(:title) } + + it 'is invalid if title would be empty after sanitation', :aggregate_failures do + milestone = build(:milestone, project: project, title: '<img src=x onerror=prompt(1)>') + + expect(milestone).not_to be_valid + expect(milestone.errors[:title]).to include("can't be blank") + end + end + describe 'milestone_releases' do let(:milestone) { build(:milestone, project: project) } diff --git a/spec/requests/api/search_spec.rb b/spec/requests/api/search_spec.rb index 24cd95781c3..4d2a69cd85b 100644 --- a/spec/requests/api/search_spec.rb +++ b/spec/requests/api/search_spec.rb @@ -8,6 +8,11 @@ RSpec.describe API::Search do let_it_be(:project, reload: true) { create(:project, :wiki_repo, :public, name: 'awesome project', group: group) } let_it_be(:repo_project) { create(:project, :public, :repository, group: group) } + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:threshold).with(:search_rate_limit).and_return(1000) + allow(Gitlab::ApplicationRateLimiter).to receive(:threshold).with(:search_rate_limit_unauthenticated).and_return(1000) + end + shared_examples 'response is correct' do |schema:, size: 1| it { expect(response).to have_gitlab_http_status(:ok) } it { expect(response).to match_response_schema(schema) } @@ -347,7 +352,7 @@ RSpec.describe API::Search do end end - it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do let(:current_user) { user } def request @@ -522,7 +527,7 @@ RSpec.describe API::Search do it_behaves_like 'response is correct', schema: 'public_api/v4/user/basics' end - it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do let(:current_user) { user } def request @@ -803,7 +808,7 @@ RSpec.describe API::Search do end end - it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do let(:current_user) { user } def request diff --git a/spec/support/shared_examples/controllers/rate_limited_endpoint_shared_examples.rb b/spec/support/shared_examples/controllers/rate_limited_endpoint_shared_examples.rb index bb2a4159071..20edca1ee9f 100644 --- a/spec/support/shared_examples/controllers/rate_limited_endpoint_shared_examples.rb +++ b/spec/support/shared_examples/controllers/rate_limited_endpoint_shared_examples.rb @@ -13,10 +13,16 @@ RSpec.shared_examples 'rate limited endpoint' do |rate_limit_key:| env: :"#{rate_limit_key}_request_limit", remote_ip: kind_of(String), request_method: kind_of(String), - path: kind_of(String), - user_id: current_user.id, - username: current_user.username - } + path: kind_of(String) + }.merge(expected_user_attributes) + end + + let(:expected_user_attributes) do + if defined?(current_user) && current_user.present? + { user_id: current_user.id, username: current_user.username } + else + {} + end end let(:error_message) { _('This endpoint has been requested too many times. Try again later.') } diff --git a/spec/support/shared_examples/models/concerns/timebox_shared_examples.rb b/spec/support/shared_examples/models/concerns/timebox_shared_examples.rb index 39121b73bc5..a2b4cdc33d0 100644 --- a/spec/support/shared_examples/models/concerns/timebox_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/timebox_shared_examples.rb @@ -66,17 +66,6 @@ RSpec.shared_examples 'a timebox' do |timebox_type| end end - describe 'title' do - it { is_expected.to validate_presence_of(:title) } - - it 'is invalid if title would be empty after sanitation' do - timebox = build(timebox_type, *timebox_args, project: project, title: '<img src=x onerror=prompt(1)>') - - expect(timebox).not_to be_valid - expect(timebox.errors[:title]).to include("can't be blank") - end - end - describe '#timebox_type_check' do it 'is invalid if it has both project_id and group_id' do timebox = build(timebox_type, *timebox_args, group: group) |