From df93be7bddf139ef83e09111f1e23f5024117cff Mon Sep 17 00:00:00 2001
From: Thomas Steur <thomas.steur@gmail.com>
Date: Wed, 29 Apr 2015 22:24:30 +0000
Subject: [PATCH] Restore auth after API request
---
core/API/Request.php | 93 ++++++++---
core/Access.php | 3 +
core/Tracker/Request.php | 2 +-
plugins/API/tests/Integration/APITest.php | 94 +++++++++++
plugins/Login/Login.php | 2 -
.../Framework/TestCase/SystemTestCase.php | 4 +-
tests/PHPUnit/Integration/API/RequestTest.php | 146 ++++++++++++++++++
tests/PHPUnit/Integration/AccessTest.php | 26 ++++
8 files changed, 346 insertions(+), 24 deletions(-)
create mode 100644 plugins/API/tests/Integration/APITest.php
create mode 100644 tests/PHPUnit/Integration/API/RequestTest.php
diff --git a/core/API/Request.php b/core/API/Request.php
index 04d055f859..561e2222e3 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::doReloadAuthUsingTokenAuth($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::doReloadAuthUsingTokenAuth($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,26 +292,52 @@ 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::doReloadAuthUsingTokenAuth($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 doReloadAuthUsingTokenAuth($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
*
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;
+ }
+
}
--
GitLab