CanCanCommunity / cancancan

The authorization Gem for Ruby on Rails.
MIT License
5.54k stars 633 forks source link

Class-based ability check is always returning true even when it shouldn't #586

Open glennfu opened 5 years ago

glennfu commented 5 years ago

Steps to reproduce

Based on this article: https://github.com/CanCanCommunity/cancancan/wiki/Checking-Abilities#checking-with-class

I expected to be able to set up this Class-based ability checking as follows:

User Setup:

@user.roles UserRole Load (0.4ms) SELECT "user_roles".* FROM "user_roles" WHERE "user_roles"."user_id" = $1 LIMIT $2 [["user_id", 7], ["LIMIT", 11]] => #<ActiveRecord::Associations::CollectionProxy [#<UserRole id: 25, account_id: 1, user_id: 7, role: "low", created_at: "2019-04-15 16:19:26", updated_at: "2019-04-15 16:19:26">, #<UserRole id: 26, account_id: 3, user_id: 7, role: "low", created_at: "2019-04-15 16:19:26", updated_at: "2019-04-15 16:19:26">]>

Ability:

can :claim, Account, user_roles: { user_id: user.id, role: ["lead", "admin"] }

ability = Ability.new(user) accessible_accounts = Account.accessible_by(ability, :claim)

=> #<ActiveRecord::Relation []>

Expected behavior

Since there exists no roles by which this user can claim anything, I expect to see:

ability.can?(:claim, Account)
=> false

Actual behavior

ability.can?(:claim, Account)
=> true

Since the original doc said Think of it as asking "can the current user read a project?". The user can read a project, so this returns true I expected this question to be "Can the user claim an Account?" to which the answer is "No, there are no Accounts that this User can claim".

System configuration

Rails version: Rails 6.0.0.beta2

Ruby version: ruby 2.6.1p33

CanCanCan version 3.0.0

coorasse commented 5 years ago

A gist which reproduces your issue would help me. Please provide one following the issue template. Thank you!

duffyjp commented 5 years ago

I'm pretty sure that's how it's always worked. You'd have to check an instance instead:

ability.can?(:claim, Account.new)

If you can :claim an Account in any way, the test against the class will return true.


Edit: Oh hey, you're in Madison too? Nice weather we're having today, isn't it? :)

glennfu commented 5 years ago

I moved from Madison, but I sure miss it!

I think this is maybe just a communication error, but when you say "you can :claim an Account in any way" I interpret that statement as present tense, so "at this moment". However in the data, there are no Accounts by which this user can claim. ability.can?(:claim, @account) will return false, correctly, for every Account that exists.

Are you saying it really means: "If it's possible for some User to potentially one day in the future claim some Account", then it's basically a noop function that returns true. Is there a way to write the question I want to ask in cancancan syntax? I want to ask "are there any Accounts RIGHT NOW that this user has permission to claim?"

EDIT: Also to clarify, this returns the answer I'm hoping to translate to cancancan: Account.accessible_by(ability, :claim).count > 0

duffyjp commented 5 years ago

I use an equivalent statement quite a bit:

Account.accessible_by(ability, :claim).exists?

I don't know of anything shorter. I just avoid checking abilities on classes as a rule.

coorasse commented 4 years ago

I see your concern here. This could be improved, I agree. A PR is welcome.

jeropaul commented 1 month ago

I know this is getting old but I thought I'd added context. This references the original cancan docs for clarity; as I haven't found an exact snippet in the updated cancancan docs

TLDR: you need to build a model to authorise against! and be aware that you do not want to build and add this object to the collection. Class authorisation may be unexpectedly broad