1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-04 04:32:43 +01:00
Commit graph

273 commits

Author SHA1 Message Date
epriestley
b2b17485b9 Clean up two straggling UI issues in Phortune
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.
2019-08-31 09:36:23 -07:00
epriestley
97a4a59cf2 Give the Phortune external portal an order view
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
2019-08-26 07:49:17 -07:00
epriestley
a0a3879712 In Phortune, send order email to account external addresses
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
2019-08-26 07:48:27 -07:00
epriestley
4e13551e85 Add credential rotation and statuses (disabled, unsubscribed) to Phortune external email
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
2019-08-26 07:47:44 -07:00
epriestley
8f6a1ab015 Roughly support external/email user views of Phortune recipts and invoices
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
2019-08-26 07:39:08 -07:00
epriestley
a39a37fc0e Update the Phortune cart/invoice workflow for policy changes
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
2019-08-23 07:09:00 -07:00
epriestley
9bcd683c08 Update Phortune Merchant UI to bring it in line with Account UI
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
2019-08-22 21:12:33 -07:00
epriestley
c93ac91dc6 Update Charge and Cart policies in Phortune, and make URIs more consistent
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
2019-08-22 21:09:35 -07:00
epriestley
a542024b63 Update Phortune subscriptions for modern infrastructure
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
2019-08-22 21:07:17 -07:00
epriestley
201634848e Make Phortune payment methods transaction-oriented and always support "Add Payment Method"
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
2019-08-22 21:04:04 -07:00
epriestley
c4e0ac4d27 Update PhortunePaymentMethod for modern policy interfaces
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
2019-08-22 21:03:16 -07:00
epriestley
0cc7e8eeb8 Update Phortune payment account interfaces to handle merchant vs customer views
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
2019-08-22 21:02:41 -07:00
epriestley
a3213ab20b In Phortune, use actual merchant authority (not authority grants) to control account visibility
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
2019-08-22 21:01:55 -07:00
epriestley
277bce5638 In Phortune, write relationships between payment accounts and merchants they interact with
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
2019-08-22 21:01:04 -07:00
epriestley
e3ba53078e Add scaffolding for ad-hoc email addresses associated with Phortune accounts
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
2019-08-22 20:57:35 -07:00
epriestley
719a7d82c5 Refactor the Phortune account detail page into a series of smaller, more focused sections
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
2019-08-22 20:56:43 -07:00
epriestley
87f878ec8a Stop trying to CC merchants on invoices/receipts
Summary:
Fixes T13341. Currently, cart emails (invoices/receipts) are sent to members of the associated merchant account. This was just a simple way to keep an eye on things when this was first written.

The system works fine, and recent changes (almost certainly D20525) stopped these emails from working (presumably because of the slightly weird merchant permissions model).

This could be sorted out in more detail, but it looks like the path forward is to introduce a side channel for email anyway (via T8389), and that's a better way to implement this behavior since it means the normal recipients won't see a bunch of random staff/merchant email addresses on their receipts.

Test Plan: Grepped for `merchant` in this editor.

Maniphest Tasks: T13341

Differential Revision: https://secure.phabricator.com/D20696
2019-08-02 10:51:17 -07:00
epriestley
f5c380bfc9 Add very basic support for generating PDF documents
Summary: Ref T13358. This is very minimal, but technically works. The eventual goal is to generate PDF invoices to make my life easier when I have to interact with Enterprise Vendor Procurement.

Test Plan: {F6672439}

Maniphest Tasks: T13358

Differential Revision: https://secure.phabricator.com/D20692
2019-08-01 10:50:24 -07:00
epriestley
2ee5e71029 Simplify implementation of "SysetemAction->getSystemActionConstant()"
Summary: Depends on D20668. Ref T13343. Just an easy cleanup/simplification while I'm here.

Test Plan: `grep` for `getActionConstant()`

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13343

Differential Revision: https://secure.phabricator.com/D20669
2019-07-19 15:45:37 -07:00
epriestley
a3397fb876 Consider "all account members are disabled" to be a permanent failure when billing a Phortune subscription
Summary:
Fixes T13327. Currently, when we try to bill an account and all members are disabled, we fail temporarily and the task retries forever.

At least for now, just treat this as a permanent failure.

Test Plan:
  - Used `bin/phortune invoice` to generate a normal invoice for a regular subscription.
  - Disabled all the account members, then tried again. Got a helpful permanent failure:

```
$ ./bin/phortune invoice --subscription PHID-PSUB-kbedwt5cyepoc6tohjq5 --auto-range
Set current time to Mon, Jun 24, 2:47 PM.
Preparing to invoice subscription "localb.phacility.com" from Fri, May 31, 10:14 AM to Sun, Jun 30, 10:14 AM.
 WARNING
Manually invoicing will double bill payment accounts if the range overlaps an
existing or future invoice. This script is intended for testing and
development, and should not be part of routine billing operations. If you
continue, you may incorrectly overcharge customers.

    Really invoice this subscription? [y/N] y

[2019-06-24 14:47:57] EXCEPTION: (PhabricatorWorkerPermanentFailureException) All members of the account ("PHID-ACNT-qp54y3unedoaxgkkjpj4") for this subscription ("PHID-PSUB-kbedwt5cyepoc6tohjq5") are disabled. at [<phabricator>/src/applications/phortune/worker/PhortuneSubscriptionWorker.php:88]
arcanist(head=experimental, ref.master=d92fa96366c0, ref.experimental=db4cd55d4673), corgi(head=master, ref.master=6371578c9d32), instances(head=stable, ref.master=ba9e4a19df1c, ref.stable=37fb1f4917c7), libcore(), phabricator(head=master, ref.master=65bc481c91de, custom=11), phutil(head=master, ref.master=7adfe4e4f4a3), services(head=master, ref.master=5424383159ac)
  #0 PhortuneSubscriptionWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:124]
  #1 PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:163]
  #2 PhabricatorWorker::scheduleTask(string, array, array) called at [<phabricator>/src/applications/phortune/management/PhabricatorPhortuneManagementInvoiceWorkflow.php:169]
  #3 PhabricatorPhortuneManagementInvoiceWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:457]
  #4 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:349]
  #5 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/setup/manage_phortune.php:21]
$
```

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13327

Differential Revision: https://secure.phabricator.com/D20613
2019-06-24 15:29:31 -07:00
epriestley
1d58f14469 Remove WePay support from Phortune, and Restful/Httpful dependencies
Summary: Ref PHI1166. I'm documenting our dependencies, and we have approximately 5,000 lines of external code to support WePay as a Phortune provider. We don't use it, I'm almost certain it doesn't work, and we have no plans to use it in the near future. If we did pursue it, I'd probably just wrap the API in a 100-line `WePayFuture` anyway since 5K lines of dependencies to make a couple method calls is ridiculous.

Test Plan: Grepped for `wepay`, `httpful`, `restful`.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: aurelijus

Differential Revision: https://secure.phabricator.com/D20521
2019-05-14 09:14:53 -07:00
epriestley
5892c78986 Replace all "setQueryParam()" calls with "remove/replaceQueryParam()"
Summary: Ref T13250. See D20149. Mostly: clarify semantics. Partly: remove magic "null" behavior.

Test Plan: Poked around, but mostly just inspection since these are pretty much one-for-one.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20154
2019-02-14 11:56:39 -08:00
epriestley
4c12420162 Replace "URI->setQueryParams()" after initialization with a constructor argument
Summary: Ref T13250. See D20149. In a number of cases, we use `setQueryParams()` immediately after URI construction. To simplify this slightly, let the constructor take parameters, similar to `HTTPSFuture`.

Test Plan: See inlines.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20151
2019-02-14 11:46:37 -08:00
epriestley
9a9fa8bed2 Rate limit attempts to add payment methods in Phortune
Summary: Ref T13249. See D20132. Although we're probably a poor way to validate a big list of stolen cards in practice in production today (it's very hard to quickly generate a large number of small charges), putting rate limiting on "Add Payment Method" is generally reasonable, can't really hurt anything (no legitimate user will ever hit this limit), and might frustrate attackers in the future if it becomes easier to generate ad-hoc charges (for example, if we run a deal on support pacts and reduce their cost from $1,000 to $1).

Test Plan: Reduced limit to 4 / hour, tried to add a card several times, got rate limited.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20158
2019-02-13 12:25:56 -08:00
epriestley
2b718d78bb Improve UI/UX when users try to add an invalid card with Stripe
Summary: Ref T13244. See PHI1052. Our error handling for Stripe errors isn't great right now. We can give users a bit more information, and a less jarring UI.

Test Plan:
Before (this is in developer mode, production doesn't get a stack trace):

{F6197394}

After:

{F6197397}

- Tried all the invalid test codes listed here: https://stripe.com/docs/testing#cards

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20132
2019-02-09 05:54:42 -08:00
epriestley
9f5e6bee90 Make the default behavior of getApplicationTransactionCommentObject() "return null" instead of "throw"
Summary:
Depends on D20115. See <https://discourse.phabricator-community.org/t/transaction-search-endpoint-does-not-work-on-differential-diffs/2369/>.

Currently, `getApplicationTransactionCommentObject()` throws by default. Subclasses must override it to `return null` to indicate that they don't support comments.

This is silly, and leads to a bunch of code that does a `try / catch` around it, and at least some code (here, `transaction.search`) which doesn't `try / catch` and gets the wrong behavior as a result.

Just make it `return null` by default, meaning "no support for comments". Then remove the `try / catch` stuff and all the `return null` implementations.

Test Plan:
  - Grepped for `getApplicationTransactionCommentObject()`, fixed each callsite / definition.
  - Called `transaction.search` on a diff with transactions (i.e., not a sourced-from-commit diff).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: jbrownEP

Differential Revision: https://secure.phabricator.com/D20121
2019-02-07 14:56:38 -08:00
epriestley
6b6c991ad4 Allow Phortune accounts to customize their billing address and name
Summary:
See PHI1023. Ref T7607. Occasionally, companies need their billing address (or some other custom text) to appear on invoices to satisfy process or compliance requirements.

Allow accounts to have a custom "Billing Name" and a custom "Billing Address" which appear on invoices.

Test Plan: {F6134707}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T7607

Differential Revision: https://secure.phabricator.com/D19979
2019-01-16 16:16:27 -08:00
epriestley
11cf8f05b1 Remove "getApplicationTransactionObject()" from ApplicationTransactionInterface
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).

The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.

The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.

So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.

Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.

Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11351

Differential Revision: https://secure.phabricator.com/D19920
2018-12-20 15:16:19 -08:00
epriestley
937e88c399 Remove obsolete, no-op implementations of "willRenderTimeline()"
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.

All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.

Test Plan: Used `grep willRenderTimeline` to find callsites, found none.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T11351

Differential Revision: https://secure.phabricator.com/D19919
2018-12-20 15:04:49 -08:00
epriestley
4967cd6ab9 Fix some "%Q" behavior in PhortuneMerchantQuery
Summary: Ref T13217. This older query does some manual joins; update it for more modern joins.

Test Plan: Ran `instances/` unit tests and got a clean result, browsed Phortune merchants.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19820
2018-11-20 07:59:57 -08:00
epriestley
86fd204148 Fix all query warnings in "arc unit --everything"
Summary:
Ref T13216. Ref T13217. Depends on D19800. This fixes all of the remaining query warnings that pop up when you run "arc unit --everything".

There's likely still quite a bit of stuff lurking around, but hopefully this covers a big set of the most common queries.

Test Plan: Ran `arc unit --everything`. Before change: lots of query warnings. After change: no query warnings.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217, T13216

Differential Revision: https://secure.phabricator.com/D19801
2018-11-15 03:51:25 -08:00
epriestley
98690ee326 Update many Phabricator queries for new %Q query semantics
Summary: Depends on D19785. Ref T13217. This converts many of the most common clause construction pathways to the new %Q / %LQ / %LO / %LA / %LJ semantics.

Test Plan: Browsed around a bunch, saw fewer warnings and no obvious behavioral errors. The transformations here are generally mechanical (although I did them by hand).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: hach-que

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19789
2018-11-15 03:48:10 -08:00
epriestley
5bcca675e1 Add card expiration information to Phortune cart screen
Summary:
See PHI689. It can be difficult to distinguish between cards with the same number but different expiration dates (common when the bank sends you a new card).

For now, show the expiration date on the cart checkout screen.

Test Plan: Viewed a cart checkout screen with multiple cards, saw expiration dates.

Reviewers: amckinley

Differential Revision: https://secure.phabricator.com/D19462
2018-06-02 18:23:44 -07:00
epriestley
94d340fcff Include OAuth targets in "form-action" Content-Security-Policy
Summary:
Ref T4340. Some "Register/Login" and "Link External Account" buttons are forms which submit to third-party sites. Whitelist these targets when pages render an OAuth form.

Safari, at least, also prevents a redirect to a third-party domain after a form submission to the local domain, so when we first redirect locally (as with Twitter and other OAuth1 providers) we need to authorize an additional URI.

Test Plan: Clicked all my registration buttons locally without hitting CSP issues.

Maniphest Tasks: T4340

Differential Revision: https://secure.phabricator.com/D19159
2018-02-28 19:28:35 -08:00
epriestley
dba4c4bdf6 Emit a "Content-Security-Policy" HTTP header
Summary:
See PHI399. Ref T4340. This header provides an additional layer of protection against various attacks, including XSS attacks which embed inline `<script ...>` or `onhover="..."` content into the document.

**style-src**: The "unsafe-inline" directive affects both `style="..."` and `<style>`. We use a lot of `style="..."`, some very legitimately, so we can't realistically get away from this any time soon. We only use one `<style>` (for monospaced font preferences) but can't disable `<style>` without disabling `style="..."`.

**img-src**: We use "data:" URIs to inline small images into CSS, and there's a significant performance benefit from doing this. There doesn't seem to be a way to allow "data" URIs in CSS without allowing them in the document itself.

**script-src** and **frame-src**: For a small number of flows (Recaptcha, Stripe) we embed external javascript, some of which embeds child elements (or additional resources) into the document. We now whitelist these narrowly on the respective pages.

This won't work with Quicksand, so I've blacklisted it for now.

**connect-src**: We need to include `'self'` for AJAX to work, and any websocket URIs.

**Clickjacking**: We now have three layers of protection:

  - X-Frame-Options: works in older browsers.
  - `frame-ancestors 'none'`: does the same thing.
  - Explicit framebust in JX.Stratcom after initialization: works in ancient IE.

We could probably drop the explicit framebust but it wasn't difficult to retain.

**script tags**: We previously used an inline `<script>` tag to start Javelin. I've moved this to `<data data-javelin-init ...>` tags, which seems to work properly.

**`__DEV__`**: We previously used an inline `<script>` tag to set the `__DEV__` mode flag. I tried using the "initialization" tags for this, but they fire too late. I moved it to `<html data-developer-mode="1">`, which seems OK everywhere.

**CSP Scope**: Only the CSP header on the original request appears to matter -- you can't refine the scope by emitting headers on CSS/JS. To reduce confusion, I disabled the headers on those response types. More headers could be disabled, although we're likely already deep in the land of diminishing returns.

**Initialization**: The initialization sequence has changed slightly. Previously, we waited for the <script> in bottom of the document to evaluate. Now, we go fishing for tags when domcontentready fires.

Test Plan:
  - Browsed around in Firefox, Safari and Chrome looking for console warnings. Interacted with various Javascript behaviors. Enabled Quicksand.
  - Disabled all the framebusting, launched a clickjacking attack, verified that each layer of protection is individually effective.
  - Verified that the XHProf iframe in Darkconsole and the PHPAST frame layout work properly.
  - Enabled notifications, verified no complaints about connecting to Aphlict.
  - Hit `__DEV__` mode warnings based on the new data attribute.
  - Tried to do sketchy stuff with `data:` URIs and SVGs. This works but doesn't seem to be able to do anything dangerous.
  - Went through the Stripe and Recaptcha workflows.
  - Dumped and examined the CSP headers with `curl`, etc.
  - Added a raw <script> tag to a page (as though I'd found an XSS attack), verified it was no longer executed.

Maniphest Tasks: T4340

Differential Revision: https://secure.phabricator.com/D19143
2018-02-27 10:17:30 -08:00
epriestley
2bb4fc9ece Fix a Phortune billing issue where subscription autopay could charge disabled cards
Summary:
See support email. There's nothing tricky here, we were just missing a check. The different parts of this got built at different times so I think this was simply overlooked.

Also add a redundant check just to future-proof this and be on the safe side.

Test Plan: Used `bin/phortune invoice` to charge a pact subscription. After deleting the card, the charge failed with an appropriate error.

Reviewers: amckinley

Differential Revision: https://secure.phabricator.com/D19020
2018-02-08 06:30:59 -08:00
epriestley
f090fa7426 Use object PHIDs for "Thread-Topic" headers in mail
Summary:
Depends on D19009. Ref T13053. For "Must Encrypt" mail, we must currently strip the "Thread-Topic" header because it sometimes contains sensitive information about the object.

I don't actually know if this header is useful or anyting uses it. My understanding is that it's an Outlook/Exchange thing, but we also implement "Thread-Index" which I think is what Outlook/Exchange actually look at. This header may have done something before we implemented "Thread-Index", or maybe never done anything. Or maybe older versions of Excel/Outlook did something with it and newer versions don't, or do less. So it's possible that an even better fix here would be to simply remove this, but I wasn't able to convince myself of that after Googling for 10 minutes and I don't think it's worth hours of installing Exchange/Outlook to figure out. Instead, I'm just trying to simplify our handling of this header for now, and maybe some day we'll learn more about Exchange/Outlook and can remove it.

In a number of cases we already use the object monogram or PHID as a "Thread-Topic" without users ever complaining, so I think that if this header is useful it probably isn't shown to users, or isn't shown very often (e.g., only in a specific "conversation" sub-view?). Just use the object PHID (which should be unique and stable) as a thread-topic, everywhere, automatically.

Then allow this header through for "Must Encrypt" mail.

Test Plan: Processed some local mail, saw object PHIDs for "Thread-Topic" headers.

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19012
2018-02-08 06:21:00 -08:00
Dmitri Iouchtchenko
9bd6a37055 Fix spelling
Summary: Noticed a couple of typos in the docs, and then things got out of hand.

Test Plan:
  - Stared at the words until my eyes watered and the letters began to swim on the screen.
  - Consulted a dictionary.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D18693
2017-10-09 10:48:04 -07:00
Chad Little
92c49c3772 Minor UX tweaks to Phortune autopay
Summary: Fixes T12958. Adds a success message when card is added, also switches to use radio buttons for clarity. Updated redirect uri for deleting methods as well.

Test Plan:
Add cards, remove cards.

{F5091084}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12958

Differential Revision: https://secure.phabricator.com/D18381
2017-08-10 07:39:18 -07:00
epriestley
301750f6e6 Add an after-purchase hook to subscriptions in Phortune
Summary:
Ref T12681. We need this to update the "paid until" window on support pacts.

(Instance billing doesn't use this because everything just checks if you have unpaid invoices, nothing actually happens when you pay them.)

Test Plan: See D18187.

Reviewers: chad, amckinley

Reviewed By: chad

Maniphest Tasks: T12681

Differential Revision: https://secure.phabricator.com/D18188
2017-07-07 16:39:47 -07:00
Chad Little
38904ea4ad More button grey conversions
Summary: Ran across a few straglers. Convert to the correct color.

Test Plan: grep for profile-image-button, check profile image selection page

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D18096
2017-06-07 09:34:50 -07:00
Chad Little
d3c464a610 Separate button CSS classes
Summary: Try to dis-ambiguate various button types and colors. Moves `simple` to `phui-button-simple` and moves colors to `button-color`.

Test Plan: Grep for buttons still inline, UIExamples, PHUIX, Herald, and Email Preferences.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D18077
2017-06-05 20:14:34 +00:00
Chad Little
0e8f72a0d9 Clarify Subscription page language
Summary: This says "Edit Subscription" but the page only lets you update the payment method for autopay. I think this is confusing users. I tried to find the best language here, but suggest something else if you prefer.

Test Plan: Review language on sample subscription page.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12451

Differential Revision: https://secure.phabricator.com/D17944
2017-05-17 14:48:22 -07:00
Chad Little
32af1d9a9f Restrict PhortuneMerchant creation at EditEngine level
Summary: Ref T12685. Checks merchant capabilities at the edit engine level

Test Plan: Test with and without admin level permissions. Get restricted access if I cheat. Still able to create with admin.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12685

Differential Revision: https://secure.phabricator.com/D17852
2017-05-08 15:56:07 -07:00
epriestley
aec19d2acf Reduce code duplication in Phortune account controllers
Summary:
Ref T12451. This is a GREAT comment (A++) but we only need one copy of it.

This uses a pattern similar to Projects, which is a little weird but works well enough.

Test Plan:
  - Viewed all four tabs of an account.
  - Viewed a page with a bad account ID which 404'd properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12451

Differential Revision: https://secure.phabricator.com/D17694
2017-04-14 10:24:56 -07:00
epriestley
7fbb5f2d92 Reduce some code duplication in PhortuneLandingController
Summary: Ref T12451. This code is the same as the other code.

Test Plan: Went through the default-account case with this code, worked the same as the other code.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12451

Differential Revision: https://secure.phabricator.com/D17693
2017-04-14 10:24:32 -07:00
epriestley
505b1d8379 Fix member edit transaction validation so it works for both implicit and explicit account creation
Summary:
Ref T12451. Ref T12484. This should deal with all the `+` / `-` / `=` cases correctly, I think.

Also makes sure that members are real users, not commits or tokens or whatever. And expands the creation test case to make some other basic sanity checks.

Test Plan:
  - Went through implicit first-time creation flow.
  - Went through explicit second-time creation flow.
  - Unit test now passes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12484, T12451

Differential Revision: https://secure.phabricator.com/D17692
2017-04-14 10:24:15 -07:00
epriestley
71d933d496 Add a failing test case for new Phortune account initialization
Summary:
Ref T12451. Ref T12484. I think D17657 fixed this, but caused the bug in D17690. The fix for that causes this bug again.

Put a unit test on it. This test currently fails; I'll correct the bug in the next change.

Test Plan: Ran `arc unit`, saw a failure.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12484, T12451

Differential Revision: https://secure.phabricator.com/D17691
2017-04-14 10:23:10 -07:00
epriestley
e1a8b5d3e9 Fix a bug where Phortune accounts created via "Create Account" would not have the viewer added as a member
Summary:
Ref T12451. When you explicitly created a second or third account or whatever, you wouldn't be added as a member.

(The editor sees that you're "already a member", so it doesn't add you.)

Test Plan:
  - Go to `/phortune/`.
  - Click "Switch Accounts".
  - Click "Create Account".
  - Create an account.
  - Before patch: unable to view it since you don't get added as a member.
  - After patch: account created with you as member.
  - Also created an accont with multiple members.
  - Tried to create an account with no members.
  - Tried to create an account with just someone else as a member.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12451

Differential Revision: https://secure.phabricator.com/D17690
2017-04-14 10:22:54 -07:00
epriestley
7274e4857c Fix a bug where Phortune could fatal while building crumbs
Summary: Ref T12451. `$this->getAccount()` may not return an account.

Test Plan:
  - Visit `/phortune/X/`, where `X` is the ID of an account you don't have permission to view.
  - Before patch: fatal.
  - After patch: normal policy exception page.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12451

Differential Revision: https://secure.phabricator.com/D17689
2017-04-14 10:22:42 -07:00