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

.reek.yml

@@ -42,3 +42,7 @@ directories:
   "test":
     InstanceVariableAssumption:
       enabled: false
+    DuplicateMethodCall:
+      enabled: false
+    UncommunicativeVariableName:
+      enabled: false

.rubocop.yml

@@ -8,6 +8,13 @@ Rails:
 Rails/RequestReferer:
   EnforcedStyle: referrer
 
+Rails/ActionOrder:
+  Enabled: false
+
+Rails/SkipsModelValidations:
+  Exclude:
+  - 'test/**/*_test.rb'
+
 Lint:
   Enabled: true
 
@@ -20,6 +27,7 @@ Metrics:
 Metrics/BlockLength:
   Exclude:
   - 'test/**/*_test.rb'
+  - 'spec/**/*_spec.rb'
 
 Lint/AssignmentInCondition:
   AllowSafeAssignment: true
@@ -75,12 +83,16 @@ Layout/LineLength:
 
 Metrics/AbcSize:
   Max: 20
+  Exclude:
+  - 'test/**/*_test.rb'
 
 Metrics/MethodLength:
   Max: 20
 
 Metrics/ClassLength:
   Max: 200
+  Exclude:
+  - 'test/**/*_test.rb'
 
 AllCops:
   NewCops: enable

app/controllers/admin/api/accounts_controller.rb

@@ -43,9 +43,8 @@ def update
 
     preload_to_present(buyer_account)
 
-    buyer_account.vat_rate = params[:vat_rate].to_f if params[:vat_rate]
     buyer_account.settings.attributes = billing_params
-    buyer_account.update_with_flattened_attributes(flat_params)
+    buyer_account.update(account_params)
 
     respond_with(buyer_account)
   end
@@ -155,8 +154,4 @@ def find_buyer_account
       raise ActiveRecord::RecordNotFound
     end
   end
-
-  def flat_params
-    super.except(:vat_rate, :id)
-  end
 end

app/controllers/admin/api/base_controller.rb

@@ -94,4 +94,21 @@ def authorize_account_plans!
   def authorize_service_plans!
     authorize!(:admin, :service_plans) if current_user
   end
+
+  def account_params
+    defined_fields_names = current_account.defined_fields_names_for(Account)
+    allowed_attrs = defined_fields_names + %w[name]
+    nested_params = {
+      extra_fields: current_account.defined_extra_fields_names_for(Account),
+      annotations: {}
+    }
+
+    if defined_fields_names.include?('billing_address')
+      allowed_attrs += %w[billing_address_name billing_address_address1 billing_address_address2 billing_address_city
+                          billing_address_country billing_address_state billing_address_zip billing_address_phone]
+      nested_params[:billing_address] = %w[name address1 address2 city country state zip phone]
+    end
+
+    params.permit(*allowed_attrs, **nested_params)
+  end
 end

app/controllers/admin/api/buyers_application_plans_controller.rb

@@ -2,54 +2,17 @@ class Admin::Api::BuyersApplicationPlansController < Admin::Api::BuyersBaseContr
   representer ApplicationPlan
 
   # FIXME: QUESTION: these two should be deprecated or removed directly. Check if they are used, not document it on the active docs.
-  #
-  # swagger
-  ## sapi = source2swagger.namespace("Account Management API")
-  ## e = sapi.apis.add
-  ## e.path = "/admin/api/accounts/{account_id}/application_plans.xml"
-  ## e.responseClass = "List[application_plan]"
-  ## @desc = "Returns the application plans bought by a partner."
-  ## e.description   = @desc
-  #
-  ## op = e.operations.add
-  ## op.nickname   = "buyer_app_plans"
-  ## op.httpMethod = "GET"
-  #
-  ## @access_token = { :name => "access_token", :description => "A personal Access Token", :dataType => "string", :required => true, :paramType => "query" }
-  ## @account_id = { :name => "account_id", :description => "ID of the partner account.", :dataType => "int", :allowMultiple => false, :required => true, :paramType => "path" }
-  #
-  ## op.parameters.add @access_token
-  ## op.parameters.add @account_id
-  #
+  # Returns the application plans bought by a partner.
+  # GET /admin/api/accounts/{account_id}/application_plans.xml
   def index
     respond_with(bought_application_plans)
   end
 
-  # swagger
-  ## e = sapi.apis.add
-  ## e.path = "/admin/api/accounts/{account_id}/application_plans/{id}/buy.xml"
-  ## e.responseClass = "application"
-  ## @desc = "Creates an application for a partner on a given application plan (deprecated)."
-  ## e.description   = @desc
-  #
-  ## op = e.operations.add
-  ## op.deprecated = true
-  ## op.nickname   = "buyer_app_plan_buy"
-  ## op.httpMethod = "POST"
-  ## op.summary    = @desc
-  #
-  ## @id = { :name => "id", :description => "ID of the application plan.", :dataType => "int", :allowMultiple => false, :required => true, :paramType => "path" }
-  #
-  ## op.parameters.add @access_token
-  ## op.parameters.add @account_id
-  ## op.parameters.add @id
-  ## op.parameters.add :name => "name", :description => "Name of the application to be created.", :dataType => "string", :allowMultiple => false, :required => true, :paramType => "query"
-  ## op.parameters.add :name => "description", :description => "Description of the application to be created.", :dataType => "string", :allowMultiple => false, :required => true, :paramType => "query"
-  #
-
+  # Creates an application for a partner on a given application plan (deprecated).
+  # POST /admin/api/accounts/{account_id}/application_plans/{id}/buy.xml
   #TODO: deprecate this, beware there are clients using it
   def buy
-    application = buyer.buy(application_plan, application_plan_params)
+    application = buyer.buy(application_plan, contract_params)
     respond_with(application, serialize: application_plan)
   end
 
@@ -63,8 +26,9 @@ def application_plan
     @application_plan ||= accessible_application_plans.stock.find(params[:id])
   end
 
-  def application_plan_params
-    attributes = current_account.fields.for(Cinstance)
-    params.slice(*attributes)
+  def contract_params
+    allowed_attrs = current_account.defined_fields_names_for(Cinstance) +
+                    %w[redirect_url first_traffic_at first_daily_traffic_at create_origin accepted_at]
+    params.permit(*allowed_attrs)
   end
 end

app/controllers/admin/api/buyers_applications_controller.rb

@@ -13,9 +13,7 @@ def index
   # POST /admin/api/accounts/{account_id}/applications.xml
   def create
     application = applications.new(user_account: buyer, plan: application_plan, create_origin: "api")
-    application.unflattened_attributes = application_params
-    application.user_key = params[:user_key] if params[:user_key]
-    application.application_id = params[:application_id] if params[:application_id]
+    application.assign_attributes(application_params)
 
     Array(params[:application_key]).each do |key|
       application.application_keys.build(value: key)
@@ -35,10 +33,7 @@ def show
   # Application Update
   # PUT /admin/api/accounts/{account_id}/applications/{id}.xml
   def update
-    application.unflattened_attributes = flat_params
-    application.user_key = params[:user_key] if params[:user_key]
-
-    application.save
+    application.update(application_update_params)
 
     respond_with application
   end
@@ -124,15 +119,13 @@ def application_plan
   end
 
   def application_params
-    flat_params.slice(*application_attributes)
-  end
-
-  def application_attributes
-    current_account.fields.for(Cinstance) + %w|user_key application_id|
+    allowed_attrs = current_account.defined_fields_names_for(Cinstance) +
+                    %w[user_key application_id redirect_url first_traffic_at first_daily_traffic_at accepted_at create_origin]
+    params.permit(*allowed_attrs)
   end
 
-  def flat_params
-    super.except(:account_id)
+  def application_update_params
+    application_params.except(:application_id)
   end
 
   def find_or_create_service_contract

app/controllers/admin/api/buyers_users_controller.rb

@@ -16,10 +16,7 @@ def create
 
     authorize! :create, user
 
-    user.unflattened_attributes = flat_params
-    user.signup_type = :created_by_provider
-
-    user.save
+    user.update(user_params.merge(signup_type: :created_by_provider))
 
     respond_with(user)
   end
@@ -37,7 +34,7 @@ def show
   def update
     authorize! :update, user
 
-    user.update_with_flattened_attributes(flat_params)
+    user.update(user_params)
 
     respond_with(user)
   end
@@ -123,4 +120,9 @@ def user
     @user ||= buyer.users.find(params[:id])
   end
 
+  private
+
+  def user_params
+    params.permit(*current_account.defined_fields_names_for(User), :password, :password_confirmation)
+  end
 end

app/controllers/admin/api/providers_controller.rb

@@ -13,7 +13,7 @@ def show
   # Provider Account Update
   # PUT /admin/api/provider.xml
   def update
-    current_account.update(provider_params, without_protection: true)
+    current_account.update(provider_params)
     respond_with current_account
   end
 

app/controllers/admin/api/signups_controller.rb

@@ -16,7 +16,8 @@ def create
   private
 
   def user_params
-    flat_params.merge({signup_type: :minimal})
+    defined_user_fields = current_account.defined_fields_names_for(User)
+    params.permit(*defined_user_fields, :password, :password_confirmation)
   end
 
   def check_creation_errors
@@ -31,7 +32,7 @@ def check_creation_errors
   end
 
   def signup_params
-    Signup::SignupParams.new(plans: plans, user_attributes: user_params, account_attributes: flat_params, defaults: defaults)
+    Signup::SignupParams.new(plans: plans, user_attributes: user_params.merge(signup_type: :minimal), account_attributes: account_params, defaults: defaults)
   end
 
   def defaults

app/controllers/admin/api/users_controller.rb

@@ -18,10 +18,7 @@ def create
 
     authorize! :create, user
 
-    user.unflattened_attributes = flat_params
-    user.signup_type = :created_by_provider
-
-    user.save
+    user.update(user_params.merge(signup_type: :created_by_provider))
 
     respond_with(user)
   end
@@ -39,7 +36,7 @@ def show
   def update
     authorize! :update, user
 
-    user.update_with_flattened_attributes(flat_params, as: current_user.try(:role))
+    user.update(user_params)
 
     respond_with(user)
   end
@@ -131,7 +128,13 @@ def can_create
 
   private
 
-  def flat_params
-    super.except(:id)
+  def user_params
+    defined_fields_names = current_account.provider_account.defined_fields_names_for(User)
+    # both `:member_permission_service_ids` and `member_permission_service_ids: []` need to be permitted, because the
+    # value can be either `nil` (meaning all services are allowed), or an array
+    permission_attrs = [:member_permission_service_ids, { member_permission_service_ids: [], member_permission_ids: [] }]
+    allowed_attrs = defined_fields_names + %w[password password_confirmation cas_identifier]
+    allowed_attrs += permission_attrs if provider_key.present? || current_user.admin?
+    params.permit(*allowed_attrs)
   end
 end

app/controllers/buyers/accounts_controller.rb

@@ -30,10 +30,7 @@ def new
   end
 
   def update
-    vat = account_params[:vat_rate]
-    account.vat_rate = vat if vat # vat_rate is protected attribute
-
-    if account.update(account_params.except(:vat_rate))
+    if account.update(account_params)
       redirect_to admin_buyers_account_path(account), success: t('.success')
     else
       render :edit
@@ -109,13 +106,18 @@ def signup_params
     Signup::SignupParams.new(plans: [], user_attributes: user_params.merge(signup_type: :created_by_provider), account_attributes: account_params, validate_fields: false)
   end
 
-  # TODO: using `permit` later
   def account_params
-    @account_params ||= params.require(:account).except(:user)
+    defined_builtin_fields_names = current_account.defined_builtin_fields_names_for(Account)
+    defined_extra_fields_names = current_account.defined_extra_fields_names_for(Account)
+    allowed_attrs = defined_builtin_fields_names - %w[billing_address country] + %w[country_id]
+    params.require(:account).permit(*allowed_attrs, extra_fields: defined_extra_fields_names)
   end
 
   def user_params
-    params.require(:account).fetch(:user, {})
+    defined_builtin_fields_names = current_account.defined_builtin_fields_names_for(User)
+    defined_extra_fields_names = current_account.defined_extra_fields_names_for(User)
+    allowed_attrs = defined_builtin_fields_names + %w[password]
+    params.require(:account).fetch(:user, {}).permit(*allowed_attrs, extra_fields: defined_extra_fields_names)
   end
 
   def set_plans

app/controllers/buyers/groups_controller.rb

@@ -10,7 +10,7 @@ class Buyers::GroupsController < Buyers::BaseController
   def show; end
 
   def update
-    flash[:success] = t('.success') if @account.update(params[:account])
+    flash[:success] = t('.success') if @account.update(groups_params)
 
     redirect_to action: :show, id: @account.id
   end
@@ -28,4 +28,8 @@ def authorize_groups
   def find_account
     @account = current_account.buyer_accounts.find(params[:account_id])
   end
+
+  def groups_params
+    params.require(:account).permit(group_ids: [])
+  end
 end

app/controllers/buyers/users_controller.rb

@@ -22,12 +22,7 @@ def show
   def edit; end
 
   def update
-    # TODO: I think this controller is used only on provider side
-    user.validate_fields! if current_account.buyer?
-
-    user.attributes = user_params
-    user.role = user_params.fetch(:role, user.role) if can?(:update_role, user)
-
+    user.assign_attributes(user_params)
     if user.save
       redirect_to({ action: :show }, success: t('.success'))
     else
@@ -86,10 +81,12 @@ def find_user
     @user = @account.users.find(params[:id]).decorate
   end
 
-  DEFAULT_PARAMS = %i[username email password password_confirmation role].freeze
-
   def user_params
-    @user_params ||= params.require(:user).permit(*DEFAULT_PARAMS, extra_fields: [*user.defined_extra_fields_names])
+    fields_names = current_account.defined_fields_names_for(User)
+    extra_fields_names = current_account.defined_extra_fields_names_for(User)
+    allowed_attrs = [*fields_names, :password, :password_confirmation]
+    allowed_attrs += [:role] if can?(:update_role, user)
+    params.require(:user).permit(allowed_attrs, extra_fields: extra_fields_names)
   end
 
   def redirect_back_or_show_detail(**opts)