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

[Martin Kosek]

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