Page MenuHomePhabricator

Add default policy to Files application
ClosedPublic

Authored by chad on Nov 20 2014, 9:01 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 18, 9:11 PM
Unknown Object (File)
Thu, Apr 11, 8:45 AM
Unknown Object (File)
Thu, Apr 4, 3:16 PM
Unknown Object (File)
Thu, Mar 28, 9:31 PM
Unknown Object (File)
Mar 11 2024, 7:56 AM
Unknown Object (File)
Mar 11 2024, 7:56 AM
Unknown Object (File)
Mar 11 2024, 7:56 AM
Unknown Object (File)
Mar 11 2024, 7:56 AM

Details

Summary

WIP This adds default capability to the Files application

Test Plan

Set default to public, go to Files page, see public preset. Upload File. Doesn't work.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

chad retitled this revision from to Add default policy to Files application.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added reviewers: epriestley, btrahan.
src/applications/files/storage/PhabricatorFile.php
279

This is my main question. Setting $viewer here means I'd have to update a dozen callsites and pass $viewer everywhere. Is that the correctest way? newFromFileData is the main function that gets called everywhere.

epriestley edited edge metadata.

We can pull the application without needing the viewer like this, using PhabricatorUser::getOmnipotentUser():

$app = id(new PhabricatorApplicationQuery())
    ->setViewer(PhabricatorUser::getOmnipotentUser())
    ->withClasses(array('PhabricatorFilesApplication'))
    ->executeOne();

It's slightly "better" to use a real user (and for many other applications there's some other reason we need a real user -- for example, to populate an author field), but there's no practical difference in this case. The big thing that using a real user does is make this fail in an obvious way if you messed something up and the user doesn't actually have access to use the application. However, Files is a required application and can not be uninstalled or have access limited, so it's moot in this case.

There are also a lot of cases where we create a file but don't necessarily have a meaningful viewer (for example, when pulling profile pictures from Facebook or Google during registration), so you wouldn't really be able to provide one there anyway.

So:

  • You can just use PhabricatorUser::getOmnipotentUser() to load the $app.
  • Maybe we'll refine this eventually and optionally let you pass a viewer in (after T5681?).
  • You don't need to update every callsite, just the user-facing ones. Some of them are really deep in non-user-facing workflows and can get cleaned up eventually.
  • Rest of this diff looks good.
This revision now requires changes to proceed.Nov 21 2014, 12:36 AM
chad edited edge metadata.
  • More lint
  • Update per comments
chad edited edge metadata.
  • missed a derp
epriestley edited edge metadata.
This revision is now accepted and ready to land.Nov 21 2014, 7:08 PM
This revision was automatically updated to reflect the committed changes.