summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2019-11-26 11:09:19 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2019-11-26 11:09:19 +0000
commit3d7b56c54620b0ff31a3a455144bf4fef2377c17 (patch)
treeb8ebaabc569670a9f561fc7c5c762e8ec9cf4487
parent4d477238500c347c6553d335d920bedfc5a46869 (diff)
downloadgitlab-ce-3d7b56c54620b0ff31a3a455144bf4fef2377c17.tar.gz
Add latest changes from gitlab-org/gitlab@12-4-stable-ee
-rw-r--r--CHANGELOG-EE.md8
-rw-r--r--CHANGELOG.md6
-rw-r--r--VERSION2
-rw-r--r--app/models/concerns/issuable.rb20
-rw-r--r--config/initializers/rack_attack_new.rb9
-rw-r--r--doc/topics/git/partial_clone.md20
-rw-r--r--doc/user/application_security/security_dashboard/index.md2
-rw-r--r--doc/user/packages/conan_repository/index.md2
-rw-r--r--spec/controllers/projects/tags_controller_spec.rb2
-rw-r--r--spec/controllers/projects_controller_spec.rb3
-rw-r--r--spec/features/projects/tags/user_edits_tags_spec.rb12
-rw-r--r--spec/features/tags/developer_deletes_tag_spec.rb8
-rw-r--r--spec/features/tags/developer_updates_tag_spec.rb4
-rw-r--r--spec/finders/tags_finder_spec.rb27
-rw-r--r--spec/lib/gitlab/bitbucket_import/importer_spec.rb1
-rw-r--r--spec/models/concerns/issuable_spec.rb28
-rw-r--r--spec/models/repository_spec.rb8
-rw-r--r--spec/requests/api/tags_spec.rb10
-rw-r--r--spec/requests/rack_attack_global_spec.rb60
-rw-r--r--spec/support/shared_examples/requests/rack_attack_shared_examples.rb114
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
diff --git a/VERSION b/VERSION
index 9f3bdf87a9c..2793e50eba5 100644
--- a/VERSION
+++ b/VERSION
@@ -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