1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 00:42:41 +01:00
Commit graph

15704 commits

Author SHA1 Message Date
epriestley
5537e29ee8 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
2019-01-18 19:50:35 -08:00
epriestley
98bf3a950d 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
2019-01-18 19:49:48 -08:00
epriestley
310ad7f8f4 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
2019-01-18 19:48:42 -08:00
epriestley
c125ab7a42 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
2019-01-17 19:18:50 -08:00
epriestley
ff220acae6 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
2019-01-17 19:17:37 -08:00
epriestley
6b6c991ad4 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
2019-01-16 16:16:27 -08:00
epriestley
a1516fefb6 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
2019-01-16 16:15:45 -08:00
epriestley
0a0afa489a 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
2019-01-16 14:44:57 -08:00
epriestley
bd077bfcb7 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
2019-01-16 13:56:32 -08:00
epriestley
c5f446defb 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
2019-01-16 13:28:08 -08:00
epriestley
dc4d7f1f3e 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
2019-01-16 13:27:10 -08:00
epriestley
35f0e31ed3 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
2019-01-16 13:25:59 -08:00
epriestley
96d3e73eed 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
2019-01-16 13:22:43 -08:00
epriestley
0c0cbb1c09 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
2019-01-16 13:21:05 -08:00
epriestley
c3cafffed7 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
2019-01-16 13:18:55 -08:00
epriestley
43a6f34e7f 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
2019-01-16 13:10:57 -08:00
epriestley
966a93334c 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
2019-01-16 13:09:59 -08:00
epriestley
64e3296fe6 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
2019-01-16 13:09:29 -08:00
epriestley
d7da3560ec 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
2019-01-16 13:02:04 -08:00
epriestley
bc97a7d755 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
2019-01-16 13:01:25 -08:00
epriestley
a8657e6ab6 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
2019-01-16 13:00:34 -08:00
epriestley
b5797ce60a 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
2019-01-16 12:58:29 -08:00
epriestley
a37b28ef79 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
2019-01-16 12:28:53 -08:00
epriestley
e3aa043a02 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
2019-01-16 12:28:02 -08:00
epriestley
a62f334d95 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
2019-01-16 12:27:23 -08:00
Austin McKinley
b98d46ce7d 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
2019-01-16 12:14:36 -08:00
epriestley
3b94b3e812 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
2019-01-15 18:09:18 -08:00
epriestley
0b8f24dfd3 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
2019-01-14 13:10:33 -08:00
Marc Lindenberg
360ebce00f call qsprintf() without warnings
Summary:
After T13217 import_repository_symbols.php was showing a lot of warnings, using %LQ fixes that.
I'm aware, that there are changes planned to the whole managing the symbols complex but until then less warnings are nice.

Test Plan: No more warnings when updating symbols

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D19962
2019-01-14 09:41:31 -08:00
Austin McKinley
e8069dfe31 Remove dead symlink
Summary: Fixes https://discourse.phabricator-community.org/t/broken-symbolic-link/2284

Test Plan: Mk. 1 eyeball

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19963
2019-01-10 16:27:45 -08:00
epriestley
73e3057c52 Rename "MetaMTA" mail attachments and add more mail message objects
Summary:
Depends on D19953. Ref T9141. We have a "MetaMTAAttachment" object, rename it to "MailAttachment".

Also add a "Header" object and an "EmailMessage" object. Currently, mail adapters have a large number of methods like `setSubject()`, `addTo()`, etc, that I would like to remove.

I'd like the API to be more like `sendMessage(PhabricatorMailExternalMessage $message)`. This is likely a significant simplification anyway, since the implementations of all these methods are just copy/pasted boilerplate anyway (lots of `$this->subject = $subject;`) and this will let Adapters support other message media (SMS, APNS, Whatsapp, etc.)

That's a larger change, but move toward a world where we can build a concrete `$message` object for "email" or "sms".

The `PhabricatorMailEmailMessage` object is just a dumb, flat object representation of the information an adapter needs to actually send mail. The existing `PhabricatorMetaMTAMail` is a much more complex object and has a lot of rich data (delivery status, related object PHIDs, etc) and is a storage object.

The new flow will be something like:

  - `PhabricatorMetaMTAMail` (possibly renamed) is the storage object for any outbound message on this channel. It tracks message content, acceptable delivery media (SMS vs email), delivery status, related objects, has a PHID, and has a daemon worker associated with delivering it.
  - It builds a `PhabricatorMailExternalMessage`, which is a simple, flat description of the message it wants to send. The subclass of this object depends on the message medium. For email, this will be an `EmailMessage`. This is just a "bag of strings" sort of object, with appropriate flattened values for the adapter to work with (e.g., Email has email addresses, SMS has phone numbers).
  - It passes the `ExternalMessage` (which is a `MailMessage` or `SMSMessage` or whatever) to the adapter.
  - The adapter reads the nice flat properties off it and turns them into an API request, SMTP call, etc.

This is sort of how things work today anyway. The major change is that I want to hand off a "bag of strings" object instead of calling `setX()`, `setY()`, `setZ()` with each individual value.

Test Plan: Grepped for `MetaMTAAttachment`. This doesn't change any behavior yet.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T9141

Differential Revision: https://secure.phabricator.com/D19954
2019-01-04 15:23:44 -08:00
epriestley
dda3ff89e0 Consolidate some application email receiver code in preparation for API changes
Summary:
Ref T7477. The various "create a new X via email" applications (Paste, Differential, Maniphest, etc) all have a bunch of duplicate code.

The inheritance stack here is generally a little weird. Extend these from a shared parent to reduce the number of callsites I need to change when this API is adjusted for T7477.

Test Plan: Ran unit tests. This will get more thorough testing once more pieces are in place.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7477

Differential Revision: https://secure.phabricator.com/D19950
2019-01-04 15:21:50 -08:00
epriestley
e48c36697a Make blame UI recover gracefully if Identities haven't been built yet for a commit
Summary:
See PHI1014. We may not have Identities if you race the import pipeline, or in some other cases which are more "bug" / "missing migration"-flavored.

Load the commit data so we can fall back to it if we don't have identities.

Test Plan:
  - Wiped out all my identities with `UPDATE ... SET authorIdentityPHID = NULL WHERE ...`.
  - Before change: blame fataled with `Attempting to access attached data on PhabricatorRepositoryCommit (via getCommitData()), but the data is not actually attached.`.
  - After change: blame falls back gracefully.
  - Restored identities with `bin/repository rebuild-identities`, checked blame again.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19958
2019-01-04 15:15:12 -08:00
epriestley
84f94994ad Remove "metamta.insecure-auth-with-reply-to" Config option
Summary:
Ref T7477. This option was added in D842 in 2011, to support a specific narrow use case at Quora with community moderators using some kind of weird Gmail config.

I don't recall it ever coming up since then, and a survey of a subset of hosted instances (see T11760) reveals that no instances are using this option today. Presumably, even Quora has completed the onboarding discussed in D842, if they still use Phabricator. This option generally does not seem very useful outside of very unusual/narrow cases like the one Quora had.

This would be relatively easy to restore as a local patch if installs //do// need it, but I suspect this has no use cases anywhere.

Test Plan: Grepped for option, blame-delved to figure out why we added it in the first place, surveyed instances for usage.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7477

Differential Revision: https://secure.phabricator.com/D19949
2019-01-04 13:56:39 -08:00
epriestley
57b3619181 Extract some email address utility code from the receiver stack
Summary:
Ref T7477. We have some address normalization code in the reciever stack that is really shared code. I want to introduce some new callsites elsewhere but don't want to put a lot of static calls to other random objects all over the place.

This technically "solves" T7477 (it changes "to" to "to + cc" for finding receivers) but doesn't yet implement proper behavior (with multiple receivers, for example).

Test Plan: Ran unit tests, which cover this pretty well. Additional changes will vet this more thoroughly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7477

Differential Revision: https://secure.phabricator.com/D19948
2019-01-04 13:55:49 -08:00
epriestley
e2f0571104 Drop empty inbound mail at the beginning of the receive workflow, not inside object handlers
Summary:
Ref T920. Ref T7477. We currently drop empty mail only once it reaches the `ReplyHandler` layer.

I think no plausible receiver can ever do anything useful with this kind of mail, so we can safely drop it earlier and simplify some of the logic. After T7477, we'd end up throwing multiple exceptions if you sent empty mail to several valid receivers.

(I also want to move away from APIs oriented around raw addresses in more specialized layers, and this is one of the few callsites for raw mail address information.)

This requires updating some unit tests to actually have message bodies, since they failed with this error before hitting the other errors otherwise.

Test Plan: Used `bin/mail receive-test` to send empty mail, got appropriate "err:empty" out of it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7477, T920

Differential Revision: https://secure.phabricator.com/D19947
2019-01-04 13:50:21 -08:00
epriestley
1f4cf23455 Remove "phabricator.csrf-key" and upgrade CSRF hashing to SHA256
Summary:
Ref T12509.

  - Remove the "phabricator.csrf-key" configuration option in favor of automatically generating an HMAC key.
  - Upgrade two hasher callsites (one in CSRF itself, one in providing a CSRF secret for logged-out users) to SHA256.
  - Extract the CSRF logic from `PhabricatorUser` to a standalone engine.

I was originally going to do this as two changes (extract logic, then upgrade hashes) but the logic had a couple of very silly pieces to it that made faithful extraction a little silly.

For example, it computed `time_block = (epoch + (offset * cycle_frequency)) / cycle_frequency` instead of `time_block = (epoch / cycle_frequency) + offset`. These are equivalent but the former was kind of silly.

It also computed `substr(hmac(substr(hmac(secret)).salt))` instead of `substr(hmac(secret.salt))`. These have the same overall effect but the former is, again, kind of silly (and a little bit materially worse, in this case).

This will cause a one-time compatibility break: pages loaded before the upgrade won't be able to submit contained forms after the upgrade, unless they're open for long enough for the Javascript to refresh the CSRF token (an hour, I think?). I'll note this in the changelog.

Test Plan:
  - As a logged-in user, submitted forms normally (worked).
  - As a logged-in user, submitted forms with a bad CSRF value (error, as expected).
  - As a logged-out user, hit the success and error cases.
  - Visually inspected tokens for correct format.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D19946
2019-01-04 13:49:47 -08:00
epriestley
93e6dc1c1d Upgrade object reply addresses to SHA256 and remove "phabricator.mail-key"
Summary:
Ref T12509.

  - Upgrade an old SHA1 to SHA256.
  - Replace an old manually configurable HMAC key with an automatically generated one.

This is generally both simpler (less configuration) and more secure (you now get a unique value automatically).

This causes a one-time compatibility break that invalidates old "Reply-To" addresses. I'll note this in the changelog.

If you leaked a bunch of addresses, you could force a change here by mucking around with `phabricator_auth.auth_hmackey`, but AFAIK no one has ever used this value to react to any sort of security issue.

(I'll note the possibility that we might want to provide/document this "manually force HMAC keys to regenerate" stuff some day in T6994.)

Test Plan: Grepped for removed config. I'll vet this pathway more heavily in upcoming changes.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D19945
2019-01-04 13:47:35 -08:00
epriestley
a0668df75a Remove "metamta.domain" and "metamta.placeholder-to-recipient" config options
Summary:
Ref T920. This simplifies mail configuration.

The "metamta.domain" option is only used to generate Thread-ID values, and we just need something that looks like a bit like a domain in order to make GMail happy. Just use the install domain. In most cases, this is almost certainly the configured value anyway. In some cases, this may cause a one-time threading break for existing threads; I'll call this out in the changelog.

The "metamta.placeholder-to-recipient" is used to put some null value in "To:" when a mail only has CCs. This is so that if you write a local client mail rule like "when I'm in CC, burn the message in a fire" it works even if all the "to" addresses have elected not to receive the mail. Instead: just send it to an unlikely address at our own domain.

I'll add some additional handling for the possiblity that we may receive this email ourselves in the next change, but it overlaps with T7477.

Test Plan: Grepped for these configuration values.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D19942
2019-01-04 13:45:15 -08:00
epriestley
afa69eedd1 Remove an old digest in Celerity code and some obsolete configuration options
Summary:
Ref T12509. This upgrades a `weakDigest()` callsite to SHA256-HMAC and removes three config options:

  - `celerity.resource-hash`: Now hard-coded, since the use case for ever adjusting it was very weak.
  - `celerity.enable-deflate`: Intended to make cache inspection easier, but we haven't needed to inspect caches in ~forever.
  - `celerity.minify`: Intended to make debugging minification easier, but we haven't needed to debug this in ~forever.

In the latter two cases, the options were purely developer-focused, and it's easy to go add an `&& false` somewhere in the code if we need to disable these features to debug something, but the relevant parts of the code basically work properly and never need debugging. These options were excessively paranoid, based on the static resource enviroment at Facebook being far more perilous.

The first case theoretically had end-user utility for fixing stuck content caches. In modern Phabricator, it's not intuitive that you'd go adjust a Config option to fix this. I don't recall any users ever actually running into problems here, though.

(An earlier version of this change did more magic with `celerity.resource-hash`, but this ended up with a more substantial simplification.)

Test Plan: Grepped for removed configuration options.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D19941
2019-01-04 13:43:38 -08:00
epriestley
7e87d254ab Add a parameterized Future for Twilio API calls
Summary: Ref T920. We currently embed the Twilio PHP API, but can replace it with about 100 lines of code and get a future-oriented interface as a bonus. Add a Future so we can move toward a simpler calling convention for the API.

Test Plan: Used this future to send SMS messages via the Twilio API.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D19937
2019-01-04 13:42:52 -08:00
epriestley
3963c86ad5 Pass timeline view data to comment previews, restoring Differential comment previews
Summary:
Ref T13222. In D19918, I refactored how timelines get "view data". Today, this is always additional data about which images/changesets/diffs are visible on the current revision/commit/mock, so we can tell if inline comments should be linked to a `#anchor` on the same page (if the inline is rendered there somewhere) or to a `/D123?id=1&vs=2` full link on a different page (if it isn't), but in general this could be any sort of state information about the current page that affects how the timeline should render.

Previously, comment previews did not use any specialized object code and always rendered a "generic" timeline story. This was actually a bug, but none of the code we have today cares about this (since it's all inline related, and inlines render separately) so it never impacted anything.

After the `TimelineEngine` change, the preview renders with Differential-specific code. This is more correct, but we were not passing the preview the "view data" so it broke.

This preview doesn't actually need the view data and we could just make it bail out if it isn't present, but pass it through for consistency and so this works like we'd expect if we do something fancier with view data in the future.

Test Plan: Viewed comment and inline comment previews in Differential, saw old behavior restored.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19943
2019-01-03 13:06:54 -08:00
Austin McKinley
65e89c239e Add status to PhabricatorProjectQuery->getPagingValueMap()
Summary: Probably fixes https://discourse.phabricator-community.org/t/unhandled-exception-for-certain-fields-in-maniphest-search/2263. Couldn't repro this locally, but this is almost certainly the correct fix.

Test Plan: doitlive

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19951
2019-01-03 11:34:24 -08:00
epriestley
9d5b933ed5 Remove all legacy configuration options for mailers
Summary:
Ref T920. About a year ago (in 2018 Week 6, see D19003) we moved from individually configured mailers to `cluster.mailers`, primarily to support fallback across multiple mail providers.

Since this has been stable for quite a while, drop support for the older options.

Test Plan: Grepped for all removed options.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D19940
2019-01-03 04:09:21 -08:00
epriestley
cfcd35d8a3 Remove standalone SMS support in favor of a "Mail, SMS, and other media are mostly the same thing" approach
Summary:
Ref T920. Over time, mail has become much more complex and I think considering "mail", "sms", "postcards", "whatsapp", etc., to be mostly-the-same is now a more promising avenue than building separate stacks for each one.

Throw away all the standalone SMS code, including the Twilio config options. I have a separate diff that adds Twilio as a mail adapter and functions correctly, but it needs some more work to bring upstream.

This permanently destroys the `sms` table, which no real reachable code ever wrote to. I'll call this out in the changelog.

Test Plan:
  - Grepped for `SMS` and `Twilio`.
  - Ran storage upgrade.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D19939
2019-01-03 04:05:20 -08:00
epriestley
e856e791f3 Remove Twilio-PHP API external
Summary:
Ref T920. D19937 provides about 100 lines of code which can do essentially everything here; throw out the trillion lines of full external API stuff.

(I am generally not sure why everyone writes API libraries like this instead of like D19937.)

Test Plan: Send SMS messages with D19937, so I don't think we need any of this code anymore. This code is techncially reachable through some pathways like `bin/sms`, but won't be for long.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: aurelijus

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D19938
2019-01-03 04:04:00 -08:00
Austin McKinley
05a9474138 Raise warning when accidentally submitting Conduit parameters as a JSON-encoded body
Summary: See T12447 for discussion. It is reasonably intuitive to try and pass Conduit parameters via a JSON-encoded HTTP body, but if you do so, you'll get an unhelpful messsage about how method so-and-so does not accept a parameter named "your_entire_json_body". Instead, detect this mistake and advise developers to use form-encoded parameters.

Test Plan:
Got a better error when attempting to make Conduit calls from React code. Tested the following additional invocations of Conduit and got the expected results without an error:

* From the Conduit UI
* With cURL:
```
~ $ curl http://local.phacility.com:8080/api/conpherence.querythread \
>     -d api.token=api-tvv2zb565zrtueab5ddprmpxvrwb \
>     -d ids[0]=1
```
* With `arc call-conduit`:
```
~ $ echo '{
>   "ids": [
>     1
>   ]
> }' | arc call-conduit --conduit-uri http://local.phacility.com:8080/ --conduit-token api-tvv2zb565zrtueab5ddprmpxvrwb conpherence.querythread
```

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19944
2019-01-02 17:31:16 -08:00
epriestley
ea8be11add Fix a qsprintf() issue in mail queries
Summary: Ref T920. Bumped into this while looking at SMS support.

Test Plan: Loaded `/mail/`, no more `qsprintf()` warning.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T920

Differential Revision: https://secure.phabricator.com/D19936
2019-01-02 14:39:31 -08:00
epriestley
106e90dcf0 Remove the "willApplyTransactions()" hook from ApplicationTransactionEditor
Summary:
Depends on D19908. Ref T13222. In D19897, I reordered some transaction code and affected the call order of `willApplyTransactions()`.

It turns out that we use this call for only one thing, and that thing is pretty silly: naming the raw paste data file when editing paste content.

This is only user-visible in the URL when you click "View Raw Paste" and seems exceptionally low-value, so remove the hook and pick a consistent name for the paste datafiles. (We could retain the name behavior in other ways, but it hardly seems worthwhile.)

Test Plan:
  - Created and edited a paste.
  - Grepped for `willApplyTransactions()`.

Note that `EditEngine` (vs `ApplicationTransacitonEditor`) still has a `willApplyTransactions()`, which has one callsite in Phabricator (in Calendar) and a couple in Instances. That's untouched and still works.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19909
2018-12-28 00:19:38 -08:00
epriestley
1729e7b467 Improve UI for "wait" and "answered" MFA challenges
Summary:
Depends on D19906. Ref T13222. This isn't going to win any design awards, but make the "wait" and "answered" elements a little more clear.

Ideally, the icon parts could be animated Google Authenticator-style timers (but I think we'd need to draw them in a `<canvas />` unless there's some clever trick that I don't know) or maybe we could just have the background be like a "water level" that empties out. Not sure I'm going to actually write the JS for either of those, but the UI at least looks a little more intentional.

Test Plan:
{F6070914}

{F6070915}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19908
2018-12-28 00:18:53 -08:00