Merge branch '320965-confidential-issue' into 'master'

Add users allowlist to ApplicationRateLimiter

See merge request gitlab-org/gitlab!53866

app/controllers/projects/notes_controller.rb

@@ -94,8 +94,7 @@ class Projects::NotesController < Projects::ApplicationController
 
   def create_rate_limit
     key = :notes_create
-
-    return unless rate_limiter.throttled?(key, scope: [current_user])
+    return unless rate_limiter.throttled?(key, scope: [current_user], users_allowlist: rate_limit_users_allowlist)
 
     rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user)
     render plain: _('This endpoint has been requested too many times. Try again later.'), status: :too_many_requests
@@ -104,4 +103,8 @@ class Projects::NotesController < Projects::ApplicationController
   def rate_limiter
     ::Gitlab::ApplicationRateLimiter
   end
+
+  def rate_limit_users_allowlist
+    Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist
+  end
 end

app/graphql/mutations/notes/create/base.rb

@@ -57,12 +57,18 @@ module Mutations
         end
 
         def verify_rate_limit!(current_user)
-          rate_limiter, key = ::Gitlab::ApplicationRateLimiter, :notes_create
-          return unless rate_limiter.throttled?(key, scope: [current_user])
+          return unless rate_limit_throttled?
 
           raise Gitlab::Graphql::Errors::ResourceNotAvailable,
             'This endpoint has been requested too many times. Try again later.'
         end
+
+        def rate_limit_throttled?
+          rate_limiter = ::Gitlab::ApplicationRateLimiter
+          allowlist = Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist
+
+          rate_limiter.throttled?(:notes_create, scope: [current_user], users_allowlist: allowlist)
+        end
       end
     end
   end

app/helpers/application_settings_helper.rb

@@ -329,6 +329,7 @@ module ApplicationSettingsHelper
       :email_restrictions,
       :issues_create_limit,
       :notes_create_limit,
+      :notes_create_limit_allowlist_raw,
       :raw_blob_request_limit,
       :project_import_limit,
       :project_export_limit,

app/models/application_setting.rb

@@ -447,6 +447,10 @@ class ApplicationSetting < ApplicationRecord
   validates :notes_create_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
+
   attr_encrypted :asset_proxy_secret_key,
                  mode: :per_attribute_iv,
                  key: Settings.attr_encrypted_db_key_base_truncated,

app/models/application_setting_implementation.rb

@@ -93,7 +93,6 @@ module ApplicationSettingImplementation
         import_sources: Settings.gitlab['import_sources'],
         invisible_captcha_enabled: false,
         issues_create_limit: 300,
-        notes_create_limit: 300,
         local_markdown_version: 0,
         login_recaptcha_protection_enabled: false,
         max_artifacts_size: Settings.artifacts['max_size'],
@@ -101,6 +100,8 @@ module ApplicationSettingImplementation
         max_import_size: 0,
         minimum_password_length: DEFAULT_MINIMUM_PASSWORD_LENGTH,
         mirror_available: true,
+        notes_create_limit: 300,
+        notes_create_limit_allowlist: [],
         notify_on_unknown_sign_in: true,
         outbound_local_requests_whitelist: [],
         password_authentication_enabled_for_git: true,
@@ -270,6 +271,14 @@ module ApplicationSettingImplementation
     self.protected_paths = strings_to_array(values)
   end
 
+  def notes_create_limit_allowlist_raw
+    array_to_string(self.notes_create_limit_allowlist)
+  end
+
+  def notes_create_limit_allowlist_raw=(values)
+    self.notes_create_limit_allowlist = strings_to_array(values).map(&:downcase)
+  end
+
   def asset_proxy_allowlist=(values)
     values = strings_to_array(values) if values.is_a?(String)
 

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

@@ -5,5 +5,8 @@
     .form-group
       = f.label :notes_create_limit, _('Max requests per minute per user'), class: 'label-bold'
       = f.number_field :notes_create_limit, class: 'form-control gl-form-input'
+    .form-group
+      = f.label :notes_create_limit_allowlist, _('List of users to be excluded from the limit'), class: 'label-bold'
+      = f.text_area :notes_create_limit_allowlist_raw, placeholder: 'username1, username2', class: 'form-control gl-form-input', rows: 5
 
   = f.submit _('Save changes'), class: "gl-button btn btn-success", data: { qa_selector: 'save_changes_button' }

changelogs/unreleased/320965-confidential-issue.yml

+---
+title: Add an allowlist to exclude users from the rate limit on notes creation
+merge_request: 53866
+author:
+type: added

db/migrate/20210210210232_add_notes_create_limit_allowlist_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddNotesCreateLimitAllowlistToApplicationSettings < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :application_settings, :notes_create_limit_allowlist, :text, array: true, default: [], null: false
+  end
+end

db/schema_migrations/20210210210232

+e1bd58eeaf63caf473680a8c4b7269cc63e7c0d6e8d4e71636608e10c9731c85
\ No newline at end of file

db/structure.sql

@@ -9414,6 +9414,7 @@ CREATE TABLE application_settings (
     asset_proxy_allowlist text,
     keep_latest_artifact boolean DEFAULT true NOT NULL,
     notes_create_limit integer DEFAULT 300 NOT NULL,
+    notes_create_limit_allowlist text[] DEFAULT '{}'::text[] 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_registry_exp_policies_worker_capacity_positive CHECK ((container_registry_expiration_policies_worker_capacity >= 0)),
     CONSTRAINT check_17d9558205 CHECK ((char_length((kroki_url)::text) <= 1024)),

lib/api/helpers/rate_limiter.rb

@@ -3,8 +3,8 @@
 module API
   module Helpers
     module RateLimiter
-      def check_rate_limit!(key, scope)
-        if rate_limiter.throttled?(key, scope: scope)
+      def check_rate_limit!(key, scope, users_allowlist = nil)
+        if rate_limiter.throttled?(key, scope: scope, users_allowlist: users_allowlist)
           log_request(key)
           render_exceeded_limit_error!
         end

lib/api/notes.rb

@@ -73,7 +73,9 @@ module API
           optional :created_at, type: String, desc: 'The creation date of the note'
         end
         post ":id/#{noteables_str}/:noteable_id/notes", feature_category: feature_category do
-          check_rate_limit! :notes_create, [current_user]
+          allowlist =
+            Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist
+          check_rate_limit! :notes_create, [current_user], allowlist
           noteable = find_noteable(noteable_type, params[:noteable_id])
 
           opts = {

lib/gitlab/application_rate_limiter.rb

@@ -47,15 +47,17 @@ module Gitlab
       # @option scope [Array<ActiveRecord>] Array of ActiveRecord models to scope throttling to a specific request (e.g. per user per project)
       # @option threshold [Integer] Optional threshold value to override default one registered in `.rate_limits`
       # @option interval [Integer] Optional interval value to override default one registered in `.rate_limits`
+      # @option users_allowlist [Array<String>] Optional list of usernames to excepted from the limit. This param will only be functional if Scope includes a current user.
       #
       # @return [Boolean] Whether or not a request should be throttled
-      def throttled?(key, scope: nil, interval: nil, threshold: nil)
+      def throttled?(key, **options)
         return unless rate_limits[key]
 
-        threshold_value = threshold || threshold(key)
+        return if scoped_user_in_allowlist?(options)
 
+        threshold_value = options[:threshold] || threshold(key)
         threshold_value > 0 &&
-          increment(key, scope, interval) > threshold_value
+          increment(key, options[:scope], options[:interval]) > threshold_value
       end
 
       # Increments the given cache key and increments the value by 1 with the
@@ -141,6 +143,15 @@ module Gitlab
       def application_settings
         Gitlab::CurrentSettings.current_application_settings
       end
+
+      def scoped_user_in_allowlist?(options)
+        return unless options[:users_allowlist].present?
+
+        scoped_user = [options[:scope]].flatten.find { |s| s.is_a?(User) }
+        return unless scoped_user
+
+        scoped_user.username.downcase.in?(options[:users_allowlist])
+      end
     end
   end
 end

locale/gitlab.pot

@@ -17661,6 +17661,9 @@ msgstr ""
 msgid "List of all merge commits"
 msgstr ""
 
+msgid "List of users to be excluded from the limit"
+msgstr ""
+
 msgid "List options"
 msgstr ""
 

spec/controllers/projects/notes_controller_spec.rb

@@ -730,11 +730,11 @@ RSpec.describe Projects::NotesController do
 
     context 'when the endpoint receives requests above the limit' do
       before do
-        stub_application_setting(notes_create_limit: 5)
+        stub_application_setting(notes_create_limit: 3)
       end
 
       it 'prevents from creating more notes', :request_store do
-        5.times { create! }
+        3.times { create! }
 
         expect { create! }
           .to change { Gitlab::GitalyClient.get_request_count }.by(0)
@@ -760,7 +760,16 @@ RSpec.describe Projects::NotesController do
         project.add_developer(user)
         sign_in(user)
 
-        6.times { create! }
+        4.times { create! }
+      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}"])
+        3.times { create! }
+
+        create!
+        expect(response).to have_gitlab_http_status(:found)
       end
     end
   end

spec/models/application_setting_spec.rb

@@ -120,6 +120,15 @@ RSpec.describe ApplicationSetting do
     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) }
 
+    def many_usernames(num = 100)
+      Array.new(num) { |i| "username#{i}" }
+    end
+
+    it { is_expected.to allow_value(many_usernames(100)).for(:notes_create_limit_allowlist) }
+    it { is_expected.not_to allow_value(many_usernames(101)).for(:notes_create_limit_allowlist) }
+    it { is_expected.not_to allow_value(nil).for(:notes_create_limit_allowlist) }
+    it { is_expected.to allow_value([]).for(:notes_create_limit_allowlist) }
+
     context 'help_page_documentation_base_url validations' do
       it { is_expected.to allow_value(nil).for(:help_page_documentation_base_url) }
       it { is_expected.to allow_value('https://docs.gitlab.com').for(:help_page_documentation_base_url) }

spec/support/shared_examples/graphql/notes_creation_shared_examples.rb

@@ -74,4 +74,12 @@ RSpec.shared_examples 'a Note mutation when there are rate limit validation erro
   it_behaves_like 'a Note mutation that does not create a Note'
   it_behaves_like 'a mutation that returns top-level errors',
                   errors: ['This endpoint has been requested too many times. Try again later.']
+
+  context 'when the user is in the allowlist' do
+    before do
+      stub_application_setting(notes_create_limit_allowlist: ["#{current_user.username}"])
+    end
+
+    it_behaves_like 'a Note mutation that creates a Note'
+  end
 end

spec/support/shared_examples/requests/api/notes_shared_examples.rb

@@ -127,6 +127,12 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
   end
 
   describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/notes" do
+    let(:params) { { body: 'hi!' } }
+
+    subject do
+      post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params: params
+    end
+
     it "creates a new note" do
       post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params: { body: 'hi!' }
 
@@ -277,15 +283,25 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
 
     context 'when request exceeds the rate limit' do
       before do
-        allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true)
+        stub_application_setting(notes_create_limit: 1)
+        allow(::Gitlab::ApplicationRateLimiter).to receive(:increment).and_return(2)
       end
 
-      it 'prevents users from creating more notes' do
-        post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params: { body: 'hi!' }
+      it 'prevents user from creating more notes' do
+        subject
 
         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.')
       end
+
+      it 'allows user in allow-list to create notes' do
+        stub_application_setting(notes_create_limit_allowlist: ["#{user.username}"])
+        subject
+
+        expect(response).to have_gitlab_http_status(:created)
+        expect(json_response['body']).to eq('hi!')
+        expect(json_response['author']['username']).to eq(user.username)
+      end
     end
   end