Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
durierem opened this issue Sep 7, 2023 · 13 comments

Comments

@durierem
Copy link

durierem commented Sep 7, 2023

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
Copy link

chmich commented Sep 9, 2023

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
Copy link
Author

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
Copy link

chmich commented Sep 10, 2023

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

@mameier
Copy link

mameier commented Sep 11, 2023

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

My understanding is:

  • :new shows the form to :create an object.
  • :editshows the form to :update an object.

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
Copy link

chmich commented Sep 11, 2023

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
Copy link

chmich commented Sep 11, 2023

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
Copy link
Author

@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
Copy link

chmich commented Sep 12, 2023

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
Copy link

chmich commented Sep 15, 2023

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
Copy link
Author

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
Copy link

@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
Copy link

chmich commented Dec 23, 2023

Suggestion:

  • can_exact function which works similar to can, but without aliases
  • cannot must never follow any aliases!

@foton
Copy link

foton commented May 22, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants