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

Figure out mutation/resolver input or arguments authorization #24

Open
palkan opened this issue May 11, 2020 · 5 comments
Open

Figure out mutation/resolver input or arguments authorization #24

palkan opened this issue May 11, 2020 · 5 comments
Labels
discussion Let's talk

Comments

@palkan
Copy link
Owner

palkan commented May 11, 2020

Related to palkan/action_policy#89 and #22.

Let's think about how we can use policies to authorize mutations inputs (and, probably, resolvers in general). What are the possible use-cases? I can recall a few:

  • Making some input fields or arguments non-public (maybe, we can re-use authorize_field: here?), for example, available only to admins/managers/etc.
  • Checking the arguments/input fields values for access rights, e.g., when a user provide an associated object ID as a part of the payload.

The goal of this ticket is to discuss how a general approach could look like, collect examples (and convert them into test cases), design the API.

@sponomarev
Copy link
Contributor

Let me leave some thoughts over that. I'm implementing a solution exposing the same concepts as action_policy-graphql but for the original Apollo Server written on NodeJS with GraphQL directives.

Let's take a look at the schema and consider several use-casses.

  input MessageInput {
    content: String!
    author_id: ID!
  }

  type Message {
    id: ID!
    content: String
    author: String
  }

  type Query {
    getMessage(id: ID!): Message
  }

  type Mutation {
    createMessage(input: MessageInput): Message
    updateMessage(id: ID!, input: MessageInput): Message
  }
  1. The most simple case: argument authorization. E.g. for getMessage(id: ID!): Message. If we want to check if the id value is permitted for that query before calling the resolver, it would be nice to have access to it in the policy context directly or, maybe, through args object to prevent name collision. authorize_field and preauthorize are the first candidates to have this feature.
class QueryType < Types::BaseObject
  field :get_message, Types::Message, null: true, :authorize_field do
    argument :id, ID, required: true
  end
end

class QueryPolicy < Federation::ApplicationPolicy
  def get_message?
    permitted_ids.include?(args.id)
  end

  private

  def permitted_ids
   # ...
  end
end

I've already implemented that approach in for Apollo Server where policies have the same arguments as resolvers and can interact with args directly.

We are still able to handle complex input objects as a whole, and it's a valid way to go for complex checks where it's necessary to consider multiple arguments, but it could be messy to handle multiple input type fields independently in one place. There is potentially another way to make those auth checks more granular.

2.1. Granular check for regular arguments.

class QueryType < Types::BaseObject
  field :get_message, Types::Message, null: true, :authorize_field  do
    argument :id, ID, required: true, authorize: true # Magic here
  end
end

class QueryPolicy < Federation::ApplicationPolicy
  def get_message?
    current_user.active?
  end
end

class GetMessageArgumentsPolicy < Federation::ApplicationPolicy
  def id?
    permitted_ids.include?(value)
  end
end

Here, we are able to decompose general access rule to the query (e.g. only active users can do that) with the argument policy where we can check the value of the argument.

2.2 This could be extended to input objects. E.g. for createMessage(input: MessageInput): Message we should ensure that author_id is equal to the current user id.

class Types::MessageInput < Types::BaseInputObject
  description "Attributes for creating or updating a blog post"
  argument :content, String, required: true
  argument :author_id, ID, required: true,  authorize: true # Magic here
end

class MessageInputArgumentsPolicy < Federation::ApplicationPolicy
  def author_id?
   value == current_user.id
  end
end

Both of them are composable together, e.g. for updateMessage(id: ID!, input: MessageInput): Message

class QueryType < Types::BaseObject
  field :updateMessage, Types::Message, null: true, :authorize_field  do
    argument :id, ID, required: true, authorize: true # Magic here
    argument :id, Types::MessageInput, required: true
  end
end

class Types::MessageInput < Types::BaseInputObject
  description "Attributes for creating or updating a blog post"
  argument :content, String, required: true
  argument :author_id, ID, required: true,  authorize: true # Magic here
end

@palkan
Copy link
Owner Author

palkan commented May 12, 2020

@sponomarev Thanks! I like the granularity idea. The only downside that it could lead to policies bloat.

On the other hand, the getMessage example seems like a worse alternative to the current approach with authorize: true and using show? policy for the record itself. That's, IMO, better reflects the underlying authorization logic: we check whether a user allowed to read this message, not whether user is allowed to use this ID.

Are there other cases, which couldn't be reduced to object-action form instead of policy-param?

As for input object, wouldn't it be enough to provide something similar to scoping (like strong parameters)?

My gut feeling is that writing policies for internal, code-specific, entities (such as GraphQL fields) is not right, we should operate on domain objects. That is, "am I allowed to see this message?" and "am I allowed to update these attributes for the message?" instead of "am I allowed to use this ID to get a message?" and "am I allowed to use this ID as an authorId field for this input?" respectively.

@palkan
Copy link
Owner Author

palkan commented May 12, 2020

A few words on passing arguments to policy checks.

Although it adds a lot of flexibility, it may lead to a separation of concerns violation (or boundaries crossing, name yourself): you can make your policy know about the context it is used within.

Action Policy polices are meant to be context-independent, general-purpose ACLs (unlike in Apollo library, which is GraphQL-specific, and that's why passing the arguments totally makes sense there).

@neiljohari
Copy link

I'll admit that I haven't thought too deeply into this, but an idea that comes to mind is perhaps a completely separate project similar to strong_params tailored to graphql.

Then we can simply use action_policy as-is to conditionally choose between which fields should be accepted/rejected.

Here's an example of something I've previously done in a Rails project using REST and strong_params:

  def user_params
    if current_user.try(:elevated?) 
      params.require(:user).permit(:first_name, :last_name, :graduation_year, :pledge_year, :pledge_semester, :member_type, :admin, :avatar, :bio, :phone_number)
    elsif current_user.try(:brother?) or current_user.try(:pledge?) 
      params.require(:user).permit(:graduation_year, :pledge_year, :pledge_semester, :avatar, :bio, :phone_number)
    end
  end

Maybe a similar approach can be used to authorize the input fields passed to a resolver, letting us explicitly require/disallow fields but do this dependent on user permissions as defined by action_policy.

@dmitry
Copy link

dmitry commented Jul 8, 2020

I have found the following approach works good in our project: reuse permit method of the strong params directly inside the params_filter scope.

Policy params_filter on steroids with help of attributes and association methods to make the readability better (to reduce over-bloating the policies):

  params_filter do
    attributes(
      :password,
      :password_confirmation,
      :locale
    )

    if user.new_record?
      attributes(
        :email,
        :profile_type
      )
    end

    association(:profile)
    association(:person)
    association(:phone_number)
  end

association calls related policy with own params_filter of the association and in the end results in nested allowed parameters.
It uses a bit of reflection methods and ActionPolicy.lookup.

GraphQL resolver looks the following way:

      def resolve(user:)
        authorize!(current_user, to: :manage?)

        user.attributes = authorized(
          user,
          with: UserPolicy,
          scope_options: {record: user}
        )

With help of additional scope_matcher for the action_controller_params:

  scope_matcher :action_controller_params, Types::Base::InputObject

If you would like to work with uploads through the ActionController::Parameters it requires a tiny addition to PERMITTED_SCALAR_TYPES:

ActionController::Parameters::PERMITTED_SCALAR_TYPES << ApolloUploadServer::Wrappers::UploadedFile

More detailed version is here:
https://gist.github.com/dmitry/3e423a473ef30fec91fdc6c6bbe0bd45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Let's talk
Projects
None yet
Development

No branches or pull requests

4 participants