Ovirt devel - May 2009 - HostController -> Service (take 2)

This is a repost of the patch series from last Tuesday.

The main change is that auth errors are now handled with Rails rescue_from
mechanism.

David

---
 src/Rakefile |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/src/Rakefile b/src/Rakefile
index 74b9d18..eebf798 100644
--- a/src/Rakefile
+++ b/src/Rakefile
@@ -1,3 +1,4 @@
+# -*- ruby -*-
 # Add your own tasks in files placed in lib/tasks ending in .rake,
 # for example lib/tasks/capistrano.rake, and they will automatically be
available to Rake.
 
@@ -8,14 +9,18 @@ require 'rake/testtask'
 require 'rake/rdoctask'
 
 require 'tasks/rails'
-require 'gettext/utils'
+# Do not require 'gettext/utils' here, it breaks rdoc,
+# require it in tasks that actually need it instead
+
 desc "Create mo-files for L10n" 
 task :makemo do
+ require 'gettext/utils'
  GetText.create_mofiles(true, "po", "locale")
 end
 
 desc "Update pot/po files to match new version." 
 task :updatepo do
+ require 'gettext/utils'
  MY_APP_TEXT_DOMAIN = "ovirt" 
  MY_APP_VERSION     = "ovirt 0.0.1" 
  GetText.update_pofiles(MY_APP_TEXT_DOMAIN,
-- 
1.6.0.6

Use Rails' rescue_from to catch permission errors and render an appropriate
response.
---
 src/app/controllers/application.rb |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/src/app/controllers/application.rb
b/src/app/controllers/application.rb
index e5f4d4b..3902e78 100644
--- a/src/app/controllers/application.rb
+++ b/src/app/controllers/application.rb
@@ -48,6 +48,8 @@ class ApplicationController < ActionController::Base
   before_filter :tmp_authorize_admin, :only => [:create, :update, :destroy]
   before_filter :is_logged_in, :get_help_section
 
+  # General error handlers
+  rescue_from PermissionError, :with => :handle_perm_error
 
   def choose_layout
     if(params[:component_layout])
@@ -140,6 +142,39 @@ class ApplicationController < ActionController::Base
     end
   end
 
+  def handle_perm_error(error)
+    handle_error(:error => error, :status => :forbidden,
+                 :title => "Access denied")
+  end
+
+  def handle_error(hash)
+    msg = hash[:message] || hash[:error].message
+    title = hash[:title] || "Internal Server Error"
+    status = hash[:status] || :internal_server_error
+    respond_to do |format|
+      format.html do
+        @title = title || "Something went very wrong"
+        @errmsg = msg
+        @ajax = params[:ajax]
+        @nolayout = params[:nolayout]
+        if @ajax
+          render :template => 'layouts/popup-error', :layout =>
'tabs-and-content'
+        elsif @nolayout
+          render :template => 'layouts/popup-error', :layout =>
'help-and-content'
+        else
+          render :template => 'layouts/popup-error', :layout =>
'popup'
+        end
+      end
+      format.json do
+        @json_hash ||= {}
+        @json_hash[:success] = (status == :ok)
+        @json_hash[:alert] = msg
+        render :json => @json_hash
+      end
+      format.xml { head status }
+    end
+  end
+
   # don't define find_opts for array inputs
   def json_hash(full_items, attributes, arg_list=[], find_opts={},
id_method=:id)
     page = params[:page].to_i
-- 
1.6.0.6

---
 src/app/controllers/application.rb               |   27 +--------------------
 src/app/controllers/storage_volume_controller.rb |    5 +++-
 src/app/controllers/vm_controller.rb             |   12 ++-------
 3 files changed, 9 insertions(+), 35 deletions(-)

diff --git a/src/app/controllers/application.rb
b/src/app/controllers/application.rb
index 3902e78..638f9c0 100644
--- a/src/app/controllers/application.rb
+++ b/src/app/controllers/application.rb
@@ -111,36 +111,13 @@ class ApplicationController < ActionController::Base
   def authorize_action(privilege, msg=nil)
     msg ||= 'You have insufficient privileges to perform action.'
     unless authorized?(privilege)
-      handle_auth_error(msg)
+      handle_error(:message => msg,
+                   :title => "Access Denied", :status =>
:forbidden)
       false
     else
       true
     end
   end
-  def handle_auth_error(msg)
-    respond_to do |format|
-      format.html do
-        @title = "Access denied"
-        @errmsg = msg
-        @ajax = params[:ajax]
-        @nolayout = params[:nolayout]
-        if @ajax
-          render :template => 'layouts/popup-error', :layout =>
'tabs-and-content'
-        elsif @nolayout
-          render :template => 'layouts/popup-error', :layout =>
'help-and-content'
-        else
-          render :template => 'layouts/popup-error', :layout =>
'popup'
-        end
-      end
-      format.json do
-        @json_hash ||= {}
-        @json_hash[:success] = false
-        @json_hash[:alert] = msg
-        render :json => @json_hash
-      end
-      format.xml { head :forbidden }
-    end
-  end
 
   def handle_perm_error(error)
     handle_error(:error => error, :status => :forbidden,
diff --git a/src/app/controllers/storage_volume_controller.rb
b/src/app/controllers/storage_volume_controller.rb
index b6b0593..c6ce58d 100644
--- a/src/app/controllers/storage_volume_controller.rb
+++ b/src/app/controllers/storage_volume_controller.rb
@@ -112,7 +112,10 @@ class StorageVolumeController < ApplicationController
     @storage_volume = StorageVolume.find(params[:id])
     set_perms(@storage_volume.storage_pool.hardware_pool)
     unless authorized?(Privilege::MODIFY) and
@storage_volume.storage_pool.user_subdividable
-      handle_auth_error("You do not have permission to delete this storage
volume.")
+      handle_error(:message =>
+                   "You do not have permission to delete this storage
volume.",
+                   :status => :forbidden,
+                   :title => "Access Denied")
     else
       alert, success = delete_volume_internal(@storage_volume)
       respond_to do |format|
diff --git a/src/app/controllers/vm_controller.rb
b/src/app/controllers/vm_controller.rb
index ee6d79e..c276f0b 100644
--- a/src/app/controllers/vm_controller.rb
+++ b/src/app/controllers/vm_controller.rb
@@ -41,13 +41,9 @@ class VmController < ApplicationController
   end
 
   def show
-    begin
-      svc_show(params[:id])
-      @actions = @vm.get_action_hash(@user)
-      render :layout => 'selection'
-    rescue PermissionError => perm_error
-      handle_auth_error(perm_error.message)
-    end
+    svc_show(params[:id])
+    @actions = @vm.get_action_hash(@user)
+    render :layout => 'selection'
   end
 
   def add_to_smart_pool
@@ -65,8 +61,6 @@ class VmController < ApplicationController
     begin
       alert = svc_create(params[:vm], params[:start_now])
       render :json => { :object => "vm", :success => true,
:alert => alert  }
-    rescue PermissionError => perm_error
-      handle_auth_error(perm_error.message)
     rescue Exception => error
       json_error("vm", @vm, error)
     end
-- 
1.6.0.6

The business logic is now in HostService.

The main functional difference is that svc_list filters the host list by
permission.
---
 src/app/controllers/host_controller.rb |  125 +++++++++++++-------------------
 src/app/services/host_service.rb       |  119 ++++++++++++++++++++++++++++++
 2 files changed, 168 insertions(+), 76 deletions(-)
 create mode 100644 src/app/services/host_service.rb

diff --git a/src/app/controllers/host_controller.rb
b/src/app/controllers/host_controller.rb
index 994b5e2..9b683fa 100644
--- a/src/app/controllers/host_controller.rb
+++ b/src/app/controllers/host_controller.rb
@@ -19,8 +19,13 @@
 
 class HostController < ApplicationController
 
-  EQ_ATTRIBUTES = [ :state, :arch, :hostname, :uuid,
-                    :hardware_pool_id ]
+  include HostService
+
+  # GETs should be safe (see http://www.w3.org/2001/tag/doc/whenToUseGet.html)
+  verify :method => [:post, :put], :only => [ :create, :update ],
+         :redirect_to => { :action => :list }
+  verify :method => [:post, :delete], :only => :destroy,
+         :redirect_to => { :action => :list }
 
   def index
     list
@@ -30,42 +35,21 @@ class HostController < ApplicationController
     end
   end
 
-  before_filter :pre_action, :only => [:host_action, :enable, :disable,
:clear_vms, :edit_network]
-  before_filter :pre_addhost, :only => [:addhost]
-
-  # GETs should be safe (see http://www.w3.org/2001/tag/doc/whenToUseGet.html)
-  verify :method => [:post, :put], :only => [ :create, :update ],
-         :redirect_to => { :action => :list }
-  verify :method => [:post, :delete], :only => :destroy,
-         :redirect_to => { :action => :list }
-
-   def list
-     conditions = []
-     EQ_ATTRIBUTES.each do |attr|
-       if params[attr]
-         conditions << "#{attr} = :#{attr}"
-       end
-     end
-     @hosts = Host.find(:all,
-                        :conditions => [conditions.join(" and "),
params],
-                        :order => "id")
-   end
-
+  def list
+    svc_list(params)
+  end
 
   def show
-    if authorize_view
-      respond_to do |format|
-        format.html { render :layout => 'selection' }
-        format.xml { render :xml => @host.to_xml(:include => [ :cpus ] )
}
-      end
+    svc_show(params[:id])
+    respond_to do |format|
+      format.html { render :layout => 'selection' }
+      format.xml { render :xml => @host.to_xml(:include => [ :cpus ] ) }
     end
   end
 
   def quick_summary
-    pre_show
-    if authorize_view
-      render :layout => false
-    end
+    svc_show(id)
+    render :layout => false
   end
 
   # retrieves data used by snapshot graphs
@@ -73,15 +57,17 @@ class HostController < ApplicationController
   end
 
   def addhost
-    @hardware_pool = Pool.find(params[:hardware_pool_id])
-    render :layout => 'popup'
-  end
+    # FIXME: This probably should go into PoolService.svc_modify,
+    # so that we have permission checks in only one place
 
-  def pre_addhost
+    # Old pre_addhost
     @pool = Pool.find(params[:hardware_pool_id])
     @parent = @pool.parent
     set_perms(@pool)
     authorize_admin
+    # Old addhost
+    @hardware_pool = Pool.find(params[:hardware_pool_id])
+    render :layout => 'popup'
   end
 
   def add_to_smart_pool
@@ -89,6 +75,11 @@ class HostController < ApplicationController
     render :layout => 'popup'
   end
 
+  # FIXME: We implement the standard controller actions, but catch
+  # them in filters and kick out friendly warnings that you can't
+  # perform them on hosts. Tat's overkill - the only way for a user
+  # to get to these actions is with URL surgery or from a bug in the
+  # application, both of which deserve a noisy error
   def new
   end
 
@@ -116,52 +107,40 @@ class HostController < ApplicationController
   end
 
   def disable
-    set_disabled(1)
+    svc_enable(params[:id], "disabled")
+    render :json => {
+      :object => :host,
+      :alert => "Host was successfully disabled",
+      :success => true
+    }
   end
+
   def enable
-    set_disabled(0)
-  end
-
-  def set_disabled(value)
-    operation = value == 1 ? "disabled" : "enabled"
-    begin
-      @host.is_disabled = value
-      @host.save!
-      @json_hash[:alert]="Host was successfully #{operation}"
-      @json_hash[:success]=true
-    rescue
-      @json_hash[:alert]="Error setting host to #{operation}"
-      @json_hash[:success]=false
-    end
-    render :json => @json_hash
+    svc_enable(params[:id], "enabled")
+    render :json => {
+      :object => :host,
+      :alert => "Host was successfully enabled",
+      :success => true
+    }
   end
 
   def clear_vms
-    begin
-      Host.transaction do
-        task = HostTask.new({ :user        => get_login_user,
-                              :task_target => @host,
-                              :action      => HostTask::ACTION_CLEAR_VMS,
-                              :state       => Task::STATE_QUEUED})
-        task.save!
-        @host.is_disabled = true
-        @host.save!
-      end
-      @json_hash[:alert]="Clear VMs action was successfully queued."
-      @json_hash[:success]=true
-    rescue
-      @json_hash[:alert]="Error in queueing Clear VMs action."
-      @json_hash[:success]=false
-    end
-    render :json => @json_hash
+    svc_clear_vms(params[:id])
+    render :json => {
+      :object => :host,
+      :alert => "Clear VMs action was successfully queued.",
+      :success => true
+    }
   end
 
   def edit_network
+    svc_modify(params[:id])
     render :layout => 'popup'
   end
 
   def bondings_json
-    bondings = Host.find(params[:id]).bondings
+    svc_show(params[:id])
+    bondings = @host.bondings
     render :json => bondings.collect{ |x| {:id => x.id, :name =>
x.name} }
   end
 
@@ -180,12 +159,6 @@ class HostController < ApplicationController
     flash[:notice] = 'Hosts may not be edited via the web UI'
     redirect_to :action=> 'show', :id => @host
   end
-  def pre_action
-    @host = Host.find(params[:id])
-    set_perms(@host.hardware_pool)
-    @json_hash = { :object => :host }
-    authorize_admin
-  end
   def pre_show
     @host = Host.find(params[:id])
     set_perms(@host.hardware_pool)
diff --git a/src/app/services/host_service.rb b/src/app/services/host_service.rb
new file mode 100644
index 0000000..074d830
--- /dev/null
+++ b/src/app/services/host_service.rb
@@ -0,0 +1,119 @@
+#--
+# Copyright (C) 2009 Red Hat, Inc.
+# Written by David Lutterkort <lutter at redhat.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+# MA  02110-1301, USA.  A copy of the GNU General Public License is
+# also available at http://www.gnu.org/copyleft/gpl.html.
+#++
+# Business logic around hosts aka nodes
+module HostService
+
+  include ApplicationService
+
+  # Host attributes on which we filter with '='
+  EQ_ATTRIBUTES = [ :state, :arch, :hostname, :uuid,
+                    :hardware_pool_id ]
+
+  # List hosts matching criteria described by +params+; only the entries
+  # for keys in +EQ_ATTRIBUTES+ are used
+  #
+  # === Instance variables
+  # [<tt>@hosts</tt>] stores list of hosts matching criteria
+  # === Required permissions
+  # [<tt>Privilege::VIEW</tt>] no exception raised,
<tt>@hosts</tt>
+  #                            is filtered by privilege
+  def svc_list(params)
+    conditions = []
+    EQ_ATTRIBUTES.each do |attr|
+      if params[attr]
+        conditions << "hosts.#{attr} = :#{attr}"
+      end
+    end
+    # Add permission check
+    params = params.dup
+    params[:user] = get_login_user
+    params[:priv] = Privilege::VIEW
+    conditions << "privileges.name=:priv"
+    conditions << "permissions.uid=:user"
+    incl = [{ :hardware_pool => { :permissions => { :role =>
:privileges}}}]
+    @hosts = Host.find(:all,
+                       :include => incl,
+                       :conditions => [conditions.join(" and "),
params],
+                       :order => "hosts.id")
+  end
+
+  # Load the Host with +id+ for viewing
+  #
+  # === Instance variables
+  # [<tt>@host</tt>] stores the Host with +id+
+  # === Required permissions
+  # [<tt>Privilege::VIEW</tt>] on host's HardwarePool
+  def svc_show(id)
+    lookup(id, Privilege::VIEW)
+  end
+
+  # Load the Host with +id+ for editing
+  #
+  # === Instance variables
+  # [<tt>@host</tt>] stores the Host with +id+
+  # === Required permissions
+  # [<tt>Privilege::MODIFY</tt>] on host's HardwarePool
+  def svc_modify(id)
+    lookup(id, Privilege::MODIFY)
+  end
+
+  # Set the disabled state of the Host with +id+ to
<tt>:enabled</tt>
+  # or <tt>:disabled</tt>
+  #
+  # === Instance variables
+  # [<tt>@host</tt>] stores the Host with +id+
+  # === Required permissions
+  # [<tt>Privilege::MODIFY</tt>] on host's HardwarePool
+  def svc_enable(id, state)
+    ind = ["enabled", "disabled"].index(state.to_s)
+    if ind.nil?
+      raise ArgumentError, "STATE must be 'enabled' or
'disabled'"
+    end
+    svc_modify(id)
+    @host.is_disabled = ind
+    @host.save!
+  end
+
+  # Queue task to migrate all VM's off the Host with +id+, and mark the
+  # host as disabled
+  #
+  # === Instance variables
+  # [<tt>@host</tt>] stores the Host with +id+
+  # === Required permissions
+  # [<tt>Privilege::MODIFY</tt>] on host's HardwarePool
+  def svc_clear_vms(id)
+    svc_modify(id)
+    Host.transaction do
+      task = HostTask.new({ :user        => get_login_user,
+                            :task_target => @host,
+                            :action      => HostTask::ACTION_CLEAR_VMS,
+                            :state       => Task::STATE_QUEUED})
+      task.save!
+      @host.is_disabled = true
+      @host.save!
+    end
+  end
+
+  private
+  def lookup(id, priv)
+    @host = Host.find(id)
+    authorized!(priv, @host.hardware_pool)
+  end
+end
-- 
1.6.0.6