[Freeipa-devel] [PATCHES] 0012-13 Don't allow deleting required config options

[Martin Kosek]

On Fri, 2012-03-02 at 10:07 +0100, Petr Viktorin wrote:
> On 02/29/2012 04:09 PM, Petr Viktorin wrote:
> > On 02/29/2012 03:53 PM, Rob Crittenden wrote:
> >> 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
> >
> > Here is an updated patch 13 that does that.
> >
> 
> And here is patch 12 rebased against current master.
> 

ACK for 0012-02 and 0013-03. I think this is a good compromise for now.

Pushed to master, ipa-2-2.

Martin