THREESCALE-12434: Migrate from protected attributes to strong parameters - Part 1

.reek.yml

@@ -39,3 +39,6 @@ directories:
   "app/mailers":
     InstanceVariableAssumption:
       enabled: false
+  "test":
+    InstanceVariableAssumption:
+      enabled: false

.rubocop.yml

@@ -17,6 +17,10 @@ Performance:
 Metrics:
   Enabled: true
 
+Metrics/BlockLength:
+  Exclude:
+  - 'test/**/*_test.rb'
+
 Lint/AssignmentInCondition:
   AllowSafeAssignment: true
 

app/controllers/admin/api/account_features_controller.rb

@@ -41,7 +41,7 @@ def destroy
   protected
 
   def feature_params
-    params.fetch(:feature)
+    params.require(:feature).permit(:name, :system_name, :description)
   end
 
   def features

app/controllers/admin/api/api_docs_services_controller.rb

@@ -27,7 +27,7 @@ def index
   # ActiveDocs Spec Create
   # POST /admin/api/active_docs.json
   def create
-    @api_docs_service = current_account.api_docs_services.create(api_docs_params(:system_name), without_protection: true)
+    @api_docs_service = current_account.api_docs_services.create(api_docs_params(:system_name))
     respond_with(@api_docs_service)
   end
 
@@ -40,7 +40,7 @@ def show
   # ActiveDocs Spec Update
   # PUT /admin/api/active_docs/{id}.json
   def update
-    @api_docs_service.update(api_docs_params, without_protection: true)
+    @api_docs_service.update(api_docs_params)
     respond_with(@api_docs_service)
   end
 

app/controllers/admin/api/application_plan_metric_limits_controller.rb

@@ -61,7 +61,7 @@ def usage_limit
   end
 
   def usage_limit_params
-    params.fetch(:usage_limit)
+    params.fetch(:usage_limit).permit(:period, :value)
   end
 
 end

app/controllers/admin/api/registry/policies_controller.rb

@@ -54,6 +54,8 @@ def authorize_policies
   def policy_params
     policy_params = params.require(:policy)
     final_params = policy_params.permit(:name, :version)
+    # permit! is necessary here because the schema field accepts arbitrary nested JSON structures
+    # that cannot be whitelisted in advance.
     final_params.merge(schema: policy_params.require(:schema)).permit!
   end
 

app/controllers/admin/api/service_features_controller.rb

@@ -26,7 +26,7 @@ def show
   # Service Feature Update
   # PUT /admin/api/services/{service_id}/features/{id}.xml
   def update
-    feature.update(feature_params)
+    feature.update(feature_update_params)
 
     respond_with(feature)
   end
@@ -42,7 +42,11 @@ def destroy
   protected
 
   def feature_params
-    params.fetch(:feature)
+    params.require(:feature).permit(:name, :system_name, :description, :scope)
+  end
+
+  def feature_update_params
+    feature_params.except(:scope)
   end
 
   def features

app/controllers/admin/api/sso_tokens_controller.rb

@@ -1,14 +1,16 @@
+# frozen_string_literal: true
+
 class Admin::Api::SSOTokensController < Admin::Api::BaseController
 
-  wrap_parameters :sso_token, :include => [:user_id, :username, :expires_in, :redirect_url, :protocol], :format => [ :url_encoded_form ]
+  wrap_parameters :sso_token, include: %i[user_id username expires_in redirect_url protocol], format: [:url_encoded_form]
 
   # parameters:
   #   * user_id
   #   * expires_in
   #   * provider_key
   #   * protocol
   def create
-    sso_token = SSOToken.new **sso_token_params.to_h
+    sso_token = SSOToken.new(sso_token_params)
     sso_token.account = domain_account
     sso_token.save
 

app/controllers/admin/api/web_hooks_controller.rb

@@ -19,7 +19,7 @@ def webhook
   end
 
   def allowed_params
-    %w(url active provider_actions) + WebHook.switchable_attributes
+    %w[url active provider_actions] + WebHook.switchable_attributes
   end
 
   def webhook_params

app/controllers/admin/api_docs/base_controller.rb

@@ -18,7 +18,7 @@ def new
   end
 
   def create
-    @api_docs_service = api_docs_services.new(api_docs_params(:system_name), without_protection: true)
+    @api_docs_service = api_docs_services.new(api_docs_params(:system_name))
     if @api_docs_service.save
       redirect_to preview_admin_api_docs_service_path(@api_docs_service), success: t('admin.api_docs.create.success')
     else
@@ -72,7 +72,7 @@ def edit; end
 
   def update
     respond_to do |format|
-      if api_docs_service.update(api_docs_params, without_protection: true)
+      if api_docs_service.update(api_docs_params)
         msg = t('admin.api_docs.update.success')
         format.html { redirect_to preview_admin_api_docs_service_path(api_docs_service), success: msg }
         format.js do

app/controllers/admin/fields_definitions_controller.rb

@@ -4,34 +4,39 @@ class Admin::FieldsDefinitionsController < Sites::BaseController
   respond_to :html
   activate_menu :audience, :accounts, :fields_definitions
 
+  before_action :set_fields_definition_params, only: %i[new create update]
+  before_action :validate_target, only: [:new]
+
   def index
-    @possible_targets = FieldsDefinition.targets
+    @possible_targets = available_targets
 
-    respond_with(field_definitions)
+    respond_with(fields_definitions)
   end
 
   def new
-    @fields_definition = field_definitions.build(field_definition_params)
+    target = fields_definition_params[:target]
+
+    @fields_definition = fields_definitions.build(target: target)
+    target_class = @fields_definition.target_class
 
-    @optional_fields = @fields_definition.target_class.builtin_fields -
-      current_account.fields_definitions.by_target(target).map{ |f|f.name }
+    @optional_fields = target_class.builtin_fields - existing_fields_names_by_target(target)
 
-    @required_fields = @fields_definition.target_class.required_fields
+    @required_fields = target_class.required_fields
 
     @optional_fields.unshift "[new field]"
 
     respond_with(@fields_definition)
   end
 
   def edit
-    @optional_fields = field_definition.target_class.builtin_fields
-    @required_fields = field_definition.target_class.required_fields
+    @optional_fields = fields_definition.target_class.builtin_fields
+    @required_fields = fields_definition.target_class.required_fields
 
-    respond_with(field_definition)
+    respond_with(fields_definition)
   end
 
   def create
-    @fields_definition = field_definitions.build(field_definition_params)
+    @fields_definition = fields_definitions.build(fields_definition_params)
 
     if @fields_definition.save
       flash[:success] = t('.success')
@@ -48,22 +53,20 @@ def create
 
   def update
     @required_fields = []
-    if field_definition.update(field_definition_params)
-      @required_fields = field_definition.target_class.required_fields
-    end
+    @required_fields = fields_definition.target_class.required_fields if fields_definition.update(fields_definition_params)
 
-    respond_with(field_definition, location: admin_fields_definitions_path)
+    respond_with(fields_definition, location: admin_fields_definitions_path)
   end
 
   def destroy
-    field_definition.destroy
-    respond_with(field_definition, location: admin_fields_definitions_path)
+    fields_definition.destroy
+    respond_with(fields_definition, location: admin_fields_definitions_path)
   end
 
   def sort
-    fields = current_account.fields_definitions.find(field_definition_params).index_by(&:id)
+    fields = current_account.fields_definitions.find(sort_params).index_by(&:id)
 
-    field_definition_params.each_with_index do |field_id, index|
+    sort_params.each_with_index do |field_id, index|
       fields.fetch(field_id.to_i).update_attribute(:pos, index + 1)
     end
 
@@ -72,19 +75,38 @@ def sort
 
   private
 
-  def field_definition_params
-    params[:fields_definition] || {}
-  end
+  attr_reader :fields_definition_params
 
-  def target
-    field_definition_params[:target]
+  def sort_params
+    @sort_params ||= params.permit(fields_definition: [])[:fields_definition] || []
   end
 
-  def field_definitions
+  def fields_definitions
     @fields_definitions ||= current_account.fields_definitions
   end
 
-  def field_definition
-    @fields_definition ||= field_definitions.find(params[:id])
+  def fields_definition
+    @fields_definition ||= fields_definitions.find(params[:id])
+  end
+
+  def available_targets
+    @available_targets ||= FieldsDefinition.targets
+  end
+
+  def existing_fields_names_by_target(target)
+    current_account.fields_definitions.by_target(target).map(&:name)
+  end
+
+  def set_fields_definition_params
+    fd_params = params.fetch(:fields_definition)
+    if fd_params.respond_to? :permit
+      @fields_definition_params = fd_params.permit(:target, :name, :label, :required, :hidden, :read_only, :choices_for_views)
+    else
+      render_error "invalid fields definition", status: :bad_request unless fd_params.is_a?(Hash)
+    end
+  end
+
+  def validate_target
+    render_error "invalid fields definition target", status: :bad_request unless available_targets.include?(fields_definition_params[:target])
   end
 end

app/controllers/api/metric_visibilities_controller.rb

@@ -76,8 +76,4 @@ def find_metric
   def authorize_section
     authorize! :manage, :plans
   end
-
-  def usage_limit_params
-    params.require(:usage_limit)
-  end
 end

app/controllers/api/pricing_rules_controller.rb

@@ -76,6 +76,6 @@ def authorize_action
   end
 
   def pricing_rule_params
-    params.require(:pricing_rule)
+    params.require(:pricing_rule).permit(:min, :max, :cost_per_unit)
   end
 end

app/controllers/api/usage_limits_controller.rb

@@ -79,6 +79,6 @@ def authorize_action
   end
 
   def usage_limit_params
-    params.require(:usage_limit)
+    params.require(:usage_limit).permit(:period, :value)
   end
 end

app/controllers/buyers/invoices_controller.rb

@@ -40,7 +40,7 @@ def create
   def update
     @invoice = @account.invoices.find(params[:id])
 
-    if @invoice.update(params[:invoice])
+    if @invoice.update(invoice_params)
       redirect_to admin_buyers_account_invoice_url(@account, @invoice), success: t('.success')
     else
       render :edit
@@ -52,4 +52,8 @@ def update
   def find_account
     @account = current_account.buyers.find(params[:account_id])
   end
+
+  def invoice_params
+    params.require(:invoice).permit(:period)
+  end
 end

app/controllers/finance/api/invoices_controller.rb

@@ -51,7 +51,7 @@ def charge
   # Invoice Update
   # PUT /api/invoices/{id}.xml
   def update
-    invoice.update(invoice_params_update, without_protection: true)
+    invoice.update(invoice_params_update)
     respond_with(invoice)
   end
 

app/controllers/finance/provider/invoices_controller.rb

@@ -61,7 +61,7 @@ def create
   end
 
   def update
-    if @invoice.update(params[:invoice])
+    if @invoice.update(invoice_params)
       redirect_to admin_finance_invoice_url(@invoice), success: t('.success')
     else
       render :edit
@@ -96,4 +96,8 @@ def find_buyer(options = {})
   def find_invoice
     @invoice = collection.find(params[:id])
   end
+
+  def invoice_params
+    @invoice_params ||= params.require(:invoice).permit(:friendly_id, :period)
+  end
 end

app/controllers/provider/admin/account/payment_gateways/braintree_blue_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class Provider::Admin::Account::PaymentGateways::BraintreeBlueController < Provider::Admin::Account::BaseController
 
   after_action :check_multiple_payment_failures, only: [:hosted_success]
@@ -7,8 +9,7 @@ class Provider::Admin::Account::PaymentGateways::BraintreeBlueController < Provi
   prepend_before_action :deny_on_premises
   activate_menu :account, :billing, :payment_details
 
-  def show
-  end
+  def show; end
 
   def edit
     current_account.require_billing_information!
@@ -33,8 +34,7 @@ def update
   end
 
   def hosted_success
-    customer_info      = params.require(:customer).permit!.to_h
-    braintree_response = braintree_blue_crypt.confirm(customer_info, params.require(:braintree).require(:nonce))
+    braintree_response = braintree_blue_crypt.confirm(customer_params.to_h, params.require(:braintree).require(:nonce))
     @payment_result    = braintree_response&.success?
 
     if @payment_result
@@ -97,4 +97,9 @@ def hack_errors
 
     current_account.billing_address.errors.instance_variable_set('@errors', new_errors)
   end
+
+  def customer_params
+    billing_address_params = %i[company street_address postal_code locality region country_name]
+    params.require(:customer).permit(:first_name, :last_name, :phone, credit_card: { billing_address: billing_address_params })
+  end
 end