From f01fce7f4683e06e83d3f91d38ca5b749e27e7ec Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Thu, 21 Jul 2016 18:11:53 -0500
Subject: [PATCH] Refactor spam validation to a concern that can be easily
reused and improve legibility in `SpamCheckService`
---
CHANGELOG | 2 +-
app/controllers/projects/issues_controller.rb | 4 +-
app/models/concerns/spammable.rb | 16 +++++++
app/models/issue.rb | 1 +
app/services/issues/create_service.rb | 17 +++++---
app/services/spam_check_service.rb | 42 ++++++++++++-------
doc/integration/akismet.md | 2 +-
lib/api/issues.rb | 12 +++---
lib/gitlab/akismet_helper.rb | 11 -----
9 files changed, 63 insertions(+), 44 deletions(-)
create mode 100644 app/models/concerns/spammable.rb
diff --git a/CHANGELOG b/CHANGELOG
index 6a5b887b16ed0..a1e233a65396b 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -10,6 +10,7 @@ v 8.11.0 (unreleased)
- Retrieve rendered HTML from cache in one request
- Nokogiri's various parsing methods are now instrumented
- Make fork counter always clickable. !5463 (winniehell)
+ - All created issues, API or WebUI, can be submitted to Akismet for spam check !5333
- Remove `search_id` of labels dropdown filter to fix 'Missleading URI for labels in Merge Requests and Issues view'. !5368 (Scott Le)
- Load project invited groups and members eagerly in `ProjectTeam#fetch_members`
- Add GitLab Workhorse version to admin dashboard (Katarzyna Kobierska Ula Budziszewska)
@@ -93,7 +94,6 @@ v 8.10.0
- Fix viewing notification settings when a project is pending deletion
- Updated compare dropdown menus to use GL dropdown
- Redirects back to issue after clicking login link
- - All created issues, API or WebUI, can be submitted to Akismet for spam check !5333
- Eager load award emoji on notes
- Allow to define manual actions/builds on Pipelines and Environments
- Fix pagination when sorting by columns with lots of ties (like priority)
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index f11e3fac9592d..d169b408b4129 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -79,11 +79,11 @@ def show
end
def create
- @issue = Issues::CreateService.new(project, current_user, issue_params.merge({ request: request })).execute
+ @issue = Issues::CreateService.new(project, current_user, issue_params.merge(request: request)).execute
respond_to do |format|
format.html do
- if @issue.errors.empty? && @issue.valid?
+ if @issue.valid?
redirect_to issue_path(@issue)
else
render :new
diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb
new file mode 100644
index 0000000000000..3b8e6df2da9e9
--- /dev/null
+++ b/app/models/concerns/spammable.rb
@@ -0,0 +1,16 @@
+module Spammable
+ extend ActiveSupport::Concern
+
+ included do
+ attr_accessor :spam
+ after_validation :check_for_spam, on: :create
+ end
+
+ 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?
+ end
+end
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 60af8c15340af..d9428ebc9fb0f 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -6,6 +6,7 @@ class Issue < ActiveRecord::Base
include Referable
include Sortable
include Taskable
+ include Spammable
DueDateStruct = Struct.new(:title, :name).freeze
NoDueDate = DueDateStruct.new('No Due Date', '0').freeze
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index 1085a1b93b8d2..5e2de2ccf6452 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -2,14 +2,13 @@ module Issues
class CreateService < Issues::BaseService
def execute
filter_params
- label_params = params[:label_ids]
- issue = project.issues.new(params.except(:label_ids, :request))
+ 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
- if SpamCheckService.new(project, current_user, params).spam_detected?
- issue.errors.add(:base, 'Your issue has been recognized as spam and has been discarded.')
- return issue
- end
+ issue.spam = spam_check_service.execute(request, api)
if issue.save
issue.update_attributes(label_ids: label_params)
@@ -22,5 +21,11 @@ def execute
issue
end
+
+ private
+
+ def spam_check_service
+ SpamCheckService.new(project, current_user, params)
+ end
end
end
diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb
index 6768047aa63d5..7c3e692bde977 100644
--- a/app/services/spam_check_service.rb
+++ b/app/services/spam_check_service.rb
@@ -1,28 +1,38 @@
-class SpamCheckService
+class SpamCheckService < BaseService
include Gitlab::AkismetHelper
- attr_accessor :subject, :current_user, :params
+ attr_accessor :request, :api
- def initialize(subject, user, params = {})
- @subject, @current_user, @params = subject, user, params.dup
- end
+ 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
- def spam_detected?
- request = params[:request]
- return false unless request || check_for_spam?(subject)
+ true
+ end
- text = [params[:title], params[:description]].reject(&:blank?).join("\n")
+ private
- return false unless is_spam?(request.env, current_user, text)
-
- attrs = {
+ def text
+ [params[:title], params[:description]].reject(&:blank?).join("\n")
+ end
+
+ def spam_log_attrs
+ {
user_id: current_user.id,
- project_id: subject.id,
+ project_id: project.id,
title: params[:title],
- description: params[:description]
+ description: params[:description],
+ source_ip: client_ip(request.env),
+ user_agent: user_agent(request.env),
+ noteable_type: 'Issue',
+ via_api: api
}
- create_spam_log(subject, current_user, attrs, request.env, api: false)
+ end
- true
+ def create_spam_log
+ CreateSpamLogService.new(project, current_user, spam_log_attrs).execute
end
end
diff --git a/doc/integration/akismet.md b/doc/integration/akismet.md
index 99a28b493c9ec..c222d21612f60 100644
--- a/doc/integration/akismet.md
+++ b/doc/integration/akismet.md
@@ -1,6 +1,6 @@
# Akismet
-> *Note:* Before 8.10 only issues submitted via the API and for non-project
+> *Note:* Before 8.11 only issues submitted via the API and for non-project
members were submitted to Akismet.
GitLab leverages [Akismet](http://akismet.com) to protect against spam. Currently
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 21b9eb367e7dd..c4d3134da6c3e 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -158,15 +158,13 @@ def filter_issues_milestone(issues, milestone)
project = user_project
- issue = ::Issues::CreateService.new(project, current_user, attrs.merge({ request: request })).execute
+ issue = ::Issues::CreateService.new(project, current_user, attrs.merge(request: request, api: true)).execute
+
+ if issue.spam?
+ render_api_error!({ error: 'Spam detected' }, 400)
+ end
if issue.valid?
- # Need to check if id is nil here, because if issue is spam, errors
- # get added, but Rails still thinks it's valid, but it is never saved
- # so id will be nil
- if issue.id.nil?
- render_api_error!({ error: 'Spam detected' }, 400)
- end
# Find or create labels and attach to issue. Labels are valid because
# we already checked its name, so there can't be an error here
if params[:labels].present?
diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb
index fc1fbc5b6006d..207736b59db18 100644
--- a/lib/gitlab/akismet_helper.rb
+++ b/lib/gitlab/akismet_helper.rb
@@ -43,16 +43,5 @@ def is_spam?(environment, user, text)
false
end
end
-
- def create_spam_log(project, current_user, attrs, env, api: true)
- params = attrs.merge({
- source_ip: client_ip(env),
- user_agent: user_agent(env),
- noteable_type: 'Issue',
- via_api: api
- })
-
- ::CreateSpamLogService.new(project, current_user, params).execute
- end
end
end
--
GitLab