diff options
author | Patricio Cano <suprnova32@gmail.com> | 2016-08-06 01:10:08 +0300 |
---|---|---|
committer | Patricio Cano <suprnova32@gmail.com> | 2016-08-15 21:18:15 +0300 |
commit | 43e756d4eafd79f4d2f366b646ebb94af78b5a4c (patch) | |
tree | 07949d3368affcda301fd266e1e5bf0649474b23 /app | |
parent | 7179165af7553720089a0b7e7024374c371e2f90 (diff) |
Refactored AkismetHelper into AkismetService and cleaned up `Spammable`
- Refactored SpamCheckService into SpamService
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/admin/spam_logs_controller.rb | 4 | ||||
-rw-r--r-- | app/controllers/concerns/spammable_actions.rb | 8 | ||||
-rw-r--r-- | app/models/concerns/spammable.rb | 68 | ||||
-rw-r--r-- | app/models/issue.rb | 13 | ||||
-rw-r--r-- | app/services/akismet_service.rb | 78 | ||||
-rw-r--r-- | app/services/issues/create_service.rb | 6 | ||||
-rw-r--r-- | app/services/spam_check_service.rb | 33 | ||||
-rw-r--r-- | app/services/spam_service.rb | 64 | ||||
-rw-r--r-- | app/services/system_note_service.rb | 4 |
9 files changed, 180 insertions, 98 deletions
diff --git a/app/controllers/admin/spam_logs_controller.rb b/app/controllers/admin/spam_logs_controller.rb index d15f00bf84c..7876d2ee767 100644 --- a/app/controllers/admin/spam_logs_controller.rb +++ b/app/controllers/admin/spam_logs_controller.rb @@ -1,5 +1,4 @@ class Admin::SpamLogsController < Admin::ApplicationController - include Gitlab::AkismetHelper def index @spam_logs = SpamLog.order(id: :desc).page(params[:page]) @@ -20,8 +19,7 @@ class Admin::SpamLogsController < Admin::ApplicationController def mark_as_ham spam_log = SpamLog.find(params[:id]) - if ham!(spam_log.source_ip, spam_log.user_agent, spam_log.text, spam_log.user) - spam_log.update_attribute(:submitted_as_ham, true) + if SpamService.new(spam_log).mark_as_ham! redirect_to admin_spam_logs_path, notice: 'Spam log successfully submitted as ham.' else redirect_to admin_spam_logs_path, notice: 'Error with Akismet. Please check the logs for more info.' diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index 85be25d84cc..296811267e5 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -6,13 +6,7 @@ module SpammableActions end def mark_as_spam - if spammable.submit_spam - spammable.user_agent_detail.update_attribute(:submitted, true) - - if spammable.is_a?(Issuable) - SystemNoteService.submit_spam(spammable, spammable.project, current_user) - end - + if SpamService.new(spammable).mark_as_spam!(current_user) redirect_to spammable, notice: 'Issue was submitted to Akismet successfully.' else flash[:error] = 'Error with Akismet. Please check the logs for more info.' diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index f272e7c5a55..694e2efcade 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -1,55 +1,36 @@ module Spammable extend ActiveSupport::Concern - include Gitlab::AkismetHelper module ClassMethods - def attr_spammable(*attrs) - attrs.each do |attr| - spammable_attrs << attr.to_s - end + def attr_spammable(attr, options = {}) + spammable_attrs << [attr.to_s, options] end end included do has_one :user_agent_detail, as: :subject, dependent: :destroy attr_accessor :spam - after_validation :check_for_spam, on: :create + after_validation :spam_detected?, on: :create cattr_accessor :spammable_attrs, instance_accessor: false do [] end + delegate :submitted?, to: :user_agent_detail, allow_nil: true end def can_be_submitted? if user_agent_detail - user_agent_detail.submittable? && akismet_enabled? + user_agent_detail.submittable? else false 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? @spam end - def submitted? - if user_agent_detail - user_agent_detail.submitted - else - false - end - end - - def check_for_spam + def spam_detected? self.errors.add(:base, "Your #{self.class.name.underscore} has been recognized as spam and has been discarded.") if spam? end @@ -61,34 +42,41 @@ module Spammable end end - def to_ability_name - self.class.to_s.underscore - end - - # Override this method if an additional check is needed before calling Akismet - def check_for_spam? - akismet_enabled? + def owner + User.find(owner_id) end def spam_title - raise NotImplementedError + attr = self.class.spammable_attrs.select do |_, options| + options.fetch(:spam_title, false) + end + + attr = attr[0].first + + public_send(attr) if respond_to?(attr.to_sym) end def spam_description - raise NotImplementedError - end + attr = self.class.spammable_attrs.select do |_, options| + options.fetch(:spam_description, false) + end - private + attr = attr[0].first + + public_send(attr) if respond_to?(attr.to_sym) + end def spammable_text result = [] - self.class.spammable_attrs.each do |entry| - result << self.send(entry) + self.class.spammable_attrs.map do |attr| + result << public_send(attr.first) end + result.reject(&:blank?).join("\n") end - def owner - User.find(owner_id) + # Override in Spammable if further checks are necessary + def check_for_spam? + current_application_settings.akismet_enabled end end diff --git a/app/models/issue.rb b/app/models/issue.rb index 40028e56489..ab98d0cf9df 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -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_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 event :close do @@ -266,16 +267,8 @@ class Issue < ActiveRecord::Base due_date.try(:past?) || false end - # To allow polymorphism with Spammable + # Only issues on public projects should be checked for spam def check_for_spam? super && project.public? end - - def spam_title - title - end - - def spam_description - description - end end diff --git a/app/services/akismet_service.rb b/app/services/akismet_service.rb new file mode 100644 index 00000000000..c09663bce85 --- /dev/null +++ b/app/services/akismet_service.rb @@ -0,0 +1,78 @@ +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 diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 9f8a642a75b..67125d5c0e4 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -8,7 +8,7 @@ module Issues @issue = project.issues.new(params) @issue.author = params[:author] || current_user - spam_check_service.execute + @issue.spam = spam_service.check(@api, @request) if @issue.save @issue.update_attributes(label_ids: label_params) @@ -25,8 +25,8 @@ module Issues private - def spam_check_service - SpamCheckService.new(@request, @api, @issue) + def spam_service + SpamService.new(@issue) end def user_agent_detail_service diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb deleted file mode 100644 index 71b9436a22e..00000000000 --- a/app/services/spam_check_service.rb +++ /dev/null @@ -1,33 +0,0 @@ -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 diff --git a/app/services/spam_service.rb b/app/services/spam_service.rb new file mode 100644 index 00000000000..ad60de368aa --- /dev/null +++ b/app/services/spam_service.rb @@ -0,0 +1,64 @@ +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 diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 56d3329f5bd..35c9ce909e6 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -395,7 +395,7 @@ module SystemNoteService create_note(noteable: noteable, project: project, author: author, note: body) end - # Called when the status of a Issuable is submitted as spam + # Called when Issuable is submitted as spam # # noteable - Noteable object # project - Project owning noteable @@ -407,7 +407,7 @@ module SystemNoteService # # Returns the created Note object 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) end |