phabricator.git
12 days agoFix an issue where Duo validation could incorrectly apply to other factor types master
epriestley [Sun, 3 Feb 2019 14:36:49 +0000 (06:36 -0800)]
Fix an issue where Duo validation could incorrectly apply to other factor types

See <https://discourse.phabricator-community.org/t/configuring-mfa-provider-totp-fails-for-missing-duo-only-options/2355>.

Test Plan: Created a TOTP provider; created a Duo provider (with missing and supplied values).

2 weeks agoLet omnipotent actors skip MFA transactions
epriestley [Fri, 1 Feb 2019 01:14:18 +0000 (17:14 -0800)]
Let omnipotent actors skip MFA transactions

Summary: This seems generally reasonable, but is also a narrow fix to "Phacility scripts try to move instances into 'up', but the daemons can't MFA".

Test Plan: Launched a new instance locally, no more "daemons can't MFA" error.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20081

2 weeks agoDon't load unnecessary handle data on "transaction.search"
epriestley [Thu, 31 Jan 2019 03:49:02 +0000 (19:49 -0800)]
Don't load unnecessary handle data on "transaction.search"

Summary: Ref T13242. Currently, the transaction query loads handles by default (this is unusual). We don't need them, so turn them off.

Test Plan: No apparent behavioral change, will compare production profiles.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13242

Differential Revision: https://secure.phabricator.com/D20068

2 weeks agoMake repository daemons periodically check for out-of-sync repositories
epriestley [Thu, 31 Jan 2019 20:15:41 +0000 (12:15 -0800)]
Make repository daemons periodically check for out-of-sync repositories

Summary:
See PHI1015. If you add new repository nodes to a cluster, we may not actually sync some repositories for up to 6 hours (if they've had no commits for 30 days).

Add an explicit check for out-of-sync repositories to trigger background sync.

Test Plan:
  - Ran `bin/phd debug pullocal`.
  - Fiddled with the `repository_workingcopy` version table to put the local node in and out of sync with the cluster.
  - Saw appropriate responses in the daemon (sync; wait if the last sync trigger was too recent).

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20078

2 weeks agoAllow "inactive" repositories to be read over SSH for cluster sync
epriestley [Thu, 31 Jan 2019 19:43:17 +0000 (11:43 -0800)]
Allow "inactive" repositories to be read over SSH for cluster sync

Summary:
Fixes T13192. See PHI1015. When you deactivate a repository, we currently stop serving it.

This creates a problem for intracluster sync, since new nodes can't sync it. If nothing else, this means that if you "ship of theseus" your cluster and turn nodes over one at a time, you will eventually lose the entire repository. Since that's clearly a bad outcome, support sync.

Test Plan:
Testing this requires a "real" cluster, so I mostly used `secure`.

I deactivated rGITTEST and ran this on `secure002`:

```
./bin/repository thaw --demote secure002.phacility.net --force GITTEST && ./bin/repository update GITTEST
```

Before the patch, this failed:

```
[2019-01-31 19:40:37] EXCEPTION: (CommandException) Command failed with error #128!
COMMAND
git fetch --prune -- 'ssh://172.30.0.64:22/diffusion/GITTEST/' '+refs/*:refs/*'

STDOUT
(empty)

STDERR
Warning: Permanently added '172.30.0.64' (RSA) to the list of known hosts.
phabricator-ssh-exec: This repository ("rGITTEST") is not available over SSH.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
```

After applying (a similar patch to) this patch to `secure001`, the sync worked.

I'll repeat this test with the actual patch once this deploys to `secure`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13192

Differential Revision: https://secure.phabricator.com/D20077

2 weeks agoRead POST data sightly earlier in request startup
epriestley [Thu, 31 Jan 2019 15:27:47 +0000 (07:27 -0800)]
Read POST data sightly earlier in request startup

Summary:
On instances, the "SiteSource" (for site config) pretty much copy-pastes the "read POST data" block because it needs to make some decisions based on POST data when handling inbound mail webhooks.

Move the upstream read a little earlier so we can get rid of this. Now that this step is separated and must happen before the profiler, there's no reason not to do it earlier.

Test Plan: POSTed some data across pages without issue, will remove duplicate code in upcoming change.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20073

2 weeks agoAllow parent and child revisions to be modified via Conduit
epriestley [Thu, 31 Jan 2019 16:21:31 +0000 (08:21 -0800)]
Allow parent and child revisions to be modified via Conduit

Summary: See PHI1048. We have similar transactions elsewhere already (particularly, on tasks) and these edges don't have any special properties (like "Closes Txx As Wontfix" edges do) so this doesn't create any sort of peril.

Test Plan: {F6170556}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20075

2 weeks agoFix method name typo in new modular transaction text/html mail methods
epriestley [Thu, 31 Jan 2019 16:52:27 +0000 (08:52 -0800)]
Fix method name typo in new modular transaction text/html mail methods

See PHI1039. See D20057.

2 weeks agoAllow modular transactions to override transaction title and body text in mail
epriestley [Tue, 29 Jan 2019 20:03:14 +0000 (12:03 -0800)]
Allow modular transactions to override transaction title and body text in mail

Summary:
Ref T12921. I'm moving Instances to modular transactions, and we have an "Alert" transaction type used to send notifications ("Your instance is going to be suspended for nonpayment.").

Currently, there's no way to specifically customize mail rendering under modular transactions. Add crude support for it.

Note that (per comment) this is fairly aspirational right now, since we actually always render everything as text (see T12921). But this API should (?) mostly survive intact when I fix this properly, and allows Instances to move to modular transactions so I can fix some more pressing issues in the meantime.

Test Plan: See next diff for Instances.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12921

Differential Revision: https://secure.phabricator.com/D20057

2 weeks agoRemove "iconv" PHP extension dependency
epriestley [Wed, 30 Jan 2019 22:44:40 +0000 (14:44 -0800)]
Remove "iconv" PHP extension dependency

Summary: Depends on D20069. Ref T13232. This is a very, very weak dependency and we can reasonably polyfill it.

Test Plan: Grepped for `iconv` in libphutil, arcanist, and Phabricator.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13232

Differential Revision: https://secure.phabricator.com/D20070

2 weeks agoCorrect a bug where milestone "spacePHID" columns could become desynchronized
epriestley [Wed, 30 Jan 2019 02:00:15 +0000 (18:00 -0800)]
Correct a bug where milestone "spacePHID" columns could become desynchronized

Summary:
Depends on D20041. See PHI1046. If you do this:

- Create a parent project called "Crab" in Space 1.
- Create a milestone called "Left Claw".
- Shift "Crab" to Space 2.
- Create a milestone called "Right Claw".

...you currently end up with "Left Claw" in the wrong `spacePHID` in the database. At the application level it's in the correct space, but when we `WHERE ... AND spacePHID IN (...)` we can incorrectly filter it out.

Test Plan:
  - Did the above setup.
  - Saved "Crab", saw the space fix itself.
  - Put things back in the broken state.
  - Ran the migration script, saw things fix themselves again.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: aeiser, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D20063

2 weeks agoImprove handling of "Deny" responses from Duo
epriestley [Wed, 30 Jan 2019 04:45:18 +0000 (20:45 -0800)]
Improve handling of "Deny" responses from Duo

Summary:
Ref T13231. See <https://discourse.phabricator-community.org/t/duo-integration-crashes-if-user-is-not-enrolled-and-enrollment-is-disabled/2340/5>

(There's an actual bug here, although I'm not sure exactly what's going on on the Duo side in the report.)

Test Plan:
To reproduce this, I was only able to actually "Deny" my account explicitly in Duo.

  - With "Deny", tried to add a factor. Got a nice helpful error message.
  - Undenied, added a factor, re-denied, tried to pass an MFA gate. Got another nice helpful error message.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13231

Differential Revision: https://secure.phabricator.com/D20065

2 weeks agoUpdate a factor query in TransactionEditor for providers
epriestley [Wed, 30 Jan 2019 22:59:30 +0000 (14:59 -0800)]
Update a factor query in TransactionEditor for providers

Summary: This query didn't get updated and could let you through an explicit "Sign with MFA" action if you have only disabled factors on your account.

Test Plan:
  - Disabled all factors.
  - Used explicit "Sign With MFA".
    - Before: Went through.
    - After: Sensible error.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20072

2 weeks agoHide "Signed with MFA" stories from feed
epriestley [Wed, 30 Jan 2019 22:59:21 +0000 (14:59 -0800)]
Hide "Signed with MFA" stories from feed

Summary:
See <https://discourse.phabricator-community.org/t/sign-with-mfa-action-on-differential-revisions-creates-strange-feed-entries/2346>.

If you "Sign with MFA" and only add a comment, we can produce a weird notification and feed story. Hide these stories from feed since they're broadly not interesting.

Test Plan:
  - Used "sign with MFA" and only posted a comment.
  - Observed feed.
    - Before: Janky "untitled story".
    - After: Sensible "added a comment" story.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20071

2 weeks agoAllow diff change detection to complete for Mercurial changes which remove a binary...
epriestley [Tue, 29 Jan 2019 17:08:32 +0000 (09:08 -0800)]
Allow diff change detection to complete for Mercurial changes which remove a binary file

Summary:
See PHI1044. When you push a commit, we try to detect if the diff is the same as the last diff in Differential. This is a soft/heuristic check and only used to provide a hint in email ("CHANGED SINCE LAST DIFF").

For some changes, we can end up on this pathway with a binary changeset for a deleted file in the commit, and try to fetch the file data. This will fail since the file has been deleted. I'm not //entirely// sure why this hasn't cropped up before and didn't try to truly build an end-to-end test case, but it's a valid state that we shouldn't fatal in.

When we get here in this state, just detect a change. This is safe, even if it isn't always correct.

(This will be revisited in the future so I'm favoring fixing the broken behavior for now.)

Test Plan: This required some rigging, but I modified `bin/differential extract` to run the `isDiffChangedBeforeCommit()` code, then rigged a commit/diff to hit this case and reproduced the reported error. After the change, the comparison worked cleanly.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20056

2 weeks agoReplace manual query string construction with "phutil_build_http_querystring()"
epriestley [Tue, 29 Jan 2019 03:29:24 +0000 (19:29 -0800)]
Replace manual query string construction with "phutil_build_http_querystring()"

Summary: Now that we have a nice function for this, use it to simplify some code.

Test Plan: Ran through the Duo enroll workflow to make sure signing still works.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20053

2 weeks agoRead "$_POST" before hooking the profiler, and remove "aphront.default-application...
epriestley [Mon, 28 Jan 2019 18:55:17 +0000 (10:55 -0800)]
Read "$_POST" before hooking the profiler, and remove "aphront.default-application-configuration-class"

Summary:
Ref T4369. Ref T12297. Ref T13242. Ref PHI1010. I want to take a quick look at `transaction.search` and see if there's anything quick and obvious we can do to improve performance.

On `secure`, the `__profile__` flag does not survive POST like it's supposed to: when you profile a page and then submit a form on the page, the result is supposed to be profiled. The intent is to make it easier to profile Conduit calls.

I believe this is because we're hooking the profiler, then rebuilding POST data a little later -- so `$_POST['__profile__']` isn't set yet when the profiler checks.

Move the POST rebuild a little earlier to fix this.

Also, remove the very ancient "aphront.default-application-configuration-class". I believe this was only used by Facebook to do CIDR checks against corpnet or something like that. It is poorly named and long-obsolete now, and `AphrontSite` does everything we might reasonably have wanted it to do.

Test Plan: Poked around locally without any issues. Will check if this fixes the issue on `secure`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13242, T12297, T4369

Differential Revision: https://secure.phabricator.com/D20046

2 weeks agoAllow MFA enrollment guidance to be customized
epriestley [Mon, 28 Jan 2019 17:08:14 +0000 (09:08 -0800)]
Allow MFA enrollment guidance to be customized

Summary: Depends on D20039. Ref T13242. If installs want users to install a specific application, reference particular help, etc., let them customize the MFA enrollment message so they can make it say "if you have issues, see this walkthrough on the corporate wiki" or whatever.

Test Plan:
{F6164340}

{F6164341}

{F6164342}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13242

Differential Revision: https://secure.phabricator.com/D20043

2 weeks agoAdd a warning about MFA requirements to edit forms
epriestley [Mon, 28 Jan 2019 18:27:12 +0000 (10:27 -0800)]
Add a warning about MFA requirements to edit forms

Summary:
Depends on D20044. Ref T13242. Similar to D20044, add reminder text to edit forms.

It would be nice to "workflow" these so the MFA flow happens inline, but Maniphest's inline edit behavior currently conflicts with this. Set it aside for now since the next workboards iteration (triggers) is probably a good opportunity to revisit it.

Test Plan: {F6164496}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13242

Differential Revision: https://secure.phabricator.com/D20045

2 weeks agoWarn users about MFA requirements when interacting with "MFA Required" objects via...
epriestley [Mon, 28 Jan 2019 18:01:08 +0000 (10:01 -0800)]
Warn users about MFA requirements when interacting with "MFA Required" objects via the comment form

Summary:
Ref T13242. Warn user that they'll need to MFA (so they can go dig their phone out of their bag first or whatever, or don't type a giant comment on mobile if their U2F key is back at the office) on the comment form.

Also, when they'll need MFA and won't be able to provide it (no MFA on account), stop them from typing up a big comment that they can't actually submit: point them at MFA setup first.

Test Plan:
{F6164448}

{F6164449}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13242

Differential Revision: https://secure.phabricator.com/D20044

2 weeks agoProvide an Editor extension point for transaction validation
epriestley [Sun, 27 Jan 2019 18:21:24 +0000 (10:21 -0800)]
Provide an Editor extension point for transaction validation

Summary:
Depends on D20040. Ref T13242. See PHI1039. See PHI873. Two reasonable cases have arisen recently where extending validation rules would help solve problems.

We can do this in a pretty straightforward way with a standard extension pattern.

Test Plan:
Used this extension to keep ducks away from projects:

```lang=php
<?php

final class NoDucksEditorExtension
  extends PhabricatorEditorExtension {

  const EXTENSIONKEY = 'no.ducks';

  public function getExtensionName() {
    return pht('No Ducks!');
  }

  public function supportsObject(
    PhabricatorApplicationTransactionEditor $editor,
    PhabricatorApplicationTransactionInterface $object) {
    return ($object instanceof PhabricatorProject);
  }

  public function validateTransactions($object, array $xactions) {
    $errors = array();

    $name_type = PhabricatorProjectNameTransaction::TRANSACTIONTYPE;

    $old_value = $object->getName();
    foreach ($xactions as $xaction) {
      if ($xaction->getTransactionType() !== $name_type) {
        continue;
      }

      $new_value = $xaction->getNewValue();
      if ($old_value === $new_value) {
        continue;
      }

      if (preg_match('/duck/i', $new_value)) {
        $errors[] = $this->newInvalidTransactionError(
          $xaction,
          pht('Project names must not contain the substring "duck".'));
      }
    }

    return $errors;
  }

}
```

{F6162585}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13242

Differential Revision: https://secure.phabricator.com/D20041

2 weeks agoSupport subtypes in Projects
epriestley [Sat, 26 Jan 2019 15:29:03 +0000 (07:29 -0800)]
Support subtypes in Projects

Summary:
Ref T13242. See PHI1039. Maniphest subtypes generally seem to be working well. I designed them as a general capability that might be extended to other `EditEngine` objects later, and PHI1039 describes a situation where extending subtypes to projects would give us some reasonable tools.

(Some installs also already use icons/colors as a sort of lightweight version of subtypes, so I believe this is generally useful capability.)

Some of this is a little bit copy-pasted and could probably be shared, but I'd like to wait a bit longer before merging it. For example, both configs have exactly the same structure right now, but Projects should possibly have some different flags (for example: to disable creating subprojects / milestones).

This implementation is pretty basic for now: notably, subprojects/milestones don't get the nice "choose from among subtype forms" treatment that tasks do. If this ends up being part of a solution to PHI1039, I'd plan to fill that in later on.

Test Plan: Defined multiple subtypes, created subtype forms, created projects with appropriate subtypes. Filtered them by subtype. Saw subtype information on list/detail views.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13242

Differential Revision: https://secure.phabricator.com/D20040

2 weeks agoGive MFA gates a more consistent UI
epriestley [Tue, 29 Jan 2019 22:42:10 +0000 (14:42 -0800)]
Give MFA gates a more consistent UI

Summary: Depends on D20057. Currently, we show an "MFA" message on one of these and an "Error" message on the other, with different icons and colors. Use "MFA" for both, with the MFA icon / color.

Test Plan: Hit both varations, saw more consistency.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20059

2 weeks agoUse "null", not "-1", as a local "no version" marker when performing intracluster...
epriestley [Mon, 28 Jan 2019 19:29:00 +0000 (11:29 -0800)]
Use "null", not "-1", as a local "no version" marker when performing intracluster repository sync

Summary:
Ref T13242. See <https://discourse.phabricator-community.org/t/out-of-range-value-for-column-deviceversion/2218>.

The synchronization log column is `uint32?` and `-1` doesn't go into that column.

Since we're only using `-1` for convenience to cheat through `$max_version > $this_version` checks, use `null` instead and make the checks more explicit.

Test Plan: Reproducing this is a bit tricky and I cheated fairly heavily to force the code down this pathway without actually building a multi-device cluster, but I did reproduce the original exception, apply the patch, and observe that it fixed things.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13242

Differential Revision: https://secure.phabricator.com/D20047

2 weeks agoBring Duo MFA upstream
epriestley [Fri, 25 Jan 2019 19:21:03 +0000 (11:21 -0800)]
Bring Duo MFA upstream

Summary: Depends on D20038. Ref T13231. Although I planned to keep this out of the upstream (see T13229) it ended up having enough pieces that I imagine it may need more fixes/updates than we can reasonably manage by copy/pasting stuff around. Until T5055, we don't really have good tools for managing this. Make my life easier by just upstreaming this.

Test Plan: See T13231 for a bunch of workflow discussion.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13231

Differential Revision: https://secure.phabricator.com/D20039

2 weeks agoRequire MFA to edit MFA providers
epriestley [Fri, 25 Jan 2019 18:42:31 +0000 (10:42 -0800)]
Require MFA to edit MFA providers

Summary: Depends on D20037. Ref T13222. Ref T7667. Although administrators can now disable MFA from the web UI, at least require that they survive MFA gates to do so. T7667 (`bin/auth lock`) should provide a sturdier approach here in the long term.

Test Plan: Created and edited MFA providers, was prompted for MFA.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222, T7667

Differential Revision: https://secure.phabricator.com/D20038

2 weeks agoReplace "Show Secret" in Passphrase with one-shot MFA
epriestley [Fri, 25 Jan 2019 18:21:03 +0000 (10:21 -0800)]
Replace "Show Secret" in Passphrase with one-shot MFA

Summary: Depends on D20036. Ref T13222. Now that we support one-shot MFA, swap this from session MFA to one-shot MFA.

Test Plan: Revealed a credential, was no longer left in high-security mode.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20037

2 weeks agoConvert "Rename User" from session MFA to one-shot MFA
epriestley [Fri, 25 Jan 2019 18:16:49 +0000 (10:16 -0800)]
Convert "Rename User" from session MFA to one-shot MFA

Summary:
Depends on D20035. Ref T13222.

  - Allow individual transactions to request one-shot MFA if available.
  - Make "change username" request MFA.

Test Plan:
  - Renamed a user, got prompted for MFA, provided it.
  - Saw that I no longer remain in high-security after performing the edit.
  - Grepped for other uses of `PhabricatorUserUsernameTransaction`, found none.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20036

2 weeks agoGet rid of "throwResult()" for control flow in MFA factors
epriestley [Fri, 25 Jan 2019 18:02:29 +0000 (10:02 -0800)]
Get rid of "throwResult()" for control flow in MFA factors

Summary: Depends on D20034. Ref T13222. This is just cleanup -- I thought we'd have like two of these, but we ended up having a whole lot in Duo and a decent number in SMS. Just let factors return a result explicitly if they can make a decision early. I think using `instanceof` for control flow is a lesser evil than using `catch`, on the balance.

Test Plan: `grep`, went through enroll/gate flows on SMS and Duo.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20035

2 weeks agoAdd more factor details to the Settings factor list
epriestley [Fri, 25 Jan 2019 17:50:02 +0000 (09:50 -0800)]
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

2 weeks agoUpdate documentation for MFA, including administrator guidance
epriestley [Fri, 25 Jan 2019 16:04:06 +0000 (08:04 -0800)]
Update documentation for MFA, including administrator guidance

Summary: Depends on D20032. Ref T13222.

Test Plan: Read documentation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20033

2 weeks agoExpand outbound mailer documentation to mention SMS and include Twilio
epriestley [Fri, 25 Jan 2019 15:06:14 +0000 (07:06 -0800)]
Expand outbound mailer documentation to mention SMS and include Twilio

Summary: Depends on D20031. Ref T13222.

Test Plan: Read "carefully".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20032

2 weeks agoAllow MFA providers to be deprecated or disabled
epriestley [Fri, 25 Jan 2019 12:37:44 +0000 (04:37 -0800)]
Allow MFA providers to be deprecated or disabled

Summary: Ref T13222. Providers can now be deprecated (existing factors still work, but users can't add new factors for the provider) or disabled (factors stop working, also can't add new ones).

Test Plan:
  - Enabled, deprecated, and disabled some providers.
  - Viewed provider detail, provider list.
  - Viewed MFA settings list.
  - Verified that I'm prompted for enabled + deprecated only at gates.
  - Tried to disable final provider, got an error.
  - Hit the MFA setup gate by enabling "Require MFA" with no providers, got a more useful message.
  - Immediately forced a user to the "MFA Setup Gate" by disabling their only active provider with another provider enabled ("We no longer support TOTP, you HAVE to finish Duo enrollment to continue starting Monday.").

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20031

2 weeks agoDon't put "spacePHID IN (...)" constraints in queries which will raise policy exceptions
epriestley [Mon, 28 Jan 2019 15:46:41 +0000 (07:46 -0800)]
Don't put "spacePHID IN (...)" constraints in queries which will raise policy exceptions

Summary:
See T13240. Ref T13242. When we're issuing a query that will raise policy exceptions (i.e., give the user a "You Shall Not Pass" dialog if they can not see objects it loads), don't do space filtering in MySQL: when objects are filtered out in MySQL, we can't distinguish between "bad/invalid ID/object" and "policy filter", so we can't raise a policy exception.

This leads to cases where viewing an object shows "You Shall Not Pass" if you can't see it for any non-Spaces reason, but "404" if the reason is Spaces.

There's no product reason for this, it's just that `spacePHID IN (...)` is important for non-policy-raising queries (like a list of tasks) to reduce how much application filtering we need to do.

Test Plan:
Before:

```
$ git pull
phabricator-ssh-exec: No repository "spellbook" exists!
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
```

After:

```
$ git pull
phabricator-ssh-exec: [You Shall Not Pass: Unknown Object (Repository)] This object is in a space you do not have permission to access.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
```

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13242

Differential Revision: https://secure.phabricator.com/D20042

3 weeks agoAdd CSRF to SMS challenges, and pave the way for more MFA types (including Duo)
epriestley [Thu, 24 Jan 2019 18:16:30 +0000 (10:16 -0800)]
Add CSRF to SMS challenges, and pave the way for more MFA types (including Duo)

Summary:
Depends on D20026. Ref T13222. Ref T13231. The primary change here is that we'll no longer send you an SMS if you hit an MFA gate without CSRF tokens.

Then there's a lot of support for genralizing into Duo (and other push factors, potentially), I'll annotate things inline.

Test Plan: Implemented Duo, elsewhere.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13231, T13222

Differential Revision: https://secure.phabricator.com/D20028

3 weeks agoAdd a Duo API future
epriestley [Wed, 23 Jan 2019 21:29:20 +0000 (13:29 -0800)]
Add a Duo API future

Summary: Depends on D20025. Ref T13231. Although I'm not currently planning to actually upstream a Duo MFA provider, it's probably easiest to put most of the support pieces in the upstream until T5055.

Test Plan: Used a test script to make some (mostly trivial) API calls and got valid results back, so I think the parameter signing is correct.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13231

Differential Revision: https://secure.phabricator.com/D20026

3 weeks agoMake the "PHP 7" setup warning more explicit about what it means
epriestley [Wed, 23 Jan 2019 22:32:00 +0000 (14:32 -0800)]
Make the "PHP 7" setup warning more explicit about what it means

Summary: See <https://discourse.phabricator-community.org/t/minimum-php-versions-supported-by-latest-stable-phabricator/2314/3>.

Test Plan: o~o

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20027

3 weeks agoFix bad "$this" references in "Must Encrypt" mail after MailEngine changes
epriestley [Thu, 24 Jan 2019 22:59:34 +0000 (14:59 -0800)]
Fix bad "$this" references in "Must Encrypt" mail after MailEngine changes

Summary: See PHI1038. I missed these when pulling the code out.

Test Plan: Sent "Must encrypt" mail, verified it made it through the queue in one piece.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20029

3 weeks agoAdd infrastructure for sending SMS via AWS SNS
Austin McKinley [Thu, 3 Jan 2019 23:00:11 +0000 (15:00 -0800)]
Add infrastructure for sending SMS via AWS SNS

Summary:
Ref T920. Ref T13235. This adds a `Future`, similar to `TwilioFuture`, for interacting with Amazon's SNS service.

Also updates the documentation.

Also makes the code consistent with the documentation by accepting a `media` argument.

Test Plan: Clicked the "send test message" button from the Settings UI.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T13235, T920

Differential Revision: https://secure.phabricator.com/D19982

3 weeks agoAdd a "test message" action for contact numbers
epriestley [Wed, 23 Jan 2019 20:15:05 +0000 (12:15 -0800)]
Add a "test message" action for contact numbers

Summary: Depends on D20024. See D20022. Put something in place temporarily until we build out validation at some point.

Test Plan: Sent myself a test message.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20025

3 weeks agoAlways require MFA to edit contact numbers
epriestley [Wed, 23 Jan 2019 19:27:11 +0000 (11:27 -0800)]
Always require MFA to edit contact numbers

Summary:
Depends on D20023. Ref T13222. Although I think this isn't strictly necessary from a pure security perspective (since you can't modify the primary number while you have MFA SMS), it seems like a generally good idea.

This adds a slightly new MFA mode, where we want MFA if it's available but don't strictly require it.

Test Plan: Disabled, enabled, primaried, unprimaried, and edited contact numbers. With MFA enabled, got prompted for MFA. With no MFA, no prompts.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20024

3 weeks agoPrevent users from editing, disabling, or swapping their primary contact number while...
epriestley [Wed, 23 Jan 2019 19:08:02 +0000 (11:08 -0800)]
Prevent users from editing, disabling, or swapping their primary contact number while they have SMS MFA

Summary:
Depends on D20022. Ref T13222. Since you can easily lock yourself out of your account by swapping to a bad number, prevent contact number edits while "contact number" MFA (today, always SMS) is enabled.

(Another approach would be to bind factors to specific contact numbers, and then prevent that number from being edited or disabled while SMS MFA was attached to it. However, I think that's a bit more complicated and a little more unwieldy, and ends up in about the same place as this. I'd consider it more strongly in the future if we had like 20 users say "I have 9 phones" but I doubt this is a real use case.)

Test Plan:
  - With SMS MFA, tried to edit my primary contact number, disable it, and promote another number to become primary. Got a sensible error message in all cases.
  - After removing SMS MFA, did all that stuff with no issues.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20023

3 weeks agoImplement SMS MFA
epriestley [Wed, 16 Jan 2019 16:38:45 +0000 (08:38 -0800)]
Implement SMS MFA

Summary:
Depends on D20021. Ref T13222. This has a few rough edges, including:

  - The challenges theselves are CSRF-able.
  - You can go disable/edit your contact number after setting up SMS MFA and lock yourself out of your account.
  - SMS doesn't require MFA so an attacker can just swap your number to their number.

...but mostly works.

Test Plan:
  - Added SMS MFA to my account.
  - Typed in the number I was texted.
  - Typed in some other different numbers (didn't work).
  - Cancelled/resumed the workflow, used SMS in conjunction with other factors, tried old codes, etc.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20022

3 weeks agoAdd a pre-enroll step for MFA, primarily as a CSRF gate
epriestley [Wed, 23 Jan 2019 17:26:32 +0000 (09:26 -0800)]
Add a pre-enroll step for MFA, primarily as a CSRF gate

Summary:
Depends on D20020. Ref T13222. This puts another step in the MFA enrollment flow: pick a provider; read text and click "Continue"; actually enroll.

This is primarily to stop CSRF attacks, since otherwise an attacker can put `<img src="phabricator.com/auth/settings/enroll/?providerPHID=xyz" />` on `cute-cat-pix.com` and get you to send yourself some SMS enrollment text messages, which would be mildly annoying.

We could skip this step if we already have a valid CSRF token (and we often will), but I think there's some value in doing it anyway. In particular:

  - For SMS/Duo, it seems nice to have an explicit "we're about to hit your phone" button.
  - We could let installs customize this text and give users a smoother onboard.
  - It allows the relatively wordy enroll form to be a little less wordy.
  - For tokens which can expire (SMS, Duo) it might save you from answering too slowly if you have to go dig your phone out of your bag downstairs or something.

Test Plan: Added factors, read text. Tried to CSRF the endpoint, got a dialog instead of a live challenge generation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20021

3 weeks agoAllow different MFA factor types (SMS, TOTP, Duo, ...) to share "sync" tokens when...
epriestley [Wed, 23 Jan 2019 16:17:05 +0000 (08:17 -0800)]
Allow different MFA factor types (SMS, TOTP, Duo, ...) to share "sync" tokens when enrolling new factors

Summary:
Depends on D20019. Ref T13222. Currently, TOTP uses a temporary token to make sure you've set up the app on your phone properly and that you're providing an answer to a secret which we generated (not an attacker-generated secret).

However, most factor types need some kind of sync token. SMS needs to send you a code; Duo needs to store a transaction ID. Turn this "TOTP" token into an "MFA Sync" token and lift the implementation up to the base class.

Also, slightly simplify some of the HTTP form gymnastics.

Test Plan:
  - Hit the TOTP enroll screen.
  - Reloaded it, got new secrets.
  - Reloaded it more than 10 times, got told to stop generating new challenges.
  - Answered a challenge properly, got a new TOTP factor.
  - Grepped for removed class name.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20020

3 weeks agoAdd a rate limit for enroll attempts when adding new MFA configurations
epriestley [Wed, 23 Jan 2019 15:22:10 +0000 (07:22 -0800)]
Add a rate limit for enroll attempts when adding new MFA configurations

Summary:
Depends on D20018. Ref T13222. When you add a new MFA configuration, you can technically (?) guess your way through it with brute force. It's not clear why this would ever really be useful (if an attacker can get here and wants to add TOTP, they can just add TOTP!) but it's probably bad, so don't let users do it.

This limit is fairly generous because I don't think this actually part of any real attack, at least today with factors we're considering.

Test Plan:
  - Added TOTP, guessed wrong a ton of times, got rate limited.
  - Added TOTP, guessed right, got a TOTP factor configuration added to my account.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20019

3 weeks agoDon't rate limit users clicking "Wait Patiently" at an MFA gate even if they typed...
epriestley [Wed, 23 Jan 2019 15:09:20 +0000 (07:09 -0800)]
Don't rate limit users clicking "Wait Patiently" at an MFA gate even if they typed some text earlier

Summary:
Depends on D20017. Ref T13222. Currently, if you:

  - type some text at a TOTP gate;
  - wait ~60 seconds for the challenge to expire;
  - submit the form into a "Wait patiently" message; and
  - mash that wait button over and over again very patiently

...you still rack up rate limiting points, because the hidden text from your original request is preserved and triggers the "is the user responding to a challenge" test. Only perform this test if we haven't already decided that we're going to make them wait.

Test Plan:
  - Did the above; before patch: rate limited; after patch: not rate limited.
  - Intentionally typed a bunch of bad answers which were actually evaluated: rate limited properly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20018

3 weeks agoAllow MFA factors to provide more guidance text on create workflows
epriestley [Wed, 23 Jan 2019 14:49:55 +0000 (06:49 -0800)]
Allow MFA factors to provide more guidance text on create workflows

Summary:
Depends on D20016. Ref T920. This does nothing interesting on its own since the TOTP provider has no guidance/warnings, but landing it separately helps to simplify an upcoming SMS diff.

SMS will have these guidance messages:

  - "Administrator: you haven't configured any mailer which can send SMS, like Twilio."
  - "Administrator: SMS is weak."
  - "User: you haven't configured a contact number."

Test Plan: {F6151283} {F6151284}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D20017

3 weeks agoCorrect an issue where the default "Settings" screen could show the wrong settings
epriestley [Wed, 23 Jan 2019 14:39:09 +0000 (06:39 -0800)]
Correct an issue where the default "Settings" screen could show the wrong settings

Summary:
Depends on D20013. Recently, I renamed the "Account" panel to "Language".

When you land on "Settings" and the first panel is an "EditEngine" panel ("Account/Langauge", "Date and Time", and "Conpherence" are all "EditEngine" panels), the engine shows the controls for the first panel.

However, the "first panel" according to EditEngine and the "first panel" in the menu are currently different: the menu groups panels into topics.

When I renamed "Account" to "Language", it went from conicidentally being the first panel in both lists to being the second panel in the grouped menu list and the, uh, like 12th panel in the ungrouped raw list.

This made landing on "Settings" show you the right chrome, but show you a different panel's controls ("Conpherence", now alphabetically first).

Instead, use the same order in both places.

(This was also a pre-existing bug if you use a language which translates the panel names such that "Account" is not alphabetically first.)

Test Plan: Visited "Settings", saw "Date & Time" form controls instead of "Conpherence" form controls on the default screen with "Date & Time" selected in the menu.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20016

3 weeks agoMake the "Test" adapter support both SMS and email
epriestley [Mon, 21 Jan 2019 23:15:52 +0000 (15:15 -0800)]
Make the "Test" adapter support both SMS and email

Summary:
Depends on D20012. Ref T920. If you have a test adapter configured, it should swallow messages and prevent them from ever hitting a lower-priority adapter.

Make the test adapter support SMS so this actually happens.

Test Plan: Ran `bin/mail send-test --type sms ...` with a test adapter (first) and a Twilio adapter (second). Got SMS swallowed by test adapter instead of live SMS messages.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D20013

3 weeks agoGive "MetaMTAMail" a "message type" and support SMS
epriestley [Mon, 21 Jan 2019 23:03:45 +0000 (15:03 -0800)]
Give "MetaMTAMail" a "message type" and support SMS

Summary:
Depends on D20011. Ref T920. This change lets a "MetaMTAMail" storage object represent various different types of messages, and  makes "all" the `bin/mail` stuff "totally work" with messages of non-email types.

In practice, a lot of the related tooling needs some polish/refinement, but the basics work.

Test Plan: Used `echo beep boop | bin/mail send-test --to epriestley --type sms` to send myself SMS.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D20012

3 weeks agoSupport designating a contact number as "primary"
epriestley [Mon, 21 Jan 2019 21:02:37 +0000 (13:02 -0800)]
Support designating a contact number as "primary"

Summary:
Depends on D20010. Ref T920. Allow users to designate which contact number is "primary": the number we'll actually send stuff to.

Since this interacts in weird ways with "disable", just do a "when any number is touched, put all of the user's rows into the right state" sort of thing.

Test Plan:
  - Added numbers, made numbers primary, disabled a primary number, un-disabled a number with no primaries. Got sensible behavior in all cases.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D20011

3 weeks agoAllow contact numbers to be enabled and disabled
epriestley [Mon, 21 Jan 2019 20:30:10 +0000 (12:30 -0800)]
Allow contact numbers to be enabled and disabled

Summary: Depends on D20008. Ref T920. Continue fleshing out contact number behaviors.

Test Plan:
  - Enabled and disabled a contact number.
  - Saw list, detail views reflect change.
  - Added number X, disabled it, added it again (allowed), enabled the disabled one ("already in use" exception).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D20010

3 weeks agoAllow users to access some settings at the "Add MFA" account setup roadblock
epriestley [Mon, 21 Jan 2019 19:04:49 +0000 (11:04 -0800)]
Allow users to access some settings at the "Add MFA" account setup roadblock

Summary:
Depends on D20006. Ref T13222. Currently, the "MFA Is Required" gate doesn't let you do anything else, but you'll need to be able to access "Contact Numbers" if an install provides SMS MFA.

Tweak this UI to give users limited access to settings, so they can set up contact numbers and change their language.

(This is a little bit fiddly, and I'm doing it early on partly so it can get more testing as these changes move forward.)

Test Plan: {F6146136}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20008

3 weeks agoAdd icons to Settings
epriestley [Mon, 21 Jan 2019 18:31:23 +0000 (10:31 -0800)]
Add icons to Settings

Summary: Depends on D20005. I love icons.

Test Plan: {F6145996}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20006

3 weeks agoExpand "Settings" UI to full-width
epriestley [Mon, 21 Jan 2019 17:50:54 +0000 (09:50 -0800)]
Expand "Settings" UI to full-width

Summary:
Depends on D19988. See D19826 for the last UI expansion. I don't have an especially strong product rationale for un-fixed-width'ing Settings since it doesn't suffer from the "mystery meat actions" issues that other fixed-width UIs do, but I like the full-width UI better and the other other fixed-width UIs all (?) have some actual rationale (e.g., large tables, multiple actions on subpanels), so "consistency" is an argument here.

Also rename "account" to "language" since both settings are language-related.

This moves away from the direction in D18436.

Test Plan:
Clicked each Settings panel, saw sensible rendering at full-width.

{F6145944}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20005

3 weeks agoAdd "Contact Numbers" so we can send users SMS mesages
epriestley [Wed, 16 Jan 2019 17:56:44 +0000 (09:56 -0800)]
Add "Contact Numbers" so we can send users SMS mesages

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: {F6137174}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: avivey, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D19988

3 weeks agoUpdate `bin/auth` MFA commands for the new "MFA Provider" indirection layer
epriestley [Tue, 15 Jan 2019 21:34:31 +0000 (13:34 -0800)]
Update `bin/auth` MFA commands for the new "MFA Provider" indirection layer

Summary:
Ref T13222. This updates the CLI tools and documentation for the changes in D19975.

The flags `--type` and `--all-types` retain their current meaning. In most cases, `bin/auth strip --type totp` is sufficient and you don't need to bother looking up the relevant provider PHID. The existing `bin/auth list-factors` is also unchanged.

The new `--provider` flag allows you to select configs from a particular provider in a more granular way. The new `bin/auth list-mfa-providers` provides an easy way to get PHIDs.

(In the Phacility cluster, the "Strip MFA" action just reaches into the database and deletes rows manually, so this isn't terribly important. I verified that the code should still work properly.)

Test Plan:
  - Ran `bin/auth list-mfa-providers`.
  - Stripped by user / type / provider.
  - Grepped for `list-factors` and `auth strip`.
  - Hit all (?) of the various possible error cases.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19976

3 weeks agoConvert user MFA factors to point at configurable "MFA Providers", not raw "MFA Factors"
epriestley [Tue, 15 Jan 2019 15:57:32 +0000 (07:57 -0800)]
Convert user MFA factors to point at configurable "MFA Providers", not raw "MFA Factors"

Summary:
Ref T13222. Users configure "Factor Configs", which say "I have an entry on my phone for TOTP secret key XYZ".

Currently, these point at raw implementations -- always "TOTP" in practice.

To support configuring available MFA types (like "no MFA") and adding MFA types that need some options set (like "Duo", which needs API keys), bind "Factor Configs" to a "Factor Provider" instead.

In the future, several "Factors" will be available (TOTP, SMS, Duo, Postal Mail, ...). Administrators configure zero or more "MFA Providers" they want to use (e.g., "Duo" + here's my API key). Then users can add configs for these providers (e.g., "here's my Duo account").

Upshot:

  - Factor: a PHP subclass, implements the technical details of a type of MFA factor (TOTP, SMS, Duo, etc).
  - FactorProvider: a storage object, owned by administrators, configuration of a Factor that says "this should be available on this install", plus provides API keys, a human-readable name, etc.
  - FactorConfig: a storage object, owned by a user, says "I have a factor for provider X on my phone/whatever with secret key Q / my duo account is X / my address is Y".

Couple of things not covered here:

  - Statuses for providers ("Disabled", "Deprecated") don't do anything yet, but you can't edit them anyway.
  - Some `bin/auth` tools need to be updated.
  - When no providers are configured, the MFA panel should probably vanish.
  - Documentation.

Test Plan:
  - Ran migration with providers, saw configs point at the first provider.
  - Ran migration without providers, saw a provider created and configs pointed at it.
  - Added/removed factors and providers. Passed MFA gates. Spot-checked database for general sanity.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19975

3 weeks agoFix bin/accountadmin when not making changes
Austin McKinley [Mon, 21 Jan 2019 20:34:16 +0000 (12:34 -0800)]
Fix bin/accountadmin when not making changes

Summary: If you go through the `accountadmin` flow and change nothing, you get an exception about the transaction not having any effect. Instead, let the `applyTransactions` call continue even on no effect.

Test Plan: Ran `accountadmin` without changing anything for an existing user. No longer got an exception about no-effect transactions.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D20009

3 weeks agoUpdate documentation to reflect bin/auth changes
Austin McKinley [Mon, 21 Jan 2019 19:56:56 +0000 (11:56 -0800)]
Update documentation to reflect bin/auth changes

Summary: See https://secure.phabricator.com/D18901#249481. Update the docs and a warning string to reflect the new reality that `bin/auth recover` is now able to recover any account, not just administrators.

Test Plan: Mk 1 eyeball

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D20007

3 weeks agoWhen dirtying repository cluster routing caches after an Almanac edit, discover linke...
epriestley [Mon, 21 Jan 2019 04:27:32 +0000 (20:27 -0800)]
When dirtying repository cluster routing caches after an Almanac edit, discover linked bindings from devices

Summary:
See PHI1030. When you edit an Almanac object, we attempt to discover all the related objects so we can dirty the repository cluster routing cache: if you modify a Device or Service that's part of a clustered repository, we need to blow away our cached view of the layout.

Currently, we don't correctly find linked Bindings when editing a Device, so we may miss Services which have keys that need to be disabled. Instead, discover these linked objects.

See D17000 for the original implementation and more context.

Test Plan:
  - Used `var_dump()` to dump out the discovered objects and dirtied cache keys.
  - Before change: editing a Service dirties repository routing keys (this is correct), but editing a Device does not.
  - After change: editing a Device now correctly dirties repository routing keys.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20003

3 weeks agoApply inverse edge edits after committing primary object edits
epriestley [Mon, 14 Jan 2019 20:33:28 +0000 (12:33 -0800)]
Apply inverse edge edits after committing primary object edits

Summary:
Fixes T13082. When you create a revision (say, `D111`) with `Ref T222` in the body, we write a `D111 -> T222` edge ("revision 111 references task 222") and an inverse `T222 -> D111` edge ("task 222 is referenced by revision 111").

We also apply a transaction to `D111` ("alice added a task: Txxx.") and an inverse transaction to `T222` ("alice added a revision: Dxxx").

Currently, it appears that the inverse transaction can sometimes generate mail faster than `D111` actually commits its (database) transactions, so the mail says "alice added a revision: Unknown Object (Differential Revision)". See T13082 for evidence that this is true, and a reproduction case.

To fix this, apply the inverse transaction (to `T222`) after we commit the main object (here, `D111`).

This is tricky because when we apply transactions, the transaction editor automatically "fixes" them to be consistent with the database state. For example, if a task already has title "XYZ" and you set the title to "XYZ" (same title), we just no-op the transaction.

It also fixes edge edits. The old sequence was:

  - Open (database) transaction.
  - Apply our transaction ("alice added a task").
  - Apply the inverse transaction ("alice added a revision").
  - Write the edges to the database.
  - Commit (database) transaction.

Under this sequence, the inverse transaction was "correct" and didn't need to be fixed, so the fixing step didn't touch it.

The new sequence is:

  - Open (database) transaction.
  - Apply our transaction ("alice added a task").
  - Write the edges.
  - Commit (database) transaction.
  - Apply the inverse transaction ("alice added a revision").

Since the inverse transaction now happens after the database edge write, the fixing step detects that it's a no-op and throws it away if we do this naively.

Instead, add some special cases around inverse edits to skip the correction/fixing logic, and just pass the "right" values in the first place.

Test Plan:
Added and removed related tasks from revisions, saw appropriate transactions render on both objects.

(It's hard to be certain this completely fixes the issue since it only happened occasionally in the first place, but we can see if it happens any more on `secure`.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13082, T222

Differential Revision: https://secure.phabricator.com/D19969

3 weeks agoProvide a richer error when an intracluster request can not be satisfied by the targe...
epriestley [Sun, 20 Jan 2019 17:27:48 +0000 (09:27 -0800)]
Provide a richer error when an intracluster request can not be satisfied by the target node

Summary: See PHI1030. When installs hit this error, provide more details about which node we ended up on and what's going on.

Test Plan:
```
$ git pull
phabricator-ssh-exec: This repository request (for repository "spellbook") has been incorrectly routed to a cluster host (with device name "local.phacility.net", and hostname "orbital-3.local") which can not serve the request.

The Almanac device address for the correct device may improperly point at this host, or the "device.id" configuration file on this host may be incorrect.

Requests routed within the cluster by Phabricator are always expected to be sent to a node which can serve the request. To prevent loops, this request will not be proxied again.

(This is a read request.)
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
```

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20002

3 weeks agoFix "Welcome Mail" check for a message when no message exists
epriestley [Sun, 20 Jan 2019 14:56:23 +0000 (06:56 -0800)]
Fix "Welcome Mail" check for a message when no message exists

Summary: Fixes T13239. See that task for discussion.

Test Plan: Tried to send welcome mail with no "Welcome" message.

Maniphest Tasks: T13239

Differential Revision: https://secure.phabricator.com/D20001

4 weeks agoTemporarily disable transaction story links in HTML mail for the deploy
epriestley [Sat, 19 Jan 2019 13:10:02 +0000 (05:10 -0800)]
Temporarily disable transaction story links in HTML mail for the deploy

Ref T12921. See that task for discussion. Behavioral revert of D19968.

4 weeks agoAllow Conduit method call logs to be exported with the standard export pipeline
epriestley [Fri, 18 Jan 2019 13:36:27 +0000 (05:36 -0800)]
Allow Conduit method call logs to be exported with the standard export pipeline

Summary:
See PHI1026. Allow installs to export Conduit call logs to a flat format.

Also, add date range queries.

Test Plan:
  - Exported some call logs.
  - Filtered logs by date.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19996

4 weeks agoUse the customizable "Welcome Mail" message in welcome mail
epriestley [Fri, 18 Jan 2019 03:05:45 +0000 (19:05 -0800)]
Use the customizable "Welcome Mail" message in welcome mail

Summary:
Depends on D19994. See PHI1027. If an install has customized the "Welcome Mail" message, include it in welcome mail. A special custom message from the profile screen overrides it, if provided.

(I fiddled with putting the custom message as "placeholder" text in the remarkup area as a hint, but newlines in "placeholder" text appear to have issues in Safari and Firefox. I think this is probably reasonably clear as-is.)

Make both render remarkup-into-text so things like links work properly, as it's reasonably likely that installs will want to link to things.

Test Plan:
  - With custom "Welcome Mail" text, sent mail with no custom override (got custom text) and a custom override (got overridden text).
  - Linked to some stuff, got sensible links in the mail (`bin/mail show-outbound`).

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19995

4 weeks agoShow the customized "Login" message on the login screen
epriestley [Fri, 18 Jan 2019 02:29:42 +0000 (18:29 -0800)]
Show the customized "Login" message on the login screen

Summary: Depends on D19992. Ref T13222. If administrators provide a custom login message, show it on the login screen.

Test Plan:
{F6137930}

  - Viewed login screen with and without a custom message.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19994

4 weeks agoAdd "Auth Messages" to support customizing onboarding/welcome flows
epriestley [Thu, 17 Jan 2019 18:47:13 +0000 (10:47 -0800)]
Add "Auth Messages" to support customizing onboarding/welcome flows

Summary:
Ref T13222. Long ago, we had a Config option (`welcome.html`) to let you dump HTML onto the login screen, but this was relatively hard to use and not good from a security perspective.

In some cases this was obsoleted by Dashboards, but there's at least some remaining set of use cases for actual login instructions on the login screen. For example, WMF has some guidance on //which// SSO mechanism to use based on what types of account you have. On `secure`, users assume they can register by clicking "Log In With GitHub" or whatever, and it might reduce frustration to tell them upfront that registration is closed.

Some other types of auth messaging could also either use customization or defaults (e.g., the invite/welcome/approve mail).

We could do this with a bunch of Config options, but I'd generally like to move to a world where there's less stuff in Config and more configuration is contextual. I think it tends to be easier to use, and we get a lot of fringe benefits (granular permissions, API, normal transaction logs, more abililty to customize workflows and provide contextual help/hints, etc). Here, for example, we can provide a remarkup preview, which would be trickier with Config.

This does not actually do anything yet.

Test Plan: {F6137541}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19992

4 weeks agoAllow administrators to provide custom welcome text when welcoming users on the profi...
epriestley [Thu, 17 Jan 2019 18:16:05 +0000 (10:16 -0800)]
Allow administrators to provide custom welcome text when welcoming users on the profile workflow

Summary:
See PHI1027. Currently, we allow you to customize invite email, but not most other types of email (approve, welcome). As a step forward, also allow welcome email to be customized with a message.

I considered separating the custom text from the main text with something heavyhanded ("alice added this custom message:") or a beautiful ASCII art divider like one of these:

https://www.asciiart.eu/art-and-design/dividers

...but nothing truly sung to me.

This only works on the profile flow for now. I'm planning to let you set a default message. I may or may not let you customize from "Create New User", seems like the default message probably covers most of that. Probably won't touch `scripts/user/add_user.php` since that's not really exactly super supported.

Test Plan:
Sent mail with and without custom messages, reviewed it with `bin/mail show-outbound`.

{F6137410}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19991

4 weeks agoWhen password auth is not enabled, don't tell users to set a password in welcome...
epriestley [Thu, 17 Jan 2019 18:02:42 +0000 (10:02 -0800)]
When password auth is not enabled, don't tell users to set a password in welcome email

Summary:
See PHI1027. Currently, the "Welcome" mail always tells users to set a password. This definitely isn't helpful if an install doesn't have password auth enabled.

We can't necessarily guess what they're supposed to do, so just give them generic instructions ("set up your account"). Upcoming changes will give administrators more control over the mail content.

Test Plan: Sent both versions of the mail, used `bin/mail show-outbound` to inspect them for correctness.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19990

4 weeks agoMove "Welcome" mail generation out of PhabricatorUser
epriestley [Thu, 17 Jan 2019 17:39:27 +0000 (09:39 -0800)]
Move "Welcome" mail generation out of PhabricatorUser

Summary:
Ref PHI1027. Currently, `PhabricatorUser` has a couple of mail-related methods which shouldn't really be there in the long term. Immediately, I want to make some adjusments to the welcome email.

Move "Welcome" mail generation to a separate class and consolidate all the error handling. (Eventually, "invite" and "verify address" email should move to similar subclasses, too.) Previously, a bunch of errors/conditions got checked in multiple places.

The only functional change is that we no longer allow you to send welcome mail to disabled users.

Test Plan:
  - Used "Send Welcome Mail" from profile pages to send mail.
  - Hit "not admin", "disabled user", "bot/mailing list" errors.
  - Used `scripts/user/add_user.php` to send welcome mail.
  - Used "Create New User" to send welcome mail.
  - Verified mail with `bin/mail show-outbound`. (Cleaned up a couple of minor display issues here.)

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19989

4 weeks agoAdd setup warnings for "local_infile" (MySQL Server) and "mysql[i].allow_local_infile...
epriestley [Fri, 18 Jan 2019 17:35:45 +0000 (09:35 -0800)]
Add setup warnings for "local_infile" (MySQL Server) and "mysql[i].allow_local_infile" (PHP Client)

Summary: Ref T13238. Warn users about these horrible options and encourage them to defuse them.

Test Plan: Hit both warnings, fixed the issues, issues went away.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13238

Differential Revision: https://secure.phabricator.com/D19999

4 weeks agoPut a hard limit on password login attempts from the same remote address
epriestley [Fri, 18 Jan 2019 13:50:52 +0000 (05:50 -0800)]
Put a hard limit on password login attempts from the same remote address

Summary:
Ref T13222. Currently, if a remote address fails a few login attempts (5) in a short period of time (15 minutes) we require a CAPTCHA for each additional attempt.

This relies on:

  - Administrators configuring ReCAPTCHA, which they may just not bother with.
  - Administrators being comfortable with Google running arbitrary trusted Javascript, which they may not be comfortable with.
  - ReCAPTCHA actually being effective, which seems likely true for unsophisticated attackers but perhaps less true for more sophisticated attackers (see <https://github.com/ecthros/uncaptcha2>, for example).

(For unsophisticated attackers and researchers, "Rumola" has been the standard CAPTCHA bypass tool for some time. This is an extension that pays humans to solve CAPTCHAs for you. This is not practical at "brute force a strong password" scale. Google appears to have removed it from the Chrome store. The "submit the captcha back to Google's APIs" trick probably isn't practical at brute-force-scale either, but it's easier to imagine weaponizing that than weaponizing human solvers.)

Add a hard gate behind the CAPTHCA wall so that we fail into a secure state if there's no CAPTCHA or the attacker can defeat CAPTCHAs at a very low cost.

The big downside to this is that an attacker who controls your remote address (e.g., is behind the same NAT device you're behind on corpnet) can lock you out of your account. However:

  - That //should// be a lot of access (although maybe this isn't that high of a barrier in many cases, since compromising a "smart fridge" or "smart water glass" or whatever might be good enough).
  - You can still do "Forgot password?" and login via email link, although this may not be obvious.

Test Plan:
  - Logged in normally.
  - Failed many many login attempts, got hard gated.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19997

4 weeks agoRemove "metamta.*.subject-prefix" options
epriestley [Thu, 17 Jan 2019 19:52:03 +0000 (11:52 -0800)]
Remove "metamta.*.subject-prefix" options

Summary:
In ~2012, the first of these options was added because someone who hates dogs and works at Asana also hated `[Differential]` in the subject line. The use case there was actually //removing// the text, not changing it, but I made the prefix editable since it seemed like slightly less of a one-off.

These options are among the dumbest and most useless config options we have and very rarely used, see T11760. A very small number of instances have configured one of these options.

Newer applications stopped providing these options and no one has complained.

You can get the same effect with `translation.override`. Although I'm not sure we'll keep that around forever, it's a reasonable replacement today. I'll call out an example in the changelog to help installs that want to preserve this option.

If we did want to provide this, it should just be in {nav Applications > Settings} for each application, but I think it's wildly-low-value and "hack via translations" or "local patch" are entirely reasonable if you really want to change these strings.

Test Plan: Grepped for `subject-prefix`.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19993

4 weeks agoDon't bounce mail messages if any recipient was reserved
epriestley [Thu, 17 Jan 2019 14:38:11 +0000 (06:38 -0800)]
Don't bounce mail messages if any recipient was reserved

Summary:
Ref T13222. If we receive a message and nothing processes it, we normally try to send the user an error message like "hey, nothing handled this, maybe you got the address wrong".

Just skip the "send them an error message" part if any recipient was reserved, so if you "Reply All" to a message that is "From: noreply@phabricator" you don't get a relatively unhelpful error.

This also makes sure that the "void" address doesn't generate bounces if the "From" is a valid user email address (e.g., with `metamta.can-send-as-user`). That is:

  - Phabricator needs to send a mail with only "CC" users.
  - Phabricator puts the "void" address in "To" as a placeholder.
  - The "void" address happens to route back to Phabricator.

We don't want that mail to bounce to anywhere. Normally, it won't:

  - From is usually "noreply@phabricator", which isn't a user, so we won't send anything back: we only send mail to verified user email addresses.
  - The message will have "X-Phabricator-Sent-This-Message: true" so we won't process it at all.

...but this is another layer of certainty.

Test Plan: Used `bin/mail receive-test` to receive mail to an invalid, unreserved address (bounce/error email) and an invalid, reserved address (no bounce/error email).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19987

4 weeks agoAllow Phortune accounts to customize their billing address and name
epriestley [Wed, 16 Jan 2019 14:59:06 +0000 (06:59 -0800)]
Allow Phortune accounts to customize their billing address and name

Summary:
See PHI1023. Ref T7607. Occasionally, companies need their billing address (or some other custom text) to appear on invoices to satisfy process or compliance requirements.

Allow accounts to have a custom "Billing Name" and a custom "Billing Address" which appear on invoices.

Test Plan: {F6134707}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T7607

Differential Revision: https://secure.phabricator.com/D19979

4 weeks agoFix an issue where "Import Columns" could fail on a board for a project with milestones
epriestley [Wed, 16 Jan 2019 14:36:10 +0000 (06:36 -0800)]
Fix an issue where "Import Columns" could fail on a board for a project with milestones

Summary:
See PHI1025. When you "Import Columns", we test if you're trying to import into a board that already has columns. However, this test is too broad (it incorrectly detects "proxy" columns for milestones as columns) and not user-friendly (it returns 400 instead of a readable error).

Correct these issues, and refine some of the logic around proxy columns.

Test Plan:
  - Created a project, A.
  - Created a milestone under that project.
  - Imported another project's columns to A's workboard.
    - Before change: Unhelpful 400.
    - After change: import worked fine.
  - Also, hit the new error dialogs and read through them.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19978

4 weeks agoWordsmith inbound mail documentation more thoroughly
epriestley [Wed, 16 Jan 2019 22:27:34 +0000 (14:27 -0800)]
Wordsmith inbound mail documentation more thoroughly

Summary: See D19973. Fix a couple typos and try to make some sections more clear / less scary.

Test Plan: Read text.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19986

4 weeks agoUpdate inbound and outbound email documentation
epriestley [Mon, 14 Jan 2019 23:19:29 +0000 (15:19 -0800)]
Update inbound and outbound email documentation

Summary: Fixes T8636. Mention Herald for inbound, update some outbound stuff, do some language / organization tweaks.

Test Plan: Read documentation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8636

Differential Revision: https://secure.phabricator.com/D19973

4 weeks agoPrevent application email addresses from shadowing user email addresses
epriestley [Tue, 15 Jan 2019 14:29:34 +0000 (06:29 -0800)]
Prevent application email addresses from shadowing user email addresses

Summary:
Fixes T13234. Don't let application email addresses be configured with user addresses. This might prevent an unlikely bit of mischief where someone does this intentionally, detailed in T13234.

(Possibly, these tables should just be merged some day, similar to how the "Password" table is now a shared resource that's modular enough for multiple applications to use it.)

Test Plan: {F6132259}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13234

Differential Revision: https://secure.phabricator.com/D19974

4 weeks agoReorder "Merge" transaction to make "Close as Duplicate" produce a "[Merged]" email...
epriestley [Mon, 14 Jan 2019 22:32:01 +0000 (14:32 -0800)]
Reorder "Merge" transaction to make "Close as Duplicate" produce a "[Merged]" email subject

Summary:
Fixes T11782. When you "Close as Duplicate", generate a "[Merged]" email by making the merge the first transaction.

(There are other, more-deterministic ways to do this with action strength, but this is much simpler and I believe it suffices.)

Test Plan: Used "Close as Duplicate", got a "[Merged]" email out of it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11782

Differential Revision: https://secure.phabricator.com/D19972

4 weeks agoAdd a Twilio SMS message adapter
epriestley [Wed, 2 Jan 2019 03:04:43 +0000 (19:04 -0800)]
Add a Twilio SMS message adapter

Summary: Ref T920. Adds a "phone number" object, an "SMS" message type, and Twilio glue.

Test Plan:
Used this test script to send myself some text messages after configuring Twilio in `cluster.mailers`.

```
<?php

require_once 'scripts/init/init-script.php';

if ($argc < 3) {
  throw new Exception('usage: test.php <number> <body>');
}
$to_number = $argv[1];
$text_body = $argv[2];

$mailers = PhabricatorMetaMTAMail::newMailers(
  array(
    'outbound' => true,
    'media' => array(
      PhabricatorMailSMSMessage::MESSAGETYPE,
    ),
  ));
if (!$mailers) {
  return new Aphront404Response();
}
$mailer = head($mailers);

$message = id(new PhabricatorMailSMSMessage())
  ->setToNumber(new PhabricatorPhoneNumber($to_number))
  ->setTextBody($text_body);

$mailer->sendMessage($message);
```

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D19971

4 weeks agoFix an issue where "CC"-only email improperly wiped CC addresses
epriestley [Mon, 14 Jan 2019 21:31:42 +0000 (13:31 -0800)]
Fix an issue where "CC"-only email improperly wiped CC addresses

Summary: Ref T920. See <https://discourse.phabricator-community.org/t/ccd-emails-not-working-with-sendgrid-since-2019-week-1-update/2294>.

Test Plan: Used `bin/mail send-test --cc ...` without `--to`, got email.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D19970

4 weeks agoFix an issue where transactions in mail were always rendered as text
epriestley [Mon, 14 Jan 2019 17:52:41 +0000 (09:52 -0800)]
Fix an issue where transactions in mail were always rendered as text

Summary:
Fixes T12921. Currently, we call `getTitleForHTMLMail()`, but that calls `getTitleForMail()` which forces us into text rendering mode.

Instead, have `getTitleForHTML/TextMail()` force the rendering mode, then call `getTitleForMail()` with the desired rendering mode.

This causes stories like "epriestely added dependent tasks: x, y." to appear as links in email instead of plain text.

Test Plan: Used `bin/mail show-outbound --id ... --dump-html > out.html` to verify HTML mail.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12921

Differential Revision: https://secure.phabricator.com/D19968

4 weeks agoUpdate the "SES" and "sendmail" mailers for the new API; remove "encoding"
epriestley [Mon, 14 Jan 2019 15:50:20 +0000 (07:50 -0800)]
Update the "SES" and "sendmail" mailers for the new API; remove "encoding"

Summary: Ref T13222. Ref T920. This is the last of the upstream adapter updates.

Test Plan:
  - Sent mail with SES.
  - Sent mail with "sendmail". I don't have sendmail actually configured to an upstream MTA so I'm not 100% sure this worked, but the `sendmail` binary didn't complain and almost all of the code is shared with SES, so I'm reasonably confident this actually works.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222, T920

Differential Revision: https://secure.phabricator.com/D19965

4 weeks agoUpdate the SMTP (PHPMailer) adapter for the new mail API; remove "encoding" and ...
epriestley [Sat, 5 Jan 2019 14:57:06 +0000 (06:57 -0800)]
Update the SMTP (PHPMailer) adapter for the new mail API; remove "encoding" and "mailer"

Summary:
Ref T920. Ref T12404.

  - Update to the new "$message" API.
  - Remove "encoding". I believe "base64" is always the best value for this since we stopped seeing issues once we changed the default.
  - Remove "mailer". This is a legacy option that makes little sense given how configuration now works.
  - Rename to "SMTP". This doesn't affect users anymore since this mailer has been configured as `smtp` for about a year.
  - This does NOT add a timeout since the SMTP code is inside PHPMailer (see T12404).

Test Plan: Sent messages with many mail features via GMail SMTP and SendGrid SMTP.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12404, T920

Differential Revision: https://secure.phabricator.com/D19961

4 weeks agoDon't require "CAN_EDIT" to watch/unwatch a project
epriestley [Wed, 16 Jan 2019 13:51:59 +0000 (05:51 -0800)]
Don't require "CAN_EDIT" to watch/unwatch a project

Summary:
See T1024. When "CAN_EDIT" became default in T13186, this was missed as an exception.

Watching shouldn't require "CAN_EDIT", so exempt it.

Test Plan:
  - Before change: tried to watch a project I could not edit, got a policy error.
  - After change: watched/unwatched a project I could not edit.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19977

4 weeks agoUpgrade Sendgrid to the modern mailer API; removes "api-user" option
epriestley [Sat, 5 Jan 2019 14:30:22 +0000 (06:30 -0800)]
Upgrade Sendgrid to the modern mailer API; removes "api-user" option

Summary:
Ref T920. Ref T5969.

  - Update to the new "$message" API.
  - Update to Sendgrid v3.
  - Add a timeout.
  - This removes the "api-user" option, which Sendgrid no longer seems to use.

Test Plan: Sent Sendgrid messages with `bin/mail send-test ...` using subject/headers/attachments/html/to/cc.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: jbrownEP

Maniphest Tasks: T5969, T920

Differential Revision: https://secure.phabricator.com/D19960

4 weeks agoUpdate Mailgun adapter for the new mail adapter API
epriestley [Sat, 5 Jan 2019 12:53:49 +0000 (04:53 -0800)]
Update Mailgun adapter for the new mail adapter API

Summary: Ref T920. Ref T5969. Update the Mailgun adapter for the API changes and add a timeout.

Test Plan: Configured Mailgun as a mailer, sent mail with subject/to/cc/headers/html/attachments using `bin/mail send-test`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5969, T920

Differential Revision: https://secure.phabricator.com/D19959

4 weeks agoUpdate Mail test adapter for the newer adapter API and make all tests pass
epriestley [Fri, 4 Jan 2019 20:23:52 +0000 (12:23 -0800)]
Update Mail test adapter for the newer adapter API and make all tests pass

Summary: Depends on D19956. Ref T920. Move the TestAdapter to the new API and adjust a couple of tests for the changes.

Test Plan: All tests now pass.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D19957

4 weeks agoUpdate Postmark adapter for multiple mail media
epriestley [Fri, 4 Jan 2019 13:40:26 +0000 (05:40 -0800)]
Update Postmark adapter for multiple mail media

Summary:
Depends on D19955. Ref T920. Ref T5969. Update Postmark to accept new Message objects. Also:

  - Update the inbound whitelist.
  - Add a little support for `media` configuration.
  - Add a service call timeout (see T5969).
  - Drop the needless word "Implementation" from the Adapter class tree. I could call these "Mailers" instead of "Adapters", but then we get "PhabricatorMailMailer" which feels questionable.

Test Plan: Used `bin/mail send-test` to send mail via Postmark with various options (mulitple recipients, text vs html, attachments).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5969, T920

Differential Revision: https://secure.phabricator.com/D19956

4 weeks agoRefactor mail to produce an intermediate "bag of strings" object in preparation for SMS
epriestley [Fri, 4 Jan 2019 13:45:38 +0000 (05:45 -0800)]
Refactor mail to produce an intermediate "bag of strings" object in preparation for SMS

Summary:
Depends on D19954. Ref T920. This is a step toward a world where "Mailers" are generic and may send messages over a broader array of channels (email, SMS, postal mail).

There are a few major parts here:

  - Instead of calling `$mailer->setSubject()`, `$mailer->setFrom()`, etc., build in intermediate `$message` object first, then pass that to the mailer.
    - This breaks every mailer! This change on its own does not fix them. I plan to fix them in a series of "update mailer X", "update mailer Y" followups.
    - This generally makes the API easier to change in the far future, and in the near future supports mailers accepting different types of `$message` objects with the same API.
  - Pull the "build an email" stuff out into a `PhabricatorMailEmailEngine`. `MetaMTAMail` is already a huge object without also doing this translation step. This is just a separation/simplification change, but also tries to fight against `MetaMTAMail` getting 5K lines of email/sms/whatsapp/postal-mail code.
  - Try to rewrite the "build an email" stuff to be a bit more straightforward while making it generate objects. Prior to this change, it had this weird flow:

```lang=php
foreach ($properties as $key => $prop) {
  switch ($key) {
    case 'xyz':
      // ...
  }
}
```

This is just inherently somewhat hard to puzzle out, and it means that processing order depends on internal property order, which is quite surprising.

Test Plan: This breaks everything on its own; adapters must be updated to use the new API. See followups.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D19955

4 weeks agoPrevent inbound processing of the "void/placeholder" address and other reserved addresses
epriestley [Thu, 3 Jan 2019 23:45:58 +0000 (15:45 -0800)]
Prevent inbound processing of the "void/placeholder" address and other reserved addresses

Summary:
Depends on D19952. Ref T13222. Never process mail targets if they match:

  - The "default" address which we send mail "From".
  - The "void" address which we use as a placholder "To" when we only have "CC" addresses.
  - Any address from a list of reserved/administrative names.

The first two prevent loops. The third one prevents abuse.

There's a reasonably well-annotated list of reservations and reasons here:

https://webmasters.stackexchange.com/questions/104811/is-there-any-list-of-email-addresses-reserved-because-of-security-concerns-for-a

Stuff like `support@` seems fine; stuff like `ssladmin@` might let you get SSL certs issued for a domain you don't control.

Also, forbid users from creating application emails with these reserved addresses.

Finally, build the default and void addresses somewhat more cleverly.

Test Plan: Added unit tests, tried to configured reserved addresses, hit the default/void cases manually with `bin/mail receive-test`.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: olexiy.myronenko

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19953

4 weeks agoAllow multiple mail receivers to react to an individual email
epriestley [Thu, 3 Jan 2019 13:08:19 +0000 (05:08 -0800)]
Allow multiple mail receivers to react to an individual email

Summary:
Fixes T7477. Fixes T13066. Currently, inbound mail is processed by the first receiver that matches any "To:" address. "Cc" addresses are ignored.

**To, CC, and Multiple Receivers**

Some users would like to be able to "Cc" addresses like `bugs@` instead of having to "To" the address, which makes perfect sense. That's the driving use case behind T7477.

Since users can To/Cc multiple "create object" or "update object" addresses, I also wanted to make the behavior more general. For example, if you email `bugs@` and also `paste@`, your mail might reasonably make both a Task and a Paste. Is this useful? I'm not sure. But it seems like it's pretty clearly the best match for user intent, and the least-surprising behavior we can have. There's also no good rule for picking which address "wins" when two or more match -- we ended up with "address order", which is pretty arbitrary since "To" and "Cc" are not really ordered fields.

One part of this change is removing `phabricator.allow-email-users`. In practice, this option only controlled whether users were allowed to send mail to "Application Email" addresses with a configured default author, and it's unlikely that we'll expand it since I think the future of external/grey users is Nuance, not richer interaction with Maniphest/Differential/etc. Since this option only made "Default Author" work and "Default Author" is optional, we can simplify behavior by making the rule work like this:

  - If an address specifies a default author, it allows public email.
  - If an address does not, it doesn't.

That's basically how it worked already, except that you could intentionally "break" the behavior by not configuring `phabricator.allow-email-users`. This is a backwards compatility change with possible security implications (it might allow email in that was previously blocked by configuration) that I'll call out in the changelog, but I suspect that no installs are really impacted and this new behavior is generally more intuitive.

A somewhat related change here is that each receiver is allowed to react to each individual email address, instead of firing once. This allows you to configure `bugs-a@` and `bugs-b@` and CC them both and get two tasks. Useful? Maybe not, but seems like the best execution of intent.

**Sender vs Author**

Adjacently, T13066 described an improvement to error handling behavior here: we did not distinguish between "sender" (the user matching the email "From" address) and "actor" (the user we're actually acting as in the application). These are different when you're some internet rando and send to `bugs@`, which has a default author. Then the "sender" is `null` and the "author" is `@bugs-robot` or whatever (some user account you've configured).

This refines "Sender" vs "Author". This is mostly a purity/correctness change, but it means that we won't send random email error messages to `@bugs-robot`.

Since receivers are now allowed to process mail with no "sender" if they have some default "actor" they would rather use instead, it's not an error to send from an invalid address unless nothing processes the mail.

**Other**

This removes the "abundant receivers" error since this is no longer an error.

This always sets "external user" mail recipients to be unverified. As far as I can tell, there's no pathway by which we send them email anyway (before or after this change), although it's possible I'm missing something somewhere.

Test Plan:
I did most of this with `bin/mail receive-test`. I rigged the workflow slightly for some of it since it doesn't support multiple addresses or explicit "CC" and adding either would be a bit tricky.

These could also be tested with `scripts/mail/mail_handler.php`, but I don't currently have the MIME parser extension installed locally after a recent upgrade to Mojave and suspect T13232 makes it tricky to install.

- Ran unit tests, which provide significant coverage of this flow.
- Sent mail to multiple Maniphest application emails, got multiple tasks.
- Sent mail to a Maniphest and a Paste application email, got a task and a paste.
- Sent mail to a task.
  - Saw original email recorded on tasks. This is a behavior particular to tasks.
- Sent mail to a paste.
- Sent mail to a mock.
- Sent mail to a Phame blog post.
- Sent mail to a Legalpad document.
- Sent mail to a Conpherence thread.
- Sent mail to a poll.
- This isn't every type of supported object but it's enough of them that I'm pretty confident I didn't break the whole flow.
- Sent mail to an object I could not view (got an error).
- As a non-user, sent mail to several "create an object..." addresses.
  - Addresses with a default user worked (e.g., created a task).
  - Addresses without a default user did not work.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13066, T7477

Differential Revision: https://secure.phabricator.com/D19952

4 weeks agoAdd a skeleton for configurable MFA provider types
epriestley [Sat, 29 Dec 2018 00:52:00 +0000 (16:52 -0800)]
Add a skeleton for configurable MFA provider types

Summary:
Ref T13222. Ref T13231. See PHI912. I'm planning to turn MFA providers into concrete objects, so you can disable and configure them.

Currently, we only support TOTP, which doesn't require any configuration, but other provider types (like Duo or Yubikey OTP) do require some configuration (server URIs, API keys, etc). TOTP //could// also have some configuration, like "bits of entropy" or "allowed window size" or whatever, if we want.

Add concrete objects for this and standard transaction / policy / query support. These objects don't do anything interesting yet and don't actually interact with MFA, this is just skeleton code for now.

Test Plan:
{F6090444}

{F6090445}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13231, T13222

Differential Revision: https://secure.phabricator.com/D19935

4 weeks agoResurrect setup check for cluster.mailers
Austin McKinley [Fri, 11 Jan 2019 22:21:11 +0000 (14:21 -0800)]
Resurrect setup check for cluster.mailers

Summary:
D19940 removed this file entirely, which has led to at least one user who was unsure how to proceed now that `cluster.mailers` is required for outbound mail: https://discourse.phabricator-community.org/t/invalid-argument-supplied-for-foreach-phabricatormetamtamail-php/2287

This isn't //always// a setup issue for installs that don't care about sending mail, but this at least this gives a sporting chance to users who don't follow the changelogs.

Also, I'm not sure if there's a way to use `pht()` to generate links; right now the phurl is just in plain text.

Test Plan: Removed `cluster.mailers` config; observed expected setup issue.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19964

4 weeks agoCorrect a zero-based month tooltip on burnup charts
epriestley [Mon, 14 Jan 2019 17:26:37 +0000 (09:26 -0800)]
Correct a zero-based month tooltip on burnup charts

Summary: See PHI1017. This is a trivial fix even though these burnups are headed toward a grisly fate.

Test Plan: Moused over some January datapoints, saw "1" instead of "0".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19967

4 weeks agoFix bad "SMTP" and "cluster.mailers" default value
epriestley [Mon, 14 Jan 2019 17:05:40 +0000 (09:05 -0800)]
Fix bad "SMTP" and "cluster.mailers" default value

Summary: See note in D19964.

Test Plan: O_o

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19966