[PATCH Conductor 1/7] bug 788148: Take environment into account for permission inheritance

[Scott Seago]

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