From 2f948f7e46aecf52994ca3cad65857e419cd98ed Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Thu, 4 Mar 2021 11:50:22 +0000 Subject: [PATCH 1/3] active_storage: fix blob update hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For some reason on Rails 6.1 the `after_update_commit` hook is properly registered – but disappears from the record later, and in the end is never run. Fix it by using the general `after_commit` hook instead. --- .../blob_titre_identite_watermark_concern.rb | 15 +++++- .../concerns/blob_virus_scanner_concern.rb | 18 +++++-- spec/models/champ_spec.rb | 52 ++++++++++++++++--- 3 files changed, 71 insertions(+), 14 deletions(-) diff --git a/app/models/concerns/blob_titre_identite_watermark_concern.rb b/app/models/concerns/blob_titre_identite_watermark_concern.rb index 19e78fe9e..85ae35263 100644 --- a/app/models/concerns/blob_titre_identite_watermark_concern.rb +++ b/app/models/concerns/blob_titre_identite_watermark_concern.rb @@ -1,8 +1,19 @@ +# Request a watermark on blobs attached to a `Champs::TitreIdentiteChamp` +# after the virus scan has run. +# +# We're using a class extension here, but we could as well have a periodic +# job that watermarks relevant attachments. +# +# The `after_commit` hook is triggered, among other cases, when +# the analyzer or virus scan updates the blob metadata. When both the analyzer +# and the virus scan have run, it is now safe to start the watermarking, +# without risking to replace the picture while it is being scanned in a +# concurrent job. module BlobTitreIdentiteWatermarkConcern extend ActiveSupport::Concern included do - after_update_commit :enqueue_watermark_job + after_commit :enqueue_watermark_job end def watermark_pending? @@ -12,7 +23,7 @@ module BlobTitreIdentiteWatermarkConcern private def watermark_required? - attachments.find { |attachment| attachment.record.class.name == 'Champs::TitreIdentiteChamp' } + attachments.any? { |attachment| attachment.record.class.name == 'Champs::TitreIdentiteChamp' } end def watermark_done? diff --git a/app/models/concerns/blob_virus_scanner_concern.rb b/app/models/concerns/blob_virus_scanner_concern.rb index 80fb3151a..25c922e70 100644 --- a/app/models/concerns/blob_virus_scanner_concern.rb +++ b/app/models/concerns/blob_virus_scanner_concern.rb @@ -1,13 +1,21 @@ -# TODO: once we're using Rails 6, use the hooks on attachments creation -# (rather than on blob creation). -# This will help to avoid cloberring metadata accidentally (as metadata -# are more stable on attachment creation than on blob creation). +# Run a virus scan on all blobs after they are analyzed. +# +# We're using a class extension to ensure that all blobs get scanned, +# regardless on how they were created. This could be an ActiveStorage::Analyzer, +# but as of Rails 6.1 only the first matching analyzer is ever run on +# a blob (and we may want to analyze the dimension of a picture as well +# as scanning it). +# +# The `after_commit` hook is triggered, among other cases, when +# the analyzer updates the blob metadata. When the analyzer has run, +# it is now safe to start our own scanning, without risking to have +# two concurrent jobs overwriting the metadata of the blob. module BlobVirusScannerConcern extend ActiveSupport::Concern included do before_create :set_pending - after_update_commit :enqueue_virus_scan + after_commit :enqueue_virus_scan end def virus_scanner diff --git a/spec/models/champ_spec.rb b/spec/models/champ_spec.rb index 482a3433e..526538213 100644 --- a/spec/models/champ_spec.rb +++ b/spec/models/champ_spec.rb @@ -1,4 +1,6 @@ describe Champ do + include ActiveJob::TestHelper + require 'models/champ_shared_example.rb' it_should_behave_like "champ_spec" @@ -435,19 +437,56 @@ describe Champ do end end - describe '#enqueue_virus_check' do - let(:champ) { build(:champ_piece_justificative, type_de_champ: type_de_champ) } - + describe '#enqueue_virus_scan' do context 'when type_champ is type_de_champ_piece_justificative' do let(:type_de_champ) { create(:type_de_champ_piece_justificative) } + let(:champ) { build(:champ_piece_justificative, type_de_champ: type_de_champ) } context 'and there is a blob' do before do - champ.piece_justificative_file.attach(io: StringIO.new("toto"), filename: "toto.txt", content_type: "text/plain") - champ.save! + allow(ClamavService).to receive(:safe_file?).and_return(true) end - it { expect(champ.piece_justificative_file.virus_scanner.started?).to be_truthy } + subject do + champ.piece_justificative_file.attach(io: StringIO.new("toto"), filename: "toto.txt", content_type: "text/plain") + champ.save! + champ + end + + it 'marks the file as pending virus scan' do + expect(subject.piece_justificative_file.virus_scanner.started?).to be_truthy + end + + it 'marks the file as safe once the scan completes' do + perform_enqueued_jobs { subject } + expect(champ.reload.piece_justificative_file.virus_scanner.safe?).to be_truthy + end + end + end + end + + describe '#enqueue_watermark_job' do + context 'when type_champ is type_de_champ_titre_identite' do + let(:type_de_champ) { create(:type_de_champ_titre_identite) } + let(:champ) { build(:champ_titre_identite, type_de_champ: type_de_champ) } + + before do + allow(ClamavService).to receive(:safe_file?).and_return(true) + end + + subject do + champ.piece_justificative_file.attach(fixture_file_upload('spec/fixtures/files/logo_test_procedure.png', 'image/png')) + champ.save! + champ + end + + it 'enqueues a watermark job on file attachment' do + expect(subject.piece_justificative_file.watermark_pending?).to be_truthy + end + + it 'watermarks the file' do + perform_enqueued_jobs { subject } + expect(champ.reload.piece_justificative_file.blob.metadata[:watermark]).to be_truthy end end end @@ -535,7 +574,6 @@ describe Champ do end context "fetch_external_data_later" do - include ActiveJob::TestHelper let(:data) { 'some other data' } it "fill data from external source" do From 7f496c43e5ba331753317b424199034501833c40 Mon Sep 17 00:00:00 2001 From: Christophe Robillard Date: Thu, 4 Mar 2021 16:13:19 +0100 Subject: [PATCH 2/3] no validation for old procedures --- app/models/procedure.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/models/procedure.rb b/app/models/procedure.rb index 73e865dfd..0797ba3f2 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -202,7 +202,7 @@ class Procedure < ApplicationRecord "image/jpg", "image/png", "text/plain" - ], size: { less_than: 20.megabytes } + ], size: { less_than: 20.megabytes }, if: -> { new_record? || created_at > Date.new(2020, 2, 27) } validates :deliberation, content_type: [ "application/msword", @@ -213,9 +213,12 @@ class Procedure < ApplicationRecord "image/jpg", "image/png", "text/plain" - ], size: { less_than: 20.megabytes } + ], size: { less_than: 20.megabytes }, if: -> { new_record? || created_at > Date.new(2020, 4, 29) } + + validates :logo, content_type: ['image/png', 'image/jpg', 'image/jpeg'], + size: { less_than: 5.megabytes }, + if: -> { new_record? || created_at > Date.new(2020, 11, 13) } - validates :logo, content_type: ['image/png', 'image/jpg', 'image/jpeg'], size: { less_than: 5.megabytes } validates :api_entreprise_token, jwt_token: true, allow_blank: true before_save :update_juridique_required From 47e232f439aaec2522bb01ca8d88b0aaed0a7c4a Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Thu, 4 Mar 2021 16:18:12 +0100 Subject: [PATCH 3/3] Some associations have no date_creation --- app/graphql/schema.graphql | 2 +- app/graphql/types/personne_morale_type.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/graphql/schema.graphql b/app/graphql/schema.graphql index e5c8f8744..76b486bec 100644 --- a/app/graphql/schema.graphql +++ b/app/graphql/schema.graphql @@ -103,7 +103,7 @@ enum AddressType { } type Association { - dateCreation: ISO8601Date! + dateCreation: ISO8601Date dateDeclaration: ISO8601Date! datePublication: ISO8601Date! objet: String! diff --git a/app/graphql/types/personne_morale_type.rb b/app/graphql/types/personne_morale_type.rb index 0ba408fba..e02293bc2 100644 --- a/app/graphql/types/personne_morale_type.rb +++ b/app/graphql/types/personne_morale_type.rb @@ -75,7 +75,7 @@ module Types field :rna, String, null: false field :titre, String, null: false field :objet, String, null: false - field :date_creation, GraphQL::Types::ISO8601Date, null: false + field :date_creation, GraphQL::Types::ISO8601Date, null: true field :date_declaration, GraphQL::Types::ISO8601Date, null: false field :date_publication, GraphQL::Types::ISO8601Date, null: false end