Scott Seago
2008-Aug-06 17:45 UTC
[Ovirt-devel] [PATCH] add permissions checks to search results.
To do so, I've enabled term-based parameters to each of the searchable
types. At the query level, appending search_users:foo limits results to items
viewable by user foo. This involved:
1) added :terms parameter to acts_as_xapian declaration for the method
search_users
2) added search_users method to searchable models which return an array of
usernames that have 'monitor' access
3) modified the acts_as_xapian plugin to handle prefix searches for which the
object provides multiple values (since we have multiple users with access to
each object) -- this is a change which may be suitable for upstream inclusion
4) When performing the search, search for "(#{terms}) AND
search_users:#{user}" instead of simply searching for terms.
This patch is dependant on the prior search denormalization patch.
Signed-off-by: Scott Seago <sseago at redhat.com>
---
wui/src/app/controllers/hardware_controller.rb | 1 +
wui/src/app/controllers/search_controller.rb | 16 ++++++++++------
wui/src/app/models/host.rb | 8 +++++++-
wui/src/app/models/pool.rb | 8 +++++++-
wui/src/app/models/storage_pool.rb | 6 +++++-
wui/src/app/models/vm.rb | 8 +++++++-
.../plugins/acts_as_xapian/lib/acts_as_xapian.rb | 6 +++++-
7 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/wui/src/app/controllers/hardware_controller.rb
b/wui/src/app/controllers/hardware_controller.rb
index 5f473db..019fdd8 100644
--- a/wui/src/app/controllers/hardware_controller.rb
+++ b/wui/src/app/controllers/hardware_controller.rb
@@ -110,6 +110,7 @@ class HardwareController < ApplicationController
unless @can_view
flash[:notice] = 'You do not have permission to view this Hardware
Pool: redirecting to top level'
redirect_to :action => 'list'
+ return
end
render :layout => 'selection'
end
diff --git a/wui/src/app/controllers/search_controller.rb
b/wui/src/app/controllers/search_controller.rb
index ca4fbb1..cc7e527 100644
--- a/wui/src/app/controllers/search_controller.rb
+++ b/wui/src/app/controllers/search_controller.rb
@@ -56,6 +56,11 @@ class SearchController < ApplicationController
@models ||= [@model_param]
end
@user = get_login_user
+ #filter terms on permissions
+ filtered_terms = "search_users:#{@user}"
+ if @terms and !@terms.empty?
+ filtered_terms = "(#{@terms}) AND #{filtered_terms}"
+ end
@page = params[:page].to_i
@page ||= 1
@@ -63,12 +68,11 @@ class SearchController < ApplicationController
@per_page ||= 20
@offset = (@page-1)*@per_page
@results = ActsAsXapian::Search.new(@models,
- @terms,
- :offset => @offset,
- :limit => @per_page,
- :sort_by_prefix => nil,
- :collapse_by_prefix => nil)
- #FIXME filter on permissions
+ filtered_terms,
+ :offset => @offset,
+ :limit => @per_page,
+ :sort_by_prefix => nil,
+ :collapse_by_prefix => nil)
end
def results
diff --git a/wui/src/app/models/host.rb b/wui/src/app/models/host.rb
index fc3b236..5b32653 100644
--- a/wui/src/app/models/host.rb
+++ b/wui/src/app/models/host.rb
@@ -43,7 +43,8 @@ class Host < ActiveRecord::Base
acts_as_xapian :texts => [ :hostname, :uuid, :hypervisor_type, :arch ],
:values => [ [ :created_at, 0, "created_at",
:date ],
[ :updated_at, 1, "updated_at", :date ]
],
- :terms => [ [ :hostname, 'H', "hostname"
] ]
+ :terms => [ [ :hostname, 'H', "hostname"
],
+ [ :search_users, 'U',
"search_users" ] ]
KVM_HYPERVISOR_TYPE = "KVM"
@@ -88,4 +89,9 @@ class Host < ActiveRecord::Base
def display_class
"Host"
end
+
+ def search_users
+ hardware_pool.search_users
+ end
+
end
diff --git a/wui/src/app/models/pool.rb b/wui/src/app/models/pool.rb
index 1870027..6599c72 100644
--- a/wui/src/app/models/pool.rb
+++ b/wui/src/app/models/pool.rb
@@ -85,7 +85,8 @@ class Pool < ActiveRecord::Base
end
end
- acts_as_xapian :texts => [ :name ]
+ acts_as_xapian :texts => [ :name ],
+ :terms => [ [ :search_users, 'U',
"search_users" ] ]
# this method lists pools with direct permission grants, but by default does
# not include implied permissions (i.e. subtrees)
@@ -246,6 +247,11 @@ class Pool < ActiveRecord::Base
def display_class
get_type_label
end
+
+ def search_users
+ permissions.collect {|perm| perm.uid}
+ end
+
protected
def traverse_parents
if id
diff --git a/wui/src/app/models/storage_pool.rb
b/wui/src/app/models/storage_pool.rb
index 3e3f58f..460b49d 100644
--- a/wui/src/app/models/storage_pool.rb
+++ b/wui/src/app/models/storage_pool.rb
@@ -32,7 +32,8 @@ class StoragePool < ActiveRecord::Base
validates_presence_of :ip_addr, :hardware_pool_id
- acts_as_xapian :texts => [ :ip_addr, :target, :export_path, :type ]
+ acts_as_xapian :texts => [ :ip_addr, :target, :export_path, :type ],
+ :terms => [ [ :search_users, 'U',
"search_users" ] ]
ISCSI = "iSCSI"
NFS = "NFS"
STORAGE_TYPES = { ISCSI => "Iscsi",
@@ -60,4 +61,7 @@ class StoragePool < ActiveRecord::Base
"Storage Pool"
end
+ def search_users
+ hardware_pool.search_users
+ end
end
diff --git a/wui/src/app/models/vm.rb b/wui/src/app/models/vm.rb
index 34d5bf4..9ac24d9 100644
--- a/wui/src/app/models/vm.rb
+++ b/wui/src/app/models/vm.rb
@@ -31,7 +31,8 @@ class Vm < ActiveRecord::Base
validates_presence_of :uuid, :description, :num_vcpus_allocated,
:memory_allocated_in_mb, :memory_allocated,
:vnic_mac_addr
- acts_as_xapian :texts => [ :uuid, :description, :vnic_mac_addr, :state ]
+ acts_as_xapian :texts => [ :uuid, :description, :vnic_mac_addr, :state ],
+ :terms => [ [ :search_users, 'U',
"search_users" ] ]
BOOT_DEV_HD = "hd"
BOOT_DEV_NETWORK = "network"
@@ -193,6 +194,11 @@ class Vm < ActiveRecord::Base
def display_class
"VM"
end
+
+ def search_users
+ vm_resource_pool.search_users
+ end
+
protected
def validate
resources = vm_resource_pool.max_resources_for_vm(self)
diff --git a/wui/src/vendor/plugins/acts_as_xapian/lib/acts_as_xapian.rb
b/wui/src/vendor/plugins/acts_as_xapian/lib/acts_as_xapian.rb
index eda0b1b..566fe82 100644
--- a/wui/src/vendor/plugins/acts_as_xapian/lib/acts_as_xapian.rb
+++ b/wui/src/vendor/plugins/acts_as_xapian/lib/acts_as_xapian.rb
@@ -523,6 +523,8 @@ module ActsAsXapian
value.utc.strftime("%Y%m%d")
elsif type == :boolean
value ? true : false
+ elsif type == :array
+ (value.class == Array) ? value.collect {|x| x.to_s} :
value.to_s
else
value.to_s
end
@@ -549,7 +551,9 @@ module ActsAsXapian
doc.add_term("I" + doc.data)
if self.xapian_options[:terms]
for term in self.xapian_options[:terms]
- doc.add_term(term[1] + xapian_value(term[0]))
+ xapian_value(term[0], :array).each do |val|
+ doc.add_term(term[1] + val)
+ end
end
end
if self.xapian_options[:values]
--
1.5.5.1
Jason Guiditta
2008-Aug-13 16:37 UTC
[Ovirt-devel] [PATCH] add permissions checks to search results.
On Wed, 2008-08-06 at 13:45 -0400, Scott Seago wrote:> To do so, I've enabled term-based parameters to each of the searchable types. At the query level, appending search_users:foo limits results to items viewable by user foo. This involved: > > 1) added :terms parameter to acts_as_xapian declaration for the method search_users > 2) added search_users method to searchable models which return an array of usernames that have 'monitor' access > 3) modified the acts_as_xapian plugin to handle prefix searches for which the object provides multiple values (since we have multiple users with access to each object) -- this is a change which may be suitable for upstream inclusion > 4) When performing the search, search for "(#{terms}) AND search_users:#{user}" instead of simply searching for terms. > > This patch is dependant on the prior search denormalization patch. > > Signed-off-by: Scott Seago <sseago at redhat.com>After much trouble of my own making, finally got this tested properly. Search works as desired, so ACK with one caveat. I noticed when I try to add a user (under any 'user access'), clicking 'create user permission' gives me a 401, and subsequent attempts give me a 401 followed by a 500. I looked at the log in /var/log/ovirt-wui/mongrel.log, but did not see anything go by, not even an attempt at running the requested action. Perhaps this is related to the freeipa perm issues I saw being discussed in irc, but I thought it should be mentioned. This may not be at all related to this patch, and if not, then definitely ACK. -j