Ovirt devel - Aug 2008 - [PATCH] permission denormalization. (revised)

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..1870027 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 
+    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

This partially works for me, but noticed a couple of things.  

*  when viewing the users grid, the column with 'inherited/direct' has
no header.  This may be ok, but when you mouse over it, you get a
clickable arrow that drops down a box that can't be fully seen
(attaching screenshot, hard to describe)
*  Not sure where this breaks exactly, but when I log in as a user with
no permissions, I still get all nodes returned in the tree (so
fetch_json must not be filtering right).  However, when I click on one
of them, I see rails attempting to reroute me to /login. Similarly, get
the full tree for a valid user with a small set of permissions, and can
go only to approve pools (though I still see them in the tree).
*  Search results return items my user does not have permission for.
Again, I cannot click on them, so that part of security is there. (I
think this is covered in the next patch, which I have not tested yet).
* I have not been able to test granting a new permission as my developer
appliance has not yet successfully reinstalled.  I kept getting the
error:
'ERROR:root:Unable to create appliance : Failed to find package
'curl' :
No package(s) available to install'
Trying a full reinstall but am pretty much done for the day/week (it has
been running a 1/2 hour is is maybe halfway through).  I'll check in on
it later this after and give feedback if it is successful.

-j
-------------- next part --------------
A non-text attachment was scrubbed...
Name: user_access.png
Type: image/png
Size: 106672 bytes
Desc: not available
URL:
<http://listman.redhat.com/archives/ovirt-devel/attachments/20080807/663790e4/attachment.png>

Sent a reply with an attachment which seems to have not gone through, so
trying again without:

This partially works for me, but noticed a couple of things.  

*  when viewing the users grid, the column with 'inherited/direct' has
no header.  This may be ok, but when you mouse over it, you get a
clickable arrow that drops down a box that can't be fully seen
(attaching screenshot, hard to describe)
*  Not sure where this breaks exactly, but when I log in as a user with
no permissions, I still get all nodes returned in the tree (so
fetch_json must not be filtering right).  However, when I click on one
of them, I see rails attempting to reroute me to /login. Similarly, get
the full tree for a valid user with a small set of permissions, and can
go only to approve pools (though I still see them in the tree).
*  Search results return items my user does not have permission for.
Again, I cannot click on them, so that part of security is there. (I
think this is covered in the next patch, which I have not tested yet).
* I have not been able to test granting a new permission as my developer
appliance has not yet successfully reinstalled.  I kept getting the
error:
'ERROR:root:Unable to create appliance : Failed to find package
'curl' :
No package(s) available to install'
Trying a full reinstall but am pretty much done for the day/week (it has
been running a 1/2 hour is is maybe halfway through).  I'll check in on
it later this after and give feedback if it is successful.

-j