Change allowlist to application settings

- Add new column to applicaton_settings table
- Use the new settings value in rate limit checks

app/controllers/projects/notes_controller.rb

@@ -105,6 +105,6 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def rate_limit_users_allowlist
-    Gitlab.config.gitlab.notes_rate_limit_users_allowlist.split(', ')
+    Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist_raw
   end
 end

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

@@ -57,13 +57,18 @@ module Mutations
         end
 
         def verify_rate_limit!(current_user)
-          rate_limiter, key = ::Gitlab::ApplicationRateLimiter, :notes_create
-          allowlist = Gitlab.config.gitlab.notes_rate_limit_users_allowlist.split(', ')
-          return unless rate_limiter.throttled?(key, scope: [current_user], users_allowlist: allowlist)
+          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_raw
+
+          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_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)
+  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 excluse users from the rate limit on notes creation
+title: Add an allowlist to exclude users from the rate limit on notes creation
 merge_request: 53866
 author:
 type: added

config/initializers/1_settings.rb

@@ -214,7 +214,6 @@ Settings.gitlab['no_todos_messages'] ||= YAML.load_file(Rails.root.join('config'
 Settings.gitlab['impersonation_enabled'] ||= true if Settings.gitlab['impersonation_enabled'].nil?
 Settings.gitlab['usage_ping_enabled'] = true if Settings.gitlab['usage_ping_enabled'].nil?
 Settings.gitlab['max_request_duration_seconds'] ||= 57
-Settings.gitlab['notes_rate_limit_users_allowlist'] ||= ENV['NOTES_RATE_LIMIT_USERS_ALLOWLIST'] || ""
 
 Gitlab.ee do
   Settings.gitlab['mirror_max_delay'] ||= 300

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, :string, array: true, limit: 255, default: []
+  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 character varying(255)[] DEFAULT '{}'::character varying[],
     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)),

doc/administration/environment_variables.md

@@ -33,7 +33,6 @@ You can use the following environment variables to override certain values:
 | `GITLAB_UNICORN_MEMORY_MIN`                | integer | The minimum memory threshold (in bytes) for the [unicorn-worker-killer](operations/unicorn.md#unicorn-worker-killer).      |
 | `RAILS_ENV`                                | string  | The Rails environment; can be one of `production`, `development`, `staging`, or `test`.                 |
 | `UNSTRUCTURED_RAILS_LOG`                   | string  | Enables the unstructured log in addition to JSON logs (defaults to `true`).                             |
-| `NOTES_RATE_LIMIT_USERS_ALLOWLIST`         | string  | Sets the list of usernames to be excluded from the notes creation rate limit.                           |
 
 ## Complete database variables
 

lib/api/notes.rb

@@ -73,7 +73,8 @@ 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
-          allowlist = Gitlab.config.gitlab.notes_rate_limit_users_allowlist.split(', ')
+          allowlist =
+            Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist_raw
           check_rate_limit! :notes_create, [current_user], allowlist
           noteable = find_noteable(noteable_type, params[:noteable_id])
 

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

@@ -764,7 +764,7 @@ RSpec.describe Projects::NotesController do
       end
 
       it 'allows user in allow-list to create notes' do
-        stub_config_setting(notes_rate_limit_users_allowlist: "#{user.username}")
+        stub_application_setting(notes_create_limit_allowlist_raw: ["#{user.username}"])
         3.times { create! }
 
         create!

spec/models/application_setting_spec.rb

@@ -120,6 +120,11 @@ 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) }
 
+    it { is_expected.to allow_value(['username'] * 100).for(:notes_create_limit_allowlist) }
+    it { is_expected.not_to allow_value(['username'] * 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

@@ -77,7 +77,7 @@ RSpec.shared_examples 'a Note mutation when there are rate limit validation erro
 
   context 'when the user is in the allowlist' do
     before do
-      stub_config_setting(notes_rate_limit_users_allowlist: "#{current_user.username}")
+      stub_application_setting(notes_create_limit_allowlist_raw: ["#{current_user.username}"])
     end
 
     it_behaves_like 'a Note mutation that creates a Note'

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

@@ -295,7 +295,7 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
       end
 
       it 'allows user in allow-list to create notes' do
-        stub_config_setting(notes_rate_limit_users_allowlist: "#{user.username}, username1, username2")
+        stub_application_setting(notes_create_limit_allowlist_raw: ["#{user.username}"])
         subject
 
         expect(response).to have_gitlab_http_status(:created)