diff --git a/core/Access.php b/core/Access.php
index a5f6f604c043e9f5c623b1d97f6f97b576adbc57..d6706237558be0a2df4d683d72971e6a528c96eb 100644
--- a/core/Access.php
+++ b/core/Access.php
@@ -117,6 +117,11 @@ class Access
* Constructor
*/
public function __construct()
+ {
+ $this->resetSites();
+ }
+
+ private function resetSites()
{
$this->idsitesByAccess = array(
'view' => array(),
@@ -138,19 +143,15 @@ class Access
*/
public function reloadAccess(Auth $auth = null)
{
- if (!is_null($auth)) {
- $this->auth = $auth;
- }
+ $this->resetSites();
- // if the Auth wasn't set, we may be in the special case of setSuperUser(), otherwise we fail
- if (is_null($this->auth)) {
- if ($this->hasSuperUserAccess()) {
- return $this->reloadAccessSuperUser();
- }
+ if (isset($auth)) {
+ $this->auth = $auth;
}
if ($this->hasSuperUserAccess()) {
- return $this->reloadAccessSuperUser();
+ $this->makeSureLoginNameIsSet();
+ return true;
}
// if the Auth wasn't set, we may be in the special case of setSuperUser(), otherwise we fail TODO: docs + review
@@ -170,18 +171,7 @@ class Access
// case the superUser is logged in
if ($result->hasSuperUserAccess()) {
- return $this->reloadAccessSuperUser();
- }
-
- // in case multiple calls to API using different tokens, we ensure we reset it as not SU
- $this->setSuperUserAccess(false);
-
- // we join with site in case there are rows in access for an idsite that doesn't exist anymore
- // (backward compatibility ; before we deleted the site without deleting rows in _access table)
- $accessRaw = $this->getRawSitesWithSomeViewAccess($this->login);
-
- foreach ($accessRaw as $access) {
- $this->idsitesByAccess[$access['access']][] = $access['idsite'];
+ $this->setSuperUserAccess(true);
}
return true;
@@ -210,27 +200,42 @@ class Access
}
/**
- * Reload Super User access
+ * Make sure a login name is set
*
- * @return bool
+ * @return true
*/
- protected function reloadAccessSuperUser()
+ protected function makeSureLoginNameIsSet()
{
- $this->hasSuperUserAccess = true;
-
- try {
- $allSitesId = Plugins\SitesManager\API::getInstance()->getAllSitesId();
- } catch (\Exception $e) {
- $allSitesId = array();
- }
- $this->idsitesByAccess['superuser'] = $allSitesId;
-
- if(empty($this->login)) {
+ if (empty($this->login)) {
// flag to force non empty login so Super User is not mistaken for anonymous
$this->login = 'super user was set';
}
+ }
- return true;
+ private function loadSitesIfNeeded()
+ {
+ if ($this->hasSuperUserAccess) {
+ if (empty($this->idsitesByAccess['superuser'])) {
+ try {
+ $allSitesId = Plugins\SitesManager\API::getInstance()->getAllSitesId();
+ } catch (\Exception $e) {
+ $allSitesId = array();
+ }
+ $this->idsitesByAccess['superuser'] = $allSitesId;
+ }
+ } else if (isset($this->login)) {
+ if (empty($this->idsitesByAccess['view'])
+ && empty($this->idsitesByAccess['admin'])) {
+
+ // we join with site in case there are rows in access for an idsite that doesn't exist anymore
+ // (backward compatibility ; before we deleted the site without deleting rows in _access table)
+ $accessRaw = $this->getRawSitesWithSomeViewAccess($this->login);
+
+ foreach ($accessRaw as $access) {
+ $this->idsitesByAccess[$access['access']][] = $access['idsite'];
+ }
+ }
+ }
}
/**
@@ -241,11 +246,12 @@ class Access
*/
public function setSuperUserAccess($bool = true)
{
+ $this->hasSuperUserAccess = (bool) $bool;
+
if ($bool) {
- $this->reloadAccessSuperUser();
+ $this->makeSureLoginNameIsSet();
} else {
- $this->hasSuperUserAccess = false;
- $this->idsitesByAccess['superuser'] = array();
+ $this->resetSites();
}
}
@@ -288,6 +294,8 @@ class Access
*/
public function getSitesIdWithAtLeastViewAccess()
{
+ $this->loadSitesIfNeeded();
+
return array_unique(array_merge(
$this->idsitesByAccess['view'],
$this->idsitesByAccess['admin'],
@@ -303,6 +311,8 @@ class Access
*/
public function getSitesIdWithAdminAccess()
{
+ $this->loadSitesIfNeeded();
+
return array_unique(array_merge(
$this->idsitesByAccess['admin'],
$this->idsitesByAccess['superuser'])
@@ -318,6 +328,8 @@ class Access
*/
public function getSitesIdWithViewAccess()
{
+ $this->loadSitesIfNeeded();
+
return $this->idsitesByAccess['view'];
}
diff --git a/core/Settings/Setting.php b/core/Settings/Setting.php
index 32883876715e9aaf8c44e4e87fc4bbfea1aad1a3..4b1b36e5a14f8dbcf49f20aff460b0261f449753 100644
--- a/core/Settings/Setting.php
+++ b/core/Settings/Setting.php
@@ -147,8 +147,6 @@ abstract class Setting
protected $key;
protected $name;
- protected $writableByCurrentUser = false;
- protected $readableByCurrentUser = false;
/**
* @var StorageInterface
@@ -188,7 +186,7 @@ abstract class Setting
*/
public function isWritableByCurrentUser()
{
- return $this->writableByCurrentUser;
+ return false;
}
/**
@@ -198,7 +196,7 @@ abstract class Setting
*/
public function isReadableByCurrentUser()
{
- return $this->readableByCurrentUser;
+ return false;
}
/**
diff --git a/core/Settings/SystemSetting.php b/core/Settings/SystemSetting.php
index 935aec13883ba96a346c4917495f5c307d46954e..94f7416c19c70e7d2e0dd94f2d380af79c2b18fb 100644
--- a/core/Settings/SystemSetting.php
+++ b/core/Settings/SystemSetting.php
@@ -31,6 +31,11 @@ class SystemSetting extends Setting
*/
public $readableByCurrentUser = false;
+ /**
+ * @var bool
+ */
+ private $writableByCurrentUser = false;
+
/**
* Constructor.
*
@@ -45,6 +50,27 @@ class SystemSetting extends Setting
$this->readableByCurrentUser = $this->writableByCurrentUser;
}
+ /**
+ * Returns `true` if this setting is writable for the current user, `false` if otherwise. In case it returns
+ * writable for the current user it will be visible in the Plugin settings UI.
+ *
+ * @return bool
+ */
+ public function isWritableByCurrentUser()
+ {
+ return $this->writableByCurrentUser;
+ }
+
+ /**
+ * Returns `true` if this setting can be displayed for the current user, `false` if otherwise.
+ *
+ * @return bool
+ */
+ public function isReadableByCurrentUser()
+ {
+ return $this->readableByCurrentUser;
+ }
+
/**
* Returns the display order. System settings are displayed before user settings.
*
diff --git a/core/Settings/UserSetting.php b/core/Settings/UserSetting.php
index 80cf66ebf21a63a7053149b1a1a58e8a0851e563..60f63e3037b8b33a2acb9e1ce8be7518d3786a78 100644
--- a/core/Settings/UserSetting.php
+++ b/core/Settings/UserSetting.php
@@ -22,6 +22,12 @@ class UserSetting extends Setting
{
private $userLogin = null;
+ /**
+ * Null while not initialized, bool otherwise.
+ * @var null|bool
+ */
+ private $hasReadAndWritePermission = null;
+
/**
* Constructor.
*
@@ -34,9 +40,32 @@ class UserSetting extends Setting
parent::__construct($name, $title);
$this->setUserLogin($userLogin);
+ }
+
+ /**
+ * Returns `true` if this setting can be displayed for the current user, `false` if otherwise.
+ *
+ * @return bool
+ */
+ public function isReadableByCurrentUser()
+ {
+ return $this->isWritableByCurrentUser();
+ }
+
+ /**
+ * Returns `true` if this setting can be displayed for the current user, `false` if otherwise.
+ *
+ * @return bool
+ */
+ public function isWritableByCurrentUser()
+ {
+ if (isset($this->hasReadAndWritePermission)) {
+ return $this->hasReadAndWritePermission;
+ }
+
+ $this->hasReadAndWritePermission = Piwik::isUserHasSomeViewAccess();
- $this->writableByCurrentUser = Piwik::isUserHasSomeViewAccess();
- $this->readableByCurrentUser = Piwik::isUserHasSomeViewAccess();
+ return $this->hasReadAndWritePermission;
}
/**
diff --git a/plugins/SitesManager/API.php b/plugins/SitesManager/API.php
index 8457778ab5b984b703e1341d62b29bacca0264fb..94b53a2fa0d71168ce24df6fa0b2e11fae9d45e1 100644
--- a/plugins/SitesManager/API.php
+++ b/plugins/SitesManager/API.php
@@ -240,7 +240,7 @@ class API extends \Piwik\Plugin\API
{
Piwik::checkUserHasSuperUserAccess();
try {
- return API::getInstance()->getSitesId();
+ return $this->getSitesId();
} catch (Exception $e) {
// can be called before Piwik tables are created so return empty
return array();
@@ -287,7 +287,7 @@ class API extends \Piwik\Plugin\API
if ($fetchAliasUrls) {
foreach ($sites as &$site) {
- $site['alias_urls'] = API::getInstance()->getSiteUrlsFromId($site['idsite']);
+ $site['alias_urls'] = $this->getSiteUrlsFromId($site['idsite']);
}
}
@@ -593,7 +593,7 @@ class API extends \Piwik\Plugin\API
{
Piwik::checkUserHasSuperUserAccess();
- $idSites = API::getInstance()->getSitesId();
+ $idSites = $this->getSitesId();
if (!in_array($idSite, $idSites)) {
throw new Exception("website id = $idSite not found");
}
@@ -1048,7 +1048,8 @@ class API extends \Piwik\Plugin\API
{
Piwik::checkUserHasAdminAccess($idSite);
- $idSites = API::getInstance()->getSitesId();
+ $idSites = $this->getSitesId();
+
if (!in_array($idSite, $idSites)) {
throw new Exception("website id = $idSite not found");
}
diff --git a/plugins/UsersManager/API.php b/plugins/UsersManager/API.php
index ccb4dac126e6c6b4ad54a00f91ec5869ab7d3b14..69986e57eb1c0a2a5ec26475596defe433755d57 100644
--- a/plugins/UsersManager/API.php
+++ b/plugins/UsersManager/API.php
@@ -107,7 +107,13 @@ class API extends \Piwik\Plugin\API
return $optionValue;
}
- return $this->getDefaultUserPreference($preferenceName, $userLogin);
+ $defaultPreference = $this->getDefaultUserPreference($preferenceName, $userLogin);
+
+ if ($defaultPreference !== false && $preferenceName === self::PREFERENCE_DEFAULT_REPORT) {
+ $this->setUserPreference($userLogin, $preferenceName, $defaultPreference);
+ }
+
+ return $defaultPreference;
}
/**
diff --git a/tests/PHPUnit/Integration/AccessTest.php b/tests/PHPUnit/Integration/AccessTest.php
index cb2d201c846ab074de2b99b1eeeaeb9b623ba807..a105b7287455610774bb2aa57bcae7b922c1e440 100644
--- a/tests/PHPUnit/Integration/AccessTest.php
+++ b/tests/PHPUnit/Integration/AccessTest.php
@@ -11,6 +11,8 @@ namespace Piwik\Tests\Integration;
use Exception;
use Piwik\Access;
use Piwik\AuthResult;
+use Piwik\Db;
+use Piwik\NoAccessException;
use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
/**
@@ -121,16 +123,49 @@ class AccessTest extends IntegrationTestCase
$access->checkUserHasSomeAdminAccess();
}
+ /**
+ * @expectedException \Piwik\NoAccessException
+ */
+ public function test_CheckUserHasSomeAdminAccessWithSomeAccessFails_IfUserHasPermissionsToSitesButIsNotAuthenticated()
+ {
+ $mock = $this->createAccessMockWithAccessToSitesButUnauthenticated(array(2, 9));
+ $mock->checkUserHasSomeAdminAccess();
+ }
+
+ /**
+ * @expectedException \Piwik\NoAccessException
+ */
+ public function test_checkUserHasAdminAccessFails_IfUserHasPermissionsToSitesButIsNotAuthenticated()
+ {
+ $mock = $this->createAccessMockWithAccessToSitesButUnauthenticated(array(2, 9));
+ $mock->checkUserHasAdminAccess('2');
+ }
+
+ /**
+ * @expectedException \Piwik\NoAccessException
+ */
+ public function test_checkUserHasSomeViewAccessFails_IfUserHasPermissionsToSitesButIsNotAuthenticated()
+ {
+ $mock = $this->createAccessMockWithAccessToSitesButUnauthenticated(array(2, 9));
+ $mock->checkUserHasSomeViewAccess();
+ }
+
+ /**
+ * @expectedException \Piwik\NoAccessException
+ */
+ public function test_checkUserHasViewAccessFails_IfUserHasPermissionsToSitesButIsNotAuthenticated()
+ {
+ $mock = $this->createAccessMockWithAccessToSitesButUnauthenticated(array(2, 9));
+ $mock->checkUserHasViewAccess('2');
+ }
+
public function testCheckUserHasSomeAdminAccessWithSomeAccess()
{
- $mock = $this->getMock(
- 'Piwik\Access',
- array('getSitesIdWithAdminAccess')
- );
+ $mock = $this->createAccessMockWithAuthenticatedUser(array('getRawSitesWithSomeViewAccess'));
$mock->expects($this->once())
- ->method('getSitesIdWithAdminAccess')
- ->will($this->returnValue(array(2, 9)));
+ ->method('getRawSitesWithSomeViewAccess')
+ ->will($this->returnValue($this->buildAdminAccessForSiteIds(array(2, 9))));
$mock->checkUserHasSomeAdminAccess();
}
@@ -153,14 +188,11 @@ class AccessTest extends IntegrationTestCase
public function testCheckUserHasSomeViewAccessWithSomeAccess()
{
- $mock = $this->getMock(
- 'Piwik\Access',
- array('getSitesIdWithAtLeastViewAccess')
- );
+ $mock = $this->createAccessMockWithAuthenticatedUser(array('getRawSitesWithSomeViewAccess'));
$mock->expects($this->once())
- ->method('getSitesIdWithAtLeastViewAccess')
- ->will($this->returnValue(array(1, 2, 3, 4)));
+ ->method('getRawSitesWithSomeViewAccess')
+ ->will($this->returnValue($this->buildViewAccessForSiteIds(array(1, 2, 3, 4))));
$mock->checkUserHasSomeViewAccess();
}
@@ -183,28 +215,24 @@ class AccessTest extends IntegrationTestCase
public function testCheckUserHasViewAccessWithSomeAccessSuccessIdSitesAsString()
{
- $mock = $this->getMock(
- 'Piwik\Access',
- array('getSitesIdWithAtLeastViewAccess')
- );
+ /** @var Access $mock */
+ $mock = $this->createAccessMockWithAuthenticatedUser(array('getRawSitesWithSomeViewAccess'));
$mock->expects($this->once())
- ->method('getSitesIdWithAtLeastViewAccess')
- ->will($this->returnValue(array(1, 2, 3, 4)));
+ ->method('getRawSitesWithSomeViewAccess')
+ ->will($this->returnValue($this->buildViewAccessForSiteIds(array(1, 2, 3, 4))));
$mock->checkUserHasViewAccess('1,3');
}
public function testCheckUserHasViewAccessWithSomeAccessSuccessAllSites()
{
- $mock = $this->getMock(
- 'Piwik\Access',
- array('getSitesIdWithAtLeastViewAccess')
- );
+ /** @var Access $mock */
+ $mock = $this->createAccessMockWithAuthenticatedUser(array('getRawSitesWithSomeViewAccess'));
$mock->expects($this->any())
- ->method('getSitesIdWithAtLeastViewAccess')
- ->will($this->returnValue(array(1, 2, 3, 4)));
+ ->method('getRawSitesWithSomeViewAccess')
+ ->will($this->returnValue($this->buildViewAccessForSiteIds(array(1, 2, 3, 4))));
$mock->checkUserHasViewAccess('all');
}
@@ -306,8 +334,7 @@ class AccessTest extends IntegrationTestCase
public function testReloadAccessWithMockedAuthValid()
{
- $mock = $this->getMock('Piwik\\Auth', array('authenticate', 'getName', 'getTokenAuthSecret', 'getLogin', 'setTokenAuth', 'setLogin',
- 'setPassword', 'setPasswordHash'));
+ $mock = $this->createPiwikAuthMockInstance();
$mock->expects($this->once())
->method('authenticate')
->will($this->returnValue(new AuthResult(AuthResult::SUCCESS, 'login', 'token')));
@@ -319,6 +346,43 @@ class AccessTest extends IntegrationTestCase
$this->assertFalse($access->hasSuperUserAccess());
}
+ public function test_reloadAccess_loadSitesIfNeeded_doesActuallyResetAllSiteIdsAndRequestThemAgain()
+ {
+ /** @var Access $mock */
+ $mock = $this->createAccessMockWithAuthenticatedUser(array('getRawSitesWithSomeViewAccess'));
+
+ $mock->expects($this->at(0))
+ ->method('getRawSitesWithSomeViewAccess')
+ ->will($this->returnValue($this->buildAdminAccessForSiteIds(array(1,2,3,4))));
+
+ $mock->expects($this->at(1))
+ ->method('getRawSitesWithSomeViewAccess')
+ ->will($this->returnValue($this->buildAdminAccessForSiteIds(array(1))));
+
+ // should succeed as permission to 1,2,3,4
+ $mock->checkUserHasAdminAccess('1,3');
+
+ // should clear permissions
+ $mock->reloadAccess();
+
+ try {
+ // should fail as now only permission to site 1
+ $mock->checkUserHasAdminAccess('1,3');
+ $this->fail('An expected exception has not been triggered. Permissions were not resetted');
+ } catch (NoAccessException $e) {
+
+ }
+
+ $mock->checkUserHasAdminAccess('1'); // it should have access to site "1"
+
+ $mock->setSuperUserAccess(true);
+
+ $mock->reloadAccess();
+
+ // should now have permission as it is a superuser
+ $mock->checkUserHasAdminAccess('1,3');
+ }
+
public function test_doAsSuperUser_ChangesSuperUserAccessCorrectly()
{
Access::getInstance()->setSuperUserAccess(false);
@@ -372,4 +436,65 @@ class AccessTest extends IntegrationTestCase
AccessTest::assertTrue($access->hasSuperUserAccess());
});
}
+
+ private function buildAdminAccessForSiteIds($siteIds)
+ {
+ $access = array();
+
+ foreach ($siteIds as $siteId) {
+ $access[] = array('access' => 'admin', 'idsite' => $siteId);
+ }
+
+ return $access;
+ }
+
+ private function buildViewAccessForSiteIds($siteIds)
+ {
+ $access = array();
+
+ foreach ($siteIds as $siteId) {
+ $access[] = array('access' => 'admin', 'idsite' => $siteId);
+ }
+
+ return $access;
+ }
+
+ private function createPiwikAuthMockInstance()
+ {
+ return $this->getMock('Piwik\\Auth', array('authenticate', 'getName', 'getTokenAuthSecret', 'getLogin', 'setTokenAuth', 'setLogin',
+ 'setPassword', 'setPasswordHash'));
+ }
+
+ private function createAccessMockWithAccessToSitesButUnauthenticated($idSites)
+ {
+ $mock = $this->getMock('Piwik\Access', array('getRawSitesWithSomeViewAccess', 'loadSitesIfNeeded'));
+
+ // this method will be actually never called as it is unauthenticated. The tests are supposed to fail if it
+ // suddenly does get called as we should not query for sites if it is not authenticated.
+ $mock->expects($this->any())
+ ->method('getRawSitesWithSomeViewAccess')
+ ->will($this->returnValue($this->buildAdminAccessForSiteIds($idSites)));
+
+ return $mock;
+ }
+
+ private function createAccessMockWithAuthenticatedUser($methodsToMock = array())
+ {
+ $methods = array('authenticate');
+
+ foreach ($methodsToMock as $methodToMock) {
+ $methods[] = $methodToMock;
+ }
+
+ $authMock = $this->createPiwikAuthMockInstance();
+ $authMock->expects($this->atLeast(1))
+ ->method('authenticate')
+ ->will($this->returnValue(new AuthResult(AuthResult::SUCCESS, 'login', 'token')));
+
+ $mock = $this->getMock('Piwik\Access', $methods);
+ $mock->reloadAccess($authMock);
+
+ return $mock;
+ }
+
}