From 044ffb742312b325c2f4d437b4dc3fbcd6b287e1 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 22 Aug 2017 17:40:01 +0200 Subject: [PATCH 1/4] Use the usermountcache to get all users who have access to a file --- lib/FilesHooks.php | 19 ++++++++++++-- tests/FilesHooksTest.php | 57 +++++++++++++++++++++++++++++++++------- 2 files changed, 65 insertions(+), 11 deletions(-) diff --git a/lib/FilesHooks.php b/lib/FilesHooks.php index fecdd8a1f..7730d4a01 100755 --- a/lib/FilesHooks.php +++ b/lib/FilesHooks.php @@ -30,6 +30,9 @@ use OCA\Activity\Extension\Files; use OCA\Activity\Extension\Files_Sharing; use OCP\Activity\IManager; +use OCP\Files\Config\ICachedMountFileInfo; +use OCP\Files\Config\ICachedMountInfo; +use OCP\Files\Config\IUserMountCache; use OCP\Files\IRootFolder; use OCP\Files\Mount\IMountPoint; use OCP\Files\Node; @@ -93,6 +96,8 @@ class FilesHooks { protected $oldParentOwner; /** @var string */ protected $oldParentId; + /** @var IUserMountCache */ + protected $userMountCache; /** * Constructor @@ -108,6 +113,7 @@ class FilesHooks { * @param IURLGenerator $urlGenerator * @param ILogger $logger * @param CurrentUser $currentUser + * @param IUserMountCache $userMountCache */ public function __construct(IManager $manager, Data $activityData, @@ -119,7 +125,8 @@ public function __construct(IManager $manager, IDBConnection $connection, IURLGenerator $urlGenerator, ILogger $logger, - CurrentUser $currentUser) { + CurrentUser $currentUser, + IUserMountCache $userMountCache) { $this->manager = $manager; $this->activityData = $activityData; $this->userSettings = $userSettings; @@ -131,6 +138,7 @@ public function __construct(IManager $manager, $this->urlGenerator = $urlGenerator; $this->logger = $logger; $this->currentUser = $currentUser; + $this->userMountCache = $userMountCache; } /** @@ -197,7 +205,14 @@ protected function addNotificationsForFileAction($filePath, $activityType, $subj $this->generateRemoteActivity($accessList['remotes'], $activityType, time(), $this->currentUser->getCloudId(), $accessList['ownerPath']); - $affectedUsers = $accessList['users']; + $mountsForFile = $this->userMountCache->getMountsForFileId($fileId); + $affectedUserIds = array_map(function (ICachedMountInfo $mount) { + return $mount->getUser()->getUID(); + }, $mountsForFile); + $affectedPaths = array_map(function (ICachedMountFileInfo $mount) { + return $mount->getPath(); + }, $mountsForFile); + $affectedUsers = array_combine($affectedUserIds, $affectedPaths); $filteredStreamUsers = $this->userSettings->filterUsersBySetting(array_keys($affectedUsers), 'stream', $activityType); $filteredEmailUsers = $this->userSettings->filterUsersBySetting(array_keys($affectedUsers), 'email', $activityType); diff --git a/tests/FilesHooksTest.php b/tests/FilesHooksTest.php index d6cd302f6..cf3b9e88b 100755 --- a/tests/FilesHooksTest.php +++ b/tests/FilesHooksTest.php @@ -22,9 +22,11 @@ namespace OCA\Activity; +use OC\Files\Config\CachedMountFileInfo; use OCA\Activity\Extension\Files; use OCA\Activity\Extension\Files_Sharing; use OCA\Activity\Tests\TestCase; +use OCP\Files\Config\IUserMountCache; use OCP\Files\IRootFolder; use OCP\Files\NotFoundException; use OCP\ILogger; @@ -61,6 +63,8 @@ class FilesHooksTest extends TestCase { protected $shareHelper; /** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */ protected $urlGenerator; + /** @var IUserMountCache|\PHPUnit_Framework_MockObject_MockObject */ + protected $userMountCache; protected function setUp(): void { parent::setUp(); @@ -73,6 +77,7 @@ protected function setUp(): void { $this->rootFolder = $this->createMock(IRootFolder::class); $this->shareHelper = $this->createMock(IShareHelper::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->userMountCache = $this->createMock(IUserMountCache::class); $this->filesHooks = $this->getFilesHooks(); } @@ -107,6 +112,7 @@ protected function getFilesHooks(array $mockedMethods = [], $user = 'user') { $this->urlGenerator, $logger, $currentUser, + $this->userMountCache ]) ->setMethods($mockedMethods) ->getMock(); @@ -123,7 +129,8 @@ protected function getFilesHooks(array $mockedMethods = [], $user = 'user') { \OC::$server->getDatabaseConnection(), $this->urlGenerator, $logger, - $currentUser + $currentUser, + $this->userMountCache ); } @@ -236,8 +243,8 @@ public function dataAddNotificationsForFileAction() { [ 'user' => [ 'subject' => 'restored_self', - 'subject_params' => [[1337 => '/user/path']], - 'path' => '/user/path', + 'subject_params' => [[1337 => '/user/files/path']], + 'path' => '/user/files/path', 'stream' => true, 'email' => 42, ], @@ -251,8 +258,8 @@ public function dataAddNotificationsForFileAction() { [ 'user1' => [ 'subject' => 'restored_by', - 'subject_params' => [[1337 => '/user1/path'], 'user'], - 'path' => '/user1/path', + 'subject_params' => [[1337 => '/user1/files/path'], 'user'], + 'path' => '/user1/files/path', 'stream' => true, 'email' => 0, ], @@ -284,13 +291,45 @@ public function testAddNotificationsForFileAction($filterUsers, $addNotification ->willReturn([ 'ownerPath' => '/owner/path', 'users' => [ - 'user' => '/user/path', - 'user1' => '/user1/path', - 'user2' => '/user2/path', + 'user' => '/user/files/path', + 'user1' => '/user1/files/path', + 'user2' => '/user2/files/path', ], 'remotes' => [], ]); + $this->userMountCache->expects($this->once()) + ->method('getMountsForFileId') + ->willReturn([ + new CachedMountFileInfo( + $this->getUserMock('user'), + 1, + 1, + '/user/files/', + null, + '', + 'path' + ), + new CachedMountFileInfo( + $this->getUserMock('user1'), + 1, + 1, + '/user1/files/', + null, + '', + 'path' + ), + new CachedMountFileInfo( + $this->getUserMock('user2'), + 1, + 1, + '/user2/files/', + null, + '', + 'path' + ) + ]); + $this->settings->expects($this->exactly(2)) ->method('filterUsersBySetting') ->willReturnMap($filterUsers); @@ -852,7 +891,7 @@ public function dataAddNotificationsForUser() { ['notAuthor', 'subject', ['parameter'], 42, 'path/subpath', 'path', true, false, true, Files::TYPE_SHARE_CREATED, false, false, 'files', false, true], ['notAuthor', 'subject', ['parameter'], 0, 'path/subpath', 'path', true, false, true, Files::TYPE_SHARE_CREATED, false, false, 'files', false, true], ['notAuthor', 'subject', ['parameter'], 0, 'path/subpath', 'path', true, false, true, Files::TYPE_SHARE_CREATED, false, false, 'files', false, true], - ['notAuthor', 'subject', ['parameter'], 0, 'path/subpath','path/subpath', false, false, true, Files::TYPE_SHARE_CREATED, false, false, 'files', false, true], + ['notAuthor', 'subject', ['parameter'], 0, 'path/subpath', 'path/subpath', false, false, true, Files::TYPE_SHARE_CREATED, false, false, 'files', false, true], ]; } From 7e795984bae44ec27e8754fdbb6dba2523a22d5c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 10 Mar 2020 10:25:24 +0100 Subject: [PATCH 2/4] Only use the visible path Signed-off-by: Joas Schilling --- lib/FilesHooks.php | 15 ++++++++++----- tests/FilesHooksTest.php | 8 ++++---- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/lib/FilesHooks.php b/lib/FilesHooks.php index 7730d4a01..2ce0aed02 100755 --- a/lib/FilesHooks.php +++ b/lib/FilesHooks.php @@ -210,7 +210,7 @@ protected function addNotificationsForFileAction($filePath, $activityType, $subj return $mount->getUser()->getUID(); }, $mountsForFile); $affectedPaths = array_map(function (ICachedMountFileInfo $mount) { - return $mount->getPath(); + return $this->getVisiblePath($mount->getPath()); }, $mountsForFile); $affectedUsers = array_combine($affectedUserIds, $affectedPaths); $filteredStreamUsers = $this->userSettings->filterUsersBySetting(array_keys($affectedUsers), 'stream', $activityType); @@ -631,15 +631,20 @@ protected function getUserPathsFromPath($path, $uidOwner) { $accessList = $this->shareHelper->getPathsForAccessList($node); $path = $node->getPath(); - $sections = explode('/', $path, 4); + $accessList['ownerPath'] = $this->getVisiblePath($path); + return $accessList; + } - $accessList['ownerPath'] = '/'; + protected function getVisiblePath(string $absolutePath): string { + $sections = explode('/', $absolutePath, 4); + + $path = '/'; if (isset($sections[3])) { // Not the case when a file in root is renamed - $accessList['ownerPath'] .= $sections[3]; + $path .= $sections[3]; } - return $accessList; + return $path; } /** diff --git a/tests/FilesHooksTest.php b/tests/FilesHooksTest.php index cf3b9e88b..3bf46858d 100755 --- a/tests/FilesHooksTest.php +++ b/tests/FilesHooksTest.php @@ -243,8 +243,8 @@ public function dataAddNotificationsForFileAction() { [ 'user' => [ 'subject' => 'restored_self', - 'subject_params' => [[1337 => '/user/files/path']], - 'path' => '/user/files/path', + 'subject_params' => [[1337 => '/path']], + 'path' => '/path', 'stream' => true, 'email' => 42, ], @@ -258,8 +258,8 @@ public function dataAddNotificationsForFileAction() { [ 'user1' => [ 'subject' => 'restored_by', - 'subject_params' => [[1337 => '/user1/files/path'], 'user'], - 'path' => '/user1/files/path', + 'subject_params' => [[1337 => '/path'], 'user'], + 'path' => '/path', 'stream' => true, 'email' => 0, ], From 2a3a63f9de5efeee3c90e848b170975f7ffed3ac Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 10 Mar 2020 11:22:35 +0100 Subject: [PATCH 3/4] Add a config to use the cached mount points Signed-off-by: Joas Schilling --- lib/FilesHooks.php | 48 +++++++--------- tests/FilesHooksTest.php | 119 ++++++++++++++++++++++++++++----------- 2 files changed, 106 insertions(+), 61 deletions(-) diff --git a/lib/FilesHooks.php b/lib/FilesHooks.php index 2ce0aed02..c2d055632 100755 --- a/lib/FilesHooks.php +++ b/lib/FilesHooks.php @@ -37,6 +37,7 @@ use OCP\Files\Mount\IMountPoint; use OCP\Files\Node; use OCP\Files\NotFoundException; +use OCP\IConfig; use OCP\IDBConnection; use OCP\IGroup; use OCP\IGroupManager; @@ -85,6 +86,10 @@ class FilesHooks { /** @var CurrentUser */ protected $currentUser; + /** @var IUserMountCache */ + protected $userMountCache; + /** @var IConfig */ + protected $config; /** @var string|bool */ protected $moveCase = false; @@ -96,25 +101,7 @@ class FilesHooks { protected $oldParentOwner; /** @var string */ protected $oldParentId; - /** @var IUserMountCache */ - protected $userMountCache; - /** - * Constructor - * - * @param IManager $manager - * @param Data $activityData - * @param UserSettings $userSettings - * @param IGroupManager $groupManager - * @param View $view - * @param IRootFolder $rootFolder - * @param IShareHelper $shareHelper - * @param IDBConnection $connection - * @param IURLGenerator $urlGenerator - * @param ILogger $logger - * @param CurrentUser $currentUser - * @param IUserMountCache $userMountCache - */ public function __construct(IManager $manager, Data $activityData, UserSettings $userSettings, @@ -126,7 +113,8 @@ public function __construct(IManager $manager, IURLGenerator $urlGenerator, ILogger $logger, CurrentUser $currentUser, - IUserMountCache $userMountCache) { + IUserMountCache $userMountCache, + IConfig $config) { $this->manager = $manager; $this->activityData = $activityData; $this->userSettings = $userSettings; @@ -139,6 +127,7 @@ public function __construct(IManager $manager, $this->logger = $logger; $this->currentUser = $currentUser; $this->userMountCache = $userMountCache; + $this->config = $config; } /** @@ -205,14 +194,19 @@ protected function addNotificationsForFileAction($filePath, $activityType, $subj $this->generateRemoteActivity($accessList['remotes'], $activityType, time(), $this->currentUser->getCloudId(), $accessList['ownerPath']); - $mountsForFile = $this->userMountCache->getMountsForFileId($fileId); - $affectedUserIds = array_map(function (ICachedMountInfo $mount) { - return $mount->getUser()->getUID(); - }, $mountsForFile); - $affectedPaths = array_map(function (ICachedMountFileInfo $mount) { - return $this->getVisiblePath($mount->getPath()); - }, $mountsForFile); - $affectedUsers = array_combine($affectedUserIds, $affectedPaths); + if ($this->config->getSystemValueBool('activity_use_cached_mountpoints', false)) { + $mountsForFile = $this->userMountCache->getMountsForFileId($fileId); + $affectedUserIds = array_map(function (ICachedMountInfo $mount) { + return $mount->getUser()->getUID(); + }, $mountsForFile); + $affectedPaths = array_map(function (ICachedMountFileInfo $mount) { + return $this->getVisiblePath($mount->getPath()); + }, $mountsForFile); + $affectedUsers = array_combine($affectedUserIds, $affectedPaths); + } else { + $affectedUsers = $accessList['users']; + } + $filteredStreamUsers = $this->userSettings->filterUsersBySetting(array_keys($affectedUsers), 'stream', $activityType); $filteredEmailUsers = $this->userSettings->filterUsersBySetting(array_keys($affectedUsers), 'email', $activityType); diff --git a/tests/FilesHooksTest.php b/tests/FilesHooksTest.php index 3bf46858d..13c1d3676 100755 --- a/tests/FilesHooksTest.php +++ b/tests/FilesHooksTest.php @@ -29,6 +29,7 @@ use OCP\Files\Config\IUserMountCache; use OCP\Files\IRootFolder; use OCP\Files\NotFoundException; +use OCP\IConfig; use OCP\ILogger; use OCP\Share; use OCP\Share\IShareHelper; @@ -63,8 +64,10 @@ class FilesHooksTest extends TestCase { protected $shareHelper; /** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */ protected $urlGenerator; - /** @var IUserMountCache|\PHPUnit_Framework_MockObject_MockObject */ + /** @var IUserMountCache|MockObject */ protected $userMountCache; + /** @var IConfig|MockObject */ + protected $config; protected function setUp(): void { parent::setUp(); @@ -78,6 +81,7 @@ protected function setUp(): void { $this->shareHelper = $this->createMock(IShareHelper::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->userMountCache = $this->createMock(IUserMountCache::class); + $this->config = $this->createMock(IConfig::class); $this->filesHooks = $this->getFilesHooks(); } @@ -112,7 +116,8 @@ protected function getFilesHooks(array $mockedMethods = [], $user = 'user') { $this->urlGenerator, $logger, $currentUser, - $this->userMountCache + $this->userMountCache, + $this->config, ]) ->setMethods($mockedMethods) ->getMock(); @@ -130,7 +135,8 @@ protected function getFilesHooks(array $mockedMethods = [], $user = 'user') { $this->urlGenerator, $logger, $currentUser, - $this->userMountCache + $this->userMountCache, + $this->config ); } @@ -240,6 +246,39 @@ public function dataAddNotificationsForFileAction() { [['user', 'user1', 'user2'], 'stream', Files::TYPE_SHARE_RESTORED, ['user' => true]], [['user', 'user1', 'user2'], 'email', Files::TYPE_SHARE_RESTORED, ['user' => 42]], ], + 'mountcache_used' => false, + [ + 'user' => [ + 'subject' => 'restored_self', + 'subject_params' => [[1337 => '/user/files/path']], + 'path' => '/user/files/path', + 'stream' => true, + 'email' => 42, + ], + ], + ], + [ + [ + [['user', 'user1', 'user2'], 'stream', Files::TYPE_SHARE_RESTORED, ['user1' => true]], + [['user', 'user1', 'user2'], 'email', Files::TYPE_SHARE_RESTORED, []], + ], + 'mountcache_used' => false, + [ + 'user1' => [ + 'subject' => 'restored_by', + 'subject_params' => [[1337 => '/user1/files/path'], 'user'], + 'path' => '/user1/files/path', + 'stream' => true, + 'email' => 0, + ], + ], + ], + [ + [ + [['user', 'user1', 'user2'], 'stream', Files::TYPE_SHARE_RESTORED, ['user' => true]], + [['user', 'user1', 'user2'], 'email', Files::TYPE_SHARE_RESTORED, ['user' => 42]], + ], + 'mountcache_used' => true, [ 'user' => [ 'subject' => 'restored_self', @@ -255,6 +294,7 @@ public function dataAddNotificationsForFileAction() { [['user', 'user1', 'user2'], 'stream', Files::TYPE_SHARE_RESTORED, ['user1' => true]], [['user', 'user1', 'user2'], 'email', Files::TYPE_SHARE_RESTORED, []], ], + 'mountcache_used' => true, [ 'user1' => [ 'subject' => 'restored_by', @@ -272,6 +312,7 @@ public function dataAddNotificationsForFileAction() { * @dataProvider dataAddNotificationsForFileAction * * @param array $filterUsers + * @param bool $mountCacheUsed * @param array $addNotifications */ public function testAddNotificationsForFileAction($filterUsers, $addNotifications) { @@ -298,37 +339,47 @@ public function testAddNotificationsForFileAction($filterUsers, $addNotification 'remotes' => [], ]); - $this->userMountCache->expects($this->once()) - ->method('getMountsForFileId') - ->willReturn([ - new CachedMountFileInfo( - $this->getUserMock('user'), - 1, - 1, - '/user/files/', - null, - '', - 'path' - ), - new CachedMountFileInfo( - $this->getUserMock('user1'), - 1, - 1, - '/user1/files/', - null, - '', - 'path' - ), - new CachedMountFileInfo( - $this->getUserMock('user2'), - 1, - 1, - '/user2/files/', - null, - '', - 'path' - ) - ]); + $this->config->expects($this->once()) + ->method('getSystemValueBool') + ->with('activity_use_cached_mountpoints', false) + ->willReturn($mountCacheUsed); + + if ($mountCacheUsed) { + $this->userMountCache->expects($this->once()) + ->method('getMountsForFileId') + ->willReturn([ + new CachedMountFileInfo( + $this->getUserMock('user'), + 1, + 1, + '/user/files/', + null, + '', + 'path' + ), + new CachedMountFileInfo( + $this->getUserMock('user1'), + 1, + 1, + '/user1/files/', + null, + '', + 'path' + ), + new CachedMountFileInfo( + $this->getUserMock('user2'), + 1, + 1, + '/user2/files/', + null, + '', + 'path' + ) + ]); + } else { + $this->userMountCache->expects($this->never()) + ->method('getMountsForFileId'); + } $this->settings->expects($this->exactly(2)) ->method('filterUsersBySetting') From de92930faa1c06c00c08a8cda5f74d63c90e6ba6 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 26 May 2020 09:34:38 +0200 Subject: [PATCH 4/4] Fix unit test Signed-off-by: Joas Schilling --- tests/FilesHooksTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/FilesHooksTest.php b/tests/FilesHooksTest.php index 13c1d3676..6544a015a 100755 --- a/tests/FilesHooksTest.php +++ b/tests/FilesHooksTest.php @@ -315,7 +315,7 @@ public function dataAddNotificationsForFileAction() { * @param bool $mountCacheUsed * @param array $addNotifications */ - public function testAddNotificationsForFileAction($filterUsers, $addNotifications) { + public function testAddNotificationsForFileAction($filterUsers, $mountCacheUsed, $addNotifications) { $filesHooks = $this->getFilesHooks([ 'getSourcePathAndOwner', 'getUserPathsFromPath',