Cavemanon/piratepoet
7
Fork 0
Code Pull Requests Actions Releases 16 Activity

Tweak Sending Service key validation after a code review [MAILPOET-743]

Browse Source 
* Abstract key state to unbound it from the API response codes
* Rename SendingServiceKeyCheck task for clarity
* Add a setter for the API key in the Bridge API class
* Make some smaller fixes
This commit is contained in:
Alexey Stoletniy
2017-01-23 23:40:20 +03:00
parent 425d45a862
commit 98d6f55a6e
13 changed files with 123 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
lib/API/Endpoints/Services.php
View File

@ -32,13 +32,13 @@ class Services extends APIEndpoint {
));
}
$code = !empty($result['code']) ? (int)$result['code'] : null;
$state = !empty($result['state']) ? $result['state'] : null;
if($code == Bridge::MAILPOET_KEY_VALID) {
if($state == Bridge::MAILPOET_KEY_VALID) {
return $this->successResponse(null);
}
switch($code) {
switch($state) {
case Bridge::MAILPOET_KEY_INVALID:
$error = __('Your MailPoet key is invalid!', 'mailpoet');
break;
@ -50,6 +50,7 @@ class Services extends APIEndpoint {
);
break;
default:
$code = !empty($result['code']) ? $result['code'] : Bridge::CHECK_ERROR_UNKNOWN;
$error = sprintf(
__('Error validating API key, please try again later (code: %s)', 'mailpoet'),
$code

2
lib/Config/Migrator.php
View File

@ -106,7 +106,7 @@ class Migrator {
function sendingQueues() {
$attributes = array(
'id mediumint(9) NOT NULL AUTO_INCREMENT,',
'type varchar(12) NULL DEFAULT NULL,',
'type varchar(90) NULL DEFAULT NULL,',
'newsletter_id mediumint(9) NOT NULL,',
'newsletter_rendered_body longtext,',
'newsletter_rendered_subject varchar(250) NULL DEFAULT NULL,',

12
lib/Config/ServicesChecker.php
View File

@ -15,12 +15,12 @@ class ServicesChecker {
return null;
}
$state = Setting::getValue(Bridge::API_KEY_STATE_SETTING_NAME);
if(empty($state['code']) || $state['code'] == Bridge::MAILPOET_KEY_VALID) {
$result = Setting::getValue(Bridge::API_KEY_STATE_SETTING_NAME);
if(empty($result['state']) || $result['state'] == Bridge::MAILPOET_KEY_VALID) {
return true;
}
if($state['code'] == Bridge::MAILPOET_KEY_INVALID) {
if($result['state'] == Bridge::MAILPOET_KEY_INVALID) {
$error = Helpers::replaceLinkTags(
__('All sending is currently paused! Your key to send with MailPoet is invalid. [link]Visit MailPoet.com to purchase a key[/link]', 'mailpoet'),
'https://account.mailpoet.com?s=' . Subscriber::getTotalSubscribers()
@ -29,10 +29,10 @@ class ServicesChecker {
WPNotice::displayError($error);
}
return false;
} elseif($state['code'] == Bridge::MAILPOET_KEY_EXPIRING
&& !empty($state['data']['expire_at'])
} elseif($result['state'] == Bridge::MAILPOET_KEY_EXPIRING
&& !empty($result['data']['expire_at'])
) {
$date = date('Y-m-d', strtotime($state['data']['expire_at']));
$date = date('Y-m-d', strtotime($result['data']['expire_at']));
$error = Helpers::replaceLinkTags(
__('Your newsletters are awesome! Don\'t forget to [link]upgrade your MailPoet email plan[/link] by %s to keep sending them to your subscribers.', 'mailpoet'),
'https://account.mailpoet.com?s=' . Subscriber::getTotalSubscribers()

8
lib/Cron/Daemon.php
View File

@ -3,7 +3,7 @@ namespace MailPoet\Cron;
use MailPoet\Cron\Workers\Scheduler as SchedulerWorker;
use MailPoet\Cron\Workers\SendingQueue\SendingQueue as SendingQueueWorker;
use MailPoet\Cron\Workers\Bounce as BounceWorker;
use MailPoet\Cron\Workers\SendingServiceKeyCheck as SSKeyCheckWorker;
use MailPoet\Cron\Workers\SendingServiceKeyCheck as SendingServiceKeyCheckWorker;
if(!defined('ABSPATH')) exit;
require_once(ABSPATH . 'wp-includes/pluggable.php');
@ -49,7 +49,7 @@ class Daemon {
try {
$this->executeScheduleWorker();
$this->executeQueueWorker();
$this->executeSSKeyCheckWorker();
$this->executeSendingServiceKeyCheckWorker();
$this->executeBounceWorker();
} catch(\Exception $e) {
// continue processing, no need to handle errors
@ -82,8 +82,8 @@ class Daemon {
return $queue->process();
}
function executeSSKeyCheckWorker() {
$worker = new SSKeyCheckWorker($this->timer);
function executeSendingServiceKeyCheckWorker() {
$worker = new SendingServiceKeyCheckWorker($this->timer);
return $worker->process();
}

15
lib/Cron/Triggers/WordPress.php
View File

@ -5,7 +5,7 @@ use MailPoet\Cron\CronHelper;
use MailPoet\Cron\Workers\Scheduler as SchedulerWorker;
use MailPoet\Cron\Workers\SendingQueue\SendingQueue as SendingQueueWorker;
use MailPoet\Cron\Workers\Bounce as BounceWorker;
use MailPoet\Cron\Workers\SendingServiceKeyCheck as SSKeyCheckWorker;
use MailPoet\Cron\Workers\SendingServiceKeyCheck as SendingServiceKeyCheckWorker;
use MailPoet\Mailer\MailerLog;
use MailPoet\Services\Bridge;
@ -29,11 +29,14 @@ class WordPress {
$bounce_due_queues = BounceWorker::getAllDueQueues();
$bounce_future_queues = BounceWorker::getFutureQueues();
// sending service key check
$sskeycheck_due_queues = SSKeyCheckWorker::getAllDueQueues();
$sskeycheck_future_queues = SSKeyCheckWorker::getFutureQueues();
return (($scheduled_queues || $running_queues) && !$sending_limit_reached)
|| ($mp_sending_enabled && ($bounce_due_queues || !$bounce_future_queues))
|| ($mp_sending_enabled && ($sskeycheck_due_queues || !$sskeycheck_future_queues));
$sskeycheck_due_queues = SendingServiceKeyCheckWorker::getAllDueQueues();
$sskeycheck_future_queues = SendingServiceKeyCheckWorker::getFutureQueues();
// check requirements for each worker
$sending_queue_active = (($scheduled_queues || $running_queues) && !$sending_limit_reached);
$bounce_sync_active = ($mp_sending_enabled && ($bounce_due_queues || !$bounce_future_queues));
$sending_service_key_check_active = ($mp_sending_enabled && ($sskeycheck_due_queues || !$sskeycheck_future_queues));
return ($sending_queue_active || $bounce_sync_active || $sending_service_key_check_active);
}
static function cleanup() {

6
lib/Cron/Workers/SendingServiceKeyCheck.php
View File

@ -10,7 +10,7 @@ use MailPoet\Services\Bridge;
if(!defined('ABSPATH')) exit;
class SendingServiceKeyCheck {
const TASK_TYPE = 'sskeycheck';
const TASK_TYPE = 'sending_service_key_check';
const UNAVAILABLE_SERVICE_RESCHEDULE_TIMEOUT = 60;
public $timer;
@ -89,10 +89,10 @@ class SendingServiceKeyCheck {
$mailer_config = Mailer::getMailerConfig();
$result = $this->bridge->checkKey($mailer_config['mailpoet_api_key']);
} catch (\Exception $e) {
$result = array('code' => 503);
$result = false;
}
if(empty($result['code']) || $result['code'] == 503) {
if(empty($result['code']) || $result['code'] == Bridge::CHECK_ERROR_UNAVAILABLE) {
// reschedule the check
$scheduled_at = Carbon::createFromTimestamp(current_time('timestamp'));
$queue->scheduled_at = $scheduled_at->addMinutes(

59
lib/Services/Bridge.php
View File

@ -9,15 +9,17 @@ if(!defined('ABSPATH')) exit;
class Bridge {
const API_KEY_STATE_SETTING_NAME = 'mta.mailpoet_api_key_state';
const MAILPOET_KEY_VALID = 200;
const MAILPOET_KEY_INVALID = 401;
const MAILPOET_KEY_EXPIRING = 402;
const MAILPOET_KEY_VALID = 'valid';
const MAILPOET_KEY_INVALID = 'invalid';
const MAILPOET_KEY_EXPIRING = 'expiring';
const MAILPOET_KEY_CHECK_ERROR = 'check_error';
const CHECK_ERROR_UNAVAILABLE = 503;
const CHECK_ERROR_UNKNOWN = 'unknown';
public $api;
function __construct() {
}
static function isMPSendingServiceEnabled() {
try {
$mailer_config = Mailer::getMailerConfig();
@ -30,7 +32,7 @@ class Bridge {
function initApi($api_key) {
if($this->api) {
$this->api->api_key = $api_key;
$this->api->setKey($api_key);
} else {
$this->api = new Bridge\API($api_key);
}
@ -43,20 +45,41 @@ class Bridge {
}
function processResult(array $result) {
if(empty($result['code'])) {
return false;
}
Setting::setValue(
self::API_KEY_STATE_SETTING_NAME,
array(
'code' => (int)$result['code'],
'data' => !empty($result['data']) ? $result['data'] : null,
)
$state_map = array(
200 => self::MAILPOET_KEY_VALID,
401 => self::MAILPOET_KEY_INVALID,
402 => self::MAILPOET_KEY_EXPIRING
);
return $result;
$update_settings = false;
if(!empty($result['code']) && isset($state_map[$result['code']])) {
$key_state = $state_map[$result['code']];
$update_settings = true;
} else {
$key_state = self::MAILPOET_KEY_CHECK_ERROR;
}
$state = array(
'state' => $key_state,
'data' => !empty($result['data']) ? $result['data'] : null,
'code' => !empty($result['code']) ? $result['code'] : self::CHECK_ERROR_UNKNOWN
);
if($update_settings) {
Setting::setValue(
self::API_KEY_STATE_SETTING_NAME,
$state
);
}
return $state;
}
static function invalidateKey() {
Setting::setValue(self::API_KEY_STATE_SETTING_NAME, array('code' => 401));
Setting::setValue(
self::API_KEY_STATE_SETTING_NAME,
array('state' => self::MAILPOET_KEY_INVALID)
);
}
}

6
lib/Services/Bridge/API.php
View File

@ -8,7 +8,7 @@ class API {
public $api_key;
function __construct($api_key) {
$this->api_key = $api_key;
$this->setKey($api_key);
}
function checkKey() {
@ -19,6 +19,10 @@ class API {
return $this->processResponse($result);
}
function setKey($api_key) {
$this->api_key = $api_key;
}
private function processResponse($result) {
$code = wp_remote_retrieve_response_code($result);
switch($code) {

10
tests/unit/API/Endpoints/ServicesTest.php
View File

@ -20,7 +20,7 @@ class ServicesTest extends MailPoetTest {
function testItRespondsWithSuccessIfKeyIsValid() {
$this->services_endpoint->bridge = Stub::make(
new Bridge(),
array('checkKey' => array('code' => Bridge::MAILPOET_KEY_VALID)),
array('checkKey' => array('state' => Bridge::MAILPOET_KEY_VALID)),
$this
);
$response = $this->services_endpoint->verifyMailPoetKey($this->data);
@ -30,7 +30,7 @@ class ServicesTest extends MailPoetTest {
function testItRespondsWithErrorIfKeyIsInvalid() {
$this->services_endpoint->bridge = Stub::make(
new Bridge(),
array('checkKey' => array('code' => Bridge::MAILPOET_KEY_INVALID)),
array('checkKey' => array('state' => Bridge::MAILPOET_KEY_INVALID)),
$this
);
$response = $this->services_endpoint->verifyMailPoetKey($this->data);
@ -42,7 +42,7 @@ class ServicesTest extends MailPoetTest {
$this->services_endpoint->bridge = Stub::make(
new Bridge(),
array('checkKey' => array(
'code' => Bridge::MAILPOET_KEY_EXPIRING,
'state' => Bridge::MAILPOET_KEY_EXPIRING,
'data' => array('expire_at' => $date->format('c'))
)),
$this
@ -55,11 +55,11 @@ class ServicesTest extends MailPoetTest {
function testItRespondsWithErrorIfServiceIsUnavailable() {
$this->services_endpoint->bridge = Stub::make(
new Bridge(),
array('checkKey' => array('code' => 503)),
array('checkKey' => array('code' => Bridge::CHECK_ERROR_UNAVAILABLE)),
$this
);
$response = $this->services_endpoint->verifyMailPoetKey($this->data);
expect($response->status)->equals(APIResponse::STATUS_NOT_FOUND);
expect($response->errors[0]['message'])->contains('503');
expect($response->errors[0]['message'])->contains((string)Bridge::CHECK_ERROR_UNAVAILABLE);
}
}

9
tests/unit/Config/ServicesCheckerTest.php
View File

@ -16,14 +16,14 @@ class ServicesCheckerTest extends MailPoetTest {
$this->setMailPoetSendingMethod();
Setting::setValue(
Bridge::API_KEY_STATE_SETTING_NAME,
array('code' => Bridge::MAILPOET_KEY_VALID)
array('state' => Bridge::MAILPOET_KEY_VALID)
);
$result = ServicesChecker::checkMailPoetAPIKeyValid();
expect($result)->true();
Setting::setValue(
Bridge::API_KEY_STATE_SETTING_NAME,
array('code' => Bridge::MAILPOET_KEY_INVALID)
array('state' => Bridge::MAILPOET_KEY_INVALID)
);
$result = ServicesChecker::checkMailPoetAPIKeyValid();
expect($result)->false();
@ -31,17 +31,18 @@ class ServicesCheckerTest extends MailPoetTest {
Setting::setValue(
Bridge::API_KEY_STATE_SETTING_NAME,
array(
'code' => Bridge::MAILPOET_KEY_EXPIRING,
'state' => Bridge::MAILPOET_KEY_EXPIRING,
'data' => array('expire_at' => date('c'))
)
);
$result = ServicesChecker::checkMailPoetAPIKeyValid();
expect($result)->true();
// unexpected state should be treated as valid
Setting::setValue(
Bridge::API_KEY_STATE_SETTING_NAME,
array(
'code' => 503
'state' => 'unexpected'
)
);
$result = ServicesChecker::checkMailPoetAPIKeyValid();

48
tests/unit/Cron/Workers/SendingServiceKeyCheckTest.php
View File

@ -3,7 +3,7 @@
use Carbon\Carbon;
use Codeception\Util\Stub;
use MailPoet\Cron\CronHelper;
use MailPoet\Cron\Workers\SendingServiceKeyCheck as SSKeyCheck;
use MailPoet\Cron\Workers\SendingServiceKeyCheck;
use MailPoet\Mailer\Mailer;
use MailPoet\Models\SendingQueue;
use MailPoet\Models\Setting;
@ -17,7 +17,7 @@ class SendingServiceKeyCheckTest extends MailPoetTest {
'good_address@example.com'
);
$this->sskeycheck = new SSKeyCheck(microtime(true));
$this->sskeycheck = new SendingServiceKeyCheck(microtime(true));
}
function testItConstructs() {
@ -26,7 +26,7 @@ class SendingServiceKeyCheckTest extends MailPoetTest {
function testItThrowsExceptionWhenExecutionLimitIsReached() {
try {
$sskeycheck = new SSKeyCheck(microtime(true) - CronHelper::DAEMON_EXECUTION_LIMIT);
$sskeycheck = new SendingServiceKeyCheck(microtime(true) - CronHelper::DAEMON_EXECUTION_LIMIT);
self::fail('Maximum execution time limit exception was not thrown.');
} catch(\Exception $e) {
expect($e->getMessage())->equals('Maximum execution time has been reached.');
@ -34,33 +34,33 @@ class SendingServiceKeyCheckTest extends MailPoetTest {
}
function testItSchedulesSendingServiceKeyCheck() {
expect(SendingQueue::where('type', SSKeyCheck::TASK_TYPE)->findMany())->isEmpty();
SSKeyCheck::schedule();
expect(SendingQueue::where('type', SSKeyCheck::TASK_TYPE)->findMany())->notEmpty();
expect(SendingQueue::where('type', SendingServiceKeyCheck::TASK_TYPE)->findMany())->isEmpty();
SendingServiceKeyCheck::schedule();
expect(SendingQueue::where('type', SendingServiceKeyCheck::TASK_TYPE)->findMany())->notEmpty();
}
function testItDoesNotScheduleSendingServiceKeyCheckTwice() {
expect(count(SendingQueue::where('type', SSKeyCheck::TASK_TYPE)->findMany()))->equals(0);
SSKeyCheck::schedule();
expect(count(SendingQueue::where('type', SSKeyCheck::TASK_TYPE)->findMany()))->equals(1);
SSKeyCheck::schedule();
expect(count(SendingQueue::where('type', SSKeyCheck::TASK_TYPE)->findMany()))->equals(1);
expect(count(SendingQueue::where('type', SendingServiceKeyCheck::TASK_TYPE)->findMany()))->equals(0);
SendingServiceKeyCheck::schedule();
expect(count(SendingQueue::where('type', SendingServiceKeyCheck::TASK_TYPE)->findMany()))->equals(1);
SendingServiceKeyCheck::schedule();
expect(count(SendingQueue::where('type', SendingServiceKeyCheck::TASK_TYPE)->findMany()))->equals(1);
}
function testItCanGetScheduledQueues() {
expect(SSKeyCheck::getScheduledQueues())->isEmpty();
expect(SendingServiceKeyCheck::getScheduledQueues())->isEmpty();
$this->createScheduledQueue();
expect(SSKeyCheck::getScheduledQueues())->notEmpty();
expect(SendingServiceKeyCheck::getScheduledQueues())->notEmpty();
}
function testItCanGetRunningQueues() {
expect(SSKeyCheck::getRunningQueues())->isEmpty();
expect(SendingServiceKeyCheck::getRunningQueues())->isEmpty();
$this->createRunningQueue();
expect(SSKeyCheck::getRunningQueues())->notEmpty();
expect(SendingServiceKeyCheck::getRunningQueues())->notEmpty();
}
function testItCanGetAllDueQueues() {
expect(SSKeyCheck::getAllDueQueues())->isEmpty();
expect(SendingServiceKeyCheck::getAllDueQueues())->isEmpty();
// scheduled for now
$this->createScheduledQueue();
@ -78,15 +78,15 @@ class SendingServiceKeyCheckTest extends MailPoetTest {
$queue->status = SendingQueue::STATUS_COMPLETED;
$queue->save();
expect(count(SSKeyCheck::getAllDueQueues()))->equals(2);
expect(count(SendingServiceKeyCheck::getAllDueQueues()))->equals(2);
}
function testItCanGetFutureQueues() {
expect(SSKeyCheck::getFutureQueues())->isEmpty();
expect(SendingServiceKeyCheck::getFutureQueues())->isEmpty();
$queue = $this->createScheduledQueue();
$queue->scheduled_at = Carbon::createFromTimestamp(current_time('timestamp'))->addDays(7);
$queue->save();
expect(count(SSKeyCheck::getFutureQueues()))->notEmpty();
expect(count(SendingServiceKeyCheck::getFutureQueues()))->notEmpty();
}
function testItFailsToProcessWithoutMailPoetMethodSetUp() {
@ -111,7 +111,7 @@ class SendingServiceKeyCheckTest extends MailPoetTest {
expect($queue->status)->null();
}
function testItProcessesSSKeyCheckQueue() {
function testItProcessesSendingServiceKeyCheckQueue() {
$this->sskeycheck->bridge = Stub::make(
new Bridge,
array('checkKey' => array('code' => Bridge::MAILPOET_KEY_VALID)),
@ -141,7 +141,7 @@ class SendingServiceKeyCheckTest extends MailPoetTest {
function testItReschedulesCheckOnError() {
$this->sskeycheck->bridge = Stub::make(
new Bridge,
array('checkKey' => array('code' => 503)),
array('checkKey' => array('code' => Bridge::CHECK_ERROR_UNAVAILABLE)),
$this
);
$this->setMailPoetSendingMethod();
@ -154,7 +154,7 @@ class SendingServiceKeyCheckTest extends MailPoetTest {
function testItCalculatesNextRunDateWithinNextWeekBoundaries() {
$current_date = Carbon::createFromTimestamp(current_time('timestamp'));
$next_run_date = SSKeyCheck::getNextRunDate();
$next_run_date = SendingServiceKeyCheck::getNextRunDate();
$difference = $next_run_date->diffInDays($current_date);
// Subtract days left in the current week
$difference -= (Carbon::DAYS_PER_WEEK - $current_date->format('N'));
@ -174,7 +174,7 @@ class SendingServiceKeyCheckTest extends MailPoetTest {
private function createScheduledQueue() {
$queue = SendingQueue::create();
$queue->type = SSKeyCheck::TASK_TYPE;
$queue->type = SendingServiceKeyCheck::TASK_TYPE;
$queue->status = SendingQueue::STATUS_SCHEDULED;
$queue->scheduled_at = Carbon::createFromTimestamp(current_time('timestamp'));
$queue->newsletter_id = 0;
@ -184,7 +184,7 @@ class SendingServiceKeyCheckTest extends MailPoetTest {
private function createRunningQueue() {
$queue = SendingQueue::create();
$queue->type = SSKeyCheck::TASK_TYPE;
$queue->type = SendingServiceKeyCheck::TASK_TYPE;
$queue->status = null;
$queue->scheduled_at = Carbon::createFromTimestamp(current_time('timestamp'));
$queue->newsletter_id = 0;

18
tests/unit/Services/BridgeTest.php
View File

@ -40,30 +40,34 @@ class BridgeTest extends MailPoetTest {
function testItChecksValidKey() {
$result = $this->bridge->checkKey($this->valid_key);
expect($result)->notEmpty();
expect($result['code'])->equals(Bridge::MAILPOET_KEY_VALID);
expect($result['state'])->equals(Bridge::MAILPOET_KEY_VALID);
$result = $this->bridge->checkKey($this->invalid_key);
expect($result)->notEmpty();
expect($result['code'])->equals(Bridge::MAILPOET_KEY_INVALID);
expect($result['state'])->equals(Bridge::MAILPOET_KEY_INVALID);
$result = $this->bridge->checkKey($this->expiring_key);
expect($result)->notEmpty();
expect($result['code'])->equals(Bridge::MAILPOET_KEY_EXPIRING);
expect($result['state'])->equals(Bridge::MAILPOET_KEY_EXPIRING);
expect($result['data']['expire_at'])->notEmpty();
}
function testItReturnsFalseOnEmptyAPIResponseCode() {
function testItReturnsErrorStateOnEmptyAPIResponseCode() {
$api = Stub::make(new API(null), array('checkKey' => array()), $this);
$this->bridge->api = $api;
$result = $this->bridge->checkKey($this->valid_key);
expect($result)->false();
expect($result)->notEmpty();
expect($result['state'])->equals(Bridge::MAILPOET_KEY_CHECK_ERROR);
}
function testItInvalidatesKey() {
Setting::setValue(Bridge::API_KEY_STATE_SETTING_NAME, array('code' => 200));
Setting::setValue(
Bridge::API_KEY_STATE_SETTING_NAME,
array('state' => Bridge::MAILPOET_KEY_VALID)
);
Bridge::invalidateKey();
$value = Setting::getValue(Bridge::API_KEY_STATE_SETTING_NAME);
expect($value)->equals(array('code' => 401));
expect($value)->equals(array('state' => Bridge::MAILPOET_KEY_INVALID));
}
private function setMailPoetSendingMethod() {

6
tests/unit/Services/BridgeTestMockAPI.php
View File

@ -9,7 +9,7 @@ class MockAPI {
public $api_key;
function __construct($api_key) {
$this->api_key = $api_key;
$this->setKey($api_key);
}
function checkKey() {
@ -19,6 +19,10 @@ class MockAPI {
return $this->processResponse($code);
}
function setKey($api_key) {
$this->api_key = $api_key;
}
private function processResponse($code) {
switch($code) {
case 200: