diff --git a/app/controllers/concerns/check_rate_limit.rb b/app/controllers/concerns/check_rate_limit.rb index 0eaf74fd3a9b4dd1921f6dddb8a9d46417f56e7b..fc3be3ad009a4658ae1eb8b7981f22d5a3a25edc 100644 --- a/app/controllers/concerns/check_rate_limit.rb +++ b/app/controllers/concerns/check_rate_limit.rb @@ -8,10 +8,7 @@ # See lib/api/helpers/rate_limiter.rb for API version module CheckRateLimit def check_rate_limit!(key, scope:, redirect_back: false, **options) - return if bypass_header_set? - return unless rate_limiter.throttled?(key, scope: scope, **options) - - rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user) + return unless Gitlab::ApplicationRateLimiter.throttled_request?(request, current_user, key, scope: scope, **options) return yield if block_given? @@ -23,14 +20,4 @@ def check_rate_limit!(key, scope:, redirect_back: false, **options) render plain: message, status: :too_many_requests end end - - private - - def rate_limiter - ::Gitlab::ApplicationRateLimiter - end - - def bypass_header_set? - ::Gitlab::Throttle.bypass_header.present? && request.get_header(Gitlab::Throttle.bypass_header) == '1' - end end diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 7b0d8cf8dcb21c36fd50d80c448a6cec16d95bc1..5060ce69d9c097b1915d28f99566c50b1191b2d6 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -3,6 +3,7 @@ module IssuableCollections extend ActiveSupport::Concern include PaginatedCollection + include SearchRateLimitable include SortingHelper include SortingPreference include Gitlab::Utils::StrongMemoize diff --git a/app/controllers/concerns/issuable_collections_action.rb b/app/controllers/concerns/issuable_collections_action.rb index 7beb86b51fd4d43652d2342e64771f920e4cf281..b8249345a5481980061b4d6d410f1c39405e9204 100644 --- a/app/controllers/concerns/issuable_collections_action.rb +++ b/app/controllers/concerns/issuable_collections_action.rb @@ -5,6 +5,12 @@ module IssuableCollectionsAction include IssuableCollections include IssuesCalendar + included do + before_action :check_search_rate_limit!, only: [:issues, :merge_requests], if: -> { + params[:search].present? && Feature.enabled?(:rate_limit_issuable_searches) + } + end + # rubocop:disable Gitlab/ModuleWithInstanceVariables def issues show_alert_if_search_is_disabled diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 631e697dd2f32167c6c535f0a544e6ffd236f702..84a9dbc641fb912ef52cf9de311e9307439eed68 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -27,6 +27,10 @@ class Projects::IssuesController < Projects::ApplicationController before_action :set_issuables_index, if: ->(c) { SET_ISSUABLES_INDEX_ONLY_ACTIONS.include?(c.action_name.to_sym) && !index_html_request? } + before_action :check_search_rate_limit!, if: ->(c) { + SET_ISSUABLES_INDEX_ONLY_ACTIONS.include?(c.action_name.to_sym) && !index_html_request? && + params[:search].present? && Feature.enabled?(:rate_limit_issuable_searches) + } # Allow write(create) issue before_action :authorize_create_issue!, only: [:new, :create] diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 3ab1f7d1d327df13f05912e839a0cebc369902ba..1b5ae7af25247c18276376a34f4218421e1befd0 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -28,6 +28,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo :codequality_mr_diff_reports ] before_action :set_issuables_index, only: [:index] + before_action :check_search_rate_limit!, only: [:index], if: -> { + params[:search].present? && Feature.enabled?(:rate_limit_issuable_searches) + } before_action :authenticate_user!, only: [:assign_related_issues] before_action :check_user_can_push_to_source_branch!, only: [:rebase] diff --git a/app/graphql/resolvers/concerns/search_arguments.rb b/app/graphql/resolvers/concerns/search_arguments.rb index ccc012f2bf9c52d33899883b2c3a9a7650a131c4..cc1a13fdf29b4b188f7727578e8cc84b7b8fbc8c 100644 --- a/app/graphql/resolvers/concerns/search_arguments.rb +++ b/app/graphql/resolvers/concerns/search_arguments.rb @@ -18,6 +18,7 @@ module SearchArguments def ready?(**args) validate_search_in_params!(args) validate_anonymous_search_access!(args) + validate_search_rate_limit!(args) super end @@ -39,6 +40,28 @@ def validate_search_in_params!(args) '`search` should be present when including the `in` argument' end + def validate_search_rate_limit!(args) + return if args[:search].blank? || context[:request].nil? || Feature.disabled?(:rate_limit_issuable_searches) + + if current_user.present? + rate_limiter_key = :search_rate_limit + rate_limiter_scope = [current_user] + else + rate_limiter_key = :search_rate_limit_unauthenticated + rate_limiter_scope = [context[:request].ip] + end + + if ::Gitlab::ApplicationRateLimiter.throttled_request?( + context[:request], + current_user, + rate_limiter_key, + scope: rate_limiter_scope + ) + raise Gitlab::Graphql::Errors::ResourceNotAvailable, + 'This endpoint has been requested with the search argument too many times. Try again later.' + end + end + def prepare_finder_params(args) prepare_search_params(args) end diff --git a/config/feature_flags/development/rate_limit_issuable_searches.yml b/config/feature_flags/development/rate_limit_issuable_searches.yml new file mode 100644 index 0000000000000000000000000000000000000000..9a4909da72ee6d4f8e5bef53424cbb30f325d27e --- /dev/null +++ b/config/feature_flags/development/rate_limit_issuable_searches.yml @@ -0,0 +1,8 @@ +--- +name: rate_limit_issuable_searches +introduced_by_url: "https://gitlab.com/gitlab-org/gitlab/-/merge_requests/104208" +rollout_issue_url: +milestone: '15.8' +type: development +group: group::project management +default_enabled: false diff --git a/ee/lib/api/epics.rb b/ee/lib/api/epics.rb index ef5f4aa4e8b01f937e2bdf7983d9d41a00881fa6..c5a389cf864c9092f03cf021970a645be1fb1a0c 100644 --- a/ee/lib/api/epics.rb +++ b/ee/lib/api/epics.rb @@ -123,6 +123,7 @@ class Epics < ::API::Base end get path, urgency: :low do validate_anonymous_search_access! if declared_params[:search].present? + validate_search_rate_limit! if declared_params[:search].present? epics = paginate(find_epics(finder_params: { group_id: user_group.id })).with_api_entity_associations # issuable_metadata has to be set because `Entities::Epic` doesn't inherit from `Entities::IssuableEntity` diff --git a/ee/spec/requests/api/epics_spec.rb b/ee/spec/requests/api/epics_spec.rb index cc122e549589570fb7381047e60daedc7a41a681..558f878186e187217c761597074489ebd3a25cc9 100644 --- a/ee/spec/requests/api/epics_spec.rb +++ b/ee/spec/requests/api/epics_spec.rb @@ -440,6 +440,10 @@ let(:issuable) { epic2 } let(:result) { issuable.id } end + + it_behaves_like 'issuable API rate-limited search' do + let(:issuable) { epic2 } + end end describe "#to_reference" do diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 0b5a471ea12558216011abf0a6d86b06e024db0f..95c81c14bf9d2548d044f6ccedd87cdd460f3328 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -695,6 +695,16 @@ def validate_anonymous_search_access! unprocessable_entity!('User must be authenticated to use search') end + def validate_search_rate_limit! + return unless Feature.enabled?(:rate_limit_issuable_searches) + + if current_user + check_rate_limit!(:search_rate_limit, scope: [current_user]) + else + check_rate_limit!(:search_rate_limit_unauthenticated, scope: [ip_address]) + end + end + private # rubocop:disable Gitlab/ModuleWithInstanceVariables diff --git a/lib/api/helpers/rate_limiter.rb b/lib/api/helpers/rate_limiter.rb index 03f3cd649b161c2f73ace78b690efe4f4265e36d..be92277c25a3bb408af9b6ac400309d788abeca0 100644 --- a/lib/api/helpers/rate_limiter.rb +++ b/lib/api/helpers/rate_limiter.rb @@ -10,25 +10,14 @@ module Helpers # See app/controllers/concerns/check_rate_limit.rb for Rails controllers version module RateLimiter def check_rate_limit!(key, scope:, **options) - return if bypass_header_set? - return unless rate_limiter.throttled?(key, scope: scope, **options) - - rate_limiter.log_request(request, "#{key}_request_limit".to_sym, current_user) + return unless Gitlab::ApplicationRateLimiter.throttled_request?( + request, current_user, key, scope: scope, **options + ) return yield if block_given? render_api_error!({ error: _('This endpoint has been requested too many times. Try again later.') }, 429) end - - private - - def rate_limiter - ::Gitlab::ApplicationRateLimiter - end - - def bypass_header_set? - ::Gitlab::Throttle.bypass_header.present? && request.get_header(Gitlab::Throttle.bypass_header) == '1' - end end end end diff --git a/lib/api/issues.rb b/lib/api/issues.rb index b08819e34e3651973a742be50ff81e688740fef7..7b6306938cfafd89d8c127bbe6d23d2ac7f849fa 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -116,6 +116,7 @@ class Issues < ::API::Base get '/issues_statistics' do authenticate! unless params[:scope] == 'all' validate_anonymous_search_access! if params[:search].present? + validate_search_rate_limit! if declared_params[:search].present? present issues_statistics, with: Grape::Presenters::Presenter end @@ -134,6 +135,7 @@ class Issues < ::API::Base get do authenticate! unless params[:scope] == 'all' validate_anonymous_search_access! if params[:search].present? + validate_search_rate_limit! if declared_params[:search].present? issues = paginate(find_issues) options = { @@ -173,6 +175,7 @@ class Issues < ::API::Base end get ":id/issues" do validate_anonymous_search_access! if declared_params[:search].present? + validate_search_rate_limit! if declared_params[:search].present? issues = paginate(find_issues(group_id: user_group.id, include_subgroups: true)) options = { @@ -192,6 +195,7 @@ class Issues < ::API::Base end get ":id/issues_statistics" do validate_anonymous_search_access! if declared_params[:search].present? + validate_search_rate_limit! if declared_params[:search].present? present issues_statistics(group_id: user_group.id, include_subgroups: true), with: Grape::Presenters::Presenter end @@ -211,6 +215,7 @@ class Issues < ::API::Base end get ":id/issues" do validate_anonymous_search_access! if declared_params[:search].present? + validate_search_rate_limit! if declared_params[:search].present? issues = paginate(find_issues(project_id: user_project.id)) options = { @@ -230,6 +235,7 @@ class Issues < ::API::Base end get ":id/issues_statistics" do validate_anonymous_search_access! if declared_params[:search].present? + validate_search_rate_limit! if declared_params[:search].present? present issues_statistics(project_id: user_project.id), with: Grape::Presenters::Presenter end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index a9572cf7ce6bf98f82684d4b6aa7f2e453dddd77..8a665b71bdb6051fa9caf00acde95ef2bda8df3e 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -149,6 +149,7 @@ def recheck_mergeability_of(merge_requests:) get feature_category: :code_review, urgency: :low do authenticate! unless params[:scope] == 'all' validate_anonymous_search_access! if params[:search].present? + validate_search_rate_limit! if declared_params[:search].present? merge_requests = find_merge_requests present merge_requests, serializer_options_for(merge_requests) @@ -177,6 +178,7 @@ def recheck_mergeability_of(merge_requests:) end get ":id/merge_requests", feature_category: :code_review, urgency: :low do validate_anonymous_search_access! if declared_params[:search].present? + validate_search_rate_limit! if declared_params[:search].present? merge_requests = find_merge_requests(group_id: user_group.id, include_subgroups: true) present merge_requests, serializer_options_for(merge_requests).merge(group: user_group) @@ -244,6 +246,7 @@ def recheck_mergeability_of(merge_requests:) get ":id/merge_requests", feature_category: :code_review, urgency: :low do authorize! :read_merge_request, user_project validate_anonymous_search_access! if declared_params[:search].present? + validate_search_rate_limit! if declared_params[:search].present? merge_requests = find_merge_requests(project_id: user_project.id) diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index 5b1bf99e2972ad9cb603bdbbe850437290c8ee00..a788586ebec97254f8ca5794f2979d845d9d031f 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -115,6 +115,38 @@ def throttled?(key, scope:, resource: nil, threshold: nil, interval: nil, users_ value > threshold_value end + # Similar to #throttled? above but checks for the bypass header in the request and logs the request when it is over the rate limit + # + # @param request [Http::Request] - Web request used to check the header and log + # @param current_user [User] Current user of the request, it can be nil + # @param key [Symbol] Key attribute registered in `.rate_limits` + # @param scope [Array] Array of ActiveRecord models, Strings + # or Symbols to scope throttling to a specific request (e.g. per user + # per project) + # @param resource [ActiveRecord] An ActiveRecord model to count an action + # for (e.g. limit unique project (resource) downloads (action) to five + # per user (scope)) + # @param threshold [Integer] Optional threshold value to override default + # one registered in `.rate_limits` + # @param interval [Integer] Optional interval value to override default + # one registered in `.rate_limits` + # @param users_allowlist [Array] Optional list of usernames to + # exclude from the limit. This param will only be functional if Scope + # includes a current user. + # @param peek [Boolean] Optional. When true the key will not be + # incremented but the current throttled state will be returned. + # + # @return [Boolean] Whether or not a request should be throttled + def throttled_request?(request, current_user, key, scope:, **options) + if ::Gitlab::Throttle.bypass_header.present? && request.get_header(Gitlab::Throttle.bypass_header) == '1' + return false + end + + throttled?(key, scope: scope, **options).tap do |throttled| + log_request(request, "#{key}_request_limit".to_sym, current_user) if throttled + end + end + # Returns the current rate limited state without incrementing the count. # # @param key [Symbol] Key attribute registered in `.rate_limits` diff --git a/spec/controllers/concerns/check_rate_limit_spec.rb b/spec/controllers/concerns/check_rate_limit_spec.rb index 75776acd52007321dec87809e16b1d994dcaef21..25574aa295b40e5d5a55f560ca4d18eb2fcf5d8f 100644 --- a/spec/controllers/concerns/check_rate_limit_spec.rb +++ b/spec/controllers/concerns/check_rate_limit_spec.rb @@ -33,8 +33,8 @@ def render(**args); end end describe '#check_rate_limit!' do - it 'calls ApplicationRateLimiter#throttled? with the right arguments' do - expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(key, scope: scope).and_return(false) + it 'calls ApplicationRateLimiter#throttled_request? with the right arguments' do + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled_request?).with(request, user, key, scope: scope).and_return(false) expect(subject).not_to receive(:render) subject.check_rate_limit!(key, scope: scope) diff --git a/spec/lib/api/helpers/rate_limiter_spec.rb b/spec/lib/api/helpers/rate_limiter_spec.rb index 3640c7e30e778ed9fda0776c1c653efb067f5398..531140a32a36ee6841eae78832db9c09589253cb 100644 --- a/spec/lib/api/helpers/rate_limiter_spec.rb +++ b/spec/lib/api/helpers/rate_limiter_spec.rb @@ -31,8 +31,8 @@ def render_api_error!(**args); end end describe '#check_rate_limit!' do - it 'calls ApplicationRateLimiter#throttled? with the right arguments' do - expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled?).with(key, scope: scope).and_return(false) + it 'calls ApplicationRateLimiter#throttled_request? with the right arguments' do + expect(::Gitlab::ApplicationRateLimiter).to receive(:throttled_request?).with(request, user, key, scope: scope).and_return(false) expect(subject).not_to receive(:render_api_error!) subject.check_rate_limit!(key, scope: scope) diff --git a/spec/lib/gitlab/application_rate_limiter_spec.rb b/spec/lib/gitlab/application_rate_limiter_spec.rb index 41e79f811fad4b133819423269ec6f8159950015..c938393adcee76ed674bb60d16e8dcf2dd6be3d6 100644 --- a/spec/lib/gitlab/application_rate_limiter_spec.rb +++ b/spec/lib/gitlab/application_rate_limiter_spec.rb @@ -214,6 +214,52 @@ end end + describe '.throttled_request?', :freeze_time do + let(:request) { instance_double('Rack::Request') } + + context 'when request is not over the limit' do + it 'returns false and does not log the request' do + expect(subject).not_to receive(:log_request) + + expect(subject.throttled_request?(request, user, :test_action, scope: [user])).to eq(false) + end + end + + context 'when request is over the limit' do + before do + subject.throttled?(:test_action, scope: [user]) + end + + it 'returns true and logs the request' do + expect(subject).to receive(:log_request).with(request, :test_action_request_limit, user) + + expect(subject.throttled_request?(request, user, :test_action, scope: [user])).to eq(true) + end + + context 'when the bypass header is set' do + before do + allow(Gitlab::Throttle).to receive(:bypass_header).and_return('SOME_HEADER') + end + + it 'skips rate limit if set to "1"' do + allow(request).to receive(:get_header).with(Gitlab::Throttle.bypass_header).and_return('1') + + expect(subject).not_to receive(:log_request) + + expect(subject.throttled_request?(request, user, :test_action, scope: [user])).to eq(false) + end + + it 'does not skip rate limit if set to something else than "1"' do + allow(request).to receive(:get_header).with(Gitlab::Throttle.bypass_header).and_return('0') + + expect(subject).to receive(:log_request).with(request, :test_action_request_limit, user) + + expect(subject.throttled_request?(request, user, :test_action, scope: [user])).to eq(true) + end + end + end + end + describe '.peek' do it 'peeks at the current state without changing its value' do freeze_time do diff --git a/spec/requests/api/graphql/issues_spec.rb b/spec/requests/api/graphql/issues_spec.rb index ba6f8ec2cab61a234d90561e35762bff1e98f0c5..145e57bc6aefa936de99a04d33f79177ef601e9f 100644 --- a/spec/requests/api/graphql/issues_spec.rb +++ b/spec/requests/api/graphql/issues_spec.rb @@ -177,6 +177,32 @@ def pagination_query(params) end end + context 'with rate limiting' do + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit, graphql: true do + let_it_be(:current_user) { developer } + + let(:error_message) do + 'This endpoint has been requested with the search argument too many times. Try again later.' + end + + def request + post_graphql(query({ search: 'test' }), current_user: developer) + end + end + + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit_unauthenticated, graphql: true do + let_it_be(:current_user) { nil } + + let(:error_message) do + 'This endpoint has been requested with the search argument too many times. Try again later.' + end + + def request + post_graphql(query({ search: 'test' })) + end + end + end + def execute_query post_query end diff --git a/spec/requests/api/issues/get_project_issues_spec.rb b/spec/requests/api/issues/get_project_issues_spec.rb index 70966d235763bd9e31b8be6712acd1f64f4569ef..6fc3903103be04fcd27159129f83a22f8fa84893 100644 --- a/spec/requests/api/issues/get_project_issues_spec.rb +++ b/spec/requests/api/issues/get_project_issues_spec.rb @@ -11,15 +11,24 @@ let_it_be(:group) { create(:group, :public) } - let(:user2) { create(:user) } - let(:non_member) { create(:user) } + let_it_be(:user2) { create(:user) } + let_it_be(:non_member) { create(:user) } let_it_be(:guest) { create(:user) } let_it_be(:author) { create(:author) } let_it_be(:assignee) { create(:assignee) } - let(:admin) { create(:user, :admin) } - let(:issue_title) { 'foo' } - let(:issue_description) { 'closed' } - let!(:closed_issue) do + let_it_be(:admin) { create(:user, :admin) } + + let_it_be(:milestone) { create(:milestone, title: '1.0.0', project: project) } + let_it_be(:empty_milestone) do + create(:milestone, title: '2.0.0', project: project) + end + + let(:no_milestone_title) { 'None' } + let(:any_milestone_title) { 'Any' } + + let_it_be(:issue_title) { 'foo' } + let_it_be(:issue_description) { 'closed' } + let_it_be(:closed_issue) do create :closed_issue, author: user, assignees: [user], @@ -31,7 +40,7 @@ closed_at: 1.hour.ago end - let!(:confidential_issue) do + let_it_be(:confidential_issue) do create :issue, :confidential, project: project, @@ -41,7 +50,7 @@ updated_at: 2.hours.ago end - let!(:issue) do + let_it_be(:issue) do create :issue, author: user, assignees: [user], @@ -53,22 +62,12 @@ description: issue_description end - let_it_be(:label) do - create(:label, title: 'label', color: '#FFAABB', project: project) - end + let_it_be(:label) { create(:label, title: 'label', color: '#FFAABB', project: project) } + let_it_be(:label_link) { create(:label_link, label: label, target: issue) } - let!(:label_link) { create(:label_link, label: label, target: issue) } - let(:milestone) { create(:milestone, title: '1.0.0', project: project) } - let_it_be(:empty_milestone) do - create(:milestone, title: '2.0.0', project: project) - end + let_it_be(:note) { create(:note_on_issue, author: user, project: project, noteable: issue) } - let!(:note) { create(:note_on_issue, author: user, project: project, noteable: issue) } - - let(:no_milestone_title) { 'None' } - let(:any_milestone_title) { 'Any' } - - let!(:merge_request1) do + let_it_be(:merge_request1) do create(:merge_request, :simple, author: user, @@ -77,7 +76,7 @@ description: "closes #{issue.to_reference}") end - let!(:merge_request2) do + let_it_be(:merge_request2) do create(:merge_request, :simple, author: user, @@ -101,7 +100,7 @@ shared_examples 'project issues statistics' do it 'returns project issues statistics' do - get api("/issues_statistics", user), params: params + get api("/projects/#{project.id}/issues_statistics", current_user), params: params expect(response).to have_gitlab_http_status(:ok) expect(json_response['statistics']).not_to be_nil @@ -138,6 +137,8 @@ end context 'issues_statistics' do + let(:current_user) { nil } + context 'no state is treated as all state' do let(:params) { {} } let(:counts) { { all: 2, closed: 1, opened: 1 } } @@ -534,30 +535,32 @@ end context 'issues_statistics' do + let(:current_user) { user } + context 'no state is treated as all state' do let(:params) { {} } - let(:counts) { { all: 2, closed: 1, opened: 1 } } + let(:counts) { { all: 3, closed: 1, opened: 2 } } it_behaves_like 'project issues statistics' end context 'statistics when all state is passed' do let(:params) { { state: :all } } - let(:counts) { { all: 2, closed: 1, opened: 1 } } + let(:counts) { { all: 3, closed: 1, opened: 2 } } it_behaves_like 'project issues statistics' end context 'closed state is treated as all state' do let(:params) { { state: :closed } } - let(:counts) { { all: 2, closed: 1, opened: 1 } } + let(:counts) { { all: 3, closed: 1, opened: 2 } } it_behaves_like 'project issues statistics' end context 'opened state is treated as all state' do let(:params) { { state: :opened } } - let(:counts) { { all: 2, closed: 1, opened: 1 } } + let(:counts) { { all: 3, closed: 1, opened: 2 } } it_behaves_like 'project issues statistics' end @@ -592,7 +595,7 @@ context 'sort does not affect statistics ' do let(:params) { { state: :opened, order_by: 'updated_at' } } - let(:counts) { { all: 2, closed: 1, opened: 1 } } + let(:counts) { { all: 3, closed: 1, opened: 2 } } it_behaves_like 'project issues statistics' end diff --git a/spec/requests/api/issues/issues_spec.rb b/spec/requests/api/issues/issues_spec.rb index 94f0443e14a252e6a72968dd25d13130262efbcc..b89db82b15049f9db551fd995c8b941bcbefed23 100644 --- a/spec/requests/api/issues/issues_spec.rb +++ b/spec/requests/api/issues/issues_spec.rb @@ -145,6 +145,11 @@ let(:result) { issuable.id } end + it_behaves_like 'issuable API rate-limited search' do + let(:url) { '/issues' } + let(:issuable) { issue } + end + it 'returns authentication error without any scope' do get api('/issues') diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 0b69000ae7e5467b97220ade0dcb132eb7e02322..4cd93603c312e187b668cd84c037bc1fdb9078da 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -55,6 +55,11 @@ let(:issuable) { merge_request } let(:result) { [merge_request_merged.id, merge_request_locked.id, merge_request_closed.id, merge_request.id] } end + + it_behaves_like 'issuable API rate-limited search' do + let(:url) { endpoint_path } + let(:issuable) { merge_request } + end end context 'when authenticated' do @@ -663,6 +668,11 @@ let(:result) { [merge_request_merged.id, merge_request_locked.id, merge_request_closed.id, merge_request.id] } end + it_behaves_like 'issuable API rate-limited search' do + let(:url) { '/merge_requests' } + let(:issuable) { merge_request } + end + it "returns authentication error without any scope" do get api("/merge_requests") diff --git a/spec/requests/dashboard_controller_spec.rb b/spec/requests/dashboard_controller_spec.rb index 9edacb27c93d7d8b960c9ede1b15375dc7c21190..1c8ab843ebe4022e7e1df5f7fd7b42e72bcdfa58 100644 --- a/spec/requests/dashboard_controller_spec.rb +++ b/spec/requests/dashboard_controller_spec.rb @@ -12,4 +12,32 @@ let(:url) { issues_dashboard_url(:ics, assignee_username: user.username) } end end + + context 'issues dashboard' do + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do + let_it_be(:current_user) { create(:user) } + + before do + sign_in current_user + end + + def request + get issues_dashboard_path, params: { scope: 'all', search: 'test' } + end + end + end + + context 'merge requests dashboard' do + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do + let_it_be(:current_user) { create(:user) } + + before do + sign_in current_user + end + + def request + get merge_requests_dashboard_path, params: { scope: 'all', search: 'test' } + end + end + end end diff --git a/spec/requests/projects/issues_controller_spec.rb b/spec/requests/projects/issues_controller_spec.rb index bbf200eaacd802ac7da7e8323318344878979176..a943bd6449c450619f9e45a782c542424d4cbf0e 100644 --- a/spec/requests/projects/issues_controller_spec.rb +++ b/spec/requests/projects/issues_controller_spec.rb @@ -32,6 +32,28 @@ end end + describe 'GET #index.json' do + let_it_be(:public_project) { create(:project, :public) } + + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do + let_it_be(:current_user) { create(:user) } + + before do + sign_in current_user + end + + def request + get project_issues_path(public_project, format: :json), params: { scope: 'all', search: 'test' } + end + end + + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit_unauthenticated do + def request + get project_issues_path(public_project, format: :json), params: { scope: 'all', search: 'test' } + end + end + end + describe 'GET #discussions' do before do login_as(user) diff --git a/spec/requests/projects/merge_requests_controller_spec.rb b/spec/requests/projects/merge_requests_controller_spec.rb index f5f8b5c2d83ed5abeff2ab44dac9440d8c4adefc..aa7a32ef2cf4c4ee5508a43971cc9dcb6c0dd15b 100644 --- a/spec/requests/projects/merge_requests_controller_spec.rb +++ b/spec/requests/projects/merge_requests_controller_spec.rb @@ -19,6 +19,28 @@ end end + describe 'GET #index' do + let_it_be(:public_project) { create(:project, :public) } + + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do + let_it_be(:current_user) { user } + + before do + sign_in current_user + end + + def request + get project_merge_requests_path(public_project), params: { scope: 'all', search: 'test' } + end + end + + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit_unauthenticated do + def request + get project_merge_requests_path(public_project), params: { scope: 'all', search: 'test' } + end + end + end + describe 'GET #discussions' do let_it_be(:discussion) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) } let_it_be(:discussion_reply) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: discussion) } diff --git a/spec/support/shared_examples/controllers/issuables_list_metadata_shared_examples.rb b/spec/support/shared_examples/controllers/issuables_list_metadata_shared_examples.rb index 02915206cc5f19ce10101bc52cf8aec0de51e98a..446bc4cd92f37f38450b3efc4d7a14d27ed727b8 100644 --- a/spec/support/shared_examples/controllers/issuables_list_metadata_shared_examples.rb +++ b/spec/support/shared_examples/controllers/issuables_list_metadata_shared_examples.rb @@ -42,6 +42,10 @@ def create_issuable(issuable_type, project, source_branch:) let(:result_issuable) { issuables.first } let(:search) { result_issuable.title } + before do + stub_application_setting(search_rate_limit: 0, search_rate_limit_unauthenticated: 0) + end + # .simple_sorts is the same across all Sortable classes sorts = ::Issue.simple_sorts.keys + %w[popularity priority label_priority] sorts.each do |sort| diff --git a/spec/support/shared_examples/controllers/rate_limited_endpoint_shared_examples.rb b/spec/support/shared_examples/controllers/rate_limited_endpoint_shared_examples.rb index 20edca1ee9f81d9e8b7b7ffa1e23c27204c5c43c..b34038ca0e49828def0c3b891cad9ef3798ff3c8 100644 --- a/spec/support/shared_examples/controllers/rate_limited_endpoint_shared_examples.rb +++ b/spec/support/shared_examples/controllers/rate_limited_endpoint_shared_examples.rb @@ -5,7 +5,9 @@ # - current_user # - error_message # optional -RSpec.shared_examples 'rate limited endpoint' do |rate_limit_key:| +RSpec.shared_examples 'rate limited endpoint' do |rate_limit_key:, graphql: false| + let(:error_message) { _('This endpoint has been requested too many times. Try again later.') } + context 'when rate limiter enabled', :freeze_time, :clean_gitlab_redis_rate_limiting do let(:expected_logger_attributes) do { @@ -25,8 +27,6 @@ end end - let(:error_message) { _('This endpoint has been requested too many times. Try again later.') } - before do allow(Gitlab::ApplicationRateLimiter).to receive(:threshold).with(rate_limit_key).and_return(1) end @@ -37,12 +37,16 @@ request request - expect(response).to have_gitlab_http_status(:too_many_requests) + if graphql + expect_graphql_errors_to_include(error_message) + else + expect(response).to have_gitlab_http_status(:too_many_requests) - if example.metadata[:type] == :controller - expect(response.body).to eq(error_message) - else # it is API spec - expect(response.body).to eq({ message: { error: error_message } }.to_json) + if response.content_type == 'application/json' # it is API spec + expect(response.body).to eq({ message: { error: error_message } }.to_json) + else + expect(response.body).to eq(error_message) + end end end end @@ -57,7 +61,11 @@ request - expect(response).not_to have_gitlab_http_status(:too_many_requests) + if graphql + expect_graphql_errors_to_be_empty + else + expect(response).not_to have_gitlab_http_status(:too_many_requests) + end end end end diff --git a/spec/support/shared_examples/requests/api/issuable_search_shared_examples.rb b/spec/support/shared_examples/requests/api/issuable_search_shared_examples.rb index 9f67bd69560458f7402898ba9694aa4f4feb991f..fcde3b65b4f645e9622b2a569e615f027c90bd22 100644 --- a/spec/support/shared_examples/requests/api/issuable_search_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/issuable_search_shared_examples.rb @@ -34,3 +34,35 @@ end end end + +RSpec.shared_examples 'issuable API rate-limited search' do + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit do + let(:current_user) { user } + + def request + get api(url, current_user), params: { scope: 'all', search: issuable.title } + end + end + + it_behaves_like 'rate limited endpoint', rate_limit_key: :search_rate_limit_unauthenticated do + def request + get api(url), params: { scope: 'all', search: issuable.title } + end + end + + context 'when rate_limit_issuable_searches is disabled', :freeze_time, :clean_gitlab_redis_rate_limiting do + before do + stub_feature_flags(rate_limit_issuable_searches: false) + + allow(Gitlab::ApplicationRateLimiter).to receive(:threshold) + .with(:search_rate_limit_unauthenticated).and_return(1) + end + + it 'does not enforce the rate limit' do + get api(url), params: { scope: 'all', search: issuable.title } + get api(url), params: { scope: 'all', search: issuable.title } + + expect(response).to have_gitlab_http_status(:ok) + end + end +end