Page MenuHomePhorge

Do not expose "Contact Numbers" in user settings when no SMS support is set up in Phorge
Open, Needs TriagePublic

Description

Steps to reproduce:

  1. Go to https://we.phorge.it/settings/panel/contact/
  2. Successfully add a "Contact Number"

Expected outcome:
It is good for something and that something is explained to me.

Actual outcome:
No idea. What is it good for?

This seems to be about sending SMS messages according to https://secure.phabricator.com/D19988 . However if your Phorge sysop has not set up anything to send SMS messages then this should not be exposed to your average confused Phorge user.

Event Timeline

Also, is it something shared to somebody? I think nope so I totally agree

Eh, grep'ing the source, src/applications/metamta/engine/PhabricatorMailSMSEngine.php seems to be the only place reading those values.

/src/docs/user/configuration/configuring_outbound_email.diviner mentions Configure this mailer for SMS.
And there's 2FA mentioned in /src/docs/user/userguide/multi_factor_auth.diviner: To use SMS, first add your phone number in {nav Settings > Contact Numbers}. However, http://phorge.localhost/auth/config/new/ doesn't list anything obviously SMS related...

I wonder if there were plans to incorporate contact numbers with the prototype app for handling support (the name escapes me).

I would probably prefer to add text to the contact page clarifying what your phone number would be used for and who can see it rather than conditionally display it.

I'd expect that a user's contact numbers might show up on their profile to other registered users. I suspect this might be something that upstream considered implementing at some point but never did for some reason...

I'd expect that a user's contact numbers might show up on their profile to other registered users.

I do not think so. As an average user with ID 4, I went to http://phorge.localhost/settings/user/user4/page/contact/ and successfully Add Contact Number.
Afterwards as an admin, I went to http://phorge.localhost/auth/contact/4/ and see that I cannot access that user's number: Users with the "Can View" capability: user4 (user4) can take this action.

Also note the auth in the URL path, implying that the "Contact Number" functionality isn't meant to provide information for others but part of implementing SMS support in https://secure.phabricator.com/T920 . See the patches in that old upstream ticket.

Code below hides the entry in the settings sidebar and the panel itself when no SMS support is configured.
However it does not block accessing /auth/contact/edit/ directly.

diff --git a/src/applications/settings/panel/PhabricatorContactNumbersSettingsPanel.php b/src/applications/settings/panel/PhabricatorContactNumbersSettingsPanel.php
index 7056fd02de..6ae698b775 100644
--- a/src/applications/settings/panel/PhabricatorContactNumbersSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorContactNumbersSettingsPanel.php
@@ -19,6 +19,28 @@ final class PhabricatorContactNumbersSettingsPanel
     return PhabricatorSettingsAuthenticationPanelGroup::PANELGROUPKEY;
   }
 
+  /**
+   * Determine whether to make "Contact Numbers" panel available in users'
+   * "Personal Settings" by checking if SMS support is configured
+   *
+   * @return bool True to display "Contact Numbers" settings panel as SMS
+   *              support is available
+   */
+  public function isUserPanel() {
+    $isSMSMailerConfigured = PhabricatorMetaMTAMail::newMailers(
+      array(
+        'outbound' => true,
+        'media' => array(
+          PhabricatorMailSMSMessage::MESSAGETYPE,
+        ),
+      ));
+    if ($isSMSMailerConfigured ) {
+      return true;
+    } else {
+      return false;
+    }
+  }
+
   public function isMultiFactorEnrollmentPanel() {
     return true;
   }