diff --git a/plugins/UsersManager/API.php b/plugins/UsersManager/API.php index 999604ff0bb..f9bd9d02987 100644 --- a/plugins/UsersManager/API.php +++ b/plugins/UsersManager/API.php @@ -129,7 +129,7 @@ public function __construct( * * StaticContainer::getContainer()->set('UsersManager_API', \Piwik\Plugins\MyCustomUsersManager\API::getInstance()); * - * @return \Piwik\Plugins\UsersManager\API + * @return API * @throws Exception */ public static function getInstance() @@ -162,10 +162,10 @@ public function getAvailableRoles() foreach ($this->roleProvider->getAllRoles() as $role) { $response[] = [ - 'id' => $role->getId(), - 'name' => $role->getName(), - 'description' => $role->getDescription(), - 'helpUrl' => $role->getHelpUrl(), + 'id' => $role->getId(), + 'name' => $role->getName(), + 'description' => $role->getDescription(), + 'helpUrl' => $role->getHelpUrl(), ]; } @@ -184,12 +184,12 @@ public function getAvailableCapabilities() foreach ($this->capabilityProvider->getAllCapabilities() as $capability) { $response[] = [ - 'id' => $capability->getId(), - 'name' => $capability->getName(), - 'description' => $capability->getDescription(), - 'helpUrl' => $capability->getHelpUrl(), - 'includedInRoles' => $capability->getIncludedInRoles(), - 'category' => $capability->getCategory(), + 'id' => $capability->getId(), + 'name' => $capability->getName(), + 'description' => $capability->getDescription(), + 'helpUrl' => $capability->getHelpUrl(), + 'includedInRoles' => $capability->getIncludedInRoles(), + 'category' => $capability->getCategory(), ]; } @@ -302,10 +302,10 @@ private function getPreferenceId($login, $preference) throw new Exception("Preference name cannot contain underscores."); } $names = [ - self::PREFERENCE_DEFAULT_REPORT, - self::PREFERENCE_DEFAULT_REPORT_DATE, - 'isLDAPUser', // used in loginldap - 'hideSegmentDefinitionChangeMessage',// used in JS + self::PREFERENCE_DEFAULT_REPORT, + self::PREFERENCE_DEFAULT_REPORT_DATE, + 'isLDAPUser', // used in loginldap + 'hideSegmentDefinitionChangeMessage',// used in JS ]; $customPreferences = StaticContainer::get('usersmanager.user_preference_names'); @@ -378,7 +378,7 @@ public function getUsersPlusRole($idSite, $limit = null, $offset = 0, $filter_se if (!Piwik::hasUserSuperUserAccess()) { $adminIdSites = Access::getInstance()->getSitesIdWithAdminAccess(); if (empty($adminIdSites)) { // sanity check - throw new \Exception("The current admin user does not have access to any sites."); + throw new Exception("The current admin user does not have access to any sites."); } $loginsToLimit = $this->model->getUsersWithAccessToSites($adminIdSites); @@ -407,8 +407,8 @@ public function getUsersPlusRole($idSite, $limit = null, $offset = 0, $filter_se $user['capabilities'] = []; } else { [ - $user['role'], - $user['capabilities'] + $user['role'], + $user['capabilities'] ] = $this->getRoleAndCapabilitiesFromAccess($user['access']); $user['role'] = empty($user['role']) ? 'noaccess' : reset($user['role']); } @@ -440,7 +440,7 @@ public function getUsers($userLogins = '') Piwik::checkUserHasSomeAdminAccess(); if (!is_string($userLogins)) { - throw new \Exception('Parameter userLogins needs to be a string containing a comma separated list of users'); + throw new Exception('Parameter userLogins needs to be a string containing a comma separated list of users'); } $logins = []; @@ -602,8 +602,8 @@ public function getSitesAccessFromUser($userLogin) $sites = $siteManagerModel->getAllSites(); foreach ($sites as $site) { $return[] = [ - 'site' => $site['idsite'], - 'access' => 'admin' + 'site' => $site['idsite'], + 'access' => 'admin' ]; } return $return; @@ -642,14 +642,14 @@ public function getSitesAccessForUser( $this->checkUserExists($userLogin); if (Piwik::hasTheUserSuperUserAccess($userLogin)) { - throw new \Exception("This method should not be used with superusers."); + throw new Exception("This method should not be used with superusers."); } $idSites = null; if (!Piwik::hasUserSuperUserAccess()) { $idSites = $this->access->getSitesIdWithAdminAccess(); if (empty($idSites)) { // sanity check - throw new \Exception("The current admin user does not have access to any sites."); + throw new Exception("The current admin user does not have access to any sites."); } } @@ -663,8 +663,8 @@ public function getSitesAccessForUser( ); foreach ($sites as &$siteAccess) { [ - $siteAccess['role'], - $siteAccess['capabilities'] + $siteAccess['role'], + $siteAccess['capabilities'] ] = $this->getRoleAndCapabilitiesFromAccess($siteAccess['access']); $siteAccess['role'] = empty($siteAccess['role']) ? 'noaccess' : reset($siteAccess['role']); unset($siteAccess['access']); @@ -755,16 +755,16 @@ public function addUser($userLogin, $password, $email, $_isPasswordHashed = fals if (!Piwik::hasUserSuperUserAccess()) { if (empty($initialIdSite)) { - throw new \Exception(Piwik::translate("UsersManager_AddUserNoInitialAccessError")); + throw new Exception(Piwik::translate("UsersManager_AddUserNoInitialAccessError")); } } $this->userRepository->create( - (string) $userLogin, - (string) $email, + (string)$userLogin, + (string)$email, $initialIdSite, - (string) $password, - (bool) $_isPasswordHashed + (string)$password, + (bool)$_isPasswordHashed ); /** @@ -795,13 +795,13 @@ public function inviteUser($userLogin, $email, $initialIdSite = null, $expiryInD } if (empty($initialIdSite)) { - throw new \Exception(Piwik::translate("UsersManager_AddUserNoInitialAccessError")); + throw new Exception(Piwik::translate("UsersManager_AddUserNoInitialAccessError")); } else { // check if the site exists new Site($initialIdSite); } - $this->userRepository->inviteUser((string) $userLogin, (string) $email, intval($initialIdSite), (int) $expiryInDays); + $this->userRepository->inviteUser((string)$userLogin, (string)$email, intval($initialIdSite), (int)$expiryInDays); /** * Triggered after a new user was invited. @@ -821,7 +821,7 @@ public function inviteUser($userLogin, $email, $initialIdSite = null, $expiryInD * access. * @param string $passwordConfirmation the current user's password. For security purposes, this value should be * sent as a POST parameter. - * @throws \Exception + * @throws Exception */ public function setSuperUserAccess($userLogin, $hasSuperUserAccess, $passwordConfirmation = null) { @@ -843,8 +843,8 @@ public function setSuperUserAccess($userLogin, $hasSuperUserAccess, $passwordCon if (!$hasSuperUserAccess && $this->isUserTheOnlyUserHavingSuperUserAccess($userLogin)) { $message = Piwik::translate("UsersManager_ExceptionRemoveSuperUserAccessOnlySuperUser", $userLogin) - . " " - . Piwik::translate("UsersManager_ExceptionYouMustGrantSuperUserAccessFirst"); + . " " + . Piwik::translate("UsersManager_ExceptionYouMustGrantSuperUserAccessFirst"); throw new Exception($message); } @@ -959,7 +959,7 @@ public function updateUser( // this will indirectly invalidate the invitation sent to the previous address $this->userRepository->reInviteUser( $userLogin, - (int) Config\GeneralConfig::getConfigValue('default_invite_user_token_expiry_days') + (int)Config\GeneralConfig::getConfigValue('default_invite_user_token_expiry_days') ); } elseif ($hasEmailChanged && $isEmailNotificationOnInConfig) { $this->sendEmailChangedEmail($userInfo, $email); @@ -1011,8 +1011,8 @@ public function deleteUser($userLogin, $passwordConfirmation = null) if ($this->isUserTheOnlyUserHavingSuperUserAccess($userLogin)) { $message = Piwik::translate("UsersManager_ExceptionDeleteOnlyUserWithSuperUserAccess", $userLogin) - . " " - . Piwik::translate("UsersManager_ExceptionYouMustGrantSuperUserAccessFirst"); + . " " + . Piwik::translate("UsersManager_ExceptionYouMustGrantSuperUserAccessFirst"); throw new Exception($message); } @@ -1020,9 +1020,9 @@ public function deleteUser($userLogin, $passwordConfirmation = null) $container = StaticContainer::getContainer(); $email = $container->make(UserDeletedEmail::class, [ - 'login' => Piwik::getCurrentUserLogin(), - 'emailAddress' => Piwik::getCurrentUserEmail(), - 'userLogin' => $userLogin + 'login' => Piwik::getCurrentUserLogin(), + 'emailAddress' => Piwik::getCurrentUserEmail(), + 'userLogin' => $userLogin ]); $email->safeSend(); @@ -1094,7 +1094,7 @@ public function getUserLoginFromUserEmail($userEmail) * * @param string $userLogin The user login * @param string|array $access Access to grant. Must have one of the following value : noaccess, view, write, admin. - * May also be an array to sent additional capabilities + * May also be an array to set additional capabilities * @param int|array $idSites The array of idSites on which to apply the access level for the user. * If the value is "all" then we apply the access level to all the websites ID for which the current authentificated user has an 'admin' access. * @param string $passwordConfirmation password confirmation. only required when setting view access for anonymous user through session auth @@ -1143,34 +1143,58 @@ public function setUserAccess($userLogin, $access, $idSites, $passwordConfirmati $ids = implode(', ', $this->roleProvider->getAllRoleIds()); throw new Exception(Piwik::translate('UsersManager_ExceptionMultipleRoleSet', $ids)); } + + $role = reset($roles); } else { // as only one access is set, we require it to be a role or "noaccess"... if ($access !== 'noaccess') { $this->roleProvider->checkValidRole($access); - $roles[] = $access; } + + $role = $access; } $this->checkUserExist($userLogin); $this->checkUsersHasNotSuperUserAccess($userLogin); - $this->model->deleteUserAccess($userLogin, $idSites); - if ($access === 'noaccess') { + // Get current roles per ID site + $idSitesAndAccess = []; + + foreach ($idSites as $siteId) { + if ($foundAccess = $this->model->getAccessForUserForSite($userLogin, $siteId)) { + $idSitesAndAccess[$siteId] = $this->getRoleAndCapabilitiesFromAccess($foundAccess)[0][0]; + } + } + + // Re-verify admin access for current user is still applicable. + $this->getIdSitesCheckAdminAccess($idSites); + + if ($role === 'noaccess') { + $this->model->deleteUserAccess($userLogin, $idSites); // if the access is noaccess then we don't save it as this is the default value // when no access are specified Piwik::postEvent('UsersManager.removeSiteAccess', [$userLogin, $idSites]); } else { - $role = array_shift($roles); - $this->model->addUserAccess($userLogin, $role, $idSites); - } + foreach ($idSites as $idSite) { + if (empty($idSitesAndAccess[$idSite])) { + $this->model->addUserAccess($userLogin, $role, [$idSite]); + } else { + $success = $this->model->updateUserAccessConditionally($userLogin, $idSite, $role, $idSitesAndAccess[$idSite]); + if ($success === false) { + throw new Exception('Concurrency problem'); + } + } + } + $this->model->deleteUserAccessExcluding($userLogin, $role, $idSites); - if (!empty($capabilities)) { - $this->addCapabilities($userLogin, $capabilities, $idSites); + if (!empty($capabilities)) { + $this->addCapabilities($userLogin, $capabilities, $idSites); + } } // Send notification to all super users if anonymous access is set for a site - if ($userLogin === 'anonymous' && $access === 'view') { + if ($userLogin === 'anonymous' && $role === 'view') { $container = StaticContainer::getContainer(); $siteNames = []; @@ -1447,7 +1471,7 @@ public function createAppSpecificTokenAuth( Piwik::postEvent('Login.authenticate.failed', [$userLogin]); } - throw new \Exception(Piwik::translate('UsersManager_CurrentPasswordNotCorrect')); + throw new Exception(Piwik::translate('UsersManager_CurrentPasswordNotCorrect')); } if (empty($expireDate) && !empty($expireHours) && is_numeric($expireHours)) { @@ -1655,7 +1679,7 @@ public function generateInviteLink($userLogin, $expiryInDays = 7, $passwordConfi return SettingsPiwik::getPiwikUrl() . 'index.php?' . Url::getQueryStringFromParameters([ 'module' => Piwik::getLoginPluginName(), 'action' => 'acceptInvitation', - 'token' => $token, + 'token' => $token, ]); } } diff --git a/plugins/UsersManager/Model.php b/plugins/UsersManager/Model.php index cf80af462a5..55e50136b7b 100644 --- a/plugins/UsersManager/Model.php +++ b/plugins/UsersManager/Model.php @@ -113,6 +113,33 @@ public function getUsersSitesFromAccess($access) return $return; } + /** + * @param string $userLogin + * @param int $idSite + * + * @return array|null The list of access for the given user login and id site + * + * @throws \Piwik\Tracker\Db\DbException + */ + public function getAccessForUserForSite(string $userLogin, int $idSite): ?array + { + $db = $this->getDb(); + + $accessResults = $db->fetchAll( + "SELECT access FROM " . Common::prefixTable("access") . " WHERE login = ? AND idsite = ?", + [ + $userLogin, + $idSite + ] + ); + + if ($accessResults === false) { + return null; + } + + return array_column($accessResults, 'access'); + } + public function getUsersAccessFromSite($idSite) { $db = $this->getDb(); @@ -722,6 +749,35 @@ public function addUserAccess($userLogin, $access, $idSites) } } + public function updateUserAccessConditionally(string $userLogin, int $idSite, string $newAccess, string $previousAccess): bool + { + $db = $this->getDb(); + + $updateSql = " + UPDATE " . Common::prefixTable("access") . " + SET access = ? + WHERE login = ? + AND idsite = ? + AND access = ? + "; + + $db->query($updateSql, [$newAccess, $userLogin, $idSite, $previousAccess]); + + // Performing query again because rows affected won't change when $newAccess == $previousAccess + $selectSql = " + SELECT COUNT(*) FROM " . Common::prefixTable("access") . " + WHERE login = ? + AND idsite = ? + AND access = ? + "; + + if ((int) $db->fetchOne($selectSql, [$userLogin, $idSite, $newAccess]) === 0) { + return false; + } + + return true; + } + public function deleteUser($userLogin): void { $this->deleteUserOnly($userLogin); @@ -773,6 +829,22 @@ public function deleteUserAccess($userLogin, $idSites = null) } } + public function deleteUserAccessExcluding(string $userLogin, string $accessToExcludeFromDeletion, array $idSites = null): void + { + $db = $this->getDb(); + + if (is_null($idSites)) { + $db->query("DELETE FROM " . Common::prefixTable("access") . " WHERE login = ? AND access != ?", [$userLogin, $accessToExcludeFromDeletion]); + } else { + foreach ($idSites as $idsite) { + $db->query( + "DELETE FROM " . Common::prefixTable("access") . " WHERE idsite = ? AND login = ? AND access != ?", + [$idsite, $userLogin, $accessToExcludeFromDeletion] + ); + } + } + } + private function getDb() { return Db::get(); diff --git a/plugins/UsersManager/tests/Integration/APITest.php b/plugins/UsersManager/tests/Integration/APITest.php index a478e9c9699..91e2a6a45ef 100644 --- a/plugins/UsersManager/tests/Integration/APITest.php +++ b/plugins/UsersManager/tests/Integration/APITest.php @@ -1174,6 +1174,32 @@ public function testSetUserAccessSetRoleAsArray() self::assertEquals([['site' => '1', 'access' => 'view']], $access); } + public function testSetUserAccessHandlesMultipleSitesWithMultiCurrentStateOfAccess() + { + $this->api->setUserAccess($this->login, [View::ID, TestCap1::ID], [1]); + $this->api->setUserAccess($this->login, [Admin::ID], [1, 2]); + + $access = $this->model->getSitesAccessFromUser($this->login); + self::assertEquals([['site' => '1', 'access' => 'admin'], ['site' => '2', 'access' => 'admin']], $access); + } + + public function testSetUserAccessHandlesMultipleSitesWithMultiCurrentStateOfAccessAndCapabilities() + { + $this->api->setUserAccess($this->login, [View::ID, TestCap1::ID, TestCap2::ID], [1, 3]); + $this->api->setUserAccess($this->login, [Write::ID, TestCap3::ID], [1, 2]); + + $access = $this->model->getSitesAccessFromUser($this->login); + self::assertEquals([ + ['site' => '1', 'access' => 'write'], + ['site' => '1', 'access' => 'test_cap3'], + ['site' => '2', 'access' => 'write'], + ['site' => '2', 'access' => 'test_cap3'], + ['site' => '3', 'access' => 'view'], + ['site' => '3', 'access' => 'test_cap1'], + ['site' => '3', 'access' => 'test_cap2'], + ], $access); + } + public function testAddCapabilitiesFailsWhenNotCapabilityIsGivenAsString() { $this->expectException(\Exception::class); diff --git a/plugins/UsersManager/tests/Integration/ModelTest.php b/plugins/UsersManager/tests/Integration/ModelTest.php index 29aa4ecba83..e5fd7dcb12b 100644 --- a/plugins/UsersManager/tests/Integration/ModelTest.php +++ b/plugins/UsersManager/tests/Integration/ModelTest.php @@ -384,4 +384,44 @@ public function testDeleteExpiredTokens() $this->assertEquals($id5, $tokens[1]['idusertokenauth']); $this->assertCount(2, $tokens); } + + public function testDeleteUserAccessExcludingWhenIdSitesNull(): void + { + $this->model->addUserAccess($this->login, 'role', [1, 2]); + $this->model->addUserAccess($this->login, 'capability_1', [1, 2]); + $this->model->addUserAccess($this->login, 'capability_2', [1, 2]); + $this->model->addUserAccess($this->login, 'capability_3', [1, 2]); + + $this->model->deleteUserAccessExcluding($this->login, 'role', null); + + $this->assertEquals( + ['role'], + $this->model->getAccessForUserForSite($this->login, 1) + ); + + $this->assertEquals( + ['role'], + $this->model->getAccessForUserForSite($this->login, 2) + ); + } + + public function testDeleteUserAccessExcludingWhenIdSitesNotNull(): void + { + $this->model->addUserAccess($this->login, 'role', [1, 2]); + $this->model->addUserAccess($this->login, 'capability_1', [1, 2]); + $this->model->addUserAccess($this->login, 'capability_2', [1, 2]); + $this->model->addUserAccess($this->login, 'capability_3', [1, 2]); + + $this->model->deleteUserAccessExcluding($this->login, 'role', [1]); + + $this->assertEquals( + ['role'], + $this->model->getAccessForUserForSite($this->login, 1) + ); + + $this->assertEquals( + ['role', 'capability_1', 'capability_2', 'capability_3'], + $this->model->getAccessForUserForSite($this->login, 2) + ); + } }