Merge branch...

Merge branch '335300-rate-limit-for-unauthenticated-api-requests-1-rename-web-attributes' into 'master'

[1/5] Rename `throttle_unauthenticated_*` attributes in application settings API

See merge request gitlab-org/gitlab!69543

lib/api/entities/application_setting.rb

@@ -27,6 +27,14 @@ module API
 
       expose(*::ApplicationSettingsHelper.external_authorization_service_attributes)
 
+      # Also expose these columns under their new attribute names.
+      #
+      # TODO: Once we rename the columns, we have to swap this around and keep supporting the old names until v5.
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/340031
+      expose :throttle_unauthenticated_enabled, as: :throttle_unauthenticated_web_enabled
+      expose :throttle_unauthenticated_period_in_seconds, as: :throttle_unauthenticated_web_period_in_seconds
+      expose :throttle_unauthenticated_requests_per_period, as: :throttle_unauthenticated_web_requests_per_period
+
       # support legacy names, can be removed in v5
       expose :password_authentication_enabled_for_web, as: :password_authentication_enabled
       expose :password_authentication_enabled_for_web, as: :signin_enabled

lib/api/helpers/settings_helpers.rb

@@ -10,10 +10,18 @@ module API
       end
 
       def self.optional_attributes
-        [*::ApplicationSettingsHelper.visible_attributes,
-         *::ApplicationSettingsHelper.external_authorization_service_attributes,
-         *::ApplicationSettingsHelper.deprecated_attributes,
-         :performance_bar_allowed_group_id].freeze
+        [
+          *::ApplicationSettingsHelper.visible_attributes,
+          *::ApplicationSettingsHelper.external_authorization_service_attributes,
+          *::ApplicationSettingsHelper.deprecated_attributes,
+          :performance_bar_allowed_group_id,
+          # TODO: Once we rename these columns, we can remove them here and add the old
+          # names to `ApplicationSettingsHelper.deprecated_attributes` instead.
+          # https://gitlab.com/gitlab-org/gitlab/-/issues/340031
+          :throttle_unauthenticated_web_enabled,
+          :throttle_unauthenticated_web_period_in_seconds,
+          :throttle_unauthenticated_web_requests_per_period
+        ].freeze
       end
     end
   end

lib/api/settings.rb

@@ -225,6 +225,16 @@ module API
         attrs[:asset_proxy_allowlist] = attrs.delete(:asset_proxy_whitelist)
       end
 
+      # Also accept these attributes under their new names.
+      #
+      # TODO: Once we rename the columns, we have to swap this around and keep supporting the old names until v5.
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/340031
+      %w[enabled period_in_seconds requests_per_period].each do |suffix|
+        old_name = :"throttle_unauthenticated_#{suffix}"
+        new_name = :"throttle_unauthenticated_web_#{suffix}"
+        attrs[old_name] = attrs.delete(new_name) if attrs.has_key?(new_name)
+      end
+
       # since 13.0 it's not possible to disable hashed storage - support can be removed in 14.0
       attrs.delete(:hashed_storage_enabled) if attrs.has_key?(:hashed_storage_enabled)
 

spec/requests/api/settings_spec.rb

@@ -222,6 +222,45 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting do
       expect(json_response['asset_proxy_allowlist']).to eq(['example.com', '*.example.com', 'localhost'])
     end
 
+    it 'supports the deprecated `throttle_unauthenticated_*` attributes' do
+      put api('/application/settings', admin), params: {
+        throttle_unauthenticated_enabled: true,
+        throttle_unauthenticated_period_in_seconds: 123,
+        throttle_unauthenticated_requests_per_period: 456
+      }
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response).to include(
+        'throttle_unauthenticated_enabled' => true,
+        'throttle_unauthenticated_period_in_seconds' => 123,
+        'throttle_unauthenticated_requests_per_period' => 456,
+        'throttle_unauthenticated_web_enabled' => true,
+        'throttle_unauthenticated_web_period_in_seconds' => 123,
+        'throttle_unauthenticated_web_requests_per_period' => 456
+      )
+    end
+
+    it 'prefers the new `throttle_unauthenticated_web_*` attributes' do
+      put api('/application/settings', admin), params: {
+        throttle_unauthenticated_enabled: false,
+        throttle_unauthenticated_period_in_seconds: 0,
+        throttle_unauthenticated_requests_per_period: 0,
+        throttle_unauthenticated_web_enabled: true,
+        throttle_unauthenticated_web_period_in_seconds: 123,
+        throttle_unauthenticated_web_requests_per_period: 456
+      }
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response).to include(
+        'throttle_unauthenticated_enabled' => true,
+        'throttle_unauthenticated_period_in_seconds' => 123,
+        'throttle_unauthenticated_requests_per_period' => 456,
+        'throttle_unauthenticated_web_enabled' => true,
+        'throttle_unauthenticated_web_period_in_seconds' => 123,
+        'throttle_unauthenticated_web_requests_per_period' => 456
+      )
+    end
+
     it 'disables ability to switch to legacy storage' do
       put api("/application/settings", admin),
           params: { hashed_storage_enabled: false }

spec/services/application_settings/update_service_spec.rb

@@ -336,6 +336,28 @@ RSpec.describe ApplicationSettings::UpdateService do
     end
   end
 
+  context 'when general rate limits are passed' do
+    let(:params) do
+      {
+        throttle_authenticated_api_enabled: true,
+        throttle_authenticated_api_period_in_seconds: 10,
+        throttle_authenticated_api_requests_per_period: 20,
+        throttle_authenticated_web_enabled: true,
+        throttle_authenticated_web_period_in_seconds: 30,
+        throttle_authenticated_web_requests_per_period: 40,
+        throttle_unauthenticated_enabled: true,
+        throttle_unauthenticated_period_in_seconds: 50,
+        throttle_unauthenticated_requests_per_period: 60
+      }
+    end
+
+    it 'updates general throttle settings' do
+      subject.execute
+
+      expect(application_settings.reload).to have_attributes(params)
+    end
+  end
+
   context 'when package registry rate limits are passed' do
     let(:params) do
       {