[#66964] Custom PDF fonts: Already uploaded font is checked again and reported as invalid

https://community.openproject.org/work_packages/66964

app/controllers/custom_styles_controller.rb

@@ -30,6 +30,7 @@
 
 class CustomStylesController < ApplicationController
   include EnterpriseHelper
+  include CustomStylesControllerHelper
 
   layout "admin"
   menu_item :custom_style
@@ -75,10 +76,12 @@ class CustomStylesController < ApplicationController
   def update
     flash.clear
     @custom_style = get_or_create_custom_style
-    if @custom_style.update(custom_style_params)
+    parameters = custom_style_params
+    error = validate_font_uploads(parameters)
+    if !error && @custom_style.update(parameters)
       redirect_to custom_style_path
     else
-      flash[:error] = @custom_style.errors.full_messages
+      flash[:error] = error || @custom_style.errors.full_messages
       render action: :show, status: :unprocessable_entity
     end
   end

app/helpers/custom_styles_controller_helper.rb

@@ -0,0 +1,73 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+module CustomStylesControllerHelper
+  MAX_FONT_UPLOAD_SIZE = 30.megabytes
+
+  def validate_font_uploads(custom_style_params)
+    %i(export_font_regular export_font_bold export_font_italic export_font_bold_italic).each do |name|
+      if custom_style_params[name]&.is_a?(ActionDispatch::Http::UploadedFile)
+        error = validate_font_file(name, custom_style_params[name].tempfile)
+        return error if error
+      end
+    end
+    nil
+  end
+
+  private
+
+  def validate_font_file(name, filename)
+    error = validate_font_file_size(name, filename)
+    error ||= validate_font_file_format(name, filename)
+    error
+  end
+
+  def validate_font_file_format(name, filename)
+    "#{name.to_s.humanize} #{I18n.t("admin.custom_styles.fonts.file_is_invalid")}" unless valid_ttf?(filename)
+  end
+
+  def font_file_size(filename)
+    File.size(filename).to_i
+  end
+
+  def validate_font_file_size(name, filename)
+    size = font_file_size(filename)
+    if size >= MAX_FONT_UPLOAD_SIZE
+      "#{name.to_s.humanize} #{I18n.t("admin.custom_styles.fonts.file_too_large", count: (MAX_FONT_UPLOAD_SIZE / 1.megabyte).to_i)}"
+    end
+  end
+
+  def valid_ttf?(filename)
+    file = TTFunk::File.open(filename)
+    file.name.font_name.present?
+  rescue StandardError
+    false
+  end
+end

app/models/custom_style.rb

@@ -42,9 +42,6 @@ class CustomStyle < ApplicationRecord
   mount_uploader :export_font_italic, OpenProject::Configuration.file_uploader
   mount_uploader :export_font_bold_italic, OpenProject::Configuration.file_uploader
 
-  MAX_FONT_UPLOAD_SIZE = 30.megabytes
-  validate :validate_font_files
-
   class << self
     def current
       RequestStore.fetch(:current_custom_style) do
@@ -83,40 +80,4 @@ class CustomStyle < ApplicationRecord
       end
     end
   end
-
-  def validate_font_files
-    %i(export_font_regular export_font_bold export_font_italic export_font_bold_italic).each do |name|
-      attachment = send(name)
-      validate_font_file(name, attachment)
-    end
-  end
-
-  private
-
-  def validate_font_file(name, attachment)
-    validate_font_file_size(name, attachment) if attachment&.file
-    validate_font_file_format(name, attachment) if attachment&.file
-  end
-
-  def validate_font_file_format(name, attachment)
-    unless valid_ttf?(attachment.file.path)
-      errors.add(name, I18n.t("admin.custom_styles.fonts.file_is_invalid"))
-      attachment.remove!
-    end
-  end
-
-  def validate_font_file_size(name, attachment)
-    size = attachment.file.size.to_i
-    if size >= MAX_FONT_UPLOAD_SIZE
-      errors.add(name, I18n.t("admin.custom_styles.fonts.file_too_large", count: (MAX_FONT_UPLOAD_SIZE / 1.megabyte).to_i))
-      attachment.remove!
-    end
-  end
-
-  def valid_ttf?(filename)
-    file = TTFunk::File.open(filename)
-    file.name.font_name.present?
-  rescue StandardError
-    false
-  end
 end

spec/controllers/custom_styles_controller_spec.rb

@@ -686,22 +686,19 @@ RSpec.describe CustomStylesController do
         let(:font_file) { Rack::Test::UploadedFile.new(Rails.public_path.join("favicon.ico"), "font/ttf") }
 
         it "does respect the file size limit" do
-          # rubocop:disable RSpec/AnyInstance
-          allow_any_instance_of(CarrierWave::SanitizedFile)
-             .to receive(:size)
-                   .and_return(40.megabytes)
-          # rubocop:enable RSpec/AnyInstance
+          controller.singleton_class.include(CustomStylesControllerHelper)
+          allow(controller).to receive(:font_file_size).and_return(40.megabytes)
           post :update, params: { custom_style: { export_font_regular: font_file } }
           expect(response).to have_http_status(:unprocessable_entity)
           expect(custom_style.reload.export_font_regular).not_to be_present
-          expect(flash[:error].join).to include("is too large")
+          expect(flash[:error]).to include("is too large")
         end
 
         it "does not accept a non-font" do
           post :update, params: { custom_style: { export_font_regular: font_file } }
           expect(response).to have_http_status(:unprocessable_entity)
           expect(custom_style.reload.export_font_regular).not_to be_present
-          expect(flash[:error].join).to include "not a valid TTF font file."
+          expect(flash[:error]).to include "not a valid TTF font file."
         end
       end
     end