From b798462710af9549c532a6ce81db2f528af8b935 Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Thu, 8 Feb 2018 09:14:26 +0200 Subject: [PATCH] More deduplication of sync code. Ensure backend is checked against existing accounts --- apps/testing/lib/Application.php | 6 +++ lib/private/User/SyncService.php | 70 +++++++++++++++++++++--------- tests/lib/User/SyncServiceTest.php | 25 +++++++++++ 3 files changed, 81 insertions(+), 20 deletions(-) diff --git a/apps/testing/lib/Application.php b/apps/testing/lib/Application.php index e00010ab4170..fc26ddcc0789 100644 --- a/apps/testing/lib/Application.php +++ b/apps/testing/lib/Application.php @@ -36,6 +36,12 @@ public function __construct (array $urlParams = array()) { // replace all user backends with this one $userManager->clearBackends(); $userManager->registerBackend($c->query(AlternativeHomeUserBackend::class)); + + // Make existing accounts use the new alternative home backed + $q = $c->getServer()->getDatabaseConnection()->getQueryBuilder(); + $q->update('*PREFIX*accounts') + ->set('backend', AlternativeHomeUserBackend::class) + ->execute(); } } } diff --git a/lib/private/User/SyncService.php b/lib/private/User/SyncService.php index baad97c293c1..9225e4752d1b 100644 --- a/lib/private/User/SyncService.php +++ b/lib/private/User/SyncService.php @@ -29,6 +29,7 @@ use OCP\User\IProvidesHomeBackend; use OCP\User\IProvidesQuotaBackend; use OCP\UserInterface; +use OCP\AppFramework\Db\MultipleObjectsReturnedException; /** * Class SyncService @@ -106,28 +107,16 @@ public function run(UserInterface $backend, \Closure $callback) { // update existing and insert new users foreach ($users as $uid) { try { - $existingAccount = $this->mapper->getByUid($uid); - if ($existingAccount->getBackend() !== $backendClass) { - $this->logger->warning( - "User <$uid> already provided by another backend({$existingAccount->getBackend()} != $backendClass), skipping.", - ['app' => self::class] - ); - continue; - } - $this->syncAccount($existingAccount, $backend); - $this->mapper->update($existingAccount); - $a = $existingAccount; - } catch(DoesNotExistException $ex) { - $newAccount = $this->createNewAccount($backendClass, $uid); - $this->syncAccount($newAccount, $backend); - $this->mapper->insert($newAccount); - $a = $newAccount; + $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); } - $uid = $a->getUserId(); // get correct case - // clean the user's preferences - $this->cleanPreferences($uid); - // call the callback $callback($uid); } @@ -135,6 +124,9 @@ public function run(UserInterface $backend, \Closure $callback) { } while(count($users) >= $limit); } + /** + * @param Account $a + */ private function syncState(Account $a) { $uid = $a->getUserId(); list($hasKey, $value) = $this->readUserConfig($uid, 'core', 'enabled'); @@ -158,6 +150,9 @@ private function syncState(Account $a) { } } + /** + * @param Account $a + */ private function syncLastLogin(Account $a) { $uid = $a->getUserId(); list($hasKey, $value) = $this->readUserConfig($uid, 'login', 'lastLogin'); @@ -171,6 +166,10 @@ private function syncLastLogin(Account $a) { } } + /** + * @param Account $a + * @param UserInterface $backend + */ private function syncEmail(Account $a, UserInterface $backend) { $uid = $a->getUserId(); $email = null; @@ -190,6 +189,10 @@ private function syncEmail(Account $a, UserInterface $backend) { } } + /** + * @param Account $a + * @param UserInterface $backend + */ private function syncQuota(Account $a, UserInterface $backend) { $uid = $a->getUserId(); $quota = null; @@ -211,6 +214,10 @@ private function syncQuota(Account $a, UserInterface $backend) { } } + /** + * @param Account $a + * @param UserInterface $backend + */ private function syncHome(Account $a, UserInterface $backend) { // Fallback for backends that dont yet use the new interfaces $proividesHome = $backend instanceof IProvidesHomeBackend || $backend->implementsActions(\OC_User_Backend::GET_HOME); @@ -247,6 +254,10 @@ private function syncHome(Account $a, UserInterface $backend) { } } + /** + * @param Account $a + * @param UserInterface $backend + */ private function syncDisplayName(Account $a, UserInterface $backend) { $uid = $a->getUserId(); if ($backend instanceof IProvidesDisplayNameBackend || $backend->implementsActions(\OC_User_Backend::GET_DISPLAYNAME)) { @@ -260,6 +271,10 @@ private function syncDisplayName(Account $a, UserInterface $backend) { } } + /** + * @param Account $a + * @param UserInterface $backend + */ private function syncSearchTerms(Account $a, UserInterface $backend) { $uid = $a->getUserId(); if ($backend instanceof IProvidesExtendedSearchBackend) { @@ -294,16 +309,31 @@ public function syncAccount(Account $a, UserInterface $backend) { * @param $uid * @param UserInterface $backend * @return Account + * @throws \Exception + * @throws \InvalidArgumentException if you try to sync with a backend + * that doesnt match an existing account */ public function createOrSyncAccount($uid, UserInterface $backend) { // Try to find the account based on the uid try { $account = $this->mapper->getByUid($uid); + // Check the backend matches + $existingAccountBackend = get_class($backend); + if ($account->getBackend() !== $existingAccountBackend) { + $this->logger->warning( + "User <$uid> already provided by another backend({$account->getBackend()} !== $existingAccountBackend), skipping.", + ['app' => self::class] + ); + throw new \InvalidArgumentException('Returned account has different backend to the requested backend for sync'); + } } catch (DoesNotExistException $e) { // Create a new account for this uid and backend pairing and sync $account = $this->createNewAccount(get_class($backend), $uid); + } catch (MultipleObjectsReturnedException $e) { + throw new \Exception("The database returned multiple accounts for this uid: $uid"); } + // The account exists, sync $account = $this->syncAccount($account, $backend); if($account->getId() === null) { diff --git a/tests/lib/User/SyncServiceTest.php b/tests/lib/User/SyncServiceTest.php index db29937ccf22..32d59e99e330 100644 --- a/tests/lib/User/SyncServiceTest.php +++ b/tests/lib/User/SyncServiceTest.php @@ -25,6 +25,7 @@ use OC\User\Account; use OC\User\AccountMapper; +use OC\User\Database; use OC\User\SyncService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\IConfig; @@ -123,4 +124,28 @@ public function testSyncHomeLogsWhenBackendDiffersFromExisting() { $this->invokePrivate($s, 'syncHome', [$a, $backend]); } + + /** + * @expectedException \InvalidArgumentException + */ + 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); + + $s = new SyncService($config, $logger, $mapper); + + $s->createOrSyncAccount($uid, $wrongBackend); + } } \ No newline at end of file