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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
