Page MenuHomePhabricator
Differential D20022 Diff 47819 src/applications/auth/factor/PhabricatorSMSAuthFactor.php

Changeset View

Standalone View

View Options

src/applications/auth/factor/PhabricatorSMSAuthFactor.php

  • This file was added.
<?php
final class PhabricatorSMSAuthFactor
extends PhabricatorAuthFactor {
public function getFactorKey() {
return 'sms';
}
public function getFactorName() {
return pht('SMS');
}
public function getFactorCreateHelp() {
return pht(
'Allow users to receive a code via SMS.');
}
public function getFactorDescription() {
return pht(
'When you need to authenticate, a text message with a code will '.
'be sent to your phone.');
}
public function getFactorOrder() {
// Sort this factor toward the end of the list because SMS is relatively
// weak.
return 2000;
}
public function canCreateNewProvider() {
return $this->isSMSMailerConfigured();
}
public function getProviderCreateDescription() {
$messages = array();
if (!$this->isSMSMailerConfigured()) {
$messages[] = id(new PHUIInfoView())
->setErrors(
array(
pht(
'You have not configured an outbound SMS mailer. You must '.
'configure one before you can set up SMS. See: %s',
phutil_tag(
'a',
array(
'href' => '/config/edit/cluster.mailers/',
),
'cluster.mailers')),
));
}
$messages[] = id(new PHUIInfoView())
->setSeverity(PHUIInfoView::SEVERITY_WARNING)
->setErrors(
array(
pht(
'SMS is weak, and relatively easy for attackers to compromise. '.
'Strongly consider using a different MFA provider.'),
));
return $messages;
}
public function canCreateNewConfiguration(PhabricatorUser $user) {
if (!$this->loadUserContactNumber($user)) {
return false;
}
return true;
}
public function getConfigurationCreateDescription(PhabricatorUser $user) {
$messages = array();
if (!$this->loadUserContactNumber($user)) {
$messages[] = id(new PHUIInfoView())
->setSeverity(PHUIInfoView::SEVERITY_WARNING)
->setErrors(
array(
pht(
'You have not configured a primary contact number. Configure '.
'a contact number before adding SMS as an authentication '.
'factor.'),
));
}
return $messages;
}
public function getEnrollDescription(
PhabricatorAuthFactorProvider $provider,
PhabricatorUser $user) {
return pht(
'To verify your phone as an authentication factor, a text message with '.
'a secret code will be sent to the phone number you have listed as '.
'your primary contact number.');
}
public function getEnrollButtonText(
PhabricatorAuthFactorProvider $provider,
PhabricatorUser $user) {
$contact_number = $this->loadUserContactNumber($user);
return pht('Send SMS: %s', $contact_number->getDisplayName());
}
public function processAddFactorForm(
PhabricatorAuthFactorProvider $provider,
AphrontFormView $form,
AphrontRequest $request,
PhabricatorUser $user) {
$token = $this->loadMFASyncToken($request, $form, $user);
$code = $request->getStr('sms.code');
$e_code = true;
if (!$token->getIsNewTemporaryToken()) {
$expect_code = $token->getTemporaryTokenProperty('code');
$okay = phutil_hashes_are_identical(
$this->normalizeSMSCode($code),
$this->normalizeSMSCode($expect_code));
if ($okay) {
$config = $this->newConfigForUser($user)
->setFactorName(pht('SMS'));
return $config;
} else {
if (!strlen($code)) {
$e_code = pht('Required');
} else {
$e_code = pht('Invalid');
}
}
}
$form->appendRemarkupInstructions(
pht(
'Enter the code from the text message which was sent to your '.
'primary contact number.'));
$form->appendChild(
id(new PHUIFormNumberControl())
->setLabel(pht('SMS Code'))
->setName('sms.code')
->setValue($code)
->setError($e_code));
}
protected function newIssuedChallenges(
PhabricatorAuthFactorConfig $config,
PhabricatorUser $viewer,
array $challenges) {
// If we already issued a valid challenge for this workflow and session,
// don't issue a new one.
$challenge = $this->getChallengeForCurrentContext(
$config,
$viewer,
$challenges);
if ($challenge) {
return array();
}
// Otherwise, issue a new challenge.
$challenge_code = $this->newSMSChallengeCode();
$envelope = new PhutilOpaqueEnvelope($challenge_code);
$this->sendSMSCodeToUser($envelope, $viewer);
$ttl_seconds = phutil_units('15 minutes in seconds');
return array(
$this->newChallenge($config, $viewer)
->setChallengeKey($challenge_code)
->setChallengeTTL(PhabricatorTime::getNow() + $ttl_seconds),
);
}
protected function newResultFromIssuedChallenges(
PhabricatorAuthFactorConfig $config,
PhabricatorUser $viewer,
array $challenges) {
$challenge = $this->getChallengeForCurrentContext(
$config,
$viewer,
$challenges);
if ($challenge->getIsAnsweredChallenge()) {
return $this->newResult()
->setAnsweredChallenge($challenge);
}
return null;
}
public function renderValidateFactorForm(
PhabricatorAuthFactorConfig $config,
AphrontFormView $form,
PhabricatorUser $viewer,
PhabricatorAuthFactorResult $result) {
$control = $this->newAutomaticControl($result);
if (!$control) {
$value = $result->getValue();
$error = $result->getErrorMessage();
$name = $this->getChallengeResponseParameterName($config);
$control = id(new PHUIFormNumberControl())
->setName($name)
->setDisableAutocomplete(true)
->setValue($value)
->setError($error);
}
$control
->setLabel(pht('SMS Code'))
->setCaption(pht('Factor Name: %s', $config->getFactorName()));
$form->appendChild($control);
}
public function getRequestHasChallengeResponse(
PhabricatorAuthFactorConfig $config,
AphrontRequest $request) {
$value = $this->getChallengeResponseFromRequest($config, $request);
return (bool)strlen($value);
}
protected function newResultFromChallengeResponse(
PhabricatorAuthFactorConfig $config,
PhabricatorUser $viewer,
AphrontRequest $request,
array $challenges) {
$challenge = $this->getChallengeForCurrentContext(
$config,
$viewer,
$challenges);
$code = $this->getChallengeResponseFromRequest(
$config,
$request);
$result = $this->newResult()
->setValue($code);
if ($challenge->getIsAnsweredChallenge()) {
return $result->setAnsweredChallenge($challenge);
}
if (phutil_hashes_are_identical($code, $challenge->getChallengeKey())) {
$ttl = PhabricatorTime::getNow() + phutil_units('15 minutes in seconds');
$challenge
->markChallengeAsAnswered($ttl);
return $result->setAnsweredChallenge($challenge);
}
if (strlen($code)) {
$error_message = pht('Invalid');
} else {
$error_message = pht('Required');
}
$result->setErrorMessage($error_message);
return $result;
}
private function newSMSChallengeCode() {
$value = Filesystem::readRandomInteger(0, 99999999);
$value = sprintf('%08d', $value);
amckinleyUnsubmitted
Not Done
Inline Actions

FWIW I usually see six-digit SMS codes in the wild, but I don't see anything wrong with using a bit more entropy.

amckinley: FWIW I usually see six-digit SMS codes in the wild, but I don't see anything wrong with using a…
epriestleyAuthorUnsubmitted
Done
Inline Actions

That was pretty much my line of thought.

We could also do XXXX-YYYY or something to make them slightly easier to read/remember.

epriestley: That was pretty much my line of thought. We could also do `XXXX-YYYY` or something to make…
return $value;
}
private function isSMSMailerConfigured() {
$mailers = PhabricatorMetaMTAMail::newMailers(
array(
'outbound' => true,
'media' => array(
PhabricatorMailSMSMessage::MESSAGETYPE,
),
));
return (bool)$mailers;
}
private function loadUserContactNumber(PhabricatorUser $user) {
$contact_numbers = id(new PhabricatorAuthContactNumberQuery())
->setViewer($user)
->withObjectPHIDs(array($user->getPHID()))
->withStatuses(
array(
PhabricatorAuthContactNumber::STATUS_ACTIVE,
))
->withIsPrimary(true)
->execute();
if (count($contact_numbers) !== 1) {
return null;
}
return head($contact_numbers);
}
protected function newMFASyncTokenProperties(PhabricatorUser $user) {
$sms_code = $this->newSMSChallengeCode();
$envelope = new PhutilOpaqueEnvelope($sms_code);
$this->sendSMSCodeToUser($envelope, $user);
return array(
'code' => $sms_code,
);
}
private function sendSMSCodeToUser(
PhutilOpaqueEnvelope $envelope,
PhabricatorUser $user) {
$uri = PhabricatorEnv::getURI('/');
$uri = new PhutilURI($uri);
return id(new PhabricatorMetaMTAMail())
->setMessageType(PhabricatorMailSMSMessage::MESSAGETYPE)
->addTos(array($user->getPHID()))
->setForceDelivery(true)
->setSensitiveContent(true)
->setBody(
pht(
'Phabricator (%s) MFA Code: %s',
$uri->getDomain(),
$envelope->openEnvelope()))
->save();
}
amckinleyUnsubmitted
Not Done
Inline Actions

Is there any way to inform the user if this daemon task fails, like if Twilio is unavailable or whatever?

amckinley: Is there any way to inform the user if this daemon task fails, like if Twilio is unavailable or…
epriestleyAuthorUnsubmitted
Done
Inline Actions

"Yes", but it's pretty complicated. We can save the message ID as part of the challenge and query for the message status, then change the UI to show "queued" vs "sent" vs "error".

However, when we render the form it will probably always be "queued" (usually, the daemons haven't picked it up and delivered it in the few milliseconds between the send and the form render), and users who don't get a message won't necessarily guess that submitting the form will give them more detail, so I think we'd need to do some javascript magic to live-update it without requiring user interaction. Seems like a lot of work until we have more evidence that SMS services routinely have long delays.

I think there's also no real next step for users, since knowing where in the queue things are gummed up doesn't really let them do anything other than keep waiting. Maybe slightly useful for administrators evaluating different SMS gateways, but probably an "average time spent in queue" readout somewhere would be more helpful for that. Also helps deflect blame away from us if Twilio or SNS or whatever are actually just awful, but Twilio seems pretty good for me locally, at least.

The one thing it would help with is "user entered a bogus number which Twilio won't deliver to", but I think the longer-term remedy for that is probably verification rather than making this flow try to cover things end-to-end. We could still get stuck in some cases like "you verified on Twilio, but SNS doesn't recognize the number", of course. None of that stuff has really cropped up for email too seriously, but maybe phone numbers are a wilder west.

Another attack we could take on this is to make bounces feed back to the user after T13115, so you get a notification whenever one of your address or contact numbers end up on the bounce list. That's a less surgical solution, but solves a wider set of problems.

epriestley: "Yes", but it's pretty complicated. We can save the message ID as part of the challenge and…
amckinleyUnsubmitted
Not Done
Inline Actions

Yeah that makes sense. One change though is that we should probably send an MFA challenge when someone adds a contact number ins, just to make sure that end-to-end SMS delivery is working correctly. Arguably that would add friction in the future where we also make actual phone calls to users, but right now I don't think there's anything in scope for phones aside from sending SMS's.

amckinley: Yeah that makes sense. One change though is that we should probably send an MFA challenge when…
private function normalizeSMSCode($code) {
return trim($code);
}
private function getChallengeResponseParameterName(
PhabricatorAuthFactorConfig $config) {
return $this->getParameterName($config, 'sms.code');
}
private function getChallengeResponseFromRequest(
PhabricatorAuthFactorConfig $config,
AphrontRequest $request) {
$name = $this->getChallengeResponseParameterName($config);
$value = $request->getStr($name);
$value = (string)$value;
$value = trim($value);
return $value;
}
}
Phabricator · Built by Phacility