[Freeipa-devel] [PATCH] 940 apply some validation to some classes only

[Martin Kosek]

On Mon, 2012-02-06 at 11:52 -0500, Rob Crittenden wrote:
> Martin Kosek wrote:
> > On Fri, 2012-02-03 at 16:58 -0500, Rob Crittenden wrote:
> >> There is some validation that we only need to apply when an entry is
> >> being created, namely the key itself. This is to allow us to manage an
> >> otherwise illegal entry that finds its way into the system (i.e. migration).
> >>
> >> Consider this. We migrate a group with a space in it. This isn't allowed
> >> in IPA but we also provide no way to delete it because the cn regex
> >> kicks out the group-del command.
> >>
> >> The trick is adding appropriate context so we can know during validation
> >> how we got here. A command object has a bases field which contains the
> >> base classes associated with it, which appears to contain only the leaf
> >> baseclass. So using this we can tell how we got to validation and can
> >> skip based on that baseclass name.
> >>
> >> I went back and forth a bit on where the right place to put this was,
> >> I'm open to more fine tuning. I initially skipped just the pattern
> >> validation then expanded it to apply to all validation in the Parameter.
> >>
> >> rob
> >
> > 1) This patch breaks API.txt, it needs re-generating.
> >
> > 2) This may be a matter of opinion but I think that
> > +        if self.onlyvalidateclasses and \
> > +          not any(x in self.onlyvalidateclasses for x in classbases):
> > +            return
> >
> > would be more readable than:
> >
> > +        if self.onlyvalidateclasses and \
> > +          not [x for x in classbases if x in self.onlyvalidateclasses]:
> > +            return
> >
> > 3) I would move the entire self.onlyvalidateclasses up to the validate()
> > method. It would have several benefits:
> > - the validation skip would be done just once, not for every value as
> > the decision does not use the value at all
> > - we would not have to modify _validate_scalar() methods at all since
> > they won't need classbases
> >
> > 4) I think it would be better to keep validation for --rename options.
> > As it is generated from RDN attribute parameter, it inherits
> > onlyvalidateclasses list as well.
> >
> > Otherwise your patch would let user to rename a valid object to an
> > invalid one:
> >
> > # ipa user-mod fbar --rename="bad boy"
> > --------------------
> > Modified user "fbar"
> > --------------------
> >    User login: bad boy
> >    First name: Foo
> >    Last name: Bar
> >    Home directory: /home/fbar
> >    Login shell: /bin/sh
> >    UID: 480800040
> >    GID: 480800040
> >    Account disabled: False
> >    Password: False
> >    Member of groups: ipausers
> >    Kerberos keys available: False
> >
> > Martin
> >
> 
> This should address your concerns.
> 
> rob
> 

Its almost OK, there is just one part that's not that OK:

@@ -831,6 +836,9 @@ class Param(ReadOnly):
                 else:
                     raise RequirementError(name=self.name)
             return
+        if 'rename' not in (self.name, self.cli_name):
+            if self.onlyvalidateclasses and not [x for x in self.onlyvalidateclasses if x in classbases]: #pylint: disable=E1101
+                return
         if self.multivalue:
             if type(value) is not tuple:
                 raise TypeError(

I don't think that hard-coding this skip for onlyvalidateclasses based
just on parameter name is a good thing to do and may cause problems in
the future. For example if we create some option called "rename" and
deliberately set onlyvalidateclasses for the option - it would then be
skipped as well.

I think it would be a better solution to just update
_get_rename_option() in baseldap.py to set onlyvalidateclasses to ()

Martin