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/application_controller/has_secure_content_security_policy_for_downloads.rb

@@ -1,25 +0,0 @@
-# Copyright (C) 2012-2021 Zammad Foundation, http://zammad-foundation.org/
-
-module ApplicationController::HasSecureContentSecurityPolicyForDownloads
-  extend ActiveSupport::Concern
-
-  included do
-
-    around_action do |_controller, block|
-
-      subscriber = proc do
-        policy = ActionDispatch::ContentSecurityPolicy.new
-        policy.default_src :none
-        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
-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]
+    view_type = params[:preview] ? 'preview' : nil
-    content ||= @file.content
 
     send_data(
-      content,
+      download_file.content(view_type),
-      filename:    @file.filename,
+      filename:    download_file.filename,
-      type:        @file.preferences['Content-Type'] || @file.preferences['Mime-Type'] || 'application/octet-stream',
+      type:        download_file.content_type,
-      disposition: sanitized_disposition
+      disposition: download_file.disposition
     )
   end
 
@@ -52,7 +51,7 @@ class AttachmentsController < ApplicationController
   end
 
   def destroy
-    Store.remove_item(@file.id)
+    Store.remove_item(download_file.id)
 
     render json: {
       success: true,
@@ -72,18 +71,8 @@ class AttachmentsController < ApplicationController
 
   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 = download_file&.store_object&.name&.safe_constantize&.find(download_file.o_id)
-
-    record = @file&.store_object&.name&.safe_constantize&.find(@file.o_id)
     authorize(record) if record
   rescue Pundit::NotAuthorizedError
     raise ActiveRecord::RecordNotFound

app/controllers/ticket_articles_controller.rb

@@ -175,29 +175,11 @@ class TicketArticlesController < ApplicationController
     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,
+      download_file.content(params[:view]),
-      filename:    file.filename,
+      filename:    download_file.filename,
-      type:        file.preferences['Content-Type'] || file.preferences['Mime-Type'] || 'application/octet-stream',
+      type:        download_file.content_type,
-      disposition: disposition
+      disposition: download_file.disposition
     )
   end
 
@@ -278,14 +260,4 @@ class TicketArticlesController < ApplicationController
 
     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

spec/requests/ticket/article_attachments_spec.rb

@@ -2,7 +2,7 @@
 
 require 'rails_helper'
 
-RSpec.describe 'Ticket Article Attachments', type: :request do
+RSpec.describe 'Ticket Article Attachments', type: :request, authenticated_as: -> { agent } do
 
   let(:group) { create(:group) }
 
@@ -11,68 +11,110 @@ RSpec.describe 'Ticket Article Attachments', type: :request do
   end
 
   describe 'request handling' do
+    context 'with attachment urls' do
+      let(:ticket1) { create(:ticket, group: group) }
+      let(:article1) { create(:ticket_article, ticket: ticket1) }
+      let(:ticket2) { create(:ticket, group: group) }
+      let(:article2) { create(:ticket_article, ticket: ticket2) }
 
-    it 'does test attachment urls' do
+      let(:store_file_content_type) { 'text/plain' }
-      ticket1  = create(:ticket, group: group)
+      let!(:store_file) do
-      article1 = create(:ticket_article, ticket_id: ticket1.id)
+        Store.add(
-
-      store1 = Store.add(
           object:        'Ticket::Article',
           o_id:          article1.id,
           data:          'some content',
           filename:      'some_file.txt',
           preferences:   {
-          'Content-Type' => 'text/plain',
+            'Content-Type' => store_file_content_type,
           },
           created_by_id: 1,
         )
-      article2 = create(:ticket_article, ticket_id: ticket1.id)
+      end
 
-      authenticated_as(agent)
+      context 'with one article attachment' do
-      get "/api/v1/ticket_attachment/#{ticket1.id}/#{article1.id}/#{store1.id}", params: {}
+        it 'does test different attachment urls' do
+          get "/api/v1/ticket_attachment/#{ticket1.id}/#{article1.id}/#{store_file.id}", params: {}
           expect(response).to have_http_status(:ok)
-      expect('some content').to eq(@response.body)
+          expect('some content').to eq(response.body)
 
-      authenticated_as(agent)
+          get "/api/v1/ticket_attachment/#{ticket1.id}/#{article2.id}/#{store_file.id}", params: {}
-      get "/api/v1/ticket_attachment/#{ticket1.id}/#{article2.id}/#{store1.id}", params: {}
           expect(response).to have_http_status(:forbidden)
-      expect(@response.body).to match(%r{403: Forbidden})
+          expect(response.body).to match(%r{403: Forbidden})
+        end
+      end
 
-      ticket2 = create(:ticket, group: group)
+      context 'with attachment from merged ticket' do
+        before do
           ticket1.merge_to(
             ticket_id: ticket2.id,
             user_id:   1,
           )
-
-      authenticated_as(agent)
-      get "/api/v1/ticket_attachment/#{ticket2.id}/#{article1.id}/#{store1.id}", params: {}
-      expect(response).to have_http_status(:ok)
-      expect('some content').to eq(@response.body)
-
-      authenticated_as(agent)
-      get "/api/v1/ticket_attachment/#{ticket2.id}/#{article2.id}/#{store1.id}", params: {}
-      expect(response).to have_http_status(:forbidden)
-      expect(@response.body).to match(%r{403: Forbidden})
-
-      # allow access via merged ticket id also
-      authenticated_as(agent)
-      get "/api/v1/ticket_attachment/#{ticket1.id}/#{article1.id}/#{store1.id}", params: {}
-      expect(response).to have_http_status(:ok)
-      expect('some content').to eq(@response.body)
-
-      authenticated_as(agent)
-      get "/api/v1/ticket_attachment/#{ticket1.id}/#{article2.id}/#{store1.id}", params: {}
-      expect(response).to have_http_status(:forbidden)
-      expect(@response.body).to match(%r{403: Forbidden})
-
         end
 
+        it 'does test attachment url after ticket merge' do
+          get "/api/v1/ticket_attachment/#{ticket2.id}/#{article1.id}/#{store_file.id}", params: {}
+          expect(response).to have_http_status(:ok)
+          expect('some content').to eq(response.body)
+
+          get "/api/v1/ticket_attachment/#{ticket2.id}/#{article2.id}/#{store_file.id}", params: {}
+          expect(response).to have_http_status(:forbidden)
+          expect(response.body).to match(%r{403: Forbidden})
+
+          # allow access via merged ticket id also
+          get "/api/v1/ticket_attachment/#{ticket1.id}/#{article1.id}/#{store_file.id}", params: {}
+          expect(response).to have_http_status(:ok)
+          expect('some content').to eq(response.body)
+
+          get "/api/v1/ticket_attachment/#{ticket1.id}/#{article2.id}/#{store_file.id}", params: {}
+          expect(response).to have_http_status(:forbidden)
+          expect(response.body).to match(%r{403: Forbidden})
+        end
+      end
+
+      context 'with different file content types' do
+        context 'without allowed inline file content type' do
+          it 'disposition can not be inline' do
+            get "/api/v1/ticket_attachment/#{ticket1.id}/#{article1.id}/#{store_file.id}?disposition=inline", params: {}
+            expect(response.headers['Content-Disposition']).to include('attachment')
+          end
+
+          it 'content-type is correct' do
+            get "/api/v1/ticket_attachment/#{ticket1.id}/#{article1.id}/#{store_file.id}?disposition=inline", params: {}
+            expect(response.headers['Content-Type']).to include('text/plain')
+          end
+        end
+
+        context 'with binary file content type' do
+          let(:store_file_content_type) { 'image/svg+xml' }
+
+          it 'disposition can not be inline' do
+            get "/api/v1/ticket_attachment/#{ticket1.id}/#{article1.id}/#{store_file.id}?disposition=inline", params: {}
+            expect(response.headers['Content-Disposition']).to include('attachment')
+          end
+
+          it 'content-type was forced to active storage binary content type' do
+            get "/api/v1/ticket_attachment/#{ticket1.id}/#{article1.id}/#{store_file.id}?disposition=inline", params: {}
+            expect(response.headers['Content-Type']).to include('application/octet-stream')
+          end
+        end
+
+        context 'with allowed inline file content type' do
+          let(:store_file_content_type) { 'application/pdf' }
+
+          it 'disposition is inline' do
+            get "/api/v1/ticket_attachment/#{ticket1.id}/#{article1.id}/#{store_file.id}?disposition=inline", params: {}
+            expect(response.headers['Content-Disposition']).to include('inline')
+          end
+        end
+      end
+    end
+
+    context 'when attachment actions are used' do
       it 'does test attachments for split' do
         email_file_path  = Rails.root.join('test/data/mail/mail024.box')
         email_raw_string = File.read(email_file_path)
         ticket_p, article_p, _user_p = Channel::EmailParser.new.process({}, email_raw_string)
 
-      authenticated_as(agent)
         get '/api/v1/ticket_split', params: { form_id: '1234-2', ticket_id: ticket_p.id, article_id: article_p.id }, as: :json
         expect(response).to have_http_status(:ok)
         expect(json_response['assets']).to be_truthy
@@ -87,7 +129,6 @@ RSpec.describe 'Ticket Article Attachments', type: :request do
         email_raw_string = File.read(email_file_path)
         _ticket_p, article_p, _user_p = Channel::EmailParser.new.process({}, email_raw_string)
 
-      authenticated_as(agent)
         post "/api/v1/ticket_attachment_upload_clone_by_article/#{article_p.id}", params: {}, as: :json
         expect(response).to have_http_status(:unprocessable_entity)
         expect(json_response).to be_a_kind_of(Hash)
@@ -115,3 +156,4 @@ RSpec.describe 'Ticket Article Attachments', type: :request do
       end
     end
   end
+end