Merge branch '346140-users-rate-limiting' into 'master'

Add rate limiting for user email lookup

See merge request gitlab-org/gitlab!76760

app/controllers/autocomplete_controller.rb

@@ -2,6 +2,7 @@
 
 class AutocompleteController < ApplicationController
   skip_before_action :authenticate_user!, only: [:users, :award_emojis, :merge_request_target_branches]
+  before_action :check_email_search_rate_limit!, only: [:users]
 
   feature_category :users, [:users, :user]
   feature_category :projects, [:projects]
@@ -71,6 +72,12 @@ 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/search_controller.rb

@@ -17,6 +17,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]
 
   rescue_from ActiveRecord::QueryCanceled, with: :render_timeout
 
@@ -198,6 +199,12 @@ 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

@@ -411,7 +411,8 @@ module ApplicationSettingsHelper
       :sidekiq_job_limiter_mode,
       :sidekiq_job_limiter_compression_threshold_bytes,
       :sidekiq_job_limiter_limit_bytes,
-      :suggest_pipeline_enabled
+      :suggest_pipeline_enabled,
+      :user_email_lookup_limit
     ].tap do |settings|
       settings << :deactivate_dormant_users unless Gitlab.com?
     end

app/models/application_setting.rb

@@ -500,6 +500,9 @@ class ApplicationSetting < ApplicationRecord
   validates :notes_create_limit,
             numericality: { only_integer: true, greater_than_or_equal_to: 0 }
 
+  validates :user_email_lookup_limit,
+            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

@@ -222,7 +222,8 @@ module ApplicationSettingImplementation
         kroki_formats: { blockdiag: false, bpmn: false, excalidraw: false },
         rate_limiting_response_text: nil,
         whats_new_variant: 0,
-        user_deactivation_emails_enabled: true
+        user_deactivation_emails_enabled: true,
+        user_email_lookup_limit: 60
       }
     end
 

app/models/instance_configuration.rb

@@ -117,7 +117,8 @@ class InstanceConfiguration
       group_export: application_setting_limit_per_minute(:group_export_limit),
       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)
+      raw_blob: application_setting_limit_per_minute(:raw_blob_request_limit),
+      user_email_lookup: application_setting_limit_per_minute(:user_email_lookup_limit)
     }
   end
 

app/services/search_service.rb

@@ -7,6 +7,8 @@ class SearchService
   DEFAULT_PER_PAGE = Gitlab::SearchResults::DEFAULT_PER_PAGE
   MAX_PER_PAGE = 200
 
+  attr_reader :params
+
   def initialize(current_user, params = {})
     @current_user = current_user
     @params = Gitlab::Search::Params.new(params, detect_abuse: prevent_abusive_searches?)
@@ -159,7 +161,7 @@ class SearchService
       end
   end
 
-  attr_reader :current_user, :params
+  attr_reader :current_user
 end
 
 SearchService.prepend_mod_with('SearchService')

db/migrate/20211213142344_add_settings_user_email_lookup_limit.rb

+# frozen_string_literal: true
+
+class AddSettingsUserEmailLookupLimit < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    add_column :application_settings, :user_email_lookup_limit, :integer, null: false, default: 60
+  end
+
+  def down
+    remove_column :application_settings, :user_email_lookup_limit
+  end
+end

db/schema_migrations/20211213142344

+837539e12be12830d388bc6142622412b40ac061c397504eac03033a08f01e72
\ No newline at end of file

db/structure.sql

@@ -10481,6 +10481,7 @@ CREATE TABLE application_settings (
     max_ssh_key_lifetime integer,
     static_objects_external_storage_auth_token_encrypted text,
     future_subscriptions jsonb DEFAULT '[]'::jsonb NOT NULL,
+    user_email_lookup_limit integer DEFAULT 60 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)),

ee/spec/controllers/groups/epics/notes_controller_spec.rb

@@ -100,9 +100,12 @@ RSpec.describe Groups::Epics::NotesController do
       end
     end
 
-    it_behaves_like 'request exceeding rate limit', :clean_gitlab_redis_cache do
-      let(:params) { request_params.except(:format) }
-      let(:request_full_path) { group_epic_notes_path(group, epic) }
+    it_behaves_like 'create notes request exceeding rate limit', :clean_gitlab_redis_cache do
+      let(:current_user) { user }
+
+      def request
+        post :create, params: request_params.except(:format)
+      end
     end
   end
 

ee/spec/controllers/projects/security/vulnerabilities/notes_controller_spec.rb

@@ -139,9 +139,12 @@ RSpec.describe Projects::Security::Vulnerabilities::NotesController do
       end
     end
 
-    it_behaves_like 'request exceeding rate limit', :clean_gitlab_redis_cache do
-      let(:params) { request_params.except(:format) }
-      let(:request_full_path) { project_security_vulnerability_notes_path(project, vulnerability) }
+    it_behaves_like 'create notes request exceeding rate limit', :clean_gitlab_redis_cache do
+      let_it_be(:current_user, reload: true) { user }
+
+      def request
+        post :create, params: request_params.except(:format)
+      end
     end
   end
 

lib/api/search.rb

@@ -4,7 +4,11 @@ module API
   class Search < ::API::Base
     include PaginationParams
 
-    before { authenticate! }
+    before do
+      authenticate!
+
+      check_rate_limit!(:user_email_lookup, scope: [current_user]) if search_service.params.email_lookup?
+    end
 
     feature_category :global_search
 
@@ -36,7 +40,7 @@ module API
         }.freeze
       end
 
-      def search(additional_params = {})
+      def search_service(additional_params = {})
         search_params = {
           scope: params[:scope],
           search: params[:search],
@@ -50,7 +54,11 @@ module API
           sort: params[:sort]
         }.merge(additional_params)
 
-        results = SearchService.new(current_user, search_params).search_objects(preload_method)
+        SearchService.new(current_user, search_params)
+      end
+
+      def search(additional_params = {})
+        results = search_service(additional_params).search_objects(preload_method)
 
         Gitlab::UsageDataCounters::SearchCounter.count(:all_searches)
 

lib/gitlab/application_rate_limiter.rb

@@ -52,7 +52,8 @@ module Gitlab
           users_get_by_id:              { threshold: 10, interval: 1.minute },
           profile_resend_email_confirmation:  { threshold: 5, interval: 1.minute },
           update_environment_canary_ingress:  { threshold: 1, interval: 1.minute },
-          auto_rollback_deployment:           { threshold: 1, interval: 3.minutes }
+          auto_rollback_deployment:           { threshold: 1, interval: 3.minutes },
+          user_email_lookup:            { threshold: -> { application_settings.user_email_lookup_limit }, interval: 1.minute }
         }.freeze
       end
 

lib/gitlab/repository_archive_rate_limiter.rb

@@ -3,7 +3,7 @@
 module Gitlab
   module RepositoryArchiveRateLimiter
     def check_archive_rate_limit!(current_user, project, &block)
-      return unless Feature.enabled?(:archive_rate_limit)
+      return unless Feature.enabled?(:archive_rate_limit, default_enabled: :yaml)
 
       threshold = current_user ? nil : 100
 

lib/gitlab/search/params.rb

@@ -7,6 +7,7 @@ module Gitlab
 
       SEARCH_CHAR_LIMIT = 4096
       SEARCH_TERM_LIMIT = 64
+      MIN_TERM_LENGTH = 3
 
       # Generic validation
       validates :query_string, length: { maximum: SEARCH_CHAR_LIMIT }
@@ -53,6 +54,10 @@ module Gitlab
         errors[:query_string].none? { |msg| msg.include? SEARCH_TERM_LIMIT.to_s }
       end
 
+      def email_lookup?
+        search_terms.any? { |term| term =~ URI::MailTo::EMAIL_REGEXP }
+      end
+
       def validate
         if detect_abuse?
           abuse_detection.validate
@@ -75,8 +80,12 @@ module Gitlab
         @detect_abuse
       end
 
+      def search_terms
+        @search_terms ||= query_string.split.select { |word| word.length >= MIN_TERM_LENGTH }
+      end
+
       def not_too_many_terms
-        if query_string.split.count { |word| word.length >= 3 } > SEARCH_TERM_LIMIT
+        if search_terms.count > SEARCH_TERM_LIMIT
           errors.add :query_string, "has too many search terms (maximum is #{SEARCH_TERM_LIMIT})"
         end
       end

spec/controllers/autocomplete_controller_spec.rb

@@ -234,6 +234,18 @@ RSpec.describe AutocompleteController do
         expect(json_response.first).to have_key('can_merge')
       end
     end
+
+    it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do
+      let(:current_user) { user }
+
+      def request
+        get(:users, params: { search: 'foo@bar.com' })
+      end
+
+      before do
+        sign_in(current_user)
+      end
+    end
   end
 
   context 'GET projects' do

spec/controllers/projects/notes_controller_spec.rb

@@ -762,9 +762,12 @@ RSpec.describe Projects::NotesController do
       end
     end
 
-    it_behaves_like 'request exceeding rate limit', :clean_gitlab_redis_cache do
-      let(:params) { request_params.except(:format) }
-      let(:request_full_path) { project_notes_path(project) }
+    it_behaves_like 'create notes request exceeding rate limit', :clean_gitlab_redis_cache do
+      let(:current_user) { user }
+
+      def request
+        post :create, params: request_params.except(:format)
+      end
     end
   end
 

spec/controllers/search_controller_spec.rb

@@ -290,6 +290,14 @@ RSpec.describe SearchController do
           expect(assigns[:search_objects].count).to eq(0)
         end
       end
+
+      it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do
+        let(:current_user) { user }
+
+        def request
+          get(:show, params: { search: 'foo@bar.com', scope: 'users' })
+        end
+      end
     end
 
     describe 'GET #count', :aggregate_failures do
@@ -346,6 +354,14 @@ RSpec.describe SearchController do
         expect(response).to have_gitlab_http_status(:ok)
         expect(json_response).to eq({ 'count' => '0' })
       end
+
+      it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do
+        let(:current_user) { user }
+
+        def request
+          get(:count, params: { search: 'foo@bar.com', scope: 'users' })
+        end
+      end
     end
 
     describe 'GET #autocomplete' do
@@ -358,6 +374,14 @@ RSpec.describe SearchController do
         expect(response).to have_gitlab_http_status(:ok)
         expect(json_response).to match_array([])
       end
+
+      it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do
+        let(:current_user) { user }
+
+        def request
+          get(:autocomplete, params: { term: 'foo@bar.com', scope: 'users' })
+        end
+      end
     end
 
     describe '#append_info_to_payload' do

spec/controllers/snippets/notes_controller_spec.rb

@@ -142,9 +142,12 @@ RSpec.describe Snippets::NotesController do
         expect { post :create, params: request_params }.to change { Note.count }.by(1)
       end
 
-      it_behaves_like 'request exceeding rate limit', :clean_gitlab_redis_cache do
-        let(:params) { request_params }
-        let(:request_full_path) { snippet_notes_path(public_snippet) }
+      it_behaves_like 'create notes request exceeding rate limit', :clean_gitlab_redis_cache do
+        let(:current_user) { user }
+
+        def request
+          post :create, params: request_params
+        end
       end
     end
 
@@ -170,9 +173,12 @@ RSpec.describe Snippets::NotesController do
         expect { post :create, params: request_params }.to change { Note.count }.by(1)
       end
 
-      it_behaves_like 'request exceeding rate limit', :clean_gitlab_redis_cache do
-        let(:params) { request_params }
-        let(:request_full_path) { snippet_notes_path(internal_snippet) }
+      it_behaves_like 'create notes request exceeding rate limit', :clean_gitlab_redis_cache do
+        let(:current_user) { user }
+
+        def request
+          post :create, params: request_params
+        end
       end
     end
 
@@ -239,10 +245,12 @@ RSpec.describe Snippets::NotesController do
           expect { post :create, params: request_params }.to change { Note.count }.by(1)
         end
 
-        it_behaves_like 'request exceeding rate limit', :clean_gitlab_redis_cache do
-          let(:params) { request_params }
-          let(:request_full_path) { snippet_notes_path(private_snippet) }
-          let(:user) { private_snippet.author }
+        it_behaves_like 'create notes request exceeding rate limit', :clean_gitlab_redis_cache do
+          let(:current_user) { private_snippet.author }
+
+          def request
+            post :create, params: request_params
+          end
         end
       end
     end

spec/lib/gitlab/search/params_spec.rb

@@ -133,4 +133,12 @@ RSpec.describe Gitlab::Search::Params do
       end
     end
   end
+
+  describe '#email_lookup?' do
+    it 'is true if at least 1 word in search is an email' do
+      expect(described_class.new({ search: 'email@example.com' })).to be_email_lookup
+      expect(described_class.new({ search: 'foo email@example.com bar' })).to be_email_lookup
+      expect(described_class.new({ search: 'foo bar' })).not_to be_email_lookup
+    end
+  end
 end

spec/models/application_setting_spec.rb

@@ -126,11 +126,13 @@ 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") }
 
-    it { is_expected.to allow_value(400).for(:notes_create_limit) }
-    it { is_expected.not_to allow_value('two').for(:notes_create_limit) }
-    it { is_expected.not_to allow_value(nil).for(:notes_create_limit) }
-    it { is_expected.not_to allow_value(5.5).for(:notes_create_limit) }
-    it { is_expected.not_to allow_value(-2).for(:notes_create_limit) }
+    %i[notes_create_limit user_email_lookup_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) }
+      it { is_expected.not_to allow_value(5.5).for(setting) }
+      it { is_expected.not_to allow_value(-2).for(setting) }
+    end
 
     def many_usernames(num = 100)
       Array.new(num) { |i| "username#{i}" }

spec/models/instance_configuration_spec.rb

@@ -205,7 +205,8 @@ RSpec.describe InstanceConfiguration do
             group_export_limit: 1018,
             group_download_export_limit: 1019,
             group_import_limit: 1020,
-            raw_blob_request_limit: 1021
+            raw_blob_request_limit: 1021,
+            user_email_lookup_limit: 1022
           )
         end
 
@@ -228,6 +229,7 @@ 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 })
         end
       end
     end

spec/requests/api/search_spec.rb

@@ -346,6 +346,14 @@ RSpec.describe API::Search do
         end
       end
     end
+
+    it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do
+      let(:current_user) { user }
+
+      def request
+        get api(endpoint, current_user), params: { scope: 'users', search: 'foo@bar.com' }
+      end
+    end
   end
 
   describe "GET /groups/:id/search" do
@@ -513,6 +521,14 @@ 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
+        let(:current_user) { user }
+
+        def request
+          get api(endpoint, current_user), params: { scope: 'users', search: 'foo@bar.com' }
+        end
+      end
     end
   end
 
@@ -786,6 +802,14 @@ RSpec.describe API::Search do
           end
         end
       end
+
+      it_behaves_like 'rate limited endpoint', rate_limit_key: :user_email_lookup do
+        let(:current_user) { user }
+
+        def request
+          get api(endpoint, current_user), params: { scope: 'users', search: 'foo@bar.com' }
+        end
+      end
     end
   end
 end

spec/support/shared_examples/controllers/create_notes_rate_limit_shared_examples.rb

@@ -3,44 +3,19 @@
 # Requires a context containing:
 # - user
 # - params
-# - request_full_path
 
-RSpec.shared_examples 'request exceeding rate limit' do
-  context 'with rate limiter', :freeze_time, :clean_gitlab_redis_rate_limiting do
-    before do
-      stub_application_setting(notes_create_limit: 2)
-      2.times { post :create, params: params }
-    end
+RSpec.shared_examples 'create notes request exceeding rate limit' do
+  include_examples 'rate limited endpoint', rate_limit_key: :notes_create
 
-    it 'prevents from creating more notes' do
-      expect { post :create, params: params }
-        .to change { Note.count }.by(0)
+  it 'allows user in allow-list to create notes, even if the case is different', :freeze_time, :clean_gitlab_redis_rate_limiting do
+    allow(Gitlab::ApplicationRateLimiter).to receive(:threshold).with(:notes_create).and_return(1)
 
-      expect(response).to have_gitlab_http_status(:too_many_requests)
-      expect(response.body).to eq(_('This endpoint has been requested too many times. Try again later.'))
-    end
+    current_user.update_attribute(:username, current_user.username.titleize)
+    stub_application_setting(notes_create_limit_allowlist: [current_user.username.downcase])
 
-    it 'logs the event in auth.log' do
-      attributes = {
-        message: 'Application_Rate_Limiter_Request',
-        env: :notes_create_request_limit,
-        remote_ip: '0.0.0.0',
-        request_method: 'POST',
-        path: request_full_path,
-        user_id: user.id,
-        username: user.username
-      }
+    request
+    request
 
-      expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
-      post :create, params: params
-    end
-
-    it 'allows user in allow-list to create notes, even if the case is different' do
-      user.update_attribute(:username, user.username.titleize)
-      stub_application_setting(notes_create_limit_allowlist: ["#{user.username.downcase}"])
-
-      post :create, params: params
-      expect(response).to have_gitlab_http_status(:found)
-    end
+    expect(response).to have_gitlab_http_status(:found)
   end
 end

spec/support/shared_examples/controllers/rate_limited_endpoint_shared_examples.rb

+# frozen_string_literal: true
+#
+# Requires a context containing:
+# - request (use method definition to avoid memoizing!)
+# - current_user
+# - error_message # optional
+
+RSpec.shared_examples 'rate limited endpoint' do |rate_limit_key:|
+  context 'when rate limiter enabled', :freeze_time, :clean_gitlab_redis_rate_limiting do
+    let(:expected_logger_attributes) do
+      {
+        message: 'Application_Rate_Limiter_Request',
+        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
+      }
+    end
+
+    let(:error_message) { _('This endpoint has been requested too many times. Try again later.') }
+
+    before do
+      allow(Gitlab::ApplicationRateLimiter).to receive(:threshold).with(rate_limit_key).and_return(1)
+    end
+
+    it 'logs request and declines it when endpoint called more than the threshold' do |example|
+      expect(Gitlab::AuthLogger).to receive(:error).with(expected_logger_attributes).once
+
+      request
+      request
+
+      expect(response).to have_gitlab_http_status(:too_many_requests)
+
+      if example.metadata[:type] == :controller
+        expect(response.body).to eq(error_message)
+      else # it is API spec
+        expect(response.body).to eq({ message: { error: error_message } }.to_json)
+      end
+    end
+  end
+
+  context 'when rate limiter is disabled' do
+    before do
+      allow(Gitlab::ApplicationRateLimiter).to receive(:threshold).with(rate_limit_key).and_return(0)
+    end
+
+    it 'does not log request and does not block the request' do
+      expect(Gitlab::AuthLogger).not_to receive(:error)
+
+      request
+
+      expect(response).not_to have_gitlab_http_status(:too_many_requests)
+    end
+  end
+end