Commit 722fc84e authored by Patricio Cano's avatar Patricio Cano

Complete refactor of the `Spammable` concern and tests:

- Merged `AkismetSubmittable` into `Spammable`
- Clean up `SpamCheckService`
- Added tests for `Spammable`
- Added submit (ham or spam) options to `AkismetHelper`
parent 95419679
module AkismetSubmittable
extend ActiveSupport::Concern
included do
has_one :user_agent_detail, as: :subject
end
def can_be_submitted?
if user_agent_detail
user_agent_detail.submittable?
else
false
end
end
end
module Spammable module Spammable
extend ActiveSupport::Concern extend ActiveSupport::Concern
include Gitlab::AkismetHelper
module ClassMethods
def attr_spammable(*attrs)
attrs.each do |attr|
spammable_attrs << attr.to_s
end
end
end
included do included do
has_one :user_agent_detail, as: :subject, dependent: :destroy
attr_accessor :spam attr_accessor :spam
after_validation :check_for_spam, on: :create after_validation :check_for_spam, on: :create
cattr_accessor :spammable_attrs, instance_accessor: false do
[]
end
end
def can_be_submitted?
if user_agent_detail
user_agent_detail.submittable?
else
false
end
end
def submit_ham
return unless akismet_enabled? && can_be_submitted?
ham!(user_agent_detail, spammable_text, creator)
end end
def spam? def submit_spam
return unless akismet_enabled? && can_be_submitted?
spam!(user_agent_detail, spammable_text, creator)
end
def spam?(env, user)
is_spam?(env, user, spammable_text)
end
def spam_detected?
@spam @spam
end end
def check_for_spam def check_for_spam
self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam? self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam_detected?
end
private
def spammable_text
result = []
self.class.spammable_attrs.each do |entry|
result << self.send(entry)
end
result.reject(&:blank?).join("\n")
end
def creator
if self.author_id
User.find(self.author_id)
elsif self.creator_id
User.find(self.creator_id)
end
end end
end end
...@@ -37,6 +37,8 @@ class Issue < ActiveRecord::Base ...@@ -37,6 +37,8 @@ class Issue < ActiveRecord::Base
scope :order_due_date_asc, -> { reorder('issues.due_date IS NULL, issues.due_date ASC') } scope :order_due_date_asc, -> { reorder('issues.due_date IS NULL, issues.due_date ASC') }
scope :order_due_date_desc, -> { reorder('issues.due_date IS NULL, issues.due_date DESC') } scope :order_due_date_desc, -> { reorder('issues.due_date IS NULL, issues.due_date DESC') }
attr_spammable :title, :description
state_machine :state, initial: :opened do state_machine :state, initial: :opened do
event :close do event :close do
transition [:reopened, :opened] => :closed transition [:reopened, :opened] => :closed
......
...@@ -8,7 +8,7 @@ module Issues ...@@ -8,7 +8,7 @@ module Issues
issue = project.issues.new(params) issue = project.issues.new(params)
issue.author = params[:author] || current_user issue.author = params[:author] || current_user
issue.spam = spam_check_service.execute(request, api) issue.spam = spam_check_service.execute(request, api, issue)
if issue.save if issue.save
issue.update_attributes(label_ids: label_params) issue.update_attributes(label_ids: label_params)
......
class SpamCheckService < BaseService class SpamCheckService < BaseService
include Gitlab::AkismetHelper attr_accessor :request, :api, :subject
attr_accessor :request, :api def execute(request, api, subject)
@request, @api, @subject = request, api, subject
def execute(request, api) return false unless request || subject.check_for_spam?(project)
@request, @api = request, api return false unless subject.spam?(request.env, current_user)
return false unless request || check_for_spam?(project)
return false unless is_spam?(request.env, current_user, text)
create_spam_log create_spam_log
...@@ -15,19 +13,15 @@ class SpamCheckService < BaseService ...@@ -15,19 +13,15 @@ class SpamCheckService < BaseService
private private
def text
[params[:title], params[:description]].reject(&:blank?).join("\n")
end
def spam_log_attrs def spam_log_attrs
{ {
user_id: current_user.id, user_id: current_user.id,
project_id: project.id, project_id: project.id,
title: params[:title], title: params[:title],
description: params[:description], description: params[:description],
source_ip: client_ip(request.env), source_ip: subject.client_ip(request.env),
user_agent: user_agent(request.env), user_agent: subject.user_agent(request.env),
noteable_type: 'Issue', noteable_type: subject.class.to_s,
via_api: api via_api: api
} }
end end
......
...@@ -160,7 +160,7 @@ module API ...@@ -160,7 +160,7 @@ module API
issue = ::Issues::CreateService.new(project, current_user, attrs.merge(request: request, api: true)).execute issue = ::Issues::CreateService.new(project, current_user, attrs.merge(request: request, api: true)).execute
if issue.spam? if issue.spam_detected?
render_api_error!({ error: 'Spam detected' }, 400) render_api_error!({ error: 'Spam detected' }, 400)
end end
......
...@@ -43,5 +43,39 @@ module Gitlab ...@@ -43,5 +43,39 @@ module Gitlab
false false
end end
end end
def ham!(details, text, user)
client = akismet_client
params = {
type: 'comment',
text: text,
author: user.name,
author_email: user.email
}
begin
client.submit_ham(details.ip_address, details.user_agent, params)
rescue => e
Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
end
end
def spam!(details, text, user)
client = akismet_client
params = {
type: 'comment',
text: text,
author: user.name,
author_email: user.email
}
begin
client.submit_spam(details.ip_address, details.user_agent, params)
rescue => e
Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
end
end
end end
end end
...@@ -2,6 +2,8 @@ FactoryGirl.define do ...@@ -2,6 +2,8 @@ FactoryGirl.define do
factory :user_agent_detail do factory :user_agent_detail do
ip_address '127.0.0.1' ip_address '127.0.0.1'
user_agent 'AppleWebKit/537.36' user_agent 'AppleWebKit/537.36'
subject_id 1
subject_type 'Issue'
trait :on_issue do trait :on_issue do
association :subject, factory: :issue association :subject, factory: :issue
......
require 'spec_helper'
describe Issue, 'Spammable' do
let(:issue) { create(:issue, description: 'Test Desc.') }
describe 'Associations' do
it { is_expected.to have_one(:user_agent_detail).dependent(:destroy) }
end
describe 'ClassMethods' do
it 'should return correct attr_spammable' do
expect(issue.send(:spammable_text)).to eq("#{issue.title}\n#{issue.description}")
end
end
describe 'InstanceMethods' do
it 'should return the correct creator' do
expect(issue.send(:creator).id).to eq(issue.author_id)
end
it 'should be invalid if spam' do
issue.spam = true
expect(issue.valid?).to be_truthy
end
it 'should be submittable' do
create(:user_agent_detail, subject_id: issue.id, subject_type: issue.class.to_s)
expect(issue.can_be_submitted?).to be_truthy
end
end
describe 'AkismetMethods' do
before do
allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(check_for_spam?: true, is_spam?: true, ham!: nil, spam!: nil)
end
it { expect(issue.spam?(:mock_env, :mock_user)).to be_truthy }
it { expect(issue.submit_spam).to be_nil }
it { expect(issue.submit_ham).to be_nil }
end
end
...@@ -13,10 +13,5 @@ describe UserAgentDetail, type: :model do ...@@ -13,10 +13,5 @@ describe UserAgentDetail, type: :model do
detail = create(:user_agent_detail, :on_issue) detail = create(:user_agent_detail, :on_issue)
expect(detail).to be_valid expect(detail).to be_valid
end end
it 'should not be valid without a subject' do
detail = build(:user_agent_detail)
expect(detail).not_to be_valid
end
end 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