[Freeipa-devel] [PATCH 72] Validate DN & RDN parameters for migrate command

[Martin Kosek]

On Tue, 2012-04-10 at 11:03 -0400, John Dennis wrote:
> On 04/06/2012 10:11 AM, John Dennis wrote:
> > On 04/06/2012 04:40 AM, Martin Kosek wrote:
> >> 1) We still crash when the parameter is empty. We may want to make it
> >> required (the same fix Rob did for cert rejection reason):
> >>
> >> # echo "secret123" | ipa migrate-ds ldap://vm-054.idm.lab.bos.redhat.com
> >> --with-compat --base-dn="dc=greyoak,dc=com" --user-container=
> >> ipa: ERROR: cannot connect to
> >> u'http://vm-022.idm.lab.bos.redhat.com/ipa/xml': Internal Server Error
> >
> > Good point, will fix.
> >
> >> 2) Do you think it would make sense to create a special Param for DN?
> >> Its quite general type and I bet there are other Params that could use
> >> DN instead of Str. It could look like that:
> >>           DN('usercontainer?',
> >>               rdn=True,<<<<   can be RDN, not DN
> >
> > Yes, I considered introducing a new DN parameter type as well. I think
> > this is a good approach and will have payoff down the road. I will make
> > that change. However I'm inclined to introduce both a DN parameter and a
> > RDN parameter, they really are different entities, if you use a flag to
> > indicate you require a RDN then you lose the type of the parameter, it
> > could be either, and it really should be one or the other and knowing
> > which it is has value.
> >
> >> 3) We should not restrict users from passing a user/group container with
> >> more than one RDN:
> >
> > Yeah, I wasn't too sure about that. The parameter distinctly called for
> > an RDN, but it seemed to me it should support any container below the
> > base, which would make it a DN not a RDN.
> >
> > FWIW, a DN is an ordered sequence of 1 or more RDN's. DN's do not have
> > to be "absolute", they can be any subset of a "path sequence".  A DN
> > with exactly one RDN is equivalent to an RDN, a special case).
> >
> > I will change the parameter to be a DN since that is what was the
> > original intent.
> >
> > All good suggestions, a revised patch will follow shortly.
> 
> Hmm... I'm rethinking this. The suggestion of adding a new DN parameter 
> is the right thing to do and I tried to implement it, but as I was 
> working I realized I needed to touch a fair amount of code to support 
> the new parameter type and to modify multiple places in the migration 
> code to work with the new type. That's more code changes at the very 
> last minute for this release than I'm comfortable with, we're too close 
> the freeze date for invasive modifications.

Ok, I agree with this approach. Lets fix just points 1) and 3) in my
intial review.

Martin