Add a pre-enroll step for MFA, primarily as a CSRF gate

Summary:
Depends on D20020. Ref T13222. This puts another step in the MFA enrollment flow: pick a provider; read text and click "Continue"; actually enroll.

This is primarily to stop CSRF attacks, since otherwise an attacker can put `<img src="phabricator.com/auth/settings/enroll/?providerPHID=xyz" />` on `cute-cat-pix.com` and get you to send yourself some SMS enrollment text messages, which would be mildly annoying.

We could skip this step if we already have a valid CSRF token (and we often will), but I think there's some value in doing it anyway. In particular:

  - For SMS/Duo, it seems nice to have an explicit "we're about to hit your phone" button.
  - We could let installs customize this text and give users a smoother onboard.
  - It allows the relatively wordy enroll form to be a little less wordy.
  - For tokens which can expire (SMS, Duo) it might save you from answering too slowly if you have to go dig your phone out of your bag downstairs or something.

Test Plan: Added factors, read text. Tried to CSRF the endpoint, got a dialog instead of a live challenge generation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20021

src/applications/auth/factor/PhabricatorAuthFactor.php

@@ -61,6 +61,16 @@ abstract class PhabricatorAuthFactor extends Phobject {
     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,9 +325,6 @@ abstract class PhabricatorAuthFactor extends Phobject {
         ->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);
 
       foreach ($properties as $key => $value) {
@@ -325,7 +332,6 @@ abstract class PhabricatorAuthFactor extends Phobject {
       }
 
       $sync_token->save();
-      unset($unguarded);
     }
 
     $form->addHiddenInput($this->getMFASyncTokenFormKey(), $sync_key);

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php

@@ -23,6 +23,21 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
       '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 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
       }
     }
 
-    $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 '.
+        'Scan the QR code or manually enter the key shown below into the '.
-        'this Phabricator install. When prompted, scan the QR code or '.
+        'application.'));
-        'manually enter the key shown below into the application.'));
 
     $prod_uri = new PhutilURI(PhabricatorEnv::getProductionURI('/'));
     $issuer = $prod_uri->getDomain();

src/applications/auth/storage/PhabricatorAuthFactorProvider.php

@@ -109,6 +109,13 @@ final class PhabricatorAuthFactorProvider
       ->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  )------------------------- */
 

src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php

@@ -231,10 +231,26 @@ final class PhabricatorMultiFactorSettingsPanel
         ->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 @@ final class PhabricatorMultiFactorSettingsPanel
 
     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())