Merge branch 'generalize-user-email-lookup-rate-limit' into 'master'

Add configurable search rate limits

See merge request gitlab-org/gitlab!80631

app/controllers/autocomplete_controller.rb

 # 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')

app/controllers/concerns/search_rate_limitable.rb

+# 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

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')

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,

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

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: []
       }

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],

app/views/admin/application_settings/_search_limits.html.haml

+= 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' }

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

db/migrate/20220307203458_rename_user_email_lookup_limit_setting_to_search_settings.rb

+# 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

db/post_migrate/20220307203459_rename_user_email_lookup_limit_setting_to_search_settings_cleanup.rb

+# 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

db/schema_migrations/20220307203458

+d4bf5f7c695c9833a07722d724b7a6363f0ebcb7f6d8a15bcf8148bdae5e1b32
\ No newline at end of file

db/schema_migrations/20220307203459

+74f6687c0793a2596467338d8b4904bef712e6ff3ad3561e3ab2546eed5cd24d
\ No newline at end of file

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)),

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. |

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

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

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 ""
 

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

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
 

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
 

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) }

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

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

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.') }