From 2ca316d652d84500c44a7412083714fd313ff932 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 14 Feb 2019 05:06:06 -0800 Subject: [PATCH] When users confirm Duo MFA in the mobile app, live-update the UI Summary: Ref T13249. Poll for Duo updates in the background so we can automatically update the UI when the user clicks the mobile phone app button. Test Plan: Hit a Duo gate, clicked "Approve" in the mobile app, saw the UI update immediately. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13249 Differential Revision: https://secure.phabricator.com/D20169 --- resources/celerity/map.php | 13 ++- src/__phutil_library_map__.php | 4 + .../PhabricatorAuthApplication.php | 2 + ...abricatorAuthChallengeStatusController.php | 40 +++++++++ .../auth/factor/PhabricatorAuthFactor.php | 23 +++-- .../factor/PhabricatorAuthFactorResult.php | 20 ++--- .../auth/factor/PhabricatorDuoAuthFactor.php | 83 ++++++++++++++++--- .../view/PhabricatorAuthChallengeUpdate.php | 44 ++++++++++ .../form/control/PHUIFormTimerControl.php | 30 ++++++- webroot/rsrc/css/phui/phui-form-view.css | 14 ++++ .../js/phui/behavior-phui-timer-control.js | 41 +++++++++ 11 files changed, 284 insertions(+), 30 deletions(-) create mode 100644 src/applications/auth/controller/mfa/PhabricatorAuthChallengeStatusController.php create mode 100644 src/applications/auth/view/PhabricatorAuthChallengeUpdate.php create mode 100644 webroot/rsrc/js/phui/behavior-phui-timer-control.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 99f3333106..2dd5cf0966 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '7a73ffc5', + 'core.pkg.css' => 'e0f5d66f', 'core.pkg.js' => '5c737607', 'differential.pkg.css' => 'b8df73d4', 'differential.pkg.js' => '67c9ea4c', @@ -151,7 +151,7 @@ return array( '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' => '0807e7ac', + 'rsrc/css/phui/phui-form-view.css' => '01b796c0', 'rsrc/css/phui/phui-form.css' => '159e2d9c', 'rsrc/css/phui/phui-head-thing.css' => 'd7f293df', 'rsrc/css/phui/phui-header-view.css' => '93cea4ec', @@ -502,6 +502,7 @@ return array( 'rsrc/js/phui/behavior-phui-selectable-list.js' => 'b26a41e4', 'rsrc/js/phui/behavior-phui-submenu.js' => 'b5e9bff9', 'rsrc/js/phui/behavior-phui-tab-group.js' => '242aa08b', + 'rsrc/js/phui/behavior-phui-timer-control.js' => 'f84bcbf4', 'rsrc/js/phuix/PHUIXActionListView.js' => 'c68f183f', 'rsrc/js/phuix/PHUIXActionView.js' => 'aaa08f3b', 'rsrc/js/phuix/PHUIXAutocomplete.js' => '58cc4ab8', @@ -650,6 +651,7 @@ return array( 'javelin-behavior-phui-selectable-list' => 'b26a41e4', 'javelin-behavior-phui-submenu' => 'b5e9bff9', 'javelin-behavior-phui-tab-group' => '242aa08b', + 'javelin-behavior-phui-timer-control' => 'f84bcbf4', 'javelin-behavior-phuix-example' => 'c2c500a7', 'javelin-behavior-policy-control' => '0eaa33a9', 'javelin-behavior-policy-rule-editor' => '9347f172', @@ -817,7 +819,7 @@ return array( 'phui-font-icon-base-css' => 'd7994e06', 'phui-fontkit-css' => '9b714a5e', 'phui-form-css' => '159e2d9c', - 'phui-form-view-css' => '0807e7ac', + 'phui-form-view-css' => '01b796c0', 'phui-head-thing-view-css' => 'd7f293df', 'phui-header-view-css' => '93cea4ec', 'phui-hovercard' => '074f0783', @@ -2111,6 +2113,11 @@ return array( 'javelin-stratcom', 'javelin-dom', ), + 'f84bcbf4' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + ), 'f8c4e135' => array( 'javelin-install', 'javelin-dom', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8f06586a68..e3287df74c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2195,6 +2195,8 @@ phutil_register_library_map(array( 'PhabricatorAuthChallengeGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthChallengeGarbageCollector.php', 'PhabricatorAuthChallengePHIDType' => 'applications/auth/phid/PhabricatorAuthChallengePHIDType.php', 'PhabricatorAuthChallengeQuery' => 'applications/auth/query/PhabricatorAuthChallengeQuery.php', + 'PhabricatorAuthChallengeStatusController' => 'applications/auth/controller/mfa/PhabricatorAuthChallengeStatusController.php', + 'PhabricatorAuthChallengeUpdate' => 'applications/auth/view/PhabricatorAuthChallengeUpdate.php', 'PhabricatorAuthChangePasswordAction' => 'applications/auth/action/PhabricatorAuthChangePasswordAction.php', 'PhabricatorAuthConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthConduitAPIMethod.php', 'PhabricatorAuthConduitTokenRevoker' => 'applications/auth/revoker/PhabricatorAuthConduitTokenRevoker.php', @@ -7925,6 +7927,8 @@ phutil_register_library_map(array( 'PhabricatorAuthChallengeGarbageCollector' => 'PhabricatorGarbageCollector', 'PhabricatorAuthChallengePHIDType' => 'PhabricatorPHIDType', 'PhabricatorAuthChallengeQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorAuthChallengeStatusController' => 'PhabricatorAuthController', + 'PhabricatorAuthChallengeUpdate' => 'Phobject', 'PhabricatorAuthChangePasswordAction' => 'PhabricatorSystemAction', 'PhabricatorAuthConduitAPIMethod' => 'ConduitAPIMethod', 'PhabricatorAuthConduitTokenRevoker' => 'PhabricatorAuthRevoker', diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php index 52cf01b2aa..4e0baff229 100644 --- a/src/applications/auth/application/PhabricatorAuthApplication.php +++ b/src/applications/auth/application/PhabricatorAuthApplication.php @@ -97,6 +97,8 @@ final class PhabricatorAuthApplication extends PhabricatorApplication { 'PhabricatorAuthFactorProviderViewController', 'message/(?P[1-9]\d*)/' => 'PhabricatorAuthFactorProviderMessageController', + 'challenge/status/(?P[1-9]\d*)/' => + 'PhabricatorAuthChallengeStatusController', ), 'message/' => array( diff --git a/src/applications/auth/controller/mfa/PhabricatorAuthChallengeStatusController.php b/src/applications/auth/controller/mfa/PhabricatorAuthChallengeStatusController.php new file mode 100644 index 0000000000..884bbaad6d --- /dev/null +++ b/src/applications/auth/controller/mfa/PhabricatorAuthChallengeStatusController.php @@ -0,0 +1,40 @@ +getViewer(); + $id = $request->getURIData('id'); + $now = PhabricatorTime::getNow(); + + $result = new PhabricatorAuthChallengeUpdate(); + + $challenge = id(new PhabricatorAuthChallengeQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->withUserPHIDs(array($viewer->getPHID())) + ->withChallengeTTLBetween($now, null) + ->executeOne(); + if ($challenge) { + $config = id(new PhabricatorAuthFactorConfigQuery()) + ->setViewer($viewer) + ->withPHIDs(array($challenge->getFactorPHID())) + ->executeOne(); + if ($config) { + $provider = $config->getFactorProvider(); + $factor = $provider->getFactor(); + + $result = $factor->newChallengeStatusView( + $config, + $provider, + $viewer, + $challenge); + } + } + + return id(new AphrontAjaxResponse()) + ->setContent($result->newContent()); + } + +} diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php index 912a2c31c9..d7e6e60ecc 100644 --- a/src/applications/auth/factor/PhabricatorAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorAuthFactor.php @@ -80,6 +80,14 @@ abstract class PhabricatorAuthFactor extends Phobject { return array(); } + public function newChallengeStatusView( + PhabricatorAuthFactorConfig $config, + PhabricatorAuthFactorProvider $provider, + PhabricatorUser $viewer, + PhabricatorAuthChallenge $challenge) { + return null; + } + /** * Is this a factor which depends on the user's contact number? * @@ -210,8 +218,6 @@ abstract class PhabricatorAuthFactor extends Phobject { get_class($this))); } - $result->setIssuedChallenges($challenges); - return $result; } @@ -242,8 +248,6 @@ abstract class PhabricatorAuthFactor extends Phobject { get_class($this))); } - $result->setIssuedChallenges($challenges); - return $result; } @@ -339,9 +343,18 @@ abstract class PhabricatorAuthFactor extends Phobject { ->setIcon('fa-commenting', 'green'); } - return id(new PHUIFormTimerControl()) + $control = id(new PHUIFormTimerControl()) ->setIcon($icon) ->appendChild($error); + + $status_challenge = $result->getStatusChallenge(); + if ($status_challenge) { + $id = $status_challenge->getID(); + $uri = "/auth/mfa/challenge/status/{$id}/"; + $control->setUpdateURI($uri); + } + + return $control; } diff --git a/src/applications/auth/factor/PhabricatorAuthFactorResult.php b/src/applications/auth/factor/PhabricatorAuthFactorResult.php index f03c3674da..b5da379545 100644 --- a/src/applications/auth/factor/PhabricatorAuthFactorResult.php +++ b/src/applications/auth/factor/PhabricatorAuthFactorResult.php @@ -11,6 +11,7 @@ final class PhabricatorAuthFactorResult private $value; private $issuedChallenges = array(); private $icon; + private $statusChallenge; public function setAnsweredChallenge(PhabricatorAuthChallenge $challenge) { if (!$challenge->getIsAnsweredChallenge()) { @@ -34,6 +35,15 @@ final class PhabricatorAuthFactorResult return $this->answeredChallenge; } + public function setStatusChallenge(PhabricatorAuthChallenge $challenge) { + $this->statusChallenge = $challenge; + return $this; + } + + public function getStatusChallenge() { + return $this->statusChallenge; + } + public function getIsValid() { return (bool)$this->getAnsweredChallenge(); } @@ -83,16 +93,6 @@ final class PhabricatorAuthFactorResult return $this->value; } - public function setIssuedChallenges(array $issued_challenges) { - assert_instances_of($issued_challenges, 'PhabricatorAuthChallenge'); - $this->issuedChallenges = $issued_challenges; - return $this; - } - - public function getIssuedChallenges() { - return $this->issuedChallenges; - } - public function setIcon(PHUIIconView $icon) { $this->icon = $icon; return $this; diff --git a/src/applications/auth/factor/PhabricatorDuoAuthFactor.php b/src/applications/auth/factor/PhabricatorDuoAuthFactor.php index 4be4c15ea8..66bd7c9ebd 100644 --- a/src/applications/auth/factor/PhabricatorDuoAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorDuoAuthFactor.php @@ -585,7 +585,7 @@ final class PhabricatorDuoAuthFactor $result = $this->newDuoFuture($provider) ->setHTTPMethod('GET') ->setMethod('auth_status', $parameters) - ->setTimeout(5) + ->setTimeout(3) ->resolve(); $state = $result['response']['result']; @@ -661,15 +661,6 @@ final class PhabricatorDuoAuthFactor PhabricatorAuthFactorResult $result) { $control = $this->newAutomaticControl($result); - if (!$control) { - $result = $this->newResult() - ->setIsContinue(true) - ->setErrorMessage( - pht( - 'A challenge has been sent to your phone. Open the Duo '. - 'application and confirm the challenge, then continue.')); - $control = $this->newAutomaticControl($result); - } $control ->setLabel(pht('Duo')) @@ -689,7 +680,27 @@ final class PhabricatorDuoAuthFactor PhabricatorUser $viewer, AphrontRequest $request, array $challenges) { - return $this->newResult(); + + $result = $this->newResult() + ->setIsContinue(true) + ->setErrorMessage( + pht( + 'A challenge has been sent to your phone. Open the Duo '. + 'application and confirm the challenge, then continue.')); + + $challenge = $this->getChallengeForCurrentContext( + $config, + $viewer, + $challenges); + if ($challenge) { + $result + ->setStatusChallenge($challenge) + ->setIcon( + id(new PHUIIconView()) + ->setIcon('fa-refresh', 'green ph-spin')); + } + + return $result; } private function newDuoFuture(PhabricatorAuthFactorProvider $provider) { @@ -790,4 +801,54 @@ final class PhabricatorDuoAuthFactor $hostname)); } + public function newChallengeStatusView( + PhabricatorAuthFactorConfig $config, + PhabricatorAuthFactorProvider $provider, + PhabricatorUser $viewer, + PhabricatorAuthChallenge $challenge) { + + $duo_xaction = $challenge->getChallengeKey(); + + $parameters = array( + 'txid' => $duo_xaction, + ); + + $default_result = id(new PhabricatorAuthChallengeUpdate()) + ->setRetry(true); + + try { + $result = $this->newDuoFuture($provider) + ->setHTTPMethod('GET') + ->setMethod('auth_status', $parameters) + ->setTimeout(5) + ->resolve(); + + $state = $result['response']['result']; + } catch (HTTPFutureCURLResponseStatus $exception) { + // If we failed or timed out, retry. Usually, this is a timeout. + return id(new PhabricatorAuthChallengeUpdate()) + ->setRetry(true); + } + + // For now, don't update the view for anything but an "Allow". Updates + // here are just about providing more visual feedback for user convenience. + if ($state !== 'allow') { + return id(new PhabricatorAuthChallengeUpdate()) + ->setRetry(false); + } + + $icon = id(new PHUIIconView()) + ->setIcon('fa-check-circle-o', 'green'); + + $view = id(new PHUIFormTimerControl()) + ->setIcon($icon) + ->appendChild(pht('You responded to this challenge correctly.')) + ->newTimerView(); + + return id(new PhabricatorAuthChallengeUpdate()) + ->setState('allow') + ->setRetry(false) + ->setMarkup($view); + } + } diff --git a/src/applications/auth/view/PhabricatorAuthChallengeUpdate.php b/src/applications/auth/view/PhabricatorAuthChallengeUpdate.php new file mode 100644 index 0000000000..a8ae5b8825 --- /dev/null +++ b/src/applications/auth/view/PhabricatorAuthChallengeUpdate.php @@ -0,0 +1,44 @@ +retry = $retry; + return $this; + } + + public function getRetry() { + return $this->retry; + } + + public function setState($state) { + $this->state = $state; + return $this; + } + + public function getState() { + return $this->state; + } + + public function setMarkup($markup) { + $this->markup = $markup; + return $this; + } + + public function getMarkup() { + return $this->markup; + } + + public function newContent() { + return array( + 'retry' => $this->getRetry(), + 'state' => $this->getState(), + 'markup' => $this->getMarkup(), + ); + } +} diff --git a/src/view/form/control/PHUIFormTimerControl.php b/src/view/form/control/PHUIFormTimerControl.php index 7229d649e9..090de2c8e4 100644 --- a/src/view/form/control/PHUIFormTimerControl.php +++ b/src/view/form/control/PHUIFormTimerControl.php @@ -3,6 +3,7 @@ final class PHUIFormTimerControl extends AphrontFormControl { private $icon; + private $updateURI; public function setIcon(PHUIIconView $icon) { $this->icon = $icon; @@ -13,11 +14,24 @@ final class PHUIFormTimerControl extends AphrontFormControl { return $this->icon; } + public function setUpdateURI($update_uri) { + $this->updateURI = $update_uri; + return $this; + } + + public function getUpdateURI() { + return $this->updateURI; + } + protected function getCustomControlClass() { return 'phui-form-timer'; } protected function renderInput() { + return $this->newTimerView(); + } + + public function newTimerView() { $icon_cell = phutil_tag( 'td', array( @@ -34,7 +48,21 @@ final class PHUIFormTimerControl extends AphrontFormControl { $row = phutil_tag('tr', array(), array($icon_cell, $content_cell)); - return phutil_tag('table', array(), $row); + $node_id = null; + + $update_uri = $this->getUpdateURI(); + if ($update_uri) { + $node_id = celerity_generate_unique_node_id(); + + Javelin::initBehavior( + 'phui-timer-control', + array( + 'nodeID' => $node_id, + 'uri' => $update_uri, + )); + } + + return phutil_tag('table', array('id' => $node_id), $row); } } diff --git a/webroot/rsrc/css/phui/phui-form-view.css b/webroot/rsrc/css/phui/phui-form-view.css index 3368bcaafb..accce86819 100644 --- a/webroot/rsrc/css/phui/phui-form-view.css +++ b/webroot/rsrc/css/phui/phui-form-view.css @@ -578,3 +578,17 @@ properly, and submit values. */ .mfa-form-enroll-button { text-align: center; } + +.phui-form-timer-updated { + animation: phui-form-timer-fade-in 2s linear; +} + + +@keyframes phui-form-timer-fade-in { + 0% { + background-color: {$lightyellow}; + } + 100% { + background-color: transparent; + } +} diff --git a/webroot/rsrc/js/phui/behavior-phui-timer-control.js b/webroot/rsrc/js/phui/behavior-phui-timer-control.js new file mode 100644 index 0000000000..d5b73a5ee2 --- /dev/null +++ b/webroot/rsrc/js/phui/behavior-phui-timer-control.js @@ -0,0 +1,41 @@ +/** + * @provides javelin-behavior-phui-timer-control + * @requires javelin-behavior + * javelin-stratcom + * javelin-dom + */ + +JX.behavior('phui-timer-control', function(config) { + var node = JX.$(config.nodeID); + var uri = config.uri; + var state = null; + + function onupdate(result) { + var markup = result.markup; + if (markup) { + var new_node = JX.$H(markup).getFragment().firstChild; + JX.DOM.replace(node, new_node); + node = new_node; + + // If the overall state has changed from the previous display state, + // animate the control to draw the user's attention to the state change. + if (result.state !== state) { + state = result.state; + JX.DOM.alterClass(node, 'phui-form-timer-updated', true); + } + } + + var retry = result.retry; + if (retry) { + setTimeout(update, 1000); + } + } + + function update() { + new JX.Request(uri, onupdate) + .setTimeout(10000) + .send(); + } + + update(); +});