Page MenuHomePhabricator

Update Phortune to work better with "enterprise" billing/accounts departments
Open, NormalPublic


Most Phacility customers have no problem just paying for things with a credit card. This system works more-or-less flawlessly and more-or-less automatically with nearly zero human intervention.

This doesn't work for a small minority of customers:

Unable to Pay With Credit Cards: On the higher end of complexity, a tiny minority of customers can not pay with a card at all (and usually need to be invoiced and do bank transfers). Almost without exception, customers who can not pay with a card also have difficulty paying on time, paying regularly, and paying without human intervention.

We also can't automatically receive/process inbound bank transfers, and they don't have any kind of automatic reference to what they're paying for so I'm not confident this is possible to do automatically even if our bank had a modern API (which it doesn't, as far as I know).

We no longer accept new customers who can not use a credit card below the $5K/month spending level and I give these customers "increase your spending level to $5K or self-host" as options when they ask.

Although Stripe does have some level of ACH support now, many of these customers have additional complicated process requirements (e.g. one customer required we extend three-month interest-free loans to them with Net 90 billing) and the ACH enrollment/verification process itself seems complicated so I'm not sure this would do much. (And some customers must initiate the ACH transfers.)

"Can you pay with a credit card?" is really a proxy question for "Is your billing/payment process mostly reasonable?". If the answer to the first question is "no", it seems like that's a good predictor that the answer the second question is "no", and we're signing up for a thrilling adventure through accounts payable if we accept you as a customer.

These are the only customers I end up on email threads with 5+ people CC'd trying to figure out a basic billing issue like "you haven't paid us for 15 months and we'd like your money please".

Can Pay With Cards + Want Separate Invoicing/Receipts: In the middle, some customers can pay with credit cards but need invoices or receipts sent somewhere. We've tried to make this somewhat easy in the hope that whoever is authorized on the account will do it for us, but they sometimes don't and we should really automate it.

The rough scope here is:

  • (T13358) We should generate and attach PDFs to invoice/receipt emails, since PDFs are the lingua franca of enterprise billing.
  • (T7607) The invoicing/receipt email should include more information (and a PDF attachment) so it can be forwarded more easily.
  • (T13341) Merchant email behavior changed recently, but was always weird. We should probably just drop these merchant emails since they aren't terribly useful. If we want separate merchant emails, we should generate/deliver them separately (like billing-address emails, below).
  • (T8389) Adding "Billing Email Addresses" to payment accounts.

Of these, the billing email addresses is probably the most complicated (since PDFs seem to be within the realm of tractable so far).

The major issues with "Billing Email Addresses" are:

Spam/Abuse: Ideally, I don't want to require billing@ to verify the address or really do anything at all. However, that means that "Billing Address" will become a possible spam vector. For now, I think we can just rate limit these since the user won't control any content of the mail. We could add a verification step or an administrative approval step eventually. A more conservative approach would be to prevent users from adding addresses and require staff to add them instead.

Features: When you receive an enrollment mail or an invoice/receipt mail to a billing address, the mail should:

  • Make it clear which user added the address.
  • Make it clear which email address is receiving the mail.
  • Make it clear which account the address is associated with.
  • Provide a way to disable the address without a user account.
  • Provide a way to review all invoices/receipts without a user account.

The "web view of account history" means that we need to do some policy magic and secret management in the web UI, and create a sub-area in Phortune which can be accessed without an account (by knowing a secret URI).

We need to make this behavior clear to users and probably provide some tools for rotating secrets and maybe logging access, although the existence of is knowable so the fact that you paid for it every month isn't surprising and we're relatively free to prioritize ease-of-use over security here. If a secret leaks it's like someone combing through your garbage for receipts, not someone stealing your credit card.

Event Timeline

epriestley triaged this task as Normal priority.Aug 2 2019, 5:40 PM
epriestley created this task.

This is also more "while I'm here", but there's no web UI way to void an invoice right now. It would be nice to have a "Void Invoice" button for staff instead of requiring database fiddling, since this comes up occasionally.

It would also be nice to figure out Phortune permissions better here since this is a good opportunity to vet everything.

Phortune has a fairly unique problem where we'd like a given Cart object (which corresponds to a cart, invoice, or receipt depending on context) to be visible to managers of the payment account ("John Doe") that is buying the items and also the merchant account ("Acme Corp") which is selling the items.

We don't want to let AcmeCorp see everything that John has purchased, only things John has purchased from AcmeCorp. More broadly, the ideal permissions model here is more granular than just "Staff/Seller" vs "Customer/Buyer". Phortune should, ideally, support a future marketplace where users have three separate roles: Staff, Merchant, Buyer.

For example, we may let developers sell applications in an "App Store" in the future. In this case, Phacility would be in a "Staff" role; the developer/seller ("AcmeAppDeveloperCorp") in a merchant role, and the buyer ("John-who-works-at-EngineeringCorp") in a customer role. AcmeCorp developing and selling an application to EngineeringCorp, and Phacility is taking a fair and reasonable 99% cut or whatever.

This use case may or may not really come to pass, but it would be desirable to retain this role separation to support it and not just say "anyone with merchant access can see everything".

Currently, Phortune partially accomplishes this with grantAuthority()/getAuthority(). This mechanism allows particular pages to grant a viewer a particular authority (here, authority over a Merchant account) and then policy checks can test if a given viewer has a particular authority.

I'm not thrilled with this mechanism, originally from D11945. This mechanism is doing two things:

  • serving as a cache, so we don't need to keep pulling merchant memberships for users multiple times on the same page; and
  • allowing workflows to actively bestow authority on particular users.

The cache part is sort of fine, but it would probably be better for this to just be an explicit cache backed by PhabricatorCaches::getRequestCache() that can be cleared via destroyRequestCache(). The "Authority" change came before the "Request Cache" change (in D13153), and daemons may not properly reset the cache on user objects today. This is very unlikely to cause any real problems, but it would generally be more robust to have all the caching go through the same APIs.

The "workflows proactively apply grants at the Controller level" part is actively bad, since it breaks the idea that Query classes can just do all policy enforcement correctly and means that if you're loading Cart objects or other similar objects, you must first do a secret grantAuthority() dance or you get the wrong result. You always get the wrong result in the better direction (i.e., the application forbids you from seeing an object you should be able to see, which is annoying but not dangerous), but it's still the wrong result.

This likely led to some issues like T13341, where TransactionEditor does not know it has to do grantAuthority() magic. It also leads to a case where loading Account handles fails even if the viewer has Merchant-derived authority to see them, which is cumbersome and vaguely frustrating from a staff perspective.

The desired behavior is that the policy check encodes this behavior instead:

  • You can see a Cart if you CAN_VIEW the associated Payment Account or CAN_EDIT the associated Merchant Account.
  • You can see a Payment Account if you're a member, or the Payment Account has at least one cart or subscription with a merchant you CAN_EDIT.

Since none of these objects have mutable policies (or are ever likely to have them), we can implement this behavior in hasAutomaticCapability() and refine it (if necessary) with PhabricatorPolicyCodexInterface. If objects could have mutable policies, we'd possibly want to use PhabricatorExtendedPolicyInterface instead, but it can not currently express OR constraints (like "you must satisfy the object policies, and also be able to see either object X or object Y"). But I can't imagine ever letting a Cart have "Visible To: only me" policy and forbidding other payment account managers or merchant managers from seeing the object, so this is pretty moot.

Carts already implement the behavior described above mostly-properly. Some sub-objects do not implement the best possible / most robust policies, but these are easy to fix.

Accounts are really the only problem. Implementing the right behavior is fairly easy, but implementing it efficiently is trickier. In particular:

  1. There's no cheap (mostly: O(number-of-merchants) instead of O(number-of-interactions)) way to get the set of merchants an account has interacted with (has one Cart or Subscription for).
  2. There's no cheap (mostly: cache-friendly) way to get the set of merchants an account CAN_EDIT.
  3. There's no trivial method for testing if sets (1) and (2) intersect.

To solve (1), we can write a "Payment Account X bought something from Merchant Y" edge when a cart or subscription is created, and migrate historical carts and subscriptions. We can reasonably pull and attach these edges in the Query unconditionally.

To solve (2) and (3), we can issue the query easily enough, but the policy layer will do a whole lot of checks/queries against it by default. We probably do still want something that looks a bit like getAuthority() from a cache perspective, but likely uses PhabricatorCaches::getRequestCache() instead of the weird, one-off Authority mechanism.

Then there are probably some minor cases where we want behavioral differences at the application level if a viewer is acting on behalf of a merchant vs an actual member of the payment account, but that probably does not need to be a policy-level test.

I think we end up with this approach:

  • Add a "PaymentAccountHasInteractedWithMerchantAccount" edge.
  • Write this edge when creating a "Subscription" or "Cart".
  • Load and attach this edge in "PaymentAccountQuery".

Then, make viewer merchants cheap to test and do the correct test:

  • Provide a PhortuneMerchantCanEditCache::canTheseViewersEditAnyOfTheseMerchants($viewer_phids, $merchant_phids) sort of API backed by the request cache.
    • It's probably fine to satisfy this by pulling all merchants the viewers can edit, since this is currently straightforward and we can anticipate that it is cheap.
  • In hasAutomaticCapability(), do this test instead of the existing test.

Then, sort out downstream policies, how the Controllers handle policies, maybe add PolicyCodex implementations where necessary, etc.

Stuff to fix with subscriptions:

  • Policies could use an update.
  • Policies should use the new cache.
  • No transactions yet.
  • "Manage Autopay" has a read MFA upgrade gate, should be a transaction-based write one-shot MFA gate.
  • "Manage Autopay" into adding a card is wonky. Should probably do "add card", then redirect you to confirm you want to set it as the autopay option.
  • Subscriptions have inconsistent icons (blank calendar vs reload arrows).
epriestley added a revision: Restricted Differential Revision.Aug 23 2019, 3:07 PM
epriestley added a commit: Restricted Diffusion Commit.Aug 29 2019, 11:58 PM