From 18f63149c87f8dac86369a4d8abb60eda0f60eec Mon Sep 17 00:00:00 2001
From: Abdul Wadood <awadood@gitlab.com>
Date: Mon, 15 Jan 2024 11:40:00 +0000
Subject: [PATCH] Make delete members API rate limit configurable
In https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118296, we
rate limited the [delete member API](https://docs.gitlab.com/ee/api/members.html#remove-a-member-from-a-group-or-project) but the limit for too low for some self-managed users.
So here we're making it configurable.
Changelog: added
---
app/helpers/application_settings_helper.rb | 1 +
app/models/application_setting.rb | 6 ++++
.../application_setting_implementation.rb | 1 +
.../application_setting_rate_limits.json | 13 ++++++++
.../_members_api_limits.html.haml | 21 ++++++++++++
.../application_settings/network.html.haml | 2 ++
...add_rate_limits_to_application_settings.rb | 10 ++++++
db/schema_migrations/20240110085226 | 1 +
db/structure.sql | 1 +
.../settings/rate_limit_on_members_api.md | 33 +++++++++++++++++++
lib/api/members.rb | 2 +-
lib/gitlab/application_rate_limiter.rb | 2 +-
locale/gitlab.pot | 9 +++++
.../application_settings_helper_spec.rb | 1 +
spec/models/application_setting_spec.rb | 6 +++-
spec/requests/api/members_spec.rb | 2 +-
.../network.html.haml_spec.rb | 8 +++++
17 files changed, 115 insertions(+), 4 deletions(-)
create mode 100644 app/validators/json_schemas/application_setting_rate_limits.json
create mode 100644 app/views/admin/application_settings/_members_api_limits.html.haml
create mode 100644 db/migrate/20240110085226_add_rate_limits_to_application_settings.rb
create mode 100644 db/schema_migrations/20240110085226
create mode 100644 doc/administration/settings/rate_limit_on_members_api.md
diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb
index b9a590472387..1affdd8f4339 100644
--- a/app/helpers/application_settings_helper.rb
+++ b/app/helpers/application_settings_helper.rb
@@ -450,6 +450,7 @@ def visible_attributes
:issues_create_limit,
:notes_create_limit,
:notes_create_limit_allowlist_raw,
+ :members_delete_limit,
:raw_blob_request_limit,
:project_import_limit,
:project_export_limit,
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 71dc5521a4db..35d4722b7111 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -579,6 +579,7 @@ def self.kroki_formats_attributes
:max_import_size,
:max_pages_custom_domains_per_project,
:max_terraform_state_size_bytes,
+ :members_delete_limit,
:notes_create_limit,
:package_registry_cleanup_policies_worker_capacity,
:packages_cleanup_package_file_worker_capacity,
@@ -594,6 +595,11 @@ def self.kroki_formats_attributes
:users_get_by_id_limit
end
+ jsonb_accessor :rate_limits,
+ members_delete_limit: [:integer, { default: 60 }]
+
+ validates :rate_limits, json_schema: { filename: "application_setting_rate_limits" }
+
validates :search_rate_limit_allowlist,
length: { maximum: 100, message: N_('is too long (maximum is 100 entries)') },
allow_nil: false
diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb
index e18476bbae6a..d1899b18a4f1 100644
--- a/app/models/application_setting_implementation.rb
+++ b/app/models/application_setting_implementation.rb
@@ -137,6 +137,7 @@ def defaults # rubocop:disable Metrics/AbcSize
mirror_available: true,
notes_create_limit: 300,
notes_create_limit_allowlist: [],
+ members_delete_limit: 60,
notify_on_unknown_sign_in: true,
outbound_local_requests_whitelist: [],
password_authentication_enabled_for_git: true,
diff --git a/app/validators/json_schemas/application_setting_rate_limits.json b/app/validators/json_schemas/application_setting_rate_limits.json
new file mode 100644
index 000000000000..e74295291dfd
--- /dev/null
+++ b/app/validators/json_schemas/application_setting_rate_limits.json
@@ -0,0 +1,13 @@
+{
+ "$schema": "http://json-schema.org/draft-07/schema#",
+ "description": "Application rate limits",
+ "type": "object",
+ "additionalProperties": false,
+ "properties": {
+ "members_delete_limit": {
+ "type": "integer",
+ "minimum": 0,
+ "description": "Number of project or group members a user can delete per minute."
+ }
+ }
+}
diff --git a/app/views/admin/application_settings/_members_api_limits.html.haml b/app/views/admin/application_settings/_members_api_limits.html.haml
new file mode 100644
index 000000000000..3065c62b7e24
--- /dev/null
+++ b/app/views/admin/application_settings/_members_api_limits.html.haml
@@ -0,0 +1,21 @@
+%section.settings.as-members-api-limits.no-animate#js-members-api-limits-settings{ class: ('expanded' if expanded_by_default?) }
+ .settings-header
+ %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only
+ = _('Members API rate limit')
+ = render Pajamas::ButtonComponent.new(button_options: { class: 'js-settings-toggle' }) do
+ = expanded_by_default? ? _('Collapse') : _('Expand')
+ %p.gl-text-secondary
+ = _('Limit the number of project or group members a user can delete per minute through API requests.')
+ = link_to _('Learn more.'), help_page_path('administration/settings/rate_limit_on_members_api'), target: '_blank', rel: 'noopener noreferrer'
+ .settings-content
+ = gitlab_ui_form_for @application_setting, url: network_admin_application_settings_path(anchor: 'js-members-api-limits-settings'), html: { class: 'fieldset-form' } do |f|
+ = form_errors(@application_setting)
+
+ %fieldset
+ .form-group
+ = f.label :members_delete_limit, _('Maximum requests per minute per group / project'), class: 'label-bold'
+ = f.number_field :members_delete_limit, min: 0, class: 'form-control gl-form-input'
+ .form-text.gl-text-gray-600
+ = _("Set to 0 to disable the limit.")
+
+ = f.submit _('Save changes'), pajamas_button: true
diff --git a/app/views/admin/application_settings/network.html.haml b/app/views/admin/application_settings/network.html.haml
index ae5f7a5cec3e..c2f19c11b4e2 100644
--- a/app/views/admin/application_settings/network.html.haml
+++ b/app/views/admin/application_settings/network.html.haml
@@ -151,6 +151,8 @@
= render 'projects_api_limits'
+= render 'members_api_limits'
+
%section.settings.as-import-export-limits.no-animate#js-import-export-limits-settings{ class: ('expanded' if expanded_by_default?) }
.settings-header
%h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only
diff --git a/db/migrate/20240110085226_add_rate_limits_to_application_settings.rb b/db/migrate/20240110085226_add_rate_limits_to_application_settings.rb
new file mode 100644
index 000000000000..2560977f9791
--- /dev/null
+++ b/db/migrate/20240110085226_add_rate_limits_to_application_settings.rb
@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+class AddRateLimitsToApplicationSettings < Gitlab::Database::Migration[2.2]
+ milestone '16.9'
+ enable_lock_retries!
+
+ def change
+ add_column :application_settings, :rate_limits, :jsonb, default: {}, null: false
+ end
+end
diff --git a/db/schema_migrations/20240110085226 b/db/schema_migrations/20240110085226
new file mode 100644
index 000000000000..35e3cc7237ac
--- /dev/null
+++ b/db/schema_migrations/20240110085226
@@ -0,0 +1 @@
+9c9eb37365dae73fb68000d18675a280e063711eaed8f96f64724bff20326957
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index b7368d8b2898..793fa64b53e2 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -12633,6 +12633,7 @@ CREATE TABLE application_settings (
lock_toggle_security_policy_custom_ci boolean DEFAULT false NOT NULL,
toggle_security_policies_policy_scope boolean DEFAULT false NOT NULL,
lock_toggle_security_policies_policy_scope boolean DEFAULT false NOT NULL,
+ rate_limits jsonb DEFAULT '{}'::jsonb 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_container_registry_pre_import_tags_rate_positive CHECK ((container_registry_pre_import_tags_rate >= (0)::numeric)),
CONSTRAINT app_settings_dep_proxy_ttl_policies_worker_capacity_positive CHECK ((dependency_proxy_ttl_group_policy_worker_capacity >= 0)),
diff --git a/doc/administration/settings/rate_limit_on_members_api.md b/doc/administration/settings/rate_limit_on_members_api.md
new file mode 100644
index 000000000000..3e8868adc919
--- /dev/null
+++ b/doc/administration/settings/rate_limit_on_members_api.md
@@ -0,0 +1,33 @@
+---
+stage: Data Stores
+group: Tenant Scale
+info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://handbook.gitlab.com/handbook/product/ux/technical-writing/#assignments
+---
+
+# Rate limit on Members API **(FREE SELF)**
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/140633) in GitLab 16.9.
+
+You can configure the rate limit per group (or project) per user to the
+[delete members API](../../api/members.md#remove-a-member-from-a-group-or-project).
+
+To change the rate limit:
+
+1. On the left sidebar, at the bottom, select **Admin Area**.
+1. Select **Settings > Network**.
+1. Expand **Members API rate limit**.
+1. In the **Maximum requests per minute per group / project** text box, enter the new value.
+1. Select **Save changes**.
+
+The rate limit:
+
+- Applies per group or project per user.
+- Can be set to 0 to disable rate limiting.
+
+The default value of the rate limit is `60`.
+
+Requests over the rate limit are logged into the `auth.log` file.
+
+For example, if you set a limit of 60, requests sent to the
+[delete members API](../../api/members.md#remove-a-member-from-a-group-or-project) exceeding a rate of 300 per minute
+are blocked. Access to the endpoint is allowed after one minute.
diff --git a/lib/api/members.rb b/lib/api/members.rb
index 56a15c41e1c5..908733d4aa12 100644
--- a/lib/api/members.rb
+++ b/lib/api/members.rb
@@ -176,7 +176,7 @@ class Members < ::API::Base
source = find_source(source_type, params[:id])
member = source_members(source).find_by!(user_id: params[:user_id])
- check_rate_limit!(:member_delete, scope: [source, current_user])
+ check_rate_limit!(:members_delete, scope: [source, current_user])
destroy_conditionally!(member) do
::Members::DestroyService.new(current_user).execute(member, skip_subresources: params[:skip_subresources], unassign_issuables: params[:unassign_issuables])
diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb
index df7f29863049..5a2881e6c966 100644
--- a/lib/gitlab/application_rate_limiter.rb
+++ b/lib/gitlab/application_rate_limiter.rb
@@ -30,7 +30,7 @@ def rate_limits # rubocop:disable Metrics/AbcSize
group_download_export: { threshold: -> { application_settings.group_download_export_limit }, interval: 1.minute },
group_import: { threshold: -> { application_settings.group_import_limit }, interval: 1.minute },
group_testing_hook: { threshold: 5, interval: 1.minute },
- member_delete: { threshold: 60, interval: 1.minute },
+ members_delete: { threshold: -> { application_settings.members_delete_limit }, interval: 1.minute },
profile_add_new_email: { threshold: 5, interval: 1.minute },
web_hook_calls: { interval: 1.minute },
web_hook_calls_mid: { interval: 1.minute },
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index fab424dc1590..8db2707aa99a 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -29013,6 +29013,9 @@ msgstr ""
msgid "Limit the number of pipeline creation requests per minute. This limit includes pipelines created through the UI, the API, and by background processing."
msgstr ""
+msgid "Limit the number of project or group members a user can delete per minute through API requests."
+msgstr ""
+
msgid "Limit the size of Sidekiq jobs stored in Redis."
msgstr ""
@@ -29882,6 +29885,9 @@ msgstr ""
msgid "Maximum requests per minute"
msgstr ""
+msgid "Maximum requests per minute per group / project"
+msgstr ""
+
msgid "Maximum running slices"
msgstr ""
@@ -30071,6 +30077,9 @@ msgstr ""
msgid "Members"
msgstr ""
+msgid "Members API rate limit"
+msgstr ""
+
msgid "Members can be added by project %{i_open}Maintainers%{i_close} or %{i_open}Owners%{i_close}"
msgstr ""
diff --git a/spec/helpers/application_settings_helper_spec.rb b/spec/helpers/application_settings_helper_spec.rb
index 5dc75a60a6e3..b378437c4075 100644
--- a/spec/helpers/application_settings_helper_spec.rb
+++ b/spec/helpers/application_settings_helper_spec.rb
@@ -65,6 +65,7 @@
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 search_rate_limit search_rate_limit_unauthenticated
+ members_delete_limit
])
end
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 9b2c14314df7..b4003469ebb6 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -28,6 +28,7 @@
it { expect(setting.decompress_archive_file_timeout).to eq(210) }
it { expect(setting.bulk_import_concurrent_pipeline_batch_limit).to eq(25) }
it { expect(setting.allow_project_creation_for_guest_and_below).to eq(true) }
+ it { expect(setting.members_delete_limit).to eq(60) }
end
describe 'validations' do
@@ -58,6 +59,8 @@
}
end
+ it { expect(described_class).to validate_jsonb_schema(['application_setting_rate_limits']) }
+
it { is_expected.to allow_value(nil).for(:home_page_url) }
it { is_expected.to allow_value(http).for(:home_page_url) }
it { is_expected.to allow_value(https).for(:home_page_url) }
@@ -225,6 +228,8 @@ def many_usernames(num = 100)
max_import_size
max_pages_custom_domains_per_project
max_terraform_state_size_bytes
+ members_delete_limit
+ notes_create_limit
package_registry_cleanup_policies_worker_capacity
packages_cleanup_package_file_worker_capacity
pipeline_limit_per_project_user_sha
@@ -237,7 +242,6 @@ def many_usernames(num = 100)
sidekiq_job_limiter_limit_bytes
terminal_max_session_time
users_get_by_id_limit
- notes_create_limit
]
end
diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb
index feb24a4e73f6..7fc58140fb6b 100644
--- a/spec/requests/api/members_spec.rb
+++ b/spec/requests/api/members_spec.rb
@@ -717,7 +717,7 @@
end.to change { source.members.count }.by(-1)
end
- it_behaves_like 'rate limited endpoint', rate_limit_key: :member_delete do
+ it_behaves_like 'rate limited endpoint', rate_limit_key: :members_delete do
let(:current_user) { maintainer }
let(:another_member) { create(:user) }
diff --git a/spec/views/admin/application_settings/network.html.haml_spec.rb b/spec/views/admin/application_settings/network.html.haml_spec.rb
index 989977bac3e0..193ee8a32d5a 100644
--- a/spec/views/admin/application_settings/network.html.haml_spec.rb
+++ b/spec/views/admin/application_settings/network.html.haml_spec.rb
@@ -18,4 +18,12 @@
expect(rendered).to have_field('application_setting_projects_api_rate_limit_unauthenticated')
end
end
+
+ context 'for Members API rate limit' do
+ it 'renders the `members_delete_limit` field' do
+ render
+
+ expect(rendered).to have_field('application_setting_members_delete_limit')
+ end
+ end
end
--
GitLab