-
Notifications
You must be signed in to change notification settings - Fork 766
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
Changes from 6 commits
edb9690
013dbbe
a2a95a4
5079a46
f80bf5f
ccce70e
91ddfbd
42d1621
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ __pycache__/ | |
# Distribution / packaging | ||
.Python | ||
env/ | ||
.env/ | ||
build/ | ||
develop-eggs/ | ||
dist/ | ||
|
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 | ||
|
@@ -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 | ||
|
||
|
@@ -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) | ||
|
||
|
@@ -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): | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( |
||
# 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, | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.