diff --git a/resources/celerity/map.php b/resources/celerity/map.php --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ '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 @@ '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 @@ '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', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1743,6 +1743,7 @@ '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 @@ 'PhabricatorEditEngineSubtypeInterface', 'PhabricatorEditEngineLockableInterface', 'PhabricatorEditEngineMFAInterface', + 'PhabricatorPolicyCodexInterface', ), 'ManiphestTaskAssignHeraldAction' => 'HeraldAction', 'ManiphestTaskAssignOtherHeraldAction' => 'ManiphestTaskAssignHeraldAction', @@ -7435,6 +7437,7 @@ 'ManiphestTaskParentTransaction' => 'ManiphestTaskTransactionType', 'ManiphestTaskPoints' => 'Phobject', 'ManiphestTaskPointsTransaction' => 'ManiphestTaskTransactionType', + 'ManiphestTaskPolicyCodex' => 'PhabricatorPolicyCodex', 'ManiphestTaskPriority' => 'ManiphestConstants', 'ManiphestTaskPriorityDatasource' => 'PhabricatorTypeaheadDatasource', 'ManiphestTaskPriorityHeraldAction' => 'HeraldAction', diff --git a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php --- a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php +++ b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php @@ -210,8 +210,9 @@ - `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. diff --git a/src/applications/maniphest/constants/ManiphestTaskStatus.php b/src/applications/maniphest/constants/ManiphestTaskStatus.php --- a/src/applications/maniphest/constants/ManiphestTaskStatus.php +++ b/src/applications/maniphest/constants/ManiphestTaskStatus.php @@ -16,6 +16,9 @@ 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 @@ 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 @@ 'keywords' => 'optional list', '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'); diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -552,6 +552,10 @@ $errors = array_merge($errors, $this->moreValidationErrors); } + foreach ($this->getLockValidationErrors($object, $xactions) as $error) { + $errors[] = $error; + } + return $errors; } @@ -1011,5 +1015,86 @@ } + 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; + } } diff --git a/src/applications/maniphest/policy/ManiphestTaskPolicyCodex.php b/src/applications/maniphest/policy/ManiphestTaskPolicyCodex.php new file mode 100644 --- /dev/null +++ b/src/applications/maniphest/policy/ManiphestTaskPolicyCodex.php @@ -0,0 +1,70 @@ +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); + } + +} diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -20,7 +20,8 @@ DoorkeeperBridgedObjectInterface, PhabricatorEditEngineSubtypeInterface, PhabricatorEditEngineLockableInterface, - PhabricatorEditEngineMFAInterface { + PhabricatorEditEngineMFAInterface, + PhabricatorPolicyCodexInterface { const MARKUP_FIELD_DESCRIPTION = 'markup:desc'; @@ -217,8 +218,16 @@ 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 @@ 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 @@ return new ManiphestTaskMFAEngine(); } + +/* -( PhabricatorPolicyCodexInterface )------------------------------------ */ + + + public function newPolicyCodex() { + return new ManiphestTaskPolicyCodex(); + } + } diff --git a/src/applications/policy/codex/PhabricatorPolicyCodex.php b/src/applications/policy/codex/PhabricatorPolicyCodex.php --- a/src/applications/policy/codex/PhabricatorPolicyCodex.php +++ b/src/applications/policy/codex/PhabricatorPolicyCodex.php @@ -29,6 +29,10 @@ return array(); } + public function getPolicyForEdit($capability) { + return $this->getObject()->getPolicy($capability); + } + public function getDefaultPolicy() { return PhabricatorPolicyQuery::getDefaultPolicyForObject( $this->viewer, diff --git a/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php b/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php --- a/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php +++ b/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php @@ -68,6 +68,14 @@ ), ); + 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 @@ $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 @@ ->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) { diff --git a/webroot/rsrc/css/phui/phui-header-view.css b/webroot/rsrc/css/phui/phui-header-view.css --- a/webroot/rsrc/css/phui/phui-header-view.css +++ b/webroot/rsrc/css/phui/phui-header-view.css @@ -249,6 +249,16 @@ 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};