Commit f01fce7f authored by Patricio Cano's avatar Patricio Cano

Refactor spam validation to a concern that can be easily reused and improve...

Refactor spam validation to a concern that can be easily reused and improve legibility in `SpamCheckService`
parent 8f04cf0e
...@@ -10,6 +10,7 @@ v 8.11.0 (unreleased) ...@@ -10,6 +10,7 @@ v 8.11.0 (unreleased)
- Retrieve rendered HTML from cache in one request - Retrieve rendered HTML from cache in one request
- Nokogiri's various parsing methods are now instrumented - Nokogiri's various parsing methods are now instrumented
- Make fork counter always clickable. !5463 (winniehell) - 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) - 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` - Load project invited groups and members eagerly in `ProjectTeam#fetch_members`
- Add GitLab Workhorse version to admin dashboard (Katarzyna Kobierska Ula Budziszewska) - Add GitLab Workhorse version to admin dashboard (Katarzyna Kobierska Ula Budziszewska)
...@@ -93,7 +94,6 @@ v 8.10.0 ...@@ -93,7 +94,6 @@ v 8.10.0
- Fix viewing notification settings when a project is pending deletion - Fix viewing notification settings when a project is pending deletion
- Updated compare dropdown menus to use GL dropdown - Updated compare dropdown menus to use GL dropdown
- Redirects back to issue after clicking login link - 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 - Eager load award emoji on notes
- Allow to define manual actions/builds on Pipelines and Environments - Allow to define manual actions/builds on Pipelines and Environments
- Fix pagination when sorting by columns with lots of ties (like priority) - Fix pagination when sorting by columns with lots of ties (like priority)
......
...@@ -79,11 +79,11 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -79,11 +79,11 @@ class Projects::IssuesController < Projects::ApplicationController
end end
def create 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| respond_to do |format|
format.html do format.html do
if @issue.errors.empty? && @issue.valid? if @issue.valid?
redirect_to issue_path(@issue) redirect_to issue_path(@issue)
else else
render :new render :new
......
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
...@@ -6,6 +6,7 @@ class Issue < ActiveRecord::Base ...@@ -6,6 +6,7 @@ class Issue < ActiveRecord::Base
include Referable include Referable
include Sortable include Sortable
include Taskable include Taskable
include Spammable
DueDateStruct = Struct.new(:title, :name).freeze DueDateStruct = Struct.new(:title, :name).freeze
NoDueDate = DueDateStruct.new('No Due Date', '0').freeze NoDueDate = DueDateStruct.new('No Due Date', '0').freeze
......
...@@ -2,14 +2,13 @@ module Issues ...@@ -2,14 +2,13 @@ module Issues
class CreateService < Issues::BaseService class CreateService < Issues::BaseService
def execute def execute
filter_params filter_params
label_params = params[:label_ids] label_params = params.delete(:label_ids)
issue = project.issues.new(params.except(:label_ids, :request)) request = params.delete(:request)
api = params.delete(:api)
issue = project.issues.new(params)
issue.author = params[:author] || current_user issue.author = params[:author] || current_user
if SpamCheckService.new(project, current_user, params).spam_detected? issue.spam = spam_check_service.execute(request, api)
issue.errors.add(:base, 'Your issue has been recognized as spam and has been discarded.')
return issue
end
if issue.save if issue.save
issue.update_attributes(label_ids: label_params) issue.update_attributes(label_ids: label_params)
...@@ -22,5 +21,11 @@ module Issues ...@@ -22,5 +21,11 @@ module Issues
issue issue
end end
private
def spam_check_service
SpamCheckService.new(project, current_user, params)
end
end end
end end
class SpamCheckService class SpamCheckService < BaseService
include Gitlab::AkismetHelper include Gitlab::AkismetHelper
attr_accessor :subject, :current_user, :params attr_accessor :request, :api
def initialize(subject, user, params = {}) def execute(request, api)
@subject, @current_user, @params = subject, user, params.dup @request, @api = request, api
end return false unless request || check_for_spam?(project)
return false unless is_spam?(request.env, current_user, text)
def spam_detected? create_spam_log
request = params[:request]
return false unless request || check_for_spam?(subject)
text = [params[:title], params[:description]].reject(&:blank?).join("\n") true
end
return false unless is_spam?(request.env, current_user, text) private
attrs = { def text
[params[:title], params[:description]].reject(&:blank?).join("\n")
end
def spam_log_attrs
{
user_id: current_user.id, user_id: current_user.id,
project_id: subject.id, project_id: project.id,
title: params[:title], 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
end end
# Akismet # 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. members were submitted to Akismet.
GitLab leverages [Akismet](http://akismet.com) to protect against spam. Currently GitLab leverages [Akismet](http://akismet.com) to protect against spam. Currently
......
...@@ -158,15 +158,13 @@ module API ...@@ -158,15 +158,13 @@ module API
project = user_project 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.valid? if issue.spam?
# 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) render_api_error!({ error: 'Spam detected' }, 400)
end end
if issue.valid?
# Find or create labels and attach to issue. Labels are valid because # 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 # we already checked its name, so there can't be an error here
if params[:labels].present? if params[:labels].present?
......
...@@ -43,16 +43,5 @@ module Gitlab ...@@ -43,16 +43,5 @@ module Gitlab
false false
end end
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
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment