From 3cfc3dc93a01beae47d52702fde2d781e38385df Mon Sep 17 00:00:00 2001 From: Connor Ferguson Date: Thu, 8 Apr 2021 14:45:48 -0600 Subject: [PATCH 1/2] Implemented filetype validations and tests for image attachments This commit implements validations for content type. Specifically,this new validation ensures image file types .png/.jpeg/.jpg/.gif are the only accepted file formats at this time, but can be edited by altering the accepted_image_types method to allow other formats should the developer desire to implement that functionality. This method was tested by building the objects and testing validity with both a valid image file and an invalid .txt file. Commit History: Created validation tests for images Implemented validation of filetypes Renamed validation Patched file not found error for url Core test update to be compatible with new validation Core test update to be compatible with new validation Updated test to have file names Implemented compatible syntax for AS and Paperclip Separate NoFileError to another branch Refactored validation tests Returned mistakenly deleted white space Removed describe block Changed accepted image types method to constant Implemented application configuration for Image MIME types Added documentation to configuration --- .../models/spree/image/active_storage_attachment.rb | 12 ++++++++++-- core/app/models/spree/image/paperclip_attachment.rb | 2 +- core/lib/spree/app_configuration.rb | 8 ++++++++ core/lib/spree/testing_support/fixtures/file.txt | 1 + core/spec/lib/search/base_spec.rb | 5 +++-- core/spec/models/spree/image_spec.rb | 12 ++++++++++++ 6 files changed, 35 insertions(+), 5 deletions(-) create mode 100644 core/lib/spree/testing_support/fixtures/file.txt diff --git a/core/app/models/spree/image/active_storage_attachment.rb b/core/app/models/spree/image/active_storage_attachment.rb index c4f87d09e08..a4d6af391bd 100644 --- a/core/app/models/spree/image/active_storage_attachment.rb +++ b/core/app/models/spree/image/active_storage_attachment.rb @@ -7,6 +7,10 @@ module Spree::Image::ActiveStorageAttachment delegate :width, :height, to: :attachment, prefix: true included do + validates :attachment, presence: true + validate :attachment_is_an_image + validate :supported_content_type + has_attachment :attachment, styles: { mini: '48x48>', @@ -15,7 +19,11 @@ module Spree::Image::ActiveStorageAttachment large: '1200x1200>' }, default_style: :product - validates :attachment, presence: true - validate :attachment_is_an_image + + def supported_content_type + unless attachment.content_type.in?(Spree::Config.allowed_image_mime_types) + errors.add(:attachment, :content_type_not_supported) + end + end end end diff --git a/core/app/models/spree/image/paperclip_attachment.rb b/core/app/models/spree/image/paperclip_attachment.rb index 2733fdce219..6a258c3ac5a 100644 --- a/core/app/models/spree/image/paperclip_attachment.rb +++ b/core/app/models/spree/image/paperclip_attachment.rb @@ -15,7 +15,7 @@ module Spree::Image::PaperclipAttachment convert_options: { all: '-strip -auto-orient -colorspace sRGB' } validates_attachment :attachment, presence: true, - content_type: { content_type: %w[image/jpeg image/jpg image/png image/gif] } + content_type: { content_type: Spree::Config.allowed_image_mime_types } # save the w,h of the original image (from which others can be calculated) # we need to look at the write-queue for images which have not been saved yet diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index 127c02ef83f..c7067816968 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -445,6 +445,14 @@ def payment_canceller # Enumerable of images adhering to the present_image_class interface class_name_attribute :image_attachment_module, default: 'Spree::Image::ActiveStorageAttachment' + # @!attribute [rw] allowed_image_mime_types + # + # Defines which MIME types are allowed for images + # `%w(image/jpeg image/jpg image/png image/gif).freeze` is the default. + # + # @return [Array] + class_name_attribute :allowed_image_mime_types, default: %w(image/jpeg image/jpg image/png image/gif).freeze + # Allows switching attachment library for Taxon # # `Spree::Taxon::ActiveStorageAttachment` diff --git a/core/lib/spree/testing_support/fixtures/file.txt b/core/lib/spree/testing_support/fixtures/file.txt new file mode 100644 index 00000000000..a496efee84a --- /dev/null +++ b/core/lib/spree/testing_support/fixtures/file.txt @@ -0,0 +1 @@ +This is a text file diff --git a/core/spec/lib/search/base_spec.rb b/core/spec/lib/search/base_spec.rb index a27f58e2f44..a5eab4bea9f 100644 --- a/core/spec/lib/search/base_spec.rb +++ b/core/spec/lib/search/base_spec.rb @@ -22,11 +22,12 @@ context "when include_images is included in the initialization params" do let(:params) { { include_images: true, keyword: @product1.name, taxon: @taxon.id } } + let(:image_file) { File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'blank.jpg')) } subject { described_class.new(params).retrieve_products } before do - @product1.master.images.create(attachment_file_name: "Test", position: 2) - @product1.master.images.create(attachment_file_name: "Test1", position: 1) + @product1.master.images.create(attachment_file_name: 'test', attachment: image_file, position: 2) + @product1.master.images.create(attachment_file_name: 'test1', attachment: image_file, position: 1) @product1.reload end diff --git a/core/spec/models/spree/image_spec.rb b/core/spec/models/spree/image_spec.rb index 2ddcd85a7f0..96665e009bf 100644 --- a/core/spec/models/spree/image_spec.rb +++ b/core/spec/models/spree/image_spec.rb @@ -9,6 +9,18 @@ let(:default_style) { :product } end + it 'is valid when attachment has allowed content type' do + image = build(:image, + attachment: File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'blank.jpg'))) + expect(image).to be_valid + end + + it 'is not valid when attachment has restricted content type' do + image = build(:image, + attachment: File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'file.txt'))) + expect(image).to_not be_valid + end + describe 'attachment details' do let(:image_file) { File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'blank.jpg')) } subject { create(:image, attachment: image_file) } From e7e8ecca58c1cb0379e1006ae145b52b4df25728 Mon Sep 17 00:00:00 2001 From: Connor Ferguson Date: Wed, 14 Apr 2021 07:13:55 -0600 Subject: [PATCH 2/2] Added a method helper for image specs to DRY code Commit History: Changed file name to represent module name Removed unnecessary line breaks --- core/spec/lib/search/base_spec.rb | 3 ++- core/spec/models/spree/image_spec.rb | 9 ++++----- core/spec/support/image_spec_helper.rb | 7 +++++++ core/spec/support/shared_examples/attachment.rb | 3 ++- 4 files changed, 15 insertions(+), 7 deletions(-) create mode 100644 core/spec/support/image_spec_helper.rb diff --git a/core/spec/lib/search/base_spec.rb b/core/spec/lib/search/base_spec.rb index a5eab4bea9f..5b0423cc3d3 100644 --- a/core/spec/lib/search/base_spec.rb +++ b/core/spec/lib/search/base_spec.rb @@ -4,6 +4,7 @@ require 'spree/core/product_filters' RSpec.describe Spree::Core::Search::Base do + include ImageSpecHelper before do include Spree::Core::ProductFilters @taxon = create(:taxon, name: "Ruby on Rails") @@ -22,7 +23,7 @@ context "when include_images is included in the initialization params" do let(:params) { { include_images: true, keyword: @product1.name, taxon: @taxon.id } } - let(:image_file) { File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'blank.jpg')) } + let(:image_file) { open_image('blank.jpg') } subject { described_class.new(params).retrieve_products } before do diff --git a/core/spec/models/spree/image_spec.rb b/core/spec/models/spree/image_spec.rb index 96665e009bf..bcf1a58c50b 100644 --- a/core/spec/models/spree/image_spec.rb +++ b/core/spec/models/spree/image_spec.rb @@ -3,6 +3,7 @@ require 'rails_helper' RSpec.describe Spree::Image, type: :model do + include ImageSpecHelper it_behaves_like 'an attachment' do subject { create(:image) } let(:attachment_name) { :attachment } @@ -10,19 +11,17 @@ end it 'is valid when attachment has allowed content type' do - image = build(:image, - attachment: File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'blank.jpg'))) + image = build(:image, attachment: open_image('blank.jpg')) expect(image).to be_valid end it 'is not valid when attachment has restricted content type' do - image = build(:image, - attachment: File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'file.txt'))) + image = build(:image, attachment: open_image('file.txt')) expect(image).to_not be_valid end describe 'attachment details' do - let(:image_file) { File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'blank.jpg')) } + let(:image_file) { open_image('blank.jpg') } subject { create(:image, attachment: image_file) } it 'returns if attachment is present' do diff --git a/core/spec/support/image_spec_helper.rb b/core/spec/support/image_spec_helper.rb new file mode 100644 index 00000000000..7efba0a2af0 --- /dev/null +++ b/core/spec/support/image_spec_helper.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module ImageSpecHelper + def open_image(image) + File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', image)) + end +end diff --git a/core/spec/support/shared_examples/attachment.rb b/core/spec/support/shared_examples/attachment.rb index 72c678bfba4..e0abf64e26a 100644 --- a/core/spec/support/shared_examples/attachment.rb +++ b/core/spec/support/shared_examples/attachment.rb @@ -1,11 +1,12 @@ # frozen_string_literal: true RSpec.shared_examples 'an attachment' do + include ImageSpecHelper context 'valid attachment' do before do subject.send( :"#{attachment_name}=", - File.open(File.join('lib', 'spree', 'testing_support', 'fixtures', 'blank.jpg')) + open_image('blank.jpg') ) end