summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/controllers/admin/spam_logs_controller.rb6
-rw-r--r--app/models/concerns/spammable.rb44
-rw-r--r--app/models/issue.rb13
-rw-r--r--app/services/create_spam_log_service.rb13
-rw-r--r--app/services/issues/create_service.rb34
-rw-r--r--app/services/spam_check_service.rb35
-rw-r--r--app/services/user_agent_detail_service.rb8
-rw-r--r--db/migrate/20160729173930_remove_project_id_from_spam_logs.rb29
-rw-r--r--db/schema.rb1
-rw-r--r--lib/api/issues.rb2
-rw-r--r--lib/gitlab/akismet_helper.rb8
-rw-r--r--spec/controllers/projects/issues_controller_spec.rb2
-rw-r--r--spec/lib/gitlab/akismet_helper_spec.rb11
-rw-r--r--spec/models/concerns/spammable_spec.rb21
-rw-r--r--spec/requests/api/issues_spec.rb3
15 files changed, 137 insertions, 93 deletions
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
View Lorry Controller Status