Always require MFA to edit contact numbers

Summary:
Depends on D20023. Ref T13222. Although I think this isn't strictly necessary from a pure security perspective (since you can't modify the primary number while you have MFA SMS), it seems like a generally good idea.

This adds a slightly new MFA mode, where we want MFA if it's available but don't strictly require it.

Test Plan: Disabled, enabled, primaried, unprimaried, and edited contact numbers. With MFA enabled, got prompted for MFA. With no MFA, no prompts.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

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

src/__phutil_library_map__.php

@@ -2205,6 +2205,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthContactNumberEditController' => 'applications/auth/controller/contact/PhabricatorAuthContactNumberEditController.php',
     'PhabricatorAuthContactNumberEditEngine' => 'applications/auth/editor/PhabricatorAuthContactNumberEditEngine.php',
     'PhabricatorAuthContactNumberEditor' => 'applications/auth/editor/PhabricatorAuthContactNumberEditor.php',
+    'PhabricatorAuthContactNumberMFAEngine' => 'applications/auth/engine/PhabricatorAuthContactNumberMFAEngine.php',
     'PhabricatorAuthContactNumberNumberTransaction' => 'applications/auth/xaction/PhabricatorAuthContactNumberNumberTransaction.php',
     'PhabricatorAuthContactNumberPHIDType' => 'applications/auth/phid/PhabricatorAuthContactNumberPHIDType.php',
     'PhabricatorAuthContactNumberPrimaryController' => 'applications/auth/controller/contact/PhabricatorAuthContactNumberPrimaryController.php',
@@ -7909,12 +7910,14 @@ phutil_register_library_map(array(
       'PhabricatorApplicationTransactionInterface',
       'PhabricatorPolicyInterface',
       'PhabricatorDestructibleInterface',
+      'PhabricatorEditEngineMFAInterface',
     ),
     'PhabricatorAuthContactNumberController' => 'PhabricatorAuthController',
     'PhabricatorAuthContactNumberDisableController' => 'PhabricatorAuthContactNumberController',
     'PhabricatorAuthContactNumberEditController' => 'PhabricatorAuthContactNumberController',
     'PhabricatorAuthContactNumberEditEngine' => 'PhabricatorEditEngine',
     'PhabricatorAuthContactNumberEditor' => 'PhabricatorApplicationTransactionEditor',
+    'PhabricatorAuthContactNumberMFAEngine' => 'PhabricatorEditEngineMFAEngine',
     'PhabricatorAuthContactNumberNumberTransaction' => 'PhabricatorAuthContactNumberTransactionType',
     'PhabricatorAuthContactNumberPHIDType' => 'PhabricatorPHIDType',
     'PhabricatorAuthContactNumberPrimaryController' => 'PhabricatorAuthContactNumberController',

src/applications/auth/controller/contact/PhabricatorAuthContactNumberDisableController.php

@@ -24,7 +24,7 @@ final class PhabricatorAuthContactNumberDisableController
     $id = $number->getID();
     $cancel_uri = $number->getURI();
 
-    if ($request->isFormPost()) {
+    if ($request->isFormOrHisecPost()) {
       $xactions = array();
 
       if ($is_disable) {
@@ -42,7 +42,8 @@ final class PhabricatorAuthContactNumberDisableController
         ->setActor($viewer)
         ->setContentSourceFromRequest($request)
         ->setContinueOnNoEffect(true)
-        ->setContinueOnMissingFields(true);
+        ->setContinueOnMissingFields(true)
+        ->setCancelURI($cancel_uri);
 
       try {
         $editor->applyTransactions($number, $xactions);

src/applications/auth/controller/contact/PhabricatorAuthContactNumberPrimaryController.php

@@ -41,7 +41,7 @@ final class PhabricatorAuthContactNumberPrimaryController
         ->addCancelButton($cancel_uri);
     }
 
-    if ($request->isFormPost()) {
+    if ($request->isFormOrHisecPost()) {
       $xactions = array();
 
       $xactions[] = id(new PhabricatorAuthContactNumberTransaction())
@@ -53,7 +53,8 @@ final class PhabricatorAuthContactNumberPrimaryController
         ->setActor($viewer)
         ->setContentSourceFromRequest($request)
         ->setContinueOnNoEffect(true)
-        ->setContinueOnMissingFields(true);
+        ->setContinueOnMissingFields(true)
+        ->setCancelURI($cancel_uri);
 
       try {
         $editor->applyTransactions($number, $xactions);

src/applications/auth/engine/PhabricatorAuthContactNumberMFAEngine.php

@@ -0,0 +1,10 @@
+<?php
+
+final class PhabricatorAuthContactNumberMFAEngine
+  extends PhabricatorEditEngineMFAEngine {
+
+  public function shouldTryMFA() {
+    return true;
+  }
+
+}

src/applications/auth/storage/PhabricatorAuthContactNumber.php

@@ -6,7 +6,8 @@ final class PhabricatorAuthContactNumber
   implements
     PhabricatorApplicationTransactionInterface,
     PhabricatorPolicyInterface,
-    PhabricatorDestructibleInterface {
+    PhabricatorDestructibleInterface,
+    PhabricatorEditEngineMFAInterface {
 
   protected $objectPHID;
   protected $contactNumber;
@@ -232,4 +233,11 @@ final class PhabricatorAuthContactNumber
   }
 
 
+/* -(  PhabricatorEditEngineMFAInterface  )---------------------------------- */
+
+
+  public function newEditEngineMFAEngine() {
+    return new PhabricatorAuthContactNumberMFAEngine();
+  }
+
 }

src/applications/transactions/editengine/PhabricatorEditEngineMFAEngine.php

@@ -34,6 +34,28 @@ abstract class PhabricatorEditEngineMFAEngine
       ->setObject($object);
   }
 
-  abstract public function shouldRequireMFA();
+  /**
+   * Do edits to this object REQUIRE that the user submit MFA?
+   *
+   * This is a strict requirement: users will need to add MFA to their accounts
+   * if they don't already have it.
+   *
+   * @return bool True to strictly require MFA.
+   */
+  public function shouldRequireMFA() {
+    return false;
+  }
+
+  /**
+   * Should edits to this object prompt for MFA if it's available?
+   *
+   * This is advisory: users without MFA on their accounts will be able to
+   * perform edits without being required to add MFA.
+   *
+   * @return bool True to prompt for MFA if available.
+   */
+  public function shouldTryMFA() {
+    return false;
+  }
 
 }

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -4916,6 +4916,10 @@ abstract class PhabricatorApplicationTransactionEditor
     $require_mfa = $engine->shouldRequireMFA();
 
     if (!$require_mfa) {
+      $try_mfa = $engine->shouldTryMFA();
+      if ($try_mfa) {
+        $this->setShouldRequireMFA(true);
+      }
       return $xactions;
     }