From d1f84d6faf713b9058975202605dfb0f08a385ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 14 Feb 2018 13:19:23 +0100 Subject: [PATCH 1/3] introduce user:sync [--uid foo|--seenOnly|--showCount] --- core/Command/User/SyncBackend.php | 158 +++++++++++++----- core/Migrations/Version20170221114437.php | 3 +- lib/private/User/AccountMapper.php | 36 +++- lib/private/User/Sync/AllUsersIterator.php | 48 ++++++ lib/private/User/Sync/SeenUsersIterator.php | 53 ++++++ lib/private/User/Sync/UsersIterator.php | 53 ++++++ lib/private/User/SyncService.php | 50 +++--- tests/lib/User/AccountMapperTest.php | 54 +++++- tests/lib/User/Sync/AllUsersIteratorTest.php | 124 ++++++++++++++ tests/lib/User/Sync/SeenUsersIteratorTest.php | 131 +++++++++++++++ tests/lib/User/SyncServiceTest.php | 3 +- 11 files changed, 638 insertions(+), 75 deletions(-) create mode 100644 lib/private/User/Sync/AllUsersIterator.php create mode 100644 lib/private/User/Sync/SeenUsersIterator.php create mode 100644 lib/private/User/Sync/UsersIterator.php create mode 100644 tests/lib/User/Sync/AllUsersIteratorTest.php create mode 100644 tests/lib/User/Sync/SeenUsersIteratorTest.php diff --git a/core/Command/User/SyncBackend.php b/core/Command/User/SyncBackend.php index 2f1c8e4f76e2..5ea25268e94e 100644 --- a/core/Command/User/SyncBackend.php +++ b/core/Command/User/SyncBackend.php @@ -1,5 +1,6 @@ * @author Thomas Müller * * @copyright Copyright (c) 2018, ownCloud GmbH @@ -23,6 +24,8 @@ use OC\User\AccountMapper; +use OC\User\Sync\AllUsersIterator; +use OC\User\Sync\SeenUsersIterator; use OC\User\SyncService; use OCP\IConfig; use OCP\ILogger; @@ -80,6 +83,24 @@ protected function configure() { InputOption::VALUE_NONE, 'List all known backend classes' ) + ->addOption( + 'uid', + 'u', + InputOption::VALUE_REQUIRED, + 'sync only the user with the given user id' + ) + ->addOption( + 'seenOnly', + 's', + InputOption::VALUE_NONE, + 'sync only seen users' + ) + ->addOption( + 'showCount', + 'c', + InputOption::VALUE_NONE, + 'calculate user count before syncing' + ) ->addOption( 'missing-account-action', 'm', @@ -97,12 +118,12 @@ protected function execute(InputInterface $input, OutputInterface $output) { return 0; } $backendClassName = $input->getArgument('backend-class'); - if (is_null($backendClassName)) { - $output->writeln("No backend class name given. Please run ./occ help user:sync to understand how this command works."); + if ($backendClassName === null) { + $output->writeln('No backend class name given. Please run ./occ help user:sync to understand how this command works.'); return 1; } $backend = $this->getBackend($backendClassName); - if (is_null($backend)) { + if ($backend === null) { $output->writeln("The backend <$backendClassName> does not exist. Did you miss to enable the app?"); return 1; } @@ -116,7 +137,7 @@ protected function execute(InputInterface $input, OutputInterface $output) { if ($input->getOption('missing-account-action') !== null) { $missingAccountsAction = $input->getOption('missing-account-action'); if (!in_array($missingAccountsAction, $validActions, true)) { - $output->writeln("Unknown action. Choose between \"disable\" or \"remove\""); + $output->writeln('Unknown action. Choose between "disable" or "remove"'); return 1; } } else { @@ -132,27 +153,91 @@ protected function execute(InputInterface $input, OutputInterface $output) { $syncService = new SyncService($this->config, $this->logger, $this->accountMapper); - // analyse unknown users - $this->handleUnknownUsers($input, $output, $backend, $syncService, $missingAccountsAction, $validActions); + $uid = $input->getOption('uid'); + + if ($uid) { + $this->syncSingleUser($input, $output, $syncService, $backend, $uid, $missingAccountsAction, $validActions); + } else { + $this->syncMultipleUsers($input, $output, $syncService, $backend, $missingAccountsAction, $validActions); + } + + return 0; + } - // insert/update known users - $output->writeln("Insert new and update existing users ..."); + + /** + * @param InputInterface $input + * @param OutputInterface $output + * @param SyncService $syncService + * @param UserInterface $backend + * @param string $missingAccountsAction + * @param array $validActions + */ + private function syncMultipleUsers ( + InputInterface $input, + OutputInterface $output, + SyncService $syncService, + UserInterface $backend, + $missingAccountsAction, + array $validActions + ) { + $output->writeln('Analyse unknown users ...'); + $p = new ProgressBar($output); + $unknownUsers = $syncService->getNoLongerExistingUsers($backend, function () use ($p) { + $p->advance(); + }); + $p->finish(); + $output->writeln(''); + $output->writeln(''); + $this->handleUnknownUsers($unknownUsers, $input, $output, $missingAccountsAction, $validActions); + + $output->writeln('Insert new and update existing users ...'); $p = new ProgressBar($output); $max = null; - if ($backend->implementsActions(\OC_User_Backend::COUNT_USERS)) { + if ($backend->implementsActions(\OC_User_Backend::COUNT_USERS) && $input->getOption('showCount')) { $max = $backend->countUsers(); } $p->start($max); - $syncService->run($backend, function () use ($p) { + + if ($input->getOption('seenOnly')) { + $iterator = new SeenUsersIterator($this->accountMapper, get_class($backend)); + } else { + $iterator = new AllUsersIterator($backend); + } + $syncService->run($backend, $iterator, function () use ($p) { $p->advance(); }); $p->finish(); $output->writeln(''); $output->writeln(''); - - return 0; } + /** + * @param InputInterface $input + * @param OutputInterface $output + * @param SyncService $syncService + * @param UserInterface $backend + * @param string $uid + * @param string $missingAccountsAction + * @param array $validActions + */ + private function syncSingleUser( + InputInterface $input, + OutputInterface $output, + SyncService $syncService, + UserInterface $backend, + $uid, + $missingAccountsAction, + array $validActions + ) { + $output->writeln("Syncing $uid ..."); + if (!$backend->userExists($uid)) { + $this->handleUnknownUsers([$uid], $input, $output, $missingAccountsAction, $validActions); + } else { + // sync + $syncService->run($backend, new \ArrayIterator([$uid]), function (){}); + } + } /** * @param $backend * @return null|UserInterface @@ -187,58 +272,49 @@ private function doActionForAccountUids(array $uids, callable $callbackExists, c } /** + * @param string[] $unknownUsers * @param InputInterface $input * @param OutputInterface $output - * @param UserInterface $backend - * @param SyncService $syncService * @param $missingAccountsAction * @param $validActions */ - private function handleUnknownUsers(InputInterface $input, OutputInterface $output, UserInterface $backend, SyncService $syncService, $missingAccountsAction, $validActions) { - $output->writeln("Analyse unknown users ..."); - $p = new ProgressBar($output); - $toBeDeleted = $syncService->getNoLongerExistingUsers($backend, function () use ($p) { - $p->advance(); - }); - $p->finish(); - $output->writeln(''); - $output->writeln(''); + private function handleUnknownUsers(array $unknownUsers, InputInterface $input, OutputInterface $output, $missingAccountsAction, $validActions) { - if (empty($toBeDeleted)) { - $output->writeln("No unknown users have been detected."); + if (empty($unknownUsers)) { + $output->writeln('No unknown users have been detected.'); } else { - $output->writeln("Following users are no longer known with the connected backend."); + $output->writeln('Following users are no longer known with the connected backend.'); switch ($missingAccountsAction) { case 'disable': - $output->writeln("Proceeding to disable the accounts"); - $this->doActionForAccountUids($toBeDeleted, + $output->writeln('Proceeding to disable the accounts'); + $this->doActionForAccountUids($unknownUsers, function ($uid, IUser $ac) use ($output) { $ac->setEnabled(false); $output->writeln($uid); }, function ($uid) use ($output) { - $output->writeln($uid . " (unknown account for the user)"); + $output->writeln("$uid (unknown account for the user)"); }); break; case 'remove': - $output->writeln("Proceeding to remove the accounts"); - $this->doActionForAccountUids($toBeDeleted, + $output->writeln('Proceeding to remove the accounts'); + $this->doActionForAccountUids($unknownUsers, function ($uid, IUser $ac) use ($output) { $ac->delete(); $output->writeln($uid); }, function ($uid) use ($output) { - $output->writeln($uid . " (unknown account for the user)"); + $output->writeln("$uid (unknown account for the user)"); }); break; case 'ask later': - $output->writeln("listing the unknown accounts"); - $this->doActionForAccountUids($toBeDeleted, + $output->writeln('listing the unknown accounts'); + $this->doActionForAccountUids($unknownUsers, function ($uid) use ($output) { $output->writeln($uid); }, function ($uid) use ($output) { - $output->writeln($uid . " (unknown account for the user)"); + $output->writeln("$uid (unknown account for the user)"); }); // overwriting variables! $helper = $this->getHelper('question'); @@ -251,23 +327,23 @@ function ($uid) use ($output) { switch ($missingAccountsAction2) { // if "nothing" is selected, just ignore and finish case 'disable': - $output->writeln("Proceeding to disable the accounts"); - $this->doActionForAccountUids($toBeDeleted, + $output->writeln('Proceeding to disable the accounts'); + $this->doActionForAccountUids($unknownUsers, function ($uid, IUser $ac) { $ac->setEnabled(false); }, function ($uid) use ($output) { - $output->writeln($uid . " (unknown account for the user)"); + $output->writeln("$uid (unknown account for the user)"); }); break; case 'remove': - $output->writeln("Proceeding to remove the accounts"); - $this->doActionForAccountUids($toBeDeleted, + $output->writeln('Proceeding to remove the accounts'); + $this->doActionForAccountUids($unknownUsers, function ($uid, IUser $ac) { $ac->delete(); }, function ($uid) use ($output) { - $output->writeln($uid . " (unknown account for the user)"); + $output->writeln("$uid (unknown account for the user)"); }); break; } diff --git a/core/Migrations/Version20170221114437.php b/core/Migrations/Version20170221114437.php index 3f6d7df8fdab..199d3b5c57e2 100644 --- a/core/Migrations/Version20170221114437.php +++ b/core/Migrations/Version20170221114437.php @@ -4,6 +4,7 @@ use OC\User\AccountMapper; use OC\User\AccountTermMapper; use OC\User\Database; +use OC\User\Sync\AllUsersIterator; use OC\User\SyncService; use OCP\Migration\ISimpleMigration; use OCP\Migration\IOutput; @@ -24,7 +25,7 @@ public function run(IOutput $out) { // insert/update known users $out->info("Insert new users ..."); $out->startProgress($backend->countUsers()); - $syncService->run($backend, function () use ($out) { + $syncService->run($backend, new AllUsersIterator($backend), function () use ($out) { $out->advance(); }); $out->finishProgress(); diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index 9a64bb6b912e..9457eea53204 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -234,7 +234,7 @@ public function callForAllUsers($callback, $search, $onlySeen) { } $stmt = $qb->execute(); while ($row = $stmt->fetch()) { - $return =$callback($this->mapRowToEntity($row)); + $return = $callback($this->mapRowToEntity($row)); if ($return === false) { break; } @@ -243,4 +243,38 @@ public function callForAllUsers($callback, $search, $onlySeen) { $stmt->closeCursor(); } + /** + * @param string $backend + * @param bool $hasLoggedIn + * @param integer $limit + * @param integer $offset + * @return string[] + */ + public function findUserIds($backend = null, $hasLoggedIn = null, $limit = null, $offset = null) { + $qb = $this->db->getQueryBuilder(); + $qb->select('user_id') + ->from($this->getTableName()) + ->orderBy('user_id'); // needed for predictable limit & offset + + if ($backend !== null) { + $qb->andWhere($qb->expr()->eq('backend', $qb->createNamedParameter($backend))); + } + if ($hasLoggedIn === true) { + $qb->andWhere($qb->expr()->gt('last_login', new Literal(0))); + } else if ($hasLoggedIn === false) { + $qb->andWhere($qb->expr()->eq('last_login', new Literal(0))); + } + if ($limit !== null) { + $qb->setMaxResults($limit); + } + if ($offset !== null) { + $qb->setFirstResult($offset); + } + + $stmt = $qb->execute(); + $rows = $stmt->fetchAll(\PDO::FETCH_COLUMN); + $stmt->closeCursor(); + return $rows; + } + } diff --git a/lib/private/User/Sync/AllUsersIterator.php b/lib/private/User/Sync/AllUsersIterator.php new file mode 100644 index 000000000000..289cfe6c96c5 --- /dev/null +++ b/lib/private/User/Sync/AllUsersIterator.php @@ -0,0 +1,48 @@ + + * + * @copyright Copyright (c) 2018, 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 + * + */ +namespace OC\User\Sync; + +use OCP\UserInterface; + +class AllUsersIterator extends UsersIterator { + /** + * @var UserInterface + */ + private $backend; + + public function __construct(UserInterface $backend) { + $this->backend = $backend; + } + + public function rewind() { + parent::rewind(); + $this->data = $this->backend->getUsers('', self::LIMIT, 0); + } + + public function next() { + $this->position++; + if ($this->currentDataPos() === 0) { + $this->page++; + $offset = $this->page * self::LIMIT; + $this->data = $this->backend->getUsers('', self::LIMIT, $offset); + } + } +} \ No newline at end of file diff --git a/lib/private/User/Sync/SeenUsersIterator.php b/lib/private/User/Sync/SeenUsersIterator.php new file mode 100644 index 000000000000..ab5e61f222eb --- /dev/null +++ b/lib/private/User/Sync/SeenUsersIterator.php @@ -0,0 +1,53 @@ + + * + * @copyright Copyright (c) 2018, 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 + * + */ +namespace OC\User\Sync; + +use OC\User\AccountMapper; + +class SeenUsersIterator extends UsersIterator { + /** + * @var AccountMapper + */ + private $mapper; + /** + * @var string class name + */ + private $backend; + + public function __construct(AccountMapper $mapper, $backend) { + $this->mapper = $mapper; + $this->backend = $backend; + } + + public function rewind() { + parent::rewind(); + $this->data = $this->mapper->findUserIds($this->backend, true, self::LIMIT, 0); + } + + public function next() { + $this->position++; + if ($this->currentDataPos() === 0) { + $this->page++; + $offset = $this->page * self::LIMIT; + $this->data = $this->mapper->findUserIds($this->backend, true, self::LIMIT, $offset); + } + } +} \ No newline at end of file diff --git a/lib/private/User/Sync/UsersIterator.php b/lib/private/User/Sync/UsersIterator.php new file mode 100644 index 000000000000..a090e9d43af7 --- /dev/null +++ b/lib/private/User/Sync/UsersIterator.php @@ -0,0 +1,53 @@ + + * + * @copyright Copyright (c) 2018, 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 + * + */ +namespace OC\User\Sync; + +abstract class UsersIterator implements \Iterator { + protected $position = 0; + protected $page; + protected $data; + + const LIMIT = 500; + + + public function rewind() { + $this->position = 0; + $this->page = 0; + } + + public function current() { + return $this->data[$this->currentDataPos()]; + } + + public function key() { + return $this->position; + } + + public abstract function next(); + + public function valid() { + return isset($this->data[$this->currentDataPos()]); + } + + protected function currentDataPos() { + return $this->position % self::LIMIT; + } +} \ No newline at end of file diff --git a/lib/private/User/SyncService.php b/lib/private/User/SyncService.php index 7b09cbd99f2a..201d2cbd2f12 100644 --- a/lib/private/User/SyncService.php +++ b/lib/private/User/SyncService.php @@ -1,5 +1,6 @@ * @author Thomas Müller * * @copyright Copyright (c) 2018, ownCloud GmbH @@ -95,33 +96,26 @@ public function getNoLongerExistingUsers(UserInterface $backend, \Closure $callb /** * @param UserInterface $backend to sync + * @param \Traversable $userIds of users * @param \Closure $callback is called for every user to progress display */ - public function run(UserInterface $backend, \Closure $callback) { - $limit = 500; - $offset = 0; - $backendClass = get_class($backend); - do { - $users = $backend->getUsers('', $limit, $offset); - - // update existing and insert new users - foreach ($users as $uid) { - try { - $account = $this->createOrSyncAccount($uid, $backend); - $uid = $account->getUserId(); // get correct case - // clean the user's preferences - $this->cleanPreferences($uid); - } catch (\Exception $e) { - // Error syncing this user - $this->logger->error("Error syncing user with uid: $uid and backend: {get_class($backend)}"); - $this->logger->logException($e); - } - - // call the callback - $callback($uid); + public function run(UserInterface $backend, \Traversable $userIds, \Closure $callback) { + // update existing and insert new users + foreach ($userIds as $uid) { + try { + $account = $this->createOrSyncAccount($uid, $backend); + $uid = $account->getUserId(); // get correct case + // clean the user's preferences + $this->cleanPreferences($uid); + } catch (\Exception $e) { + // Error syncing this user + $this->logger->error("Error syncing user with uid: $uid and backend: {get_class($backend)}"); + $this->logger->logException($e); } - $offset += $limit; - } while(count($users) >= $limit); + + // call the callback + $callback($uid); + } } /** @@ -239,10 +233,10 @@ private function syncHome(Account $a, UserInterface $backend) { if ($proividesHome) { $home = $backend->getHome($uid); } - if (!is_string($home) || substr($home, 0, 1) !== '/') { + if (!is_string($home) || $home[0] !== '/') { $home = $this->config->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data') . "/$uid"; $this->logger->debug( - "User backend ".get_class($backend)." provided no home for <$uid>", + 'User backend ' .get_class($backend)." provided no home for <$uid>", ['app' => self::class] ); } @@ -283,7 +277,7 @@ private function syncSearchTerms(Account $a, UserInterface $backend) { $searchTerms = $backend->getSearchTerms($uid); $a->setSearchTerms($searchTerms); if ($a->haveTermsChanged()) { - $logTerms = join('|', $searchTerms); + $logTerms = implode('|', $searchTerms); $this->logger->debug( "Setting searchTerms for <$uid> to <$logTerms>", ['app' => self::class] ); @@ -369,7 +363,7 @@ public function createNewAccount($backend, $uid) { */ private function readUserConfig($uid, $app, $key) { $keys = $this->config->getUserKeys($uid, $app); - if (in_array($key, $keys)) { + if (in_array($key, $keys, true)) { $enabled = $this->config->getUserValue($uid, $app, $key); return [true, $enabled]; } diff --git a/tests/lib/User/AccountMapperTest.php b/tests/lib/User/AccountMapperTest.php index 0a7722269199..11dac9c33377 100644 --- a/tests/lib/User/AccountMapperTest.php +++ b/tests/lib/User/AccountMapperTest.php @@ -56,9 +56,9 @@ public static function setUpBeforeClass() { // create test users for ($i = 1; $i <= 4; $i++) { - $account = $mapper->find("TestFind$i"); - if ($account) { - $mapper->delete($account); + $accounts = $mapper->find("TestFind$i"); + if (isset($accounts[0])) { + $mapper->delete($accounts[0]); } $account = new Account(); @@ -169,4 +169,52 @@ public function testFindLimitAndOffset() { $this->assertEquals("TestFind3", array_shift($result)->getUserId()); $this->assertEquals("TestFind4", array_shift($result)->getUserId()); } + + + public function findUserIdsDataProvider() { + return [ + [self::class, null, null, ['TestFind1','TestFind2','TestFind3','TestFind4']], + ['not existing backend', null, null, []], + [self::class, 1, null, ['TestFind1']], + [self::class, 2, 2, ['TestFind3', 'TestFind4']], + [self::class, 1, 3, ['TestFind4']], + ]; + } + + /** + * findUserIds + * + * @dataProvider findUserIdsDataProvider + */ + public function testFindUserIds($backend, $limit, $offset, $expected) { + $result = $this->mapper->findUserIds($backend, false, $limit, $offset); + $this->assertSame($expected, $result); + } + + public function findUserIdsLoggedInDataProvider() { + return [ + [self::class, null, null, ['TestFind2','TestFind4']], + ['not existing backend', null, null, []], + [self::class, 1, null, ['TestFind2']], + [self::class, 1, 1, ['TestFind4']], + ]; + } + + /** + * findUserIds that logged in + * + * @dataProvider findUserIdsLoggedInDataProvider + */ + public function testFindUserIdsLoggedIn($backend, $limit, $offset, $expected) { + + $accounts = $this->mapper->find("TestFind2"); + $accounts[0]->setLastLogin(time()); + $this->mapper->update($accounts[0]); + $accounts = $this->mapper->find("TestFind4"); + $accounts[0]->setLastLogin(time()); + $this->mapper->update($accounts[0]); + + $result = $this->mapper->findUserIds($backend, true, $limit, $offset); + $this->assertSame($expected, $result); + } } diff --git a/tests/lib/User/Sync/AllUsersIteratorTest.php b/tests/lib/User/Sync/AllUsersIteratorTest.php new file mode 100644 index 000000000000..ebb3ae7711a2 --- /dev/null +++ b/tests/lib/User/Sync/AllUsersIteratorTest.php @@ -0,0 +1,124 @@ + + * + * @copyright Copyright (c) 2018, 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 + * + */ + +namespace OC\User\Sync; + + +use OCP\UserInterface; +use Test\TestCase; + +/** + * Class AllUsersIteratorTest + * + * @package OC\User\Sync + * + * @see http://php.net/manual/en/class.iterator.php for the order of calls on an iterator + */ +class AllUsersIteratorTest extends TestCase { + + /** + * @var UserInterface|\PHPUnit_Framework_MockObject_MockObject + */ + private $backend; + /** + * @var \Iterator + */ + private $iterator; + + public function setUp() { + parent::setUp(); + + $this->backend = $this->createMock(UserInterface::class); + + $this->iterator = new AllUsersIterator($this->backend); + } + + /** + * Iterators are initialized by a call to rewind + */ + public function testRewind() { + + $this->backend->expects($this->once()) + ->method('getUsers') + ->with( + $this->equalTo(''), // all users + $this->equalTo(UsersIterator::LIMIT), // limit 500 + $this->equalTo(0) // at the beginning + ) + ->willReturn(['user0']); + $this->iterator->rewind(); + $this->assertTrue($this->iterator->valid()); + $this->assertEquals('user0', $this->iterator->current()); + $this->assertEquals(0, $this->iterator->key()); + } + + /** + * test three pages of results + */ + public function testNext() { + + // create pages for 1001 users (0..1000) + $page1 = []; + for ( $i=0; $i<500; $i++ ) { + $page1[] = "user$i"; + } + $page2 = []; + for ( $i=500; $i<1000; $i++ ) { + $page2[] = "user$i"; + } + $page3 = ['user1000']; + + $this->backend->expects($this->exactly(3)) + ->method('getUsers') + ->withConsecutive( + [ + $this->equalTo(''), // all users + $this->equalTo(UsersIterator::LIMIT), // limit 500 + $this->equalTo(0) // at the beginning + ],[ + $this->equalTo(''), // all users + $this->equalTo(UsersIterator::LIMIT), // limit 500 + $this->equalTo(500) // second page + ],[ + $this->equalTo(''), // all users + $this->equalTo(UsersIterator::LIMIT), // limit 500 + $this->equalTo(1000) // last page + ] + ) + ->willReturnOnConsecutiveCalls($page1, $page2, $page3); + + // loop over iterator manually to check key() and valid() + + $this->iterator->rewind(); + $this->assertTrue($this->iterator->valid()); + $this->assertEquals('user0', $this->iterator->current()); + $this->assertEquals(0, $this->iterator->key()); + for ( $i=1; $i<=1000; $i++ ) { + $this->iterator->next(); + $this->assertTrue($this->iterator->valid()); + $this->assertEquals("user$i", $this->iterator->current()); + $this->assertEquals($i, $this->iterator->key()); + } + $this->iterator->next(); + $this->assertFalse($this->iterator->valid()); + } + +} \ No newline at end of file diff --git a/tests/lib/User/Sync/SeenUsersIteratorTest.php b/tests/lib/User/Sync/SeenUsersIteratorTest.php new file mode 100644 index 000000000000..caf8da1f0889 --- /dev/null +++ b/tests/lib/User/Sync/SeenUsersIteratorTest.php @@ -0,0 +1,131 @@ + + * + * @copyright Copyright (c) 2018, 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 + * + */ + +namespace OC\User\Sync; + + +use OC\User\AccountMapper; +use OCP\UserInterface; +use Test\TestCase; + +/** + * Class SeenUsersIteratorTest + * + * @package OC\User\Sync + * + * @see http://php.net/manual/en/class.iterator.php for the order of calls on an iterator + */ +class SeenUsersIteratorTest extends TestCase { + + /** + * @var AccountMapper|\PHPUnit_Framework_MockObject_MockObject + */ + private $mapper; + /** + * @var \Iterator + */ + private $iterator; + + const TEST_BACKEND = 'TestBackend'; + + public function setUp() { + parent::setUp(); + + $this->mapper = $this->createMock(AccountMapper::class); + + $this->iterator = new SeenUsersIterator($this->mapper, self::TEST_BACKEND); + } + + /** + * Iterators are initialized by a call to rewind + */ + public function testRewind() { + + $this->mapper->expects($this->once()) + ->method('findUserIds') + ->with( + $this->equalTo(self::TEST_BACKEND), // only from this backend + $this->equalTo(true), // only logged in users + $this->equalTo(UsersIterator::LIMIT), // limit 500 + $this->equalTo(0) // at the beginning + ) + ->willReturn(['user0']); + $this->iterator->rewind(); + $this->assertTrue($this->iterator->valid()); + $this->assertEquals('user0', $this->iterator->current()); + $this->assertEquals(0, $this->iterator->key()); + } + + /** + * test three pages of results + */ + public function testNext() { + + // create pages for 1001 users (0..1000) + $page1 = []; + for ( $i=0; $i<500; $i++ ) { + $page1[] = "user$i"; + } + $page2 = []; + for ( $i=500; $i<1000; $i++ ) { + $page2[] = "user$i"; + } + $page3 = ['user1000']; + + $this->mapper->expects($this->exactly(3)) + ->method('findUserIds') + ->withConsecutive( + [ + $this->equalTo(self::TEST_BACKEND), // only from this backend + $this->equalTo(true), // only logged in users + $this->equalTo(UsersIterator::LIMIT), // limit 500 + $this->equalTo(0) // at the beginning + ],[ + $this->equalTo(self::TEST_BACKEND), // only from this backend + $this->equalTo(true), // only logged in users + $this->equalTo(UsersIterator::LIMIT), // limit 500 + $this->equalTo(500) // second page + ],[ + $this->equalTo(self::TEST_BACKEND), // only from this backend + $this->equalTo(true), // only logged in users + $this->equalTo(UsersIterator::LIMIT), // limit 500 + $this->equalTo(1000) // last page + ] + ) + ->willReturnOnConsecutiveCalls($page1, $page2, $page3); + + // loop over iterator manually to check key() and valid() + + $this->iterator->rewind(); + $this->assertTrue($this->iterator->valid()); + $this->assertEquals('user0', $this->iterator->current()); + $this->assertEquals(0, $this->iterator->key()); + for ( $i=1; $i<=1000; $i++ ) { + $this->iterator->next(); + $this->assertTrue($this->iterator->valid()); + $this->assertEquals("user$i", $this->iterator->current()); + $this->assertEquals($i, $this->iterator->key()); + } + $this->iterator->next(); + $this->assertFalse($this->iterator->valid()); + } + +} \ No newline at end of file diff --git a/tests/lib/User/SyncServiceTest.php b/tests/lib/User/SyncServiceTest.php index 32d59e99e330..0cb9ae4e3d23 100644 --- a/tests/lib/User/SyncServiceTest.php +++ b/tests/lib/User/SyncServiceTest.php @@ -26,6 +26,7 @@ use OC\User\Account; use OC\User\AccountMapper; use OC\User\Database; +use OC\User\Sync\AllUsersIterator; use OC\User\SyncService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\IConfig; @@ -95,7 +96,7 @@ public function testSetupNewAccount() { $s = new SyncService($config, $logger, $mapper); - $s->run($backend, function($uid) {}); + $s->run($backend, new AllUsersIterator($backend), function($uid) {}); $this->invokePrivate($s, 'syncHome', [$account, $backend]); } From 1bca4c84c7aaa32741ae595b329db0c35aa044e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 4 Apr 2018 13:59:01 +0200 Subject: [PATCH 2/3] test exception is logged --- lib/private/User/SyncService.php | 3 +- tests/lib/User/SyncServiceTest.php | 89 ++++++++++++++++++------------ 2 files changed, 57 insertions(+), 35 deletions(-) diff --git a/lib/private/User/SyncService.php b/lib/private/User/SyncService.php index 201d2cbd2f12..f3825c0a3b67 100644 --- a/lib/private/User/SyncService.php +++ b/lib/private/User/SyncService.php @@ -109,7 +109,8 @@ public function run(UserInterface $backend, \Traversable $userIds, \Closure $cal $this->cleanPreferences($uid); } catch (\Exception $e) { // Error syncing this user - $this->logger->error("Error syncing user with uid: $uid and backend: {get_class($backend)}"); + $backendClass = get_class($backend); + $this->logger->error("Error syncing user with uid: $uid and backend: $backendClass"); $this->logger->logException($e); } diff --git a/tests/lib/User/SyncServiceTest.php b/tests/lib/User/SyncServiceTest.php index 0cb9ae4e3d23..4fccbcbb6620 100644 --- a/tests/lib/User/SyncServiceTest.php +++ b/tests/lib/User/SyncServiceTest.php @@ -29,6 +29,7 @@ use OC\User\Sync\AllUsersIterator; use OC\User\SyncService; use OCP\AppFramework\Db\DoesNotExistException; +use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\IConfig; use OCP\ILogger; use OCP\User\IProvidesHomeBackend; @@ -37,23 +38,36 @@ class SyncServiceTest extends TestCase { + /** @var IConfig | \PHPUnit_Framework_MockObject_MockObject */ + private $config; + /** @var ILogger | \PHPUnit_Framework_MockObject_MockObject */ + private $logger; + /** @var AccountMapper | \PHPUnit_Framework_MockObject_MockObject */ + private $mapper; + + protected function setUp() { + parent::setUp(); + + $this->config = $this->createMock(IConfig::class); + $this->logger = $this->createMock(ILogger::class); + $this->mapper = $this->createMock(AccountMapper::class); + } + public function testSetupAccount() { - $mapper = $this->createMock(AccountMapper::class); + /** @var UserInterface | IProvidesHomeBackend | \PHPUnit_Framework_MockObject_MockObject $backend */ $backend = $this->createMock(UserInterface::class); - $config = $this->createMock(IConfig::class); - $logger = $this->createMock(ILogger::class); - $config->expects($this->any())->method('getUserKeys')->willReturnMap([ + $this->config->expects($this->any())->method('getUserKeys')->willReturnMap([ ['user1', 'core', []], ['user1', 'login', []], ['user1', 'settings', ['email']], ['user1', 'files', []], ]); - $config->expects($this->any())->method('getUserValue')->willReturnMap([ + $this->config->expects($this->any())->method('getUserValue')->willReturnMap([ ['user1', 'settings', 'email', '', 'foo@bar.net'], ]); - $s = new SyncService($config, $logger, $mapper); + $s = new SyncService($this->config, $this->logger, $this->mapper); $a = new Account(); $a->setUserId('user1'); $s->syncAccount($a, $backend); @@ -65,51 +79,64 @@ public function testSetupAccount() { * Pass in a backend that has new users anc check that they accounts are inserted */ public function testSetupNewAccount() { - $mapper = $this->createMock(AccountMapper::class); + /** @var UserInterface | IProvidesHomeBackend | \PHPUnit_Framework_MockObject_MockObject $backend */ $backend = $this->createMock(UserInterface::class); - $config = $this->createMock(IConfig::class); - $logger = $this->createMock(ILogger::class); $account = $this->createMock(Account::class); $backendUids = ['thisuserhasntbeenseenbefore']; $backend->expects($this->once())->method('getUsers')->willReturn($backendUids); // We expect the mapper to be called to find the uid - $mapper->expects($this->once())->method('getByUid')->with($backendUids[0])->willThrowException(new DoesNotExistException('entity not found')); + $this->mapper->expects($this->once())->method('getByUid')->with($backendUids[0])->willThrowException(new DoesNotExistException('entity not found')); // Lets provide some config for the user - $config->expects($this->at(0))->method('getUserKeys')->with($backendUids[0], 'core')->willReturn([]); - $config->expects($this->at(1))->method('getUserKeys')->with($backendUids[0], 'login')->willReturn([]); - $config->expects($this->at(2))->method('getUserKeys')->with($backendUids[0], 'settings')->willReturn([]); - $config->expects($this->at(3))->method('getUserKeys')->with($backendUids[0], 'files')->willReturn([]); + $this->config->expects($this->at(0))->method('getUserKeys')->with($backendUids[0], 'core')->willReturn([]); + $this->config->expects($this->at(1))->method('getUserKeys')->with($backendUids[0], 'login')->willReturn([]); + $this->config->expects($this->at(2))->method('getUserKeys')->with($backendUids[0], 'settings')->willReturn([]); + $this->config->expects($this->at(3))->method('getUserKeys')->with($backendUids[0], 'files')->willReturn([]); // Pretend we dont update anything $account->expects($this->any())->method('getUpdatedFields')->willReturn([]); // Then we should try to setup a new account and insert - $mapper->expects($this->once())->method('insert')->with($this->callback(function($arg) use ($backendUids) { + $this->mapper->expects($this->once())->method('insert')->with($this->callback(function($arg) use ($backendUids) { return $arg instanceof Account && $arg->getUserId() === $backendUids[0]; })); // Ignore state flag + $s = new SyncService($this->config, $this->logger, $this->mapper); + $s->run($backend, new AllUsersIterator($backend), function($uid) {}); + + static::invokePrivate($s, 'syncHome', [$account, $backend]); + } + + /** + * Pass in a backend that has new users anc check that they accounts are inserted + */ + public function testSetupNewAccountLogsErrorOnException() { + /** @var UserInterface | IProvidesHomeBackend | \PHPUnit_Framework_MockObject_MockObject $backend */ + $backend = $this->createMock(UserInterface::class); + $backendUids = ['thisuserhasntbeenseenbefore']; + $backend->expects($this->once())->method('getUsers')->willReturn($backendUids); + + // We expect the mapper to be called to find the uid but we throw an exception to trigger the error logging path + $this->mapper->expects($this->once())->method('getByUid')->with($backendUids[0])->willThrowException(new MultipleObjectsReturnedException('Trigger error')); - $s = new SyncService($config, $logger, $mapper); + // Should log an error in the log and log the exception + $this->logger->expects($this->at(0))->method('error'); + $this->logger->expects($this->at(1))->method('logException'); + + $s = new SyncService($this->config, $this->logger, $this->mapper); $s->run($backend, new AllUsersIterator($backend), function($uid) {}); - $this->invokePrivate($s, 'syncHome', [$account, $backend]); } public function testSyncHomeLogsWhenBackendDiffersFromExisting() { - /** @var AccountMapper | \PHPUnit_Framework_MockObject_MockObject $mapper */ - $mapper = $this->createMock(AccountMapper::class); + /** @var UserInterface | IProvidesHomeBackend | \PHPUnit_Framework_MockObject_MockObject $backend */ $backend = $this->createMock([UserInterface::class, IProvidesHomeBackend::class]); - /** @var IConfig | \PHPUnit_Framework_MockObject_MockObject $config */ - $config = $this->createMock(IConfig::class); - /** @var ILogger | \PHPUnit_Framework_MockObject_MockObject $logger */ - $logger = $this->createMock(ILogger::class); $a = $this->getMockBuilder(Account::class)->setMethods(['getHome'])->getMock(); // Accoutn returns existing home @@ -119,11 +146,11 @@ public function testSyncHomeLogsWhenBackendDiffersFromExisting() { $backend->expects($this->exactly(3))->method('getHome')->willReturn('newwrongvalue'); // Should produce an error in the log if backend home different from stored account home - $logger->expects($this->once())->method('error'); + $this->logger->expects($this->once())->method('error'); - $s = new SyncService($config, $logger, $mapper); + $s = new SyncService($this->config, $this->logger, $this->mapper); - $this->invokePrivate($s, 'syncHome', [$a, $backend]); + static::invokePrivate($s, 'syncHome', [$a, $backend]); } /** @@ -132,20 +159,14 @@ public function testSyncHomeLogsWhenBackendDiffersFromExisting() { public function testTrySyncExistingUserWithOtherBackend() { $uid = 'myTestUser'; - /** @var AccountMapper | \PHPUnit_Framework_MockObject_MockObject $mapper */ - $mapper = $this->createMock(AccountMapper::class); $wrongBackend = new Database(); - /** @var IConfig | \PHPUnit_Framework_MockObject_MockObject $config */ - $config = $this->createMock(IConfig::class); - /** @var ILogger | \PHPUnit_Framework_MockObject_MockObject $logger */ - $logger = $this->createMock(ILogger::class); $a = $this->getMockBuilder(Account::class)->setMethods(['getBackend'])->getMock(); $a->expects($this->exactly(2))->method('getBackend')->willReturn('OriginalBackedClass'); - $mapper->expects($this->once())->method('getByUid')->with($uid)->willReturn($a); + $this->mapper->expects($this->once())->method('getByUid')->with($uid)->willReturn($a); - $s = new SyncService($config, $logger, $mapper); + $s = new SyncService($this->config, $this->logger, $this->mapper); $s->createOrSyncAccount($uid, $wrongBackend); } From c14eb129b6871a6ec214e54bb514669ae3c397b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 4 Apr 2018 15:40:47 +0200 Subject: [PATCH 3/3] add SyncBackendTest --- tests/lib/Command/User/SyncBackendTest.php | 261 +++++++++++++++++++++ 1 file changed, 261 insertions(+) create mode 100644 tests/lib/Command/User/SyncBackendTest.php diff --git a/tests/lib/Command/User/SyncBackendTest.php b/tests/lib/Command/User/SyncBackendTest.php new file mode 100644 index 000000000000..965f5dc26bbf --- /dev/null +++ b/tests/lib/Command/User/SyncBackendTest.php @@ -0,0 +1,261 @@ + + * + * @copyright Copyright (c) 2018, 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 + * + */ +namespace Test\Command\User; + +use OC\Core\Command\Integrity\SignApp; +use OC\Core\Command\User\SyncBackend; +use OC\IntegrityCheck\Checker; +use OC\IntegrityCheck\Helpers\FileAccessHelper; +use OC\User\AccountMapper; +use OCP\IConfig; +use OCP\ILogger; +use OCP\IURLGenerator; +use OCP\IUserBackend; +use OCP\IUserManager; +use OCP\UserInterface; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Output\OutputInterface; +use Test\TestCase; + +class SyncBackendTest extends TestCase { + + /** @var IConfig | \PHPUnit_Framework_MockObject_MockObject */ + private $config; + /** @var ILogger | \PHPUnit_Framework_MockObject_MockObject */ + private $logger; + /** @var AccountMapper | \PHPUnit_Framework_MockObject_MockObject */ + private $mapper; + /** @var IUserManager | \PHPUnit_Framework_MockObject_MockObject */ + private $userManager; + /** @var SyncBackend */ + private $command; + + /** @var UserInterface | \PHPUnit_Framework_MockObject_MockObject */ + private $dummyBackend; + + public function setUp() { + parent::setUp(); + + $this->config = $this->createMock(IConfig::class); + $this->logger = $this->createMock(ILogger::class); + $this->mapper = $this->createMock(AccountMapper::class); + $this->userManager = $this->createMock(IUserManager::class); + + $this->dummyBackend = $this->createMock(UserInterface::class); + + $this->userManager->expects($this->any()) + ->method('getBackends') + ->willReturn([$this->dummyBackend]); + + $this->command = new SyncBackend( + $this->mapper, + $this->config, + $this->userManager, + $this->logger + ); + + } + + public function testListBackends() { + $inputInterface = $this->createMock(InputInterface::class); + $outputInterface = $this->createMock(OutputInterface::class); + + $inputInterface + ->expects($this->at(0)) + ->method('getOption') + ->with('list') + ->will($this->returnValue(true)); + + $outputInterface + ->expects($this->at(0)) + ->method('writeln') + ->with(get_class($this->dummyBackend)); + + $this->assertEquals(0, static::invokePrivate($this->command, 'execute', [$inputInterface, $outputInterface])); + } + + public function testNoBackendIsGivenShowsError() { + $inputInterface = $this->createMock(InputInterface::class); + $outputInterface = $this->createMock(OutputInterface::class); + + $inputInterface + ->expects($this->at(0)) + ->method('getOption') + ->with('list') + ->will($this->returnValue(null)); + + $inputInterface + ->expects($this->at(1)) + ->method('getArgument') + ->with('backend-class') + ->will($this->returnValue(null)); + + $outputInterface + ->expects($this->at(0)) + ->method('writeln') + ->with('No backend class name given. Please run ./occ help user:sync to understand how this command works.'); + + $this->assertEquals(1, static::invokePrivate($this->command, 'execute', [$inputInterface, $outputInterface])); + } + + public function testNotExistingBackendIsGivenShowsError() { + $inputInterface = $this->createMock(InputInterface::class); + $outputInterface = $this->createMock(OutputInterface::class); + + $inputInterface + ->expects($this->at(0)) + ->method('getOption') + ->with('list') + ->will($this->returnValue(null)); + + $backendClassName = '\OCP\Some\Backend'; + $inputInterface + ->expects($this->at(1)) + ->method('getArgument') + ->with('backend-class') + ->will($this->returnValue($backendClassName)); + + $outputInterface + ->expects($this->at(0)) + ->method('writeln') + ->with("The backend <$backendClassName> does not exist. Did you miss to enable the app?"); + + $this->assertEquals(1, static::invokePrivate($this->command, 'execute', [$inputInterface, $outputInterface])); + } + + public function testUnsupportedBackendIsGivenShowsError() { + $inputInterface = $this->createMock(InputInterface::class); + $outputInterface = $this->createMock(OutputInterface::class); + + $inputInterface + ->expects($this->at(0)) + ->method('getOption') + ->with('list') + ->will($this->returnValue(null)); + + $backendClassName = get_class($this->dummyBackend); + $inputInterface + ->expects($this->at(1)) + ->method('getArgument') + ->with('backend-class') + ->will($this->returnValue($backendClassName)); + + $this->dummyBackend + ->expects($this->at(0)) + ->method('hasUserListings') + ->will($this->returnValue(false)); + + $outputInterface + ->expects($this->at(0)) + ->method('writeln') + ->with("The backend <$backendClassName> does not allow user listing. No sync is possible"); + + $this->assertEquals(1, static::invokePrivate($this->command, 'execute', [$inputInterface, $outputInterface])); + } + + public function testInvalidAccountActionShowsError() { + $inputInterface = $this->createMock(InputInterface::class); + $outputInterface = $this->createMock(OutputInterface::class); + + $inputInterface + ->expects($this->at(0)) + ->method('getOption') + ->with('list') + ->will($this->returnValue(null)); + + $backendClassName = get_class($this->dummyBackend); + + $inputInterface + ->expects($this->at(1)) + ->method('getArgument') + ->with('backend-class') + ->will($this->returnValue($backendClassName)); + + $this->dummyBackend + ->expects($this->at(0)) + ->method('hasUserListings') + ->will($this->returnValue(true)); + + $inputInterface + ->expects($this->at(2)) + ->method('getOption') + ->with('missing-account-action') + ->will($this->returnValue('invalid')); + + $outputInterface + ->expects($this->at(0)) + ->method('writeln') + ->with('Unknown action. Choose between "disable" or "remove"'); + + $this->assertEquals(1, static::invokePrivate($this->command, 'execute', [$inputInterface, $outputInterface])); + } + + public function testSingleUserSync() { + $inputInterface = $this->createMock(InputInterface::class); + $outputInterface = $this->createMock(OutputInterface::class); + + $inputInterface + ->expects($this->at(0)) + ->method('getOption') + ->with('list') + ->will($this->returnValue(null)); + + $backendClassName = get_class($this->dummyBackend); + + $inputInterface + ->expects($this->at(1)) + ->method('getArgument') + ->with('backend-class') + ->will($this->returnValue($backendClassName)); + + $this->dummyBackend + ->expects($this->at(0)) + ->method('hasUserListings') + ->will($this->returnValue(true)); + + $inputInterface + ->expects($this->at(2)) + ->method('getOption') + ->with('missing-account-action') + ->will($this->returnValue('disable')); + $inputInterface + ->expects($this->at(3)) + ->method('getOption') + ->with('missing-account-action') + ->will($this->returnValue('disable')); + + $inputInterface + ->expects($this->at(4)) + ->method('getOption') + ->with('uid') + ->will($this->returnValue('foo')); + + $outputInterface + ->expects($this->at(0)) + ->method('writeln') + ->with('Syncing foo ...'); + + // TODO add more tests + + $this->assertEquals(0, static::invokePrivate($this->command, 'execute', [$inputInterface, $outputInterface])); + } + +}