diff --git a/CHANGELOG b/CHANGELOG
index cc0d01708f181a9b2f3b9814133017b7e79f11bc..f4567fc5bbd8e32747f264023d4536f8fe1c9beb 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -14,6 +14,7 @@ v 8.11.0 (unreleased)
- Nokogiri's various parsing methods are now instrumented
- Add build event color in HipChat messages (David Eisner)
- 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`
- Make branches sortable without push permission !5462 (winniehell)
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 91ff9407216f0d528da5e01a699143ab94d80046..3c6f29ac0ba7ced867a9324f71b3bf053e9f3238 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -82,7 +82,7 @@ class Projects::IssuesController < Projects::ApplicationController
end
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|
format.html do
@@ -92,7 +92,7 @@ class Projects::IssuesController < Projects::ApplicationController
render :new
end
end
- format.js do |format|
+ format.js do
@link = @issue.attachment.url.to_js
end
end
diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb
new file mode 100644
index 0000000000000000000000000000000000000000..3b8e6df2da9e992583d995ad0228b9e7b8d034de
--- /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 60af8c15340af52567cb7f0d40407a3659ccce43..d9428ebc9fb0f5145f704593d6ab8c93e6fc87ac 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 e63e1af876640b87b95687a43a65baa0b980c4da..5e2de2ccf64528942cddf0482320ac5f5d7a2483 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -2,10 +2,14 @@ module Issues
class CreateService < Issues::BaseService
def execute
filter_params
- label_params = params[:label_ids]
- issue = project.issues.new(params.except(:label_ids))
+ 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
+ issue.spam = spam_check_service.execute(request, api)
+
if issue.save
issue.update_attributes(label_ids: label_params)
notification_service.new_issue(issue, current_user)
@@ -17,5 +21,11 @@ module Issues
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
new file mode 100644
index 0000000000000000000000000000000000000000..7c3e692bde977951a1640e535a32be8d1a67c492
--- /dev/null
+++ b/app/services/spam_check_service.rb
@@ -0,0 +1,38 @@
+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
diff --git a/doc/integration/akismet.md b/doc/integration/akismet.md
index 5cc09bd536de65ac8f6ff2e24d28b088a9609173..c222d21612f602a49021dfa8834b003d7b5d0330 100644
--- a/doc/integration/akismet.md
+++ b/doc/integration/akismet.md
@@ -1,9 +1,14 @@
# 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 uses Akismet to prevent users who are not members of a project from
-creating spam via the GitLab API. Detected spam will be rejected, and
-an entry in the "Spam Log" section in the Admin page will be created.
+GitLab uses Akismet to prevent the creation of spam issues on public projects. Issues
+created via the WebUI or the API can be submitted to Akismet for review.
+
+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
adding a user to a project will disable the Akismet check and prevent this
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index c588103e517eb05894ba94b7d0d9292786004a09..c4d3134da6c3e6c1031809ee3a1feb1689e86f36 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -21,17 +21,6 @@ module API
def filter_issues_milestone(issues, milestone)
issues.includes(:milestone).where('milestones.title' => milestone)
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
resource :issues do
@@ -168,15 +157,13 @@ module API
end
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)
- create_spam_log(project, current_user, attrs)
+ 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
- issue = ::Issues::CreateService.new(project, current_user, attrs).execute
-
if issue.valid?
# 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
diff --git a/lib/gitlab/akismet_helper.rb b/lib/gitlab/akismet_helper.rb
index 04676fdb74839e51813671459ae89915cd4177d8..207736b59db18fdd55c22908eb94a6416b2910f7 100644
--- a/lib/gitlab/akismet_helper.rb
+++ b/lib/gitlab/akismet_helper.rb
@@ -17,8 +17,8 @@ module Gitlab
env['HTTP_USER_AGENT']
end
- def check_for_spam?(project, user)
- akismet_enabled? && !project.team.member?(user)
+ def check_for_spam?(project)
+ akismet_enabled? && project.public?
end
def is_spam?(environment, user, text)
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 7cf09fa4a4a4687173eb1a0fe80151c017f9b85e..77f65057f711bed129cffbef43900c54d0baa4a2 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -243,6 +243,37 @@ describe Projects::IssuesController do
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
context "when the user is a developer" do
before { sign_in(user) }
diff --git a/spec/lib/gitlab/akismet_helper_spec.rb b/spec/lib/gitlab/akismet_helper_spec.rb
index 88a71528867c2d2f72296546c4d65837fcf504d7..b08396da4d20cf85ca363a254e851c0a8aba76f1 100644
--- a/spec/lib/gitlab/akismet_helper_spec.rb
+++ b/spec/lib/gitlab/akismet_helper_spec.rb
@@ -1,7 +1,7 @@
require 'spec_helper'
describe Gitlab::AkismetHelper, type: :helper do
- let(:project) { create(:project) }
+ let(:project) { create(:project, :public) }
let(:user) { create(:user) }
before do
@@ -11,13 +11,13 @@ describe Gitlab::AkismetHelper, type: :helper do
end
describe '#check_for_spam?' do
- it 'returns true for non-member' do
- expect(helper.check_for_spam?(project, user)).to eq(true)
+ it 'returns true for public project' do
+ expect(helper.check_for_spam?(project)).to eq(true)
end
- it 'returns false for member' do
- project.team << [user, :guest]
- expect(helper.check_for_spam?(project, user)).to eq(false)
+ 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
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 12f2cfa69426d4dce431f72ebcd0a70792c4a0d1..9d3d28e0b9151a940c05ed9076946b4d3ea6af67 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -531,10 +531,8 @@ describe API::API, api: true do
describe 'POST /projects/:id/issues with spam filtering' do
before do
- Grape::Endpoint.before_each do |endpoint|
- allow(endpoint).to receive(:check_for_spam?).and_return(true)
- allow(endpoint).to receive(:is_spam?).and_return(true)
- end
+ 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
let(:params) do