diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2017-02-07 17:21:13 +0300 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2017-02-07 17:21:13 +0300 |
commit | 38cb0347b2ae8dc3d8bd61dd81f5f10af688562b (patch) | |
tree | f668cf3725116b72e1cdbc3c95be76a77be6b922 | |
parent | 8d5ae0d99f9194b3c6c97f5f90883fe1f07298f8 (diff) | |
parent | 3d2954e4570d236a080b0d46698d96a28fd9acec (diff) |
Merge branch '21518_recaptcha_spam_issues' into 'master'
Use reCaptcha when an issue identified as spam
Closes #21518
See merge request !8846
20 files changed, 400 insertions, 32 deletions
diff --git a/app/assets/stylesheets/pages/issues.scss b/app/assets/stylesheets/pages/issues.scss index 8734a3b1598..1e605337f09 100644 --- a/app/assets/stylesheets/pages/issues.scss +++ b/app/assets/stylesheets/pages/issues.scss @@ -148,3 +148,7 @@ ul.related-merge-requests > li { border: 1px solid $border-gray-normal; } } + +.recaptcha { + margin-bottom: 30px; +} diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index 562f92bd83c..a6891149bfa 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -1,6 +1,8 @@ module SpammableActions extend ActiveSupport::Concern + include Recaptcha::Verify + included do before_action :authorize_submit_spammable!, only: :mark_as_spam end @@ -15,6 +17,15 @@ module SpammableActions private + def recaptcha_params + return {} unless params[:recaptcha_verification] && Gitlab::Recaptcha.load_configurations! && verify_recaptcha + + { + recaptcha_verified: true, + spam_log_id: params[:spam_log_id] + } + end + def spammable raise NotImplementedError, "#{self.class} does not implement #{__method__}" end @@ -22,4 +33,11 @@ module SpammableActions def authorize_submit_spammable! access_denied! unless current_user.admin? end + + def render_recaptcha? + return false if spammable.errors.count > 1 # re-render "new" template in case there are other errors + return false unless Gitlab::Recaptcha.enabled? + + spammable.spam + end end diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 8472ceca329..c75b8987a4b 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -93,15 +93,13 @@ class Projects::IssuesController < Projects::ApplicationController def create extra_params = { request: request, merge_request_for_resolving_discussions: merge_request_for_resolving_discussions } + extra_params.merge!(recaptcha_params) + @issue = Issues::CreateService.new(project, current_user, issue_params.merge(extra_params)).execute respond_to do |format| format.html do - if @issue.valid? - redirect_to issue_path(@issue) - else - render :new - end + html_response_create end format.js do @link = @issue.attachment.url.to_js @@ -178,6 +176,20 @@ class Projects::IssuesController < Projects::ApplicationController protected + def html_response_create + if @issue.valid? + redirect_to issue_path(@issue) + elsif render_recaptcha? + if params[:recaptcha_verification] + flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' + end + + render :verify + else + render :new + end + end + def issue # The Sortable default scope causes performance issues when used with find_by @noteable = @issue ||= @project.issues.where(iid: params[:id]).reorder(nil).take || redirect_old diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index bf27f3d4d51..68bf01ba08d 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -17,7 +17,7 @@ class RegistrationsController < Devise::RegistrationsController if !Gitlab::Recaptcha.load_configurations! || verify_recaptcha super else - flash[:alert] = 'There was an error with the reCAPTCHA. Please re-solve the reCAPTCHA.' + flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' flash.delete :recaptcha_error render action: 'new' end @@ -30,7 +30,7 @@ class RegistrationsController < Devise::RegistrationsController format.html do session.try(:destroy) redirect_to new_user_session_path, notice: "Account successfully removed." - end + end end end diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 1acff093aa1..423ae98a60e 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -11,6 +11,7 @@ module Spammable has_one :user_agent_detail, as: :subject, dependent: :destroy attr_accessor :spam + attr_accessor :spam_log after_validation :check_for_spam, on: :create @@ -34,9 +35,14 @@ module Spammable end def check_for_spam - if spam? - self.errors.add(:base, "Your #{spammable_entity_type} has been recognized as spam and has been discarded.") - end + error_msg = if Gitlab::Recaptcha.enabled? + "Your #{spammable_entity_type} has been recognized as spam. "\ + "You can still submit it by solving Captcha." + else + "Your #{spammable_entity_type} has been recognized as spam and has been discarded." + end + + self.errors.add(:base, error_msg) if spam? end def spammable_entity_type diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index d2eb46ac41b..c9168f74249 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -3,6 +3,8 @@ module Issues def execute @request = params.delete(:request) @api = params.delete(:api) + @recaptcha_verified = params.delete(:recaptcha_verified) + @spam_log_id = params.delete(:spam_log_id) issue_attributes = params.merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions) @issue = BuildService.new(project, current_user, issue_attributes).execute @@ -11,7 +13,13 @@ module Issues end def before_create(issuable) - issuable.spam = spam_service.check(@api) + if @recaptcha_verified + spam_log = current_user.spam_logs.find_by(id: @spam_log_id, title: issuable.title) + spam_log.update!(recaptcha_verified: true) if spam_log + else + issuable.spam = spam_service.check(@api) + issuable.spam_log = spam_service.spam_log + end end def after_create(issuable) @@ -35,7 +43,7 @@ module Issues private def spam_service - SpamService.new(@issue, @request) + @spam_service ||= SpamService.new(@issue, @request) end def user_agent_detail_service diff --git a/app/services/spam_service.rb b/app/services/spam_service.rb index 48903291799..024a7c19d33 100644 --- a/app/services/spam_service.rb +++ b/app/services/spam_service.rb @@ -1,5 +1,6 @@ class SpamService attr_accessor :spammable, :request, :options + attr_reader :spam_log def initialize(spammable, request = nil) @spammable = spammable @@ -63,7 +64,7 @@ class SpamService end def create_spam_log(api) - SpamLog.create( + @spam_log = SpamLog.create!( { user_id: spammable_owner_id, title: spammable.spam_title, diff --git a/app/views/admin/spam_logs/_spam_log.html.haml b/app/views/admin/spam_logs/_spam_log.html.haml index 4ce4eab8753..33f6d847782 100644 --- a/app/views/admin/spam_logs/_spam_log.html.haml +++ b/app/views/admin/spam_logs/_spam_log.html.haml @@ -14,6 +14,8 @@ %td = spam_log.via_api? ? 'Y' : 'N' %td + = spam_log.recaptcha_verified ? 'Y' : 'N' + %td = spam_log.noteable_type %td = spam_log.title diff --git a/app/views/admin/spam_logs/index.html.haml b/app/views/admin/spam_logs/index.html.haml index 0fdd5bd9960..8aaa6379730 100644 --- a/app/views/admin/spam_logs/index.html.haml +++ b/app/views/admin/spam_logs/index.html.haml @@ -10,6 +10,7 @@ %th User %th Source IP %th API? + %th Recaptcha verified? %th Type %th Title %th Description diff --git a/app/views/devise/shared/_signup_box.html.haml b/app/views/devise/shared/_signup_box.html.haml index 01ecf237925..5a44ec45b7b 100644 --- a/app/views/devise/shared/_signup_box.html.haml +++ b/app/views/devise/shared/_signup_box.html.haml @@ -23,7 +23,7 @@ = f.password_field :password, class: "form-control bottom", required: true, pattern: ".{#{@minimum_password_length},}", title: "Minimum length is #{@minimum_password_length} characters." %p.gl-field-hint Minimum length is #{@minimum_password_length} characters %div - - if current_application_settings.recaptcha_enabled + - if Gitlab::Recaptcha.enabled? = recaptcha_tags %div = f.submit "Register", class: "btn-register btn" diff --git a/app/views/projects/issues/verify.html.haml b/app/views/projects/issues/verify.html.haml new file mode 100644 index 00000000000..1934b18c086 --- /dev/null +++ b/app/views/projects/issues/verify.html.haml @@ -0,0 +1,20 @@ +- page_title "Anti-spam verification" + +%h3.page-title + Anti-spam verification +%hr + +%p + We detected potential spam in the issue description. Please verify that you are not a robot to submit the issue. + += form_for [@project.namespace.becomes(Namespace), @project, @issue] do |f| + .recaptcha + - params[:issue].each do |field, value| + = hidden_field(:issue, field, value: value) + = hidden_field_tag(:merge_request_for_resolving_discussions, params[:merge_request_for_resolving_discussions]) + = hidden_field_tag(:spam_log_id, @issue.spam_log.id) + = hidden_field_tag(:recaptcha_verification, true) + = recaptcha_tags + + .row-content-block.footer-block + = f.submit "Submit #{@issue.class.model_name.human.downcase}", class: 'btn btn-create' diff --git a/changelogs/unreleased/21518_recaptcha_spam_issues.yml b/changelogs/unreleased/21518_recaptcha_spam_issues.yml new file mode 100644 index 00000000000..bd6c9d7521e --- /dev/null +++ b/changelogs/unreleased/21518_recaptcha_spam_issues.yml @@ -0,0 +1,4 @@ +--- +title: Use reCaptcha when an issue is identified as a spam +merge_request: 8846 +author: diff --git a/db/migrate/20170206071414_add_recaptcha_verified_to_spam_logs.rb b/db/migrate/20170206071414_add_recaptcha_verified_to_spam_logs.rb new file mode 100644 index 00000000000..44372334d21 --- /dev/null +++ b/db/migrate/20170206071414_add_recaptcha_verified_to_spam_logs.rb @@ -0,0 +1,15 @@ +class AddRecaptchaVerifiedToSpamLogs < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + DOWNTIME = false + + def up + add_column_with_default(:spam_logs, :recaptcha_verified, :boolean, default: false) + end + + def down + remove_column(:spam_logs, :recaptcha_verified) + end +end diff --git a/db/schema.rb b/db/schema.rb index 1099397824d..aeb0d8210f0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170204181513) do +ActiveRecord::Schema.define(version: 20170206071414) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1114,6 +1114,7 @@ ActiveRecord::Schema.define(version: 20170204181513) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.boolean "submitted_as_ham", default: false, null: false + t.boolean "recaptcha_verified", default: false, null: false end create_table "subscriptions", force: :cascade do |t| diff --git a/lib/gitlab/recaptcha.rb b/lib/gitlab/recaptcha.rb index 70e7f25d518..4bc76ea033f 100644 --- a/lib/gitlab/recaptcha.rb +++ b/lib/gitlab/recaptcha.rb @@ -10,5 +10,9 @@ module Gitlab true end end + + def self.enabled? + current_application_settings.recaptcha_enabled + end end end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 5f27f336f72..4b89381eb96 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -326,7 +326,7 @@ describe Projects::IssuesController do end describe 'POST #create' do - def post_new_issue(attrs = {}) + def post_new_issue(issue_attrs = {}, additional_params = {}) sign_in(user) project = create(:empty_project, :public) project.team << [user, :developer] @@ -334,8 +334,8 @@ describe Projects::IssuesController do post :create, { namespace_id: project.namespace.to_param, project_id: project.to_param, - issue: { title: 'Title', description: 'Description' }.merge(attrs) - } + issue: { title: 'Title', description: 'Description' }.merge(issue_attrs) + }.merge(additional_params) project.issues.first end @@ -378,24 +378,81 @@ describe Projects::IssuesController do context 'Akismet is enabled' do before do + stub_application_setting(recaptcha_enabled: true) allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) - allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) end - def post_spam_issue - post_new_issue(title: 'Spam Title', description: 'Spam lives here') - end + context 'when an issue is not identified as a spam' do + before do + allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false) + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(false) + end - it 'rejects an issue recognized as spam' do - expect{ post_spam_issue }.not_to change(Issue, :count) - expect(response).to render_template(:new) + it 'does not create an issue' do + expect { post_new_issue(title: '') }.not_to change(Issue, :count) + end end - it 'creates a spam log' do - post_spam_issue - spam_logs = SpamLog.all - expect(spam_logs.count).to eq(1) - expect(spam_logs[0].title).to eq('Spam Title') + context 'when an issue is identified as a spam' do + before { allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) } + + context 'when captcha is not verified' do + def post_spam_issue + post_new_issue(title: 'Spam Title', description: 'Spam lives here') + end + + before { allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false) } + + it 'rejects an issue recognized as a spam' do + expect { post_spam_issue }.not_to change(Issue, :count) + end + + it 'creates a spam log' do + post_spam_issue + spam_logs = SpamLog.all + + expect(spam_logs.count).to eq(1) + expect(spam_logs.first.title).to eq('Spam Title') + expect(spam_logs.first.recaptcha_verified).to be_falsey + end + + it 'does not create an issue when it is not valid' do + expect { post_new_issue(title: '') }.not_to change(Issue, :count) + end + + it 'does not create an issue when recaptcha is not enabled' do + stub_application_setting(recaptcha_enabled: false) + + expect { post_spam_issue }.not_to change(Issue, :count) + end + end + + context 'when captcha is verified' do + let!(:spam_logs) { create_list(:spam_log, 2, user: user, title: 'Title') } + + def post_verified_issue + post_new_issue({}, { spam_log_id: spam_logs.last.id, recaptcha_verification: true } ) + end + + before do + allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(true) + end + + it 'accepts an issue after recaptcha is verified' do + expect { post_verified_issue }.to change(Issue, :count) + end + + it 'marks spam log as recaptcha_verified' do + expect { post_verified_issue }.to change { SpamLog.last.recaptcha_verified }.from(false).to(true) + end + + it 'does not mark spam log as recaptcha_verified when it does not belong to current_user' do + spam_log = create(:spam_log) + + expect { post_new_issue({}, { spam_log_id: spam_log.id, recaptcha_verification: true } ) }. + not_to change { SpamLog.last.recaptcha_verified } + end + end end end @@ -405,7 +462,7 @@ describe Projects::IssuesController do end it 'creates a user agent detail' do - expect{ post_new_issue }.to change(UserAgentDetail, :count).by(1) + expect { post_new_issue }.to change(UserAgentDetail, :count).by(1) end end diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index 42fbfe89368..8cc216445eb 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -44,7 +44,7 @@ describe RegistrationsController do post(:create, user_params) expect(response).to render_template(:new) - expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please re-solve the reCAPTCHA.' + expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' end it 'redirects to the dashboard when the recaptcha is solved' do diff --git a/spec/features/issues/spam_issues_spec.rb b/spec/features/issues/spam_issues_spec.rb new file mode 100644 index 00000000000..4bc9b49f889 --- /dev/null +++ b/spec/features/issues/spam_issues_spec.rb @@ -0,0 +1,66 @@ +require 'rails_helper' + +describe 'New issue', feature: true do + include StubENV + + let(:project) { create(:project, :public) } + let(:user) { create(:user)} + + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + + current_application_settings.update!( + akismet_enabled: true, + akismet_api_key: 'testkey', + recaptcha_enabled: true, + recaptcha_site_key: 'test site key', + recaptcha_private_key: 'test private key' + ) + + project.team << [user, :master] + login_as(user) + end + + context 'when identified as a spam' do + before do + WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: "true", status: 200) + + visit new_namespace_project_issue_path(project.namespace, project) + end + + it 'creates an issue after solving reCaptcha' do + fill_in 'issue_title', with: 'issue title' + fill_in 'issue_description', with: 'issue description' + + click_button 'Submit issue' + + # it is impossible to test recaptcha automatically and there is no possibility to fill in recaptcha + # recaptcha verification is skipped in test environment and it always returns true + expect(page).not_to have_content('issue title') + expect(page).to have_css('.recaptcha') + + click_button 'Submit issue' + + expect(page.find('.issue-details h2.title')).to have_content('issue title') + expect(page.find('.issue-details .description')).to have_content('issue description') + end + end + + context 'when not identified as a spam' do + before do + WebMock.stub_request(:any, /.*akismet.com.*/).to_return(body: 'false', status: 200) + + visit new_namespace_project_issue_path(project.namespace, project) + end + + it 'creates an issue' do + fill_in 'issue_title', with: 'issue title' + fill_in 'issue_description', with: 'issue description' + + click_button 'Submit issue' + + expect(page.find('.issue-details h2.title')).to have_content('issue title') + expect(page.find('.issue-details .description')).to have_content('issue description') + end + end +end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index ac3834c32ff..30578ee4c7d 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -181,5 +181,107 @@ describe Issues::CreateService, services: true do expect(issue.title).to be_nil end end + + context 'checking spam' do + let(:opts) do + { + title: 'Awesome issue', + description: 'please fix', + request: double(:request, env: {}) + } + end + + before do + allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) + end + + context 'when recaptcha was verified' do + let(:log_user) { user } + let(:spam_logs) { create_list(:spam_log, 2, user: log_user, title: 'Awesome issue') } + + before do + opts[:recaptcha_verified] = true + opts[:spam_log_id] = spam_logs.last.id + + expect(AkismetService).not_to receive(:new) + end + + it 'does no mark an issue as a spam ' do + expect(issue).not_to be_spam + end + + it 'an issue is valid ' do + expect(issue.valid?).to be_truthy + end + + it 'does not assign a spam_log to an issue' do + expect(issue.spam_log).to be_nil + end + + it 'marks related spam_log as recaptcha_verified' do + expect { issue }.to change{SpamLog.last.recaptcha_verified}.from(false).to(true) + end + + context 'when spam log does not belong to a user' do + let(:log_user) { create(:user) } + + it 'does not mark spam_log as recaptcha_verified' do + expect { issue }.not_to change{SpamLog.last.recaptcha_verified} + end + end + + context 'when spam log title does not match the issue title' do + before do + opts[:title] = 'Another issue' + end + + it 'does not mark spam_log as recaptcha_verified' do + expect { issue }.not_to change{SpamLog.last.recaptcha_verified} + end + end + end + + context 'when recaptcha was not verified' do + context 'when akismet detects spam' do + before do + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + end + + it 'marks an issue as a spam ' do + expect(issue).to be_spam + end + + it 'an issue is not valid ' do + expect(issue.valid?).to be_falsey + end + + it 'creates a new spam_log' do + expect{issue}.to change{SpamLog.count}.from(0).to(1) + end + + it 'assigns a spam_log to an issue' do + expect(issue.spam_log).to eq(SpamLog.last) + end + end + + context 'when akismet does not detect spam' do + before do + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(false) + end + + it 'does not mark an issue as a spam ' do + expect(issue).not_to be_spam + end + + it 'an issue is valid ' do + expect(issue.valid?).to be_truthy + end + + it 'does not assign a spam_log to an issue' do + expect(issue.spam_log).to be_nil + end + end + end + end end end diff --git a/spec/services/spam_service_spec.rb b/spec/services/spam_service_spec.rb new file mode 100644 index 00000000000..271c17dd8c0 --- /dev/null +++ b/spec/services/spam_service_spec.rb @@ -0,0 +1,47 @@ +require 'spec_helper' + +describe SpamService, services: true do + describe '#check' do + let(:project) { create(:project, :public) } + let(:issue) { create(:issue, project: project) } + let(:request) { double(:request, env: {}) } + + def check_spam(issue, request) + described_class.new(issue, request).check + end + + context 'when indicated as spam by akismet' do + before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: true)) } + + it 'returns false when request is missing' do + expect(check_spam(issue, nil)).to be_falsey + end + + it 'returns false when issue is not public' do + issue = create(:issue, project: create(:project, :private)) + + expect(check_spam(issue, request)).to be_falsey + end + + it 'returns true' do + expect(check_spam(issue, request)).to be_truthy + end + + it 'creates a spam log' do + expect { check_spam(issue, request) }.to change { SpamLog.count }.from(0).to(1) + end + end + + context 'when not indicated as spam by akismet' do + before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: false)) } + + it 'returns false' do + expect(check_spam(issue, request)).to be_falsey + end + + it 'does not create a spam log' do + expect { check_spam(issue, request) }.not_to change { SpamLog.count } + end + end + end +end |