Make strawberry.Private compatible with PEP-563 (strawberry-graphql…

…#1684)

* Make `strawberry.Private` compatible with PEP-563

This commit Closes strawberry-graphql#1586 by checking for private fields at schema
conversion time. As per [PEP-563], in the future Python annotations will
no longer be evaluated at definition time. As reported by Issue strawberry-graphql#1586,
this means that `strawberry.Private` is incompatible with postponed
evaluation as the check for a private field requires an evaluated
type-hint annotation to work.

By checking for private fields at schema conversion time, it is
guaranteed that all fields that should be included in the schema are
resolvable using an eval. This ensures that the current approach for
defining private fields can be left intact.

The current filtering for fields annotated with `strawberry.Private` in
`types.type_resolver.py` are left intact to not needlessly instantiate
`StrawberryField` objects when `strawberry.Private` is resolvable.

Summary of Changes:
- Added check for private fields at schema evaluation time
- Added test to check that postponed evaluation with
  `strawberry.Private` functions correctly
- Reduced code duplication in `schema_converter.py`

[PEP-563]: https://www.python.org/dev/peps/pep-0563/

* Add RELEASE.md

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add docstring to `_get_thunk_mapping`

* Improve testing of private fields

This commit mainly addresses a limitation of PEP-563 wherby annotations
tagetting classes defined outside of module-level scope (i.e. a
function) cannot be resolved. As a result, even at schema-conversion
time, checking `is_private` is not guaranteed to evaluate to a valid
type. As a result, a test is added to ensure that a `strawberry.Private`
annotated field that forward-references a locally-defined class does not
appear in the generated Schema. Although this is a failing test for now
it demonstrates the issue to prevent a potential security vulnerability.

Summary of Changes:
- Check that the `notSeen` private field does not exist in the schema
  and that querying it returns no data.
- Add failing test to show issue of classes defined outside module-scope
- Move import of dataclass to the top of the file
- Correct typo in the word `accessible`

* Prevent unresolved types from entering the schema

This commit changes `StrawberryField.type` to return a new sentinel
`UNRESOLVED` instead of `None` to make sure that unresolvable types such
as those defined within a local scope of a function do not appear in the
schema as a `Void` field. With the new behavior a `TypeError` will be
raised during schema conversion if any types are still unresolved. Most
importantly, this new behavior prevents a security vulnerability where
`Strawberry.Private` annotated fields would previously appear in the
schema as a `Void` field; thereby, becomming executible and potentially
leaking sensitive data in the resultant error message.

Summary of Changes:
- Return a new sentinel `UNRESOLVED` from `StrawberryField.type` instead
  of `None`
- Update `GraphQLCoreConverter._get_thunk_mapping` to raise a
  `TypeError` if any unresolved types exist during schema conversion
- Update failing `test_private_field_defined_outside_module_scope` to
  check if the correct `TypeError` is raised and to further ensure
  that if no error is raised, that the `Strawberry.Private` annotated
  field does not enter the schema.

* Make `strawberry.Private` compatible with PEP-563

This commit Closes strawberry-graphql#1586 by checking for private fields at schema
conversion time. As per [PEP-563], in the future Python annotations will
no longer be evaluated at definition time. As reported by Issue strawberry-graphql#1586,
this means that `strawberry.Private` is incompatible with postponed
evaluation as the check for a private field requires an evaluated
type-hint annotation to work.

By checking for private fields at schema conversion time, it is
guaranteed that all fields that should be included in the schema are
resolvable using an eval. This ensures that the current approach for
defining private fields can be left intact.

The current filtering for fields annotated with `strawberry.Private` in
`types.type_resolver.py` are left intact to not needlessly instantiate
`StrawberryField` objects when `strawberry.Private` is resolvable.

Summary of Changes:
- Added check for private fields at schema evaluation time
- Added test to check that postponed evaluation with
  `strawberry.Private` functions correctly
- Reduced code duplication in `schema_converter.py`

[PEP-563]: https://www.python.org/dev/peps/pep-0563/

* Create custom exception for unresolved field types

* Remove dependency on sentinel package

* Fix sentinel

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Patrick Arminio <[email protected]>

strawberry/exceptions.py

@@ -131,6 +131,15 @@ def __init__(self, annotation):
         super().__init__(message)
 
 
+class UnresolvedFieldTypeError(Exception):
+    def __init__(self, field_name: str):
+        message = (
+            f"Could not resolve the type of '{field_name}'. Check that the class is "
+            "accessible from the global module scope."
+        )
+        super().__init__(message)
+
+
 class MissingFieldAnnotationError(Exception):
     def __init__(self, field_name: str):
         message = (

strawberry/field.py

@@ -40,6 +40,9 @@
 _RESOLVER_TYPE = Union[StrawberryResolver, Callable, staticmethod, classmethod]
 
 
+UNRESOLVED = object()
+
+
 class StrawberryField(dataclasses.Field):
     python_name: str
 
@@ -192,7 +195,7 @@ def base_resolver(self, resolver: StrawberryResolver) -> None:
         _ = resolver.arguments
 
     @property  # type: ignore
-    def type(self) -> Union[StrawberryType, type]:  # type: ignore
+    def type(self) -> Union[StrawberryType, type, Literal[UNRESOLVED]]:  # type: ignore
         # We are catching NameError because dataclasses tries to fetch the type
         # of the field from the class before the class is fully defined.
         # This triggers a NameError error when using forward references because
@@ -212,7 +215,7 @@ def type(self) -> Union[StrawberryType, type]:  # type: ignore
 
             return self.type_annotation.resolve()
         except NameError:
-            return None  # type: ignore
+            return UNRESOLVED
 
     @type.setter
     def type(self, type_: Any) -> None:

strawberry/schema/schema_converter.py

@@ -1,7 +1,18 @@
 from __future__ import annotations
 
 from enum import Enum
-from typing import Any, Callable, Dict, List, Optional, Tuple, Type, Union, cast
+from typing import (
+    Any,
+    Callable,
+    Dict,
+    List,
+    Optional,
+    Tuple,
+    Type,
+    TypeVar,
+    Union,
+    cast,
+)
 
 from graphql import (
     GraphQLArgument,
@@ -31,9 +42,11 @@
 from strawberry.exceptions import (
     MissingTypesForGenericError,
     ScalarAlreadyRegisteredError,
+    UnresolvedFieldTypeError,
 )
-from strawberry.field import StrawberryField
+from strawberry.field import UNRESOLVED, StrawberryField
 from strawberry.lazy_type import LazyType
+from strawberry.private import is_private
 from strawberry.schema.config import StrawberryConfig
 from strawberry.schema.types.scalar import _make_scalar_type
 from strawberry.type import StrawberryList, StrawberryOptional, StrawberryType
@@ -166,6 +179,53 @@ def from_input_field(self, field: StrawberryField) -> GraphQLInputField:
             deprecation_reason=field.deprecation_reason,
         )
 
+    FieldType = TypeVar("FieldType", GraphQLField, GraphQLInputField)
+
+    @staticmethod
+    def _get_thunk_mapping(
+        fields: List[StrawberryField],
+        name_converter: Callable[[StrawberryField], str],
+        field_converter: Callable[[StrawberryField], FieldType],
+    ) -> Dict[str, FieldType]:
+        """Create a GraphQL core `ThunkMapping` mapping of field names to field types.
+
+        This method filters out remaining `strawberry.Private` annotated fields that
+        could not be filtered during the initialization of a `TypeDefinition` due to
+        postponed type-hint evaluation (PEP-563). Performing this filtering now (at
+        schema conversion time) ensures that all types to be included in the schema
+        should have already been resolved.
+
+        Raises:
+            TypeError: If the type of a field in ``fields`` is `UNRESOLVED`
+        """
+        thunk_mapping = {}
+
+        for f in fields:
+            if f.type is UNRESOLVED:
+                raise UnresolvedFieldTypeError(f.name)
+
+            if not is_private(f.type):
+                thunk_mapping[name_converter(f)] = field_converter(f)
+        return thunk_mapping
+
+    def get_graphql_fields(
+        self, type_definition: TypeDefinition
+    ) -> Dict[str, GraphQLField]:
+        return self._get_thunk_mapping(
+            fields=type_definition.fields,
+            name_converter=self.config.name_converter.from_field,
+            field_converter=self.from_field,
+        )
+
+    def get_graphql_input_fields(
+        self, type_definition: TypeDefinition
+    ) -> Dict[str, GraphQLInputField]:
+        return self._get_thunk_mapping(
+            fields=type_definition.fields,
+            name_converter=self.config.name_converter.from_field,
+            field_converter=self.from_input_field,
+        )
+
     def from_input_object(self, object_type: type) -> GraphQLInputObjectType:
         type_definition = object_type._type_definition  # type: ignore
 
@@ -177,18 +237,9 @@ def from_input_object(self, object_type: type) -> GraphQLInputObjectType:
             assert isinstance(graphql_object_type, GraphQLInputObjectType)  # For mypy
             return graphql_object_type
 
-        def get_graphql_fields() -> Dict[str, GraphQLInputField]:
-            graphql_fields = {}
-            for field in type_definition.fields:
-                field_name = self.config.name_converter.from_field(field)
-
-                graphql_fields[field_name] = self.from_input_field(field)
-
-            return graphql_fields
-
         graphql_object_type = GraphQLInputObjectType(
             name=type_name,
-            fields=get_graphql_fields,
+            fields=lambda: self.get_graphql_input_fields(type_definition),
             description=type_definition.description,
         )
 
@@ -209,18 +260,9 @@ def from_interface(self, interface: TypeDefinition) -> GraphQLInterfaceType:
             assert isinstance(graphql_interface, GraphQLInterfaceType)  # For mypy
             return graphql_interface
 
-        def get_graphql_fields() -> Dict[str, GraphQLField]:
-            graphql_fields = {}
-
-            for field in interface.fields:
-                field_name = self.config.name_converter.from_field(field)
-                graphql_fields[field_name] = self.from_field(field)
-
-            return graphql_fields
-
         graphql_interface = GraphQLInterfaceType(
             name=interface_name,
-            fields=get_graphql_fields,
+            fields=lambda: self.get_graphql_fields(interface),
             interfaces=list(map(self.from_interface, interface.interfaces)),
             description=interface.description,
         )
@@ -246,16 +288,6 @@ def from_object(self, object_type: TypeDefinition) -> GraphQLObjectType:
             assert isinstance(graphql_object_type, GraphQLObjectType)  # For mypy
             return graphql_object_type
 
-        def get_graphql_fields() -> Dict[str, GraphQLField]:
-            graphql_fields = {}
-
-            for field in object_type.fields:
-                field_name = self.config.name_converter.from_field(field)
-
-                graphql_fields[field_name] = self.from_field(field)
-
-            return graphql_fields
-
         is_type_of: Optional[Callable[[Any, GraphQLResolveInfo], bool]]
         if object_type.is_type_of:
             is_type_of = object_type.is_type_of
@@ -269,7 +301,7 @@ def is_type_of(obj: Any, _info: GraphQLResolveInfo) -> bool:
 
         graphql_object_type = GraphQLObjectType(
             name=object_type_name,
-            fields=get_graphql_fields,
+            fields=lambda: self.get_graphql_fields(object_type),
             interfaces=list(map(self.from_interface, object_type.interfaces)),
             description=object_type.description,
             is_type_of=is_type_of,

tests/schema/test_private_field.py

@@ -1,3 +1,5 @@
+from dataclasses import dataclass
+
 import pytest
 
 import strawberry
@@ -55,3 +57,60 @@ def age_in_months(self) -> int:
     assert result.data == {
         "ageInMonths": 84,
     }
+
+
+@strawberry.type
+class Query:
+    not_seen: "strawberry.Private[SensitiveData]"
+
+    @strawberry.field
+    def accessible_info(self) -> str:
+        return self.not_seen.info
+
+
+@dataclass
+class SensitiveData:
+    value: int
+    info: str
+
+
+def test_private_field_with_str_annotations():
+    """Check compatibility of strawberry.Private with annotations as string."""
+
+    schema = strawberry.Schema(query=Query)
+
+    result = schema.execute_sync(
+        "query { accessibleInfo }",
+        root_value=Query(not_seen=SensitiveData(1, "foo")),
+    )
+    assert result.data == {"accessibleInfo": "foo"}
+
+    # Check if querying `notSeen` raises error and no data is returned
+    assert "notSeen" not in str(schema)
+    failed_result = schema.execute_sync(
+        "query { notSeen }", root_value=Query(not_seen=SensitiveData(1, "foo"))
+    )
+    assert failed_result.data is None
+
+
+def test_private_field_defined_outside_module_scope():
+    """Check compatibility of strawberry.Private when defined outside module scope."""
+
+    @strawberry.type
+    class LocallyScopedQuery:
+        not_seen: "strawberry.Private[LocallyScopedSensitiveData]"
+
+        @strawberry.field
+        def accessible_info(self) -> str:
+            return self.not_seen.info
+
+    @dataclass
+    class LocallyScopedSensitiveData:
+        value: int
+        info: str
+
+    with pytest.raises(TypeError, match=r"Could not resolve the type of 'not_seen'."):
+        schema = strawberry.Schema(query=LocallyScopedQuery)
+
+        # If schema-conversion does not raise, check if private field is visible
+        assert "notSeen" not in str(schema)