From 4601b44af1e341c3d00317ef1cfbbd909cd53070 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Sat, 1 May 2021 00:10:00 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- Gemfile | 3 + Gemfile.lock | 3 + app/models/application_setting.rb | 4 +- app/services/spam/akismet_service.rb | 2 +- app/services/spam/spam_action_service.rb | 7 +- app/services/spam/spam_verdict_service.rb | 79 +++++++------- app/views/projects/new.html.haml | 11 +- ...99558-integrate-with-anti-spam-service.yml | 5 + .../yogi-hide-templates-buttons.yml | 5 - doc/development/packages.md | 24 ++++ doc/user/group/settings/import_export.md | 8 ++ doc/user/project/settings/import_export.md | 6 + lib/gitlab/spamcheck/client.rb | 96 ++++++++++++++++ spec/features/admin/admin_settings_spec.rb | 4 +- spec/lib/gitlab/spamcheck/client_spec.rb | 103 ++++++++++++++++++ spec/models/application_setting_spec.rb | 6 +- spec/requests/api/settings_spec.rb | 4 +- .../services/spam/spam_action_service_spec.rb | 38 ++++++- .../spam/spam_verdict_service_spec.rb | 101 ++++++++++++----- 19 files changed, 415 insertions(+), 94 deletions(-) create mode 100644 changelogs/unreleased/299558-integrate-with-anti-spam-service.yml delete mode 100644 changelogs/unreleased/yogi-hide-templates-buttons.yml create mode 100644 lib/gitlab/spamcheck/client.rb create mode 100644 spec/lib/gitlab/spamcheck/client_spec.rb diff --git a/Gemfile b/Gemfile index a539a3c13d3..fcf5aae2585 100644 --- a/Gemfile +++ b/Gemfile @@ -477,6 +477,9 @@ group :ed25519 do gem 'bcrypt_pbkdf', '~> 1.0' end +# Spamcheck GRPC protocol definitions +gem 'spamcheck', '~> 0.0.5' + # Gitaly GRPC protocol definitions gem 'gitaly', '~> 13.11.0.pre.rc1' diff --git a/Gemfile.lock b/Gemfile.lock index 369ae157e0e..1054af774c7 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1211,6 +1211,8 @@ GEM thor (~> 1.0) tilt (~> 2.0) yard (~> 0.9, >= 0.9.24) + spamcheck (0.0.5) + grpc (~> 1.0) spring (2.1.1) spring-commands-rspec (1.0.4) spring (>= 0.9.1) @@ -1606,6 +1608,7 @@ DEPENDENCIES slack-messenger (~> 2.3.4) snowplow-tracker (~> 0.6.1) solargraph (~> 0.40.4) + spamcheck (~> 0.0.5) spring (~> 2.1.0) spring-commands-rspec (~> 1.0.4) sprockets (~> 3.7.0) diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 00b4363faa0..2d4b2ce135e 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -375,7 +375,7 @@ class ApplicationSetting < ApplicationRecord if: :external_authorization_service_enabled validates :spam_check_endpoint_url, - addressable_url: true, allow_blank: true + addressable_url: { schemes: %w(grpc) }, allow_blank: true validates :spam_check_endpoint_url, presence: true, @@ -524,7 +524,7 @@ class ApplicationSetting < ApplicationRecord attr_encrypted :lets_encrypt_private_key, encryption_options_base_32_aes_256_gcm attr_encrypted :eks_secret_access_key, encryption_options_base_32_aes_256_gcm attr_encrypted :akismet_api_key, encryption_options_base_32_aes_256_gcm - attr_encrypted :spam_check_api_key, encryption_options_base_32_aes_256_gcm + attr_encrypted :spam_check_api_key, encryption_options_base_32_aes_256_gcm.merge(encode: false) attr_encrypted :elasticsearch_aws_secret_access_key, encryption_options_base_32_aes_256_gcm attr_encrypted :recaptcha_private_key, encryption_options_base_32_aes_256_gcm attr_encrypted :recaptcha_site_key, encryption_options_base_32_aes_256_gcm diff --git a/app/services/spam/akismet_service.rb b/app/services/spam/akismet_service.rb index 75673518b48..4e56972ccd5 100644 --- a/app/services/spam/akismet_service.rb +++ b/app/services/spam/akismet_service.rb @@ -20,7 +20,7 @@ module Spam created_at: DateTime.current, author: owner_name, author_email: owner_email, - referrer: options[:referrer] + referer: options[:referer] } begin diff --git a/app/services/spam/spam_action_service.rb b/app/services/spam/spam_action_service.rb index 2220198583c..1cd432e1f66 100644 --- a/app/services/spam/spam_action_service.rb +++ b/app/services/spam/spam_action_service.rb @@ -53,7 +53,7 @@ module Spam if request options[:ip_address] = request.env['action_dispatch.remote_ip'].to_s options[:user_agent] = request.env['HTTP_USER_AGENT'] - options[:referrer] = request.env['HTTP_REFERRER'] + options[:referer] = request.env['HTTP_REFERER'] else # TODO: This code is never used, because we do not perform a verification if there is not a # request. Why? Should it be deleted? Or should we check even if there is no request? @@ -123,6 +123,11 @@ module Spam # https://gitlab.com/gitlab-org/gitlab/-/issues/214739 target.spam! unless target.allow_possible_spam? create_spam_log(api) + when BLOCK_USER + # TODO: improve BLOCK_USER handling, non-existent until now + # https://gitlab.com/gitlab-org/gitlab/-/issues/329666 + target.spam! unless target.allow_possible_spam? + create_spam_log(api) when ALLOW target.clear_spam_flags! end diff --git a/app/services/spam/spam_verdict_service.rb b/app/services/spam/spam_verdict_service.rb index 2463a5227d0..8dbc590314c 100644 --- a/app/services/spam/spam_verdict_service.rb +++ b/app/services/spam/spam_verdict_service.rb @@ -10,25 +10,44 @@ module Spam @request = request @user = user @options = options - @verdict_params = assemble_verdict_params(context) + @context = context end def execute - external_spam_check_result = external_verdict + spamcheck_result = nil + + external_spam_check_round_trip_time = Benchmark.realtime do + spamcheck_result = spamcheck_verdict + end + akismet_result = akismet_verdict # filter out anything we don't recognise, including nils. - valid_results = [external_spam_check_result, akismet_result].compact.select { |r| SUPPORTED_VERDICTS.key?(r) } + valid_results = [spamcheck_result, akismet_result].compact.select { |r| SUPPORTED_VERDICTS.key?(r) } + # Treat nils - such as service unavailable - as ALLOW return ALLOW unless valid_results.any? # Favour the most restrictive result. - valid_results.min_by { |v| SUPPORTED_VERDICTS[v][:priority] } + final_verdict = valid_results.min_by { |v| SUPPORTED_VERDICTS[v][:priority] } + + logger.info(class: self.class.name, + akismet_verdict: akismet_verdict, + spam_check_verdict: spamcheck_result, + spam_check_rtt: external_spam_check_round_trip_time.real, + final_verdict: final_verdict, + username: user.username, + user_id: user.id, + target_type: target.class.to_s, + project_id: target.project_id + ) + + final_verdict end private - attr_reader :user, :target, :request, :options, :verdict_params + attr_reader :user, :target, :request, :options, :context def akismet_verdict if akismet.spam? @@ -38,54 +57,34 @@ module Spam end end - def external_verdict + def spamcheck_verdict return unless Gitlab::CurrentSettings.spam_check_endpoint_enabled - return if endpoint_url.blank? begin - result = Gitlab::HTTP.post(endpoint_url, body: verdict_params.to_json, headers: { 'Content-Type' => 'application/json' }) + result, _error = spamcheck_client.issue_spam?(spam_issue: target, user: user, context: context) return unless result - json_result = Gitlab::Json.parse(result).with_indifferent_access - # @TODO metrics/logging - # Expecting: - # error: (string or nil) - # verdict: (string or nil) - # @TODO log if json_result[:error] + # @TODO log if error is not nil https://gitlab.com/gitlab-org/gitlab/-/issues/329545 - json_result[:verdict] - rescue *Gitlab::HTTP::HTTP_ERRORS => e - # @TODO: log error via try_post https://gitlab.com/gitlab-org/gitlab/-/issues/219223 + # Duplicate logic with Akismet logic in #akismet_verdict + if Gitlab::Recaptcha.enabled? && result != ALLOW + CONDITIONAL_ALLOW + else + result + end + rescue StandardError => e Gitlab::ErrorTracking.log_exception(e) - nil - rescue StandardError - # @TODO log + # Default to ALLOW if any errors occur ALLOW end end - def assemble_verdict_params(context) - return {} unless endpoint_url.present? - - project = target.try(:project) - - context.merge({ - target: { - title: target.spam_title, - description: target.spam_description, - type: target.class.to_s - }, - user: { - created_at: user.created_at, - email: user.email, - username: user.username - }, - user_in_project: user.authorized_project?(project) - }) + def spamcheck_client + @spamcheck_client ||= Gitlab::Spamcheck::Client.new end - def endpoint_url - @endpoint_url ||= Gitlab::CurrentSettings.current_application_settings.spam_check_endpoint_url + def logger + @logger ||= Gitlab::AppJsonLogger.build end end end diff --git a/app/views/projects/new.html.haml b/app/views/projects/new.html.haml index 247d2b74cdc..609d3317315 100644 --- a/app/views/projects/new.html.haml +++ b/app/views/projects/new.html.haml @@ -51,12 +51,11 @@ = render 'new_project_fields', f: f, project_name_id: "blank-project-name" #create-from-template-pane.tab-pane.js-toggle-container.px-0.pb-0{ class: active_when(active_tab == 'template'), role: 'tabpanel' } - .gl-card.gl-my-5 - .gl-card-body - %div - - contributing_templates_url = 'https://gitlab.com/gitlab-org/project-templates/contributing' - - link_start = ''.html_safe % { url: contributing_templates_url } - = _('Learn how to %{link_start}contribute to the built-in templates%{link_end}').html_safe % { link_start: link_start, link_end: ''.html_safe } + .card.card-slim.m-4.p-4 + %div + - contributing_templates_url = 'https://gitlab.com/gitlab-org/project-templates/contributing' + - link_start = ''.html_safe % { url: contributing_templates_url } + = _('Learn how to %{link_start}contribute to the built-in templates%{link_end}').html_safe % { link_start: link_start, link_end: ''.html_safe } = form_for @project, html: { class: 'new_project' } do |f| .project-template .form-group diff --git a/changelogs/unreleased/299558-integrate-with-anti-spam-service.yml b/changelogs/unreleased/299558-integrate-with-anti-spam-service.yml new file mode 100644 index 00000000000..60d9646af7b --- /dev/null +++ b/changelogs/unreleased/299558-integrate-with-anti-spam-service.yml @@ -0,0 +1,5 @@ +--- +title: Integrate with the Spamcheck anti-spam engine +merge_request: 52385 +author: +type: added diff --git a/changelogs/unreleased/yogi-hide-templates-buttons.yml b/changelogs/unreleased/yogi-hide-templates-buttons.yml deleted file mode 100644 index 0d7b23fd8ff..00000000000 --- a/changelogs/unreleased/yogi-hide-templates-buttons.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Remove template tabs in new project page for GitLab.com -merge_request: 59083 -author: Yogi (@yo) -type: changed diff --git a/doc/development/packages.md b/doc/development/packages.md index b37ace01e3b..2dd712f2692 100644 --- a/doc/development/packages.md +++ b/doc/development/packages.md @@ -256,6 +256,30 @@ These route prefixes guarantee a higher rate limit: /api/v4/groups/:group_id/-/packages/ ``` +### MVC Checklist + +When adding support to GitLab for a new package manager, the first iteration must contain the +following features. You can add the features through many merge requests as needed, but all the +features must be implemented when the feature flag is removed. + +- Project-level API +- Push event tracking +- Pull event tracking +- Authentication with Personal Access Tokens +- Authentication with Job Tokens +- Authentication with Deploy Tokens (group and project) +- File size [limit](#file-size-limits) +- File format guards (only accept valid file formats for the package type) +- Name regex with validation +- Version regex with validation +- Workhorse route for [accelerated](uploads.md#how-to-add-a-new-upload-route) uploads +- Background workers for extracting package metadata (if applicable) +- Documentation (how to use the feature) +- API Documentation (individual endpoints with curl examples) +- Seeding in [`db/fixtures/development/26_packages.rb`](https://gitlab.com/gitlab-org/gitlab/blob/master/db/fixtures/development/26_packages.rb) +- Update the [runbook](https://gitlab.com/gitlab-com/runbooks/-/blob/31fb4959e89db25fddf865bc81734c222daf32dd/dashboards/stage-groups/package.dashboard.jsonnet#L74) for the Grafana charts +- End-to-end feature tests for (at the minimum) publishing and installing a package + ### Future Work While working on the MVC, contributors might find features that are not mandatory for the MVC but can provide a better user experience. It's generally a good idea to keep an eye on those and open issues. diff --git a/doc/user/group/settings/import_export.md b/doc/user/group/settings/import_export.md index 18574912ce1..629c65bc363 100644 --- a/doc/user/group/settings/import_export.md +++ b/doc/user/group/settings/import_export.md @@ -109,6 +109,14 @@ on an existing group's page. ## Version history +### 14.0+ + +In GitLab 14.0, the JSON format is no longer supported for project and group exports. To allow for a +transitional period, you can still import any JSON exports. The new format for imports and exports +is NDJSON. + +### 13.0+ + GitLab can import bundles that were exported from a different GitLab deployment. This ability is limited to two previous GitLab [minor](../../../policy/maintenance.md#versioning) releases, which is similar to our process for [Security Releases](../../../policy/maintenance.md#security-releases). diff --git a/doc/user/project/settings/import_export.md b/doc/user/project/settings/import_export.md index 9a7fa502cab..7c87630fe72 100644 --- a/doc/user/project/settings/import_export.md +++ b/doc/user/project/settings/import_export.md @@ -57,6 +57,12 @@ Note the following: ## Version history +### 14.0+ + +In GitLab 14.0, the JSON format is no longer supported for project and group exports. To allow for a +transitional period, you can still import any JSON exports. The new format for imports and exports +is NDJSON. + ### 13.0+ Starting with GitLab 13.0, GitLab can import bundles that were exported from a different GitLab deployment. diff --git a/lib/gitlab/spamcheck/client.rb b/lib/gitlab/spamcheck/client.rb new file mode 100644 index 00000000000..74a3a298bce --- /dev/null +++ b/lib/gitlab/spamcheck/client.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true +require 'spamcheck' + +module Gitlab + module Spamcheck + class Client + include ::Spam::SpamConstants + DEFAULT_TIMEOUT_SECS = 2 + + VERDICT_MAPPING = { + ::Spamcheck::SpamVerdict::Verdict::ALLOW => ALLOW, + ::Spamcheck::SpamVerdict::Verdict::CONDITIONAL_ALLOW => CONDITIONAL_ALLOW, + ::Spamcheck::SpamVerdict::Verdict::DISALLOW => DISALLOW, + ::Spamcheck::SpamVerdict::Verdict::BLOCK => BLOCK_USER + }.freeze + + ACTION_MAPPING = { + create: ::Spamcheck::Action::CREATE, + update: ::Spamcheck::Action::UPDATE + }.freeze + + def initialize + @endpoint_url = Gitlab::CurrentSettings.current_application_settings.spam_check_endpoint_url + + # remove the `grpc://` as it's only useful to ensure we're expecting to + # connect with Spamcheck + @endpoint_url = @endpoint_url.gsub(%r(^grpc:\/\/), '') + + creds = + if Rails.env.development? || Rails.env.test? + :this_channel_is_insecure + else + GRPC::Core::ChannelCredentials.new + end + + @stub = ::Spamcheck::SpamcheckService::Stub.new(@endpoint_url, creds, + timeout: DEFAULT_TIMEOUT_SECS) + end + + def issue_spam?(spam_issue:, user:, context: {}) + issue = build_issue_protobuf(issue: spam_issue, user: user, context: context) + + response = @stub.check_for_spam_issue(issue, + metadata: { 'authorization' => + Gitlab::CurrentSettings.spam_check_api_key }) + verdict = convert_verdict_to_gitlab_constant(response.verdict) + [verdict, response.error] + end + + private + + def convert_verdict_to_gitlab_constant(verdict) + VERDICT_MAPPING.fetch(::Spamcheck::SpamVerdict::Verdict.resolve(verdict), verdict) + end + + def build_issue_protobuf(issue:, user:, context:) + issue_pb = ::Spamcheck::Issue.new + issue_pb.title = issue.spam_title || '' + issue_pb.description = issue.spam_description || '' + issue_pb.created_at = convert_to_pb_timestamp(issue.created_at) if issue.created_at + issue_pb.updated_at = convert_to_pb_timestamp(issue.updated_at) if issue.updated_at + issue_pb.user_in_project = user.authorized_project?(issue.project) + issue_pb.action = ACTION_MAPPING.fetch(context.fetch(:action)) if context.has_key?(:action) + issue_pb.user = build_user_protobuf(user) + issue_pb + end + + def build_user_protobuf(user) + user_pb = ::Spamcheck::User.new + user_pb.username = user.username + user_pb.org = user.organization || '' + user_pb.created_at = convert_to_pb_timestamp(user.created_at) + + user_pb.emails << build_email(user.email, user.confirmed?) + + user.emails.each do |email| + user_pb.emails << build_email(email.email, email.confirmed?) + end + + user_pb + end + + def build_email(email, verified) + email_pb = ::Spamcheck::User::Email.new + email_pb.email = email + email_pb.verified = verified + email_pb + end + + def convert_to_pb_timestamp(ar_timestamp) + Google::Protobuf::Timestamp.new(seconds: ar_timestamp.to_time.to_i, + nanos: ar_timestamp.to_time.nsec) + end + end + end +end diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 4e27d3833db..4f17260f87d 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -425,7 +425,7 @@ RSpec.describe 'Admin updates settings' do check 'Enable reCAPTCHA for login' fill_in 'IPs per user', with: 15 check 'Enable Spam Check via external API endpoint' - fill_in 'URL of the external Spam Check endpoint', with: 'https://www.example.com/spamcheck' + fill_in 'URL of the external Spam Check endpoint', with: 'grpc://www.example.com/spamcheck' fill_in 'Spam Check API Key', with: 'SPAM_CHECK_API_KEY' click_button 'Save changes' end @@ -435,7 +435,7 @@ RSpec.describe 'Admin updates settings' do expect(current_settings.login_recaptcha_protection_enabled).to be true expect(current_settings.unique_ips_limit_per_user).to eq(15) expect(current_settings.spam_check_endpoint_enabled).to be true - expect(current_settings.spam_check_endpoint_url).to eq 'https://www.example.com/spamcheck' + expect(current_settings.spam_check_endpoint_url).to eq 'grpc://www.example.com/spamcheck' end end diff --git a/spec/lib/gitlab/spamcheck/client_spec.rb b/spec/lib/gitlab/spamcheck/client_spec.rb new file mode 100644 index 00000000000..3978ff9fc2e --- /dev/null +++ b/spec/lib/gitlab/spamcheck/client_spec.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Spamcheck::Client do + include_context 'includes Spam constants' + + let(:endpoint) { 'grpc://grpc.test.url' } + let_it_be(:user) { create(:user, organization: 'GitLab') } + let(:verdict_value) { nil } + let(:error_value) { "" } + let_it_be(:issue) { create(:issue, description: 'Test issue description') } + + let(:response) do + verdict = ::Spamcheck::SpamVerdict.new + verdict.verdict = verdict_value + verdict.error = error_value + verdict + end + + subject { described_class.new.issue_spam?(spam_issue: issue, user: user) } + + before do + stub_application_setting(spam_check_endpoint_url: endpoint) + end + + describe '#issue_spam?' do + before do + allow_next_instance_of(::Spamcheck::SpamcheckService::Stub) do |instance| + allow(instance).to receive(:check_for_spam_issue).and_return(response) + end + end + + using RSpec::Parameterized::TableSyntax + + where(:verdict, :expected) do + ::Spamcheck::SpamVerdict::Verdict::ALLOW | Spam::SpamConstants::ALLOW + ::Spamcheck::SpamVerdict::Verdict::CONDITIONAL_ALLOW | Spam::SpamConstants::CONDITIONAL_ALLOW + ::Spamcheck::SpamVerdict::Verdict::DISALLOW | Spam::SpamConstants::DISALLOW + ::Spamcheck::SpamVerdict::Verdict::BLOCK | Spam::SpamConstants::BLOCK_USER + end + + with_them do + let(:verdict_value) { verdict } + + it "returns expected spam constant" do + expect(subject).to eq([expected, ""]) + end + end + end + + describe "#build_issue_protobuf", :aggregate_failures do + it 'builds the expected protobuf object' do + cxt = { action: :create } + issue_pb = described_class.new.send(:build_issue_protobuf, + issue: issue, user: user, + context: cxt) + expect(issue_pb.title).to eq issue.title + expect(issue_pb.description).to eq issue.description + expect(issue_pb.user_in_project). to be false + expect(issue_pb.created_at).to eq timestamp_to_protobuf_timestamp(issue.created_at) + expect(issue_pb.updated_at).to eq timestamp_to_protobuf_timestamp(issue.updated_at) + expect(issue_pb.action).to be ::Spamcheck::Action.lookup(::Spamcheck::Action::CREATE) + expect(issue_pb.user.username).to eq user.username + end + end + + describe '#build_user_proto_buf', :aggregate_failures do + it 'builds the expected protobuf object' do + user_pb = described_class.new.send(:build_user_protobuf, user) + expect(user_pb.username).to eq user.username + expect(user_pb.org).to eq user.organization + expect(user_pb.created_at).to eq timestamp_to_protobuf_timestamp(user.created_at) + expect(user_pb.emails.count).to be 1 + expect(user_pb.emails.first.email).to eq user.email + expect(user_pb.emails.first.verified).to eq user.confirmed? + end + + context 'when user has multiple email addresses' do + let(:secondary_email) {create(:email, :confirmed, user: user)} + + before do + user.emails << secondary_email + end + + it 'adds emsils to the user pb object' do + user_pb = described_class.new.send(:build_user_protobuf, user) + expect(user_pb.emails.count).to eq 2 + expect(user_pb.emails.first.email).to eq user.email + expect(user_pb.emails.first.verified).to eq user.confirmed? + expect(user_pb.emails.last.email).to eq secondary_email.email + expect(user_pb.emails.last.verified).to eq secondary_email.confirmed? + end + end + end + + private + + def timestamp_to_protobuf_timestamp(timestamp) + Google::Protobuf::Timestamp.new(seconds: timestamp.to_time.to_i, + nanos: timestamp.to_time.nsec) + end +end diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index c7d05b64235..f96444f9e83 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -216,7 +216,8 @@ RSpec.describe ApplicationSetting do setting.spam_check_endpoint_enabled = true end - it { is_expected.to allow_value('https://example.org/spam_check').for(:spam_check_endpoint_url) } + it { is_expected.to allow_value('grpc://example.org/spam_check').for(:spam_check_endpoint_url) } + it { is_expected.not_to allow_value('https://example.org/spam_check').for(:spam_check_endpoint_url) } it { is_expected.not_to allow_value('nonsense').for(:spam_check_endpoint_url) } it { is_expected.not_to allow_value(nil).for(:spam_check_endpoint_url) } it { is_expected.not_to allow_value('').for(:spam_check_endpoint_url) } @@ -227,7 +228,8 @@ RSpec.describe ApplicationSetting do setting.spam_check_endpoint_enabled = false end - it { is_expected.to allow_value('https://example.org/spam_check').for(:spam_check_endpoint_url) } + it { is_expected.to allow_value('grpc://example.org/spam_check').for(:spam_check_endpoint_url) } + it { is_expected.not_to allow_value('https://example.org/spam_check').for(:spam_check_endpoint_url) } it { is_expected.not_to allow_value('nonsense').for(:spam_check_endpoint_url) } it { is_expected.to allow_value(nil).for(:spam_check_endpoint_url) } it { is_expected.to allow_value('').for(:spam_check_endpoint_url) } diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index 01a0d124b42..66c0dcaa36c 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -123,7 +123,7 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting do issues_create_limit: 300, raw_blob_request_limit: 300, spam_check_endpoint_enabled: true, - spam_check_endpoint_url: 'https://example.com/spam_check', + spam_check_endpoint_url: 'grpc://example.com/spam_check', spam_check_api_key: 'SPAM_CHECK_API_KEY', disabled_oauth_sign_in_sources: 'unknown', import_sources: 'github,bitbucket', @@ -169,7 +169,7 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting do expect(json_response['issues_create_limit']).to eq(300) expect(json_response['raw_blob_request_limit']).to eq(300) expect(json_response['spam_check_endpoint_enabled']).to be_truthy - expect(json_response['spam_check_endpoint_url']).to eq('https://example.com/spam_check') + expect(json_response['spam_check_endpoint_url']).to eq('grpc://example.com/spam_check') expect(json_response['spam_check_api_key']).to eq('SPAM_CHECK_API_KEY') expect(json_response['disabled_oauth_sign_in_sources']).to eq([]) expect(json_response['import_sources']).to match_array(%w(github bitbucket)) diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb index e8ac826df1c..318643e04db 100644 --- a/spec/services/spam/spam_action_service_spec.rb +++ b/spec/services/spam/spam_action_service_spec.rb @@ -9,11 +9,11 @@ RSpec.describe Spam::SpamActionService do let(:issue) { create(:issue, project: project, author: user) } let(:fake_ip) { '1.2.3.4' } let(:fake_user_agent) { 'fake-user-agent' } - let(:fake_referrer) { 'fake-http-referrer' } + let(:fake_referer) { 'fake-http-referer' } let(:env) do { 'action_dispatch.remote_ip' => fake_ip, 'HTTP_USER_AGENT' => fake_user_agent, - 'HTTP_REFERRER' => fake_referrer } + 'HTTP_REFERER' => fake_referer } end let_it_be(:project) { create(:project, :public) } @@ -80,7 +80,7 @@ RSpec.describe Spam::SpamActionService do { ip_address: fake_ip, user_agent: fake_user_agent, - referrer: fake_referrer + referer: fake_referer } end @@ -222,6 +222,38 @@ RSpec.describe Spam::SpamActionService do end end + context 'spam verdict service advises to block the user' do + before do + allow(fake_verdict_service).to receive(:execute).and_return(BLOCK_USER) + end + + context 'when allow_possible_spam feature flag is false' do + before do + stub_feature_flags(allow_possible_spam: false) + end + + it_behaves_like 'only checks for spam if a request is provided' + + it 'marks as spam' do + response = subject + + expect(response.message).to match(expected_service_check_response_message) + expect(issue).to be_spam + end + end + + context 'when allow_possible_spam feature flag is true' do + it_behaves_like 'only checks for spam if a request is provided' + + it 'does not mark as spam' do + response = subject + + expect(response.message).to match(expected_service_check_response_message) + expect(issue).not_to be_spam + end + end + end + context 'when spam verdict service conditionally allows' do before do allow(fake_verdict_service).to receive(:execute).and_return(CONDITIONAL_ALLOW) diff --git a/spec/services/spam/spam_verdict_service_spec.rb b/spec/services/spam/spam_verdict_service_spec.rb index 14b788e3a86..1ec5e1f1331 100644 --- a/spec/services/spam/spam_verdict_service_spec.rb +++ b/spec/services/spam/spam_verdict_service_spec.rb @@ -7,18 +7,18 @@ RSpec.describe Spam::SpamVerdictService do let(:fake_ip) { '1.2.3.4' } let(:fake_user_agent) { 'fake-user-agent' } - let(:fake_referrer) { 'fake-http-referrer' } + let(:fake_referer) { 'fake-http-referer' } let(:env) do { 'action_dispatch.remote_ip' => fake_ip, 'HTTP_USER_AGENT' => fake_user_agent, - 'HTTP_REFERRER' => fake_referrer } + 'HTTP_REFERER' => fake_referer } end let(:request) { double(:request, env: env) } let(:check_for_spam) { true } let_it_be(:user) { create(:user) } - let(:issue) { build(:issue, author: user) } + let_it_be(:issue) { create(:issue, author: user) } let(:service) do described_class.new(user: user, target: issue, request: request, options: {}) end @@ -28,7 +28,7 @@ RSpec.describe Spam::SpamVerdictService do before do allow(service).to receive(:akismet_verdict).and_return(nil) - allow(service).to receive(:external_verdict).and_return(nil) + allow(service).to receive(:spamcheck_verdict).and_return(nil) end context 'if all services return nil' do @@ -63,7 +63,7 @@ RSpec.describe Spam::SpamVerdictService do context 'and they are supported' do before do allow(service).to receive(:akismet_verdict).and_return(DISALLOW) - allow(service).to receive(:external_verdict).and_return(BLOCK_USER) + allow(service).to receive(:spamcheck_verdict).and_return(BLOCK_USER) end it 'renders the more restrictive verdict' do @@ -74,7 +74,7 @@ RSpec.describe Spam::SpamVerdictService do context 'and one is supported' do before do allow(service).to receive(:akismet_verdict).and_return('nonsense') - allow(service).to receive(:external_verdict).and_return(BLOCK_USER) + allow(service).to receive(:spamcheck_verdict).and_return(BLOCK_USER) end it 'renders the more restrictive verdict' do @@ -85,7 +85,7 @@ RSpec.describe Spam::SpamVerdictService do context 'and none are supported' do before do allow(service).to receive(:akismet_verdict).and_return('nonsense') - allow(service).to receive(:external_verdict).and_return('rubbish') + allow(service).to receive(:spamcheck_verdict).and_return('rubbish') end it 'renders the more restrictive verdict' do @@ -150,48 +150,87 @@ RSpec.describe Spam::SpamVerdictService do end end - describe '#external_verdict' do - subject { service.send(:external_verdict) } + describe '#spamcheck_verdict' do + subject { service.send(:spamcheck_verdict) } context 'if a Spam Check endpoint enabled and set to a URL' do let(:spam_check_body) { {} } - let(:spam_check_http_status) { nil } + let(:endpoint_url) { "grpc://www.spamcheckurl.com/spam_check" } + + let(:spam_client) do + Gitlab::Spamcheck::Client.new + end before do stub_application_setting(spam_check_endpoint_enabled: true) - stub_application_setting(spam_check_endpoint_url: "http://www.spamcheckurl.com/spam_check") - stub_request(:post, /.*spamcheckurl.com.*/).to_return( body: spam_check_body.to_json, status: spam_check_http_status ) + stub_application_setting(spam_check_endpoint_url: endpoint_url) end context 'if the endpoint is accessible' do - let(:spam_check_http_status) { 200 } - let(:error) { nil } + let(:error) { '' } let(:verdict) { nil } - let(:spam_check_body) do - { verdict: verdict, error: error } + + before do + allow(service).to receive(:spamcheck_client).and_return(spam_client) + allow(spam_client).to receive(:issue_spam?).and_return([verdict, error]) end context 'the result is a valid verdict' do - let(:verdict) { 'allow' } + let(:verdict) { ALLOW } it 'returns the verdict' do expect(subject).to eq ALLOW end end - context 'the verdict is an unexpected string' do - let(:verdict) { 'this is fine' } + context 'when recaptcha is enabled' do + before do + allow(Gitlab::Recaptcha).to receive(:enabled?).and_return(true) + end - it 'returns the string' do - expect(subject).to eq verdict + using RSpec::Parameterized::TableSyntax + + # rubocop: disable Lint/BinaryOperatorWithIdenticalOperands + where(:verdict_value, :expected) do + ::Spam::SpamConstants::ALLOW | ::Spam::SpamConstants::ALLOW + ::Spam::SpamConstants::CONDITIONAL_ALLOW | ::Spam::SpamConstants::CONDITIONAL_ALLOW + ::Spam::SpamConstants::DISALLOW | ::Spam::SpamConstants::CONDITIONAL_ALLOW + ::Spam::SpamConstants::BLOCK_USER | ::Spam::SpamConstants::CONDITIONAL_ALLOW + end + # rubocop: enable Lint/BinaryOperatorWithIdenticalOperands + + with_them do + let(:verdict) { verdict_value } + + it "returns expected spam constant" do + expect(subject).to eq(expected) + end end end - context 'the JSON is malformed' do - let(:spam_check_body) { 'this is fine' } + context 'when recaptcha is disabled' do + before do + allow(Gitlab::Recaptcha).to receive(:enabled?).and_return(false) + end - it 'returns allow' do - expect(subject).to eq ALLOW + [::Spam::SpamConstants::ALLOW, + ::Spam::SpamConstants::CONDITIONAL_ALLOW, + ::Spam::SpamConstants::DISALLOW, + ::Spam::SpamConstants::BLOCK_USER].each do |verdict_value| + let(:verdict) { verdict_value } + let(:expected) { verdict_value } + + it "returns expected spam constant" do + expect(subject).to eq(expected) + end + end + end + + context 'the verdict is an unexpected value' do + let(:verdict) { :this_is_fine } + + it 'returns the string' do + expect(subject).to eq verdict end end @@ -219,11 +258,13 @@ RSpec.describe Spam::SpamVerdictService do end end - context 'the HTTP status is not 200' do - let(:spam_check_http_status) { 500 } + context 'the requested is aborted' do + before do + allow(spam_client).to receive(:issue_spam?).and_raise(GRPC::Aborted) + end it 'returns nil' do - expect(subject).to be_nil + expect(subject).to be(ALLOW) end end @@ -239,11 +280,11 @@ RSpec.describe Spam::SpamVerdictService do context 'if the endpoint times out' do before do - stub_request(:post, /.*spamcheckurl.com.*/).to_timeout + allow(spam_client).to receive(:issue_spam?).and_raise(GRPC::DeadlineExceeded) end it 'returns nil' do - expect(subject).to be_nil + expect(subject).to be(ALLOW) end end end