From ac33e11c60addf8f7b130cdda5bc9e8d982893d3 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Tue, 21 Aug 2018 09:30:44 +0200 Subject: [PATCH] Reject requests without mandatory custom fields We need to make sure subscribers cannot be created without custom fields Users require GDPR consent and we need to make sure there are no way to create a subscriber without mandatory custom fields [MAILPOET-1405] --- lib/API/JSON/API.php | 8 +-- lib/API/JSON/v1/Subscribers.php | 10 +++- lib/API/MP/v1/API.php | 4 ++ .../RequiredCustomFieldValidator.php | 55 +++++++++++++++++++ tests/unit/API/JSON/v1/SubscribersTest.php | 17 ++++++ tests/unit/API/MP/APITest.php | 15 +++++ .../RequiredCustomFieldValidatorTest.php | 48 ++++++++++++++++ 7 files changed, 152 insertions(+), 5 deletions(-) create mode 100644 lib/Subscribers/RequiredCustomFieldValidator.php create mode 100644 tests/unit/Subscribers/RequiredCustomFieldValidatorTest.php diff --git a/lib/API/JSON/API.php b/lib/API/JSON/API.php index 081818c27c..22edf3446e 100644 --- a/lib/API/JSON/API.php +++ b/lib/API/JSON/API.php @@ -58,13 +58,13 @@ class API { $this->setRequestData($_POST); $ignoreToken = ( - Setting::getValue('re_captcha.enabled') && - $this->_request_endpoint === 'subscribers' && + Setting::getValue('re_captcha.enabled') && + $this->_request_endpoint === 'subscribers' && $this->_request_method === 'subscribe' - ); + ); if(!$ignoreToken && $this->checkToken() === false) { - $error_message = __('Sorry, but we couldn\'t connect to the MailPoet server. Please refresh the web page and try again.', 'mailpoet'); + $error_message = __("Sorry, but we couldn't connect to the MailPoet server. Please refresh the web page and try again.", 'mailpoet'); $error_response = $this->createErrorResponse(Error::UNAUTHORIZED, $error_message, Response::STATUS_UNAUTHORIZED); return $error_response->send(); } diff --git a/lib/API/JSON/v1/Subscribers.php b/lib/API/JSON/v1/Subscribers.php index bf1850b72a..1a1e4cec91 100644 --- a/lib/API/JSON/v1/Subscribers.php +++ b/lib/API/JSON/v1/Subscribers.php @@ -14,6 +14,7 @@ use MailPoet\Models\Subscriber; use MailPoet\Newsletter\Scheduler\Scheduler; use MailPoet\Segments\BulkAction; use MailPoet\Segments\SubscribersListings; +use MailPoet\Subscribers\RequiredCustomFieldValidator; use MailPoet\Subscribers\Source; use MailPoet\Subscription\Throttling as SubscriptionThrottling; use MailPoet\WP\Hooks; @@ -104,7 +105,7 @@ class Subscribers extends APIEndpoint { 'body' => array( 'secret' => $recaptcha['secret_token'], 'response' => $res - ) + ) )); if(is_wp_error($res)) { return $this->badRequest(array( @@ -121,6 +122,13 @@ class Subscribers extends APIEndpoint { $data = $this->deobfuscateFormPayload($data); + try { + $validator = new RequiredCustomFieldValidator(); + $validator->validate($data); + } catch (\Exception $e) { + return $this->badRequest([APIError::BAD_REQUEST => $e->getMessage()]); + } + $segment_ids = (!empty($data['segments']) ? (array)$data['segments'] : array() diff --git a/lib/API/MP/v1/API.php b/lib/API/MP/v1/API.php index 8c7d5a512c..31f800c230 100644 --- a/lib/API/MP/v1/API.php +++ b/lib/API/MP/v1/API.php @@ -6,6 +6,7 @@ use MailPoet\Models\Segment; use MailPoet\Models\Subscriber; use MailPoet\Models\SubscriberSegment; use MailPoet\Newsletter\Scheduler\Scheduler; +use MailPoet\Subscribers\RequiredCustomFieldValidator; use MailPoet\Subscribers\Source; use MailPoet\Tasks\Sending; @@ -164,6 +165,9 @@ class API { // if some required default fields are missing, set their values $default_fields = Subscriber::setRequiredFieldsDefaultValues($default_fields); + $validator = new RequiredCustomFieldValidator(); + $validator->validate($custom_fields); + // add subscriber $new_subscriber = Subscriber::create(); $new_subscriber->hydrate($default_fields); diff --git a/lib/Subscribers/RequiredCustomFieldValidator.php b/lib/Subscribers/RequiredCustomFieldValidator.php new file mode 100644 index 0000000000..29283c1e0e --- /dev/null +++ b/lib/Subscribers/RequiredCustomFieldValidator.php @@ -0,0 +1,55 @@ +getCustomFields(); + foreach($all_custom_fields as $custom_field_id => $custom_field_name) { + if($this->isCustomFieldMissing($custom_field_id, $data)) { + throw new \Exception( + __(sprintf('Missing value for custom field "%s"', $custom_field_name), 'mailpoet') + ); + } + } + } + + private function isCustomFieldMissing($custom_field_id, $data) { + if(!array_key_exists($custom_field_id, $data) && !array_key_exists('cf_' . $custom_field_id, $data)) { + return true; + } + if(isset($data[$custom_field_id]) && !$data[$custom_field_id]) { + return true; + } + if(isset($data['cf_' . $custom_field_id]) && !$data['cf_' . $custom_field_id]) { + return true; + } + return false; + } + + private function getCustomFields() { + $result = []; + + $required_custom_fields = CustomField::findMany(); + + foreach($required_custom_fields as $custom_field) { + if(is_serialized($custom_field->params)) { + $params = unserialize($custom_field->params); + if(is_array($params) && isset($params['required']) && $params['required']) { + $result[$custom_field->id] = $custom_field->name; + } + } + } + + return $result; + } + +} diff --git a/tests/unit/API/JSON/v1/SubscribersTest.php b/tests/unit/API/JSON/v1/SubscribersTest.php index 827ddebdac..9ae432479f 100644 --- a/tests/unit/API/JSON/v1/SubscribersTest.php +++ b/tests/unit/API/JSON/v1/SubscribersTest.php @@ -6,6 +6,7 @@ use Codeception\Util\Fixtures; use MailPoet\API\JSON\v1\Subscribers; use MailPoet\API\JSON\Response as APIResponse; use MailPoet\Form\Util\FieldNameObfuscator; +use MailPoet\Models\CustomField; use MailPoet\Models\Form; use MailPoet\Models\Newsletter; use MailPoet\Models\NewsletterOption; @@ -478,6 +479,21 @@ class SubscribersTest extends \MailPoetTest { Setting::setValue('re_captcha', array()); } + function testItCannotSubscribeWithoutMandatoryCustomField() { + CustomField::createOrUpdate([ + 'name' => 'custom field', + 'type' => 'text', + 'params' => ['required' => '1'] + ]); + $router = new Subscribers(); + $response = $router->subscribe(array( + $this->obfuscatedEmail => 'toto@mailpoet.com', + 'form_id' => $this->form->id, + $this->obfuscatedSegments => array($this->segment_1->id, $this->segment_2->id) + )); + expect($response->status)->equals(APIResponse::STATUS_BAD_REQUEST); + } + function testItCanSubscribeWithoutSegmentsIfTheyAreSelectedByAdmin() { $form = $this->form->asArray(); $form['settings']['segments_selected_by'] = 'admin'; @@ -642,5 +658,6 @@ class SubscribersTest extends \MailPoetTest { \ORM::raw_execute('TRUNCATE ' . Subscriber::$_table); \ORM::raw_execute('TRUNCATE ' . SubscriberSegment::$_table); \ORM::raw_execute('TRUNCATE ' . SubscriberIP::$_table); + \ORM::raw_execute('TRUNCATE ' . CustomField::$_table); } } diff --git a/tests/unit/API/MP/APITest.php b/tests/unit/API/MP/APITest.php index 71c6cc9ea6..919b6663c7 100644 --- a/tests/unit/API/MP/APITest.php +++ b/tests/unit/API/MP/APITest.php @@ -301,6 +301,21 @@ class APITest extends \MailPoetTest { expect($result['source'])->equals('api'); } + function testItChecksForMandatoryCustomFields() { + CustomField::createOrUpdate([ + 'name' => 'custom field', + 'type' => 'text', + 'params' => ['required' => '1'] + ]); + + $subscriber = array( + 'email' => 'test@example.com', + ); + + $this->setExpectedException('Exception'); + API::MP(self::VERSION)->addSubscriber($subscriber); + } + function testItSubscribesToSegmentsWhenAddingSubscriber() { $API = Stub::makeEmptyExcept( new \MailPoet\API\MP\v1\API(), diff --git a/tests/unit/Subscribers/RequiredCustomFieldValidatorTest.php b/tests/unit/Subscribers/RequiredCustomFieldValidatorTest.php new file mode 100644 index 0000000000..ad3ca79373 --- /dev/null +++ b/tests/unit/Subscribers/RequiredCustomFieldValidatorTest.php @@ -0,0 +1,48 @@ +custom_field = CustomField::createOrUpdate([ + 'name' => 'custom field', + 'type' => 'text', + 'params' => ['required' => '1'] + ]); + } + + function testItValidatesDataWithoutCustomField() { + $validator = new RequiredCustomFieldValidator(); + $this->setExpectedException('Exception'); + $validator->validate([]); + } + + function testItValidatesDataWithCustomFieldPassedAsId() { + $validator = new RequiredCustomFieldValidator(); + $validator->validate([$this->custom_field->id() => 'value']); + } + + function testItValidatesDataWithCustomFieldPassedAsCFId() { + $validator = new RequiredCustomFieldValidator(); + $validator->validate(['cf_' . $this->custom_field->id() => 'custom field']); + } + + function testItValidatesDataWithEmptyCustomField() { + $validator = new RequiredCustomFieldValidator(); + $this->setExpectedException('Exception'); + $validator->validate([$this->custom_field->id() => '']); + } + + function testItValidatesDataWithEmptyCustomFieldAsCFId() { + $validator = new RequiredCustomFieldValidator(); + $this->setExpectedException('Exception'); + $validator->validate(['cf_' . $this->custom_field->id() => '']); + } + +} \ No newline at end of file