Scott Seago
2009-May-08 19:34 UTC
[Ovirt-devel] [PATCH server 1/3] before_filter and rescue_from refactoring in application.rb
Most of these before_filters will go away once we're using the service later for everything. I fixed a couple cases where the legacy before_filter was in conflict with the service layer code, and the authorize_xxx methods now just raise PermissionError rather than formatting the reply, since rescue_from handles that for us now. I've also added ActionError to the rescue_from handlers, and for PartialSuccessError we now automatically include failures in hte json alert, and we set @successes and @failures so that html response views can access them. Signed-off-by: Scott Seago <sseago at redhat.com> --- src/app/controllers/application.rb | 52 ++++++++++++++++++++---------------- 1 files changed, 29 insertions(+), 23 deletions(-) diff --git a/src/app/controllers/application.rb b/src/app/controllers/application.rb index 4834915..cff5b77 100644 --- a/src/app/controllers/application.rb +++ b/src/app/controllers/application.rb @@ -32,24 +32,24 @@ class ApplicationController < ActionController::Base # FIXME: once service layer is complete, the following before_filters will be # removed as their functionality has been moved to the service layer + # pre_new # pre_create - # pre_edit will remain only for :edit, not :update or :destroy + # pre_edit # pre_show - # authorize_admin will remain only for :new, :edit + # authorize_admin before_filter :pre_new, :only => [:new] before_filter :pre_create, :only => [:create] - before_filter :pre_edit, :only => [:edit] # the following is to facilitate transition to service layer - before_filter :tmp_pre_update, :only => [:update, :destroy] + before_filter :tmp_pre_update, :only => [:edit, :update, :destroy] before_filter :pre_show, :only => [:show] - before_filter :authorize_admin, :only => [:new, :edit] - before_filter :tmp_authorize_admin, :only => [:create, :update, :destroy] + before_filter :tmp_authorize_admin, :only => [:new, :edit, :create, :update, :destroy] before_filter :is_logged_in, :get_help_section # General error handlers, must be in order from least specific # to most specific rescue_from Exception, :with => :handle_general_error rescue_from PermissionError, :with => :handle_perm_error + rescue_from ActionError, :with => :handle_action_error rescue_from PartialSuccessError, :with => :handle_partial_success_error def choose_layout @@ -100,24 +100,20 @@ class ApplicationController < ActionController::Base def pre_show end - def authorize_view(msg=nil) - authorize_action(Privilege::VIEW,msg) + # These authorize_XXX methods should go away once we're fully converted to + # the service layer + def authorize_view + authorize_action(Privilege::VIEW) end - def authorize_user(msg=nil) - authorize_action(Privilege::VM_CONTROL,msg) + def authorize_user + authorize_action(Privilege::VM_CONTROL) end - def authorize_admin(msg=nil) - authorize_action(Privilege::MODIFY,msg) + def authorize_admin + authorize_action(Privilege::MODIFY) end - def authorize_action(privilege, msg=nil) - msg ||= 'You have insufficient privileges to perform action.' - unless authorized?(privilege) - handle_error(:message => msg, - :title => "Access Denied", :status => :forbidden) - false - else - true - end + def authorize_action(privilege) + authorized!(privilege) + true end def handle_perm_error(error) @@ -129,18 +125,26 @@ class ApplicationController < ActionController::Base failures_arr = error.failures.collect do |resource, reason| resource.display_name + ": " + reason end + @successes = error.successes + @failures = error.failures handle_error(:error => error, :status => :ok, :message => error.message + ": " + failures_arr.join(", "), :title => "Some actions failed") end + def handle_action_error(error) + handle_error(:error => error, :status => :conflict, + :title => "Action Error") + end + def handle_general_error(error) + flash[:errmsg] = error.message handle_error(:error => error, :status => :internal_server_error, :title => "Internal Server Error") end def handle_error(hash) - log_error(hash[:error]) + log_error(hash[:error]) if hash[:error] msg = hash[:message] || hash[:error].message title = hash[:title] || "Internal Server Error" status = hash[:status] || :internal_server_error @@ -156,7 +160,9 @@ class ApplicationController < ActionController::Base @errmsg = msg @ajax = params[:ajax] @nolayout = params[:nolayout] - if @ajax + if @layout + render :layout => @layout + elsif @ajax render :template => 'layouts/popup-error', :layout => 'tabs-and-content' elsif @nolayout render :template => 'layouts/popup-error', :layout => 'help-and-content' -- 1.6.0.6
Scott Seago
2009-May-08 19:34 UTC
[Ovirt-devel] [PATCH server 2/3] Additional vm_controller refactoring:
1) Moved all remaining auth/before_filter action into service layer 2) Added rdoc comments 3) delete VMs action now uses svc_destroy action Signed-off-by: Scott Seago <sseago at redhat.com> --- src/app/controllers/vm_controller.rb | 89 +++++++------------------- src/app/services/vm_service.rb | 103 ++++++++++++++++++++++++++---- src/app/views/resources/vm_actions.rhtml | 2 +- 3 files changed, 114 insertions(+), 80 deletions(-) diff --git a/src/app/controllers/vm_controller.rb b/src/app/controllers/vm_controller.rb index 29c0f16..a40fc5c 100644 --- a/src/app/controllers/vm_controller.rb +++ b/src/app/controllers/vm_controller.rb @@ -17,7 +17,6 @@ # MA 02110-1301, USA. A copy of the GNU General Public License is # also available at http://www.gnu.org/copyleft/gpl.html. require 'socket' -require 'services/vm_service' class VmController < ApplicationController include VmService @@ -26,8 +25,6 @@ class VmController < ApplicationController verify :method => :post, :only => [ :destroy, :create, :update ], :redirect_to => { :controller => 'dashboard' } - before_filter :pre_console, :only => [:console] - def index @vms = Vm.find(:all, :include => [{:vm_resource_pool => @@ -52,7 +49,10 @@ class VmController < ApplicationController end def new + alert = svc_new(params[:vm_resource_pool_id]) + _setup_provisioning_options @storage_tree = VmResourcePool.find(params[:vm_resource_pool_id]).get_hardware_pool.storage_tree.to_json + @networks = Network.find(:all).collect{ |net| [net.name, net.id] } render :layout => 'popup' end @@ -63,6 +63,9 @@ class VmController < ApplicationController end def edit + svc_modify(params[:id]) + _setup_provisioning_options + @networks = Network.find(:all).collect{ |net| [net.name, net.id] } @storage_tree = @vm.vm_resource_pool.get_hardware_pool.storage_tree(:vm_to_include => @vm).to_json render :layout => 'popup' end @@ -74,37 +77,26 @@ class VmController < ApplicationController render :json => { :object => "vm", :success => true, :alert => alert } end - #FIXME: we need permissions checks. user must have permission. Also state checks - # this should probably be implemented as an action on the containing VM pool once - # that service module is defined def delete - vm_ids_str = params[:vm_ids] - vm_ids = vm_ids_str.split(",").collect {|x| x.to_i} - failure_list = [] - success = false - begin - Vm.transaction do - vms = Vm.find(:all, :conditions => "id in (#{vm_ids.join(', ')})") - vms.each do |vm| - if vm.is_destroyable? - destroy_cobbler_system(vm) - vm.destroy - else - failure_list << vm.description - end - end - end - if failure_list.empty? - success = true - alert = "Virtual Machines were successfully deleted." - else - alert = "The following Virtual Machines were not deleted (a VM must be stopped to delete it): " - alert+= failure_list.join(', ') + vm_ids = params[:vm_ids].split(",") + successes = [] + failures = {} + vm_ids.each do |vm_id| + begin + svc_destroy(vm_id) + successes << @vm + rescue PermissionError => perm_error + failures[@vm] = perm_error.message + rescue Exception => ex + failures[@vm] = ex.message end - rescue - alert = "Error deleting virtual machines." end - render :json => { :object => "vm", :success => success, :alert => alert } + unless failures.empty? + raise PartialSuccessError.new("Delete failed for some VMs", + failures, successes) + end + render :json => { :object => "vm", :success => success, + :alert => "VM Pools were successfully deleted." } end def destroy @@ -142,6 +134,7 @@ class VmController < ApplicationController end def console + svc_modify(params[:id]) @show_vnc_error = "Console is unavailable for VM #{@vm.description}" unless @vm.has_console if @vm.host.hostname.match("priv\.ovirt\.org$") @vnc_hostname = IPSocket.getaddress(@vm.host.hostname) @@ -172,40 +165,6 @@ class VmController < ApplicationController end end - def pre_new - unless params[:vm_resource_pool_id] - flash[:notice] = "VM Resource Pool is required." - redirect_to :controller => 'dashboard' - end - - # random MAC - mac = [ 0x00, 0x16, 0x3e, rand(0x7f), rand(0xff), rand(0xff) ] - # random uuid - uuid = ["%02x" * 4, "%02x" * 2, "%02x" * 2, "%02x" * 2, "%02x" * 6].join("-") % - Array.new(16) {|x| rand(0xff) } - newargs = { - :vm_resource_pool_id => params[:vm_resource_pool_id], - :vnic_mac_addr => mac.collect {|x| "%02x" % x}.join(":"), - :uuid => uuid - } - @vm = Vm.new( newargs ) - unless params[:vm_resource_pool_id] - @vm.vm_resource_pool = @vm_resource_pool - end - set_perms(@vm.vm_resource_pool) - @networks = Network.find(:all).collect{ |net| [net.name, net.id] } - _setup_provisioning_options - end - def pre_edit - @vm = Vm.find(params[:id]) - set_perms(@vm.vm_resource_pool) - @networks = Network.find(:all).collect{ |net| [net.name, net.id] } - _setup_provisioning_options - end - def pre_console - pre_edit - authorize_user - end # FIXME: remove these when service transition is complete. these are here # to keep from running permissions checks and other setup steps twice def tmp_pre_update diff --git a/src/app/services/vm_service.rb b/src/app/services/vm_service.rb index 8256730..ff1a919 100644 --- a/src/app/services/vm_service.rb +++ b/src/app/services/vm_service.rb @@ -22,12 +22,54 @@ module VmService include ApplicationService - def svc_show(vm_id) - # from before_filter - @vm = Vm.find(vm_id) - authorized!(Privilege::VIEW, at vm.vm_resource_pool) + # Load the Vm with +id+ for viewing + # + # === Instance variables + # [<tt>@vm</tt>] stores the Vm with +id+ + # === Required permissions + # [<tt>Privilege::VIEW</tt>] on vm's VmResourcePool + def svc_show(id) + lookup(id,Privilege::VIEW) + end + + # Load the Vm with +id+ for editing + # + # === Instance variables + # [<tt>@vm</tt>] stores the Vm with +id+ + # === Required permissions + # [<tt>Privilege::MODIFY</tt>] on vm's VmResourcePool + def svc_modify(id) + lookup(id,Privilege::MODIFY) end + # Load a new Vm for creating + # + # === Instance variables + # [<tt>@vm</tt>] loads a new Vm object into memory + # === Required permissions + # [<tt>Privilege::MODIFY</tt>] for the vm's VmResourcePool as specified by + # +vm_resource_pool_id+ + def svc_new(vm_resource_pool_id) + raise ActionError.new("VM Resource Pool is required.") unless vm_resource_pool_id + + # random MAC + mac = [ 0x00, 0x16, 0x3e, rand(0x7f), rand(0xff), rand(0xff) ] + # random uuid + uuid = ["%02x"*4, "%02x"*2, "%02x"*2, "%02x"*2, "%02x"*6].join("-") % + Array.new(16) {|x| rand(0xff) } + + @vm = Vm.new({:vm_resource_pool_id => vm_resource_pool_id, + :vnic_mac_addr => mac.collect {|x| "%02x" % x}.join(":"), + :uuid => uuid }) + authorized!(Privilege::MODIFY, @vm.vm_resource_pool) + end + + # Save a new Vm + # + # === Instance variables + # [<tt>@vm</tt>] the newly saved Vm + # === Required permissions + # [<tt>Privilege::MODIFY</tt>] for the vm's VmResourcePool def svc_create(vm_hash, start_now) # from before_filter vm_hash[:state] = Vm::STATE_PENDING @@ -59,9 +101,15 @@ module VmService return alert end - def svc_update(vm_id, vm_hash, start_now, restart_now) + # Update attributes for the Vm with +id+ + # + # === Instance variables + # [<tt>@vm</tt>] stores the Vm with +id+ + # === Required permissions + # [<tt>Privilege::MODIFY</tt>] for the Vm's VmResourcePool + def svc_update(id, vm_hash, start_now, restart_now) # from before_filter - @vm = Vm.find(vm_id) + @vm = Vm.find(id) authorized!(Privilege::MODIFY, @vm.vm_resource_pool) #needs restart if certain fields are changed @@ -119,9 +167,15 @@ module VmService return alert end - def svc_destroy(vm_id) + # Destroys for the Vm with +id+ + # + # === Instance variables + # [<tt>@vm</tt>] stores the Vm with +id+ + # === Required permissions + # [<tt>Privilege::MODIFY</tt>] for the Vm's VmResourcePool + def svc_destroy(id) # from before_filter - @vm = Vm.find(vm_id) + @vm = Vm.find(id) authorized!(Privilege::MODIFY, @vm.vm_resource_pool) unless @vm.is_destroyable? @@ -129,20 +183,34 @@ module VmService end destroy_cobbler_system(@vm) @vm.destroy - return "Virtual Machine wa ssuccessfully deleted." + return "Virtual Machine was successfully deleted." end - def svc_vm_action(vm_id, vm_action, action_args) - @vm = Vm.find(vm_id) - authorized!(Privilege::MODIFY, @vm.vm_resource_pool) + # Queues action +vm_action+ for Vm with +id+ + # + # === Instance variables + # [<tt>@vm</tt>] stores the Vm with +id+ + # === Required permissions + # permission is action-specific as determined by + # <tt>VmTask.action_privilege(@action)</tt> + def svc_vm_action(id, vm_action, action_args) + @vm = Vm.find(id) + authorized!(VmTask.action_privilege(vm_action), + VmTask.action_privilege_object(vm_action, at vm)) unless @vm.queue_action(@user, vm_action, action_args) raise ActionError.new("#{vm_action} is an invalid action.") end return "#{vm_action} was successfully queued." end - def svc_cancel_queued_tasks(vm_id) - @vm = Vm.find(vm_id) + # Cancels queued tasks for for Vm with +id+ + # + # === Instance variables + # [<tt>@vm</tt>] stores the Vm with +id+ + # === Required permissions + # [<tt>Privilege::MODIFY</tt>] for the Vm's VmResourcePool + def svc_cancel_queued_tasks(id) + @vm = Vm.find(id) authorized!(Privilege::MODIFY, @vm.vm_resource_pool) Task.transaction do @@ -151,6 +219,7 @@ module VmService return "Queued tasks were successfully canceled." end + protected def vm_provision if @vm.uses_cobbler? # spaces are invalid in the cobbler name @@ -174,4 +243,10 @@ module VmService end end + private + def lookup(id, priv) + @vm = Vm.find(id) + authorized!(priv, at vm.vm_resource_pool) + end + end diff --git a/src/app/views/resources/vm_actions.rhtml b/src/app/views/resources/vm_actions.rhtml index b3fa1ad..2a50a31 100644 --- a/src/app/views/resources/vm_actions.rhtml +++ b/src/app/views/resources/vm_actions.rhtml @@ -20,7 +20,7 @@ <% end %> Action succeeded for these VMs: <ul> - <% for vm in @success_list %> + <% for vm in @successes %> <li><%= vm.description %></li> <% end %> </ul> -- 1.6.0.6
Ian Main
2009-May-11 20:59 UTC
[Ovirt-devel] [PATCH server 1/3] before_filter and rescue_from refactoring in application.rb
On Fri, 8 May 2009 19:34:09 +0000 Scott Seago <sseago at redhat.com> wrote:> Most of these before_filters will go away once we're using the service later for everything. I fixed a couple cases where the legacy before_filter was in conflict with the service layer code, and the authorize_xxx methods now just raise PermissionError rather than formatting the reply, since rescue_from handles that for us now. > > I've also added ActionError to the rescue_from handlers, and for PartialSuccessError we now automatically include failures in hte json alert, and we set @successes and @failures so that html response views can access them. >ACK