[PATCH 4/4] redmine #3601: whitelist all attributes as needing to be explicitly set by default

Note this patch requires rails 3.2 to work (which will be in Fedora soon [1]) This changes the application so that no mass assigments are permitted by default. I added the neccessary model changes so that things should be working again with the current mass assignments in place in the application (specs pass after this patch) Obviously this still doesn't solve the mass assignment issue, but the items that should be removed and explicitly set are marked with comments and thus we can tackle them one by one in the various areas that they need to be set explicitly in the application [1] http://lists.fedoraproject.org/pipermail/ruby-sig/2012-July/001077.html --- src/app/controllers/application_controller.rb | 2 +- src/app/models/base_permission_object.rb | 2 ++ src/app/models/catalog.rb | 5 +++++ src/app/models/catalog_entry.rb | 5 +++++ src/app/models/credential_definition.rb | 6 ++++++ src/app/models/deployable.rb | 5 +++++ src/app/models/deployment.rb | 5 +++++ src/app/models/derived_permission.rb | 3 +++ src/app/models/event.rb | 5 +++++ src/app/models/frontend_realm.rb | 2 ++ src/app/models/hardware_profile.rb | 2 ++ src/app/models/hardware_profile_property.rb | 2 ++ src/app/models/hook.rb | 2 ++ src/app/models/instance.rb | 6 ++++++ src/app/models/instance_key.rb | 5 +++++ src/app/models/instance_match.rb | 3 +++ src/app/models/instance_task.rb | 3 +++ src/app/models/metadata_object.rb | 1 + src/app/models/permission.rb | 3 +++ src/app/models/pool.rb | 2 ++ src/app/models/pool_family.rb | 2 ++ src/app/models/privilege.rb | 2 ++ src/app/models/property_enum_entry.rb | 2 ++ src/app/models/provider.rb | 2 ++ src/app/models/provider_account.rb | 2 ++ src/app/models/provider_type.rb | 2 ++ src/app/models/quota.rb | 2 ++ src/app/models/realm.rb | 2 ++ src/app/models/realm_backend_target.rb | 2 ++ src/app/models/role.rb | 2 ++ src/app/models/session_entity.rb | 3 +++ src/app/models/task.rb | 2 ++ src/app/models/user.rb | 2 ++ src/app/models/user_group.rb | 2 ++ src/app/models/view_state.rb | 2 ++ src/config/application.rb | 2 ++ src/spec/models/instance_spec.rb | 2 +- 37 files changed, 102 insertions(+), 2 deletions(-) diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb index b6381ff..daf6b01 100644 --- a/src/app/controllers/application_controller.rb +++ b/src/app/controllers/application_controller.rb @@ -172,7 +172,7 @@ class ApplicationController < ActionController::Base arr = Array.new instance_variables.each do |ivar| val = instance_variable_get(ivar) - if val && val.respond_to?(:errors) && val.errors.size > 0 + if val && val.respond_to?(:errors) && !val.errors.nil? && val.errors.size > 0 hash[:object] = ivar[1, ivar.size] hash[:errors] ||= [] val.errors.each {|key,msg| diff --git a/src/app/models/base_permission_object.rb b/src/app/models/base_permission_object.rb index c5d525a..e9cc23d 100644 --- a/src/app/models/base_permission_object.rb +++ b/src/app/models/base_permission_object.rb @@ -35,6 +35,8 @@ class BasePermissionObject < ActiveRecord::Base :include => [:role], :order => "derived_permissions.id ASC" + attr_accessible :name + validates_presence_of :name validates_uniqueness_of :name diff --git a/src/app/models/catalog.rb b/src/app/models/catalog.rb index 4ea3df1..9fdc15f 100644 --- a/src/app/models/catalog.rb +++ b/src/app/models/catalog.rb @@ -46,6 +46,11 @@ class Catalog < ActiveRecord::Base before_create :set_pool_family after_update :update_deployable_permissions + attr_accessible :name + + # TODO update app to set pool explicitly and remove this + attr_accessible :pool + validates_presence_of :pool validates_presence_of :name validates_uniqueness_of :name diff --git a/src/app/models/catalog_entry.rb b/src/app/models/catalog_entry.rb index 03461d2..f532e41 100644 --- a/src/app/models/catalog_entry.rb +++ b/src/app/models/catalog_entry.rb @@ -30,6 +30,11 @@ class CatalogEntry < ActiveRecord::Base belongs_to :catalog belongs_to :deployable + attr_accessible :name, :description + + # TODO update app to set owner and catalog explitly and remove this + attr_accessible :owner, :catalog + validates_presence_of :catalog validates_presence_of :deployable diff --git a/src/app/models/credential_definition.rb b/src/app/models/credential_definition.rb index a920cb1..c87b0ca 100644 --- a/src/app/models/credential_definition.rb +++ b/src/app/models/credential_definition.rb @@ -31,6 +31,12 @@ class CredentialDefinition < ActiveRecord::Base belongs_to :provider_type has_many :credentials + + attr_accessible :name, :label, :input_type + + # TODO update add to set provider_type_id explicitly and remove this + attr_accessible :provider_type_id + validates_presence_of :name validates_uniqueness_of :name, :scope => :provider_type_id validates_presence_of :label diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb index e1d80d5..1c5e6bb 100644 --- a/src/app/models/deployable.rb +++ b/src/app/models/deployable.rb @@ -24,6 +24,11 @@ class Deployable < ActiveRecord::Base include PermissionedObject + attr_accessible :name, :description + + # TODO update app to set owner explicitly and remove this + attr_accessible :owner, :assemblies, :xml + validates_presence_of :name validates_uniqueness_of :name validates_length_of :name, :maximum => 1024 diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb index 277f9b3..d4b782d 100644 --- a/src/app/models/deployment.rb +++ b/src/app/models/deployment.rb @@ -69,6 +69,11 @@ class Deployment < ActiveRecord::Base scope :ascending_by_name, :order => 'deployments.name ASC' + attr_accessible :name + + # TODO update app to set pool and owner explicitly and remove this + attr_accessible :pool, :owner, :pool_id + before_validation :replace_special_characters_in_name validates_presence_of :pool_id validates_presence_of :name diff --git a/src/app/models/derived_permission.rb b/src/app/models/derived_permission.rb index 80d055d..bd9f730 100644 --- a/src/app/models/derived_permission.rb +++ b/src/app/models/derived_permission.rb @@ -46,6 +46,9 @@ class DerivedPermission < ActiveRecord::Base belongs_to :base_permission_object, :class_name => "BasePermissionObject", :foreign_key => "permission_object_id" + # TODO update app to set all these attributes explicitly and remove this + attr_accessible :permission, :permission_object, :role_id, :entity_id + # role is copied from source permission belongs_to :role validates_presence_of :role_id diff --git a/src/app/models/event.rb b/src/app/models/event.rb index 8479c58..384e347 100644 --- a/src/app/models/event.rb +++ b/src/app/models/event.rb @@ -39,6 +39,11 @@ class Event < ActiveRecord::Base require "#{Rails.root}/lib/aeolus/event.rb" belongs_to :source, :polymorphic => true + attr_accessible :event_time, :summary, :status_code + + # TODO update app to set these attributes explicitly and remove this + attr_accessible :source_type, :source_id, :source + validates_presence_of :source_id validates_presence_of :source_type diff --git a/src/app/models/frontend_realm.rb b/src/app/models/frontend_realm.rb index 032d0fa..8e0e35c 100644 --- a/src/app/models/frontend_realm.rb +++ b/src/app/models/frontend_realm.rb @@ -36,6 +36,8 @@ class FrontendRealm < ActiveRecord::Base has_many :realm_backend_targets, :dependent => :destroy has_many :instances + attr_accessible :name + # there is a problem with has_many through + polymophic in AR: # http://blog.hasmanythrough.com/2006/4/3/polymorphic-through # so we define explicitly backend_realms and backend_providers diff --git a/src/app/models/hardware_profile.rb b/src/app/models/hardware_profile.rb index 2099214..881a6e0 100644 --- a/src/app/models/hardware_profile.rb +++ b/src/app/models/hardware_profile.rb @@ -70,6 +70,8 @@ class HardwareProfile < ActiveRecord::Base accepts_nested_attributes_for :memory, :cpu, :storage, :architecture + attr_accessible :name, :external_key, :cpu, :memory, :storage, :architecture + validates_presence_of :external_key, :if => Proc.new { |hwp| !hwp.provider.nil? } validates_uniqueness_of :external_key, :scope => [:provider_id], :if => Proc.new { |hwp| !hwp.provider.nil? } diff --git a/src/app/models/hardware_profile_property.rb b/src/app/models/hardware_profile_property.rb index 4f883eb..4af4fb2 100644 --- a/src/app/models/hardware_profile_property.rb +++ b/src/app/models/hardware_profile_property.rb @@ -52,6 +52,8 @@ class HardwareProfileProperty < ActiveRecord::Base has_many :property_enum_entries + attr_accessible :name, :kind, :unity, :value, :range_first, :range_last + before_validation :is_form_empty? validates_presence_of :name validates_inclusion_of :name, diff --git a/src/app/models/hook.rb b/src/app/models/hook.rb index e95ece4..435b838 100644 --- a/src/app/models/hook.rb +++ b/src/app/models/hook.rb @@ -16,4 +16,6 @@ class Hook < ActiveRecord::Base validates_presence_of :uri validates_presence_of :version + + attr_accessible :uri, :version end diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb index fad0d03..24f8384 100644 --- a/src/app/models/instance.rb +++ b/src/app/models/instance.rb @@ -93,6 +93,12 @@ class Instance < ActiveRecord::Base has_many :tasks, :as =>:task_target, :dependent => :destroy after_create "assign_owner_roles(owner)" + # TODO update app to set all these attributes explicitly and remove this + attr_accessible :name, :pool_id, :state, :external_key, :hardware_profile, :hardware_profile_id, + :provider_account_id, :provider_account, :pool, :owner, :owner_id, :provider_account_id, + :instance_hwp_id, :deployment_id, :provider_instance_id, :last_error, :frontend_realm, :image_uuid, :image_build_uuid, + :assembly_xml + validates_presence_of :pool_id validates_presence_of :hardware_profile_id diff --git a/src/app/models/instance_key.rb b/src/app/models/instance_key.rb index 457f5d6..19ebd39 100644 --- a/src/app/models/instance_key.rb +++ b/src/app/models/instance_key.rb @@ -34,6 +34,11 @@ class InstanceKey < ActiveRecord::Base belongs_to :instance before_destroy :destroy_instance_key + attr_accessible :name, :pem, :instance + + # TODO update app to set instance explicitly and remove this + attr_accessible :instance + def destroy_instance_key begin instance.provider_account.connect.key(self.name).destroy! diff --git a/src/app/models/instance_match.rb b/src/app/models/instance_match.rb index 8d797ca..9ef2100 100644 --- a/src/app/models/instance_match.rb +++ b/src/app/models/instance_match.rb @@ -5,6 +5,9 @@ class InstanceMatch < ActiveRecord::Base belongs_to :hardware_profile belongs_to :realm + # TODO update the app to set these attributes explicitly and remove this + attr_accessible :instance, :hardware_profile, :provider_account + def equals?(other) return self.nil? && other.nil? if (self.nil? || other.nil?) self.pool_family_id == other.pool_family_id && diff --git a/src/app/models/instance_task.rb b/src/app/models/instance_task.rb index d28e0e6..b71152d 100644 --- a/src/app/models/instance_task.rb +++ b/src/app/models/instance_task.rb @@ -50,7 +50,10 @@ class InstanceTask < Task # FIXME: do we need this for db-omatic? ACTION_UPDATE_STATE = "update_state" + attr_accessible :instance, :type, :state, :action, :failure_code + # TODO upate app to set task_target explicitly and remove this + attr_accessible :task_target_id, :task_target_type, :task_target def task_obj if self.instance diff --git a/src/app/models/metadata_object.rb b/src/app/models/metadata_object.rb index b344008..cd6f4b0 100644 --- a/src/app/models/metadata_object.rb +++ b/src/app/models/metadata_object.rb @@ -32,6 +32,7 @@ # Likewise, all the methods added will be available for all controllers. class MetadataObject < ActiveRecord::Base + attr_accessible :key validates_presence_of :key validates_uniqueness_of :key diff --git a/src/app/models/permission.rb b/src/app/models/permission.rb index 9b4c0ac..db8f6de 100644 --- a/src/app/models/permission.rb +++ b/src/app/models/permission.rb @@ -36,6 +36,9 @@ class Permission < ActiveRecord::Base belongs_to :role belongs_to :entity + # TODO upate app to set these attributes explicitly and remove this + attr_accessible :role, :permission_object, :entity + validates_presence_of :role_id validates_presence_of :entity_id diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb index 13baebc..e3f025d 100644 --- a/src/app/models/pool.rb +++ b/src/app/models/pool.rb @@ -47,6 +47,8 @@ class Pool < ActiveRecord::Base #has_many :images, :dependent => :destroy has_many :catalogs, :dependent => :destroy + attr_accessible :name, :quota, :pool_family, :pool_family_id, :enabled, :quota + validates_presence_of :name validates_presence_of :quota validates_presence_of :pool_family diff --git a/src/app/models/pool_family.rb b/src/app/models/pool_family.rb index f464723..50c7d20 100644 --- a/src/app/models/pool_family.rb +++ b/src/app/models/pool_family.rb @@ -52,6 +52,8 @@ class PoolFamily < ActiveRecord::Base has_many :instances has_many :deployments + attr_accessible :name, :description, :quota, :quota_attributes + validates_length_of :name, :maximum => 255 validates_format_of :name, :with => /^[\w -]*$/n diff --git a/src/app/models/privilege.rb b/src/app/models/privilege.rb index 1d857db..fbaaab1 100644 --- a/src/app/models/privilege.rb +++ b/src/app/models/privilege.rb @@ -62,6 +62,8 @@ class Privilege < ActiveRecord::Base ProviderAccount => ACTIONS, User => [ CREATE, MODIFY, VIEW] } + attr_accessible :role, :target_type, :action + belongs_to :role validates_presence_of :role_id validates_presence_of :target_type diff --git a/src/app/models/property_enum_entry.rb b/src/app/models/property_enum_entry.rb index 7032536..b61e3da 100644 --- a/src/app/models/property_enum_entry.rb +++ b/src/app/models/property_enum_entry.rb @@ -34,6 +34,8 @@ class PropertyEnumEntry < ActiveRecord::Base belongs_to :hardware_profile_property + attr_accessible :value, :hardware_profile_property + validates_presence_of :value validates_presence_of :hardware_profile_property validates_numericality_of :value, :greater_than => 0, diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb index cf2a7e7..2be719d 100644 --- a/src/app/models/provider.rb +++ b/src/app/models/provider.rb @@ -44,6 +44,8 @@ class Provider < ActiveRecord::Base has_many :frontend_realms, :through => :realm_backend_targets belongs_to :provider_type + attr_accessible :name, :provider_type, :url, :hardware_profiles, :realms + validates_presence_of :name validates_uniqueness_of :name validates_presence_of :provider_type_id diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb index 067f5cf..6ad9591 100644 --- a/src/app/models/provider_account.rb +++ b/src/app/models/provider_account.rb @@ -61,6 +61,8 @@ class ProviderAccount < ActiveRecord::Base # Helpers attr_accessor :x509_cert_priv_file, :x509_cert_pub_file + attr_accessible :provider, :quota, :label + # Validations validates_presence_of :provider validates_presence_of :label diff --git a/src/app/models/provider_type.rb b/src/app/models/provider_type.rb index 205752d..19c5e5d 100644 --- a/src/app/models/provider_type.rb +++ b/src/app/models/provider_type.rb @@ -33,6 +33,8 @@ class ProviderType < ActiveRecord::Base has_many :providers has_many :credential_definitions, :dependent => :destroy + attr_accessible :id, :name, :deltacloud_driver + validates_presence_of :name validates_uniqueness_of :name validates_presence_of :deltacloud_driver diff --git a/src/app/models/quota.rb b/src/app/models/quota.rb index 2c8dcee..6aa6672 100644 --- a/src/app/models/quota.rb +++ b/src/app/models/quota.rb @@ -41,6 +41,8 @@ class Quota < ActiveRecord::Base has_one :provider_account has_one :user + attr_accessible :running_instances, :total_instances, :maximum_running_instances, :maximum_total_instances + validates_numericality_of :maximum_total_instances, :greater_than_or_equal_to => 0, :less_than_or_equal_to => 2147483647, diff --git a/src/app/models/realm.rb b/src/app/models/realm.rb index 962cd70..1e8e110 100644 --- a/src/app/models/realm.rb +++ b/src/app/models/realm.rb @@ -41,6 +41,8 @@ class Realm < ActiveRecord::Base has_many :realm_backend_targets, :as => :realm_or_provider, :dependent => :destroy has_many :frontend_realms, :through => :realm_backend_targets + attr_accessible :name, :external_key, :provider + validates_presence_of :external_key validates_uniqueness_of :external_key, :scope => :provider_id diff --git a/src/app/models/realm_backend_target.rb b/src/app/models/realm_backend_target.rb index c3801b1..875fe39 100644 --- a/src/app/models/realm_backend_target.rb +++ b/src/app/models/realm_backend_target.rb @@ -31,6 +31,8 @@ class RealmBackendTarget < ActiveRecord::Base belongs_to :realm, :class_name => 'Realm', :foreign_key => 'realm_or_provider_id' belongs_to :provider, :class_name => 'Provider', :foreign_key => 'realm_or_provider_id' + attr_accessible :frontend_realm, :realm_or_provider + validates_uniqueness_of :frontend_realm_id, :scope => [:realm_or_provider_id, :realm_or_provider_type] validates_presence_of :realm_or_provider diff --git a/src/app/models/role.rb b/src/app/models/role.rb index dcf3178..ddff768 100644 --- a/src/app/models/role.rb +++ b/src/app/models/role.rb @@ -33,6 +33,8 @@ class Role < ActiveRecord::Base has_many :derived_permissions, :dependent => :destroy has_many :privileges, :dependent => :destroy + attr_accessible :name, :scope, :assign_to_owner + validates_presence_of :scope validates_presence_of :name validates_uniqueness_of :name diff --git a/src/app/models/session_entity.rb b/src/app/models/session_entity.rb index ea3f547..2db1bce 100644 --- a/src/app/models/session_entity.rb +++ b/src/app/models/session_entity.rb @@ -18,6 +18,9 @@ class SessionEntity < ActiveRecord::Base belongs_to :user belongs_to :entity + attr_accessible :user, :session, :entity, + :user_id, :session_id, :entity_id + validates_presence_of :user_id validates_presence_of :session_id validates_presence_of :entity_id diff --git a/src/app/models/task.rb b/src/app/models/task.rb index 6d65fcc..2f1bfe7 100644 --- a/src/app/models/task.rb +++ b/src/app/models/task.rb @@ -67,6 +67,8 @@ class Task < ActiveRecord::Base FAILURE_CODES = [FAILURE_PROVIDER_NOT_FOUND, FAILURE_PROVIDER_CONTACT_FAILED, FAILURE_PROVIDER_RETURNED_FAILED, FAILURE_OVER_POOL_QUOTA] + attr_accessible :created_at, :time_started, :time_ended, :state + validates_inclusion_of :failure_code, :in => FAILURE_CODES + [nil] diff --git a/src/app/models/user.rb b/src/app/models/user.rb index 73579e5..54a0197 100644 --- a/src/app/models/user.rb +++ b/src/app/models/user.rb @@ -74,6 +74,8 @@ class User < ActiveRecord::Base before_validation :strip_whitespace after_save :update_entity + attr_accessible :login, :password, :password_confirmation, :first_name, :last_name, :quota, :email, :ignore_password + validates_presence_of :quota validates_length_of :first_name, :maximum => 255, :allow_blank => true validates_length_of :last_name, :maximum => 255, :allow_blank => true diff --git a/src/app/models/user_group.rb b/src/app/models/user_group.rb index 3d4513d..c79eed2 100644 --- a/src/app/models/user_group.rb +++ b/src/app/models/user_group.rb @@ -31,6 +31,8 @@ class UserGroup < ActiveRecord::Base MEMBERSHIP_SOURCE_LOCAL = "local" MEMBERSHIP_SOURCES = [MEMBERSHIP_SOURCE_LOCAL, MEMBERSHIP_SOURCE_LDAP] + attr_accessible :name, :membership_source + validates_presence_of :name # scope name by membership_source to prevent errors if users are later added # to external ldap groups that have the same name as local groups diff --git a/src/app/models/view_state.rb b/src/app/models/view_state.rb index 3d763b6..8b643eb 100644 --- a/src/app/models/view_state.rb +++ b/src/app/models/view_state.rb @@ -33,6 +33,8 @@ class ViewState < ActiveRecord::Base set_primary_key :uuid belongs_to :user + attr_accessible :name, :controller, :action, :station, :association + validates_presence_of :name validates_uniqueness_of :name validates_presence_of :user_id diff --git a/src/config/application.rb b/src/config/application.rb index 88d6d1d..21a4cd8 100644 --- a/src/config/application.rb +++ b/src/config/application.rb @@ -80,5 +80,7 @@ module Conductor #config.middleware.swap Rack::MethodOverride, Rack::RestfulSubmit config.middleware.insert_before(Rack::MethodOverride, Rack::RestfulSubmit) ActiveRecord::Base.include_root_in_json = false + + config.active_record.whitelist_attributes = true end end diff --git a/src/spec/models/instance_spec.rb b/src/spec/models/instance_spec.rb index 87dd77d..81dd75b 100644 --- a/src/spec/models/instance_spec.rb +++ b/src/spec/models/instance_spec.rb @@ -171,7 +171,7 @@ describe Instance do end it "should return empty list of instance actions when connect to provider fails" do - provider = Factory.build(:mock_provider2) + provider = Factory.create(:mock_provider2) cloud_account = Factory.build(:provider_account, :provider => provider) cloud_account.stub!(:connect).and_return(nil) cloud_account.stub!(:valid_credentials?).and_return(true) -- 1.7.10.2