Resolve "Fix allowing builders to write lookup formulas onto tables into which they don't have viewer or higher rights"

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

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

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"

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.

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):
     """

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

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}"
+    )

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(

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"
+}

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

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

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.",

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()

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
         },