From 115135427806a020754e0e81aef983eb13d98371 Mon Sep 17 00:00:00 2001 From: Vlad Date: Fri, 11 Aug 2017 12:15:12 -0400 Subject: [PATCH 01/24] Conditionally uses set_time_limit() when function is not disabled --- lib/Config/Env.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/Config/Env.php b/lib/Config/Env.php index e7619ca087..971310d965 100644 --- a/lib/Config/Env.php +++ b/lib/Config/Env.php @@ -1,6 +1,9 @@ get_charset_collate(); self::$db_source_name = self::dbSourceName(self::$db_host, self::$db_socket, self::$db_port, self::$db_charset); self::$db_timezone_offset = self::getDbTimezoneOffset(); + self::$required_permission = Hooks::applyFilters('mailpoet_access_minimum_required_permission', 'manage_options'); } private static function dbSourceName($host, $socket, $port, $charset) { From 632bce789482df52532cb6e77f09d3cc764c4929 Mon Sep 17 00:00:00 2001 From: Vlad Date: Sun, 13 Aug 2017 21:02:47 -0400 Subject: [PATCH 02/24] Adds AccessControl class that defines permissions for major plugin operations --- lib/Config/AccessControl.php | 81 ++++++++++++++ lib/Config/Env.php | 4 - lib/Config/Initializer.php | 1 + tests/unit/Config/AccessControlTest.php | 139 ++++++++++++++++++++++++ 4 files changed, 221 insertions(+), 4 deletions(-) create mode 100644 lib/Config/AccessControl.php create mode 100644 tests/unit/Config/AccessControlTest.php diff --git a/lib/Config/AccessControl.php b/lib/Config/AccessControl.php new file mode 100644 index 0000000000..f1c2fb9fa4 --- /dev/null +++ b/lib/Config/AccessControl.php @@ -0,0 +1,81 @@ + WPHooks::applyFilters( + 'mailpoet_permission_access_plugin', + array( + 'administrator', + 'editor' + ) + ), + self::PERMISSION_MANAGE_SETTINGS => WPHooks::applyFilters( + 'mailpoet_permission_manage_settings', + array( + 'administrator' + ) + ), + self::PERMISSION_MANAGE_EMAILS => WPHooks::applyFilters( + 'mailpoet_permission_manage_emails', + array( + 'administrator', + 'editor' + ) + ), + self::PERMISSION_MANAGE_SUBSCRIBERS => WPHooks::applyFilters( + 'mailpoet_permission_manage_subscribers', + array( + 'administrator' + ) + ), + self::PERMISSION_MANAGE_FORMS => WPHooks::applyFilters( + 'mailpoet_permission_manage_forms', + array( + 'administrator' + ) + ), + self::PERMISSION_MANAGE_SEGMENTS => WPHooks::applyFilters( + 'mailpoet_permission_manage_segments', + array( + 'administrator' + ) + ) + ); + } + + static function validatePermission($permission) { + if(empty(self::$permissions)) self::init(); + if(empty(self::$permissions[$permission])) return false; + $current_user = wp_get_current_user(); + $current_user_roles = $current_user->roles; + $permitted_roles = array_intersect( + $current_user_roles, + self::$permissions[$permission] + ); + return (!empty($permitted_roles)); + } +} \ No newline at end of file diff --git a/lib/Config/Env.php b/lib/Config/Env.php index 971310d965..56092b6c34 100644 --- a/lib/Config/Env.php +++ b/lib/Config/Env.php @@ -2,8 +2,6 @@ namespace MailPoet\Config; -use MailPoet\WP\Hooks; - if(!defined('ABSPATH')) exit; class Env { @@ -34,7 +32,6 @@ class Env { static $db_collation; static $db_charset_collate; static $db_timezone_offset; - static $required_permission; static function init($file, $version) { global $wpdb; @@ -72,7 +69,6 @@ class Env { self::$db_charset_collate = $wpdb->get_charset_collate(); self::$db_source_name = self::dbSourceName(self::$db_host, self::$db_socket, self::$db_port, self::$db_charset); self::$db_timezone_offset = self::getDbTimezoneOffset(); - self::$required_permission = Hooks::applyFilters('mailpoet_access_minimum_required_permission', 'manage_options'); } private static function dbSourceName($host, $socket, $port, $charset) { diff --git a/lib/Config/Initializer.php b/lib/Config/Initializer.php index 8b66c4a71d..9d4080a559 100644 --- a/lib/Config/Initializer.php +++ b/lib/Config/Initializer.php @@ -24,6 +24,7 @@ class Initializer { 'version' => '1.0.0' )) { Env::init($params['file'], $params['version']); + AccessControl::init(); } function init() { diff --git a/tests/unit/Config/AccessControlTest.php b/tests/unit/Config/AccessControlTest.php new file mode 100644 index 0000000000..cfc62ef23b --- /dev/null +++ b/tests/unit/Config/AccessControlTest.php @@ -0,0 +1,139 @@ + array( + 'administrator', + 'editor' + ), + 'manage_settings' => array( + 'administrator' + ), + 'manage_emails' => array( + 'administrator', + 'editor' + ), + 'manage_subscribers' => array( + 'administrator' + ), + 'manage_forms' => array( + 'administrator' + ), + 'manage_segments' => array( + 'administrator' + ) + ); + expect(AccessControl::getPermissions())->equals($default_permissions); + } + + function testItSetsCustomPermissionsUponInitialization() { + $custom_permissions = array( + 'custom_permissions' => array( + 'custom_role' + ) + ); + AccessControl::init($custom_permissions); + expect(AccessControl::$permissions)->equals($custom_permissions); + } + + function testItGetsPermissions() { + expect(AccessControl::getPermissions())->equals( + array( + 'access_plugin' => array( + 'administrator', + 'editor' + ), + 'manage_settings' => array( + 'administrator' + ), + 'manage_emails' => array( + 'administrator', + 'editor' + ), + 'manage_subscribers' => array( + 'administrator' + ), + 'manage_forms' => array( + 'administrator' + ), + 'manage_segments' => array( + 'administrator' + ) + ) + ); + } + + function testItAllowsSettingCustonPermissions() { + Hooks::addFilter( + 'mailpoet_permission_access_plugin', + function() { + return array('custom_access_plugin_role'); + } + ); + Hooks::addFilter( + 'mailpoet_permission_manage_settings', + function() { + return array('custom_manage_settings_role'); + } + ); + Hooks::addFilter( + 'mailpoet_permission_manage_emails', + function() { + return array('custom_manage_emails_role'); + } + ); + Hooks::addFilter( + 'mailpoet_permission_manage_subscribers', + function() { + return array('custom_manage_subscribers_role'); + } + ); + Hooks::addFilter( + 'mailpoet_permission_manage_forms', + function() { + return array('custom_manage_forms_role'); + } + ); + Hooks::addFilter( + 'mailpoet_permission_manage_segments', + function() { + return array('custom_manage_forms_role'); + } + ); + AccessControl::init(); + expect(AccessControl::$permissions)->equals( + array( + 'access_plugin' => array( + 'custom_access_plugin_role' + ), + 'manage_settings' => array( + 'custom_manage_settings_role' + ), + 'manage_emails' => array( + 'custom_manage_emails_role' + ), + 'manage_subscribers' => array( + 'custom_manage_subscribers_role' + ), + 'manage_forms' => array( + 'custom_manage_forms_role' + ), + 'manage_segments' => array( + 'custom_manage_forms_role' + ) + ) + ); + } + + function _after() { + WPHooksHelper::releaseAllHooks(); + } +} \ No newline at end of file From a241d0c7bc7442ed2a807a8d962676d9ddd1b72f Mon Sep 17 00:00:00 2001 From: Vlad Date: Sun, 13 Aug 2017 21:56:57 -0400 Subject: [PATCH 03/24] Modifies JSON API to use AccessControl --- lib/API/JSON/API.php | 26 ++++++++++++---------- lib/API/JSON/Endpoint.php | 11 ++++++--- lib/API/JSON/v1/AutomatedLatestContent.php | 6 +++++ lib/API/JSON/v1/CustomFields.php | 7 ++++++ lib/API/JSON/v1/Forms.php | 14 ++++++++---- lib/API/JSON/v1/ImportExport.php | 12 +++++++--- lib/API/JSON/v1/MP2Migrator.php | 22 +++++++++++------- lib/API/JSON/v1/Mailer.php | 7 ++++++ lib/API/JSON/v1/NewsletterTemplates.php | 8 ++++++- lib/API/JSON/v1/Newsletters.php | 16 ++++++++----- lib/API/JSON/v1/Segments.php | 10 +++++++-- lib/API/JSON/v1/SendingQueue.php | 10 +++++++-- lib/API/JSON/v1/Services.php | 6 ++++- lib/API/JSON/v1/Settings.php | 15 +++++++++---- lib/API/JSON/v1/Setup.php | 6 +++++ lib/API/JSON/v1/Subscribers.php | 14 +++++++----- 16 files changed, 139 insertions(+), 51 deletions(-) diff --git a/lib/API/JSON/API.php b/lib/API/JSON/API.php index 3d0b1ba03a..8df8eb5855 100644 --- a/lib/API/JSON/API.php +++ b/lib/API/JSON/API.php @@ -1,6 +1,7 @@ permissions; - if(array_key_exists($this->_request_method, $permissions) === false || - $permissions[$this->_request_method] !== Access::ALL - ) { - if($this->checkPermissions() === false) { - $error_message = __('You do not have the required permissions.', 'mailpoet'); - $error_response = $this->createErrorResponse(Error::FORBIDDEN, $error_message, Response::STATUS_FORBIDDEN); - return $error_response; - } + if(!$this->validatePermissions($this->_request_method, $endpoint->permissions)) { + $error_message = __('You do not have the required permissions.', 'mailpoet'); + $error_response = $this->createErrorResponse(Error::FORBIDDEN, $error_message, Response::STATUS_FORBIDDEN); + return $error_response; } - $response = $endpoint->{$this->_request_method}($this->_request_data); return $response; } catch(\Exception $e) { @@ -150,8 +145,15 @@ class API { } } - function checkPermissions() { - return current_user_can(Env::$required_permission); + function validatePermissions($request_method, $permissions) { + // if method permission is defined, validate it + if (!empty($permissions['methods'][$request_method])) { + return ($permissions['methods'][$request_method] === Access::ALL) ? + true : + AccessControl::validatePermission($permissions['methods'][$request_method]); + } + // use global permission + return AccessControl::validatePermission($permissions['global']); } function checkToken() { diff --git a/lib/API/JSON/Endpoint.php b/lib/API/JSON/Endpoint.php index 975dcc90f4..e75a3ded43 100644 --- a/lib/API/JSON/Endpoint.php +++ b/lib/API/JSON/Endpoint.php @@ -1,11 +1,16 @@ array(AccessControl::PERMISSION_MANAGE_SETTINGS), + 'methods' => array() + ); function successResponse( $data = array(), $meta = array(), $status = Response::STATUS_OK @@ -18,7 +23,7 @@ abstract class Endpoint { ) { if(empty($errors)) { $errors = array( - Error::UNKNOWN => __('An unknown error occurred.', 'mailpoet') + Error::UNKNOWN => __('An unknown error occurred.', 'mailpoet') ); } return new ErrorResponse($errors, $meta, $status); diff --git a/lib/API/JSON/v1/AutomatedLatestContent.php b/lib/API/JSON/v1/AutomatedLatestContent.php index 74d0ac4a29..e28be1642e 100644 --- a/lib/API/JSON/v1/AutomatedLatestContent.php +++ b/lib/API/JSON/v1/AutomatedLatestContent.php @@ -1,12 +1,18 @@ AccessControl::PERMISSION_MANAGE_EMAILS + ); function __construct() { $this->ALC = new \MailPoet\Newsletter\AutomatedLatestContent(); diff --git a/lib/API/JSON/v1/CustomFields.php b/lib/API/JSON/v1/CustomFields.php index 529e7d476a..823f0552a5 100644 --- a/lib/API/JSON/v1/CustomFields.php +++ b/lib/API/JSON/v1/CustomFields.php @@ -1,12 +1,19 @@ AccessControl::PERMISSION_MANAGE_FORMS + ); + function getAll() { $collection = CustomField::orderByAsc('created_at')->findMany(); $custom_fields = array_map(function($custom_field) { diff --git a/lib/API/JSON/v1/Forms.php b/lib/API/JSON/v1/Forms.php index 179d805836..4789b728be 100644 --- a/lib/API/JSON/v1/Forms.php +++ b/lib/API/JSON/v1/Forms.php @@ -1,17 +1,23 @@ AccessControl::PERMISSION_MANAGE_FORMS + ); + function get($data = array()) { $id = (isset($data['id']) ? (int)$data['id'] : false); $form = Form::findOne($id); diff --git a/lib/API/JSON/v1/ImportExport.php b/lib/API/JSON/v1/ImportExport.php index 8e4f3b89c0..62ff5dead0 100644 --- a/lib/API/JSON/v1/ImportExport.php +++ b/lib/API/JSON/v1/ImportExport.php @@ -1,13 +1,19 @@ AccessControl::PERMISSION_MANAGE_SUBSCRIBERS + ); + function getMailChimpLists($data) { try { $mailChimp = new MailChimp($data['api_key']); diff --git a/lib/API/JSON/v1/MP2Migrator.php b/lib/API/JSON/v1/MP2Migrator.php index 67d851ae21..aa3bd41d75 100644 --- a/lib/API/JSON/v1/MP2Migrator.php +++ b/lib/API/JSON/v1/MP2Migrator.php @@ -1,18 +1,24 @@ AccessControl::PERMISSION_MANAGE_SETTINGS + ); + public function __construct() { $this->MP2Migrator = new \MailPoet\Config\MP2Migrator(); } - + /** * Import end point - * + * * @param object $data * @return object */ @@ -26,10 +32,10 @@ class MP2Migrator extends APIEndpoint { )); } } - + /** * Stop import end point - * + * * @param object $data * @return object */ @@ -43,10 +49,10 @@ class MP2Migrator extends APIEndpoint { )); } } - + /** * Skip import end point - * + * * @param object $data * @return object */ @@ -60,5 +66,5 @@ class MP2Migrator extends APIEndpoint { )); } } - + } diff --git a/lib/API/JSON/v1/Mailer.php b/lib/API/JSON/v1/Mailer.php index 4400453299..872ac860ff 100644 --- a/lib/API/JSON/v1/Mailer.php +++ b/lib/API/JSON/v1/Mailer.php @@ -1,12 +1,19 @@ AccessControl::PERMISSION_MANAGE_EMAILS + ); + function send($data = array()) { try { $mailer = new \MailPoet\Mailer\Mailer( diff --git a/lib/API/JSON/v1/NewsletterTemplates.php b/lib/API/JSON/v1/NewsletterTemplates.php index 69c4f5a4d5..fae112eaf1 100644 --- a/lib/API/JSON/v1/NewsletterTemplates.php +++ b/lib/API/JSON/v1/NewsletterTemplates.php @@ -1,13 +1,19 @@ AccessControl::PERMISSION_MANAGE_EMAILS + ); + function get($data = array()) { $id = (isset($data['id']) ? (int)$data['id'] : false); $template = NewsletterTemplate::findOne($id); diff --git a/lib/API/JSON/v1/Newsletters.php b/lib/API/JSON/v1/Newsletters.php index 282b8e5bb4..8fabcfeb44 100644 --- a/lib/API/JSON/v1/Newsletters.php +++ b/lib/API/JSON/v1/Newsletters.php @@ -1,16 +1,18 @@ AccessControl::PERMISSION_MANAGE_EMAILS + ); + function get($data = array()) { $id = (isset($data['id']) ? (int)$data['id'] : false); $newsletter = Newsletter::findOne($id); diff --git a/lib/API/JSON/v1/Segments.php b/lib/API/JSON/v1/Segments.php index 2ef6a07b89..6c8000564a 100644 --- a/lib/API/JSON/v1/Segments.php +++ b/lib/API/JSON/v1/Segments.php @@ -1,15 +1,21 @@ AccessControl::PERMISSION_MANAGE_SEGMENTS + ); + function get($data = array()) { $id = (isset($data['id']) ? (int)$data['id'] : false); $segment = Segment::findOne($id); diff --git a/lib/API/JSON/v1/SendingQueue.php b/lib/API/JSON/v1/SendingQueue.php index e155cbd0cc..5484e004a5 100644 --- a/lib/API/JSON/v1/SendingQueue.php +++ b/lib/API/JSON/v1/SendingQueue.php @@ -1,18 +1,24 @@ AccessControl::PERMISSION_MANAGE_EMAILS + ); + function add($data = array()) { $newsletter_id = (isset($data['newsletter_id']) ? (int)$data['newsletter_id'] diff --git a/lib/API/JSON/v1/Services.php b/lib/API/JSON/v1/Services.php index 7885e501ca..9fc447338f 100644 --- a/lib/API/JSON/v1/Services.php +++ b/lib/API/JSON/v1/Services.php @@ -1,11 +1,12 @@ AccessControl::PERMISSION_MANAGE_SETTINGS + ); function __construct() { $this->bridge = new Bridge(); diff --git a/lib/API/JSON/v1/Settings.php b/lib/API/JSON/v1/Settings.php index 1a2093c4ed..1db60103da 100644 --- a/lib/API/JSON/v1/Settings.php +++ b/lib/API/JSON/v1/Settings.php @@ -1,24 +1,31 @@ AccessControl::PERMISSION_MANAGE_SETTINGS + ); + function get() { return $this->successResponse(Setting::getAll()); } function set($settings = array()) { if(empty($settings)) { - return $this->badRequest(array( - APIError::BAD_REQUEST => - __('You have not specified any settings to be saved.', 'mailpoet') - )); + return $this->badRequest( + array( + APIError::BAD_REQUEST => + __('You have not specified any settings to be saved.', 'mailpoet') + )); } else { foreach($settings as $name => $value) { Setting::setValue($name, $value); diff --git a/lib/API/JSON/v1/Setup.php b/lib/API/JSON/v1/Setup.php index d0f6989226..1bf0647748 100644 --- a/lib/API/JSON/v1/Setup.php +++ b/lib/API/JSON/v1/Setup.php @@ -1,13 +1,19 @@ AccessControl::PERMISSION_MANAGE_SETTINGS + ); + function reset() { try { $activator = new Activator(); diff --git a/lib/API/JSON/v1/Subscribers.php b/lib/API/JSON/v1/Subscribers.php index 38ddefb9af..e391072855 100644 --- a/lib/API/JSON/v1/Subscribers.php +++ b/lib/API/JSON/v1/Subscribers.php @@ -1,21 +1,23 @@ APIAccess::ALL + 'global' => AccessControl::PERMISSION_MANAGE_SUBSCRIBERS, + 'methods' => array('subscribe' => APIAccess::ALL) ); function get($data = array()) { From c3c6ce989c7ddb1eb1b0047aff42a9d9e5591448 Mon Sep 17 00:00:00 2001 From: Vlad Date: Sun, 13 Aug 2017 22:00:36 -0400 Subject: [PATCH 04/24] Modifies Menu to use AccessControl --- lib/Config/Menu.php | 61 ++++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/lib/Config/Menu.php b/lib/Config/Menu.php index 2edcc1d5b1..ded750dc66 100644 --- a/lib/Config/Menu.php +++ b/lib/Config/Menu.php @@ -1,4 +1,5 @@ assets_url . '/img/menu_icon.png', @@ -66,7 +68,7 @@ class Menu { $main_page_slug, $this->setPageTitle(__('Emails', 'mailpoet')), __('Emails', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_EMAILS), $main_page_slug, array( $this, @@ -90,7 +92,7 @@ class Menu { $main_page_slug, $this->setPageTitle(__('Forms', 'mailpoet')), __('Forms', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_FORMS), 'mailpoet-forms', array( $this, @@ -113,7 +115,7 @@ class Menu { $main_page_slug, $this->setPageTitle(__('Subscribers', 'mailpoet')), __('Subscribers', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_SUBSCRIBERS), 'mailpoet-subscribers', array( $this, @@ -136,7 +138,7 @@ class Menu { $main_page_slug, $this->setPageTitle(__('Lists', 'mailpoet')), __('Lists', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_SEGMENTS), 'mailpoet-segments', array( $this, @@ -160,7 +162,7 @@ class Menu { $main_page_slug, $this->setPageTitle(__('Settings', 'mailpoet')), __('Settings', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_SETTINGS), 'mailpoet-settings', array( $this, @@ -172,7 +174,7 @@ class Menu { $main_page_slug, $this->setPageTitle(__('Help', 'mailpoet')), __('Help', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_ACCESS_PLUGIN), 'mailpoet-help', array( $this, @@ -185,7 +187,7 @@ class Menu { License::getLicense() ? true : $main_page_slug, $this->setPageTitle(__('Premium', 'mailpoet')), __('Premium', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_ACCESS_PLUGIN), 'mailpoet-premium', array( $this, @@ -197,7 +199,7 @@ class Menu { 'admin.php?page=mailpoet-subscribers', $this->setPageTitle(__('Import', 'mailpoet')), __('Import', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_SUBSCRIBERS), 'mailpoet-import', array( $this, @@ -209,7 +211,7 @@ class Menu { true, $this->setPageTitle(__('Export', 'mailpoet')), __('Export', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_SUBSCRIBERS), 'mailpoet-export', array( $this, @@ -221,7 +223,7 @@ class Menu { true, $this->setPageTitle(__('Welcome', 'mailpoet')), __('Welcome', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_ACCESS_PLUGIN), 'mailpoet-welcome', array( $this, @@ -229,23 +231,11 @@ class Menu { ) ); - add_submenu_page( - true, - $this->setPageTitle(__('Migration', 'mailpoet')), - '', - Env::$required_permission, - 'mailpoet-migration', - array( - $this, - 'migration' - ) - ); - add_submenu_page( true, $this->setPageTitle(__('Update', 'mailpoet')), __('Update', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_ACCESS_PLUGIN), 'mailpoet-update', array( $this, @@ -253,11 +243,23 @@ class Menu { ) ); + add_submenu_page( + true, + $this->setPageTitle(__('Migration', 'mailpoet')), + '', + AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_SETTINGS), + 'mailpoet-migration', + array( + $this, + 'migration' + ) + ); + add_submenu_page( true, $this->setPageTitle(__('Form Editor', 'mailpoet')), __('Form Editor', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_FORMS), 'mailpoet-form-editor', array( $this, @@ -269,7 +271,7 @@ class Menu { true, $this->setPageTitle(__('Newsletter', 'mailpoet')), __('Newsletter Editor', 'mailpoet'), - Env::$required_permission, + AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_EMAILS), 'mailpoet-newsletter-editor', array( $this, @@ -611,9 +613,12 @@ class Menu { true, 'MailPoet', 'MailPoet', - Env::$required_permission, + Env::$use_plugin_permission, $_REQUEST['page'], - array(__CLASS__, 'errorPageCallback') + array( + __CLASS__, + 'errorPageCallback' + ) ); } From 51fbf290314f669c4e210a47806142a015a50693 Mon Sep 17 00:00:00 2001 From: Vlad Date: Sun, 13 Aug 2017 22:02:00 -0400 Subject: [PATCH 05/24] Modifies Activator to use AccessControl --- lib/Config/Activator.php | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/Config/Activator.php b/lib/Config/Activator.php index e1834203a6..c654e96bf7 100644 --- a/lib/Config/Activator.php +++ b/lib/Config/Activator.php @@ -1,10 +1,17 @@ up(); @@ -15,7 +22,18 @@ class Activator { } static function deactivate() { + self::validatePermission(); $migrator = new Migrator(); $migrator->down(); } -} + + static function validatePermission() { + if(AccessControl::validatePermission(self::REQUIRED_PERMISSION)) return; + throw new \Exception( + sprintf( + __('MailPoet can only be activated/deactivated by a user with %s capability.', 'mailpoet'), + self::REQUIRED_PERMISSION + ) + ); + } +} \ No newline at end of file From 2e5554a3afed78b7494dc9decf5d0e19bc814861 Mon Sep 17 00:00:00 2001 From: Vlad Date: Mon, 14 Aug 2017 11:28:31 -0400 Subject: [PATCH 06/24] Refactors AccessControl and passes it as dependency to JSON API and Menu --- lib/API/JSON/API.php | 11 +- lib/API/JSON/Access.php | 12 - lib/API/JSON/v1/MP2Migrator.php | 5 +- lib/API/JSON/v1/Setup.php | 7 +- lib/API/JSON/v1/Subscribers.php | 3 +- lib/Config/AccessControl.php | 48 ++-- lib/Config/Activator.php | 26 +- lib/Config/Changelog.php | 7 +- lib/Config/Initializer.php | 13 +- lib/Config/MP2Migrator.php | 9 +- lib/Config/Menu.php | 380 ++++++++++++++----------- lib/Router/Endpoints/ViewInBrowser.php | 6 +- 12 files changed, 286 insertions(+), 241 deletions(-) delete mode 100644 lib/API/JSON/Access.php diff --git a/lib/API/JSON/API.php b/lib/API/JSON/API.php index 8df8eb5855..c7f7304d4a 100644 --- a/lib/API/JSON/API.php +++ b/lib/API/JSON/API.php @@ -2,7 +2,6 @@ namespace MailPoet\API\JSON; use MailPoet\Config\AccessControl; -use MailPoet\Config\Env; use MailPoet\Util\Helpers; use MailPoet\Util\Security; use MailPoet\WP\Hooks; @@ -20,9 +19,11 @@ class API { private $_available_api_versions = array( 'v1' ); + private $access_control; const CURRENT_VERSION = 'v1'; function __construct() { + $this->access_control = new AccessControl(); foreach($this->_available_api_versions as $available_api_version) { $this->addEndpointNamespace( sprintf('%s\%s', __NAMESPACE__, $available_api_version), @@ -127,7 +128,7 @@ class API { throw new \Exception(__('Invalid API endpoint.', 'mailpoet')); } - $endpoint = new $this->_request_endpoint_class(); + $endpoint = new $this->_request_endpoint_class($this->access_control); // check the accessibility of the requested endpoint's action // by default, an endpoint's action is considered "private" @@ -148,12 +149,12 @@ class API { function validatePermissions($request_method, $permissions) { // if method permission is defined, validate it if (!empty($permissions['methods'][$request_method])) { - return ($permissions['methods'][$request_method] === Access::ALL) ? + return ($permissions['methods'][$request_method] === AccessControl::ACCESS_ALL) ? true : - AccessControl::validatePermission($permissions['methods'][$request_method]); + $this->access_control->validatePermission($permissions['methods'][$request_method]); } // use global permission - return AccessControl::validatePermission($permissions['global']); + return $this->access_control->validatePermission($permissions['global']); } function checkToken() { diff --git a/lib/API/JSON/Access.php b/lib/API/JSON/Access.php deleted file mode 100644 index 28beab513e..0000000000 --- a/lib/API/JSON/Access.php +++ /dev/null @@ -1,12 +0,0 @@ - AccessControl::PERMISSION_MANAGE_SETTINGS ); - public function __construct() { - $this->MP2Migrator = new \MailPoet\Config\MP2Migrator(); + public function __construct(AccessControl $access_control) { + $this->access_control = $access_control; + $this->MP2Migrator = new \MailPoet\Config\MP2Migrator($this->access_control); } /** diff --git a/lib/API/JSON/v1/Setup.php b/lib/API/JSON/v1/Setup.php index 1bf0647748..8b333b4e8b 100644 --- a/lib/API/JSON/v1/Setup.php +++ b/lib/API/JSON/v1/Setup.php @@ -13,10 +13,15 @@ class Setup extends APIEndpoint { public $permissions = array( 'global' => AccessControl::PERMISSION_MANAGE_SETTINGS ); + private $access_control; + + function __construct(AccessControl $access_control) { + $this->access_control = $access_control; + } function reset() { try { - $activator = new Activator(); + $activator = new Activator($this->access_control); $activator->deactivate(); $activator->activate(); Hooks::doAction('mailpoet_setup_reset'); diff --git a/lib/API/JSON/v1/Subscribers.php b/lib/API/JSON/v1/Subscribers.php index e391072855..0631806343 100644 --- a/lib/API/JSON/v1/Subscribers.php +++ b/lib/API/JSON/v1/Subscribers.php @@ -2,7 +2,6 @@ namespace MailPoet\API\JSON\v1; -use MailPoet\API\JSON\Access as APIAccess; use MailPoet\API\JSON\Endpoint as APIEndpoint; use MailPoet\API\JSON\Error as APIError; use MailPoet\Config\AccessControl; @@ -17,7 +16,7 @@ if(!defined('ABSPATH')) exit; class Subscribers extends APIEndpoint { public $permissions = array( 'global' => AccessControl::PERMISSION_MANAGE_SUBSCRIBERS, - 'methods' => array('subscribe' => APIAccess::ALL) + 'methods' => array('subscribe' => AccessControl::ACCESS_ALL) ); function get($data = array()) { diff --git a/lib/Config/AccessControl.php b/lib/Config/AccessControl.php index f1c2fb9fa4..c56e43778c 100644 --- a/lib/Config/AccessControl.php +++ b/lib/Config/AccessControl.php @@ -8,23 +8,26 @@ if(!defined('ABSPATH')) exit; require_once(ABSPATH . 'wp-includes/pluggable.php'); class AccessControl { - static $permissions; const PERMISSION_ACCESS_PLUGIN = 'access_plugin'; const PERMISSION_MANAGE_SETTINGS = 'manage_settings'; const PERMISSION_MANAGE_EMAILS = 'manage_emails'; const PERMISSION_MANAGE_SUBSCRIBERS = 'manage_subscribers'; const PERMISSION_MANAGE_FORMS = 'manage_forms'; const PERMISSION_MANAGE_SEGMENTS = 'manage_segments'; + const PERMISSION_UPDATE_PLUGIN = 'update_plugin'; + const ACCESS_ALL = 'All'; - static function init($permissions = array()) { - self::setPermissions($permissions); + public $permissions; + public $current_user_roles; + public $user_capabilities; + + function __construct() { + $this->permissions = $this->getDefaultPermissions(); + $this->user_roles = $this->getUserRoles(); + $this->user_capabilities = $this->getUserCapabilities(); } - static function setPermissions($permissions = array()) { - self::$permissions = ($permissions) ? $permissions : self::getPermissions(); - } - - static function getPermissions() { + private function getDefaultPermissions() { return array( self::PERMISSION_ACCESS_PLUGIN => WPHooks::applyFilters( 'mailpoet_permission_access_plugin', @@ -63,18 +66,31 @@ class AccessControl { array( 'administrator' ) - ) + ), + self::PERMISSION_UPDATE_PLUGIN => WPHooks::applyFilters( + 'mailpoet_permission_update_plugin', + array( + 'administrator' + ) + ), ); } - static function validatePermission($permission) { - if(empty(self::$permissions)) self::init(); - if(empty(self::$permissions[$permission])) return false; - $current_user = wp_get_current_user(); - $current_user_roles = $current_user->roles; + function getUserRoles() { + $user = wp_get_current_user(); + return $user->roles; + } + + function getUserCapabilities() { + $user = wp_get_current_user(); + return array_keys($user->allcaps); + } + + function validatePermission($permission) { + if(empty($this->permissions[$permission])) return false; $permitted_roles = array_intersect( - $current_user_roles, - self::$permissions[$permission] + $this->user_roles, + $this->permissions[$permission] ); return (!empty($permitted_roles)); } diff --git a/lib/Config/Activator.php b/lib/Config/Activator.php index c654e96bf7..c849e4e264 100644 --- a/lib/Config/Activator.php +++ b/lib/Config/Activator.php @@ -5,13 +5,16 @@ namespace MailPoet\Config; if(!defined('ABSPATH')) exit; class Activator { - const REQUIRED_PERMISSION = AccessControl::PERMISSION_MANAGE_SETTINGS; + private $access_control; - static function activate() { - self::validatePermission(); - if(!current_user_can(self::PERMISSION_ACTIVATE)) { - throw new \Exception('MaiLpoet ID must be greater than zero'); + function __construct(AccessControl $access_control) { + $this->access_control = $access_control; + if(!$this->access_control->validatePermission(AccessControl::PERMISSION_UPDATE_PLUGIN)) { + throw new \Exception(__('You do not have permission to activate/deactivate MailPoet plugin.', 'mailpoet')); } + } + + function activate() { $migrator = new Migrator(); $migrator->up(); @@ -21,19 +24,8 @@ class Activator { update_option('mailpoet_db_version', Env::$version); } - static function deactivate() { - self::validatePermission(); + function deactivate() { $migrator = new Migrator(); $migrator->down(); } - - static function validatePermission() { - if(AccessControl::validatePermission(self::REQUIRED_PERMISSION)) return; - throw new \Exception( - sprintf( - __('MailPoet can only be activated/deactivated by a user with %s capability.', 'mailpoet'), - self::REQUIRED_PERMISSION - ) - ); - } } \ No newline at end of file diff --git a/lib/Config/Changelog.php b/lib/Config/Changelog.php index 59295e0beb..29b33f2e55 100644 --- a/lib/Config/Changelog.php +++ b/lib/Config/Changelog.php @@ -4,7 +4,10 @@ use MailPoet\Models\Setting; use MailPoet\Util\Url; class Changelog { - function __construct() { + private $access_control; + + function __construct(AccessControl $access_control) { + $this->access_control = $access_control; } function init() { @@ -34,7 +37,7 @@ class Changelog { $version = Setting::getValue('version', null); $redirect_url = null; - $mp2_migrator = new MP2Migrator(); + $mp2_migrator = new MP2Migrator($this->access_control); if(!in_array($_GET['page'], array('mailpoet-migration', 'mailpoet-settings')) && $mp2_migrator->isMigrationStartedAndNotCompleted()) { // Force the redirection if the migration has started but is not completed $redirect_url = admin_url('admin.php?page=mailpoet-migration'); diff --git a/lib/Config/Initializer.php b/lib/Config/Initializer.php index 9d4080a559..4e527c548d 100644 --- a/lib/Config/Initializer.php +++ b/lib/Config/Initializer.php @@ -13,18 +13,18 @@ if(!defined('ABSPATH')) exit; require_once(ABSPATH . 'wp-admin/includes/plugin.php'); class Initializer { - const UNABLE_TO_CONNECT = 'Unable to connect to the database (the database is unable to open a file or folder), the connection is likely not configured correctly. Please read our [link] Knowledge Base article [/link] for steps how to resolve it.'; const SOLVE_DB_ISSUE_URL = 'http://beta.docs.mailpoet.com/article/200-solving-database-connection-issues'; protected $plugin_initialized = false; + private $access_control; function __construct($params = array( 'file' => '', 'version' => '1.0.0' )) { Env::init($params['file'], $params['version']); - AccessControl::init(); + $this->access_control = new AccessControl(); } function init() { @@ -136,7 +136,8 @@ class Initializer { // if current db version and plugin version differ if(version_compare($current_db_version, Env::$version) !== 0) { - Activator::activate(); + $activator = new Activator($this->access_control); + $activator->activate(); } } @@ -186,12 +187,12 @@ class Initializer { } function setupMenu() { - $menu = new Menu($this->renderer, Env::$assets_url); + $menu = new Menu($this->renderer, Env::$assets_url, $this->access_control); $menu->init(); } function setupChangelog() { - $changelog = new Changelog(); + $changelog = new Changelog($this->access_control); $changelog->init(); } @@ -247,7 +248,7 @@ class Initializer { function handleFailedInitialization($exception) { // Check if we are able to add pages at this point if(function_exists('wp_get_current_user')) { - Menu::addErrorPage(); + Menu::addErrorPage($this->access_control); } return WPNotice::displayError($exception); } diff --git a/lib/Config/MP2Migrator.php b/lib/Config/MP2Migrator.php index 289be0bc66..e75fc6b8c5 100644 --- a/lib/Config/MP2Migrator.php +++ b/lib/Config/MP2Migrator.php @@ -19,17 +19,19 @@ class MP2Migrator { const CHUNK_SIZE = 10; // To import the data by batch private $log_file; + private $access_control; public $log_file_url; public $progressbar; private $segments_mapping = array(); // Mapping between old and new segment IDs private $wp_users_segment; - public function __construct() { + public function __construct(AccessControl $access_control) { $this->defineMP2Tables(); $log_filename = 'mp2migration.log'; $this->log_file = Env::$temp_path . '/' . $log_filename; $this->log_file_url = Env::$temp_url . '/' . $log_filename; $this->progressbar = new ProgressBar('mp2migration'); + $this->access_control = $access_control; } private function defineMP2Tables() { @@ -187,8 +189,9 @@ class MP2Migrator { * */ private function eraseMP3Data() { - Activator::deactivate(); - Activator::activate(); + $activator = new Activator($this->access_control); + $activator->deactivate(); + $activator->activate(); $this->deleteSegments(); $this->resetMigrationCounters(); diff --git a/lib/Config/Menu.php b/lib/Config/Menu.php index ded750dc66..4de7dbb867 100644 --- a/lib/Config/Menu.php +++ b/lib/Config/Menu.php @@ -26,9 +26,18 @@ use MailPoet\WP\Readme; if(!defined('ABSPATH')) exit; class Menu { - function __construct($renderer, $assets_url) { + const MAIN_PAGE_SLUG = 'mailpoet-newsletters'; + + public $renderer; + public $assets_url; + private $access_control; + private $subscribers_over_limit; + + function __construct($renderer, $assets_url, AccessControl $access_control) { $this->renderer = $renderer; $this->assets_url = $assets_url; + $this->access_control = $access_control; + $this->user_capability = $this->access_control->user_capabilities[0]; $subscribers_feature = new SubscribersFeature(); $this->subscribers_over_limit = $subscribers_feature->check(); $this->checkMailPoetAPIKey(); @@ -46,135 +55,204 @@ class Menu { } function setup() { - if(!AccessControl::validatePermission('access_plugin')) return; + if(!$this->access_control->validatePermission(AccessControl::PERMISSION_ACCESS_PLUGIN)) return; if(self::isOnMailPoetAdminPage()) { do_action('mailpoet_conflict_resolver_styles'); do_action('mailpoet_conflict_resolver_scripts'); } - $main_page_slug = 'mailpoet-newsletters'; - + // Main page add_menu_page( 'MailPoet', 'MailPoet', - AccessControl::validatePermission(AccessControl::PERMISSION_ACCESS_PLUGIN), - $main_page_slug, + $this->user_capability, + self::MAIN_PAGE_SLUG, null, $this->assets_url . '/img/menu_icon.png', 30 ); - $newsletters_page = add_submenu_page( - $main_page_slug, - $this->setPageTitle(__('Emails', 'mailpoet')), - __('Emails', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_EMAILS), - $main_page_slug, - array( - $this, - 'newsletters' - ) - ); + // Emails page + if ($this->access_control->validatePermission(AccessControl::PERMISSION_MANAGE_EMAILS)) { + $newsletters_page = add_submenu_page( + self::MAIN_PAGE_SLUG, + $this->setPageTitle(__('Emails', 'mailpoet')), + __('Emails', 'mailpoet'), + $this->user_capability, + self::MAIN_PAGE_SLUG, + array( + $this, + 'newsletters' + ) + ); - // add limit per page to screen options - add_action('load-' . $newsletters_page, function() { - add_screen_option('per_page', array( - 'label' => _x( - 'Number of newsletters per page', - 'newsletters per page (screen options)', - 'mailpoet' - ), - 'option' => 'mailpoet_newsletters_per_page' - )); - }); + // add limit per page to screen options + add_action('load-' . $newsletters_page, function() { + add_screen_option('per_page', array( + 'label' => _x( + 'Number of newsletters per page', + 'newsletters per page (screen options)', + 'mailpoet' + ), + 'option' => 'mailpoet_newsletters_per_page' + )); + }); - $forms_page = add_submenu_page( - $main_page_slug, - $this->setPageTitle(__('Forms', 'mailpoet')), - __('Forms', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_FORMS), - 'mailpoet-forms', - array( - $this, - 'forms' - ) - ); - // add limit per page to screen options - add_action('load-' . $forms_page, function() { - add_screen_option('per_page', array( - 'label' => _x( - 'Number of forms per page', - 'forms per page (screen options)', - 'mailpoet' - ), - 'option' => 'mailpoet_forms_per_page' - )); - }); + // newsletter editor + add_submenu_page( + true, + $this->setPageTitle(__('Newsletter', 'mailpoet')), + __('Newsletter Editor', 'mailpoet'), + $this->user_capability, + 'mailpoet-newsletter-editor', + array( + $this, + 'newletterEditor' + ) + ); + } - $subscribers_page = add_submenu_page( - $main_page_slug, - $this->setPageTitle(__('Subscribers', 'mailpoet')), - __('Subscribers', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_SUBSCRIBERS), - 'mailpoet-subscribers', - array( - $this, - 'subscribers' - ) - ); - // add limit per page to screen options - add_action('load-' . $subscribers_page, function() { - add_screen_option('per_page', array( - 'label' => _x( - 'Number of subscribers per page', - 'subscribers per page (screen options)', - 'mailpoet' - ), - 'option' => 'mailpoet_subscribers_per_page' - )); - }); + // Forms page + if($this->access_control->validatePermission(AccessControl::PERMISSION_MANAGE_FORMS)) { + $forms_page = add_submenu_page( + self::MAIN_PAGE_SLUG, + $this->setPageTitle(__('Forms', 'mailpoet')), + __('Forms', 'mailpoet'), + $this->user_capability, + 'mailpoet-forms', + array( + $this, + 'forms' + ) + ); - $segments_page = add_submenu_page( - $main_page_slug, - $this->setPageTitle(__('Lists', 'mailpoet')), - __('Lists', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_SEGMENTS), - 'mailpoet-segments', - array( - $this, - 'segments' - ) - ); + // add limit per page to screen options + add_action('load-' . $forms_page, function() { + add_screen_option('per_page', array( + 'label' => _x( + 'Number of forms per page', + 'forms per page (screen options)', + 'mailpoet' + ), + 'option' => 'mailpoet_forms_per_page' + )); + }); - // add limit per page to screen options - add_action('load-' . $segments_page, function() { - add_screen_option('per_page', array( - 'label' => _x( - 'Number of segments per page', - 'segments per page (screen options)', - 'mailpoet' - ), - 'option' => 'mailpoet_segments_per_page' - )); - }); + // form editor + add_submenu_page( + true, + $this->setPageTitle(__('Form Editor', 'mailpoet')), + __('Form Editor', 'mailpoet'), + $this->user_capability, + 'mailpoet-form-editor', + array( + $this, + 'formEditor' + ) + ); + } + // Subscribers page + if($this->access_control->validatePermission(AccessControl::PERMISSION_MANAGE_SUBSCRIBERS)) { + $subscribers_page = add_submenu_page( + self::MAIN_PAGE_SLUG, + $this->setPageTitle(__('Subscribers', 'mailpoet')), + __('Subscribers', 'mailpoet'), + $this->user_capability, + 'mailpoet-subscribers', + array( + $this, + 'subscribers' + ) + ); + + // add limit per page to screen options + add_action('load-' . $subscribers_page, function() { + add_screen_option('per_page', array( + 'label' => _x( + 'Number of subscribers per page', + 'subscribers per page (screen options)', + 'mailpoet' + ), + 'option' => 'mailpoet_subscribers_per_page' + )); + }); + + // import + add_submenu_page( + 'admin.php?page=mailpoet-subscribers', + $this->setPageTitle(__('Import', 'mailpoet')), + __('Import', 'mailpoet'), + $this->user_capability, + 'mailpoet-import', + array( + $this, + 'import' + ) + ); + + // export + add_submenu_page( + true, + $this->setPageTitle(__('Export', 'mailpoet')), + __('Export', 'mailpoet'), + $this->user_capability, + 'mailpoet-export', + array( + $this, + 'export' + ) + ); + } + + // Segments page + if($this->access_control->validatePermission(AccessControl::PERMISSION_MANAGE_SEGMENTS)) { + $segments_page = add_submenu_page( + self::MAIN_PAGE_SLUG, + $this->setPageTitle(__('Lists', 'mailpoet')), + __('Lists', 'mailpoet'), + $this->user_capability, + 'mailpoet-segments', + array( + $this, + 'segments' + ) + ); + + // add limit per page to screen options + add_action('load-' . $segments_page, function() { + add_screen_option('per_page', array( + 'label' => _x( + 'Number of segments per page', + 'segments per page (screen options)', + 'mailpoet' + ), + 'option' => 'mailpoet_segments_per_page' + )); + }); + } + + // Settings page + if($this->access_control->validatePermission(AccessControl::PERMISSION_MANAGE_SETTINGS)) { + add_submenu_page( + self::MAIN_PAGE_SLUG, + $this->setPageTitle(__('Settings', 'mailpoet')), + __('Settings', 'mailpoet'), + $this->user_capability, + 'mailpoet-settings', + array( + $this, + 'settings' + ) + ); + } + + // Help page add_submenu_page( - $main_page_slug, - $this->setPageTitle(__('Settings', 'mailpoet')), - __('Settings', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_SETTINGS), - 'mailpoet-settings', - array( - $this, - 'settings' - ) - ); - - add_submenu_page( - $main_page_slug, + self::MAIN_PAGE_SLUG, $this->setPageTitle(__('Help', 'mailpoet')), __('Help', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_ACCESS_PLUGIN), + $this->user_capability, 'mailpoet-help', array( $this, @@ -182,12 +260,13 @@ class Menu { ) ); + // Premium page // Only show this page in menu if the Premium plugin is not activated add_submenu_page( - License::getLicense() ? true : $main_page_slug, + License::getLicense() ? true : self::MAIN_PAGE_SLUG, $this->setPageTitle(__('Premium', 'mailpoet')), __('Premium', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_ACCESS_PLUGIN), + $this->user_capability, 'mailpoet-premium', array( $this, @@ -195,35 +274,12 @@ class Menu { ) ); - add_submenu_page( - 'admin.php?page=mailpoet-subscribers', - $this->setPageTitle(__('Import', 'mailpoet')), - __('Import', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_SUBSCRIBERS), - 'mailpoet-import', - array( - $this, - 'import' - ) - ); - - add_submenu_page( - true, - $this->setPageTitle(__('Export', 'mailpoet')), - __('Export', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_SUBSCRIBERS), - 'mailpoet-export', - array( - $this, - 'export' - ) - ); - + // Welcome page add_submenu_page( true, $this->setPageTitle(__('Welcome', 'mailpoet')), __('Welcome', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_ACCESS_PLUGIN), + $this->user_capability, 'mailpoet-welcome', array( $this, @@ -231,11 +287,12 @@ class Menu { ) ); + // Update page add_submenu_page( true, $this->setPageTitle(__('Update', 'mailpoet')), __('Update', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_ACCESS_PLUGIN), + $this->user_capability, 'mailpoet-update', array( $this, @@ -243,41 +300,18 @@ class Menu { ) ); + // Migration page add_submenu_page( true, $this->setPageTitle(__('Migration', 'mailpoet')), '', - AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_SETTINGS), + $this->user_capability, 'mailpoet-migration', array( $this, 'migration' ) ); - - add_submenu_page( - true, - $this->setPageTitle(__('Form Editor', 'mailpoet')), - __('Form Editor', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_FORMS), - 'mailpoet-form-editor', - array( - $this, - 'formEditor' - ) - ); - - add_submenu_page( - true, - $this->setPageTitle(__('Newsletter', 'mailpoet')), - __('Newsletter Editor', 'mailpoet'), - AccessControl::validatePermission(AccessControl::PERMISSION_MANAGE_EMAILS), - 'mailpoet-newsletter-editor', - array( - $this, - 'newletterEditor' - ) - ); } function welcome() { @@ -295,20 +329,20 @@ class Menu { or strpos($redirect_url, 'mailpoet') === false ) { - $redirect_url = admin_url('admin.php?page=mailpoet-newsletters'); + $redirect_url = admin_url('admin.php?page=' . self::MAIN_PAGE_SLUG); } $data = array( 'settings' => Setting::getAll(), 'current_user' => wp_get_current_user(), 'redirect_url' => $redirect_url, - 'sub_menu' => 'mailpoet-newsletters' + 'sub_menu' => self::MAIN_PAGE_SLUG ); $this->displayPage('welcome.html', $data); } function migration() { - $mp2_migrator = new MP2Migrator(); + $mp2_migrator = new MP2Migrator($this->access_control); $mp2_migrator->init(); $data = array( 'log_file_url' => $mp2_migrator->log_file_url, @@ -330,14 +364,14 @@ class Menu { or strpos($redirect_url, 'mailpoet') === false ) { - $redirect_url = admin_url('admin.php?page=mailpoet-newsletters'); + $redirect_url = admin_url('admin.php?page=' . self::MAIN_PAGE_SLUG); } $data = array( 'settings' => Setting::getAll(), 'current_user' => wp_get_current_user(), 'redirect_url' => $redirect_url, - 'sub_menu' => 'mailpoet-newsletters' + 'sub_menu' => self::MAIN_PAGE_SLUG ); $readme_file = Env::$path . '/readme.txt'; @@ -354,7 +388,7 @@ class Menu { function premium() { $data = array( 'subscriber_count' => Subscriber::getTotalSubscribers(), - 'sub_menu' => 'mailpoet-newsletters' + 'sub_menu' => self::MAIN_PAGE_SLUG ); $this->displayPage('premium.html', $data); @@ -507,7 +541,7 @@ class Menu { 'shortcodes' => ShortcodesHelper::getShortcodes(), 'settings' => Setting::getAll(), 'current_wp_user' => Subscriber::getCurrentWPUser(), - 'sub_menu' => 'mailpoet-newsletters' + 'sub_menu' => self::MAIN_PAGE_SLUG ); wp_enqueue_media(); wp_enqueue_script('tinymce-wplink', includes_url('js/tinymce/plugins/wplink/plugin.js')); @@ -599,13 +633,13 @@ class Menu { * This error page is used when the initialization is failed * to display admin notices only */ - static function addErrorPage() { + static function addErrorPage(AccessControl $access_control) { if(!self::isOnMailPoetAdminPage()) { return false; } // Check if page already exists if(get_plugin_page_hook($_REQUEST['page'], '') - || get_plugin_page_hook($_REQUEST['page'], 'mailpoet-newsletters') + || get_plugin_page_hook($_REQUEST['page'], self::MAIN_PAGE_SLUG) ) { return false; } @@ -613,7 +647,7 @@ class Menu { true, 'MailPoet', 'MailPoet', - Env::$use_plugin_permission, + $access_control->user_capabilities[0], $_REQUEST['page'], array( __CLASS__, @@ -629,7 +663,7 @@ class Menu { function checkMailPoetAPIKey(ServicesChecker $checker = null) { if(self::isOnMailPoetAdminPage()) { $show_notices = isset($_REQUEST['page']) - && stripos($_REQUEST['page'], 'mailpoet-newsletters') === false; + && stripos($_REQUEST['page'], self::MAIN_PAGE_SLUG) === false; $checker = $checker ?: new ServicesChecker(); $this->mp_api_key_valid = $checker->isMailPoetAPIKeyValid($show_notices); } @@ -638,7 +672,7 @@ class Menu { function checkPremiumKey(ServicesChecker $checker = null) { if(self::isOnMailPoetAdminPage()) { $show_notices = isset($_REQUEST['page']) - && stripos($_REQUEST['page'], 'mailpoet-newsletters') === false; + && stripos($_REQUEST['page'], self::MAIN_PAGE_SLUG) === false; $checker = $checker ?: new ServicesChecker(); $this->premium_key_valid = $checker->isPremiumKeyValid($show_notices); } diff --git a/lib/Router/Endpoints/ViewInBrowser.php b/lib/Router/Endpoints/ViewInBrowser.php index 1229d1a68a..87d257c5e9 100644 --- a/lib/Router/Endpoints/ViewInBrowser.php +++ b/lib/Router/Endpoints/ViewInBrowser.php @@ -1,6 +1,7 @@ data = $this->_processBrowserPreviewData($data); + $this->access_control = new AccessControl(); } function view() { @@ -69,8 +71,8 @@ class ViewInBrowser { $data->queue = false; } - // allow users with 'manage_options' permission to preview any newsletter - if(!empty($data->preview) && current_user_can(Env::$required_permission) + // allow users with permission to manage emails to preview any newsletter + if(!empty($data->preview) && $this->access_control->validatePermission(AccessControl::PERMISSION_MANAGE_EMAILS) ) return $data; // allow others to preview newsletters only when newsletter hash is defined From 5e7f9e3edff95f5dbea14e935991bbecb07ff6f4 Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 17 Aug 2017 09:56:24 -0400 Subject: [PATCH 07/24] Passes AccessControl to JSON API via constructor parameter Removes passing AccessControl to individual API endpoints --- lib/API/API.php | 5 ++++- lib/API/JSON/API.php | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/API/API.php b/lib/API/API.php index 3c13d8e2f0..0212981bf3 100644 --- a/lib/API/API.php +++ b/lib/API/API.php @@ -1,11 +1,14 @@ access_control = new AccessControl(); + function __construct(AccessControl $access_control) { + $this->access_control = $access_control; foreach($this->_available_api_versions as $available_api_version) { $this->addEndpointNamespace( sprintf('%s\%s', __NAMESPACE__, $available_api_version), @@ -128,7 +128,7 @@ class API { throw new \Exception(__('Invalid API endpoint.', 'mailpoet')); } - $endpoint = new $this->_request_endpoint_class($this->access_control); + $endpoint = new $this->_request_endpoint_class(); // check the accessibility of the requested endpoint's action // by default, an endpoint's action is considered "private" From 788494ec47f62084312c685252f747209df9bad9 Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 17 Aug 2017 09:57:30 -0400 Subject: [PATCH 08/24] Updates API initialization --- lib/Subscription/Form.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Subscription/Form.php b/lib/Subscription/Form.php index ec3262be0c..3a768df770 100644 --- a/lib/Subscription/Form.php +++ b/lib/Subscription/Form.php @@ -2,14 +2,14 @@ namespace MailPoet\Subscription; -use MailPoet\API\JSON\API; +use MailPoet\API\API as API; use MailPoet\API\JSON\Response as APIResponse; use MailPoet\Util\Url as UrlHelper; class Form { static function onSubmit($request_data = false) { $request_data = ($request_data) ? $request_data : $_REQUEST; - $api = new API(); + $api = API::JSON(); $api->setRequestData($request_data); $form_id = (!empty($request_data['data']['form_id'])) ? (int)$request_data['data']['form_id'] : false; $response = $api->processRoute(); From 5ba2c4bc3afdd30606b3019f7ec0e59fa7e9def5 Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 17 Aug 2017 09:58:05 -0400 Subject: [PATCH 09/24] Removes AccessControl from individual API endpoints --- lib/API/JSON/v1/MP2Migrator.php | 5 ++--- lib/API/JSON/v1/Setup.php | 7 +------ 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/API/JSON/v1/MP2Migrator.php b/lib/API/JSON/v1/MP2Migrator.php index dcc3c42b4c..aa3bd41d75 100644 --- a/lib/API/JSON/v1/MP2Migrator.php +++ b/lib/API/JSON/v1/MP2Migrator.php @@ -12,9 +12,8 @@ class MP2Migrator extends APIEndpoint { 'global' => AccessControl::PERMISSION_MANAGE_SETTINGS ); - public function __construct(AccessControl $access_control) { - $this->access_control = $access_control; - $this->MP2Migrator = new \MailPoet\Config\MP2Migrator($this->access_control); + public function __construct() { + $this->MP2Migrator = new \MailPoet\Config\MP2Migrator(); } /** diff --git a/lib/API/JSON/v1/Setup.php b/lib/API/JSON/v1/Setup.php index 8b333b4e8b..1bf0647748 100644 --- a/lib/API/JSON/v1/Setup.php +++ b/lib/API/JSON/v1/Setup.php @@ -13,15 +13,10 @@ class Setup extends APIEndpoint { public $permissions = array( 'global' => AccessControl::PERMISSION_MANAGE_SETTINGS ); - private $access_control; - - function __construct(AccessControl $access_control) { - $this->access_control = $access_control; - } function reset() { try { - $activator = new Activator($this->access_control); + $activator = new Activator(); $activator->deactivate(); $activator->activate(); Hooks::doAction('mailpoet_setup_reset'); From 8d8dfaa11fd211dcb1f1554ec9f6fc333550e006 Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 17 Aug 2017 10:00:36 -0400 Subject: [PATCH 10/24] Uses Intializer to check permissions before running Activator --- lib/Config/Activator.php | 9 --------- lib/Config/Initializer.php | 3 +++ 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/Config/Activator.php b/lib/Config/Activator.php index c849e4e264..5b7e757061 100644 --- a/lib/Config/Activator.php +++ b/lib/Config/Activator.php @@ -5,15 +5,6 @@ namespace MailPoet\Config; if(!defined('ABSPATH')) exit; class Activator { - private $access_control; - - function __construct(AccessControl $access_control) { - $this->access_control = $access_control; - if(!$this->access_control->validatePermission(AccessControl::PERMISSION_UPDATE_PLUGIN)) { - throw new \Exception(__('You do not have permission to activate/deactivate MailPoet plugin.', 'mailpoet')); - } - } - function activate() { $migrator = new Migrator(); $migrator->up(); diff --git a/lib/Config/Initializer.php b/lib/Config/Initializer.php index 4e527c548d..cc0d7541e8 100644 --- a/lib/Config/Initializer.php +++ b/lib/Config/Initializer.php @@ -136,6 +136,9 @@ class Initializer { // if current db version and plugin version differ if(version_compare($current_db_version, Env::$version) !== 0) { + if(!$this->access_control->validatePermission(AccessControl::PERMISSION_UPDATE_PLUGIN)) { + throw new \Exception(__('You do not have permission to activate/deactivate MailPoet plugin.', 'mailpoet')); + } $activator = new Activator($this->access_control); $activator->activate(); } From efa231b08f77c00e217b172b288a4bea8096e150 Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 17 Aug 2017 10:17:19 -0400 Subject: [PATCH 11/24] Removes AccessControl from Migrator and Changelog --- lib/Config/Changelog.php | 10 +++------- lib/Config/Initializer.php | 2 +- lib/Config/MP2Migrator.php | 6 ++---- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/lib/Config/Changelog.php b/lib/Config/Changelog.php index 29b33f2e55..1bf9fbfb4f 100644 --- a/lib/Config/Changelog.php +++ b/lib/Config/Changelog.php @@ -1,15 +1,11 @@ access_control = $access_control; - } - function init() { $doing_ajax = (bool)(defined('DOING_AJAX') && DOING_AJAX); @@ -37,7 +33,7 @@ class Changelog { $version = Setting::getValue('version', null); $redirect_url = null; - $mp2_migrator = new MP2Migrator($this->access_control); + $mp2_migrator = new MP2Migrator(); if(!in_array($_GET['page'], array('mailpoet-migration', 'mailpoet-settings')) && $mp2_migrator->isMigrationStartedAndNotCompleted()) { // Force the redirection if the migration has started but is not completed $redirect_url = admin_url('admin.php?page=mailpoet-migration'); diff --git a/lib/Config/Initializer.php b/lib/Config/Initializer.php index cc0d7541e8..1c42cdb7e5 100644 --- a/lib/Config/Initializer.php +++ b/lib/Config/Initializer.php @@ -195,7 +195,7 @@ class Initializer { } function setupChangelog() { - $changelog = new Changelog($this->access_control); + $changelog = new Changelog(); $changelog->init(); } diff --git a/lib/Config/MP2Migrator.php b/lib/Config/MP2Migrator.php index e75fc6b8c5..9ef7fed8c6 100644 --- a/lib/Config/MP2Migrator.php +++ b/lib/Config/MP2Migrator.php @@ -19,19 +19,17 @@ class MP2Migrator { const CHUNK_SIZE = 10; // To import the data by batch private $log_file; - private $access_control; public $log_file_url; public $progressbar; private $segments_mapping = array(); // Mapping between old and new segment IDs private $wp_users_segment; - public function __construct(AccessControl $access_control) { + public function __construct() { $this->defineMP2Tables(); $log_filename = 'mp2migration.log'; $this->log_file = Env::$temp_path . '/' . $log_filename; $this->log_file_url = Env::$temp_url . '/' . $log_filename; $this->progressbar = new ProgressBar('mp2migration'); - $this->access_control = $access_control; } private function defineMP2Tables() { @@ -189,7 +187,7 @@ class MP2Migrator { * */ private function eraseMP3Data() { - $activator = new Activator($this->access_control); + $activator = new Activator(); $activator->deactivate(); $activator->activate(); From 4b7fb3ae3d9d66a1aa7b5b4280d3985e438cb639 Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 17 Aug 2017 10:22:13 -0400 Subject: [PATCH 12/24] Updates access permission names to improve clarity --- lib/API/JSON/API.php | 2 +- lib/API/JSON/v1/Subscribers.php | 2 +- lib/Config/AccessControl.php | 8 ++++---- lib/Config/Menu.php | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/API/JSON/API.php b/lib/API/JSON/API.php index 0a77672a7f..4e9976b324 100644 --- a/lib/API/JSON/API.php +++ b/lib/API/JSON/API.php @@ -149,7 +149,7 @@ class API { function validatePermissions($request_method, $permissions) { // if method permission is defined, validate it if (!empty($permissions['methods'][$request_method])) { - return ($permissions['methods'][$request_method] === AccessControl::ACCESS_ALL) ? + return ($permissions['methods'][$request_method] === AccessControl::NO_ACCESS_RESTRICTION) ? true : $this->access_control->validatePermission($permissions['methods'][$request_method]); } diff --git a/lib/API/JSON/v1/Subscribers.php b/lib/API/JSON/v1/Subscribers.php index 0631806343..323d8f9d81 100644 --- a/lib/API/JSON/v1/Subscribers.php +++ b/lib/API/JSON/v1/Subscribers.php @@ -16,7 +16,7 @@ if(!defined('ABSPATH')) exit; class Subscribers extends APIEndpoint { public $permissions = array( 'global' => AccessControl::PERMISSION_MANAGE_SUBSCRIBERS, - 'methods' => array('subscribe' => AccessControl::ACCESS_ALL) + 'methods' => array('subscribe' => AccessControl::NO_ACCESS_RESTRICTION) ); function get($data = array()) { diff --git a/lib/Config/AccessControl.php b/lib/Config/AccessControl.php index c56e43778c..9604f6058a 100644 --- a/lib/Config/AccessControl.php +++ b/lib/Config/AccessControl.php @@ -8,14 +8,14 @@ if(!defined('ABSPATH')) exit; require_once(ABSPATH . 'wp-includes/pluggable.php'); class AccessControl { - const PERMISSION_ACCESS_PLUGIN = 'access_plugin'; + const PERMISSION_ACCESS_PLUGIN_ADMIN = 'access_plugin_admin'; const PERMISSION_MANAGE_SETTINGS = 'manage_settings'; const PERMISSION_MANAGE_EMAILS = 'manage_emails'; const PERMISSION_MANAGE_SUBSCRIBERS = 'manage_subscribers'; const PERMISSION_MANAGE_FORMS = 'manage_forms'; const PERMISSION_MANAGE_SEGMENTS = 'manage_segments'; const PERMISSION_UPDATE_PLUGIN = 'update_plugin'; - const ACCESS_ALL = 'All'; + const NO_ACCESS_RESTRICTION = 'no_access_restriction'; public $permissions; public $current_user_roles; @@ -29,8 +29,8 @@ class AccessControl { private function getDefaultPermissions() { return array( - self::PERMISSION_ACCESS_PLUGIN => WPHooks::applyFilters( - 'mailpoet_permission_access_plugin', + self::PERMISSION_ACCESS_PLUGIN_ADMIN => WPHooks::applyFilters( + 'mailpoet_permission_access_plugin_admin', array( 'administrator', 'editor' diff --git a/lib/Config/Menu.php b/lib/Config/Menu.php index 4de7dbb867..2960b1522d 100644 --- a/lib/Config/Menu.php +++ b/lib/Config/Menu.php @@ -55,7 +55,7 @@ class Menu { } function setup() { - if(!$this->access_control->validatePermission(AccessControl::PERMISSION_ACCESS_PLUGIN)) return; + if(!$this->access_control->validatePermission(AccessControl::PERMISSION_ACCESS_PLUGIN_ADMIN)) return; if(self::isOnMailPoetAdminPage()) { do_action('mailpoet_conflict_resolver_styles'); do_action('mailpoet_conflict_resolver_scripts'); From 5553817f9a12904bfb5431d87b1141a9a26db220 Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 17 Aug 2017 10:53:50 -0400 Subject: [PATCH 13/24] Creates method to get user's first capability --- lib/Config/AccessControl.php | 6 ++++++ lib/Config/Menu.php | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/Config/AccessControl.php b/lib/Config/AccessControl.php index 9604f6058a..2bbab62705 100644 --- a/lib/Config/AccessControl.php +++ b/lib/Config/AccessControl.php @@ -86,6 +86,12 @@ class AccessControl { return array_keys($user->allcaps); } + function getUserFirstCapability() { + return (!empty($this->user_capabilities)) ? + $this->user_capabilities[0] : + null; + } + function validatePermission($permission) { if(empty($this->permissions[$permission])) return false; $permitted_roles = array_intersect( diff --git a/lib/Config/Menu.php b/lib/Config/Menu.php index 2960b1522d..16c9ff43f5 100644 --- a/lib/Config/Menu.php +++ b/lib/Config/Menu.php @@ -37,7 +37,7 @@ class Menu { $this->renderer = $renderer; $this->assets_url = $assets_url; $this->access_control = $access_control; - $this->user_capability = $this->access_control->user_capabilities[0]; + $this->user_capability = $this->access_control->getUserFirstCapability(); $subscribers_feature = new SubscribersFeature(); $this->subscribers_over_limit = $subscribers_feature->check(); $this->checkMailPoetAPIKey(); From 1b756ef0b20a1abfef0d346b216ca0cc431b608d Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 17 Aug 2017 18:25:26 -0400 Subject: [PATCH 14/24] Adds access management to router and updates endpoints accordingly --- lib/Router/Endpoints/CronDaemon.php | 5 +++++ lib/Router/Endpoints/Subscription.php | 5 +++++ lib/Router/Endpoints/Track.php | 9 +++++++-- lib/Router/Endpoints/ViewInBrowser.php | 9 ++++++--- lib/Router/Router.php | 27 ++++++++++++++++++++++---- 5 files changed, 46 insertions(+), 9 deletions(-) diff --git a/lib/Router/Endpoints/CronDaemon.php b/lib/Router/Endpoints/CronDaemon.php index 89f4d07096..626e82c010 100644 --- a/lib/Router/Endpoints/CronDaemon.php +++ b/lib/Router/Endpoints/CronDaemon.php @@ -1,6 +1,8 @@ AccessControl::NO_ACCESS_RESTRICTION + ); function __construct($data) { $this->data = $data; diff --git a/lib/Router/Endpoints/Subscription.php b/lib/Router/Endpoints/Subscription.php index 04dfbf11fc..19e428d064 100644 --- a/lib/Router/Endpoints/Subscription.php +++ b/lib/Router/Endpoints/Subscription.php @@ -1,6 +1,8 @@ AccessControl::NO_ACCESS_RESTRICTION + ); function __construct($data) { $this->data = $data; diff --git a/lib/Router/Endpoints/Track.php b/lib/Router/Endpoints/Track.php index 5ab6a1266e..fe998c97f8 100644 --- a/lib/Router/Endpoints/Track.php +++ b/lib/Router/Endpoints/Track.php @@ -1,6 +1,8 @@ AccessControl::NO_ACCESS_RESTRICTION + ); function __construct($data) { $this->data = $this->_processTrackData($data); @@ -38,8 +43,8 @@ class Track { function _processTrackData($data) { $data = (object)Links::transformUrlDataObject($data); if(empty($data->queue_id) || - empty($data->subscriber_id) || - empty($data->subscriber_token) + empty($data->subscriber_id) || + empty($data->subscriber_token) ) { return false; } diff --git a/lib/Router/Endpoints/ViewInBrowser.php b/lib/Router/Endpoints/ViewInBrowser.php index 87d257c5e9..7e8a961d63 100644 --- a/lib/Router/Endpoints/ViewInBrowser.php +++ b/lib/Router/Endpoints/ViewInBrowser.php @@ -1,8 +1,8 @@ AccessControl::NO_ACCESS_RESTRICTION + ); - function __construct($data) { + function __construct($data, AccessControl $access_control) { + $this->access_control = $access_control; $this->data = $this->_processBrowserPreviewData($data); - $this->access_control = new AccessControl(); } function view() { diff --git a/lib/Router/Router.php b/lib/Router/Router.php index 93d3f9735b..c07d4557ae 100644 --- a/lib/Router/Router.php +++ b/lib/Router/Router.php @@ -1,6 +1,8 @@ endpoint = isset($api_data['endpoint']) ? Helpers::underscoreToCamelCase($api_data['endpoint']) : false; - $this->action = isset($api_data['action']) ? + $this->endpoint_action = isset($api_data['action']) ? Helpers::underscoreToCamelCase($api_data['action']) : false; $this->data = isset($api_data['data']) ? self::decodeRequestData($api_data['data']) : false; + $this->access_control = new AccessControl(); } function init() { @@ -33,15 +36,18 @@ class Router { if(!$this->endpoint || !class_exists($endpoint_class)) { return $this->terminateRequest(self::RESPONSE_ERROR, __('Invalid router endpoint', 'mailpoet')); } - $endpoint = new $endpoint_class($this->data); - if(!method_exists($endpoint, $this->action) || !in_array($this->action, $endpoint->allowed_actions)) { + $endpoint = new $endpoint_class($this->data, $this->access_control); + if(!method_exists($endpoint, $this->endpoint_action) || !in_array($this->endpoint_action, $endpoint->allowed_actions)) { return $this->terminateRequest(self::RESPONSE_ERROR, __('Invalid router endpoint action', 'mailpoet')); } + if(!$this->validatePermissions($this->endpoint_action, $endpoint->permissions)) { + return $this->terminateRequest(self::RESPONSE_ERROR, __('You do not have the required permissions.', 'mailpoet')); + } do_action('mailpoet_conflict_resolver_router_url_query_parameters'); return call_user_func( array( $endpoint, - $this->action + $this->endpoint_action ) ); } @@ -74,4 +80,17 @@ class Router { status_header($code, $message); exit; } + + function validatePermissions($endpoint_action, $permissions) { + // if method permission is defined, validate it + if(!empty($permissions['methods'][$endpoint_action])) { + return ($permissions['methods'][$endpoint_action] === AccessControl::NO_ACCESS_RESTRICTION) ? + true : + $this->access_control->validatePermission($permissions['methods'][$endpoint_action]); + } + // use global permission + return ($permissions['global'] === AccessControl::NO_ACCESS_RESTRICTION) ? + true : + $this->access_control->validatePermission($permissions['global']); + } } From 78429d8f9118d4dc68e550cc9f6b377f15dac31d Mon Sep 17 00:00:00 2001 From: Vlad Date: Sun, 20 Aug 2017 12:27:24 -0400 Subject: [PATCH 15/24] Validates global permission at the AccessControl level Changes error response code on invalid permission --- lib/Config/AccessControl.php | 1 + lib/Router/Router.php | 17 ++++++----------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/lib/Config/AccessControl.php b/lib/Config/AccessControl.php index 2bbab62705..97a142140d 100644 --- a/lib/Config/AccessControl.php +++ b/lib/Config/AccessControl.php @@ -93,6 +93,7 @@ class AccessControl { } function validatePermission($permission) { + if($permission === self::NO_ACCESS_RESTRICTION) return true; if(empty($this->permissions[$permission])) return false; $permitted_roles = array_intersect( $this->user_roles, diff --git a/lib/Router/Router.php b/lib/Router/Router.php index c07d4557ae..fb9ee2a207 100644 --- a/lib/Router/Router.php +++ b/lib/Router/Router.php @@ -14,6 +14,7 @@ class Router { public $data; const NAME = 'mailpoet_router'; const RESPONSE_ERROR = 404; + const RESPONE_FORBIDDEN = 403; function __construct($api_data = false) { $api_data = ($api_data) ? $api_data : $_GET; @@ -41,7 +42,7 @@ class Router { return $this->terminateRequest(self::RESPONSE_ERROR, __('Invalid router endpoint action', 'mailpoet')); } if(!$this->validatePermissions($this->endpoint_action, $endpoint->permissions)) { - return $this->terminateRequest(self::RESPONSE_ERROR, __('You do not have the required permissions.', 'mailpoet')); + return $this->terminateRequest(self::RESPONE_FORBIDDEN, __('You do not have the required permissions.', 'mailpoet')); } do_action('mailpoet_conflict_resolver_router_url_query_parameters'); return call_user_func( @@ -82,15 +83,9 @@ class Router { } function validatePermissions($endpoint_action, $permissions) { - // if method permission is defined, validate it - if(!empty($permissions['methods'][$endpoint_action])) { - return ($permissions['methods'][$endpoint_action] === AccessControl::NO_ACCESS_RESTRICTION) ? - true : - $this->access_control->validatePermission($permissions['methods'][$endpoint_action]); - } - // use global permission - return ($permissions['global'] === AccessControl::NO_ACCESS_RESTRICTION) ? - true : + // validate action permission if defined, otherwise validate global permission + return(!empty($permissions['actions'][$endpoint_action])) ? + $this->access_control->validatePermission($permissions['actions'][$endpoint_action]) : $this->access_control->validatePermission($permissions['global']); } -} +} \ No newline at end of file From e47c8bc701fcdfb9f917d8e61ff4dcc3daffdf45 Mon Sep 17 00:00:00 2001 From: Vlad Date: Tue, 22 Aug 2017 21:16:38 -0400 Subject: [PATCH 16/24] Adds access control tests for Router --- .../Router/Endpoints/ViewInBrowserTest.php | 18 +++- tests/unit/Router/RouterTest.php | 90 ++++++++++++++++++- tests/unit/Router/RouterTestMockEndpoint.php | 5 ++ 3 files changed, 105 insertions(+), 8 deletions(-) diff --git a/tests/unit/Router/Endpoints/ViewInBrowserTest.php b/tests/unit/Router/Endpoints/ViewInBrowserTest.php index 5727a9c863..6918c993b9 100644 --- a/tests/unit/Router/Endpoints/ViewInBrowserTest.php +++ b/tests/unit/Router/Endpoints/ViewInBrowserTest.php @@ -1,7 +1,9 @@ false ); // instantiate class - $this->view_in_browser = new ViewInBrowser($this->browser_preview_data); + $this->view_in_browser = new ViewInBrowser($this->browser_preview_data, new AccessControl()); } function testItAbortsWhenBrowserPreviewDataIsMissing() { @@ -123,6 +125,7 @@ class ViewInBrowserTest extends \MailPoetTest { } function testItDoesNotRequireWpAdministratorToBeOnProcessedListWhenPreviewIsEnabled() { + $view_in_browser = $this->view_in_browser; $data = (object)array_merge( $this->browser_preview_data, array( @@ -132,19 +135,25 @@ class ViewInBrowserTest extends \MailPoetTest { ) ); $data->preview = true; + // when WP user is not logged, false should be returned - expect($this->view_in_browser->_validateBrowserPreviewData($data))->false(); + expect($view_in_browser->_validateBrowserPreviewData($data))->false(); + // when WP user is logged in but does not have 'manage options' permission, false should be returned wp_set_current_user(1); $wp_user = wp_get_current_user(); $wp_user->remove_role('administrator'); + $view_in_browser->access_control = new AccessControl(); expect($this->view_in_browser->_validateBrowserPreviewData($data))->false(); + // when WP user is logged and has 'manage options' permission, data should be returned $wp_user->add_role('administrator'); - expect($this->view_in_browser->_validateBrowserPreviewData($data))->equals($data); + $view_in_browser->access_control = new AccessControl(); + expect($view_in_browser->_validateBrowserPreviewData($data))->equals($data); } function testItSetsSubscriberToLoggedInWPUserWhenPreviewIsEnabled() { + $view_in_browser = $this->view_in_browser; $data = (object)array_merge( $this->browser_preview_data, array( @@ -155,7 +164,8 @@ class ViewInBrowserTest extends \MailPoetTest { ); $data->preview = true; wp_set_current_user(1); - $result = $this->view_in_browser->_validateBrowserPreviewData($data); + $view_in_browser->access_control = new AccessControl(); + $result = $view_in_browser->_validateBrowserPreviewData($data); expect($result->subscriber->id)->equals(1); } diff --git a/tests/unit/Router/RouterTest.php b/tests/unit/Router/RouterTest.php index 58d31d7603..2eca6d6649 100644 --- a/tests/unit/Router/RouterTest.php +++ b/tests/unit/Router/RouterTest.php @@ -1,7 +1,9 @@ api_request)->equals(true); expect($router->endpoint)->equals('viewInBrowser'); - expect($router->action)->equals('view'); + expect($router->endpoint_action)->equals('view'); expect($router->data)->equals($data); } @@ -92,6 +94,87 @@ class RouterTest extends \MailPoetTest { ); } + function testItValidatesGlobalPermission() { + $access_control = new AccessControl(); + $router = $this->router; + + $permissions = array( + 'global' => AccessControl::PERMISSION_MANAGE_SETTINGS, + ); + $access_control->user_roles = array(); + $router->access_control = $access_control; + expect($router->validatePermissions(null, $permissions))->false(); + + $access_control->user_roles = $access_control->permissions[AccessControl::PERMISSION_MANAGE_SETTINGS]; + $router->access_control = $access_control; + expect($router->validatePermissions(null, $permissions))->true(); + } + + function testItValidatesEndpointActionPermission() { + $access_control = new AccessControl(); + $router = $this->router; + + $permissions = array( + 'global' => null, + 'actions' => array( + 'test' => AccessControl::PERMISSION_MANAGE_SETTINGS + ) + ); + + $access_control->user_roles = array(); + $router->access_control = $access_control; + expect($router->validatePermissions('test', $permissions))->false(); + + $access_control->user_roles = $access_control->permissions[AccessControl::PERMISSION_MANAGE_SETTINGS]; + $router->access_control = $access_control; + expect($router->validatePermissions('test', $permissions))->true(); + } + + function testItValidatesPermissionBeforeProcessingEndpointAction() { + $router = Stub::construct( + new Router(), + array($this->router_data), + array( + 'validatePermissions' => function($action, $permissions) { + expect($action)->equals($this->router_data['action']); + expect($permissions)->equals( + array( + 'global' => AccessControl::NO_ACCESS_RESTRICTION + ) + ); + return true; + } + ) + ); + $result = $router->init(); + expect($result)->equals( + array('data' => 'dummy data') + ); + } + + function testItReturnsForbiddenResponseWhenPermissionFailsValidation() { + $router = Stub::construct( + new Router(), + array($this->router_data), + array( + 'validatePermissions' => false, + 'terminateRequest' => function($code, $error) { + return array( + $code, + $error + ); + } + ) + ); + $result = $router->init(); + expect($result)->equals( + array( + 403, + 'You do not have the required permissions.' + ) + ); + } + function testItCallsEndpointAction() { $data = array('data' => 'dummy data'); $result = $this->router->init(); @@ -99,8 +182,7 @@ class RouterTest extends \MailPoetTest { } function testItExecutesUrlParameterConflictResolverAction() { - $data = array('data' => 'dummy data'); - $result = $this->router->init(); + $this->router->init(); expect((boolean)did_action('mailpoet_conflict_resolver_router_url_query_parameters'))->true(); } @@ -140,4 +222,4 @@ class RouterTest extends \MailPoetTest { ); expect($result)->contains(Router::NAME . '&endpoint=router_test_mock_endpoint&action=test&data=' . $encoded_data); } -} +} \ No newline at end of file diff --git a/tests/unit/Router/RouterTestMockEndpoint.php b/tests/unit/Router/RouterTestMockEndpoint.php index 24ab75f774..51c020f92a 100644 --- a/tests/unit/Router/RouterTestMockEndpoint.php +++ b/tests/unit/Router/RouterTestMockEndpoint.php @@ -2,12 +2,17 @@ namespace MailPoet\Router\Endpoints; +use MailPoet\Config\AccessControl; + class RouterTestMockEndpoint { const ACTION_TEST = 'test'; public $allowed_actions = array( self::ACTION_TEST ); public $data; + public $permissions = array( + 'global' => AccessControl::NO_ACCESS_RESTRICTION + ); function __construct($data) { $this->data = $data; From 48f3ae4ea1935065e27a225f431f6ce21eeeba92 Mon Sep 17 00:00:00 2001 From: Vlad Date: Tue, 22 Aug 2017 21:19:51 -0400 Subject: [PATCH 17/24] Adds access control unit tests --- tests/unit/Config/AccessControlTest.php | 96 ++++++++++--------------- 1 file changed, 36 insertions(+), 60 deletions(-) diff --git a/tests/unit/Config/AccessControlTest.php b/tests/unit/Config/AccessControlTest.php index cfc62ef23b..75302c4586 100644 --- a/tests/unit/Config/AccessControlTest.php +++ b/tests/unit/Config/AccessControlTest.php @@ -8,74 +8,40 @@ use MailPoet\WP\Hooks; class AccessControlTest extends \MailPoetTest { function testItSetsDefaultPermissionsUponInitialization() { - AccessControl::init(); $default_permissions = array( - 'access_plugin' => array( + AccessControl::PERMISSION_ACCESS_PLUGIN_ADMIN => array( 'administrator', 'editor' ), - 'manage_settings' => array( + AccessControl::PERMISSION_MANAGE_SETTINGS => array( 'administrator' ), - 'manage_emails' => array( + AccessControl::PERMISSION_MANAGE_EMAILS => array( 'administrator', 'editor' ), - 'manage_subscribers' => array( + AccessControl::PERMISSION_MANAGE_SUBSCRIBERS => array( 'administrator' ), - 'manage_forms' => array( + AccessControl::PERMISSION_MANAGE_FORMS => array( 'administrator' ), - 'manage_segments' => array( + AccessControl::PERMISSION_MANAGE_SEGMENTS => array( + 'administrator' + ), + AccessControl::PERMISSION_UPDATE_PLUGIN => array( 'administrator' ) ); - expect(AccessControl::getPermissions())->equals($default_permissions); + $access_control = new AccessControl(); + expect($access_control->permissions)->equals($default_permissions); } - function testItSetsCustomPermissionsUponInitialization() { - $custom_permissions = array( - 'custom_permissions' => array( - 'custom_role' - ) - ); - AccessControl::init($custom_permissions); - expect(AccessControl::$permissions)->equals($custom_permissions); - } - - function testItGetsPermissions() { - expect(AccessControl::getPermissions())->equals( - array( - 'access_plugin' => array( - 'administrator', - 'editor' - ), - 'manage_settings' => array( - 'administrator' - ), - 'manage_emails' => array( - 'administrator', - 'editor' - ), - 'manage_subscribers' => array( - 'administrator' - ), - 'manage_forms' => array( - 'administrator' - ), - 'manage_segments' => array( - 'administrator' - ) - ) - ); - } - - function testItAllowsSettingCustonPermissions() { + function testItAllowsSettingCustomPermissions() { Hooks::addFilter( - 'mailpoet_permission_access_plugin', + 'mailpoet_permission_access_plugin_admin', function() { - return array('custom_access_plugin_role'); + return array('custom_access_plugin_admin_role'); } ); Hooks::addFilter( @@ -105,30 +71,40 @@ class AccessControlTest extends \MailPoetTest { Hooks::addFilter( 'mailpoet_permission_manage_segments', function() { - return array('custom_manage_forms_role'); + return array('custom_manage_segments_role'); } ); - AccessControl::init(); - expect(AccessControl::$permissions)->equals( + Hooks::addFilter( + 'mailpoet_permission_update_plugin', + function() { + return array('custom_update_plugin_role'); + } + ); + + $access_control = new AccessControl(); + expect($access_control->permissions)->equals( array( - 'access_plugin' => array( - 'custom_access_plugin_role' + AccessControl::PERMISSION_ACCESS_PLUGIN_ADMIN => array( + 'custom_access_plugin_admin_role' ), - 'manage_settings' => array( + AccessControl::PERMISSION_MANAGE_SETTINGS => array( 'custom_manage_settings_role' ), - 'manage_emails' => array( + AccessControl::PERMISSION_MANAGE_EMAILS => array( 'custom_manage_emails_role' ), - 'manage_subscribers' => array( + AccessControl::PERMISSION_MANAGE_SUBSCRIBERS => array( 'custom_manage_subscribers_role' ), - 'manage_forms' => array( + AccessControl::PERMISSION_MANAGE_FORMS => array( 'custom_manage_forms_role' ), - 'manage_segments' => array( - 'custom_manage_forms_role' - ) + AccessControl::PERMISSION_MANAGE_SEGMENTS => array( + 'custom_manage_segments_role' + ), + AccessControl::PERMISSION_UPDATE_PLUGIN => array( + 'custom_update_plugin_role' + ), ) ); } From 28320cdbb61f300b572b991cef4ed64f815b2bb6 Mon Sep 17 00:00:00 2001 From: Vlad Date: Wed, 23 Aug 2017 09:53:54 -0400 Subject: [PATCH 18/24] Updates permission validation method on AccessControl Adds/updates unit tests --- lib/API/JSON/API.php | 12 +- tests/unit/API/JSON/APITest.php | 126 +++++++++++++++--- .../JSON/APITestNamespacedEndpointStubV1.php | 12 +- .../JSON/APITestNamespacedEndpointStubV2.php | 7 +- 4 files changed, 123 insertions(+), 34 deletions(-) diff --git a/lib/API/JSON/API.php b/lib/API/JSON/API.php index 4e9976b324..5df022d8f8 100644 --- a/lib/API/JSON/API.php +++ b/lib/API/JSON/API.php @@ -147,14 +147,10 @@ class API { } function validatePermissions($request_method, $permissions) { - // if method permission is defined, validate it - if (!empty($permissions['methods'][$request_method])) { - return ($permissions['methods'][$request_method] === AccessControl::NO_ACCESS_RESTRICTION) ? - true : - $this->access_control->validatePermission($permissions['methods'][$request_method]); - } - // use global permission - return $this->access_control->validatePermission($permissions['global']); + // validate method permission if defined, otherwise validate global permission + return(!empty($permissions['methods'][$request_method])) ? + $this->access_control->validatePermission($permissions['methods'][$request_method]) : + $this->access_control->validatePermission($permissions['global']); } function checkToken() { diff --git a/tests/unit/API/JSON/APITest.php b/tests/unit/API/JSON/APITest.php index 82c5e6f794..ba06c8a7a3 100644 --- a/tests/unit/API/JSON/APITest.php +++ b/tests/unit/API/JSON/APITest.php @@ -1,13 +1,19 @@ wp_user_id = $wp_user_id; } - - $this->api = new API(); - } - - function testItChecksPermissions() { - // logged out user - expect($this->api->checkPermissions())->false(); - - // give administrator role to wp user - $wp_user = get_user_by('id', $this->wp_user_id); - $wp_user->add_role('administrator'); - wp_set_current_user($wp_user->ID, $wp_user->user_login); - - // administrator should have permission - expect($this->api->checkPermissions())->true(); + $this->api = API::JSON(); } function testItCallsAPISetupAction() { $called = false; - add_action( + Hooks::addAction( 'mailpoet_api_setup', - function ($api) use (&$called) { + function($api) use (&$called) { $called = true; - expect($api instanceof API)->true(); + expect($api instanceof JSONAPI)->true(); } ); $api = Stub::makeEmptyExcept( @@ -136,11 +128,101 @@ class APITest extends \MailPoetTest { ); $this->api->setRequestData($data); $response = $this->api->processRoute(); - expect($response->getData()['data'])->equals($data['api_version']); } + function testItValidatesPermissionBeforeProcessingEndpointMethod() { + $namespace = array( + 'name' => 'MailPoet\API\JSON\v1', + 'version' => 'v1' + ); + $data = array( + 'endpoint' => 'a_p_i_test_namespaced_endpoint_stub_v1', + 'method' => 'restricted', + 'api_version' => 'v1', + 'data' => array('test' => 'data') + ); + $access_control = new AccessControl(); + $access_control->user_roles = $access_control->permissions[AccessControl::PERMISSION_MANAGE_SETTINGS]; + $api = Stub::make( + new \MailPoet\API\JSON\API($access_control), + array( + 'validatePermissions' => function($method, $permissions) use ($data) { + expect($method)->equals($data['method']); + expect($permissions)->equals( + array( + 'global' => AccessControl::NO_ACCESS_RESTRICTION, + 'methods' => array( + 'test' => AccessControl::NO_ACCESS_RESTRICTION, + 'restricted' => AccessControl::PERMISSION_MANAGE_SETTINGS + ) + ) + ); + return true; + } + ) + ); + $api->addEndpointNamespace($namespace['name'], $namespace['version']); + $api->setRequestData($data); + $response = $api->processRoute(); + expect($response->getData()['data'])->equals($data['data']); + } + + function testItReturnsForbiddenResponseWhenPermissionFailsValidation() { + $namespace = array( + 'name' => 'MailPoet\API\JSON\v1', + 'version' => 'v1' + ); + $data = array( + 'endpoint' => 'a_p_i_test_namespaced_endpoint_stub_v1', + 'method' => 'restricted', + 'api_version' => 'v1', + 'data' => array('test' => 'data') + ); + $access_control = new AccessControl(); + $access_control->user_roles = array(); + $api = new \MailPoet\API\JSON\API($access_control); + $api->addEndpointNamespace($namespace['name'], $namespace['version']); + $api->setRequestData($data); + $response = $api->processRoute(); + expect($response->status)->equals(Response::STATUS_FORBIDDEN); + } + + function testItValidatesGlobalPermission() { + $access_control = new AccessControl(); + $permissions = array( + 'global' => AccessControl::PERMISSION_MANAGE_SETTINGS, + ); + + $access_control->user_roles = array(); + $api = new JSONAPI($access_control); + expect($api->validatePermissions(null, $permissions))->false(); + + $access_control->user_roles = $access_control->permissions[AccessControl::PERMISSION_MANAGE_SETTINGS]; + $api = new JSONAPI($access_control); + expect($api->validatePermissions(null, $permissions))->true(); + } + + function testItValidatesEndpointMethodPermission() { + $access_control = new AccessControl(); + $permissions = array( + 'global' => null, + 'methods' => array( + 'test' => AccessControl::PERMISSION_MANAGE_SETTINGS + ) + ); + + $access_control->user_roles = array(); + $api = new JSONAPI($access_control); + expect($api->validatePermissions('test', $permissions))->false(); + + $access_control->user_roles = $access_control->permissions[AccessControl::PERMISSION_MANAGE_SETTINGS]; + $api = new JSONAPI($access_control); + expect($api->validatePermissions('test', $permissions))->true(); + } + function _after() { + WPHooksHelper::releaseAllHooks(); wp_delete_user($this->wp_user_id); } } \ No newline at end of file diff --git a/tests/unit/API/JSON/APITestNamespacedEndpointStubV1.php b/tests/unit/API/JSON/APITestNamespacedEndpointStubV1.php index 4f6d79abb5..c597d977af 100644 --- a/tests/unit/API/JSON/APITestNamespacedEndpointStubV1.php +++ b/tests/unit/API/JSON/APITestNamespacedEndpointStubV1.php @@ -3,16 +3,24 @@ namespace MailPoet\API\JSON\v1; use MailPoet\API\JSON\Endpoint as APIEndpoint; -use MailPoet\API\JSON\Access as APIAccess; +use MailPoet\Config\AccessControl; if(!defined('ABSPATH')) exit; class APITestNamespacedEndpointStubV1 extends APIEndpoint { public $permissions = array( - 'test' => APIAccess::ALL + 'global' => AccessControl::NO_ACCESS_RESTRICTION, + 'methods' => array( + 'test' => AccessControl::NO_ACCESS_RESTRICTION, + 'restricted' => AccessControl::PERMISSION_MANAGE_SETTINGS + ) ); function test($data) { return $this->successResponse($data); } + + function restricted($data) { + return $this->successResponse($data); + } } diff --git a/tests/unit/API/JSON/APITestNamespacedEndpointStubV2.php b/tests/unit/API/JSON/APITestNamespacedEndpointStubV2.php index b0029f2d81..1d3ffa51a2 100644 --- a/tests/unit/API/JSON/APITestNamespacedEndpointStubV2.php +++ b/tests/unit/API/JSON/APITestNamespacedEndpointStubV2.php @@ -2,14 +2,17 @@ namespace MailPoet\API\JSON\v2; -use MailPoet\API\JSON\Access as APIAccess; use MailPoet\API\JSON\Endpoint as APIEndpoint; +use MailPoet\Config\AccessControl; if(!defined('ABSPATH')) exit; class APITestNamespacedEndpointStubV2 extends APIEndpoint { public $permissions = array( - 'testVersion' => APIAccess::ALL + 'global' => AccessControl::NO_ACCESS_RESTRICTION, + 'methods' => array( + 'test' => AccessControl::NO_ACCESS_RESTRICTION + ) ); function testVersion() { From 7c23415d26d5f01daf3db39a160b612ad7bad5d6 Mon Sep 17 00:00:00 2001 From: Vlad Date: Wed, 23 Aug 2017 09:55:08 -0400 Subject: [PATCH 19/24] Updates unit test as a result of AccessControl implementation --- tests/unit/Config/MenuTest.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/unit/Config/MenuTest.php b/tests/unit/Config/MenuTest.php index 3312e262c1..7a81f40dee 100644 --- a/tests/unit/Config/MenuTest.php +++ b/tests/unit/Config/MenuTest.php @@ -1,7 +1,9 @@ Date: Wed, 23 Aug 2017 11:25:39 -0400 Subject: [PATCH 20/24] Moves AccessControl initialization outside of API to Initializer --- lib/API/API.php | 4 ++-- lib/Config/Initializer.php | 2 +- lib/Form/Widget.php | 4 ++-- lib/Subscription/Form.php | 3 ++- tests/unit/API/APITest.php | 3 ++- tests/unit/API/JSON/APITest.php | 2 +- 6 files changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/API/API.php b/lib/API/API.php index 0212981bf3..ef65f9dfd5 100644 --- a/lib/API/API.php +++ b/lib/API/API.php @@ -1,4 +1,5 @@ init(); + API\API::JSON($this->access_control)->init(); } function setupRouter() { diff --git a/lib/Form/Widget.php b/lib/Form/Widget.php index 9953961c4a..a93a95516e 100644 --- a/lib/Form/Widget.php +++ b/lib/Form/Widget.php @@ -3,7 +3,7 @@ namespace MailPoet\Form; use MailPoet\API\JSON\API; -use MailPoet\Config\Renderer; +use MailPoet\Config\Renderer as ConfigRenderer; use MailPoet\Form\Renderer as FormRenderer; use MailPoet\Models\Form; use MailPoet\Util\Security; @@ -174,7 +174,7 @@ class Widget extends \WP_Widget { $data['api_version'] = API::CURRENT_VERSION; // render form - $renderer = new Renderer(); + $renderer = new ConfigRenderer(); try { $output = $renderer->render('form/widget.html', $data); $output = do_shortcode($output); diff --git a/lib/Subscription/Form.php b/lib/Subscription/Form.php index 3a768df770..0dea09a960 100644 --- a/lib/Subscription/Form.php +++ b/lib/Subscription/Form.php @@ -4,12 +4,13 @@ namespace MailPoet\Subscription; use MailPoet\API\API as API; use MailPoet\API\JSON\Response as APIResponse; +use MailPoet\Config\AccessControl; use MailPoet\Util\Url as UrlHelper; class Form { static function onSubmit($request_data = false) { $request_data = ($request_data) ? $request_data : $_REQUEST; - $api = API::JSON(); + $api = API::JSON(new AccessControl()); $api->setRequestData($request_data); $form_id = (!empty($request_data['data']['form_id'])) ? (int)$request_data['data']['form_id'] : false; $response = $api->processRoute(); diff --git a/tests/unit/API/APITest.php b/tests/unit/API/APITest.php index ac98bc1909..69d2c76cbe 100644 --- a/tests/unit/API/APITest.php +++ b/tests/unit/API/APITest.php @@ -2,10 +2,11 @@ namespace MailPoet\Test\API; use MailPoet\API\API; +use MailPoet\Config\AccessControl; class APITest extends \MailPoetTest { function testItCallsJSONAPI() { - expect(API::JSON())->isInstanceOf('MailPoet\API\JSON\API'); + expect(API::JSON(new AccessControl()))->isInstanceOf('MailPoet\API\JSON\API'); } function testItCallsMPAPI() { diff --git a/tests/unit/API/JSON/APITest.php b/tests/unit/API/JSON/APITest.php index ba06c8a7a3..f92856ce21 100644 --- a/tests/unit/API/JSON/APITest.php +++ b/tests/unit/API/JSON/APITest.php @@ -28,7 +28,7 @@ class APITest extends \MailPoetTest { } else { $this->wp_user_id = $wp_user_id; } - $this->api = API::JSON(); + $this->api = API::JSON(new AccessControl()); } function testItCallsAPISetupAction() { From eac6b1b4144be57b45cbb6d1c8671f595bb75a1f Mon Sep 17 00:00:00 2001 From: Vlad Date: Wed, 23 Aug 2017 11:45:33 -0400 Subject: [PATCH 21/24] Corrects coding style --- lib/Config/Menu.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Config/Menu.php b/lib/Config/Menu.php index 16c9ff43f5..947e374e91 100644 --- a/lib/Config/Menu.php +++ b/lib/Config/Menu.php @@ -73,7 +73,7 @@ class Menu { ); // Emails page - if ($this->access_control->validatePermission(AccessControl::PERMISSION_MANAGE_EMAILS)) { + if($this->access_control->validatePermission(AccessControl::PERMISSION_MANAGE_EMAILS)) { $newsletters_page = add_submenu_page( self::MAIN_PAGE_SLUG, $this->setPageTitle(__('Emails', 'mailpoet')), From 7d9b4b31aaa6de48ce36857ef1136eb2a927fbac Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 24 Aug 2017 13:37:49 -0400 Subject: [PATCH 22/24] Removes unused constructor parameter --- lib/Config/Initializer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Config/Initializer.php b/lib/Config/Initializer.php index 345a52f015..4f2ceaaa60 100644 --- a/lib/Config/Initializer.php +++ b/lib/Config/Initializer.php @@ -139,7 +139,7 @@ class Initializer { if(!$this->access_control->validatePermission(AccessControl::PERMISSION_UPDATE_PLUGIN)) { throw new \Exception(__('You do not have permission to activate/deactivate MailPoet plugin.', 'mailpoet')); } - $activator = new Activator($this->access_control); + $activator = new Activator(); $activator->activate(); } } From af58814fe7ec5d48a113b91f8ed31aca2175e57d Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 24 Aug 2017 13:56:17 -0400 Subject: [PATCH 23/24] Moves AccessControl intialization outside of Router to Initializer --- lib/Config/Initializer.php | 2 +- lib/Router/Router.php | 4 ++-- tests/unit/Router/RouterTest.php | 30 +++++++++++++++--------------- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/lib/Config/Initializer.php b/lib/Config/Initializer.php index 4f2ceaaa60..13d95d0e3c 100644 --- a/lib/Config/Initializer.php +++ b/lib/Config/Initializer.php @@ -227,7 +227,7 @@ class Initializer { } function setupRouter() { - $router = new Router\Router(); + $router = new Router\Router($this->access_control); $router->init(); } diff --git a/lib/Router/Router.php b/lib/Router/Router.php index fb9ee2a207..fba4148fcb 100644 --- a/lib/Router/Router.php +++ b/lib/Router/Router.php @@ -16,7 +16,7 @@ class Router { const RESPONSE_ERROR = 404; const RESPONE_FORBIDDEN = 403; - function __construct($api_data = false) { + function __construct(AccessControl $access_control, $api_data = false) { $api_data = ($api_data) ? $api_data : $_GET; $this->api_request = isset($api_data[self::NAME]); $this->endpoint = isset($api_data['endpoint']) ? @@ -28,7 +28,7 @@ class Router { $this->data = isset($api_data['data']) ? self::decodeRequestData($api_data['data']) : false; - $this->access_control = new AccessControl(); + $this->access_control = $access_control; } function init() { diff --git a/tests/unit/Router/RouterTest.php b/tests/unit/Router/RouterTest.php index 2eca6d6649..27d1d75fc1 100644 --- a/tests/unit/Router/RouterTest.php +++ b/tests/unit/Router/RouterTest.php @@ -9,18 +9,18 @@ use MailPoet\Router\Router; require_once('RouterTestMockEndpoint.php'); class RouterTest extends \MailPoetTest { + public $access_control; public $router_data; - public $router; - function __construct() { - parent::__construct(); + function _before() { $this->router_data = array( Router::NAME => '', 'endpoint' => 'router_test_mock_endpoint', 'action' => 'test', 'data' => base64_encode(json_encode(array('data' => 'dummy data'))) ); - $this->router = new Router($this->router_data); + $this->access_control = new AccessControl(); + $this->router = new Router($this->access_control, $this->router_data); } function testItCanGetAPIDataFromGetRequest() { @@ -28,7 +28,7 @@ class RouterTest extends \MailPoetTest { $url = 'http://example.com/?' . Router::NAME . '&endpoint=view_in_browser&action=view&data=' . base64_encode(json_encode($data)); parse_str(parse_url($url, PHP_URL_QUERY), $_GET); - $router = new Router(); + $router = new Router($this->access_control); expect($router->api_request)->equals(true); expect($router->endpoint)->equals('viewInBrowser'); expect($router->endpoint_action)->equals('view'); @@ -39,8 +39,8 @@ class RouterTest extends \MailPoetTest { $router_data = $this->router_data; unset($router_data[Router::NAME]); $router = Stub::construct( - new Router(), - array($router_data) + '\MailPoet\Router\Router', + array($this->access_control, $router_data) ); $result = $router->init(); expect($result)->null(); @@ -50,8 +50,8 @@ class RouterTest extends \MailPoetTest { $router_data = $this->router_data; $router_data['endpoint'] = 'invalid_endpoint'; $router = Stub::construct( - new Router(), - array($router_data), + '\MailPoet\Router\Router', + array($this->access_control, $router_data), array( 'terminateRequest' => function($code, $error) { return array( @@ -74,8 +74,8 @@ class RouterTest extends \MailPoetTest { $router_data = $this->router_data; $router_data['action'] = 'invalid_action'; $router = Stub::construct( - new Router(), - array($router_data), + '\MailPoet\Router\Router', + array($this->access_control, $router_data), array( 'terminateRequest' => function($code, $error) { return array( @@ -132,8 +132,8 @@ class RouterTest extends \MailPoetTest { function testItValidatesPermissionBeforeProcessingEndpointAction() { $router = Stub::construct( - new Router(), - array($this->router_data), + '\MailPoet\Router\Router', + array($this->access_control, $this->router_data), array( 'validatePermissions' => function($action, $permissions) { expect($action)->equals($this->router_data['action']); @@ -154,8 +154,8 @@ class RouterTest extends \MailPoetTest { function testItReturnsForbiddenResponseWhenPermissionFailsValidation() { $router = Stub::construct( - new Router(), - array($this->router_data), + '\MailPoet\Router\Router', + array($this->access_control, $this->router_data), array( 'validatePermissions' => false, 'terminateRequest' => function($code, $error) { From 26bccd95d49351839a64b90e5e60b1fe971878fa Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 24 Aug 2017 13:58:54 -0400 Subject: [PATCH 24/24] Uses method vs. accessing class internals to get user capability --- lib/Config/Menu.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Config/Menu.php b/lib/Config/Menu.php index 947e374e91..521a598345 100644 --- a/lib/Config/Menu.php +++ b/lib/Config/Menu.php @@ -647,7 +647,7 @@ class Menu { true, 'MailPoet', 'MailPoet', - $access_control->user_capabilities[0], + $access_control->getUserFirstCapability(), $_REQUEST['page'], array( __CLASS__,