mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-23 05:50:55 +01:00
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
This commit is contained in:
parent
3da9844564
commit
d3c325c4fc
10 changed files with 142 additions and 8 deletions
|
@ -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',
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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',
|
||||
));
|
||||
}
|
||||
|
||||
|
|
11
src/applications/maniphest/engine/ManiphestTaskMFAEngine.php
Normal file
11
src/applications/maniphest/engine/ManiphestTaskMFAEngine.php
Normal file
|
@ -0,0 +1,11 @@
|
|||
<?php
|
||||
|
||||
final class ManiphestTaskMFAEngine
|
||||
extends PhabricatorEditEngineMFAEngine {
|
||||
|
||||
public function shouldRequireMFA() {
|
||||
$status = $this->getObject()->getStatus();
|
||||
return ManiphestTaskStatus::isMFAStatus($status);
|
||||
}
|
||||
|
||||
}
|
|
@ -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();
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -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);
|
||||
|
|
|
@ -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) {
|
||||
|
|
|
@ -0,0 +1,39 @@
|
|||
<?php
|
||||
|
||||
abstract class PhabricatorEditEngineMFAEngine
|
||||
extends Phobject {
|
||||
|
||||
private $object;
|
||||
private $viewer;
|
||||
|
||||
public function setObject(PhabricatorEditEngineMFAInterface $object) {
|
||||
$this->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();
|
||||
|
||||
}
|
|
@ -0,0 +1,7 @@
|
|||
<?php
|
||||
|
||||
interface PhabricatorEditEngineMFAInterface {
|
||||
|
||||
public function newEditEngineMFAEngine();
|
||||
|
||||
}
|
|
@ -953,6 +953,7 @@ abstract class PhabricatorApplicationTransactionEditor
|
|||
$this->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;
|
||||
}
|
||||
|
||||
}
|
||||
|
|
Loading…
Reference in a new issue