[Ovirt-devel] [PATCH server] final cleanup for service layer refactoring.
Scott Seago
sseago at redhat.com
Fri May 15 17:56:27 UTC 2009
I've pulled out the no-longer-necessary remnants of the old way of handling auth and before_filters as well as fixing a couple bugs that had crept in along the way.
Unit test fixes for the refactoring will follow in a subsequent patch.
Signed-off-by: Scott Seago <sseago at redhat.com>
---
src/app/controllers/application.rb | 46 ----------------
src/app/controllers/hardware_controller.rb | 20 ++++----
src/app/controllers/host_controller.rb | 55 --------------------
src/app/controllers/network_controller.rb | 9 ---
src/app/controllers/nic_controller.rb | 28 ----------
src/app/controllers/permission_controller.rb | 7 ---
src/app/controllers/pool_controller.rb | 7 ---
src/app/controllers/quota_controller.rb | 8 ---
src/app/controllers/smart_pools_controller.rb | 2 +-
src/app/controllers/storage_controller.rb | 8 ---
src/app/controllers/storage_volume_controller.rb | 8 ---
src/app/controllers/vm_controller.rb | 13 +----
src/app/services/hardware_pool_service.rb | 1 -
src/app/services/pool_service.rb | 1 -
src/app/services/smart_pool_service.rb | 3 +-
src/app/services/vm_resource_pool_service.rb | 1 -
src/app/services/vm_service.rb | 24 +++++---
.../addhost.html.erb => hardware/addhost.rhtml} | 8 ++--
src/app/views/hardware/show_hosts.rhtml | 4 +-
19 files changed, 35 insertions(+), 218 deletions(-)
rename src/app/views/{host/addhost.html.erb => hardware/addhost.rhtml} (76%)
diff --git a/src/app/controllers/application.rb b/src/app/controllers/application.rb
index e9c515f..040b8a3 100644
--- a/src/app/controllers/application.rb
+++ b/src/app/controllers/application.rb
@@ -30,19 +30,6 @@ class ApplicationController < ActionController::Base
init_gettext "ovirt"
layout :choose_layout
- # 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
- # pre_show
- # authorize_admin
- before_filter :pre_new, :only => [:new]
- before_filter :pre_create, :only => [:create]
- # the following is to facilitate transition to service layer
- before_filter :tmp_pre_update, :only => [:edit, :update, :destroy]
- before_filter :pre_show, :only => [:show]
- 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
@@ -83,39 +70,6 @@ class ApplicationController < ActionController::Base
protected
# permissions checking
- def pre_new
- end
- def pre_edit
- end
-
- # FIXME: remove these when service layer transition is complete
- def tmp_pre_update
- pre_edit
- end
- def tmp_authorize_admin
- authorize_admin
- end
- def pre_create
- end
- def pre_show
- end
-
- # 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
- authorize_action(Privilege::VM_CONTROL)
- end
- def authorize_admin
- authorize_action(Privilege::MODIFY)
- end
- def authorize_action(privilege)
- authorized!(privilege)
- true
- end
-
def handle_perm_error(error)
handle_error(:error => error, :status => :forbidden,
:title => "Access denied")
diff --git a/src/app/controllers/hardware_controller.rb b/src/app/controllers/hardware_controller.rb
index 56b6e08..4d93e2f 100644
--- a/src/app/controllers/hardware_controller.rb
+++ b/src/app/controllers/hardware_controller.rb
@@ -57,18 +57,13 @@ class HardwareController < PoolController
end
def json_view_tree
- json_tree_internal(Privilege::VIEW, :select_hardware_and_vm_pools)
+ json_tree_internal(:svc_show, :select_hardware_and_vm_pools)
end
def json_move_tree
- json_tree_internal(Privilege::MODIFY, :select_hardware_pools)
- end
- def json_tree_internal(privilege, filter_method)
- id = params[:id]
- if id
- @pool = Pool.find(id)
- set_perms(@pool)
- return unless authorize_action(privilege)
- end
+ json_tree_internal(:svc_modify, :select_hardware_pools)
+ end
+ def json_tree_internal(perm_method, filter_method)
+ self.send(perm_method, params[:id]) if params[:id]
if @pool
pools = @pool.children
open_list = []
@@ -232,4 +227,9 @@ class HardwareController < PoolController
render :layout => 'popup'
end
+ def addhost
+ svc_modify(params[:id])
+ render :layout => 'popup'
+ end
+
end
diff --git a/src/app/controllers/host_controller.rb b/src/app/controllers/host_controller.rb
index 9b683fa..0d8a879 100644
--- a/src/app/controllers/host_controller.rb
+++ b/src/app/controllers/host_controller.rb
@@ -56,45 +56,11 @@ class HostController < ApplicationController
def snapshot_graph
end
- def addhost
- # FIXME: This probably should go into PoolService.svc_modify,
- # so that we have permission checks in only one place
-
- # 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
@pool = SmartPool.find(params[:smart_pool_id])
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
-
- def create
- end
-
- def edit
- end
-
- def update
- end
-
- def destroy
- end
-
def host_action
action = params[:action_type]
if["disable", "enable", "clear_vms"].include?(action)
@@ -144,25 +110,4 @@ class HostController < ApplicationController
render :json => bondings.collect{ |x| {:id => x.id, :name => x.name} }
end
- private
- #filter methods
- def pre_new
- flash[:notice] = 'Hosts may not be edited via the web UI'
- redirect_to :controller => 'hardware', :action => 'show', :id => params[:hardware_pool_id]
- end
- def pre_create
- flash[:notice] = 'Hosts may not be edited via the web UI'
- redirect_to :controller => 'dashboard'
- end
- def pre_edit
- @host = Host.find(params[:id])
- flash[:notice] = 'Hosts may not be edited via the web UI'
- redirect_to :action=> 'show', :id => @host
- end
- def pre_show
- @host = Host.find(params[:id])
- set_perms(@host.hardware_pool)
- end
-
-
end
diff --git a/src/app/controllers/network_controller.rb b/src/app/controllers/network_controller.rb
index 5af0a73..9066724 100644
--- a/src/app/controllers/network_controller.rb
+++ b/src/app/controllers/network_controller.rb
@@ -175,13 +175,4 @@ class NetworkController < ApplicationController
alert = svc_destroy_bonding(params[:id])
render :json => {:object => "bonding", :success => true, :alert => alert}
end
-
- protected
- # 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
- end
- def tmp_authorize_admin
- end
-
end
diff --git a/src/app/controllers/nic_controller.rb b/src/app/controllers/nic_controller.rb
index 8387f1c..2f6a2e5 100644
--- a/src/app/controllers/nic_controller.rb
+++ b/src/app/controllers/nic_controller.rb
@@ -26,32 +26,4 @@ class NicController < ApplicationController
def show
svc_show(params[:id])
end
-
- def new
- raise ActionError.new("Network Interfaces may not be edited via the web UI")
- end
-
- def create
- raise ActionError.new("Network Interfaces may not be edited via the web UI")
- end
-
- def edit
- raise ActionError.new("Network Interfaces may not be edited via the web UI")
- end
-
- def update
- raise ActionError.new("Network Interfaces may not be edited via the web UI")
- end
-
- def destroy
- raise ActionError.new("Network Interfaces may not be edited via the web UI")
- 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
- end
- def tmp_authorize_admin
- end
-
end
diff --git a/src/app/controllers/permission_controller.rb b/src/app/controllers/permission_controller.rb
index 55e7942..4447e2b 100644
--- a/src/app/controllers/permission_controller.rb
+++ b/src/app/controllers/permission_controller.rb
@@ -91,11 +91,4 @@ class PermissionController < ApplicationController
alert = svc_destroy(params[:id])
render :json => { :object => "vm", :success => true, :alert => alert }
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
- end
- def tmp_authorize_admin
- end
end
diff --git a/src/app/controllers/pool_controller.rb b/src/app/controllers/pool_controller.rb
index 5c5949a..1a65718 100644
--- a/src/app/controllers/pool_controller.rb
+++ b/src/app/controllers/pool_controller.rb
@@ -166,11 +166,4 @@ class PoolController < ApplicationController
def get_parent_id
params[:parent_id]
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
- end
- def tmp_authorize_admin
- end
-
end
diff --git a/src/app/controllers/quota_controller.rb b/src/app/controllers/quota_controller.rb
index fcdd672..07c6121 100644
--- a/src/app/controllers/quota_controller.rb
+++ b/src/app/controllers/quota_controller.rb
@@ -51,12 +51,4 @@ class QuotaController < ApplicationController
alert = svc_destroy(params[:id])
render :json => { :object => "quota", :success => true, :alert => alert }
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
- end
- def tmp_authorize_admin
- end
-
end
diff --git a/src/app/controllers/smart_pools_controller.rb b/src/app/controllers/smart_pools_controller.rb
index 83e72b5..8762ac0 100644
--- a/src/app/controllers/smart_pools_controller.rb
+++ b/src/app/controllers/smart_pools_controller.rb
@@ -142,7 +142,7 @@ class SmartPoolsController < PoolController
class_and_ids = class_and_ids_str.split(",").collect do |class_and_id_str|
class_and_id = class_and_id_str.split("_")
class_and_id[0] = class_and_id[0].constantize
- class_and_id[1] = class_and_id[1].to_a
+ class_and_id
end
alert = svc_add_remove_items(params[:id], nil, :add, class_and_ids)
diff --git a/src/app/controllers/storage_controller.rb b/src/app/controllers/storage_controller.rb
index 4cc4953..dac2a6b 100644
--- a/src/app/controllers/storage_controller.rb
+++ b/src/app/controllers/storage_controller.rb
@@ -157,12 +157,4 @@ class StorageController < ApplicationController
format.xml { head(:ok) }
end
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
- end
- def tmp_authorize_admin
- end
-
end
diff --git a/src/app/controllers/storage_volume_controller.rb b/src/app/controllers/storage_volume_controller.rb
index b87f9b6..b89513a 100644
--- a/src/app/controllers/storage_volume_controller.rb
+++ b/src/app/controllers/storage_volume_controller.rb
@@ -66,12 +66,4 @@ class StorageVolumeController < ApplicationController
format.xml { head(:ok) }
end
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
- end
- def tmp_authorize_admin
- end
-
end
diff --git a/src/app/controllers/vm_controller.rb b/src/app/controllers/vm_controller.rb
index b51f4ae..b2965af 100644
--- a/src/app/controllers/vm_controller.rb
+++ b/src/app/controllers/vm_controller.rb
@@ -135,10 +135,7 @@ class VmController < ApplicationController
end
def migrate
- @vm = Vm.find(params[:id])
- @current_pool_id=@vm.vm_resource_pool.id
- set_perms(@vm.get_hardware_pool)
- authorize_admin
+ svc_get_for_migrate(params[:id])
render :layout => 'popup'
end
@@ -173,12 +170,4 @@ class VmController < ApplicationController
#if cobbler doesn't respond/is misconfigured/etc just don't add profiles
end
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
- end
- def tmp_authorize_admin
- end
-
end
diff --git a/src/app/services/hardware_pool_service.rb b/src/app/services/hardware_pool_service.rb
index 28040f3..8b53515 100644
--- a/src/app/services/hardware_pool_service.rb
+++ b/src/app/services/hardware_pool_service.rb
@@ -90,7 +90,6 @@ module HardwarePoolService
svc_move_items_internal(pool_id, StoragePool, storage_pool_ids, target_pool_id)
end
def svc_move_items_internal(pool_id, item_class, resource_ids, target_pool_id)
- # from before_filter
target_pool = Pool.find(target_pool_id)
authorized!(Privilege::MODIFY,target_pool)
lookup(pool_id, Privilege::MODIFY)
diff --git a/src/app/services/pool_service.rb b/src/app/services/pool_service.rb
index c05b0dc..02c63f5 100644
--- a/src/app/services/pool_service.rb
+++ b/src/app/services/pool_service.rb
@@ -55,7 +55,6 @@ module PoolService
# === Required permissions
# [<tt>Privilege::MODIFY</tt>] for the pool
def svc_update(id, pool_hash)
- # from before_filter
svc_modify(id)
Pool.transaction do
additional_update_actions(@pool, pool_hash)
diff --git a/src/app/services/smart_pool_service.rb b/src/app/services/smart_pool_service.rb
index 67adfe0..28bdaf3 100644
--- a/src/app/services/smart_pool_service.rb
+++ b/src/app/services/smart_pool_service.rb
@@ -96,7 +96,8 @@ module SmartPoolService
raise PartialSuccessError.new("#{item_action.to_s} #{item_class.table_name.humanize if item_class} only partially successful",
failed_resources, successful_resources)
end
- return "#{item_action.to_s} #{item_class.table_name.humanize} successful."
+ return "#{item_action.to_s} #{item_class.nil? ? 'items' :
+ item_class.table_name.humanize} successful."
end
end
diff --git a/src/app/services/vm_resource_pool_service.rb b/src/app/services/vm_resource_pool_service.rb
index 00cf982..8930a4b 100644
--- a/src/app/services/vm_resource_pool_service.rb
+++ b/src/app/services/vm_resource_pool_service.rb
@@ -73,7 +73,6 @@ module VmResourcePoolService
# permission is action-specific as determined by
# <tt>VmTask.action_privilege(@action)</tt>
def svc_vm_actions(pool_id, vm_action, vm_ids)
- # from before_filter
@pool = VmResourcePool.find(pool_id)
@parent = @pool.parent
@action = vm_action
diff --git a/src/app/services/vm_service.rb b/src/app/services/vm_service.rb
index fb58b51..4c29bcf 100644
--- a/src/app/services/vm_service.rb
+++ b/src/app/services/vm_service.rb
@@ -71,7 +71,6 @@ module VmService
# === 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
@vm = Vm.new(vm_hash)
authorized!(Privilege::MODIFY, at vm.vm_resource_pool)
@@ -106,9 +105,7 @@ module VmService
# === 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(id)
- authorized!(Privilege::MODIFY, @vm.vm_resource_pool)
+ lookup(id,Privilege::MODIFY)
#needs restart if certain fields are changed
# (since those will only take effect the next startup)
@@ -169,9 +166,7 @@ module VmService
# === Required permissions
# [<tt>Privilege::MODIFY</tt>] for the Vm's VmResourcePool
def svc_destroy(id)
- # from before_filter
- @vm = Vm.find(id)
- authorized!(Privilege::MODIFY, @vm.vm_resource_pool)
+ lookup(id,Privilege::MODIFY)
unless @vm.is_destroyable?
raise ActionError.new("Virtual Machine must be stopped to delete it")
@@ -205,8 +200,7 @@ module VmService
# === 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)
+ lookup(id,Privilege::MODIFY)
Task.transaction do
@vm.tasks.queued.each { |task| task.cancel}
@@ -214,6 +208,18 @@ module VmService
return "Queued tasks were successfully canceled."
end
+ # Retrieves the Vm with id +id+ and checks permissions for migrate
+ #
+ # === Instance variables
+ # [<tt>@vm</tt>] stores the Vm with +id+
+ # === Required permissions
+ # [<tt>Privilege::MODIFY</tt>] for the Vm's HardwarePool
+ def svc_get_for_migrate(id)
+ @vm = Vm.find(id)
+ @current_pool_id=@vm.vm_resource_pool.id
+ authorized!(Privilege::MODIFY, @vm.get_hardware_pool)
+ end
+
protected
def vm_provision
if @vm.uses_cobbler?
diff --git a/src/app/views/host/addhost.html.erb b/src/app/views/hardware/addhost.rhtml
similarity index 76%
rename from src/app/views/host/addhost.html.erb
rename to src/app/views/hardware/addhost.rhtml
index 967643e..7279731 100644
--- a/src/app/views/host/addhost.html.erb
+++ b/src/app/views/hardware/addhost.rhtml
@@ -2,14 +2,14 @@
<%= _("Add Host") %>
<%- end -%>
<%- content_for :description do -%>
- Select hosts from the list below to add to the <%= @hardware_pool.name %> hardware pool. <a href="#">Learn how to manage hosts</a>
+ Select hosts from the list below to add to the <%= @pool.name %> hardware pool. <a href="#">Learn how to manage hosts</a>
<%- end -%>
<div id="dialog-content-area">
<div class="dialog_body_small">
<div class="panel_header"></div>
<%= render :partial => "/host/grid", :locals => { :table_id => "addhosts_grid",
:hwpool => nil,
- :exclude_pool => @hardware_pool.id,
+ :exclude_pool => @pool.id,
:exclude_host => nil,
:checkboxes => true,
:on_select => "load_widget_select",
@@ -20,8 +20,8 @@
:hosts_per_page => 10} %>
</div>
-<%= popup_footer("add_hosts('#{url_for :controller => "hardware",
+<%= popup_footer("add_hosts('#{url_for :controller => "hardware",
:action => "add_hosts",
- :id => @hardware_pool}')",
+ :id => @pool}')",
"Add Hosts") %>
</div>
diff --git a/src/app/views/hardware/show_hosts.rhtml b/src/app/views/hardware/show_hosts.rhtml
index 09a6188..fb54373 100644
--- a/src/app/views/hardware/show_hosts.rhtml
+++ b/src/app/views/hardware/show_hosts.rhtml
@@ -1,7 +1,7 @@
<div id="toolbar_nav">
<ul>
<%if @can_modify -%>
- <li><a href="<%= url_for :controller => 'host', :action => 'addhost', :hardware_pool_id => @pool %>" rel="facebox[.bolder]"><%= image_tag "icon_add_host.png", :style=>"vertical-align:middle;" %> Add Host</a></li>
+ <li><a href="<%= url_for :controller => 'hardware', :action => 'addhost', :id => @pool %>" rel="facebox[.bolder]"><%= image_tag "icon_add_host.png", :style=>"vertical-align:middle;" %> Add Host</a></li>
<li>
<a id="move_link" href="#" onClick="return validate_for_move();"><%= image_tag "icon_move.png", :style=>"vertical-align:middle;" %> Move</a>
<a id="move_link_hidden" href="<%= url_for :controller => 'hardware', :action => 'move', :id => @pool, :resource_type=>'hosts' %>" rel="facebox[.bolder]" style="display:none" ></a>
@@ -115,7 +115,7 @@
No hosts found in this pool. <br/><br/>
<%if @can_modify -%>
<%= image_tag "icon_add_host.png", :style=>"vertical-align:middle;" %>
- <a href="<%= url_for :controller => 'host', :action => 'addhost', :hardware_pool_id => @pool %>" rel="facebox[.bolder]">Add first host to this hardware pool</a>
+ <a href="<%= url_for :controller => 'hardware', :action => 'addhost', :id => @pool %>" rel="facebox[.bolder]">Add first host to this hardware pool</a>
<% end -%>
</div>
</div>
--
1.6.0.6
More information about the ovirt-devel
mailing list