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

[Jan Cholasta]

On 10.4.2012 17:03, 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.
>
> We have a work item open to introduce DN types throughout the code
> including as a new Param type. I think it's best to make just a small
> tweak to fix the traceback today and wait till the 3.0 work to "do it
> right". Therefore I'm no longer in favor of the new Param approach. I
> will fix the problem you reported when the parameter is empty, but merge
> that into the original patch.
>

Just for the record, there are some related tickets: 
<https://fedorahosted.org/freeipa/ticket/2033>, 
<https://fedorahosted.org/freeipa/ticket/2265>.

Honza

-- 
Jan Cholasta