Commit 64ab2b3d authored by Patricio Cano's avatar Patricio Cano

Refactored spam related code even further

- Removed unnecessary column from `SpamLog`
- Moved creation of SpamLogs out of its own service and into SpamCheckService
- Simplified code in SpamCheckService.
- Moved move spam related code into Spammable concern
parent 722fc84e
...@@ -14,4 +14,10 @@ class Admin::SpamLogsController < Admin::ApplicationController ...@@ -14,4 +14,10 @@ class Admin::SpamLogsController < Admin::ApplicationController
head :ok head :ok
end end
end end
def ham
spam_log = SpamLog.find(params[:id])
Gitlab::AkismetHelper.ham!(spam_log.source_ip, spam_log.user_agent, spam_log.description, spam_log.user)
end
end end
...@@ -28,26 +28,42 @@ module Spammable ...@@ -28,26 +28,42 @@ module Spammable
end end
end end
def submit_ham
return unless akismet_enabled? && can_be_submitted?
ham!(user_agent_detail, spammable_text, creator)
end
def submit_spam def submit_spam
return unless akismet_enabled? && can_be_submitted? return unless akismet_enabled? && can_be_submitted?
spam!(user_agent_detail, spammable_text, creator) spam!(user_agent_detail, spammable_text, owner)
end end
def spam?(env, user) def spam_detected?(env)
is_spam?(env, user, spammable_text) @spam = is_spam?(env, owner, spammable_text)
end end
def spam_detected? def spam?
@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_detected? self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam?
end
def owner_id
if self.respond_to?(:author_id)
self.author_id
elsif self.respond_to?(:creator_id)
self.creator_id
end
end
# Override this method if an additional check is needed before calling Akismet
def check_for_spam?
akismet_enabled?
end
def spam_title
raise 'Implement in included model!'
end
def spam_description
raise 'Implement in included model!'
end end
private private
...@@ -60,11 +76,7 @@ module Spammable ...@@ -60,11 +76,7 @@ module Spammable
result.reject(&:blank?).join("\n") result.reject(&:blank?).join("\n")
end end
def creator def owner
if self.author_id User.find(owner_id)
User.find(self.author_id)
elsif self.creator_id
User.find(self.creator_id)
end
end end
end end
...@@ -265,4 +265,17 @@ class Issue < ActiveRecord::Base ...@@ -265,4 +265,17 @@ class Issue < ActiveRecord::Base
def overdue? def overdue?
due_date.try(:past?) || false due_date.try(:past?) || false
end end
# To allow polymorphism with Spammable
def check_for_spam?
super && project.public?
end
def spam_title
title
end
def spam_description
description
end
end end
class CreateSpamLogService < BaseService
def initialize(project, user, params)
super(project, user, params)
end
def execute
spam_params = params.merge({ user_id: @current_user.id,
project_id: @project.id } )
spam_log = SpamLog.new(spam_params)
spam_log.save
spam_log
end
end
...@@ -3,34 +3,34 @@ module Issues ...@@ -3,34 +3,34 @@ module Issues
def execute def execute
filter_params filter_params
label_params = params.delete(:label_ids) label_params = params.delete(:label_ids)
request = params.delete(:request) @request = params.delete(:request)
api = params.delete(:api) @api = params.delete(:api)
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_check_service.execute
if issue.save if @issue.save
issue.update_attributes(label_ids: label_params) @issue.update_attributes(label_ids: label_params)
notification_service.new_issue(issue, current_user) notification_service.new_issue(@issue, current_user)
todo_service.new_issue(issue, current_user) todo_service.new_issue(@issue, current_user)
event_service.open_issue(issue, current_user) event_service.open_issue(@issue, current_user)
user_agent_detail_service(issue, request).create user_agent_detail_service.create
issue.create_cross_references!(current_user) @issue.create_cross_references!(current_user)
execute_hooks(issue, 'open') execute_hooks(@issue, 'open')
end end
issue @issue
end end
private private
def spam_check_service def spam_check_service
SpamCheckService.new(project, current_user, params) SpamCheckService.new(@request, @api, @issue)
end end
def user_agent_detail_service(issue, request) def user_agent_detail_service
UserAgentDetailService.new(issue, request) UserAgentDetailService.new(@issue, @request)
end end
end end
end end
class SpamCheckService < BaseService class SpamCheckService
attr_accessor :request, :api, :subject attr_accessor :request, :api, :spammable
def execute(request, api, subject) def initialize(request, api, spammable)
@request, @api, @subject = request, api, subject @request, @api, @spammable = request, api, spammable
return false unless request || subject.check_for_spam?(project) end
return false unless subject.spam?(request.env, current_user)
def execute
if request && spammable.check_for_spam?
if spammable.spam_detected?(request.env)
create_spam_log create_spam_log
end
true end
end end
private private
def spam_log_attrs def spam_log_attrs
{ {
user_id: current_user.id, user_id: spammable.owner_id,
project_id: project.id, title: spammable.spam_title,
title: params[:title], description: spammable.spam_description,
description: params[:description], source_ip: spammable.client_ip(request.env),
source_ip: subject.client_ip(request.env), user_agent: spammable.user_agent(request.env),
user_agent: subject.user_agent(request.env), noteable_type: spammable.class.to_s,
noteable_type: subject.class.to_s,
via_api: api via_api: api
} }
end end
def create_spam_log def create_spam_log
CreateSpamLogService.new(project, current_user, spam_log_attrs).execute SpamLog.create(spam_log_attrs)
end end
end end
class UserAgentDetailService class UserAgentDetailService
attr_accessor :subject, :request attr_accessor :spammable, :request
def initialize(subject, request) def initialize(spammable, request)
@subject, @request = subject, request @spammable, @request = spammable, request
end end
def create def create
return unless request return unless request
subject.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s) spammable.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s)
end end
end end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class RemoveProjectIdFromSpamLogs < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = true
# When a migration requires downtime you **must** uncomment the following
# constant and define a short and easy to understand explanation as to why the
# migration requires downtime.
DOWNTIME_REASON = 'Removing a table that contains data that is not used anywhere.'
# 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
# existing transaction. When using "add_concurrent_index" make sure that this
# method is the _only_ method called in the migration, any other changes
# should go in a separate migration. This ensures that upon failure _only_ the
# index creation fails and can be retried or reverted easily.
#
# To disable transactions uncomment the following line and remove these
# comments:
# disable_ddl_transaction!
def change
remove_column :spam_logs, :project_id, :integer
end
end
...@@ -926,7 +926,6 @@ ActiveRecord::Schema.define(version: 20160810142633) do ...@@ -926,7 +926,6 @@ ActiveRecord::Schema.define(version: 20160810142633) do
t.string "source_ip" t.string "source_ip"
t.string "user_agent" t.string "user_agent"
t.boolean "via_api" t.boolean "via_api"
t.integer "project_id"
t.string "noteable_type" t.string "noteable_type"
t.string "title" t.string "title"
t.text "description" t.text "description"
......
...@@ -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_detected? if issue.spam?
render_api_error!({ error: 'Spam detected' }, 400) render_api_error!({ error: 'Spam detected' }, 400)
end end
......
...@@ -17,10 +17,6 @@ module Gitlab ...@@ -17,10 +17,6 @@ module Gitlab
env['HTTP_USER_AGENT'] env['HTTP_USER_AGENT']
end end
def check_for_spam?(project)
akismet_enabled? && project.public?
end
def is_spam?(environment, user, text) def is_spam?(environment, user, text)
client = akismet_client client = akismet_client
ip_address = client_ip(environment) ip_address = client_ip(environment)
...@@ -44,7 +40,7 @@ module Gitlab ...@@ -44,7 +40,7 @@ module Gitlab
end end
end end
def ham!(details, text, user) def ham!(ip_address, user_agent, text, user)
client = akismet_client client = akismet_client
params = { params = {
...@@ -55,7 +51,7 @@ module Gitlab ...@@ -55,7 +51,7 @@ module Gitlab
} }
begin begin
client.submit_ham(details.ip_address, details.user_agent, params) client.submit_ham(ip_address, user_agent, params)
rescue => e rescue => e
Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!")
end end
......
...@@ -274,7 +274,7 @@ describe Projects::IssuesController do ...@@ -274,7 +274,7 @@ describe Projects::IssuesController do
describe 'POST #create' do describe 'POST #create' do
context 'Akismet is enabled' do context 'Akismet is enabled' do
before do before do
allow_any_instance_of(Gitlab::AkismetHelper).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(Gitlab::AkismetHelper).to receive(:is_spam?).and_return(true)
end end
......
...@@ -10,17 +10,6 @@ describe Gitlab::AkismetHelper, type: :helper do ...@@ -10,17 +10,6 @@ describe Gitlab::AkismetHelper, type: :helper do
allow_any_instance_of(ApplicationSetting).to receive(:akismet_api_key).and_return('12345') allow_any_instance_of(ApplicationSetting).to receive(:akismet_api_key).and_return('12345')
end end
describe '#check_for_spam?' do
it 'returns true for public project' do
expect(helper.check_for_spam?(project)).to eq(true)
end
it 'returns false for private project' do
project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PRIVATE)
expect(helper.check_for_spam?(project)).to eq(false)
end
end
describe '#is_spam?' do describe '#is_spam?' do
it 'returns true for spam' do it 'returns true for spam' do
environment = { environment = {
......
...@@ -15,7 +15,7 @@ describe Issue, 'Spammable' do ...@@ -15,7 +15,7 @@ describe Issue, 'Spammable' do
describe 'InstanceMethods' do describe 'InstanceMethods' do
it 'should return the correct creator' do it 'should return the correct creator' do
expect(issue.send(:creator).id).to eq(issue.author_id) expect(issue.send(:owner).id).to eq(issue.author_id)
end end
it 'should be invalid if spam' do it 'should be invalid if spam' do
...@@ -27,15 +27,28 @@ describe Issue, 'Spammable' do ...@@ -27,15 +27,28 @@ describe Issue, 'Spammable' do
create(:user_agent_detail, subject_id: issue.id, subject_type: issue.class.to_s) create(:user_agent_detail, subject_id: issue.id, subject_type: issue.class.to_s)
expect(issue.can_be_submitted?).to be_truthy expect(issue.can_be_submitted?).to be_truthy
end end
describe '#check_for_spam?' do
before do
allow_any_instance_of(Gitlab::AkismetHelper).to receive(:akismet_enabled?).and_return(true)
end
it 'returns true for public project' do
issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
expect(issue.check_for_spam?).to eq(true)
end
it 'returns false for other visibility levels' do
expect(issue.check_for_spam?).to eq(false)
end
end
end end
describe 'AkismetMethods' do describe 'AkismetMethods' do
before do before do
allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(check_for_spam?: true, is_spam?: true, ham!: nil, spam!: nil) allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(is_spam?: true, spam!: nil)
end end
it { expect(issue.spam?(:mock_env, :mock_user)).to be_truthy } it { expect(issue.spam_detected?(:mock_env)).to be_truthy }
it { expect(issue.submit_spam).to be_nil } it { expect(issue.submit_spam).to be_nil }
it { expect(issue.submit_ham).to be_nil }
end end
end end
...@@ -531,7 +531,7 @@ describe API::API, api: true do ...@@ -531,7 +531,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(Gitlab::AkismetHelper).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(Gitlab::AkismetHelper).to receive(:is_spam?).and_return(true)
end end
...@@ -554,7 +554,6 @@ describe API::API, api: true do ...@@ -554,7 +554,6 @@ describe API::API, api: true do
expect(spam_logs[0].description).to eq('content here') expect(spam_logs[0].description).to eq('content here')
expect(spam_logs[0].user).to eq(user) expect(spam_logs[0].user).to eq(user)
expect(spam_logs[0].noteable_type).to eq('Issue') expect(spam_logs[0].noteable_type).to eq('Issue')
expect(spam_logs[0].project_id).to eq(project.id)
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