Scramble file secrets when related objects change policies

Summary:
Ref T10262. Files have an internal secret key which is partially used to control access to them, and determines part of the URL you need to access them. Scramble (regenerate) the secret when:

  - the view policy for the file itself changes (and the new policy is not "public" or "all users"); or
  - the view policy or space for an object the file is attached to changes (and the file policy is not "public" or "all users").

This basically means that when you change the visibility of a task, any old URLs for attached files stop working and new ones are implicitly generated.

Test Plan:
  - Attached a file to a task, used `SELECT * FROM file WHERE id = ...` to inspect the secret.
  - Set view policy to public, same secret.
  - Set view policy to me, new secret.
  - Changed task view policy, new secret.
  - Changed task space, new secret.
  - Changed task title, same old secret.
  - Added and ran unit tests which cover this behavior.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10262

Differential Revision: https://secure.phabricator.com/D15641

src/applications/files/storage/PhabricatorFile.php

@@ -139,6 +139,10 @@ final class PhabricatorFile extends PhabricatorFileDAO
     return 'F'.$this->getID();
   }
 
+  public function scrambleSecret() {
+    return $this->setSecretKey($this->generateSecretKey());
+  }
+
   public static function readUploadedFileData($spec) {
     if (!$spec) {
       throw new Exception(pht('No file was uploaded!'));

src/applications/files/storage/__tests__/PhabricatorFileTestCase.php

@@ -8,6 +8,133 @@ final class PhabricatorFileTestCase extends PhabricatorTestCase {
     );
   }
 
+  public function testFileDirectScramble() {
+    // Changes to a file's view policy should scramble the file secret.
+
+    $engine = new PhabricatorTestStorageEngine();
+    $data = Filesystem::readRandomCharacters(64);
+
+    $author = $this->generateNewTestUser();
+
+    $params = array(
+      'name' => 'test.dat',
+      'viewPolicy' => PhabricatorPolicies::POLICY_USER,
+      'authorPHID' => $author->getPHID(),
+      'storageEngines' => array(
+        $engine,
+      ),
+    );
+
+    $file = PhabricatorFile::newFromFileData($data, $params);
+
+    $secret1 = $file->getSecretKey();
+
+    // First, change the name: this should not scramble the secret.
+    $xactions = array();
+    $xactions[] = id(new PhabricatorFileTransaction())
+      ->setTransactionType(PhabricatorFileTransaction::TYPE_NAME)
+      ->setNewValue('test.dat2');
+
+    $engine = id(new PhabricatorFileEditor())
+      ->setActor($author)
+      ->setContentSource($this->newContentSource())
+      ->applyTransactions($file, $xactions);
+
+    $file = $file->reload();
+
+    $secret2 = $file->getSecretKey();
+
+    $this->assertEqual(
+      $secret1,
+      $secret2,
+      pht('No secret scramble on non-policy edit.'));
+
+    // Now, change the view policy. This should scramble the secret.
+    $xactions = array();
+    $xactions[] = id(new PhabricatorFileTransaction())
+      ->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY)
+      ->setNewValue($author->getPHID());
+
+    $engine = id(new PhabricatorFileEditor())
+      ->setActor($author)
+      ->setContentSource($this->newContentSource())
+      ->applyTransactions($file, $xactions);
+
+    $file = $file->reload();
+    $secret3 = $file->getSecretKey();
+
+    $this->assertTrue(
+      ($secret1 !== $secret3),
+      pht('Changing file view policy should scramble secret.'));
+  }
+
+  public function testFileIndirectScramble() {
+    // When a file is attached to an object like a task and the task view
+    // policy changes, the file secret should be scrambled. This invalidates
+    // old URIs if tasks get locked down.
+
+    $engine = new PhabricatorTestStorageEngine();
+    $data = Filesystem::readRandomCharacters(64);
+
+    $author = $this->generateNewTestUser();
+
+    $params = array(
+      'name' => 'test.dat',
+      'viewPolicy' => $author->getPHID(),
+      'authorPHID' => $author->getPHID(),
+      'storageEngines' => array(
+        $engine,
+      ),
+    );
+
+    $file = PhabricatorFile::newFromFileData($data, $params);
+    $secret1 = $file->getSecretKey();
+
+    $task = ManiphestTask::initializeNewTask($author);
+
+    $xactions = array();
+    $xactions[] = id(new ManiphestTransaction())
+      ->setTransactionType(ManiphestTransaction::TYPE_TITLE)
+      ->setNewValue(pht('File Scramble Test Task'));
+
+    $xactions[] = id(new ManiphestTransaction())
+      ->setTransactionType(ManiphestTransaction::TYPE_DESCRIPTION)
+      ->setNewValue('{'.$file->getMonogram().'}');
+
+    id(new ManiphestTransactionEditor())
+      ->setActor($author)
+      ->setContentSource($this->newContentSource())
+      ->applyTransactions($task, $xactions);
+
+    $file = $file->reload();
+    $secret2 = $file->getSecretKey();
+
+    $this->assertEqual(
+      $secret1,
+      $secret2,
+      pht(
+        'File policy should not scramble when attached to '.
+        'newly created object.'));
+
+    $xactions = array();
+    $xactions[] = id(new ManiphestTransaction())
+      ->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY)
+      ->setNewValue($author->getPHID());
+
+    id(new ManiphestTransactionEditor())
+      ->setActor($author)
+      ->setContentSource($this->newContentSource())
+      ->applyTransactions($task, $xactions);
+
+    $file = $file->reload();
+    $secret3 = $file->getSecretKey();
+
+    $this->assertTrue(
+      ($secret1 !== $secret3),
+      pht('Changing attached object view policy should scramble secret.'));
+  }
+
+
   public function testFileVisibility() {
     $engine = new PhabricatorTestStorageEngine();
     $data = Filesystem::readRandomCharacters(64);

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -678,6 +678,10 @@ abstract class PhabricatorApplicationTransactionEditor
 
         $editor->save();
         break;
+      case PhabricatorTransactions::TYPE_VIEW_POLICY:
+      case PhabricatorTransactions::TYPE_SPACE:
+        $this->scrambleFileSecrets($object);
+        break;
     }
   }
 
@@ -914,7 +918,6 @@ abstract class PhabricatorApplicationTransactionEditor
       }
 
       $xactions = $this->applyFinalEffects($object, $xactions);
-
       if ($read_locking) {
         $object->endReadLocking();
         $read_locking = false;
@@ -3471,4 +3474,64 @@ abstract class PhabricatorApplicationTransactionEditor
     return $phids;
   }
 
+  /**
+   * When the view policy for an object is changed, scramble the secret keys
+   * for attached files to invalidate existing URIs.
+   */
+  private function scrambleFileSecrets($object) {
+    // If this is a newly created object, we don't need to scramble anything
+    // since it couldn't have been previously published.
+    if ($this->getIsNewObject()) {
+      return;
+    }
+
+    // If the object is a file itself, scramble it.
+    if ($object instanceof PhabricatorFile) {
+      if ($this->shouldScramblePolicy($object->getViewPolicy())) {
+        $object->scrambleSecret();
+        $object->save();
+      }
+    }
+
+    $phid = $object->getPHID();
+
+    $attached_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
+      $phid,
+      PhabricatorObjectHasFileEdgeType::EDGECONST);
+    if (!$attached_phids) {
+      return;
+    }
+
+    $omnipotent_viewer = PhabricatorUser::getOmnipotentUser();
+
+    $files = id(new PhabricatorFileQuery())
+      ->setViewer($omnipotent_viewer)
+      ->withPHIDs($attached_phids)
+      ->execute();
+    foreach ($files as $file) {
+      $view_policy = $file->getViewPolicy();
+      if ($this->shouldScramblePolicy($view_policy)) {
+        $file->scrambleSecret();
+        $file->save();
+      }
+    }
+  }
+
+
+  /**
+   * Check if a policy is strong enough to justify scrambling. Objects which
+   * are set to very open policies don't need to scramble their files, and
+   * files with very open policies don't need to be scrambled when associated
+   * objects change.
+   */
+  private function shouldScramblePolicy($policy) {
+    switch ($policy) {
+      case PhabricatorPolicies::POLICY_PUBLIC:
+      case PhabricatorPolicies::POLICY_USER:
+        return false;
+    }
+
+    return true;
+  }
+
 }