strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
https://strawberry.rocks/docs/django
MIT License
411 stars 118 forks source link

Create mutation persists object even if HasRetvalPerm check fails #344

Closed Mapiarz closed 1 year ago

Mapiarz commented 1 year ago

Describe the Bug

I have a mutation like this:

create_crew: Crew = strawberry_django.mutations.create(
    CrewInput, extensions=[permissions.HasRetvalPerm(models.Crew.get_perm("create"))]
)

I make a request and get a response like this:

{
  "data": {
    "createCrew": {
      "__typename": "OperationInfo",
      "messages": [
        {
          "message": "You don't have permission to access this app."
        }
      ]
    }
  }
}

Yet the actual object has been created and persisted in the DB. I have tested further and update and delete mutations seem to be working correctly and abort the transaction if the permission check fails.

System Information

Upvote & Fund

Fund with Polar

vecchp commented 1 year ago

Perhaps the HasRetVal is not meant to be used in this case? After reading the docs this is my understanding:

I believe the permission check using HasRetvalPerm is probably happening after the mutation's execution. The mutation is creating the object and then checking permissions on the resultant object (return value). This could explain why the object is being persisted even if the permission check fails.

Mapiarz commented 1 year ago

Yeah well it seems that the transaction rollback for the update/delete mutations that I was seeing is not really intended behavior. It's not meant to be used with mutations, really (which is a bummer). In fact, the HasRetvalPerm is very limited in how you can apply it. It's either for single object instances or lists if you use django-guardian.

Maybe in the future we can have a mechanism which allows us to rollback the transaction after a resolver has exited.

bellini666 commented 1 year ago

Maybe in the future we can have a mechanism which allows us to rollback the transaction after a resolver has exited.

As I mentioned on discord, feel free to send a PR adding such a feature and I'll gladly review it :)

Ping me there if you need help on writing it