cancancan icon indicating copy to clipboard operation
cancancan copied to clipboard

build_ressource: conditions hash mess with user params/default value

Open Berardpi opened this issue 5 years ago • 3 comments

Steps to reproduce

Given the following:

class Boat; end

class Ability
  include CanCan::Ability

  def initialize(super_user: false)
    can :create, Boat if super_user
  end
end

Ability.new(super_user: true).attributes_for(:create, Boat) is empty ({}), so in a create route, @boat is initialized using params only

If we had a new boolean attribute slow to Boat, which defaults to false, and add the following so that normal user can only create slow boats:

class Boat; end

class Ability
  include CanCan::Ability

  def initialize(super_user: false)
    can :create, Boat if super_user
    can :create, Boat, slow: true
  end
end

Now Ability.new(super_user: true).attributes_for(:create, Boat) => {slow: true}, so any boat created by a super user, unless explicitly set otherwise, is now slow. This is because load_ressource initializes @boat using a combination of params and attributes_for.

Expected behavior

🤷‍♂. I don't see a good solution for this. It seems to be built this way, so maybe a doc warning?

Overall I find the use of conditions hash in build_ressource for create routes to be misleading as it messes with user params/default values.

Side question: would a PR to add an option to load_ressource/load_and_authorize_ressource which disables this behavior be accepted, or are we encouraged so bypass load_ressource?

System configuration

Rails version: 6.0.0

Ruby version: 2.5.5

CanCanCan version 3.0.1

Berardpi avatar Mar 23 '20 17:03 Berardpi

The option to restrict params using hash conditions is really useful and it makes sure that no one does/create something that they are not suppose to do (That's the whole point of authorization). For your case a better approach would be to use the following:

def initialize(super_user: false)
  can :create, Boat if super_user
  can :create, Boat, slow: true if !super_user    # Or unless super_user
end

# Or

def initialize(super_user: false)
  if super_user
    can :create, Boat
  else
    can :create, Boat, slow: true
  end
end

uxxman avatar Jun 20 '20 17:06 uxxman

CCC v3.1.0. Ran into a similar case as the OP, with similar confusion. Now trying to understand how ability composition should be expected to work. Given this...

can :new, Foo, bar: 22
can :new, Foo, bar: 33

...I'd expect a new Foo with {bar: 33} in my #new action. But it blows up with '401 Unauthorized'.

Given this...

can :new, Foo, bar: 22
cannot :new, Foo
can :new, Foo, bar: 33

...I'd again expect a new Foo with {bar: 33} in my #new action. It doesn't blow up, but :bar is 22.

The docs say that successive 'can' rules are OR'ed together, which is cool for most things, but it's not making sense to me in the context of #new/#create.

I realize that conditionals may sometimes be used to achieve the intent in cases like these, but still, we're leaning on CanCanCan more and more in our project (thank you, by the way), and I'd like to be able to read an ability file with some fluency. What's the internal logic at play that explains the behavior here?

Thanks for any insight!

trlorenz avatar Oct 21 '20 00:10 trlorenz

Would be possible to get a gist to reproduce the issue? I am not sure I fully understood your use case.

coorasse avatar Jun 23 '22 12:06 coorasse