Commit 43e756d4 authored by Patricio Cano's avatar Patricio Cano

Refactored AkismetHelper into AkismetService and cleaned up `Spammable`

- Refactored SpamCheckService into SpamService
parent 7179165a
class Admin::SpamLogsController < Admin::ApplicationController class Admin::SpamLogsController < Admin::ApplicationController
include Gitlab::AkismetHelper
def index def index
@spam_logs = SpamLog.order(id: :desc).page(params[:page]) @spam_logs = SpamLog.order(id: :desc).page(params[:page])
...@@ -20,8 +19,7 @@ class Admin::SpamLogsController < Admin::ApplicationController ...@@ -20,8 +19,7 @@ class Admin::SpamLogsController < Admin::ApplicationController
def mark_as_ham def mark_as_ham
spam_log = SpamLog.find(params[:id]) spam_log = SpamLog.find(params[:id])
if ham!(spam_log.source_ip, spam_log.user_agent, spam_log.text, spam_log.user) if SpamService.new(spam_log).mark_as_ham!
spam_log.update_attribute(:submitted_as_ham, true)
redirect_to admin_spam_logs_path, notice: 'Spam log successfully submitted as ham.' redirect_to admin_spam_logs_path, notice: 'Spam log successfully submitted as ham.'
else else
redirect_to admin_spam_logs_path, notice: 'Error with Akismet. Please check the logs for more info.' redirect_to admin_spam_logs_path, notice: 'Error with Akismet. Please check the logs for more info.'
......
...@@ -6,13 +6,7 @@ module SpammableActions ...@@ -6,13 +6,7 @@ module SpammableActions
end end
def mark_as_spam def mark_as_spam
if spammable.submit_spam if SpamService.new(spammable).mark_as_spam!(current_user)
spammable.user_agent_detail.update_attribute(:submitted, true)
if spammable.is_a?(Issuable)
SystemNoteService.submit_spam(spammable, spammable.project, current_user)
end
redirect_to spammable, notice: 'Issue was submitted to Akismet successfully.' redirect_to spammable, notice: 'Issue was submitted to Akismet successfully.'
else else
flash[:error] = 'Error with Akismet. Please check the logs for more info.' flash[:error] = 'Error with Akismet. Please check the logs for more info.'
......
module Spammable module Spammable
extend ActiveSupport::Concern extend ActiveSupport::Concern
include Gitlab::AkismetHelper
module ClassMethods module ClassMethods
def attr_spammable(*attrs) def attr_spammable(attr, options = {})
attrs.each do |attr| spammable_attrs << [attr.to_s, options]
spammable_attrs << attr.to_s
end
end end
end end
included do included do
has_one :user_agent_detail, as: :subject, dependent: :destroy has_one :user_agent_detail, as: :subject, dependent: :destroy
attr_accessor :spam attr_accessor :spam
after_validation :check_for_spam, on: :create after_validation :spam_detected?, on: :create
cattr_accessor :spammable_attrs, instance_accessor: false do cattr_accessor :spammable_attrs, instance_accessor: false do
[] []
end end
delegate :submitted?, to: :user_agent_detail, allow_nil: true
end end
def can_be_submitted? def can_be_submitted?
if user_agent_detail if user_agent_detail
user_agent_detail.submittable? && akismet_enabled? user_agent_detail.submittable?
else else
false false
end end
end end
def submit_spam
return unless akismet_enabled? && can_be_submitted?
spam!(user_agent_detail, spammable_text, owner)
end
def spam_detected?(env)
@spam = is_spam?(env, owner, spammable_text)
end
def spam? def spam?
@spam @spam
end end
def submitted? def spam_detected?
if user_agent_detail
user_agent_detail.submitted
else
false
end
end
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?
end end
...@@ -61,34 +42,41 @@ module Spammable ...@@ -61,34 +42,41 @@ module Spammable
end end
end end
def to_ability_name def owner
self.class.to_s.underscore User.find(owner_id)
end end
# Override this method if an additional check is needed before calling Akismet def spam_title
def check_for_spam? attr = self.class.spammable_attrs.select do |_, options|
akismet_enabled? options.fetch(:spam_title, false)
end end
def spam_title attr = attr[0].first
raise NotImplementedError
public_send(attr) if respond_to?(attr.to_sym)
end end
def spam_description def spam_description
raise NotImplementedError attr = self.class.spammable_attrs.select do |_, options|
options.fetch(:spam_description, false)
end end
private attr = attr[0].first
public_send(attr) if respond_to?(attr.to_sym)
end
def spammable_text def spammable_text
result = [] result = []
self.class.spammable_attrs.each do |entry| self.class.spammable_attrs.map do |attr|
result << self.send(entry) result << public_send(attr.first)
end end
result.reject(&:blank?).join("\n") result.reject(&:blank?).join("\n")
end end
def owner # Override in Spammable if further checks are necessary
User.find(owner_id) def check_for_spam?
current_application_settings.akismet_enabled
end end
end end
...@@ -37,7 +37,8 @@ class Issue < ActiveRecord::Base ...@@ -37,7 +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 attr_spammable :title, spam_title: true
attr_spammable :description, spam_description: true
state_machine :state, initial: :opened do state_machine :state, initial: :opened do
event :close do event :close do
...@@ -266,16 +267,8 @@ class Issue < ActiveRecord::Base ...@@ -266,16 +267,8 @@ class Issue < ActiveRecord::Base
due_date.try(:past?) || false due_date.try(:past?) || false
end end
# To allow polymorphism with Spammable # Only issues on public projects should be checked for spam
def check_for_spam? def check_for_spam?
super && project.public? super && project.public?
end end
def spam_title
title
end
def spam_description
description
end
end end
class AkismetService
attr_accessor :spammable
def initialize(spammable)
@spammable = spammable
end
def client_ip(env)
env['action_dispatch.remote_ip'].to_s
end
def user_agent(env)
env['HTTP_USER_AGENT']
end
def is_spam?(environment)
ip_address = client_ip(environment)
user_agent = user_agent(environment)
params = {
type: 'comment',
text: spammable.spammable_text,
created_at: DateTime.now,
author: spammable.owner.name,
author_email: spammable.owner.email,
referrer: environment['HTTP_REFERER'],
}
begin
is_spam, is_blatant = akismet_client.check(ip_address, user_agent, params)
is_spam || is_blatant
rescue => e
Rails.logger.error("Unable to connect to Akismet: #{e}, skipping check")
false
end
end
def ham!
params = {
type: 'comment',
text: spammable.text,
author: spammable.user.name,
author_email: spammable.user.email
}
begin
akismet_client.submit_ham(spammable.source_ip, spammable.user_agent, params)
true
rescue => e
Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
false
end
end
def spam!
params = {
type: 'comment',
text: spammable.spammable_text,
author: spammable.owner.name,
author_email: spammable.owner.email
}
begin
akismet_client.submit_spam(spammable.user_agent_detail.ip_address, spammable.user_agent_detail.user_agent, params)
true
rescue => e
Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
false
end
end
private
def akismet_client
@akismet_client ||= ::Akismet::Client.new(current_application_settings.akismet_api_key,
Gitlab.config.gitlab.url)
end
end
...@@ -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
spam_check_service.execute @issue.spam = spam_service.check(@api, @request)
if @issue.save if @issue.save
@issue.update_attributes(label_ids: label_params) @issue.update_attributes(label_ids: label_params)
...@@ -25,8 +25,8 @@ module Issues ...@@ -25,8 +25,8 @@ module Issues
private private
def spam_check_service def spam_service
SpamCheckService.new(@request, @api, @issue) SpamService.new(@issue)
end end
def user_agent_detail_service def user_agent_detail_service
......
class SpamCheckService
attr_accessor :request, :api, :spammable
def initialize(request, api, spammable)
@request, @api, @spammable = request, api, spammable
end
def execute
if request && spammable.check_for_spam?
if spammable.spam_detected?(request.env)
create_spam_log
end
end
end
private
def spam_log_attrs
{
user_id: spammable.owner_id,
title: spammable.spam_title,
description: spammable.spam_description,
source_ip: spammable.client_ip(request.env),
user_agent: spammable.user_agent(request.env),
noteable_type: spammable.class.to_s,
via_api: api
}
end
def create_spam_log
SpamLog.create(spam_log_attrs)
end
end
class SpamService
attr_accessor :spammable
def initialize(spammable)
@spammable = spammable
end
def check(api, request)
return false unless request && spammable.check_for_spam?
return false unless akismet.is_spam?(request.env)
create_spam_log(api, request)
true
end
def mark_as_spam!(current_user)
return false unless akismet_enabled? && spammable.can_be_submitted?
if akismet.spam!
spammable.user_agent_detail.update_attribute(:submitted, true)
if spammable.is_a?(Issuable)
SystemNoteService.submit_spam(spammable, spammable.project, current_user)
end
true
else
false
end
end
def mark_as_ham!
return false unless spammable.is_a?(SpamLog)
if akismet.ham!
spammable.update_attribute(:submitted_as_ham, true)
true
else
false
end
end
private
def akismet
@akismet ||= AkismetService.new(spammable)
end
def akismet_enabled?
current_application_settings.akismet_enabled
end
def create_spam_log(api, request)
SpamLog.create(
{
user_id: spammable.owner_id,
title: spammable.spam_title,
description: spammable.spam_description,
source_ip: akismet.client_ip(request.env),
user_agent: akismet.user_agent(request.env),
noteable_type: spammable.class.to_s,
via_api: api
}
)
end
end
...@@ -395,7 +395,7 @@ module SystemNoteService ...@@ -395,7 +395,7 @@ module SystemNoteService
create_note(noteable: noteable, project: project, author: author, note: body) create_note(noteable: noteable, project: project, author: author, note: body)
end end
# Called when the status of a Issuable is submitted as spam # Called when Issuable is submitted as spam
# #
# noteable - Noteable object # noteable - Noteable object
# project - Project owning noteable # project - Project owning noteable
...@@ -407,7 +407,7 @@ module SystemNoteService ...@@ -407,7 +407,7 @@ module SystemNoteService
# #
# Returns the created Note object # Returns the created Note object
def submit_spam(noteable, project, author) def submit_spam(noteable, project, author)
body = "Submitted #{noteable.class.to_s.downcase} as spam" body = "Submitted this #{noteable.class.to_s.downcase} as spam"
create_note(noteable: noteable, project: project, author: author, note: body) create_note(noteable: noteable, project: project, author: author, note: body)
end end
......
class CreateUserAgentDetails < ActiveRecord::Migration class CreateUserAgentDetails < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
def change def change
create_table :user_agent_details do |t| create_table :user_agent_details do |t|
t.string :user_agent, null: false t.string :user_agent, null: false
......
...@@ -10,7 +10,7 @@ class RemoveProjectIdFromSpamLogs < ActiveRecord::Migration ...@@ -10,7 +10,7 @@ class RemoveProjectIdFromSpamLogs < ActiveRecord::Migration
# When a migration requires downtime you **must** uncomment the following # When a migration requires downtime you **must** uncomment the following
# constant and define a short and easy to understand explanation as to why the # constant and define a short and easy to understand explanation as to why the
# migration requires downtime. # migration requires downtime.
DOWNTIME_REASON = 'Removing a table that contains data that is not used anywhere.' DOWNTIME_REASON = 'Removing a column that contains data that is not used anywhere.'
# When using the methods "add_concurrent_index" or "add_column_with_default" # When using the methods "add_concurrent_index" or "add_column_with_default"
# you must disable the use of transactions as these methods can not run in an # you must disable the use of transactions as these methods can not run in an
......
...@@ -3,8 +3,6 @@ module API ...@@ -3,8 +3,6 @@ module API
class Issues < Grape::API class Issues < Grape::API
before { authenticate! } before { authenticate! }
helpers ::Gitlab::AkismetHelper
helpers do helpers do
def filter_issues_state(issues, state) def filter_issues_state(issues, state)
case state case state
......
module Gitlab
module AkismetHelper
def akismet_enabled?
current_application_settings.akismet_enabled
end
def akismet_client
@akismet_client ||= ::Akismet::Client.new(current_application_settings.akismet_api_key,
Gitlab.config.gitlab.url)
end
def client_ip(env)
env['action_dispatch.remote_ip'].to_s
end
def user_agent(env)
env['HTTP_USER_AGENT']
end
def is_spam?(environment, user, text)
client = akismet_client
ip_address = client_ip(environment)
user_agent = user_agent(environment)
params = {
type: 'comment',
text: text,
created_at: DateTime.now,
author: user.name,
author_email: user.email,
referrer: environment['HTTP_REFERER'],
}
begin
is_spam, is_blatant = client.check(ip_address, user_agent, params)
is_spam || is_blatant
rescue => e
Rails.logger.error("Unable to connect to Akismet: #{e}, skipping check")
false
end
end
def ham!(ip_address, user_agent, text, user)
client = akismet_client
params = {
type: 'comment',
text: text,
author: user.name,
author_email: user.email
}
begin
client.submit_ham(ip_address, user_agent, params)
true
rescue => e
Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
false
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)
true
rescue => e
Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
false
end
end
end
end
...@@ -37,7 +37,7 @@ describe Admin::SpamLogsController do ...@@ -37,7 +37,7 @@ describe Admin::SpamLogsController do
describe '#mark_as_ham' do describe '#mark_as_ham' do
before do before do
allow_any_instance_of(Gitlab::AkismetHelper).to receive(:ham!).and_return(true) allow_any_instance_of(AkismetService).to receive(:ham!).and_return(true)
end end
it 'submits the log as ham' do it 'submits the log as ham' do
post :mark_as_ham, id: first_spam.id post :mark_as_ham, id: first_spam.id
......
...@@ -275,7 +275,7 @@ describe Projects::IssuesController do ...@@ -275,7 +275,7 @@ describe Projects::IssuesController do
context 'Akismet is enabled' do context 'Akismet is enabled' do
before do before do
allow_any_instance_of(Spammable).to receive(:check_for_spam?).and_return(true) allow_any_instance_of(Spammable).to receive(:check_for_spam?).and_return(true)
allow_any_instance_of(Gitlab::AkismetHelper).to receive(:is_spam?).and_return(true) allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true)
end end
def post_spam_issue def post_spam_issue
...@@ -325,7 +325,9 @@ describe Projects::IssuesController do ...@@ -325,7 +325,9 @@ describe Projects::IssuesController do
describe 'POST #mark_as_spam' do describe 'POST #mark_as_spam' do
context 'properly submits to Akismet' do context 'properly submits to Akismet' do
before do before do
allow_any_instance_of(Spammable).to receive_messages(can_be_submitted?: true, submit_spam: true) allow_any_instance_of(AkismetService).to receive_messages(spam!: true)
allow_any_instance_of(ApplicationSetting).to receive_messages(akismet_enabled: true)
allow_any_instance_of(SpamService).to receive_messages(can_be_submitted?: true)
end end
def post_spam def post_spam
......
require 'spec_helper'
describe Gitlab::AkismetHelper, type: :helper do
let(:project) { create(:project, :public) }
let(:user) { create(:user) }
before do
allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url))
allow_any_instance_of(ApplicationSetting).to receive(:akismet_enabled).and_return(true)
allow_any_instance_of(ApplicationSetting).to receive(:akismet_api_key).and_return('12345')
end
describe '#is_spam?' do
it 'returns true for spam' do
environment = {
'action_dispatch.remote_ip' => '127.0.0.1',
'HTTP_USER_AGENT' => 'Test User Agent'
}
allow_any_instance_of(::Akismet::Client).to receive(:check).and_return([true, true])
expect(helper.is_spam?(environment, user, 'Is this spam?')).to eq(true)
end
end
end
...@@ -14,25 +14,24 @@ describe Issue, 'Spammable' do ...@@ -14,25 +14,24 @@ describe Issue, 'Spammable' do
end end
describe 'InstanceMethods' do describe 'InstanceMethods' do
before do
allow_any_instance_of(Gitlab::AkismetHelper).to receive(:akismet_enabled?).and_return(true)
end
it 'should return the correct creator' do it 'should return the correct creator' do
expect(issue.send(:owner).id).to eq(issue.author_id) expect(issue.owner_id).to eq(issue.author_id)
end end
it 'should be invalid if spam' do it 'should be invalid if spam' do
issue.spam = true issue = build(:issue, spam: true)
expect(issue.valid?).to be_truthy expect(issue.valid?).to be_falsey
end end
it 'should be submittable' do it 'should not be submitted' do
create(:user_agent_detail, subject: issue) create(:user_agent_detail, subject: issue)
expect(issue.can_be_submitted?).to be_truthy expect(issue.submitted?).to be_falsey
end end
describe '#check_for_spam?' do describe '#check_for_spam?' do
before do
allow_any_instance_of(ApplicationSetting).to receive(:akismet_enabled).and_return(true)
end
it 'returns true for public project' do it 'returns true for public project' do
issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
expect(issue.check_for_spam?).to eq(true) expect(issue.check_for_spam?).to eq(true)
...@@ -43,14 +42,4 @@ describe Issue, 'Spammable' do ...@@ -43,14 +42,4 @@ describe Issue, 'Spammable' do
end end
end end
end end
describe 'AkismetMethods' do
before do
allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(is_spam?: true, spam!: true, akismet_enabled?: true)
allow_any_instance_of(Spammable).to receive(:can_be_submitted?).and_return(true)
end
it { expect(issue.spam_detected?(:mock_env)).to be_truthy }
it { expect(issue.submit_spam).to be_truthy }
end
end end
...@@ -532,7 +532,7 @@ describe API::API, api: true do ...@@ -532,7 +532,7 @@ describe API::API, api: true do
describe 'POST /projects/:id/issues with spam filtering' do describe 'POST /projects/:id/issues with spam filtering' do
before do before do
allow_any_instance_of(Spammable).to receive(:check_for_spam?).and_return(true) allow_any_instance_of(Spammable).to receive(:check_for_spam?).and_return(true)
allow_any_instance_of(Gitlab::AkismetHelper).to receive(:is_spam?).and_return(true) allow_any_instance_of(AkismetService).to receive_messages(is_spam?: true)
end end
let(:params) do let(:params) do
......
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