Michal Fojtik wrote:
On 24/03/10 14:58 -0400, Mohammed Morsi wrote:
Hi,
Just few notes:
Michal,
I'm actually in the process of doing this in a different way that's a bit more generic, so this patch won't be pushed as-is -- but I've got a couple responses to your comments here.
- def new_user
- require_privilege(Privilege::POOL_MODIFY)
- require_privilege(Privilege::USER_MODIFY)
Why you can't just simply pass a Array as an argument to this method ? Something like require_privileges([1, 2]).
Actually I think in this case what we really want is SET_PERMS. However making this method accept an array would be useful if we find any other cases where we want to verify two different privileges on a single object -- I'm not sure we have any current need for that though.
- @users = User.find(:all)
- @roles = Role.find(:all, :conditions => [ 'scope = ?', 'PortalPool' ])
- @pool = PortalPool.find(params[:id])
- end
- def create_user
- require_privilege(Privilege::POOL_MODIFY)
- require_privilege(Privilege::USER_MODIFY)
In other hands, why not create a 'before_filter' for this ?
before_filter :require_privileges
We did perm checks with before filters in ovirt, but the end result was somewhat messy. Normally require_privilege takes an object as a second argument -- for which object we're requiring the privilege on (in fact for this patch @pool should be passed in here). So when we did permission checks in before_filters we had to have additional before_filters to do whatever is required to determine what object we're using for permissions checks. In several cases this resulted with an action with a completely empty body and a several-line before_filter, which made the controller flow rather difficult to follow.
@@ -40,3 +40,4 @@ <%= link_to "Hardware Profiles", {:action => "hardware_profiles", :id => @pool.id}, :class=>"actionlink"%>
This could be: <%= link_to "Hardware Profiles", hardware_profiles_path(@pool), :class => "actionlink"%>
I prefer the former link, as it's easier to read -- clearly identifying action, object (id), and (when appropriate) controller. But it's more of a personal preference -- no big deal either way.
(btw. @pool == hardware_profile ?)
Pools and HW profiles are different object types. In this case the hardware_profiles action on the pool controller displays a list of hardware profiles associated with the selected pool.
- Michal
Scott