Summary:
Ref T11140. This makes encryption actually work:
- Provide a new configuation option, `keyring`, for specifying encryption keys.
- One key may be marked as `default`. This activates AES256 encryption for Files.
- Add `bin/files generate-key`. This is helps when generating valid encryption keys.
- Add `bin/files encode`. This changes the storage encoding of a file, and helps test encodings and migrate existing data.
- Add `bin/files cycle`. This re-encodes the block key with a new master key, if your master key leaks or you're just paraonid.
- Document all these options and behaviors.
Test Plan:
- Configured a bad `keyring`, hit a bunch of different errors.
- Used `bin/files generate-key` to try to generate bad keys, got appropriate errors ("raw doesn't support keys", etc).
- Used `bin/files generate-key` to generate an AES256 key.
- Put the new AES256 key into the `keyring`, without `default`.
- Uploaded a new file, verified it still uploaded as raw data (no `default` key yet).
- Used `bin/files encode` to change a file to ROT13 and back to raw. Verified old data got deleted and new data got stored properly.
- Used `bin/files encode --key ...` to explicitly convert a file to AES256 with my non-default key.
- Forced a re-encode of an AES256 file, verified the old data was deleted and a new key and IV were generated.
- Used `bin/files cycle` to try to cycle raw/rot13 files, got errors.
- Used `bin/files cycle` to cycle AES256 files. Verified metadata changed but file data did not. Verified file data was still decryptable with metadata.
- Ran `bin/files cycle --all`.
- Ran `encode` and `cycle` on chunked files, saw commands fail properly. These commands operate on the underlying data blocks, not the chunk metadata.
- Set key to `default`, uploaded a file, saw it stored as AES256.
- Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11140
Differential Revision: https://secure.phabricator.com/D16127
Summary: Ref T11098. There is no reason to maintain these as separate values now that they can be configured in global settings.
Test Plan:
- Hit and read setup issue.
- Fiddled with settings.
- I'll vet this more throughly in the next diff since I need to fix an issue with global defaults in mail and can explicitly test this at the same time.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T11098
Differential Revision: https://secure.phabricator.com/D16117
Summary: Ref T11120. If this works, I'll just remove this option completely.
Test Plan: ¯\_(ツ)_/¯
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11120
Differential Revision: https://secure.phabricator.com/D16095
Summary:
Ref T4103. This doesn't get everything, but takes care of most of the easy stuff.
The tricky-ish bit here is that I need to move timezones, pronouns and translations to proper settings. I expect to pursue that next.
Test Plan:
- Grepped for `loadPreferences` to identify callsites.
- Changed start-of-week setting, loaded Calendar, saw correct start.
- Visited welcome page, read "Adjust Settings" point.
- Loaded Conpherence -- I changed behavior here slightly (switching threads drops the title glyph) but it wasn't consistent to start with and this seems like a good thing to push to the next version of Conpherence.
- Enabled Filetree, toggled in Differential.
- Disabled Filetree, no longer visible in Differential.
- Changed "Unified Diffs" preference to "Small Screens" vs "Always".
- Toggled filetree in Diffusion.
- Edited a task, saw sensible projects in policy dropdown.
- Viewed user profile, uncollapsed/collapsed side nav, reloaded page, sticky'd.
- Toggled "monospaced textareas", used a comment box, got appropriate fonts.
- Toggled durable column.
- Disabled title glyphs.
- Changed monospaced font to 18px/36px impact.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16004
Summary:
Fixes T11043. This page was still reading the old information directly instead of going through the cluster-aware stuff.
Have it ask the cluster-aware stuff for information instead.
Test Plan:
- Nuked MySQL on localhost.
- Configured cluster hosts.
- Loaded "Database Status" page -- worked after patch.
- Grepped for any remaining `mysql.configuration-provider` stragglers, came up empty.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11043
Differential Revision: https://secure.phabricator.com/D15982
Summary:
Ref T10939. I'm not //totally// opposed to the existence of this element, but I think it's the kind of thing that would never make it upstream today. I think this should just be a T418 custom sort of thing in the long run, not a mainline upstream feature.
Overall, I think this thing is nearly useless and just adds visual clutter. My dashboard is about 100% red. This also sort of teaches users that it's fine to let revisions sit for a couple of days, which isn't what I'd like the UI to teach. Finally, removing it helps the UI feel a little less cluttered after the visually busy changes in D15926.
Test Plan: Grepped for removed config. Viewed revision list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10939
Differential Revision: https://secure.phabricator.com/D15927
Summary: Ref T10694. This setting no longer has any effect: we always show a limited amount of context now.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10694
Differential Revision: https://secure.phabricator.com/D15886
Summary:
Fixes T10876. Currently, we can end up with a setup warning banner sticking on each web device, since the state is stored in local cache.
Instead:
- When we actually run the setup checks, save the current state in the database.
- Before we show a cached banner, make sure the database still says the checks are a problem.
This could lead to some inconsistencies if setup checks legitimately pass on some hosts but not on others. For example, if you have `git` installed on one machine but not on another, we may raise a setup warning ("No Git Binary!") about it on one host only.
For now, assume users have their operational environments in some sort of reasonable shape and can install the same stuff everywhere. In the future, we could split the issues into "global" and "per-host" issues if we run into problems with this.
Test Plan:
This is somewhat tricky to test locally since you really need multiple webservers to test it properly, but I:
- Created some setup issues, saw banner.
- Ignored/cleared them, saw banner go away.
- Verified database cache writes were occurring properly.
Then I sort of faked it like this:
- Created a setup issue.
- Manually set the database cache value to `[]` ("no issues").
- Reloaded page.
- No more banner.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10876
Differential Revision: https://secure.phabricator.com/D15802
Summary: Ref T4292. This adds a new high-level overview panel.
Test Plan: {F1238854}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15772
Summary:
Ref T10832. In practice, `git --version` is not a useful test for this issue:
- Vendors like Debian have backported the patch into custom versions like `0.0.0.1-debian-lots-of-patches.3232`.
- Vendors like Ubuntu distribute multiple different versions which report the same string from `git --version`, some of which are patched and some of which are not.
In other cases, we can perform an empirical test for the vulnerability. Here, we can not, because we can't write a 2GB path in a reasonable amount of time.
Since vendors (other than Apple) //generally// seem to be on top of this and any warning we try to raise based on `git --version` will frequently be incorrect, don't raise this warning.
I'll note this in the changelog instead.
Test Plan: Looked at setup issues, no more warning for vulnerable git version.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10832
Differential Revision: https://secure.phabricator.com/D15756
Summary: Ref T10832. Raise a setup warning for out-of-date versions of `git`.
Test Plan: {F1224632}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10832
Differential Revision: https://secure.phabricator.com/D15745
Summary:
Fixes T9385. This was accidentally mangled a bit a long time ago by D12797, which was a 1,000-file change which got almost everything right.
Simplify the message and fix all the `%s` conversions and how they map to parameters.
Test Plan: {F1220400}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9385
Differential Revision: https://secure.phabricator.com/D15722
Summary:
Fixes T10697. This finishes bringing the rest of the config up to cluster power levels.
Phabricator is now given an arbitrarily long list of notification servers.
Each Aphlict server is given an arbitrarily long list of ports to run services on.
Users are free to make them meet in the middle by proxying whatever they want to whatever else they want.
This should also accommodate clustering fairly easily in the future.
Also rewrote the status UI and changed a million other things. 🐗
Test Plan:
{F1217864}
{F1217865}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15703
Summary: Ref T10697. Mostly straightforward. Also allow the server to have multiple logs and log options in the future (e.g., different verbosities or separate admin/client logs or whatever). No specific plans for this, but the default log is pretty noisy today.
Test Plan: Set up a couple of logs, started server, saw it log to them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15702
Summary: Ref T10697. This isn't everything but starts generalizing options and moving us toward a cluster-ready state of affairs.
Test Plan: Started server in various configurations, hit most (all?) of the error cases with bad configs, sent test notifications.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15701
Summary:
Ref T10784. Currently, if you terminate SSL at a load balancer (very common) and use HTTP beyond that, you have to fiddle with this setting in your premable or a `SiteConfig`.
On the balance I think this makes stuff much harder to configure without any real security benefit, so don't apply this option to intracluster requests.
Also document a lot of stuff.
Test Plan: Poked around locally but this is hard to test outside of a production cluster, I'll vet it more thoroughly on `secure`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10784
Differential Revision: https://secure.phabricator.com/D15696
Summary:
Ref T4292. This adds some very basic cluster/device data to the new management view. Nothing interesting yet.
Also deal with disabled bindings a little more cleanly.
Test Plan: {F1214619}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15685
Summary:
Ref T4571. Write more of the missing documentation sections and clarify a few things.
Since the "replicating master" check needs a special permission, imposes a performance penalty, is probably very difficult to misconfigure, and likely not a big deal anyway, just drop the idea of trying to automatically detect + prevent it. We still show if it's an issue on the status page, provided we have permission to check.
When you don't have any cluster databases configured, never stop trying to connect to the default master database. We might want to do this eventually as load reduction, but just don't muddy the waters too much for now while things stabilize.
Test Plan:
- Tested functionality in cluster, non-cluster, and degraded-cluster modes.
- Used status console to monitor a health check cycle.
- Read docs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15679
Summary:
Ref T4571. When a database goes down briefly, we fall back to replicas.
However, this fallback is slow (not good for users) and keeps sending a lot of traffic to the master (might be bad if the root cause is load-related).
Keep track of recent connections and fully degrade into "severed" mode if we see a sequence of failures over a reasonable period of time. In this mode, we send much less traffic to the master (faster for users; less load for the database).
We do send a little bit of traffic still, and if the master recovers we'll recover back into normal mode seeing several connections in a row succeed.
This is similar to what most load balancers do when pulling web servers in and out of pools.
For now, the specific numbers are:
- We do at most one health check every 3 seconds.
- If 5 checks in a row fail or succeed, we sever or un-sever the database (so it takes about 15 seconds to switch modes).
- If the database is currently marked unhealthy, we reduce timeouts and retries when connecting to it.
Test Plan:
- Configured a bad `master`.
- Browsed around for a bit, initially saw "unrechable master" errors.
- After about 15 seconds, saw "major interruption" errors instead.
- Fixed the config for `master`.
- Browsed around for a while longer.
- After about 15 seconds, things recovered.
- Used "Cluster Databases" console to keep an eye on health checks: it now shows how many recent health checks were good:
{F1213397}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15677
Summary:
Ref T4571. Ref T10759. Ref T10758. This isn't complete, but gets most of the job done:
- When `cluster.databases` is set up, most things ignore `mysql.host` now.
- You can `bin/storage upgrade` and stuff works.
- You can browse around in the web UI and stuff works.
There's still a lot of weird tricky stuff to navigate, and this has real no advantages over configuring a single server yet (no automatic failover, etc).
Test Plan:
- Configured `cluster.databases` to point at my `t1.micro` hosts in EC2 (master + replica).
- Ran `bin/storage upgrade`, got a new install setup on them properly.
- Survived setup warnings, browsed around.
- Switched back to local config, ran `bin/storage upgrade`, browsed around, went through setup checks.
- Intentionally broke config (bad hosts, no masters) and things seemed to react reasonably well.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571, T10758, T10759
Differential Revision: https://secure.phabricator.com/D15668
Summary: Ref T4571. The configuration option still doesn't do anything, but add a status panel for basic setup monitoring.
Test Plan:
Here's what a good version looks like:
{F1212291}
Also faked most of the errors it can detect and got helpful diagnostic messages like this:
{F1212292}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15667
Summary:
Ref T4571. This adds a new option which allows you to upgrade your one-host configuration to a multi-host configuration by configuring it.
Doing this currently does nothing. I wrote a lot of words about what it is //supposed// to do in the future, though.
Test Plan:
- Tried to configure the option in all the possible bad ways, got errors.
- Read documentation.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15663
Summary:
Ref T4571. There will be a very long path beyond this, but add a basic read-only mode. You can explicitly enable this to put Phabricator in a sort of "maintenance" mode today if you're swapping databases or something.
In the long term, we'll automatically degrade into this mode if the master database is down.
Test Plan:
- Enabled read-only mode.
- Browsed around.
- Didn't immediately see anything that was totally 100% broken.
Most stuff is 80-90% broken right now. For example:
- Stuff like submitting comments doesn't work, and gives you a confusing, unhelpful error.
- None of the UI really knows that it's read-only. EditEngine stuff should all hide itself and say "you can't add new comments while an install is in read-only mode", for example, but currently does not.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15662
Summary: Testing out a new 'nav' layout in Settings / Config. Spent a few days here and couldn't find much better overall.
Test Plan: View each page in Settings and in Config. Save some config options. Test mobile, desktop, tablet.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15659
Summary: Converts Config to new UI, updates to `newPage`
Test Plan: Review all pages in Config, setup issues, ignore an issue, edit a config option
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15587
Summary:
Ref T10449. Currently, we store classes (like "AlmanacClusterRepositoryServiceType") in the database.
Instead, store types (like "cluster.repository").
This is a small change, but types are a little more flexible (they let us freely reanme classes), a little cleaner (fewer magic strings in the codebase), and a little better for API usage (they're more human readable).
Make this minor usability change now, before we unprototype.
Also make services searchable by type.
Also remove old Almanac API endpoints.
Test Plan:
- Ran migration, verified all data migrated properly.
- Created, edited, rebound, and changed properties of services.
- Searched for services by service type.
- Reviewed available Conduit methods.
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim
Maniphest Tasks: T10449
Differential Revision: https://secure.phabricator.com/D15346
Summary: Moves all the one off object calls to PHUIRemarkupView, adds a "Document" call as well (future plans).
Test Plan: Visited most pages I could get access to, but may want extra careful eyes on this diff.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15281
Summary: Fixes T10259. There was no real reason to do this `ip2long()` stuff in the first place -- it's very slightly smaller, but won't work with ipv6 and the savings are miniscule.
Test Plan:
- Ran migration.
- Viewed logs in web UI.
- Pulled and pushed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10259
Differential Revision: https://secure.phabricator.com/D15165
Summary:
A user in IRC seemed very confused by this, and worked extremely hard to shoot themsevles in the foot by manually writing locked configuration to the database.
Try to explain why configuration is locked better.
Test Plan:
Mostly reading.
{F1078905}
{F1078906}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15128
Summary: Mostly for consistency, we're not using other forms of icons and this makes all classes that use an icon call it in the same way.
Test Plan: tested uiexamples, lots of other random pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15125
Summary: Reasonable first pass, removes the "light" header, due to pain of upkeep. Reinforces UI color into the Profile Nav (and later likely dropmenu hovers). Most of this is reasonably easy to maintain now, but I may do a more accurate color pass after I get some more time together with it. For now this feels pretty good if you're developing in a different color UI.
Test Plan:
Switch between all the colors, hover over all the states.
{F1076766}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15120
Summary: Fixes T10217. This extra entry is just a mistake because of `rst` + `rest` both being valid suffixes. We don't need both entries.
Test Plan: Edited a paste, only saw one entry in dropdown for restructured text.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10217
Differential Revision: https://secure.phabricator.com/D15114
Summary:
Ref T10195. Distinguish between "database does not exist" and "database exists, you just don't have permission to access it".
We can't easily get this information out of INFORMATION_SCHEMA but can just `SHOW TABLES IN ...` every database that looks like it's missing and then look at the error code.
Test Plan:
- Created a user `limited` with limited access.
- Ran `bin/storage adjust`.
- Got hopefully more helpful messages about access problems, instead of "Missing" errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10195
Differential Revision: https://secure.phabricator.com/D15079
Summary: Removes header gradient images for flat, CSS controlled colors. I didn't convert the "pony" colors over, going with few options for easier theme-ability.
Test Plan:
Test each color choice.
{F1063828}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15052
Summary:
See IRC. We're supposed to repair configuration, but if custom validators throw a generic `Exception` or use `PhutilTypeSpec` to do a check, we may explode way harder than we intend to.
Instead, soften these exceptions into validation exceptions so we repair configuration, raise a setup issue, and continue.
Test Plan: {F1059609}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14998
Summary:
Ref T5155. Swaps Phabricator over to the new first-party S3 client using the v4 authentication API so it works in all regions.
The API requires an explicit region, so the new `amazon-s3.region` is now required. I'll write guidance about this.
Test Plan:
- Uploaded files to S3.
- Migrated ~1GB of files to S3.
- Loaded a bunch of files off S3.
- Browsed around the S3 bucket.
- Deleted a file, verified the data on S3 was destroyed.
- Hit new setup warning.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5155
Differential Revision: https://secure.phabricator.com/D14982
Summary:
Ref T10010. Ref T5819. General alignment of the stars:
- There were some hacks in Conduit around stripping `fa-...` off icons when reading and writing that I wanted to get rid of.
- We probably have room for a subtitle in the new heavy nav, and using the icon name is a good starting point (and maybe good enough on its own?)
- The project list was real bad looking with redundant tag/names, now it is very slightly less bad looking with non-redundant types?
- Some installs will want to call Milestones something else, and this gets us a big part of the way there.
- This may slightly help to reinforce "tag" vs "policy" vs "group" stuff?
---
I'm letting installs have enough rope to shoot themselves in the foot (e.g., define 100 icons). It isn't the end of the world if they reuse icons, and is clearly their fault.
I think the cases where 100 icons will break down are:
- Icon selector dialog may get very unwieldy.
- Query UI will be pretty iffy/huge with 100 icons.
We could improve these fairly easily if an install comes up with a reasonable use case for having 100 icons.
---
The UI on the icon itself in the list views is a little iffy -- mostly, it's too saturated/bold.
I'd ideally like to try either:
- rendering a "shade" version (i.e. lighter, less-saturated color); or
- rendering a "shade" tag with just the icon in it.
However, there didn't seem to be a way to do the first one right now (`fa-example sh-blue` doesn't work) and the second one had weird margins/padding, so I left it like this for now. I figure we can clean it up once we build the thick nav, since that will probably also want an identical element.
(I don't want to render a full tag with the icon + name since I think that's confusing -- it looks like a project/object tag, but is not.)
Test Plan:
{F1049905}
{F1049906}
Reviewers: chad
Reviewed By: chad
Subscribers: 20after4, Luke081515.2
Maniphest Tasks: T5819, T10010
Differential Revision: https://secure.phabricator.com/D14918
Summary: I feel like the daemon configuration should be locked from editing from the web UI, given that much of it won't work unless the daemons are restarted anyway.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14867
Summary:
Ref T10010. This implements technical groundwork for subprojects. Specifically, it implements policy rules like Phriction:
- to see a project, you must be able to see all of its parents (and the project itself).
- you can edit a project if you can edit any of its parents (or the project itself).
To facilitiate this, we load all project ancestors when querying projects so we can do the view/edit checks.
This does NOT yet implement:
- proper membership rules for these projects (up next);
- any kind of UI to let users create subprojects.
Test Plan:
- Added unit tests.
- Executed unit tests.
- Browsed Projects (no change in behavior is expected).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14861
Summary:
Ref T9979. This uses ngrams (specifically, trigrams) to build a reasonably efficient index for substring matching. Specifically, for a package like "Example", with ID 123, we store rows like this:
```
< ex, 123>
<exa, 123>
<xam, 123>
<amp, 123>
<mpl, 123>
<ple, 123>
<le , 123>
```
When the user searches for `exam`, we join this table for packages with tokens `exa` and `xam`. MySQL can do this a lot more efficiently than it can process a `LIKE "%exam%"` query against a huge table.
When the user searches for a one-letter or two-letter string, we only search the beginnings of words. This is probably what they want, the only thing we can do quickly, and a reasonable/expected behavior for typeaheads.
Test Plan:
- Ran storage upgrades and search indexer.
- Searched for stuff with "name contains".
- Used typehaead and got sensible results.
- Searched for `aabbccddeeffgghhiijjkkllmmnnooppqqrrssttuuvvwwxxyyzz` and saw only 16 joins.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14846
Summary:
Ref T9979. I picked this name long before the advent of modern "Engine" architecture and it ended up being pretty confusing.
Rename "SearchEngine" (currently: mysql or elasticsearch, used to store and query fulltext indexes) to "FulltextStorageEngine" to make it more clear what it does and disambituate it from ApplicationSearch, which also has a bunch of stuff called "SearchEngine", "SearchEngineExtension", etc.
Test Plan: Grepped for `phabricatorsearchengine`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14843
Summary: Ref T10003. Give installs more warning about these changes.
Test Plan: Changed configuration, saw warning. Reset configuration, no warning.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10003
Differential Revision: https://secure.phabricator.com/D14817
Summary: Ref T9908. Move all the "pro" stuff into the old locations.
Test Plan: Created/edited tasks, looked at URIs, saw non-pro ones. Grepped for `editpro`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14717
Summary: Ref T9908. This is the last of the things that need to swap over.
Test Plan:
- Created tasks from a workboard.
- Created tasks in different columns.
- Edited tasks.
- Used `?parent=..`.
- Verified that default edit form config now affects comment actions.
- No more weird comment thing on forms, at least for now.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9908
Differential Revision: https://secure.phabricator.com/D14715
Summary:
- ipull there is wrong
- The `+` wasn't doing what I thought it was doing.
- I already forgot what that detour was doing, so I wrote it down.
Test Plan: Load Versions page, see no error log.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14686
Summary: This just pretties up some config like `maniphest.custom-field-definitions` which I noticed was kind of hard to read while chasing down other stuff.
Test Plan: {F1014940}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14679
Summary:
Fixes T9874.
- Stop using the phrase "restart your webserver". Instead, say "restart Phabricator".
- Write a document explaining that "Restart Phabricator" means to restart all of the server processes, depending on how your configuration is set up, and approximately how to do that.
- Link to this document.
- In places where we are not specifically giving instructions and the user isn't expected to do anything, be intentionally vague so as to avoid being misleading.
Test Plan:
- Read document.
- Hit "exetnsion" and "PHP config" setup checks, got "restart Phabricator" with documentation links in both cases.
Reviewers: chad
Maniphest Tasks: T9874
Differential Revision: https://secure.phabricator.com/D14636
Test Plan: warning is not in warnings list.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D14594
Summary: Ref T6049, remarkup links to use short URLs and make commenting on Phurl's actually work
Test Plan:
- Create Phurl `U123`
- Comment on that Phurl `((123))`
Comment should link to `/u/123`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T6049
Differential Revision: https://secure.phabricator.com/D14477
Summary: Ref T7053. Remove the `envHash` and `envInfo` fields, which are no longer used now that the daemons restart automagically. Depends on D14458.
Test Plan: Saw no more setup issues.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: tycho.tatitscheff, epriestley
Maniphest Tasks: T7053
Differential Revision: https://secure.phabricator.com/D14446
Summary: Cleaning up house, may revisit in a v2. Removes ability to set Disqus or Facebook comments as comment system on Phame Posts.
Test Plan: Create blog, create post, edit blog, view live pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: btrahan, Korvin
Maniphest Tasks: T9746
Differential Revision: https://secure.phabricator.com/D14448
Summary: Ref T8995, config option for Phurl short domain to share shortened URL's
Test Plan:
- Configure Phurl short domain to something like "zz.us"
- Navigate to `zz.us`; get 404
- Navigate to `zz.us/u/3` or `zz.us/u/alias` where `U3` is an existing Phurl; redirect to correct destination
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T8995
Differential Revision: https://secure.phabricator.com/D14447
Summary:
Ref T9132. We have several places in the code that sometimes need to parse complex types. For example, we accept all of these in ApplicationSearch and now in ApplicationEditor:
> /?subscribers=cat,dog
> /?subscribers=PHID-USER-1111
> /?subscribers[]=cat&subscribers[]=PHID-USER-2222
..etc. The logic to parse this stuff isn't too complex, but it isn't trivial either.
Right now it lives in some odd places. Notably, `PhabricatorApplicationSearchEngine` has some weird helper methods for this stuff. Rather than give `EditEngine` the same set of weird helper methods, pull all this stuff out into "HTTPParameterTypes".
Future diffs will add "Projects" and "Users" types where all the custom parsing/lookup logic can live. Then eventually the Search stuff can reuse these.
Generally, this just breaks the code up into smaller pieces that have more specific responsibilities.
Test Plan: {F944142}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14402
Summary: See IRC. A user had a database set to 8 hours ahead of their web host. Try to catch and warn about these issues.
Test Plan: Artificially adjusted skew, saw setup warning.
Reviewers: avivey, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14371
Summary: Ref T9625. Some PHID types are missing application or icon specifications. This makes it easier to spot them.
Test Plan: {F906321}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9625
Differential Revision: https://secure.phabricator.com/D14323
Summary:
Currently, Version numbers are sort of randomly shown on "All Settings" beacuse we didn't have any better place to put them.
Now that we have modules, expose them as a config module.
Test Plan:
{F906426}
Grepped for "all settings" to look for other references to the old location, but didn't get any relevant hits.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14327
Summary:
I'm going to do some version of D13941. Clean up extra links to the old document first.
These were just randomly links from various places that we no longer really want feedback on and/or are now better covered by other documents.
Test Plan:
- `grep`
- Reviewed Config/Welcome screen.
- Reviewed `uri.allowed-editor-protocols`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14303
Summary:
Fixes T9494. This:
- Removes all the random GC.x.y.z config.
- Puts it all in one place that's locked and which you use `bin/garbage set-policy ...` to adjust.
- Makes every TTL-based GC configurable.
- Simplifies the code in the actual GCs.
Test Plan:
- Ran `bin/garbage collect` to collect some garbage, until it stopped collecting.
- Ran `bin/garbage set-policy ...` to shorten policy. Saw change in web UI. Ran `bin/garbage collect` again and saw it collect more garbage.
- Set policy to indefinite and saw it not collect garabge.
- Set policy to default and saw it reflected in web UI / `collect`.
- Ran `bin/phd debug trigger` and saw all GCs fire with reasonable looking queries.
- Read new docs.
{F857928}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9494
Differential Revision: https://secure.phabricator.com/D14219
Summary: Reachable via the cache config page, restricted to admins only. This makes it convenient to hotfix phabricator without requiring a restart.
Test Plan:
- Local dev machine doesn't have apc, so I get the not installed message.
- Faked the name and isEnabled parameters, verified dialog shows up as expected.
- Didn't test clear code
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: tycho.tatitscheff, joshuaspence, Korvin
Differential Revision: https://secure.phabricator.com/D14064
Summary:
- Fix missing space before "For example:".
- Fix instruction to run `bin/config set value` instead of `bin/config set key value`.
- Minor cleanup.
Test Plan: Tried to set `files.image-mime-types`, `load-libraries`.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14080
Summary:
Ref T9346. This mostly allows us to give users additional advice based on which instance they are trying to log in to in the Phacility cluster.
It's also slightly more flexible than `auth.login-message` was, and maybe we'll add some more hooks here eventually.
This feels like it's a sidegrade in complexity rather than really an improvement, but not too terrible.
Test Plan:
- Wrote the custom handler in T9346 to replicate old config functionality.
- Wrote a smart handler for Phacility that can provide context-sensitive messages based on which OAuth client you're trying to use.
See new message box at top (implementation in next diff):
{F780375}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9346
Differential Revision: https://secure.phabricator.com/D14057
Summary:
Fixes T9339.
- Don't show edit control for locked config at all.
- Don't show a "Cancel" button either.
- Change "Value" label to "Database Value" for non-custom config.
- Highlight effective value.
- Move examples under current state.
- Tweak some formatting.
Test Plan: {F777878}
Reviewers: chad, avivey
Reviewed By: chad, avivey
Subscribers: avivey
Maniphest Tasks: T9339
Differential Revision: https://secure.phabricator.com/D14054
Summary:
Ref T1806. Ref T7173. Depends on D14047.
Currently, all exception handling is in this big messy clump in `AphrontDefaultApplicationConfiguration`.
Split it out into modular classes. This will let a future change add new classes in the Phacility cluster which intercept particular exceptions we care about and replaces the default, generic responses with more useful, tailored responses.
Test Plan:
{F777391}
- Hit a Conduit error (made a method throw).
- Hit an Ajax error (made comment preview throw).
- Hit a high security error (tried to edit TOTP).
- Hit a rate limiting error (added a bunch of email addresses).
- Hit a policy error (tried to look at something with no permission).
- Hit an arbitrary exception (made a randomc ontroller throw).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T1806, T7173
Differential Revision: https://secure.phabricator.com/D14049
Summary: Use `PhutilClassMaQuery` instead of `PhutilSymbolLoader`, mostly for consistency. Depends on D13588.
Test Plan: Poked around a bunch of pages.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13589
Summary: Update all found callsites in Config.
Test Plan: Check setup issues, databases, edit a value, remove a warning, everything I could click on.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8628
Differential Revision: https://secure.phabricator.com/D13727
Summary: Fixes T8939. The parameters on this pht() just got juggled around at some point.
Test Plan: {F654408}
Reviewers: btrahan, avivey, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8939
Differential Revision: https://secure.phabricator.com/D13694
Summary: Fixes T6859. Adds a readout in `/config/` of installed edge and PHID types. See also D13626.
Test Plan: {F643222}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6859
Differential Revision: https://secure.phabricator.com/D13662
Summary: Use `PhutilClassMapQuery` where appropriate.
Test Plan: Browsed around the UI to verify things seemed somewhat working.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13429
Summary: Ref T8099, Missed a few various edge cases here. Cleans up the 'next' UI.
Test Plan: Review a current setup issue, then ignore it.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13529
Summary:
Ref T6817. Fixes T8731. On the old `secure` host, `feed.public` was set to `true`. I didn't bring the option over, which caused the secondary issue in T8731.
Specifically, when `feed.public` is off, a logged-out user looking at feed can't see //any// stories, so they query all of feed until they hit the time limit.
To fix this immediately, just use the most open policy, which is basically equivalent but always correct.
To fix this more thoroughly:
- Remove `feed.public`, which violates policies and has been slated for removal for a while (see T6817).
- Clean up policy handling.
Test Plan:
- As a logged-out user, viewed feed on a public install with `feed.public` off; no longer saw all stories get queried + no feed shown.
- Grepped for `feed.public`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T6817, T8731
Differential Revision: https://secure.phabricator.com/D13518
Summary:
Fixes T5702. The path here is long and windy:
- I want to move `blog.phacility.com` to the new `secure` host.
- That host has `security.require-https` set, which I want to keep set (before, this was handled in a sort of hacky way at the nginx/preamble level, but I've cleaned up everything else now).
- Currently, that setting forces blogs to HTTPS too, which won't work.
- To let blogs be individually configurable, we need to either modularize site config or make things hackier.
- Modularize rather than increasing hackiness.
- Also add a little "modules" panel in Config. See T6859. This feels like a reasonable middle ground between putting this stuff in Applications and burying it in `bin/somewhere`.
Test Plan:
- Visited normal site.
- Visited phame on-domain site.
- Visited phame off-domain site.
- Viewed static resources.
{F561897}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D13474
Summary: Ref T8099, Moving to Roboto Slab for Document Headers. It's a little less serif-y and fits well with Lato. Also took a pass at cleaning up edge cases in Documents, Diviner, Phriction, Legalpad, and Welcome Screen.
Test Plan: Test Phriction, Diviner, Legalpad, Welcome Screen
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13439
Summary: Ref T8099, uses original bgcolor when dark headers are used.
Test Plan: Switch between light and dark Phabricator, see new colors.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13361
Summary: Fixes T8589. Adds a bunch of new languages to the syntax highlighting config options so that they are supported by #paste.
Test Plan: Saw new filetypes in Paste.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8589
Differential Revision: https://secure.phabricator.com/D13337
Summary: Fixes T8346. Also gets rid of the air quotes on Auth and uses the "Appname Application" style we use elsewhere.
Test Plan: slapped an "|| true" in the checking logic and verified the link was rendered in a usable fashion just below the error message.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8346
Differential Revision: https://secure.phabricator.com/D13315
Summary: This should default to light, also made the icons on the dark headers more transparent.
Test Plan: Tested light, dark, blue, blindigo, and default.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13306
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary: Working towards a more unified look and feel. This brings in Lato as a complete base font over Helvetica Neue, as well as removing Source Sans Pro from DocumentView and Conpherence. Design-wise Lato provides the nice readability at larger font sizes that Source Sans Pro did, with the ability to scale down to tables and UI widgets with ease. This gives us one font instead of two, and now Object descriptions and Timeline posts all can benefit from a consistent, readable font.
Test Plan:
Test main UI, smaller elements like tables, menus, DocumentViews, Previews, Conpherence.
{F498135}
{F498136}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13276
Summary: Ref T8099, hashtag#yolo. Adds back the original gradients plus a 'light' theme. Unclear which should be default, but we can play with it until a decision needs to be made.
Test Plan: Change colors a lot, turn on durable column.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13146
Summary:
Ref T6367. Removes `multiplexMail()`!
We can't pass a single body into a function which splits it anymore: we need to split recipients first, then build bodies for each recipient list. This lets us build separate bodies for each recipient's individual translation/access levels.
The new logic does this:
- First, split recipients into groups called "targets".
- Each target corresponds to one actual mail we're going to build.
- Each target has a viewer (whose translation / access levels will be used to generate the mail).
- Each target has a to/cc list (the users who we'll ultimately send the mail to).
- For each target, build a custom mail body based on the viewer's access levels and settings (language prefs not actually implemented).
- Then, deliver the mail.
Test Plan:
- Read new config help.
Then did a bunch of testing, primarily with `bin/mail list-outbound` and `bin/mail show-outbound` (to review generated mail), `bin/phd debug taskmaster` (to run daemons freely) and `bin/worker execute --id <id>` (to repeatedly test a specific piece of code after identifying an issue).
With `one-mail-per-recipient` on (default):
- Sent mail to multiple users.
- Verified mail showed up in `mail list-outbound`.
- Examined mail with `mail show-outbound`.
- Added a project that a subscriber could not see.
- Verified it was not present in `X-Phabricator-Projects`.
- Verified it was rendered as "Restricted Project" for the non-permissioned viewer.
- Added a subscriber, then changed the object policy so they could not see it and sent mail.
- Verified I received mail but the other user did not.
- Enabled public replies and verified mail generated with public addresses.
- Disabld public replies and verified mail generated with private addresses.
With `one-mail-per-recipient` off:
- Verified that one mail is sent to all recipients.
- Verified users who can not see the object are still filtered.
- Verified that partially-visible projects are completely visible in the mail (this violates policies, as documented, as the best available compromise).
- Enabled public replies and verified the mail generated with "Reply To".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: carlsverre, epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13131
Summary:
Ref T8387. This describes changes I haven't made yet, but plan to make.
Also removes the long-deprecated actAsUser capability so I can remove the caveat about it from the documentation.
Test Plan: `grep`, reading
Reviewers: btrahan, eadler
Reviewed By: btrahan, eadler
Subscribers: eadler, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13120
Summary: Ref T8099, Sets a StatusIcon instead of a barColor on config status lists.
Test Plan: Review ignored and non-ignored status items.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8341, T8099
Differential Revision: https://secure.phabricator.com/D13050
Summary: Ref T8099. Adds back basic header color options, which change the logo color instead. Also RAINBOW.
Test Plan:
tested each of the new colors.
{F442284}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13040
Summary: Fixes T8274. That report is very light on details, but I think the issue is that their Elasticsearch is misconfigured and the new setup warning dies a little too hard if the server is just completely dead.
Test Plan: Set `search.elastic.host` to an invalid server, got a similar-looking exception (?), applied patch, got a setup warning instead.
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T8274
Differential Revision: https://secure.phabricator.com/D12948
Summary: Fixes T5703. These have been unused in production for a while and the new stuff seems good.
Test Plan: Mostly `grep`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5703
Differential Revision: https://secure.phabricator.com/D12949
Summary: Converts most all tables to be directly set via `setTable` to an ObjectBox. I think this path is more flexible design wise, as we can change the box based on children, and not just CSS. We also already do this with PropertyList, Forms, ObjectList, and Header. `setCollapsed` is added to ObjectBox to all children objects to bleed to the edges (like diffs).
Test Plan: I did a grep of `appendChild($table)` as well as searches for `PHUIObjectBoxView`, also with manual opening of hundreds of files. I'm sure I missed 5-8 places. If you just appendChild($table) nothing breaks, it just looks a little funny.
Reviewers: epriestley, btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12955
Summary: Removes setting colors on the headers and adds setting colors on an ObjectBoxView (which sets the headers). Ref T8099
Test Plan:
Tested each color, fixed workboard colors, added color to important setup issues.
{F410658}
{F410659}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D12942
Summary: Remove the `PhabricatorDefaultSearchEngineSelector` class. This is quite similar to D12053.
Test Plan: Went to `/view/PhabricatorSearchApplication/` and saw the storage engine configuration. Set `search.elastic.host` and saw the highlighted storage engine change.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12670
Summary:
New, cleaner, ObjectItemLists. Lots of minor style tweaks, basic overview:
- Remove FootIcons
- Remove Stackable
- Remove Plain List
- Add StatusIcon
- Add setting ObjectList to an ObjectBox
- Minor retouches to Headers
Mostly, this should give us an idea of life with the new Object Lists. I'll take another application by application pass down the road. This mostly looks at implementation in Maniphest, Differential, Audit, Workboards. Checked a few other areas and dialogs while testing, and everything looks square.
Test Plan: Maniphest, Differential, Homepage, Audit, People, and other applications. Drag reorder, etc.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12865
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Summary:
Ref T6930. This application collects and displays performance samples -- roughly, things Phabricator spent some kind of resource on. It will collect samples on different types of resources and events:
- Wall time (queries, service calls, pages)
- Bytes In / Bytes Out (requests)
- Implicit requests to CSS/JS (static resources)
I've started with the simplest case (static resources), since this can be used in an immediate, straghtforward way to improve packaging (look at which individual files have the most requests recently).
There's no aggregation yet and a lot of the data isn't collected properly. Future diffs will add more dimension data (controllers, users), more event and resource types (queries, service calls, wall time), and more display options (aggregation, sorting).
Test Plan: {F389344}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6930
Differential Revision: https://secure.phabricator.com/D12623
Summary:
Ref T4100. Ref T5595.
To support a unified "Projects:" query across all applications, a future diff is going to add a set of "Edge Logic" capabilities to `PolicyAwareQuery` which write the required SELECT, JOIN, WHERE, HAVING and GROUP clauses for you.
With the addition of "Edge Logic", we'll have three systems which may need to build components of query claues: ordering/paging, customfields/applicationsearch, and edge logic.
For most clauses, queries don't currently call into the parent explicitly to get default components. I want to move more query construction logic up the class tree so it can be shared.
For most methods, this isn't a problem, but many subclasses define a `buildWhereClause()`. Make all such definitions protected and consistent.
This causes no behavioral changes.
Test Plan: Ran `arc unit --everything`, which does a pretty through job of verifying this statically.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: yelirekim, hach-que, epriestley
Maniphest Tasks: T4100, T5595
Differential Revision: https://secure.phabricator.com/D12453
Summary: Ref T7803. Remove these in favor of more generalized paging and ordering.
Test Plan: Sorted and paged results in various applications.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12378
Summary:
Ref T5501. Currently, we emit some bad warnings about, e.g., "apc.stat" on PHP 5.5+ systems with OPcache, where the warnings are not relevant.
Generate and raise warnings out of the CacheSpec pipeline so we only run relevant code.
Test Plan: Faked various warnings and saw them render correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5501
Differential Revision: https://secure.phabricator.com/D12318
Summary: Ref T5501. This expands cache information a little more.
Test Plan: {F362975}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5501
Differential Revision: https://secure.phabricator.com/D12316
Summary:
Ref T5501. This code was headed down a bad road; dump an indirection layer between rendering and data gatehring.
In particular, this will make it much easier to lift these issues into setup warnings eventually.
Test Plan: Viewed cache status page.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5501
Differential Revision: https://secure.phabricator.com/D12315
Summary:
Ref T5501. This is just getting version detection and availability right, probably.
Eventually, this will get lifted up a bit and "$remedy" will turn into setup issues (or maybe one setup issue saying "your cache setup is messed up, click here to understand why").
Test Plan:
{F362935}
I intend to shove these up to production one-by-one since production is APC and local is Opcache + APCu.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5501
Differential Revision: https://secure.phabricator.com/D12314
Summary: Fixes T7764. These settings have low utility, are no longer used by default, have become less useful on modern Windows which has a better selection of available fonts, and will eventually be subsumed (at least, for the most part) by T4103.
Test Plan:
- Grepped for strings.
- Viewed settings.
- Changed font to "24px impact".
- Viewed diffs with default and custom font.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: chad, epriestley
Maniphest Tasks: T7764
Differential Revision: https://secure.phabricator.com/D12301
Summary: These arrays looks a little odd, most likely due to the autofix applied by `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR`. See D12296 in which I attempt to improve the autocorrection from this linter rule.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12281
Summary:
Ref T7199. In the vein of D12231, these options were a bad idea.
- They once served a very narrow, Facebook-specific need (see T1992), except even Facebook only used the Differential setting AFAIK.
- Outside of that special case, they are unused and essentially unusable (generally speaking, they do not meaningfully implement anything modular or replaceable).
- I have no knowledge of any install ever changing these settings, and can imagine no reason why they would.
Moving forward:
- If they really need to, they can fork locally and chagne one line.
- I expect "!actions" to make mail at least somewhat more modular soon, anyway.
- Any derived handlers would break after T7199 and need to be rewritten anyway, so this is just taking advantage of a BC break to do cleanup.
Test Plan:
- Grepped for removed configuration.
- Sent some mail from applications, verified the reply handlers set proper reply addresses.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12233
Summary:
Ref T7199. These were a bad idea which got copy-pasted a bunch.
- There is zero reason to ever set these to different things.
- Unsurprisingly, I don't know of any install which has them set to different things.
Unless I've completely forgotten about it, this option was not motivated by some obscure business need, it was just a bad decision which didn't catch anyone's attention at the time.
We partially remedied the mistake at some point by introducing `metamta.reply-handler-domain`, which works as a default for all applications, but never cleaned this mess up.
Test Plan: Sent some mail from applications, verified it picked up appropraite reply handler domains.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12231
Summary:
Ref T7199. Although this is useful for discovery, it's un-useful enough that we already have an option to disable it, and most applications do not provide any meaningful instructions.
Throwing it away makes it easier to move forward and lets us get rid of a config option.
This is becoming a more advanced/power-user feature anyway, and the new syntax will be significantly more complex and hard to explain with a one-liner. I'm currently thinking that I'll maybe make the "help" menu a dropdown and give it some options like:
+---+
| O |
+---+---------------------+
| Maniphest Documentation |
| Maniphest Email Actions |
+-------------------------+
Then you click the "Email Actions" thing and get a runtime-derived list of available options. Not sure if I'll actually build that, but I think we can fairly throw the in-mail instructions away even if we don't go in that specific direction.
Test Plan: Grepped for `replyHandlerInstructions`, got no hits.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12229
Summary:
Ref T6755. This is a partial fix, but:
- Allow netblocks to be blacklisted instead of making the feature all-or-nothing.
- Default to disallow requests to all reserved private/local/special IP blocks. This should generally be a "safe" setting.
- Explain the risks better.
- Improve the errors rasied by Macro when failing.
- Removed `security.allow-outbound-http`, as it is superseded by this setting and is somewhat misleading.
- We still make outbound HTTP requests to OAuth.
- We still make outbound HTTP requests for repositories.
From a technical perspective:
- Separate URIs that are safe to link to or redirect to (basically, not "javascript://") from URIs that are safe to fetch (nothing in a private block).
- Add the default blacklist.
- Be more careful with response data in Macro fetching, and don't let the user see it if it isn't ultimately valid.
Additionally:
- I want to do this check before pulling repositories, but that's enough of a mess that it should go in a separate diff.
- The future implementation of T4190 needs to perform the fetch check.
Test Plan:
- Fetched a valid macro.
- Fetched a non-image, verified it didn't result in a viewable file.
- Fetched a private-ip-space image, got an error.
- Fetched a 404, got a useful-enough error without additional revealing response content (which is usually HTML anyway and not useful).
- Fetched a bad protocol, got an error.
- Linked to a local resource, a phriction page, a valid remote site, all worked.
- Linked to private IP space, which worked fine (we want to let you link and redierect to other private services, just not fetch them).
- Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6755
Differential Revision: https://secure.phabricator.com/D12136
Summary:
Fixes T7621. The engine selection code started out making sense, but didn't make as much sense by the time I was done with it.
Specifically, from the vanilla file upload, we may incorrectly try to write directly to the chunk storage engine. This is incorrect, and produces a confusing/bad error.
Make chunk storage engines explicit and don't try to do single-file one-shot writes to them.
Test Plan:
- Tried to upload a large file with vanilla uploader, got better error message.
- Uploaded small and large files with drag and drop.
- Viewed {nav Files > Help/Options}.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7621
Differential Revision: https://secure.phabricator.com/D12110
Summary:
Ref T7522. This is mostly useful in the cluster, but could be useful for external installs too.
If you want to import an instance into a test/dry-run state (in the cluster, to test an import; in the general case, to do something like test new hardware or configuration), you currently risk spamming users with a lot of duplicate notifications. In particular, if Phabricator tracks remotes, both instances will continue importing commits and sending email about them. Both instances will try to publish to mirrors, too, which could be bad news, and both instances will try to update linked services.
Instead, provide a flag to let an instance run in "silent mode", which disables all outbound messaging and data.
We need to remember to support this flag on any new outbound channels, but we add about one of those per year so I think that's reasonable.
Test Plan:
- Flipped config.
- Saw it void email, feed and mirroring.
- Didn't test SMS since it's not really in use yet and not convenient to test.
- (Can you think of any publishing I missed?)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7522
Differential Revision: https://secure.phabricator.com/D12109
Summary:
Ref T7149. We can simplify configuration somewhat by removing the upload limit setting, now that we support arbitrarily large files.
- Merge configuration documentation.
- Tell users to set things to at least 32MB. This is 8MB maximum one-shot file + 4x headroom. Chunk sizes are 4MB.
Test Plan:
- Faked all the setup warnings.
- Read documentation.
- Uploaded some files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12083
Summary:
Right now, if a daemon dies it can leave the setup warning around for like 10 minutes or something until we reap it.
Tighten the warning so we only care about actively running daemons.
Test Plan: Checked setup issues.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12088
Summary:
Fixes T5843. File storage engines use a very old "selector" mechanism which makes them difficult to extend.
This mechanism predates widespread use of `PhutilSymbolLoader` to discover available implementations at runtime. Runtime discovery has generally proven more flexible and easier to use than explicit selection (although it sometimes needs more UI to support it in cases where order or enabled/disabled flags can not be directly determined).
Use a modern runtime discovery mechanism instead of an explicit selector. This might break any installs which subclassed the `Selector`, but I believe almost no such installs exist, and they'll receive a meaningful exception upon upgrading (any custom engines will no longer implement all of the required methods).
Looking forward, this modernizes infrastructure to prepare for new "virtual" chunked-storage engines, with the eventual goal of supporting very large file uploads and data import into the Phacility cluster.
This uses D12051 to add UI to make it easier to understand the state of storage engines.
Test Plan:
Used new UI panel to assess storage engines:
{F336270}
- Uploaded a small file, saw it go to MySQL engine.
- Uploaded a larger file, saw it go to S3 engine.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5843
Differential Revision: https://secure.phabricator.com/D12053
Summary: Renames the method in PHUIObjectBoxView to match the new PHUIInfoView class.
Test Plan: grepped codebase. Went to Calendar and tried a new status.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12005
Summary: Somewhat easier to parse and present information, with ICONS.
Test Plan:
Rebuilt current view with new layout. Tested toggling on and off some of the entries.
{F327816}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11938
Summary: Fixes some UIExample UI issues, adds a new full-width setting for DocumentView
Test Plan:
Test UIExamples at desktop and mobile breakpoints
{F327446}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7431
Differential Revision: https://secure.phabricator.com/D11933
Summary:
Fixes T7422. We'll currently choose a "binary" charset with a "utf8_general_ci" collation on "sort" columns on older MySQL, which seems to be causing problems.
Choose "utf8" in this case instead.
(I attempted to simplify the logic, too, but that's the only actual change.)
Test Plan: Went back and forth with `--disable-utf8mb4` on `storage adjust`, but this is version dependent so I'm not 100% sure it's the right fix.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7422
Differential Revision: https://secure.phabricator.com/D11928
Summary: Fixes T7308. Multiple users have encountered confusion around how they should specify a set or list in JSON; provide examples.
Test Plan:
```
epriestley@orbital ~/dev/phabricator $ ./bin/config set files.image-mime-types true
Usage Exception: Config key 'files.image-mime-types' is of type 'set'. Specify it in JSON. For example:
./bin/config set '{"value1": true, "value2": true}'
epriestley@orbital ~/dev/phabricator $ ./bin/config set cluster.addresses true
Usage Exception: Config key 'cluster.addresses' is of type 'list<string>'. Specify it in JSON. For example:
./bin/config set '["a", "b", "c"]'
epriestley@orbital ~/dev/phabricator $
```
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7308
Differential Revision: https://secure.phabricator.com/D11925
Summary: Since this element isn't strictly about errors, re-label as info view instead.
Test Plan: Grepped for all callsites, tested UIExamples and a few other random pages.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11867
Summary: In D11722, a `getGroup()` method was added to all subclasses of `PhabricatorApplicationConfigOptions`, but no abstract method was added to the base class. This will fail if a custom `*ConfigOptions` class does not provide a `getGroup()` method, in which case `$group->getGroup()` (in `PhabricatorConfigListController`) will fatal.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11756
Summary: This diff moves the default monospace font from a Global Default config value to CSS. What this will allow is some flexibility in changing this font in other areas (like Diviner and DocumentView) without changing the defaults globally. However if the admin sets a config value or a user sets a config value, that value will trump all settings in the CSS files with an !important declaration in the page head.
Test Plan:
Currently tested:
- Setting no value
- Setting an admin value
- Setting a user value
Verify remarkup blocks in Differential, Diviner, Conpherence, and Diffusion look as expected.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11597
Summary: Moves the setting from Core to UI, also adds a link to the task for further instructions.
Test Plan: Load up config in sandbox, see new instructions.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4214
Differential Revision: https://secure.phabricator.com/D11900
Summary: See IRC. This check is somewhat misleading right now because it could arise from a mangled/broken Host header rather than a bad `phabricator.base-uri` configuration.
Test Plan: Faked this to trip, read all the text.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11894
Summary:
Fixes T7287. This trades off 4-byte character support for case insensitivity in these columns, which is a much better trade on the balance.
Also adds more warnings about old MySQL. Note that we already issue a warning when you run "storage adjust" (which I've made stronger) and already "strongly recommend" MySQL 5.5 or newer in the install documentation.
Test Plan:
- Ran `storage adjust --disable-utf8mb4` to go to old definitions, then ran `storage adjust` to get back to the new ones. Everything seemed OK in both cases.
- Verified that utf8mb4 data can be migrated out of these colums with `--unsafe` (which will truncate).
- Verified that manual explains this.
- Faked my way into the setup warning.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7287
Differential Revision: https://secure.phabricator.com/D11893
Summary: Fixes T7165. Let users specify a file phid in config, and then use that file via an inline style tag. Also, cache the URI so that we don't have to query the file on every page load.
Test Plan: {F319050}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7165
Differential Revision: https://secure.phabricator.com/D11886
Summary: Ref T7352. This is pretty straightforward. I renamed `phd.start-taskmasters` to `phd.taskmasters` for clarity.
Test Plan:
- Ran `phd start`, `phd start --autoscale-reserve 0.25`, `phd restart --autoscale-reserve 0.25`, etc.
- Examined PID file to see options were passed.
- I'm defaulting this off (0 reserve) and making it a flag rather than an option because it's a very advanced feature which is probably not useful outside of instancing.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7352
Differential Revision: https://secure.phabricator.com/D11871
Summary:
Ref T6840. This feels a little dirty; open to alternate suggestions.
We currently have a race condition where multiple daemons may load a commit and then save it at the same time, when processing "reverts X" text. Prior to this feature, two daemons would never load a commit at the same time.
The "reverts X" load/save has no effect (doesn't change any object properties), but it will set the state back to the loaded state on save(). This overwrites any flag updates made to the commit in the meantime, and can produce the race in T6840.
In other cases (triggers, harbormaster, repositories) we deal with this kind of problem with "append-only-updates + single-consumer", or a bunch of locking. There isn't really a good place to add a single consumer for commits, since a lot of daemons need to access them. We could move the flags column to a separate table, but this feels pretty complicated. And locking is messy, also mostly because we have so many consumers.
Just exempting this column (which has unusual behavior) from `save()` feels OK-ish? I don't know if we'll have other use cases for this, and I like it even less if we never do, but this patch is pretty small and feels fairly understandable (that said, I also don't like that it can make some properties just silently not update if you aren't on the lookout).
So, this is //a// fix, and feels simplest/least-bad for the moment to me, I thiiink.
Test Plan: Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6840
Differential Revision: https://secure.phabricator.com/D11822
Summary:
Ref T4340. The attack this prevents is:
- An adversary penetrates your network. They acquire one of two capabilities:
- Your server is either configured to accept both HTTP and HTTPS, and they acquire the capability to observe HTTP traffic.
- Or your server is configured to accept only HTTPS, and they acquire the capability to control DNS or routing. In this case, they start a proxy server to expose your secure service over HTTP.
- They send you a link to `http://secure.service.com` (note HTTP, not HTTPS!)
- You click it since everything looks fine and the domain is correct, not noticing that the "s" is missing.
- They read your traffic.
This is similar to attacks where `https://good.service.com` is proxied to `https://good.sorvace.com` (i.e., a similar looking domain), but can be more dangerous -- for example, the browser will send (non-SSL-only) cookies and the attacker can write cookies.
This header instructs browsers that they can never access the site over HTTP and must always use HTTPS, defusing this class of attack.
Test Plan:
- Configured HTTPS locally.
- Accessed site over HTTP (got application redirect) and HTTPS.
- Enabled HSTS.
- Accessed site over HTTPS (to set HSTS).
- Tore down HTTPS part of the server and tried to load the site over HTTP. Browser refused to load "http://" and automatically tried to load "https://". In another browser which had not received the "HSTS" header, loading over HTTP worked fine.
- Brought the HTTPS server back up, things worked fine.
- Turned off the HSTS config setting.
- Loaded a page (to set HSTS with expires 0, diabling it).
- Tore down the HTTPS part of the server again.
- Tried to load HTTP.
- Now it worked.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D11820
Summary:
Ref T7185. These settings shouldn't be unlocked anywhere. Specifically:
- `reply-handler`: These are on the way out.
- `reply-handler-domain`: Also hopefully on the way out; locked because a compromised administrator account can redirect replies.
- `phabricator.cookie-prefix`: Not dangerous per se, but an admin could have a hard time fixing this if they changed it by accident since their session would become invalid immediately.
Test Plan: Browsed Config.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7185
Differential Revision: https://secure.phabricator.com/D11764
Summary:
Ref T7185. We currently have "locked", "masked", and "hidden" config.
However, "masked" does not really do anything. It was intended to mask values in DarkConsole, but Config got built out instead and "hidden" is strictly better in modern usage and protects against compromised administrator accounts. "hidden" implies "locked", so it's now strictly more powerful than just locked.
Remove "masked" and upgrade all "masked" config to "hidden". In particular, this hides some API keys and secret keys much more aggressively in Config, which is desirable.
Test Plan: Browsed things like S3 API keys in config and could no longer see them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7185
Differential Revision: https://secure.phabricator.com/D11763
Summary: Pretty sure this was me derping, not trying to make a joke.
Test Plan: New text makes sense.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11762
Summary: Fixes T7088. Mainly this updates the documentation but I also snuck in tweaking how the domain reply handler is built. This does two main things -- makes the behavior consistent as some applications who didn't override this behavior would send out emails with reply tos AND makes it easier for us to deprecate the custom domain thing on a per application basis, which is just silly. On that note, the main documentation doesn't get into how this can be overridden, though I left in that mini blurb on the config setting itself. We could deprecate this harder and LOCK things if you want as well.
Test Plan: read docs, looked good. reasoned through re-factor
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7088
Differential Revision: https://secure.phabricator.com/D11725
Summary:
Ref T7152. Ref T1139. This updates Phabricator so third-party libraries can translate their own stuff. Also:
- Hide "All Caps" when not in development mode, since some users have found this a little confusing.
- With other changes, adds a "Raw Strings" mode (development mode only).
- Add an example silly translation to make sure the serious business flag works.
- Add a basic British English translation.
- Simplify handling of translation overrides.
Test Plan:
- Flipped serious business / development on and off and saw silly/development translations drop off.
- Switched to "All Caps" and saw all caps.
- Switched to Very English, Wow!
- Switched to British english and saw "colour".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7152, T1139
Differential Revision: https://secure.phabricator.com/D11747
Summary:
Ref T7184. I managed to write a phantom setup issue which fails normally and succeeds when looked at carefully, so clicking "you have open issues..." always cleared them. This made it very difficult to figure out what the problem was.
Show issue keys in the "title" attribute to make this sort of thing easier to deal with.
Test Plan: Moused over "You have issues..." text, saw issue key, quickly fixed issue with new information.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7184
Differential Revision: https://secure.phabricator.com/D11743
Summary: Groups setup issues into Important, PHP, MySQL, and Base for easier parsing on initial installations.
Test Plan:
Test my internal server and various issues.
{F289699}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7207
Differential Revision: https://secure.phabricator.com/D11726
Summary: Adds core and apps grouping to configuration options, makes it somewhat easier to browse config options.
Test Plan: Set each option, review list. Breakdown is nearly 50/50 apps/core.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11722
Summary:
I'm hitting this in the cluster and couldn't figure it out after staring at it for a couple minutes. Produce a better error.
This dumps a hash of each configuration key value which is set to a non-default value into the daemon log. This is much more compact than the full config, and doesn't spread secrets around, so it seems like a good balance between providing information and going crazy with it.
Test Plan: {F284139}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11699
Summary:
Fixes T2380. Fixes T2382. Users should really configure this, but when we had a warning before a lot of users had trouble with it.
- Tout performance benefits.
- Document easy setup via CDN.
- We have an "Ignore" button now for users who really don't care.
Test Plan:
- Set up `admin.phacility.com` through AWS CloudFront (need a few changes to handle instances to put it on the cluster in general).
- Set up `secure.phabricator.com` through CloudFlare (almost; waiting for DNS).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T2382, T2380
Differential Revision: https://secure.phabricator.com/D11649
Summary: Minor, adds border, reduces greys, etc.
Test Plan:
View a number of config issues, see new colors.
{F282035}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11650
Summary: This sets an icon for each config, makes it easier to scan.
Test Plan:
Reload Config page, see all new icons
{F281089}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11619
Summary: Clean up the error view styling.
Test Plan:
Tested as many as I could find, built additional tests in UIExamples
{F280452}
{F280453}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11605
Summary:
Hit this locally, with an error like:
> Version <empty string> is older than 1.9, the minimum supported version.
(Where `<empty string>` was just the empty string, not literally the text `<empty string>`.)
Be more careful about parsing versions, and parse the newer string.
Test Plan: Got "unknown version" with intentionally-broken test data, then clean readout.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11558
Summary: Fixes T7034. Like HTTP, proxy requests to the correct host if a repository has an Almanac service host.
Test Plan: Ran VCS requests through the proxy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7034
Differential Revision: https://secure.phabricator.com/D11543
Summary: Add a setBorder call to CrumbsView to be more deliberate when a border is drawn. Could not find any CSS hacks to set it conditionally CSS.
Test Plan: Browsed every application that called crumbs and make a design decision. Also fixed a few bad layouts.
Reviewers: btrahan, epriestley
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11533
Summary:
Fixes T7019. In a cluster environment, pushes currently fail because the commit hook can't identify the instance.
For web processes, the hostname identifies the instance -- but we don't have a hostname in the hook.
For CLI processes, the environment identifies the instance -- but we don't have an environment in the hook under SVN.
Promote the instance identifier into the upstream and pack/unpack it explicitly for hooks. This is probably not useful for anyone but us, but the amount of special-purpose code we're introducing is very small.
I poked at trying to do this in a more general way, but:
- We MUST know this BEFORE we run code, so the normal subclassing stuff is useless.
- I couldn't come up with any other parameter which might ever be useful to pass in.
Test Plan: Used `git push` to push code through proxied HTTP, got a clean push.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7019
Differential Revision: https://secure.phabricator.com/D11495
Summary: Adds in the sidenav
Test Plan: Click on sidenav, see it persist
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11526
Summary: Select a similar or better FontAwesome icon to represent each application
Test Plan: Visual inspection
Reviewers: epriestley, btrahan
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11489
Summary: Ref T6822. This method needs to be `public` because it is called from `PhabricatorApplicationSearchController::buildApplicationMenu()`.
Test Plan: I wouldn't expect //increasing// method visibility to break anything.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11416
Summary: Fixes T6959.
Test Plan: When I was ready to test the feature, the "Daemon & Web config" error already showed up, from having added phd.variant-config. I went meta and changed the value of phd.variant-config to have phd.variant-config. The config error disappeared. I then changed the conpherence setting about conpherence email prefix and the error showed up again. Removing the conpherence config setting made the error disappear once more.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6959
Differential Revision: https://secure.phabricator.com/D11399
Summary: Fixes T6923. Turns out we can't use the editor since we don't have a user with a phid (just some omnipotent guy).
Test Plan: ./bin/config set --database syntax.filemap '{}'; ./bin/config delete --database syntax.filemap
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6923
Differential Revision: https://secure.phabricator.com/D11301
Summary:
Fixes T6559. No more flash, use Websockets. This is less aggressive than the earlier version, and retains more server logic.
- Support "wss".
- Make the client work.
- Remove "notification.user" entirely.
- Seems ok?
Test Plan:
In Safari, Firefox and Chrome, saw the browsers connect. Made a bunch of comments/updates and saw notifications.
Notable holes in the test plan:
- Haven't tested "wss" yet. I'll do this on secure.
- Notifications are //too fast// now, locally. I get them after I hit submit but before the page reloads.
- There are probably some other rough edges, this is a fairly big patch.
Reviewers: joshuaspence, btrahan
Reviewed By: joshuaspence, btrahan
Subscribers: fabe, btrahan, epriestley
Maniphest Tasks: T6713, T6559
Differential Revision: https://secure.phabricator.com/D11143
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within `PhabricatorController` subclasses.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11241
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within the `PhabricatorConfigStorageSchema` class.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11247
Summary:
Ref T2783. Ref T6706.
- Add `cluster.addresses`. This is a whitelist of CIDR blocks which define cluster hosts.
- When we recieve a request that has a cluster-based authentication token, require the cluster to be configured and require the remote address to be a cluster member before we accept it.
- This provides a general layer of security for these mechanisms.
- In particular, it means they do not work by default on unconfigured hosts.
- When cluster addresses are configured, and we receive a request //to// an address not on the list, reject it.
- This provides a general layer of security for getting the Ops side of cluster configuration correct.
- If cluster nodes have public IPs and are listening on them, we'll reject requests.
- Basically, this means that any requests which bypass the LB get rejected.
Test Plan:
- With addresses not configured, tried to make requests; rejected for using a cluster auth mechanism.
- With addresses configred wrong, tried to make requests; rejected for sending from (or to) an address outside of the cluster.
- With addresses configured correctly, made valid requests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6706, T2783
Differential Revision: https://secure.phabricator.com/D11159
Summary:
Fixes T6692. Addresses two main issues:
- The write guard would sometimes not get disposed of on exception pathways, generating an unnecessary secondary error which was just a symptom of the original root error.
- This was generally confusing and reduced the quality of reports we received because users would report the symptomatic error sometimes instead of the real error.
- Instead, reflow the handling so that we always dispose of the write guard if we create one.
- If we missed the Controller-level error page generation (normally, a nice page with full CSS, etc), we'd jump straight to Startup-level error page generation (very basic plain text).
- A large class of errors occur too early or too late to be handled by Controller-level pages, but many of these errors are not fundamental, and the plain text page is excessively severe.
- Provide a mid-level simple HTML error page for errors which can't get full CSS, but also aren't so fundamental that we have no recourse but plain text.
Test Plan:
Mid-level errors now produce an intentional-looking error page:
{F259885}
Verified that setup errors still render properly.
@chad, feel free to tweak the exception page -- I just did a rough pass on it. Like the setup error stuff, it doesn't have Celerity, so we can't use `{$colors}` and no other CSS will be loaded.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, chad
Maniphest Tasks: T6692
Differential Revision: https://secure.phabricator.com/D11126
Summary:
Removes an unused PhabricatorFeedStory Parameter from all getTitleForFeed() and getApplicationTransactionTitleForFeed() functions.
Ref D11088 Ref T6545
Test Plan: ran all unit tests and viewed some dashboard feeds
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6545
Differential Revision: https://secure.phabricator.com/D11146
Summary:
Shows a timeline of all modified settings Fixes T6545
Will show all settings (no pagination, should be not so difficult to add if needed but most installs won't have hundreds of settings changes)
I'm not happy by how the PhabricatorConfigTransaction object is instructed to render the config keys but i don't see any other reasonable way.
We could always show the keys though.
Test Plan: Changed settings and called the history page
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6545
Differential Revision: https://secure.phabricator.com/D11088
Summary:
Ref T2783. When repository services are defined, skip the check for local repository storage.
In particular, in the Phacility cluster, this path won't exist on web nodes, but nothing will ever try to access it.
Test Plan: Verified new branch gets hit with cluster services defined.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11100
Summary:
Fixes T5196
If no phd.user is configured the behaviour is unchanged besides printing a warning when run as root (Usually i would add an exit(1) here but that would break existing installs who do that).
If phd.user is set and the current user is root it will run the daemon as: su USER -c "command" (I'm not sure if this works for every platform needed)
Otherwise it will refuse to start if configured and current user mismatch.
Test Plan: Stopped & Started phd daemon with various users and different phd.user settings including root
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: vinzent, epriestley
Maniphest Tasks: T5196
Differential Revision: https://secure.phabricator.com/D11036
Summary:
When the index does not exist and auto_create_index isn't
enabled, running ./bin/index results in a failure. That's
T5990
Instead create an index properly. This also allows us to do
nice things like do a proper mapping and analysis like for
substring matching like outlined by @fabe in T6552.
Test Plan:
Deleted and created index multiple times to verify
proper index creation and usage.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, manybubbles, chasemp, fabe, epriestley
Differential Revision: https://secure.phabricator.com/D10955
Summary: Ref: T6747 add a setup warning if mysql is configured with the default boolean syntax
Test Plan: Run setup checks with default mysql config and then add the desired config option, restart and check again.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T6747
Differential Revision: https://secure.phabricator.com/D11030
Summary:
Two goals:
- If an attacker compromises an administrator account (without compromising the host itself), they can currently take advantage of vulnerabilities in prototype applications by enabling the applications, then exploiting the vulnerability. Locking this option requires CLI access to enable prototypes, so installs which do not have prototypes enabled have no exposure to security issues in prototype applications.
- Making this very slightly harder to enable is probably a good thing, given the state of the world and support.
Test Plan: Verified that web UI shows the value is locked and instructs the user to update via the CLI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10993
Summary: Fixes T6693.
Test Plan:
Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!
Successfully "showed older changes" in Maniphest too.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6693
Differential Revision: https://secure.phabricator.com/D10931
Summary: Ref T4712. These are a bit time consuming to test so might as well send off a batch now and again.
Test Plan: foreach impacted controller, made sure the timeline rendered as it did before. for project column and config, noted the "should terminate" UI was also rendered unlike before.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10923
Summary: Ref T1217, Add link to email preferences to email template
Test Plan: Add comment to object like Maniphest task, check that email has a footer with a link to email preferences.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T1217
Differential Revision: https://secure.phabricator.com/D10883
Summary: Fixes T6243.
Test Plan: simulated having this on by adding 'ONLY_FULL_GROUP_BY' to the $modes array. verified the error message looked nice {F233954}.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6243
Differential Revision: https://secure.phabricator.com/D10856
Summary: Fixes T3046. Git has no minimum or bad versions; svn has no minimum and the bad version of 1.7.1, and mercurial has a minimum of 1.9 and bad versions 2.2 and 2.1. These error messages are specific about the problem but do not include the details as to what specifically is bad about a given version. I don't think that's really necessary - the details don't help solve the problem and its probably booooooring at best to the average user. The details about a bad version are included in the code however.
Test Plan: hardcoded a VCS array to let me test all the VCS stuff. added some phlog() calls to make sure the VCS version parsing stuff was working correctly relative to the version(s) on my system. played around with setting minimum versions and bad versions and saw good results depending on what I set.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3046
Differential Revision: https://secure.phabricator.com/D10852
Summary: Fixes T6175. This particular option is only editable via the Applications application so let the user know that.
Test Plan: saw error message and clicked through to applications app. tried another locked option and saw old message
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6175
Differential Revision: https://secure.phabricator.com/D10841
