[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

[PATCH Conductor 2/7] bug 788148: whitespace changes for readibility

[Scott Seago]

https://bugzilla.redhat.com/show_bug.cgi?id=788148
---
 src/db/seeds.rb |  102 +++++++++++++++++++++++++++---------------------------
 1 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/src/db/seeds.rb b/src/db/seeds.rb
index 09a588b..d29f816 100644
--- a/src/db/seeds.rb
+++ b/src/db/seeds.rb
@@ -32,72 +32,72 @@ GPRM = "set_perms"
 
 roles =
   {Instance =>
-     {"instance.user"               => [false, {Instance     =>
[VIEW,USE]}],
-      "instance.owner"              => [true,  {Instance     =>
[VIEW,USE,MOD,    VPRM,GPRM]}]},
+     {"instance.user"               => [false, {Instance        =>
[VIEW,USE]}],
+      "instance.owner"              => [true,  {Instance        =>
[VIEW,USE,MOD,    VPRM,GPRM]}]},
    Deployment =>
-     {"deployment.user"             => [false, {Deployment =>
[VIEW,USE],
-                                                                          Instance  
=> [VIEW]}],
-      "deployment.owner"            => [true,  {Deployment =>
[VIEW,USE,MOD,    VPRM,GPRM],
-                                                Instance   => [VIEW,USE,MOD]}]},
+     {"deployment.user"             => [false, {Deployment      =>
[VIEW,USE],
+                                                Instance        => [VIEW]}],
+      "deployment.owner"            => [true,  {Deployment      =>
[VIEW,USE,MOD,    VPRM,GPRM],
+                                                Instance        => [VIEW,USE,MOD]}]},
    PoolFamily =>
-     {"pool_family.user"            => [false, {Pool         =>
[VIEW]}],
-      "pool_family.admin"           => [true,  {PoolFamily   => [VIEW,   
MOD,    VPRM,GPRM],
-                                                                          Pool        
=> [VIEW,    MOD,CRE,VPRM,GPRM]}]},
+     {"pool_family.user"            => [false, {Pool            =>
[VIEW]}],
+      "pool_family.admin"           => [true,  {PoolFamily      => [VIEW,
   MOD,    VPRM,GPRM],
+                                                Pool            => [VIEW,   
MOD,CRE,VPRM,GPRM]}]},
    Pool =>
-     {"pool.user"                   => [false, {Pool         => [VIEW],
-                                                                          Instance    
=> [             CRE],
-                                                                          Deployment  
=> [             CRE],
-                                                                          Quota       
=> [VIEW]}],
-      "pool.admin"                  => [true,  {Pool         => [VIEW,   
MOD,    VPRM,GPRM],
-                                                                          Instance    
=> [VIEW,USE,MOD,CRE],
-                                                                          Deployment  
=> [VIEW,USE,MOD,CRE],
-                                                                          Quota       
=> [VIEW]}]},
+     {"pool.user"                   => [false, {Pool            =>
[VIEW],
+                                                Instance        => [            
CRE],
+                                                Deployment      => [            
CRE],
+                                                Quota           => [VIEW]}],
+      "pool.admin"                  => [true,  {Pool            => [VIEW,
   MOD,    VPRM,GPRM],
+                                                Instance        =>
[VIEW,USE,MOD,CRE],
+                                                Deployment      =>
[VIEW,USE,MOD,CRE],
+                                                Quota           => [VIEW]}]},
    Provider =>
-     {"provider.admin"              => [true,  {Provider     => [VIEW,   
MOD,    VPRM,GPRM],
+     {"provider.admin"              => [true,  {Provider        => [VIEW,
   MOD,    VPRM,GPRM],
                                                 ProviderAccount =>
[VIEW,USE,MOD,CRE,VPRM,GPRM]}]},
    ProviderAccount =>
      {"provider_account.user"       => [false, {ProviderAccount =>
[VIEW,USE]}],
       "provider_account.owner"      => [true,  {ProviderAccount =>
[VIEW,USE,MOD,    VPRM,GPRM]}]},
    Catalog =>
-     {"catalog.user"                => [false, {Catalog => [VIEW,
USE]}],
-      "catalog.admin"               => [true,  {Catalog =>
[VIEW,USE,MOD,VPRM,GPRM]}]},
+     {"catalog.user"                => [false, {Catalog         => [VIEW,
USE]}],
+      "catalog.admin"               => [true,  {Catalog         =>
[VIEW,USE,MOD,    VPRM,GPRM]}]},
    Deployable =>
-     {"deployable.user"             => [false, {Deployable     =>
[VIEW,USE]}],
-      "deployable.owner"            => [true,  {Deployable     =>
[VIEW,USE,MOD,VPRM,GPRM]}]},
+     {"deployable.user"             => [false, {Deployable      =>
[VIEW,USE]}],
+      "deployable.owner"            => [true,  {Deployable      =>
[VIEW,USE,MOD,    VPRM,GPRM]}]},
    BasePermissionObject =>
-     {"base.provider.creator"       => [false, {Provider     => [        
    CRE]}],
-      "base.provider.admin"         => [false, {Provider     => [VIEW,   
MOD,CRE,VPRM,GPRM],
+     {"base.provider.creator"       => [false, {Provider        => [     
       CRE]}],
+      "base.provider.admin"         => [false, {Provider        => [VIEW,
   MOD,CRE,VPRM,GPRM],
                                                 ProviderAccount =>
[VIEW,USE,MOD,CRE,VPRM,GPRM]}],
       "base.hwp.admin"              => [false, {HardwareProfile => [VIEW,
   MOD,CRE,VPRM,GPRM]}],
-      "base.realm.admin"            => [false, {Realm        => [    
USE,MOD,CRE,VPRM,GPRM]}],
-      "base.pool.creator"           => [false, {Pool         => [        
    CRE]}],
-      "base.pool.admin"             => [false, {Pool         => [VIEW,   
MOD,CRE,VPRM,GPRM],
-                                                Instance     =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
-                                                Deployment   =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
-                                                Quota        => [VIEW,    MOD],
-                                                PoolFamily   => [VIEW,   
MOD,CRE,VPRM,GPRM]}],
-      "base.deployable.admin"       => [false, {Deployable =>
[VIEW,USE,MOD,CRE,VPRM,GPRM]}],
-      "base.deployable.user" => [false, {Deployable=> [VIEW,USE]}],
-      "base.catalog.user"    => [false, {Catalog => [VIEW,USE]}],
-      "base.hwp.user"        => [false, {HardwareProfile =>
[VIEW,USE]}],
-      "base.pool.user"       => [false, {Pool         => [VIEW],
-                                                Instance     => [             CRE],
-                                                Deployment   => [             CRE],
-                                                Quota        => [VIEW]}],
-      "base.image.admin"            => [false, {PoolFamily   => [VIEW,
USE] }],
-      "base.admin"                  => [false, {Provider     => [VIEW,   
MOD,CRE,VPRM,GPRM],
+      "base.realm.admin"            => [false, {Realm           => [    
USE,MOD,CRE,VPRM,GPRM]}],
+      "base.pool.creator"           => [false, {Pool            => [     
       CRE]}],
+      "base.pool.admin"             => [false, {Pool            => [VIEW,
   MOD,CRE,VPRM,GPRM],
+                                                Instance        =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Deployment      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Quota           => [VIEW,    MOD],
+                                                PoolFamily      => [VIEW,   
MOD,CRE,VPRM,GPRM]}],
+      "base.deployable.admin"       => [false, {Deployable      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM]}],
+      "base.deployable.user"        => [false, {Deployable      =>
[VIEW,USE]}],
+      "base.catalog.user"           => [false, {Catalog         =>
[VIEW,USE]}],
+      "base.hwp.user"               => [false, {HardwareProfile =>
[VIEW,USE]}],
+      "base.pool.user"              => [false, {Pool            =>
[VIEW],
+                                                Instance        => [            
CRE],
+                                                Deployment      => [            
CRE],
+                                                Quota           => [VIEW]}],
+      "base.image.admin"            => [false, {PoolFamily      => [VIEW,
USE] }],
+      "base.admin"                  => [false, {Provider        => [VIEW,
   MOD,CRE,VPRM,GPRM],
                                                 ProviderAccount =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
                                                 HardwareProfile => [VIEW,   
MOD,CRE,VPRM,GPRM],
-                                                Realm        => [    
USE,MOD,CRE,VPRM,GPRM],
-                                                User         => [VIEW,    MOD,CRE],
-                                                Pool         => [VIEW,   
MOD,CRE,VPRM,GPRM],
-                                                Instance     =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
-                                                Deployment   =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
-                                                Quota        => [VIEW,    MOD],
-                                                PoolFamily   =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
-                                                Catalog      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
-                                                Deployable =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
-                                                BasePermissionObject => [ MOD,   
VPRM,GPRM]}]}}
+                                                Realm           => [    
USE,MOD,CRE,VPRM,GPRM],
+                                                User            => [VIEW,   
MOD,CRE],
+                                                Pool            => [VIEW,   
MOD,CRE,VPRM,GPRM],
+                                                Instance        =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Deployment      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Quota           => [VIEW,    MOD],
+                                                PoolFamily      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Catalog         =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Deployable      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                BasePermissionObject    => [ MOD,   
VPRM,GPRM]}]}}
 Role.transaction do
   roles.each do |role_scope, scoped_hash|
     scoped_hash.each do |role_name, role_def|
-- 
1.7.6.4

[PATCH Conductor 3/7] bug 788148: seeds.rb update to update role definitions https://bugzilla.redhat.com/show_bug.cgi?id=788148

[Scott Seago]

---
 src/db/seeds.rb |   38 +++++++++++++++++++++++++++++++-------
 1 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/src/db/seeds.rb b/src/db/seeds.rb
index d29f816..3a3469d 100644
--- a/src/db/seeds.rb
+++ b/src/db/seeds.rb
@@ -40,17 +40,35 @@ roles =
       "deployment.owner"            => [true,  {Deployment      =>
[VIEW,USE,MOD,    VPRM,GPRM],
                                                 Instance        => [VIEW,USE,MOD]}]},
    PoolFamily =>
-     {"pool_family.user"            => [false, {Pool            =>
[VIEW]}],
-      "pool_family.admin"           => [true,  {PoolFamily      => [VIEW,
   MOD,    VPRM,GPRM],
-                                                Pool            => [VIEW,   
MOD,CRE,VPRM,GPRM]}]},
+     {"pool_family.user"            => [false, {PoolFamily      =>
[VIEW]}],
+      "pool_family.image.admin"     => [false, {PoolFamily      =>
[VIEW,USE],
+                                                Pool            => [VIEW],
+                                                Catalog         =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Deployable      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Quota           => [VIEW]}],
+      "pool_family.admin"           => [true,  {PoolFamily      =>
[VIEW,USE,MOD,    VPRM,GPRM],
+                                                Pool            => [VIEW,   
MOD,CRE,VPRM,GPRM],
+                                                Instance        =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Deployment      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Catalog         =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Deployable      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Quota           => [VIEW]}]},
    Pool =>
      {"pool.user"                   => [false, {Pool            =>
[VIEW],
                                                 Instance        => [            
CRE],
                                                 Deployment      => [            
CRE],
+                                                Catalog         => [VIEW, USE],
+                                                Deployable      => [VIEW,USE],
+                                                Quota           => [VIEW]}],
+      "pool.deployable.admin"       => [false, {Pool            =>
[VIEW],
+                                                Catalog         =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Deployable      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
                                                 Quota           => [VIEW]}],
       "pool.admin"                  => [true,  {Pool            => [VIEW,
   MOD,    VPRM,GPRM],
                                                 Instance        =>
[VIEW,USE,MOD,CRE],
                                                 Deployment      =>
[VIEW,USE,MOD,CRE],
+                                                Catalog         =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Deployable      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
                                                 Quota           => [VIEW]}]},
    Provider =>
      {"provider.admin"              => [true,  {Provider        => [VIEW,
   MOD,    VPRM,GPRM],
@@ -59,8 +77,10 @@ roles =
      {"provider_account.user"       => [false, {ProviderAccount =>
[VIEW,USE]}],
       "provider_account.owner"      => [true,  {ProviderAccount =>
[VIEW,USE,MOD,    VPRM,GPRM]}]},
    Catalog =>
-     {"catalog.user"                => [false, {Catalog         => [VIEW,
USE]}],
-      "catalog.admin"               => [true,  {Catalog         =>
[VIEW,USE,MOD,    VPRM,GPRM]}]},
+     {"catalog.user"                => [false, {Catalog         => [VIEW,
USE],
+                                                Deployable      => [VIEW,USE]}],
+      "catalog.admin"               => [true,  {Catalog         =>
[VIEW,USE,MOD,    VPRM,GPRM],
+                                                Deployable      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM]}]},
    Deployable =>
      {"deployable.user"             => [false, {Deployable      =>
[VIEW,USE]}],
       "deployable.owner"            => [true,  {Deployable      =>
[VIEW,USE,MOD,    VPRM,GPRM]}]},
@@ -74,8 +94,10 @@ roles =
       "base.pool.admin"             => [false, {Pool            => [VIEW,
   MOD,CRE,VPRM,GPRM],
                                                 Instance        =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
                                                 Deployment      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Catalog         =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Deployable      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
                                                 Quota           => [VIEW,    MOD],
-                                                PoolFamily      => [VIEW,   
MOD,CRE,VPRM,GPRM]}],
+                                                PoolFamily      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM]}],
       "base.deployable.admin"       => [false, {Deployable      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM]}],
       "base.deployable.user"        => [false, {Deployable      =>
[VIEW,USE]}],
       "base.catalog.user"           => [false, {Catalog         =>
[VIEW,USE]}],
@@ -84,7 +106,9 @@ roles =
                                                 Instance        => [            
CRE],
                                                 Deployment      => [            
CRE],
                                                 Quota           => [VIEW]}],
-      "base.image.admin"            => [false, {PoolFamily      => [VIEW,
USE] }],
+      "base.image.admin"            => [false, {PoolFamily      => [VIEW,
USE],
+                                                Catalog         =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
+                                                Deployable      =>
[VIEW,USE,MOD,CRE,VPRM,GPRM]}],
       "base.admin"                  => [false, {Provider        => [VIEW,
   MOD,CRE,VPRM,GPRM],
                                                 ProviderAccount =>
[VIEW,USE,MOD,CRE,VPRM,GPRM],
                                                 HardwareProfile => [VIEW,   
MOD,CRE,VPRM,GPRM],
-- 
1.7.6.4

[PATCH Conductor 4/7] bug 788148: Add permissions pane to provider accounts UI

[Scott Seago]

https://bugzilla.redhat.com/show_bug.cgi?id=788148
---
 .../controllers/provider_accounts_controller.rb    |    1 +
 src/app/models/provider_account.rb                 |    5 +++++
 src/app/views/provider_accounts/show.html.haml     |    7 +++++++
 3 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/src/app/controllers/provider_accounts_controller.rb
b/src/app/controllers/provider_accounts_controller.rb
index f9f5f2d..b993e33 100644
--- a/src/app/controllers/provider_accounts_controller.rb
+++ b/src/app/controllers/provider_accounts_controller.rb
@@ -44,6 +44,7 @@ class ProviderAccountsController < ApplicationController
       test_account(@provider_account)
       render :action => 'show' and return
     end
+    add_permissions_inline(@provider_account)
 
     respond_to do |format|
       format.html { render :action => 'show'}
diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
index 1e0ed69..cfdb787 100644
--- a/src/app/models/provider_account.rb
+++ b/src/app/models/provider_account.rb
@@ -404,6 +404,11 @@ class ProviderAccount < ActiveRecord::Base
     :pushed
   end
 
+  def to_polymorphic_path_param(polymorphic_path_extras)
+    [provider, self]
+  end
+
+
   private
 
   def self.apply_search_filter(search)
diff --git a/src/app/views/provider_accounts/show.html.haml
b/src/app/views/provider_accounts/show.html.haml
index 63dcc72..5c2366c 100644
--- a/src/app/views/provider_accounts/show.html.haml
+++ b/src/app/views/provider_accounts/show.html.haml
@@ -22,3 +22,10 @@
 
   .content
     = render :partial => 'provider_realms/list'
+
+- if check_privilege(Privilege::PERM_VIEW,@permission_object)
+  %section.admin-content-section
+    %header
+      %h2.roles= t('role_assignments')
+    .content
+      = render :partial => 'permissions/permissions'
-- 
1.7.6.4

[PATCH Conductor 5/7] bug 788148: Removing failing cucumber test for obsolete UI.

[Scott Seago]

This whole test should probably be removed (we don't expose the roles UI anymore), but
for now this is the minimum change to get tests working.

https://bugzilla.redhat.com/show_bug.cgi?id=788148
---
 src/features/role.feature |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/src/features/role.feature b/src/features/role.feature
index abd0157..bb83e2e 100644
--- a/src/features/role.feature
+++ b/src/features/role.feature
@@ -25,12 +25,6 @@ Feature: Manage Roles
     When I follow "Admiral"
     Then I should be on Admiral's role page
 
-  Scenario: Show default role privileges
-    Given I am on the roles page
-    And there should be a role named "Administrator"
-    When I follow "Administrator"
-    Then I should be on Administrator's role page
-
   Scenario: Delete roles
     Given a role "Admiral" exists
     And I am on the roles page
-- 
1.7.6.4

[PATCH Conductor 5/7] bug 788148: Removing failing cucumber test for obsolete UI.

[Scott Seago]

On 02/29/2012 02:43 AM, Scott Seago wrote:
>  This whole test should probably be removed (we don't expose the
> roles UI anymore), but for now this is the minimum change to get tests working.
>
>  https://bugzilla.redhat.com/show_bug.cgi?id=788148
>  ---
>    src/features/role.feature |    6 ------
>    1 files changed, 0 insertions(+), 6 deletions(-)
>
>  diff --git a/src/features/role.feature b/src/features/role.feature
>  index abd0157..bb83e2e 100644
>  --- a/src/features/role.feature
>  +++ b/src/features/role.feature
>  @@ -25,12 +25,6 @@ Feature: Manage Roles
>        When I follow "Admiral"
>        Then I should be on Admiral's role page
>
>  -  Scenario: Show default role privileges
>  -    Given I am on the roles page
>  -    And there should be a role named "Administrator"
>  -    When I follow "Administrator"
>  -    Then I should be on Administrator's role page
>  -
>      Scenario: Delete roles
>        Given a role "Admiral" exists
>        And I am on the roles page 
Self-ACK and pushed. It was breaking cucumber
tests for everyone else 
too, not just in my patchset.

I'm re-sending the rebased patchset for everything else now that this is 
pushed.

Scott

[PATCH Conductor 6/7] bug 788148: some permissions checking fixes to enable proper 'environment admin' functionality

[Scott Seago]

https://bugzilla.redhat.com/show_bug.cgi?id=788148
---
 src/app/controllers/catalogs_controller.rb         |   13 +--
 src/app/controllers/deployables_controller.rb      |   27 ++++--
 src/app/controllers/images_controller.rb           |   22 ++++-
 src/app/controllers/pool_families_controller.rb    |    2 +-
 src/app/controllers/pools_controller.rb            |    6 +-
 src/app/models/deployable.rb                       |    2 +-
 src/app/views/catalogs/_list.html.haml             |    2 +-
 src/app/views/images/show.html.haml                |   97 ++++++++++++--------
 src/app/views/pools/_form.html.haml                |    5 +-
 src/features/pool_family.feature                   |    4 +-
 src/features/step_definitions/deployable_steps.rb  |    3 +-
 src/features/step_definitions/pool_steps.rb        |   10 +--
 .../controllers/deployables_controller_spec.rb     |    5 +-
 .../controllers/deployments_controller_spec.rb     |    5 +-
 .../controllers/permissions_controller_spec.rb     |    3 +-
 src/spec/factories/catalog.rb                      |    3 +-
 16 files changed, 122 insertions(+), 87 deletions(-)

diff --git a/src/app/controllers/catalogs_controller.rb
b/src/app/controllers/catalogs_controller.rb
index 2848288..97053ce 100644
--- a/src/app/controllers/catalogs_controller.rb
+++ b/src/app/controllers/catalogs_controller.rb
@@ -21,6 +21,7 @@ class CatalogsController < ApplicationController
     @title = t('catalogs.catalogs')
     clear_breadcrumbs
     @catalogs = Catalog.apply_filters(:preset_filter_id =>
params[:catalogs_preset_filter], :search_filter =>
params[:catalogs_search]).list_for_user(current_user, Privilege::VIEW)
+    @can_create = Pool.list_for_user(current_user, Privilege::CREATE, Catalog).present?
     save_breadcrumb(catalogs_path(:viewstate => @viewstate ? @viewstate.id : nil))
     set_header
     set_admin_content_tabs 'catalogs'
@@ -31,10 +32,10 @@ class CatalogsController < ApplicationController
   end
 
   def new
-    require_privilege(Privilege::CREATE, Catalog)
+    load_pools
+    require_privilege(Privilege::CREATE, Catalog, @pools.first)
     @catalog = Catalog.new(params[:catalog]) # ...when should there be params on new?
     @title = t'catalogs.new.add_catalog'
-    load_pools
   end
 
   def show
@@ -51,14 +52,12 @@ class CatalogsController < ApplicationController
   end
 
   def create
-    require_privilege(Privilege::CREATE, Catalog)
     @catalog = Catalog.new(params[:catalog])
-    require_privilege(Privilege::MODIFY, @catalog.pool)
+    require_privilege(Privilege::CREATE, Catalog, @catalog.pool)
     if @catalog.save
       flash[:notice] = t('catalogs.flash.notice.created', :count => 1)
       redirect_to catalogs_path and return
     else
-      load_pools
       render :new and return
     end
   end
@@ -73,7 +72,7 @@ class CatalogsController < ApplicationController
   def update
     @catalog = Catalog.find(params[:id])
     require_privilege(Privilege::MODIFY, @catalog)
-    require_privilege(Privilege::MODIFY, @catalog.pool)
+    require_privilege(Privilege::CREATE, Catalog, @catalog.pool)
 
     if @catalog.update_attributes(params[:catalog])
       flash[:notice] = t('catalogs.flash.notice.updated', :count => 1)
@@ -136,6 +135,6 @@ class CatalogsController < ApplicationController
   end
 
   def load_pools
-    @pools = Pool.list_for_user(current_user, Privilege::MODIFY)
+    @pools = Pool.list_for_user(current_user, Privilege::CREATE, Catalog)
   end
 end
diff --git a/src/app/controllers/deployables_controller.rb
b/src/app/controllers/deployables_controller.rb
index e37bc4a..e6b8fe4 100644
--- a/src/app/controllers/deployables_controller.rb
+++ b/src/app/controllers/deployables_controller.rb
@@ -34,13 +34,15 @@ class DeployablesController < ApplicationController
 
   def new
     @deployable = Deployable.new(params[:deployable])
-    require_privilege(Privilege::CREATE, Deployable)
     if params[:create_from_image]
       @image = Aeolus::Image::Warehouse::Image.find(params[:create_from_image])
       @hw_profiles = HardwareProfile.frontend.list_for_user(current_user,
Privilege::VIEW)
       @deployable.name = @image.name
       @selected_catalogs = params[:catalog_id].to_a
       load_catalogs
+      @selected_catalogs.each do |catalog_id|
+        require_privilege(Privilege::CREATE, Deployable, Catalog.find_by_id(catalog_id))
+      end
       flasherr = []
       flasherr << t("deployables.flash.error.no_catalog_exists") if
@catalogs.empty?
       flasherr << t("deployables.flash.error.no_hwp_exists") if
@hw_profiles.empty?
@@ -48,7 +50,7 @@ class DeployablesController < ApplicationController
       flash[:error] = flasherr if not flasherr.empty?
     elsif params[:catalog_id].present?
       @catalog = Catalog.find(params[:catalog_id])
-      require_privilege(Privilege::MODIFY, @catalog)
+      require_privilege(Privilege::CREATE, @catalog, Deployable)
     end
     @form_option= params.has_key?(:from_url) ? 'from_url' : 'upload'
     respond_to do |format|
@@ -65,7 +67,7 @@ class DeployablesController < ApplicationController
     @providers = Provider.all
     @catalogs_options = Catalog.list_for_user(current_user, Privilege::VIEW).select do
|c|
       !(a)deployable.catalogs.include?(c) and
-        @deployable.catalogs.first.pool.pool_family == c.pool.pool_family
+        @deployable.catalogs.first.pool_family == c.pool_family
     end
 
     if @catalog.present?
@@ -81,7 +83,7 @@ class DeployablesController < ApplicationController
 
     @build_results = {}
     @pushed_count = 0
-    ProviderAccount.includes(:provider, :pool_families).where('providers.enabled'
=> true, 'pool_families.id' =>
@deployable.catalogs.first.pool.pool_family.id).each do |account|
+    ProviderAccount.includes(:provider, :pool_families).where('providers.enabled'
=> true, 'pool_families.id' =>
@deployable.catalogs.first.pool_family.id).each do |account|
       type = account.provider.provider_type.deltacloud_driver
       @build_results[type] ||= []
       status = @deployable.build_status(images, account)
@@ -114,10 +116,12 @@ class DeployablesController < ApplicationController
       return
     end
 
-    require_privilege(Privilege::CREATE, Deployable)
     @deployable = Deployable.new(params[:deployable])
     @selected_catalogs = Catalog.find(params[:catalog_id].to_a)
     @deployable.owner = current_user
+    @selected_catalogs.each do |catalog|
+      require_privilege(Privilege::CREATE, Deployable, catalog)
+    end
 
     if params.has_key? :url
         xml = import_xml_from_url(params[:url])
@@ -135,11 +139,10 @@ class DeployablesController < ApplicationController
     begin
       raise t("deployables.flash.error.no_catalog") if
@selected_catalogs.empty?
       @deployable.transaction do
-        @deployable.save!
         @selected_catalogs.each do |catalog|
-          require_privilege(Privilege::MODIFY, catalog)
-          CatalogEntry.create!(:catalog_id => catalog.id, :deployable_id =>
@deployable.id)
+          @deployable.catalogs << catalog
         end
+        @deployable.save!
         flash[:notice] = t("catalog_entries.flash.notice.added", :catalog =>
@selected_catalogs.map{|c| c.name}.join(", "))
         if params[:edit_xml]
           redirect_to edit_polymorphic_path([(a)selected_catalogs.first, @deployable],
:edit_xml =>true)
@@ -150,7 +153,11 @@ class DeployablesController < ApplicationController
         end
       end
     rescue => e
-      flash.now[:warning]= t('deployables.flash.warning.failed', :message =>
e.message) if @deployable.errors.empty?
+      if @deployable.errors.empty?
+        logger.error e.message
+        logger.error e.backtrace.join("\n ")
+        flash.now[:warning]= t('deployables.flash.warning.failed', :message =>
e.message)
+      end
       if params[:create_from_image].present?
         @image = Aeolus::Image::Warehouse::Image.find(params[:create_from_image])
         load_catalogs
@@ -257,7 +264,7 @@ class DeployablesController < ApplicationController
 
   def load_catalogs
     @pool_family = PoolFamily.where(:name => @image.environment).first
-    @catalogs = Catalog.includes(:pool).list_for_user(current_user,
Privilege::MODIFY).where('pools.pool_family_id' => @pool_family.id)
+    @catalogs = Catalog.list_for_user(current_user, Privilege::CREATE,
Deployable).where('pool_family_id' => @pool_family.id)
   end
 
   def import_xml_from_url(url)
diff --git a/src/app/controllers/images_controller.rb
b/src/app/controllers/images_controller.rb
index dcca913..009b458 100644
--- a/src/app/controllers/images_controller.rb
+++ b/src/app/controllers/images_controller.rb
@@ -16,7 +16,6 @@
 
 class ImagesController < ApplicationController
   before_filter :require_user
-  before_filter :check_permissions, :except => [:index, :show]
 
   def index
     set_admin_environments_tabs 'images'
@@ -117,7 +116,10 @@ class ImagesController < ApplicationController
                           :active_pushes => active_pushes,
                           :failed_build_counts => failed_build_counts,
                           :failed_push_counts => failed_push_counts,
-                          :latest_build_id => @latest_build }
+                          :latest_build_id => @latest_build,
+                          :user_can_build => (@environment and
+                                              check_privilege(Privilege::USE,
+                                                              @environment))}
       end
     end
   end
@@ -125,6 +127,7 @@ class ImagesController < ApplicationController
   def rebuild_all
     @image = Aeolus::Image::Warehouse::Image.find(params[:id])
     @environment = PoolFamily.where('name' => @image.environment).first
+    check_permissions
     targets = @environment.build_targets
     unless targets.empty?
       factory_image = Aeolus::Image::Factory::Image.new(:id => @image.id)
@@ -138,6 +141,7 @@ class ImagesController < ApplicationController
   def push_all
     @image = Aeolus::Image::Warehouse::Image.find(params[:id])
     @environment = PoolFamily.where('name' => @image.environment).first
+    check_permissions
     @build = Aeolus::Image::Warehouse::ImageBuild.find(params[:build_id])
     # only latest builds can be pushed
     unless latest_build?(@build)
@@ -165,6 +169,8 @@ class ImagesController < ApplicationController
 
   def template
     image = Aeolus::Image::Warehouse::Image.find(params[:id])
+    @environment = PoolFamily.where('name' => image.environment).first
+    check_permissions
     template = Aeolus::Image::Warehouse::Template.find(image.template)
     if template
       render :xml => template.body
@@ -176,6 +182,7 @@ class ImagesController < ApplicationController
 
   def new
     @environment = PoolFamily.find(params[:environment])
+    check_permissions
     if 'import' == params[:tab]
       @accounts = @environment.provider_accounts.enabled.list_for_user(current_user,
Privilege::USE)
       render :import and return
@@ -186,6 +193,7 @@ class ImagesController < ApplicationController
   def import
     account = ProviderAccount.find(params[:provider_account])
     @environment = PoolFamily.find(params[:environment])
+    check_permissions
 
     xml =
"<image><name>#{params[:name]}</name></image>" unless
params[:name].blank?
     begin
@@ -207,6 +215,7 @@ class ImagesController < ApplicationController
 
   def edit_xml
     @environment = PoolFamily.find(params[:environment])
+    check_permissions
     @name = params[:name]
 
     if params.has_key? :image_url
@@ -247,6 +256,7 @@ class ImagesController < ApplicationController
 
   def overview
     @environment = PoolFamily.find(params[:environment])
+    check_permissions
     @name = params[:name]
     @xml = params[:image_xml]
 
@@ -267,6 +277,7 @@ class ImagesController < ApplicationController
 
   def create
     @environment = PoolFamily.find(params[:environment])
+    check_permissions
     @name = params[:name]
     @xml = params[:image_xml]
 
@@ -298,13 +309,17 @@ class ImagesController < ApplicationController
   end
 
   def edit
+    check_permissions
   end
 
   def update
+    check_permissions
   end
 
   def destroy
     if image = Aeolus::Image::Warehouse::Image.find(params[:id])
+      @environment = PoolFamily.where('name' => image.environment).first
+      check_permissions
       if image.delete!
         flash[:notice] = t('images.flash.notice.deleted')
       else
@@ -320,6 +335,7 @@ class ImagesController < ApplicationController
     selected_images = params[:images_selected].to_a
     selected_images.each do |uuid|
       image = Aeolus::Image::Warehouse::Image.find(uuid)
+      @environment = PoolFamily.where('name' => image.environment).first
       image.delete!
     end
     redirect_to images_path, :notice =>
t("images.flash.notice.multiple_deleted", :count => selected_images.count)
@@ -347,7 +363,7 @@ class ImagesController < ApplicationController
 
   # For now, Image permissions hijack the previously-unused PoolFamily USE privilege
   def check_permissions
-    require_privilege(Privilege::USE, PoolFamily)
+    require_privilege(Privilege::USE, @environment)
   end
 
   def latest_build?(build)
diff --git a/src/app/controllers/pool_families_controller.rb
b/src/app/controllers/pool_families_controller.rb
index fbe5ec6..0a4f2d5 100644
--- a/src/app/controllers/pool_families_controller.rb
+++ b/src/app/controllers/pool_families_controller.rb
@@ -114,7 +114,7 @@ class PoolFamiliesController < ApplicationController
 
     @provider_accounts = ProviderAccount.
       list_for_user(current_user, Privilege::USE).
-      where('id not in (?)', @pool_family.provider_accounts.empty? ?
+      where('provider_accounts.id not in (?)',
@pool_family.provider_accounts.empty? ?
                                0 : @pool_family.provider_accounts.map(&:id))
 
     load_tab_captions_and_details_tab('provider_accounts')
diff --git a/src/app/controllers/pools_controller.rb
b/src/app/controllers/pools_controller.rb
index c639b57..6218e73 100644
--- a/src/app/controllers/pools_controller.rb
+++ b/src/app/controllers/pools_controller.rb
@@ -110,11 +110,12 @@ class PoolsController < ApplicationController
   end
 
   def new
-    require_privilege(Privilege::CREATE, Pool)
     @title = t('pools.create_new_pool')
     @pool = Pool.new
     @pool.pool_family = PoolFamily.find(params[:pool_family_id]) unless
params[:pool_family_id].blank?
+    require_privilege(Privilege::CREATE, Pool, @pool.pool_family)
     @quota = Quota.new
+    @pool_families = PoolFamily.list_for_user(current_user, Privilege::CREATE, Pool)
     respond_to do |format|
       format.html
       format.json { render :json => @pool }
@@ -123,9 +124,8 @@ class PoolsController < ApplicationController
   end
 
   def create
-    require_privilege(Privilege::CREATE, Pool)
-
     @pool = Pool.new(params[:pool])
+    require_privilege(Privilege::CREATE, Pool, @pool.pool_family)
     @pool.quota = @quota = Quota.new
     set_quota
 
diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
index ad537a7..a53bce6 100644
--- a/src/app/models/deployable.rb
+++ b/src/app/models/deployable.rb
@@ -213,7 +213,7 @@ class Deployable < ActiveRecord::Base
   end
 
   def set_pool_family
-    self[:pool_family_id] = catalogs.first.pool.pool_family_id
+    self[:pool_family_id] = catalogs.first.pool_family_id
   end
 
   private
diff --git a/src/app/views/catalogs/_list.html.haml
b/src/app/views/catalogs/_list.html.haml
index 4a8a462..f13c9b0 100644
--- a/src/app/views/catalogs/_list.html.haml
+++ b/src/app/views/catalogs/_list.html.haml
@@ -1,6 +1,6 @@
 - content_for :form_header do
   %li= restful_submit_tag t("delete"), "destroy",
multi_destroy_catalogs_path, 'DELETE', :id => 'delete_button', :class
=> 'button danger'
-  - if check_privilege(Privilege::CREATE, Catalog)
+  - if @can_create
     %li= link_to t("catalogs.new_catalog"), new_catalog_path, :id =>
'new_catalog_button', :class => 'button'
 
 - content_for :filter_controls do
diff --git a/src/app/views/images/show.html.haml b/src/app/views/images/show.html.haml
index 7681cee..b8741b2 100644
--- a/src/app/views/images/show.html.haml
+++ b/src/app/views/images/show.html.haml
@@ -5,14 +5,15 @@
     .return_to
       =t'return_to'
       = link_to t('images.index.images'), images_path
-    - if check_privilege(Privilege::USE, PoolFamily)
+    - if @environment and check_privilege(Privilege::USE, @environment)
       .button-group
-        - if @environment
-          = link_to t('.new_deployable_from_image'),
new_deployable_path(:create_from_image => @image.id), :class => 'button
pill'
+        = link_to t('.new_deployable_from_image'),
new_deployable_path(:create_from_image => @image.id), :class => 'button
pill'
         - unless @image.imported?
           = link_to t('.template_xml'), template_image_path((a)image.uuid), :class
=> 'button pill'
         = button_to t("delete"), image_path((a)image.id), :method =>
'delete', :confirm => "Are you sure you want to delete?", :class
=> 'button pill danger', :id => 'delete'
 
+- user_can_build = @environment and check_privilege(Privilege::USE, @environment)
+
 %section.admin-content-section
   %header
     %h2=t'properties'
@@ -29,10 +30,11 @@
     .section-controls
       - if @builds.any?
         = t'.view_build'
-        = form_tag image_path((a)image.id), :method => :get do
-          = select_tag :build, options_for_build_select(@builds, @build, @latest_build)
-          = submit_tag t('.select_build'), :id =>
'select_build_button'
-      - if check_privilege(Privilege::USE, PoolFamily) and @environment
+        - if user_can_build
+          = form_tag image_path((a)image.id), :method => :get do
+            = select_tag :build, options_for_build_select(@builds, @build,
@latest_build)
+            = submit_tag t('.select_build'), :id =>
'select_build_button'
+      - if user_can_build
         - if @image.imported?
           = t('.can_not_build_imported_image')
         -else
@@ -58,15 +60,18 @@
                   - if @build and b = @builder.find_active_build((a)build.id, driver)
                     = label_tag b.status
                   - elsif timg
-                    = button_to t('.delete'), image_target_image_path((a)image.id,
timg.id), :method => :delete
+                    - if user_can_build
+                      = button_to t('.delete'),
image_target_image_path((a)image.id, timg.id), :method => :delete
                   - elsif @build and @build.id == @latest_build
-                    = button_to t('.build'), image_target_images_path((a)image.id,
:target => driver, :build_id => @build ? @build.id : nil), :method => :post
+                    - if user_can_build
+                      = button_to t('.build'),
image_target_images_path((a)image.id, :target => driver, :build_id => @build ?
@build.id : nil), :method => :post
                     - failed_build_count = @builder.failed_build_count((a)build.id,
driver)
                     - if failed_build_count > 0
                       .light
                         = t('images.show.failed_build_attempts', :count =>
failed_build_count)
                   - elsif !@build
-                    = button_to t('.build'), image_target_images_path((a)image.id,
:target => driver, :build_id => @build ? @build.id : nil), :method => :post
+                    - if user_can_build
+                      = button_to t('.build'),
image_target_images_path((a)image.id, :target => driver, :build_id => @build ?
@build.id : nil), :method => :post
                   - else
                     &nbsp;
               %dd
@@ -103,12 +108,14 @@
                                 .light
                                   = t('images.show.push_started')
                               - else
-                                = button_to t('.push'),
image_provider_images_path((a)image.id, :build_id => @build.id, :target_image_id =>
timg.id, :account_id => account.id), :method => :post, :class =>
'upload_image button'
+                                - if user_can_build
+                                  = button_to t('.push'),
image_provider_images_path((a)image.id, :build_id => @build.id, :target_image_id =>
timg.id, :account_id => account.id), :method => :post, :class =>
'upload_image button'
                                 - failed_push_count = @builder.failed_push_count(timg.id,
account.provider.name, account.credentials_hash['username'])
                                 - if failed_push_count > 0
                                   = t('images.show.failed_push_attempts', :count
=> failed_push_count)
                             - elsif pimg
-                              = button_to "",
image_provider_image_path((a)image.id, pimg.id), :method => :delete, :class =>
'delete_image'
+                              - if user_can_build
+                                = button_to "",
image_provider_image_path((a)image.id, pimg.id), :method => :delete, :class =>
'delete_image'
                             - else
                               &nbsp;
             .clear
@@ -133,16 +140,20 @@
                 {{if build && active_builds[driver]}}
                  
<label>${active_builds[driver]['builder']['status']}</label>
                 {{else target_images_by_target[driver]}}
-                  #{path = image_target_image_path('replace_image_id',
'replace_target_image_id')
-                    path = path.sub('replace_image_id',
'${image[\'uuid\']}')
-                    path = path.sub('replace_target_image_id',
'${target_images_by_target[driver][\'uuid\']}')
-                    button_to(t('images.show.delete'), path, :method =>
:delete)}
+                  {{if user_can_build}}
+                    #{path = image_target_image_path('replace_image_id',
'replace_target_image_id')
+                      path = path.sub('replace_image_id',
'${image[\'uuid\']}')
+                      path = path.sub('replace_target_image_id',
'${target_images_by_target[driver][\'uuid\']}')
+                        button_to(t('images.show.delete'), path, :method =>
:delete)}
+                  {{/if}}
                 {{else build && build['uuid'] == latest_build_id}}
-                  #{path = image_target_images_path('replace_image_id', :target
=> 'replace_driver', :build_id => 'replace_build_id')
-                    path = path.sub('replace_image_id',
'${image[\'uuid\']}')
-                    path = path.sub('replace_driver', '${driver}')
-                    path = path.sub('replace_build_id',
'${build[\'uuid\']}')
-                    button_to(t('images.show.build'), path, :method =>
:post)}
+                  {{if user_can_build}}
+                    #{path = image_target_images_path('replace_image_id', :target
=> 'replace_driver', :build_id => 'replace_build_id')
+                      path = path.sub('replace_image_id',
'${image[\'uuid\']}')
+                      path = path.sub('replace_driver', '${driver}')
+                      path = path.sub('replace_build_id',
'${build[\'uuid\']}')
+                        button_to(t('images.show.build'), path, :method =>
:post)}
+                  {{/if}}
                   {{if failed_build_counts[driver] > 1}}
                     <div class="light">
                       #{t('images.show.failed_build_attempts', :count =>
'${failed_build_counts[driver]}')}
@@ -153,10 +164,12 @@
                     </div>
                   {{/if}}
                 {{else !build}}
-                  #{path = image_target_images_path('replace_image_id', :target
=> 'replace_driver')
-                    path = path.sub('replace_image_id',
'${image[\'uuid\']}')
-                    path = path.sub('replace_driver', '${driver}')
-                    button_to(t('images.show.build'), path, :method =>
:post)}
+                  {{if user_can_build}}
+                    #{path = image_target_images_path('replace_image_id', :target
=> 'replace_driver')
+                      path = path.sub('replace_image_id',
'${image[\'uuid\']}')
+                      path = path.sub('replace_driver', '${driver}')
+                        button_to(t('images.show.build'), path, :method =>
:post)}
+                  {{/if}}
                 {{else}}
                   &nbsp;
                 {{/if}}
@@ -205,22 +218,26 @@
                           {{if target_images_by_target[driver] &&
active_pushes[account_hash['account']['id']]}}
                            
<label>${active_pushes[account_hash['account']['id']]['builder']['status']}</label>
                           {{else target_images_by_target[driver] &&
!provider_images[account_hash['account']['id']] && build
&&  build['uuid'] == latest_build_id}}
-                            #{path =
image_provider_images_path('replace_image_id', :build_id =>
'replace_build_id', :target_image_id => 'replace_target_image_id',
:account_id => 'replace_account_id')
-                              path = path.sub('replace_image_id',
'${image[\'uuid\']}')
-                              path = path.sub('replace_build_id',
'${build[\'uuid\']}')
-                              path = path.sub('replace_target_image_id',
'${target_images_by_target[driver][\'uuid\']}')
-                              path = path.sub('replace_account_id',
'${account_hash[\'account\'][\'id\']}')
-                              button_to(t('images.show.push'), path, :method
=> :post, :class => 'upload_image button')}
-                              {{if
failed_push_counts[account_hash['account']['id']] > 1}}
-                                #{t('images.show.failed_push_attempts', :count
=> '${failed_push_counts[account_hash[\'account\'][\'id\']]}'
)}
-                              {{else
failed_push_counts[account_hash['account']['id']] == 1}}
-                                #{t('images.show.failed_push_attempts', :count
=> 1 )}
-                              {{/if}}
+                            {{if user_can_build}}
+                              #{path =
image_provider_images_path('replace_image_id', :build_id =>
'replace_build_id', :target_image_id => 'replace_target_image_id',
:account_id => 'replace_account_id')
+                                path = path.sub('replace_image_id',
'${image[\'uuid\']}')
+                                path = path.sub('replace_build_id',
'${build[\'uuid\']}')
+                                path = path.sub('replace_target_image_id',
'${target_images_by_target[driver][\'uuid\']}')
+                                path = path.sub('replace_account_id',
'${account_hash[\'account\'][\'id\']}')
+                                  button_to(t('images.show.push'), path, :method
=> :post, :class => 'upload_image button')}
+                            {{/if}}
+                            {{if
failed_push_counts[account_hash['account']['id']] > 1}}
+                              #{t('images.show.failed_push_attempts', :count
=> '${failed_push_counts[account_hash[\'account\'][\'id\']]}'
)}
+                            {{else
failed_push_counts[account_hash['account']['id']] == 1}}
+                              #{t('images.show.failed_push_attempts', :count
=> 1 )}
+                            {{/if}}
                           {{else
provider_images[account_hash['account']['id']]}}
-                            #{path =
image_provider_image_path('replace_image_id',
'replace_provider_image_id')
-                              path = path.sub('replace_image_id',
'${image[\'uuid\']}')
-                              path = path.sub('replace_provider_image_id',
'${provider_images[account_hash[\'account\'][\'id\']][\'uuid\']}')
-                              button_to("", path, :method => :delete, :class
=> 'delete_image')}
+                            {{if user_can_build}}
+                              #{path =
image_provider_image_path('replace_image_id',
'replace_provider_image_id')
+                                path = path.sub('replace_image_id',
'${image[\'uuid\']}')
+                                path = path.sub('replace_provider_image_id',
'${provider_images[account_hash[\'account\'][\'id\']][\'uuid\']}')
+                                  button_to("", path, :method => :delete,
:class => 'delete_image')}
+                            {{/if}}
                           {{else}}
                             &nbsp;
                           {{/if}}
diff --git a/src/app/views/pools/_form.html.haml b/src/app/views/pools/_form.html.haml
index 3993904..98f9c44 100644
--- a/src/app/views/pools/_form.html.haml
+++ b/src/app/views/pools/_form.html.haml
@@ -8,7 +8,10 @@
       = form.text_field :name, :class => 'em long'
     %p
       = form.label :pool_family_id, t("pool_families.pool_family")
-      = form.collection_select :pool_family_id, PoolFamily.all, :id, :name
+      - if @pool_families
+        = form.collection_select :pool_family_id, @pool_families, :id, :name
+      - else
+        = form.label :pool_family_id, @pool.pool_family.name
 
   %fieldset
     %p
diff --git a/src/features/pool_family.feature b/src/features/pool_family.feature
index a7589e2..ea5108f 100644
--- a/src/features/pool_family.feature
+++ b/src/features/pool_family.feature
@@ -23,9 +23,11 @@ Feature: Pool Families
 
   Scenario: List pool families as unprivileged user
     Given I am logged in
+    And there is a pool family named "hiddenpoolfamily"
     And I can view pool family "testpoolfamily"
     When I go to the pool families page
-    Then I should not see "testpoolfamily"
+    Then I should not see "hiddenpoolfamily"
+    Then I should see "testpoolfamily"
 
   Scenario: Show pool family details
     Given I am an authorised user
diff --git a/src/features/step_definitions/deployable_steps.rb
b/src/features/step_definitions/deployable_steps.rb
index 438dda3..83ba91f 100644
--- a/src/features/step_definitions/deployable_steps.rb
+++ b/src/features/step_definitions/deployable_steps.rb
@@ -25,6 +25,5 @@ end
 
 Given /^a catalog entry "([^"]*)" exists for "([^"]*)"
catalog$/ do |arg1, catalog_name|
   catalog = Catalog.find_by_name(catalog_name) || FactoryGirl.create(:catalog, :name
=> catalog_name)
-  deployable = FactoryGirl.create :deployable, :name => arg1
-  entry = FactoryGirl.create :catalog_entry, :deployable => deployable, :catalog =>
catalog
+  deployable = FactoryGirl.create :deployable, :name => arg1, :catalogs =>
[catalog]
 end
diff --git a/src/features/step_definitions/pool_steps.rb
b/src/features/step_definitions/pool_steps.rb
index 3ec1db2..3f0c050 100644
--- a/src/features/step_definitions/pool_steps.rb
+++ b/src/features/step_definitions/pool_steps.rb
@@ -144,19 +144,13 @@ Given /^"([^"]*)" has catalog
"([^"]*)"$/ do |pool_name, catalog_name|
 end
 
 Given /^"([^"]*)" has catalog_entry "([^"]*)"$/ do
|catalog_name, catalog_entry_name|
-  deployable = FactoryGirl.create :deployable, :name => catalog_entry_name
-  catalog_entry = FactoryGirl.create :catalog_entry, :deployable => deployable
   catalog = Catalog.find_by_name(catalog_name)
-  catalog_entry.catalog = catalog
-  catalog_entry.save!
+  deployable = FactoryGirl.create :deployable, :name => catalog_entry_name, :catalogs
=> [catalog]
 end
 
 Given /^"([^"]*)" has catalog_entry with parameters
"([^"]*)"$/ do |catalog_name, catalog_entry_name|
-  deployable = FactoryGirl.create :deployable_with_parameters, :name =>
catalog_entry_name
-  catalog_entry = FactoryGirl.create :catalog_entry_with_parameters, :deployable =>
deployable
   catalog = Catalog.find_by_name(catalog_name)
-  catalog_entry.catalog = catalog
-  catalog_entry.save!
+  deployable = FactoryGirl.create :deployable_with_parameters, :name =>
catalog_entry_name, :catalogs => [catalog]
 end
 
 Then /^I should see the quota usage for the "([^"]*)" pool$/ do |pool|
diff --git a/src/spec/controllers/deployables_controller_spec.rb
b/src/spec/controllers/deployables_controller_spec.rb
index 72d89f4..02fd22b 100644
--- a/src/spec/controllers/deployables_controller_spec.rb
+++ b/src/spec/controllers/deployables_controller_spec.rb
@@ -121,8 +121,9 @@ describe DeployablesController do
 
     context "with params[:deployables_selected]" do
       before do
-        @deployable1 = Factory :deployable, :name => "test_delete"
-        @deployable2 = Factory :deployable, :name => "test_delete2"
+        catalog = Factory :catalog
+        @deployable1 = Factory :deployable, :name => "test_delete",
:catalogs => [catalog]
+        @deployable2 = Factory :deployable, :name => "test_delete2",
:catalogs => [catalog]
       end
 
       it "deletes both deployables and shows flash notice" do
diff --git a/src/spec/controllers/deployments_controller_spec.rb
b/src/spec/controllers/deployments_controller_spec.rb
index 6986ef8..54b1477 100644
--- a/src/spec/controllers/deployments_controller_spec.rb
+++ b/src/spec/controllers/deployments_controller_spec.rb
@@ -62,9 +62,8 @@ describe DeploymentsController do
         Factory.create(:front_hwp1)
         Factory.create(:front_hwp2)
         @deployment = Factory.build(:deployment)
-        catalog_entry = Factory.create(:catalog_entry)
-        @deployable = catalog_entry.deployable
-        @catalog = catalog_entry.catalog
+        @catalog = Factory.create(:catalog)
+        @deployable = Factory.create(:deployable, :catalogs => [@catalog])
         Deployment.stub!(:new).and_return(@deployment)
         post :create, :deployable_id => @deployable.id
       end
diff --git a/src/spec/controllers/permissions_controller_spec.rb
b/src/spec/controllers/permissions_controller_spec.rb
index 8c4741b..dedf655 100644
--- a/src/spec/controllers/permissions_controller_spec.rb
+++ b/src/spec/controllers/permissions_controller_spec.rb
@@ -27,8 +27,7 @@ describe PermissionsController do
     mock_warden(@admin)
 
     @catalog = FactoryGirl.create(:catalog)
-    @deployable = FactoryGirl.create(:deployable)
-    @catalog.deployables << @deployable
+    @deployable = FactoryGirl.create(:deployable, :catalogs => [@catalog])
 
     @old_role = FactoryGirl.create(:role)
     @new_role = FactoryGirl.create(:role)
diff --git a/src/spec/factories/catalog.rb b/src/spec/factories/catalog.rb
index dcd7f77..71e3243 100644
--- a/src/spec/factories/catalog.rb
+++ b/src/spec/factories/catalog.rb
@@ -22,8 +22,7 @@ FactoryGirl.define do
 
   factory :catalog_with_deployable, :parent => :catalog do |catalog|
     catalog.after_create do |catalog|
-      deployable = Factory :deployable
-      Factory :catalog_entry, :catalog => catalog, :deployable => deployable
+      deployable = Factory :deployable, :catalogs => [catalog]
     end
   end
 end
-- 
1.7.6.4

[PATCH Conductor 7/7] Bug 788148: Moved catalog entry tests to deployables spec

[Scott Seago]

https://bugzilla.redhat.com/show_bug.cgi?id=788148

These tests were already testing deployables, so I moved them to the right spec test file
while fixing them.
---
 src/spec/models/catalog_entry_spec.rb |   46 ---------------------------------
 src/spec/models/deployable_spec.rb    |   28 ++++++++++++++++++++
 2 files changed, 28 insertions(+), 46 deletions(-)
 delete mode 100644 src/spec/models/catalog_entry_spec.rb

diff --git a/src/spec/models/catalog_entry_spec.rb
b/src/spec/models/catalog_entry_spec.rb
deleted file mode 100644
index fa9bea8..0000000
--- a/src/spec/models/catalog_entry_spec.rb
+++ /dev/null
@@ -1,46 +0,0 @@
-#
-#   Copyright 2011 Red Hat, Inc.
-#
-#   Licensed under the Apache License, Version 2.0 (the "License");
-#   you may not use this file except in compliance with the License.
-#   You may obtain a copy of the License at
-#
-#       http://www.apache.org/licenses/LICENSE-2.0
-#
-#   Unless required by applicable law or agreed to in writing, software
-#   distributed under the License is distributed on an "AS IS" BASIS,
-#   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-#   See the License for the specific language governing permissions and
-#   limitations under the License.
-#
-
-require 'spec_helper'
-
-describe CatalogEntry do
-  it "should have a name of reasonable length" do
-    catalog_entry = FactoryGirl.create :catalog_entry
-    [nil, '', 'x'*1025].each do |invalid_name|
-      catalog_entry.deployable.name = invalid_name
-      catalog_entry.deployable.should_not be_valid
-    end
-    catalog_entry.deployable.name = 'x'*1024
-    catalog_entry.deployable.should be_valid
-  end
-
-  it "should have unique name" do
-    catalog = FactoryGirl.create :catalog
-    catalog_entry = FactoryGirl.create :catalog_entry, :catalog => catalog
-    deployable2 = Factory.build(:deployable, :name => catalog_entry.deployable.name)
-    catalog_entry2 = Factory.build(:catalog_entry, :deployable => deployable2,
:catalog => catalog)
-    catalog_entry2.deployable.should_not be_valid
-
-    catalog_entry2.deployable.name = 'unique name'
-    catalog_entry2.deployable.should be_valid
-  end
-
-  it "should have xml content" do
-    catalog_entry = FactoryGirl.create :catalog_entry
-    catalog_entry.deployable.xml = ''
-    catalog_entry.deployable.should_not be_valid
-  end
-end
diff --git a/src/spec/models/deployable_spec.rb b/src/spec/models/deployable_spec.rb
index d1d5734..6b32968 100644
--- a/src/spec/models/deployable_spec.rb
+++ b/src/spec/models/deployable_spec.rb
@@ -43,4 +43,32 @@ describe Deployable do
     deployable.valid_deployable_xml?
     deployable.errors[:xml].should ==
[I18n.t('catalog_entries.flash.warning.not_valid_duplicate_assembly_names')]
   end
+
+  it "should have a name of reasonable length" do
+    catalog = FactoryGirl.create :catalog
+    deployable = FactoryGirl.create(:deployable, :catalogs => [catalog])
+    [nil, '', 'x'*1025].each do |invalid_name|
+      deployable.name = invalid_name
+      deployable.should_not be_valid
+    end
+    deployable.name = 'x'*1024
+    deployable.should be_valid
+  end
+
+  it "should have unique name" do
+    catalog = FactoryGirl.create :catalog
+    deployable = FactoryGirl.create :deployable, :catalogs => [catalog]
+    deployable2 = Factory.build(:deployable, :name => deployable.name)
+    deployable2.should_not be_valid
+
+    deployable2.name = 'unique name'
+    deployable2.should be_valid
+  end
+
+  it "should have xml content" do
+    catalog = FactoryGirl.create :catalog
+    deployable = FactoryGirl.create :deployable, :catalogs => [catalog]
+    deployable.xml = ''
+    deployable.should_not be_valid
+  end
 end
-- 
1.7.6.4