From e6e126dff453090fe180e98a15efeffd9b609d70 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 25 Aug 2022 23:30:26 +1200 Subject: [PATCH] Catch AuthorzationExceptions from database to throw 401 instead of 500 --- app/controllers/api/databases.php | 15 +++++++-- app/controllers/api/storage.php | 53 +++++++++++++++++++++++++++---- 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/app/controllers/api/databases.php b/app/controllers/api/databases.php index 81087037c0..7643172147 100644 --- a/app/controllers/api/databases.php +++ b/app/controllers/api/databases.php @@ -2067,7 +2067,11 @@ App::get('/v1/databases/:databaseId/collections/:collectionId/documents/:documen } if ($documentSecurity && !$valid) { - $document = $dbForProject->getDocument('database_' . $database->getInternalId() . '_collection_' . $collection->getInternalId(), $documentId); + try { + $document = $dbForProject->getDocument('database_' . $database->getInternalId() . '_collection_' . $collection->getInternalId(), $documentId); + } catch (AuthorizationException) { + throw new Exception(Exception::USER_UNAUTHORIZED); + } } else { $document = Authorization::skip(fn () => $dbForProject->getDocument('database_' . $database->getInternalId() . '_collection_' . $collection->getInternalId(), $documentId)); } @@ -2081,7 +2085,6 @@ App::get('/v1/databases/:databaseId/collections/:collectionId/documents/:documen */ $document->setAttribute('$collection', $collectionId); - $response->dynamic($document, Response::MODEL_DOCUMENT); }); @@ -2292,6 +2295,8 @@ App::patch('/v1/databases/:databaseId/collections/:collectionId/documents/:docum * Reset $collection attribute to remove prefix. */ $document->setAttribute('$collection', $collectionId); + } catch (AuthorizationException) { + throw new Exception(Exception::USER_UNAUTHORIZED); } catch (DuplicateException) { throw new Exception(Exception::DOCUMENT_ALREADY_EXISTS); } catch (StructureException $exception) { @@ -2363,7 +2368,11 @@ App::delete('/v1/databases/:databaseId/collections/:collectionId/documents/:docu } if ($documentSecurity && !$valid) { - $dbForProject->deleteDocument('database_' . $database->getInternalId() . '_collection_' . $collection->getInternalId(), $documentId); + try { + $dbForProject->deleteDocument('database_' . $database->getInternalId() . '_collection_' . $collection->getInternalId(), $documentId); + } catch (AuthorizationException) { + throw new Exception(Exception::USER_UNAUTHORIZED); + } } else { Authorization::skip(fn() => $dbForProject->deleteDocument('database_' . $database->getInternalId() . '_collection_' . $collection->getInternalId(), $documentId)); } diff --git a/app/controllers/api/storage.php b/app/controllers/api/storage.php index bd8a55705c..cdd1739b7d 100644 --- a/app/controllers/api/storage.php +++ b/app/controllers/api/storage.php @@ -14,6 +14,7 @@ use Utopia\Database\Database; use Utopia\Database\Document; use Utopia\Database\DateTime; use Utopia\Database\Exception\Duplicate; +use Utopia\Database\Exception\Authorization as AuthorizationException; use Utopia\Database\Exception\Duplicate as DuplicateException; use Utopia\Database\Exception\Structure as StructureException; use Utopia\Database\ID; @@ -571,6 +572,8 @@ App::post('/v1/storage/buckets/:bucketId/files') $file = $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file); } + } catch (AuthorizationException) { + throw new Exception(Exception::USER_UNAUTHORIZED); } catch (StructureException $exception) { throw new Exception(Exception::DOCUMENT_INVALID_STRUCTURE, $exception->getMessage()); } catch (DuplicateException) { @@ -605,6 +608,8 @@ App::post('/v1/storage/buckets/:bucketId/files') $file = $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file); } + } catch (AuthorizationException) { + throw new Exception(Exception::USER_UNAUTHORIZED); } catch (StructureException $exception) { throw new Exception(Exception::DOCUMENT_INVALID_STRUCTURE, $exception->getMessage()); } catch (DuplicateException) { @@ -741,7 +746,11 @@ App::get('/v1/storage/buckets/:bucketId/files/:fileId') } if ($fileSecurity && !$valid) { - $file = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId); + try { + $file = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId); + } catch (AuthorizationException) { + throw new Exception(Exception::USER_UNAUTHORIZED); + } } else { $file = Authorization::skip(fn() => $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId)); } @@ -820,7 +829,11 @@ App::get('/v1/storage/buckets/:bucketId/files/:fileId/preview') $key = \md5($fileId . $width . $height . $gravity . $quality . $borderWidth . $borderColor . $borderRadius . $opacity . $rotation . $background . $output); if ($fileSecurity && !$valid) { - $file = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId); + try { + $file = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId); + } catch (AuthorizationException) { + throw new Exception(Exception::USER_UNAUTHORIZED); + } } else { $file = Authorization::skip(fn() => $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId)); } @@ -954,7 +967,11 @@ App::get('/v1/storage/buckets/:bucketId/files/:fileId/download') } if ($fileSecurity && !$valid) { - $file = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId); + try { + $file = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId); + } catch (AuthorizationException) { + throw new Exception(Exception::USER_UNAUTHORIZED); + } } else { $file = Authorization::skip(fn() => $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId)); } @@ -1085,7 +1102,11 @@ App::get('/v1/storage/buckets/:bucketId/files/:fileId/view') } if ($fileSecurity && !$valid) { - $file = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId); + try { + $file = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId); + } catch (AuthorizationException) { + throw new Exception(Exception::USER_UNAUTHORIZED); + } } else { $file = Authorization::skip(fn() => $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $fileId)); } @@ -1270,7 +1291,15 @@ App::put('/v1/storage/buckets/:bucketId/files/:fileId') $file->setAttribute('$permissions', $permissions); - $file = $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file); + if ($fileSecurity && !$valid) { + try { + $file = $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file); + } catch (AuthorizationException) { + throw new Exception(Exception::USER_UNAUTHORIZED); + } + } else { + $file = Authorization::skip(fn() => $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file)); + } $events ->setParam('bucketId', $bucket->getId()) @@ -1325,6 +1354,11 @@ App::delete('/v1/storage/buckets/:bucketId/files/:fileId') throw new Exception(Exception::STORAGE_FILE_NOT_FOUND); } + // Make sure we don't delete the file before the document permission check occurs + if ($fileSecurity && !$valid && !$validator->isValid($file->getDelete())) { + throw new Exception(Exception::USER_UNAUTHORIZED); + } + $deviceDeleted = false; if ($file->getAttribute('chunksTotal') !== $file->getAttribute('chunksUploaded')) { $deviceDeleted = $deviceFiles->abort( @@ -1341,8 +1375,13 @@ App::delete('/v1/storage/buckets/:bucketId/files/:fileId') ->setResource('file/' . $fileId) ; - if ($fileSecurity && !$valid) { - $deleted = $dbForProject->deleteDocument('bucket_' . $bucket->getInternalId(), $fileId); + // Don't need to check valid here because we already ensured validity + if ($fileSecurity) { + try { + $deleted = $dbForProject->deleteDocument('bucket_' . $bucket->getInternalId(), $fileId); + } catch (AuthorizationException) { + throw new Exception(Exception::USER_UNAUTHORIZED); + } } else { $deleted = Authorization::skip(fn() => $dbForProject->deleteDocument('bucket_' . $bucket->getInternalId(), $fileId)); }