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

Namespaced policies #12

Closed
assembler opened this issue Jan 19, 2013 · 44 comments
Closed

Namespaced policies #12

assembler opened this issue Jan 19, 2013 · 44 comments

Comments

@assembler
Copy link
Contributor

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

@jnicklas
Copy link
Collaborator

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

@assembler
Copy link
Contributor Author

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.

@uberllama
Copy link

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

@thomasklemm
Copy link
Collaborator

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

@uberllama
Copy link

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

@thomasklemm
Copy link
Collaborator

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?

@uberllama
Copy link

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

@uberllama
Copy link

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

@thomasklemm
Copy link
Collaborator

# 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.

@uberllama
Copy link

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.

@thomasklemm
Copy link
Collaborator

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.

@assembler
Copy link
Contributor Author

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.

@thomasklemm
Copy link
Collaborator

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

@assembler
Copy link
Contributor Author

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
Copy link

tims commented Apr 26, 2014

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

@adamcrown
Copy link

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.

@damien-roche
Copy link

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.

@uberllama
Copy link

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

@adamcrown
Copy link

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

@uberllama
Copy link

Very handy. Cheers.

@jnicklas
Copy link
Collaborator

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?

@jnicklas jnicklas reopened this May 21, 2014
@adamcrown
Copy link

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?

@ecbypi
Copy link
Contributor

ecbypi commented May 22, 2014

@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

@adamcrown
Copy link

@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.

@jnicklas
Copy link
Collaborator

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.

@jnicklas
Copy link
Collaborator

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

@adamcrown
Copy link

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.

@ecbypi
Copy link
Contributor

ecbypi commented May 22, 2014

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.

@thomasklemm
Copy link
Collaborator

Implemented in #152.

@jizak
Copy link

jizak commented Jul 16, 2014

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

@ecbypi
Copy link
Contributor

ecbypi commented Jul 16, 2014

@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?

@jizak
Copy link

jizak commented Jul 16, 2014

@ecbypi, it doesn't work with require_dependency

@coryodaniel
Copy link

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

@andreimoment
Copy link

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).

@coryodaniel
Copy link

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.

@andreimoment
Copy link

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).

@martinverdejo
Copy link

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?

@TangMonk
Copy link

TangMonk commented May 4, 2016

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

@thomasklemm
Copy link
Collaborator

thomasklemm commented May 4, 2016

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

@TangMonk
Copy link

TangMonk commented May 4, 2016

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

@wayne5540
Copy link

wayne5540 commented Nov 20, 2017

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

wayne5540 added a commit to wayne5540/pundit that referenced this issue Nov 20, 2017
wayne5540 added a commit to wayne5540/pundit that referenced this issue Nov 20, 2017
wayne5540 added a commit to wayne5540/pundit that referenced this issue Nov 20, 2017
wayne5540 added a commit to wayne5540/pundit that referenced this issue Nov 20, 2017
@cedv
Copy link

cedv commented Dec 27, 2017

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.

@reentim
Copy link

reentim commented Apr 5, 2018

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

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

@coderliu
Copy link

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

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

No branches or pull requests