Summary: Depends on D19056. Fixes T8475. Ref T13054. Merges "ModernHunk" back into "Hunk".
Test Plan: Grepped for `modernhunk`. Reviewed revisions. Created a new revision. Used `bin/differential migrate-hunk` to migrate hunks between storage formats and back.
Maniphest Tasks: T13054, T8475
Differential Revision: https://secure.phabricator.com/D19057
Summary: Ref T13054. Ref T8475. This table has had no readers or writers for more than a year after it was migrated to the modern table.
Test Plan: Ran migration, verified that all the data was still around.
Maniphest Tasks: T13054, T8475
Differential Revision: https://secure.phabricator.com/D19056
Summary:
Fixes T6121. See PHI357.
- Allow emoji and other unicode (like Chinese characters) as long as you have at least three of them.
- Disallow macros with only latin symbols. These were previously allowed.
Test Plan: Created a macro for "🐶🐶🐶", then used it in a comment.
Maniphest Tasks: T6121
Differential Revision: https://secure.phabricator.com/D19051
Summary: Depends on D19048. Fixes T11330.
Test Plan: Wrote rules to call webhooks selectively, saw them fire appropriately with correct trigger attribution.
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19049
Summary:
Depends on D19045. Ref T11330.
- View/regenerate HMAC keys.
- Pretty JSON.
- Readable status transactions.
- test, silent, secure flags.
- Dates on request view.
- More icons.
- Can test any object.
- GC for requests.
Test Plan: Went through each feature poking at it in the web UI and with `bin/webhook call ...` / `bin/garbage collect ...`.
Subscribers: ftdysa
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19046
Summary: Ref T11330. Adds general support for webhooks. This is still rough and missing a lot of pieces -- and not yet useful for anything -- but can make HTTP requests.
Test Plan: Used `bin/webhook call ...` to complete requests to a test endpoint.
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19045
Summary: Ref T13053. See PHI126. Add an explicit "Mute" action to kill mail and notifications for a particular object.
Test Plan: Muted and umuted an object while interacting with it. Saw mail route appropriately.
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19033
Summary:
Ref T13053. Fixes T7804. Adds "Acting user" so you can have "always email me" stuff skip things you did or keep an eye on suspicious interns.
For the test console, the current user is the acting user.
For pushes, the pusher is the acting user.
Test Plan: Wrote acting user rules, triggered them via test console and via multiple actors on real objects.
Maniphest Tasks: T13053, T7804
Differential Revision: https://secure.phabricator.com/D19031
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 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 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 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:
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 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 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 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 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
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
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
Summary: Depends on D18958. Ref T13049. Support the new stuff. There are a couple more fields this needs to strictly improve on the old export, but I'll add them as extensions shortly.
Test Plan: Exported tasks to Excel, saw reasonble-looking data in the export.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18959
Summary:
Depends on D18956. Ref T13049. Make the "Export Format" selector sticky.
This is partly selfish, since it makes testing format changes a bit easier.
It also seems like it's probably a good behavior in general: if you export to Excel once, that's probably what you're going to pick next time.
Test Plan: Exported to excel. Exported again, got excel as the default option.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18957
Summary:
Depends on D18955. Ref T13049. This directory was getting a little cluttered with different kinds of code.
Put the formats (csv, json, ...), the field types (int, string, epoch, ...) and the engine-related stuff in subdirectories.
Test Plan: wow so aesthetic
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18956
Summary:
Depends on D18954. Ref T13049. This brings over the existing Maniphest Excel export pipeline in a generic way.
The `<Type>ExportField` classes know directly that `PHPExcel` exists, which is a little sketchy, but writing an Excel indirection layer sounds like a lot of work and I don't anticipate us changing Excel backends anytime soon, so trying to abstract this feels YAGNI.
This doesn't bring over the install instructions for PHPExcel or the detection of whether or not it exists. I'll bring that over in a future change.
Test Plan: Exported users as Excel, opened them up, got a sensible-looking Excel sheet.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18955
Summary:
Depends on D18953. Ref T13049. Allow applications and infrastructure to supplement exportable fields for objects.
Then, implement an extension for custom fields. Only a couple field types (int, string) are supported for now.
Test Plan: Added some custom fields to Users, populated them, exported users. Saw custom fields in the export.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18954
Summary: Depends on D18948. Ref T13051. The actual logic ended up so simple that this doesn't really feel terribly valuable, but maybe it'll catch something later on.
Test Plan: Ran test.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13051
Differential Revision: https://secure.phabricator.com/D18949
Summary:
Depends on D18947. Ref T13051. This goes through transaction tables and compacts the edge storage into the slim format.
I put this on `bin/garbage` instead of `bin/storage` because `bin/storage` has a lot of weird stuff about how it manages databases so that it can run before configuration (e.g., all the `--user`, `--password` type flags for configuring DB connections).
Test Plan:
Loaded an object with a bunch of transactions. Ran migration. Spot checked table for sanity. Loaded another copy of the object in the web UI, compared the two pages, saw no user-visible changes.
Here's a concrete example of the migration effect -- old row:
```
*************************** 44. row ***************************
id: 757
phid: PHID-XACT-PSTE-5gnaaway2vnyen5
authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
objectPHID: PHID-PSTE-5uj6oqv4kmhtr6ctwcq7
viewPolicy: public
editPolicy: PHID-USER-cvfydnwadpdj7vdon36z
commentPHID: NULL
commentVersion: 0
transactionType: core:edge
oldValue: {"PHID-PROJ-wh32nih7q5scvc5lvipv":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-wh32nih7q5scvc5lvipv","dateCreated":"1449170691","seq":"0","dataID":null,"data":[]},"PHID-PROJ-5r2ed5v27xrgltvou5or":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-5r2ed5v27xrgltvou5or","dateCreated":"1449170683","seq":"0","dataID":null,"data":[]},"PHID-PROJ-zfp44q7loir643b5i4v4":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-zfp44q7loir643b5i4v4","dateCreated":"1449170668","seq":"0","dataID":null,"data":[]},"PHID-PROJ-okljqs7prifhajtvia3t":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-okljqs7prifhajtvia3t","dateCreated":"1448902756","seq":"0","dataID":null,"data":[]},"PHID-PROJ-3cuwfuuh4pwqyuof2hhr":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-3cuwfuuh4pwqyuof2hhr","dateCreated":"1448899367","seq":"0","dataID":null,"data":[]},"PHID-PROJ-amvkc5zw2gsy7tyvocug":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-amvkc5zw2gsy7tyvocug","dateCreated":"1448833330","seq":"0","dataID":null,"data":[]}}
newValue: {"PHID-PROJ-wh32nih7q5scvc5lvipv":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-wh32nih7q5scvc5lvipv","dateCreated":"1449170691","seq":"0","dataID":null,"data":[]},"PHID-PROJ-5r2ed5v27xrgltvou5or":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-5r2ed5v27xrgltvou5or","dateCreated":"1449170683","seq":"0","dataID":null,"data":[]},"PHID-PROJ-zfp44q7loir643b5i4v4":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-zfp44q7loir643b5i4v4","dateCreated":"1449170668","seq":"0","dataID":null,"data":[]},"PHID-PROJ-okljqs7prifhajtvia3t":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-okljqs7prifhajtvia3t","dateCreated":"1448902756","seq":"0","dataID":null,"data":[]},"PHID-PROJ-3cuwfuuh4pwqyuof2hhr":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-3cuwfuuh4pwqyuof2hhr","dateCreated":"1448899367","seq":"0","dataID":null,"data":[]},"PHID-PROJ-amvkc5zw2gsy7tyvocug":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-amvkc5zw2gsy7tyvocug","dateCreated":"1448833330","seq":"0","dataID":null,"data":[]},"PHID-PROJ-tbowhnwinujwhb346q36":{"dst":"PHID-PROJ-tbowhnwinujwhb346q36","type":41,"data":[]},"PHID-PROJ-izrto7uflimduo6uw2tp":{"dst":"PHID-PROJ-izrto7uflimduo6uw2tp","type":41,"data":[]}}
contentSource: {"source":"web","params":[]}
metadata: {"edge:type":41}
dateCreated: 1450197571
dateModified: 1450197571
```
New row:
```
*************************** 44. row ***************************
id: 757
phid: PHID-XACT-PSTE-5gnaaway2vnyen5
authorPHID: PHID-USER-cvfydnwadpdj7vdon36z
objectPHID: PHID-PSTE-5uj6oqv4kmhtr6ctwcq7
viewPolicy: public
editPolicy: PHID-USER-cvfydnwadpdj7vdon36z
commentPHID: NULL
commentVersion: 0
transactionType: core:edge
oldValue: []
newValue: ["PHID-PROJ-tbowhnwinujwhb346q36","PHID-PROJ-izrto7uflimduo6uw2tp"]
contentSource: {"source":"web","params":[]}
metadata: {"edge:type":41}
dateCreated: 1450197571
dateModified: 1450197571
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13051
Differential Revision: https://secure.phabricator.com/D18948
Summary:
Ref T13051. This puts a translation layer between the raw edge data in the transaction table and the UI that uses it.
The intent is to start writing new, more compact data soon. This class give us a consistent API for interacting with either the new or old data format, so we don't have to migrate everything upfront.
Test Plan: Browsed around, saw existing edge transactions render properly in transactions and feed. Added and removed subscribers and projects, saw good transaction rendering.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13051
Differential Revision: https://secure.phabricator.com/D18946
Summary: See PHI280. We have a similar field for tasks already, this is generally a reasonable sort of thing to support, and the addition of "draft" states means there are some pretty reasonable use cases.
Test Plan:
- Wrote a status-based ("status is needs revision") Herald rule.
- Tested it against a "Needs Revision" revision (passed) and a "Changes Planned" revision (failed).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18938
Summary:
Depends on D18918. Ref T13046. Ref T5954. Pull logs can currently be browsed in the web UI, but this isn't very powerful, especially if you have thousands of them.
Allow SearchEngine implementations to define exportable fields so that users can "Use Results > Export Data" on any query. In particular, they can use this workflow to download a file with pull logs.
In the future, this can replace the existing "Export to Excel" feature in Maniphest.
For now, we hard-code JSON as the only supported datatype and don't actually make any effort to format the data properly, but this leaves room to add more exporters (CSV, Excel) and data type awareness (integer casting, date formatting, etc) in the future.
For sufficiently large result sets, this will probably time out. At some point, I'll make this use the job queue (like bulk editing) when the export is "large" (affects more than 1K rows?).
Test Plan: Downloaded pull logs in JSON format.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046, T5954
Differential Revision: https://secure.phabricator.com/D18919
Summary:
Depends on D18931. Ref T13048. Ref T13041. This field means "the first accepting reviewer, where order is mostly arbitrary". Modern rules should almost certainly use "Accepting Reviewers" instead.
Getting rid of this completely is a pain, but we can at least reduce confusion by marking it as not-the-new-hotness. Add a "Deprecated" group, move it there, and mark it for exile.
Test Plan:
Edited a commit rule, saw it in "Deprecated" group at the bottom of the list:
{F5395001}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048, T13041
Differential Revision: https://secure.phabricator.com/D18932
Summary:
Depends on D18925. Ref T13048. Currently, HeraldRule stores policies as integers (0 or 1) in the database.
The application tries to mostly use strings ("first", "every"), but doesn't do a good job of hiding the fact that the values are integers deeper in the stack. So we end up with a lot of code like this:
```lang=php
$stored_int_value = $rule->getRepetitionPolicy();
$equivalent_string = HeraldRepetitionPolicyConfig::getString($stored_int_value);
$is_first = ($equivalent_string === HeraldRepetitionPolicyConfig::FIRST);
```
This happens in several places and is generally awful. Replace it with:
```lang=php
$is_first = $rule->isRepeatFirst();
```
To do this, merge `HeraldRepetitionPolicyConfig` into `HeraldRule` and hide all the mess inside the methods.
(This may let us just get rid of the integers in a later change, although I'm not sure I want to commit to that.)
Test Plan:
- Grepped for `HeraldRepetitionPolicyConfig`, no more hits.
- Grepped for `setRepetitionPolicy(...)` and `getRepetitionPolicy(...)`. There are no remaining callers outside of `HeraldRule`.
- Browed and edited several rules. I'll vet this more convincingly after adding the new repetition rule.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18926
Summary: Depends on D18928. Ref T13043. Add some automated test coverage for SSH revocation rules.
Test Plan: Ran tests, got a clean bill of health.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18929
Summary:
Depends on D18912. Ref T13046. Add a UI to browse the existing pull log table.
The actual log still has some significant flaws, but get the basics working.
Test Plan: {F5391909}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18914
Summary:
See PHI305. Ref T13046.
The SSH workflows currently extend `PhabricatorManagementWorkflow` to benefit from sharing all the standard argument parsing code. Sharing the parsing code is good, but it also means they inherit a `getViewer()` method which returns the ommnipotent viewer.
This is appropriate for everything else which extends `ManagementWorkflow` (like `bin/storage`, `bin/auth`, etc.) but not appropriate for SSH workflows, which have a real user.
This caused a bug with the pull logs where `pullerPHID` was not recorded properly. We used `$this->getViewer()->getPHID()` but the correct code was `$this->getUser()->getPHID()`.
To harden this against future mistakes:
- Don't extend `ManagementWorkflow`. Extend `PhutilArgumentWorkflow` instead. We **only** want the argument parsing code.
- Rename `get/setUser()` to `get/setSSHUser()` to make them explicit.
Then, fix the pull log bug by calling `getSSHUser()` instead of `getViewer()`.
Test Plan:
- Pulled and pushed to a repository over SSH.
- Grepped all the SSH stuff for the altered symbols.
- Saw pulls record a valid `pullerPHID` in the pull log.
- Used `echo {} | ssh ... conduit conduit.ping` to test conduit over SSH.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18912
Summary:
Depends on D18904. Ref T13043. If an attacker compromises a victim's session and bypasses their MFA, they can try to guess the user's current account password by making repeated requests to change it: if they guess the right "Old Password", they get a different error than if they don't.
I don't think this is really a very serious concern (the attacker already got a session and MFA, if configured, somehow; many installs don't use passwords anyway) but we get occasional reports about it from HackerOne. Technically, it's better policy to rate limit it, and this should reduce the reports we receive.
Test Plan: Tried to change password over and over again, eventually got rated limited. Used `bin/auth unlimit` to clear the limit, changed password normally without issues.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18906
Summary:
Ref T13043. We have ~4 copies of this logic (registration, lost password recovery, set password, set VCS password).
Currently it varies a bit from case to case, but since it's all going to be basically identical once account passwords swap to the new infrastructure, bring it into the Engine so it can live in one place.
This also fixes VCS passwords not being affected by `account.minimum-password-length`.
Test Plan: Hit all errors in "VCS Password" panel. Successfully changed password.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18902
Summary:
Ref T13043. In D18898 I moved VCS passwords to the new shared infrastructure.
Before account passwords can move, we need to make two changes:
- For legacy reasons, VCS passwords and Account passwords have different "digest" algorithms. Both are more complicated than they should be, but we can't easily fix it without breaking existing passwords. Add a `PasswordHashInterface` so that objects which can have passwords hashes can implement custom digest logic for each password type.
- Account passwords have a dedicated external salt (`PhabricatorUser->passwordSalt`). This is a generally reasonable thing to support (since not all hashers are self-salting) and we need to keep it around so existing passwords still work. Add salt support to `AuthPassword` and make it generate/regenerate when passwords are updated.
Then add a nice story about password digestion.
Test Plan: Ran migrations. Used an existing VCS password; changed VCS password. Tried to use a revoked password. Unit tests still pass. Grepped for callers to legacy `PhabricatorHash::digestPassword()`, found none.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18900
Summary:
Ref T13043. After D18898, this has been migrated to new, more modern storage and no longer has any readers or writers.
One migration from long ago (early 2014) is affected. Since this is ancient and the cost of dropping this is small (see inline), I just dropped it.
I'll note this in the changelog.
Test Plan: Ran migrations, got a clean bill of health from `storage status`. Grepped for removed symbol.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18899
Summary:
Ref T13043. When we verify a password and a better hasher is available, we automatically upgrade the stored hash to the stronger hasher.
Add test coverage for this workflow and fix a few bugs and issues, mostly related to shuffling the old hasher name into the transaction.
This doesn't touch anything user-visible yet.
Test Plan: Ran unit tests.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18897
Summary:
Ref T13043. This provides a new piece of shared infrastructure that VCS passwords and account passwords can use to validate passwords that users enter.
This isn't reachable by anything yet.
The test coverage of the "upgrade" flow (where we rehash a password to use a stronger hasher) isn't great in this diff, I'll expand that in the next change and then start migrating things.
Test Plan: Added a bunch of unit tests.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18896
Summary:
Ref T13043. Currently:
- Passwords are stored separately in the "VCS Passwords" and "User" tables and don't share as much code as they could.
- Because User objects are all over the place in the code, password hashes are all over the place too (i.e., often somewhere in process memory). This is a very low-severity, theoretical sort of issue, but it could make leaving a stray `var_dump()` in the code somewhere a lot more dangerous than it otherwise is. Even if we never do this, third-party developers might. So it "feels nice" to imagine separating this data into a different table that we rarely load.
- Passwords can not be //revoked//. They can be //deleted//, but users can set the same password again. If you believe or suspect that a password may have been compromised, you might reasonably prefer to revoke it and force the user to select a //different// password.
This change prepares to remedy these issues by adding a new, more modern dedicated password storage table which supports storing multiple password types (account vs VCS), gives passwords real PHIDs and transactions, supports DestructionEngine, supports revocation, and supports `bin/auth revoke`.
It doesn't actually make anything use this new table yet. Future changes will migrate VCS passwords and account passwords to this table.
(This also gives third party applications a reasonable place to store password hashes in a consistent way if they have some need for it.)
Test Plan: Added some basic unit tests to cover general behavior. This is just skeleton code for now and will get more thorough testing when applications move.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18894
Summary: Ref T13043. Adds CLI support for revoking SSH keys. Also retargets UI language from "Deactivate" to "Revoke" to make it more clear that this is a one-way operation. This operation is already correctly implemented as a "Revoke" operation.
Test Plan: Used `bin/auth revoke --type ssh` to revoke keys, verified they became revoked (with proper transactions) in the UI. Revoked keys from the web UI flow.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18893
Summary: Ref T13043. Allows CLI revocation of login sessions.
Test Plan: Used `bin/auth revoke --type session` with `--from` and `--everywhere` to revoke sessions. Saw accounts get logged out in web UI.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18892
