Scott Seago
2009-Mar-30 19:08 UTC
[Ovirt-devel] [PATCH server] first round of permissions refactoring.
pulled out the roles and privileges from the ruby hashes embedded in
permission.rb into separate DB tables. This allows the eventual creation of
higher-level APIs to create custom roles, although for now the behavior of the
system should be unchanged from before the patch.
Signed-off-by: Scott Seago <sseago at redhat.com>
---
src/app/controllers/hardware_controller.rb | 8 +-
src/app/controllers/permission_controller.rb | 3 +-
src/app/controllers/pool_controller.rb | 4 +-
src/app/controllers/resources_controller.rb | 2 +-
src/app/controllers/tree_controller.rb | 8 +-
src/app/controllers/vm_controller.rb | 14 +--
src/app/models/directory_pool.rb | 2 +-
src/app/models/permission.rb | 54 +--------
src/app/models/pool.rb | 54 ++++------
.../unit/task_test.rb => app/models/privilege.rb} | 31 ++----
src/{test/unit/task_test.rb => app/models/role.rb} | 32 ++----
src/app/models/smart_pool.rb | 2 +-
src/app/models/vm_task.rb | 18 ++--
src/app/views/permission/_form.rhtml | 2 +-
src/app/views/permission/_list.rhtml | 2 +-
src/app/views/user/_change_role_menu.rhtml | 4 +-
src/app/views/user/_grid.rhtml | 2 +-
src/app/views/user/_show.rhtml | 2 +-
src/db/migrate/037_add_roles_and_privileges.rb | 119 ++++++++++++++++++++
src/lib/tasks/vm-states-dot.rake | 1 +
src/script/grant_admin_privileges | 2 +-
src/test/fixtures/permissions.yml | 30 +++---
src/test/fixtures/privileges.yml | 10 ++
src/test/fixtures/roles.yml | 12 ++
src/test/functional/host_controller_test.rb | 2 +-
src/test/functional/nic_controller_test.rb | 2 +-
src/test/functional/permission_controller_test.rb | 8 +-
src/test/functional/quota_controller_test.rb | 6 +-
src/test/functional/resources_controller_test.rb | 2 +-
src/test/functional/storage_controller_test.rb | 9 +-
src/test/functional/vm_controller_test.rb | 10 +-
src/test/unit/active_record_env_test.rb | 4 +-
src/test/unit/permission_test.rb | 16 +--
src/test/unit/task_test.rb | 2 +-
34 files changed, 272 insertions(+), 207 deletions(-)
copy src/{test/unit/task_test.rb => app/models/privilege.rb} (50%)
copy src/{test/unit/task_test.rb => app/models/role.rb} (50%)
create mode 100644 src/db/migrate/037_add_roles_and_privileges.rb
create mode 100644 src/test/fixtures/privileges.yml
create mode 100644 src/test/fixtures/roles.yml
diff --git a/src/app/controllers/hardware_controller.rb
b/src/app/controllers/hardware_controller.rb
index 3caf024..726e5bd 100644
--- a/src/app/controllers/hardware_controller.rb
+++ b/src/app/controllers/hardware_controller.rb
@@ -61,10 +61,10 @@ class HardwareController < PoolController
end
def json_view_tree
- json_tree_internal(Permission::PRIV_VIEW, :select_hardware_and_vm_pools)
+ json_tree_internal(Privilege::VIEW, :select_hardware_and_vm_pools)
end
def json_move_tree
- json_tree_internal(Permission::PRIV_MODIFY, :select_hardware_pools)
+ json_tree_internal(Privilege::MODIFY, :select_hardware_pools)
end
def json_tree_internal(privilege, filter_method)
id = params[:id]
@@ -81,7 +81,7 @@ class HardwareController < PoolController
pools = @pool.children
open_list = []
else
- pools = Pool.list_for_user(get_login_user,Permission::PRIV_VIEW)
+ pools = Pool.list_for_user(get_login_user,Privilege::VIEW)
hw_root = HardwarePool.get_default_pool
if !(pools.include?(hw_root))
if pools.include?(DirectoryPool.get_directory_root)
@@ -183,7 +183,7 @@ class HardwareController < PoolController
@resource_type = params[:resource_type]
@id = params[:id]
@pools = HardwarePool.get_default_pool.full_set_nested(:method =>
:json_hash_element,
- :privilege => Permission::PRIV_MODIFY, :user =>
get_login_user, :current_id => @id,
+ :privilege => Privilege::MODIFY, :user =>
get_login_user, :current_id => @id,
:type => :select_hardware_pools).to_json
render :layout => 'popup'
end
diff --git a/src/app/controllers/permission_controller.rb
b/src/app/controllers/permission_controller.rb
index c301e1e..3bfbffa 100644
--- a/src/app/controllers/permission_controller.rb
+++ b/src/app/controllers/permission_controller.rb
@@ -42,6 +42,7 @@ class PermissionController < ApplicationController
@pool = Pool.find(params[:pool_id])
filter = @pool.permissions.collect{ |permission| permission.uid }
@users = Account.names(filter)
+ @roles = Role.find(:all).collect{ |role| [role.name, role.id] }
set_perms(@permission.pool)
# admin permission required to view permissions
unless @can_set_perms
@@ -74,7 +75,7 @@ class PermissionController < ApplicationController
#FIXME: we need permissions checks. user must have permission. We also need
to fail
# for pools that aren't currently empty
def update_roles
- role = params[:user_role]
+ role = params[:role_id]
permission_ids_str = params[:permission_ids]
permission_ids = permission_ids_str.split(",").collect {|x|
x.to_i}
diff --git a/src/app/controllers/pool_controller.rb
b/src/app/controllers/pool_controller.rb
index 2809d6d..22cf1a4 100644
--- a/src/app/controllers/pool_controller.rb
+++ b/src/app/controllers/pool_controller.rb
@@ -57,14 +57,14 @@ class PoolController < ApplicationController
# resource's users list page
def show_users
- @roles = Permission::ROLES.keys
+ @roles = Role.find(:all).collect{ |role| [role.name, role.id] }
show
end
def users_json
attr_list = []
attr_list << :grid_id if params[:checkboxes]
- attr_list += [:uid, :user_role, :source]
+ attr_list += [:uid, [:role, :name], :source]
json_list(@pool.permissions, attr_list)
end
diff --git a/src/app/controllers/resources_controller.rb
b/src/app/controllers/resources_controller.rb
index 7bed533..b57fae6 100644
--- a/src/app/controllers/resources_controller.rb
+++ b/src/app/controllers/resources_controller.rb
@@ -31,7 +31,7 @@ class ResourcesController < PoolController
def list
@user = get_login_user
- @vm_resource_pools = VmResourcePool.list_for_user(@user,
Permission::PRIV_VIEW)
+ @vm_resource_pools = VmResourcePool.list_for_user(@user, Privilege::VIEW)
@vms = Set.new
@vm_resource_pools.each { |vm_resource_pool| @vms += vm_resource_pool.vms}
@vms = @vms.entries
diff --git a/src/app/controllers/tree_controller.rb
b/src/app/controllers/tree_controller.rb
index b6e7404..365eaf0 100644
--- a/src/app/controllers/tree_controller.rb
+++ b/src/app/controllers/tree_controller.rb
@@ -2,9 +2,9 @@ class TreeController < ApplicationController
def get_pools
# TODO: split these into separate hash elements for HW and smart pools
pools = HardwarePool.get_default_pool.full_set_nested(:method =>
:json_hash_element,
- :privilege => Permission::PRIV_VIEW, :user =>
get_login_user)
+ :privilege => Privilege::VIEW, :user =>
get_login_user)
pools += DirectoryPool.get_smart_root.full_set_nested(:method =>
:json_hash_element,
- :privilege => Permission::PRIV_VIEW, :user =>
get_login_user,
+ :privilege => Privilege::VIEW, :user =>
get_login_user,
:smart_pool_set => true)
end
@@ -32,13 +32,13 @@ class TreeController < ApplicationController
pools = build_json(
HardwarePool.get_default_pool.full_set_nested(
:method => :json_hash_element,
- :privilege => Permission::PRIV_VIEW,
+ :privilege => Privilege::VIEW,
:user => get_login_user))
smart_pools = adjust_smart_pool_list(
build_json(
DirectoryPool.get_smart_root.full_set_nested(
:method => :json_hash_element,
- :privilege => Permission::PRIV_VIEW,
+ :privilege => Privilege::VIEW,
:user => get_login_user,
:smart_pool_set => true)))
@serverHash = {:pools => smart_pools + pools }
diff --git a/src/app/controllers/vm_controller.rb
b/src/app/controllers/vm_controller.rb
index aa575c6..a81f6b5 100644
--- a/src/app/controllers/vm_controller.rb
+++ b/src/app/controllers/vm_controller.rb
@@ -26,14 +26,12 @@ class VmController < ApplicationController
before_filter :pre_vm_action, :only => [:vm_action, :cancel_queued_tasks,
:console]
def index
- roles = "('" +
-
Permission::roles_for_privilege(Permission::PRIV_VIEW).join("',
'") +
- "')"
- user = get_login_user
- @vms = Vm.find(:all,
- :joins => "join permissions p on (vm_resource_pool_id =
p.pool_id)",
- :conditions => [ "p.uid = :user and p.user_role in
#{roles}",
- { :user => user }])
+ @vms = Vm.find(:all,
+ :include => [{:vm_resource_pool =>
+ {:permissions => {:role =>
:privileges}}}],
+ :conditions => ["privileges.name=:priv
+ and permissions.uid=:user",
+ { :user => get_login_user, :priv =>
Privilege::VIEW }])
respond_to do |format|
format.xml { render :xml => @vms.to_xml(:include => :host) }
end
diff --git a/src/app/models/directory_pool.rb b/src/app/models/directory_pool.rb
index 82486af..91645c1 100644
--- a/src/app/models/directory_pool.rb
+++ b/src/app/models/directory_pool.rb
@@ -50,7 +50,7 @@ class DirectoryPool < Pool
user_root.create_with_parent(get_smart_root)
permission = Permission.new({:pool_id => user_root.id,
:uid => user,
- :user_role =>
Permission::ROLE_SUPER_ADMIN})
+ :role_id =>
Role.find_by_name(Role::SUPER_ADMIN).id})
#we don't need save_with_new_children here since there are no
children yet
permission.save!
end
diff --git a/src/app/models/permission.rb b/src/app/models/permission.rb
index b3929ad..2567b08 100644
--- a/src/app/models/permission.rb
+++ b/src/app/models/permission.rb
@@ -24,62 +24,20 @@ class Permission < ActiveRecord::Base
has_many :child_permissions, :dependent => :destroy,
:class_name => "Permission", :foreign_key =>
"inherited_from_id"
+ belongs_to :role
+
validates_presence_of :pool_id
+ validates_presence_of :role_id
validates_presence_of :uid
validates_uniqueness_of :uid, :scope => [:pool_id, :inherited_from_id]
-
- ROLE_SUPER_ADMIN = "Super Admin"
- ROLE_ADMIN = "Administrator"
- ROLE_USER = "User"
- ROLE_MONITOR = "Monitor"
-
- PRIV_PERM_SET = "set_perms"
- PRIV_PERM_VIEW = "view_perms"
- PRIV_MODIFY = "modify"
- PRIV_VM_CONTROL = "vm_control"
- PRIV_VIEW = "view"
-
- ROLES = { ROLE_SUPER_ADMIN => [PRIV_VIEW, PRIV_VM_CONTROL, PRIV_MODIFY,
- PRIV_PERM_VIEW, PRIV_PERM_SET],
- ROLE_ADMIN => [PRIV_VIEW, PRIV_VM_CONTROL, PRIV_MODIFY],
- ROLE_USER => [PRIV_VIEW, PRIV_VM_CONTROL],
- ROLE_MONITOR => [PRIV_VIEW]}
-
-
- validates_inclusion_of :user_role,
- :in => ROLES.keys
-
- def self.invert_roles
- return_hash = {}
- ROLES.each do |role, privs|
- privs.each do |priv|
- priv_key = return_hash[priv]
- priv_key ||= []
- priv_key << role
- return_hash[priv] = priv_key
- end
- end
- return_hash
- end
-
def name
@account ||= Account.find("uid=#{uid}")
@account.cn
end
- PRIVILEGES = self.invert_roles
-
- def self.privileges_for_role(role)
- ROLES[role]
- end
-
- def self.roles_for_privilege(privilege)
- PRIVILEGES[privilege]
- end
-
def is_primary?
inherited_from_id.nil?
end
@@ -94,10 +52,10 @@ class Permission < ActiveRecord::Base
end
def update_role(new_role)
self.transaction do
- self.user_role = new_role
+ self.role_id = new_role
self.save!
child_permissions.each do |permission|
- permission.user_role = new_role
+ permission.role_id = new_role
permission.save!
end
end
@@ -108,7 +66,7 @@ class Permission < ActiveRecord::Base
pool.all_children.each do |subpool|
new_permission = Permission.new({:pool_id => subpool.id,
:uid => uid,
- :user_role => user_role,
+ :role_id => role_id,
:inherited_from_id => id})
new_permission.save!
end
diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
index 372cf54..6f2a086 100644
--- a/src/app/models/pool.rb
+++ b/src/app/models/pool.rb
@@ -23,7 +23,7 @@ class Pool < ActiveRecord::Base
has_many :membership_audit_events, :as => :container_target, :dependent
=> :destroy
# moved associations here so that nested set :include directives work
# TODO: find a way to put this back into vm_resource_pool.rb
- has_many :vms, :dependent => :nullify, :order => "id ASC",
:foreign_key => :vm_resource_pool_id
+ has_many :vms, :dependent => :nullify, :order => "vms.id
ASC", :foreign_key => :vm_resource_pool_id
# TODO: find a way to put this back into hardware_pool.rb
has_many :hosts, :include => :nics, :dependent => :nullify, :order
=> "hosts.id ASC", :foreign_key => "hardware_pool_id"
do
def total_cpus
@@ -37,7 +37,7 @@ class Pool < ActiveRecord::Base
end
end
- has_many :storage_pools, :dependent => :nullify, :order => "id
ASC", :foreign_key => 'hardware_pool_id' do
+ has_many :storage_pools, :dependent => :nullify, :order =>
"storage_pools.id ASC", :foreign_key => 'hardware_pool_id'
do
def total_size_in_gb
find(:all).inject(0){ |sum, sp| sum + sp.storage_volumes.total_size_in_gb
}
end
@@ -57,20 +57,7 @@ class Pool < ActiveRecord::Base
:in => %w( DirectoryPool HardwarePool VmResourcePool SmartPool )
# overloading this method such that we can use permissions.admins to get all
the admins for an object
- has_many :permissions, :dependent => :destroy, :order => "id
ASC" do
- def super_admins
- find_all_by_user_role(Permission::ROLE_SUPER_ADMIN)
- end
- def admins
- find_all_by_user_role(Permission::ROLE_ADMIN)
- end
- def users
- find_all_by_user_role(Permission::ROLE_USER)
- end
- def monitors
- find_all_by_user_role(Permission::ROLE_MONITOR)
- end
- end
+ has_many :permissions, :dependent => :destroy, :include => :role,
:order => "permissions.id ASC"
has_one :quota, :dependent => :destroy
@@ -81,7 +68,7 @@ class Pool < ActiveRecord::Base
parent.permissions.each do |permission|
new_permission = Permission.new({:pool_id => id,
:uid => permission.uid,
- :user_role =>
permission.user_role,
+ :role_id => permission.role_id,
:inherited_from_id =>
permission.inherited_from_id.nil? ?
permission.id :
@@ -104,10 +91,10 @@ class Pool < ActiveRecord::Base
else
inherited_clause = "and permissions.inherited_from_id is null"
end
- pools = find(:all, :include => "permissions",
- :conditions => "permissions.uid='#{user}'
#{inherited_clause} and
- permissions.user_role in
-
('#{Permission.roles_for_privilege(privilege).join("',
'")}')")
+ pools = find(:all, :include => [{:permissions => {:role =>
:privileges}}],
+ :conditions => ["permissions.uid=:user
#{inherited_clause} and
+ privileges.name=:priv",
+ { :user => user, :priv => privilege }])
end
def self.select_hardware_pools(pools)
@@ -142,26 +129,26 @@ class Pool < ActiveRecord::Base
end
def can_view(user)
- has_privilege(user, Permission::PRIV_VIEW)
+ has_privilege(user, Privilege::VIEW)
end
def can_control_vms(user)
- has_privilege(user, Permission::PRIV_VM_CONTROL)
+ has_privilege(user, Privilege::VM_CONTROL)
end
def can_modify(user)
- has_privilege(user, Permission::PRIV_MODIFY)
+ has_privilege(user, Privilege::MODIFY)
end
def can_view_perms(user)
- has_privilege(user, Permission::PRIV_PERM_VIEW)
+ has_privilege(user, Privilege::PERM_VIEW)
end
def can_set_perms(user)
- has_privilege(user, Permission::PRIV_PERM_SET)
+ has_privilege(user, Privilege::PERM_SET)
end
def has_privilege(user, privilege)
- permissions.find(:first,
- :conditions =>
"permissions.uid='#{user}' and
- permissions.user_role in
-
('#{Permission.roles_for_privilege(privilege).join("',
'")}')")
+ permissions.find(:first, :include => [:role => :privileges],
+ :conditions => ["permissions.uid=:user and
+ privileges.name=:priv",
+ { :user => user, :priv => privilege
}])
end
def total_resources
@@ -239,10 +226,9 @@ class Pool < ActiveRecord::Base
type = opts.delete(:type)
smart_pool_set = opts.delete(:smart_pool_set)
if privilege and user
- opts[:include] = "permissions"
- opts[:conditions] = "permissions.uid='#{user}' and
- permissions.user_role in
-
('#{Permission.roles_for_privilege(privilege).join("',
'")}')"
+ opts[:include] = [{:permissions => {:role => :privileges}}]
+ opts[:conditions] = ["permissions.uid=:user and
privileges.name=:priv",
+ { :user => user, :priv => privilege }]
end
current_id = opts.delete(:current_id)
opts.delete(:order)
diff --git a/src/test/unit/task_test.rb b/src/app/models/privilege.rb
similarity index 50%
copy from src/test/unit/task_test.rb
copy to src/app/models/privilege.rb
index ba780e7..7a30b8f 100644
--- a/src/test/unit/task_test.rb
+++ b/src/app/models/privilege.rb
@@ -1,5 +1,5 @@
#
-# Copyright (C) 2008 Red Hat, Inc.
+# Copyright (C) 2009 Red Hat, Inc.
# Written by Scott Seago <sseago at redhat.com>
#
# This program is free software; you can redistribute it and/or modify
@@ -17,28 +17,19 @@
# MA 02110-1301, USA. A copy of the GNU General Public License is
# also available at http://www.gnu.org/copyleft/gpl.html.
-require File.dirname(__FILE__) + '/../test_helper'
+class Privilege < ActiveRecord::Base
+ has_and_belongs_to_many :roles
-class TaskTest < Test::Unit::TestCase
- fixtures :pools, :hosts, :vms, :permissions, :tasks
+ validates_presence_of :name
+ validates_uniqueness_of :name
- def setup
- @task = Task.new( :type => 'HostTask', :state =>
'finished' )
- end
- def test_valid_fails_with_bad_type
- @task.type = 'foobar'
- flunk 'Task must specify valid type' if @task.valid?
- end
+ #default privileges
+ PERM_SET = "set_perms"
+ PERM_VIEW = "view_perms"
+ MODIFY = "modify"
+ VM_CONTROL = "vm_control"
+ VIEW = "view"
- def test_valid_fails_with_bad_state
- @task.state = 'foobar'
- flunk 'Task must specify valid state' if @task.valid?
- end
-
- def test_exercise_task_relationships
- assert_equal
tasks(:shutdown_production_httpd_appliance_task).task_target.vm_resource_pool.name,
'corp.com production vmpool'
- assert_equal
tasks(:shutdown_production_httpd_appliance_task).task_target.host.hostname,
'prod.corp.com'
- end
end
diff --git a/src/test/unit/task_test.rb b/src/app/models/role.rb
similarity index 50%
copy from src/test/unit/task_test.rb
copy to src/app/models/role.rb
index ba780e7..969fbbe 100644
--- a/src/test/unit/task_test.rb
+++ b/src/app/models/role.rb
@@ -1,5 +1,5 @@
#
-# Copyright (C) 2008 Red Hat, Inc.
+# Copyright (C) 2009 Red Hat, Inc.
# Written by Scott Seago <sseago at redhat.com>
#
# This program is free software; you can redistribute it and/or modify
@@ -17,28 +17,18 @@
# MA 02110-1301, USA. A copy of the GNU General Public License is
# also available at http://www.gnu.org/copyleft/gpl.html.
-require File.dirname(__FILE__) + '/../test_helper'
+class Role < ActiveRecord::Base
+ has_many :permissions
+ has_and_belongs_to_many :privileges
-class TaskTest < Test::Unit::TestCase
- fixtures :pools, :hosts, :vms, :permissions, :tasks
+ validates_presence_of :name
+ validates_uniqueness_of :name
- def setup
- @task = Task.new( :type => 'HostTask', :state =>
'finished' )
- end
- def test_valid_fails_with_bad_type
- @task.type = 'foobar'
- flunk 'Task must specify valid type' if @task.valid?
- end
-
- def test_valid_fails_with_bad_state
- @task.state = 'foobar'
- flunk 'Task must specify valid state' if @task.valid?
- end
-
- def test_exercise_task_relationships
- assert_equal
tasks(:shutdown_production_httpd_appliance_task).task_target.vm_resource_pool.name,
'corp.com production vmpool'
- assert_equal
tasks(:shutdown_production_httpd_appliance_task).task_target.host.hostname,
'prod.corp.com'
- end
+ #default roles
+ SUPER_ADMIN = "Super Admin"
+ ADMIN = "Administrator"
+ USER = "User"
+ MONITOR = "Monitor"
end
diff --git a/src/app/models/smart_pool.rb b/src/app/models/smart_pool.rb
index ca55a2e..dba000d 100644
--- a/src/app/models/smart_pool.rb
+++ b/src/app/models/smart_pool.rb
@@ -74,7 +74,7 @@ class SmartPool < Pool
def self.smart_pools_for_user(user)
nested_pools = DirectoryPool.get_smart_root.full_set_nested(
- :privilege => Permission::PRIV_MODIFY, :user =>
user,
+ :privilege => Privilege::MODIFY, :user => user,
:smart_pool_set => true)[0][:children]
user_pools = []
other_pools = []
diff --git a/src/app/models/vm_task.rb b/src/app/models/vm_task.rb
index 27513bb..ed33564 100644
--- a/src/app/models/vm_task.rb
+++ b/src/app/models/vm_task.rb
@@ -47,7 +47,7 @@ class VmTask < Task
:running => Vm::STATE_CREATING,
:success => Vm::STATE_STOPPED,
:failure => Vm::STATE_CREATE_FAILED,
- :privilege =>
[Permission::PRIV_MODIFY,
+ :privilege => [Privilege::MODIFY,
PRIV_OBJECT_VM_POOL]},
ACTION_START_VM => { :label => "Start",
:icon => "icon_start.png",
@@ -55,7 +55,7 @@ class VmTask < Task
:running => Vm::STATE_STARTING,
:success => Vm::STATE_RUNNING,
:failure => Vm::STATE_STOPPED,
- :privilege =>
[Permission::PRIV_VM_CONTROL,
+ :privilege => [Privilege::VM_CONTROL,
PRIV_OBJECT_VM_POOL]},
ACTION_SHUTDOWN_VM => { :label => "Shutdown",
:icon => "icon_x.png",
@@ -63,7 +63,7 @@ class VmTask < Task
:running => Vm::STATE_STOPPING,
:success => Vm::STATE_STOPPED,
:failure => Vm::STATE_RUNNING,
- :privilege =>
[Permission::PRIV_VM_CONTROL,
+ :privilege => [Privilege::VM_CONTROL,
PRIV_OBJECT_VM_POOL]},
ACTION_POWEROFF_VM => { :label => "Poweroff",
:icon => "icon_x.png",
@@ -71,7 +71,7 @@ class VmTask < Task
:running => Vm::STATE_POWERING_OFF,
:success => Vm::STATE_STOPPED,
:failure => Vm::STATE_RUNNING,
- :privilege =>
[Permission::PRIV_VM_CONTROL,
+ :privilege => [Privilege::VM_CONTROL,
PRIV_OBJECT_VM_POOL]},
ACTION_SUSPEND_VM => { :label => "Suspend",
:icon =>
"icon_suspend.png",
@@ -79,7 +79,7 @@ class VmTask < Task
:running => Vm::STATE_SUSPENDING,
:success => Vm::STATE_SUSPENDED,
:failure => Vm::STATE_RUNNING,
- :privilege =>
[Permission::PRIV_VM_CONTROL,
+ :privilege => [Privilege::VM_CONTROL,
PRIV_OBJECT_VM_POOL]},
ACTION_RESUME_VM => { :label => "Resume",
:icon => "icon_start.png",
@@ -87,7 +87,7 @@ class VmTask < Task
:running => Vm::STATE_RESUMING,
:success => Vm::STATE_RUNNING,
:failure => Vm::STATE_SUSPENDED,
- :privilege =>
[Permission::PRIV_VM_CONTROL,
+ :privilege => [Privilege::VM_CONTROL,
PRIV_OBJECT_VM_POOL]},
ACTION_SAVE_VM => { :label => "Save",
:icon => "icon_save.png",
@@ -95,7 +95,7 @@ class VmTask < Task
:running => Vm::STATE_SAVING,
:success => Vm::STATE_SAVED,
:failure => Vm::STATE_RUNNING,
- :privilege =>
[Permission::PRIV_VM_CONTROL,
+ :privilege => [Privilege::VM_CONTROL,
PRIV_OBJECT_VM_POOL]},
ACTION_RESTORE_VM => { :label => "Restore",
:icon =>
"icon_restore.png",
@@ -103,7 +103,7 @@ class VmTask < Task
:running => Vm::STATE_RESTORING,
:success => Vm::STATE_RUNNING,
:failure => Vm::STATE_SAVED,
- :privilege =>
[Permission::PRIV_VM_CONTROL,
+ :privilege => [Privilege::VM_CONTROL,
PRIV_OBJECT_VM_POOL]},
ACTION_MIGRATE_VM => { :label => "Migrate",
:icon =>
"icon_restore.png",
@@ -111,7 +111,7 @@ class VmTask < Task
:running => Vm::STATE_MIGRATING,
:success => Vm::STATE_RUNNING,
:failure => Vm::STATE_RUNNING,
- :privilege =>
[Permission::PRIV_MODIFY,
+ :privilege => [Privilege::MODIFY,
PRIV_OBJECT_HW_POOL],
:popup_action => 'migrate'} }
diff --git a/src/app/views/permission/_form.rhtml
b/src/app/views/permission/_form.rhtml
index 2a1e93c..89d4e03 100644
--- a/src/app/views/permission/_form.rhtml
+++ b/src/app/views/permission/_form.rhtml
@@ -3,7 +3,7 @@
<!--[form:permission]-->
<%= hidden_field_with_label "Pool", 'permission',
'pool_id', @permission.pool.name %>
-<%= select_with_label 'Role', 'permission',
'user_role', Permission::ROLES.keys %>
+<%= select_with_label 'Role', 'permission',
'role_id', @roles %>
<%= select_with_label 'User', 'permission', 'uid',
@users %>
diff --git a/src/app/views/permission/_list.rhtml
b/src/app/views/permission/_list.rhtml
index 57613cb..713f193 100644
--- a/src/app/views/permission/_list.rhtml
+++ b/src/app/views/permission/_list.rhtml
@@ -12,7 +12,7 @@
<% for permission in permissions %>
<tr class="<%= cycle('odd','even') %>">
<td><%= permission.uid %>
- <td><%= permission.user_role %>
+ <td><%= permission.role.name %>
<%if show_object -%>
<td style="text-align:left;">
<%= link_to permission.pool.name,
diff --git a/src/app/views/user/_change_role_menu.rhtml
b/src/app/views/user/_change_role_menu.rhtml
index f35cd3b..4044b88 100644
--- a/src/app/views/user/_change_role_menu.rhtml
+++ b/src/app/views/user/_change_role_menu.rhtml
@@ -1,12 +1,12 @@
<%= image_tag "icon_move.png", :style =>
"vertical-align:middle;" %> Change Role
<%= image_tag
"icon_toolbar_arrow.gif", :style =>
"vertical-align:middle;" %>
<ul>
<% @roles.each_index { |index| %>
- <li
onClick="update_users('<%=@roles[index]%>')"
+ <li
onClick="update_users('<%=@roles[index][1]%>')"
<% if index == @roles.length - 1 %>
style="border-bottom: 1px solid black;"
<% end %>
>
- <%=@roles[index]%>
+ <%=@roles[index][0]%>
</li>
<% } %>
</ul>
diff --git a/src/app/views/user/_grid.rhtml b/src/app/views/user/_grid.rhtml
index 8f7b4cf..10a10c8 100644
--- a/src/app/views/user/_grid.rhtml
+++ b/src/app/views/user/_grid.rhtml
@@ -16,7 +16,7 @@
colModel : [
<%= "{display: '', width : 20, align: 'left',
process: #{table_id}checkbox}," if checkboxes %>
{display: 'Name', name : 'uid', width : 180, sortable :
true, align: 'left'},
- {display: 'Role', name : 'user_role', width : 80,
sortable : true, align: 'left'},
+ {display: 'Role', name : 'roles.name', width : 80,
sortable : true, align: 'left'},
{display: '', width : 80, sortable : true, align:
'left'}
],
sortname: "user",
diff --git a/src/app/views/user/_show.rhtml b/src/app/views/user/_show.rhtml
index 8ea423e..c7c8008 100644
--- a/src/app/views/user/_show.rhtml
+++ b/src/app/views/user/_show.rhtml
@@ -31,7 +31,7 @@
var permissions = get_selected_users();
if (validate_selected(permissions, "users")) {
$.post('<%= url_for :controller => "permission",
:action => "update_roles" %>',
- { permission_ids: permissions.toString(), user_role: role },
+ { permission_ids: permissions.toString(), role_id: role },
function(data,status){
$tabs.tabs("load",$tabs.data('selected.tabs'));
if (data.alert) {
diff --git a/src/db/migrate/037_add_roles_and_privileges.rb
b/src/db/migrate/037_add_roles_and_privileges.rb
new file mode 100644
index 0000000..322014e
--- /dev/null
+++ b/src/db/migrate/037_add_roles_and_privileges.rb
@@ -0,0 +1,119 @@
+#
+# Copyright (C) 2009 Red Hat, Inc.
+# Written by Scott Seago <sseago 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.
+
+class AddRolesAndPrivileges < ActiveRecord::Migration
+ def self.up
+# create_table :permissions do |t|
+# t.string :user_role
+# t.string :user
+# t.integer :pool_id
+# t.integer :inherited_from_id
+# t.integer :lock_version, :default => 0
+# end
+ create_table :roles do |t|
+ t.string :name
+ t.integer :lock_version, :default => 0
+ end
+ create_table :privileges do |t|
+ t.string :name
+ t.integer :lock_version, :default => 0
+ end
+
+ create_table :privileges_roles, :id => false do |t|
+ t.integer :privilege_id, :null => false
+ t.integer :role_id, :null => false
+ end
+ execute "alter table privileges_roles add constraint
+ fk_priv_roles_role_id
+ foreign key (role_id) references roles(id)"
+ execute "alter table privileges_roles add constraint
+ fk_priv_roles_priv_id
+ foreign key (privilege_id) references privileges(id)"
+
+ add_column :permissions, :role_id, :integer
+ execute "alter table permissions add constraint fk_perm_roles
+ foreign key (role_id) references roles(id)"
+
+ #create default roles and privileges
+ Role.transaction do
+ role_super_admin = Role.new({:name => "Super Admin"})
+ role_super_admin.save!
+ role_admin = Role.new({:name => "Administrator"})
+ role_admin.save!
+ role_user = Role.new({:name => "User"})
+ role_user.save!
+ role_monitor = Role.new({:name => "Monitor"})
+ role_monitor.save!
+
+ priv_perm_set = Privilege.new({:name => "set_perms"})
+ priv_perm_set.save!
+ priv_perm_view = Privilege.new({:name => "view_perms"})
+ priv_perm_view.save!
+ priv_modify = Privilege.new({:name => "modify"})
+ priv_modify.save!
+ priv_vm_control = Privilege.new({:name => "vm_control"})
+ priv_vm_control.save!
+ priv_view = Privilege.new({:name => "view"})
+ priv_view.save!
+
+ role_super_admin.privileges = [priv_view, priv_vm_control, priv_modify,
+ priv_perm_view, priv_perm_set]
+ role_super_admin.save!
+ role_admin.privileges = [priv_view, priv_vm_control, priv_modify]
+ role_admin.save!
+ role_user.privileges = [priv_view, priv_vm_control]
+ role_user.save!
+ role_monitor.privileges = [priv_view]
+ role_monitor.save!
+ Permission.find(:all).each do |permission|
+ permission.role = case permission.user_role
+ when "Super Admin"; role_super_admin
+ when "Administrator"; role_admin
+ when "User"; role_user
+ when "Monitor"; role_monitor
+ else nil
+ end
+ permission.save
+ end
+ end
+ remove_column :permissions, :user_role
+
+ end
+
+
+ def self.down
+ add_column :permissions, :user_role, :string
+ Permission.transaction do
+ Permission.find(:all).each do |permission|
+ case permission.role.name
+ when ["Super Admin", "Administrator",
"User", "Monitor"]
+ permission.user_role = permission.role.name
+ permission.save
+ else
+ permission.destroy
+ end
+ end
+ end
+ remove_column :permissions, :role_id
+
+ drop_table :privileges_roles
+ drop_table :privileges
+ drop_table :roles
+ end
+end
diff --git a/src/lib/tasks/vm-states-dot.rake b/src/lib/tasks/vm-states-dot.rake
index 74f1f8e..c26f00e 100644
--- a/src/lib/tasks/vm-states-dot.rake
+++ b/src/lib/tasks/vm-states-dot.rake
@@ -1,5 +1,6 @@
# -*- ruby -*-
require 'permission'
+require 'privilege'
require 'task'
require 'vm'
require 'vm_task'
diff --git a/src/script/grant_admin_privileges
b/src/script/grant_admin_privileges
index 4cafa74..231b1f8 100755
--- a/src/script/grant_admin_privileges
+++ b/src/script/grant_admin_privileges
@@ -14,7 +14,7 @@ ActiveLdap::Base.establish_connection :base => base, :host
=> host, :try_sasl =>
if Account.exists?("uid=#{uid}")
puts "Creating an admin account for #{uid}..."
$pool = DirectoryPool.get_directory_root
- permission = Permission.new(:user_role => Permission::ROLE_SUPER_ADMIN,
+ permission = Permission.new(:user_role =>
Role.find_by_name(Role::SUPER_ADMIN).id,
:uid => uid,
:pool_id => $pool.id)
permission.save_with_new_children
diff --git a/src/test/fixtures/permissions.yml
b/src/test/fixtures/permissions.yml
index 4fff79e..4895f3e 100644
--- a/src/test/fixtures/permissions.yml
+++ b/src/test/fixtures/permissions.yml
@@ -1,39 +1,39 @@
ovirtadmin_root:
- user_role: Super Admin
+ role: super_admin
uid: ovirtadmin
pool: root_dir_pool
ovirtadmin_default:
- user_role: Super Admin
+ role: super_admin
uid: ovirtadmin
pool: default
- inherited_from_id: <%= Fixtures.identify(:ovirtadmin_root) %>
+ parent_permission: ovirtadmin_root
ovirtadmin_hardware_pool:
- user_role: Super Admin
+ role: super_admin
uid: ovirtadmin
pool: hw_dir_pool
- inherited_from_id: <%= Fixtures.identify(:ovirtadmin_root) %>
+ parent_permission: ovirtadmin_root
ovirtadmin_users_pool:
- user_role: Super Admin
+ role: super_admin
uid: ovirtadmin
pool: smart_dir_pool
- inherited_from_id: <%= Fixtures.identify(:ovirtadmin_root) %>
+ parent_permission: ovirtadmin_root
ovirtadmin_corp_com_pool:
- user_role: Super Admin
+ role: super_admin
uid: ovirtadmin
pool: corp_com
- inherited_from_id: <%= Fixtures.identify(:ovirtadmin_root) %>
+ parent_permission: ovirtadmin_root
ovirtadmin_corp_com_production_vmpool:
- user_role: Super Admin
+ role: super_admin
uid: ovirtadmin
pool: corp_com_production_vmpool
- inherited_from_id: <%= Fixtures.identify(:ovirtadmin_root) %>
+ parent_permission: ovirtadmin_root
ovirtadmin_prodops_pool:
- user_role: Super Admin #Monitor
+ role: super_admin
uid: ovirtadmin
pool: corp_com_prod
- inherited_from_id: <%= Fixtures.identify(:ovirtadmin_root) %>
+ parent_permission: ovirtadmin_root
ovirtadmin_corp_com_qa_pool:
- user_role: Monitor
+ role: monitor
uid: ovirtadmin
pool: corp_com_qa
- inherited_from_id: <%= Fixtures.identify(:ovirtadmin_root) %>
\ No newline at end of file
+ parent_permission: ovirtadmin_root
diff --git a/src/test/fixtures/privileges.yml b/src/test/fixtures/privileges.yml
new file mode 100644
index 0000000..3f19584
--- /dev/null
+++ b/src/test/fixtures/privileges.yml
@@ -0,0 +1,10 @@
+set_perms:
+ name: set_perms
+view_perms:
+ name: view_perms
+modify:
+ name: modify
+vm_control:
+ name: vm_control
+view:
+ name: view
diff --git a/src/test/fixtures/roles.yml b/src/test/fixtures/roles.yml
new file mode 100644
index 0000000..774158d
--- /dev/null
+++ b/src/test/fixtures/roles.yml
@@ -0,0 +1,12 @@
+super_admin:
+ name: Super Admin
+ privileges: view, vm_control, modify, view_perms, set_perms
+administrator:
+ name: Administrator
+ privileges: view, vm_control, modify
+user:
+ name: User
+ privileges: view, vm_control
+monitor:
+ name: Monitor
+ privileges: view
diff --git a/src/test/functional/host_controller_test.rb
b/src/test/functional/host_controller_test.rb
index 9a97009..497fe5a 100644
--- a/src/test/functional/host_controller_test.rb
+++ b/src/test/functional/host_controller_test.rb
@@ -24,7 +24,7 @@ require 'host_controller'
class HostController; def rescue_action(e) raise e end; end
class HostControllerTest < Test::Unit::TestCase
- fixtures :hosts, :pools, :permissions
+ fixtures :hosts, :pools, :privileges, :roles, :permissions
def setup
@controller = HostController.new
diff --git a/src/test/functional/nic_controller_test.rb
b/src/test/functional/nic_controller_test.rb
index 8a8776a..68ea3f9 100644
--- a/src/test/functional/nic_controller_test.rb
+++ b/src/test/functional/nic_controller_test.rb
@@ -24,7 +24,7 @@ require 'nic_controller'
class NicController; def rescue_action(e) raise e end; end
class NicControllerTest < Test::Unit::TestCase
- fixtures :permissions, :pools, :nics
+ fixtures :privileges, :roles, :permissions, :pools, :nics
def setup
@controller = NicController.new
diff --git a/src/test/functional/permission_controller_test.rb
b/src/test/functional/permission_controller_test.rb
index c32100a..f5aa4a9 100644
--- a/src/test/functional/permission_controller_test.rb
+++ b/src/test/functional/permission_controller_test.rb
@@ -24,7 +24,7 @@ require 'permission_controller'
class PermissionController; def rescue_action(e) raise e end; end
class PermissionControllerTest < Test::Unit::TestCase
- fixtures :permissions, :pools
+ fixtures :privileges, :roles, :permissions, :pools
def setup
@controller = PermissionController.new
@@ -55,9 +55,9 @@ class PermissionControllerTest < Test::Unit::TestCase
def test_create
num_permissions = Permission.count
- post :create, :permission => { :user_role => 'Administrator',
- :uid => 'admin',
- :pool_id =>
pools(:corp_com_production_vmpool).id}
+ post :create, :permission => { :role_id => roles(:administrator).id,
+ :uid => 'admin',
+ :pool_id =>
pools(:corp_com_production_vmpool).id}
assert_response :success
assert_equal num_permissions + 1, Permission.count
end
diff --git a/src/test/functional/quota_controller_test.rb
b/src/test/functional/quota_controller_test.rb
index dd44dfc..b8f78e6 100644
--- a/src/test/functional/quota_controller_test.rb
+++ b/src/test/functional/quota_controller_test.rb
@@ -90,8 +90,10 @@ class QuotaControllerTest < Test::Unit::TestCase
end
def test_no_perms_to_destroy
- post :destroy, :id => quotas(:corp_com_dev_quota).id
- assert_response :redirect #Is this really what we want? Or should we just
pop up the login page?
+ post :destroy, :id => quotas(:corp_com_dev_quota).id, :format =>
"json"
+ assert_response :success
+ json = ActiveSupport::JSON.decode(@response.body)
+ assert_equal 'You do not have permission to create or modify this item
', json['alert']
end
#FIXME: write the code to make this a real test!
diff --git a/src/test/functional/resources_controller_test.rb
b/src/test/functional/resources_controller_test.rb
index 0312c0c..976e2d1 100644
--- a/src/test/functional/resources_controller_test.rb
+++ b/src/test/functional/resources_controller_test.rb
@@ -24,7 +24,7 @@ require 'resources_controller'
class ResourcesController; def rescue_action(e) raise e end; end
class ResourcesControllerTest < ActionController::TestCase
- fixtures :permissions, :pools
+ fixtures :privileges, :roles, :permissions, :pools
def setup
@controller = ResourcesController.new
diff --git a/src/test/functional/storage_controller_test.rb
b/src/test/functional/storage_controller_test.rb
index f08fe1b..66326d9 100644
--- a/src/test/functional/storage_controller_test.rb
+++ b/src/test/functional/storage_controller_test.rb
@@ -24,7 +24,8 @@ require 'storage_controller'
class StorageController; def rescue_action(e) raise e end; end
class StorageControllerTest < Test::Unit::TestCase
- fixtures :permissions, :pools, :storage_volumes, :storage_pools
+ fixtures :privileges, :roles, :permissions, :pools,
+ :storage_volumes, :storage_pools
def setup
@controller = StorageController.new
@@ -103,8 +104,10 @@ class StorageControllerTest < Test::Unit::TestCase
end
def test_no_perms_to_destroy
- post :destroy, :id => storage_pools(:corp_com_dev_nfs_ovirtnfs).id
- assert_response :redirect #Is this really what we want? Or should we just
pop up the login page?
+ xml_http_request :post, :destroy, :id =>
storage_pools(:corp_com_dev_nfs_ovirtnfs).id, :format => "json"
+ assert_response :success
+ json = ActiveSupport::JSON.decode(@response.body)
+ assert_equal 'You do not have permission to create or modify this item
', json['alert']
end
#FIXME: write the code to make this a real test!
diff --git a/src/test/functional/vm_controller_test.rb
b/src/test/functional/vm_controller_test.rb
index 7936f01..c7769dd 100644
--- a/src/test/functional/vm_controller_test.rb
+++ b/src/test/functional/vm_controller_test.rb
@@ -24,7 +24,7 @@ require 'vm_controller'
class VmController; def rescue_action(e) raise e end; end
class VmControllerTest < Test::Unit::TestCase
- fixtures :permissions, :pools, :vms
+ fixtures :privileges, :roles, :permissions, :pools, :vms
def setup
@controller = VmController.new
@@ -46,11 +46,9 @@ class VmControllerTest < Test::Unit::TestCase
end
def test_new
- get :new, :hardware_pool_id => @default_pool.id
-
- assert_response :redirect
- assert_redirected_to :controller => 'resources', :action =>
'show'
-
+ get :new, :hardware_pool_id => @default_pool.id, :format =>
"json"
+ json = ActiveSupport::JSON.decode(@response.body)
+ assert_equal 'You do not have permission to create or modify this item
', json['alert']
assert_not_nil assigns(:vm)
end
diff --git a/src/test/unit/active_record_env_test.rb
b/src/test/unit/active_record_env_test.rb
index 30db689..26fa139 100644
--- a/src/test/unit/active_record_env_test.rb
+++ b/src/test/unit/active_record_env_test.rb
@@ -22,8 +22,8 @@ require File.dirname(__FILE__) +
'/../../dutils/active_record_env'
class ActiveRecordEnvTest < Test::Unit::TestCase
fixtures :pools, :hosts, :vms, :boot_types,
- :networks, :nics, :ip_addresses, :permissions, :quotas,
- :storage_pools, :storage_volumes, :tasks
+ :networks, :nics, :ip_addresses, :privileges, :roles, :permissions,
+ :quotas, :storage_pools, :storage_volumes, :tasks
def test_can_find_hosts
database_connect
diff --git a/src/test/unit/permission_test.rb b/src/test/unit/permission_test.rb
index 0c0f4c7..2ac78d5 100644
--- a/src/test/unit/permission_test.rb
+++ b/src/test/unit/permission_test.rb
@@ -20,19 +20,19 @@
require File.dirname(__FILE__) + '/../test_helper'
class PermissionTest < Test::Unit::TestCase
- fixtures :permissions
+ fixtures :privileges, :roles, :permissions
fixtures :pools
def setup
@permission = Permission.new(
:uid => 'foobar',
- :user_role => 'Super Admin' )
+ :role_id => Role.find_by_name(Role::SUPER_ADMIN).id )
@permission.pool = pools(:root_dir_pool)
end
# Replace this with your real tests.
def test_simple_permission
- assert_equal permissions(:ovirtadmin_root).user_role, 'Super Admin'
+ assert_equal permissions(:ovirtadmin_root).role.name, 'Super Admin'
assert_equal permissions(:ovirtadmin_root).pool.name, 'root'
end
@@ -51,13 +51,9 @@ class PermissionTest < Test::Unit::TestCase
flunk 'Permission must specify uid' if @permission.valid?
end
- def test_valid_fails_without_user_role
- @permission.user_role = ''
- flunk 'Permission must specify user role' if @permission.valid?
+ def test_valid_fails_without_role_id
+ @permission.role_id = ''
+ flunk 'Permission must specify role' if @permission.valid?
end
- def test_valid_fails_with_invalid_user_role
- @permission.user_role = 'foobar'
- flunk 'Permission must specify valid user role' if
@permission.valid?
- end
end
diff --git a/src/test/unit/task_test.rb b/src/test/unit/task_test.rb
index ba780e7..761e739 100644
--- a/src/test/unit/task_test.rb
+++ b/src/test/unit/task_test.rb
@@ -20,7 +20,7 @@
require File.dirname(__FILE__) + '/../test_helper'
class TaskTest < Test::Unit::TestCase
- fixtures :pools, :hosts, :vms, :permissions, :tasks
+ fixtures :pools, :hosts, :vms, :privileges, :roles, :permissions, :tasks
def setup
@task = Task.new( :type => 'HostTask', :state =>
'finished' )
--
1.6.0.6
Jason Guiditta
2009-Apr-14 18:56 UTC
[Ovirt-devel] [PATCH server] first round of permissions refactoring.
ACK, one super-minor nit inline. -j On Mon, 2009-03-30 at 19:08 +0000, Scott Seago wrote:> diff --git a/src/db/migrate/037_add_roles_and_privileges.rb b/src/db/migrate/037_add_roles_and_privileges.rb > new file mode 100644 > index 0000000..322014e > --- /dev/null > +++ b/src/db/migrate/037_add_roles_and_privileges.rb > @@ -0,0 +1,119 @@> +class AddRolesAndPrivileges < ActiveRecord::Migration > + def self.up > +# create_table :permissions do |t| > +# t.string :user_role > +# t.string :user > +# t.integer :pool_id > +# t.integer :inherited_from_id > +# t.integer :lock_version, :default => 0 > +# endUnless there is a reason to keep the above, could you delete it rather than commenting out?> + create_table :roles do |t|