From 62f7cdbf963d71282e1ecb39deef4c144c09ab8a Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Tue, 10 Feb 2026 13:24:35 +0100 Subject: [PATCH] refactor: Strict types and property promotions Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> Signed-off-by: Carl Schwan --- lib/AppConfigOverwrite.php | 14 ++---- lib/AppWhitelist.php | 12 +++-- lib/BackgroundJob/TransferJob.php | 9 ++-- lib/Capabilities.php | 2 + lib/Command/AddCommand.php | 1 + lib/Config.php | 28 ++++------- lib/Controller/APIController.php | 7 +-- lib/Controller/SettingsController.php | 12 +++-- lib/Controller/UsersController.php | 1 + lib/Db/Transfer.php | 3 ++ lib/FilteredNavigationManager.php | 2 + lib/FilteredSettingsManager.php | 2 + lib/GroupBackend.php | 15 +++--- lib/GuestManager.php | 7 ++- lib/Hooks.php | 7 ++- .../BeforeTemplateRenderedListener.php | 6 +-- .../BeforeUserManagementRenderedListener.php | 2 +- .../LoadAdditionalScriptsListener.php | 5 +- lib/Listener/ShareAutoAcceptListener.php | 6 +-- lib/Listener/UserChangedListener.php | 10 ++-- lib/Mail.php | 3 +- lib/Repair/ResetEmails.php | 1 + lib/RestrictionManager.php | 2 + lib/Settings/Admin.php | 15 +++--- lib/Settings/Section.php | 31 +++++-------- lib/Storage/DirMask.php | 46 ++++++++++--------- lib/Storage/DirMaskCache.php | 2 + lib/Storage/ReadOnlyJail.php | 2 + lib/TransferService.php | 1 + lib/UserBackend.php | 45 +++++------------- 30 files changed, 137 insertions(+), 162 deletions(-) diff --git a/lib/AppConfigOverwrite.php b/lib/AppConfigOverwrite.php index 0f59417d..2690edb4 100644 --- a/lib/AppConfigOverwrite.php +++ b/lib/AppConfigOverwrite.php @@ -1,6 +1,7 @@ overWrite = $overwrite; } - - /** - * @param $app - * @param $key - * @param $default - * @return string - */ - public function getValue($app, $key, $default = '') { + public function getValue($app, $key, $default = ''): string { if (isset($this->overWrite[$app]) && isset($this->overWrite[$app][$key])) { return $this->overWrite[$app][$key]; } diff --git a/lib/AppWhitelist.php b/lib/AppWhitelist.php index 0d7cee42..1ee5ee80 100644 --- a/lib/AppWhitelist.php +++ b/lib/AppWhitelist.php @@ -1,5 +1,7 @@ logger->notice("Blocking access to non-whitelisted app ($app) for guest", ['app' => 'guests']); return false; } - } else { - return true; } + + return true; } public function verifyAccess(IUser $user, IRequest $request): void { @@ -130,6 +129,9 @@ private function getRequestedApp(string|false $url): string { return 'core'; } + /** + * @return list + */ public function getWhitelistAbleApps(): array { return array_values(array_diff( $this->appManager->getInstalledApps(), diff --git a/lib/BackgroundJob/TransferJob.php b/lib/BackgroundJob/TransferJob.php index cb336c22..8f6706cb 100644 --- a/lib/BackgroundJob/TransferJob.php +++ b/lib/BackgroundJob/TransferJob.php @@ -48,6 +48,7 @@ private function notifyFailure(Transfer $transfer): void { 'source' => $transfer->getSource(), 'target' => $transfer->getTarget(), ]); + $this->notificationManager->notify($notification); } @@ -63,13 +64,14 @@ private function notifySuccess(Transfer $transfer): void { 'source' => $transfer->getSource(), 'target' => $transfer->getTarget(), ]); + $this->notificationManager->notify($notification); } private function fail(Transfer $transfer, ?IUser $targetUser = null): void { $this->notifyFailure($transfer); $this->transferMapper->delete($transfer); - if (!($targetUser instanceof IUser)) { + if (!$targetUser instanceof IUser) { return; } $result = $targetUser->delete(); // Rollback created user @@ -90,7 +92,7 @@ public function run($argument): void { $target = $transfer->getTarget(); $sourceUser = $this->userManager->get($source); - if (!($sourceUser instanceof IUser)) { + if (!$sourceUser instanceof IUser) { $this->logger->error('Failed to transfer missing guest user: ' . $source); $this->fail($transfer); return; @@ -107,7 +109,7 @@ public function run($argument): void { $this->secureRandom->generate(20), // Password hash will be copied to target user from source user ); - if (!($targetUser instanceof IUser)) { + if (!$targetUser instanceof IUser) { $this->logger->error('Failed to create new user: ' . $target); $this->fail($transfer); return; @@ -141,6 +143,7 @@ public function run($argument): void { if (!$result) { $this->logger->error('Failed to delete guest user', ['user' => $sourceUser->getUID()]); } + $this->notifySuccess($transfer); $this->transferMapper->delete($transfer); } diff --git a/lib/Capabilities.php b/lib/Capabilities.php index 1e562620..43616767 100644 --- a/lib/Capabilities.php +++ b/lib/Capabilities.php @@ -1,5 +1,7 @@ appConfig->getAppValueBool('allow_external_storage', false); } - /** - * @param string|bool $allow - */ - public function setAllowExternalStorage($allow): void { + public function setAllowExternalStorage(string|bool $allow): void { $this->appConfig->setAppValueBool('allow_external_storage', $allow === true || $allow === 'true') ; } @@ -39,10 +37,7 @@ public function hideOtherUsers(): bool { return $this->appConfig->getAppValueBool('hide_users', true); } - /** - * @param string|bool $hide - */ - public function setHideOtherUsers($hide): void { + public function setHideOtherUsers(string|bool $hide): void { $this->appConfig->setAppValueBool('hide_users', $hide === true || $hide === 'true') ; } @@ -54,25 +49,19 @@ public function useWhitelist(): bool { return $this->appConfig->getAppValueBool('usewhitelist', true); } - /** - * @param string|bool $use - */ - public function setUseWhitelist($use): void { + public function setUseWhitelist(string|bool $use): void { $this->appConfig->setAppValueBool('usewhitelist', $use === true || $use === 'true') ; } /** - * @return string[] + * @return list */ public function getAppWhitelist(): array { $whitelist = $this->appConfig->getAppValueString('whitelist', AppWhitelist::DEFAULT_WHITELIST); return explode(',', $whitelist); } - /** - * @param array|string $whitelist - */ - public function setAppWhitelist($whitelist): void { + public function setAppWhitelist(array|string $whitelist): void { if (is_array($whitelist)) { $whitelist = implode(',', $whitelist); } @@ -106,12 +95,11 @@ public function canCreateGuests(): bool { } } - return !$this->isSharingRestrictedToGroup(); } /** - * @return string[] + * @return list */ public function getCreateRestrictedToGroup(): array { $groups = $this->appConfig->getAppValueArray('create_restricted_to_group', []); @@ -127,7 +115,7 @@ public function getCreateRestrictedToGroup(): array { } /** - * @param string[] $groups + * @param list $groups */ public function setCreateRestrictedToGroup(array $groups): void { $this->appConfig->setAppValueArray('create_restricted_to_group', $groups); diff --git a/lib/Controller/APIController.php b/lib/Controller/APIController.php index 1641f493..84ab60d0 100644 --- a/lib/Controller/APIController.php +++ b/lib/Controller/APIController.php @@ -1,6 +1,7 @@ l10nFactory->get('lib', $lang); // TRANSLATORS this is the language name for the language switcher in the personal settings and should be the localized version $potentialName = $l->t('__language_name__'); - if ($l->getLanguageCode() === $lang && $potentialName[0] !== '_') {//first check if the language name is in the translation file + if ($l->getLanguageCode() === $lang && $potentialName[0] !== '_') { // first check if the language name is in the translation file $ln = [ 'code' => $lang, 'name' => $potentialName, @@ -54,14 +55,14 @@ public function languages(): DataResponse { 'code' => $lang, 'name' => 'English (US)', ]; - } else {//fallback to language code + } else { // fallback to language code $ln = [ 'code' => $lang, 'name' => $lang, ]; } - // put appropriate languages into appropriate arrays, to print them sorted + // Put appropriate languages into appropriate arrays, to print them sorted // common languages -> divider -> other languages if (in_array($lang, Factory::COMMON_LANGUAGE_CODES)) { $commonLanguages[array_search($lang, Factory::COMMON_LANGUAGE_CODES)] = $ln; diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 92132aae..626e0806 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -1,5 +1,7 @@ config->allowExternalStorage(); $hideUsers = $this->config->hideOtherUsers(); $whitelist = $this->config->getAppWhitelist(); + return new DataResponse([ 'useWhitelist' => $useWhitelist, 'whitelist' => $whitelist, @@ -53,21 +56,20 @@ public function getConfig(): DataResponse { } /** - * @param $useWhitelist bool - * @param $whitelist string[] - * @param $allowExternalStorage bool - * @param $hideUsers bool + * @param list $whitelist */ public function setConfig(bool $useWhitelist, array $whitelist, bool $allowExternalStorage, bool $hideUsers, array $createRestrictedToGroup): DataResponse { $newWhitelist = []; foreach ($whitelist as $app) { $newWhitelist[] = trim((string)$app); } + $this->config->setUseWhitelist($useWhitelist); $this->config->setAppWhitelist($newWhitelist); $this->config->setAllowExternalStorage($allowExternalStorage); $this->config->setHideOtherUsers($hideUsers); $this->config->setCreateRestrictedToGroup($createRestrictedToGroup); + return new DataResponse(); } @@ -81,6 +83,7 @@ public function setConfig(bool $useWhitelist, array $whitelist, bool $allowExter public function getWhitelist(): DataResponse { $useWhitelist = $this->config->useWhitelist(); $whitelist = $this->config->getAppWhitelist(); + return new DataResponse([ 'useWhitelist' => $useWhitelist, 'whitelist' => $whitelist, @@ -94,6 +97,7 @@ public function getWhitelist(): DataResponse { */ public function resetWhitelist(): DataResponse { $this->config->setAppWhitelist(AppWhitelist::DEFAULT_WHITELIST); + return new DataResponse([ 'whitelist' => explode(',', AppWhitelist::DEFAULT_WHITELIST), ]); diff --git a/lib/Controller/UsersController.php b/lib/Controller/UsersController.php index 660dd16d..f91ceeb0 100644 --- a/lib/Controller/UsersController.php +++ b/lib/Controller/UsersController.php @@ -226,6 +226,7 @@ public function transfer(string $guestUserId, string $targetUserId): DataRespons } $this->transferService->addTransferJob($author, $sourceUser, $targetUserId); + return new DataResponse([], Http::STATUS_CREATED); } } diff --git a/lib/Db/Transfer.php b/lib/Db/Transfer.php index 840e29ec..a034549a 100644 --- a/lib/Db/Transfer.php +++ b/lib/Db/Transfer.php @@ -30,10 +30,13 @@ class Transfer extends Entity { /** @var string */ protected $author; + /** @var string */ protected $source; + /** @var string */ protected $target; + /** @var string */ protected $status; diff --git a/lib/FilteredNavigationManager.php b/lib/FilteredNavigationManager.php index 1b3aedd2..d04f1c72 100644 --- a/lib/FilteredNavigationManager.php +++ b/lib/FilteredNavigationManager.php @@ -1,5 +1,7 @@ groupName) { if ($this->guestManager->isGuest() && $this->config->hideOtherUsers()) { return [$this->userSession->getUser()->getUID()]; - } else { - return $offset === 0 ? $this->getMembers() : []; } + return $offset === 0 ? $this->getMembers() : []; } return []; @@ -121,20 +121,17 @@ public function countUsersInGroup(string $gid, string $search = ''): int { if ($gid === $this->groupName) { if ($this->guestManager->isGuest() && $this->config->hideOtherUsers()) { return 1; - } else { - return count($this->getMembers()); } - } else { - return 0; + return count($this->getMembers()); } + return 0; } public function getGroupDetails(string $gid): array { if ($gid === $this->groupName) { return ['displayName' => 'Guests']; - } else { - return []; } + return []; } public function hideGroup(string $groupId): bool { diff --git a/lib/GuestManager.php b/lib/GuestManager.php index e0b1fadb..b5ce2fc1 100644 --- a/lib/GuestManager.php +++ b/lib/GuestManager.php @@ -1,5 +1,7 @@ userSession->getUser(); return ($user !== null) && $this->userBackend->userExists($user->getUID()); diff --git a/lib/Hooks.php b/lib/Hooks.php index a8f7888d..69c3fd5b 100644 --- a/lib/Hooks.php +++ b/lib/Hooks.php @@ -1,5 +1,7 @@ Application::APP_ID] ); - return; } @@ -55,7 +56,6 @@ public function handlePostShare(ShareCreatedEvent $event): void { 'ignoring share for itemType ' . $share->getNodeType(), ['app' => Application::APP_ID] ); - return; } @@ -89,9 +89,8 @@ public function setupReadonlyFilesystem(array $params): void { 'mask' => Constants::PERMISSION_READ, 'path' => 'files' ]); - } else { - return $storage; } + return $storage; }); } } diff --git a/lib/Listener/BeforeTemplateRenderedListener.php b/lib/Listener/BeforeTemplateRenderedListener.php index 8821e55f..46d63f28 100644 --- a/lib/Listener/BeforeTemplateRenderedListener.php +++ b/lib/Listener/BeforeTemplateRenderedListener.php @@ -7,7 +7,6 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ - namespace OCA\Guests\Listener; use OCA\Guests\Config; @@ -44,10 +43,7 @@ public function handle(Event $event): void { return; } - // FIXME use this once Nextcloud 25 is the supported minimum - // if ($event->getResponse()->getApp() !== 'spreed') { - $params = $event->getResponse()->getParams(); - if (!isset($params['app']) || $params['app'] !== 'spreed') { + if ($event->getResponse()->getApp() !== 'spreed') { return; } diff --git a/lib/Listener/BeforeUserManagementRenderedListener.php b/lib/Listener/BeforeUserManagementRenderedListener.php index 28eb6bb9..732e0514 100644 --- a/lib/Listener/BeforeUserManagementRenderedListener.php +++ b/lib/Listener/BeforeUserManagementRenderedListener.php @@ -19,7 +19,7 @@ */ class BeforeUserManagementRenderedListener implements IEventListener { public function handle(Event $event): void { - if (!($event instanceof BeforeTemplateRenderedEvent)) { + if (!$event instanceof BeforeTemplateRenderedEvent) { return; } diff --git a/lib/Listener/LoadAdditionalScriptsListener.php b/lib/Listener/LoadAdditionalScriptsListener.php index 3efd452b..a0b60a1e 100644 --- a/lib/Listener/LoadAdditionalScriptsListener.php +++ b/lib/Listener/LoadAdditionalScriptsListener.php @@ -1,13 +1,12 @@ getShare(); - // Right now we only handle direct shares to this user if ($share->getShareType() !== IShare::TYPE_USER) { $this->logger->debug('Not handling share since it is not a user share'); @@ -48,7 +48,7 @@ public function handle(Event $event): void { return; } - if (!($user->getBackend() instanceof UserBackend)) { + if (!$user->getBackend() instanceof UserBackend) { $this->logger->debug('Not a guest user'); return; } diff --git a/lib/Listener/UserChangedListener.php b/lib/Listener/UserChangedListener.php index d8cc9d3c..20ea8038 100644 --- a/lib/Listener/UserChangedListener.php +++ b/lib/Listener/UserChangedListener.php @@ -1,6 +1,7 @@ * SPDX-License-Identifier: AGPL-3.0-or-later @@ -35,21 +36,24 @@ public function handle(Event $event): void { if (!$event instanceof UserChangedEvent) { return; } + if ($event->getFeature() !== 'eMailAddress') { return; } + $user = $event->getUser(); if (!$this->guestManager->isGuest($user)) { return; } - $guestEmail = $this->config->getUserValue($user->getUID(), Application::APP_ID, 'email', strtolower((string)$user->getUID())); + $userId = (string)$user->getUID(); + $guestEmail = $this->config->getUserValue($userId, Application::APP_ID, 'email', strtolower($userId)); if ($event->getValue() === $guestEmail) { return; } $allowChange = false; - if (strtolower($event->getValue()) === strtolower((string)$user->getUID())) { + if (strtolower($event->getValue()) === strtolower($userId)) { $allowChange = true; } elseif ($this->userSession->getUser() !== $user) { $allowChange = true; @@ -58,7 +62,7 @@ public function handle(Event $event): void { } if ($allowChange) { - $this->config->setUserValue($user->getUID(), Application::APP_ID, 'email', $event->getValue()); + $this->config->setUserValue($userId, Application::APP_ID, 'email', $event->getValue()); } else { $user->setSystemEMailAddress($guestEmail); $event->stopPropagation(); diff --git a/lib/Mail.php b/lib/Mail.php index 7f12949e..e9335e42 100644 --- a/lib/Mail.php +++ b/lib/Mail.php @@ -1,5 +1,7 @@ * SPDX-License-Identifier: AGPL-3.0-or-later diff --git a/lib/RestrictionManager.php b/lib/RestrictionManager.php index e3f68901..c6732372 100644 --- a/lib/RestrictionManager.php +++ b/lib/RestrictionManager.php @@ -1,5 +1,7 @@ url = $url; - $this->l = $l; + public function __construct( + private readonly IURLGenerator $url, + private readonly IL10N $l, + ) { } - /** - * {@inheritdoc} - */ + #[Override] public function getID(): string { return 'guests'; } - /** - * {@inheritdoc} - */ + #[Override] public function getName(): string { return $this->l->t('Guests'); } - /** - * {@inheritdoc} - */ + #[Override] public function getPriority(): int { return 30; } - /** - * {@inheritdoc} - */ + #[Override] public function getIcon(): string { return $this->url->imagePath('guests', 'app-dark.svg'); } diff --git a/lib/Storage/DirMask.php b/lib/Storage/DirMask.php index 9b02bed6..a6bd3c9a 100644 --- a/lib/Storage/DirMask.php +++ b/lib/Storage/DirMask.php @@ -1,5 +1,7 @@ checkPath($path)) { return parent::isUpdatable($path); - } else { - return $this->storage->isUpdatable($path); } + + return $this->storage->isUpdatable($path); } public function isCreatable($path): bool { if ($this->checkPath($path)) { return parent::isCreatable($path); - } else { - return $this->storage->isCreatable($path); } + + return $this->storage->isCreatable($path); } public function isDeletable($path): bool { if ($this->checkPath($path)) { return parent::isDeletable($path); - } else { - return $this->storage->isDeletable($path); } + + return $this->storage->isDeletable($path); } public function isSharable($path): bool { if ($this->checkPath($path)) { return parent::isSharable($path); - } else { - return $this->storage->isSharable($path); } + + return $this->storage->isSharable($path); } public function getPermissions($path): int { if ($this->checkPath($path)) { return parent::getPermissions($path); - } else { - return $this->storage->getPermissions($path); } + + return $this->storage->getPermissions($path); } public function rename($source, $target): bool { @@ -128,50 +130,50 @@ public function copy($source, $target): bool { public function touch($path, $mtime = null): bool { if ($this->checkPath($path)) { return parent::touch($path); - } else { - return $this->storage->touch($path); } + + return $this->storage->touch($path); } public function mkdir($path): bool { // Always allow creating the path of the dir mask. if ($path !== $this->path && $this->checkPath($path)) { return parent::mkdir($path); - } else { - return $this->storage->mkdir($path); } + + return $this->storage->mkdir($path); } public function rmdir($path): bool { if ($this->checkPath($path)) { return parent::rmdir($path); - } else { - return $this->storage->rmdir($path); } + + return $this->storage->rmdir($path); } public function unlink($path): bool { if ($this->checkPath($path)) { return parent::unlink($path); - } else { - return $this->storage->unlink($path); } + + return $this->storage->unlink($path); } public function file_put_contents($path, $data): int|float|false { if ($this->checkPath($path)) { return parent::file_put_contents($path, $data); - } else { - return $this->storage->file_put_contents($path, $data); } + + return $this->storage->file_put_contents($path, $data); } public function fopen($path, $mode) { if ($this->checkPath($path)) { return parent::fopen($path, $mode); - } else { - return $this->storage->fopen($path, $mode); } + + return $this->storage->fopen($path, $mode); } public function getCache($path = '', $storage = null): ICache { diff --git a/lib/Storage/DirMaskCache.php b/lib/Storage/DirMaskCache.php index f65db450..532aafd2 100644 --- a/lib/Storage/DirMaskCache.php +++ b/lib/Storage/DirMaskCache.php @@ -1,6 +1,7 @@ setObject('user', $user->getEMailAddress()) ->setDateTime(new \DateTime()) ->setUser($user->getUID()); + $this->notificationManager->notify($notification); } diff --git a/lib/UserBackend.php b/lib/UserBackend.php index 17c9d10c..4a34f56c 100644 --- a/lib/UserBackend.php +++ b/lib/UserBackend.php @@ -1,6 +1,7 @@ loadUser($uid); @@ -248,12 +229,10 @@ public function getDisplayNames($search = '', $limit = null, $offset = null): ar } /** - * Check if the password is correct - * - * @return string|false - * * Check if the password is correct without logging in the user * returns the user id or false + * + * @return string|false */ public function checkPassword(string $loginName, string $password) { if (!str_contains($loginName, '@')) { @@ -290,7 +269,6 @@ public function checkPassword(string $loginName, string $password) { * Load an user in the cache * * @param string $uid the username - * @return bool true if user was found, false otherwise */ private function loadUser($uid): bool { // guests $uid could be NULL or '' @@ -344,7 +322,7 @@ public function getUsers($search = '', $limit = null, $offset = null): array { } /** - * check if a user exists + * Check if a user exists * * @param string $uid the username */ @@ -354,9 +332,8 @@ public function userExists($uid): bool { } /** - * get the user's home directory + * Get the user's home directory * - * @param string $uid the username * @return string|false */ public function getHome(string $uid) { @@ -372,7 +349,7 @@ public function hasUserListings(): bool { } /** - * counts the users in the database + * Counts the users in the database * * @return int|false */ @@ -386,7 +363,7 @@ public function countUsers() { } /** - * returns the username for the given login name in the correct casing + * Returns the username for the given login name in the correct casing * * @param string $loginName * @return string|false