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

[Rob Crittenden]

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

rob