On 02/28/2012 03:19 PM, Jan Cholasta wrote:
On 28.2.2012 11:54, Petr Viktorin wrote:
On 02/27/2012 10:44 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/20/2012 08:51 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
https://fedorahosted.org/freeipa/ticket/2159 says various config
are not marked Required, so entering an empty value for it will pass
validation (and IPA will blow up later when it expects a string,not
None). Forexample the following:
$ ipa config-mod --groupsearch=
fails with AttributeError: 'NoneType' object has no attribute 'split'

There is a more general problem behind this, though: even if the
attributes *are* marked as Required, an empty string will pass
validation. This is because `None` is used in `Param.validate` to
both "No value supplied" and "Empty value supplied". The method
currently assumes the former, and skips validation entirely for
values to optional parameters.

For example, the following will delete "membergroup", even though
it's a
required attribute :

$ ipa delegation-add --attrs=street --group=editors \
--membergroup=admins td1
$ ipa delegation-mod --membergroup= td1

Note that some LDAPObjects handle this with a _check_empty_attrs
function, so they aren't affected. That function is specific to LADP
objects, though. So I needed to tackle this on a lower level.

This patch solves the problem by
* adding a 'nonempty' flag when a required parameter of a CRUD Update
object is auto-converted to a non-required parameter
* making the`validate` method aware of whether the parameter was
supplied; and if it was, honor the "nonempty" flag.

The second patch fixes
https://fedorahosted.org/freeipa/ticket/2159 by
marking required config options as required.

This looks good but I think there are other things to protect in
as well such as the default e-mail domain. It is probably safe to say
that everything in there is required.


Let me just double-check this with you.

According to code in the user plugin (around line 330), if the default
e-mail domain is not set, users don't get an address auto-assigned. Do
we really want to require user e-mails?

ipaconfigstring (the password plugin flags) are a set (multivalue, not

The rest of the values I left as not required are for optional features
or limits: search results & time limit, max. username length, password
expiry notification. Currently if these are missing, the feature/limit
is disabled (well, except for the time limit).
But, there are also special values (0 or -1) that have the same effect
as a missing value. Sometimes they're documented.
So we want to enforce that users use these special values instead of
removing the config entry?

I think we want to enforce that these are defined. It will be confusing
for users if these are not there at all. I don't think we need to show
the special options, just declare that the attribute is required.


Attaching updated patch 13.

Only the default e-mail domain
(https://fedorahosted.org/freeipa/ticket/2409) and ipaconfigstring are
still optional.

You have removed all the config-related defensive code in the patch, is
this a good idea? What will happen if someone e.g. deletes a required
config attribute directly from LDAP?

Then IPA crashes. The defensive code wasn't there for all cases anyway, as ticket #2159 shows.

If we want to protect against this it would probably be better to make the config class itself give the default when a required value is missing.


Freeipa-devel mailing list

Reply via email to