On 03/26/2010 09:37 AM, Scott Seago wrote:
> 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.
Also making it accept an array may be tricky though, as many calls to
require_privilege require a second, different argument, eg the target
object which the user needs permissions to in order to do the action (eg
require_privilege(POOL_VIEW, @pool)).
>>> +
>>> + @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.
Also since this method was being added to the portal pool controller,
and all of those actions do not require USER_MODIFY, it wouldn't work to
register that check as a filter. I suppose we could add it as a filter
for only the relevant methods, but this way might be the simplest.
>>> @@ -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 wasn't editing this line w/ this patch (no preceding +/-), just a
relic of the diff system.
> 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.
Agree, also like the clearer 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
>
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
