From 7853c9370b3414b4997344f2342ae71f6b0684c5 Mon Sep 17 00:00:00 2001 From: kodumbeats Date: Wed, 3 Nov 2021 12:38:06 -0400 Subject: [PATCH 1/5] Enforce that users cannot add permission roles they do not have --- app/controllers/api/database.php | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/app/controllers/api/database.php b/app/controllers/api/database.php index 5b228d6004..8f1ca01ed2 100644 --- a/app/controllers/api/database.php +++ b/app/controllers/api/database.php @@ -26,6 +26,7 @@ use Utopia\Database\Exception\Authorization as AuthorizationException; use Utopia\Database\Exception\Duplicate as DuplicateException; use Utopia\Database\Exception\Limit as LimitException; use Utopia\Database\Exception\Structure as StructureException; +use Appwrite\Auth\Auth; use Appwrite\Database\Validator\CustomId; use Appwrite\Network\Validator\Email; use Appwrite\Network\Validator\IP; @@ -1575,6 +1576,18 @@ App::post('/v1/database/collections/:collectionId/documents') $data['$read'] = (is_null($read) && !$user->isEmpty()) ? ['user:'.$user->getId()] : $read ?? []; // By default set read permissions for user $data['$write'] = (is_null($write) && !$user->isEmpty()) ? ['user:'.$user->getId()] : $write ?? []; // By default set write permissions for user + // Users can only add their roles to documents, API keys can add any + foreach ($data['$read'] as $read) { + if (!Authorization::isRole('role:'.Auth::USER_ROLE_APP) && !Authorization::isRole($read)) { + throw new Exception('Read permissions must be one of: ('.\implode(', ', Authorization::getRoles()).')', 400); + } + } + foreach ($data['$write'] as $write) { + if (!Authorization::isRole('role:'.Auth::USER_ROLE_APP) && !Authorization::isRole($write)) { + throw new Exception('Write permissions must be one of: ('.\implode(', ', Authorization::getRoles()).')', 400); + } + } + try { if ($collection->getAttribute('permission') === 'collection') { /** @var Document $document */ @@ -1813,6 +1826,18 @@ App::patch('/v1/database/collections/:collectionId/documents/:documentId') $data['$read'] = (is_null($read)) ? ($document->getRead() ?? []) : $read; // By default inherit read permissions $data['$write'] = (is_null($write)) ? ($document->getWrite() ?? []) : $write; // By default inherit write permissions + // Users can only add their roles to documents, API keys can add any + foreach ($data['$read'] as $read) { + if (!Authorization::isRole('role:'.Auth::USER_ROLE_APP) && !Authorization::isRole($read)) { + throw new Exception('Read permissions must be one of: ('.\implode(', ', Authorization::getRoles()).')', 400); + } + } + foreach ($data['$write'] as $write) { + if (!Authorization::isRole('role:'.Auth::USER_ROLE_APP) && !Authorization::isRole($write)) { + throw new Exception('Write permissions must be one of: ('.\implode(', ', Authorization::getRoles()).')', 400); + } + } + try { if ($collection->getAttribute('permission') === 'collection') { /** @var Document $document */ From 85c2078e777b5197469514db0fb9764a4954baea Mon Sep 17 00:00:00 2001 From: kodumbeats Date: Wed, 3 Nov 2021 12:39:15 -0400 Subject: [PATCH 2/5] Update tests where unowned role is used to create documents --- tests/e2e/Services/Database/DatabaseBase.php | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/e2e/Services/Database/DatabaseBase.php b/tests/e2e/Services/Database/DatabaseBase.php index 3f11154c49..568d3fb9bb 100644 --- a/tests/e2e/Services/Database/DatabaseBase.php +++ b/tests/e2e/Services/Database/DatabaseBase.php @@ -1127,8 +1127,8 @@ trait DatabaseBase 'releaseYear' => 2017, 'actors' => [], ], - 'read' => ['user:'.$this->getUser()['$id'], 'user:testx'], - 'write' => ['user:'.$this->getUser()['$id'], 'user:testy'], + 'read' => ['user:'.$this->getUser()['$id']], + 'write' => ['user:'.$this->getUser()['$id']], ]); $id = $document['body']['$id']; @@ -1136,8 +1136,8 @@ trait DatabaseBase $this->assertEquals($document['headers']['status-code'], 201); $this->assertEquals($document['body']['title'], 'Thor: Ragnaroc'); $this->assertEquals($document['body']['releaseYear'], 2017); - $this->assertEquals($document['body']['$read'][1], 'user:testx'); - $this->assertEquals($document['body']['$write'][1], 'user:testy'); + $this->assertEquals('user:'.$this->getUser()['$id'], $document['body']['$read'][0],); + $this->assertEquals('user:'.$this->getUser()['$id'], $document['body']['$write'][0],); $document = $this->client->call(Client::METHOD_PATCH, '/database/collections/' . $data['moviesId'] . '/documents/' . $id, array_merge([ 'content-type' => 'application/json', @@ -1146,11 +1146,15 @@ trait DatabaseBase 'data' => [ 'title' => 'Thor: Ragnarok', ], + 'read' => ['role:member'], + 'write' => ['role:member'], ]); $this->assertEquals($document['headers']['status-code'], 200); $this->assertEquals($document['body']['title'], 'Thor: Ragnarok'); $this->assertEquals($document['body']['releaseYear'], 2017); + $this->assertEquals('role:member', $document['body']['$read'][0]); + $this->assertEquals('role:member', $document['body']['$write'][0]); $document = $this->client->call(Client::METHOD_GET, '/database/collections/' . $data['moviesId'] . '/documents/' . $id, array_merge([ 'content-type' => 'application/json', From 4dd6f63fb910720f920b74e07d6d9e32673cee24 Mon Sep 17 00:00:00 2001 From: kodumbeats Date: Thu, 11 Nov 2021 20:14:40 -0500 Subject: [PATCH 3/5] Use Auth method to simplify code --- app/controllers/api/database.php | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/app/controllers/api/database.php b/app/controllers/api/database.php index 8f1ca01ed2..ef89961152 100644 --- a/app/controllers/api/database.php +++ b/app/controllers/api/database.php @@ -1577,14 +1577,15 @@ App::post('/v1/database/collections/:collectionId/documents') $data['$write'] = (is_null($write) && !$user->isEmpty()) ? ['user:'.$user->getId()] : $write ?? []; // By default set write permissions for user // Users can only add their roles to documents, API keys can add any + $roles = Authorization::getRoles(); foreach ($data['$read'] as $read) { - if (!Authorization::isRole('role:'.Auth::USER_ROLE_APP) && !Authorization::isRole($read)) { - throw new Exception('Read permissions must be one of: ('.\implode(', ', Authorization::getRoles()).')', 400); + if (!Auth::isAppUser($roles) && !Authorization::isRole($read)) { + throw new Exception('Read permissions must be one of: ('.\implode(', ', $roles).')', 400); } } foreach ($data['$write'] as $write) { - if (!Authorization::isRole('role:'.Auth::USER_ROLE_APP) && !Authorization::isRole($write)) { - throw new Exception('Write permissions must be one of: ('.\implode(', ', Authorization::getRoles()).')', 400); + if (!Auth::isAppUser($roles) && !Authorization::isRole($write)) { + throw new Exception('Write permissions must be one of: ('.\implode(', ', $roles).')', 400); } } @@ -1827,14 +1828,15 @@ App::patch('/v1/database/collections/:collectionId/documents/:documentId') $data['$write'] = (is_null($write)) ? ($document->getWrite() ?? []) : $write; // By default inherit write permissions // Users can only add their roles to documents, API keys can add any + $roles = Authorization::getRoles(); foreach ($data['$read'] as $read) { - if (!Authorization::isRole('role:'.Auth::USER_ROLE_APP) && !Authorization::isRole($read)) { - throw new Exception('Read permissions must be one of: ('.\implode(', ', Authorization::getRoles()).')', 400); + if (!Auth::isAppUser($roles) && !Authorization::isRole($read)) { + throw new Exception('Read permissions must be one of: ('.\implode(', ', $roles).')', 400); } } foreach ($data['$write'] as $write) { - if (!Authorization::isRole('role:'.Auth::USER_ROLE_APP) && !Authorization::isRole($write)) { - throw new Exception('Write permissions must be one of: ('.\implode(', ', Authorization::getRoles()).')', 400); + if (!Auth::isAppUser($roles) && !Authorization::isRole($write)) { + throw new Exception('Write permissions must be one of: ('.\implode(', ', $roles).')', 400); } } From 09de7fb6982928182e4c3f46c2e6595190298c05 Mon Sep 17 00:00:00 2001 From: kodumbeats Date: Thu, 11 Nov 2021 20:14:46 -0500 Subject: [PATCH 4/5] Remove trailing commas --- tests/e2e/Services/Database/DatabaseBase.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/e2e/Services/Database/DatabaseBase.php b/tests/e2e/Services/Database/DatabaseBase.php index 568d3fb9bb..e5ed635807 100644 --- a/tests/e2e/Services/Database/DatabaseBase.php +++ b/tests/e2e/Services/Database/DatabaseBase.php @@ -1136,8 +1136,8 @@ trait DatabaseBase $this->assertEquals($document['headers']['status-code'], 201); $this->assertEquals($document['body']['title'], 'Thor: Ragnaroc'); $this->assertEquals($document['body']['releaseYear'], 2017); - $this->assertEquals('user:'.$this->getUser()['$id'], $document['body']['$read'][0],); - $this->assertEquals('user:'.$this->getUser()['$id'], $document['body']['$write'][0],); + $this->assertEquals('user:'.$this->getUser()['$id'], $document['body']['$read'][0]); + $this->assertEquals('user:'.$this->getUser()['$id'], $document['body']['$write'][0]); $document = $this->client->call(Client::METHOD_PATCH, '/database/collections/' . $data['moviesId'] . '/documents/' . $id, array_merge([ 'content-type' => 'application/json', From ee5b1da82df08896e6bd7e7cc99fdfe80a811cfc Mon Sep 17 00:00:00 2001 From: kodumbeats Date: Sun, 28 Nov 2021 08:32:00 -0500 Subject: [PATCH 5/5] Roles must be assoc array with roles as keys --- app/controllers/api/database.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/database.php b/app/controllers/api/database.php index 056dfbf3a2..dcc519bc67 100644 --- a/app/controllers/api/database.php +++ b/app/controllers/api/database.php @@ -1641,7 +1641,7 @@ App::post('/v1/database/collections/:collectionId/documents') $data['$write'] = (is_null($write) && !$user->isEmpty()) ? ['user:'.$user->getId()] : $write ?? []; // By default set write permissions for user // Users can only add their roles to documents, API keys can add any - $roles = Authorization::getRoles(); + $roles = \array_fill_keys(Authorization::getRoles(), true); // Auth::isAppUser expects roles to be keys, not values of assoc array foreach ($data['$read'] as $read) { if (!Auth::isAppUser($roles) && !Authorization::isRole($read)) { throw new Exception('Read permissions must be one of: ('.\implode(', ', $roles).')', 400); @@ -1999,7 +1999,7 @@ App::patch('/v1/database/collections/:collectionId/documents/:documentId') $data['$write'] = (is_null($write)) ? ($document->getWrite() ?? []) : $write; // By default inherit write permissions // Users can only add their roles to documents, API keys can add any - $roles = Authorization::getRoles(); + $roles = \array_fill_keys(Authorization::getRoles(), true); // Auth::isAppUser expects roles to be keys, not values of assoc array foreach ($data['$read'] as $read) { if (!Auth::isAppUser($roles) && !Authorization::isRole($read)) { throw new Exception('Read permissions must be one of: ('.\implode(', ', $roles).')', 400);