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

Authorizing controller actions with no associated model #77

Closed
pbougie opened this issue Nov 17, 2013 · 31 comments
Closed

Authorizing controller actions with no associated model #77

pbougie opened this issue Nov 17, 2013 · 31 comments

Comments

@pbougie
Copy link

pbougie commented Nov 17, 2013

Pundit looks great for authorizing models but in some instances I need to authorize controller actions (with no associated model) as well. A simple check whether the user is logged in or not will suffice most of the time. However, sometimes the ability to view a controller action is based on a user role, i.e. user can view settings page if administrator but not if regular user.

What are the best practices for these situations? Any clarifications would be appreciated on how all the authorization pieces fit together.

@thomasklemm
Copy link
Collaborator

Great question, Patrick. @jnicklas, what would you do?

@tbuehlmann
Copy link

Why not depend on the Controller name? Like, having a HomePolicy class which just takes a single argument, the User, then manually retrieve the policy and check against it. If that's too "manually" and you want to use the #policy helper, why not inject nil as second argument?

@pbougie
Copy link
Author

pbougie commented Nov 19, 2013

@thomasklemm @tbuehlmann I've played around with that idea (using a custom helper method) but then you have a mixture of policies for controllers and models and things get confusing quickly. This also goes against the single responsibility principle. I'm leaning towards implementing action authorization using separate custom code in my app and Pundit for all the models. Again, any help would be appreciated.

@bradwheel
Copy link

I just ran into this problem. I have a HomeController that has 3 actions and no associated model. The second argument passed to authorize is necessary to find the policy class it needs to make the decision on whether or not the action is authorized, so I just passed it self and changed the filename and class name to reflect the controllers name.

Here is my policy class: (home_controller_policy.rb)

class HomeControllerPolicy 
  attr_reader :user, :ctrlr

  def initialize(user, ctrlr)
    @user = user
    @ctrlr = ctrlr
  end

  def profile?
    user.has_role?("Applicant")
  end

end

And in the controller I pass in self as the second argument, like this:

class HomeController < ApplicationController

before_action :authenticate_user!

    def profile
      authorize self
    end

 end

It works well for me. I don't mind having only one policy being associated with just a controller. It's clean, clear, and functions.

@jnicklas
Copy link
Collaborator

I guess we could support passing in symbols to policy, policy_scope and authorize. Something like:

authorize(:home, :show?)

And then we could have a policy:

class HomePolicy
  def initialize(user)
    @user = user
  end

  def show?
  end
end

But we need to be careful with the argument parsing, since we already have locked ourselves a bit in, and we also have many suggestions for extras like custom error messages and additional arguments.

@pbougie
Copy link
Author

pbougie commented Jan 27, 2014

I think something like @jnicklas is suggesting would be worthwhile. This would allow apps to keep authorization logic in one place. I've yet to have an app where all authorizations are based on a model. This is especially apparent when authorization logic is more complex because of multiple roles.

What about authorizations like this in a controller:

class PostsController < ApplicationController
  def special
    authorize :posts, :special?
    or
    authorize :posts # infers action name?
    or
    authorize # infers class and action names?
  end

  def create
    authorize @post, :create?
    or
    authorize @post # infers action name
  end
end

@nixpulvis
Copy link

Might be more overhead than makes sense, but since there are two pretty distinct cases for policies why not give each it's own base class.

UserPolicy < Pundit::ModelPolicy
# Looks for model named User.
end

PagePolicy < Pundit::ControllerPolicy
# Looks for a controller named PagesController.
end

Just a thought.

@jnicklas
Copy link
Collaborator

Pundit does not have base classes.

@nixpulvis
Copy link

Maybe a configuration to overload the find functionality?

@thomasklemm
Copy link
Collaborator

We could pass in a policy explicitly to authorize:

class HomeController < ApplicationController
  def show
    authorize(nil, :show?, policy: HomePolicy) # Will work too if there's a record passed in
  end
end

Related to #12.

@AntelopeSalad
Copy link

I hope you guys think of something soon because this functionality is extremely important to have and very common. For now @bradwheel's snippet seems to work though.

@nixpulvis
Copy link

authorize(nil, ...) just feels wrong.

@atstockland
Copy link

Another example is an errors controller. I want to authorize anyone access to the show action in that controller. But, the controller doesn't have a model. So, Im hacking it together.

@seppsepp
Copy link

I'd be pretty much happy about a solution. jnicklas' symbols solution sounds good to me.

@rldaggie
Copy link

This would also be a great fix for multi-tenant apps:

around_action :scope_current_company

I get a Pundit::NotAuthorizedError on yield

def current_company
  current_user.company
end
helper_method :current_company

def scope_current_company
  Company.current_id = current_company.try(:id)
  yield
ensure
  Company.current_id = nil
end

@tbuehlmann
Copy link

I highly recommend changing the scope_current_company method, it's not threadsafe. If you really have to memoize the actual company, use https://github.com/steveklabnik/request_store or a similar library.

@jorge-marques
Copy link

@thomasklemm +1!
An option to pass the class explicitly would make the gem more flexible, in my opinion.

@saulcosta
Copy link

Has this functionality been added? It would make a few things a lot easier!

@thomasklemm
Copy link
Collaborator

Headless policies have been implemented in #168. This feature has not yet been released in an official gem version though, so you'll have to reference the github source in you Gemfile for the time being,

# Gemfile
gem 'pundit', github: 'elabs/pundit'

@Senich
Copy link

Senich commented Mar 31, 2016

Is this feature is available in current gem version? Seems for me it doesn't work properly.
I've still got an error - "unable to find policy DashboardPolicy for :dashboard" while I do have dashboard_policy.rb in policies folder.

@thomasklemm
Copy link
Collaborator

Yes, it is. Maybe you'd need to restart your Rails server to pick up the changes or have a typo either in the file name or class name. Could you check that?

@Senich
Copy link

Senich commented Mar 31, 2016

I double checked that, there is no typo and of course I restarted server couple times. It's so simple so I have no idea where I screwed up.
dashboard_controller.rb

 class DashboardController < ApplicationController
    def index
      authorize :dashboard, :index?
      # skip_policy_scope
    end
  end

dashboard_policy.rb

class DashboardPolicy < Struct.new(:user, :dashboard)
  class Scope < Scope
    def resolve
      scope
    end
  end

  def index?
    true
  end

end 

my root path is 'dashboard#index' so when I start my application I get beautiful
Pundit::NotDefinedError in DashboardController#index
unable to find policy DashboardPolicy for :dashboard and again policy is in the policies/dashboard_policy.rb

@nbekirov
Copy link

Is it app/policies/dashboard_policy.rb or policies/dashboard_policy.rb?


Check this out:

  • If I make a typo in my policy class name like so

    class DashboardPolicyWrong < Struct.new(:user, :dashboard)

    The error is as follows:
    Unable to autoload constant DashboardPolicy, expected [APP_DIR]/app/policies/dashboard_policy.rb to define it

  • However if the file is missing (if I move/rename/delete it) the error is:
    unable to find policy DashboardPolicy for :dashboard

@Senich
Copy link

Senich commented Mar 31, 2016

sorry, policy is in the app/policies/ of course
Your example is correct providing pundit can initially see your policy but in my case pundit somehow can't locate that

@nbekirov
Copy link

Please try to leave only this as your policy:

class DashboardPolicy < Struct.new(:user, :dashboard)
  def index?
    true
  end
end

@Senich
Copy link

Senich commented Mar 31, 2016

Interesting, in this case I get
Pundit::PolicyScopingNotPerformedError in DashboardController#index
and I guess that means pundit at least sees the DashboardPolicy if it informs about missing scope...
As a temporal fix I can use skip_policy_scope in DashboardController#index as far as every user can see index page but it's not zen though. I'd like to get whats wrong here.

@thomasklemm
Copy link
Collaborator

class Scope < Scope can't work if you're not inheriting from an ApplicationPolicy with Scope defined. See the second line of you're dashboard_policy.rb from above. Maybe that's been the cause of the exception.

@Senich
Copy link

Senich commented Mar 31, 2016

And I can't inherit from ApplicationPolicy because I have no model, but scope is needed for work at the same time. I'm confused

@thomasklemm
Copy link
Collaborator

Just say class Scope, without inheriting it from anywhere.

@Senich
Copy link

Senich commented Mar 31, 2016

 class DashboardPolicy < Struct.new(:user, :dashboard)

   class Scope
     def resolve
       scope
     end
   end

  def index?
     true
  end
end   

Still got Pundit::PolicyScopingNotPerformedError in DashboardController#index

@Senich
Copy link

Senich commented Apr 14, 2016

I figured out what was wrong with my code.
I have after_action :verify_policy_scoped, only: :index in my application_controller and because of that Pundit tried to determine scope policy in first place and clearly failed. I just reroute index page to dashboard#show action, set ther authorize :dashboard, :show? and everything works. Thank you guys!

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