From fd440875303bc04f72abea61459f90e9523399bb Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 30 Oct 2020 15:31:24 +0100 Subject: [PATCH 1/2] adds unit test for updategroups background job Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Jobs/UpdateGroups.php | 146 +++++++++----- .../user_ldap/tests/Jobs/UpdateGroupsTest.php | 189 ++++++++++++++++++ 2 files changed, 280 insertions(+), 55 deletions(-) create mode 100644 apps/user_ldap/tests/Jobs/UpdateGroupsTest.php diff --git a/apps/user_ldap/lib/Jobs/UpdateGroups.php b/apps/user_ldap/lib/Jobs/UpdateGroups.php index 34dddb3d28..7a4ce93db7 100644 --- a/apps/user_ldap/lib/Jobs/UpdateGroups.php +++ b/apps/user_ldap/lib/Jobs/UpdateGroups.php @@ -38,17 +38,51 @@ use OCA\User_LDAP\Group_Proxy; 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\IUserManager; +use Psr\Log\LoggerInterface; class UpdateGroups extends TimedJob { private $groupsFromDB; /** @var Group_Proxy */ private $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) { + 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); } /** @@ -79,28 +113,35 @@ class UpdateGroups extends TimedJob { } /** - * @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 = []; + 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(); } @@ -109,30 +150,45 @@ class UpdateGroups extends TimedJob { $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); + $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); + $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'] + ); } /** @@ -147,7 +203,7 @@ class UpdateGroups extends TimedJob { '); 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]); @@ -169,7 +225,7 @@ class UpdateGroups extends TimedJob { '); 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]); } @@ -177,24 +233,4 @@ class UpdateGroups extends TimedJob { '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; - } } diff --git a/apps/user_ldap/tests/Jobs/UpdateGroupsTest.php b/apps/user_ldap/tests/Jobs/UpdateGroupsTest.php new file mode 100644 index 0000000000..06b6fbbda0 --- /dev/null +++ b/apps/user_ldap/tests/Jobs/UpdateGroupsTest.php @@ -0,0 +1,189 @@ + + * + * @author Arthur Schiwon + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\user_ldap\tests\Jobs; + +use Doctrine\DBAL\Driver\Statement; +use OCA\User_LDAP\Group_Proxy; +use OCA\User_LDAP\Jobs\UpdateGroups; +use OCP\DB\QueryBuilder\IExpressionBuilder; +use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\Group\Events\UserAddedEvent; +use OCP\Group\Events\UserRemovedEvent; +use OCP\IDBConnection; +use OCP\IGroup; +use OCP\IGroupManager; +use OCP\IUser; +use OCP\IUserManager; +use Psr\Log\LoggerInterface; +use Test\TestCase; + +class UpdateGroupsTest extends TestCase { + + /** @var Group_Proxy|\PHPUnit\Framework\MockObject\MockObject */ + protected $groupBackend; + /** @var IEventDispatcher|\PHPUnit\Framework\MockObject\MockObject */ + protected $dispatcher; + /** @var IGroupManager|\PHPUnit\Framework\MockObject\MockObject */ + protected $groupManager; + /** @var IUserManager|\PHPUnit\Framework\MockObject\MockObject */ + protected $userManager; + /** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */ + protected $logger; + /** @var IDBConnection|\PHPUnit\Framework\MockObject\MockObject */ + protected $dbc; + + /** @var UpdateGroups */ + protected $updateGroupsJob; + + public function setUp(): void { + $this->groupBackend = $this->createMock(Group_Proxy::class); + $this->dispatcher = $this->createMock(IEventDispatcher::class); + $this->groupManager = $this->createMock(IGroupManager::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->dbc = $this->createMock(IDBConnection::class); + + $this->updateGroupsJob = new UpdateGroups( + $this->groupBackend, + $this->dispatcher, + $this->groupManager, + $this->userManager, + $this->logger, + $this->dbc + ); + } + + public function testHandleKnownGroups() { + $knownGroups = [ + 'emptyGroup' => \serialize([]), + 'stableGroup' => \serialize(['userA', 'userC', 'userE']), + 'groupWithAdditions' => \serialize(['userA', 'userC', 'userE']), + 'groupWithRemovals' => \serialize(['userA', 'userC', 'userDeleted', 'userE']), + 'groupWithAdditionsAndRemovals' => \serialize(['userA', 'userC', 'userE']), + 'vanishedGroup' => \serialize(['userB', 'userDeleted']) + ]; + $knownGroupsDB = []; + foreach ($knownGroups as $gid => $members) { + $knownGroupsDB[] = [ + 'owncloudname' => $gid, + 'owncloudusers' => $members + ]; + } + $actualGroups = [ + 'emptyGroup' => [], + 'stableGroup' => ['userA', 'userC', 'userE'], + 'groupWithAdditions' => ['userA', 'userC', 'userE', 'userF'], + 'groupWithRemovals' => ['userA', 'userE'], + 'groupWithAdditionsAndRemovals' => ['userC', 'userE', 'userF'], + 'newGroup' => ['userB', 'userF'], + ]; + $groups = array_intersect(array_keys($knownGroups), array_keys($actualGroups)); + + /** @var IQueryBuilder|\PHPUnit\Framework\MockObject\MockObject $updateQb */ + $updateQb = $this->createMock(IQueryBuilder::class); + $updateQb->expects($this->once()) + ->method('update') + ->willReturn($updateQb); + $updateQb->expects($this->once()) + ->method('set') + ->willReturn($updateQb); + $updateQb->expects($this->once()) + ->method('where') + ->willReturn($updateQb); + // three groups need to be updated + $updateQb->expects($this->exactly(3)) + ->method('setParameters'); + $updateQb->expects($this->exactly(3)) + ->method('execute'); + $updateQb->expects($this->any()) + ->method('expr') + ->willReturn($this->createMock(IExpressionBuilder::class)); + + $stmt = $this->createMock(Statement::class); + $stmt->expects($this->once()) + ->method('fetchAll') + ->willReturn($knownGroupsDB); + + $selectQb = $this->createMock(IQueryBuilder::class); + $selectQb->expects($this->once()) + ->method('select') + ->willReturn($selectQb); + $selectQb->expects($this->once()) + ->method('from') + ->willReturn($selectQb); + $selectQb->expects($this->once()) + ->method('execute') + ->willReturn($stmt); + + $this->dbc->expects($this->any()) + ->method('getQueryBuilder') + ->willReturnOnConsecutiveCalls($updateQb, $selectQb); + + $this->groupBackend->expects($this->any()) + ->method('usersInGroup') + ->willReturnCallback(function ($groupID) use ($actualGroups) { + return isset($actualGroups[$groupID]) ? $actualGroups[$groupID] : []; + }); + + $this->groupManager->expects($this->any()) + ->method('get') + ->willReturnCallback(function (string $groupId): ?IGroup { + if ($groupId === 'vanishedGroup') { + return null; + } + return $this->createMock(IGroup::class); + }); + + $this->userManager->expects($this->exactly(5)) + ->method('get') + ->willReturnCallback(function (string $userId) { + if ($userId === 'userDeleted') { + // user already deleted + return null; + } + return $this->createMock(IUser::class); + }); + + $addedEvents = 0; + $removedEvents = 0; + $this->dispatcher->expects($this->exactly(4)) + ->method('dispatchTyped') + ->willReturnCallback(function ($event) use (&$addedEvents, &$removedEvents) { + if ($event instanceof UserRemovedEvent) { + $removedEvents++; + } elseif ($event instanceof UserAddedEvent) { + $addedEvents++; + } + }); + + $this->invokePrivate($this->updateGroupsJob, 'handleKnownGroups', [$groups]); + + $this->assertSame(2, $removedEvents); + $this->assertSame(2, $addedEvents); + // and no event for the user that is already deleted, the DB is nevertheless updated, hence 5 + } +} From 3a51160221fa6ff4dddd9b230668f060603633f1 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 30 Oct 2020 15:38:19 +0100 Subject: [PATCH 2/2] fix potentially passing null to events where IUser is expected Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Jobs/UpdateGroups.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/apps/user_ldap/lib/Jobs/UpdateGroups.php b/apps/user_ldap/lib/Jobs/UpdateGroups.php index 7a4ce93db7..0ecd43480c 100644 --- a/apps/user_ldap/lib/Jobs/UpdateGroups.php +++ b/apps/user_ldap/lib/Jobs/UpdateGroups.php @@ -41,6 +41,7 @@ use OCP\Group\Events\UserRemovedEvent; use OCP\IDBConnection; use OCP\IGroupManager; use OCP\ILogger; +use OCP\IUser; use OCP\IUserManager; use Psr\Log\LoggerInterface; @@ -153,7 +154,9 @@ class UpdateGroups extends TimedJob { $groupObject = $this->groupManager->get($group); foreach (array_diff($knownUsers, $actualUsers) as $removedUser) { $userObject = $this->userManager->get($removedUser); - $this->dispatcher->dispatchTyped(new UserRemovedEvent($groupObject, $userObject)); + if ($userObject instanceof IUser) { + $this->dispatcher->dispatchTyped(new UserRemovedEvent($groupObject, $userObject)); + } $this->logger->info( 'bgJ "updateGroups" – {user} removed from {group}', [ @@ -166,7 +169,9 @@ class UpdateGroups extends TimedJob { } foreach (array_diff($actualUsers, $knownUsers) as $addedUser) { $userObject = $this->userManager->get($addedUser); - $this->dispatcher->dispatchTyped(new UserAddedEvent($groupObject, $userObject)); + if ($userObject instanceof IUser) { + $this->dispatcher->dispatchTyped(new UserAddedEvent($groupObject, $userObject)); + } $this->logger->info( 'bgJ "updateGroups" – {user} added to {group}', [