[PATCH Conductor 1/7] bug 788148: Take environment into account for permission inheritance
Scott Seago
sseago at redhat.com
Wed Feb 29 07:43:17 UTC 2012
https://bugzilla.redhat.com/show_bug.cgi?id=788148
Add 'pool family' to permission inheritance for various types. In addition, refactor permission queries to reduce the join complexity:
1) denormalize 'pool family' association into deployments, instances, etc. -- once created it never changes, so this reduces the number of joins for permissions
2) pre-generate a list of roles that include the desired privilege -- this keeps the permission queries from having to join permissions, roles, and privileges tables (all 3 tables) multiple times in a single query. Sometimes two queries are better than one. This resulted in significant speedup in queries where the user was granted permissions via the pool family.
---
src/app/models/catalog.rb | 27 +++++++++++++++
src/app/models/deployable.rb | 34 ++++++++++++++++++-
src/app/models/deployment.rb | 20 ++++++++---
src/app/models/instance.rb | 23 ++++++++-----
src/app/models/permissioned_object.rb | 8 ++--
src/app/models/pool.rb | 19 +++++++++++
src/app/models/provider_account.rb | 7 +---
.../20120227150102_add_pool_family_to_instance.rb | 32 ++++++++++++++++++
8 files changed, 144 insertions(+), 26 deletions(-)
create mode 100644 src/db/migrate/20120227150102_add_pool_family_to_instance.rb
diff --git a/src/app/models/catalog.rb b/src/app/models/catalog.rb
index 3472c59..307c4dd 100644
--- a/src/app/models/catalog.rb
+++ b/src/app/models/catalog.rb
@@ -32,6 +32,7 @@ class Catalog < ActiveRecord::Base
include PermissionedObject
belongs_to :pool
+ belongs_to :pool_family
has_many :catalog_entries, :dependent => :destroy
has_many :deployables, :through => :catalog_entries
has_many :permissions, :as => :permission_object, :dependent => :destroy,
@@ -39,12 +40,34 @@ class Catalog < ActiveRecord::Base
:order => "permissions.id ASC"
before_destroy :destroy_deployables_related_only_to_self
+ before_create :set_pool_family
validates_presence_of :pool
validates_presence_of :name
validates_uniqueness_of :name
validates_length_of :name, :maximum => 1024
+ def object_list
+ super + [pool, pool_family]
+ end
+ class << self
+ alias orig_list_for_user_include list_for_user_include
+ alias orig_list_for_user_conditions list_for_user_conditions
+ end
+
+ def self.list_for_user_include
+ orig_list_for_user_include + [ {:pool => :permissions},
+ {:pool_family => :permissions} ]
+ end
+
+ def self.list_for_user_conditions
+ "(#{orig_list_for_user_conditions}) or
+ (permissions_pools.user_id=:user and
+ permissions_pools.role_id in (:role_ids)) or
+ (permissions_pool_families.user_id=:user and
+ permissions_pool_families.role_id in (:role_ids))"
+ end
+
PRESET_FILTERS_OPTIONS = [
{:title => I18n.t("catalogs.preset_filters.belongs_to_default_pool"), :id => "belongs_to_default_pool", :query => includes(:pool).where("pools.name" => "Default")}
]
@@ -53,6 +76,10 @@ class Catalog < ActiveRecord::Base
deployables.each {|d| d.destroy if d.catalogs.count == 1}
end
+ def set_pool_family
+ self[:pool_family_id] = pool.pool_family_id
+ end
+
private
def self.apply_search_filter(search)
diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
index c77f094..ad537a7 100644
--- a/src/app/models/deployable.rb
+++ b/src/app/models/deployable.rb
@@ -34,9 +34,11 @@ class Deployable < ActiveRecord::Base
:order => "permissions.id ASC"
has_many :catalog_entries, :dependent => :delete_all
has_many :catalogs, :through => :catalog_entries
+ belongs_to :pool_family
belongs_to :owner, :class_name => "User", :foreign_key => "owner_id"
after_create "assign_owner_roles(owner)"
+ before_create :set_pool_family
scope :without_catalog, lambda {
deployable_ids_in_association = CatalogEntry.select(:deployable_id).map(&:deployable_id)
@@ -45,6 +47,30 @@ class Deployable < ActiveRecord::Base
PRESET_FILTERS_OPTIONS = []
+ def object_list
+ super + catalogs + catalogs.collect{|c| c.pool} + [pool_family]
+ end
+ class << self
+ alias orig_list_for_user_include list_for_user_include
+ alias orig_list_for_user_conditions list_for_user_conditions
+ end
+
+ def self.list_for_user_include
+ orig_list_for_user_include + [ {:catalogs => [:permissions,
+ {:pool => :permissions}]},
+ {:pool_family => :permissions} ]
+ end
+
+ def self.list_for_user_conditions
+ "(#{orig_list_for_user_conditions}) or
+ (permissions_catalogs.user_id=:user and
+ permissions_catalogs.role_id in (:role_ids)) or
+ (permissions_pools.user_id=:user and
+ permissions_pools.role_id in (:role_ids)) or
+ (permissions_pool_families.user_id=:user and
+ permissions_pool_families.role_id in (:role_ids))"
+ end
+
def valid_deployable_xml?
begin
deployable_xml = DeployableXML.new(xml)
@@ -133,12 +159,12 @@ class Deployable < ActiveRecord::Base
:assembly => assembly.name,
:uuid => assembly.image_id)
else
- if image.environment != catalogs.first.pool.pool_family.name
+ if image.environment != pool_family.name
deployable_errors << I18n.t("deployables.flash.error.wrong_environment",
:assembly => assembly.name,
:uuid => assembly.image_id,
:wrong_env => image.environment,
- :environment => catalogs.first.pool.pool_family.name)
+ :environment => pool_family.name)
end
images << image
assembly_hash[:build_and_target_uuids] = get_build_and_target_uuids(image)
@@ -186,6 +212,10 @@ class Deployable < ActiveRecord::Base
(latest_build ? latest_build.target_images.collect { |ti| ti.uuid} : nil)]
end
+ def set_pool_family
+ self[:pool_family_id] = catalogs.first.pool.pool_family_id
+ end
+
private
def self.apply_search_filter(search)
diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
index ec66fdb..c0c88b2 100644
--- a/src/app/models/deployment.rb
+++ b/src/app/models/deployment.rb
@@ -42,6 +42,7 @@ class Deployment < ActiveRecord::Base
end
belongs_to :pool
+ belongs_to :pool_family
has_many :instances, :dependent => :destroy
@@ -71,6 +72,8 @@ class Deployment < ActiveRecord::Base
before_create :inject_launch_parameters
before_create :generate_uuid
before_save :replace_special_characters_in_name
+ before_create :set_pool_family
+
USER_MUTABLE_ATTRS = ['name']
STATE_MIXED = "mixed"
@@ -105,7 +108,7 @@ class Deployment < ActiveRecord::Base
end
def object_list
- super << pool
+ super + [pool, pool_family]
end
class << self
alias orig_list_for_user_include list_for_user_include
@@ -113,16 +116,16 @@ class Deployment < ActiveRecord::Base
end
def self.list_for_user_include
- includes = orig_list_for_user_include
- includes << { :pool => {:permissions => {:role => :privileges}}}
- includes
+ orig_list_for_user_include + [ {:pool => :permissions},
+ {:pool_family => :permissions} ]
end
def self.list_for_user_conditions
"(#{orig_list_for_user_conditions}) or
(permissions_pools.user_id=:user and
- privileges_roles.target_type=:target_type and
- privileges_roles.action=:action)"
+ permissions_pools.role_id in (:role_ids)) or
+ (permissions_pool_families.user_id=:user and
+ permissions_pool_families.role_id in (:role_ids))"
end
def get_action_list(user=nil)
@@ -596,4 +599,9 @@ class Deployment < ActiveRecord::Base
def replace_special_characters_in_name
name.gsub!(/[^a-zA-Z0-9]+/, '-')
end
+
+ def set_pool_family
+ self[:pool_family_id] = pool.pool_family_id
+ end
+
end
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
index 9027e24..09ec599 100644
--- a/src/app/models/instance.rb
+++ b/src/app/models/instance.rb
@@ -70,6 +70,7 @@ class Instance < ActiveRecord::Base
@@per_page = 15
belongs_to :pool
+ belongs_to :pool_family
belongs_to :provider_account
belongs_to :deployment
@@ -100,6 +101,7 @@ class Instance < ActiveRecord::Base
validates_length_of :name, :maximum => 1024
before_create :generate_uuid
+ before_create :set_pool_family
STATE_NEW = "new"
STATE_PENDING = "pending"
@@ -144,7 +146,7 @@ class Instance < ActiveRecord::Base
USER_MUTABLE_ATTRS = ['name']
def object_list
- super + [pool, deployment]
+ super + [deployment, pool, pool_family]
end
class << self
alias orig_list_for_user_include list_for_user_include
@@ -152,20 +154,19 @@ class Instance < ActiveRecord::Base
end
def self.list_for_user_include
- includes = orig_list_for_user_include
- includes << { :pool => {:permissions => {:role => :privileges}},
- :deployment => {:permissions => {:role => :privileges}}}
- includes
+ orig_list_for_user_include + [ {:deployment => :permissions},
+ {:pool => :permissions},
+ {:pool_family => :permissions} ]
end
def self.list_for_user_conditions
"(#{orig_list_for_user_conditions}) or
(permissions_deployments.user_id=:user and
- privileges_roles.target_type=:target_type and
- privileges_roles.action=:action) or
+ permissions_deployments.role_id in (:role_ids)) or
(permissions_pools.user_id=:user and
- privileges_roles_2.target_type=:target_type and
- privileges_roles_2.action=:action)"
+ permissions_pools.role_id in (:role_ids)) or
+ (permissions_pool_families.user_id=:user and
+ permissions_pool_families.role_id in (:role_ids))"
end
def get_action_list(user=nil)
@@ -511,6 +512,10 @@ class Instance < ActiveRecord::Base
self[:uuid] = UUIDTools::UUID.timestamp_create.to_s
end
+ def set_pool_family
+ self[:pool_family_id] = pool.pool_family_id
+ end
+
def do_operation(user, operation)
@task = self.queue_action(user, operation)
unless @task
diff --git a/src/app/models/permissioned_object.rb b/src/app/models/permissioned_object.rb
index 36a2f25..c534261 100644
--- a/src/app/models/permissioned_object.rb
+++ b/src/app/models/permissioned_object.rb
@@ -59,20 +59,20 @@ module PermissionedObject
self.name.constantize
end
def self.list_for_user_include
- [{:permissions => {:role => :privileges}}]
+ [:permissions]
end
def self.list_for_user_conditions
"permissions.user_id=:user and
- privileges.target_type=:target_type and
- privileges.action=:action"
+ permissions.role_id in (:role_ids)"
end
def self.list_for_user(user, action, target_type=self.default_privilege_target_type)
return where("1=0") if user.nil? or action.nil? or target_type.nil?
if BasePermissionObject.general_permission_scope.has_privilege(user, action, target_type)
scoped
else
+ role_ids = Role.includes(:privileges).where("privileges.target_type" => target_type, "privileges.action" => action).collect {|r| r.id}
include_clause = self.list_for_user_include
- conditions_hash = {:user => user.id, :target_type => target_type.name, :action => action}
+ conditions_hash = {:user => user.id, :target_type => target_type.name, :action => action, :role_ids => role_ids}
conditions_str = self.list_for_user_conditions
includes(include_clause).where(conditions_str, conditions_hash)
end
diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
index be3d156..3fce49b 100644
--- a/src/app/models/pool.rb
+++ b/src/app/models/pool.rb
@@ -137,6 +137,25 @@ class Pool < ActiveRecord::Base
{:title => I18n.t("pools.preset_filters.with_stopped_instances"), :id => "with_stopped_instances", :query => includes(:deployments => :instances).where("instances.state" => "stopped")}
]
+ def object_list
+ super + [pool_family]
+ end
+ class << self
+ alias orig_list_for_user_include list_for_user_include
+ alias orig_list_for_user_conditions list_for_user_conditions
+ end
+
+ def self.list_for_user_include
+ orig_list_for_user_include + [ {:pool_family => :permissions} ]
+ end
+
+ def self.list_for_user_conditions
+ "(#{orig_list_for_user_conditions}) or
+ (permissions_pool_families.user_id=:user and
+ permissions_pool_families.role_id in (:role_ids))"
+ end
+
+
private
def self.apply_search_filter(search)
diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
index eff59f9..1e0ed69 100644
--- a/src/app/models/provider_account.rb
+++ b/src/app/models/provider_account.rb
@@ -115,16 +115,13 @@ class ProviderAccount < ActiveRecord::Base
end
def self.list_for_user_include
- includes = orig_list_for_user_include
- includes << { :provider => {:permissions => {:role => :privileges}}}
- includes
+ orig_list_for_user_include + [{ :provider => :permissions}]
end
def self.list_for_user_conditions
"(#{orig_list_for_user_conditions}) or
(permissions_providers.user_id=:user and
- privileges_roles.target_type=:target_type and
- privileges_roles.action=:action)"
+ permissions_providers.role_id in (:role_ids))"
end
def destroyable?
diff --git a/src/db/migrate/20120227150102_add_pool_family_to_instance.rb b/src/db/migrate/20120227150102_add_pool_family_to_instance.rb
new file mode 100644
index 0000000..00848ba
--- /dev/null
+++ b/src/db/migrate/20120227150102_add_pool_family_to_instance.rb
@@ -0,0 +1,32 @@
+class AddPoolFamilyToInstance < ActiveRecord::Migration
+ def self.up
+ add_column :instances, :pool_family_id, :integer
+ add_column :deployments, :pool_family_id, :integer
+ add_column :catalogs, :pool_family_id, :integer
+ add_column :deployables, :pool_family_id, :integer
+
+ Deployment.all.each do |deployment|
+ deployment.pool_family_id = deployment.pool.pool_family_id
+ deployment.save!
+ end
+ Instance.all.each do |instance|
+ instance.pool_family_id = instance.pool.pool_family_id
+ instance.save!
+ end
+ Catalog.all.each do |catalog|
+ catalog.pool_family_id = catalog.pool.pool_family_id
+ catalog.save!
+ end
+ Deployable.all.each do |deployable|
+ deployable.pool_family_id = deployable.catalogs.first.pool_family_id unless deployable.catalogs.empty?
+ deployable.save!
+ end
+ end
+
+ def self.down
+ remove_column :instances, :pool_family_id
+ remove_column :deployments, :pool_family_id
+ remove_column :catalogs, :pool_family_id
+ remove_column :deployables, :pool_family_id
+ end
+end
--
1.7.6.4
More information about the aeolus-devel
mailing list