From b5d994394e2c67d37d9a9360731d08d7112d18fe Mon Sep 17 00:00:00 2001 From: Philipp Date: Sun, 17 Oct 2021 23:10:10 +0200 Subject: [PATCH] Fixing PUBLIC usage, Fixing DB-View, Creating tests --- database.sql | 20 +++ src/BaseEntity.php | 14 ++ src/Factory/Api/Mastodon/Account.php | 15 +- src/Factory/Api/Mastodon/Field.php | 2 +- src/Module/ActivityPub/Objects.php | 2 +- src/Module/Profile/Status.php | 2 +- src/Module/Settings/Profile/Index.php | 6 +- .../ProfileField/Depository/ProfileField.php | 10 +- .../ProfileField/Factory/ProfileField.php | 2 +- src/Protocol/ActivityPub/Transmitter.php | 2 +- .../Depository/PermissionSet.php | 62 ++++++-- static/dbview.config.php | 4 +- .../Depository/ProfileFieldTest.php | 148 ++++++++++++++++++ .../Depository/PermissionSetTest.php | 60 +++++-- 14 files changed, 304 insertions(+), 45 deletions(-) create mode 100644 tests/src/Profile/ProfileField/Depository/ProfileFieldTest.php diff --git a/database.sql b/database.sql index 793aae386..4fd6ba39f 100644 --- a/database.sql +++ b/database.sql @@ -2587,3 +2587,23 @@ CREATE VIEW `workerqueue-view` AS SELECT FROM `process` INNER JOIN `workerqueue` ON `workerqueue`.`pid` = `process`.`pid` WHERE NOT `workerqueue`.`done`; + +-- +-- VIEW profile_field-view +-- +DROP VIEW IF EXISTS `profile_field-view`; +CREATE VIEW `profile_field-view` AS SELECT + `profile_field`.`id` AS `id`, + `profile_field`.`uid` AS `uid`, + `profile_field`.`label` AS `label`, + `profile_field`.`value` AS `value`, + `profile_field`.`order` AS `order`, + `profile_field`.`psid` AS `psid`, + `permissionset`.`allow_cid` AS `allow_cid`, + `permissionset`.`allow_gid` AS `allow_gid`, + `permissionset`.`deny_cid` AS `deny_cid`, + `permissionset`.`deny_gid` AS `deny_gid`, + `profile_field`.`created` AS `created`, + `profile_field`.`edited` AS `edited` + FROM `profile_field` + INNER JOIN `permissionset` ON `permissionset`.`id` = `profile_field`.`psid`; diff --git a/src/BaseEntity.php b/src/BaseEntity.php index a5b968397..2ee22b1bb 100644 --- a/src/BaseEntity.php +++ b/src/BaseEntity.php @@ -53,4 +53,18 @@ abstract class BaseEntity extends BaseDataTransferObject return $this->$name; } + + /** + * @param $name + * @return bool + * @throws HTTPException\InternalServerErrorException + */ + public function __isset($name) + { + if (!property_exists($this, $name)) { + throw new HTTPException\InternalServerErrorException('Unknown property ' . $name . ' in Entity ' . static::class); + } + + return !empty($this->$name); + } } diff --git a/src/Factory/Api/Mastodon/Account.php b/src/Factory/Api/Mastodon/Account.php index 1d067352a..a987e6874 100644 --- a/src/Factory/Api/Mastodon/Account.php +++ b/src/Factory/Api/Mastodon/Account.php @@ -27,8 +27,7 @@ use Friendica\Collection\Api\Mastodon\Fields; use Friendica\Model\APContact; use Friendica\Model\Contact; use Friendica\Network\HTTPException; -use Friendica\Repository\ProfileField; -use Friendica\Security\PermissionSet\Depository\PermissionSet; +use Friendica\Profile\ProfileField\Depository\ProfileField as ProfileFieldDepository; use ImagickException; use Psr\Log\LoggerInterface; @@ -36,17 +35,17 @@ class Account extends BaseFactory { /** @var BaseURL */ private $baseUrl; - /** @var ProfileField */ - private $profileFieldRepo; + /** @var ProfileFieldDepository */ + private $profileFieldDepo; /** @var Field */ private $mstdnFieldFactory; - public function __construct(LoggerInterface $logger, BaseURL $baseURL, ProfileField $profileFieldRepo, Field $mstdnFieldFactory) + public function __construct(LoggerInterface $logger, BaseURL $baseURL, ProfileFieldDepository $profileFieldDepo, Field $mstdnFieldFactory) { parent::__construct($logger); $this->baseUrl = $baseURL; - $this->profileFieldRepo = $profileFieldRepo; + $this->profileFieldDepo = $profileFieldDepo; $this->mstdnFieldFactory = $mstdnFieldFactory; } @@ -77,7 +76,7 @@ class Account extends BaseFactory $self_contact = Contact::selectFirst(['uid'], ['nurl' => $publicContact['nurl'], 'self' => true]); if (!empty($self_contact['uid'])) { - $profileFields = $this->profileFieldRepo->select(['uid' => $self_contact['uid'], 'psid' => PermissionSet::PUBLIC]); + $profileFields = $this->profileFieldDepo->selectPublicFieldsByUserId($self_contact['uid']); $fields = $this->mstdnFieldFactory->createFromProfileFields($profileFields); } else { $fields = new Fields(); @@ -95,7 +94,7 @@ class Account extends BaseFactory { $publicContact = Contact::selectFirst([], ['uid' => $userId, 'self' => true]); - $profileFields = $this->profileFieldRepo->select(['uid' => $userId, 'psid' => PermissionSet::PUBLIC]); + $profileFields = $this->profileFieldDepo->selectPublicFieldsByUserId($userId); $fields = $this->mstdnFieldFactory->createFromProfileFields($profileFields); $apContact = APContact::getByURL($publicContact['url'], false); diff --git a/src/Factory/Api/Mastodon/Field.php b/src/Factory/Api/Mastodon/Field.php index aade421bb..397ca57ad 100644 --- a/src/Factory/Api/Mastodon/Field.php +++ b/src/Factory/Api/Mastodon/Field.php @@ -31,7 +31,7 @@ use Friendica\Network\HTTPException; class Field extends BaseFactory { /** - * @param \Friendica\Profile\ProfileField\Entity\ProfileField $profileField + * @param ProfileField $profileField * * @return \Friendica\Object\Api\Mastodon\Field * @throws HTTPException\InternalServerErrorException diff --git a/src/Module/ActivityPub/Objects.php b/src/Module/ActivityPub/Objects.php index 7e2ceb8b0..47d5409d5 100644 --- a/src/Module/ActivityPub/Objects.php +++ b/src/Module/ActivityPub/Objects.php @@ -86,7 +86,7 @@ class Objects extends BaseModule $permissionSets = DI::permissionSet()->selectByContactId($requester_id, $item['uid']); if (!empty($permissionSets)) { $psid = array_merge($permissionSets->column('id'), - [DI::permissionSet()->selectEmptyForUser($item['uid'])]); + [DI::permissionSet()->selectPublic($item['uid'])]); $validated = in_array($item['psid'], $psid); } } diff --git a/src/Module/Profile/Status.php b/src/Module/Profile/Status.php index 4cf097120..c2d691f1c 100644 --- a/src/Module/Profile/Status.php +++ b/src/Module/Profile/Status.php @@ -213,7 +213,7 @@ class Status extends BaseProfile $permissionSets = DI::permissionSet()->selectByContactId($remote_user, $profile['uid']); if (!empty($permissionSets)) { $condition = ['psid' => array_merge($permissionSets->column('id'), - [DI::permissionSet()->selectEmptyForUser($profile['uid'])->id])]; + [DI::permissionSet()->selectPublic($profile['uid'])->id])]; } } elseif ($profile['uid'] == local_user()) { $condition = []; diff --git a/src/Module/Settings/Profile/Index.php b/src/Module/Settings/Profile/Index.php index 7ebf5b712..05b5fd3ea 100644 --- a/src/Module/Settings/Profile/Index.php +++ b/src/Module/Settings/Profile/Index.php @@ -287,11 +287,11 @@ class Index extends BaseSettings $profileFieldInputs['new']['value'], $permissionSet )); - - unset($profileFieldInputs['new']); - unset($profileFieldOrder['new']); } + unset($profileFieldInputs['new']); + unset($profileFieldOrder['new']); + foreach ($profileFieldInputs as $id => $profileFieldInput) { $permissionSet = DI::permissionSet()->selectOrCreate(DI::permissionSetFactory()->createFromString( $uid, diff --git a/src/Profile/ProfileField/Depository/ProfileField.php b/src/Profile/ProfileField/Depository/ProfileField.php index dafe08f95..79bf21b49 100644 --- a/src/Profile/ProfileField/Depository/ProfileField.php +++ b/src/Profile/ProfileField/Depository/ProfileField.php @@ -125,9 +125,11 @@ class ProfileField extends BaseDepository public function selectPublicFieldsByUserId(int $uid): Collection\ProfileFields { try { + $publicPermissionSet = $this->permissionSetDepository->selectPublic($uid); + return $this->select([ 'uid' => $uid, - 'psid' => PermissionSetDepository::PUBLIC, + 'psid' => $publicPermissionSet->id ]); } catch (\Exception $exception) { throw new ProfileFieldPersistenceException(sprintf('Cannot select public ProfileField for user "%d"', $uid), $exception); @@ -166,7 +168,7 @@ class ProfileField extends BaseDepository $permissionSetIds = $permissionSets->column('id'); // Includes public custom fields - $permissionSetIds[] = PermissionSetDepository::PUBLIC; + $permissionSetIds[] = $this->permissionSetDepository->selectPublic($uid)->id; return $this->select( ['uid' => $uid, 'psid' => $permissionSetIds], @@ -215,6 +217,10 @@ class ProfileField extends BaseDepository */ public function save(Entity\ProfileField $profileField): Entity\ProfileField { + if ($profileField->permissionSet->id === null) { + throw new ProfileFieldPersistenceException('PermissionSet needs to be saved first.'); + } + $fields = $this->convertToTableRow($profileField); try { diff --git a/src/Profile/ProfileField/Factory/ProfileField.php b/src/Profile/ProfileField/Factory/ProfileField.php index 4253d93b9..80a8f5502 100644 --- a/src/Profile/ProfileField/Factory/ProfileField.php +++ b/src/Profile/ProfileField/Factory/ProfileField.php @@ -49,7 +49,7 @@ class ProfileField extends BaseFactory implements ICanCreateFromTableRow public function createFromTableRow(array $row, PermissionSet $permissionSet = null): Entity\ProfileField { if (empty($permissionSet) && - (empty($row['psid']) || !array_key_exists('allow_cid', $row) || !array_key_exists('allow_gid', $row) || !array_key_exists('deny_cid', $row) || !array_key_exists('deny_gid', $row)) + (!array_key_exists('psid', $row) || !array_key_exists('allow_cid', $row) || !array_key_exists('allow_gid', $row) || !array_key_exists('deny_cid', $row) || !array_key_exists('deny_gid', $row)) ) { throw new UnexpectedPermissionSetException('Either set the PermissionSet fields (join) or the PermissionSet itself'); } diff --git a/src/Protocol/ActivityPub/Transmitter.php b/src/Protocol/ActivityPub/Transmitter.php index e741789da..f0ed0cf5a 100644 --- a/src/Protocol/ActivityPub/Transmitter.php +++ b/src/Protocol/ActivityPub/Transmitter.php @@ -229,7 +229,7 @@ class Transmitter $permissionSets = DI::permissionSet()->selectByContactId($requester_id, $owner['uid']); if (!empty($permissionSets)) { $condition = ['psid' => array_merge($permissionSets->column('id'), - [DI::permissionSet()->selectEmptyForUser($owner['uid'])])]; + [DI::permissionSet()->selectPublic($owner['uid'])])]; } } } diff --git a/src/Security/PermissionSet/Depository/PermissionSet.php b/src/Security/PermissionSet/Depository/PermissionSet.php index b4a918b6d..b65a933b4 100644 --- a/src/Security/PermissionSet/Depository/PermissionSet.php +++ b/src/Security/PermissionSet/Depository/PermissionSet.php @@ -53,6 +53,22 @@ class PermissionSet extends BaseDepository $this->aclFormatter = $aclFormatter; } + /** + * replaces the PUBLIC id for the public permissionSet + * (no need to create the default permission set over and over again) + * + * @param $condition + */ + private function checkPublicSelect(&$condition) + { + if (empty($condition['allow_cid']) && + empty($condition['allow_gid']) && + empty($condition['deny_cid']) && + empty($condition['deny_gid'])) { + $condition['uid'] = self::PUBLIC; + } + } + /** * @param array $condition * @param array $params @@ -70,6 +86,16 @@ class PermissionSet extends BaseDepository return new Collection\PermissionSets(parent::_select($condition, $params)->getArrayCopy()); } + private function checkPublic(Entity\PermissionSet $permissionSet): bool + { + return (($permissionSet->id === self::PUBLIC) || + (is_null($permissionSet->id) && + empty($permissionSet->allow_cid) && + empty($permissionSet->allow_gid) && + empty($permissionSet->deny_cid) && + empty($permissionSet->deny_gid))); + } + /** * Converts a given PermissionSet into a DB compatible row array * @@ -89,22 +115,18 @@ class PermissionSet extends BaseDepository } /** - * @param int $id A permissionset table row id or self::PUBLIC - * @param int|null $uid Should be provided when id can be self::PUBLIC + * @param int $id A PermissionSet table row id or self::PUBLIC + * @param int $uid The owner of the PermissionSet * @return Entity\PermissionSet * @throws NotFoundException */ - public function selectOneById(int $id, int $uid = null): Entity\PermissionSet + public function selectOneById(int $id, int $uid): Entity\PermissionSet { if ($id === self::PUBLIC) { - if (empty($uid)) { - throw new \InvalidArgumentException('Missing uid for Public permission set instantiation'); - } - return $this->factory->createFromString($uid); } - return $this->selectOne(['id' => $id]); + return $this->selectOne(['id' => $id, 'uid' => $uid]); } /** @@ -174,15 +196,15 @@ class PermissionSet extends BaseDepository } /** - * Fetch the empty PermissionSet for a given user, create it if it doesn't exist + * Fetch the public PermissionSet * * @param int $uid * * @return Entity\PermissionSet */ - public function selectEmptyForUser(int $uid): Entity\PermissionSet + public function selectPublic(int $uid): Entity\PermissionSet { - return $this->selectOrCreate($this->factory->createFromString($uid)); + return $this->factory->createFromString($uid, '', '', '', '', self::PUBLIC); } /** @@ -198,6 +220,11 @@ class PermissionSet extends BaseDepository return $permissionSet; } + // Don't select/update Public permission sets + if ($this->checkPublic($permissionSet)) { + return $this->selectPublic($permissionSet->uid); + } + try { return $this->selectOne($this->convertToTableRow($permissionSet)); } catch (NotFoundException $exception) { @@ -205,8 +232,19 @@ class PermissionSet extends BaseDepository } } + /** + * @param Entity\PermissionSet $permissionSet + * + * @return Entity\PermissionSet + * @throws NotFoundException + */ public function save(Entity\PermissionSet $permissionSet): Entity\PermissionSet { + // Don't save/update the common public PermissionSet + if ($this->checkPublic($permissionSet)) { + return $this->selectPublic($permissionSet->uid); + } + $fields = $this->convertToTableRow($permissionSet); if ($permissionSet->id) { @@ -214,7 +252,7 @@ class PermissionSet extends BaseDepository } else { $this->db->insert(self::$table_name, $fields); - $permissionSet = $this->selectOneById($this->db->lastInsertId()); + $permissionSet = $this->selectOneById($this->db->lastInsertId(), $permissionSet->uid); } return $permissionSet; diff --git a/static/dbview.config.php b/static/dbview.config.php index a373f4c28..f8d11955e 100644 --- a/static/dbview.config.php +++ b/static/dbview.config.php @@ -1058,7 +1058,9 @@ "allow_cid" => ["permissionset", "allow_cid"], "allow_gid" => ["permissionset", "allow_gid"], "deny_cid" => ["permissionset", "deny_cid"], - "deny_gid" => ["permissionset", "deny_gid"] + "deny_gid" => ["permissionset", "deny_gid"], + "created" => ["profile_field", "created"], + "updated" => ["profile_field", "updated"], ], "query" => "FROM `profile_field` INNER JOIN `permissionset` ON `permissionset`.`id` = `profile_field`.`psid`" diff --git a/tests/src/Profile/ProfileField/Depository/ProfileFieldTest.php b/tests/src/Profile/ProfileField/Depository/ProfileFieldTest.php new file mode 100644 index 000000000..ecd4014fa --- /dev/null +++ b/tests/src/Profile/ProfileField/Depository/ProfileFieldTest.php @@ -0,0 +1,148 @@ +addRules(include __DIR__ . '/../../../../../static/dependencies.config.php') + ->addRule(Database::class, ['instanceOf' => StaticDatabase::class, 'shared' => true]); + + $this->depository = $dice->create(ProfileFieldDepository::class); + $this->factory = $dice->create(ProfileFieldFactory::class); + $this->permissionSetFactory = $dice->create(PermissionSetFactory::class); + $this->permissionSetDepository = $dice->create(PermissionSetDepository::class); + $this->dba = $dice->create(Database::class); + } + + /** + * Test create ProfileField without a valid PermissionSet + */ + public function testSavingWithoutPermissionSet() + { + self::expectExceptionMessage('PermissionSet needs to be saved first.'); + self::expectException(ProfileFieldPersistenceException::class); + + $this->loadFixture(__DIR__ . '/../../../../datasets/api.fixture.php', DI::dba()); + + $profileField = $this->factory->createFromValues(42, 0, 'public', 'value', $this->permissionSetFactory->createFromString(42, '', '<~>')); + + self::assertEquals($profileField->uid, $profileField->permissionSet->uid); + + $this->depository->save($profileField); + } + + /** + * Test saving a new entity + */ + public function testSaveNew() + { + $this->loadFixture(__DIR__ . '/../../../../datasets/api.fixture.php', DI::dba()); + + $profileField = $this->factory->createFromValues(42, 0, 'public', 'value', $this->permissionSetDepository->save($this->permissionSetFactory->createFromString(42, '', '<~>'))); + + self::assertEquals($profileField->uid, $profileField->permissionSet->uid); + + $savedProfileField = $this->depository->save($profileField); + + self::assertNotNull($savedProfileField->id); + self::assertNull($profileField->id); + + /** @var ProfileField $selectedProfileField */ + $selectedProfileField = $this->depository->selectOneById($savedProfileField->id); + + self::assertEquals($savedProfileField, $selectedProfileField); + } + + /** + * Test updating the order of a ProfileField + */ + public function testUpdateOrder() + { + $this->loadFixture(__DIR__ . '/../../../../datasets/api.fixture.php', DI::dba()); + + $profileField = $this->factory->createFromValues(42, 0, 'public', 'value', $this->permissionSetDepository->save($this->permissionSetFactory->createFromString(42, '', '<~>'))); + + self::assertEquals($profileField->uid, $profileField->permissionSet->uid); + + $savedProfileField = $this->depository->save($profileField); + + self::assertNotNull($savedProfileField->id); + self::assertNull($profileField->id); + + /** @var ProfileField $selectedProfileField */ + $selectedProfileField = $this->depository->selectOneById($savedProfileField->id); + + self::assertEquals($savedProfileField, $selectedProfileField); + + $selectedProfileField->setOrder(66); + + $updatedOrderProfileField = $this->depository->save($selectedProfileField); + + self::assertEquals($selectedProfileField->id, $updatedOrderProfileField->id); + self::assertEquals(66, $updatedOrderProfileField->order); + + // Even using the ID of the old, saved ProfileField returns the right instance + $updatedFromOldProfileField = $this->depository->selectOneById($savedProfileField->id); + self::assertEquals(66, $updatedFromOldProfileField->order); + } + + /** + * Test updating a whole entity + */ + public function testUpdate() + { + $this->loadFixture(__DIR__ . '/../../../../datasets/api.fixture.php', DI::dba()); + + $profileField = $this->factory->createFromValues(42, 0, 'public', 'value', $this->permissionSetDepository->save($this->permissionSetFactory->createFromString(42, '', '<~>'))); + + self::assertEquals($profileField->uid, $profileField->permissionSet->uid); + + $savedProfileField = $this->depository->save($profileField); + + self::assertNotNull($savedProfileField->id); + self::assertNull($profileField->id); + + /** @var ProfileField $selectedProfileField */ + $selectedProfileField = $this->depository->selectOneById($savedProfileField->id); + + self::assertEquals($savedProfileField, $selectedProfileField); + + $savedProfileField->update('another', 5, $this->permissionSetDepository->selectPublic(42)); + self::assertEquals(PermissionSet::PUBLIC, $savedProfileField->permissionSet->id); + + $publicProfileField = $this->depository->save($savedProfileField); + + self::assertEquals($this->permissionSetDepository->selectPublic(42), $publicProfileField->permissionSet); + self::assertEquals('another', $publicProfileField->value); + self::assertEquals(5, $publicProfileField->order); + } +} diff --git a/tests/src/Security/PermissionSet/Depository/PermissionSetTest.php b/tests/src/Security/PermissionSet/Depository/PermissionSetTest.php index 305769411..a136ff84c 100644 --- a/tests/src/Security/PermissionSet/Depository/PermissionSetTest.php +++ b/tests/src/Security/PermissionSet/Depository/PermissionSetTest.php @@ -5,36 +5,68 @@ namespace Friendica\Test\src\Security\PermissionSet\Depository; use Dice\Dice; use Friendica\Database\Database; use Friendica\DI; -use Friendica\Security\PermissionSet\Depository\PermissionSet; -use Friendica\Test\MockedTest; +use Friendica\Security\PermissionSet\Depository\PermissionSet as PermissionSetDepository; +use Friendica\Security\PermissionSet\Factory\PermissionSet as PermissionSetFactory; +use Friendica\Test\DatabaseTest; use Friendica\Test\Util\Database\StaticDatabase; -class PermissionSetTest extends MockedTest +class PermissionSetTest extends DatabaseTest { - /** @var PermissionSet */ + /** @var PermissionSetDepository */ private $depository; + /** @var PermissionSetFactory */ + private $factory; public function setUp(): void { + parent::setUp(); + $dice = (new Dice()) ->addRules(include __DIR__ . '/../../../../../static/dependencies.config.php') ->addRule(Database::class, ['instanceOf' => StaticDatabase::class, 'shared' => true]); - DI::init($dice); - $this->depository = DI::permissionSet(); - } - - public function testSelectOneByIdPublicMissingUid() - { - $this->expectException(\InvalidArgumentException::class); - - $this->depository->selectOneById(PermissionSet::PUBLIC); + $this->depository = $dice->create(PermissionSetDepository::class); + $this->factory = $dice->create(PermissionSetFactory::class); } public function testSelectOneByIdPublic() { - $permissionSet = $this->depository->selectOneById(PermissionSet::PUBLIC, 1); + $permissionSet = $this->depository->selectPublic(1); $this->assertInstanceOf(\Friendica\Security\PermissionSet\Entity\PermissionSet::class, $permissionSet); + self::assertEmpty($permissionSet->allow_cid); + self::assertEmpty($permissionSet->allow_gid); + self::assertEmpty($permissionSet->deny_cid); + self::assertEmpty($permissionSet->deny_gid); + self::assertEmpty(PermissionSetDepository::PUBLIC, $permissionSet->id); + self::assertEquals(1, $permissionSet->uid); + } + + /** + * Test create/update PermissionSets + */ + public function testSaving() + { + $this->loadFixture(__DIR__ . '/../../../../datasets/api.fixture.php', DI::dba()); + + $permissionSet = $this->factory->createFromString(42, '', '<~>'); + + $permissionSet = $this->depository->selectOrCreate($permissionSet); + + self::assertNotNull($permissionSet->id); + + $permissionSetSelected = $this->depository->selectOneById($permissionSet->id, 42); + + self::assertEquals($permissionSet, $permissionSetSelected); + + $newPermissionSet = $permissionSet->withAllowedContacts(['1', '2']); + $savedPermissionSet = $this->depository->save($newPermissionSet); + + self::assertNotNull($savedPermissionSet->id); + self::assertNull($newPermissionSet->id); + + $permissionSetSavedSelected = $this->depository->selectOneById($savedPermissionSet->id, 42); + + self::assertEquals($savedPermissionSet, $permissionSetSavedSelected); } }