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? - end - - def spam_title - raise NotImplementedError - end - - def spam_description - raise NotImplementedError - 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 owner User.find(owner_id) end + + def spam_title + 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 + attr = self.class.spammable_attrs.select do |_, options| + options.fetch(:spam_description, false) + end + + attr = attr[0].first + + public_send(attr) if respond_to?(attr.to_sym) + end + + def spammable_text + result = [] + self.class.spammable_attrs.map do |attr| + result << public_send(attr.first) + end + + result.reject(&:blank?).join("\n") + end + + # 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 diff --git a/db/migrate/20160727163552_create_user_agent_details.rb b/db/migrate/20160727163552_create_user_agent_details.rb index f9a02f310da..6677f5e80ba 100644 --- a/db/migrate/20160727163552_create_user_agent_details.rb +++ b/db/migrate/20160727163552_create_user_agent_details.rb @@ -1,4 +1,9 @@ class CreateUserAgentDetails < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + def change create_table :user_agent_details do |t| t.string :user_agent, null: false diff --git a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb index 5950874d5af..e28ab31d629 100644 --- a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb +++ b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb @@ -10,7 +10,7 @@ class RemoveProjectIdFromSpamLogs < ActiveRecord::Migration # 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.' + 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" # you must disable the use of transactions as these methods can not run in an diff --git a/lib/api/issues.rb b/lib/api/issues.rb index c4d3134da6c..077258faee1 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -3,8 +3,6 @@ module API class Issues < Grape::API before { authenticate! } - helpers ::Gitlab::AkismetHelper - helpers do def filter_issues_state(issues, state) case state diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb deleted file mode 100644 index bd71a1aaa51..00000000000 --- a/lib/gitlab/akismet_helper.rb +++ /dev/null @@ -1,81 +0,0 @@ -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 diff --git a/spec/controllers/admin/spam_logs_controller_spec.rb b/spec/controllers/admin/spam_logs_controller_spec.rb index f94afd1139d..ac0441d0a4e 100644 --- a/spec/controllers/admin/spam_logs_controller_spec.rb +++ b/spec/controllers/admin/spam_logs_controller_spec.rb @@ -37,7 +37,7 @@ describe Admin::SpamLogsController do describe '#mark_as_ham' 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 it 'submits the log as ham' do post :mark_as_ham, id: first_spam.id diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 8fcde9a38bc..0e8d4b80b0e 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -275,7 +275,7 @@ describe Projects::IssuesController do context 'Akismet is enabled' do before do 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 def post_spam_issue @@ -325,7 +325,9 @@ describe Projects::IssuesController do describe 'POST #mark_as_spam' do context 'properly submits to Akismet' 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 def post_spam diff --git a/spec/lib/gitlab/akismet_helper_spec.rb b/spec/lib/gitlab/akismet_helper_spec.rb deleted file mode 100644 index 80b4f912d41..00000000000 --- a/spec/lib/gitlab/akismet_helper_spec.rb +++ /dev/null @@ -1,24 +0,0 @@ -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 diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb index 4e52d05918f..7944305e7b3 100644 --- a/spec/models/concerns/spammable_spec.rb +++ b/spec/models/concerns/spammable_spec.rb @@ -14,25 +14,24 @@ describe Issue, 'Spammable' do end 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 - expect(issue.send(:owner).id).to eq(issue.author_id) + expect(issue.owner_id).to eq(issue.author_id) end it 'should be invalid if spam' do - issue.spam = true - expect(issue.valid?).to be_truthy + issue = build(:issue, spam: true) + expect(issue.valid?).to be_falsey end - it 'should be submittable' do + it 'should not be submitted' do create(:user_agent_detail, subject: issue) - expect(issue.can_be_submitted?).to be_truthy + expect(issue.submitted?).to be_falsey end 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 issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) expect(issue.check_for_spam?).to eq(true) @@ -43,14 +42,4 @@ describe Issue, 'Spammable' do 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 diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 353b01d4a09..30b939c797c 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -532,7 +532,7 @@ describe API::API, api: true do describe 'POST /projects/:id/issues with spam filtering' do before do 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 let(:params) do