[#7875] Fix attachment processing

app/controllers/attachment_masks_controller.rb

@@ -5,7 +5,7 @@
 class AttachmentMasksController < ApplicationController
   before_action :set_no_crawl_headers
   before_action :find_attachment
-  before_action :ensure_attachment, :ensure_referer
+  before_action :ensure_referer, :ensure_attachment
 
   def wait
     if @attachment.masked?
@@ -38,13 +38,14 @@
 
   def find_attachment
     @attachment = GlobalID::Locator.locate_signed(params[:id])
-  end
-
-  def ensure_attachment
-    raise ActiveRecord::RecordNotFound unless @attachment
+  rescue ActiveRecord::RecordNotFound
   end
 
   def ensure_referer
     raise RouteNotFound unless params[:referer].present?
   end
+
+  def ensure_attachment
+    redirect_to(params[:referer]) unless @attachment
+  end
 end

app/jobs/foi_attachment_mask_job.rb

@@ -31,6 +31,9 @@ def perform(attachment)
     end
 
     attachment.update(body: body, masked_at: Time.zone.now)
+
+  rescue FoiAttachment::MissingAttachment
+    incoming_message.parse_raw_email!(true)
   end
 
   private

app/models/foi_attachment.rb

@@ -34,6 +34,7 @@ class FoiAttachment < ApplicationRecord
   include MessageProminence
 
   MissingAttachment = Class.new(StandardError)
+  RebuiltAttachment = Class.new(StandardError)
 
   belongs_to :incoming_message,
              inverse_of: :foi_attachments
@@ -81,6 +82,33 @@ class FoiAttachment < ApplicationRecord
   }.freeze
   # rubocop:enable Style/LineLength
 
+  # Helper method which can wrap calls to #body/#body_as_text/#default_body to
+  # ensure the `RebuiltAttachment` exception is caught. This is useful for when
+  # the body is required inline eg when the search index is being built or the
+  # text of the main attachment part is being cached in the database.
+  #
+  # Need to call this so the attachment is loaded within the block, EG, this
+  # would work:
+  #   protect_against_rebuilt_attachments do
+  #     incoming_message.foi_attachment.last.body
+  #   end
+  #
+  # but this would fail:
+  #   attachment = incoming_message.foi_attachment.last
+  #   protect_against_rebuilt_attachments do
+  #     attachment.body
+  #   end
+  def self.protect_against_rebuilt_attachments(&block)
+    errored = false
+    begin
+      block.call if block_given?
+    rescue RebuiltAttachment => ex
+      raise ex if errored
+      errored = true
+      retry
+    end
+  end
+
   def delete_cached_file!
     @cached_body = nil
     file.purge if file.attached?
@@ -107,11 +135,14 @@ def body
     if masked?
       @cached_body = file.download
     else
-      job = FoiAttachmentMaskJob.perform_now(self)
-      return body if job
-
-      raise MissingAttachment, "job already queued (ID=#{id})"
+      FoiAttachmentMaskJob.unlock!(self)
+      FoiAttachmentMaskJob.perform_now(self)
+      reload
+      body
     end
+
+  rescue ActiveRecord::RecordNotFound
+    raise RebuiltAttachment, "attachment no longer present in DB (ID=#{id})"
   end
 
   # body as UTF-8 text, with scrubbing of invalid chars if needed
@@ -133,21 +164,13 @@ def unmasked_body
       hexdigest: hexdigest
     )
   rescue MailHandler::MismatchedAttachmentHexdigest
-    unless is_public?
-      raise(MissingAttachment, "prominence not public (ID=#{id})")
-    end
-
-    unless file.attached?
-      raise(MissingAttachment, "file not attached (ID=#{id})")
-    end
-
     attributes = MailHandler.attempt_to_find_original_attachment_attributes(
       raw_email.mail,
       body: file.download
-    )
+    ) if file.attached?
 
     unless attributes
-      raise(MissingAttachment, "unable to find original (ID=#{id})")
+      raise MissingAttachment, "attachment missing in raw email (ID=#{id})"
     end
 
     update(hexdigest: attributes[:hexdigest])

app/models/incoming_message.rb

@@ -345,8 +345,10 @@ def get_main_body_text_unfolded
   # Returns body text from main text part of email, converted to UTF-8
   def get_main_body_text_internal
     parse_raw_email!
-    main_part = get_main_body_text_part
-    _convert_part_body_to_text(main_part)
+    FoiAttachment.protect_against_rebuilt_attachments do
+      main_part = get_main_body_text_part
+      _convert_part_body_to_text(main_part)
+    end
   end
 
   # Given a main text part, converts it to text
@@ -479,7 +481,8 @@ def extract_attachments
     if hidden_old_attachments.any?
       # if there are hidden attachments error as we don't want to re-build and
       # lose the prominence as this will make them public
-      raise UnableToExtractAttachments, "due to prominence of attachments " \
+      raise UnableToExtractAttachments, "unable to extract attachments due " \
+        "to prominence of attachments " \
         "(ID=#{hidden_old_attachments.map(&:id).join(', ')})"
     else
       old_attachments.each(&:mark_for_destruction)
@@ -544,7 +547,9 @@ def get_body_for_quoting
 
   # Returns text version of attachment text
   def get_attachment_text_full
-    text = _get_attachment_text_internal
+    text = FoiAttachment.protect_against_rebuilt_attachments do
+      _get_attachment_text_internal
+    end
     text = apply_masks(text, 'text/html')
 
     # This can be useful for memory debugging

spec/controllers/attachment_masks_controller_spec.rb

@@ -44,11 +44,21 @@ def wait
       end
     end
 
+    context "when attachment can't be found" do
+      it 'redirects to referer' do
+        allow(GlobalID::Locator).to receive(:locate_signed).with('ABC').
+          and_raise(ActiveRecord::RecordNotFound)
+        wait
+        expect(response).to redirect_to('/referer')
+      end
+    end
+
     context 'without attachment' do
       let(:attachment) { nil }
 
-      it 'raises record not found error' do
-        expect { wait }.to raise_error(ActiveRecord::RecordNotFound)
+      it 'redirects to referer' do
+        wait
+        expect(response).to redirect_to('/referer')
       end
     end
 
@@ -90,11 +100,21 @@ def done
       end
     end
 
+    context "when attachment can't be found" do
+      it 'redirects to referer' do
+        allow(GlobalID::Locator).to receive(:locate_signed).with('ABC').
+          and_raise(ActiveRecord::RecordNotFound)
+        done
+        expect(response).to redirect_to('/referer')
+      end
+    end
+
     context 'without attachment' do
       let(:attachment) { nil }
 
-      it 'raises record not found error' do
-        expect { done }.to raise_error(ActiveRecord::RecordNotFound)
+      it 'redirects to referer' do
+        done
+        expect(response).to redirect_to('/referer')
       end
     end
 

spec/jobs/foi_attachment_mask_job_spec.rb

@@ -90,4 +90,12 @@ def perform
     expect(attachment.body).to_not include 'dull'
     expect(attachment.body).to include 'Horse'
   end
+
+  it 'reparses raw email after rescuing FoiAttachment::MissingAttachment' do
+    allow(attachment).to receive(:unmasked_body).and_raise(
+      FoiAttachment::MissingAttachment
+    )
+    expect(incoming_message).to receive(:parse_raw_email!).with(true)
+    perform
+  end
 end

spec/models/foi_attachment_spec.rb

@@ -114,8 +114,13 @@
       end
     end
 
-    context 'when unmasked and mask job is not queued' do
-      let(:foi_attachment) { FactoryBot.create(:body_text, :unmasked) }
+    context 'when unmasked and original attachment can be found' do
+      let(:incoming_message) do
+        FactoryBot.create(:incoming_message, foi_attachments_factories: [
+          [:body_text, :unmasked]
+        ])
+      end
+      let(:foi_attachment) { incoming_message.foi_attachments.last }
 
       it 'calls the FoiAttachmentMaskJob now and return the masked body' do
         expect(FoiAttachmentMaskJob).to receive(:perform_now).
@@ -129,19 +134,50 @@
       end
     end
 
-    context 'when unmasked and mask job is already queued' do
-      let(:foi_attachment) { FactoryBot.create(:body_text, :unmasked) }
+    context 'when unmasked and original attachment can not be found' do
+      let(:incoming_message) do
+        FactoryBot.create(:incoming_message, foi_attachments_factories: [
+          [:body_text, :unmasked]
+        ])
+      end
+      let(:foi_attachment) { incoming_message.foi_attachments.last }
 
       before do
-        allow(FoiAttachmentMaskJob).to receive(:perform_now).and_return(false)
+        foi_attachment.update(hexdigest: '123')
+
+        expect(FoiAttachmentMaskJob).to receive(:perform_now).
+          with(foi_attachment).
+          and_invoke(-> (_) {
+            # mock the job
+            incoming_message.parse_raw_email!(true)
+          })
       end
 
-      it 'raises missing attachment exception' do
+      it 'raises rebuilt attachment exception' do
         expect { foi_attachment.body }.to raise_error(
-          FoiAttachment::MissingAttachment,
-          "job already queued (ID=#{foi_attachment.id})"
+          FoiAttachment::RebuiltAttachment
         )
       end
+
+      context 'and called within protect_against_rebuilt_attachments block' do
+        def body
+          FoiAttachment.protect_against_rebuilt_attachments do
+            # Note, we're not using the `foi_attachment` "let" variable as the
+            # `FoiAttachment#body` code will call `parse_raw_email!(true)` and
+            # recreate the attachments, so the body returned, will belong to a
+            # new `FoiAttachment` instance
+            incoming_message.foi_attachments.last.body
+          end
+        end
+
+        it 'does not raise rebuilt attachment exception' do
+          expect { body }.to_not raise_error
+        end
+
+        it 'returns rebuilt body' do
+          expect(body).to eq('hereisthetext')
+        end
+      end
     end
 
   end
@@ -198,7 +234,7 @@
 
     before do
       allow(foi_attachment).to receive(:raw_email).
-        and_return(double.as_null_object)
+        and_return(double(mail: Mail.new))
     end
 
     context 'when mail handler finds original attachment by hexdigest' do
@@ -212,70 +248,41 @@
       end
     end
 
-    context 'when mail handler can not original attachment by hexdigest' do
+    context 'when able to find original attachment through other means' do
       before do
         allow(MailHandler).to receive(:attachment_body_for_hexdigest).
           and_raise(MailHandler::MismatchedAttachmentHexdigest)
-      end
 
-      context 'when attachment has prominence' do
-        let(:foi_attachment) do
-          FactoryBot.create(:body_text, prominence: 'hidden')
-        end
-
-        it 'raises missing attachment exception' do
-          expect { unmasked_body }.to raise_error(
-            FoiAttachment::MissingAttachment,
-            "prominence not public (ID=#{foi_attachment.id})"
-          )
-        end
+        allow(MailHandler).to receive(
+          :attempt_to_find_original_attachment_attributes
+        ).and_return(hexdigest: 'ABC', body: 'hereistheunmaskedtext')
       end
 
-      context 'when attachment file is unattached' do
-        let(:foi_attachment) do
-          FactoryBot.create(:body_text)
-        end
-
-        it 'raises missing attachment exception' do
-          foi_attachment.file.purge
-
-          expect { unmasked_body }.to raise_error(
-            FoiAttachment::MissingAttachment,
-            "file not attached (ID=#{foi_attachment.id})"
-          )
-        end
+      it 'updates the hexdigest' do
+        expect { unmasked_body }.to change { foi_attachment.hexdigest }.
+          to('ABC')
       end
 
-      context 'when unable to find original attachment through other means' do
-        before do
-          allow(MailHandler).to receive(
-            :attempt_to_find_original_attachment_attributes
-          ).and_return(nil)
-        end
-
-        it 'raises missing attachment exception' do
-          expect { unmasked_body }.to raise_error(
-            FoiAttachment::MissingAttachment,
-            "unable to find original (ID=#{foi_attachment.id})"
-          )
-        end
+      it 'returns the attachment body from the raw email' do
+        is_expected.to eq('hereistheunmaskedtext')
       end
+    end
 
-      context 'when able to find original attachment through other means' do
-        before do
-          allow(MailHandler).to receive(
-            :attempt_to_find_original_attachment_attributes
-          ).and_return(hexdigest: 'ABC', body: 'hereistheunmaskedtext')
-        end
+    context 'when unable to find original attachment through other means' do
+      before do
+        allow(MailHandler).to receive(:attachment_body_for_hexdigest).
+          and_raise(MailHandler::MismatchedAttachmentHexdigest)
 
-        it 'updates the hexdigest' do
-          expect { unmasked_body }.to change { foi_attachment.hexdigest }.
-            to('ABC')
-        end
+        allow(MailHandler).to receive(
+          :attempt_to_find_original_attachment_attributes
+        ).and_return(nil)
+      end
 
-        it 'returns the attachment body from the raw email' do
-          is_expected.to eq('hereistheunmaskedtext')
-        end
+      it 'raises missing attachment exception' do
+        expect { unmasked_body }.to raise_error(
+          FoiAttachment::MissingAttachment,
+          "attachment missing in raw email (ID=#{foi_attachment.id})"
+        )
       end
     end