Merge pull request #4548 from tchak/fix-ativestorage-url-expiration

ActiveStorage url should expire after an hour

app/controllers/instructeurs/dossiers_controller.rb

@@ -15,7 +15,7 @@ module Instructeurs
 
     def attestation
       if dossier.attestation.pdf.attached?
-        redirect_to url_for(dossier.attestation.pdf)
+        redirect_to dossier.attestation.pdf.service_url
       end
     end
 

app/controllers/users/dossiers_controller.rb

@@ -49,7 +49,7 @@ module Users
 
     def attestation
       if dossier.attestation&.pdf&.attached?
-        redirect_to url_for(dossier.attestation.pdf)
+        redirect_to dossier.attestation.pdf.service_url
       else
         flash.notice = "L'attestation n'est plus disponible sur ce dossier."
         redirect_to dossier_path(dossier)

app/graphql/types/file.rb

@@ -7,7 +7,7 @@ module Types
     field :content_type, String, null: false
 
     def url
-      Rails.application.routes.url_helpers.url_for(object)
+      object.service_url
     end
   end
 end

app/models/champs/piece_justificative_champ.rb

@@ -48,7 +48,7 @@ class Champs::PieceJustificativeChamp < Champ
 
   def for_api
     if piece_justificative_file.attached? && (piece_justificative_file.virus_scanner.safe? || piece_justificative_file.virus_scanner.pending?)
-      Rails.application.routes.url_helpers.url_for(piece_justificative_file)
+      piece_justificative_file.service_url
     end
   end
 end

app/models/concerns/blob_signed_id_concern.rb

@@ -0,0 +1,12 @@
+module BlobSignedIdConcern
+  extend ActiveSupport::Concern
+
+  included do
+    # We override signed_id to add `expires_in` option to generated hash.
+    # This is a measure to ensure that we never under any circumstance
+    # expose permanent attachment url
+    def signed_id
+      ActiveStorage.verifier.generate(id, purpose: :blob_id, expires_in: ActiveStorage::Service.url_expires_in)
+    end
+  end
+end

app/models/concerns/blob_virus_scanner_concern.rb

@@ -2,7 +2,7 @@
 # (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).
-module BlobVirusScanner
+module BlobVirusScannerConcern
   extend ActiveSupport::Concern
 
   included do

config/initializers/active_storage.rb

@@ -6,7 +6,10 @@ ActiveStorage::Service.url_expires_in = 1.hour
 # Rails 6 adds support for `.on_load(:active_storage_attachment)`, which is
 # cleaner (as it allows to enqueue the virus scan on attachment creation, rather
 # than on blob creation).
-ActiveSupport.on_load(:active_storage_blob) { include BlobVirusScanner }
+ActiveSupport.on_load(:active_storage_blob) do
+  include BlobSignedIdConcern
+  include BlobVirusScannerConcern
+end
 
 # When an OpenStack service is initialized it makes a request to fetch
 # `publicURL` to use for all operations. We intercept the method that reads

spec/controllers/api/v2/graphql_controller_spec.rb

@@ -209,7 +209,6 @@ describe API::V2::GraphqlController do
                   checksum
                   byteSize
                   contentType
-                  url
                 }
               }
               avis {
@@ -270,8 +269,7 @@ describe API::V2::GraphqlController do
                   filename: commentaire.piece_jointe.filename.to_s,
                   contentType: commentaire.piece_jointe.content_type,
                   checksum: commentaire.piece_jointe.checksum,
-                  byteSize: commentaire.piece_jointe.byte_size,
+                  byteSize: commentaire.piece_jointe.byte_size
-                  url: Rails.application.routes.url_helpers.url_for(commentaire.piece_jointe)
                 },
                 email: commentaire.email
               }

spec/controllers/instructeurs/dossiers_controller_spec.rb

@@ -17,9 +17,9 @@ describe Instructeurs::DossiersController, type: :controller do
     context 'when a dossier has an attestation' do
       let(:dossier) { create(:dossier, :accepte, attestation: create(:attestation, :with_pdf), procedure: procedure) }
 
-      it 'redirects to attestation pdf' do
+      it 'redirects to a service tmp_url' do
         get :attestation, params: { procedure_id: procedure.id, dossier_id: dossier.id }
-        expect(response).to redirect_to(dossier.attestation.pdf_url.gsub('http://localhost:3000', ''))
+        expect(response.location).to match '/rails/active_storage/disk/'
       end
     end
   end

spec/controllers/new_administrateur/mail_templates_controller_spec.rb

@@ -14,7 +14,7 @@ describe NewAdministrateur::MailTemplatesController, type: :controller do
     it { expect(response).to have_http_status(:ok) }
 
     it 'displays the procedure logo' do
-      expect(response.body).to have_css("img[src*='#{procedure.logo_url}']")
+      expect(response.body).to have_css("img[src*='/rails/active_storage/blobs/']")
     end
 
     it 'displays the action buttons' do

spec/controllers/users/dossiers_controller_spec.rb

@@ -145,7 +145,7 @@ describe Users::DossiersController, type: :controller do
 
       it 'redirects to attestation pdf' do
         get :attestation, params: { id: dossier.id }
-        expect(response).to redirect_to(dossier.attestation.pdf_url.gsub('http://localhost:3000', ''))
+        expect(response.location).to match '/rails/active_storage/disk/'
       end
     end
   end

spec/models/champs/piece_justificative_champ_spec.rb

@@ -9,12 +9,12 @@ describe Champs::PieceJustificativeChamp do
 
     context 'when file is safe' do
       let(:status) { ActiveStorage::VirusScanner::SAFE }
-      it { is_expected.to include("/rails/active_storage/blobs/") }
+      it { is_expected.to include("/rails/active_storage/disk/") }
     end
 
     context 'when file is not scanned' do
       let(:status) { ActiveStorage::VirusScanner::PENDING }
-      it { is_expected.to include("/rails/active_storage/blobs/") }
+      it { is_expected.to include("/rails/active_storage/disk/") }
     end
 
     context 'when file is infected' do

spec/serializers/champ_serializer_spec.rb

@@ -14,7 +14,7 @@ describe ChampSerializer do
       end
       after { champ.piece_justificative_file.purge }
 
-      it { is_expected.to include(value: url_for(champ.piece_justificative_file)) }
+      it { expect(subject[:value]).to match('/rails/active_storage/disk/') }
     end
 
     context 'when type champ is not piece justificative' do

spec/serializers/dossier_serializer_spec.rb

@@ -79,13 +79,14 @@ describe DossierSerializer do
         ],
         pieces_justificatives: [
           {
-            "content_url" => champ_pj.for_api,
+            "content_url" => subject[:pieces_justificatives][0]["content_url"],
             "created_at" => champ_pj.created_at.in_time_zone('UTC').iso8601(3),
             "type_de_piece_justificative_id" => original_pj_id,
             "user" => a_hash_including("id" => dossier.user.id)
           }
         ]
       )
+      expect(subject[:pieces_justificatives][0]["content_url"]).to match('/rails/active_storage/disk/')
     end
 
     it "does not expose the PJ as a champ" do

spec/spec_helper.rb

@@ -150,6 +150,8 @@ RSpec.configure do |config|
     Typhoeus::Expectation.clear
 
     ActionMailer::Base.deliveries.clear
+
+    ActiveStorage::Current.host = 'http://test.host'
   }
 
   RSpec::Matchers.define :have_same_attributes_as do |expected, options|

spec/views/instructeur/dossiers/_state_button.html.haml_spec.rb

@@ -103,7 +103,7 @@ describe 'instructeurs/dossiers/state_button.html.haml', type: :view do
 
       it 'allows to download the justificatif' do
         expect(rendered).to have_dropdown_item('Justificatif')
-        expect(rendered).to have_link(href: url_for(dossier.justificatif_motivation.attachment.blob))
+        expect(response).to have_css("a[href*='/rails/active_storage/blobs/']", text: dossier.justificatif_motivation.attachment.filename.to_s)
       end
     end
   end

spec/views/users/dossiers/brouillon.html.haml_spec.rb

@@ -17,7 +17,7 @@ describe 'users/dossiers/brouillon.html.haml', type: :view do
   end
 
   it 'affiche un lien vers la notice' do
-    expect(rendered).to have_link("Guide de la démarche", href: url_for(procedure.notice))
+    expect(response).to have_css("a[href*='/rails/active_storage/blobs/']", text: "Guide de la démarche")
   end
 
   it 'affiche les boutons de validation' do