james2m / canard

Makes role based authorization in Rails really simple. Wraps CanCan and RoleModel up with a smattering of syntactic sugar, some generators and scopes.
MIT License
125 stars 28 forks source link

.has_role? :user returns false #4

Closed vlad closed 12 years ago

vlad commented 12 years ago

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 commented 12 years ago

@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 commented 12 years ago

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 commented 12 years ago

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

vlad commented 12 years ago

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/james2m/canard/issues/4#issuecomment-7908025.