diff --git a/resources/celerity/map.php b/resources/celerity/map.php --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => 'a66ea2e7', + 'core.pkg.css' => 'e0cb8094', 'core.pkg.js' => '5c737607', 'differential.pkg.css' => 'b8df73d4', 'differential.pkg.js' => '67c9ea4c', @@ -151,7 +151,7 @@ 'rsrc/css/phui/phui-document.css' => '52b748a5', 'rsrc/css/phui/phui-feed-story.css' => 'a0c05029', 'rsrc/css/phui/phui-fontkit.css' => '9b714a5e', - 'rsrc/css/phui/phui-form-view.css' => '9508671e', + 'rsrc/css/phui/phui-form-view.css' => '0807e7ac', 'rsrc/css/phui/phui-form.css' => '159e2d9c', 'rsrc/css/phui/phui-head-thing.css' => 'd7f293df', 'rsrc/css/phui/phui-header-view.css' => '93cea4ec', @@ -159,7 +159,7 @@ 'rsrc/css/phui/phui-icon-set-selector.css' => '7aa5f3ec', 'rsrc/css/phui/phui-icon.css' => '281f964d', 'rsrc/css/phui/phui-image-mask.css' => '62c7f4d2', - 'rsrc/css/phui/phui-info-view.css' => 'f9464caf', + 'rsrc/css/phui/phui-info-view.css' => '37b8d9ce', 'rsrc/css/phui/phui-invisible-character-view.css' => 'c694c4a4', 'rsrc/css/phui/phui-left-right.css' => '68513c34', 'rsrc/css/phui/phui-lightbox.css' => '4ebf22da', @@ -817,7 +817,7 @@ 'phui-font-icon-base-css' => 'd7994e06', 'phui-fontkit-css' => '9b714a5e', 'phui-form-css' => '159e2d9c', - 'phui-form-view-css' => '9508671e', + 'phui-form-view-css' => '0807e7ac', 'phui-head-thing-view-css' => 'd7f293df', 'phui-header-view-css' => '93cea4ec', 'phui-hovercard' => '074f0783', @@ -825,7 +825,7 @@ 'phui-icon-set-selector-css' => '7aa5f3ec', 'phui-icon-view-css' => '281f964d', 'phui-image-mask-css' => '62c7f4d2', - 'phui-info-view-css' => 'f9464caf', + 'phui-info-view-css' => '37b8d9ce', 'phui-inline-comment-view-css' => '48acce5b', 'phui-invisible-character-view-css' => 'c694c4a4', 'phui-left-right-css' => '68513c34', 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 @@ -2238,6 +2238,7 @@ 'PhabricatorAuthFactorProviderTransactionType' => 'applications/auth/xaction/PhabricatorAuthFactorProviderTransactionType.php', 'PhabricatorAuthFactorProviderViewController' => 'applications/auth/controller/mfa/PhabricatorAuthFactorProviderViewController.php', 'PhabricatorAuthFactorResult' => 'applications/auth/factor/PhabricatorAuthFactorResult.php', + 'PhabricatorAuthFactorResultException' => 'applications/auth/exception/PhabricatorAuthFactorResultException.php', 'PhabricatorAuthFactorTestCase' => 'applications/auth/factor/__tests__/PhabricatorAuthFactorTestCase.php', 'PhabricatorAuthFinishController' => 'applications/auth/controller/PhabricatorAuthFinishController.php', 'PhabricatorAuthHMACKey' => 'applications/auth/storage/PhabricatorAuthHMACKey.php', @@ -7962,6 +7963,7 @@ 'PhabricatorAuthFactorProviderTransactionType' => 'PhabricatorModularTransactionType', 'PhabricatorAuthFactorProviderViewController' => 'PhabricatorAuthFactorProviderController', 'PhabricatorAuthFactorResult' => 'Phobject', + 'PhabricatorAuthFactorResultException' => 'Exception', 'PhabricatorAuthFactorTestCase' => 'PhabricatorTestCase', 'PhabricatorAuthFinishController' => 'PhabricatorAuthController', 'PhabricatorAuthHMACKey' => 'PhabricatorAuthDAO', diff --git a/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php b/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php --- a/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php +++ b/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php @@ -38,10 +38,14 @@ $request); $is_wait = false; + $is_continue = false; foreach ($results as $result) { if ($result->getIsWait()) { $is_wait = true; - break; + } + + if ($result->getIsContinue()) { + $is_continue = true; } } @@ -55,7 +59,7 @@ if ($is_wait) { $submit = pht('Wait Patiently'); - } else if ($is_upgrade) { + } else if ($is_upgrade && !$is_continue) { $submit = pht('Enter High Security'); } else { $submit = pht('Continue'); @@ -74,19 +78,21 @@ $form_layout = $form->buildLayoutView(); if ($is_upgrade) { + $messages = array( + pht( + 'You are taking an action which requires you to enter '. + 'high security.'), + ); + + $info_view = id(new PHUIInfoView()) + ->setSeverity(PHUIInfoView::SEVERITY_MFA) + ->setErrors($messages); + $dialog - ->setErrors( - array( - pht( - 'You are taking an action which requires you to enter '. - 'high security.'), - )) + ->appendChild($info_view) ->appendParagraph( pht( - 'High security mode helps protect your account from security '. - 'threats, like session theft or someone messing with your stuff '. - 'while you\'re grabbing a coffee. To enter high security mode, '. - 'confirm your credentials.')) + 'To enter high security mode, confirm your credentials:')) ->appendChild($form_layout) ->appendParagraph( pht( diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -47,6 +47,7 @@ private $workflowKey; + private $request; public function setWorkflowKey($workflow_key) { $this->workflowKey = $workflow_key; @@ -65,6 +66,10 @@ return $this->workflowKey; } + public function getRequest() { + return $this->request; + } + /** * Get the session kind (e.g., anonymous, user, external account) from a @@ -480,6 +485,7 @@ return $this->issueHighSecurityToken($session, true); } + $this->request = $request; foreach ($factors as $factor) { $factor->setSessionEngine($this); } @@ -523,10 +529,17 @@ $provider = $factor->getFactorProvider(); $impl = $provider->getFactor(); - $new_challenges = $impl->getNewIssuedChallenges( - $factor, - $viewer, - $issued_challenges); + try { + $new_challenges = $impl->getNewIssuedChallenges( + $factor, + $viewer, + $issued_challenges); + } catch (PhabricatorAuthFactorResultException $ex) { + $ok = false; + $validation_results[$factor_phid] = $ex->getResult(); + $challenge_map[$factor_phid] = $issued_challenges; + continue; + } foreach ($new_challenges as $new_challenge) { $issued_challenges[] = $new_challenge; @@ -546,7 +559,10 @@ continue; } - $ok = false; + if (!$result->getIsValid()) { + $ok = false; + } + $validation_results[$factor_phid] = $result; } diff --git a/src/applications/auth/exception/PhabricatorAuthFactorResultException.php b/src/applications/auth/exception/PhabricatorAuthFactorResultException.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/exception/PhabricatorAuthFactorResultException.php @@ -0,0 +1,17 @@ +result = $result; + parent::__construct(); + } + + public function getResult() { + return $this->result; + } + +} diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php --- a/src/applications/auth/factor/PhabricatorAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorAuthFactor.php @@ -232,6 +232,16 @@ final protected function newAutomaticControl( PhabricatorAuthFactorResult $result) { + $is_error = $result->getIsError(); + if ($is_error) { + return $this->newErrorControl($result); + } + + $is_continue = $result->getIsContinue(); + if ($is_continue) { + return $this->newContinueControl($result); + } + $is_answered = (bool)$result->getAnsweredChallenge(); if ($is_answered) { return $this->newAnsweredControl($result); @@ -271,6 +281,34 @@ pht('You responded to this challenge correctly.')); } + private function newErrorControl( + PhabricatorAuthFactorResult $result) { + + $error = $result->getErrorMessage(); + + $icon = id(new PHUIIconView()) + ->setIcon('fa-times', 'red'); + + return id(new PHUIFormTimerControl()) + ->setIcon($icon) + ->appendChild($error) + ->setError(pht('Error')); + } + + private function newContinueControl( + PhabricatorAuthFactorResult $result) { + + $error = $result->getErrorMessage(); + + $icon = id(new PHUIIconView()) + ->setIcon('fa-commenting', 'green'); + + return id(new PHUIFormTimerControl()) + ->setIcon($icon) + ->appendChild($error); + } + + /* -( Synchronizing New Factors )------------------------------------------ */ @@ -400,4 +438,86 @@ return null; } + + /** + * @phutil-external-symbol class QRcode + */ + final protected function newQRCode($uri) { + $root = dirname(phutil_get_library_root('phabricator')); + require_once $root.'/externals/phpqrcode/phpqrcode.php'; + + $lines = QRcode::text($uri); + + $total_width = 240; + $cell_size = floor($total_width / count($lines)); + + $rows = array(); + foreach ($lines as $line) { + $cells = array(); + for ($ii = 0; $ii < strlen($line); $ii++) { + if ($line[$ii] == '1') { + $color = '#000'; + } else { + $color = '#fff'; + } + + $cells[] = phutil_tag( + 'td', + array( + 'width' => $cell_size, + 'height' => $cell_size, + 'style' => 'background: '.$color, + ), + ''); + } + $rows[] = phutil_tag('tr', array(), $cells); + } + + return phutil_tag( + 'table', + array( + 'style' => 'margin: 24px auto;', + ), + $rows); + } + + final protected function throwResult(PhabricatorAuthFactorResult $result) { + throw new PhabricatorAuthFactorResultException($result); + } + + final protected function getInstallDisplayName() { + $uri = PhabricatorEnv::getURI('/'); + $uri = new PhutilURI($uri); + return $uri->getDomain(); + } + + final protected function getChallengeResponseParameterName( + PhabricatorAuthFactorConfig $config) { + return $this->getParameterName($config, 'mfa.response'); + } + + final protected function getChallengeResponseFromRequest( + PhabricatorAuthFactorConfig $config, + AphrontRequest $request) { + + $name = $this->getChallengeResponseParameterName($config); + + $value = $request->getStr($name); + $value = (string)$value; + $value = trim($value); + + return $value; + } + + final protected function hasCSRF(PhabricatorAuthFactorConfig $config) { + $engine = $config->getSessionEngine(); + $request = $engine->getRequest(); + + if (!$request->isHTTPPost()) { + return false; + } + + return $request->validateCSRF(); + } + } diff --git a/src/applications/auth/factor/PhabricatorAuthFactorResult.php b/src/applications/auth/factor/PhabricatorAuthFactorResult.php --- a/src/applications/auth/factor/PhabricatorAuthFactorResult.php +++ b/src/applications/auth/factor/PhabricatorAuthFactorResult.php @@ -5,6 +5,8 @@ private $answeredChallenge; private $isWait = false; + private $isError = false; + private $isContinue = false; private $errorMessage; private $value; private $issuedChallenges = array(); @@ -44,6 +46,24 @@ return $this->isWait; } + public function setIsError($is_error) { + $this->isError = $is_error; + return $this; + } + + public function getIsError() { + return $this->isError; + } + + public function setIsContinue($is_continue) { + $this->isContinue = $is_continue; + return $this; + } + + public function getIsContinue() { + return $this->isContinue; + } + public function setErrorMessage($error_message) { $this->errorMessage = $error_message; return $this; diff --git a/src/applications/auth/factor/PhabricatorSMSAuthFactor.php b/src/applications/auth/factor/PhabricatorSMSAuthFactor.php --- a/src/applications/auth/factor/PhabricatorSMSAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorSMSAuthFactor.php @@ -171,6 +171,38 @@ return array(); } + if (!$this->loadUserContactNumber($viewer)) { + $result = $this->newResult() + ->setIsError(true) + ->setErrorMessage( + pht( + 'Your account has no primary contact number.')); + + $this->throwResult($result); + } + + if (!$this->isSMSMailerConfigured()) { + $result = $this->newResult() + ->setIsError(true) + ->setErrorMessage( + pht( + 'No outbound mailer which can deliver SMS messages is '. + 'configured.')); + + $this->throwResult($result); + } + + if (!$this->hasCSRF($config)) { + $result = $this->newResult() + ->setIsContinue(true) + ->setErrorMessage( + pht( + 'A text message with an authorization code will be sent to your '. + 'primary contact number.')); + + $this->throwResult($result); + } + // Otherwise, issue a new challenge. $challenge_code = $this->newSMSChallengeCode(); @@ -329,10 +361,6 @@ 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())) @@ -341,7 +369,7 @@ ->setBody( pht( 'Phabricator (%s) MFA Code: %s', - $uri->getDomain(), + $this->getInstallDisplayName(), $envelope->openEnvelope())) ->save(); } @@ -350,22 +378,4 @@ 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; - } - } diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php --- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php @@ -90,7 +90,7 @@ $secret, $issuer); - $qrcode = $this->renderQRCode($uri); + $qrcode = $this->newQRCode($uri); $form->appendChild($qrcode); $form->appendChild( @@ -390,49 +390,6 @@ return $code; } - - /** - * @phutil-external-symbol class QRcode - */ - private function renderQRCode($uri) { - $root = dirname(phutil_get_library_root('phabricator')); - require_once $root.'/externals/phpqrcode/phpqrcode.php'; - - $lines = QRcode::text($uri); - - $total_width = 240; - $cell_size = floor($total_width / count($lines)); - - $rows = array(); - foreach ($lines as $line) { - $cells = array(); - for ($ii = 0; $ii < strlen($line); $ii++) { - if ($line[$ii] == '1') { - $color = '#000'; - } else { - $color = '#fff'; - } - - $cells[] = phutil_tag( - 'td', - array( - 'width' => $cell_size, - 'height' => $cell_size, - 'style' => 'background: '.$color, - ), - ''); - } - $rows[] = phutil_tag('tr', array(), $cells); - } - - return phutil_tag( - 'table', - array( - 'style' => 'margin: 24px auto;', - ), - $rows); - } - private function getTimestepDuration() { return 30; } @@ -470,24 +427,6 @@ return null; } - private function getChallengeResponseParameterName( - PhabricatorAuthFactorConfig $config) { - return $this->getParameterName($config, 'totpcode'); - } - - private function getChallengeResponseFromRequest( - PhabricatorAuthFactorConfig $config, - AphrontRequest $request) { - - $name = $this->getChallengeResponseParameterName($config); - - $value = $request->getStr($name); - $value = (string)$value; - $value = trim($value); - - return $value; - } - protected function newMFASyncTokenProperties(PhabricatorUser $user) { return array( 'secret' => self::generateNewTOTPKey(), diff --git a/src/applications/auth/future/PhabricatorDuoFuture.php b/src/applications/auth/future/PhabricatorDuoFuture.php --- a/src/applications/auth/future/PhabricatorDuoFuture.php +++ b/src/applications/auth/future/PhabricatorDuoFuture.php @@ -112,6 +112,11 @@ $this->secretKey->openEnvelope()); $signature = new PhutilOpaqueEnvelope($signature); + if ($http_method === 'GET') { + $uri->setQueryParams($data); + $data = array(); + } + $future = id(new HTTPSFuture($uri, $data)) ->setHTTPBasicAuthCredentials($this->integrationKey, $signature) ->setMethod($http_method) diff --git a/src/applications/auth/storage/PhabricatorAuthChallenge.php b/src/applications/auth/storage/PhabricatorAuthChallenge.php --- a/src/applications/auth/storage/PhabricatorAuthChallenge.php +++ b/src/applications/auth/storage/PhabricatorAuthChallenge.php @@ -163,10 +163,16 @@ $token = Filesystem::readRandomCharacters(32); $token = new PhutilOpaqueEnvelope($token); - return $this + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + + $this ->setResponseToken($token) ->setResponseTTL($ttl) ->save(); + + unset($unguarded); + + return $this; } public function markChallengeAsCompleted() { diff --git a/src/applications/auth/storage/PhabricatorAuthFactorConfig.php b/src/applications/auth/storage/PhabricatorAuthFactorConfig.php --- a/src/applications/auth/storage/PhabricatorAuthFactorConfig.php +++ b/src/applications/auth/storage/PhabricatorAuthFactorConfig.php @@ -71,6 +71,15 @@ return $this->mfaSyncToken; } + public function getAuthFactorConfigProperty($key, $default = null) { + return idx($this->properties, $key, $default); + } + + public function setAuthFactorConfigProperty($key, $value) { + $this->properties[$key] = $value; + return $this; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/view/phui/PHUIInfoView.php b/src/view/phui/PHUIInfoView.php --- a/src/view/phui/PHUIInfoView.php +++ b/src/view/phui/PHUIInfoView.php @@ -8,6 +8,7 @@ const SEVERITY_NODATA = 'nodata'; const SEVERITY_SUCCESS = 'success'; const SEVERITY_PLAIN = 'plain'; + const SEVERITY_MFA = 'mfa'; private $title; private $errors = array(); @@ -73,20 +74,22 @@ switch ($this->getSeverity()) { case self::SEVERITY_ERROR: $icon = 'fa-exclamation-circle'; - break; + break; case self::SEVERITY_WARNING: $icon = 'fa-exclamation-triangle'; - break; + break; case self::SEVERITY_NOTICE: $icon = 'fa-info-circle'; - break; + break; case self::SEVERITY_PLAIN: case self::SEVERITY_NODATA: return null; - break; case self::SEVERITY_SUCCESS: $icon = 'fa-check-circle'; - break; + break; + case self::SEVERITY_MFA: + $icon = 'fa-lock'; + break; } $icon = id(new PHUIIconView()) diff --git a/webroot/rsrc/css/phui/phui-form-view.css b/webroot/rsrc/css/phui/phui-form-view.css --- a/webroot/rsrc/css/phui/phui-form-view.css +++ b/webroot/rsrc/css/phui/phui-form-view.css @@ -574,3 +574,7 @@ color: {$darkgreytext}; vertical-align: middle; } + +.mfa-form-enroll-button { + text-align: center; +} diff --git a/webroot/rsrc/css/phui/phui-info-view.css b/webroot/rsrc/css/phui/phui-info-view.css --- a/webroot/rsrc/css/phui/phui-info-view.css +++ b/webroot/rsrc/css/phui/phui-info-view.css @@ -93,6 +93,15 @@ color: {$red}; } +.phui-info-severity-mfa { + border-color: {$blue}; + border-left-width: 6px; +} + +.phui-info-severity-mfa .phui-info-icon { + color: {$blue}; +} + .phui-info-severity-warning { border-color: {$yellow}; border-left-width: 6px;