From 64019cbc558f84ee34f4f0139c1bf2d467b76ec2 Mon Sep 17 00:00:00 2001
From: Nigel Gott <nigel@baserow.io>
Date: Fri, 25 Aug 2023 11:41:54 +0000
Subject: [PATCH] Resolve "Fix allowing builders to write lookup formulas onto
tables into which they don't have viewer or higher rights"
---
.../dependencies/dependency_rebuilder.py | 8 +-
.../database/fields/dependencies/handler.py | 73 +++++++++-
.../contrib/database/fields/field_types.py | 14 ++
.../contrib/database/fields/handler.py | 9 +-
.../contrib/database/fields/registries.py | 16 +++
.../contrib/database/formula/__init__.py | 6 +
.../database/formula/types/exceptions.py | 9 ++
.../database/formula/types/visitors.py | 9 +-
...o_write_lookup_formulas_onto_tables_i.json | 7 +
.../fields/test_link_row_field_rbac.py | 126 ++++++++++++++++++
.../rbac/SelectSubjectsListFooter.vue | 14 --
.../baserow_enterprise/locales/en.json | 3 +-
.../field/FieldSelectTargetFieldSubForm.vue | 9 +-
.../field/FieldSelectThroughFieldSubForm.vue | 11 +-
14 files changed, 286 insertions(+), 28 deletions(-)
create mode 100644 changelog/entries/unreleased/bug/1883_fix_allowing_builders_to_write_lookup_formulas_onto_tables_i.json
diff --git a/backend/src/baserow/contrib/database/fields/dependencies/dependency_rebuilder.py b/backend/src/baserow/contrib/database/fields/dependencies/dependency_rebuilder.py
index 8944e86ec..3b97cf87e 100644
--- a/backend/src/baserow/contrib/database/fields/dependencies/dependency_rebuilder.py
+++ b/backend/src/baserow/contrib/database/fields/dependencies/dependency_rebuilder.py
@@ -1,4 +1,4 @@
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, List
from django.db.models import Q
@@ -66,7 +66,7 @@ def update_fields_with_broken_references(field: "field_models.Field"):
def rebuild_field_dependencies(
field_instance,
field_cache: FieldCache,
-):
+) -> List[FieldDependency]:
"""
Deletes all existing dependencies a field has and resets them to the ones
defined by the field_instances FieldType.get_field_dependencies. Does not
@@ -75,6 +75,7 @@ def rebuild_field_dependencies(
:param field_instance: The field whose dependencies to change.
:param field_cache: A cache which will be used to lookup the actual
fields referenced by any provided field dependencies.
+ :return: Any new dependencies created by the rebuild.
"""
from baserow.contrib.database.fields.registries import field_type_registry
@@ -106,8 +107,9 @@ def rebuild_field_dependencies(
):
raise CircularFieldDependencyError()
- FieldDependency.objects.bulk_create(new_dependencies_to_create)
+ new_dependencies = FieldDependency.objects.bulk_create(new_dependencies_to_create)
# All new dependencies will have been removed from current_deps_by_str and so any
# remaining ones are old dependencies which should no longer exist. Delete them.
delete_ids = [dep.id for dep in current_deps_by_str.values()]
FieldDependency.objects.filter(pk__in=delete_ids).delete()
+ return new_dependencies
diff --git a/backend/src/baserow/contrib/database/fields/dependencies/handler.py b/backend/src/baserow/contrib/database/fields/dependencies/handler.py
index 8808ae5b2..d28f84257 100644
--- a/backend/src/baserow/contrib/database/fields/dependencies/handler.py
+++ b/backend/src/baserow/contrib/database/fields/dependencies/handler.py
@@ -1,5 +1,6 @@
from typing import Iterable, List, Optional, Tuple
+from django.contrib.auth.models import AbstractUser
from django.db.models import Q
from baserow.contrib.database.fields.dependencies.dependency_rebuilder import (
@@ -10,6 +11,9 @@ from baserow.contrib.database.fields.dependencies.dependency_rebuilder import (
from baserow.contrib.database.fields.field_cache import FieldCache
from baserow.contrib.database.fields.models import Field, LinkRowField
from baserow.contrib.database.fields.registries import FieldType, field_type_registry
+from baserow.core.handler import CoreHandler
+from baserow.core.models import Workspace
+from baserow.core.types import PermissionCheck
from .models import FieldDependency
@@ -35,16 +39,19 @@ class FieldDependencyHandler:
]
@classmethod
- def rebuild_dependencies(cls, field, field_cache: FieldCache):
+ def rebuild_dependencies(
+ cls, field, field_cache: FieldCache
+ ) -> List[FieldDependency]:
"""
Rebuilds this fields dependencies based off field_type.get_field_dependencies.
:param field: The field to rebuild its field dependencies for.
:param field_cache: A field cache which will be used to lookup fields.
+ :return: Any new dependencies created by the rebuild.
"""
update_fields_with_broken_references(field)
- rebuild_field_dependencies(field, field_cache)
+ return rebuild_field_dependencies(field, field_cache)
@classmethod
def break_dependencies_delete_dependants(cls, field):
@@ -160,3 +167,65 @@ class FieldDependencyHandler:
)
dependants.append(field_dep_tuple)
return dependants
+
+ @classmethod
+ def rebuild_or_raise_if_user_doesnt_have_permissions_after(
+ cls,
+ workspace: Workspace,
+ user: AbstractUser,
+ field: Field,
+ field_cache: FieldCache,
+ field_operation_name: str,
+ ):
+ new_dependencies = FieldDependencyHandler.rebuild_dependencies(
+ field, field_cache
+ )
+ FieldDependencyHandler.raise_if_user_doesnt_have_operation_on_dependencies_in_other_tables(
+ workspace, user, field, new_dependencies, field_operation_name
+ )
+
+ @classmethod
+ def raise_if_user_doesnt_have_operation_on_dependencies_in_other_tables(
+ cls,
+ workspace: Workspace,
+ user: AbstractUser,
+ field: Field,
+ dependencies: List[FieldDependency],
+ field_operation_name: str,
+ ):
+ """
+ For a list of dependencies checks the provided user has the provided operation
+ on each of the dependency fields raising if they do not. It skips any
+ dependencies that point at the same table assuming the user already has rights
+ on fields in the table `field` is in.
+
+ :param workspace: The workspace.
+ :param user: The user.
+ :param field: The field.
+ :param dependencies: The dependencies.
+ :param field_operation_name: The field operation name.
+ """
+
+ perm_checks = []
+ for dep in dependencies:
+ dependency_field = dep.dependency
+ if dependency_field.table_id != field.table_id:
+ perm_checks.append(
+ PermissionCheck(
+ actor=user,
+ operation_name=field_operation_name,
+ context=dependency_field,
+ )
+ )
+
+ changed_field_type = field_type_registry.get_by_model(field)
+ for (
+ check,
+ result,
+ ) in (
+ CoreHandler().check_multiple_permissions(perm_checks, workspace).items()
+ ):
+ if not result:
+ raise changed_field_type.get_permission_error_when_user_changes_field_to_depend_on_forbidden_field(
+ check.actor, field, check.context
+ )
diff --git a/backend/src/baserow/contrib/database/fields/field_types.py b/backend/src/baserow/contrib/database/fields/field_types.py
index 9b88f33ce..ea5c46b39 100755
--- a/backend/src/baserow/contrib/database/fields/field_types.py
+++ b/backend/src/baserow/contrib/database/fields/field_types.py
@@ -3739,6 +3739,20 @@ class FormulaFieldType(ReadOnlyFieldType):
def can_represent_date(self, field: "Field") -> bool:
return self.to_baserow_formula_type(field.specific).can_represent_date
+ def get_permission_error_when_user_changes_field_to_depend_on_forbidden_field(
+ self, user: AbstractUser, changed_field: Field, forbidden_field: Field
+ ) -> Exception:
+ from baserow.contrib.database.formula import (
+ InvalidFormulaType,
+ get_invalid_field_and_table_formula_error,
+ )
+
+ return InvalidFormulaType(
+ get_invalid_field_and_table_formula_error(
+ forbidden_field.name, forbidden_field.table.name
+ )
+ )
+
class CountFieldType(FormulaFieldType):
type = "count"
diff --git a/backend/src/baserow/contrib/database/fields/handler.py b/backend/src/baserow/contrib/database/fields/handler.py
index e62df1b29..02ea76a5a 100644
--- a/backend/src/baserow/contrib/database/fields/handler.py
+++ b/backend/src/baserow/contrib/database/fields/handler.py
@@ -43,6 +43,7 @@ from baserow.contrib.database.fields.operations import (
CreateFieldOperationType,
DeleteFieldOperationType,
DuplicateFieldOperationType,
+ ReadFieldOperationType,
UpdateFieldOperationType,
)
from baserow.contrib.database.table.models import Table
@@ -308,7 +309,9 @@ class FieldHandler(metaclass=baserow_trace_methods(tracer)):
field_cache = FieldCache()
instance.save(field_cache=field_cache, raise_if_invalid=True)
- FieldDependencyHandler.rebuild_dependencies(instance, field_cache)
+ FieldDependencyHandler.rebuild_or_raise_if_user_doesnt_have_permissions_after(
+ workspace, user, instance, field_cache, ReadFieldOperationType.type
+ )
# Add the field to the table schema.
with safe_django_schema_editor(atomic=False) as schema_editor:
@@ -458,7 +461,9 @@ class FieldHandler(metaclass=baserow_trace_methods(tracer)):
field = set_allowed_attrs(field_values, allowed_fields, field)
field.save(field_cache=field_cache, raise_if_invalid=True)
- FieldDependencyHandler.rebuild_dependencies(field, field_cache)
+ FieldDependencyHandler.rebuild_or_raise_if_user_doesnt_have_permissions_after(
+ workspace, user, field, field_cache, ReadFieldOperationType.type
+ )
# If no converter is found we are going to convert to field using the
# lenient schema editor which will alter the field's type and set the data
# value to null if it can't be converted.
diff --git a/backend/src/baserow/contrib/database/fields/registries.py b/backend/src/baserow/contrib/database/fields/registries.py
index e45596ff1..8f2965386 100644
--- a/backend/src/baserow/contrib/database/fields/registries.py
+++ b/backend/src/baserow/contrib/database/fields/registries.py
@@ -1,6 +1,7 @@
from typing import TYPE_CHECKING, Any, Dict, List, NoReturn, Optional, Union
from zipfile import ZipFile
+from django.contrib.auth.models import AbstractUser
from django.contrib.postgres.fields import ArrayField, JSONField
from django.core.exceptions import ValidationError
from django.core.files.storage import Storage
@@ -1517,6 +1518,21 @@ class FieldType(
return False
+ def get_permission_error_when_user_changes_field_to_depend_on_forbidden_field(
+ self, user: AbstractUser, changed_field: Field, forbidden_field: Field
+ ) -> Exception:
+ """
+ Called when the field has been created or changed in a way that resulted in
+ it depending on another field in a way that was forbidden for the user
+ who triggered the change.
+
+ :param user: The user.
+ :param changed_field: The changed field.
+ :param forbidden_field: The forbidden field.
+ """
+
+ return PermissionError(user)
+
class ReadOnlyFieldHasNoInternalDbValueError(Exception):
"""
diff --git a/backend/src/baserow/contrib/database/formula/__init__.py b/backend/src/baserow/contrib/database/formula/__init__.py
index f344d1cfe..66d24f821 100644
--- a/backend/src/baserow/contrib/database/formula/__init__.py
+++ b/backend/src/baserow/contrib/database/formula/__init__.py
@@ -34,6 +34,10 @@ allowing use of that language in Baserow easily.
from baserow.contrib.database.formula.ast.tree import BaserowExpression
from baserow.contrib.database.formula.handler import FormulaHandler
from baserow.contrib.database.formula.operations import TypeFormulaOperationType
+from baserow.contrib.database.formula.types.exceptions import (
+ InvalidFormulaType,
+ get_invalid_field_and_table_formula_error,
+)
from baserow.contrib.database.formula.types.formula_type import (
BaserowFormulaInvalidType,
BaserowFormulaType,
@@ -71,4 +75,6 @@ __all__ = [
BASEROW_FORMULA_ARRAY_TYPE_CHOICES,
literal,
TypeFormulaOperationType,
+ InvalidFormulaType,
+ get_invalid_field_and_table_formula_error,
]
diff --git a/backend/src/baserow/contrib/database/formula/types/exceptions.py b/backend/src/baserow/contrib/database/formula/types/exceptions.py
index 1bb83cf2e..d1f50e3a0 100644
--- a/backend/src/baserow/contrib/database/formula/types/exceptions.py
+++ b/backend/src/baserow/contrib/database/formula/types/exceptions.py
@@ -10,3 +10,12 @@ class UnknownFormulaType(BaserowFormulaException):
super().__init__(
f"unknown formula type found on formula field of {unknown_formula_type}"
)
+
+
+def get_invalid_field_and_table_formula_error(
+ unknown_field_name: str, table_name: str
+) -> str:
+ return (
+ f"references the deleted or unknown field {unknown_field_name} in table "
+ f"{table_name}"
+ )
diff --git a/backend/src/baserow/contrib/database/formula/types/visitors.py b/backend/src/baserow/contrib/database/formula/types/visitors.py
index aba472ec2..d7a92a9f9 100644
--- a/backend/src/baserow/contrib/database/formula/types/visitors.py
+++ b/backend/src/baserow/contrib/database/formula/types/visitors.py
@@ -16,6 +16,9 @@ from baserow.contrib.database.formula.ast.tree import (
BaserowStringLiteral,
)
from baserow.contrib.database.formula.ast.visitors import BaserowFormulaASTVisitor
+from baserow.contrib.database.formula.types.exceptions import (
+ get_invalid_field_and_table_formula_error,
+)
from baserow.contrib.database.formula.types.formula_type import (
BaserowFormulaValidType,
UnTyped,
@@ -254,9 +257,9 @@ class FormulaTypingVisitor(
)
if target_field is None:
return field_reference.with_invalid_type(
- f"references the deleted or unknown field"
- f" {field_reference.target_field} in table "
- f"{target_table.name}"
+ get_invalid_field_and_table_formula_error(
+ field_reference.target_field, target_table.name
+ )
)
else:
return self._create_lookup_reference(
diff --git a/changelog/entries/unreleased/bug/1883_fix_allowing_builders_to_write_lookup_formulas_onto_tables_i.json b/changelog/entries/unreleased/bug/1883_fix_allowing_builders_to_write_lookup_formulas_onto_tables_i.json
new file mode 100644
index 000000000..643022802
--- /dev/null
+++ b/changelog/entries/unreleased/bug/1883_fix_allowing_builders_to_write_lookup_formulas_onto_tables_i.json
@@ -0,0 +1,7 @@
+{
+ "type": "bug",
+ "message": "Fix allowing builders to create or edit lookup formulas to target fields in tables where they dont have access",
+ "issue_number": 1883,
+ "bullet_points": [],
+ "created_at": "2023-08-03"
+}
\ No newline at end of file
diff --git a/enterprise/backend/tests/baserow_enterprise_tests/fields/test_link_row_field_rbac.py b/enterprise/backend/tests/baserow_enterprise_tests/fields/test_link_row_field_rbac.py
index 9b18205c6..dab60d491 100644
--- a/enterprise/backend/tests/baserow_enterprise_tests/fields/test_link_row_field_rbac.py
+++ b/enterprise/backend/tests/baserow_enterprise_tests/fields/test_link_row_field_rbac.py
@@ -4,6 +4,7 @@ This file tests the link row field in combination with RBAC enabled
import pytest
from baserow.contrib.database.fields.handler import FieldHandler
+from baserow.contrib.database.formula import InvalidFormulaType
from baserow.core.exceptions import PermissionDenied
from baserow_enterprise.role.handler import RoleAssignmentHandler
from baserow_enterprise.role.models import Role
@@ -137,3 +138,128 @@ def test_link_row_field_linked_to_table_with_no_access_from_inaccessible_to_acce
field=link_row_field.specific,
link_row_table=table_unrelated,
)
+
+
+@pytest.mark.django_db
+def test_cant_create_lookup_at_table_where_not_viewer_or_higher(
+ data_fixture,
+):
+ user_without_access = data_fixture.create_user()
+ user_with_access = data_fixture.create_user()
+ workspace = data_fixture.create_workspace(
+ user=user_with_access, members=[user_without_access]
+ )
+ database = data_fixture.create_database_application(workspace=workspace)
+ table_with_access = data_fixture.create_database_table(
+ user_without_access, database=database
+ )
+ table_with_no_access = data_fixture.create_database_table(
+ user_without_access, database=database
+ )
+ private_field_in_no_access_table = data_fixture.create_text_field(
+ user_with_access, table=table_with_no_access, name="private"
+ )
+ no_access = Role.objects.get(uid="NO_ACCESS")
+ viewer_role = Role.objects.get(uid="VIEWER")
+
+ RoleAssignmentHandler().assign_role(
+ user_without_access, workspace, role=no_access, scope=table_with_no_access
+ )
+
+ link_row_field = FieldHandler().create_field(
+ user=user_with_access,
+ table=table_with_access,
+ type_name="link_row",
+ name="link row",
+ link_row_table=table_with_no_access,
+ )
+
+ with pytest.raises(InvalidFormulaType):
+ FieldHandler().create_field(
+ user=user_without_access,
+ table=table_with_access,
+ type_name="lookup",
+ name="shouldnt be able to create",
+ target_field_name=private_field_in_no_access_table.name,
+ through_field_name=link_row_field.name,
+ )
+
+ # Now make them a viewer and it should work
+ RoleAssignmentHandler().assign_role(
+ user_without_access, workspace, role=viewer_role, scope=table_with_no_access
+ )
+
+ FieldHandler().create_field(
+ user=user_without_access,
+ table=table_with_access,
+ type_name="lookup",
+ name="now it will work",
+ target_field_name=private_field_in_no_access_table.name,
+ through_field_name=link_row_field.name,
+ )
+
+
+@pytest.mark.django_db
+def test_cant_update_target_lookup_point_at_table_where_not_viewer_or_higher(
+ data_fixture,
+):
+ user_without_access = data_fixture.create_user()
+ user_with_access = data_fixture.create_user()
+ workspace = data_fixture.create_workspace(
+ user=user_with_access, members=[user_without_access]
+ )
+ database = data_fixture.create_database_application(workspace=workspace)
+ table_with_access = data_fixture.create_database_table(
+ user_without_access, database=database
+ )
+ table_with_no_access = data_fixture.create_database_table(
+ user_without_access, database=database
+ )
+ private_field_in_no_access_table = data_fixture.create_text_field(
+ user_with_access, table=table_with_no_access, name="private"
+ )
+ other_private_field_in_no_access_table = data_fixture.create_text_field(
+ user_with_access, table=table_with_no_access, name="other private"
+ )
+ no_access = Role.objects.get(uid="NO_ACCESS")
+ viewer_role = Role.objects.get(uid="VIEWER")
+
+ RoleAssignmentHandler().assign_role(
+ user_without_access, workspace, role=no_access, scope=table_with_no_access
+ )
+
+ link_row_field = FieldHandler().create_field(
+ user=user_with_access,
+ table=table_with_access,
+ type_name="link_row",
+ name="link row",
+ link_row_table=table_with_no_access,
+ )
+ lookup_field = FieldHandler().create_field(
+ user=user_with_access,
+ table=table_with_access,
+ type_name="lookup",
+ name="lookup",
+ target_field_id=private_field_in_no_access_table.id,
+ through_field_id=link_row_field.id,
+ )
+
+ with pytest.raises(InvalidFormulaType):
+ # The user without the access tries to point the lookup at a different field
+ FieldHandler().update_field(
+ user=user_without_access,
+ field=lookup_field,
+ target_field_id=other_private_field_in_no_access_table.id,
+ through_field_id=link_row_field.id,
+ )
+
+ # Now make them a viewer and it should work
+ RoleAssignmentHandler().assign_role(
+ user_without_access, workspace, role=viewer_role, scope=table_with_no_access
+ )
+ FieldHandler().update_field(
+ user=user_without_access,
+ field=lookup_field,
+ target_field_name=other_private_field_in_no_access_table.name,
+ through_field_name=link_row_field.name,
+ )
diff --git a/enterprise/web-frontend/modules/baserow_enterprise/components/rbac/SelectSubjectsListFooter.vue b/enterprise/web-frontend/modules/baserow_enterprise/components/rbac/SelectSubjectsListFooter.vue
index 1a4e6ec37..7f84e44ab 100644
--- a/enterprise/web-frontend/modules/baserow_enterprise/components/rbac/SelectSubjectsListFooter.vue
+++ b/enterprise/web-frontend/modules/baserow_enterprise/components/rbac/SelectSubjectsListFooter.vue
@@ -5,14 +5,6 @@
>
<div v-if="showRoleSelector" class="select-list-footer__left-side">
<RoleSelector v-model="roleSelected" :roles="roles" />
- <HelpIcon
- v-if="roleSelectedExposesData"
- class="margin-left-1"
- :tooltip="
- $t('selectSubjectsListFooter.roleSelectorBuilderDataWarningTooltip')
- "
- is-warning
- />
</div>
<div>
<HelpIcon
@@ -88,12 +80,6 @@ export default {
inviteEnabled() {
return this.count !== 0
},
- roleSelectedExposesData() {
- return (
- ['ADMIN', 'BUILDER'].includes(this.roleSelected.uid) &&
- this.scopeType === 'database_table'
- )
- },
},
mounted() {
// Set a default selected role, the last role is usually the one with the least
diff --git a/enterprise/web-frontend/modules/baserow_enterprise/locales/en.json b/enterprise/web-frontend/modules/baserow_enterprise/locales/en.json
index 25b8ca29d..d7ee8628d 100644
--- a/enterprise/web-frontend/modules/baserow_enterprise/locales/en.json
+++ b/enterprise/web-frontend/modules/baserow_enterprise/locales/en.json
@@ -238,8 +238,7 @@
"members": "members",
"teams": "teams"
},
- "helpTooltip": "Member role assignments override team role assignments.",
- "roleSelectorBuilderDataWarningTooltip": "They will be able to create formula and lookup fields exposing any fields in any linked tables."
+ "helpTooltip": "Member role assignments override team role assignments."
},
"membersRoleField": {
"noAccessHelpText": "This user requires explicit access to resources.",
diff --git a/web-frontend/modules/database/components/field/FieldSelectTargetFieldSubForm.vue b/web-frontend/modules/database/components/field/FieldSelectTargetFieldSubForm.vue
index d597cdc55..1ec1c745c 100644
--- a/web-frontend/modules/database/components/field/FieldSelectTargetFieldSubForm.vue
+++ b/web-frontend/modules/database/components/field/FieldSelectTargetFieldSubForm.vue
@@ -84,7 +84,7 @@ export default {
return (
this.linkedToTable &&
this.$hasPermission(
- 'database.table.list_fields',
+ 'database.table.create_field',
this.linkedToTable,
this.database.workspace.id
)
@@ -115,6 +115,13 @@ export default {
.get('field', f.type)
.canBeReferencedByFormulaField()
})
+ .filter((f) => {
+ return this.$hasPermission(
+ 'database.table.field.update',
+ f,
+ this.database.workspace.id
+ )
+ })
.map((f) => {
const fieldType = this.$registry.get('field', f.type)
f.icon = fieldType.getIconClass()
diff --git a/web-frontend/modules/database/components/field/FieldSelectThroughFieldSubForm.vue b/web-frontend/modules/database/components/field/FieldSelectThroughFieldSubForm.vue
index 5605f576f..920b247f5 100644
--- a/web-frontend/modules/database/components/field/FieldSelectThroughFieldSubForm.vue
+++ b/web-frontend/modules/database/components/field/FieldSelectThroughFieldSubForm.vue
@@ -77,7 +77,16 @@ export default {
return this.$store.getters['application/getAll'].reduce(
(tables, application) => {
if (application.type === databaseType) {
- return tables.concat(application.tables || [])
+ const tablesWithCreateFieldAccess = (
+ application.tables || []
+ ).filter((table) =>
+ this.$hasPermission(
+ 'database.table.create_field',
+ table,
+ this.database.workspace.id
+ )
+ )
+ return tables.concat(tablesWithCreateFieldAccess)
}
return tables
},