Remove cancancan customizations #3148

coorasse · 2019-03-18T08:06:47Z

Description
While testing solidus against cancancan 3.0 I noticed that the usage was "not correct (Euphemismus)".
This PR aims to remove the customizations introduced to cancancan, which are not necessary.
The current customizations are not compatible with version 3.0.0 of cancancan since Solidus makes use of merge of Abilities. The 3.0.0 has a breaking change which changes how aliased_actions are merged.

I removed all custom aliases and replaced display with read. Also, I corrected the rule which would have caused issues during the migration.

This is probably a breaking change, because it will cause problems for the plugins.
Plugins should now use standard cancancan actions and replace display with read.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)

core/spec/models/spree/ability_spec.rb

coorasse · 2019-03-18T08:08:04Z

api/app/controllers/spree/api/base_controller.rb

@@ -133,13 +133,13 @@ def find_product(id)

      def product_scope
        if can?(:admin, Spree::Product)
-          scope = Spree::Product.with_deleted.accessible_by(current_ability, :read).includes(*product_includes)
+          scope = Spree::Product.with_deleted.accessible_by(current_ability).includes(*product_includes)


this is not necessary anymore. The default is :index which is by default aliased by :read.

Hi @coorasse! But if :read alias was :show shouldn't it be changed like this scope = Spree::Product.with_deleted.accessible_by(current_ability, :show).includes(*product_includes) to keep same logic? Isn't your change changing scope from show to index now?

it changes from :show to :index but all the rules are defined for :read instead of :display which was including also :read. it's quite complicated, I know, I don't know why all these aliases were generated before. accessible_by(ability, :show) would not make sense because you want a list of objects. :show is for single instances.

(current_ability, :read) made scope of products that user is authorized for action :show I gues.

:read was aliased by :display and rules were (almost) always defined on :display. You can see that most of the changes are about that: https://github.com/solidusio/solidus/pull/3148/files#diff-82b30f937458e7c1052c326354dc18cbL8

I agree that :index is the right thing here, as :show would need to check every single instance of a Product if its able to be shown to the user, what is definitely not what we want here.

coorasse · 2019-03-18T08:09:02Z

api/app/controllers/spree/api/orders_controller.rb

@@ -49,8 +49,9 @@ def empty
        render plain: nil, status: 204
      end

+      # TODO: this action is never accessible. shall we simply remove it?


this method was never accessible. why don't we remove it?

@coorasse Could you explain why is it unaccessible?

it was defined with :index and since :read was an alias of :show, no rule was defined for :index. in fact, I replaced it with a random action called :impossible_action and no test failed. There is only one test and it checks that this endpoint should not be accessible 🤷‍♂️ . That's why I asked myself: "why don't we remove it?"

@coorasse As I've tested, :manage gives acces to all rules, also theese not defined, so a user with can :manage, Order will go through authorize! :impossible_action, Order too. And am I missing somethin or :index was defined in display: alias_action :index, :read, to: :display

Oh right! manage authorizes you! I forgot that 🤦‍♂️ . To do it properly here we should leave authorize! :index, Order but then this test will fail. So I'd say we could use authorize! :admin, Order and keep the current behaviour.

coorasse · 2019-03-18T08:12:11Z

core/app/models/spree/ability.rb

-    def alias_actions
-      clear_aliased_actions
-
-      # override cancan default aliasing (we don't want to differentiate between read and index)


this is supported already. you can use read.

coorasse · 2019-03-18T08:12:26Z

core/app/models/spree/ability.rb

-      clear_aliased_actions
-
-      # override cancan default aliasing (we don't want to differentiate between read and index)
-      alias_action :delete, to: :destroy


delete was never used

coorasse · 2019-03-18T08:12:37Z

core/app/models/spree/ability.rb

-      alias_action :delete, to: :destroy
-      alias_action :edit, to: :update
-      alias_action :new, to: :create
-      alias_action :new_action, to: :create


new_action was never used

coorasse · 2019-03-18T08:13:47Z

core/lib/spree/permission_sets/default_customer.rb

        can :create, Order
-        can [:read, :update], Order do |order, token|
+        can [:show, :edit, :update], Order do |order, token|


block conditions should be defined only for actions on object instances, not classes, therefore this rule applies only on :show, :edit, :update. This was actually working by mistake since version 2.0 of cacancan

coorasse · 2019-03-18T08:14:56Z

core/spec/models/spree/ability_spec.rb

@@ -254,7 +256,7 @@ def initialize(_user)
      context 'requested by same user' do
        let(:resource) { user }
        it_should_behave_like 'access granted'
-        it_should_behave_like 'no index allowed'
+        it_should_behave_like 'index allowed'


why should it not be allowed?

kennyadsl · 2019-03-18T08:23:45Z

@coorasse wow, thank you, I love this change! Do you think there's a way to deprecate the old usage any way before removing it?

coorasse · 2019-03-18T08:41:07Z

I cannot really think of a deprecation warning: we could override can? and accessible_by methods of cancancan and raise a warning when can? is called with delete, new_action, read or display and when accessible_by is called with :read or :display.

The calls to accessible_by(ability, :read) should be replaced by accessible_by(ability) but this would not work before this update! ⚠️

Also can? :read, resource should be changed to the specific action can? :show or can? :edit, etc, but again: it would not work before this change.

The calls to can? :delete, resource could already be replaced by can? :destroy, resource.

Also the calls to can? :new_action, resource could already be replaced by can? :create, resource.

The main problem was this alias:

alias_action :show, to: :read
alias_action :index, :read, to: :display

which caused read to be the same of show and display to be both read and index...
I am not even sure what this means 😄. If you notice, in fact, the changes I had to do in the code itself were not actually many. It is more a renaming issue. I can make a new comment and show in detail how/why the problem is raised only in version 3.0.0.

I let you guys decide about how to move here: I have not enough knowledge about the project.
I pointed the finger 😄 and I can provide all help you need but I think this might break quite some stuff.

coorasse · 2019-03-18T08:58:36Z

In cancancan 3.0.0 when two abilities are merged, also the aliased_actions are merged.
Solidus defines the following aliases in base ability:

{
:destroy=>[:delete], 
:update=>[:edit], 
:create=>[:new, :new_action], 
:read=>[:show], 
:display=>[:index, :read]
}

base ability is the merged with other abilities where the default aliases are defined. The following:

{
  :read => [:index, :show],
  :create => [:new],
  :update => [:edit]
}

with the following aliases as a result:

{
:destroy=>[:delete], 
:update=>[:edit], 
:create=>[:new], 
:read=>[:index, :show], 
:display=>[:index, :read]
}

The call that Solidus performs on Spree::Order.accessible_by(ability, :read) in version 3.0.0 fails because the following rule:

can [:read, :update], Order do |order, token|
  order.user == user || (order.guest_token.present? && token == order.guest_token)
end

is now identified as possible interesting rule and cancancan raises an issue because this is a block rule (do...end) defined on a collection action read

tvdeyen · 2019-08-22T15:10:11Z

api/app/controllers/spree/api/orders_controller.rb

@@ -50,7 +50,7 @@ def empty
      end

      def index
-        authorize! :index, Order
+        authorize! :admin, Order


Why this change?

kennyadsl · 2020-08-28T08:31:26Z

Closing in favor of #3701.

houndci-bot reviewed Mar 18, 2019

View reviewed changes

core/spec/models/spree/ability_spec.rb Outdated Show resolved Hide resolved

core/spec/models/spree/ability_spec.rb Outdated Show resolved Hide resolved

coorasse commented Mar 18, 2019

View reviewed changes

coorasse mentioned this pull request Mar 18, 2019

Wrong usage of cancancan actions and aliases spree/spree#9303

Closed

coorasse force-pushed the remove_cancancan_customizations branch from 269e210 to 00c266c Compare March 18, 2019 08:26

Remove cancancan customizations

5b3f8bd

coorasse force-pushed the remove_cancancan_customizations branch from 00c266c to 5b3f8bd Compare March 23, 2019 08:20

kennyadsl added the Needs Work label Apr 1, 2019

This was referenced Apr 12, 2019

Upgrade to CanCanCan 3.0 #3174

Closed

RFC: Support Rails 6 #3053

Closed

tvdeyen reviewed Aug 22, 2019

View reviewed changes

aldesantis added this to the 2.11 milestone May 22, 2020

filippoliverani mentioned this pull request Jul 10, 2020

Remove CanCanCan custom actions aliases (cont.) #3701

Merged

2 tasks

kennyadsl added Needs Core Team Review and removed Needs Work labels Aug 28, 2020

kennyadsl removed this from the 2.11 milestone Aug 28, 2020

kennyadsl closed this Aug 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove cancancan customizations #3148

Remove cancancan customizations #3148

coorasse commented Mar 18, 2019

coorasse Mar 18, 2019

jkojro Mar 19, 2019

coorasse Mar 19, 2019

jkojro Mar 19, 2019 •

edited

Loading

coorasse Mar 19, 2019

tvdeyen Aug 22, 2019

coorasse Mar 18, 2019

jkojro Mar 19, 2019

coorasse Mar 19, 2019

jkojro Mar 20, 2019 •

edited

Loading

coorasse Mar 21, 2019

coorasse Mar 18, 2019

coorasse Mar 18, 2019

coorasse Mar 18, 2019

coorasse Mar 18, 2019

coorasse Mar 18, 2019

kennyadsl commented Mar 18, 2019

coorasse commented Mar 18, 2019

coorasse commented Mar 18, 2019

tvdeyen Aug 22, 2019

kennyadsl commented Aug 28, 2020

Remove cancancan customizations #3148

Remove cancancan customizations #3148

Conversation

coorasse commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkojro Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkojro Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl commented Mar 18, 2019

coorasse commented Mar 18, 2019

coorasse commented Mar 18, 2019

Choose a reason for hiding this comment

kennyadsl commented Aug 28, 2020

jkojro Mar 19, 2019 •

edited

Loading

jkojro Mar 20, 2019 •

edited

Loading