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

14931 commits

Author SHA1 Message Date
epriestley
dbe479f0d9 Don't send error/exception mail to unverified addresses
Summary:
Depends on D19017. Fixes T12491. Ref T13053. After SES threw us in the dungeon for sending mail to a spamtrap we changed outbound mail rules to stop sending to unverified addresses, except a small amount of registration mail which we can't avoid.

However, we'll still reply to random inbound messages with a helpful error, even if the sender is unverified.

Instead, only send exception mail back if we know who the sender is.

Test Plan: Processed inbound mail with `scripts/mail/mail_handler.php`. No more outbound mail for "bad address", etc. Still got outbound mail for "unknown command !quack".

Reviewers: amckinley

Maniphest Tasks: T13053, T12491

Differential Revision: https://secure.phabricator.com/D19018
2018-02-08 06:26:16 -08:00
epriestley
5792032dc9 Support Postmark inbound mail via webhook
Summary: Depends on D19016. Ref T13053. Adds a listener for the Postmark webhook.

Test Plan:
Processed some test mail locally, at least:

{F5416053}

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19017
2018-02-08 06:25:26 -08:00
epriestley
0986c7f673 Add a "View Object" button on the web mail view page
Summary:
Depends on D19015. Ref T13053. Currently, we don't link up hyperlinks in the body of mail viewed in the web UI. We should, but this is a little tricky (see T13053#235074).

As a general improvement to make working with "Must Encrypt" mail less painful, add a big button to jump to the related object.

Test Plan: {F5415990}

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19016
2018-02-08 06:24:19 -08:00
epriestley
085221b0d6 In HTML mail, make the text for mail stamps in mail bodies smaller and lighter
Summary: Depends on D19014. Ref T13053.

Test Plan: Used `./bin/mail show-outbound --id <id> --dump-html > out.html && open out.html` to look at HTML mail, saw smaller, lighter stamp text with better spacing.

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19015
2018-02-08 06:23:37 -08:00
epriestley
6e5df2dd71 Document that disabling "metamta.one-mail-per-recipient" leaks recipients for "Must Encrypt"
Summary:
Depends on D19013. Ref T13053. When mail is marked "Must Encrypt", we normally do not include recipient information.

However, when `metamta.one-mail-per-recipient` is disabled, the recipient list will leak in the "To" and "Cc" headers. This interaction is probably not very surprising, but document it explicitly for completeness.

(Also use "Mail messages" instead of "Mails".)

Test Plan: Read documentation in the "Config" application.

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19014
2018-02-08 06:23:08 -08:00
epriestley
aa74af1983 Remove all "originalTitle"/"originalName" fields from objects
Summary:
Depends on D19012. Ref T13053. In D19012, I've changed "Thread-Topic" to always use PHIDs.

This change drops the selective on-object storage we have to track the original, human-readable title for objects.

Even if we end up backing out the "Thread-Topic" change, we'd be better off storing this in a table in the Mail app which just has `<objectPHID, first subject we used when sending mail for that object>`, since then we get the right behavior without needing every object to have this separate field.

Test Plan: Grepped for `original`, `originalName`, `originalTitle`, etc.

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19013
2018-02-08 06:22:03 -08:00
epriestley
f090fa7426 Use object PHIDs for "Thread-Topic" headers in mail
Summary:
Depends on D19009. Ref T13053. For "Must Encrypt" mail, we must currently strip the "Thread-Topic" header because it sometimes contains sensitive information about the object.

I don't actually know if this header is useful or anyting uses it. My understanding is that it's an Outlook/Exchange thing, but we also implement "Thread-Index" which I think is what Outlook/Exchange actually look at. This header may have done something before we implemented "Thread-Index", or maybe never done anything. Or maybe older versions of Excel/Outlook did something with it and newer versions don't, or do less. So it's possible that an even better fix here would be to simply remove this, but I wasn't able to convince myself of that after Googling for 10 minutes and I don't think it's worth hours of installing Exchange/Outlook to figure out. Instead, I'm just trying to simplify our handling of this header for now, and maybe some day we'll learn more about Exchange/Outlook and can remove it.

In a number of cases we already use the object monogram or PHID as a "Thread-Topic" without users ever complaining, so I think that if this header is useful it probably isn't shown to users, or isn't shown very often (e.g., only in a specific "conversation" sub-view?). Just use the object PHID (which should be unique and stable) as a thread-topic, everywhere, automatically.

Then allow this header through for "Must Encrypt" mail.

Test Plan: Processed some local mail, saw object PHIDs for "Thread-Topic" headers.

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19012
2018-02-08 06:21:00 -08:00
epriestley
19b3fb8863 Add a Postmark mail adapter so it can be configured as an outbound mailer
Summary: Depends on D19007. Ref T12677.

Test Plan: Used `bin/mail send-test ... --mailer postmark` to deliver some mail via Postmark.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12677

Differential Revision: https://secure.phabricator.com/D19009
2018-02-08 06:18:23 -08:00
epriestley
1f53aa27e4 Add unit tests for mail failover behaviors when multiple mailers are configured
Summary: Depends on D19006. Ref T13053. Ref T12677. When multiple mailers are configured but one or more fail, test that we recover (or don't) appropriately.

Test Plan: Ran unit tests.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D19007
2018-02-08 06:17:49 -08:00
epriestley
9947eee182 Add some test coverage for mailers configuration
Summary: Depends on D19005. Ref T12677. Ref T13053. Tests that turning `cluster.mailers` into an actual list of mailers more or less works as expected.

Test Plan: Ran unit tests.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D19006
2018-02-08 06:17:19 -08:00
epriestley
994d2e8e15 Use "cluster.mailers" if it is configured
Summary:
Depends on D19004. Ref T13053. Ref T12677. If the new `cluster.mailers` is configured, make use of it. Also use it in the Sengrid/Mailgun inbound stuff.

Also fix a bug where "Must Encrypt" mail to no recipients could fatal because no `$mail` was returned.

Test Plan: Processed some mail locally. The testing on this is still pretty flimsy, but I plan to solidify it in an upcoming change.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D19005
2018-02-08 06:12:54 -08:00
epriestley
4236952cdb Add a bin/config set <key> --stdin < value.json flag to make CLI configuration of complex values easier
Summary:
Depends on D19003. Ref T12677. Ref T13053. For the first time, we're requiring CLI configuration of a complex value (not just a string, integer, bool, etc) to do something fairly standard (send mail).

Users sometimes have very reasonable difficulty figuring out how to `./bin/config set key <some big JSON mess>`. Provide an easy way to handle this and make sure it gets appropriate callouts in the documentation.

(Also, hide the `cluster.mailers` value rather than just locking it, since it may have API keys or SMTP passwords.)

Test Plan: Read documentation, used old and new flags to set configuration.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D19004
2018-02-08 06:09:09 -08:00
epriestley
c868ee9c07 Introduce and document a new cluster.mailers option for configuring multiple mailers
Summary:
Depends on D19002. Ref T13053. Ref T12677. Adds a new option to allow configuration of multiple mailers.

Nothing actually uses this yet.

Test Plan: Tried to set it to various bad values, got reasonable error messages. Read documentation.

Reviewers: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D19003
2018-02-08 06:08:34 -08:00
epriestley
7f2c90fbd1 Prepare for multiple mailers of the same type
Summary:
Depends on D19000. Ref T13053. Ref T12677. Currently, most mailers are configured with a bunch of `<mailer>.setting-name` global config options.

This means that you can't configure two different SMTP servers, which is a reasonable thing to want to do in the brave new world of mail failover.

It also means you can't configure two Mailgun accounts or two SES accounts. Although this might seem a little silly, we've had more service disruptions because of policy issues / administrative error (where a particular account was disabled) than actual downtime, so maybe it's not completely ridiculous.

Realign mailers so they can take configuration directly in an explicit way. A later change will add new configuration to take advantage of this and let us move away from having ~10 global options for this stuff eventually.

(This also makes writing third-party mailers easier.)

Test Plan:
Processed some mail, ran existing unit tests. But I wasn't especially thorough.

I expect later changes to provide some tools to make this more testable, so I'll vet each provider more thoroughly and add coverage for multiple mailers after that stuff is ready.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D19002
2018-02-08 06:00:59 -08:00
epriestley
7765299f83 Mask the sender for "Must Encrypt" mail
Summary:
Depends on D18998. Ref T13053. When we send "Must Encrypt" mail, we currently send it with a normal "From" address.

This discloses a little information about the object (for example, if the Director of Silly Walks is interacting with a "must encrypt" object, the vulnerability is probably related to Silly Walks), so anonymize who is interacting with the object.

Test Plan: Processed some mail. (The actual final "From" is ephemeral and a little tricky to examine and I didn't actually transmit mail over the network, but it should be obvious if this works or not on `secure`.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19000
2018-02-08 05:59:35 -08:00
epriestley
1485debcbd Prepare mail transmission to support failover across multiple mailers
Summary:
Ref T13053. Ref T12677. This restructures the calls and error handling logic so that we can pass in a list of multiple mailers and get retry logic.

This doesn't actually ever use multiple mailers yet, and shouldn't change any behavior. I'll add multiple-mailer coverage a little further in, since there's currently no way to effectively test which of several mailers ended up transmitting a message.

Test Plan:
  - This has test coverage; tests still pass.
  - Poked around locally doing things that send mail, saw mail appear to send. I'm not attached to a real mailer though so my confidence in local testing is only so-so.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D18998
2018-02-08 05:49:06 -08:00
epriestley
150a04791c Fix bad NUX link in Legalpad search view
Summary:
See <https://discourse.phabricator-community.org/t/clicking-the-create-a-document-button-on-fresly-installed-phabricators-legalpad-results-in-404/1088>.

This URI isn't correct.

Test Plan: Visited {nav Use Results > New User State} in developer mode, clicked green button. Before: 404. After: taken to the edit screen.

Differential Revision: https://secure.phabricator.com/D19024
2018-02-08 05:47:02 -08:00
epriestley
a5bbadbaba Fix another Git 2.16.0 CLI compatibility issue
Summary:
This command also needs a "." instead of an empty string now.

(This powers the file browser typeahead in Diffusion.)

Test Plan: Will test in production since there's still no easy 2.16 installer for macOS.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19010
2018-02-07 17:54:39 -08:00
epriestley
56bf069080 Try running Herald when performing inverse edge edits
Summary:
Ref T13053. When you mention one object on another (or link two objects together with an action like "Edit Parent Revisions"), we write a transaction on each side to add the "alice added subtask X" and "alice added parent task Y" items to the timeline.

This behavior now causes problems in T13053 with the "Must Encrypt" flag because it prevents the flag from being applied to the corresponding "inverse edge" mail.

This was added in rP5050389f as a quick workaround for a fatal related to Editors not having enough data to apply Herald on mentions. However, that was in 2014, and since then:

  - Herald got a significant rewrite to modularize all the rules and adapters.
  - Editing got a significant upgrade in EditEngine and most edit workflows now operate through EditEngine.
  - We generally do more editing on more pathways, everything is more modular, and we have standardized how data is loaded to a greater degree.

I suspect there's no longer a problem with just running Herald here, and can't reproduce one. If anything does crop up, it's probably easy (and desirable) to fix it.

This makes Herald fire a little more often: if someone writes a rule, mentioning or creating a relationship to old tasks will now make the rule act. Offhand, that seems fine. If it turns out to be weird, we can probably tailor Herald's behavior.

Test Plan:
I wasn't able to break anything:

  - Mentioned a task on another task (original issue).
  - Linked tasks with commits, mocks, revisions.
  - Linked revisions with commits, tasks.
  - Mentioned users, revisions, and commits.
  - Verified that mail generated by creating links (e.g., Revision > Edit Tasks) now gets the "Must Encrypt" flag properly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18999
2018-02-06 12:18:56 -08:00
epriestley
1bf64e5cbc Add Differential and Herald mail stamps and some refinements
Summary:
Ref T13053. Adds revision stamps (status, reviewers, etc). Adds Herald rule stamps, like the existing X-Herald-Rules header.

Removes the "self" stamps, since you can just write a rule against `whatever(@epriestley)` equivalently. If there's routing logic around this, it can live in the routing layer. This avoids tons of self-actor, self-mention, self-reviewer, self-blocking-reviewer, self-resigned-reviewer, etc., stamps.

Use `natcasesort()` instead of `sort()` so that numeric values (like monograms) sort `9, 80, 700` instead of `700, 80, 9`.

Remove the commas from rendering since they don't really add anything.

Test Plan: Edited tasks and revisions, looked at mail stamps, saw stamps that looked pretty reasonable (with no more self stuff, no more commas, sorting numbers, and Herald stamps).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18997
2018-02-06 04:06:07 -08:00
epriestley
7d475eb09a Add more mail stamps: tasks, subscribers, projects, spaces
Summary: Ref T13053. Adds task stamps plus the major infrastructure applications.

Test Plan: {F5413058}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18996
2018-02-06 04:05:46 -08:00
epriestley
3131e733a8 Add Editor-based mail stamps: actor, via, silent, encrypted, new, mention, self-actor, self-mention
Summary: Ref T13053. Adds more mail tags with information available on the Editor object.

Test Plan: Banged around in Maniphest, viewed the resulting mail, all the stamps seemed to align with reality.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18995
2018-02-06 04:04:52 -08:00
epriestley
9de54aedb5 Remove inconsistent and confusing use of the term "multiplex" in mail
Summary:
Ref T13053. Because I previously misunderstood what "multiplex" means, I used it in various contradictory and inconsistent ways.

We can send mail in two ways: either one mail to everyone with a big "To" and a big "Cc" (not default; better for mailing lists) or one mail to each recipient with just them in "To" (default; better for almost everything else).

"Multiplexing" is combining multiple signals over a single channel, so it more accurately describes the big to/cc. However, it is sometimes used to descibe the other approach. Since it's ambiguous and I've tainted it through misuse, get rid of it and use more clear language.

(There's still some likely misuse in the SMS stuff, and a couple of legitimate uses in other contexts.)

Test Plan: Grepped for `multiplex`, saw less of it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18994
2018-02-06 04:04:34 -08:00
epriestley
c3f95bc410 Add basic support for mail "stamps" to improve client mail routing
Summary:
Ref T10448. Currently, we use "mail tags" (in {nav Settings > Email Preferences})  to give users some ability to route mail. There are a number of major issues with this:

  - It isn't modular and can't be extended by third-party applications.
  - The UI is a giant mess of 5,000 individual settings.
  - Settings don't map clearly to actual edits.
  - A lot of stuff isn't covered by any setting.

This adds a new system, called "mail stamps", which is similar to "mail tags" but tries to fix all these problems.

I called these "stamps" because: stamps make sense with mail; we can't throw away the old system just yet and need to keep it around for a bit; we don't use this term for anything else; it avoids confusion with project tags.

(Conceptually, imagine these as ink stamps like "RETURN TO SENDER" or "FRAGILE", not actual postage stamps.)

The only real "trick" here is that later versions of this will need to enumerate possible stamps for an object and maybe all possible stamps for all objects in the system. This is why stamp generation is separated into a "template" phase and a "value" phase. In future changes, the "template" phase can be used on its own to generate documentation and typeaheads and let users build rules. This may need some more refinement before it really works since I haven't built any of that yet.

Also adds a preference for getting stamps in the header only (default) or header and body (better for Gmail, which can't route based on headers).

Test Plan:
Fiddled with preference, sent some mail and saw a "STAMPS" setting in the body and an "X-Phabricator-Stamps" header.

{F5411694}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10448

Differential Revision: https://secure.phabricator.com/D18991
2018-02-06 04:04:13 -08:00
epriestley
ef121b3e17 Fix a Herald repetition policy selection error for rule types which support only one policy
Summary:
Ref T13048. See <https://discourse.phabricator-community.org/t/configuring-commit-hook-commit-content-rules-fail-with-exception/1077/3>.

When a rule supports only one repetition policy (always "every time") like "Commit Hook" rules, we don't render a control for `repetition_policy` and fail to update it when saving.

Before the changes to support the new "if the rule did not match the last time" policy, this workflow just defaulted to "every time" if the input was invalid, but this was changed by accident in D18926 when I removed some of the toInt/toString juggling code.

(This patch also prevents users from fiddling with the form to create a rule which evaluates with an invalid policy; this wasn't validated before.)

Test Plan:
  - Created new "Commit Hook" (only one policy available) rule.
  - Saved existing "Commit Hook" rule.
  - Created new "Task" (multiple policies) rule.
  - Saved existing Task rule.
  - Set task rule to each repetition policy, saved, verified the save worked.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048

Differential Revision: https://secure.phabricator.com/D18992
2018-02-05 13:35:36 -08:00
epriestley
b3880975e5 Add aliases for "party" emoji (🎉)
Summary:
This is currently `🎉`, which I'd never have guessed.

(This isn't a super scalable approach, but this emoji is in particularly common use. See also T12644.)

Test Plan: Typed `:party`, `:confet`, etc.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18993
2018-02-05 12:23:26 -08:00
epriestley
7e6bae471c Always setlocale() to en_US.UTF-8 for the main process
Summary:
Depends on D18987. See PHI343. Fixes T13060. See also T7339.

When the main process starts up with `LANG=POSIX` (this is the default on Ubuntu) and we later try to run a subprocess with a UTF8 character in the argument list (like `git cat-file blob 🐑.txt`), the argument is not passed to the subprocess correctly.

We already set `LANG=en_US.UTF-8` in the //subprocess// environment, but this only controls behavior for the subprocess itself. It appears that the argument list encoding before the actual subprocess starts depends on the parent process's locale setting, which makes some degree of sense.

Setting `putenv('LANG=en_US.UTF-8')` has no effect on this, but my guess is that the parent process's locale setting is read at startup (rather than read anew from `LANG` every time) and not changed by further modifications of `LANG`.

Using `setlocale(...)` does appear to fix this.

Ideally, installs would probably set some UTF-8-compatible LANG setting as the default. However, this makes setup harder and I couldn't figure out how to do it on our production Ubuntu AMI after spending a reasonable amount of time at it (see T13060).

Since it's very rare that this setting matters, try to just do the right thing. This may fail if "en_US.UTF-8" isn't available, but I think warnings/remedies to this are in the scope of T7339, since we want this locale to exist for other legitimate reasons anyway.

Test Plan:
  - Applied this fix in production, processed the failing worker task from PHI343 after kicking Apache hard enough.
  - Ran locally with `setlocale(LC_ALL, 'duck.quack')` to make sure a bad/invalid/unavailable setting didn't break anything, didn't hit any issues.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13060

Differential Revision: https://secure.phabricator.com/D18988
2018-02-05 12:23:06 -08:00
epriestley
956c4058e6 Add a bin/conduit call support binary
Summary:
Ref T13060. See PHI343. Triaging this bug required figuring out where in the pipeline UTF8 was being dropped, and bisecting the pipeline required making calls to Conduit.

Currently, there's no easy way to debug/inspect arbitrary Conduit calls, especially when they are `diffusion.*` calls which route to a different host (even if you have a real session and use the web console for these, you just see an HTTP service call to the target host in DarkConsole).

Add a `bin/conduit` utility to make this kind of debugging easier, with an eye toward the Phacility production cluster (or other similar clusters) specifically.

Test Plan:
  - Ran `echo '{}' | bin/conduit call --method conduit.ping --input -` and similar.
  - Used a similar approach to successfully diagnose the UTF8 issue in T13060.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13060

Differential Revision: https://secure.phabricator.com/D18987
2018-02-05 12:20:49 -08:00
epriestley
55f7cdb99b Fix a bad classname reference in the "Must Encrypt" action 2018-02-02 14:57:25 -08:00
epriestley
6d90c7ad92 Save mail attachments in Files, not on the actual objects
Summary:
Depends on D18985. Ref T13053. See PHI125. Currently, mail attachments are just encoded onto the actual objects in the `MetaMTAMail` table.

This fails if attachments can't be encoded in JSON -- e.g., they aren't UTF8. This happens most often when revisions or commits attach patches to mail and those patches contain source code changes for files that are not encoded in UTF8.

Instead, save attachments in (and load attachments from) Files.

Test Plan: Enabled patches for mail, created a revision, saw it attach a patch. Viewed mail in web UI, saw link to download patch. Followed link, saw sensible file. Checked database, saw a `filePHID`. Destroyed mail with `bin/remove destroy`, saw attached files also destroyed.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18986
2018-02-02 14:38:08 -08:00
epriestley
eb06aca951 Support DestructionEngine in MetaMTAMail
Summary:
Depends on D18984. Ref T13053. See D13408 for the original change and why this doesn't use DestructionEngine right now. The quick version is:

  - It causes us to write a destruction log, which is slightly silly (we're deleting one thing and creating another).
  - It's a little bit slower than not using DestructionEngine.

However, it gets us some stuff for free that's likely relevant now (e.g., Herald Transcript cleanup) and I'm planning to move attachments to Files, but want to be able to delete them when mail is destroyed.

The destruction log is a touch silly, but those records are very small and that log gets GC'd later without generating new logs. We could silence the log from the GC if it's ever an issue.

Test Plan: Used `bin/remove destroy` and `bin/garbage collect --collector mail.sent` to destroy mail and collect garbage.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18985
2018-02-02 14:37:33 -08:00
epriestley
cbe4e68c07 Add a Herald action to trigger "Must Encrypt" for mail
Summary: Depends on D18983. Ref T13053. Adds a new Herald action to activate the "must encrypt" flag and drop mail content.

Test Plan:
  - Created a new Herald rule:

{F5407075}

  - Created a "dog task" (woof woof, unsecure) and a "duck task" (quack quack, secure).
  - Viewed mail for both in `bin/mail` and web UI, saw appropriate security/encryption behavior.
  - Viewed "Must Encrypt" in "Headers" tab for the duck mail, saw why the mail was encrypted (link to Herald rule).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18984
2018-02-02 14:35:26 -08:00
epriestley
7b2b5cd91e Add basic support for a "Must Encrypt" mail flag which prevents unsecured content transmission
Summary:
Ref T13053. See PHI291. For particularly sensitive objects (like security issues), installs may reasonably wish to prevent details from being sent in plaintext over email.

This adds a "Must Encrypt" mail behavior, which discards mail content and all identifying details, replacing it with a link to the `/mail/` application. Users can follow the link to view the message over HTTPS.

The flag discards body content, attachments, and headers which imply things about the content of the object. It retains threading headers and headers which may uniquely identify the object as long as they don't disclose anyting about the content.

The `bin/mail list-outbound` command now flags these messages with a `#` mark.

The `bin/mail show-outbound` command now shows sent/suppressed headers and the body content as delivered (if it differs from the original body content).

The `/mail/` web UI now shows a tag for messages marked with this flag.

For now, there is no way to actually set this flag on mail.

Test Plan:
  - Forced this flag on, made comments and took actions to send mail.
  - Reviewed mail with `bin/mail` and `/mail/` in the web UI, saw all content information omitted.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18983
2018-02-02 14:34:34 -08:00
epriestley
032f5b2294 Allow revisions to revert commits and one another, and commits to revert revisions
Summary:
Ref T13057. This makes "reverts" syntax more visible and useful. In particular, you can now `Reverts Dxx` in a revision or commit, and `Reverts <hash>` from a revision.

When you do, the corresponding object will get a more-visible cross-reference marker in its timeline:

{F5405517}

From here, we can look at surfacing revert information more heavily, since we can now query it on revision/commit pages via edges.

Test Plan: Used "reverts <hash>" and "reverts <revision>" in Differential and Diffusion, got sensible results in the timeline.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13057

Differential Revision: https://secure.phabricator.com/D18978
2018-02-02 08:25:58 -08:00
epriestley
f535981c0d Fix a missing getSSHUser() callsite
Summary:
See <https://discourse.phabricator-community.org/t/after-upgrade-git-lfs-push-ends-up-in-call-to-undefined-method-on-diffusion-git-lfs-authenticate-workflow/1047/1>.

I renamed this method in D18912 but missed this callsite since the workflow doesn't live alongside the other ones.

Test Plan: Ran `git push` in an LFS repository over SSH. Before: fatal; after: clean push.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18977
2018-01-31 15:34:12 -08:00
epriestley
6d5f265a57 Accept null via conduit.edit to unassign a task
Summary:
See <https://discourse.phabricator-community.org/t/maniphest-edit-to-unassign-owner-documentation-is-wrong/1053>. This unusual field doesn't actually accept `null`, although the documentation says it does and that was the intent.

Accept `null`, and show `phid|null` in the docs.

Test Plan: Viewed docs, saw `phid|null`. Unassigned with `null`.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18976
2018-01-31 15:33:52 -08:00
epriestley
f9336e5694 Mangle cells that look a little bit like formulas in CSV files
Summary:
Fixes T12800. See that task for discussion. When a cell in a CSV begins with "=", "+", "-", or "@", mangle the content to discourage Excel from executing it.

This is clumsy, but we support other formats (e.g., JSON) which preserve the data faithfully and you should probably be using JSON if you're going to do anything programmatic with it.

We could add two formats or a checkbox or a warning or something but cells with these symbols are fairly rare anyway.

Some possible exceptions I can think of are "user monograms" (but we don't export those right now) and "negative numbers" (but also no direct export today). We can add exceptions for those as they arise.

Test Plan: Exported a task named `=cmd|'/C evil.exe'!A0`, saw the title get mangled with "(!)" in front.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12800

Differential Revision: https://secure.phabricator.com/D18974
2018-01-31 15:33:11 -08:00
epriestley
c9df8f77c8 Fix transcription of single-value bulk edit fields ("Assign to")
Summary: See PHI333. Some of the cleanup at the tail end of the bulk edit changes made "Assign To" stop working properly, since we don't strip the `array(...)` off the `array(PHID)` value we receive.

Test Plan:
  - Used bulk editor to assign and unassign tasks (single value datasource).
  - Used bulk editor to change projects (multi-value datasource).

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18975
2018-01-31 10:56:16 -08:00
epriestley
1e3d1271ad Make push log "flags", "reject code" human readable; add crumbs to pull/push logs
Summary:
Depends on D18972. Ref T13049.

Currently, the "flags" columns renders an inscrutible bitmask which you have to go hunt down in the code. Show a list of flags in human-readable text instead.

The "code" column renders a meaningless integer code. Show a text description instead.

The pull logs and push logs pages don't have a crumb to go back up out of the current query. Add one.

Test Plan: Viewed push logs, no more arcane numbers. Saw and clicked crumbs on each log page.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18973
2018-01-30 15:45:58 -08:00
epriestley
ff98f6f522 Make the remote address rules for Settings > Activity Logs more consistent
Summary:
Depends on D18971. Ref T13049. The rule is currently "you can see IP addresses for actions which affect your account".

There's some legitimate motivation for this, since it's good if you can see that someone you don't recognize has been trying to log into your account.

However, this includes cases where an administrator disables/enables your account, or promotes/demotes you to administrator. In these cases, //their// IP is shown!

Make the rule:

  - Administrators can see it (consistent with everything else).
  - You can see your own actions.
  - You can see actions which affected you that have no actor (these are things like login attempts).
  - You can't see other stuff: usually, administrators changing your account settings.

Test Plan: Viewed activity log as a non-admin, no longer saw administrator's IP address disclosed in "Demote from Admin" log.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18972
2018-01-30 15:45:39 -08:00
epriestley
8a2863e3f7 Change the "can see remote address?" policy to "is administrator?" everywhere
Summary:
Depends on D18970. Ref T13049. Currently, the policy for viewing remote addresses is:

  - In activity logs: administrators.
  - In push and pull logs: users who can edit the corresponding repository.

This sort of makes sense, but is also sort of weird. Particularly, I think it's kind of hard to understand and predict, and hard to guess that this is the behavior we implement. The actual implementation is complex, too.

Instead, just use the rule "administrators can see remote addresses" consistently across all applications. This should generally be more strict than the old rule, because administrators could usually have seen everyone's address in the activity logs anyway. It's also simpler and more expected, and I don't really know of any legit use cases for the "repository editor" rule.

Test Plan: Viewed pull/push/activity logs as non-admin. Saw remote addresses as an admin, and none as a non-admin.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18971
2018-01-30 15:45:23 -08:00
epriestley
75bc86589f Add date range filtering for activity, push, and pull logs
Summary: Ref T13049. This is just a general nice-to-have so you don't have to export a 300MB file if you want to check the last month of data or whatever.

Test Plan: Applied filters to all three logs, got appropriate date-range result sets.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18970
2018-01-30 15:36:22 -08:00
epriestley
0d5379ee17 Fix an export bug where queries specified in the URI ("?param=value") were ignored when filtering the result set
Summary:
Depends on D18968. Ref T13049. Currently, if you visit `/query/?param=value`, there is no `queryKey` for the page but we build a query later on.

Right now, we incorrectly link to `/query/all/export/` in this case (and export too many results), but we should actually link to `/query/<constructed query key>/export/` to export only the desired/previewed results.

Swap the logic around a little bit so we look at the query we're actually executing, not the original URI, to figure out the query key we should use when building the link.

Test Plan: Visited a `/?param=value` page, exported data, got a subset of the full data instead of everything.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18969
2018-01-30 11:19:37 -08:00
epriestley
5b22412f24 Support data export on push logs
Summary: Depends on D18967. Ref T13049. Nothing too fancy going on here.

Test Plan: Exported push logs, looked at the export, seemed sensible.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18968
2018-01-30 11:19:20 -08:00
epriestley
a5b8be0316 Support export of user activity logs
Summary:
Depends on D18966. Ref T13049. Adds export support to user activity logs.

These don't have PHIDs. We could add them, but just make the "phid" column test if the objects have PHIDs or not for now.

Test Plan:
  - Exported user activity logs, got sensible output (with no PHIDs).
  - Exported some users to make sure I didn't break PHIDs, got an export with PHIDs.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18967
2018-01-30 11:12:32 -08:00
epriestley
91108cf838 Upgrade user account activity logs to modern construction
Summary: Depends on D18965. Ref T13049. Move this Query and SearchEngine to be a little more modern, to prepare for Export support.

Test Plan:
  - Used all the query fields, viewed activity logs via People and Settings.
  - I'm not sure the "Session" query is used/useful and may remove it before I'm done here, but I just left it in place for now.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18966
2018-01-30 11:11:47 -08:00
epriestley
b27fd05eef Add a bin/bulk export CLI tool to make debugging and profiling large exports easier
Summary:
Ref T13049. When stuff executes asynchronously on the bulk workflow it can be hard to inspect directly, and/or a pain to test because you have to go through a bunch of steps to run it again.

Make future work here easier by making export triggerable from the CLI. This makes it easy to repeat, inspect with `--trace`, profile with `--xprofile`, etc.

Test Plan:
  - Ran several invalid commands, got sensible error messages.
  - Ran some valid commands, got exported data.
  - Used `--xprofile` to look at the profile for a 300MB dump of 100K tasks which took about 40 seconds to export. Nothing jumped out as sketchy to me -- CustomField wrangling is a little slow but most of the time looked like it was being spent legitimately.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18965
2018-01-30 11:11:13 -08:00
epriestley
84df122085 When exporting more than 1,000 records, export in the background
Summary:
Depends on D18961. Ref T13049. Currently, longer exports don't give the user any feedback, and exports that take longer than 30 seconds are likely to timeout.

For small exports (up to 1,000 rows) continue doing the export in the web process.

For large exports, queue a bulk job and do them in the workers instead. This sends the user through the bulk operation UI and is similar to bulk edits. It's a little clunky for now, but you get your data at the end, which is far better than hanging for 30 seconds and then fataling.

Test Plan: Exported small result sets, got the same workflow as before. Exported very large result sets, went through the bulk flow, got reasonable results out.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18962
2018-01-29 16:08:02 -08:00
epriestley
ea58b6acea Remove the old, non-modular Excel export workflow from Maniphest
Summary:
Depends on D18960. Ref T13049. Now that Maniphest fully supports "Export Data", remove the old hard-coded version.

This is a backward compatibility break with the handful of installs that might have defined a custom export by subclassing `ManiphestExcelFormat`. I suspect this is almost zero installs, and that the additional data in the new format may serve most of the needs of this tiny number of installs. They can upgrade to `ExportEngineExtensions` fairly easily if this isn't true.

Test Plan:
  - Viewed Maniphest, no longer saw the old export workflow.
  - Grepped for `export` and similar strings to try to hunt everything down.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18961
2018-01-29 16:06:13 -08:00
epriestley
c00838878a Implement common infrastructure fields as export extensions
Summary:
Depends on D18959. Ref T13049. Provide tags, subscribers, spaces, and created/modified as automatic extensions for all objects which support them.

(Also, for JSON export, be a little more consistent about exporting `null` instead of empty string when there's no value in a text field.)

Test Plan: Exported users and tasks, saw relevant fields in the export.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18960
2018-01-29 16:05:32 -08:00