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
@@ -134,7 +133,7 @@ def account_plan
   end
 
   def billing_params
-    @billing_params ||= params.permit(:monthly_billing_enabled, :monthly_charging_enabled)
+    params.permit(:monthly_billing_enabled, :monthly_charging_enabled)
   end
 
   private
@@ -156,7 +155,17 @@ def find_buyer_account
     end
   end
 
-  def flat_params
-    super.except(:vat_rate, :id)
+  def account_params
+    defined_fields_names = buyer_account.defined_fields_names
+    allowed_attrs = defined_fields_names + %w[name]
+    nested_params = { extra_fields: buyer_account.defined_extra_fields_names }
+
+    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

@@ -2,6 +2,10 @@ class Admin::Api::BuyersApplicationsController < Admin::Api::BuyersBaseControlle
   representer Cinstance
 
   before_action :find_or_create_service_contract, :only => :create
+  before_action :find_application, except: %i[create index find]
+  before_action :build_new_application, only: %i[create]
+
+  attr_reader :application
 
   # Application List
   # GET /admin/api/accounts/{account_id}/applications.xml
@@ -12,10 +16,7 @@ def index
   # Application Create
   # 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 +36,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
@@ -115,24 +113,27 @@ def applications
     @applications ||= accessible_bought_cinstances.includes(:user_account, :plan, :service)
   end
 
-  def application
-    @application ||= applications.find params[:id]
+  def build_new_application
+    @application = applications.new(user_account: buyer, plan: application_plan, create_origin: "api")
+  end
+
+  def find_application
+    @application = applications.find params[:id]
   end
 
   def application_plan
     @application_plan ||= accessible_application_plans.find(params[:plan_id])
   end
 
   def application_params
-    flat_params.slice(*application_attributes)
-  end
-
-  def application_attributes
-    current_account.fields.for(Cinstance) + %w|user_key application_id|
+    @application_params ||= begin
+      allowed_attrs = application.defined_fields_names + %w[user_key application_id redirect_url first_traffic_at first_daily_traffic_at]
+      params.permit(*allowed_attrs)
+    end
   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

@@ -1,6 +1,11 @@
 class Admin::Api::BuyersUsersController < Admin::Api::BuyersBaseController
   representer User
 
+  before_action :find_user, except: %i[create index]
+  before_action :build_new_user, only: %i[create]
+
+  attr_reader :user
+
   # User List
   # GET /admin/api/accounts/{account_id}/users.xml
   def index
@@ -12,14 +17,9 @@ def index
   # User Create
   # POST /admin/api/accounts/{account_id}/users.xml
   def create
-    user = new_user
-
     authorize! :create, user
 
-    user.unflattened_attributes = flat_params
-    user.signup_type = :api
-
-    user.save
+    user.update(user_params.merge(signup_type: :api))
 
     respond_with(user)
   end
@@ -37,7 +37,7 @@ def show
   def update
     authorize! :update, user
 
-    user.update_with_flattened_attributes(flat_params)
+    user.update(user_params)
 
     respond_with(user)
   end
@@ -108,19 +108,24 @@ def authorize!(*args)
     current_user ? super : logged_in?
   end
 
-  def new_user
-    @new_user ||= buyer.users.new
-  end
-
   def users
     @users ||= begin
       conditions = params.slice(:state, :role)
       buyer.users.where(conditions)
     end
   end
 
-  def user
-    @user ||= buyer.users.find(params[:id])
+  private
+
+  def build_new_user
+    @user = buyer.users.new
   end
 
+  def find_user
+    @user = buyer.users.find(params[:id])
+  end
+
+  def user_params
+    params.permit(*user.defined_fields_names, :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

@@ -7,16 +7,27 @@ class Admin::Api::SignupsController < Admin::Api::BaseController
   def create
     authorize!(:create, Account) if current_user
 
-    @signup_result = Signup::DeveloperAccountManager.new(current_account).create(signup_params)
+    @signup_result = account_manager.create(plans: plans, defaults: defaults,
+                                            account_params: account_params,
+                                            user_params: user_params.merge(signup_type: :minimal))
 
     check_creation_errors
     respond_with(@signup_result.account, user_options: { with_apps: true })
   end
 
   private
 
+  def account_manager
+    @account_manager ||= Signup::DeveloperAccountManager.new(current_account)
+  end
+
   def user_params
-    flat_params.merge({signup_type: :minimal})
+    params.permit(*account_manager.user.defined_fields_names, :password)
+  end
+
+  def account_params
+    allowed_attrs = account_manager.account.defined_fields_names - %w[billing_address]
+    params.permit(*allowed_attrs)
   end
 
   def check_creation_errors
@@ -30,10 +41,6 @@ def check_creation_errors
     end
   end
 
-  def signup_params
-    Signup::SignupParams.new(plans: plans, user_attributes: user_params, account_attributes: flat_params, defaults: defaults)
-  end
-
   def defaults
     { ApplicationPlan => { :name => 'API signup', :description => 'API signup', :create_origin => 'api' } }
   end

app/controllers/admin/api/users_controller.rb

@@ -2,6 +2,10 @@ class Admin::Api::UsersController < Admin::Api::BaseController
   representer User
 
   before_action :can_create, only: :create
+  before_action :build_new_user, only: %i[create]
+  before_action :find_user, except: %i[create index]
+
+  attr_reader :user
 
   # User List (provider account)
   # GET /admin/api/users.xml
@@ -14,14 +18,9 @@ def index
   # User Create (provider account)
   # POST /admin/api/users.xml
   def create
-    user = new_user
-
     authorize! :create, user
 
-    user.unflattened_attributes = flat_params
-    user.signup_type = :api
-
-    user.save
+    user.update(user_params.merge(signup_type: :api))
 
     respond_with(user)
   end
@@ -39,7 +38,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
@@ -110,19 +109,19 @@ def authorize!(*args)
     current_user ? super : logged_in?
   end
 
-  def new_user
-    @new_user ||= current_account.users.new
-  end
-
   def users
     @users ||= begin
       conditions = params.slice(:state, :role)
       current_account.users.but_impersonation_admin.where(conditions)
     end
   end
 
-  def user
-    @user ||= current_account.users.but_impersonation_admin.find(params[:id])
+  def build_new_user
+    @user = current_account.users.new
+  end
+
+  def find_user
+    @user = current_account.users.but_impersonation_admin.find(params[:id])
   end
 
   def can_create
@@ -131,7 +130,10 @@ def can_create
 
   private
 
-  def flat_params
-    super.except(:id)
+  def user_params
+    permission_attrs = [:member_permission_service_ids, { member_permission_service_ids: [], member_permission_ids: [] }]
+    allowed_attrs = user.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