From b530fd66cd4f91ef04b8df4df43789077783a6d2 Mon Sep 17 00:00:00 2001 From: Torsten Dittmann Date: Fri, 10 Dec 2021 17:48:54 +0100 Subject: [PATCH] fix(storage): add same permissions check to files uploaded --- app/controllers/api/storage.php | 43 ++++++- .../Storage/StorageCustomClientTest.php | 111 +++++++++++++++++- 2 files changed, 152 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/storage.php b/app/controllers/api/storage.php index 732e061c3f..0522826af4 100644 --- a/app/controllers/api/storage.php +++ b/app/controllers/api/storage.php @@ -1,5 +1,6 @@ isEmpty()) ? ['user:'.$user->getId()] : $read ?? []; // By default set read permissions for user + $write = (is_null($write) && !$user->isEmpty()) ? ['user:'.$user->getId()] : $write ?? []; + + // Users can only add their roles to files, API keys and Admin users can add any + $roles = \array_fill_keys(Authorization::getRoles(), true); // Auth::isAppUser expects roles to be keys, not values of assoc array + + if (!Auth::isAppUser($roles) && !Auth::isPrivilegedUser($roles)) { + foreach ($read as $role) { + if (!Authorization::isRole($role)) { + throw new Exception('Read permissions must be one of: ('.\implode(', ', array_keys($roles)).')', 400); + } + } + foreach ($write as $role) { + if (!Authorization::isRole($role)) { + throw new Exception('Write permissions must be one of: ('.\implode(', ', array_keys($roles)).')', 400); + } + } + } + $file = $request->getFiles('file'); /* @@ -563,13 +583,34 @@ App::put('/v1/storage/files/:fileId') ->param('write', [], new ArrayList(new Text(64)), 'An array of strings with write permissions. By default no user is granted with any write permissions. [learn more about permissions](/docs/permissions) and get a full list of available permissions.') ->inject('response') ->inject('dbForInternal') + ->inject('user') ->inject('audits') ->inject('usage') - ->action(function ($fileId, $read, $write, $response, $dbForInternal, $audits, $usage) { + ->action(function ($fileId, $read, $write, $response, $dbForInternal, $user, $audits, $usage) { /** @var Appwrite\Utopia\Response $response */ /** @var Utopia\Database\Database $dbForInternal */ + /** @var Utopia\Database\Document $user */ /** @var Appwrite\Event\Event $audits */ + $read = (is_null($read) && !$user->isEmpty()) ? ['user:'.$user->getId()] : $read ?? []; // By default set read permissions for user + $write = (is_null($write) && !$user->isEmpty()) ? ['user:'.$user->getId()] : $write ?? []; + + // Users can only add their roles to files, API keys and Admin users can add any + $roles = \array_fill_keys(Authorization::getRoles(), true); // Auth::isAppUser expects roles to be keys, not values of assoc array + + if (!Auth::isAppUser($roles) && !Auth::isPrivilegedUser($roles)) { + foreach ($read as $role) { + if (!Authorization::isRole($role)) { + throw new Exception('Read permissions must be one of: ('.\implode(', ', array_keys($roles)).')', 400); + } + } + foreach ($write as $role) { + if (!Authorization::isRole($role)) { + throw new Exception('Write permissions must be one of: ('.\implode(', ', array_keys($roles)).')', 400); + } + } + } + $file = $dbForInternal->getDocument('files', $fileId); if (empty($file->getId())) { diff --git a/tests/e2e/Services/Storage/StorageCustomClientTest.php b/tests/e2e/Services/Storage/StorageCustomClientTest.php index 2a0ead59f6..a0dee63156 100644 --- a/tests/e2e/Services/Storage/StorageCustomClientTest.php +++ b/tests/e2e/Services/Storage/StorageCustomClientTest.php @@ -3,6 +3,9 @@ namespace Tests\E2E\Services\Storage; use CURLFile; +use Exception; +use SebastianBergmann\RecursionContext\InvalidArgumentException; +use PHPUnit\Framework\ExpectationFailedException; use Tests\E2E\Client; use Tests\E2E\Scopes\Scope; use Tests\E2E\Scopes\ProjectCustom; @@ -14,7 +17,7 @@ class StorageCustomClientTest extends Scope use ProjectCustom; use SideClient; - public function testCreateFileDefaultPermissions():void + public function testCreateFileDefaultPermissions(): array { /** * Test for SUCCESS @@ -36,5 +39,111 @@ class StorageCustomClientTest extends Scope $this->assertEquals('permissions.png', $file['body']['name']); $this->assertEquals('image/png', $file['body']['mimeType']); $this->assertEquals(47218, $file['body']['sizeOriginal']); + + return $file['body']; + } + + public function testCreateFileAbusePermissions(): void + { + /** + * Test for FAILURE + */ + $file = $this->client->call(Client::METHOD_POST, '/storage/files', array_merge([ + 'content-type' => 'multipart/form-data', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'fileId' => 'unique()', + 'file' => new CURLFile(realpath(__DIR__ . '/../../../resources/logo.png'), 'image/png', 'permissions.png'), + 'folderId' => 'xyz', + 'read' => ['user:notme'] + ]); + + $this->assertEquals($file['headers']['status-code'], 400); + $this->assertStringStartsWith('Read permissions must be one of:', $file['body']['message']); + $this->assertStringContainsString('role:all', $file['body']['message']); + $this->assertStringContainsString('role:member', $file['body']['message']); + $this->assertStringContainsString('user:'.$this->getUser()['$id'], $file['body']['message']); + + $file = $this->client->call(Client::METHOD_POST, '/storage/files', array_merge([ + 'content-type' => 'multipart/form-data', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'fileId' => 'unique()', + 'file' => new CURLFile(realpath(__DIR__ . '/../../../resources/logo.png'), 'image/png', 'permissions.png'), + 'folderId' => 'xyz', + 'write' => ['user:notme'] + ]); + + $this->assertEquals($file['headers']['status-code'], 400); + $this->assertStringStartsWith('Write permissions must be one of:', $file['body']['message']); + $this->assertStringContainsString('role:all', $file['body']['message']); + $this->assertStringContainsString('role:member', $file['body']['message']); + $this->assertStringContainsString('user:'.$this->getUser()['$id'], $file['body']['message']); + + $file = $this->client->call(Client::METHOD_POST, '/storage/files', array_merge([ + 'content-type' => 'multipart/form-data', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'fileId' => 'unique()', + 'file' => new CURLFile(realpath(__DIR__ . '/../../../resources/logo.png'), 'image/png', 'permissions.png'), + 'folderId' => 'xyz', + 'read' => ['user:notme'], + 'write' => ['user:notme'] + ]); + + $this->assertEquals($file['headers']['status-code'], 400); + $this->assertStringStartsWith('Read permissions must be one of:', $file['body']['message']); + $this->assertStringContainsString('role:all', $file['body']['message']); + $this->assertStringContainsString('role:member', $file['body']['message']); + $this->assertStringContainsString('user:'.$this->getUser()['$id'], $file['body']['message']); + } + + /** + * @depends testCreateFileDefaultPermissions + */ + public function testUpdateFileAbusePermissions(array $data): void + { + /** + * Test for FAILURE + */ + $file = $this->client->call(Client::METHOD_PUT, '/storage/files/' . $data['$id'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'read' => ['user:notme'] + ]); + + $this->assertEquals($file['headers']['status-code'], 400); + $this->assertStringStartsWith('Read permissions must be one of:', $file['body']['message']); + $this->assertStringContainsString('role:all', $file['body']['message']); + $this->assertStringContainsString('role:member', $file['body']['message']); + $this->assertStringContainsString('user:'.$this->getUser()['$id'], $file['body']['message']); + + $file = $this->client->call(Client::METHOD_PUT, '/storage/files/' . $data['$id'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'write' => ['user:notme'] + ]); + + $this->assertEquals($file['headers']['status-code'], 400); + $this->assertStringStartsWith('Write permissions must be one of:', $file['body']['message']); + $this->assertStringContainsString('role:all', $file['body']['message']); + $this->assertStringContainsString('role:member', $file['body']['message']); + $this->assertStringContainsString('user:'.$this->getUser()['$id'], $file['body']['message']); + + $file = $this->client->call(Client::METHOD_PUT, '/storage/files/' . $data['$id'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'read' => ['user:notme'], + 'write' => ['user:notme'] + ]); + + $this->assertEquals($file['headers']['status-code'], 400); + $this->assertStringStartsWith('Read permissions must be one of:', $file['body']['message']); + $this->assertStringContainsString('role:all', $file['body']['message']); + $this->assertStringContainsString('role:member', $file['body']['message']); + $this->assertStringContainsString('user:'.$this->getUser()['$id'], $file['body']['message']); } } \ No newline at end of file