Add feedback and tests

src/DI.php

@@ -22,7 +22,6 @@
 namespace Friendica;
 
 use Dice\Dice;
-use Friendica\Security\PermissionSet\Depository\PermissionSet;
 use Psr\Log\LoggerInterface;
 
 /**
@@ -443,20 +442,14 @@ abstract class DI
 		return self::$dice->create(Repository\Introduction::class);
 	}
 
-	/**
+	public static function permissionSet(): Security\PermissionSet\Depository\PermissionSet
-	 * @return PermissionSet
-	 */
-	public static function permissionSet()
 	{
-		return self::$dice->create(PermissionSet::class);
+		return self::$dice->create(Security\PermissionSet\Depository\PermissionSet::class);
 	}
 
-	/**
+	public static function permissionSetFactory(): Security\PermissionSet\Factory\PermissionSet
-	 * @return \Friendica\Security\PermissionSet\Factory\PermissionSet
-	 */
-	public static function permissionSetFactory()
 	{
-		return self::$dice->create(\Friendica\Security\PermissionSet\Factory\PermissionSet::class);
+		return self::$dice->create(Security\PermissionSet\Factory\PermissionSet::class);
 	}
 
 	/**

src/Model/ProfileField.php

@@ -23,6 +23,7 @@ namespace Friendica\Model;
 
 use Friendica\BaseModel;
 use Friendica\Database\Database;
+use Friendica\Security\PermissionSet\Depository\PermissionSet as PermissionSetDepository;
 use Friendica\Security\PermissionSet\Entity\PermissionSet;
 use Psr\Log\LoggerInterface;
 
@@ -46,10 +47,10 @@ class ProfileField extends BaseModel
 	/** @var PermissionSet */
 	private $permissionset;
 
-	/** @var \Friendica\Security\PermissionSet\Depository\PermissionSet */
+	/** @var PermissionSetDepository */
 	private $permissionSetDepository;
 
-	public function __construct(Database $dba, LoggerInterface $logger,\Friendica\Security\PermissionSet\Depository\PermissionSet $permissionSetDepository, array $data = [])
+	public function __construct(Database $dba, LoggerInterface $logger, PermissionSetDepository $permissionSetDepository, array $data = [])
 	{
 		parent::__construct($dba, $logger, $data);
 
@@ -62,7 +63,7 @@ class ProfileField extends BaseModel
 
 		switch ($name) {
 			case 'permissionset':
-				$this->permissionset = $this->permissionset ?? $this->permissionSetDepository->selectOneById($this->psid);
+				$this->permissionset = $this->permissionset ?? $this->permissionSetDepository->selectOneForUser($this->uid, $this->psid);
 
 				$return = $this->permissionset;
 				break;

src/Security/PermissionSet/Depository/PermissionSet.php

@@ -70,6 +70,24 @@ class PermissionSet extends BaseDepository
 		return new Collection\PermissionSets(parent::_select($condition, $params)->getArrayCopy());
 	}
 
+	/**
+	 * Converts a given PermissionSet into a DB compatible row array
+	 *
+	 * @param Entity\PermissionSet $permissionSet
+	 *
+	 * @return array
+	 */
+	protected function convertToTableRow(Entity\PermissionSet $permissionSet): array
+	{
+		return [
+			'uid'       => $permissionSet->uid,
+			'allow_cid' => $this->aclFormatter->toString($permissionSet->allow_cid),
+			'allow_gid' => $this->aclFormatter->toString($permissionSet->allow_gid),
+			'deny_cid'  => $this->aclFormatter->toString($permissionSet->deny_cid),
+			'deny_gid'  => $this->aclFormatter->toString($permissionSet->deny_gid),
+		];
+	}
+
 	/**
 	 * @param int $id
 	 *
@@ -159,6 +177,23 @@ class PermissionSet extends BaseDepository
 		return $this->selectOrCreate($this->factory->createFromString($uid));
 	}
 
+	/**
+	 * Fetch one PermissionSet with check for ownership
+	 *
+	 * @param int $uid The user id
+	 * @param int $id  The unique id of the PermissionSet
+	 *
+	 * @return Entity\PermissionSet
+	 * @throws NotFoundException in case either the id is invalid or the PermissionSet does not relay to the given user
+	 */
+	public function selectOneForUser(int $uid, int $id): Entity\PermissionSet
+	{
+		return $this->selectOne([
+			'id'  => $id,
+			'uid' => $uid,
+		]);
+	}
+
 	/**
 	 * Selects or creates a PermissionSet based on it's fields
 	 *
@@ -172,16 +207,8 @@ class PermissionSet extends BaseDepository
 			return $permissionSet;
 		}
 
-		$fields = [
-			'uid'       => $permissionSet->uid,
-			'allow_cid' => $this->aclFormatter->toString($permissionSet->allow_cid),
-			'allow_gid' => $this->aclFormatter->toString($permissionSet->allow_gid),
-			'deny_cid'  => $this->aclFormatter->toString($permissionSet->deny_cid),
-			'deny_gid'  => $this->aclFormatter->toString($permissionSet->deny_gid),
-		];
-
 		try {
-			return $this->selectOne($fields);
+			return $this->selectOne($this->convertToTableRow($permissionSet));
 		} catch (NotFoundException $exception) {
 			return $this->save($permissionSet);
 		}
@@ -189,13 +216,7 @@ class PermissionSet extends BaseDepository
 
 	public function save(Entity\PermissionSet $permissionSet): Entity\PermissionSet
 	{
-		$fields = [
+		$fields = $this->convertToTableRow($permissionSet);
-			'uid'       => $permissionSet->uid,
-			'allow_cid' => $this->aclFormatter->toString($permissionSet->allow_cid),
-			'allow_gid' => $this->aclFormatter->toString($permissionSet->allow_gid),
-			'deny_cid'  => $this->aclFormatter->toString($permissionSet->deny_cid),
-			'deny_gid'  => $this->aclFormatter->toString($permissionSet->deny_gid),
-		];
 
 		if ($permissionSet->id) {
 			$this->db->update(self::$table_name, $fields, ['id' => $permissionSet->id]);

src/Security/PermissionSet/Factory/PermissionSet.php

@@ -20,39 +20,45 @@ class PermissionSet extends BaseFactory implements ICanCreateFromTableRow
 		$this->formatter = $formatter;
 	}
 
+	/**
+	 * @inheritDoc
+	 */
 	public function createFromTableRow(array $row): Entity\PermissionSet
 	{
 		return new Entity\PermissionSet(
 			$row['uid'],
-			$this->formatter->expand($row['allow_cid'] ?? []),
+			$this->formatter->expand($row['allow_cid'] ?? ''),
-			$this->formatter->expand($row['allow_gid'] ?? []),
+			$this->formatter->expand($row['allow_gid'] ?? ''),
-			$this->formatter->expand($row['deny_cid'] ?? []),
+			$this->formatter->expand($row['deny_cid'] ?? ''),
-			$this->formatter->expand($row['deny_gid'] ?? []),
+			$this->formatter->expand($row['deny_gid'] ?? ''),
 			$row['id'] ?? null
 		);
 	}
 
+	/**
+	 * Creates a new PermissionSet based on it's fields
+	 *
+	 * @param int    $uid
+	 * @param string $allow_cid
+	 * @param string $allow_gid
+	 * @param string $deny_cid
+	 * @param string $deny_gid
+	 *
+	 * @return Entity\PermissionSet
+	 */
 	public function createFromString(
 		int $uid,
 		string $allow_cid = '',
 		string $allow_gid = '',
 		string $deny_cid = '',
-		string $deny_gid = '')
+		string $deny_gid = ''): Entity\PermissionSet
 	{
-		return new Entity\PermissionSet(
+		return $this->createFromTableRow([
-			$uid,
+			'uid'       => $uid,
-			$this->formatter->expand($allow_cid),
+			'allow_cid' => $allow_cid,
-			$this->formatter->expand($allow_gid),
+			'allow_gid' => $allow_gid,
-			$this->formatter->expand($deny_cid),
+			'deny_cid'  => $deny_cid,
-			$this->formatter->expand($deny_gid)
+			'deny_gid'  => $deny_gid,
-		);
+		]);
-	}
-
-	public function createPrototypeForUser(int $uid, string $allowCid): Entity\PermissionSet
-	{
-		return new Entity\PermissionSet(
-			$uid,
-			$this->formatter->expand($allowCid)
-		);
 	}
 }

tests/src/Security/PermissionSet/Entity/PermissionSetTest.php

@@ -0,0 +1,45 @@
+<?php
+
+namespace Friendica\Test\src\Security\PermissionSet\Entity;
+
+use Friendica\Security\PermissionSet\Entity\PermissionSet;
+use Friendica\Test\MockedTest;
+
+class PermissionSetTest extends MockedTest
+{
+	public function dateAllowedContacts()
+	{
+		return [
+			'default' => [
+				'id'         => 10,
+				'allow_cid'  => ['1', '2'],
+				'allow_gid'  => ['3', '4'],
+				'deny_cid'   => ['5', '6', '7'],
+				'deny_gid'   => ['8'],
+				'update_cid' => ['10'],
+			],
+		];
+	}
+
+	/**
+	 * Test if the call "withAllowedContacts()" creates a clone
+	 *
+	 * @dataProvider dateAllowedContacts
+	 */
+	public function testWithAllowedContacts(int $id, array $allow_cid, array $allow_gid, array $deny_cid, array $deny_gid, array $update_cid)
+	{
+		$permissionSetOrig = new PermissionSet(
+			$id,
+			$allow_cid,
+			$allow_gid,
+			$deny_cid,
+			$deny_gid
+		);
+
+		$permissionSetNew = $permissionSetOrig->withAllowedContacts($update_cid);
+
+		self::assertNotSame($permissionSetOrig, $permissionSetNew);
+		self::assertEquals($update_cid, $permissionSetNew->allow_cid);
+		self::assertEquals($allow_cid, $permissionSetOrig->allow_cid);
+	}
+}

tests/src/Security/PermissionSet/Factory/PermissionSetTest.php

@@ -0,0 +1,134 @@
+<?php
+
+namespace Friendica\Test\src\Security\PermissionSet\Factory;
+
+use Friendica\Security\PermissionSet\Factory\PermissionSet;
+use Friendica\Test\MockedTest;
+use Friendica\Util\ACLFormatter;
+use Psr\Log\NullLogger;
+
+class PermissionSetTest extends MockedTest
+{
+	/** @var PermissionSet */
+	protected $permissionSet;
+
+	protected function setUp(): void
+	{
+		parent::setUp();
+
+		$this->permissionSet = new PermissionSet(new NullLogger(), new ACLFormatter());
+	}
+
+	public function dataInput()
+	{
+		return [
+			'new' => [
+				'input' => [
+					'uid'       => 12,
+					'allow_cid' => '<1>,<2>',
+					'allow_gid' => '<3>,<4>',
+					'deny_cid'  => '<6>',
+					'deny_gid'  => '<8>',
+				],
+				'assertion' => [
+					'id'        => null,
+					'uid'       => 12,
+					'allow_cid' => ['1', '2'],
+					'allow_gid' => ['3', '4'],
+					'deny_cid'  => ['6'],
+					'deny_gid'  => ['8'],
+				],
+			],
+			'full' => [
+				'input' => [
+					'id'        => 3,
+					'uid'       => 12,
+					'allow_cid' => '<1>,<2>',
+					'allow_gid' => '<3>,<4>',
+					'deny_cid'  => '<6>',
+					'deny_gid'  => '<8>',
+				],
+				'assertion' => [
+					'id'        => 3,
+					'uid'       => 12,
+					'allow_cid' => ['1', '2'],
+					'allow_gid' => ['3', '4'],
+					'deny_cid'  => ['6'],
+					'deny_gid'  => ['8'],
+				],
+			],
+			'mini' => [
+				'input' => [
+					'id'  => null,
+					'uid' => 12,
+				],
+				'assertion' => [
+					'id'        => null,
+					'uid'       => 12,
+					'allow_cid' => [],
+					'allow_gid' => [],
+					'deny_cid'  => [],
+					'deny_gid'  => [],
+				],
+			],
+			'wrong' => [
+				'input' => [
+					'id'        => 3,
+					'uid'       => 12,
+					'allow_cid' => '<1,<2>',
+				],
+				'assertion' => [
+					'id'        => 3,
+					'uid'       => 12,
+					'allow_cid' => ['2'],
+					'allow_gid' => [],
+					'deny_cid'  => [],
+					'deny_gid'  => [],
+				],
+			]
+		];
+	}
+
+	protected function assertPermissionSet(\Friendica\Security\PermissionSet\Entity\PermissionSet $permissionSet, array $assertion)
+	{
+		self::assertEquals($assertion['id'] ?? null, $permissionSet->id);
+		self::assertNotNull($permissionSet->uid);
+		self::assertEquals($assertion['uid'], $permissionSet->uid);
+		self::assertEquals($assertion['allow_cid'], $permissionSet->allow_cid);
+		self::assertEquals($assertion['allow_gid'], $permissionSet->allow_gid);
+		self::assertEquals($assertion['deny_cid'], $permissionSet->deny_cid);
+		self::assertEquals($assertion['deny_gid'], $permissionSet->deny_gid);
+	}
+
+	/**
+	 * Test the createFromTableRow method
+	 *
+	 * @dataProvider dataInput
+	 */
+	public function testCreateFromTableRow(array $input, array $assertion)
+	{
+		$permissionSet = $this->permissionSet->createFromTableRow($input);
+
+		$this->assertPermissionSet($permissionSet, $assertion);
+	}
+
+	/**
+	 * Test the createFromString method
+	 *
+	 * @dataProvider dataInput
+	 */
+	public function testCreateFromString(array $input, array $assertion)
+	{
+		$permissionSet = $this->permissionSet->createFromString(
+			$input['uid'],
+			$input['allow_cid'] ?? '',
+			$input['allow_gid'] ?? '',
+			$input['deny_cid'] ?? '',
+			$input['deny_gid'] ?? ''
+		);
+
+		unset($assertion['id']);
+
+		$this->assertPermissionSet($permissionSet, $assertion);
+	}
+}