[stable19] LDAP: fix null where IUser is expected in update groups background job

apps/user_ldap/lib/Jobs/UpdateGroups.php

@@ -46,7 +46,12 @@
 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 \OC\BackgroundJob\TimedJob {
 	private static $groupsFromDB;
@@ -85,7 +90,7 @@ public static function updateGroups() {
 	}
 
 	/**
-	 * @return int
+	 * @return array
 	 */
 	private static function getRefreshInterval() {
 		//defaults to every hour
@@ -113,30 +118,49 @@ private static function handleKnownGroups($groups) {
 			$actualUsers = self::getGroupBE()->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']
+		);
 	}
 
 	/**
@@ -151,7 +175,7 @@ private static 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(self::getGroupBE()->usersInGroup($createdGroup));
 			$query->execute([$createdGroup, $users]);
@@ -173,7 +197,7 @@ private static 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]);
 		}

apps/user_ldap/tests/Jobs/UpdateGroupsTest.php

@@ -0,0 +1,189 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2020 Arthur Schiwon <[email protected]>
+ *
+ * @author Arthur Schiwon <[email protected]>
+ *
+ * @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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+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
+	}
+}