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
@@ -156,7 +155,20 @@ def find_buyer_account
     end
   end
 
-  def flat_params
-    super.except(:vat_rate, :id)
+  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] = %i[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

@@ -25,31 +25,11 @@ 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 +43,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[user_key application_id redirect_url first_traffic_at first_daily_traffic_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]
+    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,13 @@ 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)
+  end
+
+  def account_params
+    allowed_attrs = current_account.defined_fields_names_for(Account) - %w[billing_address]
+    params.permit(*allowed_attrs, annotations: {})
   end
 
   def check_creation_errors
@@ -31,7 +37,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,11 @@ 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)
+    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,10 +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.assign_attributes(permitted_user_params)
     user.role = user_params.fetch(:role, user.role) if can?(:update_role, user)
 
     if user.save
@@ -86,10 +83,15 @@ 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])
+    @user_params ||= params.require(:user)
+  end
+
+  def permitted_user_params
+    fields_names = current_account.defined_fields_names_for(User)
+    extra_fields_names = current_account.defined_extra_fields_names_for(User)
+    user_params.permit(*fields_names, :password, :password_confirmation,
+                       extra_fields: extra_fields_names)
   end
 
   def redirect_back_or_show_detail(**opts)

app/controllers/master/api/providers_controller.rb

@@ -48,9 +48,7 @@ def create
   # Tenant Update
   # PUT /master/api/providers/{id}.xml
   def update
-    provider_account.assign_attributes(update_params, without_protection: true)
-    provider_account.assign_unflattened_attributes(params.require(:account))
-    provider_account.save
+    provider_account.update(update_account_params)
 
     respond_with signup_result_with_nil_token
   end
@@ -117,14 +115,27 @@ def find_plan_for_upgrade
     render_error "Plan with ID #{plan_id.presence} not found", status: :not_found
   end
 
-  def update_params
-    permitted_params = provider_account.scheduled_for_deletion? ? %i[state_event] : UPDATE_PARAMS
-    params.require(:account).permit(permitted_params)
-  end
-
   def create_params
     defaults = { ApplicationPlan => { :name => 'API signup', :description => 'API signup', :create_origin => 'api' } }
-    Signup::SignupParams.new(plans: plans, user_attributes: flat_params.merge(signup_type: :created_by_provider), account_attributes: flat_params, defaults: defaults)
+    Signup::SignupParams.new(plans: plans, user_attributes: user_params.merge(signup_type: :created_by_provider), account_attributes: create_account_params, defaults: defaults)
+  end
+
+  def user_params
+    defined_fields_names = current_account.defined_fields_names_for(User)
+    params.permit(*defined_fields_names, :password)
+  end
+
+  def permitted_account_attrs
+    current_account.defined_fields_names_for(Account) | UPDATE_PARAMS
+  end
+
+  def create_account_params
+    params.permit(*permitted_account_attrs)
+  end
+
+  def update_account_params
+    allowed_attrs = provider_account.scheduled_for_deletion? ? %i[state_event] : permitted_account_attrs
+    params.require(:account).permit(*allowed_attrs)
   end
 
   def plans