Pundit: Issue with new namespace finding capabilities (using an array)

[zach-taylor]

When calling various helper methods in Pundit e.g. policy([:admin, @user]).show?, the full array is being passed to the policy object, rather than just @user. Is this expected functionality? or should my policy objects be grabbing the last object in the array to use as the record in the policy.

[zach-taylor]

Better example:

# app/controllers/admin/users_controller.rb
module Admin
  class UsersController < ApplicationController
    def show
      @user = User.find(params[:id])
      authorize([:admin, @user])
    end
  end
end

...

# app/policies/admin/user_policy.rb
module Admin
  class UserPolicy
    attr_reader :user, :record
    def initialize(user, record)
      @user = user
      @record = record # => [:admin, @user]
    end
  end
end

So what I'm having to do in my policy is this:

@record = record.is_a?(Array) ? record[-1] : record

[jnicklas]

Hmm, that's not optimal. It seems somewhat unexpected to send through only the last item as well though. Damn.

[zach-taylor]

Yeah, not sure what the right solution is. @ramaboo any ideas?

[ramaboo]

@zach-taylor

At my company we solve this problem in the ApplicationPolicy like this (which all other policies inherit from):

class ApplicationPolicy
  attr_reader :user, :record

  def initialize(user, record)
    @user = user
    @record = record.is_a?(Array) ? record.last : record
  end
end

I think the bigger question is should this be done at the Pundit level where record is always a single object or should there simply be better documentation around the need to do this.

We might also consider adding the above to the default generated policy. I am working on better namespace support for scopes as well so I am open to any suggestions on how best to solve this issue and how that might effect scopes.

[Burgestrand]

This also affects the test helpers, since you'll now want to pass in the full array to reproduce the actual conditions:

expect(Namespace::Here::PolicyClass).to permit(user, [:namespace, :here, record])

[ramaboo]

What is the thinking on this. Should record aways just return the record or an array? I am working on some more pundit improvements so figured I could get this one in as well if people think it should.

[jnicklas]

So I've thought about this a bit more. I feel pretty strongly that we should pass only the _last_ object into the policy. The reason for this design choice is that there is already precedent for exactly this kind of behaviour in Rails' forms. Consider this:

<%= form_for [@post, @comment] do |f| %>
  <%= f.text_field :body %>
<% end %>

Here, we're passing an array of objects into the form_for method. Rails uses this array to infer the URL to post the form to (and a bunch of other stuff), but crucially the object used within the form is only the _last_ object passed in the array (in this case @comment).

It seems that doing the same thing in Pundit would follow Rails' own conventions, and thus work as users expect.

Since this is already in a released gem, it would be a breaking change, and we should bump the version to 2.0, but this is something we should do anyway.

[zach-taylor]

Excellent reasoning, @jnicklas. I think that makes a lot of sense.

[ramaboo]

Ok i can make this change. I also have some changes I want to push now that they have been tested in production concerning policy_scope where you can now do policy_scope([:admin, @user]).all and it will look for Admin::User::Scope.resolve. Ill try to get all that done today or tomorrow.

[doits]

Not sure if this is the right topic (might be a little bit OT), but maybe this helps in implementing picking a namespaced policy according to the controller namespace.

I'm currently using the following monkey patches / module prepends to make namespacing work seamlessly according to the controller namespace:

Pundit.prepend(
  Module.new do
    def authorize(record, *args)
      super(namespaced_record(record), *args)
    end

    def policy(record, *args)
      super(namespaced_record(record), *args)
    end

    def permitted_attributes(record, *args)
      super(namespaced_record(record), *args)
    end

    def policy_scope(scope, *args)
      super(namespaced_record(scope), *args)
    end

    private

    def namespaced_record(record)
      return record if record.is_a?(Array) # array explicitly passed, respect it

      parent_class = self.class.parent.to_s.underscore.to_sym # get parent class
      return record if parent_class == :object # parent is object, so not nested

      namespace_array = [parent_class, record]

      # If we find a policy for that namespace, return the namespaced record,
      # otherwise simply return the record.
      Pundit::PolicyFinder.new(namespace_array).policy ? namespace_array : record
    end
  end
)

Pundit::PolicyFinder.prepend(
  Module.new do
    def param_key
      to_check = object.is_a?(Array) ? object.last : object

      if to_check.respond_to?(:model_name)
        to_check.model_name.param_key.to_s
      elsif to_check.is_a?(Class)
        to_check.to_s.demodulize.underscore
      else
        to_check.class.to_s.demodulize.underscore
      end
    end
  end
)

This means that having a controller MyNamespace::UsersController, using authorize(User.new) it first tries to get the policy MyNamespace::UserPolicy and falls back to UserPolicy if it is not found.

It currently only works for one level of nesting (but could be easily extended if needed). Also, it might be desirable to make this optional according to a config setting.

I can open a PR where we can refine this (implement specs, refactor etc.) if it is a desirable addition.

[jnicklas]

@doits We've tried this before and found that there were a lot of weird edge cases with this. Ruby's module namespace feature is pretty damned weird. In the end we reverted the feature, because there were just too many problems with the implementation.

The recently merged namespacing feature is a little more wordy, but it's far more understandable and stable.

[doits]

@jnicklas OK, I'll go on with my approach to see what happens when using this for some time (in production, too). If you want make the next try to implement this seamlessly, just ping me and I can open a PR where we can talk about edge cases (implementing specs etc.)

[crispinheneise]

I also ran into this issue and @ramaboo's solution worked for me. @jnicklas does it really need to be inferred automatically? I actually kind of like this workaround, because it's explicit.

For example I have two different policies for the same model. One is non-namespaced, and the other is inside the module Api::V1. Here's how the policy looks:

# app/policies/api/v1/api_policy.rb
module Api
  class V1::ApiPolicy
    attr_reader :user, :record

    def initialize(user, record)
      @user = user
      @record = record.is_a?(Array) ? record.last : record
    end
    # ...
  end
end

# app/policies/api/v1/plan_policy.rb
module Api
  class V1::PlanPolicy < Api::V1::ApiPolicy
  end
end

# app/controllers/api/v1/plans_controller.rb
authorize [:api, :v1, @plan]

This works really well for me because it's very clear what's being passed around. Adding @record = record.is_a?(Array) ? record.last : record is another level of clarity.

Maybe the need for a breaking 2.0 change could be eliminated by explaining this workaround in the documentation?

[skorth]

Same here, i also like the explicit way like mentioned before. 👍

@record = record.is_a?(Array) ? record.last : record

[ce07c3]

I also prefer the explicit way.

[axelson]

Has there been any movement on this? This also affects the permitted_attributes helper and prevents it from working with namespaced records. You get param is missing or the value is empty: array (when called as permitted_attributes([:namespace, @model_instance]).

[md5]

I just ran into the same issue as @axelson. It seems like the private PolicyFinder#find method is correctly accounting for the array case, but PolicyFinder#param_key is not. This leads to :array being used as the param key instead of (say) :my_model.

[WaKeMaTTa]

For me this is not an issue. I use this feature to send 2 items.

For example:

# app/controllers/childrens_controller.rb
def ChildresController < ApplicationController
  def index
    authorize [@parent, @childrens], :index?
  end
end

And i have this policy:

# app/policies/parent/children_policy.rb
class Parent::ChildrenPolicy < ApplicationPolicy
  attr_reader :parent

  def initialize(user, record)
    @user = user
    if record.is_a? Array
      @parent = record.first
      @record = record.last
    else
      @record = record
    end
  end

  def index?
    parent && Pundit.policy(user, parent).show?
  end
end

[medir]

I have same issue as @axelson described when I trying to use permitted_attributes with namespaced records. I think it would we nice and intuitive if we could do something like this:
permitted_attributes([:backoffice, @event]))

ATM I deal with it using the "cumbersome" way:

class Backoffice::EventsController < ApplicationController

....

private
def event_params
  params.require(:event).permit(policy([:backoffice, @event]).permitted_attributes)
end

end

But I think this is not a polite solution becouse we are losing the feature to have diferent permitted_attributes per action and if we want to use it we have to decuple the logic and move it into the controller doing something similiar to this:

def event_params
 if params[:action] == :edit
   params.require(:event).permit(policy([:backoffice, @event]).permitted_attributes_for_edit)
 elseif params[:action] == :create
   params.require(:event).permit(policy([:backoffice, @event]).permitted_attributes_for_create)
 else
   params.require(:event).permit(policy([:backoffice, @event]).permitted_attributes)
 end    
end

I want to take the opportunity to thank all the work of the author and also of the collaborators.

[medir]

I found another provisional solution that I think it is quite better from my last post. To fix the problem of permitted_attributes helper. I apply a patch to the method PolicyFinder#param_key using this initializer:

#conifg/initializers/pundit_patch.rb

module Pundit
  class PolicyFinder

     def param_key
       object = self.object.is_a?(Array) ? self.object.last : self.object
       if object.respond_to?(:model_name)
         object.model_name.param_key.to_s
        elsif object.is_a?(Class)
          object.to_s.demodulize.underscore
        else
          object.class.to_s.demodulize.underscore
        end
    end

  end
end

[doits]

@medir yeah, that's what I have been doing in https://github.com/elabs/pundit/issues/351#issuecomment-196874426 ;-)

[medir]

@doits it's true I overlooked it :)

[v-kumar]

@doits I am interested in using your solution. It has been sometime since your monkey patch is in use. Any comments on how it is working out for you?