From bce44385e1e358cc7065c63d798376c106e62753 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 25 Jan 2019 09:50:02 -0800
Subject: [PATCH] Add more factor details to the Settings factor list

Summary:
Depends on D20033. Ref T13222. Flesh this UI out a bit, and provide bit-strength information for TOTP.

Also, stop users from adding multiple SMS factors since this is pointless (they all always text your primary contact number).

Test Plan:
{F6156245}

{F6156246}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20034
---
 .../auth/factor/PhabricatorAuthFactor.php     | 19 +++++++++++++++++++
 .../auth/factor/PhabricatorSMSAuthFactor.php  | 18 ++++++++++++++++++
 .../auth/factor/PhabricatorTOTPAuthFactor.php | 13 +++++++++++++
 .../storage/PhabricatorAuthFactorProvider.php |  9 +++++++++
 .../PhabricatorMultiFactorSettingsPanel.php   | 10 ++++++++++
 5 files changed, 69 insertions(+)

diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php
index 194f4acb59..4a4c6d4c3c 100644
--- a/src/applications/auth/factor/PhabricatorAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorAuthFactor.php
@@ -3,6 +3,7 @@
 abstract class PhabricatorAuthFactor extends Phobject {
 
   abstract public function getFactorName();
+  abstract public function getFactorShortName();
   abstract public function getFactorKey();
   abstract public function getFactorCreateHelp();
   abstract public function getFactorDescription();
@@ -66,6 +67,13 @@ abstract class PhabricatorAuthFactor extends Phobject {
     return null;
   }
 
+  public function getConfigurationListDetails(
+    PhabricatorAuthFactorConfig $config,
+    PhabricatorAuthFactorProvider $provider,
+    PhabricatorUser $viewer) {
+    return null;
+  }
+
   /**
    * Is this a factor which depends on the user's contact number?
    *
@@ -524,4 +532,15 @@ abstract class PhabricatorAuthFactor extends Phobject {
     return $request->validateCSRF();
   }
 
+  final protected function loadConfigurationsForProvider(
+    PhabricatorAuthFactorProvider $provider,
+    PhabricatorUser $user) {
+
+    return id(new PhabricatorAuthFactorConfigQuery())
+      ->setViewer($user)
+      ->withUserPHIDs(array($user->getPHID()))
+      ->withFactorProviderPHIDs(array($provider->getPHID()))
+      ->execute();
+  }
+
 }
diff --git a/src/applications/auth/factor/PhabricatorSMSAuthFactor.php b/src/applications/auth/factor/PhabricatorSMSAuthFactor.php
index d91dceb475..58065a03c3 100644
--- a/src/applications/auth/factor/PhabricatorSMSAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorSMSAuthFactor.php
@@ -8,6 +8,10 @@ final class PhabricatorSMSAuthFactor
   }
 
   public function getFactorName() {
+    return pht('Text Message (SMS)');
+  }
+
+  public function getFactorShortName() {
     return pht('SMS');
   }
 
@@ -75,6 +79,10 @@ final class PhabricatorSMSAuthFactor
       return false;
     }
 
+    if ($this->loadConfigurationsForProvider($provider, $user)) {
+      return false;
+    }
+
     return true;
   }
 
@@ -96,6 +104,16 @@ final class PhabricatorSMSAuthFactor
           ));
     }
 
+    if ($this->loadConfigurationsForProvider($provider, $user)) {
+      $messages[] = id(new PHUIInfoView())
+        ->setSeverity(PHUIInfoView::SEVERITY_WARNING)
+        ->setErrors(
+          array(
+            pht(
+              'You already have SMS authentication attached to your account.'),
+          ));
+    }
+
     return $messages;
   }
 
diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
index 1401724125..f69840abdd 100644
--- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
@@ -10,6 +10,10 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     return pht('Mobile Phone App (TOTP)');
   }
 
+  public function getFactorShortName() {
+    return pht('TOTP');
+  }
+
   public function getFactorCreateHelp() {
     return pht(
       'Allow users to attach a mobile authenticator application (like '.
@@ -38,6 +42,15 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
       'to add a new TOTP code, continue to the next step.');
   }
 
+  public function getConfigurationListDetails(
+    PhabricatorAuthFactorConfig $config,
+    PhabricatorAuthFactorProvider $provider,
+    PhabricatorUser $viewer) {
+
+    $bits = strlen($config->getFactorSecret()) * 8;
+    return pht('%d-Bit Secret', $bits);
+  }
+
   public function processAddFactorForm(
     PhabricatorAuthFactorProvider $provider,
     AphrontFormView $form,
diff --git a/src/applications/auth/storage/PhabricatorAuthFactorProvider.php b/src/applications/auth/storage/PhabricatorAuthFactorProvider.php
index dfc948a423..50031d818d 100644
--- a/src/applications/auth/storage/PhabricatorAuthFactorProvider.php
+++ b/src/applications/auth/storage/PhabricatorAuthFactorProvider.php
@@ -126,6 +126,15 @@ final class PhabricatorAuthFactorProvider
     return $this->getFactor()->getConfigurationCreateDescription($this, $user);
   }
 
+  public function getConfigurationListDetails(
+    PhabricatorAuthFactorConfig $config,
+    PhabricatorUser $viewer) {
+    return $this->getFactor()->getConfigurationListDetails(
+      $config,
+      $this,
+      $viewer);
+  }
+
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 
diff --git a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
index c355b6e14a..4da09dd324 100644
--- a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
@@ -77,6 +77,8 @@ final class PhabricatorMultiFactorSettingsPanel
         ->setIcon("{$status_icon} {$status_color}")
         ->setTooltip(pht('Provider: %s', $status->getName()));
 
+      $details = $provider->getConfigurationListDetails($factor, $viewer);
+
       $rows[] = array(
         $icon,
         javelin_tag(
@@ -86,7 +88,9 @@ final class PhabricatorMultiFactorSettingsPanel
             'sigil' => 'workflow',
           ),
           $factor->getFactorName()),
+        $provider->getFactor()->getFactorShortName(),
         $provider->getDisplayName(),
+        $details,
         phabricator_datetime($factor->getDateCreated(), $viewer),
         javelin_tag(
           'a',
@@ -107,6 +111,8 @@ final class PhabricatorMultiFactorSettingsPanel
         null,
         pht('Name'),
         pht('Type'),
+        pht('Provider'),
+        pht('Details'),
         pht('Created'),
         null,
       ));
@@ -115,6 +121,8 @@ final class PhabricatorMultiFactorSettingsPanel
         null,
         'wide pri',
         null,
+        null,
+        null,
         'right',
         'action',
       ));
@@ -125,6 +133,8 @@ final class PhabricatorMultiFactorSettingsPanel
         true,
         false,
         false,
+        false,
+        false,
         true,
       ));