diff options
31 files changed, 1613 insertions, 673 deletions
diff --git a/apps/encryption/appinfo/info.xml b/apps/encryption/appinfo/info.xml index c740afda506..926abbb6959 100644 --- a/apps/encryption/appinfo/info.xml +++ b/apps/encryption/appinfo/info.xml @@ -45,6 +45,7 @@ <command>OCA\Encryption\Command\DisableMasterKey</command> <command>OCA\Encryption\Command\RecoverUser</command> <command>OCA\Encryption\Command\ScanLegacyFormat</command> + <command>OCA\Encryption\Command\FixEncryptedVersion</command> </commands> <settings> diff --git a/apps/encryption/composer/composer/autoload_classmap.php b/apps/encryption/composer/composer/autoload_classmap.php index 7d5b84f6147..00c57e913a3 100644 --- a/apps/encryption/composer/composer/autoload_classmap.php +++ b/apps/encryption/composer/composer/autoload_classmap.php @@ -10,6 +10,7 @@ return array( 'OCA\\Encryption\\AppInfo\\Application' => $baseDir . '/../lib/AppInfo/Application.php', 'OCA\\Encryption\\Command\\DisableMasterKey' => $baseDir . '/../lib/Command/DisableMasterKey.php', 'OCA\\Encryption\\Command\\EnableMasterKey' => $baseDir . '/../lib/Command/EnableMasterKey.php', + 'OCA\\Encryption\\Command\\FixEncryptedVersion' => $baseDir . '/../lib/Command/FixEncryptedVersion.php', 'OCA\\Encryption\\Command\\RecoverUser' => $baseDir . '/../lib/Command/RecoverUser.php', 'OCA\\Encryption\\Command\\ScanLegacyFormat' => $baseDir . '/../lib/Command/ScanLegacyFormat.php', 'OCA\\Encryption\\Controller\\RecoveryController' => $baseDir . '/../lib/Controller/RecoveryController.php', diff --git a/apps/encryption/composer/composer/autoload_static.php b/apps/encryption/composer/composer/autoload_static.php index 64d608a6457..fc1fcbcf63b 100644 --- a/apps/encryption/composer/composer/autoload_static.php +++ b/apps/encryption/composer/composer/autoload_static.php @@ -25,6 +25,7 @@ class ComposerStaticInitEncryption 'OCA\\Encryption\\AppInfo\\Application' => __DIR__ . '/..' . '/../lib/AppInfo/Application.php', 'OCA\\Encryption\\Command\\DisableMasterKey' => __DIR__ . '/..' . '/../lib/Command/DisableMasterKey.php', 'OCA\\Encryption\\Command\\EnableMasterKey' => __DIR__ . '/..' . '/../lib/Command/EnableMasterKey.php', + 'OCA\\Encryption\\Command\\FixEncryptedVersion' => __DIR__ . '/..' . '/../lib/Command/FixEncryptedVersion.php', 'OCA\\Encryption\\Command\\RecoverUser' => __DIR__ . '/..' . '/../lib/Command/RecoverUser.php', 'OCA\\Encryption\\Command\\ScanLegacyFormat' => __DIR__ . '/..' . '/../lib/Command/ScanLegacyFormat.php', 'OCA\\Encryption\\Controller\\RecoveryController' => __DIR__ . '/..' . '/../lib/Controller/RecoveryController.php', diff --git a/apps/encryption/lib/Command/FixEncryptedVersion.php b/apps/encryption/lib/Command/FixEncryptedVersion.php new file mode 100644 index 00000000000..a85a96258fc --- /dev/null +++ b/apps/encryption/lib/Command/FixEncryptedVersion.php @@ -0,0 +1,286 @@ +<?php +/** + * @author Sujith Haridasan <sharidasan@owncloud.com> + * @author Ilja Neumann <ineumann@owncloud.com> + * + * @copyright Copyright (c) 2019, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see <http://www.gnu.org/licenses/> + * + */ + +namespace OCA\Encryption\Command; + +use OC\Files\View; +use OC\HintException; +use OCA\Encryption\Util; +use OCP\Files\IRootFolder; +use OCP\IConfig; +use OCP\ILogger; +use OCP\IUserManager; +use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Input\InputArgument; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Output\OutputInterface; + +class FixEncryptedVersion extends Command { + /** @var IConfig */ + private $config; + + /** @var ILogger */ + private $logger; + + /** @var IRootFolder */ + private $rootFolder; + + /** @var IUserManager */ + private $userManager; + + /** @var Util */ + private $util; + + /** @var View */ + private $view; + + public function __construct( + IConfig $config, + ILogger $logger, + IRootFolder $rootFolder, + IUserManager $userManager, + Util $util, + View $view + ) { + $this->config = $config; + $this->logger = $logger; + $this->rootFolder = $rootFolder; + $this->userManager = $userManager; + $this->util = $util; + $this->view = $view; + parent::__construct(); + } + + protected function configure(): void { + parent::configure(); + + $this + ->setName('encryption:fix-encrypted-version') + ->setDescription('Fix the encrypted version if the encrypted file(s) are not downloadable.') + ->addArgument( + 'user', + InputArgument::REQUIRED, + 'The id of the user whose files need fixing' + )->addOption( + 'path', + 'p', + InputArgument::OPTIONAL, + 'Limit files to fix with path, e.g., --path="/Music/Artist". If path indicates a directory, all the files inside directory will be fixed.' + ); + } + + /** + * @param InputInterface $input + * @param OutputInterface $output + * @return int + */ + protected function execute(InputInterface $input, OutputInterface $output): int { + $skipSignatureCheck = $this->config->getSystemValue('encryption_skip_signature_check', false); + + if ($skipSignatureCheck) { + $output->writeln("<error>Repairing is not possible when \"encryption_skip_signature_check\" is set. Please disable this flag in the configuration.</error>\n"); + return 1; + } + + if (!$this->util->isMasterKeyEnabled()) { + $output->writeln("<error>Repairing only works with master key encryption.</error>\n"); + return 1; + } + + $user = (string)$input->getArgument('user'); + $pathToWalk = "/$user/files"; + + /** + * trim() returns an empty string when the argument is an unset/null + */ + $pathOption = \trim($input->getOption('path'), '/'); + if ($pathOption !== "") { + $pathToWalk = "$pathToWalk/$pathOption"; + } + + if ($user === null) { + $output->writeln("<error>No user id provided.</error>\n"); + return 1; + } + + if ($this->userManager->get($user) === null) { + $output->writeln("<error>User id $user does not exist. Please provide a valid user id</error>"); + return 1; + } + return $this->walkPathOfUser($user, $pathToWalk, $output); + } + + /** + * @param string $user + * @param string $path + * @param OutputInterface $output + * @return int 0 for success, 1 for error + */ + private function walkPathOfUser($user, $path, OutputInterface $output): int { + $this->setupUserFs($user); + if (!$this->view->file_exists($path)) { + $output->writeln("<error>Path \"$path\" does not exist. Please provide a valid path.</error>"); + return 1; + } + + if ($this->view->is_file($path)) { + $output->writeln("Verifying the content of file \"$path\""); + $this->verifyFileContent($path, $output); + return 0; + } + $directories = []; + $directories[] = $path; + while ($root = \array_pop($directories)) { + $directoryContent = $this->view->getDirectoryContent($root); + foreach ($directoryContent as $file) { + $path = $root . '/' . $file['name']; + if ($this->view->is_dir($path)) { + $directories[] = $path; + } else { + $output->writeln("Verifying the content of file \"$path\""); + $this->verifyFileContent($path, $output); + } + } + } + return 0; + } + + /** + * @param string $path + * @param OutputInterface $output + * @param bool $ignoreCorrectEncVersionCall, setting this variable to false avoids recursion + */ + private function verifyFileContent($path, OutputInterface $output, $ignoreCorrectEncVersionCall = true): bool { + try { + /** + * In encryption, the files are read in a block size of 8192 bytes + * Read block size of 8192 and a bit more (808 bytes) + * If there is any problem, the first block should throw the signature + * mismatch error. Which as of now, is enough to proceed ahead to + * correct the encrypted version. + */ + $handle = $this->view->fopen($path, 'rb'); + + if (\fread($handle, 9001) !== false) { + $output->writeln("<info>The file \"$path\" is: OK</info>"); + } + + \fclose($handle); + + return true; + } catch (HintException $e) { + $this->logger->warning("Issue: " . $e->getMessage()); + //If allowOnce is set to false, this becomes recursive. + if ($ignoreCorrectEncVersionCall === true) { + //Lets rectify the file by correcting encrypted version + $output->writeln("<info>Attempting to fix the path: \"$path\"</info>"); + return $this->correctEncryptedVersion($path, $output); + } + return false; + } + } + + /** + * @param string $path + * @param OutputInterface $output + * @return bool + */ + private function correctEncryptedVersion($path, OutputInterface $output): bool { + $fileInfo = $this->view->getFileInfo($path); + if (!$fileInfo) { + $output->writeln("<warning>File info not found for file: \"$path\"</warning>"); + return true; + } + $fileId = $fileInfo->getId(); + $encryptedVersion = $fileInfo->getEncryptedVersion(); + $wrongEncryptedVersion = $encryptedVersion; + + $storage = $fileInfo->getStorage(); + + $cache = $storage->getCache(); + $fileCache = $cache->get($fileId); + if (!$fileCache) { + $output->writeln("<warning>File cache entry not found for file: \"$path\"</warning>"); + return true; + } + + if ($storage->instanceOfStorage('OCA\Files_Sharing\ISharedStorage')) { + $output->writeln("<info>The file: \"$path\" is a share. Please also run the script for the owner of the share</info>"); + return true; + } + + // Save original encrypted version so we can restore it if decryption fails with all version + $originalEncryptedVersion = $encryptedVersion; + if ($encryptedVersion >= 0) { + //test by decrementing the value till 1 and if nothing works try incrementing + $encryptedVersion--; + while ($encryptedVersion > 0) { + $cacheInfo = ['encryptedVersion' => $encryptedVersion, 'encrypted' => $encryptedVersion]; + $cache->put($fileCache->getPath(), $cacheInfo); + $output->writeln("<info>Decrement the encrypted version to $encryptedVersion</info>"); + if ($this->verifyFileContent($path, $output, false) === true) { + $output->writeln("<info>Fixed the file: \"$path\" with version " . $encryptedVersion . "</info>"); + return true; + } + $encryptedVersion--; + } + + //So decrementing did not work. Now lets increment. Max increment is till 5 + $increment = 1; + while ($increment <= 5) { + /** + * The wrongEncryptedVersion would not be incremented so nothing to worry about here. + * Only the newEncryptedVersion is incremented. + * For example if the wrong encrypted version is 4 then + * cycle1 -> newEncryptedVersion = 5 ( 4 + 1) + * cycle2 -> newEncryptedVersion = 6 ( 4 + 2) + * cycle3 -> newEncryptedVersion = 7 ( 4 + 3) + */ + $newEncryptedVersion = $wrongEncryptedVersion + $increment; + + $cacheInfo = ['encryptedVersion' => $newEncryptedVersion, 'encrypted' => $newEncryptedVersion]; + $cache->put($fileCache->getPath(), $cacheInfo); + $output->writeln("<info>Increment the encrypted version to $newEncryptedVersion</info>"); + if ($this->verifyFileContent($path, $output, false) === true) { + $output->writeln("<info>Fixed the file: \"$path\" with version " . $newEncryptedVersion . "</info>"); + return true; + } + $increment++; + } + } + + $cacheInfo = ['encryptedVersion' => $originalEncryptedVersion, 'encrypted' => $originalEncryptedVersion]; + $cache->put($fileCache->getPath(), $cacheInfo); + $output->writeln("<info>No fix found for \"$path\", restored version to original: $originalEncryptedVersion</info>"); + + return false; + } + + /** + * Setup user file system + * @param string $uid + */ + private function setupUserFs($uid): void { + \OC_Util::tearDownFS(); + \OC_Util::setupFS($uid); + } +} diff --git a/apps/encryption/tests/Command/FixEncryptedVersionTest.php b/apps/encryption/tests/Command/FixEncryptedVersionTest.php new file mode 100644 index 00000000000..22ae239aec2 --- /dev/null +++ b/apps/encryption/tests/Command/FixEncryptedVersionTest.php @@ -0,0 +1,346 @@ +<?php +/** + * @author Sujith Haridasan <sharidasan@owncloud.com> + * + * @copyright Copyright (c) 2019, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see <http://www.gnu.org/licenses/> + * + */ + +namespace OCA\Encryption\Tests\Command; + +use OC\Files\View; +use OCA\Encryption\Command\FixEncryptedVersion; +use OCA\Encryption\Util; +use Symfony\Component\Console\Tester\CommandTester; +use Test\TestCase; +use Test\Traits\EncryptionTrait; +use Test\Traits\MountProviderTrait; +use Test\Traits\UserTrait; + +/** + * Class FixEncryptedVersionTest + * + * @group DB + * @package OCA\Encryption\Tests\Command + */ +class FixEncryptedVersionTest extends TestCase { + use MountProviderTrait; + use EncryptionTrait; + use UserTrait; + + private $userId; + + /** @var FixEncryptedVersion */ + private $fixEncryptedVersion; + + /** @var CommandTester */ + private $commandTester; + + /** @var Util|\PHPUnit\Framework\MockObject\MockObject */ + protected $util; + + public function setUp(): void { + parent::setUp(); + + \OC::$server->getConfig()->setAppValue('encryption', 'useMasterKey', '1'); + + $this->util = $this->getMockBuilder(Util::class) + ->disableOriginalConstructor()->getMock(); + + $this->userId = $this->getUniqueId('user_'); + + $this->createUser($this->userId, 'foo12345678'); + $tmpFolder = \OC::$server->getTempManager()->getTemporaryFolder(); + $this->registerMount($this->userId, '\OC\Files\Storage\Local', '/' . $this->userId, ['datadir' => $tmpFolder]); + $this->setupForUser($this->userId, 'foo12345678'); + $this->loginWithEncryption($this->userId); + + $this->fixEncryptedVersion = new FixEncryptedVersion( + \OC::$server->getConfig(), + \OC::$server->getLogger(), + \OC::$server->getRootFolder(), + \OC::$server->getUserManager(), + $this->util, + new View('/') + ); + $this->commandTester = new CommandTester($this->fixEncryptedVersion); + + $this->assertTrue(\OC::$server->getEncryptionManager()->isEnabled()); + $this->assertTrue(\OC::$server->getEncryptionManager()->isReady()); + $this->assertTrue(\OC::$server->getEncryptionManager()->isReadyForUser($this->userId)); + } + + /** + * In this test the encrypted version of the file is less than the original value + * but greater than zero + */ + public function testEncryptedVersionLessThanOriginalValue() { + $this->util->expects($this->once())->method('isMasterKeyEnabled') + ->willReturn(true); + + $view = new View("/" . $this->userId . "/files"); + + $view->touch('hello.txt'); + $view->touch('world.txt'); + $view->touch('foo.txt'); + $view->file_put_contents('hello.txt', 'a test string for hello'); + $view->file_put_contents('hello.txt', 'Yet another value'); + $view->file_put_contents('hello.txt', 'Lets modify again1'); + $view->file_put_contents('hello.txt', 'Lets modify again2'); + $view->file_put_contents('hello.txt', 'Lets modify again3'); + $view->file_put_contents('world.txt', 'a test string for world'); + $view->file_put_contents('world.txt', 'a test string for world'); + $view->file_put_contents('world.txt', 'a test string for world'); + $view->file_put_contents('world.txt', 'a test string for world'); + $view->file_put_contents('foo.txt', 'a foo test'); + + $fileInfo1 = $view->getFileInfo('hello.txt'); + + $storage1 = $fileInfo1->getStorage(); + $cache1 = $storage1->getCache(); + $fileCache1 = $cache1->get($fileInfo1->getId()); + + //Now change the encrypted version to two + $cacheInfo = ['encryptedVersion' => 2, 'encrypted' => 2]; + $cache1->put($fileCache1->getPath(), $cacheInfo); + + $fileInfo2 = $view->getFileInfo('world.txt'); + $storage2 = $fileInfo2->getStorage(); + $cache2 = $storage2->getCache(); + $filecache2 = $cache2->get($fileInfo2->getId()); + + //Now change the encrypted version to 1 + $cacheInfo = ['encryptedVersion' => 1, 'encrypted' => 1]; + $cache2->put($filecache2->getPath(), $cacheInfo); + + $this->commandTester->execute([ + 'user' => $this->userId + ]); + + $output = $this->commandTester->getDisplay(); + + $this->assertStringContainsString("Verifying the content of file \"/$this->userId/files/foo.txt\" +The file \"/$this->userId/files/foo.txt\" is: OK", $output); + $this->assertStringContainsString("Verifying the content of file \"/$this->userId/files/hello.txt\" +Attempting to fix the path: \"/$this->userId/files/hello.txt\" +Decrement the encrypted version to 1 +Increment the encrypted version to 3 +Increment the encrypted version to 4 +Increment the encrypted version to 5 +The file \"/$this->userId/files/hello.txt\" is: OK +Fixed the file: \"/$this->userId/files/hello.txt\" with version 5", $output); + $this->assertStringContainsString("Verifying the content of file \"/$this->userId/files/world.txt\" +Attempting to fix the path: \"/$this->userId/files/world.txt\" +Increment the encrypted version to 2 +Increment the encrypted version to 3 +Increment the encrypted version to 4 +The file \"/$this->userId/files/world.txt\" is: OK +Fixed the file: \"/$this->userId/files/world.txt\" with version 4", $output); + } + + /** + * In this test the encrypted version of the file is greater than the original value + * but greater than zero + */ + public function testEncryptedVersionGreaterThanOriginalValue() { + $this->util->expects($this->once())->method('isMasterKeyEnabled') + ->willReturn(true); + + $view = new View("/" . $this->userId . "/files"); + + $view->touch('hello.txt'); + $view->touch('world.txt'); + $view->touch('foo.txt'); + $view->file_put_contents('hello.txt', 'a test string for hello'); + $view->file_put_contents('hello.txt', 'Lets modify again2'); + $view->file_put_contents('hello.txt', 'Lets modify again3'); + $view->file_put_contents('world.txt', 'a test string for world'); + $view->file_put_contents('world.txt', 'a test string for world'); + $view->file_put_contents('world.txt', 'a test string for world'); + $view->file_put_contents('world.txt', 'a test string for world'); + $view->file_put_contents('foo.txt', 'a foo test'); + + $fileInfo1 = $view->getFileInfo('hello.txt'); + + $storage1 = $fileInfo1->getStorage(); + $cache1 = $storage1->getCache(); + $fileCache1 = $cache1->get($fileInfo1->getId()); + + //Now change the encrypted version to fifteen + $cacheInfo = ['encryptedVersion' => 5, 'encrypted' => 5]; + $cache1->put($fileCache1->getPath(), $cacheInfo); + + $fileInfo2 = $view->getFileInfo('world.txt'); + $storage2 = $fileInfo2->getStorage(); + $cache2 = $storage2->getCache(); + $filecache2 = $cache2->get($fileInfo2->getId()); + + //Now change the encrypted version to 1 + $cacheInfo = ['encryptedVersion' => 6, 'encrypted' => 6]; + $cache2->put($filecache2->getPath(), $cacheInfo); + + $this->commandTester->execute([ + 'user' => $this->userId + ]); + + $output = $this->commandTester->getDisplay(); + + $this->assertStringContainsString("Verifying the content of file \"/$this->userId/files/foo.txt\" +The file \"/$this->userId/files/foo.txt\" is: OK", $output); + $this->assertStringContainsString("Verifying the content of file \"/$this->userId/files/hello.txt\" +Attempting to fix the path: \"/$this->userId/files/hello.txt\" +Decrement the encrypted version to 4 +Decrement the encrypted version to 3 +The file \"/$this->userId/files/hello.txt\" is: OK +Fixed the file: \"/$this->userId/files/hello.txt\" with version 3", $output); + $this->assertStringContainsString("Verifying the content of file \"/$this->userId/files/world.txt\" +Attempting to fix the path: \"/$this->userId/files/world.txt\" +Decrement the encrypted version to 5 +Decrement the encrypted version to 4 +The file \"/$this->userId/files/world.txt\" is: OK +Fixed the file: \"/$this->userId/files/world.txt\" with version 4", $output); + } + + public function testVersionIsRestoredToOriginalIfNoFixIsFound() { + $this->util->expects($this->once())->method('isMasterKeyEnabled') + ->willReturn(true); + + $view = new View("/" . $this->userId . "/files"); + + $view->touch('bar.txt'); + for ($i = 0; $i < 40; $i++) { + $view->file_put_contents('bar.txt', 'a test string for hello ' . $i); + } + + $fileInfo = $view->getFileInfo('bar.txt'); + + $storage = $fileInfo->getStorage(); + $cache = $storage->getCache(); + $fileCache = $cache->get($fileInfo->getId()); + + $cacheInfo = ['encryptedVersion' => 15, 'encrypted' => 15]; + $cache->put($fileCache->getPath(), $cacheInfo); + + $this->commandTester->execute([ + 'user' => $this->userId + ]); + + $cacheInfo = $cache->get($fileInfo->getId()); + $encryptedVersion = $cacheInfo["encryptedVersion"]; + + $this->assertEquals(15, $encryptedVersion); + } + + /** + * Test commands with a file path + */ + public function testExecuteWithFilePathOption() { + $this->util->expects($this->once())->method('isMasterKeyEnabled') + ->willReturn(true); + + $view = new View("/" . $this->userId . "/files"); + + $view->touch('hello.txt'); + $view->touch('world.txt'); + + $this->commandTester->execute([ + 'user' => $this->userId, + '--path' => "/hello.txt" + ]); + + $output = $this->commandTester->getDisplay(); + + $this->assertStringContainsString("Verifying the content of file \"/$this->userId/files/hello.txt\" +The file \"/$this->userId/files/hello.txt\" is: OK", $output); + $this->assertStringNotContainsString('world.txt', $output); + } + + /** + * Test commands with a directory path + */ + public function testExecuteWithDirectoryPathOption() { + $this->util->expects($this->once())->method('isMasterKeyEnabled') + ->willReturn(true); + + $view = new View("/" . $this->userId . "/files"); + + $view->mkdir('sub'); + $view->touch('sub/hello.txt'); + $view->touch('world.txt'); + + $this->commandTester->execute([ + 'user' => $this->userId, + '--path' => "/sub" + ]); + + $output = $this->commandTester->getDisplay(); + + $this->assertStringContainsString("Verifying the content of file \"/$this->userId/files/sub/hello.txt\" +The file \"/$this->userId/files/sub/hello.txt\" is: OK", $output); + $this->assertStringNotContainsString('world.txt', $output); + } + + /** + * Test commands with a directory path + */ + public function testExecuteWithNoUser() { + $this->util->expects($this->once())->method('isMasterKeyEnabled') + ->willReturn(true); + + $this->commandTester->execute([ + 'user' => null, + '--path' => "/" + ]); + + $output = $this->commandTester->getDisplay(); + + $this->assertStringContainsString('does not exist', $output); + } + + /** + * Test commands with a directory path + */ + public function testExecuteWithNonExistentPath() { + $this->util->expects($this->once())->method('isMasterKeyEnabled') + ->willReturn(true); + + $this->commandTester->execute([ + 'user' => $this->userId, + '--path' => '/non-exist' + ]); + + $output = $this->commandTester->getDisplay(); + + $this->assertStringContainsString('Please provide a valid path.', $output); + } + + /** + * Test commands without master key + */ + public function testExecuteWithNoMasterKey() { + \OC::$server->getConfig()->setAppValue('encryption', 'useMasterKey', '0'); + $this->util->expects($this->once())->method('isMasterKeyEnabled') + ->willReturn(false); + + $this->commandTester->execute([ + 'user' => $this->userId, + ]); + + $output = $this->commandTester->getDisplay(); + + $this->assertStringContainsString('only works with master key', $output); + } +} diff --git a/apps/provisioning_api/appinfo/routes.php b/apps/provisioning_api/appinfo/routes.php index 8cf57487d3a..2f981e0c924 100644 --- a/apps/provisioning_api/appinfo/routes.php +++ b/apps/provisioning_api/appinfo/routes.php @@ -54,6 +54,7 @@ return [ ['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-9_]*$']], ['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 c4fa537c2df..e358d282061 100644 --- a/apps/provisioning_api/lib/Controller/AUserData.php +++ b/apps/provisioning_api/lib/Controller/AUserData.php @@ -150,6 +150,20 @@ abstract class AUserData extends OCSController { 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 256077e9ae9..d94e8fed8a8 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -52,7 +52,8 @@ use OC\KnownUser\KnownUserService; 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 @@ use Psr\Log\LoggerInterface; class UsersController extends AUserData { - /** @var IAppManager */ - private $appManager; /** @var IURLGenerator */ protected $urlGenerator; /** @var LoggerInterface */ @@ -98,7 +97,6 @@ class UsersController extends AUserData { IRequest $request, IUserManager $userManager, IConfig $config, - IAppManager $appManager, IGroupManager $groupManager, IUserSession $userSession, IAccountManager $accountManager, @@ -119,7 +117,6 @@ class UsersController extends AUserData { $accountManager, $l10nFactory); - $this->appManager = $appManager; $this->urlGenerator = $urlGenerator; $this->logger = $logger; $this->l10nFactory = $l10nFactory; @@ -592,6 +589,7 @@ class UsersController extends AUserData { $permittedFields[] = IAccountManager::PROPERTY_EMAIL; } + $permittedFields[] = IAccountManager::COLLECTION_EMAIL; $permittedFields[] = IAccountManager::PROPERTY_PHONE; $permittedFields[] = IAccountManager::PROPERTY_ADDRESS; $permittedFields[] = IAccountManager::PROPERTY_WEBSITE; @@ -605,6 +603,92 @@ class UsersController extends AUserData { * @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 + * @PasswordConfirmationRequired + * * edit users * * @param string $userId @@ -636,6 +720,8 @@ class UsersController extends AUserData { $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 @@ class UsersController extends AUserData { $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; } $permittedFields[] = IAccountManager::PROPERTY_EMAIL; + $permittedFields[] = IAccountManager::COLLECTION_EMAIL; $permittedFields[] = 'password'; $permittedFields[] = 'language'; $permittedFields[] = 'locale'; @@ -746,24 +833,42 @@ class UsersController extends AUserData { 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 742335a919a..238bac34307 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -50,7 +50,6 @@ use OCA\Settings\Mailer\NewUserMailHelper; 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 @@ class UsersControllerTest extends TestCase { $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 @@ class UsersControllerTest extends TestCase { $this->request, $this->userManager, $this->config, - $this->appManager, $this->groupManager, $this->userSession, $this->accountManager, @@ -395,7 +390,6 @@ class UsersControllerTest extends TestCase { $this->request, $this->userManager, $this->config, - $this->appManager, $this->groupManager, $this->userSession, $this->accountManager, @@ -1071,7 +1065,8 @@ class UsersControllerTest extends TestCase { 'backendCapabilities' => [ 'setDisplayName' => true, 'setPassword' => true, - ] + ], + 'additional_mail' => [], ]; $this->assertEquals($expected, $this->invokePrivate($this->api, 'getUserData', ['UID'])); } @@ -1198,7 +1193,8 @@ class UsersControllerTest extends TestCase { 'backendCapabilities' => [ 'setDisplayName' => true, 'setPassword' => true, - ] + ], + 'additional_mail' => [], ]; $this->assertEquals($expected, $this->invokePrivate($this->api, 'getUserData', ['UID'])); } @@ -1363,7 +1359,8 @@ class UsersControllerTest extends TestCase { 'backendCapabilities' => [ 'setDisplayName' => false, 'setPassword' => false, - ] + ], + 'additional_mail' => [], ]; $this->assertEquals($expected, $this->invokePrivate($this->api, 'getUserData', ['UID'])); } @@ -3437,7 +3434,6 @@ class UsersControllerTest extends TestCase { $this->request, $this->userManager, $this->config, - $this->appManager, $this->groupManager, $this->userSession, $this->accountManager, @@ -3510,7 +3506,6 @@ class UsersControllerTest extends TestCase { $this->request, $this->userManager, $this->config, - $this->appManager, $this->groupManager, $this->userSession, $this->accountManager, @@ -3848,6 +3843,7 @@ class UsersControllerTest extends TestCase { public function dataGetEditableFields() { return [ [false, ISetDisplayNameBackend::class, [ + IAccountManager::COLLECTION_EMAIL, IAccountManager::PROPERTY_PHONE, IAccountManager::PROPERTY_ADDRESS, IAccountManager::PROPERTY_WEBSITE, @@ -3856,6 +3852,7 @@ class UsersControllerTest extends TestCase { [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 @@ class UsersControllerTest extends TestCase { ]], [true, UserInterface::class, [ IAccountManager::PROPERTY_EMAIL, + IAccountManager::COLLECTION_EMAIL, IAccountManager::PROPERTY_PHONE, IAccountManager::PROPERTY_ADDRESS, IAccountManager::PROPERTY_WEBSITE, diff --git a/apps/theming/lib/Command/UpdateConfig.php b/apps/theming/lib/Command/UpdateConfig.php index 3d5840fadd9..1ff75b5ba70 100644 --- a/apps/theming/lib/Command/UpdateConfig.php +++ b/apps/theming/lib/Command/UpdateConfig.php @@ -127,6 +127,11 @@ class UpdateConfig extends Command { $key = $key . 'Mime'; } + if ($key === 'color' && !preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $value)) { + $output->writeln('<error>The given color is invalid: ' . $value . '</error>'); + return 1; + } + $this->themingDefaults->set($key, $value); $output->writeln('<info>Updated ' . $key . ' to ' . $value . '</info>'); diff --git a/apps/theming/lib/ThemingDefaults.php b/apps/theming/lib/ThemingDefaults.php index 7e7d9a4fa13..c2ed6e305eb 100644 --- a/apps/theming/lib/ThemingDefaults.php +++ b/apps/theming/lib/ThemingDefaults.php @@ -220,7 +220,11 @@ class ThemingDefaults extends \OC_Defaults { * @return string */ public function getColorPrimary() { - return $this->config->getAppValue('theming', 'color', $this->color); + $color = $this->config->getAppValue('theming', 'color', $this->color); + if (!preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $color)) { + $color = '#0082c9'; + } + return $color; } /** diff --git a/apps/user_ldap/appinfo/info.xml b/apps/user_ldap/appinfo/info.xml index a3f126c196e..014b99a70e4 100644 --- a/apps/user_ldap/appinfo/info.xml +++ b/apps/user_ldap/appinfo/info.xml @@ -37,6 +37,9 @@ A user logs into Nextcloud with their LDAP or AD credentials, and is granted acc <install> <step>OCA\User_LDAP\Migration\SetDefaultProvider</step> </install> + <uninstall> + <step>OCA\User_LDAP\Migration\UnsetDefaultProvider</step> + </uninstall> <post-migration> <step>OCA\User_LDAP\Migration\UUIDFixInsert</step> <step>OCA\User_LDAP\Migration\RemoveRefreshTime</step> diff --git a/apps/user_ldap/composer/composer/autoload_classmap.php b/apps/user_ldap/composer/composer/autoload_classmap.php index 45af1123b57..34f17532e5b 100644 --- a/apps/user_ldap/composer/composer/autoload_classmap.php +++ b/apps/user_ldap/composer/composer/autoload_classmap.php @@ -59,6 +59,7 @@ return array( 'OCA\\User_LDAP\\Migration\\UUIDFixGroup' => $baseDir . '/../lib/Migration/UUIDFixGroup.php', 'OCA\\User_LDAP\\Migration\\UUIDFixInsert' => $baseDir . '/../lib/Migration/UUIDFixInsert.php', 'OCA\\User_LDAP\\Migration\\UUIDFixUser' => $baseDir . '/../lib/Migration/UUIDFixUser.php', + 'OCA\\User_LDAP\\Migration\\UnsetDefaultProvider' => $baseDir . '/../lib/Migration/UnsetDefaultProvider.php', 'OCA\\User_LDAP\\Migration\\Version1010Date20200630192842' => $baseDir . '/../lib/Migration/Version1010Date20200630192842.php', 'OCA\\User_LDAP\\Notification\\Notifier' => $baseDir . '/../lib/Notification/Notifier.php', 'OCA\\User_LDAP\\PagedResults\\IAdapter' => $baseDir . '/../lib/PagedResults/IAdapter.php', diff --git a/apps/user_ldap/composer/composer/autoload_static.php b/apps/user_ldap/composer/composer/autoload_static.php index bf95d3a5a03..1973a8b3183 100644 --- a/apps/user_ldap/composer/composer/autoload_static.php +++ b/apps/user_ldap/composer/composer/autoload_static.php @@ -74,6 +74,7 @@ class ComposerStaticInitUser_LDAP 'OCA\\User_LDAP\\Migration\\UUIDFixGroup' => __DIR__ . '/..' . '/../lib/Migration/UUIDFixGroup.php', 'OCA\\User_LDAP\\Migration\\UUIDFixInsert' => __DIR__ . '/..' . '/../lib/Migration/UUIDFixInsert.php', 'OCA\\User_LDAP\\Migration\\UUIDFixUser' => __DIR__ . '/..' . '/../lib/Migration/UUIDFixUser.php', + 'OCA\\User_LDAP\\Migration\\UnsetDefaultProvider' => __DIR__ . '/..' . '/../lib/Migration/UnsetDefaultProvider.php', 'OCA\\User_LDAP\\Migration\\Version1010Date20200630192842' => __DIR__ . '/..' . '/../lib/Migration/Version1010Date20200630192842.php', 'OCA\\User_LDAP\\Notification\\Notifier' => __DIR__ . '/..' . '/../lib/Notification/Notifier.php', 'OCA\\User_LDAP\\PagedResults\\IAdapter' => __DIR__ . '/..' . '/../lib/PagedResults/IAdapter.php', diff --git a/apps/user_ldap/lib/Migration/UnsetDefaultProvider.php b/apps/user_ldap/lib/Migration/UnsetDefaultProvider.php new file mode 100644 index 00000000000..a696b815856 --- /dev/null +++ b/apps/user_ldap/lib/Migration/UnsetDefaultProvider.php @@ -0,0 +1,52 @@ +<?php + +declare(strict_types=1); + +/** + * @copyright 2021 Arthur Schiwon <blizzz@arthur-schiwon.de> + * + * @author Arthur Schiwon <blizzz@arthur-schiwon.de> + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + * + */ +namespace OCA\User_LDAP\Migration; + +use OCA\User_LDAP\LDAPProviderFactory; +use OCP\IConfig; +use OCP\Migration\IOutput; +use OCP\Migration\IRepairStep; + +class UnsetDefaultProvider implements IRepairStep { + + /** @var IConfig */ + private $config; + + public function __construct(IConfig $config) { + $this->config = $config; + } + + public function getName(): string { + return 'Unset default LDAP provider'; + } + + public function run(IOutput $output): void { + $current = $this->config->getSystemValue('ldapProviderFactory', null); + if ($current === LDAPProviderFactory::class) { + $this->config->deleteSystemValue('ldapProviderFactory'); + } + } +} diff --git a/build/integration/features/bootstrap/Provisioning.php b/build/integration/features/bootstrap/Provisioning.php index 8eab793d66b..e51339c081e 100644 --- a/build/integration/features/bootstrap/Provisioning.php +++ b/build/integration/features/bootstrap/Provisioning.php @@ -168,14 +168,19 @@ trait Provisioning { $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 @@ trait Provisioning { $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 @@ trait Provisioning { } } } - + /** * @Then /^user "([^"]*)" has editable fields$/ @@ -967,4 +972,38 @@ trait Provisioning { } $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 dec6d2213e3..41041dc91da 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/core/css/apps.scss b/core/css/apps.scss index cb2b32611b1..370784c1785 100644 --- a/core/css/apps.scss +++ b/core/css/apps.scss @@ -729,8 +729,10 @@ $min-content-width: $breakpoint-mobile - $navigation-width - $list-min-width; border: 0; border-radius: 0; text-align: left; - padding-left: 42px; + padding-left: 44px; font-weight: normal; + font-size: 100%; + opacity: 0.8; /* like app-navigation a */ color: var(--color-main-text); diff --git a/lib/base.php b/lib/base.php index 20065006a8c..473a3419cb1 100644 --- a/lib/base.php +++ b/lib/base.php @@ -68,6 +68,7 @@ use OCP\Share; 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 @@ class OC { } 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 7d2a51c7d4e..1e4189f2b35 100644 --- a/lib/private/Accounts/Account.php +++ b/lib/private/Accounts/Account.php @@ -33,6 +33,7 @@ use OCP\Accounts\IAccountProperty; use OCP\Accounts\IAccountPropertyCollection; use OCP\Accounts\PropertyDoesNotExistException; use OCP\IUser; +use RuntimeException; class Account implements IAccount { use TAccountsHelper; @@ -116,13 +117,16 @@ class Account implements IAccount { 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 4d75c94346b..9fc5accfa08 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 libphonenumber\PhoneNumberUtil; 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 Psr\Log\LoggerInterface; 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 @@ class AccountManager implements IAccountManager { /** * @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 @@ class AccountManager implements IAccountManager { 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 @@ class AccountManager implements IAccountManager { } 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 @@ class AccountManager implements IAccountManager { /** * 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 @@ class AccountManager implements IAccountManager { 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 @@ class AccountManager implements IAccountManager { $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 @@ class AccountManager implements IAccountManager { /** * 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 @@ class AccountManager implements IAccountManager { */ 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 @@ class AccountManager implements IAccountManager { '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 @@ class AccountManager implements IAccountManager { */ 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 @@ class AccountManager implements IAccountManager { $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 @@ class AccountManager implements IAccountManager { */ 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 @@ class AccountManager implements IAccountManager { } 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 84e10e6a507..eb92536a6a0 100644 --- a/lib/private/Accounts/AccountPropertyCollection.php +++ b/lib/private/Accounts/AccountPropertyCollection.php @@ -27,6 +27,7 @@ declare(strict_types=1); namespace OC\Accounts; use InvalidArgumentException; +use OCP\Accounts\IAccountManager; use OCP\Accounts\IAccountProperty; use OCP\Accounts\IAccountPropertyCollection; @@ -63,6 +64,18 @@ class AccountPropertyCollection implements IAccountPropertyCollection { 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 af078bd1db0..93918284180 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/AppFramework/Bootstrap/RegistrationContext.php b/lib/private/AppFramework/Bootstrap/RegistrationContext.php index dacba0aba93..30ac63281d1 100644 --- a/lib/private/AppFramework/Bootstrap/RegistrationContext.php +++ b/lib/private/AppFramework/Bootstrap/RegistrationContext.php @@ -305,12 +305,20 @@ class RegistrationContext { */ public function delegateCapabilityRegistrations(array $apps): void { while (($registration = array_shift($this->capabilities)) !== null) { + $appId = $registration->getAppId(); + if (!isset($apps[$appId])) { + // If we land here something really isn't right. But at least we caught the + // notice that is otherwise emitted for the undefined index + $this->logger->error("App $appId not loaded for the capability registration"); + + continue; + } + try { - $apps[$registration->getAppId()] + $apps[$appId] ->getContainer() ->registerCapability($registration->getService()); } catch (Throwable $e) { - $appId = $registration->getAppId(); $this->logger->error("Error during capability registration of $appId: " . $e->getMessage(), [ 'exception' => $e, ]); @@ -372,11 +380,20 @@ class RegistrationContext { */ public function delegateContainerRegistrations(array $apps): void { while (($registration = array_shift($this->services)) !== null) { + $appId = $registration->getAppId(); + if (!isset($apps[$appId])) { + // If we land here something really isn't right. But at least we caught the + // notice that is otherwise emitted for the undefined index + $this->logger->error("App $appId not loaded for the container service registration"); + + continue; + } + try { /** * Register the service and convert the callable into a \Closure if necessary */ - $apps[$registration->getAppId()] + $apps[$appId] ->getContainer() ->registerService( $registration->getName(), @@ -384,7 +401,6 @@ class RegistrationContext { $registration->isShared() ); } catch (Throwable $e) { - $appId = $registration->getAppId(); $this->logger->error("Error during service registration of $appId: " . $e->getMessage(), [ 'exception' => $e, ]); @@ -392,15 +408,23 @@ class RegistrationContext { } while (($registration = array_shift($this->aliases)) !== null) { + $appId = $registration->getAppId(); + if (!isset($apps[$appId])) { + // If we land here something really isn't right. But at least we caught the + // notice that is otherwise emitted for the undefined index + $this->logger->error("App $appId not loaded for the container alias registration"); + + continue; + } + try { - $apps[$registration->getAppId()] + $apps[$appId] ->getContainer() ->registerAlias( $registration->getAlias(), $registration->getTarget() ); } catch (Throwable $e) { - $appId = $registration->getAppId(); $this->logger->error("Error during service alias registration of $appId: " . $e->getMessage(), [ 'exception' => $e, ]); @@ -408,16 +432,24 @@ class RegistrationContext { } while (($registration = array_shift($this->parameters)) !== null) { + $appId = $registration->getAppId(); + if (!isset($apps[$appId])) { + // If we land here something really isn't right. But at least we caught the + // notice that is otherwise emitted for the undefined index + $this->logger->error("App $appId not loaded for the container parameter registration"); + + continue; + } + try { - $apps[$registration->getAppId()] + $apps[$appId] ->getContainer() ->registerParameter( $registration->getName(), $registration->getValue() ); } catch (Throwable $e) { - $appId = $registration->getAppId(); - $this->logger->error("Error during service alias registration of $appId: " . $e->getMessage(), [ + $this->logger->error("Error during service parameter registration of $appId: " . $e->getMessage(), [ 'exception' => $e, ]); } @@ -429,12 +461,20 @@ class RegistrationContext { */ public function delegateMiddlewareRegistrations(array $apps): void { while (($middleware = array_shift($this->middlewares)) !== null) { + $appId = $middleware->getAppId(); + if (!isset($apps[$appId])) { + // If we land here something really isn't right. But at least we caught the + // notice that is otherwise emitted for the undefined index + $this->logger->error("App $appId not loaded for the container middleware registration"); + + continue; + } + try { - $apps[$middleware->getAppId()] + $apps[$appId] ->getContainer() ->registerMiddleWare($middleware->getService()); } catch (Throwable $e) { - $appId = $middleware->getAppId(); $this->logger->error("Error during capability registration of $appId: " . $e->getMessage(), [ 'exception' => $e, ]); diff --git a/lib/private/Avatar/AvatarManager.php b/lib/private/Avatar/AvatarManager.php index 83b2b4b5c3d..c3afd8094c7 100644 --- a/lib/private/Avatar/AvatarManager.php +++ b/lib/private/Avatar/AvatarManager.php @@ -39,6 +39,7 @@ use OC\KnownUser\KnownUserService; 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 @@ class AvatarManager implements IAvatarManager { $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/private/Route/Router.php b/lib/private/Route/Router.php index fddd64a7d48..0bccb8190cd 100644 --- a/lib/private/Route/Router.php +++ b/lib/private/Route/Router.php @@ -233,29 +233,32 @@ class Router implements IRouter { * @throws \Exception * @return array */ - public function findMatchingRoute(string $url, bool $loadAll = false): array { - if (strpos($url, '/apps/') === 0) { + public function findMatchingRoute(string $url): array { + if (substr($url, 0, 6) === '/apps/') { // empty string / 'apps' / $app / rest of the route [, , $app,] = explode('/', $url, 4); $app = \OC_App::cleanAppId($app); \OC::$REQUESTEDAPP = $app; $this->loadRoutes($app); - } elseif (strpos($url, '/ocsapp/apps/') === 0) { + } elseif (substr($url, 0, 13) === '/ocsapp/apps/') { // empty string / 'ocsapp' / 'apps' / $app / rest of the route [, , , $app,] = explode('/', $url, 5); $app = \OC_App::cleanAppId($app); \OC::$REQUESTEDAPP = $app; $this->loadRoutes($app); - } elseif (strpos($url, '/settings/') === 0) { + } elseif (substr($url, 0, 10) === '/settings/') { $this->loadRoutes('settings'); + } elseif (substr($url, 0, 6) === '/core/') { + \OC::$REQUESTEDAPP = $url; + if (!\OC::$server->getConfig()->getSystemValueBool('maintenance') && !Util::needUpgrade()) { + \OC_App::loadApps(); + } + $this->loadRoutes('core'); + } else { + $this->loadRoutes(); } - \OC::$REQUESTEDAPP = $url; - if (!\OC::$server->getConfig()->getSystemValueBool('maintenance') && !Util::needUpgrade()) { - \OC_App::loadApps(); - } - $this->loadRoutes('core'); $matcher = new UrlMatcher($this->root, $this->context); try { @@ -268,11 +271,6 @@ class Router implements IRouter { try { $parameters = $matcher->match($url . '/'); } catch (ResourceNotFoundException $newException) { - // Attempt to fallback to load all routes if none of the above route patterns matches and the route is not in core - if (!$loadAll) { - $this->loadRoutes(); - return $this->findMatchingRoute($url, true); - } // If we still didn't match a route, we throw the original exception throw $e; } diff --git a/lib/private/Server.php b/lib/private/Server.php index 207c53bdae3..03d6a4146ed 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1031,7 +1031,7 @@ class Server extends ServerContainer implements IServerContainer { $this->registerService(ILDAPProviderFactory::class, function (ContainerInterface $c) { $config = $c->get(\OCP\IConfig::class); $factoryClass = $config->getSystemValue('ldapProviderFactory', null); - if (is_null($factoryClass)) { + if (is_null($factoryClass) || !class_exists($factoryClass)) { return new NullLDAPProviderFactory($this); } /** @var \OCP\LDAP\ILDAPProviderFactory $factory */ diff --git a/lib/public/Accounts/IAccount.php b/lib/public/Accounts/IAccount.php index 72c207537dc..8d4d8b5c023 100644 --- a/lib/public/Accounts/IAccount.php +++ b/lib/public/Accounts/IAccount.php @@ -94,9 +94,10 @@ interface IAccount extends \JsonSerializable { /** * 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 9e026f4ce5b..779fb1299b4 100644 --- a/lib/public/Accounts/IAccountPropertyCollection.php +++ b/lib/public/Accounts/IAccountPropertyCollection.php @@ -69,6 +69,14 @@ interface IAccountPropertyCollection extends JsonSerializable { 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 * * @since 22.0.0 diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php index ea82bd04f3f..8ed0e29d7ce 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -108,65 +108,191 @@ class AccountManagerTest extends TestCase { [ '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 @@ class AccountManagerTest extends TestCase { * @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 @@ class AccountManagerTest extends TestCase { $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 @@ class AccountManagerTest extends TestCase { 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 @@ class AccountManagerTest extends TestCase { ); } - $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 @@ class AccountManagerTest extends TestCase { ->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 8af9e209034..d5c7cd60b1b 100644 --- a/tests/lib/Accounts/HooksTest.php +++ b/tests/lib/Accounts/HooksTest.php @@ -23,7 +23,9 @@ namespace Test\Accounts; 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 @@ class HooksTest extends TestCase { $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 @@ class HooksTest extends TestCase { */ 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 @@ class HooksTest extends TestCase { ], ]; } - - public function testGetAccountManager() { - $hooks = new Hooks($this->logger); - $result = $this->invokePrivate($hooks, 'getAccountManager'); - $this->assertInstanceOf(AccountManager::class, $result); - } } |