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

12 files changed, 122 insertions, 35 deletions

diff --git a/CHANGELOG b/CHANGELOG
index cc0d01708f1..f4567fc5bbd 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 91ff9407216..3c6f29ac0ba 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 00000000000..3b8e6df2da9
--- /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 60af8c15340..d9428ebc9fb 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 e63e1af8766..5e2de2ccf64 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 00000000000..7c3e692bde9
--- /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 5cc09bd536d..c222d21612f 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 c588103e517..c4d3134da6c 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 04676fdb748..207736b59db 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 7cf09fa4a4a..77f65057f71 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 88a71528867..b08396da4d2 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 12f2cfa6942..9d3d28e0b91 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