Merge branch 'configurable-rate-limit-get-users-by-id' into 'master'

Make rate limiting of /users/:id configurable

See merge request gitlab-org/gitlab!78364

app/helpers/application_settings_helper.rb

@@ -423,7 +423,9 @@ module ApplicationSettingsHelper
       :sidekiq_job_limiter_compression_threshold_bytes,
       :sidekiq_job_limiter_limit_bytes,
       :suggest_pipeline_enabled,
-      :user_email_lookup_limit
+      :user_email_lookup_limit,
+      :users_get_by_id_limit,
+      :users_get_by_id_limit_allowlist_raw
     ].tap do |settings|
       settings << :deactivate_dormant_users unless Gitlab.com?
     end

app/models/application_setting.rb

@@ -563,6 +563,12 @@ class ApplicationSetting < ApplicationRecord
     presence: true, length: { maximum: 255 },
     if: :sentry_enabled?
 
+  validates :users_get_by_id_limit,
+    numericality: { only_integer: true, greater_than_or_equal_to: 0 }
+  validates :users_get_by_id_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

@@ -231,7 +231,9 @@ module ApplicationSettingImplementation
         rate_limiting_response_text: nil,
         whats_new_variant: 0,
         user_deactivation_emails_enabled: true,
-        user_email_lookup_limit: 60
+        user_email_lookup_limit: 60,
+        users_get_by_id_limit: 300,
+        users_get_by_id_limit_allowlist: []
       }
     end
 
@@ -334,6 +336,14 @@ module ApplicationSettingImplementation
     self.notes_create_limit_allowlist = strings_to_array(values).map(&:downcase)
   end
 
+  def users_get_by_id_limit_allowlist_raw
+    array_to_string(self.users_get_by_id_limit_allowlist)
+  end
+
+  def users_get_by_id_limit_allowlist_raw=(values)
+    self.users_get_by_id_limit_allowlist = strings_to_array(values).map(&:downcase)
+  end
+
   def asset_proxy_whitelist=(values)
     values = strings_to_array(values) if values.is_a?(String)
 

app/models/instance_configuration.rb

@@ -118,7 +118,12 @@ class InstanceConfiguration
       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),
-      user_email_lookup: application_setting_limit_per_minute(:user_email_lookup_limit)
+      user_email_lookup: application_setting_limit_per_minute(:user_email_lookup_limit),
+      users_get_by_id: {
+        enabled: application_settings[:users_get_by_id_limit] > 0,
+        requests_per_period: application_settings[:users_get_by_id_limit],
+        period_in_seconds: 10.minutes
+      }
     }
   end
 

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

@@ -9,7 +9,7 @@
       = f.label :notes_create_limit_allowlist, _('Users to exclude from the rate limit'), class: 'label-bold'
       = f.text_area :notes_create_limit_allowlist_raw, placeholder: 'username1, username2', class: 'form-control gl-form-input', rows: 5
       .form-text.text-muted
-        = _('Comma-separated list of users allowed to exceed the rate limit.')
+        = _('List of users allowed to exceed the rate limit.')
 
 
   = f.submit _('Save changes'), class: "gl-button btn btn-confirm", data: { qa_selector: 'save_changes_button' }

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

+= form_for @application_setting, url: network_admin_application_settings_path(anchor: 'js-users-api-limits-settings'), html: { class: 'fieldset-form' } do |f|
+  = form_errors(@application_setting)
+
+  %fieldset
+    .form-group
+      = f.label :users_get_by_id_limit, _('Maximum requests per 10 minutes per user'), class: 'label-bold'
+      = f.number_field :users_get_by_id_limit, class: 'form-control gl-form-input'
+    .form-group
+      = f.label :users_get_by_id_limit_allowlist_raw, _('Users to exclude from the rate limit'), class: 'label-bold'
+      = f.text_area :users_get_by_id_limit_allowlist_raw, placeholder: 'username1, username2', class: 'form-control gl-form-input', rows: 5
+      .form-text.text-muted
+        = _('List of users allowed to exceed the rate limit.')
+
+  = f.submit _('Save changes'), class: "gl-button btn btn-confirm", data: { qa_selector: 'save_changes_button' }

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

@@ -122,6 +122,18 @@
   .settings-content
     = render 'note_limits'
 
+%section.settings.as-users-api-limits.no-animate#js-users-api-limits-settings{ class: ('expanded' if expanded_by_default?) }
+  .settings-header
+    %h4
+      = _('Users API rate limit')
+    %button.btn.gl-button.btn-default.js-settings-toggle{ type: 'button' }
+      = expanded_by_default? ? _('Collapse') : _('Expand')
+    %p
+      = _('Set the per-user rate limit for getting a user by ID via the API.')
+      = link_to _('Learn more.'), help_page_path('user/admin_area/settings/rate_limit_on_users_api.md'), target: '_blank', rel: 'noopener noreferrer'
+  .settings-content
+    = render 'users_api_limits'
+
 %section.settings.as-import-export-limits.no-animate#js-import-export-limits-settings{ class: ('expanded' if expanded_by_default?) }
   .settings-header
     %h4

db/migrate/20220207083129_add_users_get_by_id_limit_to_application_setting.rb

+# frozen_string_literal: true
+
+class AddUsersGetByIdLimitToApplicationSetting < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def up
+    add_column :application_settings, :users_get_by_id_limit, :integer, null: false, default: 300
+    add_column :application_settings, :users_get_by_id_limit_allowlist, :text, array: true, limit: 255, null: false, default: []
+  end
+
+  def down
+    remove_column :application_settings, :users_get_by_id_limit
+    remove_column :application_settings, :users_get_by_id_limit_allowlist
+  end
+end

db/schema_migrations/20220207083129

+01cc0139097235991fa2caf8b780ccd1c3ce580647197418424ade83ce9be77e
\ No newline at end of file

db/structure.sql

@@ -10620,6 +10620,8 @@ CREATE TABLE application_settings (
     project_runner_token_expiration_interval integer,
     ecdsa_sk_key_restriction integer DEFAULT 0 NOT NULL,
     ed25519_sk_key_restriction integer DEFAULT 0 NOT NULL,
+    users_get_by_id_limit integer DEFAULT 300 NOT NULL,
+    users_get_by_id_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_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)),

doc/user/admin_area/settings/index.md

@@ -137,6 +137,7 @@ The **Network** settings contain:
 - [Incident Management Limits](../../../operations/incident_management/index.md) - Limit the
   number of inbound alerts that can be sent to a project.
 - [Notes creation limit](rate_limit_on_notes_creation.md) - Set a rate limit on the note creation requests.
+- [Get single user limit](rate_limit_on_users_api.md) - Set a rate limit on users API endpoint to get a user by ID.
 
 ### Preferences
 

doc/user/admin_area/settings/rate_limit_on_users_api.md

+---
+type: reference
+stage: Manage
+group: Authentication & Authorization
+info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments
+---
+
+# Rate limits on Users API **(FREE SELF)**
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78364) in GitLab 14.8.
+
+You can configure the per-user rate limit for requests to the users API endpoint to get a user by ID.
+
+To change getting a single user rate limit:
+
+1. On the top bar, select **Menu > Admin**.
+1. On the left sidebar, select **Settings > Network**.
+1. Expand **Users API rate limit**.
+1. In the **Maximum requests per 10 minutes** text box, enter the new value.
+1. Optional. In the **Users to exclude from the rate limit** box, list users allowed to exceed the limit.
+1. Select **Save changes**.
+
+This limit is:
+
+- Applied independently per user.
+- Not applied per IP address.
+
+The default value is `300`.
+
+Requests over the rate limit are logged into the `auth.log` file.
+
+For example, if you set a limit of 300, requests to the `GET /users/:id` API endpoint
+exceeding a rate of 300 per 10 minutes are blocked. Access to the endpoint is allowed after ten minutes have elapsed.

lib/api/settings.rb

@@ -177,6 +177,7 @@ module API
       optional :floc_enabled, type: Grape::API::Boolean, desc: 'Enable FloC (Federated Learning of Cohorts)'
       optional :user_deactivation_emails_enabled, type: Boolean, desc: 'Send emails to users upon account deactivation'
       optional :suggest_pipeline_enabled, type: Boolean, desc: 'Enable pipeline suggestion banner'
+      optional :users_get_by_id_limit, type: Integer, desc: "Maximum number of calls to the /users/:id API per 10 minutes per user. Set to 0 for unlimited requests."
 
       ApplicationSetting::SUPPORTED_KEY_TYPES.each do |type|
         optional :"#{type}_key_restriction",

lib/api/users.rb

@@ -143,7 +143,12 @@ module API
         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?
+          unless current_user.admin?
+            check_rate_limit!(:users_get_by_id,
+              scope: current_user,
+              users_allowlist: Gitlab::CurrentSettings.current_application_settings.users_get_by_id_limit_allowlist
+            )
+          end
         end
 
         user = User.find_by(id: params[:id])

lib/gitlab/application_rate_limiter.rb

@@ -14,7 +14,7 @@ module Gitlab
       # Threshold value can be either an Integer or a Proc
       # in order to not evaluate it's value every time this method is called
       # and only do that when it's needed.
-      def rate_limits
+      def rate_limits # rubocop:disable Metrics/AbcSize
         {
           issues_create:                { threshold: -> { application_settings.issues_create_limit }, interval: 1.minute },
           notes_create:                 { threshold: -> { application_settings.notes_create_limit }, interval: 1.minute },
@@ -32,7 +32,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 },
+          users_get_by_id:              { threshold: -> { application_settings.users_get_by_id_limit }, interval: 10.minutes },
           username_exists:              { threshold: 20, interval: 1.minute },
           user_sign_up:                 { threshold: 20, interval: 1.minute },
           profile_resend_email_confirmation:  { threshold: 5, interval: 1.minute },

locale/gitlab.pot

@@ -8751,9 +8751,6 @@ msgstr ""
 msgid "Comma-separated list of email addresses."
 msgstr ""
 
-msgid "Comma-separated list of users allowed to exceed the rate limit."
-msgstr ""
-
 msgid "Comma-separated, e.g. '1.1.1.1, 2.2.2.0/24'"
 msgstr ""
 
@@ -21734,6 +21731,9 @@ msgstr ""
 msgid "List of all merge commits"
 msgstr ""
 
+msgid "List of users allowed to exceed the rate limit."
+msgstr ""
+
 msgid "List options"
 msgstr ""
 
@@ -22361,6 +22361,9 @@ msgstr ""
 msgid "Maximum push size (MB)"
 msgstr ""
 
+msgid "Maximum requests per 10 minutes per user"
+msgstr ""
+
 msgid "Maximum requests per minute"
 msgstr ""
 
@@ -33140,6 +33143,9 @@ msgstr ""
 msgid "Set the milestone to %{milestone_reference}."
 msgstr ""
 
+msgid "Set the per-user rate limit for getting a user by ID via the API."
+msgstr ""
+
 msgid "Set the per-user rate limit for notes created by web or API requests."
 msgstr ""
 
@@ -39585,6 +39591,9 @@ msgstr ""
 msgid "Users"
 msgstr ""
 
+msgid "Users API rate limit"
+msgstr ""
+
 msgid "Users can launch a development environment from a GitLab browser tab when the %{linkStart}Gitpod%{linkEnd} integration is enabled."
 msgstr ""
 

spec/features/admin/admin_settings_spec.rb

@@ -631,6 +631,20 @@ RSpec.describe 'Admin updates settings' do
         expect(current_settings.issues_create_limit).to eq(0)
       end
 
+      it 'changes Users API rate limits settings' do
+        visit network_admin_application_settings_path
+
+        page.within('.as-users-api-limits') do
+          fill_in 'Maximum requests per 10 minutes per user', with: 0
+          fill_in 'Users to exclude from the rate limit', with: 'someone, someone_else'
+          click_button 'Save changes'
+        end
+
+        expect(page).to have_content "Application settings saved successfully"
+        expect(current_settings.users_get_by_id_limit).to eq(0)
+        expect(current_settings.users_get_by_id_limit_allowlist).to eq(%w[someone someone_else])
+      end
+
       shared_examples 'regular throttle rate limit settings' do
         it 'changes rate limit settings' do
           visit network_admin_application_settings_path

spec/helpers/application_settings_helper_spec.rb

@@ -46,6 +46,15 @@ RSpec.describe ApplicationSettingsHelper do
       expect(helper.visible_attributes).to include(:deactivate_dormant_users)
     end
 
+    it 'contains rate limit parameters' do
+      expect(helper.visible_attributes).to include(*%i(
+        issues_create_limit notes_create_limit project_export_limit
+        project_download_export_limit project_export_limit project_import_limit
+        raw_blob_request_limit group_export_limit group_download_export_limit
+        group_import_limit users_get_by_id_limit user_email_lookup_limit
+      ))
+    end
+
     context 'when GitLab.com' do
       before do
         allow(Gitlab).to receive(:com?).and_return(true)

spec/models/application_setting_spec.rb

@@ -141,7 +141,7 @@ 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") }
 
-    %i[notes_create_limit user_email_lookup_limit].each do |setting|
+    %i[notes_create_limit user_email_lookup_limit users_get_by_id_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) }
@@ -158,6 +158,11 @@ RSpec.describe ApplicationSetting do
     it { is_expected.not_to allow_value(nil).for(:notes_create_limit_allowlist) }
     it { is_expected.to allow_value([]).for(:notes_create_limit_allowlist) }
 
+    it { is_expected.to allow_value(many_usernames(100)).for(:users_get_by_id_limit_allowlist) }
+    it { is_expected.not_to allow_value(many_usernames(101)).for(:users_get_by_id_limit_allowlist) }
+    it { is_expected.not_to allow_value(nil).for(:users_get_by_id_limit_allowlist) }
+    it { is_expected.to allow_value([]).for(:users_get_by_id_limit_allowlist) }
+
     it { is_expected.to allow_value('all_tiers').for(:whats_new_variant) }
     it { is_expected.to allow_value('current_tier').for(:whats_new_variant) }
     it { is_expected.to allow_value('disabled').for(:whats_new_variant) }

spec/models/instance_configuration_spec.rb

@@ -206,7 +206,8 @@ RSpec.describe InstanceConfiguration do
             group_download_export_limit: 1019,
             group_import_limit: 1020,
             raw_blob_request_limit: 1021,
-            user_email_lookup_limit: 1022
+            user_email_lookup_limit: 1022,
+            users_get_by_id_limit: 1023
           )
         end
 
@@ -230,6 +231,7 @@ RSpec.describe InstanceConfiguration do
           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 })
+          expect(rate_limits[:users_get_by_id]).to eq({ enabled: true, requests_per_period: 1023, period_in_seconds: 600 })
         end
       end
     end

spec/requests/api/settings_spec.rb

@@ -141,7 +141,8 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting do
             personal_access_token_prefix: "GL-",
             user_deactivation_emails_enabled: false,
             admin_mode: true,
-            suggest_pipeline_enabled: false
+            suggest_pipeline_enabled: false,
+            users_get_by_id_limit: 456
           }
 
         expect(response).to have_gitlab_http_status(:ok)
@@ -196,6 +197,7 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting do
         expect(json_response['admin_mode']).to be(true)
         expect(json_response['user_deactivation_emails_enabled']).to be(false)
         expect(json_response['suggest_pipeline_enabled']).to be(false)
+        expect(json_response['users_get_by_id_limit']).to eq(456)
       end
     end
 

spec/requests/api/users_spec.rb

@@ -499,7 +499,8 @@ RSpec.describe API::Users 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)
+      allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?)
+        .with(:users_get_by_id, scope: user, users_allowlist: []).and_return(false)
     end
 
     it "returns a user by id" do
@@ -600,7 +601,7 @@ RSpec.describe API::Users do
     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)
+          .to receive(:throttled?).with(:users_get_by_id, scope: user, users_allowlist: [])
           .and_return(false)
 
         get api("/users/#{user.id}", user)
@@ -613,7 +614,7 @@ RSpec.describe API::Users 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)
+            .to receive(:throttled?).with(:users_get_by_id, scope: user, users_allowlist: [])
             .and_return(true)
 
           get api("/users/#{user.id}", user)
@@ -629,6 +630,24 @@ RSpec.describe API::Users do
 
           expect(response).to have_gitlab_http_status(:ok)
         end
+
+        it 'allows users whose username is in the allowlist' do
+          allowlist = [user.username]
+          current_settings = Gitlab::CurrentSettings.current_application_settings
+
+          # Necessary to ensure the same object is returned on each call
+          allow(Gitlab::CurrentSettings).to receive(:current_application_settings).and_return current_settings
+
+          allow(current_settings).to receive(:users_get_by_id_limit_allowlist).and_return(allowlist)
+
+          expect(Gitlab::ApplicationRateLimiter)
+            .to receive(:throttled?).with(:users_get_by_id, scope: user, users_allowlist: allowlist)
+            .and_call_original
+
+          get api("/users/#{user.id}", user)
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
       end
 
       context 'when feature flag is disabled' do

spec/services/application_settings/update_service_spec.rb

@@ -475,6 +475,24 @@ RSpec.describe ApplicationSettings::UpdateService do
     end
   end
 
+  context 'when users_get_by_id_limit and users_get_by_id_limit_allowlist_raw are passed' do
+    let(:params) do
+      {
+        users_get_by_id_limit: 456,
+        users_get_by_id_limit_allowlist_raw: 'someone, someone_else'
+      }
+    end
+
+    it 'updates users_get_by_id_limit and users_get_by_id_limit_allowlist value' do
+      subject.execute
+
+      application_settings.reload
+
+      expect(application_settings.users_get_by_id_limit).to eq(456)
+      expect(application_settings.users_get_by_id_limit_allowlist).to eq(%w[someone someone_else])
+    end
+  end
+
   context 'when require_admin_approval_after_user_signup changes' do
     context 'when it goes from enabled to disabled' do
       let(:params) { { require_admin_approval_after_user_signup: false } }