use libgd instead of ImageMagick

akostadinov · akostadinov · commit a42a2fedb821 · 2022-09-23T18:42:13.000+03:00
In RHEL8+ ImageMagick is no longer provided due to being hard
to maintain and prone to security issues.

That's why we implement an image processor based on the `libgd` backed
`local-fastimage_resize` gem for logo processing instead of the
Paperclip's default ImageMagick based processor `Paperclip::Thumbnail`.

We remove support for uploading GIF profile logos in the process.
Logo is used when generating invoices. But prawn only supports JPEGs
and PNGs.

With ImageMagick we could specify target format for the `:invoice`
style to be `png` regardless of source image format.
`local-fastimage_resize` only resizes between same formats though.

Provided very low adoption of GIF logos, it is not worth implementing
such support for `local-fastimage_resize` although it shouldn't be too
hard if need arise.

Note: existing attachments don't need to be processed because the
`:invoice` style will already be PNG. Make sure NOT to manually
re-process all attachments though. Anyway no need to.

fixes THREESCALE-8579
diff --git a/Gemfile b/Gemfile
@@ -94,9 +94,11 @@ gem 'baby_squeel', '~> 1.4.3'
 gem 'ransack', '2.4.1' # we can remove line when stop using ruby 2.4
 gem 'browser', '~> 5.0.0' # we can update to lts when we stop using ruby 2.4
 gem 'diff-lcs', '~> 1.2'
+gem 'fastimage'
 gem 'hiredis', '~> 0.6.3'
 gem 'httpclient', github: 'mikz/httpclient', branch: 'ssl-env-cert'
 gem 'json-schema', git: 'https://github.com/3scale/json-schema.git'
+gem 'local-fastimage_resize', require: 'fastimage/resize'
 gem 'paperclip', '~> 6.0'
 gem 'prawn-core', git: 'https://github.com/3scale/prawn.git', branch: '0.5.1-3scale'
 gem 'prawn-format', '0.2.1'
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -351,6 +351,7 @@ GEM
       multipart-post (>= 1.2, < 3)
     faraday_middleware (0.13.1)
       faraday (>= 0.7.4, < 1.0)
+    fastimage (2.2.6)
     ffi (1.15.4)
     ffi-compiler (1.0.1)
       ffi (>= 1.0.0)
@@ -431,6 +432,9 @@ GEM
     listen (3.2.1)
       rb-fsevent (~> 0.10, >= 0.10.3)
       rb-inotify (~> 0.9, >= 0.9.10)
+    local-fastimage (3.1.3)
+    local-fastimage_resize (3.4.0)
+      local-fastimage (~> 3.0)
     loofah (2.18.0)
       crass (~> 1.0.2)
       nokogiri (>= 1.5.9)
@@ -930,6 +934,7 @@ DEPENDENCIES
   fakefs (~> 0.18.0)
   faraday (~> 0.15.3)
   faraday_middleware (~> 0.13.1)
+  fastimage
   font-awesome-rails (~> 4.7.0.5)
   formtastic (~> 2.3.1)
   github-markdown
@@ -953,6 +958,7 @@ DEPENDENCIES
   letter_opener
   license_finder (~> 6.12.0)
   listen
+  local-fastimage_resize
   mail_view (~> 2.0.4)
   mechanize
   mimemagic (~> 0.3.10)
diff --git a/INSTALL.md b/INSTALL.md
@@ -112,9 +112,8 @@ xcode-select —install
 
 ```
 brew install
-brew install chromedriver imagemagick@6 mysql@5.7 gs pkg-config openssl geckodriver postgresql@14 memcached
+brew install chromedriver gd mysql@5.7 gs pkg-config openssl geckodriver postgresql@14 memcached
 brew link mysql@5.7 --force
-brew link imagemagick@6 --force
 brew services start mysql@5.7
 ```
 
@@ -240,8 +239,9 @@ asdf install
 
 #### Dependencies
 
+<<<<<<< HEAD
 ```
-sudo yum install patch autoconf automake bison libffi-devel libtool libyaml-devel readline-devel sqlite-devel zlib-devel openssl-devel ImageMagick ImageMagick-devel mysql-devel postgresql-devel chromedriver memcached
+sudo yum install patch autoconf automake bison libffi-devel libtool libyaml-devel readline-devel sqlite-devel zlib-devel openssl-devel gd-devel mysql-devel postgresql-devel chromedriver memcached
 
 sudo systemctl restart memcached
 ```
diff --git a/app/models/profile.rb b/app/models/profile.rb
@@ -42,14 +42,19 @@ class Profile < ApplicationRecord
   belongs_to :account
   delegate :s3_provider_prefix, to: :account
 
+  LOGO_STYLES = proc do |logo|
+    dynamic_style = logo.content_type.end_with?("gif") ? ['200x50>'.freeze, :png] : '200x50>'
+    {large: '300x300>'.freeze, medium: '150x150>'.freeze, thumb: '100x100>'.freeze, invoice: dynamic_style.freeze}.freeze
+  end
+
   # Profile has attached logo.
   has_attached_file :logo,
-    styles: { large: '300x300>'.freeze, medium: '150x150>'.freeze, thumb: '100x100>'.freeze, invoice: ['200x50>'.freeze, :png].freeze }.freeze,
+    processors: [:g_d_image_processor],
+    styles: LOGO_STYLES,
     :url => ':url_root/:account_id/:class/:attachment/:style/:basename.:extension'.freeze,
     :s3_permissions => 'public-read'.freeze,
     :default_url => '/assets/3scale-logo.png'.freeze
-  validates_attachment_content_type :logo, content_type: %r{^image\/(png|gif|jpeg)}
-
+  validates_attachment_content_type :logo, content_type: %r{^image\/(png|jpeg)}, if: :will_save_change_to_logo?
 
   # Find only published profiles.
   scope :published, -> { where(:state => 'published') }
@@ -117,6 +122,10 @@ def fix_http str
 
   delegate :provider_id_for_audits, :to => :account, :allow_nil => true
 
+  def will_save_change_to_logo?
+    logo.dirty?
+  end
+
   private
 
   #issue 7486981, this is needed since Account accepts_nested_attrs from profile
diff --git a/lib/paperclip/gd_image_processor.rb b/lib/paperclip/gd_image_processor.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+module Paperclip
+  class GDImageProcessor < Processor
+    def make
+      original_geometry = Geometry.new(*FastImage.size(file))
+      computed_geometry = original_geometry.resize_to(target_geometry_str)
+      file.rewind
+      original_geometry.to_s[/\d+x\d+/] == computed_geometry.to_s[/\d+x\d+/] ? temp_copy(file) : FastImage.resize(file, computed_geometry.width, computed_geometry.height)
+    end
+
+    private
+
+    def target_geometry_str
+      options.fetch(:geometry)
+    end
+
+    def temp_copy(uploaded_file)
+      file = Tempfile.new(["copy", File.extname(uploaded_file.path)])
+      File.copy_stream(uploaded_file, file)
+      uploaded_file.rewind
+      file.rewind
+      file
+    end
+  end
+end
diff --git a/lib/tasks/fixes.rake b/lib/tasks/fixes.rake
@@ -113,6 +113,17 @@ namespace :fixes do
     end
   end
 
+  desc "Regenerate JPEG logos :invoice style"
+  task :regenerate_jpeg_invoice_logo => :environment do
+    Profile.where(logo_content_type: "image/jpeg").find_each do |profile|
+      begin
+        profile.logo.reprocess!(:invoice)
+      rescue => exception
+        Rails.logger.error("Failed to reprocess invoice logo for #{profile.account_id}: #{exception.message}")
+      end
+    end
+  end
+
   desc "Checks for cinstances with zero plan_id and associats them with the Default plan"
   task :zero_plan_id => :environment do
     Cinstance.find_each do |cinstance|
diff --git a/openshift/system/Dockerfile b/openshift/system/Dockerfile
@@ -16,8 +16,7 @@ RUN yum-config-manager --save --setopt=cbs.centos.org_repos_sclo7-rh-ruby26-rh-c
     && yum -y update \
     && yum remove -y postgresql \
     && yum -y install centos-release-scl-rh \
-              ImageMagick \
-              ImageMagick-devel \
+              gd-devel \
               unixODBC-devel \
               mysql \
               postgresql10 postgresql10-devel postgresql10-libs \
diff --git a/test/fixtures/small.gif b/test/fixtures/small.gif
diff --git a/test/fixtures/small.png b/test/fixtures/small.png
diff --git a/test/fixtures/wide.png b/test/fixtures/wide.png
diff --git a/test/integration/provider/admin/account/logos_controller_test.rb b/test/integration/provider/admin/account/logos_controller_test.rb
@@ -5,8 +5,6 @@
 class Provider::Admin::Account::LogosControllerTest < ActionDispatch::IntegrationTest
   include ActiveSupport::Testing::FileFixtures
 
-  self.file_fixture_path = Rails.root.join('test/fixtures')
-
   def setup
     @provider = FactoryBot.create(:provider_account)
     provider.settings.allow_branding!
@@ -23,11 +21,35 @@ def setup
     assert_redirected_to edit_provider_admin_account_logo_path
     assert_equal 'The logo was successfully uploaded.', flash[:notice]
     assert_nil flash[:error]
+
+    sizes = %i(large medium thumb invoice).map do |style|
+      file = Tempfile.new(["logo-", ".jpg"])
+      provider.profile.logo.copy_to_local_file style, file.path
+      file.rewind
+      FastImage.size(file)
+    end
+    assert_equal [[300, 239], [150, 120], [100, 80], [63, 50]], sizes
+  end
+
+  test 'upload a small logo' do
+    put provider_admin_account_logo_path, params: { profile: {logo: small_logo_file} }
+
+    assert_redirected_to edit_provider_admin_account_logo_path
+    assert_equal 'The logo was successfully uploaded.', flash[:notice]
+    assert_nil flash[:error]
+
+    sizes = %i(large medium thumb invoice).map do |style|
+      file = Tempfile.new(["logo-", ".png"])
+      provider.profile.logo.copy_to_local_file style, file.path
+      file.rewind
+      FastImage.size(file)
+    end
+    assert_equal [[10, 10], [10, 10], [10, 10], [10, 10]], sizes
   end
 
   test 'update when it should fail' do
     assert_no_change(of: -> { provider.profile.reload.logo_file_name }) do
-      put provider_admin_account_logo_path, params: { profile: { logo: countries_yaml_file } }
+      put provider_admin_account_logo_path, params: { profile: { logo: fixture_file_upload(file_fixture("small.gif")) } }
     end
 
     assert_redirected_to edit_provider_admin_account_logo_path
@@ -85,7 +107,7 @@ def logo_file
     @logo_file ||= fixture_file_upload(file_fixture('hypnotoad.jpg'))
   end
 
-  def countries_yaml_file
-    @countries_yaml_file ||= fixture_file_upload(file_fixture('countries.yml'))
+  def small_logo_file
+    @small_logo_file ||= fixture_file_upload(file_fixture('small.png'))
   end
 end
diff --git a/test/unit/pdf/finance/invoice_generator_test.rb b/test/unit/pdf/finance/invoice_generator_test.rb
@@ -3,8 +3,6 @@
 require 'test_helper'
 
 class Pdf::Finance::InvoiceGeneratorTest < ActiveSupport::TestCase
-  LOGO_PICTURE = Rails.root.join('test/fixtures/wide.jpg').to_s
-
   LONG_ADDRESS = [%w[Name Farnsworth],
                   ['Address', "AAA\n" * 5],
                   %w[Country Patagonia]].freeze
@@ -27,18 +25,27 @@ class Pdf::Finance::InvoiceGeneratorTest < ActiveSupport::TestCase
     assert_equal 'application/pdf', attachment.content_type
   end
 
-  test 'should generate valid PDF content with logo and line items' do
+  test 'should generate valid PDF content with jpg logo and line items' do
+    test_generate_with_logo_and_lines(file_fixture("wide.jpg").to_s)
+  end
+
+  test 'should generate valid PDF content with png logo and line items' do
+    test_generate_with_logo_and_lines(file_fixture("wide.png").to_s)
+  end
+
+  private
+  def test_generate_with_logo_and_lines(image_path)
     @data.stubs(:logo?).returns(true)
-    @data.stubs(:logo).returns(LOGO_PICTURE)
+    @data.stubs(:logo).returns(image_path)
     @data.stubs(:provider).returns(LONG_ADDRESS)
     items = [['Licorice', '5', '222', ''],
              ['Haribo  ', '11', '11', ''],
              ['Chocolatte', '', '11', ''],
              ['Sugar', nil, '11', '']]
-
     @data.stubs(:line_items).returns(items)
-
     content = @generator.generate
-    assert_not_nil content
+
+    assert content
+    assert_equal "pdf", MimeMagic.by_magic(content)&.subtype
   end
 end
diff --git a/test/unit/profile_test.rb b/test/unit/profile_test.rb
@@ -3,15 +3,17 @@
 require 'test_helper'
 
 class ProfileTest < ActiveSupport::TestCase
+  # we need to specify subject with a logo to enforce validation of content types for #validate_attachment_content_type
+  subject { FactoryBot.build :profile, logo: file_fixture("small.png").open }
   should have_db_column :logo_file_name
   should have_db_column :logo_content_type
   should have_db_column :logo_file_size
 
   should validate_presence_of :company_size
 
   should validate_attachment_content_type(:logo)
-             .allowing('image/png', 'image/gif', 'image/jpeg')
-             .rejecting('image/svg', 'text/plain', 'text/xml', 'image/abc', 'some_image/png')
+             .allowing('image/png', 'image/jpeg')
+             .rejecting('image/svg', 'text/plain', 'text/xml', 'image/abc', 'some_image/png', 'image/gif')
 
   test '#account attribute validation: not be saved without account' do
     invalid_profile = FactoryBot.build(:profile, :account_id => nil)
@@ -117,6 +119,7 @@ def test_logo_upload
       profile.save!
 
       assert profile.logo
+      assert_equal ".jpg", profile.logo.url(:invoice)[-4..-1]
     end
 
     test 'does not accept a fake image' do
@@ -127,5 +130,40 @@ def test_logo_upload
       assert_not profile.valid?
       assert_includes profile.errors[:logo], 'has contents that are not what they are reported to be'
     end
+
+    test 'invoice style for PNG logos is .png' do
+      profile = FactoryBot.build(:profile)
+      profile.logo = file_fixture("wide.png").open
+      profile.save!
+
+      assert profile.logo
+      assert_equal ".png", profile.logo.url(:invoice)[-4..-1]
+    end
+
+    test 'invoice style for historic GIF logos is .png' do
+      profile = FactoryBot.build(:profile)
+      profile.logo = file_fixture("hypnotoad.jpg").open
+      profile.save!
+
+      # setting GIF logos is not allowed anymore, so lets cheat here
+      profile.update_column(:logo_content_type, "image/gif")
+      assert profile.logo
+      assert_equal ".png", profile.logo.url(:invoice)[-4..-1]
+    end
+
+    test 'historic GIF logos are not validated' do
+      profile = FactoryBot.build(:profile, logo: file_fixture("small.png").open)
+      assert_valid profile
+
+      # when logo attachment is dirty, validation should take place
+      profile.logo_content_type = "image/gif"
+      refute_valid profile
+
+      # when logo is in db since earlier, no validation should take place
+      profile.logo_content_type = "image/png"
+      profile.save!
+      profile.update_column(:logo_content_type, "image/gif")
+      assert_valid profile
+    end
   end
 end
diff --git a/test/unit/tasks/fixes_test.rb b/test/unit/tasks/fixes_test.rb
@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+module Tasks
+  class FixesTest < ActiveSupport::TestCase
+    attr_reader :tmpfile
+
+    setup do
+      @tmpfile = Tempfile.new(%w[logo- .image])
+    end
+
+    test 'regenerate JPEG logos :invoice style' do
+      profile = FactoryBot.create(:profile)
+      profile.logo.post_processing = false
+      file_fixture("hypnotoad.jpg").open("rb") { |io| profile.logo = io }
+      profile.save!
+
+      assert_raises(Errno::ENOENT) { profile.logo.copy_to_local_file :invoice, tmpfile.path }
+
+      execute_rake_task 'fixes.rake', 'fixes:regenerate_jpeg_invoice_logo'
+      profile.logo.copy_to_local_file :invoice, tmpfile.path
+    end
+
+    test "regenerate JPEG logos ignores pngs" do
+      profile = FactoryBot.create(:profile)
+      profile.logo.post_processing = false
+      file_fixture("small.png").open("rb") { |io| profile.logo = io }
+      profile.save!
+
+      execute_rake_task 'fixes.rake', 'fixes:regenerate_jpeg_invoice_logo'
+      assert_raises(Errno::ENOENT) { profile.logo.copy_to_local_file :invoice, tmpfile.path }
+    end
+
+    test "regenerate JPEG logos ignores gifs" do
+      profile = FactoryBot.create(:profile)
+      profile.logo.post_processing = false
+      file_fixture("hypnotoad.jpg").open("rb") { |io| profile.logo = io }
+      profile.save!
+      profile.update_column(:logo_content_type, "image/gif")
+
+      execute_rake_task 'fixes.rake', 'fixes:regenerate_jpeg_invoice_logo'
+      assert_raises(Errno::ENOENT) { profile.logo.copy_to_local_file :invoice, tmpfile.path }
+    end
+
+    test "report JPEG logos fix errors" do
+      FactoryBot.create(:profile, logo_file_name: "fake.jpg", logo_content_type: "image/jpeg")
+      Rails.logger.expects(:error).with { |val| val =~ /^Failed to reprocess invoice logo for.*/ }
+      execute_rake_task 'fixes.rake', 'fixes:regenerate_jpeg_invoice_logo'
+    end
+  end
+end
