Summary:
Fixes T5912. When we write files, we attempt to share storage if two files have the same content.
In some cases, we may not share storage. Examples include:
- Files migrated with `bin/files migrate` (it's simpler not to try to dedupe them).
- Old files, from before storage was sharable (the mechanism did not exist).
- Files broken by the bug fixed in T5912.
Add a script to compact files by pointing files with the same content hash at the same file contnet.
In the particular case of files broken by the bug in T5912, we know the hash of the file's content and will only point them at a file that we can load the data for, so this fixes them.
Compaction is not hugely useful in general, but this script isn't too complex and the ability to fix damage from the bug in T5912 is desirable. We could remove this capability eventually.
Test Plan:
- Ran `files compact --all --dry-run` and sanity checked a bunch of the duplicates for actually being duplicates.
- Migrated individual files with `files compact Fnnn --trace` and verified the storage compacted and all files survived the process.
- Verified unused storage was correctly destroyed after removing the last reference to it.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5912
Differential Revision: https://secure.phabricator.com/D10327
Summary:
Fixes T5926. Fixes T5830. Ref T4767. Users currently sometimes have a hard time understanding repository update frequencies. This is compounded by aggressive backoff and incorrect backoff while importing repositories.
- Don't back off while importing repositories. This prevents us from hanging at 99.99% for inactive repositories while waiting for the next update.
- Back off less aggressively in general, and even more gradually during the first 3 days. This should make behavior around weekends better.
- Show update frequency in the UI.
- Provide an explicit "update now" button to call `diffusion.looksoon` in a more user-friendly way.
- Document how backoff policies work and how to adjust behavior.
Test Plan:
- Ran `bin/phd debug pulllocal` and verified backoff worked correctly from debugging output.
- Clicked "Update Now" to get a hint, reloaded page to see it update.
- Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4767, T5830, T5926
Differential Revision: https://secure.phabricator.com/D10323
Summary:
Fixes T5934. If you hash a password with, e.g., bcrypt, and then lose the bcrypt hasher for some reason, we currently fatal when trying to figure out if we can upgrade.
Instead, detect that the current hasher implementation has vanished and let the user reset their password (for account passwords) or choose a new one (for VCS passwords)>
Test Plan:
Account password:
- Artifically disabled bcrypt hasher.
- Viewed password panel, saw warnings about missing hasher.
- Used password reset workflow to change password, saw iterated MD5 hashed password get set.
- Enabled bcrypt hasher again.
- Saw upgrade warning.
- Upgraded password to bcrypt.
VCS password:
- Artificially disabled bcrypt hasher.
- Viewed password panel, saw warnings about missing hasher.
- Reset password.
- Saw iterated md5 password.
- Reenabled bcrypt.
- Upgraded to bcrypt.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5934
Differential Revision: https://secure.phabricator.com/D10325
Summary: Ref T4284. This fixes at least one problem which can cause the observed behavior.
Test Plan:
- Before applying patch, used `PHABRICATOR_CREDENTIAL=PHID-CDTL-... bin/ssh-connect` + debugging prints to verify the keyfile was written and cleaned up normally.
- Destroyed the credental, verified the temporary file was not cleand up correctly.
- Applied patch, verified temporary file was not written and command exited with sensible error.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4284
Differential Revision: https://secure.phabricator.com/D10328
Summary: Clean up some arg handling stuff.
Test Plan: Used this while debugging.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10314
Summary: This was broken by rP5ac36e8 by a derpy typo.
Test Plan: Ran dry run against a revision with a a repository, saw the field fill in on the transcript.
Reviewers: nickz, btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10326
Summary:
Additional audit states were made queryable for T5871.
Include them in Conduit's audit.query as well. In doing so corrects
references from "status-foo" to "audit-status-foo".
Depends on D10271
Test Plan: with an api, issues queries and got sensible results
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10290
Summary:
Ref T5932. Ref T5936. This implements build generations in Harbormaster, which provides the infrastructure required to both show users the previous states of restarted builds and to allow users to forcefully abort builds (and their targets).
You can view previous generations of a build by adding `?g=<n>` to the URI, but this isn't exposed in the UI anywhere yet.
Test Plan: Ran a build plan with a Sleep step in it. Reconfigured it for various sleep times and viewed previous generations of the build after restarting it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5932, T5936
Differential Revision: https://secure.phabricator.com/D10321
Summary:
Resolves T5937. HTTPS redirects caused by `security.require-https` use a full scheme, domain and port in the URI. Consequently, this causes invocation of the new external redirect logic and prevents redirection from occurring properly when accessing the HTTP version of Phabricator that has `security.require-https` turned on.
I've also fixed the automatic slash redirection logic to add the external flag where appropriate.
Test Plan: Configured SSL on my local machine and turned on `security.require-https`. Observed the "Refusing to redirect" exception on master, while the redirect completed successfully with this patch.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5937
Differential Revision: https://secure.phabricator.com/D10318
Summary: Caught this with the new redirect validation logic. The `$return_uri` was being set as just `B123` which is not valid. Prefixing it with `/` (like is done in `HarbormasterBuildActionController` already) gives the correct result of reloading the buildable's page.
Test Plan: Restarted all builds on a buildable, saw the page reload correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10320
Summary:
Fixes T5912. When migrating files, we try to clean up the old data. However, this code isn't aware of reference counting, and unconditionally destroys the old data.
For example, if you migrate files `F1` and `F2` and they have the same data, we'll delete the shared data when we migrate `F1`. Then you'll get an error when you migrate `F2`.
Since this only affects duplicate files, it primarily hits default profile pictures, which are the most numerous duplicate files on most installs.
Test Plan:
- Verified that the theory was correct by uploading two copies of a file and migrating the first one, before applying the patch. The second one's data was nuked and it couldn't be migrated.
- Applied patch.
- Uploaded two copies of a new file, migrated the first one (no data deletion), migrated the second one (data correctly deleted).
- Uploaded two copies of another new file, `bin/remove destory'd` the first one (no data deletion), then did it to the second one (data correctly deleted).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5912
Differential Revision: https://secure.phabricator.com/D10312
Summary: Ref T5915. Make `bin/remove destroy` a bit more thorough, since Herald transcripts can have field information in them.
Test Plan: Used `bin/remove destroy` to nuke revisions, saw their transcripts vanish too.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5915
Differential Revision: https://secure.phabricator.com/D10306
Summary: Fixes T5915. Occasionally, users derp up and diff private key material. Adding a pre-write Herald phase enables configuration of a partial layer of protection that will reject these changes before they hit disk, provided they can be detected by, e.g., filename.
Test Plan:
- Added a rule with checks on every field, verified they looked fine in the transcript.
- Created some revisions to test those changes (I have a bunch of revision rules locally).
- Verified rejects don't write transcripts to the database.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5915
Differential Revision: https://secure.phabricator.com/D10305
Summary:
If daemon data is mangled, `bin/phd restart` will SIGINT process `0`, which kills it.
uh oh T.T so sad
Test Plan: Used `bin/phd start` to start daemons; removed PID information from one; saw `bin/phd stop` shut down cleanly and not kill itself.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mholden, epriestley
Differential Revision: https://secure.phabricator.com/D10308
Summary: and for bonus, finesse some URIs a tad. Fixes T5922.
Test Plan: viewed F1 logged out and it worked! viewed the ugly URI for F1 and got redirected to the pretty URI.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5922
Differential Revision: https://secure.phabricator.com/D10309
Summary: its not necessary. Fixes T5906
Test Plan: clicked "Login to Comment" and went straight to the login form, sans intermediary dialogue
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5906
Differential Revision: https://secure.phabricator.com/D10295
Summary: Fixes T5918.
Test Plan: Verified memes work again.
Reviewers: hach-que, btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5918
Differential Revision: https://secure.phabricator.com/D10307
Summary: Ref T5894. We have a couple more similar cases. Make them all do a decision-based redirect for now.
Test Plan: Did "View Raw File" and such, and also made sure thumbnails still work.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5894
Differential Revision: https://secure.phabricator.com/D10301
Summary: Straightforward (this is the one object type we do let you delete from the web UI) implemetation of `PhabricatorDestructibleInterface`.
Test Plan: Used `bin/remove destroy` to destory several files. Used `--trace` to verify they wiped file data.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10300
Summary: This has never been enabled by default, and isn't safe. Remove it since people can use preallocated or EC2 hosts.
Test Plan: Removed it; didn't see it appear on the "Create Blueprint" page.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10287
Summary: Fixes T5894. This needs some improvement when we lay in real CDN stuff, but should get all the cases right for now.
Test Plan: Thumbnails work properly again.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5894
Differential Revision: https://secure.phabricator.com/D10299
Summary: Fixes T5798. We basically weren't using the caching mechanism. Also adds service calls for S3 stuff, and support for seeing a little info like you can for conduit.
Test Plan: uploaded a paste, looked at paste list - no s3 service calls. edited the paste, looked at paste list - no s3 service calls and edited content properly shown
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5798
Differential Revision: https://secure.phabricator.com/D10294
Summary: Fixes T5871. These queries get to use the actual column on the commit table since they are about the "aggregate" state of different audits.
Test Plan: issues queries and got sensible results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5871
Differential Revision: https://secure.phabricator.com/D10271
Summary:
Via HackerOne. Chrome (at least) interprets backslashes like forward slashes, so a redirect to "/\evil.com" is the same as a redirect to "//evil.com".
- Reject local URIs with backslashes (we never generate these).
- Fully-qualify all "Location:" redirects.
- Require external redirects to be marked explicitly.
Test Plan:
- Expanded existing test coverage.
- Verified that neither Diffusion nor Phriction can generate URIs with backslashes (they are escaped in Diffusion, and removed by slugging in Phriction).
- Logged in with Facebook (OAuth2 submits a form to the external site, and isn't affected) and Twitter (OAuth1 redirects, and is affected).
- Went through some local redirects (login, save-an-object).
- Verified file still work.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10291
Summary: Ref T2787. This provides a purchase detail screen (which has nothing useful on it yet) and converts a bunch of PHIDs into slightly more useful links.
Test Plan: Browsed around my account.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10284
Summary: Fixes T5471.
Test Plan: Used `bin/remove destroy` to destroy a dashboard and a panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5471
Differential Revision: https://secure.phabricator.com/D10283
Summary: Ref T5891. This just modernizes infrastructure.
Test Plan: Viewed "All" and "Unread" notifications.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5891
Differential Revision: https://secure.phabricator.com/D10281
Summary:
Fixes T5140. When you ajax-edit a task and we send back a full-size card, we currently always put a drag grip on it.
If you clicked the "edit" thing from a priority-ordered list, this is appropriate. However, if you clicked it from some other type of list, it is not.
Pass the expected grippableness through the call.
Test Plan:
- Edited a task from a reorderable (priority-ordered) view, got grip.
- Edited a task from a nonreorderable (author-ordered) view, got no grip.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5140
Differential Revision: https://secure.phabricator.com/D10282
Summary:
Fixes T5885. This implements optional soft point limits for workboard columns, per traditional Kanban.
- Allow columns to have a point limit set.
- When a column has a point limit, show it in the header.
- If a column has too many points in it, show the column and point count in red.
@chad, this could probably use some design tweaks. In particular:
- I changed the color of "hidden" columns to avoid confusion with "overfull" columns. We might be able to find a better color.
- UI hints for overfull columns might need adjustment.
(After T4427, we'll let you sum some custom field instead of total number of tasks, which is why this is called "points" rather than "number of tasks".)
Test Plan:
{F190914}
Note that:
- "Pre-planning" has a limit, so it shows "4/12".
- "Planning" has a limit and is overfull, so it shows "5 / 4".
- Other columns do not have limits.
- "Post-planning" is a hidden column. This might be too muted now.
Transactions:
{F190915}
Error messages / edit screen:
{F190916}
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T5885
Differential Revision: https://secure.phabricator.com/D10276
Summary:
Ref T5885. See D10276.
Currently, ActionHeaders can only have minicons, and we don't use them anywhere and they probably don't make much sense in the product anymore.
Instead, allow them to have font icons. Remove minicons, which have no callsites and probably won't in the future.
Test Plan:
{F190925}
- Grepped for `minicons`.
- Grepped for `setHeaderIcon()`.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5885
Differential Revision: https://secure.phabricator.com/D10277
Summary: Fixes T5884. Macro images are no longer public on most installs. We could generate tokens for them, but this (using Conduit to pull the file data) is easier and more correct.
Test Plan: Logged a bot into IRC and had it spam part of a macro before being killed for flooding.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5884
Differential Revision: https://secure.phabricator.com/D10274
Summary: Ref T5884. We migrated with "canCDN" and then had live writes with "cancdn". Move everything to "canCDN" for consistency.
Test Plan: Ran migration, verified DB only has "canCDN" afterward.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5884
Differential Revision: https://secure.phabricator.com/D10273
Summary:
Ref T992.
- Format text/HTML bodies explicitly in `bin/mail show-outbound`.
- Provide `bin/mail show-outbound --dump-html` so you can do something like `bin/mail show-outbound --dump-html > dump.html; open dump.html` to get a browser preview somewhat easily.
Test Plan: Ran `bin/mail show-outbound` with and without `--dump-html` flag.
Reviewers: talshiri, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D10272
Summary:
When enabled, this will show the full history of review comments in an
email-compatible threading-view.
Test Plan: Sending emails with the option on and off.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10146
Summary: Ref T4427. This always counts 1 task = 1 point. The tricky bit is making this update in JS.
Test Plan: {F190900}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4427
Differential Revision: https://secure.phabricator.com/D10275
Summary:
Added support for side-by-side HTML and plaintext email building.
We can control if the HTML stuff is sent by by a new config, metamta.html-emails
Test Plan:
Been running this in our deployment for a few months now.
====Well behaved clients====
- Gmail
- Mail.app
====Bad clients====
- [[ http://airmailapp.com/ | Airmail ]]. They confuse Gmail too, though.
====Need testing====
- Outlook (Windows + Mac)
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: webframp, taoqiping, chad, epriestley, Korvin
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D9375
Summary: Fixes T5575. Moves "All" links into title/header. Mark all read floats left, and connection status sits in footer. Also added hints to enable notifications (it's a cool feature).
Test Plan:
Tested locally both menus.
{F190630}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5575
Differential Revision: https://secure.phabricator.com/D10269
Summary: Fixes T5453.
Test Plan: made a remarkup comment that "Q1 is dumb and Q10 is awesome" and only Q10 was linked. changed the new setting to have the value " " and the Q1 also started linking.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5453
Differential Revision: https://secure.phabricator.com/D10270
Summary: Ref T5862. makes the exception work better
Test Plan: issued some queries from audit ui with and without repos - they worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5862
Differential Revision: https://secure.phabricator.com/D10268
Summary: Fixes T5862. The Diffusion table uses `id` but all the other infrastructure uses `phid` so just do a quick load of the repositories to get the ids. Long term, we should re-key the table by phid I think.
Test Plan: made a query with a repository and got a proper result set
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5862
Differential Revision: https://secure.phabricator.com/D10245
Summary: Should help with debugging. In the web UI there is now a link to "View Email". This uses the same debugging output that `./bin/mail show-outbound` uses. Fixes T5768. Code is very defensive as I think these tables may be truncated eventually?
Test Plan: viewed some emails and it worked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5768
Differential Revision: https://secure.phabricator.com/D10244
Summary:
Fixes T5727. Updates the regexes to split on '-'. Also changes the editor such that tokens are updated by the larger search process. (Note this means we update this data more often then we need to - for every project transaction.)
Users will need to make an edit to a project -or- run `bin/search index "#project-tag"` to make this actually work.
Test Plan: Made "Frontend-Engineering", "Engineering", and "Backend-Enginering". They all showed up in the typeahead!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5727
Differential Revision: https://secure.phabricator.com/D10247
Summary: Fixes T5872. This won't show up in the initial email until T4896 is further along.
Test Plan:
```
RECIPIENTS
discoball (Disco Ball)
BODY
epriestley added a comment.
ffkn
USERS
epriestley (Author)
COMMIT
http://local.aphront.com:8080/rPOEMS165b6c54f487
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5872
Differential Revision: https://secure.phabricator.com/D10266
Summary:
Ref T5884. We migrated to add a `canCDN` flag, but the code looks for a `cancdn` flag.
If this fixes the issue, I'll migrate `cancdn` to `canCDN` in the next diff.
Test Plan: Viewed some files, including old files, and saw the cacheability I expected.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5884
Differential Revision: https://secure.phabricator.com/D10264
