From 9d6d679eb3b2dd173fcfa84f787a2d7bb73568b4 Mon Sep 17 00:00:00 2001
From: davide <silvestri.eng@gmail.com>
Date: Tue, 22 Mar 2022 10:18:18 +0000
Subject: [PATCH] Resolve #244: Updating the token permissions fails hard when
a table has been deleted in the meantime
---
.../database/api/tokens/serializers.py | 6 ++
.../baserow/contrib/database/tokens/models.py | 17 ++++
.../database/api/tokens/test_token_views.py | 96 ++++++++++++++++++-
.../database/tokens/test_token_handler.py | 29 ++----
changelog.md | 4 +-
.../database/components/settings/APIToken.vue | 46 +++++++++
6 files changed, 172 insertions(+), 26 deletions(-)
diff --git a/backend/src/baserow/contrib/database/api/tokens/serializers.py b/backend/src/baserow/contrib/database/api/tokens/serializers.py
index d58c0c7cf..2fd2c841f 100644
--- a/backend/src/baserow/contrib/database/api/tokens/serializers.py
+++ b/backend/src/baserow/contrib/database/api/tokens/serializers.py
@@ -17,6 +17,8 @@ class TokenPermissionsField(serializers.Field):
'ids like [["database", 1], ["table", 1]].'
),
"invalid_instance_type": "The instance type can only be a database or table.",
+ "invalid_table_id": "The table id {instance_id} is not valid.",
+ "invalid_database_id": "The database id {instance_id} is not valid.",
}
valid_types = ["create", "read", "update", "delete"]
@@ -102,8 +104,12 @@ class TokenPermissionsField(serializers.Field):
if isinstance(value, list):
for index, (instance_type, instance_id) in enumerate(value):
if instance_type == "database":
+ if instance_id not in databases:
+ self.fail("invalid_database_id", instance_id=instance_id)
data[key][index] = databases[instance_id]
elif instance_type == "table":
+ if instance_id not in tables:
+ self.fail("invalid_table_id", instance_id=instance_id)
data[key][index] = tables[instance_id]
return {
diff --git a/backend/src/baserow/contrib/database/tokens/models.py b/backend/src/baserow/contrib/database/tokens/models.py
index 7ca0d2752..1cbdc0d91 100644
--- a/backend/src/baserow/contrib/database/tokens/models.py
+++ b/backend/src/baserow/contrib/database/tokens/models.py
@@ -1,5 +1,6 @@
from django.db import models
from django.contrib.auth import get_user_model
+from django.db.models import Q
from baserow.core.mixins import ParentGroupTrashableModelMixin
from baserow.core.models import Group
@@ -47,6 +48,20 @@ class Token(ParentGroupTrashableModelMixin, models.Model):
ordering = ("id",)
+class TokenPermissionManager(models.Manager):
+ """
+ This manager is needed to avoid problems with tokens of trashed
+ but not already deleted databases and tables.
+ After 3 days (default) trashed databases and tables are deleted permanently,
+ and so are relative token permissions (because of the CASCADE option).
+ In the meanwhile, we need to filter out the trashed databases and tables.
+ """
+
+ def get_queryset(self):
+ trashed_Q = Q(database__trashed=True) | Q(table__trashed=True)
+ return super().get_queryset().filter(~trashed_Q)
+
+
class TokenPermission(models.Model):
"""
The existence of a permission indicates that the token has access to a table. If
@@ -56,6 +71,8 @@ class TokenPermission(models.Model):
means that the token has access to that table.
"""
+ objects = TokenPermissionManager()
+
token = models.ForeignKey("database.Token", on_delete=models.CASCADE)
type = models.CharField(
max_length=6,
diff --git a/backend/tests/baserow/contrib/database/api/tokens/test_token_views.py b/backend/tests/baserow/contrib/database/api/tokens/test_token_views.py
index 907b93208..c0c0e5cef 100644
--- a/backend/tests/baserow/contrib/database/api/tokens/test_token_views.py
+++ b/backend/tests/baserow/contrib/database/api/tokens/test_token_views.py
@@ -12,6 +12,7 @@ from django.shortcuts import reverse
from baserow.contrib.database.tokens.handler import TokenHandler
from baserow.contrib.database.tokens.models import Token, TokenPermission
+from baserow.core.trash.handler import TrashHandler
@pytest.mark.django_db
@@ -209,8 +210,8 @@ def test_get_token(api_client, data_fixture):
assert len(response_json["permissions"]["read"]) == 1
assert response_json["permissions"]["read"][0] == ["database", database_2.id]
assert len(response_json["permissions"]["update"]) == 2
- assert response_json["permissions"]["update"][0] == ["database", database_1.id]
- assert response_json["permissions"]["update"][1] == ["table", table_3.id]
+ assert response_json["permissions"]["update"][0] == ["table", table_3.id]
+ assert response_json["permissions"]["update"][1] == ["database", database_1.id]
assert response_json["permissions"]["delete"] is False
TokenHandler().update_token_permissions(
@@ -227,8 +228,8 @@ def test_get_token(api_client, data_fixture):
response_json = response.json()
assert response.status_code == HTTP_200_OK
assert len(response_json["permissions"]["create"]) == 2
- assert response_json["permissions"]["create"][0] == ["database", database_1.id]
- assert response_json["permissions"]["create"][1] == ["database", database_2.id]
+ assert response_json["permissions"]["create"][0] == ["database", database_2.id]
+ assert response_json["permissions"]["create"][1] == ["database", database_1.id]
assert response_json["permissions"]["read"] is False
assert response_json["permissions"]["update"] is True
assert len(response_json["permissions"]["delete"]) == 1
@@ -525,3 +526,90 @@ def test_delete_token(api_client, data_fixture):
assert Token.objects.all().count() == 2
assert TokenPermission.objects.all().count() == 0
+
+
+@pytest.mark.django_db
+def test_trashing_table_hides_restores_tokens(api_client, data_fixture):
+ user, token = data_fixture.create_user_and_token()
+ group_1 = data_fixture.create_group(user=user)
+ token_1 = data_fixture.create_token(user=user, group=group_1)
+
+ database_1 = data_fixture.create_database_application(group=group_1)
+ database_2 = data_fixture.create_database_application(group=group_1)
+ table_1 = data_fixture.create_database_table(
+ database=database_1, create_table=False
+ )
+ table_2 = data_fixture.create_database_table(
+ database=database_1, create_table=False
+ )
+
+ TokenHandler().update_token_permissions(
+ user,
+ token_1,
+ create=[database_1, table_1, table_2, database_2],
+ read=[database_1, table_1, table_2, database_2],
+ update=[database_1, table_1, table_2, database_2],
+ delete=[database_1, table_1, table_2, database_2],
+ )
+
+ def assert_all_permission_types_for_token_are(value):
+ url = reverse("api:database:tokens:item", kwargs={"token_id": token_1.id})
+ response = api_client.get(url, format="json", HTTP_AUTHORIZATION=f"JWT {token}")
+ response_json = response.json()
+ assert response.status_code == HTTP_200_OK
+ # permissions must be the same, ignoring order
+ permissions = response_json["permissions"]
+ sorted_value = sorted(value)
+ assert sorted(permissions["update"]) == sorted_value
+ assert sorted(permissions["create"]) == sorted_value
+ assert sorted(permissions["delete"]) == sorted_value
+ assert sorted(permissions["read"]) == sorted_value
+
+ assert_all_permission_types_for_token_are(
+ [
+ ["table", table_1.id],
+ ["table", table_2.id],
+ ["database", database_2.id],
+ ["database", database_1.id],
+ ]
+ )
+
+ TrashHandler.trash(user, group_1, database_1, table_1)
+
+ assert_all_permission_types_for_token_are(
+ [
+ ["table", table_2.id],
+ ["database", database_2.id],
+ ["database", database_1.id],
+ ]
+ )
+
+ TrashHandler.trash(user, group_1, database_1, database_1)
+
+ assert_all_permission_types_for_token_are(
+ [
+ ["table", table_2.id],
+ ["database", database_2.id],
+ ]
+ )
+
+ TrashHandler.restore_item(user, "application", database_1.id)
+
+ assert_all_permission_types_for_token_are(
+ [
+ ["table", table_2.id],
+ ["database", database_2.id],
+ ["database", database_1.id],
+ ]
+ )
+
+ TrashHandler.restore_item(user, "table", table_1.id)
+
+ assert_all_permission_types_for_token_are(
+ [
+ ["table", table_2.id],
+ ["table", table_1.id],
+ ["database", database_2.id],
+ ["database", database_1.id],
+ ]
+ )
diff --git a/backend/tests/baserow/contrib/database/tokens/test_token_handler.py b/backend/tests/baserow/contrib/database/tokens/test_token_handler.py
index 67cbc955a..3ed951d1c 100644
--- a/backend/tests/baserow/contrib/database/tokens/test_token_handler.py
+++ b/backend/tests/baserow/contrib/database/tokens/test_token_handler.py
@@ -97,7 +97,6 @@ def test_generate_token(data_fixture):
@pytest.mark.django_db
def test_create_token(data_fixture):
user = data_fixture.create_user()
- data_fixture.create_user()
group_1 = data_fixture.create_group(user=user)
group_2 = data_fixture.create_group()
@@ -117,25 +116,15 @@ def test_create_token(data_fixture):
permissions = TokenPermission.objects.all()
assert permissions.count() == 4
- assert permissions[0].token_id == token.id
- assert permissions[0].type == "create"
- assert permissions[0].database_id is None
- assert permissions[0].table_id is None
-
- assert permissions[1].token_id == token.id
- assert permissions[1].type == "read"
- assert permissions[1].database_id is None
- assert permissions[1].table_id is None
-
- assert permissions[2].token_id == token.id
- assert permissions[2].type == "update"
- assert permissions[2].database_id is None
- assert permissions[2].table_id is None
-
- assert permissions[3].token_id == token.id
- assert permissions[3].type == "delete"
- assert permissions[3].database_id is None
- assert permissions[3].table_id is None
+ permissions_types = {"create", "read", "update", "delete"}
+ for i in range(4):
+ assert permissions[i].token_id == token.id
+ try:
+ permissions_types.remove(permissions[i].type)
+ except KeyError:
+ assert False, f"Permission type '{permissions[i].type}' not found"
+ assert permissions[i].database_id is None
+ assert permissions[i].table_id is None
@pytest.mark.django_db
diff --git a/changelog.md b/changelog.md
index b96bd47fa..b50141e93 100644
--- a/changelog.md
+++ b/changelog.md
@@ -12,7 +12,8 @@
* Fixed the reactivity of the row values of newly created fields in some cases.
* Fixed a bug that made it possible to delete created on/modified by fields on the web frontend.
* Allow the setting of max request page size via environment variable.
-* Boolean field converts the word `checked` to `True` value
+* Boolean field converts the word `checked` to `True` value.
+* Fixed a bug where the backend would fail hard updating token permissions for deleted tables.
## Released (2022-03-03 1.9.1)
@@ -81,7 +82,6 @@
* Add health checks for all services.
* Ensure error logging is enabled in the Backend even when DEBUG is off.
* Removed upload file size limit.
-* Boolean field converts the word `checked` to `True` value
## Released (2022-01-13 1.8.2)
diff --git a/web-frontend/modules/database/components/settings/APIToken.vue b/web-frontend/modules/database/components/settings/APIToken.vue
index 4221e6110..1ddf2b759 100644
--- a/web-frontend/modules/database/components/settings/APIToken.vue
+++ b/web-frontend/modules/database/components/settings/APIToken.vue
@@ -210,6 +210,16 @@ export default {
)
},
},
+ watch: {
+ databases: {
+ handler() {
+ // if databases or tables change, we need to ensure that token permissions
+ // are still valid
+ this.removeInvalidPermissions()
+ },
+ deep: true,
+ },
+ },
methods: {
copyTokenToClipboard() {
copyToClipboard(this.token.key)
@@ -355,6 +365,42 @@ export default {
this.exists(operation, 'table', table.id)
)
},
+ /**
+ * Indicates if the permission refer to a database or table still existent.
+ * This fixes the problem that arises when user deletes a database or table from
+ * another browser tab while this form is opened.
+ * We need to delete the permissions that are pointing to the deleted database
+ * before sending updates to the backend if we want to avoid errors.
+ */
+ removeInvalidPermissions() {
+ const tokenPermissions = JSON.parse(
+ JSON.stringify(this.token.permissions)
+ )
+ for (const [operation, permissions] of Object.entries(tokenPermissions)) {
+ if (!Array.isArray(permissions)) {
+ continue
+ }
+ permissions.forEach((permission) => {
+ if (!this.isPermissionValid(permission)) {
+ const [permType, permId] = permission
+ this.remove(operation, permType, permId)
+ }
+ })
+ }
+ },
+ isPermissionValid(permission) {
+ const databases = this.databases
+ const [permType, permId] = permission
+ if (permType === 'database') {
+ const database = databases.find((database) => database.id === permId)
+ return database !== undefined
+ } else if (permType === 'table') {
+ return databases.find((database) => {
+ const table = database.tables.find((table) => table.id === permId)
+ return table !== undefined
+ })
+ }
+ },
/**
* Changes the token permission state of all databases and tables of the given
* operation. Also updates the permissions with the backend.