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

[Martin Kosek]

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