[Freeipa-devel] [PATCHES] 0562-0563 Fix internal error when global policy is not readable

[Petr Viktorin]

On 05/30/2014 03:33 PM, Rob Crittenden wrote:
> Petr Viktorin wrote:
>> On 05/30/2014 11:02 AM, Petr Viktorin wrote:
>>> On 05/29/2014 07:13 PM, Rob Crittenden wrote:
>>>> Petr Viktorin wrote:
>>>>> When investigating this issue I became very annoyed by the star import
>>>>> hiding where names come from, so I did some cleanup first.
>>>>>
>>>>>
>>>>> In krbtpolicy, an ACIError is now raised if:
>>>>> - the user doesn't have permission to read any one of the ticket policy
>>>>>     attributes on the requested entry
>>>>>     (checked using attribute-level rights)
>>>>> - any ticket policy attribute from the default policy is not available
>>>>>     (either not readable, or not there at all)
>>>>>     (only checked if these are accessed, i.e. when the user entry
>>>>> doesn't
>>>>>      override all of the defaults, or when requesting the global policy)
>>>>>
>>>>> That means if the user is not available at all, you get a NotFound, but
>>>>> if global policy is not found it's assumed that it's just unreadable.
>>>>
>>>> That seems reasonable to me.
>>>>
>>>> I also noticed a typo, ddoesn't
>>>
>>> Fixed, thanks.
>>>
>>>> In the lower-level code, ldap2.py, we have some help
>>>> can_[read|write|etc] for managing rights. Would doing something similar
>>>> in baseldap be better than embedding the logic into each plugins?
>>>>
>>>> So instead of this:
>>>>
>>>>                       if rights is None:
>>>>                           rights = baseldap.get_effective_rights(
>>>>                               ldap, dn, self.obj.default_attributes)
>>>>                       if 'r' not in rights.get(attrname.lower(), ''):
>>>>                           raise errors.ACIError(
>>>>                               info=_('Ticket policy for %s could not be
>>>> read') %
>>>>                                   keys[-1])
>>>>
>>>> You'd have this:
>>>>
>>>>                       if not baseldap.can_read(ldap, dn, attrname):
>>>>                           raise errors.ACIError(
>>>>                               info=_('Ticket policy for %s could not be
>>>> read') %
>>>>                                   keys[-1])
>>>
>>> The second is definitely better.
>>>
>>>> This may end up fetching the rights multiple times depending on how many
>>>> things need to be read, so perhaps passing that in if you have it
>>>> already.
>>>
>>> This however means doing the get_effective_rights call anyway to get all
>>> the needed attrs, at which point most of the readability benefits are
>>> gone.
>>>
>>> Actually I'd like to add some good attrlevelrights handling right into
>>> ipaldap. Let's do this correctly rather than add a quick fix to the
>>> helpers just so we can use them.
>>> Issue here: https://fedorahosted.org/freeipa/ticket/4362
>
> Sure, I'm ok with waiting and doing it right.
>
>> Once more, with patches
>
> ACK x2

Thanks!
Pushed to master: 63a2147ac2bca82c710a6ffd025d4dbd8f1b3449


-- 
Petr³