Ovirt devel - May 2008 - [PATCH] Users are displayed from LDAP and filtered based on the current list of users.

Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
---
 wui/src/app/controllers/application.rb           |    2 +-
 wui/src/app/controllers/hardware_controller.rb   |    2 +-
 wui/src/app/controllers/permission_controller.rb |    4 ++-
 wui/src/app/models/account.rb                    |   30 +++++++++++++++++++---
 wui/src/app/models/permission.rb                 |    7 +++++
 wui/src/app/views/permission/_form.rhtml         |    3 +-
 wui/src/config/ldap.yml                          |    4 +-
 7 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/wui/src/app/controllers/application.rb
b/wui/src/app/controllers/application.rb
index d387319..a637487 100644
--- a/wui/src/app/controllers/application.rb
+++ b/wui/src/app/controllers/application.rb
@@ -34,7 +34,7 @@ class ApplicationController < ActionController::Base
 
   def get_login_user
     # user_from_principal(request.env["HTTP_X_FORWARDED_USER"])
-    'admin'
+    'dpierce'
   end
   
   def user_from_principal(principal)
diff --git a/wui/src/app/controllers/hardware_controller.rb
b/wui/src/app/controllers/hardware_controller.rb
index 599d881..734d0b2 100644
--- a/wui/src/app/controllers/hardware_controller.rb
+++ b/wui/src/app/controllers/hardware_controller.rb
@@ -107,7 +107,7 @@ class HardwareController < ApplicationController
 
   def users_json
     json_list(@pool.permissions, 
-              [:id, :user, :user_role])
+              [:id, :name, :user_role])
   end
 
   def storage_pools_json
diff --git a/wui/src/app/controllers/permission_controller.rb
b/wui/src/app/controllers/permission_controller.rb
index 086e183..c0cbc38 100644
--- a/wui/src/app/controllers/permission_controller.rb
+++ b/wui/src/app/controllers/permission_controller.rb
@@ -39,6 +39,8 @@ class PermissionController < ApplicationController
   def new
     @permission = Permission.new( { :pool_id => params[:pool_id]})
     @perms = @permission.pool.permissions
+    filter = Permission.find(:all).collect{ |permission| permission.uid }
+    @users = Account.names(filter)
     set_perms(@permission.pool)
     # admin permission required to view permissions
     unless @can_set_perms
@@ -57,7 +59,7 @@ class PermissionController < ApplicationController
       redirect_to_parent
     else
       if @permission.save
-        render :json => "created User Permissions for 
#{@permission.user}".to_json
+        render :json => "created User Permissions for 
#{@permission.uid}".to_json
       else
       # FIXME: need to handle proper error messages w/ ajax
         render :action => 'new'
diff --git a/wui/src/app/models/account.rb b/wui/src/app/models/account.rb
index 2664f18..e3a1a54 100644
--- a/wui/src/app/models/account.rb
+++ b/wui/src/app/models/account.rb
@@ -22,6 +22,8 @@
 class Account < ActiveLdap::Base
   ldap_mapping :dn_attribute => 'cn', :prefix =>
'ou=Users', :scope => :one
 
+  @@users = nil
+
   # +query+ returns the set of all accounts that contain the given search
value.
   #
   # This API requires that a previous connection be made using 
@@ -29,13 +31,33 @@ class Account < ActiveLdap::Base
   #
   def Account.query(value)
 
-    @users = Account.find(:all, value)
+    @@users ||= Account.find(:all, value)
 
     if block_given?
-      @users.each { |user| yield(user) }
+      @@users.each { |user| yield(user) }
     end
 
-    return @users
-    
+    @@users    
   end
+
+  # Retrieves the list of users from LDAP and returns a hash of
+  # their uids, indexed by their common name in the form:
+  # +username (uid) => uid+
+  #
+  # if a filter is passed in, those user ids are filtered out
+  # of the returned list.
+  #
+  def Account.names(filter = [])
+    result = {}
+
+    Account.query('*') do |user|
+      unless filter.include? user.uid
+	key = "#{user.cn} (#{user.uid})"
+	result[key] = user.uid
+      end
+    end
+
+    result.sort
+  end
+
 end
diff --git a/wui/src/app/models/permission.rb b/wui/src/app/models/permission.rb
index 7d80e1e..0aab16c 100644
--- a/wui/src/app/models/permission.rb
+++ b/wui/src/app/models/permission.rb
@@ -50,11 +50,18 @@ class Permission < ActiveRecord::Base
     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
diff --git a/wui/src/app/views/permission/_form.rhtml
b/wui/src/app/views/permission/_form.rhtml
index cb0abea..2a1e93c 100644
--- a/wui/src/app/views/permission/_form.rhtml
+++ b/wui/src/app/views/permission/_form.rhtml
@@ -5,8 +5,7 @@
 
 <%= select_with_label 'Role', 'permission',
'user_role', Permission::ROLES.keys  %>
 
-<%= text_field_with_label 'User', 'permission',
'uid'  %>
-
+<%= select_with_label 'User', 'permission', 'uid',
@users %>
 
 <!--[eoform:permission]-->
 
diff --git a/wui/src/config/ldap.yml b/wui/src/config/ldap.yml
index 796c334..243707f 100644
--- a/wui/src/config/ldap.yml
+++ b/wui/src/config/ldap.yml
@@ -1,7 +1,7 @@
 development:
-  host: ldap.for.your.domain.com
+  host: ldap.rdu.redhat.com
   port: 389
-  base: dc=domain,dc=com
+  base: dc=redhat,dc=com
 
 test:
   host: ldap.for.your.domain.com
-- 
1.5.4.1

Darryl L. Pierce wrote:
> diff --git a/wui/src/app/controllers/permission_controller.rb
b/wui/src/app/controllers/permission_controller.rb
> index 086e183..c0cbc38 100644
> --- a/wui/src/app/controllers/permission_controller.rb
> +++ b/wui/src/app/controllers/permission_controller.rb
> @@ -39,6 +39,8 @@ class PermissionController < ApplicationController
>    def new
>      @permission = Permission.new( { :pool_id => params[:pool_id]})
>      @perms = @permission.pool.permissions
> +    filter = Permission.find(:all).collect{ |permission| permission.uid }
> +    @users = Account.names(filter)
>      set_perms(@permission.pool)
>      # admin permission required to view permissions
>      unless @can_set_perms
>   
You're filtering out _all_ users that have a permission somewhere -- 
this should just filter out users that already have permissions on 
_this_ pool.
Something like this would be better:
@pool = Pool.find(params[:pool_id])
filter = @pool.permissions.collect{ |permission| permission.uid }
> diff --git a/wui/src/app/models/account.rb b/wui/src/app/models/account.rb
> index 2664f18..e3a1a54 100644
> --- a/wui/src/app/models/account.rb
> +++ b/wui/src/app/models/account.rb
> @@ -22,6 +22,8 @@
>  class Account < ActiveLdap::Base
>    ldap_mapping :dn_attribute => 'cn', :prefix =>
'ou=Users', :scope => :one
>  
> +  @@users = nil
> +
>    # +query+ returns the set of all accounts that contain the given search
value.
>    #
>    # This API requires that a previous connection be made using 
> @@ -29,13 +31,33 @@ class Account < ActiveLdap::Base
>    #
>    def Account.query(value)
>  
> -    @users = Account.find(:all, value)
> +    @@users ||= Account.find(:all, value)
>  
>      if block_given?
> -      @users.each { |user| yield(user) }
> +      @@users.each { |user| yield(user) }
>      end
>  
> -    return @users
> -    
> +    @@users    
>    end
>   
Do you really need a class variable here? This could cause unnecessary 
concurrency problems. Since you aren't referencing it anywhere else why 
not just a plain local variable?


Scott