From fc21ecbd2afb6c0097bcd9a3ce896912e1229ac6 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 15 Aug 2023 18:21:04 +0200 Subject: [PATCH] cleanup di for share permissions wrapper Signed-off-by: Robin Appelman --- apps/files_sharing/tests/CapabilitiesTest.php | 4 +- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Files/SetupManager.php | 76 +++++++------------ lib/private/Files/SetupManagerFactory.php | 45 ++++------- lib/private/Server.php | 4 +- lib/private/Share20/Manager.php | 41 ++-------- lib/private/Share20/ShareDisableChecker.php | 65 ++++++++++++++++ tests/lib/Files/ViewTest.php | 5 +- tests/lib/Share20/ManagerTest.php | 35 ++++++--- 10 files changed, 148 insertions(+), 129 deletions(-) create mode 100644 lib/private/Share20/ShareDisableChecker.php diff --git a/apps/files_sharing/tests/CapabilitiesTest.php b/apps/files_sharing/tests/CapabilitiesTest.php index fcefc556203..294bbba7d90 100644 --- a/apps/files_sharing/tests/CapabilitiesTest.php +++ b/apps/files_sharing/tests/CapabilitiesTest.php @@ -30,6 +30,7 @@ namespace OCA\Files_Sharing\Tests; use OC\KnownUser\KnownUserService; use OC\Share20\Manager; +use OC\Share20\ShareDisableChecker; use OCA\Files_Sharing\Capabilities; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\IRootFolder; @@ -96,7 +97,8 @@ class CapabilitiesTest extends \Test\TestCase { $this->createMock(\OC_Defaults::class), $this->createMock(IEventDispatcher::class), $this->createMock(IUserSession::class), - $this->createMock(KnownUserService::class) + $this->createMock(KnownUserService::class), + $this->createMock(ShareDisableChecker::class) ); $cap = new Capabilities($config, $shareManager); $result = $this->getFilesSharingPart($cap->getCapabilities()); diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 5b543ad55b9..51778181b34 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1630,6 +1630,7 @@ return array( 'OC\\Share20\\PublicShareTemplateFactory' => $baseDir . '/lib/private/Share20/PublicShareTemplateFactory.php', 'OC\\Share20\\Share' => $baseDir . '/lib/private/Share20/Share.php', 'OC\\Share20\\ShareAttributes' => $baseDir . '/lib/private/Share20/ShareAttributes.php', + 'OC\\Share20\\ShareDisableChecker' => $baseDir . '/lib/private/Share20/ShareDisableChecker.php', 'OC\\Share20\\ShareHelper' => $baseDir . '/lib/private/Share20/ShareHelper.php', 'OC\\Share20\\UserRemovedListener' => $baseDir . '/lib/private/Share20/UserRemovedListener.php', 'OC\\Share\\Constants' => $baseDir . '/lib/private/Share/Constants.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 19e2f3393d6..982249badb3 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1663,6 +1663,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Share20\\PublicShareTemplateFactory' => __DIR__ . '/../../..' . '/lib/private/Share20/PublicShareTemplateFactory.php', 'OC\\Share20\\Share' => __DIR__ . '/../../..' . '/lib/private/Share20/Share.php', 'OC\\Share20\\ShareAttributes' => __DIR__ . '/../../..' . '/lib/private/Share20/ShareAttributes.php', + 'OC\\Share20\\ShareDisableChecker' => __DIR__ . '/../../..' . '/lib/private/Share20/ShareDisableChecker.php', 'OC\\Share20\\ShareHelper' => __DIR__ . '/../../..' . '/lib/private/Share20/ShareHelper.php', 'OC\\Share20\\UserRemovedListener' => __DIR__ . '/../../..' . '/lib/private/Share20/UserRemovedListener.php', 'OC\\Share\\Constants' => __DIR__ . '/../../..' . '/lib/private/Share/Constants.php', diff --git a/lib/private/Files/SetupManager.php b/lib/private/Files/SetupManager.php index 47c58dd18da..dfc496524a8 100644 --- a/lib/private/Files/SetupManager.php +++ b/lib/private/Files/SetupManager.php @@ -35,6 +35,7 @@ use OC\Files\Storage\Wrapper\PermissionsMask; use OC\Files\Storage\Wrapper\Quota; use OC\Lockdown\Filesystem\NullStorage; use OC\Share\Share; +use OC\Share20\ShareDisableChecker; use OC_App; use OC_Hook; use OC_Util; @@ -62,57 +63,37 @@ use OCP\IUserManager; use OCP\IUserSession; use OCP\Lockdown\ILockdownManager; use OCP\Share\Events\ShareCreatedEvent; -use OCP\Share\IManager; use Psr\Log\LoggerInterface; class SetupManager { private bool $rootSetup = false; - private IEventLogger $eventLogger; - private MountProviderCollection $mountProviderCollection; - private IMountManager $mountManager; - private IUserManager $userManager; // List of users for which at least one mount is setup private array $setupUsers = []; // List of users for which all mounts are setup private array $setupUsersComplete = []; /** @var array */ private array $setupUserMountProviders = []; - private IEventDispatcher $eventDispatcher; - private IUserMountCache $userMountCache; - private ILockdownManager $lockdownManager; - private IUserSession $userSession; private ICache $cache; - private LoggerInterface $logger; - private IConfig $config; private bool $listeningForProviders; private array $fullSetupRequired = []; private bool $setupBuiltinWrappersDone = false; public function __construct( - IEventLogger $eventLogger, - MountProviderCollection $mountProviderCollection, - IMountManager $mountManager, - IUserManager $userManager, - IEventDispatcher $eventDispatcher, - IUserMountCache $userMountCache, - ILockdownManager $lockdownManager, - IUserSession $userSession, + private IEventLogger $eventLogger, + private MountProviderCollection $mountProviderCollection, + private IMountManager $mountManager, + private IUserManager $userManager, + private IEventDispatcher $eventDispatcher, + private IUserMountCache $userMountCache, + private ILockdownManager $lockdownManager, + private IUserSession $userSession, ICacheFactory $cacheFactory, - LoggerInterface $logger, - IConfig $config + private LoggerInterface $logger, + private IConfig $config, + private ShareDisableChecker $shareDisableChecker, ) { - $this->eventLogger = $eventLogger; - $this->mountProviderCollection = $mountProviderCollection; - $this->mountManager = $mountManager; - $this->userManager = $userManager; - $this->eventDispatcher = $eventDispatcher; - $this->userMountCache = $userMountCache; - $this->lockdownManager = $lockdownManager; - $this->logger = $logger; - $this->userSession = $userSession; $this->cache = $cacheFactory->createDistributed('setupmanager::'); $this->listeningForProviders = false; - $this->config = $config; $this->setupListeners(); } @@ -142,24 +123,23 @@ class SetupManager { return $storage; }); - Filesystem::addStorageWrapper('sharing_mask', function ($mountPoint, IStorage $storage, IMountPoint $mount) { - $reSharingEnabled = Share::isResharingAllowed(); - $sharingEnabledForMount = $mount->getOption('enable_sharing', true); - /** @var IUserSession $userSession */ - $userSession = \OC::$server->get(IUserSession::class); - $user = $userSession->getUser(); - /** @var IManager $shareManager */ - $shareManager = \OC::$server->get(IManager::class); - $sharingEnabledForUser = $user ? !$shareManager->sharingDisabledForUser($user->getUID()) : true; - $isShared = $storage->instanceOfStorage(ISharedStorage::class); - if (!$sharingEnabledForMount || !$sharingEnabledForUser || (!$reSharingEnabled && $isShared)) { - return new PermissionsMask([ - 'storage' => $storage, - 'mask' => Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE, - ]); + $reSharingEnabled = Share::isResharingAllowed(); + $user = $this->userSession->getUser(); + $sharingEnabledForUser = $user ? !$this->shareDisableChecker->sharingDisabledForUser($user->getUID()) : true; + Filesystem::addStorageWrapper( + 'sharing_mask', + function ($mountPoint, IStorage $storage, IMountPoint $mount) use ($reSharingEnabled, $sharingEnabledForUser) { + $sharingEnabledForMount = $mount->getOption('enable_sharing', true); + $isShared = $storage->instanceOfStorage(ISharedStorage::class); + if (!$sharingEnabledForMount || !$sharingEnabledForUser || (!$reSharingEnabled && $isShared)) { + return new PermissionsMask([ + 'storage' => $storage, + 'mask' => Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE, + ]); + } + return $storage; } - return $storage; - }); + ); // install storage availability wrapper, before most other wrappers Filesystem::addStorageWrapper('oc_availability', function ($mountPoint, IStorage $storage) { diff --git a/lib/private/Files/SetupManagerFactory.php b/lib/private/Files/SetupManagerFactory.php index 1d9efbd411f..8589cbdea42 100644 --- a/lib/private/Files/SetupManagerFactory.php +++ b/lib/private/Files/SetupManagerFactory.php @@ -23,6 +23,7 @@ declare(strict_types=1); namespace OC\Files; +use OC\Share20\ShareDisableChecker; use OCP\Diagnostics\IEventLogger; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Config\IMountProviderCollection; @@ -36,40 +37,21 @@ use OCP\Lockdown\ILockdownManager; use Psr\Log\LoggerInterface; class SetupManagerFactory { - private IEventLogger $eventLogger; - private IMountProviderCollection $mountProviderCollection; - private IUserManager $userManager; - private IEventDispatcher $eventDispatcher; - private IUserMountCache $userMountCache; - private ILockdownManager $lockdownManager; - private IUserSession $userSession; private ?SetupManager $setupManager; - private ICacheFactory $cacheFactory; - private LoggerInterface $logger; - private IConfig $config; public function __construct( - IEventLogger $eventLogger, - IMountProviderCollection $mountProviderCollection, - IUserManager $userManager, - IEventDispatcher $eventDispatcher, - IUserMountCache $userMountCache, - ILockdownManager $lockdownManager, - IUserSession $userSession, - ICacheFactory $cacheFactory, - LoggerInterface $logger, - IConfig $config + private IEventLogger $eventLogger, + private IMountProviderCollection $mountProviderCollection, + private IUserManager $userManager, + private IEventDispatcher $eventDispatcher, + private IUserMountCache $userMountCache, + private ILockdownManager $lockdownManager, + private IUserSession $userSession, + private ICacheFactory $cacheFactory, + private LoggerInterface $logger, + private IConfig $config, + private ShareDisableChecker $shareDisableChecker, ) { - $this->eventLogger = $eventLogger; - $this->mountProviderCollection = $mountProviderCollection; - $this->userManager = $userManager; - $this->eventDispatcher = $eventDispatcher; - $this->userMountCache = $userMountCache; - $this->lockdownManager = $lockdownManager; - $this->userSession = $userSession; - $this->cacheFactory = $cacheFactory; - $this->logger = $logger; - $this->config = $config; $this->setupManager = null; } @@ -86,7 +68,8 @@ class SetupManagerFactory { $this->userSession, $this->cacheFactory, $this->logger, - $this->config + $this->config, + $this->shareDisableChecker, ); } return $this->setupManager; diff --git a/lib/private/Server.php b/lib/private/Server.php index bb073891d03..68bf3d76f36 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -147,6 +147,7 @@ use OC\Security\TrustedDomainHelper; use OC\Security\VerificationToken\VerificationToken; use OC\Session\CryptoWrapper; use OC\Share20\ProviderFactory; +use OC\Share20\ShareDisableChecker; use OC\Share20\ShareHelper; use OC\SpeechToText\SpeechToTextManager; use OC\SystemTag\ManagerFactory as SystemTagManagerFactory; @@ -1317,7 +1318,8 @@ class Server extends ServerContainer implements IServerContainer { $c->get('ThemingDefaults'), $c->get(IEventDispatcher::class), $c->get(IUserSession::class), - $c->get(KnownUserService::class) + $c->get(KnownUserService::class), + $c->get(ShareDisableChecker::class) ); return $manager; diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index c7d09a3d6f7..74bb505c036 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -41,7 +41,6 @@ */ namespace OC\Share20; -use OCP\Cache\CappedMemoryCache; use OC\Files\Mount\MoveableMount; use OC\KnownUser\KnownUserService; use OC\Share20\Exception\ProviderException; @@ -103,8 +102,6 @@ class Manager implements IManager { private $userManager; /** @var IRootFolder */ private $rootFolder; - /** @var CappedMemoryCache */ - private $sharingDisabledForUsersCache; /** @var EventDispatcherInterface */ private $legacyDispatcher; /** @var LegacyHooks */ @@ -121,6 +118,7 @@ class Manager implements IManager { private $userSession; /** @var KnownUserService */ private $knownUserService; + private ShareDisableChecker $shareDisableChecker; public function __construct( LoggerInterface $logger, @@ -140,7 +138,8 @@ class Manager implements IManager { \OC_Defaults $defaults, IEventDispatcher $dispatcher, IUserSession $userSession, - KnownUserService $knownUserService + KnownUserService $knownUserService, + ShareDisableChecker $shareDisableChecker ) { $this->logger = $logger; $this->config = $config; @@ -154,7 +153,6 @@ class Manager implements IManager { $this->userManager = $userManager; $this->rootFolder = $rootFolder; $this->legacyDispatcher = $legacyDispatcher; - $this->sharingDisabledForUsersCache = new CappedMemoryCache(); // The constructor of LegacyHooks registers the listeners of share events // do not remove if those are not properly migrated $this->legacyHooks = new LegacyHooks($this->legacyDispatcher); @@ -164,6 +162,7 @@ class Manager implements IManager { $this->dispatcher = $dispatcher; $this->userSession = $userSession; $this->knownUserService = $knownUserService; + $this->shareDisableChecker = $shareDisableChecker; } /** @@ -2034,37 +2033,7 @@ class Manager implements IManager { * @return bool */ public function sharingDisabledForUser($userId) { - if ($userId === null) { - return false; - } - - if (isset($this->sharingDisabledForUsersCache[$userId])) { - return $this->sharingDisabledForUsersCache[$userId]; - } - - if ($this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes') { - $groupsList = $this->config->getAppValue('core', 'shareapi_exclude_groups_list', ''); - $excludedGroups = json_decode($groupsList); - if (is_null($excludedGroups)) { - $excludedGroups = explode(',', $groupsList); - $newValue = json_encode($excludedGroups); - $this->config->setAppValue('core', 'shareapi_exclude_groups_list', $newValue); - } - $user = $this->userManager->get($userId); - $usersGroups = $this->groupManager->getUserGroupIds($user); - if (!empty($usersGroups)) { - $remainingGroups = array_diff($usersGroups, $excludedGroups); - // if the user is only in groups which are disabled for sharing then - // sharing is also disabled for the user - if (empty($remainingGroups)) { - $this->sharingDisabledForUsersCache[$userId] = true; - return true; - } - } - } - - $this->sharingDisabledForUsersCache[$userId] = false; - return false; + return $this->shareDisableChecker->sharingDisabledForUser($userId); } /** diff --git a/lib/private/Share20/ShareDisableChecker.php b/lib/private/Share20/ShareDisableChecker.php new file mode 100644 index 00000000000..9d0c2b8c2b4 --- /dev/null +++ b/lib/private/Share20/ShareDisableChecker.php @@ -0,0 +1,65 @@ +sharingDisabledForUsersCache = new CappedMemoryCache(); + } + + + /** + * @param ?string $userId + * @return bool + */ + public function sharingDisabledForUser(?string $userId) { + if ($userId === null) { + return false; + } + + if (isset($this->sharingDisabledForUsersCache[$userId])) { + return $this->sharingDisabledForUsersCache[$userId]; + } + + if ($this->config->getAppValue('core', 'shareapi_exclude_groups', 'no') === 'yes') { + $groupsList = $this->config->getAppValue('core', 'shareapi_exclude_groups_list', ''); + $excludedGroups = json_decode($groupsList); + if (is_null($excludedGroups)) { + $excludedGroups = explode(',', $groupsList); + $newValue = json_encode($excludedGroups); + $this->config->setAppValue('core', 'shareapi_exclude_groups_list', $newValue); + } + $user = $this->userManager->get($userId); + if (!$user) { + return false; + } + $usersGroups = $this->groupManager->getUserGroupIds($user); + if (!empty($usersGroups)) { + $remainingGroups = array_diff($usersGroups, $excludedGroups); + // if the user is only in groups which are disabled for sharing then + // sharing is also disabled for the user + if (empty($remainingGroups)) { + $this->sharingDisabledForUsersCache[$userId] = true; + return true; + } + } + } + + $this->sharingDisabledForUsersCache[$userId] = false; + return false; + } +} diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php index 18a6fca05b0..d8c9331fa45 100644 --- a/tests/lib/Files/ViewTest.php +++ b/tests/lib/Files/ViewTest.php @@ -7,6 +7,7 @@ namespace Test\Files; +use OC\Share20\ShareDisableChecker; use OCP\Cache\CappedMemoryCache; use OC\Files\Cache\Watcher; use OC\Files\Filesystem; @@ -295,7 +296,7 @@ class ViewTest extends \Test\TestCase { */ public function testRemoveSharePermissionWhenSharingDisabledForUser($excludeGroups, $excludeGroupsList, $expectedShareable) { // Reset sharing disabled for users cache - self::invokePrivate(\OC::$server->getShareManager(), 'sharingDisabledForUsersCache', [new CappedMemoryCache()]); + self::invokePrivate(\OC::$server->get(ShareDisableChecker::class), 'sharingDisabledForUsersCache', [new CappedMemoryCache()]); $config = \OC::$server->getConfig(); $oldExcludeGroupsFlag = $config->getAppValue('core', 'shareapi_exclude_groups', 'no'); @@ -320,7 +321,7 @@ class ViewTest extends \Test\TestCase { $config->setAppValue('core', 'shareapi_exclude_groups_list', $oldExcludeGroupsList); // Reset sharing disabled for users cache - self::invokePrivate(\OC::$server->getShareManager(), 'sharingDisabledForUsersCache', [new CappedMemoryCache()]); + self::invokePrivate(\OC::$server->get(ShareDisableChecker::class), 'sharingDisabledForUsersCache', [new CappedMemoryCache()]); } public function testCacheIncompleteFolder() { diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 55d95ea4515..18a55ca996c 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -27,6 +27,7 @@ use OC\Share20\DefaultShareProvider; use OC\Share20\Exception; use OC\Share20\Manager; use OC\Share20\Share; +use OC\Share20\ShareDisableChecker; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\File; @@ -110,6 +111,8 @@ class ManagerTest extends \Test\TestCase { protected $userSession; /** @var KnownUserService|MockObject */ protected $knownUserService; + /** @var ShareDisableChecker|MockObject */ + protected $shareDisabledChecker; protected function setUp(): void { $this->logger = $this->createMock(LoggerInterface::class); @@ -128,6 +131,8 @@ class ManagerTest extends \Test\TestCase { $this->userSession = $this->createMock(IUserSession::class); $this->knownUserService = $this->createMock(KnownUserService::class); + $this->shareDisabledChecker = new ShareDisableChecker($this->config, $this->userManager, $this->groupManager); + $this->l10nFactory = $this->createMock(IFactory::class); $this->l = $this->createMock(IL10N::class); $this->l->method('t') @@ -159,7 +164,8 @@ class ManagerTest extends \Test\TestCase { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker ); $this->defaultProvider = $this->createMock(DefaultShareProvider::class); @@ -190,7 +196,8 @@ class ManagerTest extends \Test\TestCase { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ]); } @@ -2741,7 +2748,8 @@ class ManagerTest extends \Test\TestCase { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $share = $this->createMock(IShare::class); @@ -2789,7 +2797,8 @@ class ManagerTest extends \Test\TestCase { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $share = $this->createMock(IShare::class); @@ -2844,7 +2853,8 @@ class ManagerTest extends \Test\TestCase { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $share = $this->createMock(IShare::class); @@ -4244,7 +4254,8 @@ class ManagerTest extends \Test\TestCase { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $this->assertSame($expected, $manager->shareProviderExists($shareType) @@ -4279,7 +4290,8 @@ class ManagerTest extends \Test\TestCase { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $factory->setProvider($this->defaultProvider); @@ -4345,7 +4357,8 @@ class ManagerTest extends \Test\TestCase { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $factory->setProvider($this->defaultProvider); @@ -4463,7 +4476,8 @@ class ManagerTest extends \Test\TestCase { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $factory->setProvider($this->defaultProvider); @@ -4590,7 +4604,8 @@ class ManagerTest extends \Test\TestCase { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker ); $factory->setProvider($this->defaultProvider); -- 2.39.5