Merge pull request #34772 from nextcloud/fix/clean-ldap-access-factory-usage

Make sure to use AccessFactory to create Access instances and use DI

apps/user_ldap/ajax/wizard.php

@@ -38,7 +38,6 @@ if (!isset($_POST['action'])) {
 }
 $action = (string)$_POST['action'];
 
-
 if (!isset($_POST['ldap_serverconfig_chooser'])) {
 	\OC_JSON::error(['message' => $l->t('No configuration specified')]);
 }
@@ -52,26 +51,8 @@ $con->setConfiguration($configuration->getConfiguration());
 $con->ldapConfigurationActive = true;
 $con->setIgnoreValidation(true);
 
-$userManager = new \OCA\User_LDAP\User\Manager(
-	\OC::$server->getConfig(),
-	new \OCA\User_LDAP\FilesystemHelper(),
-	\OC::$server->get(\Psr\Log\LoggerInterface::class),
-	\OC::$server->getAvatarManager(),
-	new \OCP\Image(),
-	\OC::$server->getUserManager(),
-	\OC::$server->getNotificationManager(),
-	\OC::$server->get(\OCP\Share\IManager::class)
-);
-
-$access = new \OCA\User_LDAP\Access(
-	$con,
-	$ldapWrapper,
-	$userManager,
-	new \OCA\User_LDAP\Helper(\OC::$server->getConfig(), \OC::$server->getDatabaseConnection()),
-	\OC::$server->getConfig(),
-	\OC::$server->getUserManager(),
-	\OC::$server->get(\Psr\Log\LoggerInterface::class)
-);
+$factory = \OC::$server->get(\OCA\User_LDAP\AccessFactory::class);
+$access = $factory->get($con);
 
 $wizard = new \OCA\User_LDAP\Wizard($configuration, $ldapWrapper, $access);
 

apps/user_ldap/lib/AccessFactory.php

@@ -29,18 +29,12 @@ use OCP\IUserManager;
 use Psr\Log\LoggerInterface;
 
 class AccessFactory {
-	/** @var ILDAPWrapper */
-	protected $ldap;
-	/** @var Manager */
-	protected $userManager;
-	/** @var Helper */
-	protected $helper;
-	/** @var IConfig */
-	protected $config;
-	/** @var IUserManager */
-	private $ncUserManager;
-	/** @var LoggerInterface */
-	private $logger;
+	private ILDAPWrapper $ldap;
+	private Manager $userManager;
+	private Helper $helper;
+	private IConfig $config;
+	private IUserManager $ncUserManager;
+	private LoggerInterface $logger;
 
 	public function __construct(
 		ILDAPWrapper $ldap,
@@ -57,7 +51,7 @@ class AccessFactory {
 		$this->logger = $logger;
 	}
 
-	public function get(Connection $connection) {
+	public function get(Connection $connection): Access {
 		return new Access(
 			$connection,
 			$this->ldap,

apps/user_ldap/lib/Command/TestConfig.php

@@ -29,6 +29,7 @@ namespace OCA\User_LDAP\Command;
 use OCA\User_LDAP\AccessFactory;
 use OCA\User_LDAP\Connection;
 use OCA\User_LDAP\Helper;
+use OCA\User_LDAP\ILDAPWrapper;
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Input\InputArgument;
 use Symfony\Component\Console\Input\InputInterface;
@@ -40,29 +41,35 @@ class TestConfig extends Command {
 	protected const BINDFAILURE = 2;
 	protected const SEARCHFAILURE = 3;
 
-	/** @var AccessFactory */
-	protected $accessFactory;
+	protected AccessFactory $accessFactory;
+	protected Helper $helper;
+	protected ILDAPWrapper $ldap;
 
-	public function __construct(AccessFactory $accessFactory) {
+	public function __construct(
+		AccessFactory $accessFactory,
+		Helper $helper,
+		ILDAPWrapper $ldap
+	) {
 		$this->accessFactory = $accessFactory;
+		$this->helper = $helper;
+		$this->ldap = $ldap;
 		parent::__construct();
 	}
 
-	protected function configure() {
+	protected function configure(): void {
 		$this
 			->setName('ldap:test-config')
 			->setDescription('tests an LDAP configuration')
 			->addArgument(
-					'configID',
-					InputArgument::REQUIRED,
-					'the configuration ID'
-				)
+				'configID',
+				InputArgument::REQUIRED,
+				'the configuration ID'
+			)
 		;
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
-		$helper = new Helper(\OC::$server->getConfig(), \OC::$server->getDatabaseConnection());
-		$availableConfigs = $helper->getServerConfigurationPrefixes();
+		$availableConfigs = $this->helper->getServerConfigurationPrefixes();
 		$configID = $input->getArgument('configID');
 		if (!in_array($configID, $availableConfigs)) {
 			$output->writeln('Invalid configID');
@@ -94,8 +101,7 @@ class TestConfig extends Command {
 	 * Tests the specified connection
 	 */
 	protected function testConfig(string $configID): int {
-		$lw = new \OCA\User_LDAP\LDAP();
-		$connection = new Connection($lw, $configID);
+		$connection = new Connection($this->ldap, $configID);
 
 		// Ensure validation is run before we attempt the bind
 		$connection->getConfiguration();

apps/user_ldap/lib/Group_Proxy.php

@@ -39,8 +39,13 @@ class Group_Proxy extends Proxy implements \OCP\GroupInterface, IGroupLDAP, IGet
 	private GroupPluginManager $groupPluginManager;
 	private bool $isSetUp = false;
 
-	public function __construct(Helper $helper, ILDAPWrapper $ldap, GroupPluginManager $groupPluginManager) {
-		parent::__construct($ldap);
+	public function __construct(
+		Helper $helper,
+		ILDAPWrapper $ldap,
+		AccessFactory $accessFactory,
+		GroupPluginManager $groupPluginManager
+	) {
+		parent::__construct($ldap, $accessFactory);
 		$this->helper = $helper;
 		$this->groupPluginManager = $groupPluginManager;
 	}

apps/user_ldap/lib/ILDAPUserPlugin.php

@@ -24,7 +24,6 @@
 namespace OCA\User_LDAP;
 
 interface ILDAPUserPlugin {
-
 	/**
 	 * Check if plugin implements actions
 	 * @return int
@@ -85,7 +84,7 @@ interface ILDAPUserPlugin {
 
 	/**
 	 * Count the number of users
-	 * @return int|bool
+	 * @return int|false
 	 */
 	public function countUsers();
 }

apps/user_ldap/lib/Proxy.php

@@ -34,57 +34,41 @@ namespace OCA\User_LDAP;
 
 use OCA\User_LDAP\Mapping\GroupMapping;
 use OCA\User_LDAP\Mapping\UserMapping;
-use OCA\User_LDAP\User\Manager;
-use OCP\IConfig;
-use OCP\IUserManager;
+use OCP\ICache;
 use OCP\Server;
-use Psr\Log\LoggerInterface;
 
 abstract class Proxy {
-	private static $accesses = [];
-	private $ldap = null;
-	/** @var bool */
-	private $isSingleBackend;
+	/** @var array<string,Access> */
+	private static array $accesses = [];
+	private ILDAPWrapper $ldap;
+	private ?bool $isSingleBackend = null;
+	private ?ICache $cache = null;
+	private AccessFactory $accessFactory;
 
-	/** @var \OCP\ICache|null */
-	private $cache;
-
-	/**
-	 * @param ILDAPWrapper $ldap
-	 */
-	public function __construct(ILDAPWrapper $ldap) {
+	public function __construct(
+		ILDAPWrapper $ldap,
+		AccessFactory $accessFactory
+	) {
 		$this->ldap = $ldap;
+		$this->accessFactory = $accessFactory;
 		$memcache = \OC::$server->getMemCacheFactory();
 		if ($memcache->isAvailable()) {
 			$this->cache = $memcache->createDistributed();
 		}
 	}
 
-	/**
-	 * @param string $configPrefix
-	 */
 	private function addAccess(string $configPrefix): void {
-		$ocConfig = Server::get(IConfig::class);
 		$userMap = Server::get(UserMapping::class);
 		$groupMap = Server::get(GroupMapping::class);
-		$coreUserManager = Server::get(IUserManager::class);
-		$logger = Server::get(LoggerInterface::class);
-		$helper = Server::get(Helper::class);
-
-		$userManager = Server::get(Manager::class);
 
 		$connector = new Connection($this->ldap, $configPrefix);
-		$access = new Access($connector, $this->ldap, $userManager, $helper, $ocConfig, $coreUserManager, $logger);
+		$access = $this->accessFactory->get($connector);
 		$access->setUserMapper($userMap);
 		$access->setGroupMapper($groupMap);
 		self::$accesses[$configPrefix] = $access;
 	}
 
-	/**
-	 * @param string $configPrefix
-	 * @return mixed
-	 */
-	protected function getAccess($configPrefix) {
+	protected function getAccess(string $configPrefix): Access {
 		if (!isset(self::$accesses[$configPrefix])) {
 			$this->addAccess($configPrefix);
 		}

apps/user_ldap/lib/UserPluginManager.php

@@ -65,7 +65,7 @@ class UserPluginManager {
 				\OC::$server->getLogger()->debug("Registered action ".$action." to plugin ".get_class($plugin), ['app' => 'user_ldap']);
 			}
 		}
-		if (method_exists($plugin,'deleteUser')) {
+		if (method_exists($plugin, 'deleteUser')) {
 			$this->which['deleteUser'] = $plugin;
 			\OC::$server->getLogger()->debug("Registered action deleteUser to plugin ".get_class($plugin), ['app' => 'user_ldap']);
 		}
@@ -92,7 +92,7 @@ class UserPluginManager {
 		$plugin = $this->which[Backend::CREATE_USER];
 
 		if ($plugin) {
-			return $plugin->createUser($username,$password);
+			return $plugin->createUser($username, $password);
 		}
 		throw new \Exception('No plugin implements createUser in this LDAP Backend.');
 	}
@@ -108,7 +108,7 @@ class UserPluginManager {
 		$plugin = $this->which[Backend::SET_PASSWORD];
 
 		if ($plugin) {
-			return $plugin->setPassword($uid,$password);
+			return $plugin->setPassword($uid, $password);
 		}
 		throw new \Exception('No plugin implements setPassword in this LDAP Backend.');
 	}
@@ -176,7 +176,7 @@ class UserPluginManager {
 
 	/**
 	 * Count the number of users
-	 * @return int|bool
+	 * @return int|false
 	 * @throws \Exception
 	 */
 	public function countUsers() {

apps/user_ldap/lib/User_LDAP.php

@@ -582,7 +582,7 @@ class User_LDAP extends BackendUtility implements IUserBackend, UserInterface, I
 	/**
 	 * counts the users in LDAP
 	 *
-	 * @return int|bool
+	 * @return int|false
 	 */
 	public function countUsers() {
 		if ($this->userPluginManager->implementsActions(Backend::COUNT_USERS)) {

apps/user_ldap/lib/User_Proxy.php

@@ -41,8 +41,9 @@ use OCP\User\Backend\ICountUsersBackend;
 use OCP\UserInterface;
 
 class User_Proxy extends Proxy implements IUserBackend, UserInterface, IUserLDAP, ICountUsersBackend, ICountMappedUsersBackend {
+  /** @var array<string,User_LDAP> */
 	private $backends = [];
-	/** @var User_LDAP */
+	/** @var ?User_LDAP */
 	private $refBackend = null;
 
 	private bool $isSetUp = false;
@@ -55,12 +56,13 @@ class User_Proxy extends Proxy implements IUserBackend, UserInterface, IUserLDAP
 	public function __construct(
 		Helper $helper,
 		ILDAPWrapper $ldap,
+		AccessFactory $accessFactory,
 		IConfig $ocConfig,
 		INotificationManager $notificationManager,
 		IUserSession $userSession,
 		UserPluginManager $userPluginManager
 	) {
-		parent::__construct($ldap);
+		parent::__construct($ldap, $accessFactory);
 		$this->helper = $helper;
 		$this->ocConfig = $ocConfig;
 		$this->notificationManager = $notificationManager;
@@ -377,7 +379,7 @@ class User_Proxy extends Proxy implements IUserBackend, UserInterface, IUserLDAP
 	/**
 	 * Count the number of users
 	 *
-	 * @return int|bool
+	 * @return int|false
 	 */
 	public function countUsers() {
 		$this->setup();
@@ -386,7 +388,7 @@ class User_Proxy extends Proxy implements IUserBackend, UserInterface, IUserLDAP
 		foreach ($this->backends as $backend) {
 			$backendUsers = $backend->countUsers();
 			if ($backendUsers !== false) {
-				$users += $backendUsers;
+				$users = (int)$users + $backendUsers;
 			}
 		}
 		return $users;

apps/user_ldap/tests/User_ProxyTest.php

@@ -28,6 +28,7 @@
  */
 namespace OCA\User_LDAP\Tests;
 
+use OCA\User_LDAP\AccessFactory;
 use OCA\User_LDAP\Helper;
 use OCA\User_LDAP\ILDAPWrapper;
 use OCA\User_LDAP\User_Proxy;
@@ -35,22 +36,25 @@ use OCA\User_LDAP\UserPluginManager;
 use OCP\IConfig;
 use OCP\IUserSession;
 use OCP\Notification\IManager as INotificationManager;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
 class User_ProxyTest extends TestCase {
-	/** @var Helper|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var Helper|MockObject */
 	protected $helper;
-	/** @var ILDAPWrapper|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var ILDAPWrapper|MockObject */
 	private $ldapWrapper;
-	/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var AccessFactory|MockObject */
+	private $accessFactory;
+	/** @var IConfig|MockObject */
 	private $config;
-	/** @var INotificationManager|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var INotificationManager|MockObject */
 	private $notificationManager;
-	/** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IUserSession|MockObject */
 	private $userSession;
-	/** @var User_Proxy|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var User_Proxy|MockObject */
 	private $proxy;
-	/** @var UserPluginManager|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var UserPluginManager|MockObject */
 	private $userPluginManager;
 
 	protected function setUp(): void {
@@ -58,6 +62,7 @@ class User_ProxyTest extends TestCase {
 
 		$this->helper = $this->createMock(Helper::class);
 		$this->ldapWrapper = $this->createMock(ILDAPWrapper::class);
+		$this->accessFactory = $this->createMock(AccessFactory::class);
 		$this->config = $this->createMock(IConfig::class);
 		$this->notificationManager = $this->createMock(INotificationManager::class);
 		$this->userSession = $this->createMock(IUserSession::class);
@@ -66,6 +71,7 @@ class User_ProxyTest extends TestCase {
 			->setConstructorArgs([
 				$this->helper,
 				$this->ldapWrapper,
+				$this->accessFactory,
 				$this->config,
 				$this->notificationManager,
 				$this->userSession,

lib/private/User/Database.php

@@ -65,14 +65,14 @@ use OCP\User\Backend\ISetPasswordBackend;
  */
 class Database extends ABackend implements
 	ICreateUserBackend,
-			   ISetPasswordBackend,
-			   ISetDisplayNameBackend,
-			   IGetDisplayNameBackend,
-			   ICheckPasswordBackend,
-			   IGetHomeBackend,
-			   ICountUsersBackend,
-			   ISearchKnownUsersBackend,
-			   IGetRealUIDBackend {
+	ISetPasswordBackend,
+	ISetDisplayNameBackend,
+	IGetDisplayNameBackend,
+	ICheckPasswordBackend,
+	IGetHomeBackend,
+	ICountUsersBackend,
+	ISearchKnownUsersBackend,
+	IGetRealUIDBackend {
 	/** @var CappedMemoryCache */
 	private $cache;
 
@@ -456,7 +456,7 @@ class Database extends ABackend implements
 	/**
 	 * counts the users in the database
 	 *
-	 * @return int|bool
+	 * @return int|false
 	 */
 	public function countUsers() {
 		$this->fixDI();
@@ -464,7 +464,7 @@ class Database extends ABackend implements
 		$query = $this->dbConn->getQueryBuilder();
 		$query->select($query->func()->count('uid'))
 			->from($this->table);
-		$result = $query->execute();
+		$result = $query->executeQuery();
 
 		return $result->fetchOne();
 	}

lib/public/User/Backend/ICountUsersBackend.php

@@ -29,11 +29,10 @@ namespace OCP\User\Backend;
  * @since 14.0.0
  */
 interface ICountUsersBackend {
-
 	/**
 	 * @since 14.0.0
 	 *
-	 * @return int|bool The number of users on success false on failure
+	 * @return int|false The number of users on success false on failure
 	 */
 	public function countUsers();
 }