Summary:
Ref T13404. Except for one known issue in Multimeter, Phabricator appears to function properly in this mode. It is broadly desirable that we run in this mode; it's good on its own, and enabled by default in at least some recent MySQL.
Additionally, "ONLY_FULL_GROUP_BY" and "STRICT_ALL_TABLES" shared a setup key, so ignoring one would ignore both. Change the key so that existing ignores on "ONLY_FULL_GROUP_BY" do not mask "STRICT_ALL_TABLES" warnings.
Test Plan: Grepped for `ONLY_FULL_GROUP_BY`.
Maniphest Tasks: T13404
Differential Revision: https://secure.phabricator.com/D20791
Summary: Fixes T13406. On the logout screen, test for no configured providers and warn users they may be getting into more trouble than they expect.
Test Plan:
- Logged out of a normal install and a fresh (unconfigured) install.
{F6847659}
Maniphest Tasks: T13406
Differential Revision: https://secure.phabricator.com/D20789
Summary:
See D20779, https://discourse.phabricator-community.org/t/3089. `bin/config set` complains about
missing config file as if it's un-writable.
Test Plan: run `bin/config set` with missing, writable, unwritable conf.json and parent dir.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20788
Summary: Fixes T13405. The default behavior of the global search bar isn't currently configurable, but can be made configurable fairly easily.
Test Plan: Changed setting as an administrator, saw setting reflected as a user with no previous preference. As a user with an existing preference, saw preference retained.
Maniphest Tasks: T13405
Differential Revision: https://secure.phabricator.com/D20787
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:
Ref T13336. Currently, "bin/storage destroy" destroys every master. This is wonderfully destructive, but if replication fails it's useful to be able to destroy only a replica.
Operate on a single host, and require "--host" to target the operation in cluster mode, so `bin/storage destroy --host dbreplica001` is a useful operation.
Test Plan: Ran `bin/storage destroy` with various flags locally. Will destroy `secure002` and refresh replication.
Maniphest Tasks: T13336
Differential Revision: https://secure.phabricator.com/D20784
Summary: Ref T13366. The "authorities" mechanism was replaced, but I missed this callsite. Update it to use the request cache mechanism.
Test Plan: As a user without permission to view some initiatives, viewed a list of initiatives.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20783
Summary: Ref T13366. The "authorities" mechanism was replaced, but I missed this callsite. Update it to use the request cache mechanism.
Test Plan: As a user without permission to view some initiatives, viewed a list of initiatives.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20783
Summary: Ref T13404. This query is invalid under "sql_mode=ONLY_FULL_GROUP_BY". Rewrite it to avoid interacting with `actorIdentity` at all; this is a little more robust in the presence of weird data and not really more complicated.
Test Plan:
- Enabled "ONLY_FULL_GROUP_BY".
- Hit system actions (e.g., login).
- Before: error.
- After: clean login.
- Tried to login with a bad password many times in a row, got properly limited by the system action rate limiter.
Maniphest Tasks: T13404
Differential Revision: https://secure.phabricator.com/D20782
Summary:
Depends on D20780. Ref T13403. During initial setup, it's routine to run "bin/config" with a bad database config. We start the stack in "config optional" mode to anticipate this.
However, even in this mode, we may emit warnings if the connection fails in certain ways. These warnings aren't useful; suppress them with "@".
(Possibly this message should move from "phlog()" to "--trace" at some point, but it has a certain amount of context/history around it.)
Test Plan:
- Configured MySQL to fail with a retryable error, e.g. good host but bad port.
- Ran `bin/config set ...`.
- Before: saw retry warnings on stderr.
- After: no retry warnings on stderr.
- (Turned off suppression code artificially and verified warnings still appear under normal startup.)
Maniphest Tasks: T13403
Differential Revision: https://secure.phabricator.com/D20781
Summary: Depends on D20779. Ref T13403. Bad parameters may cause this call to fail without setting an error code; if it does, catch the issue and go down the normal connection error pathway.
Test Plan:
- With "mysql.port" set to "quack", ran `bin/storage probe`.
- Before: wild mess of warnings as the code continued below and failed when trying to interact with the connection.
- After: clean connection failure with a useful error message.
Maniphest Tasks: T13403
Differential Revision: https://secure.phabricator.com/D20780
Summary: Ref T13403. We currently emit a useful error message, but it's not tailored and has a stack trace. Since this is a relatively routine error and on the first-time-setup path, tailor it so it's a bit nicer.
Test Plan:
- Ran `bin/config set ...` with an unwritable "local.json".
- Ran `bin/config set ...` normally.
Maniphest Tasks: T13403
Differential Revision: https://secure.phabricator.com/D20779
Summary:
Ref T13286. The current (very safe / conservative) rules for retrying git reads generalize to git writes, so we can use the same ruleset in both cases.
Normally, writes converge rapidly to only having good nodes at the head of the list, so this has less impact than the similar change to reads, but it generally improves consistency and allows us to assert that writes which can be served will be served.
Test Plan:
- In a cluster with an up node and a down node, pushed changes.
- Saw a push to the down node fail, retry, and succeed.
- Did some pulls, saw appropriate retries and success.
- Note that once one write goes through, the node which received the write always ends up at the head of the writable list, so nodes need to be explicitly thawed to reproduce the failure/retry behavior.
Maniphest Tasks: T13286
Differential Revision: https://secure.phabricator.com/D20778
Summary: Ref T13286. When retrying a read request, keep retrying as long as we have canididate services. Since we consume a service with each attempt, there's no real reason to abort early, and trying every service allows reads to always succeed even if (for example) 8 nodes of a 16-node cluster are dead because of a severed network link between datacenters.
Test Plan: Ran `git pull` in a clustered repository with an up node and a down node; saw retry count dynamically adjust to available node count.
Maniphest Tasks: T13286
Differential Revision: https://secure.phabricator.com/D20777
Summary:
Depends on D20775. Ref T13286. When a Git read request fails against a cluster and there are other nodes we could safely try, try more nodes.
We DO NOT retry the request if:
- the client read anything;
- the client wrote anything;
- or we've already retried several times.
Although //some// requests where bytes went over the wire in either direction may be safe to retry, they're rare in practice under Git, and we'd need to puzzle out what state we can safely emit.
Since most types of failure result in an outright connection failure and this catches all of them, it's likely to almost always be sufficient in practice.
Test Plan:
- Started a cluster with one up node and one down node, pulled it.
- Half the time, hit the up node and got a clean pull.
- Half the time, hit the down node and got a connection failure followed by a retry and a clean pull.
- Forced `$err = 1` so even successful attempts would retry.
- On hitting the up node, got a "failure" and a decline to retry (bytes already written).
- On hitting the down node, got a failure and a real retry.
- (Note that, in both cases, "git pull" exits "0" after the valid wire transaction takes place, even though the remote exited non-zero. If the server gave Git everything it asked for, it doesn't seem to care if the server then exited with an error code.)
Maniphest Tasks: T13286
Differential Revision: https://secure.phabricator.com/D20776
Summary:
Ref T13286. To support request retries, allow the service lookup method to return an ordered list of structured service references.
Existing callsites continue to immediately discard all but the first reference and pull a URI out of it.
Test Plan: Ran `git pull` in a clustered repository with an "up" node and a "down" node, saw 50% serivce failures and 50% clean pulls.
Maniphest Tasks: T13286
Differential Revision: https://secure.phabricator.com/D20775
Ref T13401. The checkout UI didn't get fully updated to the new View objects,
and account handles are still manually building a URI that goes to the wrong
place.
Ref T13401. The checkout UI didn't get fully updated to the new View objects,
and account handles are still manually building a URI that goes to the wrong
place.
Summary:
Ref T13393. See some previous discussion in T13366.
Caching is hard and all approaches here have downsides, but the request cache likely has fewer practical downsides for this kind of policy check than other approaches. In particular, the grant approach (at least, as previously used in Phortune) has a major downside that "Query" classes can no longer fully enforce policies.
Since Phortune no longer depends on grants and they've now been removed from instances, drop the mechanism completely.
Test Plan: Grepped for callsites, found none.
Maniphest Tasks: T13393
Differential Revision: https://secure.phabricator.com/D20754
Summary:
Ref T13393. While doing a shard migration in the Phacility cluster, we'd like to stop writes to the migrating repository. It's safe to continue serving reads.
Add a simple maintenance mode for making repositories completely read-only during maintenance.
Test Plan: Put a repository into read-only mode, tried to write via HTTP + SSH. Viewed web UI. Took it back out of maintenance mode.
Maniphest Tasks: T13393
Differential Revision: https://secure.phabricator.com/D20748
Summary:
Fixes T13336.
- Prevent `--no-indexes` from being combined with `--for-replica`, since combining these options can only lead to heartbreak.
- In `--for-replica` mode, dump caches too. See discussion in T13336. It is probably "safe" to not dump these today, but fragile and not correct.
- Mark the "MarkupCache" table as having "Cache" persistence, not "Data" persistence (no need to back it up, since it can be fully regenerated from other datasources).
Test Plan: Ran `bin/storage dump` with various combinations of flags.
Maniphest Tasks: T13336
Differential Revision: https://secure.phabricator.com/D20743
Summary:
Fixes T13389. Currently, we try to "newSubtypeMap()" unconditionally, even if the underlying object does not support subtypes.
- Only try to build a subtype map if subtype transactions are actually being applied.
- When subtype transactions are applied to a non-subtypable object, fail more explicitly.
Test Plan: Clicked "Make Editable" in a fresh Calendar transaction form, got an editable form instead of a fatal from "newSubtypeMap()". (Calendar events are not currently subtypable.)
Maniphest Tasks: T13389
Differential Revision: https://secure.phabricator.com/D20741
Summary:
Fixes T13390. We have some old code which doesn't dynamically select between "utf8mb4" and "utf8". This can lead to dumping utf8mb4 data over a utf8 connection in `bin/storage dump`, which possibly corrupts some emoji/whales.
Instead, prefer "utf8mb4" if it's available.
Test Plan: Ran `bin/storage dump` and `bin/storage shell`, saw sub-commands select utf8mb4 as the client charset.
Maniphest Tasks: T13390
Differential Revision: https://secure.phabricator.com/D20742
Summary:
Depends on D20739. Ref T13366. Slightly modularize/update components of order views, and make orders viewable from either an account context (existing view) or an external context (new view).
The new view is generally simpler so this mostly just reorganizes existing code.
Test Plan: Viewed orders as an account owner and an external user.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20740
Summary: Depends on D20738. Ref T13366. Fixes T8389. Now that the infrastructure is in place, actually send email to external addresses.
Test Plan: Used `bin/phortune invoice` to generate invoices and saw associated external accounts receive mail in `bin/mail list-outbound`.
Maniphest Tasks: T13366, T8389
Differential Revision: https://secure.phabricator.com/D20739
Summary: Depends on D20737. Ref T13367. Allow external addresses to have their access key rotated. Account managers can disable them, and anyone with the link can permanently unsubscribe them.
Test Plan: Enabled/disabled addresses; permanently unsubscribed addresses.
Maniphest Tasks: T13367
Differential Revision: https://secure.phabricator.com/D20738
Summary: Ref T13366. This gives each account email address an "external portal" section so they can access invoices and receipts without an account.
Test Plan: Viewed portal as user with authority and in an incognito window.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20737
Summary:
Depends on D20734. Ref T13366. This makes the cart/order flow work under the new policy scheme with no "grantAuthority()" calls.
It prepares for a "Void Invoice" action, although the action doesn't actually do anything yet.
Test Plan: With and without merchant authority, viewed and paid invoices and went through the other invoice interaction workflows.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20735
Summary: Refresh the 404 text since it hasn't been updated in a while, and swap the "Save Query" button back to grey since I never got used to blue.
Test Plan: Hit 404 page, saved a query.
Differential Revision: https://secure.phabricator.com/D20734
Summary:
Depends on D20732. Ref T13366. This generally makes the "Merchant" UI look and work like the "Payment Account" UI.
This is mostly simpler since the permissions have largely been sorted out already and there's less going on here and less weirdness around view/edit policies.
Test Plan: Browsed all Merchant functions as a merchant member and non-member.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20733
Summary:
Ref T13366. Depends on D20721. Continue applying UI and policy updates to the last two Phortune objects.
Charges aren't mutable and Carts are already transactional, so this is less involved than prior changes.
Test Plan: Viewed various charge/order interfaces as merchants and account members.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20732
Summary:
Depends on D20720. Ref T13366.
- Use modern policies and policy interfaces.
- Use new merchant authority cache.
- Add (some) transactions.
- Move MFA from pre-upgrade-gate to post-one-shot-check.
- Simplify the autopay workflow.
- Use the "reloading arrows" icon for subscriptions more consistently.
Test Plan: As a merchant-authority and account-authority, viewed, edited, and changed autopay for subscriptions.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20721
Summary:
Depends on D20719. Currently, if a page throws an exception (like a policy exception) and rendering that exception into a response (like a policy dialog) throws another exception (for example, while constructing breadcrumbs), we only show the orginal exception.
This is usually the more useful exception, but sometimes we actually care about the other exception.
Instead of guessing which one is more likely to be useful, throw them both as an "AggregateException" and let the high-level handler flatten it for display.
Test Plan: {F6749312}
Differential Revision: https://secure.phabricator.com/D20720
Summary:
Depends on D20718. Ref T13366. Ref T13367.
- Phortune payment methods currently do not use transactions; update them.
- Give them a proper view page with a transaction log.
- Add an "Add Payment Method" button which always works.
- Show which subscriptions a payment method is associated with.
- Get rid of the "Active" status indicator since we now treat "disabled" as "removed", to align with user expectation/intent.
- Swap out of some of the super weird div-form-button UI into the new "big, clickable" UI for choice dialogs among a small number of options on a single dimension.
Test Plan:
- As a mechant-authority and account-authority, created payment methods from carts, subscriptions, and accounts. Edited and viewed payment methods.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13367, T13366
Differential Revision: https://secure.phabricator.com/D20719
Summary:
Depends on D20717. Ref T13366. Make PhortunePaymentMethod use an extended policy interface for consistency with modern approaches. Since Accounts have hard-coded policy behavior (and can't have object policies like "Subscribers") this should have no actual impact on program behavior.
This leaves one weird piece in the policy dialog UIs, see T13381.
Test Plan: Viewed and edited payment methods as a merchant and account member. Merchants can only view, not edit.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20718
Summary: Depends on D20716. Ref T13366. This implements the new policy behavior cleanly in all top-level Phortune payment account interfaces.
Test Plan: As a merchant with an account relationship (not an account member) and an account member, browsed all account interfaces and attempted to perform edits. As a merchant, saw a reduced-strength view.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20717
Summary:
Depends on D20715. Ref T13366. See that task for discussion.
Replace the unreliable "grantAuthority()"-based check with an actual "can the viewer edit any merchant this account has a relationship with?" check.
This makes these objects easier to use from a policy perspective and makes it so that the `Query` alone can fully enforce permissions properly with no setup, so general infrastructure (like handles and transactions) works properly with Phortune objects.
Test Plan: Viewed merchants and accounts as users with no authority, direct authority on the account, and indirect authority via a merchant relationship.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20716
Summary:
Depends on D20713. Ref T13366. When a payment account establishes a relationship with a merchant by creating a cart or subscription, create an edge to give the merchant access to view the payment account.
Also, migrate all existing subscriptions and carts to write these edges.
This aims at straightening out Phortune permissions, which are currently a bit wonky on a couple of dimensions. See T13366 for detailed discussion.
Test Plan:
- Created and edited carts/subscriptions, saw edges write.
- Ran migrations, saw edges write.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20715
Summary: Depends on D20697. Ref T8389. Add support for adding "billing@enterprise.com" and similar to Phortune accounts.
Test Plan: Added and edited email addresses for a payment account.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T8389
Differential Revision: https://secure.phabricator.com/D20713
Summary:
Ref T13366. Some of the information architecture is a little muddy here, notably an item called "Billing / History" which contains payment methods.
Split things up a bit to prepare for adding support for "Email Addresses".
Test Plan: {F6676988}
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20697
Summary: See PHI1396. Ideally this would be some kind of general-purpose tie-in to object relationships, but see D18456 for precedent.
Test Plan: Used `maniphest.edit` to edit associated commits for a task.
Differential Revision: https://secure.phabricator.com/D20731
Summary:
Fixes T13386. See PHI1391. These constraints largely exist already, but are not yet exposed to Conduit.
Also, tweak some keys to support the underlying query.
Test Plan: Ran `differential.revision.search` queries with the new constraints.
Maniphest Tasks: T13386
Differential Revision: https://secure.phabricator.com/D20730
Summary: Ref T13382. Currently, the "Make Administrator" action in the web UI does state-based MFA. Convert it to one-shot MFA.
Test Plan: Empowered and unempowered a user from the web UI, got one-shot MFA'd. Empowered a user from the CLI, no MFA issues.
Maniphest Tasks: T13382
Differential Revision: https://secure.phabricator.com/D20729
Summary:
Ref T13386. If you issue `differential.query` with a large offset (like 3000), it can overheat regardless of policy filtering and fail with a nonsensical error message.
This is because the overheating limit is based only on the query limit, not on the offset.
For example, querying for "limit = 100" will never examine more than 1,100 rows, so a query with "limit = 100, offset = 3000" will always fail (provided there are at least that many revisions).
Not all numbers work like you might expect them to becuase there's also a 1024-row fetch window, but basically small limits plus big offsets always fail.
Test Plan: Artificially reduced the internal window size from 1024 to 5, then ran `differential.query` with `offset=50` and `limit=3`. Before: overheated with weird error message. After: clean result.
Maniphest Tasks: T13386
Differential Revision: https://secure.phabricator.com/D20728
Summary:
Ref T13385. Currently, if you run `arc diff` in a CWD with more than 255 characters, the workflow fatals against the length of the `sourcePath` database column.
In the long term, removing this property is likely desirable.
For now, truncate long values and continue. This only meaningfully impacts relatively obscure interactive SVN workflows negatively, and even there, "some arc commands are glitchy in very long working directories in SVN" is still better than "arc diff fatals".
Test Plan:
- Modified `arc` to submit very long source paths.
- Ran `arc diff`.
- Before: Fatal when inserting >255 characters into `sourcePath`.
- After: Path truncated at 255 bytes.
Maniphest Tasks: T13385
Differential Revision: https://secure.phabricator.com/D20727
Summary:
Fixes T13384. Currently, the subtype "disabled" configuration is not respected when selecting fields for `ROLE_EDIT`.
The only meaningful caller for `ROLE_EDIT` is transaction validation, but transaction validation should respect fields being disabled by subtype configuration.
Test Plan:
- Added a "required" Maniphest custom field "F", then "disabled" it in a subtype "S".
- Created a task of subtype "S".
- Before: Form submission fails with error "F is required", even though the field is not actually visible on the form and can not be set.
- After: Form submits cleanly and creates the task.
Maniphest Tasks: T13384
Differential Revision: https://secure.phabricator.com/D20726
