chad retitled this revision from Fixing Phirction document move on to existing document placeholder to Fix Phriction document move on to existing document placeholder.Oct 17 2016, 2:20 PM

chad edited edge metadata.

chad awarded a token.Oct 17 2016, 4:06 PM

This creates a maybe-slightly-confusing policy error if you try to move over a page you don't have permission to overwrite, right? (That is: the error is correct, but could be tailored so it's clear that the target page, not the source page, is generating it.)

I think that's still a big step forward.

src/applications/phriction/controller/PhrictionMoveController.php
71	(Do we need this `rtrim()` stuff? I think we might have fixed `PhabricatorSlug` so it mostly works correctly.)
72	(Does this actually `needContent()`? Does `getStatus()` rely on it? I don't remember offhand..)

This revision is now accepted and ready to land.Oct 17 2016, 10:36 PM

Use $normal_slug as is

gd marked 2 inline comments as done.Oct 18 2016, 11:12 AM

gd added inline comments.

src/applications/phriction/controller/PhrictionMoveController.php
71	You're right, looks like this `rtrim()` is not needed.
72	`needContent()` is needed because `getContent()` is called down the line.

Use omnipotent user to produce a correct error message

Using PhabricatorUser::getOmnipotentUser() produces the correct error message even if the document is not visible to the user:

You can not move this document there, because it would overwrite an existing document which is already at that location. Move or delete the existing document first.

This probably now allows you to move documents underneath a document you can't edit, and immediately lose edit access to them -- maybe? Ideally, that would be a different error ("You can not move this document to that location because you don't have permission to edit documents at that location and would no longer be able to edit it." or similar).

The repro steps I'm imagining are:

Set a/ to "Can Edit: alice".
As any other user, move b/example to a/example.
a/example can now only be edited by user @alice?
- The moving user was able to create a document they could not normally create?
- The moving user can no longer edit the document?

That might actually work fine already, though.

In D16713#197297, @epriestley wrote:

This probably now allows you to move documents underneath a document you can't edit, and immediately lose edit access to them -- maybe? Ideally, that would be a different error ("You can not move this document to that location because you don't have permission to edit documents at that location and would no longer be able to edit it." or similar).

The repro steps I'm imagining are:

Set a/ to "Can Edit: alice".

As any other user, move b/example to a/example.

a/example can now only be edited by user @alice?

The moving user was able to create a document they could not normally create?

The moving user can no longer edit the document?

That might actually work fine already, though.

In this scenario I get same slightly-confusing policy error as before but the document is not moved:

You do not have permission to view this object.

Users with the "Can View" capability:

Administrators can take this action.

To view a wiki document, you must also be able to view all of its parents.

Cool, that's good enough. Thanks for testing!

I think this is good to go unless you want to make any other changes. We have a larger iteration on Phriction planned in the future (T9379) and can smooth out things like not-quite-the-best error messages then (since we'll move to EditEngine + Modular Transactions anyway, and have to do work in this section of the code regardless).