[Freeipa-devel] [PATCH] 784 limit what attributes may be modified

[Rob Crittenden]

Martin Kosek wrote:
> On Fri, 2011-05-27 at 11:10 -0400, Rob Crittenden wrote:
>> Martin Kosek wrote:
>>> On Mon, 2011-05-16 at 17:46 -0400, Rob Crittenden wrote:
>>>> Add option to limit the attributes allowed in an entry.
>>>>
>>>> Kerberos ticket policy can update policy in a user entry. This allowed
>>>> set/addattr to be used to modify attributes outside of the ticket policy
>>>> perview, also bypassing all validation/normalization. Likewise the
>>>> ticket policy was updatable by the user plugin bypassing all validation.
>>>>
>>>> Add two new LDAPObject values to control this behavior:
>>>>
>>>> limit_object_classes: only attributes in these are allowed
>>>> disallow_object_classes: attributes in these are disallowed
>>>>
>>>> By default both of these lists are empty so are skipped.
>>>>
>>>> ticket 744
>>>>
>>>> rob
>>>
>>> NACK. I have some concerns with this patch. In function
>>> _check_limit_object_class:
>>>
>>> 1) You change input attribute 'attrs' by removing the items from it. If
>>> user passes the same list of attrs to be checked and the function is run
>>> twice, the 'attrs' parameter in second run is corrupt.
>>>
>>> You can try it by running e.g. `ipa krbtpolicy-mod --maxrenew=24044' and
>>> checking the value of this parameter in the function.
>>
>> Good catch, updated patch attached.
>>
>>>
>>> 2) The purpose of this statement is not clear to me:
>>> +    if len(attrs)>   0 and allow_only:
>>> +        raise errors.ObjectclassViolation(info='attribute "%(attribute)s" not allowed' % dict(attribute=attrs[0]))
>>> Maybe just the exception text is misleading.
>>
>> This function has 2 modes: "allow only the attributes in these
>> objectclasses" or "specifically deny the attributes in these
>> objectclasses". This enforces the first type. If when we've gone through
>> all the attributes there are any left over they must not be allowed so
>> raise an error. This is documented in the function header.
>
> Thanks for explanation, now I get it. It all looks OK, ACK.
>
> Martin
>
>

pushed to master and ipa-2-0