From b6313f68d35c0da10ba598c6e25f20fb03d46d34 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 15 Aug 2025 14:04:25 +0200 Subject: [PATCH] perf(s3): Expose pre-signed urls for S3 This is faster than going back to nextcloud to download the files. This is an opt-in setting that can be enabled by setting use_presigned_url in the object store config. Additionally add support for the proxy config which is needed in a docker setup. See https://github.com/juliusknorr/nextcloud-docker-dev/pull/431 Signed-off-by: Carl Schwan --- apps/dav/lib/Connector/Sabre/File.php | 21 ++++++++++----- apps/dav/lib/Connector/Sabre/FilesPlugin.php | 27 ++++++++++++++----- apps/files_sharing/lib/SharedStorage.php | 10 ++++++- lib/private/Files/ObjectStore/Azure.php | 4 +++ .../Files/ObjectStore/ObjectStoreStorage.php | 19 +++++++++++++ .../Files/ObjectStore/S3ConnectionTrait.php | 9 ++++++- .../Files/ObjectStore/S3ObjectTrait.php | 20 ++++++++++++++ .../Files/ObjectStore/StorageObjectStore.php | 4 +++ lib/private/Files/ObjectStore/Swift.php | 3 +++ lib/private/Files/Storage/Common.php | 14 +++++----- lib/private/Files/Storage/FailedStorage.php | 4 +++ .../Files/Storage/Wrapper/Availability.php | 4 +++ lib/private/Files/Storage/Wrapper/Wrapper.php | 7 +++++ .../Lockdown/Filesystem/NullStorage.php | 4 +++ lib/public/Files/ObjectStore/IObjectStore.php | 6 +++++ lib/public/Files/Storage/IStorage.php | 19 ++++++++++--- .../Mount/ObjectHomeMountProviderTest.php | 4 +++ .../ObjectStore/FailDeleteObjectStore.php | 4 +++ .../ObjectStore/FailWriteObjectStore.php | 4 +++ 19 files changed, 162 insertions(+), 25 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index d2a71eb3e7b..e6f92cd5319 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -18,6 +18,7 @@ use OCA\DAV\Connector\Sabre\Exception\FileLocked; use OCA\DAV\Connector\Sabre\Exception\Forbidden as DAVForbiddenException; use OCA\DAV\Connector\Sabre\Exception\UnsupportedMediaType; use OCP\App\IAppManager; +use OCP\Constants; use OCP\Encryption\Exceptions\GenericEncryptionException; use OCP\Files; use OCP\Files\EntityTooLargeException; @@ -539,18 +540,24 @@ class File extends Node implements IFile { } /** - * @return array|bool + * @throws NotFoundException + * @throws NotPermittedException */ - public function getDirectDownload() { + public function getDirectDownload(): array|false { if (Server::get(IAppManager::class)->isEnabledForUser('encryption')) { - return []; + return false; } - [$storage, $internalPath] = $this->fileView->resolvePath($this->path); - if (is_null($storage)) { - return []; + $node = $this->getNode(); + $storage = $node->getStorage(); + if (!$storage) { + return false; + } + + if (!($node->getPermissions() & Constants::PERMISSION_READ)) { + return false; } - return $storage->getDirectDownload($internalPath); + return $storage->getDirectDownloadById((string)$node->getId()); } /** diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php index eed57ee0be8..7b2f144dfa1 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -50,6 +50,7 @@ class FilesPlugin extends ServerPlugin { public const OCM_SHARE_PERMISSIONS_PROPERTYNAME = '{http://open-cloud-mesh.org/ns}share-permissions'; public const SHARE_ATTRIBUTES_PROPERTYNAME = '{http://nextcloud.org/ns}share-attributes'; public const DOWNLOADURL_PROPERTYNAME = '{http://owncloud.org/ns}downloadURL'; + public const DOWNLOADURL_EXPIRATION_PROPERTYNAME = '{http://nextcloud.org/ns}download-url-expiration'; public const SIZE_PROPERTYNAME = '{http://owncloud.org/ns}size'; public const GETETAG_PROPERTYNAME = '{DAV:}getetag'; public const LASTMODIFIED_PROPERTYNAME = '{DAV:}lastmodified'; @@ -120,6 +121,7 @@ class FilesPlugin extends ServerPlugin { $server->protectedProperties[] = self::SHARE_ATTRIBUTES_PROPERTYNAME; $server->protectedProperties[] = self::SIZE_PROPERTYNAME; $server->protectedProperties[] = self::DOWNLOADURL_PROPERTYNAME; + $server->protectedProperties[] = self::DOWNLOADURL_EXPIRATION_PROPERTYNAME; $server->protectedProperties[] = self::OWNER_ID_PROPERTYNAME; $server->protectedProperties[] = self::OWNER_DISPLAY_NAME_PROPERTYNAME; $server->protectedProperties[] = self::CHECKSUMS_PROPERTYNAME; @@ -471,19 +473,30 @@ class FilesPlugin extends ServerPlugin { } if ($node instanceof File) { - $propFind->handle(self::DOWNLOADURL_PROPERTYNAME, function () use ($node) { + $requestProperties = $propFind->getRequestedProperties(); + + if (in_array(self::DOWNLOADURL_PROPERTYNAME, $requestProperties, true) + || in_array(self::DOWNLOADURL_EXPIRATION_PROPERTYNAME, $requestProperties, true)) { try { $directDownloadUrl = $node->getDirectDownload(); - if (isset($directDownloadUrl['url'])) { + } catch (StorageNotAvailableException|ForbiddenException) { + $directDownloadUrl = null; + } + + $propFind->handle(self::DOWNLOADURL_PROPERTYNAME, function () use ($node, $directDownloadUrl) { + if ($directDownloadUrl && isset($directDownloadUrl['url'])) { return $directDownloadUrl['url']; } - } catch (StorageNotAvailableException $e) { return false; - } catch (ForbiddenException $e) { + }); + + $propFind->handle(self::DOWNLOADURL_EXPIRATION_PROPERTYNAME, function () use ($node, $directDownloadUrl) { + if ($directDownloadUrl && isset($directDownloadUrl['expiration'])) { + return $directDownloadUrl['expiration']; + } return false; - } - return false; - }); + }); + } $propFind->handle(self::CHECKSUMS_PROPERTYNAME, function () use ($node) { $checksum = $node->getChecksum(); diff --git a/apps/files_sharing/lib/SharedStorage.php b/apps/files_sharing/lib/SharedStorage.php index eb63f93b07d..aeaaa54f370 100644 --- a/apps/files_sharing/lib/SharedStorage.php +++ b/apps/files_sharing/lib/SharedStorage.php @@ -40,6 +40,7 @@ use OCP\Lock\ILockingProvider; use OCP\Server; use OCP\Share\IShare; use OCP\Util; +use Override; use Psr\Log\LoggerInterface; /** @@ -558,8 +559,15 @@ class SharedStorage extends Jail implements LegacyISharedStorage, ISharedStorage return parent::getUnjailedPath($path); } + #[Override] public function getDirectDownload(string $path): array|false { // disable direct download for shares - return []; + return false; + } + + #[Override] + public function getDirectDownloadById(string $fileId): array|false { + // disable direct download for shares + return false; } } diff --git a/lib/private/Files/ObjectStore/Azure.php b/lib/private/Files/ObjectStore/Azure.php index 2729bb3c037..4e7b0c51678 100644 --- a/lib/private/Files/ObjectStore/Azure.php +++ b/lib/private/Files/ObjectStore/Azure.php @@ -117,4 +117,8 @@ class Azure implements IObjectStore { public function copyObject($from, $to) { $this->getBlobClient()->copyBlob($this->containerName, $to, $this->containerName, $from); } + + public function preSignedUrl(string $urn, \DateTimeInterface $expiration): ?string { + return null; + } } diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index 8ce992e93c0..c151e661939 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -29,6 +29,7 @@ use OCP\Files\Storage\IChunkedFileWrite; use OCP\Files\Storage\IStorage; use OCP\IDBConnection; use OCP\Server; +use Override; use Psr\Log\LoggerInterface; class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFileWrite { @@ -844,4 +845,22 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil return $available; } + + #[Override] + public function getDirectDownloadById(string $fileId): array|false { + $expiration = new \DateTimeImmutable('+60 minutes'); + $url = $this->objectStore->preSignedUrl($this->getURN((int)$fileId), $expiration); + return $url ? ['url' => $url, 'expiration' => $expiration->getTimestamp()] : false; + } + + #[Override] + public function getDirectDownload(string $path): array|false { + $path = $this->normalizePath($path); + $cacheEntry = $this->getCache()->get($path); + + if (!$cacheEntry || $cacheEntry->getMimeType() === FileInfo::MIMETYPE_FOLDER) { + return false; + } + return $this->getDirectDownloadById((string)$cacheEntry->getId()); + } } diff --git a/lib/private/Files/ObjectStore/S3ConnectionTrait.php b/lib/private/Files/ObjectStore/S3ConnectionTrait.php index 48fa8efdec3..360b92e7663 100644 --- a/lib/private/Files/ObjectStore/S3ConnectionTrait.php +++ b/lib/private/Files/ObjectStore/S3ConnectionTrait.php @@ -31,8 +31,8 @@ trait S3ConnectionTrait { protected bool $test; protected ?S3Client $connection = null; - private ?ICache $existingBucketsCache = null; + private bool $usePresignedUrl = false; protected function parseParams($params) { if (empty($params['bucket'])) { @@ -109,12 +109,15 @@ trait S3ConnectionTrait { ) ); + $this->usePresignedUrl = $this->params['use_presigned_url'] ?? false; + $options = [ 'version' => $this->params['version'] ?? 'latest', 'credentials' => $provider, 'endpoint' => $base_url, 'region' => $this->params['region'], 'use_path_style_endpoint' => isset($this->params['use_path_style']) ? $this->params['use_path_style'] : false, + 'proxy' => isset($this->params['proxy']) ? $this->params['proxy'] : false, 'signature_provider' => \Aws\or_chain([self::class, 'legacySignatureProvider'], ClientResolver::_default_signature_provider()), 'csm' => false, 'use_arn_region' => false, @@ -291,4 +294,8 @@ trait S3ConnectionTrait { 'SSECustomerKeyMD5' => md5($rawKey, true) ]; } + + public function isUsePresignedUrl(): bool { + return $this->usePresignedUrl; + } } diff --git a/lib/private/Files/ObjectStore/S3ObjectTrait.php b/lib/private/Files/ObjectStore/S3ObjectTrait.php index f47a01dab18..91eff90babb 100644 --- a/lib/private/Files/ObjectStore/S3ObjectTrait.php +++ b/lib/private/Files/ObjectStore/S3ObjectTrait.php @@ -7,6 +7,7 @@ namespace OC\Files\ObjectStore; use Aws\Command; +use Aws\Exception\AwsException; use Aws\Exception\MultipartUploadException; use Aws\S3\Exception\S3MultipartUploadException; use Aws\S3\MultipartCopy; @@ -295,4 +296,23 @@ trait S3ObjectTrait { ], $options)); } } + + public function preSignedUrl(string $urn, \DateTimeInterface $expiration): ?string { + $command = $this->getConnection()->getCommand('GetObject', [ + 'Bucket' => $this->getBucket(), + 'Key' => $urn, + ]); + + if (!$this->isUsePresignedUrl()) { + return null; + } + + try { + return (string)$this->getConnection()->createPresignedRequest($command, $expiration, [ + 'signPayload' => true, + ])->getUri(); + } catch (AwsException) { + return null; + } + } } diff --git a/lib/private/Files/ObjectStore/StorageObjectStore.php b/lib/private/Files/ObjectStore/StorageObjectStore.php index 888602a62e4..c20efb346a1 100644 --- a/lib/private/Files/ObjectStore/StorageObjectStore.php +++ b/lib/private/Files/ObjectStore/StorageObjectStore.php @@ -74,4 +74,8 @@ class StorageObjectStore implements IObjectStore { public function copyObject($from, $to) { $this->storage->copy($from, $to); } + + public function preSignedUrl(string $urn, \DateTimeInterface $expiration): ?string { + return null; + } } diff --git a/lib/private/Files/ObjectStore/Swift.php b/lib/private/Files/ObjectStore/Swift.php index aa8b3bb34ec..9006f29160d 100644 --- a/lib/private/Files/ObjectStore/Swift.php +++ b/lib/private/Files/ObjectStore/Swift.php @@ -134,4 +134,7 @@ class Swift implements IObjectStore { 'destination' => $this->getContainer()->name . '/' . $to ]); } + public function preSignedUrl(string $urn, \DateTimeInterface $expiration): ?string { + return null; + } } diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index e197c3c9d26..1a2aeb0e021 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -38,6 +38,7 @@ use OCP\IConfig; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; use OCP\Server; +use Override; use Psr\Log\LoggerInterface; /** @@ -445,13 +446,14 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage, return is_a($this, $class); } - /** - * A custom storage implementation can return an url for direct download of a give file. - * - * For now the returned array can hold the parameter url - in future more attributes might follow. - */ + #[Override] public function getDirectDownload(string $path): array|false { - return []; + return false; + } + + #[Override] + public function getDirectDownloadById(string $fileId): array|false { + return false; } public function verifyPath(string $path, string $fileName): void { diff --git a/lib/private/Files/Storage/FailedStorage.php b/lib/private/Files/Storage/FailedStorage.php index a8288de48d0..1b1123921aa 100644 --- a/lib/private/Files/Storage/FailedStorage.php +++ b/lib/private/Files/Storage/FailedStorage.php @@ -154,6 +154,10 @@ class FailedStorage extends Common { throw new StorageNotAvailableException($this->e->getMessage(), $this->e->getCode(), $this->e); } + public function getDirectDownloadById(string $fileId): never { + throw new StorageNotAvailableException($this->e->getMessage(), $this->e->getCode(), $this->e); + } + public function verifyPath(string $path, string $fileName): void { } diff --git a/lib/private/Files/Storage/Wrapper/Availability.php b/lib/private/Files/Storage/Wrapper/Availability.php index dc2f9a2e6ac..9d184e6a660 100644 --- a/lib/private/Files/Storage/Wrapper/Availability.php +++ b/lib/private/Files/Storage/Wrapper/Availability.php @@ -228,6 +228,10 @@ class Availability extends Wrapper { return $this->handleAvailability('getDirectDownload', $path); } + public function getDirectDownloadById(string $fileId): array|false { + return $this->handleAvailability('getDirectDownloadById', $fileId); + } + public function copyFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath): bool { return $this->handleAvailability('copyFromStorage', $sourceStorage, $sourceInternalPath, $targetInternalPath); } diff --git a/lib/private/Files/Storage/Wrapper/Wrapper.php b/lib/private/Files/Storage/Wrapper/Wrapper.php index 8d9381a87f7..d5454050fcb 100644 --- a/lib/private/Files/Storage/Wrapper/Wrapper.php +++ b/lib/private/Files/Storage/Wrapper/Wrapper.php @@ -20,6 +20,7 @@ use OCP\Files\Storage\IStorage; use OCP\Files\Storage\IWriteStreamStorage; use OCP\Lock\ILockingProvider; use OCP\Server; +use Override; use Psr\Log\LoggerInterface; class Wrapper implements \OC\Files\Storage\Storage, ILockingStorage, IWriteStreamStorage { @@ -258,10 +259,16 @@ class Wrapper implements \OC\Files\Storage\Storage, ILockingStorage, IWriteStrea return call_user_func_array([$this->getWrapperStorage(), $method], $args); } + #[Override] public function getDirectDownload(string $path): array|false { return $this->getWrapperStorage()->getDirectDownload($path); } + #[Override] + public function getDirectDownloadById(string $fileId): array|false { + return $this->getWrapperStorage()->getDirectDownloadById($fileId); + } + public function getAvailability(): array { return $this->getWrapperStorage()->getAvailability(); } diff --git a/lib/private/Lockdown/Filesystem/NullStorage.php b/lib/private/Lockdown/Filesystem/NullStorage.php index fd952fae637..d10763287c8 100644 --- a/lib/private/Lockdown/Filesystem/NullStorage.php +++ b/lib/private/Lockdown/Filesystem/NullStorage.php @@ -145,6 +145,10 @@ class NullStorage extends Common { return false; } + public function getDirectDownloadById(string $fileId): array|false { + return false; + } + public function copyFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath, bool $preserveMtime = false): never { throw new \OC\ForbiddenException('This request is not allowed to access the filesystem'); } diff --git a/lib/public/Files/ObjectStore/IObjectStore.php b/lib/public/Files/ObjectStore/IObjectStore.php index 35099ef8ba8..2b33ed485ad 100644 --- a/lib/public/Files/ObjectStore/IObjectStore.php +++ b/lib/public/Files/ObjectStore/IObjectStore.php @@ -63,4 +63,10 @@ interface IObjectStore { * @since 21.0.0 */ public function copyObject($from, $to); + + /** + * Get pre signed url for an object + * @since 33.0.0 + */ + public function preSignedUrl(string $urn, \DateTimeInterface $expiration): ?string; } diff --git a/lib/public/Files/Storage/IStorage.php b/lib/public/Files/Storage/IStorage.php index 5f6c8a0e8a0..f09f899d4f2 100644 --- a/lib/public/Files/Storage/IStorage.php +++ b/lib/public/Files/Storage/IStorage.php @@ -302,15 +302,28 @@ interface IStorage { public function instanceOfStorage(string $class); /** - * A custom storage implementation can return an url for direct download of a give file. + * A custom storage implementation can return a url for direct download of a give file. * - * For now the returned array can hold the parameter url - in future more attributes might follow. + * For now the returned array can hold the parameter url and expiration - in future more attributes might follow. * - * @return array|false + * @param string $path Either the path or the fileId + * @return array{url: ?string, expiration: ?int}|false * @since 9.0.0 + * @deprecated Use IStorage::getDirectDownloadById instead. */ public function getDirectDownload(string $path); + /** + * A custom storage implementation can return a url for direct download of a give file. + * + * For now the returned array can hold the parameter url and expiration - in future more attributes might follow. + * + * @param string $fileId The fileId of the file. + * @return array{url: ?string, expiration: ?int}|false + * @since 33.0.0 + */ + public function getDirectDownloadById(string $fileId): array|false; + /** * @return void * @throws InvalidPathException diff --git a/tests/lib/Files/Mount/ObjectHomeMountProviderTest.php b/tests/lib/Files/Mount/ObjectHomeMountProviderTest.php index ae0a53f2cc0..a9200732a18 100644 --- a/tests/lib/Files/Mount/ObjectHomeMountProviderTest.php +++ b/tests/lib/Files/Mount/ObjectHomeMountProviderTest.php @@ -268,4 +268,8 @@ class FakeObjectStore implements IObjectStore { public function copyObject($from, $to) { } + + public function preSignedUrl(string $urn, \DateTimeInterface $expiration): ?string { + return null; + } } diff --git a/tests/lib/Files/ObjectStore/FailDeleteObjectStore.php b/tests/lib/Files/ObjectStore/FailDeleteObjectStore.php index 767125d42aa..b842ec6dfd6 100644 --- a/tests/lib/Files/ObjectStore/FailDeleteObjectStore.php +++ b/tests/lib/Files/ObjectStore/FailDeleteObjectStore.php @@ -39,4 +39,8 @@ class FailDeleteObjectStore implements IObjectStore { public function copyObject($from, $to) { $this->objectStore->copyObject($from, $to); } + + public function preSignedUrl(string $urn, \DateTimeInterface $expiration): ?string { + return null; + } } diff --git a/tests/lib/Files/ObjectStore/FailWriteObjectStore.php b/tests/lib/Files/ObjectStore/FailWriteObjectStore.php index 924bbdada4f..a3cd1e8c92a 100644 --- a/tests/lib/Files/ObjectStore/FailWriteObjectStore.php +++ b/tests/lib/Files/ObjectStore/FailWriteObjectStore.php @@ -40,4 +40,8 @@ class FailWriteObjectStore implements IObjectStore { public function copyObject($from, $to) { $this->objectStore->copyObject($from, $to); } + + public function preSignedUrl(string $urn, \DateTimeInterface $expiration): ?string { + return null; + } }