Summary: See PHI514. Ref T13114. Ref T8951. When a push is an "initial import" (a push of at least 7 commits to an empty repository) don't run Herald or enormous change protection.
Test Plan: Pushed some non-initial changes to a repository, and some initial changes.
Maniphest Tasks: T13114, T8951
Differential Revision: https://secure.phabricator.com/D19265
Summary: See PHI513. `fprintf()` takes `(thing, pattern, args, ...)` but we aren't passing a `pattern`, so if the command returns a "%" in the output we get an error.
Test Plan:
- Installed `bytes`, a great useful program which prints all the bytes, on my HoaxOS(tm) system (see D19102).
```
epriestley@orbital ~/dev/phabricator $ ./bin/drydock command --lease 76287 -- bytes # Before patch.
[2018-03-29 02:09:08] ERROR 2: fprintf(): Too few arguments at [/Users/epriestley/dev/core/lib/phabricator/src/applications/drydock/management/DrydockManagementCommandWorkflow.php:60]
arcanist(head=experimental, ref.master=b8c9c385a7f5, ref.experimental=925c60e7b837), corgi(head=master, ref.master=6371578c9d32), instances(head=master, ref.master=d983b9517924), ledger(head=master, ref.master=4da4a24b8779), libcore(), phabricator(head=hoax1, ref.master=b586ee065a75, ref.hoax1=f8d7480bbdd1, custom=4), phutil(head=master, ref.master=1ad42491e44a), secure(head=master, ref.master=988cf9bd7958), services(head=master, ref.master=6b3fb8d8dd0a)
#0 fprintf(resource, string) called at [<phabricator>/src/applications/drydock/management/DrydockManagementCommandWorkflow.php:60]
#1 DrydockManagementCommandWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:441]
#2 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:333]
#3 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/drydock/drydock_control.php:21]
epriestley@orbital ~/dev/phabricator $ ./bin/drydock command --lease 76287 -- bytes # After patch.
!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????
```
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19264
Summary: Ref T13114. See PHI510. Firing Herald on mentioned objects tends to feel arbitrary and can substantially slow down edits which mention many objects.
Test Plan: Mentioned tasks on other tasks; verified that the normal path is hit normally, the new Herald-free path is hit on the mentioned object, and both still work fine and show up in the timeline.
Maniphest Tasks: T13114
Differential Revision: https://secure.phabricator.com/D19263
Summary: Depends on D19259. Ref T13105. Some examples represent image data as `["da", "ta"]` while others represent it as `"data"`. Accept either.
Test Plan: Rendered example notebooks with both kinds of images.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19260
Summary: Ref T13105. Currently, logged-out users can't render documents via the endpoint even if they otherwise have access to the file.
Test Plan: Viewed a file as a logged-out user and re-rendered it via Ajax.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19258
Summary:
Ref T13105. This adds various small client-side improvements to document rendering.
- In the menu, show which renderer is in use.
- Make linking to lines work.
- Make URIs persist information about which rendering engine is in use.
- Improve the UI feedback for transitions between document types.
- Load slower documents asynchronously by default.
- Discard irrelevant requests if you spam the view menu.
Test Plan: Loaded files, linked to lines, swapped between modes, copy/pasted URLs.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19256
Summary: Depends on D19254. This engine just formats JSON files in a nicer, more readable way.
Test Plan: Looked at some JSON files, saw them become formatted nicely.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Differential Revision: https://secure.phabricator.com/D19255
Summary: Ref T13105. Allow normal text files to be rendered as documents, and add a "source code" rendering engine.
Test Plan: Viewed some source code.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19254
Summary:
Depends on D19252. Ref T13105. This very roughly renders Jupyter notebooks.
It's probably better than showing the raw JSON, but not by much.
Test Plan:
- Viewed various notebooks with various cell types, including markdown, code, stdout, stderr, images, HTML, and Javascript.
- HTML and Javascript are not live-fired since they're wildly dangerous.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19253
Summary:
Depends on D19251. Ref T13105. This adds rendering engine support for PDFs.
It doesn't actually render them, it just renders a link which you can click to view them in a new window. This is much easier than actually rendering them inline and at least 95% as good most of the time (and probably more-than-100%-as-good some of the time).
This makes PDF a viewable MIME type by default and adds a narrow CSP exception for it. See also T13112.
Test Plan:
- Viewed PDFs in Files, got a link to view them in a new tab.
- Clicked the link in Safari, Chrome, and Firefox; got inline PDFs.
- Verified primary CSP is still `object-src 'none'` with `curl ...`.
- Interacted with the vanilla lightbox element to check that it still works.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19252
Summary:
Ref T13105. Although Markdown is trickier to deal with, we can handle Remarkup easily.
This may need some support for encoding options.
Test Plan: Viewed `.remarkup` files, got remarkup document presentation by default. Viewed other text files, got an option to render as remarkup.
Reviewers: avivey
Reviewed By: avivey
Subscribers: mydeveloperday, avivey
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19251
Summary:
Depends on D19249. Ref T13109. Add timing information to the `PushEvent`:
- `writeWait`: Time spent waiting for a write lock.
- `readWait`: Time spent waiting for a read lock.
- `hostWait`: Roughly, total time spent on the leaf node.
The primary goal here is to see if `readWait` is meaningful in the wild. If it is, that motivates smarter routing, and the value of smarter routing can be demonstrated by looking for a reduction in read wait times.
Test Plan: Pushed some stuff, saw reasonable timing values in the table. Saw timing information in "Export Data".
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19250
Summary:
Depends on D19247. Ref T13109. When we receive an SSH request, generate a random unique ID for the request. Then thread it down through the process tree.
The immediate goal is to let the `ssh-exec` process coordinate with `commit-hook` process and log information about read and write lock wait times. Today, there's no way for `ssh-exec` to interact with the `PushEvent`, but this is the most helpful place to store this data for users.
Test Plan: Made pushes, saw the `PushEvent` table populate with a random request ID. Exported data and saw the ID preserved in the export.
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19249
Summary:
Ref T13109. Make it slightly more clear what the scope of the write and read locks are, and slightly more clear that we're actively acquiring locks, not just sitting around waiting.
While waiting on another writer, show who we're waiting on so you can walk over to their desk and glare at them.
Test Plan:
Added `sleep(15)` after `willWrite()`. Pushed in two windows. Saw new, more informative messages. In the second window, saw the new guidance:
> # Waiting for hector to finish writing (on device "repo1.local.phacility.net" for 11s)...
Reviewers: asherkin
Reviewed By: asherkin
Subscribers: asherkin
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19247
Summary:
See PHI466. Ref T13108. Somewhat recently, new rules were added so that "Resigning" from a revision takes you off the default recipient list, even if you're still a member of a project or package that is still a reviewer or subscriber.
However, these rules don't currently apply to the similar expansion which occurs in notifications. If you resign from a revision you may still get some notifications (just not email) if a package or project you're a member of is a reviewer or subscriber.
(Possibly these should eventually share more code, but just get things working for now.)
Test Plan:
- Created a revision as A.
- Added B as a reviewer.
- Added a package B is an owner for as a reviewer.
- As B, resigned. (Make sure B is also not an explicit subscriber.)
- Commented on the revision as A.
- Before: B is included in the expanded notification recipient list.
- After: B is no longer included in the expanded notification recipient list.
Maniphest Tasks: T13108
Differential Revision: https://secure.phabricator.com/D19244
Summary:
This change prevents the following error when using PHP 7.2:
```
ERROR 2: count(): Parameter must be an array or an object that implements Countable at [/usr/local/lib/php/phabricator/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php:132]
```
A similar issue was fixed in D18964
Test Plan: Tested in a live system.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19242
Summary:
Depends on D19238. Ref T13105. Give document engines some reasonable automatic support for degrading gracefully when someone tries to hexdump a 100MB file or similar.
Also, make "Video" sort above "Audio" for files which could be rendered either way.
Test Plan: Viewed audio, video, image, and other files. Adjusted limits and saw full, partial, and fallback/error rendering.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19239
Summary: Depends on D19237. Ref T13105. This adds a (very basic) "Hexdump" engine (mostly just to have a second option to switch to) and a selector for choosing view modes.
Test Plan: Viewed some files, switched between audio/video/image/hexdump.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19238
Summary:
Ref T13105. This change begins modularizing document rendering. I'm starting in Files since it's the use case with the smallest amount of complexity.
Currently, we hard-coding the inline rendering for images, audio, and video. Instead, use the modular engine pattern to make rendering flexible and extensible.
There aren't any options for switching modes yet and none of the renderers do anything fancy. This API is also probably very unstable.
Test Plan: Viewwed images, audio, video, and other files. Saw reasonable renderings, with "nothing can render this" for any other file type.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19237
Summary:
DarkConsole could warn when "Analyze Query Plans" was not active.
`msort()` is not stable, so Ferret results with similar relevance could be returned out-of-order.
Test Plan: Saw fewer traces and more-stable result ordering.
Differential Revision: https://secure.phabricator.com/D19236
Summary:
Ref T13108. See PHI364. See the task and issue for discussion.
If a `git fetch` during synchronization hangs, the whole node currently hangs. While the causes of a `git fetch` hang aren't clear, we don't expect synchronization to ever reasonably take more than 15 minutes, so add a default timeout.
Test Plan: Will deploy and observe; this is difficult to reproduce or test directly.
Maniphest Tasks: T13108
Differential Revision: https://secure.phabricator.com/D19235
Summary:
See PHI430. Ref T13102. When the "Build Status" element raises a policy exception, we currently fatal the whole page rather than raising a normal policy error.
This is because the policy check happens very late in page construction, long after we've made the decision to show the page instead of a policy error, and gets treated as a rendering error.
In turn, this is because the rendering is event-based rather than using a more modern Engine + EngineExtension sort of construct, so some of the actual logic runs way later than it should.
Since unwinding all of this isn't trivial and the current behavior is materially bad, limit the damage here for now by just hiding the element. See T13088 for notes on handling this in a more nuanced way in the future.
Test Plan:
- Created a revision visible to "Public".
- Ran a build against it with a build plan visible to "All Users".
- Viewed revision in an incognito window.
- Before patch: Policy fatal with a red "rendering phase" error box.
- After patch: Mostly-functional page with a missing "Build Status" element.
- Viewed revision as a user with a normal session, saw the same UI before and after the change.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19232
Summary:
Ref T13102. See PHI461. An install is interested in querying projects by slug.
I think I omitted this capability originally only because we're not consistent about what slugs are called (they are "Slugs" internally, but "Hashtags" in the UI).
However, this ship has sort of already sailed because the results have a "slug" field. Just expose this as "slugs" for consistency with the existing API field and try to smooth thing over with a little documentation hint.
Test Plan: Queried for projects by slug, got the desired results back.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19230
Summary: Ref T13106. When profiling service queries, there's no convenient way to easily get a sense of why a query was issued. Add a mode to collect traces for each query to make this more clear. This is rough, but works well enough to be useful.
Test Plan: Clicked "Analyze Query Plans", got stack traces for each service call.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19221
Summary:
See PHI457. There's no real reason not to allow this, it just wasn't clear if it was useful. See D18626.
An install had a user `arc diff` and then sprint out the door to take a very long vacation before the builds finished. One failed, so the revision is stuck as a draft forever. This seems like a reasonable motivation for allowing "Commandeer".
Test Plan: Successfully commandeered a draft.
Differential Revision: https://secure.phabricator.com/D19228
Summary: See PHI448. Ref T13106. The current implementation here can end up in an infinite stack if, e.g., a project uses "Visible to: Subscribers".
Test Plan: Will push.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19226
Summary: Depends on D19224. Ref T13106. Computing this is expensive and the value is not used. This came from D15432, but we never actually shipped that feature.
Test Plan: Saw local query cost drop from 139 to 110 with no change in functionality. Grepped for removed symbols.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19225
Summary:
Depends on D19223. Ref T13106. When we're loading a file, we currently test if it's a transformed version of another file (usually, a thumbnail) and apply policy behavior if it is.
We know that builtins and profile images are never transforms and that the policy behavior for these files doesn't matter anyway. Skip loading transforms for these files.
Test Plan: Saw local queries drop from 146 to 139 with no change in behavior.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19224
Summary:
Depends on D19222. Ref T13106. We currently execute an edge query (and possibly an object query) when loading builtin files, but this is never necessary because we know these files are always visible.
Instead, skip this logic for builtin files and profile image files; these files have global visibility and will never get a different policy result because of file attachment information.
(In theory, we could additionally skip this for files with the most open visibility policy or some other trivially visible policy like the user's PHID, but we do actually care about the attachment data some of the time.)
Test Plan: Saw queries drop from 151 to 145 on local test page. Checked file attachment data in Files, saw it still working correctly.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19223
Summary: Depends on D19221. Ref T13106. When we fall back to default profile images for projects, bulk load them instead of doing individual queries.
Test Plan: Saw local task drop from 199 queries to 151 queries with the same actual outcome. Saw custom and default profile images on the project list page.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19222
Summary:
See PHI451. Ref T13102. DarkConsole uses an ancient inline "onclick" handler to expand the stack traces for errors.
The new Content-Security-Policy prevents this from functioning.
Replace this with a more modern behavior-driven action instead.
Test Plan:
- Clicked some errors in DarkConsole, saw stack traces appear.
- Grepped for `onclick` and `jsprintf()` to see if I could find any more of these, but came up empty.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19218
Summary:
See PHI446. Ref T13088. Currently, there's no way to access older generations of a build unless you know the secret `?g=1` URI magic.
When a build has multiple generations, show a history table and let users click to see older run information.
This is currently very basic. It would be nice to show when each generation started, who started/restarted it, and what the build status was at the time the build was restarted. There's currently no convenient source for this information so just add a bare-bones, working version of this for now.
Test Plan:
Viewed pending, single-run and multi-restart builds. Saw table on builds with more than one generation. Clicked table entries to see different build data.
{F5471160}
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19217
Summary:
Fixes T12994. We need `MYSQLI_ASYNC` to implement client-side query timeouts, and we need MySQLi + MySQL Native Driver to get `MYSQLI_ASYNC`.
Recommend users install MySQLi and MySQL Native Driver if they don't have them. These are generally the defaults and best practice anyway, but Ubuntu makes it easy to use the older stuff.
All the cases we're currently aware of stem from `apt-get install php5-mysql` (which explicitly selects the non-native driver) so issue particular guidance about `php5-mysqlnd`.
Test Plan:
- Faked both issues locally, reviewed the text.
- Will deploy to `secure`, which currently has the non-native driver.
Maniphest Tasks: T12994
Differential Revision: https://secure.phabricator.com/D19216
Summary:
Ref T13102. An install has a custom rule for bridging JIRA references via Doorkeeper and would like to be able to render them as `JIRA-123` instead of `JIRA JIRA-123 Full JIRA title`.
I think it's reasonable to imagine future support upstream for `JIRA-123`, `{JIRA-123}`, and so on, although we do not support these today. We can take a small step toward eventual support by letting the rendering pipeline understand different view modes.
This adds an optional `name` (the default text rendered before we do the OAuth sync) and an optional `view`, which can be `short` or `full`.
Test Plan:
I tested this primarily with Asana, since it's less of a pain to set up than JIRA. The logic should be similar, hopefully.
I changed `DoorkeeperAsanaRemarkupRule` to specify `name` and `view`, e.g `'view' => (mt_rand(0, 1) ? 'short' : 'full')`. Then I made a bunch of Asana references in a comment and saw them randomly go short or long.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19215
Summary:
Fixes T5741. We break GIFs apart with "-coalesce" which completely rasterizes each frame, but stitch them back together without specifying "-dispose".
This produces the default "-dispose none" behavior, which causes GIF frames to "pile up" if they contain transparency.
Instead, use "-dispose background" so that the previous frame is erased before each new frame is drawn.
Test Plan: See T5741 for additional details.
Maniphest Tasks: T5741
Differential Revision: https://secure.phabricator.com/D19214
Summary:
Fixes T13104. The "Subscribers" policy implementation still uses older logic to query project membership and misses parent projects and milestones which a user is a member of.
Instead of doing an edge query for explicit membership, use a project query to find all projects the viewer belongs to.
Test Plan:
- Created a parent project A.
- Created a subproject B.
- As Bailey, created a task with "Visible To: Bailey, Subscribers".
- Added parent project A as a task subscriber.
Then:
- As Alice, verified I could not see the task.
- As Alice, joined subproject B.
- Before patch: still unable to see the task.
- After patch: can see the task.
- Removed parent project A as a subscriber, verified I could no longer see the task.
Maniphest Tasks: T13104
Differential Revision: https://secure.phabricator.com/D19213
Summary:
See PHI431. Ref T13102. An install is interested in a custom "non-sticky" accept action, roughly.
On the one hand, this is a pretty hacky patch. However, I suspect it inches us closer to T731, and I'm generally comfortable with exploring the realms of "Accept Next Update", "Unblock Without Accepting", etc., as long as most of it doesn't end up enabled by default in the upstream.
Test Plan:
- Accepted and updated revisions normally, saw accepts respect global stickiness.
- Modified the "Accept" action to explicitly be unsticky, saw nonsticky accept behavior after update.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19211
Summary: See PHI433. Ref T13102. Users in the wild have mixed expecations about exactly what "draft" means. Recent changes have tried to make behavior more clear. As part of clarifying messaging, make it explicit that `@mention` does not work on drafts by showing users a warning when they try to `@mention` a user.
Test Plan:
- Mentioned users on drafts, got a warning.
- Posted normal comments on drafts, no warning.
- Posted normal/mention comments on non-drafts, no warning.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19210
Summary: Ref T13103. Make favicons customizable, and perform dynamic compositing to add marker to indicate things like "unread messages".
Test Plan: Viewed favicons in Safari, Firefox and Chrome. With unread messages, saw pink dot composited into icon.
Maniphest Tasks: T13103
Differential Revision: https://secure.phabricator.com/D19209
Summary: See PHI439. Use slightly richer "dominion" return values for consistency.
Test Plan: Fetched results with `owners.search` API method.
Differential Revision: https://secure.phabricator.com/D19208
Summary:
See PHI439. This fills in additional information about Owners packages.
Also removes dead `primaryOwnerPHID`.
Test Plan: Called `owners.search` and reviewed the results. Grepped for `primaryOwnerPHID`.
Differential Revision: https://secure.phabricator.com/D19207
Summary: See PHI433. This beefs up reminder texts for drafts a little bit since some users in the wild aren't always seeing/remembering the existing, fairly subtle hints.
Test Plan: Created a reivsion with `--draft`, viewed it, saw richer reminders.
Differential Revision: https://secure.phabricator.com/D19204
Summary:
Depends on D19201. Ref T13101. This likely produces relatively stable-ish image references for email.
They currently TTL after 30 days but this makes the jokes more exclusive and special so it's a feature, not a bug.
Test Plan: I'm just going to test this in production because I'm a ninja superstar developer.
Maniphest Tasks: T13101
Differential Revision: https://secure.phabricator.com/D19203
Summary:
Depends on D19198. Ref T13101. Ref T5258. Pull compositing logic out of the `Controller`.
This is moving toward fixing memes in email.
Test Plan: Used new and old memes. Used API memes.
Maniphest Tasks: T13101, T5258
Differential Revision: https://secure.phabricator.com/D19200
Summary: See PHI432. Ref T13099. Short names never made it to the UI/API but seem stable now, so support them.
Test Plan: {F5465173}
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19202
Summary:
Ref T13101. This is a minimal change to make "{meme ...}" work with the new Content-Security-Policy by using an Ajax request to generate the image and then swapping the source on the client.
This could be much cleaner (see T5258, etc).
Test Plan: Used `{meme, src=cat6, above=i am, below=cat}`, chuckled completely unironically.
Maniphest Tasks: T13101
Differential Revision: https://secure.phabricator.com/D19196
Summary: Depends on D19194. Fixes T4190. This should be in good-enough shape now to release and support more generally.
Test Plan: Used `{image ...}` in remarkup.
Maniphest Tasks: T4190
Differential Revision: https://secure.phabricator.com/D19195
Summary: Depends on D19193. Ref T13101. Fixes T4190. Before we render a fancy AJAX placeholder, check if we already have a valid cache for the image. If we do, render a direct `<img />` tag. This is a little cleaner and, e.g., avoids flicker in Safari, at least.
Test Plan: Rendered `{image ...}` rules in remarkup with new and existing URIs.
Maniphest Tasks: T13101, T4190
Differential Revision: https://secure.phabricator.com/D19194
Summary:
Depends on D19192. Ref T4190. Ref T13101. Instead of directly including the proxy endpoint with `<img src="..." />`, emit a placeholder and use AJAX to make the request. If the proxy fetch fails, replace the placeholder with an error message.
This isn't the most polished implementation imaginable, but it's much less mysterious about errors.
Test Plan: Used `{image ...}` for valid and invalid images, got images and useful error messages respectively.
Maniphest Tasks: T13101, T4190
Differential Revision: https://secure.phabricator.com/D19193
Summary:
Ref T13101. Ref T4190. This rule is currently single-phase but I'd like to check for a valid proxied image in cache already and just emit an `<img ... />` tag pointing at it if we have one.
To support batching these lookups, split the rule into a parse phase (where we extract URIs) and a markup phase (where we build tags).
Test Plan: Used `{img ...}` in Remarkup with no apparent behavioral changes. (This change should do nothing on its own.)
Maniphest Tasks: T13101, T4190
Differential Revision: https://secure.phabricator.com/D19192
Summary: Depends on D19190. Fixes T12590. Ref T13099. Replaces the barely-usable, gigantic, poorly ordered "<select />" control with a tokenizer. Attempts to fix various minor issues.
Test Plan:
- Edited paths: include/exclude paths, from different repositories, different actual paths.
- Used "Add New Path" to add rows, got repository selector prepopulated with last value.
- Used "remove".
- Used validation typeahead, got reasonable behaviors?
The error behavior if you delete the repository for a path is a little sketchy still, but roughly okay.
Maniphest Tasks: T13099, T12590
Differential Revision: https://secure.phabricator.com/D19191
Summary:
Depends on D19189. Ref T12590. The "validate" and "complete" endpoints for this UI could incorrectly return redirect responses. These aren't critical to the behavior of Owners, but they're nice to have, and shouldn't redirect.
Instead, skip the canonicalizing redirect for AJAX requests.
Test Plan: Edited Owners paths in a repository with a short name, got completion/validation again.
Maniphest Tasks: T12590
Differential Revision: https://secure.phabricator.com/D19190
Summary: Ref T12590. This is ancient code which was used to prefill `/trunk/tfb/www/` or similar at Facebook. I don't think it ever had a UI and no install has asked for this feature since 2011.
Test Plan: Grepped for affected symbols, edited paths in Owners.
Maniphest Tasks: T12590
Differential Revision: https://secure.phabricator.com/D19189
Summary: See PHI425. See T12678. This should be an integer, but may be a string.
Test Plan: Called `differential.revision.edit`, observed integer in result instead of string.
Differential Revision: https://secure.phabricator.com/D19186
Summary: Depends on D19184. Ref T11015. Now that we have a digest index column, we can improve some of the queries a bit.
Test Plan:
- Ran queries from revision pages before and after with and without EXPLAIN.
- Saw the same results with much better EXPLAIN plans.
- Fragment size is now fixed at 12 bytes per fragment, so we can shove more of them in a single query.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19185
Summary:
Depends on D19183. Ref T11015. Currently, adding a trailing slash works great and omitting it mysteriously doesn't work.
Store a normalized version with an unconditional trailing slash for the lookup logic to operate on, and a separate display version which tracks what the user actually typed.
Test Plan:
- Entered "/src/main.c", "/src/main.c/", saw them de-duplicate.
- Entered "/src/main.c", saw it stay that way in the UI but appear as "/src/main.c/" internally.
- Added a rule for "/src/applications/owners" (no slash), created a revision touching paths in that directory, saw Owners fire for it.
- Changed the display value of a path only ("/src/main.c" to "/src/main.c/"), saw the update reflected in the UI without any beahvioral change.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19184
Summary:
Depends on D19182. Ref T11015. This changes `path` from `text255` to `longtext` because paths may be arbitrarily long.
It adds `pathDisplay` to prepare for display paths and storage paths having different values. For now, `pathDisplay` is copied from `path` and always has the same value.
Test Plan:
- Ran migration, checked database for sanity (all `pathDisplay` and `path` values identical).
- Added new paths, saw `pathDisplay` and `path` get the same values.
- Added an unreasonably enormous path with far more than 255 characters.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19183
Summary:
Depends on D19181. Ref T11015. This nukes duplicates from the table if they exist, then adds a unique key.
(Duplicates should not exist and can not be added with any recent version of the web UI.)
Test Plan:
- Tried to add duplicates with web UI, didn't have any luck.
- Explicitly added duplicates with manual `INSERT`s.
- Viewed packages in web UI and saw duplicates.
- Ran migrations, got a clean purge and a nice unique key.
- There's still no way to actually hit a duplicate key error in the UI (unless you can collide hashes, I suppose), this is purely a correctness/robustness change.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19182
Summary: Ref T11015. This supports making path names arbitrarily long and putting a proper unique key on the table.
Test Plan:
- Migrated, checked database, saw nice digested indexes.
- Edited a package, saw new rows update with digested indexes.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19181
Summary: Ref T13099. See PHI424. Fixes T11664. Several installs are interested in having these behaviors available in Owners by default and they aren't difficult to provide, it just makes the UI kind of messy. But I think there's enough general interest to justify it, now.
Test Plan: Created a package which owns "/" with a "With Non-Owner Author" review rule which I own. Created a revision, no package reviewer. Changed rule to "All", updated revision, got package reviewer.
Maniphest Tasks: T13099, T11664
Differential Revision: https://secure.phabricator.com/D19180
Summary:
Ref T13099. Ref T12787. See PHI417. Differential has new "irresponsible" warnings in the timeline somewhat recently, but these publish feed stories that don't link to the revision or have other relevant details, so they're confusing on the balance.
These have a high strength so they render on top, but we actually just want to hide them from the feed and let "abraham closed Dxyz by committing rXzzz." be the primary story.
Modularize things more so that we can get this behavior. Also, respect `shouldHideForFeed()` at display time, not just publishing time.
Test Plan: Used `bin/differential attach-commit` on a non-accepted revision to "irresponsibly land" a revision. Verified that feed story now shows "closed by commit" instead of "closed irresponsibly".
Maniphest Tasks: T13099, T12787
Differential Revision: https://secure.phabricator.com/D19179
Summary: Ref T13100. Since rules may begin failing for PRCE configuration reasons soon, provide a more complete explanation of possible causes in the UI.
Test Plan: Faked this, hit it via test console, saw explanation in web UI.
Maniphest Tasks: T13100
Differential Revision: https://secure.phabricator.com/D19178
Summary:
Ref T13099. See that task for discussion. Chrome is unhappy with an MFA form submitting to an endpoint which redirects you to an OAuth URI.
Instead, do the redirect entirely on the client.
Chrome's rationale here isn't obvious, so we may be able to revert this at some point.
Test Plan: Went through the OAuth flow locally, was redirected on the client. Will verify in production.
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19177
Summary: Depends on D19175. Ref T13099. This fills in "close" and "update" transactions so that they show which commit(s) caused the action.
Test Plan: Used `transaction.search` to query some revisions, saw commit PHID information.
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19176
Summary: Ref T13099. Move most of the "Update" logic to modular transactions
Test Plan: Created and updated revisions. Flushed the task queue. Grepped for `TYPE_UPDATE`. Reviewed update transactions in the timeline and feed.
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19175
Summary: Depends on D19173. Ref T13096. Adds an optional, disabled-by-default lock log to make it easier to figure out what is acquiring and holding locks.
Test Plan: Ran `bin/lock log --enable`, `--disable`, `--name`, etc. Saw sensible-looking output with log enabled and daemons restarted. Saw no additional output with log disabled and daemons restarted.
Maniphest Tasks: T13096
Differential Revision: https://secure.phabricator.com/D19174
Summary:
Ref T13096. Currently, we do a fair amount of clever digesting and string manipulation to build lock names which are less than 64 characters long while still being reasonably readable.
Instead, do more of this automatically. This will let lock acquisition become simpler and make it more possible to build a useful lock log.
Test Plan: Ran `bin/repository update`, saw a reasonable lock acquire and release.
Maniphest Tasks: T13096
Differential Revision: https://secure.phabricator.com/D19173
Summary:
See PHI416. If you raise a lint message in a deleted file, we don't render any text on the right hand side so the message never displays.
This is occasionally still legitimate/useful, e.g. to display a "don't delete this file" message. At least for now, show these messages on the left.
Test Plan: Posted a lint message on a deleted file via `harbormaster.sendmessage`, viewed revision, saw file expand with synthetic inline for lint.
Differential Revision: https://secure.phabricator.com/D19171
Summary: See PHI193. Previously, see similar D18763. Skip this legacy-style policy check when creating a project, since we know you can add members, even if the policy doesn't actually resolve in your favor.
Test Plan:
- Created a project with edit policy "Members of project" and myself, plus any other user (so the code goes down this path, not the "join/leave" path) as members.
Differential Revision: https://secure.phabricator.com/D19169
Summary:
See PHI413. You can pre-generate these with `bin/people profileimage --all`, but they're needlessly expensive to generate.
Streamline the workflow and cache some of the cacheable parts to reduce the generation cost.
Test Plan:
- Ran `bin/people profileimage --all` and saw cost drop from {nav 15.801s > 4.839s}.
- Set `defaultProfileImagePHID` to `NULL` in `phabricator_user.user` and purged caches with `bin/cache purge --all`.
- Loaded user directory.
- Saw default images regenerate relatively quickly.
Differential Revision: https://secure.phabricator.com/D19168
Summary:
Depends on D19166. Ref T13088. When the user scrolls away from a followed log, break the focus lock.
Let users stop following a live log.
Show when lines are added more clearly.
Don't refresh quite as quickly give users a better shot at clicking the stop button.
These behaviors can probably be refined but are at least more plausible and less actively user-hostile than the first version of this behavior was.
Test Plan: Used `write-log --rate` to write a large log slowly. Clicked "Follow Log", followed for a bit. Scrolled away, still got live updates but no more scroll lock. Clicked stop, no more updates.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19167
Summary:
Depends on D19165. Ref T13088. Currently, in other applications, we use Zero Width Spaces and Javascript "copy" listeners to prevent line numbers from being copied. This isn't terribly elegant.
Modern browsers support a second approach: using psuedo-elements with `content`. Try this in Harbormaster since it's conceptually cleaner, at least. One immediate drawback is that Command-F can't find this text either.
Test Plan: In Safari, Chrome and Firefox, highlighted ranges of lines and copy/pasted text. Got just text (no line numbers) in all cases.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19166
Summary: Depends on D19164. Ref T13088. Now that the JS behaviors are generic, use them on the Harbormaster standalone page.
Test Plan: Clicked lines and dragged across line ranges. Reloaded pages. Saw expected highlighting behavior in the client and on the server across reloads.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19165
Summary:
Depends on D19162. Ref T13088. When a user links to `$1234`, we need to render a default view of the log with a piece at the head, a piece at the end, and a piece in the middle.
We also need to figure out the offset for line 1234, or multiple offsets for "1234-2345".
Since the logic views/reads mostly anticipated this it isn't too much of a mess, although there are a couple of bugs this exposes with view specifications that use combinations of parameters which were previously impossible.
Test Plan: Viewed a large log with no line marker. Viewed `$1`. Viewed `$end`. Viewed `$35-40`, etc. Expanded context around logs.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19163
Summary: Ref T13088. This lifts the code for parsing "$x-y" line ranges in URIs into AphrontRequest so Diffusion, Paste, Harbormaster, etc., can share it.
Test Plan: Viewed lines, line ranges, no lines, negative line ranges, line ranges with 0, and extremely long line ranges in Paste.
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19162
Summary:
Ref T4340. Some "Register/Login" and "Link External Account" buttons are forms which submit to third-party sites. Whitelist these targets when pages render an OAuth form.
Safari, at least, also prevents a redirect to a third-party domain after a form submission to the local domain, so when we first redirect locally (as with Twitter and other OAuth1 providers) we need to authorize an additional URI.
Test Plan: Clicked all my registration buttons locally without hitting CSP issues.
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19159
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
Summary:
Depends on D19154. Ref T13094. This controller was removed at some point and this route no longer works.
I plan to add a new `download/` route to let us tighten the `form-action` Content Security Policy.
Test Plan: Grepped for the route and controller, no hits.
Maniphest Tasks: T13094
Differential Revision: https://secure.phabricator.com/D19155
Summary: Depends on D19152. Ref T13088. This adds live log tailing. It is probably not the final version of this feature because it prevents escape once you begin tailing a log.
Test Plan: Used `bin/harbormaster write-log --rate ...` to write a log slowly. Viewed it in the web UI. Clicked "Follow Log". Followed the log until the write finished, a lifetime later.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19153
Summary: Depends on D19151. Ref T13088. While dramatically less exciting than using `lolcat` and less general than `pv`, this should do the job adequately.
Test Plan: Piped a sizable log into `bin/harbormaster write-log` with `--rate 2048`, saw a progress bar. Loaded the log in the web UI and saw it grow as the page reloaded.
Reviewers: yelirekim
Reviewed By: yelirekim
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19152
Summary:
Depends on D19150. Ref T13088. Allow clients to retrieve information about build logs, including log data, over the API.
(To fetch log data, take the `filePHID` to `file.search`, then issue a normal GET against the URI. Use a `Content-Range` header to get part of the log.)
Test Plan: Ran `harbormaster.log.search`, got sensible-looking results.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19151
Summary: Depends on D19149. Ref T13088. Since the new log requires a bunch of log reprocessing, the cutover is going to require at least some time for installs to run migrations. Add a link in the UI to ease the transition, smooth over some behaviors a little, and fix a fetch issue where we'd request past the end of the log (since this is now enforced).
Test Plan: Viewed a traditional Harbormaster build, saw links to the new standalone log pages.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19150
Summary: Depends on D19148. Ref T13088. The new rendering always executes range requests for data it needs, and we can satisfy these requests by loading the smallest number of chunks which span that range.
Test Plan: Piped 50,000 lines of Apache log into Harbormaster, viewed it in the new UI, got sensible rendering times and a reasonable amount of data actually going over the wire.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19149
Summary: Ref T13088. This variable bled through from an earlier loop and caused us to drop some of the lines in the middle.
Test Plan: Clicked "Show More", got an equal number of header and footer lines.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19148
Summary: Ref T13093. Depends on D19145. See PHI398. Previously, see D18933. This provides the current viewer to `ConduitCall` so that we don't try to use device credentials from unprivileged web hosts.
Test Plan: Evaluated the "Branches" field locally, saw an appropriate field value.
Maniphest Tasks: T13093
Differential Revision: https://secure.phabricator.com/D19146
Summary:
Ref T13093. See PHI396. These are possibly somewhat niche, but reasonable to support and consistent with the existing "Pusher's projects".
Also relabel "Pusher's projects" and "Project tags" for consistency and, hopefully, clarity.
Test Plan:
- Created new "commit" and "hook: commit content" Herald rules which run against "Author's projects" and "Committer's projects".
- Test console'd the "Commit" rules.
- Pushed through the "Hook" rule.
- In all cases, saw fields populate appropriately.
Maniphest Tasks: T13093
Differential Revision: https://secure.phabricator.com/D19145
Summary:
See PHI399. Ref T4340. This header provides an additional layer of protection against various attacks, including XSS attacks which embed inline `<script ...>` or `onhover="..."` content into the document.
**style-src**: The "unsafe-inline" directive affects both `style="..."` and `<style>`. We use a lot of `style="..."`, some very legitimately, so we can't realistically get away from this any time soon. We only use one `<style>` (for monospaced font preferences) but can't disable `<style>` without disabling `style="..."`.
**img-src**: We use "data:" URIs to inline small images into CSS, and there's a significant performance benefit from doing this. There doesn't seem to be a way to allow "data" URIs in CSS without allowing them in the document itself.
**script-src** and **frame-src**: For a small number of flows (Recaptcha, Stripe) we embed external javascript, some of which embeds child elements (or additional resources) into the document. We now whitelist these narrowly on the respective pages.
This won't work with Quicksand, so I've blacklisted it for now.
**connect-src**: We need to include `'self'` for AJAX to work, and any websocket URIs.
**Clickjacking**: We now have three layers of protection:
- X-Frame-Options: works in older browsers.
- `frame-ancestors 'none'`: does the same thing.
- Explicit framebust in JX.Stratcom after initialization: works in ancient IE.
We could probably drop the explicit framebust but it wasn't difficult to retain.
**script tags**: We previously used an inline `<script>` tag to start Javelin. I've moved this to `<data data-javelin-init ...>` tags, which seems to work properly.
**`__DEV__`**: We previously used an inline `<script>` tag to set the `__DEV__` mode flag. I tried using the "initialization" tags for this, but they fire too late. I moved it to `<html data-developer-mode="1">`, which seems OK everywhere.
**CSP Scope**: Only the CSP header on the original request appears to matter -- you can't refine the scope by emitting headers on CSS/JS. To reduce confusion, I disabled the headers on those response types. More headers could be disabled, although we're likely already deep in the land of diminishing returns.
**Initialization**: The initialization sequence has changed slightly. Previously, we waited for the <script> in bottom of the document to evaluate. Now, we go fishing for tags when domcontentready fires.
Test Plan:
- Browsed around in Firefox, Safari and Chrome looking for console warnings. Interacted with various Javascript behaviors. Enabled Quicksand.
- Disabled all the framebusting, launched a clickjacking attack, verified that each layer of protection is individually effective.
- Verified that the XHProf iframe in Darkconsole and the PHPAST frame layout work properly.
- Enabled notifications, verified no complaints about connecting to Aphlict.
- Hit `__DEV__` mode warnings based on the new data attribute.
- Tried to do sketchy stuff with `data:` URIs and SVGs. This works but doesn't seem to be able to do anything dangerous.
- Went through the Stripe and Recaptcha workflows.
- Dumped and examined the CSP headers with `curl`, etc.
- Added a raw <script> tag to a page (as though I'd found an XSS attack), verified it was no longer executed.
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19143
Summary:
Depends on D19141. Ref T13088. Some of the fundamental log behaviors like "loading the correct rows" are now a bit better behaved.
The UI is a little less garbage, too.
Test Plan: Viewed some logs and loaded more context by clicking the buttons.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19142
Summary: Depends on D19139. Ref T13088. This doesn't actually work, but is close enough that a skilled attacker might be able to briefly deceive a small child.
Test Plan:
- Viewed some very small logs under very controlled conditions, saw content.
- Larger logs vaguely do something resembling working correctly.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19141
Summary:
Depends on D19138. Ref T13088. When we want to read the last part of a logfile //and show accurate line numbers//, we need to be able to get from byte offsets to line numbers somehow.
Our fundamental unit must remain byte offsets, because a test can emit an arbitrarily long line, and we should accommodate it cleanly if a test emits 2GB of the letter "A".
To support going from byte offsets to line numbers, compute a map with periodic line markers throughout the offsets of the file. From here, we can figure out the line numbers for arbitrary positions in the file with only a constant amount of work.
Test Plan: Added unit tests; ran unit tests.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19139
Summary: Depends on D19137. Ref T13088. This allows `rebuild-log` to skip work if the chunks are already compressed. It also prepares for a future GC which is looking for "text" or "gzip" chunks to throw away in favor of archival into Files; such a GC can use this column to find collectable logs and then write "file" to it, meaning "chunks are gone, this data is only available in Files".
Test Plan: Ran migration, saw logs populate as "text". Ran `rebuild-log`, saw logs rebuild as "gzip".
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19138
Summary: Depends on D19136. Ref T13088. Since it's probably impractical to do all the migrations these changes imply during `bin/storage upgrade`, provide some support for performing them online.
Test Plan: Ran `bin/harbormaster rebuild-log` with `--all`, `--id`, and with and without `--force`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19137
Summary: Depends on D19135. Ref T13088. Denormalize the total log size onto the log itself. This makes reasoning about the log at display time easier, and we don't need to fish around in the database as much to figure out what we're dealing with.
Test Plan: Ran `bin/harbormaster rebuild-log`, saw an existing log populate. Ran `bin/harbormaster write-log`, saw new log write with proper length information.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19136
Summary:
Depends on D19134. Ref T13088. Future changes will support API writers, so push the log lock into the Log object.
Allow open/close ("this process is writing to this log") to be separate from live/final ("this log is still generating more data").
Test Plan: Wrote logs with `bin/harbormater write-log` and updated logs with `bin/harbormaster rebuild-log`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19135
Summary: Depends on D19133. Ref T13088. Allows build logs to be formally destroyed, cleaning up their chunks and file data.
Test Plan:
- Used `bin/remove destroy` to destroy a log, verified chunks and files were removed.
- Used `bin/harbormaster rebuild-log` to force a log to rebuild, verified files were destroyed and regenerated.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19134
Summary: Depends on D19132. Ref T13088. This implements an extremely skeletal dedicated log page with a more-or-less functional "Download Log" button.
Test Plan: Downloaded a recent log. Tried to download an old (un-finalized) log, couldn't. Used `bin/harbormaster write-log` to get a convenient standalone link to a log.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19133
Summary: Depends on D19131. Ref T13088. During log finalization, stream the log into Files to support "Download Log", archive to Files, and API access.
Test Plan: Ran `write-log` and `rebuild-log`, saw Files objects generate with log content and appropriate permissions.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19132
Summary: Depends on D19130. Ref T13088. Currently, when a build log is closed we compress it in the same process. Separate this out into a dedicated worker since the plan is to do a lot more work during finalization, none of which needs to happen inline during builds (or, particuarly, inline during a Conduit call for API writes in the future).
Test Plan: Ran `bin/harbormaster write-log --trace`, saw compression run inline.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19131
Summary: Ref T13088. This is currently minimal but the modify-execute development loop on build logs is extremely long without it.
Test Plan: Ran `echo hi | ./bin/harbormaster write-log --target 12345`, saw the log show up in the web UI.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19130
Summary:
Ref T13090. The default width changed recently to become much wider, but the behavior on this control isn't great. Instead:
- Pick a default width somewhere between the two.
- Make the width sticky across show/hide (pressing "f" twice remembers your width instead of resetting it).
- Make the width sticky across reloads (dragging the bar, then reloading the page keeps the bar in the same place).
Test Plan:
- Without settings, loaded page: got medium-width bar.
- Dragged bar wide/narrow, toggled on/off with "f", got persistent width.
- Dragged bar wide/narrow, reloaded page, got persistent width.
- Dragged bar wide/narrow, toggled it off, reloaded page, toggled it on, got persistent width.
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19129
Summary: Ref T13090. The doc string in "any()" wasn't specified correctly and the help page wasn't getting enough supporting data to build properly.
Test Plan: Viewed "Reference: Advanced Functions" for a custom datasource field and got more helpful help.
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19128
Summary: Depends on D19126. Ref T13090. For datasource custom fields, this proxies the datasource and provides "none()" and "any()" functions to allow you to search for objects with no values or any values.
Test Plan:
- Created a custom "Owning Group" field in Maniphest using a Projects datasource.
- For a task with no owner assigned, searched for "none()" (hit) and "any()" (miss).
- Assigned the task to an owning project.
- Searched for "none()" (miss), "any()" (hit), the project it is now a member of (hit) and some random other project (miss).
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19127
Summary: Ref T13090. Ref T13077. This adds `parentPaths` and `ancestorPaths` constraints to `phriction.document.query`. These should be a little more usable than the internal `slugPrefix` / `depth` stuff -- that's technically more powerful, but requires callers to know more slug normalization rules. We could perhaps expose `minDepth` / `maxDepth` in the future.
Test Plan: Ran valid and invalid `parentPaths` and `ancestorPaths` queries for `/`, `aaa/`, `AAA/`, etc. Got sensible-seeming results.
Maniphest Tasks: T13090, T13077
Differential Revision: https://secure.phabricator.com/D19125
Summary:
Fixes T13087. Ref T13090. An install ran into a situation where mail was being double-delivered, and it wasn't immediately clear where in the pipeline the issue lay.
This change adds some headers which should rule out (or, at least, render very unlikely) some possible causes if we encounter similar issues in the future.
The `X-Phabricator-Mail-ID` header stores the ID of the `MetaMTAMail` storage object so we can distinguish between two messages sent to two different targets and one message which may have been split or re-sent. It also makes it easier to know what to `bin/mail show-outbound --id <id>` and where to find the message in the web UI for additional information.
The `X-Phabricator-Send-Attempt` is a unique value per attempt. If two mail messages are delivered with the same attempt value, the split is probably downstream from Phabricator. If they have different attempt values, the split is probably in Phabricator.
(In this case, the split was somewhere downstream from us, since sending mail with `/usr/bin/mail` also resulted in duplicates.)
Test Plan: Send some mail, inspected it with `bin/mail show-outbound --id <id>`, saw new headers with sensible/expected values.
Maniphest Tasks: T13090, T13087
Differential Revision: https://secure.phabricator.com/D19124
Summary:
See PHI385. Ref T13054. Ref T13083. The dashboard "arrange" operations (add, remove, move) rely on doing `$dashboard->setThing(...)` and then applying transactions.
This no longer works after the read locking change from T13054. To make this function again, just add an explicit `save()` after layout adjustment. This should be more nuanced eventually, but all arrange operations are nonfunctional in a corrupting way at HEAD of `master`/`stable`, so stop the bleeding first.
Test Plan:
- Created new empty and template dashboards.
- Moved panels.
- Added new and existing panels.
- Removed panels.
Maniphest Tasks: T13083, T13054
Differential Revision: https://secure.phabricator.com/D19123
Summary:
Depends on D19121. Ref T13083. Group transactions and show groups in the debugging view.
Fix some of the most obvious issues with fact generation:
- No more 0-point facts.
- Engine can now generate at least one of every type of fact.
Test Plan: Generated facts, viewed them in the debugging view, fact generation largely appeared to align with reality. No more "no facts in storage" facts.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19122
Summary:
Depends on D19120. Ref T13083. When you write a fact engine, it's currently somewhat difficult to figure out exactly what it's doing. It would also be difficult to diagnose bugs or report them to the upstream.
To ease this, add a page which shows all the facts an object generates. This allows you to iterate on an engine quickly without needing to reanalyze facts, take a screenshot, easily compare the timeline to the fact view, etc.
Test Plan: Viewed the object fact page for several objects.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19121
Summary:
Depends on D19119. Ref T13083. This is probably still very buggy, but I'm planning to build support tools to make debugging facts easier shortly.
This generates a large number of datapoints, at least, and can render some charts which aren't all completely broken in an obvious way.
Test Plan: Ran `bin/fact analyze --all`, got some charts with lines that went up and down in the web UI.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19120
Summary:
Ref T13083. Facts has a fair amount of weird hardcoding and duplication of responsibilities. Reduce this somewhat: no more hard-coded fact aggregates, no more database-driven list of available facts, etc. Generally, derive all objective truth from FactEngines. This is more similar to how most other modern applications work.
For clarity, hopefully: rename "FactSpec" to "Fact". Rename "RawFact" to "Datapoint".
Split the fairly optimistic "RawFact" table into an "IntDatapoint" table with less stuff in it, then dimension tables for the object PHIDs and key names. This is primarily aimed at reducing the row size of each datapoint. At the time I originally wrote this code we hadn't experimented much with storing similar data in multiple tables, but this is now more common and has worked well elsewhere (CustomFields, Edges, Ferret) so I don't anticipate this causing issues. If we need more complex or multidimension/multivalue tables later we can accommodate them. The queries a single table supports (like "all facts of all kinds in some time window") don't make any sense as far as I can tell and could likely be UNION ALL'd anyway.
Remove all the aggregation stuff for now, it's not really clear to me what this should look like.
Test Plan: Ran `bin/fact analyze` and viewed web UI. Nothing exploded too violently.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19119
Summary: See D19117. Instead of automatically figuring this out inside `phutil_tag()`, explicitly add rel="noreferrer" at the application level to all external links.
Test Plan:
- Grepped for `_blank`, `isValidRemoteURIForLink`, checked all callsites for user-controlled data.
- Created a link menu item, verified noreferrer in markup.
- Created a link custom field, verified no referrer in markup.
- Verified noreferrer for `{nav href=...}`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19118
Summary: Ref T13077. The context object wasn't being passed into the engine properly here, affecting relative link rendering in Phriction.
Test Plan: Viewed rendered Phriction documents with relative links, got clean renders.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19115
Summary: Ref T13077. This content extraction rule wasn't right and caused rendering on Phriction pages to extract context improperly.
Test Plan: Viewed pages in Phriction with relative links to other documents.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19114
Summary: Ref T13077. Freeze "phriction.info" in favor of the more modern "phriction.document.search".
Test Plan: Reviewed older method in web UI, saw frozen markers.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19113
Summary: Ref T13077. Adds a "paths" constraint to the API query.
Test Plan: Used paths constraint to fetch documents.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19112
Summary: Depends on D19107. Ref T13077. The underlying datasource may need some adjustment but this appears to work properly locally.
Test Plan: Typed `[[ por` locally, was suggested "Porcupine Facts". Typed `[[ / ]]`, saw it render as a reference to the wiki root instead of the install root.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19108
Summary:
Depends on D19106. Fixes T5941. Ref T13077. Allows you to find Phriction documents as suggestions from global quick search.
Also supports `w` to jump to Phriction and `w query` to query Phriction.
The actual query logic for the datasource may need some tweaking after it collides with reality, but seems to produce fairly reasonable results in local testing against synthetic data.
Test Plan: Searched for "Porcupine Facts", "Travel Companions", and other useful local pages. Searched for `w`. Searched for `w zebra facts`.
Maniphest Tasks: T13077, T5941
Differential Revision: https://secure.phabricator.com/D19107
Summary:
Depends on D19105. Ref T13077. Fixes T12344.
The `[[ ... ]]` syntax accepts and handles characters which would require URL encoding if they appeared in URIs. For example, `[[ 100% Natural Cheese Dust ]]` is a legitimate, supported piece of remarkup syntax, and does not need to be written as `... 100%25 Natural ...`.
Likewise, `[[ BUY $DOGE ]]` is legitimate and does not need to be written as `[[ BUY %24DOGE ]]`. This piece of syntax creates a link to `/w/buy_$doge/`. This may or may not appear in your browser's URL bar as `/w/buy_%24doge/`, but internally "$" is a valid slug character and you'll see `buy_$doge` over Conduit, etc.
However, since users may reasonably copy paths from their browser URL bar, they may have unnecessary URL encoding. The syntax `[[ buy_$doge ]]` is legitimate, but a user copy/pasting may write `[[ buy_%24doge ]]` instead.
Currently, this extra URL encoding causes links to break, since `[[ buy_%24doge ]]` is a treated as link to `/w/buy_24doge/`, just like `[[ Fresh 100%AB Blood ]]` is a link to `/w/fresh_100_ab_blood/`.
To fix this:
- When the target for a link can be URL decoded, try to do lookups on both the un-decoded and decoded variations.
- If the un-decoded variation works, great: use it. This preserves behavior for all existing, working links.
- If the un-decoded variation fails but the decoded variation works, okay: we'll assume you copy-pasted a URL-encoded version and strip URL encoding.
- If both fail, same behavior as before.
Also, use a different spelling for "existent".
See T13084 for some "attacks" based on this behavior. I think the usability affordance this behavior provides greatly outweighs the very mild threat those attacks represent.
Test Plan:
- Created links to existing, nonexisting, and existing-but-not-visible documents, all of which worked normally.
- Created links to `[[ $doge ]]` and `[[ %24doge ]]`, saw them both go to the right place.
- Performed the "attacks" in T13084.
Maniphest Tasks: T13077, T12344
Differential Revision: https://secure.phabricator.com/D19106
Summary:
Fixes T10969. Ref T13077. When you create a Phriction document with a relative link (`[[ ./path/to/page ]]`) the initial email currently points to the wrong place.
This is because the context object (the page) isn't passed to the markup engine. Without this context, the relative link is rendered as though it appeared somewhere else (like a task or revision) where relative links don't make sense.
Test Plan: Created a new Phriction document with a relative link to `[[ ./porcupine_facts/starmap ]]`, saw a usable link in the resulting email.
Maniphest Tasks: T13077, T10969
Differential Revision: https://secure.phabricator.com/D19105
Summary:
See PHI370. Support the "Affected packages" and "Affected package owners" Herald fields in pre-commit hooks.
I believe there's no technical reason these fields aren't supported and this was just overlooked.
Test Plan: Wrote a rule which makes use of the new fields, pushed commits through it. Checked transcripts and saw sensible-looking values.
Differential Revision: https://secure.phabricator.com/D19104
Summary: Depends on D19100. Ref T13077. Adds a "content" attachment to get the actual page text. This works on both "phriction.document.search" and "phriction.content.search".
Test Plan: Called both API methods with the attachment, saw proper text content returned.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19103
Summary:
Depends on D19099. Ref T13077. Updates Phriction documents to string constants to make API interactions cleaner and statuses more practical to extend.
This does not seem to require any transaction migrations because none of the Phriction transactions actually store status values: status is always a side effect of other edits.
Test Plan: Created, edited, deleted, moved documents. Saw appropriate UI cues. Browsed and filtered documents by status in the index.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19100
Summary:
Depends on D19098. Ref T13077.
Phriction status constants currently use the "bag of statuses" approach typical of older code, and store integers in the database.
This fixes the "bag of statuses" stuff; a future change will fix the integers.
Also adds a skeleton for `phriction.document.search`, but doesn't implement the Conduit interface yet.
Test Plan: Searched for documents with various status constraints. Grepped for removed status constants. Viewed document list.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19099
Summary: Depends on D19097. Ref T13077. Freeze the older method now that the newer one is available.
Test Plan: Viewed the older method's page and saw it frozen; called it to make sure I didn't break it by accident.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19098
Summary: Depends on D19096. Ref T13077. Adds a new "v3" API method for Phriction document content, to replace the existing "phriction.history" call.
Test Plan: Made various calls via web API console.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19097
Summary:
Depends on D19095. Ref T6203. Ref T13077. This column is nullable in an inconsistent way. Make it non-nullable.
Also clean up one more content query on the history view.
Test Plan: Ran migration, then created and edited documents without providing a descriptino or hitting `NULL` exceptions.
Maniphest Tasks: T13077, T6203
Differential Revision: https://secure.phabricator.com/D19096
Summary: Depends on D19094. Ref T13077. Use modern infrastructure to perform these loads. I left a couple of calls in the older API methods unconverted.
Test Plan: Viewed documents. Viewed older versions. Viewed diffs. Did revert edits to older versions.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19095
Summary:
Depends on D19093. Ref T13077. Although content objects normally don't have any edges today, they may in the future.
Also implement Policy stuff properly.
Test Plan: Used `bin/remove destroy` to destroy a document, verified it also loaded and destroyed the correspoding Content correctly by looking at `--trace` and the database rows.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19094
Summary:
Depends on D19092. Ref T13077. This modernizes markup rendering for PhrictionContent.
This is a little messy because table of contents generation isn't straightforward.
Test Plan: Viewed Phriction documents with and without 3+ headers, saw ToC vs no ToC. Edited/previewed documents. Grepped for affected symbols. Checked DarkConsole for sensible cache behavior.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19093
Summary: Ref T13077. Prepares for modern API access to document history using standard "v3" APIs.
Test Plan: Ran migration, verified PHIDs appeared in the database. Created/edited a document, got even more PHIDs in the database.
Maniphest Tasks: T13077
Differential Revision: https://secure.phabricator.com/D19092
Summary: Ref T13073. The new log output from `bin/drydock lease` currently uses HTML handle rendering, but should render to text.
Test Plan: Ran `bin/drydock lease` and saw normal text in log output. Viewed the same logs from the web UI and saw HTML.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19101
Summary: Depends on D19089. Fixes T13079. This is likely not the final form of this, but creates a defensible extension point.
Test Plan: See T13079 for discussion.
Maniphest Tasks: T13079
Differential Revision: https://secure.phabricator.com/D19090
Summary:
Depends on D19088. Ref T13079.
> Any sufficiently complicated C or Fortran program contains an ad hoc, informally-specified, bug-ridden, slow implementation of half of Common Lisp.
> - Greenspun's Tenth Rule
Move us a step closer to this noble goal.
This doesn't implement any `viewer(project())` stuff but it looks like the API doesn't need to change to do that in the future.
Test Plan: Grimmaced in pain.
Maniphest Tasks: T13079
Differential Revision: https://secure.phabricator.com/D19089
Summary: Depends on D19087. Ref T13079. This still doesn't feel like the most clean, general system in the world, but is a step forward from hard-coded `switch()` stuff.
Test Plan:
- Jumped to `r`.
- Jumped to `a`.
- Jumped to `r poe` (multiple results).
- Jumped to `r poetry` (one result).
- Jumped to `r syzygy` (no results).
- Jumped to `p`.
- Jumped to `p robot` (multiple results); `p assessment` (one result).
- The behavior for `p <string>` has changed slightly but should be more powerful now (it's consistent with `r <string>`).
- Jumped to `s <symbol>` and `s <context>-><symbol>`.
- Jumped to `d`.
- Jumped to `f`.
- Jumped to `t`.
- Jumped to `T123`, `D123`, `@dog`, `PHID-DREV-abcd`, etc.
Maniphest Tasks: T13079
Differential Revision: https://secure.phabricator.com/D19088
Summary: Ref T13079. This recently-introduced Engine/EngineExtension are a good fit for adding more datasource functions in general, but we didn't think quite big enough in naming them.
Test Plan: Used quick search typeahead, hit applications/users/monograms/symbols/etc.
Maniphest Tasks: T13079
Differential Revision: https://secure.phabricator.com/D19087
Summary:
Depends on D19084. Fixes T13078. When `phabricator.silent` is enabled, immediately fail the "HTTP Request", "CircleCI" and "Buildkite" build steps.
This doesn't feel quite as clean as most of the other behavior of `phabricator.silent`, since these calls are not exactly notifications in the same way that email is, and failing to make these calls means that builds run differently (whereas failing to deliver email doesn't really do anything).
However, I suspect that this behavior is almost always reasonable/correct, and that we can probably get away with it until this grey area between "notifications" and "external service calls" is more clearly defined.
Test Plan:
- Created a build with HTTP, CircleCI, and Buildkite steps.
- Put install in `phabricator.silent` mode: all three steps failed with "declining, because silent" messages.
- Put install back in normal mode: all three steps made HTTP requests.
- Read updated documentation.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13078
Differential Revision: https://secure.phabricator.com/D19085
Summary: Ref T13078. The `phabricator.silent` configuration flag should disable webhook calls, since this is consistent with the documented and desired behavior.
Test Plan: Enabled `phabricator.silent`, made test hook calls, saw them fail with a "silent" failure reason.
Maniphest Tasks: T13078
Differential Revision: https://secure.phabricator.com/D19084
Summary: Revisions with blocking reviewers had this stamp built incorrectly, which cascaded into trying to use `array()` as a PHID. Recover so these tasks succeed.
Test Plan: Will deploy production.
Differential Revision: https://secure.phabricator.com/D19082
Summary:
Depends on D19078. Ref T13073. Currently, there is a narrow window where we can acquire a resource after a reclaim has started against it.
To prevent this, briefly lock resources before acquiring them and make sure they're still good. If a resource isn't good, throw the lease back in the pool.
Test Plan:
This is tricky. You need:
- Hoax blueprint with limits and a rule where leases of a given "flavor" can only be satisfied by resources of the same flavor.
- Reduce the 3-minute "wait before resources can be released" to 3 seconds.
- Limit Hoaxes to 1.
- Allocate one "cherry" flavored Hoax and release the lease.
- Add a `sleep(15)` to `releaseResource()` in `DrydockResourceUpdateWorker`, after the `canReclaimResource()` check, with a `print`.
Now:
- Run `bin/phd debug task` in two windows.
- Run `bin/drydock lease --type host --attributes flavor=banana` in a third window.
- This will start to reclaim the existing "cherry" resource. Once one of the `phd` windows prints the "RECLAIMING" message run `bin/drydock lease --type host --attributes flavor=cherry` in a fourth window.
- Before patch: the "cherry" lease acquired immediately, then was released and destroyed moments later.
- After patch: the "cherry" lease yields.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19080
Summary:
Depends on D19077. Ref T13073. When we're using slot locks to enforce a limit (e.g., maximum of 5 simultaneous things) we currently load locks owned by the blueprint to identify which slots are likely to be free.
However, this isn't right: the blueprint doesn't own these locks. The resources do.
We still get the right behavior eventually, but we incorrectly identify that every slot lock is always free, so as the slots fill up we'll tend to guess wrong more and more often.
Instead, load the slot locks by name explicitly.
Test Plan: Implemented lock-based limiting on `HoaxBlueprint`, `var_dump()`'d the candidate locks, saw correct test state for locks. Acquired leases without releasing, got all of the slots filled without any slot lock collisions (previously, the last slot or two tended to collide a lot).
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19078
Summary:
Depends on D19076. Ref T13073. Blueprints are stored as an attribute and `setAttributes()` overwrites all attributes.
This is sorta junk but make it less obviously broken, at least.
Test Plan: Ran `bin/drydock lease --type working-copy --attributes x=y` without instantly getting a fatal about "no blueprint PHIDs".
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19077
Summary:
Depends on D19075. Ref T13073. If a lease acquires a resource but finds that the resource builds directly into a dead state (which can happen for any number of reasonable reasons), reset the lease and throw it back in the pool.
This isn't the lease's fault and it hasn't caused any side effects or done anything we can't undo, so we can safely reset it and throw it back in the pool.
Test Plan:
- Created a blueprint which throws from `allocateResource()` so that resources never activate.
- Tried to lease it with `bin/drydock lease ...`.
- Before patch: lease was broken and destroyed after a failed activation.
- After patch: lease was returned to the pool after a failed activation.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19076
Summary: Ref T13073. Depends on D19074. Update icons and UI for resource status.
Test Plan: Viewed resources in detail view and list view, saw better status icons.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19075
Summary:
Depends on D19073. Ref T13073. Give leases a normal header tag and try to wrangle their status constants a bit.
Also, try to capture the "status class" pattern a bit. Since we target PHP 5.2.3 we can't use `static::` so the actual subclass is kind of a mess. Not exactly sure if I want to stick with this or not. We could consider targeting PHP 5.3.0 instead to get `static::` / late static binding.
Test Plan: Viewed leases and lease lists, saw better and more conventional status information.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19074
Summary:
Depends on D19072. Ref T13073. Currently, you can leave leases stranded by using `^C` to interrupt the script. Handle signals and release leases on destruction if they haven't activated yet.
Also, print out more useful information before and after activation.
Test Plan: Mashed ^C while runnning `bin/drydock lease ... --trace`, saw the lease release.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19073
Summary: Depends on D19071. Ref T13073. While the daemons are supposedly doing things, show the user any logs they generate. There's often something relevant but unearthing it can be involved.
Test Plan: {F5427773}
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19072
Summary: Depends on D19070. Ref T13073. Some messages contain an interesting story or a clever anecdote. Respect newlines during rendering to preserve authorial intent.
Test Plan:
Viewed a message with linebreaks and could still read it.
{F5427754}
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19071
Summary:
Ref T13073. When a Blueprint says it will be able to allocate a resource but then throws an exception while attempting that allocation, we currently fail the lease permanently.
This is excessively harsh. This blueprint may have the best of intentions and have encountered a legitimately unforseeable failure (like a `vm.new` call to build a VM failed) and be able to succeed in the future.
Even if this blueprint is a dirty liar, other blueprints (or existing resources) may be able to satisfy the lease in the future.
Even if every blueprint is implemented incorrectly, leaving the lease alive lets it converge to success after the blueprints are fixed.
Instead of failing, log the issue and yield.
(In the future, it might make sense to distinguish more narrowly between "actually, all the resources are used up" and all other failure types, since the former is likely more routine and less concerning.)
Test Plan:
- Wrote a broken `Hoax` blueprint which always claims it can allocate but never actually allocates (just `throw` in `allocateResource()`).
- Used `bin/phd drydock lease` to acquire a Hoax lease.
- Before patch: lease abruptly failed permanently.
- After patch: lease yields after allocation fails.
{F5427747}
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13073
Differential Revision: https://secure.phabricator.com/D19070
Summary: Like "Commit Hook" rules, these also fire oddly and don't have an object PHID or a list of transactions.
Test Plan: Verified that "Call Webhooks" was no longer available from Diff rules, but still available from other rule types.
Differential Revision: https://secure.phabricator.com/D19069
Summary:
See PHI346. Ref T13054. If you have prototypes enabled on the server but use `master` / `stable` on the client and run `arc diff --plan-changes`, the transition is rejected because "Draft -> Changes Planned" isn't currently a legal transition.
Allow this transition if not coming from the web UI (to keep it out of the dropdown).
Test Plan:
- Ran `arc diff --plan-changes` on `master`, got a "Changes Planned" revision instead of a validation error.
- Ran `arc diff` without `--plan-changes`, got a draft, verified that "Plan Changes" still doesn't appear in the action dropdown.
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19067
Summary:
Depends on D19065. Ref T13054. Instead of just updating `containerPHID` and hoping for the best, queue a proper BuildWorker to process a "your container has changed, update it" message.
We also need to remove a (superfluous) `withContainerPHIDs()` when loading active diffs for a revision.
Test Plan:
- Without daemons, created a revision and saw builds stick in "preparing" with no container PHID, but also stay in draft mode.
- With daemons, saw builds actually build and get the right container PHID.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19066
Summary:
Depends on D19064. Ref T13054. See that task for additional discussion.
When buildables are created by `arc` and have lint/unit messages, they can currently pass or fail before Herald triggers actual builds. This puts them in a pre-build state where they can't complete until Herald says it's okay.
On its own, this change intentionally strands `arc diff --only` diffs in the "PREPARING" stage forever.
Test Plan:
- Ran a build with `bin/harbormaster`, saw it build normally.
- Ran a build with web UI, saw it build normally.
- Ran a build with `arc diff`, saw it build normally.
- Ran a build with `arc diff --only`, saw it hang in "PREPARING" forever.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19065
Summary: Depends on D19063. Ref T13054. Prepare for the addition of a new `PREPARING` status by getting rid of the "scattered mess of switch statements" pattern of status management.
Test Plan: Searched/browsed buildables. Viewed buildables. Viewed revisions. Grepped for all affected symbols.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19064
Summary: Ref T13054. Companion storage change for D19062.
Test Plan: Applied migration and adjustments. Viewed messages in Harbormaster; created them with `harbormaster.sendmessage`; processed them with `bin/phd debug task`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19063
Summary:
See T13054. This prepares for Buildables to be sent messages ("attach", "done scheduling builds") to fix races between Harbormaster and Differential.
The `buildTargetPHID` is replaced with a `recipientPHID` in the API. An additional change will fix the storage.
In the future, this table could probably also replace `HarbormasterBuildCommand` now, which is approximately the same bus, but for Builds.
Test Plan: Viewed builds with messages. Sent messages with `harbormaster.sendmessage`. Processed messages with `bin/phd debug task`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19062
Summary: Fixes T13071. See that task for discusison. I think this `<= version` constraint is needless in normal cases (it should match everything in the table anyway), and slightly harmful in bizarre cases where a draft somehow gets a much larger ID than it should have.
Test Plan:
- Gave a draft an unreasonably large ID.
- Pre-patch, observed: submitting comments on the draft's object does not clear the draft.
- Post-patch: submitting comments on the draft's object now clears the draft correctly.
- Also added comments/actions, reloaded pages, saw drafts stick properly.
Maniphest Tasks: T13071
Differential Revision: https://secure.phabricator.com/D19060
Summary: Ref T13054. Fixes T12714. Applies read locks to all transactions instead of only a very select subset (chat messages in Conpherence).
Test Plan: See <T13054#235650> for discussion and testing.
Maniphest Tasks: T13054, T12714
Differential Revision: https://secure.phabricator.com/D19059
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:
Ref T13054. Fixes T10746. Fixes T11154. This is really a one-line fix (include `ABORTED` in `BuildEngine->updateBuildable()`) but try to structure the code a little more clearly too and reduce (at least slightly) the number of random lists of status attributes spread throughout the codebase.
Also add a header tag for buildable status.
Test Plan: Aborted a build, saw buildable fail properly.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054, T11154, T10746
Differential Revision: https://secure.phabricator.com/D19055
Summary: See PHI358. The `bin/almanac [un]trust-key` workflows don't properly purge the SSH key cache, but should.
Test Plan:
- Added key `ssh-rsa xyz` to a device.
- Used `bin/ssh-auth | grep xyz` to test for the presence of the key.
- Before patch: Saw it not present, trusted it, saw it still not present.
- After patch: Saw it not present, trusted it, saw it now present. Untrusted it, saw it no longer present.
Differential Revision: https://secure.phabricator.com/D19053
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 D19049. Ref T11330. Adds some documentation for webhooks.
Test Plan: Read the documentation and found it to be exceptionally accurate and helpful.
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19050
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 D19047. Ref T11330. Triggers every firehose hook on every edit; prepares for Herald triggers.
Test Plan: Configured a firehose hook, edited some objects, saw callbacks.
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19048
Summary: Depends on D19046. Ref T11330. Supports querying for specific transactions while responding to webhooks.
Test Plan: Called `transaction.search` with and without PHID constraints.
Maniphest Tasks: T11330
Differential Revision: https://secure.phabricator.com/D19047
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: I made the red stronger (always visible, not just a hover state) for the "Mute" feature, but this made Logout look a little intense. Just make it normal-colored, logging out isn't a big deal.
Test Plan: No longer saw bright red logout action in profile dropdown menu.
Differential Revision: https://secure.phabricator.com/D19044
Summary: Ref T12677. Skip these checks if we're doing the new stuff. Also, allow priority to be unspecified.
Test Plan: Will deploy.
Maniphest Tasks: T12677
Differential Revision: https://secure.phabricator.com/D19043
Summary: See PHI356. Adds inline comment and done counts to the filetree. Also makes the filetree wider by default.
Test Plan: Fiddled with filetrees in different browsers on different revisions. Added inlines, marked them done/undone.
Differential Revision: https://secure.phabricator.com/D19041
Summary: See PHI356. Makes it easier to pick out change types in the filetree view in Differential.
Test Plan: Created a diff with adds, copies, moves, deletions, and binary files. Viewed in Differential, had an easier time picking stuff out.
Differential Revision: https://secure.phabricator.com/D19040
Summary: Depends on D19038. Fixes T4434. Updates the SearchEngine and Query to handle these fields.
Test Plan: Filtered and ordered by date and closer.
Maniphest Tasks: T4434
Differential Revision: https://secure.phabricator.com/D19039
Summary:
Depends on D19037. Ref T4434. Adds closed date to `maniphest.search` and "Export Data".
When a task has been closed, show the closed date with a checkmark in the UI instead of the modified date.
Test Plan:
- Exported data to CSV, saw close information.
- Saw close information in `/maniphest/`.
- Queried for close information via `maniphest.search`.
Maniphest Tasks: T4434
Differential Revision: https://secure.phabricator.com/D19038
Summary:
Ref T4434. Although some of the use cases for this data are better fits for Facts, this data is reasonable to track separately.
I have an approximate view of it already ("closed, ordered by date modified") that's useful to review things that were fixed recently. This lets us make that view more effective.
This just adds (and populates) the storage. Followups will add Conduit, Export, Search, and UI support.
This is slightly tricky because merges work oddly (see T13020).
Test Plan:
- Ran migration, checked database for sensible results.
- Created a task in open/closed status, got the right database values.
- Modified a task to close/open it, got the right values.
- Merged an open task, got updates.
Maniphest Tasks: T4434
Differential Revision: https://secure.phabricator.com/D19037
Summary:
See D18176. This query has no effect (other than wasting resources) and the result is unused.
`$repository` already has the URI loaded because we load them unconditionally during request initialization.
Test Plan: Viewed repository URIs.
Subscribers: jmeador
Differential Revision: https://secure.phabricator.com/D19036
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: Fixes T10189. Ref T13053. We haven't sent these headers in a very long time. Briefly mention the new stamps header instead, although I expect to integrate stamp documentation into the UI in a more cohesive way in the future.
Test Plan: Read documentation.
Maniphest Tasks: T13053, T10189
Differential Revision: https://secure.phabricator.com/D19030
Summary: Depends on D19028. Ref T13053. Fixes T6576. An HTML body was built here, but not passed to the actual mail message.
Test Plan: Will verify production push mail.
Maniphest Tasks: T13053, T6576
Differential Revision: https://secure.phabricator.com/D19029
Summary: Ref T13053. Some mail (like push notification mail) doesn't currently generate any stamps. Drop this section if there aren't any stamps on the mail.
Test Plan: Will check push mail in production.
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19028
Summary: Ref T13053. Uses the changes in D19026 to escape mail addresses. Those rules may not be right yet, but they're at least all in one place, have test coverage, and aren't obviously incorrect.
Test Plan: Will vet this more extensively when re-testing all mailers.
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19027
Summary:
Ref T13053. Postmark support recommends testing requests against a whitelist of known remote addresses to determine request authenticity. Today, the list can be found here:
<https://postmarkapp.com/support/article/800-ips-for-firewalls>
This is potentially less robust than, e.g., HMAC verification, since they may need to add new datacenters or support IPv6 or something. Users might also have weird network topologies where everything is proxied, and this makes testing/simulating more difficult.
Allow users to configure the list so that they don't need to hack things apart if Postmark adds a new datacenter or remote addresses are unreliable for some other reason, but ship with safe defaults for today.
Test Plan:
Tried to make local requests, got kicked out. Added `0.0.0.0/0` to the list, stopped getting kicked out.
I don't have a convenient way to route real Postmark traffic to my development laptop with an authentic remote address so I haven't verified that the published remote address is legitimate, but I'll vet that in production when I go through all the other mailers.
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19025
Summary:
See support email. There's nothing tricky here, we were just missing a check. The different parts of this got built at different times so I think this was simply overlooked.
Also add a redundant check just to future-proof this and be on the safe side.
Test Plan: Used `bin/phortune invoice` to charge a pact subscription. After deleting the card, the charge failed with an appropriate error.
Reviewers: amckinley
Differential Revision: https://secure.phabricator.com/D19020
Summary:
Depends on D19021. Ref T13053. When you "Subscribe", or make some other types of edits, we don't necessarily have reviewer data, but may now need it to do the new recipient list logic.
I don't have a totally clean way to deal with this in the general case in mind, but just load it for now so that things don't fatal.
Test Plan: Subscribed to a revision with the "Subscribe" action.
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19022
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:
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: 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
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 D18957. Ref T13049. To do Excel exports, PHPExcel needs to be installed on the system somewhere.
This library is enormous (1K files, ~100K SLOC), which is why we don't just include it in `externals/`. This install process is a little weird and we could improve it, but users don't seem to have too much difficulty with it. This shouldn't be worse than the existing workflow in Maniphest, and I tried to make it at least slightly more clear.
Test Plan: Uninstalled PHPExcel, got it marked "Unavailable" and got reasonably-helpful-ish guidance on how to get it to work. Reinstalled, exported, got a sheet.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18958
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 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 D18952. Ref T13049. For files larger than 8MB, we need to engage the chunk storage engine. `PhabricatorFile::newFromFileData()` always writes a single chunk, and can't handle files larger than the mandatory chunk threshold (8MB).
Use `IteratorUploadSource`, which can, and "stream" the data into it. This should raise the limit from 8MB to 2GB (maximum size of a string in PHP).
If we need to go above 2GB we could stream CSV and text pretty easily, and JSON without too much trouble, but Excel might be trickier. Hopefully no one is trying to export 2GB+ datafiles, though.
Test Plan:
- Changed the JSON exporter to just export 8MB of the letter "q": `return str_repeat('q', 1024 * 1024 * 9);`.
- Before change: fatal, "no storage engine can store this file".
- After change: export works cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18953
Summary:
Depends on D18951. Ref T13049. When we export to CSV or plain text, add a header row in the first line of the file to explain what each column means. This often isn't obvious with PHIDs, etc.
JSON has keys and is essentially self-labeling, so don't do anything special.
Test Plan: Exported CSV and text, saw new headers. Exported JSON, no changes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18952
Summary:
Ref T13049. All exportable objects should always have these fields, so make them builtins.
This also sets things up for extensions (like custom fields).
Test Plan: Exported user data, got the same export as before.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18951
Summary: See PHI276. Ref T13048. The fix in D18933 got one callsite, but missed the one in the `callConduit()` method, so the issue isn't fully fixed in production. Convert this adapter to use a real viewer (if one is available) more thoroughly.
Test Plan: Ran rules in test console, saw field values. Will test in production again.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18950
Summary: Depends on D18946. Ref T13051. Begins writing edge transactions as just a list of changed PHIDs.
Test Plan: Added, edited, and removed projects. Reviewed transaction record and database. Saw no user-facing changes but a far more compact database representation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13051
Differential Revision: https://secure.phabricator.com/D18947
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: Ref T13050. Oh boy. Both of them run `grep`!
Test Plan: Will push again.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13050
Differential Revision: https://secure.phabricator.com/D18945
Summary:
Ref T13048. Fixes T11112. I mostly fixed this before in D18887, but missed that these other actions are also affected. T11112 had a more complete list of missing limits.
(It's possible there are some others somewhere, but this is everything we know about, I think.)
Test Plan: Created rules using these actions, typed stuff into the box, was only able to enter one value.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048, T11112
Differential Revision: https://secure.phabricator.com/D18943
A recent version of Git has changed some piece of behavior here and we
now get "fatal: ssh variant 'simple' does not support setting port"
when using a port. Explicitly setting GIT_SSH_VARIANT to `ssh` likely
fixes this.
Summary:
See PHI316. Maniphest and other applications currently have controls like `Created After: [_____]` where you just get an empty text field.
Although most formats work -- including relative formats like "3 days ago" -- and we validate inputs so you get an error if you enter something nonsensical, this still isn't very user friendly.
T8060 or some other approach is likely the long term of this control.
In the meantime, add placeholder text to suggest that `YYYY-MM-DD` or `X days ago` will work.
Test Plan: Viewed date inputs, saw placeholder text.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18942
Summary:
See PHI307. Currently, when reviews undraft, we retroactively add in older activity to the mail ("alice created this revision...").
However, we don't add that activity to the mail tags, so the relevant tags (like "revision created") are dropped forever.
Instead, use the same set of transactions for both mail body and mail tag construction.
This should be obsoleted in the relatively near future by T10448, but it's a better/more correct behavior in general and we probably can't get rid of tags completely for a while.
Test Plan:
Applied patch, created a revision with builds, saw it auto-undraft after builds finished. Used `bin/mail list-outbound` and `bin/mail show-outbound` to see the mail. Verified that it included retroactive text ("created this revision") AND retroactive tags.
Note that the tag for "A new revision is created" is `DifferentialTransaction::MAILTAG_REVIEW_REQUEST` with literal value `differential-review-request`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18941
Summary:
Depends on D18939. Ref T13047. Symbol lookup can be activated from a diff (in Differential or Diffusion) or from the static view of a file at a particular commit.
In the latter case, we need to figure out the path a little differently. The character and line number approaches still work as written.
Test Plan:
- Command-clicked symbols in the Diffusion browse view with blame on and off; saw path, line and char populate properly.
- Command-clicked symbols in Differential diff view to check I didn't break anything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18940
Summary: Depends on D18937. Ref T13047. When available, provide character positions so external indexers can return more accurate results.
Test Plan: Clicked symbols in Safari, Firefox and Chrome, got sensible-looking character positions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18939
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 D18936. Ref T13047. Third parties can define external symbol sources that let users jump to PHP or Python documentation or query some server.
Give these queries more information so they can try to get better results: the path and line where the symbol appeared, and any known repository scope.
Test Plan: Wrote a fake external source that used this data, command-clicked a symbol in Differential, saw a fake external symbol result.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18937
Summary:
Ref T13047. In some reasonable cases, knowing the path and line number where a symbol appears is useful in ranking or filtering the set of matching symbols.
Giving symbol sources more information can't hurt, and it's generally free for us to include this context since we just need to grab it out of the document and pass it along.
We can't always get this data (for example, if a user types `s idx` into global search, we have no clue) but this is similar to other types of context which are only available sometimes (like which repository a symbol appears in).
Test Plan: Command-clicked some symbols in 1-up (unified) and 2-up (side-by-side) diff views with symbol indexes configured. Got accurate path and line information in the URI I was redirected to.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18936
Summary:
Depends on D18934. Ref T13046. Add support for the new export flow to a second application.
My goal here is mostly just to make sure that this is general enough to work in more than one place, and exporting user accounts seems plausible as a useful feature, although we do see occasional requests for this feature exactly (like <https://discourse.phabricator-community.org/t/users-export-to-csv/968>).
The exported data may not truly be useful for much (no disabled/admin/verified/MFA flags, no external account data, no email addresses for policy reasons) but we can expand it as use cases arise.
Test Plan: Exported user accounts in several formats.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18935
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 D18932. Ref T13048. See PHI276. In the cluster, we don't have device keys on `web` nodes. This is generally good, since they don't need them, and it means that we aren't putting more credentials than we need on those hosts.
However, it means that when we pull diff content to test "Commit" rules via the Herald test console, we use the omnipotent user and try to use device credentials, and this fails since we don't have any.
Instead, pass the real viewer in this case so we just sign the request as them, like we do for normal Diffusion requests.
Test Plan:
Wrote and ran a commit content rule locally, no issues.
This isn't completely convincing since my local setup does have device credentials, but I'll double-check in production once this deploys.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18933
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 D18927. Ref T13048. This implements a new policy which allows Herald rules to fire on some kinds of state changes.
Test Plan:
Wrote and tested rules with the new policy:
{F5394971}
{F5394972}
Also wrote and tested rules with the old policies:
{F5394973}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18930
Summary:
Depends on D18926. Ref T6203. Ref T13048. Herald rule repetition policies are stored as integers but treated as strings in most contexts.
After D18926, the integer stuff is almost totally hidden inside `HeraldRule` and getting rid of it completely isn't too tricky.
Do so now.
Test Plan:
- Created "only the first time" and "every time" rules. Did a SELECT on their rows in the database.
- Ran migrations, got a clean bill of health from `storage adjust`.
- Did another SELECT on the rows, saw a faithful conversion to strings "every" and "first".
- Edited and reviewed rules, swapping them between "every" and "first".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048, T6203
Differential Revision: https://secure.phabricator.com/D18927
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 D18924. Ref T13048. Each adapter defines which repetition options ("every time", "only the first time") users may select for rules.
Currently, this is all explicit and hard-coded. However, every adapter really just implements this rule (except for some bugs, see below):
> You can pick "only the first time" if this adapter fires more than once on the same object.
Since we already have a `isSingleEventAdapter()` method which lets us tell if an adapter fires more than once, just write this rule in the base class and delete all the copy/pasting.
This also fixes two bugs because of the copy/pasting: Pholio Mocks and Phriction Documents did not allow you to write "only the first time" rules. There's no reason for this, they just didn't copy/paste enough methods when they were implemented.
This will make a future diff (which introduces an "if the rule did not match last time" policy) cleaner.
Test Plan:
- Checked several different types of rules, saw appropriate options in the dropdown (pre-commit: no options; tasks: first or every).
- Checked mocks and wiki docs, saw that you can now write "only the first time" rules.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18925
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:
Ref T13043. In an earlier change I updated this langauge from "Deactivate" to "Revoke", but the behavior doesn't quite match.
This table has a unique key on `<isActive, keyBody>`, which enforces the rule that "a key can only be active for one unique user".
However, we set `isActive` to `null` when we revoke a key, and multiple rows are allowed to have the value `<null, "asdf">` (since a `null` column in a unique key basically means "don't enforce this unique key").
This is intentional, to support this workflow:
- You add key X to bot A.
- Whoops, wrong account.
- You revoke key X from bot A.
- You add key X to bot B.
This isn't necessarily a great workflow -- ideally, you'd throw key X away and go generate a new key after you realize you made a mistake -- but it's the sort of practical workflow that users are likely to expect and want to see work ("I don't want to generate a new key, it's already being used by 5 other services and cycling it is a ton of work and this is just a test install for my dog anyway."), and there's no technical reason we can't support it.
To prevent users from adding keys on the revocation list back to their account, just check explicitly.
(This is probably better in general anyway, because "cert-authority" support from PHI269 may mean that two keys are "equivalent" even if their text differs, and we may not be able to rely on a database test anyway.)
Test Plan:
- Added the key `ssh-rsa asdf` to my account.
- Revoked it.
- Tried to add it again.
- Before patch: worked.
- After patch: error, "this key has been revoked".
- Added it to a different account (the "I put it on the wrong bot" workflow).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18928
Summary:
Ref T13025. See <https://discourse.phabricator-community.org/t/bulk-edit-no-actions-available/1011/1>.
I'm not sure if this is what the user is seeing, but in Chrome, the `<select />` does not automatically get set to the first valid value like it does in Safari.
Set it to the first valid value explicitly.
Test Plan: In Chrome, bulk editor previously hit a JS error when trying to read a bad action off the `<select />`. After patch, bulk edits go through cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025
Differential Revision: https://secure.phabricator.com/D18923
Summary:
Fixes T5965.
Fixes two issues:
- Observing an empty repository could write a warning to the log.
- Mirroring an empty repository to a remote could fail.
For observing:
If newly-created with `git init --bare`, `git ls-remote` will
return the empty string. Properly return an empty set of refs, rather
than attempting to parse the single "line" that is produced by
splitting that on newlines:
```
[2018-01-23 18:47:00] ERROR 8: Undefined offset: 1 at [/phab_path/phabricator/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:405]
arcanist(head=master, ref.master=5634f8410176), phabricator(head=master, ref.master=12551a1055ce), phutil(head=master, ref.master=4755785517cf)
#0 PhabricatorRepositoryPullEngine::loadGitRemoteRefs(PhabricatorRepository) called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:343]
#1 PhabricatorRepositoryPullEngine::executeGitUpdate() called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:126]
#2 PhabricatorRepositoryPullEngine::pullRepositoryWithLock() called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:40]
#3 PhabricatorRepositoryPullEngine::pullRepository() called at [<phabricator>/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php:59]
...
```
For mirroring:
`git` treats `git push --mirror` specially when a repository is empty. Detect this case by seeing if `git for-each-ref --count 1` does anything. If the repository is empty, just bail.
Test Plan:
- Observed an empty and non-empty repository.
- Mirrored an empty and non-empty repository.
Reviewers: alexmv, amckinley
Reviewed By: alexmv
Subscribers: Korvin, epriestley
Maniphest Tasks: T5965
Differential Revision: https://secure.phabricator.com/D18920
Summary: Help for GD SetupChcek was missing the "how to install extension" content.
Test Plan: Uninstalled gd, validated extension installation instructions were present in Setup Issue.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18922
Summary:
See PHI309. There is a window of time between when all builds pass and when Harbormaster actually publishes a revision out of draft.
If any other user tries to interact with the revision during that window, they'll pick up the undraft transaction as a side effect. However, they won't have permission to apply it and will be stopped by a validation error.
Instead, only automatically publish a revision if the actor is the revision author or some system/application user (essentially always Harbormaster).
Test Plan:
- Added a `echo ...; sleep(30);` to `HarbormasterBuildEngine->updateBuildable()` before the `applyTransactions()` at the bottom.
- Wrote an "Always, run an HTTP request" Herald rule and Harbormaster build plan.
- Ran daemons with `bin/phd debug task`.
- Created a new revision with `arc diff`, as user A.
- Waited for `phd` to enter the race window.
- In a separate browser, as user B, submitted a comment via `differential.revision.edit`.
- Before patch: edits during the race window were rejected with a validation error, "you don't have permission to request review".
- After patch: edits go through cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18921
Summary: Depends on D18917. Ref T13046. While I'm in here, update this to use more modern construction.
Test Plan: Browed and queried for push logs.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18918
Summary:
Depends on D18915. Ref T13046.
- Distinguish between HTTP and HTTPS.
- Use more constants and fewer magical strings.
- For HTTP responses, give them better type information and more helpful UI behaviors.
Test Plan: Pulled over SSH and HTTP. Reviewed resulting logs from the web UI. Hit errors like missing/invalid credentials.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18917
Summary: Depends on D18914. Updates this Query to use slightly more modern construction while I'm working in adjacent code.
Test Plan: Viewed push logs in web UI.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18915
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 D18908. Ref T13043. Allow users to get information about what revokers do with a new `--list` flag.
You can use `--list --type <key>` to get information about a specfic revoker.
Test Plan: Ran `bin/auth revoke --list`, saw a list of revokers with useful information.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18910
Summary:
Depends on D18907. Ref T13043. Ref T12509. We have some weird old password digest behavior that isn't terribly concerning, but also isn't great.
Specifically, old passwords were digested in weird ways before being hashed. Notably, account passwords were digested with usernames, so your password stops working if your username is chagned. Not the end of the world, but silly.
Mark all existing hashes as "v1", and automatically upgrade then when they're used or changed. Some day, far in the future, we could stop supporting these legacy digests and delete the code and passwords and just issue upgrade advice ("Passwords which haven't been used in more than two years no longer work."). But at least get things on a path toward sane, modern behavior.
Test Plan: Ran migration. Spot-checked that everthing in the database got marked as "v1". Used an existing password to login successfully. Verified that it was upgraded to a `null` (modern) digest. Logged in with it again.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043, T12509
Differential Revision: https://secure.phabricator.com/D18908
Summary:
Depends on D18906. Ref T13043. When SSH keys are edited, we normally include a warning that if you don't recognize the activity you might have problems in the mail body.
Currently, this warning is also shown for revocations with `bin/auth revoke --type ssh`. However, these revocations are safe (revocations are generally not dangerous anyway) and almost certainly legitimate and administrative, so don't warn users about them.
Test Plan:
- Created and revoked a key.
- Creation mail still had warning; revocation mail no longer did.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18907
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. After D18903, this data has migrated to shared infrastructure and has no remaining readers or writers.
Just delete it now, since the cost of a mistake here is very small (users need to "Forgot Password?" and pick a new password).
Test Plan: Grepped for `passwordHash`, `passwordSalt`, and variations.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18904
Summary:
Ref T13043. This moves user account passwords to the new shared infrastructure.
There's a lot of code changes here, but essentially all of it is the same as the VCS password logic in D18898.
Test Plan:
- Ran migration.
- Spot checked table for general sanity.
- Logged in with an existing password.
- Hit all error conditions on "change password", "set password", "register new account" flows.
- Verified that changing password logs out other sessions.
- Verified that revoked passwords of a different type can't be selected.
- Changed passwords a bunch.
- Verified that salt regenerates properly after password change.
- Tried to login with the wrong password, which didn't work.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18903
Summary:
Ref T13045. See that task for discussion.
This replaces `digestForIndex()` with a "clever" algorithm in `digestForAnchor()`. The new digest is the same as `digestForIndex()` except when the original output was "." or "_". In those cases, a replacement character is selected based on entropy accumulated by the digest function as it iterates through the string.
Test Plan: Added unit tests.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13045
Differential Revision: https://secure.phabricator.com/D18909
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. This cleans some things up to prepare for moving account passwords to shared infrastructure.
Currently, the (very old, fairly unusual) `bin/accountadmin` tool can set account passwords. This is a bit weird, generally not great, and makes upgrading to shared infrastructure more difficult. Just get rid of this to simplify things. Many installs don't have passwords and this is pointless and unhelpful in those cases.
Instead, let `bin/auth recover` recover any account, not just administrator accounts. This was a guardrail against administrative abuse, but it has always seemed especially flimsy (since anyone who can run the tool can easily comment out the checks) and I use this tool in cluster support with some frequency, occasionally just commenting out the checks. This is generally a better solution than actually setting a password on accounts anyway. Just get rid of the check and give users enough rope to shoot themselves in the foot with if they truly desire.
Test Plan:
- Ran `bin/accountadmin`, didn't get prompted to swap passwords anymore.
- Ran `bin/auth recover` to recover a non-admin account.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18901
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
