Migrate application controllers to strong parameters

Author: mayorova

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,16 +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)
+    @application_params ||= begin
+      allowed_attrs = application.defined_fields_names + %w[user_key application_id redirect_url first_traffic_at first_daily_traffic_at]
+      flat_params.permit(*allowed_attrs)
+    end
+  end
+
+  def application_update_params
+    application_params.except(:application_id)
   end
 
   def application_attributes

app/controllers/api/applications_controller.rb

@@ -20,6 +20,7 @@ class Api::ApplicationsController < FrontendController
   def new; end
 
   def create
+    @cinstance.assign_attributes(application_params)
     if @cinstance.save
       redirect_to provider_admin_application_path(@cinstance), success: t('.success')
     else

app/controllers/buyers/applications_controller.rb

@@ -23,6 +23,8 @@ def new
   end
 
   def create
+    @cinstance.assign_attributes(application_params)
+
     if @cinstance.save
       redirect_to provider_admin_application_path(@cinstance), success: t('.success')
     else

app/controllers/provider/admin/applications_controller.rb

@@ -29,6 +29,8 @@ def show
   def new; end
 
   def create
+    @cinstance.assign_attributes(application_params)
+
     if @cinstance.save
       redirect_to provider_admin_application_path(@cinstance), success: t('.success')
     else
@@ -43,7 +45,7 @@ def edit; end
   def update
     # TODO: this is not needed if this controller is used only by providers
     @cinstance.validate_human_edition!
-    @cinstance.attributes = params[:cinstance]
+    @cinstance.assign_attributes(application_params)
 
     respond_to do |format|
       json = @cinstance.to_json(only: %i[id name], methods: %i[errors])

app/lib/applications_controller_methods.rb

@@ -23,7 +23,7 @@ def index
 
   # TODO: this should be done by buy! method
   def initialize_cinstance
-    @cinstance = current_account.provider_builds_application_for(@account, @application_plan, params[:cinstance], @service_plan)
+    @cinstance = current_account.provider_builds_application_for(@account, @application_plan, @service_plan)
     @cinstance.validate_human_edition!
   end
 
@@ -74,15 +74,24 @@ def find_application_plan
 
   def find_service_plan
     service_plans = @service.service_plans
-    @service_plan = if (service_plan_id = params[:cinstance].delete(:service_plan_id))
+    @service_plan = if (service_plan_id = cinstance_params[:service_plan_id])
                       service_plans.find(service_plan_id)
                     else
                       @service.default_service_plan || service_plans.first
                     end
   end
 
+  def cinstance_params
+    @cinstance_params ||= params.require(:cinstance)
+  end
+
+  def application_params
+    allowed_attrs = @cinstance.defined_builtin_fields_names
+    cinstance_params.permit(*allowed_attrs, extra_fields: @cinstance.defined_extra_fields_names)
+  end
+
   def plan_id
-    @plan_id ||= params.require(:cinstance).permit(:plan_id).tap { |plan_params| plan_params.require(:plan_id) }[:plan_id]
+    @plan_id ||= cinstance_params.require(:plan_id)
   end
 
   def accessible_services

app/lib/logic/contracting.rb

@@ -28,7 +28,7 @@ def can_create_service_contract?
     end
 
     module Provider
-      def provider_builds_application_for(buyer, application_plan, application_attrs = {}, service_plan = nil)
+      def provider_builds_application_for(buyer, application_plan, service_plan = nil, application_attrs: {})
         service_contracted = buyer.bought_service_contracts.map(&:service).include?(application_plan.service)
 
         service_contract = unless service_contracted

app/models/contract.rb

@@ -48,8 +48,6 @@ class Contract < ApplicationRecord
   # TODO: remove with Rails 3
   attr_reader :old_plan, :accepted_on_create
 
-  attr_protected :plan_id, :state, :provider_public_key, :paid_until, :trial_period_expires_at, :setup_fee, :type, :variable_cost_paid_until, :application_id, :user_key, :user_account_id, :tenant_id, :audit_ids
-
   # TODO: unit test this scope
   def self.provided_by(account)
     where.has do

app/models/contract/trial.rb

@@ -6,8 +6,6 @@ module Contract::Trial
   included do
     before_create :set_trial_period_expires_at, :set_setup_fee
 
-    attr_protected :trial_period_expires_at
-
     sifter :trial_period_expires_on do |date|
       sift(:date, trial_period_expires_at) == sift(:to_date, quoted(date.to_date))
     end

lib/developer_portal/app/controllers/developer_portal/admin/applications_controller.rb

@@ -63,7 +63,7 @@ def update
       application.validate_fields!
     end
 
-    application.update(application_params)
+    application.update(permitted_application_params)
     assign_drops application: application
     # make sure to prevent xss on js rendering if this notice is changed to include e.g. application name
     if application.valid?
@@ -127,7 +127,7 @@ def application
   end
 
   def new_application
-    @cinstance ||= applications.build_with_fields(application_params) do |application|
+    @cinstance ||= applications.build_with_fields(permitted_application_params) do |application|
       application.plan = application.can_change_plan?(service) ? plan : default_plan
     end
   end
@@ -148,10 +148,6 @@ def plan
     end
   end
 
-  def application_params
-    @application_params ||= accepted_application_params
-  end
-
   def authorize_new_app
     if service
       authorize! :create_application, service
@@ -168,13 +164,14 @@ def authorize_update_app
     authorize! :update, application
   end
 
-  def accepted_application_params
+  def application_params
     # cinstance[*] naming is present for legacy reasons
-    application_attributes = params[:application] || params[:cinstance]
-    return {} unless application_attributes
+    @application_params ||= params[:application] || params.fetch(:cinstance, {})
+  end
 
-    permitted_params = fields_definitions + %i[plan_id redirect_url]
-    application_attributes.permit(*permitted_params)
+  def permitted_application_params
+    permitted_params = fields_definitions + %i[redirect_url]
+    application_params.permit(*permitted_params)
   end
 
   def fields_definitions

test/integration/api/applications_controller_test.rb

@@ -82,22 +82,22 @@ def setup
 
       attr_reader :service_plan, :buyer, :application_plan, :service
 
-      test 'crate application redirects to the provider admin index page' do
+      test 'create application redirects to the provider admin index page' do
         post admin_service_applications_path(service), params: { account_id: buyer.id,
                                                                  cinstance: { service_plan_id: service_plan.id, plan_id: application_plan.id, name: 'My Application' } }
 
         assert_redirected_to provider_admin_application_path(Cinstance.last)
       end
 
-      test 'crate application with no service plan selected' do
+      test 'create application with no service plan selected' do
         post admin_service_applications_path(service), params: { account_id: buyer.id,
                                                                  cinstance: { plan_id: application_plan.id, name: 'My Application' } }
 
         application = Cinstance.last
         assert_redirected_to provider_admin_application_path(application)
       end
 
-      test 'crate application with no service plan selected and a default service plan' do
+      test 'create application with no service plan selected and a default service plan' do
         default_service_plan = FactoryBot.create(:service_plan, service: service)
         service.update(default_service_plan: default_service_plan)
         post admin_service_applications_path(service), params: { account_id: buyer.id,
@@ -107,7 +107,7 @@ def setup
         assert_equal default_service_plan, buyer.bought_service_contracts.first.service_plan
       end
 
-      test 'crate application with no service plan selected and no default service plan' do
+      test 'create application with no service plan selected and no default service plan' do
         other_service_plan = FactoryBot.create(:service_plan, service: service)
         service.update(default_service_plan: nil)
         post admin_service_applications_path(service), params: { account_id: buyer.id,
@@ -117,7 +117,7 @@ def setup
         assert_not_equal other_service_plan, buyer.bought_service_contracts.first.service_plan
       end
 
-      test 'crate application with no service plan selected and a subscription' do
+      test 'create application with no service plan selected and a subscription' do
         subscribed_service_plan = FactoryBot.create(:service_plan, service: service)
         buyer.bought_service_contracts.create(plan: subscribed_service_plan)
         post admin_service_applications_path(service), params: { account_id: buyer.id,