Backend: Extra cancancan validations for some URL resource links #2823
Conversation
| @@ -7,7 +7,7 @@ | |||
| <li> | |||
| <%= link_to t('spree.new_order'), new_admin_order_url, id: 'admin_new_order', class: 'btn btn-primary' %> | |||
| </li> | |||
| <% end if can? :create, Spree::Order %> | |||
| <% end if can? :manage, Spree::Order %> | |||
There was a problem hiding this comment.
why manage if the link is for creating orders only?
There was a problem hiding this comment.
Hello @kennyadsl .
There are two classes to manage the permissions sets Spree::PermissionSets::OrderManagement and Spree::PermissionSets::OrderDisplay. The OrderManagement establishes the ':manage' permissions. The display establishes the ':display' and ':admin' permissions. If I want to restrict a user with only the permission OrderDisplay, I have to check for ':manage' permission as for some reasons, the ':create' permission on OrderDisplay is still allowing the button to be rendered, allowing the user to click the New Order button, and creating an empty order that it cannot be modified, not great user experience I guess. I'm not sure why the ':create' is still available on OrderDisplay. Do you know how to fix this issue? Maybe I misunderstood the OrderDisplay role. Let me know your thoughts about it.
There was a problem hiding this comment.
There was a problem hiding this comment.
By default only this permissions set is activated. I think that at line 10 it sets the create ability for Order. Are you setting the other permissions for a specific user in some way via code?
There was a problem hiding this comment.
From what I can understand from @memotoro's comment and the proposed patch is that, OrderDisplay works only to create orders without any further actions, whereas OrderManagement allows full control over them. Since we're only talking about backend, this feels reasonable.
As @tvdeyen said on his comment below, if you can create an order, you probably should be able to update it as well.
| @@ -3,7 +3,7 @@ | |||
| <% content_for :page_actions do %> | |||
| <% if @order.shipments.any? &:shipped? %> | |||
| <li> | |||
| <% if can? :create, Spree::ReturnAuthorization %> | |||
| <% if can? :manage, Spree::ReturnAuthorization %> | |||
There was a problem hiding this comment.
@kennyadsl Same comment with the OrderDisplay issue mentioned above.
|
I think Please fix the specs. Thanks for the contribution |
|
@kennyadsl @tvdeyen . Changes as requested. Let me know your thoughts. Regards |
|
Looks good. Would you mind squashing your commits? |
|
Hello @ericsaupe @tvdeyen @kennyadsl Sorry for the late replay. I had issues merging last time that I've squashed my commits to make it a single commit. Have a look and it is passing the CI tests. Regards |
kennyadsl
dismissed
their stale review
Dismissing my review since specs are not broken anymore
|
Hello @kennyadsl @aitbw . Rebase the commit against the current master. Specs were added before to test the feature. Regards |
|
Thanks @memotoro!
What do you mean? |
|
Hello @kennyadsl. What I meant is that I already changed a spec to test this feature, and I did it before the rebase that I just did. This commit is only a rebase against the current master, so it will pass the test cases now. Sorry for the misunderstanding. The spec I modified is |
There was a problem hiding this comment.
Thanks for rebasing, @memotoro 🙌 left two extra comments as further improvements
Regarding specs: I still think that you should add some tests that reflect your changes; I know that you fixed one spec associated to what you changed but these files were affected and they don't have any tests associated to reflect said changes:
backend/app/views/spree/admin/users/items.html.erbbackend/app/views/spree/admin/users/orders.html.erbbackend/app/views/spree/admin/images/index.html.erbbackend/app/views/spree/admin/payments/index.html.erbbackend/app/views/spree/admin/return_authorizations/index.html.erb
If I remove what you introduced (on those files) and run the backend spec suite, I still get 💯 green. We'd like to avoid these kind of problems in the future, and I think that's the best way to do so. What do you think?
| @@ -13,29 +13,31 @@ | |||
| <%= render 'new', product: @product, image: Spree::Image.new(viewable: @product) %> | |||
| </div> | |||
|
|
|||
| <fieldset class="no-border-bottom"> | |||
| <legend align="center"><%= t(".upload_images") %></legend> | |||
| <% if can?(:create, Spree::Image) %> | |||
There was a problem hiding this comment.
Following the same idea with orders, shouldn't we have :manage here instead of :create?
| <% if can? :manage, Spree::Order %> | ||
| <%= render 'spree/admin/shared/no_objects_found', | ||
| resource: Spree::Order, | ||
| new_resource_url: spree.new_admin_order_path %> |
There was a problem hiding this comment.
Why this change for new_resource_url? Shouldn't it stay as spree.new_admin_order_path(user_id: @user.id)?
|
Hey @memotoro! Is there any chance for you to complete the PR. I think the last missing piece is this Angel concern, and are good to go. Thanks! |
|
@memotoro I'll be pushing this PR forward, thanks for the contribution! |
Hello
In the backend system, some resource link are accesible without any cancancan validations. These extra simple conditionals will help to avoid a leak on those resources for users without the right roles and permissions sets. These changes simply avoid rendering the "create new resource" links if the user has no permissions to do so, given a cleaner backend user experience.
Regards