Page MenuHomePhabricator

Upgrading: Legacy "Can Edit <Field>" policies in Maniphest; requireCapabilities() in TransactionEditor
Closed, ResolvedPublic

Description

The 2018 Week 33/35 releases makes two changes which may affect policy behavior for a small subset of installs:

  • (Week 33) The Maniphest field level policies (like "Can Edit Task Status") have been removed. These were deprecated in late 2015 and have raised a setup warning since then.
    • You are affected only if you have customized these policies and ignored the setup warning for two and a half years.
    • See T10003 for details and discussion.
  • (Week 35) The PhabricatorApplicationTransactionEditor->requireCapabilities() method has been removed.
    • You are affected only if you have subclassed TransactionEditor and implemented requireCapabilities() in an extension, or installed an extension which does. This is advanced and rare.

Changes to requireCapabilities()

The PhabricatorApplicationTransactionEditor->requireCapabilities() has been removed. If you have written extensions which implement this method, this upgrade will break compatibility (they will begin throwing exceptions) and they need to be updated. The upgrade pathway here is fairly abrupt because this is an advanced (and relatively obscure) feature which very few extensions are likely to make use of, and in most cases updating is trivial (you can often just delete the method completely).

In almost all cases, users must have CAN_EDIT on the object to apply edits to it. However, there are a few cases where users need additional permissions to perform a particular edit, or can perform an edit without having CAN_EDIT permission.

An example of a normal edit is changing a task title. To change a task title, you must be able to edit the task.

An example of an edit which requires increased capabilities is locking a project. To lock a project, you must be able to edit the project and also have the "Can Lock Projects" permission.

An example of an edit which requires reduced capabilities is leaving a Conpherence thread. To leave a thread, you only need to be a member: you can always leave even if you don't have permission to edit the thread.

(There are also some special edits -- like leaving comments, subscribing, and awarding tokens -- which require CAN_INTERACT, which is weaker than CAN_EDIT, but I mention these only for completeness.)

The requireCapabilities() method was not clear about whether it was for enforcing requirements or changing requirements. Long ago it was for enforcing requirements, but it became a method about changing requirements over time. However, a lot of cruft was left in the upstream that still looked like enforcement. Generally, this turned into a big confusing mess.

The new behavior clears out all the cruft, makes it more clear that the goal is changing requirements, implements sensible and explicit defaults, and more effectively allows you to reduce/weaken/remove requirements in the context of other modern infrastructure (EditEngine, *.edit API methods).

Old Behavior

Previously, applying transactions to an object called requireCapabilities() on the Editor. This method could perform additional capability checks or (theoretically) reduce or replace the required capabilities to perform an action.

Although this method technically worked as advertised, it was misleading and many of the upstream implementations were pointless and confusing. The actual need for CAN_EDIT was effectively always enforced before the user reaches the Editor (generally, by loading the object with a CAN_EDIT check in the Controller). The upstream had a smattering of pointless explicit checks for CAN_EDIT, like those removed in D19582, which usually could never fail because they weren't reachable without CAN_EDIT.

The method could reduce required permissions, but only if control flow made the Editor reachable without CAN_EDIT permission, which was rare and surprising.

There was no actual way to reduce the permissions required for a modern Conduit API call, since the API checked CAN_EDIT unconditionally:

  • A practical issue is that you can not leave projects you can't edit via project.edit, even though you can via the web UI.
  • We're implementing a "Can Disable Users" permission as part of T13164, but there was no way to make it accessible via the API in a modern way for users who are not also administrators.

New Behavior

After upgrading to 2018 Week 34:

  • requireCapabilities() is no longer called. For safety, edits will fail (with an exception) if an Editor defines this method.
  • You should not write any code to require CAN_EDIT. Requiring CAN_EDIT is now the default behavior. If you have any code which requires CAN_EDIT, you can (and should) simply remove it.
  • If you are increasing the capabilities that a transaction requires, strongly consider testing for the additional capabilities in validation methods (probably ModularTransactionType->validateTransactions()).
  • If you are removing or replacing the capabilities that a transaction requires, implement ModularTransactionType->getRequiredCapabilities().
  • If you remove or weaken requirements, users who meet the lower barrier can now apply the edit via *.edit API methods, even without CAN_EDIT on the object.

An example of a modern increased capability is locking projects, in D19585.

An example of a modern reduced capability is leaving Conpherence threads, in D19586.

Event Timeline

epriestley triaged this task as Low priority.Aug 16 2018, 4:00 PM
epriestley created this task.
20after4 added a subscriber: 20after4.
epriestley updated the task description. (Show Details)Aug 17 2018, 3:30 PM

I'm going to push this out to next week since D19586 probably has a few minor issues with it and it's close to the release cut. It adds a lot of new policy checks which weren't explicit before, so I'd guess it may cause a few improper policy errors on things which are actually allowed. I caught a bunch of them (like "Mute Thread") but probably didn't get every single one.

epriestley updated the task description. (Show Details)Aug 18 2018, 8:10 PM
epriestley updated the task description. (Show Details)Aug 24 2018, 4:25 PM

Pushing the requireCapabilities() change out one more week since I had some stuff crop up early this week and it didn't get a chance to soak.

epriestley closed this task as Resolved.Dec 17 2018, 8:46 PM
epriestley claimed this task.

This appears to be stable and working properly. D19897 removes a straggling guardrail.