diff options
author | Matthieu Napoli <matthieu@mnapoli.fr> | 2015-05-10 05:42:31 +0300 |
---|---|---|
committer | Matthieu Napoli <matthieu@mnapoli.fr> | 2015-05-10 05:42:31 +0300 |
commit | 2db472c33ba7e131f65c83b8b4f4cebb1d7a8297 (patch) | |
tree | bb1b9351af2a175daa7fbfc21f6d9556c43d5652 | |
parent | 179ed7eedd8d8ba092122a336bbe91b9eccb1099 (diff) | |
parent | 6f6eb151d69d9cadb0717964a284840a21dab410 (diff) |
Merge pull request #7804 from piwik/restore_access
Restore auth when calling API only if needed
-rw-r--r-- | core/API/Request.php | 93 | ||||
-rw-r--r-- | core/Access.php | 3 | ||||
-rw-r--r-- | core/Tracker/Request.php | 2 | ||||
-rw-r--r-- | plugins/API/tests/Integration/APITest.php | 94 | ||||
-rw-r--r-- | plugins/Login/Login.php | 2 | ||||
-rwxr-xr-x | tests/PHPUnit/Framework/TestCase/SystemTestCase.php | 4 | ||||
-rw-r--r-- | tests/PHPUnit/Integration/API/RequestTest.php | 146 | ||||
-rw-r--r-- | tests/PHPUnit/Integration/AccessTest.php | 26 |
8 files changed, 346 insertions, 24 deletions
diff --git a/core/API/Request.php b/core/API/Request.php index 04d055f859..a21212e729 100644 --- a/core/API/Request.php +++ b/core/API/Request.php @@ -213,32 +213,61 @@ class Request $corsHandler = new CORSHandler(); $corsHandler->handle(); + $tokenAuth = Common::getRequestVar('token_auth', '', 'string', $this->request); + $shouldReloadAuth = false; + try { // read parameters $moduleMethod = Common::getRequestVar('method', null, 'string', $this->request); list($module, $method) = $this->extractModuleAndMethod($moduleMethod); - list($module, $method) = self::getRenamedModuleAndAction($module, $method); PluginManager::getInstance()->checkIsPluginActivated($module); $apiClassName = self::getClassNameAPI($module); - self::reloadAuthUsingTokenAuth($this->request); + if ($shouldReloadAuth = self::shouldReloadAuthUsingTokenAuth($this->request)) { + $access = Access::getInstance(); + $tokenAuthToRestore = $access->getTokenAuth(); + $hadSuperUserAccess = $access->hasSuperUserAccess(); + self::forceReloadAuthUsingTokenAuth($tokenAuth); + } // call the method $returnedValue = Proxy::getInstance()->call($apiClassName, $method, $this->request); $toReturn = $response->getResponse($returnedValue, $module, $method); + } catch (Exception $e) { Log::debug($e); $toReturn = $response->getResponseException($e); } + + if ($shouldReloadAuth) { + $this->restoreAuthUsingTokenAuth($tokenAuthToRestore, $hadSuperUserAccess); + } + return $toReturn; } + private function restoreAuthUsingTokenAuth($tokenToRestore, $hadSuperUserAccess) + { + // if we would not make sure to unset super user access, the tokenAuth would be not authenticated and any + // token would just keep super user access (eg if the token that was reloaded before had super user access) + Access::getInstance()->setSuperUserAccess(false); + + // we need to restore by reloading the tokenAuth as some permissions could have been removed in the API + // request etc. Otherwise we could just store a clone of Access::getInstance() and restore here + self::forceReloadAuthUsingTokenAuth($tokenToRestore); + + if ($hadSuperUserAccess && !Access::getInstance()->hasSuperUserAccess()) { + // we are in context of `doAsSuperUser()` and need to restore this behaviour + Access::getInstance()->setSuperUserAccess(true); + } + } + /** * Returns the name of a plugin's API class by plugin name. * @@ -263,27 +292,53 @@ class Request { // if a token_auth is specified in the API request, we load the right permissions $token_auth = Common::getRequestVar('token_auth', '', 'string', $request); - $access = Access::getInstance(); - - if ($token_auth && $token_auth !== $access->getTokenAuth()) { - - /** - * Triggered when authenticating an API request, but only if the **token_auth** - * query parameter is found in the request. - * - * Plugins that provide authentication capabilities should subscribe to this event - * and make sure the global authentication object (the object returned by `StaticContainer::get('Piwik\Auth')`) - * is setup to use `$token_auth` when its `authenticate()` method is executed. - * - * @param string $token_auth The value of the **token_auth** query parameter. - */ - Piwik::postEvent('API.Request.authenticate', array($token_auth)); - Access::getInstance()->reloadAccess(); - SettingsServer::raiseMemoryLimitIfNecessary(); + + if (self::shouldReloadAuthUsingTokenAuth($request)) { + self::forceReloadAuthUsingTokenAuth($token_auth); } } /** + * The current session will be authenticated using this token_auth. + * It will overwrite the previous Auth object. + * + * @param string $tokenAuth + * @return void + */ + private static function forceReloadAuthUsingTokenAuth($tokenAuth) + { + /** + * Triggered when authenticating an API request, but only if the **token_auth** + * query parameter is found in the request. + * + * Plugins that provide authentication capabilities should subscribe to this event + * and make sure the global authentication object (the object returned by `StaticContainer::get('Piwik\Auth')`) + * is setup to use `$token_auth` when its `authenticate()` method is executed. + * + * @param string $token_auth The value of the **token_auth** query parameter. + */ + Piwik::postEvent('API.Request.authenticate', array($tokenAuth)); + Access::getInstance()->reloadAccess(); + SettingsServer::raiseMemoryLimitIfNecessary(); + } + + private static function shouldReloadAuthUsingTokenAuth($request) + { + if (!isset($request['token_auth'])) { + // no token is given so we just keep the current loaded user + return false; + } + + // a token is specified, we need to reload auth in case it is different than the current one, even if it is empty + $tokenAuth = Common::getRequestVar('token_auth', '', 'string', $request); + + // not using !== is on purpose as getTokenAuth() might return null whereas $tokenAuth is '' . In this case + // we do not need to reload. + + return $tokenAuth != Access::getInstance()->getTokenAuth(); + } + + /** * Returns array($class, $method) from the given string $class.$method * * @param string $parameter diff --git a/core/Access.php b/core/Access.php index d670623755..8a3e89ae44 100644 --- a/core/Access.php +++ b/core/Access.php @@ -154,6 +154,9 @@ class Access return true; } + $this->token_auth = null; + $this->login = null; + // if the Auth wasn't set, we may be in the special case of setSuperUser(), otherwise we fail TODO: docs + review if ($this->auth === null) { return false; diff --git a/core/Tracker/Request.php b/core/Tracker/Request.php index 7194101ee3..7eed4104e0 100644 --- a/core/Tracker/Request.php +++ b/core/Tracker/Request.php @@ -172,7 +172,7 @@ class Request if (!empty($idSite) && $idSite > 0) { $website = Cache::getCacheWebsiteAttributes($idSite); - if (array_key_exists('admin_token_auth', $website) && in_array($tokenAuth, $website['admin_token_auth'])) { + if (array_key_exists('admin_token_auth', $website) && in_array((string) $tokenAuth, $website['admin_token_auth'])) { return true; } } diff --git a/plugins/API/tests/Integration/APITest.php b/plugins/API/tests/Integration/APITest.php new file mode 100644 index 0000000000..2362bebdf2 --- /dev/null +++ b/plugins/API/tests/Integration/APITest.php @@ -0,0 +1,94 @@ +<?php +/** + * Piwik - free/libre analytics platform + * + * @link http://piwik.org + * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later + */ + +namespace Piwik\Plugins\API\tests\Integration; + +use Piwik\Access; +use Piwik\API\Request; +use Piwik\Container\StaticContainer; +use Piwik\Piwik; +use Piwik\Plugins\API\API; +use Piwik\Tests\Framework\Fixture; +use Piwik\Tests\Framework\TestCase\IntegrationTestCase; + +/** + * @group API + * @group APITest + * @group Plugins + */ +class APITest extends IntegrationTestCase +{ + /** + * @var API + */ + private $api; + + private $hasSuperUserAccess = false; + + public function setUp() + { + parent::setUp(); + + $this->api = API::getInstance(); + + Fixture::createSuperUser(true); + + if (!Fixture::siteCreated(1)) { + Fixture::createWebsite('2012-01-01 00:00:00'); + } + + $this->makeSureTestRunsInContextOfAnonymousUser(); + } + + public function tearDown() + { + Access::getInstance()->hasSuperUserAccess($this->hasSuperUserAccess); + parent::tearDown(); + } + + public function test_getBulkRequest_IsAbleToHandleManyDifferentRequests() + { + $token = Fixture::getTokenAuth(); + $urls = array( + "method%3dVisitsSummary.get%26idSite%3d1%26date%3d2015-01-26%26period%3dday", + "method%3dVisitsSummary.get%26token_auth%3d$token%26idSite%3d1%26date%3d2015-01-26%26period%3dday", + "method%3dVisitsSummary.get%26idSite%3d1%26date%3d2015-01-26%26period%3dday", + "method%3dVisitsSummary.get%26idSite%3d1%26token_auth%3danonymous%26date%3d2015-01-26%26period%3dday", + ); + $response = $this->api->getBulkRequest($urls); + + $this->assertResponseIsPermissionError($response[0]); + $this->assertResponseIsSuccess($response[1]); + $this->assertSame(0, $response[1]['nb_visits']); + $this->assertResponseIsPermissionError($response[2]); + $this->assertResponseIsPermissionError($response[3]); + } + + private function assertResponseIsPermissionError($response) + { + $this->assertSame('error', $response['result']); + $this->assertStringStartsWith('General_ExceptionPrivilegeAccessWebsite', $response['message']); + } + + private function assertResponseIsSuccess($response) + { + $this->assertArrayNotHasKey('result', $response); + } + + private function makeSureTestRunsInContextOfAnonymousUser() + { + Piwik::postEvent('Request.initAuthenticationObject'); + + $access = Access::getInstance(); + $this->hasSuperUserAccess = $access->hasSuperUserAccess(); + $access->setSuperUserAccess(false); + $access->reloadAccess(StaticContainer::get('Piwik\Auth')); + Request::reloadAuthUsingTokenAuth(array('token_auth' => 'anonymous')); + } + +} diff --git a/plugins/Login/Login.php b/plugins/Login/Login.php index ccaa203112..e8e74ed37f 100644 --- a/plugins/Login/Login.php +++ b/plugins/Login/Login.php @@ -13,9 +13,7 @@ use Piwik\Config; use Piwik\Container\StaticContainer; use Piwik\Cookie; use Piwik\FrontController; -use Piwik\Option; use Piwik\Piwik; -use Piwik\Plugins\UsersManager\UsersManager; use Piwik\Session; /** diff --git a/tests/PHPUnit/Framework/TestCase/SystemTestCase.php b/tests/PHPUnit/Framework/TestCase/SystemTestCase.php index f8944bf880..cf26fde724 100755 --- a/tests/PHPUnit/Framework/TestCase/SystemTestCase.php +++ b/tests/PHPUnit/Framework/TestCase/SystemTestCase.php @@ -597,9 +597,9 @@ abstract class SystemTestCase extends PHPUnit_Framework_TestCase } } - public function assertHttpResponseText($expectedResponseCode, $url, $message = '') + public function assertHttpResponseText($expectedResponseText, $url, $message = '') { - self::assertThat($url, new HttpResponseText($expectedResponseCode), $message); + self::assertThat($url, new HttpResponseText($expectedResponseText), $message); } public function assertResponseCode($expectedResponseCode, $url, $message = '') diff --git a/tests/PHPUnit/Integration/API/RequestTest.php b/tests/PHPUnit/Integration/API/RequestTest.php new file mode 100644 index 0000000000..9394d28306 --- /dev/null +++ b/tests/PHPUnit/Integration/API/RequestTest.php @@ -0,0 +1,146 @@ +<?php +/** + * Piwik - free/libre analytics platform + * + * @link http://piwik.org + * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later + */ + +namespace Piwik\Tests\Integration\API; + +use Piwik\Access; +use Piwik\API\Request; +use Piwik\AuthResult; +use Piwik\Container\StaticContainer; +use Piwik\Db; +use Piwik\Tests\Framework\TestCase\IntegrationTestCase; + +/** + * @group Core + */ +class RequestTest extends IntegrationTestCase +{ + /** @var \Piwik\Auth|\PHPUnit_Framework_MockObject_MockObject */ + private $auth; + /** @var \Piwik\Access|\PHPUnit_Framework_MockObject_MockObject */ + private $access; + + private $userAuthToken = 'token'; + + public function setUp() + { + parent::setUp(); + + $this->auth = $this->createAuthMock(); + $this->access = $this->createAccessMock($this->auth); + Access::setSingletonInstance($this->access); + } + + public function test_process_shouldNotReloadAccessIfNoTokenAuthIsGiven() + { + $this->assertAccessNotReloaded(); + + $request = new Request(array('method' => 'API.getPiwikVersion')); + $request->process(); + + $this->assertSameUserAsBeforeIsAuthenticated(); + } + + public function test_process_shouldNotReloadAccessIfSameAuthTokenIsAlreadyLoaded() + { + $this->assertAccessNotReloaded(); + + $request = new Request(array('method' => 'API.getPiwikVersion', 'token_auth' => $this->access->getTokenAuth())); + $request->process(); + + $this->assertSameUserAsBeforeIsAuthenticated(); + } + + public function test_process_shouldReloadAccessIfAuthTokenIsDifferent() + { + // make sure tokenAuth is different then set 'AnYTOkEN' token + $this->assertEquals('token', $this->access->getTokenAuth()); + + $this->assertAccessReloadedAndRestored('AnYTOkEN'); + + $request = new Request(array('method' => 'API.getPiwikVersion', 'token_auth' => 'AnYTOkEN')); + $request->process(); + + // make sure token auth was restored after it was loaded with AnYTOkEN + $this->assertSameUserAsBeforeIsAuthenticated(); + } + + public function test_process_shouldReloadAccessIfAuthTokenIsDifferentButEmpty() + { + $this->assertEquals('token', $this->access->getTokenAuth()); + $this->assertAccessReloadedAndRestored(''); + + $request = new Request(array('method' => 'API.getPiwikVersion', 'token_auth' => '')); + $request->process(); + + $this->assertSameUserAsBeforeIsAuthenticated(); + } + + public function test_process_shouldKeepSuperUserPermission_IfAccessWasManuallySet() + { + $this->access->setSuperUserAccess(true); + $this->assertAccessReloadedAndRestored('difFenrenT'); + + $request = new Request(array('method' => 'API.getPiwikVersion', 'token_auth' => 'difFenrenT')); + $request->process(); + + // make sure token auth was restored after it was loaded with difFenrenT + $this->assertSameUserAsBeforeIsAuthenticated(); + $this->assertTrue($this->access->hasSuperUserAccess()); + } + + private function assertSameUserAsBeforeIsAuthenticated() + { + $this->assertEquals($this->userAuthToken, $this->access->getTokenAuth()); + } + + private function assertAccessNotReloaded() + { + $this->access->expects($this->never())->method('reloadAccess'); + } + + private function assertAccessReloadedAndRestored($expectedTokenToBeReloaded) + { + $this->access->expects($this->exactly(2))->method('reloadAccess'); + + // verify access reloaded + $this->auth->expects($this->at(0))->method('setLogin')->with($this->equalTo(null)); + $this->auth->expects($this->at(1))->method('setTokenAuth')->with($this->equalTo($expectedTokenToBeReloaded)); + $this->auth->expects($this->at(2))->method('authenticate')->will($this->returnValue(new AuthResult(AuthResult::SUCCESS, 'login1', $expectedTokenToBeReloaded))); + + // verify access restored + $this->auth->expects($this->at(3))->method('setLogin')->with($this->equalTo(null)); + $this->auth->expects($this->at(4))->method('setTokenAuth')->with($this->equalTo($tokenRestored = $this->userAuthToken)); + $this->auth->expects($this->at(5))->method('authenticate')->will($this->returnValue(new AuthResult(AuthResult::SUCCESS, 'login', $this->userAuthToken))); + } + + private function createAuthMock() + { + $authMock = $this->getMock('Piwik\Plugins\Login\Auth', array('authenticate', 'setTokenAuth', 'setLogin')); + + $authMock->expects($this->any()) + ->method('authenticate') + ->will($this->returnValue(new AuthResult(AuthResult::SUCCESS, 'login', $this->userAuthToken))); + + StaticContainer::getContainer()->set('Piwik\Auth', $authMock); + + return $authMock; + } + + private function createAccessMock($auth) + { + $mock = $this->getMockBuilder('Piwik\Access') + ->setMethods(array('getTokenAuth', 'reloadAccess')) + ->enableProxyingToOriginalMethods() + ->getMock(); + $mock->reloadAccess($auth); + + return $mock; + } + +} diff --git a/tests/PHPUnit/Integration/AccessTest.php b/tests/PHPUnit/Integration/AccessTest.php index a105b72874..dcb49ad5db 100644 --- a/tests/PHPUnit/Integration/AccessTest.php +++ b/tests/PHPUnit/Integration/AccessTest.php @@ -332,6 +332,22 @@ class AccessTest extends IntegrationTestCase $this->assertTrue($access->reloadAccess(null)); } + public function testReloadAccess_ShouldResetTokenAuthAndLogin_IfAuthIsNotValid() + { + $mock = $this->createAuthMockWithAuthResult(AuthResult::SUCCESS); + $access = Access::getInstance(); + + $this->assertTrue($access->reloadAccess($mock)); + $this->assertSame('login', $access->getLogin()); + $this->assertSame('token', $access->getTokenAuth()); + + $mock = $this->createAuthMockWithAuthResult(AuthResult::FAILURE); + + $this->assertFalse($access->reloadAccess($mock)); + $this->assertNull($access->getLogin()); + $this->assertNull($access->getTokenAuth()); + } + public function testReloadAccessWithMockedAuthValid() { $mock = $this->createPiwikAuthMockInstance(); @@ -497,4 +513,14 @@ class AccessTest extends IntegrationTestCase return $mock; } + private function createAuthMockWithAuthResult($resultCode) + { + $mock = $this->createPiwikAuthMockInstance(); + $mock->expects($this->once()) + ->method('authenticate') + ->will($this->returnValue(new AuthResult($resultCode, 'login', 'token'))); + + return $mock; + } + } |