varvet/pundit

Namespaced policies

Closed this issue ยท 44 comments

The app I'm working on has two sections: user and admin. There are multiple types of users and multiple types of admins, and different authentication solutions are used for each of them.

Although all the application models are used by both admins and users, policies are very different. Handling both users and admins and all distinct types within single policy was not so clear and full of ifs.

The way I handled this was to dynamically define policy_class on every ActiveRecord descendant in before filter. Something like this:

def set_policy_namespace
  m = self.class.name.split("::").first
  active_record_descendants.each do |ar|
    ar.class.instance_eval do
      define_method(:policy_class) do
        "::Policies::#{m}::#{self.name}Policy"
      end
    end
  end
end

def active_record_descendants
  Rails.application.eager_load! unless Rails.application.config.cache_classes
  ActiveRecord::Base.descendants
end

So, if I'm in Admin::HomeController, Project model will have policy_class redefined to return ::Policies::Admin::ProjectPolicy.

I've tried to demonstrate valid use case for this. Do you have any thoughts on how can this be made easier? I'm not sure whether or not it should be a part of gem (since it is somewhat rare).. I'm just asking for opinion.

thanks

I don't really understand what you're trying to do.

I realised that it was design issue with my application. I had multiple "sub applications" inside one large rails application, and each one required completely different policies. The solution was to break the big app into smaller ones.

I think this is still a valid issue. I'm about to create a system that has controllers for the same resource namespaced under two user classes (User and Admin). What's the recommended practice for doing so other than filling up policy files with tons of conditionals.

Example:

admins/widgets
users/widgets

Could you be trying to do what @assembler described as fitting multiple sub apps into one large app?

They're not distinct apps, just namespaced areas of the web app for different users types.

I see, like a web interface and an admin backend in one app. They might share models but have a completely different set of controllers / workflows and might thus need completely different policies for one model, right? Any suggestions how Pundit can help you do what you want to do?

Exactly. Off the top of my mind, I'd assume namespacing the policies themselves, such as

/policies/widget_policy.rb -> WidgetPolicy
/policies/admins/widget_policy.rb -> Admins::WidgetPolicy

I guess the alternative would be to check the class of current_user and include different policies subclasses manually?

# Currently this works
authorize @widget, :update?
# which is equivalent to
raise Pundit::NotAuthorizedError unless WidgetPolicy.new(current_user, @widget).update?
# However, if you need a custom namespaced policy you can use
raise Pundit::NotAuthorizedError unless Admins::WidgetPolicy.new(current_user, @widget).update?

Would that be enough to get started? When you're further into your app, we'll see if a custom solution is still needed and how it can look like.

Are you suggesting perhaps creating an authorize_admin method on my end that would infer the class and make the second call? That could work. I'll update this thread as I get deeper. Cheers.

How about an authorize @widget, :update?, policy: Admin::WidgetPolicy option in an options hash on the Pundit side? Or a policy_namespace: 'Admin' option for writing a general helper.

# app/helpers/authorization_helper.rb
module AuthorizationHelper
  def authorize_admin(*args)
     record, query = args[0], args[1]
     options = args[2].merge({policy_namespace: 'Admin'}) # Will look for an Admin::WidgetPolicy when @widget is passed in
     authorize record, query, options
  end
end

That being said, it's all hypothetical currently with no app to use it, so it won't go into Pundit yet.

I think that this could work out of the box.

If you are in Admin module:

  module Admin
    class WidgetsController
      # referencing WidgetPolicy
    end
  end

Referencing WidgetPolicy from that controller would see if Admin::WidgetPolicy exists, and if it is not found, it would try to find ::WidgetPolicy. This is default behavior of ruby, and pundit should confront to it.

We're currently not inferring policy names from the controller, but from the record passed in. (Source).

I know. My point was that any policy you reference from within controller within specific module, should naturally search inside that module for policy definition. My example would be the same for e.g. UserPolicy within WidgetsController.

tims commented

I would also like to have separate policies for different needs (eg, admins), it would be nice for keeping the policies focussed and descriptive, and for returning different error messages.

@thomasklemm More than passing in a namespace, I like your idea of being able to do

authorize @widget, :update?, policy: Admin::WidgetPolicy

Controller namespaces for the same model is a common use case for us as well. I think the simple policy: option makes a lot of sense.

I'd love to see how people are wiring together their authorization layer without namespaces on non-trivial apps. For example, many people use active_admin which is commonly integrated into the same Rails app. So how are you supposed to build policies for UsersController vs Admin::UsersController? Currently they both refer to the same UserPolicy.

Here's why I consider it an insecure default:

/users => UserPolicy.index?
/admin/users => UserPolicy.index?

With that in mind, I'd consider automatic namespacing essential.

We never use the index/resolve component of Pundit. It's not useful in apps with any kind of user role system.

Here's the controller concern I created for our admin namespace for anybody trying to do something similar. It works as a temporary workaround but I'd rather have a solution built into pundit.

module AdminPolicies
  extend ActiveSupport::Concern

  included do
    helper_method :admin_authorize
    helper_method :admin_policy_scope
  end

  # These are workarounds for the lack of support for namespacing in pundit
  # https://github.com/elabs/pundit/issues/12

  def admin_authorize(record, query=nil)
    klass = "Admin::#{record.model_name}Policy".constantize
    policy = klass.new(current_user, record)
    query ||= "#{params[:action]}?"

    @_policy_authorized = true

    unless policy.public_send(query)
      error = Pundit::NotAuthorizedError.new("not allowed to #{query} this #{record}")
      error.query, error.record, error.policy = query, record, policy

      raise error
    end

    true
  end

  def admin_policy_scope(scope)
    klass = "Admin::#{scope.model_name}Policy::Scope".constantize
    policy = klass.new(current_user, scope)

    @_policy_scoped = true

    policy.resolve
  end
end

Very handy. Cheers.

I'm not opposed to inferring the namespace from the controller. If we do a const lookup on a particular constant, it will look in it first, and then in the global namespace, which sounds like exactly the behaviour we want. And it does seem sane to do something like Admin.const_get(...). Can anyone provide a reasonably clean PR for this?

I'm taking a stab at this. Hopefully I'll have a PR tomorrow.

I'm running into a frustrating problem though. const_defined? returns false for the policies because they have not yet been autoloaded. Does anybody know any way around that other than a rescue block?

@adamcrown Do you have code anywhere to look at? I was looking into using Module.parent provided from the controller but was running into problems since all lookups are done via the Pundit module. It's an initial pass and I was trying to avoid significant refactors. I was thinking it would be reasonable to have the Pundit.policy* defined on the controller to make this simpler. @jnicklas @thomasklemm, any thoughts?

ecbypi/pundit@71db8c35d18aa0ceaaf8403388257cd384ef105f

@ecbypi, here's what I have so far: adamcrown@d3092ec

I had to pass the controller around as an argument a bit because the finder is always called from the Pundit module, as you said. I'm hoping I can improve on that but I wanted to get something working initially before attempting some bigger refactor.

No tests yet, and one test still red. But it does seem to be working in the app I'm using it with. Feel free to take whatever you want from my code and take a closer look at yours when I have some more time.

I like the code that @ecbypi linked. I'm against moving Pundit.policy though. It's nice that the code as is does not break any APIs, and I'm cool with having the namespace parameter, so I think moving it would just break people's code unnecessarily.

@adamcrown's suggestion was maybe a bit convoluted? Are all these changes really necessary? The resulting code is pretty hard to read.

My code is definitely a work in progress. It works. But I'm not happy with it yet either. If @ecbypi can get a PR up soon, I'm happy to have his code merged.

I'm adding a test to make sure that it still finds policies in the global namespace if there isn't one in the controller's namespace. After I get that working I'll submit a PR.

Implemented in #152.

I'm still having issue with namespaces.
I have model Question.
app/models/question.rb

class Question < ActiveRecord::Base
end

Created two different policies for simple user and admin:
app/policies/question_policy.rb

class QuestionPolicy < ApplicationPolicy
end

app/policies/admin/question_policy.rb

class Admin::QuestionPolicy < Admin::ApplicationPolicy
end

app/controllers/admin/questions_controller.rb

class Admin::QuestionsController < Admin::ApplicationController
  def index
    authorize @questions.first
  end
end

In my admin controller app/policies/question_policy.rb is used, not app/policies/admin/question_policy.rb

@jizak, const_get looks through namespace's ancestry to find a constant. It might be that if Admin::QuestionPolicy hasn't been loaded, then when it gets to Object and looks for QuestionPolicy it loads QuestionPolicy through the Rails' constant autoloading.

Does it work if you add require_dependency 'admin/question_policy' in the controller?

@ecbypi, it doesn't work with require_dependency

I just released a gem the other day. It is compatible with Pundit's policies and allows you to namespace based on controller, model or both. Check it out if you still need this functionality -> Regulator

Thank you!

Hope you can add "Use" section with a few examples to the readme... The
tests don't speak clearly to me.
On Fri, Jul 24, 2015 at 12:47 PM Cory ODaniel notifications@github.com
wrote:

I just released a gem the other day. It is compatible with Pundit's
policies and allows you to namespace based on controller, model or both.
Check it out if you still need this functionality. Regulator
https://github.com/coryodaniel/regulator

โ€”
Reply to this email directly or view it on GitHub
#12 (comment).

Will do this Sunday. No problem!

On Jul 24, 2015, at 6:31 PM, Andrei Andreev notifications@github.com wrote:

Thank you!

Hope you can add "Use" section with a few examples to the readme... The
tests don't speak clearly to me.
On Fri, Jul 24, 2015 at 12:47 PM Cory ODaniel notifications@github.com
wrote:

I just released a gem the other day. It is compatible with Pundit's
policies and allows you to namespace based on controller, model or both.
Check it out if you still need this functionality. Regulator
https://github.com/coryodaniel/regulator

โ€”
Reply to this email directly or view it on GitHub
#12 (comment).

โ€”
Reply to this email directly or view it on GitHub.

And -- I appreciate your releasing. It. Model-based permissions feel
strange for me as most of my apps have Admin-namespaced controllers. I have
an app ready to use this !
On Fri, Jul 24, 2015 at 8:04 PM Cory ODaniel notifications@github.com
wrote:

Will do this Sunday. No problem!

On Jul 24, 2015, at 6:31 PM, Andrei Andreev notifications@github.com
wrote:

Thank you!

Hope you can add "Use" section with a few examples to the readme... The
tests don't speak clearly to me.
On Fri, Jul 24, 2015 at 12:47 PM Cory ODaniel notifications@github.com
wrote:

I just released a gem the other day. It is compatible with Pundit's
policies and allows you to namespace based on controller, model or
both.
Check it out if you still need this functionality. Regulator
https://github.com/coryodaniel/regulator

โ€”
Reply to this email directly or view it on GitHub
#12 (comment).

โ€”
Reply to this email directly or view it on GitHub.

โ€”
Reply to this email directly or view it on GitHub
#12 (comment).

how do I implement the policy: option from here?

I have this working:
raise Pundit::NotAuthorizedError unless Admin::HomePolicy.new(current_user, :home).index?

but not this:
authorize :home, :index?, policy: Admin::HomePolicy

is this still the best approach?

Still do not know how to use, where is the documentation?

@TangMonk Namespaced policies are not supported right now (see #178 and 753bb0a for more info on the reason for that).

@thomasklemm, actually I figure it from older issues, please look at #382 #216

TL;DR: override policy(record) method

It took me a long time to figure out how to add namespace and do a controller based authorization, after tracing down the source code, it turns out very easy, I just need to override policy(record) from Pundit.

My code:

I'll just post my solution as reference here, hope I can save someone's time.

In my base controller:

# app/controllers/api/v1/base_controller.rb
class Api::V1::BaseController < ApplicationController
  include Pundit
  after_action :verify_authorized

  private

  def policy(record)
    policies[record] ||= "#{controller_path.classify}Policy".constantize.new(pundit_user, record)
  end
end

In my controller

# app/controllers/api/v1/auth/offers_controller.rb
class Api::V1::Auth::OffersController < Api::V1::Auth::BaseController
  def index
    authorize Offer
    # ...
  end
end

My policy

# app/policies/api/v1/auth/offer_policy.rb
class Api::V1::Auth::OfferPolicy < Api::V1::BasePolicy
  def index?
    true
  end
end
cedv commented

I came up with a different approach to the problem. Since Pundit does model-based authorization, no point fighting it.

Given a Widget model and the need for a Admin namespace policy, I created a new model Admin::Widget which just inherit Widget:

# app/models/admin/widget.rb
class Admin::Widget < Widget
end

On the controller side, instead of using the Widget model for admin-related tasks, I use Admin::Widget:

@widget = Admin::Widget.first

Policy-wise, I can have both WidgetPolicy and Admin::WidgetPolicy without any problem, keeping both authorization rules separate and neat.

Yet another option, in cases where defining policy_class on the model is undesirable:

authorize OpenStruct.new(policy_class: ArbitraryPolicy, widget: @widget)

Thanks @adamcrown for his great work around.

Below is my modified base on his solution. Link to gist

module ScopedPolicies
  extend ActiveSupport::Concern

  included do
    helper_method :authorize
    helper_method :policy_scope
  end

  # These are workarounds for the lack of support for namespacing in pundit
  # https://github.com/elabs/pundit/issues/12
  def controller_namespace
    @controller_namespace ||= self.class.to_s.sub(/::[A-z]*Controller/, '')
  end

  def authorize(record, query = nil)
    klass = "#{controller_namespace}::#{record.model_name}Policy".constantize
    policy = klass.new(current_user, record)
    query ||= "#{params[:action]}?"

    @_policy_authorized = true

    unless policy.public_send(query)
      error = Pundit::NotAuthorizedError.new("not allowed to #{query} this #{record}")
      error.query, error.record, error.policy = query, record, policy
      raise error
    end

    true
  end

  def policy_scope(scope)
    klass = "#{controller_namespace}::#{scope.model_name}Policy::Scope".constantize
    policy = klass.new(current_user, scope)

    @_policy_scoped = true

    policy.resolve
  end
end