[PATCH conductor] bug 790735: Enforce image permissions on deleting imported images https://bugzilla.redhat.com/show_bug.cgi?id=790735

Scott Seago sseago at redhat.com
Fri Mar 9 05:55:09 UTC 2012


Recent commits added most of the needed permissions checking for
various image actions. Unfortunately there was a bug in the code
that determined whether the user had permissions.

In particular, although 'and' and '&&' have a similar meaning in isolation
(i.e. 'a and b' and 'a && b' will resolve to the same result, they
have a different operator precedence. && takes precedence over assignment =
However, assignment = takes precedence over 'and'

In this case, it meant that code written like

  @foo = @bar and @bar.my_method

Did not do what was needed. The above method is equivalent to:

 (@foo = bar) and @bar.my_method

Which is obviously incorrect. It must be rewritten as:
 @foo = (@bar and @bar.my_method)
or
 @foo = @bar && @bar.my_method

In addition, not only were the 'hide the actions for non-privileged user' filters
in the view not working as desired, but the actual delete actions on provider_images_controller
and target_images_controller were not enforcing permissions.

This patch resolves both issues
---
 src/app/controllers/images_controller.rb          |    1 +
 src/app/controllers/provider_images_controller.rb |   39 ++++++++++++--------
 src/app/controllers/target_images_controller.rb   |    5 +++
 src/app/views/images/show.html.haml               |    3 +-
 4 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/src/app/controllers/images_controller.rb b/src/app/controllers/images_controller.rb
index 954983a..846ff89 100644
--- a/src/app/controllers/images_controller.rb
+++ b/src/app/controllers/images_controller.rb
@@ -350,6 +350,7 @@ class ImagesController < ApplicationController
     selected_images.each do |uuid|
       image = Aeolus::Image::Warehouse::Image.find(uuid)
       @environment = PoolFamily.where('name' => image.environment).first
+      check_permissions
       image.delete!
     end
     redirect_to images_path, :notice => t("images.flash.notice.multiple_deleted", :count => selected_images.count)
diff --git a/src/app/controllers/provider_images_controller.rb b/src/app/controllers/provider_images_controller.rb
index 0f94e38..4155f7a 100644
--- a/src/app/controllers/provider_images_controller.rb
+++ b/src/app/controllers/provider_images_controller.rb
@@ -18,24 +18,28 @@ class ProviderImagesController < ApplicationController
   before_filter :require_user
 
   def create
-    provider_account = ProviderAccount.find(params[:account_id])
-    @provider_image = Aeolus::Image::Factory::ProviderImage.new(
-      :provider => provider_account.provider.name,
-      :credentials => provider_account.to_xml(:with_credentials => true),
-      :image_id => params[:image_id],
-      :build_id => params[:build_id],
-      :target_image_id => params[:target_image_id]
-    )
-    begin
-      if @provider_image.save
-        flash[:notice] = t('provider_images.flash.notice.upload_start')
-        @push_started = true
-      else
+    if image = Aeolus::Image::Warehouse::Image.find(params[:image_id])
+      @environment = PoolFamily.where('name' => image.environment).first
+      require_privilege(Privilege::USE, @environment)
+      provider_account = ProviderAccount.find(params[:account_id])
+      @provider_image = Aeolus::Image::Factory::ProviderImage.new(
+        :provider => provider_account.provider.name,
+        :credentials => provider_account.to_xml(:with_credentials => true),
+        :image_id => params[:image_id],
+        :build_id => params[:build_id],
+        :target_image_id => params[:target_image_id]
+      )
+      begin
+        if @provider_image.save
+          flash[:notice] = t('provider_images.flash.notice.upload_start')
+          @push_started = true
+        else
+          flash[:warning] = t('provider_images.flash.warning.upload_failed')
+        end
+      rescue Exception => e
+        logger.error "Caught exception importing image: #{e.message}"
         flash[:warning] = t('provider_images.flash.warning.upload_failed')
       end
-    rescue Exception => e
-      logger.error "Caught exception importing image: #{e.message}"
-      flash[:warning] = t('provider_images.flash.warning.upload_failed')
     end
     redirect_to image_path(params[:image_id], :build => params[:build_id], :push_started => @push_started,
                            :provider_account_id => @push_started ? provider_account.id : nil)
@@ -43,6 +47,9 @@ class ProviderImagesController < ApplicationController
 
   def destroy
     if image = Aeolus::Image::Warehouse::ProviderImage.find(params[:id])
+      top_image = Aeolus::Image::Warehouse::Image.find(params[:image_id])
+      @environment = PoolFamily.where('name' => top_image.environment).first
+      require_privilege(Privilege::USE, @environment)
       target_id = image.target_identifier
       provider = image.provider
       i = image.target_image.build.image
diff --git a/src/app/controllers/target_images_controller.rb b/src/app/controllers/target_images_controller.rb
index 4b35fa0..a647315 100644
--- a/src/app/controllers/target_images_controller.rb
+++ b/src/app/controllers/target_images_controller.rb
@@ -19,6 +19,8 @@ class TargetImagesController < ApplicationController
 
   def create
     wh_image = Aeolus::Image::Warehouse::Image.find(params[:image_id])
+    @environment = PoolFamily.where('name' => wh_image.environment).first
+    require_privilege(Privilege::USE, @environment)
 
     begin
       timage = Aeolus::Image::Factory::Image.new(
@@ -38,6 +40,9 @@ class TargetImagesController < ApplicationController
 
   def destroy
     if image = Aeolus::Image::Warehouse::TargetImage.find(params[:id])
+      wh_image = Aeolus::Image::Warehouse::Image.find(params[:image_id])
+      @environment = PoolFamily.where('name' => wh_image.environment).first
+      require_privilege(Privilege::USE, @environment)
       i = image.build.image
       if i.imported?
         if i.delete!
diff --git a/src/app/views/images/show.html.haml b/src/app/views/images/show.html.haml
index 6a4615a..25e4b0b 100644
--- a/src/app/views/images/show.html.haml
+++ b/src/app/views/images/show.html.haml
@@ -12,8 +12,7 @@
           = link_to t('.template_xml'), template_image_path(@image.uuid), :class => 'button pill'
         = button_to t("delete"), image_path(@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)
-
+- user_can_build = (@environment and check_privilege(Privilege::USE, @environment))
 %section.admin-content-section
   %header
     %h2=t'properties'
-- 
1.7.6.4




More information about the aeolus-devel mailing list