diff --git a/resources/sql/autopatches/20181214.auth.01.workflowkey.sql b/resources/sql/autopatches/20181214.auth.01.workflowkey.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20181214.auth.01.workflowkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_challenge + ADD workflowKey VARCHAR(255) NOT NULL COLLATE {$COLLATE_TEXT}; 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 @@ -46,6 +46,26 @@ const ONETIME_USERNAME = 'rename'; + private $workflowKey; + + public function setWorkflowKey($workflow_key) { + $this->workflowKey = $workflow_key; + return $this; + } + + public function getWorkflowKey() { + + // TODO: A workflow key should become required in order to issue an MFA + // challenge, but allow things to keep working for now until we can update + // callsites. + if ($this->workflowKey === null) { + return 'legacy'; + } + + return $this->workflowKey; + } + + /** * Get the session kind (e.g., anonymous, user, external account) from a * session token. Returns a `KIND_` constant. @@ -473,6 +493,10 @@ return $this->issueHighSecurityToken($session, true); } + foreach ($factors as $factor) { + $factor->setSessionEngine($this); + } + // Check for a rate limit without awarding points, so the user doesn't // get partway through the workflow only to get blocked. PhabricatorSystemActionEngine::willTakeAction( 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 @@ -43,10 +43,13 @@ PhabricatorAuthFactorConfig $config, PhabricatorUser $viewer) { + $engine = $config->getSessionEngine(); + return id(new PhabricatorAuthChallenge()) ->setUserPHID($viewer->getPHID()) ->setSessionPHID($viewer->getSession()->getPHID()) - ->setFactorPHID($config->getPHID()); + ->setFactorPHID($config->getPHID()) + ->setWorkflowKey($engine->getWorkflowKey()); } final public function getNewIssuedChallenges( 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 @@ -225,6 +225,9 @@ $session_phid = $viewer->getSession()->getPHID(); + $engine = $config->getSessionEngine(); + $workflow_key = $engine->getWorkflowKey(); + foreach ($challenges as $challenge) { $challenge_timestep = (int)$challenge->getChallengeKey(); @@ -249,6 +252,17 @@ 'again.', new PhutilNumber($wait_duration))); } + + if ($challenge->getWorkflowKey() !== $workflow_key) { + return $this->newResult() + ->setIsWait(true) + ->setErrorMessage( + pht( + 'This factor recently issued a challenge for a different '. + 'workflow. Wait %s seconds for the code to cycle, then try '. + 'again.', + new PhutilNumber($wait_duration))); + } } return null; 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 @@ -7,6 +7,7 @@ protected $userPHID; protected $factorPHID; protected $sessionPHID; + protected $workflowKey; protected $challengeKey; protected $challengeTTL; protected $properties = array(); @@ -20,6 +21,7 @@ self::CONFIG_COLUMN_SCHEMA => array( 'challengeKey' => 'text255', 'challengeTTL' => 'epoch', + 'workflowKey' => 'text255', ), self::CONFIG_KEY_SCHEMA => array( 'key_issued' => array( 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 @@ -8,6 +8,8 @@ protected $factorSecret; protected $properties = array(); + private $sessionEngine; + protected function getConfiguration() { return array( self::CONFIG_SERIALIZATION => array( @@ -49,4 +51,17 @@ return $impl; } + public function setSessionEngine(PhabricatorAuthSessionEngine $engine) { + $this->sessionEngine = $engine; + return $this; + } + + public function getSessionEngine() { + if (!$this->sessionEngine) { + throw new PhutilInvalidStateException('setSessionEngine'); + } + + return $this->sessionEngine; + } + } diff --git a/src/applications/legalpad/controller/LegalpadDocumentSignController.php b/src/applications/legalpad/controller/LegalpadDocumentSignController.php --- a/src/applications/legalpad/controller/LegalpadDocumentSignController.php +++ b/src/applications/legalpad/controller/LegalpadDocumentSignController.php @@ -154,7 +154,12 @@ // Require two-factor auth to sign legal documents. if ($viewer->isLoggedIn()) { + $workflow_key = sprintf( + 'legalpad.sign(%s)', + $document->getPHID()); + $hisec_token = id(new PhabricatorAuthSessionEngine()) + ->setWorkflowKey($workflow_key) ->requireHighSecurityToken( $viewer, $request,