Commit bcb69815 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'ajk-design-ref-filter' into 'master'

Add reference filter for design URLs

Closes #217160

See merge request gitlab-org/gitlab!32446
parents 4186e343 bafd6674
......@@ -126,68 +126,23 @@ module DesignManagement
# #12["filename with [] in it.jpg"]
def to_reference(from = nil, full: false)
infix = full ? '/designs' : ''
totally_simple = %r{ \A #{self.class.simple_file_name} \z }x
safe_name = if totally_simple.match?(filename)
filename
elsif filename =~ /[<>]/
%Q{base64:#{Base64.strict_encode64(filename)}}
else
escaped = filename.gsub(%r{[\\"]}) { |x| "\\#{x}" }
%Q{"#{escaped}"}
end
safe_name = Sanitize.fragment(filename)
"#{issue.to_reference(from, full: full)}#{infix}[#{safe_name}]"
end
def self.reference_pattern
@reference_pattern ||= begin
# Filenames can be escaped with double quotes to name filenames
# that include square brackets, or other special characters
%r{
#{Issue.reference_pattern}
(\/designs)?
\[
(?<design> #{simple_file_name} | #{quoted_file_name} | #{base_64_encoded_name})
\]
}x
end
end
def self.simple_file_name
%r{
(?<simple_file_name>
( \w | [_:,'-] | \. | \s )+
\.
\w+
)
}x
end
def self.base_64_encoded_name
%r{
base64:
(?<base_64_encoded_name>
[A-Za-z0-9+\n]+
=?
)
}x
end
def self.quoted_file_name
%r{
"
(?<escaped_filename>
(\\ \\ | \\ " | [^"\\])+
)
"
}x
# no-op: We only support link_reference_pattern parsing
end
def self.link_reference_pattern
@link_reference_pattern ||= begin
exts = SAFE_IMAGE_EXT + DANGEROUS_IMAGE_EXT
path_segment = %r{issues/#{Gitlab::Regex.issue}/designs}
filename_pattern = %r{(?<simple_file_name>[a-z0-9_=-]+\.(#{exts.join('|')}))}i
ext = Regexp.new(Regexp.union(SAFE_IMAGE_EXT + DANGEROUS_IMAGE_EXT).source, Regexp::IGNORECASE)
valid_char = %r{[^/\s]} # any char that is not a forward slash or whitespace
filename_pattern = %r{
(?<url_filename> #{valid_char}+ \. #{ext})
}x
super(path_segment, filename_pattern)
end
......
---
title: Enable GitLab-Flavored Markdown processing for design links
merge_request: 32446
author:
type: added
......@@ -442,7 +442,7 @@ In addition to this, links to some objects are also recognized and formatted. So
- Comments on issues: `"https://gitlab.com/gitlab-org/gitlab/-/issues/1234#note_101075757"`, which will be rendered as `#1234 (note1)`
- The issues designs tab: `"https://gitlab.com/gitlab-org/gitlab/-/issues/1234/designs"`, which will be rendered as `#1234 (designs)`.
**(PREMIUM)**
- Links to individual designs: `"https://gitlab.com/gitlab-org/gitlab/-/issues/1234/designs/layout.png"`, which will be rendered as `#1234[layout.png]`.
### Task lists
......
......@@ -176,3 +176,44 @@ Different discussions have different pin numbers:
From GitLab 12.5 on, new discussions will be outputted to the issue activity,
so that everyone involved can participate in the discussion.
## Referring to designs in Markdown
> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/217160) in **GitLab 13.1**.
> - It is deployed behind a feature flag, disabled by default.
> - It is disabled on GitLab.com.
> - It is not recommended for production use.
> - To use it in GitLab self-managed instances, ask a GitLab administrator to [enable it](#enable-or-disable-design-references-core-only). **(CORE ONLY)**
We support referring to designs in [Markdown](../../markdown.md), which is available
throughout the application, including in merge request and issue descriptions, in discussions and comments, and in wiki pages.
At present, full URL references are supported. For example, if we refer to a design
somewhere with:
```markdown
See http://gitlab.com/your-group/your-project/-/issues/123/designs/homescreen.png
```
This will be rendered as:
> See [#123[homescreen.png]](http://gitlab.com/your-group/your-project/-/issues/123/designs/homescreen.png)
### Enable or disable design references **(CORE ONLY)**
Design reference parsing is under development and not ready for production use. It is
deployed behind a feature flag that is **disabled by default**.
[GitLab administrators with access to the GitLab Rails console](../../../administration/feature_flags.md)
can enable it for your instance.
To enable it:
```ruby
Feature.enable(:design_management_reference_filter_gfm_pipeline)
```
To disable it:
```ruby
Feature.disable(:design_management_reference_filter_gfm_pipeline)
```
# frozen_string_literal: true
module Banzai
module Filter
class DesignReferenceFilter < AbstractReferenceFilter
FEATURE_FLAG = :design_management_reference_filter_gfm_pipeline
class Identifier
include Comparable
attr_reader :issue_iid, :filename
def initialize(issue_iid:, filename:)
@issue_iid = issue_iid
@filename = filename
end
def as_composite_id(id_for_iid)
id = id_for_iid[issue_iid]
return unless id
{ issue_id: id, filename: filename }
end
def <=>(other)
return unless other.is_a?(Identifier)
[issue_iid, filename] <=> [other.issue_iid, other.filename]
end
alias_method :eql?, :==
def hash
[issue_iid, filename].hash
end
end
self.reference_type = :design
# This filter must be enabled by setting the
# design_management_reference_filter_gfm_pipeline flag
def call
return doc unless enabled?
super
end
def find_object(project, identifier)
records_per_parent[project][identifier]
end
def parent_records(project, identifiers)
return [] unless project.design_management_enabled?
iids = identifiers.map(&:issue_iid).to_set
issues = project.issues.where(iid: iids)
id_for_iid = issues.index_by(&:iid).transform_values(&:id)
issue_by_id = issues.index_by(&:id)
designs(identifiers, id_for_iid).each do |d|
issue = issue_by_id[d.issue_id]
# optimisation: assign values we have already fetched
d.project = project
d.issue = issue
end
end
def relation_for_paths(paths)
super.includes(:route, :namespace, :group)
end
def parent_type
:project
end
# optimisation to reuse the parent_per_reference query information
def parent_from_ref(ref)
parent_per_reference[ref || current_parent_path]
end
def url_for_object(design, project)
path_options = { vueroute: design.filename }
Gitlab::Routing.url_helpers.designs_project_issue_path(project, design.issue, path_options)
end
def data_attributes_for(_text, _project, design, **_kwargs)
super.merge(issue: design.issue_id)
end
def self.object_class
::DesignManagement::Design
end
def self.object_sym
:design
end
def self.parse_symbol(raw, match_data)
filename = match_data[:url_filename]
iid = match_data[:issue].to_i
Identifier.new(filename: CGI.unescape(filename), issue_iid: iid)
end
def record_identifier(design)
Identifier.new(filename: design.filename, issue_iid: design.issue.iid)
end
private
def designs(identifiers, id_for_iid)
identifiers
.map { |identifier| identifier.as_composite_id(id_for_iid) }
.compact
.in_groups_of(100, false) # limitation of by_issue_id_and_filename, so we batch
.flat_map { |ids| DesignManagement::Design.by_issue_id_and_filename(ids) }
end
def enabled?
Feature.enabled?(FEATURE_FLAG, parent)
end
end
end
end
......@@ -44,7 +44,7 @@ module Banzai
end
def read_designs?(issue)
Ability.allowed?(current_user, :read_design, issue)
issue.project.design_management_enabled?
end
end
end
......
......@@ -56,6 +56,7 @@ module Banzai
[
Filter::UserReferenceFilter,
Filter::ProjectReferenceFilter,
Filter::DesignReferenceFilter,
Filter::IssueReferenceFilter,
Filter::ExternalIssueReferenceFilter,
Filter::MergeRequestReferenceFilter,
......
# frozen_string_literal: true
require 'spec_helper'
describe 'viewing issues with design references' do
include DesignManagementTestHelpers
let_it_be(:public_project) { create(:project_empty_repo, :public) }
let_it_be(:private_project) { create(:project_empty_repo) }
let(:user) { create(:user) }
let(:design_issue) { create(:issue, project: project) }
let(:design_a) { create(:design, :with_file, issue: design_issue) }
let(:design_b) { create(:design, :with_file, issue: design_issue) }
let(:issue_ref) { design_issue.to_reference(public_project) }
let(:design_ref_a) { design_a.to_reference(public_project) }
let(:design_ref_b) { design_b.to_reference(public_project) }
let(:design_tab_ref) { "#{issue_ref} (designs)" }
let(:description) do
<<~MD
The designs I mentioned:
* #{url_for_designs(design_issue)}
* #{url_for_design(design_a)}
* #{url_for_design(design_b)}
MD
end
def visit_page_with_design_references
public_issue = create(:issue, project: public_project, description: description)
visit project_issue_path(public_issue.project, public_issue)
end
shared_examples 'successful use of design link references' do
before do
enable_design_management
end
it 'shows the issue description and design references', :aggregate_failures do
visit_page_with_design_references
expect(page).to have_text('The designs I mentioned')
expect(page).to have_link(design_tab_ref)
expect(page).to have_link(design_ref_a)
expect(page).to have_link(design_ref_b)
end
end
context 'the user has access to a public project' do
let(:project) { public_project }
it_behaves_like 'successful use of design link references'
end
context 'the user does not have access to a private project' do
let(:project) { private_project }
it 'redacts inaccessible design references', :aggregate_failures do
visit_page_with_design_references
expect(page).to have_text('The designs I mentioned')
expect(page).not_to have_link(issue_ref)
expect(page).not_to have_link(design_tab_ref)
expect(page).not_to have_link(design_ref_a)
expect(page).not_to have_link(design_ref_b)
end
end
context 'the user has access to a private project' do
let(:project) { private_project }
before do
project.add_developer(user)
sign_in(user)
end
it_behaves_like 'successful use of design link references'
context 'design management is entirely disabled' do
it 'processes design links as issue references', :aggregate_failures do
enable_design_management(false)
visit_page_with_design_references
expect(page).to have_text('The designs I mentioned')
expect(page).to have_link(issue_ref)
expect(page).not_to have_link(design_tab_ref)
expect(page).not_to have_link(design_ref_a)
expect(page).not_to have_link(design_ref_b)
end
end
context 'design management is enabled, but the filter is disabled globally' do
before do
enable_design_management
stub_feature_flags(
Banzai::Filter::DesignReferenceFilter::FEATURE_FLAG => false
)
end
it 'processes design tab links successfully, and design references as issue references', :aggregate_failures do
visit_page_with_design_references
expect(page).to have_text('The designs I mentioned')
expect(page).to have_link(design_tab_ref)
expect(page).to have_link(issue_ref)
expect(page).not_to have_link(design_ref_a)
expect(page).not_to have_link(design_ref_b)
end
end
context 'design management is enabled, and the filter is enabled for the current project' do
before do
stub_feature_flags(
Banzai::Filter::DesignReferenceFilter::FEATURE_FLAG => public_project
)
end
it_behaves_like 'successful use of design link references'
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Banzai::Filter::DesignReferenceFilter do
include FilterSpecHelper
include DesignManagementTestHelpers
let_it_be(:issue) { create(:issue, iid: 10) }
let_it_be(:issue_proj_2) { create(:issue, iid: 20) }
let_it_be(:issue_b) { create(:issue, project: issue.project) }
let_it_be(:developer) { create(:user, developer_projects: [issue.project, issue_proj_2.project]) }
let_it_be(:design_a) { create(:design, :with_versions, issue: issue) }
let_it_be(:design_b) { create(:design, :with_versions, issue: issue_b) }
let_it_be(:design_proj_2) { create(:design, :with_versions, issue: issue_proj_2) }
let_it_be(:project_with_no_lfs) { create(:project, :public, lfs_enabled: false) }
let(:design) { design_a }
let(:project) { issue.project }
let(:project_2) { issue_proj_2.project }
let(:reference) { design.to_reference }
let(:design_url) { url_for_design(design) }
let(:input_text) { "Added #{design_url}" }
let(:doc) { process_doc(input_text) }
let(:current_user) { developer }
before do
enable_design_management
end
shared_examples 'a no-op filter' do
it 'does nothing' do
expect(process(input_text)).to eq(baseline(input_text).to_html)
end
end
shared_examples 'a good link reference' do
let(:link) { doc.css('a').first }
let(:href) { url_for_design(design) }
let(:title) { design.filename }
it 'produces a good link', :aggregate_failures do
expect(link.attr('href')).to eq(href)
expect(link.attr('title')).to eq(title)
expect(link.attr('class')).to eq('gfm gfm-design has-tooltip')
expect(link.attr('data-project')).to eq(design.project.id.to_s)
expect(link.attr('data-issue')).to eq(design.issue.id.to_s)
expect(link.attr('data-original')).to eq(href)
expect(link.attr('data-reference-type')).to eq('design')
expect(link.text).to eq(design.to_reference(project))
end
end
describe '.call' do
it 'requires project context' do
expect { described_class.call('') }.to raise_error(ArgumentError, /:project/)
end
end
it 'does not error when we add redaction to the pipeline' do
enable_design_management
res = reference_pipeline(redact: true).to_document(input_text)
expect(res.css('a').first).to be_present
end
describe '#call' do
describe 'feature flags' do
context 'design management is not enabled' do
before do
enable_design_management(false)
end
it_behaves_like 'a no-op filter'
end
context 'design reference filter is not enabled' do
before do
stub_feature_flags(described_class::FEATURE_FLAG => false)
end
it_behaves_like 'a no-op filter'
it 'issues no queries' do
expect { process(input_text) }.not_to exceed_query_limit(0)
end
end
context 'the filter is enabled for the context project' do
before do
stub_feature_flags(described_class::FEATURE_FLAG => project)
end
it_behaves_like 'a good link reference'
end
end
end
%w(pre code a style).each do |elem|
context "wrapped in a <#{elem}/>" do
let(:input_text) { "<#{elem}>Design #{url_for_design(design)}</#{elem}>" }
it_behaves_like 'a no-op filter'
end
end
describe '.identifier' do
where(:filename) do
[
['simple.png'],
['SIMPLE.PNG'],
['has spaces.png'],
['has-hyphen.jpg'],
['snake_case.svg'],
['has "quotes".svg'],
['has <special> characters [o].svg']
]
end
with_them do
let(:design) { build(:design, issue: issue, filename: filename) }
let(:url) { url_for_design(design) }
let(:pattern) { described_class.object_class.link_reference_pattern }
let(:parsed) do
m = pattern.match(url)
described_class.identifier(m) if m
end
it 'can parse the reference' do
expect(parsed).to have_attributes(
filename: filename,
issue_iid: issue.iid
)
end
end
end
describe 'static properties' do
specify do
expect(described_class).to have_attributes(
object_sym: :design,
object_class: ::DesignManagement::Design
)
end
end
describe '#data_attributes_for' do
let(:subject) { filter_instance.data_attributes_for(input_text, project, design) }
specify do
is_expected.to include(issue: design.issue_id,
original: input_text,
project: project.id,
design: design.id)
end
end
context 'a design with a quoted filename' do
let(:filename) { %q{A "very" good file.png} }
let(:design) { create(:design, :with_versions, issue: issue, filename: filename) }
it 'links to the design' do
expect(doc.css('a').first.attr('href'))
.to eq url_for_design(design)
end
end
context 'internal reference' do
it_behaves_like 'a reference containing an element node'
context 'the reference is valid' do
it_behaves_like 'a good link reference'
context 'the filename needs to be escaped' do
where(:filename) do
[
['with some spaces.png'],
['with <script>console.log("pwded")<%2Fscript>.png']
]
end
with_them do
let(:design) { create(:design, :with_versions, filename: filename, issue: issue) }
let(:link) { doc.css('a').first }
it 'replaces the content with the reference, but keeps the link', :aggregate_failures do
expect(doc.text).to eq(CGI.unescapeHTML("Added #{design.to_reference}"))
expect(link.attr('title')).to eq(design.filename)
expect(link.attr('href')).to eq(design_url)
end
end
end
end
context 'the reference is to a non-existant design' do
let(:design_url) { url_for_design(build(:design, issue: issue)) }
it_behaves_like 'a no-op filter'
end
context 'design management is disabled for the referenced project' do
let(:public_issue) { create(:issue, project: project_with_no_lfs) }
let(:design) { create(:design, :with_versions, issue: public_issue) }
it_behaves_like 'a no-op filter'
end
end
describe 'link pattern' do
let(:reference) { url_for_design(design) }
it 'matches' do
expect(reference).to match(DesignManagement::Design.link_reference_pattern)
end
end
context 'cross-project / cross-namespace complete reference' do
let(:design) { design_proj_2 }
it_behaves_like 'a reference containing an element node'
it_behaves_like 'a good link reference'
it 'links to a valid reference' do
expect(doc.css('a').first.attr('href')).to eq(design_url)
end
context 'design management is disabled for that project' do
let(:design) { create(:design, project: project_with_no_lfs) }
it_behaves_like 'a no-op filter'
end
it 'link has valid text' do
ref = "#{design.project.full_path}##{design.issue.iid}[#{design.filename}]"
expect(doc.css('a').first.text).to eql(ref)
end
it 'includes default classes' do
expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-design has-tooltip'
end
context 'the reference is invalid' do
let(:design_url) { url_for_design(design).gsub(/jpg/, 'gif') }
it_behaves_like 'a no-op filter'
end
end
describe 'performance' do
it 'is linear in the number of projects with design management enabled each design refers to' do
design_c = build(:design, :with_versions, issue: issue)
design_d = build(:design, :with_versions, issue: issue_b)
design_e = build(:design, :with_versions, issue: build_stubbed(:issue, project: project_2))
one_ref_per_project = <<~MD
Design #{url_for_design(design_a)}, #{url_for_design(design_proj_2)}
MD
multiple_references = <<~MD
Designs that affect the count:
* #{url_for_design(design_a)}
* #{url_for_design(design_b)}
* #{url_for_design(design_c)}
* #{url_for_design(design_d)}
* #{url_for_design(design_proj_2)}
* #{url_for_design(design_e)}
Things that do not affect the count:
* #{url_for_design(build_stubbed(:design, project: project_with_no_lfs))}
* #{url_for_designs(issue)}
* #1[not a valid reference.gif]
MD
baseline = ActiveRecord::QueryRecorder.new { process(one_ref_per_project) }
# each project mentioned requires 2 queries:
#
# * SELECT "issues".* FROM "issues" WHERE "issues"."project_id" = 1 AND ...
# :in `parent_records'*/
# * SELECT "_designs".* FROM "_designs"
# WHERE (issue_id = ? AND filename = ?) OR ...
# :in `parent_records'*/
#
# In addition there is a 1 query overhead for all the projects at the
# start. Currently, the baseline for 2 projects is `2 * 2 + 1 = 5` queries
#
expect { process(multiple_references) }.not_to exceed_query_limit(baseline.count)
end
end
private
def process_doc(text)
reference_filter(text, project: project)
end
def baseline(text)
null_filter(text, project: project)
end
def process(text)
process_doc(text).to_html
end
end
......@@ -374,6 +374,16 @@ describe Banzai::Filter::IssueReferenceFilter do
expect(link.attr('href')).to eq(designs_tab_url)
expect(link.text).to eq("#{issue.to_reference} (designs)")
end
context 'design management is not available' do
before do
enable_design_management(false)
end
it 'links to the issue, but not to the designs tab' do
expect(link.text).to eq(issue.to_reference)
end
end
end
context 'group context' do
......
......@@ -460,38 +460,6 @@ describe DesignManagement::Design do
it 'uses the simple format' do
expect(reference).to eq "#1[homescreen.jpg]"
end
context 'when the filename contains spaces, hyphens, periods, single-quotes, underscores and colons' do
let(:filename) { %q{a complex filename: containing - _ : etc., but still 'simple'.gif} }
it 'uses the simple format' do
expect(reference).to eq "#1[#{filename}]"
end
end
context 'when the filename contains HTML angle brackets' do
let(:filename) { 'a <em>great</em> filename.jpg' }
it 'uses Base64 encoding' do
expect(reference).to eq "#1[base64:#{Base64.strict_encode64(filename)}]"
end
end
context 'when the filename contains quotation marks' do
let(:filename) { %q{a "great" filename.jpg} }
it 'uses enclosing quotes, with backslash encoding' do
expect(reference).to eq %q{#1["a \"great\" filename.jpg"]}
end
end
context 'when the filename contains square brackets' do
let(:filename) { %q{a [great] filename.jpg} }
it 'uses enclosing quotes' do
expect(reference).to eq %q{#1["a [great] filename.jpg"]}
end
end
end
context 'when full is true' do
......@@ -525,31 +493,55 @@ describe DesignManagement::Design do
end
describe 'reference_pattern' do
let(:match) { described_class.reference_pattern.match(ref) }
let(:ref) { design.to_reference }
let(:design) { build(:design, filename: filename) }
it 'is nil' do
expect(described_class.reference_pattern).to be_nil
end
end
context 'simple_file_name' do
let(:filename) { 'simple-file-name.jpg' }
describe 'link_reference_pattern' do
it 'is not nil' do
expect(described_class.link_reference_pattern).not_to be_nil
end
it 'does not match the designs tab' do
expect(described_class.link_reference_pattern).not_to match(url_for_designs(issue))
end
it 'matches :simple_file_name' do
expect(match[:simple_file_name]).to eq(filename)
where(:ext) do
(described_class::SAFE_IMAGE_EXT + described_class::DANGEROUS_IMAGE_EXT).flat_map do |ext|
[[ext], [ext.upcase]]
end
end
context 'quoted_file_name' do
let(:filename) { 'simple "file" name.jpg' }
with_them do
let(:filename) { "my-file.#{ext}" }
let(:design) { build(:design, filename: filename) }
let(:url) { url_for_design(design) }
let(:captures) { described_class.link_reference_pattern.match(url)&.named_captures }
it 'matches :simple_file_name' do
expect(match[:escaped_filename].gsub(/\\"/, '"')).to eq(filename)
it 'matches the URL' do
expect(captures).to include(
'url_filename' => filename,
'issue' => design.issue.iid.to_s,
'namespace' => design.project.namespace.to_param,
'project' => design.project.name
)
end
end
context 'Base64 name' do
let(:filename) { '<>.png' }
context 'the file needs to be encoded' do
let(:filename) { "my file.#{ext}" }
it 'matches base_64_encoded_name' do
expect(Base64.decode64(match[:base_64_encoded_name])).to eq(filename)
it 'extracts the encoded filename' do
expect(captures).to include('url_filename' => 'my%20file.' + ext)
end
end
context 'the file is all upper case' do
let(:filename) { "file.#{ext}".upcase }
it 'extracts the encoded filename' do
expect(captures).to include('url_filename' => filename)
end
end
end
end
......
# frozen_string_literal: true
module DesignManagementTestHelpers
def enable_design_management(enabled = true, ref_filter = true)
def enable_design_management(enabled = true)
stub_lfs_setting(enabled: enabled)
stub_feature_flags(design_management_reference_filter_gfm_pipeline: ref_filter)
end
def delete_designs(*designs)
......
......@@ -56,14 +56,11 @@ module FilterSpecHelper
pipeline.call(body)
end
def reference_pipeline(context = {})
def reference_pipeline(filter: described_class, **context)
context.reverse_merge!(project: project) if defined?(project)
context.reverse_merge!(current_user: current_user) if defined?(current_user)
filters = [
Banzai::Filter::AutolinkFilter,
described_class
]
filters = [Banzai::Filter::AutolinkFilter, filter].compact
redact = context.delete(:redact)
filters.push(Banzai::Filter::ReferenceRedactorFilter) if redact
......@@ -75,8 +72,13 @@ module FilterSpecHelper
reference_pipeline(context).call(body)
end
def reference_filter(html, context = {})
reference_pipeline(context).to_document(html)
def reference_filter(text, context = {})
reference_pipeline(**context).to_document(text)
end
# Use to test no-ops
def null_filter(text, context = {})
reference_pipeline(filter: nil, **context).to_document(text)
end
# Modify a String reference to make it invalid
......
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