Move return authorizations from spree_backend to OFN #4450
Conversation
luisramos0
changed the title
luisramos0
force-pushed
the
return_auth
branch
from
2441f52 to
e38bcb1
Compare
| create.after :associate_inventory_units | ||
|
|
||
| def fire | ||
| @return_authorization.public_send("#{params[:e]}!") |
There was a problem hiding this comment.
This is a terrible pattern for security. I can't even see any authorisation here. The client can call any method ending in ! on this object. Good to have that visible in our code now.
There was a problem hiding this comment.
Um... I'm not sure we should merge this as is?
There was a problem hiding this comment.
Or at least make an issue to change this line, and put it at the top of Tech Debt.
There was a problem hiding this comment.
this is live code .
this is not only spree 2 code but also spree master code 🙈 🤕
https://github.com/spree/spree/blob/98fa319554e9bad01fee9a9f56500eea99847d29/backend/app/controllers/spree/admin/return_authorizations_controller.rb#L11
I think the security risk is really just adding return authorizations.
I'll create a tech debt issue.
https://github.com/openfoodfoundation/ofn-security/issues/21
|
@luisramos0 what or where is the return authorizations page? 😁 |
|
ahah, it's a dead feature but I think we decided to keep it at some point during the upgrade to spree v2... anyway, it's in orders, first you have to ship the order to be able to register return authorizations. |
|
Hey @luisramos0, I am not so familiar with this feature, but I had a look before and after staging the PR, and all behaved similarly. I was able to create, edit, delete Return Authorizations, and these had the effect on the values in the orders table. I found some small issues/possible improvements, but I think these are not related with this PR:
Should I open Issues on these? Or is this rather a fearture which will be removed in the future? I moved it to Ready to Go, but please let me know if there is something more specific I should check. Thank you! :-) |
luisramos0
force-pushed
the
return_auth
branch
from
e38bcb1 to
e2087bc
Compare
|
rebased to resolve conflict. Thanks Filipe! Merging. |
What? Why?
Part of #4050
This moves return authorizations from spree_backend to OFN. No change in functionality.
What should we test?
Make sure the return authorizations page is work correctly for a given order.
Release notes
Changelog Category: Changed
Imported views related to return authorizations management from spree_backend to make OFN more independent of Spree.