From fb79350d3ea30239cc0d6db06dfbe3b7bcdbb576 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 7 May 2021 23:11:25 +0200 Subject: [PATCH 01/10] extend AccountManager API with collection const Signed-off-by: Arthur Schiwon --- lib/public/Accounts/IAccountManager.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/public/Accounts/IAccountManager.php b/lib/public/Accounts/IAccountManager.php index 63fbc9ba31..6289792063 100644 --- a/lib/public/Accounts/IAccountManager.php +++ b/lib/public/Accounts/IAccountManager.php @@ -96,6 +96,8 @@ interface IAccountManager { public const PROPERTY_ADDRESS = 'address'; public const PROPERTY_TWITTER = 'twitter'; + public const COLLECTION_EMAIL = 'additional_mail'; + public const NOT_VERIFIED = '0'; public const VERIFICATION_IN_PROGRESS = '1'; public const VERIFIED = '2'; @@ -123,7 +125,10 @@ interface IAccountManager { /** * Search for users based on account data * - * @param string $property + * @param string $property - property or property collection name – since + * NC 22 the implementation MAY add a fitting property collection into the + * search even if a property name was given e.g. email property and email + * collection) * @param string[] $values * @return array * From 956bfba2e247aab5fd33a9bb31a110467ce66348 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 17 May 2021 21:22:49 +0200 Subject: [PATCH 02/10] refactor validators and sanitizers Signed-off-by: Arthur Schiwon --- lib/private/Accounts/AccountManager.php | 92 +++++++++++++++---------- 1 file changed, 57 insertions(+), 35 deletions(-) diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index 53792c70d2..83315ff39b 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -139,6 +139,61 @@ class AccountManager implements IAccountManager { return $input; } + protected function sanitizeLength(array &$propertyData, bool $throwOnData = false): void { + if (isset($propertyData) && isset($propertyData['value']) && strlen($propertyData['value']) > 2048) { + if ($throwOnData) { + throw new \InvalidArgumentException(); + } else { + $propertyData['value'] = ''; + } + } + } + + protected function testValueLengths(array &$data, bool $throwOnData = false): void { + try { + foreach ($data as $propertyName => &$propertyData) { + if ($this->isCollection($propertyName)) { + $this->testValueLengths($propertyData, $throwOnData); + } else { + $this->sanitizeLength($propertyData, $throwOnData); + } + } + } catch (\InvalidArgumentException $e) { + throw new \InvalidArgumentException($propertyName); + } + } + + protected function testPropertyScopes(array &$data, array $allowedScopes, bool $throwOnData = false, string $parentPropertyName = null): void { + foreach ($data as $propertyNameOrIndex => &$propertyData) { + if ($this->isCollection($propertyNameOrIndex)) { + $this->testPropertyScopes($propertyData, $allowedScopes, $throwOnData); + } else if (isset($propertyData['scope'])) { + $effectivePropertyName = $parentPropertyName ?? $propertyNameOrIndex; + + if ($throwOnData && !in_array($propertyData['scope'], $allowedScopes, true)) { + throw new \InvalidArgumentException('scope'); + } + + if ( + $propertyData['scope'] === self::SCOPE_PRIVATE + && ($effectivePropertyName === self::PROPERTY_DISPLAYNAME || $effectivePropertyName === self::PROPERTY_EMAIL) + ) { + if ($throwOnData) { + // v2-private is not available for these fields + throw new \InvalidArgumentException('scope'); + } else { + // default to local + $data[$propertyNameOrIndex]['scope'] = self::SCOPE_LOCAL; + } + } else { + // migrate scope values to the new format + // invalid scopes are mapped to a default value + $data[$propertyNameOrIndex]['scope'] = AccountProperty::mapScopeToV2($propertyData['scope']); + } + } + } + } + /** * update user record * @@ -166,16 +221,7 @@ class AccountManager implements IAccountManager { } } - // set a max length - foreach ($data as $propertyName => $propertyData) { - if (isset($data[$propertyName]) && isset($data[$propertyName]['value']) && strlen($data[$propertyName]['value']) > 2048) { - if ($throwOnData) { - throw new \InvalidArgumentException($propertyName); - } else { - $data[$propertyName]['value'] = ''; - } - } - } + $this->testValueLengths($data); if (isset($data[self::PROPERTY_WEBSITE]) && $data[self::PROPERTY_WEBSITE]['value'] !== '') { try { @@ -198,31 +244,7 @@ class AccountManager implements IAccountManager { self::VISIBILITY_PUBLIC, ]; - // validate and convert scope values - foreach ($data as $propertyName => $propertyData) { - if (isset($propertyData['scope'])) { - if ($throwOnData && !in_array($propertyData['scope'], $allowedScopes, true)) { - throw new \InvalidArgumentException('scope'); - } - - if ( - $propertyData['scope'] === self::SCOPE_PRIVATE - && ($propertyName === self::PROPERTY_DISPLAYNAME || $propertyName === self::PROPERTY_EMAIL) - ) { - if ($throwOnData) { - // v2-private is not available for these fields - throw new \InvalidArgumentException('scope'); - } else { - // default to local - $data[$propertyName]['scope'] = self::SCOPE_LOCAL; - } - } else { - // migrate scope values to the new format - // invalid scopes are mapped to a default value - $data[$propertyName]['scope'] = AccountProperty::mapScopeToV2($propertyData['scope']); - } - } - } + $this->testPropertyScopes($data, $allowedScopes, $throwOnData); if (empty($userData)) { $this->insertNewUser($user, $data); From 839bff1641ba1b2fda6b2658d005d606e04ab00a Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 17 May 2021 21:24:45 +0200 Subject: [PATCH 03/10] deal with property collections when fetching users (with update) Signed-off-by: Arthur Schiwon --- lib/private/Accounts/AccountManager.php | 53 ++++++++++++++++++------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index 83315ff39b..3241ad27c6 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -45,6 +45,7 @@ use OCP\IUser; use Psr\Log\LoggerInterface; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\EventDispatcher\GenericEvent; +use function array_flip; use function json_decode; use function json_last_error; @@ -298,12 +299,9 @@ class AccountManager implements IAccountManager { /** * get stored data from a given user * - * @param IUser $user - * @return array - * * @deprecated use getAccount instead to make sure migrated properties work correctly */ - public function getUser(IUser $user) { + public function getUser(IUser $user, bool $insertIfNotExists = true): array { $uid = $user->getUID(); $query = $this->connection->getQueryBuilder(); $query->select('data') @@ -316,7 +314,9 @@ class AccountManager implements IAccountManager { if (empty($accountData)) { $userData = $this->buildDefaultUserRecord($user); - $this->insertNewUser($user, $userData); + if ($insertIfNotExists) { + $this->insertNewUser($user, $userData); + } return $userData; } @@ -327,9 +327,7 @@ class AccountManager implements IAccountManager { return $this->buildDefaultUserRecord($user); } - $userDataArray = $this->addMissingDefaultValues($userDataArray); - - return $userDataArray; + return $this->addMissingDefaultValues($userDataArray); } public function searchUsers(string $property, array $values): array { @@ -346,12 +344,23 @@ class AccountManager implements IAccountManager { $result = $query->execute(); while ($row = $result->fetch()) { - $matches[$row['value']] = $row['uid']; + $matches[$row['uid']] = $row['value']; } $result->closeCursor(); } - return $matches; + $result = array_merge($matches, $this->searchUsersForRelatedCollection($property, $values)); + + return array_flip($result); + } + + protected function searchUsersForRelatedCollection(string $property, array $values): array { + switch ($property) { + case IAccountManager::PROPERTY_EMAIL: + return array_flip($this->searchUsers(IAccountManager::COLLECTION_EMAIL, $values)); + default: + return []; + } } /** @@ -362,7 +371,7 @@ class AccountManager implements IAccountManager { * @param IUser $user * @return array */ - protected function checkEmailVerification($oldData, $newData, IUser $user) { + protected function checkEmailVerification($oldData, $newData, IUser $user): array { if ($oldData[self::PROPERTY_EMAIL]['value'] !== $newData[self::PROPERTY_EMAIL]['value']) { $this->jobList->add(VerifyUserData::class, [ @@ -403,7 +412,7 @@ class AccountManager implements IAccountManager { * @param array $newData * @return array */ - protected function updateVerifyStatus($oldData, $newData) { + protected function updateVerifyStatus(array $oldData, array $newData): array { // which account was already verified successfully? $twitterVerified = isset($oldData[self::PROPERTY_TWITTER]['verified']) && $oldData[self::PROPERTY_TWITTER]['verified'] === self::VERIFIED; @@ -503,12 +512,20 @@ class AccountManager implements IAccountManager { 'value' => $query->createParameter('value'), ] ); + $this->writeUserDataProperties($query, $data); + } + + protected function writeUserDataProperties(IQueryBuilder $query, array $data, string $parentPropertyName = null): void { foreach ($data as $propertyName => $property) { - if ($propertyName === self::PROPERTY_AVATAR) { + if ($this->isCollection($propertyName)) { + $this->writeUserDataProperties($query, $property, $propertyName); + continue; + } + if (($parentPropertyName ?? $propertyName) === self::PROPERTY_AVATAR) { continue; } - $query->setParameter('name', $propertyName) + $query->setParameter('name', $parentPropertyName ?? $propertyName) ->setParameter('value', $property['value'] ?? ''); $query->execute(); } @@ -590,4 +607,12 @@ class AccountManager implements IAccountManager { $this->updateUser($account->getUser(), $data, true); } + + protected function isCollection(string $propertyName): bool { + return in_array($propertyName, + [ + IAccountManager::COLLECTION_EMAIL, + ] + ); + } } From afea57352bc4690656c1a8a75079d25985fe4aa4 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 17 May 2021 21:25:18 +0200 Subject: [PATCH 04/10] update unit tests Signed-off-by: Arthur Schiwon --- tests/lib/Accounts/AccountManagerTest.php | 182 ++++++++++++++++++++-- 1 file changed, 166 insertions(+), 16 deletions(-) diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php index 9bce92716a..86f38b9f6c 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -26,6 +26,7 @@ use OC\Accounts\AccountManager; use OCP\Accounts\IAccountManager; use OCP\BackgroundJob\IJobList; use OCP\IConfig; +use OCP\IDBConnection; use OCP\IUser; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; @@ -59,13 +60,24 @@ class AccountManagerTest extends TestCase { /** @var LoggerInterface|MockObject */ private $logger; + /** @var AccountManager */ + private $accountManager; + protected function setUp(): void { parent::setUp(); $this->eventDispatcher = $this->createMock(EventDispatcherInterface::class); - $this->connection = \OC::$server->getDatabaseConnection(); + $this->connection = \OC::$server->get(IDBConnection::class); $this->config = $this->createMock(IConfig::class); $this->jobList = $this->createMock(IJobList::class); $this->logger = $this->createMock(LoggerInterface::class); + + $this->accountManager = new AccountManager( + $this->connection, + $this->config, + $this->eventDispatcher, + $this->jobList, + $this->logger, + ); } protected function tearDown(): void { @@ -74,6 +86,90 @@ class AccountManagerTest extends TestCase { $query->delete($this->table)->execute(); } + protected function makeUser(string $uid, string $name, string $email = null): IUser { + $user = $this->createMock(IUser::class); + $user->expects($this->any()) + ->method('getUid') + ->willReturn($uid); + $user->expects($this->any()) + ->method('getDisplayName') + ->willReturn($name); + if ($email !== null) { + $user->expects($this->any()) + ->method('getEMailAddress') + ->willReturn($email); + } + + return $user; + } + + protected function populateOrUpdate(): void { + $users = [ + [ + 'user' => $this->makeUser('j.doe', 'Jane Doe', 'jane.doe@acme.com'), + 'data' => [ + IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Jane Doe', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_EMAIL => ['value' => 'jane.doe@acme.com', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_ADDRESS => ['value' => 'some street', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://acme.com', 'scope' => IAccountManager::SCOPE_PRIVATE], + ], + ], + [ + 'user' => $this->makeUser('a.allison', 'Alice Allison', 'a.allison@example.org'), + 'data' => [ + IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Alice Allison', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_EMAIL => ['value' => 'a.allison@example.org', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_TWITTER => ['value' => '@a_alice', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_PHONE => ['value' => '+491602312121', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_ADDRESS => ['value' => 'Dundee Road 45', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::SCOPE_LOCAL], + ], + ], + [ + 'user' => $this->makeUser('b32c5a5b-1084-4380-8856-e5223b16de9f', 'Armel Oliseh', 'oliseh@example.com'), + 'data' => [ + IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Armel Oliseh', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_EMAIL => ['value' => 'oliseh@example.com', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_TWITTER => ['value' => '', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_PHONE => ['value' => '+491603121212', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_ADDRESS => ['value' => 'Sunflower Blvd. 77', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.com', 'scope' => IAccountManager::SCOPE_PUBLISHED], + ], + ], + [ + 'user' => $this->makeUser('31b5316a-9b57-4b17-970a-315a4cbe73eb', 'K. Cheng', 'cheng@emca.com'), + 'data' => [ + IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'K. Cheng', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_EMAIL => ['value' => 'cheng@emca.com', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_TWITTER => ['value' => '', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_PHONE => ['value' => '+71601212123', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_ADDRESS => ['value' => 'Pinapple Street 22', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://emca.com', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::COLLECTION_EMAIL => [ + ['value' => 'k.cheng@emca.com', 'scope' => IAccountManager::SCOPE_LOCAL], + ['value' => 'kai.cheng@emca.com', 'scope' => IAccountManager::SCOPE_LOCAL], + ], + ], + ], + [ + 'user' => $this->makeUser('goodpal@elpmaxe.org', 'Goodpal, Kim', 'goodpal@elpmaxe.org'), + 'data' => [ + IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Goodpal, Kim', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_EMAIL => ['value' => 'goodpal@elpmaxe.org', 'scope' => IAccountManager::SCOPE_PUBLISHED], + IAccountManager::PROPERTY_TWITTER => ['value' => '', 'scope' => IAccountManager::SCOPE_LOCAL], + IAccountManager::PROPERTY_PHONE => ['value' => '+71602121231', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_ADDRESS => ['value' => 'Octopus Ave 17', 'scope' => IAccountManager::SCOPE_FEDERATED], + IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://elpmaxe.org', 'scope' => IAccountManager::SCOPE_PUBLISHED], + ], + ], + ]; + foreach ($users as $userInfo) { + $this->accountManager->updateUser($userInfo['user'], $userInfo['data'], false); + } + } + /** * get a instance of the accountManager * @@ -340,9 +436,8 @@ class AccountManagerTest extends TestCase { $oldData = ['key' => ['value' => 'value']]; $newData = ['newKey' => ['value' => 'newValue']]; - $accountManager = $this->getInstance(); $this->addDummyValuesToTable('uid', $oldData); - $this->invokePrivate($accountManager, 'updateExistingUser', [$user, $newData]); + $this->invokePrivate($this->accountManager, 'updateExistingUser', [$user, $newData]); $newDataFromTable = $this->getDataFromTable('uid'); $this->assertEquals($newData, $newDataFromTable); } @@ -352,18 +447,15 @@ class AccountManagerTest extends TestCase { $uid = 'uid'; $data = ['key' => ['value' => 'value']]; - $accountManager = $this->getInstance(); $user->expects($this->atLeastOnce())->method('getUID')->willReturn($uid); $this->assertNull($this->getDataFromTable($uid)); - $this->invokePrivate($accountManager, 'insertNewUser', [$user, $data]); + $this->invokePrivate($this->accountManager, 'insertNewUser', [$user, $data]); $dataFromDb = $this->getDataFromTable($uid); $this->assertEquals($data, $dataFromDb); } public function testAddMissingDefaultValues() { - $accountManager = $this->getInstance(); - $input = [ 'key1' => ['value' => 'value1', 'verified' => '0'], 'key2' => ['value' => 'value1'], @@ -374,7 +466,7 @@ class AccountManagerTest extends TestCase { 'key2' => ['value' => 'value1', 'verified' => '0'], ]; - $result = $this->invokePrivate($accountManager, 'addMissingDefaultValues', [$input]); + $result = $this->invokePrivate($this->accountManager, 'addMissingDefaultValues', [$input]); $this->assertSame($expected, $result); } @@ -461,13 +553,11 @@ class AccountManagerTest extends TestCase { $this->config->method('getSystemValueString') ->willReturn($defaultRegion); - $instance = $this->getInstance(); - if ($phoneNumber === null) { $this->expectException(\InvalidArgumentException::class); - self::invokePrivate($instance, 'parsePhoneNumber', [$phoneInput]); + self::invokePrivate($this->accountManager, 'parsePhoneNumber', [$phoneInput]); } else { - self::assertEquals($phoneNumber, self::invokePrivate($instance, 'parsePhoneNumber', [$phoneInput])); + self::assertEquals($phoneNumber, self::invokePrivate($this->accountManager, 'parsePhoneNumber', [$phoneInput])); } } @@ -487,13 +577,73 @@ class AccountManagerTest extends TestCase { * @param string|null $websiteOutput */ public function testParseWebsite(string $websiteInput, ?string $websiteOutput): void { - $instance = $this->getInstance(); - if ($websiteOutput === null) { $this->expectException(\InvalidArgumentException::class); - self::invokePrivate($instance, 'parseWebsite', [$websiteInput]); + self::invokePrivate($this->accountManager, 'parseWebsite', [$websiteInput]); } else { - self::assertEquals($websiteOutput, self::invokePrivate($instance, 'parseWebsite', [$websiteInput])); + self::assertEquals($websiteOutput, self::invokePrivate($this->accountManager, 'parseWebsite', [$websiteInput])); } } + + /** + * @dataProvider searchDataProvider + */ + public function testSearchUsers(string $property, array $values, array $expected): void { + $this->populateOrUpdate(); + + $matchedUsers = $this->accountManager->searchUsers($property, $values); + $this->assertSame($expected, $matchedUsers); + } + + public function searchDataProvider(): array { + return [ + [ #0 Search for an existing name + IAccountManager::PROPERTY_DISPLAYNAME, + ['Jane Doe'], + ['Jane Doe' => 'j.doe'] + ], + [ #1 Search for part of a name (no result) + IAccountManager::PROPERTY_DISPLAYNAME, + ['Jane'], + [] + ], + [ #2 Search for part of a name (no result, test wildcard) + IAccountManager::PROPERTY_DISPLAYNAME, + ['Jane%'], + [] + ], + [ #3 Search for phone + IAccountManager::PROPERTY_PHONE, + ['+491603121212'], + ['+491603121212' => 'b32c5a5b-1084-4380-8856-e5223b16de9f'], + ], + [ #4 Search for twitter handles + IAccountManager::PROPERTY_TWITTER, + ['@sometwitter', '@a_alice', '@unseen'], + ['@sometwitter' => 'j.doe', '@a_alice' => 'a.allison'], + ], + [ #5 Search for email + IAccountManager::PROPERTY_EMAIL, + ['cheng@emca.com'], + ['cheng@emca.com' => '31b5316a-9b57-4b17-970a-315a4cbe73eb'], + ], + [ #6 Search for email by additional email + IAccountManager::PROPERTY_EMAIL, + ['kai.cheng@emca.com'], + ['kai.cheng@emca.com' => '31b5316a-9b57-4b17-970a-315a4cbe73eb'], + ], + [ #7 Search for additional email + IAccountManager::COLLECTION_EMAIL, + ['kai.cheng@emca.com', 'cheng@emca.com'], + ['kai.cheng@emca.com' => '31b5316a-9b57-4b17-970a-315a4cbe73eb'], + ], + [ #8 Search for email by additional email (two valid search values, but the same user) + IAccountManager::PROPERTY_EMAIL, + ['kai.cheng@emca.com', 'cheng@emca.com'], + [ + 'kai.cheng@emca.com' => '31b5316a-9b57-4b17-970a-315a4cbe73eb', + ], + ], + ]; + } } From 0bade2747902b22002203c31e7e13f36256f42bc Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 26 May 2021 18:39:43 +0200 Subject: [PATCH 05/10] add IAccountPropertyCollection with implementation Signed-off-by: Arthur Schiwon --- lib/composer/composer/autoload_classmap.php | 2 + lib/composer/composer/autoload_static.php | 2 + .../Accounts/AccountPropertyCollection.php | 86 +++++++ .../Accounts/IAccountPropertyCollection.php | 77 +++++++ .../AccountPropertyCollectionTest.php | 209 ++++++++++++++++++ 5 files changed, 376 insertions(+) create mode 100644 lib/private/Accounts/AccountPropertyCollection.php create mode 100644 lib/public/Accounts/IAccountPropertyCollection.php create mode 100644 tests/lib/Accounts/AccountPropertyCollectionTest.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index f63d74b560..0912eb5396 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -10,6 +10,7 @@ return array( 'OCP\\Accounts\\IAccount' => $baseDir . '/lib/public/Accounts/IAccount.php', 'OCP\\Accounts\\IAccountManager' => $baseDir . '/lib/public/Accounts/IAccountManager.php', 'OCP\\Accounts\\IAccountProperty' => $baseDir . '/lib/public/Accounts/IAccountProperty.php', + 'OCP\\Accounts\\IAccountPropertyCollection' => $baseDir . '/lib/public/Accounts/IAccountPropertyCollection.php', 'OCP\\Accounts\\PropertyDoesNotExistException' => $baseDir . '/lib/public/Accounts/PropertyDoesNotExistException.php', 'OCP\\Activity\\ActivitySettings' => $baseDir . '/lib/public/Activity/ActivitySettings.php', 'OCP\\Activity\\IConsumer' => $baseDir . '/lib/public/Activity/IConsumer.php', @@ -581,6 +582,7 @@ return array( 'OC\\Accounts\\Account' => $baseDir . '/lib/private/Accounts/Account.php', 'OC\\Accounts\\AccountManager' => $baseDir . '/lib/private/Accounts/AccountManager.php', 'OC\\Accounts\\AccountProperty' => $baseDir . '/lib/private/Accounts/AccountProperty.php', + 'OC\\Accounts\\AccountPropertyCollection' => $baseDir . '/lib/private/Accounts/AccountPropertyCollection.php', 'OC\\Accounts\\Hooks' => $baseDir . '/lib/private/Accounts/Hooks.php', 'OC\\Activity\\ActivitySettingsAdapter' => $baseDir . '/lib/private/Activity/ActivitySettingsAdapter.php', 'OC\\Activity\\Event' => $baseDir . '/lib/private/Activity/Event.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 9c861fd327..04adffc410 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -39,6 +39,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OCP\\Accounts\\IAccount' => __DIR__ . '/../../..' . '/lib/public/Accounts/IAccount.php', 'OCP\\Accounts\\IAccountManager' => __DIR__ . '/../../..' . '/lib/public/Accounts/IAccountManager.php', 'OCP\\Accounts\\IAccountProperty' => __DIR__ . '/../../..' . '/lib/public/Accounts/IAccountProperty.php', + 'OCP\\Accounts\\IAccountPropertyCollection' => __DIR__ . '/../../..' . '/lib/public/Accounts/IAccountPropertyCollection.php', 'OCP\\Accounts\\PropertyDoesNotExistException' => __DIR__ . '/../../..' . '/lib/public/Accounts/PropertyDoesNotExistException.php', 'OCP\\Activity\\ActivitySettings' => __DIR__ . '/../../..' . '/lib/public/Activity/ActivitySettings.php', 'OCP\\Activity\\IConsumer' => __DIR__ . '/../../..' . '/lib/public/Activity/IConsumer.php', @@ -610,6 +611,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Accounts\\Account' => __DIR__ . '/../../..' . '/lib/private/Accounts/Account.php', 'OC\\Accounts\\AccountManager' => __DIR__ . '/../../..' . '/lib/private/Accounts/AccountManager.php', 'OC\\Accounts\\AccountProperty' => __DIR__ . '/../../..' . '/lib/private/Accounts/AccountProperty.php', + 'OC\\Accounts\\AccountPropertyCollection' => __DIR__ . '/../../..' . '/lib/private/Accounts/AccountPropertyCollection.php', 'OC\\Accounts\\Hooks' => __DIR__ . '/../../..' . '/lib/private/Accounts/Hooks.php', 'OC\\Activity\\ActivitySettingsAdapter' => __DIR__ . '/../../..' . '/lib/private/Activity/ActivitySettingsAdapter.php', 'OC\\Activity\\Event' => __DIR__ . '/../../..' . '/lib/private/Activity/Event.php', diff --git a/lib/private/Accounts/AccountPropertyCollection.php b/lib/private/Accounts/AccountPropertyCollection.php new file mode 100644 index 0000000000..8e74d45d52 --- /dev/null +++ b/lib/private/Accounts/AccountPropertyCollection.php @@ -0,0 +1,86 @@ + + * + * @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 OC\Accounts; + +use InvalidArgumentException; +use OCP\Accounts\IAccountProperty; +use OCP\Accounts\IAccountPropertyCollection; + +class AccountPropertyCollection implements IAccountPropertyCollection { + + /** @var string */ + protected $collectionName = ''; + + /** @var IAccountProperty[] */ + protected $properties = []; + + public function __construct(string $collectionName) { + $this->collectionName = $collectionName; + } + + public function setProperties(array $properties): IAccountPropertyCollection { + /** @var IAccountProperty $property */ + $this->properties = []; + foreach ($properties as $property) { + $this->addProperty($property); + } + return $this; + } + + public function getProperties(): array { + return $this->properties; + } + + public function addProperty(IAccountProperty $property): IAccountPropertyCollection { + if ($property->getName() !== $this->collectionName) { + throw new InvalidArgumentException('Provided property does not match collection name'); + } + $this->properties[] = $property; + return $this; + } + + public function removeProperty(IAccountProperty $property): IAccountPropertyCollection { + $ref = array_search($property, $this->properties, true); + if ($ref !== false) { + unset($this->properties[$ref]); + } + return $this; + } + + public function removePropertyByValue(string $value): IAccountPropertyCollection { + foreach ($this->properties as $i => $property) { + if ($property->getValue() === $value) { + unset($this->properties[$i]); + } + } + return $this; + } + + public function jsonSerialize() { + return [$this->collectionName => $this->properties]; + } +} diff --git a/lib/public/Accounts/IAccountPropertyCollection.php b/lib/public/Accounts/IAccountPropertyCollection.php new file mode 100644 index 0000000000..b67fa5646f --- /dev/null +++ b/lib/public/Accounts/IAccountPropertyCollection.php @@ -0,0 +1,77 @@ + + * + * @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 OCP\Accounts; + +use InvalidArgumentException; +use JsonSerializable; + +/** + * Interface IAccountPropertyCollection + * + * @package OCP\Accounts + * + * @since 22.0.0 + */ +interface IAccountPropertyCollection extends JsonSerializable { + + /** + * set properties of this collection + * + * @param IAccountProperty[] $properties + * @throws InvalidArgumentException + * @since 22.0.0 + */ + public function setProperties(array $properties): IAccountPropertyCollection; + + /** + * @return IAccountProperty[] + * @since 22.0.0 + */ + public function getProperties(): array; + + /** + * adds a property to this collection + * + * @throws InvalidArgumentException + * @since 22.0.0 + */ + public function addProperty(IAccountProperty $property): IAccountPropertyCollection; + + /** + * removes a property of this collection + * + * @since 22.0.0 + */ + public function removeProperty(IAccountProperty $property): IAccountPropertyCollection; + + /** + * removes a property identified by its value + * + * @since 22.0.0 + */ + public function removePropertyByValue(string $value): IAccountPropertyCollection; +} diff --git a/tests/lib/Accounts/AccountPropertyCollectionTest.php b/tests/lib/Accounts/AccountPropertyCollectionTest.php new file mode 100644 index 0000000000..d8a6bafd24 --- /dev/null +++ b/tests/lib/Accounts/AccountPropertyCollectionTest.php @@ -0,0 +1,209 @@ + + * + * @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 lib\Accounts; + +use InvalidArgumentException; +use OC\Accounts\AccountPropertyCollection; +use OCP\Accounts\IAccountProperty; +use OCP\Accounts\IAccountPropertyCollection; +use PHPUnit\Framework\MockObject\MockObject; +use Test\TestCase; + +class AccountPropertyCollectionTest extends TestCase { + /** @var IAccountPropertyCollection */ + protected $collection; + + protected const COLLECTION_NAME = 'my_multivalue_property'; + + public function setUp(): void { + parent::setUp(); + + $this->collection = new AccountPropertyCollection(self::COLLECTION_NAME); + } + + /** + * @return IAccountProperty|MockObject + */ + protected function makePropertyMock(string $propertyName): MockObject { + $mock = $this->createMock(IAccountProperty::class); + $mock->expects($this->any()) + ->method('getName') + ->willReturn($propertyName); + + return $mock; + } + + public function testSetAndGetProperties() { + $propsBefore = $this->collection->getProperties(); + $this->assertIsArray($propsBefore); + $this->assertEmpty($propsBefore); + + $props = [ + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock(self::COLLECTION_NAME), + ]; + + $this->collection->setProperties($props); + $propsAfter = $this->collection->getProperties(); + $this->assertIsArray($propsAfter); + $this->assertCount(count($props), $propsAfter); + } + + public function testSetPropertiesMixedInvalid() { + $props = [ + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock('sneaky_property'), + $this->makePropertyMock(self::COLLECTION_NAME), + ]; + + $this->expectException(InvalidArgumentException::class); + $this->collection->setProperties($props); + } + + public function testAddProperty() { + $props = [ + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock(self::COLLECTION_NAME), + ]; + $this->collection->setProperties($props); + + $additionalProperty = $this->makePropertyMock(self::COLLECTION_NAME); + $this->collection->addProperty($additionalProperty); + + $propsAfter = $this->collection->getProperties(); + $this->assertCount(count($props) + 1, $propsAfter); + $this->assertNotFalse(array_search($additionalProperty, $propsAfter, true)); + } + + public function testAddPropertyInvalid() { + $props = [ + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock(self::COLLECTION_NAME), + ]; + $this->collection->setProperties($props); + + $additionalProperty = $this->makePropertyMock('sneaky_property'); + $exceptionThrown = false; + try { + $this->collection->addProperty($additionalProperty); + } catch (\InvalidArgumentException $e) { + $exceptionThrown = true; + } finally { + $propsAfter = $this->collection->getProperties(); + $this->assertCount(count($props), $propsAfter); + $this->assertFalse(array_search($additionalProperty, $propsAfter, true)); + $this->assertTrue($exceptionThrown); + } + } + + public function testRemoveProperty() { + $additionalProperty = $this->makePropertyMock(self::COLLECTION_NAME); + $props = [ + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock(self::COLLECTION_NAME), + $additionalProperty, + $this->makePropertyMock(self::COLLECTION_NAME), + ]; + $this->collection->setProperties($props); + + $propsBefore = $this->collection->getProperties(); + $this->collection->removeProperty($additionalProperty); + $propsAfter = $this->collection->getProperties(); + + $this->assertTrue(count($propsBefore) > count($propsAfter)); + $this->assertCount(count($propsBefore) - 1, $propsAfter); + $this->assertFalse(array_search($additionalProperty, $propsAfter, true)); + } + + public function testRemovePropertyNotFound() { + $additionalProperty = $this->makePropertyMock(self::COLLECTION_NAME); + $props = [ + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock(self::COLLECTION_NAME), + ]; + $this->collection->setProperties($props); + + $propsBefore = $this->collection->getProperties(); + $this->collection->removeProperty($additionalProperty); + $propsAfter = $this->collection->getProperties(); + + // no errors, gently + $this->assertCount(count($propsBefore), $propsAfter); + } + + public function testRemovePropertyByValue() { + $additionalProperty = $this->makePropertyMock(self::COLLECTION_NAME); + $additionalProperty->expects($this->any()) + ->method('getValue') + ->willReturn('Lorem ipsum'); + + $additionalPropertyTwo = clone $additionalProperty; + + $props = [ + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock(self::COLLECTION_NAME), + $additionalProperty, + $this->makePropertyMock(self::COLLECTION_NAME), + $additionalPropertyTwo + ]; + $this->collection->setProperties($props); + + $propsBefore = $this->collection->getProperties(); + $this->collection->removePropertyByValue('Lorem ipsum'); + $propsAfter = $this->collection->getProperties(); + + $this->assertTrue(count($propsBefore) > count($propsAfter)); + $this->assertCount(count($propsBefore) - 2, $propsAfter); + $this->assertFalse(array_search($additionalProperty, $propsAfter, true)); + $this->assertFalse(array_search($additionalPropertyTwo, $propsAfter, true)); + } + + public function testRemovePropertyByValueNotFound() { + $additionalProperty = $this->makePropertyMock(self::COLLECTION_NAME); + $additionalProperty->expects($this->any()) + ->method('getValue') + ->willReturn('Lorem ipsum'); + + $props = [ + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock(self::COLLECTION_NAME), + $this->makePropertyMock(self::COLLECTION_NAME), + ]; + $this->collection->setProperties($props); + + $propsBefore = $this->collection->getProperties(); + $this->collection->removePropertyByValue('Lorem ipsum'); + $propsAfter = $this->collection->getProperties(); + + // no errors, gently + $this->assertCount(count($propsBefore), $propsAfter); + } +} From 44827e37c0a2e9f69feae6a741a223bf19e49685 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 3 Jun 2021 19:53:18 +0200 Subject: [PATCH 06/10] allow interacting with IAccountPropertyCollections - in fact the API could be done in a nicer way and it might be possible to work without IAccountPropertyCollection, but only with the IAccountProperties. - To keep it simple at first and not overengineer the blunt attempt is followed - If necessary helpful in the further cause of development adjustements or extensions can be done quickly with this base Signed-off-by: Arthur Schiwon --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Accounts/Account.php | 58 +++++++++++++++++-- lib/private/Accounts/AccountManager.php | 9 +-- .../Accounts/AccountPropertyCollection.php | 4 ++ lib/private/Accounts/TAccountsHelper.php | 41 +++++++++++++ lib/public/Accounts/IAccount.php | 37 +++++++++++- .../Accounts/IAccountPropertyCollection.php | 7 +++ tests/lib/Accounts/AccountTest.php | 27 ++++++++- 9 files changed, 166 insertions(+), 19 deletions(-) create mode 100644 lib/private/Accounts/TAccountsHelper.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 0912eb5396..ef7085cd5e 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -584,6 +584,7 @@ return array( 'OC\\Accounts\\AccountProperty' => $baseDir . '/lib/private/Accounts/AccountProperty.php', 'OC\\Accounts\\AccountPropertyCollection' => $baseDir . '/lib/private/Accounts/AccountPropertyCollection.php', 'OC\\Accounts\\Hooks' => $baseDir . '/lib/private/Accounts/Hooks.php', + 'OC\\Accounts\\TAccountsHelper' => $baseDir . '/lib/private/Accounts/TAccountsHelper.php', 'OC\\Activity\\ActivitySettingsAdapter' => $baseDir . '/lib/private/Activity/ActivitySettingsAdapter.php', 'OC\\Activity\\Event' => $baseDir . '/lib/private/Activity/Event.php', 'OC\\Activity\\EventMerger' => $baseDir . '/lib/private/Activity/EventMerger.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 04adffc410..b89068acbe 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -613,6 +613,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Accounts\\AccountProperty' => __DIR__ . '/../../..' . '/lib/private/Accounts/AccountProperty.php', 'OC\\Accounts\\AccountPropertyCollection' => __DIR__ . '/../../..' . '/lib/private/Accounts/AccountPropertyCollection.php', 'OC\\Accounts\\Hooks' => __DIR__ . '/../../..' . '/lib/private/Accounts/Hooks.php', + 'OC\\Accounts\\TAccountsHelper' => __DIR__ . '/../../..' . '/lib/private/Accounts/TAccountsHelper.php', 'OC\\Activity\\ActivitySettingsAdapter' => __DIR__ . '/../../..' . '/lib/private/Activity/ActivitySettingsAdapter.php', 'OC\\Activity\\Event' => __DIR__ . '/../../..' . '/lib/private/Activity/Event.php', 'OC\\Activity\\EventMerger' => __DIR__ . '/../../..' . '/lib/private/Activity/EventMerger.php', diff --git a/lib/private/Accounts/Account.php b/lib/private/Accounts/Account.php index 109691641f..ff9b78dae4 100644 --- a/lib/private/Accounts/Account.php +++ b/lib/private/Accounts/Account.php @@ -27,12 +27,15 @@ declare(strict_types=1); namespace OC\Accounts; +use Generator; use OCP\Accounts\IAccount; use OCP\Accounts\IAccountProperty; +use OCP\Accounts\IAccountPropertyCollection; use OCP\Accounts\PropertyDoesNotExistException; use OCP\IUser; class Account implements IAccount { + use TAccountsHelper; /** @var IAccountProperty[] */ private $properties = []; @@ -45,11 +48,17 @@ class Account implements IAccount { } public function setProperty(string $property, string $value, string $scope, string $verified, string $verificationData = ''): IAccount { + if ($this->isCollection($property)) { + throw new \InvalidArgumentException('setProperty cannot set an IAccountsPropertyCollection'); + } $this->properties[$property] = new AccountProperty($property, $value, $scope, $verified, $verificationData); return $this; } public function getProperty(string $property): IAccountProperty { + if ($this->isCollection($property)) { + throw new \InvalidArgumentException('getProperty cannot retrieve an IAccountsPropertyCollection'); + } if (!array_key_exists($property, $this->properties)) { throw new PropertyDoesNotExistException($property); } @@ -57,19 +66,41 @@ class Account implements IAccount { } public function getProperties(): array { - return $this->properties; + return array_filter($this->properties, function ($obj) { + return $obj instanceof IAccountProperty; + }); + } + + public function getAllProperties(): Generator { + foreach ($this->properties as $propertyObject) { + if ($propertyObject instanceof IAccountProperty) { + yield $propertyObject; + } else if ($propertyObject instanceof IAccountPropertyCollection) { + foreach ($propertyObject->getProperties() as $property) { + yield $property; + } + } + } } public function getFilteredProperties(string $scope = null, string $verified = null): array { - return \array_filter($this->properties, function (IAccountProperty $obj) use ($scope, $verified) { + $result = $incrementals = []; + /** @var IAccountProperty $obj */ + foreach ($this->getAllProperties() as $obj) { if ($scope !== null && $scope !== $obj->getScope()) { - return false; + continue; } if ($verified !== null && $verified !== $obj->getVerified()) { - return false; + continue; } - return true; - }); + $index = $obj->getName(); + if ($this->isCollection($index)) { + $incrementals[$index] = ($incrementals[$index] ?? -1) + 1; + $index .= '#' . $incrementals[$index]; + } + $result[$index] = $obj; + } + return $result; } public function jsonSerialize() { @@ -79,4 +110,19 @@ class Account implements IAccount { public function getUser(): IUser { return $this->user; } + + public function setPropertyCollection(IAccountPropertyCollection $propertyCollection): IAccount { + $this->properties[$propertyCollection->getName()] = $propertyCollection; + return $this; + } + + public function getPropertyCollection(string $propertyCollection): IAccountPropertyCollection { + if (!array_key_exists($propertyCollection, $this->properties)) { + throw new PropertyDoesNotExistException($propertyCollection); + } + if (!$this->properties[$propertyCollection] instanceof IAccountPropertyCollection) { + throw new \RuntimeException('Requested collection is not an IAccountPropertyCollection'); + } + return $this->properties[$propertyCollection]; + } } diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index 3241ad27c6..3b3117d8be 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -58,6 +58,7 @@ use function json_last_error; * @package OC\Accounts */ class AccountManager implements IAccountManager { + use TAccountsHelper; /** @var IDBConnection database connection */ private $connection; @@ -607,12 +608,4 @@ class AccountManager implements IAccountManager { $this->updateUser($account->getUser(), $data, true); } - - protected function isCollection(string $propertyName): bool { - return in_array($propertyName, - [ - IAccountManager::COLLECTION_EMAIL, - ] - ); - } } diff --git a/lib/private/Accounts/AccountPropertyCollection.php b/lib/private/Accounts/AccountPropertyCollection.php index 8e74d45d52..84e10e6a50 100644 --- a/lib/private/Accounts/AccountPropertyCollection.php +++ b/lib/private/Accounts/AccountPropertyCollection.php @@ -83,4 +83,8 @@ class AccountPropertyCollection implements IAccountPropertyCollection { public function jsonSerialize() { return [$this->collectionName => $this->properties]; } + + public function getName(): string { + return $this->collectionName; + } } diff --git a/lib/private/Accounts/TAccountsHelper.php b/lib/private/Accounts/TAccountsHelper.php new file mode 100644 index 0000000000..09cc44c87c --- /dev/null +++ b/lib/private/Accounts/TAccountsHelper.php @@ -0,0 +1,41 @@ + + * + * @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 OC\Accounts; + + +use OCP\Accounts\IAccountManager; + +trait TAccountsHelper { + protected function isCollection(string $propertyName): bool { + return in_array($propertyName, + [ + IAccountManager::COLLECTION_EMAIL, + ], + true + ); + } +} diff --git a/lib/public/Accounts/IAccount.php b/lib/public/Accounts/IAccount.php index 7397eb4fea..1d882b5515 100644 --- a/lib/public/Accounts/IAccount.php +++ b/lib/public/Accounts/IAccount.php @@ -26,6 +26,7 @@ declare(strict_types=1); namespace OCP\Accounts; +use Generator; use OCP\IUser; /** @@ -62,16 +63,48 @@ interface IAccount extends \JsonSerializable { /** * Get all properties of an account. Array indices are property names. + * Values from IAccountPropertyCollections are not included in the return + * array. * * @since 15.0.0 - * - * @return IAccountProperty[] + * @deprecated 22.0.0 use getAllProperties() */ public function getProperties(): array; + /** + * Get all properties of an account. Array indices are numeric. To get + * the property name, call getName() against the value. + * + * IAccountPropertyCollections are being flattened into an IAccountProperty + * for each value. + * + * @since 22.0.0 + * + * @return Generator + */ + public function getAllProperties(): Generator; + + /** + * Set a property collection (multi-value properties) + * + * @since 22.0.0 + */ + public function setPropertyCollection(IAccountPropertyCollection $propertyCollection): IAccount; + + /** + * Returns the requestes propery collection (multi-value properties) + * + * @since 22.0.0 + */ + public function getPropertyCollection(string $propertyCollection): IAccountPropertyCollection; + /** * Get all properties that match the provided filters for scope and verification status * + * Since 22.0.0 values from IAccountPropertyCollection are included, but also + * as IAccountProperty instances. They for properties of IAccountPropertyCollection are + * suffixed incrementally, i.e. #0, #1 ... #n – the numbers have no further meaning. + * * @since 15.0.0 * * @param string $scope Must be one of the VISIBILITY_ prefixed constants of \OCP\Accounts\IAccountManager diff --git a/lib/public/Accounts/IAccountPropertyCollection.php b/lib/public/Accounts/IAccountPropertyCollection.php index b67fa5646f..9e026f4ce5 100644 --- a/lib/public/Accounts/IAccountPropertyCollection.php +++ b/lib/public/Accounts/IAccountPropertyCollection.php @@ -38,6 +38,13 @@ use JsonSerializable; */ interface IAccountPropertyCollection extends JsonSerializable { + /** + * retuns the collection name + * + * @since 22.0.0 + */ + public function getName(): string; + /** * set properties of this collection * diff --git a/tests/lib/Accounts/AccountTest.php b/tests/lib/Accounts/AccountTest.php index 9c2a5333d2..0e0c42804e 100644 --- a/tests/lib/Accounts/AccountTest.php +++ b/tests/lib/Accounts/AccountTest.php @@ -25,6 +25,7 @@ namespace Test\Accounts; use OC\Accounts\Account; use OC\Accounts\AccountProperty; +use OC\Accounts\AccountPropertyCollection; use OCP\Accounts\IAccountManager; use OCP\IUser; use Test\TestCase; @@ -49,7 +50,7 @@ class AccountTest extends TestCase { $this->assertEquals($property, $account->getProperty(IAccountManager::PROPERTY_WEBSITE)); } - public function testGetProperties() { + public function testGetAndGetAllProperties() { $user = $this->createMock(IUser::class); $properties = [ IAccountManager::PROPERTY_WEBSITE => new AccountProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED, ''), @@ -59,7 +60,14 @@ class AccountTest extends TestCase { $account->setProperty(IAccountManager::PROPERTY_WEBSITE, 'https://example.com', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED); $account->setProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED); + $col = new AccountPropertyCollection(IAccountManager::COLLECTION_EMAIL); + $additionalProperty = new AccountProperty($col->getName(), 'second@example.org', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED, ''); + $col->addProperty($additionalProperty); + $account->setPropertyCollection($col); + $this->assertEquals($properties, $account->getProperties()); + $properties[] = $additionalProperty; + $this->assertEquals(array_values($properties), \iterator_to_array($account->getAllProperties())); } public function testGetFilteredProperties() { @@ -74,11 +82,20 @@ class AccountTest extends TestCase { $account->setProperty(IAccountManager::PROPERTY_EMAIL, 'user@example.com', IAccountManager::SCOPE_LOCAL, IAccountManager::VERIFIED); $account->setProperty(IAccountManager::PROPERTY_PHONE, '123456', IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED); + $col = new AccountPropertyCollection(IAccountManager::COLLECTION_EMAIL); + $additionalProperty1 = new AccountProperty($col->getName(), 'second@example.org', IAccountManager::SCOPE_PUBLISHED, IAccountManager::NOT_VERIFIED, ''); + $additionalProperty2 = new AccountProperty($col->getName(), 'third@example.org', IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED, ''); + $col->addProperty($additionalProperty1); + $col->addProperty($additionalProperty2); + $account->setPropertyCollection($col); + $this->assertEquals( [ IAccountManager::PROPERTY_WEBSITE => $properties[IAccountManager::PROPERTY_WEBSITE], IAccountManager::PROPERTY_PHONE => $properties[IAccountManager::PROPERTY_PHONE], + IAccountManager::COLLECTION_EMAIL . '#0' => $additionalProperty1, + IAccountManager::COLLECTION_EMAIL . '#1' => $additionalProperty2, ], $account->getFilteredProperties(IAccountManager::SCOPE_PUBLISHED) ); @@ -86,12 +103,16 @@ class AccountTest extends TestCase { [ IAccountManager::PROPERTY_EMAIL => $properties[IAccountManager::PROPERTY_EMAIL], IAccountManager::PROPERTY_PHONE => $properties[IAccountManager::PROPERTY_PHONE], + IAccountManager::COLLECTION_EMAIL . '#0' => $additionalProperty2, ], $account->getFilteredProperties(null, IAccountManager::VERIFIED) ); $this->assertEquals( - [IAccountManager::PROPERTY_PHONE => $properties[IAccountManager::PROPERTY_PHONE]], - $account->getFilteredProperties(IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED) + [ + IAccountManager::PROPERTY_PHONE => $properties[IAccountManager::PROPERTY_PHONE], + IAccountManager::COLLECTION_EMAIL . '#0' => $additionalProperty2, + ], + $account->getFilteredProperties(IAccountManager::SCOPE_PUBLISHED, IAccountManager::VERIFIED), ); } From 2701c3e7dcfc8cd6bd8c094f8df0562d1c3f9679 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 3 Jun 2021 20:54:25 +0200 Subject: [PATCH 07/10] fix code style Signed-off-by: Arthur Schiwon --- lib/private/Accounts/Account.php | 2 +- lib/private/Accounts/AccountManager.php | 2 +- lib/private/Accounts/TAccountsHelper.php | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/private/Accounts/Account.php b/lib/private/Accounts/Account.php index ff9b78dae4..dd07a930a8 100644 --- a/lib/private/Accounts/Account.php +++ b/lib/private/Accounts/Account.php @@ -75,7 +75,7 @@ class Account implements IAccount { foreach ($this->properties as $propertyObject) { if ($propertyObject instanceof IAccountProperty) { yield $propertyObject; - } else if ($propertyObject instanceof IAccountPropertyCollection) { + } elseif ($propertyObject instanceof IAccountPropertyCollection) { foreach ($propertyObject->getProperties() as $property) { yield $property; } diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index 3b3117d8be..801250004f 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -169,7 +169,7 @@ class AccountManager implements IAccountManager { foreach ($data as $propertyNameOrIndex => &$propertyData) { if ($this->isCollection($propertyNameOrIndex)) { $this->testPropertyScopes($propertyData, $allowedScopes, $throwOnData); - } else if (isset($propertyData['scope'])) { + } elseif (isset($propertyData['scope'])) { $effectivePropertyName = $parentPropertyName ?? $propertyNameOrIndex; if ($throwOnData && !in_array($propertyData['scope'], $allowedScopes, true)) { diff --git a/lib/private/Accounts/TAccountsHelper.php b/lib/private/Accounts/TAccountsHelper.php index 09cc44c87c..530204b451 100644 --- a/lib/private/Accounts/TAccountsHelper.php +++ b/lib/private/Accounts/TAccountsHelper.php @@ -26,7 +26,6 @@ declare(strict_types=1); namespace OC\Accounts; - use OCP\Accounts\IAccountManager; trait TAccountsHelper { From 8ec640d14ac6dec7c6f52ac8bca997615931478b Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 3 Jun 2021 21:07:59 +0200 Subject: [PATCH 08/10] adjust property type declaration Signed-off-by: Arthur Schiwon --- lib/private/Accounts/Account.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Accounts/Account.php b/lib/private/Accounts/Account.php index dd07a930a8..5f063d5dc5 100644 --- a/lib/private/Accounts/Account.php +++ b/lib/private/Accounts/Account.php @@ -37,7 +37,7 @@ use OCP\IUser; class Account implements IAccount { use TAccountsHelper; - /** @var IAccountProperty[] */ + /** @var IAccountPropertyCollection[]|IAccountProperty[] */ private $properties = []; /** @var IUser */ From 8235d90ecc303c00912d0ce18c13748190ece57e Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 3 Jun 2021 21:55:57 +0200 Subject: [PATCH 09/10] psalm happiness Signed-off-by: Arthur Schiwon --- build/psalm-baseline.xml | 3 --- lib/private/Accounts/AccountManager.php | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index fc6c553d0c..e95c067701 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -1769,9 +1769,6 @@ - - $publicData[IAccountManager::PROPERTY_DISPLAYNAME]['value'] - $this->retries + 1 diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index 801250004f..4d7aae644b 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -142,7 +142,7 @@ class AccountManager implements IAccountManager { } protected function sanitizeLength(array &$propertyData, bool $throwOnData = false): void { - if (isset($propertyData) && isset($propertyData['value']) && strlen($propertyData['value']) > 2048) { + if (isset($propertyData['value']) && strlen($propertyData['value']) > 2048) { if ($throwOnData) { throw new \InvalidArgumentException(); } else { From e4e8e36af17f4300f802aa113b7be5a25a66d1c4 Mon Sep 17 00:00:00 2001 From: Christopher Ng Date: Fri, 4 Jun 2021 02:46:39 +0000 Subject: [PATCH 10/10] [WIP] Vuetiful email section Signed-off-by: Christopher Ng --- .../components/PersonalInfo/EmailSection.vue | 108 ++++++++++++++++++ apps/settings/src/main-personal-info.js | 39 +++++++ .../settings/personal/personal.info.php | 4 + apps/settings/webpack.js | 1 + 4 files changed, 152 insertions(+) create mode 100644 apps/settings/src/components/PersonalInfo/EmailSection.vue create mode 100644 apps/settings/src/main-personal-info.js diff --git a/apps/settings/src/components/PersonalInfo/EmailSection.vue b/apps/settings/src/components/PersonalInfo/EmailSection.vue new file mode 100644 index 0000000000..733d876379 --- /dev/null +++ b/apps/settings/src/components/PersonalInfo/EmailSection.vue @@ -0,0 +1,108 @@ + + + + + + + diff --git a/apps/settings/src/main-personal-info.js b/apps/settings/src/main-personal-info.js new file mode 100644 index 0000000000..c8e37056d9 --- /dev/null +++ b/apps/settings/src/main-personal-info.js @@ -0,0 +1,39 @@ +/** + * @copyright 2021, Christopher Ng + * + * @author Christopher Ng + * + * @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 . + * + */ + +import Vue from 'vue' +import { loadState } from '@nextcloud/initial-state' + +import EmailSection from './components/PersonalInfo/EmailSection' + +// eslint-disable-next-line camelcase +__webpack_nonce__ = btoa(OC.requestToken) + +Vue.prototype.t = t + +const View = Vue.extend(EmailSection) +new View({ + propsData: { + initialEmails: loadState('settings', 'emails'), + // ...more initial props + }, +}).$mount('#vue-emailform') diff --git a/apps/settings/templates/settings/personal/personal.info.php b/apps/settings/templates/settings/personal/personal.info.php index 6f8516e643..8c6d37890b 100644 --- a/apps/settings/templates/settings/personal/personal.info.php +++ b/apps/settings/templates/settings/personal/personal.info.php @@ -31,6 +31,7 @@ script('settings', [ 'federationsettingsview', 'federationscopemenu', 'settings/personalInfo', + 'vue-settings-personal-info', ]); ?> @@ -125,6 +126,9 @@ script('settings', [ +
+
+

diff --git a/apps/settings/webpack.js b/apps/settings/webpack.js index 756a748ae1..12ac464fed 100644 --- a/apps/settings/webpack.js +++ b/apps/settings/webpack.js @@ -32,6 +32,7 @@ module.exports = { 'settings-personal-security': path.join(__dirname, 'src', 'main-personal-security'), 'settings-personal-webauthn': path.join(__dirname, 'src', 'main-personal-webauth'), 'settings-nextcloud-pdf': path.join(__dirname, 'src', 'main-nextcloud-pdf'), + 'settings-personal-info': path.join(__dirname, 'src', 'main-personal-info'), }, output: { path: path.resolve(__dirname, './js'),