From 33970276b296eaeb0924af9d1e7fe33ebf3fdfad Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Mon, 15 Aug 2022 19:20:10 +1200 Subject: [PATCH] Self review --- app/config/roles.php | 2 +- app/controllers/api/databases.php | 8 +++++--- app/controllers/api/storage.php | 14 +++++--------- app/controllers/general.php | 4 ++-- src/Appwrite/Auth/Auth.php | 4 ++-- .../Databases/DatabasesPermissionsMemberTest.php | 2 +- tests/unit/Auth/AuthTest.php | 16 ++++++++-------- tests/unit/Messaging/MessagingChannelsTest.php | 2 +- 8 files changed, 25 insertions(+), 27 deletions(-) diff --git a/app/config/roles.php b/app/config/roles.php index 312d56403a..cd1afdfa4a 100644 --- a/app/config/roles.php +++ b/app/config/roles.php @@ -80,7 +80,7 @@ return [ 'label' => 'Owner', 'scopes' => \array_merge($member, $admins, []), ], - Auth::USER_ROLE_APP => [ + Auth::USER_ROLE_APPS => [ 'label' => 'Application', 'scopes' => ['health.read'], ], diff --git a/app/controllers/api/databases.php b/app/controllers/api/databases.php index 4539695b27..6d277cfdc7 100644 --- a/app/controllers/api/databases.php +++ b/app/controllers/api/databases.php @@ -611,7 +611,9 @@ App::get('/v1/databases/:databaseId/collections') throw new Exception("Collection '{$cursor}' for the 'cursor' value not found.", 400, Exception::GENERAL_CURSOR_NOT_FOUND); } - $queries[] = $cursorDirection === Database::CURSOR_AFTER ? Query::cursorAfter($cursorDocument) : Query::cursorBefore($cursorDocument); + $queries[] = $cursorDirection === Database::CURSOR_AFTER + ? Query::cursorAfter($cursorDocument) + : Query::cursorBefore($cursorDocument); } $usage @@ -2051,9 +2053,9 @@ App::get('/v1/databases/:databaseId/collections/:collectionId/documents') if (!empty($cursor)) { if ($documentSecurity) { - $cursorDocument = Authorization::skip(fn() => $dbForProject->getDocument('database_' . $database->getInternalId() . '_collection_' . $collection->getInternalId(), $cursor)); - } else { $cursorDocument = $dbForProject->getDocument('database_' . $database->getInternalId() . '_collection_' . $collection->getInternalId(), $cursor); + } else { + $cursorDocument = Authorization::skip(fn() => $dbForProject->getDocument('database_' . $database->getInternalId() . '_collection_' . $collection->getInternalId(), $cursor)); } if ($cursorDocument->isEmpty()) { throw new Exception("Document '{$cursor}' for the 'cursor' value not found.", 400, Exception::GENERAL_CURSOR_NOT_FOUND); diff --git a/app/controllers/api/storage.php b/app/controllers/api/storage.php index 8c8a9f35fb..3177f0c7f7 100644 --- a/app/controllers/api/storage.php +++ b/app/controllers/api/storage.php @@ -560,7 +560,7 @@ App::post('/v1/storage/buckets/:bucketId/files') 'metadata' => $metadata, ]); - $file = Authorization::skip(fn () => $dbForProject->createDocument('bucket_' . $bucket->getInternalId(), $doc)); + $file = $dbForProject->createDocument('bucket_' . $bucket->getInternalId(), $doc); } else { $file = $file ->setAttribute('$permissions', $permissions) @@ -575,7 +575,7 @@ App::post('/v1/storage/buckets/:bucketId/files') ->setAttribute('metadata', $metadata) ->setAttribute('chunksUploaded', $chunksUploaded); - $file = Authorization::skip(fn () => $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file)); + $file = $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file); } } catch (StructureException $exception) { throw new Exception($exception->getMessage(), 400, Exception::DOCUMENT_INVALID_STRUCTURE); @@ -613,13 +613,13 @@ App::post('/v1/storage/buckets/:bucketId/files') 'metadata' => $metadata, ]); - $file = Authorization::skip(fn () => $dbForProject->createDocument('bucket_' . $bucket->getInternalId(), $doc)); + $file = $dbForProject->createDocument('bucket_' . $bucket->getInternalId(), $doc); } else { $file = $file ->setAttribute('chunksUploaded', $chunksUploaded) ->setAttribute('metadata', $metadata); - $file = Authorization::skip(fn () => $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file)); + $file = $dbForProject->updateDocument('bucket_' . $bucket->getInternalId(), $fileId, $file); } } catch (StructureException $exception) { throw new Exception($exception->getMessage(), 400, Exception::DOCUMENT_INVALID_STRUCTURE); @@ -687,11 +687,7 @@ App::get('/v1/storage/buckets/:bucketId/files') $queries[] = Query::offset($offset); $queries[] = $orderType === Database::ORDER_ASC ? Query::orderAsc('') : Query::orderDesc(''); if (!empty($cursor)) { - if ($bucket->getAttribute('fileSecurity', false)) { - $cursorDocument = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $cursor); - } else { - $cursorDocument = Authorization::skip(fn () => $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $cursor)); - } + $cursorDocument = $dbForProject->getDocument('bucket_' . $bucket->getInternalId(), $cursor); if ($cursorDocument->isEmpty()) { throw new Exception("File '{$cursor}' for the 'cursor' value not found.", 400, Exception::GENERAL_CURSOR_NOT_FOUND); diff --git a/app/controllers/general.php b/app/controllers/general.php index f249e02193..dad1f80a1f 100644 --- a/app/controllers/general.php +++ b/app/controllers/general.php @@ -290,7 +290,7 @@ App::init() 'name' => $project->getAttribute('name', 'Untitled'), ]); - $role = Auth::USER_ROLE_APP; + $role = Auth::USER_ROLE_APPS; $scopes = \array_merge($roles[$role]['scopes'], $key->getAttribute('scopes', [])); $expire = $key->getAttribute('expire'); @@ -299,7 +299,7 @@ App::init() throw new AppwriteException('Project key expired', 401, AppwriteException:: PROJECT_KEY_EXPIRED); } - Authorization::setRole(Auth::USER_ROLE_APP); + Authorization::setRole(Auth::USER_ROLE_APPS); Authorization::setDefaultStatus(false); // Cancel security segmentation for API keys. } } diff --git a/src/Appwrite/Auth/Auth.php b/src/Appwrite/Auth/Auth.php index f9efc7ef09..7b1c1c0f01 100644 --- a/src/Appwrite/Auth/Auth.php +++ b/src/Appwrite/Auth/Auth.php @@ -17,7 +17,7 @@ class Auth public const USER_ROLE_ADMIN = 'admin'; public const USER_ROLE_DEVELOPER = 'developer'; public const USER_ROLE_OWNER = 'owner'; - public const USER_ROLE_APP = 'app'; + public const USER_ROLE_APPS = 'apps'; public const USER_ROLE_SYSTEM = 'system'; /** @@ -290,7 +290,7 @@ class Auth */ public static function isAppUser(array $roles): bool { - if (in_array(self::USER_ROLE_APP, $roles)) { + if (in_array(self::USER_ROLE_APPS, $roles)) { return true; } diff --git a/tests/e2e/Services/Databases/DatabasesPermissionsMemberTest.php b/tests/e2e/Services/Databases/DatabasesPermissionsMemberTest.php index de819a6f16..44a25fb545 100644 --- a/tests/e2e/Services/Databases/DatabasesPermissionsMemberTest.php +++ b/tests/e2e/Services/Databases/DatabasesPermissionsMemberTest.php @@ -145,7 +145,7 @@ class DatabasesPermissionsMemberTest extends Scope $this->assertEquals(201, $response['headers']['status-code']); /** - * Check role:all collection + * Check "any" collection */ $documents = $this->client->call(Client::METHOD_GET, '/databases/' . $databaseId . '/collections/' . $collections['public'] . '/documents', [ 'origin' => 'http://localhost', diff --git a/tests/unit/Auth/AuthTest.php b/tests/unit/Auth/AuthTest.php index 03a0a1510a..be445e62aa 100644 --- a/tests/unit/Auth/AuthTest.php +++ b/tests/unit/Auth/AuthTest.php @@ -18,7 +18,7 @@ class AuthTest extends TestCase public function tearDown(): void { Authorization::cleanRoles(); - Authorization::setRole('role:all'); + Authorization::setRole('any'); } public function testCookieName(): void @@ -176,11 +176,11 @@ class AuthTest extends TestCase $this->assertEquals(true, Auth::isPrivilegedUser([Auth::USER_ROLE_ADMIN])); $this->assertEquals(true, Auth::isPrivilegedUser([Auth::USER_ROLE_DEVELOPER])); $this->assertEquals(true, Auth::isPrivilegedUser([Auth::USER_ROLE_OWNER])); - $this->assertEquals(false, Auth::isPrivilegedUser([Auth::USER_ROLE_APP])); + $this->assertEquals(false, Auth::isPrivilegedUser([Auth::USER_ROLE_APPS])); $this->assertEquals(false, Auth::isPrivilegedUser([Auth::USER_ROLE_SYSTEM])); - $this->assertEquals(false, Auth::isPrivilegedUser([Auth::USER_ROLE_APP, Auth::USER_ROLE_APP])); - $this->assertEquals(false, Auth::isPrivilegedUser([Auth::USER_ROLE_APP, Auth::USER_ROLE_GUESTS])); + $this->assertEquals(false, Auth::isPrivilegedUser([Auth::USER_ROLE_APPS, Auth::USER_ROLE_APPS])); + $this->assertEquals(false, Auth::isPrivilegedUser([Auth::USER_ROLE_APPS, Auth::USER_ROLE_GUESTS])); $this->assertEquals(true, Auth::isPrivilegedUser([Auth::USER_ROLE_OWNER, Auth::USER_ROLE_GUESTS])); $this->assertEquals(true, Auth::isPrivilegedUser([Auth::USER_ROLE_OWNER, Auth::USER_ROLE_ADMIN, Auth::USER_ROLE_DEVELOPER])); } @@ -193,11 +193,11 @@ class AuthTest extends TestCase $this->assertEquals(false, Auth::isAppUser([Auth::USER_ROLE_ADMIN])); $this->assertEquals(false, Auth::isAppUser([Auth::USER_ROLE_DEVELOPER])); $this->assertEquals(false, Auth::isAppUser([Auth::USER_ROLE_OWNER])); - $this->assertEquals(true, Auth::isAppUser([Auth::USER_ROLE_APP])); + $this->assertEquals(true, Auth::isAppUser([Auth::USER_ROLE_APPS])); $this->assertEquals(false, Auth::isAppUser([Auth::USER_ROLE_SYSTEM])); - $this->assertEquals(true, Auth::isAppUser([Auth::USER_ROLE_APP, Auth::USER_ROLE_APP])); - $this->assertEquals(true, Auth::isAppUser([Auth::USER_ROLE_APP, Auth::USER_ROLE_GUESTS])); + $this->assertEquals(true, Auth::isAppUser([Auth::USER_ROLE_APPS, Auth::USER_ROLE_APPS])); + $this->assertEquals(true, Auth::isAppUser([Auth::USER_ROLE_APPS, Auth::USER_ROLE_GUESTS])); $this->assertEquals(false, Auth::isAppUser([Auth::USER_ROLE_OWNER, Auth::USER_ROLE_GUESTS])); $this->assertEquals(false, Auth::isAppUser([Auth::USER_ROLE_OWNER, Auth::USER_ROLE_ADMIN, Auth::USER_ROLE_DEVELOPER])); } @@ -282,7 +282,7 @@ class AuthTest extends TestCase public function testAppUserRoles(): void { - Authorization::setRole(Auth::USER_ROLE_APP); + Authorization::setRole(Auth::USER_ROLE_APPS); $user = new Document([ '$id' => ID::custom('123'), 'memberships' => [ diff --git a/tests/unit/Messaging/MessagingChannelsTest.php b/tests/unit/Messaging/MessagingChannelsTest.php index 336f972b83..94f7b450ed 100644 --- a/tests/unit/Messaging/MessagingChannelsTest.php +++ b/tests/unit/Messaging/MessagingChannelsTest.php @@ -147,7 +147,7 @@ class MessagingChannelsTest extends TestCase } /** - * Tests Wildcard (role:all) Permissions on every channel. + * Tests Wildcard ("any") Permissions on every channel. */ public function testWildcardPermission(): void {