[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