Merge branch 'refactor-rendering-redacting' into 'master'

app/controllers/projects/issues_controller.rb

@@ -62,8 +62,12 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def show
+    raw_notes = @issue.notes_with_associations.fresh
+
+    @notes = Banzai::NoteRenderer.
+      render(raw_notes, @project, current_user, @path, @project_wiki, @ref)
+
     @note     = @project.notes.new(noteable: @issue)
-    @notes    = @issue.notes.with_associations.fresh
     @noteable = @issue
 
     respond_to do |format|

app/controllers/projects/merge_requests_controller.rb

@@ -85,6 +85,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     @grouped_diff_notes = @merge_request.notes.grouped_diff_notes
 
+    Banzai::NoteRenderer.render(
+      @grouped_diff_notes.values.flatten,
+      @project,
+      current_user,
+      @path,
+      @project_wiki,
+      @ref
+    )
+
     respond_to do |format|
       format.html
       format.json { render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } }
@@ -325,8 +334,21 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def define_show_vars
     # Build a note object for comment form
     @note = @project.notes.new(noteable: @merge_request)
-    @discussions = @merge_request.mr_and_commit_notes.inc_author_project_award_emoji.fresh.discussions
-    @notes = @discussions.flatten
+
+    @discussions = @merge_request.mr_and_commit_notes.
+      inc_author_project_award_emoji.
+      fresh.
+      discussions
+
+    @notes = Banzai::NoteRenderer.render(
+      @discussions.flatten,
+      @project,
+      current_user,
+      @path,
+      @project_wiki,
+      @ref
+    )
+
     @noteable = @merge_request
 
     # Get commits from repository

app/controllers/projects/notes_controller.rb

@@ -24,6 +24,10 @@ class Projects::NotesController < Projects::ApplicationController
   def create
     @note = Notes::CreateService.new(project, current_user, note_params).execute
 
+    if @note.is_a?(Note)
+      Banzai::NoteRenderer.render([@note], @project, current_user)
+    end
+
     respond_to do |format|
       format.json { render json: note_json(@note) }
       format.html { redirect_back_or_default }
@@ -33,6 +37,10 @@ class Projects::NotesController < Projects::ApplicationController
   def update
     @note = Notes::UpdateService.new(project, current_user, note_params).execute(note)
 
+    if @note.is_a?(Note)
+      Banzai::NoteRenderer.render([@note], @project, current_user)
+    end
+
     respond_to do |format|
       format.json { render json: note_json(@note) }
       format.html { redirect_back_or_default }
@@ -118,6 +126,8 @@ class Projects::NotesController < Projects::ApplicationController
         name:   note.name
       }
     elsif note.valid?
+      Banzai::NoteRenderer.render([note], @project, current_user)
+
       {
         valid: true,
         id: note.id,

app/models/note.rb

@@ -6,6 +6,10 @@ class Note < ActiveRecord::Base
   include Awardable
   include Importable
 
+  # Attribute containing rendered and redacted Markdown as generated by
+  # Banzai::ObjectRenderer.
+  attr_accessor :note_html
+
   default_value_for :system, false
 
   attr_mentionable :note, pipeline: :note

app/views/projects/notes/_note.html.haml

@@ -32,7 +32,7 @@
       .note-body{class: note_editable ? 'js-task-list-container' : ''}
         .note-text
           = preserve do
-            = markdown(note.note, pipeline: :note, cache_key: [note, "note"], author: note.author)
+            = note.note_html
           = edited_time_ago_with_tooltip(note, placement: 'bottom', html_class: 'note_edited_ago', include_author: true)
         - if note_editable
           = render 'projects/notes/edit_form', note: note

config/initializers/metrics.rb

@@ -113,6 +113,10 @@ if Gitlab::Metrics.enabled?
     config.instrument_methods(Banzai::Renderer)
     config.instrument_methods(Banzai::Querying)
 
+    config.instrument_instance_methods(Banzai::ObjectRenderer)
+    config.instrument_instance_methods(Banzai::Redactor)
+    config.instrument_methods(Banzai::NoteRenderer)
+
     [Issuable, Mentionable, Participable].each do |klass|
       config.instrument_instance_methods(klass)
       config.instrument_instance_methods(klass::ClassMethods)

lib/banzai/filter/redactor_filter.rb

@@ -7,40 +7,13 @@ module Banzai
     #
     class RedactorFilter < HTML::Pipeline::Filter
       def call
-        nodes = Querying.css(doc, 'a.gfm[data-reference-type]')
-        visible = nodes_visible_to_user(nodes)
-
-        nodes.each do |node|
-          unless visible.include?(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
-            node.replace(text)
-          end
-        end
+        Redactor.new(project, current_user).redact([doc])
 
         doc
       end
 
       private
 
-      def nodes_visible_to_user(nodes)
-        per_type = Hash.new { |h, k| h[k] = [] }
-        visible = Set.new
-
-        nodes.each do |node|
-          per_type[node.attr('data-reference-type')] << node
-        end
-
-        per_type.each do |type, nodes|
-          parser = Banzai::ReferenceParser[type].new(project, current_user)
-
-          visible.merge(parser.nodes_visible_to_user(current_user, nodes))
-        end
-
-        visible
-      end
-
       def current_user
         context[:current_user]
       end

lib/banzai/note_renderer.rb

+module Banzai
+  module NoteRenderer
+    # Renders a collection of Note instances.
+    #
+    # notes - The notes to render.
+    # project - The project to use for rendering/redacting.
+    # user - The user viewing the notes.
+    # path - The request path.
+    # wiki - The project's wiki.
+    # git_ref - The current Git reference.
+    def self.render(notes, project, user = nil, path = nil, wiki = nil, git_ref = nil)
+      renderer = ObjectRenderer.new(project,
+                                    user,
+                                    requested_path: path,
+                                    project_wiki: wiki,
+                                    ref: git_ref,
+                                    pipeline: :note)
+
+      renderer.render(notes, :note)
+    end
+  end
+end

lib/banzai/object_renderer.rb

+module Banzai
+  # Class for rendering multiple objects (e.g. Note instances) in a single pass.
+  #
+  # Rendered Markdown is stored in an attribute in every object based on the
+  # name of the attribute containing the Markdown. For example, when the
+  # attribute `note` is rendered the HTML is stored in `note_html`.
+  class ObjectRenderer
+    attr_reader :project, :user
+
+    # Make sure to set the appropriate pipeline in the `raw_context` attribute
+    # (e.g. `:note` for Note instances).
+    #
+    # project - A Project to use for rendering and redacting Markdown.
+    # user - The user viewing the Markdown/HTML documents, if any.
+    # context - A Hash containing extra attributes to use in the rendering
+    #           pipeline.
+    def initialize(project, user = nil, raw_context = {})
+      @project = project
+      @user = user
+      @raw_context = raw_context
+    end
+
+    # Renders and redacts an Array of objects.
+    #
+    # objects - The objects to render
+    # attribute - The attribute containing the raw Markdown to render.
+    #
+    # Returns the same input objects.
+    def render(objects, attribute)
+      documents = render_objects(objects, attribute)
+      redacted = redact_documents(documents)
+
+      objects.each_with_index do |object, index|
+        object.__send__("#{attribute}_html=", redacted.fetch(index))
+      end
+
+      objects
+    end
+
+    # Renders the attribute of every given object.
+    def render_objects(objects, attribute)
+      objects.map do |object|
+        render_attribute(object, attribute)
+      end
+    end
+
+    # Redacts the list of documents.
+    #
+    # Returns an Array containing the redacted documents.
+    def redact_documents(documents)
+      redactor = Redactor.new(project, user)
+
+      redactor.redact(documents).map do |document|
+        document.to_html.html_safe
+      end
+    end
+
+    # Returns a Banzai context for the given object and attribute.
+    def context_for(object, attribute)
+      context = base_context.merge(cache_key: [object, attribute])
+
+      if object.respond_to?(:author)
+        context[:author] = object.author
+      end
+
+      context
+    end
+
+    # Renders the attribute of an object.
+    #
+    # Returns a `Nokogiri::HTML::Document`.
+    def render_attribute(object, attribute)
+      context = context_for(object, attribute)
+
+      string = object.__send__(attribute)
+      html = Banzai.render(string, context)
+
+      Banzai::Pipeline[:relative_link].to_document(html, context)
+    end
+
+    def base_context
+      @base_context ||= @raw_context.merge(current_user: user, project: project)
+    end
+  end
+end

lib/banzai/pipeline/relative_link_pipeline.rb

+module Banzai
+  module Pipeline
+    class RelativeLinkPipeline < BasePipeline
+      def self.filters
+        FilterArray[
+          Filter::RelativeLinkFilter
+        ]
+      end
+    end
+  end
+end

lib/banzai/redactor.rb

+module Banzai
+  # Class for removing Markdown references a certain user is not allowed to
+  # view.
+  class Redactor
+    attr_reader :user, :project
+
+    # project - A Project to use for redacting links.
+    # user - The currently logged in user (if any).
+    def initialize(project, user = nil)
+      @project = project
+      @user = user
+    end
+
+    # Redacts the references in the given Array of documents.
+    #
+    # This method modifies the given documents in-place.
+    #
+    # documents - A list of HTML documents containing references to redact.
+    #
+    # Returns the documents passed as the first argument.
+    def redact(documents)
+      nodes = documents.flat_map do |document|
+        Querying.css(document, 'a.gfm[data-reference-type]')
+      end
+
+      redact_nodes(nodes)
+
+      documents
+    end
+
+    # Redacts the given nodes
+    #
+    # nodes - An Array of HTML nodes to redact.
+    def redact_nodes(nodes)
+      visible = nodes_visible_to_user(nodes)
+
+      nodes.each do |node|
+        unless visible.include?(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
+          node.replace(text)
+        end
+      end
+    end
+
+    # Returns the nodes visible to the current user.
+    #
+    # nodes - The input nodes to check.
+    #
+    # Returns a new Array containing the visible nodes.
+    def nodes_visible_to_user(nodes)
+      per_type = Hash.new { |h, k| h[k] = [] }
+      visible = Set.new
+
+      nodes.each do |node|
+        per_type[node.attr('data-reference-type')] << node
+      end
+
+      per_type.each do |type, nodes|
+        parser = Banzai::ReferenceParser[type].new(project, user)
+
+        visible.merge(parser.nodes_visible_to_user(user, nodes))
+      end
+
+      visible
+    end
+  end
+end

spec/lib/banzai/note_renderer_spec.rb

+require 'spec_helper'
+
+describe Banzai::NoteRenderer do
+  describe '.render' do
+    it 'renders a Note' do
+      note = double(:note)
+      project = double(:project)
+      wiki = double(:wiki)
+      user = double(:user)
+
+      expect(Banzai::ObjectRenderer).to receive(:new).
+        with(project, user,
+             requested_path: 'foo',
+             project_wiki: wiki,
+             ref: 'bar',
+             pipeline: :note).
+        and_call_original
+
+      expect_any_instance_of(Banzai::ObjectRenderer).
+        to receive(:render).with([note], :note)
+
+      described_class.render([note], project, user, 'foo', wiki, 'bar')
+    end
+  end
+end

spec/lib/banzai/object_renderer_spec.rb

+require 'spec_helper'
+
+describe Banzai::ObjectRenderer do
+  let(:project) { create(:empty_project) }
+  let(:user) { project.owner }
+
+  describe '#render' do
+    it 'renders and redacts an Array of objects' do
+      renderer = described_class.new(project, user)
+      object = double(:object, note: 'hello', note_html: nil)
+
+      expect(renderer).to receive(:render_objects).with([object], :note).
+        and_call_original
+
+      expect(renderer).to receive(:redact_documents).
+        with(an_instance_of(Array)).
+        and_call_original
+
+      expect(object).to receive(:note_html=).with('<p>hello</p>')
+
+      renderer.render([object], :note)
+    end
+  end
+
+  describe '#render_objects' do
+    it 'renders an Array of objects' do
+      object = double(:object, note: 'hello')
+      renderer = described_class.new(project, user)
+
+      expect(renderer).to receive(:render_attribute).with(object, :note).
+        and_call_original
+
+      rendered = renderer.render_objects([object], :note)
+
+      expect(rendered).to be_an_instance_of(Array)
+      expect(rendered[0]).to be_an_instance_of(Nokogiri::HTML::DocumentFragment)
+    end
+  end
+
+  describe '#redact_documents' do
+    it 'redacts a set of documents and returns them as an Array of Strings' do
+      doc = Nokogiri::HTML.fragment('<p>hello</p>')
+      renderer = described_class.new(project, user)
+
+      expect_any_instance_of(Banzai::Redactor).to receive(:redact).
+        with([doc]).
+        and_call_original
+
+      redacted = renderer.redact_documents([doc])
+
+      expect(redacted).to eq(['<p>hello</p>'])
+    end
+  end
+
+  describe '#context_for' do
+    let(:object) { double(:object, note: 'hello') }
+    let(:renderer) { described_class.new(project, user) }
+
+    it 'returns a Hash' do
+      expect(renderer.context_for(object, :note)).to be_an_instance_of(Hash)
+    end
+
+    it 'includes the cache key' do
+      context = renderer.context_for(object, :note)
+
+      expect(context[:cache_key]).to eq([object, :note])
+    end
+
+    context 'when the object responds to "author"' do
+      it 'includes the author in the context' do
+        expect(object).to receive(:author).and_return('Alice')
+
+        context = renderer.context_for(object, :note)
+
+        expect(context[:author]).to eq('Alice')
+      end
+    end
+
+    context 'when the object does not respond to "author"' do
+      it 'does not include the author in the context' do
+        context = renderer.context_for(object, :note)
+
+        expect(context.key?(:author)).to eq(false)
+      end
+    end
+  end
+
+  describe '#render_attribute' do
+    it 'renders the attribute of an object' do
+      object = double(:doc, note: 'hello')
+      renderer = described_class.new(project, user, pipeline: :note)
+      doc = renderer.render_attribute(object, :note)
+
+      expect(doc).to be_an_instance_of(Nokogiri::HTML::DocumentFragment)
+      expect(doc.to_html).to eq('<p>hello</p>')
+    end
+  end
+
+  describe '#base_context' do
+    let(:context) do
+      described_class.new(project, user, pipeline: :note).base_context
+    end
+
+    it 'returns a Hash' do
+      expect(context).to be_an_instance_of(Hash)
+    end
+
+    it 'includes the custom attributes' do
+      expect(context[:pipeline]).to eq(:note)
+    end
+
+    it 'includes the current user' do
+      expect(context[:current_user]).to eq(user)
+    end
+
+    it 'includes the current project' do
+      expect(context[:project]).to eq(project)
+    end
+  end
+end

spec/lib/banzai/redactor_spec.rb

+require 'spec_helper'
+
+describe Banzai::Redactor do
+  let(:user) { build(:user) }
+  let(:project) { build(:empty_project) }
+  let(:redactor) { described_class.new(project, user) }
+
+  describe '#redact' do
+    it 'redacts an Array of documents' do
+      doc1 = Nokogiri::HTML.
+        fragment('<a class="gfm" data-reference-type="issue">foo</a>')
+
+      doc2 = Nokogiri::HTML.
+        fragment('<a class="gfm" data-reference-type="issue">bar</a>')
+
+      expect(redactor).to receive(:nodes_visible_to_user).and_return([])
+
+      expect(redactor.redact([doc1, doc2])).to eq([doc1, doc2])
+
+      expect(doc1.to_html).to eq('foo')
+      expect(doc2.to_html).to eq('bar')
+    end
+  end
+
+  describe '#redact_nodes' do
+    it 'redacts an Array of nodes' do
+      doc = Nokogiri::HTML.fragment('<a href="foo">foo</a>')
+      node = doc.children[0]
+
+      expect(redactor).to receive(:nodes_visible_to_user).
+        with([node]).
+        and_return(Set.new)
+
+      redactor.redact_nodes([node])
+
+      expect(doc.to_html).to eq('foo')
+    end
+  end
+
+  describe '#nodes_visible_to_user' do
+    it 'returns a Set containing the visible nodes' do
+      doc = Nokogiri::HTML.fragment('<a data-reference-type="issue"></a>')
+      node = doc.children[0]
+
+      expect_any_instance_of(Banzai::ReferenceParser::IssueParser).
+        to receive(:nodes_visible_to_user).
+        with(user, [node]).
+        and_return([node])
+
+      expect(redactor.nodes_visible_to_user([node])).to eq(Set.new([node]))
+    end
+  end
+end