From d0c9daa49c24f295caa7527544cbbca459d33296 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Thu, 27 Mar 2014 17:27:41 -0700 Subject: [PATCH] CRM-14370 - Convert _civicrm_api3_api_check_permission to Civi\API\Subscriber\PermissionCheck Also: Restrict old permissions checks to APIv3 Conflicts: Civi/Core/Container.php --- Civi/API/Subscriber/PermissionCheck.php | 77 +++++++++++++++++++++++++ Civi/Core/Container.php | 7 +-- api/v3/utils.php | 42 -------------- tests/phpunit/api/v3/UtilsTest.php | 43 +++++++++++--- 4 files changed, 112 insertions(+), 57 deletions(-) create mode 100644 Civi/API/Subscriber/PermissionCheck.php diff --git a/Civi/API/Subscriber/PermissionCheck.php b/Civi/API/Subscriber/PermissionCheck.php new file mode 100644 index 0000000000..f108d7b152 --- /dev/null +++ b/Civi/API/Subscriber/PermissionCheck.php @@ -0,0 +1,77 @@ + array( + array('onApiAuthorize', Events::W_LATE), + ), + ); + } + + public function onApiAuthorize(\Civi\API\Event\AuthorizeEvent $event) { + $apiRequest = $event->getApiRequest(); + if ($apiRequest['version'] < 4) { + // return early unless we’re told explicitly to do the permission check + if (empty($apiRequest['params']['check_permissions']) or $apiRequest['params']['check_permissions'] == FALSE) { + $event->authorize(); + $event->stopPropagation(); + return; + } + + require_once 'CRM/Core/DAO/permissions.php'; + $permissions = _civicrm_api3_permissions($apiRequest['entity'], $apiRequest['action'], $apiRequest['params']); + + // $params might’ve been reset by the alterAPIPermissions() hook + if (isset($apiRequest['params']['check_permissions']) and $apiRequest['params']['check_permissions'] == FALSE) { + $event->authorize(); + $event->stopPropagation(); + return; + } + + if (!\CRM_Core_Permission::check($permissions)) { + if (is_array($permissions)) { + $permissions = implode(' and ', $permissions); + } + // FIXME: Generating the exception ourselves allows for detailed error but doesn't play well with multiple authz subscribers. + throw new \API_Exception("API permission check failed for {$apiRequest['entity']}/{$apiRequest['action']} call; insufficient permission: require $permissions", \API_Exception::UNAUTHORIZED); + } + + $event->authorize(); + $event->stopPropagation(); + } + } +} \ No newline at end of file diff --git a/Civi/Core/Container.php b/Civi/Core/Container.php index f6d51c5162..c5ba5c7cd5 100644 --- a/Civi/Core/Container.php +++ b/Civi/Core/Container.php @@ -89,6 +89,7 @@ class Container { $dispatcher->addSubscriber(new \Civi\API\Subscriber\TransactionSubscriber()); $dispatcher->addSubscriber(new \Civi\API\Subscriber\I18nSubscriber()); $dispatcher->addSubscriber(new \Civi\API\Provider\MagicFunctionProvider()); + $dispatcher->addSubscriber(new \Civi\API\Subscriber\PermissionCheck()); $dispatcher->addSubscriber(new \Civi\API\Subscriber\APIv3SchemaAdapter()); $dispatcher->addSubscriber(new \Civi\API\Subscriber\WrapperAdapter(array( \CRM_Utils_API_HTMLInputCoder::singleton(), @@ -97,12 +98,6 @@ class Container { \CRM_Utils_API_MatchOption::singleton(), ))); $dispatcher->addSubscriber(new \Civi\API\Subscriber\XDebugSubscriber()); - $dispatcher->addListener(\Civi\API\Events::AUTHORIZE, function(\Civi\API\Event\AuthorizeEvent $event) { - $apiRequest = $event->getApiRequest(); - // At time of writing, _civicrm_api3_api_check_permission generates an exception on failure - _civicrm_api3_api_check_permission($apiRequest['entity'], $apiRequest['action'], $apiRequest['params']); - $event->authorize(); - }); $kernel = new \Civi\API\Kernel($dispatcher, array()); return $kernel; } diff --git a/api/v3/utils.php b/api/v3/utils.php index 657011b4b5..cd7fb34b5b 100644 --- a/api/v3/utils.php +++ b/api/v3/utils.php @@ -978,48 +978,6 @@ function _civicrm_api3_check_required_fields($params, $daoName, $return = FALSE) return TRUE; } -/** - * Check permissions for a given API call. - * - * @param $entity string API entity being accessed - * @param $action string API action being performed - * @param $params array params of the API call - * @param $throw deprecated bool whether to throw exception instead of returning false - * - * @throws Exception - * @return bool whether the current API user has the permission to make the call - */ -function _civicrm_api3_api_check_permission($entity, $action, &$params, $throw = TRUE) { - // return early unless we’re told explicitly to do the permission check - if (empty($params['check_permissions']) or $params['check_permissions'] == FALSE) { - return TRUE; - } - - require_once 'CRM/Core/DAO/permissions.php'; - $permissions = _civicrm_api3_permissions($entity, $action, $params); - - // $params might’ve been reset by the alterAPIPermissions() hook - if (isset($params['check_permissions']) and $params['check_permissions'] == FALSE) { - return TRUE; - } - - if (!CRM_Core_Permission::check($permissions)) { - if ($throw) { - if(is_array($permissions)) { - $permissions = implode(' and ', $permissions); - } - throw new Exception("API permission check failed for $entity/$action call; insufficient permission: require $permissions"); - } - else { - //@todo remove this - this is an internal api function called with $throw set to TRUE. It is only called with false - // in tests & that should be tidied up - return FALSE; - } - } - - return TRUE; -} - /** * Function to do a 'standard' api get - when the api is only doing a $bao->find then use this * diff --git a/tests/phpunit/api/v3/UtilsTest.php b/tests/phpunit/api/v3/UtilsTest.php index 00c8d89cf0..651cc46d7c 100644 --- a/tests/phpunit/api/v3/UtilsTest.php +++ b/tests/phpunit/api/v3/UtilsTest.php @@ -68,17 +68,17 @@ class api_v3_UtilsTest extends CiviUnitTestCase { $check = array('check_permissions' => TRUE); $config = CRM_Core_Config::singleton(); $config->userPermissionClass->permissions = array(); - $this->assertFalse(_civicrm_api3_api_check_permission('contact', 'create', $check, FALSE), 'empty permissions should not be enough'); + $this->assertFalse($this->runPermissionCheck('contact', 'create', $check), 'empty permissions should not be enough'); $config->userPermissionClass->permissions = array('access CiviCRM'); - $this->assertFalse(_civicrm_api3_api_check_permission('contact', 'create', $check, FALSE), 'lacking permissions should not be enough'); + $this->assertFalse($this->runPermissionCheck('contact', 'create', $check), 'lacking permissions should not be enough'); $config->userPermissionClass->permissions = array('add contacts'); - $this->assertFalse(_civicrm_api3_api_check_permission('contact', 'create', $check, FALSE), 'lacking permissions should not be enough'); + $this->assertFalse($this->runPermissionCheck('contact', 'create', $check), 'lacking permissions should not be enough'); $config->userPermissionClass->permissions = array('access CiviCRM', 'add contacts'); - $this->assertTrue(_civicrm_api3_api_check_permission('contact', 'create', $check, FALSE), 'exact permissions should be enough'); + $this->assertTrue($this->runPermissionCheck('contact', 'create', $check), 'exact permissions should be enough'); $config->userPermissionClass->permissions = array('access CiviCRM', 'add contacts', 'import contacts'); - $this->assertTrue(_civicrm_api3_api_check_permission('contact', 'create', $check, FALSE), 'overfluous permissions should be enough'); + $this->assertTrue($this->runPermissionCheck('contact', 'create', $check), 'overfluous permissions should be enough'); } function testCheckPermissionThrow() { @@ -86,7 +86,7 @@ class api_v3_UtilsTest extends CiviUnitTestCase { $config = CRM_Core_Config::singleton(); try { $config->userPermissionClass->permissions = array('access CiviCRM'); - _civicrm_api3_api_check_permission('contact', 'create', $check); + $this->runPermissionCheck('contact', 'create', $check, TRUE); } catch(Exception $e) { $message = $e->getMessage(); @@ -94,16 +94,41 @@ class api_v3_UtilsTest extends CiviUnitTestCase { $this->assertEquals($message, 'API permission check failed for contact/create call; insufficient permission: require access CiviCRM and add contacts', 'lacking permissions should throw an exception'); $config->userPermissionClass->permissions = array('access CiviCRM', 'add contacts', 'import contacts'); - $this->assertTrue(_civicrm_api3_api_check_permission('contact', 'create', $check), 'overfluous permissions should return true'); + $this->assertTrue($this->runPermissionCheck('contact', 'create', $check), 'overfluous permissions should return true'); } function testCheckPermissionSkip() { $config = CRM_Core_Config::singleton(); $config->userPermissionClass->permissions = array('access CiviCRM'); $params = array('check_permissions' => TRUE); - $this->assertFalse(_civicrm_api3_api_check_permission('contact', 'create', $params, FALSE), 'lacking permissions should not be enough'); + $this->assertFalse($this->runPermissionCheck('contact', 'create', $params), 'lacking permissions should not be enough'); $params = array('check_permissions' => FALSE); - $this->assertTrue(_civicrm_api3_api_check_permission('contact', 'create', $params, FALSE), 'permission check should be skippable'); + $this->assertTrue($this->runPermissionCheck('contact', 'create', $params), 'permission check should be skippable'); + } + + /** + * @param string $entity + * @param string $action + * @param array $params + * @param bool $throws whether we should pass any exceptions for authorization failures + * @return bool TRUE or FALSE depending on the outcome of the authorization check + */ + function runPermissionCheck($entity, $action, $params, $throws = FALSE) { + $dispatcher = new \Symfony\Component\EventDispatcher\EventDispatcher(); + $dispatcher->addSubscriber(new \Civi\API\Subscriber\PermissionCheck()); + $kernel = new \Civi\API\Kernel($dispatcher); + $apiRequest = $kernel->createRequest($entity, $action, $params, NULL); + try { + $kernel->authorize(NULL, $apiRequest); + return TRUE; + } catch (\API_Exception $e) { + $extra = $e->getExtraParams(); + if (!$throws && $extra['error_code'] == API_Exception::UNAUTHORIZED) { + return FALSE; + } else { + throw $e; + } + } } /* -- 2.25.1