Save virus scan status to blob metadata (#3816)

Migration du statut de l'analyse anti-virus vers les objets `ActiveStorage` (plutôt que d'être liés à chaque modèle).

app/jobs/anti_virus_job.rb

@@ -1,20 +0,0 @@
-class AntiVirusJob < ApplicationJob
-  include ActiveStorage::Downloading
-
-  attr_reader :blob
-
-  def perform(virus_scan)
-    @blob = ActiveStorage::Blob.find_by(key: virus_scan.blob_key)
-
-    if @blob.present?
-      download_blob_to_tempfile do |file|
-        if ClamavService.safe_file?(file.path)
-          status = VirusScan.statuses.fetch(:safe)
-        else
-          status = VirusScan.statuses.fetch(:infected)
-        end
-        virus_scan.update(scanned_at: Time.zone.now, status: status)
-      end
-    end
-  end
-end

app/jobs/virus_scanner_job.rb

@@ -0,0 +1,10 @@
+class VirusScannerJob < ApplicationJob
+  def perform(blob)
+    metadata = extract_metadata_via_virus_scanner(blob)
+    blob.update!(metadata: blob.metadata.merge(metadata))
+  end
+
+  def extract_metadata_via_virus_scanner(blob)
+    ActiveStorage::VirusScanner.new(blob).metadata
+  end
+end

app/lib/active_storage/virus_scanner.rb

@@ -0,0 +1,46 @@
+class ActiveStorage::VirusScanner
+  include ActiveStorage::Downloading
+
+  def initialize(blob)
+    @blob = blob
+  end
+
+  attr_reader :blob
+
+  PENDING = 'pending'
+  INFECTED = 'infected'
+  SAFE = 'safe'
+
+  def pending?
+    blob.metadata[:virus_scan_result] == PENDING
+  end
+
+  def infected?
+    blob.metadata[:virus_scan_result] == INFECTED
+  end
+
+  def safe?
+    blob.metadata[:virus_scan_result] == SAFE
+  end
+
+  def analyzed?
+    blob.metadata[:virus_scan_result].present?
+  end
+
+  def analyze_later
+    if !analyzed?
+      blob.update!(metadata: blob.metadata.merge(virus_scan_result: PENDING))
+      VirusScannerJob.perform_later(blob)
+    end
+  end
+
+  def metadata
+    download_blob_to_tempfile do |file|
+      if ClamavService.safe_file?(file.path)
+        { virus_scan_result: SAFE, scanned_at: Time.zone.now }
+      else
+        { virus_scan_result: INFECTED, scanned_at: Time.zone.now }
+      end
+    end
+  end
+end

app/models/avis.rb

@@ -1,6 +1,5 @@
 class Avis < ApplicationRecord
   include EmailSanitizableConcern
-  include VirusScanConcern
 
   belongs_to :dossier, touch: true
   belongs_to :gestionnaire
@@ -22,9 +21,6 @@ class Avis < ApplicationRecord
   scope :by_latest, -> { order(updated_at: :desc) }
   scope :updated_since?, -> (date) { where('avis.updated_at > ?', date) }
 
-  after_commit :create_avis_virus_scan
-  after_initialize { add_virus_scan_on(self.piece_justificative_file) }
-
   # The form allows subtmitting avis requests to several emails at once,
   # hence this virtual attribute.
   attr_accessor :emails
@@ -41,6 +37,27 @@ class Avis < ApplicationRecord
     Avis.find_by(id: avis_id)&.email == email
   end
 
+  # FIXME remove this after migrating virus_scan to blob metadata
+  def virus_scan
+    VirusScan.find_by(blob_key: piece_justificative_file.blob.key)
+  end
+
+  def virus_scan_safe?
+    virus_scan&.safe? || piece_justificative_file.virus_scanner.safe?
+  end
+
+  def virus_scan_infected?
+    virus_scan&.infected? || piece_justificative_file.virus_scanner.infected?
+  end
+
+  def virus_scan_pending?
+    virus_scan&.pending? || piece_justificative_file.virus_scanner.pending?
+  end
+
+  def virus_scan_no_scan?
+    virus_scan.blank? && !piece_justificative_file.virus_scanner.analyzed?
+  end
+
   private
 
   def notify_gestionnaire
@@ -54,8 +71,4 @@ class Avis < ApplicationRecord
       self.email = nil
     end
   end
-
-  def create_avis_virus_scan
-    create_virus_scan(self.piece_justificative_file)
-  end
 end

app/models/champs/piece_justificative_champ.rb

@@ -1,6 +1,4 @@
 class Champs::PieceJustificativeChamp < Champ
-  after_commit :create_virus_scan
-
   PIECE_JUSTIFICATIVE_FILE_MAX_SIZE = 200.megabytes
 
   PIECE_JUSTIFICATIVE_FILE_ACCEPTED_FORMATS = [
@@ -48,20 +46,26 @@ class Champs::PieceJustificativeChamp < Champ
     errors
   end
 
+  # FIXME remove this after migrating virus_scan to blob metadata
+  def virus_scan_safe?
+    virus_scan&.safe? || piece_justificative_file.virus_scanner.safe?
+  end
+
+  def virus_scan_infected?
+    virus_scan&.infected? || piece_justificative_file.virus_scanner.infected?
+  end
+
+  def virus_scan_pending?
+    virus_scan&.pending? || piece_justificative_file.virus_scanner.pending?
+  end
+
+  def virus_scan_no_scan?
+    virus_scan.blank? && !piece_justificative_file.virus_scanner.analyzed?
+  end
+
   def for_api
-    if piece_justificative_file.attached? && (virus_scan&.safe? || virus_scan&.pending?)
+    if piece_justificative_file.attached? && (virus_scan_safe? || virus_scan_pending?)
       Rails.application.routes.url_helpers.url_for(piece_justificative_file)
     end
   end
-
-  private
-
-  def create_virus_scan
-    if self.piece_justificative_file&.attachment&.blob.present?
-      VirusScan.where(champ: self).where.not(blob_key: self.piece_justificative_file.blob.key).delete_all
-      VirusScan.find_or_create_by!(champ: self, blob_key: self.piece_justificative_file.blob.key) do |virus_scan|
-        virus_scan.status = VirusScan.statuses.fetch(:pending)
-      end
-    end
-  end
 end

app/models/concerns/virus_scan_concern.rb

@@ -1,21 +0,0 @@
-module VirusScanConcern
-  extend ActiveSupport::Concern
-
-  attr_reader :attachment_attribute
-
-  def add_virus_scan_on(piece_justificative)
-    @attachment_attribute = piece_justificative
-  end
-
-  def virus_scan
-    VirusScan.find_by(blob_key: self.attachment_attribute.blob.key)
-  end
-
-  def create_virus_scan(piece_justificative)
-    if piece_justificative&.attachment&.blob.present?
-      VirusScan.find_or_create_by!(blob_key: piece_justificative.blob.key) do |virus_scan|
-        virus_scan.status = VirusScan.statuses.fetch(:pending)
-      end
-    end
-  end
-end

app/models/virus_scan.rb

@@ -6,12 +6,4 @@ class VirusScan < ApplicationRecord
     safe: 'safe',
     infected: 'infected'
   }
-
-  validates :champ_id, uniqueness: { scope: :blob_key }
-
-  after_create :perform_scan
-
-  def perform_scan
-    AntiVirusJob.perform_later(self)
-  end
 end

app/views/shared/champs/piece_justificative/_pj_link.html.haml

@@ -1,21 +0,0 @@
-- pj = champ.piece_justificative_file
-
-.pj-link
-  - if champ.virus_scan.blank? || champ.virus_scan.safe?
-    = link_to url_for(pj), target: '_blank', rel: 'noopener', title: "Télécharger la pièce jointe" do
-      %span.icon.attachment
-      = pj.filename.to_s
-    - if champ.virus_scan.blank?
-      (ce fichier n’a pas été analysé par notre antivirus, téléchargez-le avec précaution)
-
-  - else
-    = pj.filename.to_s
-    - if champ.virus_scan.pending?
-      (analyse antivirus en cours
-      = link_to "rafraichir", request.path
-      )
-    - elsif champ.virus_scan.infected?
-      - if user_can_upload
-        (virus détecté, merci d’envoyer un autre fichier)
-      - else
-        (virus détecté, le téléchargement de ce fichier est bloqué)

app/views/shared/champs/piece_justificative/_show.html.haml

@@ -1,5 +1,5 @@
 - pj = champ.piece_justificative_file
 - if pj.attached?
-  = render partial: "shared/champs/piece_justificative/pj_link", locals: { champ: champ, user_can_upload: false }
+  = render partial: "shared/piece_jointe/pj_link", locals: { object: champ, pj: pj, user_can_upload: false }
 - else
   Pièce justificative non fournie

app/views/shared/dossiers/editable_champs/_piece_justificative.html.haml

@@ -9,7 +9,7 @@
   - if pj.attached?
     .piece-justificative-actions{ id: "piece_justificative_#{champ.id}" }
       .piece-justificative-action
-        = render partial: "shared/champs/piece_justificative/pj_link", locals: { champ: champ, user_can_upload: true }
+        = render partial: "shared/piece_jointe/pj_link", locals: { object: champ, pj: champ.piece_justificative_file, user_can_upload: true }
       .piece-justificative-action
         - if champ.private?
           = link_to 'Supprimer', gestionnaire_champ_purge_champ_piece_justificative_path(procedure_id: champ.dossier.procedure_id, dossier_id: champ.dossier_id, champ_id: champ.id), remote: true, method: :delete, class: 'button small danger'

app/views/shared/piece_jointe/_pj_link.html.haml

@@ -1,19 +1,19 @@
 - if pj.attached?
   .pj-link
-    - if object.virus_scan.blank? || object.virus_scan.safe?
+    - if object.virus_scan_safe? || object.virus_scan_no_scan?
-      = link_to url_for(pj), target: '_blank', title: "Télécharger la pièce jointe" do
+      = link_to url_for(pj), target: '_blank', rel: 'noopener',  title: "Télécharger la pièce jointe" do
         %span.icon.attachment
         = pj.filename.to_s
-      - if object.virus_scan.blank?
+      - if object.virus_scan_no_scan?
         (ce fichier n’a pas été analysé par notre antivirus, téléchargez-le avec précaution)
 
     - else
       = pj.filename.to_s
-      - if object.virus_scan.pending?
+      - if object.virus_scan_pending?
         (analyse antivirus en cours
         = link_to "rafraichir", request.path
         )
-      - elsif object.virus_scan.infected?
+      - elsif object.virus_scan_infected?
         - if user_can_upload
           (virus détecté, merci d’envoyer un autre fichier)
         - else

config/initializers/active_storage.rb

@@ -1 +1,29 @@
 ActiveStorage::Service.url_expires_in = 1.hour
+
+# We want to run the virus scan on every ActiveStorage attachment,
+# regardless of its type (user-uploaded document, instructeur-uploaded attestation, form template, etc.)
+#
+# To do this, the best place to work on is the ActiveStorage::Attachment
+# objects themselves.
+#
+# We have to monkey patch ActiveStorage in order to always run an analyzer.
+# The way analyzable blob interface work is by running the first accepted analyzer.
+# This is not what we want for the virus scan. Using analyzer interface is still beneficial
+# as it takes care of downloading the blob.
+ActiveStorage::Attachment.class_eval do
+  after_create_commit :virus_scan
+
+  private
+
+  def virus_scan
+    ActiveStorage::VirusScanner.new(blob).analyze_later
+  end
+end
+
+ActiveStorage::Attached::One.class_eval do
+  def virus_scanner
+    if attached?
+      ActiveStorage::VirusScanner.new(attachment.blob)
+    end
+  end
+end

lib/tasks/deployment/20190425102459_migrate_virus_scans.rake

@@ -0,0 +1,22 @@
+namespace :after_party do
+  desc 'Deployment task: migrate_virus_scans'
+  task migrate_virus_scans: :environment do
+    puts "Running deploy task 'migrate_virus_scans'"
+
+    virus_scans = VirusScan.all
+    progress = ProgressReport.new(virus_scans.count)
+    virus_scans.find_each do |virus_scan|
+      blob = ActiveStorage::Blob.find_by(key: virus_scan.blob_key)
+      if blob
+        metadata = { virus_scan_result: virus_scan.status, scanned_at: virus_scan.scanned_at }
+        blob.update_column(:metadata, blob.metadata.merge(metadata))
+      end
+      progress.inc
+    end
+    progress.finish
+
+    # Update task as completed.  If you remove the line below, the task will
+    # run with every deploy (or every time you call after_party:run).
+    AfterParty::TaskRecord.create version: '20190425102459'
+  end
+end

spec/features/users/brouillon_spec.rb

@@ -167,8 +167,8 @@ feature 'The user' do
     expect(page).to have_text('analyse antivirus en cours')
 
     # Mark file as scanned and clean
-    virus_scan = VirusScan.last
+    attachment = ActiveStorage::Attachment.last
-    virus_scan.update(scanned_at: Time.zone.now, status: VirusScan.statuses.fetch(:safe))
+    attachment.blob.update(metadata: attachment.blob.metadata.merge(scanned_at: Time.zone.now, virus_scan_result: ActiveStorage::VirusScanner::SAFE))
     within '.piece-justificative' do
       click_on 'rafraichir'
     end

spec/jobs/virus_scanner_job_spec.rb

@@ -1,12 +1,11 @@
-RSpec.describe AntiVirusJob, type: :job do
+RSpec.describe VirusScannerJob, type: :job do
   let(:champ) do
     champ = create(:champ, :piece_justificative)
     champ.piece_justificative_file.attach(io: StringIO.new("toto"), filename: "toto.txt", content_type: "text/plain")
     champ
   end
-  let(:virus_scan) { create(:virus_scan, status: VirusScan.statuses.fetch(:pending), champ: champ, blob_key: champ.piece_justificative_file.blob.key) }
 
-  subject { AntiVirusJob.new.perform(virus_scan) }
+  subject { VirusScannerJob.new.perform(champ.piece_justificative_file.blob) }
 
   context "when no virus is found" do
     let(:virus_found?) { true }
@@ -16,7 +15,7 @@ RSpec.describe AntiVirusJob, type: :job do
       subject
     end
 
-    it { expect(virus_scan.reload.status).to eq(VirusScan.statuses.fetch(:safe)) }
+    it { expect(champ.piece_justificative_file.virus_scanner.safe?).to be_truthy }
   end
 
   context "when a virus is found" do
@@ -27,6 +26,6 @@ RSpec.describe AntiVirusJob, type: :job do
       subject
     end
 
-    it { expect(virus_scan.reload.status).to eq(VirusScan.statuses.fetch(:infected)) }
+    it { expect(champ.piece_justificative_file.virus_scanner.infected?).to be_truthy }
   end
 end

spec/models/champ_spec.rb

@@ -383,20 +383,19 @@ describe Champ do
       let(:type_de_champ) { create(:type_de_champ_piece_justificative) }
 
       context 'and there is a blob' do
-        before { champ.piece_justificative_file.attach(io: StringIO.new("toto"), filename: "toto.txt", content_type: "text/plain") }
+        before do
+          champ.piece_justificative_file.attach(io: StringIO.new("toto"), filename: "toto.txt", content_type: "text/plain")
+          champ.save
+        end
 
-        it { expect { champ.save }.to change(VirusScan, :count).by(1) }
+        it { expect(champ.piece_justificative_file.virus_scanner.analyzed?).to be_truthy }
       end
 
       context 'and there is no blob' do
-        it { expect { champ.save }.to_not change(VirusScan, :count) }
+        before { champ.save }
-      end
-    end
 
-    context 'when type_champ is not type_de_champ_piece_justificative' do
+        it { expect(champ.piece_justificative_file.virus_scanner).to be_nil }
-      let(:type_de_champ) { create(:type_de_champ_textarea) }
+      end
-
-      it { expect { champ.save }.to_not change(VirusScan, :count) }
     end
   end
 

spec/models/champs/piece_justificative_champ_spec.rb

@@ -1,23 +1,24 @@
 describe Champs::PieceJustificativeChamp do
   describe '#for_api' do
     let(:champ_pj) { create(:champ_piece_justificative) }
+    let(:metadata) { champ_pj.piece_justificative_file.blob.metadata }
 
-    before { champ_pj.virus_scan.update(status: status) }
+    before { champ_pj.piece_justificative_file.blob.update(metadata: metadata.merge(virus_scan_result: status)) }
 
     subject { champ_pj.for_api }
 
     context 'when file is safe' do
-      let(:status) { 'safe' }
+      let(:status) { ActiveStorage::VirusScanner::SAFE }
       it { is_expected.to include("/rails/active_storage/blobs/") }
     end
 
     context 'when file is not scanned' do
-      let(:status) { 'pending' }
+      let(:status) { ActiveStorage::VirusScanner::PENDING }
       it { is_expected.to include("/rails/active_storage/blobs/") }
     end
 
     context 'when file is infected' do
-      let(:status) { 'infected' }
+      let(:status) { ActiveStorage::VirusScanner::INFECTED }
       it { is_expected.to be_nil }
     end
   end

spec/serializers/champ_serializer_spec.rb

@@ -10,7 +10,7 @@ describe ChampSerializer do
 
       before do
         champ.piece_justificative_file.attach({ filename: __FILE__, io: File.open(__FILE__) })
-        champ.reload.virus_scan.safe!
+        champ.piece_justificative_file.virus_scanner.analyze_later
       end
       after { champ.piece_justificative_file.purge }
 

spec/views/shared/piece_jointe/_pj_link.html.haml_spec.rb

@@ -1,19 +1,17 @@
 require 'rails_helper'
 
-describe 'shared/champs/piece_justificative/_pj_link.html.haml', type: :view do
+describe 'shared/piece_jointe/_pj_link.html.haml', type: :view do
-  let(:champ) { create(:champ, :piece_justificative, :with_piece_justificative_file) }
+  let(:champ) { create(:champ_piece_justificative) }
-  let(:virus_scan) { nil }
+  let(:virus_scan_result) { nil }
 
   before do
-    if virus_scan
+    champ.piece_justificative_file.blob.update(metadata: champ.piece_justificative_file.blob.metadata.merge(virus_scan_result: virus_scan_result))
-      champ.update(virus_scan: virus_scan)
-    end
   end
 
-  subject { render 'shared/champs/piece_justificative/pj_link', champ: champ, user_can_upload: false }
+  subject { render 'shared/piece_jointe/pj_link', object: champ, pj: champ.piece_justificative_file, user_can_upload: false }
 
   context 'when there is no anti-virus scan' do
-    let(:virus_scan) { nil }
+    let(:virus_scan_result) { nil }
 
     it 'allows to download the file' do
       expect(subject).to have_link(champ.piece_justificative_file.filename.to_s)
@@ -22,7 +20,7 @@ describe 'shared/champs/piece_justificative/_pj_link.html.haml', type: :view do
   end
 
   context 'when the anti-virus scan is pending' do
-    let(:virus_scan) { create(:virus_scan, :pending) }
+    let(:virus_scan_result) { ActiveStorage::VirusScanner::PENDING }
 
     it 'displays the filename, but doesn’t allow to download the file' do
       expect(subject).to have_text(champ.piece_justificative_file.filename.to_s)
@@ -32,7 +30,7 @@ describe 'shared/champs/piece_justificative/_pj_link.html.haml', type: :view do
   end
 
   context 'when the file is scanned and safe' do
-    let(:virus_scan) { create(:virus_scan, :safe) }
+    let(:virus_scan_result) { ActiveStorage::VirusScanner::SAFE }
 
     it 'allows to download the file' do
       expect(subject).to have_link(champ.piece_justificative_file.filename.to_s)
@@ -40,7 +38,7 @@ describe 'shared/champs/piece_justificative/_pj_link.html.haml', type: :view do
   end
 
   context 'when the file is scanned and infected' do
-    let(:virus_scan) { create(:virus_scan, :infected) }
+    let(:virus_scan_result) { ActiveStorage::VirusScanner::INFECTED }
 
     it 'displays the filename, but doesn’t allow to download the file' do
       expect(subject).to have_text(champ.piece_justificative_file.filename.to_s)