From 3e90cc881c08b7a46155a6f31bbb6624e4fce4fb Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 7 Oct 2025 20:01:10 +0200 Subject: [PATCH] refactor: untangle Node|ICacheEntry parameter in shouldWatermark Results in split methods as we do not have nice overloading in PHP. But simplifies parameters when calling them, while implementation is being hidden yet simplified as it does not have to deal with different objects but a few primitive types. Signed-off-by: Arthur Schiwon --- lib/Controller/WopiController.php | 2 +- lib/Listener/BeforeFetchPreviewListener.php | 2 +- ...OverwritePublicSharePropertiesListener.php | 2 +- lib/PermissionManager.php | 52 +++++++++++++++---- lib/Storage/SecureViewWrapper.php | 2 +- tests/lib/PermissionManagerTest.php | 42 +++++++++++++-- 6 files changed, 83 insertions(+), 19 deletions(-) diff --git a/lib/Controller/WopiController.php b/lib/Controller/WopiController.php index b473aec53..868e8f0ee 100644 --- a/lib/Controller/WopiController.php +++ b/lib/Controller/WopiController.php @@ -136,7 +136,7 @@ class WopiController extends Controller { $isTaskProcessingEnabled = $isSmartPickerEnabled && $this->taskProcessingManager->isTaskProcessingEnabled(); $share = $this->getShareForWopiToken($wopi, $file); - $shouldUseSecureView = $this->permissionManager->shouldWatermark($file, $wopi->getEditorUid(), $share); + $shouldUseSecureView = $this->permissionManager->shouldWatermarkByNode($file, $wopi->getEditorUid(), $share); // If the file is locked manually by a user we want to open it read only for all others $canWriteThroughLock = true; diff --git a/lib/Listener/BeforeFetchPreviewListener.php b/lib/Listener/BeforeFetchPreviewListener.php index f8e0ed20e..0413cd11b 100644 --- a/lib/Listener/BeforeFetchPreviewListener.php +++ b/lib/Listener/BeforeFetchPreviewListener.php @@ -49,7 +49,7 @@ class BeforeFetchPreviewListener implements IEventListener { } $userId = $this->userSession->getUser() ? $this->userSession->getUser()->getUID() : null; - if ($this->permissionManager->shouldWatermark($event->getNode(), $userId, $share)) { + if ($this->permissionManager->shouldWatermarkByNode($event->getNode(), $userId, $share)) { throw new NotFoundException(); } } diff --git a/lib/Listener/OverwritePublicSharePropertiesListener.php b/lib/Listener/OverwritePublicSharePropertiesListener.php index fadfa4981..e773f5363 100644 --- a/lib/Listener/OverwritePublicSharePropertiesListener.php +++ b/lib/Listener/OverwritePublicSharePropertiesListener.php @@ -33,7 +33,7 @@ class OverwritePublicSharePropertiesListener implements IEventListener { return; } - if ($this->permissionManager->shouldWatermark($node, $this->userId, $share)) { + if ($this->permissionManager->shouldWatermarkByNode($node, $this->userId, $share)) { $share->setHideDownload(true); } } diff --git a/lib/PermissionManager.php b/lib/PermissionManager.php index e7806a867..a68783f3a 100644 --- a/lib/PermissionManager.php +++ b/lib/PermissionManager.php @@ -113,21 +113,54 @@ class PermissionManager { return false; } - public function shouldWatermark(Node|ICacheEntry $nodeOrCacheEntry, ?string $userId = null, ?IShare $share = null, ?string $ownerId = null): bool { + public function shouldWatermarkByNode( + Node $node, + ?string $userId = null, + ?IShare $share = null + ): bool { + return $this->shouldWatermark( + $node->getId(), + $node->getMimetype(), + $node->isUpdateable(), + $userId, + $share, + $node->getOwner()?->getUID(), + ); + } + + public function shouldWatermarkByCacheEntry( + ICacheEntry $cacheEntry, + ?string $userId = null, + ?IShare $share = null, + ?string $ownerId = null + ): bool { + return $this->shouldWatermark( + $cacheEntry->getId(), + $cacheEntry->getMimetype(), + (bool)($cacheEntry->getPermissions() & Constants::PERMISSION_UPDATE), + $userId, + $share, + $ownerId, + ); + } + + protected function shouldWatermark( + int $fileId, + string $mimetype, + bool $isFileUpdatable, + ?string $userId, + ?IShare $share, + ?string $ownerId, + ): bool { if ($this->config->getAppValue(AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_enabled', 'no') === 'no') { return false; } - if (!in_array($nodeOrCacheEntry->getMimetype(), $this->appConfig->getMimeTypes(), true)) { + if (!in_array($mimetype, $this->appConfig->getMimeTypes(), true)) { return false; } - $fileId = $nodeOrCacheEntry->getId(); - - $isUpdatable = $nodeOrCacheEntry instanceof Node - ? $nodeOrCacheEntry->isUpdateable() - : $nodeOrCacheEntry->getPermissions() & Constants::PERMISSION_UPDATE; - $isUpdatable = $isUpdatable && (!$share || $share->getPermissions() & Constants::PERMISSION_UPDATE); + $isUpdatable = $isFileUpdatable && (!$share || $share->getPermissions() & Constants::PERMISSION_UPDATE); $hasShareAttributes = $share && method_exists($share, 'getAttributes') && $share->getAttributes() instanceof IAttributes; $isDisabledDownload = $hasShareAttributes && $share->getAttributes()->getAttribute('permissions', 'download') === false; @@ -157,9 +190,6 @@ class PermissionManager { } if ($this->config->getAppValue(AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_shareAll', 'no') === 'yes') { - if (!$ownerId && $nodeOrCacheEntry instanceof Node) { - $ownerId = $nodeOrCacheEntry->getOwner()?->getUID(); - } if ($userId === null || $ownerId !== $userId) { return true; } diff --git a/lib/Storage/SecureViewWrapper.php b/lib/Storage/SecureViewWrapper.php index a3ce2c616..4a8ad19e2 100644 --- a/lib/Storage/SecureViewWrapper.php +++ b/lib/Storage/SecureViewWrapper.php @@ -107,7 +107,7 @@ class SecureViewWrapper extends Wrapper { $share = $isSharedStorage ? $storage->getShare() : null; $userId = $this->userSession->getUser()?->getUID(); - return $this->permissionManager->shouldWatermark($cacheEntry, $userId, $share, $storage->getOwner($path) ?: null); + return $this->permissionManager->shouldWatermarkByCacheEntry($cacheEntry, $userId, $share, $storage->getOwner($path) ?: null); } diff --git a/tests/lib/PermissionManagerTest.php b/tests/lib/PermissionManagerTest.php index e4d18ab1f..ba6220a53 100644 --- a/tests/lib/PermissionManagerTest.php +++ b/tests/lib/PermissionManagerTest.php @@ -8,6 +8,8 @@ namespace Tests\Richdocuments; use OCA\Richdocuments\AppConfig; use OCA\Richdocuments\Capabilities; use OCA\Richdocuments\PermissionManager; +use OCP\Constants; +use OCP\Files\Cache\ICacheEntry; use OCP\Files\Node; use OCP\IConfig; use OCP\IGroupManager; @@ -162,7 +164,7 @@ class PermissionManagerTest extends TestCase { $this->systemTagMapper->expects($this->once())->method('getTagIdsForObjects')->willReturn(['testFileId' => $objectTagIds]); $this->appConfig->expects($this->once())->method('getAppValueArray')->willReturn($watermarkTagIds); - $result = $this->permissionManager->shouldWatermark($node, $userId, $share); + $result = $this->permissionManager->shouldWatermarkByNode($node, $userId, $share); $this->assertTrue($result); } @@ -190,7 +192,7 @@ class PermissionManagerTest extends TestCase { $this->systemTagMapper->expects($this->once())->method('getTagIdsForObjects')->willReturn(['testFileId' => $objectTagIds]); $this->appConfig->expects($this->once())->method('getAppValueArray')->willReturn($watermarkTagIds); - $result = $this->permissionManager->shouldWatermark($node, $userId, null); + $result = $this->permissionManager->shouldWatermarkByNode($node, $userId, null); $this->assertTrue($result); } @@ -214,7 +216,31 @@ class PermissionManagerTest extends TestCase { [AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_linkTags', 'no', 'no'], [AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_shareTalkPublic', 'no', 'yes'], ]); - $result = $this->permissionManager->shouldWatermark($node, $userId, $share); + $result = $this->permissionManager->shouldWatermarkByNode($node, $userId, $share); + + $this->assertTrue($result); + } + + public function testShouldWatermarkCacheEntry(): void { + $cacheEntry = $this->createCacheEntryMock(); + $share = $this->createShareMock(IShare::TYPE_ROOM); + $userId = null; + + $this->appConfig->expects($this->any()) + ->method('getMimeTypes') + ->willReturn(Capabilities::MIMETYPES); + + $this->config + ->method('getAppValue') + ->willReturnMap([ + [AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_enabled', 'no', 'yes'], + [AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_linkAll', 'no', 'no'], + [AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_linkRead', 'no', 'no'], + [AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_linkSecure', 'no', 'no'], + [AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_linkTags', 'no', 'no'], + [AppConfig::WATERMARK_APP_NAMESPACE, 'watermark_shareTalkPublic', 'no', 'yes'], + ]); + $result = $this->permissionManager->shouldWatermarkByCacheEntry($cacheEntry, $userId, $share); $this->assertTrue($result); } @@ -222,11 +248,19 @@ class PermissionManagerTest extends TestCase { private function createNodeMock(): Node { $node = $this->createMock(Node::class); $node->expects($this->any())->method('getMimetype')->willReturn('application/vnd.oasis.opendocument.spreadsheet'); - $node->expects($this->any())->method('getId')->willReturn('testFileId'); + $node->expects($this->any())->method('getId')->willReturn(42); $node->expects($this->any())->method('isUpdateable')->willReturn(false); return $node; } + private function createCacheEntryMock(): ICacheEntry { + $node = $this->createMock(ICacheEntry::class); + $node->expects($this->any())->method('getMimetype')->willReturn('application/vnd.oasis.opendocument.spreadsheet'); + $node->expects($this->any())->method('getId')->willReturn(42); + $node->expects($this->any())->method('getPermissions')->willReturn(Constants::PERMISSION_READ); + return $node; + } + private function createShareMock(?int $shareType): ?IShare { if ($shareType === null) { return null;