[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