Allow task statuses to specify that either "comments" or "edits" are "locked"

Summary:
Ref T13249. See PHI1059. This allows "locked" in `maniphest.statuses` to specify that either "comments" are locked (current behavior, advisory, overridable by users with edit permission, e.g. for calming discussion on a contentious issue or putting a guard rail on things); or "edits" are locked (hard lock, only task owner can edit things).

Roughly, "comments" is a soft/advisory lock. "edits" is a hard/strict lock. (I think both types of locks have reasonable use cases, which is why I'm not just making locks stronger across the board.)

When "edits" are locked:

  - The edit policy looks like "no one" to normal callers.
  - In one special case, we sneak the real value through a back channel using PolicyCodex in the specific narrow case that you're editing the object. Otherwise, the policy selector control incorrectly switches to "No One".
  - We also have to do a little more validation around applying a mixture of status + owner transactions that could leave the task uneditable.

For now, I'm allowing you to reassign a hard-locked task to someone else. If you get this wrong, we can end up in a state where no one can edit the task. If this is an issue, we could respond in various ways: prevent these edits; prevent assigning to disabled users; provide a `bin/task reassign`; uh maybe have a quorum convene?

Test Plan:
  - Defined "Soft Locked" and "Hard Locked" statues.
  - "Hard Locked" a task, hit errors (trying to unassign myself, trying to hard lock an unassigned task).
  - Saw nice new policy guidance icon in header.

{F6210362}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20165

resources/celerity/map.php

@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => '3c8a0668',
     'conpherence.pkg.js' => '020aebcf',
-    'core.pkg.css' => '85a1da99',
+    'core.pkg.css' => 'f2319e1f',
     'core.pkg.js' => '5c737607',
     'differential.pkg.css' => 'b8df73d4',
     'differential.pkg.js' => '67c9ea4c',
@@ -154,7 +154,7 @@ return array(
     'rsrc/css/phui/phui-form-view.css' => '01b796c0',
     'rsrc/css/phui/phui-form.css' => '159e2d9c',
     'rsrc/css/phui/phui-head-thing.css' => 'd7f293df',
-    'rsrc/css/phui/phui-header-view.css' => '93cea4ec',
+    'rsrc/css/phui/phui-header-view.css' => '285c9139',
     'rsrc/css/phui/phui-hovercard.css' => '6ca90fa0',
     'rsrc/css/phui/phui-icon-set-selector.css' => '7aa5f3ec',
     'rsrc/css/phui/phui-icon.css' => '4cbc684a',
@@ -821,7 +821,7 @@ return array(
     'phui-form-css' => '159e2d9c',
     'phui-form-view-css' => '01b796c0',
     'phui-head-thing-view-css' => 'd7f293df',
-    'phui-header-view-css' => '93cea4ec',
+    'phui-header-view-css' => '285c9139',
     'phui-hovercard' => '074f0783',
     'phui-hovercard-view-css' => '6ca90fa0',
     'phui-icon-set-selector-css' => '7aa5f3ec',

src/__phutil_library_map__.php

@@ -1743,6 +1743,7 @@ phutil_register_library_map(array(
     'ManiphestTaskParentTransaction' => 'applications/maniphest/xaction/ManiphestTaskParentTransaction.php',
     'ManiphestTaskPoints' => 'applications/maniphest/constants/ManiphestTaskPoints.php',
     'ManiphestTaskPointsTransaction' => 'applications/maniphest/xaction/ManiphestTaskPointsTransaction.php',
+    'ManiphestTaskPolicyCodex' => 'applications/maniphest/policy/ManiphestTaskPolicyCodex.php',
     'ManiphestTaskPriority' => 'applications/maniphest/constants/ManiphestTaskPriority.php',
     'ManiphestTaskPriorityDatasource' => 'applications/maniphest/typeahead/ManiphestTaskPriorityDatasource.php',
     'ManiphestTaskPriorityHeraldAction' => 'applications/maniphest/herald/ManiphestTaskPriorityHeraldAction.php',
@@ -7384,6 +7385,7 @@ phutil_register_library_map(array(
       'PhabricatorEditEngineSubtypeInterface',
       'PhabricatorEditEngineLockableInterface',
       'PhabricatorEditEngineMFAInterface',
+      'PhabricatorPolicyCodexInterface',
     ),
     'ManiphestTaskAssignHeraldAction' => 'HeraldAction',
     'ManiphestTaskAssignOtherHeraldAction' => 'ManiphestTaskAssignHeraldAction',
@@ -7435,6 +7437,7 @@ phutil_register_library_map(array(
     'ManiphestTaskParentTransaction' => 'ManiphestTaskTransactionType',
     'ManiphestTaskPoints' => 'Phobject',
     'ManiphestTaskPointsTransaction' => 'ManiphestTaskTransactionType',
+    'ManiphestTaskPolicyCodex' => 'PhabricatorPolicyCodex',
     'ManiphestTaskPriority' => 'ManiphestConstants',
     'ManiphestTaskPriorityDatasource' => 'PhabricatorTypeaheadDatasource',
     'ManiphestTaskPriorityHeraldAction' => 'HeraldAction',

src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php

@@ -210,8 +210,9 @@ The keys you can provide in a specification are:
   - `claim` //Optional bool.// By default, closing an unassigned task claims
     it. You can set this to `false` to disable this behavior for a particular
     status.
-  - `locked` //Optional bool.// Lock tasks in this status, preventing users
-    from commenting.
+  - `locked` //Optional string.// Lock tasks in this status. Specify "comments"
+    to lock comments (users who can edit the task may override this lock).
+    Specify "edits" to prevent anyone except the task owner from making edits.
   - `mfa` //Optional bool.// Require all edits to this task to be signed with
     multi-factor authentication.
 

src/applications/maniphest/constants/ManiphestTaskStatus.php

@@ -16,6 +16,9 @@ final class ManiphestTaskStatus extends ManiphestConstants {
   const SPECIAL_CLOSED      = 'closed';
   const SPECIAL_DUPLICATE   = 'duplicate';
 
+  const LOCKED_COMMENTS = 'comments';
+  const LOCKED_EDITS = 'edits';
+
   private static function getStatusConfig() {
     return PhabricatorEnv::getEnvConfig('maniphest.statuses');
   }
@@ -156,8 +159,13 @@ final class ManiphestTaskStatus extends ManiphestConstants {
     return !self::isOpenStatus($status);
   }
 
-  public static function isLockedStatus($status) {
-    return self::getStatusAttribute($status, 'locked', false);
+  public static function areCommentsLockedInStatus($status) {
+    return (bool)self::getStatusAttribute($status, 'locked', false);
+  }
+
+  public static function areEditsLockedInStatus($status) {
+    $locked = self::getStatusAttribute($status, 'locked');
+    return ($locked === self::LOCKED_EDITS);
   }
 
   public static function isMFAStatus($status) {
@@ -285,11 +293,35 @@ final class ManiphestTaskStatus extends ManiphestConstants {
           'keywords' => 'optional list<string>',
           'disabled' => 'optional bool',
           'claim' => 'optional bool',
-          'locked' => 'optional bool',
+          'locked' => 'optional bool|string',
           'mfa' => 'optional bool',
         ));
     }
 
+    // Supported values are "comments" or "edits". For backward compatibility,
+    // "true" is an alias of "comments".
+
+    foreach ($config as $key => $value) {
+      $locked = idx($value, 'locked', false);
+      if ($locked === true || $locked === false) {
+        continue;
+      }
+
+      if ($locked === self::LOCKED_EDITS ||
+          $locked === self::LOCKED_COMMENTS) {
+        continue;
+      }
+
+      throw new Exception(
+        pht(
+          'Task status ("%s") has unrecognized value for "locked" '.
+          'configuration ("%s"). Supported values are: "%s", "%s".',
+          $key,
+          $locked,
+          self::LOCKED_COMMENTS,
+          self::LOCKED_EDITS));
+    }
+
     $special_map = array();
     foreach ($config as $key => $value) {
       $special = idx($value, 'special');

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -552,6 +552,10 @@ final class ManiphestTransactionEditor
       $errors = array_merge($errors, $this->moreValidationErrors);
     }
 
+    foreach ($this->getLockValidationErrors($object, $xactions) as $error) {
+      $errors[] = $error;
+    }
+
     return $errors;
   }
 
@@ -1011,5 +1015,86 @@ final class ManiphestTransactionEditor
   }
 
 
+  private function getLockValidationErrors($object, array $xactions) {
+    $errors = array();
+
+    $old_owner = $object->getOwnerPHID();
+    $old_status = $object->getStatus();
+
+    $new_owner = $old_owner;
+    $new_status = $old_status;
+
+    $owner_xaction = null;
+    $status_xaction = null;
+
+    foreach ($xactions as $xaction) {
+      switch ($xaction->getTransactionType()) {
+        case ManiphestTaskOwnerTransaction::TRANSACTIONTYPE:
+          $new_owner = $xaction->getNewValue();
+          $owner_xaction = $xaction;
+          break;
+        case ManiphestTaskStatusTransaction::TRANSACTIONTYPE:
+          $new_status = $xaction->getNewValue();
+          $status_xaction = $xaction;
+          break;
+      }
+    }
+
+    $actor_phid = $this->getActingAsPHID();
+
+    $was_locked = ManiphestTaskStatus::areEditsLockedInStatus(
+      $old_status);
+    $now_locked = ManiphestTaskStatus::areEditsLockedInStatus(
+      $new_status);
+
+    if (!$now_locked) {
+      // If we're not ending in an edit-locked status, everything is good.
+    } else if ($new_owner !== null) {
+      // If we ending the edit with some valid owner, this is allowed for
+      // now. We might need to revisit this.
+    } else {
+      // The edits end with the task locked and unowned. No one will be able
+      // to edit it, so we forbid this. We try to be specific about what the
+      // user did wrong.
+
+      $owner_changed = ($old_owner && !$new_owner);
+      $status_changed = ($was_locked !== $now_locked);
+      $message = null;
+
+      if ($status_changed && $owner_changed) {
+        $message = pht(
+          'You can not lock this task and unassign it at the same time '.
+          'because no one will be able to edit it anymore. Lock the task '.
+          'or remove the owner, but not both.');
+        $problem_xaction = $status_xaction;
+      } else if ($status_changed) {
+        $message = pht(
+          'You can not lock this task because it does not have an owner. '.
+          'No one would be able to edit the task. Assign the task to an '.
+          'owner before locking it.');
+        $problem_xaction = $status_xaction;
+      } else if ($owner_changed) {
+        $message = pht(
+          'You can not remove the owner of this task because it is locked '.
+          'and no one would be able to edit the task. Reassign the task or '.
+          'unlock it before removing the owner.');
+        $problem_xaction = $owner_xaction;
+      } else {
+        // If the task was already broken, we don't have a transaction to
+        // complain about so just let it through. In theory, this is
+        // impossible since policy rules should kick in before we get here.
+      }
+
+      if ($message) {
+        $errors[] = new PhabricatorApplicationTransactionValidationError(
+          $problem_xaction->getTransactionType(),
+          pht('Lock Error'),
+          $message,
+          $problem_xaction);
+      }
+    }
+
+    return $errors;
+  }
 
 }

src/applications/maniphest/policy/ManiphestTaskPolicyCodex.php

@@ -0,0 +1,70 @@
+<?php
+
+final class ManiphestTaskPolicyCodex
+  extends PhabricatorPolicyCodex {
+
+  public function getPolicyShortName() {
+    $object = $this->getObject();
+
+    if ($object->areEditsLocked()) {
+      return pht('Edits Locked');
+    }
+
+    return null;
+  }
+
+  public function getPolicyIcon() {
+    $object = $this->getObject();
+
+    if ($object->areEditsLocked()) {
+      return 'fa-lock';
+    }
+
+    return null;
+  }
+
+  public function getPolicyTagClasses() {
+    $object = $this->getObject();
+    $classes = array();
+
+    if ($object->areEditsLocked()) {
+      $classes[] = 'policy-adjusted-locked';
+    }
+
+    return $classes;
+  }
+
+  public function getPolicySpecialRuleDescriptions() {
+    $object = $this->getObject();
+
+    $rules = array();
+
+    $rules[] = $this->newRule()
+      ->setCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->setIsActive($object->areEditsLocked())
+      ->setDescription(
+        pht(
+          'Tasks with edits locked may only be edited by their owner.'));
+
+    return $rules;
+  }
+
+  public function getPolicyForEdit($capability) {
+
+    // When a task has its edits locked, the effective edit policy is locked
+    // to "No One". However, the task owner may still bypass the lock and edit
+    // the task. When they do, we want the control in the UI to have the
+    // correct value. Return the real value stored on the object.
+
+    switch ($capability) {
+      case PhabricatorPolicyCapability::CAN_EDIT:
+        return $this->getObject()->getEditPolicy();
+    }
+
+    return parent::getPolicyForEdit($capability);
+  }
+
+}

src/applications/maniphest/storage/ManiphestTask.php

@@ -20,7 +20,8 @@ final class ManiphestTask extends ManiphestDAO
     DoorkeeperBridgedObjectInterface,
     PhabricatorEditEngineSubtypeInterface,
     PhabricatorEditEngineLockableInterface,
-    PhabricatorEditEngineMFAInterface {
+    PhabricatorEditEngineMFAInterface,
+    PhabricatorPolicyCodexInterface {
 
   const MARKUP_FIELD_DESCRIPTION = 'markup:desc';
 
@@ -217,8 +218,16 @@ final class ManiphestTask extends ManiphestDAO
     return ManiphestTaskStatus::isClosedStatus($this->getStatus());
   }
 
-  public function isLocked() {
-    return ManiphestTaskStatus::isLockedStatus($this->getStatus());
+  public function areCommentsLocked() {
+    if ($this->areEditsLocked()) {
+      return true;
+    }
+
+    return ManiphestTaskStatus::areCommentsLockedInStatus($this->getStatus());
+  }
+
+  public function areEditsLocked() {
+    return ManiphestTaskStatus::areEditsLockedInStatus($this->getStatus());
   }
 
   public function setProperty($key, $value) {
@@ -371,13 +380,17 @@ final class ManiphestTask extends ManiphestDAO
       case PhabricatorPolicyCapability::CAN_VIEW:
         return $this->getViewPolicy();
       case PhabricatorPolicyCapability::CAN_INTERACT:
-        if ($this->isLocked()) {
+        if ($this->areCommentsLocked()) {
           return PhabricatorPolicies::POLICY_NOONE;
         } else {
           return $this->getViewPolicy();
         }
       case PhabricatorPolicyCapability::CAN_EDIT:
-        return $this->getEditPolicy();
+        if ($this->areEditsLocked()) {
+          return PhabricatorPolicies::POLICY_NOONE;
+        } else {
+          return $this->getEditPolicy();
+        }
     }
   }
 
@@ -628,4 +641,12 @@ final class ManiphestTask extends ManiphestDAO
     return new ManiphestTaskMFAEngine();
   }
 
+
+/* -(  PhabricatorPolicyCodexInterface  )------------------------------------ */
+
+
+  public function newPolicyCodex() {
+    return new ManiphestTaskPolicyCodex();
+  }
+
 }

src/applications/policy/codex/PhabricatorPolicyCodex.php

@@ -29,6 +29,10 @@ abstract class PhabricatorPolicyCodex
     return array();
   }
 
+  public function getPolicyForEdit($capability) {
+    return $this->getObject()->getPolicy($capability);
+  }
+
   public function getDefaultPolicy() {
     return PhabricatorPolicyQuery::getDefaultPolicyForObject(
       $this->viewer,

src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php

@@ -68,6 +68,14 @@ final class PhabricatorPolicyEditEngineExtension
       ),
     );
 
+    if ($object instanceof PhabricatorPolicyCodexInterface) {
+      $codex = PhabricatorPolicyCodex::newFromObject(
+        $object,
+        $viewer);
+    } else {
+      $codex = null;
+    }
+
     $fields = array();
     foreach ($map as $type => $spec) {
       if (empty($types[$type])) {
@@ -82,6 +90,18 @@ final class PhabricatorPolicyEditEngineExtension
       $conduit_description = $spec['description.conduit'];
       $edit = $spec['edit'];
 
+      // Objects may present a policy value to the edit workflow that is
+      // different from their nominal policy value: for example, when tasks
+      // are locked, they appear as "Editable By: No One" to other applications
+      // but we still want to edit the actual policy stored in the database
+      // when we show the user a form with a policy control in it.
+
+      if ($codex) {
+        $policy_value = $codex->getPolicyForEdit($capability);
+      } else {
+        $policy_value = $object->getPolicy($capability);
+      }
+
       $policy_field = id(new PhabricatorPolicyEditField())
         ->setKey($key)
         ->setLabel($label)
@@ -94,7 +114,7 @@ final class PhabricatorPolicyEditEngineExtension
         ->setDescription($description)
         ->setConduitDescription($conduit_description)
         ->setConduitTypeDescription(pht('New policy PHID or constant.'))
-        ->setValue($object->getPolicy($capability));
+        ->setValue($policy_value);
       $fields[] = $policy_field;
 
       if ($object instanceof PhabricatorSpacesInterface) {

webroot/rsrc/css/phui/phui-header-view.css

@@ -249,6 +249,16 @@ body .phui-header-shell.phui-bleed-header
   color: {$sh-indigotext};
 }
 
+.policy-header-callout.policy-adjusted-locked {
+  background: {$sh-pinkbackground};
+}
+
+.policy-header-callout.policy-adjusted-locked .policy-link,
+.policy-header-callout.policy-adjusted-locked .phui-icon-view {
+  color: {$sh-pinktext};
+}
+
+
 .policy-header-callout .policy-space-container {
   font-weight: bold;
   color: {$sh-redtext};