Petr Viktorin wrote:
On 02/29/2012 11:14 AM, Jan Cholasta wrote:
On 29.2.2012 11:09, Petr Viktorin wrote:
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
options
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
mean
both "No value supplied" and "Empty value supplied". The method
currently assumes the former, and skips validation entirely for
`None`
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
config
as well such as the default e-mail domain. It is probably safe to
say
that everything in there is required.
rob
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
required).
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.
rob
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.
This, and raise an error in cases where no default is available (the
check should probably be done in ldap.get_ipa_config).
Honza
Would a better approach be to modify the LDAP schema to require these
values?
I think that may be a longer-term fix. I propose we keep the defensive
code in for now and correct it in the future.
rob
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
