Commit aba62427 authored by GitLab Bot's avatar GitLab Bot

Automatic merge of gitlab-org/gitlab master

parents d6a3752f 33656537
...@@ -107,7 +107,7 @@ export default { ...@@ -107,7 +107,7 @@ export default {
<td v-if="error" class="js-error-lazy-load-diff diff-loading-error-block"> <td v-if="error" class="js-error-lazy-load-diff diff-loading-error-block">
{{ __('Unable to load the diff') }} {{ __('Unable to load the diff') }}
<button <button
class="btn-link btn-link-retry btn-no-padding js-toggle-lazy-diff-retry-button" class="btn-link btn-link-retry gl-p-0 js-toggle-lazy-diff-retry-button"
@click="fetchDiff" @click="fetchDiff"
> >
{{ __('Try again') }} {{ __('Try again') }}
......
<script> <script>
import { GlLink } from '@gitlab/ui'; import { GlBadge, GlLink } from '@gitlab/ui';
export default { export default {
name: 'AccessibilityIssueBody', name: 'AccessibilityIssueBody',
components: { components: {
GlBadge,
GlLink, GlLink,
}, },
props: { props: {
...@@ -38,9 +39,9 @@ export default { ...@@ -38,9 +39,9 @@ export default {
<template> <template>
<div class="report-block-list-issue-description gl-mt-2 gl-mb-2"> <div class="report-block-list-issue-description gl-mt-2 gl-mb-2">
<div ref="accessibility-issue-description" class="report-block-list-issue-description-text"> <div ref="accessibility-issue-description" class="report-block-list-issue-description-text">
<div v-if="isNew" ref="accessibility-issue-is-new-badge" class="badge badge-danger gl-mr-2"> <gl-badge v-if="isNew" class="gl-mr-2" variant="danger">{{
{{ s__('AccessibilityReport|New') }} s__('AccessibilityReport|New')
</div> }}</gl-badge>
<div> <div>
{{ {{
sprintf( sprintf(
......
...@@ -158,12 +158,6 @@ ...@@ -158,12 +158,6 @@
line-height: $gl-btn-small-line-height; line-height: $gl-btn-small-line-height;
} }
&.btn-xs {
padding: 2px $gl-btn-padding;
font-size: $gl-btn-xs-font-size;
line-height: $gl-btn-xs-line-height;
}
&.btn-success { &.btn-success {
@include btn-green; @include btn-green;
} }
...@@ -438,10 +432,6 @@ fieldset[disabled] .btn, ...@@ -438,10 +432,6 @@ fieldset[disabled] .btn,
cursor: default; cursor: default;
} }
.btn-no-padding {
padding: 0;
}
// This class helps convert `.gl-button` children so that they consistently // This class helps convert `.gl-button` children so that they consistently
// match the style of `.btn` elements which might be around them. Ideally we // match the style of `.btn` elements which might be around them. Ideally we
// wouldn't need this class. // wouldn't need this class.
......
...@@ -589,8 +589,6 @@ $gl-btn-vert-padding: 8px; ...@@ -589,8 +589,6 @@ $gl-btn-vert-padding: 8px;
$gl-btn-horz-padding: 12px; $gl-btn-horz-padding: 12px;
$gl-btn-small-font-size: 13px; $gl-btn-small-font-size: 13px;
$gl-btn-small-line-height: 18px; $gl-btn-small-line-height: 18px;
$gl-btn-xs-font-size: 13px;
$gl-btn-xs-line-height: 13px;
/* /*
* Badges * Badges
......
...@@ -29,7 +29,7 @@ ...@@ -29,7 +29,7 @@
%td.line_content.js-success-lazy-load %td.line_content.js-success-lazy-load
.js-code-placeholder .js-code-placeholder
%td.js-error-lazy-load-diff.hidden.diff-loading-error-block %td.js-error-lazy-load-diff.hidden.diff-loading-error-block
- button = button_tag(_("Try again"), class: "btn-link gl-button btn-link-retry btn-no-padding js-toggle-lazy-diff-retry-button") - button = button_tag(_("Try again"), class: "btn-link gl-button btn-link-retry gl-p-0 js-toggle-lazy-diff-retry-button")
= _("Unable to load the diff. %{button_try_again}").html_safe % { button_try_again: button} = _("Unable to load the diff. %{button_try_again}").html_safe % { button_try_again: button}
= render "discussions/diff_discussion", discussions: [discussion], expanded: true = render "discussions/diff_discussion", discussions: [discussion], expanded: true
- else - else
......
--- ---
name: github_importer_use_diff_note_with_suggestions name: rate_limit_frontend_requests
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71765 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/79341
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/344309 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/350623
milestone: '14.5' milestone: '14.8'
type: development type: development
group: group::import group: group::integrations
default_enabled: true default_enabled: false
...@@ -22,6 +22,10 @@ NOTE: ...@@ -22,6 +22,10 @@ NOTE:
By default, all Git operations are first tried unauthenticated. Because of this, HTTP Git operations By default, all Git operations are first tried unauthenticated. Because of this, HTTP Git operations
may trigger the rate limits configured for unauthenticated requests. may trigger the rate limits configured for unauthenticated requests.
NOTE:
The rate limits for API requests don't affect requests made by the frontend, as these are always
counted as web traffic.
## Enable unauthenticated API request rate limit ## Enable unauthenticated API request rate limit
To enable the unauthenticated request rate limit: To enable the unauthenticated request rate limit:
......
...@@ -26,7 +26,7 @@ The following aspects of a project are imported: ...@@ -26,7 +26,7 @@ The following aspects of a project are imported:
- Regular issue and pull request comments - Regular issue and pull request comments
- [Git Large File Storage (LFS) Objects](../../../topics/git/lfs/index.md) - [Git Large File Storage (LFS) Objects](../../../topics/git/lfs/index.md)
- Pull request comments replies in discussions ([GitLab.com & 14.5+](https://gitlab.com/gitlab-org/gitlab/-/issues/336596)) - Pull request comments replies in discussions ([GitLab.com & 14.5+](https://gitlab.com/gitlab-org/gitlab/-/issues/336596))
- Diff Notes suggestions ([GitLab.com & 14.7+](https://gitlab.com/gitlab-org/gitlab/-/issues/340624)) [with a flag](../../../administration/feature_flags.md) named `github_importer_use_diff_note_with_suggestions`. Enabled by default. - Diff Notes suggestions ([GitLab.com & 14.7+](https://gitlab.com/gitlab-org/gitlab/-/issues/340624))
References to pull requests and issues are preserved (GitLab.com & 8.7+), and References to pull requests and issues are preserved (GitLab.com & 8.7+), and
each imported repository maintains visibility level unless that [visibility each imported repository maintains visibility level unless that [visibility
......
...@@ -26,7 +26,7 @@ module Gitlab ...@@ -26,7 +26,7 @@ module Gitlab
# because it cannot use the BulkImporting strategy, which skips # because it cannot use the BulkImporting strategy, which skips
# callbacks and validations. For this reason, notes that don't have # callbacks and validations. For this reason, notes that don't have
# suggestions are still imported with LegacyDiffNote # suggestions are still imported with LegacyDiffNote
if import_with_diff_note? if note.contains_suggestion?
import_with_diff_note import_with_diff_note
else else
import_with_legacy_diff_note import_with_legacy_diff_note
...@@ -48,17 +48,6 @@ module Gitlab ...@@ -48,17 +48,6 @@ module Gitlab
attr_reader :note, :project, :client, :author_id, :author_found attr_reader :note, :project, :client, :author_id, :author_found
def import_with_diff_note?
note.contains_suggestion? && use_diff_note_with_suggestions_enabled?
end
def use_diff_note_with_suggestions_enabled?
Feature.enabled?(
:github_importer_use_diff_note_with_suggestions,
default_enabled: :yaml
)
end
def build_author_attributes def build_author_attributes
@author_id, @author_found = user_finder.author_id_for(note) @author_id, @author_found = user_finder.author_id_for(note)
end end
......
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
module Gitlab module Gitlab
module RackAttack module RackAttack
module Request module Request
include ::Gitlab::Utils::StrongMemoize
FILES_PATH_REGEX = %r{^/api/v\d+/projects/[^/]+/repository/files/.+}.freeze FILES_PATH_REGEX = %r{^/api/v\d+/projects/[^/]+/repository/files/.+}.freeze
GROUP_PATH_REGEX = %r{^/api/v\d+/groups/[^/]+/?$}.freeze GROUP_PATH_REGEX = %r{^/api/v\d+/groups/[^/]+/?$}.freeze
...@@ -66,6 +68,7 @@ module Gitlab ...@@ -66,6 +68,7 @@ module Gitlab
def throttle_unauthenticated_api? def throttle_unauthenticated_api?
api_request? && api_request? &&
!should_be_skipped? && !should_be_skipped? &&
!frontend_request? &&
!throttle_unauthenticated_packages_api? && !throttle_unauthenticated_packages_api? &&
!throttle_unauthenticated_files_api? && !throttle_unauthenticated_files_api? &&
!throttle_unauthenticated_deprecated_api? && !throttle_unauthenticated_deprecated_api? &&
...@@ -74,7 +77,7 @@ module Gitlab ...@@ -74,7 +77,7 @@ module Gitlab
end end
def throttle_unauthenticated_web? def throttle_unauthenticated_web?
web_request? && (web_request? || frontend_request?) &&
!should_be_skipped? && !should_be_skipped? &&
# TODO: Column will be renamed in https://gitlab.com/gitlab-org/gitlab/-/issues/340031 # TODO: Column will be renamed in https://gitlab.com/gitlab-org/gitlab/-/issues/340031
Gitlab::Throttle.settings.throttle_unauthenticated_enabled && Gitlab::Throttle.settings.throttle_unauthenticated_enabled &&
...@@ -83,6 +86,7 @@ module Gitlab ...@@ -83,6 +86,7 @@ module Gitlab
def throttle_authenticated_api? def throttle_authenticated_api?
api_request? && api_request? &&
!frontend_request? &&
!throttle_authenticated_packages_api? && !throttle_authenticated_packages_api? &&
!throttle_authenticated_files_api? && !throttle_authenticated_files_api? &&
!throttle_authenticated_deprecated_api? && !throttle_authenticated_deprecated_api? &&
...@@ -90,7 +94,7 @@ module Gitlab ...@@ -90,7 +94,7 @@ module Gitlab
end end
def throttle_authenticated_web? def throttle_authenticated_web?
web_request? && (web_request? || frontend_request?) &&
!throttle_authenticated_git_lfs? && !throttle_authenticated_git_lfs? &&
Gitlab::Throttle.settings.throttle_authenticated_web_enabled Gitlab::Throttle.settings.throttle_authenticated_web_enabled
end end
...@@ -185,6 +189,17 @@ module Gitlab ...@@ -185,6 +189,17 @@ module Gitlab
path.match?(FILES_PATH_REGEX) path.match?(FILES_PATH_REGEX)
end end
def frontend_request?
return false unless Feature.enabled?(:rate_limit_frontend_requests, default_enabled: :yaml)
strong_memoize(:frontend_request) do
next false unless env.include?('HTTP_X_CSRF_TOKEN') && session.include?(:_csrf_token)
# CSRF tokens are not verified for GET/HEAD requests, so we pretend that we always have a POST request.
Gitlab::RequestForgeryProtection.verified?(env.merge('REQUEST_METHOD' => 'POST'))
end
end
def deprecated_api_request? def deprecated_api_request?
# The projects member of the groups endpoint is deprecated. If left # The projects member of the groups endpoint is deprecated. If left
# unspecified, with_projects defaults to true # unspecified, with_projects defaults to true
......
...@@ -3,15 +3,11 @@ ...@@ -3,15 +3,11 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe ApplicationCable::Connection, :clean_gitlab_redis_sessions do RSpec.describe ApplicationCable::Connection, :clean_gitlab_redis_sessions do
let(:session_id) { Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') } include SessionHelpers
context 'when session cookie is set' do context 'when session cookie is set' do
before do before do
Gitlab::Redis::Sessions.with do |redis| stub_session(session_hash)
redis.set("session:gitlab:#{session_id.private_id}", Marshal.dump(session_hash))
end
cookies[Gitlab::Application.config.session_options[:key]] = session_id.public_id
end end
context 'when user is logged in' do context 'when user is logged in' do
......
import { GlBadge } from '@gitlab/ui';
import { shallowMount } from '@vue/test-utils'; import { shallowMount } from '@vue/test-utils';
import AccessibilityIssueBody from '~/reports/accessibility_report/components/accessibility_issue_body.vue'; import AccessibilityIssueBody from '~/reports/accessibility_report/components/accessibility_issue_body.vue';
...@@ -29,7 +30,7 @@ describe('CustomMetricsForm', () => { ...@@ -29,7 +30,7 @@ describe('CustomMetricsForm', () => {
}); });
}; };
const findIsNewBadge = () => wrapper.find({ ref: 'accessibility-issue-is-new-badge' }); const findIsNewBadge = () => wrapper.findComponent(GlBadge);
beforeEach(() => { beforeEach(() => {
mountComponent(issue); mountComponent(issue);
...@@ -37,7 +38,6 @@ describe('CustomMetricsForm', () => { ...@@ -37,7 +38,6 @@ describe('CustomMetricsForm', () => {
afterEach(() => { afterEach(() => {
wrapper.destroy(); wrapper.destroy();
wrapper = null;
}); });
it('Displays the issue message', () => { it('Displays the issue message', () => {
...@@ -52,7 +52,7 @@ describe('CustomMetricsForm', () => { ...@@ -52,7 +52,7 @@ describe('CustomMetricsForm', () => {
.find({ ref: 'accessibility-issue-learn-more' }) .find({ ref: 'accessibility-issue-learn-more' })
.attributes('href'); .attributes('href');
expect(learnMoreUrl).toEqual(issue.learnMoreUrl); expect(learnMoreUrl).toBe(issue.learnMoreUrl);
}); });
}); });
...@@ -69,7 +69,7 @@ describe('CustomMetricsForm', () => { ...@@ -69,7 +69,7 @@ describe('CustomMetricsForm', () => {
.find({ ref: 'accessibility-issue-learn-more' }) .find({ ref: 'accessibility-issue-learn-more' })
.attributes('href'); .attributes('href');
expect(learnMoreUrl).toEqual('https://www.w3.org/TR/WCAG20-TECHS/Overview.html'); expect(learnMoreUrl).toBe('https://www.w3.org/TR/WCAG20-TECHS/Overview.html');
}); });
}); });
...@@ -86,7 +86,7 @@ describe('CustomMetricsForm', () => { ...@@ -86,7 +86,7 @@ describe('CustomMetricsForm', () => {
.find({ ref: 'accessibility-issue-learn-more' }) .find({ ref: 'accessibility-issue-learn-more' })
.attributes('href'); .attributes('href');
expect(learnMoreUrl).toEqual('https://www.w3.org/TR/WCAG20-TECHS/Overview.html'); expect(learnMoreUrl).toBe('https://www.w3.org/TR/WCAG20-TECHS/Overview.html');
}); });
}); });
...@@ -96,7 +96,7 @@ describe('CustomMetricsForm', () => { ...@@ -96,7 +96,7 @@ describe('CustomMetricsForm', () => {
}); });
it('Renders the new badge', () => { it('Renders the new badge', () => {
expect(findIsNewBadge().exists()).toEqual(true); expect(findIsNewBadge().exists()).toBe(true);
}); });
}); });
...@@ -106,7 +106,7 @@ describe('CustomMetricsForm', () => { ...@@ -106,7 +106,7 @@ describe('CustomMetricsForm', () => {
}); });
it('Does not render the new badge', () => { it('Does not render the new badge', () => {
expect(findIsNewBadge().exists()).toEqual(false); expect(findIsNewBadge().exists()).toBe(false);
}); });
}); });
}); });
...@@ -119,123 +119,80 @@ RSpec.describe Gitlab::GithubImport::Importer::DiffNoteImporter, :aggregate_fail ...@@ -119,123 +119,80 @@ RSpec.describe Gitlab::GithubImport::Importer::DiffNoteImporter, :aggregate_fail
.and_return(discussion_id) .and_return(discussion_id)
end end
context 'when github_importer_use_diff_note_with_suggestions is disabled' do it_behaves_like 'diff notes without suggestion'
before do
stub_feature_flags(github_importer_use_diff_note_with_suggestions: false) context 'when the note has suggestions' do
let(:note_body) do
<<~EOB
Suggestion:
```suggestion
what do you think to do it like this
```
EOB
end end
it_behaves_like 'diff notes without suggestion' before do
stub_user_finder(user.id, true)
end
context 'when the note has suggestions' do it 'imports the note as diff note' do
let(:note_body) do expect { subject.execute }
<<~EOB .to change(DiffNote, :count)
.by(1)
note = project.notes.diff_notes.take
expect(note).to be_valid
expect(note.noteable_type).to eq('MergeRequest')
expect(note.noteable_id).to eq(merge_request.id)
expect(note.project_id).to eq(project.id)
expect(note.author_id).to eq(user.id)
expect(note.system).to eq(false)
expect(note.discussion_id).to eq(discussion_id)
expect(note.commit_id).to eq('original123abc')
expect(note.line_code).to eq(note_representation.line_code)
expect(note.type).to eq('DiffNote')
expect(note.created_at).to eq(created_at)
expect(note.updated_at).to eq(updated_at)
expect(note.position.to_h).to eq({
base_sha: merge_request.diffs.diff_refs.base_sha,
head_sha: merge_request.diffs.diff_refs.head_sha,
start_sha: merge_request.diffs.diff_refs.start_sha,
new_line: 15,
old_line: nil,
new_path: file_path,
old_path: file_path,
position_type: 'text',
line_range: nil
})
expect(note.note)
.to eq <<~NOTE
Suggestion: Suggestion:
```suggestion ```suggestion:-0+0
what do you think to do it like this what do you think to do it like this
``` ```
EOB NOTE
end
it 'imports the note' do
stub_user_finder(user.id, true)
expect { subject.execute }
.to change(LegacyDiffNote, :count)
.and not_change(DiffNote, :count)
note = project.notes.diff_notes.take
expect(note).to be_valid
expect(note.note)
.to eq <<~NOTE
Suggestion:
```suggestion:-0+0
what do you think to do it like this
```
NOTE
end
end
end
context 'when github_importer_use_diff_note_with_suggestions is enabled' do
before do
stub_feature_flags(github_importer_use_diff_note_with_suggestions: true)
end end
it_behaves_like 'diff notes without suggestion' context 'when the note diff file creation fails' do
it 'falls back to the LegacyDiffNote' do
exception = ::DiffNote::NoteDiffFileCreationError.new('Failed to create diff note file')
context 'when the note has suggestions' do expect_next_instance_of(::Import::Github::Notes::CreateService) do |service|
let(:note_body) do expect(service)
<<~EOB .to receive(:execute)
Suggestion: .and_raise(exception)
```suggestion end
what do you think to do it like this
```
EOB
end
before do expect(Gitlab::GithubImport::Logger)
stub_user_finder(user.id, true) .to receive(:warn)
end .with(
message: 'Failed to create diff note file',
'error.class': 'DiffNote::NoteDiffFileCreationError'
)
it 'imports the note as diff note' do
expect { subject.execute } expect { subject.execute }
.to change(DiffNote, :count) .to change(LegacyDiffNote, :count)
.by(1) .and not_change(DiffNote, :count)
note = project.notes.diff_notes.take
expect(note).to be_valid
expect(note.noteable_type).to eq('MergeRequest')
expect(note.noteable_id).to eq(merge_request.id)
expect(note.project_id).to eq(project.id)
expect(note.author_id).to eq(user.id)
expect(note.system).to eq(false)
expect(note.discussion_id).to eq(discussion_id)
expect(note.commit_id).to eq('original123abc')
expect(note.line_code).to eq(note_representation.line_code)
expect(note.type).to eq('DiffNote')
expect(note.created_at).to eq(created_at)
expect(note.updated_at).to eq(updated_at)
expect(note.position.to_h).to eq({
base_sha: merge_request.diffs.diff_refs.base_sha,
head_sha: merge_request.diffs.diff_refs.head_sha,
start_sha: merge_request.diffs.diff_refs.start_sha,
new_line: 15,
old_line: nil,
new_path: file_path,
old_path: file_path,
position_type: 'text',
line_range: nil
})
expect(note.note)
.to eq <<~NOTE
Suggestion:
```suggestion:-0+0
what do you think to do it like this
```
NOTE
end
context 'when the note diff file creation fails' do
it 'falls back to the LegacyDiffNote' do
exception = ::DiffNote::NoteDiffFileCreationError.new('Failed to create diff note file')
expect_next_instance_of(::Import::Github::Notes::CreateService) do |service|
expect(service)
.to receive(:execute)
.and_raise(exception)
end
expect(Gitlab::GithubImport::Logger)
.to receive(:warn)
.with(
message: 'Failed to create diff note file',
'error.class': 'DiffNote::NoteDiffFileCreationError'
)
expect { subject.execute }
.to change(LegacyDiffNote, :count)
.and not_change(DiffNote, :count)
end
end end
end end
end end
......
...@@ -192,6 +192,44 @@ RSpec.describe Gitlab::RackAttack::Request do ...@@ -192,6 +192,44 @@ RSpec.describe Gitlab::RackAttack::Request do
end end
end end
describe '#frontend_request?', :allow_forgery_protection do
subject { request.send(:frontend_request?) }
let(:path) { '/' }
# Define these as local variables so we can use them in the `where` block.
valid_token = SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH)
other_token = SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH)
where(:session, :env, :expected) do
{} | {} | false # rubocop:disable Lint/BinaryOperatorWithIdenticalOperands
{} | { 'HTTP_X_CSRF_TOKEN' => valid_token } | false
{ _csrf_token: valid_token } | { 'HTTP_X_CSRF_TOKEN' => other_token } | false
{ _csrf_token: valid_token } | { 'HTTP_X_CSRF_TOKEN' => valid_token } | true
end
with_them do
it { is_expected.to eq(expected) }
end
context 'when the feature flag is disabled' do
before do
stub_feature_flags(rate_limit_frontend_requests: false)
end
where(:session, :env) do
{} | {} # rubocop:disable Lint/BinaryOperatorWithIdenticalOperands
{} | { 'HTTP_X_CSRF_TOKEN' => valid_token }
{ _csrf_token: valid_token } | { 'HTTP_X_CSRF_TOKEN' => other_token }
{ _csrf_token: valid_token } | { 'HTTP_X_CSRF_TOKEN' => valid_token }
end
with_them do
it { is_expected.to be(false) }
end
end
end
describe '#deprecated_api_request?' do describe '#deprecated_api_request?' do
subject { request.send(:deprecated_api_request?) } subject { request.send(:deprecated_api_request?) }
......
...@@ -5,6 +5,7 @@ require 'mime/types' ...@@ -5,6 +5,7 @@ require 'mime/types'
RSpec.describe API::Commits do RSpec.describe API::Commits do
include ProjectForksHelper include ProjectForksHelper
include SessionHelpers
let(:user) { create(:user) } let(:user) { create(:user) }
let(:guest) { create(:user).tap { |u| project.add_guest(u) } } let(:guest) { create(:user).tap { |u| project.add_guest(u) } }
...@@ -384,14 +385,7 @@ RSpec.describe API::Commits do ...@@ -384,14 +385,7 @@ RSpec.describe API::Commits do
context 'when using warden' do context 'when using warden' do
it 'increments usage counters', :clean_gitlab_redis_sessions do it 'increments usage counters', :clean_gitlab_redis_sessions do
session_id = Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') stub_session('warden.user.user.key' => [[user.id], user.encrypted_password[0, 29]])
session_hash = { 'warden.user.user.key' => [[user.id], user.encrypted_password[0, 29]] }
Gitlab::Redis::Sessions.with do |redis|
redis.set("session:gitlab:#{session_id.private_id}", Marshal.dump(session_hash))
end
cookies[Gitlab::Application.config.session_options[:key]] = session_id.public_id
expect(::Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_commits_count) expect(::Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_commits_count)
expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).to receive(:track_web_ide_edit_action) expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).to receive(:track_web_ide_edit_action)
......
...@@ -4,6 +4,7 @@ require 'spec_helper' ...@@ -4,6 +4,7 @@ require 'spec_helper'
RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_caching do RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_caching do
include RackAttackSpecHelpers include RackAttackSpecHelpers
include SessionHelpers
let(:settings) { Gitlab::CurrentSettings.current_application_settings } let(:settings) { Gitlab::CurrentSettings.current_application_settings }
...@@ -63,6 +64,22 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac ...@@ -63,6 +64,22 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
end end
end end
describe 'API requests from the frontend', :api, :clean_gitlab_redis_sessions do
context 'when unauthenticated' do
it_behaves_like 'rate-limited frontend API requests' do
let(:throttle_setting_prefix) { 'throttle_unauthenticated' }
end
end
context 'when authenticated' do
it_behaves_like 'rate-limited frontend API requests' do
let_it_be(:personal_access_token) { create(:personal_access_token) }
let(:throttle_setting_prefix) { 'throttle_authenticated' }
end
end
end
describe 'API requests authenticated with personal access token', :api do describe 'API requests authenticated with personal access token', :api do
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:token) { create(:personal_access_token, user: user) } let_it_be(:token) { create(:personal_access_token, user: user) }
......
...@@ -26,14 +26,14 @@ module RackAttackSpecHelpers ...@@ -26,14 +26,14 @@ module RackAttackSpecHelpers
{ 'AUTHORIZATION' => "Basic #{encoded_login}" } { 'AUTHORIZATION' => "Basic #{encoded_login}" }
end end
def expect_rejection(&block) def expect_rejection(name = nil, &block)
yield yield
expect(response).to have_gitlab_http_status(:too_many_requests) expect(response).to have_gitlab_http_status(:too_many_requests)
expect(response.headers.to_h).to include( expect(response.headers.to_h).to include(
'RateLimit-Limit' => a_string_matching(/^\d+$/), 'RateLimit-Limit' => a_string_matching(/^\d+$/),
'RateLimit-Name' => a_string_matching(/^throttle_.*$/), 'RateLimit-Name' => name || a_string_matching(/^throttle_.*$/),
'RateLimit-Observed' => a_string_matching(/^\d+$/), 'RateLimit-Observed' => a_string_matching(/^\d+$/),
'RateLimit-Remaining' => a_string_matching(/^\d+$/), 'RateLimit-Remaining' => a_string_matching(/^\d+$/),
'Retry-After' => a_string_matching(/^\d+$/) 'Retry-After' => a_string_matching(/^\d+$/)
......
# frozen_string_literal: true # frozen_string_literal: true
module SessionHelpers module SessionHelpers
# Stub a session in Redis, for use in request specs where we can't mock the session directly.
# This also needs the :clean_gitlab_redis_sessions tag on the spec.
def stub_session(session_hash)
unless RSpec.current_example.metadata[:clean_gitlab_redis_sessions]
raise 'Add :clean_gitlab_redis_sessions to your spec!'
end
session_id = Rack::Session::SessionId.new(SecureRandom.hex)
Gitlab::Redis::Sessions.with do |redis|
redis.set("session:gitlab:#{session_id.private_id}", Marshal.dump(session_hash))
end
cookies[Gitlab::Application.config.session_options[:key]] = session_id.public_id
end
def expect_single_session_with_authenticated_ttl def expect_single_session_with_authenticated_ttl
expect_single_session_with_expiration(Settings.gitlab['session_expire_delay'] * 60) expect_single_session_with_expiration(Settings.gitlab['session_expire_delay'] * 60)
end end
......
...@@ -10,6 +10,8 @@ RSpec.shared_examples 'when the snippet is not found' do ...@@ -10,6 +10,8 @@ RSpec.shared_examples 'when the snippet is not found' do
end end
RSpec.shared_examples 'snippet edit usage data counters' do RSpec.shared_examples 'snippet edit usage data counters' do
include SessionHelpers
context 'when user is sessionless' do context 'when user is sessionless' do
it 'does not track usage data actions' do it 'does not track usage data actions' do
expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).not_to receive(:track_snippet_editor_edit_action) expect(::Gitlab::UsageDataCounters::EditorUniqueCounter).not_to receive(:track_snippet_editor_edit_action)
...@@ -20,14 +22,7 @@ RSpec.shared_examples 'snippet edit usage data counters' do ...@@ -20,14 +22,7 @@ RSpec.shared_examples 'snippet edit usage data counters' do
context 'when user is not sessionless', :clean_gitlab_redis_sessions do context 'when user is not sessionless', :clean_gitlab_redis_sessions do
before do before do
session_id = Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') stub_session('warden.user.user.key' => [[current_user.id], current_user.encrypted_password[0, 29]])
session_hash = { 'warden.user.user.key' => [[current_user.id], current_user.encrypted_password[0, 29]] }
Gitlab::Redis::Sessions.with do |redis|
redis.set("session:gitlab:#{session_id.private_id}", Marshal.dump(session_hash))
end
cookies[Gitlab::Application.config.session_options[:key]] = session_id.public_id
end end
it 'tracks usage data actions', :clean_gitlab_redis_sessions do it 'tracks usage data actions', :clean_gitlab_redis_sessions do
......
...@@ -580,3 +580,88 @@ RSpec.shared_examples 'rate-limited unauthenticated requests' do ...@@ -580,3 +580,88 @@ RSpec.shared_examples 'rate-limited unauthenticated requests' do
end end
end end
end end
# Requires let variables:
# * throttle_setting_prefix: "throttle_authenticated", "throttle_unauthenticated"
RSpec.shared_examples 'rate-limited frontend API requests' do
let(:requests_per_period) { 1 }
let(:csrf_token) { SecureRandom.base64(ActionController::RequestForgeryProtection::AUTHENTICITY_TOKEN_LENGTH) }
let(:csrf_session) { { _csrf_token: csrf_token } }
let(:personal_access_token) { nil }
let(:api_path) { '/projects' }
# These don't actually exist, so a 404 is the expected response.
let(:files_api_path) { '/projects/1/repository/files/ref/path' }
let(:packages_api_path) { '/projects/1/packages/foo' }
let(:deprecated_api_path) { '/groups/1?with_projects=true' }
def get_api(path: api_path, csrf: false)
headers = csrf ? { 'X-CSRF-Token' => csrf_token } : nil
get api(path, personal_access_token: personal_access_token), headers: headers
end
def expect_not_found(&block)
yield
expect(response).to have_gitlab_http_status(:not_found)
end
before do
stub_application_setting(
"#{throttle_setting_prefix}_enabled" => true,
"#{throttle_setting_prefix}_requests_per_period" => requests_per_period,
"#{throttle_setting_prefix}_api_enabled" => true,
"#{throttle_setting_prefix}_api_requests_per_period" => requests_per_period,
"#{throttle_setting_prefix}_web_enabled" => true,
"#{throttle_setting_prefix}_web_requests_per_period" => requests_per_period,
"#{throttle_setting_prefix}_files_api_enabled" => true,
"#{throttle_setting_prefix}_packages_api_enabled" => true,
"#{throttle_setting_prefix}_deprecated_api_enabled" => true
)
stub_session(csrf_session)
end
context 'with a CSRF token' do
it 'uses the rate limit for web requests' do
requests_per_period.times { get_api csrf: true }
expect_rejection("#{throttle_setting_prefix}_web") { get_api csrf: true }
expect_rejection("#{throttle_setting_prefix}_web") { get_api csrf: true, path: files_api_path }
expect_rejection("#{throttle_setting_prefix}_web") { get_api csrf: true, path: packages_api_path }
expect_rejection("#{throttle_setting_prefix}_web") { get_api csrf: true, path: deprecated_api_path }
# API rate limit is not triggered yet
expect_ok { get_api }
expect_not_found { get_api path: files_api_path }
expect_not_found { get_api path: packages_api_path }
expect_not_found { get_api path: deprecated_api_path }
end
context 'without a CSRF session' do
let(:csrf_session) { nil }
it 'always uses the rate limit for API requests' do
requests_per_period.times { get_api csrf: true }
expect_rejection("#{throttle_setting_prefix}_api") { get_api csrf: true }
expect_rejection("#{throttle_setting_prefix}_api") { get_api }
end
end
end
context 'without a CSRF token' do
it 'uses the rate limit for API requests' do
requests_per_period.times { get_api }
expect_rejection("#{throttle_setting_prefix}_api") { get_api }
# Web and custom API rate limits are not triggered yet
expect_ok { get_api csrf: true }
expect_not_found { get_api path: files_api_path }
expect_not_found { get_api path: packages_api_path }
expect_not_found { get_api path: deprecated_api_path }
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment