From 6d90c7ad92b35e95ceda1b94772b7120cea84acd Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 1 Feb 2018 11:17:47 -0800
Subject: [PATCH] Save mail attachments in Files, not on the actual objects

Summary:
Depends on D18985. Ref T13053. See PHI125. Currently, mail attachments are just encoded onto the actual objects in the `MetaMTAMail` table.

This fails if attachments can't be encoded in JSON -- e.g., they aren't UTF8. This happens most often when revisions or commits attach patches to mail and those patches contain source code changes for files that are not encoded in UTF8.

Instead, save attachments in (and load attachments from) Files.

Test Plan: Enabled patches for mail, created a revision, saw it attach a patch. Viewed mail in web UI, saw link to download patch. Followed link, saw sensible file. Checked database, saw a `filePHID`. Destroyed mail with `bin/remove destroy`, saw attached files also destroyed.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18986
---
 .../PhabricatorMetaMTAMailViewController.php  |  6 +++
 .../storage/PhabricatorMetaMTAAttachment.php  | 44 ++++++++++++++++---
 .../storage/PhabricatorMetaMTAMail.php        | 41 +++++++++++++++++
 3 files changed, 86 insertions(+), 5 deletions(-)

diff --git a/src/applications/metamta/controller/PhabricatorMetaMTAMailViewController.php b/src/applications/metamta/controller/PhabricatorMetaMTAMailViewController.php
index 1aca34c2ea..03d340bac9 100644
--- a/src/applications/metamta/controller/PhabricatorMetaMTAMailViewController.php
+++ b/src/applications/metamta/controller/PhabricatorMetaMTAMailViewController.php
@@ -151,6 +151,12 @@ final class PhabricatorMetaMTAMailViewController
 
     $properties->addTextContent($body);
 
+    $file_phids = $mail->getAttachmentFilePHIDs();
+    if ($file_phids) {
+      $properties->addProperty(
+        pht('Attached Files'),
+        $viewer->loadHandles($file_phids)->renderList());
+    }
 
     return $properties;
   }
diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAAttachment.php b/src/applications/metamta/storage/PhabricatorMetaMTAAttachment.php
index b26bb0b8a7..256bee46e8 100644
--- a/src/applications/metamta/storage/PhabricatorMetaMTAAttachment.php
+++ b/src/applications/metamta/storage/PhabricatorMetaMTAAttachment.php
@@ -1,9 +1,12 @@
 <?php
 
 final class PhabricatorMetaMTAAttachment extends Phobject {
-  protected $data;
-  protected $filename;
-  protected $mimetype;
+
+  private $data;
+  private $filename;
+  private $mimetype;
+  private $file;
+  private $filePHID;
 
   public function __construct($data, $filename, $mimetype) {
     $this->setData($data);
@@ -39,18 +42,49 @@ final class PhabricatorMetaMTAAttachment extends Phobject {
   }
 
   public function toDictionary() {
+    if (!$this->file) {
+      $iterator = new ArrayIterator(array($this->getData()));
+
+      $source = id(new PhabricatorIteratorFileUploadSource())
+        ->setName($this->getFilename())
+        ->setViewPolicy(PhabricatorPolicies::POLICY_NOONE)
+        ->setMIMEType($this->getMimeType())
+        ->setIterator($iterator);
+
+      $this->file = $source->uploadFile();
+    }
+
     return array(
       'filename' => $this->getFilename(),
       'mimetype' => $this->getMimeType(),
-      'data' => $this->getData(),
+      'filePHID' => $this->file->getPHID(),
     );
   }
 
   public static function newFromDictionary(array $dict) {
-    return new PhabricatorMetaMTAAttachment(
+    $file = null;
+
+    $file_phid = idx($dict, 'filePHID');
+    if ($file_phid) {
+      $file = id(new PhabricatorFileQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withPHIDs(array($file_phid))
+        ->executeOne();
+      if ($file) {
+        $dict['data'] = $file->loadFileData();
+      }
+    }
+
+    $attachment = new self(
       idx($dict, 'data'),
       idx($dict, 'filename'),
       idx($dict, 'mimetype'));
+
+    if ($file) {
+      $attachment->file = $file;
+    }
+
+    return $attachment;
   }
 
 }
diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php
index d5111529f3..a61951650c 100644
--- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php
+++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php
@@ -197,6 +197,35 @@ final class PhabricatorMetaMTAMail
     return $result;
   }
 
+  public function getAttachmentFilePHIDs() {
+    $file_phids = array();
+
+    $dictionaries = $this->getParam('attachments');
+    if ($dictionaries) {
+      foreach ($dictionaries as $dictionary) {
+        $file_phid = idx($dictionary, 'filePHID');
+        if ($file_phid) {
+          $file_phids[] = $file_phid;
+        }
+      }
+    }
+
+    return $file_phids;
+  }
+
+  public function loadAttachedFiles(PhabricatorUser $viewer) {
+    $file_phids = $this->getAttachmentFilePHIDs();
+
+    if (!$file_phids) {
+      return array();
+    }
+
+    return id(new PhabricatorFileQuery())
+      ->setViewer($viewer)
+      ->withPHIDs($file_phids)
+      ->execute();
+  }
+
   public function setAttachments(array $attachments) {
     assert_instances_of($attachments, 'PhabricatorMetaMTAAttachment');
     $this->setParam('attachments', mpull($attachments, 'toDictionary'));
@@ -526,6 +555,12 @@ final class PhabricatorMetaMTAMail
               mpull($cc_actors, 'getEmailAddress'));
             break;
           case 'attachments':
+            $attached_viewer = PhabricatorUser::getOmnipotentUser();
+            $files = $this->loadAttachedFiles($attached_viewer);
+            foreach ($files as $file) {
+              $file->attachToObject($this->getPHID());
+            }
+
             // If the mail content must be encrypted, don't add attachments.
             if ($must_encrypt) {
               break;
@@ -1299,6 +1334,12 @@ final class PhabricatorMetaMTAMail
 
   public function destroyObjectPermanently(
     PhabricatorDestructionEngine $engine) {
+
+    $files = $this->loadAttachedFiles($engine->getViewer());
+    foreach ($files as $file) {
+      $engine->destroyObject($file);
+    }
+
     $this->delete();
   }