[PATCH conductor 3/3] Redmine #3158: Use denormalized model for permission checks

Scott Seago sseago at redhat.com
Thu May 3 14:25:56 UTC 2012


https://www.aeolusproject.org/redmine/issues/3158

Use the new derived_permissions model for permission checks -- remove no-longer-needed helper methods that added in the joins for the non-denormalized model.
---
 src/app/models/catalog.rb                          |   17 --------
 src/app/models/deployable.rb                       |   20 ----------
 src/app/models/deployment.rb                       |   17 --------
 src/app/models/instance.rb                         |   20 ----------
 src/app/models/permissioned_object.rb              |   41 +++++++------------
 src/app/models/pool.rb                             |   15 -------
 src/app/models/provider_account.rb                 |   14 -------
 .../controllers/deployables_controller_spec.rb     |    2 +
 .../controllers/deployments_controller_spec.rb     |    2 +-
 src/spec/controllers/pools_controller_spec.rb      |    2 +-
 10 files changed, 19 insertions(+), 131 deletions(-)

diff --git a/src/app/models/catalog.rb b/src/app/models/catalog.rb
index 5ab51c4..4ea3df1 100644
--- a/src/app/models/catalog.rb
+++ b/src/app/models/catalog.rb
@@ -62,23 +62,6 @@ class Catalog < ActiveRecord::Base
   def self.additional_privilege_target_types
     [Deployable]
   end
-  class << self
-    alias orig_list_for_user_include list_for_user_include
-    alias orig_list_for_user_conditions list_for_user_conditions
-  end
-
-  def self.list_for_user_include
-    orig_list_for_user_include + [ {:pool => :permissions},
-                                   {:pool_family => :permissions} ]
-  end
-
-  def self.list_for_user_conditions
-    "(#{orig_list_for_user_conditions}) or
-     (permissions_pools.user_id=:user and
-      permissions_pools.role_id in (:role_ids)) or
-     (permissions_pool_families.user_id=:user and
-      permissions_pool_families.role_id in (:role_ids))"
-  end
 
   PRESET_FILTERS_OPTIONS = [
     {:title => "catalogs.preset_filters.belongs_to_default_pool", :id => "belongs_to_default_pool", :query => includes(:pool).where("pools.name" => "Default")}
diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
index 02c53ff..201123f 100644
--- a/src/app/models/deployable.rb
+++ b/src/app/models/deployable.rb
@@ -56,26 +56,6 @@ class Deployable < ActiveRecord::Base
   def perm_ancestors
     super + catalogs + catalogs.collect{|c| c.pool}.uniq + [pool_family]
   end
-  class << self
-    alias orig_list_for_user_include list_for_user_include
-    alias orig_list_for_user_conditions list_for_user_conditions
-  end
-
-  def self.list_for_user_include
-    orig_list_for_user_include + [ {:catalogs => [:permissions,
-                                                  {:pool => :permissions}]},
-                                   {:pool_family => :permissions} ]
-  end
-
-  def self.list_for_user_conditions
-    "(#{orig_list_for_user_conditions}) or
-     (permissions_catalogs.user_id=:user and
-      permissions_catalogs.role_id in (:role_ids)) or
-     (permissions_pools.user_id=:user and
-      permissions_pools.role_id in (:role_ids)) or
-     (permissions_pool_families.user_id=:user and
-      permissions_pool_families.role_id in (:role_ids))"
-  end
 
   def valid_deployable_xml?
     begin
diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
index 172aaca..3f5f6d1 100644
--- a/src/app/models/deployment.rb
+++ b/src/app/models/deployment.rb
@@ -122,23 +122,6 @@ class Deployment < ActiveRecord::Base
   def self.additional_privilege_target_types
     [Instance]
   end
-  class << self
-    alias orig_list_for_user_include list_for_user_include
-    alias orig_list_for_user_conditions list_for_user_conditions
-  end
-
-  def self.list_for_user_include
-    orig_list_for_user_include + [ {:pool => :permissions},
-                                   {:pool_family => :permissions} ]
-  end
-
-  def self.list_for_user_conditions
-    "(#{orig_list_for_user_conditions}) or
-     (permissions_pools.user_id=:user and
-      permissions_pools.role_id in (:role_ids)) or
-     (permissions_pool_families.user_id=:user and
-      permissions_pool_families.role_id in (:role_ids))"
-  end
 
   def get_action_list(user=nil)
     # FIXME: how do actions and states interact for deployments?
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
index 5430eb0..4150431 100644
--- a/src/app/models/instance.rb
+++ b/src/app/models/instance.rb
@@ -150,26 +150,6 @@ class Instance < ActiveRecord::Base
     ancestors << deployment unless deployment.nil?
     ancestors += [pool, pool_family]
   end
-  class << self
-    alias orig_list_for_user_include list_for_user_include
-    alias orig_list_for_user_conditions list_for_user_conditions
-  end
-
-  def self.list_for_user_include
-    orig_list_for_user_include + [ {:deployment => :permissions},
-                                   {:pool => :permissions},
-                                   {:pool_family => :permissions} ]
-  end
-
-  def self.list_for_user_conditions
-    "(#{orig_list_for_user_conditions}) or
-     (permissions_deployments.user_id=:user and
-      permissions_deployments.role_id in (:role_ids)) or
-     (permissions_pools.user_id=:user and
-      permissions_pools.role_id in (:role_ids)) or
-     (permissions_pool_families.user_id=:user and
-      permissions_pool_families.role_id in (:role_ids))"
-  end
 
   def get_action_list(user=nil)
     # return empty list rather than nil
diff --git a/src/app/models/permissioned_object.rb b/src/app/models/permissioned_object.rb
index 3f083ea..27f28e1 100644
--- a/src/app/models/permissioned_object.rb
+++ b/src/app/models/permissioned_object.rb
@@ -19,26 +19,20 @@ module PermissionedObject
   def has_privilege(user, action, target_type=nil)
     return false if user.nil? or action.nil?
     target_type = self.class.default_privilege_target_type if target_type.nil?
-    perm_ancestors.each do |obj|
-      return true if obj and obj.permissions.find(:first,
-                                          :include => [:role => :privileges],
-                                          :conditions =>
-                                          ["permissions.user_id=:user and
-                                            privileges.target_type=:target_type and
-                                            privileges.action=:action",
-                                           { :user => user.id,
-                                             :target_type => target_type.name,
-                                             :action => action}])
-    end
-    return false
+    derived_permissions.includes(:role => :privileges).where(
+      ["derived_permissions.user_id=:user and
+        privileges.target_type=:target_type and
+        privileges.action=:action",
+        { :user => user.id,
+          :target_type => target_type.name,
+          :action => action}]).any?
   end
 
   # Returns the list of objects to check for permissions on -- by default
   # this object plus the Base permission object
   # At the moment, the retuned list is not necessarily ordered
-  # FIXME: remove self once has_privilege stops using this method
   def perm_ancestors
-    [self, BasePermissionObject.general_permission_scope]
+    [BasePermissionObject.general_permission_scope]
   end
   # Returns the list of objects to generate derived permissions for
   # -- by default just this object
@@ -101,23 +95,18 @@ module PermissionedObject
       def self.default_privilege_target_type
         self
       end
-      def self.list_for_user_include
-        [:permissions]
-      end
-      def self.list_for_user_conditions
-        "permissions.user_id=:user and
-         permissions.role_id in (:role_ids)"
-      end
       def self.list_for_user(user, action, target_type=self.default_privilege_target_type)
         return where("1=0") if user.nil? or action.nil? or target_type.nil?
         if BasePermissionObject.general_permission_scope.has_privilege(user, action, target_type)
           scoped
         else
-          role_ids = Role.includes(:privileges).where("privileges.target_type" => target_type, "privileges.action" => action).collect {|r| r.id}
-          include_clause = self.list_for_user_include
-          conditions_hash = {:user => user.id, :target_type => target_type.name, :action => action, :role_ids => role_ids}
-          conditions_str = self.list_for_user_conditions
-          includes(include_clause).where(conditions_str, conditions_hash)
+          includes([:derived_permissions => {:role => :privileges}]).
+            where("derived_permissions.user_id=:user and
+                   privileges.target_type=:target_type and
+                   privileges.action=:action",
+                  {:user => user.id,
+                   :target_type => target_type.name,
+                   :action => action})
         end
       end
     end
diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
index 9110481..f8daa58 100644
--- a/src/app/models/pool.rb
+++ b/src/app/models/pool.rb
@@ -165,21 +165,6 @@ class Pool < ActiveRecord::Base
   def self.additional_privilege_target_types
     [Deployment, Instance, Catalog, Quota]
   end
-  class << self
-    alias orig_list_for_user_include list_for_user_include
-    alias orig_list_for_user_conditions list_for_user_conditions
-  end
-
-  def self.list_for_user_include
-    orig_list_for_user_include + [ {:pool_family => :permissions} ]
-  end
-
-  def self.list_for_user_conditions
-    "(#{orig_list_for_user_conditions}) or
-     (permissions_pool_families.user_id=:user and
-      permissions_pool_families.role_id in (:role_ids))"
-  end
-
 
   private
 
diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
index 0ebc848..7fbc584 100644
--- a/src/app/models/provider_account.rb
+++ b/src/app/models/provider_account.rb
@@ -115,20 +115,6 @@ class ProviderAccount < ActiveRecord::Base
   def self.additional_privilege_target_types
     [Quota]
   end
-  class << self
-    alias orig_list_for_user_include list_for_user_include
-    alias orig_list_for_user_conditions list_for_user_conditions
-  end
-
-  def self.list_for_user_include
-    orig_list_for_user_include + [{ :provider => :permissions}]
-  end
-
-  def self.list_for_user_conditions
-    "(#{orig_list_for_user_conditions}) or
-     (permissions_providers.user_id=:user and
-      permissions_providers.role_id in (:role_ids))"
-  end
 
   def destroyable?
     instances.empty? || instances.all? { |i| i.destroyable? }
diff --git a/src/spec/controllers/deployables_controller_spec.rb b/src/spec/controllers/deployables_controller_spec.rb
index 3e08905..e952dbf 100644
--- a/src/spec/controllers/deployables_controller_spec.rb
+++ b/src/spec/controllers/deployables_controller_spec.rb
@@ -83,6 +83,7 @@ describe DeployablesController do
     context "deletion successfully" do
       before do
         Deployable.any_instance.stub(:destroy).and_return(true)
+        Deployable.any_instance.stub(:has_privilege).and_return(true)
       end
 
       it "deletes a deployable and redirect to deployables#index" do
@@ -99,6 +100,7 @@ describe DeployablesController do
     context "deletion fails" do
       before do
         Deployable.any_instance.stub(:destroy).and_return(false)
+        Deployable.any_instance.stub(:has_privilege).and_return(true)
       end
 
       it "not delete a deployable and redirect to deployables#show" do
diff --git a/src/spec/controllers/deployments_controller_spec.rb b/src/spec/controllers/deployments_controller_spec.rb
index 54b1477..4629ad7 100644
--- a/src/spec/controllers/deployments_controller_spec.rb
+++ b/src/spec/controllers/deployments_controller_spec.rb
@@ -74,7 +74,7 @@ describe DeploymentsController do
 
     describe "#destroy" do
       before do
-        @deployment = Factory.build(:deployment)
+        @deployment = Factory.create(:deployment)
         Deployment.stub!(:find).and_return([@deployment])
         delete :multi_destroy, :deployments_selected => [@deployment.id], :format => :json
       end
diff --git a/src/spec/controllers/pools_controller_spec.rb b/src/spec/controllers/pools_controller_spec.rb
index 21af9d0..6e1a5eb 100644
--- a/src/spec/controllers/pools_controller_spec.rb
+++ b/src/spec/controllers/pools_controller_spec.rb
@@ -118,7 +118,7 @@ describe PoolsController do
 
     describe "#destroy" do
       before do
-        @pool = Factory.build(:pool)
+        @pool = Factory.create(:pool)
         Pool.stub!(:find).and_return([@pool])
         delete :multi_destroy, :pools_selected => [@pool.id], :format => :json
       end
-- 
1.7.6.5




More information about the aeolus-devel mailing list