diff --git a/apps/provisioning_api/appinfo/routes.php b/apps/provisioning_api/appinfo/routes.php index 8cf57487d3aba..77dcda0d95560 100644 --- a/apps/provisioning_api/appinfo/routes.php +++ b/apps/provisioning_api/appinfo/routes.php @@ -54,6 +54,7 @@ ['root' => '/cloud', 'name' => 'Users#getEditableFields', 'url' => '/user/fields', 'verb' => 'GET'], ['root' => '/cloud', 'name' => 'Users#getEditableFieldsForUser', 'url' => '/user/fields/{userId}', 'verb' => 'GET'], ['root' => '/cloud', 'name' => 'Users#editUser', 'url' => '/users/{userId}', 'verb' => 'PUT'], + ['root' => '/cloud', 'name' => 'Users#editUserMultiValue', 'url' => '/users/{userId}/{collectionName}', 'verb' => 'PUT', 'requirements' => ['collectionName' => '^(?!enable$|disable$)[a-zA-Z0-0_]*$']], ['root' => '/cloud', 'name' => 'Users#wipeUserDevices', 'url' => '/users/{userId}/wipe', 'verb' => 'POST'], ['root' => '/cloud', 'name' => 'Users#deleteUser', 'url' => '/users/{userId}', 'verb' => 'DELETE'], ['root' => '/cloud', 'name' => 'Users#enableUser', 'url' => '/users/{userId}/enable', 'verb' => 'PUT'], diff --git a/apps/provisioning_api/lib/Controller/AUserData.php b/apps/provisioning_api/lib/Controller/AUserData.php index c4fa537c2dff7..e358d28206186 100644 --- a/apps/provisioning_api/lib/Controller/AUserData.php +++ b/apps/provisioning_api/lib/Controller/AUserData.php @@ -150,6 +150,20 @@ protected function getUserData(string $userId, bool $includeScopes = false): arr if ($includeScopes) { $data[IAccountManager::PROPERTY_EMAIL . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getScope(); } + + $additionalEmails = $additionalEmailScopes = []; + $emailCollection = $userAccount->getPropertyCollection(IAccountManager::COLLECTION_EMAIL); + foreach ($emailCollection->getProperties() as $property) { + $additionalEmails[] = $property->getValue(); + if ($includeScopes) { + $additionalEmailScopes[] = $property->getScope(); + } + } + $data[IAccountManager::COLLECTION_EMAIL] = $additionalEmails; + if ($includeScopes) { + $data[IAccountManager::COLLECTION_EMAIL . self::SCOPE_SUFFIX] = $additionalEmailScopes; + } + $data[IAccountManager::PROPERTY_DISPLAYNAME] = $targetUserObject->getDisplayName(); if ($includeScopes) { $data[IAccountManager::PROPERTY_DISPLAYNAME . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_DISPLAYNAME)->getScope(); diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 256077e9ae99b..d94e8fed8a8a9 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -52,7 +52,8 @@ use OC\User\Backend; use OCA\Settings\Mailer\NewUserMailHelper; use OCP\Accounts\IAccountManager; -use OCP\App\IAppManager; +use OCP\Accounts\IAccountProperty; +use OCP\Accounts\PropertyDoesNotExistException; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSException; @@ -75,8 +76,6 @@ class UsersController extends AUserData { - /** @var IAppManager */ - private $appManager; /** @var IURLGenerator */ protected $urlGenerator; /** @var LoggerInterface */ @@ -98,7 +97,6 @@ public function __construct(string $appName, IRequest $request, IUserManager $userManager, IConfig $config, - IAppManager $appManager, IGroupManager $groupManager, IUserSession $userSession, IAccountManager $accountManager, @@ -119,7 +117,6 @@ public function __construct(string $appName, $accountManager, $l10nFactory); - $this->appManager = $appManager; $this->urlGenerator = $urlGenerator; $this->logger = $logger; $this->l10nFactory = $l10nFactory; @@ -592,6 +589,7 @@ public function getEditableFieldsForUser(string $userId): DataResponse { $permittedFields[] = IAccountManager::PROPERTY_EMAIL; } + $permittedFields[] = IAccountManager::COLLECTION_EMAIL; $permittedFields[] = IAccountManager::PROPERTY_PHONE; $permittedFields[] = IAccountManager::PROPERTY_ADDRESS; $permittedFields[] = IAccountManager::PROPERTY_WEBSITE; @@ -600,6 +598,92 @@ public function getEditableFieldsForUser(string $userId): DataResponse { return new DataResponse($permittedFields); } + /** + * @NoAdminRequired + * @NoSubAdminRequired + * @PasswordConfirmationRequired + * + * @throws OCSException + */ + public function editUserMultiValue( + string $userId, + string $collectionName, + string $key, + string $value + ): DataResponse { + $currentLoggedInUser = $this->userSession->getUser(); + if ($currentLoggedInUser === null) { + throw new OCSException('', OCSController::RESPOND_UNAUTHORISED); + } + + $targetUser = $this->userManager->get($userId); + if ($targetUser === null) { + throw new OCSException('', OCSController::RESPOND_NOT_FOUND); + } + + $permittedFields = []; + if ($targetUser->getUID() === $currentLoggedInUser->getUID()) { + // Editing self (display, email) + $permittedFields[] = IAccountManager::COLLECTION_EMAIL; + $permittedFields[] = IAccountManager::COLLECTION_EMAIL . self::SCOPE_SUFFIX; + } else { + // Check if admin / subadmin + $subAdminManager = $this->groupManager->getSubAdmin(); + if ($this->groupManager->isAdmin($currentLoggedInUser->getUID()) + || $subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) { + // They have permissions over the user + + $permittedFields[] = IAccountManager::COLLECTION_EMAIL; + } else { + // No rights + throw new OCSException('', OCSController::RESPOND_NOT_FOUND); + } + } + + // Check if permitted to edit this field + if (!in_array($collectionName, $permittedFields)) { + throw new OCSException('', 103); + } + + switch ($collectionName) { + case IAccountManager::COLLECTION_EMAIL: + $userAccount = $this->accountManager->getAccount($targetUser); + $mailCollection = $userAccount->getPropertyCollection(IAccountManager::COLLECTION_EMAIL); + $mailCollection->removePropertyByValue($key); + if ($value !== '') { + $mailCollection->addPropertyWithDefaults($value); + } + $this->accountManager->updateAccount($userAccount); + break; + + case IAccountManager::COLLECTION_EMAIL . self::SCOPE_SUFFIX: + $userAccount = $this->accountManager->getAccount($targetUser); + $mailCollection = $userAccount->getPropertyCollection(IAccountManager::COLLECTION_EMAIL); + $targetProperty = null; + foreach ($mailCollection->getProperties() as $property) { + if ($property->getValue() === $key) { + $targetProperty = $property; + break; + } + } + if ($targetProperty instanceof IAccountProperty) { + try { + $targetProperty->setScope($value); + $this->accountManager->updateAccount($userAccount); + } catch (\InvalidArgumentException $e) { + throw new OCSException('', 102); + } + } else { + throw new OCSException('', 102); + } + break; + + default: + throw new OCSException('', 103); + } + return new DataResponse(); + } + /** * @NoAdminRequired * @NoSubAdminRequired @@ -636,6 +720,8 @@ public function editUser(string $userId, string $key, string $value): DataRespon $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME . self::SCOPE_SUFFIX; $permittedFields[] = IAccountManager::PROPERTY_EMAIL . self::SCOPE_SUFFIX; + $permittedFields[] = IAccountManager::COLLECTION_EMAIL; + $permittedFields[] = 'password'; if ($this->config->getSystemValue('force_language', false) === false || $this->groupManager->isAdmin($currentLoggedInUser->getUID())) { @@ -674,6 +760,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; } $permittedFields[] = IAccountManager::PROPERTY_EMAIL; + $permittedFields[] = IAccountManager::COLLECTION_EMAIL; $permittedFields[] = 'password'; $permittedFields[] = 'language'; $permittedFields[] = 'locale'; @@ -746,24 +833,42 @@ public function editUser(string $userId, string $key, string $value): DataRespon throw new OCSException('', 102); } break; + case IAccountManager::COLLECTION_EMAIL: + if (filter_var($value, FILTER_VALIDATE_EMAIL) && $value !== $targetUser->getEMailAddress()) { + $userAccount = $this->accountManager->getAccount($targetUser); + $mailCollection = $userAccount->getPropertyCollection(IAccountManager::COLLECTION_EMAIL); + foreach ($mailCollection->getProperties() as $property) { + if ($property->getValue() === $value) { + break; + } + } + $mailCollection->addPropertyWithDefaults($value); + $this->accountManager->updateAccount($userAccount); + } else { + throw new OCSException('', 102); + } + break; case IAccountManager::PROPERTY_PHONE: case IAccountManager::PROPERTY_ADDRESS: case IAccountManager::PROPERTY_WEBSITE: case IAccountManager::PROPERTY_TWITTER: $userAccount = $this->accountManager->getAccount($targetUser); - $userProperty = $userAccount->getProperty($key); - if ($userProperty->getValue() !== $value) { - try { - $userProperty->setValue($value); - $this->accountManager->updateAccount($userAccount); - - if ($userProperty->getName() === IAccountManager::PROPERTY_PHONE) { - $this->knownUserService->deleteByContactUserId($targetUser->getUID()); + try { + $userProperty = $userAccount->getProperty($key); + if ($userProperty->getValue() !== $value) { + try { + $userProperty->setValue($value); + if ($userProperty->getName() === IAccountManager::PROPERTY_PHONE) { + $this->knownUserService->deleteByContactUserId($targetUser->getUID()); + } + } catch (\InvalidArgumentException $e) { + throw new OCSException('Invalid ' . $e->getMessage(), 102); } - } catch (\InvalidArgumentException $e) { - throw new OCSException('Invalid ' . $e->getMessage(), 102); } + } catch (PropertyDoesNotExistException $e) { + $userAccount->setProperty($key, $value, IAccountManager::SCOPE_PRIVATE, IAccountManager::NOT_VERIFIED); } + $this->accountManager->updateAccount($userAccount); break; case IAccountManager::PROPERTY_DISPLAYNAME . self::SCOPE_SUFFIX: case IAccountManager::PROPERTY_EMAIL . self::SCOPE_SUFFIX: diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 742335a919ae8..238bac3430735 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -50,7 +50,6 @@ use OCP\Accounts\IAccount; use OCP\Accounts\IAccountManager; use OCP\Accounts\IAccountProperty; -use OCP\App\IAppManager; use OCP\AppFramework\Http\DataResponse; use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; @@ -77,8 +76,6 @@ class UsersControllerTest extends TestCase { protected $userManager; /** @var IConfig|MockObject */ protected $config; - /** @var IAppManager|MockObject */ - protected $appManager; /** @var Manager|MockObject */ protected $groupManager; /** @var IUserSession|MockObject */ @@ -111,7 +108,6 @@ protected function setUp(): void { $this->userManager = $this->createMock(IUserManager::class); $this->config = $this->createMock(IConfig::class); - $this->appManager = $this->createMock(IAppManager::class); $this->groupManager = $this->createMock(Manager::class); $this->userSession = $this->createMock(IUserSession::class); $this->logger = $this->createMock(LoggerInterface::class); @@ -131,7 +127,6 @@ protected function setUp(): void { $this->request, $this->userManager, $this->config, - $this->appManager, $this->groupManager, $this->userSession, $this->accountManager, @@ -395,7 +390,6 @@ public function testAddUserSuccessfulWithDisplayName() { $this->request, $this->userManager, $this->config, - $this->appManager, $this->groupManager, $this->userSession, $this->accountManager, @@ -1071,7 +1065,8 @@ public function testGetUserDataAsAdmin() { 'backendCapabilities' => [ 'setDisplayName' => true, 'setPassword' => true, - ] + ], + 'additional_mail' => [], ]; $this->assertEquals($expected, $this->invokePrivate($this->api, 'getUserData', ['UID'])); } @@ -1198,7 +1193,8 @@ public function testGetUserDataAsSubAdminAndUserIsAccessible() { 'backendCapabilities' => [ 'setDisplayName' => true, 'setPassword' => true, - ] + ], + 'additional_mail' => [], ]; $this->assertEquals($expected, $this->invokePrivate($this->api, 'getUserData', ['UID'])); } @@ -1363,7 +1359,8 @@ public function testGetUserDataAsSubAdminSelfLookup() { 'backendCapabilities' => [ 'setDisplayName' => false, 'setPassword' => false, - ] + ], + 'additional_mail' => [], ]; $this->assertEquals($expected, $this->invokePrivate($this->api, 'getUserData', ['UID'])); } @@ -3437,7 +3434,6 @@ public function testGetCurrentUserLoggedIn() { $this->request, $this->userManager, $this->config, - $this->appManager, $this->groupManager, $this->userSession, $this->accountManager, @@ -3510,7 +3506,6 @@ public function testGetUser() { $this->request, $this->userManager, $this->config, - $this->appManager, $this->groupManager, $this->userSession, $this->accountManager, @@ -3848,6 +3843,7 @@ public function testResendWelcomeMessageFailed() { public function dataGetEditableFields() { return [ [false, ISetDisplayNameBackend::class, [ + IAccountManager::COLLECTION_EMAIL, IAccountManager::PROPERTY_PHONE, IAccountManager::PROPERTY_ADDRESS, IAccountManager::PROPERTY_WEBSITE, @@ -3856,6 +3852,7 @@ public function dataGetEditableFields() { [true, ISetDisplayNameBackend::class, [ IAccountManager::PROPERTY_DISPLAYNAME, IAccountManager::PROPERTY_EMAIL, + IAccountManager::COLLECTION_EMAIL, IAccountManager::PROPERTY_PHONE, IAccountManager::PROPERTY_ADDRESS, IAccountManager::PROPERTY_WEBSITE, @@ -3863,6 +3860,7 @@ public function dataGetEditableFields() { ]], [true, UserInterface::class, [ IAccountManager::PROPERTY_EMAIL, + IAccountManager::COLLECTION_EMAIL, IAccountManager::PROPERTY_PHONE, IAccountManager::PROPERTY_ADDRESS, IAccountManager::PROPERTY_WEBSITE, diff --git a/build/integration/features/bootstrap/Provisioning.php b/build/integration/features/bootstrap/Provisioning.php index 8eab793d66bb1..e51339c081e02 100644 --- a/build/integration/features/bootstrap/Provisioning.php +++ b/build/integration/features/bootstrap/Provisioning.php @@ -168,14 +168,19 @@ public function userHasSetting($user, $settings) { $response = $client->get($fullUrl, $options); foreach ($settings->getRows() as $setting) { $value = json_decode(json_encode(simplexml_load_string($response->getBody())->data->{$setting[0]}), 1); - if (isset($value[0])) { + if (isset($value['element']) && in_array($setting[0], ['additional_mail', 'additional_mailScope'], true)) { + $expectedValues = explode(';', $setting[1]); + foreach ($expectedValues as $expected) { + Assert::assertTrue(in_array($expected, $value['element'], true)); + } + } elseif (isset($value[0])) { Assert::assertEquals($setting[1], $value[0], "", 0.0, 10, true); } else { Assert::assertEquals('', $setting[1]); } } } - + /** * @Then /^group "([^"]*)" has$/ * @@ -194,7 +199,7 @@ public function groupHasSetting($group, $settings) { $options['headers'] = [ 'OCS-APIREQUEST' => 'true', ]; - + $response = $client->get($fullUrl, $options); $groupDetails = simplexml_load_string($response->getBody())->data[0]->groups[0]->element; foreach ($settings->getRows() as $setting) { @@ -206,7 +211,7 @@ public function groupHasSetting($group, $settings) { } } } - + /** * @Then /^user "([^"]*)" has editable fields$/ @@ -967,4 +972,38 @@ public function cleanupGroups() { } $this->usingServer($previousServer); } + + /** + * @Then /^user "([^"]*)" has not$/ + */ + public function userHasNotSetting($user, \Behat\Gherkin\Node\TableNode $settings) { + $fullUrl = $this->baseUrl . "v{$this->apiVersion}.php/cloud/users/$user"; + $client = new Client(); + $options = []; + if ($this->currentUser === 'admin') { + $options['auth'] = $this->adminUser; + } else { + $options['auth'] = [$this->currentUser, $this->regularUser]; + } + $options['headers'] = [ + 'OCS-APIREQUEST' => 'true', + ]; + + $response = $client->get($fullUrl, $options); + foreach ($settings->getRows() as $setting) { + $value = json_decode(json_encode(simplexml_load_string($response->getBody())->data->{$setting[0]}), 1); + if (isset($value[0])) { + if (in_array($setting[0], ['additional_mail', 'additional_mailScope'], true)) { + $expectedValues = explode(';', $setting[1]); + foreach ($expectedValues as $expected) { + Assert::assertFalse(in_array($expected, $value, true)); + } + } else { + Assert::assertNotEquals($setting[1], $value[0], "", 0.0, 10, true); + } + } else { + Assert::assertNotEquals('', $setting[1]); + } + } + } } diff --git a/build/integration/features/provisioning-v1.feature b/build/integration/features/provisioning-v1.feature index dec6d2213e30d..41041dc91dacb 100644 --- a/build/integration/features/provisioning-v1.feature +++ b/build/integration/features/provisioning-v1.feature @@ -62,6 +62,7 @@ Feature: provisioning Then user "brand-new-user" has editable fields | displayname | | email | + | additional_mail | | phone | | address | | website | @@ -70,6 +71,7 @@ Feature: provisioning Then user "brand-new-user" has editable fields | displayname | | email | + | additional_mail | | phone | | address | | website | @@ -77,6 +79,7 @@ Feature: provisioning Then user "self" has editable fields | displayname | | email | + | additional_mail | | phone | | address | | website | @@ -100,6 +103,16 @@ Feature: provisioning | value | no-reply@nextcloud.com | And the OCS status code should be "100" And the HTTP status code should be "200" + And sending "PUT" to "/cloud/users/brand-new-user" with + | key | additional_mail | + | value | no.reply@nextcloud.com | + And the OCS status code should be "100" + And the HTTP status code should be "200" + And sending "PUT" to "/cloud/users/brand-new-user" with + | key | additional_mail | + | value | noreply@nextcloud.com | + And the OCS status code should be "100" + And the HTTP status code should be "200" And sending "PUT" to "/cloud/users/brand-new-user" with | key | phone | | value | +49 711 / 25 24 28-90 | @@ -124,6 +137,7 @@ Feature: provisioning | id | brand-new-user | | displayname | Brand New User | | email | no-reply@nextcloud.com | + | additional_mail | no.reply@nextcloud.com;noreply@nextcloud.com | | phone | +4971125242890 | | address | Foo Bar Town | | website | https://nextcloud.com | @@ -177,6 +191,33 @@ Feature: provisioning | displaynameScope | v2-federated | | avatarScope | v2-local | + Scenario: Edit a user account multivalue property scopes + Given user "brand-new-user" exists + And As an "brand-new-user" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | additional_mail | + | value | no.reply@nextcloud.com | + And the OCS status code should be "100" + And the HTTP status code should be "200" + And sending "PUT" to "/cloud/users/brand-new-user" with + | key | additional_mail | + | value | noreply@nextcloud.com | + And the OCS status code should be "100" + And the HTTP status code should be "200" + When sending "PUT" to "/cloud/users/brand-new-user/additional_mailScope" with + | key | no.reply@nextcloud.com | + | value | v2-federated | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + When sending "PUT" to "/cloud/users/brand-new-user/additional_mailScope" with + | key | noreply@nextcloud.com | + | value | v2-published | + Then the OCS status code should be "100" + And the HTTP status code should be "200" + Then user "brand-new-user" has + | id | brand-new-user | + | additional_mailScope | v2-federated;v2-published | + Scenario: Edit a user account properties scopes with invalid or unsupported value Given user "brand-new-user" exists And As an "brand-new-user" @@ -196,6 +237,43 @@ Feature: provisioning Then the OCS status code should be "102" And the HTTP status code should be "200" + Scenario: Edit a user account multi-value property scopes with invalid or unsupported value + Given user "brand-new-user" exists + And As an "brand-new-user" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | additional_mail | + | value | no.reply@nextcloud.com | + And the OCS status code should be "100" + And the HTTP status code should be "200" + When sending "PUT" to "/cloud/users/brand-new-user/additional_mailScope" with + | key | no.reply@nextcloud.com | + | value | invalid | + Then the OCS status code should be "102" + And the HTTP status code should be "200" + + Scenario: Delete a user account multi-value property value + Given user "brand-new-user" exists + And As an "brand-new-user" + When sending "PUT" to "/cloud/users/brand-new-user" with + | key | additional_mail | + | value | no.reply@nextcloud.com | + And the OCS status code should be "100" + And the HTTP status code should be "200" + And sending "PUT" to "/cloud/users/brand-new-user" with + | key | additional_mail | + | value | noreply@nextcloud.com | + And the OCS status code should be "100" + And the HTTP status code should be "200" + When sending "PUT" to "/cloud/users/brand-new-user/additional_mail" with + | key | no.reply@nextcloud.com | + | value | | + And the OCS status code should be "100" + And the HTTP status code should be "200" + Then user "brand-new-user" has + | additional_mail | noreply@nextcloud.com | + Then user "brand-new-user" has not + | additional_mail | no.reply@nextcloud.com | + Scenario: An admin cannot edit user account property scopes Given As an "admin" And user "brand-new-user" exists @@ -233,7 +311,7 @@ Feature: provisioning And group "new-group" exists And group "new-group" has | displayname | new-group | - + Scenario: Create a group with custom display name Given As an "admin" And group "new-group" does not exist diff --git a/lib/base.php b/lib/base.php index 20065006a8c7b..473a3419cb143 100644 --- a/lib/base.php +++ b/lib/base.php @@ -68,6 +68,7 @@ use OC\Encryption\HookManager; use OC\Files\Filesystem; use OC\Share20\Hooks; +use OCP\User\Events\UserChangedEvent; require_once 'public/Constants.php'; @@ -843,8 +844,9 @@ private static function registerEncryptionWrapperAndHooks() { } private static function registerAccountHooks() { - $hookHandler = \OC::$server->get(\OC\Accounts\Hooks::class); - \OCP\Util::connectHook('OC_User', 'changeUser', $hookHandler, 'changeUserHook'); + /** @var IEventDispatcher $dispatcher */ + $dispatcher = \OC::$server->get(IEventDispatcher::class); + $dispatcher->addServiceListener(UserChangedEvent::class, \OC\Accounts\Hooks::class); } private static function registerAppRestrictionsHooks() { diff --git a/lib/private/Accounts/Account.php b/lib/private/Accounts/Account.php index 7d2a51c7d4e2c..1e4189f2b35c7 100644 --- a/lib/private/Accounts/Account.php +++ b/lib/private/Accounts/Account.php @@ -33,6 +33,7 @@ use OCP\Accounts\IAccountPropertyCollection; use OCP\Accounts\PropertyDoesNotExistException; use OCP\IUser; +use RuntimeException; class Account implements IAccount { use TAccountsHelper; @@ -116,13 +117,16 @@ public function setPropertyCollection(IAccountPropertyCollection $propertyCollec return $this; } - public function getPropertyCollection(string $propertyCollection): IAccountPropertyCollection { - if (!array_key_exists($propertyCollection, $this->properties)) { - throw new PropertyDoesNotExistException($propertyCollection); + public function getPropertyCollection(string $propertyCollectionName): IAccountPropertyCollection { + if (!$this->isCollection($propertyCollectionName)) { + throw new PropertyDoesNotExistException($propertyCollectionName); } - if (!$this->properties[$propertyCollection] instanceof IAccountPropertyCollection) { - throw new \RuntimeException('Requested collection is not an IAccountPropertyCollection'); + if (!array_key_exists($propertyCollectionName, $this->properties)) { + $this->properties[$propertyCollectionName] = new AccountPropertyCollection($propertyCollectionName); } - return $this->properties[$propertyCollection]; + if (!$this->properties[$propertyCollectionName] instanceof IAccountPropertyCollection) { + throw new RuntimeException('Requested collection is not an IAccountPropertyCollection'); + } + return $this->properties[$propertyCollectionName]; } } diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index 4d75c94346b42..9fc5accfa08a4 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -32,6 +32,7 @@ */ namespace OC\Accounts; +use InvalidArgumentException; use libphonenumber\NumberParseException; use libphonenumber\PhoneNumber; use libphonenumber\PhoneNumberFormat; @@ -39,6 +40,9 @@ use OCA\Settings\BackgroundJobs\VerifyUserData; use OCP\Accounts\IAccount; use OCP\Accounts\IAccountManager; +use OCP\Accounts\IAccountProperty; +use OCP\Accounts\IAccountPropertyCollection; +use OCP\Accounts\PropertyDoesNotExistException; use OCP\BackgroundJob\IJobList; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IConfig; @@ -48,7 +52,9 @@ use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\EventDispatcher\GenericEvent; use function array_flip; +use function iterator_to_array; use function json_decode; +use function json_encode; use function json_last_error; /** @@ -98,7 +104,7 @@ public function __construct(IDBConnection $connection, /** * @param string $input * @return string Provided phone number in E.164 format when it was a valid number - * @throws \InvalidArgumentException When the phone number was invalid or no default region is set and the number doesn't start with a country code + * @throws InvalidArgumentException When the phone number was invalid or no default region is set and the number doesn't start with a country code */ protected function parsePhoneNumber(string $input): string { $defaultRegion = $this->config->getSystemValueString('default_phone_region', ''); @@ -106,7 +112,7 @@ protected function parsePhoneNumber(string $input): string { if ($defaultRegion === '') { // When no default region is set, only +49… numbers are valid if (strpos($input, '+') !== 0) { - throw new \InvalidArgumentException(self::PROPERTY_PHONE); + throw new InvalidArgumentException(self::PROPERTY_PHONE); } $defaultRegion = 'EN'; @@ -121,140 +127,107 @@ protected function parsePhoneNumber(string $input): string { } catch (NumberParseException $e) { } - throw new \InvalidArgumentException(self::PROPERTY_PHONE); + throw new InvalidArgumentException(self::PROPERTY_PHONE); } /** * * @param string $input * @return string - * @throws \InvalidArgumentException When the website did not have http(s) as protocol or the host name was empty + * @throws InvalidArgumentException When the website did not have http(s) as protocol or the host name was empty */ protected function parseWebsite(string $input): string { $parts = parse_url($input); if (!isset($parts['scheme']) || ($parts['scheme'] !== 'https' && $parts['scheme'] !== 'http')) { - throw new \InvalidArgumentException(self::PROPERTY_WEBSITE); + throw new InvalidArgumentException(self::PROPERTY_WEBSITE); } if (!isset($parts['host']) || $parts['host'] === '') { - throw new \InvalidArgumentException(self::PROPERTY_WEBSITE); + throw new InvalidArgumentException(self::PROPERTY_WEBSITE); } 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); + /** + * @param IAccountProperty[] $properties + */ + protected function testValueLengths(array $properties, bool $throwOnData = false): void { + foreach ($properties as $property) { + if (strlen($property->getValue()) > 2048) { + if ($throwOnData) { + throw new InvalidArgumentException(); } else { - $this->sanitizeLength($propertyData, $throwOnData); + $property->setValue(''); } } - } 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'); - } + protected function testPropertyScope(IAccountProperty $property, array $allowedScopes, bool $throwOnData): void { + if ($throwOnData && !in_array($property->getScope(), $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']); - } + if ( + $property->getScope() === self::SCOPE_PRIVATE + && in_array($property->getName(), [self::PROPERTY_DISPLAYNAME, self::PROPERTY_EMAIL]) + ) { + if ($throwOnData) { + // v2-private is not available for these fields + throw new InvalidArgumentException('scope'); + } else { + // default to local + $property->setScope(self::SCOPE_LOCAL); } + } else { + // migrate scope values to the new format + // invalid scopes are mapped to a default value + $property->setScope(AccountProperty::mapScopeToV2($property->getScope())); } } - /** - * update user record - * - * @param IUser $user - * @param array $data - * @param bool $throwOnData Set to true if you can inform the user about invalid data - * @return array The potentially modified data (e.g. phone numbers are converted to E.164 format) - * @throws \InvalidArgumentException Message is the property that was invalid - */ - public function updateUser(IUser $user, array $data, bool $throwOnData = false): array { - $userData = $this->getUser($user); - $updated = true; - - if (isset($data[self::PROPERTY_PHONE]) && $data[self::PROPERTY_PHONE]['value'] !== '') { - // Sanitize null value. - $data[self::PROPERTY_PHONE]['value'] = $data[self::PROPERTY_PHONE]['value'] ?? ''; - - try { - $data[self::PROPERTY_PHONE]['value'] = $this->parsePhoneNumber($data[self::PROPERTY_PHONE]['value']); - } catch (\InvalidArgumentException $e) { - if ($throwOnData) { - throw $e; - } - $data[self::PROPERTY_PHONE]['value'] = ''; + protected function sanitizePhoneNumberValue(IAccountProperty $property, bool $throwOnData = false) { + if ($property->getName() !== self::PROPERTY_PHONE) { + if ($throwOnData) { + throw new InvalidArgumentException(sprintf('sanitizePhoneNumberValue can only sanitize phone numbers, %s given', $property->getName())); } + return; } - - $this->testValueLengths($data); - - if (isset($data[self::PROPERTY_WEBSITE]) && $data[self::PROPERTY_WEBSITE]['value'] !== '') { - try { - $data[self::PROPERTY_WEBSITE]['value'] = $this->parseWebsite($data[self::PROPERTY_WEBSITE]['value']); - } catch (\InvalidArgumentException $e) { - if ($throwOnData) { - throw $e; - } - $data[self::PROPERTY_WEBSITE]['value'] = ''; + if ($property->getValue() === '') { + return; + } + try { + $property->setValue($this->parsePhoneNumber($property->getValue())); + } catch (InvalidArgumentException $e) { + if ($throwOnData) { + throw $e; } + $property->setValue(''); } + } - $allowedScopes = [ - self::SCOPE_PRIVATE, - self::SCOPE_LOCAL, - self::SCOPE_FEDERATED, - self::SCOPE_PUBLISHED, - self::VISIBILITY_PRIVATE, - self::VISIBILITY_CONTACTS_ONLY, - self::VISIBILITY_PUBLIC, - ]; + protected function sanitizeWebsite(IAccountProperty $property, bool $throwOnData = false) { + if ($property->getName() !== self::PROPERTY_WEBSITE) { + if ($throwOnData) { + throw new InvalidArgumentException(sprintf('sanitizeWebsite can only sanitize web domains, %s given', $property->getName())); + } + } + try { + $property->setValue($this->parseWebsite($property->getValue())); + } catch (InvalidArgumentException $e) { + if ($throwOnData) { + throw $e; + } + $property->setValue(''); + } + } - $this->testPropertyScopes($data, $allowedScopes, $throwOnData); + protected function updateUser(IUser $user, array $data, bool $throwOnData = false): array { + $oldUserData = $this->getUser($user, false); + $updated = true; - if (empty($userData)) { - $this->insertNewUser($user, $data); - } elseif ($userData !== $data) { - $data = $this->checkEmailVerification($userData, $data, $user); - $data = $this->updateVerifyStatus($userData, $data); + if ($oldUserData !== $data) { $this->updateExistingUser($user, $data); } else { // nothing needs to be done if new and old data set are the same @@ -301,17 +274,15 @@ public function deleteUserData(IUser $user): void { /** * get stored data from a given user - * - * @deprecated use getAccount instead to make sure migrated properties work correctly */ - public function getUser(IUser $user, bool $insertIfNotExists = true): array { + protected function getUser(IUser $user, bool $insertIfNotExists = true): array { $uid = $user->getUID(); $query = $this->connection->getQueryBuilder(); $query->select('data') ->from($this->table) ->where($query->expr()->eq('uid', $query->createParameter('uid'))) ->setParameter('uid', $uid); - $result = $query->execute(); + $result = $query->executeQuery(); $accountData = $result->fetchAll(); $result->closeCursor(); @@ -323,10 +294,8 @@ public function getUser(IUser $user, bool $insertIfNotExists = true): array { return $userData; } - $userDataArray = json_decode($accountData[0]['data'], true); - $jsonError = json_last_error(); - if ($userDataArray === null || $userDataArray === [] || $jsonError !== JSON_ERROR_NONE) { - $this->logger->critical("User data of $uid contained invalid JSON (error $jsonError), hence falling back to a default user record"); + $userDataArray = $this->importFromJson($accountData[0]['data'], $uid); + if ($userDataArray === null || $userDataArray === []) { return $this->buildDefaultUserRecord($user); } @@ -344,7 +313,7 @@ public function searchUsers(string $property, array $values): array { $matches = []; foreach ($chunks as $chunk) { $query->setParameter('values', $chunk, IQueryBuilder::PARAM_STR_ARRAY); - $result = $query->execute(); + $result = $query->executeQuery(); while ($row = $result->fetch()) { $matches[$row['uid']] = $row['value']; @@ -369,100 +338,75 @@ protected function searchUsersForRelatedCollection(string $property, array $valu /** * check if we need to ask the server for email verification, if yes we create a cronjob * - * @param $oldData - * @param $newData - * @param IUser $user - * @return array */ - protected function checkEmailVerification($oldData, $newData, IUser $user): array { - if ($oldData[self::PROPERTY_EMAIL]['value'] !== $newData[self::PROPERTY_EMAIL]['value']) { + protected function checkEmailVerification(IAccount $updatedAccount, array $oldData): void { + try { + $property = $updatedAccount->getProperty(self::PROPERTY_EMAIL); + } catch (PropertyDoesNotExistException $e) { + return; + } + $oldMail = isset($oldData[self::PROPERTY_EMAIL]) ? $oldData[self::PROPERTY_EMAIL]['value']['value'] : ''; + if ($oldMail !== $property->getValue()) { $this->jobList->add(VerifyUserData::class, [ 'verificationCode' => '', - 'data' => $newData[self::PROPERTY_EMAIL]['value'], + 'data' => $property->getValue(), 'type' => self::PROPERTY_EMAIL, - 'uid' => $user->getUID(), + 'uid' => $updatedAccount->getUser()->getUID(), 'try' => 0, 'lastRun' => time() ] ); - $newData[self::PROPERTY_EMAIL]['verified'] = self::VERIFICATION_IN_PROGRESS; - } - return $newData; + + + + $property->setVerified(self::VERIFICATION_IN_PROGRESS); + } } /** * make sure that all expected data are set * - * @param array $userData - * @return array */ - protected function addMissingDefaultValues(array $userData) { - foreach ($userData as $key => $value) { - if (!isset($userData[$key]['verified'])) { - $userData[$key]['verified'] = self::NOT_VERIFIED; + protected function addMissingDefaultValues(array $userData): array { + foreach ($userData as $i => $value) { + if (!isset($value['verified'])) { + $userData[$i]['verified'] = self::NOT_VERIFIED; } } return $userData; } - /** - * reset verification status if personal data changed - * - * @param array $oldData - * @param array $newData - * @return array - */ - 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; - $websiteVerified = isset($oldData[self::PROPERTY_WEBSITE]['verified']) && $oldData[self::PROPERTY_WEBSITE]['verified'] === self::VERIFIED; - $emailVerified = isset($oldData[self::PROPERTY_EMAIL]['verified']) && $oldData[self::PROPERTY_EMAIL]['verified'] === self::VERIFIED; - - // keep old verification status if we don't have a new one - if (!isset($newData[self::PROPERTY_TWITTER]['verified'])) { - // keep old verification status if value didn't changed and an old value exists - $keepOldStatus = $newData[self::PROPERTY_TWITTER]['value'] === $oldData[self::PROPERTY_TWITTER]['value'] && isset($oldData[self::PROPERTY_TWITTER]['verified']); - $newData[self::PROPERTY_TWITTER]['verified'] = $keepOldStatus ? $oldData[self::PROPERTY_TWITTER]['verified'] : self::NOT_VERIFIED; - } - - if (!isset($newData[self::PROPERTY_WEBSITE]['verified'])) { - // keep old verification status if value didn't changed and an old value exists - $keepOldStatus = $newData[self::PROPERTY_WEBSITE]['value'] === $oldData[self::PROPERTY_WEBSITE]['value'] && isset($oldData[self::PROPERTY_WEBSITE]['verified']); - $newData[self::PROPERTY_WEBSITE]['verified'] = $keepOldStatus ? $oldData[self::PROPERTY_WEBSITE]['verified'] : self::NOT_VERIFIED; - } - - if (!isset($newData[self::PROPERTY_EMAIL]['verified'])) { - // keep old verification status if value didn't changed and an old value exists - $keepOldStatus = $newData[self::PROPERTY_EMAIL]['value'] === $oldData[self::PROPERTY_EMAIL]['value'] && isset($oldData[self::PROPERTY_EMAIL]['verified']); - $newData[self::PROPERTY_EMAIL]['verified'] = $keepOldStatus ? $oldData[self::PROPERTY_EMAIL]['verified'] : self::VERIFICATION_IN_PROGRESS; - } - - // reset verification status if a value from a previously verified data was changed - if ($twitterVerified && - $oldData[self::PROPERTY_TWITTER]['value'] !== $newData[self::PROPERTY_TWITTER]['value'] - ) { - $newData[self::PROPERTY_TWITTER]['verified'] = self::NOT_VERIFIED; - } - - if ($websiteVerified && - $oldData[self::PROPERTY_WEBSITE]['value'] !== $newData[self::PROPERTY_WEBSITE]['value'] - ) { - $newData[self::PROPERTY_WEBSITE]['verified'] = self::NOT_VERIFIED; - } + protected function updateVerificationStatus(IAccount $updatedAccount, array $oldData): void { + static $propertiesVerifiableByLookupServer = [ + self::PROPERTY_TWITTER, + self::PROPERTY_WEBSITE, + self::PROPERTY_EMAIL, + ]; - if ($emailVerified && - $oldData[self::PROPERTY_EMAIL]['value'] !== $newData[self::PROPERTY_EMAIL]['value'] - ) { - $newData[self::PROPERTY_EMAIL]['verified'] = self::NOT_VERIFIED; + foreach ($propertiesVerifiableByLookupServer as $propertyName) { + try { + $property = $updatedAccount->getProperty($propertyName); + } catch (PropertyDoesNotExistException $e) { + continue; + } + $wasVerified = isset($oldData[$propertyName]) + && isset($oldData[$propertyName]['verified']) + && $oldData[$propertyName]['verified'] === self::VERIFIED; + if ((!isset($oldData[$propertyName]) + || !isset($oldData[$propertyName]['value']) + || $property->getValue() !== $oldData[$propertyName]['value']) + && ($property->getVerified() !== self::NOT_VERIFIED + || $wasVerified) + ) { + $property->setVerified(self::NOT_VERIFIED); + } } - - return $newData; } + /** * add new user to accounts table * @@ -471,7 +415,7 @@ protected function updateVerifyStatus(array $oldData, array $newData): array { */ protected function insertNewUser(IUser $user, array $data): void { $uid = $user->getUID(); - $jsonEncodedData = json_encode($data); + $jsonEncodedData = $this->prepareJson($data); $query = $this->connection->getQueryBuilder(); $query->insert($this->table) ->values( @@ -480,12 +424,55 @@ protected function insertNewUser(IUser $user, array $data): void { 'data' => $query->createNamedParameter($jsonEncodedData), ] ) - ->execute(); + ->executeStatement(); $this->deleteUserData($user); $this->writeUserData($user, $data); } + protected function prepareJson(array $data): string { + $preparedData = []; + foreach ($data as $dataRow) { + $propertyName = $dataRow['name']; + unset($dataRow['name']); + if (!$this->isCollection($propertyName)) { + $preparedData[$propertyName] = $dataRow; + continue; + } + if (!isset($preparedData[$propertyName])) { + $preparedData[$propertyName] = []; + } + $preparedData[$propertyName][] = $dataRow; + } + return json_encode($preparedData); + } + + protected function importFromJson(string $json, string $userId): ?array { + $result = []; + $jsonArray = json_decode($json, true); + $jsonError = json_last_error(); + if ($jsonError !== JSON_ERROR_NONE) { + $this->logger->critical( + 'User data of {uid} contained invalid JSON (error {json_error}), hence falling back to a default user record', + [ + 'uid' => $userId, + 'json_error' => $jsonError + ] + ); + return null; + } + foreach ($jsonArray as $propertyName => $row) { + if (!$this->isCollection($propertyName)) { + $result[] = array_merge($row, ['name' => $propertyName]); + continue; + } + foreach ($row as $singleRow) { + $result[] = array_merge($singleRow, ['name' => $propertyName]); + } + } + return $result; + } + /** * update existing user in accounts table * @@ -494,12 +481,12 @@ protected function insertNewUser(IUser $user, array $data): void { */ protected function updateExistingUser(IUser $user, array $data): void { $uid = $user->getUID(); - $jsonEncodedData = json_encode($data); + $jsonEncodedData = $this->prepareJson($data); $query = $this->connection->getQueryBuilder(); $query->update($this->table) ->set('data', $query->createNamedParameter($jsonEncodedData)) ->where($query->expr()->eq('uid', $query->createNamedParameter($uid))) - ->execute(); + ->executeStatement(); $this->deleteUserData($user); $this->writeUserData($user, $data); @@ -518,19 +505,16 @@ protected function writeUserData(IUser $user, array $data): void { $this->writeUserDataProperties($query, $data); } - protected function writeUserDataProperties(IQueryBuilder $query, array $data, string $parentPropertyName = null): void { - foreach ($data as $propertyName => $property) { - if ($this->isCollection($propertyName)) { - $this->writeUserDataProperties($query, $property, $propertyName); - continue; - } - if (($parentPropertyName ?? $propertyName) === self::PROPERTY_AVATAR) { + protected function writeUserDataProperties(IQueryBuilder $query, array $data): void { + foreach ($data as $property) { + if ($property['name'] === self::PROPERTY_AVATAR) { continue; } - $query->setParameter('name', $parentPropertyName ?? $propertyName) + + $query->setParameter('name', $property['name']) ->setParameter('value', $property['value'] ?? ''); - $query->execute(); + $query->executeStatement(); } } @@ -542,53 +526,80 @@ protected function writeUserDataProperties(IQueryBuilder $query, array $data, st */ protected function buildDefaultUserRecord(IUser $user) { return [ - self::PROPERTY_DISPLAYNAME => - [ - 'value' => $user->getDisplayName(), - 'scope' => self::SCOPE_FEDERATED, - 'verified' => self::NOT_VERIFIED, - ], - self::PROPERTY_ADDRESS => - [ - 'value' => '', - 'scope' => self::SCOPE_LOCAL, - 'verified' => self::NOT_VERIFIED, - ], - self::PROPERTY_WEBSITE => - [ - 'value' => '', - 'scope' => self::SCOPE_LOCAL, - 'verified' => self::NOT_VERIFIED, - ], - self::PROPERTY_EMAIL => - [ - 'value' => $user->getEMailAddress(), - 'scope' => self::SCOPE_FEDERATED, - 'verified' => self::NOT_VERIFIED, - ], - self::PROPERTY_AVATAR => - [ - 'scope' => self::SCOPE_FEDERATED - ], - self::PROPERTY_PHONE => - [ - 'value' => '', - 'scope' => self::SCOPE_LOCAL, - 'verified' => self::NOT_VERIFIED, - ], - self::PROPERTY_TWITTER => - [ - 'value' => '', - 'scope' => self::SCOPE_LOCAL, - 'verified' => self::NOT_VERIFIED, - ], + + [ + 'name' => self::PROPERTY_DISPLAYNAME, + 'value' => $user->getDisplayName(), + 'scope' => self::SCOPE_FEDERATED, + 'verified' => self::NOT_VERIFIED, + ], + + [ + 'name' => self::PROPERTY_ADDRESS, + 'value' => '', + 'scope' => self::SCOPE_LOCAL, + 'verified' => self::NOT_VERIFIED, + ], + + [ + 'name' => self::PROPERTY_WEBSITE, + 'value' => '', + 'scope' => self::SCOPE_LOCAL, + 'verified' => self::NOT_VERIFIED, + ], + + [ + 'name' => self::PROPERTY_EMAIL, + 'value' => $user->getEMailAddress(), + 'scope' => self::SCOPE_FEDERATED, + 'verified' => self::NOT_VERIFIED, + ], + + [ + 'name' => self::PROPERTY_AVATAR, + 'scope' => self::SCOPE_FEDERATED + ], + + [ + 'name' => self::PROPERTY_PHONE, + 'value' => '', + 'scope' => self::SCOPE_LOCAL, + 'verified' => self::NOT_VERIFIED, + ], + + [ + 'name' => self::PROPERTY_TWITTER, + 'value' => '', + 'scope' => self::SCOPE_LOCAL, + 'verified' => self::NOT_VERIFIED, + ], + ]; } + private function arrayDataToCollection(IAccount $account, array $data): IAccountPropertyCollection { + $collection = $account->getPropertyCollection($data['name']); + + $p = new AccountProperty( + $data['name'], + $data['value'] ?? '', + $data['scope'] ?? self::SCOPE_LOCAL, + $data['verified'] ?? self::NOT_VERIFIED, + '' + ); + $collection->addProperty($p); + + return $collection; + } + private function parseAccountData(IUser $user, $data): Account { $account = new Account($user); - foreach ($data as $property => $accountData) { - $account->setProperty($property, $accountData['value'] ?? '', $accountData['scope'] ?? self::SCOPE_LOCAL, $accountData['verified'] ?? self::NOT_VERIFIED); + foreach ($data as $accountData) { + if ($this->isCollection($accountData['name'])) { + $account->setPropertyCollection($this->arrayDataToCollection($account, $accountData)); + } else { + $account->setProperty($accountData['name'], $accountData['value'] ?? '', $accountData['scope'] ?? self::SCOPE_LOCAL, $accountData['verified'] ?? self::NOT_VERIFIED); + } } return $account; } @@ -598,10 +609,42 @@ public function getAccount(IUser $user): IAccount { } public function updateAccount(IAccount $account): void { - $data = []; + $this->testValueLengths(iterator_to_array($account->getAllProperties()), true); + try { + $property = $account->getProperty(self::PROPERTY_PHONE); + $this->sanitizePhoneNumberValue($property); + } catch (PropertyDoesNotExistException $e) { + // valid case, nothing to do + } + + try { + $property = $account->getProperty(self::PROPERTY_WEBSITE); + $this->sanitizeWebsite($property); + } catch (PropertyDoesNotExistException $e) { + // valid case, nothing to do + } + + static $allowedScopes = [ + self::SCOPE_PRIVATE, + self::SCOPE_LOCAL, + self::SCOPE_FEDERATED, + self::SCOPE_PUBLISHED, + self::VISIBILITY_PRIVATE, + self::VISIBILITY_CONTACTS_ONLY, + self::VISIBILITY_PUBLIC, + ]; + foreach ($account->getAllProperties() as $property) { + $this->testPropertyScope($property, $allowedScopes, true); + } - foreach ($account->getProperties() as $property) { - $data[$property->getName()] = [ + $oldData = $this->getUser($account->getUser(), false); + $this->updateVerificationStatus($account, $oldData); + $this->checkEmailVerification($account, $oldData); + + $data = []; + foreach ($account->getAllProperties() as $property) { + $data[] = [ + 'name' => $property->getName(), 'value' => $property->getValue(), 'scope' => $property->getScope(), 'verified' => $property->getVerified(), diff --git a/lib/private/Accounts/AccountPropertyCollection.php b/lib/private/Accounts/AccountPropertyCollection.php index 84e10e6a507af..eb92536a6a0a5 100644 --- a/lib/private/Accounts/AccountPropertyCollection.php +++ b/lib/private/Accounts/AccountPropertyCollection.php @@ -27,6 +27,7 @@ namespace OC\Accounts; use InvalidArgumentException; +use OCP\Accounts\IAccountManager; use OCP\Accounts\IAccountProperty; use OCP\Accounts\IAccountPropertyCollection; @@ -63,6 +64,18 @@ public function addProperty(IAccountProperty $property): IAccountPropertyCollect return $this; } + public function addPropertyWithDefaults(string $value): IAccountPropertyCollection { + $property = new AccountProperty( + $this->collectionName, + $value, + IAccountManager::SCOPE_LOCAL, + IAccountManager::NOT_VERIFIED, + '' + ); + $this->addProperty($property); + return $this; + } + public function removeProperty(IAccountProperty $property): IAccountPropertyCollection { $ref = array_search($property, $this->properties, true); if ($ref !== false) { diff --git a/lib/private/Accounts/Hooks.php b/lib/private/Accounts/Hooks.php index af078bd1db099..93918284180fe 100644 --- a/lib/private/Accounts/Hooks.php +++ b/lib/private/Accounts/Hooks.php @@ -25,66 +25,55 @@ namespace OC\Accounts; use OCP\Accounts\IAccountManager; +use OCP\Accounts\PropertyDoesNotExistException; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; use OCP\IUser; +use OCP\User\Events\UserChangedEvent; use Psr\Log\LoggerInterface; -class Hooks { +class Hooks implements IEventListener { - /** @var AccountManager|null */ + /** @var IAccountManager */ private $accountManager; - /** @var LoggerInterface */ private $logger; - public function __construct(LoggerInterface $logger) { + public function __construct(LoggerInterface $logger, IAccountManager $accountManager) { $this->logger = $logger; + $this->accountManager = $accountManager; } /** * update accounts table if email address or display name was changed from outside - * - * @param array $params */ - public function changeUserHook($params) { - $accountManager = $this->getAccountManager(); - - /** @var IUser $user */ - $user = isset($params['user']) ? $params['user'] : null; - $feature = isset($params['feature']) ? $params['feature'] : null; - $newValue = isset($params['value']) ? $params['value'] : null; + public function changeUserHook(IUser $user, string $feature, $newValue): void { + $account = $this->accountManager->getAccount($user); - if (is_null($user) || is_null($feature) || is_null($newValue)) { - $this->logger->warning('Missing expected parameters in change user hook'); + try { + switch ($feature) { + case 'eMailAddress': + $property = $account->getProperty(IAccountManager::PROPERTY_EMAIL); + break; + case 'displayName': + $property = $account->getProperty(IAccountManager::PROPERTY_DISPLAYNAME); + break; + } + } catch (PropertyDoesNotExistException $e) { + $this->logger->debug($e->getMessage(), ['exception' => $e]); return; } - $accountData = $accountManager->getUser($user); - - switch ($feature) { - case 'eMailAddress': - if ($accountData[IAccountManager::PROPERTY_EMAIL]['value'] !== $newValue) { - $accountData[IAccountManager::PROPERTY_EMAIL]['value'] = $newValue; - $accountManager->updateUser($user, $accountData); - } - break; - case 'displayName': - if ($accountData[IAccountManager::PROPERTY_DISPLAYNAME]['value'] !== $newValue) { - $accountData[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $newValue; - $accountManager->updateUser($user, $accountData); - } - break; + if (isset($property) && $property->getValue() !== (string)$newValue) { + $property->setValue($newValue); + $this->accountManager->updateAccount($account); } } - /** - * return instance of accountManager - * - * @return AccountManager - */ - protected function getAccountManager(): AccountManager { - if ($this->accountManager === null) { - $this->accountManager = \OC::$server->query(AccountManager::class); + public function handle(Event $event): void { + if (!$event instanceof UserChangedEvent) { + return; } - return $this->accountManager; + $this->changeUserHook($event->getUser(), $event->getFeature(), $event->getValue()); } } diff --git a/lib/private/Avatar/AvatarManager.php b/lib/private/Avatar/AvatarManager.php index 83b2b4b5c3df8..c3afd8094c741 100644 --- a/lib/private/Avatar/AvatarManager.php +++ b/lib/private/Avatar/AvatarManager.php @@ -39,6 +39,7 @@ use OC\User\Manager; use OC\User\NoUserException; use OCP\Accounts\IAccountManager; +use OCP\Accounts\PropertyDoesNotExistException; use OCP\Files\IAppData; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; @@ -126,9 +127,13 @@ public function getAvatar(string $userId) : IAvatar { $folder = $this->appData->newFolder($userId); } - $account = $this->accountManager->getAccount($user); - $avatarProperties = $account->getProperty(IAccountManager::PROPERTY_AVATAR); - $avatarScope = $avatarProperties->getScope(); + try { + $account = $this->accountManager->getAccount($user); + $avatarProperties = $account->getProperty(IAccountManager::PROPERTY_AVATAR); + $avatarScope = $avatarProperties->getScope(); + } catch (PropertyDoesNotExistException $e) { + $avatarScope = ''; + } if ( // v2-private scope hides the avatar from public access and from unknown users diff --git a/lib/public/Accounts/IAccount.php b/lib/public/Accounts/IAccount.php index 72c207537dce2..8d4d8b5c0234f 100644 --- a/lib/public/Accounts/IAccount.php +++ b/lib/public/Accounts/IAccount.php @@ -94,9 +94,10 @@ public function setPropertyCollection(IAccountPropertyCollection $propertyCollec /** * Returns the requestes propery collection (multi-value properties) * + * @throws PropertyDoesNotExistException against invalid collection name * @since 22.0.0 */ - public function getPropertyCollection(string $propertyCollection): IAccountPropertyCollection; + public function getPropertyCollection(string $propertyCollectionName): IAccountPropertyCollection; /** * Get all properties that match the provided filters for scope and verification status diff --git a/lib/public/Accounts/IAccountPropertyCollection.php b/lib/public/Accounts/IAccountPropertyCollection.php index 9e026f4ce5b2b..779fb1299b4cd 100644 --- a/lib/public/Accounts/IAccountPropertyCollection.php +++ b/lib/public/Accounts/IAccountPropertyCollection.php @@ -68,6 +68,14 @@ public function getProperties(): array; */ public function addProperty(IAccountProperty $property): IAccountPropertyCollection; + /** + * adds a property to this collection with only specifying the value + * + * @throws InvalidArgumentException + * @since 22.0.0 + */ + public function addPropertyWithDefaults(string $value): IAccountPropertyCollection; + /** * removes a property of this collection * diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php index ea82bd04f3f39..8ed0e29d7ce53 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -108,65 +108,191 @@ protected function populateOrUpdate(): void { [ '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], + [ + 'name' => IAccountManager::PROPERTY_DISPLAYNAME, + 'value' => 'Jane Doe', + 'scope' => IAccountManager::SCOPE_PUBLISHED + ], + [ + 'name' => IAccountManager::PROPERTY_EMAIL, + 'value' => 'jane.doe@acme.com', + 'scope' => IAccountManager::SCOPE_LOCAL + ], + [ + 'name' => IAccountManager::PROPERTY_TWITTER, + 'value' => '@sometwitter', + 'scope' => IAccountManager::SCOPE_PUBLISHED + ], + [ + 'name' => IAccountManager::PROPERTY_PHONE, + 'value' => '+491601231212', + 'scope' => IAccountManager::SCOPE_FEDERATED + ], + [ + 'name' => IAccountManager::PROPERTY_ADDRESS, + 'value' => 'some street', + 'scope' => IAccountManager::SCOPE_LOCAL + ], + [ + 'name' => 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], + [ + 'name' => IAccountManager::PROPERTY_DISPLAYNAME, + 'value' => 'Alice Allison', + 'scope' => IAccountManager::SCOPE_LOCAL + ], + [ + 'name' => IAccountManager::PROPERTY_EMAIL, + 'value' => 'a.allison@example.org', + 'scope' => IAccountManager::SCOPE_LOCAL + ], + [ + 'name' => IAccountManager::PROPERTY_TWITTER, + 'value' => '@a_alice', + 'scope' => IAccountManager::SCOPE_FEDERATED + ], + [ + 'name' => IAccountManager::PROPERTY_PHONE, + 'value' => '+491602312121', + 'scope' => IAccountManager::SCOPE_LOCAL + ], + [ + 'name' => IAccountManager::PROPERTY_ADDRESS, + 'value' => 'Dundee Road 45', + 'scope' => IAccountManager::SCOPE_LOCAL + ], + [ + 'name' => 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], + [ + 'name' => IAccountManager::PROPERTY_DISPLAYNAME, + 'value' => 'Armel Oliseh', + 'scope' => IAccountManager::SCOPE_PUBLISHED + ], + [ + 'name' => IAccountManager::PROPERTY_EMAIL, + 'value' => 'oliseh@example.com', + 'scope' => IAccountManager::SCOPE_PUBLISHED + ], + [ + 'name' => IAccountManager::PROPERTY_TWITTER, + 'value' => '', + 'scope' => IAccountManager::SCOPE_LOCAL + ], + [ + 'name' => IAccountManager::PROPERTY_PHONE, + 'value' => '+491603121212', + 'scope' => IAccountManager::SCOPE_PUBLISHED + ], + [ + 'name' => IAccountManager::PROPERTY_ADDRESS, + 'value' => 'Sunflower Blvd. 77', + 'scope' => IAccountManager::SCOPE_PUBLISHED + ], + [ + 'name' => 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], + [ + 'name' => IAccountManager::PROPERTY_DISPLAYNAME, + 'value' => 'K. Cheng', + 'scope' => IAccountManager::SCOPE_FEDERATED + ], + [ + 'name' => IAccountManager::PROPERTY_EMAIL, + 'value' => 'cheng@emca.com', + 'scope' => IAccountManager::SCOPE_FEDERATED + ], + [ + 'name' => IAccountManager::PROPERTY_TWITTER, + 'value' => '', ' + scope' => IAccountManager::SCOPE_LOCAL + ], + [ + 'name' => IAccountManager::PROPERTY_PHONE, + 'value' => '+71601212123', + 'scope' => IAccountManager::SCOPE_LOCAL + ], + [ + 'name' => IAccountManager::PROPERTY_ADDRESS, + 'value' => 'Pinapple Street 22', + 'scope' => IAccountManager::SCOPE_LOCAL + ], + [ + 'name' => IAccountManager::PROPERTY_WEBSITE, + 'value' => 'https://emca.com', + 'scope' => IAccountManager::SCOPE_FEDERATED + ], + [ + 'name' => IAccountManager::COLLECTION_EMAIL, + 'value' => 'k.cheng@emca.com', + 'scope' => IAccountManager::SCOPE_LOCAL + ], + [ + 'name' => IAccountManager::COLLECTION_EMAIL, + '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], + [ + 'name' => IAccountManager::PROPERTY_DISPLAYNAME, + 'value' => 'Goodpal, Kim', + 'scope' => IAccountManager::SCOPE_PUBLISHED + ], + [ + 'name' => IAccountManager::PROPERTY_EMAIL, + 'value' => 'goodpal@elpmaxe.org', + 'scope' => IAccountManager::SCOPE_PUBLISHED + ], + [ + 'name' => IAccountManager::PROPERTY_TWITTER, + 'value' => '', + 'scope' => IAccountManager::SCOPE_LOCAL + ], + [ + 'name' => IAccountManager::PROPERTY_PHONE, + 'value' => '+71602121231', + 'scope' => IAccountManager::SCOPE_FEDERATED + ], + [ + 'name' => IAccountManager::PROPERTY_ADDRESS, + 'value' => 'Octopus Ave 17', + 'scope' => IAccountManager::SCOPE_FEDERATED + ], + [ + 'name' => IAccountManager::PROPERTY_WEBSITE, + 'value' => 'https://elpmaxe.org', + 'scope' => IAccountManager::SCOPE_PUBLISHED + ], ], ], ]; foreach ($users as $userInfo) { - $this->accountManager->updateUser($userInfo['user'], $userInfo['data'], false); + $this->invokePrivate($this->accountManager, 'updateUser', [$userInfo['user'], $userInfo['data'], false]); } } @@ -198,7 +324,7 @@ public function getInstance($mockedMethods = null) { * @param bool $updateExisting */ public function testUpdateUser($newData, $oldData, $insertNew, $updateExisting) { - $accountManager = $this->getInstance(['getUser', 'insertNewUser', 'updateExistingUser', 'updateVerifyStatus', 'checkEmailVerification']); + $accountManager = $this->getInstance(['getUser', 'insertNewUser', 'updateExistingUser']); /** @var IUser $user */ $user = $this->createMock(IUser::class); @@ -206,10 +332,6 @@ public function testUpdateUser($newData, $oldData, $insertNew, $updateExisting) $accountManager->expects($this->once())->method('getUser')->with($user)->willReturn($oldData); if ($updateExisting) { - $accountManager->expects($this->once())->method('checkEmailVerification') - ->with($oldData, $newData, $user)->willReturn($newData); - $accountManager->expects($this->once())->method('updateVerifyStatus') - ->with($oldData, $newData)->willReturn($newData); $accountManager->expects($this->once())->method('updateExistingUser') ->with($user, $newData); $accountManager->expects($this->never())->method('insertNewUser'); @@ -222,8 +344,6 @@ public function testUpdateUser($newData, $oldData, $insertNew, $updateExisting) if (!$insertNew && !$updateExisting) { $accountManager->expects($this->never())->method('updateExistingUser'); - $accountManager->expects($this->never())->method('checkEmailVerification'); - $accountManager->expects($this->never())->method('updateVerifyStatus'); $accountManager->expects($this->never())->method('insertNewUser'); $this->eventDispatcher->expects($this->never())->method('dispatch'); } else { @@ -239,231 +359,26 @@ function ($eventName, $event) use ($user, $newData) { ); } - $accountManager->updateUser($user, $newData); + $this->invokePrivate($accountManager, 'updateUser', [$user, $newData]); } public function dataTrueFalse() { return [ + #$newData | $oldData | $insertNew | $updateExisting [['myProperty' => ['value' => 'newData']], ['myProperty' => ['value' => 'oldData']], false, true], - [['myProperty' => ['value' => 'newData']], [], true, false], [['myProperty' => ['value' => 'oldData']], ['myProperty' => ['value' => 'oldData']], false, false] ]; } - public function updateUserSetScopeProvider() { - return [ - // regular scope switching - [ - [ - IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_PUBLISHED], - IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_PUBLISHED], - IAccountManager::PROPERTY_AVATAR => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PUBLISHED], - 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://example.org', 'scope' => IAccountManager::SCOPE_PRIVATE], - ], - [ - IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_LOCAL], - IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_FEDERATED], - IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PRIVATE], - IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_LOCAL], - IAccountManager::PROPERTY_ADDRESS => ['value' => 'some street', 'scope' => IAccountManager::SCOPE_FEDERATED], - IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::SCOPE_PUBLISHED], - ], - [ - IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_LOCAL], - IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_FEDERATED], - IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PRIVATE], - IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_LOCAL], - IAccountManager::PROPERTY_ADDRESS => ['value' => 'some street', 'scope' => IAccountManager::SCOPE_FEDERATED], - IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::SCOPE_PUBLISHED], - ], - ], - // legacy scope mapping, the given visibility values get converted to scopes - [ - [ - IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PUBLISHED], - IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_FEDERATED], - IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::SCOPE_PRIVATE], - ], - [ - IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::VISIBILITY_PUBLIC], - IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::VISIBILITY_CONTACTS_ONLY], - IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::VISIBILITY_PRIVATE], - ], - [ - IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PUBLISHED], - IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_FEDERATED], - IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::SCOPE_LOCAL], - ], - ], - // invalid or unsupported scope values get converted to SCOPE_LOCAL - [ - [ - IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_PUBLISHED], - IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_PUBLISHED], - IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PUBLISHED], - IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_FEDERATED], - ], - [ - // SCOPE_PRIVATE is not allowed for display name and email - IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_PRIVATE], - IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_PRIVATE], - IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_LOCAL], - IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_LOCAL], - ], - [ - IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_LOCAL], - IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_LOCAL], - IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_LOCAL], - IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_LOCAL], - ], - false, false, - ], - // illegal scope values - [ - [ - IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => IAccountManager::SCOPE_FEDERATED], - IAccountManager::PROPERTY_ADDRESS => ['value' => 'some street', 'scope' => IAccountManager::SCOPE_LOCAL], - IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => IAccountManager::SCOPE_PRIVATE], - ], - [ - IAccountManager::PROPERTY_PHONE => ['value' => '+491601231212', 'scope' => ''], - IAccountManager::PROPERTY_ADDRESS => ['value' => 'some street', 'scope' => 'v2-invalid'], - IAccountManager::PROPERTY_WEBSITE => ['value' => 'https://example.org', 'scope' => 'invalid'], - ], - [], - true, true - ], - // invalid or unsupported scope values throw an exception when passing $throwOnData=true - [ - [IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_PUBLISHED]], - [IAccountManager::PROPERTY_DISPLAYNAME => ['value' => 'Display Name', 'scope' => IAccountManager::SCOPE_PRIVATE]], - null, - // throw exception - true, true, - ], - [ - [IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_PUBLISHED]], - [IAccountManager::PROPERTY_EMAIL => ['value' => 'test@example.org', 'scope' => IAccountManager::SCOPE_PRIVATE]], - null, - // throw exception - true, true, - ], - [ - [IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => IAccountManager::SCOPE_PUBLISHED]], - [IAccountManager::PROPERTY_TWITTER => ['value' => '@sometwitter', 'scope' => 'invalid']], - null, - // throw exception - true, true, - ], - ]; - } - - /** - * @dataProvider updateUserSetScopeProvider - */ - public function testUpdateUserSetScope($oldData, $newData, $savedData, $throwOnData = true, $expectedThrow = false) { - $accountManager = $this->getInstance(['getUser', 'insertNewUser', 'updateExistingUser', 'updateVerifyStatus', 'checkEmailVerification']); - /** @var IUser $user */ - $user = $this->createMock(IUser::class); - - $accountManager->expects($this->once())->method('getUser')->with($user)->willReturn($oldData); - - if ($expectedThrow) { - $accountManager->expects($this->never())->method('updateExistingUser'); - $this->expectException(\InvalidArgumentException::class); - $this->expectExceptionMessage('scope'); - } else { - $accountManager->expects($this->once())->method('checkEmailVerification') - ->with($oldData, $savedData, $user)->willReturn($savedData); - $accountManager->expects($this->once())->method('updateVerifyStatus') - ->with($oldData, $savedData)->willReturn($savedData); - $accountManager->expects($this->once())->method('updateExistingUser') - ->with($user, $savedData); - $accountManager->expects($this->never())->method('insertNewUser'); - } - - $accountManager->updateUser($user, $newData, $throwOnData); - } - - /** - * @dataProvider dataTestGetUser - * - * @param string $setUser - * @param array $setData - * @param IUser $askUser - * @param array $expectedData - * @param bool $userAlreadyExists - */ - public function testGetUser($setUser, $setData, $askUser, $expectedData, $userAlreadyExists) { - $accountManager = $this->getInstance(['buildDefaultUserRecord', 'insertNewUser', 'addMissingDefaultValues']); - if (!$userAlreadyExists) { - $accountManager->expects($this->once())->method('buildDefaultUserRecord') - ->with($askUser)->willReturn($expectedData); - $accountManager->expects($this->once())->method('insertNewUser') - ->with($askUser, $expectedData); - } - - if (empty($expectedData)) { - $accountManager->expects($this->never())->method('addMissingDefaultValues'); - } else { - $accountManager->expects($this->once())->method('addMissingDefaultValues')->with($expectedData) - ->willReturn($expectedData); - } - - $this->addDummyValuesToTable($setUser, $setData); - $this->assertEquals($expectedData, - $accountManager->getUser($askUser) - ); - } - - public function dataTestGetUser() { - $user1 = $this->getMockBuilder(IUser::class)->getMock(); - $user1->expects($this->any())->method('getUID')->willReturn('user1'); - $user2 = $this->getMockBuilder(IUser::class)->getMock(); - $user2->expects($this->any())->method('getUID')->willReturn('user2'); - return [ - ['user1', ['key' => 'value'], $user1, ['key' => 'value'], true], - ['user1', ['key' => 'value'], $user2, [], false], - ]; - } - - public function testUpdateExistingUser() { - $user = $this->getMockBuilder(IUser::class)->getMock(); - $user->expects($this->atLeastOnce())->method('getUID')->willReturn('uid'); - $oldData = ['key' => ['value' => 'value']]; - $newData = ['newKey' => ['value' => 'newValue']]; - - $this->addDummyValuesToTable('uid', $oldData); - $this->invokePrivate($this->accountManager, 'updateExistingUser', [$user, $newData]); - $newDataFromTable = $this->getDataFromTable('uid'); - $this->assertEquals($newData, $newDataFromTable); - } - - public function testInsertNewUser() { - $user = $this->getMockBuilder(IUser::class)->getMock(); - $uid = 'uid'; - $data = ['key' => ['value' => 'value']]; - - $user->expects($this->atLeastOnce())->method('getUID')->willReturn($uid); - $this->assertNull($this->getDataFromTable($uid)); - $this->invokePrivate($this->accountManager, 'insertNewUser', [$user, $data]); - - $dataFromDb = $this->getDataFromTable($uid); - $this->assertEquals($data, $dataFromDb); - } - public function testAddMissingDefaultValues() { $input = [ - 'key1' => ['value' => 'value1', 'verified' => '0'], - 'key2' => ['value' => 'value1'], + ['value' => 'value1', 'verified' => '0', 'name' => 'key1'], + ['value' => 'value1', 'name' => 'key2'], ]; $expected = [ - 'key1' => ['value' => 'value1', 'verified' => '0'], - 'key2' => ['value' => 'value1', 'verified' => '0'], + ['value' => 'value1', 'verified' => '0', 'name' => 'key1'], + ['value' => 'value1', 'name' => 'key2', 'verified' => '0'], ]; $result = $this->invokePrivate($this->accountManager, 'addMissingDefaultValues', [$input]); @@ -483,46 +398,30 @@ private function addDummyValuesToTable($uid, $data) { ->execute(); } - private function getDataFromTable($uid) { - $query = $this->connection->getQueryBuilder(); - $query->select('data')->from($this->table) - ->where($query->expr()->eq('uid', $query->createParameter('uid'))) - ->setParameter('uid', $uid); - $query->execute(); - - $qResult = $query->execute(); - $result = $qResult->fetchAll(); - $qResult->closeCursor(); - - if (!empty($result)) { - return json_decode($result[0]['data'], true); - } - } - public function testGetAccount() { $accountManager = $this->getInstance(['getUser']); /** @var IUser $user */ $user = $this->createMock(IUser::class); $data = [ - IAccountManager::PROPERTY_TWITTER => - [ - 'value' => '@twitterhandle', - 'scope' => IAccountManager::SCOPE_LOCAL, - 'verified' => IAccountManager::NOT_VERIFIED, - ], - IAccountManager::PROPERTY_EMAIL => - [ - 'value' => 'test@example.com', - 'scope' => IAccountManager::SCOPE_PUBLISHED, - 'verified' => IAccountManager::VERIFICATION_IN_PROGRESS, - ], - IAccountManager::PROPERTY_WEBSITE => - [ - 'value' => 'https://example.com', - 'scope' => IAccountManager::SCOPE_FEDERATED, - 'verified' => IAccountManager::VERIFIED, - ], + [ + 'value' => '@twitterhandle', + 'scope' => IAccountManager::SCOPE_LOCAL, + 'verified' => IAccountManager::NOT_VERIFIED, + 'name' => IAccountManager::PROPERTY_TWITTER, + ], + [ + 'value' => 'test@example.com', + 'scope' => IAccountManager::SCOPE_PUBLISHED, + 'verified' => IAccountManager::VERIFICATION_IN_PROGRESS, + 'name' => IAccountManager::PROPERTY_EMAIL, + ], + [ + 'value' => 'https://example.com', + 'scope' => IAccountManager::SCOPE_FEDERATED, + 'verified' => IAccountManager::VERIFIED, + 'name' => IAccountManager::PROPERTY_WEBSITE, + ], ]; $expected = new Account($user); $expected->setProperty(IAccountManager::PROPERTY_TWITTER, '@twitterhandle', IAccountManager::SCOPE_LOCAL, IAccountManager::NOT_VERIFIED); diff --git a/tests/lib/Accounts/HooksTest.php b/tests/lib/Accounts/HooksTest.php index 8af9e209034df..d5c7cd60b1b2d 100644 --- a/tests/lib/Accounts/HooksTest.php +++ b/tests/lib/Accounts/HooksTest.php @@ -23,7 +23,9 @@ use OC\Accounts\AccountManager; use OC\Accounts\Hooks; +use OCP\Accounts\IAccount; use OCP\Accounts\IAccountManager; +use OCP\Accounts\IAccountProperty; use OCP\IUser; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; @@ -43,7 +45,7 @@ class HooksTest extends TestCase { /** @var AccountManager|MockObject */ private $accountManager; - /** @var Hooks|MockObject */ + /** @var Hooks */ private $hooks; protected function setUp(): void { @@ -53,12 +55,7 @@ protected function setUp(): void { $this->accountManager = $this->getMockBuilder(AccountManager::class) ->disableOriginalConstructor()->getMock(); - $this->hooks = $this->getMockBuilder(Hooks::class) - ->setConstructorArgs([$this->logger]) - ->setMethods(['getAccountManager']) - ->getMock(); - - $this->hooks->method('getAccountManager')->willReturn($this->accountManager); + $this->hooks = new Hooks($this->logger, $this->accountManager); } /** @@ -72,48 +69,57 @@ protected function setUp(): void { */ public function testChangeUserHook($params, $data, $setEmail, $setDisplayName, $error) { if ($error) { - $this->accountManager->expects($this->never())->method('getUser'); - $this->accountManager->expects($this->never())->method('updateUser'); + $this->accountManager->expects($this->never())->method('updateAccount'); } else { - $this->accountManager->expects($this->once())->method('getUser')->willReturn($data); - $newData = $data; + $account = $this->createMock(IAccount::class); + $this->accountManager->expects($this->atLeastOnce())->method('getAccount')->willReturn($account); if ($setEmail) { - $newData[IAccountManager::PROPERTY_EMAIL]['value'] = $params['value']; - $this->accountManager->expects($this->once())->method('updateUser') - ->with($params['user'], $newData); + $property = $this->createMock(IAccountProperty::class); + $property->expects($this->atLeastOnce()) + ->method('getValue') + ->willReturn($data[IAccountManager::PROPERTY_EMAIL]['value']); + $property->expects($this->atLeastOnce()) + ->method('setValue') + ->with($params['value']); + + $account->expects($this->atLeastOnce()) + ->method('getProperty') + ->with(IAccountManager::PROPERTY_EMAIL) + ->willReturn($property); + + $this->accountManager->expects($this->once()) + ->method('updateAccount') + ->with($account); } elseif ($setDisplayName) { - $newData[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $params['value']; - $this->accountManager->expects($this->once())->method('updateUser') - ->with($params['user'], $newData); + $property = $this->createMock(IAccountProperty::class); + $property->expects($this->atLeastOnce()) + ->method('getValue') + ->willReturn($data[IAccountManager::PROPERTY_DISPLAYNAME]['value']); + $property->expects($this->atLeastOnce()) + ->method('setValue') + ->with($params['value']); + + $account->expects($this->atLeastOnce()) + ->method('getProperty') + ->with(IAccountManager::PROPERTY_DISPLAYNAME) + ->willReturn($property); + + $this->accountManager->expects($this->once()) + ->method('updateAccount') + ->with($account); } else { - $this->accountManager->expects($this->never())->method('updateUser'); + $this->accountManager->expects($this->never())->method('updateAccount'); } } - $this->hooks->changeUserHook($params); + $this->hooks->changeUserHook($params['user'], $params['feature'], $params['value']); } public function dataTestChangeUserHook() { $user = $this->createMock(IUser::class); return [ [ - ['feature' => '', 'value' => ''], - [ - IAccountManager::PROPERTY_EMAIL => ['value' => ''], - IAccountManager::PROPERTY_DISPLAYNAME => ['value' => ''] - ], - false, false, true - ], - [ - ['user' => $user, 'value' => ''], - [ - IAccountManager::PROPERTY_EMAIL => ['value' => ''], - IAccountManager::PROPERTY_DISPLAYNAME => ['value' => ''] - ], - false, false, true - ], - [ - ['user' => $user, 'feature' => ''], + ['user' => $user, 'feature' => '', 'value' => ''], [ IAccountManager::PROPERTY_EMAIL => ['value' => ''], IAccountManager::PROPERTY_DISPLAYNAME => ['value' => ''] @@ -146,10 +152,4 @@ public function dataTestChangeUserHook() { ], ]; } - - public function testGetAccountManager() { - $hooks = new Hooks($this->logger); - $result = $this->invokePrivate($hooks, 'getAccountManager'); - $this->assertInstanceOf(AccountManager::class, $result); - } }