Merge pull request #9755 from opf/fix/39130/validate-whitelist-in-finish-direct-upload

[39130] Validate the whitelist on finishing a direct upload

app/workers/attachments/finish_direct_upload_job.rb

@@ -42,27 +42,47 @@ class Attachments::FinishDirectUploadJob < ApplicationJob
       return Rails.logger.error("File for attachment #{attachment_id} was not uploaded.")
     end
 
-    begin
+    User.execute_as(attachment.author) do
-      set_attributes_from_file(attachment, local_file)
+      attach_uploaded_file(attachment, local_file)
-      save_attachment(attachment)
-      journalize_container(attachment)
-    ensure
-      File.unlink(local_file.path) if File.exist?(local_file.path)
     end
   end
 
   private
 
+  def attach_uploaded_file(attachment, local_file)
+    set_attributes_from_file(attachment, local_file)
+    validate_attachment(attachment)
+    save_attachment(attachment)
+    journalize_container(attachment)
+    attachment_created_event(attachment)
+  rescue StandardError => e
+    ::OpenProject.logger.error e
+    attachment.destroy
+  ensure
+    File.unlink(local_file.path) if File.exist?(local_file.path)
+  end
+
   def set_attributes_from_file(attachment, local_file)
-    attachment.downloads = 0
+    attachment.extend(OpenProject::ChangedBySystem)
-    attachment.set_file_size local_file
+    attachment.change_by_system do
-    attachment.set_content_type local_file
+      attachment.downloads = 0
-    attachment.set_digest local_file
+      attachment.set_file_size local_file
+      attachment.set_content_type local_file
+      attachment.set_digest local_file
+    end
   end
 
   def save_attachment(attachment)
-    User.execute_as(attachment.author) do
+    attachment.save! if attachment.changed?
-      attachment.save! if attachment.changed?
+  end
+
+  def validate_attachment(attachment)
+    contract = ::Attachments::CreateContract
+                 .new(attachment, attachment.author)
+
+    unless contract.valid?
+      errors = contract.errors.full_messages.join(", ")
+      raise "Failed to validate attachment #{attachment.id}: #{errors}"
     end
   end
 
@@ -93,4 +113,11 @@ class Attachments::FinishDirectUploadJob < ApplicationJob
     timestamps = attributes.index_with { Time.now }
     journable.update_columns(timestamps) if timestamps.any?
   end
+
+  def attachment_created_event(attachment)
+    OpenProject::Notifications.send(
+      OpenProject::Events::ATTACHMENT_CREATED,
+      attachment: attachment
+    )
+  end
 end

spec/workers/attachments/finish_direct_upload_job_integration_spec.rb

@@ -31,8 +31,11 @@
 require 'spec_helper'
 
 describe Attachments::FinishDirectUploadJob, 'integration', type: :job do
+  shared_let(:user) { FactoryBot.create :admin }
+
   let!(:pending_attachment) do
     FactoryBot.create(:attachment,
+                      author: user,
                       downloads: -1,
                       digest: '',
                       container: container)
@@ -111,4 +114,41 @@ describe Attachments::FinishDirectUploadJob, 'integration', type: :job do
     it_behaves_like 'turning pending attachment into a standard attachment'
     it_behaves_like "adding a journal to the attachment in the name of the attachment's author"
   end
+
+  context 'with an incompatible attachment whitelist',
+          with_settings: { attachment_whitelist: %w[image/png] } do
+    let!(:container) { FactoryBot.create(:work_package) }
+    let!(:container_timestamp) { container.updated_at }
+
+    it "Does not save the attachment" do
+      allow(pending_attachment).to receive(:save!)
+      allow(OpenProject.logger).to receive(:error)
+
+      job.perform(pending_attachment.id)
+
+      expect(pending_attachment).not_to have_received(:save!)
+      expect(OpenProject.logger).to have_received(:error)
+
+      container.reload
+
+      expect(container.attachments).to be_empty
+      expect { pending_attachment.reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
+
+  context 'with the user not being allowed',
+          with_settings: { attachment_whitelist: %w[image/png] } do
+    shared_let(:user) { FactoryBot.create :user }
+    let!(:container) { FactoryBot.create(:work_package) }
+
+    it "Does not save the attachment" do
+      allow(pending_attachment).to receive(:save!)
+
+      job.perform(pending_attachment.id)
+
+      expect(pending_attachment).not_to have_received(:save!)
+
+      expect { pending_attachment.reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
 end