Skip to content

Extend Accounts with multivalue properties (PropertyCollection) #27189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
blizzz merged 10 commits into from
Jun 8, 2021
Merged

Extend Accounts with multivalue properties (PropertyCollection) #27189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fb79350
extend AccountManager API with collection const
blizzz May 7, 2021
956bfba
refactor validators and sanitizers
blizzz May 17, 2021
839bff1
deal with property collections when fetching users (with update)
blizzz May 17, 2021
afea573
update unit tests
blizzz May 17, 2021
0bade27
add IAccountPropertyCollection with implementation
blizzz May 26, 2021
44827e3
allow interacting with IAccountPropertyCollections
blizzz Jun 3, 2021
2701c3e
fix code style
blizzz Jun 3, 2021
8ec640d
adjust property type declaration
blizzz Jun 3, 2021
35d5395
psalm happiness
blizzz Jun 3, 2021
ff2382e
CI: more flexible assertions
blizzz Jun 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions build/psalm-baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1769,9 +1769,6 @@
</TooManyArguments>
</file>
<file src="apps/lookup_server_connector/lib/BackgroundJobs/RetryJob.php">
<InvalidArrayOffset occurrences="1">
<code>$publicData[IAccountManager::PROPERTY_DISPLAYNAME]['value']</code>
</InvalidArrayOffset>
<InvalidScalarArgument occurrences="1">
<code>$this-&gt;retries + 1</code>
</InvalidScalarArgument>
Expand Down
3 changes: 3 additions & 0 deletions lib/composer/composer/autoload_classmap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
'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',
Expand Down Expand Up @@ -581,7 +582,9 @@
'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\\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',
Expand Down
3 changes: 3 additions & 0 deletions lib/composer/composer/autoload_static.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -610,7 +611,9 @@ 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\\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',
Expand Down
62 changes: 54 additions & 8 deletions lib/private/Accounts/Account.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,17 @@

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[] */
/** @var IAccountPropertyCollection[]|IAccountProperty[] */
private $properties = [];

/** @var IUser */
Expand All @@ -45,31 +48,59 @@ public function __construct(IUser $user) {
}

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 (!array_key_exists($property, $this->properties)) {
if ($this->isCollection($property)) {
throw new \InvalidArgumentException('getProperty cannot retrieve an IAccountsPropertyCollection');
}
if (!array_key_exists($property, $this->properties) || !$this->properties[$property] instanceof IAccountProperty) {
throw new PropertyDoesNotExistException($property);
}
return $this->properties[$property];
}

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;
} elseif ($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() {
Expand All @@ -79,4 +110,19 @@ public function jsonSerialize() {
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];
}
}
138 changes: 89 additions & 49 deletions lib/private/Accounts/AccountManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
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;

Expand All @@ -57,6 +58,7 @@
* @package OC\Accounts
*/
class AccountManager implements IAccountManager {
use TAccountsHelper;

/** @var IDBConnection database connection */
private $connection;
Expand Down Expand Up @@ -139,6 +141,61 @@ protected function parseWebsite(string $input): string {
return $input;
}

protected function sanitizeLength(array &$propertyData, bool $throwOnData = false): void {
if (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);
} elseif (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
*
Expand Down Expand Up @@ -166,16 +223,7 @@ public function updateUser(IUser $user, array $data, bool $throwOnData = false):
}
}

// 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 {
Expand All @@ -198,31 +246,7 @@ public function updateUser(IUser $user, array $data, bool $throwOnData = false):
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);
Expand Down Expand Up @@ -276,12 +300,9 @@ public function deleteUserData(IUser $user): void {
/**
* 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')
Expand All @@ -294,7 +315,9 @@ public function getUser(IUser $user) {

if (empty($accountData)) {
$userData = $this->buildDefaultUserRecord($user);
$this->insertNewUser($user, $userData);
if ($insertIfNotExists) {
$this->insertNewUser($user, $userData);
}
return $userData;
}

Expand All @@ -305,9 +328,7 @@ public function getUser(IUser $user) {
return $this->buildDefaultUserRecord($user);
}

$userDataArray = $this->addMissingDefaultValues($userDataArray);

return $userDataArray;
return $this->addMissingDefaultValues($userDataArray);
}

public function searchUsers(string $property, array $values): array {
Expand All @@ -324,12 +345,23 @@ public function searchUsers(string $property, array $values): array {
$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 [];
}
}

/**
Expand All @@ -340,7 +372,7 @@ public function searchUsers(string $property, array $values): array {
* @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,
[
Expand Down Expand Up @@ -381,7 +413,7 @@ protected function addMissingDefaultValues(array $userData) {
* @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;
Expand Down Expand Up @@ -481,12 +513,20 @@ protected function writeUserData(IUser $user, array $data): void {
'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();
}
Expand Down
Loading