Commit 193ea959 authored by Eugenia Grieff's avatar Eugenia Grieff Committed by Alex Kalderimis

Add users allowlist to ApplicationRateLimiter

- Add env variable for usernames that will be
excluded from the notes rate limit
- Add specs
- Add documentation
parent e3121749
...@@ -94,8 +94,7 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -94,8 +94,7 @@ class Projects::NotesController < Projects::ApplicationController
def create_rate_limit def create_rate_limit
key = :notes_create key = :notes_create
return unless rate_limiter.throttled?(key, scope: [current_user], users_allowlist: rate_limit_users_allowlist)
return unless rate_limiter.throttled?(key, scope: [current_user])
rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user) 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 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 ...@@ -104,4 +103,8 @@ class Projects::NotesController < Projects::ApplicationController
def rate_limiter def rate_limiter
::Gitlab::ApplicationRateLimiter ::Gitlab::ApplicationRateLimiter
end end
def rate_limit_users_allowlist
Gitlab.config.gitlab.notes_rate_limit_users_allowlist.split(', ')
end
end end
...@@ -58,7 +58,8 @@ module Mutations ...@@ -58,7 +58,8 @@ module Mutations
def verify_rate_limit!(current_user) def verify_rate_limit!(current_user)
rate_limiter, key = ::Gitlab::ApplicationRateLimiter, :notes_create rate_limiter, key = ::Gitlab::ApplicationRateLimiter, :notes_create
return unless rate_limiter.throttled?(key, scope: [current_user]) allowlist = Gitlab.config.gitlab.notes_rate_limit_users_allowlist.split(', ')
return unless rate_limiter.throttled?(key, scope: [current_user], users_allowlist: allowlist)
raise Gitlab::Graphql::Errors::ResourceNotAvailable, raise Gitlab::Graphql::Errors::ResourceNotAvailable,
'This endpoint has been requested too many times. Try again later.' 'This endpoint has been requested too many times. Try again later.'
......
---
title: Add an allowlist to excluse users from the rate limit on notes creation
merge_request: 53866
author:
type: added
...@@ -214,6 +214,7 @@ Settings.gitlab['no_todos_messages'] ||= YAML.load_file(Rails.root.join('config' ...@@ -214,6 +214,7 @@ 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['impersonation_enabled'] ||= true if Settings.gitlab['impersonation_enabled'].nil?
Settings.gitlab['usage_ping_enabled'] = true if Settings.gitlab['usage_ping_enabled'].nil? Settings.gitlab['usage_ping_enabled'] = true if Settings.gitlab['usage_ping_enabled'].nil?
Settings.gitlab['max_request_duration_seconds'] ||= 57 Settings.gitlab['max_request_duration_seconds'] ||= 57
Settings.gitlab['notes_rate_limit_users_allowlist'] ||= ENV['NOTES_RATE_LIMIT_USERS_ALLOWLIST'] || ""
Gitlab.ee do Gitlab.ee do
Settings.gitlab['mirror_max_delay'] ||= 300 Settings.gitlab['mirror_max_delay'] ||= 300
......
...@@ -33,6 +33,7 @@ You can use the following environment variables to override certain values: ...@@ -33,6 +33,7 @@ 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). | | `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`. | | `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`). | | `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 ## Complete database variables
......
...@@ -3,8 +3,8 @@ ...@@ -3,8 +3,8 @@
module API module API
module Helpers module Helpers
module RateLimiter module RateLimiter
def check_rate_limit!(key, scope) def check_rate_limit!(key, scope, users_allowlist = nil)
if rate_limiter.throttled?(key, scope: scope) if rate_limiter.throttled?(key, scope: scope, users_allowlist: users_allowlist)
log_request(key) log_request(key)
render_exceeded_limit_error! render_exceeded_limit_error!
end end
......
...@@ -73,7 +73,8 @@ module API ...@@ -73,7 +73,8 @@ module API
optional :created_at, type: String, desc: 'The creation date of the note' optional :created_at, type: String, desc: 'The creation date of the note'
end end
post ":id/#{noteables_str}/:noteable_id/notes", feature_category: feature_category do post ":id/#{noteables_str}/:noteable_id/notes", feature_category: feature_category do
check_rate_limit! :notes_create, [current_user] allowlist = Gitlab.config.gitlab.notes_rate_limit_users_allowlist.split(', ')
check_rate_limit! :notes_create, [current_user], allowlist
noteable = find_noteable(noteable_type, params[:noteable_id]) noteable = find_noteable(noteable_type, params[:noteable_id])
opts = { opts = {
......
...@@ -47,15 +47,17 @@ module Gitlab ...@@ -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 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 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 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 # @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] 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 && threshold_value > 0 &&
increment(key, scope, interval) > threshold_value increment(key, options[:scope], options[:interval]) > threshold_value
end end
# Increments the given cache key and increments the value by 1 with the # Increments the given cache key and increments the value by 1 with the
...@@ -141,6 +143,15 @@ module Gitlab ...@@ -141,6 +143,15 @@ module Gitlab
def application_settings def application_settings
Gitlab::CurrentSettings.current_application_settings Gitlab::CurrentSettings.current_application_settings
end 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].map(&:downcase))
end
end end
end end
end end
...@@ -730,11 +730,11 @@ RSpec.describe Projects::NotesController do ...@@ -730,11 +730,11 @@ RSpec.describe Projects::NotesController do
context 'when the endpoint receives requests above the limit' do context 'when the endpoint receives requests above the limit' do
before do before do
stub_application_setting(notes_create_limit: 5) stub_application_setting(notes_create_limit: 3)
end end
it 'prevents from creating more notes', :request_store do it 'prevents from creating more notes', :request_store do
5.times { create! } 3.times { create! }
expect { create! } expect { create! }
.to change { Gitlab::GitalyClient.get_request_count }.by(0) .to change { Gitlab::GitalyClient.get_request_count }.by(0)
...@@ -760,7 +760,15 @@ RSpec.describe Projects::NotesController do ...@@ -760,7 +760,15 @@ RSpec.describe Projects::NotesController do
project.add_developer(user) project.add_developer(user)
sign_in(user) sign_in(user)
6.times { create! } 4.times { create! }
end
it 'allows user in allow-list to create notes' do
stub_config_setting(notes_rate_limit_users_allowlist: "#{user.username}")
3.times { create! }
create!
expect(response).to have_gitlab_http_status(:found)
end end
end end
end end
......
...@@ -74,4 +74,12 @@ RSpec.shared_examples 'a Note mutation when there are rate limit validation erro ...@@ -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 Note mutation that does not create a Note'
it_behaves_like 'a mutation that returns top-level errors', it_behaves_like 'a mutation that returns top-level errors',
errors: ['This endpoint has been requested too many times. Try again later.'] errors: ['This endpoint has been requested too many times. Try again later.']
context 'when the user is in the allowlist' do
before do
stub_config_setting(notes_rate_limit_users_allowlist: "#{current_user.username}")
end
it_behaves_like 'a Note mutation that creates a Note'
end
end end
...@@ -127,6 +127,12 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name| ...@@ -127,6 +127,12 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
end end
describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/notes" do 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 it "creates a new note" do
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params: { body: 'hi!' } 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| ...@@ -277,15 +283,25 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
context 'when request exceeds the rate limit' do context 'when request exceeds the rate limit' do
before 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 end
it 'prevents users from creating more notes' do it 'prevents user from creating more notes' do
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params: { body: 'hi!' } subject
expect(response).to have_gitlab_http_status(:too_many_requests) 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.') expect(json_response['message']['error']).to eq('This endpoint has been requested too many times. Try again later.')
end end
it 'allows user in allow-list to create notes' do
stub_config_setting(notes_rate_limit_users_allowlist: "#{user.username}, username1, username2")
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
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment