diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2019-11-26 11:09:19 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2019-11-26 11:09:19 +0000 |
commit | 3d7b56c54620b0ff31a3a455144bf4fef2377c17 (patch) | |
tree | b8ebaabc569670a9f561fc7c5c762e8ec9cf4487 | |
parent | 4d477238500c347c6553d335d920bedfc5a46869 (diff) | |
download | gitlab-ce-3d7b56c54620b0ff31a3a455144bf4fef2377c17.tar.gz |
Add latest changes from gitlab-org/gitlab@12-4-stable-ee
20 files changed, 218 insertions, 128 deletions
diff --git a/CHANGELOG-EE.md b/CHANGELOG-EE.md index b6156f8d254..b0e0af8e359 100644 --- a/CHANGELOG-EE.md +++ b/CHANGELOG-EE.md @@ -1,5 +1,13 @@ Please view this file on the master branch, on stable branches it's out of date. +## 12.4.3 + +### Fixed (2 changes) + +- Fix admin welcome image not found. !19676 +- Revert ES support for public/internal project snippets. !19715 + + ## 12.4.2 ### Fixed (1 change) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4de0606589f..5db49b71961 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,11 @@ entry. ## 12.4.3 -- No changes. +### Fixed (2 changes) + +- Only enable protected paths for POST requests. !19184 +- Fix Bitbucket Cloud importer pull request state. !19734 + ## 12.4.2 @@ -1 +1 @@ -12.4.3 +12.4.3-ee diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 852576dbbc2..796e6438a2c 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -137,6 +137,26 @@ module Issuable strip_attributes :title + # The state_machine gem will reset the value of state_id unless it + # is a raw attribute passed in here: + # https://gitlab.com/gitlab-org/gitlab/issues/35746#note_241148787 + # + # This assumes another initialize isn't defined. Otherwise this + # method may need to be prepended. + def initialize(attributes = nil) + if attributes.is_a?(Hash) + attr = attributes.symbolize_keys + + if attr.key?(:state) && !attr.key?(:state_id) + value = attr.delete(:state) + state_id = self.class.available_states[value] + attributes[:state_id] = state_id if state_id + end + end + + super(attributes) + end + # We want to use optimistic lock for cases when only title or description are involved # http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html def locking_enabled? diff --git a/config/initializers/rack_attack_new.rb b/config/initializers/rack_attack_new.rb index b0f7febe427..50f9d7971cc 100644 --- a/config/initializers/rack_attack_new.rb +++ b/config/initializers/rack_attack_new.rb @@ -59,7 +59,8 @@ class Rack::Attack end throttle('throttle_unauthenticated_protected_paths', Gitlab::Throttle.protected_paths_options) do |req| - Gitlab::Throttle.protected_paths_enabled? && + req.post? && + Gitlab::Throttle.protected_paths_enabled? && req.unauthenticated? && !req.should_be_skipped? && req.protected_path? && @@ -67,14 +68,16 @@ class Rack::Attack end throttle('throttle_authenticated_protected_paths_api', Gitlab::Throttle.protected_paths_options) do |req| - Gitlab::Throttle.protected_paths_enabled? && + req.post? && + 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.post? && + Gitlab::Throttle.protected_paths_enabled? && req.web_request? && req.protected_path? && req.authenticated_user_id([:api, :rss, :ics]) diff --git a/doc/topics/git/partial_clone.md b/doc/topics/git/partial_clone.md index ce1b551ddb6..e6f84ee8251 100644 --- a/doc/topics/git/partial_clone.md +++ b/doc/topics/git/partial_clone.md @@ -39,16 +39,20 @@ Follow [Git for enormous repositories](https://gitlab.com/groups/gitlab-org/-/ep ## Enabling partial clone -GitLab 12.1 uses Git 2.21.0 which has an arbitrary file access security -vulnerability when `uploadpack.allowFilter` is enabled, and should not be -enabled in production environments. +> [Introduced](https://gitlab.com/gitlab-org/gitaly/issues/1553) in GitLab 12.4. -A feature flag is planned to enable `uploadpack.allowFilter` and -`uploadpack.allowAnySHA1InWant` once the version of Git used by GitLab has been -updated to Git 2.22.0. +To enable partial clone, use the [feature flags API](../../api/features.md). +For example: -Follow [this issue](https://gitlab.com/gitlab-org/gitaly/issues/1553) for -updated. +```sh +curl --data "value=true" --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/features/gitaly_upload_pack_filter +``` + +Alternatively, flip the switch and enable the feature flag: + +```ruby +Feature.enable(:gitaly_upload_pack_filter) +``` ## Excluding objects by size diff --git a/doc/user/application_security/security_dashboard/index.md b/doc/user/application_security/security_dashboard/index.md index 0e26206f070..17f63577f0c 100644 --- a/doc/user/application_security/security_dashboard/index.md +++ b/doc/user/application_security/security_dashboard/index.md @@ -71,7 +71,7 @@ Once you're on the dashboard, at the top you should see a series of filters for: - Report type - Project -To the right of the filters, you should see a **Hide dismissed** toggle button. +To the right of the filters, you should see a **Hide dismissed** toggle button ([available in GitLab Ultimate 12.5](https://gitlab.com/gitlab-org/gitlab/issues/9102)). NOTE: **Note:** The dashboard only shows projects with [security reports](#supported-reports) enabled in a group. diff --git a/doc/user/packages/conan_repository/index.md b/doc/user/packages/conan_repository/index.md index f81756f7979..953c7472f4d 100644 --- a/doc/user/packages/conan_repository/index.md +++ b/doc/user/packages/conan_repository/index.md @@ -1,6 +1,6 @@ # GitLab Conan Repository **(PREMIUM)** -> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/issues/8248) in [GitLab Premium](https://about.gitlab.com/pricing/) 12.4. +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/issues/8248) in [GitLab Premium](https://about.gitlab.com/pricing/) 12.5. With the GitLab Conan Repository, every project can have its own space to store Conan packages. diff --git a/spec/controllers/projects/tags_controller_spec.rb b/spec/controllers/projects/tags_controller_spec.rb index f077b4c99fc..7e5237facf6 100644 --- a/spec/controllers/projects/tags_controller_spec.rb +++ b/spec/controllers/projects/tags_controller_spec.rb @@ -13,7 +13,7 @@ describe Projects::TagsController do end it 'returns the tags for the page' do - expect(assigns(:tags).map(&:name)).to eq(['v1.1.0', 'v1.0.0']) + expect(assigns(:tags).map(&:name)).to include('v1.1.0', 'v1.0.0') end it 'returns releases matching those tags' do diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index e0df9556eb8..f84978024ec 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -837,8 +837,7 @@ describe ProjectsController do get :refs, params: { namespace_id: project.namespace, id: project, sort: 'updated_desc' } expect(json_response['Branches']).to include('master') - expect(json_response['Tags'].first).to eq('v1.1.0') - expect(json_response['Tags'].last).to eq('v1.0.0') + expect(json_response['Tags']).to include('v1.0.0') expect(json_response['Commits']).to be_nil end diff --git a/spec/features/projects/tags/user_edits_tags_spec.rb b/spec/features/projects/tags/user_edits_tags_spec.rb index 63f97eeb4e0..b1cb7685f63 100644 --- a/spec/features/projects/tags/user_edits_tags_spec.rb +++ b/spec/features/projects/tags/user_edits_tags_spec.rb @@ -21,23 +21,21 @@ describe 'Project > Tags', :js do context 'page with tags list' do it 'shows tag name' do - page.within first('.tags > .content-list > li') do - expect(page.find('.row-main-content')).to have_content 'v1.1.0 Version 1.1.0' - end + expect(page).to have_content 'v1.1.0 Version 1.1.0' end it 'shows tag edit button' do - page.within first('.tags > .content-list > li') do - edit_btn = page.find('.row-fixed-content.controls a.btn-edit') + page.within '.tags > .content-list' do + edit_btn = page.find("li > .row-fixed-content.controls a.btn-edit[href='/#{project.full_path}/-/tags/v1.1.0/release/edit']") - expect(edit_btn['href']).to have_content '/tags/v1.1.0/release/edit' + expect(edit_btn['href']).to end_with("/#{project.full_path}/-/tags/v1.1.0/release/edit") end end end context 'edit tag release notes' do before do - find('.tags > .content-list > li:first-child .row-fixed-content.controls a.btn-edit').click + page.find("li > .row-fixed-content.controls a.btn-edit[href='/#{project.full_path}/-/tags/v1.1.0/release/edit']").click end it 'shows tag name header' do diff --git a/spec/features/tags/developer_deletes_tag_spec.rb b/spec/features/tags/developer_deletes_tag_spec.rb index 82b416c3a7f..165f5ca0a78 100644 --- a/spec/features/tags/developer_deletes_tag_spec.rb +++ b/spec/features/tags/developer_deletes_tag_spec.rb @@ -17,7 +17,7 @@ describe 'Developer deletes tag' do it 'deletes the tag' do expect(page).to have_content 'v1.1.0' - delete_first_tag + delete_tag 'v1.1.0' expect(page).not_to have_content 'v1.1.0' end @@ -44,15 +44,15 @@ describe 'Developer deletes tag' do end it 'shows the error message' do - delete_first_tag + delete_tag 'v1.1.0' expect(page).to have_content('Do not delete tags') end end - def delete_first_tag + def delete_tag(tag) page.within('.content') do - accept_confirm { first('.btn-remove').click } + accept_confirm { find("li > .row-fixed-content.controls a.btn-remove[href='/#{project.full_path}/-/tags/#{tag}']").click } end end end diff --git a/spec/features/tags/developer_updates_tag_spec.rb b/spec/features/tags/developer_updates_tag_spec.rb index 0cdd953b9ae..167079c3f31 100644 --- a/spec/features/tags/developer_updates_tag_spec.rb +++ b/spec/features/tags/developer_updates_tag_spec.rb @@ -15,9 +15,7 @@ describe 'Developer updates tag' do context 'from the tags list page' do it 'updates the release notes' do - page.within(first('.content-list .controls')) do - click_link 'Edit release notes' - end + find("li > .row-fixed-content.controls a.btn-edit[href='/#{project.full_path}/-/tags/v1.1.0/release/edit']").click fill_in 'release_description', with: 'Awesome release notes' click_button 'Save changes' diff --git a/spec/finders/tags_finder_spec.rb b/spec/finders/tags_finder_spec.rb index 85f970b71c4..e2224d3dd60 100644 --- a/spec/finders/tags_finder_spec.rb +++ b/spec/finders/tags_finder_spec.rb @@ -57,24 +57,25 @@ describe TagsFinder do end context 'filter and sort' do - it 'filters tags by name and sorts by recently_updated' do - params = { sort: 'updated_desc', search: 'v1' } - tags_finder = described_class.new(repository, params) + let(:tags_to_compare) { %w[v1.0.0 v1.1.0] } + subject { described_class.new(repository, params).execute.select { |tag| tags_to_compare.include?(tag.name) } } - result = tags_finder.execute + context 'when sort by updated_desc' do + let(:params) { { sort: 'updated_desc', search: 'v1' } } - expect(result.first.name).to eq('v1.1.0') - expect(result.count).to eq(2) + it 'filters tags by name' do + expect(subject.first.name).to eq('v1.1.0') + expect(subject.count).to eq(2) + end end - it 'filters tags by name and sorts by last_updated' do - params = { sort: 'updated_asc', search: 'v1' } - tags_finder = described_class.new(repository, params) - - result = tags_finder.execute + context 'when sort by updated_asc' do + let(:params) { { sort: 'updated_asc', search: 'v1' } } - expect(result.first.name).to eq('v1.0.0') - expect(result.count).to eq(2) + it 'filters tags by name' do + expect(subject.first.name).to eq('v1.0.0') + expect(subject.count).to eq(2) + end end end end diff --git a/spec/lib/gitlab/bitbucket_import/importer_spec.rb b/spec/lib/gitlab/bitbucket_import/importer_spec.rb index 7f7a285c453..b0d07c6e0b0 100644 --- a/spec/lib/gitlab/bitbucket_import/importer_spec.rb +++ b/spec/lib/gitlab/bitbucket_import/importer_spec.rb @@ -158,6 +158,7 @@ describe Gitlab::BitbucketImport::Importer do expect { subject.execute }.to change { MergeRequest.count }.by(1) merge_request = MergeRequest.first + expect(merge_request.state).to eq('merged') expect(merge_request.notes.count).to eq(2) expect(merge_request.notes.map(&:discussion_id).uniq.count).to eq(1) diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index e8116f0a301..f7bef9e71e2 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -111,6 +111,34 @@ describe Issuable do end end + describe '.initialize' do + it 'maps the state to the right state_id' do + described_class::STATE_ID_MAP.each do |key, value| + issuable = MergeRequest.new(state: key) + + expect(issuable.state).to eq(key) + expect(issuable.state_id).to eq(value) + end + end + + it 'maps a string version of the state to the right state_id' do + described_class::STATE_ID_MAP.each do |key, value| + issuable = MergeRequest.new('state' => key) + + expect(issuable.state).to eq(key) + expect(issuable.state_id).to eq(value) + end + end + + it 'gives preference to state_id if present' do + issuable = MergeRequest.new('state' => 'opened', + 'state_id' => described_class::STATE_ID_MAP['merged']) + + expect(issuable.state).to eq('merged') + expect(issuable.state_id).to eq(described_class::STATE_ID_MAP['merged']) + end + end + describe '#milestone_available?' do let(:group) { create(:group) } let(:project) { create(:project, group: group) } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index cf9100eb6cf..9e300418ade 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -66,14 +66,16 @@ describe Repository do end describe 'tags_sorted_by' do + let(:tags_to_compare) { %w[v1.0.0 v1.1.0] } + context 'name_desc' do - subject { repository.tags_sorted_by('name_desc').map(&:name) } + subject { repository.tags_sorted_by('name_desc').map(&:name) & tags_to_compare } it { is_expected.to eq(['v1.1.0', 'v1.0.0']) } end context 'name_asc' do - subject { repository.tags_sorted_by('name_asc').map(&:name) } + subject { repository.tags_sorted_by('name_asc').map(&:name) & tags_to_compare } it { is_expected.to eq(['v1.0.0', 'v1.1.0']) } end @@ -115,7 +117,7 @@ describe Repository do context 'annotated tag pointing to a blob' do let(:annotated_tag_name) { 'annotated-tag' } - subject { repository.tags_sorted_by('updated_asc').map(&:name) } + subject { repository.tags_sorted_by('updated_asc').map(&:name) & (tags_to_compare + [annotated_tag_name]) } before do options = { message: 'test tag message\n', diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb index c4f4a2cb889..9d9757ec2e7 100644 --- a/spec/requests/api/tags_spec.rb +++ b/spec/requests/api/tags_spec.rb @@ -5,6 +5,7 @@ describe API::Tags do let(:guest) { create(:user).tap { |u| project.add_guest(u) } } let(:project) { create(:project, :repository, creator: user, path: 'my.project') } let(:tag_name) { project.repository.find_tag('v1.1.0').name } + let(:tag_message) { project.repository.find_tag('v1.1.0').message } let(:project_id) { project.id } let(:current_user) { nil } @@ -73,7 +74,7 @@ describe API::Tags do expect(response).to have_gitlab_http_status(200) expect(response).to match_response_schema('public_api/v4/tags') expect(response).to include_pagination_headers - expect(json_response.first['name']).to eq(tag_name) + expect(json_response.map { |r| r['name'] }).to include(tag_name) end context 'when repository is disabled' do @@ -133,9 +134,10 @@ describe API::Tags do expect(response).to have_gitlab_http_status(200) expect(response).to match_response_schema('public_api/v4/tags') expect(response).to include_pagination_headers - expect(json_response.first['name']).to eq(tag_name) - expect(json_response.first['message']).to eq('Version 1.1.0') - expect(json_response.first['release']['description']).to eq(description) + + expected_tag = json_response.find { |r| r['name'] == tag_name } + expect(expected_tag['message']).to eq(tag_message) + expect(expected_tag['release']['description']).to eq(description) end end end diff --git a/spec/requests/rack_attack_global_spec.rb b/spec/requests/rack_attack_global_spec.rb index ca8720cd414..8d594ae2630 100644 --- a/spec/requests/rack_attack_global_spec.rb +++ b/spec/requests/rack_attack_global_spec.rb @@ -20,6 +20,7 @@ describe 'Rack Attack global throttles' do } end + let(:request_method) { 'GET' } let(:requests_per_period) { 1 } let(:period_in_seconds) { 10000 } let(:period) { period_in_seconds.seconds } @@ -141,15 +142,15 @@ describe 'Rack Attack global throttles' do let(:api_partial_url) { '/todos' } 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)] } + let(:request_args) { [api(api_partial_url, personal_access_token: token)] } + let(:other_user_request_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)) } + let(:request_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(token)) } + let(:other_user_request_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 @@ -168,15 +169,15 @@ describe 'Rack Attack global throttles' do let(:api_partial_url) { '/todos' } context 'with the token in the query string' do - let(:get_args) { [api(api_partial_url, oauth_access_token: token)] } - let(:other_user_get_args) { [api(api_partial_url, oauth_access_token: other_user_token)] } + let(:request_args) { [api(api_partial_url, oauth_access_token: token)] } + let(:other_user_request_args) { [api(api_partial_url, oauth_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, oauth_token_headers(token)) } - let(:other_user_get_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(other_user_token)) } + let(:request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(token)) } + let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(other_user_token)) } it_behaves_like 'rate-limited token-authenticated requests' end @@ -188,8 +189,8 @@ describe 'Rack Attack global throttles' do let(:throttle_setting_prefix) { 'throttle_authenticated_web' } context 'with the token in the query string' do - let(:get_args) { [rss_url(user), params: nil] } - let(:other_user_get_args) { [rss_url(other_user), params: nil] } + let(:request_args) { [rss_url(user), params: nil] } + let(:other_user_request_args) { [rss_url(other_user), params: nil] } it_behaves_like 'rate-limited token-authenticated requests' end @@ -204,10 +205,13 @@ describe 'Rack Attack global throttles' do end describe 'protected paths' do + let(:request_method) { 'POST' } + context 'unauthenticated requests' do let(:protected_path_that_does_not_require_authentication) do - '/users/confirmation' + '/users/sign_in' end + let(:post_params) { { user: { login: 'username', password: 'password' } } } before do settings_to_set[:throttle_protected_paths_requests_per_period] = requests_per_period # 1 @@ -222,7 +226,7 @@ describe 'Rack Attack global throttles' do it 'allows requests over the rate limit' do (1 + requests_per_period).times do - get protected_path_that_does_not_require_authentication + post protected_path_that_does_not_require_authentication, params: post_params expect(response).to have_http_status 200 end end @@ -236,11 +240,11 @@ describe 'Rack Attack global throttles' do it 'rejects requests over the rate limit' do requests_per_period.times do - get protected_path_that_does_not_require_authentication + post protected_path_that_does_not_require_authentication, params: post_params expect(response).to have_http_status 200 end - expect_rejection { get protected_path_that_does_not_require_authentication } + expect_rejection { post protected_path_that_does_not_require_authentication, params: post_params } end context 'when Omnibus throttle is present' do @@ -251,7 +255,7 @@ describe 'Rack Attack global throttles' do it 'allows requests over the rate limit' do (1 + requests_per_period).times do - get protected_path_that_does_not_require_authentication + post protected_path_that_does_not_require_authentication, params: post_params expect(response).to have_http_status 200 end end @@ -265,11 +269,11 @@ describe 'Rack Attack global throttles' do 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(:api_partial_url) { '/user/emails' } let(:protected_paths) do [ - '/api/v4/users' + '/api/v4/user/emails' ] end @@ -279,22 +283,22 @@ describe 'Rack Attack global throttles' do 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)] } + let(:request_args) { [api(api_partial_url, personal_access_token: token)] } + let(:other_user_request_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)) } + let(:request_args) { api_get_args_with_token_headers(api_partial_url, personal_access_token_headers(token)) } + let(:other_user_request_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)] } + let(:request_args) { [api(api_partial_url, personal_access_token: token)] } + let(:other_user_request_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 @@ -308,8 +312,8 @@ describe 'Rack Attack global throttles' do it 'allows requests over the rate limit' do (1 + requests_per_period).times do - get(*get_args) - expect(response).to have_http_status 200 + post(*request_args) + expect(response).not_to have_http_status 429 end end end @@ -318,7 +322,7 @@ describe 'Rack Attack global throttles' do 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(:url_that_requires_authentication) { '/users/confirmation' } let(:protected_paths) do [ @@ -348,8 +352,8 @@ describe 'Rack Attack global throttles' do 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 + post url_that_requires_authentication + expect(response).not_to have_http_status 429 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 a2e38cfc60b..934889597ba 100644 --- a/spec/support/shared_examples/requests/rack_attack_shared_examples.rb +++ b/spec/support/shared_examples/requests/rack_attack_shared_examples.rb @@ -2,8 +2,9 @@ # # Requires let variables: # * throttle_setting_prefix: "throttle_authenticated_api", "throttle_authenticated_web", "throttle_protected_paths" -# * get_args -# * other_user_get_args +# * request_method +# * request_args +# * other_user_request_args # * requests_per_period # * period_in_seconds # * period @@ -31,66 +32,66 @@ shared_examples_for 'rate-limited token-authenticated requests' do it 'rejects requests over the rate limit' do # At first, allow requests under the rate limit. requests_per_period.times do - get(*get_args) - expect(response).to have_http_status 200 + make_request(request_args) + expect(response).not_to have_http_status 429 end # the last straw - expect_rejection { get(*get_args) } + expect_rejection { make_request(request_args) } end it 'allows requests after throttling and then waiting for the next period' do requests_per_period.times do - get(*get_args) - expect(response).to have_http_status 200 + make_request(request_args) + expect(response).not_to have_http_status 429 end - expect_rejection { get(*get_args) } + expect_rejection { make_request(request_args) } Timecop.travel(period.from_now) do requests_per_period.times do - get(*get_args) - expect(response).to have_http_status 200 + make_request(request_args) + expect(response).not_to have_http_status 429 end - expect_rejection { get(*get_args) } + expect_rejection { make_request(request_args) } end end it 'counts requests from different users separately, even from the same IP' do requests_per_period.times do - get(*get_args) - expect(response).to have_http_status 200 + make_request(request_args) + expect(response).not_to have_http_status 429 end # would be over the limit if this wasn't a different user - get(*other_user_get_args) - expect(response).to have_http_status 200 + make_request(other_user_request_args) + expect(response).not_to have_http_status 429 end it 'counts all requests from the same user, even via different IPs' do requests_per_period.times do - get(*get_args) - expect(response).to have_http_status 200 + make_request(request_args) + expect(response).not_to have_http_status 429 end expect_any_instance_of(Rack::Attack::Request).to receive(:ip).and_return('1.2.3.4') - expect_rejection { get(*get_args) } + expect_rejection { make_request(request_args) } end it 'logs RackAttack info into structured logs' do requests_per_period.times do - get(*get_args) - expect(response).to have_http_status 200 + make_request(request_args) + expect(response).not_to have_http_status 429 end arguments = { message: 'Rack_Attack', env: :throttle, remote_ip: '127.0.0.1', - request_method: 'GET', - path: get_args.first, + request_method: request_method, + path: request_args.first, user_id: user.id, username: user.username, throttle_type: throttle_types[throttle_setting_prefix] @@ -98,7 +99,7 @@ shared_examples_for 'rate-limited token-authenticated requests' do expect(Gitlab::AuthLogger).to receive(:error).with(arguments).once - expect_rejection { get(*get_args) } + expect_rejection { make_request(request_args) } end end @@ -110,17 +111,26 @@ shared_examples_for 'rate-limited token-authenticated requests' do it 'allows requests over the rate limit' do (1 + requests_per_period).times do - get(*get_args) - expect(response).to have_http_status 200 + make_request(request_args) + expect(response).not_to have_http_status 429 end end end + + def make_request(args) + if request_method == 'POST' + post(*args) + else + get(*args) + end + end end # Requires let variables: # * throttle_setting_prefix: "throttle_authenticated_web" or "throttle_protected_paths" # * user # * url_that_requires_authentication +# * request_method # * requests_per_period # * period_in_seconds # * period @@ -149,68 +159,68 @@ shared_examples_for 'rate-limited web authenticated requests' do it 'rejects requests over the rate limit' do # At first, allow requests under the rate limit. requests_per_period.times do - get url_that_requires_authentication - expect(response).to have_http_status 200 + request_authenticated_web_url + expect(response).not_to have_http_status 429 end # the last straw - expect_rejection { get url_that_requires_authentication } + expect_rejection { request_authenticated_web_url } end it 'allows requests after throttling and then waiting for the next period' do requests_per_period.times do - get url_that_requires_authentication - expect(response).to have_http_status 200 + request_authenticated_web_url + expect(response).not_to have_http_status 429 end - expect_rejection { get url_that_requires_authentication } + expect_rejection { request_authenticated_web_url } Timecop.travel(period.from_now) do requests_per_period.times do - get url_that_requires_authentication - expect(response).to have_http_status 200 + request_authenticated_web_url + expect(response).not_to have_http_status 429 end - expect_rejection { get url_that_requires_authentication } + expect_rejection { request_authenticated_web_url } end end it 'counts requests from different users separately, even from the same IP' do requests_per_period.times do - get url_that_requires_authentication - expect(response).to have_http_status 200 + request_authenticated_web_url + expect(response).not_to have_http_status 429 end # would be over the limit if this wasn't a different user login_as(create(:user)) - get url_that_requires_authentication - expect(response).to have_http_status 200 + request_authenticated_web_url + expect(response).not_to have_http_status 429 end it 'counts all requests from the same user, even via different IPs' do requests_per_period.times do - get url_that_requires_authentication - expect(response).to have_http_status 200 + request_authenticated_web_url + expect(response).not_to have_http_status 429 end expect_any_instance_of(Rack::Attack::Request).to receive(:ip).and_return('1.2.3.4') - expect_rejection { get url_that_requires_authentication } + expect_rejection { request_authenticated_web_url } end it 'logs RackAttack info into structured logs' do requests_per_period.times do - get url_that_requires_authentication - expect(response).to have_http_status 200 + request_authenticated_web_url + expect(response).not_to have_http_status 429 end arguments = { message: 'Rack_Attack', env: :throttle, remote_ip: '127.0.0.1', - request_method: 'GET', - path: '/dashboard/snippets', + request_method: request_method, + path: url_that_requires_authentication, user_id: user.id, username: user.username, throttle_type: throttle_types[throttle_setting_prefix] @@ -218,7 +228,7 @@ shared_examples_for 'rate-limited web authenticated requests' do expect(Gitlab::AuthLogger).to receive(:error).with(arguments).once - get url_that_requires_authentication + request_authenticated_web_url end end @@ -230,9 +240,17 @@ shared_examples_for 'rate-limited web authenticated requests' do 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 + request_authenticated_web_url + expect(response).not_to have_http_status 429 end end end + + def request_authenticated_web_url + if request_method == 'POST' + post url_that_requires_authentication + else + get url_that_requires_authentication + end + end end |