Openfoodnetwork: [Spree 2.1] Implement Strong Parameters

[Matt-Yorkley]

Rails 4 introduces Strong Paramaters, and we need to adapt the codebase to use it.

Spree's implementation is here:

https://github.com/spree/spree/blob/4cc2c2967352d3fe7652bd873e492c074ffcc3da/core/lib/spree/core/controller_helpers/strong_parameters.rb
https://github.com/spree/spree/blob/4cc2c2967352d3fe7652bd873e492c074ffcc3da/core/lib/spree/permitted_attributes.rb
https://github.com/spree/spree/blob/4cc2c2967352d3fe7652bd873e492c074ffcc3da/core/spec/lib/spree/core/controller_helpers/strong_parameters_spec.rb

[luisramos0]

We need to see what we are going to do with this.
It's not mandatory to adopt strong parameters in rails 4 because there's a gem, protected_attributes, that just keeps the behaviour as in rails 3.
@Matt-Yorkley you mentioned that protected_attributes was creating other problems. I cant remember if there was any link to that.

Anyway, I am commenting here just because if we decide to implement Strong parameters we need to remember to remove the gem protected_attributes from the Gemfile in the upgrade branch :+1:

[luisramos0]

I found that when spree added their solution for strong parameters a lot of the attr_accessible entries were removed from the models here:
https://github.com/openfoodfoundation/spree/commit/98bbbd7a9f6f2075878064b64039fa69596164e8#diff-d8e6742595fa0b47b62606ef77ddbe04

if we want to make the solution with protected_attributes work we need to re-add these attr_acessible entries to the model decorators on our side. I tried with one and it worked well. It should be straight forward to get these re-added.

[Matt-Yorkley]

So the change here is that the whitelisting of which attributes can be updated has moved from models to controllers in Rails 4+.

If we use strong_paramaters we only need to whitelist the params we use in _our_ controller actions.

If we use protected_attributes it will enforce the old logic, which will require models to use attr_accessible. So not only will we have to re-add all of the attr_accessible declarations in all the Spree models we use, but as we run bundler and update our (huge) list of gems, some of them that are designed for Rails 4 will not work, as the models they include will not have any attr_accessible declarations, and this will cause fatal errors. So we will also need to patch and maintain attr_accessible declarations in models in other gems. As things progress and we update our gems more and more, we will have to add more patches and update existing patches where things have changed...

[luisramos0]

ok, I am not sure how many of those gems we would have to patch as we are talking about models but I think that sounds reasonable and we will have to go strong parameters sooner or later anyway.
So I guess the question is: how do we do it?
this permit here fixed the build but I dont really know what I am doing :see_no_evil:
https://github.com/openfoodfoundation/openfoodnetwork/pull/4822/files

[luisramos0]

I am giving it a go by removing protected_attributes, attr_accessible and without_protection (see last 3 commits):
https://github.com/openfoodfoundation/openfoodnetwork/pull/4827/files

[luisramos0]

We have 31 models with attr_accessible and 9 usages of without_protection. We should probably create an epic and one issue for each model, right?

[luisramos0]

ok, so we started strong_params work with #4828 and #4827

We need to list the errors and controllers we need to work on and create issues for them.

4828 covers:

• spree/admin/orders_controllers
• user_registrations_controller

4827 covers:

• admin/customers_controller
• admin/inventory_items_controller
• admin/subscriptions_controller
• spree/admin/orders/customer_details_controller
• spree/admin/payment_methods_controller
• spree/admin/payments_controller
• spree/admin/resource_controller
• spree/admin/taxons_controller
• spree/admin/users_controller
• spree/orders_controller

The remaining errors in the build are coming from:

• [x] admin/enterprise_relationships_controller #4832
• [x] admin/enterprises_controller #4827
• [x] order_cycle_form #4827
• [x] spree/product_set (spree/admin/products_controller) #4827
• [x] admin/bulk_line_items_controller #4827
• [x] spree/users_controller #4827
• [x] admin/enterprise_roles_controller.rb (2 specs)
• [x] model_set (admin/variant_overrides_controller) #4827
• [x] model set (admin/column_preferences_controller) #4833
• [x] spree/credit_cards_controller #4827
• [x] admin/subscription_line_items_controller #4827

[Matt-Yorkley]

I think a lot of these will be really simple and quick, except Admin::EnterprisesController, which will need a big refactor. There's a bunch of messy logic in the controller at the moment for stripping out specific user-submitted params based on additional cancan permissions checks. It'll need a careful clean-up but it should look a lot nicer when we're finished...

[luisramos0]

cool!
meanwhile I have added admin/enterprises_controller and spree/admin/products_controller to #4827
On enterprises_controller, I tried a quick fix and it's looking good.
I see another problem there related to #4821 and saving user_ids

[Matt-Yorkley]

The following callbacks in admin/enterprise_controller.rb all mangle the params object in a disorganised way after performing additional permissions checks:

before_filter :check_can_change_sells, only: :update
before_filter :check_can_change_bulk_sells, only: :bulk_update
before_filter :check_can_change_owner, only: :update
before_filter :check_can_change_bulk_owner, only: :bulk_update
before_filter :check_can_change_managers, only: :update
before_filter :strip_new_properties, only: [:create, :update]

Here's an example:

def check_can_change_managers
  unless ( spree_current_user == @enterprise.owner ) || spree_current_user.admin?
    params[:enterprise].delete :user_ids
  end
end

It would be great to refactor these callbacks whilst we're switching to strong_params, no?

I'm wondering if we can do something like this (in a single method):

def enterprise_params
  permitted_params = [:list, :of, :uncontroversial, :params]
  permitted_params << :controversial_param if spree_current_user.can? :admin, @enterprise
  permitted_params << :controversial_param_2 if spree_current_user == @enterprise.owner

  params.permit(permitted_params)
end

And then :fire: all 6 of those callbacks...?

I guess this isn't a priority if https://github.com/openfoodfoundation/openfoodnetwork/pull/4827 is working...

[luisramos0]

yeah, that would be great. I ended up going for the quick path in enterprises and it looks like its working, it's in #4827

So, this issue is finished with #4827, #4828, #4832 and #4833
No ForbiddenAttribute errors in the build now.

We may find other specs broken because of missing permitted attributes but we can add them as we go fixing the upgrade build.

[Matt-Yorkley]

I had a nagging thought at the back of my mind: what if we have an endpoint somewhere where in the test suite we only test some but not all of it's possible combinations of submitted params? :grimacing:

[luisramos0]

yes, that will probably happen, we can only go ahead and test it to find out :-)
We probably need to be thorough on the manual tests with do on the forms, we basically need to test every field in every form of the app and make sure we can change it...

[luisramos0]

This issue will be closed when the 8 PRs currently in code review are merged.

[Matt-Yorkley]

Closing as all the related PRs have now been merged :tada: