Pundit authorization adapter #2857

Merged
merged 3 commits into from Mar 17, 2014

Conversation

Projects
None yet
Contributor

romansklenar commented Jan 12, 2014

I've found that Pundit gem is nice alternative to CanCan and it would be nice to support it out of the box. Thanks to authorization adapter implementation shouldn't be so hard. Some resources about Pundit:

I'd like to contribute to AA and give back something to comunity but I'll need your feedback and code review. I've made basic tests implementation same as CanCan adapter tests but there are 2 things:

  • Any tips how can I improve specs and specially mocking of testing classes?
  • I'm unable now to get authorization_pundit.feature test passing since I'm not experienced enough with cucumber (info about failing feature tests).

@seanlinsley seanlinsley commented on an outdated diff Jan 12, 2014

features/authorization_pundit.feature
+ def show? ; true ; end
+ def new? ; create? ; end
+ def create? ; false ; end
+ def edit? ; update? ; end
+ def update? ; false ; end
+ def destroy?; false ; end
+
+ def scope
+ Pundit.policy_scope!(user, record.class)
+ end
+
+ class Scope < Struct.new(:user, :scope)
+ def resolve
+ scope
+ end
+ end
@seanlinsley

seanlinsley Jan 12, 2014

Owner

Simply:

>> class Scope < Struct.new(:user, :scope); end
 => nil 
>> class Scope < Struct.new(:user, :scope); end
TypeError: superclass mismatch for class Scope

The error is a symptom of a larger problem: you should move this code into a static file in the test environment. You can do that like this:

  • make the folder spec/support/templates/policies
  • move the code into its respective files in that folder
  • add this line to spec/support/rails_template.rb:
directory File.expand_path('../templates/policies', __FILE__), 'app/policies'

Then recreate your test environment by running rm -rf spec/rails and rerunning the tests :octocat:

Owner

seanlinsley commented Jan 19, 2014

@romansklenar I'd love to get this merged 🐱 Do you not have time to work on this anymore?

Contributor

romansklenar commented Jan 19, 2014

@seanlinsley I might get to it this week - there're still few failing feature tests and I'd like to write documentation too.

Owner

seanlinsley commented Jan 19, 2014

Okay, sounds good. I don't mean to bug you, I just want to keep the backlog of stale PRs from growing any further. 😓

Pundit is a great and lightweight tool for authorization. It'll be nice that AA will support this.

So sad that I've implemented it a month ago but not have time to open a PR. Thanks @romansklenar for implement and open PR for this. It looks really great and is good to merge :)

I'd like to post my implementation here (without tests/specs) just for your references : https://gist.github.com/tomchentw/8579571

Basically it's like what @romansklenar did but we have different opinion over subject is a Class. @romansklenar handle it by checking returned scope have any models, mine just rewrite that action to :index? (I don't fully understand AA so please correct me if I'm wrong).

Contributor

romansklenar commented Jan 24, 2014

@tomchentw You're right checking index? method is better and correctly working way how to handle it. Thanks for sharing your implementation.

@seanlinsley I've completed and refactored this PR, rebased it against master and locally rake test:major_supported_rails is passing so if Travis is going to be green it'll be ready for merge.

Coverage Status

Coverage decreased (-0.31%) when pulling 01666f6 on romansklenar:feature/WIP-add-pundit-adapter into b7e8c7d on gregbell:master.

This looks great. Hope it gets merged into master soon. Is there anything one could help with to make this happen?

Owner

seanlinsley commented Feb 13, 2014

Sorry everyone, this fell off of my radar. I'll review & merge this in the next day or two.

demisx commented Feb 13, 2014

Great choice. We've replaced outdated CanCan with Pundit as well and it's been awesome.

Looking forward to using this on a new project. Hope it gets merged into master soon!

Another person who is hoping this will be merged soon! Thanks for your efforts

onomojo commented Feb 27, 2014

What's the hold up on the merge?

getconor commented Mar 6, 2014

Is there any way we can help to get this merged?

@seanlinsley possible to get this merged?

Owner

seanlinsley commented Mar 11, 2014

Wow, if only people were this fired up about other tickets! Has anyone tried this out in their application? I'm not going to promise a ETA for when I'll have the time to review this, because I've already failed to uphold that promise once. At the very least: this is at the top of my list.

onomojo commented Mar 11, 2014

I've tried it. It works fine for what I'm using it for but my menus are pretty standard. I originally had a dropdown menu in the nav bar which linked out to some custom actions which I think it didn't work as expected for. I changed my navigation to work around it so I don't have a broken example at this point. I just wanted to mention it in case someone wanted to double check that it works in more than just the simple cases.

I have used this too. It works quite well for me. And mine is a fairly
complex implementation

On Tue, Mar 11, 2014 at 11:17 PM, Brian McQuay notifications@github.comwrote:

I've tried it. It works fine for what I'm using it for but my menus are
pretty standard. I originally had a dropdown menu in the nav bar which
linked out to some custom actions which I think it didn't work as expected
for. I changed my navigation to work around it so I don't have a broken
example at this point. I just wanted to mention it in case someone wanted
to double check that it works in more than just the simple cases.

Reply to this email directly or view it on GitHubhttps://github.com/gregbell/active_admin/pull/2857#issuecomment-37307242
.

I am using it as well. No problems yet.

I tried it. I couldn't get it to work. Always had authorization issues, although the rest of the app worked fine. But I guess the error was somewhere on my part.

Contributor

romansklenar commented Mar 12, 2014

I'm using it in one larger project with complicated authentication logic and I can say implementation works as expected.

@5minpause Have you implemented policy classes (examples here) for each resource you want to authorize?

Owner

seanlinsley commented Mar 17, 2014

This looks good to me; sorry for the delay.

Thanks for the contribution @romansklenar 💙

@seanlinsley seanlinsley added a commit that referenced this pull request Mar 17, 2014

@seanlinsley seanlinsley Merge pull request #2857 from romansklenar/feature/WIP-add-pundit-ada…
…pter

Pundit authorization adapter
7363388

@seanlinsley seanlinsley merged commit 7363388 into activeadmin:master Mar 17, 2014

1 check passed

default The Travis CI build passed
Details
Contributor

mindhalt commented Apr 3, 2014

Can current_user be normally accessed with this adapter? I always get NoMethodError somehow.

Try user variable.
Something like this:

class PostPolicy < Struct.new(:user, :post)
  def update?
    user.admin? or not post.published?
  end
end
Contributor

mindhalt commented Apr 3, 2014

I'm trying to use current_user in controller block. user I've tried, too. Still NoMethodError. I'm trying to do something like this:

ActiveAdmin.register Post do
  permit_params do
    params = [:title, :content, :publisher_id]
    params.push :author_id if current_user.admin?
    params
  end
end

from here

Contributor

mindhalt commented Apr 4, 2014

Ho-ho-ho! How I could've missed this:

Notice that if your Devise model is called Member instead of User, for example, then the helpers available are:

before_action :authenticate_member!
member_signed_in?
current_member
member_session

from Devise docs

Soooo, AA has AdminUser model and helper will be current_admin_user or current_active_admin_user from BaseController of AA, which will give same results.

Contributor

mindhalt commented Apr 4, 2014

Maybe, change everywhere in docs and etc current_user to current_active_admin_user? What do you think, @seanlinsley ?

Owner

seanlinsley commented Apr 4, 2014

Honestly I'd prefer that Active Admin not automatically create the AdminUser model in the first place, and not run the setup for Devise unless you specifically ask for that.

Owner

seanlinsley commented Apr 4, 2014

I've never used the AdminUser in my own projects -- I've always added permissions to my normal User model that marked a given user has being an admin and thus having access.

I agree with @seanlinsley. I tried it couple of times to separate admin user from usual user model and it always led to more code and more complicated solutions.

Contributor

mindhalt commented Apr 4, 2014

I don't understand. We have config.current_user_method = :current_admin_user in initializer which is used in current_active_admin_user. You don't use current_active_admin_user?

Owner

seanlinsley commented Apr 4, 2014

Oh, you're talking about current_active_admin_user! Sorry, I misunderstood 🐱

I think it'd be good to explain how Devise works in the documentation, but I think it'd be too cumbersome to use current_active_admin_user in every example throughout the docs.

Owner

seanlinsley commented Apr 15, 2014

Does anyone here have comments on #3068?

t-anjan commented May 7, 2014

Could somebody give details of how to actually authorize the actions in ActiveAdmin? I have configured ActiveAdmin to use the Pundit adapter. I have setup an app/policies/user_policy.rb file with the required rules, such that non-admins are not allowed to do anything with the User resource. But when I navigate to the /admin/users path in ActiveAdmin as a non-admin, I am able to perform all actions.

I have asked the question on SO too.

This adapter doesn't seem to execute the authorize method as it should. I will open a new issue.

I get a Pundit::AuthorizationNotPerformedError in Admin::DashboardController#index error when accessing the dashboard. Any ideas?

To anyone else who gets here via google and has trouble with Pundit::AuthorizationNotPerformedError I posted an explination and solution over here on SO: http://stackoverflow.com/a/34980939/511168

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment