From ab0f61aa323d3557b6eff806f257a53e2ca5e759 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 27 Nov 2017 17:16:09 -0800
Subject: [PATCH] Tell users to "Wait Patiently" for admin account verification
 later in the registration process

Summary:
Depends on D18790. Ref T13024. Fixes T8335. Currently, "unapproved" and "disabled" users are bundled together. This prevents users from completing some registration steps (verification, legalpad documents, MFA enrollment) before approval.

Separate approval out and move it to the end so users can do all the required enrollment stuff on their end before we roadblock them.

Test Plan: Required approval, email verification, signatures, and MFA. Registered an account. Verified email, signed documents, enrolled in MFA, and then got prompted to wait for approval.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024, T8335

Differential Revision: https://secure.phabricator.com/D18791
---
 .../PhabricatorAuthNeedsMultiFactorController.php | 12 ++++++++++++
 .../base/controller/PhabricatorController.php     | 15 +++++++++++----
 .../PhabricatorAccessControlTestCase.php          |  6 +++---
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/src/applications/auth/controller/PhabricatorAuthNeedsMultiFactorController.php b/src/applications/auth/controller/PhabricatorAuthNeedsMultiFactorController.php
index f3e2562a1c..d295812d3e 100644
--- a/src/applications/auth/controller/PhabricatorAuthNeedsMultiFactorController.php
+++ b/src/applications/auth/controller/PhabricatorAuthNeedsMultiFactorController.php
@@ -9,9 +9,21 @@ final class PhabricatorAuthNeedsMultiFactorController
     return false;
   }
 
+  public function shouldRequireEnabledUser() {
+    // Users who haven't been approved yet are allowed to enroll in MFA. We'll
+    // kick disabled users out later.
+    return false;
+  }
+
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
 
+    if ($viewer->getIsDisabled()) {
+      // We allowed unapproved and disabled users to hit this controller, but
+      // want to kick out disabled users now.
+      return new Aphront400Response();
+    }
+
     $panel = id(new PhabricatorMultiFactorSettingsPanel())
       ->setUser($viewer)
       ->setViewer($viewer)
diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php
index 09aa24a42e..0fee880378 100644
--- a/src/applications/base/controller/PhabricatorController.php
+++ b/src/applications/base/controller/PhabricatorController.php
@@ -137,10 +137,6 @@ abstract class PhabricatorController extends AphrontController {
     }
 
     if ($this->shouldRequireEnabledUser()) {
-      if ($user->isLoggedIn() && !$user->getIsApproved()) {
-        $controller = new PhabricatorAuthNeedsApprovalController();
-        return $this->delegateToController($controller);
-      }
       if ($user->getIsDisabled()) {
         $controller = new PhabricatorDisabledUserController();
         return $this->delegateToController($controller);
@@ -233,6 +229,17 @@ abstract class PhabricatorController extends AphrontController {
           ->withPHIDs(array($application->getPHID()))
           ->executeOne();
       }
+
+      // If users need approval, require they wait here. We do this near the
+      // end so they can take other actions (like verifying email, signing
+      // documents, and enrolling in MFA) while waiting for an admin to take a
+      // look at things. See T13024 for more discussion.
+      if ($this->shouldRequireEnabledUser()) {
+        if ($user->isLoggedIn() && !$user->getIsApproved()) {
+          $controller = new PhabricatorAuthNeedsApprovalController();
+          return $this->delegateToController($controller);
+        }
+      }
     }
 
     // NOTE: We do this last so that users get a login page instead of a 403
diff --git a/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php b/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php
index b6c268d88c..98fa948722 100644
--- a/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php
+++ b/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php
@@ -159,10 +159,10 @@ final class PhabricatorAccessControlTestCase extends PhabricatorTestCase {
         $u_unverified,
         $u_admin,
         $u_public,
+        $u_notapproved,
       ),
       array(
         $u_disabled,
-        $u_notapproved,
       ));
 
 
@@ -224,7 +224,7 @@ final class PhabricatorAccessControlTestCase extends PhabricatorTestCase {
       ));
 
     $this->checkAccess(
-      pht('Application Controller'),
+      pht('Application Controller, No Login Required'),
       id(clone $app_controller)->setConfig('login', false),
       $request,
       array(
@@ -232,10 +232,10 @@ final class PhabricatorAccessControlTestCase extends PhabricatorTestCase {
         $u_unverified,
         $u_admin,
         $u_public,
+        $u_notapproved,
       ),
       array(
         $u_disabled,
-        $u_notapproved,
       ));
   }