Summary:
Depends on D19019. Ref T13053. Fixes T12689. See PHI178.
Currently, if `@alice` resigns from a revision but `#alice-fan-club` is still a subscriber or reviewer, she'll continue to get mail. This is undesirable.
When users are associated with an object but have explicitly disengaged in an individal role (currently, only resign in audit/differential) mark them "unexpandable", so that they can no longer be included through implicit membership in a group (a project or package).
`@alice` can still get mail if she's a explicit recipient: as an author, owner, or if she adds herself back as a subscriber.
Test Plan:
- Added `@ducker` and `#users-named-ducker` as reviewers. Ducker got mail.
- Resigned as ducker, stopped getting future mail.
- Subscribed explicitly, got mail again.
- (Plus some `var_dump()` sanity checking in the internals.)
Reviewers: amckinley
Maniphest Tasks: T13053, T12689
Differential Revision: https://secure.phabricator.com/D19021
Summary:
Depends on D19018. Fixes T4776. Ref T13053. When you remove someone from an object's recipient list (for example, by removing them a reviewer, auditor, subscriber, owner or author) we currently do not send them mail about it because they're no longer connected to the object.
In many of these cases (Commandeer, Reassign) the actual action in the UI adds them back to the object somehow (as a reviewer or subscriber, respectively) so this doesn't actually matter. However, there's no recovery mechanism for reviewer or subscriber removal.
This is slightly bad from a policy/threat viewpoint since it means an attacker can remove all the recipients of an object "somewhat" silently. This isn't really silent, but it's less un-silent than it should be.
It's also just not very good from a human interaction perspective: if Alice removes Bob as a reviewer, possibly "against his will", he should be notified about that. In the good case, Alice wrote a nice goodbye note that he should get to read. In the bad case, he should get a chance to correct the mistake.
Also add a `removed(@user)` mail stamp so you can route these locally if you want.
Test Plan:
- Created and edited some different objects without catching anything broken.
- Removed subscribers from tasks, saw the final email include the removed recipients with a `removed()` stamp.
I'm not totally sure this doesn't have any surprising behavior or break any weird objects, but I think anything that crops up should be easy to fix.
Reviewers: amckinley
Subscribers: sophiebits
Maniphest Tasks: T13053, T4776
Differential Revision: https://secure.phabricator.com/D19019
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
