From 40b0c081f72e9c1aa1f2afdc0bb0351dfcea261c Mon Sep 17 00:00:00 2001 From: prateek banga Date: Tue, 1 Aug 2023 23:29:15 +0530 Subject: [PATCH] remove checkPermission from Appwrite and more complex test case This commit removes check pemission from update document in appwrite as permission is being checked by Utopia already. This commits also improves the test case to have 3 levels of depth with relationships --- app/controllers/api/databases.php | 59 +------- .../Databases/DatabasesCustomClientTest.php | 129 ++++++++++++++++-- 2 files changed, 121 insertions(+), 67 deletions(-) diff --git a/app/controllers/api/databases.php b/app/controllers/api/databases.php index a61b453e0f..b57ab1eff8 100644 --- a/app/controllers/api/databases.php +++ b/app/controllers/api/databases.php @@ -3287,24 +3287,10 @@ App::patch('/v1/databases/:databaseId/collections/:collectionId/documents/:docum $data['$createdAt'] = $document->getCreatedAt(); // Make sure user doesn't switch createdAt $data['$id'] = $document->getId(); // Make sure user doesn't switch document unique ID $data['$permissions'] = $permissions; + $data['$collection'] = $document->getAttribute('$collection'); // Attribute $collection is required for Utopia. Copying it from old version of document $newDocument = new Document($data); - $checkPermissions = (function (Document $collection, Document $document, Document $old, string $permission, bool $shouldUpdate = false) use (&$checkPermissions, $dbForProject, $database) { - $documentSecurity = $collection->getAttribute('documentSecurity', false); - $validator = new Authorization($permission); - - $valid = $validator->isValid($collection->getPermissionsByType($permission)); - if (!$documentSecurity && !$valid && $shouldUpdate) { - throw new Exception(Exception::USER_UNAUTHORIZED); - } - - if ($permission === Database::PERMISSION_UPDATE) { - $valid = $valid || $validator->isValid($old->getPermissionsByType($permission)); - if ($documentSecurity && !$valid) { - throw new Exception(Exception::USER_UNAUTHORIZED); - } - } - + $addCollectionAttributeToRelations = (function (Document $collection, Document $document) use (&$addCollectionAttributeToRelations, $dbForProject, $database) { $relationships = \array_filter( $collection->getAttribute('attributes', []), fn($attribute) => $attribute->getAttribute('type') === Database::VAR_RELATIONSHIP @@ -3331,7 +3317,6 @@ App::patch('/v1/databases/:databaseId/collections/:collectionId/documents/:docum ); foreach ($relations as &$relation) { - $shouldUpdate = false; // If the relation is an array it can be either update or create a child document. if ( \is_array($relation) @@ -3347,35 +3332,18 @@ App::patch('/v1/databases/:databaseId/collections/:collectionId/documents/:docum $relation->getId() )); - // Attribute $collection is required for update document. Copying it from old version of document. + // Attribute $collection is required for Utopia. Copying it from old version of document. $relation->setAttribute('$collection', $relatedDocumentOldVersion->getAttribute('$collection')); // If the child document has to be created it will need checking permissions. if ($relatedDocumentOldVersion->isEmpty()) { - $type = Database::PERMISSION_CREATE; - if (isset($relation['$id']) && $relation['$id'] === 'unique()') { $relation['$id'] = ID::unique(); } } else { - $type = Database::PERMISSION_UPDATE; - - foreach ($relation as $key => $value) { - // No need to compare values of relations as for each relation we are recursively checking permission. - if ($relatedDocumentOldVersion->getAttribute($key) instanceof Document) { - continue; - } - // If any of the values are different, we need to check permission. - if ($relatedDocumentOldVersion->getAttribute($key) !== $value) { - $shouldUpdate = true; - $relation->removeAttribute('$collectionId'); - $relation->removeAttribute('$databaseId'); - $relation->setAttribute('$collection', $relatedCollection->getId()); - break; - } - } + $relation->setAttribute('$collection', 'database_' . $database->getInternalId() . '_collection_' . $relatedCollection->getInternalId()); } - $checkPermissions($relatedCollection, $relation, $relatedDocumentOldVersion, $type, $shouldUpdate); + $addCollectionAttributeToRelations($relatedCollection, $relation); } } @@ -3387,22 +3355,7 @@ App::patch('/v1/databases/:databaseId/collections/:collectionId/documents/:docum } }); - $shouldUpdate = false; - foreach ($newDocument as $key => $value) { - if ($document->getAttribute($key) instanceof Document) { - continue; - } - //If any of the values are different, we need to check permission. - if ($newDocument->getAttribute($key) !== $value) { - $shouldUpdate = true; - $newDocument->removeAttribute('$collectionId'); - $newDocument->removeAttribute('$databaseId'); - $newDocument->setAttribute('$collection', $collection->getId()); - break; - } - } - - $checkPermissions($collection, $newDocument, $document, Database::PERMISSION_UPDATE, $shouldUpdate); + $addCollectionAttributeToRelations($collection, $newDocument); try { $document = $dbForProject->withRequestTimestamp( diff --git a/tests/e2e/Services/Databases/DatabasesCustomClientTest.php b/tests/e2e/Services/Databases/DatabasesCustomClientTest.php index f0b26c77b6..406d785aee 100644 --- a/tests/e2e/Services/Databases/DatabasesCustomClientTest.php +++ b/tests/e2e/Services/Databases/DatabasesCustomClientTest.php @@ -344,8 +344,8 @@ class DatabasesCustomClientTest extends Scope 'x-appwrite-project' => $this->getProject()['$id'], 'x-appwrite-key' => $this->getProject()['apiKey'] ]), [ - 'collectionId' => ID::unique(), - 'name' => ID::unique(), + 'collectionId' => ID::custom('collection1'), + 'name' => ID::custom('collection1'), 'documentSecurity' => false, 'permissions' => [ Permission::create(Role::user($userId)), @@ -361,8 +361,8 @@ class DatabasesCustomClientTest extends Scope 'x-appwrite-project' => $this->getProject()['$id'], 'x-appwrite-key' => $this->getProject()['apiKey'] ]), [ - 'collectionId' => ID::unique(), - 'name' => ID::unique(), + 'collectionId' => ID::custom('collection2'), + 'name' => ID::custom('collection2'), 'documentSecurity' => false, 'permissions' => [ Permission::read(Role::user($userId)), @@ -374,14 +374,44 @@ class DatabasesCustomClientTest extends Scope 'x-appwrite-project' => $this->getProject()['$id'], 'x-appwrite-key' => $this->getProject()['apiKey'] ]), [ - 'collectionId' => ID::unique(), - 'name' => ID::unique(), + 'collectionId' => ID::custom('collection3'), + 'name' => ID::custom('collection3'), 'documentSecurity' => false, 'permissions' => [ Permission::create(Role::user($userId)), Permission::read(Role::user($userId)), Permission::update(Role::user($userId)), - Permission::delete(Role::user($userId)), ] + Permission::delete(Role::user($userId)), + ] + ]); + + $collection4 = $this->client->call(Client::METHOD_POST, '/databases/' . $databaseId . '/collections', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + 'x-appwrite-key' => $this->getProject()['apiKey'] + ]), [ + 'collectionId' => ID::custom('collection4'), + 'name' => ID::custom('collection4'), + 'documentSecurity' => false, + 'permissions' => [ + Permission::read(Role::user($userId)), + ] + ]); + + $collection5 = $this->client->call(Client::METHOD_POST, '/databases/' . $databaseId . '/collections', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + 'x-appwrite-key' => $this->getProject()['apiKey'] + ]), [ + 'collectionId' => ID::custom('collection5'), + 'name' => ID::custom('collection5'), + 'documentSecurity' => false, + 'permissions' => [ + Permission::create(Role::user($userId)), + Permission::read(Role::user($userId)), + Permission::update(Role::user($userId)), + Permission::delete(Role::user($userId)), + ] ]); // Creating one to one relationship from collection 1 to colletion 2 @@ -410,6 +440,32 @@ class DatabasesCustomClientTest extends Scope 'key' => $collection3['body']['$id'] ]); + // Creating one to one relationship from collection 3 to colletion 4 + $this->client->call(Client::METHOD_POST, '/databases/' . $databaseId . '/collections/' . $collection3['body']['$id'] . '/attributes/relationship', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + 'x-appwrite-key' => $this->getProject()['apiKey'] + ]), [ + 'relatedCollectionId' => $collection4['body']['$id'], + 'type' => 'oneToOne', + 'twoWay' => false, + 'onDelete' => 'setNull', + 'key' => $collection4['body']['$id'] + ]); + + // Creating one to one relationship from collection 4 to colletion 5 + $this->client->call(Client::METHOD_POST, '/databases/' . $databaseId . '/collections/' . $collection4['body']['$id'] . '/attributes/relationship', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + 'x-appwrite-key' => $this->getProject()['apiKey'] + ]), [ + 'relatedCollectionId' => $collection5['body']['$id'], + 'type' => 'oneToOne', + 'twoWay' => false, + 'onDelete' => 'setNull', + 'key' => $collection5['body']['$id'] + ]); + $this->client->call(Client::METHOD_POST, '/databases/' . $databaseId . '/collections/' . $collection1['body']['$id'] . '/attributes/string', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], @@ -446,6 +502,29 @@ class DatabasesCustomClientTest extends Scope 'default' => null, ]); + $this->client->call(Client::METHOD_POST, '/databases/' . $databaseId . '/collections/' . $collection4['body']['$id'] . '/attributes/string', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + 'x-appwrite-key' => $this->getProject()['apiKey'] + ]), [ + 'key' => "Rating", + 'size' => 100, + 'required' => false, + 'array' => false, + 'default' => null, + ]); + + $this->client->call(Client::METHOD_POST, '/databases/' . $databaseId . '/collections/' . $collection5['body']['$id'] . '/attributes/string', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + 'x-appwrite-key' => $this->getProject()['apiKey'] + ]), [ + 'key' => "Rating", + 'size' => 100, + 'required' => false, + 'array' => false, + 'default' => null, + ]); \sleep(2); // Creating parent document with a child reference to test the permissions @@ -454,7 +533,7 @@ class DatabasesCustomClientTest extends Scope 'x-appwrite-project' => $this->getProject()['$id'], 'x-appwrite-key' => $this->getProject()['apiKey'] ]), [ - 'documentId' => ID::unique(), + 'documentId' => ID::custom($collection1['body']['$id']), 'data' => [ 'Title' => 'Captain America', $collection2['body']['$id'] => [ @@ -462,22 +541,44 @@ class DatabasesCustomClientTest extends Scope 'Rating' => '10', $collection3['body']['$id'] => [ '$id' => ID::custom($collection3['body']['$id']), - 'Rating' => '10' + 'Rating' => '10', + $collection4['body']['$id'] => [ + '$id' => ID::custom($collection4['body']['$id']), + 'Rating' => '10', + $collection5['body']['$id'] => [ + '$id' => ID::custom($collection5['body']['$id']), + 'Rating' => '10' + ] + ] ] ] - ], - 'permissions' => [], + ] ]); $this->assertEquals(201, $parentDocument['headers']['status-code']); - // Update collection 3 document // This is the point of this test. We should be allowed to do this action, and it should not fail on permission check - $response = $this->client->call(Client::METHOD_PATCH, '/databases/' . $databaseId . '/collections/' . $collection3['body']['$id'] . '/documents/' . $collection3['body']['$id'], array_merge([ + $response = $this->client->call(Client::METHOD_PATCH, '/databases/' . $databaseId . '/collections/' . $collection1['body']['$id'] . '/documents/' . $collection1['body']['$id'], array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], ], $this->getHeaders()), [ 'data' => [ - 'Rating' => '11', + 'Title' => 'Captain America', + $collection2['body']['$id'] => [ + '$id' => ID::custom($collection2['body']['$id']), + 'Rating' => '10', + $collection3['body']['$id'] => [ + '$id' => ID::custom($collection3['body']['$id']), + 'Rating' => '11', + $collection4['body']['$id'] => [ + '$id' => ID::custom($collection4['body']['$id']), + 'Rating' => '10', + $collection5['body']['$id'] => [ + '$id' => ID::custom($collection5['body']['$id']), + 'Rating' => '11' + ] + ] + ] + ] ] ]);