From 7c6e48d15b28ddaafee22e44f6c9579057da519f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Julius=20H=C3=A4rtl?= <jus@bitgrid.net>
Date: Tue, 3 Oct 2017 21:52:42 +0200
Subject: [PATCH] Notifications: Fix issues with user fetching
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Julius Härtl <jus@bitgrid.net>
---
 lib/Db/BoardMapper.php                       |  4 +-
 lib/Service/PermissionService.php            | 16 ++---
 tests/unit/Service/PermissionServiceTest.php | 72 +++++++++++++++++++-
 3 files changed, 82 insertions(+), 10 deletions(-)

diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php
index a1f2b2a92..55127cc58 100644
--- a/lib/Db/BoardMapper.php
+++ b/lib/Db/BoardMapper.php
@@ -23,6 +23,7 @@
 
 namespace OCA\Deck\Db;
 
+use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\IDBConnection;
 use OCP\IUserManager;
 use OCP\IGroupManager;
@@ -56,7 +57,8 @@ class BoardMapper extends DeckMapper implements IPermissionMapper {
 	 * @param $id
 	 * @param bool $withLabels
 	 * @param bool $withAcl
-	 * @return \OCP\AppFramework\Db\Entity if not found
+	 * @return \OCP\AppFramework\Db\Entity
+	 * @throws DoesNotExistException
 	 */
 	public function find($id, $withLabels = false, $withAcl = false) {
 		$sql = 'SELECT id, title, owner, color, archived, deleted_at FROM `*PREFIX*deck_boards` ' .
diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php
index cf5631a51..99f918506 100644
--- a/lib/Service/PermissionService.php
+++ b/lib/Service/PermissionService.php
@@ -51,7 +51,7 @@ class PermissionService {
 	/** @var string */
 	private $userId;
 	/** @var array */
-	private $users;
+	private $users = [];
 
 	public function __construct(
 		ILogger $logger,
@@ -196,24 +196,24 @@ class PermissionService {
 		} catch (DoesNotExistException $e) {
 			return [];
 		}
-		$users = [
-			new User($this->userManager->get($board->getOwner()))
-		];
+		$owner = $this->userManager->get($board->getOwner());
+		$users = [];
+		$users[$owner->getUID()] = new User($owner);
 		$acls = $this->aclMapper->findAll($boardId);
 		/** @var Acl $acl */
 		foreach ($acls as $acl) {
 			if ($acl->getType() === Acl::PERMISSION_TYPE_USER) {
 				$user = $this->userManager->get($acl->getParticipant());
-				$users[] = new User($user);
+				$users[$user->getUID()] = new User($user);
 			}
 			if($acl->getType() === Acl::PERMISSION_TYPE_GROUP) {
 				$group = $this->groupManager->get($acl->getParticipant());
 				foreach ($group->getUsers() as $user) {
-					$users[] = new User($user);
+					$users[$user->getUID()] = new User($user);
 				}
 			}
 		}
-		$this->users[(string)$boardId] = array_unique($users);
-		return $this->users;
+		$this->users[(string)$boardId] = $users;
+		return $this->users[(string)$boardId];
 	}
 }
\ No newline at end of file
diff --git a/tests/unit/Service/PermissionServiceTest.php b/tests/unit/Service/PermissionServiceTest.php
index 2c7b787ce..0dcaf77b0 100644
--- a/tests/unit/Service/PermissionServiceTest.php
+++ b/tests/unit/Service/PermissionServiceTest.php
@@ -28,10 +28,13 @@ use OCA\Deck\Db\AclMapper;
 use OCA\Deck\Db\Board;
 use OCA\Deck\Db\BoardMapper;
 use OCA\Deck\Db\IPermissionMapper;
+use OCA\Deck\Db\User;
 use OCA\Deck\NoPermissionException;
 use OCP\AppFramework\Db\DoesNotExistException;
+use OCP\IGroup;
 use OCP\IGroupManager;
 use OCP\ILogger;
+use OCP\IUser;
 use OCP\IUserManager;
 
 class PermissionServiceTest extends \PHPUnit_Framework_TestCase {
@@ -74,7 +77,6 @@ class PermissionServiceTest extends \PHPUnit_Framework_TestCase {
 		);
 	}
 
-
 	public function testGetPermissionsOwner() {
 		$board = new Board();
 		$board->setOwner('admin');
@@ -295,5 +297,73 @@ class PermissionServiceTest extends \PHPUnit_Framework_TestCase {
         return $result;
     }
 
+	public function testFindUsersFail() {
+		$this->boardMapper->expects($this->once())
+			->method('find')
+			->with(123)
+			->will($this->throwException(new DoesNotExistException('')));
+		$users = $this->service->findUsers(123);
+		$this->assertEquals([], $users);
+	}
+
+	/**
+	 * @param $uid
+	 * @return IUser
+	 */
+	private function mockUser($uid) {
+		$user = $this->createMock(IUser::class);
+		$user->expects($this->any())
+			->method('getUID')
+			->willReturn($uid);
+		return $user;
+	}
+
+	public function testFindUsers() {
+		$user1 = $this->mockUser('user1');
+		$user2 = $this->mockUser('user2');
+		$user3 = $this->mockUser('user3');
+		$aclUser = new Acl();
+		$aclUser->setType(Acl::PERMISSION_TYPE_USER);
+		$aclUser->setParticipant('user2');
+		$aclGroup = new Acl();
+		$aclGroup->setType(Acl::PERMISSION_TYPE_GROUP);
+		$aclGroup->setParticipant('group1');
+
+		$board = $this->createMock(Board::class);
+		$board->expects($this->at(0))
+			->method('__call')
+			->with('getOwner', [])
+			->willReturn('user1');
+		$this->aclMapper->expects($this->once())
+			->method('findAll')
+			->with(123)
+			->willReturn([$aclUser, $aclGroup]);
+		$this->boardMapper->expects($this->once())
+			->method('find')
+			->with(123)
+			->willReturn($board);
+		$this->userManager->expects($this->at(0))
+			->method('get')
+			->with('user1')
+			->willReturn($user1);
+		$this->userManager->expects($this->at(1))
+			->method('get')
+			->with('user2')
+			->willReturn($user2);
+		$group = $this->createMock(IGroup::class);
+		$group->expects($this->once())
+			->method('getUsers')
+			->willReturn([$user3]);
+		$this->groupManager->expects($this->once())
+			->method('get')
+			->with('group1')
+			->willReturn($group);
+		$users = $this->service->findUsers(123);
+		$this->assertEquals([
+			'user1' => new User($user1),
+			'user2' => new User($user2),
+			'user3' => new User($user3),
+		], $users);
+	}
 
 }