mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-19 05:12:41 +01:00
Bind file thumbnail visibility to the visibility of the original files
Summary: Ref T6013. Currently, when we create a thumbnail, it gets its own (default) file visibility policy. In particular, this causes the issue in T6013: thumbnails get "all users" visibility, which does not include logged-out users. Instead, a thumbnail should just have the same visibility as the original file does. Enforce this: - When loading thumbnails, reject thumbnails with invisible originals. - When filtering thumbnails, permit thumbnails with visible originals. Test Plan: As a logged-out user, thumbnails are now visible when the original files are attached to visible objects. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T6013 Differential Revision: https://secure.phabricator.com/D10410
This commit is contained in:
parent
8efea3abe9
commit
2e0361bd98
3 changed files with 196 additions and 4 deletions
|
@ -117,9 +117,9 @@ final class PhabricatorFileQuery
|
|||
// First, load the edges.
|
||||
|
||||
$edge_type = PhabricatorEdgeConfig::TYPE_FILE_HAS_OBJECT;
|
||||
$phids = mpull($files, 'getPHID');
|
||||
$file_phids = mpull($files, 'getPHID');
|
||||
$edges = id(new PhabricatorEdgeQuery())
|
||||
->withSourcePHIDs($phids)
|
||||
->withSourcePHIDs($file_phids)
|
||||
->withEdgeTypes(array($edge_type))
|
||||
->execute();
|
||||
|
||||
|
@ -131,6 +131,21 @@ final class PhabricatorFileQuery
|
|||
$object_phids[$phid] = true;
|
||||
}
|
||||
}
|
||||
|
||||
// If this file is a transform of another file, load that file too. If you
|
||||
// can see the original file, you can see the thumbnail.
|
||||
|
||||
// TODO: It might be nice to put this directly on PhabricatorFile and remove
|
||||
// the PhabricatorTransformedFile table, which would be a little simpler.
|
||||
|
||||
$xforms = id(new PhabricatorTransformedFile())->loadAllWhere(
|
||||
'transformedPHID IN (%Ls)',
|
||||
$file_phids);
|
||||
$xform_phids = mpull($xforms, 'getOriginalPHID', 'getTransformedPHID');
|
||||
foreach ($xform_phids as $derived_phid => $original_phid) {
|
||||
$object_phids[$original_phid] = true;
|
||||
}
|
||||
|
||||
$object_phids = array_keys($object_phids);
|
||||
|
||||
// Now, load the objects.
|
||||
|
@ -156,6 +171,27 @@ final class PhabricatorFileQuery
|
|||
$file->attachObjects($file_objects);
|
||||
}
|
||||
|
||||
foreach ($files as $key => $file) {
|
||||
$original_phid = idx($xform_phids, $file->getPHID());
|
||||
if ($original_phid == PhabricatorPHIDConstants::PHID_VOID) {
|
||||
// This is a special case for builtin files, which are handled
|
||||
// oddly.
|
||||
$original = null;
|
||||
} else if ($original_phid) {
|
||||
$original = idx($objects, $original_phid);
|
||||
if (!$original) {
|
||||
// If the viewer can't see the original file, also prevent them from
|
||||
// seeing the transformed file.
|
||||
$this->didRejectResult($file);
|
||||
unset($files[$key]);
|
||||
continue;
|
||||
}
|
||||
} else {
|
||||
$original = null;
|
||||
}
|
||||
$file->attachOriginalFile($original);
|
||||
}
|
||||
|
||||
return $files;
|
||||
}
|
||||
|
||||
|
|
|
@ -50,6 +50,14 @@ final class PhabricatorFile extends PhabricatorFileDAO
|
|||
|
||||
private $objects = self::ATTACHABLE;
|
||||
private $objectPHIDs = self::ATTACHABLE;
|
||||
private $originalFile = self::ATTACHABLE;
|
||||
|
||||
public static function initializeNewFile() {
|
||||
return id(new PhabricatorFile())
|
||||
->attachOriginalFile(null)
|
||||
->attachObjects(array())
|
||||
->attachObjectPHIDs(array());
|
||||
}
|
||||
|
||||
public function getConfiguration() {
|
||||
return array(
|
||||
|
@ -190,7 +198,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
|
|||
$copy_of_byteSize = $file->getByteSize();
|
||||
$copy_of_mimeType = $file->getMimeType();
|
||||
|
||||
$new_file = new PhabricatorFile();
|
||||
$new_file = PhabricatorFile::initializeNewFile();
|
||||
|
||||
$new_file->setByteSize($copy_of_byteSize);
|
||||
|
||||
|
@ -226,7 +234,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
|
|||
throw new Exception('No valid storage engines are available!');
|
||||
}
|
||||
|
||||
$file = new PhabricatorFile();
|
||||
$file = PhabricatorFile::initializeNewFile();
|
||||
|
||||
$data_handle = null;
|
||||
$engine_identifier = null;
|
||||
|
@ -848,6 +856,15 @@ final class PhabricatorFile extends PhabricatorFileDAO
|
|||
return $this;
|
||||
}
|
||||
|
||||
public function getOriginalFile() {
|
||||
return $this->assertAttached($this->originalFile);
|
||||
}
|
||||
|
||||
public function attachOriginalFile(PhabricatorFile $file = null) {
|
||||
$this->originalFile = $file;
|
||||
return $this;
|
||||
}
|
||||
|
||||
public function getImageHeight() {
|
||||
if (!$this->isViewableImage()) {
|
||||
return null;
|
||||
|
@ -942,6 +959,23 @@ final class PhabricatorFile extends PhabricatorFileDAO
|
|||
}
|
||||
|
||||
|
||||
/**
|
||||
* Remove the policy edge between this file and some object.
|
||||
*
|
||||
* @param phid Object PHID to detach from.
|
||||
* @return this
|
||||
*/
|
||||
public function detachFromObject($phid) {
|
||||
$edge_type = PhabricatorEdgeConfig::TYPE_OBJECT_HAS_FILE;
|
||||
|
||||
id(new PhabricatorEdgeEditor())
|
||||
->removeEdge($phid, $edge_type, $this->getPHID())
|
||||
->save();
|
||||
|
||||
return $this;
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* Configure a newly created file object according to specified parameters.
|
||||
*
|
||||
|
@ -1033,6 +1067,12 @@ final class PhabricatorFile extends PhabricatorFileDAO
|
|||
|
||||
switch ($capability) {
|
||||
case PhabricatorPolicyCapability::CAN_VIEW:
|
||||
// If you can see the file this file is a transform of, you can see
|
||||
// this file.
|
||||
if ($this->getOriginalFile()) {
|
||||
return true;
|
||||
}
|
||||
|
||||
// If you can see any object this file is attached to, you can see
|
||||
// the file.
|
||||
return (count($this->getObjects()) > 0);
|
||||
|
@ -1049,6 +1089,9 @@ final class PhabricatorFile extends PhabricatorFileDAO
|
|||
$out[] = pht(
|
||||
'Files attached to objects are visible to users who can view '.
|
||||
'those objects.');
|
||||
$out[] = pht(
|
||||
'Thumbnails are visible only to users who can view the original '.
|
||||
'file.');
|
||||
break;
|
||||
}
|
||||
|
||||
|
|
|
@ -8,6 +8,119 @@ final class PhabricatorFileTestCase extends PhabricatorTestCase {
|
|||
);
|
||||
}
|
||||
|
||||
public function testFileVisibility() {
|
||||
$engine = new PhabricatorTestStorageEngine();
|
||||
$data = Filesystem::readRandomCharacters(64);
|
||||
|
||||
$author = $this->generateNewTestUser();
|
||||
$viewer = $this->generateNewTestUser();
|
||||
$users = array($author, $viewer);
|
||||
|
||||
$params = array(
|
||||
'name' => 'test.dat',
|
||||
'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
|
||||
'authorPHID' => $author->getPHID(),
|
||||
'storageEngines' => array(
|
||||
$engine,
|
||||
),
|
||||
);
|
||||
|
||||
$file = PhabricatorFile::newFromFileData($data, $params);
|
||||
$filter = new PhabricatorPolicyFilter();
|
||||
|
||||
// Test bare file policies.
|
||||
$this->assertEqual(
|
||||
array(
|
||||
true,
|
||||
false,
|
||||
),
|
||||
$this->canViewFile($users, $file),
|
||||
pht('File Visibility'));
|
||||
|
||||
// Create an object and test object policies.
|
||||
|
||||
$object = ManiphestTask::initializeNewTask($author);
|
||||
$object->setViewPolicy(PhabricatorPolicies::getMostOpenPolicy());
|
||||
$object->save();
|
||||
|
||||
$this->assertTrue(
|
||||
$filter->hasCapability(
|
||||
$author,
|
||||
$object,
|
||||
PhabricatorPolicyCapability::CAN_VIEW),
|
||||
pht('Object Visible to Author'));
|
||||
|
||||
$this->assertTrue(
|
||||
$filter->hasCapability(
|
||||
$viewer,
|
||||
$object,
|
||||
PhabricatorPolicyCapability::CAN_VIEW),
|
||||
pht('Object Visible to Others'));
|
||||
|
||||
// Attach the file to the object and test that the association opens a
|
||||
// policy exception for the non-author viewer.
|
||||
|
||||
$file->attachToObject($author, $object->getPHID());
|
||||
|
||||
// Test the attached file's visibility.
|
||||
$this->assertEqual(
|
||||
array(
|
||||
true,
|
||||
true,
|
||||
),
|
||||
$this->canViewFile($users, $file),
|
||||
pht('Attached File Visibility'));
|
||||
|
||||
// Create a "thumbnail" of the original file.
|
||||
$params = array(
|
||||
'name' => 'test.thumb.dat',
|
||||
'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
|
||||
'storageEngines' => array(
|
||||
$engine,
|
||||
),
|
||||
);
|
||||
|
||||
$xform = PhabricatorFile::newFromFileData($data, $params);
|
||||
|
||||
id(new PhabricatorTransformedFile())
|
||||
->setOriginalPHID($file->getPHID())
|
||||
->setTransform('test-thumb')
|
||||
->setTransformedPHID($xform->getPHID())
|
||||
->save();
|
||||
|
||||
// Test the thumbnail's visibility.
|
||||
$this->assertEqual(
|
||||
array(
|
||||
true,
|
||||
true,
|
||||
),
|
||||
$this->canViewFile($users, $xform),
|
||||
pht('Attached Thumbnail Visibility'));
|
||||
|
||||
// Detach the object and make sure it affects the thumbnail.
|
||||
$file->detachFromObject($object->getPHID());
|
||||
|
||||
// Test the detached thumbnail's visibility.
|
||||
$this->assertEqual(
|
||||
array(
|
||||
true,
|
||||
false,
|
||||
),
|
||||
$this->canViewFile($users, $xform),
|
||||
pht('Detached Thumbnail Visibility'));
|
||||
}
|
||||
|
||||
private function canViewFile(array $users, PhabricatorFile $file) {
|
||||
$results = array();
|
||||
foreach ($users as $user) {
|
||||
$results[] = (bool)id(new PhabricatorFileQuery())
|
||||
->setViewer($user)
|
||||
->withPHIDs(array($file->getPHID()))
|
||||
->execute();
|
||||
}
|
||||
return $results;
|
||||
}
|
||||
|
||||
public function testFileStorageReadWrite() {
|
||||
$engine = new PhabricatorTestStorageEngine();
|
||||
|
||||
|
|
Loading…
Reference in a new issue