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

.has_role? :user returns false #4

Closed
vlad opened this issue Aug 14, 2012 · 4 comments
Closed

.has_role? :user returns false #4

vlad opened this issue Aug 14, 2012 · 4 comments

Comments

@vlad
Copy link

vlad commented Aug 14, 2012

Great job wrapping up role_model! This is actually a role_model suggestion but I guess I want to point that the example:

class User < ActiveRecord::Base

acts_as_user :roles => :manager, :admin

end

suggests that abilities are checked in the following order:

app/abilities/users.rb
app/abilities/manager.rb
app/abilities/admin.rb

And indeed, I confirmed that /users.rb is called for users with empty roles, and yet of course has_role? :user returns false. Since, of course, you're keeping canard consistent with role_model, then an end-user work around would explicitly specify the :user role in the list of roles and explicitly set each new user to be a :user,

acts_as_user :roles => :user, :manager, :admin

Of course, while it's easy to add a :user role later and add the :user role to every existing user if someone really feels the need to do this, maybe it would be worth documenting a sentence or two about this inconsistency.

@james2m
Copy link
Owner

james2m commented Aug 14, 2012

@vlad Thanks. The reason is there may be abilities that are not optional and are common to all users such as;

cannot :destroy, User, :id => user.id

I treat roles as additions to the common abilities. So even if you don't have any roles defined you can still define abilities for a user.

class User < ActiveRecord::Base
  acts_as_user
end

Will load app/abilities/users.rb. The roles augment the common abilities with further abilities that role can perform. If the ability is optional it should be defined in a role, if it is common to all users it goes in the common (user) abilities.

Given your example, I would ask yourself what your :user role does that is not common to all users and separate them out into a role that describes the difference.

@james2m
Copy link
Owner

james2m commented Aug 14, 2012

I think we are approaching this from different angles. I don't regard the User abilities as a role. I approach it like this;

u = User.create(....)
u.roles = [:admin]
u.save

u.is_a? User  # true
u.has_role? :admin # true
u.has_role? :manager # false
u.has_role? :user # false

In fact I almost never use has_role? for anything except perhaps as a catchall for an admin area in the controller

class Admin::BaseController < ApplicationController
  before_filter :authenticate_admin

  private
  def authenticate_admin
    raise CanCan::AccessDenied.new("You need to be an administrator to
access the admin area.") unless current_user.has_role?(:admin)
  end
end

Otherwise I leave everything to CanCan and the abilities because it means I don't have to refactor anything but the abilities if I choose to change what a role can access. This makes accessing models cleaner with CanCan's accessible_by scope, and is why I inject an ability method with acts_as_user.

I have some applications where the authentication is in a separate model and there is no User class but an Account model with a polymorphic user association to one of several classes e.g. Vendor, Customer. Both Vendor and Customer 'act_as_user' but each has multiple roles and a Vendor has very different common abilities to a Customer.

So in short, from my perspective I don't regard the User instance in the example above as having a role of :user, it is just a User instance.

If you do want has_role? to return true as per your example you could override the method in your User class;

def has_role?(role)
  String(role).classify == self.class.name || super
end

@james2m
Copy link
Owner

james2m commented Aug 21, 2012

Closing as it's not a bug but a design choice to separate the base class abilities from the Roles.

@james2m james2m closed this as completed Aug 21, 2012
@vlad
Copy link
Author

vlad commented Aug 22, 2012

Thanks for the responses James. I was mostly suggesting it might be
helpful to mention/remind this design decision in the README; you should
likely not change the way role_model works since this is a wrapper around
it. You're absolutely right that this makes sense.

On Tue, Aug 21, 2012 at 10:00 AM, James McCarthy
notifications@github.comwrote:

Closing as it's not a bug but a design choice to separate the base class
abilities from the Roles.


Reply to this email directly or view it on GitHubhttps://github.com//issues/4#issuecomment-7908025.

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

2 participants