[Freeipa-devel] [PATCH][WIP] LDAP encoding redone

[Jan Cholasta]

Dne 29.6.2012 21:07, Rob Crittenden napsal(a):
> Jan Cholasta wrote:
>> Hi,
>>
>> this is the next patch in the input validation & handling series
>> <https://fedorahosted.org/freeipa/ticket/2357>. It changes the way
>> entries are encoded and decoded in the LDAP backend.
>>
>> The patch consists of several changes:
>>
>>    * Refactored the Encoder class to be more universally usable. It uses
>> a polymorphic interface, which hopefully makes the encoding code more
>> readable.
>>
>>    * Attribute values now use Python data types matching the syntax of
>> the attribute. This removes the need to decode/encode the values from/to
>> raw LDAP values in the CallbackInterface callbacks as well as other
>> parts of IPA.
>>
>>    * On command output, attribute values are converted to strings so
>> that the resulting entry is the same as it is without the patch. I don't
>> like this code and I'd like to get rid of at least some parts of it, but
>> I'm not sure how that would affect API compatibility. Removing the
>> special case for boolean values would fix
>> <https://fedorahosted.org/freeipa/ticket/2025>.
>>
>>    * Entries are more strictly checked when they are encoded and
>> decoded. Values of multi-value attributes must be lists (not tuples!) of
>> objects of the appropriate python type, values of single-value
>> attributes must be objects of the appropriate python type. This helps
>> detecting data type errors that would previously go unnoticed.
>>
>>    * Some parameters use data type that doesn't match the syntax of the
>> according attribute, or are single-value even when the according
>> attribute is multi-value. Values of such parameters wouldn't pass the
>> new strict checking if they were used in attributes without
>> modifications. To remedy this, added a new parameter option
>> attr_convertor, which allows specifying a custom function for converting
>> parameter values to attribute values.
>>
>> Note that this is work in progress, some things may be (and certainly
>> are) broken, there is some low-quality code and docstrings, comments and
>> tests are TBD.
>>
>> Suggestions and comments are welcome.
>>
>> Honza
>
> I haven't tried this yet, but this change jumped out at me:
>
> if attr not in ('aciname', 'group', 'memberof', 'nsaccountlock',
> 'subtree', 'targetgroup', 'type') and self.obj is not None and attr in
> self.obj.params and 'virtual_attribute' not in self.obj.params[attr].flags:
>
> Why exclude this subset of attributes?

Generally, attribute are returned in the form returned by python-ldap, 
that is lists of strings. This rule does not apply to the attributes in 
the subset (most of them are single strings, nsaccountlock is a single 
boolean), so they must be excluded from the conversion.

>
> Is the big block of code adding to __call__ meant to maintain backwards
> compatibility?

Yes.

Like I said above, I'm not sure how much of this is really necessary and 
what can be safely removed.

We can skip the whole thing for new clients and return entries without 
converting them first, but that would require modifications on the 
client side as well.

If the excluded attribute subset stays, it would be better to introduce 
a new parameter flag that inhibits the conversion, instead of having a 
hardcoded list of attributes.

>
> This seems to make lists out of a lot of things that weren't previously
> lists. Is that to satisfy the schema?

Exactly.

I am concerned about how much we can depend on the schema to be what we 
expect it to be. I know that an attribute value might not match the 
syntax of that attribute because of replication 
(<https://fedorahosted.org/freeipa/ticket/2131>). It is probably also 
possible for a single-value attribute to have more than one value this 
way. But what if someone changes the schema so that an attribute that 
was previously single-value becomes multi-value? Do we allow that? The 
code in my patch wouldn't cope with such a situation very well. Maybe it 
would be better to require all (including single-value) attributes to be 
represented by lists in IPA...?

>
> rob

Honza

-- 
Jan Cholasta