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 @@ -2231,6 +2231,7 @@ 'PhabricatorAuthLoginController' => 'applications/auth/controller/PhabricatorAuthLoginController.php', 'PhabricatorAuthLoginHandler' => 'applications/auth/handler/PhabricatorAuthLoginHandler.php', 'PhabricatorAuthLogoutConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthLogoutConduitAPIMethod.php', + 'PhabricatorAuthMFAEditEngineExtension' => 'applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php', 'PhabricatorAuthMainMenuBarExtension' => 'applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php', 'PhabricatorAuthManagementCachePKCS8Workflow' => 'applications/auth/management/PhabricatorAuthManagementCachePKCS8Workflow.php', 'PhabricatorAuthManagementLDAPWorkflow' => 'applications/auth/management/PhabricatorAuthManagementLDAPWorkflow.php', @@ -7885,6 +7886,7 @@ 'PhabricatorAuthLoginController' => 'PhabricatorAuthController', 'PhabricatorAuthLoginHandler' => 'Phobject', 'PhabricatorAuthLogoutConduitAPIMethod' => 'PhabricatorAuthConduitAPIMethod', + 'PhabricatorAuthMFAEditEngineExtension' => 'PhabricatorEditEngineExtension', 'PhabricatorAuthMainMenuBarExtension' => 'PhabricatorMainMenuBarExtension', 'PhabricatorAuthManagementCachePKCS8Workflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementLDAPWorkflow' => 'PhabricatorAuthManagementWorkflow', diff --git a/src/applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php b/src/applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php @@ -0,0 +1,52 @@ +getViewer(); + + $mfa_field = id(new PhabricatorApplyEditField()) + ->setViewer($viewer) + ->setKey(self::FIELDKEY) + ->setLabel(pht('MFA')) + ->setIsFormField(false) + ->setCommentActionLabel(pht('Sign With MFA')) + ->setCommentActionOrder(12000) + ->setActionDescription( + pht('You will be prompted to provide MFA when you submit.')) + ->setDescription(pht('Sign this transaction group with MFA.')) + ->setTransactionType($mfa_type); + + return array( + $mfa_field, + ); + } + +} diff --git a/src/applications/transactions/constants/PhabricatorTransactions.php b/src/applications/transactions/constants/PhabricatorTransactions.php --- a/src/applications/transactions/constants/PhabricatorTransactions.php +++ b/src/applications/transactions/constants/PhabricatorTransactions.php @@ -16,6 +16,7 @@ const TYPE_COLUMNS = 'core:columns'; const TYPE_SUBTYPE = 'core:subtype'; const TYPE_HISTORY = 'core:history'; + const TYPE_MFA = 'core:mfa'; const COLOR_RED = 'red'; const COLOR_ORANGE = 'orange'; diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -1105,6 +1105,7 @@ $editor = $object->getApplicationTransactionEditor() ->setActor($viewer) ->setContentSourceFromRequest($request) + ->setCancelURI($cancel_uri) ->setContinueOnNoEffect(true); try { @@ -1785,7 +1786,9 @@ $controller = $this->getController(); $request = $controller->getRequest(); - if (!$request->isFormPost()) { + // NOTE: We handle hisec inside the transaction editor with "Sign With MFA" + // comment actions. + if (!$request->isFormOrHisecPost()) { return new Aphront400Response(); } @@ -1919,6 +1922,7 @@ ->setContinueOnNoEffect($request->isContinueRequest()) ->setContinueOnMissingFields(true) ->setContentSourceFromRequest($request) + ->setCancelURI($view_uri) ->setRaiseWarnings(!$request->getBool('editEngine.warnings')) ->setIsPreview($is_preview); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -84,6 +84,10 @@ private $transactionQueue = array(); private $sendHistory = false; + private $shouldRequireMFA = false; + private $hasRequiredMFA = false; + private $request; + private $cancelURI; const STORAGE_ENCODING_BINARY = 'binary'; @@ -284,6 +288,22 @@ return $this->raiseWarnings; } + public function setShouldRequireMFA($should_require_mfa) { + if ($this->hasRequiredMFA) { + throw new Exception( + pht( + 'Call to setShouldRequireMFA() is too late: this Editor has already '. + 'checked for MFA requirements.')); + } + + $this->shouldRequireMFA = $should_require_mfa; + return $this; + } + + public function getShouldRequireMFA() { + return $this->shouldRequireMFA; + } + public function getTransactionTypesForObject($object) { $old = $this->object; try { @@ -328,6 +348,8 @@ $types[] = PhabricatorTransactions::TYPE_SPACE; } + $types[] = PhabricatorTransactions::TYPE_MFA; + $template = $this->object->getApplicationTransactionTemplate(); if ($template instanceof PhabricatorModularTransaction) { $xtypes = $template->newModularTransactionTypes(); @@ -383,6 +405,8 @@ return null; case PhabricatorTransactions::TYPE_SUBTYPE: return $object->getEditEngineSubtype(); + case PhabricatorTransactions::TYPE_MFA: + return null; case PhabricatorTransactions::TYPE_SUBSCRIBERS: return array_values($this->subscribers); case PhabricatorTransactions::TYPE_VIEW_POLICY: @@ -473,6 +497,8 @@ case PhabricatorTransactions::TYPE_SUBTYPE: case PhabricatorTransactions::TYPE_HISTORY: return $xaction->getNewValue(); + case PhabricatorTransactions::TYPE_MFA: + return true; case PhabricatorTransactions::TYPE_SPACE: $space_phid = $xaction->getNewValue(); if (!strlen($space_phid)) { @@ -611,6 +637,7 @@ case PhabricatorTransactions::TYPE_CREATE: case PhabricatorTransactions::TYPE_HISTORY: case PhabricatorTransactions::TYPE_SUBTYPE: + case PhabricatorTransactions::TYPE_MFA: case PhabricatorTransactions::TYPE_TOKEN: case PhabricatorTransactions::TYPE_VIEW_POLICY: case PhabricatorTransactions::TYPE_EDIT_POLICY: @@ -673,6 +700,7 @@ case PhabricatorTransactions::TYPE_CREATE: case PhabricatorTransactions::TYPE_HISTORY: case PhabricatorTransactions::TYPE_SUBTYPE: + case PhabricatorTransactions::TYPE_MFA: case PhabricatorTransactions::TYPE_EDGE: case PhabricatorTransactions::TYPE_TOKEN: case PhabricatorTransactions::TYPE_VIEW_POLICY: @@ -850,10 +878,6 @@ $xaction->setIsSilentTransaction(true); } - if ($actor->hasHighSecuritySession()) { - $xaction->setIsMFATransaction(true); - } - return $xaction; } @@ -893,6 +917,7 @@ } public function setContentSourceFromRequest(AphrontRequest $request) { + $this->setRequest($request); return $this->setContentSource( PhabricatorContentSource::newFromRequest($request)); } @@ -901,6 +926,24 @@ return $this->contentSource; } + public function setRequest(AphrontRequest $request) { + $this->request = $request; + return $this; + } + + public function getRequest() { + return $this->request; + } + + public function setCancelURI($cancel_uri) { + $this->cancelURI = $cancel_uri; + return $this; + } + + public function getCancelURI() { + return $this->cancelURI; + } + final public function applyTransactions( PhabricatorLiskDAO $object, array $xactions) { @@ -966,9 +1009,29 @@ $warnings); } } + } + + foreach ($xactions as $xaction) { + $this->adjustTransactionValues($object, $xaction); + } + + // Now that we've merged and combined transactions, check for required + // capabilities. Note that we're doing this before filtering + // transactions: if you try to apply an edit which you do not have + // permission to apply, we want to give you a permissions error even + // if the edit would have no effect. + $this->applyCapabilityChecks($object, $xactions); + $xactions = $this->filterTransactions($object, $xactions); + + if (!$is_preview) { $this->willApplyTransactions($object, $xactions); + $this->hasRequiredMFA = true; + if ($this->getShouldRequireMFA()) { + $this->requireMFA($object, $xactions); + } + if ($object->getID()) { $this->buildOldRecipientLists($object, $xactions); @@ -994,33 +1057,6 @@ $this->applyInitialEffects($object, $xactions); } - foreach ($xactions as $xaction) { - $this->adjustTransactionValues($object, $xaction); - } - - // Now that we've merged and combined transactions, check for required - // capabilities. Note that we're doing this before filtering - // transactions: if you try to apply an edit which you do not have - // permission to apply, we want to give you a permissions error even - // if the edit would have no effect. - $this->applyCapabilityChecks($object, $xactions); - - // See T13186. Fatal hard if this object has an older - // "requireCapabilities()" method. The code may rely on this method being - // called to apply policy checks, so err on the side of safety and fatal. - // TODO: Remove this check after some time has passed. - if (method_exists($this, 'requireCapabilities')) { - throw new Exception( - pht( - 'Editor (of class "%s") implements obsolete policy method '. - 'requireCapabilities(). The implementation for this Editor '. - 'MUST be updated. See <%s> for discussion.', - get_class($this), - 'https://secure.phabricator.com/T13186')); - } - - $xactions = $this->filterTransactions($object, $xactions); - // TODO: Once everything is on EditEngine, just use getIsNewObject() to // figure this out instead. $mark_as_create = false; @@ -1580,6 +1616,10 @@ // email and is only partially supported in the upstream. You don't // need any capabilities to apply it. return null; + case PhabricatorTransactions::TYPE_MFA: + // Signing a transaction group with MFA does not require permissions + // on its own. + return null; case PhabricatorTransactions::TYPE_EDGE: return $this->getLegacyRequiredEdgeCapabilities($xaction); default: @@ -2272,11 +2312,19 @@ array $xactions) { $type_comment = PhabricatorTransactions::TYPE_COMMENT; + $type_mfa = PhabricatorTransactions::TYPE_MFA; $no_effect = array(); $has_comment = false; $any_effect = false; + + $meta_xactions = array(); foreach ($xactions as $key => $xaction) { + if ($xaction->getTransactionType() === $type_mfa) { + $meta_xactions[$key] = $xaction; + continue; + } + if ($this->transactionHasEffect($object, $xaction)) { if ($xaction->getTransactionType() != $type_comment) { $any_effect = true; @@ -2286,15 +2334,30 @@ } else { $no_effect[$key] = $xaction; } + if ($xaction->hasComment()) { $has_comment = true; } } + // If every transaction is a meta-transaction applying to the transaction + // group, these transactions are junk. + if (count($meta_xactions) == count($xactions)) { + $no_effect = $xactions; + $any_effect = false; + } + if (!$no_effect) { return $xactions; } + // If none of the transactions have an effect, the meta-transactions also + // have no effect. Add them to the "no effect" list so we get a full set + // of errors for everything. + if (!$any_effect) { + $no_effect += $meta_xactions; + } + if (!$this->getContinueOnNoEffect() && !$this->getIsPreview()) { throw new PhabricatorApplicationTransactionNoEffectException( $no_effect, @@ -2375,6 +2438,12 @@ $xactions, $type); break; + case PhabricatorTransactions::TYPE_MFA: + $errors[] = $this->validateMFATransactions( + $object, + $xactions, + $type); + break; case PhabricatorTransactions::TYPE_CUSTOMFIELD: $groups = array(); foreach ($xactions as $xaction) { @@ -2556,6 +2625,36 @@ return $errors; } + private function validateMFATransactions( + PhabricatorLiskDAO $object, + array $xactions, + $transaction_type) { + $errors = array(); + + $factors = id(new PhabricatorAuthFactorConfig())->loadAllWhere( + 'userPHID = %s', + $this->getActingAsPHID()); + + foreach ($xactions as $xaction) { + if (!$factors) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $transaction_type, + pht('No MFA'), + pht( + 'You do not have any MFA factors attached to your account, so '. + 'you can not sign this transaction group with MFA. Add MFA to '. + 'your account in Settings.'), + $xaction); + } + } + + if ($xactions) { + $this->setShouldRequireMFA(true); + } + + return $errors; + } + protected function adjustObjectForPolicyChecks( PhabricatorLiskDAO $object, array $xactions) { @@ -4707,4 +4806,48 @@ ->setNewValue($new_value); } + private function requireMFA(PhabricatorLiskDAO $object, array $xactions) { + $editor_class = get_class($this); + + $object_phid = $object->getPHID(); + if ($object_phid) { + $workflow_key = sprintf( + 'editor(%s).phid(%s)', + $editor_class, + $object_phid); + } else { + $workflow_key = sprintf( + 'editor(%s).new()', + $editor_class); + } + + $actor = $this->getActor(); + + $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.')); + } + + $cancel_uri = $this->getCancelURI(); + if ($cancel_uri === null) { + throw new Exception( + pht( + 'This transaction group requires MFA to apply, but the Editor was '. + 'not configured with a Cancel URI. This workflow can not perform '. + 'an MFA check.')); + } + + id(new PhabricatorAuthSessionEngine()) + ->setWorkflowKey($workflow_key) + ->requireHighSecurityToken($actor, $request, $cancel_uri); + + foreach ($xactions as $xaction) { + $xaction->setIsMFATransaction(true); + } + } + } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -473,6 +473,8 @@ return 'fa-th-large'; case PhabricatorTransactions::TYPE_COLUMNS: return 'fa-columns'; + case PhabricatorTransactions::TYPE_MFA: + return 'fa-vcard'; } return 'fa-pencil'; @@ -510,6 +512,8 @@ return 'sky'; } break; + case PhabricatorTransactions::TYPE_MFA; + return 'pink'; } return null; } @@ -835,6 +839,10 @@ return pht( 'You have not moved this object to any columns it is not '. 'already in.'); + case PhabricatorTransactions::TYPE_MFA: + return pht( + 'You can not sign a transaction group that has no other '. + 'effects.'); } return pht( @@ -1076,6 +1084,12 @@ } break; + + case PhabricatorTransactions::TYPE_MFA: + return pht( + '%s signed these changes with MFA.', + $this->renderHandleLink($author_phid)); + default: // In developer mode, provide a better hint here about which string // we're missing. @@ -1238,6 +1252,9 @@ } break; + case PhabricatorTransactions::TYPE_MFA: + return null; + } return $this->getTitle(); @@ -1320,6 +1337,10 @@ // (which are shown anyway) but less interesting than any other type of // transaction. return 0.75; + case PhabricatorTransactions::TYPE_MFA: + // We want MFA signatures to render at the top of transaction groups, + // on top of the things they signed. + return 10; } return 1.0; @@ -1434,6 +1455,8 @@ $this_source = $this->getContentSource()->getSource(); } + $type_mfa = PhabricatorTransactions::TYPE_MFA; + foreach ($group as $xaction) { // Don't group transactions by different authors. if ($xaction->getAuthorPHID() != $this->getAuthorPHID()) { @@ -1477,6 +1500,13 @@ if ($is_mfa != $xaction->getIsMFATransaction()) { return false; } + + // Don't group two "Sign with MFA" transactions together. + if ($this->getTransactionType() === $type_mfa) { + if ($xaction->getTransactionType() === $type_mfa) { + return false; + } + } } return true; diff --git a/src/view/phui/PHUITimelineEventView.php b/src/view/phui/PHUITimelineEventView.php --- a/src/view/phui/PHUITimelineEventView.php +++ b/src/view/phui/PHUITimelineEventView.php @@ -605,7 +605,7 @@ // provide a hint that it was extra authentic. if ($this->getIsMFA()) { $extra[] = id(new PHUIIconView()) - ->setIcon('fa-vcard', 'green') + ->setIcon('fa-vcard', 'pink') ->setTooltip(pht('MFA Authenticated')); } }