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 @@ -61,6 +61,16 @@ return null; } + abstract public function getEnrollDescription( + PhabricatorAuthFactorProvider $provider, + PhabricatorUser $user); + + public function getEnrollButtonText( + PhabricatorAuthFactorProvider $provider, + PhabricatorUser $user) { + return pht('Continue'); + } + public function getFactorOrder() { return 1000; } @@ -315,17 +325,13 @@ ->setTokenCode($sync_key_digest) ->setTokenExpires($now + $sync_ttl); - // Note that property generation is unguarded, since factors that push - // a challenge generally need to perform a write there. - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $properties = $this->newMFASyncTokenProperties($user); + $properties = $this->newMFASyncTokenProperties($user); - foreach ($properties as $key => $value) { - $sync_token->setTemporaryTokenProperty($key, $value); - } + foreach ($properties as $key => $value) { + $sync_token->setTemporaryTokenProperty($key, $value); + } - $sync_token->save(); - unset($unguarded); + $sync_token->save(); } $form->addHiddenInput($this->getMFASyncTokenFormKey(), $sync_key); 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 @@ -23,6 +23,21 @@ 'authenticate, you will enter a code shown on your phone.'); } + public function getEnrollDescription( + PhabricatorAuthFactorProvider $provider, + PhabricatorUser $user) { + + return pht( + 'To add a TOTP factor to your account, you will first need to install '. + 'a mobile authenticator application on your phone. Two applications '. + 'which work well are **Google Authenticator** and **Authy**, but any '. + 'other TOTP application should also work.'. + "\n\n". + 'If you haven\'t already, download and install a TOTP application on '. + 'your phone now. Once you\'ve launched the application and are ready '. + 'to add a new TOTP code, continue to the next step.'); + } + public function processAddFactorForm( PhabricatorAuthFactorProvider $provider, AphrontFormView $form, @@ -60,17 +75,10 @@ } } - $form->appendRemarkupInstructions( - pht( - 'First, download an authenticator application on your phone. Two '. - 'applications which work well are **Authy** and **Google '. - 'Authenticator**, but any other TOTP application should also work.')); - $form->appendInstructions( pht( - 'Launch the application on your phone, and add a new entry for '. - 'this Phabricator install. When prompted, scan the QR code or '. - 'manually enter the key shown below into the application.')); + 'Scan the QR code or manually enter the key shown below into the '. + 'application.')); $prod_uri = new PhutilURI(PhabricatorEnv::getProductionURI('/')); $issuer = $prod_uri->getDomain(); diff --git a/src/applications/auth/storage/PhabricatorAuthFactorProvider.php b/src/applications/auth/storage/PhabricatorAuthFactorProvider.php --- a/src/applications/auth/storage/PhabricatorAuthFactorProvider.php +++ b/src/applications/auth/storage/PhabricatorAuthFactorProvider.php @@ -109,6 +109,13 @@ ->addInt($this->getID()); } + public function getEnrollDescription(PhabricatorUser $user) { + return $this->getFactor()->getEnrollDescription($this, $user); + } + + public function getEnrollButtonText(PhabricatorUser $user) { + return $this->getFactor()->getEnrollButtonText($this, $user); + } /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php --- a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php @@ -231,10 +231,26 @@ ->addCancelButton($cancel_uri); } + // NOTE: Beyond providing guidance, this step is also providing a CSRF gate + // on this endpoint, since prompting the user to respond to a challenge + // sometimes requires us to push a challenge to them as a side effect (for + // example, with SMS). + if (!$request->isFormPost() || !$request->getBool('mfa.start')) { + $description = $selected_provider->getEnrollDescription($viewer); + + return $this->newDialog() + ->addHiddenInput('providerPHID', $selected_provider->getPHID()) + ->addHiddenInput('mfa.start', 1) + ->setTitle(pht('Add Authentication Factor')) + ->appendChild(new PHUIRemarkupView($viewer, $description)) + ->addCancelButton($cancel_uri) + ->addSubmitButton($selected_provider->getEnrollButtonText($viewer)); + } + $form = id(new AphrontFormView()) ->setViewer($viewer); - if ($request->isFormPost()) { + if ($request->getBool('mfa.enroll')) { // Subject users to rate limiting so that it's difficult to add factors // by pure brute force. This is normally not much of an attack, but push // factor types may have side effects. @@ -295,6 +311,8 @@ return $this->newDialog() ->addHiddenInput('providerPHID', $selected_provider->getPHID()) + ->addHiddenInput('mfa.start', 1) + ->addHiddenInput('mfa.enroll', 1) ->setWidth(AphrontDialogView::WIDTH_FORM) ->setTitle(pht('Add Authentication Factor')) ->appendChild($form->buildLayoutView())