[Freeipa-devel] [PATCH 0021] Forbid overlapping rid ranges for the same id range

Fri Dec 14 13:49:52 UTC 2012

On 12/14/2012 01:59 PM, Alexander Bokovoy wrote:
> On Fri, 14 Dec 2012, Tomas Babej wrote:
>> On 12/13/2012 02:48 PM, Martin Kosek wrote:
>>> On 12/13/2012 11:52 AM, Tomas Babej wrote:
>>>> On 12/12/2012 04:32 PM, Martin Kosek wrote:
>>>>> On 10/26/2012 03:43 PM, Tomas Babej wrote:
>>>>>> Hi,
>>>>>>
>>>>>> creating an id range with overlapping primary and secondary
>>>>>> rid range using idrange-add or idrange-mod command now
>>>>>> raises ValidationError. Unit tests have been added to
>>>>>> test_range_plugin.py.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/3171
>>>>>>
>>>>>> Tomas
>>>>>>
>>>>> 1) Add command can cause crash:
>>>>>
>>>>> # ipa idrange-add range9 --base-id=1000 --rid-base=1000 
>>>>> --secondary-rid-base=
>>>>> --range-size 1000
>>>>> ipa: ERROR: an internal error has occurred
>>>>>
>>>>> 2) I don't like this construct very much:
>>>>>
>>>>> updated_values = dict(zip(rid_range_attributes,[None]*3))
>>>>>
>>>>> This would look better, IMO:
>>>>> updated_values = dict((attr, None) for attr in rid_range_attributes)
>>>>>
>>>>> Why do you need this dict pre-created anyway? You overwrite all 
>>>>> keys here:
>>>>>
>>>>> +            for attr in rid_range_attributes:
>>>>> +                if attr in entry_attrs:
>>>>> +                    updated_values[attr] = entry_attrs[attr]
>>>>> +                else:
>>>>> +                    updated_values[attr] = int(old_attrs[attr][0])
>>>>>
>>>>>
>>>>> 3) [nitpick] We don't end ValidationError with '.':
>>>>>
>>>>> +                   raise errors.ValidationError(name='ID Range 
>>>>> setup',
>>>>> +                       error=_("Primary rid range and secondary 
>>>>> rid range"\
>>>>> +                               " cannot overlap."))
>>>>>
>>>>> There is also a duplication of the same error message...
>>>>>
>>>>> 4) The -mod operation will also crash:
>>>>>
>>>>> # ipa idrange-add range9 --base-id=1000 --rid-base=1000
>>>>> --secondary-rid-base=2000 --range-size 1000
>>>>> # ipa idrange-mod range9 --secondary-rid-base=
>>>>> ipa: ERROR: an internal error has occurred
>>>>>
>>>>> Martin
>>>> New patch version as well as diff between
>>>> patch versions (for your convenience) attached.
>>>>
>>>> Tomas
>>> 1) You introduced mixed spaces and tabs - Python gods do not like that!
>> Oops, I'd rather send a fixed patch sooner than they bring down their 
>> wrath on me. :)
>>> 2) This is a nitpick, but there are too many redundant parens and 
>>> brackets in
>>> this statement:
>>>
>>> +    if(any([attr is None for attr in [rid_base,secondary_rid_base, 
>>> size]])):
>>> +            return False
>>>
>>> This would look nicer and would not create unnecessary list:
>>>
>>> +    if any(attr is None for attr in (rid_base, secondary_rid_base, 
>>> size)):
>>> +            return False
>> Brackets reduced.
>>> 3) Another construct I did not like very much:
>>>
>>> +    is_set = lambda x : (x in entry_attrs) and not (x is None)
>>>
>>> a) "x is not None" reads better than "not (x is None)"
>>> b) I would rather replace all is_set lambdas with use of "if
>>> entry_attrs.get('attribute')" which is also used in other places in 
>>> ipalib
>> I don't think this approach would be beneficial. If any of rid_base,
>> secondary_rid_base, base_id, e.g. would be set to 0, the expression like
>>
>> /entry_attrs.get('base_id')
>>
>> /would be evaluated as False both in case that there is no key 'base_id'
>> in the dictionary and in the case that the value associated with the key
>> is 0. To avoid these problems, we would have to complicate conditions
>> used in if-s, and therefore make the readability worse.
>> /
>> /
>>> 4) I see a suspicions check
>>>
>>> +            if (is_set('ipasecondarybaserid') != 
>>> is_set('ipabaserid')):
>>>
>>> I though that ipasecondarybaserid is optional. With your change it 
>>> is not:
>>>
>>> # ipa idrange-add range9 --base-id=1000 --rid-base=1000 --range-size 
>>> 1000
>>> ipa: ERROR: invalid 'ID Range setup': Options secondary_rid_base and 
>>> rid_base
>>> must be used together
>>>
>>> It is also quite ugly condition, I would do something like:
>>>
I see I forgot to react to this one. This construct is not introduced by 
this patch,
and anyway, I personally like it, because it is an easy way of 
expressing that the
condition is satisfied if and only if ipabaserid and ipasecondarybaserid 
are equivalent.

>>> if entry_attrs.get('ipasecondarybaserid') and not 
>>> entry_attrs.get('ipabaserid'):
>>> ... raise error
>> It's not a bug. It's a feature :)
>>
>> Secondary base RID indeed is mandatory when primary RID base has been 
>> defined.
>> Its purpose is to prevent collisions when user and group share the 
>> same POSIX ID number.
>>
>> From the documentation of idrange.py:
>>
>> /To create an ID range for the local domain it is not necessary to 
>> specify a//
>> //domain SID. But since it is possible that a user and a group can 
>> have the same//
>> //value as Posix ID a second RID interval is needed to handle conflicts.
>>
>> /
>>> 5) I would not create a list when it is not necessary, a tuple is more
>>> efficient I think:
>>>
>>> +            rid_range_attributes =
>>> ['ipabaserid','ipasecondarybaserid','ipaidrangesize']
>> Fixed.
>>> 6) If we want to check user does not create secondary RID range 
>>> without a
>>> primary RID range, we should also do it in -mod operation:
>>>
>>> # ipa idrange-mod range9 --rid-base=
>>> --------------------------
>>> Modified ID range "range9"
>>> --------------------------
>>>   Range name: range9
>>>   First Posix ID of the range: 1000
>>>   Number of IDs in the range: 1000
>>>   First RID of the secondary RID range: 2000
>>>   Range type: local domain range
>> This is fixed as part of my patch 0024 as it falls under the scope of
>> http://fedorahosted.org/freeipa/ticket/3170
>>
>> I will send a rebased version later today.
>>> 7) I am sorry I did not come with this in my previous review, but I 
>>> have one
>>> more nitpick for the error message:
>>> +                       error=_("Primary rid range and secondary rid 
>>> range"\
>>> +                               " cannot overlap"))
>>>
>>> I would do s/rid/RID/ as we also refer it as RID in our help...
>>>
>>> Martin
>> Fixed. However, lower-case rid is used in ipa_range_check.c 389 plugin.
>> We might want to consider filing a naming convention ticket then.
> RID is RID as it is abbreviation of Relative ID.
> See http://msdn.microsoft.com/en-us/library/cc246018.aspx for details of
> SID (and RID as it is part of SID).
>
Ok, I replaced rid range for RID range on all occasions.

Updated patch attached :)

Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0021-4-Forbid-overlapping-rid-ranges-for-the-same-id-range.patch
Type: text/x-patch
Size: 11761 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20121214/0b1db937/attachment.bin>