Maintenance: Enhance attachment preview capabilities

app/controllers/application_controller.rb

@@ -10,8 +10,8 @@ class ApplicationController < ActionController::Base
   include ApplicationController::RendersModels
   include ApplicationController::HasUser
   include ApplicationController::HasResponseExtentions
+  include ApplicationController::HasDownload
   include ApplicationController::PreventsCsrf
-  include ApplicationController::HasSecureContentSecurityPolicyForDownloads
   include ApplicationController::LogsHttpAccess
   include ApplicationController::Authorizes
 end

app/controllers/application_controller/has_download.rb

@@ -0,0 +1,44 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+module ApplicationController::HasDownload
+  extend ActiveSupport::Concern
+
+  included do
+    around_action do |_controller, block|
+
+      subscriber = proc do
+        policy = ActionDispatch::ContentSecurityPolicy.new
+        policy.default_src :none
+
+        # The 'plugin_types' rule is deprecated and should be changed in the future.
+        policy.plugin_types 'application/pdf'
+
+        request.content_security_policy = policy
+      end
+
+      ActiveSupport::Notifications.subscribed(subscriber, 'send_file.action_controller') do
+        ActiveSupport::Notifications.subscribed(subscriber, 'send_data.action_controller') do
+          block.call
+        end
+      end
+    end
+  end
+
+  private
+
+  def file_id
+    @file_id ||= params[:id]
+  end
+
+  def download_file
+    @download_file ||= ::ApplicationController::HasDownload::DownloadFile.new(file_id, disposition: sanitized_disposition)
+  end
+
+  def sanitized_disposition
+    disposition = params.fetch(:disposition, 'inline')
+    valid_disposition = %w[inline attachment]
+    return disposition if valid_disposition.include?(disposition)
+
+    raise Exceptions::Forbidden, "Invalid disposition #{disposition} requested. Only #{valid_disposition.join(', ')} are valid."
+  end
+end

app/controllers/application_controller/has_download/download_file.rb

@@ -0,0 +1,54 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+class ApplicationController::HasDownload::DownloadFile < SimpleDelegator
+  attr_reader :requested_disposition
+
+  def initialize(id, disposition: 'inline')
+    @requested_disposition = disposition
+
+    super(Store.find(id))
+  end
+
+  def disposition
+    return 'attachment' if forcibly_download_as_binary? || !allowed_inline?
+
+    requested_disposition
+  end
+
+  def content_type
+    return ActiveStorage.binary_content_type if forcibly_download_as_binary?
+
+    file_content_type
+  end
+
+  def content(view_type)
+    return __getobj__.content if view_type.blank? || !preferences[:resizable]
+
+    return content_inline if content_inline? && view_type == 'inline'
+    return content_preview if content_preview? && view_type == 'preview'
+
+    __getobj__.content
+  end
+
+  private
+
+  def allowed_inline?
+    ActiveStorage.content_types_allowed_inline.include?(content_type)
+  end
+
+  def forcibly_download_as_binary?
+    ActiveStorage.content_types_to_serve_as_binary.include?(file_content_type)
+  end
+
+  def file_content_type
+    @file_content_type ||= preferences['Content-Type'] || preferences['Mime-Type'] || ActiveStorage.binary_content_type
+  end
+
+  def content_inline?
+    preferences[:content_inline] == true
+  end
+
+  def content_preview?
+    preferences[:content_preview] == true
+  end
+end

app/controllers/attachments_controller.rb

@@ -6,14 +6,13 @@ class AttachmentsController < ApplicationController
   prepend_before_action :authentication_check_only, only: %i[show destroy]
 
   def show
-    content   = @file.content_preview if params[:preview] && @file.preferences[:content_preview]
-    content ||= @file.content
+    view_type = params[:preview] ? 'preview' : nil
 
     send_data(
-      content,
-      filename:    @file.filename,
-      type:        @file.preferences['Content-Type'] || @file.preferences['Mime-Type'] || 'application/octet-stream',
-      disposition: sanitized_disposition
+      download_file.content(view_type),
+      filename:    download_file.filename,
+      type:        download_file.content_type,
+      disposition: download_file.disposition
     )
   end
 
@@ -52,7 +51,7 @@ def create
   end
 
   def destroy
-    Store.remove_item(@file.id)
+    Store.remove_item(download_file.id)
 
     render json: {
       success: true,
@@ -72,18 +71,8 @@ def destroy_form
 
   private
 
-  def sanitized_disposition
-    disposition = params.fetch(:disposition, 'inline')
-    valid_disposition = %w[inline attachment]
-    return disposition if valid_disposition.include?(disposition)
-
-    raise Exceptions::Forbidden, "Invalid disposition #{disposition} requested. Only #{valid_disposition.join(', ')} are valid."
-  end
-
   def authorize!
-    @file = Store.find(params[:id])
-
-    record = @file&.store_object&.name&.safe_constantize&.find(@file.o_id)
+    record = download_file&.store_object&.name&.safe_constantize&.find(download_file.o_id)
     authorize(record) if record
   rescue Pundit::NotAuthorizedError
     raise ActiveRecord::RecordNotFound

app/controllers/ticket_articles_controller.rb

@@ -175,29 +175,11 @@ def attachment
     end
     raise Exceptions::Forbidden, 'Requested file id is not linked with article_id.' if !access
 
-    # find file
-    file = Store.find(params[:id])
-
-    disposition = sanitized_disposition
-
-    content = nil
-    if params[:view].present? && file.preferences[:resizable] == true
-      if file.preferences[:content_inline] == true && params[:view] == 'inline'
-        content = file.content_inline
-      elsif file.preferences[:content_preview] == true && params[:view] == 'preview'
-        content = file.content_preview
-      end
-    end
-
-    if content.blank?
-      content = file.content
-    end
-
     send_data(
-      content,
-      filename:    file.filename,
-      type:        file.preferences['Content-Type'] || file.preferences['Mime-Type'] || 'application/octet-stream',
-      disposition: disposition
+      download_file.content(params[:view]),
+      filename:    download_file.filename,
+      type:        download_file.content_type,
+      disposition: download_file.disposition
     )
   end
 
@@ -278,14 +260,4 @@ def retry_security_process
 
     render json: result
   end
-
-  private
-
-  def sanitized_disposition
-    disposition = params.fetch(:disposition, 'inline')
-    valid_disposition = %w[inline attachment]
-    return disposition if valid_disposition.include?(disposition)
-
-    raise Exceptions::Forbidden, "Invalid disposition #{disposition} requested. Only #{valid_disposition.join(', ')} are valid."
-  end
 end

spec/controllers/application_controller/has_download/download_file_spec.rb

@@ -0,0 +1,88 @@
+# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
+
+require 'rails_helper'
+
+RSpec.describe ApplicationController::HasDownload::DownloadFile do
+  subject(:download_file) { described_class.new(stored_file.id, disposition: 'inline') }
+
+  let(:file_content_type) { 'application/pdf' }
+  let(:file_data) { 'A example file.' }
+  let(:file_name) { 'example.pdf' }
+
+  let(:stored_file) do
+    Store.add(
+      object:        'Ticket',
+      o_id:          1,
+      data:          file_data,
+      filename:      file_name,
+      preferences:   {
+        'Content-Type' => file_content_type,
+      },
+      created_by_id: 1,
+    )
+  end
+
+  describe '#disposition' do
+    context "with given object dispostion 'inline'" do
+      context 'with allowed inline content type (from ActiveStorage.content_types_allowed_inline)' do
+        it 'disposition is inline' do
+          expect(download_file.disposition).to eq('inline')
+        end
+      end
+
+      context 'with binary content type (ActiveStorage.content_types_to_serve_as_binary)' do
+        let(:file_content_type) { 'image/svg+xml' }
+
+        it 'disposition forced to attachment' do
+          expect(download_file.disposition).to eq('attachment')
+        end
+      end
+    end
+
+    context "with given object dispostion 'attachment'" do
+      subject(:download_file) { described_class.new(stored_file.id, disposition: 'attachment') }
+
+      it 'disposition is attachment' do
+        expect(download_file.disposition).to eq('attachment')
+      end
+    end
+  end
+
+  describe '#content_type' do
+    context 'with none binary content type' do
+      it 'check content type' do
+        expect(download_file.content_type).to eq('application/pdf')
+      end
+    end
+
+    context 'with forced active storage binary content type' do
+      let(:file_content_type) { 'image/svg+xml' }
+
+      it 'check content type' do
+        expect(download_file.content_type).to eq('application/octet-stream')
+      end
+    end
+  end
+
+  describe '#content' do
+    context 'with not resizable file' do
+      it 'check that normal content will be returned' do
+        expect(download_file.content('preview')).to eq('A example file.')
+      end
+    end
+
+    context 'with image content type' do
+      let(:file_content_type) { 'image/jpg' }
+      let(:file_data) { File.binread(Rails.root.join('test/data/upload/upload2.jpg')) }
+      let(:file_name) { 'image.jpg' }
+
+      it 'check that inline content will be returned' do
+        expect(download_file.content('inline')).to not_eq(file_data)
+      end
+
+      it 'check that preview content will be returned' do
+        expect(download_file.content('preview')).to not_eq(file_data)
+      end
+    end
+  end
+end