Resolve #244: Updating the token permissions fails hard when a table has been deleted in the meantime

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 {

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,

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],
+        ]
+    )

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

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)
 

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.