From e73889a24099091810a66623ded57346692ae2d6 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 0d21718f98a6a..1a3c416b10cb0 100644 --- a/apps/files_sharing/tests/CapabilitiesTest.php +++ b/apps/files_sharing/tests/CapabilitiesTest.php @@ -30,6 +30,7 @@ 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; @@ -94,7 +95,8 @@ private function getResults(array $map) { $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 2e5c239b6ed1b..5e33fff0afbb4 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1642,6 +1642,7 @@ '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 48c6701c7c605..3a98c00d49d7a 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1675,6 +1675,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 b1d609a9225dd..56b573d8173b6 100644 --- a/lib/private/Files/SetupManager.php +++ b/lib/private/Files/SetupManager.php @@ -35,6 +35,7 @@ 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\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 @@ private function setupBuiltinWrappers() { 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 1d9efbd411fbe..8589cbdea42a4 100644 --- a/lib/private/Files/SetupManagerFactory.php +++ b/lib/private/Files/SetupManagerFactory.php @@ -23,6 +23,7 @@ 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 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 @@ public function create(IMountManager $mountManager): SetupManager { $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 e9966e83caee9..f273d64944391 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -146,6 +146,7 @@ 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; @@ -1237,7 +1238,8 @@ public function __construct($webRoot, \OC\Config $config) { $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 9360046bc248e..3701d2686cea9 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; @@ -106,8 +105,6 @@ class Manager implements IManager { private $userManager; /** @var IRootFolder */ private $rootFolder; - /** @var CappedMemoryCache */ - private $sharingDisabledForUsersCache; /** @var LegacyHooks */ private $legacyHooks; /** @var IMailer */ @@ -122,6 +119,7 @@ class Manager implements IManager { private $userSession; /** @var KnownUserService */ private $knownUserService; + private ShareDisableChecker $shareDisableChecker; public function __construct( LoggerInterface $logger, @@ -140,7 +138,8 @@ public function __construct( \OC_Defaults $defaults, IEventDispatcher $dispatcher, IUserSession $userSession, - KnownUserService $knownUserService + KnownUserService $knownUserService, + ShareDisableChecker $shareDisableChecker ) { $this->logger = $logger; $this->config = $config; @@ -153,7 +152,6 @@ public function __construct( $this->factory = $factory; $this->userManager = $userManager; $this->rootFolder = $rootFolder; - $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($dispatcher); @@ -163,6 +161,7 @@ public function __construct( $this->dispatcher = $dispatcher; $this->userSession = $userSession; $this->knownUserService = $knownUserService; + $this->shareDisableChecker = $shareDisableChecker; } /** @@ -2011,37 +2010,7 @@ public function currentUserCanEnumerateTargetUser(?IUser $currentUser, IUser $ta * @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 0000000000000..9d0c2b8c2b480 --- /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 18a6fca05b0ad..d8c9331fa450b 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 @@ public function sharingDisabledPermissionProvider() { */ 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 @@ public function testRemoveSharePermissionWhenSharingDisabledForUser($excludeGrou $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 e8704600e2a86..eaf085cf50d77 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -27,6 +27,7 @@ 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; @@ -111,6 +112,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 @@ protected function setUp(): void { $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') @@ -158,7 +163,8 @@ protected function setUp(): void { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker ); $this->defaultProvider = $this->createMock(DefaultShareProvider::class); @@ -188,7 +194,8 @@ private function createManagerMock() { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ]); } @@ -2753,7 +2760,8 @@ public function testGetShareByToken() { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $share = $this->createMock(IShare::class); @@ -2798,7 +2806,8 @@ public function testGetShareByTokenRoom() { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $share = $this->createMock(IShare::class); @@ -2850,7 +2859,8 @@ public function testGetShareByTokenWithException() { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $share = $this->createMock(IShare::class); @@ -4191,7 +4201,8 @@ public function testShareProviderExists($shareType, $expected) { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $this->assertSame($expected, $manager->shareProviderExists($shareType) @@ -4225,7 +4236,8 @@ public function testGetSharesInFolder() { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $factory->setProvider($this->defaultProvider); @@ -4290,7 +4302,8 @@ public function testGetAccessList() { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $factory->setProvider($this->defaultProvider); @@ -4407,7 +4420,8 @@ public function testGetAccessListWithCurrentAccess() { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker, ); $factory->setProvider($this->defaultProvider); @@ -4533,7 +4547,8 @@ public function testGetAllShares() { $this->defaults, $this->dispatcher, $this->userSession, - $this->knownUserService + $this->knownUserService, + $this->shareDisabledChecker ); $factory->setProvider($this->defaultProvider);