Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dev 18809 make set user access atomical #22931

Draft
wants to merge 8 commits into
base: 5.2.x-dev
Choose a base branch
from
+134 −50
Draft

Dev 18809 make set user access atomical #22931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0af40c0
Prevent bad data
caddoo Dec 23, 2024
da89ada
Add new model method and change when current state fetched
caddoo Dec 23, 2024
027ddff
Add additional check for admin access before making modifications
caddoo Jan 9, 2025
839f04e
Fix picking correct access
caddoo Jan 14, 2025
73da9f7
Fix saving access when not array
caddoo Jan 14, 2025
643941d
Support fetching multiple records from access table
caddoo Jan 14, 2025
0dd10a6
Fix styling issue
caddoo Jan 14, 2025
f068719
Be consistent with naming
caddoo Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 72 additions & 50 deletions plugins/UsersManager/API.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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(),
];
}

Expand All @@ -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(),
];
}

Expand Down Expand Up @@ -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');

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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']);
}
Expand Down Expand Up @@ -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 = [];
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.");
}
}

Expand All @@ -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']);
Expand Down Expand Up @@ -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
);

/**
Expand Down Expand Up @@ -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.
Expand All @@ -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)
{
Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -1011,18 +1011,18 @@ 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);
}

$this->model->deleteUser($userLogin);

$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();

Expand Down Expand Up @@ -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
Expand All @@ -1112,6 +1112,15 @@ public function setUserAccess($userLogin, $access, $idSites, $passwordConfirmati

$idSites = $this->getIdSitesCheckAdminAccess($idSites);

// Get current permissions per ID site
$idSitesAndAccess = [];

foreach ($idSites as $siteId) {
if ($foundAccess = $this->model->getAccessForUserForSite($userLogin, $siteId)) {
$idSitesAndAccess[$siteId] = $this->getRoleAndCapabilitiesFromAccess($foundAccess)[0][0];
}
}

// check password confirmation only when using session auth and setting view access for anonymous user
if ($userLogin === 'anonymous' && Request::fromRequest()->getBoolParameter('force_api_session', false) && $access === 'view') {
$this->confirmCurrentUserPassword($passwordConfirmation);
Expand Down Expand Up @@ -1143,34 +1152,47 @@ 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);
// Re-verify admin access for current user is still applicable.
$this->getIdSitesCheckAdminAccess($idSites);

if ($access === 'noaccess') {
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);
if (empty($idSitesAndAccess)) {
$this->model->addUserAccess($userLogin, $role, $idSites);
}

foreach ($idSitesAndAccess as $idSite => $previousAccess) {
$success = $this->model->updateUserAccessConditionally($userLogin, $idSite, $role, $previousAccess);
if ($success === false) {
throw new Exception('Concurrency problem');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess at this point we would need to remove all capabilities that were set for a specific site, as deleteUserAccess is no longer called upfront. If there are new capabilities that should be set, that will happen below.

Another possibility is to iterate over each site, check which capabilities are already set and which should be set. And based on a diff remove or add capabilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgiehl can you elaborate why need to remove all capabilities here, why would new capabilities have to be set if the exception is thrown?

The flows are either:

  • Update role fails - capabilities don't get touched.
  • Update role is successful - capabilities get updated.

}
}

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 = [];
Expand Down Expand Up @@ -1447,7 +1469,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)) {
Expand Down Expand Up @@ -1655,7 +1677,7 @@ public function generateInviteLink($userLogin, $expiryInDays = 7, $passwordConfi
return SettingsPiwik::getPiwikUrl() . 'index.php?' . Url::getQueryStringFromParameters([
'module' => Piwik::getLoginPluginName(),
'action' => 'acceptInvitation',
'token' => $token,
'token' => $token,
]);
}
}
62 changes: 62 additions & 0 deletions plugins/UsersManager/Model.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,39 @@ 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;
}

$accessList = [];

foreach ($accessResults as $accessResult) {
$accessList[] = $accessResult['access'];
}

return $accessList;
}

public function getUsersAccessFromSite($idSite)
{
$db = $this->getDb();
Expand Down Expand Up @@ -722,6 +755,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 ($db->fetchOne($selectSql, [$userLogin, $idSite, $newAccess]) === 0) {
return false;
}

return true;
}

public function deleteUser($userLogin): void
{
$this->deleteUserOnly($userLogin);
Expand Down
Loading