CanCanCommunity / cancancan

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

Can't apply rules to `create` action without affecting `new` because of aliasing #832

Open durierem opened 10 months ago

durierem commented 10 months ago

Steps to reproduce

I have a resource for which I would want to have different rules for #new and #create. But when I do:

can :new, Foo # renders a form
cannot :create, Foo { |foo| <some condition on foo> } # some options for foo may not be authorized

Since create is an alias for new and create I can't have seperate rules for create only without affecting new

Can I opt-out of aliasing somehow?

Expected behavior

N/A

Actual behavior

N/A

System configuration

Rails version: 6.1.7

Ruby version: 3.2.0

CanCanCan version: 3.5.0

chmich commented 10 months ago

similar case with :update and :edit, see my Stackoverflow issue.

Reason for this behaviour is described at cancancan docs.

@durierem: Could it be a workaround to put another can :new ... after the cannot? In my case (:update) this did the trick. For me this was not even the solution as described in Stackoverflow, but maybe it helps you?

durierem commented 10 months ago

Redefining can :edit afterwards works but is very impractical. In my case, the original can definition is not straightforward and would require copy-pasting and keep in sync multiple rules between multiple files.

I don't understand how these aliases have been thought through without realizing that it clashes with the very same action it tries to alias. The reasoning is given as:

This will be very convenient when we will authorize the Rails Controller actions.

Which, to my understanding, doesn't change anything apart from the fact that I can write can :update instead of can [:edit, :update]. When authorizing actions, we still check for the individual action name can?(:edit, foo) or can?(:update, foo). May I have missed something?

chmich commented 10 months ago

Same like in my case: The first can ... has a lot of logic that i dont want to touch.

mameier commented 10 months ago

For me, the aliases are very reasonable, even with the clash of names of :create and :update.

My understanding is:

so there is no reason to allow a :new without also allowing :create and no reason to allow :edit without also allowing :update.

All case I saw that seem to require such a decoupling were misuse of :edit to actually :show an object or something similar.

Use dedicated actions.
You can use partials to combine the common parts of the view, but use dedicated controller actions.
You are free to define additional actions to the standard CRUD actions and define abilities on these extra actions.

chmich commented 10 months ago

I set up a new project, just for testing this.

In a current project, I have read-only permissions through a form with a disabled=true and forbidden :update action.

can

can [:edit, :new], Customer 

# in ability.rb results in:

can? :edit, @customer #=> true
can? :update, @customer #=> false
can? :new, @customer #=> true
can? :create, @customer #=> false

This way you can create a read-only privilege set that allows you to see the forms, but not to write anything to the database (:update / :create).

But a

can [:update, :create], Customer 

# in ability.rb results in:

can? :edit, @customer #=> true
can? :update, @customer #=> true
can? :new, @customer #=> true
can? :create, @customer #=> true

Now i understand what @mameier says: in that case theese linkins making sense! Because if update is allowed there would be no reason for restrict the form. And that exactly is described at the docs and thats making sense. But when is the first case I want to restrict :edit, but need :update for javascript, by example, and want to set a custom ability check inside the controller action?

cannot

But the point comes now:

can :manage, :all
cannot [:update, :create], Customer

can? :edit, @customer #=> false
can? :update, @customer #=> false
can? :new, @customer #=> false
can? :create, @customer #=> false

cannot follows the same aliases as can, but in the opposite direction, and now cannot do what is possible in the first can example.

chmich commented 10 months ago

I understand the argument that :edit and :update are closely verbs. But either they are the same then aliases should work in both directions consequently, but that would break thousands existing apps.

Or cannot should be excluded from all theese aliases and this should be mentioned on docs. I strongly vote for that.

This should be clearified by the maintainers.

durierem commented 10 months ago

@mameier Your point is completely valid... in an ideal world.

Also, organizing my app to satisfy CanCanCan's assumptions feels off. It should be the other way around.

chmich commented 10 months ago

Yes. Its really better to use dedicated controller actions for new apps like @mameier says.

But for the cannot action, which is rarely used and only in very specific places, I see aliases in the wrong place.

chmich commented 9 months ago

In my example, the customer did not want to have an edit and a show button separately. So if you really want to have "dedicated actions" at this point, that means separate routes (edit/show), separate request tests, separate route tests, and separate controller actions. The controller action needs to provide instance variables. And in the index view you would need a condition: If update allowed, then take the edit route helper, otherwise take the show route helper. For each view in the entire application

From a concise naming point of view, you have to separate: edit is edit and show is show. The same app has a "backoffice" with edit and show actions, and yes, of course it is cleaner.

But from a functional point of view, it is a lot of code for nothing else than always rendering the same partial with a form that is disabled in one case and enabled in the other case.

Why Html is givig us the disabled attribute for controls? If it is possible and valid to set a submit button to disabled, then a allowed :edit and a restricted :update is valid!

My example application was taken from another developer. In short, I'd say: @mameier's statement should be a general recommendation, and in case of doubt: be more conciese. But is should not be a requirement.

durierem commented 9 months ago

Imagine a form for which some controls are locked because of insufficient permissions or a paid feature you don't have access to. In this case, it seems perfectly logical to me to have the update action be more restrictive than the edit action.

newfylox commented 6 months ago

@mameier

so there is no reason to allow a :new without also allowing :create and no reason to allow :edit without also allowing :update.

Of course there is a reason in a belongs_to and/or has_many associations and having multiple roles/permissions.

In my case, I want to create a Member that belongs_to a plan on a specific condition

can [:create], Member, plan: { condition: false }

The user has many roles and permissions that let him create members with a dropdown of multiple plans. I'm not gonna create a MembersController for each different plan only for having different [:new, :create].

I want a single new action as I have a single create action so that Cancancan will handle the rest

def new
  @member = Member.new
end
chmich commented 6 months ago

Suggestion:

foton commented 1 month ago

Or just add some way to disable/remove selected aliases.