why is my CanCan Ability class overly permissive?

I'm (finally) wiring CanCan / Ability into my app, and I've started by writing the RSpec tests. But they're failing — my Abilities appear to be overly permissive, and I don't understand why.

First, the Ability class. The intention is that non-admin users can manage only themselves. In particular, they cannot look at other users:

class Ability
include CanCan::Ability

  def initialize(user)
    user ||= User.new           # create guest user if needed
    if (user.has_role?(:admin))
      can(:manage, :all)
    else
      can(:manage, User, :id => user.id)
    end
  end
end

The RSpec tests:

require 'spec_helper'
require 'cancan/matchers'

describe Ability do
  before(:each) do
    @use开发者_如何学Gor = User.create
  end

  describe 'guest user' do
    before(:each) do
      @guest = nil
      @ability = Ability.new(@guest)
    end

    it "should_not list other users" do
      @ability.should_not be_able_to(:read, User)
    end

    it "should_not show other user" do
      @ability.should_not be_able_to(:read, @user)
    end

    it "should_not create other user" do
      @ability.should_not be_able_to(:create, User)
    end

    it "should_not update other user" do
      @ability.should_not be_able_to(:update, @user)
    end

    it "should_not destroy other user" do
      @ability.should_not be_able_to(:destroy, @user)
    end
  end
end

All five of these tests fail. I've read the part of Ryan's documentation where he says:

Important: If a block or hash of conditions exist they will be ignored when checking on a class, and it will return true.

... but at most, that would only explain two of the five failures. So clearly I'm missing something fundamental.

I would expect this to work:

class Ability
  include CanCan::Ability

  def initialize(user)
    user ||= User.new           # create guest user if needed
    if (user.has_role?(:admin))
      can(:manage, :all)
    elsif user.persisted?
      can(:manage, User, :id => user.id)
    end
  end
end

I'm not sure what the behavior is defined to be if you pass :id => nil, which is what happens in the guest case, but at any rate, if you don't want the guest to access the list view, you shouldn't call can :manage, User for that user at all.

In general, I find that assigning user ||= User.new to make the ability harder to reason about.

Hey, apparently this should work, but some refactoring would help you to find the issue:

require 'spec_helper'
require 'cancan/matchers'

describe Ability do
  before(:each) { @user = User.create }

  describe 'guest user' do
    before(:each) { @ability = Ability.new(nil) }
    subject { @ability } # take advantage of subject

    it "should not be an admin user" do
      @user.should_not be_admin
      @user.should be_guest
    end

    it "should_not show other user" do
      should_not be_able_to(:read, @user)
    end

    it "should_not create other user" do
      should_not be_able_to(:create, User)
    end

    it "should_not update other user" do
      should_not be_able_to(:update, @user)
    end

    it "should_not destroy other user" do
      should_not be_able_to(:destroy, @user)
    end
  end
end

Note that also I removed this example @ability.should_not be_able_to(:read, User).

Hope it helps you.

I've got this bad habit of answering my own questions, but I give props to @jpemberthy and @Austin Taylor for pointing me in the right direction. First (and this is cosmetic), I added this to my User model:

class User
  ...
  def self.create_guest
    self.new
  end

  def guest?
    uninitialized?
  end
end

and cleaned up my Abilities model accordingly:

class Ability
  include CanCan::Ability

  def initialize(user)
    user ||= User.create_guest
    if (user.admin?)
      <admin abilities here>
    elsif (user.guest?)
      <guest abilities here>
    else
      <regular user abilities here>
    end
  end
end

But the real fix was in my RSpec tests. Since User has validations on email and password fields, my original code of:

before(:each) do
  @user = User.create
end

was failing, thus creating an uninitialized @user. Since the :id field was nil, the Ability clause:

can(:manage, User, :id => user.id)

was succeeding with a guest user because nil == nil (if that makes sense). Adding the required fields to satisfy the User validations made (almost) everything work.

Moral: just as @jpemberthy suggested in his code, always include a test to make sure your user objects have the privileges that they are supposed to! (I still have another question regarding CanCan, hopefully less boneheaded than this one, appearing in a StackOverflow topic near you...)