Commit 769bad8f authored by Benni Mack's avatar Benni Mack Committed by Georg Ringer
Browse files

[!!!][BUGFIX] Do not cache a BE users' group list

When a user logs in, the "cache" field "usergroup_cached_list"
is updated in the be_users table.

This value is used so the groups do not need to be recalculated
each time for each user (e.g. in Workspaces when sending
notifications and in Log module).

However, this has serious flaws:
1) If an admin removes a previously active user from a BE
   group, the cached list still contains the old group, until
   the user logs in
2) If a user never logged in before, the list is empty

This change removes this conceptual bug, and builds this
into the GroupResolver to use direct (optimized) DB queries
for these use cases, making the DB field obsolete.

Resolves: #79565
Releases: master
Change-Id: I9d8fd04cc466906d7f8da43887788f48de81c642
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/67137

Tested-by: Christian Kuhn's avatarChristian Kuhn <lolli@schwarzbu.ch>
Tested-by: default avatarTYPO3com <noreply@typo3.com>
Tested-by: Georg Ringer's avatarGeorg Ringer <georg.ringer@gmail.com>
Reviewed-by: Christian Kuhn's avatarChristian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Georg Ringer's avatarGeorg Ringer <georg.ringer@gmail.com>
parent d4cbdc61
......@@ -742,12 +742,12 @@ class BackendUtility
* Returns an array with be_users records of all user NOT DELETED sorted by their username
* Keys in the array is the be_users uid
*
* @param string $fields Optional $fields list (default: username,usergroup,usergroup_cached_list,uid) can be used to set the selected fields
* @param string $fields Optional $fields list (default: username,usergroup,uid) can be used to set the selected fields
* @param string $where Optional $where clause (fx. "AND username='pete'") can be used to limit query
* @return array
* @internal should only be used from within TYPO3 Core, use a direct SQL query instead to ensure proper DBAL where statements
*/
public static function getUserNames($fields = 'username,usergroup,usergroup_cached_list,uid', $where = '')
public static function getUserNames($fields = 'username,usergroup,uid', $where = '')
{
return self::getRecordsSortedByTitle(
GeneralUtility::trimExplode(',', $fields, true),
......
......@@ -16,10 +16,10 @@
namespace TYPO3\CMS\Belog\Domain\Repository;
use TYPO3\CMS\Backend\Tree\View\PageTreeView;
use TYPO3\CMS\Backend\Utility\BackendUtility;
use TYPO3\CMS\Belog\Domain\Model\Constraint;
use TYPO3\CMS\Belog\Domain\Model\LogEntry;
use TYPO3\CMS\Belog\Domain\Model\Workspace;
use TYPO3\CMS\Core\Authentication\GroupResolver;
use TYPO3\CMS\Core\Database\ConnectionPool;
use TYPO3\CMS\Core\Type\Bitmask\Permission;
use TYPO3\CMS\Core\Utility\GeneralUtility;
......@@ -33,19 +33,11 @@ use TYPO3\CMS\Extbase\Persistence\Repository;
*/
class LogEntryRepository extends Repository
{
/**
* Backend users, with UID as key
*
* @var array
*/
protected $beUserList = [];
/**
* Initialize some local variables to be used during creation of objects
*/
public function initializeObject()
{
$this->beUserList = $this->getBackendUsers();
/** @var \TYPO3\CMS\Extbase\Persistence\Generic\QuerySettingsInterface $defaultQuerySettings */
$defaultQuerySettings = $this->objectManager->get(QuerySettingsInterface::class);
$defaultQuerySettings->setRespectStoragePage(false);
......@@ -146,13 +138,11 @@ class LogEntryRepository extends Repository
// Constraint for a group
if (strpos($userOrGroup, 'gr-') === 0) {
$groupId = (int)substr($userOrGroup, 3);
$userIds = [];
foreach ($this->beUserList as $userId => $userData) {
if (GeneralUtility::inList($userData['usergroup_cached_list'], (string)$groupId)) {
$userIds[] = $userId;
}
}
$groupResolver = GeneralUtility::makeInstance(GroupResolver::class);
$userIds = $groupResolver->findAllUsersInGroups([$groupId], 'be_groups', 'be_users');
if (!empty($userIds)) {
$userIds = array_column($userIds, 'uid');
$userIds = array_map('intval', $userIds);
$queryConstraints[] = $query->in('userid', $userIds);
} else {
// If there are no group members -> use -1 as constraint to not find anything
......@@ -186,14 +176,4 @@ class LogEntryRepository extends Repository
->where(...$constraints)
->execute();
}
/**
* Get a list of all backend users that are not deleted
*
* @return array
*/
protected function getBackendUsers()
{
return BackendUtility::getUserNames();
}
}
......@@ -1274,11 +1274,10 @@ class BackendUserAuthentication extends AbstractUserAuthentication
}
// Populating the $this->userGroupsUID -array with the groups in the order in which they were LAST included.!!
$this->userGroupsUID = array_reverse(array_unique(array_reverse($this->userGroupsUID)));
// Finally this is the list of group_uid's in the order they are parsed (including subgroups!)
// and without duplicates (duplicates are presented with their last entrance in the list,
// which thus reflects the order of the TypoScript in TSconfig)
$this->setCachedList(implode(',', $this->userGroupsUID));
$this->userGroupsUID = array_reverse(array_unique(array_reverse($this->userGroupsUID)));
$this->prepareUserTsConfig();
......@@ -1395,27 +1394,6 @@ TCAdefaults.sys_note.email = ' . $this->user['email'];
}
}
/**
* Updates the field be_users.usergroup_cached_list if the groupList of the user
* has changed/is different from the current list.
* The field "usergroup_cached_list" contains the list of groups which the user is a member of.
* After authentication (where these functions are called...) one can depend on this list being
* a representation of the exact groups/subgroups which the BE_USER has membership with.
*
* @param string $cList The newly compiled group-list which must be compared with the current list in the user record and possibly stored if a difference is detected.
* @internal
*/
public function setCachedList($cList)
{
if ((string)$cList != (string)$this->user['usergroup_cached_list']) {
GeneralUtility::makeInstance(ConnectionPool::class)->getConnectionForTable('be_users')->update(
'be_users',
['usergroup_cached_list' => $cList],
['uid' => (int)$this->user['uid']]
);
}
}
/**
* Sets up all file storages for a user.
* Needs to be called AFTER the groups have been loaded.
......
......@@ -127,4 +127,107 @@ class GroupResolver
}
return $groups;
}
/**
* This works the other way around: Find all users that belong to some groups. Because groups are nested,
* we need to find all groups and subgroups first, because maybe a user is only part of a higher group,
* instead of a "All editors" group.
*
* @param int[] $groupIds a list of IDs of groups
* @param string $sourceTable e.g. be_groups or fe_groups
* @param string $userSourceTable e.g. be_users or fe_users
* @return array full user records
*/
public function findAllUsersInGroups(array $groupIds, string $sourceTable, string $userSourceTable): array
{
$this->sourceTable = $sourceTable;
// Ensure the given groups exist
$mainGroups = $this->fetchRowsFromDatabase($groupIds);
$groupIds = array_map('intval', array_column($mainGroups, 'uid'));
if (empty($groupIds)) {
return [];
}
$parentGroupIds = $this->fetchParentGroupsRecursive($groupIds, $groupIds);
$queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)->getQueryBuilderForTable($userSourceTable);
$queryBuilder
->select('*')
->from($userSourceTable);
$constraints = [];
foreach ($groupIds as $groupUid) {
$constraints[] = $queryBuilder->expr()->inSet($this->sourceField, (string)$groupUid);
}
foreach ($parentGroupIds as $groupUid) {
$constraints[] = $queryBuilder->expr()->inSet($this->sourceField, (string)$groupUid);
}
$users = $queryBuilder
->where(
$queryBuilder->expr()->orX(...$constraints)
)
->execute()
->fetchAll();
return !empty($users) ? $users : [];
}
/**
* Load a list of group uids, and take into account if groups have been loaded before as part of recursive detection.
*
* @param int[] $groupIds a list of groups to find THEIR ancestors
* @param array $processedGroupIds helper function to avoid recursive detection
* @return array a list of parent groups and thus, grand grand parent groups as well
*/
protected function fetchParentGroupsRecursive(array $groupIds, array $processedGroupIds = []): array
{
if (empty($groupIds)) {
return [];
}
$parentGroups = $this->fetchParentGroupsFromDatabase($groupIds);
$validParentGroupIds = [];
foreach ($parentGroups as $parentGroup) {
$parentGroupId = (int)$parentGroup['uid'];
// Record was already processed, continue to avoid adding this group again
if (in_array($parentGroupId, $processedGroupIds, true)) {
continue;
}
$processedGroupIds[] = $parentGroupId;
$validParentGroupIds[] = $parentGroupId;
}
$grandParentGroups = $this->fetchParentGroupsRecursive($validParentGroupIds, $processedGroupIds);
return array_merge($validParentGroupIds, $grandParentGroups);
}
/**
* Find all groups that have a FIND_IN_SET(subgroups, [$subgroupIds]) => the parent groups
* via one SQL query.
*
* @param array $subgroupIds
* @return array
*/
protected function fetchParentGroupsFromDatabase(array $subgroupIds): array
{
$queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)->getQueryBuilderForTable($this->sourceTable);
$queryBuilder
->select('*')
->from($this->sourceTable);
$constraints = [];
foreach ($subgroupIds as $subgroupId) {
$constraints[] = $queryBuilder->expr()->inSet($this->recursiveSourceField, (string)$subgroupId);
}
$result = $queryBuilder
->where(
$queryBuilder->expr()->orX(...$constraints)
)
->execute();
$groups = [];
while ($row = $result->fetch()) {
$groups[(int)$row['uid']] = $row;
}
return $groups;
}
}
.. include:: ../../Includes.txt
=================================================================
Breaking: #79565 - Removed "usergroup_cached_list" database field
=================================================================
See :issue:`79565`
Description
===========
The database field `be_users.usergroup_cached_list` has been
removed. It was populated by a list of all groups (including
subgroups) the user belongs to, and stored when a user logged in,
but was never updated when an admin added or removed a group
from the users' group list.
Impact
======
The mentioned database field is removed, any direct SQL queries
accessing or writing this field will result in a database error.
Affected Installations
======================
TYPO3 installations using or querying this database field
with third-party extensions.
Migration
=========
Use the GroupResolver class to fetch all groups of a user directly.
.. index:: Backend, PHP-API, NotScanned, ext:core
\ No newline at end of file
<?php
declare(strict_types=1);
/*
* This file is part of the TYPO3 CMS project.
*
* It is free software; you can redistribute it and/or modify it under
* the terms of the GNU General Public License, either version 2
* of the License, or any later version.
*
* For the full copyright and license information, please read the
* LICENSE.txt file that was distributed with this source code.
*
* The TYPO3 project - inspiring people to share!
*/
namespace TYPO3\CMS\Core\Tests\Functional\Authentication;
use TYPO3\CMS\Core\Authentication\GroupResolver;
use TYPO3\CMS\Core\Utility\GeneralUtility;
use TYPO3\TestingFramework\Core\Functional\FunctionalTestCase;
class GroupResolverTest extends FunctionalTestCase
{
public function setUp(): void
{
parent::setUp();
$this->importDataSet(__DIR__ . '/Fixtures/be_users.xml');
$this->importDataSet(__DIR__ . '/Fixtures/be_groups.xml');
}
public function findAllUsersOfGroupsHandlesRecursiveCallsDataProvider(): array
{
return [
'invalid group' => [
[238],
[]
],
'direct group with multiple users' => [
[1],
[2, 3]
],
'direct group with one users' => [
[4],
[3]
],
'direct and indirect subgroup with one users' => [
[2],
[3]
],
'subgroup with no direct reference' => [
[5],
[3]
],
'subgroup and direct with no direct reference' => [
[5, 2, 3],
[3]
],
'no group given' => [
[],
[]
],
];
}
/**
* @test
* @dataProvider findAllUsersOfGroupsHandlesRecursiveCallsDataProvider
* @param int[] $groupIds
* @param array $expectedUsers
*/
public function findAllUsersOfGroupsHandlesRecursiveCalls(array $groupIds, array $expectedUsers): void
{
$subject = GeneralUtility::makeInstance(GroupResolver::class);
$users = $subject->findAllUsersInGroups($groupIds, 'be_groups', 'be_users');
self::assertEquals($expectedUsers, array_map('intval', array_column($users, 'uid')));
}
}
......@@ -56,7 +56,6 @@ CREATE TABLE be_users (
workspace_perms tinyint(3) DEFAULT '1' NOT NULL,
TSconfig text,
lastlogin int(10) unsigned DEFAULT '0' NOT NULL,
usergroup_cached_list text,
workspace_id int(11) DEFAULT '0' NOT NULL,
category_perms text,
KEY username (username)
......
......@@ -17,8 +17,7 @@ namespace TYPO3\CMS\Workspaces\Service;
use TYPO3\CMS\Backend\Utility\BackendUtility;
use TYPO3\CMS\Core\Authentication\BackendUserAuthentication;
use TYPO3\CMS\Core\Database\Connection;
use TYPO3\CMS\Core\Database\ConnectionPool;
use TYPO3\CMS\Core\Authentication\GroupResolver;
use TYPO3\CMS\Core\Localization\LanguageService;
use TYPO3\CMS\Core\SingletonInterface;
use TYPO3\CMS\Core\Utility\GeneralUtility;
......@@ -62,16 +61,6 @@ class StagesService implements SingletonInterface
*/
protected $workspaceStageAllowedCache = [];
/**
* @var array
*/
protected $fetchGroupsCache = [];
/**
* @var array
*/
protected $userGroups = [];
/**
* Getter for current workspace id
*
......@@ -452,19 +441,6 @@ class StagesService implements SingletonInterface
return $recipientArray;
}
/**
* Gets backend user ids from a mixed list of backend users
* and backend users groups. This is used for notifying persons
* responsible for a particular stage or workspace.
*
* @param string $stageRespValue Responsible_person value from stage record
* @return string List of backend user ids
*/
public function getResponsibleUser($stageRespValue)
{
return implode(',', $this->resolveBackendUserIds($stageRespValue));
}
/**
* Resolves backend user ids from a mixed list of backend users
* and backend user groups (e.g. "be_users_1,be_groups_3,be_users_4,...")
......@@ -483,23 +459,17 @@ class StagesService implements SingletonInterface
// Current value is a uid of a be_user record
$backendUserIds[] = str_replace('be_users_', '', $element);
} elseif (strpos($element, 'be_groups_') === 0) {
$backendGroupIds[] = str_replace('be_groups_', '', $element);
$backendGroupIds[] = (int)str_replace('be_groups_', '', $element);
} elseif ((int)$element) {
$backendUserIds[] = (int)$element;
}
}
if (!empty($backendGroupIds)) {
$allBeUserArray = BackendUtility::getUserNames();
$backendGroupList = implode(',', $backendGroupIds);
$this->userGroups = [];
$backendGroups = $this->fetchGroups($backendGroupList);
foreach ($backendGroups as $backendGroup) {
foreach ($allBeUserArray as $backendUserId => $backendUser) {
if (GeneralUtility::inList($backendUser['usergroup_cached_list'], $backendGroup['uid'])) {
$backendUserIds[] = $backendUserId;
}
}
$groupResolver = GeneralUtility::makeInstance(GroupResolver::class);
$backendUsersInGroups = $groupResolver->findAllUsersInGroups($backendGroupIds, 'be_groups', 'be_users');
foreach ($backendUsersInGroups as $backendUsers) {
$backendUserIds[] = (int)$backendUsers['uid'];
}
}
......@@ -553,90 +523,6 @@ class StagesService implements SingletonInterface
return WorkspaceRecord::get($this->getWorkspaceId());
}
/**
* @param string $grList
* @param string $idList
* @return array
*/
private function fetchGroups($grList, $idList = '')
{
$cacheKey = md5($grList . $idList);
if (isset($this->fetchGroupsCache[$cacheKey])) {
return $this->fetchGroupsCache[$cacheKey];
}
if ($idList === '') {
// we're at the beginning of the recursion and therefore we need to reset the userGroups member
$this->userGroups = [];
}
$groupList = $this->fetchGroupsRecursive($grList);
$this->fetchGroupsCache[$cacheKey] = $groupList;
return $groupList;
}
/**
* @param array $groups
*/
private function fetchGroupsFromDB(array $groups)
{
// The userGroups array is filled
$queryBuilder = GeneralUtility::makeInstance(ConnectionPool::class)->getQueryBuilderForTable('be_groups');
$result = $queryBuilder
->select('*')
->from('be_groups')
->where(
$queryBuilder->expr()->in(
'uid',
$queryBuilder->createNamedParameter($groups, Connection::PARAM_INT_ARRAY)
)
)
->execute();
while ($row = $result->fetch()) {
$this->userGroups[$row['uid']] = $row;
}
}
/**
* Fetches particular groups recursively.
*
* @param string $grList
* @param string $idList
* @return array
*/
private function fetchGroupsRecursive($grList, $idList = '')
{
$requiredGroups = GeneralUtility::intExplode(',', $grList, true);
$existingGroups = array_keys($this->userGroups);
$missingGroups = array_diff($requiredGroups, $existingGroups);
if (!empty($missingGroups)) {
$this->fetchGroupsFromDB($missingGroups);
}
// Traversing records in the correct order
foreach ($requiredGroups as $uid) {
// traversing list
// Get row:
$row = $this->userGroups[$uid];
if (is_array($row) && !GeneralUtility::inList($idList, (string)$uid)) {
// Must be an array and $uid should not be in the idList, because then it is somewhere previously in the grouplist
// If the localconf.php option isset the user of the sub- sub- groups will also be used
if ($GLOBALS['TYPO3_CONF_VARS']['BE']['customStageShowRecipientRecursive'] == 1) {
// Include sub groups
if (trim($row['subgroup'])) {
// Make integer list
$theList = implode(',', GeneralUtility::intExplode(',', $row['subgroup']));
// Get the subgroups
$subGroups = $this->fetchGroups($theList, $idList . ',' . $uid);
// Merge the subgroups to the already existing userGroups array
$subUid = key($subGroups);
$this->userGroups[$subUid] = $subGroups[$subUid];
}
}
}
}
return $this->userGroups;
}
/**
* Gets a property of a workspaces stage.
*
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment