mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-24 14:30:56 +01:00
Prevent users from editing, disabling, or swapping their primary contact number while they have SMS MFA
Summary: Depends on D20022. Ref T13222. Since you can easily lock yourself out of your account by swapping to a bad number, prevent contact number edits while "contact number" MFA (today, always SMS) is enabled. (Another approach would be to bind factors to specific contact numbers, and then prevent that number from being edited or disabled while SMS MFA was attached to it. However, I think that's a bit more complicated and a little more unwieldy, and ends up in about the same place as this. I'd consider it more strongly in the future if we had like 20 users say "I have 9 phones" but I doubt this is a real use case.) Test Plan: - With SMS MFA, tried to edit my primary contact number, disable it, and promote another number to become primary. Got a sensible error message in all cases. - After removing SMS MFA, did all that stuff with no issues. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13222 Differential Revision: https://secure.phabricator.com/D20023
This commit is contained in:
parent
ada8a56bb7
commit
7805b217ad
7 changed files with 112 additions and 2 deletions
|
@ -55,7 +55,16 @@ final class PhabricatorAuthContactNumberPrimaryController
|
||||||
->setContinueOnNoEffect(true)
|
->setContinueOnNoEffect(true)
|
||||||
->setContinueOnMissingFields(true);
|
->setContinueOnMissingFields(true);
|
||||||
|
|
||||||
$editor->applyTransactions($number, $xactions);
|
try {
|
||||||
|
$editor->applyTransactions($number, $xactions);
|
||||||
|
} catch (PhabricatorApplicationTransactionValidationException $ex) {
|
||||||
|
// This happens when you try to make a number into your primary
|
||||||
|
// number, but you have contact number MFA on your account.
|
||||||
|
return $this->newDialog()
|
||||||
|
->setTitle(pht('Unable to Make Primary'))
|
||||||
|
->setValidationException($ex)
|
||||||
|
->addCancelButton($cancel_uri);
|
||||||
|
}
|
||||||
|
|
||||||
return id(new AphrontRedirectResponse())->setURI($cancel_uri);
|
return id(new AphrontRedirectResponse())->setURI($cancel_uri);
|
||||||
}
|
}
|
||||||
|
|
|
@ -62,6 +62,18 @@ abstract class PhabricatorAuthFactor extends Phobject {
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Is this a factor which depends on the user's contact number?
|
||||||
|
*
|
||||||
|
* If a user has a "contact number" factor configured, they can not modify
|
||||||
|
* or switch their primary contact number.
|
||||||
|
*
|
||||||
|
* @return bool True if this factor should lock contact numbers.
|
||||||
|
*/
|
||||||
|
public function isContactNumberFactor() {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
abstract public function getEnrollDescription(
|
abstract public function getEnrollDescription(
|
||||||
PhabricatorAuthFactorProvider $provider,
|
PhabricatorAuthFactorProvider $provider,
|
||||||
PhabricatorUser $user);
|
PhabricatorUser $user);
|
||||||
|
|
|
@ -28,6 +28,10 @@ final class PhabricatorSMSAuthFactor
|
||||||
return 2000;
|
return 2000;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public function isContactNumberFactor() {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
public function canCreateNewProvider() {
|
public function canCreateNewProvider() {
|
||||||
return $this->isSMSMailerConfigured();
|
return $this->isSMSMailerConfigured();
|
||||||
}
|
}
|
||||||
|
|
|
@ -83,6 +83,11 @@ final class PhabricatorAuthContactNumberNumberTransaction
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
$mfa_error = $this->newContactNumberMFAError($object, $xaction);
|
||||||
|
if ($mfa_error) {
|
||||||
|
$errors[] = $mfa_error;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
return $errors;
|
return $errors;
|
||||||
|
|
|
@ -41,6 +41,12 @@ final class PhabricatorAuthContactNumberPrimaryTransaction
|
||||||
$xaction);
|
$xaction);
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
$mfa_error = $this->newContactNumberMFAError($object, $xaction);
|
||||||
|
if ($mfa_error) {
|
||||||
|
$errors[] = $mfa_error;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
return $errors;
|
return $errors;
|
||||||
|
|
|
@ -46,6 +46,12 @@ final class PhabricatorAuthContactNumberStatusTransaction
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
$mfa_error = $this->newContactNumberMFAError($object, $xaction);
|
||||||
|
if ($mfa_error) {
|
||||||
|
$errors[] = $mfa_error;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
// NOTE: Enabling a contact number may cause us to collide with another
|
// NOTE: Enabling a contact number may cause us to collide with another
|
||||||
// active contact number. However, there might also be a transaction in
|
// active contact number. However, there might also be a transaction in
|
||||||
// this group that changes the number itself. Since we can't easily
|
// this group that changes the number itself. Since we can't easily
|
||||||
|
|
|
@ -1,4 +1,72 @@
|
||||||
<?php
|
<?php
|
||||||
|
|
||||||
abstract class PhabricatorAuthContactNumberTransactionType
|
abstract class PhabricatorAuthContactNumberTransactionType
|
||||||
extends PhabricatorModularTransactionType {}
|
extends PhabricatorModularTransactionType {
|
||||||
|
|
||||||
|
protected function newContactNumberMFAError($object, $xaction) {
|
||||||
|
// If a contact number is attached to a user and that user has SMS MFA
|
||||||
|
// configured, don't let the user modify their primary contact number or
|
||||||
|
// make another contact number into their primary number.
|
||||||
|
|
||||||
|
$primary_type =
|
||||||
|
PhabricatorAuthContactNumberPrimaryTransaction::TRANSACTIONTYPE;
|
||||||
|
|
||||||
|
if ($xaction->getTransactionType() === $primary_type) {
|
||||||
|
// We're trying to make a non-primary number into the primary number,
|
||||||
|
// so do MFA checks.
|
||||||
|
$is_primary = false;
|
||||||
|
} else if ($object->getIsPrimary()) {
|
||||||
|
// We're editing the primary number, so do MFA checks.
|
||||||
|
$is_primary = true;
|
||||||
|
} else {
|
||||||
|
// Editing a non-primary number and not making it primary, so this is
|
||||||
|
// fine.
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
$target_phid = $object->getObjectPHID();
|
||||||
|
$omnipotent = PhabricatorUser::getOmnipotentUser();
|
||||||
|
|
||||||
|
$user_configs = id(new PhabricatorAuthFactorConfigQuery())
|
||||||
|
->setViewer($omnipotent)
|
||||||
|
->withUserPHIDs(array($target_phid))
|
||||||
|
->execute();
|
||||||
|
|
||||||
|
$problem_configs = array();
|
||||||
|
foreach ($user_configs as $config) {
|
||||||
|
$provider = $config->getFactorProvider();
|
||||||
|
$factor = $provider->getFactor();
|
||||||
|
|
||||||
|
if ($factor->isContactNumberFactor()) {
|
||||||
|
$problem_configs[] = $config;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!$problem_configs) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
$problem_config = head($problem_configs);
|
||||||
|
|
||||||
|
if ($is_primary) {
|
||||||
|
return $this->newInvalidError(
|
||||||
|
pht(
|
||||||
|
'You currently have multi-factor authentication ("%s") which '.
|
||||||
|
'depends on your primary contact number. You must remove this '.
|
||||||
|
'authentication factor before you can modify or disable your '.
|
||||||
|
'primary contact number.',
|
||||||
|
$problem_config->getFactorName()),
|
||||||
|
$xaction);
|
||||||
|
} else {
|
||||||
|
return $this->newInvalidError(
|
||||||
|
pht(
|
||||||
|
'You currently have multi-factor authentication ("%s") which '.
|
||||||
|
'depends on your primary contact number. You must remove this '.
|
||||||
|
'authentication factor before you can designate a new primary '.
|
||||||
|
'contact number.',
|
||||||
|
$problem_config->getFactorName()),
|
||||||
|
$xaction);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
}
|
||||||
|
|
Loading…
Reference in a new issue