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

Passing mutation arguments to policy #89

Closed
alexander37137 opened this issue Oct 7, 2019 · 11 comments
Closed

Passing mutation arguments to policy #89

alexander37137 opened this issue Oct 7, 2019 · 11 comments
Labels
enhancement New feature or request

Comments

@alexander37137
Copy link

Is there a easier way to pass mutation arguments to policy

Right now im wrapping record and arguments to hash

def resolve(id:, attributes:)
    record = Record.find(id)

    authorize!({ record: record, attributes: attributes }, with: RecordPolicy, to: :edit?)

I tried to add attributes to authorization context, but queries also start require attributes even if i made it nullable

authorize :attributes, allow_nil: true

@palkan palkan transferred this issue from palkan/action_policy Oct 7, 2019
@palkan
Copy link
Owner

palkan commented Oct 7, 2019

queries also start require attributes even if i made it nullable

Yep, that's a correct behaviour.

User input (attributes) should not affect authorization and could not be a part of authorization context.

What's is your use-case? What do you have in your edit? rule?

@alexander37137
Copy link
Author

Some fields in my mutation can be edited only by users with certain roles. In policy i check if those fields changed and users have correct roles

@palkan
Copy link
Owner

palkan commented Oct 8, 2019

Some fields in my mutation can be edited only by users with certain roles

That sounds very similar to strong parameters. So, I suggest using filtering instead. Something like:

# mutation
def resolve(id:, attributes:)
  record = Record.find(id)
  # check that user has a right to perform this action
  authorize! record, to: :do_something?

  # make sure that user pass correct params
  filtered_attributes = authorized_scope(attributes, type: :gql_input, with: RecordPolicy)

  # do something with filtered_attributes
end

# policy
class RecordPolicy < ApplicationPolicy
  scope_for :gql_input do |input|
    # remove fields depending on the role/permissions
  end
end

Another idea is to make it possible to authorize: true input field:

class Attributes < GraphQL::Schema::InputObject 
  field :a, String, authorize: true
end

@alexander37137
Copy link
Author

If I filter mutation attributes, it will be unclear for api user why passed attributes not saving. I want to provide detailed reason of failure with "Failure Reasons" feature.

Authorising input fields is good idea but in some cases i anyway need to check attribute value if only some of values available for user

@palkan
Copy link
Owner

palkan commented Oct 9, 2019

Got it.

Then it seems that we need to think about adding support for rules arguments:

def rule?(args:)
  # do smth
end

Not sure about the authorize! API:

# allowing to pass arrays in `to:` ?
authorize! record, to: [:rule?, args: arguments] 

# additional keyword argument ?
authorize! record, to: :rule?, with_args: {args: arguments}

@alexander37137
Copy link
Author

alexander37137 commented Oct 10, 2019

Another idea is add another flag to authorize: allow_undefined or something like this, which allow to skip context argument
authorize :attributes, allow_nil: true, allow_undefined: true

But i like your idea with rule arguments more.

@palkan palkan transferred this issue from palkan/action_policy-graphql Oct 21, 2019
@palkan palkan added the enhancement New feature or request label Oct 21, 2019
@somenugget
Copy link
Contributor

Another idea is add another flag to authorize: allow_undefined or something like this, which allow to skip context argument
authorize :attributes, allow_nil: true, allow_undefined: true

But i like your idea with rule arguments more.

Sounds like a good-first-issue 🙂

@somenugget
Copy link
Contributor

Btw, I'm using this workaround

# frozen_string_literal: true

class ApplicationPolicy < ActionPolicy::Base
  def initialize(*args, **params)
    # allow to skip explicit passing {promotable: nil} if `authorize :promotable, allow_nil: true`
    allowed_nil_params = self.class.authorization_targets.each_with_object({}) do |(target, options), nil_params|
      nil_params[target] = nil if options[:allow_nil]
    end

    super(*args, **allowed_nil_params.merge(params))
  end
end

@palkan
Copy link
Owner

palkan commented Nov 22, 2019

Sounds like a good-first-issue

Yeah)

We can add the following API in addition to the existing allow_nil: true:

authorize :smith, optional: true

@somenugget Would you like to propose a PR?)

@somenugget
Copy link
Contributor

@somenugget Would you like to propose a PR?)

@palkan yep) will do it soon

@palkan
Copy link
Owner

palkan commented May 11, 2020

Closed in favour of palkan/action_policy-graphql#24

@palkan palkan closed this as completed May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants