Page MenuHomePhabricator

Provide SMS Support
Closed, ResolvedPublic


It would be good to get SMS/phone integration into Phabricator. I have half a patch here (which is almost a year old):

We can use Twilio to do all the hard work (actually sending SMS/phone stuff). The major technical challenges are actually pretty straightforward:

  • Email is a lot like SMS, and it seems like they should share as much infrastructure as possible. In particular, we have existing tools for doing things like queueing, delivering, logging, and auditing sent email (in /mail/), and it would be good to reuse as much of this as possible for SMS.
  • We deliver email via "MetaMTA", which is a little bit of a mess. In particular:
    • {T388}
    • {T750}
  • Basically, MetaMTA has this super-custom daemon for mail delivery, but we have a generic worker queue for every other type of similar task. So it would be good to use the normal task queue for MetaMTA mail. I think the only feature the task queue lacks that MetaMTA has is gradual backoff (wait 1s on failure, then 2s, then 3s, etc...) but this should be easy to add. That should reduce the total amount of code in question and simplify things in general.
  • This stuff doesn't necessarily need to be sorted out to do SMS, but it probably gets harder to sort out of we do SMS first, and I try to build things in the order where every patch makes the other patches easier as a general rule.
  • That patch isn't actually THAT bad but the amount of "if (sms) { ... } else { ... }" is kind of nasty. I think my hope was that resolving T388/T750 would magically make the patch super clean. It probably won't.
  • Anyway, after you land some approximation of that patch you need to implement it into the application:
    • The user needs to be able to enter their SMS number (rough implementation in D319) and probably verify it.
    • Herald currently has "Send me an email", but can now have "Send me an SMS". With Twilio, it could also conceivably have "Call me and read me the object", or there could be some way to interact with the SMS to escalate to a call. I think you could do pull too (where you dial a number and it reads you unbreak tasks or something?) but I'm not immediately sure about utility there.
    • We don't support Tasks in Herald right now, but with SMS that probably creates a significant goodness.
    • At Facebook, we had an IRC bot feature like "ircbot: get epriestley now: derp is broken" which would send that user an SMS like "alincoln needs you in #e now: derp is broken" and email them and IRC message them, etc. Basically blast them on every available notification channel. This was pretty useful for release engineers when something was broken.
    • It would be great to support two-factor authentication as an option once we have SMS support (e.g., if you log in from a new machine we require you to enter a code from an SMS).
    • Some semi-related features are:
    • The idea there is that you set up an "oncall" rotation for a project, and then can assign tasks to the project and it will determine the responsible individual by figuring out who is oncall. This would generally makes it easier to get things setup so the right people are getting SMSes and we're firing very few 4AM SMSes to innocent bystanders.


Differential Revisions
D8930: Add SMS support
D19982 / rPe72684a4ba5d: Add infrastructure for sending SMS via AWS SNS
D20017 / rPbb20c136513c: Allow MFA factors to provide more guidance text on create workflows
D20013 / rPf69fbf5ea6d0: Make the "Test" adapter support both SMS and email
D20012 / rPaf71c51f0a0b: Give "MetaMTAMail" a "message type" and support SMS
D20011 / rP596435b35e30: Support designating a contact number as "primary"
D20010 / rP12203762b7f0: Allow contact numbers to be enabled and disabled
D19988 / rPf0c6ee48233a: Add "Contact Numbers" so we can send users SMS mesages
D19971 / rP35f0e31ed3b2: Add a Twilio SMS message adapter
D19970 / rP96d3e73eed64: Fix an issue where "CC"-only email improperly wiped CC addresses
D19965 / rPc3cafffed726: Update the "SES" and "sendmail" mailers for the new API; remove "encoding"
D19961 / rP43a6f34e7f2b: Update the SMTP (PHPMailer) adapter for the new mail API; remove "encoding" and…
D19960 / rP64e3296fe682: Upgrade Sendgrid to the modern mailer API; removes "api-user" option
D19959 / rPd7da3560ec56: Update Mailgun adapter for the new mail adapter API
D19957 / rPbc97a7d7556a: Update Mail test adapter for the newer adapter API and make all tests pass
D19956 / rPa8657e6ab6fd: Update Postmark adapter for multiple mail media
D19955 / rPb5797ce60a35: Refactor mail to produce an intermediate "bag of strings" object in preparation…
D19947 / rPe2f057110462: Drop empty inbound mail at the beginning of the receive workflow, not inside…
D19942 / rPa0668df75a2d: Remove "metamta.domain" and "metamta.placeholder-to-recipient" config options
D19937 / rP7e87d254ab33: Add a parameterized Future for Twilio API calls
D19940 / rP9d5b933ed5a3: Remove all legacy configuration options for mailers
D19939 / rPcfcd35d8a390: Remove standalone SMS support in favor of a "Mail, SMS, and other media are…
D19938 / rPe856e791f3ee: Remove Twilio-PHP API external
D19936 / rPea8be11addd7: Fix a qsprintf() issue in mail queries

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

We should also use this API to provide a "Send me a letter" action in Herald:

I wonder how much extra people would pay for a physical newspaper of Phabricator activity mailed to their organization? Tens of pennies? BOOM!

epriestley assigned this task to btrahan.Apr 9 2014, 1:29 AM

I don't think this is a major priority, but it might be pretty fun/interesting to build and it would get us a lot closer to multi-factor auth (T4398). I'd sleep a little more soundly knowing we had proven SMS integration and didn't have to figure that out in order to implement multi-factor stuff down the road.

Some rough thoughts on this:

  • My experience with Twilio was really good, but we should maybe have a bit of room for different adapters, similar to the mail stuff (I think it has worked out well there). For example, being able to implement a "do nothing" adapter can be nice for testing.
  • D319 has some code but it's incredibly out of date and mostly no longer makes sense. Maybe worth a quick glance.
  • Over time, MetaMTA is much less in the business of queuing and retrying, and I think sharing code with MetaMTAMail makes no sense now.
  • However, some of the approaches that we use in MetaMTA for mail do make sense and are probably worth emulating.

I'd expect support to look very roughly something like:

  • To send an SMS, schedule a worker task with the message (or, for i18n, some way to build the message?) and the PHID (user) targets.
  • If necessary, multiplex it in the daemon queue. I think this is a mistake we made with MetaMTA (mutiplexing happens in process).
  • The worker task does whatever multiplexing is necessary (e.g., if we need to have one object per recipient, which we probably do to track errors), then saves some new type of object to the database, similar to PhabricatorMetaMTAMail, called PhabricatorMetaMTASMS or something, and schedules a task to actually send it.
  • The per-SMS tasks dequeue and transmit their SMSes.
  • This is probably overkill for v1 and we could skip half of this to start out with.

The easiest initial use for this would be a "Send me a text" rule in Herald, I think. It could maybe just send the rule name and object name, without worrying too much about specifics.

We'll also need to add an SMS number field to profiles, but this should be pretty easy nowadays.

epriestley edited this Maniphest Task.Apr 22 2014, 11:30 PM

This should plug into D8875 (and friends) nicely.

btrahan edited this Maniphest Task.May 1 2014, 6:24 PM

Alrighty - the attached diff / commit gets us some SMS support that uses Twilio but its not hooked up anywhere. (As in, the application has no hooks to send SMS.) Things are extensible such that adding new providers is not a big deal.

Next steps are

  • get two factor auth going powered by SMS
  • add "telephone number" to profile
  • investigate "recovering" better - right now if we fail we fail permanently and in some cases we can retry BUT things like two factor auth require very timely texts anyway.
  • add additional providers beyond Twilio
  • add herald integration for "send me a text" action

I am not planning on tackling this immediately - going to do some dashboard stuff, etc for a bit - but will circle back to this eventually. Feel free to 'yoink' it if you want, or do bits of the next steps, etc.

btrahan moved this task from Backlog to Do After Launch on the Phacility board.
btrahan removed btrahan as the assignee of this task.

Putting this upforgrabs... I think this is something we should build ASAP after getting Phacility out the door.

I'm going to kick this off Phacility, since I don't think it's really a Phacility task (except maybe for monitoring, but that needs other work) -- yell if I'm missing something. We can drive this forward whenever, though.

avivey changed the visibility from "All Users" to "Public (No Login Required)".Oct 30 2015, 9:19 PM
eadler added a project: Restricted Project.Aug 5 2016, 5:24 PM
hskiba added a subscriber: hskiba.Feb 19 2017, 4:14 AM
epriestley added a comment.EditedJun 20 2017, 11:51 AM

Yeah, I'm pretty pessimistic about the security of SMS (or voice) as a factor. See also T7639 and T6549#97130. The article identifies these vectors:

  • Social engineering: phone companies happily transfer your number to attackers if they ask nicely. This was used to mount a specific attack on Cloudflare in 2012:
  • Protocol weaknesses: the SMS protocol was discovered in 1620 AD -- before math was invented -- and early SMS pioneers assumed radio waves were magic auras created by angels working miracles, so if you tune a radio to the right frequency you can just hear everyone's SMS read out over the air unencrypted.

Additionally, I think this merits some consideration, although I haven't seen much discussion of it:

  • Apple, at least, has generally built a very connected messaging ecosystem where all my devices receive my text messages, not just my phone, and this is mostly opt-out. So to break an SMS factor for me, any device I own is probably good enough, not just my phone. You can probably just climb a tree outside my house and use ye olde spyglass to read the SMS code off my fridge or a thermostat or the 50' display on my rooftop solar panels in a couple years. Apple's product goals (and other providers' goals -- I assume Android or WinPhonePro or whatever do this too, or will in the future) are generally not aligned well with security goals because 99.9% of SMS traffic is not security-related traffic.

I think SMS is still better than nothing, but if/when we support it we should caution users and administrators about it appropriately ("Better than nothing, but not by much."), and make it clear that TOTP is preferred for these reasons.

I'm considering pushing this forward since MFA is getting an update (see T13231 and adjacent tasks).

Revisiting this, and despite my inclination to the contrary above, I'm now again inclined to try to make "SMS" and "Mail" the same object. There are a lot of new systems which both should likely share (like mail tags, group recipients, encryption, translation preferences, blocklisting, cluster/fallback/provider configuration, the web UI, and probably other stuff that doesn't spring to mind). Nowadays, the path forward from where we are now to a reasonably modern implementation on parity with mail looks like it starts with copy-pasting ten thousand lines of stuff.

Instead, I'm going to look at letting a PhabricatorMetaMTAMail object represent a non-mail message (today: SMS; in the future, perhaps other messaging channels). See also T9141 for general evolution here.

Sadly, whatever was seems to have gone out of business in ~2013, but there are some other "postcard via API" providers.

Also, I suppose Postcard-via-API isn't actually pure memes, from the perspective of MFA recovery? Like, "send a postcard to my previously-entered physical mailing address to prove I own this account" is fairly reasonable for some projects.

As tends to be par for the course, this is turning into a bit of a rabbit hole.

Some adapters can reasonably deliver messages via multiple media. For example, Twilio can deliver messages via SMS or Whatsapp (or some other methods that are likely less interesting). And the "test" adapter (which just drops messages) can "deliver" anything. I'd like to avoid a world where you have to configure Twilio twice to get SMS and Whatsapp support, since I think this is generally counterintuitive and user-hostile (although explicitly configuring two separate Twilio accounts for SMS and Whatsapp is fine).

PhabricatorMailImplementationAdapter currently has a lot of API methods which are very specific to email, like supportsMessageIDHeader() and addReplyTo(). I think this API is largely not very good and a better API would look more like sendEmailMessage(PhabricatorEmailMessage $message).

This stuff can be refactored fairly easily, but it's relatively far afield from my actual goal (broader MFA adapter support) and at some point we maybe run into T12404.

Support for cluster.mailers shipped in 2018 Week 6 (D19003) so I'm pretty comfortable moving to a world where it's required and dropping some of the legacy support, at least. That puts us a few steps closer, if nothing else.

Currently, the SES adapter extends from the Sendmail (PHPMailerLite) adapter. I'd like to fix this.

The SES SendEmail API endpoint still doesn't seem to support custom headers or attachments, so I think we have to keep using SendRawEmail. This requires us to build a raw mail object. (If SendEmail supported everything, we could just post JSON or XML or whatever to it instead.)

We currently use PHPMailerLite to do this, and since it has no method like getMessageAsNiceRawEmail(), we do so in a weird, convoluted way.

Fixing T12404 has three major parts:

  1. Building a nice raw email.
  2. The SMTP protocol bit.
  3. The sendmail protocol bit (which is likely trivial).

A first-party implementation of (1) would make fixing the SES classtree easy and get us closer to T12404. I'm not sure how involved this is. PHPMailerLite is ~2K SLOC but has a lot of stuff which definitely isn't reachable and a bunch of stuff we likely don't need to support (like non-Base64 encoding) so I may see how far I can get with this.

Currently, the SES adapter extends from the Sendmail (PHPMailerLite) adapter. I'd like to fix this.

A fix which doesn't carve out as much new scope is:

  • Create PHPMailerLite::newFromMessage(PhabricatorExternalMessage $message).
  • Call that from SES and Sendmail.

This is a very small step forward, but it's at least along the right path.

epriestley closed this task as Resolved.Wed, Jan 23, 11:07 PM
epriestley claimed this task.

I think this is more or less at the end of the line in terms of actionable work that doesn't make more sense being scoped in elsewhere.