From 95419679f23f0628d1885dd9656cc159e9d55ea9 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Wed, 27 Jul 2016 19:03:06 -0500 Subject: Lay the ground works to submit information to Akismet - New concern `AkismetSubmittable` to allow issues and other `Spammable` models to be submitted to Akismet. - New model `UserAgentDetail` to store information needed for Akismet. - Services needed for their creation and tests. --- app/models/concerns/akismet_submittable.rb | 15 +++++++++++++++ app/models/issue.rb | 1 + app/models/user_agent_detail.rb | 16 ++++++++++++++++ app/services/issues/create_service.rb | 5 +++++ app/services/user_agent_detail_service.rb | 12 ++++++++++++ .../20160727163552_create_user_agent_details.rb | 12 ++++++++++++ db/schema.rb | 9 +++++++++ .../controllers/projects/issues_controller_spec.rb | 20 ++++++++++++++++++++ spec/factories/user_agent_details.rb | 10 ++++++++++ spec/models/user_agent_detail_spec.rb | 22 ++++++++++++++++++++++ 10 files changed, 122 insertions(+) create mode 100644 app/models/concerns/akismet_submittable.rb create mode 100644 app/models/user_agent_detail.rb create mode 100644 app/services/user_agent_detail_service.rb create mode 100644 db/migrate/20160727163552_create_user_agent_details.rb create mode 100644 spec/factories/user_agent_details.rb create mode 100644 spec/models/user_agent_detail_spec.rb diff --git a/app/models/concerns/akismet_submittable.rb b/app/models/concerns/akismet_submittable.rb new file mode 100644 index 00000000000..17821688941 --- /dev/null +++ b/app/models/concerns/akismet_submittable.rb @@ -0,0 +1,15 @@ +module AkismetSubmittable + extend ActiveSupport::Concern + + included do + has_one :user_agent_detail, as: :subject + end + + def can_be_submitted? + if user_agent_detail + user_agent_detail.submittable? + else + false + end + end +end diff --git a/app/models/issue.rb b/app/models/issue.rb index d62ffb21467..6c2635498e5 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -8,6 +8,7 @@ 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 diff --git a/app/models/user_agent_detail.rb b/app/models/user_agent_detail.rb new file mode 100644 index 00000000000..6d76dff20e3 --- /dev/null +++ b/app/models/user_agent_detail.rb @@ -0,0 +1,16 @@ +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 + + def submittable? + user_agent.present? && ip_address.present? + end +end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 5e2de2ccf64..8e9d74103c7 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -15,6 +15,7 @@ module Issues notification_service.new_issue(issue, current_user) todo_service.new_issue(issue, current_user) event_service.open_issue(issue, current_user) + user_agent_detail_service(issue, request).create issue.create_cross_references!(current_user) execute_hooks(issue, 'open') end @@ -27,5 +28,9 @@ module Issues def spam_check_service SpamCheckService.new(project, current_user, params) end + + def user_agent_detail_service(issue, request) + UserAgentDetailService.new(issue, request) + end end end diff --git a/app/services/user_agent_detail_service.rb b/app/services/user_agent_detail_service.rb new file mode 100644 index 00000000000..dd995955be3 --- /dev/null +++ b/app/services/user_agent_detail_service.rb @@ -0,0 +1,12 @@ +class UserAgentDetailService + attr_accessor :subject, :request + + def initialize(subject, request) + @subject, @request = subject, request + end + + def create + 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) + end +end diff --git a/db/migrate/20160727163552_create_user_agent_details.rb b/db/migrate/20160727163552_create_user_agent_details.rb new file mode 100644 index 00000000000..05c21a476fa --- /dev/null +++ b/db/migrate/20160727163552_create_user_agent_details.rb @@ -0,0 +1,12 @@ +class CreateUserAgentDetails < ActiveRecord::Migration + def change + create_table :user_agent_details do |t| + t.string :user_agent, null: false + t.string :ip_address, null: false + t.integer :subject_id, null: false + t.string :subject_type, null: false + + t.timestamps null: false + end + end +end diff --git a/db/schema.rb b/db/schema.rb index f008a6bd7a7..2e5ffac5935 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -999,6 +999,15 @@ ActiveRecord::Schema.define(version: 20160810142633) do add_index "u2f_registrations", ["key_handle"], name: "index_u2f_registrations_on_key_handle", using: :btree add_index "u2f_registrations", ["user_id"], name: "index_u2f_registrations_on_user_id", using: :btree + create_table "user_agent_details", force: :cascade do |t| + t.string "user_agent", null: false + t.string "ip_address", null: false + t.integer "subject_id", null: false + t.string "subject_type", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + create_table "users", force: :cascade do |t| t.string "email", default: "", null: false t.string "encrypted_password", default: "", null: false diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index b6a0276846c..4e39826d694 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -300,6 +300,26 @@ describe Projects::IssuesController do expect(spam_logs[0].title).to eq('Spam Title') end end + + context 'user agent details are saved' do + before do + request.env['action_dispatch.remote_ip'] = '127.0.0.1' + end + + def post_new_issue + sign_in(user) + project = create(:empty_project, :public) + post :create, { + namespace_id: project.namespace.to_param, + project_id: project.to_param, + issue: { title: 'Title', description: 'Description' } + } + end + + it 'creates a user agent detail' do + expect{ post_new_issue }.to change(UserAgentDetail, :count) + end + end end describe "DELETE #destroy" do diff --git a/spec/factories/user_agent_details.rb b/spec/factories/user_agent_details.rb new file mode 100644 index 00000000000..5fc40915911 --- /dev/null +++ b/spec/factories/user_agent_details.rb @@ -0,0 +1,10 @@ +FactoryGirl.define do + factory :user_agent_detail do + ip_address '127.0.0.1' + user_agent 'AppleWebKit/537.36' + + trait :on_issue do + association :subject, factory: :issue + end + end +end diff --git a/spec/models/user_agent_detail_spec.rb b/spec/models/user_agent_detail_spec.rb new file mode 100644 index 00000000000..8debcbbeba6 --- /dev/null +++ b/spec/models/user_agent_detail_spec.rb @@ -0,0 +1,22 @@ +require 'rails_helper' + +describe UserAgentDetail, type: :model do + describe '.submittable?' do + it 'should be submittable' do + detail = create(:user_agent_detail, :on_issue) + expect(detail.submittable?).to be_truthy + end + end + + describe '.valid?' do + it 'should be valid with a subject' do + detail = create(:user_agent_detail, :on_issue) + expect(detail).to be_valid + end + + it 'should not be valid without a subject' do + detail = build(:user_agent_detail) + expect(detail).not_to be_valid + end + end +end -- cgit v1.2.1 From 722fc84e3d4785fb3a9db5f1c7d2aabad22e8e01 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Thu, 28 Jul 2016 19:02:56 -0500 Subject: Complete refactor of the `Spammable` concern and tests: - Merged `AkismetSubmittable` into `Spammable` - Clean up `SpamCheckService` - Added tests for `Spammable` - Added submit (ham or spam) options to `AkismetHelper` --- app/models/concerns/akismet_submittable.rb | 15 -------- app/models/concerns/spammable.rb | 58 ++++++++++++++++++++++++++++-- app/models/issue.rb | 2 ++ app/services/issues/create_service.rb | 2 +- app/services/spam_check_service.rb | 22 +++++------- lib/api/issues.rb | 2 +- lib/gitlab/akismet_helper.rb | 34 ++++++++++++++++++ spec/factories/user_agent_details.rb | 2 ++ spec/models/concerns/spammable_spec.rb | 41 +++++++++++++++++++++ spec/models/user_agent_detail_spec.rb | 5 --- 10 files changed, 145 insertions(+), 38 deletions(-) delete mode 100644 app/models/concerns/akismet_submittable.rb create mode 100644 spec/models/concerns/spammable_spec.rb diff --git a/app/models/concerns/akismet_submittable.rb b/app/models/concerns/akismet_submittable.rb deleted file mode 100644 index 17821688941..00000000000 --- a/app/models/concerns/akismet_submittable.rb +++ /dev/null @@ -1,15 +0,0 @@ -module AkismetSubmittable - extend ActiveSupport::Concern - - included do - has_one :user_agent_detail, as: :subject - end - - def can_be_submitted? - if user_agent_detail - user_agent_detail.submittable? - else - false - end - end -end diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 3b8e6df2da9..bbf6a3e0be3 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -1,16 +1,70 @@ module Spammable extend ActiveSupport::Concern + include Gitlab::AkismetHelper + + module ClassMethods + def attr_spammable(*attrs) + attrs.each do |attr| + spammable_attrs << attr.to_s + end + end + end included do + has_one :user_agent_detail, as: :subject, dependent: :destroy attr_accessor :spam after_validation :check_for_spam, on: :create + + cattr_accessor :spammable_attrs, instance_accessor: false do + [] + end + end + + def can_be_submitted? + if user_agent_detail + user_agent_detail.submittable? + else + false + end + end + + def submit_ham + return unless akismet_enabled? && can_be_submitted? + ham!(user_agent_detail, spammable_text, creator) + end + + def submit_spam + return unless akismet_enabled? && can_be_submitted? + spam!(user_agent_detail, spammable_text, creator) + end + + def spam?(env, user) + is_spam?(env, user, spammable_text) end - def spam? + def spam_detected? @spam 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_detected? + 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 creator + if self.author_id + User.find(self.author_id) + elsif self.creator_id + User.find(self.creator_id) + end end end diff --git a/app/models/issue.rb b/app/models/issue.rb index 6c2635498e5..5408e24b21c 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -37,6 +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 + state_machine :state, initial: :opened do event :close do transition [:reopened, :opened] => :closed diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 8e9d74103c7..d580834be83 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_check_service.execute(request, api) + issue.spam = spam_check_service.execute(request, api, issue) if issue.save issue.update_attributes(label_ids: label_params) diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb index 7c3e692bde9..7d6754546a8 100644 --- a/app/services/spam_check_service.rb +++ b/app/services/spam_check_service.rb @@ -1,23 +1,17 @@ class SpamCheckService < BaseService - include Gitlab::AkismetHelper + attr_accessor :request, :api, :subject - attr_accessor :request, :api + def execute(request, api, subject) + @request, @api, @subject = request, api, subject + return false unless request || subject.check_for_spam?(project) + return false unless subject.spam?(request.env, current_user) - def execute(request, api) - @request, @api = request, api - return false unless request || check_for_spam?(project) - return false unless is_spam?(request.env, current_user, text) - create_spam_log true end private - - def text - [params[:title], params[:description]].reject(&:blank?).join("\n") - end def spam_log_attrs { @@ -25,9 +19,9 @@ class SpamCheckService < BaseService project_id: project.id, title: params[:title], description: params[:description], - source_ip: client_ip(request.env), - user_agent: user_agent(request.env), - noteable_type: 'Issue', + source_ip: subject.client_ip(request.env), + user_agent: subject.user_agent(request.env), + noteable_type: subject.class.to_s, via_api: api } end diff --git a/lib/api/issues.rb b/lib/api/issues.rb index c4d3134da6c..7bbfc137c2c 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -160,7 +160,7 @@ module API issue = ::Issues::CreateService.new(project, current_user, attrs.merge(request: request, api: true)).execute - if issue.spam? + if issue.spam_detected? render_api_error!({ error: 'Spam detected' }, 400) end diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb index 207736b59db..19e73820321 100644 --- a/lib/gitlab/akismet_helper.rb +++ b/lib/gitlab/akismet_helper.rb @@ -43,5 +43,39 @@ module Gitlab false end end + + def ham!(details, text, user) + client = akismet_client + + params = { + type: 'comment', + text: text, + author: user.name, + author_email: user.email + } + + begin + client.submit_ham(details.ip_address, details.user_agent, params) + rescue => e + Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") + 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) + rescue => e + Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") + end + end end end diff --git a/spec/factories/user_agent_details.rb b/spec/factories/user_agent_details.rb index 5fc40915911..10de5dcb329 100644 --- a/spec/factories/user_agent_details.rb +++ b/spec/factories/user_agent_details.rb @@ -2,6 +2,8 @@ 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 diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb new file mode 100644 index 00000000000..e61a6dcb69d --- /dev/null +++ b/spec/models/concerns/spammable_spec.rb @@ -0,0 +1,41 @@ +require 'spec_helper' + +describe Issue, 'Spammable' do + let(:issue) { create(:issue, description: 'Test Desc.') } + + describe 'Associations' do + it { is_expected.to have_one(:user_agent_detail).dependent(:destroy) } + end + + describe 'ClassMethods' do + it 'should return correct attr_spammable' do + expect(issue.send(:spammable_text)).to eq("#{issue.title}\n#{issue.description}") + end + end + + describe 'InstanceMethods' do + it 'should return the correct creator' do + expect(issue.send(:creator).id).to eq(issue.author_id) + end + + it 'should be invalid if spam' do + issue.spam = true + expect(issue.valid?).to be_truthy + end + + it 'should be submittable' do + create(:user_agent_detail, subject_id: issue.id, subject_type: issue.class.to_s) + expect(issue.can_be_submitted?).to be_truthy + end + end + + describe 'AkismetMethods' do + before do + allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(check_for_spam?: true, is_spam?: true, ham!: nil, spam!: nil) + end + + it { expect(issue.spam?(:mock_env, :mock_user)).to be_truthy } + it { expect(issue.submit_spam).to be_nil } + it { expect(issue.submit_ham).to be_nil } + end +end diff --git a/spec/models/user_agent_detail_spec.rb b/spec/models/user_agent_detail_spec.rb index 8debcbbeba6..ba21161fc7f 100644 --- a/spec/models/user_agent_detail_spec.rb +++ b/spec/models/user_agent_detail_spec.rb @@ -13,10 +13,5 @@ describe UserAgentDetail, type: :model do detail = create(:user_agent_detail, :on_issue) expect(detail).to be_valid end - - it 'should not be valid without a subject' do - detail = build(:user_agent_detail) - expect(detail).not_to be_valid - end end end -- cgit v1.2.1 From 64ab2b3d9f10366249c03a6bcf5e8b1d20010d8f Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Fri, 29 Jul 2016 23:18:32 -0500 Subject: 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 --- app/controllers/admin/spam_logs_controller.rb | 6 +++ app/models/concerns/spammable.rb | 44 ++++++++++++++-------- app/models/issue.rb | 13 +++++++ app/services/create_spam_log_service.rb | 13 ------- app/services/issues/create_service.rb | 34 ++++++++--------- app/services/spam_check_service.rb | 35 ++++++++--------- app/services/user_agent_detail_service.rb | 8 ++-- ...60729173930_remove_project_id_from_spam_logs.rb | 29 ++++++++++++++ db/schema.rb | 1 - lib/api/issues.rb | 2 +- lib/gitlab/akismet_helper.rb | 8 +--- .../controllers/projects/issues_controller_spec.rb | 2 +- spec/lib/gitlab/akismet_helper_spec.rb | 11 ------ spec/models/concerns/spammable_spec.rb | 21 +++++++++-- spec/requests/api/issues_spec.rb | 3 +- 15 files changed, 137 insertions(+), 93 deletions(-) delete mode 100644 app/services/create_spam_log_service.rb create mode 100644 db/migrate/20160729173930_remove_project_id_from_spam_logs.rb diff --git a/app/controllers/admin/spam_logs_controller.rb b/app/controllers/admin/spam_logs_controller.rb index 3a2f0185315..bc6fce0ec4e 100644 --- a/app/controllers/admin/spam_logs_controller.rb +++ b/app/controllers/admin/spam_logs_controller.rb @@ -14,4 +14,10 @@ class Admin::SpamLogsController < Admin::ApplicationController head :ok 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 diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index bbf6a3e0be3..5c75275b6e2 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -28,26 +28,42 @@ module Spammable end end - def submit_ham - return unless akismet_enabled? && can_be_submitted? - ham!(user_agent_detail, spammable_text, creator) - end - def submit_spam return unless akismet_enabled? && can_be_submitted? - spam!(user_agent_detail, spammable_text, creator) + spam!(user_agent_detail, spammable_text, owner) end - def spam?(env, user) - is_spam?(env, user, spammable_text) + def spam_detected?(env) + @spam = is_spam?(env, owner, spammable_text) end - def spam_detected? + def spam? @spam 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_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 private @@ -60,11 +76,7 @@ module Spammable result.reject(&:blank?).join("\n") end - def creator - if self.author_id - User.find(self.author_id) - elsif self.creator_id - User.find(self.creator_id) - end + def owner + User.find(owner_id) end end diff --git a/app/models/issue.rb b/app/models/issue.rb index 5408e24b21c..40028e56489 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -265,4 +265,17 @@ class Issue < ActiveRecord::Base def overdue? due_date.try(:past?) || false 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 diff --git a/app/services/create_spam_log_service.rb b/app/services/create_spam_log_service.rb deleted file mode 100644 index 59a66fde47a..00000000000 --- a/app/services/create_spam_log_service.rb +++ /dev/null @@ -1,13 +0,0 @@ -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 diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index d580834be83..9f8a642a75b 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -3,34 +3,34 @@ module Issues def execute filter_params label_params = params.delete(:label_ids) - request = params.delete(:request) - api = params.delete(:api) - issue = project.issues.new(params) - issue.author = params[:author] || current_user + @request = params.delete(:request) + @api = params.delete(:api) + @issue = project.issues.new(params) + @issue.author = params[:author] || current_user - issue.spam = spam_check_service.execute(request, api, issue) + spam_check_service.execute - if issue.save - issue.update_attributes(label_ids: label_params) - notification_service.new_issue(issue, current_user) - todo_service.new_issue(issue, current_user) - event_service.open_issue(issue, current_user) - user_agent_detail_service(issue, request).create - issue.create_cross_references!(current_user) - execute_hooks(issue, 'open') + if @issue.save + @issue.update_attributes(label_ids: label_params) + notification_service.new_issue(@issue, current_user) + todo_service.new_issue(@issue, current_user) + event_service.open_issue(@issue, current_user) + user_agent_detail_service.create + @issue.create_cross_references!(current_user) + execute_hooks(@issue, 'open') end - issue + @issue end private def spam_check_service - SpamCheckService.new(project, current_user, params) + SpamCheckService.new(@request, @api, @issue) end - def user_agent_detail_service(issue, request) - UserAgentDetailService.new(issue, request) + def user_agent_detail_service + UserAgentDetailService.new(@issue, @request) end end end diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb index 7d6754546a8..71b9436a22e 100644 --- a/app/services/spam_check_service.rb +++ b/app/services/spam_check_service.rb @@ -1,32 +1,33 @@ -class SpamCheckService < BaseService - attr_accessor :request, :api, :subject +class SpamCheckService + attr_accessor :request, :api, :spammable - def execute(request, api, subject) - @request, @api, @subject = request, api, subject - return false unless request || subject.check_for_spam?(project) - return false unless subject.spam?(request.env, current_user) - - create_spam_log + def initialize(request, api, spammable) + @request, @api, @spammable = request, api, spammable + end - true + 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: current_user.id, - project_id: project.id, - title: params[:title], - description: params[:description], - source_ip: subject.client_ip(request.env), - user_agent: subject.user_agent(request.env), - noteable_type: subject.class.to_s, + 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 - CreateSpamLogService.new(project, current_user, spam_log_attrs).execute + SpamLog.create(spam_log_attrs) end end diff --git a/app/services/user_agent_detail_service.rb b/app/services/user_agent_detail_service.rb index dd995955be3..c07e2ca12a6 100644 --- a/app/services/user_agent_detail_service.rb +++ b/app/services/user_agent_detail_service.rb @@ -1,12 +1,12 @@ class UserAgentDetailService - attr_accessor :subject, :request + attr_accessor :spammable, :request - def initialize(subject, request) - @subject, @request = subject, request + def initialize(spammable, request) + @spammable, @request = spammable, request end def create 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 diff --git a/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb new file mode 100644 index 00000000000..5950874d5af --- /dev/null +++ b/db/migrate/20160729173930_remove_project_id_from_spam_logs.rb @@ -0,0 +1,29 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 2e5ffac5935..cc881e54763 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -926,7 +926,6 @@ ActiveRecord::Schema.define(version: 20160810142633) do t.string "source_ip" t.string "user_agent" t.boolean "via_api" - t.integer "project_id" t.string "noteable_type" t.string "title" t.text "description" diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 7bbfc137c2c..c4d3134da6c 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -160,7 +160,7 @@ module API 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) end diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb index 19e73820321..b74d8176cc7 100644 --- a/lib/gitlab/akismet_helper.rb +++ b/lib/gitlab/akismet_helper.rb @@ -17,10 +17,6 @@ module Gitlab env['HTTP_USER_AGENT'] end - def check_for_spam?(project) - akismet_enabled? && project.public? - end - def is_spam?(environment, user, text) client = akismet_client ip_address = client_ip(environment) @@ -44,7 +40,7 @@ module Gitlab end end - def ham!(details, text, user) + def ham!(ip_address, user_agent, text, user) client = akismet_client params = { @@ -55,7 +51,7 @@ module Gitlab } begin - client.submit_ham(details.ip_address, details.user_agent, params) + client.submit_ham(ip_address, user_agent, params) rescue => e Rails.logger.error("Unable to connect to Akismet: #{e}, skipping!") end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 4e39826d694..efca838613f 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(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) end diff --git a/spec/lib/gitlab/akismet_helper_spec.rb b/spec/lib/gitlab/akismet_helper_spec.rb index b08396da4d2..80b4f912d41 100644 --- a/spec/lib/gitlab/akismet_helper_spec.rb +++ b/spec/lib/gitlab/akismet_helper_spec.rb @@ -10,17 +10,6 @@ describe Gitlab::AkismetHelper, type: :helper do allow_any_instance_of(ApplicationSetting).to receive(:akismet_api_key).and_return('12345') 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 it 'returns true for spam' do environment = { diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb index e61a6dcb69d..3f7d2721d22 100644 --- a/spec/models/concerns/spammable_spec.rb +++ b/spec/models/concerns/spammable_spec.rb @@ -15,7 +15,7 @@ describe Issue, 'Spammable' do describe 'InstanceMethods' 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 it 'should be invalid if spam' do @@ -27,15 +27,28 @@ describe Issue, 'Spammable' do create(:user_agent_detail, subject_id: issue.id, subject_type: issue.class.to_s) expect(issue.can_be_submitted?).to be_truthy 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 describe 'AkismetMethods' 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 - 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_ham).to be_nil } end end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 3cd4e981fb2..353b01d4a09 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(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) end @@ -554,7 +554,6 @@ describe API::API, api: true do expect(spam_logs[0].description).to eq('content here') expect(spam_logs[0].user).to eq(user) expect(spam_logs[0].noteable_type).to eq('Issue') - expect(spam_logs[0].project_id).to eq(project.id) end end -- cgit v1.2.1 From abf2dcd25c4a176801314872733ede91297d1ab0 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Mon, 1 Aug 2016 12:14:03 -0500 Subject: Allow `SpamLog` to be submitted as ham - Added `submitted_as_ham` to `SpamLog` to mark which logs have been submitted to Akismet. - Added routes and controller action. --- app/controllers/admin/spam_logs_controller.rb | 11 +++++++++-- app/models/spam_log.rb | 4 ++++ app/views/admin/spam_logs/_spam_log.html.haml | 5 +++++ config/routes.rb | 6 +++++- ...160801163709_add_submitted_as_ham_to_spam_logs.rb | 20 ++++++++++++++++++++ db/schema.rb | 1 + lib/gitlab/akismet_helper.rb | 4 ++++ spec/models/concerns/spammable_spec.rb | 5 +++-- 8 files changed, 51 insertions(+), 5 deletions(-) create mode 100644 db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb diff --git a/app/controllers/admin/spam_logs_controller.rb b/app/controllers/admin/spam_logs_controller.rb index bc6fce0ec4e..d15f00bf84c 100644 --- a/app/controllers/admin/spam_logs_controller.rb +++ b/app/controllers/admin/spam_logs_controller.rb @@ -1,4 +1,6 @@ class Admin::SpamLogsController < Admin::ApplicationController + include Gitlab::AkismetHelper + def index @spam_logs = SpamLog.order(id: :desc).page(params[:page]) end @@ -15,9 +17,14 @@ class Admin::SpamLogsController < Admin::ApplicationController end end - def ham + def mark_as_ham spam_log = SpamLog.find(params[:id]) - Gitlab::AkismetHelper.ham!(spam_log.source_ip, spam_log.user_agent, spam_log.description, spam_log.user) + if ham!(spam_log.source_ip, spam_log.user_agent, spam_log.text, spam_log.user) + spam_log.update_attribute(:submitted_as_ham, true) + 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.' + end end end diff --git a/app/models/spam_log.rb b/app/models/spam_log.rb index 12df68ef83b..3b8b9833565 100644 --- a/app/models/spam_log.rb +++ b/app/models/spam_log.rb @@ -7,4 +7,8 @@ class SpamLog < ActiveRecord::Base user.block user.destroy end + + def text + [title, description].join("\n") + end end diff --git a/app/views/admin/spam_logs/_spam_log.html.haml b/app/views/admin/spam_logs/_spam_log.html.haml index 8aea67f4497..f2b6106fb4a 100644 --- a/app/views/admin/spam_logs/_spam_log.html.haml +++ b/app/views/admin/spam_logs/_spam_log.html.haml @@ -24,6 +24,11 @@ = link_to 'Remove user', admin_spam_log_path(spam_log, remove_user: true), data: { confirm: "USER #{user.name} WILL BE REMOVED! Are you sure?" }, method: :delete, class: "btn btn-xs btn-remove" %td + - if spam_log.submitted_as_ham? + .btn.btn-xs.disabled + Submitted as Ham + - else + = link_to 'Submit as ham', mark_as_ham_admin_spam_log_path(spam_log), method: :post, class: 'btn btn-xs btn-warning' - if user && !user.blocked? = link_to 'Block user', block_admin_user_path(user), data: {confirm: 'USER WILL BE BLOCKED! Are you sure?'}, method: :put, class: "btn btn-xs" - else diff --git a/config/routes.rb b/config/routes.rb index 9a98fab15a3..8214bc26d59 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -252,7 +252,11 @@ Rails.application.routes.draw do resource :impersonation, only: :destroy resources :abuse_reports, only: [:index, :destroy] - resources :spam_logs, only: [:index, :destroy] + resources :spam_logs, only: [:index, :destroy] do + member do + post :mark_as_ham + end + end resources :applications diff --git a/db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb b/db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb new file mode 100644 index 00000000000..296f1dfac7b --- /dev/null +++ b/db/migrate/20160801163709_add_submitted_as_ham_to_spam_logs.rb @@ -0,0 +1,20 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddSubmittedAsHamToSpamLogs < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + # 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 = '' + + disable_ddl_transaction! + + def change + add_column_with_default :spam_logs, :submitted_as_ham, :boolean, default: false + end +end diff --git a/db/schema.rb b/db/schema.rb index cc881e54763..355eed13b68 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -931,6 +931,7 @@ ActiveRecord::Schema.define(version: 20160810142633) do t.text "description" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.boolean "submitted_as_ham", default: false, null: false end create_table "subscriptions", force: :cascade do |t| diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb index b74d8176cc7..bd71a1aaa51 100644 --- a/lib/gitlab/akismet_helper.rb +++ b/lib/gitlab/akismet_helper.rb @@ -52,8 +52,10 @@ module Gitlab 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 @@ -69,8 +71,10 @@ module Gitlab 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 diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb index 3f7d2721d22..7492c42f71e 100644 --- a/spec/models/concerns/spammable_spec.rb +++ b/spec/models/concerns/spammable_spec.rb @@ -45,10 +45,11 @@ describe Issue, 'Spammable' do describe 'AkismetMethods' do before do - allow_any_instance_of(Gitlab::AkismetHelper).to receive_messages(is_spam?: true, spam!: nil) + 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_nil } + it { expect(issue.submit_spam).to be_truthy } end end -- cgit v1.2.1 From 96399a81cbb2e8a0f666241eeaff7cc784c26983 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Tue, 2 Aug 2016 16:21:57 -0500 Subject: Allow `Issue` to be submitted as spam - Added controller actions as reusable concerns - Added controller tests --- app/assets/stylesheets/framework/buttons.scss | 4 +++ app/controllers/concerns/spammable_actions.rb | 32 ++++++++++++++++++++++ app/controllers/projects/issues_controller.rb | 2 ++ app/models/concerns/spammable.rb | 18 ++++++++++-- app/services/system_note_service.rb | 17 ++++++++++++ app/views/admin/spam_logs/_spam_log.html.haml | 2 +- app/views/projects/issues/show.html.haml | 11 ++++++-- config/routes.rb | 1 + .../20160727163552_create_user_agent_details.rb | 1 + db/schema.rb | 1 + .../controllers/admin/spam_logs_controller_spec.rb | 12 ++++++++ .../controllers/projects/issues_controller_spec.rb | 29 ++++++++++++++++++++ spec/models/concerns/spammable_spec.rb | 9 +++--- 13 files changed, 129 insertions(+), 10 deletions(-) create mode 100644 app/controllers/concerns/spammable_actions.rb diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index 473530cf094..f1fe1697d30 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -164,6 +164,10 @@ @include btn-outline($white-light, $orange-normal, $orange-normal, $orange-light, $white-light, $orange-light); } + &.btn-spam { + @include btn-outline($white-light, $red-normal, $red-normal, $red-light, $white-light, $red-light); + } + &.btn-danger, &.btn-remove, &.btn-red { diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb new file mode 100644 index 00000000000..85be25d84cc --- /dev/null +++ b/app/controllers/concerns/spammable_actions.rb @@ -0,0 +1,32 @@ +module SpammableActions + extend ActiveSupport::Concern + + included do + before_action :authorize_submit_spammable!, only: :mark_as_spam + 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 + + redirect_to spammable, notice: 'Issue was submitted to Akismet successfully.' + else + flash[:error] = 'Error with Akismet. Please check the logs for more info.' + redirect_to spammable + end + end + + private + + def spammable + raise NotImplementedError + end + + def authorize_submit_spammable! + access_denied! unless current_user.admin? + end +end diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 660e0eba06f..e9fb11e8f94 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -4,6 +4,7 @@ class Projects::IssuesController < Projects::ApplicationController include IssuableActions include ToggleAwardEmoji include IssuableCollections + include SpammableActions before_action :redirect_to_external_issue_tracker, only: [:index, :new] before_action :module_enabled @@ -185,6 +186,7 @@ class Projects::IssuesController < Projects::ApplicationController alias_method :subscribable_resource, :issue alias_method :issuable, :issue alias_method :awardable, :issue + alias_method :spammable, :issue def authorize_read_issue! return render_404 unless can?(current_user, :read_issue, @issue) diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 5c75275b6e2..f272e7c5a55 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -22,7 +22,7 @@ module Spammable def can_be_submitted? if user_agent_detail - user_agent_detail.submittable? + user_agent_detail.submittable? && akismet_enabled? else false end @@ -41,6 +41,14 @@ module Spammable @spam end + def submitted? + 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? end @@ -53,17 +61,21 @@ 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 'Implement in included model!' + raise NotImplementedError end def spam_description - raise 'Implement in included model!' + raise NotImplementedError end private diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index e13dc9265b8..56d3329f5bd 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -395,6 +395,23 @@ module SystemNoteService create_note(noteable: noteable, project: project, author: author, note: body) end + # Called when the status of a 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 #{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/views/admin/spam_logs/_spam_log.html.haml b/app/views/admin/spam_logs/_spam_log.html.haml index f2b6106fb4a..4ce4eab8753 100644 --- a/app/views/admin/spam_logs/_spam_log.html.haml +++ b/app/views/admin/spam_logs/_spam_log.html.haml @@ -26,7 +26,7 @@ %td - if spam_log.submitted_as_ham? .btn.btn-xs.disabled - Submitted as Ham + Submitted as ham - else = link_to 'Submit as ham', mark_as_ham_admin_spam_log_path(spam_log), method: :post, class: 'btn btn-xs btn-warning' - if user && !user.blocked? diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index e5cce16a171..30e6a35db53 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -37,14 +37,21 @@ = 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 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 New issue - 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_namespace_project_issue_path(@project.namespace, @project, @issue), class: 'hidden-xs hidden-sm btn btn-grouped issuable-edit' do - Edit + = 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? + = 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' .issue-details.issuable-details diff --git a/config/routes.rb b/config/routes.rb index 8214bc26d59..2cf2d111920 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -817,6 +817,7 @@ Rails.application.routes.draw do member do post :toggle_subscription post :toggle_award_emoji + post :mark_as_spam get :referenced_merge_requests get :related_branches get :can_create_branch diff --git a/db/migrate/20160727163552_create_user_agent_details.rb b/db/migrate/20160727163552_create_user_agent_details.rb index 05c21a476fa..f9a02f310da 100644 --- a/db/migrate/20160727163552_create_user_agent_details.rb +++ b/db/migrate/20160727163552_create_user_agent_details.rb @@ -5,6 +5,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.timestamps null: false end diff --git a/db/schema.rb b/db/schema.rb index 355eed13b68..5ac08099e90 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1004,6 +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.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 520a4f6f9c5..f94afd1139d 100644 --- a/spec/controllers/admin/spam_logs_controller_spec.rb +++ b/spec/controllers/admin/spam_logs_controller_spec.rb @@ -34,4 +34,16 @@ describe Admin::SpamLogsController do expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound) end end + + describe '#mark_as_ham' do + before do + allow_any_instance_of(Gitlab::AkismetHelper).to receive(:ham!).and_return(true) + end + it 'submits the log as ham' do + post :mark_as_ham, id: first_spam.id + + expect(response).to have_http_status(302) + expect(SpamLog.find(first_spam.id).submitted_as_ham).to be_truthy + end + end end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index efca838613f..8fcde9a38bc 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -322,6 +322,35 @@ describe Projects::IssuesController do end end + 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) + end + + def post_spam + admin = create(:admin) + create(:user_agent_detail, subject: issue) + project.team << [admin, :master] + sign_in(admin) + post :mark_as_spam, { + namespace_id: project.namespace.path, + project_id: project.path, + id: issue.iid + } + 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 + end + end + end + describe "DELETE #destroy" do context "when the user is a developer" do before { sign_in(user) } diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb index 7492c42f71e..4e52d05918f 100644 --- a/spec/models/concerns/spammable_spec.rb +++ b/spec/models/concerns/spammable_spec.rb @@ -14,6 +14,10 @@ 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) end @@ -24,14 +28,11 @@ describe Issue, 'Spammable' do end it 'should be submittable' do - create(:user_agent_detail, subject_id: issue.id, subject_type: issue.class.to_s) + create(:user_agent_detail, subject: issue) expect(issue.can_be_submitted?).to be_truthy 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) -- cgit v1.2.1 From 7179165af7553720089a0b7e7024374c371e2f90 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Thu, 4 Aug 2016 12:29:43 -0500 Subject: Added Documentation and CHANGELOG --- CHANGELOG | 1 + doc/integration/akismet.md | 23 +++++++++++++++++++++++ doc/integration/img/spam_log.png | Bin 0 -> 187190 bytes doc/integration/img/submit_issue.png | Bin 0 -> 176450 bytes 4 files changed, 24 insertions(+) create mode 100644 doc/integration/img/spam_log.png create mode 100644 doc/integration/img/submit_issue.png diff --git a/CHANGELOG b/CHANGELOG index ccc60846787..4aad4545509 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -81,6 +81,7 @@ v 8.11.0 (unreleased) - Make "New issue" button in Issue page less obtrusive !5457 (winniehell) - Gitlab::Metrics.current_transaction needs to be public for RailsQueueDuration - Fix search for notes which belongs to deleted objects + - Allow Akismet to be trained by submitting issues as spam or ham !5538 - Add GitLab Workhorse version to admin dashboard (Katarzyna Kobierska Ula Budziszewska) - Allow branch names ending with .json for graph and network page !5579 (winniehell) - Add the `sprockets-es6` gem diff --git a/doc/integration/akismet.md b/doc/integration/akismet.md index c222d21612f..06c787cfcc7 100644 --- a/doc/integration/akismet.md +++ b/doc/integration/akismet.md @@ -33,3 +33,26 @@ To use Akismet: 7. Save the configuration. ![Screenshot of Akismet settings](img/akismet_settings.png) + + +## Training + +> *Note:* Training the Akismet filter is only available in 8.11 and above. + +As a way to better recognize between spam and ham, you can train the Akismet +filter whenever there is a false positive or false negative. + +When an entry is recognized as spam, it is rejected and added to the Spam Logs. +From here you can review if they are really spam. If one of them is not really +spam, you can use the `Submit as ham` button to tell Akismet that it falsely +recognized an entry as spam. + +![Screenshot of Spam Logs](img/spam_log.png) + +If an entry that is actually spam was not recognized as such, you will be able +to also submit this to Akismet. The `Submit as spam` button will only appear +to admin users. + +![Screenshot of Issue](img/submit_issue.png) + +Training Akismet will help it to recognize spam more accurately in the future. diff --git a/doc/integration/img/spam_log.png b/doc/integration/img/spam_log.png new file mode 100644 index 00000000000..8d574448690 Binary files /dev/null and b/doc/integration/img/spam_log.png differ diff --git a/doc/integration/img/submit_issue.png b/doc/integration/img/submit_issue.png new file mode 100644 index 00000000000..9fd9c20f6b3 Binary files /dev/null and b/doc/integration/img/submit_issue.png differ -- cgit v1.2.1 From 43e756d4eafd79f4d2f366b646ebb94af78b5a4c Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Fri, 5 Aug 2016 17:10:08 -0500 Subject: Refactored AkismetHelper into AkismetService and cleaned up `Spammable` - Refactored SpamCheckService into SpamService --- app/controllers/admin/spam_logs_controller.rb | 4 +- app/controllers/concerns/spammable_actions.rb | 8 +-- app/models/concerns/spammable.rb | 68 ++++++++---------- app/models/issue.rb | 13 +--- app/services/akismet_service.rb | 78 +++++++++++++++++++++ app/services/issues/create_service.rb | 6 +- app/services/spam_check_service.rb | 33 --------- app/services/spam_service.rb | 64 +++++++++++++++++ app/services/system_note_service.rb | 4 +- .../20160727163552_create_user_agent_details.rb | 5 ++ ...60729173930_remove_project_id_from_spam_logs.rb | 2 +- lib/api/issues.rb | 2 - lib/gitlab/akismet_helper.rb | 81 ---------------------- .../controllers/admin/spam_logs_controller_spec.rb | 2 +- .../controllers/projects/issues_controller_spec.rb | 6 +- spec/lib/gitlab/akismet_helper_spec.rb | 24 ------- spec/models/concerns/spammable_spec.rb | 27 +++----- spec/requests/api/issues_spec.rb | 2 +- 18 files changed, 200 insertions(+), 229 deletions(-) create mode 100644 app/services/akismet_service.rb delete mode 100644 app/services/spam_check_service.rb create mode 100644 app/services/spam_service.rb delete mode 100644 lib/gitlab/akismet_helper.rb delete mode 100644 spec/lib/gitlab/akismet_helper_spec.rb 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 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 -- cgit v1.2.1 From 5994c11910822463faeabb7b5f11d6529036db9d Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Tue, 9 Aug 2016 12:43:47 -0500 Subject: Further refactor and syntax fixes. --- app/controllers/admin/spam_logs_controller.rb | 5 +- app/controllers/concerns/spammable_actions.rb | 9 ++- app/models/concerns/spammable.rb | 42 ++++-------- app/models/issue.rb | 3 +- app/models/user_agent_detail.rb | 11 +--- app/services/akismet_service.rb | 59 ++++++++--------- app/services/ham_service.rb | 26 ++++++++ app/services/issues/create_service.rb | 4 +- app/services/spam_service.rb | 76 +++++++++++++--------- app/services/system_note_service.rb | 17 ----- app/services/user_agent_detail_service.rb | 1 + app/views/projects/issues/show.html.haml | 12 ++-- .../20160727163552_create_user_agent_details.rb | 2 +- db/schema.rb | 2 +- .../controllers/admin/spam_logs_controller_spec.rb | 2 +- .../controllers/projects/issues_controller_spec.rb | 13 ++-- spec/factories/user_agent_details.rb | 7 +- spec/models/concerns/spammable_spec.rb | 14 +--- spec/models/user_agent_detail_spec.rb | 22 +++++-- spec/requests/api/issues_spec.rb | 2 +- 20 files changed, 160 insertions(+), 169 deletions(-) create mode 100644 app/services/ham_service.rb 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 -- cgit v1.2.1