Commit f6063bae authored by Douwe Maan's avatar Douwe Maan

Merge branch 'akismet-ui-check' into 'master'

Submit new issues created via the WebUI or API to Akismet for spam check on public projects.

## What does this MR do?

Submit new issues created via the WebUI by non project members to Akismet for spam check.

## Why was this MR needed?

Support for Akismet was added only to the API with !2266. This MR builds on that functionality to also check issues submitted via the WebUI for spam.

## What are the relevant issue numbers?

Related to:

- #5573 
- #5932 
- gitlab-com/infrastructure#14
- gitlab-com/support#61
- !2266

cc @stanhu @MrChrisW 

See merge request !5333
parents ce31fb48 f01fce7f
...@@ -14,6 +14,7 @@ v 8.11.0 (unreleased) ...@@ -14,6 +14,7 @@ v 8.11.0 (unreleased)
- Nokogiri's various parsing methods are now instrumented - Nokogiri's various parsing methods are now instrumented
- Add build event color in HipChat messages (David Eisner) - Add build event color in HipChat messages (David Eisner)
- 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`
- Make branches sortable without push permission !5462 (winniehell) - Make branches sortable without push permission !5462 (winniehell)
......
...@@ -82,7 +82,7 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -82,7 +82,7 @@ class Projects::IssuesController < Projects::ApplicationController
end end
def create def create
@issue = Issues::CreateService.new(project, current_user, issue_params).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
...@@ -92,7 +92,7 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -92,7 +92,7 @@ class Projects::IssuesController < Projects::ApplicationController
render :new render :new
end end
end end
format.js do |format| format.js do
@link = @issue.attachment.url.to_js @link = @issue.attachment.url.to_js
end end
end end
......
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,10 +2,14 @@ module Issues ...@@ -2,10 +2,14 @@ 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 = params.delete(:request)
api = params.delete(:api)
issue = project.issues.new(params)
issue.author = params[:author] || current_user issue.author = params[:author] || current_user
issue.spam = spam_check_service.execute(request, api)
if issue.save if issue.save
issue.update_attributes(label_ids: label_params) issue.update_attributes(label_ids: label_params)
notification_service.new_issue(issue, current_user) notification_service.new_issue(issue, current_user)
...@@ -17,5 +21,11 @@ module Issues ...@@ -17,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 < BaseService
include Gitlab::AkismetHelper
attr_accessor :request, :api
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
{
user_id: current_user.id,
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',
via_api: api
}
end
def create_spam_log
CreateSpamLogService.new(project, current_user, spam_log_attrs).execute
end
end
# Akismet # Akismet
> *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 GitLab leverages [Akismet](http://akismet.com) to protect against spam. Currently
GitLab uses Akismet to prevent users who are not members of a project from GitLab uses Akismet to prevent the creation of spam issues on public projects. Issues
creating spam via the GitLab API. Detected spam will be rejected, and created via the WebUI or the API can be submitted to Akismet for review.
an entry in the "Spam Log" section in the Admin page will be created.
Detected spam will be rejected, and an entry in the "Spam Log" section in the
Admin page will be created.
Privacy note: GitLab submits the user's IP and user agent to Akismet. Note that Privacy note: GitLab submits the user's IP and user agent to Akismet. Note that
adding a user to a project will disable the Akismet check and prevent this adding a user to a project will disable the Akismet check and prevent this
......
...@@ -21,17 +21,6 @@ module API ...@@ -21,17 +21,6 @@ module API
def filter_issues_milestone(issues, milestone) def filter_issues_milestone(issues, milestone)
issues.includes(:milestone).where('milestones.title' => milestone) issues.includes(:milestone).where('milestones.title' => milestone)
end end
def create_spam_log(project, current_user, attrs)
params = attrs.merge({
source_ip: client_ip(env),
user_agent: user_agent(env),
noteable_type: 'Issue',
via_api: true
})
::CreateSpamLogService.new(project, current_user, params).execute
end
end end
resource :issues do resource :issues do
...@@ -168,15 +157,13 @@ module API ...@@ -168,15 +157,13 @@ module API
end end
project = user_project project = user_project
text = [attrs[:title], attrs[:description]].reject(&:blank?).join("\n")
if check_for_spam?(project, current_user) && is_spam?(env, current_user, text) issue = ::Issues::CreateService.new(project, current_user, attrs.merge(request: request, api: true)).execute
create_spam_log(project, current_user, attrs)
if issue.spam?
render_api_error!({ error: 'Spam detected' }, 400) render_api_error!({ error: 'Spam detected' }, 400)
end end
issue = ::Issues::CreateService.new(project, current_user, attrs).execute
if issue.valid? 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
......
...@@ -17,8 +17,8 @@ module Gitlab ...@@ -17,8 +17,8 @@ module Gitlab
env['HTTP_USER_AGENT'] env['HTTP_USER_AGENT']
end end
def check_for_spam?(project, user) def check_for_spam?(project)
akismet_enabled? && !project.team.member?(user) akismet_enabled? && project.public?
end end
def is_spam?(environment, user, text) def is_spam?(environment, user, text)
......
...@@ -243,6 +243,37 @@ describe Projects::IssuesController do ...@@ -243,6 +243,37 @@ describe Projects::IssuesController do
end end
end end
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(Gitlab::AkismetHelper).to receive(:is_spam?).and_return(true)
end
def post_spam_issue
sign_in(user)
spam_project = create(:empty_project, :public)
post :create, {
namespace_id: spam_project.namespace.to_param,
project_id: spam_project.to_param,
issue: { title: 'Spam Title', description: 'Spam lives here' }
}
end
it 'rejects an issue recognized as spam' do
expect{ post_spam_issue }.not_to change(Issue, :count)
expect(response).to render_template(:new)
end
it 'creates a spam log' do
post_spam_issue
spam_logs = SpamLog.all
expect(spam_logs.count).to eq(1)
expect(spam_logs[0].title).to eq('Spam Title')
end
end
end
describe "DELETE #destroy" do describe "DELETE #destroy" do
context "when the user is a developer" do context "when the user is a developer" do
before { sign_in(user) } before { sign_in(user) }
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::AkismetHelper, type: :helper do describe Gitlab::AkismetHelper, type: :helper do
let(:project) { create(:project) } let(:project) { create(:project, :public) }
let(:user) { create(:user) } let(:user) { create(:user) }
before do before do
...@@ -11,13 +11,13 @@ describe Gitlab::AkismetHelper, type: :helper do ...@@ -11,13 +11,13 @@ describe Gitlab::AkismetHelper, type: :helper do
end end
describe '#check_for_spam?' do describe '#check_for_spam?' do
it 'returns true for non-member' do it 'returns true for public project' do
expect(helper.check_for_spam?(project, user)).to eq(true) expect(helper.check_for_spam?(project)).to eq(true)
end end
it 'returns false for member' do it 'returns false for private project' do
project.team << [user, :guest] project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PRIVATE)
expect(helper.check_for_spam?(project, user)).to eq(false) expect(helper.check_for_spam?(project)).to eq(false)
end end
end end
......
...@@ -531,10 +531,8 @@ describe API::API, api: true do ...@@ -531,10 +531,8 @@ describe API::API, api: true do
describe 'POST /projects/:id/issues with spam filtering' do describe 'POST /projects/:id/issues with spam filtering' do
before do before do
Grape::Endpoint.before_each do |endpoint| allow_any_instance_of(Gitlab::AkismetHelper).to receive(:check_for_spam?).and_return(true)
allow(endpoint).to receive(:check_for_spam?).and_return(true) allow_any_instance_of(Gitlab::AkismetHelper).to receive(:is_spam?).and_return(true)
allow(endpoint).to receive(:is_spam?).and_return(true)
end
end end
let(:params) do let(:params) do
......
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