[Ovirt-devel] [PATCH] permission denormalization.

Scott Seago sseago at redhat.com
Mon Aug 4 17:26:09 UTC 2008


Permission records are now stored for all pools a user has access to, even implied/inherited permissions. This means that if an admin is granted access to the 'default' pool, there will be one permission record for this pool, and an additional "inherited" record for each pool below 'default' in the hierarchy -- for the rest of these records, inherited_from_id will point to the master permission record.

Any time a user is granted permission on a pool, the inherited records are automatically created. When a new pool is created, inherited records are created here too.

You need to run 'rake db:migrate' to get the updated permissions table --  the migration script will take care of creating inherited records for existing permissions.

In the UI, both direct and inherited grants are shown, but only direct grants have the checkbox to allow modification/deletion.

Signed-off-by: Scott Seago <sseago at redhat.com>
---
 wui/src/app/controllers/hardware_controller.rb    |    2 +-
 wui/src/app/controllers/permission_controller.rb  |   14 +++---
 wui/src/app/controllers/resources_controller.rb   |    2 +-
 wui/src/app/models/permission.rb                  |   42 ++++++++++++++++-
 wui/src/app/models/pool.rb                        |   35 ++++++++++-----
 wui/src/app/views/user/_grid.rhtml                |   51 +++++++++++---------
 wui/src/db/migrate/012_denormalize_permissions.rb |   32 +++++++++++++
 7 files changed, 133 insertions(+), 45 deletions(-)
 create mode 100644 wui/src/db/migrate/012_denormalize_permissions.rb

diff --git a/wui/src/app/controllers/hardware_controller.rb b/wui/src/app/controllers/hardware_controller.rb
index e105f7c..5f473db 100644
--- a/wui/src/app/controllers/hardware_controller.rb
+++ b/wui/src/app/controllers/hardware_controller.rb
@@ -151,7 +151,7 @@ class HardwareController < ApplicationController
 
   def users_json
     json_list(@pool.permissions, 
-              [:id, :uid, :user_role])
+              [:grid_id, :uid, :user_role, :source])
   end
 
   def storage_pools_json
diff --git a/wui/src/app/controllers/permission_controller.rb b/wui/src/app/controllers/permission_controller.rb
index 14358de..4367275 100644
--- a/wui/src/app/controllers/permission_controller.rb
+++ b/wui/src/app/controllers/permission_controller.rb
@@ -59,10 +59,11 @@ class PermissionController < ApplicationController
       flash[:notice] = 'You do not have permission to create this permission record'
       redirect_to_parent
     else
-      if @permission.save
+      begin
+        @permission.save_with_new_children
         render :json => "created User Permissions for  #{@permission.uid}".to_json
-      else
-      # FIXME: need to handle proper error messages w/ ajax
+      rescue
+        # FIXME: need to handle proper error messages w/ ajax
         render :action => 'new'
       end
     end
@@ -79,13 +80,12 @@ class PermissionController < ApplicationController
       Permission.transaction do
         permissions = Permission.find(:all, :conditions => "id in (#{permission_ids.join(', ')})")
         permissions.each do |permission|
-          permission.user_role = role
-          permission.save!
+          permission.update_role(role) if permission.is_primary?
         end
       end
       render :json => { :object => "permission", :success => true, 
         :alert => "User roles were successfully updated." }
-    rescue 
+    rescue
       render :json => { :object => "permission", :success => false, 
         :alert => "Error updating user roles: #{$!}" }
     end
@@ -101,7 +101,7 @@ class PermissionController < ApplicationController
       Permission.transaction do
         permissions = Permission.find(:all, :conditions => "id in (#{permission_ids.join(', ')})")
         permissions.each do |permission|
-          permission.destroy
+          permission.destroy if permission.is_primary?
         end
       end
       render :json => { :object => "permission", :success => true, 
diff --git a/wui/src/app/controllers/resources_controller.rb b/wui/src/app/controllers/resources_controller.rb
index dbf9ad7..20defca 100644
--- a/wui/src/app/controllers/resources_controller.rb
+++ b/wui/src/app/controllers/resources_controller.rb
@@ -97,7 +97,7 @@ class ResourcesController < ApplicationController
 
   def users_json
     json_list(@vm_resource_pool.permissions, 
-              [:id, :uid, :user_role])
+              [:grid_id, :uid, :user_role, :source])
   end
 
   def new
diff --git a/wui/src/app/models/permission.rb b/wui/src/app/models/permission.rb
index 74c6c26..ece3da5 100644
--- a/wui/src/app/models/permission.rb
+++ b/wui/src/app/models/permission.rb
@@ -19,8 +19,13 @@
 
 class Permission < ActiveRecord::Base
   belongs_to :pool
+  belongs_to :parent_permission, :class_name => "Permission",
+             :foreign_key => "inherited_from_id"
+  has_many   :child_permissions, :dependent => :destroy,
+             :class_name => "Permission", :foreign_key => "inherited_from_id"
 
-  validates_uniqueness_of :uid, :scope => "pool_id"
+
+  validates_uniqueness_of :uid, :scope => [:pool_id, :inherited_from_id]
 
   ROLE_SUPER_ADMIN = "Super Admin"
   ROLE_ADMIN       = "Administrator"
@@ -68,5 +73,38 @@ class Permission < ActiveRecord::Base
     PRIVILEGES[privilege]
   end
 
-
+  def is_primary?
+    inherited_from_id.nil?
+  end
+  def is_inherited?
+    !is_primary?
+  end
+  def source
+    is_primary? ? "Direct" : "Inherited"
+  end
+  def grid_id
+    id.to_s + "_" + (is_primary? ? "1" : "0")
+  end
+  def update_role(new_role)
+    self.transaction do
+      self.user_role = new_role
+      self.save!
+      child_permissions.each do |permission|
+        permission.user_role = new_role
+        permission.save!
+      end
+    end
+  end
+  def save_with_new_children
+    self.transaction do
+      self.save!
+      pool.all_children.each do |subpool|
+          new_permission = Permission.new({:pool_id     => subpool.id,
+                                           :uid         => uid,
+                                           :user_role   => user_role,
+                                           :inherited_from_id => id})
+          new_permission.save!
+      end
+    end
+  end
 end
diff --git a/wui/src/app/models/pool.rb b/wui/src/app/models/pool.rb
index 4aa240b..74fe698 100644
--- a/wui/src/app/models/pool.rb
+++ b/wui/src/app/models/pool.rb
@@ -71,18 +71,33 @@ class Pool < ActiveRecord::Base
     transaction do
       save!
       move_to_child_of(parent)
+      parent.permissions.each do |permission|
+        new_permission = Permission.new({:pool_id     => id,
+                                         :uid         => permission.uid,
+                                         :user_role   => permission.user_role,
+                                         :inherited_from_id =>
+                                          permission.inherited_from_id.nil? ?
+                                          permission.id :
+                                          permission.inherited_from_id})
+        new_permission.save!
+      end
       yield other_actions if other_actions
     end
   end
 
   acts_as_xapian :texts => [ :name ]
 
-  # this method lists pools with direct permission grants, but does not 
-  # include implied permissions (i.e. subtrees)
-  def self.list_for_user(user, privilege)
-    pools = find(:all, :include => "permissions", 
-                 :conditions => "permissions.uid='#{user}' and 
-                       permissions.user_role in 
+  # this method lists pools with direct permission grants, but by default does
+  #  not include implied permissions (i.e. subtrees)
+  def self.list_for_user(user, privilege, include_indirect = false)
+    if include_indirect
+      inherited_clause = ""
+    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("', '")}')")
   end
 
@@ -126,12 +141,10 @@ class Pool < ActiveRecord::Base
   end
 
   def has_privilege(user, privilege)
-    traverse_parents do |pool|
-      pool.permissions.find(:first, 
-                            :conditions => "permissions.uid='#{user}' and 
-                         permissions.user_role in 
+    pool.permissions.find(:first,
+                          :conditions => "permissions.uid='#{user}' and
+                         permissions.user_role in
                          ('#{Permission.roles_for_privilege(privilege).join("', '")}')")
-    end
   end
 
   def total_resources
diff --git a/wui/src/app/views/user/_grid.rhtml b/wui/src/app/views/user/_grid.rhtml
index d9ad795..cabf2af 100644
--- a/wui/src/app/views/user/_grid.rhtml
+++ b/wui/src/app/views/user/_grid.rhtml
@@ -5,27 +5,32 @@
 </form>
 </div>
 <script type="text/javascript">
-	$("#<%= table_id %>").flexigrid
-	(
-	{
-	url: '<%=  url_for :controller => parent_controller, :action => "users_json", :id => pool.id %>',
-	dataType: 'json',
-	colModel : [
-		{display: '', name : 'id', width : 20, sortable : false, align: 'left', process: <%= table_id %>checkbox},
-		{display: 'Name', name : 'uid', width : 180, sortable : true, align: 'left'},
-	        {display: 'Role', name : 'user_role', width : 80, sortable : true, align: 'left'}
-		],
-	sortname: "user",
-	sortorder: "asc",
-	usepager: <%= pool.permissions.size > users_per_page ? 'true' : 'false' %>,
-	useRp: true,
-	rp: <%= users_per_page %>,
-	showTableToggleBtn: true
-	}
-	);   
-	function <%= table_id %>checkbox(celDiv)
-	{
-	       $(celDiv).html('<input type="checkbox" name="grid_checkbox'+$(celDiv).html()+'" class="grid_checkbox" value="'+$(celDiv).html()+'"/>');
-	} 
-
+    $("#<%= table_id %>").flexigrid
+    (
+    {
+    url: '<%=  url_for :controller => parent_controller, :action => "users_json", :id => pool.id %>',
+    dataType: 'json',
+    colModel : [
+        {display: '', name : 'id', width : 20, sortable : false, align: 'left', process: <%= table_id %>checkbox},
+        {display: 'Name', name : 'uid', width : 180, sortable : true, align: 'left'},
+        {display: 'Role', name : 'user_role', width : 80, sortable : true, align: 'left'},
+        {display: '', width : 80, sortable : true, align: 'left'}
+        ],
+    sortname: "user",
+    sortorder: "asc",
+    usepager: <%= pool.permissions.size > users_per_page ? 'true' : 'false' %>,
+    useRp: true,
+    rp: <%= users_per_page %>,
+    showTableToggleBtn: true
+    }
+    );
+    function <%= table_id %>checkbox(celDiv)
+    {
+        var grid_id = $(celDiv).html().split("_");
+        if (grid_id[1] == 1) {
+            $(celDiv).html('<input type="checkbox" name="grid_checkbox'+grid_id[0]+'" class="grid_checkbox" value="'+grid_id[0]+'"/>');
+	} else {
+            $(celDiv).html('')
+        }
+     }
 </script>
diff --git a/wui/src/db/migrate/012_denormalize_permissions.rb b/wui/src/db/migrate/012_denormalize_permissions.rb
new file mode 100644
index 0000000..f68f555
--- /dev/null
+++ b/wui/src/db/migrate/012_denormalize_permissions.rb
@@ -0,0 +1,32 @@
+class DenormalizePermissions < ActiveRecord::Migration
+  def self.up
+    add_column :permissions, :inherited_from_id, :integer
+    execute "alter table permissions add constraint fk_perm_parent
+             foreign key (inherited_from_id) references permissions(id)"
+
+    Permission.transaction do
+      Permission.find(:all,
+                      :conditions => "inherited_from_id is null"
+                      ).each do |permission|
+        permission.pool.all_children.each do |subpool|
+          new_permission = Permission.new({:pool_id     => subpool.id,
+                                           :uid         => permission.uid,
+                                           :user_role   => permission.user_role,
+                                           :inherited_from_id => permission.id})
+          new_permission.save!
+        end
+      end
+    end
+  end
+
+  def self.down
+    Permission.transaction do
+      Permission.find(:all,
+                      :conditions => "inherited_from_id is not null"
+                      ).each do |permission|
+        permission.destroy
+      end
+    end
+    remove_column :permissions, :inherited_from_id
+  end
+end
-- 
1.5.5.1




More information about the ovirt-devel mailing list