Add attachment prominence checks

We need to check attachment prominence when:
1. serving them directly or when converted to HTML
2. generating the Xapian index.

app/controllers/attachments_controller.rb

@@ -113,9 +113,15 @@ def authenticate_attachment
       )
     end
 
-    return if @attachment
-
-    if params[:file_name]
+    if @attachment
+      if cannot?(:read, @attachment)
+        request.format = :html
+        render_hidden(
+          'request/hidden_attachment',
+          locals: { attachment: @attachment }
+        )
+      end
+    elsif 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
@@ -127,7 +133,7 @@ def authenticate_attachment_as_html
     # The conversion process can generate files in the cache directory that can
     # be served up directly by the webserver according to httpd.conf, so don't
     # allow it unless that's OK.
-    return if message_is_public?
+    return if attachment_is_public?
 
     raise ActiveRecord::RecordNotFound, 'Attachment HTML not found.'
   end
@@ -152,7 +158,7 @@ def cache_attachments
         # various fragment cache functions using Ruby Marshall to write the file
         # which adds a header, so isn't compatible with images that have been
         # extracted elsewhere from PDFs)
-        if message_is_cacheable?
+        if attachment_is_cacheable?
           logger.info("Writing cache for #{cache_key_path}")
           foi_fragment_cache_write(cache_key_path, response.body)
         end
@@ -182,16 +188,20 @@ def content_type
       'application/octet-stream'
   end
 
-  def message_is_public?
-    # If this a request and message public then it can be served up without
-    # authentication
-    prominence.is_public? && @incoming_message.is_public?
+  def attachment_is_public?
+    # If this a request, message and attachment are public then it can be served
+    # up without authentication
+    prominence.is_public? &&
+      @incoming_message.is_public? &&
+      @attachment.is_public?
   end
 
-  def message_is_cacheable?
-    # If this a request searchable and message public then we can cache any
-    # attachments as there are no custom response headers (EG X-Robots-Tag)
-    prominence.is_searchable? && message_is_public?
+  def attachment_is_cacheable?
+    # If this a request, message and attachment are searchable then we can cache
+    # as there are no custom response headers (EG X-Robots-Tag)
+    prominence.is_searchable? &&
+      @incoming_message.indexed_by_search? &&
+      @attachment.indexed_by_search?
   end
 
   def cache_key_path

app/models/ability.rb

@@ -43,7 +43,8 @@ def initialize(user, project: nil, public_token: false)
 
     # Viewing messages with prominence
     can :read, FoiAttachment do |attachment|
-      can_view_with_prominence?(attachment.prominence, attachment.incoming_message.info_request)
+      can_view_with_prominence?(attachment.prominence,
+                                attachment.incoming_message.info_request)
     end
 
     can :read, [IncomingMessage, OutgoingMessage] do |msg|

app/models/incoming_message.rb

@@ -498,6 +498,10 @@ def get_attachments_for_display
     return attachments
   end
 
+  def get_attachments_for_search_index
+    get_attachments_for_display.select(&:indexed_by_search?)
+  end
+
   def extract_attachments!
     extract_attachments
     save!
@@ -623,7 +627,7 @@ def get_attachment_text_clipped
 
   def _extract_text
     # Extract text from each attachment
-    self.get_attachments_for_display.reduce('') { |memo, attachment|
+    get_attachments_for_search_index.reduce('') { |memo, attachment|
       memo += MailHandler.get_attachment_text_one_file(attachment.content_type,
                                                        attachment.default_body,
                                                        attachment.charset)

app/views/request/hidden_attachment.html.erb

@@ -0,0 +1,7 @@
+<%- if !attachment.prominence_reason.blank? %>
+  <%= _('This attachment has been hidden.') %>
+  <%= attachment.prominence_reason %>
+<%- else %>
+  <%= _("This attachment has been hidden. There are various reasons why we " \
+        "might have done this, sorry we can't be more specific here.") %>
+<%- end %>

spec/controllers/attachments_controller_spec.rb

@@ -346,6 +346,12 @@ def expect_hidden(hidden_template)
                       prominence: prominence)
   end
 
+  let(:attachment) do
+    FactoryBot.create(:pdf_attachment,
+                      prominence: prominence,
+                      incoming_message: FactoryBot.build(:incoming_message))
+  end
+
   context 'when the request is hidden' do
     let(:prominence) { 'hidden' }
     let(:incoming_message) { info_request.incoming_messages.first }
@@ -597,6 +603,137 @@ def expect_hidden(hidden_template)
       end.to raise_error(ActiveRecord::RecordNotFound)
     end
   end
+
+  context 'when the attachment has prominence hidden' do
+    let(:prominence) { 'hidden' }
+    let(:info_request) { incoming_message.info_request }
+    let(:incoming_message) { attachment.incoming_message }
+
+    it 'does not download attachments for a non-logged in user' do
+      get :show,
+          params: {
+            incoming_message_id: incoming_message.id,
+            id: info_request.id,
+            part: 2,
+            file_name: 'interesting.pdf',
+            skip_cache: 1
+          }
+      expect_hidden('request/hidden_attachment')
+    end
+
+    it 'does not download attachments for the request owner' do
+      sign_in info_request.user
+      get :show,
+          params: {
+            incoming_message_id: incoming_message.id,
+            id: info_request.id,
+            part: 2,
+            file_name: 'interesting.pdf',
+            skip_cache: 1
+          }
+      expect_hidden('request/hidden_attachment')
+    end
+
+    it 'downloads attachments for an admin user' do
+      sign_in FactoryBot.create(:admin_user)
+      get :show,
+          params: {
+            incoming_message_id: incoming_message.id,
+            id: info_request.id,
+            part: 2,
+            file_name: 'interesting.pdf',
+            skip_cache: 1
+          }
+      expect(response.media_type).to eq('application/pdf')
+      expect(response).to be_successful
+    end
+
+    it 'should not generate an HTML version of an attachment for a request whose prominence is hidden even for an admin but should return a 404' do
+      sign_in FactoryBot.create(:admin_user)
+      expect do
+        get :show_as_html,
+            params: {
+              incoming_message_id: incoming_message.id,
+              id: info_request.id,
+              part: 2,
+              file_name: 'interesting.pdf',
+              skip_cache: 1
+            }
+      end.to raise_error(ActiveRecord::RecordNotFound)
+    end
+
+    it 'does not cache an attachment when showing an attachment to the requester or admin' do
+      sign_in info_request.user
+      expect(@controller).not_to receive(:foi_fragment_cache_write)
+      get :show,
+          params: {
+            incoming_message_id: incoming_message.id,
+            id: info_request.id,
+            part: 2,
+            file_name: 'interesting.pdf'
+          }
+    end
+  end
+
+  context 'when the attachment has prominence requester_only' do
+    let(:prominence) { 'requester_only' }
+    let(:info_request) { incoming_message.info_request }
+    let(:incoming_message) { attachment.incoming_message }
+
+    it 'does not download attachments for a non-logged in user' do
+      get :show,
+          params: {
+            incoming_message_id: incoming_message.id,
+            id: info_request.id,
+            part: 2,
+            file_name: 'interesting.pdf',
+            skip_cache: 1
+          }
+      expect_hidden('request/hidden_attachment')
+    end
+
+    it 'downloads attachments for the request owner' do
+      sign_in info_request.user
+      get :show,
+          params: {
+            incoming_message_id: incoming_message.id,
+            id: info_request.id,
+            part: 2,
+            file_name: 'interesting.pdf',
+            skip_cache: 1
+          }
+      expect(response.media_type).to eq('application/pdf')
+      expect(response).to be_successful
+    end
+
+    it 'downloads attachments for an admin user' do
+      sign_in FactoryBot.create(:admin_user)
+      get :show,
+          params: {
+            incoming_message_id: incoming_message.id,
+            id: info_request.id,
+            part: 2,
+            file_name: 'interesting.pdf',
+            skip_cache: 1
+          }
+      expect(response.media_type).to eq('application/pdf')
+      expect(response).to be_successful
+    end
+
+    it 'should not generate an HTML version of an attachment for a request whose prominence is hidden even for an admin but should return a 404' do
+      sign_in FactoryBot.create(:admin_user)
+      expect do
+        get :show_as_html,
+            params: {
+              incoming_message_id: incoming_message.id,
+              id: info_request.id,
+              part: 2,
+              file_name: 'interesting.pdf',
+              skip_cache: 1
+            }
+      end.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
 end
 
 RSpec.describe AttachmentsController, "when caching fragments",