[Freeipa-devel] [PATCHES 0024-0025] Improvements to idrange.py

[Martin Kosek]

On 02/25/2013 02:16 PM, Tomas Babej wrote:
> On Fri 22 Feb 2013 04:34:55 PM CET, Martin Kosek wrote:
>> On 02/22/2013 03:01 PM, Tomas Babej wrote:
>>> On 02/21/2013 02:22 PM, Martin Kosek wrote:
>>>> On 02/20/2013 03:19 PM, Tomas Babej wrote:
>>>>> On Wed 20 Feb 2013 02:24:03 PM CET, Alexander Bokovoy wrote:
>>>>>> On Wed, 20 Feb 2013, Tomas Babej wrote:
>>>>>>> On 12/21/2012 12:15 PM, Tomas Babej wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Sending updated and rebased versions of patches 0024 and 0025.
>>>>>>>>
>>>>>>>> Tomas
>>>>>>>>
>>>>>>>>
>>>>>>> Sending rebased version, these got quite rotten.
>>>>>> Thanks for updating them.
>>>>>>
>>>>>>> @@ -504,25 +515,37 @@ class idrange_mod(LDAPUpdate):
>>>>>>>                               'not be found. Please specify the SID
>>>>>>> directly '
>>>>>>>                               'using dom-sid option.'))
>>>>>>>
>>>>>>> -        try:
>>>>>>> -            (old_dn, old_attrs) = ldap.get_entry(dn,
>>>>>>> -                                                ['ipabaseid',
>>>>>>> -                                                'ipaidrangesize',
>>>>>>> -                                                'ipabaserid',
>>>>>>> -                                                'ipasecondarybaserid'])
>>>>>>> -        except errors.NotFound:
>>>>>>> -            self.obj.handle_not_found(*keys)
>>>>>>> +        if in_updated_attrs('ipanttrusteddomainsid'):
>>>>>>> +            if in_updated_attrs('ipasecondarybaserid'):
>>>>>>> +                raise errors.ValidationError(name='ID Range setup',
>>>>>>> +                    error=_('Options dom_sid and secondary_rid_base
>>>>>>> cannot '
>>>>>>> +                            'be used together'))
>>>>>> Since we agreed to refer to options by their CLI name (--dom-sid and
>>>>>> --secondary-rid-base) in the other patch, it makes sense to use it
>>>>>> here too.
>>>>>>
>>>>>>
>>>>>>> -        if is_set('ipanttrusteddomainsid'):
>>>>>>> -            # Validate SID as the one of trusted domains
>>>>>>> -
>>>>>>> self.obj.validate_trusted_domain_sid(entry_attrs['ipanttrusteddomainsid'])
>>>>>>>
>>>>>>> +            if not in_updated_attrs('ipabaserid'):
>>>>>>> +                raise errors.ValidationError(name='ID Range setup',
>>>>>>> +                    error=_('Options dom_sid and rid_base must '
>>>>>>> +                            'be used together'))
>>>>>> Same here.
>>>>>>
>>>>>>> +            # secondary base rid must be set if and only if base rid
>>>>>>> is set
>>>>>>> +            if in_updated_attrs('ipasecondarybaserid') !=\
>>>>>>> +                in_updated_attrs('ipabaserid'):
>>>>>>> +                raise errors.ValidationError(name='ID Range setup',
>>>>>>> +                    error=_('Options secondary_rid_base and rid_base
>>>>>>> must '
>>>>>>> +                            'be used together'))
>>>>>> Same here.
>>>>>>
>>>>>>> +        dict(
>>>>>>> +            desc='Try to modify ID range %r so it has only primary
>>>>>>> rid range set' % (testrange8),
>>>>>>> +            command=('idrange_mod', [testrange8],
>>>>>>> +                      dict(ipabaserid=testrange8_base_rid)),
>>>>>>> +            expected=errors.ValidationError(
>>>>>>> +                name='ID Range setup', error='Options
>>>>>>> secondary_rid_base and rid_base must be used together'),
>>>>>>> +        ),
>>>>>> And synchronize error message here too.
>>>>>>
>>>>> Thanks!
>>>>>
>>>>> Sending the updated patch 0024.
>>>>>
>>>>> Tomas
>>>>>
>>>> In patch 0024 your intention is OK, but the checking functions are not:
>>>>
>>>>            is_set = lambda x: (x in entry_attrs) and (x is not None)
>>>> +        in_updated_attrs = lambda x: any((x in attrs and x is not None)
>>>> +                                         for attrs in (entry_attrs,
>>>> old_attrs))
>>>>
>>>> They return True even when the attribute is None because they check if *x* is
>>>> None and not if *attrs[x]* is None. Example:
>>>>
>>>> # ipa idrange-add --base-id=1200000 --range-size=200000 --rid-base=1000
>>>> --secondary-rid-base=1000000 local_range
>>>> ----------------------------
>>>> Added ID range "local_range"
>>>> ----------------------------
>>>>     Range name: local_range
>>>>     First Posix ID of the range: 1200000
>>>>     Number of IDs in the range: 200000
>>>>     First RID of the corresponding RID range: 1000
>>>>     First RID of the secondary RID range: 1000000
>>>>     Range type: local domain range
>>>>
>>>> This command should be NOOP, but is not:
>>>>
>>>> # ipa idrange-mod local_range --dom-sid=
>>>> ipa: ERROR: invalid 'ID Range setup': Options dom-sid and secondary-rid-base
>>>> cannot be used together
>>>>
>>>> Martin
>>> Thanks for the catch, all checking functions fixed.
>>>
>>> Sending the complete patchset, up to date.
>>>
>>> Tomas
>>
>> I think I am being a nitpicker now (sorry), but this condition also fails if
>> the old_attrs has a setting, but I am removing it in a current -mod command:
>>
>> # ipa idrange-add --base-id=1200000 --range-size=200000 --rid-base=1000
>> --secondary-rid-base=1000000 local_range
>> ----------------------------
>> Added ID range "local_range"
>> ----------------------------
>>    Range name: local_range
>>    First Posix ID of the range: 1200000
>>    Number of IDs in the range: 200000
>>    First RID of the corresponding RID range: 1000
>>    First RID of the secondary RID range: 1000000
>>    Range type: local domain range
>> # ipa idrange-mod local_range --dom-sid S-1-2 --secondary-rid-base=
>> ipa: ERROR: invalid 'ID Range setup': Options dom-sid and secondary-rid-base
>> cannot be used together
>>
>> Martin
> 
> Yep. Ok, the command should now handle this use case as well.
> 
> Tomas

Thanks, it is working fine now.

ACK. Pushed to master, ipa-3-1.

Martin