Merge branch 'mention-all' into 'master'

Only allow group/project members to mention `@all`

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/3473

See merge request !2205

11 files changed, 66 insertions, 18 deletions

diff --git a/CHANGELOG b/CHANGELOG
index a20c3978a11..8135a5d5180 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -5,6 +5,7 @@ v 8.4.0 (unreleased)
   - Implement search inside emoji picker
   - Add API support for looking up a user by username (Stan Hu)
   - Add project permissions to all project API endpoints (Stan Hu)
+  - Only allow group/project members to mention `@all`
   - Expose Git's version in the admin area
   - Add "Frequently used" category to emoji picker
   - Add CAS support (tduehr)
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 2dab04f2a7c..3004722bce0 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -178,7 +178,7 @@ class ProjectsController < ApplicationController
   def markdown_preview
     text = params[:text]
 
-    ext = Gitlab::ReferenceExtractor.new(@project, current_user)
+    ext = Gitlab::ReferenceExtractor.new(@project, current_user, current_user)
     ext.analyze(text)
 
     render json: {
diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb
index 1fdcda97520..6316ee208b5 100644
--- a/app/models/concerns/mentionable.rb
+++ b/app/models/concerns/mentionable.rb
@@ -44,7 +44,7 @@ module Mentionable
   end
 
   def all_references(current_user = self.author, text = nil)
-    ext = Gitlab::ReferenceExtractor.new(self.project, current_user)
+    ext = Gitlab::ReferenceExtractor.new(self.project, current_user, self.author)
 
     if text
       ext.analyze(text)
diff --git a/lib/banzai/filter/redactor_filter.rb b/lib/banzai/filter/redactor_filter.rb
index 89e7a79789a..f01a32b5ae5 100644
--- a/lib/banzai/filter/redactor_filter.rb
+++ b/lib/banzai/filter/redactor_filter.rb
@@ -11,7 +11,7 @@ module Banzai
     class RedactorFilter < HTML::Pipeline::Filter
       def call
         doc.css('a.gfm').each do |node|
-          unless user_can_reference?(node)
+          unless user_can_see_reference?(node)
             # The reference should be replaced by the original text,
             # which is not always the same as the rendered text.
             text = node.attr('data-original') || node.text
@@ -24,12 +24,12 @@ module Banzai
 
       private
 
-      def user_can_reference?(node)
+      def user_can_see_reference?(node)
         if node.has_attribute?('data-reference-filter')
           reference_type = node.attr('data-reference-filter')
           reference_filter = Banzai::Filter.const_get(reference_type)
 
-          reference_filter.user_can_reference?(current_user, node, context)
+          reference_filter.user_can_see_reference?(current_user, node, context)
         else
           true
         end
diff --git a/lib/banzai/filter/reference_filter.rb b/lib/banzai/filter/reference_filter.rb
index a22a7a7afd3..8ca05ace88c 100644
--- a/lib/banzai/filter/reference_filter.rb
+++ b/lib/banzai/filter/reference_filter.rb
@@ -12,7 +12,7 @@ module Banzai
     #   :project (required) - Current project, ignored if reference is cross-project.
     #   :only_path          - Generate path-only links.
     class ReferenceFilter < HTML::Pipeline::Filter
-      def self.user_can_reference?(user, node, context)
+      def self.user_can_see_reference?(user, node, context)
         if node.has_attribute?('data-project')
           project_id = node.attr('data-project').to_i
           return true if project_id == context[:project].try(:id)
@@ -24,6 +24,10 @@ module Banzai
         end
       end
 
+      def self.user_can_reference?(user, node, context)
+        true
+      end
+
       def self.referenced_by(node)
         raise NotImplementedError, "#{self} does not implement #{__method__}"
       end
diff --git a/lib/banzai/filter/reference_gatherer_filter.rb b/lib/banzai/filter/reference_gatherer_filter.rb
index 855f238ac1e..12412ff7ea9 100644
--- a/lib/banzai/filter/reference_gatherer_filter.rb
+++ b/lib/banzai/filter/reference_gatherer_filter.rb
@@ -35,7 +35,9 @@ module Banzai
 
         return if context[:reference_filter] && reference_filter != context[:reference_filter]
 
-        return unless reference_filter.user_can_reference?(current_user, node, context)
+        return if author && !reference_filter.user_can_reference?(author, node, context)
+
+        return unless reference_filter.user_can_see_reference?(current_user, node, context)
 
         references = reference_filter.referenced_by(node)
         return unless references
@@ -57,6 +59,10 @@ module Banzai
       def current_user
         context[:current_user]
       end
+
+      def author
+        context[:author]
+      end
     end
   end
 end
diff --git a/lib/banzai/filter/user_reference_filter.rb b/lib/banzai/filter/user_reference_filter.rb
index 7f302d51dd7..964ab60f614 100644
--- a/lib/banzai/filter/user_reference_filter.rb
+++ b/lib/banzai/filter/user_reference_filter.rb
@@ -39,7 +39,7 @@ module Banzai
         end
       end
 
-      def self.user_can_reference?(user, node, context)
+      def self.user_can_see_reference?(user, node, context)
         if node.has_attribute?('data-group')
           group = Group.find(node.attr('data-group')) rescue nil
           Ability.abilities.allowed?(user, :read_group, group)
@@ -48,6 +48,18 @@ module Banzai
         end
       end
 
+      def self.user_can_reference?(user, node, context)
+        # Only team members can reference `@all`
+        if node.has_attribute?('data-project')
+          project = Project.find(node.attr('data-project')) rescue nil
+          return false unless project
+
+          user && project.team.member?(user)
+        else
+          super
+        end
+      end
+
       def call
         replace_text_nodes_matching(User.reference_pattern) do |content|
           user_link_filter(content)
diff --git a/lib/gitlab/reference_extractor.rb b/lib/gitlab/reference_extractor.rb
index 0a70d21b1ce..be795649e59 100644
--- a/lib/gitlab/reference_extractor.rb
+++ b/lib/gitlab/reference_extractor.rb
@@ -3,11 +3,12 @@ require 'banzai'
 module Gitlab
   # Extract possible GFM references from an arbitrary String for further processing.
   class ReferenceExtractor < Banzai::ReferenceExtractor
-    attr_accessor :project, :current_user
+    attr_accessor :project, :current_user, :author
 
-    def initialize(project, current_user = nil)
+    def initialize(project, current_user = nil, author = nil)
       @project = project
       @current_user = current_user
+      @author = author
 
       @references = {}
 
@@ -20,18 +21,22 @@ module Gitlab
 
     %i(user label merge_request snippet commit commit_range).each do |type|
       define_method("#{type}s") do
-        @references[type] ||= references(type, project: project, current_user: current_user)
+        @references[type] ||= references(type, reference_context)
       end
     end
 
     def issues
-      options = { project: project, current_user: current_user }
-
       if project && project.jira_tracker?
-        @references[:external_issue] ||= references(:external_issue, options)
+        @references[:external_issue] ||= references(:external_issue, reference_context)
       else
-        @references[:issue] ||= references(:issue, options)
+        @references[:issue] ||= references(:issue, reference_context)
       end
     end
+
+    private
+
+    def reference_context
+      { project: project, current_user: current_user, author: author }
+    end
   end
 end
diff --git a/spec/lib/banzai/filter/user_reference_filter_spec.rb b/spec/lib/banzai/filter/user_reference_filter_spec.rb
index 3534bf97784..8bdebae1841 100644
--- a/spec/lib/banzai/filter/user_reference_filter_spec.rb
+++ b/spec/lib/banzai/filter/user_reference_filter_spec.rb
@@ -37,9 +37,22 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do
         .to eq urls.namespace_project_url(project.namespace, project)
     end
 
-    it 'adds to the results hash' do
-      result = reference_pipeline_result("Hey #{reference}")
-      expect(result[:references][:user]).to eq [project.creator]
+    context "when the author is a member of the project" do
+
+      it 'adds to the results hash' do
+        result = reference_pipeline_result("Hey #{reference}", author: project.creator)
+        expect(result[:references][:user]).to eq [project.creator]
+      end
+    end
+
+    context "when the author is not a member of the project" do
+
+      let(:other_user) { create(:user) }
+
+      it "doesn't add to the results hash" do
+        result = reference_pipeline_result("Hey #{reference}", author: other_user)
+        expect(result[:references][:user]).to eq []
+      end
     end
   end
 
diff --git a/spec/models/concerns/mentionable_spec.rb b/spec/models/concerns/mentionable_spec.rb
index 6653621a83e..20f0c561e44 100644
--- a/spec/models/concerns/mentionable_spec.rb
+++ b/spec/models/concerns/mentionable_spec.rb
@@ -3,6 +3,10 @@ require 'spec_helper'
 describe Mentionable do
   include Mentionable
 
+  def author
+    nil
+  end
+
   describe :references do
     let(:project) { create(:project) }
 
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index d7a898e85ff..c103752198d 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -115,6 +115,7 @@ describe NotificationService, services: true do
 
       before do
         build_team(note.project)
+        note.project.team << [note.author, :master]
         ActionMailer::Base.deliveries.clear
       end
 
@@ -126,6 +127,8 @@ describe NotificationService, services: true do
           note.project.team.members.each do |member|
             # User with disabled notification should not be notified
             next if member.id == @u_disabled.id
+            # Author should not be notified
+            next if member.id == note.author.id
             should_email(member)
           end