Page MenuHomePhabricator

Add "Contact Numbers" so we can send users SMS mesages
ClosedPublic

Authored by epriestley on Jan 17 2019, 3:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 20, 5:54 PM
Unknown Object (File)
Sat, Mar 16, 10:42 AM
Unknown Object (File)
Sat, Mar 16, 1:45 AM
Unknown Object (File)
Fri, Mar 1, 4:31 AM
Unknown Object (File)
Thu, Feb 29, 5:04 PM
Unknown Object (File)
Thu, Feb 29, 5:04 PM
Unknown Object (File)
Thu, Feb 29, 5:04 PM
Unknown Object (File)
Thu, Feb 29, 5:04 PM
Subscribers
Restricted Owners Package

Details

Summary

Ref T920. To send you SMS messages, we need to know your phone number.

This adds bare-bone basics (transactions, storage, editor, etc).

From here:

Disabling Numbers: I'll let you disable numbers in an upcoming diff.

Primary Number: I think I'm just going to let you pick a number as "primary", similar to how email works. We could imagine a world where you have one "MFA" number and one "notifications" number, but this seems unlikely-ish?

Publishing Numbers (Profile / API): At some point, we could let you say that a number is public / "show on my profile" and provide API access / directory features. Not planning to touch this for now.

Non-Phone Numbers: Eventually this could be a list of other similar contact mechanisms (APNS/GCM devices, Whatsapp numbers, ICQ number, twitter handle so MFA can slide into your DM's?). Not planning to touch this for now, but the path should be straightforward when we get there. This is why it's called "Contact Number", not "Phone Number".

MFA-Required + SMS: Right now, if the only MFA provider is SMS and MFA is required on the install, you can't actually get into Settings to add a contact number to configure SMS. I'll look at the best way to deal with this in an upcoming diff -- likely, giving you partial access to more of Setings before you get thorugh the MFA gate. Conceptually, it seems reasonable to let you adjust some other settings, like "Language" and "Accessibility", before you set up MFA, so if the "you need to add MFA" portal was more like a partial Settings screen, maybe that's pretty reasonable.

Verifying Numbers: We'll probably need to tackle this eventually, but I'm not planning to worry about it for now.

Test Plan

Screen Shot 2019-01-17 at 7.38.00 AM.png (862×1 px, 201 KB)

Diff Detail

Repository
rP Phabricator
Branch
mfa3
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21573
Build 29401: Run Core Tests
Build 29400: arc lint + arc unit

Event Timeline

Owners added a subscriber: Restricted Owners Package.Jan 17 2019, 3:48 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 17 2019, 3:49 PM
Harbormaster failed remote builds in B21573: Diff 47721!
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 17 2019, 3:54 PM
Harbormaster failed remote builds in B21574: Diff 47722!

Harbormaster failed remote builds in B21574: Diff 47722!

Oh, this is a MySQL charset/version thing:

Target Error
phabricator_unittest_l2h564al5pey5px3d6vtdznc_auth.auth_contactnumber.key_number #1071: Specified key was too long; max key length is 767 bytes

The easiest way to fix this it to change contactNumber from text255 to text128.

I think I'm going to deal with this by adding a contactNumberUniqueDigest sort of column with digestForIndex() and putting the unique constraint on that instead though. Some considerations:

  • I expect this key to eventually be on <contactNumberType, contactNumber>, e.g. you can have 555-555-5555 as a phone number (<phone, 555...>) and someone else can have that as an ICQ number (<icq, 555...>). If we put it on the actual columns, we may have to keep smushing things or risk running out of bytes, since we can only have ~191 total characters in the key.
  • I may want this key to only guarantee uniqueness of non-disabled contact numbers. An easy way to do this would be to make the key column nullable, and have the object return NULL for its unique key if it is disabled, and an index digest otherwise.
  • This gives us more tools to normalize numbers for uniqueness: the value for the digest can be more normalized than the contactNumber value. For example, we can store +1 (555) 555-5555 but hash +15555555555 if we want, or let users store 1-800-CALL-SAUL but hash +18002255728.
  • Some "contact numbers" -- notably postal addresses -- may be more than 255 characters long. I'm not really sold on implementing "postcard MFA" yet, but don't want to completely write it off since it might actually be kind of reasonable in some cases. Sending users postcards is also fun and cute and this software platform is mostly about being fun and cute, with technical and product considerations a distant second.
  • This makes one line of uniqueness-testing code cleaner and more future-proof (the ->withContactNumbers(...) uniqueness test). As written, it would need to change for ICQ support. With a separate uniqueness column, it'll be good as-is.

Another case is that "phone" and "whatsapp" are different contact mechanisms but it looks like they should actually be in the same "uniqueness namespace", i.e. if you have <whatsapp, 555...> another user shouldn't have <phone, 555...> for the same number. We can hash "phone:555..." for both of them while still knowing that they're different record types.

  • Use a dedicated unique column to enforce uniqueness.

Take THAT, PagerDuty!

src/applications/auth/xaction/PhabricatorAuthContactNumberNumberTransaction.php
26–30

Shouldn't this have the standard if (!$old) { } else if (!$new) { } else { }logic?

src/applications/metamta/message/PhabricatorPhoneNumber.php
22–26

I have no idea what the implications are for storing phone numbers as a bag of digits vs having distinct countryCode, exchange, lastCoupleDigits, etc, fields. I feel reasonably good about always adding a US prefix for numbers that don't specify it, though. Obviously in the future this should use your time zone preference to infer the correct country code on the fly.

Have you read any of the E.164 spec, or heard any war stories about normalizing phone numbers? What we're doing here all seems reasonable to me, but I can imagine a horrible future where Phabricator needs to make phone calls to users who live in the distant past and we need to store phone numbers like PEnnsylvania 6-5000 or whatever.

src/applications/search/view/PhabricatorSearchResultView.php
129

This is the only "nubmer" typo in the whole code base!

This revision is now accepted and ready to land.Jan 19 2019, 12:25 AM

I have some war stories about normalizing phone numbers... IIRC, the worst offender is Brazil (numbers may vary in length).

I have zero experience with phone numbers but I'm sure they're highly regular, unambiguous, and users have totally consistent expectations about how to input them and how they should be formatted.

src/applications/auth/xaction/PhabricatorAuthContactNumberNumberTransaction.php
26–30

You can't remove the contact number (since the value is required), and the !$old transaction will always be a "create" transaction that doesn't ever render.

This revision was automatically updated to reflect the committed changes.