Merge branch '21518_recaptcha_spam_issues' into 'master'

Use reCaptcha when an issue identified as spam

Closes #21518

See merge request !8846

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;
+}

app/controllers/concerns/spammable_actions.rb

 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

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

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
 

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

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

app/services/spam_service.rb

 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,

app/views/admin/spam_logs/_spam_log.html.haml

@@ -13,6 +13,8 @@
     = spam_log.source_ip
   %td
     = spam_log.via_api? ? 'Y' : 'N'
+  %td
+    = spam_log.recaptcha_verified ? 'Y' : 'N'
   %td
     = spam_log.noteable_type
   %td

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

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"

app/views/projects/issues/verify.html.haml

+- 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'

changelogs/unreleased/21518_recaptcha_spam_issues.yml

+---
+title: Use reCaptcha when an issue is identified as a spam
+merge_request: 8846
+author:

db/migrate/20170206071414_add_recaptcha_verified_to_spam_logs.rb

+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

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|

lib/gitlab/recaptcha.rb

@@ -10,5 +10,9 @@ module Gitlab
         true
       end
     end
+
+    def self.enabled?
+      current_application_settings.recaptcha_enabled
+    end
   end
 end

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
 

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

spec/features/issues/spam_issues_spec.rb

+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

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

spec/services/spam_service_spec.rb

+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