Ovirt devel - May 2009 - [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

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

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