More deduplication of sync code. Ensure backend is checked against ex…

…isting accounts

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();
  }
  }
 }

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,35 +107,26 @@ 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);
  }
  $offset += $limit;
  } 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) {

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);
+ }
 }