diff options
-rw-r--r-- | app/controllers/admin/spam_logs_controller.rb | 5 | ||||
-rw-r--r-- | app/controllers/concerns/spammable_actions.rb | 9 | ||||
-rw-r--r-- | app/models/concerns/spammable.rb | 42 | ||||
-rw-r--r-- | app/models/issue.rb | 3 | ||||
-rw-r--r-- | app/models/user_agent_detail.rb | 11 | ||||
-rw-r--r-- | app/services/akismet_service.rb | 59 | ||||
-rw-r--r-- | app/services/ham_service.rb | 26 | ||||
-rw-r--r-- | app/services/issues/create_service.rb | 4 | ||||
-rw-r--r-- | app/services/spam_service.rb | 76 | ||||
-rw-r--r-- | app/services/system_note_service.rb | 17 | ||||
-rw-r--r-- | app/services/user_agent_detail_service.rb | 1 | ||||
-rw-r--r-- | app/views/projects/issues/show.html.haml | 12 | ||||
-rw-r--r-- | db/migrate/20160727163552_create_user_agent_details.rb | 2 | ||||
-rw-r--r-- | db/schema.rb | 2 | ||||
-rw-r--r-- | spec/controllers/admin/spam_logs_controller_spec.rb | 2 | ||||
-rw-r--r-- | spec/controllers/projects/issues_controller_spec.rb | 13 | ||||
-rw-r--r-- | spec/factories/user_agent_details.rb | 7 | ||||
-rw-r--r-- | spec/models/concerns/spammable_spec.rb | 14 | ||||
-rw-r--r-- | spec/models/user_agent_detail_spec.rb | 22 | ||||
-rw-r--r-- | spec/requests/api/issues_spec.rb | 2 |
20 files changed, 160 insertions, 169 deletions
diff --git a/app/controllers/admin/spam_logs_controller.rb b/app/controllers/admin/spam_logs_controller.rb index 7876d2ee767..2abfa22712d 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 - def index @spam_logs = SpamLog.order(id: :desc).page(params[:page]) end @@ -19,10 +18,10 @@ class Admin::SpamLogsController < Admin::ApplicationController def mark_as_ham spam_log = SpamLog.find(params[:id]) - if SpamService.new(spam_log).mark_as_ham! + if HamService.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.' + redirect_to admin_spam_logs_path, alert: 'Error with Akismet. Please check the logs for more info.' end end end diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index 296811267e5..29e243c66a3 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -6,18 +6,17 @@ module SpammableActions end def mark_as_spam - if SpamService.new(spammable).mark_as_spam!(current_user) - redirect_to spammable, notice: 'Issue was submitted to Akismet successfully.' + if SpamService.new(spammable).mark_as_spam! + redirect_to spammable, notice: "#{spammable.class.to_s} was submitted to Akismet successfully." else - flash[:error] = 'Error with Akismet. Please check the logs for more info.' - redirect_to spammable + redirect_to spammable, alert: 'Error with Akismet. Please check the logs for more info.' end end private def spammable - raise NotImplementedError + raise NotImplementedError, "#{self.class} does not implement #{__method__}" end def authorize_submit_spammable! diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 694e2efcade..ce54fe5d3bf 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -9,16 +9,19 @@ module Spammable included do has_one :user_agent_detail, as: :subject, dependent: :destroy + attr_accessor :spam - after_validation :spam_detected?, on: :create + + after_validation :check_for_spam, on: :create cattr_accessor :spammable_attrs, instance_accessor: false do [] end - delegate :submitted?, to: :user_agent_detail, allow_nil: true + + delegate :ip_address, :user_agent, to: :user_agent_detail, allow_nil: true end - def can_be_submitted? + def submittable_as_spam? if user_agent_detail user_agent_detail.submittable? else @@ -30,46 +33,29 @@ module Spammable @spam end - def spam_detected? + def check_for_spam 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 - - def owner - User.find(owner_id) - end - def spam_title - attr = self.class.spammable_attrs.select do |_, options| + attr = self.class.spammable_attrs.find do |_, options| options.fetch(:spam_title, false) end - attr = attr[0].first - - public_send(attr) if respond_to?(attr.to_sym) + public_send(attr.first) if attr && respond_to?(attr.first.to_sym) end def spam_description - attr = self.class.spammable_attrs.select do |_, options| + attr = self.class.spammable_attrs.find do |_, options| options.fetch(:spam_description, false) end - attr = attr[0].first - - public_send(attr) if respond_to?(attr.to_sym) + public_send(attr.first) if attr && respond_to?(attr.first.to_sym) end def spammable_text - result = [] - self.class.spammable_attrs.map do |attr| - result << public_send(attr.first) + result = self.class.spammable_attrs.map do |attr| + public_send(attr.first) end result.reject(&:blank?).join("\n") @@ -77,6 +63,6 @@ module Spammable # Override in Spammable if further checks are necessary def check_for_spam? - current_application_settings.akismet_enabled + true end end diff --git a/app/models/issue.rb b/app/models/issue.rb index ab98d0cf9df..788611305fe 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -8,7 +8,6 @@ class Issue < ActiveRecord::Base include Taskable include Spammable include FasterCacheKeys - include AkismetSubmittable DueDateStruct = Struct.new(:title, :name).freeze NoDueDate = DueDateStruct.new('No Due Date', '0').freeze @@ -269,6 +268,6 @@ class Issue < ActiveRecord::Base # Only issues on public projects should be checked for spam def check_for_spam? - super && project.public? + project.public? end end diff --git a/app/models/user_agent_detail.rb b/app/models/user_agent_detail.rb index 6d76dff20e3..0949c6ef083 100644 --- a/app/models/user_agent_detail.rb +++ b/app/models/user_agent_detail.rb @@ -1,16 +1,9 @@ class UserAgentDetail < ActiveRecord::Base belongs_to :subject, polymorphic: true - validates :user_agent, - presence: true - validates :ip_address, - presence: true - validates :subject_id, - presence: true - validates :subject_type, - presence: true + validates :user_agent, :ip_address, :subject_id, :subject_type, presence: true def submittable? - user_agent.present? && ip_address.present? + !submitted? end end diff --git a/app/services/akismet_service.rb b/app/services/akismet_service.rb index c09663bce85..5c60addbe7c 100644 --- a/app/services/akismet_service.rb +++ b/app/services/akismet_service.rb @@ -1,33 +1,26 @@ class AkismetService - attr_accessor :spammable + attr_accessor :owner, :text, :options - def initialize(spammable) - @spammable = spammable + def initialize(owner, text, options = {}) + @owner = owner + @text = text + @options = options 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) + def is_spam? + return false unless akismet_enabled? params = { type: 'comment', - text: spammable.spammable_text, + text: text, created_at: DateTime.now, - author: spammable.owner.name, - author_email: spammable.owner.email, - referrer: environment['HTTP_REFERER'], + author: owner.name, + author_email: owner.email, + referrer: options[:referrer], } begin - is_spam, is_blatant = akismet_client.check(ip_address, user_agent, params) + is_spam, is_blatant = akismet_client.check(options[:ip_address], options[:user_agent], params) is_spam || is_blatant rescue => e Rails.logger.error("Unable to connect to Akismet: #{e}, skipping check") @@ -35,16 +28,18 @@ class AkismetService end end - def ham! + def submit_ham + return false unless akismet_enabled? + params = { type: 'comment', - text: spammable.text, - author: spammable.user.name, - author_email: spammable.user.email + text: text, + author: owner.name, + author_email: owner.email } begin - akismet_client.submit_ham(spammable.source_ip, spammable.user_agent, params) + akismet_client.submit_ham(options[:ip_address], options[:user_agent], params) true rescue => e Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") @@ -52,16 +47,18 @@ class AkismetService end end - def spam! + def submit_spam + return false unless akismet_enabled? + params = { type: 'comment', - text: spammable.spammable_text, - author: spammable.owner.name, - author_email: spammable.owner.email + text: text, + author: owner.name, + author_email: owner.email } begin - akismet_client.submit_spam(spammable.user_agent_detail.ip_address, spammable.user_agent_detail.user_agent, params) + akismet_client.submit_spam(options[:ip_address], options[:user_agent], params) true rescue => e Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") @@ -75,4 +72,8 @@ class AkismetService @akismet_client ||= ::Akismet::Client.new(current_application_settings.akismet_api_key, Gitlab.config.gitlab.url) end + + def akismet_enabled? + current_application_settings.akismet_enabled + end end diff --git a/app/services/ham_service.rb b/app/services/ham_service.rb new file mode 100644 index 00000000000..b0e1799b489 --- /dev/null +++ b/app/services/ham_service.rb @@ -0,0 +1,26 @@ +class HamService + attr_accessor :spam_log + + def initialize(spam_log) + @spam_log = spam_log + end + + def mark_as_ham! + if akismet.submit_ham + spam_log.update_attribute(:submitted_as_ham, true) + else + false + end + end + + private + + def akismet + @akismet ||= AkismetService.new( + spam_log.user, + spam_log.text, + ip_address: spam_log.source_ip, + user_agent: spam_log.user_agent + ) + end +end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 67125d5c0e4..65550ab8ec6 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 - @issue.spam = spam_service.check(@api, @request) + @issue.spam = spam_service.check(@api) if @issue.save @issue.update_attributes(label_ids: label_params) @@ -26,7 +26,7 @@ module Issues private def spam_service - SpamService.new(@issue) + SpamService.new(@issue, @request) end def user_agent_detail_service diff --git a/app/services/spam_service.rb b/app/services/spam_service.rb index ad60de368aa..48903291799 100644 --- a/app/services/spam_service.rb +++ b/app/services/spam_service.rb @@ -1,61 +1,75 @@ class SpamService - attr_accessor :spammable + attr_accessor :spammable, :request, :options - def initialize(spammable) + def initialize(spammable, request = nil) @spammable = spammable + @request = request + @options = {} + + if @request + @options[:ip_address] = @request.env['action_dispatch.remote_ip'].to_s + @options[:user_agent] = @request.env['HTTP_USER_AGENT'] + @options[:referrer] = @request.env['HTTP_REFERRER'] + else + @options[:ip_address] = @spammable.ip_address + @options[:user_agent] = @spammable.user_agent + end end - def check(api, request) - return false unless request && spammable.check_for_spam? - return false unless akismet.is_spam?(request.env) + def check(api = false) + return false unless request && check_for_spam? - create_spam_log(api, request) + return false unless akismet.is_spam? + + create_spam_log(api) 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) + def mark_as_spam! + return false unless spammable.submittable_as_spam? - if spammable.is_a?(Issuable) - SystemNoteService.submit_spam(spammable, spammable.project, current_user) - end - true + if akismet.submit_spam + spammable.user_agent_detail.update_attribute(:submitted, true) else false end end - def mark_as_ham! - return false unless spammable.is_a?(SpamLog) + private - if akismet.ham! - spammable.update_attribute(:submitted_as_ham, true) - true - else - false - end + def akismet + @akismet ||= AkismetService.new( + spammable_owner, + spammable.spammable_text, + options + ) end - private + def spammable_owner + @user ||= User.find(spammable_owner_id) + end - def akismet - @akismet ||= AkismetService.new(spammable) + def spammable_owner_id + @owner_id ||= + if spammable.respond_to?(:author_id) + spammable.author_id + elsif spammable.respond_to?(:creator_id) + spammable.creator_id + end end - def akismet_enabled? - current_application_settings.akismet_enabled + def check_for_spam? + spammable.check_for_spam? end - def create_spam_log(api, request) + def create_spam_log(api) SpamLog.create( { - user_id: spammable.owner_id, + 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), + source_ip: options[:ip_address], + user_agent: options[:user_agent], noteable_type: spammable.class.to_s, via_api: api } diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 35c9ce909e6..e13dc9265b8 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -395,23 +395,6 @@ module SystemNoteService create_note(noteable: noteable, project: project, author: author, note: body) end - # Called when Issuable is submitted as spam - # - # noteable - Noteable object - # project - Project owning noteable - # author - User performing the change - # - # Example Note text: - # - # "Issue submitted as spam." - # - # Returns the created Note object - def submit_spam(noteable, project, author) - body = "Submitted this #{noteable.class.to_s.downcase} as spam" - - create_note(noteable: noteable, project: project, author: author, note: body) - end - private def notes_for_mentioner(mentioner, noteable, notes) diff --git a/app/services/user_agent_detail_service.rb b/app/services/user_agent_detail_service.rb index c07e2ca12a6..a1ee3df5fe1 100644 --- a/app/services/user_agent_detail_service.rb +++ b/app/services/user_agent_detail_service.rb @@ -7,6 +7,7 @@ class UserAgentDetailService def create return unless request + spammable.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s) end end diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index 30e6a35db53..9f1a046ea74 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -37,10 +37,9 @@ = link_to 'Close issue', issue_path(@issue, issue: { state_event: :close }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "btn-close #{issue_button_visibility(@issue, true)}", title: 'Close issue' %li = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue) - - if @issue.can_be_submitted? && current_user.admin? - - unless @issue.submitted? - %li - = link_to 'Submit as spam', mark_as_spam_namespace_project_issue_path(@project.namespace, @project, @issue), method: :post, class: 'btn-spam', title: 'Submit as spam' + - if @issue.submittable_as_spam? && current_user.admin? + %li + = link_to 'Submit as spam', mark_as_spam_namespace_project_issue_path(@project.namespace, @project, @issue), method: :post, class: 'btn-spam', title: 'Submit as spam' - if can?(current_user, :create_issue, @project) = link_to new_namespace_project_issue_path(@project.namespace, @project), class: 'hidden-xs hidden-sm btn btn-grouped new-issue-link btn-new btn-inverted', title: 'New issue', id: 'new_issue_link' do @@ -48,10 +47,9 @@ - if can?(current_user, :update_issue, @issue) = link_to 'Reopen issue', issue_path(@issue, issue: { state_event: :reopen }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-grouped btn-reopen #{issue_button_visibility(@issue, false)}", title: 'Reopen issue' = link_to 'Close issue', issue_path(@issue, issue: { state_event: :close }, status_only: true, format: 'json'), data: {no_turbolink: true}, class: "hidden-xs hidden-sm btn btn-grouped btn-close #{issue_button_visibility(@issue, true)}", title: 'Close issue' - = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-grouped issuable-edit' - - if @issue.can_be_submitted? && current_user.admin? - - unless @issue.submitted? + - if @issue.submittable_as_spam? && current_user.admin? = link_to 'Submit as spam', mark_as_spam_namespace_project_issue_path(@project.namespace, @project, @issue), method: :post, class: 'hidden-xs hidden-sm btn btn-grouped btn-spam', title: 'Submit as spam' + = link_to 'Edit', edit_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-grouped issuable-edit' .issue-details.issuable-details diff --git a/db/migrate/20160727163552_create_user_agent_details.rb b/db/migrate/20160727163552_create_user_agent_details.rb index 6677f5e80ba..ed4ccfedc0a 100644 --- a/db/migrate/20160727163552_create_user_agent_details.rb +++ b/db/migrate/20160727163552_create_user_agent_details.rb @@ -10,7 +10,7 @@ class CreateUserAgentDetails < ActiveRecord::Migration t.string :ip_address, null: false t.integer :subject_id, null: false t.string :subject_type, null: false - t.boolean :submitted, default: false + t.boolean :submitted, default: false, null: false t.timestamps null: false end diff --git a/db/schema.rb b/db/schema.rb index 5ac08099e90..52ba60ace11 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1004,7 +1004,7 @@ ActiveRecord::Schema.define(version: 20160810142633) do t.string "ip_address", null: false t.integer "subject_id", null: false t.string "subject_type", null: false - t.boolean "submitted", default: false + t.boolean "submitted", default: false, null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false end diff --git a/spec/controllers/admin/spam_logs_controller_spec.rb b/spec/controllers/admin/spam_logs_controller_spec.rb index ac0441d0a4e..585ca31389d 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(AkismetService).to receive(:ham!).and_return(true) + allow_any_instance_of(AkismetService).to receive(:submit_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 0e8d4b80b0e..0836b71056c 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -274,7 +274,7 @@ describe Projects::IssuesController do describe 'POST #create' 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(SpamService).to receive(:check_for_spam?).and_return(true) allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) end @@ -317,7 +317,7 @@ describe Projects::IssuesController do end it 'creates a user agent detail' do - expect{ post_new_issue }.to change(UserAgentDetail, :count) + expect{ post_new_issue }.to change(UserAgentDetail, :count).by(1) end end end @@ -325,9 +325,8 @@ describe Projects::IssuesController do describe 'POST #mark_as_spam' do context 'properly submits to Akismet' do before do - allow_any_instance_of(AkismetService).to receive_messages(spam!: true) + allow_any_instance_of(AkismetService).to receive_messages(submit_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 @@ -342,13 +341,9 @@ describe Projects::IssuesController do } end - it 'creates a system note' do - expect{ post_spam }.to change(Note, :count) - end - it 'updates issue' do post_spam - expect(issue.submitted?).to be_truthy + expect(issue.submittable_as_spam?).to be_falsey end end end diff --git a/spec/factories/user_agent_details.rb b/spec/factories/user_agent_details.rb index 10de5dcb329..9763cc0cf15 100644 --- a/spec/factories/user_agent_details.rb +++ b/spec/factories/user_agent_details.rb @@ -2,11 +2,6 @@ FactoryGirl.define do factory :user_agent_detail do ip_address '127.0.0.1' user_agent 'AppleWebKit/537.36' - subject_id 1 - subject_type 'Issue' - - trait :on_issue do - association :subject, factory: :issue - end + association :subject, factory: :issue end end diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb index 7944305e7b3..32935bc0b09 100644 --- a/spec/models/concerns/spammable_spec.rb +++ b/spec/models/concerns/spammable_spec.rb @@ -9,29 +9,17 @@ describe Issue, 'Spammable' do describe 'ClassMethods' do it 'should return correct attr_spammable' do - expect(issue.send(:spammable_text)).to eq("#{issue.title}\n#{issue.description}") + expect(issue.spammable_text).to eq("#{issue.title}\n#{issue.description}") end end describe 'InstanceMethods' do - it 'should return the correct creator' do - expect(issue.owner_id).to eq(issue.author_id) - end - it 'should be invalid if spam' do issue = build(:issue, spam: true) expect(issue.valid?).to be_falsey end - it 'should not be submitted' do - create(:user_agent_detail, subject: issue) - 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) diff --git a/spec/models/user_agent_detail_spec.rb b/spec/models/user_agent_detail_spec.rb index ba21161fc7f..a8c25766e73 100644 --- a/spec/models/user_agent_detail_spec.rb +++ b/spec/models/user_agent_detail_spec.rb @@ -2,16 +2,30 @@ require 'rails_helper' describe UserAgentDetail, type: :model do describe '.submittable?' do - it 'should be submittable' do - detail = create(:user_agent_detail, :on_issue) + it 'is submittable when not already submitted' do + detail = build(:user_agent_detail) + expect(detail.submittable?).to be_truthy end + + it 'is not submittable when already submitted' do + detail = build(:user_agent_detail, submitted: true) + + expect(detail.submittable?).to be_falsey + end end describe '.valid?' do - it 'should be valid with a subject' do - detail = create(:user_agent_detail, :on_issue) + it 'is valid with a subject' do + detail = build(:user_agent_detail) + expect(detail).to be_valid end + + it 'is invalid without a subject' do + detail = build(:user_agent_detail, subject: nil) + + expect(detail).not_to be_valid + end end end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 30b939c797c..a40e1a93b71 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -531,7 +531,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(SpamService).to receive(:check_for_spam?).and_return(true) allow_any_instance_of(AkismetService).to receive_messages(is_spam?: true) end |