From 8e1c3b8c03527ae7aff9c0277c93a6c38f965ae5 Mon Sep 17 00:00:00 2001 From: Rostislav Wolny Date: Mon, 5 Aug 2019 08:55:16 +0200 Subject: [PATCH] Use CaptchaSession service instead of PHP Session [MAILPOET-2248] --- lib/API/JSON/v1/Subscribers.php | 24 ++++++++++++------- lib/Subscription/Captcha.php | 15 ++++++++---- lib/Subscription/CaptchaRenderer.php | 9 +++++-- .../API/JSON/v1/SubscribersTest.php | 9 ++++++- .../integration/Subscription/CaptchaTest.php | 9 +++++-- 5 files changed, 47 insertions(+), 19 deletions(-) diff --git a/lib/API/JSON/v1/Subscribers.php b/lib/API/JSON/v1/Subscribers.php index 061cb528ed..f8c02d3afc 100644 --- a/lib/API/JSON/v1/Subscribers.php +++ b/lib/API/JSON/v1/Subscribers.php @@ -20,6 +20,7 @@ use MailPoet\Subscribers\RequiredCustomFieldValidator; use MailPoet\Subscribers\Source; use MailPoet\Subscribers\SubscriberActions; use MailPoet\Subscription\Captcha; +use MailPoet\Subscription\CaptchaSession; use MailPoet\Subscription\Throttling as SubscriptionThrottling; use MailPoet\Subscription\Url as SubscriptionUrl; use MailPoet\WP\Functions as WPFunctions; @@ -58,6 +59,9 @@ class Subscribers extends APIEndpoint { /** @var SettingsController */ private $settings; + /** @var CaptchaSession */ + private $captcha_session; + public function __construct( Listing\BulkActionController $bulk_action_controller, SubscribersListings $subscribers_listings, @@ -66,7 +70,8 @@ class Subscribers extends APIEndpoint { Listing\Handler $listing_handler, Captcha $subscription_captcha, WPFunctions $wp, - SettingsController $settings + SettingsController $settings, + CaptchaSession $captcha_session ) { $this->bulk_action_controller = $bulk_action_controller; $this->subscribers_listings = $subscribers_listings; @@ -76,6 +81,7 @@ class Subscribers extends APIEndpoint { $this->subscription_captcha = $subscription_captcha; $this->wp = $wp; $this->settings = $settings; + $this->captcha_session = $captcha_session; } function get($data = []) { @@ -164,10 +170,10 @@ class Subscribers extends APIEndpoint { if (!empty($captcha_settings['type']) && $captcha_settings['type'] === Captcha::TYPE_BUILTIN) { if (!isset($data['captcha'])) { // Save form data to session - $_SESSION[Captcha::SESSION_FORM_KEY] = array_merge($data, ['form_id' => $form_id]); - } elseif (!empty($_SESSION[Captcha::SESSION_FORM_KEY])) { + $this->captcha_session->setFormData(array_merge($data, ['form_id' => $form_id])); + } elseif ($this->captcha_session->getFormData()) { // Restore form data from session - $data = array_merge($_SESSION[Captcha::SESSION_FORM_KEY], ['captcha' => $data['captcha']]); + $data = array_merge($this->captcha_session->getFormData(), ['captcha' => $data['captcha']]); } // Otherwise use the post data } @@ -222,8 +228,7 @@ class Subscribers extends APIEndpoint { } else { if (!empty($captcha_settings['type']) && $captcha_settings['type'] === Captcha::TYPE_BUILTIN) { // Captcha has been verified, invalidate the session vars - $_SESSION[Captcha::SESSION_KEY] = null; - $_SESSION[Captcha::SESSION_FORM_KEY] = null; + $this->captcha_session->reset(); } $meta = []; @@ -299,12 +304,13 @@ class Subscribers extends APIEndpoint { ]); } } elseif ($captcha_settings['type'] === Captcha::TYPE_BUILTIN && $is_builtin_captcha_required) { - if (empty($_SESSION[Captcha::SESSION_KEY])) { + $captcha_hash = $this->captcha_session->getCaptchaHash(); + if (empty($captcha_hash)) { return $this->badRequest([ APIError::BAD_REQUEST => WPFunctions::get()->__('Please regenerate the CAPTCHA.', 'mailpoet'), ]); - } elseif (!hash_equals(strtolower($data['captcha']), $_SESSION[Captcha::SESSION_KEY])) { - $_SESSION[Captcha::SESSION_KEY] = null; + } elseif (!hash_equals(strtolower($data['captcha']), $captcha_hash)) { + $this->captcha_session->setCaptchaHash(null); $meta = []; $meta['refresh_captcha'] = true; return $this->badRequest([ diff --git a/lib/Subscription/Captcha.php b/lib/Subscription/Captcha.php index 6ddb48e609..c2c754dc35 100644 --- a/lib/Subscription/Captcha.php +++ b/lib/Subscription/Captcha.php @@ -2,6 +2,7 @@ namespace MailPoet\Subscription; +use MailPoet\Config\Session; use MailPoet\Models\Subscriber; use MailPoet\Models\SubscriberIP; use MailPoet\Util\Helpers; @@ -13,17 +14,21 @@ class Captcha { const TYPE_RECAPTCHA = 'recaptcha'; const TYPE_DISABLED = null; - const SESSION_KEY = 'mailpoet_captcha'; - const SESSION_FORM_KEY = 'mailpoet_captcha_form'; - /** @var WPFunctions */ private $wp; - function __construct(WPFunctions $wp = null) { + /** @var CaptchaSession */ + private $captcha_session; + + function __construct(WPFunctions $wp = null, CaptchaSession $captcha_session = null) { if ($wp === null) { $wp = new WPFunctions; } + if ($captcha_session === null) { + $captcha_session = new CaptchaSession($wp, new Session()); + } $this->wp = $wp; + $this->captcha_session = $captcha_session; } function isSupported() { @@ -95,7 +100,7 @@ class Captcha { ->setMaxBehindLines(0) ->build($width ?: 220, $height ?: 60, $font); - $_SESSION[self::SESSION_KEY] = $builder->getPhrase(); + $this->captcha_session->setCaptchaHash($builder->getPhrase()); if ($return) { return $builder->get(); diff --git a/lib/Subscription/CaptchaRenderer.php b/lib/Subscription/CaptchaRenderer.php index 5360c03731..595136b162 100644 --- a/lib/Subscription/CaptchaRenderer.php +++ b/lib/Subscription/CaptchaRenderer.php @@ -14,9 +14,13 @@ class CaptchaRenderer { /** @var WPFunctions */ private $wp; - function __construct(UrlHelper $url_helper, WPFunctions $wp) { + /** @var CaptchaSession */ + private $captcha_session; + + function __construct(UrlHelper $url_helper, WPFunctions $wp, CaptchaSession $captcha_session) { $this->url_helper = $url_helper; $this->wp = $wp; + $this->captcha_session = $captcha_session; } public function getCaptchaPageTitle() { @@ -49,7 +53,8 @@ class CaptchaRenderer { ] ); - $form_id = isset($_SESSION[Captcha::SESSION_FORM_KEY]['form_id']) ? (int)$_SESSION[Captcha::SESSION_FORM_KEY]['form_id'] : 0; + $captcha_session_form = $this->captcha_session->getFormData(); + $form_id = isset($captcha_session_form['form_id']) ? (int)$captcha_session_form['form_id'] : 0; $form_model = FormModel::findOne($form_id); if (!$form_model instanceof FormModel) { return false; diff --git a/tests/integration/API/JSON/v1/SubscribersTest.php b/tests/integration/API/JSON/v1/SubscribersTest.php index fba45a74b2..424e881156 100644 --- a/tests/integration/API/JSON/v1/SubscribersTest.php +++ b/tests/integration/API/JSON/v1/SubscribersTest.php @@ -5,6 +5,7 @@ use Carbon\Carbon; use Codeception\Util\Fixtures; use MailPoet\API\JSON\v1\Subscribers; use MailPoet\API\JSON\Response as APIResponse; +use MailPoet\Config\Session; use MailPoet\DI\ContainerWrapper; use MailPoet\Form\Util\FieldNameObfuscator; use MailPoet\Models\CustomField; @@ -21,6 +22,8 @@ use MailPoet\Models\SubscriberSegment; use MailPoet\Settings\SettingsController; use MailPoet\Subscribers\Source; use MailPoet\Subscription\Captcha; +use MailPoet\Subscription\CaptchaSession; +use MailPoet\WP\Functions; class SubscribersTest extends \MailPoetTest { @@ -77,6 +80,9 @@ class SubscribersTest extends \MailPoetTest { 'address' => 'sender@mailpoet.com', 'name' => 'Sender', ]); + + // MAILPOET SESSION + $_COOKIE[Session::COOKIE_NAME] = 'abcd'; } function testItCanGetASubscriber() { @@ -517,7 +523,8 @@ class SubscribersTest extends \MailPoetTest { $subscriber->count_confirmations = 1; $subscriber->save(); $captcha_value = 'ihg5w'; - $_SESSION[Captcha::SESSION_KEY] = $captcha_value; + $captcha_session = new CaptchaSession(new Functions(), new Session()); + $captcha_session->setCaptchaHash($captcha_value); $response = $this->endpoint->subscribe([ $this->obfuscatedEmail => $email, 'form_id' => $this->form->id, diff --git a/tests/integration/Subscription/CaptchaTest.php b/tests/integration/Subscription/CaptchaTest.php index 85f6bbef50..d7023a8a1c 100644 --- a/tests/integration/Subscription/CaptchaTest.php +++ b/tests/integration/Subscription/CaptchaTest.php @@ -3,10 +3,13 @@ namespace MailPoet\Test\Subscription; use Carbon\Carbon; use Codeception\Util\Fixtures; +use MailPoet\Config\Session; use MailPoet\Models\Subscriber; use MailPoet\Models\SubscriberIP; use MailPoet\Subscription\Captcha; +use MailPoet\Subscription\CaptchaSession; use MailPoet\WP\Functions as WPFunctions; +use MailPoet\WP\Functions; class CaptchaTest extends \MailPoetTest { function _before() { @@ -40,10 +43,12 @@ class CaptchaTest extends \MailPoetTest { } function testItRendersImage() { - expect_that(empty($_SESSION[Captcha::SESSION_KEY])); + $_COOKIE[Session::COOKIE_NAME] = 'abcd'; + $captcha_session = new CaptchaSession(new Functions(), new Session()); + expect($captcha_session->getCaptchaHash())->false(); $image = $this->captcha->renderImage(null, null, true); expect($image)->notEmpty(); - expect_that(!empty($_SESSION[Captcha::SESSION_KEY])); + expect($captcha_session->getCaptchaHash())->notEmpty(); } function _after() {