Commit a97a2d27 authored by Robert Speicher's avatar Robert Speicher

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
parents a5274664 9a0e16f4
...@@ -5,6 +5,7 @@ v 8.4.0 (unreleased) ...@@ -5,6 +5,7 @@ v 8.4.0 (unreleased)
- Implement search inside emoji picker - Implement search inside emoji picker
- Add API support for looking up a user by username (Stan Hu) - Add API support for looking up a user by username (Stan Hu)
- Add project permissions to all project API endpoints (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 - Expose Git's version in the admin area
- Add "Frequently used" category to emoji picker - Add "Frequently used" category to emoji picker
- Add CAS support (tduehr) - Add CAS support (tduehr)
......
...@@ -178,7 +178,7 @@ class ProjectsController < ApplicationController ...@@ -178,7 +178,7 @@ class ProjectsController < ApplicationController
def markdown_preview def markdown_preview
text = params[:text] text = params[:text]
ext = Gitlab::ReferenceExtractor.new(@project, current_user) ext = Gitlab::ReferenceExtractor.new(@project, current_user, current_user)
ext.analyze(text) ext.analyze(text)
render json: { render json: {
......
...@@ -44,7 +44,7 @@ module Mentionable ...@@ -44,7 +44,7 @@ module Mentionable
end end
def all_references(current_user = self.author, text = nil) 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 if text
ext.analyze(text) ext.analyze(text)
......
...@@ -11,7 +11,7 @@ module Banzai ...@@ -11,7 +11,7 @@ module Banzai
class RedactorFilter < HTML::Pipeline::Filter class RedactorFilter < HTML::Pipeline::Filter
def call def call
doc.css('a.gfm').each do |node| 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, # The reference should be replaced by the original text,
# which is not always the same as the rendered text. # which is not always the same as the rendered text.
text = node.attr('data-original') || node.text text = node.attr('data-original') || node.text
...@@ -24,12 +24,12 @@ module Banzai ...@@ -24,12 +24,12 @@ module Banzai
private private
def user_can_reference?(node) def user_can_see_reference?(node)
if node.has_attribute?('data-reference-filter') if node.has_attribute?('data-reference-filter')
reference_type = node.attr('data-reference-filter') reference_type = node.attr('data-reference-filter')
reference_filter = Banzai::Filter.const_get(reference_type) 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 else
true true
end end
......
...@@ -12,7 +12,7 @@ module Banzai ...@@ -12,7 +12,7 @@ module Banzai
# :project (required) - Current project, ignored if reference is cross-project. # :project (required) - Current project, ignored if reference is cross-project.
# :only_path - Generate path-only links. # :only_path - Generate path-only links.
class ReferenceFilter < HTML::Pipeline::Filter 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') if node.has_attribute?('data-project')
project_id = node.attr('data-project').to_i project_id = node.attr('data-project').to_i
return true if project_id == context[:project].try(:id) return true if project_id == context[:project].try(:id)
...@@ -24,6 +24,10 @@ module Banzai ...@@ -24,6 +24,10 @@ module Banzai
end end
end end
def self.user_can_reference?(user, node, context)
true
end
def self.referenced_by(node) def self.referenced_by(node)
raise NotImplementedError, "#{self} does not implement #{__method__}" raise NotImplementedError, "#{self} does not implement #{__method__}"
end end
......
...@@ -35,7 +35,9 @@ module Banzai ...@@ -35,7 +35,9 @@ module Banzai
return if context[:reference_filter] && reference_filter != context[:reference_filter] 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) references = reference_filter.referenced_by(node)
return unless references return unless references
...@@ -57,6 +59,10 @@ module Banzai ...@@ -57,6 +59,10 @@ module Banzai
def current_user def current_user
context[:current_user] context[:current_user]
end end
def author
context[:author]
end
end end
end end
end end
...@@ -39,7 +39,7 @@ module Banzai ...@@ -39,7 +39,7 @@ module Banzai
end end
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') if node.has_attribute?('data-group')
group = Group.find(node.attr('data-group')) rescue nil group = Group.find(node.attr('data-group')) rescue nil
Ability.abilities.allowed?(user, :read_group, group) Ability.abilities.allowed?(user, :read_group, group)
...@@ -48,6 +48,18 @@ module Banzai ...@@ -48,6 +48,18 @@ module Banzai
end end
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 def call
replace_text_nodes_matching(User.reference_pattern) do |content| replace_text_nodes_matching(User.reference_pattern) do |content|
user_link_filter(content) user_link_filter(content)
......
...@@ -3,11 +3,12 @@ require 'banzai' ...@@ -3,11 +3,12 @@ require 'banzai'
module Gitlab module Gitlab
# Extract possible GFM references from an arbitrary String for further processing. # Extract possible GFM references from an arbitrary String for further processing.
class ReferenceExtractor < Banzai::ReferenceExtractor 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 @project = project
@current_user = current_user @current_user = current_user
@author = author
@references = {} @references = {}
...@@ -20,18 +21,22 @@ module Gitlab ...@@ -20,18 +21,22 @@ module Gitlab
%i(user label merge_request snippet commit commit_range).each do |type| %i(user label merge_request snippet commit commit_range).each do |type|
define_method("#{type}s") do define_method("#{type}s") do
@references[type] ||= references(type, project: project, current_user: current_user) @references[type] ||= references(type, reference_context)
end end
end end
def issues def issues
options = { project: project, current_user: current_user }
if project && project.jira_tracker? if project && project.jira_tracker?
@references[:external_issue] ||= references(:external_issue, options) @references[:external_issue] ||= references(:external_issue, reference_context)
else else
@references[:issue] ||= references(:issue, options) @references[:issue] ||= references(:issue, reference_context)
end end
end end
private
def reference_context
{ project: project, current_user: current_user, author: author }
end
end end
end end
...@@ -37,9 +37,22 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do ...@@ -37,9 +37,22 @@ describe Banzai::Filter::UserReferenceFilter, lib: true do
.to eq urls.namespace_project_url(project.namespace, project) .to eq urls.namespace_project_url(project.namespace, project)
end end
it 'adds to the results hash' do context "when the author is a member of the project" do
result = reference_pipeline_result("Hey #{reference}")
expect(result[:references][:user]).to eq [project.creator] 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
end end
......
...@@ -3,6 +3,10 @@ require 'spec_helper' ...@@ -3,6 +3,10 @@ require 'spec_helper'
describe Mentionable do describe Mentionable do
include Mentionable include Mentionable
def author
nil
end
describe :references do describe :references do
let(:project) { create(:project) } let(:project) { create(:project) }
......
...@@ -115,6 +115,7 @@ describe NotificationService, services: true do ...@@ -115,6 +115,7 @@ describe NotificationService, services: true do
before do before do
build_team(note.project) build_team(note.project)
note.project.team << [note.author, :master]
ActionMailer::Base.deliveries.clear ActionMailer::Base.deliveries.clear
end end
...@@ -126,6 +127,8 @@ describe NotificationService, services: true do ...@@ -126,6 +127,8 @@ describe NotificationService, services: true do
note.project.team.members.each do |member| note.project.team.members.each do |member|
# User with disabled notification should not be notified # User with disabled notification should not be notified
next if member.id == @u_disabled.id next if member.id == @u_disabled.id
# Author should not be notified
next if member.id == note.author.id
should_email(member) should_email(member)
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