From 6ba09ba5b4c0cecadee517167d95b72171120fa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Wed, 1 Apr 2026 14:56:22 +0200 Subject: [PATCH 1/2] fix: Use stricter typing and increase psalm level MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/Command/GroupAdminsToLdap.php | 32 ++++++++++++------------------- lib/LDAPConnect.php | 24 ++++++++++------------- lib/LDAPUserManager.php | 4 ++-- psalm.xml | 2 +- tests/stubs/stub.phpstub | 2 +- 5 files changed, 26 insertions(+), 38 deletions(-) diff --git a/lib/Command/GroupAdminsToLdap.php b/lib/Command/GroupAdminsToLdap.php index 426e7926..0b1e875e 100644 --- a/lib/Command/GroupAdminsToLdap.php +++ b/lib/Command/GroupAdminsToLdap.php @@ -22,33 +22,19 @@ class GroupAdminsToLdap extends Command { /** * This adds/removes group subadmins as ldap group owners */ - private $simulate = false; - private $verbose = false; - - /** @var SubAdmin */ - private $subAdmin; - /** @var IConfig */ - private $ocConfig; - /** @var Helper */ - private $helper; - /** @var Group_Proxy */ - private $groupProxy; + private bool $simulate = false; + private bool $verbose = false; /** * GroupAdminsToLdap constructor. */ public function __construct( - SubAdmin $subAdmin, - IConfig $ocConfig, - Helper $helper, - Group_Proxy $groupProxy, + private SubAdmin $subAdmin, + private IConfig $ocConfig, + private Helper $helper, + private Group_Proxy $groupProxy, ) { parent::__construct(); - - $this->subAdmin = $subAdmin; - $this->ocConfig = $ocConfig; - $this->helper = $helper; - $this->groupProxy = $groupProxy; } #[\Override] @@ -147,6 +133,9 @@ function diff_user_arrays($array1, $array2) { foreach ($onlyInNC as $gid => $users) { $groupDN = $access->getGroupMapper()->getDNByName($gid); + if ($groupDN === false) { + throw new Exception('Failed to find group '.$gid); + } foreach ($users as $uid) { $userDN = $access->getUserMapper()->getDNByName($uid); $entry = [ @@ -163,6 +152,9 @@ function diff_user_arrays($array1, $array2) { foreach ($onlyInLDAP as $gid => $users) { $groupDN = $access->getGroupMapper()->getDNByName($gid); + if ($groupDN === false) { + throw new Exception('Failed to find group '.$gid); + } foreach ($users as $uid) { $userDN = $access->getUserMapper()->getDNByName($uid); $entry = [ diff --git a/lib/LDAPConnect.php b/lib/LDAPConnect.php index 3da3cebf..0a194afa 100644 --- a/lib/LDAPConnect.php +++ b/lib/LDAPConnect.php @@ -16,10 +16,8 @@ use Psr\Log\LoggerInterface; class LDAPConnect { - /** @var Configuration */ - private $ldapConfig; - /** @var bool|null */ - private $passwdSupport; + private Configuration $ldapConfig; + private ?bool $passwdSupport; public function __construct( Helper $ldapBackendHelper, @@ -27,15 +25,14 @@ public function __construct( ) { $this->passwdSupport = null; $ldapConfigPrefixes = $ldapBackendHelper->getServerConfigurationPrefixes(true); - $prefix = array_shift($ldapConfigPrefixes); + $prefix = array_shift($ldapConfigPrefixes) ?? ''; $this->ldapConfig = new Configuration($prefix); } /** - * @return resource|Connection * @throws ServerNotAvailableException */ - public function connect() { + public function connect(): Connection { $ldapHost = $this->ldapConfig->ldapHost; $ldapPort = $this->ldapConfig->ldapPort; @@ -51,7 +48,7 @@ public function connect() { // Connecting to LDAP - TODO: connect directly via LDAP plugin $cr = ldap_connect($ldapHost); - if (!is_resource($cr) && !is_object($cr)) { + if (!is_object($cr)) { $this->logger->error('Unable to connect to LDAP host {ldapHost}:{ldapPort}', [ 'app' => Application::APP_ID, @@ -72,10 +69,9 @@ public function connect() { } /** - * @return false|resource|Connection * @throws ServerNotAvailableException */ - public function bind() { + public function bind(): Connection|false { $ds = $this->connect(); $dn = $this->ldapConfig->ldapAgentName; $secret = $this->ldapConfig->ldapAgentPassword; @@ -95,10 +91,9 @@ public function bind() { } /** - * @return false|resource|Connection * @throws ServerNotAvailableException */ - public function getLDAPConnection() { + public function getLDAPConnection(): Connection|false { return $this->bind(); } @@ -142,11 +137,12 @@ public function hasPasswordPolicy(): bool { * checks whether the LDAP server supports the passwd exop * * @param Connection $connection LDAP connection to check - * @return boolean either the user can or cannot + * @return bool either the user can or cannot */ - public function hasPasswdExopSupport($connection): bool { + public function hasPasswdExopSupport(Connection $connection): bool { // TODO: We should cache this by ldap prefix, but currently we have no access to it. if (is_null($this->passwdSupport)) { + /** @var \LDAP\Result|false */ $ret = ldap_read($connection, '', '(objectclass=*)', ['supportedExtension']); if ($ret === false) { $this->passwdSupport = false; diff --git a/lib/LDAPUserManager.php b/lib/LDAPUserManager.php index d81d0047..c7c92319 100644 --- a/lib/LDAPUserManager.php +++ b/lib/LDAPUserManager.php @@ -84,7 +84,7 @@ public function setDisplayName($uid, $displayName) { ); } - if (!is_resource($connection) && !is_object($connection)) { + if (!is_object($connection)) { $this->logger->debug('LDAP resource not available', ['app' => 'ldap_write_support']); throw new ServerNotAvailableException('LDAP server is not available'); } @@ -253,7 +253,7 @@ public function buildNewEntry($username, $password, $base): array { foreach ($lines as $line) { $split = explode(':', $line, 2); $key = trim($split[0]); - $value = trim($split[1]); + $value = trim($split[1] ?? ''); if (!isset($entry[$key])) { $entry[$key] = $value; } elseif (is_array($entry[$key])) { diff --git a/psalm.xml b/psalm.xml index c34a9ec4..a203a6b6 100644 --- a/psalm.xml +++ b/psalm.xml @@ -4,7 +4,7 @@ - SPDX-License-Identifier: AGPL-3.0-or-later --> Date: Wed, 1 Apr 2026 15:19:57 +0200 Subject: [PATCH 2/2] fix: As much strong type as possible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/AppInfo/Application.php | 2 ++ lib/Command/GroupAdminsToLdap.php | 4 ++- lib/LDAPConnect.php | 2 ++ lib/LDAPGroupManager.php | 2 ++ lib/LDAPUserManager.php | 34 +++++++------------ .../GroupBackendRegisteredListener.php | 7 ++-- .../UserBackendRegisteredListener.php | 7 ++-- lib/Service/Configuration.php | 13 ++++--- lib/Settings/Admin.php | 25 +++----------- 9 files changed, 36 insertions(+), 60 deletions(-) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 1a9c427c..0e3152cc 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -1,10 +1,12 @@ @@ -38,7 +40,7 @@ public function __construct( } #[\Override] - protected function configure() { + protected function configure(): void { $this ->setName('ldap-ext:sync-group-admins') ->setDescription('syncs group admin informations to ldap') diff --git a/lib/LDAPConnect.php b/lib/LDAPConnect.php index 0a194afa..30ae9232 100644 --- a/lib/LDAPConnect.php +++ b/lib/LDAPConnect.php @@ -1,5 +1,7 @@ diff --git a/lib/LDAPGroupManager.php b/lib/LDAPGroupManager.php index fcf01056..33f93186 100644 --- a/lib/LDAPGroupManager.php +++ b/lib/LDAPGroupManager.php @@ -1,5 +1,7 @@ diff --git a/lib/LDAPUserManager.php b/lib/LDAPUserManager.php index c7c92319..2536b6bd 100644 --- a/lib/LDAPUserManager.php +++ b/lib/LDAPUserManager.php @@ -1,5 +1,7 @@ @@ -48,7 +50,7 @@ public function __construct( * @return int bitwise-or'ed actions */ #[\Override] - public function respondToActions() { + public function respondToActions(): int { $setPassword = $this->canSetPassword() && !$this->ldapConnect->hasPasswordPolicy() ? Backend::SET_PASSWORD : 0; @@ -63,12 +65,11 @@ public function respondToActions() { * * @param string $uid user ID of the user * @param string $displayName new user's display name - * @return string * @throws HintException * @throws ServerNotAvailableException */ #[\Override] - public function setDisplayName($uid, $displayName) { + public function setDisplayName($uid, $displayName): string { $userDN = $this->getUserDN($uid); $connection = $this->ldapProvider->getLDAPConnection($uid); @@ -106,10 +107,9 @@ public function setDisplayName($uid, $displayName) { * checks whether the user is allowed to change his avatar in Nextcloud * * @param string $uid the Nextcloud user name - * @return bool either the user can or cannot */ #[\Override] - public function canChangeAvatar($uid) { + public function canChangeAvatar($uid): bool { return $this->configuration->hasAvatarPermission(); } @@ -158,11 +158,10 @@ public function changeEmail(IUser $user, string $newEmail): void { * * @param string $uid The username of the user to create * @param string $password The password of the new user - * @return bool|string the created user of false * @throws Exception */ #[\Override] - public function createUser($uid, $password) { + public function createUser($uid, $password): string|false { $adminUser = $this->userSession->getUser(); $requireActorFromLDAP = $this->configuration->isLdapActorRequired(); if ($requireActorFromLDAP && !$adminUser instanceof IUser) { @@ -230,7 +229,7 @@ public function ensureAttribute(array &$ldif, string $attribute, string $fallbac } } - public function buildNewEntry($username, $password, $base): array { + private function buildNewEntry(string $username, string $password, string $base): array { // Make sure the parameters don't fool the following algorithm if (str_contains($username, PHP_EOL)) { throw new Exception('Username contains a new line'); @@ -268,10 +267,6 @@ public function buildNewEntry($username, $password, $base): array { return [$dn, $entry]; } - /** - * @param $uid - * @return bool - */ public function deleteUser($uid): bool { $connection = $this->ldapProvider->getLDAPConnection($uid); $userDN = $this->getUserDN($uid); @@ -318,12 +313,11 @@ public function canSetPassword(): bool { * * @param string $uid The username * @param string $password The new password - * @return bool * * Change the password of a user */ #[\Override] - public function setPassword($uid, $password) { + public function setPassword($uid, $password): bool { $connection = $this->ldapProvider->getLDAPConnection($uid); $userDN = $this->getUserDN($uid); @@ -334,10 +328,9 @@ public function setPassword($uid, $password) { * get the user's home directory * * @param string $uid the username - * @return bool */ #[\Override] - public function getHome($uid) { + public function getHome($uid): bool { // Not implemented return false; } @@ -346,21 +339,18 @@ public function getHome($uid) { * get display name of the user * * @param string $uid user ID of the user - * @return string display name */ #[\Override] - public function getDisplayName($uid) { + public function getDisplayName($uid): string { // Not implemented return $uid; } /** * Count the number of users - * - * @return int|bool */ #[\Override] - public function countUsers() { + public function countUsers(): false { // Not implemented return false; } @@ -398,7 +388,7 @@ public function changeUserHook(IUser $user, string $feature, $attr1, $attr2): vo } } - private function getUserDN($uid): string { + private function getUserDN(string $uid): string { return $this->ldapProvider->getUserDN($uid); } diff --git a/lib/Listener/GroupBackendRegisteredListener.php b/lib/Listener/GroupBackendRegisteredListener.php index 1ab8abbf..10cdc34f 100644 --- a/lib/Listener/GroupBackendRegisteredListener.php +++ b/lib/Listener/GroupBackendRegisteredListener.php @@ -1,6 +1,7 @@ */ class GroupBackendRegisteredListener implements IEventListener { - /** @var IAppManager */ - private $appManager; - public function __construct( - IAppManager $appManager, + private IAppManager $appManager, private LDAPGroupManager $ldapGroupManager, ) { - $this->appManager = $appManager; } /** diff --git a/lib/Listener/UserBackendRegisteredListener.php b/lib/Listener/UserBackendRegisteredListener.php index eb816746..18cb238e 100644 --- a/lib/Listener/UserBackendRegisteredListener.php +++ b/lib/Listener/UserBackendRegisteredListener.php @@ -1,6 +1,7 @@ */ class UserBackendRegisteredListener implements IEventListener { - /** @var IAppManager */ - private $appManager; - public function __construct( - IAppManager $appManager, + private IAppManager $appManager, private LDAPUserManager $ldapUserManager, ) { - $this->appManager = $appManager; } /** diff --git a/lib/Service/Configuration.php b/lib/Service/Configuration.php index 4737607b..b57467f8 100644 --- a/lib/Service/Configuration.php +++ b/lib/Service/Configuration.php @@ -1,6 +1,7 @@ config = $config; + public function __construct( + private IConfig $config, + ) { } public function isLdapActorRequired(): bool { @@ -39,7 +38,7 @@ public function useUnicodePassword(): bool { return $this->config->getAppValue('ldap_write_support', 'useUnicodePassword', '0') === '1'; } - public function getUserTemplate() { + public function getUserTemplate(): string { return $this->config->getAppValue( Application::APP_ID, 'template.user', @@ -47,7 +46,7 @@ public function getUserTemplate() { ); } - public function getUserTemplateDefault() { + public function getUserTemplateDefault(): string { return 'dn: uid={UID},{BASE}' . PHP_EOL . 'objectClass: inetOrgPerson' . PHP_EOL diff --git a/lib/Settings/Admin.php b/lib/Settings/Admin.php index ca06a9de..2225358b 100644 --- a/lib/Settings/Admin.php +++ b/lib/Settings/Admin.php @@ -1,6 +1,7 @@ initialStateService = $initialStateService; } /** @@ -31,7 +28,7 @@ public function __construct( * @since 9.1 */ #[\Override] - public function getForm() { + public function getForm(): TemplateResponse { $this->initialStateService->provideInitialState( 'templates', [ @@ -58,25 +55,13 @@ public function getForm() { return new TemplateResponse(Application::APP_ID, 'settings-admin'); } - /** - * @return string the section ID, e.g. 'sharing' - * @since 9.1 - */ #[\Override] - public function getSection() { + public function getSection(): string { return 'ldap'; } - /** - * @return int whether the form should be rather on the top or bottom of - * the admin section. The forms are arranged in ascending order of the - * priority values. It is required to return a value between 0 and 100. - * - * E.g.: 70 - * @since 9.1 - */ #[\Override] - public function getPriority() { + public function getPriority(): int { return 35; } }