graphql-ruby icon indicating copy to clipboard operation
graphql-ruby copied to clipboard

[PRO] Pundit authorization for a mutation creating a new record

Open thornomad opened this issue 5 years ago • 4 comments

I have another Pundit integration question. This relates to authenticating a create mutation. (I've had good look with authenticating input arguments and update, thanks to your help. See this example for what I mean)

With an update mutation there is a record instance to check the policy against based on the :id argument that loads the record instance; however, with a create, all we have is an Input object that has not yet turned into a record instance or been created.

class NotePolicy < ApplicationPolicy

  # An admin can create a note on behalf of someone else;
  # everyone else can only create their own notes
  def create?
    person.admin? || record.person == person
  end

end

class Mutations::NoteCreate < Mutations::BaseMutation
  type Types::NoteType

  pundit_role nil # Anyone can call the mutation at this point

  class NoteInput < Types::BaseInputObject
    argument :comment, String, required: true
    argument :person_id, ID, required: true
  end

  argument :input, NoteInput, required: true

  # NOTE: what I would would like to be adding to this :input argument call
  # would be something like a --> pundit_role: :create
  # but that requires initializing a new object from the input kwargs

  def resolve(input:)
    # not authorization happening at this point
    Note.create!(input.to_kwargs)
  end

end

The first pundit_role nil allows anyone to call the mutation, which is what we want; it's really the NoteInput that I would like to attach the pundit_role: :create too ... kind of like how it can be done in the update example linked above.

But in order for that to happen, NoteInput needs to initialize Note.new before it can check the :create?.

I imagined that to play this out manually in the resolve method it would look something like this:

def resolve(input:)
  note = Note.new(input.to_kwargs)
  Pundit.authorize(current_user, note, :create?)
  note.save!
  note
end

This authorizes things correctly, but the raised error doesn't get handled the same way as it would if we were using pundit_role: :create at the mutation, argument, or type level.

Am wondering if you have a recommendation for how to approach this?

thornomad avatar Dec 01 '19 17:12 thornomad

Thanks for such a great question. It's a tricky scenario because, from GraphQL's perspective, the authorization is happening "in the middle of application logic."

~~My first recommendation would be to see if we can run the authorization check at one of GraphQL's normal authorization boundaries. For example, would it be possible to massage the pundit class so that you could use~~

  pundit_role :create?

~~in the mutation definition? It seems like you need to check that person.id == mutation.input[:person_id] (unless that's just example code 😅 ) .~~ Well, now that I look, you wouldn't have access to the arguments there.

It seems like there might be another possibility with

-   argument :input, NoteInput, required: true
+   argument :input, NoteInput, required: true, pundit_role: :create

but I'm not exactly sure how that actually behaves. I can definitely take a closer look tomorrow.

There's a somewhat-related PR just merged into master to support turning input objects into application object (ie, NoteInput -> Note as described in https://github.com/rmosolgo/graphql-ruby/blob/master/guides/type_definitions/input_objects.md#converting-to-other-ruby-objects).

raised error doesn't get handled the same way

Finally, as a work-around, you could fake proper error handling with

def resolve(input:)
  note = Note.new(input.to_kwargs)
  Pundit.authorize(current_user, note, :create?)
  note.save!
  note
rescue Pundit::NotAuthorizedError 
  # This method comes from the PunditIntegration, so we can call it manually here:
  unauthorized_by_pundit(self.class, self)
end

Not a great solution, but if you need something to move forward with in the meantime, I thought I'd share!

rmosolgo avatar Dec 02 '19 22:12 rmosolgo

Thanks for looking into this and the tip about unauthorized_by_pundit!

Curious what you find about adding pundit_role: create to the argument call. While doing what you showed (above) in the resolve method does what we would expect it to do ... it's a lot of repeated code for every create mutation.

I actually bumped into that documentation about converting an argument to other ruby objects but when I tried the master branch and added a prepare method I didn't see it being called or returning anything differently ... not sure if I am just setting it up wrong or what is going on. If you are having success with that method I will re-load master and give it another try again.

thornomad avatar Dec 03 '19 14:12 thornomad

I didn't see it being called

Yeah, the problem was, we brought that behavior in from a veeeery old PR, so it worked with old execution code but not the new code! It's fixed for new code on master.

rmosolgo avatar Dec 03 '19 15:12 rmosolgo

Trying to use the InputObject solution doesn't appear to work for me. It tries to find the policy for the mutation and not the instance that is returned in the prepare method.

thijsnado avatar Sep 25 '20 17:09 thijsnado