From 9ec0cb0a90ecdc7f4181ce8399bfe0c3a35842a2 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 13 May 2022 12:59:51 +0200 Subject: [PATCH] Fix psalm issues related to the user backend - Reflect the actual return value returned by the implementation in the the interface. E.g. IUser|bool -> IUser|false - Remove $hasLoggedIn parameter from private countUser implementation. Replace the two call with the equivalent countSeenUser - getBackend is nuallable, add this to the interface - Use backend interface to make psalm happy about call to undefined methods. Also helps with getting rid at some point of the old implementActions Signed-off-by: Carl Schwan --- build/psalm-baseline.xml | 61 +------------------ lib/private/DB/QueryBuilder/QueryBuilder.php | 2 +- lib/private/User/Database.php | 2 +- lib/private/User/LazyUser.php | 3 +- lib/private/User/Manager.php | 30 ++++----- lib/private/User/Session.php | 10 +-- lib/private/User/User.php | 31 +++++++--- lib/public/DB/QueryBuilder/IQueryBuilder.php | 2 +- lib/public/IAvatar.php | 2 +- lib/public/IUser.php | 3 +- lib/public/IUserManager.php | 8 +-- .../User/Backend/ICheckPasswordBackend.php | 2 +- tests/lib/User/ManagerTest.php | 4 +- 13 files changed, 59 insertions(+), 101 deletions(-) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index b2953298fa9..21da75d5608 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -2310,11 +2310,6 @@ searchCollections - - - $svg === null - - null @@ -4354,28 +4349,11 @@ false - - $query->func()->lower('displayname') - - - array|int - - - $callback - - - bool|IUser - - - $this->createUserFromBackend($uid, $password, $backend) - $this->createUserFromBackend($uid, $password, $backend) - - - checkPassword - checkPassword - countUsers + + + createUser getUsersForUserValueCaseInsensitive @@ -4387,23 +4365,13 @@ IUser::class . '::firstLogin' - - $this->timeFactory->getTime() - $this->timeFactory->getTime() - $request->server $request->server - - null - dispatch - - getByEmail - @@ -4413,22 +4381,6 @@ IUser::class . '::preDelete' IUser::class . '::preSetPassword' - - getBackend - - - $image - - - IImage|null - - - $quota - $this->lastLogin - - - $this->backend - dispatch dispatch @@ -4436,13 +4388,6 @@ dispatch dispatch - - canChangeAvatar - deleteUserAvatar - getHome - setDisplayName - setPassword - diff --git a/lib/private/DB/QueryBuilder/QueryBuilder.php b/lib/private/DB/QueryBuilder/QueryBuilder.php index fc436383b04..71dd18fd68b 100644 --- a/lib/private/DB/QueryBuilder/QueryBuilder.php +++ b/lib/private/DB/QueryBuilder/QueryBuilder.php @@ -1096,7 +1096,7 @@ class QueryBuilder implements IQueryBuilder { * Specifies an ordering for the query results. * Replaces any previously specified orderings, if any. * - * @param string $sort The ordering expression. + * @param string|IQueryFunction|ILiteral|IParameter $sort The ordering expression. * @param string $order The ordering direction. * * @return $this This QueryBuilder instance. diff --git a/lib/private/User/Database.php b/lib/private/User/Database.php index a9464c27085..4821a2fc632 100644 --- a/lib/private/User/Database.php +++ b/lib/private/User/Database.php @@ -275,7 +275,7 @@ class Database extends ABackend implements ->setMaxResults($limit) ->setFirstResult($offset); - $result = $query->execute(); + $result = $query->executeQuery(); $displayNames = []; while ($row = $result->fetch()) { $displayNames[(string)$row['uid']] = (string)$row['displayname']; diff --git a/lib/private/User/LazyUser.php b/lib/private/User/LazyUser.php index 8b98b112731..8e93d6481ab 100644 --- a/lib/private/User/LazyUser.php +++ b/lib/private/User/LazyUser.php @@ -25,6 +25,7 @@ namespace OC\User; use OCP\IUser; use OCP\IUserManager; +use OCP\UserInterface; class LazyUser implements IUser { private ?IUser $user = null; @@ -83,7 +84,7 @@ class LazyUser implements IUser { return $this->getUser()->getBackendClassName(); } - public function getBackend() { + public function getBackend(): ?UserInterface { return $this->getUser()->getBackend(); } diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index c59cbaa7b20..a6f56585325 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -48,6 +48,8 @@ use OCP\Notification\IManager; use OCP\Support\Subscription\IRegistry; use OCP\User\Backend\IGetRealUIDBackend; use OCP\User\Backend\ISearchKnownUsersBackend; +use OCP\User\Backend\ICheckPasswordBackend; +use OCP\User\Backend\ICountUsersBackend; use OCP\User\Events\BeforeUserCreatedEvent; use OCP\User\Events\UserCreatedEvent; use OCP\UserInterface; @@ -223,7 +225,7 @@ class Manager extends PublicEmitter implements IUserManager { * * @param string $loginName * @param string $password - * @return mixed the User object on success, false otherwise + * @return IUser|false the User object on success, false otherwise */ public function checkPassword($loginName, $password) { $result = $this->checkPasswordNoLogging($loginName, $password); @@ -254,7 +256,8 @@ class Manager extends PublicEmitter implements IUserManager { $backends = $this->backends; } foreach ($backends as $backend) { - if ($backend->implementsActions(Backend::CHECK_PASSWORD)) { + if ($backend instanceof ICheckPasswordBackend || $backend->implementsActions(Backend::CHECK_PASSWORD)) { + /** @var ICheckPasswordBackend $backend */ $uid = $backend->checkPassword($loginName, $password); if ($uid !== false) { return $this->getUserObject($uid, $backend); @@ -268,7 +271,8 @@ class Manager extends PublicEmitter implements IUserManager { $password = urldecode($password); foreach ($backends as $backend) { - if ($backend->implementsActions(Backend::CHECK_PASSWORD)) { + if ($backend instanceof ICheckPasswordBackend || $backend->implementsActions(Backend::CHECK_PASSWORD)) { + /** @var ICheckPasswordBackend|UserInterface $backend */ $uid = $backend->checkPassword($loginName, $password); if ($uid !== false) { return $this->getUserObject($uid, $backend); @@ -376,7 +380,7 @@ class Manager extends PublicEmitter implements IUserManager { * @param string $uid * @param string $password * @throws \InvalidArgumentException - * @return bool|IUser the created user or false + * @return false|IUser the created user or false */ public function createUser($uid, $password) { // DI injection is not used here as IRegistry needs the user manager itself for user count and thus it would create a cyclic dependency @@ -415,7 +419,7 @@ class Manager extends PublicEmitter implements IUserManager { * @param string $uid * @param string $password * @param UserInterface $backend - * @return IUser|null + * @return IUser|false * @throws \InvalidArgumentException */ public function createUserFromBackend($uid, $password, UserInterface $backend) { @@ -469,8 +473,9 @@ class Manager extends PublicEmitter implements IUserManager { /** @deprecated 21.0.0 use UserCreatedEvent event with the IEventDispatcher instead */ $this->emit('\OC\User', 'postCreateUser', [$user, $password]); $this->eventDispatcher->dispatchTyped(new UserCreatedEvent($user, $password)); + return $user; } - return $user; + return false; } /** @@ -478,16 +483,13 @@ class Manager extends PublicEmitter implements IUserManager { * * @param boolean $hasLoggedIn when true only users that have a lastLogin * entry in the preferences table will be affected - * @return array|int an array of backend class as key and count number as value - * if $hasLoggedIn is true only an int is returned + * @return array an array of backend class as key and count number as value */ - public function countUsers($hasLoggedIn = false) { - if ($hasLoggedIn) { - return $this->countSeenUsers(); - } + public function countUsers() { $userCountStatistics = []; foreach ($this->backends as $backend) { - if ($backend->implementsActions(Backend::COUNT_USERS)) { + if ($backend instanceof ICountUsersBackend || $backend->implementsActions(Backend::COUNT_USERS)) { + /** @var ICountUsersBackend|IUserBackend $backend */ $backendUsers = $backend->countUsers(); if ($backendUsers !== false) { if ($backend instanceof IUserBackend) { @@ -528,7 +530,7 @@ class Manager extends PublicEmitter implements IUserManager { * The callback is executed for each user on each backend. * If the callback returns false no further users will be retrieved. * - * @param \Closure $callback + * @psalm-param \Closure(\OCP\IUser):?bool $callback * @param string $search * @param boolean $onlySeen when true only users that have a lastLogin entry * in the preferences table will be affected diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index 365a01c4595..626ddca2dad 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -90,7 +90,7 @@ use Symfony\Component\EventDispatcher\GenericEvent; */ class Session implements IUserSession, Emitter { - /** @var Manager|PublicEmitter $manager */ + /** @var Manager $manager */ private $manager; /** @var ISession $session */ @@ -288,9 +288,9 @@ class Session implements IUserSession, Emitter { } /** - * get the login name of the current user + * Get the login name of the current user * - * @return string + * @return ?string */ public function getLoginName() { if ($this->activeUser) { @@ -870,7 +870,7 @@ class Session implements IUserSession, Emitter { // replace successfully used token with a new one $this->config->deleteUserValue($uid, 'login_token', $currentToken); $newToken = $this->random->generate(32); - $this->config->setUserValue($uid, 'login_token', $newToken, $this->timeFactory->getTime()); + $this->config->setUserValue($uid, 'login_token', $newToken, (string)$this->timeFactory->getTime()); try { $sessionId = $this->session->getId(); @@ -905,7 +905,7 @@ class Session implements IUserSession, Emitter { */ public function createRememberMeToken(IUser $user) { $token = $this->random->generate(32); - $this->config->setUserValue($user->getUID(), 'login_token', $token, $this->timeFactory->getTime()); + $this->config->setUserValue($user->getUID(), 'login_token', $token, (string)$this->timeFactory->getTime()); $this->setMagicInCookie($user->getUID(), $token); } diff --git a/lib/private/User/User.php b/lib/private/User/User.php index e7aa72fafba..de9af35f541 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -52,6 +52,10 @@ use OCP\IUserBackend; use OCP\User\Events\BeforeUserDeletedEvent; use OCP\User\Events\UserDeletedEvent; use OCP\User\GetQuotaEvent; +use OCP\User\Backend\ISetDisplayNameBackend; +use OCP\User\Backend\ISetPasswordBackend; +use OCP\User\Backend\IProvideAvatarBackend; +use OCP\User\Backend\IGetHomeBackend; use OCP\UserInterface; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\EventDispatcher\GenericEvent; @@ -155,7 +159,9 @@ class User implements IUser { $displayName = trim($displayName); $oldDisplayName = $this->getDisplayName(); if ($this->backend->implementsActions(Backend::SET_DISPLAYNAME) && !empty($displayName) && $displayName !== $oldDisplayName) { - $result = $this->backend->setDisplayName($this->uid, $displayName); + /** @var ISetDisplayNameBackend $backend */ + $backend = $this->backend; + $result = $backend->setDisplayName($this->uid, $displayName); if ($result) { $this->displayName = $displayName; $this->triggerChange('displayName', $displayName, $oldDisplayName); @@ -241,7 +247,7 @@ class User implements IUser { $firstTimeLogin = ($this->getLastLogin() === 0); $this->lastLogin = time(); $this->config->setUserValue( - $this->uid, 'login', 'lastLogin', $this->lastLogin); + $this->uid, 'login', 'lastLogin', (string)$this->lastLogin); return $firstTimeLogin; } @@ -280,7 +286,7 @@ class User implements IUser { \OC::$server->getCommentsManager()->deleteReferencesOfActor('users', $this->uid); \OC::$server->getCommentsManager()->deleteReadMarksFromUser($this); - /** @var IAvatarManager $avatarManager */ + /** @var AvatarManager $avatarManager */ $avatarManager = \OC::$server->query(AvatarManager::class); $avatarManager->deleteUserAvatar($this->uid); @@ -319,7 +325,9 @@ class User implements IUser { $this->emitter->emit('\OC\User', 'preSetPassword', [$this, $password, $recoveryPassword]); } if ($this->backend->implementsActions(Backend::SET_PASSWORD)) { - $result = $this->backend->setPassword($this->uid, $password); + /** @var ISetPasswordBackend $backend */ + $backend = $this->backend; + $result = $backend->setPassword($this->uid, $password); if ($result !== false) { $this->legacyDispatcher->dispatch(IUser::class . '::postSetPassword', new GenericEvent($this, [ @@ -344,7 +352,8 @@ class User implements IUser { */ public function getHome() { if (!$this->home) { - if ($this->backend->implementsActions(Backend::GET_HOME) and $home = $this->backend->getHome($this->uid)) { + /** @psalm-suppress UndefinedInterfaceMethod Once we get rid of the legacy implementsActions, psalm won't complain anymore */ + if (($this->backend instanceof IGetHomeBackend || $this->backend->implementsActions(Backend::GET_HOME)) && $home = $this->backend->getHome($this->uid)) { $this->home = $home; } elseif ($this->config) { $this->home = $this->config->getSystemValue('datadirectory', \OC::$SERVERROOT . '/data') . '/' . $this->uid; @@ -367,18 +376,20 @@ class User implements IUser { return get_class($this->backend); } - public function getBackend() { + public function getBackend(): ?UserInterface { return $this->backend; } /** - * check if the backend allows the user to change his avatar on Personal page + * Check if the backend allows the user to change his avatar on Personal page * * @return bool */ public function canChangeAvatar() { - if ($this->backend->implementsActions(Backend::PROVIDE_AVATAR)) { - return $this->backend->canChangeAvatar($this->uid); + if ($this->backend instanceof IProvideAvatarBackend || $this->backend->implementsActions(Backend::PROVIDE_AVATAR)) { + /** @var IProvideAvatarBackend $backend */ + $backend = $this->backend; + return $backend->canChangeAvatar($this->uid); } return true; } @@ -501,7 +512,7 @@ class User implements IUser { $oldQuota = $this->config->getUserValue($this->uid, 'files', 'quota', ''); if ($quota !== 'none' and $quota !== 'default') { $quota = OC_Helper::computerFileSize($quota); - $quota = OC_Helper::humanFileSize($quota); + $quota = OC_Helper::humanFileSize((int)$quota); } if ($quota !== $oldQuota) { $this->config->setUserValue($this->uid, 'files', 'quota', $quota); diff --git a/lib/public/DB/QueryBuilder/IQueryBuilder.php b/lib/public/DB/QueryBuilder/IQueryBuilder.php index e3257e82bca..9b074db563e 100644 --- a/lib/public/DB/QueryBuilder/IQueryBuilder.php +++ b/lib/public/DB/QueryBuilder/IQueryBuilder.php @@ -820,7 +820,7 @@ interface IQueryBuilder { * Specifies an ordering for the query results. * Replaces any previously specified orderings, if any. * - * @param string $sort The ordering expression. + * @param string|IQueryFunction $sort The ordering expression. * @param string $order The ordering direction. * * @return $this This QueryBuilder instance. diff --git a/lib/public/IAvatar.php b/lib/public/IAvatar.php index 8a1bc792450..218ef258250 100644 --- a/lib/public/IAvatar.php +++ b/lib/public/IAvatar.php @@ -38,7 +38,7 @@ interface IAvatar { /** * get the users avatar * @param int $size size in px of the avatar, avatars are square, defaults to 64, -1 can be used to not scale the image - * @return boolean|\OCP\IImage containing the avatar or false if there's no image + * @return false|\OCP\IImage containing the avatar or false if there's no image * @since 6.0.0 - size of -1 was added in 9.0.0 */ public function get($size = 64); diff --git a/lib/public/IUser.php b/lib/public/IUser.php index 1a1d1e44d8a..ce9cf0096ec 100644 --- a/lib/public/IUser.php +++ b/lib/public/IUser.php @@ -113,10 +113,9 @@ interface IUser { /** * Get the backend for the current user object * - * @return UserInterface * @since 15.0.0 */ - public function getBackend(); + public function getBackend(): ?UserInterface; /** * check if the backend allows the user to change his avatar on Personal page diff --git a/lib/public/IUserManager.php b/lib/public/IUserManager.php index e5c220af40c..77e2fc21a22 100644 --- a/lib/public/IUserManager.php +++ b/lib/public/IUserManager.php @@ -98,7 +98,7 @@ interface IUserManager { * * @param string $loginName * @param string $password - * @return mixed the User object on success, false otherwise + * @return IUser|false the User object on success, false otherwise * @since 8.0.0 */ public function checkPassword($loginName, $password); @@ -141,7 +141,7 @@ interface IUserManager { * @param string $uid * @param string $password * @throws \InvalidArgumentException - * @return bool|\OCP\IUser the created user or false + * @return false|\OCP\IUser the created user or false * @since 8.0.0 */ public function createUser($uid, $password); @@ -157,9 +157,9 @@ interface IUserManager { public function createUserFromBackend($uid, $password, UserInterface $backend); /** - * returns how many users per backend exist (if supported by backend) + * Get how many users per backend exist (if supported by backend) * - * @return array an array of backend class as key and count number as value + * @return array an array of backend class name as key and count number as value * @since 8.0.0 */ public function countUsers(); diff --git a/lib/public/User/Backend/ICheckPasswordBackend.php b/lib/public/User/Backend/ICheckPasswordBackend.php index b3eaf7cedb0..0d4026be859 100644 --- a/lib/public/User/Backend/ICheckPasswordBackend.php +++ b/lib/public/User/Backend/ICheckPasswordBackend.php @@ -35,7 +35,7 @@ interface ICheckPasswordBackend { * * @param string $loginName The loginname * @param string $password The password - * @return string|bool The uid on success false on failure + * @return string|false The uid on success false on failure */ public function checkPassword(string $loginName, string $password); } diff --git a/tests/lib/User/ManagerTest.php b/tests/lib/User/ManagerTest.php index c8c1430d583..2536eee8441 100644 --- a/tests/lib/User/ManagerTest.php +++ b/tests/lib/User/ManagerTest.php @@ -609,7 +609,7 @@ class ManagerTest extends TestCase { public function testCountUsersOnlySeen() { $manager = \OC::$server->getUserManager(); // count other users in the db before adding our own - $countBefore = $manager->countUsers(true); + $countBefore = $manager->countSeenUsers(); //Add test users $user1 = $manager->createUser('testseencount1', 'testseencount1'); @@ -623,7 +623,7 @@ class ManagerTest extends TestCase { $user4 = $manager->createUser('testseencount4', 'testseencount4'); $user4->updateLastLoginTimestamp(); - $this->assertEquals($countBefore + 3, $manager->countUsers(true)); + $this->assertEquals($countBefore + 3, $manager->countSeenUsers()); //cleanup $user1->delete(); -- 2.39.5