Merge pull request #7804 from piwik/restore_access

Restore auth when calling API only if needed

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;
+    }
+
 }