Page MenuHomePhabricator

Explore restricting editing task priority to Trusted-Contributors/staff/etc
Open, MediumPublic

Description

cf. T133541: [RFC] Limit option (or provide alternative) to change the priority of tickets to project members or T819: Restricting modification of tasks when they enter sprints (both not exactly the same but closely related).

Folks changing a task's Priority value (often without bad intentions but "the field was visible so I thought I can") regularly comes up as an issue faced by teams whose workflows rely on the Priority field.

Upstream Phorge (Phabricator) code itself has no further differentiation in the Maniphest (=tasks) application settings: The default Maniphest Edit Policy covers all fields of a task - Priority field and any other fields.
To some extent this was mitigated by the introduction of forms allowing to disable or hide specific UI fields, which however still does not solve the situation as a (Create or Edit) form configuration cannot be bound to a specific user group - it's all users or no users affected by the form configuration, once a task has been created via a form.

I was quite reluctant in the past to restrict editing the Priority field. After having seen vandalism (though that's only weakly related here) and repeated tension over the years I've started to consider this an option.
It would slightly reduce UI complexity for Phab newcomers not aware of its social conventions.
It would reduce workflow collisions, see e.g. https://phabricator.wikimedia.org/T14396#9680109 or https://phabricator.wikimedia.org/T362986#9730145 or https://phabricator.wikimedia.org/T365323 or https://phabricator.wikimedia.org/T365315#9811246 (random latest examples).

Details

TitleReferenceAuthorSource BranchDest Branch
Draft: Expose the Edit Task Priority dropdown field only to trusted usersrepos/phabricator/phabricator!50aklapperT363337editPrioHidewmf/stable
Customize query in GitLab

Event Timeline

Aklapper triaged this task as Medium priority.EditedApr 24 2024, 1:47 PM
Aklapper created this task.

Proof of concept:

1diff --git a/src/applications/maniphest/editor/ManiphestEditEngine.php b/src/applications/maniphest/editor/ManiphestEditEngine.php
2index 46877168e7..ed83650e1b 100644
3--- a/src/applications/maniphest/editor/ManiphestEditEngine.php
4+++ b/src/applications/maniphest/editor/ManiphestEditEngine.php
5@@ -145,6 +145,38 @@ EODOCS
6
7 $column_map = $this->getColumnMap($object);
8
9+// WMF BEGIN: Only show Edit Priority when user is member of a trusted project
10+ $can_edit_priority = false;
11+ $viewer = $this->getViewer();
12+ $viewer_phid = $viewer->getPHID();
13+
14+ // ["Trusted-Contributors", "WMF-NDA", "acl*sre-team", "acl*security"];
15+ $trusted_project_phids =
16+ ['PHID-PROJ-rte2hubdntrhiwyoau6n', 'PHID-PROJ-ibxm3v6ithf3jpqpqhl7',
17+ 'PHID-PROJ-fqb3bhereyljcqrsbju7', 'PHID-PROJ-koo4qqdng27q7r65x3cw'];
18+ // PhabricatorProjectQuery does not seem to `&&` for withPHIDs and
19+ // withMemberPHIDs for a nice shortcut, so loop on `isUserMember()` later
20+ $trusted_projects = id(new PhabricatorProjectQuery())
21+ ->setViewer($viewer)
22+ ->withPHIDs($trusted_project_phids)
23+ ->execute();
24+ if (count($trusted_projects) !== 4) {
25+ phlog('WMF Error: Not all projects for checking Edit Priority ' .
26+ 'rights exist. Please correct the PHIDs in the code.');
27+ }
28+ if ($trusted_projects) {
29+ foreach ($trusted_projects as $proj) {
30+ if ($proj instanceof PhabricatorProject &&
31+ $proj->isUserMember($viewer_phid)) {
32+ $can_edit_priority = true;
33+ break;
34+ }
35+ }
36+ } else { // allow to edit priority if no $trusted_projects found AT ALL
37+ $can_edit_priority = true;
38+ }
39+// WMF END: Only show Edit Priority when user is member of a trusted project
40+
41 $fields = array(
42 id(new PhabricatorHandlesEditField())
43 ->setKey('parent')
44@@ -225,6 +257,7 @@ EODOCS
45 ->setIsCopyable(true)
46 ->setValue($object->getPriorityKeyword())
47 ->setOptions($priority_map)
48+ ->setIsHidden(!$can_edit_priority)
49 ->setOptionAliases($alias_map)
50 ->setCommentActionLabel(pht('Change Priority')),
51 );

(A "proper" implementation would span across at least 4 or 5 source code files and I don't consider this request acceptable by upstream, so I prefer to keep custom downstream changes quite local. Details for myself: Properly implementing a Capacity would require expanding ManiphestTask.php to have a setPriorityEditPolicy() method, expand getCustomCapabilities() in PhabricatorManiphestApplication.php, introduce a class ManiphestDefaultPriorityEditCapability.php, etc etc).

Pppery rescinded a token.
Pppery awarded a token.

Notes to myself:

  • Tested task creation as average user. Works as expected: Priority field not shown in the UI and task successfully created wth default priority.
  • This code change does not affect bulk editing: The "Set Priority" option is still available as a bulk action even though the performing user is not a member of one of the four projects. That's fine IMO as bulk edits are limited to acl*Batch-Editors members anyway.

Would this mean that people like me couldn’t put Unbreak Now! priority to the tasks that require it?

It would mean that only members of Trusted-Contributors, WMF-NDA acl*sre-team or acl*security can change the Priority field value.

It would mean that only members of Trusted-Contributors, WMF-NDA acl*sre-team or acl*security can change the Priority field value.

I hope this would mean that addition to Trusted-Contributors would be more active then because this would effectively hinder ability of many less active Phabricator contributors to report the urgent tasks (I was added to the group but I assume many others, even admins from different wikis, will be forgotten).