Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fk resolver permissions leak #1411

Merged
merged 8 commits into from
Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ __pycache__/
# Distribution / packaging
.Python
env/
.env/
build/
develop-eggs/
dist/
Expand Down
127 changes: 124 additions & 3 deletions graphene_django/converter.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import inspect
from collections import OrderedDict
from functools import singledispatch, wraps
from functools import partial, singledispatch, wraps

from django.db import models
from django.utils.encoding import force_str
Expand All @@ -25,6 +26,7 @@
)
from graphene.types.json import JSONString
from graphene.types.scalars import BigInt
from graphene.types.resolver import get_default_resolver
from graphene.utils.str_converters import to_camel_case
from graphql import GraphQLError

Expand Down Expand Up @@ -258,14 +260,60 @@ def convert_time_to_string(field, registry=None):

@convert_django_field.register(models.OneToOneRel)
def convert_onetoone_field_to_djangomodel(field, registry=None):
from graphene.utils.str_converters import to_snake_case
from .types import DjangoObjectType

model = field.related_model

def dynamic_type():
_type = registry.get_type_for_model(model)
if not _type:
return

return Field(_type, required=not field.null)
class CustomField(Field):
def wrap_resolve(self, parent_resolver):
"""
Implements a custom resolver which goes through the `get_node` method to ensure that
it goes through the `get_queryset` method of the DjangoObjectType.
"""
resolver = super().wrap_resolve(parent_resolver)

# If `get_queryset` was not overridden in the DjangoObjectType,
# we can just return the default resolver.
if (
_type.get_queryset.__func__
is DjangoObjectType.get_queryset.__func__
):
return resolver

def custom_resolver(root, info, **args):
# Note: this function is used to resolve 1:1 relation fields

is_resolver_awaitable = inspect.iscoroutinefunction(resolver)

if is_resolver_awaitable:
fk_obj = resolver(root, info, **args)
# In case the resolver is a custom awaitable resolver that overwrites
# the default Django resolver
return fk_obj

field_name = to_snake_case(info.field_name)
reversed_field_name = root.__class__._meta.get_field(
field_name
).remote_field.name
return _type.get_queryset(
_type._meta.model.objects.filter(
**{reversed_field_name: root.pk}
),
info,
).get()

return custom_resolver

return CustomField(
_type,
required=not field.null,
)

return Dynamic(dynamic_type)

Expand Down Expand Up @@ -313,14 +361,87 @@ def dynamic_type():
@convert_django_field.register(models.OneToOneField)
@convert_django_field.register(models.ForeignKey)
def convert_field_to_djangomodel(field, registry=None):
from graphene.utils.str_converters import to_snake_case
from .types import DjangoObjectType

model = field.related_model

def dynamic_type():
_type = registry.get_type_for_model(model)
if not _type:
return

return Field(
class CustomField(Field):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a general comment (and I don't mean this as a slight in any way), the code to achieve this behavior feels quite complex/confusing to me, bordering on it seeming like a code smell. e.g., are we properly handling all scenarios with it? How brittle is it? But I'm still not super familiar with all of the internals of graphene-django, so perhaps that's just how something of this lower-level nature would have to be written. I'd be curious to see @mahdyhamad's thoughts, since you're incorporating some of his efforts he'd described in #1315 (comment) but hadn't completed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this needs to be tested further and maybe simplified/refactored.
Also, it seems that this part from #1361 by-passes get_queryset when the resolver is awaitable. I plan to tackle this in a future PR.

if is_resolver_awaitable:
    fk_obj = resolver(root, info, **args)
    # In case the resolver is a custom awaitable resolver that overwrites
    # the default Django resolver
    return fk_obj

def wrap_resolve(self, parent_resolver):
"""
Implements a custom resolver which goes through the `get_node` method to ensure that
it goes through the `get_queryset` method of the DjangoObjectType.
"""
resolver = super().wrap_resolve(parent_resolver)

# If `get_queryset` was not overridden in the DjangoObjectType,
# we can just return the default resolver.
if (
_type.get_queryset.__func__
is DjangoObjectType.get_queryset.__func__
):
return resolver

def custom_resolver(root, info, **args):
# Note: this function is used to resolve FK or 1:1 fields
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, if this is used to resolve 1:1 fields, why does convert_onetoone_field_to_djangomodel also need this similar custom_resolver overriding logic separately?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question; for each relation kind, we need to deal with both directions: forward (OneToOneField, ForeignKey, ManyToManyField) and backward (ManyToManyRel, ManyToOneRel, OneToOneRel). The resolving logic depends on which side you're working on, hence the separation.

# it does not differentiate between custom-resolved fields
# and default resolved fields.

# because this is a django foreign key or one-to-one field, the primary-key for
# this node can be accessed from the root node.
# ex: article.reporter_id

# get the name of the id field from the root's model
field_name = to_snake_case(info.field_name)
db_field_key = root.__class__._meta.get_field(field_name).attname
if hasattr(root, db_field_key):
# get the object's primary-key from root
object_pk = getattr(root, db_field_key)
else:
return None

is_resolver_awaitable = inspect.iscoroutinefunction(resolver)

if is_resolver_awaitable:
fk_obj = resolver(root, info, **args)
# In case the resolver is a custom awaitable resolver that overwrites
# the default Django resolver
return fk_obj

instance_from_get_node = _type.get_node(info, object_pk)

if instance_from_get_node is None:
# no instance to return
return
elif (
isinstance(resolver, partial)
and resolver.func is get_default_resolver()
):
return instance_from_get_node
elif resolver is not get_default_resolver():
# Default resolver is overridden
# For optimization, add the instance to the resolver
setattr(root, field_name, instance_from_get_node)
# Explanation:
# previously, _type.get_node` is called which results in at least one hit to the database.
# But, if we did not pass the instance to the root, calling the resolver will result in
# another call to get the instance which results in at least two database queries in total
# to resolve this node only.
# That's why the value of the object is set in the root so when the object is accessed
# in the resolver (root.field_name) it does not access the database unless queried explicitly.
fk_obj = resolver(root, info, **args)
return fk_obj
else:
return instance_from_get_node

return custom_resolver

return CustomField(
_type,
description=get_django_field_description(field),
required=not field.null,
Expand Down
Loading