From d3c325c4fc7321a2411ef0c2608527ee701c041e Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 18 Dec 2018 07:01:20 -0800 Subject: [PATCH] Allow objects to be put in an "MFA required for all interactions" mode, and support "MFA required" statuses in Maniphest Summary: Depends on D19898. Ref T13222. See PHI873. Allow objects to opt into an "MFA is required for all edits" mode. Put tasks in this mode if they're in a status that specifies it is an `mfa` status. This is still a little rough for now: - There's no UI hint that you'll have to MFA. I'll likely add some hinting in a followup. - All edits currently require MFA, even subscribe/unsubscribe. We could maybe relax this if it's an issue. Test Plan: - Edited an MFA-required object via comments, edit forms, and most/all of the extensions. These prompted for MFA, then worked correctly. - Tried to edit via Conduit, failed with a reasonably comprehensible error. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13222 Differential Revision: https://secure.phabricator.com/D19899 --- src/__phutil_library_map__.php | 6 ++ .../PhabricatorManiphestConfigOptions.php | 2 + .../constants/ManiphestTaskStatus.php | 5 ++ .../engine/ManiphestTaskMFAEngine.php | 11 ++++ .../maniphest/storage/ManiphestTask.php | 11 +++- ...PhabricatorSubscriptionsEditController.php | 3 +- .../editengine/PhabricatorEditEngine.php | 2 +- .../PhabricatorEditEngineMFAEngine.php | 39 +++++++++++ .../PhabricatorEditEngineMFAInterface.php | 7 ++ ...habricatorApplicationTransactionEditor.php | 64 +++++++++++++++++-- 10 files changed, 142 insertions(+), 8 deletions(-) create mode 100644 src/applications/maniphest/engine/ManiphestTaskMFAEngine.php create mode 100644 src/applications/transactions/editengine/PhabricatorEditEngineMFAEngine.php create mode 100644 src/applications/transactions/editengine/PhabricatorEditEngineMFAInterface.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b788d5b924..552a27a321 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1731,6 +1731,7 @@ phutil_register_library_map(array( 'ManiphestTaskListController' => 'applications/maniphest/controller/ManiphestTaskListController.php', 'ManiphestTaskListHTTPParameterType' => 'applications/maniphest/httpparametertype/ManiphestTaskListHTTPParameterType.php', 'ManiphestTaskListView' => 'applications/maniphest/view/ManiphestTaskListView.php', + 'ManiphestTaskMFAEngine' => 'applications/maniphest/engine/ManiphestTaskMFAEngine.php', 'ManiphestTaskMailReceiver' => 'applications/maniphest/mail/ManiphestTaskMailReceiver.php', 'ManiphestTaskMergeInRelationship' => 'applications/maniphest/relationship/ManiphestTaskMergeInRelationship.php', 'ManiphestTaskMergedFromTransaction' => 'applications/maniphest/xaction/ManiphestTaskMergedFromTransaction.php', @@ -2977,6 +2978,8 @@ phutil_register_library_map(array( 'PhabricatorEditEngineListController' => 'applications/transactions/controller/PhabricatorEditEngineListController.php', 'PhabricatorEditEngineLock' => 'applications/transactions/editengine/PhabricatorEditEngineLock.php', 'PhabricatorEditEngineLockableInterface' => 'applications/transactions/editengine/PhabricatorEditEngineLockableInterface.php', + 'PhabricatorEditEngineMFAEngine' => 'applications/transactions/editengine/PhabricatorEditEngineMFAEngine.php', + 'PhabricatorEditEngineMFAInterface' => 'applications/transactions/editengine/PhabricatorEditEngineMFAInterface.php', 'PhabricatorEditEnginePointsCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEnginePointsCommentAction.php', 'PhabricatorEditEngineProfileMenuItem' => 'applications/search/menuitem/PhabricatorEditEngineProfileMenuItem.php', 'PhabricatorEditEngineQuery' => 'applications/transactions/query/PhabricatorEditEngineQuery.php', @@ -7298,6 +7301,7 @@ phutil_register_library_map(array( 'DoorkeeperBridgedObjectInterface', 'PhabricatorEditEngineSubtypeInterface', 'PhabricatorEditEngineLockableInterface', + 'PhabricatorEditEngineMFAInterface', ), 'ManiphestTaskAssignHeraldAction' => 'HeraldAction', 'ManiphestTaskAssignOtherHeraldAction' => 'ManiphestTaskAssignHeraldAction', @@ -7336,6 +7340,7 @@ phutil_register_library_map(array( 'ManiphestTaskListController' => 'ManiphestController', 'ManiphestTaskListHTTPParameterType' => 'AphrontListHTTPParameterType', 'ManiphestTaskListView' => 'ManiphestView', + 'ManiphestTaskMFAEngine' => 'PhabricatorEditEngineMFAEngine', 'ManiphestTaskMailReceiver' => 'PhabricatorObjectMailReceiver', 'ManiphestTaskMergeInRelationship' => 'ManiphestTaskRelationship', 'ManiphestTaskMergedFromTransaction' => 'ManiphestTaskTransactionType', @@ -8754,6 +8759,7 @@ phutil_register_library_map(array( 'PhabricatorEditEngineExtensionModule' => 'PhabricatorConfigModule', 'PhabricatorEditEngineListController' => 'PhabricatorEditEngineController', 'PhabricatorEditEngineLock' => 'Phobject', + 'PhabricatorEditEngineMFAEngine' => 'Phobject', 'PhabricatorEditEnginePointsCommentAction' => 'PhabricatorEditEngineCommentAction', 'PhabricatorEditEngineProfileMenuItem' => 'PhabricatorProfileMenuItem', 'PhabricatorEditEngineQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', diff --git a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php index 609db0d1b6..c5527aa485 100644 --- a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php +++ b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php @@ -212,6 +212,8 @@ The keys you can provide in a specification are: status. - `locked` //Optional bool.// Lock tasks in this status, preventing users from commenting. + - `mfa` //Optional bool.// Require all edits to this task to be signed with + multi-factor authentication. Statuses will appear in the UI in the order specified. Note the status marked `special` as `duplicate` is not settable directly and will not appear in UI diff --git a/src/applications/maniphest/constants/ManiphestTaskStatus.php b/src/applications/maniphest/constants/ManiphestTaskStatus.php index 53d2e1afe3..4d58816e2a 100644 --- a/src/applications/maniphest/constants/ManiphestTaskStatus.php +++ b/src/applications/maniphest/constants/ManiphestTaskStatus.php @@ -160,6 +160,10 @@ final class ManiphestTaskStatus extends ManiphestConstants { return self::getStatusAttribute($status, 'locked', false); } + public static function isMFAStatus($status) { + return self::getStatusAttribute($status, 'mfa', false); + } + public static function getStatusActionName($status) { return self::getStatusAttribute($status, 'name.action'); } @@ -282,6 +286,7 @@ final class ManiphestTaskStatus extends ManiphestConstants { 'disabled' => 'optional bool', 'claim' => 'optional bool', 'locked' => 'optional bool', + 'mfa' => 'optional bool', )); } diff --git a/src/applications/maniphest/engine/ManiphestTaskMFAEngine.php b/src/applications/maniphest/engine/ManiphestTaskMFAEngine.php new file mode 100644 index 0000000000..2aa0e303e3 --- /dev/null +++ b/src/applications/maniphest/engine/ManiphestTaskMFAEngine.php @@ -0,0 +1,11 @@ +getObject()->getStatus(); + return ManiphestTaskStatus::isMFAStatus($status); + } + +} diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 1372e2cb88..ada537fa4d 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -19,7 +19,8 @@ final class ManiphestTask extends ManiphestDAO PhabricatorFerretInterface, DoorkeeperBridgedObjectInterface, PhabricatorEditEngineSubtypeInterface, - PhabricatorEditEngineLockableInterface { + PhabricatorEditEngineLockableInterface, + PhabricatorEditEngineMFAInterface { const MARKUP_FIELD_DESCRIPTION = 'markup:desc'; @@ -619,4 +620,12 @@ final class ManiphestTask extends ManiphestDAO return new ManiphestTaskFerretEngine(); } + +/* -( PhabricatorEditEngineMFAInterface )---------------------------------- */ + + + public function newEditEngineMFAEngine() { + return new ManiphestTaskMFAEngine(); + } + } diff --git a/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php b/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php index 13a0e14c23..941c0c5811 100644 --- a/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php +++ b/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php @@ -8,7 +8,7 @@ final class PhabricatorSubscriptionsEditController $phid = $request->getURIData('phid'); $action = $request->getURIData('action'); - if (!$request->isFormPost()) { + if (!$request->isFormOrHisecPost()) { return new Aphront400Response(); } @@ -73,6 +73,7 @@ final class PhabricatorSubscriptionsEditController $editor = id($object->getApplicationTransactionEditor()) ->setActor($viewer) + ->setCancelURI($handle->getURI()) ->setContinueOnNoEffect(true) ->setContinueOnMissingFields(true) ->setContentSourceFromRequest($request); diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index e664197278..8c4a557e83 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -1041,7 +1041,7 @@ abstract class PhabricatorEditEngine } $validation_exception = null; - if ($request->isFormPost() && $request->getBool('editEngine')) { + if ($request->isFormOrHisecPost() && $request->getBool('editEngine')) { $submit_fields = $fields; foreach ($submit_fields as $key => $field) { diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineMFAEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngineMFAEngine.php new file mode 100644 index 0000000000..523672340a --- /dev/null +++ b/src/applications/transactions/editengine/PhabricatorEditEngineMFAEngine.php @@ -0,0 +1,39 @@ +object = $object; + return $this; + } + + public function getObject() { + return $this->object; + } + + public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; + return $this; + } + + public function getViewer() { + if (!$this->viewer) { + throw new PhutilInvalidStateException('setViewer'); + } + + return $this->viewer; + } + + final public static function newEngineForObject( + PhabricatorEditEngineMFAInterface $object) { + return $object->newEditEngineMFAEngine() + ->setObject($object); + } + + abstract public function shouldRequireMFA(); + +} diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineMFAInterface.php b/src/applications/transactions/editengine/PhabricatorEditEngineMFAInterface.php new file mode 100644 index 0000000000..48acb43c56 --- /dev/null +++ b/src/applications/transactions/editengine/PhabricatorEditEngineMFAInterface.php @@ -0,0 +1,7 @@ +isNewObject = ($object->getPHID() === null); $this->validateEditParameters($object, $xactions); + $xactions = $this->newMFATransactions($object, $xactions); $actor = $this->requireActor(); @@ -4825,11 +4826,22 @@ abstract class PhabricatorApplicationTransactionEditor $request = $this->getRequest(); if ($request === null) { - throw new Exception( - pht( - 'This transaction group requires MFA to apply, but the Editor was '. - 'not configured with a Request. This workflow can not perform an '. - 'MFA check.')); + $source_type = $this->getContentSource()->getSourceTypeConstant(); + $conduit_type = PhabricatorConduitContentSource::SOURCECONST; + $is_conduit = ($source_type === $conduit_type); + if ($is_conduit) { + throw new Exception( + pht( + 'This transaction group requires MFA to apply, but you can not '. + 'provide an MFA response via Conduit. Edit this object via the '. + 'web UI.')); + } else { + throw new Exception( + pht( + 'This transaction group requires MFA to apply, but the Editor was '. + 'not configured with a Request. This workflow can not perform an '. + 'MFA check.')); + } } $cancel_uri = $this->getCancelURI(); @@ -4850,4 +4862,46 @@ abstract class PhabricatorApplicationTransactionEditor } } + private function newMFATransactions( + PhabricatorLiskDAO $object, + array $xactions) { + + $is_mfa = ($object instanceof PhabricatorEditEngineMFAInterface); + if (!$is_mfa) { + return $xactions; + } + + $engine = PhabricatorEditEngineMFAEngine::newEngineForObject($object) + ->setViewer($this->getActor()); + $require_mfa = $engine->shouldRequireMFA(); + + if (!$require_mfa) { + return $xactions; + } + + $type_mfa = PhabricatorTransactions::TYPE_MFA; + + $has_mfa = false; + foreach ($xactions as $xaction) { + if ($xaction->getTransactionType() === $type_mfa) { + $has_mfa = true; + break; + } + } + + if ($has_mfa) { + return $xactions; + } + + $template = $object->getApplicationTransactionTemplate(); + + $mfa_xaction = id(clone $template) + ->setTransactionType($type_mfa) + ->setNewValue(true); + + array_unshift($xactions, $mfa_xaction); + + return $xactions; + } + }