Merge branch '321047-confidential-issue' into 'master'

Add create rate limit to NotesActions

See merge request gitlab-org/gitlab!53896

app/controllers/concerns/check_rate_limit.rb

+# frozen_string_literal: true
+
+# == CheckRateLimit
+#
+# Controller concern that checks if the rate limit for a given action is throttled by calling the
+# Gitlab::ApplicationRateLimiter class. If the action is throttled for the current user, the request
+# will be logged and an error message will be rendered with a Too Many Requests response status.
+module CheckRateLimit
+  def check_rate_limit(key)
+    return unless rate_limiter.throttled?(key, scope: current_user, users_allowlist: rate_limit_users_allowlist)
+
+    rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user)
+    render plain: _('This endpoint has been requested too many times. Try again later.'), status: :too_many_requests
+  end
+
+  def rate_limiter
+    ::Gitlab::ApplicationRateLimiter
+  end
+
+  def rate_limit_users_allowlist
+    Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist
+  end
+end

app/controllers/concerns/notes_actions.rb

@@ -3,6 +3,7 @@
 module NotesActions
   include RendersNotes
   include Gitlab::Utils::StrongMemoize
+  include CheckRateLimit
   extend ActiveSupport::Concern
 
   # last_fetched_at is an integer number of microseconds, which is the same
@@ -15,6 +16,7 @@ module NotesActions
     before_action :require_noteable!, only: [:index, :create]
     before_action :authorize_admin_note!, only: [:update, :destroy]
     before_action :note_project, only: [:create]
+    before_action -> { check_rate_limit(:notes_create) }, only: [:create]
   end
 
   def index

app/controllers/projects/notes_controller.rb

@@ -10,7 +10,6 @@ class Projects::NotesController < Projects::ApplicationController
   before_action :authorize_read_note!
   before_action :authorize_create_note!, only: [:create]
   before_action :authorize_resolve_note!, only: [:resolve, :unresolve]
-  before_action :create_rate_limit, only: [:create]
 
   feature_category :issue_tracking
 
@@ -91,20 +90,4 @@ class Projects::NotesController < Projects::ApplicationController
   def whitelist_query_limiting
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42383')
   end
-
-  def create_rate_limit
-    key = :notes_create
-    return unless rate_limiter.throttled?(key, scope: [current_user], users_allowlist: rate_limit_users_allowlist)
-
-    rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user)
-    render plain: _('This endpoint has been requested too many times. Try again later.'), status: :too_many_requests
-  end
-
-  def rate_limiter
-    ::Gitlab::ApplicationRateLimiter
-  end
-
-  def rate_limit_users_allowlist
-    Gitlab::CurrentSettings.current_application_settings.notes_create_limit_allowlist
-  end
 end

ee/spec/controllers/groups/epics/notes_controller_spec.rb

@@ -99,6 +99,11 @@ RSpec.describe Groups::Epics::NotesController do
         expect(parsed_response[:errors]).to be_nil
       end
     end
+
+    it_behaves_like 'request exceeding rate limit', :clean_gitlab_redis_cache do
+      let(:params) { request_params.except(:format) }
+      let(:request_full_path) { group_epic_notes_path(group, epic) }
+    end
   end
 
   describe 'PUT update' do

ee/spec/controllers/projects/security/vulnerabilities/notes_controller_spec.rb

@@ -138,6 +138,11 @@ RSpec.describe Projects::Security::Vulnerabilities::NotesController do
         end
       end
     end
+
+    it_behaves_like 'request exceeding rate limit', :clean_gitlab_redis_cache do
+      let(:params) { request_params.except(:format) }
+      let(:request_full_path) { project_security_vulnerability_notes_path(project, vulnerability) }
+    end
   end
 
   describe 'PUT update' do

lib/gitlab/application_rate_limiter.rb

@@ -47,7 +47,7 @@ module Gitlab
       # @option scope [Array<ActiveRecord>] Array of ActiveRecord models to scope throttling to a specific request (e.g. per user per project)
       # @option threshold [Integer] Optional threshold value to override default one registered in `.rate_limits`
       # @option interval [Integer] Optional interval value to override default one registered in `.rate_limits`
-      # @option users_allowlist [Array<String>] Optional list of usernames to excepted from the limit. This param will only be functional if Scope includes a current user.
+      # @option users_allowlist [Array<String>] Optional list of usernames to exclude from the limit. This param will only be functional if Scope includes a current user.
       #
       # @return [Boolean] Whether or not a request should be throttled
       def throttled?(key, **options)

spec/controllers/projects/notes_controller_spec.rb

@@ -762,49 +762,9 @@ RSpec.describe Projects::NotesController do
       end
     end
 
-    context 'when the endpoint receives requests above the limit' do
-      before do
-        stub_application_setting(notes_create_limit: 3)
-      end
-
-      it 'prevents from creating more notes', :request_store do
-        3.times { create! }
-
-        expect { create! }
-          .to change { Gitlab::GitalyClient.get_request_count }.by(0)
-
-        create!
-        expect(response.body).to eq(_('This endpoint has been requested too many times. Try again later.'))
-        expect(response).to have_gitlab_http_status(:too_many_requests)
-      end
-
-      it 'logs the event in auth.log' do
-        attributes = {
-          message: 'Application_Rate_Limiter_Request',
-          env: :notes_create_request_limit,
-          remote_ip: '0.0.0.0',
-          request_method: 'POST',
-          path: "/#{project.full_path}/notes",
-          user_id: user.id,
-          username: user.username
-        }
-
-        expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
-
-        project.add_developer(user)
-        sign_in(user)
-
-        4.times { create! }
-      end
-
-      it 'allows user in allow-list to create notes, even if the case is different' do
-        user.update_attribute(:username, user.username.titleize)
-        stub_application_setting(notes_create_limit_allowlist: ["#{user.username.downcase}"])
-        3.times { create! }
-
-        create!
-        expect(response).to have_gitlab_http_status(:found)
-      end
+    it_behaves_like 'request exceeding rate limit', :clean_gitlab_redis_cache do
+      let(:params) { request_params.except(:format) }
+      let(:request_full_path) { project_notes_path(project) }
     end
   end
 

spec/controllers/snippets/notes_controller_spec.rb

@@ -141,6 +141,11 @@ RSpec.describe Snippets::NotesController do
       it 'creates the note' do
         expect { post :create, params: request_params }.to change { Note.count }.by(1)
       end
+
+      it_behaves_like 'request exceeding rate limit', :clean_gitlab_redis_cache do
+        let(:params) { request_params }
+        let(:request_full_path) { snippet_notes_path(public_snippet) }
+      end
     end
 
     context 'when a snippet is internal' do
@@ -164,6 +169,11 @@ RSpec.describe Snippets::NotesController do
       it 'creates the note' do
         expect { post :create, params: request_params }.to change { Note.count }.by(1)
       end
+
+      it_behaves_like 'request exceeding rate limit', :clean_gitlab_redis_cache do
+        let(:params) { request_params }
+        let(:request_full_path) { snippet_notes_path(internal_snippet) }
+      end
     end
 
     context 'when a snippet is private' do
@@ -228,6 +238,12 @@ RSpec.describe Snippets::NotesController do
         it 'creates the note' do
           expect { post :create, params: request_params }.to change { Note.count }.by(1)
         end
+
+        it_behaves_like 'request exceeding rate limit', :clean_gitlab_redis_cache do
+          let(:params) { request_params }
+          let(:request_full_path) { snippet_notes_path(private_snippet) }
+          let(:user) { private_snippet.author }
+        end
       end
     end
   end

spec/support/shared_examples/controllers/create_notes_rate_limit_shared_examples.rb

+# frozen_string_literal: true
+#
+# Requires a context containing:
+# - user
+# - params
+# - request_full_path
+
+RSpec.shared_examples 'request exceeding rate limit' do
+  before do
+    stub_application_setting(notes_create_limit: 2)
+    2.times { post :create, params: params }
+  end
+
+  it 'prevents from creating more notes', :request_store do
+    expect { post :create, params: params }
+      .to change { Note.count }.by(0)
+
+    expect(response).to have_gitlab_http_status(:too_many_requests)
+    expect(response.body).to eq(_('This endpoint has been requested too many times. Try again later.'))
+  end
+
+  it 'logs the event in auth.log' do
+    attributes = {
+      message: 'Application_Rate_Limiter_Request',
+      env: :notes_create_request_limit,
+      remote_ip: '0.0.0.0',
+      request_method: 'POST',
+      path: request_full_path,
+      user_id: user.id,
+      username: user.username
+    }
+
+    expect(Gitlab::AuthLogger).to receive(:error).with(attributes).once
+    post :create, params: params
+  end
+
+  it 'allows user in allow-list to create notes, even if the case is different' do
+    user.update_attribute(:username, user.username.titleize)
+    stub_application_setting(notes_create_limit_allowlist: ["#{user.username.downcase}"])
+
+    post :create, params: params
+    expect(response).to have_gitlab_http_status(:found)
+  end
+end