From 57f6b01b0b4f0fe493a379fa5330c03abf289afa Mon Sep 17 00:00:00 2001 From: Damodar Lohani Date: Tue, 2 Jan 2024 08:28:27 +0000 Subject: [PATCH 1/4] fix permission issue with chunk upload --- app/controllers/api/storage.php | 39 ++++++++++++++++++---- tests/e2e/Services/Storage/StorageBase.php | 9 ++--- 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/app/controllers/api/storage.php b/app/controllers/api/storage.php index 15746786726..5577e5e2fa1 100644 --- a/app/controllers/api/storage.php +++ b/app/controllers/api/storage.php @@ -332,7 +332,7 @@ ->label('audits.event', 'file.create') ->label('event', 'buckets.[bucketId].files.[fileId].create') ->label('audits.resource', 'file/{response.$id}') - ->label('abuse-key', 'ip:{ip},method:{method},url:{url},userId:{userId}') + ->label('abuse-key', 'ip:{ip},method:{method},url:{url},userId:{userId},chunkId:{chunkId}') ->label('abuse-limit', APP_LIMIT_WRITE_RATE_DEFAULT) ->label('abuse-time', APP_LIMIT_WRITE_RATE_PERIOD_DEFAULT) ->label('sdk.auth', [APP_AUTH_TYPE_SESSION, APP_AUTH_TYPE_KEY, APP_AUTH_TYPE_JWT]) @@ -531,19 +531,24 @@ $fileHash = $deviceFiles->getFileHash($path); // Get file hash before compression and encryption $data = ''; // Compression - $algorithm = $bucket->getAttribute('compression', COMPRESSION_TYPE_NONE); - if ($fileSize <= APP_STORAGE_READ_BUFFER && $algorithm != COMPRESSION_TYPE_NONE) { + $algorithm = $bucket->getAttribute('compression', Compression::NONE); + if ($fileSize <= APP_STORAGE_READ_BUFFER && $algorithm != Compression::NONE) { $data = $deviceFiles->read($path); switch ($algorithm) { - case COMPRESSION_TYPE_ZSTD: + case Compression::ZSTD: $compressor = new Zstd(); break; - case COMPRESSION_TYPE_GZIP: + case Compression::GZIP: default: $compressor = new GZIP(); break; } $data = $compressor->compress($data); + } else { + // reset the algorithm to none as we do not compress the file + // if file size exceedes the APP_STORAGE_READ_BUFFER + // regardless the bucket compression algoorithm + $algorithm = Compression::NONE; } if ($bucket->getAttribute('encryption', true) && $fileSize <= APP_STORAGE_READ_BUFFER) { @@ -615,7 +620,17 @@ ->setAttribute('metadata', $metadata) ->setAttribute('chunksUploaded', $chunksUploaded); - $file = $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file); + /** + * Validate create permission and skip authorization in updateDocument + * Without this, the file creation will fail when user doesn't have update permission + * However as with chunk upload even if we are updating, we are essentially creating a file + * adding it's new chunk so we validate create permission instead of update + */ + $validator = new Authorization(Database::PERMISSION_CREATE); + if (!$validator->isValid($bucket->getCreate())) { + throw new Exception(Exception::USER_UNAUTHORIZED); + } + $file = Authorization::skip(fn() => $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file)); } } catch (AuthorizationException) { throw new Exception(Exception::USER_UNAUTHORIZED); @@ -652,7 +667,17 @@ ->setAttribute('chunksUploaded', $chunksUploaded) ->setAttribute('metadata', $metadata); - $file = $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file); + /** + * Validate create permission and skip authorization in updateDocument + * Without this, the file creation will fail when user doesn't have update permission + * However as with chunk upload even if we are updating, we are essentially creating a file + * adding it's new chunk so we validate create permission instead of update + */ + $validator = new Authorization(Database::PERMISSION_CREATE); + if (!$validator->isValid($bucket->getCreate())) { + throw new Exception(Exception::USER_UNAUTHORIZED); + } + $file = Authorization::skip(fn() => $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file)); } } catch (AuthorizationException) { throw new Exception(Exception::USER_UNAUTHORIZED); diff --git a/tests/e2e/Services/Storage/StorageBase.php b/tests/e2e/Services/Storage/StorageBase.php index 36d2ee2365b..dba5aa8bcdd 100644 --- a/tests/e2e/Services/Storage/StorageBase.php +++ b/tests/e2e/Services/Storage/StorageBase.php @@ -74,10 +74,7 @@ public function testCreateBucketFile(): array 'name' => 'Test Bucket 2', 'fileSecurity' => true, 'permissions' => [ - Permission::read(Role::any()), Permission::create(Role::any()), - Permission::update(Role::any()), - Permission::delete(Role::any()), ], ]); $this->assertEquals(201, $bucket2['headers']['status-code']); @@ -110,9 +107,7 @@ public function testCreateBucketFile(): array 'fileId' => $fileId, 'file' => $curlFile, 'permissions' => [ - Permission::read(Role::any()), - Permission::update(Role::any()), - Permission::delete(Role::any()), + Permission::read(Role::any()) ], ]); $counter++; @@ -871,4 +866,4 @@ public function testDeleteBucketFile(array $data): array return $data; } -} +} \ No newline at end of file From 439bf3cc38cd40e4068270782bb969501b5f39e1 Mon Sep 17 00:00:00 2001 From: Damodar Lohani Date: Tue, 2 Jan 2024 08:29:25 +0000 Subject: [PATCH 2/4] upgrade composer --- composer.lock | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/composer.lock b/composer.lock index e54ae50f789..e22f7f66d80 100644 --- a/composer.lock +++ b/composer.lock @@ -2023,16 +2023,16 @@ }, { "name": "utopia-php/storage", - "version": "0.18.1", + "version": "0.18.3", "source": { "type": "git", "url": "https://github.com/utopia-php/storage.git", - "reference": "983e6dee137012f9f57f126d3c79aab54e4e8824" + "reference": "faa0279519ac14f3501e8b138e0865ad9d12bff6" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/utopia-php/storage/zipball/983e6dee137012f9f57f126d3c79aab54e4e8824", - "reference": "983e6dee137012f9f57f126d3c79aab54e4e8824", + "url": "https://api.github.com/repos/utopia-php/storage/zipball/faa0279519ac14f3501e8b138e0865ad9d12bff6", + "reference": "faa0279519ac14f3501e8b138e0865ad9d12bff6", "shasum": "" }, "require": { @@ -2072,9 +2072,9 @@ ], "support": { "issues": "https://github.com/utopia-php/storage/issues", - "source": "https://github.com/utopia-php/storage/tree/0.18.1" + "source": "https://github.com/utopia-php/storage/tree/0.18.3" }, - "time": "2023-10-24T14:44:19+00:00" + "time": "2023-12-31T11:45:12+00:00" }, { "name": "utopia-php/swoole", @@ -5128,5 +5128,5 @@ "platform-overrides": { "php": "8.0" }, - "plugin-api-version": "2.2.0" + "plugin-api-version": "2.6.0" } From 4da73fa195a3c4fc93607c377795458e8bcd9987 Mon Sep 17 00:00:00 2001 From: Damodar Lohani Date: Tue, 2 Jan 2024 08:38:18 +0000 Subject: [PATCH 3/4] large file handling improvement and code quality improvement --- app/controllers/api/storage.php | 45 ++++++++++--------- app/init.php | 4 -- src/Appwrite/Utopia/Response/Model/Bucket.php | 5 ++- 3 files changed, 28 insertions(+), 26 deletions(-) diff --git a/app/controllers/api/storage.php b/app/controllers/api/storage.php index 5577e5e2fa1..95b65d8ea32 100644 --- a/app/controllers/api/storage.php +++ b/app/controllers/api/storage.php @@ -44,6 +44,7 @@ use Utopia\Validator\WhiteList; use Utopia\DSN\DSN; use Utopia\Swoole\Request; +use Utopia\Storage\Compression\Compression; App::post('/v1/storage/buckets') ->desc('Create bucket') @@ -66,7 +67,7 @@ ->param('enabled', true, new Boolean(true), 'Is bucket enabled? When set to \'disabled\', users cannot access the files in this bucket but Server SDKs with and API key can still access the bucket. No files are lost when this is toggled.', true) ->param('maximumFileSize', (int) App::getEnv('_APP_STORAGE_LIMIT', 0), new Range(1, (int) App::getEnv('_APP_STORAGE_LIMIT', 0)), 'Maximum file size allowed in bytes. Maximum allowed value is ' . Storage::human(App::getEnv('_APP_STORAGE_LIMIT', 0), 0) . '.', true) ->param('allowedFileExtensions', [], new ArrayList(new Text(64), APP_LIMIT_ARRAY_PARAMS_SIZE), 'Allowed file extensions. Maximum of ' . APP_LIMIT_ARRAY_PARAMS_SIZE . ' extensions are allowed, each 64 characters long.', true) - ->param('compression', COMPRESSION_TYPE_NONE, new WhiteList([COMPRESSION_TYPE_NONE, COMPRESSION_TYPE_GZIP, COMPRESSION_TYPE_ZSTD]), 'Compression algorithm choosen for compression. Can be one of ' . COMPRESSION_TYPE_NONE . ', [' . COMPRESSION_TYPE_GZIP . '](https://en.wikipedia.org/wiki/Gzip), or [' . COMPRESSION_TYPE_ZSTD . '](https://en.wikipedia.org/wiki/Zstd), For file size above ' . Storage::human(APP_STORAGE_READ_BUFFER, 0) . ' compression is skipped even if it\'s enabled', true) + ->param('compression', Compression::NONE, new WhiteList([Compression::NONE, Compression::GZIP, Compression::ZSTD]), 'Compression algorithm choosen for compression. Can be one of ' . Compression::NONE . ', [' . Compression::GZIP . '](https://en.wikipedia.org/wiki/Gzip), or [' . Compression::ZSTD . '](https://en.wikipedia.org/wiki/Zstd), For file size above ' . Storage::human(APP_STORAGE_READ_BUFFER, 0) . ' compression is skipped even if it\'s enabled', true) ->param('encryption', true, new Boolean(true), 'Is encryption enabled? For file size above ' . Storage::human(APP_STORAGE_READ_BUFFER, 0) . ' encryption is skipped even if it\'s enabled', true) ->param('antivirus', true, new Boolean(true), 'Is virus scanning enabled? For file size above ' . Storage::human(APP_LIMIT_ANTIVIRUS, 0) . ' AntiVirus scanning is skipped even if it\'s enabled', true) ->inject('response') @@ -237,7 +238,7 @@ ->param('enabled', true, new Boolean(true), 'Is bucket enabled? When set to \'disabled\', users cannot access the files in this bucket but Server SDKs with and API key can still access the bucket. No files are lost when this is toggled.', true) ->param('maximumFileSize', null, new Range(1, (int) App::getEnv('_APP_STORAGE_LIMIT', 0)), 'Maximum file size allowed in bytes. Maximum allowed value is ' . Storage::human((int)App::getEnv('_APP_STORAGE_LIMIT', 0), 0) . '.', true) ->param('allowedFileExtensions', [], new ArrayList(new Text(64), APP_LIMIT_ARRAY_PARAMS_SIZE), 'Allowed file extensions. Maximum of ' . APP_LIMIT_ARRAY_PARAMS_SIZE . ' extensions are allowed, each 64 characters long.', true) - ->param('compression', COMPRESSION_TYPE_NONE, new WhiteList([COMPRESSION_TYPE_NONE, COMPRESSION_TYPE_GZIP, COMPRESSION_TYPE_ZSTD]), 'Compression algorithm choosen for compression. Can be one of ' . COMPRESSION_TYPE_NONE . ', [' . COMPRESSION_TYPE_GZIP . '](https://en.wikipedia.org/wiki/Gzip), or [' . COMPRESSION_TYPE_ZSTD . '](https://en.wikipedia.org/wiki/Zstd), For file size above ' . Storage::human(APP_STORAGE_READ_BUFFER, 0) . ' compression is skipped even if it\'s enabled', true) + ->param('compression', Compression::NONE, new WhiteList([Compression::NONE, Compression::GZIP, Compression::ZSTD]), 'Compression algorithm choosen for compression. Can be one of ' . Compression::NONE . ', [' . Compression::GZIP . '](https://en.wikipedia.org/wiki/Gzip), or [' . Compression::ZSTD . '](https://en.wikipedia.org/wiki/Zstd), For file size above ' . Storage::human(APP_STORAGE_READ_BUFFER, 0) . ' compression is skipped even if it\'s enabled', true) ->param('encryption', true, new Boolean(true), 'Is encryption enabled? For file size above ' . Storage::human(APP_STORAGE_READ_BUFFER, 0) . ' encryption is skipped even if it\'s enabled', true) ->param('antivirus', true, new Boolean(true), 'Is virus scanning enabled? For file size above ' . Storage::human(APP_LIMIT_ANTIVIRUS, 0) . ' AntiVirus scanning is skipped even if it\'s enabled', true) ->inject('response') @@ -884,14 +885,6 @@ throw new Exception(Exception::USER_UNAUTHORIZED); } - if ((\strpos($request->getAccept(), 'image/webp') === false) && ('webp' === $output)) { // Fallback webp to jpeg when no browser support - $output = 'jpg'; - } - - $inputs = Config::getParam('storage-inputs'); - $outputs = Config::getParam('storage-outputs'); - $fileLogos = Config::getParam('storage-logos'); - if ($fileSecurity && !$valid) { $file = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId); } else { @@ -902,9 +895,17 @@ throw new Exception(Exception::STORAGE_FILE_NOT_FOUND); } + if ((\strpos($request->getAccept(), 'image/webp') === false) && ('webp' === $output)) { // Fallback webp to jpeg when no browser support + $output = 'jpg'; + } + + $inputs = Config::getParam('storage-inputs'); + $outputs = Config::getParam('storage-outputs'); + $fileLogos = Config::getParam('storage-logos'); + $path = $file->getAttribute('path'); $type = \strtolower(\pathinfo($path, PATHINFO_EXTENSION)); - $algorithm = $file->getAttribute('algorithm', 'none'); + $algorithm = $file->getAttribute('algorithm', Compression::NONE); $cipher = $file->getAttribute('openSSLCipher'); $mime = $file->getAttribute('mimeType'); if (!\in_array($mime, $inputs) || $file->getAttribute('sizeActual') > (int) App::getEnv('_APP_STORAGE_PREVIEW_LIMIT', 20000000)) { @@ -915,7 +916,7 @@ $path = $fileLogos['default_image']; } - $algorithm = 'none'; + $algorithm = Compression::NONE; $cipher = null; $background = (empty($background)) ? 'eceff1' : $background; $type = \strtolower(\pathinfo($path, PATHINFO_EXTENSION)); @@ -953,11 +954,11 @@ } switch ($algorithm) { - case 'zstd': + case Compression::ZSTD: $compressor = new Zstd(); $source = $compressor->decompress($source); break; - case 'gzip': + case Compression::GZIP: $compressor = new GZIP(); $source = $compressor->decompress($source); break; @@ -1096,15 +1097,15 @@ ); } - switch ($file->getAttribute('algorithm', 'none')) { - case 'zstd': + switch ($file->getAttribute('algorithm', Compression::NONE)) { + case Compression::ZSTD: if (empty($source)) { $source = $deviceFiles->read($path); } $compressor = new Zstd(); $source = $compressor->decompress($source); break; - case 'gzip': + case Compression::GZIP: if (empty($source)) { $source = $deviceFiles->read($path); } @@ -1118,10 +1119,12 @@ $response->send(substr($source, $start, ($end - $start + 1))); } $response->send($source); + return; } if (!empty($rangeHeader)) { $response->send($deviceFiles->read($path, $start, ($end - $start + 1))); + return; } if ($size > APP_STORAGE_READ_BUFFER) { @@ -1245,15 +1248,15 @@ ); } - switch ($file->getAttribute('algorithm', 'none')) { - case 'zstd': + switch ($file->getAttribute('algorithm', Compression::NONE)) { + case Compression::ZSTD: if (empty($source)) { $source = $deviceFiles->read($path); } $compressor = new Zstd(); $source = $compressor->decompress($source); break; - case 'gzip': + case Compression::GZIP: if (empty($source)) { $source = $deviceFiles->read($path); } @@ -1267,10 +1270,12 @@ $response->send(substr($source, $start, ($end - $start + 1))); } $response->send($source); + return; } if (!empty($rangeHeader)) { $response->send($deviceFiles->read($path, $start, ($end - $start + 1))); + return; } $size = $deviceFiles->getFileSize($path); diff --git a/app/init.php b/app/init.php index 3dbb9ec3570..4d6c0bd8a03 100644 --- a/app/init.php +++ b/app/init.php @@ -174,10 +174,6 @@ const DELETE_TYPE_CACHE_BY_TIMESTAMP = 'cacheByTimeStamp'; const DELETE_TYPE_CACHE_BY_RESOURCE = 'cacheByResource'; const DELETE_TYPE_SCHEDULES = 'schedules'; -// Compression type -const COMPRESSION_TYPE_NONE = 'none'; -const COMPRESSION_TYPE_GZIP = 'gzip'; -const COMPRESSION_TYPE_ZSTD = 'zstd'; // Mail Types const MAIL_TYPE_VERIFICATION = 'verification'; const MAIL_TYPE_MAGIC_SESSION = 'magicSession'; diff --git a/src/Appwrite/Utopia/Response/Model/Bucket.php b/src/Appwrite/Utopia/Response/Model/Bucket.php index 3c5715efc2b..ac8563a41d9 100644 --- a/src/Appwrite/Utopia/Response/Model/Bucket.php +++ b/src/Appwrite/Utopia/Response/Model/Bucket.php @@ -4,6 +4,7 @@ use Appwrite\Utopia\Response; use Appwrite\Utopia\Response\Model; +use Utopia\Storage\Compression\Compression; class Bucket extends Model { @@ -68,9 +69,9 @@ public function __construct() ]) ->addRule('compression', [ 'type' => self::TYPE_STRING, - 'description' => 'Compression algorithm choosen for compression. Will be one of ' . COMPRESSION_TYPE_NONE . ', [' . COMPRESSION_TYPE_GZIP . '](https://en.wikipedia.org/wiki/Gzip), or [' . COMPRESSION_TYPE_ZSTD . '](https://en.wikipedia.org/wiki/Zstd).', + 'description' => 'Compression algorithm choosen for compression. Will be one of ' . Compression::NONE . ', [' . Compression::GZIP . '](https://en.wikipedia.org/wiki/Gzip), or [' . Compression::ZSTD . '](https://en.wikipedia.org/wiki/Zstd).', 'default' => '', - 'example' => 'gzip', + 'example' => Compression::GZIP, 'array' => false ]) ->addRule('encryption', [ From 5642cb0eb0aac3a09d94718a7a36a99bcd336687 Mon Sep 17 00:00:00 2001 From: Damodar Lohani Date: Tue, 2 Jan 2024 08:41:58 +0000 Subject: [PATCH 4/4] linter fixes --- tests/e2e/Services/Storage/StorageBase.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/Services/Storage/StorageBase.php b/tests/e2e/Services/Storage/StorageBase.php index dba5aa8bcdd..c4a15585ebb 100644 --- a/tests/e2e/Services/Storage/StorageBase.php +++ b/tests/e2e/Services/Storage/StorageBase.php @@ -866,4 +866,4 @@ public function testDeleteBucketFile(array $data): array return $data; } -} \ No newline at end of file +}