Martin Kosek wrote:
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.


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


This should address your concerns.


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

@@ -831,6 +836,9 @@ class Param(ReadOnly):
                      raise RequirementError(
+        if 'rename' not in (, 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 to set onlyvalidateclasses to ()

I'm not too keen on it either but otherwise all validation would happen whenever there is a rename option, which is what we're trying to prevent.


Freeipa-devel mailing list

Reply via email to