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

Gemfile

@@ -20,8 +20,6 @@ gem "activejob-uniqueness"
 # Needed for XML serialization of ActiveRecord::Base
 gem 'activemodel-serializers-xml'
 
-gem 'protected_attributes_continued', '~> 1.9.0'
-
 gem 'rails-observers'
 
 gem 'strong_migrations', '~> 2.1.0'

Gemfile.lock

@@ -618,8 +618,6 @@ GEM
       bigdecimal
       logger
       rb_sys (~> 0.9.117)
-    protected_attributes_continued (1.9.0)
-      activemodel (>= 5.0)
     pry (0.15.2)
       coderay (~> 1.1)
       method_source (~> 1.0)
@@ -1101,7 +1099,6 @@ DEPENDENCIES
   prawn
   prawn-svg
   prawn-table!
-  protected_attributes_continued (~> 1.9.0)
   pry-byebug (>= 3.11.0)
   pry-doc (>= 0.8)
   pry-rails

app/events/applications/application_deleted_event.rb

@@ -5,7 +5,7 @@ class Applications::ApplicationDeletedEvent < ApplicationRelatedEvent
   # @param [Cinstance] application
   # :reek:NilCheck but proxy can be nil at this point
   def self.create(application)
-    service = application.service || Service.new({id: application.service_id}, without_protection: true)
+    service = application.service || Service.new(id: application.service_id)
     new(
       application: MissingModel::MissingApplication.new(id: application.id),
       service_backend_id: service.backend_id,

app/events/zync_event.rb

@@ -70,7 +70,7 @@ def self.type_for(model)
   def non_persisted_dependencies
     case record
     when Proxy, Cinstance
-      [Service.new({id: data[:service_id]}, without_protection: true)]
+      [Service.new(id: data[:service_id])]
     else
       NONE
     end

app/lib/backend_api_logic/service_extension.rb

@@ -18,6 +18,11 @@ module ServiceExtension
       end, class_name: 'Metric'
     end
 
+    # This class is a compatibility layer added in https://github.com/3scale/porta/pull/1211
+    # to make the legacy code work after BackendApis ("API as a Product" feature) were introduced.
+    # For example, code like `proxy.update(api_backend: "value")` still works, even though `api_backend` is 
+    # now a field of BackendApi (aka `private_endpoint`), and not Proxy's.
+    # It is achieved by building a BackendApi and a BackendApiConfig objects lazily .
     class BackendApiProxy
       attr_reader :service
       delegate :account, :backend_apis, :backend_api_configs, to: :service
@@ -32,8 +37,16 @@ def backend_api_config
                                 backend_api_configs.build(path: '/', backend_api: backend_api)
       end
 
+      # Lazy building is required for some old code to work.
+      # However, it is needed to be dissociated, because otherwise `provider.backend_apis` will include this object,
+      # and validation will fail, because the lazily built BackendApi object itself might not be valid (due to a missing
+      # `backend_endpoint`).
       def backend_api
-        @backend_api ||= backend_api_configs.first&.backend_api || account.backend_apis.build(system_name: service_system_name, name: "#{service_name} Backend", description: "Backend of #{service_name}")
+        # Return early if we already have a persisted backend_api
+        return @backend_api if @backend_api&.persisted?
+
+        # Try to get backend_api from backend_api_configs, or keep the memoized unpersisted one, or build a new one
+        @backend_api = backend_api_configs.first&.backend_api || @backend_api || build_dissociated_backend_api
       end
 
       def update!(attrs = {})
@@ -52,6 +65,18 @@ def update(attrs = {})
       rescue ActiveRecord::RecordInvalid
         false
       end
+
+      private
+
+      # Build a BackendApi without adding it to `backend_apis` association to avoid polluting it with unpersisted records.
+      def build_dissociated_backend_api
+        BackendApi.new(
+          account: account,
+          system_name: service_system_name,
+          name: "#{service_name} Backend",
+          description: "Backend of #{service_name}"
+        )
+      end
     end
 
     def backend_api_proxy

app/lib/fields/fields.rb

@@ -115,7 +115,7 @@ def initialize(attributes = nil, *args)
     # otherwise it raises exception on unknown (extra) fields
     # check http://api.rubyonrails.org/classes/ActiveRecord/AttributeAssignment.html#method-i-assign_attributes
 
-    def assign_attributes(extra_attributes, options = {})
+    def assign_attributes(extra_attributes)
       # dup the fields because we are mutating them (params hash)
       attributes = extra_attributes.present? ? extra_attributes.dup : {}
 
@@ -126,7 +126,7 @@ def assign_attributes(extra_attributes, options = {})
         self.extra_fields = fields_attributes
       end
 
-      super(attributes, options)
+      super(attributes)
     end
 
     alias attributes= assign_attributes

app/lib/finance/billing.rb

@@ -8,7 +8,7 @@ def initialize(invoice)
 
     def create_line_item(params)
       bill do
-        @invoice.line_items.create(params, {without_protection: true})
+        @invoice.line_items.create(params)
       end
     rescue Invoice::InvalidInvoiceStateException
       line_item_with_error(params, :invalid_invoice_state)
@@ -18,7 +18,7 @@ def create_line_item(params)
 
     def create_line_item!(params)
       bill do
-        @invoice.line_items.create!(params, {without_protection: true})
+        @invoice.line_items.create!(params)
       end
     end
 
@@ -37,7 +37,7 @@ def bill
     end
 
     def line_item_with_error(params, error_type)
-      LineItem.new(params, {without_protection: true}).tap do |line_item|
+      LineItem.new(params).tap do |line_item|
         line_item.errors.add(:base, error_type.to_sym)
       end
     end

app/lib/logic/cms.rb

@@ -64,7 +64,7 @@ def cms_toolbar_intro_visible?
       end
 
       def create_cms_assets
-        self.builtin_sections.create!({ title: 'Root', system_name: 'root', parent_id: nil }, without_protection: true)
+        self.builtin_sections.create!(title: 'Root', system_name: 'root', parent_id: nil)
         # TODO: add SimpleLayout logic here
       end
     end

app/lib/signup/impersonation_admin_builder.rb

@@ -6,14 +6,12 @@ def self.build(account:)
       config = ThreeScale.config.impersonation_admin
       username = config[:username]
       impersonation_admin = account.users.new(
-        {
-          username: username,
-          email: "#{username}+#{account.internal_domain}@#{config[:domain]}",
-          first_name: '3scale',
-          last_name: 'Admin',
-          state: 'active'
-        },
-        without_protection: true)
+        username: username,
+        email: "#{username}+#{account.internal_domain}@#{config[:domain]}",
+        first_name: '3scale',
+        last_name: 'Admin',
+        state: 'active'
+      )
       impersonation_admin.role = :admin
       impersonation_admin.signup_type = :minimal
       impersonation_admin

app/models/access_token.rb

@@ -178,8 +178,7 @@ def validate_expiration_date
   end
 
   def generate_if_missing
-    return if persisted?
-    return if @plaintext_value.present?
+    return if value.present?
 
     @plaintext_value = self.class.random_id
     self.value = self.class.compute_digest(@plaintext_value)

app/models/account_setting.rb

@@ -11,9 +11,6 @@ def self.find_sti_class(type_name)
     self
   end
 
-  # TODO: remove attr_accessible once protected_attributes_continued gem is removed
-  attr_accessible :type, :value, :account, :tenant_id
-
   belongs_to :account, inverse_of: :account_settings
 
   audited associated_with: :account

app/models/cms/template.rb

@@ -94,7 +94,7 @@ def current
 
   def build_version(attrs = {})
     version = versions.build
-    version.assign_attributes(version_attributes.merge(attrs), :without_protection => true)
+    version.assign_attributes(version_attributes.merge(attrs))
     version
   end
 

app/models/cms/template/version.rb

@@ -32,10 +32,8 @@ def diff(other)
   end
 
   def revert_attributes
-    accessible = template.class.accessible_attributes.delete('draft').add('updated_at').add('updated_by')
-
-    accessible << state.to_s
-    attributes.slice *accessible
+    accessible = [:updated_at, :updated_by, state.to_s]
+    attributes.slice(*accessible)
   end
 
 end

app/models/contract.rb

@@ -4,7 +4,7 @@ class Contract < ApplicationRecord
   # https://github.com/collectiveidea/audited/blob/f03c5b5d1717f2ebec64032d269316dc74476056/lib/audited/auditor.rb#L305-L311
   self.table_name = 'cinstances'
 
-  audited allow_mass_assignment: true
+  audited
 
   # FIXME: This class should be an abstract class I think, but doing so makes plenty of tests fail
   # self.abstract_class = true

app/models/feature.rb

@@ -2,7 +2,7 @@ class Feature < ApplicationRecord
 
   self.background_deletion = [:features_plans]
 
-  audited :allow_mass_assignment => true
+  audited
 
   #TODO: these need tests
   #OPTIMIZE subclassing a better solution for these 2

app/models/finance/billing_strategy.rb

@@ -11,7 +11,7 @@ class << self
     prepend NonAuditedColumns
   end
 
-  audited :allow_mass_assignment => true
+  audited
 
   attr_reader :failed_buyers
 

app/models/invoice.rb

@@ -14,7 +14,7 @@ class Invoice < ApplicationRecord
   enum creation_type: {manual: 'manual', background: 'background'}
 
   include AfterCommitQueue
-  audited :allow_mass_assignment => true
+  audited
   has_associated_audits
 
   class InvalidInvoiceStateException < RuntimeError; end

app/models/line_item.rb

@@ -5,7 +5,7 @@ class LineItem < ApplicationRecord
   belongs_to :contract, polymorphic: true
   belongs_to :metric, inverse_of: :line_items
 
-  audited associated_with: :invoice, allow_mass_assignment: true
+  audited associated_with: :invoice
 
   validates :name, :description, :type, :contract_type, length: { maximum: 255 }
   validates :type, inclusion: {in: [LineItem::PlanCost, LineItem::VariableCost].flat_map { |klass| [klass, klass.to_s]}, allow_blank: true}

app/models/mail_dispatch_rule.rb

@@ -10,14 +10,15 @@ def self.fetch_with_retry!(options, &block)
     retries = 0
 
     begin
-      MailDispatchRule.find_or_create_by!(options, &block)
+      # Use find_by + create! directly to avoid transaction wrapper from create_or_find_by!
+      # This preserves the old behavior from protected_attributes_continued gem
+      # where records created in the block are committed even if the outer create fails
+      find_by(options) || create!(options, &block)
     rescue ActiveRecord::RecordNotUnique
-      if retries > 10
-        raise
-      else
-        retries += 1
-        retry
-      end
+      raise if retries > 10
+
+      retries += 1
+      retry
     end
   end
 end

app/models/metric.rb

@@ -21,7 +21,7 @@ class Metric < ApplicationRecord
 
   has_many :proxy_rules, :dependent => :destroy, inverse_of: :metric
 
-  audited :allow_mass_assignment => true
+  audited
   has_system_name uniqueness_scope: %i[owner_type owner_id], human_name: :friendly_name
 
   acts_as_tree

app/models/payment_detail.rb

@@ -10,7 +10,7 @@ class PaymentDetail < ApplicationRecord
 
   belongs_to :account
 
-  audited allow_mass_assignment: true
+  audited
 
   validates :credit_card_partial_number, length: { :maximum => 4,
                                                    :allow_blank => true,

app/models/plan.rb

@@ -16,7 +16,7 @@ class PeriodRangeCalculationError < StandardError; end
 
   has_system_name :uniqueness_scope => [ :type, :issuer_id, :issuer_type ]
 
-  audited :allow_mass_assignment => true
+  audited
   acts_as_list scope: %i[issuer_id issuer_type]
 
   # There is a race condition here, the record gets deleted but the callbacks were not triggered yet

app/models/pricing_rule.rb

@@ -1,6 +1,6 @@
 class PricingRule < ApplicationRecord
   DECIMALS = 4
-  audited :allow_mass_assignment => true
+  audited
 
   belongs_to :plan
   belongs_to :metric

app/models/provider_constraints.rb

@@ -1,7 +1,7 @@
 class ProviderConstraints < ApplicationRecord
   NO_LIMIT = Float::INFINITY
 
-  audited allow_mass_assignment: true
+  audited
 
   belongs_to :provider, class_name: 'Account'
 

app/models/settings.rb

@@ -2,7 +2,7 @@ class Settings < ApplicationRecord
   include Symbolize
   belongs_to :account, inverse_of: :settings
 
-  audited allow_mass_assignment: true
+  audited
 
   validates :product, inclusion: { in: %w[connect enterprise].freeze }
   validates :change_account_plan_permission, :change_service_plan_permission, inclusion: { in: %w[request none credit_card request_credit_card direct].freeze }
@@ -37,8 +37,8 @@ def approval_required_disabled?
     not_custom_account_plans.size > 1 && account_plans_ui_visible?
   end
 
-  def assign_attributes(attrs, options = {})
-    super(sanitize_attributes(attrs), options)
+  def assign_attributes(attrs)
+    super(sanitize_attributes(attrs))
   end
 
   def update(attrs)

app/services/finance/stripe_payment_intent_update_service.rb

@@ -41,6 +41,6 @@ def create_payment_transaction
       params: source_object
     }
 
-    payment_transactions.create(attributes, without_protection: true)
+    payment_transactions.create(attributes)
   end
 end

app/services/service_discovery/import_cluster_definitions_service.rb

@@ -91,7 +91,7 @@ def import_cluster_active_docs_to(service)
     end
 
     def build_api_doc_service(service)
-      service.api_docs_services.build({ name: cluster_service.name, published: true, discovered: true }, without_protection: true)
+      service.api_docs_services.build(name: cluster_service.name, published: true, discovered: true)
     end
 
     def valid_cluster_service_spec?(service)

app/workers/audited_worker.rb

@@ -3,7 +3,7 @@ class AuditedWorker
 
   def perform(attributes)
     audit = Audited.audit_class.new
-    audit.assign_attributes(attributes, without_protection: true) # Rails 4 can remove the option
+    audit.assign_attributes(attributes)
     audit.save!
   end
 end

app/workers/backend_delete_service_worker.rb

@@ -11,7 +11,7 @@ def perform(event_id)
     event = EventStore::Repository.find_event!(event_id)
     service_id = event.service_id
     ThreeScale::Core::Service.delete_stats(service_id, {})
-    service = Service.new({id: service_id}, without_protection: true)
+    service = Service.new(id: service_id)
     service.delete_backend_service
   rescue ActiveRecord::RecordNotFound => exception
     System::ErrorReporting.report_error(exception, parameters: {event_id: event_id})

config/application.rb

@@ -131,8 +131,6 @@ def try_config_for(*args)
 
     config.logger = ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(STDOUT)) if ENV['RAILS_LOG_TO_STDOUT'].present?
 
-    config.active_record.whitelist_attributes = false
-
     config.boot_time = Time.now
 
     # Configuration for the application, engines, and railties goes here.