Fix attachment caching

Update the attachment caching to take place after the authentication. In
changing this a regression for returning a 'Directory listing not
allowed' error is introduced.

In order to fix the routes have been tightened up so we can simply check
for the present of `file_name` parameter.

Fixes #6477

app/controllers/attachments_controller.rb

@@ -13,11 +13,11 @@ class AttachmentsController < ApplicationController
 
   include ProminenceHeaders
 
-  around_action :cache_attachments
-
   before_action :authenticate_attachment
   before_action :authenticate_attachment_as_html, only: :show_as_html
 
+  around_action :cache_attachments
+
   def show
     # Prevent spam to magic request address. Note that the binary
     # substitution method used depends on the content type
@@ -115,8 +115,12 @@ def authenticate_attachment
 
     return if @attachment
 
-    # If we can't find the right attachment, redirect to the incoming message:
-    redirect_to incoming_message_url(@incoming_message), status: 303
+    if params[:file_name]
+      # If we can't find the right attachment, redirect to the incoming message:
+      redirect_to incoming_message_url(@incoming_message), status: 303
+    else
+      render plain: 'Directory listing not allowed', status: 403
+    end
   end
 
   def authenticate_attachment_as_html
@@ -136,12 +140,8 @@ def cache_attachments
       if foi_fragment_cache_exists?(cache_key_path)
         logger.info("Reading cache for #{cache_key_path}")
 
-        if File.directory?(cache_key_path)
-          render plain: 'Directory listing not allowed', status: 403
-        else
-          render body: foi_fragment_cache_read(cache_key_path),
-                 content_type: content_type
-        end
+        render body: foi_fragment_cache_read(cache_key_path),
+               content_type: content_type
         return
       end
 
@@ -166,6 +166,8 @@ def part_number
 
   def original_filename
     filename = params[:file_name]
+    return unless filename
+
     if action_name == 'show_as_html'
       filename.gsub(/\.html$/, '')
     else

config/routes.rb

@@ -110,14 +110,16 @@
         :as => :similar_request,
         :via => :get
 
-  match '/request/:id/response/:incoming_message_id/attach/html/:part/*file_name' => 'attachments#show_as_html',
+  match '/request/:id/response/:incoming_message_id/attach/html/(:part(/*file_name))' => 'attachments#show_as_html',
         :format => false,
         :as => :get_attachment_as_html,
-        :via => :get
+        :via => :get,
+        :constraints => { :part => /\d+/ }
   match '/request/:id/response/:incoming_message_id/attach/:part(/*file_name)' => 'attachments#show',
         :format => false,
         :as => :get_attachment,
-        :via => :get
+        :via => :get,
+        :constraints => { :part => /\d+/ }
 
   match '/request_event/:info_request_event_id' => 'request#show_request_event',
         :as => :info_request_event,

spec/integration/errors_spec.rb

@@ -99,7 +99,7 @@
 
       # make a fake cache
       cache_key_path = Rails.root.join(
-        "cache/views/request/#{prefix}/#{id}/response/#{msg_id}/attach/0/1"
+        "cache/views/request/#{prefix}/#{id}/response/#{msg_id}/attach/html/1"
       )
       FileUtils.mkdir_p(cache_key_path)