From 12d29d8206728fdf2acd256722536a5aaed173b5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 26 Oct 2016 15:02:28 -0700 Subject: [PATCH] Fix a Phortune bug where an invalid viewer could sometimes be selected for billing a subscription Summary: A live instance hit the scenario described in the comment, where an out-of-date user was being selected as the actor. Since they were no longer an account member, they could not see the payment method and autopay was failing. Instead, select a relatively arbitrary user who is a current, valid, non-disabled member. Test Plan: Ran subscriptions with `bin/worker execute ...`, saw it select a valid actor. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D16757 --- .../worker/PhortuneSubscriptionWorker.php | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/applications/phortune/worker/PhortuneSubscriptionWorker.php b/src/applications/phortune/worker/PhortuneSubscriptionWorker.php index 19399150e8..097fb54dd9 100644 --- a/src/applications/phortune/worker/PhortuneSubscriptionWorker.php +++ b/src/applications/phortune/worker/PhortuneSubscriptionWorker.php @@ -36,17 +36,32 @@ final class PhortuneSubscriptionWorker extends PhabricatorWorker { ->setSubscription($subscription); // TODO: This isn't really ideal. It would be better to use an application - // actor than the original author of the subscription. In particular, if - // someone initiates a subscription, adds some other account managers, and - // later leaves the company, they'll continue "acting" here indefinitely. + // actor than a fairly arbitrary account member. + // However, for now, some of the stuff later in the pipeline requires a // valid actor with a real PHID. The subscription should eventually be // able to create these invoices "as" the application it is acting on // behalf of. - $actor = id(new PhabricatorPeopleQuery()) + + $members = id(new PhabricatorPeopleQuery()) ->setViewer($viewer) - ->withPHIDs(array($subscription->getAuthorPHID())) - ->executeOne(); + ->withPHIDs($account->getMemberPHIDs()) + ->execute(); + $actor = null; + foreach ($members as $member) { + + // Don't act as a disabled user. If all of the users on the account are + // disabled this means we won't charge the subscription, but that's + // probably correct since it means no one can cancel or pay it anyway. + if ($member->getIsDisabled()) { + continue; + } + + // For now, just pick the first valid user we encounter as the actor. + $actor = $member; + break; + } + if (!$actor) { throw new Exception(pht('Failed to load actor to bill subscription!')); }