Merge pull request #4828 from Matt-Yorkley/3-0-strong_params

[Spree 2.1] Strong params

Gemfile

@@ -81,7 +81,6 @@ gem 'gmaps4rails'
 gem 'oj'
 gem 'paper_trail', '~> 5.2.3'
 gem 'paperclip', '~> 3.4.1'
-gem 'protected_attributes'
 gem 'rack-rewrite'
 gem 'rack-ssl', require: 'rack/ssl'
 gem 'roadie-rails', '~> 1.3.0'

Gemfile.lock

@@ -483,8 +483,6 @@ GEM
       activerecord (>= 3.0)
     polyglot (0.3.5)
     power_assert (1.1.5)
-    protected_attributes (1.1.4)
-      activemodel (>= 4.0.1, < 5.0)
     pry (0.12.2)
       coderay (~> 1.1.0)
       method_source (~> 0.9.0)
@@ -732,7 +730,6 @@ DEPENDENCIES
   paper_trail (~> 5.2.3)
   paperclip (~> 3.4.1)
   pg (~> 0.21.0)
-  protected_attributes
   pry-byebug (>= 3.4.3)
   rabl
   rack-mini-profiler (< 2.0.0)

app/controllers/api/base_controller.rb

@@ -32,6 +32,12 @@ module Api
     use_renderers :json
     check_authorization
 
+    # Temporary measure to help debugging strong_parameters
+    rescue_from ActiveModel::ForbiddenAttributesError, with: :print_params
+    def print_params
+      raise ActiveModel::ForbiddenAttributesError, params.to_s
+    end
+
     def set_jsonp_format
       return unless params[:callback] && request.get?
 

app/controllers/application_controller.rb

@@ -10,6 +10,12 @@ class ApplicationController < ActionController::Base
   include EnterprisesHelper
   include Spree::AuthenticationHelpers
 
+  # Temporary measure to help debugging strong_parameters
+  rescue_from ActiveModel::ForbiddenAttributesError, with: :print_params
+  def print_params
+    raise ActiveModel::ForbiddenAttributesError, params.to_s
+  end
+
   def redirect_to(options = {}, response_status = {})
     ::Rails.logger.error("Redirected by #{begin
                                             caller(1).first

app/controllers/spree/admin/orders_controller.rb

@@ -44,7 +44,7 @@ module Spree
       end
 
       def update
-        unless @order.update_attributes(params[:order]) && @order.line_items.present?
+        unless @order.update_attributes(order_params) && @order.line_items.present?
           if @order.line_items.empty?
             @order.errors.add(:line_items, Spree.t('errors.messages.blank'))
           end
@@ -108,6 +108,10 @@ module Spree
 
       private
 
+      def order_params
+        params.require(:order).permit(:distributor_id, :order_cycle_id)
+      end
+
       def load_order
         @order = Order.find_by_number!(params[:id], include: :adjustments) if params[:id]
         authorize! action, @order

app/controllers/user_registrations_controller.rb

@@ -10,7 +10,7 @@ class UserRegistrationsController < Spree::UserRegistrationsController
 
   # POST /resource/sign_up
   def create
-    @user = build_resource(params[:spree_user])
+    @user = build_resource(spree_user_params)
     @user.locale = I18n.locale.to_s
     unless resource.save
       return render_error(@user.errors)
@@ -32,6 +32,11 @@ class UserRegistrationsController < Spree::UserRegistrationsController
 
   private
 
+  def spree_user_params
+    params.require(:spree_user).
+      permit(:email, :password, :password_confirmation, :remember_me)
+  end
+
   def render_error(errors = {})
     clean_up_passwords(resource)
     respond_to do |format|

app/models/calculator/flat_percent_per_item.rb

@@ -11,8 +11,6 @@ class Calculator::FlatPercentPerItem < Spree::Calculator
 
   preference :flat_percent, :decimal, default: 0
 
-  attr_accessible :preferred_flat_percent
-
   localize_number :preferred_flat_percent
 
   def self.description

app/models/calculator/weight.rb

@@ -4,7 +4,6 @@ module Calculator
   class Weight < Spree::Calculator
     extend Spree::LocalizedNumber
     preference :per_kg, :decimal, default: 0.0
-    attr_accessible :preferred_per_kg
     localize_number :preferred_per_kg
 
     def self.description

app/models/column_preference.rb

@@ -3,12 +3,6 @@ require 'open_food_network/column_preference_defaults'
 class ColumnPreference < ActiveRecord::Base
   extend OpenFoodNetwork::ColumnPreferenceDefaults
 
-  # These are the attributes used to identify a preference
-  attr_accessible :user_id, :action_name, :column_name
-
-  # These are attributes that need to be mass assignable
-  attr_accessible :name, :visible
-
   # Non-persisted attributes that only have one
   # setting (ie. the default) for a given column
   attr_accessor :name

app/models/concerns/variant_stock.rb

@@ -16,7 +16,6 @@ module VariantStock
   extend ActiveSupport::Concern
 
   included do
-    attr_accessible :on_hand, :on_demand
     after_update :save_stock
   end
 

app/models/enterprise_fee.rb

@@ -10,8 +10,6 @@ class EnterpriseFee < ActiveRecord::Base
   has_many :exchange_fees, dependent: :destroy
   has_many :exchanges, through: :exchange_fees
 
-  attr_accessible :enterprise_id, :fee_type, :name, :tax_category_id, :calculator_type, :inherits_tax_category
-
   FEE_TYPES = %w(packing transport admin sales fundraising).freeze
   PER_ORDER_CALCULATORS = ['Spree::Calculator::FlatRate', 'Spree::Calculator::FlexiRate', 'Spree::Calculator::PriceSack'].freeze
 

app/models/enterprise_group.rb

@@ -21,13 +21,6 @@ class EnterpriseGroup < ActiveRecord::Base
   before_validation :sanitize_permalink
   validates :permalink, uniqueness: true, presence: true
 
-  attr_accessible :name, :description, :long_description, :on_front_page, :enterprise_ids
-  attr_accessible :owner_id
-  attr_accessible :permalink
-  attr_accessible :logo, :promo_image
-  attr_accessible :address_attributes
-  attr_accessible :email, :website, :facebook, :instagram, :linkedin, :twitter
-
   delegate :phone, :address1, :address2, :city, :zipcode, :state, :country, to: :address
 
   has_attached_file :logo,

app/models/inventory_item.rb

@@ -1,6 +1,4 @@
 class InventoryItem < ActiveRecord::Base
-  attr_accessible :enterprise, :enterprise_id, :variant, :variant_id, :visible
-
   belongs_to :enterprise
   belongs_to :variant, class_name: "Spree::Variant"
 

app/models/schedule.rb

@@ -2,8 +2,6 @@ class Schedule < ActiveRecord::Base
   has_and_belongs_to_many :order_cycles, join_table: 'order_cycle_schedules'
   has_many :coordinators, -> { uniq }, through: :order_cycles
 
-  attr_accessible :name, :order_cycle_ids
-
   validates :order_cycles, presence: true
 
   scope :with_coordinator, lambda { |enterprise| joins(:order_cycles).where('coordinator_id = ?', enterprise.id).select('DISTINCT schedules.*') }

app/models/spree/adjustment_decorator.rb

@@ -25,8 +25,6 @@ module Spree
     scope :shipping,       -> { where(AdjustmentScopes::SHIPPING_SCOPE) }
     scope :eligible,       -> { where(AdjustmentScopes::ELIGIBLE_SCOPE) }
 
-    attr_accessible :included_tax
-
     localize_number :amount
 
     def set_included_tax!(rate)

app/models/spree/concerns/payment_method_distributors.rb

@@ -8,7 +8,6 @@ module Spree::PaymentMethodDistributors
 
   def self.included(base)
     base.class_eval do
-      attr_accessible :distributor_ids
       has_and_belongs_to_many :distributors, join_table: 'distributors_payment_methods', class_name: 'Enterprise', foreign_key: 'payment_method_id', association_foreign_key: 'distributor_id'
     end
   end

app/models/spree/credit_card_decorator.rb

@@ -1,11 +1,5 @@
 Spree::CreditCard.class_eval do
-  # Allows user to submit these attributes with checkout request
-  # Required to be able to correctly store details for token-based charges
-  # Obviously can be removed once we are using strong params
-  attr_accessible :cc_type, :last_digits
-
   # For holding customer preference in memory
-  attr_accessible :save_requested_by_customer, :is_default
   attr_writer :save_requested_by_customer
 
   # Should be able to remove once we reach Spree v2.2.0

app/models/spree/gateway/migs.rb

@@ -3,8 +3,6 @@ module Spree
     preference :login, :string
     preference :password, :string
 
-    attr_accessible :preferred_login, :preferred_password
-
     def provider_class
       ActiveMerchant::Billing::MigsGateway
     end

app/models/spree/gateway/pin.rb

@@ -2,8 +2,6 @@ module Spree
   class Gateway::Pin < Gateway
     preference :api_key, :string
 
-    attr_accessible :preferred_api_key
-
     def provider_class
       ActiveMerchant::Billing::PinGateway
     end

app/models/spree/gateway/stripe_connect.rb

@@ -7,8 +7,6 @@ module Spree
 
       validate :ensure_enterprise_selected
 
-      attr_accessible :preferred_enterprise_id
-
       CARD_TYPE_MAPPING = {
         'American Express' => 'american_express',
         'Diners Club' => 'diners_club',

app/models/spree/line_item_decorator.rb

@@ -12,10 +12,6 @@ Spree::LineItem.class_eval do
   # Allows manual skipping of Stock::AvailabilityValidator
   attr_accessor :skip_stock_check
 
-  attr_accessible :max_quantity, :final_weight_volume, :price
-  attr_accessible :final_weight_volume, :price, as: :api
-  attr_accessible :skip_stock_check
-
   before_save :calculate_final_weight_volume, if: :quantity_changed?, unless: :final_weight_volume_changed?
   after_save :update_units
 

app/models/spree/order_decorator.rb

@@ -28,7 +28,6 @@ Spree::Order.class_eval do
   validates :customer, presence: true, if: :require_customer?
   validate :products_available_from_new_distribution, if: lambda { distributor_id_changed? || order_cycle_id_changed? }
   validate :disallow_guest_order
-  attr_accessible :order_cycle_id, :distributor_id, :customer_id
 
   # Removes Spree 2.1 additional email validation (currently failing every time)
   # See: spree/core/validators/email.rb

app/models/spree/payment_decorator.rb

@@ -10,8 +10,6 @@ module Spree
 
     after_save :ensure_correct_adjustment, :update_order
 
-    attr_accessible :source
-
     localize_number :amount
 
     def ensure_correct_adjustment
@@ -64,7 +62,7 @@ module Spree
                               payment_method: payment_method,
                               amount: refund_amount.abs * -1,
                               response_code: response.authorization,
-                              state: 'completed' }, without_protection: true)
+                              state: 'completed' })
         else
           gateway_error(response)
         end

app/models/spree/payment_method_decorator.rb

@@ -8,8 +8,6 @@ Spree::PaymentMethod.class_eval do
 
   has_many :credit_cards, class_name: "Spree::CreditCard" # from Spree v.2.3.0 d470b31798f37
 
-  attr_accessible :tag_list
-
   after_initialize :init
 
   validates_with DistributorsValidator

app/models/spree/product_decorator.rb

@@ -17,11 +17,6 @@ Spree::Product.class_eval do
   delegate_belongs_to :master, :unit_value, :unit_description
   delegate :images_attributes=, :display_as=, to: :master
 
-  attr_accessible :supplier_id, :primary_taxon_id, :distributor_ids
-  attr_accessible :group_buy, :group_buy_unit_size, :unit_description, :notes, :images_attributes, :display_as
-  attr_accessible :variant_unit, :variant_unit_scale, :variant_unit_name, :unit_value
-  attr_accessible :inherits_properties, :sku
-
   validates :supplier, presence: true
   validates :primary_taxon, presence: true
   validates :tax_category_id, presence: true, if: "Spree::Config.products_require_tax_category"

app/models/spree/property.rb

@@ -3,8 +3,6 @@ module Spree
     has_many :product_properties, dependent: :destroy
     has_many :products, through: :product_properties
 
-    attr_accessible :name, :presentation
-
     validates :name, :presentation, presence: true
 
     scope :sorted, -> { order(:name) }

app/models/spree/shipping_category_decorator.rb

@@ -1,3 +0,0 @@
-Spree::ShippingCategory.class_eval do
-  attr_accessible :temperature_controlled
-end

app/models/spree/shipping_method_decorator.rb

@@ -5,8 +5,6 @@ Spree::ShippingMethod.class_eval do
   has_many :distributors, through: :distributor_shipping_methods, class_name: 'Enterprise', foreign_key: 'distributor_id'
 
   after_save :touch_distributors
-  attr_accessible :distributor_ids, :description
-  attr_accessible :require_ship_address, :tag_list
 
   validates_with DistributorsValidator
 

app/models/spree/user.rb

@@ -10,10 +10,6 @@ module Spree
     before_validation :set_login
     before_destroy :check_completed_orders
 
-    # Setup accessible (or protected) attributes for your model
-    attr_accessible :email, :password, :password_confirmation,
-                    :remember_me, :persistence_token, :login
-
     users_table_name = User.table_name
     roles_table_name = Role.table_name
 
@@ -34,8 +30,6 @@ module Spree
     accepts_nested_attributes_for :bill_address
     accepts_nested_attributes_for :ship_address
 
-    attr_accessible :enterprise_ids, :enterprise_roles_attributes, :enterprise_limit,
-                    :locale, :bill_address_attributes, :ship_address_attributes
     after_create :associate_customers
 
     validate :limit_owned_enterprises

app/models/spree/variant_decorator.rb

@@ -16,7 +16,6 @@ Spree::Variant.class_eval do
   has_many :variant_overrides
   has_many :inventory_items
 
-  attr_accessible :unit_value, :unit_description, :images_attributes, :display_as, :display_name, :import_date
   accepts_nested_attributes_for :images
 
   validates :unit_value, presence: true, if: ->(variant) {

app/models/tag_rule.rb

@@ -5,9 +5,6 @@ class TagRule < ActiveRecord::Base
 
   validates :enterprise, presence: true
 
-  attr_accessible :enterprise, :enterprise_id, :is_default, :priority
-  attr_accessible :preferred_customer_tags
-
   scope :for, ->(enterprise) { where(enterprise_id: enterprise) }
   scope :prioritised, -> { order('priority ASC') }
 

app/models/tag_rule/filter_order_cycles.rb

@@ -2,8 +2,6 @@ class TagRule::FilterOrderCycles < TagRule
   preference :matched_order_cycles_visibility, :string, default: "visible"
   preference :exchange_tags, :string, default: ""
 
-  attr_accessible :preferred_matched_order_cycles_visibility, :preferred_exchange_tags
-
   def tags_match?(order_cycle)
     exchange_tags = exchange_for(order_cycle).andand.tag_list || []
     preferred_tags = preferred_exchange_tags.split(",")

app/models/tag_rule/filter_payment_methods.rb

@@ -2,8 +2,6 @@ class TagRule::FilterPaymentMethods < TagRule
   preference :matched_payment_methods_visibility, :string, default: "visible"
   preference :payment_method_tags, :string, default: ""
 
-  attr_accessible :preferred_matched_payment_methods_visibility, :preferred_payment_method_tags
-
   def tags_match?(payment_method)
     payment_method_tags = payment_method.andand.tag_list || []
     preferred_tags = preferred_payment_method_tags.split(",")

app/models/tag_rule/filter_products.rb

@@ -3,8 +3,6 @@ class TagRule
     preference :matched_variants_visibility, :string, default: "visible"
     preference :variant_tags, :string, default: ""
 
-    attr_accessible :preferred_matched_variants_visibility, :preferred_variant_tags
-
     def self.tagged_children_for(product)
       product["variants"]
     end

app/models/tag_rule/filter_shipping_methods.rb

@@ -2,8 +2,6 @@ class TagRule::FilterShippingMethods < TagRule
   preference :matched_shipping_methods_visibility, :string, default: "visible"
   preference :shipping_method_tags, :string, default: ""
 
-  attr_accessible :preferred_matched_shipping_methods_visibility, :preferred_shipping_method_tags
-
   def reject_matched?
     preferred_matched_shipping_methods_visibility != "visible"
   end

config/initializers/spree.rb

@@ -15,7 +15,6 @@ require 'spree/product_filters'
 # https://github.com/openfoodfoundation/openfoodnetwork/issues/3121
 Spree::Gateway.class_eval do
   acts_as_taggable
-  attr_accessible :tag_list
 end
 
 require "#{Rails.root}/app/models/spree/payment_method_decorator"

db/migrate/20130814010857_remove_shipping_methods_using_itemwise_calculator.rb

@@ -10,6 +10,6 @@ class RemoveShippingMethodsUsingItemwiseCalculator < ActiveRecord::Migration
   end
 
   def down
-    Spree::ShippingMethod.create!({name: 'Delivery', zone: Spree::Zone.last, calculator: OpenFoodNetwork::Calculator::Itemwise.new}, without_protection: true)
+    Spree::ShippingMethod.create!({name: 'Delivery', zone: Spree::Zone.last, calculator: OpenFoodNetwork::Calculator::Itemwise.new})
   end
 end

db/seeds.rb

@@ -41,10 +41,7 @@ states.each do |state|
   puts "State: " + state.to_s
 
   unless Spree::State.find_by(name: state['name'])
-    Spree::State.create!(
-      { name: state['name'], abbr: state['abbr'], country: country },
-      without_protection: true
-    )
+    Spree::State.create!({ name: state['name'], abbr: state['abbr'], country: country })
   end
 end
 

lib/open_food_network/variant_and_line_item_naming.rb

@@ -59,7 +59,7 @@ module OpenFoodNetwork
       option_type = product.variant_unit_option_type
       if option_type
         name = option_value_name
-        ov = Spree::OptionValue.where(option_type_id: option_type, name: name, presentation: name).first || Spree::OptionValue.create!({ option_type: option_type, name: name, presentation: name }, without_protection: true)
+        ov = Spree::OptionValue.where(option_type_id: option_type, name: name, presentation: name).first || Spree::OptionValue.create!({ option_type: option_type, name: name, presentation: name })
         option_values << ov
       end
     end

spec/controllers/spree/admin/payment_methods_controller_spec.rb

@@ -2,7 +2,6 @@ require 'spec_helper'
 
 module Spree
   class GatewayWithPassword < PaymentMethod
-    attr_accessible :preferred_password
     preference :password, :string, default: "password"
   end
 

spec/features/consumer/registration_spec.rb

@@ -10,9 +10,9 @@ feature "Registration", js: true do
     before do
       Spree::Config.enterprises_require_tos = false
 
-      albania = Spree::Country.create!({ name: "Albania", iso3: "ALB", iso: "AL", iso_name: "ALBANIA", numcode: "8" }, without_protection: true)
-      Spree::State.create!({ name: "Berat", abbr: "BRA", country: albania }, without_protection: true)
-      Spree::Country.create!({ name: "Chad", iso3: "TCD", iso: "TD", iso_name: "CHAD", numcode: "148" }, without_protection: true)
+      albania = Spree::Country.create!({ name: "Albania", iso3: "ALB", iso: "AL", iso_name: "ALBANIA", numcode: "8" })
+      Spree::State.create!({ name: "Berat", abbr: "BRA", country: albania })
+      Spree::Country.create!({ name: "Chad", iso3: "TCD", iso: "TD", iso_name: "CHAD", numcode: "148" })
     end
 
     after do

spec/models/enterprise_fee_spec.rb

@@ -130,7 +130,7 @@ describe EnterpriseFee do
                                  source: order,
                                  originator: tax_rate,
                                  state: 'closed',
-                                 label: 'hello' }, without_protection: true)
+                                 label: 'hello' })
 
       expect do
         EnterpriseFee.clear_all_adjustments_on_order order

spec/models/spree/product_spec.rb

@@ -450,9 +450,9 @@ module Spree
         pb = Spree::Property.create! name: 'B', presentation: 'B'
         pc = Spree::Property.create! name: 'C', presentation: 'C'
 
-        product.product_properties.create!({ property_id: pa.id, value: '1', position: 1 }, without_protection: true)
-        product.product_properties.create!({ property_id: pc.id, value: '3', position: 3 }, without_protection: true)
-        supplier.producer_properties.create!({ property_id: pb.id, value: '2', position: 2 }, without_protection: true)
+        product.product_properties.create!({ property_id: pa.id, value: '1', position: 1 })
+        product.product_properties.create!({ property_id: pc.id, value: '3', position: 3 })
+        supplier.producer_properties.create!({ property_id: pb.id, value: '2', position: 2 })
 
         expect(product.properties_including_inherited).to eq(
           [{ id: pa.id, name: "A", value: '1' },

spec/models/tag_rule/discount_order_spec.rb

@@ -31,7 +31,7 @@ describe TagRule::DiscountOrder, type: :model do
 
   pending "determining whether a the rule has already been applied to an order" do
     let!(:order) { create(:order) }
-    let!(:adjustment) { order.adjustments.create({ amount: 12.34, source: order, originator: tag_rule, label: 'discount' }, without_protection: true) }
+    let!(:adjustment) { order.adjustments.create({ amount: 12.34, source: order, originator: tag_rule, label: 'discount' }) }
 
     before do
       tag_rule.context = { subject: order }

spec/support/seeds.rb

@@ -6,10 +6,10 @@
 # You can add more entries here if you need them for your tests.
 
 if Spree::Country.where(nil).empty?
-  Spree::Country.create!({ "name" => "Australia", "iso3" => "AUS", "iso" => "AU", "iso_name" => "AUSTRALIA", "numcode" => "36" }, without_protection: true)
+  Spree::Country.create!({ "name" => "Australia", "iso3" => "AUS", "iso" => "AU", "iso_name" => "AUSTRALIA", "numcode" => "36" })
   country = Spree::Country.find_by(name: 'Australia')
-  Spree::State.create!({ "name" => "Victoria", "abbr" => "Vic", :country => country }, without_protection: true)
-  Spree::State.create!({ "name" => "New South Wales", "abbr" => "NSW", :country => country }, without_protection: true)
+  Spree::State.create!({ "name" => "Victoria", "abbr" => "Vic", :country => country })
+  Spree::State.create!({ "name" => "New South Wales", "abbr" => "NSW", :country => country })
 end
 
 # Since the country seeding differs from other environments, the default