Page MenuHomePhabricator
Files F13325522

D19897.diff
No OneTemporary
Actions

D19897.diff
View Options

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 @@
+<?php
+
+final class PhabricatorAuthMFAEditEngineExtension
+ extends PhabricatorEditEngineExtension {
+
+ const EXTENSIONKEY = 'auth.mfa';
+ const FIELDKEY = 'mfa';
+
+ public function getExtensionPriority() {
+ return 12000;
+ }
+
+ public function isExtensionEnabled() {
+ return true;
+ }
+
+ public function getExtensionName() {
+ return pht('MFA');
+ }
+
+ public function supportsObject(
+ PhabricatorEditEngine $engine,
+ PhabricatorApplicationTransactionInterface $object) {
+ return true;
+ }
+
+ public function buildCustomEditFields(
+ PhabricatorEditEngine $engine,
+ PhabricatorApplicationTransactionInterface $object) {
+
+ $mfa_type = PhabricatorTransactions::TYPE_MFA;
+
+ $viewer = $engine->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'));
}
}

File Metadata

Mime Type
text/plain
Expires
Sun, Jun 16, 1:56 AM (1 w, 3 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6276246
Default Alt Text
D19897.diff (18 KB)

Event Timeline

Phabricator · Built by Phacility