From 85b9d5b7e428f0b4f648bacb84d2bf41b07dbaad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Lys=C3=BD?= Date: Thu, 27 May 2021 19:29:03 +0200 Subject: [PATCH] Use Doctrine entity in calling LinkTokens::verifyToken [MAILPOET-3269] --- .../ViewInBrowser/ViewInBrowserController.php | 7 +- lib/Router/Endpoints/Track.php | 6 +- lib/Subscription/Manage.php | 25 ++++++-- lib/Subscription/Pages.php | 11 +++- .../ViewInBrowserControllerTest.php | 4 +- .../Router/Endpoints/TrackTest.php | 13 ++-- tests/integration/Subscription/ManageTest.php | 2 + tests/integration/Subscription/PagesTest.php | 64 +++++++++++-------- 8 files changed, 77 insertions(+), 55 deletions(-) diff --git a/lib/Newsletter/ViewInBrowser/ViewInBrowserController.php b/lib/Newsletter/ViewInBrowser/ViewInBrowserController.php index 388953e91f..8469affb20 100644 --- a/lib/Newsletter/ViewInBrowser/ViewInBrowserController.php +++ b/lib/Newsletter/ViewInBrowser/ViewInBrowserController.php @@ -4,7 +4,6 @@ namespace MailPoet\Newsletter\ViewInBrowser; use MailPoet\Entities\SendingQueueEntity; use MailPoet\Models\Newsletter; -use MailPoet\Models\Subscriber; use MailPoet\Newsletter\Sending\SendingQueuesRepository; use MailPoet\Newsletter\Url as NewsletterUrl; use MailPoet\Subscribers\LinkTokens; @@ -91,11 +90,7 @@ class ViewInBrowserController { throw new \InvalidArgumentException("Missing 'subscriber_token'"); } - $subscriberModel = Subscriber::findOne($subscriber->getId()); - if (!$subscriberModel) { - return null; - } - if (!$this->linkTokens->verifyToken($subscriberModel, $data['subscriber_token'])) { + if (!$this->linkTokens->verifyToken($subscriber, $data['subscriber_token'])) { throw new \InvalidArgumentException("Invalid 'subscriber_token'"); } return $subscriber; diff --git a/lib/Router/Endpoints/Track.php b/lib/Router/Endpoints/Track.php index 7a8032d851..e957d8a37e 100644 --- a/lib/Router/Endpoints/Track.php +++ b/lib/Router/Endpoints/Track.php @@ -5,7 +5,6 @@ namespace MailPoet\Router\Endpoints; use MailPoet\Config\AccessControl; use MailPoet\Cron\Workers\StatsNotifications\NewsletterLinkRepository; use MailPoet\Entities\SendingQueueEntity; -use MailPoet\Models\Subscriber; use MailPoet\Newsletter\Links\Links; use MailPoet\Newsletter\NewslettersRepository; use MailPoet\Newsletter\Sending\SendingQueuesRepository; @@ -99,13 +98,12 @@ class Track { public function _validateTrackData($data) { if (!$data->subscriber || !$data->queue || !$data->newsletter) return false; - $subscriberModel = Subscriber::findOne($data->subscriber->getId()); - $subscriberTokenMatch = $this->linkTokens->verifyToken($subscriberModel, $data->subscriber_token); // phpcs:ignore Squiz.NamingConventions.ValidVariableName.NotCamelCaps + $subscriberTokenMatch = $this->linkTokens->verifyToken($data->subscriber, $data->subscriber_token); // phpcs:ignore Squiz.NamingConventions.ValidVariableName.NotCamelCaps if (!$subscriberTokenMatch) { $this->terminate(403); } // return if this is a WP user previewing the newsletter - if ($subscriberModel->isWPUser() && $data->preview) { + if ($data->subscriber->isWPUser() && $data->preview) { return $data; } // check if the newsletter was sent to the subscriber diff --git a/lib/Subscription/Manage.php b/lib/Subscription/Manage.php index 30946c343b..68773d8925 100644 --- a/lib/Subscription/Manage.php +++ b/lib/Subscription/Manage.php @@ -6,6 +6,7 @@ use MailPoet\Entities\StatisticsUnsubscribeEntity; use MailPoet\Entities\SubscriberEntity; use MailPoet\Entities\SubscriberSegmentEntity; use MailPoet\Form\Util\FieldNameObfuscator; +use MailPoet\InvalidStateException; use MailPoet\Models\CustomField; use MailPoet\Models\Segment; use MailPoet\Models\Subscriber; @@ -16,6 +17,7 @@ use MailPoet\Statistics\Track\Unsubscribes; use MailPoet\Subscribers\LinkTokens; use MailPoet\Subscribers\NewSubscriberNotificationMailer; use MailPoet\Subscribers\SubscriberSegmentRepository; +use MailPoet\Subscribers\SubscribersRepository; use MailPoet\Util\Url as UrlHelper; class Manage { @@ -41,6 +43,9 @@ class Manage { /** @var WelcomeScheduler */ private $welcomeScheduler; + /** @var SubscribersRepository */ + private $subscribersRepository; + /** @var SubscriberSegmentRepository */ private $subscriberSegmentRepository; @@ -52,6 +57,7 @@ class Manage { SettingsController $settings, NewSubscriberNotificationMailer $newSubscriberNotificationMailer, WelcomeScheduler $welcomeScheduler, + SubscribersRepository $subscribersRepository, SubscriberSegmentRepository $subscriberSegmentRepository ) { $this->urlHelper = $urlHelper; @@ -61,6 +67,7 @@ class Manage { $this->settings = $settings; $this->newSubscriberNotificationMailer = $newSubscriberNotificationMailer; $this->welcomeScheduler = $welcomeScheduler; + $this->subscribersRepository = $subscribersRepository; $this->subscriberSegmentRepository = $subscriberSegmentRepository; } @@ -76,25 +83,29 @@ class Manage { $result = []; if (!empty($subscriberData['email'])) { - $subscriber = Subscriber::where('email', $subscriberData['email'])->findOne(); + $subscriber = $this->subscribersRepository->findOneBy(['email' => $subscriberData['email']]); if ( ($subscriberData['status'] === SubscriberEntity::STATUS_UNSUBSCRIBED) - && ($subscriber instanceof Subscriber) - && ($subscriber->status === SubscriberEntity::STATUS_SUBSCRIBED) + && ($subscriber instanceof SubscriberEntity) + && ($subscriber->getStatus() === SubscriberEntity::STATUS_SUBSCRIBED) ) { $this->unsubscribesTracker->track( - (int)$subscriber->id, + (int)$subscriber->getId(), StatisticsUnsubscribeEntity::SOURCE_MANAGE ); } if ($subscriber && $this->linkTokens->verifyToken($subscriber, $token)) { + $subscriberModel = Subscriber::findOne($subscriber->getId()); + if (!$subscriberModel instanceof Subscriber) { + throw new InvalidStateException(); + } if ($subscriberData['email'] !== Pages::DEMO_EMAIL) { - $this->updateSubscriptions($subscriber, $subscriberData); + $this->updateSubscriptions($subscriberModel, $subscriberData); unset($subscriberData['segments']); - $subscriber = Subscriber::createOrUpdate($this->filterOutEmptyMandatoryFields($subscriberData)); - $subscriber->getErrors(); + $subscriberModel = Subscriber::createOrUpdate($this->filterOutEmptyMandatoryFields($subscriberData)); + $subscriberModel->getErrors(); } } $result = ['success' => true]; diff --git a/lib/Subscription/Pages.php b/lib/Subscription/Pages.php index 09d686e6e5..0d37184945 100644 --- a/lib/Subscription/Pages.php +++ b/lib/Subscription/Pages.php @@ -12,6 +12,7 @@ use MailPoet\Settings\SettingsController; use MailPoet\Statistics\Track\Unsubscribes; use MailPoet\Subscribers\LinkTokens; use MailPoet\Subscribers\NewSubscriberNotificationMailer; +use MailPoet\Subscribers\SubscribersRepository; use MailPoet\Util\Helpers; use MailPoet\WP\Functions as WPFunctions; @@ -60,6 +61,9 @@ class Pages { /** @var ManageSubscriptionFormRenderer */ private $manageSubscriptionFormRenderer; + /** @var SubscribersRepository */ + private $subscribersRepository; + public function __construct( NewSubscriberNotificationMailer $newSubscriberNotificationSender, WPFunctions $wp, @@ -71,7 +75,8 @@ class Pages { AssetsController $assetsController, TemplateRenderer $templateRenderer, Unsubscribes $unsubscribesTracker, - ManageSubscriptionFormRenderer $manageSubscriptionFormRenderer + ManageSubscriptionFormRenderer $manageSubscriptionFormRenderer, + SubscribersRepository $subscribersRepository ) { $this->wp = $wp; $this->newSubscriberNotificationSender = $newSubscriberNotificationSender; @@ -84,6 +89,7 @@ class Pages { $this->templateRenderer = $templateRenderer; $this->unsubscribesTracker = $unsubscribesTracker; $this->manageSubscriptionFormRenderer = $manageSubscriptionFormRenderer; + $this->subscribersRepository = $subscribersRepository; } public function init($action = false, $data = [], $initShortcodes = false, $initPageFilters = false) { @@ -135,7 +141,8 @@ class Pages { } $subscriber = Subscriber::where('email', $email)->findOne(); - return ($subscriber && $this->linkTokens->verifyToken($subscriber, $token)) ? $subscriber : null; + $subscriberEntity = $subscriber ? $this->subscribersRepository->findOneById($subscriber->id) : null; + return ($subscriber && $subscriberEntity && $this->linkTokens->verifyToken($subscriberEntity, $token)) ? $subscriber : null; } public function confirm() { diff --git a/tests/integration/Newsletter/ViewInBrowser/ViewInBrowserControllerTest.php b/tests/integration/Newsletter/ViewInBrowser/ViewInBrowserControllerTest.php index 899e87051c..e1a8d1d744 100644 --- a/tests/integration/Newsletter/ViewInBrowser/ViewInBrowserControllerTest.php +++ b/tests/integration/Newsletter/ViewInBrowser/ViewInBrowserControllerTest.php @@ -66,16 +66,14 @@ class ViewInBrowserControllerTest extends \MailPoetTest { $sendingTask->setSubscribers([$subscriber->getId()]); $sendingTask->updateProcessedSubscribers([$subscriber->getId()]); $this->sendingTask = $sendingTask->save(); - $linkTokens = new LinkTokens; // build browser preview data - $subscriberModel = Subscriber::findOne($subscriber->getId()); $this->browserPreviewData = [ 'queue_id' => $sendingTask->queue()->id, 'subscriber_id' => $subscriber->getId(), 'newsletter_id' => $newsletter->id, 'newsletter_hash' => $newsletter->hash, - 'subscriber_token' => $linkTokens->getToken($subscriberModel), + 'subscriber_token' => $this->linkTokens->getToken($subscriber), 'preview' => false, ]; } diff --git a/tests/integration/Router/Endpoints/TrackTest.php b/tests/integration/Router/Endpoints/TrackTest.php index ea36ffc604..f30f909368 100644 --- a/tests/integration/Router/Endpoints/TrackTest.php +++ b/tests/integration/Router/Endpoints/TrackTest.php @@ -12,7 +12,6 @@ use MailPoet\Entities\SubscriberEntity; use MailPoet\Models\Newsletter; use MailPoet\Models\NewsletterLink; use MailPoet\Models\SendingQueue; -use MailPoet\Models\Subscriber; use MailPoet\Newsletter\Sending\SendingQueuesRepository; use MailPoet\Router\Endpoints\Track; use MailPoet\Subscribers\LinkTokens; @@ -26,6 +25,9 @@ class TrackTest extends \MailPoetTest { public $subscriber; public $newsletter; + /** @var LinkTokens */ + private $linkTokens; + public function _before() { parent::_before(); $this->cleanup(); @@ -60,21 +62,20 @@ class TrackTest extends \MailPoetTest { $scheduledTaskSubscriber = new ScheduledTaskSubscriberEntity($task, $subscriber, 1); $this->entityManager->persist($scheduledTaskSubscriber); $this->entityManager->flush(); - $subscriberModel = Subscriber::findOne($subscriber->getId()); - $linkTokens = new LinkTokens; + $this->linkTokens = $this->diContainer->get(LinkTokens::class); // build track data $this->trackData = [ 'queue_id' => $queue->getId(), 'subscriber_id' => $subscriber->getId(), 'newsletter_id' => $newsletter->getId(), - 'subscriber_token' => $linkTokens->getToken($subscriberModel), + 'subscriber_token' => $this->linkTokens->getToken($subscriber), 'link_hash' => $link->getHash(), 'preview' => false, ]; $queue = SendingQueue::findOne($queue->getId()); assert($queue instanceof SendingQueue); $queue = SendingTask::createFromQueue($queue); - $queue->updateProcessedSubscribers([$subscriberModel->id]); + $queue->updateProcessedSubscribers([$subscriber->getId()]); // instantiate class $this->track = $this->diContainer->get(Track::class); } @@ -106,8 +107,8 @@ class TrackTest extends \MailPoetTest { $data->subscriber->setEmail('random@email.com'); $this->entityManager->flush(); $track = Stub::make(Track::class, [ - 'linkTokens' => new LinkTokens, 'sendingQueuesRepository' => $this->diContainer->get(SendingQueuesRepository::class), + 'linkTokens' => $this->linkTokens, 'terminate' => function($code) { expect($code)->equals(403); }, diff --git a/tests/integration/Subscription/ManageTest.php b/tests/integration/Subscription/ManageTest.php index 8b6501cfdc..5d62eb67a9 100644 --- a/tests/integration/Subscription/ManageTest.php +++ b/tests/integration/Subscription/ManageTest.php @@ -13,6 +13,7 @@ use MailPoet\Statistics\Track\Unsubscribes; use MailPoet\Subscribers\LinkTokens; use MailPoet\Subscribers\NewSubscriberNotificationMailer; use MailPoet\Subscribers\SubscriberSegmentRepository; +use MailPoet\Subscribers\SubscribersRepository; use MailPoet\Subscription\Manage; use MailPoet\Util\Url as UrlHelper; use MailPoetVendor\Idiorm\ORM; @@ -62,6 +63,7 @@ class ManageTest extends \MailPoetTest { $this->settings, $this->diContainer->get(NewSubscriberNotificationMailer::class), $this->diContainer->get(WelcomeScheduler::class), + $this->diContainer->get(SubscribersRepository::class), $this->diContainer->get(SubscriberSegmentRepository::class) ); $_POST['action'] = 'mailpoet_subscription_update'; diff --git a/tests/integration/Subscription/PagesTest.php b/tests/integration/Subscription/PagesTest.php index 716c192ab7..a2b258d1ca 100644 --- a/tests/integration/Subscription/PagesTest.php +++ b/tests/integration/Subscription/PagesTest.php @@ -3,9 +3,9 @@ namespace MailPoet\Test\Subscription; use Codeception\Stub; -use Codeception\Util\Fixtures; use MailPoet\Config\Renderer; use MailPoet\DI\ContainerWrapper; +use MailPoet\Entities\SubscriberEntity; use MailPoet\Form\AssetsController; use MailPoet\Models\Newsletter; use MailPoet\Models\NewsletterOption; @@ -21,6 +21,7 @@ use MailPoet\Settings\SettingsController; use MailPoet\Statistics\Track\Unsubscribes; use MailPoet\Subscribers\LinkTokens; use MailPoet\Subscribers\NewSubscriberNotificationMailer; +use MailPoet\Subscribers\SubscribersRepository; use MailPoet\Subscription\CaptchaRenderer; use MailPoet\Subscription\ManageSubscriptionFormRenderer; use MailPoet\Subscription\Pages; @@ -32,18 +33,24 @@ use MailPoetVendor\Idiorm\ORM; class PagesTest extends \MailPoetTest { private $testData = []; - /** @var Subscriber */ + /** @var SubscriberEntity */ private $subscriber; + /** @var SubscribersRepository */ + private $subscribersRepository; + public function _before() { parent::_before(); - $this->subscriber = Subscriber::create(); - $this->subscriber->hydrate(Fixtures::get('subscriber_template')); - $this->subscriber->status = Subscriber::STATUS_UNCONFIRMED; - $this->subscriber->save(); - $linkTokens = new LinkTokens; - expect($this->subscriber->getErrors())->false(); - $this->testData['email'] = $this->subscriber->email; + $this->subscribersRepository = $this->diContainer->get(SubscribersRepository::class); + $this->subscriber = new SubscriberEntity(); + $this->subscriber->setFirstName('John'); + $this->subscriber->setLastName('John'); + $this->subscriber->setEmail('john.doe@example.com'); + $this->subscriber->setStatus(Subscriber::STATUS_UNCONFIRMED); + $this->subscribersRepository->persist($this->subscriber); + $this->subscribersRepository->flush(); + $linkTokens = $this->diContainer->get(LinkTokens::class); + $this->testData['email'] = $this->subscriber->getEmail(); $this->testData['token'] = $linkTokens->getToken($this->subscriber); } @@ -52,7 +59,7 @@ class PagesTest extends \MailPoetTest { $pages = $this->getPages($newSubscriberNotificationSender); $subscription = $pages->init($action = false, $this->testData, false, false); $subscription->confirm(); - $confirmedSubscriber = Subscriber::findOne($this->subscriber->id); + $confirmedSubscriber = Subscriber::findOne($this->subscriber->getId()); expect($confirmedSubscriber->status)->equals(Subscriber::STATUS_SUBSCRIBED); expect($confirmedSubscriber->lastSubscribedAt)->greaterOrEquals(Carbon::createFromTimestamp((int)current_time('timestamp'))->subSecond()); expect($confirmedSubscriber->lastSubscribedAt)->lessOrEquals(Carbon::createFromTimestamp((int)current_time('timestamp'))->addSecond()); @@ -62,21 +69,23 @@ class PagesTest extends \MailPoetTest { $newSubscriberNotificationSender = $this->makeEmpty(NewSubscriberNotificationMailer::class, ['send' => Stub\Expected::never()]); $pages = $this->getPages($newSubscriberNotificationSender); $subscriber = $this->subscriber; - $subscriber->status = Subscriber::STATUS_SUBSCRIBED; - $subscriber->firstName = 'First name'; - $subscriber->unconfirmedData = '{"first_name" : "Updated first name", "email" : "' . $this->subscriber->email . '"}'; - $subscriber->lastSubscribedAt = Carbon::now()->subDays(10); - $subscriber->confirmedAt = Carbon::now()->subDays(10); - $subscriber->save(); + $subscriber->setStatus(Subscriber::STATUS_SUBSCRIBED); + $subscriber->setFirstName('First name'); + $subscriber->setUnconfirmedData('{"first_name" : "Updated first name", "email" : "' . $this->subscriber->getEmail() . '"}'); + $subscriber->setLastSubscribedAt(Carbon::now()->subDays(10)); + $subscriber->setConfirmedIp(Carbon::now()->subDays(10)); + $this->entityManager->flush(); $subscription = $pages->init($action = false, $this->testData, false, false); $subscription->confirm(); - $confirmedSubscriber = Subscriber::findOne($this->subscriber->id); - expect($confirmedSubscriber->status)->equals(Subscriber::STATUS_SUBSCRIBED); - expect($confirmedSubscriber->confirmedAt)->greaterOrEquals(Carbon::createFromTimestamp((int)current_time('timestamp'))->subSecond()); - expect($confirmedSubscriber->confirmedAt)->lessOrEquals(Carbon::createFromTimestamp((int)current_time('timestamp'))->addSecond()); - expect($confirmedSubscriber->lastSubscribedAt)->greaterOrEquals(Carbon::createFromTimestamp((int)current_time('timestamp'))->subSecond()); - expect($confirmedSubscriber->lastSubscribedAt)->lessOrEquals(Carbon::createFromTimestamp((int)current_time('timestamp'))->addSecond()); - expect($confirmedSubscriber->firstName)->equals('Updated first name'); + $this->entityManager->clear(); + $confirmedSubscriber = $this->subscribersRepository->findOneById($subscriber->getId()); + assert($confirmedSubscriber instanceof SubscriberEntity); + expect($confirmedSubscriber->getStatus())->equals(Subscriber::STATUS_SUBSCRIBED); + expect($confirmedSubscriber->getConfirmedAt())->greaterOrEquals(Carbon::createFromTimestamp((int)current_time('timestamp'))->subSecond()); + expect($confirmedSubscriber->getConfirmedAt())->lessOrEquals(Carbon::createFromTimestamp((int)current_time('timestamp'))->addSecond()); + expect($confirmedSubscriber->getLastSubscribedAt())->greaterOrEquals(Carbon::createFromTimestamp((int)current_time('timestamp'))->subSecond()); + expect($confirmedSubscriber->getLastSubscribedAt())->lessOrEquals(Carbon::createFromTimestamp((int)current_time('timestamp'))->addSecond()); + expect($confirmedSubscriber->getFirstName())->equals('Updated first name'); } public function testItSendsWelcomeNotificationUponConfirmingSubscription() { @@ -92,7 +101,7 @@ class PagesTest extends \MailPoetTest { $subscriberSegment = SubscriberSegment::create(); $subscriberSegment->hydrate( [ - 'subscriber_id' => $this->subscriber->id, + 'subscriber_id' => $this->subscriber->getId(), 'segment_id' => $segment->id, ] ); @@ -143,7 +152,7 @@ class PagesTest extends \MailPoetTest { public function testItUnsubscribes() { $pages = $this->getPages()->init($action = 'unsubscribe', $this->testData); $pages->unsubscribe(); - $updatedSubscriber = Subscriber::findOne($this->subscriber->id); + $updatedSubscriber = Subscriber::findOne($this->subscriber->getId()); expect($updatedSubscriber->status)->equals(Subscriber::STATUS_UNSUBSCRIBED); } @@ -167,7 +176,7 @@ class PagesTest extends \MailPoetTest { $this->testData['preview'] = 1; $pages = $this->getPages()->init($action = 'unsubscribe', $this->testData); $pages->unsubscribe(); - $updatedSubscriber = Subscriber::findOne($this->subscriber->id); + $updatedSubscriber = Subscriber::findOne($this->subscriber->getId()); expect($updatedSubscriber->status)->notEquals(Subscriber::STATUS_UNSUBSCRIBED); } @@ -199,7 +208,8 @@ class PagesTest extends \MailPoetTest { $container->get(AssetsController::class), $container->get(Renderer::class), $unsubscribesMock ?? $container->get(Unsubscribes::class), - $container->get(ManageSubscriptionFormRenderer::class) + $container->get(ManageSubscriptionFormRenderer::class), + $this->subscribersRepository ); } }