Page MenuHomePhabricator
Files F13177013

D20021.diff
No OneTemporary
Actions

D20021.diff
View Options

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())

File Metadata

Mime Type
text/plain
Expires
Thu, May 9, 6:38 PM (2 w, 1 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6275437
Default Alt Text
D20021.diff (5 KB)

Event Timeline

Phabricator · Built by Phacility