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

Allow enforcement of permissions for related fields in mutations #618

Open
1 of 3 tasks
SupImDos opened this issue Aug 27, 2024 · 2 comments
Open
1 of 3 tasks

Allow enforcement of permissions for related fields in mutations #618

SupImDos opened this issue Aug 27, 2024 · 2 comments
Labels
documentation Improvements or additions to documentation help wanted Extra attention is needed

Comments

@SupImDos
Copy link
Contributor

SupImDos commented Aug 27, 2024

Description

At a high-level our problem is:

  • We want to stop someone from setting a ForeignKey on a record that they do have access to, to a record that they do not have access to.
  • We're not really doing this because of what we perceive to be 'unique' business rules. It seems like it'd be fairly common for a data model to be such that it'd be undesired to, e.g., attach a 'child' record to a 'parent' that the user does not have access to, just because the user knows the parent record's ID (which is easily possible with e.g. sequential integer IDs).

We can't really do this kind of validation in our model-layer, as we need certain request context in order to determine if the user has access to the record they're trying to attach, such as the current user, and which related fields are actually being set. In other frameworks that we've used, we've been able to hook into the layer where primary keys are resolved to do this kind of validation, but this can't currently be done in strawberry-django.

Example

Take for example a simple project management application where users can create projects and assign tasks to those projects. Some models for this application might look like this:

class Project(models.Model):
    name = models.CharField()
    owner = models.ForeignKey(User, on_delete=models.CASCADE)

class Task(models.Model):
    name = models.CharField()
    description = models.TextField()
    project = models.ForeignKey(Project, on_delete=models.CASCADE)

We only want to allow users to create tasks for projects that they own. This means that when setting the project field on a task, the allowed values should only be the subset that they own:

@strawberry_django.type(Task)
class TaskType:
    id: strawberry.auto
    name: strawberry.auto
    description: strawberry.auto

@strawberry_django.input(Task)
class CreateTaskInput:
    name: strawberry.auto
    description: strawberry.auto
    project: strawberry.auto  # The allowed projects here should be filtered

@strawberry.type
class Mutation:
    create_task: Task = strawberry_django.mutations.create(CreateTaskInput)

Feature Request Type

  • Core functionality
  • Alteration (enhancement/optimization) of existing feature(s)
  • New behavior

Potential Solutions

At risk of muddying the waters, here are some potential approaches we see which could solve this (although we're open to other suggestions if they're more in line with the project's goals):

a) Allow the queryset for a related field to be customized

In some way or another, allow the queryset that primary keys are resolved from to be customized.
This would allow for the enforcement of permissions for related fields, and the flexibility of other filtering as well.

def my_queryset_func(queryset, info, **kwargs):
    user = get_current_user(info)
    return queryset.do_some_filtering_with_the_user(user=user)

@strawberry_django.input(MyModel)
class MyInputType:
    my_related_field: strawberry.auto = strawberry_django.field(queryset=my_queryset_func)

b) Allow a hook for resolving primary keys to be customized

In some way or another, allow the whole resolution of primary keys to be customized.
This would also allow for the enforcement of permissions for related fields, and the flexibility of other filtering as well.

def my_resolver_func(pk, info, **kwargs):
    obj = MyRelatedModel.objects.get(pk=pk)
    user = get_current_user(info)
    if not user.has_perm("myapp.view_myrelatedmodel", obj):
        raise PermissionDenied
    return obj

@strawberry_django.input(MyModel)
class MyInputType:
    my_related_field: strawberry.auto = strawberry_django.field(resolver=my_resolver_func)

c) Implement permissions field extensions for input fields

Continue the current pattern of using field extensions to add permissions to input fields.
This is consistent with the project's current approach, but doesn't provide the added flexibility of queryset filtering.

@strawberry_django.input(MyModel)
class MyInputType:
    my_related_field: strawberry.auto = strawberry_django.field(extensions=[HasPerm("myapp.view_myrelatedmodel")])

Setting aside doing the actual work, are any of these options inline with your vision for the project and how it should work? Otherwise, do have you any other ideas or suggestions?

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@SupImDos
Copy link
Contributor Author

SupImDos commented Sep 2, 2024

I've discovered that this functionality appears to (maybe incidentally) be possible when you use Relay.

For reference, take the following example:

@strawberry_django.type(MyParentModel)
class MyParentType(relay.Node)
    name: strawberry.auto

    @classmethod
    def get_queryset(cls, queryset, info, kwargs):
        user = get_current_user(info)
        return queryset.filter_for_permissions(user)


@strawberry_django.input(MyChildModel)
class MyChildInput(relay.NodeInput):
    name: strawberry.auto
    parent: strawberry.auto


@strawberry.type
class Mutation:
    create_child: MyChildType = strawberry_django.mutations.create(MyChildInput)

In this scenario, the parent field on the MyChildInput is a NodeInput (i.e., a Global ID) which is ultimately resolved via the MyParentType type using its get_queryset method. If the get_queryset method is set up to filter for permissions based on the current user, then you implicitly get the functionality requested in this ticket.

See:

So, its possible this issue is resolved, but maybe we could do with some documentation to make it clearer how this works?

@bellini666 bellini666 added documentation Improvements or additions to documentation help wanted Extra attention is needed labels Sep 24, 2024
@bellini666
Copy link
Member

@SupImDos sorry for taking long to reply, but glad you found out how to solve this.

So, its possible this issue is resolved, but maybe we could do with some documentation to make it clearer how this works?

Yes, strawberry-django unfortunately is lacking a lot of documentation. I'm marking this with a documentation label and will try to tackle this in the future. But anyone is very welcomed to open a PR to contribute to this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants
@bellini666 @SupImDos and others