Commit 5c68ab72 authored by Luke Duncalfe's avatar Luke Duncalfe Committed by Stan Hu

Serve resized Design Management design files

Previously, Design Management would only serve the full size designs
that were uploaded, even when viewed as thumbnails.

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/22860
introduced a change in the app to begin resizing designs down to a new
"v432x230" version.

This change refactors the existing Projects::DesignsController into a
new base class DesignManagement::DesignsController. The old controller
that served the full-size original design files is now
DesignManagement::Designs::RawImageController, and there is an
additional DesignManagement::Designs::ResizedImageController that can
serve resized versions of the designs.

This change represents a breaking change to the routes - however these
routes were only ever used internally. The extenal-facing parts of the
application (the GraphQL `DesignType`) has been updated with the new
routes.

https://gitlab.com/gitlab-org/gitlab/issues/12577
https://gitlab.com/gitlab-org/gitlab/issues/13815
parent 2e08c928
......@@ -26,6 +26,7 @@ class ApplicationController < ActionController::Base
before_action :ldap_security_check
around_action :sentry_context
before_action :default_headers
before_action :default_cache_headers
before_action :add_gon_variables, if: :html_request?
before_action :configure_permitted_parameters, if: :devise_controller?
before_action :require_email, unless: :devise_controller?
......@@ -259,7 +260,9 @@ class ApplicationController < ActionController::Base
headers['X-XSS-Protection'] = '1; mode=block'
headers['X-UA-Compatible'] = 'IE=edge'
headers['X-Content-Type-Options'] = 'nosniff'
end
def default_cache_headers
if current_user
headers['Cache-Control'] = default_cache_control
headers['Pragma'] = 'no-cache' # HTTP 1.0 compatibility
......
......@@ -9,6 +9,7 @@ module UploadsActions
included do
prepend_before_action :set_request_format_from_path_extension
skip_before_action :default_cache_headers, only: :show
rescue_from FileUploader::InvalidSecret, with: :render_404
end
......@@ -35,10 +36,6 @@ module UploadsActions
def show
return render_404 unless uploader&.exists?
# We need to reset caching from the applications controller to get rid of the no-store value
headers['Cache-Control'] = ''
headers['Pragma'] = ''
ttl, directives = *cache_settings
ttl ||= 0
directives ||= { private: true, must_revalidate: true }
......
# frozen_string_literal: true
# Returns full-size design images
module Projects
module DesignManagement
module Designs
class RawImagesController < Projects::DesignManagement::DesignsController
include SendsBlob
skip_before_action :default_cache_headers, only: :show
def show
blob = design_repository.blob_at(ref, design.full_path)
send_blob(design_repository, blob, { inline: false })
end
private
def design_repository
@design_repository ||= project.design_repository
end
def ref
sha || design_repository.root_ref
end
end
end
end
end
# frozen_string_literal: true
# Returns smaller sized design images
module Projects
module DesignManagement
module Designs
class ResizedImageController < Projects::DesignManagement::DesignsController
include SendFileUpload
before_action :validate_size!
skip_before_action :default_cache_headers, only: :show
def show
relation = design.actions
relation = relation.up_to_version(sha) if sha
action = relation.most_recent.first
return render_404 unless action
# This controller returns a 404 if the the `size` param
# is not one of our specific sizes, so using `send` here is safe.
uploader = action.public_send(:"image_#{size}") # rubocop:disable GitlabSecurity/PublicSend
return render_404 unless uploader.file # The image has not been processed
if stale?(etag: action.cache_key)
workhorse_set_content_type!
send_upload(uploader, attachment: design.filename)
end
end
private
def validate_size!
render_404 unless ::DesignManagement::DESIGN_IMAGE_SIZES.include?(size)
end
def size
params[:id]
end
end
end
end
end
# frozen_string_literal: true
class Projects::DesignsController < Projects::ApplicationController
include SendsBlob
class Projects::DesignManagement::DesignsController < Projects::ApplicationController
before_action :authorize_read_design!
def show
blob = design_repository.blob_at(ref, design.full_path)
send_blob(design_repository, blob, { inline: false })
end
private
def ref
@ref ||= params[:ref] || design_repository.root_ref
def authorize_read_design!
unless can?(current_user, :read_design, design)
access_denied!
end
def design
@design ||= project.designs.find(params[:id])
end
def design_repository
@design_repository ||= @project.design_repository
def design
@design ||= project.designs.find(params[:design_id])
end
def authorize_read_design!
unless can?(current_user, :read_design, design)
access_denied!
end
def sha
params[:sha].presence
end
end
......@@ -36,7 +36,7 @@ module Types
def image(parent:)
sha = cached_stateful_version(parent).sha
::Gitlab::Routing.url_helpers.project_design_url(design.project, design, sha)
Gitlab::UrlBuilder.build(design, ref: sha)
end
def event(parent:)
......
# frozen_string_literal: true
module DesignManagement
DESIGN_IMAGE_SIZES = %w(v432x230).freeze
def self.designs_directory
'designs'
end
......
......@@ -30,8 +30,12 @@ module DesignManagement
all
when DesignManagement::Version
where(arel_table[:version_id].lteq(version.id))
when ::Gitlab::Git::COMMIT_ID
versions = DesignManagement::Version.arel_table
subquery = versions.project(versions[:id]).where(versions[:sha].eq(version))
where(arel_table[:version_id].lteq(subquery))
else
raise "Expected a DesignManagement::Version, got #{version}"
raise ArgumentError, "Expected a DesignManagement::Version, got #{version}"
end
end
end
......
......@@ -48,9 +48,24 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
end
end
resources :designs, only: [], constraints: { id: /\d+/ } do
member do
get '(*ref)', action: 'show', as: '', constraints: { ref: Gitlab::PathRegex.git_reference_regex }
# DEPRECATED: Remove this redirection in GitLab 13.0.
# This redirection supports old (pre-12.9) routes to Design Management raw images.
# https://gitlab.com/gitlab-org/gitlab/issues/208256
get '/designs/:id(/*ref)',
as: :design,
contraints: { id: /\d+/, ref: Gitlab::PathRegex.git_reference_regex },
to: redirect { |params|
namespace_id, project_id, id, ref = params.values_at(:namespace_id, :project_id, :id, :ref)
# The :ref route segment is optional in both this route, and the route
# we redirect to (where it is called :sha).
ref_path = "/#{ref}" if ref
"#{namespace_id}/#{project_id}/-/design_management/designs/#{id}#{ref_path}/raw_image"
}
namespace :design_management do
namespace :designs, path: 'designs/:design_id(/:sha)', constraints: -> (params) { params[:sha].nil? || Gitlab::Git.commit_id?(params[:sha]) } do
resource :raw_image, only: :show
resources :resized_image, only: :show, constraints: -> (params) { DesignManagement::DESIGN_IMAGE_SIZES.include?(params[:id]) }
end
end
......
......@@ -8,8 +8,8 @@ module EE
override :url
def url
case object
when DesignManagement::Design
project_design_url(object.project, object)
when ::DesignManagement::Design
design_url
when Epic
group_epic_url(object.group, object)
else
......@@ -24,6 +24,19 @@ module EE
epic = object.noteable
group_epic_url(epic.group, epic, anchor: dom_id(object))
end
private
def design_url
size, ref = opts.values_at(:size, :ref)
design = object
if size
project_design_management_designs_resized_image_url(design.project, design, ref, size)
else
project_design_management_designs_raw_image_url(design.project, design, ref)
end
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Projects::DesignManagement::Designs::RawImagesController do
include DesignManagementTestHelpers
let_it_be(:project) { create(:project, :private) }
let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:viewer) { issue.author }
let(:design_id) { design.id }
let(:sha) { design.versions.first.sha }
let(:filename) { design.filename }
before do
enable_design_management
end
describe 'GET #show' do
subject do
get(:show,
params: {
namespace_id: project.namespace,
project_id: project,
design_id: design_id,
sha: sha
})
end
before do
sign_in(viewer)
end
context 'when the design is not an LFS file' do
let_it_be(:design) { create(:design, :with_file, issue: issue, versions_count: 2) }
# For security, .svg images should only ever be served with Content-Disposition: attachment.
# If this specs ever fails we must assess whether we should be serving svg images.
# See https://gitlab.com/gitlab-org/gitlab/issues/12771
it 'serves files with `Content-Disposition: attachment`' do
subject
expect(response.header['Content-Disposition']).to eq('attachment')
expect(response).to have_gitlab_http_status(:ok)
end
it 'serves files with Workhorse' do
subject
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
expect(response).to have_gitlab_http_status(:ok)
end
context 'when the user does not have permission' do
let_it_be(:viewer) { create(:user) }
specify do
subject
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when design does not exist' do
let(:design_id) { 'foo' }
specify do
subject
expect(response).to have_gitlab_http_status(:not_found)
end
end
describe 'sha param' do
let(:newest_version) { design.versions.ordered.first }
let(:oldest_version) { design.versions.ordered.last }
shared_examples 'a successful request for sha' do
it do
expect_next_instance_of(DesignManagement::Repository) do |repository|
expect(repository).to receive(:blob_at).with(expected_ref, design.full_path).and_call_original
end
subject
expect(response).to have_gitlab_http_status(:ok)
end
end
specify { expect(newest_version.sha).not_to eq(oldest_version.sha) }
context 'when sha is the newest version sha' do
let(:sha) { newest_version.sha }
let(:expected_ref) { sha }
it_behaves_like 'a successful request for sha'
end
context 'when sha is the oldest version sha' do
let(:sha) { oldest_version.sha }
let(:expected_ref) { sha }
it_behaves_like 'a successful request for sha'
end
context 'when sha is nil' do
let(:sha) { nil }
let(:expected_ref) { 'master' }
it_behaves_like 'a successful request for sha'
end
end
end
context 'when the design is an LFS file' do
let_it_be(:design) { create(:design, :with_lfs_file, issue: issue) }
# For security, .svg images should only ever be served with Content-Disposition: attachment.
# If this specs ever fails we must assess whether we should be serving svg images.
# See https://gitlab.com/gitlab-org/gitlab/issues/12771
it 'serves files with `Content-Disposition: attachment`' do
subject
expect(response.header['Content-Disposition']).to eq(%Q(attachment; filename=\"#{filename}\"; filename*=UTF-8''#{filename}))
end
it 'sets appropriate caching headers' do
subject
expect(response.header['ETag']).to be_present
expect(response.header['Cache-Control']).to eq("max-age=60, private")
end
end
# Pass `skip_lfs_disabled_tests: true` to this shared example to disable
# the test scenarios for when LFS is disabled globally.
#
# When LFS is disabled then the design management feature also becomes disabled.
# When the feature is disabled, the `authorize :read_design` check within the
# controller will never authorize the user. Therefore #show will return a 403 and
# we cannot test the data that it serves.
it_behaves_like 'a controller that can serve LFS files', skip_lfs_disabled_tests: true do
let(:file) { fixture_file_upload('spec/fixtures/dk.png', '`/png') }
let(:lfs_pointer) { Gitlab::Git::LfsPointerFile.new(file.read) }
let(:design) { create(:design, :with_lfs_file, file: lfs_pointer.pointer, issue: issue) }
let(:lfs_oid) { project.design_repository.blob_at('HEAD', design.full_path).lfs_oid }
let(:filepath) { design.full_path }
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Projects::DesignManagement::Designs::ResizedImageController do
include DesignManagementTestHelpers
let_it_be(:project) { create(:project, :private) }
let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:viewer) { issue.author }
let_it_be(:size) { :v432x230 }
let(:design) { create(:design, :with_smaller_image_versions, issue: issue, versions_count: 2) }
let(:design_id) { design.id }
let(:sha) { design.versions.first.sha }
before do
enable_design_management
end
describe 'GET #show' do
subject do
get(:show,
params: {
namespace_id: project.namespace,
project_id: project,
design_id: design_id,
sha: sha,
id: size
})
end
before do
sign_in(viewer)
subject
end
context 'when the user does not have permission' do
let_it_be(:viewer) { create(:user) }
specify do
expect(response).to have_gitlab_http_status(:not_found)
end
end
describe 'Response headers' do
it 'completes the request successfully' do
expect(response).to have_gitlab_http_status(:ok)
end
it 'sets Content-Disposition as attachment' do
filename = design.filename
expect(response.header['Content-Disposition']).to eq(%Q(attachment; filename=\"#{filename}\"; filename*=UTF-8''#{filename}))
end
it 'serves files with Workhorse' do
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq 'true'
end
it 'sets appropriate caching headers' do
expect(response.header['Cache-Control']).to eq('private')
expect(response.header['ETag']).to be_present
end
end
context 'when design does not exist' do
let(:design_id) { 'foo' }
specify do
subject
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'when size is invalid' do
let_it_be(:size) { :foo }
it 'returns a 404' do
expect(response).to have_gitlab_http_status(:not_found)
end
end
describe 'sha param' do
let(:newest_version) { design.versions.ordered.first }
let(:oldest_version) { design.versions.ordered.last }
# The design images generated by Factorybot are identical, so
# refer to the `ETag` header, which is uniquely generated from the Action
# (the record that represents the design at a specific version), to
# verify that the correct file is being returned.
def etag(action)
ActionDispatch::TestResponse.new.send(:generate_weak_etag, [action.cache_key, ''])
end
specify { expect(newest_version.sha).not_to eq(oldest_version.sha) }
context 'when sha is the newest version sha' do
let(:sha) { newest_version.sha }
it 'serves the newest image' do
action = newest_version.actions.first
expect(response.header['ETag']).to eq(etag(action))
expect(response).to have_gitlab_http_status(:ok)
end
end
context 'when sha is the oldest version sha' do
let(:sha) { oldest_version.sha }
it 'serves the oldest image' do
action = oldest_version.actions.first
expect(response.header['ETag']).to eq(etag(action))
expect(response).to have_gitlab_http_status(:ok)
end
end
context 'when sha is nil' do
let(:sha) { nil }
it 'serves the newest image' do
action = newest_version.actions.first
expect(response.header['ETag']).to eq(etag(action))
expect(response).to have_gitlab_http_status(:ok)
end
end
context 'when sha is not a valid version sha' do
let(:sha) { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' }
it 'returns a 404' do
expect(response).to have_gitlab_http_status(:not_found)
end
end
end
context 'when design does not have a smaller image size available' do
let(:design) { create(:design, :with_file, issue: issue) }
it 'returns a 404' do
expect(response).to have_gitlab_http_status(:not_found)
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Projects::DesignsController do
include DesignManagementTestHelpers
let_it_be(:project) { create(:project, :public) }
let_it_be(:issue) { create(:issue, project: project) }
let(:file) { fixture_file_upload('spec/fixtures/dk.png', '`/png') }
let(:lfs_pointer) { Gitlab::Git::LfsPointerFile.new(file.read) }
let(:design) { create(:design, :with_lfs_file, file: lfs_pointer.pointer, issue: issue) }
let(:filename) { design.filename }
before do
enable_design_management
end
describe 'GET #show' do
subject do
get(:show,
params: {
namespace_id: project.namespace,
project_id: project,
id: design.id,
ref: 'HEAD'
})
end
# For security, .svg images should only ever be served with Content-Disposition: attachment.
# If these specs ever fail we must assess whether we should be serving svg images.
# See https://gitlab.com/gitlab-org/gitlab/issues/12771
describe 'Response headers' do
it 'serves LFS files with `Content-Disposition: attachment`' do
lfs_object = create(:lfs_object, file: file, oid: lfs_pointer.sha256, size: lfs_pointer.size)
create(:lfs_objects_project, project: project, lfs_object: lfs_object, repository_type: :design)
subject
expect(response.header['Content-Disposition']).to eq(%Q(attachment; filename=\"#{filename}\"; filename*=UTF-8''#{filename}))
end
context 'when the design is not an LFS file' do
let(:design) { create(:design, :with_file, issue: issue) }
it 'serves files with `Content-Disposition: attachment`' do
subject
expect(response.header['Content-Disposition']).to eq('attachment')
end
it 'serves files with Workhorse' do
subject
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
end
end
end
# Pass `skip_lfs_disabled_tests: true` to this shared example to disable
# the test scenarios for when LFS is disabled globally.
#
# When LFS is disabled then the design management feature also becomes disabled.
# When the feature is disabled, the `authorize :read_design` check within the
# controller will never authorize the user. Therefore #show will return a 403 and
# we cannot test the data that it serves.
it_behaves_like 'a controller that can serve LFS files', skip_lfs_disabled_tests: true do
let(:lfs_oid) { project.design_repository.blob_at('HEAD', design.full_path).lfs_oid }
let(:filepath) { design.full_path }
end
end
end
......@@ -116,5 +116,13 @@ FactoryBot.define do
create_versions[design, evaluator, commit_version]
end
end
trait :with_smaller_image_versions do
with_lfs_file
after :create do |design|
design.versions.each { |v| DesignManagement::GenerateImageVersionsService.new(v).execute }
end
end
end
end
......@@ -5,12 +5,20 @@ require 'spec_helper'
describe Gitlab::UrlBuilder do
describe '.build' do
context 'when passing a DesignManagement::Design' do
it 'returns a proper URL' do
it 'returns a proper URL to the raw (unresized) image' do
design = build_stubbed(:design)
url = described_class.build(design, ref: 'master')
expect(url).to eq "#{Settings.gitlab['url']}/#{design.project.full_path}/-/design_management/designs/#{design.id}/master/raw_image"
end
it 'returns a proper URL to the resized image' do
design = build_stubbed(:design)
url = described_class.build(design)
url = described_class.build(design, ref: 'master', size: 'small')
expect(url).to eq "#{Settings.gitlab['url']}/#{design.project.full_path}/-/designs/#{design.id}"
expect(url).to eq "#{Settings.gitlab['url']}/#{design.project.full_path}/-/design_management/designs/#{design.id}/master/resized_image/small"
end
end
......
......@@ -55,6 +55,7 @@ describe DesignManagement::Action do
end
end
context 'when given a Version instance' do
context 'the version is the most current' do
let(:version) { newest }
......@@ -73,5 +74,32 @@ describe DesignManagement::Action do
it { is_expected.to have_attributes(size: 4) }
end
end
context 'when given a commit SHA' do
context 'the version is the most current' do
let(:version) { newest.sha }
it { is_expected.to have_attributes(size: 6) }
end
context 'the version is the oldest' do
let(:version) { oldest.sha }
it { is_expected.to have_attributes(size: 2) }
end
context 'the version is the middle one' do
let(:version) { middle.sha }
it { is_expected.to have_attributes(size: 4) }
end
end
context 'when given a String that is not a commit SHA' do
let(:version) { 'foo' }
it { expect { subject }.to raise_error(ArgumentError) }
end
end
end
end
......@@ -180,7 +180,7 @@ describe 'Getting designs related to an issue' do
end
def image_url(design, sha = nil)
Gitlab::Routing.url_helpers.project_design_url(design.project, design, sha)
Gitlab::UrlBuilder.build(design, ref: sha)
end
def global_id(object)
......
......@@ -35,11 +35,28 @@ describe 'EE-specific project routing' do
end
end
describe Projects::DesignsController, 'routing' do
describe Projects::DesignManagement::Designs::RawImagesController, 'routing' do
it 'to #show' do
expect(get('/gitlab/gitlabhq/-/designs/1/master')).to route_to('projects/designs#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1', ref: 'master')
expect(get('/gitlab/gitlabhq/-/designs/1/my/branch')).to route_to('projects/designs#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1', ref: 'my/branch')
expect(get('/gitlab/gitlabhq/-/designs/1/f166f5c7afaed9e1236e4e5965585f235795db4c3f45e8a9f6ea9dde098c')).to route_to('projects/designs#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1', ref: 'f166f5c7afaed9e1236e4e5965585f235795db4c3f45e8a9f6ea9dde098c')
expect(get('/gitlab/gitlabhq/-/design_management/designs/1/raw_image')).to route_to('projects/design_management/designs/raw_images#show', namespace_id: 'gitlab', project_id: 'gitlabhq', design_id: '1')
expect(get('/gitlab/gitlabhq/-/design_management/designs/1/c6f00aa50b80887ada30a6fe517670be9f8f9ece/raw_image')).to route_to('projects/design_management/designs/raw_images#show', namespace_id: 'gitlab', project_id: 'gitlabhq', design_id: '1', sha: 'c6f00aa50b80887ada30a6fe517670be9f8f9ece')
end
end
describe Projects::DesignManagement::Designs::ResizedImageController, 'routing' do
it 'to #show' do
expect(get('/gitlab/gitlabhq/-/design_management/designs/1/resized_image/v432x230')).to route_to('projects/design_management/designs/resized_image#show', namespace_id: 'gitlab', project_id: 'gitlabhq', design_id: '1', id: 'v432x230')
expect(get('/gitlab/gitlabhq/-/design_management/designs/1/c6f00aa50b80887ada30a6fe517670be9f8f9ece/resized_image/v432x230')).to route_to('projects/design_management/designs/resized_image#show', namespace_id: 'gitlab', project_id: 'gitlabhq', design_id: '1', sha: 'c6f00aa50b80887ada30a6fe517670be9f8f9ece', id: 'v432x230')
expect(get('/gitlab/gitlabhq/-/design_management/designs/1/invalid/resized_image/v432x230')).to route_to('application#route_not_found', unmatched_route: 'gitlab/gitlabhq/-/design_management/designs/1/invalid/resized_image/v432x230')
expect(get('/gitlab/gitlabhq/-/design_management/designs/1/c6f00aa50b80887ada30a6fe517670be9f8f9ece/resized_image/small')).to route_to('application#route_not_found', unmatched_route: 'gitlab/gitlabhq/-/design_management/designs/1/c6f00aa50b80887ada30a6fe517670be9f8f9ece/resized_image/small')
end
end
describe 'Deprecated Design management legacy paths' do
include RSpec::Rails::RequestExampleGroup
it 'redirects to canonical path' do
expect(get('/gitlab/gitlabhq/-/designs/1/c6f00aa50b80887ada30a6fe517670be9f8f9ece')).to redirect_to('/gitlab/gitlabhq/-/design_management/designs/1/c6f00aa50b80887ada30a6fe517670be9f8f9ece/raw_image')
expect(get('/gitlab/gitlabhq/-/designs/1')).to redirect_to('/gitlab/gitlabhq/-/design_management/designs/1/raw_image')
end
end
......
......@@ -42,7 +42,7 @@ RSpec.shared_examples 'a GraphQL type with design fields' do
it 'resolves to the design image URL' do
image = field.resolve(instance, args, context)
sha = design.versions.first.sha
url = ::Gitlab::Routing.url_helpers.project_design_url(design.project, design, sha)
url = ::Gitlab::Routing.url_helpers.project_design_management_designs_raw_image_url(design.project, design, sha)
expect(image).to eq(url)
end
......
......@@ -725,6 +725,7 @@ describe ApplicationController do
get :index
expect(response.headers['Cache-Control']).to be_nil
expect(response.headers['Pragma']).to be_nil
end
end
......@@ -735,6 +736,7 @@ describe ApplicationController do
get :index
expect(response.headers['Cache-Control']).to eq 'max-age=0, private, must-revalidate, no-store'
expect(response.headers['Pragma']).to eq 'no-cache'
end
it 'does not set the "no-store" header for XHR requests' do
......
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