Commit e355e29d authored by Magdalena Frankiewicz's avatar Magdalena Frankiewicz Committed by Bob Van Landuyt

Add rate limiting to users get by id endpoint

parent 29d69b33
......@@ -8,6 +8,7 @@
# See lib/api/helpers/rate_limiter.rb for API version
module CheckRateLimit
def check_rate_limit!(key, scope:, redirect_back: false, **options)
return if bypass_header_set?
return unless rate_limiter.throttled?(key, scope: scope, **options)
rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user)
......@@ -28,4 +29,8 @@ module CheckRateLimit
def rate_limiter
::Gitlab::ApplicationRateLimiter
end
def bypass_header_set?
::Gitlab::Throttle.bypass_header.present? && request.get_header(Gitlab::Throttle.bypass_header) == '1'
end
end
---
name: rate_limit_user_by_id_endpoint
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/73069
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/348796
milestone: '14.6'
type: development
group: group::optimize
default_enabled: false
......@@ -10,6 +10,7 @@ module API
# See app/controllers/concerns/check_rate_limit.rb for Rails controllers version
module RateLimiter
def check_rate_limit!(key, scope:, **options)
return if bypass_header_set?
return unless rate_limiter.throttled?(key, scope: scope, **options)
rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user)
......@@ -24,6 +25,10 @@ module API
def rate_limiter
::Gitlab::ApplicationRateLimiter
end
def bypass_header_set?
::Gitlab::Throttle.bypass_header.present? && request.get_header(Gitlab::Throttle.bypass_header) == '1'
end
end
end
end
......@@ -142,11 +142,15 @@ module API
get ":id", feature_category: :users do
forbidden!('Not authorized!') unless current_user
if Feature.enabled?(:rate_limit_user_by_id_endpoint, type: :development)
check_rate_limit! :users_get_by_id, scope: current_user unless current_user.admin?
end
user = User.find_by(id: params[:id])
not_found!('User') unless user && can?(current_user, :read_user, user)
opts = { with: current_user&.admin? ? Entities::UserDetailsWithAdmin : Entities::User, current_user: current_user }
opts = { with: current_user.admin? ? Entities::UserDetailsWithAdmin : Entities::User, current_user: current_user }
user, opts = with_custom_attributes(user, opts)
present user, opts
......
......@@ -49,6 +49,7 @@ module Gitlab
group_testing_hook: { threshold: 5, interval: 1.minute },
profile_add_new_email: { threshold: 5, interval: 1.minute },
web_hook_calls: { interval: 1.minute },
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 }
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe CheckRateLimit do
let(:key) { :some_key }
let(:scope) { [:some, :scope] }
let(:request) { instance_double('Rack::Request') }
let(:user) { build_stubbed(:user) }
let(:controller_class) do
Class.new do
include CheckRateLimit
attr_reader :request, :current_user
def initialize(request, current_user)
@request = request
@current_user = current_user
end
def redirect_back_or_default(**args)
end
def render(**args)
end
end
end
subject { controller_class.new(request, user) }
before do
allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?)
allow(::Gitlab::ApplicationRateLimiter).to receive(:log_request)
end
describe '#check_rate_limit!' do
it 'calls ApplicationRateLimiter#throttled? with the right arguments' do
expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(key, scope: scope).and_return(false)
expect(subject).not_to receive(:render)
subject.check_rate_limit!(key, scope: scope)
end
it 'renders error and logs request if throttled' do
expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(key, scope: scope).and_return(true)
expect(::Gitlab::ApplicationRateLimiter).to receive(:log_request).with(request, "#{key}_request_limit".to_sym, user)
expect(subject).to receive(:render).with({ plain: _('This endpoint has been requested too many times. Try again later.'), status: :too_many_requests })
subject.check_rate_limit!(key, scope: scope)
end
it 'redirects back if throttled and redirect_back option is set to true' do
expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(key, scope: scope).and_return(true)
expect(::Gitlab::ApplicationRateLimiter).to receive(:log_request).with(request, "#{key}_request_limit".to_sym, user)
expect(subject).not_to receive(:render)
expect(subject).to receive(:redirect_back_or_default).with(options: { alert: _('This endpoint has been requested too many times. Try again later.') })
subject.check_rate_limit!(key, scope: scope, redirect_back: true)
end
context 'when the bypass header is set' do
before do
allow(Gitlab::Throttle).to receive(:bypass_header).and_return('SOME_HEADER')
end
it 'skips rate limit if set to "1"' do
allow(request).to receive(:get_header).with(Gitlab::Throttle.bypass_header).and_return('1')
expect(::Gitlab::ApplicationRateLimiter).not_to receive(:throttled?)
expect(subject).not_to receive(:render)
subject.check_rate_limit!(key, scope: scope)
end
it 'does not skip rate limit if set to something else than "1"' do
allow(request).to receive(:get_header).with(Gitlab::Throttle.bypass_header).and_return('0')
expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?)
subject.check_rate_limit!(key, scope: scope)
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe API::Helpers::RateLimiter do
let(:key) { :some_key }
let(:scope) { [:some, :scope] }
let(:request) { instance_double('Rack::Request') }
let(:user) { build_stubbed(:user) }
let(:api_class) do
Class.new do
include API::Helpers::RateLimiter
attr_reader :request, :current_user
def initialize(request, current_user)
@request = request
@current_user = current_user
end
def render_api_error!(**args)
end
end
end
subject { api_class.new(request, user) }
before do
allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?)
allow(::Gitlab::ApplicationRateLimiter).to receive(:log_request)
end
describe '#check_rate_limit!' do
it 'calls ApplicationRateLimiter#throttled? with the right arguments' do
expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(key, scope: scope).and_return(false)
expect(subject).not_to receive(:render_api_error!)
subject.check_rate_limit!(key, scope: scope)
end
it 'renders api error and logs request if throttled' do
expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(key, scope: scope).and_return(true)
expect(::Gitlab::ApplicationRateLimiter).to receive(:log_request).with(request, "#{key}_request_limit".to_sym, user)
expect(subject).to receive(:render_api_error!).with({ error: _('This endpoint has been requested too many times. Try again later.') }, 429)
subject.check_rate_limit!(key, scope: scope)
end
context 'when the bypass header is set' do
before do
allow(Gitlab::Throttle).to receive(:bypass_header).and_return('SOME_HEADER')
end
it 'skips rate limit if set to "1"' do
allow(request).to receive(:get_header).with(Gitlab::Throttle.bypass_header).and_return('1')
expect(::Gitlab::ApplicationRateLimiter).not_to receive(:throttled?)
expect(subject).not_to receive(:render_api_error!)
subject.check_rate_limit!(key, scope: scope)
end
it 'does not skip rate limit if set to something else than "1"' do
allow(request).to receive(:get_header).with(Gitlab::Throttle.bypass_header).and_return('0')
expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?)
subject.check_rate_limit!(key, scope: scope)
end
end
end
end
......@@ -498,6 +498,10 @@ RSpec.describe API::Users do
describe "GET /users/:id" do
let_it_be(:user2, reload: true) { create(:user, username: 'another_user') }
before do
allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(:users_get_by_id, scope: user).and_return(false)
end
it "returns a user by id" do
get api("/users/#{user.id}", user)
......@@ -593,6 +597,55 @@ RSpec.describe API::Users do
expect(json_response).not_to have_key('sign_in_count')
end
context 'when the rate limit is not exceeded' do
it 'returns a success status' do
expect(Gitlab::ApplicationRateLimiter)
.to receive(:throttled?).with(:users_get_by_id, scope: user)
.and_return(false)
get api("/users/#{user.id}", user)
expect(response).to have_gitlab_http_status(:ok)
end
end
context 'when the rate limit is exceeded' do
context 'when feature flag is enabled' do
it 'returns "too many requests" status' do
expect(Gitlab::ApplicationRateLimiter)
.to receive(:throttled?).with(:users_get_by_id, scope: user)
.and_return(true)
get api("/users/#{user.id}", user)
expect(response).to have_gitlab_http_status(:too_many_requests)
end
it 'still allows admin users' do
expect(Gitlab::ApplicationRateLimiter)
.not_to receive(:throttled?)
get api("/users/#{user.id}", admin)
expect(response).to have_gitlab_http_status(:ok)
end
end
context 'when feature flag is disabled' do
before do
stub_feature_flags(rate_limit_user_by_id_endpoint: false)
end
it 'does not throttle the request' do
expect(Gitlab::ApplicationRateLimiter).not_to receive(:throttled?)
get api("/users/#{user.id}", user)
expect(response).to have_gitlab_http_status(:ok)
end
end
end
context 'when job title is present' do
let(:job_title) { 'Fullstack Engineer' }
......
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