Skip to content

LDAP: fix null where IUser is expected in update groups background job #23799

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

Merged
blizzz merged 2 commits into from
Nov 2, 2020
Merged

LDAP: fix null where IUser is expected in update groups background job #23799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd44087
adds unit test for updategroups background job
blizzz Oct 30, 2020
3a51160
fix potentially passing null to events where IUser is expected
blizzz Oct 30, 2020
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
153 changes: 97 additions & 56 deletions apps/user_ldap/lib/Jobs/UpdateGroups.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,52 @@
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Group\Events\UserAddedEvent;
use OCP\Group\Events\UserRemovedEvent;
use OCP\IDBConnection;
use OCP\IGroupManager;
use OCP\ILogger;
use OCP\IUser;
use OCP\IUserManager;
use Psr\Log\LoggerInterface;

class UpdateGroups extends TimedJob {
private $groupsFromDB;

/** @var Group_Proxy */
private $groupBackend;

public function __construct(Group_Proxy $groupBackend) {
/** @var IEventDispatcher */
private $dispatcher;
/** @var IGroupManager */
private $groupManager;
/** @var IUserManager */
private $userManager;
/** @var LoggerInterface */
private $logger;
/** @var IDBConnection */
private $dbc;

public function __construct(
Group_Proxy $groupBackend,
IEventDispatcher $dispatcher,
IGroupManager $groupManager,
IUserManager $userManager,
LoggerInterface $logger,
IDBConnection $dbc
) {
$this->interval = $this->getRefreshInterval();
$this->groupBackend = $groupBackend;
$this->dispatcher = $dispatcher;
$this->groupManager = $groupManager;
$this->userManager = $userManager;
$this->logger = $logger;
$this->dbc = $dbc;
}

/**
* @return int
*/
private function getRefreshInterval() {
//defaults to every hour
return \OC::$server->getConfig()->getAppValue('user_ldap', 'bgjRefreshInterval', 3600);
}

/**
Expand Down Expand Up @@ -79,28 +114,35 @@ public function updateGroups() {
}

/**
* @return int
* @return array
*/
private function getRefreshInterval() {
//defaults to every hour
return \OC::$server->getConfig()->getAppValue('user_ldap', 'bgjRefreshInterval', 3600);
private function getKnownGroups() {
if (is_array($this->groupsFromDB)) {
$this->groupsFromDB;
}
$qb = $this->dbc->getQueryBuilder();
$qb->select(['owncloudname', 'owncloudusers'])
->from('ldap_group_members');
$result = $qb->execute()->fetchAll();

$this->groupsFromDB = [];
Copy link
Member

@ChristophWurst ChristophWurst Nov 2, 2020

Choose a reason for hiding this comment

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

you could use array_map :)

Copy link
Member Author

@blizzz blizzz Nov 2, 2020

Choose a reason for hiding this comment

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

yes. doesn't make much difference, but perhaps lookin a bit fancier than good ol' foreach. iirc it is also more memory hungry than foreach.

Copy link
Member

@ChristophWurst ChristophWurst Nov 2, 2020

Choose a reason for hiding this comment

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

who cares about memory footprint if you can have fancy 😉

for me it's mostly about readability. If I see an array_map I immediately can guess what's happening.

Copy link
Member Author

@blizzz blizzz Nov 2, 2020

Choose a reason for hiding this comment

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

Actually can be a concern, depending how users and groups you throw on it. In this case the readability wouldn't really change as it is a one-line-transformation of the result array, either way.

foreach ($result as $dataset) {
$this->groupsFromDB[$dataset['owncloudname']] = $dataset;
}

return $this->groupsFromDB;
}

/**
* @param string[] $groups
*/
private function handleKnownGroups($groups) {
/** @var IEventDispatcher $dispatcher */
$dispatcher = \OC::$server->query(IEventDispatcher::class);
$groupManager = \OC::$server->getGroupManager();
$userManager = \OC::$server->getUserManager();
private function handleKnownGroups(array $groups) {
$this->logger->debug(
'bgJ "updateGroups" – Dealing with known Groups.',
['app' => 'user_ldap']
);
$qb = $this->dbc->getQueryBuilder();
$qb->update('ldap_group_members')
->set('owncloudusers', $qb->createParameter('members'))
->where($qb->expr()->eq('owncloudname', $qb->createParameter('groupId')));

\OCP\Util::writeLog('user_ldap', 'bgJ "updateGroups" – Dealing with known Groups.', ILogger::DEBUG);
$query = \OC_DB::prepare('
UPDATE `*PREFIX*ldap_group_members`
SET `owncloudusers` = ?
WHERE `owncloudname` = ?
');
if (!is_array($this->groupsFromDB)) {
$this->getKnownGroups();
}
Expand All @@ -109,30 +151,49 @@ private function handleKnownGroups($groups) {
$actualUsers = $this->groupBackend->usersInGroup($group);
$hasChanged = false;

$groupObject = $groupManager->get($group);
$groupObject = $this->groupManager->get($group);
foreach (array_diff($knownUsers, $actualUsers) as $removedUser) {
$userObject = $userManager->get($removedUser);
$dispatcher->dispatchTyped(new UserRemovedEvent($groupObject, $userObject));
\OCP\Util::writeLog('user_ldap',
'bgJ "updateGroups" – "'.$removedUser.'" removed from "'.$group.'".',
ILogger::INFO);
$userObject = $this->userManager->get($removedUser);
if ($userObject instanceof IUser) {
$this->dispatcher->dispatchTyped(new UserRemovedEvent($groupObject, $userObject));
}
$this->logger->info(
'bgJ "updateGroups" – {user} removed from {group}',
[
'app' => 'user_ldap',
'user' => $removedUser,
'group' => $group
]
);
$hasChanged = true;
}
foreach (array_diff($actualUsers, $knownUsers) as $addedUser) {
$userObject = $userManager->get($addedUser);
$dispatcher->dispatchTyped(new UserAddedEvent($groupObject, $userObject));
\OCP\Util::writeLog('user_ldap',
'bgJ "updateGroups" – "'.$addedUser.'" added to "'.$group.'".',
ILogger::INFO);
$userObject = $this->userManager->get($addedUser);
if ($userObject instanceof IUser) {
$this->dispatcher->dispatchTyped(new UserAddedEvent($groupObject, $userObject));
}
$this->logger->info(
'bgJ "updateGroups" – {user} added to {group}',
[
'app' => 'user_ldap',
'user' => $addedUser,
'group' => $group
]
);
$hasChanged = true;
}
if ($hasChanged) {
$query->execute([serialize($actualUsers), $group]);
$qb->setParameters([
'members' => serialize($actualUsers),
'groupId' => $group
]);
$qb->execute();
}
}
\OCP\Util::writeLog('user_ldap',
$this->logger->debug(
'bgJ "updateGroups" – FINISHED dealing with known Groups.',
ILogger::DEBUG);
['app' => 'user_ldap']
);
}

/**
Expand All @@ -147,7 +208,7 @@ private function handleCreatedGroups($createdGroups) {
');
foreach ($createdGroups as $createdGroup) {
\OCP\Util::writeLog('user_ldap',
'bgJ "updateGroups" – new group "'.$createdGroup.'" found.',
'bgJ "updateGroups" – new group "' . $createdGroup . '" found.',
ILogger::INFO);
$users = serialize($this->groupBackend->usersInGroup($createdGroup));
$query->execute([$createdGroup, $users]);
Expand All @@ -169,32 +230,12 @@ private function handleRemovedGroups($removedGroups) {
');
foreach ($removedGroups as $removedGroup) {
\OCP\Util::writeLog('user_ldap',
'bgJ "updateGroups" – group "'.$removedGroup.'" was removed.',
'bgJ "updateGroups" – group "' . $removedGroup . '" was removed.',
ILogger::INFO);
$query->execute([$removedGroup]);
}
\OCP\Util::writeLog('user_ldap',
'bgJ "updateGroups" – FINISHED dealing with removed groups.',
ILogger::DEBUG);
}

/**
* @return array
*/
private function getKnownGroups() {
if (is_array($this->groupsFromDB)) {
$this->groupsFromDB;
}
$query = \OC_DB::prepare('
SELECT `owncloudname`, `owncloudusers`
FROM `*PREFIX*ldap_group_members`
');
$result = $query->execute()->fetchAll();
$this->groupsFromDB = [];
foreach ($result as $dataset) {
$this->groupsFromDB[$dataset['owncloudname']] = $dataset;
}

return $this->groupsFromDB;
}
}
Loading