Merge pull request #7713 from nextcloud/fix-sync-offset

Fix LDAP Background Sync does not reset offset

apps/user_ldap/composer/composer/autoload_classmap.php

@@ -7,6 +7,7 @@ $baseDir = $vendorDir;
 
 return array(
     'OCA\\User_LDAP\\Access' => $baseDir . '/../lib/Access.php',
+    'OCA\\User_LDAP\\AccessFactory' => $baseDir . '/../lib/AccessFactory.php',
     'OCA\\User_LDAP\\AppInfo\\Application' => $baseDir . '/../lib/AppInfo/Application.php',
     'OCA\\User_LDAP\\BackendUtility' => $baseDir . '/../lib/BackendUtility.php',
     'OCA\\User_LDAP\\Command\\CheckUser' => $baseDir . '/../lib/Command/CheckUser.php',
@@ -19,6 +20,7 @@ return array(
     'OCA\\User_LDAP\\Command\\TestConfig' => $baseDir . '/../lib/Command/TestConfig.php',
     'OCA\\User_LDAP\\Configuration' => $baseDir . '/../lib/Configuration.php',
     'OCA\\User_LDAP\\Connection' => $baseDir . '/../lib/Connection.php',
+    'OCA\\User_LDAP\\ConnectionFactory' => $baseDir . '/../lib/ConnectionFactory.php',
     'OCA\\User_LDAP\\Controller\\ConfigAPIController' => $baseDir . '/../lib/Controller/ConfigAPIController.php',
     'OCA\\User_LDAP\\Controller\\RenewPasswordController' => $baseDir . '/../lib/Controller/RenewPasswordController.php',
     'OCA\\User_LDAP\\Exceptions\\ConstraintViolationException' => $baseDir . '/../lib/Exceptions/ConstraintViolationException.php',

apps/user_ldap/composer/composer/autoload_static.php

@@ -19,6 +19,7 @@ class ComposerStaticInitUser_LDAP
 
     public static $classMap = array (
         'OCA\\User_LDAP\\Access' => __DIR__ . '/..' . '/../lib/Access.php',
+        'OCA\\User_LDAP\\AccessFactory' => __DIR__ . '/..' . '/../lib/AccessFactory.php',
         'OCA\\User_LDAP\\AppInfo\\Application' => __DIR__ . '/..' . '/../lib/AppInfo/Application.php',
         'OCA\\User_LDAP\\BackendUtility' => __DIR__ . '/..' . '/../lib/BackendUtility.php',
         'OCA\\User_LDAP\\Command\\CheckUser' => __DIR__ . '/..' . '/../lib/Command/CheckUser.php',
@@ -31,6 +32,7 @@ class ComposerStaticInitUser_LDAP
         'OCA\\User_LDAP\\Command\\TestConfig' => __DIR__ . '/..' . '/../lib/Command/TestConfig.php',
         'OCA\\User_LDAP\\Configuration' => __DIR__ . '/..' . '/../lib/Configuration.php',
         'OCA\\User_LDAP\\Connection' => __DIR__ . '/..' . '/../lib/Connection.php',
+        'OCA\\User_LDAP\\ConnectionFactory' => __DIR__ . '/..' . '/../lib/ConnectionFactory.php',
         'OCA\\User_LDAP\\Controller\\ConfigAPIController' => __DIR__ . '/..' . '/../lib/Controller/ConfigAPIController.php',
         'OCA\\User_LDAP\\Controller\\RenewPasswordController' => __DIR__ . '/..' . '/../lib/Controller/RenewPasswordController.php',
         'OCA\\User_LDAP\\Exceptions\\ConstraintViolationException' => __DIR__ . '/..' . '/../lib/Exceptions/ConstraintViolationException.php',

apps/user_ldap/lib/AccessFactory.php

@@ -0,0 +1,61 @@
+<?php
+/**
+ * @copyright Copyright (c) 2018 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;
+
+
+use OCA\User_LDAP\User\Manager;
+use OCP\IConfig;
+
+class AccessFactory {
+	/** @var ILDAPWrapper */
+	protected $ldap;
+	/** @var Manager */
+	protected $userManager;
+	/** @var Helper */
+	protected $helper;
+	/** @var IConfig */
+	protected $config;
+
+	public function __construct(
+		ILDAPWrapper $ldap,
+		Manager $userManager,
+		Helper $helper,
+		IConfig $config)
+	{
+		$this->ldap = $ldap;
+		$this->userManager = $userManager;
+		$this->helper = $helper;
+		$this->config = $config;
+	}
+
+	public function get(Connection $connection) {
+		return new Access(
+			$connection,
+			$this->ldap,
+			$this->userManager,
+			$this->helper,
+			$this->config
+		);
+	}
+}

apps/user_ldap/lib/ConnectionFactory.php

@@ -0,0 +1,38 @@
+<?php
+/**
+ * @copyright Copyright (c) 2018 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;
+
+
+class ConnectionFactory {
+	/** @var ILDAPWrapper */
+	private $ldap;
+
+	public function __construct(ILDAPWrapper $ldap) {
+		$this->ldap = $ldap;
+	}
+
+	public function get($prefix) {
+		return new Connection($this->ldap, $prefix, 'user_ldap');
+	}
+}

apps/user_ldap/lib/Jobs/Sync.php

@@ -26,8 +26,10 @@ namespace OCA\User_LDAP\Jobs;
 use OC\BackgroundJob\TimedJob;
 use OC\ServerNotAvailableException;
 use OCA\User_LDAP\Access;
+use OCA\User_LDAP\AccessFactory;
 use OCA\User_LDAP\Configuration;
 use OCA\User_LDAP\Connection;
+use OCA\User_LDAP\ConnectionFactory;
 use OCA\User_LDAP\FilesystemHelper;
 use OCA\User_LDAP\Helper;
 use OCA\User_LDAP\LDAP;
@@ -62,6 +64,10 @@ class Sync extends TimedJob {
 	protected $ncUserManager;
 	/** @var  IManager */
 	protected $notificationManager;
+	/** @var ConnectionFactory */
+	protected $connectionFactory;
+	/** @var AccessFactory */
+	protected $accessFactory;
 
 	public function __construct() {
 		$this->setInterval(
@@ -112,7 +118,7 @@ class Sync extends TimedJob {
 	/**
 	 * @param array $argument
 	 */
-	protected function run($argument) {
+	public function run($argument) {
 		$this->setArgument($argument);
 
 		$isBackgroundJobModeAjax = $this->config
@@ -140,11 +146,11 @@ class Sync extends TimedJob {
 			if ($expectMoreResults) {
 				$this->increaseOffset($cycleData);
 			} else {
-				$this->determineNextCycle();
+				$this->determineNextCycle($cycleData);
 			}
 			$this->updateInterval();
 		} catch (ServerNotAvailableException $e) {
-			$this->determineNextCycle();
+			$this->determineNextCycle($cycleData);
 		}
 	}
 
@@ -153,8 +159,8 @@ class Sync extends TimedJob {
 	 * @return bool whether more results are expected from the same configuration
 	 */
 	public function runCycle($cycleData) {
-		$connection = new Connection($this->ldap, $cycleData['prefix']);
+		$connection = $this->connectionFactory->get($cycleData['prefix']);
-		$access = new Access($connection, $this->ldap, $this->userManager, $this->ldapHelper, $this->config);
+		$access = $this->accessFactory->get($connection);
 		$access->setUserMapper($this->mapper);
 
 		$filter = $access->combineFilterWithAnd(array(
@@ -173,7 +179,7 @@ class Sync extends TimedJob {
 		if($connection->ldapPagingSize === 0) {
 			return true;
 		}
-		return count($results) !== $connection->ldapPagingSize;
+		return count($results) >= $connection->ldapPagingSize;
 	}
 
 	/**
@@ -246,7 +252,7 @@ class Sync extends TimedJob {
 	 * @param $cycleData
 	 * @return bool
 	 */
-	protected function qualifiesToRun($cycleData) {
+	public function qualifiesToRun($cycleData) {
 		$lastChange = $this->config->getAppValue('user_ldap', $cycleData['prefix'] . '_lastChange', 0);
 		if((time() - $lastChange) > 60 * 30) {
 			return true;
@@ -358,5 +364,22 @@ class Sync extends TimedJob {
 		} else {
 			$this->mapper = new UserMapping($this->dbc);
 		}
+		
+		if(isset($argument['connectionFactory'])) {
+			$this->connectionFactory = $argument['connectionFactory'];
+		} else {
+			$this->connectionFactory = new ConnectionFactory($this->ldap);
+		}
+
+		if(isset($argument['accessFactory'])) {
+			$this->accessFactory = $argument['accessFactory'];
+		} else {
+			$this->accessFactory = new AccessFactory(
+				$this->ldap,
+				$this->userManager,
+				$this->ldapHelper,
+				$this->config
+			);
+		}
 	}
 }

apps/user_ldap/tests/Jobs/SyncTest.php

@@ -23,6 +23,10 @@
 
 namespace OCA\User_LDAP\Tests\Jobs;
 
+use OCA\User_LDAP\Access;
+use OCA\User_LDAP\AccessFactory;
+use OCA\User_LDAP\Connection;
+use OCA\User_LDAP\ConnectionFactory;
 use OCA\User_LDAP\Helper;
 use OCA\User_LDAP\Jobs\Sync;
 use OCA\User_LDAP\LDAP;
@@ -33,6 +37,7 @@ use OCP\IConfig;
 use OCP\IDBConnection;
 use OCP\IUserManager;
 use OCP\Notification\IManager;
+use function Sodium\memcmp;
 use Test\TestCase;
 
 class SyncTest extends TestCase {
@@ -59,6 +64,10 @@ class SyncTest extends TestCase {
 	protected $ncUserManager;
 	/** @var  IManager|\PHPUnit_Framework_MockObject_MockObject */
 	protected $notificationManager;
+	/** @var ConnectionFactory|\PHPUnit_Framework_MockObject_MockObject */
+	protected $connectionFactory;
+	/** @var AccessFactory|\PHPUnit_Framework_MockObject_MockObject */
+	protected $accessFactory;
 
 	public function setUp() {
 		parent::setUp();
@@ -72,6 +81,8 @@ class SyncTest extends TestCase {
 		$this->dbc = $this->createMock(IDBConnection::class);
 		$this->ncUserManager = $this->createMock(IUserManager::class);
 		$this->notificationManager = $this->createMock(IManager::class);
+		$this->connectionFactory = $this->createMock(ConnectionFactory::class);
+		$this->accessFactory = $this->createMock(AccessFactory::class);
 
 		$this->arguments = [
 			'helper' => $this->helper,
@@ -83,6 +94,8 @@ class SyncTest extends TestCase {
 			'dbc' => $this->dbc,
 			'ncUserManager' => $this->ncUserManager,
 			'notificationManager' => $this->notificationManager,
+			'connectionFactory' => $this->connectionFactory,
+			'accessFactory' => $this->accessFactory,
 		];
 
 		$this->sync = new Sync();
@@ -141,4 +154,216 @@ class SyncTest extends TestCase {
 		$this->sync->updateInterval();
 	}
 
+	public function moreResultsProvider() {
+		return [
+			[ 3, 3, true ],
+			[ 3, 5, true ],
+			[ 3, 2, false]
+		];
+	}
+
+	/**
+	 * @dataProvider moreResultsProvider
+	 */
+	public function testMoreResults($pagingSize, $results, $expected) {
+		$connection = $this->createMock(Connection::class);
+		$this->connectionFactory->expects($this->any())
+			->method('get')
+			->willReturn($connection);
+		$connection->expects($this->any())
+			->method('__get')
+			->willReturnCallback(function ($key) use ($pagingSize) {
+				if($key === 'ldapPagingSize') {
+					return $pagingSize;
+				}
+				return null;
+			});
+
+		/** @var Access|\PHPUnit_Framework_MockObject_MockObject $access */
+		$access = $this->createMock(Access::class);
+		$this->accessFactory->expects($this->any())
+			->method('get')
+			->with($connection)
+			->willReturn($access);
+
+		$access->expects($this->once())
+			->method('fetchListOfUsers')
+			->willReturn(array_pad([], $results, 'someUser'));
+		$access->connection = $connection;
+		$access->userManager = $this->userManager;
+
+		$this->sync->setArgument($this->arguments);
+		$hasMoreResults = $this->sync->runCycle(['prefix' => 's01', 'offset' => 100]);
+		$this->assertSame($expected, $hasMoreResults);
+	}
+
+	public function cycleDataProvider() {
+		$lastCycle = ['prefix' => 's01', 'offset' => 1000];
+		$lastCycle2 = ['prefix' => '', 'offset' => 1000];
+		return [
+			[ null, ['s01'], ['prefix' => 's01', 'offset' => 0] ],
+			[ null, [''], ['prefix' => '', 'offset' => 0] ],
+			[ $lastCycle, ['s01', 's02'], ['prefix' => 's02', 'offset' => 0] ],
+			[ $lastCycle, [''], ['prefix' => '', 'offset' => 0] ],
+			[ $lastCycle2, ['', 's01'], ['prefix' => 's01', 'offset' => 0] ],
+			[ $lastCycle, [], null ],
+		];
+	}
+
+	/**
+	 * @dataProvider cycleDataProvider
+	 */
+	public function testDetermineNextCycle($cycleData, $prefixes, $expectedCycle) {
+		$this->helper->expects($this->any())
+			->method('getServerConfigurationPrefixes')
+			->with(true)
+			->willReturn($prefixes);
+
+		if(is_array($expectedCycle)) {
+			$this->config->expects($this->exactly(2))
+				->method('setAppValue')
+				->withConsecutive(
+					['user_ldap', 'background_sync_prefix', $expectedCycle['prefix']],
+					['user_ldap', 'background_sync_offset', $expectedCycle['offset']]
+				);
+		} else {
+			$this->config->expects($this->never())
+				->method('setAppValue');
+		}
+
+		$this->sync->setArgument($this->arguments);
+		$nextCycle = $this->sync->determineNextCycle($cycleData);
+
+		if($expectedCycle === null) {
+			$this->assertNull($nextCycle);
+		} else {
+			$this->assertSame($expectedCycle['prefix'], $nextCycle['prefix']);
+			$this->assertSame($expectedCycle['offset'], $nextCycle['offset']);
+		}
+	}
+
+	public function testQualifiesToRun() {
+		$cycleData = ['prefix' => 's01'];
+
+		$this->config->expects($this->exactly(2))
+			->method('getAppValue')
+			->willReturnOnConsecutiveCalls(time() - 60*40, time() - 60*20);
+
+		$this->sync->setArgument($this->arguments);
+		$this->assertTrue($this->sync->qualifiesToRun($cycleData));
+		$this->assertFalse($this->sync->qualifiesToRun($cycleData));
+	}
+
+	public function runDataProvider() {
+		return [
+			#0 - one LDAP server, reset
+			[[
+				'prefixes' => [''],
+				'scheduledCycle' => ['prefix' => '', 'offset' => '4500'],
+				'pagingSize' => 500,
+				'usersThisCycle' => 0,
+				'expectedNextCycle' => ['prefix' => '', 'offset' => '0'],
+				'mappedUsers' => 123,
+			]],
+			#1 - 2 LDAP servers, next prefix
+			[[
+				'prefixes' => ['', 's01'],
+				'scheduledCycle' => ['prefix' => '', 'offset' => '4500'],
+				'pagingSize' => 500,
+				'usersThisCycle' => 0,
+				'expectedNextCycle' => ['prefix' => 's01', 'offset' => '0'],
+				'mappedUsers' => 123,
+			]],
+			#2 - 2 LDAP servers, rotate prefix
+			[[
+				'prefixes' => ['', 's01'],
+				'scheduledCycle' => ['prefix' => 's01', 'offset' => '4500'],
+				'pagingSize' => 500,
+				'usersThisCycle' => 0,
+				'expectedNextCycle' => ['prefix' => '', 'offset' => '0'],
+				'mappedUsers' => 123,
+			]],
+		];
+	}
+
+	/**
+	 * @dataProvider runDataProvider
+	 */
+	public function testRun($runData) {
+		$this->config->expects($this->any())
+			->method('getAppValue')
+			->willReturnCallback(function($app, $key, $default) use ($runData) {
+				if($app === 'core' && $key === 'backgroundjobs_mode') {
+					return 'cron';
+				}
+				if($app = 'user_ldap') {
+					// for getCycle()
+					if($key === 'background_sync_prefix') {
+						return $runData['scheduledCycle']['prefix'];
+					}
+					if($key === 'background_sync_offset') {
+						return $runData['scheduledCycle']['offset'];
+					}
+					// for qualifiesToRun()
+					if($key === $runData['scheduledCycle']['prefix'] . '_lastChange') {
+						return time() - 60*40;
+					}
+					// for getMinPagingSize
+					if($key === $runData['scheduledCycle']['prefix'] . 'ldap_paging_size') {
+						return $runData['pagingSize'];
+					}
+				}
+
+				return $default;
+			});
+		$this->config->expects($this->exactly(3))
+			->method('setAppValue')
+			->withConsecutive(
+				['user_ldap', 'background_sync_prefix', $runData['expectedNextCycle']['prefix']],
+				['user_ldap', 'background_sync_offset', $runData['expectedNextCycle']['offset']],
+				['user_ldap', 'background_sync_interval', $this->anything()]
+			);
+		$this->config->expects($this->any())
+			->method('getAppKeys')
+			->with('user_ldap')
+			->willReturn([$runData['scheduledCycle']['prefix'] . 'ldap_paging_size']);
+
+		$this->helper->expects($this->any())
+			->method('getServerConfigurationPrefixes')
+			->with(true)
+			->willReturn($runData['prefixes']);
+
+		$connection = $this->createMock(Connection::class);
+		$this->connectionFactory->expects($this->any())
+			->method('get')
+			->willReturn($connection);
+		$connection->expects($this->any())
+			->method('__get')
+			->willReturnCallback(function ($key) use ($runData) {
+				if($key === 'ldapPagingSize') {
+					return $runData['pagingSize'];
+				}
+				return null;
+			});
+
+		/** @var Access|\PHPUnit_Framework_MockObject_MockObject $access */
+		$access = $this->createMock(Access::class);
+		$this->accessFactory->expects($this->any())
+			->method('get')
+			->with($connection)
+			->willReturn($access);
+
+		$access->expects($this->once())
+			->method('fetchListOfUsers')
+			->willReturn(array_pad([], $runData['usersThisCycle'], 'someUser'));
+		$access->connection = $connection;
+		$access->userManager = $this->userManager;
+
+		$this->mapper->expects($this->any())
+			->method('count')
+			->willReturn($runData['mappedUsers']);
+
+		$this->sync->run($this->arguments);
+	}
+
 }