Implement rate-limiting for a deprecated API endpoint

GET /api/v4/groups/:id?with_projects=true is deprecated, as it includes
the `projects` member in the returned API response. That's expensive to
calculate and is generally unnecessary - other endpoints let you access
the same information in a more efficient manner.

Limiting requests to deprecated API endpoints is a way to induce users
to switch to the non-deprecated alternatives. We can add more endpoints
over time.

Changelog: added

lib/gitlab/metrics/subscribers/rack_attack.rb

@@ -22,7 +22,8 @@ module Gitlab
           :throttle_authenticated_protected_paths_web,
           :throttle_authenticated_packages_api,
           :throttle_authenticated_git_lfs,
-          :throttle_authenticated_files_api
+          :throttle_authenticated_files_api,
+          :throttle_authenticated_deprecated_api
         ].freeze
 
         PAYLOAD_KEYS = [

lib/gitlab/rack_attack/request.rb

@@ -4,6 +4,7 @@ module Gitlab
   module RackAttack
     module Request
       FILES_PATH_REGEX = %r{^/api/v\d+/projects/[^/]+/repository/files/.+}.freeze
+      GROUP_PATH_REGEX = %r{^/api/v\d+/groups/[^/]+/?$}.freeze
 
       def unauthenticated?
         !(authenticated_user_id([:api, :rss, :ics]) || authenticated_runner_id)
@@ -71,6 +72,7 @@ module Gitlab
         !should_be_skipped? &&
         !throttle_unauthenticated_packages_api? &&
         !throttle_unauthenticated_files_api? &&
+        !throttle_unauthenticated_deprecated_api? &&
         Gitlab::Throttle.settings.throttle_unauthenticated_api_enabled &&
         unauthenticated?
       end
@@ -87,6 +89,7 @@ module Gitlab
         api_request? &&
         !throttle_authenticated_packages_api? &&
         !throttle_authenticated_files_api? &&
+        !throttle_authenticated_deprecated_api? &&
         Gitlab::Throttle.settings.throttle_authenticated_api_enabled
       end
 
@@ -147,6 +150,17 @@ module Gitlab
         Gitlab::Throttle.settings.throttle_authenticated_files_api_enabled
       end
 
+      def throttle_unauthenticated_deprecated_api?
+        deprecated_api_request? &&
+        Gitlab::Throttle.settings.throttle_unauthenticated_deprecated_api_enabled &&
+        unauthenticated?
+      end
+
+      def throttle_authenticated_deprecated_api?
+        deprecated_api_request? &&
+        Gitlab::Throttle.settings.throttle_authenticated_deprecated_api_enabled
+      end
+
       private
 
       def authenticated_user_id(request_formats)
@@ -176,6 +190,15 @@ module Gitlab
       def files_api_path?
         path =~ FILES_PATH_REGEX
       end
+
+      def deprecated_api_request?
+        # The projects member of the groups endpoint is deprecated. If left
+        # unspecified, with_projects defaults to true
+        with_projects = params['with_projects']
+        with_projects = true if with_projects.blank?
+
+        path =~ GROUP_PATH_REGEX && Gitlab::Utils.to_boolean(with_projects)
+      end
     end
   end
 end

lib/gitlab/throttle.rb

@@ -7,7 +7,7 @@ module Gitlab
     # Each of these settings follows the same pattern of specifying separate
     # authenticated and unauthenticated rates via settings. New throttles should
     # ideally be regular as well.
-    REGULAR_THROTTLES = [:api, :packages_api, :files_api].freeze
+    REGULAR_THROTTLES = [:api, :packages_api, :files_api, :deprecated_api].freeze
 
     def self.settings
       Gitlab::CurrentSettings.current_application_settings

spec/lib/gitlab/rack_attack/request_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::RackAttack::Request do
+  using RSpec::Parameterized::TableSyntax
+
   describe 'FILES_PATH_REGEX' do
     subject { described_class::FILES_PATH_REGEX }
 
@@ -13,4 +15,33 @@ RSpec.describe Gitlab::RackAttack::Request do
     it { is_expected.to match('/api/v4/projects/some%2Fnested%2Frepo/repository/files/README') }
     it { is_expected.not_to match('/api/v4/projects/some/nested/repo/repository/files/README') }
   end
+
+  describe '#deprecated_api_request?' do
+    let(:env) { { 'REQUEST_METHOD' => 'GET', 'rack.input' => StringIO.new, 'PATH_INFO' => path, 'QUERY_STRING' => query } }
+    let(:request) { ::Rack::Attack::Request.new(env) }
+
+    subject { !!request.__send__(:deprecated_api_request?) }
+
+    where(:path, :query, :expected) do
+      '/' | '' | false
+
+      '/api/v4/groups/1/'   | '' | true
+      '/api/v4/groups/1'    | '' | true
+      '/api/v4/groups/foo/' | '' | true
+      '/api/v4/groups/foo'  | '' | true
+
+      '/api/v4/groups/1'  | 'with_projects='  | true
+      '/api/v4/groups/1'  | 'with_projects=1' | true
+      '/api/v4/groups/1'  | 'with_projects=0' | false
+
+      '/foo/api/v4/groups/1' | '' | false
+      '/api/v4/groups/1/foo' | '' | false
+
+      '/api/v4/groups/nested%2Fgroup' | '' | true
+    end
+
+    with_them do
+      it { is_expected.to eq(expected) }
+    end
+  end
 end

spec/requests/rack_attack_global_spec.rb

@@ -30,7 +30,11 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
       throttle_unauthenticated_files_api_requests_per_period: 100,
       throttle_unauthenticated_files_api_period_in_seconds: 1,
       throttle_authenticated_files_api_requests_per_period: 100,
-      throttle_authenticated_files_api_period_in_seconds: 1
+      throttle_authenticated_files_api_period_in_seconds: 1,
+      throttle_unauthenticated_deprecated_api_requests_per_period: 100,
+      throttle_unauthenticated_deprecated_api_period_in_seconds: 1,
+      throttle_authenticated_deprecated_api_requests_per_period: 100,
+      throttle_authenticated_deprecated_api_period_in_seconds: 1
     }
   end
 
@@ -790,6 +794,213 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
     end
   end
 
+  describe 'Deprecated API', :api do
+    let_it_be(:group) { create(:group, :public) }
+
+    let(:request_method) { 'GET' }
+    let(:path) { "/groups/#{group.id}" }
+    let(:params) { {} }
+
+    context 'unauthenticated' do
+      let(:throttle_setting_prefix) { 'throttle_unauthenticated_deprecated_api' }
+
+      def do_request
+        get(api(path), params: params)
+      end
+
+      before do
+        settings_to_set[:throttle_unauthenticated_deprecated_api_requests_per_period] = requests_per_period
+        settings_to_set[:throttle_unauthenticated_deprecated_api_period_in_seconds] = period_in_seconds
+      end
+
+      context 'when unauthenticated deprecated api throttle is disabled' do
+        before do
+          settings_to_set[:throttle_unauthenticated_deprecated_api_enabled] = false
+          stub_application_setting(settings_to_set)
+        end
+
+        it 'allows requests over the rate limit' do
+          (1 + requests_per_period).times do
+            do_request
+            expect(response).to have_gitlab_http_status(:ok)
+          end
+        end
+
+        context 'when unauthenticated api throttle is enabled' do
+          before do
+            settings_to_set[:throttle_unauthenticated_api_requests_per_period] = requests_per_period
+            settings_to_set[:throttle_unauthenticated_api_period_in_seconds] = period_in_seconds
+            settings_to_set[:throttle_unauthenticated_api_enabled] = true
+            stub_application_setting(settings_to_set)
+          end
+
+          it 'rejects requests over the unauthenticated api rate limit' do
+            requests_per_period.times do
+              do_request
+              expect(response).to have_gitlab_http_status(:ok)
+            end
+
+            expect_rejection { do_request }
+          end
+        end
+
+        context 'when unauthenticated web throttle is enabled' do
+          before do
+            settings_to_set[:throttle_unauthenticated_web_requests_per_period] = requests_per_period
+            settings_to_set[:throttle_unauthenticated_web_period_in_seconds] = period_in_seconds
+            settings_to_set[:throttle_unauthenticated_web_enabled] = true
+            stub_application_setting(settings_to_set)
+          end
+
+          it 'ignores unauthenticated web throttle' do
+            (1 + requests_per_period).times do
+              do_request
+              expect(response).to have_gitlab_http_status(:ok)
+            end
+          end
+        end
+      end
+
+      context 'when unauthenticated deprecated api throttle is enabled' do
+        before do
+          settings_to_set[:throttle_unauthenticated_deprecated_api_requests_per_period] = requests_per_period # 1
+          settings_to_set[:throttle_unauthenticated_deprecated_api_period_in_seconds] = period_in_seconds # 10_000
+          settings_to_set[:throttle_unauthenticated_deprecated_api_enabled] = true
+          stub_application_setting(settings_to_set)
+        end
+
+        context 'when group endpoint is given with_project=false' do
+          let(:params) { { with_projects: false } }
+
+          it 'permits requests over the rate limit' do
+            (1 + requests_per_period).times do
+              do_request
+              expect(response).to have_gitlab_http_status(:ok)
+            end
+          end
+        end
+
+        it 'rejects requests over the rate limit' do
+          requests_per_period.times do
+            do_request
+            expect(response).to have_gitlab_http_status(:ok)
+          end
+
+          expect_rejection { do_request }
+        end
+
+        context 'when unauthenticated api throttle is lower' do
+          before do
+            settings_to_set[:throttle_unauthenticated_api_requests_per_period] = 0
+            settings_to_set[:throttle_unauthenticated_api_period_in_seconds] = period_in_seconds
+            settings_to_set[:throttle_unauthenticated_api_enabled] = true
+            stub_application_setting(settings_to_set)
+          end
+
+          it 'ignores unauthenticated api throttle' do
+            requests_per_period.times do
+              do_request
+              expect(response).to have_gitlab_http_status(:ok)
+            end
+
+            expect_rejection { do_request }
+          end
+        end
+
+        it_behaves_like 'tracking when dry-run mode is set' do
+          let(:throttle_name) { 'throttle_unauthenticated_deprecated_api' }
+        end
+      end
+    end
+
+    context 'authenticated' do
+      let_it_be(:user) { create(:user) }
+      let_it_be(:member) { group.add_owner(user) }
+      let_it_be(:token) { create(:personal_access_token, user: user) }
+      let_it_be(:other_user) { create(:user) }
+      let_it_be(:other_user_token) { create(:personal_access_token, user: other_user) }
+
+      let(:throttle_setting_prefix) { 'throttle_authenticated_deprecated_api' }
+
+      before do
+        stub_application_setting(settings_to_set)
+      end
+
+      context 'with the token in the query string' do
+        let(:request_args) { [api(path, personal_access_token: token), {}] }
+        let(:other_user_request_args) { [api(path, personal_access_token: other_user_token), {}] }
+
+        it_behaves_like 'rate-limited token-authenticated requests'
+      end
+
+      context 'with the token in the headers' do
+        let(:request_args) { api_get_args_with_token_headers(path, personal_access_token_headers(token)) }
+        let(:other_user_request_args) { api_get_args_with_token_headers(path, personal_access_token_headers(other_user_token)) }
+
+        it_behaves_like 'rate-limited token-authenticated requests'
+      end
+
+      context 'precedence over authenticated api throttle' do
+        before do
+          settings_to_set[:throttle_authenticated_deprecated_api_requests_per_period] = requests_per_period
+          settings_to_set[:throttle_authenticated_deprecated_api_period_in_seconds] = period_in_seconds
+        end
+
+        def do_request
+          get(api(path, personal_access_token: token), params: params)
+        end
+
+        context 'when authenticated deprecated api throttle is enabled' do
+          before do
+            settings_to_set[:throttle_authenticated_deprecated_api_enabled] = true
+          end
+
+          context 'when authenticated api throttle is lower' do
+            before do
+              settings_to_set[:throttle_authenticated_api_requests_per_period] = 0
+              settings_to_set[:throttle_authenticated_api_period_in_seconds] = period_in_seconds
+              settings_to_set[:throttle_authenticated_api_enabled] = true
+              stub_application_setting(settings_to_set)
+            end
+
+            it 'ignores authenticated api throttle' do
+              requests_per_period.times do
+                do_request
+                expect(response).to have_gitlab_http_status(:ok)
+              end
+
+              expect_rejection { do_request }
+            end
+          end
+        end
+
+        context 'when authenticated deprecated api throttle is disabled' do
+          before do
+            settings_to_set[:throttle_authenticated_deprecated_api_enabled] = false
+          end
+
+          context 'when authenticated api throttle is enabled' do
+            before do
+              settings_to_set[:throttle_authenticated_api_requests_per_period] = requests_per_period
+              settings_to_set[:throttle_authenticated_api_period_in_seconds] = period_in_seconds
+              settings_to_set[:throttle_authenticated_api_enabled] = true
+              stub_application_setting(settings_to_set)
+            end
+
+            it 'rejects requests over the authenticated api rate limit' do
+              requests_per_period.times do
+                do_request
+                expect(response).to have_gitlab_http_status(:ok)
+              end
+
+              expect_rejection { do_request }
+            end
+          end
+        end
+      end
+    end
+  end
+
   describe 'throttle bypass header' do
     let(:headers) { {} }
     let(:bypass_header) { 'gitlab-bypass-rate-limiting' }

spec/support/shared_examples/requests/rack_attack_shared_examples.rb

 # frozen_string_literal: true
 #
 # Requires let variables:
-# * throttle_setting_prefix: "throttle_authenticated_api", "throttle_authenticated_web", "throttle_protected_paths", "throttle_authenticated_packages_api", "throttle_authenticated_git_lfs", "throttle_authenticated_files_api"
+# * throttle_setting_prefix: "throttle_authenticated_api", "throttle_authenticated_web", "throttle_protected_paths", "throttle_authenticated_packages_api", "throttle_authenticated_git_lfs", "throttle_authenticated_files_api", "throttle_authenticated_deprecated_api"
 # * request_method
 # * request_args
 # * other_user_request_args
@@ -16,7 +16,8 @@ RSpec.shared_examples 'rate-limited token-authenticated requests' do
       "throttle_authenticated_web" => "throttle_authenticated_web",
       "throttle_authenticated_packages_api" => "throttle_authenticated_packages_api",
       "throttle_authenticated_git_lfs" => "throttle_authenticated_git_lfs",
-      "throttle_authenticated_files_api" => "throttle_authenticated_files_api"
+      "throttle_authenticated_files_api" => "throttle_authenticated_files_api",
+      "throttle_authenticated_deprecated_api" => "throttle_authenticated_deprecated_api"
     }
   end