Summary:
We want users to use Ponder as primary support vector.
T15084
Test Plan: Generated diviner, saw that the reference works correctly.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25125
Summary:
Note that the git version 2.5.0 is surely already assumed
as required as implicit fact, so it's just better to clarify
this in the documentation to prevent weird situations and
unclear related exceptions.
Note that if you have seen this Phabricator/Phorge error
in your installation:
This path was a submodule at R123:5378198ea123asdlol
This is probably that kind of weird errors that will be
fixed just updating your git version to 2.5.0, so that
the git escape sequence ('--') is correctly parsed by git.
Example of command that will fail with outdated git versions:
git cat-file -t -- <hash>:<file>
If you do not want to update git, but you want to fix,
you can convert that command to this one, manually patching
your installation:
git cat-file -t <hash>:<file>
Related material:
- https://we.phorge.it/T15179
- https://unix.stackexchange.com/a/740621/85666
- 6f9504c48e/Documentation/RelNotes/2.5.0.txt
Wontfix T15179
Test Plan:
- open Diviner page "Installation Guide"
- see that the git version is not explicit - yup!
Reviewers: O1 Blessed Committers, Cigaryno, avivey
Reviewed By: O1 Blessed Committers, Cigaryno, avivey
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15179
Differential Revision: https://we.phorge.it/D25081
Test Plan:
- look at Diviner and say "Java..."
- Diviner will look at you saying "...Script"
- you land, satisfied
If Diviner says "...script" instead (lowercase "S"), abandon the ship.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25075
Summary:
Add some small Diviner documentation details like:
* mention PHP 8.1 (thanks https://we.phorge.it/Q18)
* mention the fact that you need multiple databases (it may be not obvious from the startup)
* mention MariaDB and not just MySQL (thanks MariaDB community!)
* mention "Administrator" and not just "User" for the "Instructions for installing, configuring, and using Phorge."
I was not bold enough to do more changes.
Test Plan: I tested locally and Diviner does not look bad.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25064
Summary:
Ref T5479. Ref T13658. This was a contributed application from the early days of Phabricator which never had customers or users in the wild. The contributor moved on from the project many years ago.
Any capabilities in this general role would look different today. It also has one or two product name literal strings, so this is as good a time as any to remove it.
This change does not remove storage; I'll issue upgrade guidance and do that separately after some time.
Test Plan: Grepped for "phragment", got no relevant hits.
Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658, T5479
Differential Revision: https://secure.phabricator.com/D21793
Summary:
Ref T9530. Ref T13658. The "Releeph" application was never useful outside of Facebook and any application providing release support would not resemble it much.
It has some product name literal strings, so now is as good a time as any to get rid of it.
This application never left prototype and I'm not aware of any install in the wild that uses it (or has ever used it).
I did not destroy the database itself. I'll issue upgrade guidance and destroy the database in some future release, just in case.
Test Plan: Grepped for "releeph", found no relevant/removable hits.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658, T9530
Differential Revision: https://secure.phabricator.com/D21792
Summary:
Ref T13669. See that task for discussion. Based on a disheartening interaction in June 2020, discourage use of Mailgun.
Also update the default Postmark inbound address list, since they've added a new address.
Test Plan: Read documentation, grepped for "mailgun".
Maniphest Tasks: T13669
Differential Revision: https://secure.phabricator.com/D21738
Summary:
Ref T13658. This just scrubs some of the simple references from the codebase.
Most of what's left is in documentation which won't be relevant for a fork and/or which I need to separately revise (or more-or-less delete) at some point anyway.
I removed the "install RHEL" and "install Ubuntu" scripts outright since I don't have any reasonable way to test them and don't plan to maintain them.
Test Plan: Grepped for "phacility", "epriestley"; ran unit tests.
Reviewers: cspeckmim
Reviewed By: cspeckmim
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21678
Summary: Ref PHI2071. This path is incorrect; the correct path is `local.json`.
Test Plan: Looked in my `conf/local/` directory.
Differential Revision: https://secure.phabricator.com/D21663
Summary: This makes the whole setup easier, future proof and reproducible.
Test Plan:
cd support/aphlict/server/
npm install
See that ws gets installed as expected.
Reviewers: O1 Blessed Committers, Matthew
Reviewed By: O1 Blessed Committers, Matthew
Subscribers: Matthew, Ekubischta, speck, tobiaswiese
Maniphest Tasks: T15019
Differential Revision: https://we.phorge.it/D25006
Summary: This commit also removes references to support pacts and updates links to point to the new upstream.
Test Plan: Generated Diviner documentation on a local install and verified that the changes look good.
Reviewers: O1 Blessed Committers, chris
Reviewed By: O1 Blessed Committers, chris
Subscribers: chris, speck, tobiaswiese
Maniphest Tasks: T15012
Differential Revision: https://we.phorge.it/D25007
Summary: Ref T13614. Provide "bin/repository lock" to temporarily lock repositories for manual maintenance.
Test Plan:
- Read instructions.
- Used `bin/repository lock` according to the instructions.
- Saw Storage tab in Diffusion report lock held during maintenance, released after it completes.
- Saw "maintenance" push log generated and repository version bump.
- Tried to lock some invalid repositories.
Maniphest Tasks: T13614
Differential Revision: https://secure.phabricator.com/D21671
Summary:
Ref T13395. "libphutil/" was stripped for parts, but some documentation still references it. This is mostly minor corrections, but:
- Removes "Javelin at Facebook", long obsolete.
- Removes "php FPM warmup", which was always a prototype and is obsoleted by PHP preloading in recent PHP.
Test Plan: `grep` / reading
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D21624
Summary:
Ref T13395. Libphutil has merged into Arcanist and no longer needs to be installed or upgraded. Additionally:
- The minimum PHP version is now PHP 5.5.
- Although older versions of PHP should still install APC, modern versions come with Opcache and do not need APC. Setup issues guide administrators thorugh the correct install procedure now.
Test Plan: Read documentation.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D21550
Summary: Ref T13591. Provide some guidance on the most common cases for wanting to interact with the worker queue.
Test Plan: Read documentation.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21536
Summary:
Ref T13570. Fixes T13235. In most cases, we use modern (v4) signatures for almost all AWS API calls, and have for several years.
However, sending email via SES currently uses an older piece of external code which uses the older (v3) signature method.
AWS is retiring v3 signatures on October 1 2020, so this pathway will stop working.
Update the pathway to use `PhutilAWSFuture`, which provides v4 signatures.
T13235 discusses poor error messages from SES. Switching to Futures fixes this for free, as they have more useful error handling.
Test Plan:
- Configured an SES mailer, including the new `region` parameter.
- Used `bin/mail send-test` to send mail via SES.
- Sent invalid mail (from an unverified address); got a more useful error message.
- Grepped for removed external, no hits.
Maniphest Tasks: T13570, T13235
Differential Revision: https://secure.phabricator.com/D21461
Summary: Ref T13515. External editor URIs currently depend on repositories having callsigns, but callsigns are no longer required. Add some variables to support configuring this feature for repositories that do not have callsigns.
Test Plan: Changed settings to use new variables, saw links generate appropriately.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21147
Summary:
Ref T13515.
- Previously valid editor URIs may become invalid without being changed (if an administrator removes a protocol from the list, for example), but this isn't explained very well. Show an error on the settings page if the current value isn't usable.
- Generate a list of functions from an authority in the parser.
- Generate a list of protocols from configuration.
Test Plan: {F7370872}
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21146
Summary:
Ref T13515. It's not intuitive that these settings are "Display Preferences", even thought they're intenrally related to some of the other display preferences.
Give them a separate group.
Test Plan: {F7370500}
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21140
Summary:
See <https://hackerone.com/reports/850114>.
An attacker with administrator privileges can configure "notification.servers" to connect to internal services, either directly or with chosen parameters by selecting an attacker-controlled service and having it issue a "Location" redirect.
Generally, we allow this attack to occur. The same administrator can use an authentication provider or a VCS repository to perform the same attack, and we can't reasonably harden these workflows without breaking things that users expect to be able to do.
There's no reason this particular variation of the attack needs to be allowable, though, and the current behavior isn't consistent with how other similar things work.
- Hide the "notification.servers" configuration, which also locks it. This is similar to other modern service/server configuration.
- Don't follow redirects on these requests. Aphlict should never issue a "Location" header, so if we encounter one something is misconfigured. Declining to follow this header likely makes the issue easier to debug.
Test Plan:
- Viewed configuration in web UI.
- Configured a server that "Location: ..." redirects, got a followed redirect before and a failure afterward.
{F7365973}
Differential Revision: https://secure.phabricator.com/D21123
Summary: Ref T13444. This is an ancient event and part of the old event system. It is not likely to be in use anymore, and repository identities should generally replace it nowadays anyway.
Test Plan: Grepped for constant and related methods, no longer found any hits.
Maniphest Tasks: T13444
Differential Revision: https://secure.phabricator.com/D20909
Summary: Ref T13410. Fixes T4280. Allows you to put a named anchor into a document explicitly.
Test Plan: Used `{anchor ...}` in Remarkup, used location bar to jump to anchors.
Maniphest Tasks: T13410, T4280
Differential Revision: https://secure.phabricator.com/D20825
Summary: This psueudocode should use the result of computation at the end.
Test Plan: Read carefully.
Differential Revision: https://secure.phabricator.com/D20813
Summary:
Ref T13411. This cleans up policy name rendering. We ultimately render into three contexts:
- Plain text contexts, like `bin/policy show`.
- Transaction contexts, where we're showing a policy change. In these cases, we link some policies (like project policies and custom policies) but the links go directly to the relevant object or a minimal explanation of the change. We don't link policies like "All Users".
- Capability contexts, where we're describing a capability, like "Can Push" or cases in Applicaitons. In these cases, we link all policies to the full policy explanation flow.
Test Plan:
- Used `bin/policy show` to examine the policy of an object with a project policy, no longer saw HTML.
- Viewed the transaction logs of Applications (ModularTransactions) and Tasks (not ModularTransactions) with policy edits, including project and custom policies.
- Clicked "Custom Policy" in both logs, got consistent dialogs.
- Viewed application detail pages, saw all capabities linked to explanatory capability dialogs. The value of having this dialog is that the user can get a full explanation of special rules even if the policy is something mundane like "All Users".
Maniphest Tasks: T13411
Differential Revision: https://secure.phabricator.com/D20804
Summary:
Fixes T13392. If you have 17 load balancers in sequence, Phabricator will receive requests with at least 17 "X-Forwarded-For" components in the header.
We want to select the 17th-from-last element, since prior elements are not trustworthy.
This currently isn't very easy/obvious, and you have to add a kind of sketchy piece of custom code to `preamble.php` to do any "X-Forwarded-For" parsing. Make handling this correctly easier.
Test Plan:
- Ran unit tests.
- Configured my local `preamble.php` to call `preamble_trust_x_forwarded_for_header(4)`, then made `/debug/` dump the header and the final value of `REMOTE_ADDR`.
```
$ curl http://local.phacility.com/debug/
<pre>
HTTP_X_FORWARDED_FOR =
FINAL REMOTE_ADDR = 127.0.0.1
</pre>
```
```
$ curl -H 'X-Forwarded-For: 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/
<pre>
HTTP_X_FORWARDED_FOR = 1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4, 5.5.5.5, 6.6.6.6
FINAL REMOTE_ADDR = 3.3.3.3
</pre>
```
```
$ curl -H 'X-Forwarded-For: 5.5.5.5, 6.6.6.6' http://local.phacility.com/debug/
<pre>
HTTP_X_FORWARDED_FOR = 5.5.5.5, 6.6.6.6
FINAL REMOTE_ADDR = 5.5.5.5
</pre>
```
Maniphest Tasks: T13392
Differential Revision: https://secure.phabricator.com/D20785
Summary:
Fixes T13382. Depends on D20724. These ancient scripts are no longer necessary since we've had a smooth web-based onboarding process for a long time.
I retained `bin/user empower` and `bin/user enable` for recovering from situations where you accidentally delete or disable all administrators. This is normally difficult, but some users are industrious.
Test Plan: Grepped for `accountadmin` and `add_user.php`, found no more hits.
Maniphest Tasks: T13382
Differential Revision: https://secure.phabricator.com/D20725
Summary:
Ref T13382.
- Remove "bin/people profileimage" which previously generated profile image caches but now feels obsolete.
- Replace it with "bin/user", with "enable" and "empower" flows. This command is now focused on regaining access to an install after you lock your keys inside.
- Document the various ways to unlock objects and accounts from the CLI.
Test Plan:
- Ran `bin/user enable` and `bin/user empower` with various flags.
- Grepped for `people profileimage` and found no references.
- Grepped for `bin/people` and found no references.
- Read documentation.
Maniphest Tasks: T13382
Differential Revision: https://secure.phabricator.com/D20724
Summary: See PHI1392. This flag is `--all`, not `--all-caches`.
Test Plan: Ran `bin/cache purge --all`.
Differential Revision: https://secure.phabricator.com/D20722
Summary:
Fixes T8830. Fixes T13364.
- The inability to destroy objects from the web UI is intentional. Make this clear in the messaging, which is somewhat out of date and partly reflects an earlier era when things could be destroyed.
- `bin/remove destroy` can't rewind time. Document expectations around the "put the cat back in the bag" use case.
Test Plan: Read documentation, clicked through both workflows.
Maniphest Tasks: T13364, T8830
Differential Revision: https://secure.phabricator.com/D20694
Summary:
Fixes T13356. This option is supported and works fine, it just isn't documented.
Add documentation and fix the config option to actually link to it to make life a little easier.
Test Plan: Read documentation.
Maniphest Tasks: T13356
Differential Revision: https://secure.phabricator.com/D20691
Summary:
Fixes T13304. Shell pipes and redirects do not have robust behavior when errors occur. We provide "--compress" and "--output" flags as robust alternatives, but do not currently recommend their use.
- Recommend their use, since their error handling behavior is more robust in the face of issues like full disks.
- If "--compress" is provided but won't work because the "zlib" extension is missing, raise an explicit error. I believe this extension is very common and this error should be rare. If that turns out to be untrue, we could take another look at this.
- Also, verify some flag usage sooner so we can exit with an error faster if you mistype a "bin/storage dump" command.
Test Plan: Read documentation, hit affected error cases, did a dump and spot-checked that it came out sane looking.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13304
Differential Revision: https://secure.phabricator.com/D20572
Summary:
Depends on D20433. Ref T13277. Since "Autoclose" no longer exists, update the documentation.
Currently, this documentation focuses a lot on troubleshooting because users historically had a lot of trouble with figuring out why things were or were not autoclosing. I haven't seen any real confusion about this in years, so I suspect we may have improved the import pipeline and/or UI to make this less of a problem.
It's also possible that this document "fixed" the problem, but usually I expect a documentation fix to not affect the frequency of reports, just make them easier to resolve, so I doubt it.
If unclear things remain //and// documentation really did fix it, maybe we can fix the issues. Or we can just put the troubleshooting documentation back.
Test Plan: Read documentation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13277
Differential Revision: https://secure.phabricator.com/D20434
Summary:
Fixes T13265. See that task for discussion. Briefly:
- For mailers that use other mailers (SMTP, Sendmail), optionally let administrators set `"message-id": false` to improve threading behavior if their local Postfix is ultimately sending through SES or some other mailer which will replace the "Message-ID" header.
Also:
- Postmark is currently marked as supporting "Message-ID", but it does not actually support "Message-ID" on `secure.phabricator.com` (mail arrives with a non-Phabricator message ID). I suspect this was just an oversight in building or refactoring the adapter; correct it.
- Remove the "encoding" parameter from "sendmail". It think this was just missed in the cleanup a couple months ago; it is no longer used or documented.
Test Plan: Added and ran unit tests. (These feel like overkill, but this is super hard to test on real code.) See T13265 for evidence that this overall approach improves behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13265
Differential Revision: https://secure.phabricator.com/D20285
Summary:
Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely.
I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction.
Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T3498
Differential Revision: https://secure.phabricator.com/D20185
Summary:
Ref T13249. See <https://discourse.phabricator-community.org/t/configuring-the-number-of-taskmaster-daemons/2394/>.
Today, when a configuration value is "locked", we prevent //writes// to the database. However, we still perform reads. When you upgrade, we generally don't want a bunch of your configuration to change by surprise.
Some day, I'd like to stop reading locked configuration from the database. This would defuse an escalation where an attacker finds a way to write to locked configuration despite safeguards, e.g. through SQL injection or policy bypass. Today, they could write to `cluster.mailers` or similar and substantially escalate access. A better behavior would be to ignore database values for `cluster.mailers` and other locked config, so that these impermissible writes have no effect.
Doing this today would break a lot of installs, but we can warn them about it now and then make the change at a later date.
Test Plan:
- Forced a `phd.taskmasters` config value into the database.
- Saw setup warning.
- Used `bin/config delete --database phd.taskmasters` to clear the warning.
- Reviewed documentation changes.
- Reviewed `phd.taskmasters` documentation adjustment.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20159
Summary:
Depends on D20126. See PHI1056. Ref T13244.
- `bin/audit delete` destroys audit requests, but does not update the overall audit state for associated commits. For example, if you destroy all audit requests for a commit, it does not move to "No Audit Required".
- `bin/audit synchronize` does this synchronize step, but is poorly documented.
Make `bin/audit delete` synchronize affected commits.
Document `bin/audit synchronize` better.
There's some reasonable argument that `bin/audit synchronize` perhaps shouldn't exist, but it does let you recover from an accidentally (or intentionally) mangled database state. For now, let it live.
Test Plan:
- Ran `bin/audit delete`, saw audits destroyed and affected commits synchornized.
- Ran `bin/audit synchronize`, saw behavior unchanged.
- Ran `bin/audit help`, got better help.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20127
Summary:
Ref T6703. Currently, when you create an account on a new install, we prompt you to select a password.
You can't actually use that password unless you set up a password provider, and that password can't be associated with a provider since a password provider won't exist yet.
Instead, just don't ask for a password: create an account with a username and an email address only. Setup guidance points you toward Auth.
If you lose the session, you can send yourself an email link (if email works yet) or `bin/auth recover` it. This isn't really much different than the pre-change behavior, since you can't use the password you set anyway until you configure password auth.
This also makes fixing T9512 more important, which I'll do in a followup. I also plan to add slightly better guideposts toward Auth.
Test Plan: Hit first-time setup, created an account.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: revi
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20111
Summary:
Depends on D20129. Ref T13244. See PHI1058. When a revision has an "Accept" from a package, count the owners as "involved" in the change whether or not any actual human owners are actually accepting reviewers.
If a user owns "/" and uses "force accept" to cause "/src/javascript" to accept, or a user who legitimately owns "/src/javascript" accepts on behalf of the package but not on behalf of themselves (for whatever reason), it generally makes practical sense that these changes have owners involved in them (i.e., that's what a normal user would expect in both cases) and don't need to trigger audits under "no involvement" rules.
Test Plan: Used `bin/repository reparse --force --owners <commit>` to trigger audit logic. Saw a commit owned by `O1` with a revision counted as "involved" when `O1` had accepted the revision, even though no actual human owner had accepted it.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20130
Summary:
Depends on D20124. Ref T13244. See PHI1055. Add a few more builtin audit behaviors to make Owners more flexible.
(At the upper end of flexibility you can trigger audits in a very granular way with Herald, but you tend to need to write one rule per Owners package, and providing a middle ground here has worked reasonably well for "review" rules so far.)
Test Plan:
- Edited a package to select the various different audit rules.
- Used `bin/repository reparse --force --owners <commit>` to trigger package audits under varied conditions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20126
Summary: This option no longer needs to be configured if you configure inbound mail (and that's the easiest setup approach in a lot of cases), so stop telling users they have to set it up.
Test Plan: Read documentation and configuration help.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20104
Summary: See PHI1050. Although Diviner hasn't received a ton of new development for a while, it's: not exaclty new; and pretty useful for what we need it for.
Test Plan: Reading.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: leoluk
Differential Revision: https://secure.phabricator.com/D20086
Summary: Depends on D20038. Ref T13231. Although I planned to keep this out of the upstream (see T13229) it ended up having enough pieces that I imagine it may need more fixes/updates than we can reasonably manage by copy/pasting stuff around. Until T5055, we don't really have good tools for managing this. Make my life easier by just upstreaming this.
Test Plan: See T13231 for a bunch of workflow discussion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13231
Differential Revision: https://secure.phabricator.com/D20039
