diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 7d3ac2501e10666d4699c234c24a6997402e9a1a..b136831635d97bb2c5031ae5eadd5ab1164ca677 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -599,9 +599,9 @@ def present_project def check_export_rate_limit! prefixed_action = "project_#{params[:action]}".to_sym - group_scope = params[:action] == 'download_export' ? @project.namespace : nil + project_scope = params[:action] == 'download_export' ? @project : nil - check_rate_limit!(prefixed_action, scope: [current_user, group_scope].compact) + check_rate_limit!(prefixed_action, scope: [current_user, project_scope].compact) end def render_edit diff --git a/doc/user/project/settings/import_export.md b/doc/user/project/settings/import_export.md index d72924f01fb2765247e5119bf6accc0da709449e..9bdfcf91cd26800522763e0e9f41fe23853a86b6 100644 --- a/doc/user/project/settings/import_export.md +++ b/doc/user/project/settings/import_export.md @@ -320,7 +320,7 @@ To help avoid abuse, by default, users are rate limited to: | Request type | Limit | |:----------------|:--------------------------------| | Export | 6 projects per minute | -| Download export | 1 download per group per minute | +| Download export | 1 download per project per minute | | Import | 6 projects per minute | ## Migrate groups by uploading an export file (deprecated) diff --git a/lib/api/project_export.rb b/lib/api/project_export.rb index 6d4db1dd9804af14f926f1aa86ccc40784bc2111..c1e73dedfaf8b79ec83171868111b5db12d05936 100644 --- a/lib/api/project_export.rb +++ b/lib/api/project_export.rb @@ -44,7 +44,7 @@ class ProjectExport < ::API::Base produces %w[application/octet-stream application/json] end get ':id/export/download' do - check_rate_limit! :project_download_export, scope: [current_user, user_project.namespace] + check_rate_limit! :project_download_export, scope: [current_user, user_project] if user_project.export_file_exists?(current_user) if user_project.export_archive_exists?(current_user) diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index b57687137dc3c5421dd093d3b4d00c37bc2ecad5..ba9c681b010c0942e9efb40f61275ee20e17fe0d 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -1702,7 +1702,7 @@ def update_project_feature end it 'prevents requesting project export' do - post action, params: { namespace_id: project.namespace, id: project } + get action, params: { namespace_id: project.namespace, id: project } 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) @@ -1710,39 +1710,12 @@ def update_project_feature end context 'applies correct scope when throttling', :clean_gitlab_redis_rate_limiting do - before do - stub_application_setting(project_download_export_limit: 1) - - travel_to Date.current.beginning_of_day - end - - after do - travel_back - end - - it 'applies throttle per namespace' do + it 'applies throttle per project' do expect(Gitlab::ApplicationRateLimiter) .to receive(:throttled?) - .with(:project_download_export, scope: [user, project.namespace]) - - post action, params: { namespace_id: project.namespace, id: project } - end - - it 'throttles downloads within same namespaces' do - # simulate prior request to the same namespace, which increments the rate limit counter for that scope - Gitlab::ApplicationRateLimiter.throttled?(:project_download_export, scope: [user, project.namespace]) + .with(:project_download_export, scope: [user, project]) get action, params: { namespace_id: project.namespace, id: project } - expect(response).to have_gitlab_http_status(:too_many_requests) - end - - it 'allows downloads from different namespaces' do - # simulate prior request to a different namespace, which increments the rate limit counter for that scope - Gitlab::ApplicationRateLimiter.throttled?(:project_download_export, - scope: [user, create(:project, :with_export).namespace]) - - get action, params: { namespace_id: project.namespace, id: project } - expect(response).to have_gitlab_http_status(:ok) end end end diff --git a/spec/requests/api/project_export_spec.rb b/spec/requests/api/project_export_spec.rb index c694e6ed8d603552ea15281113961c288196a899..c4abea5da9ffc0431a5248abcec13bad6a62fb00 100644 --- a/spec/requests/api/project_export_spec.rb +++ b/spec/requests/api/project_export_spec.rb @@ -267,35 +267,15 @@ end it 'prevents requesting project export' do + expect(Gitlab::ApplicationRateLimiter).to receive(:throttled?) + .with(:project_download_export, scope: [user, project]).and_call_original + request expect(response).to have_gitlab_http_status(:too_many_requests) expect(json_response['message']['error']).to eq('This endpoint has been requested too many times. Try again later.') end end - - context 'applies correct scope when throttling' do - before do - stub_application_setting(project_download_export_limit: 1) - end - - it 'throttles downloads within same namespaces', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/413230' do - # simulate prior request to the same namespace, which increments the rate limit counter for that scope - Gitlab::ApplicationRateLimiter.throttled?(:project_download_export, scope: [user, project_finished.namespace]) - - get api(download_path_finished, user, admin_mode: true) - expect(response).to have_gitlab_http_status(:too_many_requests) - end - - it 'allows downloads from different namespaces' do - # simulate prior request to a different namespace, which increments the rate limit counter for that scope - Gitlab::ApplicationRateLimiter.throttled?(:project_download_export, - scope: [user, create(:project, :with_export, export_user: user).namespace]) - - get api(download_path_finished, user, admin_mode: true) - expect(response).to have_gitlab_http_status(:ok) - end - end end context 'when user is a maintainer' do