Page MenuHomePhabricator

Remove requireCapabilities() from ApplicationTransactionEditor and require CAN_EDIT by default
ClosedPublic

Authored by epriestley on Aug 14 2018, 11:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 25, 1:57 AM
Unknown Object (File)
Sun, Apr 21, 5:45 PM
Unknown Object (File)
Sat, Apr 20, 6:29 AM
Unknown Object (File)
Fri, Apr 19, 7:51 PM
Unknown Object (File)
Fri, Apr 19, 1:05 PM
Unknown Object (File)
Fri, Apr 19, 1:04 PM
Unknown Object (File)
Fri, Apr 19, 1:03 PM
Unknown Object (File)
Thu, Apr 18, 9:13 AM
Subscribers
None

Details

Summary

Depends on D19585. Ref T13164.

Almost all transactions require CAN_EDIT on the object, but they generally do not enforce this directly today. Instead, this is effectively enforced by Controllers, API methods, and EditEngine doing a CAN_EDIT check when loading the object to be edited.

A small number of transactions do not require CAN_EDIT, and instead require only a weaker/lesser permission. These are:

  • Joining a project which you have CAN_JOIN on.
  • Leaving a project which isn't locked.
  • Joining a Conpherence thread you can see (today, no separate CAN_JOIN permission for Conpherence).
  • Leaving a Conpherence thread.
  • Unsubscribing.
  • Using the special !history command from email.

Additionally, these require CAN_INTERACT, which is weaker than CAN_EDIT:

  • Adding comments.
  • Subscribing.
  • Awarding tokens.

Soon, I want to add "disabling users" to this list, so that you can disable users if you have "Can Disable User" permission, even if you can not otherwise edit users.

It's possible this list isn't exhaustive, so this change might break something by adding a policy check to a place where we previously didn't have one. If so, we can go weaken that policy check to the appropriate level.

Enforcement of these special cases is currently weird:

  • We mostly don't actually enforce CAN_EDIT in the Editor; instead, it's enforced before you get to the editor (in EditEngine/Controllers).
  • To apply a weaker requirement (like leaving comments or leaving a project), we let you get through the Controller without CAN_EDIT, then apply the weaker policy check in the Editor.
  • Some transactions apply a confusing/redundant explicit CAN_EDIT policy check. These mostly got cleaned up in previous changes.

Instead, the new world order is:

  • Every transaction has capability/policy requirements.
  • The default is CAN_EDIT, but transactions can weaken this explicitly they want.
  • So now we'll get requirements right in the Editor, even if Controllers or API endpoints make a mistake.
  • And you don't have to copy/paste a bunch of code to say "yes, every transaction should require CAN_EDIT".
Test Plan
  • Tried to add members to a Conpherence thread I could not edit (permissions error).
  • Left a Conpherence thread I could not edit (worked properly).
  • Joined a thread I could see but could not edit (worked properly).
  • Tried to join a thread I could not see (permissions error).
  • Implemented requireCapabilites() on ManiphestTransactionEditor and tried to edit a task (upgrade guidance error).
  • Mentioned an object I can not edit on another object (works).
  • Mentioned another object on an object I can not edit (works).
  • Added a {F...} reference to an object I can not edit (works).
  • Awarded tokens to an object I can not edit (works).
  • Subscribed/unsubscribed from an object I can not edit (works).
  • Muted/unmuted an object I can not edit (works).
  • Tried to do other types of edits to an object I can not edit (correctly results in a permissions error).
  • Joined and left a project I can not edit (works).
  • Tried to edit and add members to a project I can not edit (permissions error).

Event Timeline

epriestley planned changes to this revision.

Still needs actual testing, I just wanted to write this up while it's on my mind before taking Darcy for a walk.

  • Made unit tests pass (see D19592).
  • Added a throw if an Editor defines requireCapabilities(), so that we break compatibility with an explicit error instead of with a missing policy check.
  • See T13186 for additional discussion and guidance.
  • Add a reduced strength case for "Mute Notifications".
  • Add a reduced strength case for inverse edge edits, like when you mention an object you can't edit somewhere else.

Looks good other than minor inlines.

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
1487

"often don't can not be"

1491

How can we end up here without any transactions? And if PhabricatorApplicationTransactionEditor has $xactions as an instance variable, why is it also passed along as an argument?

1493

"If we"

1514

So this just Does The Right Thing ™ to turn objects into arrays and leaves arrays alone? Oh, PHP...

1529

->setViewer($actor)

src/applications/transactions/storage/PhabricatorModularTransactionType.php
374

Maybe use an example other than leaving a project, since leaving a project occasionally does require CAN_EDIT?

This revision is now accepted and ready to land.Aug 17 2018, 5:37 PM
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
1491

We can reach this code with no transactions if, for example, a user calls whatever.edit with an empty list of transactions. At least today, this is generally "legal" even though it's somewhat silly. It may even do things: for example, if a recently-written Herald rule takes an action as a result of the current object state. I believe we also will ping webhooks with an empty transaction list today (but: perhaps we should not).

Slightly later, we'll abort if setContinueOnNoEffect() isn't set and there are no transactions remaining after filtering. The common case where this happens is that you go to a task "Edit" page and then click "Save" without changing anything. This applies a bunch of transactions (set title to current title, set description to current description, etc) but none of them change anything. They all get filtered out and we're left with no transactions.

When all the transactions get filtered, we throw if setContinueOnNoEffect() is false, or continue if it is true. For common edits:

  • We continue on no effect from an "Edit Object" screen (so "Save" without changing anything is legal).
  • We continue on no effect from *.edit (so "setting" an object to a set of values it already has is a legal no-op).
  • We stop (and then prompt) from "Comment" action areas to reduce user collisions when updating objects.

$this->xactions are the raw incoming transactions, and $xactions are transactions after some processing.

I fiddled around with the execution order here a bit and we now do these checks before filtering transactions, so it's possible that you currently can't actually reach this code with $this->xactions and $xactions having different emptiness (that is, either both are empty or both have content). But this code is more robust against reordering if it checks "nothing was attempted AND nothing has made it this far", since if it executed slightly (after filtering) this distinction would matter. I'm inclined to keep this check since it's possible it will get reordered at some point.

1514

Yeaaaaah.

This is sufficiently weird that maybe we should call it phutil_vectorize() and just return (array)$object;.

But note that (array)null is array(), not array(null), which is perhaps counterintuitive. That is, the two forks here aren't really necessary, but I've arbitrarily drawn the line at (array) being too bizarre/confusing when applied to null? Maybe this should just move to a function. We don't do it terribly often but it sure is weird.

In particular, (array)$object has a bad behavior which we never want: it turns the object into a dictionary. We'd almost certainly prefer this return array($object). Maybe a good argument for phutil_vectorize().

  • Fix mangled comments; use a less ambiguous example of policy strength reduction.
This revision was automatically updated to reflect the committed changes.