-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix permissions for users to change their own orders #2787
Fix permissions for users to change their own orders #2787
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that change. You have choosen to not split the rules into two. What is the reason?
@@ -11,6 +11,9 @@ def activate! | |||
can [:read, :update], Order do |order, token| | |||
order.user == user || (order.guest_token.present? && token == order.guest_token) | |||
end | |||
cannot :update, Order do |order| | |||
order.completed? | |||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not have two seperate rules? One read and one update rule. You mentioned that in your commit message. What was the reason you did it this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started with that in mind and this has been my first attempt commit. Than I saw #547 and investigated a bit more in the cancancan doc and I found this page in their wiki which says that rules are logically or'ed and I think it's a good way to avoid duplicate logic or move it into a method which I don't know where to place.
What do you think? I'd go more with this and change the commit message to better reflect its content (thanks for noticing that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, it seems the right way to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is absolutely fine 👌
@@ -11,6 +11,9 @@ def activate! | |||
can [:read, :update], Order do |order, token| | |||
order.user == user || (order.guest_token.present? && token == order.guest_token) | |||
end | |||
cannot :update, Order do |order| | |||
order.completed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be done as cannot :update, Order, &:completed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but I prefer this way for clarity and cleaner git history if the block content changes in the future.
Specs ran successfully, not sure what's happening, I'll try to re-trigger this build. |
Now our permissions set allows to change completed orders only by user that placed it (and admin). This is not what the majority of users is expecting even if there are stores that let user change their own completed orders.
It basically moves authorization into each action that needs it. In specs we were testing that passing an order :id as params to the request it would have updated the order with that id instead of the current_order. This was not true, we were just authorizing actions against that order but actually performing all actions against current order so I think it's better to remove those specs.
2bd609f
to
95e63e8
Compare
ref #546 and #2772
With this PR the default permissions set does not allow to change an order after its completion by its owner (other users cannot change it anyway 🔐 )
This is what the majority of users expect.
I also changed some specs which were just testing the wrong thing, more details in the commit messages.
cc @sebfie