Fix 5404 update document without relation permission by fanatic75 · Pull Request #5887 · appwrite/appwrite

fanatic75 · 2023-07-26T20:53:41Z

What does this PR do?

Update of document with relations without permissions of relation collection

Fixes: #5404

Test Plan

Added E2E test case

Related PRs and Issues

🐛 Bug Report: Nested update, permission issue #5404

Checklist

Have you read the Contributing Guidelines on issues?
If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

abnegate

Just some linter issues otherwise looks good 👍

…0.39.0 Make 1.4.x compatible with latest db release 0.39.0

…mplex scenario

…n updateDocument

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

stnguyen90

I left a comment. Also, I tried to update a document in the console but go an error:

The appwrite logs has:

appwrite  | [Error] Timestamp: 2023-08-01T22:20:36+00:00
appwrite  | [Error] Method: PATCH
appwrite  | [Error] URL: /v1/databases/:databaseId/collections/:collectionId/documents/:documentId
appwrite  | [Error] Type: PDOException
appwrite  | [Error] Message: Unknown column 'databaseId' in 'field list'
appwrite  | [Error] File: @swoole-src/library/core/Database/PDOStatementProxy.php
appwrite  | [Error] Line: 64

Are these problems just my instance?

fanatic75 · 2023-08-02T06:29:08Z

I left a comment. Also, I tried to update a document in the console but go an error:

The appwrite logs has:

appwrite  | [Error] Timestamp: 2023-08-01T22:20:36+00:00
appwrite  | [Error] Method: PATCH
appwrite  | [Error] URL: /v1/databases/:databaseId/collections/:collectionId/documents/:documentId
appwrite  | [Error] Type: PDOException
appwrite  | [Error] Message: Unknown column 'databaseId' in 'field list'
appwrite  | [Error] File: @swoole-src/library/core/Database/PDOStatementProxy.php
appwrite  | [Error] Line: 64

Are these problems just my instance?

Hmm, not really sure. This branch now requires more updates in Utopia. Could be because of that, let's wait till we have the changes merged in Utopia.

stnguyen90 · 2023-08-11T00:26:38Z

        "utopia-php/cli": "0.15.*",
        "utopia-php/config": "0.2.*",
-        "utopia-php/database": "0.38.*",
+        "utopia-php/database": "0.41.*",


I have 3 collections with a two-way one-to-one relationship between them like level1 => level2 => level3 and I experienced 2 problems:

First problem

With update access to level1 but read-only access to level2, I can update a document in level1 to point to a document in level2, but when I switch it to point to another document:

{ "level2": "level2other" }

I get an error:

{ "message": "Document with the requested ID already exists.", "code": 409, "type": "document_already_exists", "version": "dev", "file": "/usr/src/code/app/controllers/api/databases.php", "line": 3286, "trace": [ { "file": "/usr/src/code/vendor/utopia-php/framework/src/App.php", "line": 593, "function": "{closure}", "args": [ "one-to-one", "level1", "level1a", { "$internalId": "1", "$id": "level1a", "$createdAt": "2023-08-10T23:15:31.935+00:00", "$updatedAt": "2023-08-11T00:19:12.395+00:00", "$permissions": [], "$collection": "level1", "level2": "level2a" }, [], null, {}, {}, {}, "default" ] }, { "file": "/usr/src/code/vendor/utopia-php/framework/src/App.php", "line": 770, "function": "execute", "class": "Utopia\\App", "type": "->", "args": [ {}, {} ] }, { "file": "/usr/src/code/app/http.php", "line": 248, "function": "run", "class": "Utopia\\App", "type": "->", "args": [ {}, {} ] } ] }

Second problem

With update access to level1 and level2 but read-only access to level3, I can't set level3 on level2:

{ "level2": { "$id": "level2b", "level3": "level3a" } }

results in:

{ "$id": "level1a", "$createdAt": "2023-08-10T23:15:31.935+00:00", "$updatedAt": "2023-08-11T00:25:44.052+00:00", "$permissions": [], "level2": { "_createdAt": null, "$id": "level2b", "$updatedAt": "2023-08-11T00:16:59.164+00:00", "$permissions": [], "level3": null, "$databaseId": "one-to-one", "$collectionId": "level2" }, "$databaseId": "one-to-one", "$collectionId": "level1" }

Also, when I sent a request like:

{ "level2": { "$id": "level2b", "level3": "level3a" } }

my level2 document ended up with a _createdAt attribute:

{ "level2": { "_createdAt": null, "$id": "level2a", "$updatedAt": "2023-08-10T23:15:53.689+00:00", "$permissions": [], "level3": null, "$databaseId": "one-to-one", "$collectionId": "level2" } }

With two way relationship you need permission for both collection because when you are linking to a new document, you are updating both documents.

With one way you can link to a new document of related collection by only having the permission of the parent collection.

abnegate · 2023-08-11T01:41:13Z

@stnguyen90 Please re-review with the latest DB updates

abnegate · 2023-08-11T01:46:11Z

@stnguyen90 Please re-review with the latest DB updates

Hold off on that.. looks like there are some issues

stnguyen90 · 2023-08-15T01:06:36Z

+            'name' => ID::custom('collection2'),
+            'documentSecurity' => false,
+            'permissions' => [
+                Permission::read(Role::user($userId)),


Is there any test where a document in collection1 is updated to link to a different collection2 document? When I tried:

{ "data": { "level2": "level2b" } }

I got an error:

{ "message": "The current user is not authorized to perform the requested action.", "code": 401, "type": "user_unauthorized", "version": "dev", "file": "/usr/src/code/app/controllers/api/databases.php", "line": 3333, "trace": [ { "file": "/usr/src/code/vendor/utopia-php/framework/src/App.php", "line": 593, "function": "{closure}", "args": [ "one-to-one", "level1", "level1a", { "$internalId": "1", "$id": "level1a", "$createdAt": "2023-08-15T00:50:55.532+00:00", "$updatedAt": "2023-08-15T01:04:34.280+00:00", "$permissions": [], "$collection": "database_1_collection_1", "level2": "level2b" }, [], null, {}, {}, {}, "default" ] }, { "file": "/usr/src/code/vendor/utopia-php/framework/src/App.php", "line": 770, "function": "execute", "class": "Utopia\\App", "type": "->", "args": [ {}, {} ] }, { "file": "/usr/src/code/app/http.php", "line": 248, "function": "run", "class": "Utopia\\App", "type": "->", "args": [ {}, {} ] } ] }

Interestingly, when I updated like:

{ "data": { "level2": { "$id": "level2b" } } }

it screwed up the document such that $createdAt and $updatedAt" disappeared and _createdAt and _updatedAt were added.

{ "$id": "level1a", "$createdAt": "2023-08-15T00:50:55.532+00:00", "$updatedAt": "2023-08-15T01:07:00.060+00:00", "$permissions": [], "level2": { "_createdAt": null, "_updatedAt": null, "text": null, "$id": "level2b", "$permissions": [], "level3": null, "$databaseId": "one-to-one", "$collectionId": "level2" }, "$databaseId": "one-to-one", "$collectionId": "level1" }

In the test case, there is now the ability to link to a different document without update permission in the child level. If you are performing a two-way link, it is necessary to have update permission in both collections, as updating is required in both documents. For a one-way link, update permission is only required in the parent level.

Additionally, when running something like this, ensure that you have create permission in the level2 collection if the document does not already exist. If the document already exists and it is a one-way link, you should be able to link it with update permission in level1. This has been included in the test case. If it is a two-way link, permission is required in both levels only during linking documents. If you are updating attribute of a level/collection in two way relationship type, you don't need permission for both levels but only the level you're updating.

{ "data": { "level2": { "$id": "level2b" } } }

Regarding the null values for _createdAt and _updatedAt, I have confirmed that this issue is already present in version 1.4.x. It is not directly related to the permission bug, and we have not made any changes to it.

If you are performing a two-way link, it is necessary to have update permission in both collections

Good point! I'll make sure to test accordingly.

Regarding the null values for _createdAt and _updatedAt, I have confirmed that this issue is already present in version 1.4.x. It is not directly related to the permission bug, and we have not made any changes to it.

Do we have an issue or something to track this so that we can make a fix?

Do we have an issue or something to track this so that we can make a fix?

We don't have an issue open for this. Let me create an issue for that.

#5999
Added an issue

fanatic75 · 2023-08-16T08:00:41Z

        "utopia-php/cli": "0.15.*",
        "utopia-php/config": "0.2.*",
-        "utopia-php/database": "0.38.*",
+        "utopia-php/database": "0.41.*",


With two way relationship you need permission for both collection because when you are linking to a new document, you are updating both documents.

With one way you can link to a new document of related collection by only having the permission of the parent collection.

fanatic75 · 2023-08-16T08:17:20Z

+            'name' => ID::custom('collection2'),
+            'documentSecurity' => false,
+            'permissions' => [
+                Permission::read(Role::user($userId)),


In the test case, there is now the ability to link to a different document without update permission in the child level. If you are performing a two-way link, it is necessary to have update permission in both collections, as updating is required in both documents. For a one-way link, update permission is only required in the parent level.

Additionally, when running something like this, ensure that you have create permission in the level2 collection if the document does not already exist. If the document already exists and it is a one-way link, you should be able to link it with update permission in level1. This has been included in the test case. If it is a two-way link, permission is required in both levels only during linking documents. If you are updating attribute of a level/collection in two way relationship type, you don't need permission for both levels but only the level you're updating.

{ "data": { "level2": { "$id": "level2b" } } }

fanatic75 · 2023-08-16T09:15:26Z

+            'name' => ID::custom('collection2'),
+            'documentSecurity' => false,
+            'permissions' => [
+                Permission::read(Role::user($userId)),


Regarding the null values for _createdAt and _updatedAt, I have confirmed that this issue is already present in version 1.4.x. It is not directly related to the permission bug, and we have not made any changes to it.

…-5404-update-document-without-relation-permission

Prateek Banga and others added 6 commits July 27, 2023 02:02

Skipping checking permission of relations that are not being updated

0fc0255

added test case for verifying permission issue with relations

b8b3d13

lint issues

21bc2ae

updated logic to skip checking permission if no change in new document

54ea98a

format issues

5c5a8a9

fix lint issues and remove sleep from test

e631f39

fanatic75 requested a review from abnegate July 26, 2023 20:54

fix lint issues

6247e52

abnegate reviewed Jul 26, 2023

View reviewed changes

updates abuse and audit deps

a639c27

fanatic75 requested a review from abnegate July 27, 2023 08:46

fanatic75 added 2 commits July 27, 2023 15:48

adds name for database events in on function

0d8eeb5

updates deps

d743eb2

fanatic75 changed the base branch from 1.4.x to chore-make-compatible-with-db-0.39.0 July 27, 2023 14:42

fanatic75 changed the base branch from chore-make-compatible-with-db-0.39.0 to 1.4.x July 27, 2023 14:55

Merge pull request #5890 from appwrite/chore-make-compatible-with-db-…

e909503

…0.39.0 Make 1.4.x compatible with latest db release 0.39.0

stnguyen90 self-requested a review July 27, 2023 17:33

stnguyen90 reviewed Jul 27, 2023

View reviewed changes

Comment thread app/controllers/shared/api.php Outdated

better naming for on callback function

6b2d246

fanatic75 requested a review from stnguyen90 July 28, 2023 12:24

stnguyen90 reviewed Jul 28, 2023

View reviewed changes

Comment thread app/controllers/api/databases.php Outdated

Comment thread app/controllers/shared/api.php Outdated

Comment thread app/controllers/api/databases.php

Comment thread app/controllers/api/databases.php Outdated

Comment thread app/controllers/api/databases.php Outdated

fanatic75 added 2 commits July 29, 2023 12:01

adds attribute for update method and makes test case to check more co…

f7e9628

…mplex scenario

refactors checkPermission to throw exception when a change is found i…

409376e

…n updateDocument

fanatic75 requested a review from stnguyen90 July 31, 2023 08:34

abnegate reviewed Aug 1, 2023

View reviewed changes

Comment thread tests/e2e/Services/Databases/DatabasesCustomClientTest.php

github-actions Bot mentioned this pull request Aug 1, 2023

PRs Opened in July 2023 appwrite/.github#61

Closed

fanatic75 added 2 commits August 1, 2023 23:29

remove checkPermission from Appwrite and more complex test case

40b0c08

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

fix lint issues

fb3bab7

stnguyen90 previously requested changes Aug 1, 2023

View reviewed changes

Comment thread app/controllers/api/databases.php

fanatic75 marked this pull request as ready for review August 10, 2023 11:54

fanatic75 closed this Aug 10, 2023

fanatic75 reopened this Aug 10, 2023

fanatic75 and others added 2 commits August 10, 2023 20:03

fix test cases

40047a2

Review fixes

6cd0e32

stnguyen90 reviewed Aug 11, 2023

View reviewed changes

abnegate added 2 commits August 10, 2023 21:28

Update libs

b0b6f93

Lint

16cb495

abnegate requested a review from stnguyen90 August 11, 2023 01:39

fix test cases

971ebbc

fanatic75 force-pushed the fix-5404-update-document-without-relation-permission branch from f7f3ff2 to 971ebbc Compare August 11, 2023 10:47

fanatic75 added 3 commits August 11, 2023 17:25

Adds more complex test scenario

68736a5

lint fix

29e1757

resolve merge conflicts

8e1ef81

fanatic75 force-pushed the fix-5404-update-document-without-relation-permission branch from adbfa30 to 8e1ef81 Compare August 14, 2023 16:30

removes unnecessary use statement

cb6760a

abnegate requested review from stnguyen90 and removed request for stnguyen90 August 14, 2023 23:42

stnguyen90 reviewed Aug 15, 2023

View reviewed changes

adds another scenario in test case

d6f4d76

fanatic75 requested a review from stnguyen90 August 16, 2023 11:49

fanatic75 commented Aug 16, 2023

View reviewed changes

fanatic75 added 2 commits August 17, 2023 19:04

lint fix

860e995

Merge branch '1.4.x' of https://github.com/appwrite/appwrite into fix…

27df545

…-5404-update-document-without-relation-permission

abnegate approved these changes Aug 17, 2023

View reviewed changes

abnegate merged commit a019d01 into 1.4.x Aug 17, 2023

github-actions Bot mentioned this pull request Sep 1, 2023

PRs Closed in August 2023 appwrite/.github#70

Closed

abnegate deleted the fix-5404-update-document-without-relation-permission branch October 25, 2023 04:53

Conversation

fanatic75 commented Jul 26, 2023 • edited by stnguyen90 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Test Plan

Related PRs and Issues

Checklist

Uh oh!

abnegate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stnguyen90 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanatic75 commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

First problem

Second problem

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abnegate commented Aug 11, 2023

Uh oh!

abnegate commented Aug 11, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fanatic75 commented Jul 26, 2023 •

edited by stnguyen90

Loading

fanatic75 commented Aug 2, 2023 •

edited

Loading