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

Mon Dec 17 14:57:57 UTC 2012

On 12/14/2012 02:49 PM, Tomas Babej wrote:
> 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

Ok, this looks good. I just removed extraneous parens in "if (...):" as we
agreed to.

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

Martin