Any suggestions on how to test the file validation? I went and edited PhabricatorProjectEditPictureController to hardcode the PHID of a restricted image into setNewValue(). That's throwing a "You Shall Not Pass" permissions errors, but it's not coming from my validateTransactions() code, so it looks like something is kicking me out before we get far enough through applyTransactions() to trigger my validation code.

Hmm, show me the code? I have no clue what's doing the extra validation offhand.

It's possible that extractFilePHIDs() runs before validation, in which case the extra validation might effectively be untestable today, but I think writing a test is still worthwhile to make sure we don't break the validation after changes like T11035 (which may reorder some of the transaction phases), even if it doesn't actually test new validation code.

In D17954#217448, @epriestley wrote:

Hmm, show me the code? I have no clue what's doing the extra validation offhand.

Here's my janky patch: https://secure.phabricator.com/P2052 (Substitute that PHID for one that exists but is restricted on your instance). Then edit a project picture (I was using the drag and drop, but the quick icon thing should work too).

In D17954#217450, @epriestley wrote:

It's possible that extractFilePHIDs() runs before validation, in which case the extra validation might effectively be untestable today, but I think writing a test is still worthwhile to make sure we don't break the validation after changes like T11035 (which may reorder some of the transaction phases), even if it doesn't actually test new validation code.

For giggles, I tried commenting-out extractFilePHIDs() but that didn't change what I was seeing.

Perfect, thanks. Let me see if I can sort out what's happening...

Oh, from the Project slugs thing, I get this under PHP7:

>>> UNRECOVERABLE FATAL ERROR <<<

&#039;break&#039; not in the &#039;loop&#039; or &#039;switch&#039; context

/Users/epriestley/dev/core/lib/phabricator/src/applications/project/xaction/PhabricatorProjectSlugsTransaction.php:64


┻━┻ ︵ ¯\_(ツ)_/¯ ︵ ┻━┻

That's because a break; got left around at the bottom of PhabricatorProjectSlugsTransaction->getTitle().

(I have never seen this error before.)

I think there are a couple of subtle things going wrong here:

If I hard-code an image I can't see, the file attaches successfully, then redirects to /project/view/<id>/. Then, that page throws a policy error while trying to load the project because I can't see the project's profile image. This error is arising from the PhabricatorFileQuery issued by PhabricatorProjectQuery->didFilterPage(). I got a stack trace by just try/catch'ing all of PhabricatorProjectViewController->handleRequest() and phlog(...)'ing the exception:

[Thu May 18 14:49:38.784002 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496] [2017-05-18 16:49:38] EXCEPTION: (PhabricatorPolicyException) [You Shall Not Pass: Restricted File] (Can View) You do not have permission to view this object. // By default, no one can take this action. The user who uploaded a file can always view and edit it. Files attached to objects are visible to users who can view those objects. Thumbnails are visible only to users who can view the original file. at [<phabricator>/src/applications/policy/filter/PhabricatorPolicyFilter.php:640]
[Thu May 18 14:49:38.784630 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496] arcanist(head=experimental, ref.master=3c4735795a29, ref.experimental=dc65bfbe5434), corgi(head=master, ref.master=c732aa7b3fef), instances(head=master, ref.master=74b660f19aa0), ledger(head=master, ref.master=4da4a24b8779), libcore(), phabricator(head=master, ref.master=90c562fe572e, custom=1), phutil(head=master, ref.master=a900d7b63e95), secure(head=master, ref.master=fa7bd57d338a), services(head=image1, ref.master=99e27a5eed75, ref.image1=bad018b269c9)
[Thu May 18 14:49:38.784641 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #0 <#2> PhabricatorPolicyFilter::rejectObject(PhabricatorFile, string, string) called at [<phabricator>/src/applications/policy/filter/PhabricatorPolicyFilter.php:543]
[Thu May 18 14:49:38.784644 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #1 <#2> PhabricatorPolicyFilter::checkCapability(PhabricatorFile, string) called at [<phabricator>/src/applications/policy/filter/PhabricatorPolicyFilter.php:250]
[Thu May 18 14:49:38.784647 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #2 <#2> PhabricatorPolicyFilter::apply(array) called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:258]
[Thu May 18 14:49:38.784649 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #3 <#2> PhabricatorPolicyAwareQuery::execute() called at [<phabricator>/src/applications/project/query/PhabricatorProjectQuery.php:370]
[Thu May 18 14:49:38.784652 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #4 <#2> PhabricatorProjectQuery::didFilterPage(array) called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:273]
[Thu May 18 14:49:38.784654 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #5 <#2> PhabricatorPolicyAwareQuery::execute() called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:168]
[Thu May 18 14:49:38.784658 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #6 <#2> PhabricatorPolicyAwareQuery::executeOne() called at [<phabricator>/src/applications/project/controller/PhabricatorProjectController.php:51]
[Thu May 18 14:49:38.784661 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #7 <#2> PhabricatorProjectController::loadProject() called at [<phabricator>/src/applications/project/controller/PhabricatorProjectViewController.php:15]
[Thu May 18 14:49:38.784663 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #8 phlog(PhabricatorPolicyException) called at [<phabricator>/src/applications/project/controller/PhabricatorProjectViewController.php:37]
[Thu May 18 14:49:38.784666 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #9 PhabricatorProjectViewController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:268]
[Thu May 18 14:49:38.784668 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #10 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:177]
[Thu May 18 14:49:38.784671 2017] [php7:notice] [pid 2526] [client 127.0.0.1:63496]   #11 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:17]

(Possibly, in development mode, we should make silent exceptions go to the log anyway, although I think this sort of thing is fairly rare.)

You can verify that the edit actually went through by looking at the project row in the database -- I hard-coded PHID-FILE-xhvuatzxmc73hcmmz3we, and SELECT * FROM phabricator_project.project WHERE id = 38 now shows that project 38 has that as the profileImagePHID.

This shouldn't be allowed, and validation logic should be able to prevent it.

Second, we're actually missing some logic to attach the file to the project. Repro for this should be:

• As user A, upload a file which only you can see.
• Set it as the profile for public project B.
• Other users won't be able to see the project any more.

The fix for this is to call $file->attachToObject($project->getPHID()) when the file is set as the profile picture. This lets anyone who can see the project see the file.

You can double-check this on /F123 on the "Attached" tab.

But: note that we also generate a thumbnail. The thumbnail does get attached. So if you set F123 as the profile image, then load the page, we'll generate F124: a thumbnail of F123 which is properly attached to the project. In practice, this means it's very difficult to actually hit any issues because of this bug, since you'd normally never actually go look at the original F123.

PhabricatorMacroFileTransaction->applyExternalEffects() has some reference logic for doing attachToObject() properly. Possibly, we should move that up the stack somewhere and call the same (or similar) logic from both places since I think both transactions are doing roughly the same thing.

Validation looks good, just bring over the attachToObject() stuff and I think we're all set.

validation log

"log" is the hip way the kids are saying "logic" these days

In D17954#217463, @epriestley wrote:

If I hard-code an image I can't see, the file attaches successfully, then redirects to /project/view/<id>/. Then, that page throws a policy error while trying to load the project because I can't see the project's profile image.

Oh, that's interesting. Are you sure that's how it works? When I attempt to edit the project image to an unviewable image, I get redirected to a page like this: http://local.phacility.com/project/picture/1/

That page throws a permissions error, but if I examine the DB, there's no row for the transaction object, the original PHID is still listed on the projects table, and refreshing the project page (http://local.phacility.com/project/profile/1/) shows the original, viewable project image. Did you also arc patch my migration? I wonder if the modular transactions framework does something clever that prevents the transaction from going through.

That's how it seems to be working for me, at least. Here's what I did:

• I have the earlier version of this change patched locally, prior to validateTransactions().

$ git show
commit 90c562fe572e50a77140fdb5badd892deed24245
Author: Austin McKinley <austin@phacility.com>
Date:   Thu May 18 14:24:47 2017 -0700

    Migrate Project image to modular transactions

• I set the Files default view permission to "No One":

• As user @dog, I uploaded a file by drag-and-dropping it onto the home page. Note permissions are "No One":

• I grabbed the PHID of the file from the database:

mysql> SELECT phid FROM local_file.file WHERE id = 29055;
+--------------------------------+
| phid                           |
+--------------------------------+
| PHID-FILE-dokbmtmdwwr655muykvr |
+--------------------------------+
1 row in set (0.00 sec)

• I hard-coded that PHID into PhabricatorProjectEditPictureController:

• As user @hector, I went to the picture edit UI for project ID 123, picked a file with "Choose File", and clicked Upload Picture:

• Then I end up on /project/profile/123/ (I might have mistranscribed this as /view/123/ earlier, I think?) with that page raising the policy error:

• ...and the project has the no-permissions PHID in the database:

mysql> SELECT * FROM local_project.project WHERE id = 123\G
*************************** 1. row ***************************
                id: 123
              name: Elite Papercraft
              phid: PHID-PROJ-qmzlmz2cek67xw4mjimt
        authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
       dateCreated: 1454551863
      dateModified: 1495146684
            status: 0
        viewPolicy: users
        editPolicy: obj.project.members
        joinPolicy: users
isMembershipLocked: 0
  profileImagePHID: PHID-FILE-dokbmtmdwwr655muykvr
              icon: project
             color: blue
           mailKey: l6ca7j6queygcxz2aiay
       primarySlug: elite_papercraft
 parentProjectPHID: NULL
      hasWorkboard: 1
     hasMilestones: 1
    hasSubprojects: 0
   milestoneNumber: NULL
       projectPath: xDOK
      projectDepth: 0
    projectPathKey: xDOK
        properties: []
1 row in set (0.00 sec)

Oh, I get the /project/picture/xxx/ error (which is correct) with the validation -- maybe you were just testing post-validation and I was testing pre-validation? My comments have been a little out of sequence here since I had to take a quick IRL meeting about whether our pup is adorable or not.