diff options
22 files changed, 470 insertions, 17 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0d0384ba52f..224ce75c83f 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -13,6 +13,7 @@ class ApplicationController < ActionController::Base include WithPerformanceBar include SessionlessAuthentication include ConfirmEmailWarning + include Gitlab::Tracking::ControllerConcern before_action :authenticate_user! before_action :enforce_terms!, if: :should_enforce_terms? diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 8c5be1c315d..42fe42398f1 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -265,6 +265,10 @@ module ApplicationSettingsHelper :throttle_unauthenticated_enabled, :throttle_unauthenticated_period_in_seconds, :throttle_unauthenticated_requests_per_period, + :throttle_protected_paths_enabled, + :throttle_protected_paths_period_in_seconds, + :throttle_protected_paths_requests_per_period, + :protected_paths_raw, :time_tracking_limit_to_hours, :two_factor_grace_period, :unique_ips_limit_enabled, @@ -308,6 +312,10 @@ module ApplicationSettingsHelper def instance_clusters_enabled? can?(current_user, :read_cluster, Clusters::Instance.new) end + + def omnibus_protected_paths_throttle? + Rack::Attack.throttles.key?('protected paths') + end end ApplicationSettingsHelper.prepend_if_ee('EE::ApplicationSettingsHelper') # rubocop: disable Cop/InjectEnterpriseEditionModule diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 92526def144..02f214341fb 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -210,6 +210,10 @@ class ApplicationSetting < ApplicationRecord presence: true, if: :static_objects_external_storage_url? + validates :protected_paths, + length: { maximum: 100, message: N_('is too long (maximum is 100 entries)') }, + allow_nil: false + SUPPORTED_KEY_TYPES.each do |type| validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type } end diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index 8d9597aa5a4..d486347bb1d 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -4,7 +4,7 @@ module ApplicationSettingImplementation extend ActiveSupport::Concern include Gitlab::Utils::StrongMemoize - DOMAIN_LIST_SEPARATOR = %r{\s*[,;]\s* # comma or semicolon, optionally surrounded by whitespace + STRING_LIST_SEPARATOR = %r{\s*[,;]\s* # comma or semicolon, optionally surrounded by whitespace | # or \s # any whitespace character | # or @@ -16,6 +16,19 @@ module ApplicationSettingImplementation FORBIDDEN_KEY_VALUE = KeyRestrictionValidator::FORBIDDEN SUPPORTED_KEY_TYPES = %i[rsa dsa ecdsa ed25519].freeze + DEFAULT_PROTECTED_PATHS = [ + '/users/password', + '/users/sign_in', + '/api/v3/session.json', + '/api/v3/session', + '/api/v4/session.json', + '/api/v4/session', + '/users', + '/users/confirmation', + '/unsubscribes/', + '/import/github/personal_access_token' + ].freeze + class_methods do def defaults { @@ -92,6 +105,10 @@ module ApplicationSettingImplementation throttle_unauthenticated_enabled: false, throttle_unauthenticated_period_in_seconds: 3600, throttle_unauthenticated_requests_per_period: 3600, + throttle_protected_paths_enabled: false, + throttle_protected_paths_in_seconds: 10, + throttle_protected_paths_per_period: 60, + protected_paths: DEFAULT_PROTECTED_PATHS, time_tracking_limit_to_hours: false, two_factor_grace_period: 48, unique_ips_limit_enabled: false, @@ -149,11 +166,11 @@ module ApplicationSettingImplementation end def domain_whitelist_raw=(values) - self.domain_whitelist = domain_strings_to_array(values) + self.domain_whitelist = strings_to_array(values) end def domain_blacklist_raw=(values) - self.domain_blacklist = domain_strings_to_array(values) + self.domain_blacklist = strings_to_array(values) end def domain_blacklist_file=(file) @@ -167,7 +184,7 @@ module ApplicationSettingImplementation def outbound_local_requests_whitelist_raw=(values) clear_memoization(:outbound_local_requests_whitelist_arrays) - self.outbound_local_requests_whitelist = domain_strings_to_array(values) + self.outbound_local_requests_whitelist = strings_to_array(values) end def add_to_outbound_local_requests_whitelist(values_array) @@ -200,8 +217,16 @@ module ApplicationSettingImplementation end end + def protected_paths_raw + array_to_string(self.protected_paths) + end + + def protected_paths_raw=(values) + self.protected_paths = strings_to_array(values) + end + def asset_proxy_whitelist=(values) - values = domain_strings_to_array(values) if values.is_a?(String) + values = strings_to_array(values) if values.is_a?(String) # make sure we always whitelist the running host values << Gitlab.config.gitlab.host unless values.include?(Gitlab.config.gitlab.host) @@ -316,11 +341,11 @@ module ApplicationSettingImplementation arr&.join("\n") end - def domain_strings_to_array(values) + def strings_to_array(values) return [] unless values values - .split(DOMAIN_LIST_SEPARATOR) + .split(STRING_LIST_SEPARATOR) .map(&:strip) .reject(&:empty?) .uniq diff --git a/app/views/admin/application_settings/_protected_paths.html.haml b/app/views/admin/application_settings/_protected_paths.html.haml new file mode 100644 index 00000000000..cfb04562b59 --- /dev/null +++ b/app/views/admin/application_settings/_protected_paths.html.haml @@ -0,0 +1,31 @@ += form_for @application_setting, url: network_admin_application_settings_path(anchor: 'js-protected-paths-settings'), html: { class: 'fieldset-form' } do |f| + = form_errors(@application_setting) + + %fieldset + - if omnibus_protected_paths_throttle? + .bs-callout.bs-callout-danger + - relative_url_link = 'https://docs.gitlab.com/ee/user/admin_area/settings/protected_paths.html#migrating-from-omnibus' + - relative_url_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: relative_url_link } + = _("Omnibus Protected Paths throttle is active. From 12.4, Omnibus throttle is deprecated and will be removed in a future release. Please read the %{relative_url_link_start}Migrating Protected Paths documentation%{relative_url_link_end}.").html_safe % { relative_url_link_start: relative_url_link_start, relative_url_link_end: '</a>'.html_safe } + + .form-group + .form-check + = f.check_box :throttle_protected_paths_enabled, class: 'form-check-input' + = f.label :throttle_protected_paths_enabled, class: 'form-check-label' do + = _('Enable protected paths rate limit') + %span.form-text.text-muted + = _('Helps reduce request volume for protected paths') + .form-group + = f.label :throttle_protected_paths_requests_per_period, 'Max requests per period per user', class: 'label-bold' + = f.number_field :throttle_protected_paths_requests_per_period, class: 'form-control' + .form-group + = f.label :throttle_protected_paths_period_in_seconds, 'Rate limit period in seconds', class: 'label-bold' + = f.number_field :throttle_protected_paths_period_in_seconds, class: 'form-control' + .form-group + = f.label :protected_paths, class: 'label-bold' do + - relative_url_link = 'https://docs.gitlab.com/omnibus/settings/configuration.html#configuring-a-relative-url-for-gitlab' + - relative_url_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: relative_url_link } + = _('All paths are relative to the GitLab URL. Do not include %{relative_url_link_start}relative URL%{relative_url_link_end}.').html_safe % { relative_url_link_start: relative_url_link_start, relative_url_link_end: '</a>'.html_safe } + = f.text_area :protected_paths_raw, placeholder: '/users/sign_in,/users/password', class: 'form-control', rows: 10 + + = f.submit 'Save changes', class: 'btn btn-success' diff --git a/app/views/admin/application_settings/network.html.haml b/app/views/admin/application_settings/network.html.haml index 3a4d901ca1d..a1f39e22e80 100644 --- a/app/views/admin/application_settings/network.html.haml +++ b/app/views/admin/application_settings/network.html.haml @@ -34,3 +34,14 @@ = _('Allow requests to the local network from hooks and services.') .settings-content = render 'outbound' + +%section.settings.as-protected-paths.no-animate#js-protected-paths-settings{ class: ('expanded' if expanded_by_default?) } + .settings-header + %h4 + = _('Protected Paths') + %button.btn.btn-default.js-settings-toggle{ type: 'button' } + = expanded_by_default? ? _('Collapse') : _('Expand') + %p + = _('Configure paths to be protected by Rack Attack. A web server restart is required after changing these settings.') + .settings-content + = render 'protected_paths' diff --git a/changelogs/unreleased/mc-moves-protected-path-throttle-to-gitlab-rails.yml b/changelogs/unreleased/mc-moves-protected-path-throttle-to-gitlab-rails.yml new file mode 100644 index 00000000000..47c6c926b42 --- /dev/null +++ b/changelogs/unreleased/mc-moves-protected-path-throttle-to-gitlab-rails.yml @@ -0,0 +1,5 @@ +--- +title: Allow users to configure protected paths from Admin panel +merge_request: 31246 +author: +type: added diff --git a/config/initializers/rack_attack_logging.rb b/config/initializers/rack_attack_logging.rb index b43fff24bb0..be7c2175cb2 100644 --- a/config/initializers/rack_attack_logging.rb +++ b/config/initializers/rack_attack_logging.rb @@ -12,10 +12,18 @@ ActiveSupport::Notifications.subscribe('rack.attack') do |name, start, finish, r path: req.fullpath } - if %w(throttle_authenticated_api throttle_authenticated_web).include? req.env['rack.attack.matched'] + throttles_with_user_information = [ + :throttle_authenticated_api, + :throttle_authenticated_web, + :throttle_authenticated_protected_paths_api, + :throttle_authenticated_protected_paths_web + ] + + if throttles_with_user_information.include? req.env['rack.attack.matched'].to_sym user_id = req.env['rack.attack.match_discriminator'] user = User.find_by(id: user_id) + rack_attack_info[:throttle_type] = req.env['rack.attack.matched'] rack_attack_info[:user_id] = user_id rack_attack_info[:username] = user.username unless user.nil? end diff --git a/config/initializers/rack_attack_global.rb b/config/initializers/rack_attack_new.rb index 7f0439ef9bf..2812ceb3fd5 100644 --- a/config/initializers/rack_attack_global.rb +++ b/config/initializers/rack_attack_new.rb @@ -3,6 +3,15 @@ module Gitlab::Throttle Gitlab::CurrentSettings.current_application_settings end + def self.protected_paths_enabled? + !self.omnibus_protected_paths_present? && + self.settings.throttle_protected_paths_enabled? + end + + def self.omnibus_protected_paths_present? + Rack::Attack.throttles.key?('protected paths') + end + def self.unauthenticated_options limit_proc = proc { |req| settings.throttle_unauthenticated_requests_per_period } period_proc = proc { |req| settings.throttle_unauthenticated_period_in_seconds.seconds } @@ -20,6 +29,13 @@ module Gitlab::Throttle period_proc = proc { |req| settings.throttle_authenticated_web_period_in_seconds.seconds } { limit: limit_proc, period: period_proc } end + + def self.protected_paths_options + limit_proc = proc { |req| settings.throttle_protected_paths_requests_per_period } + period_proc = proc { |req| settings.throttle_protected_paths_period_in_seconds.seconds } + + { limit: limit_proc, period: period_proc } + end end class Rack::Attack @@ -42,6 +58,28 @@ class Rack::Attack req.authenticated_user_id([:api, :rss, :ics]) end + throttle('throttle_unauthenticated_protected_paths', Gitlab::Throttle.protected_paths_options) do |req| + Gitlab::Throttle.protected_paths_enabled? && + req.unauthenticated? && + !req.should_be_skipped? && + req.protected_path? && + req.ip + end + + throttle('throttle_authenticated_protected_paths_api', Gitlab::Throttle.protected_paths_options) do |req| + Gitlab::Throttle.protected_paths_enabled? && + req.api_request? && + req.protected_path? && + req.authenticated_user_id([:api]) + end + + throttle('throttle_authenticated_protected_paths_web', Gitlab::Throttle.protected_paths_options) do |req| + Gitlab::Throttle.protected_paths_enabled? && + req.web_request? && + req.protected_path? && + req.authenticated_user_id([:api, :rss, :ics]) + end + class Request def unauthenticated? !authenticated_user_id([:api, :rss, :ics]) @@ -66,6 +104,24 @@ class Rack::Attack def web_request? !api_request? end + + def protected_path? + !protected_path_regex.nil? + end + + def protected_path_regex + path =~ protected_paths_regex + end + + private + + def protected_paths + Gitlab::CurrentSettings.current_application_settings.protected_paths + end + + def protected_paths_regex + Regexp.union(protected_paths.map { |path| /\A#{Regexp.escape(path)}/ }) + end end end diff --git a/db/migrate/20190801142441_add_throttle_protected_path_columns.rb b/db/migrate/20190801142441_add_throttle_protected_path_columns.rb new file mode 100644 index 00000000000..bb6d54f3b7b --- /dev/null +++ b/db/migrate/20190801142441_add_throttle_protected_path_columns.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class AddThrottleProtectedPathColumns < ActiveRecord::Migration[5.2] + DOWNTIME = false + + DEFAULT_PROTECTED_PATHS = [ + '/users/password', + '/users/sign_in', + '/api/v3/session.json', + '/api/v3/session', + '/api/v4/session.json', + '/api/v4/session', + '/users', + '/users/confirmation', + '/unsubscribes/', + '/import/github/personal_access_token' + ] + + def change + add_column :application_settings, :throttle_protected_paths_enabled, :boolean, default: true, null: false + add_column :application_settings, :throttle_protected_paths_requests_per_period, :integer, default: 10, null: false + add_column :application_settings, :throttle_protected_paths_period_in_seconds, :integer, default: 60, null: false + add_column :application_settings, :protected_paths, :string, array: true, limit: 255, default: DEFAULT_PROTECTED_PATHS + end +end diff --git a/db/schema.rb b/db/schema.rb index 8fcced21d56..a1a5e19e75d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -322,6 +322,10 @@ ActiveRecord::Schema.define(version: 2019_09_26_041216) do t.string "encrypted_asset_proxy_secret_key_iv" t.string "static_objects_external_storage_url", limit: 255 t.string "static_objects_external_storage_auth_token", limit: 255 + t.boolean "throttle_protected_paths_enabled", default: true, null: false + t.integer "throttle_protected_paths_requests_per_period", default: 10, null: false + t.integer "throttle_protected_paths_period_in_seconds", default: 60, null: false + t.string "protected_paths", limit: 255, default: ["/users/password", "/users/sign_in", "/api/v3/session.json", "/api/v3/session", "/api/v4/session.json", "/api/v4/session", "/users", "/users/confirmation", "/unsubscribes/", "/import/github/personal_access_token"], array: true t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id" t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id" t.index ["instance_administration_project_id"], name: "index_applicationsettings_on_instance_administration_project_id" diff --git a/doc/development/changelog.md b/doc/development/changelog.md index cd09438e7c7..954a4de53a0 100644 --- a/doc/development/changelog.md +++ b/doc/development/changelog.md @@ -33,8 +33,13 @@ the `author` field. GitLab team members **should not**. ## What warrants a changelog entry? +- Any change that introduces a database migration **must** have a changelog entry. - Any user-facing change **should** have a changelog entry. Example: "GitLab now uses system fonts for all text." +- Performance improvements **should** have a changelog entry. +- _Any_ contribution from a community member, no matter how small, **may** have + a changelog entry regardless of these guidelines if the contributor wants one. + Example: "Fixed a typo on the search results page." - Any docs-only changes **should not** have a changelog entry. - Any change behind a feature flag **should not** have a changelog entry. The entry should be added [in the merge request removing the feature flags](feature_flags/development.md). - A fix for a regression introduced and then fixed in the same release (i.e., @@ -43,12 +48,6 @@ the `author` field. GitLab team members **should not**. - Any developer-facing change (e.g., refactoring, technical debt remediation, test suite changes) **should not** have a changelog entry. Example: "Reduce database records created during Cycle Analytics model spec." -- _Any_ contribution from a community member, no matter how small, **may** have - a changelog entry regardless of these guidelines if the contributor wants one. - Example: "Fixed a typo on the search results page." -- Performance improvements **should** have a changelog entry. -- Any change that introduces a database migration **must** have a - changelog entry. ## Writing good changelog entries diff --git a/lib/gitlab/auth/ip_rate_limiter.rb b/lib/gitlab/auth/ip_rate_limiter.rb index 0b7055b3256..74d359bcd28 100644 --- a/lib/gitlab/auth/ip_rate_limiter.rb +++ b/lib/gitlab/auth/ip_rate_limiter.rb @@ -24,6 +24,7 @@ module Gitlab # Allow2Ban.filter will return false if this IP has not failed too often yet @banned = Rack::Attack::Allow2Ban.filter(ip, config) do # If we return false here, the failure for this IP is ignored by Allow2Ban + # If we return true here, the count for the IP is incremented. ip_can_be_banned? end end diff --git a/lib/gitlab/tracking.rb b/lib/gitlab/tracking.rb index b167cb098b0..34eaf45aa75 100644 --- a/lib/gitlab/tracking.rb +++ b/lib/gitlab/tracking.rb @@ -6,6 +6,17 @@ module Gitlab module Tracking SNOWPLOW_NAMESPACE = 'gl' + module ControllerConcern + extend ActiveSupport::Concern + + protected + + def track_event(action = action_name, **args) + category = args.delete(:category) || self.class.name + Gitlab::Tracking.event(category, action.to_s, **args) + end + end + class << self def enabled? Gitlab::CurrentSettings.snowplow_enabled? diff --git a/locale/gitlab.pot b/locale/gitlab.pot index e66140789a2..c77c28e9fef 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1270,6 +1270,9 @@ msgstr "" msgid "All merge conflicts were resolved. The merge request can now be merged." msgstr "" +msgid "All paths are relative to the GitLab URL. Do not include %{relative_url_link_start}relative URL%{relative_url_link_end}." +msgstr "" + msgid "All projects" msgstr "" @@ -4038,6 +4041,9 @@ msgstr "" msgid "Configure limits for web and API requests." msgstr "" +msgid "Configure paths to be protected by Rack Attack. A web server restart is required after changing these settings." +msgstr "" + msgid "Configure push mirrors." msgstr "" @@ -5680,6 +5686,9 @@ msgstr "" msgid "Enable or disable version check and usage ping." msgstr "" +msgid "Enable protected paths rate limit" +msgstr "" + msgid "Enable proxy" msgstr "" @@ -8058,6 +8067,9 @@ msgstr "" msgid "Helps prevent bots from creating accounts." msgstr "" +msgid "Helps reduce request volume for protected paths" +msgstr "" + msgid "Hide archived projects" msgstr "" @@ -10706,6 +10718,9 @@ msgstr "" msgid "OmniAuth" msgstr "" +msgid "Omnibus Protected Paths throttle is active. From 12.4, Omnibus throttle is deprecated and will be removed in a future release. Please read the %{relative_url_link_start}Migrating Protected Paths documentation%{relative_url_link_end}." +msgstr "" + msgid "Onboarding" msgstr "" @@ -12571,6 +12586,9 @@ msgstr "" msgid "Protected Environments" msgstr "" +msgid "Protected Paths" +msgstr "" + msgid "Protected Tag" msgstr "" @@ -18958,6 +18976,9 @@ msgstr "" msgid "is not an email you own" msgstr "" +msgid "is too long (maximum is 100 entries)" +msgstr "" + msgid "is too long (maximum is 1000 entries)" msgstr "" diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 8c3642fb047..e87c5e59b3b 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -56,6 +56,8 @@ describe ApplicationController do end end + it_behaves_like 'a Trackable Controller' + describe '#add_gon_variables' do before do Gon.clear diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb index f9e83af352d..ef9daf70b0c 100644 --- a/spec/features/issues_spec.rb +++ b/spec/features/issues_spec.rb @@ -470,6 +470,8 @@ describe 'Issues' do expect(page).to have_content 'None' end + wait_for_requests + expect(issue.reload.assignees).to be_empty end diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index d12f9b9100a..84c25b93fc6 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -51,6 +51,10 @@ describe ApplicationSetting do it { is_expected.to allow_value(nil).for(:static_objects_external_storage_url) } it { is_expected.to allow_value(http).for(:static_objects_external_storage_url) } it { is_expected.to allow_value(https).for(:static_objects_external_storage_url) } + it { is_expected.to allow_value(['/example'] * 100).for(:protected_paths) } + it { is_expected.not_to allow_value(['/example'] * 101).for(:protected_paths) } + it { is_expected.not_to allow_value(nil).for(:protected_paths) } + it { is_expected.to allow_value([]).for(:protected_paths) } context "when user accepted let's encrypt terms of service" do before do diff --git a/spec/requests/rack_attack_global_spec.rb b/spec/requests/rack_attack_global_spec.rb index cf459ba99c1..0e757e8743a 100644 --- a/spec/requests/rack_attack_global_spec.rb +++ b/spec/requests/rack_attack_global_spec.rb @@ -12,7 +12,9 @@ describe 'Rack Attack global throttles' do throttle_authenticated_api_requests_per_period: 100, throttle_authenticated_api_period_in_seconds: 1, throttle_authenticated_web_requests_per_period: 100, - throttle_authenticated_web_period_in_seconds: 1 + throttle_authenticated_web_period_in_seconds: 1, + throttle_authenticated_protected_paths_request_per_period: 100, + throttle_authenticated_protected_paths_in_seconds: 1 } end @@ -35,6 +37,10 @@ describe 'Rack Attack global throttles' do let(:url_api_internal) { '/api/v4/internal/check' } before do + # Disabling protected paths throttle, otherwise requests to + # '/users/sign_in' are caught by this throttle. + settings_to_set[:throttle_protected_paths_enabled] = false + # Set low limits settings_to_set[:throttle_unauthenticated_requests_per_period] = requests_per_period settings_to_set[:throttle_unauthenticated_period_in_seconds] = period_in_seconds @@ -203,6 +209,159 @@ describe 'Rack Attack global throttles' do it_behaves_like 'rate-limited web authenticated requests' end + describe 'protected paths' do + context 'unauthenticated requests' do + let(:protected_path_that_does_not_require_authentication) do + '/users/confirmation' + end + + before do + settings_to_set[:throttle_protected_paths_requests_per_period] = requests_per_period # 1 + settings_to_set[:throttle_protected_paths_period_in_seconds] = period_in_seconds # 10_000 + end + + context 'when protected paths throttle is disabled' do + before do + settings_to_set[:throttle_protected_paths_enabled] = false + stub_application_setting(settings_to_set) + end + + it 'allows requests over the rate limit' do + (1 + requests_per_period).times do + get protected_path_that_does_not_require_authentication + expect(response).to have_http_status 200 + end + end + end + + context 'when protected paths throttle is enabled' do + before do + settings_to_set[:throttle_protected_paths_enabled] = true + stub_application_setting(settings_to_set) + end + + it 'rejects requests over the rate limit' do + requests_per_period.times do + get protected_path_that_does_not_require_authentication + expect(response).to have_http_status 200 + end + + expect_rejection { get protected_path_that_does_not_require_authentication } + end + + context 'when Omnibus throttle is present' do + before do + allow(Gitlab::Throttle) + .to receive(:omnibus_protected_paths_present?).and_return(true) + end + + it 'allows requests over the rate limit' do + (1 + requests_per_period).times do + get protected_path_that_does_not_require_authentication + expect(response).to have_http_status 200 + end + end + end + end + end + + context 'API requests authenticated with personal access token', :api do + let(:user) { create(:user) } + let(:token) { create(:personal_access_token, user: user) } + let(:other_user) { create(:user) } + let(:other_user_token) { create(:personal_access_token, user: other_user) } + let(:throttle_setting_prefix) { 'throttle_protected_paths' } + let(:api_partial_url) { '/users' } + + let(:protected_paths) do + [ + '/api/v4/users' + ] + end + + before do + settings_to_set[:protected_paths] = protected_paths + stub_application_setting(settings_to_set) + end + + context 'with the token in the query string' do + let(:get_args) { [api(api_partial_url, personal_access_token: token)] } + let(:other_user_get_args) { [api(api_partial_url, personal_access_token: other_user_token)] } + + it_behaves_like 'rate-limited token-authenticated requests' + end + + context 'with the token in the headers' do + let(:get_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(token)) } + let(:other_user_get_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(other_user_token)) } + + it_behaves_like 'rate-limited token-authenticated requests' + end + + context 'when Omnibus throttle is present' do + let(:get_args) { [api(api_partial_url, personal_access_token: token)] } + let(:other_user_get_args) { [api(api_partial_url, personal_access_token: other_user_token)] } + + before do + settings_to_set[:"#{throttle_setting_prefix}_requests_per_period"] = requests_per_period + settings_to_set[:"#{throttle_setting_prefix}_period_in_seconds"] = period_in_seconds + settings_to_set[:"#{throttle_setting_prefix}_enabled"] = true + stub_application_setting(settings_to_set) + + allow(Gitlab::Throttle) + .to receive(:omnibus_protected_paths_present?).and_return(true) + end + + it 'allows requests over the rate limit' do + (1 + requests_per_period).times do + get(*get_args) + expect(response).to have_http_status 200 + end + end + end + end + + describe 'web requests authenticated with regular login' do + let(:throttle_setting_prefix) { 'throttle_protected_paths' } + let(:user) { create(:user) } + let(:url_that_requires_authentication) { '/dashboard/snippets' } + + let(:protected_paths) do + [ + url_that_requires_authentication + ] + end + + before do + settings_to_set[:protected_paths] = protected_paths + stub_application_setting(settings_to_set) + end + + it_behaves_like 'rate-limited web authenticated requests' + + context 'when Omnibus throttle is present' do + before do + settings_to_set[:"#{throttle_setting_prefix}_requests_per_period"] = requests_per_period + settings_to_set[:"#{throttle_setting_prefix}_period_in_seconds"] = period_in_seconds + settings_to_set[:"#{throttle_setting_prefix}_enabled"] = true + stub_application_setting(settings_to_set) + + allow(Gitlab::Throttle) + .to receive(:omnibus_protected_paths_present?).and_return(true) + + login_as(user) + end + + it 'allows requests over the rate limit' do + (1 + requests_per_period).times do + get url_that_requires_authentication + expect(response).to have_http_status 200 + end + end + end + end + end + def api_get_args_with_token_headers(partial_url, token_headers) ["/api/#{API::API.version}#{partial_url}", params: nil, headers: token_headers] end diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb index 51fb43907a6..ea7ea02cee3 100644 --- a/spec/services/application_settings/update_service_spec.rb +++ b/spec/services/application_settings/update_service_spec.rb @@ -295,4 +295,26 @@ describe ApplicationSettings::UpdateService do expect(application_settings.raw_blob_request_limit).to eq(600) end end + + context 'when protected path settings are passed' do + let(:params) do + { + throttle_protected_paths_enabled: 1, + throttle_protected_paths_period_in_seconds: 600, + throttle_protected_paths_requests_per_period: 100, + protected_paths_raw: "/users/password\r\n/users/sign_in\r\n" + } + end + + it 'updates protected path settings' do + subject.execute + + application_settings.reload + + expect(application_settings.throttle_protected_paths_enabled).to be_truthy + expect(application_settings.throttle_protected_paths_period_in_seconds).to eq(600) + expect(application_settings.throttle_protected_paths_requests_per_period).to eq(100) + expect(application_settings.protected_paths).to eq(['/users/password', '/users/sign_in']) + end + end end diff --git a/spec/support/shared_examples/requests/rack_attack_shared_examples.rb b/spec/support/shared_examples/requests/rack_attack_shared_examples.rb index afc6f59b773..a2e38cfc60b 100644 --- a/spec/support/shared_examples/requests/rack_attack_shared_examples.rb +++ b/spec/support/shared_examples/requests/rack_attack_shared_examples.rb @@ -8,6 +8,14 @@ # * period_in_seconds # * period shared_examples_for 'rate-limited token-authenticated requests' do + let(:throttle_types) do + { + "throttle_protected_paths" => "throttle_authenticated_protected_paths_api", + "throttle_authenticated_api" => "throttle_authenticated_api", + "throttle_authenticated_web" => "throttle_authenticated_web" + } + end + before do # Set low limits settings_to_set[:"#{throttle_setting_prefix}_requests_per_period"] = requests_per_period @@ -84,7 +92,8 @@ shared_examples_for 'rate-limited token-authenticated requests' do request_method: 'GET', path: get_args.first, user_id: user.id, - username: user.username + username: user.username, + throttle_type: throttle_types[throttle_setting_prefix] } expect(Gitlab::AuthLogger).to receive(:error).with(arguments).once @@ -116,6 +125,13 @@ end # * period_in_seconds # * period shared_examples_for 'rate-limited web authenticated requests' do + let(:throttle_types) do + { + "throttle_protected_paths" => "throttle_authenticated_protected_paths_web", + "throttle_authenticated_web" => "throttle_authenticated_web" + } + end + before do login_as(user) @@ -196,7 +212,8 @@ shared_examples_for 'rate-limited web authenticated requests' do request_method: 'GET', path: '/dashboard/snippets', user_id: user.id, - username: user.username + username: user.username, + throttle_type: throttle_types[throttle_setting_prefix] } expect(Gitlab::AuthLogger).to receive(:error).with(arguments).once diff --git a/spec/support/shared_examples/trackable_shared_examples.rb b/spec/support/shared_examples/trackable_shared_examples.rb new file mode 100644 index 00000000000..6ad75a14d6b --- /dev/null +++ b/spec/support/shared_examples/trackable_shared_examples.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +shared_examples 'a Trackable Controller' do + describe '#track_event' do + before do + sign_in user + end + + context 'with no params' do + controller(described_class) do + def index + track_event + head :ok + end + end + + it 'tracks the action name' do + expect(Gitlab::Tracking).to receive(:event).with('AnonymousController', 'index', {}) + get :index + end + end + + context 'with params' do + controller(described_class) do + def index + track_event('some_event', category: 'SomeCategory', label: 'errorlabel') + head :ok + end + end + + it 'tracks with the specified param' do + expect(Gitlab::Tracking).to receive(:event).with('SomeCategory', 'some_event', label: 'errorlabel') + get :index + end + end + end +end |