From ca81323e0e5f4717797992aa0290327e702077f7 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 19 Aug 2022 10:26:40 +0200 Subject: [PATCH] More improvements - Don't fetch OCP\Files nodes each time but instead use the one in $davNode->getNode() Signed-off-by: Carl Schwan --- composer.json | 2 +- composer.lock | 14 ++-- lib/Connector/Sabre/APlugin.php | 24 +----- lib/Connector/Sabre/LockPlugin.php | 8 +- lib/Connector/Sabre/RedirectRequestPlugin.php | 8 +- lib/E2EEnabledPathCache.php | 74 +++++-------------- tests/Unit/Connector/Sabre/LockPluginTest.php | 1 - tests/psalm-baseline.xml | 15 ---- tests/stub.phpstub | 9 ++- 9 files changed, 43 insertions(+), 112 deletions(-) diff --git a/composer.json b/composer.json index f75ab59e..e9075cfd 100644 --- a/composer.json +++ b/composer.json @@ -19,7 +19,7 @@ "cs:check": "php-cs-fixer fix --dry-run --diff", "cs:fix": "php-cs-fixer fix", "psalm": "psalm.phar --threads=1", - "psalm:update-baseline": "psalm.phar --threads=1 --update-baseline --set-baseline=tests/psalm-baseline.xml", + "psalm:update-baseline": "psalm.phar --threads=1 --update-baseline", "psalm:clear": "psalm.phar --clear-cache && psalm --clear-global-cache", "psalm:fix": "psalm.phar --alter --issues=InvalidReturnType,InvalidNullableReturnType,MissingParamType,InvalidFalsableReturnType", "test": "phpunit --configuration phpunit.xml --fail-on-warning", diff --git a/composer.lock b/composer.lock index 8836c82b..6c562479 100644 --- a/composer.lock +++ b/composer.lock @@ -13,17 +13,17 @@ "source": { "type": "git", "url": "https://github.com/ChristophWurst/nextcloud_composer.git", - "reference": "19c58278e072ef28bcce2370995b785aca350149" + "reference": "9f9f727cf66a75ece93cee24c01074a477e1615e" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/ChristophWurst/nextcloud_composer/zipball/19c58278e072ef28bcce2370995b785aca350149", - "reference": "19c58278e072ef28bcce2370995b785aca350149", + "url": "https://api.github.com/repos/ChristophWurst/nextcloud_composer/zipball/9f9f727cf66a75ece93cee24c01074a477e1615e", + "reference": "9f9f727cf66a75ece93cee24c01074a477e1615e", "shasum": "" }, "require": { "php": "^7.4 || ~8.0 || ~8.1", - "psr/container": "^1.0", + "psr/container": "^1.1.1", "psr/event-dispatcher": "^1.0", "psr/log": "^1.1" }, @@ -31,7 +31,7 @@ "type": "library", "extra": { "branch-alias": { - "dev-master": "24.0.0-dev" + "dev-master": "25.0.0-dev" } }, "notification-url": "https://packagist.org/downloads/", @@ -49,7 +49,7 @@ "issues": "https://github.com/ChristophWurst/nextcloud_composer/issues", "source": "https://github.com/ChristophWurst/nextcloud_composer/tree/master" }, - "time": "2022-04-07T01:42:11+00:00" + "time": "2022-08-18T02:33:31+00:00" }, { "name": "composer/pcre", @@ -4250,5 +4250,5 @@ "platform-overrides": { "php": "7.4" }, - "plugin-api-version": "2.2.0" + "plugin-api-version": "2.3.0" } diff --git a/lib/Connector/Sabre/APlugin.php b/lib/Connector/Sabre/APlugin.php index 20b71fb3..1b107f74 100644 --- a/lib/Connector/Sabre/APlugin.php +++ b/lib/Connector/Sabre/APlugin.php @@ -22,9 +22,7 @@ */ namespace OCA\EndToEndEncryption\Connector\Sabre; -use OCP\AppFramework\Http; use OCA\DAV\Connector\Sabre\Directory; -use OCA\DAV\Connector\Sabre\Exception\Forbidden; use OCA\DAV\Connector\Sabre\File; use OCA\EndToEndEncryption\E2EEnabledPathCache; use OCP\Files\IRootFolder; @@ -99,29 +97,11 @@ protected function getNodeForPath(string $path): INode { throw new Conflict(); } - /** - * Get file system node of requested file - * @throws NotFound - */ - protected function getFileNode(string $path): Node { - $user = $this->userSession->getUser(); - if ($user === null) { - throw new Forbidden('No user session found'); - } - $uid = $user->getUID(); - return $this->pathCache->getFileNode($uid, $path, $this->rootFolder); - } - /** * Checks if the path is an E2E folder or inside an E2E folder */ - protected function isE2EEnabledPath(string $path): bool { - try { - $node = $this->getFileNode($path); - } catch (NotFound $e) { - return false; - } - return $this->pathCache->isE2EEnabledPath($node, $path); + protected function isE2EEnabledPath(\OCA\DAV\Connector\Sabre\Node $node): bool { + return $this->pathCache->isE2EEnabledPath($node->getNode()); } /** diff --git a/lib/Connector/Sabre/LockPlugin.php b/lib/Connector/Sabre/LockPlugin.php index a84f7f92..b1e1ea1a 100644 --- a/lib/Connector/Sabre/LockPlugin.php +++ b/lib/Connector/Sabre/LockPlugin.php @@ -98,21 +98,21 @@ public function checkLock(RequestInterface $request): void { $destNode = $this->getNode($destInfo['destination'], $method); if ($node instanceof FutureFile) { - if ($this->isE2EEnabledPath($destNode->getPath()) === false) { + if ($this->isE2EEnabledPath($destNode) === false) { return; } } else { // If neither is an end to end encrypted folders, we don't care - if (!$this->isE2EEnabledPath($node->getPath()) && !$this->isE2EEnabledPath($destNode->getPath())) { + if (!$this->isE2EEnabledPath($node) && !$this->isE2EEnabledPath($destNode)) { return; } // Prevent moving or copying stuff from non-encrypted to encrypted folders - if ($this->isE2EEnabledPath($node->getPath()) xor $this->isE2EEnabledPath($destNode->getPath())) { + if ($this->isE2EEnabledPath($node) xor $this->isE2EEnabledPath($destNode)) { throw new Forbidden('Cannot copy or move files from non-encrypted folders to end to end encrypted folders or vice versa.'); } } - } elseif (!$this->isE2EEnabledPath($node->getPath())) { + } elseif (!$this->isE2EEnabledPath($node)) { return; } diff --git a/lib/Connector/Sabre/RedirectRequestPlugin.php b/lib/Connector/Sabre/RedirectRequestPlugin.php index e9e8d9cc..db9a8d52 100644 --- a/lib/Connector/Sabre/RedirectRequestPlugin.php +++ b/lib/Connector/Sabre/RedirectRequestPlugin.php @@ -92,7 +92,7 @@ public function httpCopyMove(RequestInterface $request): void { return; } /** @var File|Directory $node */ - if (!$this->isE2EEnabledPath($node->getPath())) { + if (!$this->isE2EEnabledPath($node)) { return; } @@ -114,7 +114,7 @@ public function httpDelete(RequestInterface $request, ResponseInterface $respons return true; } /** @var File|Directory $node */ - if (!$this->isE2EEnabledPath($node->getPath())) { + if (!$this->isE2EEnabledPath($node)) { // If this is no e2e-enabled path, return true to continue up in the event chain return true; } @@ -149,7 +149,7 @@ public function httpMkColPut(RequestInterface $request): void { return; } /** @var File|Directory $node */ - if (!$this->isE2EEnabledPath($node->getPath())) { + if (!$this->isE2EEnabledPath($node)) { return; } @@ -184,7 +184,7 @@ public function propFind(PropFind $propFind, INode $node): bool { } /** @var File|Directory $node */ - if (!$this->isE2EEnabledPath($node->getPath())) { + if (!$this->isE2EEnabledPath($node)) { return true; } diff --git a/lib/E2EEnabledPathCache.php b/lib/E2EEnabledPathCache.php index 4408628a..291766f5 100644 --- a/lib/E2EEnabledPathCache.php +++ b/lib/E2EEnabledPathCache.php @@ -24,68 +24,37 @@ namespace OCA\EndToEndEncryption; -use Sabre\DAV\INode; use OCP\Files\Node; use OCP\Files\Storage\IStorage; use OCP\Files\IHomeStorage; use OCP\Files\Cache\ICache; use OCP\Cache\CappedMemoryCache; -use Sabre\DAV\Exception\NotFound; class E2EEnabledPathCache { /** * @psalm-type EncryptedState=bool * - * @psalm-type Path=string - * - * @psalm-type StorageId=string|int + * @psalm-type FileId=int */ - /** @var CappedMemoryCache> */ private CappedMemoryCache $perStorageEncryptedStateCache; public function __construct() { $this->perStorageEncryptedStateCache = new CappedMemoryCache(); - $this->nodeCache = []; } /** * Checks if the path is an E2E folder or inside an E2E folder - * - * @param INode&Node $node */ - public function isE2EEnabledPath($node): bool { + public function isE2EEnabledPath(Node $node): bool { + if ($node->isEncrypted()) { + return true; + } $storage = $node->getStorage(); $cache = $storage->getCache(); return $this->getEncryptedStates($cache, $node, $storage); } - /** - * Get file system node of requested file - * @throws NotFound - */ - public function getFileNode($uid, string $path, $rootFolder): Node { - if (!isset($this->nodeCache[$uid])) { - $this->nodeCache[$uid] = []; - } else if (isset($this->nodeCache[$uid][$path])) { - $node = $this->nodeCache[$uid][$path]; - if ($node instanceof \Exception) { - throw new NotFound('file not found', Http::STATUS_NOT_FOUND, $node); - } - return $node; - } - try { - $node = $rootFolder - ->getUserFolder($uid) - ->get($path); - $this->nodeCache[$uid][$path] = $node; - return $node; - } catch (Exception $e) { - $this->nodeCache[$uid][$path] = $e; - throw new NotFound('file not found', Http::STATUS_NOT_FOUND, $e); - } - } - /** * Get the encryption state for the path */ @@ -94,43 +63,36 @@ protected function getEncryptedStates(ICache $cache, $node, IStorage $storage): return false; } - $storageId = $cache->getNumericStorageId(); - if (isset($this->perStorageEncryptedStateCache[$storageId][$node->getPath()])) { - return $this->perStorageEncryptedStateCache[$storageId][$node->getPath()]; + $storageId = (string)$cache->getNumericStorageId(); + if (!isset($this->perStorageEncryptedStateCache[$storageId])) { + $this->perStorageEncryptedStateCache[$storageId] = []; + } + if (isset($this->perStorageEncryptedStateCache[$storageId][$node->getId()])) { + return $this->perStorageEncryptedStateCache[$storageId][$node->getId()]; } $parentIds = []; - if ($node->getPath() === '/' || $node->getPath() === '') { + if ($node->getPath() === '/') { // root is never encrypted - $this->perStorageEncryptedStateCache[$storageId][$node->getPath()] = false; + $this->perStorageEncryptedStateCache[$storageId][$node->getId()] = false; return false; } if ($node->isEncrypted()) { // no need to go further down in the tree - $this->perStorageEncryptedStateCache[$storageId][$node->getPath()] = true; + $this->perStorageEncryptedStateCache[$storageId][$node->getId()] = true; return true; } // go down more, but try first just with the parent path to spare a lot of // queries if already cached - $parentPath = $this->dirname($node->getPath()); - if (isset($this->perStorageEncryptedStateCache[$storageId][$parentPath])) { - return $this->perStorageEncryptedStateCache[$storageId][$parentPath]; - } - - if ($parentPath === '/' || $parentPath === '.') { - $this->perStorageEncryptedStateCache[$storageId][$node->getPath()] = false; - return false; + $parentId = $node->getFileInfo()['parent']; + if (isset($this->perStorageEncryptedStateCache[$storageId][$parentId])) { + return $this->perStorageEncryptedStateCache[$storageId][$parentId]; } $encrypted = $this->getEncryptedStates($cache, $node->getParent(), $storage); - $this->perStorageEncryptedStateCache[$storageId][$node->getPath()] = $encrypted; + $this->perStorageEncryptedStateCache[$storageId][$node->getId()] = $encrypted; return $encrypted; } - - protected function dirname(string $path): string { - $dir = dirname($path); - return $dir === '.' ? '' : $dir; - } } diff --git a/tests/Unit/Connector/Sabre/LockPluginTest.php b/tests/Unit/Connector/Sabre/LockPluginTest.php index 4e666a7b..6b06bce4 100644 --- a/tests/Unit/Connector/Sabre/LockPluginTest.php +++ b/tests/Unit/Connector/Sabre/LockPluginTest.php @@ -34,7 +34,6 @@ use OCA\EndToEndEncryption\E2EEnabledPathCache; use OCP\Files\FileInfo; use OCP\Files\IRootFolder; -use OCP\Files\Storage\IStorage; use OCP\Files\IHomeStorage; use OCP\Files\Node; use OCP\IUserSession; diff --git a/tests/psalm-baseline.xml b/tests/psalm-baseline.xml index 5e623b62..d553aba7 100644 --- a/tests/psalm-baseline.xml +++ b/tests/psalm-baseline.xml @@ -7,9 +7,6 @@ - - $node - $this->server->tree @@ -34,16 +31,4 @@ $this->server->tree - - - CappedMemoryCache - - - $this->perStorageEncryptedStateCache - $this->perStorageEncryptedStateCache - $this->perStorageEncryptedStateCache - $this->perStorageEncryptedStateCache - CappedMemoryCache - - diff --git a/tests/stub.phpstub b/tests/stub.phpstub index 3d1ab8cb..df0a2581 100644 --- a/tests/stub.phpstub +++ b/tests/stub.phpstub @@ -76,8 +76,13 @@ namespace Sabre\Dav { } namespace OCA\DAV\Connector\Sabre { - class File extends \Sabre\Dav\INode {} - class Directory extends \Sabre\Dav\INode {} + class Node extends \Sabre\Dav\INode { + public function getNode(): \OCP\Files\Node { + + } + } + class File extends Node {} + class Directory extends Node {} } namespace OCA\DAV\Connector\Sabre\Exception {